feat: add --edit session flag to edit conversation before forking - #9799
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 422be626cd
ℹ️ 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".
422be62 to
b8c252c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8c252c4f9
ℹ️ 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".
|
Thanks for this! One naming concern: It's a cool power-user tool though. Could you attach the editing behavior to the existing Also, codex still has one open comment: |
b8c252c to
9ead476
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ead476a33
ℹ️ 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".
9ead476 to
a94329e
Compare
DOsinga
left a comment
There was a problem hiding this comment.
Thanks for iterating on this — the semantics are clean and read well (--resume = open it, --edit = edit it first, --fork = make a copy before doing all this). A few things before this can land:
-
(blocker) The
--edit --forkpath drops session metadata — see the inline thread oncli.rs. It should route throughcopy_sessionso it stays consistent with plain--fork, which also lets you collapse the three branches into one. -
let is_fork = fork;is a no-op rename — just useforkdirectly. -
editor.rshas several comments that just restate the code (// Inner resolution logic, separated for testability,// Always try to clean up the symlink,// Empty prefill should not appear in content, etc.). These can go — the names already carry the meaning. -
The
resolve_editor_*tests are over-fitted — e.g.test_resolve_editor_default_is_vijust asserts a constant, and there are many near-identical permutations. One test covering the config → VISUAL → EDITOR priority order plus the empty-skip behavior is plenty; the rest add noise without catching real bugs.
Will revisit in a few days.
Signed-off-by: Sean Murphy <murphysean84@gmail.com>
…tting - Rename --fork-session to --edit for a more composable interface: - --resume --edit: edit conversation in nvim before resuming - --resume --fork --edit: edit conversation then fork into new session - --resume --fork: unchanged (fork without editing) - Fix editor command parsing in both editor.rs files to use shlex::split() instead of split_whitespace(), so quoted paths like "/Applications/Sublime Text.app/.../subl" -w and empty-string args like emacsclient -a "" -t work correctly
…VISUAL bug - Remove commands/editor.rs and fold edit_conversation() into session/editor.rs, reusing existing launch_editor() and resolve_editor_command() — eliminates duplicated editor resolution and shlex parsing logic - Fix detect_editor() bug: empty/whitespace-only no longer panics on parts[0] indexing; falls through to nvim then default - Add resolve_editor_or_default() public helper wrapping config/env resolution with vi/notepad fallback - Add 5 tests for the new fallback and resolution logic - Make session::editor module public for cli.rs access
- Update in-session-actions.md: replace 'not available in CLI' with --edit and --fork --edit usage examples for both Edit in Place and Fork Session sections - Update goose-cli-commands.md: add --edit flag documentation and usage examples for editing and forking sessions
…logic, comments, and tests
6439800 to
e32a2c9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e32a2c981a
ℹ️ 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".
shlex::split applies Unix shell escaping which strips backslashes from unquoted Windows paths like C:\Windows\System32\notepad.exe. Only use shell-word splitting when the command contains quotes, otherwise split on whitespace.
|
Pushed a fix for the codex Windows-backslash comment: Thanks for the iterations — the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05d3be5e3b
ℹ️ 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 copied = session_manager | ||
| .copy_session(id, original.name.clone()) | ||
| .await?; |
There was a problem hiding this comment.
Defer creating the fork until edit succeeds
When --fork --edit is used and the editor exits non-zero or the YAML does not parse, this copy_session has already persisted a duplicate session with the unedited history; the command then returns before replace_conversation, leaving a stray most-recent fork even though the edit was rejected. Parse the edited conversation before creating the fork, or delete the copied session on any edit/replace failure.
Useful? React with 👍 / 👎.
DOsinga
left a comment
There was a problem hiding this comment.
CI is green and the Windows editor-path fix is in. The copy_session rework addressed the earlier blocker cleanly, and the --resume --fork --edit shape is nice. LGTM 🚀
|
Nice, thanks for the feedback, and helping me get this across the line. I kinda agree with the latest ai assisted review, so I'm going to put in a fix for that right now. Hopefully it runs out of feedback, lol :) |
* main: (26 commits) Fix MCP app sandbox bridge lifecycle (#10064) fix(bedrock): send inference config (max_tokens, temperature) on Converse (#9889) feat(providers): support OpenRouter request parameters (#9276) Migrate local inference model management to ACP (#10124) (attempt to) fix disk space errors in linux release builds (#10024) feat: add --edit session flag to edit conversation before forking (#9799) feat: add iFlytek Spark and Astron MaaS providers (#9837) fix(desktop): dedupe Nostr session deep link imports (#9918) [codex] Add SessionStart hook parity outside CLI (#9970) feat(providers): add Fireworks AI declarative provider (#9990) fix(providers): don't retry deterministically-permanent 400s (thinking-block immutability) (#10005) fix(deps): downgrade pkcs8 to v0.10 to match sec1/pkcs1 v0.7 (#10119) chore(deps): bump actions/cache from 5.0.2 to 6.0.0 (#10051) Make OpenAI Responses API store param configurable (#10040) remove unsupported model (#10121) chore(release): bump version to 1.40.0 (minor) (#10099) move ollama provider into goose-providers (#9986) UI acp migratoin: Decouple desktop UI types from generated OpenAPI types (#10109) fix(otel): use async reqwest client so OTLP export works in `goose serve` mode (#10100) feat (acp): exposed available tools in acp schema (#10097) ...
* main: (42 commits) Fix MCP app sandbox bridge lifecycle (#10064) fix(bedrock): send inference config (max_tokens, temperature) on Converse (#9889) feat(providers): support OpenRouter request parameters (#9276) Migrate local inference model management to ACP (#10124) (attempt to) fix disk space errors in linux release builds (#10024) feat: add --edit session flag to edit conversation before forking (#9799) feat: add iFlytek Spark and Astron MaaS providers (#9837) fix(desktop): dedupe Nostr session deep link imports (#9918) [codex] Add SessionStart hook parity outside CLI (#9970) feat(providers): add Fireworks AI declarative provider (#9990) fix(providers): don't retry deterministically-permanent 400s (thinking-block immutability) (#10005) fix(deps): downgrade pkcs8 to v0.10 to match sec1/pkcs1 v0.7 (#10119) chore(deps): bump actions/cache from 5.0.2 to 6.0.0 (#10051) Make OpenAI Responses API store param configurable (#10040) remove unsupported model (#10121) chore(release): bump version to 1.40.0 (minor) (#10099) move ollama provider into goose-providers (#9986) UI acp migratoin: Decouple desktop UI types from generated OpenAPI types (#10109) fix(otel): use async reqwest client so OTLP export works in `goose serve` mode (#10100) feat (acp): exposed available tools in acp schema (#10097) ...
* main: (31 commits) test: generic validator for declarative providers (#10010) UI acp migratoin: Decouple desktop UI types from generated OpenAPI types (Part 2) (#10149) Remove MCP sampling support (#10087) Support TLS for ACP serve (#10088) feat (ui): Migrate dictation local model manager to ACP (#10131) Fix MCP app sandbox bridge lifecycle (#10064) fix(bedrock): send inference config (max_tokens, temperature) on Converse (#9889) feat(providers): support OpenRouter request parameters (#9276) Migrate local inference model management to ACP (#10124) (attempt to) fix disk space errors in linux release builds (#10024) feat: add --edit session flag to edit conversation before forking (#9799) feat: add iFlytek Spark and Astron MaaS providers (#9837) fix(desktop): dedupe Nostr session deep link imports (#9918) [codex] Add SessionStart hook parity outside CLI (#9970) feat(providers): add Fireworks AI declarative provider (#9990) fix(providers): don't retry deterministically-permanent 400s (thinking-block immutability) (#10005) fix(deps): downgrade pkcs8 to v0.10 to match sec1/pkcs1 v0.7 (#10119) chore(deps): bump actions/cache from 5.0.2 to 6.0.0 (#10051) Make OpenAI Responses API store param configurable (#10040) remove unsupported model (#10121) ...
Summary
Adds a
--editflag togoose sessionthat opens the session's conversation in your editor as YAML, letting you edit/trim/rewrite messages before continuing. Combined with--fork, this enables "branching" a conversation at any point — remove unwanted messages, rewrite a prompt, or trim context before continuing.Usage
How it works
$VISUAL/$EDITOR/viChanges
cli.rs: New--editflag (requires--resume, can be paired with--fork)session/editor.rs: Newedit_conversation()— consolidated editor logic here, reusing existinglaunch_editor()andresolve_editor_command()commands/editor.rs— the separate module was unnecessary, editor resolution and shlex parsing were duplicated withsession/editor.rsresolve_editor_or_default()public helper wraps config/env resolution withvi/notepadfallbacksession/editor.rs: Fixeddetect_editor()bug where an empty/whitespace-only$VISUALwould bypass the$EDITORfallback and panic onparts[0]indexing — now properly skips empty values and falls throughsession/editor.rs: Changedshell-wordsplitting fromsplit_whitespace()(breaks on quoted paths like"/Applications/Sublime Text.app/.../subl") toshlex::split()with proper error handlingsession/mod.rs: Madeeditormodule publicTesting
$VISUALenv var, quoted editor paths with spacescargo fmt,cargo clippy -- -D warningsclean