Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions docs/BOT-WORKFLOWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,42 @@ Composite workflows like `ship` insert a child row per step. When the child comp
- **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.
- **Example trigger**: add label `bot:ship`, or comment "`@chrisleekr-bot ship this`"

## User-facing surfaces

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.

### Tracking comments

- `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.
- `review` and `resolve` already post upfront; behaviour unchanged.
- 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.

### Trigger-comment reactions

Comment-driven workflows stack four GitHub reactions on the user's trigger comment so the lifecycle is visible without scrolling:

| Stage | Reaction | Where it fires |
| ------------------------------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------------------ |
| Trigger detected, before classifier | 👀 `eyes` | `src/webhook/events/issue-comment.ts`, `review-comment.ts` (after allowlist) |
| Job dispatched to a daemon | 🚀 `rocket` | `src/workflows/dispatcher.ts` (after `enqueueJob`) |
| Workflow succeeded | 🎉 `hooray` | `src/daemon/workflow-executor.ts` for atomic runs; `src/workflows/orchestrator.ts` for composite parents (cascade) |
| Workflow failed (handler error, daemon disconnect, OOM) | 😕 `confused` | `workflow-executor.ts`, `orchestrator.ts`, `src/orchestrator/connection-handler.ts` (orphan path) |

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.

### Failure surface on daemon disconnect

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:

1. Updates the ancestor's tracking comment with an `❌ Daemon disconnected (likely OOM)` message and resume instructions.
2. Adds 😕 `confused` to the user's trigger comment.

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.

### Re-trigger / resume

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.

## Comment intent classifier

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:
Expand Down
23 changes: 23 additions & 0 deletions src/daemon/workflow-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Octokit } from "octokit";
import { logger } from "../logger";
import type { SerializableBotContext } from "../shared/daemon-types";
import { createMessageEnvelope, type JobPayloadMessage } from "../shared/ws-messages";
import { addReaction, type ReactionContent } from "../utils/reactions";
import { type CompletionResult, onStepComplete } from "../workflows/orchestrator";
import { getByName, type WorkflowRunContext } from "../workflows/registry";
import { markFailed, markRunning, markSucceeded, mergeState } from "../workflows/runs-store";
Expand Down Expand Up @@ -59,6 +60,22 @@ export async function executeWorkflowRun(

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

// Best-effort reaction on the user's trigger comment. No-op for child runs
// (commentId === 0 because children inherit nothing from the parent's
// dispatch payload) and for label-triggered runs (no comment exists).
const reactOnTrigger = (content: ReactionContent): void => {
if (context.commentId === 0) return;
void addReaction({
octokit,
logger: log,
owner: context.owner,
repo: context.repo,
commentId: context.commentId,
eventType: context.eventName,
content,
});
};

try {
const entry = getByName(workflowRun.workflowName);
const daemonId = getDaemonId();
Expand Down Expand Up @@ -163,6 +180,8 @@ export async function executeWorkflowRun(
"Workflow run completed",
);

reactOnTrigger("hooray");

completion = { status: "succeeded" };

send({
Expand Down Expand Up @@ -202,6 +221,8 @@ export async function executeWorkflowRun(
"Workflow run reported failure",
);

reactOnTrigger("confused");

completion = { status: "failed", reason: result.reason };

send({
Expand Down Expand Up @@ -250,6 +271,8 @@ export async function executeWorkflowRun(
"Workflow handler threw",
);

reactOnTrigger("confused");

try {
await onStepComplete({ octokit, logger: log }, workflowRun.runId, {
status: "failed",
Expand Down
21 changes: 21 additions & 0 deletions src/db/migrations/007_trigger_comment.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
-- Migration 007: persist the user's trigger comment on workflow_runs and executions.
--
-- Required so the orphan/disconnect cleanup path (which has no live BotContext)
-- can update the right tracking comment and add a failure reaction (👀 → ❌)
-- when a daemon dies mid-job. Also lets every workflow stage update the same
-- trigger-comment reaction set throughout its lifecycle (eyes → rocket →
-- hooray → confused).
--
-- Both columns are NULL because label-triggered workflows (e.g. bot:ship via
-- issues.labeled) have no originating comment to react on. No backfill — the
-- reaction lifecycle only matters for jobs dispatched after this migration.

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'));

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'));
138 changes: 137 additions & 1 deletion src/orchestrator/connection-handler.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import type { ServerWebSocket } from "bun";
import { App } from "octokit";
import { App, type Octokit } from "octokit";

import { config } from "../config";
import { logger } from "../logger";
import { addReaction } from "../utils/reactions";
import { findById, findInflightByOwner, type WorkflowRunRow } from "../workflows/runs-store";
import { setState } from "../workflows/tracking-mirror";

// Read orchestrator app version at module load so we can detect daemon drift
// in handleRegister and request an update via daemon:update-required.
Expand Down Expand Up @@ -152,11 +155,144 @@ async function cleanupAfterDisconnect(daemonId: string): Promise<void> {
"Cleaned up orphaned executions after daemon disconnect",
);
}

// User-facing notification: any in-flight workflow_runs owned by this
// daemon will be flipped to 'failed' by the liveness reaper. We update
// the user's tracking comment + react on the trigger comment now so the
// user sees the failure immediately instead of staring at a stale
// "starting…" comment.
await notifyOrphanedWorkflowRuns(daemonId);
} catch (err) {
logger.error({ err, daemonId }, "Failed to cleanup after daemon disconnect");
}
}

/**
* Update the user-facing tracking comment + add a `confused` reaction on the
* originating comment for every in-flight workflow_run owned by the dying
* daemon. Walks the parent chain so a child step's failure shows up on the
* top-level run's surface (the surface the user is actually watching) rather
* than on a per-child comment they may not have noticed.
*
* Best-effort throughout — a missing GitHub App config or a comment-update
* failure must never bubble up and prevent the rest of cleanup from running.
*/
async function notifyOrphanedWorkflowRuns(daemonId: string): Promise<void> {
let inflight: WorkflowRunRow[];
try {
inflight = await findInflightByOwner("daemon", daemonId);
} catch (err) {
logger.error({ err, daemonId }, "Failed to query in-flight workflow_runs for orphan cleanup");
return;
}

if (inflight.length === 0) return;

// Dedupe by ancestor so a single ship cascade (ship → plan → implement)
// only updates one comment + one reaction even if multiple of its rows
// were owned by this daemon at the moment of disconnect.
const ancestorIds = new Set<string>();
for (const row of inflight) {
// eslint-disable-next-line no-await-in-loop
const ancestor = await findTopAncestor(row);
if (ancestor === null) continue;
if (ancestorIds.has(ancestor.id)) continue;
ancestorIds.add(ancestor.id);

try {
// eslint-disable-next-line no-await-in-loop
await postOrphanNotification(ancestor);
} catch (err) {
logger.warn(
{
err: err instanceof Error ? err.message : String(err),
ancestorRunId: ancestor.id,
daemonId,
},
"Orphan notification (comment/reaction) failed",
);
}
}
}

/**
* Walk parent_run_id up to the topmost row. Returns the input row if it has
* no parent, or null if the parent chain is broken (orphaned mid-walk) or
* exceeds the safety cap. Returning the cap-iteration row would be a silent
* bug: it still has a non-null parent_run_id, so we'd update the wrong
* (mid-chain) tracking comment.
*/
async function findTopAncestor(row: WorkflowRunRow): Promise<WorkflowRunRow | null> {
let current: WorkflowRunRow | null = row;
// Bound at 8 levels of nesting — defensive cap for a chain that should
// realistically never exceed depth 2 (ship → step). A null parent ends
// the walk naturally.
for (let i = 0; i < 8; i++) {
if (current === null) return null;
if (current.parent_run_id === null) return current;
// eslint-disable-next-line no-await-in-loop
current = await findById(current.parent_run_id);
}
logger.warn(
{ startRunId: row.id, lastSeenRunId: current?.id ?? null },
"findTopAncestor: parent chain exceeded 8 levels — skipping orphan notification to avoid touching the wrong comment",
);
return null;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

async function postOrphanNotification(ancestor: WorkflowRunRow): Promise<void> {
if (config.appId === undefined || config.privateKey === undefined) {
logger.debug(
{ ancestorRunId: ancestor.id },
"Skipping orphan notification — GitHub App credentials not configured",
);
return;
}

const app = getOrCreateApp();
const { data: installation } = await app.octokit.rest.apps.getRepoInstallation({
owner: ancestor.target_owner,
repo: ancestor.target_repo,
});
const octokit = await app.getInstallationOctokit(installation.id);

const humanMessage = [
`❌ **Daemon disconnected during execution** — likely an OOM kill on the workflow pod.`,
``,
`The in-flight step has been marked failed. Its workflow_run row will be flipped`,
`to \`failed\` by the liveness reaper. To resume, re-trigger the workflow:`,
``,
`- For \`ship\`: re-apply the \`bot:ship\` label, or comment again. Resume picks up`,
` from the failed step and reuses prior succeeded steps.`,
`- For standalone workflows: re-comment with the same trigger.`,
].join("\n");

// Re-uses tracking-mirror.setState so the cascade refresh and `_lastHumanMessage`
// bookkeeping stay consistent — and so the parent's composite body picks up the
// failure narrative on the next render.
const installationOctokit = octokit as unknown as Octokit;
await setState(
{ octokit: installationOctokit, logger },
{
runId: ancestor.id,
patch: { phase: "orphaned" },
humanMessage,
},
);

if (ancestor.trigger_comment_id !== null && ancestor.trigger_event_type !== null) {
await addReaction({
octokit: installationOctokit,
logger,
owner: ancestor.target_owner,
repo: ancestor.target_repo,
commentId: ancestor.trigger_comment_id,
eventType: ancestor.trigger_event_type,
content: "confused",
});
}
}

/** Route validated daemon messages to type-specific handlers. */
export function handleDaemonMessage(
ws: ServerWebSocket<WsConnectionData>,
Expand Down
23 changes: 17 additions & 6 deletions src/orchestrator/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ export interface CreateExecutionParams {
triageConfidence?: number;
triageCostUsd?: number;
contextJson?: SerializableBotContext;
/**
* REST id of the user comment that triggered this run, persisted so the
* orphan/disconnect path (which has no live BotContext) can still react
* on the right comment after a daemon dies. NULL for label/system runs.
*/
triggerCommentId?: number | null;
triggerEventType?: "issue_comment" | "pull_request_review_comment" | null;
}

/**
Expand All @@ -63,21 +70,24 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
// CHECK (= 'daemon'). Hardcoding the literal here makes the invariant
// unbreakable at the data-layer boundary — a stray caller passing any
// other `dispatchMode` value cannot fail the INSERT at runtime.
const triggerCommentId = params.triggerCommentId ?? null;
const triggerEventType = params.triggerEventType ?? null;

let rows: { id: string }[];
if (hasTriageFields) {
rows = await db`
INSERT INTO executions (
delivery_id, repo_owner, repo_name, entity_number, entity_type,
event_name, trigger_username, dispatch_mode, dispatch_target, dispatch_reason,
triage_confidence, triage_cost_usd,
status, context_json
status, context_json, trigger_comment_id, trigger_event_type
) VALUES (
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
${params.entityNumber}, ${params.entityType}, ${params.eventName},
${params.triggerUsername}, 'daemon', 'daemon',
${params.dispatchReason ?? "persistent-daemon"},
${params.triageConfidence ?? null}, ${params.triageCostUsd ?? null},
'queued', ${params.contextJson ?? null}
'queued', ${params.contextJson ?? null}, ${triggerCommentId}, ${triggerEventType}
)
RETURNING id
`;
Expand All @@ -86,25 +96,26 @@ export async function createExecution(params: CreateExecutionParams): Promise<st
INSERT INTO executions (
delivery_id, repo_owner, repo_name, entity_number, entity_type,
event_name, trigger_username, dispatch_mode, dispatch_target, dispatch_reason,
status, context_json
status, context_json, trigger_comment_id, trigger_event_type
) VALUES (
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
${params.entityNumber}, ${params.entityType}, ${params.eventName},
${params.triggerUsername}, 'daemon', 'daemon', ${params.dispatchReason},
'queued', ${params.contextJson ?? null}
'queued', ${params.contextJson ?? null}, ${triggerCommentId}, ${triggerEventType}
)
RETURNING id
`;
} else {
rows = await db`
INSERT INTO executions (
delivery_id, repo_owner, repo_name, entity_number, entity_type,
event_name, trigger_username, dispatch_mode, dispatch_target, status, context_json
event_name, trigger_username, dispatch_mode, dispatch_target, status, context_json,
trigger_comment_id, trigger_event_type
) VALUES (
${params.deliveryId}, ${params.repoOwner}, ${params.repoName},
${params.entityNumber}, ${params.entityType}, ${params.eventName},
${params.triggerUsername}, 'daemon', 'daemon', 'queued',
${params.contextJson ?? null}
${params.contextJson ?? null}, ${triggerCommentId}, ${triggerEventType}
)
RETURNING id
`;
Expand Down
13 changes: 13 additions & 0 deletions src/shared/dispatch-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,19 @@ export const DISPATCH_TARGETS = ["daemon"] as const;

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

/**
* TriggerEventType — the GitHub webhook event class for the user comment that
* started a workflow run. Drives which Octokit reactions endpoint is used
* downstream (`createForIssueComment` vs `createForPullRequestReviewComment`).
*
* Persisted on `workflow_runs.trigger_event_type` and `executions.trigger_event_type`
* (see migration `007_trigger_comment.sql`). NULL on label-triggered runs.
*
* Single source of truth — `dispatcher.ts`, `runs-store.ts`, `execution-row.ts`,
* and `utils/reactions.ts` import from here so the union can't silently drift.
*/
export type TriggerEventType = "issue_comment" | "pull_request_review_comment";

export const DispatchTargetSchema = z.enum(DISPATCH_TARGETS);

/**
Expand Down
Loading
Loading