PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This is a narrow, high-ROI observability fix for the summary backlog lane. It preserves the existing scheduler trigger shape, keeps the limited fetch as the cheap existence check, and only changes the logged reason count to use an uncapped scheduler-side count.

Peer-Review Opening: Reviewed #12810 against #12809, the refreshed origin/dev scheduler baseline, the exact-head diff, close-target labels, CI state, and the related orchestrator scheduling test suite.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12809 issue body and ACs; changed-file list; refreshed origin/dev versions of ai/daemons/orchestrator/scheduling/memorySummaryBackfill.mjs and ai/daemons/orchestrator/scheduling/summary.mjs; PR conversation/body; exact-head commit messages; close-target labels; CI state; local related test execution.
• Expected Solution Shape: Correct shape is a scheduler-local, fail-soft uncapped count paired with the existing limited fetch, so the child process still owns actual draining and the trigger output shape does not become a new API. This must not hardcode live operator backlog assumptions or turn the scheduler into the worker; test isolation should cover both pure trigger builders and the getDueTask seams.
• Patch Verdict: Matches the expected shape. The diff adds separate getPendingMemorySummaryBackfillCount() / getPendingSummarizationCount() helpers, threads totalPending only when limited pending rows exist, preserves pendingCount as the fetched-count compatibility field, and adds four tests proving 50 fetched / true depth > 50 logs the true backlog count.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12809
• Related Graph Nodes: #12740, #12746, #12804, summary backlog observability, orchestrator scheduling

🔬 Depth Floor

Challenge: The new count helpers are not directly unit-tested for the SQL/catch path; the tests cover the public scheduler behavior through totalPending and getDueTask seams. I treated this as non-blocking because the SQL predicates match the existing limited fetch predicates, both helpers are fail-soft by construction, and the actual user-visible contract is the logged trigger reason, which is covered for both schedulers.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; "true backlog depth" is implemented as an uncapped scheduler snapshot count, not claimed as worker progress.
• Anchor & Echo summaries: new JSDoc uses concrete scheduler/count/logging terminology.
• [RETROSPECTIVE] tag: N/A — no retrospective tag in the PR body.
• Linked anchors: #12740 / #12746 / #12804 are scope context only, not borrowed authority for a new pattern.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None found in the patch. The scheduling contract was already discoverable in the touched files and tests.
• [TOOLING_GAP]: ask_knowledge_base timed out during review despite a healthy KB healthcheck; review proceeded from live issue/PR state, refreshed origin/dev, exact diff, and local focused tests.
• [RETROSPECTIVE]: The useful pattern here is preserving a cheap capped existence check while adding a separate uncapped observability count; this avoids turning scheduler selection into worker execution while fixing operator backlog blindness.

🎯 Close-Target Audit

• Close-targets identified: Resolves #12809 in the PR body.
• #12809 labels checked via GitHub API: enhancement, ai; not epic.
• Exact-head commit messages checked; no stale Closes / Fixes / extra Resolves close-target hazard.

Findings: Pass.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: this PR does not introduce a public/consumed contract ledger surface, observable host-only evidence AC, MCP OpenAPI tool description, new workflow convention, wire format, or turn-loaded substrate.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request; exact head 0bbe39adae401e2faa8e2f4c46d35c0883ee02bc.
• Canonical Location: modified tests remain under test/playwright/unit/ai/daemons/orchestrator/scheduling/, matching right-hemisphere unit-test placement.
• Related verification run: npm run test-unit -- "orchestrator/scheduling" → 116/116 passed locally.
• CI state verified before review: GitHub lint-pr-body, CodeQL, retired-primitives check, test classification, unit, and integration-unified all SUCCESS; merge state CLEAN.

Findings: Tests pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - 5 points deducted for a small race/read-skew caveat inherent in doing a separate count after the limited fetch; acceptable because the value is only an operator-facing trigger reason and remains fail-soft.
• [CONTENT_COMPLETENESS]: 100 - I actively checked new helper JSDoc, updated parameter docs, PR body scope notes, and close-target metadata; none of the usual missing-doc / unclear-scope deductions apply.
• [EXECUTION_QUALITY]: 90 - 10 points deducted because the new count helpers are not directly tested against mock SQL/catch behavior; the shipped scheduler behavior is still verified by focused tests, local execution, and green CI.
• [PRODUCTIVITY]: 100 - The PR directly satisfies #12809's core goal: replace fetch-limit visibility with true-depth backlog visibility while preserving output shape.
• [IMPACT]: 70 - Solid operational impact: fixes prio-0 backlog blindness for summary maintenance, but it is localized scheduling observability rather than a new subsystem.
• [COMPLEXITY]: 35 - Low-to-moderate: two scheduler modules plus two specs, simple SQL/count threading, no wire-format or runtime worker rewrite.
• [EFFORT_PROFILE]: Quick Win - High operator value with a small, contained scheduler change and focused regression coverage.

Approved. The non-blocking direct-helper-test gap is worth remembering, but it is not enough to hold this PR back given the preserved contract and verified behavior.