From 284c6ffbb7459b8d80a6fba8d159dc7aeda9a3fc Mon Sep 17 00:00:00 2001 From: aaight Date: Thu, 25 Jun 2026 15:13:44 +0200 Subject: [PATCH 1/8] =?UTF-8?q?feat(web):=20show=20"Run=20is=20starting?= =?UTF-8?q?=E2=80=A6"=20pending=20state=20for=20fresh=20run=20links=20(#14?= =?UTF-8?q?54)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A freshly-shared /runs/ URL can point at a run row the worker has not committed yet (the window between "URL shared in a GitHub/PM comment" and "worker commits the run row"). Previously the page flashed a misleading terminal "Run not found" immediately. - Add shared RunPendingState component (Loader2 spinner + "Run is starting…" heading + subtext, optional message prop) for reuse by the work-item runs page in a follow-up story. - Add NOT_FOUND-aware retry to the runs.getById query (retry only while the error is NOT_FOUND and within RUN_PENDING_MAX_RETRIES, retryDelay RUN_PENDING_POLL_MS) so the page self-heals to the real run within seconds. - Replace the terminal "Run not found" branch with resolveRunDetailView() branching (pending / loading / not-found / error / ready); soften the not-found copy to acknowledge the run may have been cancelled/removed. - Preserve the existing 5s refetchInterval while status === running. Co-authored-by: Cascade Bot Co-authored-by: Claude Opus 4.8 --- tests/unit/web/run-pending-state.test.ts | 44 +++++++++++++ web/src/components/runs/run-pending-state.tsx | 35 +++++++++++ web/src/routes/runs/$runId.tsx | 61 +++++++++++++++++-- 3 files changed, 135 insertions(+), 5 deletions(-) create mode 100644 tests/unit/web/run-pending-state.test.ts create mode 100644 web/src/components/runs/run-pending-state.tsx diff --git a/tests/unit/web/run-pending-state.test.ts b/tests/unit/web/run-pending-state.test.ts new file mode 100644 index 00000000..53934a62 --- /dev/null +++ b/tests/unit/web/run-pending-state.test.ts @@ -0,0 +1,44 @@ +import { createElement } from 'react'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { describe, expect, it } from 'vitest'; + +import { RunPendingState } from '../../../web/src/components/runs/run-pending-state.js'; + +/** + * Tests for the shared RunPendingState placeholder (MNG-1679). + * + * The web test suite runs in a node environment with no jsdom, so the component + * is rendered to a static HTML string via `react-dom/server` and the output is + * asserted — mirroring the style of tests/unit/web/trello-webhook-step.test.ts. + */ + +describe('RunPendingState', () => { + it('renders the "Run is starting…" heading', () => { + const html = renderToStaticMarkup(createElement(RunPendingState, {})); + expect(html).toContain('Run is starting'); + }); + + it('renders an animate-spin spinner', () => { + const html = renderToStaticMarkup(createElement(RunPendingState, {})); + expect(html).toContain('animate-spin'); + }); + + it('uses the shared py-8 text-center text-muted-foreground container styling', () => { + const html = renderToStaticMarkup(createElement(RunPendingState, {})); + expect(html).toContain('py-8 text-center text-muted-foreground'); + }); + + it('renders the default subtext when no message prop is provided', () => { + const html = renderToStaticMarkup(createElement(RunPendingState, {})); + expect(html).toContain('update automatically'); + }); + + it('renders a custom message when the message prop is provided', () => { + const html = renderToStaticMarkup( + createElement(RunPendingState, { message: 'Waiting for the worker to boot' }), + ); + expect(html).toContain('Waiting for the worker to boot'); + // The default subtext is replaced, not appended. + expect(html).not.toContain('update automatically'); + }); +}); diff --git a/web/src/components/runs/run-pending-state.tsx b/web/src/components/runs/run-pending-state.tsx new file mode 100644 index 00000000..b36c8d32 --- /dev/null +++ b/web/src/components/runs/run-pending-state.tsx @@ -0,0 +1,35 @@ +import { Loader2 } from 'lucide-react'; + +/** Default subtext shown beneath the "Run is starting…" heading. */ +const DEFAULT_PENDING_MESSAGE = + 'This page will update automatically once the run begins (usually a few seconds).'; + +interface RunPendingStateProps { + /** + * Optional override for the subtext shown beneath the heading. Lets callers + * (e.g. the work-item runs page) reuse the same pending visual with copy + * tailored to their context. + */ + message?: string; +} + +/** + * Pending placeholder rendered while a freshly-dispatched run row is still being + * materialized by the worker pipeline. + * + * A freshly-shared `/runs/` link can point at a run row that does not exist + * yet — the window between "URL shared in a GitHub/PM comment" and "worker + * commits the run row". Showing this "Run is starting…" state (and polling + * within a bounded grace window) avoids a misleading instant "Run not found". + * + * Reused by the run-detail page (`/runs/$runId`) and the work-item runs page. + */ +export function RunPendingState({ message }: RunPendingStateProps) { + return ( +
+
+ ); +} diff --git a/web/src/routes/runs/$runId.tsx b/web/src/routes/runs/$runId.tsx index 631b7eaf..550849fa 100644 --- a/web/src/routes/runs/$runId.tsx +++ b/web/src/routes/runs/$runId.tsx @@ -7,14 +7,51 @@ import { LlmCallList } from '@/components/llm-calls/llm-call-list.js'; import { LogViewer } from '@/components/logs/log-viewer.js'; import { CancelRunButton } from '@/components/runs/cancel-run-button.js'; import { RetryRunButton } from '@/components/runs/retry-run-button.js'; +import { RunPendingState } from '@/components/runs/run-pending-state.js'; import { RunStatusBadge } from '@/components/runs/run-status-badge.js'; import { RunSummaryCard } from '@/components/runs/run-summary-card.js'; +import { + isNotFoundError, + RUN_PENDING_MAX_RETRIES, + RUN_PENDING_POLL_MS, + type RunDetailView, + resolveRunDetailView, +} from '@/lib/run-pending.js'; import { trpc } from '@/lib/trpc.js'; import { cn } from '@/lib/utils.js'; import { rootRoute } from '../__root.js'; type Tab = 'overview' | 'logs' | 'llm-calls' | 'debug'; +/** + * Renders the non-ready states for the run-detail page. `pending` shows the + * shared "Run is starting…" placeholder (a freshly-shared link can resolve + * before the worker commits the run row); `not-found` softens the copy to + * acknowledge the run may have been cancelled/removed; `error` surfaces the + * underlying message. + */ +function RunDetailPlaceholder({ view, error }: { view: RunDetailView; error: unknown }) { + if (view === 'pending') { + return ; + } + if (view === 'loading') { + return
Loading run...
; + } + if (view === 'not-found') { + return ( +
+ Run not found. It may have been cancelled or removed. +
+ ); + } + // 'error' (and the unreachable 'ready' fallback): surface the error message. + return ( +
+ {error instanceof Error ? error.message : 'Failed to load run'} +
+ ); +} + function RunDetailPage() { const { runId } = runDetailRoute.useParams(); const [activeTab, setActiveTab] = useState('overview'); @@ -23,14 +60,28 @@ function RunDetailPage() { ...trpc.runs.getById.queryOptions({ id: runId }), // Poll while the run is active so status + the live-updating tabs refresh. refetchInterval: (query) => (query.state.data?.status === 'running' ? 5000 : false), + // A freshly-shared /runs/ link can resolve before the worker has + // committed the run row. Retry NOT_FOUND within a bounded grace window so + // the page resolves to the real run within seconds instead of flashing + // "Run not found"; other errors are surfaced immediately. + retry: (failureCount, error) => + isNotFoundError(error) && failureCount < RUN_PENDING_MAX_RETRIES, + retryDelay: RUN_PENDING_POLL_MS, }); - if (runQuery.isLoading) { - return
Loading run...
; - } + const view = resolveRunDetailView({ + hasData: !!runQuery.data, + isError: runQuery.isError, + error: runQuery.error, + failureCount: runQuery.failureCount, + failureReason: runQuery.failureReason, + }); - if (runQuery.isError || !runQuery.data) { - return
Run not found
; + // Anything other than a resolved run (pending / loading / not-found / error) + // renders a placeholder. The `!runQuery.data` clause is an unreachable + // type-narrowing guard so `run` below is non-null. + if (view !== 'ready' || !runQuery.data) { + return ; } const run = runQuery.data; From f412c16d471ec7108f8ba194cd6ba4d82b5cada4 Mon Sep 17 00:00:00 2001 From: aaight Date: Thu, 25 Jun 2026 15:57:54 +0200 Subject: [PATCH 2/8] =?UTF-8?q?feat(web):=20show=20"Run=20is=20starting?= =?UTF-8?q?=E2=80=A6"=20for=20ack-time=20work-item=20runs=20links=20(#1455?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(web): show "Run is starting…" for ack-time work-item runs links Work-item runs links (`/work-items/$projectId/$workItemId`) are posted at ack time — before the worker creates the run row — so `workItems.runs` returns `[]` and the page flashed a terminal "No runs found". Track mount time, keep polling through the bounded grace window, and render the shared `RunPendingState` while empty-within-grace. - Route tracks `mountedAt = useRef(Date.now())` and computes `elapsedMs`. - Replace the bespoke refetchInterval with the shared `workItemRunsRefetchInterval({ hasRunning, isEmpty, elapsedMs })` so the page polls through the ack-before-create window. - Derive `isPending` from `resolveWorkItemRunsView(...)` and pass it into `WorkItemRunsTable`; the empty branch renders `RunPendingState` when pending, else keeps "No runs found" (default preserved for the PR page). - Reuses the `run-pending.ts` helper + `RunPendingState` from the earlier stories — no new logic primitives. - Doc: add a run-links note to docs/architecture/08-config-credentials.md about the transient "starting" state for links shared before the run row. Co-Authored-By: Claude Opus 4.8 * fix(web): re-render work-item runs page at the pending grace boundary The work-item runs page derived `isPending` from elapsed time during render but had no driver to re-render once the grace window elapsed with a still-empty list. React Query structural-shares the same empty-array reference across polls and the component reads only data/isLoading/ isError/error, so no tracked prop changed after the first empty result — leaving the page stuck on "Run is starting…" forever instead of reverting to "No runs found". Schedule a one-shot timer (a `useState` tick) for the moment the grace window elapses while pending, so `isPending` is recomputed to false exactly at the boundary and the table falls back to "No runs found". Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Cascade Bot Co-authored-by: Claude Opus 4.8 --- docs/architecture/08-config-credentials.md | 7 ++ tests/unit/web/work-item-runs-table.test.ts | 85 +++++++++++++++++++ .../components/runs/work-item-runs-table.tsx | 21 ++++- .../work-items/$projectId.$workItemId.tsx | 58 ++++++++++++- 4 files changed, 168 insertions(+), 3 deletions(-) create mode 100644 tests/unit/web/work-item-runs-table.test.ts diff --git a/docs/architecture/08-config-credentials.md b/docs/architecture/08-config-credentials.md index b48d0d10..66294160 100644 --- a/docs/architecture/08-config-credentials.md +++ b/docs/architecture/08-config-credentials.md @@ -56,6 +56,13 @@ interface ProjectConfig { } ``` +**Run links.** When `runLinksEnabled` is `true`, agent comments carry a subtle dashboard +footer linking back to the run — `/runs/` once a run row exists, or the work-item runs +page `/work-items//` posted at ack time before the worker has committed +the run row. Because such a link can be opened before the run row exists, the work-item runs +page renders a transient "Run is starting…" state and keeps polling through a bounded grace +window rather than flashing a terminal "No runs found". + ### Agent update channel `src/config/updateChannel.ts` diff --git a/tests/unit/web/work-item-runs-table.test.ts b/tests/unit/web/work-item-runs-table.test.ts new file mode 100644 index 00000000..784154d9 --- /dev/null +++ b/tests/unit/web/work-item-runs-table.test.ts @@ -0,0 +1,85 @@ +import { createElement } from 'react'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { describe, expect, it } from 'vitest'; + +import { WorkItemRunsTable } from '../../../web/src/components/runs/work-item-runs-table.js'; + +/** + * Tests for the WorkItemRunsTable empty-branch pending state (MNG-1680). + * + * Work-item / PR runs links are posted at ack time — before the worker commits + * the run row — so an empty list within the grace window means "the run is + * starting", not "no runs". When `isPending` is true the empty branch renders + * the shared "Run is starting…" placeholder instead of the terminal + * "No runs found" copy. + * + * The web test suite runs in a node environment with no jsdom, so the component + * is rendered to a static HTML string via `react-dom/server` and the output is + * asserted — mirroring the style of tests/unit/web/run-pending-state.test.ts. + * Only the non-table branches (loading / error / empty) are rendered; the table + * rows pull in `@tanstack/react-router`'s `Link`, which needs a router context. + */ + +const EMPTY = { runs: [], isLoading: false, isError: false } as const; + +describe('WorkItemRunsTable — empty branch', () => { + it('renders the "Run is starting…" pending state when isPending is true', () => { + const html = renderToStaticMarkup( + createElement(WorkItemRunsTable, { ...EMPTY, isPending: true }), + ); + expect(html).toContain('Run is starting'); + expect(html).toContain('animate-spin'); + // The work-item-tailored subtext is shown (apostrophe is HTML-escaped, so + // assert on a substring without it). + expect(html).toContain('will appear here automatically once the worker starts'); + // The terminal copy must not leak through while pending. + expect(html).not.toContain('No runs found'); + }); + + it('renders "No runs found" when isPending is false', () => { + const html = renderToStaticMarkup( + createElement(WorkItemRunsTable, { ...EMPTY, isPending: false }), + ); + expect(html).toContain('No runs found'); + expect(html).not.toContain('Run is starting'); + }); + + it('defaults to "No runs found" when isPending is omitted (PR runs page path)', () => { + // The PR runs page reuses this table without passing isPending, so the + // historical terminal copy must remain the default. + const html = renderToStaticMarkup(createElement(WorkItemRunsTable, EMPTY)); + expect(html).toContain('No runs found'); + expect(html).not.toContain('Run is starting'); + }); +}); + +describe('WorkItemRunsTable — loading / error take precedence over pending', () => { + it('shows the loading copy even when isPending is true', () => { + const html = renderToStaticMarkup( + createElement(WorkItemRunsTable, { + runs: undefined, + isLoading: true, + isError: false, + isPending: true, + }), + ); + expect(html).toContain('Loading runs'); + expect(html).not.toContain('Run is starting'); + expect(html).not.toContain('No runs found'); + }); + + it('shows the error copy even when isPending is true', () => { + const html = renderToStaticMarkup( + createElement(WorkItemRunsTable, { + runs: undefined, + isLoading: false, + isError: true, + error: { message: 'boom' }, + isPending: true, + }), + ); + expect(html).toContain('Failed to load runs'); + expect(html).toContain('boom'); + expect(html).not.toContain('Run is starting'); + }); +}); diff --git a/web/src/components/runs/work-item-runs-table.tsx b/web/src/components/runs/work-item-runs-table.tsx index 4e6fc182..c64c543c 100644 --- a/web/src/components/runs/work-item-runs-table.tsx +++ b/web/src/components/runs/work-item-runs-table.tsx @@ -11,6 +11,7 @@ import { formatCost, formatRelativeTime } from '@/lib/utils.js'; import { CancelRunButton } from './cancel-run-button.js'; import { LiveDuration } from './live-duration.js'; import { RetryRunButton } from './retry-run-button.js'; +import { RunPendingState } from './run-pending-state.js'; import { RunStatusBadge } from './run-status-badge.js'; interface WorkItemRun { @@ -30,9 +31,22 @@ interface WorkItemRunsTableProps { isLoading: boolean; isError: boolean; error?: { message: string } | null; + /** + * When true, an empty result renders the shared "Run is starting…" pending + * placeholder instead of the terminal "No runs found" copy. Work-item / PR + * runs links are posted at ack time — before the worker commits the run row — + * so an empty list within the grace window means "starting", not "none". + */ + isPending?: boolean; } -export function WorkItemRunsTable({ runs, isLoading, isError, error }: WorkItemRunsTableProps) { +export function WorkItemRunsTable({ + runs, + isLoading, + isError, + error, + isPending, +}: WorkItemRunsTableProps) { if (isLoading) { return
Loading runs...
; } @@ -46,6 +60,11 @@ export function WorkItemRunsTable({ runs, isLoading, isError, error }: WorkItemR } if (!runs || runs.length === 0) { + if (isPending) { + return ( + + ); + } return
No runs found
; } diff --git a/web/src/routes/work-items/$projectId.$workItemId.tsx b/web/src/routes/work-items/$projectId.$workItemId.tsx index 9b1d487f..91d49bc7 100644 --- a/web/src/routes/work-items/$projectId.$workItemId.tsx +++ b/web/src/routes/work-items/$projectId.$workItemId.tsx @@ -1,20 +1,45 @@ import { useQuery } from '@tanstack/react-query'; import { createRoute } from '@tanstack/react-router'; import { ExternalLink } from 'lucide-react'; +import { useEffect, useRef, useState } from 'react'; import { WorkItemCostChart } from '@/components/runs/work-item-cost-chart.js'; import { WorkItemDurationChart } from '@/components/runs/work-item-duration-chart.js'; import { WorkItemRunsTable } from '@/components/runs/work-item-runs-table.js'; +import { + RUN_PENDING_GRACE_MS, + resolveWorkItemRunsView, + workItemRunsRefetchInterval, +} from '@/lib/run-pending.js'; import { trpc } from '@/lib/trpc.js'; import { rootRoute } from '../__root.js'; function WorkItemRunsPage() { const { projectId, workItemId } = workItemRunsRoute.useParams(); + // A work-item runs link is posted at ack time — before the worker commits the + // run row — so the first fetches can return an empty list. Track when the page + // mounted so we can keep polling through (and show a "starting" state during) + // the bounded grace window instead of flashing a terminal "No runs found". + const mountedAt = useRef(Date.now()); + + // `isPending` below is derived from elapsed time *during render*, so something + // must force a re-render when the grace window crosses while the list stays + // empty. In the genuinely-empty case React Query structural-shares the same + // empty-array reference across polls, so no tracked query prop changes after + // the first empty result and polling stops right at the boundary — without + // this tick the page would be stuck on "Run is starting…" forever instead of + // falling back to "No runs found". The grace-boundary effect below bumps it + // exactly once, when the window elapses. + const [, forceTick] = useState(0); + const runsQuery = useQuery({ ...trpc.workItems.runs.queryOptions({ projectId, workItemId }), refetchInterval: (query) => { - const hasRunning = query.state.data?.some((r) => r.status === 'running'); - return hasRunning ? 5000 : false; + const data = query.state.data; + const hasRunning = data?.some((r) => r.status === 'running') ?? false; + const isEmpty = (data?.length ?? 0) === 0; + const elapsedMs = Date.now() - mountedAt.current; + return workItemRunsRefetchInterval({ hasRunning, isEmpty, elapsedMs }); }, }); @@ -23,6 +48,34 @@ function WorkItemRunsPage() { const workItemTitle = firstRun?.workItemTitle ?? workItemId; const workItemUrl = firstRun?.workItemUrl; + // Within the grace window an empty list means "the worker is still starting", + // not "no runs" — render the shared pending placeholder in that case. Past the + // grace window `resolveWorkItemRunsView` returns `empty`, so the table falls + // back to "No runs found" (that post-grace contract is pinned by + // `resolveWorkItemRunsView` in tests/unit/web/run-pending.test.ts). + const isPending = + resolveWorkItemRunsView({ + isLoading: runsQuery.isLoading, + isError: runsQuery.isError, + isEmpty: (runs?.length ?? 0) === 0, + elapsedMs: Date.now() - mountedAt.current, + }) === 'pending'; + + // While pending, schedule a single re-render for the moment the grace window + // elapses. That recomputes `isPending` (→ false) so a list that never + // materializes a run reverts from "Run is starting…" to "No runs found" + // instead of spinning forever. The small buffer guards against the timer + // firing a hair early (which would leave `elapsedMs` just shy of the boundary + // and keep the view `pending`). + useEffect(() => { + if (!isPending) { + return; + } + const remainingMs = RUN_PENDING_GRACE_MS - (Date.now() - mountedAt.current); + const timer = setTimeout(() => forceTick((tick) => tick + 1), Math.max(0, remainingMs) + 50); + return () => clearTimeout(timer); + }, [isPending]); + return (

Work Item Runs

@@ -57,6 +110,7 @@ function WorkItemRunsPage() { isLoading={runsQuery.isLoading} isError={runsQuery.isError} error={runsQuery.error} + isPending={isPending} />
); From 48bdb26329b086efcc22b0a13392887ed714fe47 Mon Sep 17 00:00:00 2001 From: aaight Date: Thu, 25 Jun 2026 19:41:49 +0200 Subject: [PATCH 3/8] fix(api): make debug-analysis status durable and cross-process (#1453) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(api): make debug-analysis status durable and cross-process In production the debug-analysis job runs in a separate worker process, so the dashboard API's in-memory `isAnalysisRunning()` Set never reports `running`. Derive running/queued/failed status from the durable BullMQ dashboard-jobs queue (keyed by the deterministic `debug-analysis-` id) when REDIS_URL is set, and keep the in-memory + DB path for local dev. - getDebugAnalysisStatus: queue-mode precedence (in-flight job -> running short-circuiting the DB; DB record -> completed; failed job -> failed; else idle). Status union gains an additive `failed` literal. - triggerDebugAnalysis: queue-mode already-running guard throws CONFLICT on an in-flight job; re-run removes any prior terminal job then re-enqueues with the deterministic id so it is idempotent and reusable. - Read the REDIS_URL gate at call time (isQueueMode()) instead of a module-load const so it reflects the live env and is unit-testable. Co-Authored-By: Claude Opus 4.8 * fix(api): make debug-analysis status reflect the analysis lifecycle, not spawn The durable signal the previous approach relied on (the cascade-dashboard-jobs BullMQ job) reaches `completed` at container *spawn*, not at analysis completion: guardedSpawn resolves once `container.start()` returns, while the debug agent runs for tens of seconds to minutes inside the spawned container and the `debug_analyses` row is written only at the very end. So `running` was observable only during the brief spawn window — during the actual analysis, status read `idle` (frontend stopped polling and showed the empty "Run Analysis" state) and the double-press guard lapsed (a second trigger re-enqueued a duplicate: 2× LLM cost + duplicate PM comment). Replace the job-state signal with a worker-owned, durable, cross-process lifecycle signal that models the analysis itself: - New `debug_analysis_status` table (PK on analyzed_run_id, ON DELETE CASCADE), migration 0056. The worker marks `running` around `triggerDebugAnalysis`, clears it on success (a present `debug_analyses` row is then `completed`), and marks `failed` on error. The dashboard also marks `running` at trigger time to cover the enqueue→spawn window. - `getDebugAnalysisStatus` and the re-trigger guard now read this row uniformly in queue and local-dev mode — no more BullMQ job-state reads. A stale `running` row (crashed worker, > DEBUG_ANALYSIS_RUNNING_STALE_MS = 2h, above the 30 min worker timeout) is ignored so a crash never wedges the run permanently. - The deterministic job id + remove-before-submit re-enqueue is kept so a near-simultaneous second trigger that slips past the guard cannot spawn a duplicate container. `running` is marked only after a successful enqueue so a failed enqueue cannot leave a phantom marker blocking retries. - Remove the worker-local in-memory tracker (`debug-status.ts`) — never visible to the dashboard process — and the now-unused `getDashboardJobState` helper. Co-Authored-By: Claude Opus 4.8 * fix(cli): surface terminal failed status in debug-analysis --wait loop The `cascade runs debug --analyze --wait` poll loop only returned on `completed` and `idle`. After the durable-status PR added a real `failed` status, a failed analysis was no longer surfaced as `idle`, so the loop ignored it and polled for the full 5-minute deadline before printing a misleading "Timed out waiting for debug analysis to complete." message. Add a `failed` branch mirroring the existing `idle` branch so a failed analysis returns promptly with an accurate "Debug analysis failed." message. Also corrects the imprecise comment in runs.ts: the leftover terminal status row is overwritten by the markDebugAnalysisRunning upsert, not deleted by deleteDebugAnalysisByRunId. Co-Authored-By: Claude Opus 4.8 * test(debug): cover durable failed-write error path in debug analysis runner Pins the defensive invariant in triggerDebugAnalysis that a failing markDebugAnalysisFailed write logs a warning but does not mask or replace the original agent error. This is the catch-handler branch that codecov flagged as the uncovered patch lines in src/triggers/shared/debug-runner.ts. Test-only; no production code change. Co-Authored-By: Claude Opus 4.8 * docs(api): correct debug-analysis guard comment to reflect uniform durable read The already-running guard comment claimed it was 'durable in queue mode, in-memory in local dev' — leftover narrative from the abandoned approach. The shipped guard (assertDebugAnalysisNotInFlight) reads the durable debug_analysis_status row uniformly in both queue and local-dev mode; the in-memory isAnalysisRunning mechanism was deleted. Aligns the comment with the function's own docstring and the rest of the durable-table design. Co-Authored-By: Claude Opus 4.8 * test(db): exercise debug_analysis_status ON DELETE CASCADE with a targeted delete The ON DELETE CASCADE test used truncateAll(), which issues `TRUNCATE ... CASCADE` and clears every referencing table regardless of the FK action — so it would pass even if the constraint were NO ACTION/RESTRICT, never actually exercising the clause it names. Delete only the parent agent_runs row instead: the DELETE now succeeds and removes the status row solely because the FK cascades; a non-cascading FK would raise a foreign-key violation. Also assert the status row exists before the delete to make the cascade observable. Co-Authored-By: Claude Opus 4.8 * docs(debug): document failed-status coverage caveat for hard kills The `failed` debug-analysis status is written only from the in-process catch in triggerDebugAnalysis, so it covers catchable in-process errors only. A hard kill (watchdog/OOM) or a throw before the runner is reached (e.g. processDashboardJob failing to load the project config after the dashboard already marked running) leaves a `running` row that self-stales to `idle` rather than surfacing `failed`. Documents this deliberate tradeoff in-code per repeated review feedback (non-blocking). Co-Authored-By: Claude Opus 4.8 * docs(debug): note getRunById-null early-return in failed-coverage caveat The latest review observed that the runner's own early getRunById-null return is another path where the dashboard-written 'running' marker is never cleared or flipped to 'failed' (it self-stales to 'idle' after DEBUG_ANALYSIS_RUNNING_STALE_MS). That early return is a bare return before the try block, so the existing catch-block caveat did not cover it. Document the gap at the early-return site and extend the catch-block caveat to list it. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 * docs(db): document the durable debug_analysis_status table Records the shipped cross-process debug-analysis status design (new table from MNG-1667) in the database architecture doc: ER relationship, key-tables row, and repository purpose. Captures the durable-table approach that actually shipped, not the abandoned queue-derived design the reviewer flagged, so the permanent in-repo record is accurate. Co-Authored-By: Claude Opus 4.8 * docs(db): note failed-status coverage caveat on debug_analysis_status schema Surface the reviewer's persistently-flagged caveat in the canonical table definition: `failed` is written only for catchable in-process errors (the debug-runner's `catch`), so a hard kill (watchdog/OOM) or a throw before the runner is reached self-stales the `running` row to `idle` rather than surfacing `failed`. Also names DEBUG_ANALYSIS_RUNNING_STALE_MS and points at isDebugAnalysisRunActive so the staleness threshold is discoverable from the schema. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 * test(db): pin exact staleness boundary for isDebugAnalysisRunActive Lock the strict `<` against DEBUG_ANALYSIS_RUNNING_STALE_MS so a future refactor to `<=` (or back) is caught. Existing tests covered only a fresh (~0ms) and a clearly-stale (threshold + 1s) row; this adds the boundary itself — exactly at the threshold (stale) and one ms under (active) — under a frozen clock with real-timer cleanup. That boundary is what guarantees a crashed/OOM-killed worker never wedges a debug analysis as permanently `running`, which the durable debug_analysis_status design relies on. Co-Authored-By: Claude Opus 4.8 * fix(worker): mark debug analysis failed on pre-runner config-load failure When `processDashboardJob` cannot load the project config for a `debug-analysis` job, it throws before `triggerDebugAnalysis` (whose `catch` writes the durable `failed` status) is ever reached. The dashboard has already marked the run `running` at trigger time, so the `running` row would otherwise linger and self-stale to `idle` after `DEBUG_ANALYSIS_RUNNING_STALE_MS` rather than surfacing `failed` — blocking re-trigger with CONFLICT for that ~2h window. Mark the run `failed` (best-effort, so a status-write error never masks the original project-not-found failure) at that site, extending `failed` coverage to this catchable in-process worker error. Hard kills (watchdog/OOM) remain the documented deliberate follow-up (router-side reconciliation on non-zero container exit). Keeps the in-code coverage caveats accurate (debug-runner `catch` block and the `debug_analysis_status` schema docstring) and adds worker-entry tests asserting the run is marked failed and that the original error still propagates when the failed-write itself fails. Co-Authored-By: Claude Opus 4.8 * docs(db): document debug-analysis status precedence and failed caveat Record the shipped durable-table behavior in the merged architecture doc: the getDebugAnalysisStatus precedence (active running → completed (DB-wins) → failed → idle, uniform in queue + local dev) and the failed-coverage caveat (catchable in-process errors only; hard-kill/OOM self-stales the running row to idle, with router-side reconciliation the deliberate follow-up). Documentation-only; no behavior change. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Cascade Bot Co-authored-by: Claude Opus 4.8 Co-authored-by: cascade-bot --- docs/architecture/09-database.md | 4 +- src/api/routers/runs.ts | 109 ++++++-- src/cli/dashboard/runs/debug.ts | 8 + .../migrations/0056_debug_analysis_status.sql | 15 ++ src/db/migrations/meta/_journal.json | 7 + .../repositories/debugAnalysisRepository.ts | 83 ++++++- src/db/repositories/runsRepository.ts | 8 +- src/db/schema/index.ts | 8 +- src/db/schema/runs.ts | 31 +++ src/queue/client.ts | 35 +-- src/triggers/shared/debug-runner.ts | 42 +++- src/triggers/shared/debug-status.ts | 13 - src/worker-entry.ts | 17 +- tests/integration/db/runsRepository.test.ts | 71 ++++++ tests/unit/api/access-control.test.ts | 4 - tests/unit/api/router.test.ts | 4 - tests/unit/api/routers/runs.test.ts | 233 ++++++++++++++++-- tests/unit/cli/dashboard/runs/runs.test.ts | 16 ++ .../debugAnalysisRepository.test.ts | 126 +++++++++- tests/unit/queue/client.test.ts | 36 --- tests/unit/queue/retry-run-projectId.test.ts | 14 +- tests/unit/triggers/debug-runner.test.ts | 61 +++-- tests/unit/triggers/debug-status.test.ts | 55 ----- tests/unit/worker-entry.test.ts | 29 ++- 24 files changed, 814 insertions(+), 215 deletions(-) create mode 100644 src/db/migrations/0056_debug_analysis_status.sql delete mode 100644 src/triggers/shared/debug-status.ts delete mode 100644 tests/unit/triggers/debug-status.test.ts diff --git a/docs/architecture/09-database.md b/docs/architecture/09-database.md index 2f6a4bf1..0ac62812 100644 --- a/docs/architecture/09-database.md +++ b/docs/architecture/09-database.md @@ -24,6 +24,7 @@ erDiagram agent_runs ||--o| agent_run_logs : "has" agent_runs ||--o{ agent_run_llm_calls : "logs" agent_runs ||--o| debug_analyses : "analyzed by" + agent_runs ||--o| debug_analysis_status : "status" users ||--o{ sessions : "has" users ||--o{ org_memberships : "has" @@ -151,6 +152,7 @@ erDiagram | `org_memberships` | Multi-org membership: links a user to an org with a per-org role, so one account can belong to many orgs. `users.org_id`/`users.role` remain the home org + global role. Read by effective-org resolution + the per-org actor-role helper (spec 021 plan 2); written by the grant mutation (`users.addExistingUserToOrg`) and the membership-mirroring create, and read by membership-based member listing (spec 021 plan 3). The listing returns BOTH the per-org `role` and the global `users.role` so the Settings → Users editor keeps targeting the global role until the plan-4 UI reconciliation. The idempotent home-org backfill runs in migration `0053` and is re-run by `0054` so accounts created via the old `createUser` (and bootstrap superadmins) never vanish from the inner-join listing. | UNIQUE(`user_id`, `org_id`) | | `sessions` | Session tokens for cookie auth (30-day expiry); `active_org_id` (nullable) tracks the org the session is currently acting in for multi-org | — | | `debug_analyses` | AI debug analysis results | — | +| `debug_analysis_status` | Durable, cross-process lifecycle status (`running` / `failed`) for a debug analysis. The analysis runs in a separate worker container, so an in-memory flag is invisible to the dashboard API; the worker (and the dashboard at trigger time) writes this row instead. It is deleted on success — a present `debug_analyses` row is then the `completed` signal — and a `running` row older than `DEBUG_ANALYSIS_RUNNING_STALE_MS` is treated as stale (`idle`) so a crashed worker never wedges the run. Status read precedence (uniform in queue mode + local dev): active `running` → `completed` (a persisted `debug_analyses` row wins over a stale terminal status row) → `failed` → `idle`. `failed` is written only for catchable in-process errors (the runner's `catch`, plus the pre-runner project-config-load failure); a hard kill (watchdog/OOM) leaves the `running` row to self-stale to `idle` rather than surfacing `failed`, with router-side reconciliation on non-zero container exit the deliberate follow-up. | PK on `analyzed_run_id`, FK → `agent_runs` ON DELETE CASCADE | ## Repositories @@ -177,7 +179,7 @@ Each table has a dedicated repository providing typed query methods. Key reposit | `partialsRepository` | Prompt partial CRUD | | `prWorkItemsRepository` | PR ↔ work item mapping | | `webhookLogsRepository` | Webhook audit trail | -| `debugAnalysisRepository` | Debug analysis results | +| `debugAnalysisRepository` | Debug analysis results + durable cross-process analysis lifecycle status (`debug_analysis_status`: mark running/failed, clear on success, read run state, staleness check) | ## Connection Management diff --git a/src/api/routers/runs.ts b/src/api/routers/runs.ts index 59ed77e8..f74c303f 100644 --- a/src/api/routers/runs.ts +++ b/src/api/routers/runs.ts @@ -7,21 +7,53 @@ import { DEFAULT_STALE_RUN_THRESHOLD_MS, deleteDebugAnalysisByRunId, getDebugAnalysisByRunId, + getDebugAnalysisRunState, getLlmCallByNumber, getRunById, getRunLogs, hasActiveRunForWorkItem, + isDebugAnalysisRunActive, listLlmCallsMeta, listRuns, + markDebugAnalysisRunning, } from '../../db/repositories/runsRepository.js'; import { publishCancelCommand } from '../../queue/cancel.js'; -import { isAnalysisRunning } from '../../triggers/shared/debug-status.js'; import { parseLlmResponse } from '../../utils/llmResponseParser.js'; import { logger } from '../../utils/logging.js'; import { protectedProcedure, router, superAdminProcedure } from '../trpc.js'; import { verifyProjectOrgAccess } from './_shared/projectAccess.js'; -const useQueue = !!process.env.REDIS_URL; +/** + * Whether dashboard jobs run through the durable BullMQ queue (production) or + * are fired in-process (local dev without REDIS_URL). + * + * Read at call time — not memoised at module load — so the REDIS_URL gate + * reflects the live environment and is observable in unit tests. + */ +function isQueueMode(): boolean { + return !!process.env.REDIS_URL; +} + +/** + * Throw CONFLICT when a debug analysis is already in progress for `runId`. + * + * Reads the durable `debug_analysis_status` row (the worker-owned, cross-process + * signal of the *analysis* lifecycle). This is authoritative in both queue mode + * (analysis runs in a separate worker container) and local dev (in-process): the + * dashboard BullMQ job reaches `completed` at container *spawn*, not at analysis + * completion, so the queue cannot be used to detect a still-running analysis. A + * stale `running` row (crashed worker) is ignored, so a crash never wedges the + * re-trigger permanently. + */ +async function assertDebugAnalysisNotInFlight(runId: string): Promise { + const state = await getDebugAnalysisRunState(runId); + if (isDebugAnalysisRunActive(state)) { + throw new TRPCError({ + code: 'CONFLICT', + message: 'Debug analysis is already running for this run', + }); + } +} export const runsRouter = router({ list: protectedProcedure @@ -193,13 +225,31 @@ export const runsRouter = router({ if (!ctx.effectiveOrgId) throw new TRPCError({ code: 'UNAUTHORIZED' }); await verifyProjectOrgAccess(run.projectId, ctx.effectiveOrgId); } - if (isAnalysisRunning(input.runId)) { + // Status is derived from the durable `debug_analysis_status` row (the + // worker-owned, cross-process signal of the *analysis* lifecycle) plus + // the persisted `debug_analyses` content row. This is identical in queue + // mode (analysis runs in a separate worker container) and local dev + // (in-process): the dashboard BullMQ job reaches `completed` at container + // *spawn*, not at analysis completion, so it cannot represent a + // still-running analysis. + // + // Precedence: + // 1. An active `running` row wins and short-circuits the content lookup. + // 2. else a persisted analysis means a prior run completed. + // 3. else a `failed` status row means the last attempt errored out. + // 4. else idle. (A stale `running` row — crashed worker — falls through + // here so a crash never wedges the run as permanently `running`.) + const state = await getDebugAnalysisRunState(input.runId); + if (isDebugAnalysisRunActive(state)) { return { status: 'running' as const }; } const analysis = await getDebugAnalysisByRunId(input.runId); if (analysis) { return { status: 'completed' as const }; } + if (state?.status === 'failed') { + return { status: 'failed' as const }; + } return { status: 'idle' as const }; }), @@ -222,12 +272,9 @@ export const runsRouter = router({ }); } - if (isAnalysisRunning(input.runId)) { - throw new TRPCError({ - code: 'CONFLICT', - message: 'Debug analysis is already running for this run', - }); - } + // Already-running guard — reads the durable `debug_analysis_status` row + // uniformly in queue mode and local dev (see assertDebugAnalysisNotInFlight). + await assertDebugAnalysisNotInFlight(input.runId); if (!run.projectId) { throw new TRPCError({ @@ -244,18 +291,40 @@ export const runsRouter = router({ }); } - // Delete existing analysis before re-running + // Delete the prior analysis content row before re-running. Any leftover + // terminal status row (e.g. `failed`) is not removed here — it is + // overwritten by the `markDebugAnalysisRunning` upsert below. await deleteDebugAnalysisByRunId(input.runId); - if (useQueue) { - const { submitDashboardJob } = await import('../../queue/client.js'); - await submitDashboardJob({ - type: 'debug-analysis', - runId: input.runId, - projectId: run.projectId, - workItemId: run.workItemId ?? undefined, - }); + if (isQueueMode()) { + const { debugAnalysisJobId, removeDashboardJob, submitDashboardJob } = await import( + '../../queue/client.js' + ); + // Reuse the deterministic id so a re-run replaces (rather than races) + // any prior job, and so a near-simultaneous second trigger that slips + // past the guard cannot spawn a duplicate container. Clear any prior + // terminal job first so the re-enqueue isn't rejected for a duplicate id. + const jobId = debugAnalysisJobId(input.runId); + await removeDashboardJob(jobId); + await submitDashboardJob( + { + type: 'debug-analysis', + runId: input.runId, + projectId: run.projectId, + workItemId: run.workItemId ?? undefined, + }, + jobId, + ); + // Mark running only after the job is durably enqueued: a failed enqueue + // then leaves no `running` row to block (and self-stale) a retry. The + // worker re-marks running (idempotent) when it starts the analysis; + // this write covers the enqueue→container-spawn window so status reads + // `running` immediately and a second trigger gets CONFLICT. + await markDebugAnalysisRunning(input.runId); } else { + // Local dev: mark running before firing so the guard is effective, then + // run in-process (the runner re-marks running and clears/fails at end). + await markDebugAnalysisRunning(input.runId); const { triggerDebugAnalysis } = await import('../../triggers/shared/debug-runner.js'); triggerDebugAnalysis(input.runId, pc.project, pc.config, run.workItemId ?? undefined).catch( (err) => { @@ -329,7 +398,7 @@ export const runsRouter = router({ }); } - if (useQueue) { + if (isQueueMode()) { const { submitDashboardJob } = await import('../../queue/client.js'); await submitDashboardJob({ type: 'manual-run', @@ -430,7 +499,7 @@ export const runsRouter = router({ }); } - if (useQueue) { + if (isQueueMode()) { const { submitDashboardJob } = await import('../../queue/client.js'); await submitDashboardJob({ type: 'retry-run', diff --git a/src/cli/dashboard/runs/debug.ts b/src/cli/dashboard/runs/debug.ts index 5802e74a..f0bfed39 100644 --- a/src/cli/dashboard/runs/debug.ts +++ b/src/cli/dashboard/runs/debug.ts @@ -83,6 +83,14 @@ export default class RunsDebug extends DashboardCommand { return; } + if (status.status === 'failed') { + // Terminal failure — the analysis errored out. Surface it promptly + // instead of polling to the deadline and printing a misleading + // timeout message. + this.log('Debug analysis failed.'); + return; + } + if (status.status === 'idle') { // Analysis finished but no result — likely failed this.log('Debug analysis finished but no result was stored (analysis may have failed).'); diff --git a/src/db/migrations/0056_debug_analysis_status.sql b/src/db/migrations/0056_debug_analysis_status.sql new file mode 100644 index 00000000..9805c51d --- /dev/null +++ b/src/db/migrations/0056_debug_analysis_status.sql @@ -0,0 +1,15 @@ +-- Durable, cross-process lifecycle status for debug analyses (MNG-1667). +-- +-- The debug_analyses content row is written only at the END of a successful +-- analysis, and the analysis runs inside a separate worker container, so neither +-- that row nor the dashboard BullMQ job (which reaches `completed` at container +-- spawn, not at analysis completion) can represent an in-progress analysis. This +-- table is the worker-owned signal: 'running' while the debug agent executes, +-- 'failed' on error; the row is removed on success, after which a present +-- debug_analyses row is the 'completed' signal. updated_at lets readers treat a +-- 'running' row left behind by a crashed worker as stale. +CREATE TABLE IF NOT EXISTS "debug_analysis_status" ( + "analyzed_run_id" uuid PRIMARY KEY REFERENCES "agent_runs"("id") ON DELETE CASCADE, + "status" text NOT NULL, + "updated_at" timestamp NOT NULL DEFAULT now() +); diff --git a/src/db/migrations/meta/_journal.json b/src/db/migrations/meta/_journal.json index 6659f6d0..40a06a8c 100644 --- a/src/db/migrations/meta/_journal.json +++ b/src/db/migrations/meta/_journal.json @@ -393,6 +393,13 @@ "when": 1790000000000, "tag": "0055_agent_config_update_channel", "breakpoints": false + }, + { + "idx": 56, + "version": "7", + "when": 1791000000000, + "tag": "0056_debug_analysis_status", + "breakpoints": false } ] } diff --git a/src/db/repositories/debugAnalysisRepository.ts b/src/db/repositories/debugAnalysisRepository.ts index b01b0d62..0eee2699 100644 --- a/src/db/repositories/debugAnalysisRepository.ts +++ b/src/db/repositories/debugAnalysisRepository.ts @@ -1,6 +1,6 @@ import { eq } from 'drizzle-orm'; import { getDb } from '../client.js'; -import { debugAnalyses } from '../schema/index.js'; +import { debugAnalyses, debugAnalysisStatus } from '../schema/index.js'; // ============================================================================ // Types @@ -17,6 +17,11 @@ export interface CreateDebugAnalysisInput { severity?: string; } +export interface DebugAnalysisRunState { + status: string; + updatedAt: Date | null; +} + // ============================================================================ // Debug Analysis // ============================================================================ @@ -61,3 +66,79 @@ export async function getDebugAnalysisByDebugRunId(debugRunId: string) { .where(eq(debugAnalyses.debugRunId, debugRunId)); return row ?? null; } + +// ============================================================================ +// Debug Analysis lifecycle status (durable, cross-process) +// ============================================================================ +// +// The analysis runs inside a separate worker container and the debug_analyses +// content row is written only at the end, so this table — written by the worker +// around the analysis (and by the dashboard at trigger time) — is the source of +// truth for the in-progress / failed lifecycle. A present debug_analyses row is +// the `completed` signal; this status row covers `running` and `failed`. + +/** + * A `running` status row older than this is treated as stale — a worker that + * crashed (OOM/kill) without clearing it — so it no longer reports `running` or + * blocks a re-trigger. Set comfortably above the global worker timeout + * (`WORKER_TIMEOUT_MS`, default 30 min) so a legitimately long debug analysis is + * never misread as dead; mirrors the 2h `DEFAULT_STALE_RUN_THRESHOLD_MS` the + * runs repository uses for active-run staleness. + */ +export const DEBUG_ANALYSIS_RUNNING_STALE_MS = 2 * 60 * 60 * 1000; + +/** + * Whether a status row represents an analysis that is genuinely still running + * (status `running` and not stale). A `null` row, a terminal status (`failed`), + * or a stale `running` row all return `false`. + */ +export function isDebugAnalysisRunActive(state: DebugAnalysisRunState | null): boolean { + if (!state || state.status !== 'running') return false; + if (!state.updatedAt) return true; + return Date.now() - state.updatedAt.getTime() < DEBUG_ANALYSIS_RUNNING_STALE_MS; +} + +/** Mark a debug analysis as running (idempotent upsert keyed by analyzed run). */ +export async function markDebugAnalysisRunning(analyzedRunId: string): Promise { + const db = getDb(); + await db + .insert(debugAnalysisStatus) + .values({ analyzedRunId, status: 'running', updatedAt: new Date() }) + .onConflictDoUpdate({ + target: debugAnalysisStatus.analyzedRunId, + set: { status: 'running', updatedAt: new Date() }, + }); +} + +/** Mark a debug analysis as failed (idempotent upsert keyed by analyzed run). */ +export async function markDebugAnalysisFailed(analyzedRunId: string): Promise { + const db = getDb(); + await db + .insert(debugAnalysisStatus) + .values({ analyzedRunId, status: 'failed', updatedAt: new Date() }) + .onConflictDoUpdate({ + target: debugAnalysisStatus.analyzedRunId, + set: { status: 'failed', updatedAt: new Date() }, + }); +} + +/** + * Clear a debug analysis status row. Called on successful completion — the + * present `debug_analyses` content row is then the `completed` signal. + */ +export async function clearDebugAnalysisStatus(analyzedRunId: string): Promise { + const db = getDb(); + await db.delete(debugAnalysisStatus).where(eq(debugAnalysisStatus.analyzedRunId, analyzedRunId)); +} + +/** Read the lifecycle status row for a debug analysis, or null when none exists. */ +export async function getDebugAnalysisRunState( + analyzedRunId: string, +): Promise { + const db = getDb(); + const [row] = await db + .select({ status: debugAnalysisStatus.status, updatedAt: debugAnalysisStatus.updatedAt }) + .from(debugAnalysisStatus) + .where(eq(debugAnalysisStatus.analyzedRunId, analyzedRunId)); + return row ?? null; +} diff --git a/src/db/repositories/runsRepository.ts b/src/db/repositories/runsRepository.ts index 9221f7db..17f74467 100644 --- a/src/db/repositories/runsRepository.ts +++ b/src/db/repositories/runsRepository.ts @@ -320,11 +320,17 @@ export async function cancelRunById(runId: string, reason: string): Promise [index('idx_debug_analyses_analyzed_run_id').on(table.analyzedRunId)], ); + +/** + * Durable, cross-process lifecycle status of a debug analysis, keyed by the + * analyzed run. + * + * The `debug_analyses` content row is written only at the END of a successful + * analysis, and the analysis itself runs inside a separate worker container — + * the dashboard BullMQ job reaches `completed` at container *spawn*, not at + * analysis completion. Neither of those signals can therefore represent an + * in-progress analysis. This table is the worker-owned signal: the worker (and + * the dashboard at trigger time) writes `running` while the debug agent is + * executing and `failed` if it errors; the row is deleted on success, after + * which a present `debug_analyses` row is the `completed` signal. `updated_at` + * lets readers treat a `running` row left behind by a crashed worker as stale + * (older than `DEBUG_ANALYSIS_RUNNING_STALE_MS` → read as `idle`; see + * `isDebugAnalysisRunActive` in `debugAnalysisRepository`). + * + * Coverage caveat: `failed` is written for catchable in-process errors — the + * debug-runner's `catch` plus the worker's pre-runner project-config-load failure + * in `processDashboardJob`. A hard kill (watchdog timeout / OOM) still leaves the + * `running` row to self-stale to `idle` rather than surfacing `failed`; + * router-side reconciliation on non-zero container exit is a deliberate + * follow-up. + */ +export const debugAnalysisStatus = pgTable('debug_analysis_status', { + analyzedRunId: uuid('analyzed_run_id') + .primaryKey() + .references(() => agentRuns.id, { onDelete: 'cascade' }), + status: text('status').notNull(), + updatedAt: timestamp('updated_at').notNull().defaultNow(), +}); diff --git a/src/queue/client.ts b/src/queue/client.ts index a3567f11..b2e142bb 100644 --- a/src/queue/client.ts +++ b/src/queue/client.ts @@ -5,7 +5,7 @@ * Only loaded when REDIS_URL is set (production dashboard container). */ -import { type JobState, Queue } from 'bullmq'; +import { Queue } from 'bullmq'; import { parseRedisUrl } from '../utils/redis.js'; // ── Job types ──────────────────────────────────────────────────────────────── @@ -80,41 +80,26 @@ export async function submitDashboardJob(job: DashboardJob, jobId?: string): Pro return result.id ?? id; } -// ── Debug-analysis job-state helpers ─────────────────────────────────────────── +// ── Debug-analysis re-enqueue helpers ────────────────────────────────────────── // -// These let the dashboard process (where the API runs) observe running/queued/ -// failed state for a debug-analysis job cross-process, using the BullMQ queue as -// the source of truth rather than the worker-local in-memory Set in -// `src/triggers/shared/debug-status.ts`. Pairing a deterministic job id (one job -// per analyzed run) with state read + idempotent removal lets a re-run reuse the -// same id without a stale completed/failed job blocking the add. +// Pairing a deterministic job id (one job per analyzed run) with idempotent +// removal lets a re-run reuse the same id without a stale completed/failed job +// blocking the add, and stops a near-simultaneous second trigger from spawning a +// duplicate container. The *analysis* lifecycle (running/failed) is tracked in +// the durable `debug_analysis_status` table by the worker — not via BullMQ job +// state, which reaches `completed` at container spawn rather than at analysis +// completion. /** * Deterministic BullMQ job id for the debug-analysis job of a given run. * * One job per analyzed run: passing this id to {@link submitDashboardJob} makes - * the queue self-deduplicating for `debug-analysis` and lets other processes - * resolve the job's state by run id alone. + * the queue self-deduplicating for `debug-analysis`. */ export function debugAnalysisJobId(runId: string): string { return `debug-analysis-${runId}`; } -/** - * Resolve the BullMQ state of a dashboard job by id. - * - * Returns the job's state (`waiting | active | delayed | prioritized | - * waiting-children | completed | failed | unknown`) or `null` when no job with - * that id exists in the queue. - */ -export async function getDashboardJobState(jobId: string): Promise { - const job = await getQueue().getJob(jobId); - if (!job) { - return null; - } - return job.getState(); -} - /** * Remove a dashboard job by id. * diff --git a/src/triggers/shared/debug-runner.ts b/src/triggers/shared/debug-runner.ts index aa999146..fe140a0b 100644 --- a/src/triggers/shared/debug-runner.ts +++ b/src/triggers/shared/debug-runner.ts @@ -3,16 +3,18 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { runAgent } from '../../agents/registry.js'; import { + clearDebugAnalysisStatus, getLlmCallsByRunId, getRunById, getRunLogs, + markDebugAnalysisFailed, + markDebugAnalysisRunning, storeDebugAnalysis, } from '../../db/repositories/runsRepository.js'; import { getPMProvider } from '../../pm/index.js'; import type { AgentResult, CascadeConfig, ProjectConfig } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; import { cleanupTempDir } from '../../utils/repo.js'; -import { markAnalysisComplete, markAnalysisRunning } from './debug-status.js'; /** * Extract logs from the database and write them to a temp directory @@ -142,6 +144,12 @@ export async function triggerDebugAnalysis( ): Promise { const run = await getRunById(analyzedRunId); if (!run) { + // `failed`-coverage gap (same class as the catch block below): the dashboard + // already wrote a `running` marker at trigger time, and this bare `return` + // neither clears it nor marks `failed`, so the row lingers and self-stales to + // `idle` after `DEBUG_ANALYSIS_RUNNING_STALE_MS` rather than surfacing + // `failed`. A missing run is a non-retryable terminal condition (the analyzed + // run was deleted), so the bounded re-trigger CONFLICT window is acceptable. logger.warn('Run not found for debug analysis', { analyzedRunId }); return; } @@ -152,7 +160,11 @@ export async function triggerDebugAnalysis( workItemId, }); - markAnalysisRunning(analyzedRunId); + // Durable, cross-process `running` marker. The analysis runs in this worker + // container while the dashboard API (a separate process) polls status, so an + // in-memory flag is never visible to it. Idempotent: the dashboard may have + // already marked running at trigger time. + await markDebugAnalysisRunning(analyzedRunId); let logDir: string | undefined; try { logDir = await extractLogsToTempDir(analyzedRunId); @@ -185,13 +197,37 @@ export async function triggerDebugAnalysis( await postDebugComment(workItemId, analyzedRunId, parsed); } + // Success: clear the lifecycle marker. The persisted debug_analyses row is + // now the `completed` signal. + await clearDebugAnalysisStatus(analyzedRunId); + logger.info('Debug analysis completed', { analyzedRunId, debugRunId: agentResult.runId, success: agentResult.success, }); + } catch (err) { + // Failure: persist `failed` so status reflects the failed analysis (not + // `idle`) and surfaces the re-run affordance. Don't let a status-write + // error mask the original failure. + // + // Coverage caveat: this path only fires for catchable in-process errors. A + // hard kill (watchdog timeout / OOM), or this runner's own early + // `getRunById`-null return above, skip it; the lingering `running` row then + // self-stales to `idle` after `DEBUG_ANALYSIS_RUNNING_STALE_MS` rather than + // surfacing `failed`. (A worker-side throw *before* this runner is reached — + // e.g. `processDashboardJob` failing to load the project config after the + // dashboard already marked running — now marks `failed` at that site.) + // Surfacing `failed` on hard kill (e.g. router-side reconciliation on + // non-zero container exit) is a deliberate follow-up. + await markDebugAnalysisFailed(analyzedRunId).catch((statusErr) => { + logger.warn('Failed to mark debug analysis failed', { + analyzedRunId, + error: String(statusErr), + }); + }); + throw err; } finally { - markAnalysisComplete(analyzedRunId); if (logDir) { try { cleanupTempDir(logDir); diff --git a/src/triggers/shared/debug-status.ts b/src/triggers/shared/debug-status.ts deleted file mode 100644 index 75cb0463..00000000 --- a/src/triggers/shared/debug-status.ts +++ /dev/null @@ -1,13 +0,0 @@ -const runningAnalyses = new Set(); - -export function markAnalysisRunning(runId: string): void { - runningAnalyses.add(runId); -} - -export function markAnalysisComplete(runId: string): void { - runningAnalyses.delete(runId); -} - -export function isAnalysisRunning(runId: string): boolean { - return runningAnalyses.has(runId); -} diff --git a/src/worker-entry.ts b/src/worker-entry.ts index 0ddba454..c7850178 100644 --- a/src/worker-entry.ts +++ b/src/worker-entry.ts @@ -229,7 +229,22 @@ export async function processDashboardJob(jobId: string, jobData: DashboardJobDa logger.info('[Worker] Processing debug-analysis job', { jobId, runId: jobData.runId }); const { triggerDebugAnalysis } = await import('./triggers/shared/debug-runner.js'); const pc = await loadProjectConfigById(jobData.projectId); - if (!pc) throw new Error(`Project not found: ${jobData.projectId}`); + if (!pc) { + // The dashboard marked this run `running` at trigger time, and this throw + // happens *before* `triggerDebugAnalysis` (whose `catch` would otherwise + // flip the status to `failed`) is reached. Surface the failure here so the + // status reads `failed` instead of leaving the `running` row to self-stale + // to `idle` after `DEBUG_ANALYSIS_RUNNING_STALE_MS`. Best-effort: a + // status-write error must not mask the original project-not-found failure. + const { markDebugAnalysisFailed } = await import('./db/repositories/runsRepository.js'); + await markDebugAnalysisFailed(jobData.runId).catch((statusErr) => { + logger.warn('Failed to mark debug analysis failed', { + runId: jobData.runId, + error: String(statusErr), + }); + }); + throw new Error(`Project not found: ${jobData.projectId}`); + } await triggerDebugAnalysis(jobData.runId, pc.project, pc.config, jobData.workItemId); } } diff --git a/tests/integration/db/runsRepository.test.ts b/tests/integration/db/runsRepository.test.ts index f438aca5..eb62e52f 100644 --- a/tests/integration/db/runsRepository.test.ts +++ b/tests/integration/db/runsRepository.test.ts @@ -4,10 +4,12 @@ import { linkPRToWorkItem, } from '../../../src/db/repositories/prWorkItemsRepository.js'; import { + clearDebugAnalysisStatus, completeRun, createRun, deleteDebugAnalysisByRunId, getDebugAnalysisByRunId, + getDebugAnalysisRunState, getLlmCallByNumber, getLlmCallsByRunId, getRunById, @@ -16,9 +18,12 @@ import { getRunsByWorkItem, getRunsByWorkItemId, getRunsForPR, + isDebugAnalysisRunActive, listLlmCallsMeta, listProjectsForOrg, listRuns, + markDebugAnalysisFailed, + markDebugAnalysisRunning, storeDebugAnalysis, storeLlmCall, storeLlmCallsBulk, @@ -429,6 +434,72 @@ describe('runsRepository (integration)', () => { }); }); + // ========================================================================= + // Debug Analysis lifecycle status (durable, cross-process) + // ========================================================================= + + describe('debug analysis status lifecycle', () => { + it('marks running, reads an active state, then clears on success', async () => { + const runId = await createRun({ + projectId: 'test-project', + agentType: 'implementation', + engine: 'claude-code', + }); + + // No row → not active. + expect(await getDebugAnalysisRunState(runId)).toBeNull(); + + await markDebugAnalysisRunning(runId); + const running = await getDebugAnalysisRunState(runId); + expect(running?.status).toBe('running'); + expect(isDebugAnalysisRunActive(running)).toBe(true); + + // Success clears the lifecycle row. + await clearDebugAnalysisStatus(runId); + expect(await getDebugAnalysisRunState(runId)).toBeNull(); + }); + + it('upserts the same analyzed run (one row per run) and records failure', async () => { + const runId = await createRun({ + projectId: 'test-project', + agentType: 'implementation', + engine: 'claude-code', + }); + + await markDebugAnalysisRunning(runId); + // Idempotent upsert keyed by analyzed run — a second mark must not create + // a duplicate row (PK on analyzed_run_id). + await markDebugAnalysisFailed(runId); + + const state = await getDebugAnalysisRunState(runId); + expect(state?.status).toBe('failed'); + expect(isDebugAnalysisRunActive(state)).toBe(false); + }); + + it('is removed when the analyzed run is deleted (ON DELETE CASCADE)', async () => { + const runId = await createRun({ + projectId: 'test-project', + agentType: 'implementation', + engine: 'claude-code', + }); + await markDebugAnalysisRunning(runId); + expect(await getDebugAnalysisRunState(runId)).not.toBeNull(); + + // Delete the parent agent_runs row directly so this genuinely exercises + // the FK's ON DELETE CASCADE. truncateAll() issues `TRUNCATE ... CASCADE`, + // which clears every referencing table regardless of the FK action — it + // would pass even if the constraint were NO ACTION/RESTRICT. A targeted + // DELETE only succeeds (and removes the status row) because the FK + // cascades; a non-cascading FK would raise a foreign-key violation here. + const { getDb } = await import('../../../src/db/client.js'); + const { agentRuns } = await import('../../../src/db/schema/index.js'); + const { eq } = await import('drizzle-orm'); + await getDb().delete(agentRuns).where(eq(agentRuns.id, runId)); + + expect(await getDebugAnalysisRunState(runId)).toBeNull(); + }); + }); + // ========================================================================= // Dashboard queries // ========================================================================= diff --git a/tests/unit/api/access-control.test.ts b/tests/unit/api/access-control.test.ts index 3d69f792..9c76e737 100644 --- a/tests/unit/api/access-control.test.ts +++ b/tests/unit/api/access-control.test.ts @@ -84,10 +84,6 @@ vi.mock('../../../src/db/crypto.js', () => ({ encryptCredential: (v: string) => v, })); -vi.mock('../../../src/triggers/shared/debug-status.js', () => ({ - isAnalysisRunning: vi.fn().mockReturnValue(false), -})); - vi.mock('../../../src/config/provider.js', () => ({ loadProjectConfigById: vi.fn(), })); diff --git a/tests/unit/api/router.test.ts b/tests/unit/api/router.test.ts index eac0be65..0520b020 100644 --- a/tests/unit/api/router.test.ts +++ b/tests/unit/api/router.test.ts @@ -27,10 +27,6 @@ vi.mock('../../../src/config/provider.js', () => ({ loadConfig: vi.fn(), })); -vi.mock('../../../src/triggers/shared/debug-status.js', () => ({ - isAnalysisRunning: vi.fn(), -})); - vi.mock('../../../src/triggers/shared/debug-runner.js', () => ({ triggerDebugAnalysis: vi.fn(), })); diff --git a/tests/unit/api/routers/runs.test.ts b/tests/unit/api/routers/runs.test.ts index c7abe030..8895088d 100644 --- a/tests/unit/api/routers/runs.test.ts +++ b/tests/unit/api/routers/runs.test.ts @@ -15,15 +15,20 @@ const { mockGetLlmCallByNumber, mockGetDebugAnalysisByRunId, mockDeleteDebugAnalysisByRunId, + mockGetDebugAnalysisRunState, + mockMarkDebugAnalysisRunning, + mockIsDebugAnalysisRunActive, mockHasActiveRunForWorkItem, mockCancelRunById, - mockIsAnalysisRunning, mockTriggerDebugAnalysis, mockTriggerManualRun, mockTriggerRetryRun, mockLoadProjectConfigById, mockPublishCancelCommand, mockIsAgentEnabledForProject, + mockDebugAnalysisJobId, + mockRemoveDashboardJob, + mockSubmitDashboardJob, } = vi.hoisted(() => ({ mockListRuns: vi.fn(), mockGetRunById: vi.fn(), @@ -32,15 +37,29 @@ const { mockGetLlmCallByNumber: vi.fn(), mockGetDebugAnalysisByRunId: vi.fn(), mockDeleteDebugAnalysisByRunId: vi.fn(), + // Durable debug-analysis lifecycle status (debug_analysis_status table). + mockGetDebugAnalysisRunState: vi.fn(), + mockMarkDebugAnalysisRunning: vi.fn().mockResolvedValue(undefined), + // Real-logic stand-in so router precedence (running vs stale) is exercised + // faithfully; staleness itself is unit-tested in the repository. + mockIsDebugAnalysisRunActive: vi.fn( + (state: { status: string; updatedAt: Date | null } | null) => { + if (!state || state.status !== 'running') return false; + if (!state.updatedAt) return true; + return Date.now() - new Date(state.updatedAt).getTime() < 2 * 60 * 60 * 1000; + }, + ), mockHasActiveRunForWorkItem: vi.fn().mockResolvedValue(false), mockCancelRunById: vi.fn().mockResolvedValue(true), - mockIsAnalysisRunning: vi.fn(), mockTriggerDebugAnalysis: vi.fn(), mockTriggerManualRun: vi.fn(), mockTriggerRetryRun: vi.fn(), mockLoadProjectConfigById: vi.fn(), mockPublishCancelCommand: vi.fn().mockResolvedValue(undefined), mockIsAgentEnabledForProject: vi.fn().mockResolvedValue(true), + mockDebugAnalysisJobId: vi.fn((runId: string) => `debug-analysis-${runId}`), + mockRemoveDashboardJob: vi.fn().mockResolvedValue(undefined), + mockSubmitDashboardJob: vi.fn().mockResolvedValue('debug-analysis-job-id'), })); vi.mock('../../../../src/db/repositories/runsRepository.js', () => ({ @@ -52,6 +71,9 @@ vi.mock('../../../../src/db/repositories/runsRepository.js', () => ({ getLlmCallByNumber: mockGetLlmCallByNumber, getDebugAnalysisByRunId: mockGetDebugAnalysisByRunId, deleteDebugAnalysisByRunId: mockDeleteDebugAnalysisByRunId, + getDebugAnalysisRunState: mockGetDebugAnalysisRunState, + markDebugAnalysisRunning: mockMarkDebugAnalysisRunning, + isDebugAnalysisRunActive: mockIsDebugAnalysisRunActive, hasActiveRunForWorkItem: mockHasActiveRunForWorkItem, cancelRunById: mockCancelRunById, })); @@ -69,11 +91,6 @@ vi.mock('../../../../src/db/schema/index.js', () => ({ projects: { id: 'id', orgId: 'org_id' }, })); -// Mock debug-status tracker -vi.mock('../../../../src/triggers/shared/debug-status.js', () => ({ - isAnalysisRunning: mockIsAnalysisRunning, -})); - // Mock triggerDebugAnalysis (fire-and-forget) vi.mock('../../../../src/triggers/shared/debug-runner.js', () => ({ triggerDebugAnalysis: mockTriggerDebugAnalysis, @@ -105,6 +122,13 @@ vi.mock('../../../../src/db/repositories/agentConfigsRepository.js', () => ({ isAgentEnabledForProject: mockIsAgentEnabledForProject, })); +// Mock the durable dashboard-jobs queue client (queue-mode debug-analysis path) +vi.mock('../../../../src/queue/client.js', () => ({ + debugAnalysisJobId: mockDebugAnalysisJobId, + removeDashboardJob: mockRemoveDashboardJob, + submitDashboardJob: mockSubmitDashboardJob, +})); + import { runsRouter } from '../../../../src/api/routers/runs.js'; const createCaller = createCallerFor(runsRouter); @@ -598,23 +622,27 @@ describe('runsRouter', () => { }); describe('getDebugAnalysisStatus', () => { - it('returns running when analysis is in progress', async () => { + // Status is derived uniformly (queue + local dev) from the durable + // `debug_analysis_status` row plus the persisted analysis — never from the + // BullMQ job, which completes at container spawn rather than at analysis end. + beforeEach(() => { mockGetRunById.mockResolvedValue({ id: RUN_UUID, projectId: 'p1' }); mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); - mockIsAnalysisRunning.mockReturnValue(true); + }); + + it('returns running when an active status row exists, short-circuiting the DB', async () => { + mockGetDebugAnalysisRunState.mockResolvedValue({ status: 'running', updatedAt: new Date() }); const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); const result = await caller.getDebugAnalysisStatus({ runId: RUN_UUID }); expect(result).toEqual({ status: 'running' }); - // Should not query DB for analysis when running + // An active run short-circuits the content lookup. expect(mockGetDebugAnalysisByRunId).not.toHaveBeenCalled(); }); it('returns completed when analysis exists in DB', async () => { - mockGetRunById.mockResolvedValue({ id: RUN_UUID, projectId: 'p1' }); - mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); - mockIsAnalysisRunning.mockReturnValue(false); + mockGetDebugAnalysisRunState.mockResolvedValue(null); mockGetDebugAnalysisByRunId.mockResolvedValue({ summary: 'done' }); const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); @@ -623,16 +651,48 @@ describe('runsRouter', () => { expect(result).toEqual({ status: 'completed' }); }); - it('returns idle when not running and no analysis exists', async () => { - mockGetRunById.mockResolvedValue({ id: RUN_UUID, projectId: 'p1' }); - mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); - mockIsAnalysisRunning.mockReturnValue(false); + it('returns completed when a content row exists even alongside a failed status row (DB wins)', async () => { + mockGetDebugAnalysisRunState.mockResolvedValue({ status: 'failed', updatedAt: new Date() }); + mockGetDebugAnalysisByRunId.mockResolvedValue({ summary: 'done' }); + + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + const result = await caller.getDebugAnalysisStatus({ runId: RUN_UUID }); + + expect(result).toEqual({ status: 'completed' }); + }); + + it('returns failed when the status row is failed and no analysis exists', async () => { + mockGetDebugAnalysisRunState.mockResolvedValue({ status: 'failed', updatedAt: new Date() }); + mockGetDebugAnalysisByRunId.mockResolvedValue(null); + + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + const result = await caller.getDebugAnalysisStatus({ runId: RUN_UUID }); + + expect(result).toEqual({ status: 'failed' }); + }); + + it('returns idle when there is no status row and no analysis', async () => { + mockGetDebugAnalysisRunState.mockResolvedValue(null); + mockGetDebugAnalysisByRunId.mockResolvedValue(null); + + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + const result = await caller.getDebugAnalysisStatus({ runId: RUN_UUID }); + + expect(result).toEqual({ status: 'idle' }); + }); + + it('treats a stale running row as idle (crashed worker never wedges)', async () => { + // `running` but older than the staleness window → not active. With no + // content row, precedence falls through to idle and the DB is consulted. + const stale = new Date(Date.now() - 3 * 60 * 60 * 1000); + mockGetDebugAnalysisRunState.mockResolvedValue({ status: 'running', updatedAt: stale }); mockGetDebugAnalysisByRunId.mockResolvedValue(null); const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); const result = await caller.getDebugAnalysisStatus({ runId: RUN_UUID }); expect(result).toEqual({ status: 'idle' }); + expect(mockGetDebugAnalysisByRunId).toHaveBeenCalled(); }); it('throws UNAUTHORIZED when unauthenticated', async () => { @@ -663,7 +723,7 @@ describe('runsRouter', () => { it('allows superadmin to get debug analysis status from any org', async () => { mockGetRunById.mockResolvedValue({ id: RUN_UUID, projectId: 'p1' }); - mockIsAnalysisRunning.mockReturnValue(false); + mockGetDebugAnalysisRunState.mockResolvedValue(null); mockGetDebugAnalysisByRunId.mockResolvedValue({ summary: 'done' }); const superAdmin = createMockSuperAdmin(); @@ -675,7 +735,12 @@ describe('runsRouter', () => { }); }); - describe('triggerDebugAnalysis', () => { + describe('triggerDebugAnalysis (local dev / non-queue)', () => { + beforeEach(() => { + // No status row → guard passes. + mockGetDebugAnalysisRunState.mockResolvedValue(null); + }); + it('triggers analysis for a valid run', async () => { mockGetRunById.mockResolvedValue({ id: RUN_UUID, @@ -684,7 +749,6 @@ describe('runsRouter', () => { workItemId: 'card-1', }); mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); - mockIsAnalysisRunning.mockReturnValue(false); mockLoadProjectConfigById.mockResolvedValue({ project: { id: 'p1', name: 'Test' }, config: {}, @@ -696,6 +760,8 @@ describe('runsRouter', () => { expect(result).toEqual({ triggered: true }); expect(mockDeleteDebugAnalysisByRunId).toHaveBeenCalledWith(RUN_UUID); + // Durable `running` marker is written before the in-process fire. + expect(mockMarkDebugAnalysisRunning).toHaveBeenCalledWith(RUN_UUID); expect(mockTriggerDebugAnalysis).toHaveBeenCalledWith( RUN_UUID, { id: 'p1', name: 'Test' }, @@ -712,7 +778,6 @@ describe('runsRouter', () => { workItemId: null, }); mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); - mockIsAnalysisRunning.mockReturnValue(false); mockLoadProjectConfigById.mockResolvedValue({ project: { id: 'p1', name: 'Test' }, config: {}, @@ -767,19 +832,46 @@ describe('runsRouter', () => { }); }); - it('throws CONFLICT when analysis is already running', async () => { + it('throws CONFLICT when an analysis is already running (durable status row)', async () => { mockGetRunById.mockResolvedValue({ id: RUN_UUID, projectId: 'p1', agentType: 'implementation', }); mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); - mockIsAnalysisRunning.mockReturnValue(true); + mockGetDebugAnalysisRunState.mockResolvedValue({ status: 'running', updatedAt: new Date() }); const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); await expect(caller.triggerDebugAnalysis({ runId: RUN_UUID })).rejects.toMatchObject({ code: 'CONFLICT', }); + expect(mockMarkDebugAnalysisRunning).not.toHaveBeenCalled(); + expect(mockTriggerDebugAnalysis).not.toHaveBeenCalled(); + }); + + it('proceeds when the status row is a stale running marker (crash recovery)', async () => { + mockGetRunById.mockResolvedValue({ + id: RUN_UUID, + projectId: 'p1', + agentType: 'implementation', + workItemId: 'card-1', + }); + mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); + mockLoadProjectConfigById.mockResolvedValue({ + project: { id: 'p1', name: 'Test' }, + config: {}, + }); + // `running` but older than the staleness window → not active. + mockGetDebugAnalysisRunState.mockResolvedValue({ + status: 'running', + updatedAt: new Date(Date.now() - 3 * 60 * 60 * 1000), + }); + + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + const result = await caller.triggerDebugAnalysis({ runId: RUN_UUID }); + + expect(result).toEqual({ triggered: true }); + expect(mockTriggerDebugAnalysis).toHaveBeenCalled(); }); it('throws BAD_REQUEST when run has no projectId', async () => { @@ -788,7 +880,6 @@ describe('runsRouter', () => { projectId: null, agentType: 'implementation', }); - mockIsAnalysisRunning.mockReturnValue(false); const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); await expect(caller.triggerDebugAnalysis({ runId: RUN_UUID })).rejects.toMatchObject({ @@ -803,7 +894,6 @@ describe('runsRouter', () => { agentType: 'implementation', }); mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); - mockIsAnalysisRunning.mockReturnValue(false); mockLoadProjectConfigById.mockResolvedValue(undefined); const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); @@ -820,6 +910,99 @@ describe('runsRouter', () => { }); }); + describe('triggerDebugAnalysis (queue mode)', () => { + beforeEach(() => { + vi.stubEnv('REDIS_URL', 'redis://localhost:6379'); + mockGetRunById.mockResolvedValue({ + id: RUN_UUID, + projectId: 'p1', + agentType: 'implementation', + workItemId: 'card-1', + }); + mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); + mockLoadProjectConfigById.mockResolvedValue({ + project: { id: 'p1', name: 'Test' }, + config: {}, + }); + mockDeleteDebugAnalysisByRunId.mockResolvedValue(undefined); + mockRemoveDashboardJob.mockResolvedValue(undefined); + mockSubmitDashboardJob.mockResolvedValue('debug-analysis-job-id'); + // No status row → guard passes. + mockGetDebugAnalysisRunState.mockResolvedValue(null); + }); + + it('throws CONFLICT when an analysis is already running (durable status row)', async () => { + mockGetDebugAnalysisRunState.mockResolvedValue({ status: 'running', updatedAt: new Date() }); + + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + await expect(caller.triggerDebugAnalysis({ runId: RUN_UUID })).rejects.toMatchObject({ + code: 'CONFLICT', + }); + // Must not enqueue or mark when an analysis is already in flight. + expect(mockRemoveDashboardJob).not.toHaveBeenCalled(); + expect(mockSubmitDashboardJob).not.toHaveBeenCalled(); + expect(mockMarkDebugAnalysisRunning).not.toHaveBeenCalled(); + }); + + it('re-enqueues with the deterministic id, clearing any prior job first', async () => { + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + const result = await caller.triggerDebugAnalysis({ runId: RUN_UUID }); + + expect(result).toEqual({ triggered: true }); + const jobId = `debug-analysis-${RUN_UUID}`; + expect(mockDeleteDebugAnalysisByRunId).toHaveBeenCalledWith(RUN_UUID); + expect(mockRemoveDashboardJob).toHaveBeenCalledWith(jobId); + expect(mockSubmitDashboardJob).toHaveBeenCalledWith( + { + type: 'debug-analysis', + runId: RUN_UUID, + projectId: 'p1', + workItemId: 'card-1', + }, + jobId, + ); + expect(mockMarkDebugAnalysisRunning).toHaveBeenCalledWith(RUN_UUID); + // Non-queue fire-and-forget branch must not run in queue mode + expect(mockTriggerDebugAnalysis).not.toHaveBeenCalled(); + }); + + it('marks running only after the job is enqueued (remove → submit → mark)', async () => { + const callOrder: string[] = []; + mockRemoveDashboardJob.mockImplementation(async () => { + callOrder.push('remove'); + }); + mockSubmitDashboardJob.mockImplementation(async () => { + callOrder.push('submit'); + return 'debug-analysis-job-id'; + }); + mockMarkDebugAnalysisRunning.mockImplementation(async () => { + callOrder.push('mark'); + }); + + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + await caller.triggerDebugAnalysis({ runId: RUN_UUID }); + + expect(callOrder).toEqual(['remove', 'submit', 'mark']); + }); + + it('passes undefined workItemId when the run has no card', async () => { + mockGetRunById.mockResolvedValue({ + id: RUN_UUID, + projectId: 'p1', + agentType: 'implementation', + workItemId: null, + }); + + const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); + await caller.triggerDebugAnalysis({ runId: RUN_UUID }); + + expect(mockSubmitDashboardJob).toHaveBeenCalledWith( + expect.objectContaining({ workItemId: undefined }), + `debug-analysis-${RUN_UUID}`, + ); + }); + }); + describe('trigger', () => { it('fires a manual run and returns triggered:true', async () => { mockDbWhere.mockResolvedValue([{ orgId: 'org-1' }]); diff --git a/tests/unit/cli/dashboard/runs/runs.test.ts b/tests/unit/cli/dashboard/runs/runs.test.ts index cd0402a2..43f1107d 100644 --- a/tests/unit/cli/dashboard/runs/runs.test.ts +++ b/tests/unit/cli/dashboard/runs/runs.test.ts @@ -502,6 +502,22 @@ describe('RunsDebug trigger with wait (runs debug --analyze --wait)', () => { ); }); + it('stops polling when status is failed', async () => { + const client = makeClient(); + const statusQuery = client.runs.getDebugAnalysisStatus.query as ReturnType; + statusQuery.mockResolvedValue({ status: 'failed' }); + mockCreateDashboardClient.mockReturnValue(client); + + const cmd = new RunsDebug(['run-uuid-dbg', '--analyze', '--wait'], oclifConfig as never); + const logSpy = vi.spyOn(cmd as unknown as { log: (s: string) => void }, 'log'); + + const runPromise = cmd.run(); + await vi.advanceTimersByTimeAsync(5000); + await runPromise; + + expect(logSpy).toHaveBeenCalledWith('Debug analysis failed.'); + }); + it('logs timeout message after 5-minute deadline', async () => { const client = makeClient(); const statusQuery = client.runs.getDebugAnalysisStatus.query as ReturnType; diff --git a/tests/unit/db/repositories/debugAnalysisRepository.test.ts b/tests/unit/db/repositories/debugAnalysisRepository.test.ts index 8425759d..c1ff925c 100644 --- a/tests/unit/db/repositories/debugAnalysisRepository.test.ts +++ b/tests/unit/db/repositories/debugAnalysisRepository.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { createMockDbWithGetDb } from '../../../helpers/mockDb.js'; import { mockDbClientModule } from '../../../helpers/sharedMocks.js'; @@ -16,12 +16,23 @@ vi.mock('../../../../src/db/schema/index.js', () => ({ rootCause: 'root_cause', severity: 'severity', }, + debugAnalysisStatus: { + analyzedRunId: 'analyzed_run_id', + status: 'status', + updatedAt: 'updated_at', + }, })); import { + clearDebugAnalysisStatus, + DEBUG_ANALYSIS_RUNNING_STALE_MS, deleteDebugAnalysisByRunId, getDebugAnalysisByDebugRunId, getDebugAnalysisByRunId, + getDebugAnalysisRunState, + isDebugAnalysisRunActive, + markDebugAnalysisFailed, + markDebugAnalysisRunning, storeDebugAnalysis, } from '../../../../src/db/repositories/debugAnalysisRepository.js'; @@ -29,7 +40,7 @@ describe('debugAnalysisRepository', () => { let mockDb: ReturnType; beforeEach(() => { - mockDb = createMockDbWithGetDb(); + mockDb = createMockDbWithGetDb({ withUpsert: true }); }); describe('storeDebugAnalysis', () => { @@ -169,4 +180,115 @@ describe('debugAnalysisRepository', () => { expect(result).toBeNull(); }); }); + + describe('markDebugAnalysisRunning', () => { + it('upserts a running status row keyed by analyzed run', async () => { + await markDebugAnalysisRunning('run-1'); + + expect(mockDb.db.insert).toHaveBeenCalled(); + expect(mockDb.chain.values).toHaveBeenCalledWith( + expect.objectContaining({ analyzedRunId: 'run-1', status: 'running' }), + ); + expect(mockDb.chain.onConflictDoUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + set: expect.objectContaining({ status: 'running' }), + }), + ); + }); + }); + + describe('markDebugAnalysisFailed', () => { + it('upserts a failed status row keyed by analyzed run', async () => { + await markDebugAnalysisFailed('run-1'); + + expect(mockDb.chain.values).toHaveBeenCalledWith( + expect.objectContaining({ analyzedRunId: 'run-1', status: 'failed' }), + ); + expect(mockDb.chain.onConflictDoUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + set: expect.objectContaining({ status: 'failed' }), + }), + ); + }); + }); + + describe('clearDebugAnalysisStatus', () => { + it('deletes the status row by analyzed run', async () => { + mockDb.chain.where.mockResolvedValueOnce(undefined); + + await clearDebugAnalysisStatus('run-1'); + + expect(mockDb.db.delete).toHaveBeenCalled(); + expect(mockDb.chain.where).toHaveBeenCalled(); + }); + }); + + describe('getDebugAnalysisRunState', () => { + it('returns the status row when present', async () => { + const updatedAt = new Date(); + mockDb.chain.where.mockResolvedValueOnce([{ status: 'running', updatedAt }]); + + const result = await getDebugAnalysisRunState('run-1'); + + expect(result).toEqual({ status: 'running', updatedAt }); + }); + + it('returns null when no status row exists', async () => { + mockDb.chain.where.mockResolvedValueOnce([]); + + const result = await getDebugAnalysisRunState('run-1'); + + expect(result).toBeNull(); + }); + }); + + describe('isDebugAnalysisRunActive', () => { + it('returns false for a null state', () => { + expect(isDebugAnalysisRunActive(null)).toBe(false); + }); + + it('returns false for a terminal (failed) status', () => { + expect(isDebugAnalysisRunActive({ status: 'failed', updatedAt: new Date() })).toBe(false); + }); + + it('returns true for a fresh running status', () => { + expect(isDebugAnalysisRunActive({ status: 'running', updatedAt: new Date() })).toBe(true); + }); + + it('returns true for a running status with no timestamp (defensive)', () => { + expect(isDebugAnalysisRunActive({ status: 'running', updatedAt: null })).toBe(true); + }); + + it('returns false for a stale running status (crashed worker)', () => { + const stale = new Date(Date.now() - DEBUG_ANALYSIS_RUNNING_STALE_MS - 1_000); + expect(isDebugAnalysisRunActive({ status: 'running', updatedAt: stale })).toBe(false); + }); + }); + + // The staleness check uses a strict `<` against DEBUG_ANALYSIS_RUNNING_STALE_MS, + // so a `running` row aged *exactly* at the threshold is already treated as stale. + // That boundary is what guarantees a crashed/OOM-killed worker never wedges the + // run as permanently `running`. Freeze the clock so the off-by-one is asserted + // deterministically, then restore real timers — this file runs under unit-core + // with `isolate: false`, where a leaked fake timer would bleed into sibling files. + describe('isDebugAnalysisRunActive — exact staleness boundary', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-06-25T12:00:00Z')); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('treats a running row aged exactly at the threshold as stale (strict <)', () => { + const updatedAt = new Date(Date.now() - DEBUG_ANALYSIS_RUNNING_STALE_MS); + expect(isDebugAnalysisRunActive({ status: 'running', updatedAt })).toBe(false); + }); + + it('treats a running row aged one ms under the threshold as active', () => { + const updatedAt = new Date(Date.now() - DEBUG_ANALYSIS_RUNNING_STALE_MS + 1); + expect(isDebugAnalysisRunActive({ status: 'running', updatedAt })).toBe(true); + }); + }); }); diff --git a/tests/unit/queue/client.test.ts b/tests/unit/queue/client.test.ts index 6f3905ab..378e8f65 100644 --- a/tests/unit/queue/client.test.ts +++ b/tests/unit/queue/client.test.ts @@ -8,13 +8,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; // ── Mocks (must be set up before dynamic import) ────────────────────────────── const mockQueueAdd = vi.fn(); -const mockQueueGetJob = vi.fn(); const mockQueueRemove = vi.fn(); vi.mock('bullmq', () => ({ Queue: vi.fn().mockImplementation(() => ({ add: mockQueueAdd, - getJob: mockQueueGetJob, remove: mockQueueRemove, })), })); @@ -39,7 +37,6 @@ async function freshImport() { vi.mock('bullmq', () => ({ Queue: vi.fn().mockImplementation(() => ({ add: mockQueueAdd, - getJob: mockQueueGetJob, remove: mockQueueRemove, })), })); @@ -212,39 +209,6 @@ describe('debugAnalysisJobId', () => { }); }); -describe('getDashboardJobState', () => { - beforeEach(() => { - vi.stubEnv('REDIS_URL', 'redis://localhost:6379'); - }); - - afterEach(() => { - vi.unstubAllEnvs(); - vi.resetModules(); - }); - - it('resolves the job by id and returns its BullMQ state', async () => { - const getState = vi.fn().mockResolvedValue('active'); - mockQueueGetJob.mockResolvedValue({ getState }); - const { getDashboardJobState } = await freshImport(); - - const state = await getDashboardJobState('debug-analysis-run-1'); - - expect(mockQueueGetJob).toHaveBeenCalledWith('debug-analysis-run-1'); - expect(getState).toHaveBeenCalledTimes(1); - expect(state).toBe('active'); - }); - - it('returns null when the job is absent', async () => { - mockQueueGetJob.mockResolvedValue(undefined); - const { getDashboardJobState } = await freshImport(); - - const state = await getDashboardJobState('debug-analysis-missing'); - - expect(mockQueueGetJob).toHaveBeenCalledWith('debug-analysis-missing'); - expect(state).toBeNull(); - }); -}); - describe('removeDashboardJob', () => { beforeEach(() => { vi.stubEnv('REDIS_URL', 'redis://localhost:6379'); diff --git a/tests/unit/queue/retry-run-projectId.test.ts b/tests/unit/queue/retry-run-projectId.test.ts index cc8c02e6..b4f7a1e9 100644 --- a/tests/unit/queue/retry-run-projectId.test.ts +++ b/tests/unit/queue/retry-run-projectId.test.ts @@ -7,7 +7,10 @@ */ import { beforeEach, describe, expect, it, vi } from 'vitest'; -// Set REDIS_URL before any imports to enable queue path +// Enable the queue path. The router reads REDIS_URL at call time, so the +// load-bearing stub is the per-test one in beforeEach (vitest's unstubEnvs:true +// clears env stubs before each test); this top-level stub keeps import-time +// intent clear. vi.stubEnv('REDIS_URL', 'redis://localhost:6379'); // Mock the queue client to capture job submissions @@ -57,11 +60,6 @@ vi.mock('../../../src/db/repositories/agentConfigsRepository.js', () => ({ isAgentEnabledForProject: (...args: unknown[]) => mockIsAgentEnabledForProject(...args), })); -// Mock debug-status -vi.mock('../../../src/triggers/shared/debug-status.js', () => ({ - isAnalysisRunning: vi.fn().mockReturnValue(false), -})); - // Mock logger vi.mock('../../../src/utils/logging.js', () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, @@ -88,6 +86,10 @@ const RUN_UUID = 'aaaaaaaa-1111-2222-3333-444444444444'; describe('retry-run job submission with projectId', () => { beforeEach(() => { + // Queue mode is gated on REDIS_URL, read at call time by the router. + // Re-stub each test because vitest's unstubEnvs:true clears env stubs + // before every test. + vi.stubEnv('REDIS_URL', 'redis://localhost:6379'); mockSubmitDashboardJob.mockClear(); mockGetRunById.mockReset(); mockLoadProjectConfigById.mockReset(); diff --git a/tests/unit/triggers/debug-runner.test.ts b/tests/unit/triggers/debug-runner.test.ts index 44513d8d..38f80a89 100644 --- a/tests/unit/triggers/debug-runner.test.ts +++ b/tests/unit/triggers/debug-runner.test.ts @@ -10,6 +10,9 @@ vi.mock('../../../src/db/repositories/runsRepository.js', () => ({ getRunLogs: vi.fn(), getLlmCallsByRunId: vi.fn(), storeDebugAnalysis: vi.fn(), + markDebugAnalysisRunning: vi.fn().mockResolvedValue(undefined), + markDebugAnalysisFailed: vi.fn().mockResolvedValue(undefined), + clearDebugAnalysisStatus: vi.fn().mockResolvedValue(undefined), })); vi.mock('../../../src/pm/index.js', () => ({ @@ -28,25 +31,20 @@ vi.mock('../../../src/utils/repo.js', () => ({ cleanupTempDir: vi.fn(), })); -vi.mock('../../../src/triggers/shared/debug-status.js', () => ({ - markAnalysisRunning: vi.fn(), - markAnalysisComplete: vi.fn(), -})); - import { runAgent } from '../../../src/agents/registry.js'; import { + clearDebugAnalysisStatus, getLlmCallsByRunId, getRunById, getRunLogs, + markDebugAnalysisFailed, + markDebugAnalysisRunning, storeDebugAnalysis, } from '../../../src/db/repositories/runsRepository.js'; import type { PMProvider } from '../../../src/pm/index.js'; import { getPMProvider } from '../../../src/pm/index.js'; import { triggerDebugAnalysis } from '../../../src/triggers/shared/debug-runner.js'; -import { - markAnalysisComplete, - markAnalysisRunning, -} from '../../../src/triggers/shared/debug-status.js'; +import { logger } from '../../../src/utils/logging.js'; const mockPMProvider = { addComment: vi.fn() }; @@ -152,7 +150,7 @@ describe('triggerDebugAnalysis', () => { ); }); - it('calls markAnalysisRunning at start and markAnalysisComplete on success', async () => { + it('marks the analysis running at start and clears the status on success', async () => { vi.mocked(getRunById).mockResolvedValue({ id: 'run-1', agentType: 'implementation', @@ -172,11 +170,14 @@ describe('triggerDebugAnalysis', () => { await triggerDebugAnalysis('run-1', mockProject, mockConfig); - expect(markAnalysisRunning).toHaveBeenCalledWith('run-1'); - expect(markAnalysisComplete).toHaveBeenCalledWith('run-1'); + expect(markDebugAnalysisRunning).toHaveBeenCalledWith('run-1'); + // Success clears the durable status row; the persisted analysis is then + // the `completed` signal. + expect(clearDebugAnalysisStatus).toHaveBeenCalledWith('run-1'); + expect(markDebugAnalysisFailed).not.toHaveBeenCalled(); }); - it('calls markAnalysisComplete even when agent throws', async () => { + it('marks the analysis failed (durably) when the agent throws', async () => { vi.mocked(getRunById).mockResolvedValue({ id: 'run-1', agentType: 'implementation', @@ -192,8 +193,38 @@ describe('triggerDebugAnalysis', () => { 'Agent crashed', ); - expect(markAnalysisRunning).toHaveBeenCalledWith('run-1'); - expect(markAnalysisComplete).toHaveBeenCalledWith('run-1'); + expect(markDebugAnalysisRunning).toHaveBeenCalledWith('run-1'); + // Failure persists `failed` and must NOT clear the status (status should + // read `failed`, not `idle`). + expect(markDebugAnalysisFailed).toHaveBeenCalledWith('run-1'); + expect(clearDebugAnalysisStatus).not.toHaveBeenCalled(); + }); + + it('logs a warning but still throws the original error when the durable failed-write itself fails', async () => { + vi.mocked(getRunById).mockResolvedValue({ + id: 'run-1', + agentType: 'implementation', + status: 'failed', + } as ReturnType extends Promise ? NonNullable : never); + + vi.mocked(getRunLogs).mockResolvedValue(null); + vi.mocked(getLlmCallsByRunId).mockResolvedValue([]); + + vi.mocked(runAgent).mockRejectedValue(new Error('Agent crashed')); + // The durable `failed` write itself errors (e.g. DB unavailable); it must + // not mask or replace the original agent failure. + vi.mocked(markDebugAnalysisFailed).mockRejectedValueOnce(new Error('DB unavailable')); + + await expect(triggerDebugAnalysis('run-1', mockProject, mockConfig)).rejects.toThrow( + 'Agent crashed', + ); + + expect(markDebugAnalysisFailed).toHaveBeenCalledWith('run-1'); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to mark debug analysis failed', + expect.objectContaining({ analyzedRunId: 'run-1' }), + ); + expect(clearDebugAnalysisStatus).not.toHaveBeenCalled(); }); it('sets severity to manual for completed runs', async () => { diff --git a/tests/unit/triggers/debug-status.test.ts b/tests/unit/triggers/debug-status.test.ts deleted file mode 100644 index 95efa3c7..00000000 --- a/tests/unit/triggers/debug-status.test.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { afterEach, describe, expect, it } from 'vitest'; -import { - isAnalysisRunning, - markAnalysisComplete, - markAnalysisRunning, -} from '../../../src/triggers/shared/debug-status.js'; - -describe('debug-status tracker', () => { - afterEach(() => { - // Clean up any leftover state - markAnalysisComplete('run-1'); - markAnalysisComplete('run-2'); - }); - - it('reports idle by default', () => { - expect(isAnalysisRunning('run-1')).toBe(false); - }); - - it('marks a run as running', () => { - markAnalysisRunning('run-1'); - expect(isAnalysisRunning('run-1')).toBe(true); - }); - - it('marks a run as complete', () => { - markAnalysisRunning('run-1'); - markAnalysisComplete('run-1'); - expect(isAnalysisRunning('run-1')).toBe(false); - }); - - it('tracks multiple runs independently', () => { - markAnalysisRunning('run-1'); - markAnalysisRunning('run-2'); - - expect(isAnalysisRunning('run-1')).toBe(true); - expect(isAnalysisRunning('run-2')).toBe(true); - - markAnalysisComplete('run-1'); - expect(isAnalysisRunning('run-1')).toBe(false); - expect(isAnalysisRunning('run-2')).toBe(true); - }); - - it('is idempotent for markAnalysisRunning', () => { - markAnalysisRunning('run-1'); - markAnalysisRunning('run-1'); - expect(isAnalysisRunning('run-1')).toBe(true); - - markAnalysisComplete('run-1'); - expect(isAnalysisRunning('run-1')).toBe(false); - }); - - it('is safe to call markAnalysisComplete on non-running run', () => { - markAnalysisComplete('run-1'); - expect(isAnalysisRunning('run-1')).toBe(false); - }); -}); diff --git a/tests/unit/worker-entry.test.ts b/tests/unit/worker-entry.test.ts index f5b4fd4d..6ec73a4d 100644 --- a/tests/unit/worker-entry.test.ts +++ b/tests/unit/worker-entry.test.ts @@ -84,6 +84,7 @@ vi.mock('../../src/triggers/shared/debug-runner.js', () => ({ vi.mock('../../src/db/repositories/runsRepository.js', () => ({ getRunById: vi.fn(), + markDebugAnalysisFailed: vi.fn().mockResolvedValue(undefined), })); vi.mock('../../src/db/seeds/seedAgentDefinitions.js', () => ({ @@ -102,7 +103,7 @@ vi.mock('../../src/agents/prompts/index.js', () => ({ import { BootFailureError } from '../../src/agents/shared/bootFailureError.js'; import { loadProjectConfigById } from '../../src/config/provider.js'; -import { getRunById } from '../../src/db/repositories/runsRepository.js'; +import { getRunById, markDebugAnalysisFailed } from '../../src/db/repositories/runsRepository.js'; import { extractJiraContext, extractLinearContext, @@ -865,7 +866,7 @@ describe('processDashboardJob - debug-analysis', () => { ); }); - it('throws when project not found for debug-analysis', async () => { + it('marks the run failed and throws when project not found for debug-analysis', async () => { vi.mocked(loadProjectConfigById).mockResolvedValue(undefined); const jobData: DebugAnalysisJobData = { @@ -877,6 +878,30 @@ describe('processDashboardJob - debug-analysis', () => { await expect(processDashboardJob('job-debug-noproj', jobData)).rejects.toThrow( 'Project not found: bad-proj', ); + + // The dashboard already marked this run `running` at trigger time, and this + // throw happens before triggerDebugAnalysis (whose catch would flip it to + // `failed`) is reached. The worker surfaces the failure here so status reads + // `failed` instead of leaving a `running` row to self-stale to `idle`. + expect(markDebugAnalysisFailed).toHaveBeenCalledWith('run-xyz'); + }); + + it('still throws the original project-not-found error when the failed-write fails (best-effort)', async () => { + vi.mocked(loadProjectConfigById).mockResolvedValue(undefined); + // The durable `failed` write itself errors (e.g. DB unavailable); it must + // not mask or replace the original project-not-found failure. + vi.mocked(markDebugAnalysisFailed).mockRejectedValueOnce(new Error('DB unavailable')); + + const jobData: DebugAnalysisJobData = { + type: 'debug-analysis', + runId: 'run-xyz', + projectId: 'bad-proj', + }; + + await expect(processDashboardJob('job-debug-noproj', jobData)).rejects.toThrow( + 'Project not found: bad-proj', + ); + expect(markDebugAnalysisFailed).toHaveBeenCalledWith('run-xyz'); }); }); From f8088dc9979c6ddb710ae0c14e4978f935a87097 Mon Sep 17 00:00:00 2001 From: aaight Date: Thu, 25 Jun 2026 19:58:54 +0200 Subject: [PATCH 4/8] test(cli): cover debug-analysis --wait failed branch json + no-timeout (MNG-1669) (#1456) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `failed` branch in the `cascade runs debug --analyze --wait` poll loop shipped in #1453 (commit 48bdb26), but two of MNG-1669's acceptance criteria lacked dedicated regression coverage: - AC4 ("returns on failed without waiting for the timeout"): the existing test only asserted the failure message. Add a test pinning that the loop polls exactly once and never prints the timeout message. - AC3 ("--json output for the failed case is consistent with other terminal branches"): no test existed. Add a test asserting that under --json the failed branch logs the plain message, does not emit a JSON object (outputJson not called), and does not fetch analysis content — mirroring the idle/timeout branches. Test-only; no production behavior change. Co-authored-by: Cascade Bot Co-authored-by: Claude Opus 4.8 --- tests/unit/cli/dashboard/runs/runs.test.ts | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/unit/cli/dashboard/runs/runs.test.ts b/tests/unit/cli/dashboard/runs/runs.test.ts index 43f1107d..f847f226 100644 --- a/tests/unit/cli/dashboard/runs/runs.test.ts +++ b/tests/unit/cli/dashboard/runs/runs.test.ts @@ -518,6 +518,55 @@ describe('RunsDebug trigger with wait (runs debug --analyze --wait)', () => { expect(logSpy).toHaveBeenCalledWith('Debug analysis failed.'); }); + it('returns on the first failed poll without waiting for the timeout', async () => { + const client = makeClient(); + const statusQuery = client.runs.getDebugAnalysisStatus.query as ReturnType; + statusQuery.mockResolvedValue({ status: 'failed' }); + mockCreateDashboardClient.mockReturnValue(client); + + const cmd = new RunsDebug(['run-uuid-dbg', '--analyze', '--wait'], oclifConfig as never); + const logSpy = vi.spyOn(cmd as unknown as { log: (s: string) => void }, 'log'); + + const runPromise = cmd.run(); + // A single poll cycle is enough — the failed branch must return on the first poll. + await vi.advanceTimersByTimeAsync(5000); + await runPromise; + + // Polled exactly once, then returned — it did NOT keep polling toward the + // 5-minute deadline (the regression this branch guards against). + expect(statusQuery).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith('Debug analysis failed.'); + expect(logSpy).not.toHaveBeenCalledWith('Timed out waiting for debug analysis to complete.'); + }); + + it('failed --json output is consistent with the other no-result terminal branches', async () => { + const client = makeClient(); + const statusQuery = client.runs.getDebugAnalysisStatus.query as ReturnType; + statusQuery.mockResolvedValue({ status: 'failed' }); + mockCreateDashboardClient.mockReturnValue(client); + + const cmd = new RunsDebug( + ['run-uuid-dbg', '--analyze', '--wait', '--json'], + oclifConfig as never, + ); + const logSpy = vi.spyOn(cmd as unknown as { log: (s: string) => void }, 'log'); + const outputJsonSpy = vi.spyOn( + cmd as unknown as { outputJson: (d: unknown) => void }, + 'outputJson', + ); + + const runPromise = cmd.run(); + await vi.advanceTimersByTimeAsync(5000); + await runPromise; + + // Like the idle and timeout branches, the failed branch surfaces a plain + // status message and — even under --json — does NOT emit a JSON object or + // fetch analysis content (there is no result to serialize). + expect(logSpy).toHaveBeenCalledWith('Debug analysis failed.'); + expect(outputJsonSpy).not.toHaveBeenCalled(); + expect(client.runs.getDebugAnalysis.query).not.toHaveBeenCalled(); + }); + it('logs timeout message after 5-minute deadline', async () => { const client = makeClient(); const statusQuery = client.runs.getDebugAnalysisStatus.query as ReturnType; From 1ebc56617a80fa810b03fc0aa6507506696e7d86 Mon Sep 17 00:00:00 2001 From: aaight Date: Thu, 25 Jun 2026 20:10:12 +0200 Subject: [PATCH 5/8] docs(architecture): document cross-process debug-analysis status model (#1457) Record the shipped durable, cross-process debug-analysis status model in the architecture deep-dives and the trigger README, replacing the stale "in-memory Set fallback" framing the work item inherited from an abandoned design. - 01-services.md: add "Cross-process debug-analysis status" subsection documenting the durable `debug_analysis_status` table read uniformly in queue + local dev, the writer/reader split, status precedence + 2h staleness self-heal, and the deterministic `debug-analysis-` dashboard job as a dedup (not status) mechanism. - 03-trigger-system.md: expand the auto-debug bullet and add a "Debug-analysis status" subsection cross-linking the service + DB docs. - src/triggers/README.md: note the durable status on the auto-debug row and add a focused subsection. Aligns the docs with the durable-table design already documented in 09-database.md (MNG-1667), which deleted the worker-local in-memory `debug-status.ts` Set because it was invisible to the dashboard process. Co-authored-by: Cascade Bot Co-authored-by: Claude Opus 4.8 --- docs/architecture/01-services.md | 10 ++++++++++ docs/architecture/03-trigger-system.md | 8 +++++++- src/triggers/README.md | 8 +++++++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/docs/architecture/01-services.md b/docs/architecture/01-services.md index dff020ba..ebdb31cc 100644 --- a/docs/architecture/01-services.md +++ b/docs/architecture/01-services.md @@ -177,3 +177,13 @@ Every tRPC request builds a context containing: - `token` — the session token, used by `auth.setActiveOrg` to switch the session's active org Procedure types enforce auth levels: `publicProcedure`, `protectedProcedure`, `adminProcedure`, `superAdminProcedure`. User-management permission checks additionally consume `resolveActorRoleInOrg()` so the caller's **per-org** membership role — not the global `users.role` — governs (an org admin who switches into an org where they are only a member cannot act as an admin there). + +### Cross-process debug-analysis status + +Post-mortem [debug analysis](./03-trigger-system.md) runs in a **separate worker container** from the dashboard API that reports its progress. Its running/failed lifecycle is therefore tracked in a **durable, cross-process** signal — the `debug_analysis_status` table (see [09-database](./09-database.md)) — rather than an in-process flag, which would be invisible across the process boundary. An earlier worker-local in-memory `Set` (`debug-status.ts`) was removed for exactly this reason (MNG-1667); status is now read from the table **uniformly in queue mode and local dev**, with no separate non-queue path. + +**Status writers** — every `triggerDebugAnalysis()` run (the automatic post-failure path and the manual "Run Analysis" button alike) marks `running` around the analysis, clears the row on success (a persisted `debug_analyses` content row is then the `completed` signal), and marks `failed` on a catchable in-process error. The dashboard additionally marks `running` at manual-trigger time to cover the enqueue→spawn window. + +**Status reads** — `runs.getDebugAnalysisStatus` derives status from `debug_analysis_status` plus the `debug_analyses` content row; the re-trigger `CONFLICT` guard reads the same `debug_analysis_status` row (active-`running` check only). BullMQ job state is deliberately **not** consulted: the dashboard job reaches `completed` at container *spawn*, not at analysis completion (the debug agent then runs for tens of seconds to minutes), so it cannot represent a still-running analysis. Read precedence: active `running` → `completed` (a persisted analysis wins) → `failed` → `idle`. A `running` row older than `DEBUG_ANALYSIS_RUNNING_STALE_MS` (2h, comfortably above the 30-min worker timeout) is treated as stale `idle`, so a crashed/OOM-killed worker never wedges the run as permanently `running`. `failed` covers catchable in-process errors only; a hard kill (watchdog/OOM) leaves the `running` row to self-stale to `idle`. + +**Deterministic job id (dedup, not status)** — in queue mode the manual trigger enqueues the `debug-analysis` dashboard job under the deterministic id `debug-analysis-` (`debugAnalysisJobId()` in `src/queue/client.ts`). One job per analyzed run makes the queue self-deduplicating: a re-run removes any prior terminal job and re-submits the same id, and a near-simultaneous second trigger that slips past the guard cannot spawn a duplicate worker container (which would double LLM cost and post a duplicate PM comment). The automatic post-run path instead calls the runner in-process (fire-and-forget) and does not use this job id, but writes the same durable status. diff --git a/docs/architecture/03-trigger-system.md b/docs/architecture/03-trigger-system.md index 1f19b3db..d646708e 100644 --- a/docs/architecture/03-trigger-system.md +++ b/docs/architecture/03-trigger-system.md @@ -238,6 +238,12 @@ This includes: - Agent execution in `agent-execution-runtime.ts`: call `runAgent()` with the resolved input plus project, config, and remaining budget. - Post-run PM behavior in `agent-pm-summary.ts` and `agent-execution-lifecycle.ts`: post review/output summaries to the PM work item, handle artifacts, post budget warnings, clean up processing state, and call `handleSuccess` or `handleFailure`. - Follow-up dispatch in `agent-execution-followups.ts`: dispatch review after a successful implementation PR once CI is passing and the review dedup key is claimed, and chain backlog-manager after a successful splitting run when the auto label/capacity checks allow it. -- Auto-debug in `agent-auto-debug.ts`: fire-and-forget debug analysis for eligible failed or timed-out runs after callbacks and follow-up dispatch complete. +- Auto-debug in `agent-auto-debug.ts`: fire-and-forget debug analysis for eligible failed or timed-out runs after callbacks and follow-up dispatch complete. It calls the shared `triggerDebugAnalysis()` runner, whose running/failed lifecycle is durable and cross-process — see [Debug-analysis status](#debug-analysis-status-durable-cross-process) below. Credential scoping still happens before the facade runs. PM webhook handling enters provider credentials and PM provider scope before dispatch; GitHub and Sentry use `webhook-execution.ts` / `credential-scope.ts` to inject LLM keys, PM credentials, PM provider scope, and GitHub persona tokens as needed. + +### Debug-analysis status (durable, cross-process) + +Post-mortem debug analysis runs in a separate worker container, so its running/failed lifecycle is tracked in the **durable, cross-process** `debug_analysis_status` table (see the [`debug_analysis_status` table](./09-database.md)) rather than an in-process flag invisible to the dashboard process. Both entry points — the automatic `agent-auto-debug.ts` path above and the manual dashboard "Run Analysis" button — drive the shared `triggerDebugAnalysis()` runner, which marks `running` around the analysis, clears the row on success (a persisted `debug_analyses` row is then the `completed` signal), and marks `failed` on a catchable in-process error. + +`runs.getDebugAnalysisStatus` reads this table **uniformly in queue mode and local dev** with precedence active `running` → `completed` → `failed` → `idle`; a `running` row older than `DEBUG_ANALYSIS_RUNNING_STALE_MS` (2h) self-heals to `idle` so a crashed worker never wedges the run. The deterministic `debug-analysis-` dashboard job (queue mode) provides idempotent re-enqueue and double-trigger dedup — **not** the status signal, since a BullMQ job reaches `completed` at container spawn rather than at analysis completion. An earlier in-memory `Set` (`debug-status.ts`) was removed because it was never visible to the dashboard process (MNG-1667). See [Cross-process debug-analysis status](./01-services.md) for the service-level view. diff --git a/src/triggers/README.md b/src/triggers/README.md index 7c4d7cc7..4c758e1e 100644 --- a/src/triggers/README.md +++ b/src/triggers/README.md @@ -87,9 +87,15 @@ To reduce duplication across the three worker-side handlers, shared utilities ar | `agent-execution-followups.ts` | Recursive follow-up dispatch for post-completion review and splitting auto-chain | `agent-execution.ts` | | `post-completion-review.ts` | Builds the deterministic review dispatch after a successful implementation PR with passing CI | `agent-execution-followups.ts` | | `splitting-auto-chain.ts` | Propagates the auto label after splitting and optionally chains backlog-manager | `agent-execution-followups.ts` | -| `agent-auto-debug.ts` | Triggers configured debug analysis after failed or timed-out runs | `agent-execution.ts` | +| `agent-auto-debug.ts` | Triggers configured debug analysis after failed or timed-out runs; the shared runner records a durable, cross-process status (see [Debug-analysis status](#debug-analysis-status-durable-cross-process)) | `agent-execution.ts` | | `webhook-execution.ts` | `runAgentWithCredentials()` — LLM keys + credentials + pipeline | GitHub, PM | +### Debug-analysis status (durable, cross-process) + +`agent-auto-debug.ts` and the manual dashboard "Run Analysis" button both drive the shared `triggerDebugAnalysis()` runner in `debug-runner.ts`. Because the analysis runs in a separate worker container from the dashboard API that polls its progress, the running/failed lifecycle is recorded in the **durable, cross-process** `debug_analysis_status` table (`debugAnalysisRepository.ts`), not an in-process flag — an in-memory flag in the worker is invisible to the dashboard process. The runner marks `running` around the analysis, clears the row on success (a persisted `debug_analyses` row is then the `completed` signal), and marks `failed` on a catchable in-process error. + +`runs.getDebugAnalysisStatus` reads that table uniformly in queue mode and local dev (precedence: active `running` → `completed` → `failed` → `idle`; a row older than `DEBUG_ANALYSIS_RUNNING_STALE_MS` self-stales to `idle`). The deterministic `debug-analysis-` dashboard job (`debugAnalysisJobId()` in `src/queue/client.ts`) only provides idempotent re-enqueue + double-trigger dedup, since a BullMQ job reaches `completed` at container spawn rather than at analysis completion. An earlier in-memory `Set` (`debug-status.ts`) was removed because it was never visible to the dashboard process (MNG-1667). Full write-up: [`docs/architecture/01-services.md`](../../docs/architecture/01-services.md) and [`docs/architecture/03-trigger-system.md`](../../docs/architecture/03-trigger-system.md). + --- ## Trigger Authoring Contracts From 767dd94892a89cecf31f10ee1f94f2eb3ef71056 Mon Sep 17 00:00:00 2001 From: aaight Date: Thu, 25 Jun 2026 20:26:53 +0200 Subject: [PATCH 6/8] feat(web): persist debug-analysis in-progress state and disable trigger button (MNG-1668) (#1458) Keep the Debug Analysis panel's in-progress affordance and the disabled trigger button visible from the moment the analysis is triggered until a terminal status, instead of flickering back the instant the trigger mutation settles. - Extract pure decision helpers into web/src/lib/debug-analysis.ts (computeDebugAnalysisRunning, debugAnalysisRefetchInterval) so the polling/in-progress logic is node-testable. - Poll the status query from trigger until a terminal status via a polling-active flag (set on trigger success, cleared on completed/failed) in addition to status === 'running'. - isRunning now covers in-flight mutation + just-triggered + queued/running. - Split the component into a data wrapper + presentational DebugAnalysisView so the rendered UX is render-testable; disable both Run / Re-run buttons while running and show a spinner affordance. - Render an error for the failed status and re-enable the button; preserve the existing synchronous trigger-error handling. Co-authored-by: Cascade Bot Co-authored-by: Claude Opus 4.8 --- tests/unit/web/debug-analysis.test.ts | 312 ++++++++++++++++++++ web/src/components/debug/debug-analysis.tsx | 245 ++++++++++----- web/src/lib/debug-analysis.ts | 104 +++++++ 3 files changed, 592 insertions(+), 69 deletions(-) create mode 100644 tests/unit/web/debug-analysis.test.ts create mode 100644 web/src/lib/debug-analysis.ts diff --git a/tests/unit/web/debug-analysis.test.ts b/tests/unit/web/debug-analysis.test.ts new file mode 100644 index 00000000..1ead0f2d --- /dev/null +++ b/tests/unit/web/debug-analysis.test.ts @@ -0,0 +1,312 @@ +import { createElement } from 'react'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { describe, expect, it, vi } from 'vitest'; + +/** + * Tests for the Debug Analysis panel persistent in-progress UX (MNG-1668). + * + * Two layers are covered: + * 1. The pure decision helpers in `web/src/lib/debug-analysis.ts` + * (`computeDebugAnalysisRunning` + `debugAnalysisRefetchInterval`) — these + * encode the "don't flicker the button back" logic and are node-testable. + * 2. The presentational `DebugAnalysisView` rendered to a static HTML string + * via `react-dom/server` (the web suite runs in a node environment with no + * jsdom — mirrors tests/unit/web/run-pending-state.test.ts). + * + * The component module imports `@/lib/trpc.js`, which builds a real tRPC client + * at module load. `DebugAnalysisView` never touches it, so stub the module to + * keep the render hermetic — mirrors the `@/lib/trpc.js` stub in + * tests/unit/web/stats-page.test.ts. `react-markdown` is left real: it resolves + * transitively from web/node_modules (like `lucide-react` does for + * run-pending-state.test.ts) and renders the section text we assert on. + */ + +vi.mock('@/lib/trpc.js', () => ({ + trpc: { + runs: { + getDebugAnalysis: { queryOptions: () => ({ queryKey: [] }) }, + getDebugAnalysisStatus: { queryOptions: () => ({ queryKey: [] }) }, + }, + }, + trpcClient: { runs: { triggerDebugAnalysis: { mutate: () => Promise.resolve() } } }, +})); + +import { + type DebugAnalysisContent, + DebugAnalysisRunningIndicator, + DebugAnalysisView, + type DebugAnalysisViewProps, +} from '../../../web/src/components/debug/debug-analysis.js'; +import { + computeDebugAnalysisRunning, + DEBUG_ANALYSIS_POLL_MS, + type DebugAnalysisStatus, + debugAnalysisRefetchInterval, + isTerminalDebugAnalysisStatus, +} from '../../../web/src/lib/debug-analysis.js'; + +const ALL_STATUSES: (DebugAnalysisStatus | undefined)[] = [ + undefined, + 'idle', + 'running', + 'completed', + 'failed', +]; + +/** Render the presentational view with sensible defaults overridden per test. */ +function renderView(overrides: Partial = {}): string { + const props: DebugAnalysisViewProps = { + status: 'idle', + isRunning: false, + analysis: null, + isTriggerError: false, + triggerError: null, + onTrigger: () => {}, + ...overrides, + }; + return renderToStaticMarkup(createElement(DebugAnalysisView, props)); +} + +/** + * A native `disabled` boolean attribute serializes as `disabled=""`. The button + * className always contains `disabled:opacity-50`, so this exact token (with the + * `="`) is what distinguishes a truly-disabled button from the class utility. + */ +function hasDisabledButton(html: string): boolean { + return html.includes('disabled=""'); +} + +// ─── DEBUG_ANALYSIS_POLL_MS ────────────────────────────────────────────────── + +describe('DEBUG_ANALYSIS_POLL_MS', () => { + it('polls every 5000ms', () => { + expect(DEBUG_ANALYSIS_POLL_MS).toBe(5000); + }); +}); + +// ─── isTerminalDebugAnalysisStatus ─────────────────────────────────────────── + +describe('isTerminalDebugAnalysisStatus', () => { + it('is true for completed', () => { + expect(isTerminalDebugAnalysisStatus('completed')).toBe(true); + }); + + it('is true for failed', () => { + expect(isTerminalDebugAnalysisStatus('failed')).toBe(true); + }); + + it('is false for running', () => { + expect(isTerminalDebugAnalysisStatus('running')).toBe(false); + }); + + it('is false for idle', () => { + expect(isTerminalDebugAnalysisStatus('idle')).toBe(false); + }); + + it('is false for undefined', () => { + expect(isTerminalDebugAnalysisStatus(undefined)).toBe(false); + }); +}); + +// ─── computeDebugAnalysisRunning (AC #2) ───────────────────────────────────── + +describe('computeDebugAnalysisRunning', () => { + const base = { + triggerIsPending: false, + triggerIsSuccess: false, + status: undefined as DebugAnalysisStatus | undefined, + }; + + it('is true while the trigger mutation is in flight', () => { + expect(computeDebugAnalysisRunning({ ...base, triggerIsPending: true })).toBe(true); + }); + + it('is true right after trigger success while status is still undefined (queued gap)', () => { + expect(computeDebugAnalysisRunning({ ...base, triggerIsSuccess: true })).toBe(true); + }); + + it('is true right after trigger success while status still reads idle', () => { + expect(computeDebugAnalysisRunning({ ...base, triggerIsSuccess: true, status: 'idle' })).toBe( + true, + ); + }); + + it('is true while status is running even after the mutation has settled', () => { + expect(computeDebugAnalysisRunning({ ...base, status: 'running' })).toBe(true); + }); + + it('is false once status is completed (even with a lingering isSuccess)', () => { + expect( + computeDebugAnalysisRunning({ ...base, triggerIsSuccess: true, status: 'completed' }), + ).toBe(false); + }); + + it('is false once status is failed so the button re-enables for retry', () => { + expect(computeDebugAnalysisRunning({ ...base, triggerIsSuccess: true, status: 'failed' })).toBe( + false, + ); + }); + + it('is false when idle and nothing is pending or just-succeeded', () => { + expect(computeDebugAnalysisRunning({ ...base, status: 'idle' })).toBe(false); + }); + + it('is false when status is undefined and nothing is pending or just-succeeded', () => { + expect(computeDebugAnalysisRunning(base)).toBe(false); + }); + + it('matches the acceptance-criteria expression for every input combination', () => { + for (const triggerIsPending of [true, false]) { + for (const triggerIsSuccess of [true, false]) { + for (const status of ALL_STATUSES) { + const expected = + triggerIsPending || + (triggerIsSuccess && status !== 'completed' && status !== 'failed') || + status === 'running'; + expect(computeDebugAnalysisRunning({ triggerIsPending, triggerIsSuccess, status })).toBe( + expected, + ); + } + } + } + }); +}); + +// ─── debugAnalysisRefetchInterval (AC #1) ──────────────────────────────────── + +describe('debugAnalysisRefetchInterval', () => { + it('polls while status is running', () => { + expect(debugAnalysisRefetchInterval({ status: 'running', pollingActive: false })).toBe( + DEBUG_ANALYSIS_POLL_MS, + ); + }); + + it('polls while the polling-active flag is set even if status still reads idle', () => { + expect(debugAnalysisRefetchInterval({ status: 'idle', pollingActive: true })).toBe( + DEBUG_ANALYSIS_POLL_MS, + ); + }); + + it('polls while the polling-active flag is set and status is undefined (trigger gap)', () => { + expect(debugAnalysisRefetchInterval({ status: undefined, pollingActive: true })).toBe( + DEBUG_ANALYSIS_POLL_MS, + ); + }); + + it('stops once status is completed even if the polling-active flag has not cleared yet', () => { + expect(debugAnalysisRefetchInterval({ status: 'completed', pollingActive: true })).toBe(false); + }); + + it('stops once status is failed even if the polling-active flag has not cleared yet', () => { + expect(debugAnalysisRefetchInterval({ status: 'failed', pollingActive: true })).toBe(false); + }); + + it('does not poll when idle and the polling-active flag is unset', () => { + expect(debugAnalysisRefetchInterval({ status: 'idle', pollingActive: false })).toBe(false); + }); + + it('does not poll when status is undefined and the polling-active flag is unset', () => { + expect(debugAnalysisRefetchInterval({ status: undefined, pollingActive: false })).toBe(false); + }); +}); + +// ─── DebugAnalysisRunningIndicator ─────────────────────────────────────────── + +describe('DebugAnalysisRunningIndicator', () => { + it('renders a spinner and the multi-minute in-progress copy', () => { + const html = renderToStaticMarkup(createElement(DebugAnalysisRunningIndicator)); + expect(html).toContain('animate-spin'); + expect(html).toContain('Debug analysis is running'); + expect(html).toContain('this can take a few minutes'); + }); +}); + +// ─── DebugAnalysisView — first run, no prior analysis (AC #3, #4, #5) ───────── + +describe('DebugAnalysisView — first run (no prior analysis)', () => { + it('after trigger: shows the in-progress affordance and disables the Run button', () => { + const html = renderView({ status: 'idle', isRunning: true, analysis: null }); + expect(html).toContain('Debug analysis is running'); + expect(html).toContain('animate-spin'); + expect(html).toContain('Run Analysis'); + expect(hasDisabledButton(html)).toBe(true); + // The idle "nothing here yet" copy is replaced by the affordance. + expect(html).not.toContain('No debug analysis available'); + }); + + it('stays in-progress (disabled) while the status reads running', () => { + const html = renderView({ status: 'running', isRunning: true, analysis: null }); + expect(html).toContain('Debug analysis is running'); + expect(hasDisabledButton(html)).toBe(true); + }); + + it('failed: renders an error message and re-enables the Run button for retry', () => { + const html = renderView({ status: 'failed', isRunning: false, analysis: null }); + expect(html).toContain('Debug analysis failed'); + expect(html).toContain('Run Analysis'); + expect(hasDisabledButton(html)).toBe(false); + expect(html).not.toContain('Debug analysis is running'); + }); + + it('idle with no analysis: shows the empty copy and an enabled Run button', () => { + const html = renderView({ status: 'idle', isRunning: false, analysis: null }); + expect(html).toContain('No debug analysis available'); + expect(hasDisabledButton(html)).toBe(false); + expect(html).not.toContain('Debug analysis is running'); + }); + + it('preserves the synchronous trigger error message (CONFLICT / validation)', () => { + const html = renderView({ + status: 'idle', + isRunning: false, + analysis: null, + isTriggerError: true, + triggerError: new Error('Debug analysis is already running for this run'), + }); + expect(html).toContain('Debug analysis is already running for this run'); + }); + + it('falls back to a generic trigger-error message for a non-Error rejection', () => { + const html = renderView({ + status: 'idle', + isRunning: false, + analysis: null, + isTriggerError: true, + triggerError: 'boom', + }); + expect(html).toContain('Failed to trigger analysis'); + }); +}); + +// ─── DebugAnalysisView — existing analysis, re-run (AC #3, #4) ──────────────── + +describe('DebugAnalysisView — existing analysis (re-run)', () => { + const analysis: DebugAnalysisContent = { + severity: 'high', + summary: 'Root cause summary text', + issues: 'The issue list', + }; + + it('disables the Re-run button and shows the affordance while running', () => { + const html = renderView({ status: 'running', isRunning: true, analysis }); + expect(html).toContain('Re-run Analysis'); + expect(hasDisabledButton(html)).toBe(true); + expect(html).toContain('Debug analysis is running'); + // The prior analysis stays visible during a re-run. + expect(html).toContain('Root cause summary text'); + }); + + it('keeps the Re-run button enabled when the analysis is complete', () => { + const html = renderView({ status: 'completed', isRunning: false, analysis }); + expect(html).toContain('Re-run Analysis'); + expect(hasDisabledButton(html)).toBe(false); + expect(html).not.toContain('Debug analysis is running'); + expect(html).toContain('Root cause summary text'); + }); + + it('failed re-run: shows the error and keeps the Re-run button enabled', () => { + const html = renderView({ status: 'failed', isRunning: false, analysis }); + expect(html).toContain('Debug analysis failed'); + expect(hasDisabledButton(html)).toBe(false); + }); +}); diff --git a/web/src/components/debug/debug-analysis.tsx b/web/src/components/debug/debug-analysis.tsx index 89dfcdda..a33ba71e 100644 --- a/web/src/components/debug/debug-analysis.tsx +++ b/web/src/components/debug/debug-analysis.tsx @@ -1,13 +1,30 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; -import { useEffect, useRef } from 'react'; +import { Loader2 } from 'lucide-react'; +import { useEffect, useRef, useState } from 'react'; import ReactMarkdown from 'react-markdown'; import { Button } from '@/components/ui/button.js'; +import { + computeDebugAnalysisRunning, + type DebugAnalysisStatus, + debugAnalysisRefetchInterval, + isTerminalDebugAnalysisStatus, +} from '@/lib/debug-analysis.js'; import { trpc, trpcClient } from '@/lib/trpc.js'; interface DebugAnalysisProps { runId: string; } +/** Minimal shape of the persisted debug-analysis content the panel renders. */ +export interface DebugAnalysisContent { + severity?: string | null; + summary?: string | null; + issues?: string | null; + rootCause?: string | null; + timeline?: string | null; + recommendations?: string | null; +} + function Section({ title, content }: { title: string; content: string | null | undefined }) { if (!content) return null; return ( @@ -20,72 +37,98 @@ function Section({ title, content }: { title: string; content: string | null | u ); } -export function DebugAnalysis({ runId }: DebugAnalysisProps) { - const queryClient = useQueryClient(); - const prevStatusRef = useRef(undefined); - - const analysisQuery = useQuery(trpc.runs.getDebugAnalysis.queryOptions({ runId })); - - const statusQuery = useQuery({ - ...trpc.runs.getDebugAnalysisStatus.queryOptions({ runId }), - refetchInterval: (query) => { - const status = query.state.data?.status; - return status === 'running' ? 5000 : false; - }, - }); - - const triggerMutation = useMutation({ - mutationFn: () => trpcClient.runs.triggerDebugAnalysis.mutate({ runId }), - onSuccess: () => { - queryClient.invalidateQueries({ - queryKey: trpc.runs.getDebugAnalysisStatus.queryOptions({ runId }).queryKey, - }); - }, - }); - - // When status transitions from running → completed, refetch the analysis - useEffect(() => { - const currentStatus = statusQuery.data?.status; - if (prevStatusRef.current === 'running' && currentStatus === 'completed') { - queryClient.invalidateQueries({ - queryKey: trpc.runs.getDebugAnalysis.queryOptions({ runId }).queryKey, - }); - } - prevStatusRef.current = currentStatus; - }, [statusQuery.data?.status, queryClient, runId]); - - const isRunning = triggerMutation.isPending || statusQuery.data?.status === 'running'; +/** + * In-progress affordance shown while a debug analysis is running. Persists from + * the moment the trigger is accepted until a terminal status, so the user always + * has feedback that the (multi-minute) analysis is under way. Mirrors the + * `RunPendingState` spinner styling. + */ +export function DebugAnalysisRunningIndicator() { + return ( +
+
+ ); +} - if (analysisQuery.isLoading || statusQuery.isLoading) { - return
Loading analysis...
; - } +/** Failed-status + synchronous-trigger-error messaging, shared by both views. */ +function DebugAnalysisErrors({ + status, + isTriggerError, + triggerError, +}: { + status: DebugAnalysisStatus | undefined; + isTriggerError: boolean; + triggerError: unknown; +}) { + return ( + <> + {status === 'failed' && ( +

+ Debug analysis failed. Please try running it again. +

+ )} + {isTriggerError && ( +

+ {triggerError instanceof Error ? triggerError.message : 'Failed to trigger analysis'} +

+ )} + + ); +} - if (isRunning) { - return ( -
Debug analysis is running...
- ); - } +export interface DebugAnalysisViewProps { + /** Latest analysis status; `undefined` before the first status read. */ + status: DebugAnalysisStatus | undefined; + /** Whether the panel should show the in-progress affordance + disable buttons. */ + isRunning: boolean; + /** The persisted analysis content row, or `null` when none exists yet. */ + analysis: DebugAnalysisContent | null; + /** Whether the trigger mutation settled in an error state. */ + isTriggerError: boolean; + /** Synchronous trigger error (CONFLICT / validation), or `null`. */ + triggerError: unknown; + /** Invoked when the user presses Run / Re-run. */ + onTrigger: () => void; +} - if (!analysisQuery.data) { +/** + * Presentational body of the Debug Analysis panel. Pure (no hooks) so it can be + * rendered to static markup in the node-only web test suite — all derived state + * is computed by the `DebugAnalysis` data wrapper and passed in as props. + */ +export function DebugAnalysisView({ + status, + isRunning, + analysis, + isTriggerError, + triggerError, + onTrigger, +}: DebugAnalysisViewProps) { + if (!analysis) { return (
-

No debug analysis available for this run

- - {triggerMutation.isError && ( -

- {triggerMutation.error instanceof Error - ? triggerMutation.error.message - : 'Failed to trigger analysis'} -

- )} +
); } - const analysis = analysisQuery.data; - return (
@@ -97,22 +140,16 @@ export function DebugAnalysis({ runId }: DebugAnalysisProps) { )}
-
- {triggerMutation.isError && ( -

- {triggerMutation.error instanceof Error - ? triggerMutation.error.message - : 'Failed to trigger analysis'} -

- )} + {isRunning && } +
@@ -121,3 +158,73 @@ export function DebugAnalysis({ runId }: DebugAnalysisProps) { ); } + +export function DebugAnalysis({ runId }: DebugAnalysisProps) { + const queryClient = useQueryClient(); + const prevStatusRef = useRef(undefined); + // Polling stays active from trigger acceptance until a terminal status, even + // across the brief window where the status row can still read `idle` before + // the worker marks it `running`. + const [pollingActive, setPollingActive] = useState(false); + + const analysisQuery = useQuery(trpc.runs.getDebugAnalysis.queryOptions({ runId })); + + const statusQuery = useQuery({ + ...trpc.runs.getDebugAnalysisStatus.queryOptions({ runId }), + refetchInterval: (query) => + debugAnalysisRefetchInterval({ status: query.state.data?.status, pollingActive }), + }); + + const status = statusQuery.data?.status; + + const triggerMutation = useMutation({ + mutationFn: () => trpcClient.runs.triggerDebugAnalysis.mutate({ runId }), + onSuccess: () => { + // Keep the in-progress affordance and polling alive from the instant the + // trigger is accepted, not just while the status row reads `running`. + setPollingActive(true); + queryClient.invalidateQueries({ + queryKey: trpc.runs.getDebugAnalysisStatus.queryOptions({ runId }).queryKey, + }); + }, + }); + + // When status transitions from running → completed, refetch the analysis. + useEffect(() => { + if (prevStatusRef.current === 'running' && status === 'completed') { + queryClient.invalidateQueries({ + queryKey: trpc.runs.getDebugAnalysis.queryOptions({ runId }).queryKey, + }); + } + prevStatusRef.current = status; + }, [status, queryClient, runId]); + + // Clear the polling-active flag once the analysis reaches a terminal status so + // polling stops and the buttons re-enable (failed) / settle (completed). + useEffect(() => { + if (isTerminalDebugAnalysisStatus(status)) { + setPollingActive(false); + } + }, [status]); + + const isRunning = computeDebugAnalysisRunning({ + triggerIsPending: triggerMutation.isPending, + triggerIsSuccess: triggerMutation.isSuccess, + status, + }); + + if (analysisQuery.isLoading || statusQuery.isLoading) { + return
Loading analysis...
; + } + + return ( + triggerMutation.mutate()} + /> + ); +} diff --git a/web/src/lib/debug-analysis.ts b/web/src/lib/debug-analysis.ts new file mode 100644 index 00000000..5a846d04 --- /dev/null +++ b/web/src/lib/debug-analysis.ts @@ -0,0 +1,104 @@ +/** + * Pure decision helpers for the Debug Analysis panel + * (`web/src/components/debug/debug-analysis.tsx`). + * + * The panel triggers a debug analysis (a separate worker run) and then polls + * `trpc.runs.getDebugAnalysisStatus` until the analysis reaches a terminal + * status. To avoid the "button flickers back the instant the trigger mutation + * settles" bug (MNG-1668), both the poll cadence and the in-progress flag must + * stay alive across the gap between "mutation accepted" and "status row observed + * as running" — and must keep polling until a terminal status is seen. + * + * All branching logic lives here — outside React — because the web test suite + * runs in a node environment with no jsdom, so the logic must be node-testable + * (mirrors the `run-pending.ts` pattern). This module has no React imports and + * no side effects. + */ + +/** Poll interval (ms) for the debug-analysis status query while in progress. */ +export const DEBUG_ANALYSIS_POLL_MS = 5000; + +/** + * Lifecycle status of a run's debug analysis, as reported by + * `trpc.runs.getDebugAnalysisStatus`. Mirrors the backend union in + * `src/api/routers/runs.ts` (`getDebugAnalysisStatus`). + */ +export type DebugAnalysisStatus = 'idle' | 'running' | 'completed' | 'failed'; + +/** + * True once the analysis has reached a terminal status (`completed` or + * `failed`). Terminal statuses stop polling and clear the polling-active flag. + */ +export function isTerminalDebugAnalysisStatus(status: DebugAnalysisStatus | undefined): boolean { + return status === 'completed' || status === 'failed'; +} + +export interface DebugAnalysisRunningInput { + /** `triggerMutation.isPending` — the trigger request is in flight. */ + triggerIsPending: boolean; + /** `triggerMutation.isSuccess` — the trigger request settled successfully. */ + triggerIsSuccess: boolean; + /** Latest status from the status query (`undefined` before the first read). */ + status: DebugAnalysisStatus | undefined; +} + +/** + * Whether the panel should treat the analysis as actively in progress. + * + * Covers three overlapping windows so the in-progress affordance never flickers + * back between "mutation settled" and "status row observed as running": + * - the trigger request is still in flight (`isPending`); + * - the trigger just succeeded and the status has not yet reached a terminal + * value (`isSuccess && !terminal`) — bridges the enqueue→running gap where + * the status row can briefly still read `idle`; + * - the status query reports `running`. + * + * Equivalent to the acceptance-criteria expression: + * `isPending || (isSuccess && status !== 'completed' && status !== 'failed') || status === 'running'`. + */ +export function computeDebugAnalysisRunning({ + triggerIsPending, + triggerIsSuccess, + status, +}: DebugAnalysisRunningInput): boolean { + if (triggerIsPending) { + return true; + } + if (triggerIsSuccess && !isTerminalDebugAnalysisStatus(status)) { + return true; + } + return status === 'running'; +} + +export interface DebugAnalysisRefetchInput { + /** Latest status from the status query. */ + status: DebugAnalysisStatus | undefined; + /** + * Polling-active flag set on `triggerMutation` success and cleared once the + * status reaches a terminal value. Keeps polling alive across the + * trigger→running gap where the status row may still read `idle`. + */ + pollingActive: boolean; +} + +/** + * Refetch cadence for the status query: + * - `false` once the status is terminal (`completed`/`failed`) — stop polling. + * - `DEBUG_ANALYSIS_POLL_MS` while `running` or the polling-active flag is set. + * - `false` otherwise (idle and nothing pending). + * + * The terminal check is deliberately first so a stale `pollingActive === true` + * (the clearing effect has not run yet) cannot keep polling a finished analysis. + */ +export function debugAnalysisRefetchInterval({ + status, + pollingActive, +}: DebugAnalysisRefetchInput): number | false { + if (isTerminalDebugAnalysisStatus(status)) { + return false; + } + if (status === 'running' || pollingActive) { + return DEBUG_ANALYSIS_POLL_MS; + } + return false; +} From 38f052ac0c7645876b3d9bd8818bda9d8fc7f7f4 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Fri, 26 Jun 2026 09:50:04 +0200 Subject: [PATCH 7/8] fix(web): remove @theme inline so dark mode CSS tokens propagate correctly (#1459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tailwind v4's `@theme inline` bakes token values directly into utility classes (e.g. bg-card → background-color: oklch(1 0 0)) instead of emitting CSS custom properties. The .dark { --color-card: ... } overrides had no effect on those utilities, leaving cards, tab bars, and other token-backed components stuck at their light-mode colours in dark mode. Removing `inline` makes Tailwind emit :root CSS custom properties so the existing .dark { } block takes effect for every token-based utility across the dashboard — bg-card, bg-muted, bg-input, bg-sidebar, border-border, etc. Co-authored-by: Claude Sonnet 4.6 --- web/src/index.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/index.css b/web/src/index.css index e9c5b957..cf985afd 100644 --- a/web/src/index.css +++ b/web/src/index.css @@ -4,7 +4,7 @@ @custom-variant dark (&:is(.dark *)); -@theme inline { +@theme { --color-background: oklch(1 0 0); --color-foreground: oklch(0.145 0 0); --color-card: oklch(1 0 0); From e67e8dcdb76cec22f7587778659d0afa9a28f705 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Fri, 26 Jun 2026 12:06:10 +0200 Subject: [PATCH 8/8] fix(worker): disable setup.sh idle timeout, bump to Node 24, scope agent lint to changed files (#1460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(worker): disable setup.sh idle timeout, bump to Node 24, scope agent lint to changed files Three related fixes after a worker run where .cascade/setup.sh was killed mid-Ruby-compilation and the agent then stalled on a full-tree eslint:fix: 1. setup.sh idle timeout: ruby-build suppresses make output by design, causing the 120s idle timeout to fire during compilation. Disable the idle timeout for setup.sh; the wall timeout (10min) remains the safety net for truly hung scripts. Also surface `reason` in the log so wall-timeout kills are distinguishable from idle-timeout kills. 2. Worker image Node 24: bump builder and production stages from node:22 to node:24, eliminating the EBADENGINE mismatch for projects that require Node >=24 (and reducing what setup.sh needs to install via asdf). 3. implementation.eta step 7: replace vague "run linting" instruction with an explicit changed-files-only lint pattern. Full-tree eslint --fix can take 10+ minutes on large TypeScript codebases and kills sessions. Also add a kill-on-timeout instruction to tmux.eta partial. Co-Authored-By: Claude Sonnet 4.6 * fix(prompts): include untracked files in scoped agent lint Review feedback on PR #1460: the scoped-lint guidance in step 7 still relies on `git diff --name-only HEAD`, which lists only tracked changes. At step 7 the agent has not committed yet (commit happens inside CreatePR at step 8) and nothing was `git add`ed, so brand-new source/test files are untracked and get excluded from the lint scope. Local lint then passes while the full-project `lint-and-test` CI job fails on exactly those new files — the opposite of this step's intent. Pick up untracked files via `git ls-files --others --exclude-standard`, dedupe with `sort -u`, and use `xargs -r` so the linter is not invoked with zero args when nothing matches. Note the same treatment applies to the Biome/other-linter variant. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Sonnet 4.6 Co-authored-by: Cascade Bot --- Dockerfile.worker | 4 ++-- .../prompts/templates/implementation.eta | 9 ++++++++- .../prompts/templates/partials/tmux.eta | 2 ++ src/agents/shared/repository.ts | 19 +++++++++++++++---- tests/unit/agents/shared/repository.test.ts | 1 + 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/Dockerfile.worker b/Dockerfile.worker index 39e32cec..0ce46bde 100644 --- a/Dockerfile.worker +++ b/Dockerfile.worker @@ -1,4 +1,4 @@ -FROM node:22-slim AS builder +FROM node:24-slim AS builder WORKDIR /app # Install dependencies (including dev for build) @@ -12,7 +12,7 @@ COPY src ./src RUN npm run build # Production image -FROM node:22-bookworm AS production +FROM node:24-bookworm AS production WORKDIR /app # `cascade.managed=true` is the contract the router's dangling-image cleanup diff --git a/src/agents/prompts/templates/implementation.eta b/src/agents/prompts/templates/implementation.eta index a3b59160..aeeab7e9 100644 --- a/src/agents/prompts/templates/implementation.eta +++ b/src/agents/prompts/templates/implementation.eta @@ -25,7 +25,14 @@ You are an expert software engineer implementing features and fixing issues base ### Phase 4: Verify & Ship -7. **Run linting** (with fixing first, then without) **and type checking** +7. **Run type checking and lint changed files only** + - Type check: `npm run typecheck` (or `npx tsc --noEmit`) + - Lint: run the linter directly on changed files — **never** `npm run lint` / `npm run eslint:fix` unscoped; full-tree lint can take 10+ min on large codebases. Scope to changed **and newly-created (untracked)** files: you have not committed yet (commit happens inside `CreatePR` at step 8), so new modules/test files are untracked and `git diff --name-only HEAD` alone would skip them — pick them up with `git ls-files --others --exclude-standard`: + ``` + { git diff --name-only HEAD; git ls-files --others --exclude-standard; } \ + | grep -E '\.(ts|tsx|js|jsx)$' | sort -u | xargs -r npx eslint --fix --max-warnings 0 + ``` + Swap `eslint --fix --max-warnings 0` for `biome check --write` (or your project's linter) as appropriate; `xargs -r` skips the linter entirely when nothing matches. 8. **Create a PR** using the `CreatePR` gadget (it handles commit, push, and PR creation atomically) - **FORBIDDEN**: Do NOT run `gh pr create` or `git push` via Tmux — they will FAIL - IMPORTANT: DO NOT PROCEED FURTHER UNTIL YOU HAVE CONFIRMED the CreatePR output shows a `prUrl`. diff --git a/src/agents/prompts/templates/partials/tmux.eta b/src/agents/prompts/templates/partials/tmux.eta index a2286c1f..191b09f7 100644 --- a/src/agents/prompts/templates/partials/tmux.eta +++ b/src/agents/prompts/templates/partials/tmux.eta @@ -15,6 +15,8 @@ Use the Tmux gadget for ALL shell commands: **Avoid interactive/watch modes:** Commands run inside Tmux have a TTY, so tools may default to interactive or watch mode. Always pass flags to force non-interactive, single-run execution (e.g., `--run`, `--watchAll=false`, `--no-watch`, `--ci`, or similar). If a session hangs with `status=running` after your command should have finished, it is likely in watch/interactive mode — kill it and retry with the correct flag. +**Do NOT poll indefinitely.** If a command runs much longer than expected and a more targeted scope is possible (e.g. lint only changed files instead of the full project), kill it and retry with narrower scope. + **Command Format:** Pass command as a shell string. Pipes, &&, ||, redirects, and globs all work: - Simple: `command="npm test"` - Chained: `command="npm run lint && npm test"` diff --git a/src/agents/shared/repository.ts b/src/agents/shared/repository.ts index 01e10c2e..65739853 100644 --- a/src/agents/shared/repository.ts +++ b/src/agents/shared/repository.ts @@ -238,16 +238,27 @@ export async function setupRepository(options: SetupRepositoryOptions): Promise< const setupScriptPath = join(repoDir, '.cascade', 'setup.sh'); if (existsSync(setupScriptPath)) { log.info('Running project setup script', { path: '.cascade/setup.sh', agentType }); - const setupResult = await runCommand('bash', [setupScriptPath], repoDir, { - AGENT_PROFILE_NAME: agentType, - }); + const setupResult = await runCommand( + 'bash', + [setupScriptPath], + repoDir, + { AGENT_PROFILE_NAME: agentType }, + // Disable idle timeout: setup.sh may compile language runtimes (e.g. Ruby via + // asdf/ruby-build) whose make output is suppressed, causing false idle-timeout + // kills. The wall timeout (10 min) remains the safety net for truly hung setups. + { idleTimeoutMs: 0 }, + ); log.info('Setup script completed', { exitCode: setupResult.exitCode, + reason: setupResult.reason, stdout: setupResult.stdout.slice(-500), stderr: setupResult.stderr.slice(-500), }); if (setupResult.exitCode !== 0) { - log.warn('Setup script exited with non-zero code', { exitCode: setupResult.exitCode }); + log.warn('Setup script exited with non-zero code', { + exitCode: setupResult.exitCode, + reason: setupResult.reason, + }); } } diff --git a/tests/unit/agents/shared/repository.test.ts b/tests/unit/agents/shared/repository.test.ts index 52b39c23..d9fa1acd 100644 --- a/tests/unit/agents/shared/repository.test.ts +++ b/tests/unit/agents/shared/repository.test.ts @@ -467,6 +467,7 @@ describe('setupRepository', () => { ['/tmp/cascade-test-project-12345/.cascade/setup.sh'], '/tmp/cascade-test-project-12345', { AGENT_PROFILE_NAME: 'coder' }, + { idleTimeoutMs: 0 }, ); });