Skip to content

fix: replace assert with runtime guard in session recovery - #73050

Open
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/assert-session-recovery
Open

JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/assert-session-recovery

Conversation

@JonthanaHanh

Copy link
Copy Markdown
Contributor

Summary

assert output is not None at session_recovery.py:1241 is stripped by python -O, silently removing the invariant check before output.parent is accessed on the next line.

Replace with an explicit if output is None: raise SessionRecoverySafetyError(...) using the same error class that other validation checks in this function already use.

Changes

  • hermes_cli/session_recovery.py:1241: assert output is not None → if output is None: raise SessionRecoverySafetyError(...)

Test Plan

  • Syntax check passes
  • SessionRecoverySafetyError is already imported in the file

`assert output is not None` at session_recovery.py:1241 is stripped by
`python -O`, silently removing the invariant check before
`output.parent` is accessed on the next line.

Replace with an explicit `if output is None: raise` using the same
`SessionRecoverySafetyError` that other validation checks in this
function already use.
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 28, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tightening this recovery invariant. The targeted guard matches the module's existing safety-error convention.

Problems

  • The change has no regression test for its stated optimized-Python behavior. recover_session_database() still relies on the assertion at hermes_cli/session_recovery.py:1241 on current main, while existing recovery calls in tests/hermes_cli/test_session_recovery.py:435 and :516 provide valid output paths.

Suggested changes

  • Add a focused python -O regression test passing output_path=None directly to recover_session_database() and asserting SessionRecoverySafetyError. The CLI already rejects a missing output at hermes_cli/sessions_cmd.py:143-145, so direct API coverage is the relevant path.

Automated hermes-sweeper review.

work_dir=work_dir,
)
assert output is not None
if output is None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a regression test that invokes this path under python -O with output_path=None and asserts SessionRecoverySafetyError; that directly protects the optimization-sensitive behavior this guard replaces.

@teknium1 teknium1 added sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/sessions Session lifecycle, resume, persistence, history labels Jul 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants