[jatmn]

Summary

• TA-001 slice 5 (Phase 0 plan item 6): attribution isolation for sideQuery.attribution and authRouting.attribution. Test files only.
• Root cause: fastMode.test.ts stubs utils/auth (API key test-key, no OAuth, not a subscriber), and its cleanup re-registers the live namespace, which Bun has already redirected to the mock. A later betas.test.ts re-registers the real provider functions, so the provider-only check in providerModuleIsolation.ts saw nothing wrong and the attribution suites ran in-process against mocked auth. That produced the 5 sideQuery failures (OAuth attribution off, test-key sent instead of the suite's key) and 2 authRouting failures when run after those two files.
• Fix:
  • src/test/providerModuleIsolation.ts: adds an attribution detector. It compares the canonical and cache-busted export surfaces of providers, auth, betas, authRouting, routeMetadata and the API client. It counts missing exports and changed function source, and treats anything it can't compare as needing a child. Inside the child it skips detection, so it can't recurse. The existing API, child launcher and deadline are unchanged.
  • Both attribution suites now choose either the child wrapper or the real suite, instead of keeping describe.skip copies. When isolation is needed, the parent registers only the wrapper, and the suites' setup and hooks stay inside the real-suite branch. All 8 sideQuery and 13 authRouting cases and their assertions are unchanged.
  • New src/test/providerModuleIsolation.test.ts with fixture src/test/fixtures/providerModuleIsolation.fixture.ts. These are regression tests for clean exports, polluted auth, beta-only and routing-only replacement, missing exports, recursion prevention and child failure propagation. Deliberate pollution runs only in disposable subprocesses.

Impact

• user-facing impact: none (test-only).
• developer/maintainer impact: the attribution suites stay correct in single-process runs even when earlier files leave auth, beta or routing mocks behind. The polluters themselves are not touched here. CI keeps --isolate, and no CONTRIBUTING change is needed because this extends the existing child-isolation pattern.

Testing

• I ran the required local preflight.
• exact commands and results (base 3a646df, after bun run build, temp config dirs):
  • single-process whole-repo bun test on main: before 20 fail, after 17 fail. All 5 sideQuery failures are gone; remaining failures belong to other TA-001 slices, plus the 2 gitSettings tests below.
  • same with open test: restore leaked provider, betas, sessionTitle and discovery module mocks (TA-001 slice 2) #2332 applied locally (measurement only, not stacked): before 11 fail, after 8 fail.
  • skip count 44 → 31, because the describe.skip duplicate copies are gone.
  • bun run typecheck: pass.
  • bun run check: every step passes except 2 gitSettings.test.ts tests (shard 1).
• focused tests: attribution suites alone, after fastMode + betas, after useApiKeyVerification, and providerModuleIsolation.test.ts.
• verified pre-existing failures: gitSettings.test.ts "omits git instructions outside a git repository" and "follows the session cwd" fail on unmodified main in the same worker environment, including when the file runs alone. That makes them environmental and unrelated to this diff.

Notes

• provider/model path tested: Anthropic first-party (OAuth, API key, managed Desktop/remote/Unix-socket sessions).
• follow-up work or known limitations: fastMode.test.ts and useApiKeyVerification.test.tsx still re-register a live auth namespace on cleanup. Restoring those is a separate polluter slice.

Summary by CodeRabbit

• Tests
  • Improved test isolation for authentication, provider, routing, and query-attribution scenarios.
  • Added coverage for provider-module differences, import failures, and child-process test failures.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Twigpine/openclaude
• Branch: test/ta001-slice5-attribution-isolation
• File: .coderabbit.yaml
• Commit: f81285a
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Essentials
• Run ID: cc395f91-d8aa-46f7-86f5-cf3749e1e6a5

📥 Commits

Reviewing files that changed from the base of the PR and between 3a646df and f81285a.

📒 Files selected for processing: 5

• src/services/api/authRouting.attribution.test.ts
• src/test/fixtures/providerModuleIsolation.fixture.ts
• src/test/providerModuleIsolation.test.ts
• src/test/providerModuleIsolation.ts
• src/utils/sideQuery.attribution.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout: 18

• GitHub Check: unit-tests (2)
• GitHub Check: unit-tests (5)
• GitHub Check: unit-tests (6)
• GitHub Check: launcher-node-floor
• GitHub Check: unit-tests (4)
• GitHub Check: unit-tests (7)
• GitHub Check: launcher-compatibility (24.11.x)
• GitHub Check: unit-tests (8)
• GitHub Check: launcher-compatibility (22)
• GitHub Check: dedicated-tests
• GitHub Check: unit-tests (3)
• GitHub Check: smoke-and-tests
• GitHub Check: unit-tests (1)
• GitHub Check: typecheck
• GitHub Check: CodeQL (javascript-typescript)
• GitHub Check: CodeQL (actions)
• GitHub Check: pr-intent-scan
• GitHub Check: Analyze (javascript-typescript)

🧰 Additional context used

📓 Path-based instructions: 3

Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny.

⚙️ CodeRabbit configuration file

Files:

• src/services/api/authRouting.attribution.test.ts

Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions.

⚙️ CodeRabbit configuration file

Files:

• src/test/providerModuleIsolation.test.ts
• src/services/api/authRouting.attribution.test.ts
• src/utils/sideQuery.attribution.test.ts

Apply the OpenClaude maintainer review rubric from AGENTS.md.

⚙️ CodeRabbit configuration file

Files:

• src/test/providerModuleIsolation.test.ts
• src/services/api/authRouting.attribution.test.ts
• src/test/providerModuleIsolation.ts
• src/utils/sideQuery.attribution.test.ts
• src/test/fixtures/providerModuleIsolation.fixture.ts

🪛 ast-grep (0.45.3)

src/services/api/authRouting.attribution.test.ts

[error] 55-61: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of routeEnvKeys) {
if (originalEnv[key] === undefined) {
delete process.env[key]
} else {
process.env[key] = originalEnv[key]
}
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').

(prototype-pollution-recursive-merge-typescript)

src/utils/sideQuery.attribution.test.ts

[error] 191-197: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of envKeys) {
if (originalEnv[key] === undefined) {
delete process.env[key]
} else {
process.env[key] = originalEnv[key]
}
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').

(prototype-pollution-recursive-merge-typescript)

🔇 Additional comments: 5

src/test/providerModuleIsolation.ts: 1

35-60: LGTM!

Also applies to: 62-89

src/test/fixtures/providerModuleIsolation.fixture.ts: 1

1-107: LGTM!

src/test/providerModuleIsolation.test.ts: 1

1-107: LGTM!

src/services/api/authRouting.attribution.test.ts: 1

8-24: LGTM!

Also applies to: 26-65, 67-202

src/utils/sideQuery.attribution.test.ts: 1

8-29: LGTM!

Also applies to: 31-54, 55-222, 224-353

📝 Walkthrough

The change adds provider-module export comparison and disposable-process tests for isolation behavior. The auth-routing and side-query attribution suites now choose between direct execution and a real-provider child process.

Changes: Attribution Test Isolation

Layer / File(s)	Summary
Provider export comparison src/test/providerModuleIsolation.ts	Compares module export surfaces and selects isolation when exports differ or inspection fails. The marked child process skips the comparison.
Isolation fixture scenarios src/test/fixtures/providerModuleIsolation.fixture.ts	Adds fixture scenarios for suite imports, provider mocks, unsupported exports, import failures, and the child-process marker. The fixture checks the detector result.
Disposable process verification src/test/providerModuleIsolation.test.ts	Runs fixture scenarios in disposable Bun processes and checks output and exit status. A separate test checks child assertion-failure reporting.
Attribution suite execution src/services/api/authRouting.attribution.test.ts, src/utils/sideQuery.attribution.test.ts	Both suites select isolation before importing their subjects. They run in a real-provider child process when needed, or load dependencies and run tests directly.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Other

Suggested reviewers: chioarub

Merge Risk: ⚪ Minimal · up to f8128

No issue identified here needs to be fixed before merging. Complete the normal checks before merge.

Pre-merge checks: 6 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 6 passed

Check name	Status	Explanation
Title check		The title is concise, scoped, and accurately describes the attribution-suite isolation changes in the diff.
Description check		The description includes all required sections and provides the change rationale, impact, testing commands and results, focused tests, known failures, provider path, and follow-up work.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Risk Surface Disclosed		The PR clearly discloses the risk surface. The description identifies leaked auth, beta, and routing mocks as the cause and explains the isolation change. It states that the impact is test-only with n…
No Hidden Policy Change		PASS. The PR changes only attribution test files and test-isolation utilities. The diff has no production-code paths or runtime references to the new detector. Existing attribution test names and poli…

Full details: Risk Surface Disclosed

Explanation

The PR clearly discloses the risk surface. The description identifies leaked auth, beta, and routing mocks as the cause and explains the isolation change. It states that the impact is test-only with no user-facing impact. The diff confirms that only attribution tests and test isolation helpers changed, so no production blocker is introduced.

Full details: No Hidden Policy Change

Explanation

PASS. The PR changes only attribution test files and test-isolation utilities. The diff has no production-code paths or runtime references to the new detector. Existing attribution test names and policy assertions remain; the PR moves them behind child-process selection and adds disposable mock fixtures. No product, trust-model, routing-default, telemetry/network, or permission-policy decision is introduced.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[jatmn]

— jatmn's PR Review agent

Merge readiness

Pinned head f81285a0c7b720685d4f07c11935a153dfe46f16 is mergeable against main with merge-base 3a646df82182c5d63bff2587e2df070aed7209ad (live-base-three-dot, not behind default). Required CI on that commit is green (typecheck, smoke-and-tests, dedicated-tests, unit shards, CodeQL, pr-intent-scan). The diff is test-only (TA-001 slice 5), does not depend on stacking #2332, and matches the stated auth/beta/routing isolation scope.

🟢 Thanks for the contribution. I do not see any actionable issues from my review.