Skip to content

feat(007): robust review dispatch — per-type lock + post-completion hook#1136

Merged
zbigniewsobiecki merged 7 commits intodevfrom
feat/007-robust-review-dispatch
Apr 17, 2026
Merged

feat(007): robust review dispatch — per-type lock + post-completion hook#1136
zbigniewsobiecki merged 7 commits intodevfrom
feat/007-robust-review-dispatch

Conversation

@zbigniewsobiecki
Copy link
Copy Markdown
Member

Summary

Addresses the silent review drop observed on MNG-122/PR-572: the review agent was never dispatched because the work-item lock's total-concurrency cap blocked it while 2 other agents were enqueued for the same work item. The blocked check-suite-success webhooks were silently discarded with no retry.

Spec 007 delivers two intersecting fixes across two plans.

Plan 1 — Lock infrastructure (per-agent-type)

Removes MAX_WORK_ITEM_CONCURRENCY total cap from isWorkItemLocked. Only the per-type cap (MAX_SAME_TYPE_PER_WORK_ITEM = 1) remains. Different agent types can now run concurrently on the same work item.

  • src/router/work-item-lock.ts — deleted MAX_WORK_ITEM_CONCURRENCY, removed total-count checks (in-memory + DB), simplified getInMemoryCountsgetInMemorySameTypeCount (single key lookup, no iteration), removed the dbTotal query (saves one DB round-trip per lock check), deleted dead keyPrefix helper.
  • src/router/webhook-processor.ts — enriched lock-skip log with source and blockedAgentType.
  • Net: -33 lines (simplification).

Plan 2 — Post-completion review dispatch

When an implementation agent succeeds with a PR, the execution pipeline now checks CI status and fires the review agent before the container exits.

  • src/triggers/shared/agent-execution.ts — new tryDispatchPostCompletionReview function. Uses claimReviewDispatch with the same dedup key format as check-suite-success, so the two paths cannot double-enqueue. Uses the recursive runAgentExecutionPipeline pattern (same as splitting → backlog-manager chain). Best-effort: errors are logged as warn but never break the implementation pipeline.

Tests

  • Plan 1: +2 new cross-type concurrency tests, ~6 updated existing tests reflecting the total-cap removal. 16 work-item-lock tests total.
  • Plan 2: +7 new tests covering: fires review on success + green CI, skips for non-implementation, skips on failure, skips when no prUrl, skips when CI not green, skips when already dispatched, swallows errors gracefully.
  • Total: 7859 unit tests pass. Lint + typecheck + build clean.

Documentation

CLAUDE.md updated under "Agent triggers" with:

  • Per-agent-type lock semantics note
  • Post-completion review dispatch documentation

Out of scope

  • ❌ Enabling pr-opened for review on any project (per-project config decision)
  • ❌ Lock persistence to Redis/DB (in-memory with 30-min TTL is sufficient)
  • ❌ Generalizing the hook to other chains (splitting → planning, etc.)
  • ❌ Distributed lock across multiple router instances (single-process deployment)

Test plan

  • npm test (7859 pass)
  • npm run typecheck, npm run lint clean
  • After deploy: trigger a Linear-backed implementation run on llmist, watch the review agent fire from the post-completion hook within seconds of implementation completion — no reliance on check-suite-success webhook timing.

🤖 Generated with Claude Code

zbigniewsobiecki and others added 7 commits April 17, 2026 06:47
Spec 007 addresses the silent review drop observed on MNG-122/PR-572:
the work-item lock's total-concurrency cap (MAX_WORK_ITEM_CONCURRENCY=2)
blocked review dispatch while other agents were enqueued for the same
work item. Three intersecting fixes specified: per-agent-type locking,
lock release timing, and a post-completion review hook.

Two plans:
- 007/1 (lock-infra): remove MAX_WORK_ITEM_CONCURRENCY total cap,
  keep per-type MAX_SAME_TYPE_PER_WORK_ITEM=1, enrich lock-skip log.
- 007/2 (post-completion-review): deterministic review dispatch from
  the implementation pipeline after success + green CI.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…serialization

Removes the MAX_WORK_ITEM_CONCURRENCY total cap from isWorkItemLocked.
The total cap falsely serialized unrelated agent types: the review for
MNG-122/PR-572 was silently dropped because 2 agents (implementation +
backlog-manager) were already enqueued for the same work item, hitting
the total limit of 2.

Now only MAX_SAME_TYPE_PER_WORK_ITEM = 1 is enforced. Different agent
types can run concurrently on the same work item (e.g. review starts
while implementation's container is still cleaning up). Same-type
duplicate prevention is preserved.

Changes:
- src/router/work-item-lock.ts — deleted MAX_WORK_ITEM_CONCURRENCY
  constant and the total-count checks (in-memory + DB). Simplified
  getInMemoryCounts → getInMemorySameTypeCount (no longer iterates all
  keys). Removed the dbTotal query (saves one DB round-trip per lock
  check). Deleted the unused keyPrefix helper.
- src/router/webhook-processor.ts — enriched the lock-skip log with
  source (adapter type) and renamed agentType → blockedAgentType for
  clarity.
- CLAUDE.md — added per-agent-type lock semantics note under "Agent
  triggers".

Tests: updated 6 existing tests + added 2 new cross-type concurrency
tests. All 7852 unit tests pass. Lint + typecheck clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fter implementation

When an implementation agent succeeds with a PR, the execution pipeline
now checks CI status and fires the review agent before the container
exits. This guarantees review dispatch within seconds of implementation
completion, regardless of GitHub webhook timing.

Uses the same recursive `runAgentExecutionPipeline` pattern as the
splitting → backlog-manager chain. The review runs in the same container,
same credential scope. Uses `claimReviewDispatch` with the same dedup key
format as the `check-suite-success` trigger, so the two paths cannot
double-enqueue.

The hook is best-effort: GitHub API failures, Redis errors, or any
exception is caught, logged as warn, and does NOT break the
implementation pipeline.

New function `tryDispatchPostCompletionReview` in agent-execution.ts:
1. Extracts prNumber from agentResult.prUrl
2. Fetches PR details (headSha, headRef) from GitHub
3. Checks CI status via getCheckSuiteStatus — if not allPassing, returns
   (check-suite-success webhook will handle it when CI finishes)
4. Claims the dedup key via claimReviewDispatch — if already claimed,
   returns (review was already dispatched by the webhook path)
5. Builds a review TriggerResult and calls runAgentExecutionPipeline
   recursively (same pattern as splitting → backlog-manager)

Tests: +7 new tests covering: fires review on success + green CI, skips
for non-implementation, skips on failure, skips when no prUrl, skips when
CI not green, skips when already dispatched, swallows errors gracefully.
All 7859 unit tests pass. Lint + typecheck clean.

CLAUDE.md updated with post-completion review dispatch documentation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
npm audit flagged protobufjs <7.5.5 for arbitrary code execution
(GHSA-xq3m-2v4x-88gg). Updated via `npm update protobufjs` — lockfile
only, no package.json change.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@zbigniewsobiecki zbigniewsobiecki merged commit 1225ea8 into dev Apr 17, 2026
8 checks passed
@zbigniewsobiecki zbigniewsobiecki deleted the feat/007-robust-review-dispatch branch April 17, 2026 07:35
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

❌ Patch coverage is 98.95833% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/triggers/shared/agent-execution.ts 98.71% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant