fix(agent): keep credential-pool entries when terminal-OAuth quarantine save fails

[roryford]

Supersedes #44856 (re-cut onto current main; the region was restructured since — notably the singleton source name changed to device_code and Nous now routes through _quarantine_* helpers — so this is a hand re-implementation, not a rebase).

Problem

On a terminal OAuth refresh failure (xAI OAuth, Codex OAuth, Nous device-code), agent/credential_pool.py evicts the pool entry and persists even when the auth-store save/clear failed — diverging the in-memory pool from on-disk auth.json.

Fix

Introduce a cleared flag per provider block: set cleared = True only after the save genuinely succeeds, upgrade the swallowed debug log to warning, and gate the entire removal block — removed_ids computation, the self._entries mutation, the _current_id reset, and _persist — behind if cleared:. (Gating only _persist would be insufficient: the in-memory mutation is itself the divergence.) return None stays unconditional. Applied consistently to all three provider blocks.

Verification

• Dynamic repro (red→green): three new tests (test_{nous,xai,codex}_..._keeps_entries_when_auth_store_save_fails) fail on unfixed main with assert [] == ['device_code'] (pool wrongly emptied when the save raised) and pass with this fix; on-disk tokens asserted untouched.
• Full local suite (1976 files) against this branch vs pristine base 889533545: zero new failures vs baseline.

Notes

• Related but separate (not fixed here): Bug: Dashboard credential pool delete succeeds but entries reappear on refresh #55217 (dashboard delete reappears) — different root cause (missing suppress_credential_source()).

[teknium1]

Thanks for covering all three terminal-refresh quarantine paths. The underlying bug is present on current main: failed auth-store clears are caught and followed by unconditional in-memory removal and persistence (agent/credential_pool.py:1222-1236, :1292-1306, :1356-1373).

Problems

• The proposed cleared = True is outside the nested write guard. For xAI, _save_auth_store() only runs when state["tokens"] is a dict (agent/credential_pool.py:1203-1221); Codex has the equivalent guard at :1273-1291. If that nested state is malformed, no save occurs but the proposed flag still permits eviction, preserving the divergence this PR is intended to prevent.

Suggested changes

• Assign cleared = True immediately after the actual _save_auth_store(auth_store) call in each successful quarantine branch.
• Add a malformed-nested-state regression test for the xAI/Codex guard.

This is an automated hermes-sweeper review.

[teknium1]

cleared should be set immediately after _save_auth_store(auth_store) inside the isinstance(tokens, dict) branch. If tokens is malformed, that branch performs no save but this post-block assignment still authorizes pool eviction.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address the same credential-pool divergence: both retain in-memory OAuth entries when quarantine persistence raises, but #63131 reimplements the fix for the restructured current code. The current diff still treats some no-write paths as successful clears, so it does not yet fully eliminate the reported inconsistency.

Related pull requests

• #44856 [closed] duplicate — (+195/-29) — superseded: This closed PR introduced save-failure guards for the xAI, Codex, and Nous terminal-refresh paths, but remains relevant as the original implementation and test basis superseded by the current-main reimplementation in #63131.
• #63131 related — (+207/-41) — keep open with a salvage path: The diff correctly gates entry removal and persistence when _save_auth_store() raises and adds provider-specific regression tests. The contributor keep_open review identifies a remaining diff-level flaw: for xAI and Codex, cleared = True is assigned outside the nested token-state write guard, so malformed nested state can permit eviction without any save; move the assignment immediately after each actual _save_auth_store(auth_store) call and add the requested malformed-state tests.

Duplicates

#44856 and #63131 implement substantially the same three-provider quarantine save-failure guard; #44856 is the stale predecessor superseded by #63131.

Suggested consolidation

Keep #63131 open with a salvage path: preserve its current-main adaptation, warning-level diagnostics, full removal gating, and three save-failure regression tests, while addressing the contributor keep_open review by tying cleared = True to an actual successful save and covering malformed xAI/Codex nested state. Keep #44856 closed as superseded by #63131.

Complex graph

flowchart 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
    subgraph Dup44856 ["PRs duplicating each other"]
        P44856["PR #44856 (closed)"]
        P63131["PR #63131 (open)"]
    end
    class P44856 closed
    class P63131 open
    class P63131 target
    click P44856 "https://github.com/NousResearch/hermes-agent/pull/44856"
    click P63131 "https://github.com/NousResearch/hermes-agent/pull/63131"

Loading

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 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 28 kB of PR diffs, 4 kB of issue/PR text, 3 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.