Skip to content

Move low-value tests to their owner boundaries and drop test-only code - #84

Merged
szweibel merged 1 commit into
mainfrom
test-audit/owner-boundary
Sep 24, 2026
Merged

szweibel merged 1 commit into
mainfrom
test-audit/owner-boundary

Conversation

@szweibel

Copy link
Copy Markdown
Contributor

Summary

A test audit pass. It removes tests that only kept test-only exports alive, tests that duplicate stronger route or component proof, and one negative assertion that could not fail. It also removes the production code whose only callers were those tests. It adds no new product behavior.

Changes, grouped by what they removed

Dead production code with test-only callers

  • jsonHeaders (lib/http.ts) had no production caller since the March Workers migration. The whole lib/http.test.ts goes with it.
    • The readFormData and binaryBody cases repeated route-level proof that stays in place:
      • regressions.test.ts: "stores the exact bytes of a valid multipart upload"
      • regressions.test.ts: "rejects malformed multipart framing without writing a file"
      • regressions.test.ts: "SS-21: accepts a real PNG body on the thumbnail route and preserves its bytes"
      • preview.test.ts: "preserves published HTML bytes when no mount rewrite is needed"
  • supportsDocumentExtraction (lib/document.ts) had no caller. The agent calls extractDocumentText, and "rejects unsupported document types" still covers the PDF-only rule.
  • outcomeForStatus (lib/logging.ts) was a one-line wrapper. Its test assertion now calls the owner, terminalForStatus(200).outcome.
  • getRequestIdentity (lib/cail-identity.ts) was a test-only wrapper that returned null. All 18 identity cases remain, now on resolveRequestIdentity: rejections assert {status:"invalid"}, which the wrapper could not tell apart from absent, and successes assert the verified identity.
  • parsePositiveInteger (types.ts) had no callers anywhere.
  • cailIdentityJwt context value and getCailIdentityJwt: since e1cf4b6, model calls forward only the gateway leg. Nothing read the stored app-leg token.
    • A grep across packages/, including routes/agents.ts and the agent/Durable Object code, found only the setter, the getter, and a type field.
    • The stale comment claiming handlers forward it is corrected.

Duplicate tests

  • fleet-projection.test.ts: the test "recognizes every canonical action route…" repeated the action inventory already pinned by observability-contract.test.ts "defines queryable build and publish action seams". Its only unique check, ROUTE_TEMPLATE_RE validity, moved into that test.
  • AgentChat.test.ts: "schedules a reconnect with backoff after an unexpected socket close" ran the same first reconnect as "keeps reconnecting when a reconnect attempt errors without logging socket details", which goes further. The one unique assertion, no new socket before the timer fires, moved into the retained test.

Rewritten at the owner boundary

  • session.test.ts: "stores the verified canonical token for downstream calls" is replaced by two tests through the real authMiddleware. Before this, nothing tested the forwarded gateway leg.
    • A same-subject cail:gateway token is stored and returned by getCailGatewayJwt(c).
    • A gateway token for a different subject returns 401.
    • Mutation check: removing c.set("cailGatewayJwt", …) fails the first test, and ignoring gatewayLeg === "invalid" fails the second.
  • site-builder.test.ts (buildProjectContext): not.toContain("Uploaded documents in the project: assets/legacy.docx") could never fail, because files are sorted and a leaked .docx would follow cv.pdf in that line. "supported documents" only pinned wording removed in Repair Site Studio uploads, tools, and recovery #68.
    • The new check finds the extract_document_text line and asserts that it contains assets/cv.pdf and not legacy.docx.
    • Mutation result: with the filter in agents/project-context.ts changed to also admit .docx, the old assertions passed. The new test fails with expected 'Uploaded documents in the project: as…' not to contain 'legacy.docx'. The source was restored afterward.

LOC

Added Removed Net
Production 4 45 −41
Tests 85 163 −78

Test counts: app goes from 760 to 750 (−9 http, −1 document, −1 fleet-projection, +1 session). Frontend goes from 225 to 224.

Kept on purpose (false positives)

  • api/errors.test.ts message assertions check passthrough of server messages. "collapses plain Errors to the generic message" is a leak guard.
  • Preview.test.ts alert text and .loading-overlay test the preview-ready protocol and the role=alert recovery.
  • The generic error text in HandleClaimDialog, ProjectHistoryDialog, AgentChat and site-builder guards against leaks or project-switch races, or passes through server messages.
  • boundary-logging "Service event recorded." guards against free-text log bodies.
  • The health and version-metadata tests in observability-contract.test.ts back the deploy job's release probes.
  • The mount dispatch tests in index.test.ts, plus the protected-files, agent-identity and csrf tests, cover routing and security.
  • The archive README.txt text in r2.test.ts is part of the archive-contents contract.
  • The reconnect tests at AgentChat.test.ts lines 2263 and 2306 each cover a different failure mode.

Open question (not changed here)

Several parts of lib/observability/contract.ts and lib/observability/action-attempt.ts have no production reader:

  • createCloudflareHealthCheckSpec, auditTelemetryLifecyclePairs, summarizeDurableActionReliability and CAIL_LOG_ENVIRONMENTS are called only by tests.
  • The collection, fleetProjection, access, serviceLevels and dashboardViews sections of OBSERVABILITY_CONTRACT are never read at runtime.

docs/cail-log-alignment.md describes these as an offline source contract and lifecycle auditor. Should they stay as a documented operator recipe, or be removed with their tests (about 300 production lines)? This PR leaves them untouched.

A smaller follow-up: after this change, the token field of resolveRequestIdentity's verified result is read only by tests. The doc comment on that function still says downstream model calls forward it.

Validation

  • bunx vitest run over the 10 affected app files, including regressions and preview: 267 of 267 passed.
  • bunx tsc --noEmit in packages/app: exit 0.
  • bunx vitest run src/lib/components/AgentChat.test.ts: 60 of 60 passed.
  • bun run check: rc=0.
    • App: 32 files, 750 tests passed.
    • Chat-runtime and preview-route contracts passed.
    • Frontend: 23 files, 224 tests passed; svelte-check reported 0 errors; the build succeeded.
  • git diff --check: clean.

🤖 Generated with Claude Code

Delete tests that only kept test-only exports alive, duplicated stronger
route or component proof, or asserted a negative that could not fail.
Remove the production code whose only callers were those tests:
jsonHeaders, supportsDocumentExtraction, outcomeForStatus,
getRequestIdentity, parsePositiveInteger, and the unread cailIdentityJwt
context value with its getter.

Retarget the identity verification cases at resolveRequestIdentity with
the same case count, and replace the app-leg storage test with coverage
of the gateway leg the middleware actually forwards, including the
different-subject rejection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@szweibel
szweibel merged commit e727f72 into main Sep 24, 2026
5 checks passed
@szweibel
szweibel deleted the test-audit/owner-boundary branch September 24, 2026 16:21
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.

1 participant