PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise, placement, and instrument design are right — this is the epic's AC-B delivered with a stronger verification surface than the ticket demanded. Three bounded, in-place repairs remain: one operational defect in the artifact that ships to the constrained plane (boot-time registry dependency), one missing mechanical bound that a day-one binding requirement demands living in exactly the contract this PR creates (the embedding model's hard context ceiling), and one body-completeness line for a real behavior removal. None of these transfers scope; all are same-head repairable — so Request Changes, not Approve+Follow-Up.

Peer-Review Opening: This is the strongest compose artifact in the deploy tree, Emmy. The :?-required envelope (the election owns every resource value — this profile refuses to guess), four-level digest pinning (engine image, model manifest, weights blob, GGUF sha), both-direction drift validation, and the census-vs-grep set-equality instrument are each individually better than what the ticket asked for. The three Required Actions below are small relative to the diff — two are single-block edits, one is a body line.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17021 (AC list + Contract Ledger), epic #17018 AC-B, D#17015 r6 converged rows, ADR-0014 §8 amendment (the topology authority), ADR-0019 (read per §critical_gates 10 before this review), current dev source of providerDispatch.mjs / providerReadinessHelper.mjs / InferenceLifecycleService.mjs / base docker-compose.yml, and the 20-file changed list.
• Expected Solution Shape: Role selectors resolved at use sites through the existing graph-dispatch seam (no new construction path); all three selectors projected into every owning Compose service; one new two-lane canonical overlay with digest-pinned engines/models whose resource values come from the #17024 election rather than being guessed; readiness derived from active roles with a wrong-lane regression; a machine-readable composition receipt that fails on an inconsistent split. Must NOT hardcode: the {1,2,4} election result (#17024), generation-election semantics (#17023), any external-plane probe. Test isolation: committed templates + stubs, zero live endpoints.
• Patch Verdict: Matches and locally improves the expected shape. Confirming evidence: every lane resource value is ${…:?}-required (election-owned, never defaulted); getGraphProviderReadinessTarget now emits roles[] and the spec proves an ollama graph lane requires no openAiCompatible embedding model; the role-lane-assignment fixture refuses a graph role moved onto the embedding lane even when internally self-consistent; ConceptDiscoveryService routes through buildGraphProvider/resolveGraphModelProvider — verified against providerDispatch.mjs:29/:69 on dev as the seam's documented contract, so the config blocks are the established construction shape, not new pass-along.
• Premise Coherence: Coheres — verify-before-assert is built INTO the artifact: the receipt validates rendered reality against the declared contract and against live source anchors instead of trusting any description. No value-surface conflicts.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17021
• Related Graph Nodes: #17018 (epic), D#17015 r6, ADR-0014 §8 (via #17019/PR #17020), ADR-0019, #17024 (receipt consumer), #17026 (cutover), #17023 (generation election — correctly untouched)
• Origin Session ID: 8637b4b9-b852-45d9-b057-de34184aae8b

🔬 Depth Floor

Challenge (per guide §7.1):

1. Boot-time registry dependency (RA 1): the chat lane's boot command runs ollama pull unconditionally before its digest checks. With warm cache and an unreachable/throttled registry.ollama.ai, the pull exits non-zero, sh -ec kills the lane, and every application service gates on service_healthy — a registry outage bricks a plane whose weights are already verified on local disk. The embedding lane's own command short-circuits the network when its sha matches; the two lanes in the same file carry opposite failure postures.
2. No model context ceiling in the contract (RA 2): contextTokensPerSlotRequired is a floor; nothing bounds it from above by what the pinned model can natively serve. An election that sets per-slot context above the embedding model's hard 32k window renders, passes every current check, and silently degrades every vector — the exact class the two-lane design exists to prevent, and a binding day-one requirement in the D#17015→#17018 lineage.
3. Census completeness instrument is asymmetric (non-blocking): the grep-vs-census set-equality proof covers the graph role only; embedding/model/ask consumers get anchor-existence (rot detection) but not omission-proofing. A future TextEmbeddingService.embed* caller lands outside the census silently. Reasonable to extend in #17024/#17026 — flagging so it's a decision, not an accident.
4. Float-sum equality (non-blocking): total.cpuCores === chat + embedding is exact float equality. Quarter-core values are binary-exact so current practice is safe, but a 1.1+2.3 election would fail the sum spuriously — worth a comment or epsilon when the schema next evolves.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff with one gap — the body nowhere names that InferenceLifecycleService DELETED the local-inference spawn capability (child_process/homebrew/MLX resolution paths). "Readiness … now follow active role ownership" under-states a real capability removal (RA 3).
• Anchor & Echo summaries: precise; the census rows carry exact source anchors that the analyzer greps — self-verifying prose.
• [RETROSPECTIVE]-class claims: none inflated.
• Linked anchors: #17024/#17026 ownership claims match the epic's leaf split.

Findings: one body-completeness drift → RA 3.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the diff demonstrates correct use of the dispatch seam, the leaf/env layering, and the §10.7 profile-placement discipline.
• [TOOLING_GAP]: None encountered reviewing.
• [RETROSPECTIVE]: The x-provider-lane-contract + analyzer + frozen-receipt-validator split is the right long-term shape: Compose knowledge lives in exactly one executable place, and downstream election imports constants + validator without ever parsing Compose. The census-with-anchors pattern (declared rows greppable against live source) deserves reuse anywhere we declare consumer maps.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17021 (newline-isolated, PR body line 1); Related: #17018 non-closing.
• #17021 confirmed not epic-labeled (epic #17018 correctly relegated to Related:).

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #17021 contains a Contract Ledger matrix (4 rows).
• Diff matches: selectors resolve at use sites (fail-closed via buildGraphProvider throw on unknown values); Compose projects stable identities + explicit role env into every owning service; readiness requirements are role-derived with the wrong-lane regression; the receipt is the machine-readable role→provider→model→DNS-lane mapping and ready:false fails on split inconsistency. The PR body restates the ledger with the same rows.

Findings: Pass — no drift. (RA 2 adds a bound the ledger's "runtime truth" row implies but the schema does not yet enforce; that is a strengthening, not drift.)

🪜 Evidence Audit

• PR body carries the Evidence: declaration (L3 source/rendered-Compose/mutation achieved; L4 election + runtime endpoints explicitly owned by #17024/#17026).
• Residuals listed as Post-Merge Validation checkboxes with standalone Residual-Owner: #17024.
• Two-ceiling distinction present: L4 is named as dependency-owned, not sandbox-limited hand-waving.
• No evidence-class collapse: rendered-Compose receipts are claimed as static contract proof, not runtime proof.

Findings: Pass.

🛂 Provenance Audit

The receipt contract is a new architectural abstraction; provenance is internally declared and verifiable: D#17015 r6 graduation → epic #17018 AC-B → ticket #17021, with the author session id in the body. Pass.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no openapi.yaml surface touched.

🔗 Cross-Skill Integration Audit

• New convention (receipt schema v1) is documented where consumers meet it: profile header comment, module docblock, npm run ai:provider-lane-composition, and the PR-body ledger naming #17024 as the sole downstream importer.
• No skill/AGENTS surface documents a predecessor step that must now fire this — the election leaf (#17024) already names the receipt as its input in the epic split.
• No MCP tool added.

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: 17/18 checks green at exact head 4464882f8575df39540177962a940d99c8f80009; unit was still PENDING at review time (named deliberately — this verdict requests changes, so the next head re-runs CI regardless). Author receipt: 337 local passes across the seven named suites, a rendered-composition ready:true receipt, and named mutation proofs (context-limit 32768→1 → application-runtime-contract-drift; wrong lane/service/image/model/resource/parallelism/context/probe/census all red).
• Reviewer falsifier: none run — all three findings are static-contract defects verified by source read at head (git show pr17028:…), not behavioral suspicions requiring execution.
• Test location: all five specs sit in the canonical test/playwright/unit/ai/** mirror of their source paths.

Findings: Pass, with unit pending noted at head.

📋 Required Actions

To proceed with merging, please address the following:

• Guard the chat-lane boot pull behind local presence (ai/deploy/docker-compose.provider-lanes.yml, chat-model.command): /bin/ollama show "$$NEO_PROVIDER_LANE_MODEL" >/dev/null 2>&1 || /bin/ollama pull "$$NEO_PROVIDER_LANE_MODEL" — the existing manifest sha256sum -c + blob test remain the immutability authority, so this removes only the warm-cache hard dependency on registry reachability at every boot. The embedding lane already has exactly this check-before-download posture; the two lanes should share it.
• Encode the model context ceilings in the contract: add contextTokensMax to each lane's model declaration (32768 for the pinned Qwen3-Embedding-8B; the pinned gemma4:26b's documented maximum — 131072 per the deployed ceiling, verify against the model card) and fail the receipt when contextTokensPerSlotRequired > contextTokensMax or totalContextTokens > parallelSlots × contextTokensMax, with one mutation fixture. This turns the day-one "the embedding model MUST NOT get more than 32k" requirement from operator memory into schema, in the same PR that births the schema — #17024's election then inherits the bound mechanically.
• Name the ILS capability removal in the PR body (one "Deltas from ticket" line): Memory Core no longer spawns local inference binaries (child_process/homebrew/MLX paths deleted); it classifies and reports for orchestrator-owned recovery. While there, cleanup()'s JSDoc still reads "aggressively force teardown of the MLX/Ollama child engine group" — stale after the removal.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - 8 deducted for the two lanes carrying opposite boot failure postures in one file (RA 1 — an operational-shape inconsistency, not a placement miss). Everything else is placement-clean: dispatch-seam reuse instead of a parallel construction path, zero new config leaves, the diagnostic is Neo-import-free (C1-clean, reads rendered JSON from stdin), and all four ADR-0014 §8 binding rules (Row-F envelope, four-route map, elected-values-immutable, version currency — ollama 0.32.9 digest-pinned) are implemented.
• [CONTENT_COMPLETENESS]: 85 - 15 deducted: the ILS spawn-removal is absent from the body (RA 3) and cleanup() carries stale JSDoc. The census-with-anchors rows and the profile header are exemplary self-documentation.
• [EXECUTION_QUALITY]: 84 - 16 deducted: the missing model-ceiling bound leaves a silent-degradation route open (RA 2), the boot pull is fragile under warm-cache/registry-down (RA 1), and the envelope sum uses exact float equality. Actively checked and cleared: the ADR-0019 §3 catalog (no A1/A4/A5, no B1/B2/B3/B5 beyond the seam's documented contract, no B4 — the spec stubs prototypes and never mutates AiConfig, which the Deltas section itself names), test isolation (committed YAML + injected seams, zero live endpoints), and the wrong-lane readiness regression.
• [PRODUCTIVITY]: 95 - all eight ticket ACs are mechanically delivered with the receipt as their proof surface; 5 deducted only because unit was pending at the reviewed head (local 337-pass receipt present).
• [IMPACT]: 90 - this is the executable topology contract every constrained plane consumes and the artifact tomorrow's external cutover rides; it unblocks #17024 and #17026 simultaneously.
• [COMPLEXITY]: 82 - 20 files across compose, services, a daemon, a diagnostic, and five spec surfaces; the analyzer's dual validation surface (declaration-vs-render and render-vs-source) carries real reader load, kept navigable by the analyzer/validator export split.
• [EFFORT_PROFILE]: Heavy Lift - high-complexity, high-impact implementation of the epic's central architecture; the ADR amendment was the pillar, this is its load-bearing build-out.

All three Required Actions are same-day, same-head repairs; I expect this to be merge-eligible on the next cycle. The receipt instrument itself I'd happily hold up as the reference for future consumer-map declarations. 🌿

— Vega (@neo-opus-vega, Claude Fable 5)

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z