diff --git a/codev/plans/1210-codev-doctor-detect-protocol-f.md b/codev/plans/1210-codev-doctor-detect-protocol-f.md new file mode 100644 index 000000000..86948e0c5 --- /dev/null +++ b/codev/plans/1210-codev-doctor-detect-protocol-f.md @@ -0,0 +1,298 @@ +# Plan: `codev doctor` — detect protocol-file drift + +## Metadata +- **ID**: plan-2026-07-22-codev-doctor-detect-protocol-f +- **Status**: draft +- **Specification**: [codev/specs/1210-codev-doctor-detect-protocol-f.md](../specs/1210-codev-doctor-detect-protocol-f.md) +- **Created**: 2026-07-22 +- **Issue**: #1210 + +## Executive Summary + +Implements the spec's **Approach 1** (skeleton-driven diff via a new pure audit lib), combined with +Approach 2's quiet-by-default gating (the Framework Drift section prints nothing unless there is +something actionable — a local shadow exists **or** the skeleton is behind; see the "no local +overrides" decision below). The work +splits into three independently-testable phases: + +1. A pure **`protocol-drift-audit` library** that detects shadow drift (local `.codev/` / `codev/` + copies that also exist in the installed skeleton) and classifies each as `identical` (redundant) + or `differs` (adjudicate) — plus a **staleness** helper (installed vs npm-latest version). +2. **Wiring** the drift report into `codev doctor` as a new section, rolled into the existing warning + summary; report-only, no file mutation. +3. **Tests** (unit for the lib, e2e/CLI for the doctor integration) covering the spec's seven + functional scenarios and the two non-functional ones. + +The design deliberately mirrors the three existing precedents — `lib/pr-gate-audit.ts` (#943), +`lib/framework-ref-audit.ts` (#1011), `lib/gitignore.ts` — each a pure lib (findings + formatter) +consumed by `doctor.ts`. It reuses the resolver primitives in `lib/skeleton.ts` so the audit's notion +of "the skeleton" is identical to runtime resolution. + +**Item 2 of the issue (known-default / historical-hash detection)** is explicitly marked non-blocking +by the spec and is **deferred to a follow-up** (see Notes) — it layers on the same findings and needs +a release-time hash-manifest step that would inflate this PR. + +## Success Metrics +- [ ] All specification success criteria met (shadow-drift identical/differs/no-op, dual override + roots, resources excluded, staleness behind/offline, no file mutation). +- [ ] New `lib/protocol-drift-audit.ts` is a standalone unit-tested module (findings + formatters). +- [ ] `codev doctor` surfaces a Framework Drift section; drift findings increment the warning count. +- [ ] Existing doctor unit + e2e tests continue to pass; new tests added. +- [ ] Zero file mutation of user content by the drift check (asserted by test). +- [ ] No hang when offline (staleness bounded by a ~2–3s timeout). + +## Key Design Decisions (resolving spec open questions + consult feedback) + +- **Byte-comparison semantics (Gemini pt 1 / spec risk)**: compare **raw bytes** (SHA-256 of file + contents). A local copy is called `identical`/redundant **only** when byte-for-byte equal to the + skeleton file. Anything differing — including EOL-only (CRLF vs LF) or trailing-newline differences + — is classified `differs` → adjudicate. Rationale: the `identical` verdict carries a "safe to + remove" suggestion; being conservative (never suggest removing a file that isn't a perfect + duplicate) is the safe failure direction. This is documented in the lib so the choice is explicit. +- **Dual override roots (Codex pt 1 & 5)**: scan **both** `.codev/` and `codev/` for each + skeleton file; emit a finding per local copy found, tagged with its tier. Also compute which copy + the resolver actually resolves (the "winner") via `resolveCodevFile`, and mark it, so the human sees + which is live. No-op/presence detection checks **both** roots (not just `hasLocalOverride`'s tier-2). +- **Scan set (Codex pt 2)**: pinned in the spec — skeleton `protocols/`, `consult-types/`, `roles/` + trees, all `.md`/`.json` files. Enumerated via `listSkeletonFiles(subdir)`. `resources/` is + **excluded** (user-evolved files). +- **Staleness output (Codex pt 3 / Gemini pt 2)**: report explicit `installed X; latest Y` and a + boolean "behind", not a computed distance. `npm view @cluesmith/codev version` via `spawnSync` in + **argv form** (no shell string) with a ~2500ms timeout; any failure/timeout/offline → `latest: null` + and a neutral "could not check (offline?)" line, never a doctor failure or hang. +- **Item 2 non-blocking (Codex pt 4)**: deferred; see Notes. +- **"No local overrides" behavior — single, unambiguous rule (Codex plan review, REQUEST_CHANGES)**: + the Framework Drift section is **quiet by default**. doctor computes shadows + staleness, then: + - **No shadows AND not-behind** (up-to-date, or offline/uncheckable) → **print nothing** (no header, + no lines). This is the spec's "true no-op". + - **Otherwise** → print the section: shadow lines (differs=warn, identical=info) and, only if + `behind`, a staleness warning. Staleness is **never** printed unconditionally-per-run: it is + silent when up-to-date or uncheckable, and a warning only when genuinely behind (the issue's + sibling failure mode). This removes the earlier draft's self-contradiction (Exec Summary "no-op" + vs Phase 2 "staleness always shown"). + +## Phases (Machine Readable) + +```json +{ + "phases": [ + {"id": "phase_1", "title": "protocol-drift-audit library (shadow drift + staleness)"}, + {"id": "phase_2", "title": "Wire drift report into codev doctor"}, + {"id": "phase_3", "title": "Tests: unit (lib) + e2e (doctor integration)"} + ] +} +``` + +## Phase Breakdown + +### Phase 1: `protocol-drift-audit` library (shadow drift + staleness) +**Dependencies**: None + +#### Objectives +- Provide a pure, side-effect-free module that computes framework-file drift and skeleton staleness, + returning structured findings plus display formatters — the single source of truth both `doctor` + (and, optionally later, `update`) consume. + +#### Deliverables +- [ ] New file `packages/codev/src/lib/protocol-drift-audit.ts` exporting: + - `FRAMEWORK_DRIFT_DIRS = ['protocols', 'consult-types', 'roles']` (the pinned scan set — documented + maintenance point). + - `auditProtocolDrift(workspaceRoot: string): DriftFinding[]` — for each skeleton file under the + scan set (`listSkeletonFiles(subdir)`), check `.codev/` and `codev/`; for each local + copy present, hash-compare raw bytes against the skeleton file → `status: 'identical' | 'differs'`; + record `{ relativePath, tier: '.codev' | 'codev', status, isResolvedWinner }`. + - `hasFrameworkShadows(workspaceRoot: string): boolean` — true if any local copy of any scanned + skeleton file exists in **either** root (no-op gate for doctor; checks both tiers). + - `checkSkeletonStaleness(): StalenessResult` — `{ installed: string, latest: string | null, + behind: boolean, note?: string }`; `installed` from `version.ts`, `latest` from + `npm view @cluesmith/codev version` (argv form, ~2500ms timeout, offline-tolerant → `latest:null`). + - Formatters: `formatDriftFinding(f): string`, `formatStaleness(s): string`. +- [ ] Reuse `getSkeletonDir`, `listSkeletonFiles`, `resolveCodevFile` from `lib/skeleton.ts`; use + `node:crypto` for hashing and read the installed `version` from `../version.js`. + +#### Implementation Details +- Path mapping: skeleton relative path (e.g. `protocols/spir/protocol.md`) maps to local + `/.codev/` and `/codev/`. Only `.md` and `.json` files are compared + (skeleton dirs contain only these for the scan set; guard defensively). +- `isResolvedWinner`: computed by calling `resolveCodevFile(rel, root)` and checking whether the + resolved absolute path equals this finding's local path — marks the copy the runtime actually loads. +- Staleness: never throw. `spawnSync('npm', ['view', '@cluesmith/codev', 'version'], {timeout, encoding})`; + on non-zero/empty/exception → `latest: null, note: 'could not check (offline?)'`. `behind` computed + with a small semver compare (reuse the `versionGte` shape already in doctor.ts, or a local helper). + +#### Acceptance Criteria +- [ ] `auditProtocolDrift` returns `identical` for a byte-identical local copy, `differs` for a + one-byte-changed copy, and nothing for a skeleton file with no local copy. +- [ ] Findings include tier and `isResolvedWinner`; both `.codev/` and `codev/` copies are reported. +- [ ] `checkSkeletonStaleness` returns explicit installed/latest and never hangs > timeout offline. +- [ ] Module performs **no writes** to disk. + +#### Test Plan +- Covered in Phase 3 (kept separate so the lib and its wiring commit independently). Lib is written + test-first-friendly (pure functions, injectable `workspaceRoot`). + +#### Rollback Strategy +- Delete `lib/protocol-drift-audit.ts`; no other module imports it until Phase 2. + +#### Risks +- **Risk**: scan set drifts from actual skeleton framework dirs. **Mitigation**: Phase 3 test asserts + each `FRAMEWORK_DRIFT_DIRS` entry exists in the skeleton; document the maintenance point in-file. + +--- + +### Phase 2: Wire drift report into `codev doctor` +**Dependencies**: Phase 1 + +#### Objectives +- Surface the drift + staleness findings in `codev doctor`'s output as a new section, integrated with + the existing warning roll-up — report-only. + +#### Deliverables +- [ ] Edit `packages/codev/src/commands/doctor.ts`: + - Inside the `if (workspaceRoot && existsSync(codev))` block (alongside the framework-ref and + pr-gate sections), add a **quiet-by-default "Framework Drift"** section. Compute + `shadows = auditProtocolDrift(root)` and `staleness = checkSkeletonStaleness()`, then: + - **Guard**: if `shadows.length === 0` **and** `staleness.behind !== true` → **print nothing at + all** (no header). This is the spec's true no-op (covers no-overrides + up-to-date, and + no-overrides + offline). Uses `hasFrameworkShadows`/`shadows` for the shadow half and + `staleness.behind` for the staleness half — the section header is only emitted when at least one + half has something to say. + - When the section IS shown: + - Staleness: if `behind` → `⚠` warning line `installed X; latest Y — behind` (→ `warningDetails`, + recommend `codev update`). If up-to-date → dim info line `installed X; latest Y (up to date)`. + If uncheckable → dim `latest: could not check (offline?)`. **Only `behind` is a warning; the + up-to-date / uncheckable lines are informational and shown only because shadows already forced + the section open.** + - `differs` shadows → `⚠` warning lines ("customized or stale? — adjudicate", named file + tier + + resolved-winner marker), each incrementing `warnings` and pushed to `warningDetails`. + - `identical` shadows → informational `○`/dim lines ("redundant copy — safe to remove; falls + back to package"); **not** counted as warnings (per spec). +- [ ] Import the new lib; no change to doctor's exit-code contract beyond the added warnings. + +#### Implementation Details +- Mirror the existing section pattern (header via `chalk.bold`, per-finding lines, `warningDetails.push`). +- Recommendation text for `differs`: "review vs installed skeleton; if unintentional, remove local + copy so resolution falls through to the package (`codev update` migrates unmodified copies)". +- Keep the section ordering sensible: place after the "Framework refs" / "Protocol PR Gates" blocks. + +#### Acceptance Criteria +- [ ] Running doctor in a project with a differing shadow prints the adjudicate warning and increments + the warning count; identical shadow prints an info line and does not. +- [ ] **No shadows AND not-behind (up-to-date or offline) → the Framework Drift section is not printed + at all** (true no-op); doctor exit code unchanged. +- [ ] **No shadows BUT skeleton behind → the staleness warning is printed** (section shown for + staleness alone); this is intentionally not a no-op. +- [ ] `codev/resources/arch.md` modifications are never reported (not in scan set). + +#### Test Plan +- Phase 3 e2e test drives the built CLI against fixture projects. + +#### Rollback Strategy +- Revert the doctor.ts hunk; Phase 1 lib becomes dead code but harmless. + +#### Risks +- **Risk**: added output noise for projects with many legitimate customizations. **Mitigation**: + identical copies are info-only (not warnings); differs are the actionable set the issue targets. + +--- + +### Phase 3: Tests — unit (lib) + e2e (doctor integration) +**Dependencies**: Phase 1, Phase 2 + +#### Objectives +- Lock in all spec scenarios with automated tests at both the lib and CLI levels. + +#### Deliverables +- [ ] `packages/codev/src/__tests__/protocol-drift-audit.test.ts` (unit): + - identical shadow → `identical`; differing shadow → `differs`; no local copy → no finding. + - `.codev/` (tier-1) differing copy detected same as `codev/`; both-present → both reported with + correct `isResolvedWinner` (`.codev/` wins). + - `resources/` modification not scanned. + - `hasFrameworkShadows` false when no overrides (no-op property). + - `checkSkeletonStaleness`: behind when installed < latest (inject/stub latest); offline/timeout → + `latest: null`, no throw, bounded. + - EOL-only difference → `differs` (documents the conservative raw-byte decision). + - No-mutation: fixture dir contents unchanged after audit runs. + - Scan-set integrity: every `FRAMEWORK_DRIFT_DIRS` entry exists under the skeleton. +- [ ] `packages/codev/src/__tests__/cli/doctor-drift.e2e.test.ts` (or extend `doctor.e2e.test.ts`): + - fixture project with a differing `codev/protocols/.../*.md` shadow → doctor output contains the + adjudicate warning; with an identical shadow → info line, no warning. + - **no overrides + skeleton up-to-date (stub `fetchLatest` = installed) → no "Framework Drift" + section header in output** (true no-op). + - **no overrides + skeleton behind (stub `fetchLatest` > installed) → staleness warning present** + even though there are no shadows (section shown for staleness alone). + +#### Implementation Details +- Follow the existing test harness conventions in `packages/codev/src/__tests__/` (fixture temp dirs, + `getSkeletonDir()` as the source of skeleton fixtures — copy a real skeleton file to build the + identical case, then mutate one byte for the differs case). +- For staleness, avoid real network: factor the `npm view` call behind an injectable seam (e.g. an + optional `fetchLatest` param defaulting to the real spawn) so the unit test stubs it deterministically. + +#### Acceptance Criteria +- [ ] All seven functional + two non-functional spec scenarios have a corresponding assertion. +- [ ] `pnpm --filter @cluesmith/codev test` passes (new + existing). + +#### Test Plan +- **Unit**: the lib test above. **Integration/e2e**: the doctor CLI test above. **Manual**: run + `codev doctor` in this repo (which has real `codev/protocols/*` overrides) and eyeball the section. + +#### Rollback Strategy +- Tests are additive; revert the test files if needed. + +#### Risks +- **Risk**: e2e flakiness from real network in staleness. **Mitigation**: staleness offline-tolerant + by design; e2e asserts on drift lines, not on a specific latest version; unit test stubs the fetch. + +## Dependency Map +``` +Phase 1 (lib) ──→ Phase 2 (doctor wiring) ──→ Phase 3 (tests) +``` + +## Integration Points +### Internal Systems +- **`lib/skeleton.ts`** — resolver primitives (`getSkeletonDir`, `listSkeletonFiles`, `resolveCodevFile`). + Read-only consumption. +- **`version.ts`** — installed package version for staleness. +- **`commands/doctor.ts`** — host of the new section; existing warning roll-up. +- **`commands/update.ts`** — *optional* future consumer (SHOULD, not in these phases). + +### External Systems +- **npm registry** — `npm view` for latest version; best-effort, offline-tolerant, bounded timeout. + +## Risk Analysis +### Technical Risks +| Risk | Probability | Impact | Mitigation | Owner | +|------|------------|--------|------------|-------| +| Scan set omits a framework subtree | Med | Med | Explicit `FRAMEWORK_DRIFT_DIRS` + integrity test | builder | +| EOL noise → spurious `differs` | Med | Low | Documented raw-byte decision; `differs` never auto-acts | builder | +| Staleness hangs doctor offline | Low | High | Bounded ~2.5s timeout + null-tolerant | builder | +| Output noise for heavily-customized projects | Med | Low | identical = info-only; differs = actionable | builder | + +## Validation Checkpoints +1. **After Phase 1**: lib compiles; can be exercised in a scratch script against this repo's `codev/`. +2. **After Phase 2**: `codev doctor` in this repo prints a Framework Drift section. +3. **Before PR**: full test suite green; manual `codev doctor` eyeball; no file mutations. + +## Documentation Updates Required +- [ ] None required to framework docs for core behavior (product code only). If any skeleton/doc text + is touched, mirror in **both** `codev/` and `codev-skeleton/` (arch-critical mirror rule). +- [ ] Review file (Phase R) documents the feature and the deferred item-2 follow-up. + +## Notes +- **Deferred (item 2 / known-default detection)**: ship a manifest of historical skeleton-file hashes + so a `differs` copy that matches a known *old* default is provably rot → stronger "safe to delete" + verdict. Deferred because it needs a release-time hash-generation step (new maintenance surface) and + the spec marks it non-blocking. It layers cleanly onto `DriftFinding` (add an optional + `matchesHistoricalDefault` field) in a follow-up. +- **`codev update` wiring** is intentionally left out of these phases (spec: SHOULD/optional). The lib + is structured so update can consume `auditProtocolDrift` later with no refactor. +- Encodes the lessons-critical principle *single source of truth beats distributed state*: a + byte-identical local copy is pure distributed-state risk; doctor names the divergence rather than + silently serving rot. + +--- + +## Amendment History + + diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_1-iter1-rebuttals.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_1-iter1-rebuttals.md new file mode 100644 index 000000000..5fef078c4 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_1-iter1-rebuttals.md @@ -0,0 +1,35 @@ +# Phase 1 (implement) iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES + +## Codex — Issue 1 (blocking): re-implements skeleton enumeration instead of reusing `listSkeletonFiles()` + +> `protocol-drift-audit.ts` reimplements skeleton traversal via `collectFrameworkFiles()` instead of +> using `listSkeletonFiles()` from `lib/skeleton.ts`. That conflicts with the phase-1 deliverable and +> the spec constraint to reuse the existing resolver primitives so the audit and resolver stay +> aligned. Please switch enumeration to `listSkeletonFiles(subdir)` and keep the `.md`/`.json` +> filtering on top of that. + +**Accepted — fixed.** I had added a custom `collectFrameworkFiles()` walk (with an injectable +`skeletonDir`) to ease unit testing. That was an unnecessary deviation: the spec and plan both list +`listSkeletonFiles` among the resolver primitives to reuse, and reusing it is what guarantees the +audit's notion of "the skeleton" is byte-identical to the resolver's. + +Changes: +- Removed `collectFrameworkFiles()` and the `skeletonDir` parameter from `auditProtocolDrift()` and + `hasFrameworkShadows()`. +- Added a small `skeletonFrameworkFiles(sub)` helper = `listSkeletonFiles(sub)` filtered to the + `.md`/`.json` framework extensions (the "keep filtering on top" Codex asked for). Both public + functions now enumerate through it. +- `getSkeletonDir()` is still used only to build the absolute skeleton path for hashing — consistent + with `listSkeletonFiles`, which walks that same dir. + +Testability is unaffected: `workspaceRoot` remains injectable, so unit tests build a temp workspace +with `.codev/` / `codev/` copies and diff them against the **real** installed skeleton (copy a real +skeleton file verbatim → `identical`; mutate one byte → `differs`). No injectable `skeletonDir` is +needed. + +`tsc --noEmit` clean (0 errors) after the change. + +## gemini / claude +Both APPROVE. Gemini specifically endorsed the raw-byte comparison decision; no changes requested. diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_1-iter2-context.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_1-iter2-context.md new file mode 100644 index 000000000..2cc3a1944 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_1-iter2-context.md @@ -0,0 +1,50 @@ +### Iteration 1 Reviews +- gemini: APPROVE — Phase 1 deliverables for protocol-drift-audit library are fully implemented, follow the plan specs, and handle edge cases cleanly. +- codex: REQUEST_CHANGES — Phase 1 is close, but the new audit lib diverges from the spec/plan by re-implementing skeleton file enumeration instead of reusing `listSkeletonFiles()`. +- claude: APPROVE — Clean, well-documented pure audit lib that faithfully implements all Phase 1 deliverables — shadow drift, staleness, injectable test seams, raw-byte comparison, offline tolerance, zero file mutation. + +### Builder Response to Iteration 1 +# Phase 1 (implement) iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES + +## Codex — Issue 1 (blocking): re-implements skeleton enumeration instead of reusing `listSkeletonFiles()` + +> `protocol-drift-audit.ts` reimplements skeleton traversal via `collectFrameworkFiles()` instead of +> using `listSkeletonFiles()` from `lib/skeleton.ts`. That conflicts with the phase-1 deliverable and +> the spec constraint to reuse the existing resolver primitives so the audit and resolver stay +> aligned. Please switch enumeration to `listSkeletonFiles(subdir)` and keep the `.md`/`.json` +> filtering on top of that. + +**Accepted — fixed.** I had added a custom `collectFrameworkFiles()` walk (with an injectable +`skeletonDir`) to ease unit testing. That was an unnecessary deviation: the spec and plan both list +`listSkeletonFiles` among the resolver primitives to reuse, and reusing it is what guarantees the +audit's notion of "the skeleton" is byte-identical to the resolver's. + +Changes: +- Removed `collectFrameworkFiles()` and the `skeletonDir` parameter from `auditProtocolDrift()` and + `hasFrameworkShadows()`. +- Added a small `skeletonFrameworkFiles(sub)` helper = `listSkeletonFiles(sub)` filtered to the + `.md`/`.json` framework extensions (the "keep filtering on top" Codex asked for). Both public + functions now enumerate through it. +- `getSkeletonDir()` is still used only to build the absolute skeleton path for hashing — consistent + with `listSkeletonFiles`, which walks that same dir. + +Testability is unaffected: `workspaceRoot` remains injectable, so unit tests build a temp workspace +with `.codev/` / `codev/` copies and diff them against the **real** installed skeleton (copy a real +skeleton file verbatim → `identical`; mutate one byte → `differs`). No injectable `skeletonDir` is +needed. + +`tsc --noEmit` clean (0 errors) after the change. + +## gemini / claude +Both APPROVE. Gemini specifically endorsed the raw-byte comparison decision; no changes requested. + + +### IMPORTANT: Stateful Review Context +This is NOT the first review iteration. Previous reviewers raised concerns and the builder has responded. +Before re-raising a previous concern: +1. Check if the builder has already addressed it in code +2. If the builder disputes a concern with evidence, verify the claim against actual project files before insisting +3. Do not re-raise concerns that have been explained as false positives with valid justification +4. Check package.json and config files for version numbers before flagging missing configuration diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_2-iter1-rebuttals.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_2-iter1-rebuttals.md new file mode 100644 index 000000000..b4d6cb558 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_2-iter1-rebuttals.md @@ -0,0 +1,36 @@ +# Phase 2 (implement) iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES + +Both Codex points are valid and trace directly to the spec. Accepted and fixed. + +## Codex — Issue 1 (blocking): differs warning omits the skeleton package version + +> The `differs` warning does not include the installed skeleton/package version. The spec's success +> criteria explicitly require the adjudication warning to name the file **and** the skeleton package +> version, but current output only says "differs from installed skeleton." + +**Accepted — fixed.** `formatDriftFinding(f, skeletonVersion?)` now takes an optional version and +renders it: `... — differs from installed skeleton v3.2.3; customized or stale? — adjudicate ...`. +doctor passes `staleness.installed` (the skeleton version IS the installed package version — the +skeleton ships inside the package, and staleness already resolved it, so no extra lookup). The +`warningDetails` entry now includes `v${skeletonVersion}` too. Verified in a real run against this +repo's overrides. + +## Codex — Issue 2 (blocking): misleading header in the staleness-only path + +> When there are no shadows but the package is behind, the section still opens with +> "Framework Drift (local copies shadowing the installed skeleton)". That parenthetical is false in +> the staleness-only path and will mislead users. + +**Accepted — fixed.** The header subtitle is now computed from what's actually present: +- shadows present → `(local copies shadowing the installed skeleton)` +- staleness-only (no shadows, skeleton behind) → `(installed skeleton is behind npm latest)` + +So the parenthetical is always accurate. (When both are true, the shadowing subtitle is correct and +the staleness warning is an additional line under it.) + +`tsc --noEmit` clean; `npm run build` green after both changes. + +## gemini / claude +Both APPROVE, no changes requested. diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_2-iter2-context.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_2-iter2-context.md new file mode 100644 index 000000000..d309408f5 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_2-iter2-context.md @@ -0,0 +1,51 @@ +### Iteration 1 Reviews +- gemini: APPROVE — Phase 2 cleanly wires protocol drift audit and skeleton staleness reporting into `codev doctor` with exact spec and plan alignment. +- codex: REQUEST_CHANGES — Doctor wiring is close, but the new drift warning still misses required context and has a misleading header in the staleness-only case. +- claude: APPROVE — Phase 2 wiring is clean, spec-compliant, follows existing doctor audit patterns, and correctly implements the quiet-by-default semantics including all edge cases (no-op, staleness-only, shadows-only, both). + +### Builder Response to Iteration 1 +# Phase 2 (implement) iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES + +Both Codex points are valid and trace directly to the spec. Accepted and fixed. + +## Codex — Issue 1 (blocking): differs warning omits the skeleton package version + +> The `differs` warning does not include the installed skeleton/package version. The spec's success +> criteria explicitly require the adjudication warning to name the file **and** the skeleton package +> version, but current output only says "differs from installed skeleton." + +**Accepted — fixed.** `formatDriftFinding(f, skeletonVersion?)` now takes an optional version and +renders it: `... — differs from installed skeleton v3.2.3; customized or stale? — adjudicate ...`. +doctor passes `staleness.installed` (the skeleton version IS the installed package version — the +skeleton ships inside the package, and staleness already resolved it, so no extra lookup). The +`warningDetails` entry now includes `v${skeletonVersion}` too. Verified in a real run against this +repo's overrides. + +## Codex — Issue 2 (blocking): misleading header in the staleness-only path + +> When there are no shadows but the package is behind, the section still opens with +> "Framework Drift (local copies shadowing the installed skeleton)". That parenthetical is false in +> the staleness-only path and will mislead users. + +**Accepted — fixed.** The header subtitle is now computed from what's actually present: +- shadows present → `(local copies shadowing the installed skeleton)` +- staleness-only (no shadows, skeleton behind) → `(installed skeleton is behind npm latest)` + +So the parenthetical is always accurate. (When both are true, the shadowing subtitle is correct and +the staleness warning is an additional line under it.) + +`tsc --noEmit` clean; `npm run build` green after both changes. + +## gemini / claude +Both APPROVE, no changes requested. + + +### IMPORTANT: Stateful Review Context +This is NOT the first review iteration. Previous reviewers raised concerns and the builder has responded. +Before re-raising a previous concern: +1. Check if the builder has already addressed it in code +2. If the builder disputes a concern with evidence, verify the claim against actual project files before insisting +3. Do not re-raise concerns that have been explained as false positives with valid justification +4. Check package.json and config files for version numbers before flagging missing configuration diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter1-rebuttals.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter1-rebuttals.md new file mode 100644 index 000000000..cf93e075c --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter1-rebuttals.md @@ -0,0 +1,47 @@ +# Phase 3 (tests) iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude COMMENT · codex REQUEST_CHANGES + +Codex (blocking) and Claude (comment) independently flagged the same primary gap; both accepted and +fixed. + +## Codex Issue 1 (blocking) / Claude Issue 1: missing e2e for the staleness-only "behind" branch + +> The e2e file forces npm offline in every test, making `behind` unreachable and leaving the main +> Phase 2 integration branch ("no overrides + skeleton behind → section shown for staleness alone") +> unverified. (Claude: "explicitly listed as a Phase 3 e2e deliverable … add a `fetchLatest` +> injection seam to `doctor.ts` to enable the e2e case.") + +**Accepted — fixed by adding the seam Claude suggested.** `doctor.ts` now reads an optional +`CODEV_DOCTOR_FAKE_LATEST` env var and, when set, injects it as the npm-latest value into the +already-injectable `checkSkeletonStaleness(fetchLatest?)`. Unset in real use → the real `npm view` +lookup runs unchanged (the seam is inert for actual users). New e2e test: + +> "shows the Framework Drift section for staleness alone when the skeleton is behind (no shadows)" — +> no local overrides, `CODEV_DOCTOR_FAKE_LATEST=999.0.0`, asserts the section opens with the +> **staleness-specific subtitle** (`installed skeleton is behind npm latest`), a `latest 999.0.0 — +> behind` warning, and **no** adjudication line (no shadows in this path). + +This exercises exactly the Phase 2 branch (staleness-only header subtitle + behind warning) that was +previously unreachable end-to-end. + +## Codex Issue 2 (blocking): identical-shadow test doesn't assert info-only / not-a-warning + +> Checks the identical message appears, but does not assert this path is info-only / not a warning. + +**Accepted — fixed.** The identical-shadow e2e test now also asserts the output does **not** contain +`customized or stale? — adjudicate` (the warning/`differs` marker). With only an identical copy in +the fixture there is no `differs` finding, so the absence of the adjudicate line confirms the +identical path is informational, not a warning. + +## Claude Issue 2 (comment): EOL unit test could vacuously pass + +> The EOL test silently passes without asserting when the picked file has no newlines; add a guard. + +**Accepted — fixed.** The EOL unit test now asserts the precondition explicitly +(`expect(skeletonBytes(rel).includes(0x0a)).toBe(true)`) before the CRLF transform, then asserts +`differs` unconditionally — so it can no longer pass vacuously. + +## Result +Unit: 19/19. e2e: now 4 tests (added the staleness-only case). `tsc` clean, `npm run build` green. +gemini APPROVE with no issues. diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter2-context.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter2-context.md new file mode 100644 index 000000000..142f2e336 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter2-context.md @@ -0,0 +1,62 @@ +### Iteration 1 Reviews +- gemini: APPROVE — Phase 3 unit and CLI integration tests are complete, well-isolated, and fully cover all spec requirements and plan scenarios. +- codex: REQUEST_CHANGES — Phase 3 adds solid unit coverage, but it misses the required doctor integration test for the staleness-only path. +- claude: COMMENT — Phase 3 tests are comprehensive and well-crafted; all spec scenarios are covered at the unit level; one plan-listed e2e scenario (staleness-only section) is absent but mitigated by unit coverage. + +### Builder Response to Iteration 1 +# Phase 3 (tests) iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude COMMENT · codex REQUEST_CHANGES + +Codex (blocking) and Claude (comment) independently flagged the same primary gap; both accepted and +fixed. + +## Codex Issue 1 (blocking) / Claude Issue 1: missing e2e for the staleness-only "behind" branch + +> The e2e file forces npm offline in every test, making `behind` unreachable and leaving the main +> Phase 2 integration branch ("no overrides + skeleton behind → section shown for staleness alone") +> unverified. (Claude: "explicitly listed as a Phase 3 e2e deliverable … add a `fetchLatest` +> injection seam to `doctor.ts` to enable the e2e case.") + +**Accepted — fixed by adding the seam Claude suggested.** `doctor.ts` now reads an optional +`CODEV_DOCTOR_FAKE_LATEST` env var and, when set, injects it as the npm-latest value into the +already-injectable `checkSkeletonStaleness(fetchLatest?)`. Unset in real use → the real `npm view` +lookup runs unchanged (the seam is inert for actual users). New e2e test: + +> "shows the Framework Drift section for staleness alone when the skeleton is behind (no shadows)" — +> no local overrides, `CODEV_DOCTOR_FAKE_LATEST=999.0.0`, asserts the section opens with the +> **staleness-specific subtitle** (`installed skeleton is behind npm latest`), a `latest 999.0.0 — +> behind` warning, and **no** adjudication line (no shadows in this path). + +This exercises exactly the Phase 2 branch (staleness-only header subtitle + behind warning) that was +previously unreachable end-to-end. + +## Codex Issue 2 (blocking): identical-shadow test doesn't assert info-only / not-a-warning + +> Checks the identical message appears, but does not assert this path is info-only / not a warning. + +**Accepted — fixed.** The identical-shadow e2e test now also asserts the output does **not** contain +`customized or stale? — adjudicate` (the warning/`differs` marker). With only an identical copy in +the fixture there is no `differs` finding, so the absence of the adjudicate line confirms the +identical path is informational, not a warning. + +## Claude Issue 2 (comment): EOL unit test could vacuously pass + +> The EOL test silently passes without asserting when the picked file has no newlines; add a guard. + +**Accepted — fixed.** The EOL unit test now asserts the precondition explicitly +(`expect(skeletonBytes(rel).includes(0x0a)).toBe(true)`) before the CRLF transform, then asserts +`differs` unconditionally — so it can no longer pass vacuously. + +## Result +Unit: 19/19. e2e: now 4 tests (added the staleness-only case). `tsc` clean, `npm run build` green. +gemini APPROVE with no issues. + + +### IMPORTANT: Stateful Review Context +This is NOT the first review iteration. Previous reviewers raised concerns and the builder has responded. +Before re-raising a previous concern: +1. Check if the builder has already addressed it in code +2. If the builder disputes a concern with evidence, verify the claim against actual project files before insisting +3. Do not re-raise concerns that have been explained as false positives with valid justification +4. Check package.json and config files for version numbers before flagging missing configuration diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter2-rebuttals.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter2-rebuttals.md new file mode 100644 index 000000000..b099404c9 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter2-rebuttals.md @@ -0,0 +1,34 @@ +# Phase 3 (tests) iteration 2 — Rebuttals + +**Verdicts**: gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES (MEDIUM confidence) + +## Codex (blocking): no assertion for the bounded offline-timeout non-functional scenario + +> behind/equal/offline behavior is covered functionally, but there is still no assertion that the +> offline/unreachable path completes within a bounded time. Phase 3 explicitly promised assertions +> for both non-functional scenarios, and only the no-mutation one is asserted today. Add a +> timing-bounded test for the real offline path. + +**Accepted — legitimate and distinct from the iter-1 points.** The plan's Phase 3 non-functional +list was: (1) staleness completes within a bounded timeout offline, and (2) no mutation. I had +asserted (2) but only exercised (1) through an injected instant `() => null` stub, which never runs +the real `npm view` timeout that actually provides the bound. + +Fix: +- Exported the real default fetcher as `fetchLatestVersion()` (was the private `defaultFetchLatest`) + and a named constant `NPM_LATEST_TIMEOUT_MS = 2500`. `checkSkeletonStaleness`'s default param now + references the exported function; no behavior change. +- Added unit test **"the REAL default lookup is offline-tolerant AND bounded when the registry is + unreachable"**: points npm at `http://127.0.0.1:1` (immediate ECONNREFUSED) with + `fetch_retries=0`, calls the real `fetchLatestVersion()`, and asserts it returns `null` **and** + completes in `< NPM_LATEST_TIMEOUT_MS + 5000` ms. ECONNREFUSED is immediate and the spawnSync + timeout is the hard backstop, so the generous ceiling can't flake; env vars are restored in a + `finally`. This exercises the real bounded path rather than a stub. + +Unit suite now 20/20 (added the timing test); e2e unchanged at 4/4. `tsc` clean. + +Not a cycling nitpick: iter-1 addressed the staleness-*behind* integration branch; this addresses a +separate promised non-functional assertion (bounded offline timing) that neither iter-1 fix touched. + +## gemini / claude +Both APPROVE, no issues. diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter3-context.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter3-context.md new file mode 100644 index 000000000..40771bf47 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-phase_3-iter3-context.md @@ -0,0 +1,104 @@ +### Iteration 1 Reviews +- gemini: APPROVE — Phase 3 unit and CLI integration tests are complete, well-isolated, and fully cover all spec requirements and plan scenarios. +- codex: REQUEST_CHANGES — Phase 3 adds solid unit coverage, but it misses the required doctor integration test for the staleness-only path. +- claude: COMMENT — Phase 3 tests are comprehensive and well-crafted; all spec scenarios are covered at the unit level; one plan-listed e2e scenario (staleness-only section) is absent but mitigated by unit coverage. + +### Builder Response to Iteration 1 +# Phase 3 (tests) iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude COMMENT · codex REQUEST_CHANGES + +Codex (blocking) and Claude (comment) independently flagged the same primary gap; both accepted and +fixed. + +## Codex Issue 1 (blocking) / Claude Issue 1: missing e2e for the staleness-only "behind" branch + +> The e2e file forces npm offline in every test, making `behind` unreachable and leaving the main +> Phase 2 integration branch ("no overrides + skeleton behind → section shown for staleness alone") +> unverified. (Claude: "explicitly listed as a Phase 3 e2e deliverable … add a `fetchLatest` +> injection seam to `doctor.ts` to enable the e2e case.") + +**Accepted — fixed by adding the seam Claude suggested.** `doctor.ts` now reads an optional +`CODEV_DOCTOR_FAKE_LATEST` env var and, when set, injects it as the npm-latest value into the +already-injectable `checkSkeletonStaleness(fetchLatest?)`. Unset in real use → the real `npm view` +lookup runs unchanged (the seam is inert for actual users). New e2e test: + +> "shows the Framework Drift section for staleness alone when the skeleton is behind (no shadows)" — +> no local overrides, `CODEV_DOCTOR_FAKE_LATEST=999.0.0`, asserts the section opens with the +> **staleness-specific subtitle** (`installed skeleton is behind npm latest`), a `latest 999.0.0 — +> behind` warning, and **no** adjudication line (no shadows in this path). + +This exercises exactly the Phase 2 branch (staleness-only header subtitle + behind warning) that was +previously unreachable end-to-end. + +## Codex Issue 2 (blocking): identical-shadow test doesn't assert info-only / not-a-warning + +> Checks the identical message appears, but does not assert this path is info-only / not a warning. + +**Accepted — fixed.** The identical-shadow e2e test now also asserts the output does **not** contain +`customized or stale? — adjudicate` (the warning/`differs` marker). With only an identical copy in +the fixture there is no `differs` finding, so the absence of the adjudicate line confirms the +identical path is informational, not a warning. + +## Claude Issue 2 (comment): EOL unit test could vacuously pass + +> The EOL test silently passes without asserting when the picked file has no newlines; add a guard. + +**Accepted — fixed.** The EOL unit test now asserts the precondition explicitly +(`expect(skeletonBytes(rel).includes(0x0a)).toBe(true)`) before the CRLF transform, then asserts +`differs` unconditionally — so it can no longer pass vacuously. + +## Result +Unit: 19/19. e2e: now 4 tests (added the staleness-only case). `tsc` clean, `npm run build` green. +gemini APPROVE with no issues. + + +### Iteration 2 Reviews +- gemini: APPROVE — Phase 3 unit and CLI e2e test suites are complete, thoroughly cover all spec/plan scenarios, and incorporate all iteration 1 feedback. +- codex: REQUEST_CHANGES — Phase 3 substantially improves coverage, but it still misses an assertion for the bounded offline-timeout scenario called out in the phase plan/spec. +- claude: APPROVE — Phase 3 tests are comprehensive — all 7 functional and 2 non-functional spec scenarios are covered, all three iteration-1 issues are verified fixed in code, and the `CODEV_DOCTOR_FAKE_LATEST` seam cleanly enables the previously-unreachable staleness-only e2e branch. + +### Builder Response to Iteration 2 +# Phase 3 (tests) iteration 2 — Rebuttals + +**Verdicts**: gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES (MEDIUM confidence) + +## Codex (blocking): no assertion for the bounded offline-timeout non-functional scenario + +> behind/equal/offline behavior is covered functionally, but there is still no assertion that the +> offline/unreachable path completes within a bounded time. Phase 3 explicitly promised assertions +> for both non-functional scenarios, and only the no-mutation one is asserted today. Add a +> timing-bounded test for the real offline path. + +**Accepted — legitimate and distinct from the iter-1 points.** The plan's Phase 3 non-functional +list was: (1) staleness completes within a bounded timeout offline, and (2) no mutation. I had +asserted (2) but only exercised (1) through an injected instant `() => null` stub, which never runs +the real `npm view` timeout that actually provides the bound. + +Fix: +- Exported the real default fetcher as `fetchLatestVersion()` (was the private `defaultFetchLatest`) + and a named constant `NPM_LATEST_TIMEOUT_MS = 2500`. `checkSkeletonStaleness`'s default param now + references the exported function; no behavior change. +- Added unit test **"the REAL default lookup is offline-tolerant AND bounded when the registry is + unreachable"**: points npm at `http://127.0.0.1:1` (immediate ECONNREFUSED) with + `fetch_retries=0`, calls the real `fetchLatestVersion()`, and asserts it returns `null` **and** + completes in `< NPM_LATEST_TIMEOUT_MS + 5000` ms. ECONNREFUSED is immediate and the spawnSync + timeout is the hard backstop, so the generous ceiling can't flake; env vars are restored in a + `finally`. This exercises the real bounded path rather than a stub. + +Unit suite now 20/20 (added the timing test); e2e unchanged at 4/4. `tsc` clean. + +Not a cycling nitpick: iter-1 addressed the staleness-*behind* integration branch; this addresses a +separate promised non-functional assertion (bounded offline timing) that neither iter-1 fix touched. + +## gemini / claude +Both APPROVE, no issues. + + +### IMPORTANT: Stateful Review Context +This is NOT the first review iteration. Previous reviewers raised concerns and the builder has responded. +Before re-raising a previous concern: +1. Check if the builder has already addressed it in code +2. If the builder disputes a concern with evidence, verify the claim against actual project files before insisting +3. Do not re-raise concerns that have been explained as false positives with valid justification +4. Check package.json and config files for version numbers before flagging missing configuration diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/1210-plan-iter1-rebuttals.md b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-plan-iter1-rebuttals.md new file mode 100644 index 000000000..24832a148 --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/1210-plan-iter1-rebuttals.md @@ -0,0 +1,64 @@ +# Plan iteration 1 — Rebuttals + +**Verdicts**: gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES + +Only Codex requested changes. Both points are valid and I accepted them — the plan had a genuine +internal contradiction. Summary of resolution below. + +--- + +## Codex — Issue 1 (blocking): plan conflicts with itself on *when the section appears* + +> The Executive Summary says it adopts no-op gating ("report nothing when the project ships no +> framework overrides"), but Phase 2 says the staleness line is **always shown when in a project**. +> Those cannot both be true, and the latter breaks the spec's "true no-op" requirement. + +**Accepted — this was a real contradiction. Fixed.** + +I introduced a single, unambiguous rule in both the spec and the plan (spec *Desired State* + +Success Criteria; plan *Key Design Decisions* + Phase 2 + Phase 3): the **Framework Drift section is +quiet by default**. doctor computes shadows + staleness, then: + +- **No shadows AND not-behind** (up-to-date, *or* offline/uncheckable) → **prints nothing at all** + (no header). This is the spec's true no-op. +- **Otherwise** the section is shown. Staleness is a **warning only when `behind`**; the up-to-date + and "could not check (offline?)" lines are informational and appear only because shadows already + forced the section open. Staleness is therefore **never** printed unconditionally-per-run. + +Rationale for keeping the `behind`-forces-section case (rather than making staleness purely +subordinate to shadows): a stale installed skeleton with no local overrides is exactly the issue's +**sibling failure mode** ("before the upgrade, the installed skeleton itself was a version behind, so +even non-shadowed resolution served pre-fix templates. Equally silent."). Suppressing it would +re-hide the very bug #1210 asks doctor to surface. I updated the spec's no-op success criterion to +scope it precisely — "no overrides **and** up-to-date/unreachable" → no-op; "no overrides **but** +behind" → staleness warning surfaces (explicitly not a no-op). + +## Codex — Issue 2 (blocking): testing under-specifies no-overrides + staleness-behind/offline + +> The plan should explicitly say whether staleness is suppressed when there are no shadows, or +> whether the spec is being intentionally reinterpreted. + +**Accepted. Fixed.** Phase 3 now names three explicit no-overrides cases and asserts each: +1. no overrides + skeleton up-to-date (stub `fetchLatest` = installed) → **no "Framework Drift" + header** in output (true no-op); +2. no overrides + skeleton behind (stub `fetchLatest` > installed) → **staleness warning present** + (section shown for staleness alone); +3. offline → staleness silent, no hang, bounded by the ~2.5s timeout. + +Phase 2 acceptance criteria were amended to match. The `fetchLatest` seam (injectable in +`checkSkeletonStaleness`) makes these deterministic without real network. + +--- + +## Codex — non-blocking observations (acknowledged) + +- **"Phase 1 isn't truly independently testable if all tests are in Phase 3."** Fair; the phrasing was + slightly overstated. Phase 1 delivers pure functions with an injectable `workspaceRoot`/`fetchLatest` + so it *is* exercisable in isolation; tests are consolidated in Phase 3 so lib + wiring commit + cleanly. Left the phase split as-is (deliberate), but the plan notes the seam. +- **File choices / precedent alignment / `resources/` exclusion** — confirmed correct; no change. + +## gemini / claude + +Both APPROVE, KEY_ISSUES: none. Gemini's spec-phase notes (raw-byte compare for EOL; ~2–3s timeout) +were already incorporated into the plan's Key Design Decisions. No further action. diff --git a/codev/projects/1210-codev-doctor-detect-protocol-f/status.yaml b/codev/projects/1210-codev-doctor-detect-protocol-f/status.yaml new file mode 100644 index 000000000..b132b13ff --- /dev/null +++ b/codev/projects/1210-codev-doctor-detect-protocol-f/status.yaml @@ -0,0 +1,28 @@ +id: '1210' +title: codev-doctor-detect-protocol-f +protocol: aspir +phase: review +plan_phases: + - id: phase_1 + title: protocol-drift-audit library (shadow drift + staleness) + status: complete + - id: phase_2 + title: Wire drift report into codev doctor + status: complete + - id: phase_3 + title: 'Tests: unit (lib) + e2e (doctor integration)' + status: complete +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-07-22T13:32:55.614Z' + approved_at: '2026-07-22T13:36:09.759Z' + verify-approval: + status: pending +iteration: 1 +build_complete: true +history: [] +started_at: '2026-07-22T12:28:50.210Z' +updated_at: '2026-07-22T13:36:17.274Z' +pr_ready_for_human: false diff --git a/codev/reviews/1210-codev-doctor-detect-protocol-f.md b/codev/reviews/1210-codev-doctor-detect-protocol-f.md new file mode 100644 index 000000000..d03e6898f --- /dev/null +++ b/codev/reviews/1210-codev-doctor-detect-protocol-f.md @@ -0,0 +1,124 @@ +# Review: `codev doctor` — detect protocol-file drift (#1210) + +## Summary + +`codev doctor` gains a **Framework Drift** report that surfaces a previously-silent failure class of +the four-tier resolver: project-local copies (`.codev/` / `codev/`) of framework files that shadow +the installed skeleton, and an installed skeleton that is itself behind npm latest. + +Two deliverables: +- A pure, unit-tested library `packages/codev/src/lib/protocol-drift-audit.ts` that (a) diffs every + local copy of a skeleton framework file (under `protocols/`, `consult-types/`, `roles/`) against + the skeleton and classifies it `identical` (redundant, safe to remove) or `differs` (customized or + stale? — adjudicate), and (b) compares the installed package version against npm latest + (best-effort, offline-tolerant, bounded). +- Wiring in `packages/codev/src/commands/doctor.ts` that renders the report **quiet by default**: + the section prints only when a shadow exists or the skeleton is behind. Report-only — no user file + is ever modified. + +Item 2 of the issue (historical-default hash detection) was spec'd as non-blocking and deferred to a +follow-up. `codev update` wiring was left optional (the lib is structured for it). + +## Spec Compliance + +All spec success criteria are met: +- Identical shadow → info-only "redundant copy, safe to remove" line (not a warning). ✓ +- Differing shadow → adjudicate warning naming the file, tier, resolved-winner, and **skeleton + package version**; increments the warning count. ✓ +- No overrides + up-to-date/offline → **true no-op** (no section printed). ✓ +- No overrides + behind → staleness warning surfaces (the issue's sibling failure mode), with a + staleness-specific header subtitle. ✓ +- Staleness reports explicit `installed X; latest Y`; offline-tolerant; bounded (~2.5s). ✓ +- Both override roots (`.codev/` and `codev/`) considered; each local copy reported and classified. ✓ +- `codev/resources/` (user-evolved) excluded. ✓ +- Report-only — no file mutation (asserted by test). ✓ +- Standalone unit-tested lib mirroring the pr-gate / framework-ref precedent. ✓ + +## Deviations from Plan + +- None material. The plan's Approach 1 (skeleton-driven diff) was implemented as specified, with + Approach 2's quiet-by-default gating. Enumeration reuses `listSkeletonFiles()` per the plan (an + early draft used a custom walk; corrected in phase-1 review). +- Added a small documented test seam (`CODEV_DOCTOR_FAKE_LATEST` env var in doctor.ts) to make the + staleness-only "behind" integration branch e2e-testable without a live registry — this was Claude's + suggested option during phase-3 review, not a plan change. + +## Key Metrics + +- Product code: **+328 lines** across 2 files (`protocol-drift-audit.ts` +264, `doctor.ts` +64). +- Tests: **+345 lines** — 20 unit cases + 4 CLI e2e cases. +- Full suite: 3555+ passing, 0 failures. +- 31 commits on the branch; 3 implement phases, each with a 3-way consult round (several with a + Codex REQUEST_CHANGES → fix → re-approve cycle). + +## Consultation Iteration Summary + +Every phase ran a 3-way consult (gemini / codex / claude). Codex was the consistent gatekeeper: +- **Spec**: APPROVE / APPROVE / COMMENT — folded in explicit scan set, dual override roots, explicit + `installed X; latest Y` wording, item-2 non-blocking, raw-byte compare. +- **Plan**: APPROVE / APPROVE / **REQUEST_CHANGES** — resolved a genuine self-contradiction (Exec + Summary "no-op when no overrides" vs Phase 2 "staleness always shown") into one quiet-by-default rule. +- **Phase 1**: **REQUEST_CHANGES** — reuse `listSkeletonFiles()` instead of a custom walk. Fixed. +- **Phase 2**: **REQUEST_CHANGES** ×2 — name the skeleton version in the differs line; fix the false + header parenthetical in the staleness-only path. Both fixed. +- **Phase 3**: **REQUEST_CHANGES** ×2 across two iters — add the staleness-only "behind" e2e branch + (via the test seam) + assert identical-is-info-only; then assert the *real* npm lookup is + bounded/offline-tolerant (not just a stub). Both fixed → unanimous APPROVE. + +Rebuttal docs for each round are in `codev/projects/1210-*/`. + +## Lessons Learned + +### What Went Well +- Reusing the three existing audit precedents (`pr-gate-audit`, `framework-ref-audit`, `gitignore`) + made the shape obvious and the review fast — pure lib (findings + formatter) + thin doctor wiring. +- Manually running `codev doctor` against *this* self-hosted repo (which has real `codev/protocols` + overrides) validated the feature end-to-end immediately — a live, high-signal fixture. + +### Challenges +- **Testing a network-bound, timeout-guaranteed path deterministically** cost the most review cycles. + Injecting `fetchLatest` covered logic, but the *bounded-when-offline* non-functional guarantee lives + in the real `npm view` timeout — asserting it required exporting the real fetcher and driving it at + an unreachable registry. Two Codex rounds to land it fully. +- **A self-contradiction in the plan** (no-op vs always-show staleness) slipped past my own drafting + and was caught by Codex — a reminder that "quiet by default" needs one precisely-stated rule, not + two independently-reasonable sentences. + +### What Would Be Done Differently +- State cross-cutting output rules (like "quiet by default") once, as an explicit precedence rule, + before writing per-branch prose — would have avoided the plan contradiction. +- When a plan lists non-functional assertions (bounded timing), write the *real-path* test up front, + not a stub — the stub reads as coverage but isn't. + +## Architecture Updates + +- Routed: **cold** — `codev/resources/arch.md` (Core Components / System-Wide Patterns) already + documents the four-tier resolver and the doctor audit-lib pattern; this feature is another instance + of that established pattern (pure audit lib + doctor wiring), so no new architectural shape was + introduced. No hot-tier change: the resolver + audit-precedent facts are already in + `arch-critical.md` and the hot file is at cap. **No arch.md edit required** — the drift audit is a + faithful application of the documented "doctor diagnoses silent misconfiguration via a pure audit + lib" pattern, not a new invariant. + +## Lessons Learned Updates + +- Routed: **cold** — `codev/resources/lessons-learned.md` (Testing) — *"When a plan promises a + non-functional assertion backed by a real timeout/network path, test the real path, not an injected + stub — a stub that returns the expected value reads as coverage but exercises none of the guarantee + (the bound lives in the real code path)."* Not hot-tier: it's a useful testing refinement, not a + top-10 always-injected lesson, and the hot file is at cap. (Recorded here; the MAINTAIN pass can + fold it into lessons-learned.md's Testing section.) + +## Technical Debt + +- None introduced. The `CODEV_DOCTOR_FAKE_LATEST` env seam is documented and inert in real use; it is + the minimal cross-process injection needed for e2e coverage of the staleness branch. + +## Follow-up Items + +- **Item 2 (known-default detection)**: ship a manifest of historical skeleton-file hashes so a + `differs` copy matching a known old default is provably rot → stronger "safe to delete" verdict. + Layers onto `DriftFinding` with an optional `matchesHistoricalDefault` field. Deferred (non-blocking + per spec; needs a release-time hash-generation step). +- **`codev update` wiring**: optionally consume `auditProtocolDrift` in `update` to surface drift at + upgrade time. The lib is structured for this with no refactor. diff --git a/codev/specs/1210-codev-doctor-detect-protocol-f.md b/codev/specs/1210-codev-doctor-detect-protocol-f.md new file mode 100644 index 000000000..7b7ec7fa9 --- /dev/null +++ b/codev/specs/1210-codev-doctor-detect-protocol-f.md @@ -0,0 +1,296 @@ +# Specification: `codev doctor` — detect protocol-file drift + +## Metadata +- **ID**: 1210-codev-doctor-detect-protocol-f +- **Status**: draft +- **Created**: 2026-07-22 +- **Issue**: #1210 +- **Area**: `area/scaffold` (resolver / doctor) + +## Clarifying Questions Asked +No clarifying questions were needed — issue #1210 fully specifies the problem, the proposal +(4 numbered items), and the field evidence (17 confirmed instances in one adopting repo). The +issue contains **no "Baked Decisions" section**, so the design below is explored freely. The +proposal's four items are treated as the requirement backbone, with item 2 (known-default +detection) explicitly flagged "(stretch)" by the issue. + +## Problem Statement + +Codev's four-tier resolver (`.codev/` → `codev/` → cache → installed skeleton) lets a +project-local file shadow the shipped skeleton. That is the intended customization mechanism. +It fails **silently**, however, when a local copy is not a deliberate customization but a stale +snapshot of an old upstream default: the project keeps running old framework behavior forever, +with no signal, even after the installed package ships a fix. Nothing today distinguishes +"deliberately customized" from "rotted copy of an old default" — both look identical to the +resolver. + +There is a sibling failure mode: even with **no** local shadow, the *installed skeleton itself* +can be a version behind, so non-shadowed resolution still serves pre-fix framework files. Also +silent. + +### Field evidence (from the issue) +- A "bugfix PRs reviewed against SPIR conventions" consult-noise bug hit **3×** in one CI-cleanup + sweep. Root cause: two project-local `codev/protocols/bugfix/consult-types/*.md` files, checked + in months earlier from old upstream defaults, shadowing the skeleton's fixed templates. Upgrading + the package changed nothing — tier precedence kept loading the stale local copies. +- A follow-up sweep of the same repo found **15 more** differing local protocol files (air / maintain + / bugfix families) each needing customization-vs-rot adjudication. +- Before the upgrade, that repo's *installed skeleton* was itself a version behind — even + non-shadowed resolution served pre-fix templates. +- Meta-note: the first diagnosis of this bug was itself derailed by a silently-vacuous check + (relative paths run from the wrong cwd). This failure class compounds because every layer is quiet. + +## Current State + +`codev doctor` (`packages/codev/src/commands/doctor.ts`) already runs a battery of section-based +checks and rolls warnings into a `warningDetails` summary. It has three precedents for exactly this +shape of check — a pure audit lib that returns findings plus a formatter, wired into doctor +(and sometimes `update`): +- **PR-gate audit** (`lib/pr-gate-audit.ts`, #943) — resolved protocol overrides missing a `pr` gate. +- **Framework-ref audit** (`lib/framework-ref-audit.ts`, #1011) — local overrides that shell-fetch + framework files by literal path. +- **State-file gitignore audit** (`lib/gitignore.ts`). + +What is **missing**: doctor has no notion of *shadow drift* (a local file diverging from the skeleton +file it shadows) and no notion of *skeleton staleness* (installed package version vs npm latest). +The resolver (`lib/skeleton.ts`) already exposes the primitives needed — `getSkeletonDir()`, +`listSkeletonFiles(subdir)`, `resolveCodevFile()`, `hasLocalOverride()` — but nothing consumes them +for drift reporting. Today the only signal an adopter gets is a bug in production. + +## Desired State + +`codev doctor`, when run inside a codev project, gains a **Protocol / Framework Drift** report with +three checks. All are **report-only** — doctor never deletes or rewrites a local file: + +1. **Shadow drift** — For every local framework file (under the scanned subtrees) that *also* exists + in the installed skeleton: + - **Byte-identical** → informational: this is a redundant copy that adds nothing but risk; suggest + removing it so resolution falls through to the package. + - **Differs** → warning: list the file, flagged "customized or stale? — adjudicate", with enough + context (the skeleton's package version) for a human to decide. +2. **Skeleton staleness** — Compare the installed `@cluesmith/codev` version against the npm + `latest` version (best-effort, offline-tolerant). Report how far behind the resolved framework + files are, or stay silent/neutral when offline. +3. **Known-default detection (stretch)** — If the local copy of a differing file byte-matches a + *historical* skeleton default (from shipped hashes), it is provably rot, not customization → + emit a stronger, safe-to-delete recommendation. Optional; see Solution Approaches. +4. **No auto-delete.** Report only. Adjudication stays human — local copies may be deliberate. + +The report is **quiet by default**: the Framework Drift section prints **nothing** unless it has +something actionable to say — i.e. either (a) at least one local shadow of a skeleton file exists, or +(b) the installed skeleton is behind npm latest. When the project ships no local framework overrides +**and** the skeleton is up to date (or the registry can't be reached), the section is a **true no-op +(no section printed / no warnings)**, mirroring how the framework-ref audit stays silent for projects +with no overrides. The staleness check is thus silent when up-to-date or offline, and surfaces only +when the installed skeleton is genuinely behind (the issue's sibling failure mode) — it is *not* +unconditionally printed just because doctor runs inside a project. + +## Scope + +### In scope +- A new drift-audit library (pure, testable) returning structured findings + formatters. +- Wiring the report into `codev doctor`. +- Shadow-drift detection over the framework subtrees that ship in the skeleton and resolve via the + resolver. **Explicit scan set** (per Codex spec review — pinned here, not deferred to Plan): every + `.md` / `.json` file under the skeleton's **`protocols/`**, **`consult-types/`**, and **`roles/`** + trees (prompts and per-protocol templates live *within* `protocols/`, so they are covered). This is + the enumerable "framework files that can be shadowed" set; adding a new top-level framework subtree + is a documented maintenance point (same shape as `PR_PRODUCING_PROTOCOLS` in pr-gate-audit). +- **Both override roots are considered independently.** A given skeleton file may be shadowed by a + tier-1 `.codev/` copy, a tier-2 `codev/` copy, or both. doctor reports **each local + copy it finds**, labeled with its tier, each classified on its own (identical vs differs) — because + a stale lower-precedence `codev/` copy is still rot even when a `.codev/` copy currently wins + resolution. The report also names which copy the resolver actually resolves (the winner), so the + human knows which one is live. (Note: the existing `hasLocalOverride()` helper only checks tier-2 + `codev/`; the no-op / presence detection here must check **both** roots.) +- Skeleton staleness: installed-version vs npm-latest, best-effort and offline-tolerant. +- Classification of each shadow as **identical** (redundant) vs **differs** (adjudicate). +- Tests: unit tests for the audit lib; an e2e/CLI test asserting doctor surfaces drift. + +### Out of scope +- **Auto-deletion or auto-migration** of any local file (explicitly forbidden by the issue). +- Changing the **resolver's precedence** — tier order is unchanged; local still wins. +- Drift of **user-evolved resources** (`codev/resources/arch.md`, `lessons-learned.md`, and their + `-critical` companions). These are intentionally project-owned, not framework files, and must not + be flagged. (Consistent with framework-ref-audit deliberately excluding `codev/resources/`.) +- Detecting drift against the **cache tier** (tier 3). The failure class in the field is local-copy + vs *skeleton*; cache drift is a separate concern and stays out to keep the check focused. +- Wiring the report into `codev update` is **optional / SHOULD**, not required (the issue says + "and optionally `codev update`"). The primary home is doctor. + +## Success Criteria +- [ ] Running `codev doctor` in a project with a **byte-identical** local shadow of a skeleton file + prints an informational "redundant copy — safe to remove" line for that file. +- [ ] Running `codev doctor` in a project with a local shadow that **differs** from the skeleton + prints a warning flagged "customized or stale? — adjudicate", naming the file and the skeleton + package version, and increments the doctor warning count. +- [ ] Running `codev doctor` in a project with **no** local framework overrides **and an up-to-date + (or unreachable) skeleton** prints no Framework Drift section at all (true no-op — no false "all + clean", no warnings, no crash). If the skeleton is behind, the staleness warning still surfaces + (that case is not a no-op — it is the issue's sibling failure mode). +- [ ] Skeleton-staleness check reports the **installed and latest versions explicitly** (e.g. + `installed 3.2.1; latest 3.2.3` — per Codex, an explicit pair is crisply testable where a + computed "N versions behind" distance is not) and flags "behind" when installed < latest. It + degrades gracefully (no error, no hang) when offline or the registry is unreachable, bounded by + a short timeout (~2–3s per Gemini). +- [ ] doctor **never** modifies, deletes, or moves any local file as part of the drift check. +- [ ] The drift audit is a standalone, unit-tested library (findings + formatter), mirroring the + pr-gate / framework-ref precedent. +- [ ] **Item 3 (known-default detection) is explicitly non-blocking** for this spec: it MAY ship, but + items 1 (shadow drift) and the staleness check are the required deliverables. The spec is + satisfied without item 3. +- [ ] All new tests pass; existing doctor tests continue to pass. + +## Constraints + +### Technical Constraints +- Must reuse the existing resolver primitives in `lib/skeleton.ts` (`getSkeletonDir`, + `listSkeletonFiles`, `resolveCodevFile`, `hasLocalOverride`) rather than re-deriving skeleton + paths, so the audit and the resolver agree on what the skeleton is. +- The skeleton→local path mapping is a direct prefix relationship: skeleton `protocols/spir/...` + ↔ local `codev/protocols/spir/...` (and `.codev/protocols/spir/...`). +- Must follow the mirror rule (arch-critical): any change touching a framework file must be applied + to **both** `codev/` and `codev-skeleton/`. This feature is primarily product code + (`packages/codev/src`), so the mirror rule applies only if any skeleton doc/text changes. +- Offline tolerance: the staleness check must have a bounded timeout (model on doctor's existing + `runCommand`/`spawnSync` 5s pattern) and must never make doctor hang or fail when the network is + unavailable. +- Report-only: no filesystem mutation of user files. + +### Business Constraints +- None beyond the above. This is internal tooling / adopter-facing diagnostics. + +## Assumptions +- The installed skeleton is the correct "current default" to diff against — i.e. detecting shadow + drift against `getSkeletonDir()` is meaningful. (Staleness check item 3 handles the case where the + installed skeleton is itself behind.) +- npm `latest` for `@cluesmith/codev` is the right staleness baseline (the package the user installs). +- The set of framework subtrees to scan (protocols, consult-types, roles, prompts within them) is + stable enough to enumerate; new framework subtrees would need to be added to the scan set (same + maintenance shape as `PR_PRODUCING_PROTOCOLS` in pr-gate-audit). + +## Solution Approaches + +### Approach 1 (recommended): Skeleton-driven diff via a new `protocol-drift-audit` lib +**Description**: A new pure lib (e.g. `lib/protocol-drift-audit.ts`) enumerates skeleton files under +the in-scope subtrees via `listSkeletonFiles()`, and for each, checks whether a local copy exists in +`.codev/` or `codev/`. If a local copy exists, it byte-compares (hash or direct content) against the +skeleton file and classifies it `identical` | `differs`. A separate function performs the +staleness check (installed version from `version.ts` vs `npm view @cluesmith/codev version`, +bounded + offline-tolerant). doctor consumes findings + formatters, exactly like pr-gate-audit. + +**Pros**: +- Directly mirrors three existing, reviewed precedents — low architectural risk, familiar to reviewers. +- Pure lib is trivially unit-testable with fixture dirs (as pr-gate-audit / framework-ref-audit are). +- Uses the resolver's own notion of the skeleton, so it can't disagree with resolution. + +**Cons**: +- Enumerating "skeleton files that could be shadowed" requires a defined scan set (maintenance point). + +**Estimated Complexity**: Low–Medium +**Risk Level**: Low + +### Approach 2: Local-driven scan (walk `codev/` overrides, look up each in skeleton) +**Description**: Instead of enumerating the skeleton, walk the project's local `codev/` (and +`.codev/`) framework subtrees and, for each local file, ask the resolver/skeleton whether a skeleton +counterpart exists; diff if so. + +**Pros**: Naturally a no-op when the project has no overrides (nothing to walk); closely matches the +"scan the user's overrides" scope of framework-ref-audit. +**Cons**: Symmetric to Approach 1 in effort; both need the same diff + classification. Slightly more +prone to scanning non-framework files that happen to live under `codev/`. + +**Estimated Complexity**: Low–Medium +**Risk Level**: Low + +*Approaches 1 and 2 are near-equivalent; the plan may combine them (drive by skeleton for +completeness, gate output on "project has overrides" for the no-op property). Left to the Plan phase.* + +### Approach 3 (for item 3 / stretch): Ship historical-default hashes +**Description**: Bundle a manifest of SHA hashes of *historical* skeleton versions of each framework +file. A differing local copy whose hash matches a known old default is provably rot → stronger +"safe to delete" recommendation; a local copy matching no known default is (probably) a real +customization. + +**Pros**: Turns the ambiguous "customized or stale?" into a definitive verdict for the common rot case. +**Cons**: Requires generating and maintaining a historical-hash manifest (a build/release step); +higher effort and a new maintenance surface. The issue explicitly marks this "(stretch)". + +**Recommendation**: Specify the mechanism but **defer to a follow-up** unless it fits cheaply within +this PR. Items 1 (shadow drift) and 3 (staleness) deliver the core value; item 2 (known-default) is +an enhancement layered on the same findings. + +## Open Questions + +### Critical (Blocks Progress) +- None. The issue's four numbered items define the requirement. + +### Important (Affects Design) +- **Exact scan set**: which subtrees/files count as "framework files" for shadow drift? Proposed: + `protocols/`, `consult-types/`, `roles/`, and prompt/template `.md` files within `protocols/`. + Explicitly exclude `resources/` (user-evolved). Final enumeration → Plan phase. +- **Staleness data source**: `npm view @cluesmith/codev version` vs a registry HTTP GET. Proposed: + reuse the CLI/spawn approach doctor already uses for external tools, with a short timeout. → Plan. +- **Should the report also wire into `codev update`?** Issue says "optionally". Proposed: SHOULD, + implement if low-cost; not a release blocker. + +### Nice-to-Know (Optimization) +- Whether to offer a `--fix`-style *suggested* command output (copy-pasteable `rm` for byte-identical + redundant copies) without ever executing it. Report-only stays the invariant; this is presentation. + +## Test Scenarios + +### Functional Tests +1. **Identical shadow** → project has `codev/protocols/bugfix/consult-types/x.md` byte-identical to + skeleton → doctor prints "redundant copy, safe to remove" info line; no adjudication warning. +2. **Differing shadow** → same file, one byte changed → doctor prints "customized or stale? — + adjudicate" warning naming the file + skeleton version; warning count increments. +3. **No overrides** → project ships no local framework files → doctor prints no drift warnings and + does not crash (true no-op). +4. **`.codev/` (tier-1) shadow** → a differing copy under `.codev/protocols/...` is detected the same + as a `codev/` copy. +5. **Resources excluded** → a modified `codev/resources/arch.md` is **not** flagged as drift. +6. **Staleness behind** → installed version < npm latest → "N versions behind" reported. +7. **Staleness offline** → registry unreachable / timeout → check degrades gracefully (no hang, no + error, doctor still completes and returns its normal exit code). + +### Non-Functional Tests +1. Staleness check completes within a bounded timeout even with no network. +2. Drift audit does not mutate any file on disk (verify fixtures unchanged after the run). + +## Dependencies +- **Internal**: `lib/skeleton.ts` (resolver primitives), `version.ts` (installed version), + doctor's warning roll-up. Precedent libs: `lib/pr-gate-audit.ts`, `lib/framework-ref-audit.ts`. +- **External**: npm registry (best-effort, for staleness only); `npm`/`git` already assumed present + by doctor. +- **Libraries/Frameworks**: Node stdlib (`node:fs`, `node:crypto` for hashing, `node:child_process`). + +## Security Considerations +- Read-only over local files and the installed skeleton; no file writes to user content → minimal + surface. +- The staleness check spawns a network-bound command (`npm view`); must use a bounded timeout and + must not interpolate untrusted input into a shell (fixed package name; use argv form, not shell + string) — consistent with doctor's existing `spawnSync(cmd, args)` usage. + +## Risks and Mitigation +| Risk | Probability | Impact | Mitigation | +|------|------------|--------|------------| +| Scan set omits a framework subtree → drift missed | Med | Med | Enumerate explicitly; add a test that every skeleton framework subtree is covered; document the maintenance point (like `PR_PRODUCING_PROTOCOLS`). | +| False positive: a legitimately customized file flagged as "stale" | High | Low | Frame as "customized or stale? — adjudicate", never auto-act; item-3 known-default detection (if built) resolves the ambiguity for the rot case. | +| Staleness check hangs doctor offline | Low | High | Bounded timeout + offline-tolerant, mirroring existing `agy`/`codex` probes. | +| Line-ending / trailing-newline noise causes spurious "differs" | Med | Low | Define byte-comparison semantics in the Plan (raw bytes vs normalized); test with a known-identical fixture to catch normalization bugs. | +| Accidental scope creep into resources/ | Low | Med | Explicit exclusion + test scenario 5. | + +## Notes +- This is a diagnostic addition, not a behavior change: the resolver is untouched and no user file is + modified. The value is purely in **making a silent failure class visible**, which is precisely + doctor's contract ("diagnose silent misconfiguration"). +- The lesson this encodes (per lessons-critical): *single source of truth beats distributed state* — + a byte-identical local copy is distributed state with no upside, and drift is what happens when it + rots. doctor can't consolidate for the user, but it can name the divergence. + +--- + +## Amendments + + diff --git a/codev/state/aspir-1210_thread.md b/codev/state/aspir-1210_thread.md new file mode 100644 index 000000000..064d54e5e --- /dev/null +++ b/codev/state/aspir-1210_thread.md @@ -0,0 +1,91 @@ +# aspir-1210 — codev doctor: detect protocol-file drift + +## Context +Issue #1210. Add drift detection to `codev doctor`: +- **Shadow drift**: local `codev/**` (and `.codev/**`) files that also exist in the installed + skeleton — diff them; identical = redundant, differs = "customized or stale? adjudicate". +- **Skeleton staleness**: installed package version vs npm latest (best-effort, offline-tolerant). +- **Known-default detection** (stretch): historical skeleton hashes → provably-rot local copies. +- **No auto-delete** — report only; adjudication stays human. + +No "Baked Decisions" section in the issue → free to explore the design. + +## Key codebase facts (gathered during Specify) +- `codev doctor` lives in `packages/codev/src/commands/doctor.ts`. It already has a mature + pattern of section-by-section checks + `warningDetails` roll-up. Existing analogous audits: + `pr-gate-audit.ts` (#943), `framework-ref-audit.ts` (#1011), `gitignore.ts`. Each is a pure + lib returning findings + a formatter, wired into both `doctor.ts` and (some) `update.ts`. +- Four-tier resolver: `packages/codev/src/lib/skeleton.ts` → `resolveCodevFile()` + (.codev/ → codev/ → cache → skeleton). `getSkeletonDir()` = built `packages/codev/skeleton/`. + `listSkeletonFiles(subdir)` walks the skeleton. `hasLocalOverride()` checks tier-2. +- Skeleton relative path == local path minus the `codev/` prefix (e.g. skeleton + `protocols/spir/protocol.md` ↔ local `codev/protocols/spir/protocol.md`). +- Installed pkg version: `version.ts` (reads package.json). No npm-latest check exists anywhere yet. + +## Progress +- [done] Specify — spec drafted + committed. 3-way spec consult: gemini APPROVE, claude APPROVE, + codex COMMENT. Folded codex/gemini tightenings into spec: explicit scan set, dual override roots + (both `.codev/` and `codev/` reported, winner marked), staleness reports explicit `installed X; + latest Y` (not "N behind"), item-2 marked non-blocking, ~2.5s timeout, raw-byte compare for EOL. +- [done] Plan — 3 phases: (1) `lib/protocol-drift-audit.ts` (shadow drift + staleness), (2) wire + into doctor.ts, (3) unit + e2e tests. Item 2 (historical-hash known-default) deferred to follow-up; + `codev update` wiring deferred (spec: optional). Checks pass. +- Plan 3-way consult: gemini APPROVE, claude APPROVE, **codex REQUEST_CHANGES** (legit): plan + self-contradicted — Exec Summary "no-op when no overrides" vs Phase 2 "staleness always shown". + Resolved with a single unambiguous rule in both spec + plan: Framework Drift section is + **quiet by default** — prints nothing unless a shadow exists OR skeleton is behind. Staleness is + silent when up-to-date/offline; warns only when genuinely behind (the issue's sibling failure mode). + Re-running plan consult after the fix. + +## Implement +- Worktree had NO node_modules on spawn — ran `pnpm install` + built `@cluesmith/codev-core` first + (its .d.ts are needed or tsc floods with module-not-found). Noting for siblings. +- Phase 1 [done]: `lib/protocol-drift-audit.ts` — `auditProtocolDrift(root?, skeletonDir?)`, + `hasFrameworkShadows`, `checkSkeletonStaleness(fetchLatest?)` + formatters. All injectable for tests. + Raw-byte SHA-256 compare; scan set protocols/consult-types/roles; staleness via `npm view` (2.5s, + offline→null). tsc clean, `npm run build` ✓, full suite 3555 passing. Committed. Phase_1 3-way consult running. + +- Phase 1 iter2: codex REQUEST_CHANGES (used custom walk instead of `listSkeletonFiles`) → fixed + (reuse `listSkeletonFiles(sub)` filtered to .md/.json). Re-consult: unanimous APPROVE. Committed. +- Phase 2 [done]: wired "Framework Drift" section into doctor.ts — quiet-by-default (prints only if a + shadow exists OR skeleton behind). Manually verified against THIS repo's real codev/ overrides: + differs→⚠ warnings, identical→○ redundant-copy info, staleness "up to date", `[resolved — live]` + marker all render. e2e doctor tests unaffected (they run in an empty sandbox, no codev/ project). + Committed. Next: phase_2 build+test checks + 3-way consult. + +- Phase 2 iter2: codex REQUEST_CHANGES ×2 (differs line lacked skeleton version; header parenthetical + false in staleness-only path) → both fixed (version threaded via staleness.installed; subtitle + adapts). Re-consult: unanimous APPROVE. +- Phase 3 [done]: unit test (19 cases: identical/differs/no-copy/both-tiers/resources-excluded/ + EOL/no-op/staleness behind|uptodate|offline|throws/formatters/no-mutation/scan-set integrity) + + e2e (3 cases). e2e forces unreachable npm registry so staleness is deterministic ("could not + check") — keeps the no-overrides no-op assertion stable & offline. Unit 19/19, e2e 3/3. Committed. + +- Phase 3 iter1: codex REQUEST_CHANGES + claude COMMENT (same gap): plan's e2e deliverable + "no overrides + skeleton behind → staleness section shown" was unreachable because e2e forced npm + offline everywhere. Fixed with a documented `CODEV_DOCTOR_FAKE_LATEST` env seam in doctor.ts → + added e2e for the staleness-only behind branch; asserted identical=info-only (no adjudicate line); + made EOL unit precondition explicit. Unit 19/19, e2e 4/4. Re-consult (iter2) running. + +- Phase 3 iter2: codex REQUEST_CHANGES (MEDIUM) — plan's 2nd non-functional assertion (staleness + bounded-when-offline) missing; only stub-offline covered. Fixed: exported real `fetchLatestVersion` + + `NPM_LATEST_TIMEOUT_MS`; added unit test hitting unreachable registry asserting null + bounded + time. Unit 20/20. iter3 re-consult running. + +- Phase 3 iter3: unanimous APPROVE. All 3 implement phases done. +## Review +- Wrote review (codev/reviews/1210-...md) with Architecture Updates (cold: no change — faithful + instance of the doctor audit-lib pattern) + Lessons Learned Updates (cold/Testing: test the real + timeout path, not a stub). Creating PR next; will STOP at pr gate for human approval. + +- PR #1223 created + pushed. Worktree clean (only builder-local .builder-*/.claude/hooks untracked). +- PR-review 3-way consult: gemini APPROVE, codex COMMENT (non-blocking), claude APPROVE. + Codex comments both non-actionable: (1) `chore(porch)` commits aren't [Spec] format — those are + porch's own auto-commits, not my feature commits; (2) reviewer couldn't run vitest in read-only + sandbox (I ran full suite green). No changes made. +- **STOPPED at pr gate — awaiting human approval.** Not auto-approving (human-only gate). Architect + notified. Builder will run `porch approve 1210 pr` only after the human explicitly approves. + +- pr gate APPROVED (Waleed, relayed by architect) → ran `porch approve 1210 pr --a-human-explicitly-approved-this`. +- CI: all 6 checks GREEN (Unit, CLI ubuntu/macos, CLI Integration, Tower Integration, Package Install). + Not merging — branch protection requires architect admin-merge. Reported to architect. diff --git a/packages/codev/src/__tests__/cli/doctor-drift.e2e.test.ts b/packages/codev/src/__tests__/cli/doctor-drift.e2e.test.ts new file mode 100644 index 000000000..d25829997 --- /dev/null +++ b/packages/codev/src/__tests__/cli/doctor-drift.e2e.test.ts @@ -0,0 +1,97 @@ +/** + * CLI Integration: `codev doctor` Framework Drift section (#1210). + * + * Runs the built CLI against a fixture project whose `codev/` shadows the installed + * skeleton, asserting the drift report surfaces (or stays silent) as designed. + * Runs against dist/ (built artifact) — the skeleton is read from the built package. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { setupCliEnv, teardownCliEnv, CliEnv, runCodev } from './helpers.js'; +import { getSkeletonDir, listSkeletonFiles } from '../../lib/skeleton.js'; + +/** A real skeleton `.md` framework file to base fixtures on. */ +function pickSkeletonMd(): string { + for (const sub of ['protocols', 'consult-types', 'roles']) { + const files = listSkeletonFiles(sub).filter((f) => f.endsWith('.md')); + if (files.length) return files[0]; + } + throw new Error('no skeleton framework .md files found — build the skeleton first'); +} + +/** Copy a skeleton file into the fixture's tier-2 `codev/`, optionally mutated. */ +function seedLocalCopy(root: string, rel: string, mutate: boolean): void { + const src = path.join(getSkeletonDir(), rel); + const dest = path.join(root, 'codev', rel); + fs.mkdirSync(path.dirname(dest), { recursive: true }); + const bytes = fs.readFileSync(src); + fs.writeFileSync(dest, mutate ? Buffer.concat([bytes, Buffer.from('\nLOCAL DRIFT\n')]) : bytes); +} + +describe('codev doctor — Framework Drift (CLI)', () => { + let env: CliEnv; + let rel: string; + + beforeEach(() => { + env = setupCliEnv(); + // Point npm at an unreachable registry so the staleness check deterministically + // resolves to "could not check" (latest=null → not behind), instead of hitting the + // live registry. This keeps the "no overrides → no section" assertion stable (a real + // `latest` newer than the built version would otherwise open the section on staleness + // alone) and removes network flakiness. Drift detection itself is offline (local vs + // installed skeleton), so the shadow assertions are unaffected. + env.env = { + ...env.env, + npm_config_registry: 'http://127.0.0.1:1', + npm_config_fetch_retries: '0', + }; + rel = pickSkeletonMd(); + // Make the fixture a recognizable codev project so doctor runs its project checks. + fs.mkdirSync(path.join(env.dir, 'codev'), { recursive: true }); + }); + + afterEach(() => { + teardownCliEnv(env); + }); + + it('flags a differing local shadow for adjudication, naming the skeleton version', () => { + seedLocalCopy(env.dir, rel, /* mutate */ true); + const result = runCodev(['doctor'], env.dir, env.env); + expect(result.stdout).toContain('Framework Drift'); + expect(result.stdout).toContain('customized or stale? — adjudicate'); + // The differ line names the skeleton package version (vN.N.N). + expect(result.stdout).toMatch(/differs from installed skeleton v\d+\.\d+\.\d+/); + }); + + it('reports a byte-identical local shadow as info-only (redundant copy, not a warning)', () => { + seedLocalCopy(env.dir, rel, /* mutate */ false); + const result = runCodev(['doctor'], env.dir, env.env); + expect(result.stdout).toContain('Framework Drift'); + expect(result.stdout).toContain('safe to remove'); + // Info-only path: an identical copy must NOT be flagged for adjudication (that is the + // `differs`/warning path). No differing shadow exists here, so no adjudicate line. + expect(result.stdout).not.toContain('customized or stale? — adjudicate'); + }); + + it('prints no Framework Drift section when there are no local overrides and skeleton is current', () => { + // codev/ exists but holds no framework files → no shadows. npm is unreachable (beforeEach) + // so staleness is "could not check" → not behind → the section must be a true no-op. + const result = runCodev(['doctor'], env.dir, env.env); + expect(result.stdout).not.toContain('Framework Drift'); + }); + + it('shows the Framework Drift section for staleness alone when the skeleton is behind (no shadows)', () => { + // No local overrides, but inject a newer npm-latest via the documented test seam so the + // installed skeleton is "behind". The section must open for staleness alone, with the + // staleness-specific subtitle (not the shadowing one) and a behind warning. + const behindEnv = { ...env.env, CODEV_DOCTOR_FAKE_LATEST: '999.0.0' }; + const result = runCodev(['doctor'], env.dir, behindEnv); + expect(result.stdout).toContain('Framework Drift'); + expect(result.stdout).toContain('installed skeleton is behind npm latest'); // staleness subtitle + expect(result.stdout).toMatch(/latest 999\.0\.0 — behind/); + // No shadows → no adjudication line in this path. + expect(result.stdout).not.toContain('customized or stale? — adjudicate'); + }); +}); diff --git a/packages/codev/src/__tests__/protocol-drift-audit.test.ts b/packages/codev/src/__tests__/protocol-drift-audit.test.ts new file mode 100644 index 000000000..3d1fb0a24 --- /dev/null +++ b/packages/codev/src/__tests__/protocol-drift-audit.test.ts @@ -0,0 +1,248 @@ +/** + * Tests for the protocol-file drift audit (#1210). + * + * Strategy: diff project-local copies against the REAL installed skeleton + * (`getSkeletonDir()`), using a temp workspace root (injectable). A byte-identical + * copy of a real skeleton file must classify `identical`; a one-byte mutation must + * classify `differs`. Staleness is exercised via the injectable `fetchLatest` seam + * (no network). + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { tmpdir } from 'node:os'; +import { createHash } from 'node:crypto'; +import { + auditProtocolDrift, + hasFrameworkShadows, + checkSkeletonStaleness, + fetchLatestVersion, + formatDriftFinding, + formatStaleness, + FRAMEWORK_DRIFT_DIRS, + NPM_LATEST_TIMEOUT_MS, + type OverrideTier, +} from '../lib/protocol-drift-audit.js'; +import { getSkeletonDir, listSkeletonFiles } from '../lib/skeleton.js'; +import { version as installedVersion } from '../version.js'; + +/** Pick a real skeleton `.md` framework file (relative path) to use as the baseline. */ +function pickSkeletonMd(): string { + for (const sub of FRAMEWORK_DRIFT_DIRS) { + const files = listSkeletonFiles(sub).filter((f) => f.endsWith('.md')); + if (files.length) return files[0]; + } + throw new Error('no skeleton framework .md files found — build the skeleton first'); +} + +/** Read a skeleton file's raw bytes. */ +function skeletonBytes(rel: string): Buffer { + return fs.readFileSync(path.join(getSkeletonDir(), rel)); +} + +/** Write a project-local copy under a given override tier. */ +function writeLocal(root: string, tier: OverrideTier, rel: string, content: Buffer | string): void { + const p = path.join(root, tier, rel); + fs.mkdirSync(path.dirname(p), { recursive: true }); + fs.writeFileSync(p, content); +} + +const sha = (b: Buffer | string) => createHash('sha256').update(b).digest('hex'); + +describe('protocol-drift-audit', () => { + let root: string; + let rel: string; // a real skeleton framework file + + beforeEach(() => { + root = fs.mkdtempSync(path.join(tmpdir(), 'drift-audit-')); + // A bare codev/ dir so this is recognizably a project root. + fs.mkdirSync(path.join(root, 'codev'), { recursive: true }); + rel = pickSkeletonMd(); + }); + + afterEach(() => { + fs.rmSync(root, { recursive: true, force: true }); + }); + + describe('auditProtocolDrift — shadow classification', () => { + it('classifies a byte-identical local copy as identical (redundant)', () => { + writeLocal(root, 'codev', rel, skeletonBytes(rel)); + const findings = auditProtocolDrift(root); + const f = findings.find((x) => x.relativePath === rel && x.tier === 'codev'); + expect(f).toBeDefined(); + expect(f!.status).toBe('identical'); + expect(f!.isResolvedWinner).toBe(true); + }); + + it('classifies a differing local copy as differs', () => { + writeLocal(root, 'codev', rel, Buffer.concat([skeletonBytes(rel), Buffer.from('\nDRIFT\n')])); + const findings = auditProtocolDrift(root); + const f = findings.find((x) => x.relativePath === rel && x.tier === 'codev'); + expect(f).toBeDefined(); + expect(f!.status).toBe('differs'); + }); + + it('emits no finding for a skeleton file with no local copy', () => { + // Fresh root with only a different local copy present → the untouched file has no finding. + const findings = auditProtocolDrift(root); + expect(findings.find((x) => x.relativePath === rel)).toBeUndefined(); + expect(findings).toHaveLength(0); + }); + + it('detects a tier-1 .codev copy the same as a codev copy', () => { + writeLocal(root, '.codev', rel, Buffer.concat([skeletonBytes(rel), Buffer.from('X')])); + const findings = auditProtocolDrift(root); + const f = findings.find((x) => x.relativePath === rel && x.tier === '.codev'); + expect(f).toBeDefined(); + expect(f!.status).toBe('differs'); + expect(f!.isResolvedWinner).toBe(true); // .codev wins resolution + }); + + it('reports BOTH tiers when a file exists in .codev and codev, marking .codev the winner', () => { + writeLocal(root, '.codev', rel, Buffer.concat([skeletonBytes(rel), Buffer.from('A')])); + writeLocal(root, 'codev', rel, Buffer.concat([skeletonBytes(rel), Buffer.from('B')])); + const findings = auditProtocolDrift(root).filter((x) => x.relativePath === rel); + expect(findings).toHaveLength(2); + const dotCodev = findings.find((x) => x.tier === '.codev')!; + const codev = findings.find((x) => x.tier === 'codev')!; + expect(dotCodev.isResolvedWinner).toBe(true); + expect(codev.isResolvedWinner).toBe(false); // shadowed by .codev, but still reported (still rot) + }); + + it('does NOT scan codev/resources (user-evolved files)', () => { + const resourcePath = path.join(root, 'codev', 'resources', 'arch.md'); + fs.mkdirSync(path.dirname(resourcePath), { recursive: true }); + fs.writeFileSync(resourcePath, 'heavily customized arch doc'); + const findings = auditProtocolDrift(root); + expect(findings.some((x) => x.relativePath.startsWith('resources'))).toBe(false); + }); + + it('classifies an EOL-only difference as differs (conservative raw-byte compare)', () => { + // Precondition made explicit: the baseline file must contain a newline to convert, + // otherwise the CRLF transform is a no-op and the test would vacuously pass. + expect(skeletonBytes(rel).includes(0x0a)).toBe(true); + const crlf = skeletonBytes(rel).toString('utf-8').replace(/\n/g, '\r\n'); + writeLocal(root, 'codev', rel, crlf); + const f = auditProtocolDrift(root).find((x) => x.relativePath === rel); + expect(f!.status).toBe('differs'); + }); + }); + + describe('hasFrameworkShadows — no-op gate over both tiers', () => { + it('is false when the project has no local framework copies', () => { + expect(hasFrameworkShadows(root)).toBe(false); + }); + + it('is true for a tier-2 codev copy', () => { + writeLocal(root, 'codev', rel, skeletonBytes(rel)); + expect(hasFrameworkShadows(root)).toBe(true); + }); + + it('is true for a tier-1 .codev copy (which hasLocalOverride would miss)', () => { + writeLocal(root, '.codev', rel, skeletonBytes(rel)); + expect(hasFrameworkShadows(root)).toBe(true); + }); + }); + + describe('checkSkeletonStaleness', () => { + it('reports behind when installed < latest', () => { + const bumped = installedVersion.replace(/^(\d+)\.(\d+)\.(\d+)/, (_m, a, b, c) => `${a}.${b}.${Number(c) + 1}`); + const s = checkSkeletonStaleness(() => bumped); + expect(s.installed).toBe(installedVersion); + expect(s.latest).toBe(bumped); + expect(s.behind).toBe(true); + }); + + it('reports not-behind when installed === latest', () => { + const s = checkSkeletonStaleness(() => installedVersion); + expect(s.behind).toBe(false); + expect(s.latest).toBe(installedVersion); + }); + + it('is offline-tolerant when latest cannot be fetched (null)', () => { + const s = checkSkeletonStaleness(() => null); + expect(s.latest).toBeNull(); + expect(s.behind).toBe(false); + expect(s.note).toMatch(/offline|could not check/i); + }); + + it('never throws even if the fetcher throws', () => { + const s = checkSkeletonStaleness(() => { + throw new Error('network down'); + }); + expect(s.latest).toBeNull(); + expect(s.behind).toBe(false); + }); + + it('the REAL default lookup is offline-tolerant AND bounded when the registry is unreachable', () => { + // Exercises the real `npm view` path (not an injected stub) against an unreachable + // registry, asserting the non-functional guarantee from the plan: it returns null and + // completes within a bound. ECONNREFUSED is immediate; the spawnSync timeout + // (NPM_LATEST_TIMEOUT_MS) is the hard backstop, so a generous ceiling never flakes. + const prevRegistry = process.env.npm_config_registry; + const prevRetries = process.env.npm_config_fetch_retries; + process.env.npm_config_registry = 'http://127.0.0.1:1'; + process.env.npm_config_fetch_retries = '0'; + try { + const start = performance.now(); + const latest = fetchLatestVersion(); + const elapsedMs = performance.now() - start; + expect(latest).toBeNull(); + expect(elapsedMs).toBeLessThan(NPM_LATEST_TIMEOUT_MS + 5000); + } finally { + if (prevRegistry === undefined) delete process.env.npm_config_registry; + else process.env.npm_config_registry = prevRegistry; + if (prevRetries === undefined) delete process.env.npm_config_fetch_retries; + else process.env.npm_config_fetch_retries = prevRetries; + } + }); + }); + + describe('formatters', () => { + it('names the skeleton version in a differs line', () => { + const line = formatDriftFinding( + { relativePath: rel, tier: 'codev', status: 'differs', isResolvedWinner: true }, + '9.9.9', + ); + expect(line).toContain('v9.9.9'); + expect(line).toContain('adjudicate'); + expect(line).toContain('[resolved'); + }); + + it('marks an identical line as safe to remove', () => { + const line = formatDriftFinding( + { relativePath: rel, tier: 'codev', status: 'identical', isResolvedWinner: true }, + '9.9.9', + ); + expect(line).toContain('safe to remove'); + }); + + it('renders staleness states explicitly', () => { + expect(formatStaleness({ installed: '1.0.0', latest: '1.0.1', behind: true })).toContain('behind'); + expect(formatStaleness({ installed: '1.0.0', latest: '1.0.0', behind: false })).toContain('up to date'); + expect( + formatStaleness({ installed: '1.0.0', latest: null, behind: false, note: 'could not check (offline?)' }), + ).toContain('could not check'); + }); + }); + + describe('safety & integrity', () => { + it('performs no writes — skeleton and local copies are unchanged after an audit', () => { + writeLocal(root, 'codev', rel, skeletonBytes(rel)); + const localPath = path.join(root, 'codev', rel); + const beforeLocal = sha(fs.readFileSync(localPath)); + const beforeSkeleton = sha(skeletonBytes(rel)); + auditProtocolDrift(root); + hasFrameworkShadows(root); + expect(sha(fs.readFileSync(localPath))).toBe(beforeLocal); + expect(sha(skeletonBytes(rel))).toBe(beforeSkeleton); + }); + + it('every scan-set dir exists in the installed skeleton', () => { + for (const sub of FRAMEWORK_DRIFT_DIRS) { + expect(fs.existsSync(path.join(getSkeletonDir(), sub))).toBe(true); + } + }); + }); +}); diff --git a/packages/codev/src/commands/doctor.ts b/packages/codev/src/commands/doctor.ts index 317c1b41a..5bdb83182 100644 --- a/packages/codev/src/commands/doctor.ts +++ b/packages/codev/src/commands/doctor.ts @@ -15,6 +15,12 @@ import { detectHarnessFromCommand } from '../agent-farm/utils/harness.js'; import { auditPrGates, formatPrGateWarning } from '../lib/pr-gate-audit.js'; import { auditStateFileIgnore } from '../lib/gitignore.js'; import { auditFrameworkRefs, formatFrameworkRefFinding, hasFrameworkOverrides } from '../lib/framework-ref-audit.js'; +import { + auditProtocolDrift, + checkSkeletonStaleness, + formatDriftFinding, + formatStaleness, +} from '../lib/protocol-drift-audit.js'; import { resolveAgyBin, AGY_OAUTH_MARKERS } from './consult/index.js'; const __filename = fileURLToPath(import.meta.url); @@ -794,6 +800,64 @@ export async function doctor(): Promise { } console.log(''); + // Framework drift (#1210): a project-local copy (tier-1 `.codev/` or tier-2 `codev/`) of a + // framework file that also ships in the installed skeleton silently shadows the package — a + // stale snapshot of an old default keeps winning resolution forever, with no signal. And the + // installed skeleton can itself be a version behind. Both are invisible in normal operation. + // Report-only (never mutates a user file), and QUIET BY DEFAULT: the section prints only when it + // has something actionable to say — a shadow exists OR the skeleton is behind. No overrides + + // up-to-date/offline => true no-op (no section at all). + const drift = auditProtocolDrift(workspaceRoot); + // Test seam: `CODEV_DOCTOR_FAKE_LATEST` injects the npm-latest version so the + // staleness-only "behind" integration branch is e2e-testable without a live + // registry. Unset in real use → the actual `npm view` lookup runs. + const fakeLatest = process.env.CODEV_DOCTOR_FAKE_LATEST; + const staleness = checkSkeletonStaleness(fakeLatest ? () => fakeLatest : undefined); + if (drift.length > 0 || staleness.behind) { + // Header parenthetical reflects the actual finding: the shadowing subtitle is only accurate + // when a local shadow exists; in the staleness-only path (no shadows, skeleton behind) it + // would be false, so use a staleness-specific subtitle instead. + const subtitle = drift.length > 0 + ? 'local copies shadowing the installed skeleton' + : 'installed skeleton is behind npm latest'; + console.log(chalk.bold('Framework Drift') + ` (${subtitle})`); + console.log(''); + + // Staleness line. Only `behind` is a warning; up-to-date / uncheckable lines are informational + // and shown only because the section is already open (a shadow or a behind result opened it). + if (staleness.behind) { + console.log(` ${chalk.yellow('⚠')} ${formatStaleness(staleness)}`); + warnings++; + warningDetails.push({ + name: 'Skeleton staleness', + issue: `installed ${staleness.installed} < latest ${staleness.latest}`, + recommendation: 'run: codev update (and reinstall @cluesmith/codev if globally out of date)', + }); + } else { + console.log(` ${chalk.dim('○')} ${formatStaleness(staleness)}`); + } + + // Shadow drift. `differs` => adjudicate warning; `identical` => informational (redundant copy). + // The skeleton version IS the installed package version (the skeleton ships with the + // package), which staleness already resolved — name it in each drift line per the spec. + const skeletonVersion = staleness.installed; + const differs = drift.filter((f) => f.status === 'differs'); + const identical = drift.filter((f) => f.status === 'identical'); + for (const f of differs) { + console.log(` ${chalk.yellow('⚠')} ${formatDriftFinding(f, skeletonVersion)}`); + warnings++; + warningDetails.push({ + name: 'Framework drift', + issue: `${f.tier}/${f.relativePath} differs from installed skeleton v${skeletonVersion} (customized or stale?)`, + recommendation: 'review vs the installed skeleton; if unintentional, remove the local copy so resolution falls back to the package', + }); + } + for (const f of identical) { + console.log(` ${chalk.dim('○')} ${formatDriftFinding(f, skeletonVersion)}`); + } + console.log(''); + } + // Full forge concept reporting: all 15 concepts with resolution source and executable check const forgeConfig = loadForgeConfig(workspaceRoot); const provider = forgeConfig?.provider ?? 'github'; diff --git a/packages/codev/src/lib/protocol-drift-audit.ts b/packages/codev/src/lib/protocol-drift-audit.ts new file mode 100644 index 000000000..30b612e3f --- /dev/null +++ b/packages/codev/src/lib/protocol-drift-audit.ts @@ -0,0 +1,264 @@ +/** + * Protocol-file drift audit (#1210). + * + * Detects two silent failure modes of the four-tier resolver + * (`.codev/` → `codev/` → cache → installed skeleton): + * + * 1. **Shadow drift** — a project-local copy (tier-1 `.codev/` or tier-2 `codev/`) + * of a framework file that ALSO ships in the installed skeleton. The resolver + * serves the local copy, so a stale snapshot of an old upstream default keeps + * winning forever, silently, even after the package ships a fix. This audit + * diffs each local copy against its skeleton counterpart and classifies it: + * - `identical` → a redundant copy that adds nothing but risk (safe to remove; + * resolution then falls back to the package). + * - `differs` → "customized or stale? — adjudicate" (a human must decide; + * we NEVER auto-act). + * + * 2. **Skeleton staleness** — the installed `@cluesmith/codev` package is itself a + * version behind npm `latest`, so even NON-shadowed resolution serves pre-fix + * framework files. Reported as an explicit `installed X; latest Y` pair + * (a computed "N behind" distance is not crisply testable), best-effort and + * offline-tolerant. + * + * Report-only: this module performs NO writes to disk. Adjudication stays human — + * local copies may be deliberate customizations (that is the resolver's whole point). + * + * Mirrors the existing doctor audit precedents: `pr-gate-audit.ts` (#943) and + * `framework-ref-audit.ts` (#1011) — a pure lib returning findings + formatters, + * consumed by `commands/doctor.ts`. + */ + +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { createHash } from 'node:crypto'; +import { spawnSync } from 'node:child_process'; +import { getSkeletonDir, listSkeletonFiles, resolveCodevFile, findWorkspaceRoot } from './skeleton.js'; +import { version as installedVersion } from '../version.js'; + +/** + * The framework subtrees whose files ship in the skeleton and resolve via the + * four-tier resolver. Pinned scan set (per the spec) — prompts and per-protocol + * templates live WITHIN `protocols/`, so they are covered transitively. + * + * Maintenance point: a new top-level framework subtree must be added here (same + * shape as `PR_PRODUCING_PROTOCOLS` in pr-gate-audit). `codev/resources/` is + * deliberately EXCLUDED — those are user-evolved files (arch.md, lessons-learned.md, + * and their -critical companions), not framework files, and must never be flagged. + */ +export const FRAMEWORK_DRIFT_DIRS = ['protocols', 'consult-types', 'roles'] as const; + +/** Only these extensions are framework files worth diffing. */ +const FRAMEWORK_EXTS = new Set(['.md', '.json']); + +/** The two project-local override roots, in resolver-precedence order (tier 1 first). */ +const OVERRIDE_TIERS = ['.codev', 'codev'] as const; +export type OverrideTier = (typeof OVERRIDE_TIERS)[number]; + +export type DriftStatus = 'identical' | 'differs'; + +export interface DriftFinding { + /** Path relative to the skeleton/override root, e.g. `protocols/spir/protocol.md`. */ + relativePath: string; + /** Which local override root holds this copy. */ + tier: OverrideTier; + /** Byte-comparison result against the skeleton counterpart. */ + status: DriftStatus; + /** + * Whether the four-tier resolver actually resolves THIS copy (i.e. it is the + * live one the runtime loads). A tier-2 `codev/` copy shadowed by a tier-1 + * `.codev/` copy is still reported (it is still rot), but marked not-the-winner. + */ + isResolvedWinner: boolean; +} + +export interface StalenessResult { + /** Installed `@cluesmith/codev` version. */ + installed: string; + /** npm `latest`, or null when the registry could not be reached. */ + latest: string | null; + /** True only when a latest version was obtained AND installed < latest. */ + behind: boolean; + /** Human note when latest could not be determined. */ + note?: string; +} + +/** Injectable seam for the npm-latest lookup so tests are deterministic (no network). */ +export type FetchLatest = () => string | null; + +/** SHA-256 of a file's raw bytes (no text decoding — EOL/trailing-newline sensitive). */ +function hashBytes(filePath: string): string { + return createHash('sha256').update(fs.readFileSync(filePath)).digest('hex'); +} + +/** + * Skeleton-relative framework files under a scan-set subtree, filtered to + * framework extensions. Enumerated via the resolver's own `listSkeletonFiles` + * (over `getSkeletonDir()`) so this audit and runtime resolution agree on exactly + * what the skeleton is. Returned paths (e.g. `protocols/spir/protocol.md`) are + * both the skeleton-relative and the override-relative path. + */ +function skeletonFrameworkFiles(sub: string): string[] { + return listSkeletonFiles(sub).filter((rel) => FRAMEWORK_EXTS.has(path.extname(rel))); +} + +/** + * Audit a project for shadow drift against the installed skeleton. + * + * For every framework file in the skeleton (under FRAMEWORK_DRIFT_DIRS), checks + * whether a local copy exists in `.codev/` and/or `codev/`; for each local copy + * found, byte-compares it against the skeleton file and records a finding. Returns + * one finding per local copy (so a file present in BOTH tiers yields two findings). + * + * @param workspaceRoot - project root (auto-detected via findWorkspaceRoot if omitted) + */ +export function auditProtocolDrift(workspaceRoot?: string): DriftFinding[] { + const root = workspaceRoot ?? findWorkspaceRoot(); + const skeletonDir = getSkeletonDir(); + const findings: DriftFinding[] = []; + + for (const sub of FRAMEWORK_DRIFT_DIRS) { + for (const rel of skeletonFrameworkFiles(sub)) { + const skeletonPath = path.join(skeletonDir, rel); + let skeletonHash: string; + try { + skeletonHash = hashBytes(skeletonPath); + } catch { + continue; // unreadable skeleton file — nothing to diff against + } + + // Which copy does the resolver actually pick? (absolute path or null) + const resolved = resolveCodevFile(rel, root); + const resolvedAbs = resolved ? path.resolve(resolved) : null; + + for (const tier of OVERRIDE_TIERS) { + const localPath = path.join(root, tier, rel); + if (!fs.existsSync(localPath) || !fs.statSync(localPath).isFile()) continue; + + const status: DriftStatus = + hashBytes(localPath) === skeletonHash ? 'identical' : 'differs'; + findings.push({ + relativePath: rel, + tier, + status, + isResolvedWinner: resolvedAbs === path.resolve(localPath), + }); + } + } + } + + return findings; +} + +/** + * Whether the project has ANY local copy of a scanned skeleton file, in EITHER + * override root. Cheap existence-only scan (no hashing) used for the no-op gate: + * the existing `hasLocalOverride()` only checks tier-2 `codev/`, so drift detection + * must check both tiers itself. + */ +export function hasFrameworkShadows(workspaceRoot?: string): boolean { + const root = workspaceRoot ?? findWorkspaceRoot(); + for (const sub of FRAMEWORK_DRIFT_DIRS) { + for (const rel of skeletonFrameworkFiles(sub)) { + for (const tier of OVERRIDE_TIERS) { + if (fs.existsSync(path.join(root, tier, rel))) return true; + } + } + } + return false; +} + +/** Parse a dotted version into numeric parts; non-numeric segments → 0. */ +function parseVersion(v: string): number[] { + return v.split('.').map((p) => parseInt(p.replace(/[^0-9]/g, ''), 10) || 0); +} + +/** True if `a` is strictly older than `b` (semver-ish, major.minor.patch). */ +function versionLt(a: string, b: string): boolean { + const pa = parseVersion(a); + const pb = parseVersion(b); + for (let i = 0; i < Math.max(pa.length, pb.length); i++) { + const x = pa[i] || 0; + const y = pb[i] || 0; + if (x < y) return true; + if (x > y) return false; + } + return false; +} + +/** Bound on the npm-latest lookup (ms). The registry can't stall doctor beyond this. */ +export const NPM_LATEST_TIMEOUT_MS = 2500; + +/** + * Default npm-latest lookup: `npm view @cluesmith/codev version`. Bounded by + * `NPM_LATEST_TIMEOUT_MS` (spawnSync kills the child past it) and offline-tolerant — + * an unreachable registry, non-zero exit, missing `npm`, or unparsable output all + * yield `null` rather than throwing or hanging. Exported so tests can assert the + * real bounded/offline behavior (not just an injected stub). + */ +export function fetchLatestVersion(): string | null { + try { + const r = spawnSync('npm', ['view', '@cluesmith/codev', 'version'], { + encoding: 'utf-8', + timeout: NPM_LATEST_TIMEOUT_MS, + stdio: 'pipe', + }); + if (r.status === 0 && r.stdout) { + const v = r.stdout.trim(); + return /^\d+\.\d+\.\d+/.test(v) ? v : null; + } + return null; + } catch { + return null; + } +} + +/** + * Compare the installed package version against npm `latest`. Never throws and + * never hangs beyond the fetch timeout — an unreachable registry yields + * `{ latest: null, behind: false, note }` so doctor stays usable offline. + * + * @param fetchLatest - injectable latest-version source (defaults to `npm view`) + */ +export function checkSkeletonStaleness( + fetchLatest: FetchLatest = fetchLatestVersion, +): StalenessResult { + const installed = installedVersion; + let latest: string | null = null; + try { + latest = fetchLatest(); + } catch { + latest = null; + } + if (!latest) { + return { installed, latest: null, behind: false, note: 'could not check (offline?)' }; + } + return { installed, latest, behind: versionLt(installed, latest) }; +} + +/** + * Render a shadow-drift finding for doctor output. + * + * @param skeletonVersion - installed skeleton/package version; when provided it is + * named in the line so the human can tell WHICH skeleton the local copy diverged + * from (the spec requires the adjudication warning to name the package version). + */ +export function formatDriftFinding(f: DriftFinding, skeletonVersion?: string): string { + const loc = `${f.tier}/${f.relativePath}`; + const ver = skeletonVersion ? ` v${skeletonVersion}` : ''; + if (f.status === 'identical') { + return `${loc} — byte-identical redundant copy of the installed skeleton${ver}; safe to remove (resolution then falls back to the package)`; + } + const winner = f.isResolvedWinner ? ' [resolved — this copy is live]' : ''; + return `${loc} — differs from installed skeleton${ver}; customized or stale? — adjudicate${winner}`; +} + +/** Render the staleness result for doctor output. */ +export function formatStaleness(s: StalenessResult): string { + if (s.latest === null) { + return `installed ${s.installed}; latest: ${s.note ?? 'unknown'}`; + } + if (s.behind) { + return `installed ${s.installed}; latest ${s.latest} — behind (run: codev update)`; + } + return `installed ${s.installed}; latest ${s.latest} (up to date)`; +}