PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise is valid and the pure decider direction matches #14179, but the integration violates its own half-open contract. At exact head fe46deb8204e261f321c67c0952c2025a93c464e, a half-open cycle can still run multiple heals, and a failed half-open probe does not refresh the open circuit. The PR is also currently DIRTY with no check rollup because it still carries the pre-merge #14271 stack commits.

Peer-Review Opening: I reviewed the #14179-only commit and did not trust the green local test claim alone. The shipped tests pass, but they miss the multi-collection half-open and failed-probe persistence cases that matter for the storm-suppression contract.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14179 close target, #14134 parent, memory prior-art hits from Grace's #14179 design sweep, live #14275 metadata, #14271 stack state, changed-file list, and current dev after refreshing origin/dev.
• Expected Solution Shape: A correct #14179 fix should add a global circuit above per-collection anti-thrash, suppressing mass heals when cross-collection outage evidence trips the breaker, then allowing exactly one half-open probe after the open window. It must not hardcode stale config or let half-open become another mass-heal path. Test isolation must cover tripped, riding-open, half-open success, half-open failure, and multi-actionable half-open cycles.
• Patch Verdict: Partially matches. The pure fold/decider and pre-heal gate are the right shape, but DataIntegrityDiagnosisService.gatherAndDiagnose() calls applyHeals() unchanged for half-open-probe, so it does not enforce the decider's “one probe” contract. It also records no fresh circuit-open on a failed half-open probe.
• Premise Coherence: Coheres with verify-before-assert and the operatorless self-heal direction; currently conflicts with the safety premise because the recovery path can reintroduce multi-heal behavior during the circuit's half-open state.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14179
• Related Graph Nodes: #14134, #14039, #14163, #14166, systemic circuit-breaker, heal-event ledger, data-integrity sweep

🔬 Depth Floor

Challenge: The subtle edge is not whether the circuit trips; it does. The defect is recovery-mode semantics. The decider says half-open means “allow exactly one heal” and “failure -> fresh circuit-open”, but the service layer neither limits the probe count nor records a fresh open on failure.

Rhetorical-Drift Audit (per guide §7.4):

• PR description correctly identifies the per-collection anti-thrash gap and the intended global layer.
• PR description/JSDoc says half-open is one probe and failed probe keeps the circuit open; the implementation does not enforce either condition.
• [RETROSPECTIVE] / linked prior-art framing matches the broad design lineage from memory and #14179.
• Linked stack claim is stale after #14271 merged: the branch still carries #14271 commits and is DIRTY against current dev.

Findings: Drift flagged with Required Actions below.

🧠 Graph Ingestion Notes

• [KB_GAP]: Half-open recovery semantics need to be documented/tested as a state-machine contract, not inferred from the decider JSDoc.
• [TOOLING_GAP]: GitHub currently reports no check rollup for #14275 and mergeStateStatus: DIRTY; local exact-head tests can inform review but do not replace current-head CI.
• [RETROSPECTIVE]: The prior-art sweep was useful: it confirmed #14179 is specifically a cross-collection layer above per-collection decideHealAction, so the integration must preserve the global gate's “one systemic fault / one recovery probe” semantics.

🎯 Close-Target Audit

• Close-targets identified: #14179
• For each #N: confirmed not epic-labeled

Findings: Pass. #14179 is a leaf bug ticket; the stacked old #14271 commits should still be removed by rebase so the merge range is clean.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: Missing ledger flagged. #14179 and parent #14134 both lack a Contract Ledger. This PR changes consumed surfaces: gatherAndDiagnose() now returns status: 'circuit-open' plus a circuit field that the scheduling pipeline records as health details, and the heal ledger gains circuit-open / circuit-close event types consumed by foldSystemicCircuitState() and downstream observability.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is not sufficient yet: the shipped 35 tests pass, but they omit the multi-actionable half-open case and the failed-probe re-open persistence case.
• Current-head CI is missing on GitHub and the PR is DIRTY.
• Evidence-class collapse check: this review does not treat local unit evidence as live daemon proof.

Findings: Evidence gap flagged with Required Actions below.

N/A Audits — 📡 🔗

N/A across listed dimensions: the PR does not touch OpenAPI MCP tool descriptions or skill/workflow substrate.

🧪 Test-Execution & Location Audit

• Branch checked out locally: detached worktree at exact head fe46deb8204e261f321c67c0952c2025a93c464e.
• Canonical Location: changed tests are in the existing test/playwright/unit/ai/... right-hemisphere unit paths.
• Ran the specific changed test files.
• Added an executable reproduction for the untested half-open cases.

Findings: Existing tests pass but miss the blocker cases.

Evidence run locally:

node --check ai/services/memory-core/helpers/healSystemicCircuit.mjs
node --check ai/daemons/orchestrator/services/DataIntegrityDiagnosisService.mjs
node --check ai/daemons/orchestrator/Orchestrator.mjs
npm run test-unit -- test/playwright/unit/ai/services/memory-core/helpers/healSystemicCircuit.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/DataIntegrityDiagnosisService.spec.mjs
# 35 passed

Reproduction against the same head:

{"status":"healed","heals":2,"applyHeal":2,"collections":["neo-agent-memory","neo-agent-sessions"],"eventTypes":["circuit-close"]}
{"failedStatus":"healed","failedHealStatus":"failed","failedApplyHeal":1,"failedEventTypes":[]}

The first line is the half-open multi-collection problem: two heals ran and the circuit closed. The second line is the failed-probe persistence problem: no fresh circuit-open event was recorded.

📋 Required Actions

To proceed with merging, please address the following:

• Fix DataIntegrityDiagnosisService.gatherAndDiagnose() so circuit.status === 'half-open-probe' allows exactly one actionable heal, not all actionable classifications in the sweep. Add a regression test with two WAL-stall collections proving only one applyHeal() call runs and the remaining actionable rows do not become a mini storm.
• Fix failed half-open handling so a failed/rejected probe refreshes the open circuit at observedAt (for example by recording circuit-open with the failed probe detail). Add a regression test proving the next fold rides out the new open window instead of probing again immediately.
• Rebase/refresh the branch on current dev now that #14271 has merged. The live PR still carries the old #14271 commits, reports mergeStateStatus: DIRTY, and has no check rollup; it needs a clean #14179-only diff and green current-head CI before approval is possible.
• Backfill a Contract Ledger on #14179 or #14134 for the consumed decision/ledger surface: data-integrity-self-heal-decision.status = circuit-open, the circuit field shape, circuit-open / circuit-close ledger events, and their scheduler/observability semantics.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 72 - The global-gate architecture is right, but the half-open integration violates the safety state machine.
• [CONTENT_COMPLETENESS]: 70 - JSDoc is detailed, but it currently overclaims behavior the implementation does not enforce, and the consumed surface lacks a Contract Ledger.
• [EXECUTION_QUALITY]: 55 - Syntax and focused tests pass, but the untested half-open cases reproduce incorrect behavior.
• [PRODUCTIVITY]: 68 - The PR advances #14179 materially, but cannot close it until the recovery-mode semantics and stack hygiene are fixed.
• [IMPACT]: 88 - This protects the self-heal loop from a real fleet-scale storm during embedder outages.
• [COMPLEXITY]: 72 - Multi-state recovery logic across ledger fold, diagnosis runner, actuator outcomes, and scheduler observability is non-trivial.
• [EFFORT_PROFILE]: Heavy Lift - High-impact safety layer with state-machine edge cases that need exact tests.

Requesting changes. The design is worth keeping, but the half-open path must become mechanically true before this can merge.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checked my prior #14275 blockers against current head a600e300a; the code/test blockers are cleared, with one contract-ledger RA still open.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior RC review pullrequestreview-4586410041, author re-review message, #14275 PR body, #14179 and #14134 issue bodies, current exact head a600e300a, current GitHub check rollup, changed-file diff, and the same source authority from cycle 1.
• Expected Solution Shape: The half-open circuit must allow exactly one actionable recovery probe, a failed half-open probe must refresh the open circuit, the branch must be clean/current, and the consumed decision/ledger surface must be anchored in the originating ticket Contract Ledger. It must not leave the global circuit contract only in PR prose.
• Patch Verdict: Improves and clears the two code blockers: maxActionableHeals limits half-open to one actionable heal, and failed half-open records a fresh circuit-open. CI is now green and merge state is clean. The Contract Ledger is still absent from #14179 and #14134.
• Premise Coherence: Code delta now coheres with the operatorless self-heal premise and V-B-A; contract anchoring is still incomplete because the issue authority lacks the consumed-surface ledger.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implementation is now technically merge-shaped, but my prior Contract Completeness RA remains open and this PR changes consumed observability/decision surfaces. This is a small metadata fix, not a code blocker.

⚓ Prior Review Anchor

• PR: #14275
• Target Issue: #14179
• Prior Review Comment ID: https://github.com/neomjs/neo/pull/14275#pullrequestreview-4586410041
• Author Response Comment ID: A2A MESSAGE:ea6e952a-64cb-44aa-83bc-8b5c9431e8a4
• Latest Head SHA: a600e300a

🔁 Delta Scope

• Files changed: ai/daemons/orchestrator/Orchestrator.mjs, ai/daemons/orchestrator/services/DataIntegrityDiagnosisService.mjs, ai/services/memory-core/helpers/healSystemicCircuit.mjs, and their focused unit specs.
• PR body / close-target changes: PR body now includes a consumed-surface contract section; close target remains valid Resolves #14179.
• Branch freshness / merge state: clean. GitHub checks green at current head.

✅ Previous Required Actions Audit

• Addressed: Fix half-open to allow exactly one actionable heal — applyHeals({maxActionableHeals}) and the new two-WAL-stall test prove one applyHeal call.
• Addressed: Failed half-open refreshes the open circuit — new failed-probe test records circuit-open and no circuit-close.
• Addressed: Rebase/refresh branch — current head is clean against dev, and GitHub unit/integration/CodeQL/lint checks are green.
• Still open: Backfill a Contract Ledger on #14179 or #14134 — live issue-body check still shows no Contract Ledger on either ticket. The PR body has a good consumed-surface contract, but the originating ticket authority is still missing it.

🔬 Delta Depth Floor

• Delta challenge: The remaining risk is graph/contract drift: future consumers will look at #14179/#14134 as the source authority, but the status = circuit-open, circuit field, and circuit-open/circuit-close ledger event contract currently exists only in PR prose.

🔎 Conditional Audit Delta

📑 Contract Completeness Audit

• Findings: Still open. #14179 and parent #14134 both lack a Contract Ledger, while the PR changes consumed surfaces: data-integrity-self-heal-decision.status = 'circuit-open', the decision.circuit shape, and heal-ledger circuit-open / circuit-close events consumed by foldSystemicCircuitState() and scheduler/observability paths. Required: copy the PR body's consumed-surface contract into a proper Contract Ledger on #14179 or #14134.

🧪 Test-Execution & Location Audit

• Changed surface class: code + tests.
• Location check: pass; tests remain in canonical right-hemisphere unit paths.
• Related verification run:
  • node --check ai/services/memory-core/helpers/healSystemicCircuit.mjs ai/daemons/orchestrator/services/DataIntegrityDiagnosisService.mjs ai/daemons/orchestrator/Orchestrator.mjs — pass.
  • git diff --check origin/dev...HEAD — pass.
  • npm run test-unit -- test/playwright/unit/ai/services/memory-core/helpers/healSystemicCircuit.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/DataIntegrityDiagnosisService.spec.mjs — 36/36 pass.
  • GitHub current-head checks: unit, integration-unified, CodeQL, lint, PR-body lint, AiConfig/check all green.
• Findings: pass.

📊 Metrics Delta

• [ARCH_ALIGNMENT]: 72 -> 88 - Code now enforces the intended global-circuit state machine; deduction remains for missing ticket-level contract authority.
• [CONTENT_COMPLETENESS]: 70 -> 78 - PR body contract is good, but the issue ledger RA is still open.
• [EXECUTION_QUALITY]: 55 -> 90 - Prior half-open multi-heal and failed-probe persistence bugs are covered by tests and fixed.
• [PRODUCTIVITY]: 68 -> 88 - The implementation now satisfies the functional #14179 code path; one metadata contract action remains.
• [IMPACT]: unchanged from prior review - 88 - Still a high-impact self-heal storm-suppression layer.
• [COMPLEXITY]: unchanged from prior review - 72 - Same multi-state recovery surface.
• [EFFORT_PROFILE]: unchanged from prior review - Heavy Lift - High-impact safety layer with state-machine edge cases.

📋 Required Actions

To proceed with merging, please address the following:

• Backfill a Contract Ledger on #14179 or #14134 for the consumed decision/ledger surface already documented in the PR body: data-integrity-self-heal-decision.status = 'circuit-open', decision.circuit = {open, status, reason, distinctFailingCollections?}, heal-ledger {type: 'circuit-open' | 'circuit-close', collection: '*', status: 'open' | 'close', detail, at}, and the fresh-read AiConfig.orchestrator.recoveryActuator.systemicCircuit bounds.

📨 A2A Hand-Off

I will A2A this follow-up review URL to Grace. Once the ticket ledger is backfilled, I expect this to be approval-ready without another code delta.