test(oauth): cover stored-credential validation guards

[KrasimirKralev]

What Problem This Solves

src/oauth-credential-validation.ts exports the two type-guards that decide whether a stored OAuth credential read back from disk is trusted or dropped — isStoredOAuthTokens and isStoredOAuthClientInformation — and the module shipped with no direct test coverage (no tests/oauth-credential-validation.test.ts on main). Both gate real persistence decisions:

• oauth-persistence-stores.ts:146 / :198 — readTokens() / readClientInfo() return the stored value only if the guard passes, otherwise undefined (drop-and-re-auth).
• oauth-vault.ts:247 / :248 — a snapshot persists tokens / clientInfo with a hidden generation marker only if the guard passes.

The guards carry a subtle contract a plausible refactor could silently break:

• isStoredOAuthTokens requires a non-empty access_token string and a non-empty token_type string; refresh_token/scope/issuer must each be absent-or-string; expires_in/expires_at/expiresAt must each be absent-or-a-finite-number; a non-record (null, array, primitive) is rejected.
• isStoredOAuthClientInformation rejects a non-record and rejects a present-but-non-string issuer.

Dropping the empty-string check, the Number.isFinite guard, the optional-type checks, or the array/record discrimination would let a malformed credential through (or reject a valid one) with nothing to catch it.

Why This Change Was Made

Coverage-only. Adds one new file, tests/oauth-credential-validation.test.ts (+79, no production code touched), pinning both guards' current main behavior. The tests import the real exported functions and drive them directly — no stubs. No new config, defaults, dependencies, or behavior.

Not a duplicate of the existing boundary suite (scope note). tests/oauth-persistence-stores.test.ts reaches these guards, but only on the accept arm — it stores valid tokens and asserts they round-trip (:37, :59, :99), and its "corrupt credential JSON" case (:71) writes '{bad', which fails JSON.parse in the reader before the guard runs, so it never exercises the guards' rejection logic. Every rejection branch (empty required string, wrong optional type, non-finite number, non-record, non-string issuer) is unreached by any test at any level. This suite adds exactly those provably-skipped arms — see the mutation evidence below, where the existing boundary suite stays green under guard mutations that this suite catches.

User Impact

No user-visible or runtime change. For maintainers, the stored-credential validation contract now regresses loudly instead of silently: a future edit that breaks the empty-token gate, an optional-field type check, the finite-number check, or the record/array discrimination will fail this suite.

Evidence

Linux, Node 22.22, pnpm install --frozen-lockfile from source, branched off current main (ae3d900).

10/10 pass on clean main:

$ ./node_modules/.bin/vitest run tests/oauth-credential-validation.test.ts
 Test Files  1 passed (1)
      Tests  10 passed (10)

Non-vacuous — the suite bites when the target is mutated (each mutation applied to oauth-credential-validation.ts, suite re-run, then reverted byte-identical):

Mutation to oauth-credential-validation.ts	Result
access_token.length > 0 → >= 0 (accept empty)	1 fail
token_type.length > 0 → >= 0 (accept empty)	1 fail
isRecord drops !Array.isArray(value)	1 fail
isOptionalString drops the === undefined arm (require string)	2 fail
isOptionalFiniteNumber drops Number.isFinite	1 fail
isStoredOAuthClientInformation returns true after the record check	1 fail
(reverted — control)	10 pass

These regressions are invisible to the existing suite. Running the current caller/boundary tests (oauth-persistence-stores, oauth-persistence, vault-command, oauth-session) under the first, fifth, and third mutations above leaves their pass/fail count unchanged — the boundary suite never feeds a parseable-but-invalid credential to the guard, so it cannot catch these. This suite is the only coverage of the rejection contract.

Format / lint / types clean on the new file:

$ ./node_modules/.bin/oxfmt --check tests/oauth-credential-validation.test.ts   # All matched files use the correct format.
$ ./node_modules/.bin/oxlint --type-aware --tsconfig tsconfig.json --deny-warnings tests/oauth-credential-validation.test.ts   # exit 0
$ ./node_modules/.bin/tsc --project tsconfig.json --noEmit   # exit 0

Scope note: one new *.test.ts under tests/, +79 / -0, no production code touched. Same module family as the merged OAuth-helper coverage #246 (token generation) and #281 (client info) — this is the stored-credential validation sibling.

AI-assisted contribution.

_{Opened from an org-owned fork via the API; if GitHub's Allow edits by maintainers toggle isn't honored on this PR, a maintainer can still push to the branch or supersede-and-land.}

---

Generated by Claude Code

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 24, 2026, 7:02 AM ET / 11:02 UTC.

ClawSweeper review

What this changes

The PR adds direct Vitest coverage for the guards that accept or reject stored OAuth token and client-information records.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 3 items remain

The patch is a focused, correct coverage addition, but it still lacks real persistence-path proof; keep it open for that contributor evidence.

Priority: P3
Reviewed head: 93d52128b21606468d302155e568f3775c2d2051

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused test suite appears sound, but its current evidence is unit-test-only rather than real persistence-path proof.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The PR body shows direct Vitest and mutation-test output, but no after-fix run through the persistence reader; add a redacted terminal trace that parseable malformed credential JSON is dropped before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The PR body shows direct Vitest and mutation-test output, but no after-fix run through the persistence reader; add a redacted terminal trace that parseable malformed credential JSON is dropped before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	5 items	Focused diff: The branch adds one 79-line test file and does not change production code or dependencies. Current validation contract: Current main requires non-empty token fields, rejects invalid optional values and non-records, and validates an optional client-information issuer. Persistence boundary: The directory-backed reader returns undefined when either validation guard rejects stored JSON, preventing malformed credentials from reaching session recovery.
Findings	None	None.
Security	None	None.

Live Verification

Command: pnpm exec vitest run tests/oauth-credential-validation.test.ts

Result: PASS (completed)

pnpm exec vitest run tests/oauth-credential-validation.test.ts
runner@runnervm76f27:/tmp/clawsweeper-live-proof-331-wBfqMd/target$ pnpm exec vitest run tests/oauth-credential-validation.test.ts

 RUN  v4.1.10 /tmp/clawsweeper-live-proof-331-wBfqMd/target

 ✓ tests/oauth-credential-validation.test.ts (10 tests) 6ms

 Test Files  1 passed (1)
      Tests  10 passed (10)
   Start at  11:02:28
   Duration  183ms (transform 32ms, setup 29ms, import 19ms, tests 6ms, environment 0ms)

runner@runnervm76f27:/tmp/clawsweeper-live-proof-331-wBfqMd/target$ pnpm exec vitest run tests/oauth-credential-validation.test.ts

 RUN  v4.1.10 /tmp/clawsweeper-live-proof-331-wBfqMd/target

 ✓ tests/oauth-credential-validation.test.ts (10 tests) 6ms

 Test Files  1 passed (1)
      Tests  10 passed (10)
   Start at  11:02:29
   Duration  181ms (transform 32ms, setup 29ms, import 19ms, tests 6ms, environment 0ms)

runner@runnervm76f27:/tmp/clawsweeper-live-proof-331-wBfqMd/target$

Assertions:

• PASS expect_output: Test Files 1 passed
• PASS expect_output: Tests 10 passed

How this fits together

MCPorter’s OAuth persistence layer reads token and client JSON from disk, validates stored shapes, and either recovers a session or drops invalid data so authentication can restart. This PR tests that validation boundary directly.

flowchart LR
  A[Credential JSON on disk] --> B[OAuth persistence reader]
  B --> C[Stored credential validation]
  C -->|valid| D[Recovered OAuth session]
  C -->|invalid| E[Drop credential]
  E --> F[Re-authentication]
  G[New direct tests] --> C

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The PR body shows direct Vitest and mutation-test output, but no after-fix run through the persistence reader; add a redacted terminal trace that parseable malformed credential JSON is dropped before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve merge risk (P1) - Unit coverage alone does not demonstrate that parseable malformed credential files are dropped through the directory persistence reader.
• Complete next step (P2) - The contributor needs real persistence-path evidence; no code repair or maintainer product choice is currently required.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Test-only scope	production +0, tests +79; 1 file added	The patch adds focused coverage without changing runtime behavior or package inputs.

Merge-risk options

Maintainer options:

1. Decide the mitigation before merge
   Add a redacted terminal trace that writes parseable malformed token and client JSON through the directory persistence reader and shows both are dropped, then land the focused test-only patch.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Add a redacted terminal trace that writes parseable malformed token and client JSON through the directory persistence reader and shows both are dropped, then land the focused test-only patch.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this is a coverage-only PR, and its acceptance target is the existing stored-credential validation contract rather than a reported runtime failure.

Is this the best way to solve the issue?

Yes: a direct regression suite is the narrowest way to pin the shared validation rules, provided the claimed persistence effect is also shown with a real reader trace.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ae3d9000c320.

Labels

Label justifications:

• P3: This is a low-risk test-coverage improvement with no user-visible runtime change.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body shows direct Vitest and mutation-test output, but no after-fix run through the persistence reader; add a redacted terminal trace that parseable malformed credential JSON is dropped before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Focused diff: The branch adds one 79-line test file and does not change production code or dependencies. (tests/oauth-credential-validation.test.ts:1, 93d52128b216)
• Current validation contract: Current main requires non-empty token fields, rejects invalid optional values and non-records, and validates an optional client-information issuer. (src/oauth-credential-validation.ts:17, ae3d9000c320)
• Persistence boundary: The directory-backed reader returns undefined when either validation guard rejects stored JSON, preventing malformed credentials from reaching session recovery. (src/oauth-persistence-stores.ts:144, ae3d9000c320)
• Behavior provenance: History identifies the malformed stored-credential validation change as commit dac92c3; it is contained by v0.13.0, and v0.13.6 and current main have the same validation-file blob. (src/oauth-credential-validation.ts:17, dac92c30c876)
• Proof gap: The PR body and previous review provide direct unit and mutation-test output only; neither shows a parseable malformed on-disk credential being dropped by the production persistence reader.

Likely related people:

• Peter Steinberger: Feature-history search attributes the guard introduction to the malformed-credential fix commit. (role: introduced stored-credential validation behavior; confidence: high; commits: dac92c30c876; files: src/oauth-credential-validation.ts, src/oauth-persistence-stores.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Post a redacted terminal trace showing DirectoryPersistence drops parseable malformed token and client JSON, then update the PR body; a fresh review should run automatically, or a maintainer can request one with @clawsweeper re-review.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

• reviewed 2026-08-21T08:33:05.521Z sha 93d5212 :: needs maintainer review before merge. :: none
• reviewed 2026-08-23T19:55:37.190Z sha 93d5212 :: needs real behavior proof before merge. :: none