refactor(shared): single TaskStatus enum replaces dual db-status / ui_status split #1183

Merged

charles merged 1 commit from dev/1163 into main 2026-05-14 23:23:13 +00:00

Conversation 2 Commits 1 Files changed 15 +118 -95

dev commented 2026-05-14 22:27:40 +00:00

Collaborator

Copy link

Summary

• Introduces packages/shared/src/task-status.ts with the canonical TaskStatus union (all DB + in-memory values including aborted_no_skill), DisplayStatus (UI display states, same values as the old TaskUiStatus), and toDisplayStatus(s: TaskStatus): DisplayStatus — a simple, exhaustive mapping function.
• Removes ui_status and ui_status_reason from the /history API wire format; components now call toDisplayStatus(task.status) directly.
• HistoryListEntry loses both fields; fallbackUiStatus helper removed.
• task_history.status Drizzle column typed with .$type<TaskStatus>() to align schema to the shared union.
• StatusPill and board.ts updated to reference DisplayStatus instead of TaskUiStatus; TaskUiStatus is kept as a @deprecated alias in task-ui-status.ts for backward compat.

Closes #1163

Test plan

• just qa passes (typecheck + lint + format + all tests)
• History contract test updated — ui_status / ui_status_reason removed from EXPECTED_KEYS
• Board card ui_status (from board API, not history API) intentionally unchanged

## Summary - Introduces `packages/shared/src/task-status.ts` with the canonical `TaskStatus` union (all DB + in-memory values including `aborted_no_skill`), `DisplayStatus` (UI display states, same values as the old `TaskUiStatus`), and `toDisplayStatus(s: TaskStatus): DisplayStatus` — a simple, exhaustive mapping function. - Removes `ui_status` and `ui_status_reason` from the `/history` API wire format; components now call `toDisplayStatus(task.status)` directly. - `HistoryListEntry` loses both fields; `fallbackUiStatus` helper removed. - `task_history.status` Drizzle column typed with `.$type<TaskStatus>()` to align schema to the shared union. - `StatusPill` and `board.ts` updated to reference `DisplayStatus` instead of `TaskUiStatus`; `TaskUiStatus` is kept as a `@deprecated` alias in `task-ui-status.ts` for backward compat. Closes #1163 ## Test plan - [x] `just qa` passes (typecheck + lint + format + all tests) - [x] History contract test updated — `ui_status` / `ui_status_reason` removed from `EXPECTED_KEYS` - [x] Board card `ui_status` (from board API, not history API) intentionally unchanged

dev added the

type:chore

area:dashboard

labels 2026-05-14 22:27:40 +00:00

dev added 1 commit 2026-05-14 22:27:40 +00:00

refactor(shared): single TaskStatus enum replaces dual db-status / ui_status split ... f1d7c47a1f

Introduces `packages/shared/src/task-status.ts` with the canonical
`TaskStatus` union (all DB + in-memory values), `DisplayStatus` (UI
display states), and `toDisplayStatus(s: TaskStatus): DisplayStatus`
mapping function.

- `TaskStatus` moved from `task-record.ts` to `task-status.ts`; the
  record file now imports it from the new module.
- `TaskUiStatus` in `task-ui-status.ts` is now an alias for
  `DisplayStatus`; `deriveTaskUiStatus`/`deriveTaskUiStatusReason` kept
  with `@deprecated` tags for board-card backward compat.
- Drizzle `task_history.status` column typed with `.$type<TaskStatus>()`
  so the schema aligns to the shared union.
- `handleHistory` no longer computes or emits `ui_status` /
  `ui_status_reason`; history-contract test updated to match.
- All web components (`TaskList`, `TaskDetail`, `FleetCard`,
  `StatusPill`, `BoardCard`) migrated from `task.ui_status ??
  fallbackUiStatus(status)` to `toDisplayStatus(task.status)`.
- `HistoryListEntry` loses `ui_status` and `ui_status_reason` fields;
  `fallbackUiStatus` helper removed.
- `StatusPill` and `board.ts` updated to reference `DisplayStatus`
  instead of `TaskUiStatus`.

Closes #1163

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer 2026-05-14 22:32:21 +00:00

reviewer requested changes 2026-05-14 22:34:19 +00:00

Dismissed

reviewer left a comment

Copy link

Code Review — refactor(shared): single TaskStatus enum replaces dual db-status / ui_status split

CI: ✅ green. Architecture is sound and the refactor is well-executed overall. Two issues need to be fixed before merge.

---

✅ What's good

• Clean module boundary — TaskStatus (DB/runtime) and DisplayStatus (UI) live together in task-status.ts with a single exhaustive conversion function. Conceptually correct.
• Exhaustive switch without default — TypeScript will catch any future TaskStatus additions that aren't mapped. The right pattern.
• aborted_no_skill properly included — it was missing from the old TaskStatus in task-record.ts. Now canonically modelled.
• fallbackUiStatus removal is safe — with HistoryListEntry.status now typed as TaskStatus and toDisplayStatus replacing the derivation, the client-side fallback is correctly eliminated.
• Backward-compat alias — TaskUiStatus = DisplayStatus as a @deprecated re-export is the right way to handle this without breaking other consumers.

---

🔴 Issues requiring changes

1. Dead code in live-runs-board.tsx — reviewing branch is unreachable

File: apps/web/src/features/agents/live-runs-board.tsx, taskColumn function

// After this PR, toDisplayStatus("running") always returns "executing".
// The || uiStatus === "reviewing" branch can never be true.
if (uiStatus === "executing" || uiStatus === "reviewing") return "working";

toDisplayStatus is documented to never produce "reviewing" from TaskStatus alone. Since HistoryListEntry.ui_status has been removed, uiStatus here is always the result of toDisplayStatus(task.status ?? "running"), meaning "reviewing" is unreachable dead code. This is misleading and should be removed:

if (uiStatus === "executing") return "working";

Same reasoning applies to any other condition in this file that checks uiStatus === "reviewing" or uiStatus === "waiting" — audit and trim them.

---

2. ui_status_reason hover tooltip silently dropped from history/fleet views

Files: task-detail.tsx, task-list.tsx, live-runs-board.tsx (three <StatusPill> callsites)

// Before
<StatusPill status={uiStatus} reason={task.ui_status_reason} />

// After
<StatusPill status={uiStatus} />

The reason prop is gone from all three callsites because ui_status_reason has been removed from HistoryListEntry. For active tasks the reason strings ("Awaiting approval to write .env", etc.) came from the server via deriveTaskUiStatusReason. Removing them silently is a user-visible regression — the hover tooltip that tells an operator why a task is in the waiting state disappears.

The board card API is intentionally unchanged (noted in the PR description), but the fleet view (live-runs-board.tsx) uses fetchHistory(), not the board endpoint. Operators watching a running task from the fleet/history views lose the context tooltip entirely.

Options:

• Restore ui_status_reason on HistoryListEntry (server still has the data via deriveTaskUiStatusReason) — keep it as a standalone field independent of ui_status.
• Accept the regression intentionally — document in the PR that reason tooltips are now board-card-only, and open a follow-up issue to restore them.

Either is fine, but the current state silently removes a feature without acknowledgement in the PR body.

---

ℹ️ Minor observations (no change required)

• deriveTaskUiStatus deprecated but still in use server-side — the board endpoint (not in this diff) presumably still calls it. Deprecating without a migration path is a bit confusing; a follow-up to move the rich derivation to use toDisplayStatus as the base would clean this up.
• tasks.ts import ordering — import type { TaskStatus } is inserted before the drizzle import. Biome will normalise this; no issue.

---

Summary: Fix the dead reviewing branch in taskColumn, and either restore ui_status_reason on HistoryListEntry or explicitly acknowledge its removal. Everything else is clean.

## Code Review — `refactor(shared): single TaskStatus enum replaces dual db-status / ui_status split` CI: ✅ green. Architecture is sound and the refactor is well-executed overall. Two issues need to be fixed before merge. --- ### ✅ What's good - **Clean module boundary** — `TaskStatus` (DB/runtime) and `DisplayStatus` (UI) live together in `task-status.ts` with a single exhaustive conversion function. Conceptually correct. - **Exhaustive switch without `default`** — TypeScript will catch any future `TaskStatus` additions that aren't mapped. The right pattern. - **`aborted_no_skill` properly included** — it was missing from the old `TaskStatus` in `task-record.ts`. Now canonically modelled. - **`fallbackUiStatus` removal is safe** — with `HistoryListEntry.status` now typed as `TaskStatus` and `toDisplayStatus` replacing the derivation, the client-side fallback is correctly eliminated. - **Backward-compat alias** — `TaskUiStatus = DisplayStatus` as a `@deprecated` re-export is the right way to handle this without breaking other consumers. --- ### 🔴 Issues requiring changes #### 1. Dead code in `live-runs-board.tsx` — `reviewing` branch is unreachable **File:** `apps/web/src/features/agents/live-runs-board.tsx`, `taskColumn` function ```ts // After this PR, toDisplayStatus("running") always returns "executing". // The || uiStatus === "reviewing" branch can never be true. if (uiStatus === "executing" || uiStatus === "reviewing") return "working"; ``` `toDisplayStatus` is documented to never produce `"reviewing"` from `TaskStatus` alone. Since `HistoryListEntry.ui_status` has been removed, `uiStatus` here is always the result of `toDisplayStatus(task.status ?? "running")`, meaning `"reviewing"` is unreachable dead code. This is misleading and should be removed: ```ts if (uiStatus === "executing") return "working"; ``` Same reasoning applies to any other condition in this file that checks `uiStatus === "reviewing"` or `uiStatus === "waiting"` — audit and trim them. --- #### 2. `ui_status_reason` hover tooltip silently dropped from history/fleet views **Files:** `task-detail.tsx`, `task-list.tsx`, `live-runs-board.tsx` (three `<StatusPill>` callsites) ```tsx // Before <StatusPill status={uiStatus} reason={task.ui_status_reason} /> // After <StatusPill status={uiStatus} /> ``` The `reason` prop is gone from all three callsites because `ui_status_reason` has been removed from `HistoryListEntry`. For active tasks the reason strings ("Awaiting approval to write .env", etc.) came from the server via `deriveTaskUiStatusReason`. Removing them silently is a user-visible regression — the hover tooltip that tells an operator *why* a task is in the `waiting` state disappears. The board card API is intentionally unchanged (noted in the PR description), but the fleet view (`live-runs-board.tsx`) uses `fetchHistory()`, not the board endpoint. Operators watching a running task from the fleet/history views lose the context tooltip entirely. Options: - **Restore `ui_status_reason` on `HistoryListEntry`** (server still has the data via `deriveTaskUiStatusReason`) — keep it as a standalone field independent of `ui_status`. - **Accept the regression intentionally** — document in the PR that reason tooltips are now board-card-only, and open a follow-up issue to restore them. Either is fine, but the current state silently removes a feature without acknowledgement in the PR body. --- ### ℹ️ Minor observations (no change required) - **`deriveTaskUiStatus` deprecated but still in use server-side** — the board endpoint (not in this diff) presumably still calls it. Deprecating without a migration path is a bit confusing; a follow-up to move the rich derivation to use `toDisplayStatus` as the base would clean this up. - **`tasks.ts` import ordering** — `import type { TaskStatus }` is inserted before the drizzle import. Biome will normalise this; no issue. --- **Summary:** Fix the dead `reviewing` branch in `taskColumn`, and either restore `ui_status_reason` on `HistoryListEntry` or explicitly acknowledge its removal. Everything else is clean.

charles force-pushed dev/1163 from f1d7c47a1f to 1bfda8d2fa 2026-05-14 23:00:52 +00:00 Compare

dev requested review from reviewer 2026-05-14 23:09:55 +00:00

reviewer requested changes 2026-05-14 23:14:13 +00:00

reviewer left a comment

Copy link

Code Review — PR #1183

Clean, well-scoped refactor. The new task-status.ts module is clearly written, the toDisplayStatus switch is exhaustive (TypeScript will catch regressions), and removing ui_status / ui_status_reason from the /history wire format is the right call. CI is green. Two issues to fix before merge.

---

🔴 Required changes

1. Error hover tooltip silently dropped from history rows

StatusPill's reason prop is now always undefined for every history row in task-detail.tsx, task-list.tsx, and live-runs-board.tsx. Before this PR, failure tasks showed task.error.slice(0, 200) on hover via ui_status_reason; that detail is now gone with no replacement.

task.error is still present on HistoryListEntry. The fix is a one-liner in each of the three components — pass reason={task.error ?? undefined} to the <StatusPill> when rendering history rows. No API change needed.

AC #1163 never explicitly said to remove the reason; this looks like an unintended side-effect of stripping ui_status_reason from the wire type.

2. deriveTaskUiStatus falls through for aborted_no_skill → maps to "executing" (wrong)

task-status.ts adds aborted_no_skill to TaskStatus, but deriveTaskUiStatus in task-ui-status.ts doesn't handle it — the if-chain falls through to return "executing". It should return "abandoned".

In practice this is unreachable today (board cards are live tasks; aborted_no_skill is terminal), but it's a silent semantic error on a deprecated-but-still-exported function. Fix: add if (status === "aborted_no_skill") return "abandoned"; alongside the existing cancelled/interrupted branch.

---

🟡 Worth noting (non-blocking)

• deriveTaskUiStatus also misses paused — falls to "executing" instead of "waiting". Pre-existing bug not introduced here, but since you're already touching the deprecation annotations this is the natural time to fix it.

• AC "No raw string literals for status values in query files" — not addressed in the diff. If any infrastructure query files use raw "running" / "queued" etc. in SQL expressions the typecheck won't catch them. Worth a quick grep before closing #1163.

• mergeable: false — the PR has a conflict with main. Needs a rebase before merge even after the above fixes.

---

AC verification against #1163

AC	Status
task-status.ts exports TaskStatus, DisplayStatus, toDisplayStatus	✅
index.ts re-exports all three	✅
Drizzle column typed with .$type<TaskStatus>()	✅
ui_status removed from /history wire format	✅
All components use toDisplayStatus(task.status)	✅
StatusPill props unchanged	✅
just qa passes	✅ (CI green)
No raw string literals in query files	❓ not visible in diff

## Code Review — PR #1183 Clean, well-scoped refactor. The new `task-status.ts` module is clearly written, the `toDisplayStatus` switch is exhaustive (TypeScript will catch regressions), and removing `ui_status` / `ui_status_reason` from the `/history` wire format is the right call. CI is green. Two issues to fix before merge. --- ### 🔴 Required changes #### 1. Error hover tooltip silently dropped from history rows `StatusPill`'s `reason` prop is now always `undefined` for every history row in `task-detail.tsx`, `task-list.tsx`, and `live-runs-board.tsx`. Before this PR, failure tasks showed `task.error.slice(0, 200)` on hover via `ui_status_reason`; that detail is now gone with no replacement. `task.error` is still present on `HistoryListEntry`. The fix is a one-liner in each of the three components — pass `reason={task.error ?? undefined}` to the `<StatusPill>` when rendering history rows. No API change needed. AC #1163 never explicitly said to remove the reason; this looks like an unintended side-effect of stripping `ui_status_reason` from the wire type. #### 2. `deriveTaskUiStatus` falls through for `aborted_no_skill` → maps to `"executing"` (wrong) `task-status.ts` adds `aborted_no_skill` to `TaskStatus`, but `deriveTaskUiStatus` in `task-ui-status.ts` doesn't handle it — the if-chain falls through to `return "executing"`. It should return `"abandoned"`. In practice this is unreachable today (board cards are live tasks; `aborted_no_skill` is terminal), but it's a silent semantic error on a deprecated-but-still-exported function. Fix: add `if (status === "aborted_no_skill") return "abandoned";` alongside the existing `cancelled`/`interrupted` branch. --- ### 🟡 Worth noting (non-blocking) - **`deriveTaskUiStatus` also misses `paused`** — falls to `"executing"` instead of `"waiting"`. Pre-existing bug not introduced here, but since you're already touching the deprecation annotations this is the natural time to fix it. - **AC "No raw string literals for status values in query files"** — not addressed in the diff. If any infrastructure query files use raw `"running"` / `"queued"` etc. in SQL expressions the typecheck won't catch them. Worth a quick grep before closing #1163. - **`mergeable: false`** — the PR has a conflict with main. Needs a rebase before merge even after the above fixes. --- ### AC verification against #1163 | AC | Status | |---|---| | `task-status.ts` exports `TaskStatus`, `DisplayStatus`, `toDisplayStatus` | ✅ | | `index.ts` re-exports all three | ✅ | | Drizzle column typed with `.$type<TaskStatus>()` | ✅ | | `ui_status` removed from `/history` wire format | ✅ | | All components use `toDisplayStatus(task.status)` | ✅ | | `StatusPill` props unchanged | ✅ | | `just qa` passes | ✅ (CI green) | | No raw string literals in query files | ❓ not visible in diff |

charles force-pushed dev/1163 from 1bfda8d2fa to 19aabe1c91 2026-05-14 23:17:48 +00:00 Compare

charles merged commit 070d20edf6 into main 2026-05-14 23:23:13 +00:00

charles deleted branch dev/1163 2026-05-14 23:23:14 +00:00

charles referenced this pull request from a commit 2026-05-14 23:23:15 +00:00

refactor(shared): single TaskStatus enum replaces dual db-status / ui_status split (#1183)

claude-desktop referenced this pull request 2026-05-14 23:27:19 +00:00

refactor(shared): single TaskStatus enum replaces dual db-status / ui_status split #1173

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/agent-hooks!1183

No description provided.