fix(oneshot): honor resume state - #74397
Conversation
Related: #57859 and #70136 already implement one-shot session hydration. This patch adds explicit CWD restoration but differs in missing-session and first-use behavior, so please consolidate the intended resume contract rather than treating the patches as duplicates. |
|
Consolidation decision after comparing #57859 and #70136:
So the intended contract is: exact durable identity + canonical tip + history + recorded CWD, or fail visibly; never claim a resume after silently creating/forking/statelessly continuing. @teknium1, you reviewed the directly related #57859 path; could you review this consolidated, current-main variant? |
|
Follow-up pushed in |
|
Follow-up hardening pushed in
The two follow-up commits touch only |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for consolidating one-shot resume around the canonical session tip and adding fail-closed workspace handling. The current-main premise is valid: hermes_cli/oneshot.py:245-251 forwards no resume state, and hermes_cli/oneshot.py:410-443 creates a fresh session.
Problems
hermes_cli/oneshot.py:389changes only the process CWD. It does not retargetTERMINAL_CWD. The established interactive helper deliberately sets both (cli.py:7134-7142,cli.py:7178-7179), becauseagent/runtime_cwd.py:67-73andtools/file_tools.py:314-321preferTERMINAL_CWD. With an inherited/configured value, this resumes the history but still gives prompt construction and file/terminal tools the prior workspace.
Suggested changes
- After successful
os.chdir(saved_cwd), setos.environ["TERMINAL_CWD"] = saved_cwd. - Add coverage with a distinct pre-set
TERMINAL_CWDand assert it is changed before agent construction.
This is an automated hermes-sweeper review.
| f"{saved_cwd}" | ||
| ) | ||
| try: | ||
| os.chdir(saved_cwd) |
There was a problem hiding this comment.
Please also set os.environ["TERMINAL_CWD"] = saved_cwd after this succeeds. Interactive resume does this in cli.py:7178-7179; prompt, terminal, and file resolution prefer that variable, so os.chdir() alone can leave a resumed one-shot operating on the prior configured workspace.
There was a problem hiding this comment.
Addressed in e9db5835a: after os.chdir(saved_cwd) succeeds, one-shot resume now sets os.environ["TERMINAL_CWD"] = saved_cwd before the durable session is reopened and before AIAgent construction. Added focused coverage in b5123936c for stale-value replacement, missing/unenterable workspaces leaving the prior environment and session untouched, --no-restore-cwd preservation, and the value observed during agent construction. The implementation commit diff is limited to this synchronization. I reproduced the transition logic in an isolated environment (4/4); the full repository pytest run still requires a runnable checkout.
There was a problem hiding this comment.
Full validation is now complete. The exact PR code head b5123936cb13005ba446835ee46b5da5d24564f7 was tested against the same main base in a disposable internal PR; its only additional changes were contributor-attribution mappings required by the fork's governance check. CI run #19 passed all 8 Python slices (including the focused one-shot tests), Python E2E, desktop Playwright E2E, ruff/ty and Windows checks, OSV/supply-chain scans, and lockfile/history gates. The validation PR was closed without merge. Leaving this thread for reviewer confirmation rather than self-resolving it.
There was a problem hiding this comment.
Final verification on head c55ce708ed2889ca3e9ec4529f6da9b1fd48b609: the focused workspace tests now also restore the process CWD after each test, preventing their intentional production chdir() calls from leaking into later tests. Disposable validation PR SE87H/hermes-agent#4 completed full CI run #24 successfully; one unrelated asynchronous TTS timing flake passed when only its job was rerun, with no code change. Leaving the thread unresolved for reviewer confirmation.
|
Current-main integration postflight:
I am not force-updating or replacing current-main files from the older branch, because that would risk erasing newer upstream work. The next safe operation is a line-by-line rebase/cherry-pick in a real Git checkout, followed by the same full CI validation. The validated semantics and focused tests are ready; only current-main conflict resolution remains. |
|
Non-blocking architectural follow-up — not proposing to broaden this PR: Hermes already has a centralized read side in A useful follow-up could be a small typed write-side API with two deliberately separate modes:
Resume resolution could also return one resolved context — canonical durable session ID, filtered history, recorded workspace — and let each surface apply the appropriate workspace mode before reopening the session or constructing the agent. That would turn this split-brain class into one contract and test seam instead of requiring each surface to remember every carrier independently. The current PR should remain focused; this is only a possible follow-up design direction. |
|
Final third-eye validation pass:
No additional product behavior was added in this pass. The separate architectural comment is explicitly non-blocking and should not broaden this PR. |
SummarySeven PRs cover two one-shot flag-loss causes: #26669, #26771, and #72075 propagate rule isolation into Related pull requests
Duplicates#26669, #26771, and #72075 implement substantially the same Suggested consolidationKeep #72075 open as the consolidated salvage path for #26633/#72064 and close #26771 as duplicate of #72075 despite its keep-open review, because #72075's diff supplies the review's missing Termux forwarding and current tuple contract. Author action: rebase #74397 onto current main, preserve its validated session-tip/CWD/ 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
I49195(["issue #49195 (open)"])
P74397["PR #74397 (open)"]
P74397 -->|best fix| I49195
class I49195 open
class P74397 open
class P74397 best
class P74397 target
click I49195 "https://github.com/NousResearch/hermes-agent/issues/49195"
click P74397 "https://github.com/NousResearch/hermes-agent/pull/74397"
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 7 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 88 kB of PR diffs, 33 kB of issue/PR text, 18 kB of discussion (27 comments), 18 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
c55ce70 to
5931f54
Compare
|
M6R fresh-main rebase complete.
|
What changed
--resume,--continue, and--no-restore-cwdthrough the top-level-z/--oneshotpathTERMINAL_CWDafter a successful workspace restore so prompt, file, and terminal resolution use the same resumed workspace-z --continueprefer the current workspace before the historical global-MRU fallback, matching interactive CLI behaviorTERMINAL_CWDchdir()calls cannot leak into later testsRoot cause
The CLI parser accepted resume options for one-shot mode, but
_run_and_exit_oneshot()andrun_oneshot()did not forward or consume that state. Ahermes -z --resume <id>invocation therefore created a fresh session instead of continuing the requested durable row.The first patch also resolved bare
--continuedirectly from the global CLI MRU. The follow-up scopes that lookup to the current Git workspace/CWD first, matching the established interactive resolver and preventing an unrelated project session from winning when the current workspace has history.A later review identified a second workspace reference: Hermes prompt, file, and terminal resolution can prefer
TERMINAL_CWD. Restoring only the process CWD could therefore resume the right history while tools still targeted a stale configured workspace. The current patch updatesTERMINAL_CWDonly afteros.chdir()succeeds and before reopening the session or constructing the agent.Impact
One-shot callers can now resume a persistent Hermes conversation without forking the session or losing its recorded workspace. Process CWD and
TERMINAL_CWDare kept coherent for restored sessions. Failed workspace restoration leaves the prior environment and durable session untouched.--no-restore-cwdremains available for deliberate context changes. Bare--continuenow selects the current workspace's latest CLI session when available.Validation
tests/hermes_cli/test_tui_resume_flow.py— 75 passedTERMINAL_CWD, ordering before session reopen/agent construction, missing workspace, failedchdir, and explicit opt-outc55ce708ed2889ca3e9ec4529f6da9b1fd48b609SE87H/hermes-agent#4; the validation branch differed only by contributor-attribution mappings required by fork governanceScope
This PR intentionally remains a focused one-shot resume correction. A separate non-blocking follow-up comment proposes centralizing future workspace transitions behind typed task-local and process-local APIs; that architectural direction is not required for this fix and is not bundled here.