Skip to content

Commit befe07c

Browse files
chrisleekrclaude
andauthored
feat(workflows): up-front tracking comments, trigger reactions, parent cascade (#61)
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 04f6fcc commit befe07c

20 files changed

Lines changed: 879 additions & 61 deletions

docs/BOT-WORKFLOWS.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,42 @@ Composite workflows like `ship` insert a child row per step. When the child comp
143143
- **Cost note**: every ship pays for both `review` and `resolve` agent runs. Per project direction (2026-04-25), accuracy beats cost — closing the loop justifies the extra spend.
144144
- **Example trigger**: add label `bot:ship`, or comment "`@chrisleekr-bot ship this`"
145145

146+
## User-facing surfaces
147+
148+
Each workflow run produces two GitHub-visible signals: a **tracking comment** (the bot's working/result body) and a **reaction set** on the user's trigger comment.
149+
150+
### Tracking comments
151+
152+
- `triage`, `plan`, and `implement` post an **up-front "starting…" comment** as soon as they fetch the issue title, before the (multi-minute) agent run. The terminal `setState` call rewrites the same comment with the verdict / plan / PR link. Skipping the up-front write would leave the user staring at an empty issue while the daemon worked.
153+
- `review` and `resolve` already post upfront; behaviour unchanged.
154+
- For composite parents (`ship`), the tracking comment is rendered as a **verbose composite**: the parent's narrative followed by one `### <emoji> <step> — <status>` block per child step, each linking back to the child's own tracking comment via deep `#issuecomment-<id>` anchors. The composite refresh is triggered automatically by `tracking-mirror.setState` whenever a child run writes — the cascade walks `parent_run_id` and re-renders the parent's body so the user always sees the latest child status on the surface they're already watching.
155+
156+
### Trigger-comment reactions
157+
158+
Comment-driven workflows stack four GitHub reactions on the user's trigger comment so the lifecycle is visible without scrolling:
159+
160+
| Stage | Reaction | Where it fires |
161+
| ------------------------------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------------------ |
162+
| Trigger detected, before classifier | 👀 `eyes` | `src/webhook/events/issue-comment.ts`, `review-comment.ts` (after allowlist) |
163+
| Job dispatched to a daemon | 🚀 `rocket` | `src/workflows/dispatcher.ts` (after `enqueueJob`) |
164+
| Workflow succeeded | 🎉 `hooray` | `src/daemon/workflow-executor.ts` for atomic runs; `src/workflows/orchestrator.ts` for composite parents (cascade) |
165+
| Workflow failed (handler error, daemon disconnect, OOM) | 😕 `confused` | `workflow-executor.ts`, `orchestrator.ts`, `src/orchestrator/connection-handler.ts` (orphan path) |
166+
167+
GitHub reactions are additive — the combined set is the audit trail. Label-triggered runs (`bot:ship` via label apply) skip reactions silently because no comment exists to react on. Reaction failures (e.g., missing `reactions:write` scope) are logged at warn level and swallowed; they never block a workflow.
168+
169+
### Failure surface on daemon disconnect
170+
171+
When a daemon dies abruptly (OOM, pod eviction, network partition), `connection-handler.cleanupAfterDisconnect` walks every in-flight `workflow_runs` row owned by that daemon, finds the topmost ancestor (so a child step failure shows up on the parent's surface), and:
172+
173+
1. Updates the ancestor's tracking comment with an `❌ Daemon disconnected (likely OOM)` message and resume instructions.
174+
2. Adds 😕 `confused` to the user's trigger comment.
175+
176+
This closes the silent-failure window that previously left users staring at a stale "starting…" comment after an OOM. The liveness reaper still flips the `workflow_runs.status` to `failed`; the cleanup path only owns the user-visible surface.
177+
178+
### Re-trigger / resume
179+
180+
Re-triggering `ship` (re-applying the `bot:ship` label or re-commenting the intent) walks the prior runs via `computeStartIndex` in `src/workflows/handlers/ship.ts`: succeeded `triage`/`plan` rows are reused, succeeded `implement` is reused only while its PR is still open, and `review`/`resolve` always re-run. A failed `implement` row from a prior crash means resume picks up at `implement` — the row is not "succeeded" so `isFresh` returns false and the step is re-queued.
181+
146182
## Comment intent classifier
147183

148184
Comments that mention `@chrisleekr-bot` are routed through `src/workflows/intent-classifier.ts`, which returns `{ workflow, confidence, rationale }` using a single-turn Haiku call. Rules:

src/daemon/workflow-executor.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { Octokit } from "octokit";
33
import { logger } from "../logger";
44
import type { SerializableBotContext } from "../shared/daemon-types";
55
import { createMessageEnvelope, type JobPayloadMessage } from "../shared/ws-messages";
6+
import { addReaction, type ReactionContent } from "../utils/reactions";
67
import { type CompletionResult, onStepComplete } from "../workflows/orchestrator";
78
import { getByName, type WorkflowRunContext } from "../workflows/registry";
89
import { markFailed, markRunning, markSucceeded, mergeState } from "../workflows/runs-store";
@@ -59,6 +60,22 @@ export async function executeWorkflowRun(
5960

6061
const octokit = new Octokit({ auth: installationToken });
6162

63+
// Best-effort reaction on the user's trigger comment. No-op for child runs
64+
// (commentId === 0 because children inherit nothing from the parent's
65+
// dispatch payload) and for label-triggered runs (no comment exists).
66+
const reactOnTrigger = (content: ReactionContent): void => {
67+
if (context.commentId === 0) return;
68+
void addReaction({
69+
octokit,
70+
logger: log,
71+
owner: context.owner,
72+
repo: context.repo,
73+
commentId: context.commentId,
74+
eventType: context.eventName,
75+
content,
76+
});
77+
};
78+
6279
try {
6380
const entry = getByName(workflowRun.workflowName);
6481
const daemonId = getDaemonId();
@@ -163,6 +180,8 @@ export async function executeWorkflowRun(
163180
"Workflow run completed",
164181
);
165182

183+
reactOnTrigger("hooray");
184+
166185
completion = { status: "succeeded" };
167186

168187
send({
@@ -202,6 +221,8 @@ export async function executeWorkflowRun(
202221
"Workflow run reported failure",
203222
);
204223

224+
reactOnTrigger("confused");
225+
205226
completion = { status: "failed", reason: result.reason };
206227

207228
send({
@@ -250,6 +271,8 @@ export async function executeWorkflowRun(
250271
"Workflow handler threw",
251272
);
252273

274+
reactOnTrigger("confused");
275+
253276
try {
254277
await onStepComplete({ octokit, logger: log }, workflowRun.runId, {
255278
status: "failed",
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
-- Migration 007: persist the user's trigger comment on workflow_runs and executions.
2+
--
3+
-- Required so the orphan/disconnect cleanup path (which has no live BotContext)
4+
-- can update the right tracking comment and add a failure reaction (👀 → ❌)
5+
-- when a daemon dies mid-job. Also lets every workflow stage update the same
6+
-- trigger-comment reaction set throughout its lifecycle (eyes → rocket →
7+
-- hooray → confused).
8+
--
9+
-- Both columns are NULL because label-triggered workflows (e.g. bot:ship via
10+
-- issues.labeled) have no originating comment to react on. No backfill — the
11+
-- reaction lifecycle only matters for jobs dispatched after this migration.
12+
13+
ALTER TABLE workflow_runs
14+
ADD COLUMN trigger_comment_id BIGINT NULL,
15+
ADD COLUMN trigger_event_type TEXT NULL
16+
CHECK (trigger_event_type IN ('issue_comment', 'pull_request_review_comment'));
17+
18+
ALTER TABLE executions
19+
ADD COLUMN trigger_comment_id BIGINT NULL,
20+
ADD COLUMN trigger_event_type TEXT NULL
21+
CHECK (trigger_event_type IN ('issue_comment', 'pull_request_review_comment'));

src/orchestrator/connection-handler.ts

Lines changed: 137 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
import type { ServerWebSocket } from "bun";
2-
import { App } from "octokit";
2+
import { App, type Octokit } from "octokit";
33

44
import { config } from "../config";
55
import { logger } from "../logger";
6+
import { addReaction } from "../utils/reactions";
7+
import { findById, findInflightByOwner, type WorkflowRunRow } from "../workflows/runs-store";
8+
import { setState } from "../workflows/tracking-mirror";
69

710
// Read orchestrator app version at module load so we can detect daemon drift
811
// in handleRegister and request an update via daemon:update-required.
@@ -152,11 +155,144 @@ async function cleanupAfterDisconnect(daemonId: string): Promise<void> {
152155
"Cleaned up orphaned executions after daemon disconnect",
153156
);
154157
}
158+
159+
// User-facing notification: any in-flight workflow_runs owned by this
160+
// daemon will be flipped to 'failed' by the liveness reaper. We update
161+
// the user's tracking comment + react on the trigger comment now so the
162+
// user sees the failure immediately instead of staring at a stale
163+
// "starting…" comment.
164+
await notifyOrphanedWorkflowRuns(daemonId);
155165
} catch (err) {
156166
logger.error({ err, daemonId }, "Failed to cleanup after daemon disconnect");
157167
}
158168
}
159169

170+
/**
171+
* Update the user-facing tracking comment + add a `confused` reaction on the
172+
* originating comment for every in-flight workflow_run owned by the dying
173+
* daemon. Walks the parent chain so a child step's failure shows up on the
174+
* top-level run's surface (the surface the user is actually watching) rather
175+
* than on a per-child comment they may not have noticed.
176+
*
177+
* Best-effort throughout — a missing GitHub App config or a comment-update
178+
* failure must never bubble up and prevent the rest of cleanup from running.
179+
*/
180+
async function notifyOrphanedWorkflowRuns(daemonId: string): Promise<void> {
181+
let inflight: WorkflowRunRow[];
182+
try {
183+
inflight = await findInflightByOwner("daemon", daemonId);
184+
} catch (err) {
185+
logger.error({ err, daemonId }, "Failed to query in-flight workflow_runs for orphan cleanup");
186+
return;
187+
}
188+
189+
if (inflight.length === 0) return;
190+
191+
// Dedupe by ancestor so a single ship cascade (ship → plan → implement)
192+
// only updates one comment + one reaction even if multiple of its rows
193+
// were owned by this daemon at the moment of disconnect.
194+
const ancestorIds = new Set<string>();
195+
for (const row of inflight) {
196+
// eslint-disable-next-line no-await-in-loop
197+
const ancestor = await findTopAncestor(row);
198+
if (ancestor === null) continue;
199+
if (ancestorIds.has(ancestor.id)) continue;
200+
ancestorIds.add(ancestor.id);
201+
202+
try {
203+
// eslint-disable-next-line no-await-in-loop
204+
await postOrphanNotification(ancestor);
205+
} catch (err) {
206+
logger.warn(
207+
{
208+
err: err instanceof Error ? err.message : String(err),
209+
ancestorRunId: ancestor.id,
210+
daemonId,
211+
},
212+
"Orphan notification (comment/reaction) failed",
213+
);
214+
}
215+
}
216+
}
217+
218+
/**
219+
* Walk parent_run_id up to the topmost row. Returns the input row if it has
220+
* no parent, or null if the parent chain is broken (orphaned mid-walk) or
221+
* exceeds the safety cap. Returning the cap-iteration row would be a silent
222+
* bug: it still has a non-null parent_run_id, so we'd update the wrong
223+
* (mid-chain) tracking comment.
224+
*/
225+
async function findTopAncestor(row: WorkflowRunRow): Promise<WorkflowRunRow | null> {
226+
let current: WorkflowRunRow | null = row;
227+
// Bound at 8 levels of nesting — defensive cap for a chain that should
228+
// realistically never exceed depth 2 (ship → step). A null parent ends
229+
// the walk naturally.
230+
for (let i = 0; i < 8; i++) {
231+
if (current === null) return null;
232+
if (current.parent_run_id === null) return current;
233+
// eslint-disable-next-line no-await-in-loop
234+
current = await findById(current.parent_run_id);
235+
}
236+
logger.warn(
237+
{ startRunId: row.id, lastSeenRunId: current?.id ?? null },
238+
"findTopAncestor: parent chain exceeded 8 levels — skipping orphan notification to avoid touching the wrong comment",
239+
);
240+
return null;
241+
}
242+
243+
async function postOrphanNotification(ancestor: WorkflowRunRow): Promise<void> {
244+
if (config.appId === undefined || config.privateKey === undefined) {
245+
logger.debug(
246+
{ ancestorRunId: ancestor.id },
247+
"Skipping orphan notification — GitHub App credentials not configured",
248+
);
249+
return;
250+
}
251+
252+
const app = getOrCreateApp();
253+
const { data: installation } = await app.octokit.rest.apps.getRepoInstallation({
254+
owner: ancestor.target_owner,
255+
repo: ancestor.target_repo,
256+
});
257+
const octokit = await app.getInstallationOctokit(installation.id);
258+
259+
const humanMessage = [
260+
`❌ **Daemon disconnected during execution** — likely an OOM kill on the workflow pod.`,
261+
``,
262+
`The in-flight step has been marked failed. Its workflow_run row will be flipped`,
263+
`to \`failed\` by the liveness reaper. To resume, re-trigger the workflow:`,
264+
``,
265+
`- For \`ship\`: re-apply the \`bot:ship\` label, or comment again. Resume picks up`,
266+
` from the failed step and reuses prior succeeded steps.`,
267+
`- For standalone workflows: re-comment with the same trigger.`,
268+
].join("\n");
269+
270+
// Re-uses tracking-mirror.setState so the cascade refresh and `_lastHumanMessage`
271+
// bookkeeping stay consistent — and so the parent's composite body picks up the
272+
// failure narrative on the next render.
273+
const installationOctokit = octokit as unknown as Octokit;
274+
await setState(
275+
{ octokit: installationOctokit, logger },
276+
{
277+
runId: ancestor.id,
278+
patch: { phase: "orphaned" },
279+
humanMessage,
280+
},
281+
);
282+
283+
if (ancestor.trigger_comment_id !== null && ancestor.trigger_event_type !== null) {
284+
await addReaction({
285+
octokit: installationOctokit,
286+
logger,
287+
owner: ancestor.target_owner,
288+
repo: ancestor.target_repo,
289+
commentId: ancestor.trigger_comment_id,
290+
eventType: ancestor.trigger_event_type,
291+
content: "confused",
292+
});
293+
}
294+
}
295+
160296
/** Route validated daemon messages to type-specific handlers. */
161297
export function handleDaemonMessage(
162298
ws: ServerWebSocket<WsConnectionData>,

src/orchestrator/history.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,13 @@ export interface CreateExecutionParams {
3737
triageConfidence?: number;
3838
triageCostUsd?: number;
3939
contextJson?: SerializableBotContext;
40+
/**
41+
* REST id of the user comment that triggered this run, persisted so the
42+
* orphan/disconnect path (which has no live BotContext) can still react
43+
* on the right comment after a daemon dies. NULL for label/system runs.
44+
*/
45+
triggerCommentId?: number | null;
46+
triggerEventType?: "issue_comment" | "pull_request_review_comment" | null;
4047
}
4148

4249
/**
@@ -63,21 +70,24 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
6370
// CHECK (= 'daemon'). Hardcoding the literal here makes the invariant
6471
// unbreakable at the data-layer boundary — a stray caller passing any
6572
// other `dispatchMode` value cannot fail the INSERT at runtime.
73+
const triggerCommentId = params.triggerCommentId ?? null;
74+
const triggerEventType = params.triggerEventType ?? null;
75+
6676
let rows: { id: string }[];
6777
if (hasTriageFields) {
6878
rows = await db`
6979
INSERT INTO executions (
7080
delivery_id, repo_owner, repo_name, entity_number, entity_type,
7181
event_name, trigger_username, dispatch_mode, dispatch_target, dispatch_reason,
7282
triage_confidence, triage_cost_usd,
73-
status, context_json
83+
status, context_json, trigger_comment_id, trigger_event_type
7484
) VALUES (
7585
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
7686
${params.entityNumber}, ${params.entityType}, ${params.eventName},
7787
${params.triggerUsername}, 'daemon', 'daemon',
7888
${params.dispatchReason ?? "persistent-daemon"},
7989
${params.triageConfidence ?? null}, ${params.triageCostUsd ?? null},
80-
'queued', ${params.contextJson ?? null}
90+
'queued', ${params.contextJson ?? null}, ${triggerCommentId}, ${triggerEventType}
8191
)
8292
RETURNING id
8393
`;
@@ -86,25 +96,26 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
8696
INSERT INTO executions (
8797
delivery_id, repo_owner, repo_name, entity_number, entity_type,
8898
event_name, trigger_username, dispatch_mode, dispatch_target, dispatch_reason,
89-
status, context_json
99+
status, context_json, trigger_comment_id, trigger_event_type
90100
) VALUES (
91101
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
92102
${params.entityNumber}, ${params.entityType}, ${params.eventName},
93103
${params.triggerUsername}, 'daemon', 'daemon', ${params.dispatchReason},
94-
'queued', ${params.contextJson ?? null}
104+
'queued', ${params.contextJson ?? null}, ${triggerCommentId}, ${triggerEventType}
95105
)
96106
RETURNING id
97107
`;
98108
} else {
99109
rows = await db`
100110
INSERT INTO executions (
101111
delivery_id, repo_owner, repo_name, entity_number, entity_type,
102-
event_name, trigger_username, dispatch_mode, dispatch_target, status, context_json
112+
event_name, trigger_username, dispatch_mode, dispatch_target, status, context_json,
113+
trigger_comment_id, trigger_event_type
103114
) VALUES (
104115
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
105116
${params.entityNumber}, ${params.entityType}, ${params.eventName},
106117
${params.triggerUsername}, 'daemon', 'daemon', 'queued',
107-
${params.contextJson ?? null}
118+
${params.contextJson ?? null}, ${triggerCommentId}, ${triggerEventType}
108119
)
109120
RETURNING id
110121
`;

src/shared/dispatch-types.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,19 @@ export const DISPATCH_TARGETS = ["daemon"] as const;
1313

1414
export type DispatchTarget = (typeof DISPATCH_TARGETS)[number];
1515

16+
/**
17+
* TriggerEventType — the GitHub webhook event class for the user comment that
18+
* started a workflow run. Drives which Octokit reactions endpoint is used
19+
* downstream (`createForIssueComment` vs `createForPullRequestReviewComment`).
20+
*
21+
* Persisted on `workflow_runs.trigger_event_type` and `executions.trigger_event_type`
22+
* (see migration `007_trigger_comment.sql`). NULL on label-triggered runs.
23+
*
24+
* Single source of truth — `dispatcher.ts`, `runs-store.ts`, `execution-row.ts`,
25+
* and `utils/reactions.ts` import from here so the union can't silently drift.
26+
*/
27+
export type TriggerEventType = "issue_comment" | "pull_request_review_comment";
28+
1629
export const DispatchTargetSchema = z.enum(DISPATCH_TARGETS);
1730

1831
/**

0 commit comments

Comments
 (0)