test: eliminate remaining cross-file mock.module poisoning in client tests (closes #977)

[ms2sato]

Closes #977

What

Converts the 5 remaining active mock.module() call sites in packages/client (across 4 target modules) to cross-file-safe patterns, and codifies the prohibition + conversion how-to. Full suite green, both directions of every poisoner/victim pair verified.

Phase 1 — classification (all 5 call sites are active poisoners)

Call site	Mocked target	Overridden export(s)	Real cross-file consumer	Verdict
hooks/__tests__/useCreateWorktree.test.ts:7	routes/__root	useWorktreeCreationTasksContext	any real re-import of routes/__root (verified directly); transitively, routes/index.tsx's RepositoryCard (rendered by routes/__tests__/index.test.tsx for the >=1-repo case)	ACTIVE
components/__tests__/SessionSettings.test.tsx:98	routes/__root	useWorktreeDeletionTasksContext, WorktreeDeletionTasksContext	__tests__/routes/WorktreeRow.test.tsx (imports WorktreeDeletionTasksContext directly)	ACTIVE
__tests__/routes/WorktreeRow.test.tsx:10	lib/capabilities	hasVSCode, getVSCodeOpenMode, getVSCodeRemoteHost (not setCapabilities)	routes/__tests__/index.test.tsx (spies on the real module)	ACTIVE
components/sessions/hooks/__tests__/useSessionPageState.test.ts:24	hooks/useAppWs	useAppWsEvent, useAppWsState (the module's only 2 exports)	routes/__tests__/index.test.tsx, __tests__/routes/agents/index.test.tsx (both spy on the real module)	ACTIVE
components/worktrees/__tests__/QuickWorktreeDialog.test.tsx:11	hooks/useCreateWorktree	useCreateWorktree (the module's only export)	hooks/__tests__/useCreateWorktree.test.ts (its own unit-under-test)	ACTIVE

Zero benign/self-consumed cases this round; scope matched the routes/__root-centric shape the Issue anticipated (2 of 5 call sites target __root), so no PR-split was needed.

A note on classification methodology (worth flagging)

Bun's mock.module(specifier, factory) merges factory()'s return onto the real module rather than fully replacing it — an export the factory doesn't declare falls through to the real implementation for every importer. This means "does the victim's test suite still pass" is not a reliable signal for classification: a victim can receive a genuinely poisoned export and still pass every assertion if it happens to be structurally tolerant (e.g. it re-derives both a Provider and consumer from the same poisoned reference, so both sides stay mutually consistent without ever touching the real module).

Classification here was done by object-identity checks (does the victim's imported Context === the poisoner's locally-created mock Context?) and source-string checks (does the victim's resolved function's .toString() match the poisoner's factory body?), forcing deterministic poisoner-then-victim load order via a lexicographically-earlier-sorting temp file copy (Bun's scheduler does not respect CLI argument order or simple directory position — verified empirically, not assumed). Documented in test-standards.md's new section for future classification work.

Phase 2 — conversions

• routes/__root (both mockers): replaced with the real WorktreeCreationTasksContext / WorktreeDeletionTasksContext Providers from contexts/root-contexts.ts (the module routes/__root.tsx re-exports from, without pulling in the router-registration barrel) wrapping an injected fake value. Zero production changes.
• lib/capabilities: replaced with the module's own real setCapabilities() setter (already existed for exactly this purpose). Zero production changes.
• hooks/useAppWs: replaced with spyOn() + .mockRestore() in afterEach, matching the pattern already used by routes/__tests__/index.test.tsx / __tests__/routes/agents/index.test.tsx for the same module.
• hooks/useCreateWorktree: replaced with spyOn() + .mockRestore() in afterEach.

Regression verification — polarity + order-independence

A plain "full suite is green" only shows the current readdir order happens not to expose the poisoning (that's exactly how #970/PR #976's bug hid locally and only surfaced in CI). Explicit polarity evidence per converted poisoner, all pairs run in the same bun:test process with deterministically forced load order:

• Before the fix (pre-fix content re-materialized from HEAD for just the poisoner+victim pair, forced poisoner-first): every pair demonstrably leaks.
  • useCreateWorktree.test.ts → routes/__root: a real re-import's useWorktreeCreationTasksContext resolves to the poisoner's canned closure body.
  • SessionSettings.test.tsx → routes/__root: WorktreeRow.test.tsx's imported WorktreeDeletionTasksContext is Object.is-identical to the poisoner's locally-created mock Context.
  • WorktreeRow.test.tsx → lib/capabilities: a real re-import's hasVSCode resolves to the poisoner's () => false, not the real cached-value reader (setCapabilities, not overridden, stays real — confirming the merge, not full-replace, semantics).
  • useSessionPageState.test.ts → hooks/useAppWs: a real re-import's useAppWsEvent resolves to the poisoner's callback-capturing closure.
  • QuickWorktreeDialog.test.tsx → hooks/useCreateWorktree: forcing this pair against useCreateWorktree.test.ts (its own unit test) drops 4 of 13 tests to failing, because the victim's own hook-under-test is silently replaced by the poisoner's canned mock.
• After the fix: every pair re-run in both orders (poisoner-path-first and victim-path-first, using the real current file paths as explicit bun test arguments) is green with the full expected test count, for all 5 pairs.

Phase 3 — rule / skill updates

• .claude/rules/testing.md Anti-Pattern docs: add development workflow and project guidelines to CLAUDE.md #2: added the cross-file-imported-target prohibition, a prohibited example (routes/__root) and a permitted example (self-consumed target), plus a pre-implementation-checklist bullet.
• .claude/skills/test-standards/test-standards.md: added the 3-pattern conversion how-to (DI seam / spyOn / real-module-plus-injected-fake) with worked code examples referencing this PR and PR feat(client): scroll-back history paging over archived output (closes #959) #976, plus the merge-semantics + load-order-control methodology note above.

Out of scope (per Issue AC)

A mechanical lint gating new cross-file-imported mock.module() targets requires import-graph analysis and is filed as a follow-up: #1226.

Test plan

• bun run test (full workspace: client 2075, shared 596, integration 73, server 3653, embedded-agent 287, scripts/hooks 483) — all green, TEST_EXIT: 0
• bun run typecheck — clean (part of bun run test)
• bun run check:lang — clean (113 files scanned)
• node .claude/skills/orchestrator/preflight-check.js — clean (no coverage gaps, no rule/skill duplication, no language violations, no blame-shift comments)
• Per-pair polarity + both-orders-green verification (above)
• coderabbit review --agent --base main

🤖 Generated with Claude Code

CodeRabbit status (2026-07-27, final) — CLEAN

main's red state (#1225) resolved via PR #1229; head rebased onto it (5cfe359f). First genuine CodeRabbit walkthrough on 5cfe359f (after two rate-limit rounds -- local CLI: headless-worktree automatic_login_failed, structural; GitHub bot: rate-limited at PR open, then the "Review finished... does not re-review" quirk on a same-commit retrigger, then a fresh rate-limit on auto-review-on-push) returned 2 actionable findings, both in test-standards.md:

1. Major -- the spyOn() cross-file-safety claim was stated as an unconditional guarantee; it actually depends on this repo's test files running sequentially (no --concurrent, no concurrentTestGlob). Fixed: named the precondition explicitly and the concurrent-execution case where it would not hold.
2. Minor -- the useAppWsState mock example used an arbitrary false as T cast instead of exercising the real selector contract. Fixed: replaced with a selector(fakeState) example, kept the cast form only as a documented exception (matching the existing routes/__tests__/index.test.tsx precedent).

Fix commit abc6efb7 pushed; verified against actual repo config before writing (no --concurrent flag in packages/client/package.json's test script, no concurrentTestGlob in bunfig.toml). Re-review on abc6efb7 (after a third rate-limit round, cleared and manually retriggered) returned a genuine walkthrough: "No actionable comments were generated in the recent review." Zero new findings; both prior findings confirmed resolved, not carried forward.

3-layer verdict on abc6efb7: pre-merge checks clean, CodeRabbit commit status state: success / description: "Review completed" (verified via the walkthrough body, not just the status line, per the state-vs-description trap), inline actionable comments = 0, reviewDecision empty (walkthrough-exists clean-equivalent case per coderabbit-ops troubleshooting -- a genuine review ran, found nothing to flag, and CodeRabbit does not always submit a formal APPROVED event for zero-finding passes). Independent Architect code-appropriateness audit (session 56b4d446, gen-2): CLEAN -- confirmed zero production changes, verified the context-identity chain between routes/__root.tsx and contexts/root-contexts.ts, verified no hidden dependency on the removed SessionSettings mock's no-Provider fallback branch, and independently confirmed the CR-Major fix's config claims and the documented-cast exception's premise.

Note on CI

The test job's failure is a confirmed pre-existing main-branch flake (this.#handle.unref is not a function in packages/client/src/lib/api.ts's openPath), already tracked in #1225 and reproduced identically on main's own most recent CI run (2026-07-23). This PR's diff never touches packages/client/src/lib/api.ts; locally bun run test (full workspace) is green with TEST_EXIT: 0. All other CI jobs (preflight, language-lint, comment-blame-shift-lint, check-structure, CodeQL/Analyze) pass.

Summary by CodeRabbit

• Tests

  • Improved test isolation by removing process-global module mocking in favor of per-test spies, real context providers, and resettable setup (covering worktree rows, session settings, WebSocket hook behavior, and worktree creation dialogs/hooks).
  • Kept existing assertions for UI states and navigation/DELETE behavior while making setup and cleanup more reliable.

• Documentation

  • Expanded testing guidance with a new “cross-file module mocking poisoning” procedure, clearer semantics/examples, and a repo-wide checklist to prevent unsafe mocking patterns.

[coderabbitai]

📝 Walkthrough

Walkthrough

Testing guidance now restricts cross-file mock.module() usage, and affected client tests use real providers, injected state, or per-test spies with cleanup instead of process-global mocks.

Changes

Cross-file mock isolation

Layer / File(s)	Summary
Document cross-file mocking restrictions .claude/rules/testing.md	Adds rules prohibiting mock.module() targets that other test files import for real, with a required importer check.
Document conversion patterns .claude/skills/test-standards/test-standards.md	Documents dependency injection, named-export spies, provider injection, mock leakage verification, and Bun load-order behavior.
Use real contexts in component and hook tests packages/client/src/components/__tests__/SessionSettings.test.tsx, packages/client/src/hooks/__tests__/useCreateWorktree.test.ts	Replaces route-root module mocks with real context providers and injected task handlers.
Use per-test spies and state reset packages/client/src/__tests__/routes/WorktreeRow.test.tsx, packages/client/src/components/sessions/hooks/__tests__/useSessionPageState.test.ts, packages/client/src/components/worktrees/__tests__/QuickWorktreeDialog.test.tsx	Replaces shared module mocks with capability resets and restored per-test spies.

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

Possibly related issues

• Add lint to gate new cross-file-imported mock.module() targets #1226 — The changes apply the cross-file mock.module() restriction that this issue proposes enforcing with lint.

Possibly related PRs

• ms2sato/agent-console#347 — Modifies the same WorktreeRow and SessionSettings mocking setups.
• ms2sato/agent-console#407 — Adds related process-global mock.module() guidance and dependency-injection alternatives.
• ms2sato/agent-console#513 — Relates to extracting and using real worktree task contexts instead of route-root mocks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The five client mock.module call sites were converted to safer patterns, and the testing rules now prohibit cross-file poisoning.
Out of Scope Changes check	✅ Passed	The diff stays focused on test conversions and testing-rule docs, with no unrelated product or API changes.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: removing cross-file mock.module poisoning in client tests.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/977-mock-module-cross-file-poisoning

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

Test Coverage Check

No production files matching coverage patterns were changed.

---

Rule/Skill Duplication Check

✅ No rule paragraphs found verbatim in any skill file.

---

Language Check (public artifacts)

✅ All public artifacts use Latin / Greek / Cyrillic scripts only.

---

Source-Comment Blame-Shift Check

✅ No new Issue / PR / dated CodeRabbit references in source comments.

[ms2sato]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[ms2sato]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

.claude/skills/test-standards/test-standards.md (1)

266-268: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use explicit process ordering or separate processes instead of ordering-based proof.

The __ directory prefix and filename ordering rely on observed Bun test-file loading behavior, not a guaranteed contract; these documents describe them as normative/deterministic levers, which can change across versions, platforms, randomization, or worker concurrency. Reword this as a version/platform-specific empirical note, or add a driver/preload/process isolation and unique sentinels if the verification needs a stable order.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/skills/test-standards/test-standards.md around lines 266 - 268, The
documented verification approach incorrectly presents Bun’s directory and
filename ordering as deterministic. Update the guidance in
.claude/skills/test-standards/test-standards.md (lines 266-268) and
.claude/rules/testing.md (lines 24-29) to label this behavior as
version/platform-specific empirical evidence only; recommend explicit process
ordering, a driver/preload, or process isolation with unique sentinels for
stable proofs, and require verification rather than relying on naming
conventions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.claude/skills/test-standards/test-standards.md:
- Around line 214-218: Update the useAppWsState mock implementation to avoid
returning false as an arbitrary generic cast. Provide a typed fake
AppWebSocketState and invoke the selector with that state, or specialize each
test mock to the concrete return type being tested; preserve the generic
selector contract.
- Around line 196-199: Update the “Pattern 2 — spyOn() on a named export”
guidance to remove the claim that spyOn cannot leak across test files. State
that mockRestore() is still required and that shared module state or concurrent
execution requires test isolation or serial execution for these spies.

---

Nitpick comments:
In @.claude/skills/test-standards/test-standards.md:
- Around line 266-268: The documented verification approach incorrectly presents
Bun’s directory and filename ordering as deterministic. Update the guidance in
.claude/skills/test-standards/test-standards.md (lines 266-268) and
.claude/rules/testing.md (lines 24-29) to label this behavior as
version/platform-specific empirical evidence only; recommend explicit process
ordering, a driver/preload, or process isolation with unique sentinels for
stable proofs, and require verification rather than relying on naming
conventions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 82b6f7b2-b03b-4c0b-91ad-31255dadf12f

📥 Commits

Reviewing files that changed from the base of the PR and between f396420 and 5cfe359.

📒 Files selected for processing (7)

• .claude/rules/testing.md
• .claude/skills/test-standards/test-standards.md
• packages/client/src/__tests__/routes/WorktreeRow.test.tsx
• packages/client/src/components/__tests__/SessionSettings.test.tsx
• packages/client/src/components/sessions/hooks/__tests__/useSessionPageState.test.ts
• packages/client/src/components/worktrees/__tests__/QuickWorktreeDialog.test.tsx
• packages/client/src/hooks/__tests__/useCreateWorktree.test.ts

[ms2sato]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.