diff --git a/.changeset/phi-scan-refuse-an-unobserved-scan-root.md b/.changeset/phi-scan-refuse-an-unobserved-scan-root.md new file mode 100644 index 0000000..0808a0e --- /dev/null +++ b/.changeset/phi-scan-refuse-an-unobserved-scan-root.md @@ -0,0 +1,22 @@ +--- +"@cosyte/cli": patch +--- + +The PHI scanner refuses (exit 2) when a declared scan root was never observed, instead of printing +`OK, no hits` and exiting 0 over a corpus it never opened. + +In all-mode (`pnpm phi-scan` with no arguments, which is what CI runs) each root's walk is now +reconciled against `git ls-files`, and two independent conditions refuse: the root contributed +nothing, or git tracks an in-scope file under the root that the walk did not open. Six states +previously reported clean, all measured on this repository: the root missing, the root emptied, the +root a dangling symbolic link, the root a live symbolic link to an outside directory, one tracked +fixture removed from the working tree, and the source root moved away. The dangling case is the one +no kind check could reach, because `existsSync` follows the link and answers false before anything +about the entry is inspected. + +Also fixed: a present-but-unreadable `phi-scan-overrides.md` threw past every handler and exited 1, +which is this scanner's code for "hits found". It now exits 2 with a diagnostic, matching the +allow-list reader beside it. + +Scoped to the all-mode sweep. `--staged` is a diff rather than a corpus and is unchanged, and so is +the behaviour of naming paths explicitly. diff --git a/CHANGELOG.md b/CHANGELOG.md index cc763eb..eac0a9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,49 @@ still do. Each entry was assigned to the release whose tag first contains it, re ### Fixed +- **`pnpm phi-scan` printed `OK, no hits` and exited 0 over a corpus it never opened + (PHI-SCAN-OBSERVED-NOTHING-IS-GLOBAL).** A declared scan root that the walk never observed is now + a refusal at **exit 2**, in the all-mode sweep CI runs. Each root's walk is reconciled against + `git ls-files`, and two independent conditions refuse: the root contributed **nothing**, or git + tracks an in-scope file under the root that the walk did not open. + - **Six states measured on this repository, every one of them previously exiting 0 with + `OK, no hits`:** the root missing; the root emptied; the root a **dangling** symbolic link; the + root a **live** symbolic link to a directory outside the repository; a single tracked fixture + removed from the working tree with the rest of the root still opened; and `src` moved away. + - **The dangling case is why a kind check cannot stand in for this rule.** `existsSync` follows + the link and answers false, so `walk()` returns before `readdirSync` and the existing + not-a-regular-file refusal never fires. Nothing about the entry is ever inspected, so no check + on its kind can reach it. Refusing on what was **observed** needs no opinion about the entry. + - **A denominator is deliberately not what this is.** A count counts the roots and the files that + did exist, so a healthy-looking total is exactly what a starved root produces. This scanner + prints no file count and one was not added. + - **Existence is not observation, which is why both conditions ship.** Refusing only a missing + root leaves the emptied one open; refusing only an empty result leaves the swapped one open, + because a root pointed at another directory opens plenty. Neither subsumes the other. + - **Exit 2 was derived from this scanner's own contract, not ported from a sibling.** `1` means + "hits found" here; `walk()` already raises an unreadable root as an invocation failure, and a + root replaced by a regular file already exits 2 through `readdirSync`. Sibling scanners disagree + on this code, and carrying one across would have been the defect. + - **`git ls-files` failing refuses rather than answering the empty set**, because an empty answer + is indistinguishable from "this root tracks nothing" and would switch the rule off in silence. + - **The rule is one-directional on purpose.** A tracked in-scope file the walk missed refuses; an + untracked working-tree file the walk found does not, because scanning more than git carries is + the safe direction. + - **Scope, stated rather than left to be inferred:** all-mode only. `--staged` is a diff and not a + corpus, and widening it would change what a **commit** is blocked on, which is a separate + decision. Naming paths explicitly is unchanged. + - **Narrowed, not closed, and still disclosed in the module header:** a scan root that is itself a + live link is still followed. It is now refused whenever git tracks an in-scope file under the + root that the link's target does not also carry **at the same relative path**, so a link to an + unrelated directory refuses; the reconciliation compares path **sets** rather than the bytes git + carries at those paths, so a target mirroring the tracked names still passes at exit 0, and a + root git tracks nothing under is the degenerate case of that rather than the whole of it. An + **ancestor** of a scan root remains out of the staged route's scope, and paths mode still follows + a link a caller names. +- **A present-but-unreadable `phi-scan-overrides.md` exited 1, the code reserved for "hits found".** + `loadOverrideLog` threw a raw filesystem error past every handler while its sibling reader, + `loadAllowList`, had already been wrapped. It now exits 2 with a diagnostic. A caller branching on + the exit code read a broken invocation as a PHI finding. - **A red pre-publish gate showed a red X and merged anyway, because `ci / prepublish` was a required check nowhere (CI-REQUIRED-CHECKS).** The shared pipeline this repo calls grew a `prepublish` job on 2026-08-05 (`cosyte/.github#35`, `6142ac4`; its second layer defaulted on in diff --git a/CLAUDE.md b/CLAUDE.md index 1250be1..4754294 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,14 +160,25 @@ Why: [agent-notes § The pre-commit PHI gate and git mv](documentation/agent-not path is printed deliberately and every offender is named. **That guarantee is about a REFUSAL and does not extend to a hit.** - **Exit `1` means "hits found", so a broken invocation must never exit `1`.** An unreadable - allow-list or scan root is **exit 2**; both used to throw past every handler and read as a finding. + allow-list, **override log** or scan root is **exit 2**; all three threw past every handler and + read as a finding. +- **A declared scan root the all-mode walk never OBSERVED refuses at exit 2**, reconciled per root + against `git ls-files`: it opened nothing, or git tracks an in-scope file it did not open. **Both + conditions ship, neither subsumes the other** (an emptied root opens nothing, a swapped one opens + plenty). **The dangling link is why a kind check cannot stand in**: `existsSync` follows it and + answers false, so `walk()` returns before `readdirSync` and nothing is ever inspected. **Never add + a denominator instead** (a count counts the roots that DID exist). **Exit 2 is derived from this + contract, never ported from a sibling.** `git ls-files` failing REFUSES, never the empty set. + **One-directional**: an untracked file the walk found is not a refusal. **All-mode only**; + widening `--staged` changes what a COMMIT is blocked on. - **Never state the refusal rule unqualified.** It is scoped to an **enumerated** entry; a refuter falsified "neither route follows such an entry" using this very file. -- **Three escapes are PRE-EXISTING, measured, and deliberately NOT closed** (a scan root that is - itself a live link is followed by the all-mode walk and prints values under a fabricated in-repo - path; an **ancestor** of a scan root is in neither route's scope; paths mode follows a named link). - **Do not "fix" them inside an unrelated slice**: closing them needs a - refuse-a-scan-that-observed-nothing rule plus a decision about how far above a root to look. +- **Three escapes remain, NARROWED not closed** (a scan root that is itself a live link is still + followed, and **never say it survives "only where git tracks nothing"**: the reconciliation + compares PATH SETS, so a target mirroring the tracked NAMES passes at exit 0, decoy bytes and all, + measured; an **ancestor** of a scan root is in neither route's scope; paths mode follows a named + link). **Do not "fix" them inside an unrelated slice**: what is left needs a decision about how far + ABOVE a root to look. - **Other residuals, also not closed:** `D` and `U` are unenumerated (`U` costs nothing that can reach a commit: `git commit` refuses an unmerged index, exit 128); under `src/` the staged route covers `.ts` only while the all-mode walk covers every non-`.md` file, and the CI sweep is the cover. diff --git a/documentation/agent-notes.md b/documentation/agent-notes.md index 3a7895c..d81390d 100644 --- a/documentation/agent-notes.md +++ b/documentation/agent-notes.md @@ -16,8 +16,8 @@ relocation target. **Read the bound off `REPO_CLAUDE` in `.claude/hooks/doc-budg number written down here.** The mechanism is a per-repo ratchet that is LOWERED as relocations land, and the first number quoted for it went stale within a day, which is the same defect this whole audit exists to fix. That amendment's own warning is the one that binds here: -*"These files are where the traps that cost a defect to learn are written down. Relocate the -narrative; keep the cursor, the rule, and every trap."* +_"These files are where the traps that cost a defect to learn are written down. Relocate the +narrative; keep the cursor, the rule, and every trap."_ --- @@ -137,6 +137,83 @@ exactly as it stood, including its leading `- ` bullet. if git had stopped emitting the record shape the whole change is about. **Assert the premise, not only the remedy.** +### Refusing a scan root the walk never observed (2026-08-07) + +The measurement first, because the class this belongs to is one where a phrase sweep reads as +authoritative while measuring nothing, and every number below was re-derived here rather than carried +from a sibling. + +Every count below is **as of `cd221a0`**, the base this was measured on, and the anchor is not +decoration: this slice adds files of its own, so a bare "122 tracked" goes stale in the commit that +ships it, and the neither-route and `PID|` counts move with it (this very section is one of the files +that moves them, since the paragraph below contains the literal it counts). A refuter caught exactly +that. Re-derive against a named sha or do not write the number down. + +- **122 tracked files. The all-mode walk opens 34** (7 under `test/__fixtures__`, 27 under `src/`). + **88 are scanned by NEITHER route**, and **6 of those carry an inline `PID|` literal**: five HL7 + v2 messages built as `.ts` string literals inside `test/*.test.ts`, plus one `"PID|secret"` fed to + a locator function to prove it does not echo. All six were read by hand and are placeholder shapes + (`DOE^JANE`, `X^^^H^MR`, `123^^^HOSP`); none is a real identifier. **That 88 is an ENUMERATION gap + and it is a different item** (`PHI-SCAN-WALK-ROOT-SCOPE`): this change widened no root, so it + neither opened nor needed to open any of them. +- **The recogniser was NOT widened, and the measurement is why.** This slice adds no newly opened + file in the healthy state, so there is no new document shape for a detector to miss. The floor is + still SSN plus non-test email, still disclosed as a floor in the module banner, and the structured + field-level detector the banner demands is still owed. + +**What was open, all six measured on this tree, every one of them printing `OK, no hits` and exiting +0 beforehand:** the fixture root missing; the fixture root emptied; the fixture root a **dangling** +symbolic link; the fixture root a **live** symbolic link to a directory outside the repository; one +tracked fixture removed from the working tree with the rest of the root still opened; and `src` moved +away. **9 of `test/scripts/phi-scan.test.ts`'s 47 tests run red against `cd221a0`'s scanner** and +green against this one. **Name the denominator and re-derive it, because this very number went stale +inside the paragraph correcting a stale number**: it read `8` of `45` one commit earlier, and adding +the unmerged-count test moved it. The 38 that stay green are the floor tests, the controls, and the +ones that PIN a residual, which are green on both trees by design. + +**The remedy is reconciliation, not counting.** Each root's walk is compared against +`git ls-files -z -- `, and two independent conditions refuse: the root contributed nothing, or +git tracks an in-scope file under the root that the walk did not open. + +- **Neither condition subsumes the other, which is why both ship.** An emptied root opens nothing; a + root swapped for another directory opens plenty. Refusing only a missing root would leave the + emptied half open, because **existence is not observation**. +- **The dangling link is why a kind check cannot stand in.** `existsSync` FOLLOWS the link and + answers false, so `walk()` returns before `readdirSync` and the not-a-regular-file refusal above + never fires. Nothing about the entry is ever inspected, so no check on its kind can reach it. +- **A denominator was deliberately not added.** A count counts the roots and files that DID exist, so + a healthy-looking total is precisely what a starved root produces. This scanner prints no file + count and one was not introduced. +- **Exit 2 was derived from this scanner's own contract and not ported.** `1` means "hits found" + here; `walk()` already raises an unreadable root as an invocation failure, and a root replaced by a + regular file already exits 2 through `readdirSync`'s `ENOTDIR` (re-measured, not assumed). Sibling + scanners disagree on this code and carrying one across would have been the defect. +- **`git ls-files` failing REFUSES rather than answering the empty set**, because an empty answer is + indistinguishable from "this root tracks nothing" and would switch the whole rule off in silence. +- **One-directional on purpose:** a tracked in-scope file the walk missed refuses; an untracked + working-tree file the walk found does not. Scanning more than git carries is the safe direction, + and refusing it would red the gate on every fixture written but not yet added. +- **All-mode only.** `--staged` is a diff and not a corpus, and widening it changes what a COMMIT is + blocked on, which is a separate decision that two siblings declined deliberately. + +**Two of the three disclosed escapes are narrowed, not closed, and the disclosure says so.** A scan +root that is itself a live link is still followed. It is refused whenever git tracks an in-scope file +under the root that the link's target does not also carry **at the same relative path**, so a link to +an unrelated directory refuses here. **Do NOT shorten that to "survives only where git tracks nothing +under it": a refuter falsified exactly that sentence in one run**, and it had been written on three +surfaces at once including this one. The reconciliation compares **path sets**, not the bytes git +carries at those paths, so a target directory mirroring this repo's own seven tracked fixture NAMES +passes at exit 0 with decoy contents; a root tracking nothing is the degenerate case of that, not the +whole of it. An **ancestor** of a scan root is still out of the staged route's scope; the all-mode +half of it is covered incidentally, because replacing `test` leaves `test/__fixtures__` unopenable. +Paths mode is untouched. **How far ABOVE a root to look is still undecided.** + +**Also fixed, and re-derived rather than inherited:** the two `PRE-EXISTING` minors a sibling named +were measured **NOT open here** (`loadAllowList` and `readdirSync` were already wrapped to exit 2, and +an unmerged `U` entry is already pinned as out of scope and unable to reach a commit). Their sibling +reader **was** open: a present-but-unreadable `phi-scan-overrides.md` threw a raw `EACCES` past every +handler and node exited **1**, this contract's code for "hits found". Now exit 2. + ### The em-dash brand gate - **Em-dash brand gate armed, and unlike most siblings this repo was NOT clean when it landed.** @@ -460,13 +537,13 @@ open at the time of this write, one of them being the PR performing it; of the o **three** were stranded by it, and the first draft of this paragraph named the wrong set by reading `mergeStateStatus` instead of the check runs: -| PR | head sha | state before the write | stranded by this write? | -| --- | ---------- | ------------------------------------------------------- | ----------------------- | -| #33 | `f69ab63a` | six older required contexts green | **yes** | -| #18 | `73758565` | six older required contexts green | **yes** | -| #16 | `6cc21d8a` | six older required contexts green | **yes** | -| #29 | `95510b9d` | `ci / verify` **red on both matrix legs** | no, already unmergeable | -| #15 | `b63cd115` | no `no-emdash`, no `no-internal-refs` (predates both) | no, already stranded | +| PR | head sha | state before the write | stranded by this write? | +| --- | ---------- | ----------------------------------------------------- | ----------------------- | +| #33 | `f69ab63a` | six older required contexts green | **yes** | +| #18 | `73758565` | six older required contexts green | **yes** | +| #16 | `6cc21d8a` | six older required contexts green | **yes** | +| #29 | `95510b9d` | `ci / verify` **red on both matrix legs** | no, already unmergeable | +| #15 | `b63cd115` | no `no-emdash`, no `no-internal-refs` (predates both) | no, already stranded | All three affected PRs are Dependabot's, and Dependabot regenerates its branches, so **nothing was pushed to them**: a push onto a branch this slice does not own, to clear a condition this slice @@ -511,7 +588,7 @@ scans **tracked files** and was green throughout, both in the pre-commit hook an body is a surface that exists only on GitHub, reached only by `no-emdash.yml`'s `edited` trigger, and **no local run of anything in this repo could have caught it**. So the two halves of that gate are not redundant: the tracked-file half is the one a worker exercises constantly and the PR-text half is the -one that catches what a worker writes *about* the work. +one that catches what a worker writes _about_ the work. **Read alongside the standing note that the PR body lands under none of the three merge methods.** That is still true, and the gate scans it anyway as deliberate over-strictness. This is what that @@ -646,41 +723,41 @@ first-time-contributor approval gate nor whether `codeql / analyze` can report o The long-form half of standing discipline 4. The rule itself, and the founder directive it comes from, stay in `CLAUDE.md`. - **Four surfaces, three different answers.** `/** */` doc comments compile into `dist/*.d.ts` and - `dist/*.d.cts` and render on hover, so they are **gated**, and in this repo they were by far the - largest violating surface. String literals reach a consumer as terminal diagnostic text, so they - are **gated too**: this package printed an internal work item inside `CLI_NOT_IMPLEMENTED` and an - ADR number inside `CLI_PARSER_UNAVAILABLE` before that pass existed. `//` and plain `/* */` - comments are **not gated** and identifiers are **welcome** in them, because **the convention says - source comments are a place identifiers belong**. That is the whole reason. **Do not justify that - boundary from what reaches `dist/`**: two drafts of the `ncpdp` copy tried, a refuter proved both - false, and two drafts of this paragraph made the same mistake again. Measured on this tree, at - `06abc86`: `dist` is `files[0]`, there is no `.npmignore`, and **24 of the 27** tracked `src/` - files appear whole in a build map's `sourcesContent` (`src/index.ts`, `src/core/result.ts` and - `src/mcp/index.ts` contribute only re-exports and types, so the bundler erases them). Two - sentences that read well and are **false**, so do not write them: "everything in `src/` ships", - and "the bundles carry `//` comments verbatim" (measured: of the 43 whole-line `//` comments in - tracked `src/*.ts`, exactly **one** survives into any emitted `.mjs`/`.cjs`). **The boundary rests - on the convention, not on either fact.** The line is not what a consumer receives; it is what a - consumer is **shown**. - - **This repo is where the `WORD-N` trap is widest**, because the CLI wraps all eight formats and its - pages reach for every one of their vocabularies at once. `CLI-6` is ours; `HL7-V2`, `FHIR-R4`, - `DICOM-SR`, `NCPDP-SCRIPT`, `X12-837P`, `CCDA-R2.1`, `MSH-2`, `NM1-03`, `ST-01`, `439-E4` and - `ICD-10-CM` are reference material a consumer came here for. Never re-key a rule on the `WORD-N` - shape; the negative self-tests exist to make that attempt red. - - **Two remediation rules that matter more here than anywhere else.** (1) **Repair the head**: a - sentence with an identifier stripped off the front reads worse than the text it replaced. (2) - **CUT, do not rewrite.** This package's whole posture is honesty about what it _cannot_ do: gated - stubs that exit `69` and never fake a scrub, value-free stderr, the per-(format, operation) - `OP_SUPPORT` matrix. Softening a stated limit into an implied capability while tidying a sentence - is a worse defect than the bookkeeping being removed. Delete the claim rather than replace it, and - revert a rewrite verbatim rather than repair it. - - **What the gate cannot do:** it catches identifiers, not English sentences about our process, and - it reads `src/`, never `dist/` (untracked build output it cannot see without building). A new - programme prefix has to be added by hand. So the reviewer still owns half the rule. +**Four surfaces, three different answers.** `/** */` doc comments compile into `dist/*.d.ts` and +`dist/*.d.cts` and render on hover, so they are **gated**, and in this repo they were by far the +largest violating surface. String literals reach a consumer as terminal diagnostic text, so they +are **gated too**: this package printed an internal work item inside `CLI_NOT_IMPLEMENTED` and an +ADR number inside `CLI_PARSER_UNAVAILABLE` before that pass existed. `//` and plain `/* */` +comments are **not gated** and identifiers are **welcome** in them, because **the convention says +source comments are a place identifiers belong**. That is the whole reason. **Do not justify that +boundary from what reaches `dist/`**: two drafts of the `ncpdp` copy tried, a refuter proved both +false, and two drafts of this paragraph made the same mistake again. Measured on this tree, at +`06abc86`: `dist` is `files[0]`, there is no `.npmignore`, and **24 of the 27** tracked `src/` +files appear whole in a build map's `sourcesContent` (`src/index.ts`, `src/core/result.ts` and +`src/mcp/index.ts` contribute only re-exports and types, so the bundler erases them). Two +sentences that read well and are **false**, so do not write them: "everything in `src/` ships", +and "the bundles carry `//` comments verbatim" (measured: of the 43 whole-line `//` comments in +tracked `src/*.ts`, exactly **one** survives into any emitted `.mjs`/`.cjs`). **The boundary rests +on the convention, not on either fact.** The line is not what a consumer receives; it is what a +consumer is **shown**. + +**This repo is where the `WORD-N` trap is widest**, because the CLI wraps all eight formats and its +pages reach for every one of their vocabularies at once. `CLI-6` is ours; `HL7-V2`, `FHIR-R4`, +`DICOM-SR`, `NCPDP-SCRIPT`, `X12-837P`, `CCDA-R2.1`, `MSH-2`, `NM1-03`, `ST-01`, `439-E4` and +`ICD-10-CM` are reference material a consumer came here for. Never re-key a rule on the `WORD-N` +shape; the negative self-tests exist to make that attempt red. + +**Two remediation rules that matter more here than anywhere else.** (1) **Repair the head**: a +sentence with an identifier stripped off the front reads worse than the text it replaced. (2) +**CUT, do not rewrite.** This package's whole posture is honesty about what it _cannot_ do: gated +stubs that exit `69` and never fake a scrub, value-free stderr, the per-(format, operation) +`OP_SUPPORT` matrix. Softening a stated limit into an implied capability while tidying a sentence +is a worse defect than the bookkeeping being removed. Delete the claim rather than replace it, and +revert a rewrite verbatim rather than repair it. + +**What the gate cannot do:** it catches identifiers, not English sentences about our process, and +it reads `src/`, never `dist/` (untracked build output it cannot see without building). A new +programme prefix has to be added by hand. So the reviewer still owns half the rule. --- diff --git a/scripts/phi-scan.ts b/scripts/phi-scan.ts index c523719..8832984 100644 --- a/scripts/phi-scan.ts +++ b/scripts/phi-scan.ts @@ -83,29 +83,98 @@ * `src/notes.json` is under a scan root and `--staged` exits 0 over it, because * the `src/` half of the scope is `.ts` only. The all-mode sweep does refuse it, * so nothing escapes the gate as a whole. The rule does not reach the three - * shapes below either, all PRE-EXISTING, all measured on this repo, none closed: - * - * 1. A SCAN ROOT THAT IS ITSELF A LIVE LINK IS FOLLOWED, in the all-mode walk. - * `existsSync` and `readdirSync` both resolve links, so with - * `test/__fixtures__` pointing at a directory outside the repository the - * walk reads files no commit contains and reports their values under a - * FABRICATED in-repo path that holds no such file. That is a confident - * wrong provenance, on the same channel this banner argues is itself a PHI - * surface. The DANGLING direction is the mirror image of the same shape: - * `existsSync` follows the link, answers false, and the walk reports clean - * over a corpus it never opened. + * shapes below either. THE OBSERVATION RULE IN THE NEXT BANNER NOW REACHES MOST + * OF THEM, IN THE ALL-MODE WALK ONLY, and each is restated with exactly what is + * left rather than ticked off: + * + * 1. A SCAN ROOT THAT IS ITSELF A LIVE LINK IS STILL FOLLOWED, in the all-mode + * walk: `existsSync` and `readdirSync` both resolve links. Following is not + * what got fixed; being unable to TELL is. With `test/__fixtures__` + * pointing at a directory outside the repository the walk reads files no + * commit contains and would report their values under a FABRICATED in-repo + * path that holds no such file - a confident wrong provenance, on the same + * channel this banner argues is itself a PHI surface. The observation rule + * now REFUSES that (exit 2, measured) whenever git tracks an in-scope file + * under the root that the link's target does not also carry at the same + * relative path: seven files here, so a link to an UNRELATED directory + * refuses here. STILL OPEN, AND STATED RATHER THAN IMPLIED AWAY, BECAUSE + * THE SHORTER VERSION OF THIS SENTENCE IS FALSE AND WAS MEASURED FALSE: the + * reconciliation compares PATH SETS, not the bytes git carries at those + * paths, so a target directory that mirrors the tracked NAMES satisfies both + * conditions and is followed silently, decoy contents and all - measured at + * exit 0 over this repo's own seven tracked fixture names. A root git tracks + * NOTHING under is the degenerate case of that, not the whole of it. The + * DANGLING direction IS closed outright, whatever is tracked, because it + * opens nothing. * 2. AN ANCESTOR of a scan root is in neither route's scope. Fact 3 below puts * `test/__fixtures__` and `src` in scope, but not `test`, so staging `test` - * as a link leaves `--staged` at exit 0 (measured), and the walk then - * follows it exactly as in (1). + * as a link leaves `--staged` at exit 0 (measured) - STILL OPEN, and it is + * the half that gates a commit. The all-mode walk no longer follows it + * quietly: replacing `test` leaves `test/__fixtures__` unopenable, which + * the observation rule refuses. HOW FAR ABOVE A ROOT TO LOOK IS STILL NOT + * DECIDED, and is deliberately not decided here. * 3. PATHS MODE FOLLOWS AN EXPLICITLY NAMED LINK. `buildTargetsForPaths` uses * `statSync`, which resolves, so `pnpm phi-scan ` reads the target's - * bytes. A caller naming a path is a different act from an enumeration - * reaching one, but it is stated here rather than left to be discovered. + * bytes. UNCHANGED AND STILL OPEN: paths mode has no corpus to reconcile + * against, because a caller naming a path is asking about that path. It is + * a floor of one and it is its own slice. + * + * =========================================================================== + * A DECLARED SCAN ROOT THE WALK DID NOT OBSERVE REFUSES THE SCAN (exit 2), IN + * ALL MODE. `existsSync` and `readdirSync` describe a WORKING TREE; what a scan + * result is a claim about is the CORPUS GIT CARRIES. So each root's walk is + * reconciled against `git ls-files`, and TWO independent conditions refuse: + * + * - the root contributed NOTHING, or + * - git tracks an in-scope file under the root that the walk did not open. + * + * NEITHER SUBSUMES THE OTHER AND BOTH SHIP. Measured on this repo, all six of + * these previously printed `OK, no hits` and exited 0: the root missing, the root + * emptied, the root a DANGLING link, the root a LIVE link to an outside + * directory, one tracked fixture removed from the working tree, and `src` moved + * away. The middle two are exactly why one condition is not enough - a swapped + * root opens plenty, an emptied one opens nothing. + * + * ▶ THE DANGLING CASE IS WHY A KIND CHECK CANNOT STAND IN. `existsSync` FOLLOWS + * the link and answers false, so `walk()` returns before `readdirSync` and the + * not-a-regular-file rule above never fires: nothing about the entry is ever + * inspected. Refusing on what was OBSERVED needs no opinion about the entry. + * + * ▶ A DENOMINATOR IS NOT THIS RULE AND WAS DELIBERATELY NOT ADDED. A count + * counts the roots and the files that DID exist, so a healthy-looking total is + * precisely what a starved root produces. This scanner prints no file count and + * one is not added here; the reconciliation is the signal. * - * Closing any of those needs a refuse-a-scan-that-observed-nothing rule plus a - * decision about how far ABOVE a scan root to look, which is its own slice. The - * `--staged` half of shape (1) IS closed here, by fact 3 below. + * ▶ IT IS ONE-DIRECTIONAL ON PURPOSE. A tracked in-scope file the walk missed + * refuses; an untracked working-tree file the walk found does NOT, because + * scanning more than git carries is the safe direction, and refusing it would + * red the gate on every fixture a developer has written but not yet added. + * + * ▶ SCOPED TO ALL MODE. `--staged` is a DIFF, not a corpus, and has no corpus to + * reconcile against; paths mode is a caller naming paths. Widening either is a + * different decision, and widening `--staged` changes what a COMMIT is blocked + * on, so neither is taken here. + * + * ▶ `git ls-files` FAILING REFUSES rather than answering the empty set. An empty + * answer is indistinguishable from "this root tracks nothing", so a broken git + * would switch the whole rule off silently and restore the green it exists to + * end. A tracked path is also never reported by `git check-ignore` (it consults + * the index by default), so a stray `.gitignore` line cannot excuse one out of + * the reconciliation set. + * + * THREE RESIDUALS, STATED RATHER THAN DISCOVERED, and the third is the one an + * earlier draft of this list left out while its own shape (1) above was busy + * describing it: + * + * - the reconciliation compares PATH SETS, not the bytes git carries at those + * paths, so a directory mirroring the tracked NAMES clears both conditions + * with decoy contents (measured, exit 0). Comparing blobs is a different and + * larger rule and is deliberately not taken here; + * - a root git tracks nothing under is held only by the opened-nothing + * condition, which is a FLOOR OF ONE (one observed file satisfies it), and + * is the degenerate case of the first residual rather than a separate one; + * - the rule says nothing about a path ABOVE a root. + * =========================================================================== * * ▶ THE PRE-COMMIT HOLE THAT MADE THIS URGENT WAS RENAME DETECTION, AND IT IS * NOT LIMITED TO LINKS. `R` and `C` are returned by neither `--diff-filter=AM` @@ -195,6 +264,17 @@ const OVERRIDE_LOG_PATH = join(REPO_ROOT, "phi-scan-overrides.md"); const FIXTURE_ROOT = join(REPO_ROOT, "test", "__fixtures__"); const SRC_ROOT = join(REPO_ROOT, "src"); +/** + * The declared scan roots, each paired with the repo-relative identity used both + * in a refusal and in the `git ls-files` reconciliation below. The pair is here + * rather than derived with `normalizePath` so a root's reported name cannot + * depend on whether the root currently resolves. + */ +const SCAN_ROOTS: readonly { abs: string; rel: string }[] = [ + { abs: FIXTURE_ROOT, rel: "test/__fixtures__" }, + { abs: SRC_ROOT, rel: "src" }, +]; + // --------------------------------------------------------------------------- // Types // --------------------------------------------------------------------------- @@ -357,7 +437,19 @@ function normalizePath(p: string): string { function loadOverrideLog(): Set { if (!existsSync(OVERRIDE_LOG_PATH)) return new Set(); - const raw = readFileSync(OVERRIDE_LOG_PATH, "utf8"); + // Same reason as `loadAllowList` above, and it was still open here: a + // present-but-unreadable override log threw a raw fs error past every handler + // and node exited 1, which is this contract's code for "hits found". + let raw: string; + try { + raw = readFileSync(OVERRIDE_LOG_PATH, "utf8"); + } catch (err) { + throw new InvocationError( + `could not read the override log at ${OVERRIDE_LOG_PATH}: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } const out = new Set(); for (const lineRaw of raw.split(/\r?\n/)) { const m = /^###\s+(.+?)\s*$/.exec(lineRaw); @@ -480,16 +572,124 @@ function gitIgnored(paths: string[]): Set { return ignored; } +/** + * Every path git tracks under `rel`, repo-relative, in git's own spelling. + * + * This is the GROUND TRUTH the walk gets reconciled against. `existsSync` and + * `readdirSync` describe a WORKING TREE, and a working tree can be missing, + * emptied, or pointed at another directory entirely while the commit git would + * produce from the index is unchanged. Asking git what it carries is the only + * question whose answer a scan result is allowed to be a statement about. + * + * A failure REFUSES rather than answering the empty set, and that is the whole + * design: an empty answer is indistinguishable from "this root tracks nothing", + * so a broken `git` would silently switch the reconciliation off and restore + * exactly the green-over-an-unopened-corpus this rule exists to end. + */ +function trackedUnder(rel: string): string[] { + let out: Buffer; + try { + // SECURITY: array-form execFileSync, no shell. `--` ends the option list, so + // a root name can never be read as one. + out = execFileSync("git", ["ls-files", "-z", "--", rel], { + encoding: "buffer", + stdio: ["ignore", "pipe", "pipe"], + }); + } catch (err) { + throw new InvocationError( + `could not ask git what it tracks under ${rel}: ${ + err instanceof Error ? err.message : String(err) + }. Refusing rather than reconciling the walk against an empty list.`, + ); + } + // De-duplicated: `git ls-files` emits an UNMERGED path once per stage, so a + // conflicted fixture was named three times in one refusal, reading as three + // missing files. The refusal was right; only its count was not. + return [ + ...new Set( + out + .toString("utf8") + .split("\0") + .filter((p) => p.length > 0), + ), + ]; +} + +/** What one scan root actually contributed, against what git says is under it. */ +interface RootObservation { + rel: string; + /** In-scope files the walk actually opened under this root. */ + opened: number; + /** Tracked, in-scope paths under this root that the walk did NOT open. */ + unopened: string[]; +} + +/** + * Refuse (exit 2) when a declared scan root was not observed. TWO independent + * conditions, and the second is "in addition to", never "instead of": + * + * - the root contributed NOTHING. A missing root, an emptied one and a + * DANGLING link all reach this the same way, and the dangling one is why a + * kind check cannot stand in: `existsSync` FOLLOWS the link and answers + * false, so `walk()` returns before `readdirSync` and the not-a-regular-file + * rule never fires. Nothing about the entry is ever inspected. + * - git tracks an in-scope file under the root that the walk did not open. An + * EMPTIED root opens nothing but so does a root whose corpus was moved + * aside, and a root swapped for another directory opens plenty. Counting is + * no help for either: a COUNT COUNTS THE FILES THAT WERE THERE, which is why + * a denominator is deliberately not what this rule is. + * + * Exit 2, from THIS scanner's own contract and not ported from a sibling (they + * differ, and porting one is the bug): `1` means "hits found" here, `walk()` + * already raises an unreadable root as an InvocationError, and a root replaced + * by a regular file already exits 2 through `readdirSync`'s ENOTDIR. A starved + * root belongs with those, not with a finding. + */ +function refuseUnobserved(observations: RootObservation[]): void { + const bad = observations.filter((o) => o.opened === 0 || o.unopened.length > 0); + if (bad.length === 0) return; + const lines = bad.map((o) => { + if (o.unopened.length === 0) return ` - ${o.rel}: opened nothing`; + const names = o.unopened.map((p) => ` - ${p}`).join("\n"); + return ( + ` - ${o.rel}: opened ${String(o.opened)} file(s), and git tracks ` + + `${String(o.unopened.length)} in-scope file(s) under it that the walk never opened:\n${names}` + ); + }); + const noun = bad.length === 1 ? "scan root was" : "scan roots were"; + throw new InvocationError( + `refusing the scan: ${String(bad.length)} ${noun} not observed:\n${lines.join("\n")}\n` + + "A root the walk never opened has not been cleared by it, so reporting no hits would state " + + "a result about a corpus nobody read. Restore the root as a real directory holding the " + + "files git tracks under it, or stop declaring it as a scan root.", + ); +} + function buildTargetsForAll(): Target[] { - const files: string[] = []; - const unscannable: Unscannable[] = []; - walk(FIXTURE_ROOT, files, unscannable); - walk(SRC_ROOT, files, unscannable); + const perRoot = SCAN_ROOTS.map((root) => { + const files: string[] = []; + const unscannable: Unscannable[] = []; + walk(root.abs, files, unscannable); + // Asked BEFORE any refusal below, so one `git check-ignore` can cover the + // walked entries and the tracked ones together. + return { ...root, files, unscannable, tracked: trackedUnder(root.rel) }; + }); + + const files = perRoot.flatMap((r) => r.files); + const unscannable = perRoot.flatMap((r) => r.unscannable); - // One `git check-ignore` over both lists. An ignored entry is already out of + // One `git check-ignore` over every list. An ignored entry is already out of // scope for the file route, so applying the same rule to a link keeps a single // boundary rather than inventing a second, stricter one for links alone. - const ignored = gitIgnored([...files.map(normalizePath), ...unscannable.map((u) => u.path)]); + // + // A TRACKED path is never reported ignored (check-ignore consults the index by + // default), so this filter is a no-op on the reconciliation set and a tracked + // file cannot be excused out of it by a stray .gitignore line. + const ignored = gitIgnored([ + ...files.map(normalizePath), + ...unscannable.map((u) => u.path), + ...perRoot.flatMap((r) => r.tracked), + ]); refuseUnscannable( unscannable.filter((u) => !ignored.has(u.path)), @@ -498,6 +698,23 @@ function buildTargetsForAll(): Target[] { "corpus) untrack it and add it to .gitignore.", ); + // The observation rule. It runs AFTER the unscannable refusal because that one + // names a specific entry and is the more actionable message when both apply. + refuseUnobserved( + perRoot.map((r) => { + const opened = new Set(r.files.map(normalizePath).filter((p) => !ignored.has(p))); + // Filtered by the walk's OWN in-scope rule, so the two sides of the + // comparison mean the same thing: a `.md` file the walk skips by design is + // not evidence that the walk was starved. + const expected = r.tracked.filter((p) => !p.toLowerCase().endsWith(".md") && !ignored.has(p)); + return { + rel: r.rel, + opened: opened.size, + unopened: expected.filter((p) => !opened.has(p)), + }; + }), + ); + return files .filter((abs) => !ignored.has(normalizePath(abs))) .map((abs) => ({ path: normalizePath(abs), read: () => readFileSync(abs) })); diff --git a/test/scripts/phi-scan.test.ts b/test/scripts/phi-scan.test.ts index a9e9d9e..cf85127 100644 --- a/test/scripts/phi-scan.test.ts +++ b/test/scripts/phi-scan.test.ts @@ -581,6 +581,225 @@ describe("phi-scan --staged: the scope is widened, never narrowed", { timeout: S }); }); +/** + * `makeRepo()` with both scan roots' contents ADDED to the index, which is what + * `git ls-files` answers off. Every reconciliation case below needs a TRACKED + * corpus, because the rule compares what git carries against what the walk + * opened, and a repo tracking nothing has nothing to reconcile. A commit is not + * needed: `git ls-files` reads the index. + */ +function makeTrackedRepo(): string { + const root = makeRepo(); + git(root, ["add", "test/__fixtures__/ordinary.txt", "src/ok.ts"]); + // Assert the premise rather than assume it. If the add ever stopped taking, + // every case below would hold over an empty expected set and pass vacuously, + // which is the failure mode this file has already sprung twice. + expect(gitOut(root, ["ls-files", "test/__fixtures__"]).trim()).toBe( + "test/__fixtures__/ordinary.txt", + ); + return root; +} + +describe( + "phi-scan: a declared scan root the walk never observed refuses (exit 2)", + { timeout: SLOW_MS }, + () => { + // CI runs `pnpm phi-scan` with no arguments, so this route is the one that + // can print `OK, no hits` and exit 0 over a corpus nobody opened. Each case + // here exited 0 with that message before the observation rule, measured. + + it("stays green when both roots are healthy and fully observed", () => { + // The premise, first: a refusal rule that reds the ordinary case teaches + // people to disable it, and every case below would pass vacuously against + // a scanner that had simply started refusing everything. + const root = makeTrackedRepo(); + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(0); + expect(r.stdout).toMatch(/OK, no hits/); + }); + + it("refuses a MISSING root whose corpus git still tracks", () => { + const root = makeTrackedRepo(); + rmSync(join(root, "test", "__fixtures__"), { recursive: true }); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("test/__fixtures__: opened 0 file(s)"); + expect(r.stderr).toContain("test/__fixtures__/ordinary.txt"); + expect(r.stdout).not.toMatch(/OK/); + }); + + it("refuses an EMPTIED root, which existence alone cannot tell from a clean one", () => { + // `existsSync` answers true here and `readdirSync` succeeds: the root is a + // real, readable directory. Refusing a MISSING root would leave this half + // wide open, which is why the rule is about observation and not existence. + const root = makeTrackedRepo(); + rmSync(join(root, "test", "__fixtures__", "ordinary.txt")); + expect(existsSync(join(root, "test", "__fixtures__"))).toBe(true); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("test/__fixtures__/ordinary.txt"); + }); + + it("refuses a DANGLING root link, which existsSync FOLLOWS and answers false for", () => { + // The sharpest case, and the reason a not-a-regular-file check cannot + // stand in for this rule: `existsSync` resolves the link, answers false, + // and `walk()` returns before `readdirSync`. Nothing about the entry is + // ever inspected, so no kind check can fire on it. + const root = makeTrackedRepo(); + rmSync(join(root, "test", "__fixtures__"), { recursive: true }); + symlinkSync(join("..", "nowhere-at-all"), join(root, "test", "__fixtures__")); + expect(existsSync(join(root, "test", "__fixtures__"))).toBe(false); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("test/__fixtures__/ordinary.txt"); + expect(r.stdout).not.toMatch(/OK/); + }); + + it("refuses a root swapped for an outside directory, which a count reads as healthy", () => { + // Shape (1) of the module header in its TRACKED form. The walk follows the + // link and opens a file, so a denominator or a per-root count looks fine. + // What gives it away is that none of the corpus git carries was among what + // was opened, and the reported path resolves while naming nothing tracked. + const root = makeTrackedRepo(); + const outside = realpathSync(mkdtempSync(join(tmpdir(), "cli-phi-scan-outside-"))); + repos.push(outside); + writeFileSync(join(outside, "unrelated.txt"), "synthetic placeholder\n"); + rmSync(join(root, "test", "__fixtures__"), { recursive: true }); + symlinkSync(outside, join(root, "test", "__fixtures__")); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("test/__fixtures__: opened 1 file(s)"); + expect(r.stderr).toContain("test/__fixtures__/ordinary.txt"); + }); + + it("refuses one tracked file removed while the rest of the root is opened", () => { + // Root granularity would miss this: the root exists, is readable, and + // yields files. The rule is per tracked FILE, not per root. + const root = makeTrackedRepo(); + writeFileSync(join(root, "test", "__fixtures__", "second.txt"), "synthetic placeholder\n"); + git(root, ["add", "test/__fixtures__/second.txt"]); + rmSync(join(root, "test", "__fixtures__", "ordinary.txt")); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("test/__fixtures__: opened 1 file(s)"); + expect(r.stderr).toContain("test/__fixtures__/ordinary.txt"); + expect(r.stderr).not.toContain("test/__fixtures__/second.txt"); + }); + + it("refuses when src/ is the starved root, not only the fixture root", () => { + const root = makeTrackedRepo(); + rmSync(join(root, "src"), { recursive: true }); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("src: opened 0 file(s)"); + expect(r.stderr).toContain("src/ok.ts"); + }); + + it("is ONE-DIRECTIONAL: an untracked file the walk found is not a refusal", () => { + // Scanning more than git carries is the safe direction. Refusing it would + // red the gate on every fixture a developer has written but not yet added. + const root = makeTrackedRepo(); + writeFileSync(join(root, "test", "__fixtures__", "not-added-yet.txt"), "placeholder\n"); + expect(gitOut(root, ["ls-files", "test/__fixtures__/not-added-yet.txt"]).trim()).toBe(""); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(0); + }); + + it("still scans the corpus it reconciles: a tracked violator is a hit, not a refusal", () => { + // The rule must not become the only thing the all-mode route reports. + const root = makeTrackedRepo(); + writeFileSync(join(root, "test", "__fixtures__", "violator.txt"), SYNTHETIC_PHI); + git(root, ["add", "test/__fixtures__/violator.txt"]); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(1); + expect(r.stderr).toContain("test/__fixtures__/violator.txt"); + expect(r.stderr).not.toContain("refusing the scan"); + }); + + it("refuses rather than reconciling against an empty list when git cannot answer", () => { + // An empty `git ls-files` answer is indistinguishable from "this root + // tracks nothing", so a git that cannot answer would switch the whole rule + // off in silence and restore the exact green it exists to end. + const bare = realpathSync(mkdtempSync(join(tmpdir(), "cli-phi-scan-nogit-"))); + repos.push(bare); + mkdirSync(join(bare, "scripts")); + mkdirSync(join(bare, "test", "__fixtures__"), { recursive: true }); + mkdirSync(join(bare, "src")); + copyFileSync( + join(REPO_ROOT, "scripts", "phi-allow-list.txt"), + join(bare, "scripts", "phi-allow-list.txt"), + ); + writeFileSync(join(bare, "test", "__fixtures__", "ordinary.txt"), "placeholder\n"); + writeFileSync(join(bare, "src", "ok.ts"), "export const ok = 1;\n"); + // The premise: this really is outside any repository, so `git ls-files` + // fails rather than answering about some enclosing one. + expect(gitOut(bare, ["rev-parse", "--show-toplevel"]).trim()).toBe(""); + + const r = runScanner([], bare); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("could not ask git what it tracks"); + expect(r.stdout).not.toMatch(/OK/); + }); + + it("names an unmerged path ONCE, not once per stage", () => { + // `git ls-files` emits an unmerged path once per stage, so a conflicted + // fixture was named three times in one refusal and read as three missing + // files. The refusal was right either way; a diagnostic nobody can trust + // is how a gate stops being read. + const root = makeTrackedRepo(); + git(root, [...COMMIT, "base"]); + const base = gitOut(root, ["rev-parse", "--abbrev-ref", "HEAD"]).trim(); + git(root, ["checkout", "-q", "-b", "other"]); + writeFileSync(join(root, "test", "__fixtures__", "ordinary.txt"), "theirs\n"); + git(root, ["add", "test/__fixtures__/ordinary.txt"]); + git(root, [...COMMIT, "theirs"]); + git(root, ["checkout", "-q", base]); + writeFileSync(join(root, "test", "__fixtures__", "ordinary.txt"), "ours\n"); + git(root, ["add", "test/__fixtures__/ordinary.txt"]); + git(root, [...COMMIT, "ours"]); + const merge = spawnSync("git", [...MERGE, "other"], { + cwd: root, + encoding: "utf8", + shell: false, + }); + // The premise, asserted rather than discarded: a merge that does not + // conflict leaves one stage, and every assertion below would hold for the + // wrong reason. This suite has already shipped that exact vacuity once. + expect(merge.status, `merge: ${merge.stdout}${merge.stderr}`).not.toBe(0); + expect(gitOut(root, ["ls-files", "-u", "test/__fixtures__"]).trim().split("\n")).toHaveLength( + 3, + ); + rmSync(join(root, "test", "__fixtures__", "ordinary.txt")); + + const r = runScanner([], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("git tracks 1 in-scope file(s)"); + expect(r.stderr.match(/test\/__fixtures__\/ordinary\.txt/g)).toHaveLength(1); + }); + + it("leaves --staged alone: it is a diff, with no corpus to reconcile against", () => { + // Widening `--staged` changes what a COMMIT is blocked on, which is a + // different decision and is deliberately not taken here. + const root = makeTrackedRepo(); + git(root, [...COMMIT, "base"]); + rmSync(join(root, "test", "__fixtures__"), { recursive: true }); + writeFileSync(join(root, "src", "added.ts"), "export const added = 1;\n"); + git(root, ["add", "src/added.ts"]); + + const r = runScanner(["--staged"], root); + expect(r.code, `stderr: ${r.stderr}`).toBe(0); + }); + }, +); + describe("phi-scan: an invocation failure exits 2, never 1", { timeout: SLOW_MS }, () => { // `1` is this contract's code for "hits found". A caller branching on the exit // code read a broken invocation as a PHI finding; a caller branching on @@ -598,6 +817,41 @@ describe("phi-scan: an invocation failure exits 2, never 1", { timeout: SLOW_MS expect(r.stderr).not.toContain("at loadAllowList"); }); + it("an unreadable OVERRIDE LOG exits 2, not 1 with a stack trace", () => { + // The allow-list reader was wrapped; its sibling, `loadOverrideLog`, was + // not, so a present-but-unreadable `phi-scan-overrides.md` threw a raw + // EACCES past every handler and node exited 1: this contract's code for + // "hits found". Measured at exit 1 before this change. + // + // `hasAssertions` for the same reason as the case below: under a uid that + // ignores mode bits the file stays readable and the early return would be a + // silent pass rather than a visible skip. + expect.hasAssertions(); + const root = makeRepo(); + const log = join(root, "phi-scan-overrides.md"); + writeFileSync(log, "# overrides\n\n### test/__fixtures__/ordinary.txt\n"); + let r: RunResult; + try { + spawnSync("chmod", ["000", log], { encoding: "utf8", shell: false }); + let readable = true; + try { + readFileSync(log, "utf8"); + } catch { + readable = false; + } + expect(typeof readable).toBe("boolean"); + if (readable) return; + r = runScanner(["--allow-fixture", "test/__fixtures__/ordinary.txt"], root); + } finally { + spawnSync("chmod", ["644", log], { encoding: "utf8", shell: false }); + } + expect(r.code, `stderr: ${r.stderr}`).toBe(2); + expect(r.stderr).toContain("[phi-scan]"); + expect(r.stderr).toContain("could not read the override log"); + expect(r.stderr).not.toContain("InvocationError:"); + expect(r.stderr).not.toContain("at loadOverrideLog"); + }); + it("an unreadable scan root exits 2, rather than throwing out of readdirSync", () => { // `expect.hasAssertions()` because the early-out below is otherwise a silent // pass: under a uid that ignores mode bits (root in a container) traversal @@ -709,6 +963,14 @@ describe( // links. So the corpus root can point outside the repository and the walk // reads bytes no commit contains, reporting them under an in-repo path that // holds no such file. + // + // NARROWED, NOT CLOSED, AND THE NARROWING IS THE WHOLE POINT OF USING + // `makeRepo()` HERE RATHER THAN THE TRACKED HELPER: the observation rule + // refuses this shape as soon as git tracks anything under the root (the + // tracked form is asserted in the observation suite). What survives is + // exactly this - a root git carries NOTHING under, so the reconciliation + // has no expected path to miss and the walk's one hit satisfies the + // opened-nothing floor. const root = makeRepo(); const outside = realpathSync(mkdtempSync(join(tmpdir(), "cli-phi-scan-outside-"))); repos.push(outside); @@ -727,10 +989,24 @@ describe( expect(gitOut(root, ["ls-files", "test/__fixtures__/real-notes.txt"]).trim()).toBe(""); }); - it("reports clean over a corpus it never opened when a scan root DANGLES", () => { - const root = makeRepo(); + it("FOLLOWS a root link whose target MIRRORS the tracked names, corpus fully tracked", () => { + // The exact shape a refuter used to falsify the shorter disclosure. The + // reconciliation compares PATH SETS, not the bytes git carries at those + // paths, so a target directory holding the same relative filenames + // satisfies both conditions with decoy contents and the gate prints the + // headline sentence this whole rule exists to end. "Survives only where + // git tracks nothing under it" is FALSE, and this pins that it is false. + const root = makeTrackedRepo(); + const decoy = realpathSync(mkdtempSync(join(tmpdir(), "cli-phi-scan-decoy-"))); + repos.push(decoy); + writeFileSync(join(decoy, "ordinary.txt"), "decoy, not the tracked bytes\n"); rmSync(join(root, "test", "__fixtures__"), { recursive: true }); - symlinkSync(join("..", "nowhere-at-all"), join(root, "test", "__fixtures__")); + symlinkSync(decoy, join(root, "test", "__fixtures__")); + // The premise: git really does carry a DIFFERENT blob at that path, so a + // pass here is a pass over a corpus that was never opened. + expect(gitOut(root, ["show", ":test/__fixtures__/ordinary.txt"])).toContain( + "synthetic placeholder", + ); const r = runScanner([], root); expect(r.code, `stderr: ${r.stderr}`).toBe(0);