Add /rename builtin slash command#287
Conversation
0f258a0 to
e911a5f
Compare
e911a5f to
00deaa9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00deaa9fd3
ℹ️ 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".
| self.refresh_skills().await; | ||
| self.client | ||
| .send_notification(SessionUpdate::AvailableCommandsUpdate( | ||
| AvailableCommandsUpdate::new(self.available_commands()), | ||
| )); |
There was a problem hiding this comment.
Delay command updates until the session is visible
When session/new or session/load calls thread.load().await, this actor now sends AvailableCommandsUpdate immediately after resolving the internal oneshot, but before CodexAgent has responded to the outer ACP request with the session ID. The previous delayed task avoided this ordering problem. In clients that ignore or cannot attach updates for sessions they have not finished creating/loading yet, this notification can be dropped, leaving the slash-command palette without the builtins/skills until another update happens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d47615e by restacking #287 on the updated #279 tip (6bbb49f).
The available-commands update is delayed again so session/new / session/load callers can finish attaching the session before the notification arrives, but it is computed only after refresh_skills() from the current cache. That preserves the ordering behavior without reintroducing the stale pre-refresh snapshot. Local validation is green: cargo fmt --all -- --check, cargo test --locked --lib, cargo check --locked, cargo clippy --all-targets --all-features -- -D warnings, and git diff --check.
00deaa9 to
d47615e
Compare
|
@codex review |
d47615e to
0d31a62
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d47615e2d7
ℹ️ 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".
| self.cached_implicit_invocation_skills | ||
| .iter() | ||
| .find(|s| s.name == name) |
There was a problem hiding this comment.
Rewrite explicit-only skill slash commands
The advertised command list includes every enabled skill, but this lookup only matches cached_implicit_invocation_skills. For a skill with policy.allow_implicit_invocation: false, the user will still see and select /skill-name, but the prompt is not rewritten to $skill-name; it is sent to Codex as plain /skill-name text, so explicit-only skills exposed by the slash menu are not actually invoked despite the README saying selection is equivalent to $name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d31a62724
ℹ️ 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".
| .handle_set_title(title.to_owned(), Some(title.to_owned())) | ||
| .await |
There was a problem hiding this comment.
Sync
/rename titles to the state DB
When a user invokes the new /rename slash command in a normal session with SQLite state enabled, this path only calls handle_set_title, which appends to session_index.jsonl but never updates the StateDb row. The ACP session/setTitle path explicitly calls state_db.update_thread_title(...) before appending the index, so /rename leaves state_*.sqlite with the old title; any resume/search/listing surface that reads the SQLite metadata directly can keep showing or resolving the stale name after a successful rename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in feec0b3 as well. The slash /rename path now uses the same SQLite state DB title update that ACP session/setTitle relies on, then appends to the legacy session_index.jsonl title index and notifies the client. Local validation is green: cargo fmt --all -- --check, cargo test --locked --lib (37 tests), cargo check --locked, cargo clippy --all-targets --all-features -- -D warnings, and git diff --check.
0d31a62 to
feec0b3
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ 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". |
feec0b3 to
bbd2042
Compare
|
Restacked this branch in @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". |
bbd2042 to
7cf7167
Compare
|
Restacked again in @codex review |
7cf7167 to
0ce206b
Compare
|
Restacked once more in @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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ce206bbe9
ℹ️ 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 = match thread_titles.get(&thread_id) { | ||
| Some(Some(title)) => Some(title.clone()), | ||
| Some(None) | None => first_user_message.as_deref().and_then(format_session_title), |
There was a problem hiding this comment.
Preserve cleared titles in session lists
When a title is cleared via session/setTitle with a blank value, append_thread_name records an empty thread_name, and find_latest_thread_titles_by_ids returns Some(None) for that thread. This branch treats that the same as no custom-title entry and falls back to first_user_message, so after the next session/list refresh a cleared title reappears as the generated first-message title even though the client was notified that the title was cleared.
Useful? React with 👍 / 👎.
0ce206b to
a78550a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a78550a86b
ℹ️ 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".
|
This branch is supposed to be I just re-pushed it in that clean state. Flagging it because the branch has been rebased twice now with the #279 skills commit pulled back in (last on 2026-06-02), which bundles two unrelated features together and makes the diff much bigger than it needs to be. Is something automated rebasing this branch? If so, could it start from this |
a78550a to
b57de84
Compare
|
Restacked @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b57de84cac
ℹ️ 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".
b57de84 to
508f104
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
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.
508f104 to
749e71e
Compare
|
Rebased this onto the refreshed #286 stack ( Validation after the rebase:
@codex review |
Exposes `/rename <new title>` so users can rename the current ACP session from the chat input without a title-editor UI in the client. Works on stock Zed because the command resolves entirely inside codex-acp via the existing handle_set_title path that writes to $CODEX_HOME/session_index.jsonl and emits a SessionInfoUpdate so the client repaints its session list inline. The empty-arg case prints a usage hint instead of a silent no-op.
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
749e71e to
9306582
Compare
|
Restacked this on top of the updated #286 head ( Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9306582f18
ℹ️ 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".
| .send_agent_text("Usage: /rename <new session title>\n"); | ||
| } else { | ||
| match self | ||
| .handle_set_title(title.to_owned(), Some(title.to_owned())) |
There was a problem hiding this comment.
Normalize /rename title notifications
When /rename is used with a title containing a newline/CR or more than 120 graphemes, this passes the raw trimmed title into SessionInfoUpdate, but session/list displays titles via stored_session_title/format_session_title in src/codex_agent.rs, which replaces newlines and truncates. That means the client repaints to a different inline title than it will show after the next list/reload, bypassing the title-notification normalization fix; use the same notification formatting here before calling handle_set_title.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c13d0d9. /rename now formats the SessionInfoUpdate title through the same trim/newline/truncation path used for listed titles while keeping the persisted title unchanged. Added rename_notification_title_matches_session_list_formatting and extended the slash rename regression to assert the normalized SessionInfoUpdate. Validation is green: cargo fmt --all -- --check, git diff --check, CARGO_TARGET_DIR=/Users/danielstojanovic/Documents/Codex/2026-05-28/update-this-pr-and-make-it/codex-acp/target/pr286-check cargo check --locked, and CARGO_TARGET_DIR=/Users/danielstojanovic/Documents/Codex/2026-05-28/update-this-pr-and-make-it/codex-acp/target/pr286-check cargo test --locked --lib (36 passed).
|
Pushed Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
|
Hi thanks for taking the time to dig in! |
Summary
Adds
/rename <new title>as a builtin slash command for codex-acp. Typing/rename Fooin any ACP client (including stock Zed.app -- no client-side change required) renames the current session: the title is persisted tosession_index.jsonl, the SQLite state DB is updated, and the client repaints its session list inline via theSessionInfoUpdatenotification.Empty argument prints a usage hint instead of being a silent no-op. The new builtin is added to
builtin_command_names()so a same-named skill cannot shadow it.Why
Today users can rename a session only by opening the title editor in the agent panel header, which depends on Zed having merged the title-editor wiring and on the protocol/handler PRs landing. A slash command works from any ACP client immediately because the dispatch lives entirely inside codex-acp.
What's in this PR
thread.rs:renametobuiltin_command_names()andbuiltin_commands()with anUnstructured("new session title")input hint so the/picker shows the argument slot.handle_prompt, intercepts/renamebefore theOp::UserInputdispatch. Empty arg ->Usage:agent message. Non-empty -> callsself.handle_set_title(title, Some(title)), which already persists + emitsSessionInfoUpdate, then sends a confirmation chunk. ReturnsStopReason::EndTurnimmediately and never reaches the model.rename_is_listed_in_builtin_commands,slash_rename_without_arg_shows_usage_message,slash_rename_with_arg_writes_title_to_session_index.Tests
37 lib tests pass.
Dependencies (drafted until merged)
This PR is intentionally split out as a separate, smaller concern from the underlying rename infrastructure. It stacks on:
I'll mark this PR ready-for-review and rebase to a clean single-commit diff once the dependencies land.