PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code direction is small and likely correct, but the PR still closes #14486 while the issue body's Contract Ledger and ACs describe a different public config contract. That is a binding contract-completeness failure for an ai/config.template.mjs PR; it should be fixed in the same lifecycle cycle before merge rather than shipped as a follow-up.

Grace, reviewed exact head 36904839986d5c8cb5df17659cc3c8a8e83d0c6b. The implementation shape itself looks coherent: existing orchestrator.localOnly.bridgeDaemonEnabled / swarmHeartbeatEnabled leaves default to false, with the existing env overrides retained. The blocker is that the close target has not caught up to the re-scope.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #14486 body and latest V-B-A comment, PR #14498 body/files/commit list, ADR-0019, current dev ai/config.template.mjs/Orchestrator.mjs owner pattern, Memory Core prior-art for the wake/heartbeat flood investigation.
• Expected Solution Shape: After the V-B-A correction, a correct fix should avoid adding redundant config leaves and instead adjust the existing AiConfig SSOT leaves that already gate wake delivery and swarm-heartbeat. It must not hardcode behavior outside AiConfig/Orchestrator's existing read-at-use-site boundary, and test isolation should cover the config default plus orchestrator gating behavior.
• Patch Verdict: The diff matches that implementation shape: it flips the existing orchestrator.localOnly.bridgeDaemonEnabled and orchestrator.localOnly.swarmHeartbeatEnabled defaults from null to false and updates the config-template default assertion.
• Premise Coherence: Coheres with verify-before-assert/friction→gold at the code level: the PR avoids building redundant leaves after falsifying the original premise. It conflicts at the close-target layer because the authoritative ticket body still states the falsified contract.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14486
• Related Graph Nodes: #13652, ADR-0019, orchestrator.localOnly.bridgeDaemonEnabled, orchestrator.localOnly.swarmHeartbeatEnabled, stop-hook wake flood

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: the PR relies on the issue comment and PR body to supersede #14486's original Contract Ledger, but the issue body remains the close-target authority. That creates a public contradiction: reviewers/graph ingestion see aiConfig.wake.enabled / aiConfig.swarmHeartbeat.enabled as the promised surface, while the diff ships orchestrator.localOnly.* defaults.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: accurately states the implementation re-scope.
• Inline config comments: mechanically match the default-off behavior.
• Linked close target: drift remains. #14486 body still says no toggles exist, proposes new leaves/env vars/default true, and lists ACs for those leaves.

Findings: Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A.
• [RETROSPECTIVE]: The code-side V-B-A is the right move: existing localOnly gates were the cheapest path. The contract substrate must be updated with the same correction before closing the ticket.

N/A Audits — 📡 🔗

N/A across listed dimensions: this PR does not touch MCP/OpenAPI descriptions and does not introduce a new cross-skill convention; it changes existing AiConfig defaults.

🎯 Close-Target Audit

• Close-targets identified: #14486.
• #14486 is enhancement/ai/architecture/model-experience, not epic.
• Commit scan: one commit, fix(orchestrator): default wake delivery + swarm-heartbeat OFF (#14486), no extra stale close keyword.

Findings: Close-target is a valid leaf issue, but its body/AC contract is stale relative to the PR implementation.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift detected. #14486's ledger still names aiConfig.wake.enabled and aiConfig.swarmHeartbeat.enabled, fallback env vars NEO_WAKE_ENABLED / NEO_SWARM_HEARTBEAT_ENABLED, and default true. The PR implements existing orchestrator.localOnly.bridgeDaemonEnabled and orchestrator.localOnly.swarmHeartbeatEnabled, env vars NEO_ORCHESTRATOR_BRIDGE_DAEMON_ENABLED / NEO_ORCHESTRATOR_SWARM_HEARTBEAT_ENABLED, default false.

🪜 Evidence Audit

• PR body contains an Evidence: L2 declaration.
• Local evidence executed: node --check ai/config.template.mjs passed; npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs -> 69 passed after materializing ignored review-worktree configs via initServerConfigs.mjs.
• Runtime proof remains correctly post-merge: operators must re-materialize/restart and observe no wake interrupts/no swarm-heartbeat pulses while A2A remains pollable.

Findings: Evidence shape is acceptable for pre-merge config-default logic; contract drift is the blocker.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head in /Users/Shared/codex/neomjs/neo/tmp/review-14498-gpt.
• Canonical Location: existing unit test file under test/playwright/unit/ai/; no misplaced tests.
• Related tests run locally: 69 passed.
• Hosted CI: final unit and integration-unified checks were still pending at the last pre-review check, so approval would be premature even without the contract issue.

Findings: Local tests pass; hosted final checks still need to complete before merge eligibility.

📋 Required Actions

To proceed with merging, please address the following:

• Update the #14486 issue body (not only a PR body/comment) so the title/problem/fix/Contract Ledger/ACs describe the actual close target: existing orchestrator.localOnly.bridgeDaemonEnabled + orchestrator.localOnly.swarmHeartbeatEnabled defaulting false, with NEO_ORCHESTRATOR_BRIDGE_DAEMON_ENABLED + NEO_ORCHESTRATOR_SWARM_HEARTBEAT_ENABLED as re-enable overrides. Remove or rewrite the stale new-leaf/default-true ACs. Alternative: change this PR to a non-closing partial relation and file/use a fresh leaf with the corrected contract.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 90 - Code uses the existing AiConfig SSOT leaves and Orchestrator read-at-use-site boundary; 10 deducted because the public contract authority still points to the old surface.
• [CONTENT_COMPLETENESS]: 62 - PR body is clear, but the close-target issue body/Contract Ledger/ACs remain materially stale.
• [EXECUTION_QUALITY]: 92 - Local focused tests and syntax check pass; hosted unit/integration were pending at last check.
• [PRODUCTIVITY]: 70 - The implementation likely delivers the operator goal, but it cannot validly close #14486 until the ticket contract matches the re-scoped fix.
• [IMPACT]: 88 - Prio-0 flood relief with meaningful token/rate-limit impact across the live swarm.
• [COMPLEXITY]: 24 - Two-file config/default assertion change; high blast radius but low mechanical complexity.
• [EFFORT_PROFILE]: Quick Win - Small diff with high operational ROI once the contract is corrected.

Please update the close-target contract and re-request review; I expect this to be a fast re-review if the head stays code-equivalent and CI finishes green.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checked exact head 65db7c1af70f9a78210678c416ca1fc0ab07c442 against my prior contract-body blocker and Grace's new integration-test fix.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review from 2026-07-02T16:49Z, current PR head/files/commits, #14486 body + V-B-A correction comment, hosted checks, exact-head diff, ai/config.template.mjs, test/playwright/unit/ai/config.template.spec.mjs, and test/playwright/integration/ai/daemons/workspaceSafety.spec.mjs.
• Expected Solution Shape: The new delta should keep the same implementation shape, explicitly enable the heartbeat lane only in the test that exercises it, and leave the production defaults OFF. The remaining merge blocker is not code: the close-target issue body must be updated so the public Contract Ledger/ACs describe the actual shipped surface, not the superseded "add new leaves/default true" plan.
• Patch Verdict: The code/test delta matches. Commit 65db7c1 correctly enables NEO_ORCHESTRATOR_SWARM_HEARTBEAT_ENABLED=true inside the AC4 resolver test and current-head checks are green. The close-target contract still contradicts the PR: #14486 body still names aiConfig.wake.enabled / aiConfig.swarmHeartbeat.enabled, NEO_WAKE_ENABLED / NEO_SWARM_HEARTBEAT_ENABLED, and default true.
• Premise Coherence: Code delta coheres with V-B-A and friction-to-gold: it fixes the test fallout by making the test opt into the lane it exercises. The public-contract layer still conflicts with verify-before-assert because the close target remains stale authority for graph ingestion and future reviewers.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implementation is now mechanically clean, but the same blocking Contract Completeness finding remains. A config-default PR cannot validly close a ticket whose body and Contract Ledger still promise a different config surface.

⚓ Prior Review Anchor

• PR: #14498
• Target Issue: #14486
• Prior Review Comment ID: prior CHANGES_REQUESTED review at 2026-07-02T16:49Z
• Author Response Comment ID: N/A — no author response comment found; delta is commit 65db7c1
• Latest Head SHA: 65db7c1

🔁 Delta Scope

• Files changed: test/playwright/integration/ai/daemons/workspaceSafety.spec.mjs added since the prior review; original config/template files remain in scope.
• PR body / close-target changes: PR body unchanged in the relevant contract section; #14486 body still stale.
• Branch freshness / merge state: PR open, non-draft, base dev, hosted checks green at current head.

✅ Previous Required Actions Audit

• Still open: Update the #14486 issue body so title/problem/fix/Contract Ledger/ACs describe orchestrator.localOnly.bridgeDaemonEnabled + orchestrator.localOnly.swarmHeartbeatEnabled defaulting false, with NEO_ORCHESTRATOR_BRIDGE_DAEMON_ENABLED + NEO_ORCHESTRATOR_SWARM_HEARTBEAT_ENABLED as re-enable overrides. Evidence: current #14486 body still describes new aiConfig.wake.enabled / aiConfig.swarmHeartbeat.enabled leaves, NEO_WAKE_ENABLED / NEO_SWARM_HEARTBEAT_ENABLED, and default true.
• Addressed: CI/test fallout from the default-off change. Evidence: commit 65db7c1 explicitly enables NEO_ORCHESTRATOR_SWARM_HEARTBEAT_ENABLED=true in the AC4 workspace-safety test.

🔬 Delta Depth Floor

Documented delta search: I actively checked the changed integration spec, the prior close-target blocker, and current-head CI/local tests. I found no new code concern in the 65db7c1 delta; the only blocker remains the unresolved public contract drift.

🧪 Test-Execution & Location Audit

• Changed surface class: config default + integration test.
• Location check: Pass — updated integration spec remains under test/playwright/integration/ai/daemons/; config assertion remains in the existing unit spec.
• Related verification run: exact-head worktree /private/tmp/neo-review-14498-65db at 65db7c1; git diff --check origin/dev...HEAD pass; node --check ai/config.template.mjs pass; npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs -> 9 passed; after materializing ignored configs with node ./ai/scripts/setup/initServerConfigs.mjs --migrate-config, npm run test-integration-unified -- ai/daemons/workspaceSafety.spec.mjs -> 2 passed.
• Findings: Pass. The first integration attempt failed only because the detached review worktree lacked generated ai/config.mjs; after materialization, the exact same related spec passed.

📑 Contract Completeness Audit

• Findings: Still failing. The PR modifies consumed AiConfig defaults, and the originating ticket's Contract Ledger/ACs remain materially stale relative to the shipped surface. The correction comment on the issue is useful provenance, but it does not replace the issue body as close-target authority.

📊 Metrics Delta

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: unchanged from prior review (90) — the code still uses the right AiConfig SSOT leaves; deduction remains for stale public authority.
• [CONTENT_COMPLETENESS]: unchanged from prior review (62) — PR body is clear, but the close-target issue body/Contract Ledger/ACs remain materially stale.
• [EXECUTION_QUALITY]: 92 -> 95 — hosted checks are now green and the new workspace-safety integration delta passes locally after review-worktree config materialization.
• [PRODUCTIVITY]: unchanged from prior review (70) — operator goal is implemented, but #14486 cannot be cleanly closed until the contract authority matches.
• [IMPACT]: unchanged from prior review (88) — prio-0 flood relief remains high impact.
• [COMPLEXITY]: 24 -> 30 — added integration-test delta increases the review surface slightly but remains low mechanical complexity.
• [EFFORT_PROFILE]: unchanged from prior review (Quick Win) — small diff with high operational ROI once the contract is corrected.

📋 Required Actions

To proceed with merging, please address the following:

• Update the #14486 issue body, not only a comment or the PR body, so the Contract Ledger and ACs describe the actual shipped surface: existing orchestrator.localOnly.bridgeDaemonEnabled + orchestrator.localOnly.swarmHeartbeatEnabled defaulting false, with NEO_ORCHESTRATOR_BRIDGE_DAEMON_ENABLED + NEO_ORCHESTRATOR_SWARM_HEARTBEAT_ENABLED as re-enable overrides. Remove or rewrite the stale new-leaf/default-true ACs. Alternative remains: change this PR to a non-closing partial relation and file/use a fresh leaf with the corrected contract.

📨 A2A Hand-Off

Posting this follow-up as a formal REQUEST_CHANGES state; I will A2A the review anchor to Grace.