From 08bb67221b472674bc776ec4b27bd3db652a9735 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 8 Sep 2026 10:17:53 +0000 Subject: [PATCH 01/11] feat(cli): let builtin Codex own its ACP session title acp-extension-codex >= 1.8.0 generates its own title on an ephemeral thread and publishes it as session_info_update tagged _meta.lody.titleSource: 'explicit'. Lody still spawned an isolated codex ACP session to generate a second title for it, so drop that duplicate work and hide the now-unreachable title-generation config for Codex. Split the single usesAcpProvidedSessionTitle() predicate, because the two ACP-owning adapters need opposite trust rules: - acpOwnsSessionTitleGeneration() (claude + codex) skips the isolated generator and hides the title config. - trustsUntaggedAcpSessionTitle() (claude only) gates the agent-client trust check. Codex must stay out of it: it emits a first-prompt `fallback` preview before its generated `explicit` title, and trusting untagged titles would promote that preview to the session title. An audit of all five builtin adapters found only Claude and Codex generate titles over ACP. Grok has no title code, Kimi's session_info_update carries the first prompt truncated to 200 chars with no _meta, and the DeepSeek Harness never mounts its upstream dsh-session-title plugin, so all three keep the isolated generator and its titleGeneration config. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 87 +++++++++++++++++++ apps/cli/src/agent/AGENTS.md | 15 ++-- apps/cli/src/agent/README.md | 21 +++-- apps/cli/src/agent/agent-client.ts | 6 +- apps/cli/src/lib/message-handler.ts | 7 +- apps/cli/tests/message-handler-title.test.ts | 44 ++++++++-- .../settings/agent-config-dialog.tsx | 4 +- packages/shared/src/ai.ts | 30 ++++++- .../tests/title-generation-defaults.test.ts | 42 +++++++-- 9 files changed, 218 insertions(+), 38 deletions(-) create mode 100644 .agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md new file mode 100644 index 000000000..adbd848e4 --- /dev/null +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -0,0 +1,87 @@ +# Let Codex own its session title, and split the ACP title predicates + +Status: implemented +Translation: pending + +## Abstract + +Lody wanted every builtin agent to stop carrying its own `titleGeneration` +session config and take the session title from the ACP adapter instead. An audit +of all five builtin adapters shows that only acp-extension-claude and +acp-extension-codex actually generate titles; Grok, Kimi and the DeepSeek Harness +publish nothing usable, so the isolated generator and its config must stay for +them. Codex was the one adapter doing the work twice — Lody spawned an isolated +codex ACP session for the title while the adapter independently generated and +pushed its own — so Codex now joins Claude as an ACP-owned title provider. Doing +that required splitting the single `usesAcpProvidedSessionTitle()` predicate, +because the two adapters need opposite trust rules and conflating them would have +promoted Codex's first-prompt preview to the session title. Branch naming still +falls back to the isolated generator when no ACP title has landed yet, so this +reduces title work but does not yet eliminate the isolated session. + +## The audit + +Each adapter was read at the commit this repository pins. + +| Adapter | Version | Publishes a title | `_meta.lody.titleSource` | Real generation | +| --- | --- | --- | --- | --- | +| `acp-extension-claude` | 0.70.0 | yes | no `_meta` at all | yes — SDK `generate_session_title` control request | +| `acp-extension-codex` | 1.10.0 (since 1.8.0) | yes | yes, generated titles are `explicit` | yes — cheap-model turn on an ephemeral thread | +| `acp-extension-grok` | 0.1.0 | no | no | no | +| `acp-extension-kimi` | acp-server 0.0.1 | yes, but the title is the first prompt truncated to 200 chars | no `_meta` at all | no | +| `acp-extension-dsh` | 0.1.1 | no | no | no | + +Two near-misses are worth recording because they change what "add title support" +would cost later. Kimi's engine already tracks +`SessionTitleKind = 'replaceable' | 'generated' | 'custom'`, and it has a real +generator (`SessionTitleService`, backed by Moonshot's managed `chat_title` +endpoint) — but the generator is reachable only from the kap-server HTTP route and +the node SDK, and the kind is discarded at the ACP boundary in +`packages/acp-server/src/events-map.ts`. The DeepSeek Harness pins +`@deepseek-ai/dsh-session-title` in its dependency closure but never mounts it in +`createDeepSeekHarnessCordisConfig`, so the plugin is inert. Grok is a pure stdio +proxy with no title code of any kind and no upstream title capability to forward. + +## Decision + +`usesAcpProvidedSessionTitle()` answered two different questions at three call +sites, and Codex needs opposite answers to them: + +- *May Lody skip its isolated generator and hide the title config?* Yes for + Claude and Codex. This is now `acpOwnsSessionTitleGeneration()`. +- *May Lody trust a pushed title that carries no `titleSource`?* Only for Claude, + which sends a bare `session_info_update`. This is now + `trustsUntaggedAcpSessionTitle()`. + +Codex must stay out of the second predicate. It emits a `fallback` prompt-preview +title before its generated `explicit` one, and `apps/cli/src/agent/AGENTS.md` +already required rejecting that preview. Extending the original single predicate +to Codex would have silently made the raw first prompt the session title — the +main trap this split exists to prevent. + +The `titleGeneration` config surface (schema field, settings section, CLI flags) +is deliberately left in place. Removing it would strip the cheap-model and +least-privilege-mode selection that Grok, Kimi, DeepSeek Harness, registry and +custom providers still rely on. The config simply stops being reachable for +Codex, as it already was for Claude. + +## Trade-offs and limits + +Codex generates its title after the first turn completes and skips generation +entirely on resumed sessions (its internal source is `unknown` there), so a +resumed codex session no longer gets a Lody-generated title. Generation is also +best-effort inside the adapter and swallows failures without signalling the +client, so a failed generation now leaves the draft title rather than falling +back to Lody's generator. + +This change does not reach zero isolated ACP sessions. `generateBranchNameWithTimeout` +reuses an in-flight or already-stored generated title, and otherwise still calls +`generateTitleIsolated()`; with the title path skipped, Codex now takes that +branch as Claude already did. Making branch naming wait for the ACP-pushed title +within its existing 20s budget is the remaining step, and is not attempted here +because the timeout path currently abandons the rename rather than falling back +to the prompt text — changing that affects every provider, not just the +ACP-owned ones. + +Verification is type checks, lint, and the shared unit tests covering both +predicates. No live codex, Kimi, Grok or DeepSeek session was exercised. diff --git a/apps/cli/src/agent/AGENTS.md b/apps/cli/src/agent/AGENTS.md index 6cc6c8806..3e5aec5dd 100644 --- a/apps/cli/src/agent/AGENTS.md +++ b/apps/cli/src/agent/AGENTS.md @@ -105,10 +105,11 @@ context/acp-agent-edit-evidence.md; adapter repos: [apps/cli/AGENTS.md](../../AG the cache, and requests/responses carry that id to keep configs of one provider isolated. `ManagedRuntimeUpdateCoordinator` never hot-swaps a running ACP process, and Machine Flock writes ignore `fetchedAt` when comparing entries. -- Builtin Claude owns session titles through ACP `session_info_update`; store them only after - `sanitizeLodyInternalInstructions`, and never start `title-generator.ts`'s isolated session - for Claude. For Codex accept only `explicit` `_meta.lody.titleSource` names, ignore its - first-prompt `fallback`, and require `_meta.lody.messagePhase === 'final_answer'`; untyped - chunks, error/warning payloads, and internal-instruction tails are never candidates. - Each isolated run owns and removes a unique temp directory; concurrent session-title and - branch-name work reuses one in-flight result. +- Builtin Claude and Codex own session titles via ACP `session_info_update` + (`acpOwnsSessionTitleGeneration()`): store them only after + `sanitizeLodyInternalInstructions` and never start `title-generator.ts`'s isolated session + for them. Only Claude is trusted untagged (`trustsUntaggedAcpSessionTitle()`); for Codex + take only `explicit` `_meta.lody.titleSource`, ignore its first-prompt `fallback`, + and require `_meta.lody.messagePhase === 'final_answer'`; untyped chunks, error/warning + payloads, and internal-instruction tails never qualify. Each isolated run owns and removes + a temp dir; session-title and branch-name work share one in-flight result. diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index dcf6fa254..b48555149 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -178,8 +178,19 @@ is a context window). Vendor model `_meta` never enters the CLI. ### Session titles -Builtin Claude owns session title generation through ACP `session_info_update`. Builtin Codex -still uses the isolated generator in `title-generator.ts`, but its adapter tags every pushed -title with `_meta.lody.titleSource`. Other providers use `title-generator.ts` / -`response-utils.ts`. The shared `usesAcpProvidedSessionTitle()` predicate hides obsolete -provider title settings only for Claude. +Builtin Claude and Codex own session title generation through ACP `session_info_update`: +acp-extension-claude asks the Agent SDK for a title via its `generate_session_title` control +request, and acp-extension-codex (>= 1.8.0) runs a cheap-model turn on an ephemeral thread and +persists the result as the codex thread name. The shared `acpOwnsSessionTitleGeneration()` +predicate keeps `title-generator.ts`'s isolated session out of their title path and hides the +obsolete provider title settings for both. + +The two adapters differ in how they label a title, so the trust gate is separate. Claude sends +a bare `session_info_update` with no `_meta`, so it needs the `trustsUntaggedAcpSessionTitle()` +allowlist. Codex tags every title and emits a first-prompt `fallback` preview before its +generated `explicit` one, so it must stay outside that allowlist or the preview would win. + +Grok, Kimi and the DeepSeek Harness publish no usable ACP title — Grok has no title code at +all, Kimi's `session_info_update` carries the first prompt truncated to 200 chars with no +`_meta`, and the DeepSeek Harness never mounts its upstream `dsh-session-title` plugin — so +they keep using `title-generator.ts` / `response-utils.ts` and the `titleGeneration` config. diff --git a/apps/cli/src/agent/agent-client.ts b/apps/cli/src/agent/agent-client.ts index 1a86f5c99..6d9eefd93 100644 --- a/apps/cli/src/agent/agent-client.ts +++ b/apps/cli/src/agent/agent-client.ts @@ -26,7 +26,7 @@ import { type SessionGoalContent, type SessionTurnInputConfig, sanitizeGoalObjective, - usesAcpProvidedSessionTitle, + trustsUntaggedAcpSessionTitle, parseSessionNotification, SessionContextWindowUsage, SessionId, @@ -1542,7 +1542,7 @@ export class AgentClient implements acp.Client { return; } - const ownsTitleGeneration = usesAcpProvidedSessionTitle( + const trustsUntaggedTitle = trustsUntaggedAcpSessionTitle( this.options.agentConfig?.cliType, this.options.agentConfig?.agentType ); @@ -1556,7 +1556,7 @@ export class AgentClient implements acp.Client { (lodyTitleMeta.success && lodyTitleMeta.data.titleSource === 'explicit') || (legacyCodexTitleMeta?.success === true && legacyCodexTitleMeta.data.titleSource === 'explicit'); - if (!ownsTitleGeneration && !isExplicitProviderTitle) { + if (!trustsUntaggedTitle && !isExplicitProviderTitle) { return; } diff --git a/apps/cli/src/lib/message-handler.ts b/apps/cli/src/lib/message-handler.ts index 64ed7ca2e..785a6c149 100644 --- a/apps/cli/src/lib/message-handler.ts +++ b/apps/cli/src/lib/message-handler.ts @@ -39,7 +39,7 @@ import { type TitleGenerationConfig, isManagedBuiltinAgentType, sanitizeLodyInternalInstructions, - usesAcpProvidedSessionTitle, + acpOwnsSessionTitleGeneration, SessionCreateResponse, SessionChatResponse, SessionStatusFactory, @@ -8980,8 +8980,9 @@ export class MessageHandler { runtimeOverrides?: BuiltinRuntimeOverrides, titleConfig?: TitleGenerationConfig ): Promise { - // Builtin Claude publishes a generated session_info_update title. - if (usesAcpProvidedSessionTitle(cliType, agentType)) { + // Builtin Claude and Codex generate their own titles and publish them as + // session_info_update; starting the isolated agent would only duplicate them. + if (acpOwnsSessionTitleGeneration(cliType, agentType)) { return; } const existingGeneration = this.titleGenerationInFlight.get(sessionId); diff --git a/apps/cli/tests/message-handler-title.test.ts b/apps/cli/tests/message-handler-title.test.ts index 03106773f..729973bb5 100644 --- a/apps/cli/tests/message-handler-title.test.ts +++ b/apps/cli/tests/message-handler-title.test.ts @@ -127,7 +127,7 @@ describe('MessageHandler title generation', () => { expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); }); - it('runs isolated generation for Codex when title is missing', async () => { + it('runs isolated generation for Kimi when title is missing', async () => { const { handler, sessionDoc } = await createHandler(undefined); const titleHost = handler as unknown as { @@ -141,7 +141,7 @@ describe('MessageHandler title generation', () => { await titleHost.maybeGenerateAndStoreSessionTitle( 's-2' as SessionId, 'builtin', - 'codex', + 'kimi', 'Do something cool' ); @@ -172,13 +172,13 @@ describe('MessageHandler title generation', () => { const first = titleHost.maybeGenerateAndStoreSessionTitle( 's-shared' as SessionId, 'builtin', - 'codex', + 'kimi', 'Fix title races' ); const second = titleHost.maybeGenerateAndStoreSessionTitle( 's-shared' as SessionId, 'builtin', - 'codex', + 'kimi', 'Fix title races' ); await vi.waitFor(() => expect(mockedGenerateTitleIsolated).toHaveBeenCalledTimes(1)); @@ -296,9 +296,9 @@ describe('MessageHandler title generation', () => { expect(sessionDoc.setTitle).not.toHaveBeenCalled(); }); - it('applies Codex titleGeneration overrides when no titleConfig is passed', async () => { + it('applies Kimi titleGeneration overrides when no titleConfig is passed', async () => { const titleGeneration = { - configOptionValues: { model: 'gpt-5.1-codex', reasoning_effort: 'low' }, + configOptionValues: { model: 'kimi-k2-turbo', reasoning_effort: 'low' }, }; const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { agentConfigId: 'agent-config-1', @@ -316,7 +316,7 @@ describe('MessageHandler title generation', () => { await titleHost.maybeGenerateAndStoreSessionTitle( 's-6' as SessionId, 'builtin', - 'codex', + 'kimi', 'Do something cool' ); @@ -351,6 +351,36 @@ describe('MessageHandler title generation', () => { expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); }); + // acp-extension-codex >= 1.8.0 generates its own title on an ephemeral thread + // and pushes it as session_info_update, so the isolated generator would only + // duplicate that work — and would not read the agent config to do it. + it('skips isolated generation for builtin Codex', async () => { + const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { + agentConfigId: 'agent-config-1', + agentConfigMeta: { + titleGeneration: { configOptionValues: { model: 'gpt-5.1-codex' } }, + }, + }); + + const titleHost = handler as unknown as { + maybeGenerateAndStoreSessionTitle: ( + sessionId: SessionId, + cliType: string, + agentType: string, + taskPrompt: string + ) => Promise; + }; + await titleHost.maybeGenerateAndStoreSessionTitle( + 's-codex-acp' as SessionId, + 'builtin', + 'codex', + 'Do something cool' + ); + + expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); + expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); + }); + it('filters Lody internal prompt instructions before storing an ACP title', async () => { const { handler, sessionDoc } = await createHandler(undefined); const titleHost = handler as unknown as { diff --git a/packages/components/src/components/settings/agent-config-dialog.tsx b/packages/components/src/components/settings/agent-config-dialog.tsx index 44f95733f..2ac8b00a3 100644 --- a/packages/components/src/components/settings/agent-config-dialog.tsx +++ b/packages/components/src/components/settings/agent-config-dialog.tsx @@ -20,7 +20,7 @@ import { machineSupportsAcpProtocolAuthentication, supportsBuiltinAuthentication, usesAcpProtocolAuthentication, - usesAcpProvidedSessionTitle, + acpOwnsSessionTitleGeneration, REGISTRY_ACP_AGENTS, type AgentBrandId, type AgentConfigCliType, @@ -1014,7 +1014,7 @@ export function AgentConfigDialog(props: AgentConfigDialogProps) { const activePreset = formData.presetId ? PRESETS_BY_ID[formData.presetId] : undefined; const isPreset = !!activePreset; - const acpProvidesSessionTitle = usesAcpProvidedSessionTitle(formData.cliType, formData.agentType); + const acpProvidesSessionTitle = acpOwnsSessionTitleGeneration(formData.cliType, formData.agentType); const activeCredentialMode = activePreset ? getPresetCredentialMode(activePreset, formData.presetCredentialModeId) : undefined; diff --git a/packages/shared/src/ai.ts b/packages/shared/src/ai.ts index be41b124e..ae5248100 100644 --- a/packages/shared/src/ai.ts +++ b/packages/shared/src/ai.ts @@ -40,8 +40,34 @@ export type BuiltinAgentType = BuiltinAgent['agentType']; export type AgentConfigCliType = 'builtin' | 'registry' | 'custom'; export type AgentType = string; -/** Builtin ACP adapters that publish their own session titles. */ -export const usesAcpProvidedSessionTitle = ( +/** + * Builtin ACP adapters that generate their own session titles, so Lody never + * starts its isolated title agent for them and hides the title-generation + * config from their agent settings. + * + * - `claude`: acp-extension-claude asks the Agent SDK for a real title via the + * `generate_session_title` control request and publishes it at turn end. + * - `codex`: acp-extension-codex (>= 1.8.0) runs its own cheap-model generation + * on an ephemeral thread, persists it as the codex thread name, and publishes + * it tagged `_meta.lody.titleSource: 'explicit'`. + * + * Grok, Kimi and the DeepSeek Harness have no ACP title generation at all and + * still depend on the isolated generator. + */ +export const acpOwnsSessionTitleGeneration = ( + cliType: AgentConfigCliType | null | undefined, + agentType: AgentType | null | undefined +): boolean => cliType === 'builtin' && (agentType === 'claude' || agentType === 'codex'); + +/** + * Adapters whose pushed titles are authoritative without a `titleSource` tag. + * + * Deliberately narrower than {@link acpOwnsSessionTitleGeneration}: only + * acp-extension-claude omits `_meta.lody.titleSource`, so it needs an allowlist. + * Codex tags every title, and its first-prompt `fallback` preview must stay + * rejected — widening this predicate would turn that preview into the title. + */ +export const trustsUntaggedAcpSessionTitle = ( cliType: AgentConfigCliType | null | undefined, agentType: AgentType | null | undefined ): boolean => cliType === 'builtin' && agentType === 'claude'; diff --git a/packages/shared/tests/title-generation-defaults.test.ts b/packages/shared/tests/title-generation-defaults.test.ts index 582fed676..4fef0b237 100644 --- a/packages/shared/tests/title-generation-defaults.test.ts +++ b/packages/shared/tests/title-generation-defaults.test.ts @@ -2,7 +2,8 @@ import { describe, expect, it } from 'vitest'; import { computeTitleGenerationDefaults, getBuiltinTitleGenerationDefaults, - usesAcpProvidedSessionTitle, + acpOwnsSessionTitleGeneration, + trustsUntaggedAcpSessionTitle, type AcpConfigOptionSummary, } from '../src/ai'; @@ -118,15 +119,38 @@ describe('getBuiltinTitleGenerationDefaults', () => { }); }); -describe('usesAcpProvidedSessionTitle', () => { - it('uses the builtin Claude ACP title', () => { - expect(usesAcpProvidedSessionTitle('builtin', 'claude')).toBe(true); +describe('acpOwnsSessionTitleGeneration', () => { + it('lets the builtin Claude and Codex adapters generate their own titles', () => { + expect(acpOwnsSessionTitleGeneration('builtin', 'claude')).toBe(true); + expect(acpOwnsSessionTitleGeneration('builtin', 'codex')).toBe(true); }); - it('keeps isolated title generation for other providers', () => { - expect(usesAcpProvidedSessionTitle('builtin', 'codex')).toBe(false); - expect(usesAcpProvidedSessionTitle('builtin', 'kimi')).toBe(false); - expect(usesAcpProvidedSessionTitle('registry', 'codex')).toBe(false); - expect(usesAcpProvidedSessionTitle('custom', 'claude')).toBe(false); + it('keeps isolated title generation for adapters without ACP title support', () => { + expect(acpOwnsSessionTitleGeneration('builtin', 'kimi')).toBe(false); + expect(acpOwnsSessionTitleGeneration('builtin', 'grok')).toBe(false); + expect(acpOwnsSessionTitleGeneration('builtin', 'deepseek')).toBe(false); + }); + + it('never applies to registry or custom providers', () => { + expect(acpOwnsSessionTitleGeneration('registry', 'codex')).toBe(false); + expect(acpOwnsSessionTitleGeneration('custom', 'claude')).toBe(false); + }); +}); + +describe('trustsUntaggedAcpSessionTitle', () => { + it('trusts builtin Claude, which publishes titles without a titleSource tag', () => { + expect(trustsUntaggedAcpSessionTitle('builtin', 'claude')).toBe(true); + }); + + // Codex tags every title and emits a first-prompt `fallback` preview before + // its generated `explicit` title. Trusting untagged titles here would promote + // that preview to the session title. + it('does not trust Codex titles that lack an explicit titleSource', () => { + expect(trustsUntaggedAcpSessionTitle('builtin', 'codex')).toBe(false); + }); + + it('never applies to registry or custom providers', () => { + expect(trustsUntaggedAcpSessionTitle('registry', 'claude')).toBe(false); + expect(trustsUntaggedAcpSessionTitle('custom', 'claude')).toBe(false); }); }); From 5e9cf08f1c8f1af7aea134fdf050fc73f99942e8 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 8 Sep 2026 10:30:32 +0000 Subject: [PATCH 02/11] fix(cli): stop stale title config steering ACP-owned branch names Claude and Codex no longer expose a title-generation config, but branch naming still resolved the persisted titleGeneration for them, so a value stored before that change kept steering their isolated runs after the setting disappeared from the UI. Skip the lookup for ACP-owned agents and let computeTitleGenerationDefaults() pick from the live configOptions. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 7 +++ apps/cli/src/lib/message-handler.ts | 13 +++- apps/cli/tests/message-handler-title.test.ts | 60 +++++++++++++++++++ 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index adbd848e4..a98d6169b 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -65,6 +65,13 @@ least-privilege-mode selection that Grok, Kimi, DeepSeek Harness, registry and custom providers still rely on. The config simply stops being reachable for Codex, as it already was for Claude. +"Unreachable" has to hold on every path, not just the settings form. Branch +naming resolved the persisted `titleGeneration` for whatever agent it was naming +a branch for, so a value stored before this change would have kept steering +Claude and Codex runs after their config disappeared from the UI. Branch naming +now skips that lookup for ACP-owned agents and lets +`computeTitleGenerationDefaults()` pick from the live `configOptions` instead. + ## Trade-offs and limits Codex generates its title after the first turn completes and skips generation diff --git a/apps/cli/src/lib/message-handler.ts b/apps/cli/src/lib/message-handler.ts index 785a6c149..9e2180bcf 100644 --- a/apps/cli/src/lib/message-handler.ts +++ b/apps/cli/src/lib/message-handler.ts @@ -9716,8 +9716,17 @@ export class MessageHandler { ); } - const resolvedTitleConfig = - titleConfig ?? (await this.resolveTitleConfig(sessionId, metaAgentConfigId)); + // Claude and Codex no longer expose a title-generation config, so a value + // persisted before that must not keep steering their runs here. They still + // reach the isolated generator when no ACP title has landed yet, and it + // falls back to computeTitleGenerationDefaults() for them. + // Claude and Codex no longer expose a title-generation config, so a value + // persisted before that must not keep steering their runs here. They still + // reach the isolated generator when no ACP title has landed yet, and it + // falls back to computeTitleGenerationDefaults() for them. + const resolvedTitleConfig = acpOwnsSessionTitleGeneration(cliType, agentType) + ? undefined + : (titleConfig ?? (await this.resolveTitleConfig(sessionId, metaAgentConfigId))); const branchName = await this.generateBranchNameWithTimeout( cliType, agentType, diff --git a/apps/cli/tests/message-handler-title.test.ts b/apps/cli/tests/message-handler-title.test.ts index 729973bb5..7b8a6a17e 100644 --- a/apps/cli/tests/message-handler-title.test.ts +++ b/apps/cli/tests/message-handler-title.test.ts @@ -102,6 +102,32 @@ const createHandler = async ( return { handler, sessionDoc, workspaceDocument }; }; +// Drives the branch-rename path far enough to observe which titleConfig reaches +// the isolated generator. `exec` reports no branch, so the rename itself is a +// no-op and no git command runs. +const renameBranch = async (handler: MessageHandler, agentType: string): Promise => { + const branchHost = handler as unknown as { + maybeRenameSessionBranchFromPrompt: ( + sessionId: SessionId, + session: unknown, + cliType: string, + agentType: string, + taskPrompt: string + ) => Promise; + }; + const session = { + getWorkdir: () => '/tmp/lody-branch-test', + exec: async () => ({ stdout: '', stderr: '', exitCode: 1 }), + }; + await branchHost.maybeRenameSessionBranchFromPrompt( + 's-branch' as SessionId, + session, + 'builtin', + agentType, + 'Fix the flaky login redirect' + ); +}; + describe('MessageHandler title generation', () => { beforeEach(() => { mockedGenerateTitleIsolated.mockClear(); @@ -221,6 +247,40 @@ describe('MessageHandler title generation', () => { expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); }); + // Claude and Codex no longer expose a title-generation config, so a value + // persisted before that must not keep steering their branch-name runs. + it.each(['claude', 'codex'])( + 'ignores a stale persisted titleGeneration when naming a branch for %s', + async (agentType) => { + const { handler } = await createHandler(undefined, undefined, undefined, { + agentConfigId: 'agent-config-1', + agentConfigMeta: { titleGeneration: { configOptionValues: { model: 'stale-model' } } }, + }); + + await renameBranch(handler, agentType); + + expect(mockedGenerateTitleIsolated).toHaveBeenCalledTimes(1); + expect(mockedGenerateTitleIsolated).toHaveBeenCalledWith( + expect.objectContaining({ titleConfig: undefined }) + ); + } + ); + + it('still applies the persisted titleGeneration when naming a branch for Kimi', async () => { + const titleGeneration = { configOptionValues: { model: 'kimi-k2-turbo' } }; + const { handler } = await createHandler(undefined, undefined, undefined, { + agentConfigId: 'agent-config-1', + agentConfigMeta: { titleGeneration }, + }); + + await renameBranch(handler, 'kimi'); + + expect(mockedGenerateTitleIsolated).toHaveBeenCalledTimes(1); + expect(mockedGenerateTitleIsolated).toHaveBeenCalledWith( + expect.objectContaining({ titleConfig: titleGeneration }) + ); + }); + it('keeps skipping isolated generation when an existing title has no draft source', async () => { const prompt = 'Do something cool'; const placeholder = prompt.slice(0, 50); From d2037ce61d38692b7ce5d1fe5e95c71991402c62 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 8 Sep 2026 12:59:45 +0000 Subject: [PATCH 03/11] docs(cli): correct the Grok session-title finding The earlier audit recorded that Grok has no title capability. That was read off acp-extension-grok, which is a pure stdio proxy with no title code, and it is wrong about Grok itself. The pinned @xai-official/grok 1.0.13 runtime ships a full automatic title generator: the shipped binary carries the session_title tool-call prompt, the "falling back to truncated user text" failure path, and docs for /rename and /rename --auto. The logic sits in xai-grok-shell/src/session/acp_session_impl/title_refresh.rs, inside the ACP session impl, so it is not TUI-only. Whether the title reaches the wire as a pushed session_info_update or only inside the x.ai/session/info response is still unverified: this machine has no Grok credentials and all stored sessions are zero-message capability probes. Both candidate integration points are recorded. No behavior change; Grok keeps the isolated generator either way. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 47 +++++++++++++++---- apps/cli/src/agent/README.md | 20 ++++++-- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index a98d6169b..5c49225b0 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -7,14 +7,16 @@ Translation: pending Lody wanted every builtin agent to stop carrying its own `titleGeneration` session config and take the session title from the ACP adapter instead. An audit -of all five builtin adapters shows that only acp-extension-claude and -acp-extension-codex actually generate titles; Grok, Kimi and the DeepSeek Harness -publish nothing usable, so the isolated generator and its config must stay for -them. Codex was the one adapter doing the work twice — Lody spawned an isolated +of all five builtin adapters shows only acp-extension-claude and +acp-extension-codex surface a usable title today, so the isolated generator and +its config must stay for Grok, Kimi and the DeepSeek Harness — though for all +three the gap is ours rather than a missing upstream feature, most sharply for +Grok, whose official runtime generates titles inside its own ACP session impl. +Codex was the one adapter doing the work twice, since Lody spawned an isolated codex ACP session for the title while the adapter independently generated and -pushed its own — so Codex now joins Claude as an ACP-owned title provider. Doing -that required splitting the single `usesAcpProvidedSessionTitle()` predicate, -because the two adapters need opposite trust rules and conflating them would have +pushed its own, so Codex now joins Claude as an ACP-owned title provider. That +required splitting the single `usesAcpProvidedSessionTitle()` predicate, because +the two adapters need opposite trust rules and conflating them would have promoted Codex's first-prompt preview to the session title. Branch naming still falls back to the isolated generator when no ACP title has landed yet, so this reduces title work but does not yet eliminate the isolated session. @@ -27,7 +29,7 @@ Each adapter was read at the commit this repository pins. | --- | --- | --- | --- | --- | | `acp-extension-claude` | 0.70.0 | yes | no `_meta` at all | yes — SDK `generate_session_title` control request | | `acp-extension-codex` | 1.10.0 (since 1.8.0) | yes | yes, generated titles are `explicit` | yes — cheap-model turn on an ephemeral thread | -| `acp-extension-grok` | 0.1.0 | no | no | no | +| `acp-extension-grok` | 0.1.0 | no (adapter); the official runtime does generate titles | no | not in the adapter — upstream `title_refresh.rs` does | | `acp-extension-kimi` | acp-server 0.0.1 | yes, but the title is the first prompt truncated to 200 chars | no `_meta` at all | no | | `acp-extension-dsh` | 0.1.1 | no | no | no | @@ -39,8 +41,33 @@ endpoint) — but the generator is reachable only from the kap-server HTTP route the node SDK, and the kind is discarded at the ACP boundary in `packages/acp-server/src/events-map.ts`. The DeepSeek Harness pins `@deepseek-ai/dsh-session-title` in its dependency closure but never mounts it in -`createDeepSeekHarnessCordisConfig`, so the plugin is inert. Grok is a pure stdio -proxy with no title code of any kind and no upstream title capability to forward. +`createDeepSeekHarnessCordisConfig`, so the plugin is inert. + +Grok is the sharpest correction to an earlier reading of this audit. The adapter +is a pure stdio proxy with no title code, which is easy to mistake for "Grok has +no titles". The official `@xai-official/grok` 1.0.13 runtime pinned by +`runtime-manifest.json` in fact ships a full automatic title generator: strings in +the shipped binary include the `session_title` tool-call prompt ("Final session +title, just 5-10 word descriptive title for the session"), the failure path +"session title generation failed, falling back to truncated user text", and user +documentation stating the title is generated right after the first prompt, +regenerated over a couple of early turns, then frozen, with `/rename` and +`/rename --auto` as manual overrides. Decisively, the logic lives at +`crates/codegen/xai-grok-shell/src/session/acp_session_impl/title_refresh.rs` — +inside the ACP session implementation, alongside `goal.rs`, `mcp.rs` and +`prompt_build.rs` — so it is not TUI-only, and the runtime's ACP `SessionUpdate` +enum includes `session_info_update` with `title` and `updatedAt`. + +What remains unverified is whether that title reaches the ACP wire as a pushed +`session_info_update` or only as a field of the `x.ai/session/info` response. It +could not be settled on the machine used for this audit: there are no Grok +credentials there (`~/.grok/auth.json` is absent) so no turn could be run, and all +1375 stored Grok sessions are zero-message Lody capability probes with an empty +`session_summary`. Both candidate paths are already within reach of existing code: +`handleRuntimeMethod` in `packages/acp-extension-grok/src/proxy.js` ends in a +verbatim `passthrough`, so a pushed update already flows to Lody and is dropped +only for lacking `_meta.lody.titleSource`; and the adapter already issues +`x.ai/session/info` every turn but consumes only `sessionInfo?.context`. ## Decision diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index b48555149..be1386695 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -190,7 +190,19 @@ a bare `session_info_update` with no `_meta`, so it needs the `trustsUntaggedAcp allowlist. Codex tags every title and emits a first-prompt `fallback` preview before its generated `explicit` one, so it must stay outside that allowlist or the preview would win. -Grok, Kimi and the DeepSeek Harness publish no usable ACP title — Grok has no title code at -all, Kimi's `session_info_update` carries the first prompt truncated to 200 chars with no -`_meta`, and the DeepSeek Harness never mounts its upstream `dsh-session-title` plugin — so -they keep using `title-generator.ts` / `response-utils.ts` and the `titleGeneration` config. +Grok, Kimi and the DeepSeek Harness deliver no title Lody can currently use, so they keep +using `title-generator.ts` / `response-utils.ts` and the `titleGeneration` config. In all +three cases the gap is on our side, not a missing upstream feature: + +- **Grok**: the official runtime generates titles itself, from + `xai-grok-shell/src/session/acp_session_impl/title_refresh.rs` — inside its ACP session + impl, so it runs under `grok acp`, not only in the TUI. `acp-extension-grok` adds no title + handling: an emitted `session_info_update` would reach Lody untouched through + `handleRuntimeMethod`'s default passthrough and then be dropped for lacking + `_meta.lody.titleSource`. The adapter also already calls `x.ai/session/info` every turn but + keeps only `.context`. +- **Kimi**: its `session_info_update` carries the first prompt truncated to 200 chars with no + `_meta`, while the engine's real `SessionTitleService` stays reachable only from kap-server + and the node SDK. `SessionMeta.titleKind` is discarded at the ACP boundary. +- **DeepSeek Harness**: pins `@deepseek-ai/dsh-session-title` in its dependency closure but + never mounts it in `createDeepSeekHarnessCordisConfig`. From 66c4b27a403707583700ff2b634ca81697b6ec50 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 8 Sep 2026 13:27:29 +0000 Subject: [PATCH 04/11] docs(cli): record the verified Grok ACP title behavior A live probe on a credentialed machine settles the open question from the previous commit. Grok pushes its generated title as a session_info_update notification, once per session, and acp-extension-grok forwards it untouched -- direct and proxied runs both saw exactly one push, matching the non-empty session_summary written to disk. The message carries no _meta, so Grok has the same shape as Claude: an authoritative pushed title with no titleSource. Lody already receives it today and discards it in handleAgentSessionTitleUpdate for want of a tag. The pull path does not exist in the mode Lody runs: x.ai/session/info answers -32601 under `grok agent stdio`. That also means proxy.js's internalRequest('context', ...) always errors and is silently dropped, so builtin Grok's context-window usage notification is dead against runtime 1.0.13. Recorded for separate follow-up. No behavior change. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 48 +++++++++++++------ apps/cli/src/agent/README.md | 12 ++--- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index 5c49225b0..452cae3e7 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -8,10 +8,11 @@ Translation: pending Lody wanted every builtin agent to stop carrying its own `titleGeneration` session config and take the session title from the ACP adapter instead. An audit of all five builtin adapters shows only acp-extension-claude and -acp-extension-codex surface a usable title today, so the isolated generator and -its config must stay for Grok, Kimi and the DeepSeek Harness — though for all -three the gap is ours rather than a missing upstream feature, most sharply for -Grok, whose official runtime generates titles inside its own ACP session impl. +acp-extension-codex are wired up today, so the isolated generator and its config +must stay for Grok, Kimi and the DeepSeek Harness. For all three the gap is ours, +not a missing upstream feature — most sharply for Grok, which a live probe shows +already pushes a real generated title that Lody receives and then discards for +carrying no `titleSource`. Codex was the one adapter doing the work twice, since Lody spawned an isolated codex ACP session for the title while the adapter independently generated and pushed its own, so Codex now joins Claude as an ACP-owned title provider. That @@ -29,7 +30,7 @@ Each adapter was read at the commit this repository pins. | --- | --- | --- | --- | --- | | `acp-extension-claude` | 0.70.0 | yes | no `_meta` at all | yes — SDK `generate_session_title` control request | | `acp-extension-codex` | 1.10.0 (since 1.8.0) | yes | yes, generated titles are `explicit` | yes — cheap-model turn on an ephemeral thread | -| `acp-extension-grok` | 0.1.0 | no (adapter); the official runtime does generate titles | no | not in the adapter — upstream `title_refresh.rs` does | +| `acp-extension-grok` | 0.1.0 | yes — the runtime pushes it and the proxy forwards it | no `_meta` at all | yes — upstream `title_refresh.rs` | | `acp-extension-kimi` | acp-server 0.0.1 | yes, but the title is the first prompt truncated to 200 chars | no `_meta` at all | no | | `acp-extension-dsh` | 0.1.1 | no | no | no | @@ -58,16 +59,33 @@ inside the ACP session implementation, alongside `goal.rs`, `mcp.rs` and `prompt_build.rs` — so it is not TUI-only, and the runtime's ACP `SessionUpdate` enum includes `session_info_update` with `title` and `updatedAt`. -What remains unverified is whether that title reaches the ACP wire as a pushed -`session_info_update` or only as a field of the `x.ai/session/info` response. It -could not be settled on the machine used for this audit: there are no Grok -credentials there (`~/.grok/auth.json` is absent) so no turn could be run, and all -1375 stored Grok sessions are zero-message Lody capability probes with an empty -`session_summary`. Both candidate paths are already within reach of existing code: -`handleRuntimeMethod` in `packages/acp-extension-grok/src/proxy.js` ends in a -verbatim `passthrough`, so a pushed update already flows to Lody and is dropped -only for lacking `_meta.lody.titleSource`; and the adapter already issues -`x.ai/session/info` every turn but consumes only `sessionInfo?.context`. +That title does reach the ACP wire, as a pushed notification. A probe run on a +credentialed machine against runtime 1.0.13 — one short turn, then a 25s wait — +produced exactly one push per run, both talking straight to `grok agent stdio` +and routing through `acp-extension-grok`: + +```json +{"jsonrpc":"2.0","method":"session/update","params":{"sessionId":"01a08127-...", + "update":{"sessionUpdate":"session_info_update","title":"Reply with single word ok"}}} +``` + +The same title landed in the session's on-disk `summary.json` (`session_summary` +non-empty, so generation genuinely ran), and the proxy filtered nothing — the two +paths differed only in generated wording. Crucially the message carries **no +`_meta` at all**, so Grok has the same shape as Claude: one authoritative pushed +title with no `titleSource` to gate on. Lody therefore already receives Grok's +title today and discards it in `handleAgentSessionTitleUpdate` for want of a tag. +Only one push was observed, with no `fallback`-style preview beforehand. + +The pull path does not exist in the mode Lody runs: `x.ai/session/info` answers +`-32601 Method not found` under `grok agent stdio`, direct and through the proxy +alike. The literal string is present in the shipped binary, so the method is +presumably registered on some other channel, but not on the ACP agent one. That +has a consequence beyond titles, recorded here because the evidence is in hand: +`proxy.js` issues `internalRequest('context', ...)` after `model_changed` and at +session start, and drops the reply when it is an error, so builtin Grok's +context-window usage notification is silently dead against this runtime. Fixing +that is separate work and is not attempted here. ## Decision diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index be1386695..d912ab1ea 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -194,13 +194,11 @@ Grok, Kimi and the DeepSeek Harness deliver no title Lody can currently use, so using `title-generator.ts` / `response-utils.ts` and the `titleGeneration` config. In all three cases the gap is on our side, not a missing upstream feature: -- **Grok**: the official runtime generates titles itself, from - `xai-grok-shell/src/session/acp_session_impl/title_refresh.rs` — inside its ACP session - impl, so it runs under `grok acp`, not only in the TUI. `acp-extension-grok` adds no title - handling: an emitted `session_info_update` would reach Lody untouched through - `handleRuntimeMethod`'s default passthrough and then be dropped for lacking - `_meta.lody.titleSource`. The adapter also already calls `x.ai/session/info` every turn but - keeps only `.context`. +- **Grok**: verified by live probe to already push one real generated + `session_info_update` title per session, which `acp-extension-grok` forwards untouched. + It carries no `_meta`, so Lody receives it and discards it for lacking + `_meta.lody.titleSource` — the same untagged shape as Claude. Enabling it is a tagging + decision, not missing upstream support. - **Kimi**: its `session_info_update` carries the first prompt truncated to 200 chars with no `_meta`, while the engine's real `SessionTitleService` stays reachable only from kap-server and the node SDK. `SessionMeta.titleKind` is discarded at the ACP boundary. From ae2b096986498737649854dac7284c89b2407a85 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 8 Sep 2026 14:00:39 +0000 Subject: [PATCH 05/11] feat(cli): let builtin Grok own its ACP session title A live probe confirmed the official Grok runtime generates its session title in its own ACP session impl and pushes exactly one session_info_update per session, which acp-extension-grok forwards untouched. Lody already received that title and dropped it for carrying no _meta.lody.titleSource, while separately spawning an isolated Grok ACP session to generate a second one. Add builtin Grok to both predicates. It joins Claude in the trusted-untagged set because it also pushes a bare session_info_update with no _meta, and unlike Codex it emits no first-prompt fallback preview that could win the title. Codex stays out of that set. Grok now skips the isolated generator, hides the title-generation config, and ignores any titleGeneration persisted before this change. Trade-off: title wording for all three ACP-owned agents now belongs to their adapters, so DEFAULT_TITLE_GENERATION_PROMPT no longer constrains them, and Grok keeps refining its title over the first few turns before freezing it. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 67 +++++++++++-------- apps/cli/src/agent/AGENTS.md | 14 ++-- apps/cli/src/agent/README.md | 49 +++++++------- apps/cli/tests/message-handler-title.test.ts | 14 ++-- packages/shared/src/ai.ts | 27 +++++--- .../tests/title-generation-defaults.test.ts | 17 +++-- 6 files changed, 105 insertions(+), 83 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index 452cae3e7..a1b00b887 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -1,4 +1,4 @@ -# Let Codex own its session title, and split the ACP title predicates +# Let Codex and Grok own their session titles, and split the ACP title predicates Status: implemented Translation: pending @@ -6,21 +6,20 @@ Translation: pending ## Abstract Lody wanted every builtin agent to stop carrying its own `titleGeneration` -session config and take the session title from the ACP adapter instead. An audit -of all five builtin adapters shows only acp-extension-claude and -acp-extension-codex are wired up today, so the isolated generator and its config -must stay for Grok, Kimi and the DeepSeek Harness. For all three the gap is ours, -not a missing upstream feature — most sharply for Grok, which a live probe shows -already pushes a real generated title that Lody receives and then discards for -carrying no `titleSource`. -Codex was the one adapter doing the work twice, since Lody spawned an isolated -codex ACP session for the title while the adapter independently generated and -pushed its own, so Codex now joins Claude as an ACP-owned title provider. That -required splitting the single `usesAcpProvidedSessionTitle()` predicate, because -the two adapters need opposite trust rules and conflating them would have -promoted Codex's first-prompt preview to the session title. Branch naming still +session config and take the session title from its ACP adapter instead. An audit +of all five builtin adapters found three that already produce a usable title — +Claude, Codex, and, contrary to a first reading that inspected only our proxy, +Grok, whose official runtime generates one and pushes it, confirmed by live probe +— so all three now take the ACP title while Kimi and the DeepSeek Harness keep +the isolated generator. Codex and Grok were both doing the work twice, each +generating a title that Lody then either duplicated or discarded. Delivering this +required splitting the single `usesAcpProvidedSessionTitle()` predicate into +ownership and trust, because Codex tags its titles and emits a prompt-preview +`fallback` first while Claude and Grok push one bare authoritative title, and +conflating the two would have promoted Codex's preview to the session title. The +cost is that title wording now belongs to the adapters, and branch naming still falls back to the isolated generator when no ACP title has landed yet, so this -reduces title work but does not yet eliminate the isolated session. +removes duplicated work without yet eliminating the isolated session. ## The audit @@ -30,7 +29,7 @@ Each adapter was read at the commit this repository pins. | --- | --- | --- | --- | --- | | `acp-extension-claude` | 0.70.0 | yes | no `_meta` at all | yes — SDK `generate_session_title` control request | | `acp-extension-codex` | 1.10.0 (since 1.8.0) | yes | yes, generated titles are `explicit` | yes — cheap-model turn on an ephemeral thread | -| `acp-extension-grok` | 0.1.0 | yes — the runtime pushes it and the proxy forwards it | no `_meta` at all | yes — upstream `title_refresh.rs` | +| `acp-extension-grok` | 0.1.0 (runtime 1.0.13) | yes — the runtime pushes it and the proxy forwards it | no `_meta` at all | yes — upstream `title_refresh.rs` | | `acp-extension-kimi` | acp-server 0.0.1 | yes, but the title is the first prompt truncated to 200 chars | no `_meta` at all | no | | `acp-extension-dsh` | 0.1.1 | no | no | no | @@ -93,27 +92,29 @@ that is separate work and is not attempted here. sites, and Codex needs opposite answers to them: - *May Lody skip its isolated generator and hide the title config?* Yes for - Claude and Codex. This is now `acpOwnsSessionTitleGeneration()`. -- *May Lody trust a pushed title that carries no `titleSource`?* Only for Claude, - which sends a bare `session_info_update`. This is now + Claude, Codex and Grok. This is now `acpOwnsSessionTitleGeneration()`. +- *May Lody trust a pushed title that carries no `titleSource`?* Only for Claude + and Grok, which both send a bare `session_info_update`. This is now `trustsUntaggedAcpSessionTitle()`. -Codex must stay out of the second predicate. It emits a `fallback` prompt-preview -title before its generated `explicit` one, and `apps/cli/src/agent/AGENTS.md` -already required rejecting that preview. Extending the original single predicate -to Codex would have silently made the raw first prompt the session title — the -main trap this split exists to prevent. +The two sets are deliberately not the same, and Codex is the reason. It emits a +`fallback` prompt-preview title before its generated `explicit` one, and +`apps/cli/src/agent/AGENTS.md` already required rejecting that preview. Keeping +one predicate and extending it to Codex would have silently made the raw first +prompt the session title — the main trap this split exists to prevent. Grok, by +contrast, was observed to push exactly one title with no preview, so it joins +Claude in the trusted-untagged set. The `titleGeneration` config surface (schema field, settings section, CLI flags) is deliberately left in place. Removing it would strip the cheap-model and -least-privilege-mode selection that Grok, Kimi, DeepSeek Harness, registry and -custom providers still rely on. The config simply stops being reachable for -Codex, as it already was for Claude. +least-privilege-mode selection that Kimi, the DeepSeek Harness, registry and +custom providers still rely on. The config simply stops being reachable for Codex +and Grok, as it already was for Claude. "Unreachable" has to hold on every path, not just the settings form. Branch naming resolved the persisted `titleGeneration` for whatever agent it was naming a branch for, so a value stored before this change would have kept steering -Claude and Codex runs after their config disappeared from the UI. Branch naming +Claude, Codex and Grok runs after their config disappeared from the UI. Branch naming now skips that lookup for ACP-owned agents and lets `computeTitleGenerationDefaults()` pick from the live `configOptions` instead. @@ -126,6 +127,12 @@ best-effort inside the adapter and swallows failures without signalling the client, so a failed generation now leaves the draft title rather than falling back to Lody's generator. +Handing titles to the adapters also hands over their wording. None of the three +sees `DEFAULT_TITLE_GENERATION_PROMPT`, so constraints it carries — the 26-letter +English budget, the single-line rule — no longer apply to them. Grok additionally +keeps refining its title over the first few turns before freezing it, so a Grok +session title can change after it first appears. + This change does not reach zero isolated ACP sessions. `generateBranchNameWithTimeout` reuses an in-flight or already-stored generated title, and otherwise still calls `generateTitleIsolated()`; with the title path skipped, Codex now takes that @@ -136,4 +143,6 @@ to the prompt text — changing that affects every provider, not just the ACP-owned ones. Verification is type checks, lint, and the shared unit tests covering both -predicates. No live codex, Kimi, Grok or DeepSeek session was exercised. +predicates, plus the branch-name cases for all three ACP-owned agents. The Grok +behaviour rests on the live probe described above; no live Codex, Kimi or +DeepSeek session was exercised. diff --git a/apps/cli/src/agent/AGENTS.md b/apps/cli/src/agent/AGENTS.md index 3e5aec5dd..3395b9fc9 100644 --- a/apps/cli/src/agent/AGENTS.md +++ b/apps/cli/src/agent/AGENTS.md @@ -105,11 +105,11 @@ context/acp-agent-edit-evidence.md; adapter repos: [apps/cli/AGENTS.md](../../AG the cache, and requests/responses carry that id to keep configs of one provider isolated. `ManagedRuntimeUpdateCoordinator` never hot-swaps a running ACP process, and Machine Flock writes ignore `fetchedAt` when comparing entries. -- Builtin Claude and Codex own session titles via ACP `session_info_update` +- Builtin Claude, Codex and Grok own session titles via ACP `session_info_update` (`acpOwnsSessionTitleGeneration()`): store them only after - `sanitizeLodyInternalInstructions` and never start `title-generator.ts`'s isolated session - for them. Only Claude is trusted untagged (`trustsUntaggedAcpSessionTitle()`); for Codex - take only `explicit` `_meta.lody.titleSource`, ignore its first-prompt `fallback`, - and require `_meta.lody.messagePhase === 'final_answer'`; untyped chunks, error/warning - payloads, and internal-instruction tails never qualify. Each isolated run owns and removes - a temp dir; session-title and branch-name work share one in-flight result. + `sanitizeLodyInternalInstructions`, never via `title-generator.ts`. + Claude and Grok push untagged and are trusted so (`trustsUntaggedAcpSessionTitle()`); + Codex is not — take only its `explicit` `_meta.lody.titleSource`, ignore its first-prompt + `fallback`, and require `_meta.lody.messagePhase === 'final_answer'`. Untyped chunks, + error/warning payloads, and internal-instruction tails never qualify. Each isolated run + owns and removes a temp dir; session-title and branch-name share one in-flight result. diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index d912ab1ea..dc6aecc1c 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -178,29 +178,26 @@ is a context window). Vendor model `_meta` never enters the CLI. ### Session titles -Builtin Claude and Codex own session title generation through ACP `session_info_update`: -acp-extension-claude asks the Agent SDK for a title via its `generate_session_title` control -request, and acp-extension-codex (>= 1.8.0) runs a cheap-model turn on an ephemeral thread and -persists the result as the codex thread name. The shared `acpOwnsSessionTitleGeneration()` -predicate keeps `title-generator.ts`'s isolated session out of their title path and hides the -obsolete provider title settings for both. - -The two adapters differ in how they label a title, so the trust gate is separate. Claude sends -a bare `session_info_update` with no `_meta`, so it needs the `trustsUntaggedAcpSessionTitle()` -allowlist. Codex tags every title and emits a first-prompt `fallback` preview before its -generated `explicit` one, so it must stay outside that allowlist or the preview would win. - -Grok, Kimi and the DeepSeek Harness deliver no title Lody can currently use, so they keep -using `title-generator.ts` / `response-utils.ts` and the `titleGeneration` config. In all -three cases the gap is on our side, not a missing upstream feature: - -- **Grok**: verified by live probe to already push one real generated - `session_info_update` title per session, which `acp-extension-grok` forwards untouched. - It carries no `_meta`, so Lody receives it and discards it for lacking - `_meta.lody.titleSource` — the same untagged shape as Claude. Enabling it is a tagging - decision, not missing upstream support. -- **Kimi**: its `session_info_update` carries the first prompt truncated to 200 chars with no - `_meta`, while the engine's real `SessionTitleService` stays reachable only from kap-server - and the node SDK. `SessionMeta.titleKind` is discarded at the ACP boundary. -- **DeepSeek Harness**: pins `@deepseek-ai/dsh-session-title` in its dependency closure but - never mounts it in `createDeepSeekHarnessCordisConfig`. +Builtin Claude, Codex and Grok own session title generation through ACP +`session_info_update`. acp-extension-claude asks the Agent SDK for a title via its +`generate_session_title` control request; acp-extension-codex (>= 1.8.0) runs a cheap-model +turn on an ephemeral thread and persists the result as the codex thread name; Grok's official +runtime generates the title inside its own ACP session impl +(`xai-grok-shell/src/session/acp_session_impl/title_refresh.rs`) and pushes one update per +session, which the `acp-extension-grok` proxy forwards untouched. The shared +`acpOwnsSessionTitleGeneration()` predicate keeps `title-generator.ts`'s isolated session out +of their title path and hides the obsolete provider title settings for all three. + +How a title is labelled decides whether Lody may trust it, and that is a separate, +narrower set. Claude and Grok both send a bare `session_info_update` with no `_meta`, so they +need the `trustsUntaggedAcpSessionTitle()` allowlist. Codex tags every title and emits a +first-prompt `fallback` preview before its generated `explicit` one, so it must stay outside +that allowlist even though it does own its generation — otherwise the preview wins. + +Kimi and the DeepSeek Harness still use `title-generator.ts` / `response-utils.ts` and the +`titleGeneration` config. Neither gap is a missing upstream feature: Kimi's +`session_info_update` carries the first prompt truncated to 200 chars with no `_meta` while +its real `SessionTitleService` stays reachable only from kap-server and the node SDK (the +engine's `SessionMeta.titleKind` is discarded at the ACP boundary), and the DeepSeek Harness +pins `@deepseek-ai/dsh-session-title` in its dependency closure but never mounts it in +`createDeepSeekHarnessCordisConfig`. diff --git a/apps/cli/tests/message-handler-title.test.ts b/apps/cli/tests/message-handler-title.test.ts index 7b8a6a17e..f4919f812 100644 --- a/apps/cli/tests/message-handler-title.test.ts +++ b/apps/cli/tests/message-handler-title.test.ts @@ -249,7 +249,7 @@ describe('MessageHandler title generation', () => { // Claude and Codex no longer expose a title-generation config, so a value // persisted before that must not keep steering their branch-name runs. - it.each(['claude', 'codex'])( + it.each(['claude', 'codex', 'grok'])( 'ignores a stale persisted titleGeneration when naming a branch for %s', async (agentType) => { const { handler } = await createHandler(undefined, undefined, undefined, { @@ -411,10 +411,10 @@ describe('MessageHandler title generation', () => { expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); }); - // acp-extension-codex >= 1.8.0 generates its own title on an ephemeral thread - // and pushes it as session_info_update, so the isolated generator would only - // duplicate that work — and would not read the agent config to do it. - it('skips isolated generation for builtin Codex', async () => { + // Codex generates its title on an ephemeral thread and Grok's runtime pushes one + // per session; either way the isolated generator would only duplicate that work, + // and must not read the agent config to do it. + it.each(['codex', 'grok'])('skips isolated generation for builtin %s', async (agentType) => { const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { agentConfigId: 'agent-config-1', agentConfigMeta: { @@ -431,9 +431,9 @@ describe('MessageHandler title generation', () => { ) => Promise; }; await titleHost.maybeGenerateAndStoreSessionTitle( - 's-codex-acp' as SessionId, + 's-acp-owned' as SessionId, 'builtin', - 'codex', + agentType, 'Do something cool' ); diff --git a/packages/shared/src/ai.ts b/packages/shared/src/ai.ts index ae5248100..6835c1a45 100644 --- a/packages/shared/src/ai.ts +++ b/packages/shared/src/ai.ts @@ -40,6 +40,12 @@ export type BuiltinAgentType = BuiltinAgent['agentType']; export type AgentConfigCliType = 'builtin' | 'registry' | 'custom'; export type AgentType = string; +/** Builtin agents whose ACP adapter generates the session title itself. */ +const ACP_TITLE_OWNING_AGENTS = new Set(['claude', 'codex', 'grok']); + +/** Of those, the ones that push a title carrying no `_meta.lody.titleSource`. */ +const UNTAGGED_TITLE_AGENTS = new Set(['claude', 'grok']); + /** * Builtin ACP adapters that generate their own session titles, so Lody never * starts its isolated title agent for them and hides the title-generation @@ -50,27 +56,32 @@ export type AgentType = string; * - `codex`: acp-extension-codex (>= 1.8.0) runs its own cheap-model generation * on an ephemeral thread, persists it as the codex thread name, and publishes * it tagged `_meta.lody.titleSource: 'explicit'`. + * - `grok`: the official runtime generates the title in its own ACP session impl + * and pushes one `session_info_update` per session, which acp-extension-grok + * forwards untouched. * - * Grok, Kimi and the DeepSeek Harness have no ACP title generation at all and - * still depend on the isolated generator. + * Kimi and the DeepSeek Harness still depend on the isolated generator: Kimi's + * pushed title is only the first prompt truncated to 200 chars, and the Harness + * never mounts its upstream title plugin. */ export const acpOwnsSessionTitleGeneration = ( cliType: AgentConfigCliType | null | undefined, agentType: AgentType | null | undefined -): boolean => cliType === 'builtin' && (agentType === 'claude' || agentType === 'codex'); +): boolean => cliType === 'builtin' && !!agentType && ACP_TITLE_OWNING_AGENTS.has(agentType); /** * Adapters whose pushed titles are authoritative without a `titleSource` tag. * - * Deliberately narrower than {@link acpOwnsSessionTitleGeneration}: only - * acp-extension-claude omits `_meta.lody.titleSource`, so it needs an allowlist. - * Codex tags every title, and its first-prompt `fallback` preview must stay - * rejected — widening this predicate would turn that preview into the title. + * Deliberately narrower than {@link acpOwnsSessionTitleGeneration}: Claude and + * Grok both publish a bare `session_info_update` with no `_meta`, so they need + * an allowlist. Codex tags every title and emits a first-prompt `fallback` + * preview before its generated `explicit` one, so it must stay out — widening + * this predicate would turn that preview into the session title. */ export const trustsUntaggedAcpSessionTitle = ( cliType: AgentConfigCliType | null | undefined, agentType: AgentType | null | undefined -): boolean => cliType === 'builtin' && agentType === 'claude'; +): boolean => cliType === 'builtin' && !!agentType && UNTAGGED_TITLE_AGENTS.has(agentType); /** * User-defined ACP launch spec for `cliType: 'custom'` providers: the exact diff --git a/packages/shared/tests/title-generation-defaults.test.ts b/packages/shared/tests/title-generation-defaults.test.ts index 4fef0b237..0971b26e7 100644 --- a/packages/shared/tests/title-generation-defaults.test.ts +++ b/packages/shared/tests/title-generation-defaults.test.ts @@ -120,32 +120,37 @@ describe('getBuiltinTitleGenerationDefaults', () => { }); describe('acpOwnsSessionTitleGeneration', () => { - it('lets the builtin Claude and Codex adapters generate their own titles', () => { + it('lets the builtin Claude, Codex and Grok adapters generate their own titles', () => { expect(acpOwnsSessionTitleGeneration('builtin', 'claude')).toBe(true); expect(acpOwnsSessionTitleGeneration('builtin', 'codex')).toBe(true); + expect(acpOwnsSessionTitleGeneration('builtin', 'grok')).toBe(true); }); it('keeps isolated title generation for adapters without ACP title support', () => { expect(acpOwnsSessionTitleGeneration('builtin', 'kimi')).toBe(false); - expect(acpOwnsSessionTitleGeneration('builtin', 'grok')).toBe(false); expect(acpOwnsSessionTitleGeneration('builtin', 'deepseek')).toBe(false); }); it('never applies to registry or custom providers', () => { expect(acpOwnsSessionTitleGeneration('registry', 'codex')).toBe(false); expect(acpOwnsSessionTitleGeneration('custom', 'claude')).toBe(false); + expect(acpOwnsSessionTitleGeneration('custom', 'grok')).toBe(false); }); }); describe('trustsUntaggedAcpSessionTitle', () => { - it('trusts builtin Claude, which publishes titles without a titleSource tag', () => { + // Claude and Grok both publish a bare session_info_update with no _meta. + it('trusts the builtin adapters that publish titles without a titleSource tag', () => { expect(trustsUntaggedAcpSessionTitle('builtin', 'claude')).toBe(true); + expect(trustsUntaggedAcpSessionTitle('builtin', 'grok')).toBe(true); }); - // Codex tags every title and emits a first-prompt `fallback` preview before - // its generated `explicit` title. Trusting untagged titles here would promote - // that preview to the session title. + // Codex tags every title and emits a first-prompt `fallback` preview before its + // generated `explicit` one. Trusting untagged titles here would promote that + // preview to the session title, so it must stay outside this set even though it + // does own its title generation. it('does not trust Codex titles that lack an explicit titleSource', () => { + expect(acpOwnsSessionTitleGeneration('builtin', 'codex')).toBe(true); expect(trustsUntaggedAcpSessionTitle('builtin', 'codex')).toBe(false); }); From d45186144a717bed85a3146066f3582b1a0c9679 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 8 Sep 2026 15:05:34 +0000 Subject: [PATCH 06/11] test(components): move the title-config test off ACP-owned Codex The dialog test edited a builtin Codex config and asserted the Title generation section renders. Codex now generates its own title over ACP, so that section is hidden and the test asserted behavior this branch deliberately removed. Retarget the fixture to Kimi, which still owns the setting. Two fixture details the Codex version did not need: - provenance: 'runtime' -- builtin Kimi is held to an authoritative cache entry before the dialog renders config selectors, so without it the section rendered in its "Probing capabilities" state with no selectors. - modelReasoningEfforts -- Codex resolves its effort ladder through a dedicated branch, so the map was never consulted for it. Every other agent needs it for a stored effort to be seen as invalid and normalized. Also pin the behavior the moved test stopped covering: Claude, Codex and Grok hide the section. Model: claude-opus-5[1m] --- .../tests/agent-config-dialog.test.tsx | 59 ++++++++++++++----- 1 file changed, 44 insertions(+), 15 deletions(-) diff --git a/packages/components/tests/agent-config-dialog.test.tsx b/packages/components/tests/agent-config-dialog.test.tsx index bda72b81f..ff3b6a9d3 100644 --- a/packages/components/tests/agent-config-dialog.test.tsx +++ b/packages/components/tests/agent-config-dialog.test.tsx @@ -23,7 +23,7 @@ import { TooltipProvider } from '../src/ui/tooltip'; const machineId = 'machine-test' as MachineId; const claudeConfigId = 'claude-config' as AgentConfigId; -const codexConfigId = 'codex-config' as AgentConfigId; +const kimiConfigId = 'kimi-config' as AgentConfigId; type RefreshCapabilities = ComponentProps['onRefreshCapabilities']; /** Omits `protocolCapabilities` by default, so the machine reads as legacy. */ @@ -53,14 +53,17 @@ const createMachine = ( }, }); -const createCodexMachine = (): MachineViewMeta => ({ - ...createMachine('Codex workstation'), +const createKimiMachine = (): MachineViewMeta => ({ + ...createMachine('Kimi workstation'), acpCapabilities: { - [getAcpCapabilityCacheKey(codexConfigId)]: { + [getAcpCapabilityCacheKey(kimiConfigId)]: { cliType: 'builtin', - agentType: 'codex', + agentType: 'kimi', cacheVersion: ACP_CAPABILITY_CACHE_VERSION, - sourceVersion: 'codex@1.0.0', + sourceVersion: 'kimi-code@1.0.0', + // Builtin Kimi is the one agent the dialog holds to an authoritative + // (real runtime probe) cache entry before it renders config selectors. + provenance: 'runtime', modes: [], models: [], configOptions: [ @@ -69,10 +72,10 @@ const createCodexMachine = (): MachineViewMeta => ({ name: 'Model', category: 'model', type: 'select', - currentValue: 'gpt-5.6-sol', + currentValue: 'kimi-k2', options: [ - { value: 'gpt-5.6-sol', name: 'GPT-5.6 Sol' }, - { value: 'gpt-5.6-other', name: 'GPT-5.6 Other' }, + { value: 'kimi-k2', name: 'Kimi K2' }, + { value: 'kimi-k2-turbo', name: 'Kimi K2 Turbo' }, ], }, { @@ -89,6 +92,11 @@ const createCodexMachine = (): MachineViewMeta => ({ ], }, ], + // The title model's ladder omits `ultra`, so a stored `ultra` effort is + // invalid for it and must normalize to the selector's current value. + // Codex resolves its ladder through a dedicated branch; every other agent + // goes through this map. + modelReasoningEfforts: { 'kimi-k2-turbo': ['low', 'medium'] }, availableCommands: [], fetchedAt: Date.now(), }, @@ -1012,25 +1020,46 @@ describe('AgentConfigDialog', () => { expect(findSignInAgainButton()).toBeUndefined(); }); + // Claude, Codex and Grok generate their own title over ACP, so the setting is + // obsolete for them. Kimi keeps it (covered by the normalization test below). + it.each(['claude', 'codex', 'grok'])( + 'hides the title generation section for builtin %s', + async (agentType) => { + const config = { + id: kimiConfigId, + machineId, + name: 'ACP-owned', + description: undefined, + cliType: 'builtin', + agentType, + env: {}, + } as AgentConfigMeta; + + await renderDialog({ kind: 'edit', config }, createKimiMachine()); + + expect(document.body.textContent).not.toContain('Title generation'); + } + ); + it('saves a normalized title reasoning effort after the title model changes', async () => { const onSubmit = vi.fn(async () => {}); const config = { - id: codexConfigId, + id: kimiConfigId, machineId, - name: 'Codex', + name: 'Kimi', description: undefined, cliType: 'builtin', - agentType: 'codex', + agentType: 'kimi', env: {}, titleGeneration: { configOptionValues: { - model: 'gpt-5.6-other', + model: 'kimi-k2-turbo', reasoning_effort: 'ultra', }, }, } as AgentConfigMeta; - await renderDialog({ kind: 'edit', config }, createCodexMachine(), onSubmit); + await renderDialog({ kind: 'edit', config }, createKimiMachine(), onSubmit); expect(document.body.textContent).toContain('Title generation'); @@ -1046,7 +1075,7 @@ describe('AgentConfigDialog', () => { expect.objectContaining({ titleGeneration: { configOptionValues: { - model: 'gpt-5.6-other', + model: 'kimi-k2-turbo', reasoning_effort: 'medium', }, }, From 5e799e685ceb255f2843ade66e7a18c69efe49f5 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 9 Sep 2026 03:33:43 +0000 Subject: [PATCH 07/11] refactor(cli): derive worktree branch names without an ACP agent Skipping isolated title generation for the ACP-owned agents did not reduce anything for worktree sessions: maybeRenameSessionBranchFromPrompt runs at session-ready, before any turn, so no pushed title can have arrived and it simply started its own agent instead. The isolated session moved from the title path to the branch path rather than disappearing. titleToBranchName was always a pure transform, so the agent only ever compressed the prompt into a shorter title first. Branch naming now prefers a title already stored or in flight for the session and otherwise converts the prompt directly, so nothing about session titles starts an extra ACP agent anymore. generateBranchNameWithTimeout drops its provider, env, launch spec and title config parameters, and the branch path no longer reads the agent config -- which also removes the stale title config guard added earlier in this branch. Two behaviour changes. A prompt yielding no valid name now leaves the managed session/ branch alone instead of renaming it to task/; kebab conversion strips non-ASCII, so that is the normal outcome for a Chinese prompt, and those sessions previously paid for an isolated agent that could never produce a usable name. A timeout now falls back to the prompt instead of abandoning the rename. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 53 ++++++-- apps/cli/src/agent/AGENTS.md | 2 +- apps/cli/src/agent/README.md | 8 ++ apps/cli/src/lib/message-handler.ts | 121 +++++------------- .../src/session/session-execution-service.ts | 10 +- apps/cli/tests/message-handler-title.test.ts | 111 ++++++++-------- 6 files changed, 139 insertions(+), 166 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index a1b00b887..a592b835c 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -17,9 +17,9 @@ required splitting the single `usesAcpProvidedSessionTitle()` predicate into ownership and trust, because Codex tags its titles and emits a prompt-preview `fallback` first while Claude and Grok push one bare authoritative title, and conflating the two would have promoted Codex's preview to the session title. The -cost is that title wording now belongs to the adapters, and branch naming still -falls back to the isolated generator when no ACP title has landed yet, so this -removes duplicated work without yet eliminating the isolated session. +cost is that title wording now belongs to the adapters. Branch naming, the last +caller that could still start an isolated session, now derives its name locally, +so session titles no longer start an extra ACP agent anywhere. ## The audit @@ -133,16 +133,41 @@ English budget, the single-line rule — no longer apply to them. Grok additiona keeps refining its title over the first few turns before freezing it, so a Grok session title can change after it first appears. -This change does not reach zero isolated ACP sessions. `generateBranchNameWithTimeout` -reuses an in-flight or already-stored generated title, and otherwise still calls -`generateTitleIsolated()`; with the title path skipped, Codex now takes that -branch as Claude already did. Making branch naming wait for the ACP-pushed title -within its existing 20s budget is the remaining step, and is not attempted here -because the timeout path currently abandons the rename rather than falling back -to the prompt text — changing that affects every provider, not just the -ACP-owned ones. +Branch naming had to change too, or the isolated session would simply have moved +from the title path to the branch path. `maybeRenameSessionBranchFromPrompt` runs +at session-ready, before any turn, so an ACP title can never have arrived by then; +with the title path skipped it would have started its own agent, leaving worktree +sessions at exactly the same one isolated session as before. + +Two options were considered and rejected. Deferring the rename until the pushed +title arrives moves a "once, at session creation" operation into the middle of a +running conversation, where a turn may already have pushed the branch or opened a +PR — `renameBranchWithAvailableSuffix` is a bare `git branch -m` with no upstream +check. Dropping branch naming entirely and relying on the injected instruction +("Name branches based on the task content", `session-execution-helpers.ts`) fails +because `buildPrompt` runs only in `startSession`, and the instruction is stripped +before storage, so it is absent from turn two onward and from every resumed +session — trading a deterministic behaviour for one whose odds fall as the session +grows. It is also injected only for `project.kind === 'github'`, while worktrees +are also created for local projects with `useWorktree`. + +What actually landed is simpler: `titleToBranchName` was always a pure transform, +so the isolated agent only ever compressed the prompt into a shorter title first. +Branch naming now prefers a title that is already stored or in flight, and +otherwise converts the prompt directly. `generateBranchNameWithTimeout` no longer +takes a provider, env, launch spec or title config, and the branch path no longer +reads the agent config at all. + +One deliberate behaviour change: a prompt that yields no valid name now leaves the +managed `session/` branch alone instead of renaming it to `task/`. +Kebab conversion strips every non-ASCII character, so this is the normal outcome +for a Chinese prompt — and it was the outcome before this change too, since the +generator was asked for a title in the prompt's own language. The isolated session +those sessions paid for could never have produced a usable branch name. A timeout +now also falls back to the prompt instead of abandoning the rename. Verification is type checks, lint, and the shared unit tests covering both -predicates, plus the branch-name cases for all three ACP-owned agents. The Grok -behaviour rests on the live probe described above; no live Codex, Kimi or -DeepSeek session was exercised. +predicates, the branch-name derivation cases, and the dialog cases covering the +hidden title-generation section. The Grok behaviour rests on the live probe +described above; no live Codex, Kimi or DeepSeek session was exercised, and no +real worktree rename was driven end to end. diff --git a/apps/cli/src/agent/AGENTS.md b/apps/cli/src/agent/AGENTS.md index 3395b9fc9..ecdf688d7 100644 --- a/apps/cli/src/agent/AGENTS.md +++ b/apps/cli/src/agent/AGENTS.md @@ -112,4 +112,4 @@ context/acp-agent-edit-evidence.md; adapter repos: [apps/cli/AGENTS.md](../../AG Codex is not — take only its `explicit` `_meta.lody.titleSource`, ignore its first-prompt `fallback`, and require `_meta.lody.messagePhase === 'final_answer'`. Untyped chunks, error/warning payloads, and internal-instruction tails never qualify. Each isolated run - owns and removes a temp dir; session-title and branch-name share one in-flight result. + owns and removes a temp dir; branch naming never starts one. diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index dc6aecc1c..e9027a4a0 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -194,6 +194,14 @@ need the `trustsUntaggedAcpSessionTitle()` allowlist. Codex tags every title and first-prompt `fallback` preview before its generated `explicit` one, so it must stay outside that allowlist even though it does own its generation — otherwise the preview wins. +Branch naming never starts an isolated session. `titleToBranchName` is a pure transform, so +the only thing an agent ever added was compressing the prompt into a shorter title first. +`generateBranchNameWithTimeout` now prefers a title that is already stored or in flight for +the session and otherwise converts the prompt directly, falling back to the prompt if a +pending title misses its budget. When no valid name can be derived — kebab conversion drops +every non-ASCII character, so this is the normal outcome for a Chinese prompt — the managed +`session/` branch is left alone rather than renamed to a timestamp. + Kimi and the DeepSeek Harness still use `title-generator.ts` / `response-utils.ts` and the `titleGeneration` config. Neither gap is a missing upstream feature: Kimi's `session_info_update` carries the first prompt truncated to 200 chars with no `_meta` while diff --git a/apps/cli/src/lib/message-handler.ts b/apps/cli/src/lib/message-handler.ts index 9e2180bcf..d36746350 100644 --- a/apps/cli/src/lib/message-handler.ts +++ b/apps/cli/src/lib/message-handler.ts @@ -253,7 +253,7 @@ import type { AcpAgentEditEvidence, AcpStandardDiffBlockEvidence } from '@/lib/a import { mergeAcpRuntimeConfigUpdates } from '@/lib/acp/runtime-config'; import { generateTitleIsolated, sanitizeTitle } from '@/agent/title-generator'; import type { AgentSessionWarning } from '@/agent/agent-client'; -import { ensureValidBranchName } from '@/agent/branch-name-generator'; +import { isValidGitBranchName, titleToBranchName } from '@/agent/branch-name-generator'; import { SessionActivePresenceController, type SessionActivePresencePhase, @@ -386,7 +386,6 @@ import { handleLocalProjectWorktreeConfigRequest, isLocalProjectWorktreeConfigRequest, } from '@/session/worktree/worktree-setup-config-store'; -import { readLegacySessionLaunchConfig } from '@/session/session-launch-config-resolver'; import { resolveSessionWorktreeCleanupConfig } from '@/session/worktree/worktree-config-resolver'; type RepoDocMetaPatch = Parameters[1]; @@ -3155,22 +3154,8 @@ export class MessageHandler { customAcp, runtimeOverrides ), - maybeRenameSessionBranchFromPrompt: async ( - sessionId, - session, - cliType, - agentType, - prompt, - env - ) => - await this.maybeRenameSessionBranchFromPrompt( - sessionId, - session, - cliType, - agentType, - prompt, - env - ), + maybeRenameSessionBranchFromPrompt: async (sessionId, session, prompt) => + await this.maybeRenameSessionBranchFromPrompt(sessionId, session, prompt), processMessageQueue: async (sessionId) => await this.processMessageQueue(sessionId), syncLiveActivitySummary: async (userId) => { await this.syncLiveActivitySummary(userId); @@ -9669,11 +9654,7 @@ export class MessageHandler { private async maybeRenameSessionBranchFromPrompt( sessionId: SessionId, session: ISession, - cliType: AgentConfigCliType, - agentType: string, - taskPrompt: string, - env?: Record, - titleConfig?: TitleGenerationConfig + taskPrompt: string ): Promise { const trimmedPrompt = taskPrompt.trim(); if (!trimmedPrompt) { @@ -9681,32 +9662,15 @@ export class MessageHandler { } let metaBranchName: string | null = null; - let metaCustomAcp: CustomAcpLaunchSpec | undefined; - let metaRuntimeOverrides: BuiltinRuntimeOverrides | undefined; - let metaAgentConfigId: AgentConfigId | undefined; let reusableTitlePromise: Promise | undefined; try { const sessionDoc = await this.workspaceDocument.getOrCreateSessionDoc(sessionId); const meta = await sessionDoc.getMetaState(); metaBranchName = meta?.branchName?.trim() || null; - metaAgentConfigId = meta?.agentConfigId; const generatedMetaTitle = meta?.titleSource === 'generated' ? meta.title?.trim() : ''; reusableTitlePromise = generatedMetaTitle ? Promise.resolve(generatedMetaTitle) : this.titleGenerationInFlight.get(sessionId); - const agentConfig = metaAgentConfigId - ? await this.workspaceDocument.getAgentConfigById(metaAgentConfigId) - : null; - const legacyLaunchConfig = await readLegacySessionLaunchConfig({ - repo: this.workspaceDocument.repo, - workspaceId: this.workspaceId, - machineId: this.machineId, - sessionId, - sessionMeta: meta, - logger: this.logger, - }); - metaCustomAcp = agentConfig?.customAcp ?? legacyLaunchConfig?.customAcp; - metaRuntimeOverrides = agentConfig?.runtimeOverrides ?? legacyLaunchConfig?.runtimeOverrides; if (metaBranchName && !isManagedWorktreeBranchName(metaBranchName)) { return; } @@ -9716,30 +9680,15 @@ export class MessageHandler { ); } - // Claude and Codex no longer expose a title-generation config, so a value - // persisted before that must not keep steering their runs here. They still - // reach the isolated generator when no ACP title has landed yet, and it - // falls back to computeTitleGenerationDefaults() for them. - // Claude and Codex no longer expose a title-generation config, so a value - // persisted before that must not keep steering their runs here. They still - // reach the isolated generator when no ACP title has landed yet, and it - // falls back to computeTitleGenerationDefaults() for them. - const resolvedTitleConfig = acpOwnsSessionTitleGeneration(cliType, agentType) - ? undefined - : (titleConfig ?? (await this.resolveTitleConfig(sessionId, metaAgentConfigId))); const branchName = await this.generateBranchNameWithTimeout( - cliType, - agentType, trimmedPrompt, - env, 20_000, - resolvedTitleConfig, - metaCustomAcp, - metaRuntimeOverrides, reusableTitlePromise ); if (!branchName) { - this.logger.debug(`[${sessionId}] Skipping branch rename: name generation timed out`); + this.logger.debug( + `[${sessionId}] Skipping branch rename: the prompt yields no usable branch name` + ); return; } @@ -9781,47 +9730,47 @@ export class MessageHandler { } } + /** + * Derives a worktree branch name without ever starting an ACP agent. + * + * `titleToBranchName` is a pure transform, so the only thing an isolated agent + * ever added here was compressing the prompt into a shorter title first. A title + * is still preferred when one is already stored or in flight for this session + * (agents that keep the local generator produce one anyway); otherwise the prompt + * names the branch directly. + * + * Returns null when no valid name can be derived — a prompt with no ASCII words + * (kebab conversion strips everything else) leaves the managed `session/` + * branch alone rather than renaming it to a meaningless timestamp. + */ private async generateBranchNameWithTimeout( - cliType: AgentConfigCliType, - agentType: string, taskPrompt: string, - env: Record | undefined, timeoutMs: number, - titleConfig?: TitleGenerationConfig, - customAcp?: CustomAcpLaunchSpec, - runtimeOverrides?: BuiltinRuntimeOverrides, reusableTitlePromise?: Promise ): Promise { + const toBranchName = (base: string): string | null => { + const candidate = titleToBranchName(base); + return candidate && isValidGitBranchName(candidate) ? candidate : null; + }; + + if (!reusableTitlePromise) { + return toBranchName(taskPrompt); + } + let timeoutHandle: NodeJS.Timeout | null = null; const timeoutPromise = new Promise((resolve) => { timeoutHandle = setTimeout(() => resolve(null), timeoutMs); }); - - const namePromise = (async (): Promise => { - const title = reusableTitlePromise - ? await reusableTitlePromise - : await generateTitleIsolated({ - cliType, - agentType, - customAcp, - runtimeOverrides, - taskPrompt, - logger: this.logger, - env, - titleConfig, - }); - const base = title ?? taskPrompt; - return ensureValidBranchName(base, 'task'); - })(); - try { - const result = await Promise.race([namePromise, timeoutPromise]); - return result ?? null; + // A slow or failed title must not hold up (or cancel) the rename: the prompt + // is always available as the naming input. + const title = await Promise.race([reusableTitlePromise, timeoutPromise]); + return toBranchName(title?.trim() || taskPrompt); } catch (error) { this.logger.debug( - `[branch-name] Failed to generate branch name: ${formatErrorMessage(error)}` + `[branch-name] Falling back to the prompt after title generation failed: ${formatErrorMessage(error)}` ); - return null; + return toBranchName(taskPrompt); } finally { if (timeoutHandle) { clearTimeout(timeoutHandle); diff --git a/apps/cli/src/session/session-execution-service.ts b/apps/cli/src/session/session-execution-service.ts index 2c06333fa..4dd313df8 100644 --- a/apps/cli/src/session/session-execution-service.ts +++ b/apps/cli/src/session/session-execution-service.ts @@ -528,10 +528,7 @@ export type SessionExecutionServiceDeps = { maybeRenameSessionBranchFromPrompt: ( sessionId: SessionId, session: ISession, - cliType: AgentConfigCliType, - agentType: string, - prompt: string, - env?: Record + prompt: string ) => Promise; processMessageQueue: (sessionId: SessionId) => Promise; syncLiveActivitySummary?: (userId: string) => Promise; @@ -4675,10 +4672,7 @@ export class SessionExecutionService { void self.deps.maybeRenameSessionBranchFromPrompt( sessionId, session, - sessionConfig.agentCliType, - sessionConfig.agentType, - agentConfig.prompt ?? '', - env + agentConfig.prompt ?? '' ); } diff --git a/apps/cli/tests/message-handler-title.test.ts b/apps/cli/tests/message-handler-title.test.ts index f4919f812..06eab2154 100644 --- a/apps/cli/tests/message-handler-title.test.ts +++ b/apps/cli/tests/message-handler-title.test.ts @@ -102,16 +102,13 @@ const createHandler = async ( return { handler, sessionDoc, workspaceDocument }; }; -// Drives the branch-rename path far enough to observe which titleConfig reaches -// the isolated generator. `exec` reports no branch, so the rename itself is a -// no-op and no git command runs. -const renameBranch = async (handler: MessageHandler, agentType: string): Promise => { +// Drives the whole branch-rename path. `exec` reports no branch, so the rename +// itself is a no-op and no git command runs. +const renameBranch = async (handler: MessageHandler): Promise => { const branchHost = handler as unknown as { maybeRenameSessionBranchFromPrompt: ( sessionId: SessionId, session: unknown, - cliType: string, - agentType: string, taskPrompt: string ) => Promise; }; @@ -122,8 +119,6 @@ const renameBranch = async (handler: MessageHandler, agentType: string): Promise await branchHost.maybeRenameSessionBranchFromPrompt( 's-branch' as SessionId, session, - 'builtin', - agentType, 'Fix the flaky login redirect' ); }; @@ -215,70 +210,72 @@ describe('MessageHandler title generation', () => { expect(sessionDoc.setTitleIfSourceIn).toHaveBeenCalledTimes(1); }); - it('reuses an existing title promise for branch-name generation', async () => { - const { handler } = await createHandler(undefined); - const titleHost = handler as unknown as { - generateBranchNameWithTimeout: ( - cliType: string, - agentType: string, - taskPrompt: string, - env: Record | undefined, - timeoutMs: number, - titleConfig: undefined, - customAcp: undefined, - runtimeOverrides: undefined, - reusableTitlePromise: Promise - ) => Promise; - }; + type BranchNameHost = { + generateBranchNameWithTimeout: ( + taskPrompt: string, + timeoutMs: number, + reusableTitlePromise?: Promise + ) => Promise; + }; - const branch = await titleHost.generateBranchNameWithTimeout( - 'builtin', - 'codex', + it('prefers an already-available session title over the prompt', async () => { + const { handler } = await createHandler(undefined); + const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( 'Fallback prompt', - undefined, 1_000, - undefined, - undefined, - undefined, Promise.resolve('Fix title races') ); expect(branch).toBe('fix/title-races'); + }); + + it('names the branch from the prompt when no title is available', async () => { + const { handler } = await createHandler(undefined); + const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( + 'Add a retry to the upload queue', + 1_000 + ); + + expect(branch).toBe('feat/a-retry-to-the-upload-queue'); expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); }); - // Claude and Codex no longer expose a title-generation config, so a value - // persisted before that must not keep steering their branch-name runs. - it.each(['claude', 'codex', 'grok'])( - 'ignores a stale persisted titleGeneration when naming a branch for %s', - async (agentType) => { - const { handler } = await createHandler(undefined, undefined, undefined, { - agentConfigId: 'agent-config-1', - agentConfigMeta: { titleGeneration: { configOptionValues: { model: 'stale-model' } } }, - }); - - await renameBranch(handler, agentType); - - expect(mockedGenerateTitleIsolated).toHaveBeenCalledTimes(1); - expect(mockedGenerateTitleIsolated).toHaveBeenCalledWith( - expect.objectContaining({ titleConfig: undefined }) - ); - } - ); + // Kebab conversion drops every non-ASCII character, so such a prompt yields no + // name at all. Leaving the managed `session/` branch alone beats renaming it + // to a meaningless timestamp. + it('returns no name when the prompt has no ASCII words', async () => { + const { handler } = await createHandler(undefined); + const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( + '把标题生成迁移到会话协议', + 1_000 + ); + + expect(branch).toBeNull(); + }); + + it('falls back to the prompt when the title does not arrive in time', async () => { + const { handler } = await createHandler(undefined); + const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( + 'Fix the flaky login redirect', + 10, + new Promise(() => {}) + ); + + expect(branch).toBe('fix/the-flaky-login-redirect'); + }); - it('still applies the persisted titleGeneration when naming a branch for Kimi', async () => { - const titleGeneration = { configOptionValues: { model: 'kimi-k2-turbo' } }; - const { handler } = await createHandler(undefined, undefined, undefined, { + // Branch naming is a pure local transform now, so it starts no agent and never + // consults the agent config -- the provider no longer reaches this path at all. + it('never starts an isolated agent to name a branch', async () => { + const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { agentConfigId: 'agent-config-1', - agentConfigMeta: { titleGeneration }, + agentConfigMeta: { titleGeneration: { configOptionValues: { model: 'stale-model' } } }, }); - await renameBranch(handler, 'kimi'); + await renameBranch(handler); - expect(mockedGenerateTitleIsolated).toHaveBeenCalledTimes(1); - expect(mockedGenerateTitleIsolated).toHaveBeenCalledWith( - expect.objectContaining({ titleConfig: titleGeneration }) - ); + expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); + expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); }); it('keeps skipping isolated generation when an existing title has no draft source', async () => { From 88ec62308cd0b45b4db291cf1aca8aa376edc70b Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Thu, 10 Sep 2026 06:29:24 +0000 Subject: [PATCH 08/11] refactor: fold the title predicates into one exhaustive table Cleanup pass over this branch's own diff; no behaviour change. - packages/shared/src/ai.ts: the two hand-synced agentType Sets become one Record. The trusted set is a strict subset of the owning set, which is now structural instead of a comment, and the Record is exhaustive so a new builtin agent cannot silently default (verified: dropping a key fails typecheck). - message-handler.ts: generateBranchNameWithTimeout no longer generates and its timeout applies to only one of its two paths, so it becomes deriveWorktreeBranchName. Its hand-rolled Promise.race/setTimeout/finally now calls withTimeoutOrUndefined, already defined in this file, and its inline toBranchName closure calls the shared helper below. 28 lines -> 16. - branch-name-generator.ts: ensureValidBranchName lost its last production caller in this branch, and its task/ fallback is the behaviour the branch deliberately dropped. Replaced by tryBranchName, the nullable core the caller actually wanted, so the "kebab then validate" rule lives in one place again. - Merged a duplicated dialog test into its it.each table, renamed the components fixture that three of its four uses had made false, and folded two near-identical config literals into a factory. - Trimmed README prose that restated the decision note verbatim and named upstream internals that will drift, leaving the contract plus a link. Recorded in the note why capability negotiation is the right eventual shape and what deferred it, and the residual config-write surfaces. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 31 ++++++- apps/cli/src/agent/README.md | 45 ++++----- apps/cli/src/agent/branch-name-generator.ts | 19 ++-- apps/cli/src/lib/message-handler.ts | 52 ++++------- apps/cli/tests/branch-name-generator.test.ts | 20 ++-- apps/cli/tests/message-handler-title.test.ts | 93 +++++++------------ .../tests/agent-config-dialog.test.tsx | 43 +++++---- packages/shared/src/ai.ts | 64 +++++++------ .../tests/title-generation-defaults.test.ts | 14 ++- 9 files changed, 187 insertions(+), 194 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index 1733090e1..fcfbe89db 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -99,7 +99,27 @@ sites, and Codex needs opposite answers to them: and Grok, which both send a bare `session_info_update`. This is now `trustsUntaggedAcpSessionTitle()`. -The two sets are deliberately not the same, and Codex is the reason. It emits a +Capability negotiation was the alternative, and it was deferred rather than +overlooked. The adapters already declare `_meta.lody` capabilities that Lody +consumes (`usage`, `rateLimits`, `compaction`, ...), and the Grok proxy even +synthesizes some the runtime never sends, so a `sessionTitle` capability is the +shape this rule eventually wants — it would degrade correctly when a +`BuiltinRuntimeOverrides` path points at an older binary, and would let registry +and custom providers opt in, neither of which an identity allowlist can do. The +cost is what deferred it: `acpOwnsSessionTitleGeneration` is consulted at session +start before `initialize` returns, and again in the settings dialog where no +client exists, so it needs the capability *persisted* — a new field on +`AcpCapabilityCacheEntry`, threaded through the capability probe and both +positional doc signatures, plus an `ACP_CAPABILITY_CACHE_VERSION` bump that +invalidates every user's cache and a bootstrap path for never-probed configs. +That is a larger change than this one, and it spans three adapter submodules. +`BUILTIN_ACP_TITLE_OWNERSHIP` is the interim stand-in; it is exhaustive over +`BuiltinAgentType` so a new builtin agent cannot silently default. + +The table also collapses what began as two hand-synced lists. The trusted set is +a strict subset of the owning set, and expressing that as one `none | untagged | +tagged` value per agent makes the relation structural instead of a comment. +Codex is the reason the two questions differ at all. It emits a `fallback` prompt-preview title before its generated `explicit` one, and `apps/cli/src/agent/AGENTS.md` already required rejecting that preview. Keeping one predicate and extending it to Codex would have silently made the raw first @@ -156,7 +176,7 @@ are also created for local projects with `useWorktree`. What actually landed is simpler: `titleToBranchName` was always a pure transform, so the isolated agent only ever compressed the prompt into a shorter title first. Branch naming now prefers a title that is already stored or in flight, and -otherwise converts the prompt directly. `generateBranchNameWithTimeout` no longer +otherwise converts the prompt directly. `deriveWorktreeBranchName` no longer takes a provider, env, launch spec or title config, and the branch path no longer reads the agent config at all. @@ -168,6 +188,13 @@ generator was asked for a title in the prompt's own language. The isolated sessi those sessions paid for could never have produced a usable branch name. A timeout now also falls back to the prompt instead of abandoning the rename. +One residual inconsistency is known and left alone: `acpOwnsSessionTitleGeneration` +gates the settings dialog, but `lody agent-config` and the onboarding provider +screen still accept and persist a `titleGeneration` block for these agents. The +stored value is now provably inert — nothing reads it for them on any path — so +this is cosmetic, and applying the predicate in the config write path is a +follow-up. + Verification is type checks, lint, and the shared unit tests covering both predicates, the branch-name derivation cases, and the dialog cases covering the hidden title-generation section. The Grok behaviour rests on the live probe diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index ed50eabb2..db8f61f83 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -187,36 +187,27 @@ override entries still apply only when their source-version suffix matches the s ### Session titles Builtin Claude, Codex and Grok own session title generation through ACP -`session_info_update`. acp-extension-claude asks the Agent SDK for a title via its -`generate_session_title` control request; acp-extension-codex (>= 1.8.0) runs a cheap-model -turn on an ephemeral thread and persists the result as the codex thread name; Grok's official -runtime generates the title inside its own ACP session impl -(`xai-grok-shell/src/session/acp_session_impl/title_refresh.rs`) and pushes one update per -session, which the `acp-extension-grok` proxy forwards untouched. The shared -`acpOwnsSessionTitleGeneration()` predicate keeps `title-generator.ts`'s isolated session out -of their title path and hides the obsolete provider title settings for all three. - -How a title is labelled decides whether Lody may trust it, and that is a separate, -narrower set. Claude and Grok both send a bare `session_info_update` with no `_meta`, so they -need the `trustsUntaggedAcpSessionTitle()` allowlist. Codex tags every title and emits a -first-prompt `fallback` preview before its generated `explicit` one, so it must stay outside -that allowlist even though it does own its generation — otherwise the preview wins. +`session_info_update`; Kimi and the DeepSeek Harness still use `title-generator.ts` / +`response-utils.ts` and the `titleGeneration` config. `BUILTIN_ACP_TITLE_OWNERSHIP` in +`packages/shared/src/ai.ts` is the single table behind both facts, and its doc comment +carries the per-adapter mechanism; the audit evidence and what each remaining gap would +cost to close live in the [decision note](../../../../.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md). + +Two predicates read that table, and the difference between them is the part worth knowing. +`acpOwnsSessionTitleGeneration()` keeps the isolated session out of an agent's title path +and hides its obsolete title settings. `trustsUntaggedAcpSessionTitle()` is narrower: it +answers whether a pushed title may be stored without a `_meta.lody.titleSource` tag, which +is true only for the adapters that send no tag at all. Codex owns its generation but tags +every title and previews the raw first prompt as `fallback`, so trusting it untagged would +make that preview the session title. Branch naming never starts an isolated session. `titleToBranchName` is a pure transform, so the only thing an agent ever added was compressing the prompt into a shorter title first. -`generateBranchNameWithTimeout` now prefers a title that is already stored or in flight for -the session and otherwise converts the prompt directly, falling back to the prompt if a -pending title misses its budget. When no valid name can be derived — kebab conversion drops -every non-ASCII character, so this is the normal outcome for a Chinese prompt — the managed -`session/` branch is left alone rather than renamed to a timestamp. - -Kimi and the DeepSeek Harness still use `title-generator.ts` / `response-utils.ts` and the -`titleGeneration` config. Neither gap is a missing upstream feature: Kimi's -`session_info_update` carries the first prompt truncated to 200 chars with no `_meta` while -its real `SessionTitleService` stays reachable only from kap-server and the node SDK (the -engine's `SessionMeta.titleKind` is discarded at the ACP boundary), and the DeepSeek Harness -pins `@deepseek-ai/dsh-session-title` in its dependency closure but never mounts it in -`createDeepSeekHarnessCordisConfig`. +`deriveWorktreeBranchName` prefers a title already stored or in flight for the session and +otherwise converts the prompt directly, falling back to the prompt if a pending title misses +its budget. When no valid name can be derived — kebab conversion drops every non-ASCII +character, so this is the normal outcome for a Chinese prompt — the managed `session/` +branch is left alone rather than renamed to a timestamp. ### Local project identity diff --git a/apps/cli/src/agent/branch-name-generator.ts b/apps/cli/src/agent/branch-name-generator.ts index 0e236688a..d6ae53ed5 100644 --- a/apps/cli/src/agent/branch-name-generator.ts +++ b/apps/cli/src/agent/branch-name-generator.ts @@ -139,16 +139,13 @@ export const isValidGitBranchName = (name: string): boolean => { }; /** - * Ensure a branch name is valid, falling back to a safe default if not. + * Convert a title or prompt into a valid branch name, or null when it yields none. + * + * Kebab conversion drops every non-ASCII character, so a prompt written entirely + * in another script has no name to give. Callers are expected to leave the + * existing branch alone in that case rather than invent a meaningless one. */ -export const ensureValidBranchName = (name: string, fallbackPrefix: string = 'task'): string => { - const generated = titleToBranchName(name); - - if (generated && isValidGitBranchName(generated)) { - return generated; - } - - // Fallback: use a timestamp-based name - const timestamp = Date.now().toString(36); - return `${fallbackPrefix}/${timestamp}`; +export const tryBranchName = (base: string): string | null => { + const candidate = titleToBranchName(base); + return candidate && isValidGitBranchName(candidate) ? candidate : null; }; diff --git a/apps/cli/src/lib/message-handler.ts b/apps/cli/src/lib/message-handler.ts index d8a96a52d..4c280565f 100644 --- a/apps/cli/src/lib/message-handler.ts +++ b/apps/cli/src/lib/message-handler.ts @@ -254,7 +254,7 @@ import type { AcpAgentEditEvidence, AcpStandardDiffBlockEvidence } from '@/lib/a import { mergeAcpRuntimeConfigUpdates } from '@/lib/acp/runtime-config'; import { generateTitleIsolated, sanitizeTitle } from '@/agent/title-generator'; import type { AgentSessionWarning } from '@/agent/agent-client'; -import { isValidGitBranchName, titleToBranchName } from '@/agent/branch-name-generator'; +import { tryBranchName } from '@/agent/branch-name-generator'; import { SessionActivePresenceController, type SessionActivePresencePhase, @@ -8990,8 +8990,8 @@ export class MessageHandler { runtimeOverrides?: BuiltinRuntimeOverrides, titleConfig?: TitleGenerationConfig ): Promise { - // Builtin Claude and Codex generate their own titles and publish them as - // session_info_update; starting the isolated agent would only duplicate them. + // Builtin Claude, Codex and Grok generate their own titles and publish them + // as session_info_update; the isolated agent would only duplicate that work. if (acpOwnsSessionTitleGeneration(cliType, agentType)) { return; } @@ -9705,7 +9705,7 @@ export class MessageHandler { ); } - const branchName = await this.generateBranchNameWithTimeout( + const branchName = await this.deriveWorktreeBranchName( trimmedPrompt, 20_000, reusableTitlePromise @@ -9762,45 +9762,25 @@ export class MessageHandler { * ever added here was compressing the prompt into a shorter title first. A title * is still preferred when one is already stored or in flight for this session * (agents that keep the local generator produce one anyway); otherwise the prompt - * names the branch directly. - * - * Returns null when no valid name can be derived — a prompt with no ASCII words - * (kebab conversion strips everything else) leaves the managed `session/` - * branch alone rather than renaming it to a meaningless timestamp. + * names the branch directly. A slow or failed title never blocks or cancels the + * rename, because the prompt is always an acceptable naming input. */ - private async generateBranchNameWithTimeout( + private async deriveWorktreeBranchName( taskPrompt: string, timeoutMs: number, reusableTitlePromise?: Promise ): Promise { - const toBranchName = (base: string): string | null => { - const candidate = titleToBranchName(base); - return candidate && isValidGitBranchName(candidate) ? candidate : null; - }; - - if (!reusableTitlePromise) { - return toBranchName(taskPrompt); - } - - let timeoutHandle: NodeJS.Timeout | null = null; - const timeoutPromise = new Promise((resolve) => { - timeoutHandle = setTimeout(() => resolve(null), timeoutMs); - }); - try { - // A slow or failed title must not hold up (or cancel) the rename: the prompt - // is always available as the naming input. - const title = await Promise.race([reusableTitlePromise, timeoutPromise]); - return toBranchName(title?.trim() || taskPrompt); - } catch (error) { - this.logger.debug( - `[branch-name] Falling back to the prompt after title generation failed: ${formatErrorMessage(error)}` - ); - return toBranchName(taskPrompt); - } finally { - if (timeoutHandle) { - clearTimeout(timeoutHandle); + let title: string | null | undefined; + if (reusableTitlePromise) { + try { + title = await withTimeoutOrUndefined(reusableTitlePromise, timeoutMs); + } catch (error) { + this.logger.debug( + `[branch-name] Falling back to the prompt after title generation failed: ${formatErrorMessage(error)}` + ); } } + return tryBranchName(title?.trim() || taskPrompt); } private async notifySessionCompleted( diff --git a/apps/cli/tests/branch-name-generator.test.ts b/apps/cli/tests/branch-name-generator.test.ts index 1a526bf7d..abd40cb54 100644 --- a/apps/cli/tests/branch-name-generator.test.ts +++ b/apps/cli/tests/branch-name-generator.test.ts @@ -3,7 +3,7 @@ import { describe, expect, it } from 'vitest'; import { titleToBranchName, isValidGitBranchName, - ensureValidBranchName, + tryBranchName, } from '../src/agent/branch-name-generator'; describe('branch-name-generator', () => { @@ -96,19 +96,19 @@ describe('branch-name-generator', () => { }); }); - describe('ensureValidBranchName', () => { - it('returns generated branch name when valid', () => { - expect(ensureValidBranchName('Fix login bug')).toBe('fix/login-bug'); + describe('tryBranchName', () => { + it('returns the generated branch name when valid', () => { + expect(tryBranchName('Fix login bug')).toBe('fix/login-bug'); }); - it('returns fallback for invalid input', () => { - const result = ensureValidBranchName(''); - expect(result).toMatch(/^task\/[a-z0-9]+$/); + it('returns null for empty input', () => { + expect(tryBranchName('')).toBeNull(); }); - it('uses custom fallback prefix', () => { - const result = ensureValidBranchName('', 'session'); - expect(result).toMatch(/^session\/[a-z0-9]+$/); + // Kebab conversion strips every non-ASCII character, leaving nothing to name + // the branch after; the caller keeps the branch it already has. + it('returns null when the input has no ASCII words', () => { + expect(tryBranchName('把标题生成迁移到会话协议')).toBeNull(); }); }); }); diff --git a/apps/cli/tests/message-handler-title.test.ts b/apps/cli/tests/message-handler-title.test.ts index 06eab2154..3ade5f866 100644 --- a/apps/cli/tests/message-handler-title.test.ts +++ b/apps/cli/tests/message-handler-title.test.ts @@ -211,7 +211,7 @@ describe('MessageHandler title generation', () => { }); type BranchNameHost = { - generateBranchNameWithTimeout: ( + deriveWorktreeBranchName: ( taskPrompt: string, timeoutMs: number, reusableTitlePromise?: Promise @@ -220,7 +220,7 @@ describe('MessageHandler title generation', () => { it('prefers an already-available session title over the prompt', async () => { const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( + const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( 'Fallback prompt', 1_000, Promise.resolve('Fix title races') @@ -231,7 +231,7 @@ describe('MessageHandler title generation', () => { it('names the branch from the prompt when no title is available', async () => { const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( + const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( 'Add a retry to the upload queue', 1_000 ); @@ -245,7 +245,7 @@ describe('MessageHandler title generation', () => { // to a meaningless timestamp. it('returns no name when the prompt has no ASCII words', async () => { const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( + const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( '把标题生成迁移到会话协议', 1_000 ); @@ -255,7 +255,7 @@ describe('MessageHandler title generation', () => { it('falls back to the prompt when the title does not arrive in time', async () => { const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).generateBranchNameWithTimeout( + const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( 'Fix the flaky login redirect', 10, new Promise(() => {}) @@ -384,59 +384,36 @@ describe('MessageHandler title generation', () => { ); }); - it('skips isolated generation for builtin Claude', async () => { - const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { - agentConfigId: 'agent-config-1', - }); - - const titleHost = handler as unknown as { - maybeGenerateAndStoreSessionTitle: ( - sessionId: SessionId, - cliType: string, - agentType: string, - taskPrompt: string - ) => Promise; - }; - await titleHost.maybeGenerateAndStoreSessionTitle( - 's-8' as SessionId, - 'builtin', - 'claude', - 'Do something cool' - ); - - expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); - expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); - }); - - // Codex generates its title on an ephemeral thread and Grok's runtime pushes one - // per session; either way the isolated generator would only duplicate that work, - // and must not read the agent config to do it. - it.each(['codex', 'grok'])('skips isolated generation for builtin %s', async (agentType) => { - const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { - agentConfigId: 'agent-config-1', - agentConfigMeta: { - titleGeneration: { configOptionValues: { model: 'gpt-5.1-codex' } }, - }, - }); - - const titleHost = handler as unknown as { - maybeGenerateAndStoreSessionTitle: ( - sessionId: SessionId, - cliType: string, - agentType: string, - taskPrompt: string - ) => Promise; - }; - await titleHost.maybeGenerateAndStoreSessionTitle( - 's-acp-owned' as SessionId, - 'builtin', - agentType, - 'Do something cool' - ); - - expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); - expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); - }); + // Claude asks the Agent SDK, Codex generates on an ephemeral thread and Grok's + // runtime pushes one per session; either way the isolated generator would only + // duplicate that work, and must not read the agent config to do it. + it.each(['claude', 'codex', 'grok'])( + 'skips isolated generation for builtin %s', + async (agentType) => { + const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { + agentConfigId: 'agent-config-1', + agentConfigMeta: { titleGeneration: { configOptionValues: { model: 'stale-model' } } }, + }); + + const titleHost = handler as unknown as { + maybeGenerateAndStoreSessionTitle: ( + sessionId: SessionId, + cliType: string, + agentType: string, + taskPrompt: string + ) => Promise; + }; + await titleHost.maybeGenerateAndStoreSessionTitle( + 's-acp-owned' as SessionId, + 'builtin', + agentType, + 'Do something cool' + ); + + expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); + expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); + } + ); it('filters Lody internal prompt instructions before storing an ACP title', async () => { const { handler, sessionDoc } = await createHandler(undefined); diff --git a/packages/components/tests/agent-config-dialog.test.tsx b/packages/components/tests/agent-config-dialog.test.tsx index ff3b6a9d3..12b3f0ef2 100644 --- a/packages/components/tests/agent-config-dialog.test.tsx +++ b/packages/components/tests/agent-config-dialog.test.tsx @@ -53,7 +53,8 @@ const createMachine = ( }, }); -const createKimiMachine = (): MachineViewMeta => ({ +/** A machine whose cached capabilities expose title-eligible config options. */ +const createTitleConfigMachine = (): MachineViewMeta => ({ ...createMachine('Kimi workstation'), acpCapabilities: { [getAcpCapabilityCacheKey(kimiConfigId)]: { @@ -103,6 +104,18 @@ const createKimiMachine = (): MachineViewMeta => ({ }, }); +const createBuiltinConfig = (overrides: Partial = {}): AgentConfigMeta => + ({ + id: kimiConfigId, + machineId, + name: 'Kimi', + description: undefined, + cliType: 'builtin', + agentType: 'kimi', + env: {}, + ...overrides, + }) as AgentConfigMeta; + const getOptionButtons = (): HTMLButtonElement[] => Array.from(document.body.querySelectorAll('button[role="option"]')); @@ -1025,17 +1038,10 @@ describe('AgentConfigDialog', () => { it.each(['claude', 'codex', 'grok'])( 'hides the title generation section for builtin %s', async (agentType) => { - const config = { - id: kimiConfigId, - machineId, - name: 'ACP-owned', - description: undefined, - cliType: 'builtin', - agentType, - env: {}, - } as AgentConfigMeta; - - await renderDialog({ kind: 'edit', config }, createKimiMachine()); + await renderDialog( + { kind: 'edit', config: createBuiltinConfig({ name: 'ACP-owned', agentType }) }, + createTitleConfigMachine() + ); expect(document.body.textContent).not.toContain('Title generation'); } @@ -1043,23 +1049,16 @@ describe('AgentConfigDialog', () => { it('saves a normalized title reasoning effort after the title model changes', async () => { const onSubmit = vi.fn(async () => {}); - const config = { - id: kimiConfigId, - machineId, - name: 'Kimi', - description: undefined, - cliType: 'builtin', - agentType: 'kimi', - env: {}, + const config = createBuiltinConfig({ titleGeneration: { configOptionValues: { model: 'kimi-k2-turbo', reasoning_effort: 'ultra', }, }, - } as AgentConfigMeta; + }); - await renderDialog({ kind: 'edit', config }, createKimiMachine(), onSubmit); + await renderDialog({ kind: 'edit', config }, createTitleConfigMachine(), onSubmit); expect(document.body.textContent).toContain('Title generation'); diff --git a/packages/shared/src/ai.ts b/packages/shared/src/ai.ts index 98bd524d0..e9dbf0eb1 100644 --- a/packages/shared/src/ai.ts +++ b/packages/shared/src/ai.ts @@ -41,48 +41,60 @@ export type BuiltinAgentType = BuiltinAgent['agentType']; export type AgentConfigCliType = 'builtin' | 'registry' | 'custom'; export type AgentType = string; -/** Builtin agents whose ACP adapter generates the session title itself. */ -const ACP_TITLE_OWNING_AGENTS = new Set(['claude', 'codex', 'grok']); +/** + * How each builtin agent's ACP adapter handles the session title. + * + * - `none` — no usable title over ACP, so Lody runs its isolated title agent and + * keeps the title-generation config for it. Kimi's pushed title is only the + * first prompt truncated to 200 chars; the Harness never mounts its upstream + * title plugin. + * - `untagged` — pushes one authoritative `session_info_update` carrying no + * `_meta`, so it can only be trusted on identity. Claude asks the Agent SDK via + * its `generate_session_title` control request; Grok's official runtime + * generates one in its own ACP session impl and the proxy forwards it untouched. + * - `tagged` — labels every title with `_meta.lody.titleSource`, so only an + * `explicit` one may be stored. Codex (>= 1.8.0) emits a first-prompt `fallback` + * preview before its generated title, and storing that would make the raw prompt + * the session title. + * + * Exhaustive on purpose: adding a builtin agent must not silently default it. + */ +const BUILTIN_ACP_TITLE_OWNERSHIP: Record = { + claude: 'untagged', + codex: 'tagged', + grok: 'untagged', + kimi: 'none', + deepseek: 'none', +}; -/** Of those, the ones that push a title carrying no `_meta.lody.titleSource`. */ -const UNTAGGED_TITLE_AGENTS = new Set(['claude', 'grok']); +const builtinAcpTitleOwnership = ( + cliType: AgentConfigCliType | null | undefined, + agentType: AgentType | null | undefined +): 'none' | 'untagged' | 'tagged' => + cliType === 'builtin' && agentType && isBuiltinAgentType(agentType) + ? BUILTIN_ACP_TITLE_OWNERSHIP[agentType] + : 'none'; /** * Builtin ACP adapters that generate their own session titles, so Lody never - * starts its isolated title agent for them and hides the title-generation - * config from their agent settings. - * - * - `claude`: acp-extension-claude asks the Agent SDK for a real title via the - * `generate_session_title` control request and publishes it at turn end. - * - `codex`: acp-extension-codex (>= 1.8.0) runs its own cheap-model generation - * on an ephemeral thread, persists it as the codex thread name, and publishes - * it tagged `_meta.lody.titleSource: 'explicit'`. - * - `grok`: the official runtime generates the title in its own ACP session impl - * and pushes one `session_info_update` per session, which acp-extension-grok - * forwards untouched. - * - * Kimi and the DeepSeek Harness still depend on the isolated generator: Kimi's - * pushed title is only the first prompt truncated to 200 chars, and the Harness - * never mounts its upstream title plugin. + * starts its isolated title agent for them and hides the title-generation config + * from their agent settings. */ export const acpOwnsSessionTitleGeneration = ( cliType: AgentConfigCliType | null | undefined, agentType: AgentType | null | undefined -): boolean => cliType === 'builtin' && !!agentType && ACP_TITLE_OWNING_AGENTS.has(agentType); +): boolean => builtinAcpTitleOwnership(cliType, agentType) !== 'none'; /** * Adapters whose pushed titles are authoritative without a `titleSource` tag. * - * Deliberately narrower than {@link acpOwnsSessionTitleGeneration}: Claude and - * Grok both publish a bare `session_info_update` with no `_meta`, so they need - * an allowlist. Codex tags every title and emits a first-prompt `fallback` - * preview before its generated `explicit` one, so it must stay out — widening - * this predicate would turn that preview into the session title. + * Deliberately narrower than {@link acpOwnsSessionTitleGeneration}, and narrower + * by construction rather than by a second list kept in sync by hand. */ export const trustsUntaggedAcpSessionTitle = ( cliType: AgentConfigCliType | null | undefined, agentType: AgentType | null | undefined -): boolean => cliType === 'builtin' && !!agentType && UNTAGGED_TITLE_AGENTS.has(agentType); +): boolean => builtinAcpTitleOwnership(cliType, agentType) === 'untagged'; /** * User-defined ACP launch spec for `cliType: 'custom'` providers: the exact diff --git a/packages/shared/tests/title-generation-defaults.test.ts b/packages/shared/tests/title-generation-defaults.test.ts index 0971b26e7..afdd2f1d9 100644 --- a/packages/shared/tests/title-generation-defaults.test.ts +++ b/packages/shared/tests/title-generation-defaults.test.ts @@ -131,10 +131,10 @@ describe('acpOwnsSessionTitleGeneration', () => { expect(acpOwnsSessionTitleGeneration('builtin', 'deepseek')).toBe(false); }); - it('never applies to registry or custom providers', () => { + it('never applies to registry, custom, or unknown agents', () => { expect(acpOwnsSessionTitleGeneration('registry', 'codex')).toBe(false); expect(acpOwnsSessionTitleGeneration('custom', 'claude')).toBe(false); - expect(acpOwnsSessionTitleGeneration('custom', 'grok')).toBe(false); + expect(acpOwnsSessionTitleGeneration('builtin', 'not-an-agent')).toBe(false); }); }); @@ -154,6 +154,16 @@ describe('trustsUntaggedAcpSessionTitle', () => { expect(trustsUntaggedAcpSessionTitle('builtin', 'codex')).toBe(false); }); + // The trusted set is a subset of the owning set by construction, not by two + // lists kept in sync; assert the relation rather than restating the members. + it('never trusts an agent that does not own its title generation', () => { + for (const agentType of ['claude', 'codex', 'grok', 'kimi', 'deepseek', 'nope']) { + if (trustsUntaggedAcpSessionTitle('builtin', agentType)) { + expect(acpOwnsSessionTitleGeneration('builtin', agentType)).toBe(true); + } + } + }); + it('never applies to registry or custom providers', () => { expect(trustsUntaggedAcpSessionTitle('registry', 'claude')).toBe(false); expect(trustsUntaggedAcpSessionTitle('custom', 'claude')).toBe(false); From c80160adb291050fbf351bc5b22387e1520451e5 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Thu, 10 Sep 2026 07:09:05 +0000 Subject: [PATCH 09/11] fix(cli): keep prompt credentials out of branch names, and honour runtime overrides Two review findings on this branch. Naming a branch after the prompt publishes the prompt: a ref reaches the remote as soon as the session opens a PR, and "rotate sk_live_... before Friday" is an ordinary request. This was reachable before this branch too -- the old generateTitleIsolated returned sanitizeGeneratedTitle(taskPrompt) on every failure path -- but skipping title generation for the three ACP-owned agents turned a rare fallback into the common path. tryBranchName now strips credential-shaped tokens first: known prefixes (sk_, ghp_, AKIA, xox*), PEM blocks, and unprefixed runs of 20+ alphanumerics mixing letters and digits, which catches hex and base62 secrets while leaving prose alone. Stripping beats refusing -- "Fix API key sk_live_..." still yields fix/api-key -- and over-matching only shortens a branch name. BUILTIN_ACP_TITLE_OWNERSHIP describes the managed runtime each agent normally launches, but BuiltinRuntimeOverrides can aim the same agentType at any executable, including one predating the title behaviour: Grok's generation lives in the runtime itself and Codex's needs an ephemeral thread older builds lack. Such a session got no title at all -- generator skipped, nothing pushed, and the setting that would fix it hidden. acpOwnsSessionTitleGeneration now returns false while an override is set. The trust gate is deliberately unchanged: an override that does push a good title still gets it, as Claude already did before this branch. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 25 ++++++++++++++++ apps/cli/src/agent/AGENTS.md | 16 +++++----- apps/cli/src/agent/README.md | 11 +++++++ apps/cli/src/agent/branch-name-generator.ts | 29 ++++++++++++++++++- apps/cli/src/lib/message-handler.ts | 2 +- apps/cli/tests/branch-name-generator.test.ts | 27 +++++++++++++++++ .../settings/agent-config-dialog.tsx | 6 +++- packages/shared/src/ai.ts | 16 ++++++++-- .../tests/title-generation-defaults.test.ts | 21 ++++++++++++++ 9 files changed, 140 insertions(+), 13 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index fcfbe89db..a0b1ab133 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -188,6 +188,31 @@ generator was asked for a title in the prompt's own language. The isolated sessi those sessions paid for could never have produced a usable branch name. A timeout now also falls back to the prompt instead of abandoning the rename. +Two issues found in review after the first implementation landed, both fixed here. + +Naming a branch after the prompt publishes the prompt. A ref reaches the remote as +soon as the session opens a PR, and asking an agent to "rotate sk_live_… before +Friday" is ordinary. This was reachable before this branch too — the old +`generateTitleIsolated` returned `sanitizeGeneratedTitle(taskPrompt)` on every +failure path — but it went from a rare fallback to the common path for the three +ACP-owned agents, so the exposure changed in kind. `tryBranchName` now strips +credential-shaped tokens before deriving a name: known prefixes (`sk_`, `ghp_`, +`AKIA`, `xox…`, PEM blocks) plus unprefixed runs of 20+ alphanumerics containing +both letters and digits, which catches hex and base62 tokens while leaving English +prose untouched. Stripping beats refusing: "Fix API key sk_live_…" still yields +`fix/api-key`. Over-matching costs only a shorter branch name. + +Ownership also had to account for `BuiltinRuntimeOverrides`. The table describes the +managed runtime each agent normally launches, but an override can aim the same +`agentType` at any executable, including one predating the title behaviour — Grok's +title generation lives in the runtime itself, and Codex's needs an `ephemeral` thread +its older builds lack. Such a session got no title at all: the isolated generator was +skipped, nothing arrived over ACP, and the setting that would have fixed it was +hidden. `acpOwnsSessionTitleGeneration` now returns false whenever an override is +active, restoring the local generator and the config for it. The trust gate is +deliberately unchanged: an override that does push a good title still gets it, and +Claude behaved this way before this branch. + One residual inconsistency is known and left alone: `acpOwnsSessionTitleGeneration` gates the settings dialog, but `lody agent-config` and the onboarding provider screen still accept and persist a `titleGeneration` block for these agents. The diff --git a/apps/cli/src/agent/AGENTS.md b/apps/cli/src/agent/AGENTS.md index ecdf688d7..0b1137649 100644 --- a/apps/cli/src/agent/AGENTS.md +++ b/apps/cli/src/agent/AGENTS.md @@ -105,11 +105,11 @@ context/acp-agent-edit-evidence.md; adapter repos: [apps/cli/AGENTS.md](../../AG the cache, and requests/responses carry that id to keep configs of one provider isolated. `ManagedRuntimeUpdateCoordinator` never hot-swaps a running ACP process, and Machine Flock writes ignore `fetchedAt` when comparing entries. -- Builtin Claude, Codex and Grok own session titles via ACP `session_info_update` - (`acpOwnsSessionTitleGeneration()`): store them only after - `sanitizeLodyInternalInstructions`, never via `title-generator.ts`. - Claude and Grok push untagged and are trusted so (`trustsUntaggedAcpSessionTitle()`); - Codex is not — take only its `explicit` `_meta.lody.titleSource`, ignore its first-prompt - `fallback`, and require `_meta.lody.messagePhase === 'final_answer'`. Untyped chunks, - error/warning payloads, and internal-instruction tails never qualify. Each isolated run - owns and removes a temp dir; branch naming never starts one. +- Builtin Claude, Codex and Grok own session titles via `session_info_update` + (`acpOwnsSessionTitleGeneration()`) unless a runtime override is set: store them only after + `sanitizeLodyInternalInstructions`, never via `title-generator.ts`. Claude and Grok push + untagged and are trusted (`trustsUntaggedAcpSessionTitle()`); Codex is not — take only its + `explicit` `_meta.lody.titleSource` with `messagePhase === 'final_answer'`, not its + first-prompt `fallback`. Untyped chunks, error/warning payloads and instruction tails never + qualify. Each isolated run owns and removes a temp dir; branch naming starts none and strips + credential-shaped prompt tokens. diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index db8f61f83..19f439a59 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -201,6 +201,11 @@ is true only for the adapters that send no tag at all. Codex owns its generation every title and previews the raw first prompt as `fallback`, so trusting it untagged would make that preview the session title. +A runtime override revokes ownership. `BuiltinRuntimeOverrides` can aim the same +`agentType` at an executable predating the title behaviour, and that session would otherwise +get no title at all — generator skipped, nothing pushed, and the setting that would fix it +hidden — so an overridden runtime keeps the local generator. + Branch naming never starts an isolated session. `titleToBranchName` is a pure transform, so the only thing an agent ever added was compressing the prompt into a shorter title first. `deriveWorktreeBranchName` prefers a title already stored or in flight for the session and @@ -209,6 +214,12 @@ its budget. When no valid name can be derived — kebab conversion drops every n character, so this is the normal outcome for a Chinese prompt — the managed `session/` branch is left alone rather than renamed to a timestamp. +Naming a branch after a prompt publishes the prompt: a ref reaches the remote as soon as the +session opens a PR, and "rotate sk_live_… before Friday" is an ordinary request. `tryBranchName` +therefore strips credential-shaped tokens — known prefixes, PEM blocks, and unprefixed runs of +20+ alphanumerics mixing letters and digits — before deriving the name. Over-matching is the +safe direction; the cost is a shorter branch name. + ### Local project identity For local project sessions, `SessionManager` supplies a resolver for the original diff --git a/apps/cli/src/agent/branch-name-generator.ts b/apps/cli/src/agent/branch-name-generator.ts index d6ae53ed5..95b4c775b 100644 --- a/apps/cli/src/agent/branch-name-generator.ts +++ b/apps/cli/src/agent/branch-name-generator.ts @@ -138,6 +138,33 @@ export const isValidGitBranchName = (name: string): boolean => { return true; }; +/** + * Credential-shaped tokens, removed before a branch name is derived. + * + * A branch name is a ref: it is written to `.git`, shown in the UI, and pushed to + * the remote when the session opens a PR. Naming a branch after a prompt therefore + * publishes whatever the prompt contained, and "rotate sk_live_… before Friday" is + * an ordinary thing to ask an agent. Stripping beats refusing outright, because + * "Fix API key sk_live_…" still yields a useful `fix/api-key`. + * + * The last alternative catches unprefixed high-entropy tokens: a run of at least + * 20 alphanumerics containing both letters and digits. English prose has no such + * runs, while hex and base62 credentials do. Over-matching is safe here — the + * worst case is a slightly shorter branch name. + */ +const CREDENTIAL_LIKE_TOKEN = new RegExp( + [ + // Whole block first, so a short body cannot escape between the markers. + '-----BEGIN[\\s\\S]*?-----END[\\s\\S]*?-----', + '-----BEGIN[\\s\\S]*?-----', + '\\b(?:sk|pk|rk|ghp|gho|ghu|ghs|ghr|glpat|shpat|xox[abprs])[-_][A-Za-z0-9_-]{6,}', + '\\bgithub_pat_[A-Za-z0-9_]{10,}', + '\\b(?:AKIA|ASIA|AIza)[A-Za-z0-9]{6,}', + '\\b(?=[A-Za-z0-9]*[0-9])(?=[A-Za-z0-9]*[A-Za-z])[A-Za-z0-9]{20,}\\b', + ].join('|'), + 'g' +); + /** * Convert a title or prompt into a valid branch name, or null when it yields none. * @@ -146,6 +173,6 @@ export const isValidGitBranchName = (name: string): boolean => { * existing branch alone in that case rather than invent a meaningless one. */ export const tryBranchName = (base: string): string | null => { - const candidate = titleToBranchName(base); + const candidate = titleToBranchName(base.replace(CREDENTIAL_LIKE_TOKEN, ' ')); return candidate && isValidGitBranchName(candidate) ? candidate : null; }; diff --git a/apps/cli/src/lib/message-handler.ts b/apps/cli/src/lib/message-handler.ts index 4c280565f..7c1c99cac 100644 --- a/apps/cli/src/lib/message-handler.ts +++ b/apps/cli/src/lib/message-handler.ts @@ -8992,7 +8992,7 @@ export class MessageHandler { ): Promise { // Builtin Claude, Codex and Grok generate their own titles and publish them // as session_info_update; the isolated agent would only duplicate that work. - if (acpOwnsSessionTitleGeneration(cliType, agentType)) { + if (acpOwnsSessionTitleGeneration(cliType, agentType, runtimeOverrides)) { return; } const existingGeneration = this.titleGenerationInFlight.get(sessionId); diff --git a/apps/cli/tests/branch-name-generator.test.ts b/apps/cli/tests/branch-name-generator.test.ts index abd40cb54..ca01b3d7e 100644 --- a/apps/cli/tests/branch-name-generator.test.ts +++ b/apps/cli/tests/branch-name-generator.test.ts @@ -110,5 +110,32 @@ describe('branch-name-generator', () => { it('returns null when the input has no ASCII words', () => { expect(tryBranchName('把标题生成迁移到会话协议')).toBeNull(); }); + + // A branch name is a ref: it reaches the remote when the session opens a PR, + // so a prompt that mentions a credential must not publish it. + it.each([ + ['sk_live_ABC123def456', 'Fix API key sk_live_ABC123def456', 'fix/api-key'], + ['sk-proj-', 'rotate sk-proj-9aBcDeFgHiJkLmNoPqRs now', 'feat/rotate-now'], + ['ghp_', 'update ghp_16C7e42F292c6912E7710c838347Ae178B4a', 'chore/update'], + ['AKIA', 'aws creds AKIAIOSFODNN7EXAMPLE leaked', 'feat/aws-creds-leaked'], + ['bare hex', 'token is 0123456789abcdef0123456789abcdef', 'feat/token-is'], + [ + 'PEM block', + 'paste of -----BEGIN RSA PRIVATE KEY----- MIIEow -----END RSA PRIVATE KEY----- here', + 'feat/paste-of-here', + ], + ])('strips a %s credential before naming the branch', (_label, prompt, expected) => { + const branch = tryBranchName(prompt); + expect(branch).toBe(expected); + }); + + it('leaves ordinary prompts intact', () => { + expect(tryBranchName('Fix crash when opening FooBar with empty input')).toBe( + 'fix/crash-when-opening-foobar-with-empty-input' + ); + expect(tryBranchName('Bump codex to 1.10.1 and grok to 0.1.3')).toBe( + 'chore/bump-codex-to-1101-and-grok-to-013' + ); + }); }); }); diff --git a/packages/components/src/components/settings/agent-config-dialog.tsx b/packages/components/src/components/settings/agent-config-dialog.tsx index 2ac8b00a3..fe7b8db48 100644 --- a/packages/components/src/components/settings/agent-config-dialog.tsx +++ b/packages/components/src/components/settings/agent-config-dialog.tsx @@ -1014,7 +1014,11 @@ export function AgentConfigDialog(props: AgentConfigDialogProps) { const activePreset = formData.presetId ? PRESETS_BY_ID[formData.presetId] : undefined; const isPreset = !!activePreset; - const acpProvidesSessionTitle = acpOwnsSessionTitleGeneration(formData.cliType, formData.agentType); + const acpProvidesSessionTitle = acpOwnsSessionTitleGeneration( + formData.cliType, + formData.agentType, + formData.runtimeOverrides + ); const activeCredentialMode = activePreset ? getPresetCredentialMode(activePreset, formData.presetCredentialModeId) : undefined; diff --git a/packages/shared/src/ai.ts b/packages/shared/src/ai.ts index e9dbf0eb1..a1d229f91 100644 --- a/packages/shared/src/ai.ts +++ b/packages/shared/src/ai.ts @@ -79,11 +79,23 @@ const builtinAcpTitleOwnership = ( * Builtin ACP adapters that generate their own session titles, so Lody never * starts its isolated title agent for them and hides the title-generation config * from their agent settings. + * + * A runtime override revokes this. The table describes the managed runtime each + * agent normally launches, but `BuiltinRuntimeOverrides` can point the same + * `agentType` at any executable — including one predating the title behaviour. + * Such a session would otherwise get no title at all: the isolated generator is + * skipped, no title arrives over ACP, and the settings that would fix it are + * hidden. Keeping the local generator for overridden runtimes is the conservative + * side to be wrong on, and it costs only the duplicate work this change removed + * for the managed case. */ export const acpOwnsSessionTitleGeneration = ( cliType: AgentConfigCliType | null | undefined, - agentType: AgentType | null | undefined -): boolean => builtinAcpTitleOwnership(cliType, agentType) !== 'none'; + agentType: AgentType | null | undefined, + runtimeOverrides?: BuiltinRuntimeOverrides +): boolean => + !hasBuiltinRuntimeOverrideValues(runtimeOverrides) && + builtinAcpTitleOwnership(cliType, agentType) !== 'none'; /** * Adapters whose pushed titles are authoritative without a `titleSource` tag. diff --git a/packages/shared/tests/title-generation-defaults.test.ts b/packages/shared/tests/title-generation-defaults.test.ts index afdd2f1d9..33b0de18c 100644 --- a/packages/shared/tests/title-generation-defaults.test.ts +++ b/packages/shared/tests/title-generation-defaults.test.ts @@ -131,6 +131,27 @@ describe('acpOwnsSessionTitleGeneration', () => { expect(acpOwnsSessionTitleGeneration('builtin', 'deepseek')).toBe(false); }); + // The table describes each agent's managed runtime. An override can aim the + // same agentType at an older executable with no title behaviour, and such a + // session would otherwise get no title at all -- generator skipped, nothing + // pushed, and the setting that would fix it hidden. + it('gives ownership back to the local generator when a runtime is overridden', () => { + expect(acpOwnsSessionTitleGeneration('builtin', 'codex', { codexPath: '/opt/old-codex' })).toBe( + false + ); + expect(acpOwnsSessionTitleGeneration('builtin', 'grok', { grokPath: '/opt/old-grok' })).toBe( + false + ); + expect( + acpOwnsSessionTitleGeneration('builtin', 'claude', { claudeCodeExecutable: '/opt/old' }) + ).toBe(false); + }); + + it('ignores an override object with no usable value', () => { + expect(acpOwnsSessionTitleGeneration('builtin', 'codex', {})).toBe(true); + expect(acpOwnsSessionTitleGeneration('builtin', 'codex', { codexPath: ' ' })).toBe(true); + }); + it('never applies to registry, custom, or unknown agents', () => { expect(acpOwnsSessionTitleGeneration('registry', 'codex')).toBe(false); expect(acpOwnsSessionTitleGeneration('custom', 'claude')).toBe(false); From eb5c840f59bd00b8573d38e74f7aaf59b0c6adbe Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Thu, 10 Sep 2026 07:58:31 +0000 Subject: [PATCH 10/11] fix(cli): fail closed on credential syntax when naming a branch The previous commit stripped credential-shaped tokens and named the branch from what was left. That is the wrong shape and review caught it: a secret has no reliable shape -- hunter2 is a password and an ordinary word -- so removing what looks secret leaves everything that does not. Fix DB_PASSWORD=hunter2 and Fix https://alice:hunter2@example.com both survived verbatim into the ref. tryBranchName now fails closed. It matches the syntax that carries secrets rather than the secrets themselves -- a value assigned to a sensitive name, URL userinfo, known key prefixes, PEM blocks, and 20+ alphanumeric runs mixing letters and digits -- and returns null on any hit, leaving the session on its session/ branch. Failing closed is only affordable because it rarely fires on real work, so the tests pin both directions: nine credential syntaxes refused, and six prompts that merely mention auth, token, secret or credential still named. Measuring that corpus is also what caught the regex missing its `i` flag, which had let the uppercase DB_PASSWORD and AWS_SECRET_ACCESS_KEY cases through. Still best-effort, and the note says so: prose like "the password is hunter2" carries no syntax to match. The sound alternative -- never deriving a ref from prompt text -- would cost branch naming entirely for the three ACP-owned agents, since no generated title exists when the branch is named. Model: claude-opus-5[1m] --- .../2026-09-08-acp-owned-session-titles.md | 26 +++++++--- apps/cli/src/agent/AGENTS.md | 4 +- apps/cli/src/agent/README.md | 17 +++++-- apps/cli/src/agent/branch-name-generator.ts | 45 +++++++++-------- apps/cli/tests/branch-name-generator.test.ts | 49 +++++++++++-------- 5 files changed, 88 insertions(+), 53 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index a0b1ab133..a9372bf8d 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -195,12 +195,26 @@ soon as the session opens a PR, and asking an agent to "rotate sk_live_… befor Friday" is ordinary. This was reachable before this branch too — the old `generateTitleIsolated` returned `sanitizeGeneratedTitle(taskPrompt)` on every failure path — but it went from a rare fallback to the common path for the three -ACP-owned agents, so the exposure changed in kind. `tryBranchName` now strips -credential-shaped tokens before deriving a name: known prefixes (`sk_`, `ghp_`, -`AKIA`, `xox…`, PEM blocks) plus unprefixed runs of 20+ alphanumerics containing -both letters and digits, which catches hex and base62 tokens while leaving English -prose untouched. Stripping beats refusing: "Fix API key sk_live_…" still yields -`fix/api-key`. Over-matching costs only a shorter branch name. +ACP-owned agents, so the exposure changed in kind. + +The first attempt stripped credential-shaped tokens and kept naming the branch from +what was left. Review rejected it, correctly: a secret has no reliable shape — +`hunter2` is a password and an ordinary word — so a shape-based denylist removes +what looks secret and leaves everything that does not. `Fix DB_PASSWORD=hunter2` +and `Fix https://alice:hunter2@example.com` both survived it verbatim. + +`tryBranchName` now fails closed instead. It matches the *syntax* that carries +secrets rather than the secrets themselves — a value assigned to a sensitive name, +URL userinfo, known key prefixes, PEM blocks, and 20+ alphanumeric runs mixing +letters and digits — and returns null on any hit, leaving the session on its +`session/` branch. Failing closed is affordable only because it rarely fires on +real work, which the tests pin in both directions: nine credential syntaxes refused, +and six prompts that merely mention `auth`, `token`, `secret` or `credential` +still named. It remains best-effort, and the note is explicit about that: prose +like "the password is hunter2" carries no syntax to match. The sound alternative — +never deriving a ref from prompt text at all — would cost branch naming entirely +for the three ACP-owned agents, since no generated title exists when the branch is +named. Ownership also had to account for `BuiltinRuntimeOverrides`. The table describes the managed runtime each agent normally launches, but an override can aim the same diff --git a/apps/cli/src/agent/AGENTS.md b/apps/cli/src/agent/AGENTS.md index 0b1137649..8ca188f85 100644 --- a/apps/cli/src/agent/AGENTS.md +++ b/apps/cli/src/agent/AGENTS.md @@ -111,5 +111,5 @@ context/acp-agent-edit-evidence.md; adapter repos: [apps/cli/AGENTS.md](../../AG untagged and are trusted (`trustsUntaggedAcpSessionTitle()`); Codex is not — take only its `explicit` `_meta.lody.titleSource` with `messagePhase === 'final_answer'`, not its first-prompt `fallback`. Untyped chunks, error/warning payloads and instruction tails never - qualify. Each isolated run owns and removes a temp dir; branch naming starts none and strips - credential-shaped prompt tokens. + qualify. Each isolated run owns and removes a temp dir; branch naming starts none and skips + prompts carrying credential syntax. diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index 19f439a59..890dc7b33 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -215,10 +215,19 @@ character, so this is the normal outcome for a Chinese prompt — the managed `s branch is left alone rather than renamed to a timestamp. Naming a branch after a prompt publishes the prompt: a ref reaches the remote as soon as the -session opens a PR, and "rotate sk_live_… before Friday" is an ordinary request. `tryBranchName` -therefore strips credential-shaped tokens — known prefixes, PEM blocks, and unprefixed runs of -20+ alphanumerics mixing letters and digits — before deriving the name. Over-matching is the -safe direction; the cost is a shorter branch name. +session opens a PR, and "rotate the key before Friday" is an ordinary request. `tryBranchName` +therefore **fails closed** — on any credential signal it returns null and the session keeps its +`session/` branch. Stripping the offending token was tried first and abandoned: a secret has +no reliable shape (`hunter2` is both a password and an ordinary word), so removing what looks +secret-shaped leaves everything that does not. The signals are the *syntax* that carries +secrets — a value assigned to a sensitive name, URL userinfo, known key prefixes, PEM blocks, +and 20+ alphanumeric runs mixing letters and digits. + +This is best-effort, not a guarantee: prose like "the password is hunter2" carries no syntax to +match. It leans wide on purpose, because a false positive costs one branch name while a false +negative publishes a secret. The `it.each` tables in `tests/branch-name-generator.test.ts` pin +both directions — nine credential syntaxes refused, and six prompts that merely mention `auth`, +`token`, `secret` or `credential` still named. ### Local project identity diff --git a/apps/cli/src/agent/branch-name-generator.ts b/apps/cli/src/agent/branch-name-generator.ts index 95b4c775b..81de66c83 100644 --- a/apps/cli/src/agent/branch-name-generator.ts +++ b/apps/cli/src/agent/branch-name-generator.ts @@ -139,40 +139,45 @@ export const isValidGitBranchName = (name: string): boolean => { }; /** - * Credential-shaped tokens, removed before a branch name is derived. + * Signals that a prompt is carrying a credential. * * A branch name is a ref: it is written to `.git`, shown in the UI, and pushed to - * the remote when the session opens a PR. Naming a branch after a prompt therefore - * publishes whatever the prompt contained, and "rotate sk_live_… before Friday" is - * an ordinary thing to ask an agent. Stripping beats refusing outright, because - * "Fix API key sk_live_…" still yields a useful `fix/api-key`. + * the remote as soon as the session opens a PR. Naming a branch after a prompt + * therefore publishes whatever the prompt held, and "rotate the key before Friday" + * is an ordinary thing to ask an agent. * - * The last alternative catches unprefixed high-entropy tokens: a run of at least - * 20 alphanumerics containing both letters and digits. English prose has no such - * runs, while hex and base62 credentials do. Over-matching is safe here — the - * worst case is a slightly shorter branch name. + * This list cannot be complete, and is not meant to be: a secret has no reliable + * shape, since `hunter2` is both a password and an ordinary word. It recognizes + * the *syntax* that carries secrets rather than the secrets themselves, and the + * boundary fails closed on a hit — the session keeps its `session/` branch + * instead of getting a name derived from that prompt. A false positive costs one + * branch name, so the list leans deliberately wide. */ -const CREDENTIAL_LIKE_TOKEN = new RegExp( +const CREDENTIAL_SIGNAL = new RegExp( [ - // Whole block first, so a short body cannot escape between the markers. + // A value assigned to a sensitive name: DB_PASSWORD=…, "api key: …". + '(?:api[_-]?key|auth|bearer|credential|passwd|password|secret|token)\\w*\\s*[:=]\\s*\\S', + // URL userinfo: https://alice:hunter2@example.com + '[a-zA-Z][a-zA-Z0-9+.-]*://[^/\\s@]*:[^/\\s@]*@', + // Whole PEM block first, so a short body cannot escape between the markers. '-----BEGIN[\\s\\S]*?-----END[\\s\\S]*?-----', '-----BEGIN[\\s\\S]*?-----', + // Known credential prefixes. '\\b(?:sk|pk|rk|ghp|gho|ghu|ghs|ghr|glpat|shpat|xox[abprs])[-_][A-Za-z0-9_-]{6,}', '\\bgithub_pat_[A-Za-z0-9_]{10,}', '\\b(?:AKIA|ASIA|AIza)[A-Za-z0-9]{6,}', + // Unprefixed high-entropy run: 20+ alphanumerics mixing letters and digits. + // English prose has no such runs; hex and base62 credentials do. '\\b(?=[A-Za-z0-9]*[0-9])(?=[A-Za-z0-9]*[A-Za-z])[A-Za-z0-9]{20,}\\b', ].join('|'), - 'g' + // Case-insensitive: DB_PASSWORD and AWS_SECRET_ACCESS_KEY are how these appear. + 'i' ); -/** - * Convert a title or prompt into a valid branch name, or null when it yields none. - * - * Kebab conversion drops every non-ASCII character, so a prompt written entirely - * in another script has no name to give. Callers are expected to leave the - * existing branch alone in that case rather than invent a meaningless one. - */ export const tryBranchName = (base: string): string | null => { - const candidate = titleToBranchName(base.replace(CREDENTIAL_LIKE_TOKEN, ' ')); + if (CREDENTIAL_SIGNAL.test(base)) { + return null; + } + const candidate = titleToBranchName(base); return candidate && isValidGitBranchName(candidate) ? candidate : null; }; diff --git a/apps/cli/tests/branch-name-generator.test.ts b/apps/cli/tests/branch-name-generator.test.ts index ca01b3d7e..49ef6dc6f 100644 --- a/apps/cli/tests/branch-name-generator.test.ts +++ b/apps/cli/tests/branch-name-generator.test.ts @@ -112,30 +112,37 @@ describe('branch-name-generator', () => { }); // A branch name is a ref: it reaches the remote when the session opens a PR, - // so a prompt that mentions a credential must not publish it. + // so a prompt carrying a credential must not name the branch. Secrets have no + // reliable shape, so the boundary fails closed on the syntax that carries them + // and the session keeps its `session/` branch. it.each([ - ['sk_live_ABC123def456', 'Fix API key sk_live_ABC123def456', 'fix/api-key'], - ['sk-proj-', 'rotate sk-proj-9aBcDeFgHiJkLmNoPqRs now', 'feat/rotate-now'], - ['ghp_', 'update ghp_16C7e42F292c6912E7710c838347Ae178B4a', 'chore/update'], - ['AKIA', 'aws creds AKIAIOSFODNN7EXAMPLE leaked', 'feat/aws-creds-leaked'], - ['bare hex', 'token is 0123456789abcdef0123456789abcdef', 'feat/token-is'], + ['an assignment to a sensitive name', 'Fix DB_PASSWORD=hunter2'], + ['a lowercase assignment', 'debug with password: correcthorse'], + ['an AWS-style assignment', 'set AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI'], + ['URL userinfo', 'Fix https://alice:hunter2@example.com'], + ['an Authorization header', 'Authorization: Bearer eyJhbGciOiJIUzI1'], + ['a known key prefix', 'Fix API key sk_live_ABC123def456'], + ['a GitHub token', 'update ghp_16C7e42F292c6912E7710c838347Ae178B4a'], + ['an unprefixed hex secret', 'token is 0123456789abcdef0123456789abcdef'], [ - 'PEM block', - 'paste of -----BEGIN RSA PRIVATE KEY----- MIIEow -----END RSA PRIVATE KEY----- here', - 'feat/paste-of-here', + 'a PEM block', + 'paste -----BEGIN RSA PRIVATE KEY----- MIIEow -----END RSA PRIVATE KEY-----', ], - ])('strips a %s credential before naming the branch', (_label, prompt, expected) => { - const branch = tryBranchName(prompt); - expect(branch).toBe(expected); - }); - - it('leaves ordinary prompts intact', () => { - expect(tryBranchName('Fix crash when opening FooBar with empty input')).toBe( - 'fix/crash-when-opening-foobar-with-empty-input' - ); - expect(tryBranchName('Bump codex to 1.10.1 and grok to 0.1.3')).toBe( - 'chore/bump-codex-to-1101-and-grok-to-013' - ); + ])('refuses to name a branch after %s', (_label, prompt) => { + expect(tryBranchName(prompt)).toBeNull(); + }); + + // Failing closed is only affordable because it does not fire on ordinary work. + // These all mention a sensitive word without carrying a value. + it.each([ + ['Fix crash when opening FooBar with empty input', 'fix/crash-when-opening-foobar-with-empty-input'], + ['Bump codex to 1.10.1 and grok to 0.1.3', 'chore/bump-codex-to-1101-and-grok-to-013'], + ['Fix auth redirect loop after logout', 'fix/auth-redirect-loop-after-logout'], + ['Add a token bucket rate limiter', 'feat/a-token-bucket-rate-limiter'], + ['Support GITHUB_TOKEN in CI', 'feat/support-github-token-in-ci'], + ['Write tests for the credential broker', 'feat/write-tests-for-the-credential-broker'], + ])('still names a branch after %s', (prompt, expected) => { + expect(tryBranchName(prompt)).toBe(expected); }); }); }); From 81fba159ce877b24139d7c401900268496dcc4a4 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Thu, 10 Sep 2026 09:15:51 +0000 Subject: [PATCH 11/11] fix(cli)!: remove the prompt-to-branch rename instead of filtering it A branch name is a ref: it reaches the remote as soon as the session opens a PR, so deriving one from prompt text publishes prompt text. Two filters were tried on this branch and both failed for the same reason. Stripping credential-shaped tokens leaves everything that does not look like one -- a secret has no reliable shape, since hunter2 is a password and an ordinary word. Failing closed on credential syntax caught DB_PASSWORD=hunter2 and URL userinfo, but any prompt-based check fails open on every miss, so plain prose like "the password is hunter2" still published. A boundary that fails open is not a boundary, so the path is removed rather than filtered again. Deleted: maybeRenameSessionBranchFromPrompt and deriveWorktreeBranchName from MessageHandler, its SessionExecutionDeps entry and startSession call, branch-name-generator.ts entirely, and the renameBranchWithAvailableSuffix / listLocalBranchNames / isManagedWorktreeBranchName exports it was the only caller of. resolveAvailableBranchName stays -- worktree-manager still allocates session branch names with it. Behaviour: a worktree session keeps the session/ branch worktree-manager gave it. Nothing is silently lost. syncSessionBranchName still records the session's real branch after every turn, so an agent that renames it is picked up, and GitHub-project prompts already ask the agent to name branches after the task. That instruction is not a replacement -- it is injected only in startSession, stripped before storage, and only for project.kind === 'github' -- which is recorded in the note. Restoring automatic naming needs a naming source provably isolated from the prompt. None exists at session-ready: the ACP title has not arrived, and the isolated generator's own fallback is the raw prompt. Model: claude-opus-5 --- .../2026-09-08-acp-owned-session-titles.md | 116 +++++------ apps/cli/src/agent/AGENTS.md | 16 +- apps/cli/src/agent/README.md | 38 ++-- apps/cli/src/agent/branch-name-generator.ts | 183 ------------------ apps/cli/src/lib/message-handler.ts | 115 ----------- .../src/session/session-execution-service.ts | 13 -- .../worktree/branch-name-allocation.test.ts | 130 ------------- .../worktree/branch-name-allocation.ts | 71 ------- apps/cli/tests/branch-name-generator.test.ts | 148 -------------- apps/cli/tests/message-handler-title.test.ts | 89 --------- .../tests/session-execution-service.test.ts | 1 - 11 files changed, 76 insertions(+), 844 deletions(-) delete mode 100644 apps/cli/src/agent/branch-name-generator.ts delete mode 100644 apps/cli/tests/branch-name-generator.test.ts diff --git a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md index a9372bf8d..f3239a158 100644 --- a/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -17,9 +17,11 @@ required splitting the single `usesAcpProvidedSessionTitle()` predicate into ownership and trust, because Codex tags its titles and emits a prompt-preview `fallback` first while Claude and Grok push one bare authoritative title, and conflating the two would have promoted Codex's preview to the session title. The -cost is that title wording now belongs to the adapters. Branch naming, the last -caller that could still start an isolated session, now derives its name locally, -so session titles no longer start an extra ACP agent anywhere. +cost is that title wording now belongs to the adapters. Branch naming was the last +caller able to start an isolated session, and it is removed outright rather than +reimplemented locally: deriving a git ref from prompt text publishes the prompt, and +no prompt-based filter can prove a secret absent. Worktree sessions keep their +`session/` branch. ## The audit @@ -137,8 +139,7 @@ and Grok, as it already was for Claude. naming resolved the persisted `titleGeneration` for whatever agent it was naming a branch for, so a value stored before this change would have kept steering Claude, Codex and Grok runs after their config disappeared from the UI. That -lookup is gone outright: branch naming no longer reads the agent config for any -provider (see the branch-naming change below). +lookup is gone with the branch-naming path itself (below). ## Trade-offs and limits @@ -156,65 +157,52 @@ keeps refining its title over the first few turns before freezing it, so a Grok session title can change after it first appears. Branch naming had to change too, or the isolated session would simply have moved -from the title path to the branch path. `maybeRenameSessionBranchFromPrompt` runs -at session-ready, before any turn, so an ACP title can never have arrived by then; -with the title path skipped it would have started its own agent, leaving worktree -sessions at exactly the same one isolated session as before. - -Two options were considered and rejected. Deferring the rename until the pushed -title arrives moves a "once, at session creation" operation into the middle of a -running conversation, where a turn may already have pushed the branch or opened a -PR — `renameBranchWithAvailableSuffix` is a bare `git branch -m` with no upstream -check. Dropping branch naming entirely and relying on the injected instruction -("Name branches based on the task content", `session-execution-helpers.ts`) fails -because `buildPrompt` runs only in `startSession`, and the instruction is stripped -before storage, so it is absent from turn two onward and from every resumed -session — trading a deterministic behaviour for one whose odds fall as the session -grows. It is also injected only for `project.kind === 'github'`, while worktrees -are also created for local projects with `useWorktree`. - -What actually landed is simpler: `titleToBranchName` was always a pure transform, -so the isolated agent only ever compressed the prompt into a shorter title first. -Branch naming now prefers a title that is already stored or in flight, and -otherwise converts the prompt directly. `deriveWorktreeBranchName` no longer -takes a provider, env, launch spec or title config, and the branch path no longer -reads the agent config at all. - -One deliberate behaviour change: a prompt that yields no valid name now leaves the -managed `session/` branch alone instead of renaming it to `task/`. -Kebab conversion strips every non-ASCII character, so this is the normal outcome -for a Chinese prompt — and it was the outcome before this change too, since the -generator was asked for a title in the prompt's own language. The isolated session -those sessions paid for could never have produced a usable branch name. A timeout -now also falls back to the prompt instead of abandoning the rename. - -Two issues found in review after the first implementation landed, both fixed here. - -Naming a branch after the prompt publishes the prompt. A ref reaches the remote as -soon as the session opens a PR, and asking an agent to "rotate sk_live_… before -Friday" is ordinary. This was reachable before this branch too — the old -`generateTitleIsolated` returned `sanitizeGeneratedTitle(taskPrompt)` on every -failure path — but it went from a rare fallback to the common path for the three -ACP-owned agents, so the exposure changed in kind. - -The first attempt stripped credential-shaped tokens and kept naming the branch from -what was left. Review rejected it, correctly: a secret has no reliable shape — -`hunter2` is a password and an ordinary word — so a shape-based denylist removes -what looks secret and leaves everything that does not. `Fix DB_PASSWORD=hunter2` -and `Fix https://alice:hunter2@example.com` both survived it verbatim. - -`tryBranchName` now fails closed instead. It matches the *syntax* that carries -secrets rather than the secrets themselves — a value assigned to a sensitive name, -URL userinfo, known key prefixes, PEM blocks, and 20+ alphanumeric runs mixing -letters and digits — and returns null on any hit, leaving the session on its -`session/` branch. Failing closed is affordable only because it rarely fires on -real work, which the tests pin in both directions: nine credential syntaxes refused, -and six prompts that merely mention `auth`, `token`, `secret` or `credential` -still named. It remains best-effort, and the note is explicit about that: prose -like "the password is hunter2" carries no syntax to match. The sound alternative — -never deriving a ref from prompt text at all — would cost branch naming entirely -for the three ACP-owned agents, since no generated title exists when the branch is -named. +from the title path to the branch path. `maybeRenameSessionBranchFromPrompt` ran at +session-ready, before any turn, so an ACP title can never have arrived by then; with +the title path skipped it would have started its own agent, leaving worktree sessions +at exactly the same one isolated session as before. It took three attempts to land, +and the first two are recorded because each looks reasonable until you see why it +fails. + +Deferring the rename until the pushed title arrives was rejected first: it moves a +"once, at session creation" operation into the middle of a running conversation, +where a turn may already have pushed the branch or opened a PR, and +`renameBranchWithAvailableSuffix` was a bare `git branch -m` with no upstream check. + +Deriving the name locally from the prompt landed next, and review found it publishes +secrets. A branch name is a ref: it reaches the remote as soon as the session opens a +PR, and asking an agent to "rotate the password before Friday" is ordinary. This was +reachable before this branch too — the old `generateTitleIsolated` returned +`sanitizeGeneratedTitle(taskPrompt)` on every failure path — but skipping title +generation for the three ACP-owned agents turned a rare fallback into the common path. + +Two filters were then tried, and both failed for the same reason. The first stripped +credential-shaped tokens and named the branch from what was left; a secret has no +reliable shape, since `hunter2` is a password and an ordinary word, so a shape-based +denylist removes what looks secret and keeps everything else — `Fix DB_PASSWORD=hunter2` +and `Fix https://alice:hunter2@example.com` both survived it verbatim. The second +failed closed on credential *syntax* (an assignment to a sensitive name, URL userinfo, +known prefixes, PEM blocks, high-entropy runs) and caught those two, but any +prompt-based check fails *open* on every miss, so plain prose like "the password is +hunter2" still published. A boundary that fails open is not a boundary. + +So the path was removed rather than filtered a third time. +`maybeRenameSessionBranchFromPrompt`, `deriveWorktreeBranchName`, +`branch-name-generator.ts` and the now-unreachable `renameBranchWithAvailableSuffix` / +`isManagedWorktreeBranchName` are deleted. A worktree session keeps the `session/` +branch `worktree-manager.ts` gave it. Nothing is silently lost: `syncSessionBranchName` +still records the session's real branch after every turn, so an agent that renames it +is picked up, and GitHub-project prompts already carry an instruction asking the agent +to name branches after the task (`GITHUB_WORKTREE_SYSTEM_COMMANDS`). That instruction +is not a replacement — `buildPrompt` runs only in `startSession` and the instruction is +stripped before storage, so it is absent from turn two onward and from resumed +sessions, and it is injected only for `project.kind === 'github'` while worktrees are +also created for local projects with `useWorktree`. + +Restoring automatic naming needs a source provably isolated from the prompt. None +exists at session-ready: the ACP title has not arrived yet, and the isolated +generator's own fallback is the raw prompt. A title known to be model-generated rather +than prompt-derived would qualify, but the current code cannot distinguish the two. Ownership also had to account for `BuiltinRuntimeOverrides`. The table describes the managed runtime each agent normally launches, but an override can aim the same diff --git a/apps/cli/src/agent/AGENTS.md b/apps/cli/src/agent/AGENTS.md index 8ca188f85..8a1f445d2 100644 --- a/apps/cli/src/agent/AGENTS.md +++ b/apps/cli/src/agent/AGENTS.md @@ -105,11 +105,11 @@ context/acp-agent-edit-evidence.md; adapter repos: [apps/cli/AGENTS.md](../../AG the cache, and requests/responses carry that id to keep configs of one provider isolated. `ManagedRuntimeUpdateCoordinator` never hot-swaps a running ACP process, and Machine Flock writes ignore `fetchedAt` when comparing entries. -- Builtin Claude, Codex and Grok own session titles via `session_info_update` - (`acpOwnsSessionTitleGeneration()`) unless a runtime override is set: store them only after - `sanitizeLodyInternalInstructions`, never via `title-generator.ts`. Claude and Grok push - untagged and are trusted (`trustsUntaggedAcpSessionTitle()`); Codex is not — take only its - `explicit` `_meta.lody.titleSource` with `messagePhase === 'final_answer'`, not its - first-prompt `fallback`. Untyped chunks, error/warning payloads and instruction tails never - qualify. Each isolated run owns and removes a temp dir; branch naming starts none and skips - prompts carrying credential syntax. +- Builtin Claude, Codex and Grok own session titles + (`acpOwnsSessionTitleGeneration()`) unless a runtime override is set; store only after + `sanitizeLodyInternalInstructions`. Claude and Grok push untagged and are trusted + (`trustsUntaggedAcpSessionTitle()`); Codex is not — only `explicit` `titleSource` with + `messagePhase === 'final_answer'` qualifies, never its first-prompt `fallback`. Untyped chunks, + error/warning payloads and instruction tails never qualify. +- NEVER derive a git ref from prompt text: refs reach the remote and no filter proves a + prompt secret-free. Worktree sessions keep `session/` unless the agent renames it. diff --git a/apps/cli/src/agent/README.md b/apps/cli/src/agent/README.md index 890dc7b33..e961d5c5b 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -206,28 +206,22 @@ A runtime override revokes ownership. `BuiltinRuntimeOverrides` can aim the same get no title at all — generator skipped, nothing pushed, and the setting that would fix it hidden — so an overridden runtime keeps the local generator. -Branch naming never starts an isolated session. `titleToBranchName` is a pure transform, so -the only thing an agent ever added was compressing the prompt into a shorter title first. -`deriveWorktreeBranchName` prefers a title already stored or in flight for the session and -otherwise converts the prompt directly, falling back to the prompt if a pending title misses -its budget. When no valid name can be derived — kebab conversion drops every non-ASCII -character, so this is the normal outcome for a Chinese prompt — the managed `session/` -branch is left alone rather than renamed to a timestamp. - -Naming a branch after a prompt publishes the prompt: a ref reaches the remote as soon as the -session opens a PR, and "rotate the key before Friday" is an ordinary request. `tryBranchName` -therefore **fails closed** — on any credential signal it returns null and the session keeps its -`session/` branch. Stripping the offending token was tried first and abandoned: a secret has -no reliable shape (`hunter2` is both a password and an ordinary word), so removing what looks -secret-shaped leaves everything that does not. The signals are the *syntax* that carries -secrets — a value assigned to a sensitive name, URL userinfo, known key prefixes, PEM blocks, -and 20+ alphanumeric runs mixing letters and digits. - -This is best-effort, not a guarantee: prose like "the password is hunter2" carries no syntax to -match. It leans wide on purpose, because a false positive costs one branch name while a false -negative publishes a secret. The `it.each` tables in `tests/branch-name-generator.test.ts` pin -both directions — nine credential syntaxes refused, and six prompts that merely mention `auth`, -`token`, `secret` or `credential` still named. +The daemon does not name branches. A worktree session stays on the `session/` branch +`worktree-manager.ts` created for it, and `syncSessionBranchName` records whatever branch the +session is actually on after every turn, so an agent that renames the branch itself is picked +up. For GitHub projects the agent is asked to do exactly that — see +`GITHUB_WORKTREE_SYSTEM_COMMANDS` in `session/session-execution-helpers.ts`. + +This used to be an automatic prompt-to-branch rename, removed because it could not be made +safe. A branch name is a ref: it reaches the remote as soon as the session opens a PR, so +deriving one from prompt text publishes prompt text, and "rotate the password before Friday" +is an ordinary request. Two filters were tried and both failed for the same reason — a secret +has no reliable shape, since `hunter2` is a password and an ordinary word. Stripping +credential-shaped tokens left everything that did not look like one; failing closed on +credential *syntax* still let plain prose through, so it fails open on every miss and cannot +be a security boundary. Naming refs after user text needs a source provably isolated from the +prompt, and no such source exists at session-ready: the ACP title has not arrived yet, and the +isolated generator's own fallback is the raw prompt. ### Local project identity diff --git a/apps/cli/src/agent/branch-name-generator.ts b/apps/cli/src/agent/branch-name-generator.ts deleted file mode 100644 index 81de66c83..000000000 --- a/apps/cli/src/agent/branch-name-generator.ts +++ /dev/null @@ -1,183 +0,0 @@ -/** - * Branch name generator - converts session titles or task descriptions to valid git branch names. - */ - -/** - * Convert a title or task description to a valid git branch name. - * - * Rules: - * - Converts to lowercase kebab-case - * - Removes special characters - * - Limits length to 50 characters (git best practice) - * - Adds appropriate prefix (fix/, feat/, chore/, etc.) - */ -export const titleToBranchName = (title: string): string => { - if (!title || typeof title !== 'string') { - return ''; - } - - const normalized = title.trim().toLowerCase(); - - // Detect prefix based on common patterns - const prefix = detectBranchPrefix(normalized); - - // Remove the detected prefix pattern from the title for cleaner branch name - const withoutPrefixPattern = removeKnownPrefixPatterns(normalized); - - // Convert to kebab-case - const kebab = withoutPrefixPattern - // Replace spaces and underscores with hyphens - .replace(/[\s_]+/g, '-') - // Remove all characters that are not alphanumeric or hyphens - .replace(/[^a-z0-9-]/g, '') - // Replace multiple consecutive hyphens with single hyphen - .replace(/-+/g, '-') - // Remove leading/trailing hyphens - .replace(/^-+|-+$/g, ''); - - if (!kebab) { - return ''; - } - - // Limit length (50 chars for branch name is a good practice) - // Account for prefix length - const maxKebabLength = 50 - prefix.length; - const truncated = kebab.slice(0, maxKebabLength).replace(/-+$/, ''); - - return `${prefix}${truncated}`; -}; - -/** - * Detect the appropriate branch prefix based on the task/title content. - */ -const detectBranchPrefix = (text: string): string => { - const lowerText = text.toLowerCase(); - - // Fix-related patterns - if (/\b(fix|bug|issue|error|crash|broken|repair|resolve)\b/.test(lowerText)) { - return 'fix/'; - } - - // Feature-related patterns - if (/\b(add|implement|create|new|feature|introduce|support)\b/.test(lowerText)) { - return 'feat/'; - } - - // Refactor-related patterns - if (/\b(refactor|restructure|reorganize|improve|optimize|clean)\b/.test(lowerText)) { - return 'refactor/'; - } - - // Documentation-related patterns - if (/\b(doc|document|readme|comment)\b/.test(lowerText)) { - return 'docs/'; - } - - // Test-related patterns - if (/\b(test|spec|coverage)\b/.test(lowerText)) { - return 'test/'; - } - - // Chore-related patterns - if (/\b(chore|update|upgrade|bump|dependency|deps)\b/.test(lowerText)) { - return 'chore/'; - } - - // Default to feat/ for general tasks - return 'feat/'; -}; - -/** - * Remove known prefix patterns that would be redundant with the branch prefix. - */ -const removeKnownPrefixPatterns = (text: string): string => { - return text - .replace( - /^(fix|bug|feature|feat|add|implement|create|refactor|docs?|test|chore|update)[:\s-]+/i, - '' - ) - .trim(); -}; - -/** - * Validate if a string is a valid git branch name. - */ -export const isValidGitBranchName = (name: string): boolean => { - if (!name || typeof name !== 'string') { - return false; - } - - // Git branch name rules: - // - Cannot start with a dot - // - Cannot contain consecutive dots - // - Cannot end with .lock - // - Cannot contain control characters, space, ~, ^, :, ?, *, [, \ - // - Cannot contain @{ - - if (name.startsWith('.') || name.startsWith('-')) { - return false; - } - - if (name.endsWith('.lock') || name.endsWith('.') || name.endsWith('/')) { - return false; - } - - if (/\.\./.test(name)) { - return false; - } - - if (/@\{/.test(name)) { - return false; - } - - // eslint-disable-next-line no-control-regex - if (/[\x00-\x1f\x7f ~^:?*[\]\\]/.test(name)) { - return false; - } - - return true; -}; - -/** - * Signals that a prompt is carrying a credential. - * - * A branch name is a ref: it is written to `.git`, shown in the UI, and pushed to - * the remote as soon as the session opens a PR. Naming a branch after a prompt - * therefore publishes whatever the prompt held, and "rotate the key before Friday" - * is an ordinary thing to ask an agent. - * - * This list cannot be complete, and is not meant to be: a secret has no reliable - * shape, since `hunter2` is both a password and an ordinary word. It recognizes - * the *syntax* that carries secrets rather than the secrets themselves, and the - * boundary fails closed on a hit — the session keeps its `session/` branch - * instead of getting a name derived from that prompt. A false positive costs one - * branch name, so the list leans deliberately wide. - */ -const CREDENTIAL_SIGNAL = new RegExp( - [ - // A value assigned to a sensitive name: DB_PASSWORD=…, "api key: …". - '(?:api[_-]?key|auth|bearer|credential|passwd|password|secret|token)\\w*\\s*[:=]\\s*\\S', - // URL userinfo: https://alice:hunter2@example.com - '[a-zA-Z][a-zA-Z0-9+.-]*://[^/\\s@]*:[^/\\s@]*@', - // Whole PEM block first, so a short body cannot escape between the markers. - '-----BEGIN[\\s\\S]*?-----END[\\s\\S]*?-----', - '-----BEGIN[\\s\\S]*?-----', - // Known credential prefixes. - '\\b(?:sk|pk|rk|ghp|gho|ghu|ghs|ghr|glpat|shpat|xox[abprs])[-_][A-Za-z0-9_-]{6,}', - '\\bgithub_pat_[A-Za-z0-9_]{10,}', - '\\b(?:AKIA|ASIA|AIza)[A-Za-z0-9]{6,}', - // Unprefixed high-entropy run: 20+ alphanumerics mixing letters and digits. - // English prose has no such runs; hex and base62 credentials do. - '\\b(?=[A-Za-z0-9]*[0-9])(?=[A-Za-z0-9]*[A-Za-z])[A-Za-z0-9]{20,}\\b', - ].join('|'), - // Case-insensitive: DB_PASSWORD and AWS_SECRET_ACCESS_KEY are how these appear. - 'i' -); - -export const tryBranchName = (base: string): string | null => { - if (CREDENTIAL_SIGNAL.test(base)) { - return null; - } - const candidate = titleToBranchName(base); - return candidate && isValidGitBranchName(candidate) ? candidate : null; -}; diff --git a/apps/cli/src/lib/message-handler.ts b/apps/cli/src/lib/message-handler.ts index 7c1c99cac..be1c6329e 100644 --- a/apps/cli/src/lib/message-handler.ts +++ b/apps/cli/src/lib/message-handler.ts @@ -254,7 +254,6 @@ import type { AcpAgentEditEvidence, AcpStandardDiffBlockEvidence } from '@/lib/a import { mergeAcpRuntimeConfigUpdates } from '@/lib/acp/runtime-config'; import { generateTitleIsolated, sanitizeTitle } from '@/agent/title-generator'; import type { AgentSessionWarning } from '@/agent/agent-client'; -import { tryBranchName } from '@/agent/branch-name-generator'; import { SessionActivePresenceController, type SessionActivePresencePhase, @@ -265,7 +264,6 @@ import { } from './session-activity-status'; import { markAssistantTurnFinished } from './assistant-turn-finalize'; import type { RepoWatchHandle } from 'loro-repo'; -import { resolveGitBranchName } from './git/resolve-git-branch-name'; import { AgentClient, type AcpWriteTextFileEvidence, @@ -274,10 +272,6 @@ import { } from 'src/agent/agent-client'; import type { RateLimit, SessionUsageUpdate } from 'acp-extension-core'; import { getWorktreeManager } from '@/session/worktree/worktree-manager'; -import { - isManagedWorktreeBranchName, - renameBranchWithAvailableSuffix, -} from '@/session/worktree/branch-name-allocation'; import { createWorktreeScriptHistoryRecorder } from '@/session/worktree/worktree-script-history'; import { runWorktreeCleanup } from '@/session/worktree/worktree-setup-runner'; import { @@ -3155,8 +3149,6 @@ export class MessageHandler { customAcp, runtimeOverrides ), - maybeRenameSessionBranchFromPrompt: async (sessionId, session, prompt) => - await this.maybeRenameSessionBranchFromPrompt(sessionId, session, prompt), processMessageQueue: async (sessionId) => await this.processMessageQueue(sessionId), syncLiveActivitySummary: async (userId) => { await this.syncLiveActivitySummary(userId); @@ -9676,113 +9668,6 @@ export class MessageHandler { await this.sessionManager.cleanUp(); } - private async maybeRenameSessionBranchFromPrompt( - sessionId: SessionId, - session: ISession, - taskPrompt: string - ): Promise { - const trimmedPrompt = taskPrompt.trim(); - if (!trimmedPrompt) { - return; - } - - let metaBranchName: string | null = null; - let reusableTitlePromise: Promise | undefined; - try { - const sessionDoc = await this.workspaceDocument.getOrCreateSessionDoc(sessionId); - const meta = await sessionDoc.getMetaState(); - metaBranchName = meta?.branchName?.trim() || null; - const generatedMetaTitle = meta?.titleSource === 'generated' ? meta.title?.trim() : ''; - reusableTitlePromise = generatedMetaTitle - ? Promise.resolve(generatedMetaTitle) - : this.titleGenerationInFlight.get(sessionId); - if (metaBranchName && !isManagedWorktreeBranchName(metaBranchName)) { - return; - } - } catch (error) { - this.logger.debug( - `[${sessionId}] Failed to read session meta before branch rename: ${formatErrorMessage(error)}` - ); - } - - const branchName = await this.deriveWorktreeBranchName( - trimmedPrompt, - 20_000, - reusableTitlePromise - ); - if (!branchName) { - this.logger.debug( - `[${sessionId}] Skipping branch rename: the prompt yields no usable branch name` - ); - return; - } - - const workdir = session.getWorkdir(); - const currentBranch = await resolveGitBranchName(session.exec.bind(session), workdir); - if (!currentBranch || currentBranch === branchName) { - return; - } - if (!isManagedWorktreeBranchName(currentBranch)) { - this.logger.debug( - `[${sessionId}] Skipping branch rename: not on a managed worktree branch (currentBranch=${currentBranch})` - ); - return; - } - if (metaBranchName && metaBranchName !== currentBranch) { - this.logger.debug( - `[${sessionId}] Skipping branch rename: branch changed before rename (metaBranchName=${metaBranchName} currentBranch=${currentBranch})` - ); - return; - } - - try { - const renamedBranch = await renameBranchWithAvailableSuffix({ - exec: session.exec.bind(session), - workdir, - currentBranch, - desiredBranchName: branchName, - maxLength: 50, - }); - if (!renamedBranch) { - this.logger.debug( - `[${sessionId}] Skipping branch rename: branch changed or git rejected the rename` - ); - return; - } - await this.turnPostProcessingService.syncSessionBranchName(sessionId, session); - } catch (error) { - this.logger.debug(`[${sessionId}] Failed to rename branch: ${formatErrorMessage(error)}`); - } - } - - /** - * Derives a worktree branch name without ever starting an ACP agent. - * - * `titleToBranchName` is a pure transform, so the only thing an isolated agent - * ever added here was compressing the prompt into a shorter title first. A title - * is still preferred when one is already stored or in flight for this session - * (agents that keep the local generator produce one anyway); otherwise the prompt - * names the branch directly. A slow or failed title never blocks or cancels the - * rename, because the prompt is always an acceptable naming input. - */ - private async deriveWorktreeBranchName( - taskPrompt: string, - timeoutMs: number, - reusableTitlePromise?: Promise - ): Promise { - let title: string | null | undefined; - if (reusableTitlePromise) { - try { - title = await withTimeoutOrUndefined(reusableTitlePromise, timeoutMs); - } catch (error) { - this.logger.debug( - `[branch-name] Falling back to the prompt after title generation failed: ${formatErrorMessage(error)}` - ); - } - } - return tryBranchName(title?.trim() || taskPrompt); - } - private async notifySessionCompleted( sessionId: SessionId, userId: string, diff --git a/apps/cli/src/session/session-execution-service.ts b/apps/cli/src/session/session-execution-service.ts index ed99fd6fc..d4769682e 100644 --- a/apps/cli/src/session/session-execution-service.ts +++ b/apps/cli/src/session/session-execution-service.ts @@ -545,11 +545,6 @@ export type SessionExecutionServiceDeps = { customAcp?: CustomAcpLaunchSpec, runtimeOverrides?: BuiltinRuntimeOverrides ) => Promise; - maybeRenameSessionBranchFromPrompt: ( - sessionId: SessionId, - session: ISession, - prompt: string - ) => Promise; processMessageQueue: (sessionId: SessionId) => Promise; syncLiveActivitySummary?: (userId: string) => Promise; collectMachineResources: () => Promise; @@ -4875,14 +4870,6 @@ export class SessionExecutionService { self.deps.logger.debug( `[${sessionId}] session ready (workdir=${session.getWorkdir()} acpSessionId=${session.acpSessionId ?? 'null'})` ); - if (shouldPrepareWorktree) { - void self.deps.maybeRenameSessionBranchFromPrompt( - sessionId, - session, - agentConfig.prompt ?? '' - ); - } - yield* self.tryPromise(() => traceAsync( self.deps.logger, diff --git a/apps/cli/src/session/worktree/branch-name-allocation.test.ts b/apps/cli/src/session/worktree/branch-name-allocation.test.ts index fbf5e06d0..e4fa3bf57 100644 --- a/apps/cli/src/session/worktree/branch-name-allocation.test.ts +++ b/apps/cli/src/session/worktree/branch-name-allocation.test.ts @@ -1,20 +1,9 @@ import { describe, expect, it } from 'vitest'; -import type { SessionExec } from '@/lib/git/resolve-git-branch-name'; import { hasLocalBranchNameConflict, - isManagedWorktreeBranchName, - renameBranchWithAvailableSuffix, resolveAvailableBranchName, } from './branch-name-allocation'; -describe('isManagedWorktreeBranchName', () => { - it('recognizes GitHub and shared-local placeholder branches', () => { - expect(isManagedWorktreeBranchName('session/12345678')).toBe(true); - expect(isManagedWorktreeBranchName('lody/123456789abc')).toBe(true); - expect(isManagedWorktreeBranchName('feat/user-owned')).toBe(false); - }); -}); - describe('resolveAvailableBranchName', () => { it('adds increasing suffixes without reusing an existing branch', () => { expect(resolveAvailableBranchName('fix/branch-collision', ['fix/branch-collision'])).toBe( @@ -43,122 +32,3 @@ describe('resolveAvailableBranchName', () => { expect(result).toMatch(/-2$/); }); }); - -const createFakeGit = (options: { - currentBranch: string; - branches: string[]; - ambiguousBranchNames?: string[]; - raceOnFirstRenameTo?: string; -}) => { - let currentBranch = options.currentBranch; - const branches = new Set(options.branches); - let renameAttempts = 0; - - const exec: SessionExec = async (_command, args) => { - if (args[0] === 'for-each-ref') { - const usesCanonicalBranchNames = args.includes('--format=%(refname:lstrip=2)'); - return Array.from(branches) - .sort() - .map((branchName) => - !usesCanonicalBranchNames && options.ambiguousBranchNames?.includes(branchName) - ? `heads/${branchName}` - : branchName - ) - .join('\n'); - } - if (args[0] === 'branch' && args[1] === '--show-current') { - return currentBranch; - } - if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') { - return currentBranch; - } - if (args[0] === 'branch' && args[1] === '-m') { - const oldBranch = args[2] ?? ''; - const newBranch = args[3] ?? ''; - renameAttempts += 1; - if (renameAttempts === 1 && options.raceOnFirstRenameTo === newBranch) { - branches.add(newBranch); - } - if ( - oldBranch !== currentBranch || - !branches.has(oldBranch) || - hasLocalBranchNameConflict(newBranch, branches) - ) { - return ''; - } - branches.delete(oldBranch); - branches.add(newBranch); - currentBranch = newBranch; - return ''; - } - throw new Error(`Unexpected git args: ${args.join(' ')}`); - }; - - return { - exec, - getCurrentBranch: () => currentBranch, - getBranches: () => branches, - getRenameAttempts: () => renameAttempts, - }; -}; - -describe('renameBranchWithAvailableSuffix', () => { - it('renames a managed branch to a fresh suffixed name', async () => { - const git = createFakeGit({ - currentBranch: 'session/12345678', - branches: ['session/12345678', 'fix/branch-collision'], - }); - - await expect( - renameBranchWithAvailableSuffix({ - exec: git.exec, - workdir: '/repo', - currentBranch: 'session/12345678', - desiredBranchName: 'fix/branch-collision', - maxLength: 50, - }) - ).resolves.toBe('fix/branch-collision-2'); - expect(git.getCurrentBranch()).toBe('fix/branch-collision-2'); - expect(git.getBranches()).toContain('fix/branch-collision'); - }); - - it('uses the canonical branch name when a tag has the same short name', async () => { - const git = createFakeGit({ - currentBranch: 'session/12345678', - branches: ['session/12345678', 'fix/branch-collision'], - ambiguousBranchNames: ['fix/branch-collision'], - }); - - await expect( - renameBranchWithAvailableSuffix({ - exec: git.exec, - workdir: '/repo', - currentBranch: 'session/12345678', - desiredBranchName: 'fix/branch-collision', - maxLength: 50, - }) - ).resolves.toBe('fix/branch-collision-2'); - expect(git.getCurrentBranch()).toBe('fix/branch-collision-2'); - expect(git.getBranches()).toContain('fix/branch-collision'); - }); - - it('retries with a suffix when another creator wins the first candidate', async () => { - const git = createFakeGit({ - currentBranch: 'lody/123456789abc', - branches: ['lody/123456789abc'], - raceOnFirstRenameTo: 'feat/new-task', - }); - - await expect( - renameBranchWithAvailableSuffix({ - exec: git.exec, - workdir: '/repo', - currentBranch: 'lody/123456789abc', - desiredBranchName: 'feat/new-task', - maxLength: 50, - }) - ).resolves.toBe('feat/new-task-2'); - expect(git.getRenameAttempts()).toBe(2); - expect(git.getCurrentBranch()).toBe('feat/new-task-2'); - }); -}); diff --git a/apps/cli/src/session/worktree/branch-name-allocation.ts b/apps/cli/src/session/worktree/branch-name-allocation.ts index 84f78b2dc..8e1505a52 100644 --- a/apps/cli/src/session/worktree/branch-name-allocation.ts +++ b/apps/cli/src/session/worktree/branch-name-allocation.ts @@ -1,5 +1,3 @@ -import { resolveGitBranchName, type SessionExec } from '@/lib/git/resolve-git-branch-name'; - const DEFAULT_MAX_CANDIDATES = 1_000; export type AvailableBranchNameOptions = { @@ -7,9 +5,6 @@ export type AvailableBranchNameOptions = { maxCandidates?: number; }; -export const isManagedWorktreeBranchName = (branchName: string): boolean => - branchName.startsWith('session/') || branchName.startsWith('lody/'); - const normalizeBranchNameForLength = (branchName: string, maxLength?: number): string => { const trimmed = branchName.trim(); if (!trimmed) { @@ -90,69 +85,3 @@ export const resolveAvailableBranchName = ( } throw new Error(`Unable to find an available branch name for ${desiredBranchName}`); }; - -const listLocalBranchNames = async (exec: SessionExec, workdir: string): Promise> => { - const output = await exec( - 'git', - ['for-each-ref', '--format=%(refname:lstrip=2)', 'refs/heads'], - workdir, - false - ); - return new Set( - output - .split('\n') - .map((line) => line.trim()) - .filter(Boolean) - ); -}; - -/** - * Rename a managed placeholder branch without ever attaching to an existing ref. - * - * Session.exec intentionally does not reject non-zero command exits, so success is - * verified by reading HEAD. If another creator wins the candidate between the ref - * scan and rename, refresh the refs and allocate the next suffix. - */ -export const renameBranchWithAvailableSuffix = async (options: { - exec: SessionExec; - workdir: string; - currentBranch: string; - desiredBranchName: string; - maxLength?: number; - maxCandidates?: number; -}): Promise => { - const unavailable = await listLocalBranchNames(options.exec, options.workdir); - const maxCandidates = options.maxCandidates ?? DEFAULT_MAX_CANDIDATES; - - for (let attempt = 0; attempt < maxCandidates; attempt += 1) { - const candidate = resolveAvailableBranchName(options.desiredBranchName, unavailable, { - maxLength: options.maxLength, - maxCandidates, - }); - await options.exec( - 'git', - ['branch', '-m', options.currentBranch, candidate], - options.workdir, - false - ); - - const actualBranch = await resolveGitBranchName(options.exec, options.workdir); - if (actualBranch === candidate) { - return candidate; - } - if (actualBranch !== options.currentBranch) { - return null; - } - - const refreshed = await listLocalBranchNames(options.exec, options.workdir); - if (!hasLocalBranchNameConflict(candidate, refreshed)) { - return null; - } - for (const branchName of refreshed) { - unavailable.add(branchName); - } - unavailable.add(candidate); - } - - throw new Error(`Unable to rename ${options.currentBranch} to an available branch name`); -}; diff --git a/apps/cli/tests/branch-name-generator.test.ts b/apps/cli/tests/branch-name-generator.test.ts deleted file mode 100644 index 49ef6dc6f..000000000 --- a/apps/cli/tests/branch-name-generator.test.ts +++ /dev/null @@ -1,148 +0,0 @@ -import { describe, expect, it } from 'vitest'; - -import { - titleToBranchName, - isValidGitBranchName, - tryBranchName, -} from '../src/agent/branch-name-generator'; - -describe('branch-name-generator', () => { - describe('titleToBranchName', () => { - it('converts simple titles to kebab-case with prefix', () => { - // "Add" is detected as a feature keyword, so prefix is stripped - expect(titleToBranchName('Add dark mode')).toBe('feat/dark-mode'); - expect(titleToBranchName('Fix login bug')).toBe('fix/login-bug'); - expect(titleToBranchName('Update dependencies')).toBe('chore/dependencies'); - }); - - it('detects fix-related patterns', () => { - expect(titleToBranchName('Fix authentication error')).toBe('fix/authentication-error'); - // "Resolve" triggers fix prefix but isn't stripped - expect(titleToBranchName('Resolve crash on startup')).toBe('fix/resolve-crash-on-startup'); - expect(titleToBranchName('Bug in user registration')).toBe('fix/in-user-registration'); - }); - - it('detects feature-related patterns', () => { - expect(titleToBranchName('Add user profile page')).toBe('feat/user-profile-page'); - expect(titleToBranchName('Implement OAuth2')).toBe('feat/oauth2'); - expect(titleToBranchName('Create new dashboard')).toBe('feat/new-dashboard'); - }); - - it('detects refactor-related patterns', () => { - expect(titleToBranchName('Refactor database layer')).toBe('refactor/database-layer'); - // "Improve" triggers refactor but isn't stripped - expect(titleToBranchName('Improve performance')).toBe('refactor/improve-performance'); - // "Clean" triggers refactor but isn't stripped - expect(titleToBranchName('Clean up legacy code')).toBe('refactor/clean-up-legacy-code'); - }); - - it('detects docs-related patterns', () => { - // "Document" triggers docs but isn't stripped - expect(titleToBranchName('Document API endpoints')).toBe('docs/document-api-endpoints'); - expect(titleToBranchName('Update README')).toBe('docs/readme'); - }); - - it('detects test-related patterns', () => { - // "Add" has higher priority than "test" in detection - expect(titleToBranchName('Add test coverage')).toBe('feat/test-coverage'); - // "Write" doesn't match test pattern, but "tests" does - expect(titleToBranchName('Write unit tests for utils')).toBe('feat/write-unit-tests-for-utils'); - }); - - it('detects chore-related patterns', () => { - expect(titleToBranchName('Update npm packages')).toBe('chore/npm-packages'); - // "Bump" triggers chore but isn't stripped - expect(titleToBranchName('Bump version to 2.0')).toBe('chore/bump-version-to-20'); - // "Upgrade" triggers chore but isn't stripped - expect(titleToBranchName('Upgrade TypeScript')).toBe('chore/upgrade-typescript'); - }); - - it('removes special characters', () => { - expect(titleToBranchName("Fix user's profile (issue #123)")).toBe('fix/users-profile-issue-123'); - // "feature:" pattern gets stripped - expect(titleToBranchName('Add feature: dark mode')).toBe('feat/feature-dark-mode'); - }); - - it('limits length to 50 characters', () => { - const longTitle = 'Add a very long feature that spans multiple words and should be truncated'; - const result = titleToBranchName(longTitle); - expect(result.length).toBeLessThanOrEqual(50); - }); - - it('handles empty or invalid input', () => { - expect(titleToBranchName('')).toBe(''); - expect(titleToBranchName(null as any)).toBe(''); - expect(titleToBranchName(undefined as any)).toBe(''); - }); - }); - - describe('isValidGitBranchName', () => { - it('accepts valid branch names', () => { - expect(isValidGitBranchName('feat/add-dark-mode')).toBe(true); - expect(isValidGitBranchName('fix/issue-123')).toBe(true); - expect(isValidGitBranchName('main')).toBe(true); - expect(isValidGitBranchName('release/v1.0.0')).toBe(true); - }); - - it('rejects invalid branch names', () => { - expect(isValidGitBranchName('.hidden')).toBe(false); - expect(isValidGitBranchName('-dash-start')).toBe(false); - expect(isValidGitBranchName('branch.lock')).toBe(false); - expect(isValidGitBranchName('with..dots')).toBe(false); - expect(isValidGitBranchName('with spaces')).toBe(false); - expect(isValidGitBranchName('with@{at}')).toBe(false); - expect(isValidGitBranchName('')).toBe(false); - expect(isValidGitBranchName(null as any)).toBe(false); - }); - }); - - describe('tryBranchName', () => { - it('returns the generated branch name when valid', () => { - expect(tryBranchName('Fix login bug')).toBe('fix/login-bug'); - }); - - it('returns null for empty input', () => { - expect(tryBranchName('')).toBeNull(); - }); - - // Kebab conversion strips every non-ASCII character, leaving nothing to name - // the branch after; the caller keeps the branch it already has. - it('returns null when the input has no ASCII words', () => { - expect(tryBranchName('把标题生成迁移到会话协议')).toBeNull(); - }); - - // A branch name is a ref: it reaches the remote when the session opens a PR, - // so a prompt carrying a credential must not name the branch. Secrets have no - // reliable shape, so the boundary fails closed on the syntax that carries them - // and the session keeps its `session/` branch. - it.each([ - ['an assignment to a sensitive name', 'Fix DB_PASSWORD=hunter2'], - ['a lowercase assignment', 'debug with password: correcthorse'], - ['an AWS-style assignment', 'set AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI'], - ['URL userinfo', 'Fix https://alice:hunter2@example.com'], - ['an Authorization header', 'Authorization: Bearer eyJhbGciOiJIUzI1'], - ['a known key prefix', 'Fix API key sk_live_ABC123def456'], - ['a GitHub token', 'update ghp_16C7e42F292c6912E7710c838347Ae178B4a'], - ['an unprefixed hex secret', 'token is 0123456789abcdef0123456789abcdef'], - [ - 'a PEM block', - 'paste -----BEGIN RSA PRIVATE KEY----- MIIEow -----END RSA PRIVATE KEY-----', - ], - ])('refuses to name a branch after %s', (_label, prompt) => { - expect(tryBranchName(prompt)).toBeNull(); - }); - - // Failing closed is only affordable because it does not fire on ordinary work. - // These all mention a sensitive word without carrying a value. - it.each([ - ['Fix crash when opening FooBar with empty input', 'fix/crash-when-opening-foobar-with-empty-input'], - ['Bump codex to 1.10.1 and grok to 0.1.3', 'chore/bump-codex-to-1101-and-grok-to-013'], - ['Fix auth redirect loop after logout', 'fix/auth-redirect-loop-after-logout'], - ['Add a token bucket rate limiter', 'feat/a-token-bucket-rate-limiter'], - ['Support GITHUB_TOKEN in CI', 'feat/support-github-token-in-ci'], - ['Write tests for the credential broker', 'feat/write-tests-for-the-credential-broker'], - ])('still names a branch after %s', (prompt, expected) => { - expect(tryBranchName(prompt)).toBe(expected); - }); - }); -}); diff --git a/apps/cli/tests/message-handler-title.test.ts b/apps/cli/tests/message-handler-title.test.ts index 3ade5f866..75dc44cba 100644 --- a/apps/cli/tests/message-handler-title.test.ts +++ b/apps/cli/tests/message-handler-title.test.ts @@ -102,27 +102,6 @@ const createHandler = async ( return { handler, sessionDoc, workspaceDocument }; }; -// Drives the whole branch-rename path. `exec` reports no branch, so the rename -// itself is a no-op and no git command runs. -const renameBranch = async (handler: MessageHandler): Promise => { - const branchHost = handler as unknown as { - maybeRenameSessionBranchFromPrompt: ( - sessionId: SessionId, - session: unknown, - taskPrompt: string - ) => Promise; - }; - const session = { - getWorkdir: () => '/tmp/lody-branch-test', - exec: async () => ({ stdout: '', stderr: '', exitCode: 1 }), - }; - await branchHost.maybeRenameSessionBranchFromPrompt( - 's-branch' as SessionId, - session, - 'Fix the flaky login redirect' - ); -}; - describe('MessageHandler title generation', () => { beforeEach(() => { mockedGenerateTitleIsolated.mockClear(); @@ -210,74 +189,6 @@ describe('MessageHandler title generation', () => { expect(sessionDoc.setTitleIfSourceIn).toHaveBeenCalledTimes(1); }); - type BranchNameHost = { - deriveWorktreeBranchName: ( - taskPrompt: string, - timeoutMs: number, - reusableTitlePromise?: Promise - ) => Promise; - }; - - it('prefers an already-available session title over the prompt', async () => { - const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( - 'Fallback prompt', - 1_000, - Promise.resolve('Fix title races') - ); - - expect(branch).toBe('fix/title-races'); - }); - - it('names the branch from the prompt when no title is available', async () => { - const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( - 'Add a retry to the upload queue', - 1_000 - ); - - expect(branch).toBe('feat/a-retry-to-the-upload-queue'); - expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); - }); - - // Kebab conversion drops every non-ASCII character, so such a prompt yields no - // name at all. Leaving the managed `session/` branch alone beats renaming it - // to a meaningless timestamp. - it('returns no name when the prompt has no ASCII words', async () => { - const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( - '把标题生成迁移到会话协议', - 1_000 - ); - - expect(branch).toBeNull(); - }); - - it('falls back to the prompt when the title does not arrive in time', async () => { - const { handler } = await createHandler(undefined); - const branch = await (handler as unknown as BranchNameHost).deriveWorktreeBranchName( - 'Fix the flaky login redirect', - 10, - new Promise(() => {}) - ); - - expect(branch).toBe('fix/the-flaky-login-redirect'); - }); - - // Branch naming is a pure local transform now, so it starts no agent and never - // consults the agent config -- the provider no longer reaches this path at all. - it('never starts an isolated agent to name a branch', async () => { - const { handler, workspaceDocument } = await createHandler(undefined, undefined, undefined, { - agentConfigId: 'agent-config-1', - agentConfigMeta: { titleGeneration: { configOptionValues: { model: 'stale-model' } } }, - }); - - await renameBranch(handler); - - expect(mockedGenerateTitleIsolated).not.toHaveBeenCalled(); - expect(workspaceDocument.getAgentConfigById).not.toHaveBeenCalled(); - }); - it('keeps skipping isolated generation when an existing title has no draft source', async () => { const prompt = 'Do something cool'; const placeholder = prompt.slice(0, 50); diff --git a/apps/cli/tests/session-execution-service.test.ts b/apps/cli/tests/session-execution-service.test.ts index 1d11b4671..3da80c4ce 100644 --- a/apps/cli/tests/session-execution-service.test.ts +++ b/apps/cli/tests/session-execution-service.test.ts @@ -180,7 +180,6 @@ const createBaseDeps = ( }, recordChatFailure: vi.fn(async () => {}), maybeGenerateAndStoreSessionTitle: vi.fn(async () => {}), - maybeRenameSessionBranchFromPrompt: vi.fn(async () => {}), processMessageQueue: vi.fn(async () => {}), collectMachineResources: vi.fn(async () => ({ totalMemoryGB: 1,