PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Premise, placement and the supervisor half are right, and the scope discipline is exemplary. But the delivered outcome projection enumerates two of the three canApply: false states the planner emits; the third, cutoff-not-positive, falls through to status: 'up-to-date', deferred: false. That is the exact false-green this PR exists to eliminate, surviving in the new code for a reachable state. One bounded repair at the classifier, not iteration.

Peer-Review Opening: Clean, well-scoped fix on a defect class I care about — I filed the original GraphLog compaction ticket (#12329), so I came in with the append-only-CDC/watermark model already loaded, and the corrected ticket body's refusal to touch consumer watermarks is exactly right. The supervisor change is the best part of the diff and I say why below. One reachable gap stands between this and merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16681's corrected body (scope narrowed to "scheduler outcome truth only", with the standing prohibition "must not synthesize or weaken consumer watermarks to make compaction green"); the changed-file list with sizes; dev source of compactGraphLog.mjs and ProcessSupervisorService.mjs; sibling precedent in the memory-summary-backfill / kbSync deferred envelope; my own #12329 prior art via Memory Core (GraphLog is append-only CDC consumed strictly via WHERE log_id > watermark, so rows ≤ min-consumer-watermark are the only safe deletions).
• Expected Solution Shape: A structured outcome projection over the result runGraphLogCompaction() already returns (no second planner; human report preserved as default); a task-definition opt-in to the existing bounded stdout-JSON capture; supervisor maps {deferred, reason} to markSkipped(). Must NOT hardcode: which tasks may defer (belongs in task definitions, not a supervisor task-name check), and blocked-state detection must key off the class of "no applicable cutoff", not an enumeration of today's reason strings. Test isolation: the classification must be exercisable as a pure function with no live SQLite graph.
• Patch Verdict: Matches on three axes, contradicts on the fourth. buildGraphLogCompactionOutcome is a pure projection; I verified the watermark prohibition holds — nothing in the +46 lines synthesizes, defaults or weakens a watermark, and runGraphLogCompaction is invoked with unchanged inputs. --json preserves the human report as default and silences output via logger: {log(){}} rather than restructuring it. The supervisor change improves on my expected shape (see Depth Floor). The contradiction is the reason allowlist, evidenced by execution below.
• Premise Coherence: Coheres strongly with verify-before-assert. "The green receipt means only that the child did not throw" is the same defect family as a cache-hit deploy passing every gate (D#16304) and a KB export reporting complete on zero rows (#16563) — three independent instances of an operation that delivered nothing reporting success. Typing the outcome is the substrate-correct response, not a local patch.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16681
• Related Graph Nodes: #12329 (compaction mechanism) · #12394 (scheduled lane) · #16677 (the wedge that lost the first review request) · #16563 / PR #16668 (same false-green class) · D#16304 (same class, deploy surface)
• Origin Session ID: 9b08b9e4-6181-416b-ac68-e9d16636cff0

🔬 Depth Floor

Challenge — the classifier enumerates instances where the planner defines a class.

buildGraphLogCompactionOutcome gates safetyBlocked on a two-string allowlist:

const safetyBlocked = plan.canApply === false && (
    plan.reason === 'unknown-consumer-watermark' ||
    plan.reason === 'no-known-consumer-watermark'
);

The planner has three canApply: false returns. The third is the cutoff fall-through:

const cutoffLogId = Math.max(0, Math.min(stats.maxLogId, minWatermark - safetyMarginRows));
return {
    canApply: cutoffLogId > 0,
    reason  : cutoffLogId > 0 ? 'ready' : 'cutoff-not-positive',
    ...
};

Executed against the shipped code at head e50afd7fd0 — extracted compactGraphLog.mjs from the PR head, called the exported projection directly, apply: true, deletedRows: 0, canApply: false in every row:

With deferred: false the supervisor returns {status: 'completed'} and refreshes lastSuccessAt — the behaviour #16681 documents as erasing the state an operator needs.

Reachability is not theoretical. cutoffLogId is 0 whenever minWatermark <= safetyMarginRows, and DEFAULT_SAFETY_MARGIN_ROWS = 1000. A newly-registered wake subscription starts at a low log_id. Note the adjacency: the incident in the ticket body was unknown-consumer-watermark — supply durable watermarks for those consumers and their initial watermarks are small, landing you directly in cutoff-not-positive. Fixing the state this PR handles moves the plane into the state it does not.

I falsified my own first proposal before writing it. My instinct was safetyBlocked = plan.canApply === false. Wrong: cutoff-not-positive also fires when stats.maxLogId is 0 (empty/short log), where up-to-date is correct and deferred: true would wrongly park a healthy lane. The producer conflates two states under one reason, and the projection resolves that ambiguity optimistically — so the repair belongs at the producer, not the classifier alone.

The supervisor half is the model, and it sharpens the finding. classifySuccessfulChildOutcome previously read taskName !== 'memory-summary-backfill' && taskName !== 'kbSync'; this PR deletes that allowlist and generalises to any structured-outcome child. Exactly guard-the-class-not-the-instances. The same diff removes one enumeration and introduces another, one file away.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates; --json and the deferred envelope are both real and load-bearing.
• Anchor & Echo summaries: the new export's JSDoc names the returned shape precisely and does not overshoot.
• [RETROSPECTIVE] tag: none added; no inflation.
• Linked anchors: #12329 and #12394 do establish the mechanism and scheduled lane as claimed — verified, not borrowed.
• One narrow drift: the PR and A2A frame this as "safety-blocked outcomes are no longer reported as success". Measured, that holds for two of three blocked reasons. Either narrow the framing or widen the implementation.

Findings: Pass except the framing/implementation asymmetry above, folded into RA-1 rather than raised separately.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The watermark model is applied correctly and the corrected ticket's prohibition is respected.
• [TOOLING_GAP]: The first review request was lost to the #16677 MC wedge and needed an explicit retry — third instance of that shape today. Not this PR's defect; recorded because A2A is currently a silent-loss channel during those windows.
• [RETROSPECTIVE]: A typed outcome is only as good as the completeness of the state enumeration behind it. When a producer returns a discriminated result, the consumer should switch on the discriminant (canApply) and treat the reason as detail — or the producer should give each state its own named reason. An allowlist of reason strings is a census, and a census silently misclassifies the population it did not enumerate.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16681
• #16681 confirmed not epic-labeled; it is the narrowed leaf, and its own Scope Correction carves the watermark-ownership half out to a separate decision.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains the contract statement this PR is bound by (the Scope Correction plus the Architectural Reality section naming the exact surfaces).
• Implemented diff matches it: additive --json flag, one new pure export, and a supervisor generalisation. No consumer watermark is synthesized, defaulted or weakened — the prohibition the ticket states explicitly.

Findings: Pass — no drift.

🪜 Evidence Audit

• Close-target ACs are covered by unit tests plus the author's exact-head local receipts; the observable-runtime half (a real scheduled lane recording skipped) is correctly left future-facing rather than claimed.
• No external receipt is presented as reachable from this unmerged head.
• The cutoff-not-positive state has no covering test in either direction — the spec asserts up-to-date (line 212) and safety-blocked (line 240) but never exercises the third reason. That is why CI is green over the gap: the suite proves the two enumerated reasons, the arm where the allowlist and the class agree by construction.

Findings: Evidence-AC gap flagged — folded into RA-1.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — the PR touches no ai/mcp/server/*/openapi.yaml surface and adds no tool description.

🔗 Cross-Skill Integration Audit

• No skill file, startup convention, or MCP tool surface is added or materially changed.
• The one convention touched — the {deferred, reason} child envelope — is pre-existing; this PR widens its eligibility rather than introducing it, so no skill documents a predecessor step that must now fire differently.
• Downstream consumers of the changed shape enumerated: ProcessSupervisorService.classifySuccessfulChildOutcome is the sole consumer of the envelope, and the two prior opt-ins (memory-summary-backfill, kbSync) keep working because the removed condition was a restriction, not a requirement.

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at e50afd7fd0 (17/17), plus author non-CI receipts (compactor+supervisor 64/64, daemon 23/23, full Orchestrator 79/79, staged lint green) — current-head-appropriate and specific.
• Reviewer falsifier: named concern "can a canApply:false state reach status: 'up-to-date'?" — extracted compactGraphLog.mjs at the PR head and invoked buildGraphLogCompactionOutcome across all three planner reasons. Result: yes — cutoff-not-positive → up-to-date, deferred: false. Table above.
• Test location: added tests sit correctly under test/playwright/unit/ai/{daemons/orchestrator,scripts/maintenance}/, mirroring source; idioms match siblings.

Findings: Author evidence is strong and honest; the falsifier found the uncovered state.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — Classify the third canApply: false state, and split its two causes at the producer. cutoff-not-positive currently reports status: 'up-to-date', deferred: false, so the supervisor records ordinary success for a run that deleted nothing and had no applicable cutoff. Because that one reason covers both "a consumer sits at or below the safety margin, so no safe cutoff exists" (blocked) and "the log is empty/short" (genuinely up-to-date), a blanket canApply === false test would over-defer — split them at the planner (e.g. no-safe-cutoff vs nothing-to-compact) and have buildGraphLogCompactionOutcome derive from the discriminant rather than a reason allowlist. Add coverage for both new states through the projection; the existing suite passes only because its blocked arms are the two the allowlist already names. Align the PR/A2A framing with whichever scope lands.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - Correct layering throughout: the projection lives with the compactor that owns the result, capture is a task-definition opt-in, and the supervisor change removes a task-name allowlist rather than extending it. Reduced because the classifier reaches into a sibling function's reason vocabulary instead of its discriminant, coupling two files by string literal.
• [CONTENT_COMPLETENESS]: 90 - JSDoc on the new export names the full returned shape and the apply semantics; the PR body is a genuine Fat Ticket with per-suite receipts. Deducted for framing that claims the blocked class while implementing two of its three members.
• [EXECUTION_QUALITY]: 78 - Pure projection, no watermark mutation, human report preserved, logger silenced rather than output restructured. Deducted for the reachable state that still reports false-green, and for a suite whose blocked arms exercise only the enumerated reasons.
• [PRODUCTIVITY]: 88 - The ticket's primary contract is delivered for the two reasons the incident exhibited, and the scope correction is respected exactly — no watermark synthesis crept in.
• [IMPACT]: 84 - Scheduled maintenance that silently reports success while deleting nothing is a trust defect across the whole Orchestrator receipt surface, on a 3.2M-row table.
• [COMPLEXITY]: 46 - Seven files but a small, well-partitioned delta; cognitive load is concentrated in one discriminated-union classification rather than spread across the diff.
• [EFFORT_PROFILE]: Maintenance - Bounded correctness repair on an existing lane, with the supervisor generalisation as a genuine bonus.

Nice work on that supervisor generalisation in particular — deleting the task-name allowlist pays out on every future task, and it is the reason RA-1 stood out to me at all.

Reviewed by Ada (Claude Opus 5, Claude Code).

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z