diff --git a/AGENTS.md b/AGENTS.md index 226d30d83c..a85304f186 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -166,6 +166,12 @@ When a branch or PR review completes, record it with `npm run ledger:append -- - - For non-trivial source/config/test changes, prefer `npm run verify:cheap` as the first broad gate and `npm run verify:pr-local` before PR handoff when the change is ready. The PR-local gate runs the full unit suite once, then conditionally adds the production build/client-bundle scan and RAG fixture/manifest validation. Browser, dependency-audit, Docker/Supabase replay, and provider-backed checks remain separate gates. Use `npm run verify:pr-local -- --dry-run --files ` to inspect selection without running commands. The broader `--extended` plan is dry-run only unless explicit approval is reflected by `ALLOW_EXTENDED_PR_LOCAL=true`. - Let the repository run coordinator control cross-worktree verification. It permits at most two focused Vitest/read-only typecheck leases from different worktrees; full Vitest, coverage, lint, build, Playwright, and live-provider tests remain exclusive. Do not install while a repository test, build, lint, typecheck, or server command is active. Avoid aggressive short-interval polling, and do not repeat an unchanged full gate after it passes. - For UI, frontend, browser, routing, styling, reduced-motion, or forced-colors changes, run `npm run ensure` before browser work and use `npm run verify:ui` as the Chromium UI gate. For phone-chrome changes, run `npm run verify:phone-chrome` first: it checks installed-lock parity, selects the affected browser/PWA owners and exact journeys, and adds `verify:ui` last only when shared chrome foundations make the broad gate necessary. Inspect uncertain scope with `-- --dry-run`. +- **Run `npm run format` and commit the result before every push.** `format:check` is in neither `npm run test`, `npm run typecheck`, nor `npm run lint`, so the ordinary loop reports green while `Static PR checks` and `ci/circleci: verify` both fail on `prettier --check .`. Three CI failures on 2026-07-30 came from exactly this. Two traps beyond simply running it: + - **Formatting without committing does nothing for the push.** A push sends commits, not your working tree, so formatting after committing leaves the unformatted blob on the branch. Amend or add a follow-up commit. + - **A per-file check is not the repository-wide check.** `prettier --check ` on the source file you edited passes while a doc or ledger edit in the same push fails; that was the missed file twice out of three. + + `.githooks/pre-push` carries the guard, and since 2026-07-30 it checks the pushed commit where CI checks it: `guard-push.mjs` puts the pushed SHA in a temporary `git worktree` with `node_modules` linked in and runs Prettier there, so neither the working tree's contents nor its prettier config can vouch for the commit, and a dynamic `prettier.config.*` still loads. A push that changes prettier policy (`.prettierrc*`, `.prettierignore`, `.editorconfig`, or a `package.json` carrying a `prettier` field) escalates to a whole-tree `prettier --check .`, because a policy change alters the verdict for files the push never touched. But `core.hooksPath` is set by this checkout's `npm install`, so an agent pushing from its own environment bypasses the hook entirely and only CI catches the break — which is why the rule above is still a rule. + - For release or handoff confidence, use `npm run verify:release`; this includes the full Playwright project set. - For clinical ingestion, answer generation, source governance, privacy, production-readiness, or environment changes, run the smallest relevant domain check plus `npm run check:production-readiness`. - For pull requests that touch ingestion, answer generation, search/ranking, source rendering, document access, privacy, production env, or clinical output, complete the clinical governance preflight in `.github/pull_request_template.md`. diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index c43671fecc..5828e8627c 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -1313,6 +1313,7 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-30 | claude/top-search-design-mockups-w53znc | 7577a1ea60ab5f0918885f90e849bbac754234b1 | PR #1394 search-results-band-adoption + #096/#115 | No P0/P1. Disposition1 partial: isAlwaysStandaloneShellPath fixes services/etc; /tools still layout-false-positive (P2). Disposition2 verified: import-as-rendered deferred as #115 (P3). #096 closure text accurate for root-path; row still open with stale Still-live clause. | vitest tests/search-results-band-adoption.test.ts 6/6; offline gutting repro tools vs services; static read search-route-ownership + outstanding-issues | | 2026-07-30 | PR #1394 / `claude/top-search-design-mockups-w53znc` | `8c39158d99876338613d5bb3195847fd253ef5ff` | CI/review closeout: /tools page-only roots + thread disposition | FIXED. Layout false-positive for `/tools` closed via `isStandaloneModeHomePath` in reachabilityRoots. Import-as-rendered finding left as `#115` (pre-existing; lint catches the plausible slip). Both Codex threads dispositioned. Merge clean vs main. | vitest adoption 6/6; full unit 4451 passed / 4 skipped; typecheck; prettier; Bugbot pr-bugbot | | 2026-07-30 | PR #1394 / `claude/top-search-design-mockups-w53znc` | `d658f120253ac0a2dbfe1c9b1a14c6a0744af14d` | CI/review closeout tip (supersedes 8c39158d row) | FIXED layout `/tools` false-positive; `#115` deferred with human disposition; threads resolved; merge-tree clean; CI settling | vitest adoption 6/6; full unit 4451/4; typecheck; Bugbot | +| 2026-07-30 | PR #1394 / `claude/top-search-design-mockups-w53znc` | `4a001efadedea6e8f8ad59ac7374ff9293cf7e14` | CI/review closeout after format + main sync | FIXED. Tip `66c5eb2c` failed Static PR / CircleCI solely on prettier padding in `#096` row; fixed on `61314887`. Main synced via `4a001efa` (shallow-clone inventory refusal from #1392). No open review threads; layout/`/tools` false-positive already fixed; `#115` remains deferred. No product-code change this pass. | format:check pass; Static PR pass; Unit coverage pass; PR required pass; CircleCI pass; vitest adoption 6/6; typecheck; Bugbot no open P0/P1; merge-tree clean vs main | | 2026-07-28 | claude/pr-policy-caveman-fix-6875bb | ebd169358d1d1c9c596a486c12d51101cb5860ee | PR policy caveman-mode carve-out (docs-only) | Opened via reopened PR #1322; merged origin/main (real conflict in handoff SKILL.md resolved by hand, both step-6/step-7 edits kept); squash-auto-merge enabling | check:pr-policy self-test+workflow-guard PASS; prettier --check PASS; lint PASS 0 warnings; typecheck/test/rag:fixtures not run locally (cross-worktree heavy-lock contention, docs-only diff, deferred to hosted CI); no provider-backed checks | | 2026-07-30 | claude/test-coverage-analysis-2vcd8a | 4f498b66a56b2a7eddde6c841a79621f23b59cc7 | PR #1398 babysit | BLOCKER CLEARED: CONFLICTING due to docs/outstanding-issues.md vs main (#115 band-adoption follow-up). Kept main #115 + next-id=116; preserved PR #109 single-branch/refspec update. Prior tip had no GitHub CI suite (only PR Policy/CircleCI) — push retriggered full CI. 0 review threads; 0 Bugbot findings. | verify:cheap PASS (432 files, 4467 passed \| 4 skipped); repo-hygiene 38/38; sweep:branch-ledger --no-fetch exit 0; format:changed PASS; Bugbot none; hosted CI re-triggered on tip | | 2026-07-30 | PR #1394 / `claude/top-search-design-mockups-w53znc` | `0d47141fc030684299dcb265e3d853c93b9e2a91` | CI/review closeout — merged | MERGED as squash `0d47141f`. Prior tip `4a001efa` had required CI green after prettier fix `61314887` (Static PR/CircleCI red on `#096` padding) and main sync. Layout/`/tools` false-positive fixed; `#115` deferred; review threads resolved. Post-merge ledger-only follow-up. | hosted Static/Unit/PR-required/CircleCI pass on pre-merge tip; vitest adoption 6/6; typecheck; Bugbot no open P0/P1; merge-tree clean | diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index c695effaa8..d4b4aeb997 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -81,7 +81,7 @@ removed after current-main verification; it is not missing recommended work. | 33 | `#103` | A3 | Operator — Supabase schema | Same window as `#102` | 30–60 minutes | Confirm whether the wide `document_table_facts` trigram index from `20260714190000` exists live, then either mirror it into `schema.sql` (retained) or drop it via a forward migration (redundant). **Not the allowlist** — it suppresses live-vs-`schema.sql` findings only and cannot make the migration chain and the mirror agree. Stop: do not drop it without live scan evidence. | | 34 | `#105` | Optional | High — browser/UI verification | When the heavy-run lock is free | 20–40 minutes | Run `verify:ui` over the ten `LoadingPanel` fallbacks and confirm the Supabase `preconnect` reaches `` on a live page. Implementation already shipped; this row is the outstanding verification only. | - + ## Open items @@ -152,7 +152,8 @@ removed after current-main verification; it is not missing recommended work. | #112 | P2 | issue | `issues:next-id` has no concurrency protection | **Outcome:** two agents working the same hour cannot allocate the same ledger id. **Detail:** the marker at the top of this file is a plain HTML comment read-modify-written by whoever edits next, with no lock and no post-merge check. On 2026-07-29 it collided **twice in one hour**: PR #1391 claimed `#096`/`#097`, lost them, claimed `#098`/`#099`, lost those too, and its final pair `#108`/`#109` collided a third time with the branch-cleanup work that reached `main` first — resolved during the #1391 merge by renumbering to `#110`/`#111` and bumping the marker to 112. Each collision is silent: `docs/outstanding-issues.md` has NO union merge driver (unlike `docs/branch-review-ledger.md`), so it presents as an ordinary content conflict that a hurried resolution can settle by dropping one side's rows entirely. Nothing checks for duplicate ids afterwards. **Next:** add a duplicate-id and marker-consistency check to the verification gates — the cheapest useful form is a test asserting every `#NNN` id appears exactly once across both tables and that the marker exceeds the max, which turns a silent loss into a red gate. Consider`merge=union`in`.gitattributes` as well, though row-level union does not by itself prevent two rows sharing an id. **Stop:** do not resolve one of these conflicts by taking one side wholesale without diffing the id sets first; that is how rows get dropped. | session 2026-07-29 PR sweep; PR #1391 conflict resolution; `.gitattributes` | 2026-07-29 | | #113 | P2 | issue | `ModeNav` clips its labels at every phone width on `main` | **Outcome:** the mode navigation bar honours its own "labels are never abbreviated" contract, or does not render. **Detail:** measured in Chromium against the running app on 2026-07-29 (PR #1390 review, landed as `8f861bb0`), `span.truncate` `scrollWidth` vs `clientWidth` on `/therapy-compass/search`: 320px viewport clips `Compare` to **15 of 56px**; 360px → 29px; 390px → 39px; 412px → 46px; **430px is the worst case at 3 of 4 labels clipped** (`Compare` 17px, `Recommend` 57 of 77, `Pathways` 57 of 60) because crossing the 26rem band adds a fourth slot to the same space; clean only from ~35rem. Root cause is `grid-auto-columns: 1fr` (`globals.css:2373`) in both lower bands — equal tracks make the WIDEST slot set what every slot needs — combined with `truncate`, so the shortfall is silent. The CSS block's stated budgets are wrong: it claims four labels need 394px, measured intrinsic widths are Search 92.4 / Compare 144.3 / Recommend 125.6 / Pathways 108.5 = **471px**, short by roughly the `0/4` badge plus its gap. **Next:** pick one — (a) content-size every band (adopt the ≥34rem `display: flex` shape throughout) and move thresholds to ~21rem for three slots and ~31rem for four; (b) drop the count badge below the top band, worth ~38px; (c) raise the thresholds so phones keep the collapsed control. (a) is the smallest change and deletes a layout mode. Add the band-boundary browser assertion review asked for, covering the widest label in the set. **Stop:** a hardcoded `rem` threshold cannot guarantee fit for an arbitrary item list, and `ModeNav` is shared — do not treat a Therapy-tuned number as a general solution. | PR #1390 thread `PRRT_kwDOSh5Fis6UyB5X` (open at merge); measured session 2026-07-29 | 2026-07-29 | | #114 | P2 | issue | The live Web-Vitals instrument cannot measure its own noise | **Outcome:** the `#017` baseline rests on evidence whose reproducibility can be checked. **Detail:** `live-web-vitals.yml` (landed `8dbfc5d1`) runs Lighthouse **once** per route/strategy. `#017` asks for reproducible evidence and says to stop when the evidence is too noisy — one sample can neither establish reproducibility nor recognise noise, so the instrument cannot detect the condition its own governing item tells the operator to stop on. The rule is a hard threshold (LCP < 2500ms), so a route near the line resolves to a pass or a breach on run-to-run variance alone, invisibly, and a favourable sample would mark **seven** gated findings WONTFIX. Related and also unclosed: Chrome ships with the `ubuntu-24.04` runner image and is NOT pinned by `LIGHTHOUSE_VERSION`, so a metric shift between a baseline and its follow-up can originate in the browser; the build is now recorded per report in `summary.json` (`chromeVersions`) so a cross-version comparison is visibly invalid, but nothing prevents one. **Next:** N runs per route/strategy with a sample-indexed report name, `expectedRuns`/`incompleteEvidence` reworked to expect N per cell, median as the graded aggregate (the Lighthouse and `lighthouse-ci` convention), and — most important — a spread that straddles a threshold treated as INCOMPLETE EVIDENCE rather than resolved either way. Costs N× dispatch time. Pinning Chrome needs a container or a setup action. **Stop:** do not record an `#017` verdict from a single-sample run, and do not compare baselines whose `chromeVersions` differ. | PR #1385 thread `PRRT_kwDOSh5Fis6U0Zq0` (open at merge); `scripts/summarise-web-vitals.mjs` | 2026-07-29 | -| #115 | P3 | rec | Band adoption gate treats a discovered import as rendered | **Outcome:** the gate fails when a route keeps its results import but stops rendering it. **Detail:** `tests/search-results-band-adoption.test.ts` walks imports and reports adoption if any reachable module contains a band element; it does not track whether the imported binding is used in rendered JSX. Reducing `(search-app)/services/page.tsx` to `
` while retaining its imports keeps the gate green. This is **pre-existing** — the previous two-hop walker had the same flaw — and not a live risk, because `npm run lint` fails the same edit with five `@typescript-eslint/no-unused-vars` warnings under `--max-warnings 0`, so the composite static gate does catch it. The genuinely uncaught shape is an import referenced somewhere non-rendering (a type position, or `void Binding;`), which is a deliberate act rather than a plausible slip. **Next:** if tightened, track which imported bindings appear in JSX element position or as the default export — a re-export like `export default Child` is a real mount with no JSX — and follow `dynamic()` bindings the same way; `@babel/parser` is already used by `tests/route-reachability.test.ts`. **Stop:** do not add a fixture page under `src/app` to prove it — `tests/codebase-index-coverage.test.ts` also walks that tree and Vitest runs files in parallel, so a materialising route can be observed mid-run and left behind on failure. | PR #1394 review; session 2026-07-30 | 2026-07-30 | +| #115 | P3 | rec | Band adoption gate treats a discovered import as rendered | **Closed 2026-07-30.** `tests/search-results-band-adoption.test.ts` no longer asks "does this file mention the band?" but "does anything the route actually mounts reach it?". It parses each module with `@babel/parser` into a small graph — exported name to local declaration, local to the identifiers its body references, and which locals render the band — then walks from the route's default export, carrying at each hop the set of exports the importer mounts. So a static `import { X }` is followed only when `X` is reachable from a mounted declaration; `dynamic(() => import("…").then((m) => m.Named))` follows only that binding, which is how the code-split dashboard workspaces are written; a bare `import "…"` is not followed at all; `export { X } from "…"` is followed only when the importer wants `X`; and `export * from "…"` never supplies a default, so a page whose importer wants only the default gets no hop from it. **Why the redesign rather than more patches:** six false greens were reported in one day (unrendered import, `export { X }`, `export { X } from`, `export *`, bare side-effect import, JSX in an unmounted helper, and a lazy import reaching every sibling export), all one defect — presence is not reach. Two of the six were introduced by an earlier patch to the same walker. **Verified:** all five production search routes still reach the band; gutting `(search-app)/services/page.tsx` and `tools/page.tsx` to `
` each reports an orphan; fourteen temp-dir fixtures cover both directions, and the two guarding the new mechanisms were confirmed to fail against the prior behaviour by targeted mutation (presence-based band check, and following bare imports). Residual: reachability is per module, so a mounted declaration referencing an identifier anywhere in its body counts, and control flow inside it is not modelled. | PR #1400; session 2026-07-30 | 2026-07-30 | +| #116 | P2 | issue | An unmergeable PR runs no CI at all, with no signal that it stopped | **Outcome:** a PR that has silently stopped being tested says so. **Detail:** on 2026-07-30 PR #1400 ran **no** `CI`, `Gitleaks` or `Semgrep` workflow across three consecutive pushes, and nothing anywhere said why. Cause: the PR had a real content conflict with `main`, so GitHub could not build `refs/pull/1400/merge`, and every `pull_request`-triggered workflow is skipped in that state. `pull_request_target` ones (`PR Policy`) still ran, and CircleCI posted failure ~3s after each push - faster than a checkout, so no step executed. The symptom reads as "CI is broken" or "my tests fail", and about 40 minutes went into replicating CI steps locally (all green) before the cause was found. **Diagnostic:** compare check-run counts with another open PR (3 vs 16-19), list workflow runs and note only `pull_request_target` fired, then confirm with `git merge-tree --write-tree origin/main HEAD`. **Next:** make staleness visible instead of silent - e.g. a `pull_request_target` job that fails when `mergeable_state` is `dirty`, so a conflicted PR shows one red check naming the conflict rather than an empty check list. `npm run sync:pr-branches` covers the behind-but-clean case only. **Stop:** never conclude "CI is failing" from a missing check - the absence of a check is not a failing check. | PR #1400; session 2026-07-30 | 2026-07-30 | ## Resolved / archive diff --git a/scripts/guard-push.mjs b/scripts/guard-push.mjs index 432f4925b1..ae224ba95d 100644 --- a/scripts/guard-push.mjs +++ b/scripts/guard-push.mjs @@ -15,8 +15,12 @@ * * 2. Format-before-push * verify:cheap does NOT run format:check but CI requires it, so unformatted - * files reach CI and fail there. Runs `prettier --check` on the files in the - * push range. Override: SKIP_FORMAT_GUARD=1. + * files reach CI and fail there. Reproduces what CI sees: the pushed blobs + * *and* the pushed prettier config are materialised into a scratch tree and + * checked there. Neither half can come from the working copy — formatting + * after committing, or correcting a committed config without committing the + * correction, both left the guard green and CI red. + * Override: SKIP_FORMAT_GUARD=1. * * 3. Drift-manifest freshness * Editing supabase/schema.sql without regenerating supabase/drift-manifest.json @@ -29,8 +33,9 @@ */ import { execFileSync } from "node:child_process"; import { createHash } from "node:crypto"; -import { existsSync, readFileSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, rmSync, symlinkSync } from "node:fs"; import { createRequire } from "node:module"; +import { tmpdir } from "node:os"; import path from "node:path"; import { pathToFileURL } from "node:url"; @@ -108,6 +113,25 @@ function collectChangedFiles(ranges) { return [...files]; } +/** + * Changed files paired with the commit they are being pushed at. + * + * A push sends commits, not the working tree. Checking `` on disk lets a + * formatted working copy vouch for an unformatted committed blob: commit + * `const a = 1`, run `npm run format`, and `prettier --check ` passes + * while `git show HEAD:` is still unformatted — so the guard went green and + * CI failed anyway. Carry the sha so the guard can read what is actually pushed. + */ +function collectChangedBlobs(ranges) { + const seen = new Map(); + for (const range of ranges) { + for (const file of changedFilesForRange(range)) { + seen.set(`${range.localSha}:${file}`, { sha: range.localSha, file }); + } + } + return [...seen.values()]; +} + // --------------------------------------------------------------------------- // Guard 1: auto-merge race sentinel // --------------------------------------------------------------------------- @@ -174,40 +198,216 @@ function resolvePrettierBin() { return path.join(path.dirname(pkgJson), "bin", "prettier.cjs"); } -function formatGuard(changedFiles) { - if (process.env.SKIP_FORMAT_GUARD === "1") { - return { name: "format", ok: true, skipped: "SKIP_FORMAT_GUARD=1" }; - } - const existing = changedFiles.filter((f) => existsSync(f)); - if (existing.length === 0) return { name: "format", ok: true }; - let prettierBin; +/** + * Does this path decide Prettier's verdict for files other than itself? + * + * Matched on the basename so a nested config counts too. When one of these + * changes, the verdict for *unchanged* files can change with it — a `tabWidth` + * edit can make existing source fail CI's repository-wide `prettier --check .` + * while a changed-paths-only check passes — so a policy change escalates to a + * whole-tree check. + */ +/** Does `:` parse as JSON carrying a top-level `prettier` field? */ +function carriesPrettierField(ref, file) { + const contents = tryGit(["show", `${ref}:${file}`]); + if (contents === undefined) return false; // absent at this ref (or no such ref) try { - prettierBin = resolvePrettierBin(); + return JSON.parse(contents).prettier !== undefined; } catch { - return { name: "format", ok: true, note: "prettier not resolvable — format check skipped" }; + // Unparseable: assume it is policy rather than assume it is not. + return true; + } +} + +/** + * Does this path decide Prettier's verdict for files other than itself? + * + * A package.json counts only when it actually carries a `prettier` field — + * matching every package.json would send each routine dependency bump through a + * whole-tree check it cannot possibly need. **Both endpoints are inspected**, not + * just the pushed one: adding a field and removing one each re-decide the verdict + * for untouched files, and reading only the pushed side misses the removal (drop + * `tabWidth: 4` and four-space-formatted source starts failing CI). + */ +function isPrettierPolicyFile(file, sha) { + const base = path.basename(file); + if (/^(?:\.prettierrc(?:\..+)?|prettier\.config\.(?:js|cjs|mjs|ts)|\.prettierignore|\.editorconfig)$/.test(base)) { + return true; + } + if (base !== "package.json") return false; + return carriesPrettierField(sha, file) || carriesPrettierField(`${sha}^`, file); +} + +/** + * Check the pushed commit the way CI does: in a real checkout of it. + * + * A worktree is what makes the verdict trustworthy, and it is cheap (<1s here). + * Every earlier attempt leaked working-tree state into the answer: + * - checking `` on disk let a formatted working copy vouch for an + * unformatted committed blob (a push sends commits, not the working tree) + * - piping committed blobs through stdin still resolved `.prettierrc` from disk, + * so a committed-broken/tree-corrected config passed here and failed CI + * - hand-staging config files could not evaluate a dynamic `prettier.config.mjs` + * at all, because it may import plugins from `node_modules` + * A checkout with `node_modules` linked in has none of those gaps. + */ +function checkPushedCommit(prettierBin, sha, files) { + tryGit(["worktree", "prune"]); // clear any worktree a crashed run left behind + const dir = mkdtempSync(path.join(tmpdir(), "guard-push-format-")); + rmSync(dir, { recursive: true, force: true }); // `git worktree add` wants a fresh path + try { + execFileSync("git", ["worktree", "add", "--detach", "--quiet", dir, sha], { + stdio: ["ignore", "ignore", "pipe"], + }); + } catch (error) { + // Fail CLOSED. Being unable to check is not evidence that the push is clean, + // and SKIP_FORMAT_GUARD=1 is the escape hatch when this is genuinely stuck. + const detail = (error?.stderr ? error.stderr.toString() : "").trim(); + return { + verdict: "error", + detail: `could not check out ${sha} to verify formatting${detail ? `: ${detail}` : ""}`, + }; + } + try { + // The Prettier doing the checking is this checkout's, not the pushed + // lockfile's. That only matters when the push changes dependencies — then a + // version difference can make CI disagree with this verdict, so say so rather + // than answer confidently with the wrong Prettier. + if (files.some((file) => ["package.json", "package-lock.json"].includes(path.basename(file)))) { + const mismatch = prettierVersionMismatch(prettierBin, dir); + if (mismatch) return { verdict: "error", detail: mismatch }; + } + // A dynamic config may import plugins; without this it cannot load at all. + const modules = path.resolve("node_modules"); + if (existsSync(modules)) { + try { + symlinkSync(modules, path.join(dir, "node_modules"), "dir"); + } catch { + // Already present, or symlinks unavailable — prettier still loads static configs. + } + } + const policyChanged = files.some((file) => isPrettierPolicyFile(file, sha)); + // Deleted paths are gone from the checkout, and prettier errors on a missing + // argument, so ask only for what is actually there. + const present = files.filter((file) => existsSync(path.join(dir, file))); + if (!policyChanged && present.length === 0) return { verdict: "formatted" }; + // A policy change alters the verdict for files this push never touched, so + // check the whole tree — exactly what CI's `prettier --check .` does. + const batches = policyChanged ? [["."]] : chunk(present, 200); + for (const batch of batches) { + const result = runPrettierCheck(prettierBin, dir, batch); + if (result.verdict !== "formatted") return result; + } + return { verdict: "formatted" }; + } finally { + tryGit(["worktree", "remove", "--force", dir]); + rmSync(dir, { recursive: true, force: true }); } +} + +function chunk(items, size) { + const out = []; + for (let index = 0; index < items.length; index += size) out.push(items.slice(index, index + size)); + return out; +} + +/** + * Does the pushed lockfile pin a different Prettier than the one installed? + * + * The guard formats with this checkout's Prettier while CI installs the pushed + * lockfile, so a push that bumps Prettier itself would be judged by the wrong + * version. Returns a message when they disagree, and null when they agree or when + * either version cannot be read — an unknown must not manufacture a block. + */ +function prettierVersionMismatch(prettierBin, checkoutDir) { + const readJson = (file) => { + try { + return JSON.parse(readFileSync(file, "utf8")); + } catch { + return undefined; + } + }; + const installed = readJson(path.join(path.dirname(path.dirname(prettierBin)), "package.json"))?.version; + const pinned = readJson(path.join(checkoutDir, "package-lock.json"))?.packages?.["node_modules/prettier"]?.version; + if (!installed || !pinned || installed === pinned) return null; + return ( + `node_modules has prettier ${installed}, but the pushed lockfile pins ${pinned}, so this ` + + `check would not match CI. Run \`npm ci\` and push again.` + ); +} + +/** Prettier: 0 clean, 1 unformatted, anything else a real failure. */ +function runPrettierCheck(prettierBin, cwd, args) { try { - // prettier respects .prettierignore for listed paths; --ignore-unknown skips - // files it has no parser for (e.g. images) without failing. - execFileSync(process.execPath, [prettierBin, "--check", "--ignore-unknown", ...existing], { + execFileSync(process.execPath, [prettierBin, "--check", "--ignore-unknown", ...args], { + cwd, stdio: ["ignore", "pipe", "pipe"], }); - return { name: "format", ok: true }; + return { verdict: "formatted" }; } catch (error) { - const detail = [error.stdout, error.stderr] - .map((b) => (b ? b.toString() : "")) + const detail = [error?.stdout, error?.stderr] + .map((buffer) => (buffer ? buffer.toString() : "")) .join("") .trim(); + // An invalid config option also exits 1 under --check, reported as + // `[error] Invalid tabWidth value…`, so the status alone cannot tell a + // formatting difference from a broken config. + if (error?.status === 1 && !detail.includes("[error]")) return { verdict: "unformatted", detail }; + // A malformed or unloadable config in the push fails CI's `prettier --check .` + // for the same reason. Treating it as "unknown, allow" is how the guard would + // wave through the break it exists to catch. + return { verdict: "error", detail: detail || `prettier exited ${error?.status ?? "non-zero"}` }; + } +} + +function formatGuard(changedBlobs) { + if (process.env.SKIP_FORMAT_GUARD === "1") { + return { name: "format", ok: true, skipped: "SKIP_FORMAT_GUARD=1" }; + } + if (changedBlobs.length === 0) return { name: "format", ok: true }; + let prettierBin; + try { + prettierBin = resolvePrettierBin(); + } catch { + return { name: "format", ok: true, note: "prettier not resolvable — format check skipped" }; + } + + const bySha = new Map(); + for (const { sha, file } of changedBlobs) { + if (!bySha.has(sha)) bySha.set(sha, []); + bySha.get(sha).push(file); + } + + const unformatted = []; + const errors = []; + for (const [sha, files] of bySha) { + const { verdict, detail } = checkPushedCommit(prettierBin, sha, files); + if (verdict === "unformatted") unformatted.push(detail); + else if (verdict === "error") errors.push(detail); + } + + if (errors.length > 0) { return { name: "format", ok: false, message: - `Prettier found unformatted files in this push (CI format:check would fail):\n` + - (detail ? `${detail}\n` : "") + - ` Fix with: npm run format\n` + + `Prettier could not check this push, so CI's \`prettier --check .\` will fail too:\n` + + errors.map((detail) => `${detail}\n`).join("") + + ` A malformed prettier config in the push is the usual cause.\n` + ` To push anyway: SKIP_FORMAT_GUARD=1 git push`, }; } + if (unformatted.length === 0) return { name: "format", ok: true }; + return { + name: "format", + ok: false, + message: + `Prettier found unformatted files in this push (CI format:check would fail):\n` + + unformatted.map((detail) => `${detail}\n`).join("") + + ` This is a checkout of the pushed commit, not your working copy — run\n` + + ` \`npm run format\` and commit the result.\n` + + ` To push anyway: SKIP_FORMAT_GUARD=1 git push`, + }; } // --------------------------------------------------------------------------- @@ -273,7 +473,8 @@ function main() { if (ranges.length === 0) process.exit(0); // deletion-only push or nothing to do const branch = currentBranch(); const changedFiles = collectChangedFiles(ranges); - const results = [autoMergeGuard(branch), formatGuard(changedFiles), driftGuard(changedFiles)]; + // formatGuard reads the pushed blobs; driftGuard only needs the paths. + const results = [autoMergeGuard(branch), formatGuard(collectChangedBlobs(ranges)), driftGuard(changedFiles)]; process.exit(report(results)); } diff --git a/tests/search-results-band-adoption.test.ts b/tests/search-results-band-adoption.test.ts index 0edffec318..e69a2f3c3c 100644 --- a/tests/search-results-band-adoption.test.ts +++ b/tests/search-results-band-adoption.test.ts @@ -2,6 +2,7 @@ import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSy import { tmpdir } from "node:os"; import path from "node:path"; +import { parse } from "@babel/parser"; import { describe, expect, it } from "vitest"; import { appModeDefinitions } from "@/lib/app-modes"; @@ -157,11 +158,350 @@ function reachabilityRoots(routeAbs: string): string[] { return [routeAbs, ...layouts]; } -/** `from "x"` and `import("x")`. The lazy form is load-bearing: the dashboard - code-splits its mode workspaces through `dynamic(() => import(...))` in - `clinical-dashboard-lazy.tsx`, so a static-only walk cannot see the band - behind Differentials, Favourites or the prescribing workspace. */ -const IMPORT_SPECIFIER = /(?:from\s*"([^"]+)")|(?:import\(\s*"([^"]+)"\s*\))/g; +/** + * A module reduced to what decides whether it can render the band. + * + * Presence is not reach. Five separate false greens on this gate were all the + * same defect — the walk asked "does this file mention it?" when the question is + * "does anything the route actually mounts reach it?". Every spelling that got + * patched individually (an unrendered import, `export { X }`, `export { X } from`, + * `export *`, a bare side-effect import, JSX inside an unmounted helper) is a + * reachability question, so this models reachability once instead. + */ +type ModuleGraph = { + /** exported name (including "default") -> the local declarations behind it */ + exportedLocals: Map; + /** local declaration -> identifiers its body references, JSX element names included */ + localRefs: Map>; + /** locals whose own body renders the band */ + bandLocals: Set; + /** `import { imported as local } from source` */ + imports: Array<{ source: string; local: string; imported: string }>; + /** `export { imported as exported } from source`; `exported: "*"` for `export *` */ + reexports: Array<{ source: string; exported: string; imported: string }>; + /** `import(source)[.then(m => m.imported)]`, attributed to the local that owns it */ + lazyImports: Array<{ owner: string; source: string; imported: string }>; +}; + +/** Which exports of a module the importer actually mounts. `"*"` means any. */ +type MountRoots = Set | "*"; + +/** + * AST keys whose subtrees are type-only, so identifiers inside them are erased at + * runtime and must not count as component references. + */ +const TYPE_POSITION_KEYS = new Set([ + "typeAnnotation", + "returnType", + "typeParameters", + "typeArguments", + "superTypeParameters", + "implements", +]); + +/** `export default () => …` has no name to hang references off. */ +const ANONYMOUS_DEFAULT = "__default__"; + +function declaredNames(node: Record | undefined): string[] { + if (!node) return []; + if (node.type === "FunctionDeclaration" || node.type === "ClassDeclaration") { + const name = (node.id as { name?: string })?.name; + return name ? [name] : []; + } + if (node.type === "VariableDeclaration") { + return ((node.declarations ?? []) as Array>) + .map((declarator) => (declarator.id as { name?: string })?.name) + .filter((name): name is string => typeof name === "string"); + } + return []; +} + +/** + * Which export a `.then(…)` selects out of a dynamic import. + * + * Two shapes, both live in this repo: + * - `.then((m) => m.Foo)` -> "Foo" (every entry in clinical-dashboard-lazy.tsx) + * - `.then((mod) => ({ default: mod.Foo }))` -> "Foo" — the Next.js wrapper for a + * named export, used by `global-search-shell.tsx` for `ClinicalDashboard`. Left + * unhandled this returned null and fell back to following *every* export of that + * module, which is precisely the over-approximation the walk is meant to avoid. + */ +function thenExportName(args: unknown): string | null { + const first = ((args ?? []) as Array>)[0]; + const body = first?.body as Record | undefined; + if (body?.type === "MemberExpression") { + const property = body.property as { name?: string } | undefined; + return typeof property?.name === "string" ? property.name : null; + } + if (body?.type === "ObjectExpression") { + for (const property of (body.properties ?? []) as Array>) { + if ((property.key as { name?: string })?.name !== "default") continue; + const value = property.value as Record | undefined; + if (value?.type === "MemberExpression") { + const selected = value.property as { name?: string } | undefined; + return typeof selected?.name === "string" ? selected.name : null; + } + } + } + return null; +} + +/** + * A dynamic import whose result is thrown away cannot mount anything. + * + * `void import("x")` in an effect, or a bare `import("x");` statement, is a + * preload or a side effect. Scoped to discarded *results* rather than to "must be + * lexically inside dynamic()", because this repo also writes the loader as a + * separate binding — `const load = () => import("…").then(…)` passed to + * `dynamic(load)` — and requiring lexical containment would report that as + * unreachable. + */ +function discardsItsResult(node: Record): boolean { + if (node.type === "UnaryExpression" && node.operator === "void") return true; + if (node.type !== "ExpressionStatement") return false; + let expression = node.expression as Record | undefined; + // Unwrap a `.then(…)`/`.catch(…)` chain back to its root. + while (expression?.type === "CallExpression") { + const callee = expression.callee as Record | undefined; + if (callee?.type !== "MemberExpression") break; + expression = callee.object as Record | undefined; + } + return Boolean(expression && dynamicImportSource(expression)); +} + +function dynamicImportSource(node: Record | undefined): string | null { + if (!node) return null; + const isImport = + node.type === "ImportExpression" || + (node.type === "CallExpression" && (node.callee as { type?: string })?.type === "Import"); + if (!isImport) return null; + const arg = (node.source ?? (node.arguments as unknown[])?.[0] ?? null) as Record | null; + return arg?.type === "StringLiteral" && typeof arg.value === "string" ? arg.value : null; +} + +function buildModuleGraph(source: string, filename: string): ModuleGraph { + let ast: ReturnType; + try { + ast = parse(source, { sourceType: "unambiguous", plugins: ["jsx", "typescript"] }); + } catch { + // An unparseable file must not silently drop out of the walk. + throw new Error(`search-results-band-adoption: could not parse ${filename}`); + } + + const graph: ModuleGraph = { + exportedLocals: new Map(), + localRefs: new Map(), + bandLocals: new Set(), + imports: [], + reexports: [], + lazyImports: [], + }; + const addExport = (exported: string, local: string) => { + graph.exportedLocals.set(exported, [...(graph.exportedLocals.get(exported) ?? []), local]); + }; + + /** Record everything one local declaration's body reaches. */ + const scan = (root: unknown, owner: string) => { + const refs = graph.localRefs.get(owner) ?? new Set(); + graph.localRefs.set(owner, refs); + const visit = (value: unknown, discarded = false) => { + if (!value || typeof value !== "object") return; + if (Array.isArray(value)) { + for (const item of value) visit(item, discarded); + return; + } + const node = value as Record; + const type = node.type; + const resultDropped = discarded || discardsItsResult(node); + if (type === "JSXOpeningElement") { + let name = node.name as Record | undefined; + while (name && name.type === "JSXMemberExpression") name = name.object as Record; + if (name && name.type === "JSXIdentifier" && typeof name.name === "string") { + refs.add(name.name); + if (name.name === BAND_IDENTIFIER) graph.bandLocals.add(owner); + } + } + if (type === "Identifier" && typeof node.name === "string") refs.add(node.name); + // `dynamic(() => import("x").then(m => m.Foo))` names the binding it mounts, + // which is how every lazy workspace in clinical-dashboard-lazy.tsx is written. + // Matched on the outer `.then(…)` so the inner bare-import branch below can + // skip it and not also enqueue the module's default. + if (type === "CallExpression" && !resultDropped) { + const callee = node.callee as Record | undefined; + if (callee?.type === "MemberExpression" && (callee.property as { name?: string })?.name === "then") { + const inner = dynamicImportSource(callee.object as Record | undefined); + if (inner) { + graph.lazyImports.push({ owner, source: inner, imported: thenExportName(node.arguments) ?? "*" }); + } + } + } + const bare = resultDropped ? null : dynamicImportSource(node); + if (bare && !graph.lazyImports.some((entry) => entry.owner === owner && entry.source === bare)) { + // A plain `import("x")` in `dynamic()` mounts the module's default export. + graph.lazyImports.push({ owner, source: bare, imported: "default" }); + } + for (const key of Object.keys(node)) { + if (key === "loc" || key === "leadingComments" || key === "trailingComments") continue; + // Type positions are erased, so an identifier that appears only there is + // not a runtime reference. `ComponentProps` needs the value + // import but never mounts it, and this repo does not enforce + // `consistent-type-imports`, so such an import is not necessarily written + // as `import type`. Value subtrees never live under these keys. + if (TYPE_POSITION_KEYS.has(key)) continue; + visit(node[key], resultDropped); + } + }; + visit(root); + }; + + /** Attribute a declaration's references to each name it declares. */ + const scanDeclaration = (statement: Record) => { + if (statement.type === "VariableDeclaration") { + for (const declarator of (statement.declarations ?? []) as Array>) { + const name = (declarator.id as { name?: string })?.name; + if (name) scan(declarator, name); + } + return; + } + for (const name of declaredNames(statement)) scan(statement, name); + }; + + for (const raw of (ast.program?.body ?? []) as unknown[] as Array>) { + const type = raw.type; + + if (type === "ImportDeclaration") { + const specifier = (raw.source as { value?: string })?.value; + if (typeof specifier !== "string") continue; + // `import type { X }` is erased at runtime and cannot mount anything, so it + // is not an edge. Both spellings matter: the declaration-level `importKind` + // and the per-specifier one from `import { type X }`. + if (raw.importKind === "type") continue; + // A bare `import "x"` executes the module but renders nothing, so it is not + // a mount and is deliberately not followed. + for (const spec of (raw.specifiers ?? []) as Array>) { + if (spec.importKind === "type") continue; + const local = (spec.local as { name?: string })?.name; + if (!local) continue; + const imported = + spec.type === "ImportDefaultSpecifier" + ? "default" + : spec.type === "ImportNamespaceSpecifier" + ? "*" + : ((spec.imported as { name?: string })?.name ?? local); + graph.imports.push({ source: specifier, local, imported }); + } + continue; + } + + if (type === "ExportAllDeclaration") { + const specifier = (raw.source as { value?: string })?.value; + if (typeof specifier === "string") graph.reexports.push({ source: specifier, exported: "*", imported: "*" }); + continue; + } + + if (type === "ExportNamedDeclaration") { + const specifier = (raw.source as { value?: string })?.value; + if (typeof specifier === "string") { + // `export type { X } from "…"` is erased too. + if (raw.exportKind === "type") continue; + for (const spec of (raw.specifiers ?? []) as Array>) { + if (spec.exportKind === "type") continue; + const exported = (spec.exported as { name?: string })?.name; + const imported = (spec.local as { name?: string })?.name ?? exported; + if (exported && imported) graph.reexports.push({ source: specifier, exported, imported }); + } + continue; + } + const declaration = raw.declaration as Record | undefined; + if (declaration) { + for (const name of declaredNames(declaration)) addExport(name, name); + scanDeclaration(declaration); + continue; + } + for (const spec of (raw.specifiers ?? []) as Array>) { + const exported = (spec.exported as { name?: string })?.name; + const local = (spec.local as { name?: string })?.name; + if (exported && local) addExport(exported, local); + } + continue; + } + + if (type === "ExportDefaultDeclaration") { + const declaration = raw.declaration as Record | undefined; + if (declaration?.type === "Identifier" && typeof declaration.name === "string") { + addExport("default", declaration.name); + continue; + } + const owner = declaredNames(declaration)[0] ?? ANONYMOUS_DEFAULT; + addExport("default", owner); + if (declaration) scan(declaration, owner); + continue; + } + + scanDeclaration(raw); + } + + return graph; +} + +/** Locals reachable from the exports the importer mounts. */ +function reachableLocals(graph: ModuleGraph, roots: MountRoots): Set { + const queue: string[] = + roots === "*" + ? [...graph.exportedLocals.values()].flat() + : [...roots].flatMap((name) => graph.exportedLocals.get(name) ?? []); + const reached = new Set(); + while (queue.length > 0) { + const local = queue.pop() as string; + if (reached.has(local)) continue; + reached.add(local); + for (const ref of graph.localRefs.get(local) ?? []) if (!reached.has(ref)) queue.push(ref); + } + return reached; +} + +/** + * Modules the mounted code actually reaches, and which of their exports it wants. + * + * Re-exports are followed by their own semantics rather than by a special case: + * `export { X as default } from "…"` supplies a default and is followed when the + * importer wants the default; `export { X } from "…"` only when it wants `X`; and + * `export * from "…"` never supplies a default, so a page whose importer wants + * only the default gets no hop from it. + */ +function nextHops(graph: ModuleGraph, reached: Set, roots: MountRoots) { + const hops = new Map(); + const want = (source: string, imported: string | string[]) => { + const existing = hops.get(source); + if (existing === "*") return; + if (imported === "*") { + hops.set(source, "*"); + return; + } + const names = Array.isArray(imported) ? imported : [imported]; + hops.set(source, new Set([...(existing ?? []), ...names])); + }; + + for (const { source, local, imported } of graph.imports) if (reached.has(local)) want(source, imported); + for (const { owner, source, imported } of graph.lazyImports) if (reached.has(owner)) want(source, imported); + for (const { source, exported, imported } of graph.reexports) { + if (exported === "*") { + // `export *` re-exports named exports only, never the default. + if (roots === "*") want(source, "*"); + else { + const named = [...roots].filter((name) => name !== "default"); + if (named.length > 0) want(source, named); + } + continue; + } + if (roots === "*" || roots.has(exported)) want(source, imported); + } + return [...hops.entries()].map(([source, wanted]) => ({ source, roots: wanted })); +} + +function rootsKey(roots: MountRoots): string { + return roots === "*" ? "*" : [...roots].sort().join(","); +} function resolveSpecifier(specifier: string, fromFile: string): string | null { let base: string; @@ -179,32 +519,48 @@ function resolveSpecifier(specifier: string, fromFile: string): string | null { * real chain on the root dashboard route is four — * `layout → shared-search-app-shell → global-search-shell → ClinicalDashboard → * document-search-results` — so a fixed hop count silently under-reported - * reachability rather than failing loudly. + * reachability rather than failing loudly. The lazy indirection through + * `clinical-dashboard-lazy` adds a hop, so the cap has headroom over the longest + * real chain rather than sitting on it. */ -const MAX_IMPORT_DEPTH = 8; +const MAX_IMPORT_DEPTH = 10; +/** + * Breadth-first from what a route actually mounts to a band it actually renders. + * + * Each hop carries the exports the importer wants, so the same module can be + * visited twice for different bindings — that is the point. `clinical-dashboard-lazy` + * lazily exports every mode workspace, and reaching it for `FavouritesHub` must + * not also reach `document-search-results`. + */ function routeReachesBand(routeAbs: string): boolean { - const roots = reachabilityRoots(routeAbs); - const seen = new Set(roots); - let frontier = roots.map((file) => ({ file, depth: 0 })); + // Next renders a route's (and a layout's) default export. + let frontier = reachabilityRoots(routeAbs).map((file) => ({ + file, + depth: 0, + roots: new Set(["default"]) as MountRoots, + })); + const seen = new Set(frontier.map(({ file, roots }) => `${file}::${rootsKey(roots)}`)); while (frontier.length > 0) { - const next: Array<{ file: string; depth: number }> = []; - for (const { file, depth } of frontier) { + const next: typeof frontier = []; + for (const { file, depth, roots } of frontier) { let source: string; try { source = readFileSync(file, "utf8"); } catch { continue; } - if (rendersBand(source)) return true; + const graph = buildModuleGraph(source, file); + const reached = reachableLocals(graph, roots); + for (const local of graph.bandLocals) if (reached.has(local)) return true; if (depth >= MAX_IMPORT_DEPTH) continue; - for (const match of source.matchAll(IMPORT_SPECIFIER)) { - const specifier = match[1] ?? match[2]; - if (!specifier) continue; - const target = resolveSpecifier(specifier, file); - if (!target || seen.has(target) || isMockupPath(target)) continue; - seen.add(target); - next.push({ file: target, depth: depth + 1 }); + for (const hop of nextHops(graph, reached, roots)) { + const target = resolveSpecifier(hop.source, file); + if (!target || isMockupPath(target)) continue; + const key = `${target}::${rootsKey(hop.roots)}`; + if (seen.has(key)) continue; + seen.add(key); + next.push({ file: target, depth: depth + 1, roots: hop.roots }); } } frontier = next; @@ -381,8 +737,9 @@ describe("band adoption detection", () => { ); expect(routeReachesBand(path.join(dir, "orphan-route.tsx"))).toBe(false); - // Same shape, but the child mounts the band — and via a lazy import, which - // is how the dashboard code-splits its mode workspaces. + // Same shape, but the child mounts the band — and via a lazy import, in the + // exact form the dashboard code-splits its mode workspaces with: + // `dynamic(() => import("…").then((m) => m.Named))`. writeFileSync( path.join(dir, "banded-child.tsx"), `export function Banded() { return <${BAND_IDENTIFIER} modeId="documents" />; }\n`, @@ -390,10 +747,218 @@ describe("band adoption detection", () => { ); writeFileSync( path.join(dir, "wired-route.tsx"), - 'const Lazy = () => import("./banded-child");\nexport default Lazy;\n', + 'const Lazy = dynamic(() => import("./banded-child").then((m) => m.Banded));\nexport default Lazy;\n', "utf8", ); expect(routeReachesBand(path.join(dir, "wired-route.tsx"))).toBe(true); + + // A bare `dynamic(() => import("…"))` mounts the module's default, so this + // one reaches the band and the named-export module above would not have. + writeFileSync( + path.join(dir, "default-banded-child.tsx"), + `export default function Banded() { return <${BAND_IDENTIFIER} modeId="documents" />; }\n`, + "utf8", + ); + writeFileSync( + path.join(dir, "lazy-default-route.tsx"), + 'const Lazy = dynamic(() => import("./default-banded-child"));\nexport default Lazy;\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "lazy-default-route.tsx"))).toBe(true); + + // The gap this redesign closes: one lazy module exporting several + // workspaces. Mounting the non-band one must not reach the band one. + writeFileSync( + path.join(dir, "lazy-barrel.tsx"), + 'export const Plain = dynamic(() => import("./plain-child").then((m) => m.Plain));\n' + + 'export const Banded = dynamic(() => import("./banded-child").then((m) => m.Banded));\n', + "utf8", + ); + writeFileSync(path.join(dir, "plain-child.tsx"), "export function Plain() { return
; }\n", "utf8"); + writeFileSync( + path.join(dir, "mounts-plain-only-route.tsx"), + 'import { Plain } from "./lazy-barrel";\nexport default function Page() {\n return ;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "mounts-plain-only-route.tsx"))).toBe(false); + + // ...and mounting the band one still does reach it. + writeFileSync( + path.join(dir, "mounts-banded-route.tsx"), + 'import { Banded } from "./lazy-barrel";\nexport default function Page() {\n return ;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "mounts-banded-route.tsx"))).toBe(true); + + // The band rendered directly inside an exported helper of a module the route + // *does* reach, but never mounts. This is the case the reachability-scoped + // band check owns: hop filtering cannot catch it, because the module is + // legitimately reached for its other export. + writeFileSync( + path.join(dir, "mixed-exports.tsx"), + "export function Plain() {\n return
;\n}\n" + + `export function Helper() {\n return <${BAND_IDENTIFIER} modeId="documents" />;\n}\n`, + "utf8", + ); + writeFileSync( + path.join(dir, "mounts-plain-export-route.tsx"), + 'import { Plain } from "./mixed-exports";\nexport default function Page() {\n return ;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "mounts-plain-export-route.tsx"))).toBe(false); + + // Same module, mounting the export that does render it. + writeFileSync( + path.join(dir, "mounts-helper-export-route.tsx"), + 'import { Helper } from "./mixed-exports";\nexport default function Page() {\n return ;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "mounts-helper-export-route.tsx"))).toBe(true); + + // JSX inside an exported helper the route never renders is not a mount. + writeFileSync( + path.join(dir, "unmounted-helper-route.tsx"), + 'import { Banded } from "./banded-child";\n' + + "export function Helper() {\n return ;\n}\n" + + "export default function Page() {\n return
;\n}\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "unmounted-helper-route.tsx"))).toBe(false); + + // A bare side-effect import executes the module but renders nothing. + writeFileSync( + path.join(dir, "side-effect-route.tsx"), + 'import "./banded-child";\nexport default function Page() {\n return
;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "side-effect-route.tsx"))).toBe(false); + + // The Next.js object-wrapper shape, which global-search-shell.tsx uses for + // ClinicalDashboard. Selecting the plain export must not drag in the sibling + // that renders the band; before this was handled the wrapper returned no name + // and the walk fell back to following every export. + writeFileSync( + path.join(dir, "wrapper-plain-route.tsx"), + 'const Lazy = dynamic(() => import("./lazy-barrel").then((mod) => ({ default: mod.Plain })));\n' + + "export default Lazy;\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "wrapper-plain-route.tsx"))).toBe(false); + + // Same wrapper selecting the banded export does reach it. + writeFileSync( + path.join(dir, "wrapper-banded-route.tsx"), + 'const Lazy = dynamic(() => import("./mixed-exports").then((mod) => ({ default: mod.Helper })));\n' + + "export default Lazy;\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "wrapper-banded-route.tsx"))).toBe(true); + + // A dynamic import whose result is discarded is a preload, not a mount. + writeFileSync( + path.join(dir, "preload-route.tsx"), + "export default function Page() {\n" + + ' useEffect(() => {\n void import("./default-banded-child");\n }, []);\n' + + " return
;\n}\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "preload-route.tsx"))).toBe(false); + + // A type-only import is erased at runtime, so it cannot mount the band even + // though the identifier appears in the mounted component's annotations. + writeFileSync( + path.join(dir, "type-only-route.tsx"), + 'import type { Banded } from "./banded-child";\n' + + "export default function Page({ render }: { render?: typeof Banded }) {\n" + + " void render;\n return
;\n}\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "type-only-route.tsx"))).toBe(false); + + // A *value* import used only in a type position is erased too. This repo does + // not enforce `consistent-type-imports`, so this is not necessarily spelled + // `import type` — and `ComponentProps` genuinely needs the value + // import while never mounting it. + // The annotation must be inline on the scanned declaration: a top-level + // `type Props = …` alias is never scanned at all, so it would pass for the + // wrong reason and prove nothing. + writeFileSync( + path.join(dir, "type-position-route.tsx"), + 'import { Banded } from "./banded-child";\n' + + "export default function Page(props: { render?: typeof Banded }) {\n" + + " void props;\n return
;\n}\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "type-position-route.tsx"))).toBe(false); + + // A non-exported local the mounted component does render is reachable. + writeFileSync( + path.join(dir, "indirect-route.tsx"), + 'import { Banded } from "./banded-child";\n' + + "function Inner() {\n return ;\n}\n" + + "export default function Page() {\n return ;\n}\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "indirect-route.tsx"))).toBe(true); + + // The regression this gate exists to catch: the results component is still + // imported, but nothing renders it. A walk that follows every specifier + // reports adoption here, which is how gutting a real page to `
` with + // its imports intact stayed green. + writeFileSync( + path.join(dir, "imported-only-route.tsx"), + 'import { Banded } from "./banded-child";\nexport default function Page() {\n return
;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "imported-only-route.tsx"))).toBe(false); + + // A wrapper page mounts with no JSX of its own, so a default re-export of + // an imported binding has to count. + writeFileSync( + path.join(dir, "reexport-route.tsx"), + 'import { Banded } from "./banded-child";\nexport default Banded;\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "reexport-route.tsx"))).toBe(true); + + // A *named* re-export is not a mount. Re-exporting `Banded` renders + // nothing, so a page whose own default renders `
` must still be + // reported as an orphan — the false green Codex found on #1400. + writeFileSync( + path.join(dir, "named-reexport-route.tsx"), + 'import { Banded } from "./banded-child";\n' + + "export { Banded };\n" + + "export default function Page() {\n return
;\n}\n", + "utf8", + ); + expect(routeReachesBand(path.join(dir, "named-reexport-route.tsx"))).toBe(false); + + // Same hole in one statement rather than two. `export { Banded } from "…"` + // took the re-export path and was followed unconditionally. + writeFileSync( + path.join(dir, "direct-named-reexport-route.tsx"), + 'export { Banded } from "./banded-child";\nexport default function Page() {\n return
;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "direct-named-reexport-route.tsx"))).toBe(false); + + // And `export * from "…"`, which re-exports the band component by name + // without mounting it either. + writeFileSync( + path.join(dir, "star-reexport-route.tsx"), + 'export * from "./banded-child";\nexport default function Page() {\n return
;\n}\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "star-reexport-route.tsx"))).toBe(false); + + // But a default re-export still is a mount: this page's default component + // *is* the banded one. + writeFileSync( + path.join(dir, "default-reexport-route.tsx"), + 'export { Banded as default } from "./banded-child";\n', + "utf8", + ); + expect(routeReachesBand(path.join(dir, "default-reexport-route.tsx"))).toBe(true); } finally { rmSync(dir, { recursive: true, force: true }); }