mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 16:02:50 +00:00
fix(native-chat): stop a text-free first message burning the naming attempt
Both providers claimed namingAttempted before deriving the prompt text, so a caption-free screenshot as the first message left the chat on its placeholder for the session's whole life. Claim the flag after the text is in hand; the prompt reader is now total by construction, so deriving it before the promise cannot fail a delivered message. Also reattaches the JSDoc the max-lines extraction left on the wrapper, and softens a 'never titled on its own' claim to what was actually observed.
This commit is contained in:
@@ -46,6 +46,33 @@ describe('startClaudeConversationNaming', () => {
|
||||
expect(onConversationName).toHaveBeenCalledExactlyOnceWith(SESSION, 'Lease probe flake')
|
||||
})
|
||||
|
||||
it('leaves the attempt unspent when the first message carries no text', async () => {
|
||||
const generateSessionTitle = vi.fn(async () => ({
|
||||
outcome: 'named' as const,
|
||||
title: 'Lease probe flake'
|
||||
}))
|
||||
const session = sessionWith(generateSessionTitle)
|
||||
const imageOnly = {
|
||||
kind: 'message',
|
||||
role: 'user',
|
||||
blocks: [{ type: 'image', path: '/tmp/shot.png' }]
|
||||
} as unknown as AgentJournalMessageItem
|
||||
|
||||
startClaudeConversationNaming(SESSION, session, imageOnly, { onConversationName: vi.fn() })
|
||||
await settle()
|
||||
|
||||
expect(generateSessionTitle).not.toHaveBeenCalled()
|
||||
// The conversation must stay nameable: a caption-free screenshot is not an
|
||||
// answer, so the next message with text still gets to ask.
|
||||
expect(session.namingAttempted).toBe(false)
|
||||
|
||||
const onConversationName = vi.fn()
|
||||
startClaudeConversationNaming(SESSION, session, USER_TURN, { onConversationName })
|
||||
await settle()
|
||||
|
||||
expect(onConversationName).toHaveBeenCalledExactlyOnceWith(SESSION, 'Lease probe flake')
|
||||
})
|
||||
|
||||
it('passes the user text alone, imposing no title style of its own', async () => {
|
||||
const generateSessionTitle = vi.fn(async () => ({
|
||||
outcome: 'named' as const,
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
// Naming a Claude conversation.
|
||||
//
|
||||
// Claude's stream-json protocol carries no title frame, and the CLI's own
|
||||
// auto-titling lives in its interactive UI: a session driven over stream-json is
|
||||
// never titled on its own. The Agent SDK exposes the request directly, so Orca
|
||||
// auto-titling does not fire in practice for a session driven over stream-json
|
||||
// on current builds. The Agent SDK exposes the request directly, so Orca
|
||||
// asks once, and `persist` makes the CLI write the answer into its transcript as
|
||||
// the `ai-title` record a later attach reads back.
|
||||
//
|
||||
@@ -31,9 +31,10 @@ export type ClaudeConversationNamingDeps = {
|
||||
/**
|
||||
* Names the session once, off the turn's critical path.
|
||||
*
|
||||
* Asked at most once per CONVERSATION rather than once per session object, and
|
||||
* every step runs inside the promise: this sits on the send path, and nothing
|
||||
* here may turn a delivered message into a reported failure.
|
||||
* Asked at most once per CONVERSATION rather than once per session object.
|
||||
* Nothing here may turn a delivered message into a reported failure: this sits
|
||||
* on the send path, so the provider call runs inside the promise and the prompt
|
||||
* reader is total by construction.
|
||||
*/
|
||||
export function startClaudeConversationNaming(
|
||||
sessionId: string,
|
||||
@@ -44,6 +45,13 @@ export function startClaudeConversationNaming(
|
||||
if (session.namingAttempted || !deps.onConversationName) {
|
||||
return
|
||||
}
|
||||
// Claimed only once there is text to name from. A caption-free screenshot as
|
||||
// the first message would otherwise spend the conversation's one attempt and
|
||||
// leave it on the placeholder for good.
|
||||
const description = agentSessionNamingPromptText(body)
|
||||
if (!description) {
|
||||
return
|
||||
}
|
||||
session.namingAttempted = true
|
||||
void Promise.resolve()
|
||||
.then(async () => {
|
||||
@@ -51,10 +59,6 @@ export function startClaudeConversationNaming(
|
||||
if (durable?.conversationName || durable?.namingAttempted) {
|
||||
return
|
||||
}
|
||||
const description = agentSessionNamingPromptText(body)
|
||||
if (!description) {
|
||||
return
|
||||
}
|
||||
const result = await session.connection.generateSessionTitle(description, {
|
||||
persist: true,
|
||||
...(deps.requestTimeoutMs ? { timeoutMs: deps.requestTimeoutMs } : {})
|
||||
|
||||
@@ -28,17 +28,6 @@ export type CodexConversationNamingInput = {
|
||||
onError?: (scope: string, error: unknown) => void
|
||||
}
|
||||
|
||||
/**
|
||||
* Names the thread once, off the turn's critical path.
|
||||
*
|
||||
* Asked at most once per CONVERSATION, not once per session object: the durable
|
||||
* marker means a thread the model declined to name, and a name a person
|
||||
* deliberately cleared, are not re-asked after an eviction or a restart.
|
||||
*
|
||||
* Everything runs inside the promise, including reading the user's text: this
|
||||
* sits on the send path, and nothing here may turn a delivered message into a
|
||||
* reported failure.
|
||||
*/
|
||||
/** Shapes the adapter's optional naming deps into a naming turn, one dispatch at a time. */
|
||||
export function startCodexConversationNamingForTurn(
|
||||
sessionId: string,
|
||||
@@ -58,21 +47,35 @@ export function startCodexConversationNamingForTurn(
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* Names the thread once, off the turn's critical path.
|
||||
*
|
||||
* Asked at most once per CONVERSATION, not once per session object: the durable
|
||||
* marker means a thread the model declined to name, and a name a person
|
||||
* deliberately cleared, are not re-asked after an eviction or a restart.
|
||||
*
|
||||
* Nothing here may turn a delivered message into a reported failure: this sits
|
||||
* on the send path, so the provider call runs inside the promise and the prompt
|
||||
* reader is total by construction.
|
||||
*/
|
||||
export function startCodexConversationNaming(input: CodexConversationNamingInput): void {
|
||||
const { session, sessionId } = input
|
||||
if (session.namingAttempted || session.conversationName || !input.onConversationName) {
|
||||
return
|
||||
}
|
||||
// Claimed only once there is text to name from. A caption-free screenshot as
|
||||
// the first message would otherwise spend the conversation's one attempt and
|
||||
// leave it on the placeholder for good.
|
||||
const prompt = agentSessionNamingPromptText(input.body)
|
||||
if (!prompt) {
|
||||
return
|
||||
}
|
||||
session.namingAttempted = true
|
||||
void Promise.resolve()
|
||||
.then(async () => {
|
||||
if (input.readNamingAttempted?.(sessionId)) {
|
||||
return
|
||||
}
|
||||
const prompt = agentSessionNamingPromptText(input.body)
|
||||
if (!prompt) {
|
||||
return
|
||||
}
|
||||
const outcome = await generateAndSetCodexConversationName({
|
||||
connection: session.connection,
|
||||
cwd: session.cwd,
|
||||
|
||||
@@ -207,10 +207,21 @@ const USER_TURN = {
|
||||
blocks: [{ type: 'text', text: 'fix the flaky lease probe' }]
|
||||
} as const
|
||||
|
||||
const IMAGE_ONLY_TURN = {
|
||||
kind: 'message',
|
||||
role: 'user',
|
||||
blocks: [{ type: 'image', path: '/tmp/shot.png' }]
|
||||
} as const
|
||||
|
||||
async function dispatchedAdapter(
|
||||
codex: ReturnType<typeof namingCodex>,
|
||||
naming: { readNamingAttempted?: () => boolean; markNamingAttempted?: () => void } = {}
|
||||
naming: {
|
||||
readNamingAttempted?: () => boolean
|
||||
markNamingAttempted?: () => void
|
||||
body?: unknown
|
||||
} = {}
|
||||
) {
|
||||
const { body = USER_TURN, ...namingDeps } = naming
|
||||
const onConversationName = vi.fn()
|
||||
const events: unknown[] = []
|
||||
const adapter = new CodexStructuredSessionAdapter({
|
||||
@@ -225,13 +236,13 @@ async function dispatchedAdapter(
|
||||
readProcessStartTime: async () => 1_700_000_000_000,
|
||||
onEvent: (event) => events.push(event),
|
||||
onConversationName,
|
||||
...naming
|
||||
...namingDeps
|
||||
})
|
||||
await adapter.acquire({ identity, fence: 7, spawnToken: 'spawn-9' })
|
||||
await adapter.dispatch({
|
||||
sessionId: SESSION,
|
||||
clientMessageId: 'client-1',
|
||||
body: USER_TURN as never,
|
||||
body: body as never,
|
||||
fence: 7
|
||||
})
|
||||
return { adapter, onConversationName, events }
|
||||
@@ -332,6 +343,36 @@ describe('Codex conversation-name generation', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('Codex naming attempt accounting', () => {
|
||||
it('leaves the attempt unspent when the first message carries no text', async () => {
|
||||
const codex = namingCodex()
|
||||
const markNamingAttempted = vi.fn()
|
||||
const { adapter, onConversationName } = await dispatchedAdapter(codex, {
|
||||
body: IMAGE_ONLY_TURN,
|
||||
markNamingAttempted
|
||||
})
|
||||
await settle()
|
||||
|
||||
const ephemeralStarts = () =>
|
||||
codex.calls.filter((call) => call.method === 'thread/start' && call.params.ephemeral === true)
|
||||
expect(ephemeralStarts()).toHaveLength(0)
|
||||
expect(markNamingAttempted).not.toHaveBeenCalled()
|
||||
|
||||
// The conversation must stay nameable: a caption-free screenshot is not an
|
||||
// answer, so the next message with text still gets to ask.
|
||||
await adapter.dispatch({
|
||||
sessionId: SESSION,
|
||||
clientMessageId: 'client-2',
|
||||
body: USER_TURN as never,
|
||||
fence: 7
|
||||
})
|
||||
await settle()
|
||||
|
||||
expect(ephemeralStarts()).toHaveLength(1)
|
||||
expect(onConversationName).toHaveBeenCalledExactlyOnceWith(SESSION, 'Fix lease probe')
|
||||
})
|
||||
})
|
||||
|
||||
describe('Codex naming-turn isolation', () => {
|
||||
/** Every event the adapter emitted for the user's session, by thread. */
|
||||
function emittedThreads(events: unknown[]): string[] {
|
||||
|
||||
@@ -14,6 +14,17 @@ export function agentSessionNamingPromptText(body: AgentJournalMessageItem): str
|
||||
// `blocks` reaches here as provider/journal data, not something the type system
|
||||
// verified: only the RPC send path runs it through a schema. A non-array here
|
||||
// would throw on the send path and turn a delivered message into a failed one.
|
||||
// The guards below cover every shape this reads; the catch makes that
|
||||
// structural, so callers may derive the text before the naming attempt is
|
||||
// claimed rather than only from inside the naming promise.
|
||||
try {
|
||||
return readNamingPromptText(body)
|
||||
} catch {
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
function readNamingPromptText(body: AgentJournalMessageItem): string | null {
|
||||
if (!Array.isArray(body?.blocks)) {
|
||||
return null
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user