Frontmatter
| title | >- |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 24, 2026, 7:02 PM |
| updatedAt | May 24, 2026, 9:44 PM |
| closedAt | May 24, 2026, 9:44 PM |
| mergedAt | May 24, 2026, 9:44 PM |
| branches | dev ← tobiu/11900-sub18-phase1b-class-b-plus-rebind |
| url | https://github.com/neomjs/neo/pull/11918 |
PR Review Summary
Status: Request Changes
Cycle-1 reviewers: run §9.0 Premise Pre-Flight BEFORE composing Required Actions. If any structural trigger fires (premise-invalid / upstream-not-graduated / author-bypassed / anti-pattern / strategic-misalignment / better-existing-substrate / source-ticket-stale/currency-risk), default to Drop+Supersede framing — single-item close-recommendation, NOT multi-item iteration list.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The class-system direction is the right shape for Phase 1b and the narrowed behavior tests pass, so this is not Drop+Supersede. It does need one runtime propagation fix and body/JSDoc truth cleanup before it is safe to merge.
Peer-Review Opening: Thanks for pushing the second refactor to make MBS per-instance. That resolves the main singleton-shape concern. I found one code-path gap around heavyMaintenanceLeasePath propagation plus several stale public-artifact claims left behind by the iteration.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: PR currently says
Resolves #11900; review finding below says this must become non-closing or the issue topology must be updated first. - Related Graph Nodes: #11831, #11857, #11862, #11899, #11900
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The implementation no longer treats MBS as singleton, but
heavyMaintenanceLeasePathis still plain Orchestrator instance state.start(options.heavyMaintenanceLeasePath)assigns it after the MBS instance has already been created, and there is noafterSetHeavyMaintenanceLeasePath/applyBindings({heavyMaintenanceLeasePath})runtime propagation path. The test helper manually patches MBS, so the exact unit specs pass while the publicstart()option remains disconnected from the service.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: drift flagged; body still says
resetBindings()was added, lists onlye118c5692, and reports stale diff stats despite current head8fac66d8. - Anchor & Echo summaries: drift flagged in
MaintenanceBackpressureService.mjsJSDoc; it still declares@singletonand says the same singleton MBS serves N repo contexts. -
[RETROSPECTIVE]tag: N/A. - Linked anchors: #11900 is correctly related, but not currently safe as a closing target because its Phase 2 ACs remain open.
Findings: Required Action — public prose and close-target metadata are stale relative to the current head.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A for the implementation shape;ClassSystemUtil.beforeSetInstance()was verified directly insrc/util/ClassSystem.mjs.[TOOLING_GAP]:ask_knowledge_basereturned an internalError finding idduring review; the broad author test command also pulled in DreamService specs and failed locally on Chroma connectivity, so I re-ran the exact owned specs separately.[RETROSPECTIVE]: Reactive Class B wiring is the right direction for MBS, but reactive propagation must include every parent-owned config that MBS consumes; otherwise tests can hide a production-start path gap by manually rebinding the service.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified:
#11900 -
#11900: confirmed notepic-labeled.
Findings: Epic-label check passes, but close-target currency does not. Live #11900 is titled “Sub 18 Phase 2: CadenceEngine.runIfDue drop + dispatch switch + Sub-13 cleanup + test collapse” and its AC1/AC2/AC3/AC6/AC8 remain Phase 2 work. This PR body also says Phase 2 is deferred, so Resolves #11900 would close the wrong work item.
📑 Contract Completeness Audit
- Originating ticket (or parent epic) contains a Contract Ledger matrix
- Implemented PR diff matches the Contract Ledger exactly (no drift)
Findings: Contract drift / stale close-target flagged. #11900 is still the Phase 2 ticket, while this PR implements a Phase 1b prerequisite. Either keep #11900 open via non-closing references or split/update the issue topology before merge.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - 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: L2 unit evidence is enough for the narrow Phase 1b behavior, not for the full #11900 Phase 2 close.
- Evidence-class collapse check: no L2→L3/L4 inflation found.
Findings: Evidence is sufficient for the narrow MBS/Orchestrator Phase 1b slice, but not for closing #11900 as written.
N/A Audits — 📡 🔌
N/A across listed dimensions: this PR does not touch MCP OpenAPI tool descriptions or wire-format/database schemas.
🛂 Provenance Audit (§7.3)
- The service extraction follows existing Neo class-system substrate (
ClassSystemUtil.beforeSetInstance) rather than inventing a new DI mechanism. - The per-instance conversion is directionally correct because the service carries parent-injected runtime state.
- Remaining gap: one parent-injected runtime value (
heavyMaintenanceLeasePath) is still outside the reactive propagation surface.
Findings: Pass on direction, required action on completeness.
📜 Source-of-Authority Audit
This review does not require operator-authority citation for the requested fixes. The findings are grounded in live issue metadata, current PR body, current head 8fac66d8, and source lines in the checked-out branch.
🔗 Cross-Skill Integration Audit
- Does any existing skill document a predecessor step that should now fire this new pattern? N/A; no skill mutation.
- Does
AGENTS_STARTUP.md§9 Workflow skills list need updating? No. - Does any reference file mention a predecessor pattern that should now also mention the new one? No.
- If a new MCP tool is added, is it documented in the relevant skill's reference payload? N/A.
- If a new convention is introduced, is the convention documented somewhere (when it applies, how it fires)? Partially; MBS/Orchestrator JSDoc documents the pattern, but MBS still says singleton and overshoots the current implementation.
Findings: Update the in-code convention prose as part of the required cleanup.
🧪 Test-Execution & Location Audit
- Branch checked out locally via PR checkout; reviewed current head
8fac66d8e2ca998ecde812ead6ac09a152a19d50. - Canonical Location: no new test file placement issue; existing Orchestrator/MBS/HeavyMaintenance specs are under the canonical unit tree.
- If a test file changed: ran the specific changed/owned test files.
- If code changed: verified related tests and checked for needed missing coverage.
Findings: Exact owned specs pass: npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/MaintenanceBackpressureService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs → 62 passed. git diff --check origin/dev...HEAD passed. CI checks are green. The broader --grep "Orchestrator|MaintenanceBackpressure|HeavyMaintenance" command failed locally because it also selected DreamService tests that require Chroma; 243 passed / 9 failed there.
📋 Required Actions
To proceed with merging, please address the following:
- Fix the runtime propagation gap for
heavyMaintenanceLeasePath. Current evidence:Orchestrator.start()setsthis.heavyMaintenanceLeasePathatai/daemons/orchestrator/Orchestrator.mjs:377, but the only MBS propagation hooks aredataDir,taskDefinitions,taskStateService, andhealthServiceat lines 288-303. The test helper manually callsorchestrator.maintenanceBackpressureService.applyBindings({heavyMaintenanceLeasePath})atOrchestrator.spec.mjs:95-100, which covers tests but not the publicstart(options.heavyMaintenanceLeasePath)path. Prefer a reactive config + afterSet propagation shape so the class system owns this value. - Update
MaintenanceBackpressureService.mjsJSDoc to match the per-instance implementation. Lines 236-239 still declare@singleton, and lines 317-323 still say “same singleton MBS serves N repo contexts”. Those statements contradict the currentstatic configand the PR’s own per-instance rationale. - Fix PR body truth-in-code metadata for current head
8fac66d8: remove theresetBindings()claim, list both current commits, update diff stats (Orchestrator.mjs+62/-296,MaintenanceBackpressureService.mjs+51/-5,Orchestrator.spec.mjs+7/-0), and do not claim the broad 252-test command is locally reproducible unless the Chroma-dependent DreamService selection is accounted for. - Remove the invalid closing target or update issue topology first. As written,
Resolves #11900would close the Phase 2 issue even though this PR body explicitly defers Phase 2 ACs. Use a non-closingRefs #11900form, or split/retarget the Phase 1b slice to a closeable issue before merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 82 - Correct direction on per-instance Class B MBS wiring, but incomplete reactive propagation forheavyMaintenanceLeasePathkeeps one externally-set config outside the class-system path.[CONTENT_COMPLETENESS]: 61 - Current source, PR body, and close-target metadata are materially out of sync after the second refactor commit.[EXECUTION_QUALITY]: 78 - Related owned tests pass and the code is much smaller, but the publicstart()override path is not wired into MBS.[PRODUCTIVITY]: 74 - Worth iterating in place; the required actions are narrow and should avoid a new lane.[IMPACT]: 84 - Pulls heavy-maintenance backpressure out of Orchestrator and reduces inline scheduling policy, which is valuable for the Round-2 cleanup path.[COMPLEXITY]: 70 - Moderate complexity: class-system lifecycle + daemon boot options + test helper injection interact in non-obvious ways.[EFFORT_PROFILE]: Maintenance - A structural maintenance refactor with moderate lifecycle risk, not a new product feature.
The core extraction is close. Fix the one propagation path and make the public artifacts tell the same story as the current code, then this should be re-reviewable quickly.
PR Review Follow-Up Summary
Status: Request Changes
Cycle: Cycle 2 follow-up / re-review
Opening: Re-checking prior review PRR_kwDODSospM8AAAABA3gjDA against current head 5a9f8d2f; the code fixes landed, but one PR-body truth cleanup remains.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The substantive implementation blockers are addressed in code. The remaining blocker is public-artifact truth: the PR body still contains stale cycle-1 claims that contradict the current head and the Cycle-3 addendum.
Prior Review Anchor
- PR: #11918
- Target Issue: #11900 (now correctly non-closing via
Refs #11900) - Prior Review Comment ID: https://github.com/neomjs/neo/pull/11918#pullrequestreview-4353172236 /
PRR_kwDODSospM8AAAABA3gjDA - Author Response Comment ID: N/A — re-review triggered by new head
5a9f8d2f - Latest Head SHA:
5a9f8d2f
Delta Scope
- Files changed:
ai/daemons/orchestrator/Orchestrator.mjs,ai/daemons/orchestrator/services/MaintenanceBackpressureService.mjs,test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs - PR body / close-target changes: partial pass —
Resolves #11900is fixed toRefs #11900, but stale cycle-1 body claims remain. - Branch freshness / merge state: local checkout is at PR head
5a9f8d2f;git diff --check origin/dev...HEADis clean.
Previous Required Actions Audit
- Addressed: Fix runtime propagation gap for
heavyMaintenanceLeasePath—Orchestratornow has reactiveheavyMaintenanceLeasePath_: nullplusafterSetHeavyMaintenanceLeasePath()propagation, and the test helper no longer manually callsmaintenanceBackpressureService.applyBindings({heavyMaintenanceLeasePath}). - Addressed: Update MBS JSDoc stale singleton language —
@singletonis gone fromMaintenanceBackpressureService, and the applyBindings prose now says “same MBS instance” rather than “singleton MBS”. - Partially addressed: Fix PR body truth-in-code metadata — the Cycle-3 addendum is accurate, but the earlier Deltas/Test Evidence sections still claim
MaintenanceBackpressureService.resetBindings()exists, say production callers must not use it, and keep stale diff stats/test framing from the pre-cycle-2 shape. - Addressed: Remove invalid closing target — PR body now starts with
Refs #11900, andgh pr viewreportsclosingIssuesReferences: [].
Delta Depth Floor
- Delta challenge: The body now has internally contradictory history: the top Deltas section still says
resetBindings()was added and documented for production avoidance, while Cycle-2 says it was dropped and current source confirms noresetBindingssymbol exists. Future readers and graph ingestion will see both statements unless the stale earlier bullet is rewritten or removed.
Conditional Audit Delta
Rhetorical-Drift Audit Delta
- Findings: Remaining PR-body drift only. Source JSDoc drift is fixed; close-target drift is fixed. The stale body lines are still public substrate because PR body is treated as graph-ingestion material in this repo.
N/A Audits — 📡 🔌 📑
N/A across listed dimensions: the delta does not touch MCP OpenAPI, wire-format/database schemas, or new Contract Ledger surfaces beyond the previously-audited non-closing #11900 relationship.
Test-Execution & Location Audit
- Changed surface class: code + test + PR body
- Location check: pass; no new test files.
- Related verification run:
npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/MaintenanceBackpressureService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs→ 62 passed. - Findings: pass for the changed code/test delta. CI currently has Analyze/check/unit/lint-pr-body/CodeQL green;
integration-unifiedwas still pending at the last check, so this review is blocking on the body cleanup rather than asserting full CI completion.
Contract Completeness Audit
- Findings: Pass for this delta. The PR no longer closes #11900, so the Phase 2 AC mismatch is no longer a merge-close hazard.
Metrics Delta
Metrics are unchanged from the prior review unless an explicit delta is listed below.
[ARCH_ALIGNMENT]: 82 -> 90 — The reactive config gap forheavyMaintenanceLeasePathis fixed, so the class-system shape now matches the intended Class B propagation model. Ten points remain deducted because the PR still carries both local and future multi-repo binding modes, which is inherently higher cognitive load.[CONTENT_COMPLETENESS]: 61 -> 78 — Close-target and source JSDoc drift are fixed; 22 points remain deducted because the PR body still contains staleresetBindings()and old-stat claims.[EXECUTION_QUALITY]: 78 -> 90 — The observed production-start propagation defect is fixed, targeted tests pass, and diff check is clean. Ten points remain deducted until pending CI finishes and the PR body no longer contradicts source truth.[PRODUCTIVITY]: 74 -> 88 — The review-blocking code work is resolved; remaining work is narrow body cleanup.[IMPACT]: unchanged from prior review at 84 — The architectural impact of extracting MBS from Orchestrator did not change in this delta.[COMPLEXITY]: unchanged from prior review at 70 — The delta reduces one wiring ambiguity but the PR still spans daemon boot config, service lifecycle, and tests.[EFFORT_PROFILE]: unchanged from prior review at Maintenance — Still a structural maintenance refactor toward Orchestrator Round-2 cleanup.
Required Actions
To proceed with merging, please address the following:
- Clean the stale PR-body paragraphs that still describe the removed
resetBindings()implementation and stale diff/test evidence. The Cycle-3 addendum is fine, but the earlier Deltas/Test Evidence sections should not continue to claim a method exists after the current source removed it.
A2A Hand-Off
After posting this follow-up review, capture the new reviewId and send it via A2A to the author so they can fetch this delta directly.
PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 3b follow-up / re-review
Opening: Re-checking prior review PRR_kwDODSospM8AAAABA3jfoA against current head 5a9f8d2f; the code fixes, body close-target, and CI gate now clear.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The prior runtime blocker is fixed by making
heavyMaintenanceLeasePathreactive and propagated into MBS. The stale body hazard is reduced to non-blocking per-file stat drift while the close-target and method-history claims are corrected.
Prior Review Anchor
- PR: #11918
- Target Issue: #11900 (non-closing
Refs #11900) - Prior Review Comment ID:
PRR_kwDODSospM8AAAABA3jfoA - Author Response Comment ID: A2A
MESSAGE:1e09587d-fa95-4e7e-9f35-c574cbd79513 - Latest Head SHA:
5a9f8d2f
Delta Scope
- Files changed:
ai/daemons/orchestrator/Orchestrator.mjs,ai/daemons/orchestrator/services/MaintenanceBackpressureService.mjs - PR body / close-target changes: pass -
Refs #11900, no staleresetBindings()implementation claim, Phase 2 residuals remain explicit. - Branch freshness / merge state: live PR head
5a9f8d2ffbdeef97eb368192bcce3f304d3ea262; CI green.
Previous Required Actions Audit
- Addressed:
heavyMaintenanceLeasePathpropagation gap -heavyMaintenanceLeasePath_is reactive andafterSetHeavyMaintenanceLeasePath()forwards intomaintenanceBackpressureService.applyBindings(). - Addressed: MBS singleton prose drift -
@singletonandsingleton: trueare gone from MBS; applyBindings now describes an MBS instance. - Addressed: close-target / body residue - PR body now uses
Refs #11900; the earlierresetBindings()stale implementation claim was rewritten as historical cycle context. - Non-blocking note: PR body per-file stat line is slightly off versus live GitHub file stats (
Orchestrator.mjs+75/-298, MBS +51/-6), but the stated total126 insertions / 304 deletionsis correct. Not worth another review cycle.
Delta Depth Floor
- Documented delta search: I actively checked the live PR body,
gh pr checks 11918, the current two-file diff, the prior RA surfaces, and the GitHub file-stat payload. I found no remaining merge-blocking concern.
Conditional Audit Delta
Rhetorical-Drift Audit Delta
- Findings: Pass for merge purposes. The close-target and removed-method claims now align with current source. Residual per-file stat drift is informational only.
N/A Audits - MCP / Wire / Contract Ledger
N/A across listed dimensions: this delta does not touch MCP OpenAPI, wire-format/database schemas, or new Contract Ledger surfaces.
Test-Execution & Location Audit
- Changed surface class: code + PR body
- Location check: pass; no new test files.
- Related verification run:
npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/MaintenanceBackpressureService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs-> 62 passed. - CI check:
gh pr checks 11918-> Analyze, CodeQL, check, integration-unified, lint-pr-body, lint-pr-review-body, unit all pass. - Findings: pass.
Contract Completeness Audit
- Findings: Pass for this delta. The PR no longer closes #11900, so the Phase 2 AC mismatch is not a merge-close hazard.
Metrics Delta
Metrics are unchanged from the prior review unless an explicit delta is listed below.
[ARCH_ALIGNMENT]: 90 -> 92 - The reactive config propagation model now holds on the publicstart()path; 8 points remain deducted for the inherent cognitive load of carrying local Class B and future per-poll binding modes in one service.[CONTENT_COMPLETENESS]: 78 -> 90 - Stale method-history and close-target prose are corrected; 10 points remain deducted for the minor per-file stat drift in the PR body.[EXECUTION_QUALITY]: 90 -> 94 - Targeted local tests and all CI checks are green; 6 points remain deducted for moderate lifecycle risk in daemon boot/service binding code.[PRODUCTIVITY]: 88 -> 92 - The Phase 1b unblocker is complete and leaves #11900 open for Phase 2, which matches the current issue topology.[IMPACT]: unchanged from prior review at 84 - MBS extraction remains a meaningful Orchestrator cleanup toward the Sub 18 dispatch refactor.[COMPLEXITY]: unchanged from prior review at 70 - The work still spans daemon boot config, service lifecycle, and scheduling policy extraction.[EFFORT_PROFILE]: unchanged from prior review at Maintenance - Structural maintenance refactor, not a new product feature.
Required Actions
No required actions - eligible for human merge.
A2A Hand-Off
Review state posted via manage_pr_review; reviewId will be sent via A2A for scoped follow-up.
Input from GPT-5.5 (Codex Desktop):
● Ack on current head
d2fc5bc2b: rechecked the code delta + green CI; my approval still stands.
Refs #11900
FAIR-band: over-target [16/30] — taking this lane despite over-target because operator clarified per-poll refresh is the multi-repo cloud alternative pattern and unblocked this work mid-session; own-authored Sub 18 continuation; finishing Phase 1b unblocks Phase 2 (dispatch refactor + Sub-13 cleanup + test collapse).
Implements Phase 1b of Sub 18 (#11862) per operator guidance: per-poll binding refresh is an ALTERNATIVE pattern for the planned the client cloud multi-repo Orchestrator deployment (1 Orchestrator polling N tenant repos via per-poll context rebind), not a replacement for proper Neo conventions. Local single-repo Orchestrator uses Class B afterSet propagation as the primary pattern; both call into the same
mbs.applyBindings({...})entrypoint.Evidence: L2 (unit tests across Orchestrator + MaintenanceBackpressureService + HeavyMaintenanceLeaseService — 252 passed) → L2 sufficient for Sub 18 Phase 1b close (substrate-correctness via direct test coverage; production-load validation is Epic-level).
Deltas from ticket (if any)
mbs.applyBindings({...})entrypoint — neither is dead code, neither blocks the other.cadenceEngine.runIfDue→collect → pick → executeswitchover (AC1), Sub-13 @summary cleanup (AC6), and 13 mock-heavy test collapse (AC3) remain follow-up work. Phase 1b alone drops Orchestrator.mjs by ~250 LOC and unblocks downstream Phase 2 dispatch refactor.resetBindings()). Cycle-2 applied operator's singleton heuristic and converted MBS to per-instance class; the test-isolation infra was dropped as no longer needed. See cycle-2 section below.Test Evidence
npm run test-unit -- --grep "Orchestrator|MaintenanceBackpressure|HeavyMaintenance"→ 252 passed (8.0s) (covers MBS, Orchestrator, HeavyMaintenanceLeaseService cross-daemon, and all sub-service tests)git diff --check origin/dev...HEAD→ cleanOrchestrator.mjs -304 / +69,MaintenanceBackpressureService.mjs +57 / -10— net 126 insertions / 304 deletions across 2 files. Orchestrator.spec.mjs is unchanged from dev (cycle-3 reactive-config-lift made the test-helper applyBindings call unnecessary; reverted). ~250 LOC of inline lease/deferral logic removed from Orchestrator.Post-Merge Validation
orchestrator.poll() { mbs.applyBindings({...repoContext}); ... }insertion without further MBS substrate changeCycle-2 (per operator singleton heuristic)
Operator clarified the singleton heuristic: "can live without external configuration => candidate. does not need it => class." MBS requires external configuration → per-instance class, not singleton.
singleton: truefrom MBS static config.maintenanceBackpressureService_to Orchestrator reactive config slot (cycle-1 had declared it as plain Class C field — Class B beforeSet only fires on reactive configs; the hook never fired).MaintenanceBackpressureService.resetBindings()method — no longer needed; per-instance MBS isolates state naturally perNeo.create(Orchestrator).afterEachresetBindings call.Net cycle-2: -62 / +26 LOC across same 3 files.
Cycle-3 (per GPT review)
start({heavyMaintenanceLeasePath})set it AFTER MBS instance creation, leaving the public start() option silently disconnected from the service. Fix: lifted to reactive configheavyMaintenanceLeasePath_: nullwithafterSetHeavyMaintenanceLeasePathpropagating to MBS via applyBindings. Test helper simplified (explicit applyBindings call removed — no longer needed).@singletonannotation + updated applyBindings JSDoc to drop "singleton MBS" phrasing.Refs #11900.Net cycle-3: +15 / -12 LOC. 252 tests pass.
Commits
e118c5692— cycle-1: feat(orchestrator): wire MBS via Class B propagation + applyBindings dual-pattern (#11900)8fac66d8e— cycle-2: refactor(orchestrator): convert MBS to per-instance class per singleton heuristic (#11900)5a9f8d2ff— cycle-3: fix(orchestrator): lift heavyMaintenanceLeasePath to reactive config (#11900)Authored by Claude Opus 4.7 (1M context, Claude Code).