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..f3239a158 --- /dev/null +++ b/.agents/notes/implemented/architecture/2026-09-08-acp-owned-session-titles.md @@ -0,0 +1,229 @@ +# Let Codex and Grok own their session titles, 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 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. 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 + +Each adapter was read at the commit this repository pins; the versions below were +re-confirmed after merging main, which moved the Codex, Grok and Harness pins +without changing any of these findings. + +| 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.1 (since 1.8.0) | yes | yes, generated titles are `explicit` | yes — cheap-model turn on an ephemeral thread | +| `acp-extension-grok` | 0.1.3 (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.2 | 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 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`. + +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 + +`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, 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()`. + +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 +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 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, Codex and Grok runs after their config disappeared from the UI. That +lookup is gone with the branch-naming path itself (below). + +## 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. + +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. + +Branch naming had to change too, or the isolated session would simply have moved +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 +`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 +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 +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 6cc6c8806..8a1f445d2 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, 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 522f47b15..e961d5c5b 100644 --- a/apps/cli/src/agent/README.md +++ b/apps/cli/src/agent/README.md @@ -186,11 +186,42 @@ override entries still apply only when their source-version suffix matches the s ### 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, Codex and Grok own session title generation through ACP +`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. + +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. + +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/agent-client.ts b/apps/cli/src/agent/agent-client.ts index 033cd7058..d0934da7e 100644 --- a/apps/cli/src/agent/agent-client.ts +++ b/apps/cli/src/agent/agent-client.ts @@ -27,7 +27,7 @@ import { type SessionGoalContent, type SessionTurnInputConfig, sanitizeGoalObjective, - usesAcpProvidedSessionTitle, + trustsUntaggedAcpSessionTitle, parseSessionNotification, SessionContextWindowUsage, SessionId, @@ -1547,7 +1547,7 @@ export class AgentClient implements acp.Client { return; } - const ownsTitleGeneration = usesAcpProvidedSessionTitle( + const trustsUntaggedTitle = trustsUntaggedAcpSessionTitle( this.options.agentConfig?.cliType, this.options.agentConfig?.agentType ); @@ -1561,7 +1561,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/agent/branch-name-generator.ts b/apps/cli/src/agent/branch-name-generator.ts deleted file mode 100644 index 0e236688a..000000000 --- a/apps/cli/src/agent/branch-name-generator.ts +++ /dev/null @@ -1,154 +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; -}; - -/** - * Ensure a branch name is valid, falling back to a safe default if not. - */ -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}`; -}; diff --git a/apps/cli/src/lib/message-handler.ts b/apps/cli/src/lib/message-handler.ts index cec5d54d3..be1c6329e 100644 --- a/apps/cli/src/lib/message-handler.ts +++ b/apps/cli/src/lib/message-handler.ts @@ -40,7 +40,7 @@ import { type TitleGenerationConfig, isManagedBuiltinAgentType, sanitizeLodyInternalInstructions, - usesAcpProvidedSessionTitle, + acpOwnsSessionTitleGeneration, SessionCreateResponse, SessionChatResponse, SessionStatusFactory, @@ -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 { ensureValidBranchName } 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 { @@ -387,7 +381,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]; @@ -3156,22 +3149,6 @@ export class MessageHandler { customAcp, runtimeOverrides ), - maybeRenameSessionBranchFromPrompt: async ( - sessionId, - session, - cliType, - agentType, - prompt, - env - ) => - await this.maybeRenameSessionBranchFromPrompt( - sessionId, - session, - cliType, - agentType, - prompt, - env - ), processMessageQueue: async (sessionId) => await this.processMessageQueue(sessionId), syncLiveActivitySummary: async (userId) => { await this.syncLiveActivitySummary(userId); @@ -9005,8 +8982,9 @@ export class MessageHandler { runtimeOverrides?: BuiltinRuntimeOverrides, titleConfig?: TitleGenerationConfig ): Promise { - // Builtin Claude publishes a generated session_info_update title. - if (usesAcpProvidedSessionTitle(cliType, agentType)) { + // 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, runtimeOverrides)) { return; } const existingGeneration = this.titleGenerationInFlight.get(sessionId); @@ -9690,160 +9668,6 @@ export class MessageHandler { await this.sessionManager.cleanUp(); } - private async maybeRenameSessionBranchFromPrompt( - sessionId: SessionId, - session: ISession, - cliType: AgentConfigCliType, - agentType: string, - taskPrompt: string, - env?: Record, - titleConfig?: TitleGenerationConfig - ): Promise { - const trimmedPrompt = taskPrompt.trim(); - if (!trimmedPrompt) { - return; - } - - 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; - } - } catch (error) { - this.logger.debug( - `[${sessionId}] Failed to read session meta before branch rename: ${formatErrorMessage(error)}` - ); - } - - const resolvedTitleConfig = - 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`); - 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)}`); - } - } - - private async generateBranchNameWithTimeout( - cliType: AgentConfigCliType, - agentType: string, - taskPrompt: string, - env: Record | undefined, - timeoutMs: number, - titleConfig?: TitleGenerationConfig, - customAcp?: CustomAcpLaunchSpec, - runtimeOverrides?: BuiltinRuntimeOverrides, - reusableTitlePromise?: Promise - ): Promise { - 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; - } catch (error) { - this.logger.debug( - `[branch-name] Failed to generate branch name: ${formatErrorMessage(error)}` - ); - return null; - } finally { - if (timeoutHandle) { - clearTimeout(timeoutHandle); - } - } - } - 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 355f26bdf..d4769682e 100644 --- a/apps/cli/src/session/session-execution-service.ts +++ b/apps/cli/src/session/session-execution-service.ts @@ -545,14 +545,6 @@ export type SessionExecutionServiceDeps = { customAcp?: CustomAcpLaunchSpec, runtimeOverrides?: BuiltinRuntimeOverrides ) => Promise; - maybeRenameSessionBranchFromPrompt: ( - sessionId: SessionId, - session: ISession, - cliType: AgentConfigCliType, - agentType: string, - prompt: string, - env?: Record - ) => Promise; processMessageQueue: (sessionId: SessionId) => Promise; syncLiveActivitySummary?: (userId: string) => Promise; collectMachineResources: () => Promise; @@ -4878,17 +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, - sessionConfig.agentCliType, - sessionConfig.agentType, - agentConfig.prompt ?? '', - env - ); - } - 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 1a526bf7d..000000000 --- a/apps/cli/tests/branch-name-generator.test.ts +++ /dev/null @@ -1,114 +0,0 @@ -import { describe, expect, it } from 'vitest'; - -import { - titleToBranchName, - isValidGitBranchName, - ensureValidBranchName, -} 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('ensureValidBranchName', () => { - it('returns generated branch name when valid', () => { - expect(ensureValidBranchName('Fix login bug')).toBe('fix/login-bug'); - }); - - it('returns fallback for invalid input', () => { - const result = ensureValidBranchName(''); - expect(result).toMatch(/^task\/[a-z0-9]+$/); - }); - - it('uses custom fallback prefix', () => { - const result = ensureValidBranchName('', 'session'); - expect(result).toMatch(/^session\/[a-z0-9]+$/); - }); - }); -}); diff --git a/apps/cli/tests/message-handler-title.test.ts b/apps/cli/tests/message-handler-title.test.ts index 03106773f..75dc44cba 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)); @@ -189,38 +189,6 @@ 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; - }; - - const branch = await titleHost.generateBranchNameWithTimeout( - 'builtin', - 'codex', - 'Fallback prompt', - undefined, - 1_000, - undefined, - undefined, - undefined, - Promise.resolve('Fix title races') - ); - - expect(branch).toBe('fix/title-races'); - expect(mockedGenerateTitleIsolated).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); @@ -296,9 +264,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 +284,7 @@ describe('MessageHandler title generation', () => { await titleHost.maybeGenerateAndStoreSessionTitle( 's-6' as SessionId, 'builtin', - 'codex', + 'kimi', 'Do something cool' ); @@ -327,29 +295,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(); - }); + // 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/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, diff --git a/packages/components/src/components/settings/agent-config-dialog.tsx b/packages/components/src/components/settings/agent-config-dialog.tsx index 44f95733f..fe7b8db48 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,11 @@ 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, + formData.runtimeOverrides + ); const activeCredentialMode = activePreset ? getPresetCredentialMode(activePreset, formData.presetCredentialModeId) : undefined; diff --git a/packages/components/tests/agent-config-dialog.test.tsx b/packages/components/tests/agent-config-dialog.test.tsx index bda72b81f..12b3f0ef2 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,18 @@ const createMachine = ( }, }); -const createCodexMachine = (): MachineViewMeta => ({ - ...createMachine('Codex workstation'), +/** A machine whose cached capabilities expose title-eligible config options. */ +const createTitleConfigMachine = (): 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 +73,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,12 +93,29 @@ 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(), }, }, }); +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"]')); @@ -1012,25 +1033,32 @@ 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) => { + await renderDialog( + { kind: 'edit', config: createBuiltinConfig({ name: 'ACP-owned', agentType }) }, + createTitleConfigMachine() + ); + + 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, - machineId, - name: 'Codex', - description: undefined, - cliType: 'builtin', - agentType: 'codex', - env: {}, + const config = createBuiltinConfig({ 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 }, createTitleConfigMachine(), onSubmit); expect(document.body.textContent).toContain('Title generation'); @@ -1046,7 +1074,7 @@ describe('AgentConfigDialog', () => { expect.objectContaining({ titleGeneration: { configOptionValues: { - model: 'gpt-5.6-other', + model: 'kimi-k2-turbo', reasoning_effort: 'medium', }, }, diff --git a/packages/shared/src/ai.ts b/packages/shared/src/ai.ts index 29f69904c..a1d229f91 100644 --- a/packages/shared/src/ai.ts +++ b/packages/shared/src/ai.ts @@ -41,11 +41,72 @@ 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 = ( +/** + * 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', +}; + +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. + * + * 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, + runtimeOverrides?: BuiltinRuntimeOverrides +): boolean => + !hasBuiltinRuntimeOverrideValues(runtimeOverrides) && + builtinAcpTitleOwnership(cliType, agentType) !== 'none'; + +/** + * Adapters whose pushed titles are authoritative without a `titleSource` tag. + * + * 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 === 'claude'; +): 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 582fed676..33b0de18c 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,74 @@ describe('getBuiltinTitleGenerationDefaults', () => { }); }); -describe('usesAcpProvidedSessionTitle', () => { - it('uses the builtin Claude ACP title', () => { - expect(usesAcpProvidedSessionTitle('builtin', 'claude')).toBe(true); +describe('acpOwnsSessionTitleGeneration', () => { + 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 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', '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); + expect(acpOwnsSessionTitleGeneration('builtin', 'not-an-agent')).toBe(false); + }); +}); + +describe('trustsUntaggedAcpSessionTitle', () => { + // 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` 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); + }); + + // 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); }); });