test(oauth): cover static client-information + client-secret resolver

[KrasimirKralev]

What Problem This Solves

src/oauth-client-info.ts exports the two helpers that assemble a static OAuth client information object and resolve its client secret, and the module shipped with no direct test coverage — no tests/oauth-client-info.test.ts exists on main. Both helpers carry contracts a plausible refactor could silently break:

• resolveOAuthClientSecret(definition, options) — when oauthClientSecretEnv is set it reads the env var and throws a labeled error if the value is missing (!value) or, under rejectBlank, blank; otherwise it returns the env value (whitespace preserved when rejectBlank is off). With no env ref it falls back to the inline oauthClientSecret, or undefined.
• buildStaticClientInformation(definition, options) — returns undefined with no oauthClientId; otherwise emits fixed grant_types/response_types, conditionally includes client_secret (via the resolver), redirect_uris (from a URL or string), and token_endpoint_auth_method, and propagates the resolver's throw.

A regression in any of these — dropping the !value guard, inverting a conditional-spread ternary, or losing the required-secret throw — would ship green today because nothing executes the module.

Why This Change Was Made

Coverage-only. This adds one new file, tests/oauth-client-info.test.ts (+112, no production code touched), pinning both helpers' current main behavior. The tests import the real exported functions and drive them directly — no stubs — so they exercise the production path callers actually hit. Both the primary and negative/omit paths are pinned for each helper: the resolver's env/inline/undefined returns plus its three throw paths, and the builder's undefined gate, each conditional field's include-and-omit pair, and the propagated throw. No new config, defaults, or dependencies.

User Impact

No user-visible or runtime change. For maintainers, the static-client-information contract now regresses loudly instead of silently: a future edit that drops the required-secret throw, breaks a conditional-field spread, or loses the env/inline resolver fallback will fail this suite.

Evidence

Linux, Node 22.22, pnpm install --frozen-lockfile from source. Branched off current main (base 4219927); the module under test, src/oauth-client-info.ts, is byte-identical to its state at the earlier-verified base 58986a7 (the only change between them is CHANGELOG.md), so the pinned behavior is current.

15/15 pass on current main:

$ ./node_modules/.bin/vitest run tests/oauth-client-info.test.ts
 RUN  v4.1.10 /home/user/mcporter
 Test Files  1 passed (1)
      Tests  15 passed (15)

Non-vacuous — the suite bites when the target is mutated (each mutation applied to src/oauth-client-info.ts, suite re-run, then reverted byte-identical). The representative M1 case was re-confirmed on this exact rebased head; the full matrix was validated against the byte-identical source:

Mutation to oauth-client-info.ts	Failing tests
drop the !value required-secret guard	3
drop the rejectBlank blank-value clause	1
env branch returns inline secret instead	3
drop the no-oauthClientId undefined gate	1
invert the client_secret conditional spread	2
invert the redirect_uris conditional spread	2
invert the token_endpoint_auth_method spread	1
break the fixed grant_types array	1
(reverted — control)	0 (15 pass)

$ # M1 re-confirmed on rebased head: drop `!value` guard
$ ./node_modules/.bin/vitest run tests/oauth-client-info.test.ts   # → 3 failed | 12 passed (15)
$ # source reverted byte-identical
$ ./node_modules/.bin/vitest run tests/oauth-client-info.test.ts   # → 15 passed (15)

Format / lint / types clean on the new file:

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

Scope note: one new *.test.ts file under tests/, +112 / -0, no production code touched. Direct sibling of the just-landed #246 (OAuth token-generation coverage) — same module family, mirror side.

_{Opened from a 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]

Codex review: needs changes before merge. Reviewed August 6, 2026, 9:17 AM ET / 13:17 UTC.

ClawSweeper review

What this changes

The PR adds 112 lines of direct Vitest coverage for static OAuth client metadata construction and client-secret resolution.

Merge readiness

⚠️ Needs maintainer review before merge - 3 items remain

Keep open: the focused coverage is useful, but the unchanged test file still deletes any caller-provided secret environment value and does not explicitly establish the unset precondition.

Priority: P3
Reviewed head: 44e9275a34d51e706c6461b1d8fa15eb07552c56

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The coverage and validation evidence are strong, with one small but concrete test-isolation repair still needed.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The PR body supplies focused after-change Vitest output and mutation results demonstrating that the new assertions detect regressions.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR body supplies focused after-change Vitest output and mutation results demonstrating that the new assertions detect regressions.
Evidence reviewed	5 items	Unfixed environment isolation: The added test suite deletes the fixed environment key after every test, but never snapshots and restores a pre-existing caller value or unsets it before cases that require absence. Current production contract: The production resolver reads its configured environment variable and throws only when the value is absent or invalid, so isolation is necessary for tests asserting the absent-secret path. Existing repository test convention: An existing environment-sensitive test snapshots the prior value, explicitly deletes it for the missing-value case, and restores it in cleanup.
Findings	1 actionable finding	[P3] Preserve the inherited test environment value
Security	None	None.

How this fits together

MCPorter builds static OAuth client metadata from a configured server definition and resolves secrets from environment variables or inline configuration. The resulting metadata is consumed by OAuth authorization and token-refresh flows.

flowchart LR
  A[Server definition] --> B[OAuth client metadata builder]
  C[Environment secret] --> B
  B --> D[Static client information]
  D --> E[Authorization flow]
  D --> F[Token refresh flow]

Loading

Before merge

• Preserve the inherited test environment value (P3) - Snapshot and restore this key, and explicitly unset it before each case. As previously reported, deleting it unconditionally can erase a caller-provided value and lets the unset-secret assertions depend on ambient process state.
• Resolve merge risk (P1) - Merging unchanged lets this suite erase a caller-provided MCPORTER_TEST_CLIENT_INFO_SECRET value and can make absent-secret assertions depend on ambient process state.
• Complete next step (P2) - A narrow mechanical test-isolation repair is clear and suitable for an automated follow-up.

Findings

• [P3] Preserve the inherited test environment value — tests/oauth-client-info.test.ts:12-14

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Test-only delta	production +0, tests +112	The single added file is focused coverage; no OAuth runtime or dependency surface changes.

Merge-risk options

Maintainer options:

1. Make the test environment hermetic (recommended)
   Snapshot the secret before each case, explicitly clear it for test setup, and restore the original value after each case.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Snapshot MCPORTER_TEST_CLIENT_INFO_SECRET, unset it before every test, restore the original value afterward, and run the focused Vitest file plus pnpm check and pnpm test.

Technical review

Best possible solution:

Keep the direct coverage, but snapshot the environment value, clear it before each test, and restore it afterward while leaving production OAuth behavior unchanged.

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

Yes, from source: start the focused suite with MCPORTER_TEST_CLIENT_INFO_SECRET already set; the current cleanup deletes it, and absent-secret cases lack an explicit unset setup.

Is this the best way to solve the issue?

No. Direct coverage is the right approach, but hermetic environment setup and restoration are needed for a reliable regression suite.

Full review comments:

• [P3] Preserve the inherited test environment value — tests/oauth-client-info.test.ts:12-14
  Snapshot and restore this key, and explicitly unset it before each case. As previously reported, deleting it unconditionally can erase a caller-provided value and lets the unset-secret assertions depend on ambient process state.
  Confidence: 0.99

Overall correctness: patch is correct
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

• P3: This is a narrow test-isolation repair with no user-visible runtime behavior change.
• merge-risk: 🚨 automation: The added cleanup can mutate inherited process state and make later test execution depend on ordering or ambient environment.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies focused after-change Vitest output and mutation results demonstrating that the new assertions detect regressions.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies focused after-change Vitest output and mutation results demonstrating that the new assertions detect regressions.

Evidence

Acceptance criteria:

• [P1] pnpm exec vitest run tests/oauth-client-info.test.ts.
• [P1] pnpm check.
• [P1] pnpm test.

What I checked:

• Unfixed environment isolation: The added test suite deletes the fixed environment key after every test, but never snapshots and restores a pre-existing caller value or unsets it before cases that require absence. (tests/oauth-client-info.test.ts:12, 44e9275a34d5)
• Current production contract: The production resolver reads its configured environment variable and throws only when the value is absent or invalid, so isolation is necessary for tests asserting the absent-secret path. (src/oauth-client-info.ts:24, 4219927ec680)
• Existing repository test convention: An existing environment-sensitive test snapshots the prior value, explicitly deletes it for the missing-value case, and restores it in cleanup. (tests/runtime-header-utils.test.ts:8, 4219927ec680)
• Review continuity: The current PR head is the same 44e9275 revision reviewed previously, so the prior environment-isolation finding remains unresolved rather than being a new late finding. (tests/oauth-client-info.test.ts:12, 44e9275a34d5)
• Feature provenance: Current main introduced the OAuth client-information helper in this commit; it remains the sole history entry for the file and is the relevant ownership trail. (src/oauth-client-info.ts:4, 49dcd3e7fffd)

Likely related people:

• Peter Steinberger: Introduced the current OAuth client-information helper on main; its metadata and secret-resolution contract are what this test covers. (role: feature introducer; confidence: high; commits: 49dcd3e7fffd; files: src/oauth-client-info.ts)

Rank-up moves

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

• Snapshot, clear, and restore the test environment value before rerunning the focused suite.

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-06T10:30:33.427Z sha 44e9275 :: needs changes before merge. :: [P3] Restore the inherited test secret after each test
• reviewed 2026-08-06T11:39:31.781Z sha 44e9275 :: needs changes before merge. :: [P3] Isolate and restore the test environment variable