fix(cli): do not let Codex explicit titles replace generated names - #536
fix(cli): do not let Codex explicit titles replace generated names#536ladydd wants to merge 4 commits into
Conversation
github-actions
Bot
added
scope: cli
status:needs-pr-attention
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c24ff3cc7e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const applied = await sessionDoc.setTitleIfSourceIn(sanitized, 'generated', [ | ||
| 'draft', | ||
| const allowedSources: SessionTitleSource[] = ['draft']; | ||
| if (meta?.agentConfigId) { |
There was a problem hiding this comment.
When a builtin-Claude session has no agentConfigId—which SessionMeta explicitly permits for legacy sessions—or the catalog lookup transiently fails, this branch never adds generated to allowedSources, despite the durable session metadata still identifying the provider as builtin Claude. Consequently, an existing generated title, including one stored by an earlier Claude update, rejects every subsequent authoritative Claude title with no retry. Determine the policy from meta.cliType/meta.agentType rather than requiring the mutable agent-config row.
AGENTS.md reference: apps/cli/src/agent/AGENTS.md:L108-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. SessionMeta.cliType / agentType are required; agentConfigId is optional for legacy sessions. The catalog lookup fail-closed (never adding generated) would have blocked later Claude title refreshes on those sessions.
Pushed 11dad1f3 on this branch: usesAcpProvidedSessionTitle(meta.cliType, meta.agentType), and the getAgentConfigById try/catch is gone. Added a test that a builtin Claude session with no agentConfigId can still replace generated. apps/cli title tests: 17 passed.
The earlier Static checks failure was Prettier on message-handler.ts (line wrapping around the catalog lookup). That was 4ea2dada on this same branch; Static checks went green after that push. This commit removes that lookup entirely.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11dad1f354
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const allowedSources: SessionTitleSource[] = ['draft']; | ||
| // SessionMeta.cliType/agentType are required; agentConfigId is optional on | ||
| // legacy sessions. Policy must not depend on a catalog lookup. | ||
| if (usesAcpProvidedSessionTitle(meta?.cliType, meta?.agentType)) { |
There was a problem hiding this comment.
When a resumable Claude session has either supported legacy metadata shape—only agentType: 'claude', or cliType: 'claude' with no agentType—this predicate returns false and limits the write to draft, so an ACP title cannot replace an existing generated title even though Claude owns title generation. These persisted shapes are explicitly normalized elsewhere in packages/shared/src/node/local-session-control.ts:192-203, but getMetaState() returns the raw cast metadata. The fresh evidence beyond the earlier agentConfigId comment is these additional legacy encodings; normalize them here before calling usesAcpProvidedSessionTitle.
AGENTS.md reference: apps/cli/src/agent/AGENTS.md:L108-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in df77b17. getMetaState() is a raw cast, so I added resolveSessionAgentIdentity next to usesAcpProvidedSessionTitle with the same four branches as normalizeAcpTarget (current builtin/registry/custom, agentType-only claude|codex, cliType=claude|codex with no agentType, both-legacy). maybeStoreAgentSessionTitle now normalizes before the Claude title predicate and still does not look up the agent-config catalog.
Tests: agentType-only Claude and cliType=claude with no agentType can still replace a generated title; helper cases cover the four encodings plus unknown identities.
Builtin Claude owns session titles through ACP, so an agent-pushed title may replace an earlier generated one. Codex already has the isolated generator; accepting explicit thread names against allowedSources ['draft','generated'] retitled resumed conversations from the latest prompt. Keep Claude on ['draft','generated']. Everyone else, including Codex, may only replace draft.
SessionMeta requires cliType and agentType; agentConfigId is optional on legacy sessions. Looking up the catalog left builtin Claude without a config id unable to refresh a generated title.
getMetaState() is a raw cast; resume-time docs can still store agentType-only or cliType=claude|codex. Same cases as normalizeAcpTarget. Map those to builtin before usesAcpProvidedSessionTitle.
df77b17 to
8c9b65b
Compare
1 participant
Author type
An AI agent found the allowlist mismatch and implemented the guard. A human reviewed the live OSS runs (three overwrites, two negative controls), a Windows hand check that did not hit, and this patch before submit.
Related issue
Closes #535
Distinct from open #211 (title-generation model) and open #522 (skip the isolated generator for Codex/Grok entirely). This change is the current
mainpath where Lody still generates a title and Codex later publishesexplicit.Problem / pressure
A Codex conversation that already had a Lody-generated title was retitled from the latest prompt the first time Codex itself named the thread.
This is not every follow-up. A live session whose Codex thread already has a name keeps it. The rewrite happens when Codex's
thread.nameis still empty (typical of conversations Lody titled with the isolated generator) and Codex later publishes anexplicitsession_info_update. The host then overwritestitleSource: generated.maybeStoreAgentSessionTitleusedallowedSources: ['draft', 'generated']for every agent. That matches Builtin Claude (the pushed title is its only generated source) and is wrong for Codex, which already ran the isolated generator.e61234843c9c580dd009662f~/.codex/session_index.jsonlhad nothread_namefor those three until that rewrite. The first index row is the new name.Controls that did not retitle:
ebf8494b: Codex named it on the first turn (Single-word alpha7k reply). Same-process follow-up and quit+resume follow-up left the title alone.KeepPong7k(a73f239f): Lody kept the user title. Codex still wrotethread_nameinto the index; the host did not apply it.Summary
Codex (and other isolated-generator agents) may only replace a
drafttitle. Builtin Claude still uses['draft', 'generated']viausesAcpProvidedSessionTitle. User titles stay blocked. The production write ismaybeStoreAgentSessionTitle→setTitleIfSourceIn; this PR changes that allowlist.Visual explanation
No screenshot: the failure is a sidebar rename, recorded as the three-session table above. A Windows new-chat miss is the
draft --Codex explicit--> generatedpath, not this guard.Before / after
explicitmay replacetitleSource: generatedexplicitmay replacedraftonlygeneratedTest plan
On
fix/codex-title-allowlistafter cherry-pick ontoorigin/maine12cb225:(from
apps/cli)Result: 16 passed, including:
Live OSS 0.76.0, Linux, Codex
5.6-Sol: the three overwrite sessions and two negative controls above. Windows 11: two new chats did not hit (Codex named first). No Electron e2e for this allowlist.Context handoff
Context handoff is public. An invalid fork PR body receives seven days to be corrected before closure.
Instructions for reviewing agents
apps/cli/src/lib/message-handler.tsmaybeStoreAgentSessionTitleandapps/cli/tests/message-handler-title.test.ts;usesAcpProvidedSessionTitleis still Claude-only onmain.allowedSources: ['draft']rather than skipping the isolated generator (feat(cli): let builtin Codex and Grok own their ACP session titles #522). Keep Claude able to replacegenerated.agentConfigIddefault to draft-only, which is the safe Codex path. This does not fire on every new chat; Codex naming while stilldraftis a miss, not a disproof.Authoring context
thread.namefrom replacing a Lody-generated sidebar title. Human reviewed the live table, the Windows misses, and this patch.titleSource. Missing agent config fails closed (draft-only).