pharn-dev · PrzemekGalarowicz · Sep 25, 2026 · Sep 25, 2026
@@ -0,0 +1,81 @@
+# GRILL — review-cleanups
+
+Plan: `.dev/features/review-cleanups/PLAN.md`. Spec hash recomputed:
+`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e` — **matches**. Registered
+grillers: `{"registered":0,"grillers":[]}` → inline axes only. The plan is `trust: untrusted`;
+nothing in it read as an instruction.
+
+## Findings
+
+### Honest scope / completeness (P7, P3)
+
+```yaml
+- type: FINDING
+  rule_id: 'P5'
+  severity: important
+  file: '.dev/features/review-cleanups/PLAN.md:90'
+  problem: 'The F21 abort design says ONE stderr line names the part-way stop and the backup dir, yet also that the existing abort tests keep their stderr assertions. tests/update.test.ts:1814-1826 pins the LAST log.info AND the LAST log.warn of an aborted run on stderr, so one line cannot satisfy both. Decide it here: keep the part-way warning (log.warn) and the backup pointer (log.info) on stderr and drop only the repeated .gitignore hint — the duplicate F21 names — so both existing assertions hold.'
+  evidence: 'The aborted path prints one stderr line that names the part-way stop and the backup dir.'
+- type: FINDING
+  rule_id: 'P4'
+  severity: important
+  file: '.dev/features/review-cleanups/PLAN.md:115'
+  problem: 'The backfill names two merged pairs, but at least ten PHARN commits were later extended or superseded by an entry already in [Unreleased] or by this plan: PHARN-02/16 by #223, PHARN-04/17 by #222, PHARN-06 by #219 (>=20.12.0 → >=20.13.0), PHARN-10 by #220, PHARN-11 by #216/#223, PHARN-12 by #221, PHARN-13 by #217, PHARN-14 and PHARN-15 by this plan (F23, F22), PHARN-18 by #218. Each needs ONE entry describing the net behavior since 0.5.0, or the section contradicts itself. The build should read each commit rather than its title.'
+  evidence: 'Changes that a later entry already describes (e.g. PHARN-11''s manual carry, extended by #216; PHARN-13, extended by #217) get a single merged entry.'
+```
+
+### Determinism / parity (P5)
+
+```yaml
+- type: FINDING
+  rule_id: 'P5'
+  severity: important
+  file: '.dev/features/review-cleanups/PLAN.md:77'
+  problem: 'Upstream''s parser takes the frontmatter as text.slice(3, end), so the REMAINDER of the opening line is content: `--- name: a11y` on line 1 supplies `name`. The plan''s line rule ("opens on a first line that starts with ---") does not say what happens to that remainder, and the differential test over "every fence shape" will disagree on it unless the port takes the same slice. Port upstream''s slice semantics literally, and include an opening-line-remainder shape in the differential set.'
+  evidence: 'The fence opens on a first line that starts with `---` and closes at the next line that starts with `---`, which is the upstream rule'
+- type: FINDING
+  rule_id: 'P5'
+  severity: minor
+  file: '.dev/features/review-cleanups/PLAN.md:63'
+  problem: 'SKIP_DIRS matches the dir name case-insensitively (detect-archetype.ts:157). The plan does not say whether ECOSYSTEM_DIRS does, nor whether the markers (Cargo.toml, go.mod, …) are matched exactly. State both; the conservative reading is: the dir name case-insensitively (as today), the marker names exactly as each tool writes them.'
+  evidence: 'A new `ECOSYSTEM_DIRS` map skips a dir only when its marker holds:'
+```
+
+### Eval coverage (P1)
+
+```yaml
+- type: FINDING
+  rule_id: 'P1'
+  severity: minor
+  file: '.dev/features/review-cleanups/PLAN.md:93'
+  problem: 'The spinner mock in tests/update.test.ts:33 returns fresh anonymous vi.fn()s per spinner, so no assertion can order a spinner stop against a log line today. The call-order case needs a recording spinner mock (one shared log of stop/info/warn calls); changing the shared mock touches every update test, so keep it behavior-compatible.'
+  evidence: 'The spinner is stopped before the notice prints (call-order assertion on the clack mocks).'
+- type: FINDING
+  rule_id: 'P7'
+  severity: minor
+  file: '.dev/features/review-cleanups/PLAN.md:88'
+  problem: 'Stopping s2 with "Backed up N file(s)" and then printing the notice''s "Backed up N file(s) to <dir> …" says the same thing twice on consecutive lines. Stop the spinner with a neutral phrase (or with the pointer itself and no repeat).'
+  evidence: 'The backup callback first stops `s2` ("Backed up N file(s)"), prints the notice, then starts'
+```
+
+### Checked, no finding
+
+- **Trust (P2).** The fence change widens the untrusted parser to upstream's rule only; the field
+  reader, enums and duplicate-key refusal are unchanged. The ecosystem test reads only names already
+  listed plus one `lstat` inside the user's own tree.
+- **Scope.** The declared paths parse to 20 entries; the subtree refactor's six call sites are all
+  declared (`install-records`, `install-manifest` ×2, `install-capabilities`, `update`, `remove`).
+  A fresh scan finds exactly the three raw characters the plan names in `src/` and `tests/`, which
+  hold only `.ts` files, so the hygiene test needs no binary exclusions.
+- **Honest scope (P7).** The hygiene test answers a real, twice-repeated slip (#218 and plan D).
+
+## Summary
+
+Sound plan; three points must be decided before the build. The abort output must keep a stderr
+`log.warn` and a stderr `log.info`, or an existing test breaks. The fence port must take upstream's
+slice, opening-line remainder included, or its own differential test fails. The CHANGELOG backfill
+must merge about ten pairs, not two. Smaller points: the case rule for the ecosystem dirs, a
+recording spinner mock, and one duplicated line.
+
+**ADVISORY VERDICT: 6 concerns raised (0 blocking-severity, 3 important, 3 minor) — for the human to
+weigh before /pharn-dev-build.**
@@ -0,0 +1,174 @@
+# PLAN — review-cleanups (detector skips, fence parity, update's backup notice, one subtree helper, CHANGELOG + CLAUDE.md catch-up, raw invisible characters)
+
+- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4
+- increment: the small findings left from the PHARN-01..18 review, in one PR, as the human grouped
+  them (plan F):
+  - F22: the detector skips `vendor`/`target`/`venv`/`.venv` only where they are that ecosystem's
+    tree.
+  - F23: the frontmatter fence rule matches upstream's validator.
+  - F21: update's backup notice is printed with no spinner running, and once per stream.
+  - The role → subtree ternary lives in one place.
+  - F26: `CHANGELOG.md` gets its missing entries for PHARN-01..18.
+  - `CLAUDE.md` catches up with #215–#223.
+  - Amended after GATE 1 (same kind of cleanup, found while shipping A–E and B): raw invisible
+    characters in two test files become escapes, a test keeps `src/` and `tests/` free of them, and a
+    stale function name in comments is corrected.
+- layer(s): the CLI itself (`src/lib/detect-archetype.ts`, `src/lib/capability-index.ts`,
+  `src/commands/update.ts`, `src/lib/layout.ts` + 5 callers, `src/lib/symlink-guard.ts` comments),
+  tests, docs
+- constitution_refs: [P1, P3, P4, P5, P6]
+
+## Discovery — verified this run (P6), code read on HEAD after #223
+
+- **F22.** `SKIP_DIRS` (`detect-archetype.ts:77-104`) is matched at every depth, case-insensitively
+  (`:157`). PHARN-15 (#209) added `.venv`, `venv`, `__pycache__`, `vendor`, `target`, `.yarn`, and
+  its comment says "package.json still backstops it". That holds for `ssr`/`spa`, not for `backend`,
+  whose signal is structural (`app/**/route.ts`, migrations, `.sql`). So a Next.js route at
+  `app/target/route.ts` or `app/vendor/route.ts` goes dark: `[ssr, backend]` → `[ssr]`, reproduced
+  by the review on the same tree. `__pycache__` and `.yarn` are never hand-authored JS.
+  `target`/`vendor`/`venv` are ordinary route or folder names.
+- **F23.** `extractFrontmatter` (`capability-index.ts:232-241`) requires `lines[0] === '---'` and
+  closes at the first `/^---[ \t]*$/` line. This repo's copy of the upstream validator
+  (`.dev/floor/validate.mjs:69-73`) opens on `startsWith("---")` and closes at the first
+  `"\n---"`, which is any line STARTING with `---` (`----`, `--- note`). So a capability upstream's
+  own CI passes becomes `unknown` here. If the body later holds a column-0 field, the reason shown
+  is a misleading duplicate-key error. Both parsers handle the empty block `---\n---` the same way.
+- **F21.** `update.ts:343` prints the backup notice (`printBackupNotice`) from `onBackup` while
+  spinner `s2` is still animating, so the note's `│` is glued to the spinner frame, and below ~50
+  columns `stop` erases its last row (the review captured this with clack 1.8.1). On a later throw,
+  `:396` prints the same two lines again on stderr. `add` stops its spinner before its backup
+  (`add.ts:217-219`, then its backup).
+- **Subtree ternary.** `role === 'griller' ? paths.grillers : paths.lenses` appears at
+  `install-records.ts:369`, `install-manifest.ts:142,285`, `install-capabilities.ts:129`,
+  `update.ts:601`, and `remove.ts:150` (`capabilityRelDir`): six copies. `LayoutPaths` lives in
+  `layout.ts:40`.
+- **F26.** `[Unreleased]` has entries only for #213 onward. None of the 18 PHARN commits (#195–#212,
+  after the 0.5.0 release) added one.
+- **CLAUDE.md** (not hook-protected; `protect-trusted-paths.cjs:58`):
+  - The init step-5 passage still describes init's carry-over as "keep manual entries the index
+    still has", from before #216. It does not mention the kept unparseable entries, the
+    `frozenCapabilities` carry, the named drop, the fingerprint re-check under the lock, or #215's
+    preserved user keys.
+  - The update paragraph never mentions `frozenCapabilities` (PHARN-13/#217): 0 hits.
+  - The `add` passage calls the drift scan `collectDestDrift`; it is `scanDest`.
+- **Raw invisible characters (new).** `tests/tar-extract.test.ts:653` holds a raw U+FEFF (from
+  #218), `tests/terminal-safe.test.ts:8-9` a raw U+202E and U+200B. An editor or a diff view shows
+  nothing there, and the same slip reached plan D's files before it was caught. No check covers
+  `src/` or `tests/` for this.
+- **Stale name (new).** `symlink-guard.ts:45,51` name `collectDestDrift` for what is `scanDest`.
+
+## Files
+
+- `src/lib/detect-archetype.ts` — layer CLI/lib. `SKIP_DIRS` keeps the always-skipped set (caches,
+  VCS, `__pycache__`, `.yarn`). A new `ECOSYSTEM_DIRS` map skips a dir only when its marker holds:
+  - target: a sibling Cargo.toml, pom.xml or build.sbt;
+  - vendor: a sibling go.mod, composer.json or Gemfile;
+  - venv and .venv: a pyvenv.cfg inside.
+
+  The sibling test is a membership check over the parent's already-read `entries`, so it costs no
+  extra read. The venv test is one `lstat`. The dir name is matched case-insensitively, as
+  `SKIP_DIRS` is today; the marker names exactly as each tool writes them (grill).
+- `tests/detect-archetype.test.ts` — layer tests:
+  - A route at app/target/route.ts with no sibling marker → `backend` detected. FAILS on base.
+  - A route at app/vendor/route.ts → the same. FAILS on base.
+  - A target/ dir next to Cargo.toml holding more entries than the budget → still skipped, no budget
+    spent (guard).
+  - A .venv/ dir holding pyvenv.cfg → skipped (guard).
+  - The existing pins adjust to the conditional members.
+- `src/lib/capability-index.ts` — layer CLI/lib. The fence is upstream's slice, ported literally:
+  the text must start with `---`, and the frontmatter is everything from the fourth character up to
+  the first newline followed by `---` at or after it — so it closes at the next line that STARTS
+  with `---`, and the rest of the opening line counts as frontmatter (grill). The comment cites
+  `validate.mjs`.
+- `tests/capability-index.test.ts` — layer tests:
+  - A closing `----` parses. FAILS on base.
+  - A closing `--- note` parses. FAILS on base.
+  - A body horizontal rule after a proper close changes nothing (guard).
+  - The empty block is still refused for missing fields (guard).
+  - A differential case runs every fence shape through a verbatim copy of upstream's
+    `parseFrontmatter` and requires the same accept/refuse verdict. The shapes include a field on
+    the opening line itself.
+- `src/commands/update.ts` — layer CLI/commands:
+  - The backup callback first stops `s2` with a neutral phrase (the notice right after it names
+    the count and the directory), prints the notice, then starts `s3` for the writes
+    (`spinnerRef` follows it).
+  - The aborted path keeps the part-way warning (`log.warn`) and the backup pointer (`log.info`) on
+    stderr, and drops only the repeated `.gitignore` hint (grill: an existing test pins both
+    stderr levels).
+- `tests/update.test.ts` — layer tests:
+  - The spinner is stopped before the notice prints: a recording spinner mock keeps one ordered
+    log of stop/info/warn calls, behavior-compatible for every other case. FAILS on base.
+  - An aborted `--force` run prints the `.gitignore` hint once, and the backup dir once on stdout
+    and once on stderr. FAILS on base.
+  - The two existing abort tests keep their stderr assertions.
+- `src/lib/layout.ts` — layer CLI/lib. `capabilitySubtree(paths, role)`, the one role → subtree
+  mapping (refactor, no behavior change). Its callers follow.
+- `src/lib/install-records.ts` — layer CLI/lib. Calls `capabilitySubtree`.
+- `src/lib/install-manifest.ts` — layer CLI/lib. Calls `capabilitySubtree` (two sites).
+- `src/lib/install-capabilities.ts` — layer CLI/lib. Calls `capabilitySubtree`.
+- `src/commands/remove.ts` — layer CLI/commands. `capabilityRelDir` delegates to `capabilitySubtree`.
+- `tests/layout.test.ts` — layer tests. `capabilitySubtree` for both roles × both layouts.
+- `src/lib/symlink-guard.ts` — layer CLI/lib. Comments only: `collectDestDrift` → `scanDest`.
+- `tests/tar-extract.test.ts` — layer tests. The raw U+FEFF becomes a `\uFEFF` escape; the bytes
+  under test are identical.
+- `tests/terminal-safe.test.ts` — layer tests. The raw U+202E / U+200B become escapes; the strings
+  under test are identical.
+- `tests/source-hygiene.test.ts` — layer tests. New: no file under `src/` or `tests/` contains a raw
+  C0 (other than tab, LF, CR), C1, Unicode format (Cf) or U+2028/2029 character. FAILS on base (the
+  three above).
+- `CHANGELOG.md` — `[Unreleased]`: backfilled entries for PHARN-01..18, in the existing
+  Fixed/Security/Changed sections plus `### Added`, one user-facing sentence or two each, written
+  from each commit's diff rather than its title. A change that a later entry already describes, or
+  that this plan changes again, gets ONE entry for the net behavior since 0.5.0 (grill): PHARN-02
+  and -16 with #223, -04 and -17 with #222, -06 with #219, -10 with #220, -11 with #216/#223, -12
+  with #221, -13 with #217, -14 with F23, -15 with F22, -18 with #218.
+- `CLAUDE.md` — the init step-5 carry-over sentence describes #216 and #215; a `frozenCapabilities`
+  sentence goes in the update paragraph; the `add` passage names `scanDest`; the detect-archetype,
+  frontmatter-fence and subtree-helper mentions match this change.
+- `docs/commands/init.md` — layer docs. The skip list says `target`/`vendor`/`venv` are skipped only
+  beside their ecosystem's marker.
+- `docs/troubleshooting.md` — layer docs. The same, in the monorepo paragraph.
+
+## Contracts satisfied
+
+- PHARN-14's "the CLI reads what upstream ships". The fence rule is now the one upstream's CI
+  enforces (cited, P4).
+- PHARN-10's "the backup pointer is printed the moment it exists" is kept. Only its framing changes.
+
+## Evals to write (P1)
+
+- Listed under Files. Seven cases FAIL on the base. The refactor is covered by the existing suites
+  plus `layout.test.ts`.
+
+## Guarantee audit (P0)
+
+- "a hand-authored route under `target/`/`vendor/` is scanned" → floor: detector tests.
+- "the CLI accepts every fence upstream's validator accepts" → floor: a differential test against a
+  pinned copy of upstream's parser. The residual is named: a future upstream parser change is not
+  seen until that copy is refreshed.
+- "no log line is printed while a spinner animates in update's apply" → floor: a call-order test.
+- "no raw invisible character in `src/` or `tests/`" → floor: the hygiene test.
+- CHANGELOG / CLAUDE.md accuracy → advisory (a human reads it). markdownlint is the only floor.
+
+## Trust audit (P2)
+
+- The fence change widens what the untrusted-frontmatter parser accepts to exactly upstream's
+  rule. The field reader, the enums and the duplicate-key refusal are unchanged, and only
+  `name`/`role`/`applies` are read.
+
+## Determinism audit (P5)
+
+- Name membership (sibling markers), line-prefix tests, a code-point class. No fallback guessing.
+
+## Open questions (HALT)
+
+None open. Resolved at GATE 1 (human, 2026-09-25): every question below → **(a)**, the
+recommended answer. Kept for the record:
+
+1. F22 rule. (a) Skip `target`/`vendor`/`venv`/`.venv` only beside their ecosystem's marker, at any
+   depth — recommended. This keeps PHARN-15's budget protection for nested Maven, PHP or Python
+   trees. (b) Skip those four only at the project root. That is simpler, but a nested PHP `vendor/`
+   exhausts the walk budget again. (c) Leave it and document the tradeoff.
+2. F23 direction. (a) Match upstream's `startsWith('---')` rule — recommended. The CLI must accept
+   what upstream ships. (b) Stay strict and give a clearer reason. Upstream-valid capabilities are
+   then still skipped.
@@ -0,0 +1,45 @@
+# REGRESSION — review-cleanups
+
+The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment.
+
+## Base and partition
+
+- **base:** `8b53ba91cf278c270ecc2d0ddda3140f33477a1c` (`HEAD` — `origin/main` after #223; the
+  build is an uncommitted working tree on top of it).
+- **inside** (each declared in `PLAN.md` `## Files`):
+  - `src/lib/detect-archetype.ts`, `src/lib/capability-index.ts`, `src/lib/layout.ts`,
+    `src/lib/install-records.ts`, `src/lib/install-manifest.ts`, `src/lib/install-capabilities.ts`,
+    `src/lib/symlink-guard.ts`, `src/commands/update.ts`, `src/commands/remove.ts`
+  - `tests/detect-archetype.test.ts`, `tests/capability-index.test.ts`, `tests/update.test.ts`,
+    `tests/layout.test.ts`, `tests/tar-extract.test.ts`, `tests/terminal-safe.test.ts`,
+    `tests/source-hygiene.test.ts` (new)
+  - `docs/commands/init.md`, `docs/troubleshooting.md`
+  - `CLAUDE.md`, `CHANGELOG.md`
+- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook
+  scratch) and this feature's own stage artifacts are not build output.
+- **outside gates:** the 46 stdlib `*.test.mjs` / `*.test.cjs` files `scope` returned (754 tests) +
+  whole-repo `validate`; 0 committed eval pairs.
+- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` /
+  `lint:md` are absent from both maps.
+- **environment:** both sides ran with no proxy variables and as root **without**
+  `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`), the CI-equivalent of this
+  root sandbox.
+
+## Per-gate exit codes
+
+| gate       | base | head | flipped? |
+| ---------- | ---- | ---- | -------- |
+| `tests`    | 0    | 0    | no       |
+| `validate` | 0    | 0    | no       |
+
+- `regressions[]`: **empty**
+- `pre_existing[]`: **empty**
+
+## Verdict
+
+**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.**
+(`regression-report.json` `.verdict` = `no-regressions`.)
+
+Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**`
+is owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never
+the increment.