feat(workflows): up-front tracking comments, trigger reactions, parent cascade - #61
Merged
Merged
Conversation
…t cascade Closes the silent-multi-minute UX gap during triage/plan/implement runs and the silent-failure window when a daemon dies mid-job. - Triage, plan, and implement post a starting comment with input snapshot before the agent runs, so users see progress instead of an empty thread. - 4-stage GitHub reactions on the trigger comment: eyes (queued), rocket (dispatched), hooray (success), confused (failure / OOM disconnect). - Composite parents (ship) now render as a verbose composite body — each child step shows status, cost, turns, and a deep link to its own comment. Cascade fires automatically on child setState via tracking-mirror. - Orphan/disconnect cleanup in the orchestrator now updates the existing ancestor tracking comment with a failure message and adds a confused reaction, instead of failing silently. - Persist trigger_comment_id + trigger_event_type on workflow_runs and executions so the orphan path (no live BotContext) can reach the right comment after a daemon dies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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 45 minutes and 31 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 (7)
📝 WalkthroughWalkthroughThis PR introduces trigger-comment tracking and reaction workflows throughout the system. New database columns store trigger comment IDs and event types. Webhook handlers post immediate "eyes" reactions and forward metadata. Dispatch/execution paths thread this metadata; terminal handlers post completion reactions ("hooray"/"confused"). The tracking mirror renders composite comments with child status blocks and cascades parent updates. Changes
Sequence DiagramsequenceDiagram
participant GitHub as GitHub Webhook
participant Handler as Issue/PR Handler
participant Dispatcher as Dispatcher
participant RunStore as Run Store
participant Executor as Executor
participant Orchestrator as Orchestrator
participant Mirror as Tracking Mirror
participant Reactions as Reactions Utils
GitHub->>Handler: issue_comment event
Handler->>Reactions: addReaction("eyes")
Reactions-->>GitHub: ✅ eyes emoji posted
Handler->>Dispatcher: dispatchByIntent(triggerCommentId, triggerEventType)
Dispatcher->>RunStore: insertQueued(triggerCommentId, triggerEventType)
RunStore->>RunStore: persist trigger metadata
Dispatcher->>Reactions: addReaction("rocket")
Reactions-->>GitHub: ✅ rocket emoji posted
RunStore-->>Dispatcher: queued_run_id
Dispatcher->>Executor: execute workflow
Executor->>Mirror: setState(phase: "starting")
Mirror->>GitHub: create/update tracking comment
Executor->>Executor: run agent
Executor->>Orchestrator: onStepComplete(result)
alt Success
Orchestrator->>Mirror: setState(phase: "succeeded", ...)
Orchestrator->>Orchestrator: reactOnParentTrigger("hooray")
else Failure
Orchestrator->>Mirror: setState(phase: "failed", ...)
Orchestrator->>Orchestrator: reactOnParentTrigger("confused")
end
Orchestrator->>Reactions: addReaction(content)
Reactions-->>GitHub: ✅ reaction posted on trigger comment
Mirror->>GitHub: update parent composite with child status blocks
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/workflows/execution-row.ts (1)
36-46:⚠️ Potential issue | 🟡 Minor
dispatcher.tslacks guard beforeaddReaction—though webhook paths are safe in practice.The concern is partially validated:
reactions.utilmakes unguarded API calls, anddispatcher.ts(line 357) callsaddReaction()withtriggerCommentIdandtriggerEventTypewithout checking they are valid. However, the real-world risk is contained:
workflow-executor.tsproperly guardsif (context.commentId === 0) return;before reactions.connection-handler.tsandorchestrator.tsboth null-check before callingaddReaction().- Webhook handlers (
issue-comment.ts,review-comment.ts) always pass real comment IDs from GitHub (payload.comment.id).The unguarded
dispatcher.tscall (line 357) is only reached from webhooks with valid IDs. However, for consistency and safety, either:
- Add a guard in
dispatcher.tsbefore line 357:if (triggerCommentId && triggerEventType) { addReaction(...) }- Or document that
dispatchByIntentcallers must always provide valid comment IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/execution-row.ts` around lines 36 - 46, dispatcher.ts calls addReaction(...) from dispatchByIntent without validating triggerCommentId and triggerEventType; add a guard in dispatchByIntent that verifies triggerCommentId is a non-zero/falsy-safe value and triggerEventType is present before invoking addReaction (e.g., if (triggerCommentId && triggerEventType) { addReaction(...) }). Reference the dispatchByIntent function and the addReaction call so the check prevents unguarded API calls when triggerCommentId or triggerEventType are missing.
🧹 Nitpick comments (11)
src/workflows/handlers/implement.ts (1)
199-221: Guard against multi-line / markdown-bearing issue titles in the blockquote.
> ${input.title}produces a single-line blockquote, but GitHub issue titles can contain characters that break Markdown rendering (a stray backtick, leading#, or — rarely — embedded newlines via API clients). For an "up-front comment that the user sees first" this is the highest-visibility path; consider either escaping with a thin sanitizer (collapse whitespace) or using inline code formatting:- `> ${input.title}`, + `> ${input.title.replace(/\s+/g, " ").trim()}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/handlers/implement.ts` around lines 199 - 221, postStartingComment currently inserts raw input.title into a blockquote which can break Markdown if the title contains newlines, backticks, or leading '#' — sanitize the title before building the body in postStartingComment: normalize and collapse all whitespace/newlines to single spaces, escape or remove backticks and leading '#' characters (or replace internal backticks with a safe placeholder) and then use the sanitized string in the blockquote (or alternatively wrap the sanitized title in inline code) when calling ctx.setState so the up-front comment renders reliably.src/db/migrations/007_trigger_comment.sql (1)
13-21: Schema looks correct; consider naming the CHECK constraints for future migrations.Inline anonymous
CHECKconstraints get auto-generated names likeworkflow_runs_trigger_event_type_check, which makes targetedALTER ... DROP CONSTRAINTharder if the allowed enum ever needs to grow (e.g. addingpull_request_revieworcommit_comment). Naming them explicitly now is a cheap insurance policy.♻️ Optional: name the constraints
ALTER TABLE workflow_runs ADD COLUMN trigger_comment_id BIGINT NULL, ADD COLUMN trigger_event_type TEXT NULL - CHECK (trigger_event_type IN ('issue_comment', 'pull_request_review_comment')); + CONSTRAINT workflow_runs_trigger_event_type_chk + CHECK (trigger_event_type IN ('issue_comment', 'pull_request_review_comment')); ALTER TABLE executions ADD COLUMN trigger_comment_id BIGINT NULL, ADD COLUMN trigger_event_type TEXT NULL - CHECK (trigger_event_type IN ('issue_comment', 'pull_request_review_comment')); + CONSTRAINT executions_trigger_event_type_chk + CHECK (trigger_event_type IN ('issue_comment', 'pull_request_review_comment'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/migrations/007_trigger_comment.sql` around lines 13 - 21, Name the anonymous CHECK constraints on trigger_event_type to make future ALTER/DROP simpler: replace the inline CHECKs in ALTER TABLE workflow_runs and ALTER TABLE executions with named constraints (e.g. CONSTRAINT workflow_runs_trigger_event_type_chk CHECK (... ) and CONSTRAINT executions_trigger_event_type_chk CHECK (...)) so the constraints on the trigger_event_type column for both workflow_runs and executions are explicitly named and can be referenced later.src/orchestrator/history.ts (1)
46-46: Recommend reusing the exportedTriggerEventTypeinstead of inlining the union.
src/workflows/dispatcher.tsalready exportsTriggerEventType = "issue_comment" | "pull_request_review_comment", andsrc/workflows/runs-store.tsuses it onWorkflowRunRow.trigger_event_type/InsertQueuedParams. Inlining the union here forks the source of truth — if a future event (e.g.commit_comment) is added, the migration 007 CHECK and one of the TS unions will drift silently.♻️ Suggested change
- triggerCommentId?: number | null; - triggerEventType?: "issue_comment" | "pull_request_review_comment" | null; + triggerCommentId?: number | null; + triggerEventType?: TriggerEventType | null;Add the import (avoiding a circular import — if
dispatcher.ts → history.tsalready exists, moveTriggerEventTypeto a smallsrc/workflows/trigger-types.tsand re-export from both):+import type { TriggerEventType } from "../workflows/dispatcher";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/orchestrator/history.ts` at line 46, The field type for triggerEventType is inlined and duplicates the exported union TriggerEventType; replace the inline union with the exported TriggerEventType to keep a single source of truth (update the triggerEventType?: declaration to use TriggerEventType). If importing TriggerEventType would create a circular import, extract TriggerEventType into a small shared module (e.g., trigger-types) and re-export it from dispatcher.ts so both history.ts and dispatcher.ts can import the single exported symbol; ensure references to TriggerEventType and the triggerEventType field name are updated accordingly.src/workflows/handlers/plan.ts (1)
190-216: Optional: extract a sharedpostStartingCommenthelper.
triage.ts,plan.ts, and (per the PR summary)implement.tsall carry near-identical copies of this helper — same try/catch shape, samephase: "starting"patch, same warn fallback. Extracting to e.g.src/workflows/handlers/_starting-comment.ts(parameterized by emoji + verb) would keep the three handlers in lockstep and make future tweaks (e.g. adding arunIdlink) a one-file change.Not blocking — the duplication is small and each copy is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/handlers/plan.ts` around lines 190 - 216, Extract the duplicated postStartingComment logic into a single exported helper (e.g. function postStartingComment in a new src/workflows/handlers/_starting-comment.ts) that accepts the existing parameters (ctx: Parameters<WorkflowHandler>[0], input: {title: string; number: number; author: string|null}) plus optional customization params (emoji and verb strings) and implements the same try { await ctx.setState({ phase: "starting" }, body) } catch (err) { ctx.logger.warn({ err: err instanceof Error ? err.message : String(err) }, "plan starting-comment write failed — continuing without up-front comment") } behavior; then replace the duplicates in triage.ts, plan.ts, and implement.ts to import and call this helper (preserving phase: "starting", the body formatting, and the exact warn call) so future changes are made in one place.src/orchestrator/connection-handler.ts (1)
264-287: Consider structural typing to eliminate theas unknown as Octokitdouble-cast.
app.getInstallationOctokit(...)already returns an Octokit-compatible client; theas unknown as Octokitat line 266 silences a type drift between theoctokitmeta package'sOctokittype and the one returned bygetInstallationOctokit. SincesetStateonly usesrest.issues.*methods (createComment, deleteComment, updateComment) andaddReactiononly usesrest.reactions.*methods, you could instead define parameter interfaces with those specific method signatures—allowing callers to pass installation clients without theunknownbridge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/orchestrator/connection-handler.ts` around lines 264 - 287, The double-cast to Octokit hides a type mismatch—replace it by using structural typing for only the REST methods you need: update the parameter types of setState and addReaction to accept an object exposing the specific methods you call (e.g., rest.issues.createComment/deleteComment/updateComment and rest.reactions.create/update as appropriate) so any installation client from getInstallationOctokit satisfies those interfaces; then remove the "as unknown as Octokit" cast on installationOctokit and pass the client directly to setState and addReaction (refer to symbols: installationOctokit, getInstallationOctokit, setState, addReaction).src/workflows/tracking-mirror.ts (3)
195-260: Composite render LGTM; one small UX nit on the deep link.The verbose-block layout (emoji + workflow + status + cost/turns + truncated message) reads cleanly. Two minor things:
- Line 213 builds
…/issues/{number}#issuecomment-{id}regardless oftarget_type. GitHub redirects/issues/N→/pull/Nfor PRs so the link works, but for PR-targeted children it's worth using/pull/directly to avoid the redirect hop and to match GH's canonical URL.- Line 259's
slice(0, 600)operates on UTF-16 code units, so a trailing emoji or other surrogate-pair character at the boundary can be split. Probably acceptable given the 600-char headroom, but aArray.from(trimmed).slice(0, limit).join("")(or a grapheme-aware truncation) avoids the edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/tracking-mirror.ts` around lines 195 - 260, The deep-link uses a hardcoded /issues/ path in renderChildBlock (link variable) which should use child.target_type to prefer /pull/ for PR-targeted children (use `/pull/${target_number}` when target_type === "pull_request", otherwise `/issues/${target_number}`) and include the comment fragment as before; also make truncateForComposite UTF-16-safe by slicing grapheme/codepoint-aware (e.g. use Array.from(trimmed).slice(0, limit).join("") or a grapheme-aware library) instead of trimmed.slice(0, limit) so emojis/surrogate pairs aren’t split.
67-70: PersistinghumanMessageunder_lastHumanMessage— beware key collision with callerpatch.
{ ...patch, [LAST_HUMAN_MESSAGE_KEY]: humanMessage }lets the internal key win over a caller-provided same-named key, which is the right precedence — but a handler that accidentally writes_lastHumanMessageinpatchwill silently have it dropped on every call. Consider asserting thatpatchdoes not contain the reserved key (in dev/test) or namespacing it more defensively (e.g.__internal.lastHumanMessage) so future handler authors get a loud signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/tracking-mirror.ts` around lines 67 - 70, The merge call persists humanMessage into state by doing await mergeState(runId, { ...patch, [LAST_HUMAN_MESSAGE_KEY]: humanMessage }), which silently drops any caller-supplied key matching LAST_HUMAN_MESSAGE_KEY; update the code to first ensure callers can't clobber this reserved key by asserting patch does not contain LAST_HUMAN_MESSAGE_KEY (throw or log a dev/test-only error) or change LAST_HUMAN_MESSAGE_KEY to a more defensive name (e.g., __internal.lastHumanMessage) and update all uses (mergeState call, any readers) so the reserved internal key cannot be accidentally overwritten by handler-provided patch.
142-161: Cascade fires on every childsetState— fine for now, but watch the GH-API budget on long composites.For a composite like
shipwith N children each doing M progress updates, this is O(N·M) extrafindById+listChildrenByParent+updateCommentcalls in addition to the per-run write. Best-effort +.catch(...)makes this safe, but as the composite grows you may want to debounce/coalesce parent refreshes (e.g. throttle to once per few seconds per parent) so we don't burn secondary-rate-limit budget on intermediate states the user never sees.Not blocking — flagging as a future-tuning note.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/tracking-mirror.ts` around lines 142 - 161, The cascade refresh in the setState path calls refreshParentCompositeBody(resultRow.parent_run_id) for every child update, which can cause O(N·M) GH API calls for large composites; modify the caller (where resultRow.parent_run_id and runId are available) to debounce/coalesce refreshes per parent by introducing a short per-parent throttle (e.g., maintain an in-memory map keyed by parent_run_id with lastScheduled timer or timestamp) so that multiple rapid calls schedule only one refresh within the throttle window, and ensure the existing .catch(...) behavior remains so failures are still best-effort; keep the public refreshParentCompositeBody API unchanged and only alter the call site logic around it.src/workflows/runs-store.ts (3)
76-108: Document that children deliberately inherit trigger metadata via parent walk, not direct copy.Per the cross-file snippets, child INSERTs in
orchestrator.ts:125-137andhandlers/ship.ts:244-256do not passtriggerCommentId/triggerEventType, so children persistNULLfor both. Combined with the disconnect-cleanup walk-to-ancestor semantics in the PR description, this is intentional — but a single-line note here would save the next maintainer a trip to git blame:📝 Suggested doc update
/** * REST id of the user comment that triggered this run. NULL for - * label-triggered or system-spawned runs (no comment to react on). + * label-triggered or system-spawned runs (no comment to react on). + * Also NULL for child runs of a composite — the disconnect/reaction + * paths walk up `parent_run_id` to the topmost ancestor to recover + * the trigger comment. */ triggerCommentId?: number | null; triggerEventType?: TriggerEventType | null;🤖 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 76 - 108, Add a one-line comment in the insertQueued function next to the triggerCommentId/triggerEventType handling (and/or above the INSERT statement) stating that child runs intentionally do not copy triggerCommentId/triggerEventType and that trigger metadata is inherited via the parent-walk/disconnect-cleanup logic elsewhere; reference insertQueued, triggerCommentId, triggerEventType and the parent-walk semantics so future readers know to inspect orchestrator.ts and handlers/ship.ts for child-creation behavior.
333-345:findInflightByOwnerLGTM — consider an index check for the(owner_kind, owner_id, status)lookup.Query is correct and used by the disconnect cleanup path. Since this fires on every daemon disconnect and scans
workflow_runsby(owner_kind, owner_id, status IN (…)), make sure migration 007 (or a prior one) has a supporting index — otherwise it's a sequential scan that grows with the all-time row count, not the in-flight count. A partial index likeCREATE INDEX … ON workflow_runs (owner_kind, owner_id) WHERE status IN ('queued','running')is typically the right shape for this query.#!/bin/bash # Check existing indexes on workflow_runs across all migrations fd -e sql . src/db/migrations -x rg -nP --with-filename '(CREATE\s+(UNIQUE\s+)?INDEX|owner_kind|owner_id)' {} \;🤖 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 333 - 345, findInflightByOwner runs a WHERE lookup on owner_kind, owner_id and status IN ('queued','running') and may trigger full table scans; add a supporting index in the migrations (e.g., migration 007 or earlier) such as a partial index on workflow_runs for (owner_kind, owner_id) WHERE status IN ('queued','running') so the query in findInflightByOwner uses the index instead of scanning all rows; update the migrations to create that index and verify with the database inspection command mentioned in the review.
16-16: Establishruns-store.tsas the canonical home forTriggerEventType.This is the right place for the type given it mirrors a DB column constraint. Worth pairing with an
export type { TriggerEventType } from "./runs-store"re-export fromdispatcher.ts(rather than letting consumers import it from there directly), so there's no risk of dispatcher growing a duplicate alias that drifts from the DB CHECK constraint values in migration 007.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/runs-store.ts` at line 16, Declare and maintain TriggerEventType in runs-store.ts as the single source of truth (it already exists there as export type TriggerEventType = "issue_comment" | "pull_request_review_comment"); remove or avoid any duplicate alias in dispatcher.ts and instead add a re-export in dispatcher.ts that re-exports the type from runs-store (i.e., export the type from runs-store so consumers import TriggerEventType via dispatcher without defining a new alias), ensuring the dispatcher does not define its own TriggerEventType that could drift from the DB CHECK constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/orchestrator/connection-handler.ts`:
- Around line 222-234: The current findTopAncestor function can return a
non-topmost row if the 8-iteration cap is hit; update findTopAncestor (and
callers that expect a true ancestor) to detect when the loop exits due to the
cap and fail loudly or only return genuine topmost rows: after the for-loop
check whether current is non-null and current.parent_run_id === null before
returning it, otherwise log an error (or throw) indicating the depth cap was
reached and return null; use the existing findById callsite and process/logger
helper to emit the message so callers can handle the null safely.
In `@src/workflows/dispatcher.ts`:
- Around line 357-365: The call to addReaction in dispatcher.ts passes an object
whose destructuring happens in the helper before its try/catch, so under strict
TS settings a missing property could throw outside the helper's error handler;
fix by moving the destructuring inside the try/catch within the addReaction
implementation in src/utils/reactions.ts (ensure the try block wraps the
parameter destructuring and API call), or if you prefer the existing pattern,
add a concise comment above the addReaction call in dispatcher.ts documenting
the strict-TS guarantees (exactOptionalPropertyTypes/noUncheckedIndexedAccess)
that make destructuring safe so reviewers know this is intentional.
In `@src/workflows/execution-row.ts`:
- Line 5: There are duplicate definitions of TriggerEventType across the
codebase; remove the redundant declaration and make a single source of truth by
importing the canonical TriggerEventType from dispatcher.ts (or, if you prefer,
move the canonical type to a new shared module and import it everywhere). Update
any references in runs-store.ts and execution-row.ts to import TriggerEventType
from the chosen canonical module (e.g., dispatcher.ts or
src/shared/dispatch-types.ts) and delete the duplicate type declaration so only
the single exported TriggerEventType remains.
In `@src/workflows/tracking-mirror.ts`:
- Around line 94-130: In the lost-race branch, avoid re-rendering the comment
body from the stale captured `row`; instead re-fetch the latest workflow row
from the DB (the same query used earlier to populate `row`) to get the freshest
state, re-render `body` from that freshRow, then call
octokit.rest.issues.updateComment(...) with reservation.trackingCommentId and
the newly rendered body, and set resultRow based on that freshRow (including
tracking_comment_id: reservation.trackingCommentId). Ensure you use the same
identifiers (reservation.trackingCommentId, octokit.rest.issues.updateComment,
created.data.id, logger.warn) as in the diff so the new fetch-and-render
replaces the stale merge/update sequence.
---
Outside diff comments:
In `@src/workflows/execution-row.ts`:
- Around line 36-46: dispatcher.ts calls addReaction(...) from dispatchByIntent
without validating triggerCommentId and triggerEventType; add a guard in
dispatchByIntent that verifies triggerCommentId is a non-zero/falsy-safe value
and triggerEventType is present before invoking addReaction (e.g., if
(triggerCommentId && triggerEventType) { addReaction(...) }). Reference the
dispatchByIntent function and the addReaction call so the check prevents
unguarded API calls when triggerCommentId or triggerEventType are missing.
---
Nitpick comments:
In `@src/db/migrations/007_trigger_comment.sql`:
- Around line 13-21: Name the anonymous CHECK constraints on trigger_event_type
to make future ALTER/DROP simpler: replace the inline CHECKs in ALTER TABLE
workflow_runs and ALTER TABLE executions with named constraints (e.g. CONSTRAINT
workflow_runs_trigger_event_type_chk CHECK (... ) and CONSTRAINT
executions_trigger_event_type_chk CHECK (...)) so the constraints on the
trigger_event_type column for both workflow_runs and executions are explicitly
named and can be referenced later.
In `@src/orchestrator/connection-handler.ts`:
- Around line 264-287: The double-cast to Octokit hides a type mismatch—replace
it by using structural typing for only the REST methods you need: update the
parameter types of setState and addReaction to accept an object exposing the
specific methods you call (e.g.,
rest.issues.createComment/deleteComment/updateComment and
rest.reactions.create/update as appropriate) so any installation client from
getInstallationOctokit satisfies those interfaces; then remove the "as unknown
as Octokit" cast on installationOctokit and pass the client directly to setState
and addReaction (refer to symbols: installationOctokit, getInstallationOctokit,
setState, addReaction).
In `@src/orchestrator/history.ts`:
- Line 46: The field type for triggerEventType is inlined and duplicates the
exported union TriggerEventType; replace the inline union with the exported
TriggerEventType to keep a single source of truth (update the triggerEventType?:
declaration to use TriggerEventType). If importing TriggerEventType would create
a circular import, extract TriggerEventType into a small shared module (e.g.,
trigger-types) and re-export it from dispatcher.ts so both history.ts and
dispatcher.ts can import the single exported symbol; ensure references to
TriggerEventType and the triggerEventType field name are updated accordingly.
In `@src/workflows/handlers/implement.ts`:
- Around line 199-221: postStartingComment currently inserts raw input.title
into a blockquote which can break Markdown if the title contains newlines,
backticks, or leading '#' — sanitize the title before building the body in
postStartingComment: normalize and collapse all whitespace/newlines to single
spaces, escape or remove backticks and leading '#' characters (or replace
internal backticks with a safe placeholder) and then use the sanitized string in
the blockquote (or alternatively wrap the sanitized title in inline code) when
calling ctx.setState so the up-front comment renders reliably.
In `@src/workflows/handlers/plan.ts`:
- Around line 190-216: Extract the duplicated postStartingComment logic into a
single exported helper (e.g. function postStartingComment in a new
src/workflows/handlers/_starting-comment.ts) that accepts the existing
parameters (ctx: Parameters<WorkflowHandler>[0], input: {title: string; number:
number; author: string|null}) plus optional customization params (emoji and verb
strings) and implements the same try { await ctx.setState({ phase: "starting" },
body) } catch (err) { ctx.logger.warn({ err: err instanceof Error ? err.message
: String(err) }, "plan starting-comment write failed — continuing without
up-front comment") } behavior; then replace the duplicates in triage.ts,
plan.ts, and implement.ts to import and call this helper (preserving phase:
"starting", the body formatting, and the exact warn call) so future changes are
made in one place.
In `@src/workflows/runs-store.ts`:
- Around line 76-108: Add a one-line comment in the insertQueued function next
to the triggerCommentId/triggerEventType handling (and/or above the INSERT
statement) stating that child runs intentionally do not copy
triggerCommentId/triggerEventType and that trigger metadata is inherited via the
parent-walk/disconnect-cleanup logic elsewhere; reference insertQueued,
triggerCommentId, triggerEventType and the parent-walk semantics so future
readers know to inspect orchestrator.ts and handlers/ship.ts for child-creation
behavior.
- Around line 333-345: findInflightByOwner runs a WHERE lookup on owner_kind,
owner_id and status IN ('queued','running') and may trigger full table scans;
add a supporting index in the migrations (e.g., migration 007 or earlier) such
as a partial index on workflow_runs for (owner_kind, owner_id) WHERE status IN
('queued','running') so the query in findInflightByOwner uses the index instead
of scanning all rows; update the migrations to create that index and verify with
the database inspection command mentioned in the review.
- Line 16: Declare and maintain TriggerEventType in runs-store.ts as the single
source of truth (it already exists there as export type TriggerEventType =
"issue_comment" | "pull_request_review_comment"); remove or avoid any duplicate
alias in dispatcher.ts and instead add a re-export in dispatcher.ts that
re-exports the type from runs-store (i.e., export the type from runs-store so
consumers import TriggerEventType via dispatcher without defining a new alias),
ensuring the dispatcher does not define its own TriggerEventType that could
drift from the DB CHECK constraint.
In `@src/workflows/tracking-mirror.ts`:
- Around line 195-260: The deep-link uses a hardcoded /issues/ path in
renderChildBlock (link variable) which should use child.target_type to prefer
/pull/ for PR-targeted children (use `/pull/${target_number}` when target_type
=== "pull_request", otherwise `/issues/${target_number}`) and include the
comment fragment as before; also make truncateForComposite UTF-16-safe by
slicing grapheme/codepoint-aware (e.g. use Array.from(trimmed).slice(0,
limit).join("") or a grapheme-aware library) instead of trimmed.slice(0, limit)
so emojis/surrogate pairs aren’t split.
- Around line 67-70: The merge call persists humanMessage into state by doing
await mergeState(runId, { ...patch, [LAST_HUMAN_MESSAGE_KEY]: humanMessage }),
which silently drops any caller-supplied key matching LAST_HUMAN_MESSAGE_KEY;
update the code to first ensure callers can't clobber this reserved key by
asserting patch does not contain LAST_HUMAN_MESSAGE_KEY (throw or log a
dev/test-only error) or change LAST_HUMAN_MESSAGE_KEY to a more defensive name
(e.g., __internal.lastHumanMessage) and update all uses (mergeState call, any
readers) so the reserved internal key cannot be accidentally overwritten by
handler-provided patch.
- Around line 142-161: The cascade refresh in the setState path calls
refreshParentCompositeBody(resultRow.parent_run_id) for every child update,
which can cause O(N·M) GH API calls for large composites; modify the caller
(where resultRow.parent_run_id and runId are available) to debounce/coalesce
refreshes per parent by introducing a short per-parent throttle (e.g., maintain
an in-memory map keyed by parent_run_id with lastScheduled timer or timestamp)
so that multiple rapid calls schedule only one refresh within the throttle
window, and ensure the existing .catch(...) behavior remains so failures are
still best-effort; keep the public refreshParentCompositeBody API unchanged and
only alter the call site logic around it.
🪄 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: 9a63e34f-df28-441d-84aa-9b4cda76236f
📒 Files selected for processing (19)
docs/BOT-WORKFLOWS.mdsrc/daemon/workflow-executor.tssrc/db/migrations/007_trigger_comment.sqlsrc/orchestrator/connection-handler.tssrc/orchestrator/history.tssrc/utils/reactions.tssrc/webhook/events/issue-comment.tssrc/webhook/events/review-comment.tssrc/workflows/dispatcher.tssrc/workflows/execution-row.tssrc/workflows/handlers/implement.tssrc/workflows/handlers/plan.tssrc/workflows/handlers/triage.tssrc/workflows/orchestrator.tssrc/workflows/runs-store.tssrc/workflows/tracking-mirror.tstest/db/migrate.test.tstest/utils/reactions.test.tstest/webhook/events/issue-comment.test.ts
- src/shared/dispatch-types.ts: TriggerEventType becomes the single source of truth; dispatcher / runs-store / execution-row / utils/reactions all import from here so the union can't silently drift. - src/orchestrator/connection-handler.ts: findTopAncestor now returns null (with a warn log) when the parent chain hits the 8-level safety cap, instead of returning a non-topmost row and updating the wrong tracking comment. - src/workflows/tracking-mirror.ts: lost-race branch now re-fetches the row before re-rendering so we don't clobber the winner's freshly written body with our stale snapshot. - src/utils/reactions.ts: documented the floating-promise contract that makes parameter destructuring outside the try/catch safe under strict TS. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
chrisleekr
pushed a commit
that referenced
this pull request
Apr 26, 2026
# [1.4.0](v1.3.2...v1.4.0) (2026-04-26) ### Features * **workflows:** up-front tracking comments, trigger reactions, parent cascade ([#61](#61)) ([befe07c](befe07c))
Owner
Author
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This was referenced Apr 29, 2026
This was referenced May 8, 2026
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
Closes the silent-multi-minute UX gap during
triage/plan/implementruns (users currently see nothing until the agent finishes, sometimes 10+ minutes later) and the silent-failure window when a daemon dies mid-job (issue #16's OOM produced zero user-facing signal — thebot:shipchain just stopped).Adds three coordinated user-facing surfaces:
ship's tracking comment now renders each child step as### emoji workflow — statuswith cost/turns and a deep link to the per-step comment. Cascade fires automatically on every childsetState.Plus an end-state for the OOM scenario:
cleanupAfterDisconnectwalks orphanedworkflow_runs, finds the topmost ancestor (so a child failure surfaces on the parent's comment that the user is actually watching), updates that ancestor's tracking comment with a recoverable failure message, and adds 😕 to the trigger comment.Before
flowchart LR User["User comments<br/>@chrisleekr-bot ship"]:::user SilentTriage["triage runs silently<br/>~3min, no comment"]:::silent SilentPlan["plan runs silently<br/>~80s, no comment"]:::silent PlanComment["Plan ready comment<br/>posted at end"]:::ok SilentImpl["implement runs silently<br/>~10min"]:::silent OOM["Daemon OOM kill"]:::fail Nothing["No comment update<br/>No reaction<br/>User stares at stale Plan comment"]:::silent User --> SilentTriage --> SilentPlan --> PlanComment --> SilentImpl --> OOM --> Nothing classDef user fill:#1e3a8a,stroke:#bfdbfe,color:#ffffff classDef silent fill:#7f1d1d,stroke:#fecaca,color:#ffffff classDef ok fill:#065f46,stroke:#a7f3d0,color:#ffffff classDef fail fill:#7f1d1d,stroke:#fecaca,color:#ffffffAfter
flowchart LR User["User comments<br/>@chrisleekr-bot ship"]:::user Eyes["👀 reaction added<br/>immediately"]:::react Rocket["🚀 reaction added<br/>after dispatch"]:::react TriageStart["Triage starting comment<br/>posted up-front"]:::ok TriageDone["Triage verdict<br/>replaces start comment"]:::ok ShipBody["Ship comment shows<br/>composite with child steps<br/>updated on each setState"]:::ok ImplStart["Implement starting comment<br/>posted up-front"]:::ok OOM["Daemon OOM kill"]:::fail Confused["😕 reaction on trigger<br/>Ship comment updated<br/>with recoverable failure msg"]:::warn User --> Eyes --> Rocket --> TriageStart --> TriageDone --> ShipBody --> ImplStart --> OOM --> Confused classDef user fill:#1e3a8a,stroke:#bfdbfe,color:#ffffff classDef react fill:#0f766e,stroke:#99f6e4,color:#ffffff classDef ok fill:#065f46,stroke:#a7f3d0,color:#ffffff classDef fail fill:#7f1d1d,stroke:#fecaca,color:#ffffff classDef warn fill:#92400e,stroke:#fde68a,color:#ffffffRelated Issues
agent timeout never aborts SDK query) — independent of the underlying timeout bug; fixes the silent-failure UXCloses #here — this is a DX feature PR, not an issue fixTesting
Quality gates run locally
bun run typecheck— cleanbun run lint— 0 errors (130 pre-existing warnings unchanged)bun run format— clean (auto-formatted by lint-staged on commit)bun test test/utils/reactions.test.ts— 3/3 pass, 100% line coverage on the new modulebun test test/db/migrate.test.ts— 7/7 versions tracked (also fixes a pre-existing failure where the assertion fell behind migration 006)bun test test/workflows/runs-store.test.ts— 14/14 passSchema change
Migration
007_trigger_comment.sqladds two NULL columns to bothworkflow_runsandexecutions:trigger_comment_id BIGINT NULLtrigger_event_type TEXT NULL CHECK ('issue_comment','pull_request_review_comment')Both are NULL because label-triggered runs (
bot:shipvia label apply) have no originating comment. Pre-existing rows are not backfilled — the reaction lifecycle only matters for jobs dispatched after this migration.Files changed (19 files, +855 / -62 LOC)
src/db/migrations/007_trigger_comment.sql,test/db/migrate.test.tssrc/utils/reactions.ts,test/utils/reactions.test.tsaddReactionhelper, 100% coveredsrc/webhook/events/issue-comment.ts,review-comment.ts,test/webhook/events/issue-comment.test.tspayload.comment.id, fire 👀, thread through dispatchersrc/workflows/dispatcher.ts,execution-row.ts,runs-store.ts,src/orchestrator/history.tscommentId: 0; fire 🚀 on dispatchedsrc/workflows/handlers/triage.ts,plan.ts,implement.tssetStatewith input snapshotsrc/workflows/tracking-mirror.ts_lastHumanMessage, cascade refresh of parent composite bodysrc/workflows/orchestrator.tssrc/daemon/workflow-executor.tssrc/orchestrator/connection-handler.tsdocs/BOT-WORKFLOWS.mdScreenshots/Recordings
N/A — change is to GitHub-comment / GitHub-reaction surfaces. End-to-end UX is best observed by triggering a
bot:shipcycle on a real issue post-merge.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests