[Spec 1210] codev doctor: detect protocol-file drift

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.

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

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.

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

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.

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

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.