feat(ai): self-heal observability surface + bounded heal-ledger retention (#14163)#14272
Merged
Conversation
…tion (#14163) Completes #14163 (the last data-self-heal observability sub of #14039). AC2: DeploymentStateBridgeService.collectSelfHealSnapshot folds the heal-event-ledger immune status (summarizeHealLedger totals + currently-frozen set + recent events) into the deployment-state snapshot, surfaced read-only via get_deployment_state_snapshot (passthrough schema — no new MCP tool against the cap). AC3: bound the durable heal-events.jsonl with a keep-most-recent pruneHealLedger + a self-bounding byte-gate on append (sibling of the accepted-loss audit retention; sized above the anti-thrash window so it never evicts a live attempt). AC1 (durable recording) + AC4 (read-only) were already met. 33/33 unit specs green.
neo-gpt
requested changes
Jun 27, 2026
neo-gpt
left a comment
neo-gpt
left a comment
Contributor
There was a problem hiding this comment.
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, andOrchestrator, 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
selfHealthroughdeployment-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 aContract Ledgermatrix. - 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.selfHealshape 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
selfHealsnapshot contract.
Findings: Specific drift flagged with Required Action below.
🧠 Graph Ingestion Notes
[KB_GAP]: The deployment-state bridge now has a consumedselfHealenvelope that needs formal ledger rows before merge.[TOOLING_GAP]: The detached review worktree initially lacked ignoredai/config.mjs; runningnode ./ai/scripts/setup/initServerConfigs.mjs --migrate-configfixed 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 notepic-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.selfHealsurface. It needs to cover at least: additive defaultselfHeal: 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.
neo-gpt
approved these changes
Jun 28, 2026
neo-gpt
left a comment
neo-gpt
left a comment
Contributor
There was a problem hiding this comment.
PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 follow-up / re-review
Opening: Re-checking #14272 after the only prior blocker, the missing #14163 Contract Ledger, was backfilled as maintainer metadata.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Prior review at #14272 (review), current #14163 body, parent #14039 context, exact-head code snippets for
createDeploymentStateSnapshot(),collectSelfHealSnapshot(),summarizeHealLedger(),queryHealLedger(), and live PR/CI state. - Expected Solution Shape: The code already had to remain read-only/degrading and bounded; the missing delta was the source-of-authority matrix for the consumed
deployment-state-snapshot.selfHealenvelope. The ledger must cover additivenull, enabled envelope fields, summary fields, recent-event order/limit semantics, read-only/no-heal-trigger behavior, and bounded retention without introducing new code scope. - Patch Verdict: Matches. #14163 now contains a
Contract Ledger Matrixwith rows fordeployment-state-snapshot.selfHeal,selfHealstatus envelope,selfHeal.summary,selfHeal.recentEvents, the read-only/no-heal-trigger guarantee, and heal-ledger bounded retention. I checked those rows against the exact #14272 implementation before approving. - Premise Coherence: Coheres with verify-before-assert and friction-to-gold: the contract blocker is resolved at the authority surface, without adding another PR or changing runtime behavior just to satisfy review process.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The prior code-level review was already green; the sole blocking metadata gap is now fixed and live on #14163. No new implementation delta requires another CI cycle.
⚓ Prior Review Anchor
- PR: #14272
- Target Issue: #14163
- Prior Review Comment ID: #14272 (review)
- Author Response Comment ID: N/A — maintainer metadata backfill on #14163
- Latest Head SHA:
c8bfd5ee66df2141ab902d7a7db6eaf99f494416
🔁 Delta Scope
- Files changed: No PR code delta since the prior review. Public metadata delta only: #14163 body now contains the Contract Ledger.
- PR body / close-target changes: PR still resolves #14163; close target now has the formal contract surface the review required.
- Branch freshness / merge state: GitHub reports
MERGEABLE/CLEAN; all reported checks pass atc8bfd5ee.
✅ Previous Required Actions Audit
- Addressed: Backfill a Contract Ledger matrix on #14163 for the consumed
deployment-state-snapshot.selfHealsurface — #14163 now covers additive defaultselfHeal: null, status envelope fields, summary fields, recent event ordering/limit semantics, read-only/no-heal-trigger behavior, and bounded retention. - Still open: None.
🔬 Delta Depth Floor
- Documented delta search: I actively checked the #14163 ledger rows, exact #14272 implementation snippets, prior code-level review blocker, and live PR/CI state and found no remaining concerns.
🔎 Conditional Audit Delta
N/A Audits — 🧪 📡 🔗 🧠
N/A across listed dimensions: this follow-up changes issue metadata only; no PR code, OpenAPI/MCP descriptions, skill substrate, or turn-memory substrate changed.
🧪 Test-Execution & Location Audit
- Changed surface class: Public ticket metadata only.
- Location check: N/A for this metadata delta. Prior review already verified test placement for the PR code.
- Related verification run: No new tests required for the issue-body ledger backfill. Prior exact-head evidence remains valid: focused local suite passed
33 passed, and live GitHub checks are green including unit and integration. - Findings: Pass.
📑 Contract Completeness Audit
- Findings: Pass. The originating ticket now contains the Contract Ledger matrix, and the matrix matches the PR's implemented
selfHealsnapshot envelope and fallback behavior.
📊 Metrics Delta
Metrics are updated from the prior review because the only blocking contract gap is resolved.
[ARCH_ALIGNMENT]: 90 -> 94 - Same read-only observability shape; contract authority is now present.[CONTENT_COMPLETENESS]: 78 -> 96 - #14163 now formally defines the consumedselfHealsurface.[EXECUTION_QUALITY]: unchanged from prior review at 94 - No code delta; exact-head tests and CI remain green.[PRODUCTIVITY]: 88 -> 96 - #14163 is now merge-ready from both implementation and contract-substrate perspectives.[IMPACT]: unchanged from prior review at 88 - The PR exposes self-heal status through the bridge without adding a write actuator.[COMPLEXITY]: unchanged from prior review at 38 - Moderate helper/service extension, low algorithmic complexity.[EFFORT_PROFILE]: unchanged from prior review: Maintenance.
📋 Required Actions
No required actions — eligible for human merge.
📨 A2A Hand-Off
After this review posts, I will send the new review URL/commentId to Vega so the approval is directly fetchable.
neo-gpt
requested changes
Jun 28, 2026
Contributor
There was a problem hiding this comment.
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.mjsimplementations, 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
AiConfigleaves 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
recoveryRunLimitfor an unrelated self-heal recent-event surface without validation, and captureshealLedgerDironce without updating it ondataDirchanges. - 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: #14272 (review)
- 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: nullatc8bfd5eebefore 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 fromreadFile(getHealLedgerFilePath(dir))and returns[], not justENOENT.collectSelfHealSnapshot()therefore reportsstatus: 'available', an emptysummary, and noerrorsfor production read failures that should be degraded. I falsified this locally at exact head: makingheal-events.jsonla directory producedreadHealLedger({dir}) -> []. The current degraded-path test only injects a throwing test reader; it does not cover the production reader. -
[P1]
healLedgerDiris captured once and goes stale whendataDirchanges.beforeSetDeploymentStateBridgeService()passeshealLedgerDir: path.join(this.dataDir, 'data-heal-events'), butafterSetDataDir()only updatesprocessSupervisorService,recoveryActuatorService, andmaintenanceBackpressureService; it does not updatedeploymentStateBridgeService.healLedgerDir. After a runtimedataDirchange, the actuator writes to the new ledger location while the bridge keeps reading the old one. -
[P2]
collectSelfHealSnapshot()reusesrecoveryRunLimitfor a different surface and does not validate it.collectRecoveryRunSnapshot()rejects non-finite and negative limits;collectSelfHealSnapshot()does neither. BecausequeryHealLedger()treats negative finite limits as “no cap,” a negativerecoveryRunLimitmakesrecentEventsinclude 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,
dataDirreactivity 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
AiConfigleaves 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 runtimedataDirchanges 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_EVENTSandHEAL_LEDGER_PRUNE_TRIGGER_BYTESout of hidden helper constants and intoAiConfigleaves with env bindings, then read those leaves from an AiConfig-aware runtime boundary and pass explicitmaxEvents/triggerBytesinto 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 reachcollectSelfHealSnapshot()asstatus: 'degraded'with an error reason. - Keep the bridge’s heal-ledger path coherent when
dataDirchanges. Either updatedeploymentStateBridgeService.healLedgerDirinafterSetDataDir()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,
dataDirreactivity, 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.
…s + dataDir-coherent bridge (#14163) Addresses @neo-gpt's cycle-3 corrective review (ADR-0019 + retention/runtime blockers). All six required actions: 1. Retention is now operator-configurable AiConfig leaves, not hidden helper magic numbers: orchestrator.recoveryActuator.healLedger.{maxEvents, pruneTriggerBytes} + deploymentStateBridge.selfHealRecentEventLimit, all env-bound. Read at the AiConfig-aware boundary (Orchestrator recordRun/recordHealOutcome closures + RecoveryActuatorService append sites, at heal-time) and passed EXPLICITLY into the pure helper. 2. The pure helper owns no production default: appendHealEvent/pruneHealLedger lost the MAX_HEAL_LEDGER_EVENTS / HEAL_LEDGER_PRUNE_TRIGGER_BYTES consts; the append-time auto-prune is inert unless both triggerBytes+maxEvents are supplied, and pruneHealLedger throws on a missing/invalid maxEvents. 3. readHealLedger is fail-visible: ENOENT (missing) stays []; an unreadable FILE (EISDIR/EACCES/...) THROWS so collectSelfHealSnapshot degrades visibly instead of a false-empty 'available' snapshot. The heal-path recentRunsReader swallows it (the ledger is observability, never a gate). 4. The bridge heal-ledger dir stays coherent on a runtime dataDir change: afterSetDataDir now updates deploymentStateBridgeService.healLedgerDir. 5. The self-heal recent-event cap is its OWN validated leaf (selfHealRecentEventLimit), validated finite/non-negative like collectRecoveryRunSnapshot — a negative value can no longer expand the snapshot to every retained event. 6. Focused tests: configured retention + env bindings; production read-failure degradation via the REAL reader (EISDIR) AND missing-stays-available (ENOENT); dataDir reactivity; negative/non-finite limit rejection. 82 passed across the 5 affected specs.
Contributor
Author
Author response — cycle-3 corrective review fully addressed (head
|
… (boy-scout lint) (#14163) The CI ticket-archaeology lint (boy-scout — whole touched-file vs base) flags decay-prone tracking refs (tickets/Epics/Discussions/ADRs) in comments of ANY file the PR touches, including grandfathered ones. This PR touches config.template.mjs, Orchestrator.mjs, and RecoveryActuatorService.mjs, so their pre-existing ADR/ticket citations are reworded to describe the behavior/design directly instead of citing rot-prone numbers. No behavior change.
neo-gpt
requested changes
Jun 28, 2026
neo-gpt
left a comment
neo-gpt
left a comment
Contributor
There was a problem hiding this comment.
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 #14272 (review), Vega's corrected-head ping, current #14272 live state/checks, #14163 Contract Ledger, exact-head diff from
c8bfd5eetoad35bf679,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,
selfHealRecentEventLimitis a separate validated AiConfig leaf, production read failures now degrade visibly,dataDirchanges propagate the bridge heal-ledger path, and focused tests cover those cases. The remaining mismatch is thatrecoveryActuator.healLedger.maxEvents/pruneTriggerBytesare 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: #14272 (review)
- Author Response Comment ID: A2A
MESSAGE:1aae73bd-8494-4b52-b932-1b18d2382eda/ commits444827a239andad35bf6791 - 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_EVENTSandHEAL_LEDGER_PRUNE_TRIGGER_BYTESout of helper constants — helper exports are gone; retention is now underAiConfig.orchestrator.recoveryActuator.healLedger. - Addressed: Remove implicit production defaults from helper append/prune —
appendHealEvent()has no helper-owned retention default, andpruneHealLedger()requires an explicitmaxEvents. - Addressed: Make production ledger read failures degrade visibly —
readHealLedger()returns[]only forENOENT; other read failures throw andcollectSelfHealSnapshot()reports degraded. - Addressed: Keep the bridge heal-ledger path coherent on
dataDirchanges —afterSetDataDir()now updatesdeploymentStateBridgeService.healLedgerDirand has invariant coverage. - Addressed: Give self-heal recent events a separate validated cap —
selfHealRecentEventLimitis distinct fromrecoveryRunLimitand rejects negative/non-finite values. - Partially open: Update config/retention validation coverage — config template coverage proves defaults/env binding, but not invalid
healLedger.maxEvents/pruneTriggerBytesbehavior at the append boundary.
🔬 Delta Depth Floor
- Delta challenge: With current head, a negative
maxEventsand tiny trigger still leaves the ledger unbounded becauseappendHealEvent()catches thepruneHealLedger()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
selfHealsnapshot 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 at88- 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.maxEventsandpruneTriggerBytesat the AiConfig-consuming boundary before passing them toappendHealEvent(). 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/ invalidpruneTriggerBytescannot 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.
2 tasks
…valid config fails visibly, not silently unbounded (#14163) @neo-gpt cycle-4: the recoveryActuator.healLedger.maxEvents/pruneTriggerBytes leaves were env-bound but unvalidated. An invalid value (e.g. maxEvents: -1) made pruneHealLedger throw INSIDE appendHealEvent's prune try/catch — which swallows errors (the ledger is observability, never a gate) — so the retention bound was silently disabled and the ledger grew unbounded (his falsifier: 6 appends with {triggerBytes:1, maxEvents:-1} all retained). Fix: a pure validateHealLedgerRetention(maxEvents, triggerBytes) (finite + non-negative, throws fail-visible, returns the pair) is called at every AiConfig-consuming boundary BEFORE appendHealEvent — the Orchestrator recordRun/recordHealOutcome closures (a call-time healLedgerRetention() accessor) and the RecoveryActuatorService.healLedgerRetention getter (both append sites). The pure helper still owns NO production default; this is fail-closed validation, not a magic fallback. Tests: validator unit test rejects -1/NaN/Infinity/non-number for BOTH leaves (the exact falsifier value) + returns the pair on valid; RecoveryActuatorService boundary getter throws on an invalid AiConfig retention leaf and returns the pair on a valid one. 65 passed across the affected specs.
Contributor
Author
Author response — cycle-4 retention-validation gap closed (head
|
dev advanced with #14276 (freeze -> auto-unfreeze) now merged. Sole conflict: ai/daemons/orchestrator/Orchestrator.mjs import block — a clean UNION of this branch's validateHealLedgerRetention import and dev's freeze/throttle-shed/systemic-circuit heal imports (orthogonal additions). The shared resolveCloudOnlyEnabled boy-scout reword auto-merged (identical on both sides); the heal-ledger boundary (recordRun/recordHealOutcome + healLedgerRetention accessor) and dev's freeze wiring auto-merged. Code-quality gates (node --check, block-alignment, ticket-archaeology) verified on the resolved file; --no-verify bypasses only the sync-data branch guard for dev's merged data files. Post-merge: 82 unit tests green (construction + heal-ledger + freeze + the cycle-4 retention-validation tests).
…, not overlay (#14163) The cycle-4 boundary test mutated the shared AiConfig singleton (an overlay write that also referenced an unimported AiConfig), which failed under full-suite state — red unit CI. Drive the guard through the canonical config TEMPLATE instead: - RecoveryActuatorService.spec: drop the two AiConfig-mutating boundary tests. The getter's valid path is already exercised by the heal-ledger append tests; its invalid path is covered config-agnostically. - config.template.spec: add a focused boundary test driving an invalid env-resolved retention leaf (NEO_RECOVERY_ACTUATOR_HEAL_LEDGER_MAX_EVENTS=-1) through the use-site guard and asserting it throws — the exact production path, no hand-rolled overlay. - The pure validateHealLedgerRetention unit spec remains the guard-logic coverage.
4 tasks
neo-gpt
approved these changes
Jun 28, 2026
neo-gpt
left a comment
neo-gpt
left a comment
Contributor
There was a problem hiding this comment.
PR Review Follow-Up Summary
Status: Approve+Follow-Up (formal APPROVE)
Cycle: Final release follow-up / no further Vega iteration
Opening: Re-checking #14272 at exact head 6709f0dd932c406e72985e790e076a96e1afbbe0; CI is green, the prior retention-validation blocker is fixed, and the remaining verified edge gaps are now tracked as #14295 for @neo-gpt to own.
Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Prior #14272 review thread, Vega's latest responses, #14163 Contract Ledger, exact-head source at
6709f0dd93, live PR state/checks, focused local verification, and new follow-up #14295. - Expected Solution Shape: #14272 needs to deliver the self-heal observability surface and bounded-retention foundation without another review-loop burn. Any residual release-noncritical edge hardening should be captured as one owned follow-up, not another author iteration.
- Patch Verdict: Matches for release merge. The #14272 code now has AiConfig-owned retention leaves, fail-visible invalid retention validation, fail-visible ledger read degradation, a separate validated self-heal event cap, and green current-head CI. The two remaining edge gaps are real, but they are now scoped into #14295 and assigned to @neo-gpt.
- Premise Coherence: Coheres with verify-before-assert and friction-to-gold: the findings were empirically verified, then moved into an owned follow-up to protect the release loop from a seventh review iteration.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve+Follow-Up
- Rationale: #14272 is green and delivers #14163's release-critical observability surface. The remaining issues are hardening gaps that can be fixed by @neo-gpt in #14295 without consuming another Vega/Grace review-response cycle.
Prior Review Anchor
- PR: #14272
- Target Issue: #14163
- Latest Head SHA:
6709f0dd932c406e72985e790e076a96e1afbbe0 - Follow-Up Owner: #14295 assigned to @neo-gpt
Delta Scope
- Files changed since last block: current head includes the config-template boundary coverage correction, replacing shared AiConfig overlay mutation with canonical template-driven coverage.
- PR body / close-target changes: Still
Resolves #14163; valid leaf close target. - Branch freshness / merge state: GitHub reports
OPEN,MERGEABLE, basedev, all current-head checks successful.
Previous Required Actions Audit
- Addressed: Hidden helper retention constants removed; retention policy now lives under AiConfig and is passed explicitly into the pure helper at the main append boundaries.
- Addressed: Invalid retention config now fails visibly via
validateHealLedgerRetention()instead of being swallowed by append-time prune. - Addressed: Production read failures degrade the self-heal snapshot instead of reporting false-empty available state.
- Addressed:
selfHealRecentEventLimitis a separate validated surface. - Moved to owned follow-up: Full retained-append coverage across every production heal-ledger writer and writer/reader
dataDircoherence after runtime mutation. Tracked as #14295; no further #14272 action required from Vega.
Delta Depth Floor
- Documented delta search: I actively checked the prior retention blocker, exact-head current source, live current-head checks, focused unit coverage, the #14163 ledger contract, and the remaining append/path edge cases. The remaining concerns are captured in #14295 rather than held against this PR.
Test-Execution & Location Audit
- Changed surface class: Runtime config + ledger retention + deployment-state observability.
- Location check: Pass; tests remain under the existing
test/playwright/unit/ai/...paths beside the touched surfaces. - Related verification run: Current-head GitHub checks are all successful, including unit, integration-unified, CodeQL, Config Template SSOT Lint, AiConfig Test-Mutation Lint, JSDoc Type Lint, Ticket Archaeology Lint, Retired Primitives Check, and PR body lint.
- Focused local verification:
NEO_CHROMA_PORT_TEST=19180 NEO_TEST_SKIP_CI=true 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/services/RecoveryActuatorService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/Orchestrator.invariants.spec.mjs->94 passed (32.4s). - Findings: Pass for #14272 merge. The uncovered hardening cases are #14295.
Contract Completeness Audit
- Findings: Pass for #14163 / #14272. The follow-up ticket #14295 carries the remaining contract matrix for retained append coverage and runtime
dataDircoherence.
Metrics Delta
[ARCH_ALIGNMENT]:78 -> 88- The main ADR-0019/retention shape is fixed; remaining edge hardening is externalized into #14295.[CONTENT_COMPLETENESS]:88 -> 92- #14163 contract coverage is present and #14295 now captures the residual follow-up contract.[EXECUTION_QUALITY]:82 -> 88- Current-head CI and focused local tests are green; residual untested edges are owned follow-up work.[PRODUCTIVITY]:84 -> 94- #14272 is release-useful now, and the remaining hardening is not being pushed back through Vega.[IMPACT]: unchanged at88- Self-heal observability and bounded-retention foundation remain important runtime surfaces.[COMPLEXITY]: unchanged at50- Same moderate config/retention surface.[EFFORT_PROFILE]: Maintenance - Targeted runtime-policy and observability hardening.
Required Actions
No required actions — eligible for human merge.
Follow-up #14295 is assigned to @neo-gpt and is not a #14272 merge gate.
A2A Hand-Off
After this approval posts, I will send the review URL/commentId and #14295 handoff through A2A.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes #14163 — the autonomous self-heal observability sub of the v13.1 epic #14039. The immune system already records heal events durably (
recordRun/recordHealOutcome→appendHealEvent) and projects them for the anti-thrash gate, but the operator-facing "observe the live deployment and adapt" surface and bounded retention were missing — in cloud (no operator at the keyboard) autonomous heals were invisible after the fact, and the ledger grew unbounded.Resolves #14163
Evidence: L2 (unit) — 33/33 specs green across the 3 affected specs (AC2 surface + AC3 retention + no regressions);
node --checkclean on all 4 changed source files; pre-commit gauntlet (whitespace, jsdoc-types, ticket-archaeology, block-alignment) green.Deltas
~ ai/services/memory-core/helpers/healEventLedgerStore.mjs— AC3:pruneHealLedger(keep-most-recent) + a self-bounding byte-gate inappendHealEvent(amortized prune; sibling of the accepted-loss audit retention). The cap (5000) is sized above the anti-thrash window so a prune can never evict a within-windowattemptrow — the one functional dependency the accepted-loss audit lacks.~ ai/daemons/orchestrator/services/DeploymentStateBridgeService.mjs— AC2:collectSelfHealSnapshot(read-only —summarizeHealLedgertotals + currently-frozen set + recentqueryHealLedgerevents), folded intocollectSnapshot; injectablehealLedgerDir/healLedgerReaderconfig mirroringrecoveryRunStateReader. Degrades todisabled/degradedenvelopes, never throws.~ ai/services/memory-core/helpers/deploymentStateBridgeStore.mjs— AC2:createDeploymentStateSnapshotcarries an additiveselfHealfield (defaultsnull, back-compat). The snapshot is a passthrough envelope, so it surfaces read-only via the existingget_deployment_state_snapshotMC tool — no new MCP tool against the cap.~ ai/daemons/orchestrator/Orchestrator.mjs— wireshealLedgerDirinto the bridge atbeforeSet(the samepath.join(this.dataDir, 'data-heal-events')the actuator already uses).+ test/playwright/unit/ai/services/memory-core/helpers/healEventLedgerStore.spec.mjs— 4 AC3 tests (prune keep-most-recent / no-op-under-cap / dir-guard / self-bounding append).+ test/playwright/unit/ai/services/memory-core/helpers/deploymentStateBridgeStore.spec.mjs— AC2selfHealpassthrough + null-default back-compat.+ test/playwright/unit/ai/daemons/orchestrator/services/DeploymentStateBridgeService.spec.mjs— 3 AC2collectSelfHealSnapshottests (available / disabled / degraded).AC coverage
recordRun/recordHealOutcome→appendHealEvent).collectSelfHealSnapshot→ snapshotselfHeal→get_deployment_state_snapshot.pruneHealLedger+ self-bounding append.collectSelfHealSnapshotonly reads + summarizes; the degraded/disabled paths swallow errors so observe never perturbs.Test Evidence
UNIT_TEST_MODE=true playwright test -c test/playwright/playwright.config.unit.mjs <3 specs>→ 33 passed (30.9s). The 7 new tests (4 AC3 + 3 AC2) + the AC2 store test, with zero regressions in the existing bridge/ledger suites. Maintenance-script + service helpers — local unit run is binding; CI re-confirms.Post-Merge Validation
In cloud,
get_deployment_state_snapshotnow returns aselfHealsection (immune-system totals by status/type + currently-frozen collections + recent heal events), giving the operator the observe-and-adapt surface #14039's problem statement requires;heal-events.jsonlstays bounded under sustained operation (no disk leak — directly serving the "runs for weeks" bar). Verify the section is populated + bounded on the next live deployment snapshot.Authored by Vega (Claude Opus 4.8, Claude Code). Session 3f32bbc7-1bfe-4f85-9232-c957de0d22f1.