S541 — test-tree restructure: the 2b factoring pass and workstream 3

Migrated at S541 close from .user-scratch/test-tree-structure-conventions-and-consolidation.md (gitignored). Point-in-time: describes the state at the close of S541, not current state.

Problem

The __tests__ tree has drifted into two competing layouts for the same kind of test, with no automated guard to hold either. Route-handler tests live in both __tests__/api/** (96 files) and __tests__/app/api/** (8 files); lib/validation tests live in both __tests__/lib/validation/** and __tests__/validation/**, including a schemas.test.ts in each. Several cross-cutting and catch-all files have no principled home. The immediate question — whether to mirror the Next.js production path — needed answering before any files move, because moving the wrong way doubles the work.

Research findings that decide the direction

Next.js has no opinion. The official project-structure guide states Next.js is unopinionated about organisation, and the official Vitest guide’s own example places a page test at __tests__/page.test.tsx — a centralised tree that does not mirror app/ — noting only that colocation inside app/ is also possible. There is no framework recommendation to mirror the production path, so this is purely a project convention. Mirroring is already the repo’s dominant convention. Of the 18 top-level directories in __tests__, nine mirror a production directory exactly (app, components, contexts, deploy, docs, hooks, lib, scripts, types) and six are test infrastructure with no production counterpart (build, fixtures, helpers, integration, pipeline, workflows). Exactly three elide their production parent segment: api (for app/api), validation (for lib/validation) and mcp (for lib/mcp). Those three are precisely the directories that have produced split trees, duplicate filenames and repeated agent confusion. The pattern is self-evidently “test path equals production path”, and api/ is the anomaly — not app/api/. The documented rule was not written to avoid churn. docs/reference/testing/test-philosophy.md §3 states a principle (“a test file’s location should be derivable from its production-code import”) and the audit behind it flagged 25 files as mislocated — it was written to cause moves, not to avoid them. Layout has no CI impact. .github/workflows/ci.yml shards with bunx vitest run --shard=N/4, which partitions by file count, not directory. There is no functional argument either way. Domain-first cannot apply to routes. Next.js derives the URL from the path, so app/api/procurement/** must stay where it is; the route surface stays layer-first at the top permanently. A domain-first test tree such as __tests__/procurement/api/** would diverge from the route path rather than track it. Domain-first is the right shape for lib/, and that already holds: lib/domains/procurement/ is mirrored by __tests__/lib/domains/procurement/. Adding the app/ segment costs almost nothing. 92 of the 95 files under __tests__/api/ already import helpers through the @/__tests__/... alias and the remaining three import no helpers at all, so there are zero relative-import edits: the move is a single directory rename that git records as such, preserving history. No config or CI file hardcodes the path (vitest.config.ts globs __tests__/**, CI shards by file count). The only external references are two explanatory comments in route files and docs/reference/testing/corpus-manifest.json, all of which point at __tests__/validation/corpus-manifest.test.ts — a guard that stays where it is regardless.

Decision

Reversed from the initial recommendation. Adopt one rule with no exceptions: a test’s path under __tests__/ equals its production path from the repo root. This means __tests__/api/** moves to __tests__/app/api/** (joining the eight files already there), __tests__/validation/** library tests move to __tests__/lib/validation/**, and __tests__/mcp/** moves to __tests__/lib/mcp/**. The earlier recommendation — consolidate into __tests__/api/ and document the exception — was wrong. It preserved the anomaly and relied on documentation plus a guard to stop humans and agents drawing the obvious inference from the other nine mirrored directories. Since the mapping is mechanical, the migration needs no import edits, and the application is not yet live, removing the root cause is cheaper than policing it. It also collapses three separate special cases into one principle a guard can enforce trivially. Directories that are test infrastructure rather than tests of production code (helpers, fixtures, integration, build, workflows, pipeline) keep their current names — they mirror nothing, so the rule does not apply. Next.js private-folder syntax (_cross-cutting) remains available for grouping inside the mirror without implying a route.

Proposed conventions to record in test-philosophy.md §3

These are currently unwritten, which is why the tree drifted in shape as well as location. Directory mirrors the production path exactly, including [param] brackets. This fixes the existing inconsistency between __tests__/app/api/intelligence/workspaces/[id]/ (brackets) and __tests__/app/api/refinement/touchpoints/id/ (no brackets). Filename is the production module’s basename plus .test.ts(x) — so route.test.ts for a route handler, page.test.tsx for a page, new-item-tabs.test.tsx for a colocated component. Prefer one test file per production module. The dot-suffix aspect form (<module>.<aspect>.test.ts) is reserved for genuine variants of the same subject — page.mobile.test.tsx beside page.test.tsx, and the existing layout.branding.test.tsx / client-config.branding.test.ts instances. Hyphenated forms are not used, since a hyphen reads as part of the module name and flattening it loses the aspect. A collision is a factoring signal, not a naming problem. Where a mechanical move lands two files on one path, the resolution is to split or merge so the tests map one-to-one onto production routes — not to invent a suffix. The .mobile conversions already applied are the extent of the dot-suffix expansion; no further ones are sanctioned. The parent directory name is never repeated in the filename, which retires the procurement/procurement-export.test.ts style. Location is not factoring. The mirror rule constrains where a file sits, not what it covers, and the codemod only checks the former. Two failure modes survive a clean run: a multi-route file parked at its common-ancestor directory, and one route split across files by HTTP method. __tests__/app/api/guides/guides.test.ts is the clearest example — it covers three routes and overlaps guides/route.test.ts on GET /api/guides, yet reports OK. Cross-cutting route tests — those that reach several unrelated routes to prove a shared property — live in __tests__/app/api/_cross-cutting/. The underscore prefix is Next.js private-folder syntax, so it groups without implying a route. Boundary tests (error.tsx / loading.tsx) fold into the route folder they belong to as boundaries.test.tsx, not into a boundaries/ subfolder. Grouping by file-kind would cut across the route mirror and repeat the same axis mistake as the old api anomaly.

Workstreams

Ordered so that each lands independently and the guard arrives only once the rules it enforces are settled.

1. Consolidate route tests into the mirror — DONE (commit 8f1cb4c2b)

All 95 files moved from __tests__/api/** to __tests__/app/api/**, joining the eight already there (103 total). Git recorded every move as a 100%-similarity rename, so history is preserved. Five files still carrying deep relative helper imports were normalised to the @/__tests__/helpers/... alias, so the tree now has zero relative helper imports and future moves need no import edits. Verified: 103 route test files pass, typecheck clean, lint 0 errors, Prettier clean.

2. Align every test path to its production path — DONE (commits 4587afdde, 597158bfd)

scripts/codemods/align-test-paths.ts is a ts-morph CLI following the wrap-define-route house pattern (dry-run default, --apply, --scope, artefacts to the gitignored docs/generated/, 26 tests under __tests__/scripts/codemods/). It moves files with git mv so renames are recorded. ts-morph rather than regex was load-bearing: a regex prototype counted vi.mock('@/app/item/new/batch/batch-create-client') as a second subject for new-item-tabs.test.tsx and would have mis-routed it. Only imported @/app/** modules are subjects; vi.mock() / vi.doMock() arguments are dependencies, which is an AST property and not a textual one. 84 files relocated. Dynamic segments regained their brackets, boundary tests folded into the route folder they cover, and tests spanning unrelated routes moved to _cross-cutting/. Re-running reaches a fixed point at 112 OK and 0 pending moves. Verified: 140 test files pass, typecheck clean, lint 0 errors, Prettier clean. Two defects surfaced during the run, both now covered by tests. A hyphen-suffixed variant was being flattened to the bare module name, so session/page-mobile.test.tsx became session/page.test.tsx — discarding the mobile aspect while reading as the canonical page test; hyphen aspects now convert to dot form, which also dissolved three escalations. And a relative specifier inside an import() type position survived the earlier alias sweep, which only matched from '...'; __tests__/app now has no relative specifiers in any position.

2a. Escalations — partially resolved (commit b012fa2a4)

Eleven of the original 23 escalations are resolved by hand; the codemod now reports 119 OK and 16 escalated. The remainder are not naming problems to settle with suffixes — each needs an investigation into how the tests should be factored against production, which may also change route.test.ts files that currently report OK. The files named below are starting points, not the full scope.

2b. Split/merge review (in progress)

One test file per route is the target. For each group: read the production routes, decide the split or merge, then move. Each group lands as its own commit so the diffs stay reviewable. Every group below except q-a-pairs is running in a parallel sub-agent in an isolated worktree, cherry-picked back on completion.

• q-a-pairs — DONE (commit 94bfae654). route.test.ts tested PATCH on [id]/route while [id]/route.test.ts tested DELETE on the same route, so the name implied a collection route that does not exist — there is no app/api/q-a-pairs/route.ts. Merged onto [id]/route.test.ts covering both handlers; 19 tests pass. The two files used different auth-helper styles (configureRole/configureUnauthenticated vs configureAuth); both are retained, each inside its own describe with its own beforeEach, since vi.clearAllMocks() isolates them. The DELETE file’s @vitest-environment node pragma was dropped and its tests pass under the jsdom default.
• All of procurement, starting from procurement-responses-crud.test.ts, procurement.test.ts and procurement-responses.test.ts.
• templates-cron.test.ts — split.
• history.test.ts and review-history.test.ts — both land on review/history/route.test.ts.
• queue-publication.test.ts, queue.test.ts, review.test.ts.
• sources-test-poll-web.test.ts and sources.test.ts.
• guides.test.ts and guides/route.test.ts — DONE (commit 2e490f0a5). Split into route.test.ts (12), [slug]/route.test.ts (10) and [slug]/sections/route.test.ts (9); 31 tests before, 31 after. The codemod’s clean report was confirmed by execution, not repetition: reconstructing the pre-change state gave 134 examined / 119 OK with guides.test.ts in neither the moves list nor the escalations. The “overlap” claim in this doc was wrong, and the sub-agent was asked to challenge it rather than confirm it. Same route, yes — different behaviour, with zero duplicated assertions. route.test.ts proved only that the retired ?include=stats leg is inert (a negative regression guard from ID-131.19 escalation 2b, DR-034); guides.test.ts’s GET block proved 401-unauthenticated, the plain 200 listing and the type=sector filter, none of which touch include. So the merge was a union under the one-file-per-route rule, not a de-duplication — and had it been treated as duplication, one side would have been silently lost. This generalises: a collision means “same route”, never “same behaviour”, and the assertion-level check is the one that decides. A near-miss worth carrying: the two files used different fixture shapes (route.test.ts fully-populated, guides.test.ts sparse). Unifying them looks like tidying but would have been a real coverage loss — nine projected columns in app/api/guides/route.ts are marked .optional() because the sparse assertions exist, and the route’s own comment named the test as the reason. Both shapes retained; the production comment repointed at the merged file. guides.test.ts never imported [slug]/sections/[sectionId]/route, so this split did not collide with the remaining-routes.test.ts lane.

RSS feed routes — DECIDED: move under api/intelligence/, and authenticate them

The investigation recommended leaving app/api/feeds/[workspaceId]/rss/ where it was, on two grounds: that the public/authenticated split made the unauthenticated surface auditable at a glance (/api/feeds/ public, /api/intelligence/ auth-required), and that the path was a published URL contract rendered by components/intelligence/rss-feed-panel.tsx. The owner ruled against both. The unauthenticated surface is an oversight, not a design: auth should be implemented, and the routes belong under the intelligence namespace. The URL-contract objection does not apply — the app is pre-launch, with no real users and no bookmarks to break, and the direction is to resolve architectural issues rather than preserve current shape. Two facts qualify the change without altering it. First, proxy.ts exempts all of /api/** from the auth redirect, so relocating the route into the intelligence namespace confers no auth by itself — every route there gates itself in its own handler, and namespace-as-auth-boundary is a convention, not an enforcement point. The relocation and the auth work are therefore independent. Second, an RSS feed’s consumers are external feed readers and intranet embeds, which cannot perform a cookie/session login; if that holds, getAuthorisedClient() would make the feature unusable rather than secure it, which is precisely why id-166 named signed URLs and workspace-scoped tokens as the candidates. That claim was dispatched to be challenged rather than confirmed. Executing: routes move to app/api/intelligence/workspaces/[id]/rss/{route,filtered/route}.ts with callers and tests following and behaviour unchanged; rss-output.test.ts splits into rss/route.test.ts and rss/filtered/route.test.ts at the new mirror path. The auth mechanism is not part of that change — it is the whole of id-166, whose Goal, priority (low→medium) and Progress log now carry this directive.

Deferred

entities-metadata-bridge.test.ts and its collision partner entities/metadata/route.test.ts are deferred to the separate entities investigation already being scoped. remaining-routes.test.ts still needs splitting by route rather than relocating. It covers five routes across three domains, with clean seams: the guides section PATCH/DELETE block, the procurement template-completion download, and the three OAuth routes (decision, grants, revoke). Destinations are guides/[slug]/sections/[sectionId]/route.test.ts, procurement/[id]/templates/[templateId]/completions/[completionId]/download/route.test.ts, and oauth/{decision,grants,revoke}/route.test.ts. The file already draws its Supabase, request and role setup from __tests__/helpers/, so the only unavoidable duplication is the four vi.mock() declarations — roughly 20 lines per destination, irreducible because Vitest hoists vi.mock into the file it literally appears in. Each destination needs only the subset it uses; the layer-schemas mock is relevant to the guides split alone. The download block’s describe title still reads /api/bids/:id/... and should be renamed to procurement as part of the move. procurement-form-reanchor-guard.test.ts reads route files as text and imports no production module, so it has no mirror path. It moves to __tests__/guards/ with the other structural guards — see workstream 3.

3. Consolidate the validation and mcp trees — DONE (commits d0cb21125, 3211cfc54, 6c7c1f445)

__tests__/validation/ and __tests__/mcp/ are both retired; __tests__/guards/ exists. Full suite 8948 passed / 5 skipped; escalations 3 → 2, and the 2 that remain are only the deferred entities pair. Combined in-scope baseline of 40 files / 1313 tests survives assertion-for-assertion. One of the six “guards” was not a guard. url-normalisation-parity.test.ts imports normaliseUrl from @/lib/extraction/url-normalise and executes it — its own docblock says “this one executes behaviour through a shared fixture rather than reading sources”. Subject is a production module, so the mirror rule wins and it went to its mirror path, not guards/. The lesson recorded in §3.2: a guard may import a production module as a helper without that module being its subject — the assertions decide, never the filename. Two of the five that did move (corpus-manifest, eval-fixture-sync) import production modules exactly that way. The schemas.test.ts collision was three-way, not two. __tests__/validation/validation.test.ts reads as a test of the validation mechanism but imports @/lib/validation/schemas and nothing else, so it landed on the same path. (The api-tier file that invokes routes and asserts real 400s is a different file — __tests__/app/api/_cross-cutting/validation.test.ts — and is untouched; both are still kept.) All three cover disjoint export sets with zero shared assertions, so it was a union: 88 tests before, 88 after. The URL-normalisation parity/unit pair was also a union — the 25 fixture cases prove a superset of the 5 hand-written cases’ properties but on different inputs, so both sets stay (56 before, 56 after). A third merge hazard, now in §3.4: the parity file’s two vi.mock() calls were inert (vestigial from when normaliseUrl lived in content-extractor.ts). Carrying them into the merge would have made them live against the other block, because vi.mock() hoists into whatever file it lands in. Dropped, confirmed by execution. Open follow-ups recorded but deliberately not actioned: __tests__/docs/ holds four more source-scanning guards meeting the same test as the five that moved (out of brief scope, not a ruling that __tests__/docs/ is correct); four comments cite test files that no longer exist (where-are-we-exposed-tool, tool-annotations-coverage, schema-db-consistency, pipeline-parity) — repointing a dead reference would make it read as live, so whether that coverage should return is a requirement question.

Original plan for workstream 3

Both __tests__/validation/validation-sweep.test.ts and the api-tier validation.test.ts are kept — they are complementary, not duplicates. The sweep is a static source scan proving every route is wired to @/lib/validation; it never executes a route. The api-tier file invokes routes and asserts real 400 responses, proving the mechanism works. Deleting either loses real coverage. Under the single rule, the nine files in __tests__/validation/ that import @/lib/validation/* move to __tests__/lib/validation/. Both trees hold a schemas.test.ts covering different exports of lib/validation/schemas.ts; per the factoring rule this is a split/merge decision, not a suffix — review what each covers and land one file per module unless a genuine variant justifies an aspect. __tests__/guards/ is agreed. The source-scanning guards move there, since they test no export and so have no mirror path: validation-sweep, corpus-manifest, eval-fixture-sync and url-normalisation-parity from __tests__/validation/, mcp-fixture-sync from __tests__/mcp/, and procurement-form-reanchor-guard from the route tree. Update the §8 guard-test list in the same commit as the move, and note that docs/reference/testing/corpus-manifest.json and two route-file comments cite __tests__/validation/corpus-manifest.test.ts by path. __tests__/mcp/** imports @/lib/mcp/* throughout and moves to __tests__/lib/mcp/** by the same rule. Note the test-philosophy factory-consolidation item (24 near-duplicate createMockMcpServer() definitions, roughly 600 lines) lives in this tree; the move is independent of that consolidation and should not wait for it.

4. Boundary-test coverage gap

The boundary-test moves are absorbed into workstream 2. What remains here is a genuine coverage gap: 17 route directories have an error.tsx but only 10 have boundary tests. app/intelligence, app/intelligence/[workspaceId] and app/review have none. This is new test authoring, not a move, and is independent of the restructure.

5. Document — DONE; guard still outstanding

docs/reference/testing/test-philosophy.md §3 is rewritten around the single mirror rule. The prior row sending app/api/**/route.ts tests to __tests__/api/** is reversed, with the rationale recorded in §3.1 so the reversal is not a silent amendment. New subsections cover the directories that mirror nothing (§3.2), naming and the narrow dot-suffix exception (§3.3), the location-versus-factoring distinction (§3.4), and the codemod (§3.5). Last-verified bumped per §11. The location guard is still outstanding and should come after the 2b split/merge review, since it encodes those decisions. Under the single rule the location check is mechanical: strip the __tests__/ prefix and assert the file’s subject import sits under that path, allow-listing the infrastructure directories. Worth going further than the codemod does and failing on multi-subject route tests too — §3.4 exists because a file can be correctly located and still wrongly factored, and guides.test.ts shows a clean location report hiding an overlap. Register it in the §8 guard-test list.

Already completed

The redundant __tests__/api/refinement/refinement-page-and-routes.test.ts is deleted; the four per-route files in __tests__/api/refinement/touchpoints/id/ were confirmed a strict superset (they add viewer-role, 404 and 500 cases) and still pass, 17 tests across 4 files. Its unique raindrop.ai source grep was dropped without rehoming, as agreed. The lint failure at e2e/fixtures/test-data-fixture.ts:563 is fixed: kbSectionId and the workspaceIds line feeding it were dead after the ID-145 W1e change that deleted the procurement workspaces umbrella. Both are removed and the insert retained for its seeding side effect. Lint reports 0 errors; the 4 remaining warnings pre-date this work. The 56 .DS_Store files under __tests__ are deleted. They are written by Finder when a folder is browsed, are already gitignored, and cannot be disabled for local volumes. The two settings-sidebar test files are merged. They were not duplicates: the flat __tests__/components/settings-sidebar.test.tsx uniquely covered group visibility and labels, aria-current highlighting, the onSectionChange and /provenance click paths, the integrations/taxonomy/team params and SettingsMobileSidebar, while __tests__/components/settings/settings-sidebar.test.tsx uniquely covered nav entry counts, unknown-section fallback and the id-420 reviewer-assignments retirement. Both sets now live in the correctly located nested file (17 tests passing, 81 across the settings folder) and the flat file is removed. .github/workflows/identity-guard.yml is out of scope. It pins literal test paths under a shrink-only protocol and 10 of its 27 exclusions are already stale, but the guard is being retired, so moves in this plan need not update it.

Notes on scope

2b is done. Escalations went 16 → 3, and the three that remain are all out of 2b’s scope: the two entities files (deferred to the separate entities investigation) and procurement-form-reanchor-guard.test.ts (workstream 3’s __tests__/guards/ move). 1249 tests pass under __tests__/app/api, typecheck clean. Each group landed as its own commit, dispatched to parallel sub-agents in isolated worktrees and cherry-picked back. What the pass produced beyond the moves — all folded into test-philosophy.md §3.4:

• A third failure mode the codemod passes. A test file at a path where no production module exists: procurement/[id]/export/route.test.ts sat at a path whose route.ts had never existed. Deriving a plausible path is not the same as hitting a real one, and the guard in workstream 5 must check the target exists, not merely that it is derivable.
• A collision means “same route”, never “same behaviour”. Resolve by comparing assertions, not filenames. Both the guides and the review-history cases proved this in opposite directions — guides shared zero assertions (union), review-history was eight-of-nine genuine duplicates (de-duplication). Only reading the assertions distinguishes them.
• The inverse of the HTTP-method split is not a defect. One handler serving two query shapes (review/queue/route.ts pivots on ?publication_status=in_review) is one route and stays one file.
• Two merge hazards that pass on the day and fail later: load-bearing fixture shapes (nine columns are .optional() because sparse assertions exist), and per-file shared state crossing a threshold on merge (the queue route’s 20/min limiter landed exactly at the cap — green that day, 429 on the next case added). Workstream 3 is unblocked (the parallel session’s __tests__/validation/ edits have landed) and is largely mechanical once the schemas.test.ts factoring is settled. Workstream 4 is new test authoring and can be deferred indefinitely. Workstream 5’s guard is now the substantive remaining piece, and it should encode the three failure modes above rather than location alone.

Findings that outlived the restructure

• defineRoute response validation is blind to additive drift — measured by probe, not inferred. An undeclared key is neither rejected nor stripped (the wrapper returns the original response, so Zod’s default stripping never reaches the caller); a missing declared key throws even in test. Only 4 of 142 z.object() in lib/validation/schemas.ts are .strict(). Origin per mempalace: recorded at the time as defect B4 “blind-spec-execution — spec’d raw-payload but 178/193 return NextResponse, never verified vs corpus”, alongside a generator deliberately made permissive to protect an AC, producing “FALSE runtime confidence”. The pass-through/fail-open design is deliberate; the blind spot is the accident. One real instance was found and fixed (a DR-130-retired by_domain asserted as reaching the client, contradicting its own sibling test); the class is open.
• No workspace-scoped table has an RLS isolation predicate. All 11 tables carrying workspace_id are either USING (true) for authenticated or a facet check that only proves the parent row exists. Isolation is entirely handler-side. Bounded: anon has zero grants on all of them, so this is authenticated-to-authenticated, not public.