fix: preserve one-shot resume context - #70136
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the one-shot resume gap; current main still drops that state at hermes_cli/main.py:12365-12371.
Problems
- PR
hermes_cli/oneshot.py:327callsresolve_session_id(), which only accepts an exact or uniquely prefixed ID (hermes_state.py:4547-4572). The parser documents--resumeas accepting an ID or title (hermes_cli/_parser.py:157-162), and normal CLI resolves titles athermes_cli/main.py:2526-2533. - PR
hermes_cli/main.py:13377-13378and15537-15538forward only explicit resume/session-ID state.--continueand--no-restore-cwdremain accepted by the parser but bypassed in one-shot mode. - The compression child is now published atomically via
agent/conversation_compression.py:2147-2160; the PR's direct child-creation hunk needs to be relocated during salvage.
Suggested changes
- Use the normal ID-or-title/continue resolution contract, including recorded-CWD restoration and its opt-out.
- Rework the lineage marker at the current atomic publication boundary and validate it against current resume tests.
Automated hermes-sweeper review.
| return None, [] | ||
| if session_db is None: | ||
| raise RuntimeError("Session database is unavailable; cannot resume safely") | ||
| resolved = session_db.resolve_session_id(requested) |
There was a problem hiding this comment.
resolve_session_id() only resolves an exact or uniquely prefixed ID. The top-level parser documents --resume as accepting an ID or title, and normal CLI startup resolves titles before resume; add the equivalent title fallback here so one-shot does not reject valid titled sessions.
| provider=getattr(args, "provider", None), | ||
| toolsets=getattr(args, "toolsets", None), | ||
| usage_file=getattr(args, "usage_file", None), | ||
| resume=getattr(args, "resume", None), |
There was a problem hiding this comment.
This forwards only explicit --resume. One-shot still bypasses parsed --continue and --no-restore-cwd, while normal cmd_chat() implements both; carry those options through this dispatch and the Termux fast path so the resume contract is consistent.
Three gaps in the oneshot resume path, found reviewing NousResearch#80799 against CONTRIBUTING.md and the review findings on sibling PR NousResearch#70136. 1. `--resume` bypassed the ID-or-title resolution contract. `_parser.py` documents `--resume SESSION` as "by ID or title" and `cmd_chat` resolves titles via `_resolve_session_by_name_or_id`. `_resolve_oneshot_resume` passed the value through verbatim, so `hermes --resume "my project" -z "..."` silently minted an empty session literally named "my project" instead of resuming — the same silently-drops-a-flag class as the bug being fixed, and the exact finding teknium1 gave NousResearch#70136. Now resolves first; an unresolved value still falls through to create-on-first-use, so the design intent (scripted callers minting stable ids) is preserved. The asymmetry was visible inside the PR itself: `--continue "name"` already resolved. 2. Create-on-first-use was an unguarded entry boundary for caller-supplied session ids. A session id becomes a filename downstream — `SessionDB._remove_session_files` builds `sessions_dir / f"{id}.json"` and globs `request_dump_{id}_*.json` without sanitizing — so `hermes --resume ../../x -z "hi"` minted a row that a later `sessions delete`/`prune` unlinks outside the sessions dir (verified: the victim file is removed). `gateway/session.py` already rejects exactly this at its own entry boundary (`_is_path_unsafe`, CWE-22); the new CLI boundary now does too. Path-unsafe values that *resolve* to a real session are still fine — the guard only applies to values about to become new ids, so titles containing `/` keep working. 3. The integration test asserted a dedup property it never exercised. `_RecordingFakeAgent` appended only its own two new messages, so "4 rows, not 8" held by construction regardless of whether the flush's identity-based seeded-history dedup worked. It now drives the real `AIAgent._flush_messages_to_session_db` with the real `messages = conversation_history + this turn` shape; break the identity contract and the assertion fails (verified by mutation). Also adds the coverage teknium1's review would still have flagged: `_resolve_oneshot_resume` had zero tests (title, exact id, unknown id, path-unsafe, `--continue` by name / bare / unmatched), a third-turn accumulation case, and the compression-chain redirect — a caller passing the pre-compression parent id must land on the continuation child, with this turn's writes going there and not into the dead parent. Docs: `-z` help and the `hermes_cli/oneshot` module docstring now state ID-or-title, create-on-first-use, and that an unmatched `--continue` errors while an unmatched `--resume` does not. tests/hermes_cli/test_oneshot_resume.py: 26 passed (was 9).
What does this PR do?
Prevents one-shot resume calls from silently creating or binding to the wrong durable session.
The top-level
--oneshotshortcut previously dropped--resumeand--pass-session-id; the one-shot runner therefore started a fresh agent without hydrating the requested transcript. Callers that tried to recover identity by selecting the newest same-directory session could then bind to a nested, delegated, tool, or otherwise unrelated session.Changes made
--resumeand--pass-session-idthrough the top-level one-shot path.session_metarecords before replay._compression_from.How to test
uv run --with pytest python -m pytest \ tests/hermes_state/test_resolve_resume_session_id.py \ tests/hermes_cli/test_oneshot_resume.py \ tests/hermes_cli/test_tui_resume_flow.py \ tests/hermes_cli/test_oneshot_usage_file.py \ -o addopts='' -qResult: 97 passed.
Type of change