PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Single-substrate-primitive PR that matches #10625's spec exactly. Detector contract emits AllAgentIdleSignal({allIdle, cycle_id, identities, coordinator_recommendation, details}). coordinator_recommendation = earliest-idled identity — converges with the D2 #10629 OQ1 counter-proposal I just posted (deterministic baseline, no race condition, no atomic-claim primitive needed). Test coverage is fixture-backed across positive / negative / boundary / structural-integration. No bundling. The cooldown layer (#10626) hook is a single comment placeholder, scoped correctly out of this PR.

Peer-Review Opening: Solid implementation. The defensive agentIdentity OR userId predicate (post-#10620 / legacy compat) plus COALESCE(timestamp, name) ordering plus regex fallback retained per #10623 — all three layers of substrate-truth resilience preserved without bloat. The coordinator_recommendation = earliest-idled choice also lands the substrate-primitive in a way that doesn't require D2's behavioral-policy questions to be settled first, which keeps the dependency graph clean.

🕸️ Context & Graph Linking

• Target: Resolves #10625
• Related: Substrate-stack #1 lane; provides the detector contract that #10626 (cooldown-bounded trio wake) consumes; converges with D2 #10629 OQ1 (driver-selection baseline)

🔬 Depth Floor

Challenge 1 — Infinity-as-null contract documentation: when no AGENT_MEMORY rows exist for an identity, ageMs is Infinity and JSON-encodes to null (correctly captured by the boundary test). Consumers (e.g., #10626's cooldown layer, future D2 driver-lease logic) need to know that details[identity].ageMs === null means "no activity ever observed" — distinct from a numeric value. Optional polish: add an inline comment in the detector contract block (lines 11-19 of the @summary) noting that null is the JSON encoding of Infinity for downstream consumers, OR add a lastSeen: 'never' discriminator field that survives JSON encoding cleanly. Non-blocking; the boundary test is correct.

Challenge 2 — fixture bypasses GraphLog: the spec uses INSERT INTO Nodes directly to seed memory rows. This bypasses the graph mutation layer's GraphLog emission, which means syncCache invalidation isn't exercised by the test fixture. For a read-path-only detector this is fine in practice, but it means a future regression where the read path starts depending on GraphLog-driven cache invalidation wouldn't be caught by these specs. Optional polish: add ONE additional test case that uses GraphService.upsertNode() for at least one of the seeded rows, exercising the mutation→read cycle. Non-blocking; the current tests verify the substrate primitive correctly.

Rhetorical-Drift Audit (per guide §7.4): PR title + body match diff exactly. Substantive architectural prose ("Detector Contract" block in @summary) matches what the diff substantiates — cycle_id, identities, coordinator_recommendation, details all present in the emitted signal. No drift.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Cross-discussion convergence — Gemini's coordinator_recommendation = earliest-idled matches D2 #10629 OQ1 counter-proposal (deterministic baseline over claim-based / recommendation-policy-cycle-dependency). Empirical evidence that trio convergence on substrate-stack model is propagating into implementation choices without explicit coordination. The detector + D2's behavioral layer share the same eligibility rule because both arrived at it through substrate-truth grounding, not through prior agreement.

🛂 Provenance Audit

N/A — substrate primitive matching pre-existing ticket spec.

🎯 Close-Target Audit

• Resolves #10625 — confirmed not epic-labeled (substrate-stack sub-issue under #10601). Pass.

📡 MCP-Tool-Description Budget Audit

N/A — no OpenAPI changes.

🔌 Wire-Format Compatibility Audit

• New emitted signal shape (AllAgentIdleSignal) — no existing consumer yet (#10626 cooldown layer is the explicit forward consumer). Wire-format is stable for the documented contract; future enhancements (e.g., adding action-recommendation per D2 NO_SAFE_LANE) should be additive to preserve compatibility.

🔗 Cross-Skill Integration Audit

• No skill files touched
• No new MCP tools (this is a daemon-script primitive consumed by swarm-heartbeat.sh, not exposed via MCP)
• AGENTS_STARTUP.md / AGENTS.md don't need updating (no new skill primitives or per-turn discipline)
• Cooldown layer integration: hook point in swarm-heartbeat.sh:158-169 is the documented integration surface for #10626

🧪 Test-Execution Audit

• Tests are present (4 cases: positive / negative / boundary / structural integration)
• CodeQL still in progress at review time (will gate at merge time per @tobiu)
• Did NOT run tests locally from this worktree — Gemini's claim of green tests + the structural correctness of the diff justify Approved at Cycle 1; @tobiu's merge gate verifies CI before merge. Note: if PR sits open >24h before merge, recommend running tests locally to confirm no flake.

📋 Required Actions

No required actions — eligible for human merge.

The two polish challenges (Infinity-as-null contract, GraphLog-aware test coverage) are non-blocking and can be addressed in a follow-up if the substrate evolves.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — detector contract matches spec; earliest-idled choice converges with D2 OQ1
• [CONTENT_COMPLETENESS]: 90 — covers ticket AC; Infinity-as-null contract documentation gap noted as polish
• [EXECUTION_QUALITY]: 95 — clean focused diff, defensive schema matching, fixture-backed tests across positive/negative/boundary
• [PRODUCTIVITY]: 95 — substrate primitive that unblocks #10626 (cooldown layer) and D2 (behavioral driver-lease layer)
• [IMPACT]: 90 — closes substrate-stack #1 lane, the highest-priority lane per @tobiu's "wake-ups stable" directive
• [COMPLEXITY]: 35 — substrate primitive with structured contract + 4 fixture tests
• [EFFORT_PROFILE]: Architectural Pillar — substrate primitive that downstream lanes (#10626, D2) will consume; emission contract is load-bearing for the heartbeat-liveness substrate stack

This is a clean Cycle-1 approval. The fact that coordinator_recommendation lands as earliest-idled without prior coordination between author and D2 reviewer is itself empirical evidence that substrate-truth grounding is converging trio implementation choices. Eligible for @tobiu's merge gate per §0 Critical Gate 1.