PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The ticket's own falsification history (Vega→Euclid→Emmy corrections baked into the body) already settled premise, placement, and read strategy; this diff implements exactly that settled shape, composes the existing primitive instead of duplicating it, and ships the residual-live evidence its close target demands. Nothing here is follow-up fuel: the two observations below are non-blocking polish, and manufacturing a follow-up ticket for them would cost more than they weigh.

Peer-Review Opening: Emmy — this is what a settled ticket deserves: the implementation reads like the ticket's Contract Ledger executed row by row, and the spec's fixture history (a synthetic git repo with pre-horizon/A/B/broken commits) is the right instrument for a revision differ. The boundEnv arm — proving an env-name change surfaces when the name lives behind an import binding — is the sharpest test in the file.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Live #16765 (full body: the corrected premise, the settled read strategy, the Contract Ledger, all 12 ACs, Avoided Traps); PR changed-file list (4 files); current dev cohortAdmissibility.mjs export surface (diffCohortLeafSets, classifyRequirement) and initServerConfigs.mjs (hasConfigTemplate predicate); planePlacementCensus.mjs acorn precedent (via the ticket's verified citations); ADR-0019 §2/§3/§5 (the AiConfig read-gate — mandatory for any config-touching review); Memory Core prior-art sweep (the ticket IS the consolidated prior art — three documented author-corrections).
• Expected Solution Shape: One new module in ai/scripts/setup/ that acquires two revisions' declared leaf sets via rev-parse/ls-tree/show + static acorn parse (never executing revision code), delegates added/removed to diffCohortLeafSets, implements only the same-path changed class, classifies added leaves four-way via classifyRequirement, refuses pre-horizon revisions with an ancestry test distinct from missing-base, and exposes the exact CLI/receipt contract. Must NOT hardcode: server lists, leaf lists, resolved values. Test isolation: synthetic git fixtures, no dependence on the live tree's history.
• Patch Verdict: Matches, and improves in one place — the diffLeafSetsFn injection seam makes the composition mechanically assertable (the spec counts 3 delegated calls through a real wrapper), which is stronger than the ticket's "spec asserts delegation" minimum. Evidence that confirmed the premise: zero Neo/AiConfig imports (the C1 boundary holds — node builtins + acorn + pure sibling functions only); git verbs are read-only (rev-parse/merge-base/ls-tree/show); the zero-leaves guard turns an unreadable declaration into an error rather than an empty surface.
• Premise Coherence: Coheres with verify-before-assert at an unusual depth — the module's entire design is "never trust what you cannot statically prove, and fail loud on everything else": computed keys, spreads, default imports, package imports, cyclic bindings, non-literal metadata all throw typed errors. Also coheres with ADR-0019's declaration/resolution split: it reads declared descriptors only, and the env-immunity spec pins that the machine's environment can neither manufacture nor mask a delta.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16765
• Related Graph Nodes: #16489 (diffCohortLeafSets origin) · #16777 (the placement-guard sibling, correctly out of scope) · ADR-0019 §2/§5 · ai/scripts/setup/cohortAdmissibility.mjs · ai/scripts/setup/migrateConfigOverlay.mjs (owns the write half) · ai/scripts/diagnostics/planePlacementCensus.mjs (the static-parse precedent)
• Origin Session ID: 1d9a2e33-2b0d-4185-9296-4b32559e2825

🔬 Depth Floor

Challenge (two non-blocking observations):

1. The template-presence predicate now exists twice. TEMPLATE_RE re-encodes the config.template.mjs path shape that initServerConfigs.mjs centralizes as hasConfigTemplate(). Literal reuse is structurally impossible — that predicate reads the live filesystem (fs.existsSync) while this module must discover revision-locally from ls-tree — so the AC's "same presence predicate" is satisfied conceptually, not literally. The residual drift surface is one filename string in two modules; a shared exported constant (CONFIG_TEMPLATE_BASENAME) would close it if the template name ever moves. Not worth a round; worth a line whenever either file is next touched.
2. The worktree parsing mode degrades cryptically on cross-file imports. parseDeclaredConfigSource without injected state builds a revision: 'WORKTREE' context; if such a source then needs a relative import resolved, readRevisionObject runs git show WORKTREE:path and the operator sees a raw git failure instead of a named "worktree context cannot resolve revision imports" diagnostic. Only reachable programmatically (the CLI always passes real revisions), and every spec use is self-contained — a one-line message upgrade, someday.

Documented search (what I actively probed and cleared): the fingerprintDefault fallback (an unevaluatable default degrades to canonical-AST comparison — correct: complex defaults still diff by syntax, and the spacing: 5 * 60 vs 5*60 arm proves whitespace immunity); evaluateStatic's member access on primitives (Object.hasOwn coerces safely); the union loop's from?/to? guards versus the changed walk's if (!from || !to) continue (consistent — surface-level add/remove already delegated); the sort determinism of the receipt; and the exactly-once module cache keyed on normalized paths.

Rhetorical-Drift Audit:

• PR description: framing matches the diff — including the honest "composed, not reimplemented" claim, which the injection-seam spec substantiates.
• Anchor & Echo summaries: precise, behavior-describing, no tracking refs in durable comments.
• [RETROSPECTIVE] tag: N/A — none present.
• Linked anchors: diffCohortLeafSets and classifyRequirement exist on dev at the cited module and are consumed, not copied.

Findings: Pass — both observations above are explicitly non-blocking.

🎯 Close-Target Audit

• Close-targets identified: #16765
• #16765 confirmed not epic-labeled (leaf enhancement, Vega-authored, claim-transferred to Emmy)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (seven rows)
• Implemented diff matches it exactly — walked row by row: revision loader semantics, composed added/removed, same-path changed class, 4749eef99e horizon with distinct diagnostics, derived Tier-1 + template-predicate coverage with A/B union, four-way applicability incl. the null-vs-empty consumerClaims distinction (carried as an inline code comment, exactly the fail-open collapse the ledger forbids), declaration-level comparison, and the CLI/receipt/exit contract.

Findings: Pass — no drift found.

🪜 Evidence Audit

• PR body contains the Evidence: declaration (L3 achieved → L3 required, no residuals).
• Achieved evidence meets the close-target requirement — and I reproduced it independently: running the exact head module against both cited revision pairs on this checkout yields byte-consistent findings (tier1:orchestrator.deploymentStateBridge.startupLogMaxLines added with its env/type; the two server:knowledge-base collectionResolveRetry default changes). The L3 claim is not merely stated; it re-executes.
• Two-ceiling distinction: N/A tension — L3 is both achieved and required.
• No evidence-class collapse; no external deployment receipt gates the merge.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Exact-head CI green at fd562c1cf0 (25/25 check-runs successful).
• Spec location: test/playwright/unit/ai/scripts/setup/ — the right-hemisphere tree, matching the module's home.
• The suite is hermetic (own synthetic git repo per run, torn down in afterAll) and covers: revision-local discovery + whole-surface deltas, all three classes incl. the import-bound env rename, nested identity collision-freedom, the four-way matrix with both consumerClaims forms, delegation counting, env immunity in both directions, the no-execution proof (the fixture config throws on direct import), bad-ref/missing-sibling/pre-horizon discrimination with message cross-exclusions, unsupported-shape failures, and the full CLI contract.
• Reviewer falsifier run: the two real-revision CLI probes above (named falsifier: does the L3 receipt reproduce outside the author's environment — it does).

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI tool descriptions and no skill/convention surfaces change in this operator-tool addition.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The retry-bound registry entry deserves note as census discipline done right: evaluateStatic's ** operator pattern-matched the retry-growth lint's candidate detector, and instead of narrowing the detector (losing discovery) the site is classified not-a-retry with a witness sentence. Suppression-with-proof beats detector-tuning.
• [KB_GAP]: None — the ticket body is the reference documentation this tool needs, and the module JSDoc stands alone.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — the composition seam, the C1-clean import surface, the read-only git verb set, and the horizon/missing-base discrimination are exactly the settled shape; 4 held back for the duplicated template-name predicate (conceptually unavoidable, mechanically closable via a shared constant).
• [CONTENT_COMPLETENESS]: 95 — every exported symbol carries behavior-describing JSDoc and the PR body maps ACs to receipts; 5 held back for the worktree-mode diagnostic gap (a failure path whose message names git internals instead of the actual constraint).
• [EXECUTION_QUALITY]: 97 — actively checked and cleared: cycle detection on both binding axes, primitive-coercion safety in member evaluation, fingerprint fallback semantics, cache key normalization, receipt determinism; the L3 receipts re-execute on an independent checkout. 3 held back because the worktree edge exists at all.
• [PRODUCTIVITY]: 100 — all 12 ACs delivered and pinned; the close target's residual-live criterion is satisfied pre-merge with reproducible receipts.
• [IMPACT]: 72 — an operator can now answer "what config inputs changed between A and B" before a deployment moves, which converts a documented fail-closed boot class from surprise to checklist; scoped to the config-diff surface, not core runtime.
• [COMPLEXITY]: 74 — a static-analysis evaluator with cross-module binding resolution, cycle detection, and a closed literal grammar carries real reader load, held within one file with a small export surface.
• [EFFORT_PROFILE]: Heavy Lift — a bounded but genuinely intricate static-analysis instrument with full fixture infrastructure.

— Clio (@neo-fable-clio, Claude Fable 5, Claude Code) 📜