docs(observability): standalone portable observability design (#1237)

[itsmiso-ai]

Closes #1237.

Design-only deliverable. Adds docs/observability.md, a standalone, backend-neutral observability design for Dispatch, plus a one-line README pointer.

What's in the document

• Signal/state catalogue — gauges/counters/histograms with units, source table/column, bounded (allowlisted) dimensions, timestamp/reset semantics, and explicit unknown states, covering: work supply by status/lane, assignment vs active execution, issue leases, AgentWork lifecycle, PR-fix hand-out/ack/reclaim, PR-fix settlement, admission/dependency withholding, grooming freshness, sync/scheduler freshness, and failure outcomes.
• Dashboard view specification — backend-neutral state predicates; explicitly separates settled/closed work, awaiting review, stale claims, blocked dependencies, genuine human decisions (BLOCKED), and operational exhaustion (PARKED). Notes that an empty PR-fix listing does not prove there is no assigned issue work.
• Alert catalogue — 13 alerts with condition, an explicit Enabled wiring flag, severity, for, rationale, and recovery/operator action, covering stale ownership, failure to hand out eligible work, unavailable upstream evidence, and normal inactivity (which must not alert).
• Integration matrix — optional Prometheus/VictoriaMetrics pull exporter, OTel push metrics/logs, snapshot API, and warehouse approaches; Grafana and Alertmanager are not assumed.
• Healthy / idle / degraded examples for a non-Courier consumer.
• Current instrumentation inventory and unknowns (no /metrics/OTel today; free-string dimensions need allowlisting; exporter choice deferred).

Boundaries honored

Design only: no implementation, requeues, claim release, worker credential changes, dashboard/rule assets, deployments, or home-ops configuration. No dependency on Courier telemetry, phases, dashboards, cadence, or shared identifiers. No arbitrary repo/task IDs, credentials, or error strings as metric labels.

Revision history

Initial revision (b1494b2)

An independent reviewer checked factual accuracy against source and acceptance coverage. One major finding (an internal contradiction between the A4 alert condition and the §8.3 degraded example, plus an undefined "scheduler healthy" predicate) and several minor/editorial findings were fixed: added dispatch_scheduler_healthy, made §8.3 consistent, added the stale GroomingRun status, tightened the settlement signal source, corrected the lease orphan citation, and added runType/notes to the inventory.

Second revision (2684f72)

Three review findings from the human change-request on the first revision were addressed in the alert contract: A4's counter/gauge semantics (no increase() on windowed DB gauges; delta()/_in_window convention stated in §4.1), A4's sync-failure suppression moved into the predicate, and A7 made age-tied via dispatch_pr_fix_overdue_unacked with a steady-churn no-alert example (§8.5).

Third revision (734ce30) — addressing the second review round

Both open review threads are resolved, each claim verified against current source before acting:

• [P1] IssueSyncRun has no success status. The only persisted values are running (src/lib/sync-lock.ts:160), completed (src/app/api/sync/route.ts:67, src/app/api/sync/scheduled/route.ts:151), failed (scheduled/route.ts:227). §4.9 signals now match "completed"/"failed" with the enum documented and cited. Never-successful-first-sync baseline: A2 gains an explicit absent-series OR branch (absence must fire; time() - <no data> > x yields no data, not true). The absent series also keeps A4's fresh-sync guard unsatisfied, routing to A2/A13. Bonus finding: the manual-sync error path (sync/route.ts:80-82) leaves rows running forever without persisting failed — documented as a coverage gap with A2 freshness as backstop.
• [P1] dispatch_agent_runs_started_in_window cannot observe pickups. AgentRun rows land at tasks/report finish (route.ts:571) with the retrospective optional startedAt. Its blind spot is documented and it no longer fires A4. A4 now gates on: dispatch_execution_active == 0 (unexpired worker Lease or CLAIMED/IN_PROGRESS AgentWork; BLOCKED excluded — active but not flowing), dispatch_work_pickups_in_window == 0 (new AgentWork.createdAt at agent-work/start, new Lease.createdAt at claim; renewals excluded), and dispatch_agent_runs_reported_in_window == 0 (added after independent review found the canonical next-task → report loop writes no pickup row at all). New §8.6 pins the in-flight-without-report no-alert case; the residual blind spot (claim-less worker mid-single-task is unobservable; A4's firing there is a truthful "Dispatch cannot observe this fleet" warning) is stated in A4 and tied to the §10 acknowledgement-gap question.
• System-emitter filter (§4.3, from independent review). Lease/AgentRun rows are also written by non-task emitters: hosted groomer leases (groomer/run.ts:250-254) and runType: "groom" runs, the heartbeat route's runType: "heartbeat" rows (heartbeat/route.ts:100-104), and the legacy agent-runs ingestion route. All pickup/execution/reported signals exclude them via an exporter-side task-run-type allowlist (implement/followup-pr) — a subtract-groom filter leaked heartbeats and would have let a polling-only fleet disarm A4. Agent identity stays out of metric labels (§9 unchanged).

Fourth revision (6117985) — addressing the third review round

Both new P2 threads fixed via the remedies each one allowed; every source claim re-verified against current code:

• [P2] A6 aggregate false-fire. dispatch_assigned_without_execution > 0 for 30m measures an aggregate that stays continuously nonzero under healthy assignment churn (overlapping sub-minute waits), so it fires on healthy fleets. Verified there is no per-item age source to qualify it with: Issue has no assignedAt (prisma/schema.prisma:21-100); updatedAt is overwritten from GitHub's updated_at on every sync (src/lib/issue-sync.ts:115) and lastSyncedAt is bumped by claims/webhooks (src/lib/issue-claim.ts:83,106,163, src/app/api/issues/webhook/route.ts:164); only actions-route assignments write assign_agent audit rows (src/app/api/issues/actions/route.ts:14; claims audit as claim_issue but materialize a lease and fall outside the predicate). Per the reviewer's future-gap branch: A6 is pinned as "must NOT fire on the aggregate" with the intended age-qualified predicate (open, eligible issue; agent/*; no active lease/AgentWork; waiting > T) spelled out; the §4.3 gauge is marked dashboard-only; new §8.7 steady-churn no-alert example; the §6 churn bullet, gate sentence, and a §10 age-source decision (sync-diff timestamp vs audit join, with retention caveat — verified no AuditLog pruning exists today) updated in lockstep. Execution-loss detection stays with A5, pickup starvation with A4/A10.
• [P2] A12 delta() over a windowed gauge. delta() of an already-windowed rolling count is not the event count in the window — a 15m failure gauge can read 1 at both ends while an old failure expires and a new one arrives (delta == 0, failure missed). A12 now evaluates dispatch_audit_failures_in_window{window="900s"} > 0 directly; the signal replaces dispatch_audit_failures_total (§4.10, source AuditLog.success = false AND createdAt >= now() - window, windows allowlisted 900s/3600s). §4.1 states the general rule — increase()/rate()/delta() are counter/cumulative-only, with the rotation false-negative spelled out and _total-suffixed rows declared historical windowed counts that must not fire alerts through rate-style evaluators — and §4.10/§7 row A/§5 row 10/the shared-window sentence are aligned to it.

Fifth revision (f4b2c2e, this head) — making the catalogue's invariants checkable

The review of 6117985 withheld approval for a requirement-traceability gap: the alert catalogue, the IssueSyncRun.status enum, and the "nothing happened must not alert" separation were asserted only in prose, with no enforcement location and no test evidence. For a design-only deliverable the honest fix is an executable check over the document itself — not production code that does not exist yet.

• Enabled column in §6 — the wiring contract. yes means a consumer may evaluate the predicate and map it to a rule; no means the row documents a known gap and must not be wired into an evaluator. A6 is the only disabled row. It previously carried a live for: 30m while being pinned, which made "defined but disabled" indistinguishable from "defined with correct logic" without reading the prose; its for cell is now — (T applies only when promoted). Its condition text, severity (info (pinned; not firing)), the §4.3 dashboard-only pin, the §6 churn bullet, §8.7, and the §10 graduation path are unchanged and still mutually consistent.
• New docs/observability.test.ts (198 lines, 6 checks, run by npm run test) enforces: A1–A13 present exactly once with required columns and non-empty required cells; every row declares its enabled state, exactly A6 is disabled, and a disabled row carries no for duration, severity info, and a condition that says "must NOT fire"; no alert condition evaluates anything with increase()/rate()/delta() (§4.1); every dispatch_* signal named in an alert condition is defined in §4 (52 defined, 18 referenced); every file:line citation resolves to an existing file and an existing line (36 citations across 18 files); and the documented IssueSyncRun.status values equal exactly the set the three writer files write ({running, completed, failed}). Every check carries a cardinality guard so it cannot pass vacuously if the document is restructured.
• Two bare citations normalized (pr-fix-queue.ts:579-597, pr-fix-queue.ts:937-1010 → src/lib/pr-fix-queue.ts:…) so the citation check covers every citation in the document.
• New §11 "Design self-verification" states what the check covers (structure and source drift) and, explicitly, what it does not: alert runtime behavior, because no exporter exists yet (§10). No production code is claimed to enforce any of these rules.

A bounded adversarial pass over this diff found no blockers, confirmed the new checks are non-vacuous (52 signals collected, 36 citations matched, exact 13-row ID assertion), and found three checks that could pass on an empty input set — fixed in the same commit by the cardinality guards. Note for transparency: that reviewer, like this run's author, is a MiniMax-family model, so it adds adversarial perspective but no model-family diversity.

Invariants this change must keep (and every other code path enforcing the same rule)

1. Signal predicates reference only persisted status values. IssueSyncRun.status writers: sync-lock.ts:160 (running), sync/route.ts:64-70 (completed), scheduled/route.ts:148-151 (completed) / :224-227 (failed) — no other writers exist (grep-verified); the doc enumerates exactly these and cites each, and docs/observability.test.ts now enforces the equality against the source on every run. A2/A4/failures signals all read "completed"/"failed". dispatch_audit_failures_in_window reads only AuditLog.success (Boolean) and createdAt (prisma/schema.prisma:334,337, indexed :341) — both persisted non-null fields; no deletion path exists for AuditLog (grep-verified: no delete/deleteMany call sites), with retention treated as an open §10 question, not an assumption.
2. Task-flow signals count worker task rows only. Other Lease writers: lease.ts:69 via claim (worker; the only other creator besides the groomer). Other AgentRun inserters: tasks/report (task rows), heartbeat route (heartbeat), legacy agent-runs (client-supplied), groomer (groom). Kept by the §4.3 allowlist filter applied to dispatch_execution_active, dispatch_work_pickups_in_window, and dispatch_agent_runs_reported_in_window, and by A4/A2/§5/§6/§8 all referencing the filtered definitions.
3. "Nothing happened" must not alert; "pipeline stale" must. Other paths enforcing this separation: §6 "Normal inactivity must NOT alert" bullets, the gate sentence under it, §8.1/§8.2/§8.5/§8.6/§8.7 examples, and the A2↔A4 fresh-guard. Kept mutually consistent: the A6 pin removes the last aggregate false-fire rail, the churn bullet + §8.7 name the no-alert case, and the Enabled column makes "this alert cannot fire" readable at a glance.
4. Bounded cardinality. Every signal uses fixed enums or allowlisted windows; agentName is used only as an exporter-side filter, never a label — the §9 rules are unchanged and referenced from §4.3. dispatch_audit_failures_in_window adds only action (allowlisted) and window (900s/3600s).
5. Un-age-qualified aggregates must not fire alerts; windowed counts must not be rate-evaluated. The same rule A7 enforces via dispatch_pr_fix_overdue_unacked (aggregate dispatch_pr_fix_unacked_handouts is dashboard-only) is enforced for A6 (§4.3 dashboard-only pin, §6 Enabled: no, §6 churn bullet, §6 gate sentence, §8.7) and for A12 (§4.1 rate-function rule, §4.10 direct-evaluation semantics, §7 row A exporter guidance). An exporter that ships an assignment-age source promotes A6 by editing the Enabled cell and §10; no other alert depends on the aggregate.

Local validation (head f4b2c2e)

• npm run lint — ok (0 errors; 1 pre-existing unrelated warning in src/app/login/page.tsx:39)
• npm run typecheck — ok
• npm run test — 4311 passed, 16 skipped, 1 todo (179 files: 176 passed, 3 skipped). Up from 4305 on 6117985: the 6 new checks are the delta
• npx vitest run docs/observability.test.ts — 6 passed
• Mutation-checked, not merely green: restoring A6's for: 30m, renaming a signal in an alert condition to an undefined name, and moving a citation to an out-of-range line each fail the new file; all three reverted before this commit
• Markdown table column-consistency scan across the whole doc — clean; every file:line citation now re-verified mechanically on each test run rather than only by hand

Docs-only change plus one test file; no production code touched.

Independent review

Three independent review passes over the pushed document, plus a fourth over this revision's diff. First pass on the third revision found the groomer-lease contamination (blocker) and the canonical-loop coverage hole (major) — fixed in 11a2eb5/c087b0b. Second pass found the heartbeat/legacy-row leak in the reported-gauge filter (major) and an overclaiming §6 bullet (minor) — fixed in c087b0b/734ce30. Third pass on 6117985's diff approved it — it re-verified the factual claims (no assignedAt; updatedAt sync overwrite; assign_agent/claim_issue audit split; AuditLog fields and absence of pruning; the delta() rotation math) and cross-section consistency — and its two minor wording findings were fixed in the same commit. Fourth pass (this revision) reviewed only the new executable check and the §6 enablement change, and its one substantive finding was the vacuous-pass risk fixed here.

Remaining external verification

CI on the previous head 6117985 was green — 13/13 check runs: Lint, Typecheck, Build, Tests, Database migrations, Database integration, Coverage, npm audit, Docker Build, Docker Build (MCP), smoke, Workflow lint, review. CI on this head f4b2c2e is pending and is not claimed green; the operator verifies it after this run.

Also open for the human reviewer: joryirving's change request from 734ce30 (the A6 aggregate and A12 delta() findings) was answered and fixed in 6117985. Both threads carry the author's reply with source citations, and this revision only makes that resulting state explicit and mechanically checked. Their re-review is theirs to make — this run does not sweep for further feedback.

[joryirving]

Reviewed exact head b1494b2 against #1237 and the document's own signal kinds, example traces and alert semantics. This is a well-scoped, genuinely Dispatch-independent design; the AI review's missing runtime enforcement for a design-only alert catalogue is not itself a blocker. There are, however, two correctness issues in the alert contract that a later exporter/dashboard would otherwise implement literally:

[P1] A4 applies a counter-only rate function to a windowed gauge; no reliable “runs started in 30m” signal exists as written. docs/observability.md §4.10 explicitly types dispatch_agent_runs_total as gauge (windowed) sourced from AgentRun rows, and §4.1 explains these totals are not process-monotonic and must be gauges or windowed deltas. Yet §6 A4 uses increase(dispatch_agent_runs_total{run_type="implement"}[30m]) == 0. PromQL increase() assumes a monotonically increasing counter with resets and does not correctly interpret a moving-window count that falls as old runs expire. This can false-alert (or fail to alert) when the window rolls. Specify either a real event-derived number of starts in the previous 30 minutes as an as-of-scrape gauge (query AgentRun.startedAt >= now()-30m), used directly as == 0, or an actual monotonic counter with well-defined restart/reset semantics, then align §4.10, A4, §7 and examples. Do not leave a counter-looking name with incompatible gauge semantics.

[P1/P2] A4's promised sync-failure suppression is not in the predicate. §6 says A4 is “suppressed while A1/A2 fire” and §8.3 says a stopped supply pipeline will not be misdiagnosed as a worker outage. But A4 tests only dispatch_scheduler_healthy == 1, which means armed and ticks not overdue. A scheduled sync that keeps ticking yet fails (e.g. GitHub unavailable, no successful IssueSyncRun) can leave dispatch_scheduler_healthy=1 while A2 correctly fires. With cached eligible work and no starts, A4 fires too, falsely blaming hand-out/worker capacity. Require a positively fresh/healthy successful sync (with an explicit unknown/startup case), or explicitly remove the suppression promise and classify this ambiguity; ideally prove it by adding a worked trace: scheduler ticks on time, sync fails, A2 fires, A4 stays silent.

[P2] A7 needs an aged unacknowledged hand-out, not a continuously nonzero aggregate. dispatch_pr_fix_unacked_handouts > 0 for > hand-out timeout is true if the system always has some recently handed-out item, even if every individual hand-out is acknowledged within seconds. The 35m for duration measures the continuity of the aggregate, not the age of any one identity. Provide an explicit predicate tied to dispatchedAt / generation with now - dispatchedAt > configured timeout for currently unacknowledged items, or a gauge counting overdue unacked hand-outs, then pin the normal steady-churn/no-alert example.

These are documentation/contract corrections only. No implementation or runtime instrumentation is required for #1237. The existing 13 exact-head checks are green, including the AI review job (which itself recommends approval in prose). Please fix the alert semantics and resync examples; I'll re-review and merge.

[itsmiso-ai]

Thanks for the careful review — the alert contract fixes have landed on 2684f72. Summary of what changed in docs/observability.md:

A4 (P1 counter/gauge, P1/P2 sync-suppression): the predicate no longer uses increase() against a windowed-delta gauge, and the fresh-sync suppression is now spelled out in the predicate rather than promised in prose.

• New §4.10 gauge: dispatch_agent_runs_started_in_window{run_type, window} (as-of-scrape, AgentRun.startedAt >= now() - window). A4 uses this directly via == 0.
• New A4 clause: (time() - dispatch_sync_last_success_timestamp_seconds) <= 2 × sync interval. A scheduler that ticks on time while sync is failing no longer satisfies A4; the failure routes to A2/A13 instead of being misread as a worker outage.
• §4.1 now states increase()/rate() are counter-only, and DB-derived totals must use delta()-style window deltas or _in_window as-of-scrape gauges. A12 switched to delta() for consistency. §7 Prometheus row points at the same convention.
• §8.3 "Read" rewritten against the new guards; §8.4 also updated to use the age-tied signal.

A7 (P2 age-tied): A7 is now driven by dispatch_pr_fix_overdue_unacked (items where now - dispatchedAt > PR_FIX_HANDOUT_TIMEOUT_MS for the current generation with no matching ack). The aggregate dispatch_pr_fix_unacked_handouts stays in §5 row 5 for the dashboard view but is not used to fire alerts.

• New §8.5 "Healthy — PR-fix steady churn (A7 must NOT fire)" pins down the no-alert case.
• "Normal inactivity must NOT alert" gained a bullet naming the A7 carve-out explicitly.

No production code, schema, or workflow touched; only docs/observability.md was modified in this revision. README pointer and non-goals unchanged.

Local checks on the new head: npm ci ok; npm run lint ok (1 pre-existing warning in src/app/login/page.tsx:39); npm run typecheck ok; npm run test — 4305 passed, 16 skipped, 1 todo. CI itself isn't claimed green here; that's for the follow-up verification cycle.

[joryirving]

Re-reviewed exact head 2684f72 against #1237, the revised alert catalogue, and current producer code. The prior A4 counter/gauge and A7 age-based fixes are present and the GitHub Actions workflows are green. There are still two alert-contract correctness blockers in this design-only document; no runtime implementation is requested.

[P1] The proposed successful-sync metric filters on a status Dispatch never writes (docs/observability.md:273, A2/A4). §4.9 says dispatch_sync_last_success_timestamp_seconds reads the latest IssueSyncRun.completedAt with status success. Both src/app/api/sync/scheduled/route.ts and src/app/api/sync/route.ts write status: "completed" on success, while the scheduled failure path writes "failed". Therefore a literal exporter using the catalogue will have no successful-sync data even when sync is healthy: A2 cannot reliably detect stale sync (missing Prometheus series does not satisfy time()-metric > ...), and A4's new fresh-sync guard can never be true. Align the metric source/status with actual persisted values, document whether a completed run with partial per-repo errors counts as healthy, and explicitly cover a deployment that has never completed a successful sync: absent baseline must transition from an initial startup grace to an actionable unhealthy/unknown state, not silence forever. Worked traces for healthy completed sync, failed sync after a prior success, and first sync never succeeding would pin this.

[P1/P2] A4's purported worker-start gauge is populated only when a worker reports, not when it starts (docs/observability.md:290,353). The canonical POST /api/agents/[agentName]/tasks/report creates AgentRun at task completion; it stores the worker-supplied startedAt retrospectively or falls back to report time when omitted/invalid. A worker can have received and started an implement task 5 minutes ago, yet have no AgentRun row until it finishes an hour later. During that hour A4 will claim no starts for 30m despite work progressing, even with more eligible work in the cache. Fix the signal contract by deriving starts from a durable claim/hand-out event that exists at pickup time, or explicitly classify AgentRun as reported/completed runs and adjust A4's assertion and guards so it cannot claim absence of starts. Add a non-Courier worked example where a worker has an in-flight long task and no report yet; A4 must not falsely diagnose a hand-out failure.

Both issues affect what downstream implementers would literally build from this standalone alert specification. No blocker found in README, integration options, privacy rules, or the prior A7 fix.

[joryirving]

This claims IssueSyncRun.status=success, but both scheduled/manual sync success paths persist status="completed". Consequently the proposed metric has no healthy data, A2's no-data case is silent, and A4's fresh-sync guard cannot pass. Include the never-successful-first-sync baseline.

[itsmiso-ai]

Fixed in 89cee2a. Confirmed against source: the only persisted IssueSyncRun.status values are running (src/lib/sync-lock.ts:160), completed (src/app/api/sync/route.ts:67, src/app/api/sync/scheduled/route.ts:151), and failed (scheduled/route.ts:227) — no success anywhere. The §4.9 table now matches "completed"/"failed" with the observed enum documented and cited. The never-successful-first-sync baseline is in A2: the absent-series case is an explicit OR branch (with the note that time() - <no data> > x yields no data, not true, so consumers must evaluate absence as firing, e.g. absent(...) == 1). The absent series also keeps A4's fresh-sync guard unsatisfied, so the failure routes to A2/A13, never to a worker-outage misdiagnosis. Bonus finding while verifying: the manual-sync error path (src/app/api/sync/route.ts:80-82) returns 500 without persisting failed, leaving the row running forever — documented as a dispatch_sync_failures_total coverage gap with A2 freshness as the backstop.

[joryirving]

AgentRun is inserted by tasks/report when work finishes, with a retrospective optional startedAt (else report time). This gauge cannot observe task starts in real time. A4 can false-alert while a long-running worker is active; define a pickup-time signal or narrow the promised detection and add an in-flight-without-report example.

[itsmiso-ai]

Fixed across 89cee2a + c087b0b. Confirmed: AgentRun rows land at tasks/report finish (route.ts:571) with the retrospective optional startedAt, so dispatch_agent_runs_started_in_window is now documented as measuring reported starts with that blind spot stated, and it is no longer used to fire A4.

A4 now uses a pickup-time signal plus an in-flight guard:

• dispatch_work_pickups_in_window — new AgentWork.createdAt (POST /api/agent-work/start, src/lib/agent-work.ts:186) or new Lease.createdAt (claim via upsertLease, src/lib/lease.ts:69, createdAt @default(now())); renewals excluded.
• dispatch_execution_active — unexpired worker Lease or CLAIMED/IN_PROGRESS AgentWork, suppressing A4 for the task's full duration.
• dispatch_agent_runs_reported_in_window — added after my own independent review found the canonical loop (next-task → execute → report) writes no pickup row, so pickups alone would have fired A4 continuously for a report-only fleet. Filtered by a task-run-type allowlist (implement/followup-pr): a first pass that merely excluded groom leaked the heartbeat route's runType: "heartbeat" rows (heartbeat/route.ts:100-104) and legacy agent-runs rows — a polling-only fleet would have kept the gauge non-zero and disarmed A4. §4.3 now names all three system emitters and requires exporter-side filtering (agent name stays out of labels per §9).
• New §8.6 "Healthy — long-running worker, no report yet (A4 must NOT fire)" pins the in-flight-without-report case; §6's mid-task bullet and the residual blind spot (claim-less worker mid-single-task is unobservable, A4's firing there is a truthful "Dispatch cannot observe this fleet" warning, tied to the §10 acknowledgement-gap question) are scoped accordingly.

Superseded by a newer automated review for this pull request.

[itsmiso-ai]

Courier: already addressed (run courier-31f1681d0781ccdf74ebfdb8214ebd77)

PR #1239 is already on the run branch courier/misospace/dispatch/issue-1237 with head 734ce30 fully addressing the assigned scope. Both P1 review findings from joryirving are resolved in code and prose, and the author has replied to both review threads with detailed commit references. State at exit: (1) PR is open, not draft, head SHA 734ce30 matches origin/courier/misospace/dispatch/issue-1237; (2) all 13 CI check_runs are green (Lint, Typecheck, Build, Tests, Database migrations, Database integration, Coverage, npm audit, Docker Build, Docker Build (MCP), smoke, Workflow lint, review) per check_runs API at run 38438209306; (3) local validation re-run on 734ce30: npm ci + prisma generate ok; npm run lint clean (1 pre-existing warning in src/app/login/page.tsx:39 matches the PR body); npm run typecheck clean; npm run test 4305 passed, 16 skipped, 1 todo (matches PR body); npm run build clean. (4) P1 review finding on IssueSyncRun.status = 'success': §4.9 now matches 'completed'/'failed' (src/lib/sync-lock.ts:160 running, src/app/api/sync/route.ts:67 completed, src/app/api/sync/scheduled/route.ts:151 completed, src/app/api/sync/scheduled/route.ts:227 failed — all four citations re-verified against current source); manual-sync 500-without-failed gap (src/app/api/sync/route.ts:80-82) is documented. (5) P1 review finding on dispatch_agent_runs_started_in_window pickup blind spot: the gauge now states its blind spot (AgentRun inserted at tasks/report finish, src/app/api/agents/[agentName]/tasks/report/route.ts:571) and no longer drives A4; A4 gates on dispatch_execution_active + dispatch_work_pickups_in_window + dispatch_agent_runs_reported_in_window with a §4.3 task-run-type allowlist (implement/followup-pr) that excludes the heartbeat route (src/app/api/agents/[agentName]/heartbeat/route.ts:100-104 runType=heartbeat), the hosted groomer (src/lib/groomer/run.ts:250-254 lease, run.ts:1196-1200 AgentRun runType=groom), and the legacy agent-runs route (src/app/api/agent-runs/route.ts:78). All three system-emitter citations re-verified. The two joryirving review threads are is_outdated: true (lines moved past the original 273/290 anchors) with author replies already on each thread and a full PR body revision-history entry pointing at the fix commits 89cee2a/11a2eb5/c087b0b/734ce30. The thread is_resolved: false is a UI state the reviewer owns after re-verification; the work to satisfy the finding is complete on the branch. No new changes were needed in this run.

[joryirving]

Reviewed Dispatch #1239 at exact head 734ce30 against #1237, the prior A4/A7 and sync-status findings, the current signal catalogue, and the cited source paths. The earlier blockers are fixed: sync success now correctly means IssueSyncRun.status="completed" (with an explicit absent-first-sync case), A4 distinguishes task pickup, in-flight ownership and task completion, and A7 correctly keys on an overdue hand-out, not the always-nonzero count during healthy churn. Docs-only boundaries, backend-neutral integration, and privacy/cardinality requirements are respected. The head is mergeable; Security Audit, Build Dispatch Image, CI, AI PR Review, and PR Smoke are all green.

Requesting changes on two remaining alert-predicate correctness issues. They need only doc/spec changes, no exporter or production implementation.

[P2] A6 mistakes a perpetually nonzero aggregate for an individually overdue assignment (docs/observability.md:159,390). dispatch_assigned_without_execution counts assignments with agent/* but no active Lease/AgentWork. A6 then fires when that aggregate is >0 for 30 minutes, although the for time measures continuous aggregate occupancy, not how long any specific issue has waited. Counterexample: one issue is assigned per minute and picked up after 30 seconds, with overlapping assignments so there is always >=1 waiting. Every assignment is healthy, but the aggregate stays nonzero for 30m and A6 falsely diagnoses an assignment "never picked up". It also does not restrict the signal to eligible/open issues, potentially counting old assigned labels on settled tasks. Please define an age-qualified individual not-yet-picked-up predicate/gauge (with a bounded category dimension), or explicitly mark the age evidence unavailable and defer A6 rather than presenting the aggregate + for as equivalent. Pin one healthy steady-churn/no-alert trace and one genuinely stranded assignment trace. This is the same time-over-aggregate mistake already corrected in A7.

[P2] A12 still computes a delta of an already-windowed gauge (docs/observability.md:319,396). §4.1 and §4.10 explicitly say dispatch_audit_failures_total is a gauge (windowed): the DB-derived failure count already has a window applied. delta(dispatch_audit_failures_total[15m]) > 0 therefore measures how the rolling count changed over 15m, not whether any failures occurred during that 15m. Concrete counterexample: the 15m window contains one old failure at its start; it rolls off while one new failure arrives. Gauge at evaluation start=1 and end=1, so delta=0, silently missing the new failure. This remains incorrect even though delta() is legal on a gauge (unlike increase()); legality isn't equivalence to a count of failures. Define an explicit as-of-scrape dispatch_audit_failures_in_window{window="900s"} and alert on >0 directly, or instead define a genuine monotonic cumulative count and use a counter-aware function. Align §4.1, the signal row, §6 A12 and §7 mapping; include a counterexample with equal rolling counts but a new failure.

These are targeted contract fixes, not a request for runtime tests or instrumentation. No other blocking issue identified.

[joryirving]

P2: dispatch_assigned_without_execution > 0 for 30m measures an aggregate continuously being nonzero, not the age of an individual assigned-but-unstarted issue. Healthy overlapping assignments picked up in under a minute can keep this gauge >0 indefinitely and falsely fire A6. Qualify by per-item waiting age and eligible/open status, or explicitly mark the age predicate as a future gap, then document a steady-churn no-alert scenario.

[itsmiso-ai]

Fixed in 6117985 via the future-gap branch you allowed, after verifying against current source that no per-item age source exists: Issue has no assignedAt (prisma/schema.prisma:21-100 — only createdAt/updatedAt/lastSyncedAt); updatedAt is overwritten from GitHub's updated_at on every sync (src/lib/issue-sync.ts:115) and lastSyncedAt is bumped by claims/webhooks (src/lib/issue-claim.ts:83,106,163, src/app/api/issues/webhook/route.ts:164), so neither measures assignment waiting time; and only actions-route assignments write assign_agent audit rows (src/app/api/issues/actions/route.ts:14 — claims audit as claim_issue but materialize a lease and fall outside the predicate), so a GitHub-side label add has no age source at all.

• A6's row is now explicitly pinned ("must NOT fire on the aggregate") with the intended final predicate spelled out — per-item waiting age on an open, eligible issue carrying agent/* with no active lease/AgentWork — and a promotion path once an age source ships.
• §4.3 marks dispatch_assigned_without_execution as a dashboard aggregate that must not fire alerts, mirroring the dispatch_pr_fix_unacked_handouts treatment.
• New §8.7 "Healthy — assignment steady churn (A6 must NOT fire)" pins the overlapping-sub-minute-waits scenario, including why for: 30m does not rescue the aggregate.
• §6's "Normal inactivity" list gained the churn bullet; the gate sentence no longer lists A6 as firing (execution-loss detection stays with A5, pickup starvation with A4/A10); §10 gained the age-source decision (sync-diff assignedAt timestamp vs assign_agent audit join, with the retention caveat — verified no AuditLog pruning exists today).

[joryirving]

P2: delta() of this already-windowed DB gauge is not the number of failures in the evaluation window. A rolling 15m count can be 1 at both start and end even when a new failure arrives and the older one expires, making delta==0 and missing the failure. Use an as-of-scrape ..._in_window{window="900s"} > 0 gauge directly, or use an actual monotonic event counter with counter-aware evaluation. Align §4.1/§4.10 and the integration guidance.

[itsmiso-ai]

Fixed in 6117985. Confirmed your rotation analysis: delta() over an already-windowed rolling count can read zero exactly when a failure rotates out while a new one rotates in, so A12 moved to the direct as-of-scrape gauge you named:

• dispatch_audit_failures_total → dispatch_audit_failures_in_window — as-of-scrape count of AuditLog.success = false AND createdAt >= now() - window, window allowlisted (900s, 3600s), action allowlisted. Directly evaluable as persisted: success/createdAt are non-null (prisma/schema.prisma:334,337, createdAt indexed at :341).
• A12 is now dispatch_audit_failures_in_window{window="900s"} > 0, evaluated directly.
• §4.1 rewritten: increase(), rate(), and delta() are counter/cumulative-only; the window-roll false-negative is stated as its own rule; _total-suffixed rows are declared historical windowed counts that must not fire alerts through rate-style evaluators.
• §4.10/§7 aligned: shared-window sentence includes the audit gauge's allowlist; the pull-exporter row now forbids delta() over already-windowed counts (sound only if the exporter instead emits cumulative totals); residual "windowed delta" phrasings swept to "windowed counts" (§4.1 kinds, §10 counter modeling).

An independent review pass over this diff verified the hazard math and the alignment, with two wording-precision findings (overbroad "Dispatch-mediated" for assign_agent; residual "windowed deltas" terms) fixed in the same commit.

Superseded by a newer automated review for this pull request.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: qwen3.8-flash-next-chat (openai) — fallback (primary failed) · pr-reviewer-action v3.4.1

Requirement traceability gap: the review itself completed, but one or more in-scope requirements could not be verified against enforcement or test evidence. Approval is withheld until that requirement is covered.

Findings (3 minor)

Severity	Location	Finding
Minor	docs/observability.md	P2: `delta()` of this already-windowed DB gauge is not the number of failures in the evaluation window. A rolling 15m count can be 1 at both start and end even when a new failure arrives and the older one expires, making `delta==0` and missing the failure. Use an as-of-scrape `..._in_window{window
Minor	docs/observability.md	P2: `dispatch_assigned_without_execution > 0 for 30m` measures an aggregate continuously being nonzero, not the age of an individual assigned-but-unstarted issue. Healthy overlapping assignments picked up in under a minute can keep this gauge >0 indefinitely and falsely fire A6. Qualify by per-item
Minor	docs/observability.md	`AgentRun` is inserted by `tasks/report` when work finishes, with a retrospective optional `startedAt` (else report time). This gauge cannot observe task starts in real time. A4 can false-alert while a long-running worker is active; define a pickup-time signal or narrow the promised detection and

Recommendation

Approve. The PR delivers a comprehensive, standalone observability design document (docs/observability.md) and an accompanying structural test (docs/observability.test.ts) that aligns with the linked issue's requirements. It successfully addresses previous review feedback regarding alert predicates (A4, A6, A7, A12) by introducing age-tied signals, explicit future-gap pinning for unobservable states, and strict rules against rate-style functions on windowed gauges.

Findings

No blocking or major findings. The design is internally consistent, respects privacy/cardinality constraints, and correctly identifies current instrumentation gaps.

Minor / Info

• Info: The dispatch_assigned_without_execution signal (A6) is explicitly pinned as a "future gap" because per-item assignment age cannot be derived from persisted columns today. This is well-documented in §4.3, §6, and §8.7, and the test enforces that it remains disabled. This is acceptable for a design-only deliverable.
• Info: The dispatch_agent_runs_started_in_window gauge has a documented blind spot (it measures reported starts, not real-time pickups). The design mitigates this by using dispatch_work_pickups_in_window and dispatch_execution_active for A4, which is correct.

Standards Compliance

The PR adheres to repository standards:

• Design Only: No implementation code, schema changes, or dashboard assets are included, respecting the non-goals of issue PR 1237.
• Privacy/Cardinality: Strict allowlists for metric labels are defined (§9), avoiding high-cardinality fields like repo names, issue IDs, or error strings.
• Testing: The new docs/observability.test.ts validates the structure of the design doc, ensuring alert IDs are unique, required columns exist, disabled alerts don't look live, and citations resolve to existing files/lines. This fits the repo's pattern of testing documentation contracts.

Linked Issue Fit

The PR fully satisfies issue PR 1237:

• Signal Catalogue: §4 provides a detailed catalogue with units, sources, dimensions, and semantics.
• Dashboard Spec: §5 defines backend-neutral views separating settled work, human decisions, and operational exhaustion.
• Alert Catalogue: §6 lists 13 alerts with conditions, severities, and recovery actions, including explicit handling of normal inactivity.
• Integration Matrix: §7 outlines optional Prometheus/OTel approaches without assuming specific vendors.
• Boundaries: The design is independent of Courier and does not implement any production code.

Thread Dispositions

• PRRT_kwDOSZt6uM6r_3m9 (A12 delta misuse): Fixed. The diff shows dispatch_audit_failures_total replaced by dispatch_audit_failures_in_window (as-of-scrape gauge), and A12 now evaluates it directly (> 0). §4.1 explicitly forbids delta() on windowed counts.
• PRRT_kwDOSZt6uM6r_3mz (A6 false positives): Fixed. A6 is now marked "no — pinned future gap" in the Enabled column. The condition explicitly states "must NOT fire on the aggregate" and explains the missing age source. §8.7 adds a steady-churn example showing why it must not fire.
• PRRT_kwDOSZt6uM6r9RCh (A4 pickup blind spot): Fixed. A4 now uses dispatch_work_pickups_in_window and dispatch_execution_active instead of relying solely on reported starts. §8.6 documents the long-running worker case where A4 must not fire due to the in-flight guard.

Requirement trace (8 unresolved of 10)

8 of 10 requirement(s) not fully traced to enforcement and a test:

• req-43ec84865ad7 — unverifiable (the reviewer reported no trace for this requirement)
• req-f205cf467bdd — unverifiable (the reviewer reported no trace for this requirement)
• req-e10c9f7df335 — unverifiable (the reviewer reported no trace for this requirement)
• req-1a1bbcc66e5b — unverifiable (the reviewer reported no trace for this requirement)
• req-d6f0edb51ce1 — unverifiable (the reviewer reported no trace for this requirement)
• req-9386eb6b9a59 — unverifiable (the reviewer reported no trace for this requirement)
• req-a2fe56471e8f — unverifiable (the reviewer reported no trace for this requirement)
• req-a149c34621ce — unverifiable (the reviewer reported no trace for this requirement)

Unresolved Review Threads

• PRRT_kwDOSZt6uM6r_3m9: open — fixed without evidence citing current code
• PRRT_kwDOSZt6uM6r_3mz: open — fixed without evidence citing current code
• PRRT_kwDOSZt6uM6r9RCh: open — fixed without evidence citing current code

Outstanding Human Change Requests

• @​joryirving's change request (734ce30, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.

Approval withheld: the review completed, but an in-scope requirement lacks verifiable enforcement or test evidence, so it is publishing as an advisory comment rather than an approval.

[joryirving]

Re-reviewed exact head f4b2c2e against #1237 and the prior A4/A6/A7/A12 human review findings. APPROVED. A6 is explicitly disabled pending real per-item assignment-age evidence, so overlapping healthy assigned-work churn cannot trip an aggregate for alert. A12 uses the present 15-minute audit failures gauge directly rather than delta of a rolling gauge; it detects arrivals when the net count does not change. A4 is gated on healthy sync and truly observed pickup/execution/report signals with task-run allowlists; A7 is an overdue per-handout-age predicate. docs/observability.test.ts pins alert contract invariants without adding forbidden production instrumentation. Standalone design-only boundary, backend-neutral integration, privacy labels, and #1252/#1255 implementation handoff maintained. CI, security, build, PR smoke, AI review passed; mergeable.