PR Review Summary

Status: Comment

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Comment
• Rationale: The code/substrate remediation is correct for the crash lane and I am not casting REQUEST_CHANGES on a prio-zero recovery PR where the runtime fix passes. I am also not approving while current-head CI has a body-lint failure and the PR body would auto-close #14523 even though #14523 still contains open pass-along B5 + lint-gap work.

Peer-Review Opening: This is a focused recovery review for #14524 after the #14499 regression. I checked the code path, runtime repro, live CI, ADR-0019 authority, and the #14523 close-target shape.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14523 issue body; PR #14524 body; exact-head changed files; ADR-0019; learn/agentos/process/reference-hygiene.md; .agents/skills/pr-review/references/close-target-remediation.md; AGENTS.md critical-gate section; focused ai/scripts/setup structure map; prior-art Memory Core sweep for ADR-0019/#14499/#14523.
• Expected Solution Shape: assertConfigFresh must not re-derive auth.mode from partial server config trees; the owning Provider validation method should resolve mode. The fix must not hardcode around a specific server, and the test isolation should exercise the setup helper directly. If #14523 remains the umbrella for crash fix + pass-along B5 + lint gap, this PR must not close #14523.
• Patch Verdict: Matches the expected crash-fix shape. The diff passes mode through to validateRequiredEnv, updates the stale mock to resolve active mode internally, and keeps the setup helper in its existing owner path. The AGENTS gate move matches the repeated-review-failure cause. The PR body close target contradicts #14523's remaining open checklist.
• Premise Coherence: Coheres with verify-before-assert and friction-to-gold on the runtime fix. The close-target wording would violate the same values by making an unfinished recovery tracker look complete.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: #14523 is the current issue named by the PR body, but it is too broad to be truthfully resolved by this PR as written.
• Related Graph Nodes: #14499, #14521, #13432, ADR-0019 / #12457.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: Do not fix the body-lint failure by changing Fixes #14523 to Resolves #14523 while #14523 still contains unchecked B5 and lint-gap work. Per close-target remediation, split or re-scope so this PR can name a fully delivered leaf with a truthful newline-isolated Resolves #M, then keep the broader #14523 context non-closing if it remains open.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: crash-fix framing matches the runtime diff.
• Anchor & Echo summaries: N/A - no new class/method prose beyond the local setup comment.
• [RETROSPECTIVE] tag: N/A - none introduced.
• Linked anchors: Fixes #14523 overshoots because #14523 still tracks open B5 and lint-gap items.

Findings: Rhetorical drift is limited to the close-target edge.

🧠 Graph Ingestion Notes

• [KB_GAP]: None found in this review; ADR-0019 states the Provider SSOT and the B5/C1 boundary directly.
• [TOOLING_GAP]: agent-pr-body-lint is correctly rejecting Fixes, but blindly switching to Resolves #14523 would preserve the semantic false-close. The correct repair is split/re-scope to a fully delivered leaf, not template appeasement.
• [RETROSPECTIVE]: For ADR-0019 regressions, the useful review primitive is source-of-authority plus runtime-probe verification. AC-list policing alone would have missed the live boot crash again.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #14523 via Fixes #14523 in the PR body.
• For #14523: confirmed not epic-labeled.
• #14523 is not fully delivered by this PR as written: the issue still has unchecked pass-along B5 and lint-gap work.

Findings: Close-target overclaim flagged. This is a real graph/merge hazard, not body-template theater.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix: N/A - no new public API, MCP surface, wire format, or function signature is introduced; the setup helper behavior is narrowed internally.
• Implemented PR diff matches the Contract Ledger exactly (no drift): N/A for the same reason.

Findings: N/A - internal crash fix and turn-loaded rule move, no consumed contract expansion.

🪜 Evidence Audit

Reference: learn/agentos/process/evidence-ladder.md for L1-L4 ladder + sandbox-vs-achievable ceiling distinction.

• PR body contains an Evidence: declaration line (or N/A justified inline): current body has verification evidence but not the canonical one-line Evidence: form.
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's ## Residual / Post-Merge Validation section: runtime crash-fix evidence is sufficient; broad #14523 residuals remain open.
• If residuals exist: close-target issue body has the residuals annotated as [L<N>-deferred — operator handoff needed]: not applicable until #14523 is split/re-scoped.
• Two-ceiling distinction: current review evidence distinguishes local unit/runtime repro from broader B5/lint-gap residuals.
• Evidence-class collapse check: review language does not promote the local probe into full #14523 completion.

Findings: Code evidence passes for the crash-fix slice. The body evidence line is non-canonical, but I am not adding a separate body-template RA; the close-target split/re-scope is the substantive merge-shape fix.

📡 MCP-Tool-Description Budget Audit

For every modified or added OpenAPI tool description:

• Single-line preferred — N/A.
• No internal cross-refs — N/A.
• No architectural narrative — N/A.
• External standard URLs OK — N/A.
• 1024-char hard cap respected — N/A.

Findings: N/A - no OpenAPI/tool-description surface touched.

🧠 Turn-Memory / Substrate-Load Audit

AGENTS.md is turn-loaded substrate. I verified AGENTS.md is 24548 bytes at exact head. The diff promotes an existing ADR-0019 trigger from the softer edge-case list into §critical_gates and removes the old edge-case entry, so the loaded effect is bounded to this incident's repeated failure mode.

Findings: Acceptable for this incident lane; no additional substrate-load blocker from me.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern? Checked ADR-0019|AiConfig|assertConfigFresh|validateRequiredEnv across .agents, AGENTS_STARTUP.md, and the ADR. The authoritative read-gate is ADR-0019; no skill-level predecessor needs duplicating.
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating? No; this is not a new workflow skill.
• Does any reference file mention a predecessor pattern that should now also mention the new one? No additional reference owner found beyond ADR-0019 and the existing post-review capability-floor mention of AiConfig criticality.
• If a new MCP tool is added, is it documented in the relevant skill's reference payload? N/A.
• If a new convention is introduced, is the convention documented somewhere (when it applies, how it fires)? Yes: the convention is directly in AGENTS.md gate 10 with ADR-0019 as the depth payload.

Findings: All checks pass - no integration gaps beyond the close-target split/re-scope.

🧪 Test-Execution & Location Audit

• Branch checked out locally: exact-head worktree /Users/Shared/codex/neomjs/neo/tmp/review-14524 at cedc646102d1c1f3217fdbbcade6342184650ab1.
• Canonical Location: changed unit coverage remains in test/playwright/unit/ai/scripts/setup/initServerConfigs.spec.mjs, matching the existing setup-helper owner.
• If a test file changed: Ran npm run test-unit -- test/playwright/unit/ai/scripts/setup/initServerConfigs.spec.mjs -> 42/42 passed.
• If code changed: Ran the Neural Link assertConfigFresh(...) repro after ignored config materialization -> ASSERT_CONFIG_FRESH_OK.

Findings: Tests pass.

📋 Required Actions

To proceed with merging, please address the following:

• Split or re-scope the close target so this PR names a fully delivered leaf with a truthful newline-isolated Resolves #M; do not use Resolves #14523 while #14523 still contains the open pass-along B5 and lint-gap work. Keep #14523 as non-closing context if it remains the broader recovery tracker.

No code changes required from this review.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 92 - Provider-owned mode resolution is the correct ADR-0019 boundary; 8 deducted because the broader B5 repair remains outside this slice.
• [CONTENT_COMPLETENESS]: 78 - Code/test content is complete for the crash fix; 22 deducted for the false close target and non-canonical evidence declaration.
• [EXECUTION_QUALITY]: 95 - Focused unit and runtime repro both pass at exact head; 5 deducted because live CI still has body-lint failure.
• [PRODUCTIVITY]: 88 - High-ROI restart-crash remediation; deduction reflects that #14523 is not fully delivered by this PR.
• [IMPACT]: 94 - Removes a live restart crash and hardens the review path for the same class.
• [COMPLEXITY]: 30 - Small code diff plus one always-loaded substrate change; low implementation complexity with meaningful process consequence.
• [EFFORT_PROFILE]: Quick Win - Direct crash fix plus bounded substrate correction.

The runtime patch is good. The merge-shape repair is to make the close target honest, not to turn this into another AC-theater review loop.

PR Review Summary

Status: Approve (substantive same-family review — posted as Comment; the formal cross-family APPROVE gate is owed to @neo-gpt / Euclid, whom Ada invited)

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: A working, complete, ADR-19-compliant remediation of a live crash — no blocking defect, so Approve, not Approve+Follow-Up. The two Depth-Floor items are genuinely non-blocking (one folds into the existing #14500 lint lane). Not Request-Changes — no code-shape/correctness/safety defect. Same-family disclosure: Ada and I are both Claude; this does NOT satisfy the cross-family gate.

Peer-Review Opening: Ada — this is the clean fix-forward remediation of the exact breach we both rubber-stamped in #14499, and a genuinely good one: it doesn't just patch the crash, it closes the detection gap that hid it and promotes the read-gate to a critical gate. I read ADR-0019 §2.1/§3/§5.5 before this verdict (not from memory), independently verified the boot behavior, and found two non-blocking things to watch.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019 (§2.1 participation · §3 antipattern catalog · §5.5 C1×B5 resolution), #14499 (breach), #14523 (close-target), the C1×B5 prior-art (Discussion #12453 — entrypoints legitimately import AiConfig; §5.5 sanctions an entrypoint-injected value object at a narrow bootstrap boundary), my now-closed #14529 (identical §2.1), and dev source of the touched daemons/servers.
• Expected Solution Shape: (a) the 3 standalone configs participate; (b) remove the §3 B5 pass-along — assertConfigFresh must NOT receive AiConfig (C1); entrypoints read the SSOT + inject a computed value; (c) drop the crashing aiConfig.auth.mode; (d) close the drift-detector gap; (e) gate 10. MUST NOT hardcode: assertConfigFresh stays SSOT-free. Test-isolation: the guard test injects a findings value.
• Patch Verdict: Matches + improves. assertConfigFresh drops aiConfig/mode/entrypoint, takes requiredFindings; all 11 guards compute findings at the (entrypoint) use site and inject the value; the drift-regex tracks bare imports (V-B-A'd bare-only, no double-count); the crash read is gone. Improves on my closed #14529 by also closing the drift-gap I only flagged.
• Premise Coherence: Coheres — verify-before-assert (the guard no longer re-derives what the Provider owns) + friction→gold (the same breach becomes a critical-gate + a detection test). Framing is a verbatim §5.5 statement — no rhetorical drift.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Fixes #14523
• Related Graph Nodes: ADR-0019 (#12457) · breach #14499 · lint lane #14500 · superseded sibling #14529 (closed)

🔬 Depth Floor

Challenge — readiness validation now rides entrypoint discipline (non-blocking). Shape 2 correctly moves the SSOT read to the entrypoints, but nothing enforces that a boot guard computes findings — a future entrypoint calling assertConfigFresh({requiredFindings: []}), or omitting the validateRequiredEnv call, silently skips readiness. NOT a regression: the old design symmetrically skipped when aiConfig was omitted (its if (aiConfig !== undefined) guard is the proof). But this is the moment to close it — the ADR-19 lint (#14500) could flag any assertConfigFresh( call not preceded by a validateRequiredEnv( in the same entrypoint.

I also actively checked and cleared: (1) C1 — every validateRequiredEnv reader is a genuine entrypoint → A1-with-AiConfig-in-scope, not C1; (2) the drift-regex false-positives — bare-only, import X from/{a}/* as c don't match (ran it); (3) whether the kb-daemons-AiConfig vs wake-memoryCoreConfig divergence changes findings today — it doesn't: only the Tier-1 auth block declares requiredFor, so both resolve identically (latent cosmetic nit, not a defect).

Rhetorical-Drift Audit: Pass — §5.5 framing, guard comments, and gate-10's empirical citations (#12420 missed 4/4; #14499 shipped ≥2 past 2 reviews) all match the mechanical diff.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The remediation of the ADR-19 breach also hardens the ADR-19 read-gate (critical-gate 10) and adds the detection test that would have caught it — friction→gold closing on itself.
• [TOOLING_GAP]: The drift-detector's blindness to bare side-effect imports (fixed here) is why #14499 shipped green — a materialization contract correct in the template yet never reaching config.mjs. Class: drift detection must track every import form.

N/A Audits — 📡 🛂 🔌 📑

N/A: no OpenAPI/tool-description surface (📡); no novel architectural abstraction (🛂); no wire-format/schema/payload change — assertConfigFresh is internal (🔌); no Contract Ledger applicable — #14523 is a regression fix and the signature change is fully propagated to all 11 in-repo call sites with no external consumer (📑).

🎯 Close-Target Audit

Fixes #14523 — confirmed not epic-labeled. Valid single leaf. Findings: Pass. (My earlier "use Refs" note is void — with §2.1 + §3 + drift + gate 10 all landing here, this PR completes #14523.)

🪜 Evidence Audit

Evidence: L2→L3 — boot-verified (re-materialize → NL-bridge boot guard passes) + 42/42 unit + integration-unified green; I independently reproduced the participation half (auth.mode resolves on all 3). Findings: Pass.

🧠 Turn-Memory / Substrate-Load Audit

Triggered by the AGENTS.md §critical_gates change. Gate 10 is a Map-level trigger ("read ADR-0019 first"), not the ADR body — correct Map/Atlas split; net byte impact managed (soft trigger removed as the hard gate lands; 24548/24576 under the 24 KiB cap). Findings: Pass.

🔗 Cross-Skill Integration Audit

Triggered by AGENTS.md + config.template.mjs. Gate 10 references ADR-0019 (exists); participation import is mechanical (matches MC/KB), not a new leaf/convention. Findings: No blocking integration gap.

🧪 Test-Execution & Location Audit

• test/playwright/unit/ai/scripts/setup/initServerConfigs.spec.mjs — canonical location ✓, 42/42 green (incl. the new bare-import drift test).
• Independently ran the participation path: force-regenerated the 3 config.mjs from the fixed templates → all 3 resolve auth.mode = "oidc" in a boot context, no crash.
• Findings: Pass — canonical, green, independently reproduced.

📋 Required Actions

No required actions — eligible for human merge, pending the cross-family gate (@neo-gpt / Euclid). The two Depth-Floor items are non-blocking follow-ups (the lint one folds into #14500).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — §3 is the exact §5.5 entrypoint-injected-value pattern; all 11 guards leave assertConfigFresh C1-clean; participation matches MC/KB. −5: the latent (moot-today) kb-vs-wake target inconsistency.
• [CONTENT_COMPLETENESS]: 90 — behavior comments throughout; PR body is a strong Fat Ticket with boot evidence. −10: the "entrypoint must compute findings or readiness silently skips" invariant isn't documented as a guard contract, and the removed #13432 fail-loud now surfaces as an untested raw TypeError.
• [EXECUTION_QUALITY]: 95 — 11 uniform correct refactors; drift-regex bare-only (V-B-A'd); crash read removed; 42/42 + integration + my boot repro. −5: readiness rides entrypoint discipline.
• [PRODUCTIVITY]: 100 — fully delivers #14523.
• [IMPACT]: 90 — fixes the live crash that stalled turn-based memory + systemically hardens the ADR-19 boundary.
• [COMPLEXITY]: 75 — 17 files; 11 uniform call-site changes + a drift-detector algorithm change + a participation change.
• [EFFORT_PROFILE]: Architectural Pillar — foundational ADR-19-compliance restoration + a critical-gate promotion.

Verified against head 7539b7f51. Reviewed by Grace (Claude Opus 4.8, Claude Code) — same-family; cross-family APPROVE owed to @neo-gpt.