From e72dba67ac749e875424ec6485341e1efa71af9e Mon Sep 17 00:00:00 2001 From: Noah Schatz Date: Fri, 7 Aug 2026 18:02:27 +0000 Subject: [PATCH 1/3] fix(phi-scan): refuse a scan root the walk never observed (PHI-SCAN-OBSERVED-NOTHING-IS-GLOBAL) `pnpm phi-scan` with no arguments, which is what CI runs, printed `OK, no hits` and exited 0 over a corpus it never opened. Each scan root's walk is now reconciled against `git ls-files`, and two independent conditions refuse at exit 2: the root contributed nothing, or git tracks an in-scope file under it that the walk did not open. Six states measured on this tree, every one of them previously exiting 0 with `OK, no hits`: 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. 8 of the suite's tests run red on cd221a0 and green here. Neither condition subsumes the other, so both ship: an emptied root opens nothing, a swapped one opens plenty, and refusing only a MISSING root leaves the other half open, because existence is not observation. 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 refusal never fires, leaving nothing about the entry ever inspected. A denominator was deliberately NOT added. A count counts the roots and the files that did exist, so a healthy-looking total is exactly what a starved root produces. 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` (re-measured, not assumed). Sibling scanners disagree on this code. The rule is one-directional (a tracked file the walk missed refuses, an untracked one it found does not) and scoped to all mode: `--staged` is a diff rather than a corpus, and widening it would change what a commit is blocked on. `git ls-files` failing refuses rather than answering the empty set. Two of the three disclosed escapes are narrowed, not closed, and the module header says so: a scan root that is itself a live link is still followed and survives only where git tracks nothing under it, and an ancestor of a scan root is still out of the staged route's scope. Paths mode is untouched. Also fixed: 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". It now exits 2, matching the allow-list reader beside it. The two other minors a sibling named were re-derived and measured NOT open here. Measured for the record: 122 tracked files, 34 opened by the all-mode walk, 88 scanned by neither route, 6 of those carrying an inline `PID|` literal. All six were read by hand and are placeholder shapes, no real identifier. That 88 is an enumeration gap and a different item; this change widened no root, so the recogniser needed no widening. --- ...phi-scan-refuse-an-unobserved-scan-root.md | 22 ++ CHANGELOG.md | 39 +++ CLAUDE.md | 21 +- documentation/agent-notes.md | 61 +++++ scripts/phi-scan.ts | 247 ++++++++++++++++-- test/scripts/phi-scan.test.ts | 236 ++++++++++++++++- 6 files changed, 585 insertions(+), 41 deletions(-) create mode 100644 .changeset/phi-scan-refuse-an-unobserved-scan-root.md 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..d681a7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,45 @@ 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, and now survives only where git tracks nothing under it; an + **ancestor** of a scan root remains out of the staged route's scope; 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..e8b2054 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,14 +160,23 @@ 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 survives only where git tracks nothing under it; 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..34243d4 100644 --- a/documentation/agent-notes.md +++ b/documentation/agent-notes.md @@ -137,6 +137,67 @@ 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. + +- **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. **8 of the suite's tests run red on `cd221a0`** and green after. + +**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 and now survives only where git tracks nothing +under 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.** diff --git a/scripts/phi-scan.ts b/scripts/phi-scan.ts index c523719..0d803c0 100644 --- a/scripts/phi-scan.ts +++ b/scripts/phi-scan.ts @@ -83,29 +83,85 @@ * `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 it refuses here. STILL OPEN, AND + * STATED RATHER THAN IMPLIED AWAY: a root git tracks NOTHING under, swapped + * for a NON-EMPTY directory, satisfies both conditions and is followed + * silently. 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. + * + * TWO RESIDUALS, STATED RATHER THAN DISCOVERED: 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 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 +251,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 +424,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 +559,117 @@ 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.`, + ); + } + return 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 +678,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..fa3051d 100644 --- a/test/scripts/phi-scan.test.ts +++ b/test/scripts/phi-scan.test.ts @@ -581,6 +581,189 @@ 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("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 +781,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 +927,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,16 +953,6 @@ 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(); - rmSync(join(root, "test", "__fixtures__"), { recursive: true }); - symlinkSync(join("..", "nowhere-at-all"), join(root, "test", "__fixtures__")); - - const r = runScanner([], root); - expect(r.code, `stderr: ${r.stderr}`).toBe(0); - expect(r.stdout).toMatch(/OK, no hits/); - }); - it("does not see an ANCESTOR of a scan root staged as a link", () => { // Fact 3 puts `test/__fixtures__` and `src` in scope, but not `test`. The // "git records no index entry for a directory" argument applies to `test` From 79becd879a2ae8d25963a16261e09976ad8e3670 Mon Sep 17 00:00:00 2001 From: Noah Schatz Date: Fri, 7 Aug 2026 18:19:07 +0000 Subject: [PATCH 2/3] fix(phi-scan): correct a false residual disclosure and name an unmerged path once Three findings from the conformance gate on e72dba6, all INTRODUCED by that commit and none of them a change to what the rule refuses. 1. THE RESIDUAL DISCLOSURE WAS FALSE AND WAS FALSIFIED IN ONE RUN. `CLAUDE.md`, `CHANGELOG.md` and `documentation/agent-notes.md` all said the live-link scan-root escape "survives only where git tracks nothing under it". It does not. 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 satisfies both conditions and the gate prints `OK, no hits` at exit 0 over decoy contents. Measured. The module header had it right; the three restatements dropped the qualifier and inverted it. All four surfaces now name the surviving shape, and a root git tracks nothing under is stated as the degenerate case of it rather than the whole of it. An inaccurate disclosure is a defect in its own right here, so the remedy is the claim and deliberately not a bigger guard. 2. THE COUNTS WERE STALE IN THE COMMIT THAT SHIPPED THEM. 122 tracked, 88 in neither route and 6 carrying an inline `PID|` literal are all correct as of cd221a0 and all move the moment this slice adds a file, including the note that states them, which contains the literal it counts. They are now anchored to that sha, with the reason written next to them. 3. AN UNMERGED PATH WAS NAMED ONCE PER STAGE. `git ls-files` emits three records for a conflicted path, so one missing fixture was reported as three. `trackedUnder` de-duplicates. The refusal and its exit code were right either way; only the diagnostic was wrong. Two tests added: one pins the surviving live-link shape so the disclosure cannot quietly revert to the absolute wording, and one pins the unmerged count, asserting the three stages exist before asserting the one line. --- CHANGELOG.md | 10 +++- CLAUDE.md | 8 ++- documentation/agent-notes.md | 110 +++++++++++++++++++--------------- scripts/phi-scan.ts | 30 +++++++--- test/scripts/phi-scan.test.ts | 60 +++++++++++++++++++ 5 files changed, 154 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d681a7a..eac0a9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,9 +49,13 @@ still do. Each entry was assigned to the release whose tag first contains it, re 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, and now survives only where git tracks nothing under it; an - **ancestor** of a scan root remains out of the staged route's scope; paths mode still follows a - link a caller names. + 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 diff --git a/CLAUDE.md b/CLAUDE.md index e8b2054..4754294 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -174,9 +174,11 @@ Why: [agent-notes § The pre-commit PHI gate and git mv](documentation/agent-not - **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 remain, NARROWED not closed** (a scan root that is itself a live link is still - followed and survives only where git tracks nothing under it; 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. + 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 34243d4..258f0d2 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."_ --- @@ -143,6 +143,12 @@ The measurement first, because the class this belongs to is one where a phrase s 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 @@ -187,10 +193,16 @@ git tracks an in-scope file under the root that the walk did not open. 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 and now survives only where git tracks nothing -under 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.** +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 @@ -521,13 +533,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 @@ -572,7 +584,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 @@ -707,41 +719,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 0d803c0..349c936 100644 --- a/scripts/phi-scan.ts +++ b/scripts/phi-scan.ts @@ -96,11 +96,16 @@ * 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 it refuses here. STILL OPEN, AND - * STATED RATHER THAN IMPLIED AWAY: a root git tracks NOTHING under, swapped - * for a NON-EMPTY directory, satisfies both conditions and is followed - * silently. The DANGLING direction is closed outright, whatever is tracked, - * because it opens nothing. + * 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) - STILL OPEN, and it is @@ -589,10 +594,17 @@ function trackedUnder(rel: string): string[] { }. Refusing rather than reconciling the walk against an empty list.`, ); } - return out - .toString("utf8") - .split("\0") - .filter((p) => p.length > 0); + // 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. */ diff --git a/test/scripts/phi-scan.test.ts b/test/scripts/phi-scan.test.ts index fa3051d..cf85127 100644 --- a/test/scripts/phi-scan.test.ts +++ b/test/scripts/phi-scan.test.ts @@ -749,6 +749,42 @@ describe( 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. @@ -953,6 +989,30 @@ describe( expect(gitOut(root, ["ls-files", "test/__fixtures__/real-notes.txt"]).trim()).toBe(""); }); + 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(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); + expect(r.stdout).toMatch(/OK, no hits/); + }); + it("does not see an ANCESTOR of a scan root staged as a link", () => { // Fact 3 puts `test/__fixtures__` and `src` in scope, but not `test`. The // "git records no index entry for a directory" argument applies to `test` From a84099ba265e12172e2bd334d27162029fb2ffc0 Mon Sep 17 00:00:00 2001 From: Noah Schatz Date: Fri, 7 Aug 2026 18:31:00 +0000 Subject: [PATCH 3/3] docs(phi-scan): re-derive the red-test count and name the third residual Two minors from the second gate pass, both stale text and neither a change to behaviour. The red-test count went stale INSIDE the paragraph that had just been added to stop counts going stale. It read "8 of the suite's tests run red on cd221a0", true at e72dba6 when the suite held 45 tests; the remedy's own added test moved it. Re-derived against a clean tree: 9 of 47 red under cd221a0's scanner, 38 green. The denominator is now named, and so is the reason, because this is the second time in one slice. The module header's residual list said TWO while shape (1) sixty lines above it had just been corrected to describe a third: the reconciliation compares path sets rather than the bytes git carries at those paths, so a directory mirroring the tracked names clears both conditions with decoy contents. That is now listed as a residual in its own right, with the tracks-nothing case stated as its degenerate form, and with the note that comparing blobs would be a different and larger rule that is deliberately not taken here. --- documentation/agent-notes.md | 6 +++++- scripts/phi-scan.ts | 16 ++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/documentation/agent-notes.md b/documentation/agent-notes.md index 258f0d2..d81390d 100644 --- a/documentation/agent-notes.md +++ b/documentation/agent-notes.md @@ -165,7 +165,11 @@ that. Re-derive against a named sha or do not write the number down. 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. **8 of the suite's tests run red on `cd221a0`** and green after. +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 diff --git a/scripts/phi-scan.ts b/scripts/phi-scan.ts index 349c936..8832984 100644 --- a/scripts/phi-scan.ts +++ b/scripts/phi-scan.ts @@ -162,10 +162,18 @@ * the index by default), so a stray `.gitignore` line cannot excuse one out of * the reconciliation set. * - * TWO RESIDUALS, STATED RATHER THAN DISCOVERED: 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 the rule says nothing about a path ABOVE a - * root. + * 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