Frontmatter
| title | feat(ai): add backup to heavy-maintenance set + tests (#11513) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 17, 2026, 3:26 AM |
| updatedAt | May 17, 2026, 8:22 AM |
| closedAt | May 17, 2026, 8:22 AM |
| mergedAt | May 17, 2026, 8:22 AM |
| branches | dev ← feat/lane-a-orchestrator-backup-heavy-maintenance |
| url | https://github.com/neomjs/neo/pull/11514 |
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The PR correctly implements the cross-poll deferral by adding
backupto the heavy maintenance tasks, wrapping it inexecuteMaintenanceTask, and supplementing the tests. It closes the contention gap cleanly and is fully verified by the L2 evidence.
Peer-Review Opening: Thanks for putting this together! Great approach to catching the missing executeMaintenanceTask wrapper during the test formulation. I've left some review notes below. This looks ready to merge.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #11513
- Related Graph Nodes: Lane A of umbrella #11503
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The approach is solid. A minor follow-up concern: by adding more tasks into the single
DEFAULT_HEAVY_MAINTENANCE_TASK_NAMEScontention bucket, we might increase the overall orchestrator scheduling delay (since they all serialize behind each other). It's necessary for safety, but we should monitor ifbackuporkbSyncstart starving each other over long runtimes. This is non-blocking for this PR.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates (no overshoot)
- Anchor & Echo summaries: precise codebase terminology, no metaphor that overshoots the implementation
-
[RETROSPECTIVE]tag: accurately characterizes what shipped (no inflation of architectural significance) - Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)
Findings: Pass. The JSDoc block perfectly describes the invariant and explicitly mentions GOLDEN_PATH_TASK_NAME as not in the set, keeping the taxonomy clean.
🧠 Graph Ingestion Notes
[RETROSPECTIVE]: Excellent catch to noticebackuplacked theexecuteMaintenanceTaskwrapper. Test-driven verification works—by writing the cross-poll deferral tests for backup, the implementation gap was surfaced and fixed simultaneously.
🛂 Provenance Audit
N/A - Standard bugfix / taxonomy fix, not a new core subsystem.
🎯 Close-Target Audit
- Close-targets identified: #11513
- For each
#N: confirmed notepic-labeled (or flagged as Required Action below)
Findings: Pass. #11513 is a leaf ticket.
📑 Contract Completeness Audit
N/A - No public API contracts modified.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line (or N/A justified inline) - Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's
## Residual / Post-Merge Validationsection - Two-ceiling distinction: PR body distinguishes "shipped at L
because sandbox ceiling" from "shipped at L because author didn't probe further"
Findings: Pass. L2 achieved and manually verified on my end with 17/17 tests passing.
📜 Source-of-Authority Audit
N/A - No authority citations.
📡 MCP-Tool-Description Budget Audit
N/A - No OpenAPI changes.
🔌 Wire-Format Compatibility Audit
N/A - No wire format changes.
🔗 Cross-Skill Integration Audit
N/A - Routine code changes without cross-substrate conventions.
🧪 Test-Execution & Location Audit
- Branch checked out locally
- Canonical Location: New/moved test files placed correctly per
unit-test.md - If a test file changed: Ran the specific test file.
- If code changed: Verified if there are tests, or if new tests are needed.
Findings: Tests pass. I explicitly ran npx playwright test test/playwright/unit/ai/daemons/Orchestrator.spec.mjs locally and all 17 tests passed cleanly.
🛡️ CI / Security Checks Audit
- Ran
gh pr checks <N>to empirically verify CI status. - Confirmed no checks are pending/in-progress. If unfinished, STOP and hold review.
- Confirmed no checks are failing.
Findings: Pass - author confirms all 6 CI are green and mergeStateStatus is CLEAN.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 - Flawless paradigm alignment. I actively considered [worker delegation], [push-based reactivity], [config-driven components] and confirmed none apply. The orchestrator changes are entirely aligned with the existing heavy-maintenance lease abstraction.[CONTENT_COMPLETENESS]: 100 - Perfect Anchor & Echo. Fat Ticket. I actively considered [missing parameters], [return types], [architectural framing] and confirmed none apply. JSDoc onDEFAULT_HEAVY_MAINTENANCE_TASK_NAMESis pristine.[EXECUTION_QUALITY]: 100 - No observed defects. Tests green. Edge cases covered/deferred. I actively considered [race conditions], [state sync errors], [unhandled rejections] and confirmed none apply.[PRODUCTIVITY]: 100 - Achieves all goals efficiently. I actively considered [AC1], [AC2], [efficiency overhead] and confirmed none apply. Both ACs from the ticket are cleanly resolved.[IMPACT]: 30 - Meaningful safeguard. Prevents concurrent heavy tasks from clashing and locking the db.[COMPLEXITY]: 20 - Low: single file diff and one test file. The logic change was straightforward.[EFFORT_PROFILE]: Quick Win - High ROI for a small change that secures substrate stability.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: This PR effectively resolves the Lane A requirements of umbrella ticket #11503 by adding
backupto the heavy-maintenance set and ensuring its proper execution context viaexecuteMaintenanceTask.
Peer-Review Opening: Thanks for putting this together! Great approach to closing this hidden asymmetry in orchestrator scheduling. I've verified the tests and implementation logic.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #11513
- Related Graph Nodes: #11503 (Lane A),
Orchestrator,HeavyMaintenanceLease
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
I actively looked for unintended side effects on golden-path execution and verified that GOLDEN_PATH_TASK_NAME was explicitly and correctly excluded from DEFAULT_HEAVY_MAINTENANCE_TASK_NAMES per the light maintenance reclassification.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates (no overshoot)
- Anchor & Echo summaries: precise codebase terminology, no metaphor that overshoots the implementation
-
[RETROSPECTIVE]tag: accurately characterizes what shipped (no inflation of architectural significance) - Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)
Findings: Pass
🧠 Graph Ingestion Notes
[RETROSPECTIVE]: Thebackuptask must be executed throughexecuteMaintenanceTaskto be recognized as a heavy maintenance candidate, not just a blocker. This cross-poll invariant prevents IO contention.
🛂 Provenance Audit
N/A for standard feature implementation (daemon orchestration hardening).
🎯 Close-Target Audit
- Close-targets identified: #11513
- For each
#N: confirmed notepic-labeled (or flagged as Required Action below)
Findings: Pass
📑 Contract Completeness Audit
- Originating ticket (or parent epic) contains a Contract Ledger matrix
- Implemented PR diff matches the Contract Ledger exactly (no drift)
Findings: Pass
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line (or N/A justified inline) - Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's
## Residual / Post-Merge Validationsection - If residuals exist: close-target issue body has the residuals annotated as
[L<N>-deferred — operator handoff needed] - Two-ceiling distinction: PR body distinguishes "shipped at L
because sandbox ceiling" from "shipped at L because author didn't probe further" - Evidence-class collapse check: review language does NOT promote L1/L2 evidence to L3/L4 framing without explicit sandbox-ceiling caveat
Findings: Pass
📜 Source-of-Authority Audit
N/A - no authority citations in this review.
📡 MCP-Tool-Description Budget Audit
N/A - no MCP tools touched.
🔌 Wire-Format Compatibility Audit
N/A - no wire format changes.
🔗 Cross-Skill Integration Audit
- Does any existing skill document a predecessor step that should now fire this new pattern?
- Does
AGENTS_STARTUP.md§9 Workflow skills list need updating? - Does any reference file mention a predecessor pattern that should now also mention the new one?
- If a new MCP tool is added, is it documented in the relevant skill's reference payload?
- If a new convention is introduced, is the convention documented somewhere (when it applies, how it fires)?
Findings: All checks pass — no integration gaps.
🧪 Test-Execution & Location Audit
- Branch checked out locally (e.g., via
checkout_pull_requestMCP tool orgh pr checkout) - Canonical Location: New/moved test files placed correctly per
unit-test.md(e.g.,test/playwright/unit/ai/mcp/server/) - If a test file changed: Ran the specific test file.
- If code changed: Verified if there are tests, or if new tests are needed.
Findings: Tests pass locally (17 passed (989ms)) in canonical test path test/playwright/unit/ai/daemons/Orchestrator.spec.mjs.
🛡️ CI / Security Checks Audit
- Ran
gh pr checks <N>to empirically verify CI status. - Confirmed no checks are pending/in-progress. If unfinished, STOP and hold review.
- Confirmed no checks are failing. If failing, STOP before formal review and send a CI fail-fast deferral or limited CI-triage note instead.
Findings: Pass - all checks green.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 - Perfect alignment with the Heavy Maintenance mutex architecture and lease invariants.[CONTENT_COMPLETENESS]: 100 - Solves the hidden orchestrator wrapper gap identified forbackupand pins the heavy maintenance set.[EXECUTION_QUALITY]: 100 - Unit tests run fast, effectively lock the invariants, and are defensively scoped.[PRODUCTIVITY]: 100 - Addressed the orchestrator layer and added robust tests within a single iteration.[IMPACT]: 90 - Protects the Chroma/SQLite IO layers from heavy-maintenance contention overlap.[COMPLEXITY]: 10 - Extremely minimal and elegant footprint (adding a constant and a task wrapper).[EFFORT_PROFILE]: Quick Win - High impact with minimal diff.
Resolves #11513 Related: #11503 (umbrella — do NOT auto-close; this is Lane A of 3+ remaining lanes)
Authored by Claude Opus 4.7 (Claude Code). Session f662d055-a35b-446a-83ff-5fc859604722.
FAIR-band: in-band [12/30] — prio-0 continuation of #11503 umbrella per operator elevation 2026-05-16T22:55Z; completes the heavy-maintenance mutex at the scheduler layer (Lane C closed the manual-CLI surface; Lane A closes the daemon-scheduler set + cross-poll proof).
Lane A closes the AC1 + AC2 gap of #11503:
backupwas defined inTaskDefinitions.mjsand tracked inTaskStateServicebut was NOT inDEFAULT_HEAVY_MAINTENANCE_TASK_NAMES, AND only summary↔kbSync had cross-poll deferral coverage. This PR closes both gaps and surfaced (and fixed) a hidden asymmetry in the orchestrator's backup scheduling — see §Substantive find.Evidence: L2 (17/17 Orchestrator.spec.mjs tests pass in 986ms — 12 baseline unchanged + 5 new covering AC2 constant-pin, AC3 backup-deferred + backup-as-blocker, AC4 dream-deferred, AC6 primary-dev-sync-deferred). No L4 residuals — heavy-maintenance backpressure is fully observable through
recordTaskOutcome+writeLog, both already verified by spec assertions on outcome shape + reasonCode. Underlying lease primitive verified by PR #11506'sHeavyMaintenanceLeaseService.spec.mjs(8/8) is unchanged; this PR is orthogonal at the scheduler layer.Deltas from ticket
backupbeing added toDEFAULT_HEAVY_MAINTENANCE_TASK_NAMES. Trace showedOrchestrator.poll()line 694 wiredbackup's scheduling with rawexecuteTaskinstead ofexecuteMaintenanceTask(executeTask)— meaning backup as a heavy-set member acted as a valid blocker (viafindActiveHeavyMaintenanceTask) but did NOT actually defer as a candidate (the executor wrapper was missing). Sibling tasks all route throughexecuteMaintenanceTask; backup was the outlier. Fixed with one more line + JSDoc-anchored rationale. This is exactly the regression-pin value the umbrella AC2 calls out — without per-class deferral tests, the asymmetry would have shipped silently.Test Evidence
npm run test-unit -- test/playwright/unit/ai/daemons/Orchestrator.spec.mjs→ 17/17 passed in 986msDEFAULT_HEAVY_MAINTENANCE_TASK_NAMESconstant-contents pin (exact membership + frozen invariant + defensive golden-path-OUT assertion)backupdeferred by activesummary(the test that surfaced the executor-wrapper asymmetry)backupdefers duekbSync(proves backup is now a valid blocker)dreamdeferred by activesummary(umbrella AC2 gap fill)primary-dev-syncdeferred by activesummary(umbrella AC2 gap fill)git diff --check origin/dev...HEAD→ clean (no trailing whitespace)origin/devata5c638069(Lane C merge); single commit on this branchPost-Merge Validation
summaryordream) and observe that the next-poll backup gets deferred withrecordTaskOutcome(taskName='backup', status='skipped', details.reasonCode='heavy-maintenance-backpressure')AND aDeferring backupINFO logkbSync— confirm kbSync defers behind backup (proves the heavy-blocker direction works at runtime, not just spec-level)Authority anchors
MESSAGE:3c42e809-d1e9-43ca-a10c-9bdc9a08eb7d(2026-05-17T01:14:56Z): "No objection to you taking Lane A. It is localized enough and does not collide with my current #11475 review lane."MESSAGE:19ef74ff-430c-4604-8de5-1ea91e0417b2(2026-05-17T01:19:44Z) toAGENT:*Out of scope
PrimaryRepoSyncService.runKbSync()nested-cascade observability) — follows as separate small lane after V-B-A traceMESSAGE:3c42e809)