fix(statusline): handle suffixed Claude Code keychain service names

[halindrome]

What

Fix #576 — VBW statusline's Limits line wrongly shows "keychain access denied" on macOS when Claude Code stores OAuth credentials under per-install suffixed service names (e.g. Claude Code-credentials-126018b2).

Why

scripts/vbw-statusline.sh Priority 2 only queried the literal service name Claude Code-credentials. On affected installs the keychain has no item under that exact name (security returns exit 44, errSecItemNotFound), so the lookup silently returned empty. With no .credentials.json fallback file present and claude auth status --json reporting authMethod=claude.ai, control reached lines 956–970 which emitted Limits: keychain access denied (allow Terminal in Keychain Access.app or set VBW_OAUTH_TOKEN) — a misleading message that:

• blames a permissions denial that never happened (the actual condition is item-not-found, not access denied), and
• suggests a fix ("allow Terminal in Keychain Access.app") that cannot resolve item-not-found.

How

Three changes in scripts/vbw-statusline.sh, plus extensive BATS coverage. Built up across the original commit and two QA-driven follow-ups.

1. Suffixed-name lookup (Priority 2, macOS branch)

• Try the literal Claude Code-credentials first (back-compat for older installs).
• On miss, enumerate Claude Code-credentials-<suffix> candidates via a non-prompting security dump-keychain filter (no -d, so secrets are not dumped and no GUI prompt fires).
• Iterate candidates and yield .claudeAiOauth.accessToken from each — collect all valid tokens into _KEYCHAIN_FALLBACK_TOKENS.

2. Multi-install try-all-on-401 fallback (added in QA round 1)

The original commit picked the lexically-first valid token. The #576 reporter has six suffixed entries; if the chosen install's token is stale, the user would have been pinned to Limits: auth expired (run /login) until cache expiry — even though another install holds a fresh token.

• Build a candidate list (primary token + alternates from _KEYCHAIN_FALLBACK_TOKENS, deduped).
• In the usage-fetch loop, on 401/403 continue to the next candidate; 429 and other failures break immediately (retrying wouldn't help).
• Cap at 4 attempts so users with many stale installs aren't blocked by 6 × the 3-second curl timeout per render.

3. Honest fallback message (line 964–965)

Replaced Limits: keychain access denied (...) with Limits: OAuth token unavailable (set VBW_OAUTH_TOKEN) — the branch fires whenever priorities 1–3 all return empty AND claude auth status confirms an OAuth login, regardless of why the keychain didn't yield a token.

Design note: the issue's proposed fix included branching the message on security's exit code (44 = item-not-found vs 51 = access-denied). That distinction was deliberately collapsed here to avoid bumping the slow-cache schema (15 → 16 fields with back-compat read). The env-var workaround already resolves both sub-cases, and the inline comment at the message-branch site documents this for future maintainers.

Linux paths (secret-tool/pass) are unchanged — the upstream Claude Code suffix scheme on Linux needs separate confirmation. Tracked as #580 follow-up.

Test plan

• New tests/statusline-keychain-suffixed.bats (4 cases, all passing):
  • Legacy literal service name still resolves (back-compat).
  • Suffixed Claude Code-credentials-<hex> is discovered when the literal is absent — asserts the suffixed token actually flowed to the curl call.
  • Multi-install try-all-on-401 — primary token returns 401, fallback succeeds; asserts both tokens were tried in order.
  • claude.ai login + no reachable token emits the new "OAuth token unavailable" message and no longer says "keychain access denied".
• All 73 existing tests/statusline-*.bats still pass.
• bash testing/run-all.sh clean across all three commits: 1/1 lint, 51/51 contracts, 3318/3318 BATS, zero failures.

Notes

• BATS negative assertions use [[ ! "$str" =~ "..." ]] rather than ! grep -q — the latter does not fail BATS tests (SC2314), so the inverse-grep pattern would silently no-op.
• security dump-keychain (no -d) is non-prompting and dumps only metadata; it does not trigger a Keychain Access dialog and does not read secret payloads.
• Mock helpers use a portable while IFS= read -r loop instead of mapfile -t (bash 4+ only) so contributors running BATS on macOS default /bin/bash 3.2 are covered.

QA review log

Per CONTRIBUTING.md, each round's report has been posted as a separate PR comment.

• Round 1 (commit 03aa03da) — addressed multi-install non-determinism via try-all-on-401, plus minor polish.
• Round 2 (commit e196d758) — mapfile portability fix, candidate-count cap, CRED_JSON unset symmetry, inline design-choice note.
• Round 3 — pending.

🤖 Generated with Claude Code

[halindrome]

QA Round 1

- Model used: Claude Opus 4.7 (1M context)
- What was tested: PR #578 commit narrative; full diff of scripts/vbw-statusline.sh (lines 736-770, 985-992); new tests/statusline-keychain-suffixed.bats (3 tests + mock security/curl); surrounding cache lifecycle (slow cache schema lines 885-899, FETCH_OK branches 944-997); real-world `security dump-keychain` output format and timing on a system with 6+ suffixed Claude entries; AUTH_METHOD assignment paths (Priority 4 at line 813); back-compat for older slow-cache entries; Linux paths (lines 763-781, intentionally untouched); Issue #576 description and proposed fix.

- Findings:

  - "First-token-wins" with sort -u order is non-deterministic across users
    - Expected vs actual: Issue #576 reporter has 6 suffixed entries; the comment explicitly anticipates this. Code picks the first whose JSON yields a token, ordered by `sort -u` (lexical) on the suffix hex. There is zero signal as to which install is "current". On my own machine I observed entries `-126018b2`, `-183c0f3a`, `-42414e16` (and more) — `sort -u` would pick `-126018b2`. If that install's token is stale/revoked, the API call returns FETCH_OK=auth and the user sees "Limits: auth expired (run /login)" forever, even though a fresher token exists under another suffix. The cache then locks that wrong choice in for 60s (or 300s on backoff). The loop only breaks on first non-empty token — it does not validate freshness.
    - Severity: major
    - Hypothetical (the regression would only fire on multi-install users, but #576 explicitly cites such a user)
    - Location: scripts/vbw-statusline.sh:752-769

  - Pipeline failure silently swallowed; no diagnostic when dump-keychain itself denies access
    - Expected vs actual: The pipeline `security dump-keychain 2>/dev/null | grep -oE … | sed … | sort -u` redirects stderr to /dev/null and never inspects exit status. If `security dump-keychain` ever does prompt or fail (e.g. locked non-default keychain in the search list, SIP issues, exit 51), `_SUFFIXED_NAMES` is empty and the user falls through to "OAuth token unavailable (set VBW_OAUTH_TOKEN)" — losing the original fix's value. The proposed-fix in #576 specifically called for "Exit-code-aware messaging" with the 51 → keep "keychain access denied" branch. PR drops that part entirely.
    - Severity: major
    - Confirmed by reading commit + diff vs issue #576 "Proposed fix"
    - Location: scripts/vbw-statusline.sh:752-756 and 985-992

  - `security dump-keychain` runs every render on miss path (no negative cache)
    - Expected vs actual: When literal lookup misses (the common case for newer installs — the very users this PR targets), `security dump-keychain` runs on every statusline render where the slow cache hasn't sealed an OAuth token yet. Measured ~15ms / ~190KB output / 27 matched lines on my dev machine — fine. But the slow cache stores AUTH_METHOD/AUTH_CLASS/FETCH_OK, not OAUTH_TOKEN — so when the cache expires (every 60s, or 300s on persistent failure), the keychain code path re-runs from the top. With 6 suffixed entries the loop performs up to 6 `security find-generic-password` calls per render. Worst-case user with locked keychain: 6 sequential fork+exec each render every 60s. Worth confirming acceptable but not a blocker.
    - Severity: minor
    - Confirmed (timing measurement)
    - Location: scripts/vbw-statusline.sh:752 (dump invocation), 885 (cache schema)

  - Mock `security` does not faithfully reproduce real dump-keychain output (no leading whitespace fidelity check)
    - Expected vs actual: Real output is `    "svce"<blob>="Claude Code-credentials-126018b2"` (with leading 4-space indent and lots of surrounding lines). Mock prints `    "svce"<blob>="<name>"` and nothing else. The regex `grep -oE '"svce"<blob>="Claude Code-credentials-[^"]+"'` happens to be position-independent so it works on both — fine — but the mock will also pass even if a future bug changed the regex to require BOL anchoring (`^"svce"...`), since real output has 4 spaces of indent. The test wouldn't catch that regression. Tighter assertion: include realistic indentation and surrounding noise lines (class/keychain/attributes) in the dump-keychain mock.
    - Severity: minor
    - Confirmed
    - Location: tests/statusline-keychain-suffixed.bats (mock _install_mock_security `dump-keychain` branch)

  - Test does not assert the suffixed branch was actually exercised
    - Expected vs actual: The "suffixed discovered" test only asserts the negative — neither "OAuth token unavailable" nor "keychain access denied" appears in line 3. With FETCH_OK="ok" via mocked curl, the test would pass even if the suffixed lookup never ran (e.g. if a future regression made it skip the dump path entirely but env var or credentials.json provided a token). Should also assert positive evidence — e.g. line 3 contains "Session:" / "Weekly:" progress bars (proving FETCH_OK=ok was reached), and ideally that Authorization header carried `fake-token-suffixed`. Mock curl currently ignores all input.
    - Severity: minor
    - Confirmed
    - Location: tests/statusline-keychain-suffixed.bats:130-135 + mock curl (always returns 200)

  - Cleanup `unset _SUFFIXED_NAMES _sn` misses CRED_JSON exposure
    - Expected vs actual: After the loop, `CRED_JSON` (which holds the full credential JSON for the matching suffixed entry, including refresh tokens) remains in env. It's not exported, but the script is procedural, sourced into a single bash process — any later `echo $CRED_JSON` or accidental reuse would leak. Existing literal-lookup branch also leaves CRED_JSON behind, so this matches existing style. Worth a `unset CRED_JSON` after the keychain block as a hygiene improvement, not a bug.
    - Severity: minor
    - Confirmed
    - Location: scripts/vbw-statusline.sh:768

  - "OAuth token unavailable (set VBW_OAUTH_TOKEN)" loses actionable diagnostic for genuine exit-51 case
    - Expected vs actual: Issue #576 explicitly proposed exit-code-aware messaging — exit 44 → "OAuth token unavailable", exit 51 → keep "keychain access denied". PR collapses both into a single message. A user who actually hit a permission denial (Terminal not in Keychain Access ACL) now gets advice that won't fix their problem. They'll set VBW_OAUTH_TOKEN, which works, but the underlying ACL stays broken and `claude` itself may continue to fail. The PR is strictly better than the status quo but is missing half of the proposed fix.
    - Severity: minor (judgment call: could be major depending on how often exit 51 fires in practice)
    - Confirmed
    - Location: scripts/vbw-statusline.sh:986-992

  - Heredoc `<<< "$_SUFFIXED_NAMES"` when variable is empty enters loop with empty IFS line
    - Expected vs actual: When dump-keychain returns nothing (no Claude entries at all), `_SUFFIXED_NAMES=""` and `<<< ""` feeds a single empty line into the read loop. The `[ -z "$_sn" ] && continue` guard handles it correctly. Behavior is correct; called out only because it's a subtle bash idiom and the guard is load-bearing. No bug.
    - Severity: minor (none — informational)
    - Confirmed
    - Location: scripts/vbw-statusline.sh:756-767

  - Linux scope decision is reasonable but undocumented in user-facing wording
    - Expected vs actual: Linux secret-tool/pass paths only query the literal "Claude Code-credentials". Newer Claude Code on Linux likely uses the same suffix scheme (the Electron credential code is shared), so Linux users will hit the same regression. PR commit explicitly defers this. Acceptable scope decision, but the new "OAuth token unavailable (set VBW_OAUTH_TOKEN)" message will fire on Linux suffixed-install users with no recourse. A follow-up issue should be filed before merge so it doesn't get lost.
    - Severity: minor
    - Hypothetical (depends on Linux Claude Code internals)
    - Location: scripts/vbw-statusline.sh:763-781

  - Back-compat path for older Claude Code installs is intact
    - Expected vs actual: Literal `find-generic-password -s "Claude Code-credentials" -w` runs first, returns immediately on match, and the suffixed block is skipped via `if [ -z "$OAUTH_TOKEN" ]`. Verified by reading the diff and the back-compat test. No regression for the legacy code path.
    - Severity: none (positive finding)
    - Confirmed
    - Location: scripts/vbw-statusline.sh:740-745

- Recommendation: Address two items before merge — (1) restore exit-code-aware messaging from the issue's proposed fix (capture `security dump-keychain` exit and the per-candidate `find-generic-password` exit; if any returns 51 vs all returning 44, branch the message), and (2) make the suffixed-name selection deterministic and safer — either iterate ALL candidates and pick the one whose access token survives an actual API call (use existing curl probe) or document that the policy is "first by lexical sort wins" and accept the multi-install risk. Tighten the suffixed-discovery test to assert positive evidence the suffixed branch ran (Session:/Weekly: in line 3, or inspect the mock curl's recorded Authorization header). File a follow-up issue for Linux suffixed-name parity. Other findings are minor polish.

[halindrome]

QA Round 2

- Model used: Claude Opus 4.7 (1M context)
- Round: 2
- What was tested:
  - Both commits (f0c56e76 + 03aa03da) and the cumulative gh pr diff
  - Round 1 QA report (gh pr view --comments)
  - scripts/vbw-statusline.sh: keychain Priority 2 block (lines ~736-775), candidate-list build + try-all-on-401 usage-fetch loop (~826-892), Priority 3 credentials.json, Priority 4 auth CLI, FETCH_OK message branches
  - tests/statusline-keychain-suffixed.bats: all 4 tests + _install_mock_security, _install_mock_security_multi, _install_mock_curl_ok, _install_mock_curl_401_then_ok helpers
  - Verified: script lacks `set -u` (safe for empty array expansion); OAUTH_TOKEN reassignment scope; CRED_JSON unset placement; FETCH_OK final-state matrix

- New findings (introduced by round 1 fixes):

  - Mock security helper uses `mapfile -t` which requires bash 4+; statusline script and other tests carefully avoid bash-4-only constructs for macOS bash 3.2 compatibility
    - Expected vs actual: New `_install_mock_security_multi` writes a mock script with `mapfile -t NAMES_ARR <<< "$NAMES_RAW"` and `mapfile -t TOKENS_ARR <<< "$TOKENS_RAW"`. If `/usr/bin/env bash` resolves to /bin/bash on macOS (3.2.57), mapfile is undefined → mock fails silently → multi-install test produces unexpected behavior.
    - Severity: minor (most CI/dev environments have bash 4+; degrades gracefully to test failure not silent pass)
    - Confirmed
    - Location: tests/statusline-keychain-suffixed.bats (`_install_mock_security_multi`, `mapfile -t` lines)

  - Multi-install test does NOT positively assert "both tokens were tried" despite the round-1 fix commit narrative claiming it does
    - Severity: minor
    - Hypothetical (depends on what follows the truncated comment "stale fi…"; worth confirming the test contains the bearer-order grep)
    - Location: tests/statusline-keychain-suffixed.bats (multi-install test, last assertions)

  - `OAUTH_TOKEN` is reassigned to `_candidate_token` on success but downstream code reads OAUTH_TOKEN. Cache stores the working token, but on next cache miss Priority 2 re-resolves and the lexically-first (stale) token is picked again as the new primary, forcing the try-all loop on EVERY cache miss until the stale install is removed. Worst-case = 6 candidates × 3s = 18s per render even after the fix succeeded.
    - Severity: minor (acceptable given 60s cache; worth a comment in the script acknowledging the recurring cost)
    - Confirmed
    - Location: scripts/vbw-statusline.sh ~870 (OAUTH_TOKEN="$_candidate_token") + cache lifecycle

  - `unset CRED_JSON` added only on Darwin branch (line 773), Linux branch leaves CRED_JSON set
    - Severity: minor
    - Confirmed
    - Location: scripts/vbw-statusline.sh:773 (Darwin) vs ~777-790 (Linux)

  - PR body not updated to mention round-1 try-all-on-401 addition
    - Severity: minor (cosmetic)
    - Confirmed
    - Location: PR #578 body

- Carryover findings (from round 1, still open):

  - Exit-code-aware messaging deferred — documented in commit message but NOT in inline script comment near lines 985-992. Should add a brief inline comment so future maintainers don't try to "fix" it without realizing it requires a cache schema bump.
    - Severity: minor
    - Confirmed
    - Location: scripts/vbw-statusline.sh:985-992

  - Mock dump-keychain output fidelity — round 1 fix added realistic framing in `_install_mock_security_multi` but the original `_install_mock_security` (used by back-compat and discovered tests) may still produce minimal output. Worth a quick check that both mock helpers were upgraded.
    - Severity: minor
    - Hypothetical
    - Location: tests/statusline-keychain-suffixed.bats (_install_mock_security)

  - Linux suffixed-name parity → tracked as #580. Confirmed deferred.

- Other findings:
  - Worst-case latency: 6 stale tokens × 3s curl timeout = 18s blocking the statusline render. Consider capping `_USAGE_CANDIDATES` length to e.g. 4 to avoid pathological hangs.
    - Severity: minor
    - Confirmed
    - Location: scripts/vbw-statusline.sh ~826-840 (_USAGE_CANDIDATES build)

  - `[[ ! "$l3" =~ "..." ]]` with quoted RHS in tests — bash 3.2 supports this, behavior consistent with bash 4+. No portability issue.
    - Severity: none (positive finding)

  - Candidate-list `_KEYCHAIN_FALLBACK_TOKENS[@]:-}` — script lacks `set -u`, so empty array expansion is safe across bash 3.2 and 5.x. No issue.
    - Severity: none (positive finding)

- Recommendation:
  - Not blocking-merge. The round 1 major fix (try-all-on-401) is correct and well-tested. The new findings are all minor:
    1. Verify the multi-install BATS test actually contains the bearer-order assertion the commit message claims (bearer recording via `.curl-bearer` and a grep for both tokens). If missing, add it.
    2. Either drop the `mapfile -t` from the new mock helper (use a portable while-read loop) OR accept the bash 4+ requirement and note it.
    3. Add a one-line inline comment near lines 985-992 documenting why the exit-44/51 distinction is collapsed (cache schema bump avoidance) so future maintainers don't try to "fix" it.
    4. Optional polish: cap candidate count, harmonize `unset CRED_JSON` across Darwin/Linux, update PR body to mention round-1 multi-install addition.
  - Items 1 and 2 deserve a quick round-3 fix; items 3-4 can be deferred or batched.

Maintainer notes on this round

• Running vbw:plan after a fresh brownfield init throws error. #1 (bearer-order assertion): verified present in the multi-install test — .curl-bearer-log is read and asserted as token-stale-primary then token-fresh-fallback. The hypothetical concern was unfounded.
• Hook validation error during compaction - missing hookEventName field #2 (mapfile): valid concern. Replaced with a portable while IFS= read -r loop in the round 2 fix commit.
• Task blocking: Dev agents don't auto-start when blockers clear #3 (inline collapsed-message note): added in the round 2 fix commit at scripts/vbw-statusline.sh:985-992.
• Comprehensive code review: 25+ improvements across scripts, hooks, agents, and architecture #4 (polish): all four items applied in the round 2 fix commit — candidate cap = 4, unset CRED_JSON hoisted to apply to both Darwin and Linux branches, PR body updated. Mock fidelity for the original _install_mock_security was already realistic (multi-line keychain: / class: / attributes: framing) — no change needed.

[halindrome]

QA Round 3

- Model used: Claude Opus 4.7 (1M context)
- Round: 3
- What was tested:
  - All three commits (f0c56e76, 03aa03da, e196d758) plus cumulative gh pr diff 578
  - Both prior QA reports (rounds 1 & 2)
  - scripts/vbw-statusline.sh: Priority 2 macOS block (lines ~736-791), candidate cap loop (~826-892), unset CRED_JSON placement (line 791), inline design note at FETCH_OK noauth+claude.ai branch (~1016-1026), all FETCH_OK message branches
  - tests/statusline-keychain-suffixed.bats: 4 tests + portable while-read mapfile replacement in _install_mock_security_multi, _install_mock_curl_401_then_ok bearer-order recording
  - PR body wording, commit message convention compliance
  - BATS file count math (3314 + 4 = 3318)

- New findings (introduced by round 2 fixes): none
  - Portable `while IFS= read -r _line; do arr+=("$_line"); done <<< "$str"` works on bash 3.2 through 5.x. The multi-install fixture only ever passes non-empty multi-line strings, so the well-known "empty `<<<` yields one empty element" quirk is not exercised; mock's `[ -z "$n" ] && continue` guards handle it anyway.
  - Candidate cap = 4 is defensible. Worst-case latency dropped from 18s to 12s. 4 stale + 1 fresh edge case (5 suffixed total) would silently miss the fresh token, but: (a) the comment near line ~830 explicitly flags the trade-off, (b) the env-var workaround (`VBW_OAUTH_TOKEN`) covers the pathological case, (c) the cache lifecycle means the user only pays the cost once per 60s. Minor by intent.
  - `unset CRED_JSON` hoist (line 791): Priority 3 (credentials.json, lines ~803-822) does NOT use `CRED_JSON` — it reads `.claudeAiOauth.accessToken` directly into `OAUTH_TOKEN`. No later consumer reads `CRED_JSON`. Hoist is correct and complete.
  - Inline design note (~lines 1016-1026): accurately states 15→16 fields with back-compat read; references PR #578; identifies cache schema bump as the deferred prerequisite. Self-consistent.

- Carryover findings (still open from earlier rounds): none — all confirmed addressed (mapfile, candidate cap, CRED_JSON symmetry, inline note, PR body, bearer-order assertion) or deferred by design (Linux suffixed-name parity tracked in #580; exit-44/51 distinction now documented inline; OAUTH_TOKEN re-resolves stale-primary every cache miss — accepted trade-off documented in code comment).

- Other findings: none
  - Commit message convention: `fix(statusline): address QA round 2` ✓ matches the fix-issue workflow's `fix({scope}): address QA round N` pattern.
  - BATS test count math: 4 new tests in new file; prior total + 4 = 3318. Sanity check ✓.
  - No other tests/statusline-*.bats interfere with the new behavior — they all use `VBW_SKIP_KEYCHAIN=1` or independent setup paths.
  - PR body now mentions both rounds 1 & 2 explicitly. Issue #576 body, CHANGELOG references, and docs/ entries are not in the diff and don't need updating for this fix.

- Recommendation: CLEAN — no critical, major, or minor findings remaining. The QA loop terminates here. PR #578 is mergeable from a QA perspective.

QA loop complete per CONTRIBUTING (3 rounds, latest clean).

[dpearson2699]

@halindrome you need to follow the contributing guide in addition to making sure all tests pass

[halindrome]

QA Review — Round 3 (fresh read-only review of the #576 fix against base 3ca99e60, after merging current main into the branch; clean for merge)

- Model used: Claude Opus 4.8 (read-only Code Reviewer agent)

- What was tested:
  • Full net diff vs base 3ca99e60: scripts/vbw-statusline.sh (+142/-39) and
    tests/statusline-keychain-suffixed.bats (new, 357 lines, 4 tests) — exactly two files.
  • Read the keychain-discovery block (746-778), candidate-build + 401-fallthrough fetch
    loop (838-898), AUTH_METHOD detection (819-826), slow-cache write (915), and the
    display elif chain (1009-1035).
  • Ran the suite: bats tests/statusline-keychain-suffixed.bats → 4/4 PASS (bats 1.13.0);
    full repo suite green (3595 BATS, 51/51 contract, lint).
  • Read the full mock harness (fake `security` find-generic-password + dump-keychain with
    exit 44 on miss; fake `curl` 200 / 401-then-ok with bearer capture; multi-install arrays).
  • Static: shellcheck -S warning clean; confirmed NO set -e/-u/pipefail in the script.

- Expected vs actual:
  1. Suffixed-name discovery — enumerate `Claude Code-credentials-<hex>` via non-prompting
     `security dump-keychain` (metadata only, no -d), pick first valid token. Matches intent;
     the `-[^"]+` match is anchored to the `Claude Code-credentials-` prefix so over-match
     risk is negligible (intentional per the realistic-shape dump mock).
  2. 401-fallthrough loop — primary + de-duped alternates capped at 4; 200+valid → pin working
     token & break; 401/403 → continue; 429 → ratelimited+break; else → fail+break. Correct:
     no off-by-one, no premature break, no stale-data leak (per-iteration resets). Test 3 proves
     ordered try via bearer log.
  3. Honest claude.ai message — the auth-expired branch is ordered BEFORE the claude.ai branch,
     so a 401'd real token yields "auth expired (run /login)", not a keychain blame; the
     claude.ai "OAuth token unavailable" branch only fires when no token was reachable. Test 4 confirms.
  4. Shell robustness — no set -e/-u/pipefail (empty pipes & unset array on Linux/literal paths
     are safe via ${arr[@]:-}); service names/tokens always double-quoted into security/curl
     (no injection); refresh-token-bearing CRED_JSON unset after extraction; no bearer echoed.
  5. Test fidelity — bearer-capture assertions prove the suffixed token flowed end-to-end and
     both tokens were tried in order (non-tautological); Bash-3.2-portable array reads match macOS.

- Severity: one minor/optional (finding 1 regex wording); findings 2-5 none.
- Confirmed vs hypothetical: findings 1-5 all CONFIRMED (source read + green 4/4 run + shellcheck).

ROUND VERDICT: CLEAN FOR MERGE — keychain enumeration, 401 fallthrough ordering, and the honest
claude.ai message are all correct; 4/4 tests pass with non-tautological end-to-end assertions;
shellcheck clean, no set -e/-u/pipefail footguns. Only one cosmetic regex nit (below).

Disposition of the optional nit: the reviewer suggested tightening -[^"]+ to -[0-9a-f]+ to match the <8hex> comment. Deliberately not applied — the permissive match is intentional, and tightening to lowercase-hex risks a real false-negative if a future Claude Code install suffix is not lowercase hex (no authoritative spec for the suffix format exists). The over-match risk is negligible because the match is anchored to the full Claude Code-credentials- prefix. Keeping discovery permissive is the safer, root-cause-aligned choice. Recorded as an empty round-3 commit since the round is clean.

[halindrome]

QA Review — Round 4 (triggered by Linux CI failure that the macOS-only round-3 review could not observe)

What CI revealed: after round 3, the Bats shard 2 job failed on the Linux CI runner — tests/statusline-keychain-suffixed.bats tests 2 (suffixed … discovered) and 3 (multi-install … falls through) failed at [ -f "$fake_bin/.curl-bearer" ] / .curl-bearer-log. Root cause: the #576 security-based keychain discovery is correctly gated on _OS=$(uname) == Darwin (security is macOS-only), but CI runs on Linux, so the branch was skipped and the mocked security/curl were never invoked. Rounds 1–3 ran on macOS, where the path executes and the tests pass — so the Linux-only gap was invisible to them.

Fix (test-only, tests/statusline-keychain-suffixed.bats): added _install_mock_uname_darwin, which writes a mock uname returning Darwin (for no-args/-s; other flags delegate to the real binary) into the per-test fake_bin, and invoked it from _install_mock_security and _install_mock_security_multi. This exercises the macOS keychain path on Linux runners — the same mocking strategy already used for security/curl. No production code changed: the OS gate is correct, and the script keeps its separate Linux secret-tool/pass branch.

Reproduction + verification (deterministic): with a uname→Linux shim the suffixed-token .curl-bearer marker is ABSENT (reproduces CI); with uname→Darwin it is PRESENT with the suffixed token. Full suite green on macOS (3595 BATS, 51/51 contract, lint).

- Model used: Claude Opus 4.8 (read-only Code Reviewer agent)

- What was tested:
  • git diff 3ca99e60...HEAD on tests/statusline-keychain-suffixed.bats (new helper + 2 invocations).
  • Production OS gate in scripts/vbw-statusline.sh: _OS=$(uname) (line 26); Darwin keychain block
    (security find-generic-password + dump-keychain svce-regex multi-token loop); the separate Linux
    secret-tool/pass branch; and the stat -f / stat -c _OS branches in cache_fresh / file_mtime_epoch.
  • Ran the suite on macOS: 4/4. Simulated Linux CI via a fake_bin `uname`→"Linux" shim and re-ran: 4/4.
  • Verified the delegate-to-real-uname fallback is non-recursive; checked the faked-Darwin stat -f concern.

- Expected vs actual:
  1. Round-4 helper makes the Darwin-gated keychain path run on Linux — CONFIRMED (simulated-Linux 4/4).
  2. Faking uname=Darwin must not break other _OS branches (stat -f in cache_fresh/file_mtime_epoch is
     macOS-only and errors on a real Linux fs) — benign: cache_fresh short-circuits with
     `[ ! -f "$cf" ] && return 1` before any stat, and on first run in a fresh repo the cache files do
     not exist; tests assert L3 + curl markers, not freshness. Simulated-Linux 4/4 proves no leak.
  3. Delegate fallback (`exec "$_d/uname"`) is correct and cannot loop into the mock — CONFIRMED: it execs
     an ABSOLUTE path, never bare uname; fake_bin is a mktemp dir never in /usr/bin|/bin|/usr/local/bin.
  4. Tests 1 (legacy literal) and 4 (claude.ai no-token) still test what they claim under faked Darwin —
     CONFIRMED on macOS and simulated Linux.
  5. Legitimate test-only fix, not a product-bug mask — CONFIRMED: `security` is macOS-only; the script
     already has a separate Linux branch; the product code should NOT run `security` on Linux.

- Severity: no findings (all checks pass). One non-blocking nit: the mock's final fallback echoes "Darwin"
  if no real uname is found in the three delegate dirs — harmless (real uname is always present) and
  intentional for the no-arg case.
- Confirmed vs hypothetical: findings 1-5 all CONFIRMED (simulated-Linux run, code inspection, live shim probe).

ROUND VERDICT: CLEAN FOR MERGE — test-only round-4 fix correctly unblocks Linux CI; the failure was
reproduced with a Linux uname shim and all 4 tests pass, the delegate fallback is non-recursive, and
faking Darwin does not break the stat-based cache path (it short-circuits before stat).

Addressed in this fix(statusline): address QA round 4 commit. The round-3 "clean" verdict stands for the code logic on macOS; round 4 closes the environmental (Linux CI) gap that a macOS-only review could not surface.

[dpearson2699]

Verdict: REQUEST_CHANGES

Blocking findings are inline where GitHub could anchor them.

Unanchored blockers:

• None.

Evidence:

• Linked issue: #576.
• Tests: bash testing/run-all.sh was attempted from the PR worktree and hung in tests/statusline-keychain-suffixed.bats; focused bash -n tests/statusline-keychain-suffixed.bats failed with line 256: syntax error near unexpected token '}'; focused bats --filter 'multi-install' tests/statusline-keychain-suffixed.bats hung inside _install_mock_security_multi and was interrupted.
• Blind baseline: completed; expected suffixed keychain discovery plus distinct rc 44 vs rc 51 messaging and tests for literal, suffixed, rc 44, and rc 51 paths.
• QA: completed; confirmed VBW-PR-001 and VBW-PR-002 as blocking.

Review marker: VBW-PR-578-inline-review-v2

[dpearson2699]

VBW-PR-001 (blocker): Issue #576 requires exit-code-aware keychain messaging: rc 44 should render the OAuth-token-unavailable message with /login guidance, while rc 51 should keep the existing keychain-access-denied wording. This branch always renders the generic unavailable message after explicitly collapsing rc 44 and rc 51 above, and the slow cache does not preserve keychain status. Carry the keychain result through cache/rendering instead of collapsing these cases.

[dpearson2699]

VBW-PR-002 (blocker): This generated-script heredoc is unquoted, so shell expansion runs while the test is building the fake security script. The backticked mapfile text below is treated as command substitution, which makes the focused multi-install BATS case hang before the fake script is written; bash -n tests/statusline-keychain-suffixed.bats also fails. Quote this heredoc or escape every expansion point so the full suite can complete.

[halindrome]

Addressed the review feedback in e937125b.

VBW-PR-001 — exit-code-aware keychain messaging. The claude.ai-login-but-no-token branch no longer collapses the two failure modes. scripts/vbw-statusline.sh now captures the security exit code at both the literal and suffixed lookups and threads a keychain-status field through the slow cache (16th field; back-compat read — older 15-field caches yield an empty status). Rendering branches on it:

• exit 51 (errSecAuthFailed) → keeps Limits: keychain access denied (allow Terminal in Keychain Access.app or set VBW_OAUTH_TOKEN)
• exit 44 (errSecItemNotFound) / non-Darwin → Limits: OAuth token unavailable (run /login or set VBW_OAUTH_TOKEN)

VBW-PR-002 — BATS hang. Root cause: the multi-install mock used an unquoted heredoc, so a backticked word in a comment ran as a command substitution at mock-build time — it read stdin and hung, leaving a zero-byte security mock (which is why the focused run hung and bash -n tripped). Reworded the comment backtick-free and documented the constraint; the new exit-51 mock uses a quoted heredoc to demonstrate the safe pattern.

Also added the missing exit-51 test alongside the existing exit-44 case.

bash testing/run-all.sh is green (lint 1/1, contracts 51/51, BATS 3596/0). The keychain file is 5/5, including the previously-hanging multi-install case. Rebased onto the latest branch tip before pushing.

[halindrome]

QA Round 5

Model used: Claude Opus 4.8 (read-only pr-qa-reviewer sub-agent)
Branch reviewed: fix/576-statusline-keychain-suffixed-service-name (HEAD after merge of upstream/main; fix commit e937125b)
Contract source: issue #576 + prior-review blockers VBW-PR-001 / VBW-PR-002

What was tested

1. Full read of scripts/vbw-statusline.sh keychain-read, slow-cache write/read, 401-retry, and usage-rendering blocks.
2. Pipe-field accounting: slow-cache write (line 924) vs read-back (line 931) — counted programmatically.
3. Ran tests/statusline-keychain-suffixed.bats (all 5 tests) + 4 adjacent statusline bats files as a regression check against the changed shared slow-cache format.
4. shellcheck -S warning on the script.
5. Edge probes: empty/unset _KEYCHAIN_FALLBACK_TOKENS, empty trailing _KEYCHAIN_STATUS field, legacy 15-field cache read.

Expected vs actual

• VBW-PR-001 (exit-code-aware messaging carried through cache): CONFIRMED FIXED. rc 51 sets _KEYCHAIN_STATUS=access_denied on both the literal (L750) and each suffixed (L770) lookup; rc 44 leaves it empty. Written as the 16th pipe field (L924), read back as the 16th var (L931) — field count 16 = 16, no off-by-one shift. Render (L1035–1039) gates on access_denied → "keychain access denied", else → "OAuth token unavailable (run /login or set VBW_OAUTH_TOKEN)" — wording matches statusline: Limits line wrongly shows "keychain access denied" when Claude Code stores OAuth under suffixed service names #576. Legacy 15-field caches read KEYCHAIN_STATUS="" → safe "OAuth token unavailable", no false "access denied", no field shift.
• VBW-PR-002 (multi-install mock hang): CONFIRMED FIXED. Fixture values injected via quoted inner heredocs (<<'NAMES_EOF' / <<'TOKENS_EOF') with a backtick-free comment; the multi-install test runs green, full file completes in seconds, no hang.
• statusline: Limits line wrongly shows "keychain access denied" when Claude Code stores OAuth under suffixed service names #576 core (suffixed discovery + back-compat + multi-install 401 fall-through): CONFIRMED. Literal tried first; suffixed discovery gated on empty token via non-prompting dump-keychain (metadata only); candidate list primary-first, capped at 4, 401/403 retries next, 429/5xx break early. bats tests 1–3 pass.
• Regression (shared slow-cache format): CONFIRMED clean. 4 adjacent statusline bats files all pass; shellcheck -S warning: 0 findings.

Findings

• OBS-1 (minor / non-blocking, confirmed): for _alt_token in "${_KEYCHAIN_FALLBACK_TOKENS[@]:-}" yields one empty-string iteration when the array is empty (e.g. Linux path), immediately skipped by [ -z "$_alt_token" ] && continue. Cosmetic; script does not use set -u, so no unbound-variable risk.
• OBS-2 (informational, not a defect): bash -n reports a syntax error on the @test ... { DSL — expected for all bats files; the file runs correctly under bats.

Verdict

PASS (clean — only minor/cosmetic observations). Both prior-review blockers (VBW-PR-001 and VBW-PR-002) independently confirmed fully fixed; all five required test cases exist and pass; slow-cache field ordering correct (16=16); legacy caches degrade safely; no regression in adjacent statusline tests; shellcheck clean.

Branch was also re-synced with upstream/main (merge e52a7bb8) so the PR is no longer behind. Full local bash testing/run-all.sh: lint 1/1, contract 53/53, BATS 3595 passed / 1 failed — the single failure (qa-remediation-state.bats, unrelated subsystem) is a parallel-load flake that passes 3/3 in isolation; not introduced by this PR.

---

QA performed by Claude Code (claude-opus-4-8)

[halindrome]

QA Round 6

Model used: Claude Opus 4.8 (read-only pr-qa-reviewer sub-agent)
Scope: new commit 21f82b1f — "order multi-install keychain tokens by expiresAt (newest-first)", layered on the prior-confirmed #576 fix.

Why this change

The suffixed-name fallback (#576) collected tokens in service-name (lexical) order and used index [0] as primary. A machine with multiple Claude Code installs (a stale token plus the live one) could try a stale token first on every render — a guaranteed 401 before falling through to the live token, doubling usage-API calls and adding rate-limit pressure. Now candidates are ordered by stored OAuth expiresAt (ms epoch) descending, so the freshest install is tried first; tokens with no/invalid expiresAt sort as 0 (last). The 401-fallthrough retry path is unchanged.

What was tested (on macOS bash 3.2.57 — production target)

1. Full tests/statusline-keychain-suffixed.bats: 6/6 PASS (incl. new ordering test + reworded fallthrough test).
2. shellcheck -S warning scripts/vbw-statusline.sh: clean (exit 0).
3. Empirical replication of the exact sort -t<TAB> -k1,1nr pipeline on bash 3.2.57.
4. Read of the untouched retry loop, rc 44 / rc 51 messaging, legacy literal path, and slow-cache 16-field layout.
5. git diff --name-only: only the two expected files changed.
6. Full bash testing/run-all.sh: Lint 1/1, Contract 53/53, BATS 3597 passed / 0 failed, EXIT=0.

Expected vs actual (devil's-advocate)

• Sort correctness: CONFIRMED numeric-reverse on field 1 (100 > 99 > 9), locale-immune (-n on ASCII digits; LC_ALL=C and en_US identical). Token field can't corrupt the key — only field 1 (pre-sanitized to [0-9]+) drives the sort.
• Absent/null/non-numeric/negative expiresAt: CONFIRMED → forced to 0 via jq // 0 + case ''|*[!0-9]*) _exp=0 (leading - strips to 0). Sorts last.
• bash 3.2 compatibility: CONFIRMED — +=, $'\t'/$'\n', <<<, case globs all behave on 3.2.57.
• Trailing-newline phantom token: CONFIRMED guarded — rebuild loop has [ -z "$_kt" ] && continue; empirical 2-pair input → count 2, no phantom.
• Order honored downstream: CONFIRMED — OAUTH_TOKEN=[0] (freshest) and the curl candidate list derives from the same reordered array; freshest always tried first.
• Regression (rc 44/51, legacy literal, field-count-16): CONFIRMED untouched; bats Running vbw:plan after a fresh brownfield init throws error. #1/Commands fail with exit 127 when CLAUDE_PLUGIN_ROOT is unset (user-command precedence bug) #5/compile-context.sh fails to find phase directory when phase number is not zero-padded #6 pass.
• Test quality: CONFIRMED a genuine discriminator — the new test's lexically-last service name holds the highest-expiresAt token; old lexical [0] would pick the near-expiry token, so the assertion fails under the old scheme and passes only with expiry ordering. Mock emits a bare-numeric expiresAt → valid JSON.

Findings

Contract: none. Regression: none. Schema change: none. critical 0 / major 0 / minor 0.

Verdict

PASS — no blocking items. Commit 21f82b1f is correct and complete; verified empirically on the production bash 3.2.57 target; no regression to the prior-confirmed core #576 fix or to the earlier blockers (VBW-PR-001 messaging, VBW-PR-002 mock hang).

Full local suite green (EXIT=0). Branch is current with upstream/main.

---

QA performed by Claude Code (claude-opus-4-8)