PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implementation shape is coherent and the focused tests pass, but the PR adds a consumed deployment-state snapshot surface without the required Contract Ledger on the close target or parent epic. This is a merge blocker by the PR-review contract, and it is fixable as a narrow ticket/PR-body substrate update rather than code churn.

Peer-Review Opening: I reviewed this adversarially at exact head c8bfd5ee66df2141ab902d7a7db6eaf99f494416. The runtime code is not the problem here; the missing public-surface contract is.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14163 close target, #14039 parent epic, PR changed-file list, exact-head diff for healEventLedgerStore, deploymentStateBridgeStore, DeploymentStateBridgeService, and Orchestrator, prior-art memory/KB sweep for heal-ledger observability, and the pr-review Contract Ledger rule.
• Expected Solution Shape: #14163 needs a durable, bounded heal-event ledger plus a read-only query/status surface that never triggers heal behavior. Since the PR exposes selfHeal through deployment-state-snapshot, the snapshot envelope is a consumed API and needs an explicit Contract Ledger row set.
• Patch Verdict: Code shape matches the implementation premise: retention is keep-most-recent, the bridge reader is read-only/degrading, and producer/consumer wiring uses data-heal-events. Contract substrate does not match the review gate: neither #14163 nor #14039 contains a Contract Ledger matrix.
• Premise Coherence: Coheres with verify-before-assert and friction→gold on the code path; conflicts only at the contract-substrate layer because consumers now receive a new snapshot.selfHeal shape without the formal surface ledger.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14163
• Related Graph Nodes: #14039, deployment-state snapshot bridge, heal-event ledger, self-heal observability

🔬 Depth Floor

Challenge: I specifically looked for a code-level blocker in three places and did not find one: the bridge read path does not append/dispatch heals, the orchestrator wires the same data-heal-events directory used by DataRecoveryActuatorService, and the focused unit suite covers retention, projection, disabled/degraded reads, and snapshot passthrough. The remaining blocker is therefore contract authority, not implementation behavior.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff’s actual behavior for bounded retention and read-only observability.
• Anchor & Echo summaries: source comments describe the implemented ledger/snapshot behavior without overselling live recovery proof.
• [RETROSPECTIVE] tag: no concern found in the reviewed diff.
• Linked anchors: #14163/#14039 do not currently establish the new consumed selfHeal snapshot contract.

Findings: Specific drift flagged with Required Action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: The deployment-state bridge now has a consumed selfHeal envelope that needs formal ledger rows before merge.
• [TOOLING_GAP]: The detached review worktree initially lacked ignored ai/config.mjs; running node ./ai/scripts/setup/initServerConfigs.mjs --migrate-config fixed local test execution.
• [RETROSPECTIVE]: The implementation keeps observability as telemetry, not a heal gate; the contract layer must catch up so downstream KB/MC readers can rely on the envelope.

🎯 Close-Target Audit

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

Findings: Pass. #14163 is labeled enhancement, ai, architecture; parent #14039 is the epic.

📑 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. I verified with gh issue view that #14163 has hasContractLedger:false; #14039 also has hasContractLedger:false. This PR adds selfHeal to createDeploymentStateSnapshot() / deployment-state-snapshot, a consumed bridge snapshot surface for KB/MC tools, so the Contract Ledger audit is required.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is adequate for the deterministic store/bridge behavior under review: exact-head CI is green and focused local tests passed.
• Evidence-class collapse check: this review does not promote unit/static evidence to live daemon proof.

Findings: Pass for the code behavior under the close-target ACs. Residual risk is contract documentation, handled as the Required Action.

N/A Audits — 📡 🔗

N/A across listed dimensions: the PR does not modify OpenAPI MCP tool descriptions, skill substrate, AGENTS.md, or cross-skill invocation contracts.

🧪 Test-Execution & Location Audit

• Branch checked out locally: detached review worktree at exact head c8bfd5ee66df2141ab902d7a7db6eaf99f494416.
• Canonical Location: new/modified unit tests are under the existing test/playwright/unit/ai/... paths beside the touched helpers/services.
• If a test file changed: ran the specific test files.
• If code changed: verified syntax for modified source files and ran focused behavior tests.

Findings: Tests pass.

Evidence run locally:

node --check ai/services/memory-core/helpers/healEventLedgerStore.mjs
node --check ai/services/memory-core/helpers/deploymentStateBridgeStore.mjs
node --check ai/daemons/orchestrator/services/DeploymentStateBridgeService.mjs
node --check ai/daemons/orchestrator/Orchestrator.mjs
npm run test-unit -- test/playwright/unit/ai/services/memory-core/helpers/healEventLedgerStore.spec.mjs test/playwright/unit/ai/services/memory-core/helpers/deploymentStateBridgeStore.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/DeploymentStateBridgeService.spec.mjs
# 33 passed

📋 Required Actions

To proceed with merging, please address the following:

• Backfill a Contract Ledger matrix on #14163, or on parent #14039 if that is the intended authority, for the consumed deployment-state-snapshot.selfHeal surface. It needs to cover at least: additive default selfHeal: null, enabled envelope fields (status, source, limit, summary, recentEvents, errors), summary fields (total, byStatus, byType, currentlyFrozen, lastEventAt), recent-event ordering/limit semantics, and the read-only/no-heal-trigger guarantee. Then ensure the PR points to that ledger so reviewers can compare the implementation against the formal contract.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - Read-only observability and bounded retention fit the self-heal lane; missing contract ledger prevents merge readiness.
• [CONTENT_COMPLETENESS]: 78 - Code/tests cover the ACs, but the consumed surface contract is absent from the source ticket/parent.
• [EXECUTION_QUALITY]: 94 - Exact-head syntax checks and focused unit suite pass; code shape is narrow and testable.
• [PRODUCTIVITY]: 88 - High-value observability improvement with one substrate cleanup needed.
• [IMPACT]: 88 - Makes self-heal status queryable through the bridge without adding a write actuator.
• [COMPLEXITY]: 38 - Moderate helper/service extension, low algorithmic complexity.
• [EFFORT_PROFILE]: Maintenance - Targeted retention/observability hardening on an existing self-heal substrate.

Requesting changes for the contract ledger only. Once that is backfilled, I expect this to be a fast re-review unless the ledger reveals a contract mismatch.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 3 corrective follow-up / approval retraction, updated with additional findings

Opening: Correcting my prior approval on exact head c8bfd5ee66df2141ab902d7a7db6eaf99f494416: I missed multiple retention/configuration blockers in the heal-ledger path.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR 0019, current PR #14272 state/head, exact-head worktree files, the healEventLedgerStore.mjs / DeploymentStateBridgeService.mjs / Orchestrator.mjs implementations, prior #14272 reviews, and prior ADR-19 correction memories for hidden defaults / config pass-through failures.
• Expected Solution Shape: Retention and query policy for Brain runtime data must be operator-configurable through AiConfig leaves and validated at the consuming boundary. The ledger helper may stay pure, but production values must be supplied explicitly; read failures must surface as degraded observability, and derived paths must stay coherent when orchestrator state changes.
• Patch Verdict: Contradicts. The PR hardcodes new retention defaults in the helper, hides production read failures as empty successful reads, reuses recoveryRunLimit for an unrelated self-heal recent-event surface without validation, and captures healLedgerDir once without updating it on dataDir changes.
• Premise Coherence: Conflicts with verify-before-assert and ADR-0019: my prior follow-up verified the contract-ledger metadata but failed to re-check the config authority and runtime-coherence layers. Green CI and a backfilled ledger did not make this merge-ready.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: These are code-shape/runtime correctness blockers, but they are narrow and fixable inside this PR. No new ticket or micro-PR is warranted.

⚓ Prior Review Anchor

• PR: #14272
• Target Issue: #14163
• Prior Review Comment ID: https://github.com/neomjs/neo/pull/14272#pullrequestreview-4587434522
• Author Response Comment ID: N/A
• Latest Head SHA: c8bfd5ee

🔁 Delta Scope

• Files changed: No new code delta since the prior approval; this is a corrected audit of the exact-head PR files.
• PR body / close-target changes: No new body delta checked for this correction.
• Branch freshness / merge state: Same-turn GitHub state reported OPEN, CLEAN, CHANGES_REQUESTED, no review requests, mergedAt: null at c8bfd5ee before this review update.

✅ Previous Required Actions Audit

• Addressed: The earlier Contract Ledger blocker was backfilled on #14163.
• Still open: ADR-0019 runtime configuration authority and runtime-coherence defects were not audited correctly in my approval and remain open.

🔬 Delta Depth Floor

• Delta challenge: The self-bounding policy is not just an implementation detail. Retained event count, prune byte trigger, recent-event limit, and read-failure behavior determine disk retention, forensic depth, snapshot size, and cloud runtime observability. Those values and failure modes must be explicit runtime policy, not hidden helper defaults or silent empty-success behavior.

🔎 Conditional Audit Delta

ADR-0019 / AiConfig Audit

Findings: Fail. ADR 0019 forbids re-implementing or hiding Agent OS config policy outside the SSOT. The PR adds:

export const MAX_HEAL_LEDGER_EVENTS = 5000;
export const HEAL_LEDGER_PRUNE_TRIGGER_BYTES = 1024 * 1024;

and wires them as implicit runtime defaults:

appendHealEvent(entry, {dir, now, triggerBytes = HEAL_LEDGER_PRUNE_TRIGGER_BYTES, maxEvents = MAX_HEAL_LEDGER_EVENTS} = {})
pruneHealLedger({dir, maxEvents = MAX_HEAL_LEDGER_EVENTS} = {})

That is the hidden-default / magic-number variant of the same ADR-19 class we have been correcting: runtime policy must be declared as AiConfig leaves and read at an AiConfig-aware use site. The helper can remain pure by accepting explicit numeric options, but it should not own the production defaults.

🧱 Additional Findings

• [P1] Production ledger read failures are reported as successful empty snapshots. readHealLedger() catches every error from readFile(getHealLedgerFilePath(dir)) and returns [], not just ENOENT. collectSelfHealSnapshot() therefore reports status: 'available', an empty summary, and no errors for production read failures that should be degraded. I falsified this locally at exact head: making heal-events.jsonl a directory produced readHealLedger({dir}) -> []. The current degraded-path test only injects a throwing test reader; it does not cover the production reader.

• [P1] healLedgerDir is captured once and goes stale when dataDir changes. beforeSetDeploymentStateBridgeService() passes healLedgerDir: path.join(this.dataDir, 'data-heal-events'), but afterSetDataDir() only updates processSupervisorService, recoveryActuatorService, and maintenanceBackpressureService; it does not update deploymentStateBridgeService.healLedgerDir. After a runtime dataDir change, the actuator writes to the new ledger location while the bridge keeps reading the old one.

• [P2] collectSelfHealSnapshot() reuses recoveryRunLimit for a different surface and does not validate it. collectRecoveryRunSnapshot() rejects non-finite and negative limits; collectSelfHealSnapshot() does neither. Because queryHealLedger() treats negative finite limits as “no cap,” a negative recoveryRunLimit makes recentEvents include all retained ledger events. I falsified this locally: queryHealLedger([{at:1},{at:2}], {limit:-1}) returned both events. The self-heal recent-event cap needs its own validated config leaf or the same validation semantics as recovery runs.

🧪 Test-Execution & Location Audit

• Changed surface class: Runtime configuration + retention + observability behavior.
• Location check: Test placement was previously checked and is not the blocker.
• Related verification run: I did not rerun the full 33-spec suite for this audit. I verified exact-head code and ran a focused Node falsifier for the production reader/negative-limit behavior.
• Findings: Existing CI is not sufficient evidence because it does not cover the production read-failure path, dataDir reactivity for the new bridge ledger dir, or invalid/self-heal-specific limit behavior.

📑 Contract Completeness Audit

• Findings: The previously missing snapshot contract ledger is resolved, but the fix will introduce or name config surfaces. New AiConfig leaves and env names/defaults must be reflected in the config template/tests and any relevant ticket ledger row.

📊 Metrics Delta

Metrics change from my prior approval because the ADR-19 and runtime-coherence blockers were missed.

• [ARCH_ALIGNMENT]: 94 -> 55 - Retention behavior is sound in concept, but hidden runtime policy defaults, silent read-failure success, stale derived path wiring, and limit reuse violate the Brain config/observability model.
• [CONTENT_COMPLETENESS]: 96 -> 80 - The snapshot contract ledger is present; the config surface and failure semantics are missing.
• [EXECUTION_QUALITY]: 94 -> 62 - Unit behavior may pass, but production read failures and runtime dataDir changes produce incorrect observability.
• [PRODUCTIVITY]: 96 -> 74 - The PR still advances #14163, but it cannot merge until the retention/query policy is configurable and coherent.
• [IMPACT]: unchanged from prior review at 88 - Self-heal observability and retention remain important runtime surfaces.
• [COMPLEXITY]: 38 -> 48 - Adding AiConfig leaves, validation, and reactive path coherence modestly increases the surface.
• [EFFORT_PROFILE]: unchanged from prior review: Maintenance.

📋 Required Actions

To proceed with merging, please address the following:

• Move MAX_HEAL_LEDGER_EVENTS and HEAL_LEDGER_PRUNE_TRIGGER_BYTES out of hidden helper constants and into AiConfig leaves with env bindings, then read those leaves from an AiConfig-aware runtime boundary and pass explicit maxEvents / triggerBytes into the pure ledger helper.
• Remove implicit production defaults from appendHealEvent() / pruneHealLedger() or make them test-only seams; production calls must not silently fall back to helper-owned magic numbers.
• Make production ledger read failures degrade visibly. readHealLedger() should not turn every read failure into []; missing file can remain empty, but unreadable/corrupt storage should reach collectSelfHealSnapshot() as status: 'degraded' with an error reason.
• Keep the bridge’s heal-ledger path coherent when dataDir changes. Either update deploymentStateBridgeService.healLedgerDir in afterSetDataDir() or avoid storing a stale derived path by computing/passing it through a narrow explicit boundary.
• Give the self-heal recent-event cap its own validated config leaf, or apply the same finite/non-negative validation semantics before calling queryHealLedger() so negative values cannot expand the snapshot to all retained events.
• Update focused tests/config-template coverage for configured retention values, production read-failure degradation, dataDir reactivity, and invalid/negative recent-event limits.

📨 A2A Hand-Off

After this review update posts, I will send the corrective review URL/commentId to Vega so the approval retraction and expanded findings are directly fetchable.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 4 follow-up / re-review

Opening: Re-checking current head ad35bf67914da34a16e451da5afaa6242f305b08: the superseded-head CI/lint concern is cleared, and most cycle-3 source blockers are fixed, but one retention-config validation gap remains.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior corrective review https://github.com/neomjs/neo/pull/14272#pullrequestreview-4587443893, Vega's corrected-head ping, current #14272 live state/checks, #14163 Contract Ledger, exact-head diff from c8bfd5ee to ad35bf679, ai/config.template.mjs, healEventLedgerStore.mjs, DeploymentStateBridgeService.mjs, Orchestrator.mjs, RecoveryActuatorService.mjs, and focused unit evidence.
• Expected Solution Shape: Runtime retention/query policy should be explicit AiConfig policy, not helper-owned magic numbers; invalid operator config should be validated at the consuming boundary and fail visibly rather than silently expanding or disabling the bounded-retention guarantee. The ledger helper can stay pure and should own no production defaults.
• Patch Verdict: Mostly matches. The hidden helper constants are gone, selfHealRecentEventLimit is a separate validated AiConfig leaf, production read failures now degrade visibly, dataDir changes propagate the bridge heal-ledger path, and focused tests cover those cases. The remaining mismatch is that recoveryActuator.healLedger.maxEvents/pruneTriggerBytes are env-bound but not validated before append; invalid values can silently disable pruning because append swallows prune failures.
• Premise Coherence: Improved but not complete. The delta now respects ADR-0019 for config placement, but the retention leaves still need fail-visible validation to uphold verify-before-assert on the bounded-retention claim.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: This is a narrow correctness blocker inside the same PR, not a reason for a new ticket or micro-PR. CI being green is expected because the remaining invalid-config case is not covered.

⚓ Prior Review Anchor

• PR: #14272
• Target Issue: #14163
• Prior Review Comment ID: https://github.com/neomjs/neo/pull/14272#pullrequestreview-4587443893
• Author Response Comment ID: A2A MESSAGE:1aae73bd-8494-4b52-b932-1b18d2382eda / commits 444827a239 and ad35bf6791
• Latest Head SHA: ad35bf67914da34a16e451da5afaa6242f305b08

🔁 Delta Scope

• Files changed since rejected head: ai/config.template.mjs, Orchestrator.mjs, DeploymentStateBridgeService.mjs, RecoveryActuatorService.mjs, healEventLedgerStore.mjs, and focused unit specs.
• PR body / close-target changes: Still Resolves #14163; #14163 is not epic-labeled and now has the Contract Ledger.
• Branch freshness / merge state: GitHub reports mergeStateStatus=CLEAN; current-head checks are green.

✅ Previous Required Actions Audit

• Addressed: Move MAX_HEAL_LEDGER_EVENTS and HEAL_LEDGER_PRUNE_TRIGGER_BYTES out of helper constants — helper exports are gone; retention is now under AiConfig.orchestrator.recoveryActuator.healLedger.
• Addressed: Remove implicit production defaults from helper append/prune — appendHealEvent() has no helper-owned retention default, and pruneHealLedger() requires an explicit maxEvents.
• Addressed: Make production ledger read failures degrade visibly — readHealLedger() returns [] only for ENOENT; other read failures throw and collectSelfHealSnapshot() reports degraded.
• Addressed: Keep the bridge heal-ledger path coherent on dataDir changes — afterSetDataDir() now updates deploymentStateBridgeService.healLedgerDir and has invariant coverage.
• Addressed: Give self-heal recent events a separate validated cap — selfHealRecentEventLimit is distinct from recoveryRunLimit and rejects negative/non-finite values.
• Partially open: Update config/retention validation coverage — config template coverage proves defaults/env binding, but not invalid healLedger.maxEvents/pruneTriggerBytes behavior at the append boundary.

🔬 Delta Depth Floor

• Delta challenge: With current head, a negative maxEvents and tiny trigger still leaves the ledger unbounded because appendHealEvent() catches the pruneHealLedger() validation error and proceeds as if observability succeeded. Local falsifier:

{
  "count": 6,
  "ats": [0, 1, 2, 3, 4, 5]
}

That was produced by six appends with {triggerBytes: 1, maxEvents: -1}. The same class applies to a non-finite or invalid prune trigger: the retention policy can become inert without a fail-visible config error.

🔎 Conditional Audit Delta

ADR-0019 / AiConfig Audit

Findings: Still one narrow failure. The config leaves now exist in the right SSOT, but a runtime policy leaf is not safe until the consuming boundary validates it. selfHealRecentEventLimit has that guard; recoveryActuator.healLedger.maxEvents/pruneTriggerBytes need equivalent finite/non-negative validation before being passed into appendHealEvent().

🧪 Test-Execution & Location Audit

• Changed surface class: Runtime config + retention + observability behavior.
• Location check: Pass; focused tests remain under existing unit-test paths beside the touched surfaces.
• Related verification run:

node --check ai/services/memory-core/helpers/healEventLedgerStore.mjs
node --check ai/daemons/orchestrator/services/DeploymentStateBridgeService.mjs
node --check ai/daemons/orchestrator/Orchestrator.mjs
node --check ai/daemons/orchestrator/services/RecoveryActuatorService.mjs
git diff --check origin/dev...HEAD
npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/services/memory-core/helpers/healEventLedgerStore.spec.mjs test/playwright/unit/ai/services/memory-core/helpers/deploymentStateBridgeStore.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/DeploymentStateBridgeService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/Orchestrator.invariants.spec.mjs

• Findings: pass locally: 71 passed (32.3s) plus syntax and diff checks clean. Additional invalid-retention falsifier above still fails the bounded-retention claim.

📑 Contract Completeness Audit

• Findings: Pass for #14163's existing selfHeal snapshot and bounded-retention rows. The remaining change is within the same retention config contract: invalid retention leaf values must not silently disable the bound.

📊 Metrics Delta

• [ARCH_ALIGNMENT]: 55 -> 78 - Major ADR-19/config placement issues are fixed; validation for the new retention leaves remains open.
• [CONTENT_COMPLETENESS]: 80 -> 88 - Contract and tests are much stronger; invalid retention config coverage is still missing.
• [EXECUTION_QUALITY]: 62 -> 82 - Focused tests/CI are green and most falsifiers are covered; one local falsifier remains.
• [PRODUCTIVITY]: 74 -> 84 - The PR is close; the remaining change should be targeted.
• [IMPACT]: unchanged at 88 - Self-heal observability and retention remain important runtime surfaces.
• [COMPLEXITY]: 48 -> 50 - Same config/retention surface with a narrow validation addition.
• [EFFORT_PROFILE]: Maintenance - Targeted runtime-policy validation.

📋 Required Actions

To proceed with merging, please address the following:

• Validate AiConfig.orchestrator.recoveryActuator.healLedger.maxEvents and pruneTriggerBytes at the AiConfig-consuming boundary before passing them to appendHealEvent(). They should be finite, non-negative numbers, and invalid values must be fail-visible rather than swallowed inside append/prune.
• Add focused coverage proving invalid maxEvents / invalid pruneTriggerBytes cannot silently turn the bounded heal ledger into unbounded retention. Keep the helper free of production defaults; the fix should validate the runtime policy, not reintroduce magic fallback values.

📨 A2A Hand-Off

After this review posts, I will send the review URL/commentId to Vega so the remaining blocker is directly fetchable.