Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions .dev/features/init-manual-carry/GRILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# GRILL — init-manual-carry

Plan: `.dev/features/init-manual-carry/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live
`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only.

## Findings

```yaml
- type: FINDING
rule_id: 'P0'
severity: important
file: '.dev/features/init-manual-carry/PLAN.md:52'
problem: 'The fingerprint and the parse are two separate reads. If the parse runs first and the file changes before the fingerprint is taken, the carry set comes from the OLD bytes while the fingerprint (and so the under-lock check) blesses the NEW ones — the concurrent write is lost with no refusal. Take the fingerprint BEFORE the parse, or derive both from one read, so any change in between fails closed.'
evidence: 'the previous `(skillsVersion, commit)` stamp, and a config fingerprint'
- type: FINDING
rule_id: 'P5'
severity: important
file: '.dev/features/init-manual-carry/PLAN.md:6'
problem: 'update''s row 0 keeps ANY frozen entry (`frozen | any source → KEEP VERBATIM`), but the plan keeps only `source: manual` ones; a frozen AUTO entry (update kept it earlier) is still dropped by a re-run init and its files orphaned. Either extend the kept set to any source or name the gap — this is a question for the human, since GATE 1 approved the manual-only wording.'
evidence: 'A manual entry whose capability upstream ships but this CLI cannot parse is KEPT as-is'
- type: FINDING
rule_id: 'P3'
severity: minor
file: '.dev/features/init-manual-carry/PLAN.md:48'
problem: 'The carry computation (manualKeys / extra / kept / gone) is pure set logic over the previous config and the index; kept inside the command it is testable only through the fully mocked runInit. Acceptable, but keep it a pure function with no I/O besides the one tolerant read so the tests can pin every bucket.'
evidence: 'replace `carriedManualCapabilities` with one tolerant read of the previous config returning'
- type: FINDING
rule_id: 'P6'
severity: minor
file: '.dev/features/init-manual-carry/PLAN.md:59'
problem: 'Records for a kept entry are selected by prefix at the NEW clone layout; if the re-run init also moves the install from flat to `pharn/`, the kept capability''s files and records sit at the old paths and nothing is carried. update behaves the same, so this is parity, but it should be named rather than implied away.'
evidence: 'merge their records from the previous store (`recordsBaseline` + `recordsUnderCapabilities`, the pair `update` uses)'
```

## Summary

The plan closes a reproduced provenance bug (manual → auto) and a reproduced lost-write race, and brings
init into line with update's merge table for hand-added capabilities. The one soundness concern is the
order of the fingerprint and the parse: only fingerprint-first (or a single read) makes the new lock
check fail closed. The open design question is whether frozen AUTO entries get the same treatment as
frozen manual ones — update keeps both; the approved plan covers manual only. The other two are scope
notes.

ADVISORY VERDICT: 4 concerns raised (0 blocking-severity, 4 advisory — 2 important) — for the human to
weigh before /pharn-dev-build.
121 changes: 121 additions & 0 deletions .dev/features/init-manual-carry/PLAN.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# PLAN — init-manual-carry (a re-run init keeps `pharn add`s the way `update` does)

- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4
- increment: PHARN-11's carry-over is made consistent with `update`'s merge table and with PHARN-03's
lock contract. (1) A `source: "manual"` entry stays `manual` even when archetype resolution also
selects it (merge row 3, sticky). (2) An entry — of ANY `source`, manual or auto (amended at
grill, see Open questions) — whose capability upstream ships but this CLI cannot parse is KEPT
as-is — config entry VERBATIM, files and records untouched, listed in `frozenCapabilities` so
`update` re-checks it — instead of silently dropped (merge row 0, `frozen | any source`). (3) A
manual entry upstream no longer ships is still dropped, but NAMED (merge row 7's `dropped-gone`).
(4) Carried entries render as "added by hand" in the summary and no longer also appear under
SKIPPED. (5) Inside its lock, init refuses (nothing written) if `pharn.config.json` changed after
init read it for the carry-over. (6) The two docs that still say re-init resets every entry to
`auto` are corrected.
- layer(s): the CLI (`src/commands/init.ts`, `src/steps/`, `src/lib/pharn-config.ts`, `src/types.ts`)
+ user docs
- constitution_refs: [P1, P4, P5, P6, P7]

## Discovery — verified this run (P6)

- `init.ts:243-266` `carriedManualCapabilities` keeps a previous entry only if
`source === 'manual' && inIndex && !selected`, and ONLY the carried keys become `manualKeys`
(`init.ts:156`). `install-archetype.ts:135-140` stamps everything else `auto`. So a manual entry
that resolution ALSO selects is re-recorded `auto` — reproduced by review: `[spa]` + `add
griller:a11y`, project gains ssr, re-run init → a11y `auto`; a later `update` then reports
"REMOVED — no longer selected" and drops a capability the user asked for by name. update's own
table keeps it `manual` (`merge-capabilities.ts:200-205`, row 3).
- `inIndex = index.capabilities` only (`init.ts:255`). A manual entry in `index.unknown` (upstream
ships it, this CLI cannot parse it) is dropped from the config and its files orphaned; `update`
KEEPS the same entry (row 0, `kept-frozen`) with its records (`update.ts:558-580`) and
`frozenCapabilities` (`update.ts:584-600`). Reproduced by review (add lens:perf → upstream gives
perf an unparseable `applies` → `update` KEPT, re-init drops it).
- A manual entry gone from the index is dropped silently; `update` names it (`dropped-gone`).
- Display: carried entries are appended with `matched: []` (`init.ts:151`) → empty reason
(`archetype-summary.ts:16-17`), while `...resolved` (`init.ts:144`) still lists the same
capability under SKIPPED ("applies to [ssr]; detected [spa]").
- Lock: init reads the config (`init.ts:137` → `:249`) before its prompts and takes the lock only
after them (`:197`), but never calls `assertConfigUnchanged` — `add.ts`, `update.ts:308` and
`remove.ts` all do (PHARN-03). Reproduced by review: a concurrent `pharn add lens:perf` during
init's confirm is lost when init writes its config. `assertConfigUnchanged` compares PARSED configs
and throws when the current one is unreadable (`pharn-config.ts:371-387`); init reads TOLERANTLY
(absent/corrupt → carry nothing), so it needs a comparison that also covers absent/unreadable.
- Docs: `docs/reference/pharn-config.md:58-60` ("every entry becomes `auto` and previous `manual`
tags are lost") and `docs/commands/init.md:150` ("only `pharn add` writes `manual`") contradict
`docs/commands/init.md:137-138` — a P4 contradiction.

## Files

- `src/commands/init.ts` — replace `carriedManualCapabilities` with one tolerant read of the
previous config returning: `manualKeys` (every manual entry the index still has, selected or not),
`extra` (those not selected — appended with `matched: 'manual'` and REMOVED from `skipped`),
`kept` (entries of any `source` in `index.unknown`), `gone` (manual entries in neither — named in a
warning), the previous `(skillsVersion, commit)` stamp, and a config fingerprint taken BEFORE the
parse (grill finding 1 — a change between the two reads must fail closed); inside
`withProjectLock`, assert the fingerprint is unchanged BEFORE `runInstallArchetype` — layer
CLI/commands
- `src/lib/pharn-config.ts` — `configFingerprint(cwd)` (`absent` | `unreadable` | sha256 of the
bytes; never throws) + `assertConfigFingerprintUnchanged(cwd, fingerprint, command)` throwing the
existing `ProjectChangedError` message — layer CLI/lib
- `src/steps/install-archetype.ts` — take a `carry` argument (`manualKeys`, `kept`, previous stamp)
in place of bare `manualKeys`; append `kept` entries verbatim to `capabilities`, write
`frozenCapabilities` (sorted keys) when non-empty, and merge their records from the previous store
(`recordsBaseline` + `recordsUnderCapabilities`, the pair `update` uses) into the new store — layer
CLI/steps
- `src/steps/archetype-summary.ts` — `describeMatched('manual')` → "added by hand" — layer CLI/steps
- `src/types.ts` — `SelectedCapability.matched: 'universal' | 'manual' | Archetype[]` — layer types
- `tests/init.test.ts` — manual+resolved → in `manualKeys` (FAILS on base); carried entry absent
from `skipped`, `matched: 'manual'`; unparseable entry (manual AND auto) → passed as `kept`
verbatim, named; gone manual →
named warning; config changed during the prompts → refusal, `runInstallArchetype` never called,
exit 1 (FAILS on base); config unchanged/absent/corrupt → proceeds
- `tests/init-archetype.test.ts` — `kept` entry written verbatim + `frozenCapabilities`, its records
carried from a stamp-valid store, dropped from a stale/absent one (never minted)
- `tests/archetype-summary.test.ts` — a `'manual'` entry renders "added by hand"
- `tests/pharn-config.test.ts` — fingerprint: absent / bytes / unreadable (directory); assert throws
`ProjectChangedError` on any change, including absent→present
- `docs/commands/init.md` — the re-run paragraph + the config table row describe (1)-(5)
- `docs/reference/pharn-config.md` — replace the "start-over … every entry becomes `auto`" note
- `CHANGELOG.md` — `[Unreleased]` → `### Fixed` entries

## Contracts satisfied

- `lib/merge-capabilities.ts` decision table rows 0, 3, 7 — init now agrees with them (cited, not
restated, P4). Legacy `source`-absent entries are still NOT carried: the merge is "the ONLY place
absence may be resolved" (CLAUDE.md), and `docs/commands/init.md` already limits the carry to
`source: "manual"`.
- PHARN-03's "re-check the config under the lock" — extended to init.

## Evals to write (P1)

- listed under Files; the manual+resolved and config-changed cases fail on 0ca29b7.

## Guarantee audit (P0)

- "a manual entry the index still has is recorded manual" → floor: set membership over `role:name`.
- "init writes nothing if the config changed after it was read" → floor: sha256 content-hash
compare of the file bytes (or the absent/unreadable sentinel) inside the lock, before the first
write (the backup included).
- "a kept entry's records are carried" → floor: `recordsBaseline`'s stamp compare; a stale/absent
store carries nothing (never minted, never blessed).

## Trust audit (P2)

- The previous config and records are local, user-editable input, already validated at ingest
(`readPharnConfig`, `readRecords`); no new field is trusted. Upstream names in `index.unknown` are
only used as set keys against config entries that passed `CAPABILITY_NAME_RE`.

## Determinism audit (P5)

- Every branch is a `role:name` set lookup or an exact enum compare; the lock check is a string
equality over fingerprints.

## Open questions (HALT)

- none. RESOLVED at GATE 1 (2026-09-24, by the human): (2) a hand-added capability that upstream
still ships but this CLI cannot parse is KEPT as-is, like `update` — "Leave it alone", the
recommended option — not dropped with a warning. The plan was accepted as written.
- AMENDED after `/pharn-dev-grill` (2026-09-24, by the human): grill finding 2 asked whether an
AUTOMATIC (`source: auto`, or absent) entry that upstream ships but this CLI cannot parse gets the
same treatment; the human chose "Yes, leave them alone too" — so (2) keeps a frozen entry of ANY
source verbatim, exactly as `update`'s row 0 does. Files unchanged.
40 changes: 40 additions & 0 deletions .dev/features/init-manual-carry/REGRESSION.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# REGRESSION — init-manual-carry

The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment.

## Base and partition

- **base:** `f9b1bbb397ec7b659b99a8e7bfc0847ebe9976d5` (`origin/main` after #214 and #215, merged into this
branch while its PR was open). Re-run after that merge; the first run, against `5cf6ce3` (`origin/main`
after #213), gave the same verdict.
- **inside** (each declared in `PLAN.md` `## Files`): `src/commands/init.ts`, `src/lib/pharn-config.ts`,
`src/steps/install-archetype.ts`, `src/steps/archetype-summary.ts`, `src/types.ts`, `tests/init.test.ts`,
`tests/init-archetype.test.ts`, `tests/archetype-summary.test.ts`, `tests/pharn-config.test.ts`,
`docs/commands/init.md`, `docs/reference/pharn-config.md`, `CHANGELOG.md`.
- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook scratch) and
stage artifacts are not build output — this feature's own `PLAN.md` / `GRILL.md`, and the untracked
`PLAN.md` files of the two increments still queued (`update-frozen-recheck`, `tar-entry-names`).
- **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.
84 changes: 84 additions & 0 deletions .dev/features/init-manual-carry/REVIEW.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
# REVIEW — init-manual-carry

Increment: `src/commands/init.ts` (`readPreviousConfig` — fingerprint first, then a tolerant parse;
pure `carryOver`; the under-lock `assertConfigFingerprintUnchanged`), `src/lib/pharn-config.ts`
(`configFingerprint`, `assertConfigFingerprintUnchanged`, shared `configChangedError`),
`src/steps/install-archetype.ts` (`InstallCarry`; kept entries verbatim, `frozenCapabilities`, carried
records), `src/steps/archetype-summary.ts` + `src/types.ts` (`matched: 'manual'` → "added by hand"),
four test files, `docs/commands/init.md`, `docs/reference/pharn-config.md`, `CHANGELOG.md`. Treated as
`trust: untrusted`; nothing in it read as an instruction.

## Floor first (P0)

`node .dev/floor/validate.mjs .` → `FLOOR: GREEN` (exit 0). `/pharn-dev-build`'s `npm run check` exit 0
(1486 tests), `/pharn-dev-regress` `no-regressions`, `/pharn-dev-verify` `PASS`.

## Floor-gate findings (blocking)

None.

- L-floor (P0): each claim reduces to a floor primitive. "A manual entry the index still has is recorded
`manual`" → `role:name` set membership (`carryOver`). "init writes nothing if the config changed after
it was read" → a sha256 content-hash compare (or the `absent` / `unreadable:<code>` sentinel) inside the
lock, before `runInstallArchetype` and so before the backup. The fingerprint is taken before the parse,
so a change between the two reads fails closed (grill finding 1). "A kept entry's records are carried"
→ `recordsBaseline`'s stamp compare, and a stale or absent store carries nothing. The only unclaimed
residual is an edit made DURING the install, inside the lock — the same window every other writer has.
- L-eval (P1): every behavior has a test that fails on the base source (8 in `tests/init.test.ts`, checked
against the stashed base `src/`): sticky manual, the carried entry shown once, kept entries of every
source passed verbatim, the gone warning, the corrupt-config carry, and both refusal paths
(rewritten; appeared). The fixture tests pin the written config, `frozenCapabilities` and the three
record-carry cases; `tests/pharn-config.test.ts` pins the fingerprint's sentinels.
- L-trust (P2): kept entries come from the local config, already validated at ingest
(`CAPABILITY_NAME_RE`, `ROLE_VALUES`), and are written back verbatim. Upstream names from
`index.unknown` are only set keys, matched against those validated entries. Every printed key comes
from a validated entry.
- L-axis (P3): no sibling reference. The command imports a type from the step it already calls (the
allowed direction).

## Advisory findings (warn — severity is this reviewer's judgment, fix #3)

```yaml
- type: FINDING
rule_id: 'P4'
severity: minor
file: 'CLAUDE.md:54'
problem: 'CLAUDE.md still describes the re-run carry as manual entries only, via `manualKeys`; it now also keeps unparseable entries of any source (frozenCapabilities + records) and re-checks the config fingerprint under the lock. CLAUDE.md is outside this plan''s Files, so it needs a follow-up edit.'
evidence: 'to keep its `source: ''manual''` entries that the index still has — installed again and recorded `manual` via `manualKeys`'
- type: FINDING
rule_id: 'P3'
severity: minor
file: 'src/commands/init.ts:311'
problem: 'carryOver is pure membership logic sitting in a command, so it is tested only through the fully mocked runInit; a lib module beside merge-capabilities would allow direct table tests. Accepted at grill (finding 3); noted for when the table grows.'
evidence: 'function carryOver('
- type: FINDING
rule_id: 'P1'
severity: minor
file: 'src/commands/init.ts:326'
problem: 'The first-entry-wins rule for a duplicated `role:name` in the previous config has no test; it mirrors the merge, but only the merge''s copy of the rule is pinned.'
evidence: 'if (seen.has(k)) continue;'
```

## Proposed lesson for canon (NOT written — for a human-gated `/pharn-dev-memory-promote`)

- **Candidate:** "A command that reads shared state before a prompt and writes it after must re-check
that state under the lock (PHARN-03). Adding a pre-prompt read to a command that had none (PHARN-11 gave
`init` a config read for its carry-over) silently re-opens the lost-write race."
- **Provenance:** increment `init-manual-carry`; the gap entered with PHARN-11 (8d11906, #205), a few
commits after PHARN-03 (71bc375, #197) set the contract for `add`/`update`/`remove`; found by the
18-commit review on 2026-09-24; closed by this diff (`src/commands/init.ts:224`).

## After merging `origin/main` (#214, #215) into the PR branch

PR #215 landed on `main` while this PR was open and touched the same files: init now also copies the config
keys pharn does not own (`testResults`, `ship`) across a re-run. The conflicts were resolved without
changing either behavior: both helpers are kept (`keptRecords`, `readCarriedEntries`), and the three doc
passages and the CHANGELOG now describe both carry-overs. Two of #215's comments were updated: one cited
the removed `carriedManualCapabilities`, the other said a key edited while a prompt is open is carried
(under init that edit now makes init refuse first, so the key is still not lost). One test was added:
kept entries and user-owned keys both survive one re-run. Floor re-run on the merged tree: `npm run check`
exit 0 (1495 tests), regress `no-regressions` against `f9b1bbb`, verify `PASS`.

## Verdict

**GREEN — 0 floor-gate findings, 3 advisory (minor).** The standing decision is the human's (GATE 2).
Loading
Loading