Handle session/setTitle and emit SessionInfoUpdate#286
Conversation
![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: df61470c54
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [dependencies] | ||
| agent-client-protocol = { version = "=0.11.1", features = ["unstable"] } | ||
| # Temporary local override until `session/setTitle` is released in the Rust SDK. | ||
| agent-client-protocol = { version = "=0.11.1", path = "../rust-sdk/src/agent-client-protocol", features = ["unstable"] } |
There was a problem hiding this comment.
Avoid committing agent-client-protocol as a local path dependency here, because path = "../rust-sdk/src/agent-client-protocol" requires a sibling checkout that is not part of this repository. In a normal clone (including CI and downstream builds), Cargo cannot resolve that path, so cargo build --locked fails before compilation. This effectively makes the commit non-buildable outside the author’s local workspace.
Useful? React with 👍 / 👎.
daniel-agentee
force-pushed
the
acp-set-title-handler
branch
from
df61470 to
24ce239
Compare
|
Thanks @chatgpt-codex-connector — addressed in 24ce239. Replaced the
|
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ce239c69
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> Result<SetSessionTitleResponse, Error> { | ||
| info!("Setting session title for session: {}", args.session_id); | ||
|
|
||
| let thread = self.get_thread(&args.session_id)?; |
There was a problem hiding this comment.
session/setTitle now fails fast on self.get_thread(&args.session_id), which only resolves sessions in the in-memory self.sessions map. After session/close (or before session/load), a session can still appear in session/list but is no longer in that map, so rename requests for listed historical sessions return resource_not_found instead of persisting the new title. This breaks the main rename flow for non-active sessions; the handler should allow a DB/index-only rename path when no live thread is attached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 269c0d3.
session/setTitle no longer fails on sessions missing from the live sessions map. The handler now parses the thread id and updates SQLite first, then either delegates to Thread::set_title for loaded sessions or writes the title index directly for historical/listed sessions and emits SessionInfoUpdate from the agent connection. Added set_session_title_persists_unloaded_session to cover that path.
Previously the title was persisted to session_index.jsonl but the connected client (Zed) only saw the change on the next session/list refresh — usually after restart. Send a SessionUpdate::SessionInfoUpdate carrying the new title so the client can update its session list in place, no restart needed. Covers both rename surfaces: the session/setTitle JSON-RPC and the /rename builtin slash command both flow through handle_set_title.
daniel-agentee
force-pushed
the
acp-set-title-handler
branch
from
24ce239 to
2901d1e
Compare
daniel-agentee
force-pushed
the
acp-set-title-handler
branch
from
2901d1e to
269c0d3
Compare
|
Sent from my iPhoneOn 15 May 2026, at 6:31 PM, chatgpt-codex-connector[bot] ***@***.***> wrote:
@chatgpt-codex-connector[bot] commented on this pull request.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ce239
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Open a pull request for review
Mark a draft as ready
Comment ***@***.*** review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting ***@***.*** address that feedback".
In src/codex_agent.rs:
@@ -792,6 +812,37 @@ impl CodexAgent {
Ok(SetSessionModelResponse::default())
}
+ async fn set_session_title(
+ &self,
+ args: SetSessionTitleRequest,
+ ) -> Result<SetSessionTitleResponse, Error> {
+ info!("Setting session title for session: {}", args.session_id);
+
+ let thread = self.get_thread(&args.session_id)?;
Support renaming sessions that are not currently loaded
session/setTitle now fails fast on self.get_thread(&args.session_id), which only resolves sessions in the in-memory self.sessions map. After session/close (or before session/load), a session can still appear in session/list but is no longer in that map, so rename requests for listed historical sessions return resource_not_found instead of persisting the new title. This breaks the main rename flow for non-active sessions; the handler should allow a DB/index-only rename path when no live thread is attached.
Useful? React with 👍 / 👎.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 269c0d3e5f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(false) => { | ||
| debug!( | ||
| "Thread metadata unavailable before title update for session: {}", | ||
| session_id | ||
| ); | ||
| } |
There was a problem hiding this comment.
When update_thread_title returns Ok(false) for an unloaded session, this only logs and then falls through to append_thread_name below, so a session/setTitle for a typo/stale sessionId is reported as successful and emits SessionInfoUpdate even though session/list cannot ever show that session. Since historical sessions are discovered through the state index, this case should return resource_not_found (or otherwise verify the rollout exists) instead of creating an orphan custom-title entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2ebbb14.
For unloaded sessions, session/setTitle now only proceeds if the state update succeeded or the session can be located via the rollout path lookup. Missing typo/stale IDs return resource_not_found before writing session_index.jsonl or emitting SessionInfoUpdate. Added set_session_title_rejects_missing_unloaded_session for the orphan case and adjusted the historical-session test to use a real rollout file.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ebbb140c2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| })?; | ||
| notify_client(SessionNotification::new( | ||
| session_id, | ||
| SessionUpdate::SessionInfoUpdate(SessionInfoUpdate::new().title(title)), |
There was a problem hiding this comment.
When the user clears the title by submitting an empty or whitespace-only string, list_sessions later treats that stored custom title as absent and falls back to the generated first-message title, but this immediate update sends the raw empty/whitespace title. In that case the connected client repaints to a blank title until it refreshes or re-lists sessions; emit the same normalized/fallback/cleared title here (and in the loaded-session notification path) so immediate UI state matches persisted list state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c03500e.
Blank or whitespace-only session/setTitle updates now notify clients with the same title session/list would show: the generated first-message fallback when available, or null when there is no fallback. The loaded-session path now receives that normalized notification title from the agent before emitting SessionInfoUpdate. Added coverage for both the unloaded fallback case and the loaded-session clear-to-null case.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c03500e79d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| let title = item | ||
| let title = match find_thread_name_by_id(&self.config.codex_home, &thread_id).await { |
There was a problem hiding this comment.
When session/list returns a page, this awaits find_thread_name_by_id once per item; that helper scans session_index.jsonl from the end for each id. For users with many renamed sessions or a large append-only index, opening the session list now performs up to 25 repeated full-file scans per page, which can make the UI noticeably slower. Collect the page’s thread IDs and use the existing batched lookup instead so the index is read once.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1db9f3d.
session/list now collects the page thread ids, calls find_thread_names_by_ids once, and then builds each SessionInfo from that batched map with the same first-message fallback when no custom title is present or the index lookup fails. Local validation remains green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1db9f3d62c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let title = thread_names | ||
| .get(&thread_id) | ||
| .cloned() | ||
| .or_else(|| first_user_message.as_deref().and_then(format_session_title)); |
There was a problem hiding this comment.
When a session already has a custom title and the client clears it with an empty/whitespace session/setTitle, this path still prefers the batch thread_names result. I checked codex_rollout::find_thread_names_by_ids in the pinned rust-v0.133.0 dependency: it scans the append-only index forward and skips blank names, so a newer blank entry from append_thread_name does not replace the older non-empty title. As a result, session/list continues returning the stale custom title instead of falling back to the first user message after a clear; this affects both loaded and unloaded sessions that had been renamed before being cleared.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 703dd29.
The session list no longer uses find_thread_names_by_ids, since that helper skips blank entries. It now does a single local pass over session_index.jsonl and records the latest entry per id, preserving blank/whitespace entries as None so a cleared custom title falls back to the generated first-message title instead of reviving an older custom title. Added batched_title_lookup_honors_latest_blank_clear to cover the regression.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Implements the agent side of the new ACP
session/setTitlemethod so ACP clients (e.g. Zed) can persist a session rename through codex-acp. After a successful rename, codex-acp emits aSessionInfoUpdatenotification so the client can repaint its session list inline — no restart, nosession/listround-trip required.Why
Zed's agent panel already has a title editor for ACP sessions (zed#54689), but it can't persist anything for ACP threads because there was no protocol method to push the new title down. With the protocol method now defined upstream, codex-acp needs a handler that:
session/listreturns the new title even after a fresh process)custom-titleentry to the JSONL rollout (sofind_thread_name_by_idreturns the new value)session/listalso needs to consult the rollout'scustom-titleentries when assembling each session's title, otherwise the rename is invisible until the next codex restart.What's in this PR
codex_agent.rs— registers aSetSessionTitleRequesthandler. For loaded sessions it dispatches toThread::set_title; for historical/listed sessions that are not currently loaded, it updates the SQLite/index state directly and emits the sameSessionInfoUpdatenotification.listSessionsnow consultsfind_thread_name_by_idfirst and falls back to the formatted first user message.thread.rs— addsThread::set_title+ThreadMessage::SetTitle+ThreadActor::handle_set_title, which callsappend_thread_name(codex_home, thread_id, title)to persist the rename. Then emitsSessionUpdate::SessionInfoUpdate { title }so the client knows.test_set_session_title_renames_underlying_threadcovers the loaded-thread actor path, JSONL persistence, and SessionInfoUpdate emit.set_session_title_persists_unloaded_sessioncovers renaming a listed/historical session that is not present in the livesessionsmap.Tests
23 lib tests pass on the rebased branch (
269c0d3).Upstream dependency chain
This PR sits on top of two upstream PRs that must merge first. To keep the build green here in the meantime,
Cargo.tomlcarries a temporary[patch.crates-io]block pointing at branch revs on my fork — see the inline comment inCargo.tomlfor removal steps once releases ship.SetSessionTitleRequest/SetSessionTitleResponseto the protocol schema. Patched in here viaagent-client-protocol-schema = { git = ..., rev = b9739d6 }.agent-client-protocol = { git = ..., rev = 34523b3 }.Once #1199 and rust-sdk#164 merge and an
agent-client-protocolrelease ships withsession/setTitle, drop the temporary patch entries and bumpagent-client-protocolto the released version.Companion PRs
/renameas a builtin slash command, on top of Expose Codex skills as ACP slash commands #279 + this PR — keeping rename infrastructure and the inline slash command as separate reviewable concerns.Draft
Drafted while waiting on the two upstream PRs above. Ready for design review on the codex-acp side; will mark ready-for-review once the dependency chain unblocks the temp patch removal.