feat(dashboard): surface session-ended-before-Stop and Stop-hook-timed-out in Activity#250
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/review |
PR Reviewer Guide 🔍(Review updated until commit 670da7c)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit 99850be |
PR Code Suggestions ✨No code suggestions found for the PR. |
…d-out in Activity Add `end_reason TEXT` to the `workflows` table (migration 012) and wire it end-to-end: - Dispatcher writes `"session-ended-before-Stop"` / `"Stop-hook-timed-out"` via `updateWorkflow` just before throwing in both `recommender.ts` (inline, `ctx` in scope) and `implementation.ts` (`awaitNextStop` / `resolveBareStop` / `enforceVerifyOnDone` accept an optional `workflowId` arg so the write is conditional and test-safe). - `workflow-record.ts` gains `endReason?: string` in `WorkflowPatch` + `PATCH_COLUMNS`. - `db-deps.ts` `listRuns()` projects `end_reason` → `endReason` on `RunSummary`. - `wire.ts` `RunSummary` adds `endReason: string | null` with TSDoc. - `Activity.tsx` `RunRow` renders a neutral `Badge` with an HTML `title` tooltip for the two known reasons; unknown tokens show the raw value; null → no chip. Tests: 4 new SSR unit tests in `activity.test.tsx` (label + tooltip presence, null omission, unknown-reason fallback) and 1 new DB-level projection test in `runs-deps.test.ts`. All 128 dashboard tests + all dispatcher workflow tests pass. Closes #242
…2, bump version assertions
Migration 012 hand-inserted its own `schema_version` row, but migrations
003–011 don't — the runner records the version via `INSERT OR IGNORE` (db.ts).
Removed the redundant insert to match the convention.
Bumping the schema to 12 broke full-chain latest-version assertions in two
packages that targeted dashboard/dispatcher-workflow runs missed:
- packages/dispatcher/test/db.test.ts — 7 full-chain `toBe(11)` → `toBe(12)`
(the partial through002 → 2 and through010 → 10 assertions are unchanged).
- packages/cli/test/db-scripts.test.ts — restore-roundtrip `toBe(11)` → `toBe(12)`.
The through010 partial-migration test filtered migrations by `!startsWith("011_")`,
which let 012 slip into the v10 fixture dir — widened it to copy only NNN ≤ 010
so the "pre-index world" stays at v10 regardless of later migrations.
Full `bun test`: 1489 pass, 0 fail. typecheck + lint clean.
6319048 to
670da7c
Compare
|
Persistent review updated to latest commit 670da7c |
PR Code Suggestions ✨No code suggestions found for the PR. |
User description
Summary
Closes #242
Surface the two specific abnormal-termination reasons (
session-ended-before-Stop,Stop-hook-timed-out) in the Activity view's run-history rows with human-readable labels and tooltips. Follow-up from PR #233, which explicitly punted this UI work as a follow-up (see PR #233 "Punted as follow-up" section).Status
What changed
packages/dispatcher/src/db/migrations/012_workflows_end_reason.sql— newend_reason TEXTcolumn onworkflowspackages/dispatcher/src/workflow-record.ts—endReason?: stringinWorkflowPatch+PATCH_COLUMNSpackages/dispatcher/src/workflows/recommender.ts— writes"session-ended-before-Stop"/"Stop-hook-timed-out"viaupdateWorkflowbefore throwingpackages/dispatcher/src/workflows/implementation.ts— same, threaded via optionalworkflowIdarg throughawaitNextStop/resolveBareStop/enforceVerifyOnDonepackages/dashboard/src/wire.ts—endReason: string | nullonRunSummarypackages/dashboard/src/db-deps.ts—listRuns()projectsend_reason→endReasonpackages/dashboard/src/app/components/Activity.tsx—RunRowrenders a neutral Badge +titletooltip; null → no chip; unknown tokens show the raw valuepackages/dashboard/test/activity.test.tsx— 4 new SSR unit testspackages/dashboard/test/runs-deps.test.ts— 1 new DB-level projection testWhy these changes
The
statecolumn only conveyscompensated/failed— it doesn't tell the operator why the run failed. The two reasons surfaced here (session-ended-before-StopandStop-hook-timed-out) are the most actionable: one indicates a crashed/killed tmux session before the Stop hook fired; the other indicates the Stop hook exceeded its configured timeout. Both were already thrown as error messages but never persisted in the DB. Adding a dedicatedend_reasoncolumn (notmeta_json— the docs call that "scratch") lets the dashboard project it cleanly.The reason is written before throwing so the saga's compensation (
compensatedstate write) doesn't clobber it — both writes go to different columns.Verification
Full-suite output (
bun test)Migration 012 bumps the schema version 11→12, which touches full-chain latest-version assertions in
db.test.tsanddb-scripts.test.ts. Those are updated (partial-migration assertionsthrough002→2 /through010→10 left intact, and thethrough010fixture filter widened to copy only migrations ≤ 010 so the v10 "pre-index world" holds). The full suite is green:bun run typecheck→ clean (tsc --noEmit, no output).bun run lint→ clean (oxlint --fix --deny-warnings, no output).Targeted suites (subset of the above)
Acceptance criteria
session-ended-before-Stoprenders "Session ended before Stop" with tooltip "the agent session closed before the Stop hook fired" — covered byactivity.test.tsx: "renders human label and tooltip for session-ended-before-Stop" (feat(dashboard): surface session-ended-before-Stop and Stop-hook-timed-out reasons in recommender run-history UI #242)Stop-hook-timed-outrenders "Stop hook timed out" with tooltip "the Stop hook did not respond within the configured timeout" — covered byactivity.test.tsx: "renders human label and tooltip for Stop-hook-timed-out" (feat(dashboard): surface session-ended-before-Stop and Stop-hook-timed-out reasons in recommender run-history UI #242)packages/dashboard/test/activity.test.tsx(4 new tests, runs underbun test) (feat(dashboard): surface session-ended-before-Stop and Stop-hook-timed-out reasons in recommender run-history UI #242)bun run typecheckandbun run lintclean — verified above (feat(dashboard): surface session-ended-before-Stop and Stop-hook-timed-out reasons in recommender run-history UI #242)Stumbling points
session-ended-before-Stop) while the existing error message strings in the code use spaces ("session ended before Stop hook"). Confirmed the hyphen-separated tokens are the correct canonical form for the DB column — they match the issue title and are machine-readable.end_reasoncolumn (the issue says reasons "appear in the raw run data" — they actually only existed as thrown error messages). Added a migration.Tooltipcomponent is vendored inapp/components/ui/. Usedtitleattribute on the Badge — works in SSR, accessible, correct for this use case.Suggested CLAUDE.md updates
None — the tooltip convention (use
titlewhen no Radix Tooltip is vendored) and themeta_jsonvs. dedicated-column distinction are already derivable from reading the existing code. The dashboard CLAUDE.md already tells agents to check what's vendored.Decisions
See
planning/issues/242/decisions.mdfor the full rationale on: reason token format,titlevs. Radix Tooltip,end_reasonas a column vs.meta_json, and theworkflowIdoptional-arg pattern.PR Type
Bug fix, Enhancement
Description
Add
end_reasoncolumn to workflows (migration 012) and write reason tokens before throwing in recommender and implementation workflowsRace the recommender's Stop wait against session liveness via
awaitStopOrSessionEnd, so dead sessions fail fast instead of stallingSurface
session-ended-before-StopandStop-hook-timed-outas human-readable badges with tooltips in the Activity dashboardBump schema version to 12 and update assertions across dispatcher and CLI tests
Document design decisions and implementation plan for the end‑reason feature
Diagram Walkthrough
File Walkthrough
3 files
Add `end_reason` column to workflows tableUpdate schema version assertions to 12Bump expected schema version in db-scripts test2 files
Race session liveness and write end_reason before throwThread `workflowId` to write end_reason on abnormal Stop4 files
Add `endReason` to WorkflowPatch and PATCH_COLUMNSRender end‑reason badges with tooltips in run rowsProject `end_reason` from DB into RunSummaryAdd `endReason` field to RunSummary wire type2 files
Add tests for known, null, and unknown end‑reason renderingTest endReason projection for normal and abnormal runs2 files
Document design decisions for end‑reason tokens and renderingAdd implementation plan for issue 242