Skip to content

test: convert mock-module-lint baseline call sites (closes #1238) - #1249

Merged
ms2sato merged 1 commit into
mainfrom
test/1238-mock-module-baseline-conversion
Jul 30, 2026
Merged

test: convert mock-module-lint baseline call sites (closes #1238)#1249
ms2sato merged 1 commit into
mainfrom
test/1238-mock-module-baseline-conversion

Conversation

@ms2sato

@ms2sato ms2sato commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Closes #1238

Summary

Converts all 8 KNOWN_VIOLATIONS baseline entries from Issue #1226 (introduced by PR #1239) to cross-file-safe patterns, per the #977 playbook priority order (DI seam > spyOn() > fetch-level stub > central-registry migration).

File Specifier Conversion
packages/integration/src/paste-focus-isolation.test.tsx @agent-console/client/src/lib/api Pattern 1 (DI seam, existing)
packages/integration/src/paste-focus-isolation.test.tsx @agent-console/client/src/lib/worker-websocket Deleted (mocked an export the real module doesn't have)
packages/server/src/__tests__/api.test.ts ../lib/pty-provider.js Deleted (redundant duplicate of the sanctioned central-registry mock)
packages/server/src/__tests__/api.test.ts open Deleted (redundant duplicate; now imports the shared mockOpen)
packages/server/src/routes/__tests__/system.test.ts open Deleted (redundant duplicate; central-registry mock already active)
packages/server/src/services/__tests__/worktree-creation-service.test.ts ../../lib/logger.js Deleted (vestigial -- real logger already silent in NODE_ENV=test)
packages/server/src/services/__tests__/worktree-deletion-service.test.ts ../../lib/logger.js Deleted (same as above)
packages/server/src/services/inbound/__tests__/diff-worker-handler.test.ts ../../git-diff-service.js Pattern 2 (spyOn() + .mockRestore())

Priority item: paste-focus-isolation.test.tsx

This was the live #1225-class poisoner named in the Issue: it mock.module()'d @agent-console/client/src/lib/api (overriding sendWorkerMessage), which sibling integration tests (system-api-boundary.test.ts, config-api-boundary.test.ts, etc.) import for real in the same bun:test process.

Investigation found both mocks were leftover from before MessagePanel gained its current onSend prop -- MessagePanel no longer imports sendWorkerMessage or anything from worker-websocket.ts at all (it takes onSend/onEscape as injected callbacks), and worker-websocket.ts doesn't even export a sendInput function today. Fix: removed both mock.module() calls and passed onSend: mock(() => Promise.resolve()) via the existing DI seam (also closes a latent gap where defaultProps omitted a required prop, invisible because packages/integration isn't part of bun run typecheck).

Load-order-independence verification (PR #1227 methodology -- forced deterministic order via a temp src/__poison_check/ pair, not committed): with the pre-fix content forced to load before a probe that imports the real sendWorkerMessage, the probe observed a mock-stringified function ("function () { [native code] }") instead of the real implementation's source -- confirming the poisoning. With the post-fix content, the probe passed in both load orders (poisoner-first and reversed). The real integration suite (bun test --preload ./src/setup.ts src/, 73 tests / 27 files) is also green.

pty-provider.js / open duplicates (api.test.ts, system.test.ts)

Both files already import ./test-utils.js, which is the sanctioned central mock registry and already registers equivalent mock.module() calls for ../lib/pty-provider.js (its own bunPtyProvider.spawn mock) and open (via mock-open-helper.ts, re-exported as mockOpen). The two files' own local re-declarations were 100% redundant duplicates -- verified by deleting them and confirming all assertions still pass (api.test.ts: 137 tests; system.test.ts's local mockOpen was never even asserted on). api.test.ts now imports the shared mockOpen from test-utils.js for its existing assertions.

logger.js (worktree-creation-service.test.ts, worktree-deletion-service.test.ts)

lib/logger.js's pino instance sets enabled: !isTest, and Bun sets NODE_ENV=test by default for bun test runs -- so the real logger is already silent. Verified empirically (removed the mock, ran both files, all 57 tests still pass) before deleting the mocks outright; no replacement mock needed.

git-diff-service.js (diff-worker-handler.test.ts)

handlers.ts imports triggerRefresh directly at module scope (no DI seam via InboundHandlerDependencies today, contrary to the Issue's "candidate for DI" framing -- see the DI-gap note below). Per playbook priority, converted to spyOn(gitDiffServiceModule, 'triggerRefresh') + .mockRestore() in afterEach (Pattern 2) instead of adding a production DI seam in this test-only PR.

Production DI gap found (reported, not fixed)

packages/server/src/services/inbound/handlers.ts imports triggerRefresh from git-diff-service.js as a bare module-level function call, with no seam through InboundHandlerDependencies (unlike sessionManager/broadcastToApp, which are already injected). The Issue's own baseline table flagged this as "candidate for DI via InboundHandlerDependencies". Since this PR's reversibility is scoped to test-only changes, I used spyOn() (next playbook priority) instead of adding the production seam. If threading triggerRefresh through InboundHandlerDependencies is wanted, that's a separate, reviewable production change -- happy to file a follow-up Issue if desired.

Verification

  • bun run test (full workspace) -- green, TEST_EXIT: 0 (client 2075, shared 596, integration 73, server 3720+1 skip, embedded-agent 287, scripts/hooks 521 -- all 0 fail)
  • bun run check:mock-module -- exit 0, KNOWN_VIOLATIONS is now []
  • node .claude/skills/orchestrator/preflight-check.js -- clean (no coverage gaps, no rule/skill duplication, no language violations, no blame-shift comments), exit 0
  • bun run typecheck -- clean (ran as part of bun run test)

CodeRabbit status

Two independent layers checked, per coderabbit-ops's "fourth surface" guidance (commit-status state alone is not the verdict -- the description string is):

  • Local CLI (coderabbit review --agent --base main): hung with no output until timeout. coderabbit auth status confirms authentication, not rate-limiting: Status: signed out, Next step: coderabbit auth login. This is the documented headless-worktree automatic_login_failed case (interactive login is required and unavailable here).
  • GitHub-side bot: commit status is state: success / description: "Review rate limited" -- rate-limited, not a genuine clean review. reviewDecision is empty (no review submitted yet).

Both layers are currently unavailable for a genuine review (one by auth, one by rate-limit) -- distinct root causes on each surface. Flagging explicitly per the reversibility/verification requirements; owner or a session with interactive CodeRabbit auth, or a retry once the GitHub-side rate-limit clears, should confirm the review before merge.

Reversibility

Test-only changes, no production behavior change (per the Issue's own reversibility note). The one production DI gap found during conversion is reported above, not applied.

… patterns (closes #1238)

Converts all 8 KNOWN_VIOLATIONS baseline entries from Issue #1226 per the
#977 playbook (DI seam > spyOn > fetch-level stub > central registry):

- paste-focus-isolation.test.tsx (priority, #1225-class live poisoner):
  MessagePanel resolves send via an injected onSend prop, not the mocked
  modules -- removed both mock.module() calls, wired the existing DI seam.
- api.test.ts / system.test.ts: pty-provider.js and open were duplicates
  of mocks already registered by the sanctioned central registry
  (test-utils.ts / mock-open-helper.ts) once those files import test-utils;
  removed the redundant local registrations.
- worktree-creation-service.test.ts / worktree-deletion-service.test.ts:
  lib/logger.js's pino instance is already disabled when NODE_ENV==='test'
  (Bun's default for `bun test`), so the mock was vestigial; removed.
- diff-worker-handler.test.ts: triggerRefresh has no DI seam in handlers.ts
  yet, so converted to spyOn()+mockRestore() instead (next in playbook
  priority) rather than adding a production DI seam in this test-only PR.

Removes the corresponding KNOWN_VIOLATIONS entries from
check-mock-module-poisoners.mjs; bun run check:mock-module now passes with
an empty allowlist.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@ms2sato, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 40 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: dfd42936-0e9a-478c-a3aa-b445484911d4

📥 Commits

Reviewing files that changed from the base of the PR and between 6d9af63 and eaf5efb.

📒 Files selected for processing (8)
  • packages/integration/src/paste-focus-isolation.test.tsx
  • packages/server/src/__tests__/api.test.ts
  • packages/server/src/routes/__tests__/system.test.ts
  • packages/server/src/services/__tests__/worktree-creation-service.test.ts
  • packages/server/src/services/__tests__/worktree-deletion-service.test.ts
  • packages/server/src/services/inbound/__tests__/diff-worker-handler.test.ts
  • scripts/__tests__/check-mock-module-poisoners.test.mjs
  • scripts/check-mock-module-poisoners.mjs

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

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

@github-actions

Copy link
Copy Markdown

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
ms2sato merged commit 6d4c604 into main Jul 30, 2026
9 checks passed
@ms2sato
ms2sato deleted the test/1238-mock-module-baseline-conversion branch July 30, 2026 02:38
ms2sato added a commit that referenced this pull request Jul 30, 2026
…rrors (#1252)

* test: add typecheck script to packages/integration, fix latent type errors

packages/integration had no typecheck script, so the root `bun run typecheck
--filter '*'` silently skipped it (Issue #1249's onSend gap surfaced this).
Adds `tsc --noEmit`, fixes the ~30 latent type errors it uncovered (missing
required dependency fields, unused imports, discriminated-union widening,
Hono generic narrowing), and widens the package's devDependencies/tsconfig
lib to match its sibling packages.

Closes #1250

* fix: address CodeRabbit MAJOR on unsafe type assertions in integration tests

Replace the fetch mock's `as unknown as typeof fetch` double-cast with the
repo's clean Object.assign(mockFetch, { preconnect }) pattern already used
by several client tests, type suggestSessionMetadata against its real
SuggestSessionMetadataFn contract instead of `as any`, and narrow
info.startedAt with a runtime typeof guard instead of an `as string` cast.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert mock-module-lint baseline call sites (follow-up to #1226)

1 participant