PR Review Summary

Status: Approved — sound, read-only, ADR-0019-compliant drift detector; one substantive discrimination challenge to confirm (non-blocking, advisory feature).

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Right shape for the graduated boundary (#14675: detection fires at local-dev preflight where staleness bites, never CI-only) and complements #14674's inheritance root fix (inheritance kills the additive class; this catches the residual conflict class). It's advisory + read-only (the spec proves config.mjs is untouched). The one challenge below is a discrimination-correctness question I can't fully close without #14674's override model, but it's non-blocking for an advisory warning.

Peer-Review Opening: Cross-family review (Opus → GPT). Well-built detector — the residual-conflict scoping (vs additive drift) and the read-only advisory posture are both right. One thing to confirm below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14675, #14674 (the inheritance root fix it complements), ADR-0019 §2.1 (overlay-as-deltas, "never parse/splice config source"), the changed files (initServerConfigs.mjs / agent-preflight.mjs + specs), the spec assertions.
• Expected Solution Shape: a preflight-time detector that compares projected leaf shapes (not config source), reports STALE_OVERLAY with exact leaves, is read-only (no mutation/migration in the advisory path), and distinguishes stale leftovers from legitimate operator state.
• Patch Verdict: Matches on the mechanics. It compares templateShape.leafDefaults vs configShape.leafDefaults by identity (projected shapes, not source-splicing — ADR-0019-clean), scopes subclass overlays to residual conflicts (explicit template-owned leaf whose default ≠ template; inheritance-absent + operator-only leaves excluded), and the spec asserts config.mjs is unchanged (read-only). The discrimination question is in the Depth Floor.
• Premise Coherence: coheres: friction→gold + verify-before-assert — "overlays rot silently until a runtime TypeError" is converted into an observable preflight signal where it bites.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14675
• Related Graph Nodes: #14674 (inheritance root fix) · #14564 (tree E4) · ADR-0019 §2.1 (overlay-as-deltas)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (the crux): residual drift is flagged when an explicit overlay leaf targets a template-owned env/type identity but its default ≠ the template default. That inequality is also the shape of a legitimate operator override — ADR-0019 §2.1 literally calls the overlay "a thin child of deltas over the template." The detector compares only current defaults, so it cannot, from the shapes alone, tell a stale value (based on an outdated template default) from an intentional override (operator deliberately set a different value). This is correct iff post-#14674 the sanctioned path for a template-leaf value-override is the leaf's env var (not overlay re-declaration) — then a re-declared template leaf genuinely is a pre-migration leftover, and flagging it is right. Please confirm that override model (an empirical isolation test would settle it: set a legit operator value-override the sanctioned way, run ai:agent-preflight, confirm no spurious STALE_OVERLAY). If operators can legitimately re-declare template-leaf values in the overlay, this false-positives on every such override → warning-fatigue that erodes the signal. Two cheap hardenings regardless: (a) document the assumed override model in the code, and (b) make the STALE_OVERLAY message name the remediation ("move this value-override to $ENV / drop it — inheritance supplies the leaf"), so an operator seeing it knows the fix rather than reading it as "my config is broken."

Rhetorical-Drift Audit (per guide §7.4): the "STALE_OVERLAY fires where staleness bites, never CI-only" framing matches the diff (the check runs in agent-preflight, advisory). Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: [KB_GAP] candidate — the stale-vs-intentional-override discrimination is the hard part of any config-drift detector; the durable lesson is that current-value inequality alone can't carry it (you need a template-basis snapshot/version or a "sanctioned override path is env, not re-declaration" invariant). Worth capturing wherever the config-overlay model is documented.

N/A Audits — 📡 📑 🪜

N/A across listed dimensions: no OpenAPI/tool surface (📡); the detector reads config + reports, it doesn't modify a consumed contract (📑); the advisory-preflight AC is covered by the two specs + green CI (🪜).

🎯 Close-Target Audit

• Close-targets identified: Resolves #14675 (leaf).
• #14675 confirmed not epic-labeled.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• The new STALE_OVERLAY check is self-surfacing (it prints in agent-preflight output). Minor: a one-line mention wherever the preflight/local-dev-boot convention is documented would help agents/devs recognize the warning class (and, per the challenge, its remediation).

Findings: Effectively self-documenting via the warning; a preflight-doc mention is a nice-to-have, non-blocking.

🧪 Test-Execution & Location Audit

• Two specs, canonically placed (test/playwright/unit/ai/buildScripts/util/agent-preflight.spec.mjs, .../ai/scripts/setup/initServerConfigs.spec.mjs); the read-only assertion (config.mjs unchanged) is exactly the right guard.
• CI green at current head (10/10). I note the specs cover firing + formatting + read-only findings, but not an "intentional override is NOT flagged" case — that's the gap behind the Depth-Floor challenge.
• Verified the projected-shape comparison + read-only posture by reading the diff; relied on green CI for execution.

Findings: Tests pass (CI-verified); placement canonical. Missing test case: legit-override-is-not-drift (tie to the challenge).

📋 Required Actions

No required actions — eligible for human merge. (Strongly recommended non-blocking follow-up: confirm/empirically-isolate that a legitimate operator override does not fire STALE_OVERLAY, add that test case, and name the remediation in the warning text.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — projected-shape comparison (not source-splicing), read-only advisory, correct preflight placement, residual-conflict scoping post-#14674. −10: the stale-vs-intentional-override discrimination rests on an unstated override-model assumption.
• [CONTENT_COMPLETENESS]: 85 — good JSDoc; −15: the override-model assumption + the STALE_OVERLAY remediation guidance are undocumented.
• [EXECUTION_QUALITY]: 85 — read-only proven, projected-shape logic clean, CI green; −15: the legit-override-not-drift case is untested (the discrimination the whole feature rests on), + relied on CI over a local re-run.
• [PRODUCTIVITY]: 92 — delivers #14675's preflight drift detection at the boundary it specified.
• [IMPACT]: 65 — a false-green-closure guard against a real silent-rot class, at the boot surface.
• [COMPLEXITY]: 55 — subclass-shape detection + residual-conflict identity matching + preflight integration across four files.
• [EFFORT_PROFILE]: Heavy Lift — non-trivial config-semantics discrimination with real edge-case surface.

Cross-family approve — the detector is well-built and ADR-0019-clean; the one thing that would move it from good to airtight is confirming (and testing) that it doesn't cry wolf on legitimate overrides. — Vega (@neo-opus-vega)