fix(types): tighten TaskDetail numeric coercion + ChannelSource literal union (krokoko review #2, #4, #10)

Three related type-safety fixes from the code review on PR #52. Grouped
because they all touch the same contract surface
(``TaskDetail`` on both sides of the wire) and share the theme "honest
types at the DDB + API boundary".

## Findings addressed

**#2 — turns_attempted / turns_completed bypass coerceNumericOrNull**

``toTaskDetail`` at ``cdk/src/handlers/shared/types.ts`` was passing
``record.turns_attempted`` and ``record.turns_completed`` through
with only ``?? null`` — same bug class as the
``costUsd.toFixed is not a function`` crash fixed at the fanout
boundary in commit ``9fe704e`` / consolidated in ``c09bfd7``. The
DDB Document-client deserializes ``Number`` attributes as ``string``
on some code paths; any downstream caller doing arithmetic on these
fields would crash at runtime.

Fix extends beyond the two fields the review called out — ALL
numeric fields sourced from the DDB record now route through
``coerceNumericOrNull``: ``duration_s``, ``cost_usd``, ``max_turns``,
``max_budget_usd``, ``turns_attempted``, ``turns_completed``. A new
JSDoc block on ``toTaskDetail`` documents the contract ("all numeric
fields coerced through shared helper; do not bypass") so a future
field addition has a clear pattern to follow. Scope-bounded: the
non-numeric fields (``task_description``, ``pr_url``, …) and
request-body-validated ints (``issue_number``, ``pr_number``) stay
untouched.

**#4 — CLI/CDK type drift on turns_attempted / turns_completed**

Per AGENTS.md the CDK and CLI ``TaskDetail`` declarations must stay
in sync. CDK declared both fields as required (``number | null``);
CLI marked them optional (``number | null | undefined``). The
tightening means the server's guarantee (``toTaskDetail`` always
emits both fields, defaulting to ``null`` when absent on the record)
now flows honestly to the CLI type.

**#10 — channel_source typed as string instead of literal union**

Added an exported ``ChannelSource = 'api' | 'webhook'`` literal union
in both CDK and CLI ``types.ts``. ``TaskRecord.channel_source`` and
``TaskDetail.channel_source`` on both sides now use the narrow type.
Downstream switches/predicates that read ``channel_source`` get
exhaustiveness checking at compile time, matching the internal
``CreateTaskContext.channelSource`` literal already in use at
``create-task-core.ts``. Reviewer's comment: "the internal
CreateTaskContext correctly narrows it but the external types don't"
— now they do.

## Tests

+3 regression tests in
``cdk/test/handlers/shared/error-classifier.test.ts`` under the
``toTaskDetail integration`` block:

  - String-typed DDB numeric fields coerce to finite numbers on
    the ``TaskDetail`` output (``typeof === 'number'`` + exact value
    for all six coerced fields).
  - Unparseable numeric strings collapse to ``null`` without crash
    (defence test for the non-finite branch in ``coerceNumericOrNull``).
  - ``channel_source`` narrows to the literal union — uses
    ``@ts-expect-error`` to pin that a widened ``'slack'`` fails to
    compile, so a future ``ChannelSource`` regression surfaces in CI
    immediately.

CLI test fixtures updated to include ``channel_source: 'api'`` and
``turns_attempted: null`` / ``turns_completed: null`` where the
non-optional fields were previously omitted. No CLI test count
change — the fixture additions satisfy the stricter contract
without requiring new test bodies.

CDK suite: 1032 passing (was 1029). CLI suite: 190 passing.

Refs: krokoko code review on PR #52 (findings 2, 4, 10)

Author: bgagent

cdk/src/handlers/shared/types.ts

@@ -18,12 +18,25 @@
  */
 
 import { classifyError, type ErrorClassification } from './error-classifier';
+import { logger } from './logger';
+import { coerceNumericOrNull } from './numeric';
 import type { ComputeType } from './repo-config';
 import type { TaskStatusType } from '../../constructs/task-status';
 
 /** Valid task types for task creation. */
 export type TaskType = 'new_task' | 'pr_iteration' | 'pr_review';
 
+/**
+ * Provenance of a task's submission. ``api`` covers CLI / Cognito-authenticated
+ * submissions; ``webhook`` covers HMAC-signed inbound webhook submissions.
+ *
+ * Narrowed from ``string`` so switches and predicates that read
+ * ``channel_source`` get exhaustiveness checking at compile time; matches the
+ * internal ``CreateTaskContext.channelSource`` literal in ``create-task-core.ts``.
+ * Keep in sync with ``cli/src/types.ts::ChannelSource``.
+ */
+export type ChannelSource = 'api' | 'webhook';
+
 /** Task types that operate on an existing pull request. */
 export function isPrTaskType(taskType: TaskType): boolean {
   return taskType === 'pr_iteration' || taskType === 'pr_review';
@@ -51,7 +64,7 @@ export interface TaskRecord {
   readonly pr_url?: string;
   readonly error_message?: string;
   readonly idempotency_key?: string;
-  readonly channel_source: string;
+  readonly channel_source: ChannelSource;
   readonly channel_metadata?: Record<string, string>;
   readonly status_created_at: string;
   readonly created_at: string;
@@ -155,7 +168,7 @@ export interface TaskDetail {
    *  on every task record at creation time (``create-task-core.ts``)
    *  and surfaced here so CLI / dashboard / audit consumers do not have
    *  to spelunk CloudWatch to learn which channel created a task. */
-  readonly channel_source: string;
+  readonly channel_source: ChannelSource;
   readonly created_at: string;
   readonly updated_at: string;
   readonly started_at: string | null;
@@ -274,10 +287,21 @@ export interface Attachment {
 
 /**
  * Map a DynamoDB task record to the API detail response shape.
+ *
+ * All numeric fields sourced from the DDB record are routed through
+ * ``coerceNumericOrNull`` — the Document-client deserializes DynamoDB
+ * ``Number`` attributes as JavaScript ``string``s in some code paths
+ * (see ``shared/numeric.ts`` for rationale), and any downstream caller
+ * doing arithmetic (``.toFixed``, comparison, math) on a string-typed
+ * "number" crashes at runtime. Coercing uniformly here means no caller
+ * has to guess which TaskDetail numeric fields are safe — do not bypass
+ * the helper when adding new numeric fields.
+ *
  * @param record - the DynamoDB task record.
  * @returns the API-facing task detail.
  */
 export function toTaskDetail(record: TaskRecord): TaskDetail {
+  const ctx = { task_id: record.task_id };
   return {
     task_id: record.task_id,
     status: record.status,
@@ -296,13 +320,13 @@ export function toTaskDetail(record: TaskRecord): TaskDetail {
     updated_at: record.updated_at,
     started_at: record.started_at ?? null,
     completed_at: record.completed_at ?? null,
-    duration_s: record.duration_s ?? null,
-    cost_usd: record.cost_usd ?? null,
+    duration_s: coerceNumericOrNull(record.duration_s, { ...ctx, field: 'duration_s' }, logger),
+    cost_usd: coerceNumericOrNull(record.cost_usd, { ...ctx, field: 'cost_usd' }, logger),
     build_passed: record.build_passed ?? null,
-    max_turns: record.max_turns ?? null,
-    max_budget_usd: record.max_budget_usd ?? null,
-    turns_attempted: record.turns_attempted ?? null,
-    turns_completed: record.turns_completed ?? null,
+    max_turns: coerceNumericOrNull(record.max_turns, { ...ctx, field: 'max_turns' }, logger),
+    max_budget_usd: coerceNumericOrNull(record.max_budget_usd, { ...ctx, field: 'max_budget_usd' }, logger),
+    turns_attempted: coerceNumericOrNull(record.turns_attempted, { ...ctx, field: 'turns_attempted' }, logger),
+    turns_completed: coerceNumericOrNull(record.turns_completed, { ...ctx, field: 'turns_completed' }, logger),
     prompt_version: record.prompt_version ?? null,
     trace: record.trace === true,
     trace_s3_uri: record.trace_s3_uri ?? null,

cdk/test/handlers/shared/error-classifier.test.ts

@@ -420,7 +420,7 @@ describe('classifyError', () => {
       repo: 'owner/repo',
       task_type: 'new_task',
       branch_name: 'bgagent/task-1/fix',
-      channel_source: 'cli',
+      channel_source: 'api',
       status_created_at: 'FAILED#2026-01-01T00:00:00Z',
       created_at: '2026-01-01T00:00:00Z',
       updated_at: '2026-01-01T00:00:00Z',
@@ -446,5 +446,65 @@ describe('classifyError', () => {
       expect(detail.error_classification).not.toBeNull();
       expect(detail.error_classification!.category).toBe('unknown');
     });
+
+    // Regression: all numeric fields coerce through ``coerceNumericOrNull``
+    // so the DDB Document-client's string-typed Number deserialization
+    // cannot leak into downstream consumers (same bug class as the
+    // ``costUsd.toFixed`` crash fixed in commit ``c09bfd7``). The cast
+    // to ``unknown as TaskRecord`` simulates a record produced by the
+    // Document client where ``Number`` attributes came back as strings.
+    test('coerces string-typed numeric DDB fields to numbers on output', () => {
+      const record = {
+        ...baseRecord,
+        duration_s: '12.5',
+        cost_usd: '0.0042',
+        max_turns: '30',
+        max_budget_usd: '1.50',
+        turns_attempted: '7',
+        turns_completed: '6',
+      } as unknown as TaskRecord;
+      const detail = toTaskDetail(record);
+      expect(typeof detail.duration_s).toBe('number');
+      expect(detail.duration_s).toBe(12.5);
+      expect(typeof detail.cost_usd).toBe('number');
+      expect(detail.cost_usd).toBe(0.0042);
+      expect(typeof detail.max_turns).toBe('number');
+      expect(detail.max_turns).toBe(30);
+      expect(typeof detail.max_budget_usd).toBe('number');
+      expect(detail.max_budget_usd).toBe(1.5);
+      expect(typeof detail.turns_attempted).toBe('number');
+      expect(detail.turns_attempted).toBe(7);
+      expect(typeof detail.turns_completed).toBe('number');
+      expect(detail.turns_completed).toBe(6);
+    });
+
+    test('coerces unparseable numeric strings to null (does not crash)', () => {
+      const record = {
+        ...baseRecord,
+        turns_attempted: 'not-a-number',
+        turns_completed: 'NaN',
+      } as unknown as TaskRecord;
+      const detail = toTaskDetail(record);
+      expect(detail.turns_attempted).toBeNull();
+      expect(detail.turns_completed).toBeNull();
+    });
+
+    // Compile-time regression for Finding #10 — ``ChannelSource`` is a
+    // literal union, not ``string``. The ``satisfies`` assertions below
+    // exercise the valid members; the ``@ts-expect-error`` comments pin
+    // the narrowing — if someone widens ``ChannelSource`` to ``string``
+    // these will become un-erroring and fail the build.
+    test('channel_source narrows to the literal union', () => {
+      const apiRecord: TaskRecord = { ...baseRecord, channel_source: 'api' };
+      const webhookRecord: TaskRecord = { ...baseRecord, channel_source: 'webhook' };
+      expect(toTaskDetail(apiRecord).channel_source).toBe('api');
+      expect(toTaskDetail(webhookRecord).channel_source).toBe('webhook');
+
+      // @ts-expect-error — 'slack' is not a valid ChannelSource
+      const invalid: TaskRecord = { ...baseRecord, channel_source: 'slack' };
+      // Keep ``invalid`` used so the block doesn't get DCE'd and the
+      // ``@ts-expect-error`` above remains anchored to a real assignment.
+      expect(invalid.channel_source).toBeDefined();
+    });
   });
 });

cli/src/types.ts

@@ -20,6 +20,15 @@
 /** Valid task types for task creation. */
 export type TaskType = 'new_task' | 'pr_iteration' | 'pr_review';
 
+/**
+ * Provenance of a task's submission. ``api`` covers CLI / Cognito-authenticated
+ * submissions; ``webhook`` covers HMAC-signed inbound webhook submissions.
+ * Mirrors ``cdk/src/handlers/shared/types.ts::ChannelSource`` per the CLI
+ * types-sync contract so downstream switches/predicates get exhaustiveness
+ * checking on both sides of the wire.
+ */
+export type ChannelSource = 'api' | 'webhook';
+
 /** Error categories produced by the runtime error classifier. */
 export type ErrorCategoryType = 'auth' | 'network' | 'concurrency' | 'compute' | 'agent' | 'guardrail' | 'config' | 'timeout' | 'unknown';
 
@@ -50,7 +59,7 @@ export interface TaskDetail {
    *  submissions, ``webhook`` for HMAC-signed inbound webhooks.
    *  Mirrors ``cdk/src/handlers/shared/types.ts::TaskDetail``; kept
    *  in sync per the CLI types-sync contract. */
-  readonly channel_source: string;
+  readonly channel_source: ChannelSource;
   readonly created_at: string;
   readonly updated_at: string;
   readonly started_at: string | null;
@@ -62,11 +71,14 @@ export interface TaskDetail {
   readonly max_budget_usd: number | null;
   /** Rev-5 DATA-1: attempts counter from the SDK (may be `max_turns + 1`
    *  when `agent_status='error_max_turns'` — the aborted attempt is
-   *  counted). */
-  readonly turns_attempted?: number | null;
+   *  counted). Required to match ``cdk/src/handlers/shared/types.ts``
+   *  (server always emits the field, defaulted to ``null`` in
+   *  ``toTaskDetail`` when absent on the record). */
+  readonly turns_attempted: number | null;
   /** Rev-5 DATA-1: turns that actually completed (clamped to
-   *  `max_turns` when the cap tripped). */
-  readonly turns_completed?: number | null;
+   *  `max_turns` when the cap tripped). Required; see
+   *  ``turns_attempted`` above. */
+  readonly turns_completed: number | null;
   /** Whether the task was submitted with ``--trace``. Surfaces in
    *  ``bgagent status --output json`` so scripts can confirm trace
    *  capture is active. Non-optional because the server always

cli/test/format-status-snapshot.test.ts

@@ -18,7 +18,7 @@
  */
 
 import { formatStatusSnapshot } from '../src/format';
-import { TaskDetail, TaskEvent } from '../src/types';
+import { ChannelSource, TaskDetail, TaskEvent } from '../src/types';
 
 const NOW = Date.parse('2026-04-29T15:30:20Z');
 
@@ -447,9 +447,11 @@ describe('formatStatusSnapshot', () => {
   });
 
   test('Channel line is always present (even when channel_source is an unexpected value)', () => {
-    // Degrades to the placeholder rather than omitting the line —
-    // consistent with other always-present snapshot rows.
-    const task = buildTask({ channel_source: '' as unknown as string });
+    // Defence-in-depth: even though ``ChannelSource`` narrows the type to
+    // ``api | webhook``, a corrupt DDB record could still arrive at the
+    // formatter. The snapshot degrades to the placeholder rather than
+    // omitting the line — consistent with other always-present rows.
+    const task = buildTask({ channel_source: '' as unknown as ChannelSource });
     const rendered = formatStatusSnapshot(task, [], NOW);
     expect(rendered).toContain('Channel:       —');
   });

cli/test/format.test.ts

@@ -44,6 +44,8 @@ describe('format', () => {
     build_passed: true,
     max_turns: 100,
     max_budget_usd: null,
+    turns_attempted: null,
+    turns_completed: null,
     trace: false,
     trace_s3_uri: null,
   };