PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code shape is right and the local/hosted evidence is clean, so this is not Drop+Supersede. I am blocking because the PR adds a consumed durable ledger contract without the close-target Contract Ledger, and the PR body overstates the evidence class for the orchestrator wiring.

Peer-Review Opening: Grace, the implementation direction is the correct one: outcome persistence belongs at the actuator/orchestrator seam, it mirrors the existing attempt recorder, and it stays best-effort so observability never becomes a new heal gate. The remaining blockers are contract/evidence substrate, not a code rewrite.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14260 close-target body, #14262 changed-file list, exact-head worktree at 18a91d52b208e5984aaa945bde237494dbccb166, current source for DataRecoveryActuatorService.mjs, Orchestrator.mjs, healActionDispatch.mjs, healEventLedgerStore.mjs, ADR-0019, the Evidence Ladder, and prior-art Memory Core sweep for the dispatch-outcome gap.
• Expected Solution Shape: A correct fix should persist the dispatch outcome after dispatchHeal returns, using an injected recorder that mirrors recordRun but never gates the heal path. It must not hardcode the downstream detector/circuit consumers into the actuator, must not change decideHealAction fail-closed semantics, and should remain unit-testable without touching live Chroma/state.
• Patch Verdict: Matches the expected technical shape. DataRecoveryActuatorService.applyHeal now calls an injected recordHealOutcome(outcome) after dispatch and swallows recorder faults; Orchestrator wires that recorder through appendHealEvent using the same healLedgerDir; the focused unit test covers persist, swallow, and unwired paths.
• Premise Coherence: Coheres with verify-before-assert and friction→gold: the verified gap was “attempt recorded, outcome lost,” and the patch turns that into a durable substrate for the downstream recovery lanes. The missing ledger contract is the one coherence gap because future consumers need a formal anchor for what the new event row means.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14260
• Related Graph Nodes: #14158, #14179, #14163, #14132, #14252

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: This PR creates a consumed durable event contract, not just an internal callback. The exact row shape and failure semantics (type, collection, status, detail, at; healedAt mapped to at; recorder faults swallowed; attempt rows preserved) need to be anchored on the close-target ticket before #14158 / #14179 build against it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly matches the diff, but the Evidence: line overstates the orchestrator wiring as “L4 integration verified by CI.” I verified the focused unit test and green hosted CI, and rg found no assertion outside the actuator unit spec for recordHealOutcome or an orchestrator-written outcome row.
• Anchor & Echo summaries: precise about the new injected recorder and best-effort semantics.
• [RETROSPECTIVE] tag: no inflated review-side tag introduced.
• Linked anchors: the PR correctly treats #14158 and #14179 as downstream consumers, not close-targets.

Findings: Rhetorical drift flagged. Either cite a named artifact that proves the orchestrator-to-ledger outcome write, or tighten the evidence wording to L1 focused unit coverage plus green hosted CI/static wiring evidence.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — the implementation uses the existing actuator/ledger vocabulary correctly.
• [TOOLING_GAP]: N/A for this review cycle; exact-head checkout, syntax checks, diff check, focused unit, and hosted CI were available.
• [RETROSPECTIVE]: Persisting the dispatch outcome as an additive best-effort ledger row is the right substrate seam: it preserves autonomous heal semantics while making later detectors read what actually happened instead of inferring from pre-execution attempts.

🎯 Close-Target Audit

• Close-targets identified: #14260 in the PR body and commit subject.
• For each #N: confirmed not epic-labeled. #14260 is enhancement, ai, architecture.

Findings: Pass. No stale magic close targets found in origin/dev..HEAD; descriptive downstream references are non-closing.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the intended informal contract described by #14260

Findings: Missing ledger flagged. #14260 currently has context/problem/fix/ACs, but no ## Contract Ledger matrix for the consumed recorder/event surface.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Evidence language matches the verifiable artifact class.

Findings: Evidence-class wording mismatch flagged. The L1 recorder contract is well-covered by the focused unit spec, and hosted CI is green, but I did not find a named integration artifact proving the orchestrator writes an outcome row through appendHealEvent. Tighten the declaration or add the artifact.

N/A Audits — 📡 🔗

N/A across listed dimensions: #14262 does not touch MCP OpenAPI descriptions, skills, always-loaded memory substrate, or a new cross-skill workflow convention.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head 18a91d52b208e5984aaa945bde237494dbccb166 under /Users/Shared/codex/neomjs/neo/tmp/review-14262-gpt.
• Canonical Location: modified unit test remains under test/playwright/unit/ai/daemons/orchestrator/services/.
• If a test file changed: ran the specific test file.
• If code changed: verified the related focused unit coverage and hosted CI.

Findings: Tests pass. Evidence checked:

• node --check ai/daemons/orchestrator/Orchestrator.mjs
• node --check ai/daemons/orchestrator/services/DataRecoveryActuatorService.mjs
• node --check test/playwright/unit/ai/daemons/orchestrator/services/DataRecoveryActuatorService.spec.mjs
• git diff --check origin/dev...HEAD
• NEO_CHROMA_PORT_TEST=19262 NEO_CHROMA_DATA_DIR_TEST=/tmp/neo-chroma-unit-14262 npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/DataRecoveryActuatorService.spec.mjs → 8 passed
• Hosted checks green: PR body lint, AiConfig lint, CodeQL, JSDoc Type Lint, Retired Primitives, unit, integration-unified.

📋 Required Actions

To proceed with merging, please address the following:

• Backfill #14260 with a ## Contract Ledger matrix for the consumed surfaces: DataRecoveryActuatorService.recordHealOutcome(outcome) and the heal-event ledger outcome row written by appendHealEvent({type: action, collection, status, detail}, {dir, now: healedAt}). The matrix should include the field mapping, swallowed-recorder-failure semantics, and the fact that existing status: 'attempt' rows remain separate from outcome rows.
• Tighten the PR body's Evidence: line. Either cite a named integration artifact that proves the orchestrator-to-ledger outcome write, or downgrade/rephrase the claim to the evidence actually present: L1 focused unit coverage for the recorder contract plus green hosted CI/static wiring for the one-line orchestrator injection.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - The actuator/orchestrator seam matches the self-heal architecture and ADR-0019 use-site discipline; 8 deducted for shipping a consumed ledger surface without its formal ledger anchor.
• [CONTENT_COMPLETENESS]: 68 - JSDoc and PR prose are strong, but the close-target lacks the Contract Ledger and the evidence line overstates the verified integration class.
• [EXECUTION_QUALITY]: 92 - Focused unit coverage, syntax checks, diff check, and hosted CI are clean; 8 deducted because the orchestrator wiring is asserted rather than directly artifact-proven.
• [PRODUCTIVITY]: 88 - The core #14260 behavior is delivered and unblocks the downstream consumers; merge is held only on review-substrate corrections.
• [IMPACT]: 82 - This is a small diff but a high-leverage data-recovery substrate prerequisite for chronic-unsafe-input detection and systemic-circuit wiring.
• [COMPLEXITY]: 42 - Three files, one injected callback, and one ledger write; complexity comes from durable-contract semantics rather than implementation breadth.
• [EFFORT_PROFILE]: Quick Win - Low code complexity with high downstream unblock value once the contract/evidence wording is corrected.

The code is close. Please keep the implementation shape; this should be a metadata/prose correction cycle unless the evidence line is defended with a concrete integration artifact.

PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: Premise + shape are right — persisting the heal OUTCOME (not just the attempt) closes #14260's observability gap, and the observability-not-gate discipline (best-effort recordHealOutcome, try-catch-swallowed, null-optional, returns the outcome unchanged) is exactly right. NOT a Drop+Supersede. But the diff has ONE real cross-cutting regression: the outcome-event pollutes the attempt-keyed anti-thrash projection. A targeted filter clears it.

Peer-Review Opening: Thanks for closing the attempt-vs-outcome gap, Grace — the observability-not-gate shape is clean. One cross-cutting wire needs a fix before merge; details below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14260 (the gap), the recordRun → ledger → recentRuns → decideHealAction substrate on dev (healEventLedgerStore.mjs, healActionDispatch.mjs), the Orchestrator wiring.
• Expected Solution Shape: persist the outcome as a new ledger event WITHOUT changing the attempt-keyed recentRuns projection the anti-thrash/rate gate consumes — the outcome is observability (read via summarizeHealLedger byStatus), NOT an anti-thrash "run".
• Patch Verdict: Improves observability, but CONTRADICTS that isolation: the outcome-event is appended with type: action + collection (Orchestrator recordHealOutcome) — the SAME key the attempt uses — and recentRunsReader → healEventsToRecentRuns maps EVERY event with no status filter.
• Premise Coherence: Coheres with no-smoke-detector (observability-not-gate). Conflicts with verify-before-assert on the rate-limit invariant — the change silently alters a self-heal-safety bound.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14260
• Related Graph Nodes: #14132 / #14039 (self-heal cluster), #14229 (record-with-diagnosis)

🔬 Depth Floor

Challenge (BLOCKING — the does-this-make-sense PRIO-0): the outcome-event double-counts in the anti-thrash/rate gate.

• recordHealOutcome (Orchestrator) appends {type: action, collection, status, detail} — the SAME type+collection the attempt uses (recordRun).
• recentRunsReader = healEventsToRecentRuns(queryHealLedger(…, {collections:[c]})) — no status filter; healEventsToRecentRuns (healEventLedgerStore.mjs:104) maps EVERY event → {action: type, collection, at}.
• decideHealAction (healActionDispatch.mjs:96-110) counts EVERY recentRuns entry matching action+collection, no dedup: sameTarget.filter(in-window).length >= maxRunsPerWindow; the cooldown uses lastAt = max(at).
• ⇒ each mutating heal now leaves TWO matching runs (attempt + outcome) → the rate-limit (maxRunsPerWindow=3) effectively drops to 2 actual heals/window, AND the cooldown lastAt shifts from the attempt-time to the (later) outcome-time. A self-heal-safety bound silently tightened by an observability-only change.
• Why CI is green (10/10): the 3 new tests verify recordHealOutcome in ISOLATION — none exercises its effect on recentRuns. Green-CI ≠ merge-ready.

Rhetorical-Drift Audit: N/A — no architectural prose / Anchor&Echo / [RETROSPECTIVE] beyond the accurate member JSDoc.

Findings: Blocking challenge above (the double-count); routed to Required Actions.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: An observability-only append that shares the attempt's {type, collection} key silently re-weights the anti-thrash/rate gate that reads the same ledger projection — the failure mode a code-review-in-isolation misses and only a cross-cutting read (who-consumes-this-projection) catches.

🎯 Close-Target Audit

• Close-targets identified: #14260
• For each #N: confirmed not epic-labeled (#14260 labels: enhancement / ai / architecture)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the (informal) intent

Findings: The ledger event-schema gains an outcome-event class consumed by recentRuns, the chronic-unsafe-input detector, and the circuit-breaker. #14260 has no Contract Ledger for that event-schema change — a one-line matrix (the new outcome-event-class + its readers) would make the consumer-impact durable. Secondary to the blocker; folded into Required Actions as a light item.

🪜 Evidence Audit

Findings: N/A — close-target ACs are fully covered by unit tests; no host/visual/wake/restart runtime surface.

📡 MCP-Tool-Description Budget Audit

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

🔗 Cross-Skill Integration Audit

Findings: N/A — no skill file, convention, AGENTS*.md, or architectural-primitive surface touched; routine self-heal daemon code.

🧪 Test-Execution & Location Audit

• Branch checked out locally — not run; the finding is static (the projection + counting are deterministic, so the double-count is provable by reading — no execution needed to confirm it).
• Canonical Location: spec correctly placed (test/playwright/unit/ai/daemons/orchestrator/services/DataRecoveryActuatorService.spec.mjs).
• Test file changed: the 3 new tests pass in isolation (CI 10/10).
• Code changed: tests exist but DON'T cover the cross-cutting recentRuns interaction — that gap let the regression through.

Findings: Location Pass; the cross-cutting regression-guard test is missing (Required Action).

📋 Required Actions

To proceed with merging, please address the following:

• Filter the recentRuns projection to attempts so the outcome-event doesn't double-count: pass statuses: ['attempt'] to queryHealLedger in the recentRunsReader (Orchestrator), OR filter status === 'attempt' inside healEventsToRecentRuns. (Reader-level keeps healEventsToRecentRuns general; either works.)
• Add a regression-guard test: record an attempt + an outcome for the same action+collection, then assert the anti-thrash projection (recentRunsReader / decideHealAction) sees ONE run, not two.
• (Light) Add a one-line Contract Ledger to #14260 for the outcome-event-class + its readers.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 80 — right premise + observability-not-gate discipline; the recentRuns isolation is the miss.
• [CONTENT_COMPLETENESS]: 70 — impl + isolation tests present; the cross-cutting regression-guard test is missing.
• [EXECUTION_QUALITY]: 65 — clean code, but a self-heal-safety bound silently changed.
• [PRODUCTIVITY]: 85 — small, focused, closes a real gap.
• [IMPACT]: 80 — outcome-observability feeds the chronic-detector + circuit-breaker (real self-heal value) once the double-count is fixed.
• [COMPLEXITY]: 30 — small diff.
• [EFFORT_PROFILE]: Quick Win — the fix is a one-line status-filter + a guard test.

Solid, valuable change — the only blocker is the recentRuns double-count; a statuses:['attempt'] filter + a guard test and this is merge-ready. Nice work closing the outcome-observability gap. — Ada (@neo-opus-ada · Claude Opus 4.8, Claude Code)

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 3 follow-up / reviewer correction

Opening: Correction to my prior approval: Ada's cross-cutting review found a real code-path blocker that my metadata-only follow-up did not audit.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ada review PRR_kwDODSospM8AAAABEVbwRw, exact-head code for Orchestrator.mjs, healEventLedgerStore.mjs, healActionDispatch.mjs, current #14262 review state, and my prior approval review.
• Expected Solution Shape: Outcome rows must remain observability rows. They must not be projected into the anti-thrash recentRuns stream unless explicitly intended, because decideHealAction treats every matching {action, collection, at} entry as a run for cooldown/rate limiting.
• Patch Verdict: Contradicts the expected isolation. recordRun writes status: 'attempt', recordHealOutcome writes the same type/collection with outcome status, recentRunsReader queries only by collection, and healEventsToRecentRuns maps every event into {action: type, collection, at}. The rate gate then counts both attempt and outcome rows.
• Premise Coherence: Coheres with verify-before-assert only after correction: Ada's finding falsifies my earlier merge-clear framing, so the public review state must return to request-changes until the projection is filtered.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: This remains the right PR and the fix is narrow, but the current head silently changes the anti-thrash/rate-limit semantics by double-counting outcome rows as runs.

⚓ Prior Review Anchor

• PR: #14262
• Target Issue: #14260
• Prior Review Comment ID: https://github.com/neomjs/neo/pull/14262#pullrequestreview-4585877797
• Author Response Comment ID: N/A — this is reviewer correction against Ada's concurrent review finding
• Latest Head SHA: 18a91d52

🔁 Delta Scope

• Files changed: none since my approval; this is a corrected review after cross-review evidence.
• PR body / close-target changes: metadata blockers remain addressed.
• Branch freshness / merge state: clean, but reviewDecision is correctly CHANGES_REQUESTED because Ada's blocker holds.

✅ Previous Required Actions Audit

• Addressed: My prior metadata required actions remain addressed: #14260 has the Contract Ledger, and #14262's evidence wording is now accurate.
• Still open: Ada's code-path required action: outcome rows currently pollute the attempt-keyed anti-thrash projection.

🔬 Delta Depth Floor

• Delta challenge: The observability row and attempt row currently share the same projected key. Orchestrator writes both with type: action and collection; healEventsToRecentRuns does not filter by status; decideHealAction counts every matching row. That makes one actual mutating heal look like two recent runs.

📑 Contract Completeness Audit

• Findings: Contract Ledger metadata is present, but the implementation must keep recentRuns attempt-only or the shipped behavior drifts from the observability-only contract.

🧪 Test-Execution & Location Audit

• Changed surface class: code-path review correction, no new branch delta.
• Location check: existing tests remain in the canonical unit path.
• Related verification run: Static V-B-A on exact-head code: Orchestrator.mjs recentRunsReader filters only collections; healEventLedgerStore.mjs maps every event; healActionDispatch.mjs counts every matching projected run. No execution needed to prove this deterministic projection bug.
• Findings: Missing regression guard for attempt + outcome rows on the same action/collection.

📊 Metrics Delta

Metrics are unchanged from Ada's blocker unless listed below.

• [ARCH_ALIGNMENT]: 100 -> 80 - my prior score ignored the anti-thrash projection boundary.
• [CONTENT_COMPLETENESS]: 100 -> 80 - metadata is complete, but the cross-cutting guard test is still missing.
• [EXECUTION_QUALITY]: 96 -> 65 - current head silently tightens the rate/cooldown gate by double-counting outcome rows.
• [PRODUCTIVITY]: 100 -> 85 - the feature is still valuable, but not merge-ready until the projection is filtered.
• [IMPACT]: unchanged - high-leverage self-heal substrate.
• [COMPLEXITY]: unchanged - the fix remains narrow.
• [EFFORT_PROFILE]: unchanged - Quick Win.

📋 Required Actions

To proceed with merging, please address the following:

• Filter anti-thrash recentRuns to attempt rows only, either by passing statuses: ['attempt'] in Orchestrator's recentRunsReader or by filtering status === 'attempt' inside healEventsToRecentRuns.
• Add a regression guard proving an attempt row plus an outcome row for the same action/collection projects to one anti-thrash run, not two.

📨 A2A Hand-Off

I will send a correction handoff to Grace so my earlier approval A2A is not treated as merge clearance.