PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The exact-head patch now delivers the narrow ADR-0019 cleanup without preserving either forbidden capture form. Hosted lint rejected the first config-shaped exports, and an independent Provider-refresh falsifier exposed the later singleton-time dbPath capture before a formal review was posted. Both were repaired in this PR. The final head has direct use-site reads, a bootstrap-free task-definition boundary, explicit override seams, focused regressions, and no remaining behavior, architecture, correctness, or safety defect.

Peer-Review Opening: This converged into the right bounded cleanup: the root Provider owns the two leaves, each runtime owner resolves them where used, and the non-entrypoint task table stays pure. The most valuable part is that both superficially plausible compatibility shapes were falsified and removed before the terminal review.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Leaf issue #14957 and its Contract Ledger; parent epic #12456; the seven-file initial change list; current dev versions of the config template, orchestrator entrypoint/class, three service consumers, and taskDefinitions.mjs; sibling direct-read precedents; ADR 0019 §§2–6; ConfigProvider.mjs bounded env-resolution semantics; Base.mjs reactive hook behavior; and prior-art sessions 3ecb40bf-bfef-40b1-8693-a8aae5afa1b7 (ADR origin) plus 966c46fb-ad36-4e4c-88d6-899c4d18ed91 (config-alias/B2 trap).
• Expected Solution Shape: Declare the unchanged defaults and env bindings once as root AiConfig leaves; read them directly at each post-bootstrap owner; keep taskDefinitions.mjs free of Neo/AiConfig imports and config-shaped exports; and preserve the stale-overlay guard before any path dereference. The boundary must not hardcode a second fallback or pass values through a generic options bag, and tests must isolate through explicit instance/options seams or Provider env overrides rather than mutating shared config leaves.
• Patch Verdict: Matches and improves the expected shape at cb66d824147d54f86d8236e81cd5a330ebaf8d27. DEFAULT_DB_PATH / DEFAULT_DATA_DIR are gone; taskDefinitions.mjs imports without the Neo bootstrap; the entrypoint reads lazily after its freshness gate; services resolve dataDir at their own persistence use sites; and dbPath_ / beforeGetDbPath prevents the singleton constructor from freezing the Provider before start().
• Premise Coherence: Coheres with verify-before-assert and friction→gold. CI converted the first false compatibility premise into a direct-use-site migration, and the independent singleton-refresh probe converted the second subtle capture into a regression-backed class-system hook instead of leaving reviewer diligence as the only defense.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14957
• Related Graph Nodes: #12456 · #14500 · ADR 0019 · AiConfig reactive Provider SSOT · orchestrator boot/freshness boundary

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Documented search: I actively looked for remaining DEFAULT_DB_PATH / DEFAULT_DATA_DIR exports or imports, raw NEO_AI_DB_PATH / NEO_AI_ORCHESTRATOR_DIR reads outside leaf declarations, module/static/singleton-time Provider captures, defensive config fallbacks, pass-along values, stale-overlay dereferences before assertConfigFresh, bootstrap-free import regressions, Provider refresh loss, and explicit-override loss. No concern remains at cb66d82414.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: current direct-use-site, lazy boot, and reactive-default claims match the exact diff
• Anchor & Echo summaries: config ownership and singleton-time semantics use precise class-system terminology
• [RETROSPECTIVE] tag: N/A — none is used in the PR body
• Linked anchors: #14957 carries the exact ledger; #12456 is correctly related rather than closed

Findings: Pass. The earlier author-response claim that a singleton class-field initializer was never module-time was empirically superseded by cb66d82414; the current body and code now state and enforce the corrected model.

🧠 Graph Ingestion Notes

• [KB_GAP]: No remaining code gap. The review surfaced a useful distinction: replacing process.env with an AiConfig read is insufficient when the result is exported or captured while a singleton is auto-constructed.
• [TOOLING_GAP]: None. The Config Template SSOT lint caught the exported/module-scope form, while the isolated setEnvOverride probe caught the singleton class-field form that the lexical lint cannot see.
• [RETROSPECTIVE]: “Read at the use site” is temporal as well as lexical. In a Neo.setupClass() singleton, a class-field initializer runs during module import; a null reactive slot plus a substantive beforeGet* hook preserves the Provider boundary until an explicit runtime value is chosen.

🎯 Close-Target Audit

• Close-target identified: #14957 in the PR body
• #14957 is an open enhancement / ai leaf, not epic-labeled
• #12456 remains a non-closing Related: edge; no commit body carries a stale magic-close keyword

Findings: Pass. The PR closes one delivered leaf and preserves the parent epic.

📑 Contract Completeness Audit

• #14957 contains a Contract Ledger covering both leaves, all consumers, and the PID/log boot path
• The exact diff matches the ledger: unchanged defaults/env names, direct resolved reads, no hidden fallback, lazy freshness-safe entrypoint paths, and explicit override seams

Findings: Pass. No public or consumed config contract drifts from the leaf ticket.

🪜 Evidence Audit

• PR body declares L3 achieved → L3 required
• A real daemon.mjs child boots with both env overrides and reaches [Orchestrator] Started
• Static, Provider-refresh, bootstrap-free import, unit, and integration evidence cover the close-target ACs
• Canonical deployment restart remains clearly labeled post-merge operational validation, not promoted to completed evidence

Findings: Pass. The leaf has no deferred code AC; the post-merge checklist validates the production deployment rather than masking an unmet close target.

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP OpenAPI description changes and no new cross-skill convention are introduced; this PR applies the already-governing ADR-0019 contract.

🧪 Test-Execution & Location Audit

• Dedicated worktree checked out and re-verified at exact head cb66d824147d54f86d8236e81cd5a330ebaf8d27
• Modified regressions remain in the canonical test/playwright/unit/ai/daemons/orchestrator/ tree
• git diff --check, node --check, agent-preflight --no-fix, the bootstrap-free dynamic import, and ai:lint-config-template-ssot pass
• Orchestrator.spec.mjs: 61/61 passed; final Orchestrator + invariants rerun: 87/87 passed
• Broader daemon/service pass exercised all five affected consumers; its one first-repair invariant failure exposed the missing beforeGet recognition and was corrected before the final 87/87 rerun
• config.template.spec.mjs: 12/12 passed
• Real workspace-safety integration: 3/3 passed
• Structure map confirms all changes stay within the existing root-config and ai/daemons/orchestrator owners
• Exact-head hosted CI is 11/11 green, including unit, integration-unified, both AiConfig lints, CodeQL, JSDoc, archaeology, retired-primitives, and PR-body gates

Findings: Pass. Local, structural, runtime, and exact-head hosted evidence are green.

📋 Required Actions

No required actions — eligible for human merge.

📊 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]: 100 - Root leaves own defaults/env resolution; every consumer is in its existing owner; the non-entrypoint helper stays bootstrap-free; and B1/B2/B5/C1 capture and pass-along failures were actively searched and cleared.
• [CONTENT_COMPLETENESS]: 100 - The current PR body reflects all four commits, the Contract Ledger maps every consumed surface, and every new runtime hook carries precise Anchor & Echo JSDoc.
• [EXECUTION_QUALITY]: 98 - Focused unit, config, bootstrap, lint, and real-child integration probes are green; two points remain descriptive of the canonical deployment restart that only the human post-merge gate can observe.
• [PRODUCTIVITY]: 100 - All #14957 ACs are delivered: ownership leaves, no raw re-derivation, no config exports/captures, direct consumers, freshness-safe boot, and exact regressions.
• [IMPACT]: 76 - Removes a recurring Agent OS configuration split-brain class from the orchestrator's database and persistence paths without changing operator-facing values.
• [COMPLEXITY]: 48 - Nine final touched files cross root config, daemon boot, singleton class hooks, three service consumers, and focused tests; each individual change is small, but the bootstrap/reactivity interaction requires moderate reasoning.
• [EFFORT_PROFILE]: Maintenance - A bounded ADR cleanup with high correctness leverage; the work is evolutionary hardening rather than a new capability or architectural pillar.

The exact-head code is approved. Human-only merge execution remains the final gate.