Repository navigation
fix(revanote): reliable batch dispatch, merged-commit gate, generation-bound replies - #504
Open
finedesignz wants to merge 23 commits into
Open
finedesignz wants to merge 23 commits into
finedesignz wants to merge 23 commits into
Conversation
A stream-json session whose `claude` process exits repeatedly used to leak its SessionBridge (and the `/ws/agent` WS connection it owns) once local recovery gave up — both on `max_restarts_exceeded` (scheduleRestart) and on `circuit_open` (tripBreaker). The bridge was dropped from the in-process `runs` map but never stopped, so its hub-side WS channel stayed registered forever. That meant hub's `sessions.status` never flipped to 'offline' (only a real WS close does that, in hub/src/ws/agent.ts's close handler), and `ensureSessionOnline`'s fast path (`getChannel(sessionId) != null`) kept returning true — so an inbound dispatch (revanote, error-capture, scheduler) never respawned the dead session. It just silently vanished into a void the hub believed was alive. Fix: call `run.bridge.stop()` on both give-up paths before dropping the run. Closing the bridge closes its WS, which hub's existing close handler turns into `setSessionStatus(sessionId, 'offline')` — the next dispatch then sees the session offline and `ensureSessionOnline` actually respawns it. No new wire protocol needed; this reuses the existing WS-close-implies- offline contract, so old and new supervisors interoperate — a supervisor without this fix just keeps the pre-existing dangling-bridge behavior. Tests: process-manager.test.ts asserts stopCalls==1 on max_restarts_exceeded; process-manager-circuit-breaker.test.ts asserts stopCalls==1 on circuit_open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Incident: a background subagent marked 43 Lakeside annotations resolved,
citing commit 86ad71296 on every single one. That commit was real but
dangling (never pushed, never merged, never deployed) and unrelated to any
of the 43 fixes -- nothing shipped, but the client's comments were closed.
The hub now refuses to forward or persist a resolved:true callback unless
the agent's cited commit_sha is verified to exist on the GitHub remote:
- hub/src/revanote/commit-verify.ts: verifyCommitOnRemote() calls
GET /repos/{owner}/{repo}/commits/{sha} via the existing GitHub App
installation token (hub/src/auth/github-app.ts -- no new credential).
Fails closed: missing commit_sha, missing installation_id/repo_slug
context, an unparseable repo slug, a 404, or any API error all mean
"not verified".
- hub/src/revanote/run-lifecycle.ts: finalizeAnnotationReply now runs this
gate before persisting annotation_run/annotation status or enqueuing the
outbound callback. An unverifiable resolved:true claim is downgraded to
resolved:false with a specific reason (commit_sha_missing /
repo_context_missing / repo_slug_unparseable / commit_not_on_remote /
commit_verify_failed), surfaced as both skip_reason and the callback's
error field. resolved:false replies never touch this gate.
- result-schema.ts / callback.ts: added commit_sha (gated) and deploy_url
(advisory) fields end to end.
- prompt.ts: the envelope contract now asks for commit_sha/deploy_url and
states resolved:true is only honored once the commit is actually
pushed/deployed/verified. Per owner directive, also documents the batch
protocol -- one branch/PR per whole review, merge + redeploy + per-page
verification BEFORE resolving any comment, citing the merged SHA. This
prompt text is advisory only; the code gate above is the actual control
and treats a merged SHA the same as any other -- it exists on the remote,
so it passes.
Investigated PR #489 (revanote burst queueing): a review's N comments are
NOT dispatched as one batched prompt -- each annotation is its own dispatch
token, one in-flight per session with the rest FIFO-queued. #489 raised the
waiter cap so a large burst queues instead of dropping as session_busy; it
did not change the one-token-at-a-time shape. The batch protocol therefore
relies on the agent's own session memory across sequential per-comment
turns. Dispatch is intentionally NOT restructured by this fix.
Tests: hub/test/revanote-commit-verify.test.ts (the gate, GitHub API
mocked), hub/test/revanote-run-lifecycle-commit-gate.test.ts (finalize
integration: unpushed SHA -> downgraded, missing SHA -> downgraded,
verified SHA -> forwarded, resolved:false bypasses the gate). Updated
revanote-dispatch.test.ts and revanote-prompt.test.ts fixtures/baselines
for the new required field and prompt text.
docs/revanote.md updated with the commit-verify gate, the batch protocol,
and the per-comment-dispatch finding.
check-baseline: 0 fail (was already 0 fail with pre-existing skip-count
drift on origin/main, confirmed via git stash before this change -- not
introduced here). typecheck: no new errors (one pre-existing
ParseFallback|ParseOk union error in run-lifecycle.ts predates this diff).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All ~23 revanote client sites map onto ONE remo-code session (in-flight cap 1); when that session wedges every client's annotations pile up failed/failed_offline/parked-pending with zero owner-visible signal (the callback to revanote reports the failure, but nothing tells the owner). Add a boot-started sweep (hub/src/revanote/stall-alert.ts, modeled on scheduler/run-reaper.ts) that fires on two independent stall signatures: - an annotation sitting parked-offline / rejected / target-offline-expired past REMO_REVANOTE_STALL_PARKED_MAX_MS (default 1h) - an annotation_runs row stuck status='in_flight' past REMO_REVANOTE_STALL_RUN_MAX_MS (default 30min) -- worse than the above, since the dispatcher's own 20min finalizeTimeoutMs should have forced a finalize before this point Fan-out reuses the existing orchestrator notify channel (fanOutNotify -- telegram + in-app + emails4agents email); no new transport. De-dup via a new revanote_stall_alerts table (one row per user, last_alert_at) caps alerts to one per user per REMO_REVANOTE_STALL_COOLDOWN_MS (default 1h) so a backlog of dozens of stalled annotations across 23 sites sends one alert, not one per annotation. REMO_REVANOTE_STALL_DISABLED escape hatch. Also re-measures tools/regression-baseline.json: origin/main already drifted from the stale 2026-07-28 snapshot (pass=2059/skip=255) to pass=2157/skip=259 before this PR touched anything (confirmed via git stash -u); this PR's own 9 new DB-free tests land the actual at pass=2166/skip=259/fail=0, and skip_max is raised 256->260 to absorb the pre-existing drift plus zero new skips from this change. Tests: hub/test/revanote-stall-alert.test.ts -- fires at threshold for both signatures independently, does not fire below threshold or on zero-count rows, cooldown blocks a repeat alert then fires again once elapsed, one user's notify failure never aborts the pass for others, load failure is fail-open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sequential per-annotation dispatch cannot produce "all comments of one
review fixed in ONE PR": the shared pipeline only releases the next
queued token once the current one finalizes, so N comments became N
separate turns/branches/PRs relying on unsatisfiable cross-turn agent
memory.
Coalescing is keyed ONLY on payload_raw.batch_id (per prod sampling,
present on 3/88 annotations but correctly grouping a burst where set).
- dispatcher.ts: a batch_id-carrying annotation short-circuits after
mapping/session resolution and stays pending instead of dispatching.
forceSingle (used by the manual retry endpoint) bypasses this.
- batch-dispatch.ts (new): boot-started, fully DB-state-driven poll
(REMO_REVANOTE_BATCH_POLL_MS, default 5s) that groups pending
batch_id annotations by (user, batch_id, resolved target session)
and dispatches a group once no new arrival has landed for
REMO_REVANOTE_BATCH_DEBOUNCE_MS (default 30s) since the group's
latest arrival. One prompt, one dispatch token, one pipeline turn
per group. No per-batch timer state or content buffering -- restart
loses nothing, the next tick re-derives the due set from the DB.
- prompt.ts: renderBatchAnnotationPrompt fences every member
separately and asks for one branch/PR covering the whole batch, one
<<JSON>>{"annotations":[...]}<<END>> envelope keyed by external
annotation id. The single-annotation contract's unsatisfiable
"batch across turns using memory" text is replaced with a
satisfiable one-comment-one-PR contract.
- result-schema.ts: RevanoteBatchResult/parseRevanoteBatchOutput +
envelopeForBatchItem rebuilds a single-item envelope per member so
finalize reuses run-lifecycle.finalizeAnnotationReply UNCHANGED --
same commit-verify gate, DB writes, and callback shape per member.
A member missing from the reply array finalizes resolved:false/
missing_from_reply; an unparseable batch reply falls every member
back through the same single-item parse fallback.
- REMO_REVANOTE_BATCH_RUN_MAX_MS (default 2h) sets both the batch
dispatch's finalizeTimeoutMs and hookMaxMs, since a batch turn does
N comments' worth of branch/PR/CI/merge/redeploy work and needs a
longer ceiling than a single annotation's 20min default.
Tests: hub/test/revanote-batch-dispatch.test.ts (grouping, debounce
reset on new arrival, one-prompt-per-batch, array-reply finalize with
per-item commit-verify, missing member, unparseable reply, no-batch_id
single path, forceSingle retry, batch finalize ceiling not tripped by
the single path's 20min timeout). Updated the golden prompt-text
baseline in revanote-prompt.test.ts for the corrected single-comment
contract. docs/revanote.md + CLAUDE.md updated in the same commit.
check-baseline: fail=0 (pre-existing skip drift 259>256 unrelated,
left untouched per integrator note).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eiling Stall-alert flagged any annotation_runs row in_flight past 30min (REMO_REVANOTE_STALL_RUN_MAX_MS), but a batch-dispatched turn legitimately runs up to REMO_REVANOTE_BATCH_RUN_MAX_MS (default 2h) doing N comments' worth of branch/PR/CI/merge/redeploy work in one turn. A healthy batch run would false-alarm as a stuck session. Add runStallThresholdMs(isBatch) reading the live batchRunMaxMs() knob from batch-dispatch.ts (no duplicated default) for a batch run; single-annotation runs keep the existing 30min threshold. The SQL sweep now joins annotations to detect payload_raw.batch_id per row and applies the matching cutoff. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
F1 [BLOCKER]: verifyCommitOnRemote only checked GET /commits/{sha}, which
returns 200 for a commit reachable from ANY branch — so a pushed-but-unmerged
PR branch tip passed the resolved=true gate. Now fetches the repo's
default_branch then compares {default_branch}...{sha}; verified only when
compare status is 'identical' or 'behind'. 'ahead'/'diverged'/404/API error/
missing field all fail closed with a distinct reason. Updated prompt.ts
wording and docs/revanote.md to match.
F5 [MINOR]: process-manager.ts's three `void run.bridge.stop().catch(() => {})`
teardown sites swallowed a failed close silently, hiding the exact
"hub never learns the session died" failure mode this teardown exists to
prevent. Extracted a closeBridge() helper that logs the failure via the
file's existing onLog('warn', ...) pattern.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mappings
Five confirmed QC findings, all in the revanote batch-dispatch class:
- F2: dispatch() sent the batch prompt before the per-member
updateAnnotationStatus('dispatched') loop, so a crash mid-loop left
members pending under the same batch_id and the sweep re-dispatched
them after restart.
- F4: retry's reset-to-pending + forceSingle dispatch could race a sweep
tick grabbing the same batch_id-carrying row under a different token;
session-queue dedup is token-identity only and can't catch it.
- F3: a batch reply that parses as a single (non-array) object was
re-fed to the single-item parser, resolving every member from one
verdict instead of failing the whole batch.
- C3: inFlightBatches/the dispatch token was keyed by the raw batch_id,
which collides once one batch_id splits into several per-session (or,
after the C5 fix, per-mapping) groups, or repeats across users.
- C5: renderBatchAnnotationPrompt derived deploy trust/strategy from a
single member's mapping, but prod runs ~23 client sites -- each its
own mapping/trust -- sharing one session, so an untrusted site's
annotation could inherit a trusted site's direct/auto-merge plan.
Fixes: a single conditional `UPDATE annotations SET status='dispatching'
WHERE status='pending' RETURNING id` (claimAnnotationsForDispatch) runs
before ANY prompt is built or sent, on every dispatch path (single,
forceSingle retry, batch); a crash after a successful claim parks the
row at 'dispatching' rather than auto-reverting to pending, and
stall-alert.ts now alarms on a stuck 'dispatching' row. Batch sweep
sub-groups by (session, mapping) so every dispatched batch is
single-mapping, and the dispatch token is scoped
batch:<userId>:<sessionId>:<mappingId>:<batch_id>. A batch parse
failure now finalizes every member failed/resolved:false directly,
never via the single-item parser.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merges qcfix/commit-on-default (path-aware GitHub mock, default-branch commit verification) and qcfix/batch-claim (atomic pre-send claim, isolated batch dispatch tokens/mappings) into fix/revanote-reliability. Both merges auto-resolved with no conflicts; all revanote-* and process-manager* tests pass individually, check-baseline 0 fail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…R2-1/R2-2) Round-2 QC found the round-1 pre-send claim (pending->dispatching, before dispatch()) opened a new strand class: an annotation/batch merely QUEUED behind another in-flight send on the same session was already claimed even though nothing was ever sent for it. A hub restart drops the in-memory queue/waiter state that would have promoted and sent it, permanently stranding the row off the 'pending' sweep/retry path (R2-1). The claim now lives inside the pipeline's send() adapter, immediately before the WS push, for both a first dispatch and a promoted queue re-dispatch (dispatch() calls the same send() either way) - a merely-queued row never reaches it and stays 'pending'. The CAS transitions straight pending -> dispatched; the intermediate 'dispatching' claim state is removed entirely (AnnotationStatus union, schema.sql CHECK, stall-alert.ts). R2-2: POST /api/revanote/annotations/:id/retry no longer unconditionally resets a 'dispatched' row to 'pending' - it first checks pipeline.isTokenLive and batch-dispatch.isAnnotationLiveInBatch and refuses with 409 while the row's first turn is genuinely live, only allowing the reset once it's stranded (post-restart). R2-4: an unrecognized ?status= on GET /api/revanote/annotations now 400s instead of silently returning the unfiltered list. R2-3 investigated and refuted: the process-manager.ts bridge-teardown-on- ordinary-restart call site predates the revanote-reliability branch entirely (introduced by #96, long before f5dd003); f5dd003 only extracted the same call into a closeBridge() helper. No behavior change to revert. New tests: revanote-dispatch.test.ts (R2-1 single-path restart recovery), revanote-batch-dispatch.test.ts (R2-1 batch-path restart recovery), revanote-retry-guard.test.ts (R2-2 live-ownership 409, R2-4 400). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keeps main's pushed-commit verifier (#491) and layers the merged-to-default-branch requirement on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e claim, CAS retry reset) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nding Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…redundant status CHECK re-narrow; docs - Sweep re-dispatches pending non-batch rows no in-memory queue owns (REMO_REVANOTE_PENDING_REDISPATCH_MS, default 2min); fixes the docs claim of a stale-lease sweep. - Remove the DROP/ADD annotations_status_check pair (inline CHECK already has the same 5 values; main has no such pair). - Remove obsolete lifecycle commit-gate test of the superseded verifyCommitOnRemote API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tch, single doc for commit gate - batch/single send: check channel before claim; post-claim failure fails claimed members with run + callback - retry liveness via isTokenLiveAnywhere (session_id is NULL in the claim window) - single-path markSkipped CAS on pending - stale-pending sweep: exponential backoff, fail after 5 throws with callback - docs: one accurate commit-verify section; prompt says MERGED to default branch Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rget is a CAS on pending Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…class fix) - parked_offline, grace-expiry, post-send and send-failure writes are now compare-and-set; callbacks/broadcasts only when the row transitioned - grace registration deduped by dispatch token; batch sweep skips groups parked in grace while offline; session_offline rows re-eligible when no grace entry exists (restart recovery) in both sweeps - batch markFailed acts only on ids this send claimed - webhook pre-resolve writes only mapping_id - retry of a dispatched row checks durable annotation_runs state - merged-to-default is the whole commit-verify bar (drop branch containment) - send-time session_offline parks like parked_offline; stale-pending cap counts returned failed outcomes - stall alert covers dispatched rows with no in_flight run Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…neration A reply was correlated by session only, so a late reply from an earlier dispatch (hub restart, then another annotation on the same session) finalized the armed hook, and the old run's merged SHA passed the new run's commit check. - dispatch_id (the annotation_runs id; one uuid per batch turn) is stated in the single and batch prompts, REQUIRED in the reply envelope, and checked by the revanote shouldFinalize hooks: a missing/foreign id leaves the hook armed and the reply is dropped. Terminal-timeout finalizes also refuse a foreign id. - annotations.current_run_id (additive) is set at the send-time claim (batch: bindAnnotationRun); the finalize annotation write is a CAS on status='dispatched' AND current_run_id, written before the run row. A lost CAS writes nothing else. Retry closes the superseded in_flight run. - Retry durable-liveness ceiling uses the real hook max (REMO_DISPATCH_HOOK_MAX_MS for single runs) instead of the 20min narration timeout. - Batch grace replay re-gathers the CURRENT pending members instead of the list captured at park time. - commit-verify: new reason commit_predates_dispatch (committer date must be >= dispatched_at - 5min). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
AgentAutofix AI ReviewclaudePASSED
codexBLOCKED
agyadvisory -- not part of the gate decision INFRA FAILURE: reviewer CLI reported quota exhaustion Result: Blocking finding from: codex Reviewer models (requested pin, not independently confirmed):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
43 Revanote client comments were marked resolved citing a never-pushed commit. Owner rules: "resolved" only when the fix commit is MERGED to the default branch, deployed and verified live; all comments of one Revanote run (shared payload_raw.batch_id) go to the site session as ONE prompt and ONE PR.
What changed
QC history
7 rounds of cross-model adversarial QC (Claude lenses + Codex). Rounds 1-6 findings were all fixed (claim races, retry-vs-sweep, token collisions, mixed-trust grouping, stranded queued rows, stale-snapshot status writes, reply-to-generation correlation). Round 7 (Adversary/Integrator + Codex): no confirmed blocker. Codex's one BLOCKER (postgres.js
${ids}::uuid[]binding) was refuted: identical pattern is in production at hub/src/db/dal.ts:768. Open non-blocking MINORs: (a) concurrent double-retry CAS checks status only, not current_run_id; (b) grace replay bypasses batch debounce (could split a late member into another turn); (c) commit_predates_dispatch may use a prior generation's dispatched_at (lenient, narrow race); (d) a foreign reply arriving after the dispatch's own finalize timeout fails that already-timed-out run as dispatch_id_mismatch.Tests
Each hub/test/revanote-, dispatch-, token-cap-coverage file passes individually; all 292 hub test files pass individually from hub/;
bun run check-baselineOK (pass=2404 fail=0; one earlier run showed a single transient fail that did not reproduce). human-interactive-flag-guard passes now. Not run locally: the claim SQL against real Postgres (no docker/postgres locally); Woodpecker postgres:16 e2e is the check.🤖 Generated with Claude Code