PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The PR shape is right and the scheduler wiring is clean, but the new core count query has a SQLite NOT IN null-sensitivity that can silently zero the backlog count. Since this PR exists to remove a blind spot, that query needs to be made null-safe before merge.

Peer-Review Opening: This is the correct lane for #12821: count the ordinary unsummarized-session backlog only on the periodic sweep path and append it to the existing reason string. One SQL edge still blocks approval.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12821 body and ACs; #12822 PR body; live PR state/head dd868a1a04fbc667d8618c57fbcafd4e512aaeb5; refreshed origin/dev after #12820 merge; changed-file list; current ai/daemons/orchestrator/scheduling/summary.mjs; sibling memorySummaryBackfill.mjs; SessionService.mjs SESSION_SUMMARY writer; focused scheduling tests.
• Expected Solution Shape: The scheduler should append an uncapped session-summary backlog count only when the periodic summary sweep is the selected trigger, without changing handover or explicit SummarizationJobs priority. The count helper should be fail-soft and robust against graph rows outside the happy path. Tests should cover trigger wiring and the count helper’s actual distinct-session semantics.
• Patch Verdict: Mostly matches. The trigger selection and once-per-sweep count seam match the expected shape, but the count helper uses NOT IN (SELECT json_extract(...)), which SQLite invalidates when the subquery contains a null session id.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12821
• Related Graph Nodes: #12810, #12819, #12820, #12817, orchestrator/scheduling/summary, AGENT_MEMORY, SESSION_SUMMARY

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: getPendingSessionSummaryCount() currently depends on memorySessionId NOT IN (summarySessionIds). In SQLite, if the subquery returns a single NULL, every NOT IN comparison becomes unknown and the query returns no pending sessions. I verified this with an in-memory SQLite falsifier: with sessions s1 unsummarized and s2 summarized, the helper returned 1; after adding one SESSION_SUMMARY node with missing properties.sessionId, the helper returned 0.

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/source-code snapshot overshoot
• [RETROSPECTIVE] tag: N/A
• Linked anchors: sibling references establish the backlog-count pattern

Findings: Required Action. The PR says the helper counts sessions with AGENT_MEMORY turns but no SESSION_SUMMARY; the current SQL can report 0 in the presence of a malformed/legacy summary row instead of counting the unsummarized sessions.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: ask_knowledge_base timed out and the Memory Core self-review detection query returned QUERY_PATH_DEGRADED; I proceeded from live GitHub, exact-head checkout, source reads, local SQL isolation, and focused tests.
• [RETROSPECTIVE]: Observability counters need the same paranoia as write paths. A backlog-count query that fails silently to zero is worse than no counter because it tells the operator the system is clean.

🎯 Close-Target Audit

• Close-targets identified: #12821
• For each #N: confirmed not epic-labeled. #12821 is labeled ai + enhancement.

Findings: Pass.

📑 Contract Completeness Audit

Findings: N/A — internal scheduler reason string is additive and not an MCP/API contract surface.

🪜 Evidence Audit

Findings: Pass with one blocking code/test gap below. CI and focused tests cover scheduler reason wiring and fail-soft behavior; my local SQL isolation found the uncovered null-subquery failure mode in the new helper.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no OpenAPI/MCP tool description surfaces changed.

🔗 Cross-Skill Integration Audit

Findings: N/A — no skill files, MCP tool surfaces, or always-loaded agent substrate changed.

🧪 Test-Execution & Location Audit

• Branch checked out locally: claude/12821-session-summary-backlog-log at dd868a1a04fbc667d8618c57fbcafd4e512aaeb5.
• Canonical Location: scheduler test remains under test/playwright/unit/ai/daemons/orchestrator/scheduling/.
• If a test file changed: ran the specific test file.
• If code changed: verified focused coverage and ran syntax/diff checks.

Findings: Tests pass, but they do not cover the helper’s distinct-session success path against a malformed/null summary row.

Local evidence:

• gh pr checks 12822 — all current checks pass.
• node --check ai/daemons/orchestrator/scheduling/summary.mjs — pass.
• node --check test/playwright/unit/ai/daemons/orchestrator/scheduling/summary.spec.mjs — pass.
• git diff --check origin/dev...HEAD — pass.
• npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/scheduling/summary.spec.mjs — 16/16 passed.
• Manual SQLite falsifier for getPendingSessionSummaryCount() — returned 1 before a null summary row, then 0 after adding a SESSION_SUMMARY with no sessionId.

📋 Required Actions

To proceed with merging, please address the following:

• Make getPendingSessionSummaryCount() null-safe and add a deterministic helper test. Prefer NOT EXISTS correlated on summarySessionId = memorySessionId, or at minimum filter nulls out of the summary-side subquery. Add a test that seeds duplicate AGENT_MEMORY rows for one unsummarized session, one summarized session, and one malformed/legacy SESSION_SUMMARY row with no properties.sessionId; the count should still return the distinct unsummarized-session count, not 0.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - 10 points deducted because the scheduler integration follows the existing pure-function/test-seam pattern, but the graph SQL needs a null-safe anti-join for durable graph data.
• [CONTENT_COMPLETENESS]: 88 - 12 points deducted because the helper JSDoc and PR prose describe the intended count accurately, but the tests do not yet prove the count helper’s success semantics.
• [EXECUTION_QUALITY]: 78 - 22 points deducted because the focused suite is green but the manual SQL falsifier found a real silent-zero edge in the new helper.
• [PRODUCTIVITY]: 80 - 20 points deducted because the PR mostly satisfies #12821, but the primary observability counter can hide a backlog under a reachable graph-data edge.
• [IMPACT]: 70 - This is a targeted v13 observability improvement: it does not change summarization behavior, but it makes backlog drain visible to operators.
• [COMPLEXITY]: 35 - Low-to-medium: one scheduler helper, one reason-string extension, and deterministic unit coverage; the subtlety is SQL truth-table behavior.
• [EFFORT_PROFILE]: Quick Win - A small SQL/test adjustment should close the gap without changing the PR’s scope.

The fix should be surgical. Do not broaden this into backlog draining or #12820 behavior; just make the anti-join null-safe and prove the helper count.