Skip to content

🤖 tests: phase-3 re-audit residuals with no other tracking issue #7635

Open

Description

@ThomasK33

The phase-3 test audit re-graded all five areas at main 0059c29c84 (frontend at 9834c30c4d). Each area graded A- or better, and none is borderline. The auditors also listed weaknesses that no existing issue covers. This issue tracks them. Each item gives the place and the reason it matters.

Items already tracked elsewhere are not repeated: #7582 (orchestration), #7587 (streaming), #7604 (seam and export checks), #7608 (fixed sleeps), #7613 (type-aware cast rule), #7615 (orpc casts), #7626 and #7321 (flakes).

Frontend

  1. settle() is weaker than the 20 ms sleep it replaced (tests: settle reconnect suites on promise chains instead of a 20 ms sleep #7607). settle() (connection.testHarness.tsx:129) drains one setImmediate turn. Mutant evidence: a reset delivered through setTimeout(0) survives the new negative checks at RightSidebar.reconnect.test.tsx:55-58,90-91, but the pre-tests: settle reconnect suites on promise chains instead of a 20 ms sleep #7607 test killed it. Fix: advance a fake clock past the owners' timers in settle(), or assert through advanceClock. This regression came from a phase-3 PR, so it comes first.
  2. as never bypasses no-cast-to-api-client. Untyped fakes reach the API client at DraftStore.sendIds.test.ts:197,199, WorkflowRunToolCall.test.tsx:118 (APIHarness takes client: unknown, so the whole suite runs on an untyped fake), NestedToolRenderer.test.tsx:166, WorkflowTimeline.test.tsx:289,509. 🤖 tests: make no-cast-to-api-client type-aware (P3-4 follow-up) #7613's fix does not catch this. Fix: type the fakes, then flag any type assertion inside a createTestApiClient(...) argument or an APIContext api: value.
  3. no-fixed-sleep gaps not in 🤖 tests: P3-4 fixed-sleep residuals in browser tests #7608: setTimeout as delay from node:timers/promises, and wait helpers in src/browser support files (outside the rule's file scope).
  4. The WorkspaceMenuBar import-count test still runs in its own process. The reason is written only in the tests: retire the Mermaid isolation entry #7591 body. Record it next to the isolation entry, or make the test pass in the shared process.
  5. Mermaid.test.tsx:75-86 restores its LazyFeature spy inline, not in finally, so a failed assertion at :82 leaks the spy.

Orchestration

  1. 9 .sessions.set writes remain outside the real-host builder: workspaceService.rename.test.ts:217,330,410, workspaceService.initCancellation.test.ts:582, workspaceService.archive.test.ts:954, workspaceService.test.ts:414, workspaceGoals.repro.test.ts:241, serviceContainer.test.ts:526-527. Move each onto createRealHostWorkspaceService, or record why it must plant a session.
  2. Two surviving mutants in the task launch path (seen with taskLaunchRepro only, not the full taskService suite): dropping the re-check under the fork lock (taskService.ts:7934), and forcing reusedExistingCheckout: true (taskService.ts:7973). First run the full taskService suite against both. If they still survive, add tests.
  3. 🤖 tests: remaining private-state access in orchestration tests (turn seeds, WorkspaceTurnManager, contextController) #7582 says 17 turn seeds remain. Its own list has 14 call sites. Correct the count.

Streaming

  1. streamManager.completion.test.ts:97-130 ("the retry's partial delete holds the history write lock") only exercises historyService.deletePartialIfMessageIdMatches. It is that method's only lock test. Move it into a historyService.*.test.ts file.
  2. historyService.test.ts:1498,2000,2037 read the private sequenceCounters. The tests already check the next append's historySequence, which is the public behavior, so drop the private reads.
  3. Kept on purpose (no work planned): the 20 private casts in memoryService.test.ts guard a memoization step that has no public signal. The 8 private reads in agentSession.continuousCompaction.test.ts are older 🤖 tests: drive continuous-compaction collaborators through the session's real seams #4604 residuals.

Tools

  1. installService.test.ts awaits the private settledSignOutPasses at :3795, 3846, 3960, 4046, 4094, 4168, 4196, 4247, 4290 (9 of its 29 private casts), and patches writeRegistry (:1059, 1126, 1354) and removeDir (:5760, 7063, 7209). Wait on a public signal instead.
  2. workspaceMcpOverridesService.test.ts calls the private bumpOverridesEpoch at :2667, 3455, 3628 (the largest group of its 27 casts).
  3. Flaky 100 ms stale-lease window. WorkflowRunner.attemptDisposition.test.ts:18 (STALE_LEASE_MS = 100): "already-active errors name the lease owner and its freshness" failed once under load (914 ms), because the lease went stale. Use a fake clock or a much longer window.
  4. The singular "1 more server" branch (toolCatalog.ts:440) is untested: toolCatalog.test.ts:344 matches /\d+ more servers/.
  5. toolCatalog.test.ts:611 checks the fixed text "Search deferred tools", not the overview its name claims.
  6. Artifact test overlap: artifact_list.test.ts:39 repeats :46, and artifact.test.ts:102 only counts exec calls.
  7. The dynamic agent_report schema (agent_report.ts:93,156) is outside the .nullish() check over TOOL_DEFINITIONS.

Cross-cutting

  1. The seam check misses "injectable for tests", "x is for tests" and "test double(s)" wording (SEAM_COMMENT_PATTERNS, scripts/check-test-seam-comments.ts:32). Uncaught seams today: the sendRetryDelayMs option (DraftStore.ts:532, only DraftStore.sendIds.test.ts:233 passes it), the pathImpl parameter (artifactVersionsOperations.ts:65, only tests pass path.win32), and the "(test doubles)" branch at turnRequestBuilder.ts:2684. 🤖 tests: seam check flags test-double seam claims #7620 covers part of the "test doubles" wording.
  2. Exports used only in their own file: WorkspaceServiceEvents (workspaceService.ts:2305) and MOCK_STREAM_START_GATE_MARKER (mockAiRouter.ts:64). Drop export, and have check-test-only-exports (🤖 tests: fail on production exports that only tests (or nothing) reference #7561) flag this case, which its own error message already describes.
  3. 67 as any remain in src test files (43 with eslint-disable lines). This is not a regression. Reduce it as files are touched.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $43.07

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions