fix(cli): honor -w/--worktree in one-shot mode instead of silently ignoring it - #67476
fix(cli): honor -w/--worktree in one-shot mode instead of silently ignoring it#67476Sora-bluesky wants to merge 2 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
The underlying bug is present on current main: both one-shot dispatches omit worktree (hermes_cli/main.py:12970-12976, 15127-15133). The reuse of the existing worktree helpers is a reasonable salvage direction, but this version has blocking one-shot-contract issues.
Problems
cli._setup_worktree()prints successful setup output to stdout (cli.py:1630-1632). The new call occurs beforerun_oneshot()redirects output, sohermes -z ... -wpollutes machine-readable stdout.TERMINAL_CWDis captured afterfrom cli import ...; importingcliforce-exports the local cwd (cli.py:603-655). That cannot restore an in-process caller's original value.- Existing exact dispatch assertions at
tests/hermes_cli/test_tui_resume_flow.py:377-383and:616-622do not account for the new keyword.
Suggested changes
- Capture state before importing
cli; redirect setup/cleanup helper output to stderr; test setup output as well as cleanup output. - Update both existing dispatch tests and add explicit
-wforwarding coverage.
Automated hermes-sweeper review.
|
|
||
| repo = _git_repo_root() | ||
| if repo: | ||
| _prune_stale_worktrees(repo) |
There was a problem hiding this comment.
_setup_worktree() prints its successful creation banner to stdout (cli.py:1630-1632), but this call is before the one-shot redirect. Route setup output to stderr too, otherwise successful -z -w pipelines receive banner text before the final response.
| @@ -12982,6 +12982,7 @@ def _try_termux_fast_cli_launch() -> bool: | |||
| provider=getattr(args, "provider", None), | |||
| toolsets=getattr(args, "toolsets", None), | |||
| usage_file=getattr(args, "usage_file", None), | |||
| worktree=getattr(args, "worktree", False), | |||
There was a problem hiding this comment.
Please update the exact captured-kwargs assertion in tests/hermes_cli/test_tui_resume_flow.py::test_termux_fast_cli_launch_oneshot_uses_light_parser and add the corresponding -w forwarding case. This new keyword otherwise makes that existing test fail.
|
All three points addressed in
|
7bd3386 to
2a3b286
Compare
2a3b286 to
dc9926d
Compare
dc9926d to
786fb4a
Compare
|
Both points are addressed in the current branch: the worktree setup and cleanup output now go to stderr (commit "keep one-shot stdout clean during worktree setup"), so |
|
Thanks for the detailed review. It was against commit 5df0d1641a; the current head resolves all three, in a commit titled "keep one-shot stdout clean during worktree setup; restore TERMINAL_CWD":
Flagging since the salvageability=medium verdict was on the earlier commit. |
786fb4a to
2f1213c
Compare
2f1213c to
afd0141
Compare
afd0141 to
736bc81
Compare
SummaryThree open PRs address #67458 by forwarding Related pull requests
Duplicates#67471 and #67476 substantially implement the same honor- Suggested consolidationKeep #67476 open with a salvage path: rebase onto current main or split out its two-path forwarding, fail-closed worktree lifecycle, stderr routing, Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I67458(["issue #67458 (open)"])
subgraph Dup67471 ["PRs duplicating each other"]
P67471["PR #67471 (open)"]
P67476["PR #67476 (open)"]
P67484["PR #67484 (open)"]
end
P67476 -->|best fix| I67458
class I67458 open
class P67471 open
class P67476 open
class P67484 open
class P67476 best
class P67476 target
click I67458 "https://github.com/NousResearch/hermes-agent/issues/67458"
click P67471 "https://github.com/NousResearch/hermes-agent/pull/67471"
click P67476 "https://github.com/NousResearch/hermes-agent/pull/67476"
click P67484 "https://github.com/NousResearch/hermes-agent/pull/67484"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 72 kB of PR diffs, 11 kB of issue/PR text, 10 kB of discussion (14 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
736bc81 to
d63d685
Compare
d63d685 to
5d1712d
Compare
5d1712d to
bff9d4e
Compare
bff9d4e to
8063ab6
Compare
…noring it hermes -z accepted -w but never forwarded it: all three _run_and_exit_oneshot() call sites (the main dispatch, the Termux fast-CLI path and the fast chat-launch path) dropped the flag, so the agent ran in the current checkout and its commits landed directly on the live branch — the exact surprise the flag exists to prevent, and data-loss-adjacent for scripts that relied on '-z ... -w' for isolation. Forward the flag and mirror interactive chat's worktree lifecycle in run_oneshot(): prune stale worktrees, create the disposable worktree, retarget TERMINAL_CWD at it for the run, and clean up in the finally (which also covers KeyboardInterrupt/SystemExit — they re-raise after it). Setup runs BEFORE the stdout/stderr redirect so errors reach the terminal, and a requested-but-failed setup exits 2 instead of silently running without isolation — same refusal interactive chat makes. Two deliberate divergences from interactive, documented in the docstring: - The cleanup helper's print() output is routed to stderr: one-shot's contract is that stdout carries ONLY the final response, and the 'has unpushed commits, keeping' notice fires precisely in -w's main use case (the agent committed something). - The 'worktree: true' config default is not honored; one-shot is script-facing, so isolation stays opt-in per invocation. TERMINAL_CWD is restored afterward so in-process callers don't inherit a dangling path to the removed worktree. Tests: success path (TERMINAL_CWD retargeted during the run, cleanup called, stdout purity, no dangling env), requested-but-failed refusal, setup-exception hard error, and default-off passthrough. The first three fail against the previous code. Fixes NousResearch#67458 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ERMINAL_CWD on every exit Review follow-ups on NousResearch#67476 (hermes-sweeper + gpt-5.6-sol): - _setup_worktree()'s success output ('Worktree created: ...') printed to stdout before run_oneshot's redirect, polluting the machine-readable stream — route the prune/setup helpers through redirect_stdout(sys.stderr), matching the cleanup path's contract. - TERMINAL_CWD is now captured BEFORE 'from cli import ...': the import's config bridge force-exports the variable, so a post-import capture could never restore an in-process caller's original value. - Every exit path after that import — setup exception, setup returning None, and the normal finally — restores the captured value via one helper; previously the early returns leaked the force-exported value. - The exact-kwargs dispatch assertions in test_tui_resume_flow.py now include worktree=False, and both dispatch paths gain explicit worktree=True forwarding tests (top-level and Termux fast path). - Worktree tests re-pin TERMINAL_CWD after entering their patch context (mock.patch('cli....') itself imports cli, which force-exports the variable), so the exact-restore assertions hold in any test order. A faithful cold-import-ordering test isn't possible under module caching; the failure-path restore tests pin the observable contract instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8063ab6 to
0fede9b
Compare
What does this PR do?
hermes -z "..." -waccepted the worktree flag but silently ignored it: bothrun_oneshot(...)call sites dropped it — the main dispatch (hermes_cli/main.py) and the Termux fast-CLI path — so the agent ran in the current checkout and its commits landed directly on the live branch. That's data-loss-adjacent for scripts that relied on-z ... -wfor isolation, and it's the exact surprise the flag exists to prevent.This PR forwards the flag from both call sites and mirrors interactive chat's worktree lifecycle inside
run_oneshot(): prune stale worktrees → create the disposable worktree → retargetTERMINAL_CWDat it for the run → clean up in thefinally(which also coversKeyboardInterrupt/SystemExit, since they re-raise after it). Setup runs before the stdout/stderr redirect so errors reach the terminal, and a requested-but-failed setup exits 2 instead of silently running without isolation — the same refusal interactive chat makes (cli.pyreturns rather than run unisolated).Two deliberate divergences from interactive, documented in the docstring:
_cleanup_worktree's "has unpushed commits, keeping" notice fires precisely in-w's main use case (the agent committed something) — unredirected it would corruptout=$(hermes -z ... -w)pipelines.worktree: trueconfig default is not honored. One-shot is script-facing; isolation stays opt-in per invocation via the explicit flag.TERMINAL_CWDis restored afterward so in-process callers don't inherit a dangling path to the removed worktree.Related Issue
Fixes #67458
Type of Change
Changes Made
hermes_cli/oneshot.py—worktreeparameter onrun_oneshot(); setup/refusal before the output redirect; stderr-routed cleanup +TERMINAL_CWDrestore in thefinally.hermes_cli/main.py— forwardworktree=getattr(args, "worktree", False)at bothrun_oneshot(...)call sites (main dispatch and Termux fast path).tests/hermes_cli/test_oneshot_worktree.py— new: success path (agent runs withTERMINAL_CWDretargeted, cleanup called, stdout carries only the response, no dangling env), requested-but-failed refusal (exit 2, agent never runs), setup-exception hard error, and default-off passthrough (worktree machinery untouched). The first three fail against the previous code.How to Test
scripts/run_tests.sh tests/hermes_cli/test_oneshot_worktree.py tests/hermes_cli/test_oneshot_usage_file.py -qhermes -z "make a commit adding a line to README" -w→ the commit lands on a dedicatedhermes/wt-*branch in a disposable worktree; the checked-out branch is untouched; stdout contains only the agent's final response.Checklist
Code
scripts/run_tests.sh tests/hermes_cli/test_oneshot_worktree.py tests/hermes_cli/test_oneshot_usage_file.py— 10/10 pass on my platformmain;scripts/check-windows-footguns.py --diffis clean.Documentation & Housekeeping
run_oneshotdocuments the isolation contract and both divergences) — or N/Acli-config.yaml.example— N/A (no config surface added; the config default's non-application is documented in the docstring)CONTRIBUTING.md/AGENTS.md— N/Acli.py; no new OS-touching codeScreenshots / Logs
🤖 Generated with Claude Code