Skip to content

fix(fallback): restore primary route after picker errors - #76497

Open
Xipong wants to merge 1 commit into
NousResearch:mainfrom
Xipong:fix/fallback-picker-restore-errors
Open

fix(fallback): restore primary route after picker errors#76497
Xipong wants to merge 1 commit into
NousResearch:mainfrom
Xipong:fix/fallback-picker-restore-errors

Conversation

@Xipong

@Xipong Xipong commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

hermes fallback add reuses the canonical model picker, which temporarily writes the selected route as the primary model/provider before appending it to the fallback chain.

The old cleanup path restored only part of that route and only on limited exits. Ordinary picker exceptions, post-picker config-read failures, and cleanup failures could leave the temporary route active or skip the second half of restoration.

Fix

  • Abort before launching the mutating picker if the primary auth route cannot be snapshotted.
  • Cover both the picker call and post-picker selection read/extraction with the same error-preserving cleanup boundary.
  • Restore the temporary primary route exactly once on the successful path, before identity/import/append work can fail.
  • Attempt model and auth restoration independently across BaseException, so an error or interrupt in one half cannot skip the other.
  • Preserve the original picker exception and attach cleanup failure details instead of masking it.
  • Preserve exact auth-store key presence: an absent active_provider remains absent rather than becoming persisted null.
  • Snapshot and restore the auth route under the auth-store lock.
  • Retain picker-created providers/credentials and other setup side effects while restoring only the primary model/auth route.
  • Refuse to print cancellation/success when route restoration failed.

Development context

This pre-existing fallback bug was discovered while developing the CLI subagent model/reasoning picker in #76480, because that feature reuses the same canonical provider/model setup flow. It is intentionally extracted here as an independent mainline fix so the picker PR remains feature-scoped.

This does not duplicate whole-config stale-write concurrency work already tracked in #62232.

Verification

  • 20 passedtests/hermes_cli/test_fallback_cmd.py
  • Regression coverage includes ordinary picker failure, post-picker read failure, cleanup errors and interrupts, independent two-store restore, exact absent-key restoration, and setup-side-effect retention.
  • Ruff passed for the changed production/test files.
  • py_compile passed.
  • Secret scan and git diff --check passed.

Coordination graph

Hermes triage dashboard graph

Live dashboard neighbourhood: https://hermes-triage.gottz.de/?node=76497

flowchart LR
    classDef focus fill:#fef3c7,stroke:#b45309,stroke-width:3px,color:#451a03
    classDef issue fill:#ede9fe,stroke:#6d28d9,color:#2e1065
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    P76497["PR #76497 (open)"]
    I85651(["issue #85651 (open)"])
    P6554["PR #6554 (merged)"]
    P9803["PR #9803 (closed)"]
    P33846["PR #33846 (closed)"]
    P36789["PR #36789 (closed)"]
    P47433["PR #47433 (closed)"]
    P60176["PR #60176 (open)"]
    P62417["PR #62417 (merged)"]
    P65656["PR #65656 (merged)"]
    P66400["PR #66400 (open)"]
    P71533["PR #71533 (open)"]
    P77631["PR #77631 (merged)"]
    P76497 -->|closes| I85651
    P76497 -->|related| I85651
    P60176 -.->|duplicate of 0.71| P76497
    P65656 -.->|duplicate of 0.71| P76497
    P71533 -.->|duplicate of 0.71| P76497
    P9803 -.->|duplicate of 0.71| P76497
    P62417 -.->|duplicate of 0.71| P76497
    P47433 -.->|duplicate of 0.71| P76497
    P66400 -.->|duplicate of 0.71| P76497
    P6554 -.->|duplicate of 0.70| P76497
    P36789 -.->|duplicate of 0.70| P76497
    P76497 -.->|duplicate of 0.70| P77631
    P33846 -.->|duplicate of 0.70| P76497
    class P76497 focus
    click P76497 "https://github.com/NousResearch/hermes-agent/pull/76497"
    class I85651 issue
    click I85651 "https://github.com/NousResearch/hermes-agent/issues/85651"
    class P6554 merged
    click P6554 "https://github.com/NousResearch/hermes-agent/pull/6554"
    class P9803 closed
    click P9803 "https://github.com/NousResearch/hermes-agent/pull/9803"
    class P33846 closed
    click P33846 "https://github.com/NousResearch/hermes-agent/pull/33846"
    class P36789 closed
    click P36789 "https://github.com/NousResearch/hermes-agent/pull/36789"
    class P47433 closed
    click P47433 "https://github.com/NousResearch/hermes-agent/pull/47433"
    class P60176 open
    click P60176 "https://github.com/NousResearch/hermes-agent/pull/60176"
    class P62417 merged
    click P62417 "https://github.com/NousResearch/hermes-agent/pull/62417"
    class P65656 merged
    click P65656 "https://github.com/NousResearch/hermes-agent/pull/65656"
    class P66400 open
    click P66400 "https://github.com/NousResearch/hermes-agent/pull/66400"
    class P71533 open
    click P71533 "https://github.com/NousResearch/hermes-agent/pull/71533"
    class P77631 merged
    click P77631 "https://github.com/NousResearch/hermes-agent/pull/77631"
Loading

Dashboard interpretation:

  • solid edges are structural GitHub links (closes / related);
  • dashed edges are embedding-discovery candidates and show the dashboard score;
  • the fold thresholds reported by the dashboard are embedding 0.88 and file overlap 0.75;
  • a dashed duplicate of edge below the fold threshold is a similarity lead for review, not an accepted duplicate or merge-order edge;
  • the gold node is this PR; purple nodes are issues; blue/gray nodes are open/closed neighbouring PRs.

This graph is additive to the hand-audited coordination block above: the dashboard supplies discovery neighbourhoods, while the declared dependency, collision, ownership, and merge-order edges remain the reviewed coordination contract.

@teknium1

teknium1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for isolating this cleanup path. The premise is valid: current main catches only SystemExit around the mutating picker in hermes_cli/fallback_cmd.py:165-171, so ordinary picker failures can leave the temporary route selected.

Problems

  • The new failure-path test mocks _restore_primary_route() rather than observing restoration after a picker writes a temporary config route (tests/hermes_cli/test_fallback_cmd.py, added test_picker_failure_restores_route_without_masking_original_error). It proves dispatch to the helper, but not the user-visible config invariant.

Suggested changes

  • Add an isolated_home test whose picker writes a replacement model then raises, and assert both original-error preservation and restoration of the persisted primary model.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 2, 2026
@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Aug 2, 2026
@Xipong
Xipong force-pushed the fix/fallback-picker-restore-errors branch from ff138ff to 4944099 Compare August 2, 2026 01:44
@Xipong

Xipong commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 4944099. The failure-path test now uses an isolated persisted config: the picker writes a temporary primary model and raises, then the test verifies both original exception identity and exact restoration of the persisted primary model.

@Xipong
Xipong force-pushed the fix/fallback-picker-restore-errors branch from 4944099 to a76394a Compare August 2, 2026 02:21
@Xipong

Xipong commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up after the final route-isolation audit (head a76394ac0):

  • extended the cleanup boundary through the post-picker config read/extraction, so a successful picker followed by an OSError, malformed state, or interrupt cannot strand the temporary route as primary;
  • moved successful restoration before identity/import/append work and made it exactly once;
  • both model and auth restoration are attempted across BaseException;
  • an absent active_provider remains absent, and auth snapshot/restore is locked;
  • picker-created provider/setup side effects remain preserved;
  • squashed the PR to one coherent commit.

Verification on the exact pushed head: 20 passed; Ruff, py_compile, secret scan, and git diff --check are clean.

@Xipong
Xipong force-pushed the fix/fallback-picker-restore-errors branch from a76394a to abe676c Compare August 7, 2026 20:21
@Xipong

Xipong commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (a8c50eb1d). Patch is unchanged apart from the new base — verified identical to the previous head.

Verification: tests/hermes_cli/test_fallback_cmd.py 20/20, py_compile clean.

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fallback picker errors can strand the temporary route as primary

3 participants