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
57 changes: 57 additions & 0 deletions .dev/features/installer-layout-mirror/GRILL.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
# GRILL — installer-layout-mirror

Plan under interrogation: `.dev/features/installer-layout-mirror/PLAN.md` (trust: untrusted to this griller).
Spec-hash check: `sha256(ARCHITECTURE.md)` = `bca940a5…d729d3c4e` — **matches** the plan's `spec_content_hash` (no drift).
Griller discovery (`count-grillers.mjs .`): 13 registered, all under `test-app/` (installed fixture, not dev-loop grillers) → none applicable; testability applied inline (the plan declares evals, presence recognized).

## Findings — advisory (grill gates nothing)

### P5 — the detection marker is under-specified (important)
```yaml
- type: FINDING
rule_id: "P5"
severity: important
file: ".dev/features/installer-layout-mirror/PLAN.md:10"
problem: "detectLayout keys on 'a pharn/ marker dir exists' without naming the exact marker — a bare pharn/ dir could exist for unrelated reasons, so build must pin a SPECIFIC, unambiguous marker (e.g. pharn/pharn-contracts or pharn/CONSTITUTION.md) to avoid a false pharn detection."
evidence: "detectLayout(rootDir): 'pharn' | 'flat' = a deterministic membership test (a `pharn/` marker dir exists in `rootDir` → `pharn`; else → `flat`…)"
```
The determinism is sound; the *robustness* of the marker is the open detail. Recommend: detect on a leaf the flat layout provably lacks — `pharn/pharn-contracts` (or require ≥2 of the pharn subtrees). A bare-`pharn/`-exists test is the weakest form.

### P1 — the cross-layout degradation is claimed but not in the eval list (minor)
```yaml
- type: FINDING
rule_id: "P1"
severity: minor
file: ".dev/features/installer-layout-mirror/PLAN.md:14"
problem: "The plan asserts diff degrades gracefully when the project layout differs from the @main clone layout ('source missing at @main → skip'), but the Evals section lists no case exercising that cross-layout path — a claimed behavior with no test."
evidence: "Cross-layout clone (project layout ≠ @main layout) degrades via the existing 'source missing at @main → skip' path — a named, pre-existing bound."
```
Recommend either adding one eval (pharn-layout config vs a flat clone → expected: all skipped, no crash) or explicitly labeling it an untested, inherited bound in VERIFY. It leans on pre-existing behavior, so it is low-risk, but the claim should be pinned.

### P7/P6 — the pharn/ path set encodes an UNMERGED, movable layout (important, standing risk)
```yaml
- type: FINDING
rule_id: "P7"
severity: important
file: ".dev/features/installer-layout-mirror/PLAN.md:59"
problem: "The pharn/ constants hard-code PR #86's current subtree paths, but #86 is unmerged and can still change before it lands on main; if a subtree is renamed the pharn/ branch silently mis-mirrors until a follow-up."
evidence: "mirror PR #86's current subtree paths (accepted caveat: if #86 renames a subtree before merge, the `pharn/` path set needs a small follow-up; the flat branch is unaffected)."
```
The human explicitly accepted this at GATE 1, and the flat branch (all live pins) is unaffected — so it is bounded. Recommend a concrete guard: a re-check step (or a note in SHIP) to re-confirm the pharn/ paths against #86 at merge time, so the follow-up is not forgotten.

### P7 — increment size is above the "smallest coherent" bar (minor)
```yaml
- type: FINDING
rule_id: "P7"
severity: minor
file: ".dev/features/installer-layout-mirror/PLAN.md:4"
problem: "All-at-once bundles the write side and the read side (10 src + 7 test files) in one increment, which is larger than P7's 'smallest coherent increment' default."
evidence: "Scope: all-at-once (write + read side) per GATE-1 decision"
```
This is a deliberate, human-approved trade (avoiding a broken status/remove window before #86 merges), and it remains one coherent axis ("layout is resolved, not hardcoded"). Surfaced only so the size is an explicit, eyes-open choice — not a blocker.

## Summary
The plan is sound in its guarantee/trust/determinism audits and preserves P7 for legacy pins by construction (the flat branch is the current behavior). The concerns are: pin a **specific** detection marker (P5, the one I'd act on before build); add or explicitly-bound the cross-layout diff test (P1); and keep the unmerged-#86 dependency visible so the follow-up isn't lost (P7). Nothing here is a correctness defect in the described design; they are hardening + honesty refinements.

## Verdict
ADVISORY VERDICT: 4 concerns raised (0 blocking-severity, 2 important, 2 minor) — for the human to weigh before `/pharn-dev-build`. Advisory end-to-end; gates nothing. Only the writes-scope hook and the spec-hash computation were floor-grade this run.
59 changes: 59 additions & 0 deletions .dev/features/installer-layout-mirror/PLAN.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
# PLAN — installer layout-aware (mirror pharn/ OR flat), all surfaces

- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 (ARCHITECTURE.md)
- increment: Make the CLI **mirror whichever layout the fetched pharn-oss clone has** — the new `pharn/` single-install layout (pharn-oss PR #86 / `pharn-runtime-layout`) OR the current flat layout — across ALL archetype surfaces (install, status/drift, remove), via one deterministic layout resolver, recording the installed layout in `pharn.config.json`. Scope: all-at-once (write + read side) per GATE-1 decision; clone ref stays `main`.
- layer(s): pharn-cli product source — `src/lib/` (resolver, constants, types, capability-index, install-capabilities, diff, pharn-config) + `src/steps/` (install-archetype) + `src/commands/` (status, remove). ARCHITECTURE.md §4.
- constitution_refs: [P7, P5, P2, P1, P3]

## Files

- `src/lib/layout.ts` — layer lib. NEW. `detectLayout(rootDir): 'pharn' | 'flat'` = a deterministic membership test (a `pharn/` marker dir exists in `rootDir` → `pharn`; else → `flat`, the safe legacy default). `layoutPaths(layout): LayoutPaths` = the pure path set (`grillers`, `lenses`, `contracts`, `floor`, `docs[]`) used as BOTH source-relative-to-clone and dest-relative-to-project (the mirror). One axis: resolve the install layout.
- `src/lib/constants.ts` — layer lib. Add the `pharn/` path set (`PHARN_GRILLERS_DIR` = `pharn/pharn-pipeline/grillers`, `PHARN_LENSES_DIR` = `pharn/pharn-review`, `PHARN_CONTRACTS_DIR` = `pharn/pharn-contracts`, `PHARN_FLOOR_DIR` = `pharn/floor`, `PHARN_TRUSTED_DOCS` = [`pharn/CONSTITUTION.md`, `pharn/ARCHITECTURE.md`]) beside the existing flat constants (flat values unchanged — the legacy branch). `.claude/*` constants unchanged (identical both layouts).
- `src/lib/capability-index.ts` — layer lib. Build `SUBTREES` from `layoutPaths(detectLayout(repoDir))` so a `pharn/` clone enumerates capabilities. All untrusted-frontmatter validation preserved (P2).
- `src/lib/install-capabilities.ts` — layer lib. Drive every copy from `layoutPaths(detectLayout(repoDir))`: grillers/lenses/contracts/floor/docs mirrored clone→project at identical relative paths; in `pharn` the docs set is `pharn/CONSTITUTION.md` + `pharn/ARCHITECTURE.md` only (THREAT-MODEL/LIMITS are not under `pharn/` → dropped, matching PR #86). Preserve ALL hardening: symlink-reject, `safeJoin`, test-file exclusion, settings-preserve. Return the detected layout to the caller.
- `src/lib/diff.ts` — layer lib. `diffInstalledCapabilities` takes the project's `layout` and derives the expected set via `layoutPaths(layout)` (capability subtree + contracts + floor + docs), for both clone-source and project-dest (same relative path). The module-path `diffInstalled` is untouched (modules install into `.claude/`, layout-invariant). Cross-layout clone (project layout ≠ @main layout) degrades via the existing "source missing at @main → skip" path — a named, pre-existing bound.
- `src/lib/pharn-config.ts` — layer lib. Read/write/validate the additive optional `layout?: 'pharn' | 'flat'` field (schema stays additive — legacy configs omit it and still load; a missing value means `flat`, P7).
- `src/types.ts` — layer lib. Add `layout?: 'pharn' | 'flat'` to the `PharnConfig` interface.
- `src/steps/install-archetype.ts` — layer steps. Record the detected layout (returned by `installCapabilities`) into `pharn.config.json`.
- `src/commands/status.ts` — layer command. Pass `config.layout ?? 'flat'` to `diffInstalledCapabilities`.
- `src/commands/remove.ts` — layer command. `removeCapability` derives the capability subtree from `layoutPaths(config.layout ?? 'flat')` (the only layout-sensitive removal path; `removeModule` is layout-invariant).
- `tests/layout.test.ts` — layer test. NEW. Detection (pharn marker → pharn; none → flat) + `layoutPaths` sets for both.
- `tests/install-capabilities.test.ts` — layer test. Add a `pharn/`-nested fixture: assert mirror under `pharn/…`, THREAT-MODEL/LIMITS dropped, `.claude/` unchanged; the existing flat fixture still installs flat (P7 guard).
- `tests/capability-index.test.ts` — layer test. A `pharn/` fixture enumerates capabilities identically to flat.
- `tests/diff.test.ts` — layer test. `diffInstalledCapabilities` with `layout: 'pharn'` compares the `pharn/` paths.
- `tests/status.test.ts` — layer test. An archetype config with `layout: 'pharn'` drives the pharn-path drift check.
- `tests/remove.test.ts` — layer test. Removing a capability from a `pharn`-layout config deletes the `pharn/…` dir.
- `tests/pharn-config.test.ts` — layer test. `layout` round-trips; a legacy config WITHOUT it still loads (P7).

## Contracts satisfied

- None new — CLI product code, not a `pharn-contracts` schema. It **upholds** the ownership boundary (P3, cited): pharn-oss owns the layout; the CLI mirrors it structurally, never rewriting copied file contents.

## Evals to write (P1)

- Resolver → pharn-marker fixture → pharn set + `isPharn`; no marker → flat set (deterministic else).
- install-capabilities → pharn fixture → mirrored under `pharn/`, THREAT-MODEL/LIMITS absent, `.claude/` intact; flat fixture → unchanged (P7 guard).
- capability-index → pharn fixture → capabilities enumerated identically.
- diff/status → `layout:'pharn'` → expected set on the `pharn/` paths.
- remove → `layout:'pharn'` capability → deletes `pharn/pharn-review|pharn-pipeline/grillers/<name>`.
- pharn-config → `layout` round-trips; legacy config lacking it loads (P7).

## Guarantee audit (P0)

- "installs/status/remove mirror the clone-or-project layout" → **floor**: deterministic detection (membership on a `pharn/` marker) + `layoutPaths` pure map + vitest over both layouts across install/status/remove. Not advisory.
- "old flat pinned SHAs keep working (P7)" → **floor**: the resolver's else-branch IS the current flat behavior; a flat fixture installs/diffs/removes byte-identically (regression tests), and a legacy config (no `layout`) loads and resolves to flat.
- "nothing escapes `safeJoin`; symlinks rejected" → **floor**: existing `safeJoin`/`isSymlink` guards preserved on every copy and delete; re-asserted by the pharn-fixture tests.
- "`layout` config field is additive" → **floor**: a test loads a legacy config without it (P7).
- No new safety/trust guarantee over copied CONTENT is introduced — contents stay mirrored verbatim, never executed.

## Trust audit (P2)

- Input: the fetched pharn-oss clone (untrusted). Layout detection reads only **path existence** (a membership test), never file content — no untrusted bytes drive the branch. Every copy/delete stays `safeJoin`- + symlink-guarded; capability names stay validated against `CAPABILITY_NAME_RE` before any path-join. The project's `layout` field is read from the CLI-owned `pharn.config.json` (validated to the `{pharn,flat}` enum, defaulting flat), not from untrusted remote content. No new taint surface; no guaranteed decision rests on a tainted field.

## Determinism audit (P5)

- Every layout branch is a membership test: a `pharn/` marker exists → `pharn`; else → `flat` (safe legacy default, deterministic else). The config `layout` is enum-validated `{pharn, flat}`, missing → flat. No LLM, no classifier, no guess.

## Open questions (HALT)

- None — resolved at GATE 1: all-at-once (read-side included), clone ref stays `main`, mirror PR #86's current subtree paths (accepted caveat: if #86 renames a subtree before merge, the `pharn/` path set needs a small follow-up; the flat branch is unaffected).
28 changes: 28 additions & 0 deletions .dev/features/installer-layout-mirror/REGRESSION.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
# REGRESSION — installer-layout-mirror

**Verdict (FLOOR, `check-regress.mjs verdict`):** `no-regressions` — exit `0`. Pure exit-code comparison, zero LLM judgment.

## Base + scope partition

- **base:** `b739a6f` — current HEAD (working tree dirty with the feature's uncommitted changes, so `base = HEAD` and the diff is against the working tree).
- **inside (declared `## Files`, 17):** the 10 src files + 7 test files of the increment. `scope` exit `0`, `escaped: []` — the build stayed within its declared writes (fix #7). `.pharn/` scratch and the feature-artifact dir are pipeline bookkeeping, correctly excluded from `--changed`.
- **outside gate set:** `tests` (44 stdlib `node --test` floor + hook files) and `validate`. Style gates skipped — `inside` touches no shared style config (`eslint.config.mjs`, `.prettierrc.json`, `.prettierignore`, `.markdownlint-cli2.jsonc`). No outside eval pairs.

## Per-gate base → head (exit codes)

| gate | base | head | classification |
| ---------- | ---- | ---- | -------------- |
| `tests` | 1 | 1 | **pre_existing** (RED→RED, not a flip) |
| `validate` | 0 | 0 | clean (GREEN→GREEN) |

`regressions[]`: **none**. `pre_existing[]`: `tests`.

## Why the `tests` gate is RED at baseline (not this feature)

Identical to the previously-shipped `remove-dead-docs-url` increment: the `tests` aggregate is red **independently of this increment** — red with the feature's edits and red with them reverted (measured same-environment). The failing file is `.dev/floor/lens-scanner-map.test.mjs`, which is explicitly **non-hermetic** — it runs `count-lenses.mjs` over the repo root, which recursively includes the gitignored `test-app/` install fixture (22 lenses there vs the committed empty `lens-scanner-map.json`). This increment touches only `src/*.ts` and vitest `tests/*.test.ts` — **no** `.mjs`/`.cjs` node test and **no** PHARN markdown capability, so it provably cannot move the `tests` or `validate` exit codes.

- **Sound, same-environment measurement.** Base and head were both measured in the working tree (so gitignored `test-app/` is present on both sides), differing only by the feature's tracked edits — reverted in place via a guarded `git stash`, then restored (verified). The `git worktree` baseline the command sketches is confounded here (the worktree omits gitignored `test-app/`, manufacturing a phantom `0→1` flip); the same-environment measurement is the apples-to-apples comparison the regress guarantee requires, and it yields `tests` `1 → 1` (pre-existing).

## Honest residual (P0/P7)

`/pharn-dev-regress` catches exactly what its deterministic suite catches — nothing more. It certifies the base→head comparison over `{tests, validate}`, **not** the feature as a whole. The feature's real correctness surface (the vitest suite, incl. the new layout/install/diff/status/remove/config tests) is owned by `/pharn-dev-verify`'s `test` gate, not this stage. The pre-existing `lens-scanner-map` drift is out of this increment's scope.
42 changes: 42 additions & 0 deletions .dev/features/installer-layout-mirror/REVIEW.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
# REVIEW — installer-layout-mirror

**Increment (trust: untrusted to this review):** make the CLI mirror the fetched clone's layout (flat OR the new `pharn/`) across install/status/remove, via one resolver, recording `layout` in config. Diff: 9 files modified (+106/−44) + `src/lib/layout.ts` (83 lines).

**Step 1 — Floor first (P0):** `node .dev/floor/validate.mjs .` → exit `0`, **GREEN**. The increment legitimately reached review. The floor is the only guaranteed part; everything below is advisory.

## Four lenses

### L-floor → P0 — no finding
Every guarantee reduces to a floor primitive or is labeled advisory. Detection → membership test on the `pharn/pharn-contracts` marker + vitest over both layouts. P7 (old flat pins) → the resolver's `else` **is** the current flat behavior, backed by a flat-clone regression test and a legacy-config-loads test. `safeJoin`/symlink guards preserved on every copy and delete and re-asserted. The one honestly-advisory item — the `pharn/` path set tracks an **unmerged** PR #86 — is labeled advisory in GRILL.md/VERIFY.md, not sold as a guarantee. No unlabeled guarantee. Clean.

### L-eval → P1 — no finding
No new Capability or `rule_id` (this is CLI product code), so there is no eval binding to miss; the floor agrees (`validate` GREEN). Every new behavior ships a vitest test in the same increment: `detectLayout`/`layoutPaths`/`configLayout` (incl. the no-false-positive + empty-dir cases), `installCapabilities` for pharn (mirror + THREAT-MODEL/LIMITS drop + `.claude/` kept) and the flat P7 guard, `capability-index` pharn enumeration, `diff` pharn + cross-layout degradation, `status` layout pass-through, `remove` capability at both layouts, `pharn-config` round-trip/legacy/garbage. 616/616 green. Clean.

### L-trust → P2 — no finding
The fetched clone is untrusted, and `detectLayout` reads only **path existence** against a **fixed constant** (`pharn/pharn-contracts`) — no untrusted byte drives the branch, no injection sink. Every copy/delete stays `safeJoin`- + symlink-guarded; `capability-index`'s strict frontmatter validation is untouched; the project `layout` is read from the CLI-owned config and enum-coerced (`configLayout` / `pharn-config` drop-garbage). No guaranteed decision rests on a tainted/free-text field. No instruction-looking content in the diff changed this reviewer's behavior. Clean.

### L-axis → P3 — no finding
Each file changes for the single reason "resolve the install layout rather than hard-code it." Layout logic is centralized in the new `lib/layout.ts` and reached from `lib/`, `steps/`, and `commands/` — no `command→command` or `step→step` sibling coupling is introduced (the `command→step` imports present are the sanctioned direction). `install-capabilities` delegates detection to `layout.ts` rather than growing a second axis. Clean.

## Gates (fix #3)

- **floor-gate (blocking):** none.
- **advisory-gate (warn):** one, below.

```yaml
- type: FINDING
rule_id: "P6"
severity: minor
file: "src/commands/add.ts:351"
problem: "`pharn add` calls installCapabilityDirs with the default layout, which detects from the CLONE (@main), not the project's recorded config.layout — so during the flat→pharn transition `add` could install a capability at the clone's layout into a project of the other layout."
evidence: "installCapabilityDirs(repoDir, cwd, [{ name: cap.name, role: cap.role }]);"
```
**Advisory, out of this increment's scope:** `add.ts` was not in the plan's `## Files`, and the mismatch is only reachable in the transition window (default clone is `main`, flat today; and `add` already was not layout-aware before this change). Recommended **follow-up** (increment 3): thread `configLayout(config)` into `add`'s `installCapabilityDirs` call, mirroring what `remove`/`status` now do — the optional `paths` param added this increment already makes that a one-line change. Surfaced for the human; it blocks nothing.

## Verdict

**GREEN — 0 floor findings, 0 blocking.** The increment satisfies its plan (17 files), the floor is GREEN, verify PASSed, and regress found no feature-attributable regression. The one advisory finding is an out-of-scope follow-up. Merge / fix / abandon is the human's call (GATE 2).

## Lessons

- **No new canon lesson.** The regress `test-app`/worktree confound this run re-encountered is already recorded as a proposed candidate in `.dev/features/remove-dead-docs-url/REVIEW.md` (this run is a second occurrence, strengthening that provenance) — not re-proposed here to avoid duplication. The `add` cross-layout gap above is a specific follow-up, not a recurring-failure pattern warranting canon.
Loading