PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The patch is the correct graph-only fix for a telemetry marker that cannot synchronously consult Chroma. It closes the leaf ACs for #13667 with narrow SQL and targeted unit coverage. The only follow-up is commit/provenance hygiene: future commits for carved leaf tickets should prefer the delivered leaf id over the broader parent id.

Peer-Review Opening: Peer review of PR #13668. The implementation stays inside the graph-backed marker contract, avoids adding cross-store I/O to getDueTask, and uses the existing archivedAt convention to stop non-drainable orphan sessions from inflating the pending proxy.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #13667, PR body/conversation, changed-file list, current dev source for ai/daemons/orchestrator/scheduling/summary.mjs, SessionSummaryBacklogCount.spec.mjs, KB query on the session-summary marker, targeted Memory Core prior-art queries, close-target labels, commit log, exact head 2420b7ad68daa96355d81ab58bff7d13740704b4, local focused test output, and current GitHub checks.
• Expected Solution Shape: A correct change should keep the marker graph-only and fail-soft, exclude fully archived memory sessions from the AGENT_MEMORY session set, and preserve mixed sessions where at least one unarchived memory remains. It should NOT query Chroma or gate the sweep on cross-store truth; test isolation should use the real :memory: graph under UNIT_TEST_MODE with fixture nodes proving archived-only, live, and mixed behavior.
• Patch Verdict: Matches. The diff adds archivedAt IS NULL inside the memory_sessions CTE and adds the missing archived/live/mixed regression test without changing the scheduler trigger contract.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13667
• Related Graph Nodes: Related: #13647, #13624, #13656, #13639; concepts: session-summary-pending marker, orphaned memory sessions, archivedAt, periodic sweep telemetry

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: the PR body and title correctly resolve #13667, but the single commit subject still references broader parent #13647. That is not an auto-close hazard and does not block this PR, because the PR body has the correct leaf close target and commit body carries no closing keyword. It is worth tightening as a future authoring habit: once a carved leaf exists, the commit subject should point at the leaf that is being delivered.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the telemetry-only / graph-only framing matches the implementation.
• Anchor & Echo summaries: the modified summary comment explains archived sessions as non-drainable marker inflation without claiming Chroma proof at query time.
• [RETROSPECTIVE] tag: N/A, none present.
• Linked anchors: #13667 establishes the leaf ACs; #13647/#13624/#13656/#13639 are related context, not overclaimed close targets.

Findings: Pass, with the non-blocking commit-subject provenance note above.

🧠 Graph Ingestion Notes

• [KB_GAP]: No author gap. KB results described the broader Memory Core session-summary model but did not surface this newest graph-marker leaf; review grounding came from the live issue, source, and tests.
• [TOOLING_GAP]: None blocking. CI was pending during first review pass; formal review waited until current-head unit and integration-unified were terminal green.
• [RETROSPECTIVE]: This is the right layer for the marker: archive no-content rows elsewhere, then make the graph-only telemetry ignore fully archived sessions while still counting mixed sessions.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #13667
• #13667 labels verified live: enhancement, ai, model-experience; no epic label.
• Commit log checked: no Resolves / Closes / Fixes magic close keyword in commit body. The commit subject references #13647 descriptively/provenance-wise, not as a close target.

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: N/A — this PR changes internal telemetry SQL/test behavior, not a public or consumed API contract.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is sufficient for the close-target ACs: graph-query behavior covered by unit tests against the real in-memory graph surface.
• Evidence language stays at L2 and does not claim live Chroma/orchestrator proof.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

For every modified or added OpenAPI tool description:

• Single-line preferred — block-literal (|) descriptions justified by content, not authorial habit
• No internal cross-refs
• No architectural narrative
• External standard URLs OK
• 1024-char hard cap respected

Findings: N/A — no OpenAPI/MCP tool description surface touched.

🔗 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?

Findings: N/A — no skill, workflow convention, MCP surface, or new cross-substrate primitive is introduced.

🧪 Test-Execution & Location Audit

• Branch checked out locally in /Users/Shared/codex/neomjs/neo/tmp/review-13668-gpt at exact head 2420b7ad68daa96355d81ab58bff7d13740704b4.
• Canonical Location: new/modified unit coverage remains under test/playwright/unit/ai/daemons/orchestrator/scheduling/.
• Ran the specific related test files from the PR body.
• Current GitHub checks verified green after bounded watch.

Findings: Tests pass — local npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/scheduling/SessionSummaryBacklogCount.spec.mjs test/playwright/unit/ai/daemons/orchestrator/scheduling/summary.spec.mjs -> 24/24 passed; GitHub current-head checks are green.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 - Preserves the graph-only scheduler marker contract and uses the existing archivedAt graph convention instead of adding Chroma I/O.
• [CONTENT_COMPLETENESS]: 90 - PR body and test evidence are complete; 10 deducted for the non-blocking commit-subject provenance mismatch to parent #13647 instead of leaf #13667.
• [EXECUTION_QUALITY]: 100 - Exact-head local focused tests passed, GitHub CI is green, and the new regression covers archived-only, live, and mixed-session semantics.
• [PRODUCTIVITY]: 100 - Delivers both #13667 ACs exactly and leaves the parent #13647 residuals properly out of scope.
• [IMPACT]: 75 - Small SQL/test change, but it corrects misleading operational telemetry for the Memory Core drift loop.
• [COMPLEXITY]: 25 - One CTE filter plus one focused regression test; low code complexity with meaningful graph-state nuance.
• [EFFORT_PROFILE]: Quick Win - High-value telemetry correction with a narrow, well-tested patch.

Approved. Merge remains human-only.