fix(secrets): fall back to stale disk cache when bws live fetch fails

[jackjin1997]

What does this PR do?

When _run_bws_list() raises (DNS down, transient BWS outage, network blip), fetch_bitwarden_secrets() previously propagated the RuntimeError straight through. The env_loader caller catches it and the gateway just runs without any injected secrets — every model call then fails with Provider 'X' is set in config.yaml but no API key was found.. A fleet of bots sharing one BWS project all stop working on a single network glitch.

This PR wraps the live fetch in a try/except and, when the disk cache already holds secrets from a previous successful fetch, returns those secrets (regardless of TTL) with an explicit warning instead of raising. The behaviour matches the existing fresh-cache path closely: same return shape, same in-process promotion, just an extra warning carrying the cache age + the original error so operators see what happened.

Related Issue

Fixes #41925

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• agent/secret_sources/bitwarden.py: when _run_bws_list() raises RuntimeError, attempt _read_disk_cache(cache_key, ttl_seconds=float('inf'), home_path) to bypass freshness. On hit: promote into _CACHE, return (secrets, [warning]). On miss: re-raise the original exception. use_cache=False still raises (explicit opt-out — used by the setup wizard so it can surface the real error). Disk cache is not re-written, so a process restart still does a proper TTL re-check.
• tests/test_bitwarden_secrets.py: 5 regression tests + a _seed_stale_disk_cache helper:
  • test_stale_disk_cache_returned_when_bws_fails — happy path: stale entry exists → secrets returned + warning
  • test_stale_fallback_warning_includes_cache_age — warning contains the cache age so operators can decide whether to act
  • test_no_stale_fallback_when_disk_cache_missing — no cache → re-raise (no silent empty secrets)
  • test_stale_fallback_skipped_when_use_cache_false — explicit opt-out is honoured
  • test_stale_fallback_does_not_overwrite_disk_cache — disk fetched_at preserved so the next process restart triggers a proper re-check

How to Test

pytest tests/test_bitwarden_secrets.py -k stale -v   # 5 new tests pass
pytest tests/test_bitwarden_secrets.py -q            # full file (pre-existing env_loader / install_bws failures are Python 3.9 syntax issues unrelated to this change)

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix (no unrelated commits)
• I've added tests for my changes
• I've tested on my platform: macOS

Documentation & Housekeeping

• I've updated relevant documentation — N/A (behaviour change is operator-facing; warning text makes the new path observable)
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've considered cross-platform impact — N/A (pure Python, no platform-specific code)

Code Intelligence

• Analyzed: fetch_bitwarden_secrets (callers: bsm_pull_into_env env loader path L642 + 2 setup-wizard call sites in hermes_cli/secrets_cli.py)
• Blast radius: LOW — additive fallback only triggers when the live fetch raises. Fresh-cache / use_cache=False / no-disk-cache callers see no behavioural change.
• Related: _read_disk_cache line 111-134 (re-used as-is via ttl_seconds=float('inf') — no signature change), _CachedFetch.is_fresh (correctly returns True for inf so cache key lookup is unaffected)

AI Disclosure

This bug was identified and fixed with AI assistance.

[liuhao1024]

Positive verification — reviewed the full diff and 5 tests.

The fallback logic is sound: catch RuntimeError from _run_bws_list, check use_cache, attempt _read_disk_cache with ttl_seconds=inf to bypass freshness, and re-raise if no cache exists. The stale entry is returned without bumping fetched_at on disk — critical to avoid falsely marking the cache as fresh for future processes.

Edge cases covered by tests: no disk cache → re-raise, use_cache=False → re-raise, age reporting in warning, disk cache not overwritten on stale fallback.

One note: the warning string includes the raw exc message which may contain BWS stderr output. This is fine for operator-facing logs but worth being aware of if it surfaces in user UI later. Not a blocker.

No issues found. Looks good to merge.

[jackjin1997]

Thanks for the careful review @liuhao1024. Quick note on the stderr concern: the exc message comes from _run_bws_list at line ~558, which formats it as f"bws exited {rc}: {err[:200]}" where err is bws's own stderr output (stripped of ANSI, capped at 200 chars). The access token never reaches stderr — it's only ever passed via env["BWS_ACCESS_TOKEN"], never in argv. bws's stderr in practice carries operator-actionable strings like "Error: invalid access token" or DNS failure messages. Happy to redact further if maintainers prefer a stricter posture.

[jackjin1997]

Gentle nudge for a maintainer look — this one's been sitting two weeks. It fixes #41925 (a BWS DNS/transient failure silently wiping the secret set fleet-wide), is still CLEAN/mergeable on current main, and @liuhao1024 already did a positive diff+test verification above. Scope is a single fallback path in bitwarden.py with 5 regression tests. Happy to rebase if it's drifted.

[teknium1]

Thanks for the focused availability fix. The stale-cache outage premise remains real on current main (agent/secret_sources/bitwarden.py:394-410), but this needs rework before it can be salvaged.

Problems

• The patch uses removed private helpers (_read_disk_cache / _write_disk_cache). Current main moved them to the shared DiskCache API in db495b0fbaaa63ebd7f6404413730f98f0fdf76b; Bitwarden now uses _DISK_CACHE.read at agent/secret_sources/bitwarden.py:394.
• The fallback at PR line 506 ignores cache_ttl_seconds: 0. That conflicts with the current contract in agent/secret_sources/_cache.py:107-109: non-positive TTL disables cache reads and writes.
• Catching every RuntimeError also serves stale secrets after invalid credentials or malformed BWS output (agent/secret_sources/bitwarden.py:450-469). The source contract identifies stale fallback as appropriate for NETWORK/TIMEOUT, not AUTH_FAILED (agent/secret_sources/base.py:65-69).

Suggested changes

• Port the fallback and its fixtures to _DISK_CACHE.read / .write.
• Gate fallback on both caching being enabled and a positive TTL, with a zero-TTL regression test.
• Restrict fallback to classified network/timeout failures and test auth/parse failures still raise.

Automated hermes-sweeper review.

[teknium1]

This catches auth failures and malformed BWS responses too. Current ErrorKind explicitly reserves stale fallback for NETWORK/TIMEOUT rather than AUTH_FAILED (agent/secret_sources/base.py:65-69); classify the error and only reuse stale secrets for those transient cases.

[teknium1]

Please also require cache_ttl_seconds > 0 here. Current shared-cache semantics define TTL <= 0 as disabling cache reads and writes (agent/secret_sources/_cache.py:107-109), but this inf read would revive an existing disk entry when caching is explicitly disabled.

[jackjin1997]

Good catch on all three — verified each independently:

1. _read_disk_cache/_write_disk_cache were indeed removed in db495b0fb; the fallback branch was calling a name that no longer exists, so every fallback attempt raised NameError instead of serving cached secrets — the PR's whole purpose was silently defeated. Ported to _DISK_CACHE.read().
2. Gated the fallback on cache_ttl_seconds > 0, matching DiskCache's "non-positive TTL disables both read and write" contract, so a caller that opted out of caching entirely never gets a stale value on the failure path either.
3. Restricted the fallback to _classify_bws_error(str(exc)) being NETWORK/TIMEOUT (reusing the existing classifier already used by BitwardenSecretSource.fetch()) — an AUTH_FAILED or malformed-output failure now still raises.

Reverted the fix and reran: 7 of 8 stale-fallback tests fail with the original NameError, confirming the tests are meaningful. Added coverage for the auth-failure, malformed-output, and zero-TTL gates. Rebased onto current main (branch had drifted ~4400 commits) — still CLEAN. 50/50 relevant tests pass, ruff clean.

[teknium1]

Merged via #69051 — your two commits landed as-is (cherry-picked, authorship preserved), ported onto the current DiskCache API with the fallback gated by the shared ErrorKind taxonomy so AUTH_FAILED never serves stale secrets. The 7-test suite you wrote (opt-outs, timestamp preservation, auth/malformed still raising) was the deciding factor in picking this PR over the competing implementation. Thanks @jackjin1997! Fixes #41925.