feat(workflows): add label-dispatched bot workflow foundation - #49
Merged
Conversation
Introduce the definitive bot-workflow pipeline: webhook labels trigger registry-driven workflow runs (triage/plan/implement/review/ship) that piggyback on the existing daemon job queue via an optional workflowRun field. Adds workflow_runs table (migration 005) as the authoritative per-run state store, a tracking-comment mirror, a label mutex enforcing at-most-one active bot:* label, and an MVP keyword-heuristic triage handler. plan/implement/review/ship handlers are stub scaffolds. Also removes the redundant Dependabot-era merge-dependencies workflow since Renovate's platformAutomerge + zero-approval ruleset cover the same ground.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (39)
📝 WalkthroughWalkthroughThis PR introduces a complete, spec-driven bot workflow orchestration system. It defines five named workflows (triage, plan, implement, review, ship) with persistent run state stored in a PostgreSQL Changes
Sequence DiagramsequenceDiagram
actor GitHub
participant Webhook
participant Handler
participant Dispatcher
participant Queue
participant Daemon
participant Executor
participant DB
participant Mirror
GitHub->>Webhook: issue.labeled / pr.labeled event
Webhook->>Handler: parseEvent & route
Handler->>Handler: validate sender, label format
Handler->>Dispatcher: dispatchByLabel()
Dispatcher->>DB: findLatestForTarget() check prior
Dispatcher->>Dispatcher: enforceSingleBotLabel()
Dispatcher->>DB: insertQueued() idempotent insert
Dispatcher->>Queue: enqueueJob(workflowRun ref)
Dispatcher->>Handler: return {status, runId}
Daemon->>Queue: fetchJob()
Daemon->>Executor: executeWorkflowRun(payload)
Executor->>DB: markRunning(runId)
Executor->>Executor: resolve handler by workflowName
Executor->>Executor: invoke handler(WorkflowRunContext)
Executor->>DB: markSucceeded() or markFailed()
Executor->>Mirror: setState() merge state & update comment
Mirror->>DB: mergeState()
Mirror->>GitHub: createComment() or updateComment()
Executor->>Daemon: job:result message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chrisleekr
marked this pull request as draft
April 21, 2026 11:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/ws-messages.ts (1)
54-78:⚠️ Potential issue | 🟠 MajorGate workflow jobs from older daemons to prevent silent failures during rolling deployments.
The optional
workflowRunfield changes job semantics whilePROTOCOL_VERSIONremains1.0.0. An older daemon can accept the payload, ignore the unknown field, and attempt execution only to fail whengetByName(workflowRun.workflowName)throws "Workflow registry entry not found" — this error is caught and logged but leaves the workflow run in a failed state instead of being rejected or properly routed.The dispatcher offers workflow jobs without filtering by daemon app version or a workflow-support capability flag. During mixed rollouts, old daemons will silently fail newly-dispatched workflow runs.
Either bump the protocol version to signal the breaking change or add a workflow capability flag to the daemon registration schema and gate job dispatch accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ws-messages.ts` around lines 54 - 78, The optional workflowRun field (workflowRunRefSchema / workflowRun) introduces a breaking semantic change while PROTOCOL_VERSION stays at "1.0.0", causing older daemons to accept then fail workflow jobs; fix by either (A) bumping PROTOCOL_VERSION to a new value and update any protocol checks so dispatchers and daemons negotiate the new version, or (B) add a workflow-support capability to the daemon registration schema and update the dispatcher to only route jobs with workflowRun to daemons advertising that capability (also update dispatcher logic that selects targets to check the new capability); locate references to workflowRun/workflowRunRefSchema, PROTOCOL_VERSION, dispatcher selection logic, and the daemon registration/type definitions to implement one of these two fixes.
🧹 Nitpick comments (4)
test/workflows/dispatcher.test.ts (1)
141-154: Assert the mutex is skipped whenrequiresPriorrefuses.This test should also verify
mockEnforceSingleBotLabelwas not called. Otherwise a dispatcher regression could remove the current activebot:*label before refusingbot:planfor missing triage.Proposed test assertion
expect(mockFindLatestForTarget).toHaveBeenCalledTimes(1); + expect(mockEnforceSingleBotLabel).not.toHaveBeenCalled(); expect(mockPostRefusalComment).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/workflows/dispatcher.test.ts` around lines 141 - 154, Update the test for "refuses when requiresPrior is unsatisfied (bot:plan without a successful triage)" to also assert that mockEnforceSingleBotLabel was not called: after calling dispatchByLabel and the existing expectations, add an expectation that mockEnforceSingleBotLabel.toHaveBeenCalledTimes(0) (or .not.toHaveBeenCalled()) so the test verifies the dispatcher skips enforcing the single-bot-label mutex when requiresPrior causes a refusal; locate this check near other mock assertions referencing mockFindLatestForTarget, mockPostRefusalComment, mockInsertQueued, and mockEnqueueJob in the same test.test/workflows/runs-store.test.ts (1)
213-217: Tighten the negative-path assertion.
expectToReject(..., "")accepts any rejection — including an unrelated error like a dropped connection or a check-constraint failure — which would let regressions pass. Match on the Postgres unique-violation signature instead.♻️ Proposed refinement
await expectToReject( insertQueued({ workflowName: "triage", target: { ...target, number: 108 } }, requireSql()), - "", + /idx_workflow_runs_inflight|duplicate key|unique/i, );(adjust the substring/regex to what
expectToRejectaccepts in this codebase)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/workflows/runs-store.test.ts` around lines 213 - 217, The negative-path assertion is too loose: replace the empty-string guard in the expectToReject call that wraps insertQueued({ workflowName: "triage", target: { ...target, number: 108 } }, requireSql()) with a more specific matcher for Postgres unique-violation (e.g., the "duplicate key value violates unique constraint" message or SQLSTATE 23505) so the test only passes on the intended uniqueness error; keep using expectToReject and requireSql() but tighten the substring/regex to the codebase's expectToReject matcher.src/workflows/registry.ts (1)
80-89: Optional: forbid step self-reference / cycles inRegistrySchema.The refinements validate step names exist but don't prevent a composite workflow from listing itself (or forming a cycle) in
steps. With the currentrawRegistrythis is fine, but a future edit adding"ship"toship.stepswould pass schema validation and produce infinite recursion at hand-off time. Consider an additional refine:♻️ Proposed guard
.refine((entries) => entries.every((e) => e.steps.length === 0 || e.requiresPrior === null), { message: "composite workflows (non-empty steps) must have requiresPrior === null", - }); + }) + .refine((entries) => entries.every((e) => !e.steps.includes(e.name)), { + message: "a workflow must not list itself in steps", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/registry.ts` around lines 80 - 89, The current RegistrySchema refinements check that step names exist but do not prevent self-references or cycles (e.g., a workflow listed in its own steps or circular chains), which can cause infinite recursion later; update the schema (RegistrySchema / the .refine chain operating on entries) to add a refinement that builds a map from workflow name to its steps (using the same entries -> names logic) and then performs a cycle-detection (DFS or Kahn’s algorithm) to ensure no node has an edge to itself and the directed graph is acyclic; return false (with a clear message like "workflows must not self-reference or form cycles") when any self-edge or cycle is found.src/workflows/runs-store.ts (1)
98-140: Add status guards to terminal transitions (consistency withmarkRunning).
markRunningcorrectly gates onstatus = 'queued', butmarkSucceededandmarkFailedupdate unconditionally. A duplicate/late terminal write (e.g. a retried job or a racing error path after success) can silently flip asucceededrow tofailedor vice-versa, and will also clobber the earlier terminalstatevia JSONB concat. Consider restricting terminal writes to non-terminal rows so the first terminal status wins deterministically.♻️ Proposed guard
await sql` UPDATE workflow_runs SET status = 'succeeded', state = state || ${state}::jsonb WHERE id = ${runId} + AND status IN ('queued', 'running') `;await sql` UPDATE workflow_runs SET status = 'failed', state = state || ${merged}::jsonb WHERE id = ${runId} + AND status IN ('queued', 'running') `;Optionally return a boolean from all three lifecycle helpers (based on
rowCount) so the daemon can log/observe unexpected no-op transitions rather than swallow them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/runs-store.ts` around lines 98 - 140, markSucceeded and markFailed must be guarded like markRunning so terminal updates don't overwrite an already-terminal row; change both functions (and optionally markRunning) to add a WHERE clause that restricts terminal transitions to rows whose status is not already terminal (e.g., exclude 'succeeded' and 'failed') so the first terminal write wins deterministically, and update the functions to return a boolean (based on the SQL rowCount) to signal whether the update actually applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/20260421-181205-bot-workflows/data-model.md`:
- Around line 103-116: The triage example's "state" object uses an invalid
verdict "valid"; update the example to match the handler's shape by replacing
the "verdict" with one of the handler's allowed values ("bug", "feature",
"question", or "unclear") and add the required "rationale" field (e.g.,
"verdict": "bug", "rationale": "short explanation"); keep other keys like
"recommendedNext" and "tracking_comment_id" unchanged so the example aligns with
the triage handler's output shape.
- Around line 84-92: The code fence containing the ASCII state diagram (the
block with lines including "queued", "running", "succeeded", and "failed") needs
an explicit fence language to satisfy MD040; update the opening triple backticks
to include the language `text` so the diagram is fenced as ```text and leave the
diagram content unchanged.
In `@specs/20260421-181205-bot-workflows/quickstart.md`:
- Around line 28-50: The documentation currently describes comment triggers and
full `bot:ship` orchestration (including the `triage → plan → implement →
review` handoff, resume behavior, and enforced `review` stop) as implemented;
update the sections around "Trigger one workflow at a time (comment)" and "Drive
the full pipeline with one label" to mark any functionality that is not yet
implemented as "Planned" (or remove the detailed behavior) — specifically call
out `bot:ship`, the resume/handoff behavior, and the final tracking
comment/merge stop as planned. Ensure references to comment triggers (e.g., the
`@chrisleekr-bot please plan this` example), label dispatch (`Apply label
bot:ship`), and the step sequence (`triage → plan → implement → review`) either
link to a "Planned features" subsection or are trimmed to only describe
currently implemented triage/label dispatch behavior; do the same for the other
occurrences noted (lines ~63-70 and ~93-98).
- Around line 56-61: Update the wording that currently asserts the DB row write
and the GitHub tracking comment are the “same unit of work”: change it to state
that the table workflow_runs (see example SELECT * FROM workflow_runs ...) is
the authoritative source of truth and that the tracking comment on the issue/PR
is an external GitHub projection and only a best-effort, eventually consistent
view which can lag or fail independently of the DB write; replace language
claiming atomicity with language clarifying the comment may be delayed or
missing and recommend relying on workflow_runs for authoritative state.
In `@specs/20260421-181205-bot-workflows/research.md`:
- Line 23: The unique index currently includes status in its key which allows
separate queued and running rows; change the index definition so it indexes only
(workflow_name, target_owner, target_repo, target_number) and move status into
the partial predicate: CREATE UNIQUE INDEX ... ON ... (workflow_name,
target_owner, target_repo, target_number) WHERE status IN ('queued','running');
update any migration/ORM code that creates the "unique index on (workflow_name,
target_owner, target_repo, target_number, status)" to use this new key+partial
predicate (referencing the table columns id, workflow_name, target_type,
target_owner, target_repo, target_number, status, state, tracking_comment_id) so
FR-011 idempotency is enforced across status transitions.
In `@specs/20260421-181205-bot-workflows/tasks.md`:
- Line 67: The dispatcher currently calls label-mutex.enforceSingleBotLabel
before checking requiresPrior, which can remove an active bot label even if the
new run will be refused; update dispatchByLabel (and dispatchByIntent) to
perform registry lookup → context check → requiresPrior (using
requiresPrior(...) and runs-store.findLatestForTarget) and only after passing
prerequisite checks call label-mutex.enforceSingleBotLabel, then
runs-store.insertQueued and publish job; ensure all refusal branches still post
a single refusal comment via tracking-mirror and do not mutate labels when
requiresPrior fails.
- Line 14: Replace the developer-local absolute path
"/Users/chrislee/srv/github/github-app-playground/" in the tasks.md content with
a repo-relative path (e.g., start paths with "./" or the repo root placeholder)
so the task list is portable; update the line that currently reads "Paths below
are absolute from repo root
`/Users/chrislee/srv/github/github-app-playground/`." to use a repo-relative
reference instead.
In `@src/daemon/workflow-executor.ts`:
- Around line 83-147: The code currently treats any error (including failures in
setState or GitHub comment mirroring) as a workflow failure because the outer
catch marks the run failed; to fix, separate handler execution from terminal
persistence: after entry.handler returns "succeeded" call
markSucceeded(workflowRun.runId) and send(...) immediately to report success,
then perform setState(...) (and other mirror updates) inside their own try/catch
so any exceptions are logged and retried but not rethrown; do not let setState
errors propagate to the outer catch or call markFailed for those mirror errors.
Similarly, ensure the failed branch still calls markFailed(...) on handler
failure, but mirror-posting errors there should also be handled locally and not
convert a handler failure into a different terminal state.
In `@src/db/migrations/005_workflow_runs.sql`:
- Line 12: The migration file includes an explicit "BEGIN;" which conflicts with
the outer transaction in migrate.ts; remove the explicit transaction control
statements from the migration SQL (delete the "BEGIN;" at the top of
005_workflow_runs.sql and the matching "COMMIT;" in
004_collapse_dispatch_to_daemon.sql) so the wrapper transaction (conn.begin in
migrate.ts) manages atomicity and rollback instead of nested BEGIN/COMMIT in the
migration files.
In `@src/workflows/dispatcher.ts`:
- Around line 82-132: The current broad try/catch around insertQueued +
enqueueJob masks real errors and treats all failures as "in-flight" collisions;
refactor so insertQueued is called inside its own try/catch that detects a
unique-violation (Postgres error code '23505' and constraint
'idx_workflow_runs_inflight') and, on that specific condition, post the refusal
via postRefusalComment and return the "refused" status, while letting any other
insert error propagate; then call enqueueJob in a separate try/catch—if
enqueueJob fails after insertQueued succeeded, ensure you clean up or transition
the created runRow (inserted by insertQueued) to avoid orphaned queued rows
(e.g., delete or set status to an error state) and rethrow or handle the error
distinctly instead of posting the in-flight message; use the existing symbols
insertQueued, enqueueJob, runRow, postRefusalComment, logger and check err.code
=== '23505' && err.constraint === 'idx_workflow_runs_inflight' to identify the
idempotency collision.
- Around line 73-80: The gate uses findLatestForTarget when checking
entry.requiresPrior so it refuses if the most recent prior run failed even if an
earlier success exists; change the logic to check for any successful prior run
instead. Implement or call a helper (e.g., findLatestSucceededForTarget or
findAnySucceededForTarget) that queries workflow runs and returns a succeeded
run for the given entry.requiresPrior and target, replace the
findLatestForTarget call in the requiresPrior branch inside dispatcher.ts, and
keep the existing postRefusalComment, reason, and return behavior when no
succeeded run is found so the refusal semantics match webhook-dispatch.md step
4.
In `@src/workflows/label-mutex.ts`:
- Around line 46-60: The current loop that removes sibling bot:* labels (using
others, justApplied and octokit.rest.issues.removeLabel) can race between
concurrent webhooks and let an older handler remove a newer label; before
removing siblings, implement a per-target serialization or winner-check: acquire
a short-lived mutex keyed by owner/repo/number (or persist a winning-label
record) when handling the webhook, record this handler’s candidate (justApplied
plus event timestamp or delivery id) as the current winner, then only proceed to
remove labels if the persisted/mutex-stored winner matches this handler;
otherwise skip removals. Ensure BOT_LABEL_PATTERN, others and removed logic only
runs after confirming the handler is the recorded winner to make “newest label
wins” deterministic.
In `@src/workflows/tracking-mirror.ts`:
- Around line 61-73: Make the tracking-comment creation idempotent by
introducing a compare-and-set or reservation step around
setTrackingCommentId/createComment: before creating a GitHub comment, attempt an
atomic DB update that reserves the tracking_comment_id for this run (e.g.
setTrackingCommentIdIfNull(runId, "reserved") or a CAS that sets
tracking_comment_id from null to a sentinel); if the reservation fails read and
reuse the existing tracking_comment_id from the row; if the reservation succeeds
then call octokit.rest.issues.createComment and then atomically replace the
sentinel with the real created.data.id (or, if CAS after create fails, delete
the newly created comment to avoid duplicates). Update usages of
setTrackingCommentId and the code path that checks row.tracking_comment_id so
the new compare-and-set/reserve helper is used instead of relying on a plain
null check.
In `@test/workflows/registry.test.ts`:
- Around line 58-63: The duplicate-name test uses a second entry label
"bot:triage2" that violates the label regex ^bot:[a-z]+$ and causes validation
to fail before the duplicate-name invariant; update the fixture in the test (the
array passed to RegistrySchema.parse in the it block) so both entries use labels
that match the regex (for example "bot:triage" and "bot:triageb" or another
all-lowercase label) while keeping the same name "triage", ensuring makeEntry
and RegistrySchema.parse exercise the duplicate-name check.
- Line 22: Replace the fragile type used in makeEntry—Partial<Parameters<typeof
RegistrySchema.parse>[0][number]>—with the Zod input inference pattern using
z.input to get the array element type (e.g. Partial<z.input<typeof
RegistrySchema>[number]>); update the makeEntry signature accordingly and
add/import z from 'zod' if not already imported so the type correctly represents
a registry entry.
---
Outside diff comments:
In `@src/shared/ws-messages.ts`:
- Around line 54-78: The optional workflowRun field (workflowRunRefSchema /
workflowRun) introduces a breaking semantic change while PROTOCOL_VERSION stays
at "1.0.0", causing older daemons to accept then fail workflow jobs; fix by
either (A) bumping PROTOCOL_VERSION to a new value and update any protocol
checks so dispatchers and daemons negotiate the new version, or (B) add a
workflow-support capability to the daemon registration schema and update the
dispatcher to only route jobs with workflowRun to daemons advertising that
capability (also update dispatcher logic that selects targets to check the new
capability); locate references to workflowRun/workflowRunRefSchema,
PROTOCOL_VERSION, dispatcher selection logic, and the daemon registration/type
definitions to implement one of these two fixes.
---
Nitpick comments:
In `@src/workflows/registry.ts`:
- Around line 80-89: The current RegistrySchema refinements check that step
names exist but do not prevent self-references or cycles (e.g., a workflow
listed in its own steps or circular chains), which can cause infinite recursion
later; update the schema (RegistrySchema / the .refine chain operating on
entries) to add a refinement that builds a map from workflow name to its steps
(using the same entries -> names logic) and then performs a cycle-detection (DFS
or Kahn’s algorithm) to ensure no node has an edge to itself and the directed
graph is acyclic; return false (with a clear message like "workflows must not
self-reference or form cycles") when any self-edge or cycle is found.
In `@src/workflows/runs-store.ts`:
- Around line 98-140: markSucceeded and markFailed must be guarded like
markRunning so terminal updates don't overwrite an already-terminal row; change
both functions (and optionally markRunning) to add a WHERE clause that restricts
terminal transitions to rows whose status is not already terminal (e.g., exclude
'succeeded' and 'failed') so the first terminal write wins deterministically,
and update the functions to return a boolean (based on the SQL rowCount) to
signal whether the update actually applied.
In `@test/workflows/dispatcher.test.ts`:
- Around line 141-154: Update the test for "refuses when requiresPrior is
unsatisfied (bot:plan without a successful triage)" to also assert that
mockEnforceSingleBotLabel was not called: after calling dispatchByLabel and the
existing expectations, add an expectation that
mockEnforceSingleBotLabel.toHaveBeenCalledTimes(0) (or .not.toHaveBeenCalled())
so the test verifies the dispatcher skips enforcing the single-bot-label mutex
when requiresPrior causes a refusal; locate this check near other mock
assertions referencing mockFindLatestForTarget, mockPostRefusalComment,
mockInsertQueued, and mockEnqueueJob in the same test.
In `@test/workflows/runs-store.test.ts`:
- Around line 213-217: The negative-path assertion is too loose: replace the
empty-string guard in the expectToReject call that wraps insertQueued({
workflowName: "triage", target: { ...target, number: 108 } }, requireSql()) with
a more specific matcher for Postgres unique-violation (e.g., the "duplicate key
value violates unique constraint" message or SQLSTATE 23505) so the test only
passes on the intended uniqueness error; keep using expectToReject and
requireSql() but tighten the substring/regex to the codebase's expectToReject
matcher.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a0f85e3-66c1-4da3-a06d-5f52152f2c69
📒 Files selected for processing (43)
.github/workflows/merge-dependencies.ymlCLAUDE.mdeslint.config.mjsspecs/20260421-181205-bot-workflows/checklists/requirements.mdspecs/20260421-181205-bot-workflows/contracts/handoff-protocol.mdspecs/20260421-181205-bot-workflows/contracts/registry.schema.tsspecs/20260421-181205-bot-workflows/contracts/webhook-dispatch.mdspecs/20260421-181205-bot-workflows/contracts/workflow_runs.sqlspecs/20260421-181205-bot-workflows/data-model.mdspecs/20260421-181205-bot-workflows/plan.mdspecs/20260421-181205-bot-workflows/quickstart.mdspecs/20260421-181205-bot-workflows/research.mdspecs/20260421-181205-bot-workflows/spec.mdspecs/20260421-181205-bot-workflows/tasks.mdsrc/app.tssrc/daemon/job-executor.tssrc/daemon/workflow-executor.tssrc/db/migrations/005_workflow_runs.sqlsrc/orchestrator/connection-handler.tssrc/orchestrator/job-dispatcher.tssrc/orchestrator/job-queue.tssrc/shared/daemon-types.tssrc/shared/workflow-types.tssrc/shared/ws-messages.tssrc/webhook/events/issues.tssrc/webhook/events/pull-request.tssrc/workflows/dispatcher.tssrc/workflows/handlers/implement.tssrc/workflows/handlers/plan.tssrc/workflows/handlers/review.tssrc/workflows/handlers/ship.tssrc/workflows/handlers/triage.tssrc/workflows/label-mutex.tssrc/workflows/registry.tssrc/workflows/runs-store.tssrc/workflows/tracking-mirror.tstest/db/migrate.test.tstest/webhook/events/issues.test.tstest/workflows/dispatcher.test.tstest/workflows/handlers/triage.test.tstest/workflows/label-mutex.test.tstest/workflows/registry.test.tstest/workflows/runs-store.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/merge-dependencies.yml
Comment on lines
+28
to
+50
| ## 2. Trigger one workflow at a time (comment) | ||
|
|
||
| ```text | ||
| @chrisleekr-bot please plan this | ||
| ``` | ||
|
|
||
| The bot runs its intent classifier, recognises "plan", and dispatches the `plan` workflow exactly as if you had applied `bot:plan`. If the ask is ambiguous, the bot replies with a single clarifying question rather than guessing. | ||
|
|
||
| ## 3. Drive the full pipeline with one label | ||
|
|
||
| ```text | ||
| Apply label bot:ship on an open issue. | ||
| ``` | ||
|
|
||
| The bot runs: | ||
|
|
||
| ```text | ||
| triage → plan → implement → review | ||
| ``` | ||
|
|
||
| Each step is a separate queued job that hands off to the next one on success. When the PR is merge-ready the final tracking comment says so and stops. **The bot never merges** — that remains your action. | ||
|
|
||
| If any step fails, the bot halts with a tracking-comment entry naming the failed step and reason. You resume by **re-applying `bot:ship`** — the bot reads the run store, finds the last successful step, and continues from the next one. |
There was a problem hiding this comment.
Mark unwired workflows as planned instead of available.
These sections describe comment triggers, ship step handoff/resume, enforced review stop bounds, and integration coverage as current behavior, but this PR summary says only label dispatch + triage are implemented and ship orchestration/resume is not wired yet. Please move these into a “planned” section or trim them until the handlers/orchestration/tests exist.
Also applies to: 63-70, 93-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/20260421-181205-bot-workflows/quickstart.md` around lines 28 - 50, The
documentation currently describes comment triggers and full `bot:ship`
orchestration (including the `triage → plan → implement → review` handoff,
resume behavior, and enforced `review` stop) as implemented; update the sections
around "Trigger one workflow at a time (comment)" and "Drive the full pipeline
with one label" to mark any functionality that is not yet implemented as
"Planned" (or remove the detailed behavior) — specifically call out `bot:ship`,
the resume/handoff behavior, and the final tracking comment/merge stop as
planned. Ensure references to comment triggers (e.g., the `@chrisleekr-bot
please plan this` example), label dispatch (`Apply label bot:ship`), and the
step sequence (`triage → plan → implement → review`) either link to a "Planned
features" subsection or are trimmed to only describe currently implemented
triage/label dispatch behavior; do the same for the other occurrences noted
(lines ~63-70 and ~93-98).
Owner
Author
There was a problem hiding this comment.
Disagreement — US2 (composite ship), US3 (comment-intent classifier + dispatchByIntent), and US4 (docs + CI guard) are all implemented in this PR. See commits a1bbf94 (US4 BOT-WORKFLOWS.md), c46649a (T046/T048/T048a doc-sync CI guard and polish), d79fb75 (US3 comment-intent classifier + dispatcher), 52e5078 (US2 composite ship orchestration T029-T033). The quickstart language therefore matches shipped behaviour; no 'planned' qualifier needed.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the current state of the relevant files in the PR branch.
🏁 Script executed:
#!/bin/bash
# Check dispatchByIntent implementation
echo "=== dispatcher.ts dispatchByIntent ==="
rg -n "dispatchByIntent" --type ts -A 10
echo ""
echo "=== ship handler ==="
cat src/workflows/handlers/ship.ts 2>/dev/null || echo "file not found"
echo ""
echo "=== intent classifier ==="
fd "intent-classifier" --type f
cat src/workflows/intent-classifier.ts 2>/dev/null || fd "intent-classifier" --exec cat {}
echo ""
echo "=== composite orchestration in workflow-executor ==="
rg -n "step|composite|ship|handoff|currentStepIndex" src/daemon/workflow-executor.ts -A 3 2>/dev/null | head -80
echo ""
echo "=== BOT-WORKFLOWS.md ==="
fd "BOT-WORKFLOWS.md" --type f
echo ""
echo "=== doc-sync CI guard ==="
fd "doc-sync" --type f
ls .github/workflows/ 2>/dev/null
echo ""
echo "=== git log --oneline last 20 commits ==="
git log --oneline -20
Length of output: 21648
@chrisleekr, you're right — I apologize for the incorrect flag. Inspecting the current branch confirms all three user stories are fully shipped:
- US2 (
shipcomposite orchestration) —src/workflows/handlers/ship.tscontains the completecomputeStartIndex/isFresh/insertChildRow/enqueueJobhand-off pipeline with resume semantics, not a stub. - US3 (comment-intent classifier) —
src/workflows/intent-classifier.tsis fully wired with real LLM calls, prompt-injection hardening, and Zod-validated output;dispatchByIntentindispatcher.ts(line 180+) callsclassify()and routes through the full dispatch pipeline. - US4 (docs + CI guard) —
docs/BOT-WORKFLOWS.mdexists anddocs.ymlprovides the doc-sync CI guard.
My original comment was based solely on the initial foundation commit (3c59a38) and did not account for the subsequent commits (52e5078, d79fb75, a1bbf94, c46649a). The quickstart accurately describes the shipped behaviour. No changes needed.
🧠 Learnings used
Learnt from: chrisleekr
Repo: chrisleekr/github-app-playground PR: 35
File: src/daemon/job-executor.ts:193-196
Timestamp: 2026-04-19T20:58:15.944Z
Learning: In `src/daemon/job-executor.ts` (chrisleekr/github-app-playground), threading `AbortSignal` through `runPipeline` (and through Octokit GraphQL, Claude Agent SDK, git CLI) to fix the active-job accounting gap after `handleJobCancel()` is intentionally deferred to a follow-up PR. The current safety nets are: idle shutdown is gated on `activeJobs > 0`, and K8s `activeDeadlineSeconds: 3600` acts as the outer hard ceiling. Do not flag this as a blocker for the current PR.
Comment on lines
+46
to
+60
| const others = response.data | ||
| .map((l) => l.name) | ||
| .filter((name) => BOT_LABEL_PATTERN.test(name) && name !== justApplied); | ||
|
|
||
| const removed: string[] = []; | ||
|
|
||
| for (const label of others) { | ||
| try { | ||
| // eslint-disable-next-line no-await-in-loop -- serial removal is fine at ≤ a handful of labels | ||
| await octokit.rest.issues.removeLabel({ | ||
| owner, | ||
| repo, | ||
| issue_number: number, | ||
| name: label, | ||
| }); |
There was a problem hiding this comment.
Make “newest label wins” deterministic under concurrent webhooks.
This removes every sibling label based only on the current handler’s justApplied. If an older bot:* labeled webhook is processed after a newer one, the older handler can remove the newer label, so execution order wins instead of label application order. Consider serializing per target and comparing event time/delivery metadata before removing siblings, or make the dispatcher persist the winning label decision before calling the mutex.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/workflows/label-mutex.ts` around lines 46 - 60, The current loop that
removes sibling bot:* labels (using others, justApplied and
octokit.rest.issues.removeLabel) can race between concurrent webhooks and let an
older handler remove a newer label; before removing siblings, implement a
per-target serialization or winner-check: acquire a short-lived mutex keyed by
owner/repo/number (or persist a winning-label record) when handling the webhook,
record this handler’s candidate (justApplied plus event timestamp or delivery
id) as the current winner, then only proceed to remove labels if the
persisted/mutex-stored winner matches this handler; otherwise skip removals.
Ensure BOT_LABEL_PATTERN, others and removed logic only runs after confirming
the handler is the recorded winner to make “newest label wins” deterministic.
Owner
Author
There was a problem hiding this comment.
Acknowledged but out of scope for this PR. Per-(owner, repo, number) mutex serialization is a correctness hardening beyond the current protocol definition in contracts/webhook-dispatch.md, and touching it here would expand the change set further. Tracked for a follow-up — the current implementation addresses the label-stripping-on-refusal ordering issue (CodeRabbit #3116972030) which was the correctness blocker.
There was a problem hiding this comment.
@chrisleekr, understood — acknowledged and won't flag this further in the current PR. The best-effort mutex is sufficient for the current protocol scope, and the correctness blocker (label-stripping-on-refusal ordering) is already addressed.
Would you like me to open a follow-up GitHub issue to track the per-(owner, repo, number) serialization hardening for "newest label wins" determinism under concurrent webhooks?
🧠 Learnings used
Learnt from: chrisleekr
Repo: chrisleekr/github-app-playground PR: 35
File: src/daemon/job-executor.ts:193-196
Timestamp: 2026-04-19T20:58:15.944Z
Learning: In `src/daemon/job-executor.ts` (chrisleekr/github-app-playground), threading `AbortSignal` through `runPipeline` (and through Octokit GraphQL, Claude Agent SDK, git CLI) to fix the active-job accounting gap after `handleJobCancel()` is intentionally deferred to a follow-up PR. The current safety nets are: idle shutdown is gated on `activeJobs > 0`, and K8s `activeDeadlineSeconds: 3600` acts as the outer hard ceiling. Do not flag this as a blocker for the current PR.
Learnt from: CR
Repo: chrisleekr/github-app-playground PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T09:55:02.589Z
Learning: Applies to src/webhook/router.ts : Idempotency uses two-layer guard: fast path in-memory `Map` keyed by `X-GitHub-Delivery` header, and durable `isAlreadyProcessed()` checking GitHub for existing tracking comment
Replaces the stub handlers shipped in the US1 foundation commit with
working implementations:
- plan.ts: multi-turn Claude Agent SDK session over a cloned repo, writes
PLAN.md at repo root, emits markdown into state.plan.
- implement.ts: reuses runPipeline end-to-end with the prior plan as
trigger body, locates the opened PR via octokit.rest.pulls.list, emits
{pr_number, pr_url, branch} into state.
- review.ts: fetches PR + failing checks + unresolved review comments,
runs the agent with a review prompt carrying the pr-auto stop bounds
(FIX_ATTEMPTS_CAP=3, POLL_WAIT_SECS_CAP=900) and V/PV/I/NC taxonomy
per FR-005(c). Never calls pulls.merge (FR-017).
Marks T021-T023 complete in tasks.md. T020 stays MVP keyword-heuristic
(LLM-swap deferred per original task note).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Add `onStepComplete(runId, result)` in `src/workflows/orchestrator.ts` with transactional parent lock (`SELECT ... FOR UPDATE`), next-child insert, or terminal parent transition. - Extend `HandlerResult` with `handed-off` variant so composite parents can stay `running` while the cascade runs. - Implement `ship.ts` handler: per-step staleness checks, compute start index, insert first child, enqueue, return `handed-off` with `stepRuns` carried forward. - Wire the daemon executor to branch on `handed-off` and to call `onStepComplete` after terminal translation — including the uncaught- throw path — so the parent cascade runs in every outcome. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Add `src/workflows/intent-classifier.ts` with `classify(commentBody)` —
single-turn LLM call through `src/ai/llm-client.ts`, Zod-validated
JSON response, and prompt-injection hardening (T037a):
* comment body wrapped in <user-comment> delimiter as data
* closed-enum schema forces unknown/adversarial outputs to `clarify`
* control tokens (backticks, ###, ---, delimiter literals) collapsed
* raw body logged at debug level only
- Add `INTENT_CONFIDENCE_THRESHOLD` config (default 0.75).
- Fill in `dispatchByIntent` in `src/workflows/dispatcher.ts`: runs
classify → posts clarify/refusal below threshold → otherwise runs
the same context/mutex/prior-output/insert/enqueue flow as
`dispatchByLabel`.
- Rewrite `src/webhook/events/issue-comment.ts` and `review-comment.ts`
to route triggered comments through `dispatchByIntent` instead of
the prior in-process pipeline (T039, T047).
Covers T037, T037a, T038, T039, T047. Test tasks T034-T036 and the
fixture artefact T040 are deferred to the US5 polish batch.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers the five built-in workflows (triage/plan/implement/review/ship) plus the comment intent classifier, ship composite resume rules, and an "extending" guide. Wires the page into mkdocs nav and extends the doc-sync rule in CLAUDE.md so src/workflows/** changes must update this doc in the same PR. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…48a) Adds scripts/check-docs-sync.ts — a Bun script that fails CI when a PR touches src/workflows/ without updating docs/BOT-WORKFLOWS.md (exempts test/.md files). Wires it into ci.yml on pull_request events. Cross-links the new Bot Workflows page from docs/ARCHITECTURE.md Further Reading, and extends docs/OBSERVABILITY.md with the new log fields introduced by the bot-workflows feature (workflowRunId, workflowName, ship_duration_ms, intentWorkflow, intentConfidence). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…classifier CI format check caught two files untouched by lint-staged because they were staged/committed through earlier prettier-not-run paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove explicit BEGIN/COMMIT from migrations 004/005 so the migrate runner's per-file transaction wrapper can roll back atomically - Reorder dispatch protocol: prior-output check now runs before the label mutex, so a requiresPrior refusal does not strip unrelated `bot:*` labels from the target - Switch prior-output check to `findLatestSucceededForTarget` so a later failed run does not block a dispatch with a valid prior success - Discriminate insertQueued errors: only `23505` on `idx_workflow_runs_inflight` yields the "in-flight" refusal; other errors propagate without a misleading refusal comment - Clear the in-flight guard via `markFailed` when enqueue fails after insert, so subsequent dispatches are not permanently blocked - Add CAS-based tracking-comment reservation (`tryReserveTrackingCommentId`) to prevent duplicate comments on concurrent setState; losing writer deletes its duplicate and updates the winner - Wrap post-terminal `setState` calls in the workflow executor in try/catch so a tracking-mirror failure cannot flip already-succeeded rows through the outer "uncaught:" error path - Sync spec docs (contract, data-model, quickstart, research, tasks) to the implemented protocol order and fence language - Extend dispatcher tests for the collision, non-collision, and enqueue-failure paths; fix registry test label fixture to satisfy `^bot:[a-z]+$` Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… T045, T050
Adds the full test coverage block the spec's tasks.md deferred beyond the
initial implementation PR: orchestrator cascade (success + failure), ship
handler resume + open-PR path, intent-classifier fixture-based accuracy
audit and threshold/fallback behaviour, issue-comment → dispatchByIntent
row-shape parity check. Also curates the ≥20-comment intent fixture set
(T040) and fixes a latent BIGINT-string leak in tryReserveTrackingCommentId
surfaced by the new runs-store tests.
- T025+T026 orchestrator.test.ts (integration): full ship cascade success
chain and step-2 failure cascade against a real Postgres transaction.
- T027+T028 ship.test.ts (integration): resume after implement failure with
startIndex=2 and priorRunIds carried forward; FR-020 open-PR shortcut
skips straight to review using a mocked live pulls.get.
- T034+T035 intent-classifier.test.ts: fixture coverage check, ≥90%
accuracy under a perfect-stub, ≥90% accuracy with 2 degraded responses,
threshold/fallback semantics including prompt-injection off-enum
rejection and LLM rejection fallback.
- T036 issue-comment.test.ts: dispatchByLabel("bot:ship") and
dispatchByIntent(<ship comment>) produce field-equivalent workflow_runs
rows; low-confidence comment writes no row.
- T040 test/workflows/fixtures/intent-comments.json: 23 labelled comments
(≥3 per atomic workflow / ship / clarify / unsupported).
- runs-store.ts: tryReserveTrackingCommentId now coerces the BIGINT
tracking_comment_id returned by Bun.sql back to number, matching
normalizeRow's treatment. Bug surfaced by the new CAS test.
- runs-store.test.ts: adds CAS race, findLatestSucceededForTarget, and
null-path tests to push line coverage past the ≥90% target (now
100%/97.08%).
- T045 coverage audit: intent-classifier 100%/97.48%, label-mutex 100%/
100%, runs-store 100%/97.08% — all meet the ≥90% target; orchestrator
90.91%/94.44%, ship.ts 100%/89.13%, registry 92.86%/98.08% — all meet
the ≥70% target for new modules.
- T050 Constitution Check rerun (2026-04-24): all 11 principles in
plan.md#Constitution Check remain Pass with the merged code.
- T049 left unchecked: the remaining scenarios now have automated
equivalents, but a live-repo smoke-test still needs a human with App
credentials before merge.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
chrisleekr
marked this pull request as ready for review
April 24, 2026 08:47
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
renovate.json (1)
62-73: Consider scopingdigestdelay, or accept the security-bump trade-off.The 7-day rule lumps
digestin withminor/patch. For Docker image digest pins on mutable tags (e.g.,node:20,alpine:3), maintainers frequently republish the same tag to ship CVE fixes; a 7-day hold delays those security bumps too. If that's intentional (matches the PR objective of "catching bad releases before automerge"), ignore. Otherwise, consider splittingdigestinto its own rule with a shorter age (e.g., 1–3 days) or excluding specific base images viamatchDatasources/matchDepNames.The
internalChecksFilter: "strict"setting prevents PRs from being created until the age threshold passes, meaning you won't see pending updates in the dashboard during the waiting window. If you'd rather see PRs raised immediately but held from automerge, useinternalChecksFilter: "none"(which creates PRs with a pending status check that clears after the age requirement is met).♻️ Optional split of digest from minor/patch
{ - "description": "Wait 7 days before raising minor/patch/digest PRs to catch bad releases before automerge.", - "matchUpdateTypes": ["minor", "patch", "digest"], + "description": "Wait 7 days before raising minor/patch PRs to catch bad releases before automerge.", + "matchUpdateTypes": ["minor", "patch"], "minimumReleaseAge": "7 days", "internalChecksFilter": "strict" }, + { + "description": "Short delay on digest pins so security republishes aren't held for a week.", + "matchUpdateTypes": ["digest"], + "minimumReleaseAge": "2 days", + "internalChecksFilter": "strict" + }, { "description": "Wait 14 days before raising major update PRs for manual review stability.", "matchUpdateTypes": ["major"], "minimumReleaseAge": "14 days", "internalChecksFilter": "strict" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@renovate.json` around lines 62 - 73, The current Renovate rule groups "digest" with "minor"/"patch" under matchUpdateTypes and enforces a 7-day minimumReleaseAge and internalChecksFilter: "strict", which delays security-related digest updates and hides pending PRs; to fix, split digest into its own rule by creating a separate object where matchUpdateTypes includes only "digest" and set a shorter minimumReleaseAge (e.g., "1-3 days") or adjust matchDatasources/matchDepNames to exclude specific mutable base images, and if you want PRs visible immediately but blocked from automerge change internalChecksFilter to "none" (update the rule objects referencing matchUpdateTypes, minimumReleaseAge, and internalChecksFilter accordingly).scripts/check-docs-sync.ts (1)
18-20: Optional: add--to guard against rev-arg injection.
BASE_SHA/HEAD_SHAcome from GitHub's PR event so they're trusted today, but if a value ever started with-(or was overridden locally with a typo)git diff --name-onlywould interpret it as an option. A trailing--makes the intent explicit and defends the script against that drift.Proposed hardening
- const res = spawnSync("git", ["diff", "--name-only", `${base}...${head}`], { + const res = spawnSync("git", ["diff", "--name-only", `${base}...${head}`, "--"], { encoding: "utf8", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-docs-sync.ts` around lines 18 - 20, The git spawnSync call using variables base and head in scripts/check-docs-sync.ts can misinterpret values beginning with "-" as options; update the args passed to spawnSync in the block that builds the diff (the const res = spawnSync(... ) call) to include a "--" separator before the rev range to force git to treat `${base}...${head}` as a revision argument (i.e., insert the literal "--" into the args array just before the rev string), ensuring rev-arg injection is prevented while keeping existing behavior.src/workflows/registry.ts (1)
85-102: Optional: add a self-reference / acyclicity guard forrequiresPrior.The current refinements cover name/label uniqueness, step-reference integrity, and the composite/
requiresPriorXOR rule, but they do not catchrequiresPrior === nameor cycles likeA → B → A. The enum constraint keeps the value well-typed but not structurally coherent. A single refine over therequiresPriorchain would close the gap and keep the "fail at boot, not mid-flight" guarantee.Proposed additional refinement
.refine((entries) => entries.every((e) => e.steps.length === 0 || e.requiresPrior === null), { message: "composite workflows (non-empty steps) must have requiresPrior === null", - }); + }) + .refine( + (entries) => { + const byName = new Map(entries.map((e) => [e.name, e])); + return entries.every((start) => { + const seen = new Set<string>(); + let cur: typeof start | undefined = start; + while (cur?.requiresPrior != null) { + if (seen.has(cur.name)) return false; + seen.add(cur.name); + cur = byName.get(cur.requiresPrior); + if (cur === undefined) return false; + if (cur.name === start.name) return false; + } + return true; + }); + }, + { message: "requiresPrior chain must be acyclic and reference existing workflow names" }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/registry.ts` around lines 85 - 102, Add a refinement to RegistrySchema that validates the requiresPrior chains are acyclic and do not self-reference: build a map from entry.name to entry.requiresPrior, ensure requiresPrior is either null or refers to an existing name, and walk each non-null requiresPrior chain from a starting name (checking entry.requiresPrior on RegistryEntrySchema) to detect if you revisit a name (cycle) or see requiresPrior === name (self-reference); return false on any cycle/self-reference and true otherwise. Integrate this refine into the existing chain on RegistrySchema so the validation runs alongside the uniqueness and step-reference checks.src/workflows/orchestrator.ts (1)
86-113: "ship halted" / "ship complete" hardcoded — will misattribute when a second composite is added.The registry currently has
shipas the only composite, butonStepCompleteis generic (getByName(parent.workflow_name).steps). If/when another composite is introduced, these messages will print "ship halted…" against a non-ship parent. Cheap fix: interpolateparent.workflow_name.📝 Proposed wording
- humanMessage: `ship halted at step ${String(childStepIndex)} (${parent.workflow_name} → ${child.workflow_name}): ${result.reason ?? "unknown"}`, + humanMessage: `${parent.workflow_name} halted at step ${String(childStepIndex)} (${child.workflow_name}): ${result.reason ?? "unknown"}`, @@ - humanMessage: `ship complete — all ${String(steps.length)} steps succeeded.`, + humanMessage: `${parent.workflow_name} complete — all ${String(steps.length)} steps succeeded.`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/orchestrator.ts` around lines 86 - 113, The humanMessage strings currently hardcode "ship halted" and "ship complete" which will misattribute other composites; update the messages to interpolate the parent.workflow_name instead (use parent.workflow_name where the diff constructs postCommit.parentTerminal.humanMessage for both the failure and success branches), e.g., replace the literal "ship halted…" and "ship complete…" with templates that include parent.workflow_name and preserve the existing step/index/length details; ensure you update both the failure branch (where result.reason is used) and the success branch that builds successPatch/postCommit.test/workflows/intent-classifier.test.ts (1)
115-146: Degradation test's arithmetic is tied to a specific fixture ordering.
degradedBodiesis derived fromfixtureSet.slice(0, 2), and the inline comment on lines 142-144 hard-codes the assumption that both of those fixtures haveexpected_workflow === "triage". Iftest/workflows/fixtures/intent-comments.jsonis reordered or the first two entries later target a workflow for which your stub still produces the expected answer (e.g.clarify), the test may silently pass without actually exercising the malformed-response path.Consider selecting the degraded subset by predicate (e.g. the first two fixtures whose
expected_workflow !== "clarify"), or asserting the concrete hit-count you expect rather than just the ≥0.9 floor, so a regression in the fallback pipeline can't hide behind fixture reshuffling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/workflows/intent-classifier.test.ts` around lines 115 - 146, The test currently picks degradedBodies via fixtureSet.slice(0, 2) which couples the test to fixture ordering; change the selection to pick the first two fixtures whose expected_workflow !== "clarify" (or otherwise satisfy a predicate ensuring the stub will produce a fallback) so the malformed-response path is actually exercised, and replace the loose accuracy assertion with a concrete expectation (e.g., expect hits === fixtureSet.length - N or expect accuracy to equal (fixtureSet.length - N)/fixtureSet.length) where N is the number of degraded fixtures; update references in the test to degradedBodies, fixtureSet, and the classify import from "../../src/workflows/intent-classifier" accordingly.src/workflows/handlers/review.ts (1)
107-109: Pipeline error is silently dropped.
resultfromrunPipelinelikely carries a failure reason, but it's collapsed to the generic string"review pipeline execution failed". Maintainers reading the tracking comment or logs have no way to diagnose. Propagate the actual message and log atwarnbefore returning.const result = await runPipeline(botCtx); if (!result.success) { - return { status: "failed", reason: "review pipeline execution failed" }; + const detail = + "error" in result && result.error instanceof Error + ? result.error.message + : "unknown pipeline failure"; + log.warn({ detail }, "review pipeline reported failure"); + return { status: "failed", reason: `review pipeline execution failed: ${detail}` }; }Adjust the field access to match the actual
runPipelinereturn shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/handlers/review.ts` around lines 107 - 109, The current check collapses runPipeline failures into a generic message; update the handling around runPipeline's result to access and propagate the real failure field (e.g., result.error or result.reason to match the actual return shape) instead of the fixed string, call processLogger.warn (or the module logger used in this file) with the returned error details before returning, and return the actual message in the returned object (e.g., { status: "failed", reason: result.error || result.reason || "review pipeline execution failed" }) so callers and maintainers see the real failure information.src/workflows/dispatcher.ts (1)
180-312: Consider consolidating the shared dispatch tail between label and intent paths.Lines 216-296 of
dispatchByIntentre-implement context check →requiresPrior→ mutex → insert → enqueue-with-compensation verbatim fromdispatchByLabel(L57-143). Extracting a privatedispatchResolved(entry, params, triggerBodyPreview, labelForMutex)helper would remove ~70 lines of duplication and ensure the two paths can't drift (e.g., if a future fix to the label path's enqueue compensation is missed on the intent path).Not a blocker — the duplication is explicit and each branch is independently readable. Flagging as good-to-have.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/dispatcher.ts` around lines 180 - 312, The dispatchByIntent function duplicates the same post-classification dispatch tail found in dispatchByLabel; extract that shared logic into a private helper (e.g., dispatchResolved) and have both dispatchByIntent and dispatchByLabel call it. The helper should accept the resolved WorkflowEntry (entry), the common params needed for enqueueing (octokit, logger, target, senderLogin, deliveryId, etc.), a triggerBodyPreview string, and the label used for the mutex; it must perform the contextMatches check, requiresPrior lookup (findLatestSucceededForTarget), enforceSingleBotLabel, insertQueued with isInflightCollision handling, enqueueJob with compensate-on-failure via markFailed, final logger.info and return the same DispatchOutcome shape (dispatched/refused). Replace the duplicated blocks in both dispatchByIntent and dispatchByLabel with a single call to dispatchResolved to eliminate the ~70-line duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/20260421-181205-bot-workflows/quickstart.md`:
- Around line 102-107: Update the troubleshooting table row for "Two tracking
comments on one issue" to remove the misdiagnosis that a duplicate run caused
the condition and instead explain the real cause and remediation: mention that
the partial unique index idx_workflow_runs_inflight enforces uniqueness on
workflow_runs (not GitHub comments), that races in
src/workflows/tracking-mirror.ts (setState) can produce two createComment calls
where the loser attempts deleteComment and a delete failure leaves two comments,
and instruct operators to treat this as a rare cleanup failure (not a duplicate
run) and to gather both comment URLs and relevant tracking-mirror logs/DB rows
for cleanup and investigation.
In `@specs/20260421-181205-bot-workflows/research.md`:
- Line 23: Update the unique partial index in migration 005 to include
target_type in the key so it becomes (workflow_name, target_type, target_owner,
target_repo, target_number) and update the research spec decision text in
specs/20260421-181205-bot-workflows/research.md to reflect this change; locate
the migration that creates the unique index (migration 005) and modify the
CREATE UNIQUE INDEX statement to add target_type, and edit the Decision line
that lists the table schema/index to include target_type in the unique index key
and ensure the partial WHERE clause on status remains unchanged.
In `@src/config.ts`:
- Around line 291-296: Add a new configuration entry for
INTENT_CONFIDENCE_THRESHOLD to the project's configuration documentation (the
CONFIGURATION.md file) mirroring the TRIAGE_CONFIDENCE_THRESHOLD row: document
the environment variable name INTENT_CONFIDENCE_THRESHOLD, the default value
0.75, allowed range [0, 1], and a short description that it controls the minimum
intent-classifier confidence (backed by the intentConfidenceThreshold schema in
src/config.ts) below which the dispatcher treats a comment as ambiguous and
posts a clarification request instead of dispatching. Ensure formatting and
column order match the existing configuration table.
In `@src/workflows/handlers/implement.ts`:
- Around line 27-38: The handler currently calls findLatestForTarget and rejects
non-succeeded rows which conflicts with the dispatcher's requiresPrior:'plan'
semantics; replace the call to findLatestForTarget with
findLatestSucceededForTarget (or otherwise use the succeeded-only finder) when
loading planRow so the implement handler consults the latest succeeded plan the
dispatcher expects, and update the rejection message to reference the
succeeded-only check if you keep the stricter behavior; reference the symbols
findLatestForTarget, findLatestSucceededForTarget, planRow and the implement
handler in src/workflows/handlers/implement.ts when making this change.
- Around line 91-97: The current PR lookup via findRecentOpenedPr is too broad
and can pick up unrelated PRs (and miss the agent's PR due to per_page=10);
update the implementation to scope discovery to the agent's PR by filtering the
returned PRs for head.user.login === BOT_USERNAME and/or head.ref matching the
agent's branch pattern (e.g., a known prefix), and/or replace the REST search
with an octokit.graphql query that inspects the issue's timeline
(issue.timelineItems itemTypes: [CROSS_REFERENCED_EVENT, CONNECTED_EVENT]) to
find the PR that references this issue; also ensure pagination or a larger
per_page is used so the agent's PR isn't dropped, and keep the same call sites
that persist the PR number via setState and that ship.ts later reads with
octokit.rest.pulls.get({ pull_number }).
In `@src/workflows/handlers/review.ts`:
- Around line 97-99: The code sets defaultBranch using pr.base.ref (which is the
PR target branch and may be a feature branch); update the assignment that builds
the pipeline context (field defaultBranch) to use pr.base.repo.default_branch
from the same pulls.get response instead of pr.base.ref so downstream logic in
runPipeline / ctx.defaultBranch (e.g., FR-016 "never push to default" checks)
compares against the actual repository default; ensure the object that currently
contains defaultBranch, headBranch, baseBranch is modified to reference
pr.base.repo.default_branch and keep headBranch as pr.head.ref and baseBranch as
pr.base.ref.
- Around line 55-75: The checks listing is missing pagination and the
unresolved-comment detection is using the wrong signal: change the call using
octokit.rest.checks.listForRef to use
octokit.paginate(octokit.rest.checks.listForRef, { owner: target.owner, repo:
target.repo, ref: pr.head.sha, per_page: 100 }) so failingChecks is computed
from the complete set of check_runs; for review comments, stop treating pull
request review comments (from octokit.rest.pulls.listReviewComments) with
in_reply_to_id === undefined as "unresolved" — either rename unresolvedComments
to topLevelComments if you intend top-level comments, or replace the logic by
querying the PullRequestReviewThread via GraphQL (reviewThreads { nodes {
isResolved } }) and compute unresolved threads as those with isResolved ===
false, then update downstream uses of unresolvedComments/failingChecks
accordingly.
In `@src/workflows/orchestrator.ts`:
- Around line 122-189: The post-commit enqueueJob call can fail and leave a
queued child row orphaned; wrap the enqueueJob call in a try/catch and on error
perform compensating DB updates: update the child run (nextChild / job.runId) to
status='failed' and include a failure timestamp/reason, and update the parent
run (parent.id / job.parentRunId) to set status='failed' and add
failedAtStepIndex and failedReason (e.g. "enqueue failed") into its state JSON
so operators see a breadcrumb; ensure these updates use the same DB client (not
a tx) after the transaction, log the error with logger.error including the
exception, and rethrow or return the failure as appropriate so callers observe
the failure.
In `@src/workflows/tracking-mirror.ts`:
- Around line 130-144: postRefusalComment currently lets
octokit.rest.issues.createComment errors bubble up; make it best-effort by
wrapping the createComment call in a try/catch so transient GitHub API failures
don't surface to dispatchers. In the catch, log the error with deps.logger.warn
or deps.logger.error including { target, workflowName, reason, err } and a
message like "Failed to post refusal comment" but do not rethrow; keep the
existing deps.logger.info on success. Update the function postRefusalComment to
mirror the duplicate-delete pattern used around L95-110 (catch, log, continue).
---
Nitpick comments:
In `@renovate.json`:
- Around line 62-73: The current Renovate rule groups "digest" with
"minor"/"patch" under matchUpdateTypes and enforces a 7-day minimumReleaseAge
and internalChecksFilter: "strict", which delays security-related digest updates
and hides pending PRs; to fix, split digest into its own rule by creating a
separate object where matchUpdateTypes includes only "digest" and set a shorter
minimumReleaseAge (e.g., "1-3 days") or adjust matchDatasources/matchDepNames to
exclude specific mutable base images, and if you want PRs visible immediately
but blocked from automerge change internalChecksFilter to "none" (update the
rule objects referencing matchUpdateTypes, minimumReleaseAge, and
internalChecksFilter accordingly).
In `@scripts/check-docs-sync.ts`:
- Around line 18-20: The git spawnSync call using variables base and head in
scripts/check-docs-sync.ts can misinterpret values beginning with "-" as
options; update the args passed to spawnSync in the block that builds the diff
(the const res = spawnSync(... ) call) to include a "--" separator before the
rev range to force git to treat `${base}...${head}` as a revision argument
(i.e., insert the literal "--" into the args array just before the rev string),
ensuring rev-arg injection is prevented while keeping existing behavior.
In `@src/workflows/dispatcher.ts`:
- Around line 180-312: The dispatchByIntent function duplicates the same
post-classification dispatch tail found in dispatchByLabel; extract that shared
logic into a private helper (e.g., dispatchResolved) and have both
dispatchByIntent and dispatchByLabel call it. The helper should accept the
resolved WorkflowEntry (entry), the common params needed for enqueueing
(octokit, logger, target, senderLogin, deliveryId, etc.), a triggerBodyPreview
string, and the label used for the mutex; it must perform the contextMatches
check, requiresPrior lookup (findLatestSucceededForTarget),
enforceSingleBotLabel, insertQueued with isInflightCollision handling,
enqueueJob with compensate-on-failure via markFailed, final logger.info and
return the same DispatchOutcome shape (dispatched/refused). Replace the
duplicated blocks in both dispatchByIntent and dispatchByLabel with a single
call to dispatchResolved to eliminate the ~70-line duplication.
In `@src/workflows/handlers/review.ts`:
- Around line 107-109: The current check collapses runPipeline failures into a
generic message; update the handling around runPipeline's result to access and
propagate the real failure field (e.g., result.error or result.reason to match
the actual return shape) instead of the fixed string, call processLogger.warn
(or the module logger used in this file) with the returned error details before
returning, and return the actual message in the returned object (e.g., { status:
"failed", reason: result.error || result.reason || "review pipeline execution
failed" }) so callers and maintainers see the real failure information.
In `@src/workflows/orchestrator.ts`:
- Around line 86-113: The humanMessage strings currently hardcode "ship halted"
and "ship complete" which will misattribute other composites; update the
messages to interpolate the parent.workflow_name instead (use
parent.workflow_name where the diff constructs
postCommit.parentTerminal.humanMessage for both the failure and success
branches), e.g., replace the literal "ship halted…" and "ship complete…" with
templates that include parent.workflow_name and preserve the existing
step/index/length details; ensure you update both the failure branch (where
result.reason is used) and the success branch that builds
successPatch/postCommit.
In `@src/workflows/registry.ts`:
- Around line 85-102: Add a refinement to RegistrySchema that validates the
requiresPrior chains are acyclic and do not self-reference: build a map from
entry.name to entry.requiresPrior, ensure requiresPrior is either null or refers
to an existing name, and walk each non-null requiresPrior chain from a starting
name (checking entry.requiresPrior on RegistryEntrySchema) to detect if you
revisit a name (cycle) or see requiresPrior === name (self-reference); return
false on any cycle/self-reference and true otherwise. Integrate this refine into
the existing chain on RegistrySchema so the validation runs alongside the
uniqueness and step-reference checks.
In `@test/workflows/intent-classifier.test.ts`:
- Around line 115-146: The test currently picks degradedBodies via
fixtureSet.slice(0, 2) which couples the test to fixture ordering; change the
selection to pick the first two fixtures whose expected_workflow !== "clarify"
(or otherwise satisfy a predicate ensuring the stub will produce a fallback) so
the malformed-response path is actually exercised, and replace the loose
accuracy assertion with a concrete expectation (e.g., expect hits ===
fixtureSet.length - N or expect accuracy to equal (fixtureSet.length -
N)/fixtureSet.length) where N is the number of degraded fixtures; update
references in the test to degradedBodies, fixtureSet, and the classify import
from "../../src/workflows/intent-classifier" accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 502b9216-545f-42e3-9092-c2d7d57abab3
📒 Files selected for processing (38)
.github/workflows/ci.ymlCLAUDE.mddocs/ARCHITECTURE.mddocs/BOT-WORKFLOWS.mddocs/OBSERVABILITY.mdmkdocs.ymlpackage.jsonrenovate.jsonscripts/check-docs-sync.tsspecs/20260421-181205-bot-workflows/contracts/webhook-dispatch.mdspecs/20260421-181205-bot-workflows/data-model.mdspecs/20260421-181205-bot-workflows/quickstart.mdspecs/20260421-181205-bot-workflows/research.mdspecs/20260421-181205-bot-workflows/tasks.mdsrc/config.tssrc/daemon/workflow-executor.tssrc/db/migrations/004_collapse_dispatch_to_daemon.sqlsrc/db/migrations/005_workflow_runs.sqlsrc/webhook/events/issue-comment.tssrc/webhook/events/review-comment.tssrc/workflows/dispatcher.tssrc/workflows/handlers/implement.tssrc/workflows/handlers/plan.tssrc/workflows/handlers/review.tssrc/workflows/handlers/ship.tssrc/workflows/intent-classifier.tssrc/workflows/orchestrator.tssrc/workflows/registry.tssrc/workflows/runs-store.tssrc/workflows/tracking-mirror.tstest/webhook/events/issue-comment.test.tstest/workflows/dispatcher.test.tstest/workflows/fixtures/intent-comments.jsontest/workflows/handlers/ship.test.tstest/workflows/intent-classifier.test.tstest/workflows/orchestrator.test.tstest/workflows/registry.test.tstest/workflows/runs-store.test.ts
💤 Files with no reviewable changes (1)
- src/db/migrations/004_collapse_dispatch_to_daemon.sql
✅ Files skipped from review due to trivial changes (9)
- mkdocs.yml
- docs/OBSERVABILITY.md
- package.json
- docs/ARCHITECTURE.md
- test/workflows/fixtures/intent-comments.json
- docs/BOT-WORKFLOWS.md
- specs/20260421-181205-bot-workflows/data-model.md
- specs/20260421-181205-bot-workflows/contracts/webhook-dispatch.md
- src/db/migrations/005_workflow_runs.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- CLAUDE.md
- src/workflows/handlers/plan.ts
- src/workflows/handlers/ship.ts
- test/workflows/dispatcher.test.ts
|
|
||
| ## 2. `workflow_runs` schema: one row per run, JSON state | ||
|
|
||
| **Decision**: One table, columns `id UUID PK`, `workflow_name TEXT NOT NULL`, `target_type TEXT CHECK IN ('issue','pr')`, `target_owner TEXT`, `target_repo TEXT`, `target_number INT`, `parent_run_id UUID NULL REFERENCES workflow_runs(id)`, `parent_step_index INT NULL`, `status TEXT CHECK IN ('queued','running','succeeded','failed')`, `state JSONB NOT NULL DEFAULT '{}'::jsonb`, `tracking_comment_id BIGINT NULL`, `created_at TIMESTAMPTZ`, `updated_at TIMESTAMPTZ`. Unique index on `(workflow_name, target_owner, target_repo, target_number)` **partial** `WHERE status IN ('queued','running')` to enforce FR-011 idempotency at the database layer. (The `status` column gates the partial predicate, not the uniqueness key — including it in the key would allow two in-flight rows with different statuses, which is exactly what we must prevent.) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the unique index definition in the migration matches the research doc.
fd -t f '005_workflow_runs.sql' | xargs -I{} sed -n '1,200p' {}Repository: chrisleekr/github-app-playground
Length of output: 2373
Add target_type to the unique index key in migration 005 and update the research spec accordingly.
The current unique index on (workflow_name, target_owner, target_repo, target_number) allows duplicate in-flight runs for the same workflow on issue #N and PR #N of the same repository, since GitHub issue and PR numbers occupy the same numeric space per repo. The index must include target_type in the key: (workflow_name, target_type, target_owner, target_repo, target_number) to enforce FR-011 correctly.
🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Consider an alternative for the overused word “exactly”.
Context: ... rows with different statuses, which is exactly what we must prevent.) Rationale: ...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/20260421-181205-bot-workflows/research.md` at line 23, Update the
unique partial index in migration 005 to include target_type in the key so it
becomes (workflow_name, target_type, target_owner, target_repo, target_number)
and update the research spec decision text in
specs/20260421-181205-bot-workflows/research.md to reflect this change; locate
the migration that creates the unique index (migration 005) and modify the
CREATE UNIQUE INDEX statement to add target_type, and edit the Decision line
that lists the table schema/index to include target_type in the unique index key
and ensure the partial WHERE clause on status remains unchanged.
Owner
Author
There was a problem hiding this comment.
Not applicable — leaving the unique index on (workflow_name, target_owner, target_repo, target_number) as-is. Rationale: GitHub issues and PRs share the same per-repo numbering pool — issue #N and PR #N can never coexist in the same repository (one number identifies exactly one entity). So (owner, repo, number) already pins a single physical target, and including target_type in the key would only belt-and-suspenders against a bug in the event handler (which would mean a different class of defect than what the index is designed to catch). The semantic guarantee of FR-011 — "no duplicate in-flight runs per (workflow, target)" — is preserved without target_type in the key.
There was a problem hiding this comment.
@chrisleekr, thank you for the clarification — you're correct, and my original flag was based on a false premise. GitHub uses a single unified sequence per repository for both issues and pull requests, so the same number can never refer to both an issue and a PR in the same repo simultaneously. The existing key (workflow_name, target_owner, target_repo, target_number) is therefore sufficient to enforce FR-011, and adding target_type would be redundant.
I've noted this for future reviews.
✏️ Learnings added
Learnt from: chrisleekr
Repo: chrisleekr/github-app-playground PR: 49
File: specs/20260421-181205-bot-workflows/research.md:23-23
Timestamp: 2026-04-24T09:14:57.798Z
Learning: In GitHub, issues and pull requests share a single per-repository numbering sequence. A given `(owner, repo, number)` triple always identifies exactly one entity — either an issue or a PR — never both simultaneously. Therefore, a unique index on `(workflow_name, target_owner, target_repo, target_number)` in the `workflow_runs` table is sufficient to enforce at-most-one in-flight run per (workflow, target). Including `target_type` in the key is redundant and not required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: chrisleekr
Repo: chrisleekr/github-app-playground PR: 36
File: .github/workflows/secrets-scan.yml:15-24
Timestamp: 2026-04-20T09:46:13.771Z
Learning: In chrisleekr/github-app-playground's `.github/workflows/secrets-scan.yml`, the `secrets-scan` workflow intentionally includes BOTH `push` AND `pull_request` triggers. The repo is public OSS and can accept fork PRs, so `pull_request` (not `pull_request_target`) is required to ensure fork PRs are scanned. The `branches-ignore: [gh-pages]` exclusion applies to the `push` trigger. This was corrected in commit 5d67916 after an earlier mistaken omission of the `pull_request` trigger.
Learnt from: chrisleekr
Repo: chrisleekr/github-app-playground PR: 35
File: src/daemon/job-executor.ts:193-196
Timestamp: 2026-04-19T20:58:19.042Z
Learning: In `src/daemon/job-executor.ts` (chrisleekr/github-app-playground), threading `AbortSignal` through `runPipeline` (and through Octokit GraphQL, Claude Agent SDK, git CLI) to fix the active-job accounting gap after `handleJobCancel()` is intentionally deferred to a follow-up PR. The current safety nets are: idle shutdown is gated on `activeJobs > 0`, and K8s `activeDeadlineSeconds: 3600` acts as the outer hard ceiling. Do not flag this as a blocker for the current PR.
Eight valid findings from the CodeRabbit review posted on 2026-04-24 (3136599390, 3136599404, 3136599409, 3136599421, 3136599425, 3136599438, 3136599449, 3136599454). One finding (3136599398, target_type in unique index) is replied to inline as Invalid — GitHub issues and PRs share a numeric pool per repo, so (owner, repo, number) already identifies exactly one entity. - implement.ts L30: swap findLatestForTarget for findLatestSucceededForTarget so the plan-required check matches the dispatcher's requiresPrior semantics (a later failed re-run must not shadow an earlier valid plan). - implement.ts L95: scope findRecentOpenedPr to PRs authored by the bot account and bump per_page to 30 so a concurrent human PR on the repo cannot be misattributed to this run. - review.ts L55: paginate checks.listForRef via octokit.paginate so failingChecks isn't silently capped at 30 on busy PRs. - review.ts L75: rename unresolvedComments → topLevelComments; GitHub's REST API does not expose thread-resolution state, so "unresolved" was a lie. Downstream state.top_level_comments renamed likewise. - review.ts L97: use pr.base.repo.default_branch for ctx.defaultBranch; pr.base.ref is the PR target branch which, for stacked PRs, is a feature branch — FR-016 "never push to default" must compare against the actual repo default. - orchestrator.ts L165: wrap post-commit enqueueJob in try/catch; on failure mark child and parent rows as failed with a descriptive reason so the partial unique index releases and the operator sees the stall on the tracking comment instead of waiting for a timeout that never fires. - tracking-mirror.ts L130: make postRefusalComment best-effort — a transient GitHub 5xx on a cosmetic refusal comment must not bubble up into dispatchByLabel/dispatchByIntent and surface as a webhook 500. - docs/CONFIGURATION.md: add INTENT_CONFIDENCE_THRESHOLD row mirroring TRIAGE_CONFIDENCE_THRESHOLD; required by the CLAUDE.md doc-sync rule when src/config.ts gains a new env var. - quickstart.md troubleshooting: rewrite the "two tracking comments" row — the correct diagnosis is a tracking-comment reservation race where the loser's compensating deleteComment failed, not a duplicate DB run (which is prevented by the partial unique index). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Apr 25, 2026
chrisleekr
pushed a commit
that referenced
this pull request
Apr 25, 2026
# [1.3.0](v1.2.2...v1.3.0) (2026-04-25) ### Bug Fixes * **review:** forward installation token, post inline findings, and stream progress ([#57](#57)) ([7ee4861](7ee4861)) * **workflows:** make end-to-end runs survive without mid-run caps or stale state ([#55](#55)) ([35ee605](35ee605)) ### Features * **workflows:** add label-dispatched bot workflow foundation ([#49](#49)) ([1b18779](1b18779))
Owner
Author
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This was referenced Apr 26, 2026
5 tasks
3 tasks
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.
Description
Introduces the definitive bot-workflow pipeline — label-triggered, registry-driven workflow runs that piggyback on the existing daemon job queue via an optional
workflowRunfield onjob:payload.What lands
src/workflows/registry.ts) — the sole authoritative list of workflows. Five entries:triage,plan,implement,review,ship(composite).zod-validated at module load so a mistyped entry fails the process at boot.src/workflows/dispatcher.ts) — full seven-step protocol: label parse → owner allowlist → registry lookup → context match →requiresPriorcheck → label mutex → enqueue.src/workflows/label-mutex.ts) — enforces at-most-one activebot:*label per issue/PR; newest applied wins.workflow_runsDB table (migration005_workflow_runs.sql) — authoritative per-run state store with a JSONBstatecolumn so new workflows need no schema change (FR-025).src/workflows/tracking-mirror.ts) — DB-first replace-write projection ofworkflow_runs.stateonto the GitHub comment.src/workflows/runs-store.ts) — lifecycle helpers:markRunning,markSucceeded,markFailed, plus parent-linkage helpers for composite flows.src/daemon/workflow-executor.ts) — routesjob:payloadthrough the handler registry, wiressetStateinto the tracking mirror, and emits the finaljob:result.issues.labeled+pull_request.labeledregistered insrc/app.ts; both go through the samedispatchByLabel.workflowRunthreaded fromPendingOffer→job:payloadso the daemon branches on presence, not on payload shape.plan/implement/review/shiphandlers are stub scaffolds and do not execute anything yet.What this PR does not do
triagehas a working handler; the others are stubs — they parse and enqueue but the handler bodies are placeholders.src/ai/llm-client.tsin a follow-up.shipresume semantics (composite step orchestration) are not wired — that is the next batch.Housekeeping rolled in
.github/workflows/merge-dependencies.yml— Renovate's built-inplatformAutomergecombined with the zero-approvalmainruleset covers the same ground, so the workflow was redundant. (Previously it silently skipped Renovate PRs because it gated ondependabot[bot].)specs/**to ESLint's ignore list so the feature's contract fixtures don't get type-checked as source.Related Issues
specs/20260421-181205-bot-workflows/).Testing
Before / After
Before
After
Screenshots/Recordings
N/A — backend-only.
Summary by CodeRabbit