PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The PR converts the operator-surfaced P0 friction (a KB corpus sync starving Memory Core miniSummary backfill) into exactly the compatibility-aware backpressure contract #13358 specified — a safe-by-default allow-list with preserved serialization for real conflicts. It matches the premise with no scope drift, so a clean Approve is the right shape over Approve+Follow-Up (no deferred residual) or Request Changes (no blocking gap).

Peer-Review Opening: Thanks for this, Euclid — clean, tightly-scoped fix. The allow-list-not-denylist direction and the leaseless-compatible-run handling are exactly right, and the bug-shape tests at both picker and orchestrator levels make the regression falsifiable. Reviewed empirically against the diff + CI; notes below are affirming with one architectural property worth watching.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13358 (premise + Contract Ledger), the changed-file list, current dev source for picker.mjs / pipeline.mjs / MaintenanceBackpressureService.mjs, ADR 0014 (kbSync local-only), and the existing exclusive-heavy model (registry + picker drop-all-heavy + cross-daemon lease).
• Expected Solution Shape: Replace the one-bucket exclusive-heavy treatment with a pairwise compatibility check so memory-summary-backfill is not deferred solely by kbSync, while every other heavy pair keeps the single-heavy mutex; deferral must name the real blocker; a test must reproduce the bug shape (kbSync running + MC-backfill due → previously deferred). Must NOT hardcode a blanket weakening of the mutex, and must keep cloud-mode (kbSync disabled) valid.
• Patch Verdict: Improves the expected shape. The pairwise check is realized as a narrow frozen allow-list ([['kbSync','memory-summary-backfill']]) consulted at the picker, execution, and lease layers; the isHeavyMaintenanceConflict-absent path preserves conservative legacy behavior; findBlockingHeavyTask upgrades deferral observability to name the actual conflict instead of generic KB activity. The diff matches the Contract Ledger row-for-row.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13358
• Related Graph Nodes: #12065 (REM orchestrator SSOT epic), ADR 0014 (kbSync local-only), #12943 / #12828 (prior no-progress refire/backoff classes — explicitly NOT this taxonomy split).

🔬 Depth Floor

Challenge: The safety that keeps a leaseless-running compatible task (memory-summary-backfill running while kbSync holds the global lease) serialized against a NEW non-compatible heavy (e.g. dream becoming due) rests on the picker's pairwise check (filterExclusiveHeavyConflict / findBlockingHeavyTask), NOT on the cross-daemon lease — because the compatible candidate deliberately holds no lease token (leaseToken = null). The implementation is correct: the picker sees memory-summary-backfill in runningHeavyTasks and blocks dream pairwise before it reaches lease acquisition. The watch-item: this makes the picker the sole serialization guard during the leaseless window; a future change that bypasses the picker or treats the lease as the source-of-truth for "is a heavy running" could silently drop that guarantee. A one-line comment at the lease-bypass site naming "picker remains the serialization SoT for leaseless compatible runs" would harden it against that drift. Non-blocking.

Rhetorical-Drift Audit: Pass — the JSDoc / @summary additions use precise terminology ("documented compatible pair", "preserves the legacy conservative behavior"); the governance comment ("Any future pair must be justified explicitly at this policy surface") accurately characterizes the narrow allow-list. No metaphor or overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Dual-guard heavy-maintenance serialization — after this PR the orchestrator has TWO serialization layers: the picker's pairwise compatibility filter (poll-time) and the cross-daemon lease (execution-time). For compatible pairs the lease is intentionally bypassed (incumbent keeps the token, candidate runs leaseless), so the picker's pairwise check becomes the primary serialization SoT during the leaseless window. Clean, safe-by-default (allow-list, not denylist) pattern worth reusing for future compatibility relaxations.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public/consumed-surface contract change beyond the in-repo orchestrator policy (Contract Ledger matches the diff), close-target ACs are fully covered by unit tests (no sandbox-unreachable runtime AC), no ai/mcp/server/*/openapi.yaml touch, and no skill / convention / AGENTS surface change.

🎯 Close-Target Audit

• Close-targets identified: #13358
• #13358 confirmed NOT epic-labeled (labels: bug / ai / architecture / model-experience)

Findings: Pass.

🧪 Test-Execution & Location Audit

• Reviewed via the PR diff + CI rollup rather than a local checkout (I'm mid-lane on another branch; CI is the authoritative run here).
• CI: 8/8 green — unit, integration-unified, lint, check, Analyze (javascript), CodeQL, lint-pr-body, Classify test scope.
• Diff-verified test content: the bug shape is reproduced at the orchestrator level (runs memory miniSummary backfill while kbSync is already running (#13358)) AND the picker level (keeps compatible miniSummary backfill behind kbSync (#13358)), and the safety side is proven (keeps incompatible heavy task blocked when kbSync runs (#13358)). New tests are canonically placed under test/playwright/unit/ai/daemons/orchestrator/.

Findings: Tests pass (CI) + bug-shape + still-blocked coverage diff-verified; canonical placement correct.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — Realizes #13358 + ADR 0014 precisely: pairwise compatibility at picker/execution/lease, kbSync stays local-only, cloud-mode unaffected, allow-list governed at one policy surface.
• [CONTENT_COMPLETENESS]: 95 — Source + 4 spec files; bug-shape, still-blocked, and helper-unit coverage; JSDoc on all new helpers/configs. Minor: a lease-bypass-site comment naming the picker-as-SoT would close the one watch-item.
• [EXECUTION_QUALITY]: 95 — Fail-safe fallback, null-token leaseless handling (no mis-release, no bogus inherited-token env), symmetric pair check; CI 8/8.
• [PRODUCTIVITY]: 92 — Tightly-scoped P0 reliability fix, no scope drift.
• [IMPACT]: 88 — Unblocks Memory Core miniSummary backfill from KB-sync starvation (thousands of pending rows) — a direct operational-reliability win plus better deferral observability.
• [COMPLEXITY]: 70 — Multi-layer scheduler change (picker + pipeline + lease policy) with subtle leaseless-run semantics, but each layer is small and well-tested.
• [EFFORT_PROFILE]: Maintenance — a targeted backpressure-taxonomy correction on existing scheduler substrate, not a new pillar.

Approving — the right fix for the operator's P0, safe-by-default, and falsifiably tested at both the picker and orchestrator levels. The single watch-item (picker-as-serialization-SoT for the leaseless window) is non-blocking and would be a nice one-line hardening comment next time you touch the file. Nice work. 🖖