mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 00:03:15 +00:00
fix(native-chat): stop the clear path wiping a name that is still set
An emptied `custom-title` returned `cleared` immediately, so a transcript holding an ai-title plus a later emptied custom title wiped the durable name and reverted the tab to its placeholder while the CLI still displayed the ai-title. The file's own stated precedence says custom outranks ai — an EMPTY custom slot falls back to the ai slot, which is what the code did before this round. Only an emptied custom slot with nothing to fall back to is a clear. It also read `customTitle ?? ''`, so an absent, null, or renamed field counted as a deliberate clear. That is the opposite of the discipline applied on the Codex side, where a reply shape the build cannot read is refused rather than acted on. Wrongly clearing destroys a name the user can still see. The only clear test stubbed the reader and exercised the reporter, so the mechanism was bound by nothing: reverting the reader left every test green. The new tests write real transcript records — set-then-emptied, emptied with no fallback, and three unreadable field shapes — and the ablation now fails four of them. The reporter's deps are mapped explicitly beside the reporter rather than handed the adapter's object by reference, which carried an `onError` key the adapter type never declared and would have dropped silently the day that object was narrowed. Also: the sub-agent approval fixture had no `itemId`, so the registry declined it and the request was still refused, just differently. It now carries one and the test asserts a durable prompt reaches the user, which is the behaviour the narrowed gate restored. And the header comment claimed the scan is not repeated; it runs on every acquisition, deliberately, because that is how a CLI-side rename reaches Orca at all.
This commit is contained in:
@@ -3,6 +3,7 @@ import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
claudeConversationNameReporterDeps,
|
||||
readClaudeTranscriptConversationName,
|
||||
reportPersistedClaudeConversationName
|
||||
} from './claude-transcript-conversation-name'
|
||||
@@ -69,6 +70,73 @@ describe('readClaudeTranscriptConversationName', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('readClaudeTranscriptConversationName clearing', () => {
|
||||
// These write REAL transcript records. The reporter tests below stub this
|
||||
// reader, so they cannot see anything it decides — including whether an
|
||||
// emptied custom title should fall back or wipe the name.
|
||||
it('falls back to the generated title when the user empties their own', async () => {
|
||||
const path = await transcript([
|
||||
{ type: 'ai-title', aiTitle: 'Lease probe flake', sessionId: 'session-1' },
|
||||
{ type: 'custom-title', customTitle: 'My own name', sessionId: 'session-1' },
|
||||
{ type: 'custom-title', customTitle: '', sessionId: 'session-1' }
|
||||
])
|
||||
|
||||
// The CLI still shows the ai-title here; reporting `cleared` would wipe a
|
||||
// name the user can see.
|
||||
await expect(readClaudeTranscriptConversationName(path)).resolves.toEqual({
|
||||
kind: 'named',
|
||||
title: 'Lease probe flake'
|
||||
})
|
||||
})
|
||||
|
||||
it('reports cleared only when nothing remains to fall back to', async () => {
|
||||
const path = await transcript([
|
||||
{ type: 'custom-title', customTitle: 'My own name', sessionId: 'session-1' },
|
||||
{ type: 'custom-title', customTitle: '', sessionId: 'session-1' }
|
||||
])
|
||||
|
||||
await expect(readClaudeTranscriptConversationName(path)).resolves.toEqual({
|
||||
kind: 'cleared'
|
||||
})
|
||||
})
|
||||
|
||||
it.each([
|
||||
['an absent field', { type: 'custom-title', sessionId: 'session-1' }],
|
||||
['a null field', { type: 'custom-title', customTitle: null, sessionId: 'session-1' }],
|
||||
['a renamed field', { type: 'custom-title', title: '', sessionId: 'session-1' }]
|
||||
])('refuses to read %s as a deliberate clear', async (_label, record) => {
|
||||
const path = await transcript([
|
||||
{ type: 'ai-title', aiTitle: 'Lease probe flake', sessionId: 'session-1' },
|
||||
record
|
||||
])
|
||||
|
||||
// Fails closed, like isCodexThreadReadablyUnnamed: a shape this build cannot
|
||||
// read is not evidence the user removed anything.
|
||||
await expect(readClaudeTranscriptConversationName(path)).resolves.toEqual({
|
||||
kind: 'named',
|
||||
title: 'Lease probe flake'
|
||||
})
|
||||
})
|
||||
|
||||
it('reports unknown, not cleared, for a tail with no title record at all', async () => {
|
||||
const path = await transcript([{ type: 'user', sessionId: 'session-1' }])
|
||||
|
||||
await expect(readClaudeTranscriptConversationName(path)).resolves.toEqual({ kind: 'unknown' })
|
||||
})
|
||||
|
||||
it('still prefers a custom title the user actually set', async () => {
|
||||
const path = await transcript([
|
||||
{ type: 'ai-title', aiTitle: 'Lease probe flake', sessionId: 'session-1' },
|
||||
{ type: 'custom-title', customTitle: 'My own name', sessionId: 'session-1' }
|
||||
])
|
||||
|
||||
await expect(readClaudeTranscriptConversationName(path)).resolves.toEqual({
|
||||
kind: 'named',
|
||||
title: 'My own name'
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe('reportPersistedClaudeConversationName', () => {
|
||||
const session = { providerSessionId: 'provider-1', claudeConfigDir: '/home/dev/.claude' }
|
||||
|
||||
@@ -199,3 +267,32 @@ describe('reportPersistedClaudeConversationName', () => {
|
||||
expect(readTranscriptConversationName).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('claudeConversationNameReporterDeps', () => {
|
||||
it('maps the adapter’s onNamingError onto the reporter’s onError', () => {
|
||||
const onNamingError = vi.fn()
|
||||
|
||||
// The adapter names this hook differently. Handing its object over whole
|
||||
// worked only by accident and would break the day it is narrowed.
|
||||
const mapped = claudeConversationNameReporterDeps({ onNamingError })
|
||||
mapped.onError?.('scope', new Error('boom'))
|
||||
|
||||
expect(onNamingError).toHaveBeenCalledWith('scope', expect.any(Error))
|
||||
})
|
||||
|
||||
it('carries the read and both name hooks through', () => {
|
||||
const readTranscriptConversationName = vi.fn(async () => ({ kind: 'unknown' as const }))
|
||||
const onConversationName = vi.fn()
|
||||
const onConversationNameCleared = vi.fn()
|
||||
|
||||
const mapped = claudeConversationNameReporterDeps({
|
||||
readTranscriptConversationName,
|
||||
onConversationName,
|
||||
onConversationNameCleared
|
||||
})
|
||||
|
||||
expect(mapped.readTranscriptConversationName).toBe(readTranscriptConversationName)
|
||||
expect(mapped.onConversationName).toBe(onConversationName)
|
||||
expect(mapped.onConversationNameCleared).toBe(onConversationNameCleared)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -8,8 +8,12 @@
|
||||
// with the attach it runs alongside, on files that reach many megabytes.
|
||||
//
|
||||
// Bounded means a title older than the tail limit is NOT found. That reads as
|
||||
// "no name yet", never as "this conversation has no name" — and once any read
|
||||
// succeeds the name is on the durable record, so the scan is not repeated.
|
||||
// "no name yet", never as "this conversation has no name".
|
||||
//
|
||||
// This runs on EVERY acquisition, deliberately: it is how a rename made in the
|
||||
// CLI reaches Orca at all, so gating it on "already named" would freeze the
|
||||
// first name forever. It both fills and clears, and clearing an already-cleared
|
||||
// record is a no-op, so the repeat costs a bounded read and nothing else.
|
||||
|
||||
import { normalizeTitleText, parseJsonObject } from '../ai-vault/session-scanner-values'
|
||||
import { claudeTranscriptTailLines } from './claude-transcript-tail-scan'
|
||||
@@ -27,14 +31,21 @@ export type ClaudeTranscriptConversationName =
|
||||
* The transcript's stored name.
|
||||
*
|
||||
* A user's own `custom-title` outranks the generated `ai-title`, matching the
|
||||
* precedence the CLI itself applies. Read newest-first, so the first record of
|
||||
* each kind is the current one — and an emptied `custom-title` is the user
|
||||
* deleting the name, which must not read the same as never having had one.
|
||||
* precedence the CLI itself applies — including when the custom slot is EMPTY,
|
||||
* which falls back to the generated name rather than reading as no name at all.
|
||||
* Only an emptied custom slot with nothing to fall back to is a clear.
|
||||
*
|
||||
* Read newest-first, so the first record of each type is the current one.
|
||||
*
|
||||
* Fails closed on a shape it cannot read: a `custom-title` whose field is absent,
|
||||
* null, or renamed by a future CLI is skipped, never taken as a deliberate clear.
|
||||
* Wrongly clearing destroys a name the user can still see in their CLI.
|
||||
*/
|
||||
export async function readClaudeTranscriptConversationName(
|
||||
transcriptPath: string
|
||||
): Promise<ClaudeTranscriptConversationName> {
|
||||
let generated: string | null = null
|
||||
let customCleared = false
|
||||
for await (const line of claudeTranscriptTailLines(transcriptPath)) {
|
||||
if (!line.includes('-title')) {
|
||||
continue
|
||||
@@ -43,15 +54,49 @@ export async function readClaudeTranscriptConversationName(
|
||||
if (!record) {
|
||||
continue
|
||||
}
|
||||
if (record.type === 'custom-title') {
|
||||
const title = normalizeTitleText(String(record.customTitle ?? ''))
|
||||
return title ? { kind: 'named', title } : { kind: 'cleared' }
|
||||
if (record.type === 'custom-title' && !customCleared) {
|
||||
if (typeof record.customTitle !== 'string') {
|
||||
continue
|
||||
}
|
||||
const title = normalizeTitleText(record.customTitle)
|
||||
if (title) {
|
||||
return { kind: 'named', title }
|
||||
}
|
||||
customCleared = true
|
||||
continue
|
||||
}
|
||||
if (record.type === 'ai-title' && !generated) {
|
||||
generated = normalizeTitleText(String(record.aiTitle ?? '')) || null
|
||||
}
|
||||
}
|
||||
return generated ? { kind: 'named', title: generated } : { kind: 'unknown' }
|
||||
if (generated) {
|
||||
return { kind: 'named', title: generated }
|
||||
}
|
||||
return customCleared ? { kind: 'cleared' } : { kind: 'unknown' }
|
||||
}
|
||||
|
||||
/** The adapter's own dep bag, which names this reporter's error hook differently.
|
||||
* Mapped rather than passed by reference: an undeclared key survives only while
|
||||
* the object happens to be handed over whole, and typecheck cannot see it go. */
|
||||
export type ClaudeConversationNameReporterSource = ClaudeConversationNameReporter & {
|
||||
onNamingError?: (scope: string, error: unknown) => void
|
||||
}
|
||||
|
||||
export function claudeConversationNameReporterDeps(
|
||||
source: ClaudeConversationNameReporterSource
|
||||
): ClaudeConversationNameReporter {
|
||||
return {
|
||||
...(source.readTranscriptConversationName
|
||||
? { readTranscriptConversationName: source.readTranscriptConversationName }
|
||||
: {}),
|
||||
...(source.onConversationName ? { onConversationName: source.onConversationName } : {}),
|
||||
...(source.onConversationNameCleared
|
||||
? { onConversationNameCleared: source.onConversationNameCleared }
|
||||
: {}),
|
||||
...((source.onError ?? source.onNamingError)
|
||||
? { onError: (source.onError ?? source.onNamingError)! }
|
||||
: {})
|
||||
}
|
||||
}
|
||||
|
||||
export type ClaudeConversationNameReporter = {
|
||||
@@ -75,8 +120,9 @@ export function reportPersistedClaudeConversationName(
|
||||
session:
|
||||
| { providerSessionId: string; claudeConfigDir: string; namingAttempted?: boolean }
|
||||
| undefined,
|
||||
deps: ClaudeConversationNameReporter
|
||||
source: ClaudeConversationNameReporterSource
|
||||
): void {
|
||||
const deps = claudeConversationNameReporterDeps(source)
|
||||
const read = deps.readTranscriptConversationName
|
||||
if (!session || !read || !deps.onConversationName) {
|
||||
return
|
||||
|
||||
@@ -467,19 +467,20 @@ describe('Codex sub-agent threads survive the naming window', () => {
|
||||
codex.connections[0]!.handlers.onServerRequest?.({
|
||||
id: 91,
|
||||
method: 'item/commandExecution/requestApproval',
|
||||
params: { threadId: SUBAGENT_THREAD, command: 'pnpm test' }
|
||||
// `itemId` is what makes this a durable PROMPT rather than a request the
|
||||
// registry declines; without it the assertion below would pass on a
|
||||
// different refusal and prove nothing about reaching the user.
|
||||
params: { threadId: SUBAGENT_THREAD, itemId: 'item-subagent-1', command: 'pnpm test' }
|
||||
})
|
||||
await settle()
|
||||
|
||||
// Auto-refusing here denies a tool the user's OWN agent asked to run, with a
|
||||
// reason that is untrue. Asserted on the MESSAGE: -32001 is also the code the
|
||||
// normal prompt path uses when a journal admission fails, so the code alone
|
||||
// would not tell the two apart.
|
||||
const refusals = codex.replies.filter((reply) =>
|
||||
String(reply.message ?? '').includes('conversation-naming turn')
|
||||
)
|
||||
expect(refusals).toEqual([])
|
||||
expect(emittedThreads(events)).toContain(SUBAGENT_THREAD)
|
||||
expect(codex.replies).toEqual([])
|
||||
// It reaches the user as a prompt, which is the behaviour the narrowed gate
|
||||
// restored — not merely "some event was emitted".
|
||||
const prompts = events.filter((event) => (event as { type?: string }).type === 'prompt')
|
||||
expect(prompts).toEqual([
|
||||
expect.objectContaining({ threadId: SUBAGENT_THREAD, codexItemId: 'item-subagent-1' })
|
||||
])
|
||||
})
|
||||
|
||||
it('still keeps the naming thread out once its id is known', async () => {
|
||||
|
||||
@@ -93,7 +93,6 @@ export function createStructuredClaudeRuntimeAdapter(
|
||||
...(deps.onConversationNameCleared
|
||||
? { onConversationNameCleared: deps.onConversationNameCleared }
|
||||
: {}),
|
||||
...(deps.onNamingError ? { onError: deps.onNamingError } : {}),
|
||||
readTranscriptConversationName: async ({ providerSessionId, claudeConfigDir }) => {
|
||||
const transcriptPath = await resolveSessionFilePath('claude', providerSessionId, {
|
||||
claudeProjectsDir: join(claudeConfigDir, 'projects')
|
||||
|
||||
Reference in New Issue
Block a user