diff --git a/panel/src/app/(dashboard)/prompter/page.tsx b/panel/src/app/(dashboard)/prompter/page.tsx index 2b4beaa1..94d1f92c 100644 --- a/panel/src/app/(dashboard)/prompter/page.tsx +++ b/panel/src/app/(dashboard)/prompter/page.tsx @@ -4,10 +4,12 @@ import { useEffect, useRef } from "react"; import { Loader2, Sparkles, X } from "lucide-react"; import { Button } from "@/components/ui/button"; import { usePrompter } from "@/hooks/use-prompter"; +import { Team } from "@/types"; import { ChatMessages, ChatComposer, SuccessCard, + BoardReviewSentCard, IntakeForm, BatchReviewCard, } from "@/components/prompter"; @@ -42,7 +44,7 @@ export default function PrompterPage() { batch, batchWaves, batchResult, - updateBatchDraftProject, + setBatchDraftProjects, confirmBatch, } = usePrompter(); @@ -113,22 +115,40 @@ export default function PrompterPage() { createdTaskTeam ? (
- - {batchResult && ( -

- {batchResult.root_subtask_ids.length} tasks sequenced into{" "} - {batchResult.waves.length} wave - {batchResult.waves.length === 1 ? "" : "s"}. - {batchResult.warnings.length > 0 && - ` ${batchResult.warnings.length} advisory note${ - batchResult.warnings.length === 1 ? "" : "s" - }.`} -

+ {/* Board-routed MegaTask: created HELD for PO+HoM review, not + dispatched — the CEO releases it with Approve & Start on the + umbrella task. Every other path is a real "created/launched" + success. ``createdTaskTeam === BOARD`` is set only by the + batch board route (the single-draft board route parks and + never reaches success). */} + {createdTaskTeam === Team.BOARD && batchResult ? ( + + ) : ( + <> + + {batchResult && ( +

+ {batchResult.root_subtask_ids.length} tasks sequenced + into {batchResult.waves.length} wave + {batchResult.waves.length === 1 ? "" : "s"}. + {batchResult.warnings.length > 0 && + ` ${batchResult.warnings.length} advisory note${ + batchResult.warnings.length === 1 ? "" : "s" + }.`} +

+ )} + )}
@@ -149,7 +169,7 @@ export default function PrompterPage() { waves={batchWaves} projectIds={projectIds} onKeepChatting={keepChatting} - onProjectChange={updateBatchDraftProject} + onSetProjects={setBatchDraftProjects} onConfirm={confirmBatch} isLaunching={isLaunching} /> diff --git a/panel/src/components/prompter/batch-review-card.tsx b/panel/src/components/prompter/batch-review-card.tsx index ebd4e325..570e08a7 100644 --- a/panel/src/components/prompter/batch-review-card.tsx +++ b/panel/src/components/prompter/batch-review-card.tsx @@ -11,15 +11,10 @@ import { import { Button } from "@/components/ui/button"; import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card"; import { Badge } from "@/components/ui/badge"; -import { - Select, - SelectContent, - SelectItem, - SelectTrigger, - SelectValue, -} from "@/components/ui/select"; +import { Checkbox } from "@/components/ui/checkbox"; import { useProjects } from "@/hooks/use-projects"; import type { BatchProposal, StartRoute } from "@/hooks/use-prompter"; +import type { ProjectSummary } from "@/types"; import type { CellWork, DraftProposal } from "@/lib/api/prompter"; import { Team } from "@/types"; @@ -32,38 +27,40 @@ const CELL_LABEL: Record = { ux_ui: "UX/UI", }; -/** One the_work entry whose team is a delivery cell — a per-cell project picker. */ -interface CellEntry { - entry: CellWork; - entryIndex: number; - team: Team; -} - -/** A draft's per-cell entries (the the_work slots that carry a cell team), in - * the_work order. Empty for a legacy single-cell draft with no cell the_work. */ -function cellEntries(draft: DraftProposal): CellEntry[] { - return (draft.the_work ?? []) - .map((entry, entryIndex) => ({ entry, entryIndex, team: entry?.team })) +/** The project_ids a draft currently targets: the per-cell ``the_work[].project_id`` + * set (the multi-select model), falling back to a legacy top-level project_id. + * Only scoped ids count — an out-of-scope id is treated as unselected. */ +function selectedProjectIds( + draft: DraftProposal, + scoped: Set, +): string[] { + const work = Array.isArray(draft.the_work) ? draft.the_work : []; + const pids = work .filter( - (e): e is CellEntry => - !!e.team && (CELL_TEAMS as readonly string[]).includes(e.team), - ); + (w): w is CellWork & { project_id: string } => + !!w?.team && + (CELL_TEAMS as readonly string[]).includes(w.team) && + typeof w.project_id === "string" && + w.project_id !== "" && + scoped.has(w.project_id), + ) + .map((w) => w.project_id); + if (pids.length > 0) return pids; + if (draft.project_id && scoped.has(draft.project_id)) + return [draft.project_id]; + return []; } interface BatchReviewCardProps { batch: BatchProposal; /** The conflict-free waves (lists of draft indices), once previewed. */ waves: number[][] | null; - /** The repos this MegaTask is scoped to — each cell must target one of them. */ + /** The repos this MegaTask is scoped to — each task targets a subset of them. */ projectIds: string[]; onKeepChatting: () => void; - /** `entryIndex` is the the_work slot (the cell); -1 for a legacy single-cell - * draft with no per-cell map (sets the top-level project_id). */ - onProjectChange: ( - index: number, - entryIndex: number, - projectId: string, - ) => void; + /** Set the whole set of projects one task targets (multi-select across cells, + * one repo per cell — the backend stores one project per cell). */ + onSetProjects: (index: number, ids: string[]) => void; onConfirm: (route: StartRoute) => void; /** A launch is in flight — disable the actions so a double-click can't dupe. */ isLaunching?: boolean; @@ -71,38 +68,55 @@ interface BatchReviewCardProps { /** * The MegaTask review card: every task the agent proposed in one batch, each - * with its per-cell target projects (editable) and collision surface, plus the - * conflict-free wave plan. A multi-cell task (be+fe, fe+uxui) shows one project - * picker per cell, scoped to that cell's repos — a RoboCo project is per-cell, - * so each cell lands in its own repo. The human reviews the whole batch and the - * sequencing, fixes any cell in the wrong repo, then picks one start path. + * with its target projects (a multi-select checkbox list — one task can span + * several repos, one repo per delivery cell) and collision surface, plus the + * conflict-free wave plan. The human reviews the whole batch and the sequencing, + * picks the repos each task lands in, then picks one start path. */ export function BatchReviewCard({ batch, waves, projectIds, onKeepChatting, - onProjectChange, + onSetProjects, onConfirm, isLaunching = false, }: BatchReviewCardProps) { const { data: allProjects = [] } = useProjects(); // Only the scoped repos are valid targets (the agent read only those). const scoped = new Set(projectIds); + const scopedByCell = (cell: Team): ProjectSummary[] => + allProjects.filter((p) => scoped.has(p.id) && p.assigned_cell === cell); const titleOf = (i: number): string => batch.drafts[i]?.title ?? `Task ${i + 1}`; - // A task is mis-targeted when any of its cells lacks a scoped project (a - // multi-cell draft checks every the_work entry; a legacy single-cell draft - // with no cell map checks its top-level project_id). - const missingProject = batch.drafts.some((d) => { - const entries = cellEntries(d); - if (entries.length > 0) { - return entries.some( - (ce) => !ce.entry.project_id || !scoped.has(ce.entry.project_id), + // A task is mis-targeted when it has no project selected at all (the backend + // re-asserts each targeted project is in scope and the batch spans ≥2 repos). + const missingProject = batch.drafts.some( + (d) => selectedProjectIds(d, scoped).length === 0, + ); + + /** Toggle one project in a task's selection. A RoboCo project is per-cell and + * the backend stores one project per cell, so checking a repo in a cell that + * already has a different repo checked swaps it (unchecks the sibling). */ + const toggle = (index: number, projectId: string) => { + const draft = batch.drafts[index]; + const current = selectedProjectIds(draft, scoped); + const proj = allProjects.find((p) => p.id === projectId); + const cell = proj?.assigned_cell; + if (current.includes(projectId)) { + onSetProjects( + index, + current.filter((id) => id !== projectId), ); + return; } - return !d.project_id || !scoped.has(d.project_id); - }); + // Checking: drop any other project in the same cell (one repo per cell). + const next = current.filter((id) => { + const other = allProjects.find((p) => p.id === id); + return other?.assigned_cell !== cell; + }); + onSetProjects(index, [...next, projectId]); + }; return ( @@ -124,14 +138,22 @@ export function BatchReviewCard({
    {batch.drafts.map((draft, i) => { - const entries = cellEntries(draft); + const selected = selectedProjectIds(draft, scoped); return (
  1. - + {/* Title is clamped to a fixed 2-line space so a long title + can't grow the row (or, as a long unbroken token, blow the + card width out and wreck the layout). min-w-0 lets the flex + item shrink below min-content; break-words stops a token + from overflowing; the full title is on the tooltip. */} + {i + 1}. {draft.title}
    @@ -154,87 +176,48 @@ export function BatchReviewCard({ {draft.objective || draft.description}

    )} - {entries.length > 0 ? ( - /* Per-cell project picker — one Select per the_work entry, - scoped to that cell's repos (a project is per-cell). */ -
    - {entries.map(({ entry, entryIndex, team }) => { - const cellProjects = allProjects.filter( - (p) => scoped.has(p.id) && p.assigned_cell === team, - ); - const pid = entry.project_id ?? ""; - const ok = pid !== "" && scoped.has(pid); - return ( -
    - - {CELL_LABEL[team] ?? team} - - + {/* Multi-select project picker — one task can span several repos + (one per delivery cell). Grouped by cell; one repo per cell. */} +
    +

    + Projects {selected.length === 0 && "— pick at least one"} +

    + {CELL_TEAMS.map((cell) => { + const repos = scopedByCell(cell); + if (repos.length === 0) return null; + return ( +
    + + {CELL_LABEL[cell] ?? cell} + +
    + {repos.map((p) => { + const checked = selected.includes(p.id); + return ( + + ); + })}
    - ); - })} -
    - ) : ( - /* Legacy single-cell draft (no per-cell the_work) — one Select - bound to the top-level project_id, scoped to all repos. */ -
    - - Project - - -
    - )} +
    + ); + })} +
  2. ); })} @@ -248,7 +231,11 @@ export function BatchReviewCard({

      {waves.map((wave, w) => ( -
    1. +
    2. titleOf(i)).join(", ")} + > Wave {w + 1}:{" "} {wave.map((i) => titleOf(i)).join(", ")}
    3. @@ -259,7 +246,7 @@ export function BatchReviewCard({ {missingProject && (

      - Pick a project for every cell of every task before launching the + Pick at least one project for every task before launching the MegaTask.

      )} diff --git a/panel/src/components/prompter/board-review-sent-card.tsx b/panel/src/components/prompter/board-review-sent-card.tsx new file mode 100644 index 00000000..59b4618b --- /dev/null +++ b/panel/src/components/prompter/board-review-sent-card.tsx @@ -0,0 +1,91 @@ +"use client"; + +import Link from "next/link"; +import { Users, ExternalLink, RefreshCw } from "lucide-react"; +import { Button } from "@/components/ui/button"; +import { + Card, + CardContent, + CardFooter, + CardHeader, + CardTitle, +} from "@/components/ui/card"; +import { Badge } from "@/components/ui/badge"; + +interface BoardReviewSentCardProps { + /** The umbrella task id — the single board-review / CEO-approve unit. */ + taskId: string; + taskTitle: string; + rootSubtaskCount: number; + waveCount: number; + onStartAnother: () => void; +} + +/** + * The MegaTask "Board review & Start" confirmation: the umbrella + root-subtasks + * were created HELD (umbrella assigned to the Product Owner, root-subtasks in + * BACKLOG) for the Product Owner + Head of Marketing to review. Nothing is + * dispatched yet — the CEO releases the sequenced tasks with Approve & Start on + * the umbrella task once the board finishes. This is the batch analogue of the + * single-draft board route's "sent to the board" wait, reusing the existing CEO + * Approve & Start gate on the umbrella (which fires ``approve_and_start`` → + * ``_activate_batch_root_subtasks``). + */ +export function BoardReviewSentCard({ + taskId, + taskTitle, + rootSubtaskCount, + waveCount, + onStartAnother, +}: BoardReviewSentCardProps) { + return ( + + +
      + + + Sent to the Board for review + +
      +
      + + +

      {taskTitle}

      +
      + + {rootSubtaskCount} task{rootSubtaskCount === 1 ? "" : "s"} + + + {waveCount} wave{waveCount === 1 ? "" : "s"} + + + ID: {taskId.slice(0, 8)}… + +
      +

      + The Product Owner and Head of Marketing are reviewing this MegaTask. + Nothing is dispatched yet. Once they finish, open the umbrella task + and use Approve & Start to + release the sequenced tasks. You can leave and come back. +

      +
      + + + + + +
      + ); +} diff --git a/panel/src/components/prompter/draft-proposal-card.tsx b/panel/src/components/prompter/draft-proposal-card.tsx index 1a75c690..d1577f3d 100644 --- a/panel/src/components/prompter/draft-proposal-card.tsx +++ b/panel/src/components/prompter/draft-proposal-card.tsx @@ -45,7 +45,7 @@ function draftToText(draft: DraftProposal): string { "", ); } - if (draft.the_work?.length) { + if (Array.isArray(draft.the_work) && draft.the_work.length) { lines.push("## The Work"); for (const cell of draft.the_work) { lines.push(`### ${cellLabel(cell.team)}`, cell.summary); @@ -73,7 +73,7 @@ export function DraftProposalCard({ isLaunching = false, }: DraftProposalCardProps) { const priorityLabel = PRIORITY_LABELS[draft.priority ?? 2] ?? "Medium"; - const cells = draft.the_work ?? []; + const cells = Array.isArray(draft.the_work) ? draft.the_work : []; // Distinct cells only: the_work has one entry per work item, so a cell with // several items would otherwise show its badge repeated (Backend Backend …). const distinctTeams = Array.from(new Set(cells.map((c) => c.team))); diff --git a/panel/src/components/prompter/index.ts b/panel/src/components/prompter/index.ts index 55d6b98b..5b1ac1c4 100644 --- a/panel/src/components/prompter/index.ts +++ b/panel/src/components/prompter/index.ts @@ -3,4 +3,5 @@ export { ChatComposer } from "./chat-composer"; export { DraftProposalCard } from "./draft-proposal-card"; export { BatchReviewCard } from "./batch-review-card"; export { SuccessCard } from "./success-card"; +export { BoardReviewSentCard } from "./board-review-sent-card"; export { IntakeForm } from "./intake-form"; diff --git a/panel/src/components/tasks/task-table.tsx b/panel/src/components/tasks/task-table.tsx index b378f5cd..5532eb92 100644 --- a/panel/src/components/tasks/task-table.tsx +++ b/panel/src/components/tasks/task-table.tsx @@ -544,9 +544,9 @@ export function TaskTable({ )} onClick={handleRowClick} > - +
      {hasChildren ? ( @@ -569,8 +569,10 @@ export function TaskTable({ href={"/tasks/" + task.id} className="block hover:underline min-w-0" > -
      - {task.title} +
      + + {task.title} + {task.batch_id && !task.parent_task_id && ( ): DraftProposal { + return { + title: "T", + description: "objective objective objective", + acceptance_criteria: ["a"], + team: Team.BACKEND, + ...partial, + } as DraftProposal; +} + +function mk(drafts: DraftProposal[]): BatchProposal { + return { title: "batch", drafts, dropped: 0 }; +} + +describe("fillBatchProjects", () => { + it("fills a single-cell draft from the top-level project_id when it matches the cell", () => { + const batch = mk([ + draft({ the_work: [{ team: Team.BACKEND, summary: "s", items: [] }] }), + ]); + const out = fillBatchProjects(batch, [BE], [proj(BE, Team.BACKEND)]); + expect(out.drafts[0].the_work![0].project_id).toBe(BE); + }); + + it("falls back to the single scoped repo for the cell when no project_id is set", () => { + const batch = mk([ + draft({ the_work: [{ team: Team.FRONTEND, summary: "s", items: [] }] }), + ]); + const out = fillBatchProjects( + batch, + [BE, FE], + [proj(BE, Team.BACKEND), proj(FE, Team.FRONTEND)], + ); + expect(out.drafts[0].the_work![0].project_id).toBe(FE); + }); + + it("leaves a cell empty when 2+ scoped repos belong to that cell (real ambiguity)", () => { + const batch = mk([ + draft({ the_work: [{ team: Team.BACKEND, summary: "s", items: [] }] }), + ]); + const out = fillBatchProjects( + batch, + [BE, BE2], + [proj(BE, Team.BACKEND), proj(BE2, Team.BACKEND)], + ); + expect(out.drafts[0].the_work![0].project_id).toBeFalsy(); + }); + + it("does not overwrite an explicit per-cell project_id", () => { + const batch = mk([ + draft({ + the_work: [ + { team: Team.BACKEND, summary: "s", items: [], project_id: BE2 }, + ], + }), + ]); + const out = fillBatchProjects( + batch, + [BE, BE2], + [proj(BE, Team.BACKEND), proj(BE2, Team.BACKEND)], + ); + expect(out.drafts[0].the_work![0].project_id).toBe(BE2); + }); + + it("does not trust a top-level project_id whose cell mismatches the work entry", () => { + const batch = mk([ + draft({ + project_id: FE, // frontend repo, but the work says backend + the_work: [{ team: Team.BACKEND, summary: "s", items: [] }], + }), + ]); + const out = fillBatchProjects( + batch, + [FE, BE], + [proj(FE, Team.FRONTEND), proj(BE, Team.BACKEND)], + ); + expect(out.drafts[0].the_work![0].project_id).toBe(BE); + }); + + it("is idempotent (returns the same batch reference on a second pass)", () => { + const batch = mk([ + draft({ the_work: [{ team: Team.BACKEND, summary: "s", items: [] }] }), + ]); + const once = fillBatchProjects(batch, [BE], [proj(BE, Team.BACKEND)]); + const twice = fillBatchProjects(once, [BE], [proj(BE, Team.BACKEND)]); + expect(twice).toBe(once); + }); + + it("legacy draft (no the_work) keeps a scoped top-level project_id as-is", () => { + const batch = mk([draft({ the_work: [], project_id: BE })]); + const out = fillBatchProjects(batch, [BE], [proj(BE, Team.BACKEND)]); + expect(out).toBe(batch); // nothing to fill → same reference + }); +}); + +describe("rebuildCellWork", () => { + // The multi-select review card: the human picks a SET of repos for a task + // (one repo per delivery cell). rebuildCellWork turns that set into the + // the_work[] cell map the backend stores (task_cell_projects is unique per + // (task, team) — one repo per cell). + + const projects = [ + proj(BE, Team.BACKEND), + proj(BE2, Team.BACKEND), + proj(FE, Team.FRONTEND), + proj("ux-repo", Team.UX_UI), + ]; + + it("maps one selected repo per cell and clears the top-level project_id", () => { + const out = rebuildCellWork([BE, FE], projects, []); + expect(out.project_id).toBeNull(); + const teams = out.the_work.map((w) => w.team); + expect(teams).toEqual([Team.BACKEND, Team.FRONTEND]); + expect(out.the_work[0].project_id).toBe(BE); + expect(out.the_work[1].project_id).toBe(FE); + }); + + it("keeps only the first selected repo when two are picked for the same cell (one repo per cell)", () => { + const out = rebuildCellWork([BE, BE2], projects, []); + expect(out.the_work).toHaveLength(1); + expect(out.the_work[0].team).toBe(Team.BACKEND); + expect(out.the_work[0].project_id).toBe(BE); // first wins + }); + + it("preserves an existing cell entry's summary/items when the cell stays selected", () => { + const current: CellWork[] = [ + { + team: Team.BACKEND, + summary: "build API", + items: ["x", "y"], + project_id: BE2, + }, + ]; + const out = rebuildCellWork([BE], projects, current); + expect(out.the_work).toHaveLength(1); + expect(out.the_work[0].summary).toBe("build API"); + expect(out.the_work[0].items).toEqual(["x", "y"]); + expect(out.the_work[0].project_id).toBe(BE); // repo swapped, summary kept + }); + + it("drops a cell entry when its cell is no longer selected", () => { + const current: CellWork[] = [ + { team: Team.BACKEND, summary: "s", items: [] }, + { team: Team.FRONTEND, summary: "s", items: [] }, + ]; + const out = rebuildCellWork([FE], projects, current); + expect(out.the_work.map((w) => w.team)).toEqual([Team.FRONTEND]); + }); + + it("appends a minimal entry for a newly-selected cell not in the_work", () => { + const out = rebuildCellWork([FE, "ux-repo"], projects, [ + { team: Team.BACKEND, summary: "s", items: [] }, + ]); + const teams = out.the_work.map((w) => w.team); + expect(teams).toEqual([Team.FRONTEND, Team.UX_UI]); // backend dropped, fe+ux added + expect(out.the_work[0]).toMatchObject({ + team: Team.FRONTEND, + summary: "", + items: [], + }); + }); + + it("empty selection empties the_work (the task then has no project → blocked)", () => { + const out = rebuildCellWork([], projects, [ + { team: Team.BACKEND, summary: "s", items: [] }, + ]); + expect(out.the_work).toEqual([]); + expect(out.project_id).toBeNull(); + }); +}); diff --git a/panel/src/hooks/use-prompter.ts b/panel/src/hooks/use-prompter.ts index 792693b7..fd3f19a4 100644 --- a/panel/src/hooks/use-prompter.ts +++ b/panel/src/hooks/use-prompter.ts @@ -16,6 +16,7 @@ import { } from "@/lib/api/prompter"; import { getErrorMessage } from "@/lib/api/client"; import { tasksApi } from "@/lib/api/tasks"; +import { useProjects } from "@/hooks/use-projects"; import { Team } from "@/types"; import type { TaskType, TaskNature, Complexity } from "@/types"; @@ -68,7 +69,8 @@ const CELL_TEAMS: Team[] = [Team.BACKEND, Team.FRONTEND, Team.UX_UI]; * is a delivery cell and that carries a project_id. Empty for a legacy * single-cell draft that uses a top-level project_id instead. */ function draftCellProjectIds(draft: DraftProposal): string[] { - return (draft.the_work ?? []) + const work = Array.isArray(draft.the_work) ? draft.the_work : []; + return work .filter( (w): w is CellWork & { project_id: string } => !!w?.team && @@ -173,6 +175,17 @@ function draftFromEvent(data: Record | undefined): { return { draft: d as unknown as DraftProposal, scale }; } +/** Coerce a value into a `CellWork[]`: a bare object → one-element array, a + * non-array non-object → empty. The backend coerces `the_work` before it + * reaches SSE, but a stale localStorage payload or a malformed frame could + * still carry a non-array, and the batch card does `the_work.map(...)` — so + * normalize here rather than crash. */ +function asCellWork(value: unknown): CellWork[] { + if (Array.isArray(value)) return value as CellWork[]; + if (value && typeof value === "object") return [value as CellWork]; + return []; +} + /** Pull a MegaTask ({title, drafts[]}) out of a `batch` SSE event's payload. */ function batchFromEvent( data: Record | undefined, @@ -180,10 +193,15 @@ function batchFromEvent( if (!data || typeof data !== "object") return null; const raw = (data as Record).drafts; if (!Array.isArray(raw)) return null; - const drafts = raw.filter( - (x): x is DraftProposal => - !!x && typeof (x as DraftProposal).title === "string", - ); + const drafts = raw + .filter( + (x): x is Record => + !!x && typeof (x as DraftProposal).title === "string", + ) + .map((d) => ({ + ...(d as unknown as DraftProposal), + the_work: asCellWork((d as Record).the_work), + })); if (drafts.length === 0) return null; const title = (data as Record).title; // Prefer the backend's dropped count; else compute from what we filtered, so a @@ -196,6 +214,128 @@ function batchFromEvent( return { title: typeof title === "string" ? title : "", drafts, dropped }; } +/** Rebuild a draft's ``the_work`` from the set of projects the human selected + * for it (the multi-select review card: one task can span several repos, one + * repo per delivery cell — the backend's ``task_cell_projects`` is unique per + * ``(task, team)``). Existing cell entries keep their summary/items; a newly- + * selected cell gets a minimal entry; a deselected cell's entry is dropped + * (that cell no longer participates). The top-level ``project_id`` is cleared + * — a multi-repo task targets via its cell map, not a top-level repo. Pure, so + * ``setBatchDraftProjects`` can stay a thin setState wrapper and this is + * unit-tested directly. */ +export function rebuildCellWork( + ids: string[], + allProjects: { id: string; assigned_cell?: Team | "" }[], + currentWork: CellWork[], +): { the_work: CellWork[]; project_id: null } { + const cellToPid = new Map(); + for (const id of ids) { + const proj = allProjects.find((p) => p.id === id); + const cell = proj?.assigned_cell; + if (cell && !cellToPid.has(cell)) cellToPid.set(cell, id); + } + const covered = new Set(); + // Update existing cell entries in place, preserve summary/items. + const updated = currentWork.map((w) => { + const team = w?.team; + if (team && cellToPid.has(team)) { + covered.add(team); + return { ...w, project_id: cellToPid.get(team)! }; + } + return w; + }); + // Append a minimal entry for a newly-selected cell not already in the_work. + const appended: CellWork[] = []; + for (const [team, pid] of cellToPid) { + if (!covered.has(team)) { + appended.push({ team, summary: "", items: [], project_id: pid }); + } + } + // Drop entries for cells no longer selected. + const the_work = [...updated, ...appended].filter((w) => { + const team = w?.team; + return team && cellToPid.has(team); + }); + return { the_work, project_id: null }; +} + +/** Fill each draft's missing project assignment from the MegaTask's scoped + * repos, so the review card is pre-filled instead of forcing the human to + * re-pick the projects they already scoped at intake. The intake prompt tells + * the agent to set a top-level ``project_id`` (each task lives in one repo), + * and the backend's scope validator falls back to it — so this mirrors that: + * an explicit per-cell ``the_work[].project_id`` wins; else the draft's + * top-level ``project_id`` (when scoped); else the single scoped repo that + * belongs to this cell, when unambiguous. An empty field stays empty (real + * ambiguity — 2+ scoped repos for the cell — the human picks). Returns the + * same batch reference when nothing changed (idempotent, no render loop). */ +export function fillBatchProjects( + batch: BatchProposal, + projectIds: string[], + allProjects: { id: string; assigned_cell?: Team | "" }[], +): BatchProposal { + const scoped = new Set(projectIds); + const scopedProjects = allProjects.filter((p) => scoped.has(p.id)); + const byCell = (team: Team) => + scopedProjects.filter((p) => p.assigned_cell === team); + const isCell = (team: unknown): team is Team => + typeof team === "string" && + (CELL_TEAMS as readonly string[]).includes(team as Team); + + let changed = false; + const drafts = batch.drafts.map((d) => { + const work = Array.isArray(d.the_work) ? d.the_work : []; + const cellWork = work.filter((w) => isCell(w?.team)); + + // Draft with at least one the_work cell entry: fill each cell's project_id. + if (cellWork.length > 0) { + let workChanged = false; + const newWork = work.map((w) => { + if ( + !w || + !isCell(w.team) || + (w.project_id && scoped.has(w.project_id)) + ) { + return w; + } + // (1) the draft's top-level project_id (the agent's main assignment), + // but only when that repo actually belongs to this cell (a single-cell + // draft's repo should match its one cell; a mismatch is an agent error + // — fall through to auto-assign rather than land the wrong repo). + const top = d.project_id; + if (top && scoped.has(top)) { + const proj = scopedProjects.find((p) => p.id === top); + if (proj && proj.assigned_cell === w.team) { + workChanged = true; + return { ...w, project_id: top }; + } + } + // (2) exactly one scoped repo belongs to this cell. + const matching = byCell(w.team); + if (matching.length === 1) { + workChanged = true; + return { ...w, project_id: matching[0].id }; + } + return w; + }); + if (!workChanged) return d; + changed = true; + return { ...d, the_work: newWork }; + } + + // Legacy draft (no cell the_work): one top-level project_id. Fill it only + // when exactly one scoped repo exists (truly unambiguous). + if (d.project_id && scoped.has(d.project_id)) return d; + if (scopedProjects.length === 1) { + changed = true; + return { ...d, project_id: scopedProjects[0].id }; + } + return d; + }); + + return changed ? { ...batch, drafts } : batch; +} + // --------------------------------------------------------------------------- // Refresh durability // @@ -271,6 +411,10 @@ export function usePrompter() { const [messages, setMessages] = useState([]); const [sessionId, setSessionId] = useState(null); const [isSending, setIsSending] = useState(false); + // Every project (carries assigned_cell), for pre-filling a MegaTask batch's + // per-cell project_ids from the scoped repos (useProjects dedups with the + // batch card's own query by key). + const { data: allProjects = [] } = useProjects(); const [isLaunching, setIsLaunching] = useState(false); /** The latest tool the agent is using — "watch it work" status line. */ const [activity, setActivity] = useState(null); @@ -366,7 +510,15 @@ export function usePrompter() { if (evt.text) { setActivity(null); // first text clears the "preparing…" indicator appendDelta(evt.text); - setState("streaming"); + // Trailing prose AFTER a draft/batch tool call (the agent keeps + // talking once the card is up) must still render in the bubble — + // but it must NOT clobber the preview state, or the card vanishes + // mid-turn and turn_end can't restore it (it only preserves these + // states if they're still set). Keep the card up; the delta lands + // in a fresh bubble (the batch/draft handler cleared streamingId). + setState((s) => + s === "draft_preview" || s === "batch_preview" ? s : "streaming", + ); } break; case "tool_use": @@ -508,6 +660,17 @@ export function usePrompter() { } }, [sessionId, messages, state, editableDraft, batch, batchWaves]); + // Pre-fill each MegaTask draft's project from the scoped repos (the agent + // sets a top-level project_id; the backend falls back to it). Without this + // the review card shows empty Selects and blocks launch, forcing the human + // to re-pick the very projects they scoped at intake. Idempotent: once every + // cell has a project_id it returns the same batch reference (no loop). + useEffect(() => { + if (!batch) return; + const filled = fillBatchProjects(batch, projectIds, allProjects); + if (filled !== batch) setBatch(filled); + }, [batch, projectIds, allProjects]); + // On mount, reconnect to a still-running session left behind by a reload. const didRestoreRef = useRef(false); useEffect(() => { @@ -804,29 +967,33 @@ export function usePrompter() { // Confirm a MegaTask — create the umbrella + sequenced root-subtasks, reap // ----------------------------------------------------------------------- - /** Reassign one cell of one task in the proposed MegaTask to a different - * project. `entryIndex` is the the_work slot (the cell); pass -1 for a legacy - * single-cell draft that has no per-cell map (sets the top-level project_id). - * Project does not affect the wave plan (waves derive from collision surface), - * so the previewed waves stay valid. */ - const updateBatchDraftProject = useCallback( - (index: number, entryIndex: number, projectId: string) => { + /** Set the projects one MegaTask task targets. The review card exposes a + * multi-select checkbox list per task (one task can span several repos), so + * the human picks the whole set at once instead of one dropdown per cell. + * A RoboCo project is per-cell, so the selection maps to one project per + * cell in ``the_work[]`` (the backend's ``task_cell_projects`` is unique per + * ``(task, team)`` — one repo per cell). Existing entries keep their + * summary/items; a newly-selected cell gets a minimal entry; a deselected + * cell's entry is dropped (that cell no longer participates). The top-level + * ``project_id`` is cleared — a multi-repo task targets via its cell map. + * Project choice does not affect the wave plan (waves derive from collision + * surface), so the previewed waves stay valid. */ + const setBatchDraftProjects = useCallback( + (index: number, ids: string[]) => { setBatch((prev) => { if (!prev) return prev; return { ...prev, drafts: prev.drafts.map((d, i) => { if (i !== index) return d; - if (entryIndex < 0) return { ...d, project_id: projectId }; - const the_work = (d.the_work ?? []).map((w, wi) => - wi === entryIndex ? { ...w, project_id: projectId } : w, - ); - return { ...d, the_work }; + const work = Array.isArray(d.the_work) ? d.the_work : []; + const rebuilt = rebuildCellWork(ids, allProjects, work); + return { ...d, the_work: rebuilt.the_work, project_id: null }; }), }; }); }, - [], + [allProjects], ); const confirmBatch = useCallback( @@ -891,10 +1058,24 @@ export function usePrompter() { setCreatedTaskId(result.umbrella_task_id); setCreatedTaskTitle(batch.title.trim() || "MegaTask"); setCreatedTaskTeam(route === "board" ? Team.BOARD : Team.MAIN_PM); - toast.success( - `MegaTask launched — ${result.root_subtask_ids.length} tasks in ` + - `${result.waves.length} wave${result.waves.length === 1 ? "" : "s"}.`, - ); + if (route === "board") { + // Board route: the umbrella + root-subtasks are created HELD for the + // PO + HoM to review — nothing is dispatched yet. The CEO releases the + // sequenced tasks with Approve & Start on the umbrella task once the + // board finishes (the existing CEO gate, not this chat). Say so, so + // "Board review & Start" doesn't read as "launched" the way the + // Main-PM route does. + toast.success( + "Sent to the Board for review — the Product Owner and Head of " + + "Marketing will review this MegaTask. Approve & Start it from the " + + "umbrella task once they're done.", + ); + } else { + toast.success( + `MegaTask launched — ${result.root_subtask_ids.length} tasks in ` + + `${result.waves.length} wave${result.waves.length === 1 ? "" : "s"}.`, + ); + } setState("success"); } catch (err) { toast.error(`Failed to launch MegaTask: ${getErrorMessage(err)}`); @@ -978,7 +1159,7 @@ export function usePrompter() { batch, batchWaves, batchResult, - updateBatchDraftProject, + setBatchDraftProjects, confirmBatch, }; } diff --git a/roboco/agent_sdk/intake_driver.py b/roboco/agent_sdk/intake_driver.py index 24c78892..7d21785b 100644 --- a/roboco/agent_sdk/intake_driver.py +++ b/roboco/agent_sdk/intake_driver.py @@ -56,19 +56,69 @@ class StreamChunk: data: dict[str, Any] = field(default_factory=dict) +def _coerce_to_list(value: Any) -> list[Any]: + """Wrap a lone scalar/dict into a one-element list; drop junk to ``[]``. + + Mirrors ``content.validators.coerce_to_list`` but always returns a list + (never ``None``) and never passes a non-list, non-scalar/dict through — the + panel treats these fields as arrays and would crash on anything else. A + bare string/dict is the well-intentioned single-item case (wrap it); a + number/bool/None is not a work unit, so it is dropped. + """ + if isinstance(value, list): + return value + if isinstance(value, str | dict): + return [value] + return [] + + +# Fields that are lists of plain strings (vs ``the_work``, a list of work-unit +# dicts). The agent may emit these as XML-ish ```` elements, which +# the SDK parses into ``[{"item": {"$text": "…"}}, …]`` — coerce to flat str +# lists so they reach the panel and a VARCHAR[] column as strings, not dicts. +_STR_LIST_FIELDS = ("acceptance_criteria", "what_this_builds", "notes") + + def _coerce_draft(data: Any) -> dict[str, Any] | None: """Return ``data`` as a draft dict (with a string ``title``), else ``None``. - Accepts a dict, or a JSON string the agent may have passed. + Accepts a dict, or a JSON string the agent may have passed. Coerces the + list-shaped spec fields: ``the_work`` (a list of work-unit dicts) is wrapped + to a list, and each ``the_work`` entry's ``items`` plus the string-list + fields (``acceptance_criteria``, ``what_this_builds``, ``notes``) are + flattened to ``list[str]`` — the agent sometimes emits these as XML-ish + ```` elements that the SDK parses into dict wrappers, which + would crash a ``VARCHAR[]`` insert and dump ``str(dict)`` into the rendered + description. The panel renders ``(draft.the_work ?? []).map(...)`` and + throws if ``the_work`` is a bare object, so this is the single choke point + that keeps a non-array from ever reaching SSE. """ + from roboco.foundation.policy.content.validators import coerce_str_list + if isinstance(data, str): try: data = json.loads(data) except (ValueError, TypeError): return None - if isinstance(data, dict) and isinstance(data.get("title"), str): - return data - return None + if not (isinstance(data, dict) and isinstance(data.get("title"), str)): + return None + coerced = dict(data) + if "the_work" in coerced: + coerced["the_work"] = _coerce_to_list(coerced["the_work"]) + for key in _STR_LIST_FIELDS: + if key in coerced: + coerced[key] = coerce_str_list(coerced[key]) + work = coerced.get("the_work") + if isinstance(work, list): + coerced["the_work"] = [ + { + **unit, + "items": coerce_str_list(unit.get("items")), + } + for unit in work + if isinstance(unit, dict) + ] + return coerced def _extract_draft(text: str) -> dict[str, Any] | None: diff --git a/roboco/api/routes/v1/do.py b/roboco/api/routes/v1/do.py index 549a2837..a7f038e7 100644 --- a/roboco/api/routes/v1/do.py +++ b/roboco/api/routes/v1/do.py @@ -77,6 +77,9 @@ async def do_note( "next_steps": body.next_steps, }, section=body.section, + done=body.done, + next=body.next, + where_to_look=body.where_to_look, ) return envelope_to_response(env, request) diff --git a/roboco/api/schemas/v1/do.py b/roboco/api/schemas/v1/do.py index 97052235..aa658b51 100644 --- a/roboco/api/schemas/v1/do.py +++ b/roboco/api/schemas/v1/do.py @@ -72,6 +72,18 @@ class NoteRequest(BaseModel): # (auditor). Validated by the content model server-side. Omit it to write a # developer summary straight from ``text``. section: dict[str, Any] | None = None + # handoff scope — the PM/coordinator RESUMPTION fields, promoted to + # top-level typed strings so the tool schema declares them machine-visible. + # ``section: dict[str, Any]`` renders a schema with no visible sub-fields, + # so a weak model (minimax-m3) emits ``section={}`` and the resumption gate + # rejects ``done — Field required`` — the 2026-06-27 PM respawn-loop + # meltdown. The same model fills top-level ``string`` decision fields fine + # (proven live), so ``done``/``next`` follow that precedent (typed + # ``str = ""`` → schema declares ``string`` not ``anyOf[string, null]``). + # Filled into ``section`` server-side; ignored for non-handoff scopes. + done: str = "" + next: str = "" + where_to_look: list[str] | None = None # List-typed fields tolerate a lone scalar: a single string (or, for # ``options``, a single dict) is wrapped into a one-element list before diff --git a/roboco/api/schemas/v1/flow.py b/roboco/api/schemas/v1/flow.py index 17d48824..6b4cbd32 100644 --- a/roboco/api/schemas/v1/flow.py +++ b/roboco/api/schemas/v1/flow.py @@ -1,9 +1,23 @@ """Request schemas for /api/v1/flow/* intent verbs.""" -from typing import Any +from typing import Annotated, Any from uuid import UUID -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, BeforeValidator, Field, field_validator + +from roboco.foundation.policy.content.validators import coerce_str_list + +# A ``list[str]`` field that tolerates the Claude SDK's XML-ish tool-input +# parsing: an LLM emitting a bullet list as ```` elements arrives +# nested (``[[["…"]]]`` / ``[{"item": {"$text": "…"}}, …]``), which a bare +# ``list[str]`` hard-rejects at validation time (the live ``i_will_plan`` crash: +# ``technical_considerations.1 Input should be a valid string``). The +# ``BeforeValidator`` flattens it to a flat ``list[str]`` first — same +# ``coerce_str_list`` used at the intake→DB boundary (Bug 3, MegaTask memory). +# Applied to EVERY LLM-authored list-of-strings on the flow surface so the SDK +# can't crash any of them (acceptance_criteria, issues, files, ac_verdicts, +# technical_considerations, covers_parent_criteria). +StrList = Annotated[list[str], BeforeValidator(coerce_str_list)] class GiveMeWorkRequest(BaseModel): @@ -28,7 +42,7 @@ class IWillWorkOnRequest(BaseModel): # so re-entry/recovery calls that omit them still pass route validation; # depth + presence are enforced on FRESH dev claims by # choreographer._dev_plan_gate. The dev's `plan` doubles as the approach. - technical_considerations: list[str] = Field(default_factory=list) + technical_considerations: StrList = Field(default_factory=list) risks: list[dict[str, str]] = Field(default_factory=list) open_questions: list[dict[str, str | bool]] = Field(default_factory=list) @@ -111,7 +125,7 @@ class ClaimReviewRequest(BaseModel): class PassReviewRequest(BaseModel): task_id: UUID notes: str = Field(..., min_length=1) - ac_verdicts: list[str] | None = Field( + ac_verdicts: StrList | None = Field( default=None, description=( "One verification entry per acceptance criterion (in criterion " @@ -123,7 +137,7 @@ class PassReviewRequest(BaseModel): class FailReviewRequest(BaseModel): task_id: UUID - issues: list[str] = Field(..., min_length=1) + issues: StrList = Field(..., min_length=1) class ClaimPrReviewRequest(BaseModel): @@ -162,7 +176,7 @@ class PrPassRequest(BaseModel): class PrFailRequest(BaseModel): task_id: UUID - issues: list[str] = Field(..., min_length=1) + issues: StrList = Field(..., min_length=1) class ClaimDocTaskRequest(BaseModel): @@ -172,7 +186,7 @@ class ClaimDocTaskRequest(BaseModel): class IDocumentedRequest(BaseModel): task_id: UUID notes: str = Field(..., min_length=1) - files: list[str] = Field(..., min_length=1) + files: StrList = Field(..., min_length=1) class TriageRequest(BaseModel): @@ -222,7 +236,7 @@ class IWillPlanRequest(BaseModel): default_factory=list, description="List of {title, description} — server assigns id + order", ) - technical_considerations: list[str] = Field(default_factory=list) + technical_considerations: StrList = Field(default_factory=list) risks: list[dict[str, str]] = Field(default_factory=list) open_questions: list[dict[str, str | bool]] = Field(default_factory=list) @@ -253,14 +267,14 @@ class DelegateRequest(BaseModel): estimated_complexity: str = Field(..., min_length=1) # acceptance_criteria is required and non-empty; downstream policy # also denylist-checks each item against placeholder phrases. - acceptance_criteria: list[str] = Field(..., min_length=1) + acceptance_criteria: StrList = Field(..., min_length=1) # Optional per-subtask project override. When omitted, the choreographer # resolves the project from the parent's Product map for this cell, then # falls back to the parent's project. Plain optional field — no validator. project_id: UUID | None = None # Parent acceptance-criterion ids this subtask is responsible for. Lets the # coverage + roll-up AC gates verify every parent AC is claimed and satisfied. - covers_parent_criteria: list[str] | None = None + covers_parent_criteria: StrList | None = None # Pre-gateway parity: cross-field validators that catch the most common # LLM-vs-schema confusions. Pre-gateway lived in diff --git a/roboco/foundation/policy/content/validators.py b/roboco/foundation/policy/content/validators.py index c18c7429..f47b2177 100644 --- a/roboco/foundation/policy/content/validators.py +++ b/roboco/foundation/policy/content/validators.py @@ -101,3 +101,55 @@ def coerce_to_list(value: Any) -> Any: if isinstance(value, str | dict): return [value] return value + + +# Keys an LLM's tool input may wrap a piece of text under. The Claude SDK parses +# XML-ish mixed content the model sometimes emits for a list-of-strings field +# (e.g. ````) into ``{"item": {"$text": "…"}}``; ``$text`` is the +# SDK's marker for element text content. Ordered: most specific first. +_TEXT_KEYS: tuple[str, ...] = ("$text", "text", "item", "value", "content", "summary") + + +def _extract_strs(item: Any) -> list[str]: + """Pull every string out of one list element, recursing through wrappers.""" + if isinstance(item, str): + stripped = item.strip() + return [stripped] if stripped else [] + if isinstance(item, dict): + for key in _TEXT_KEYS: + if key in item: + return _extract_strs(item[key]) + # No recognized key: keep any bare string values the dict carries. + return [ + str(v).strip() for v in item.values() if isinstance(v, str) and v.strip() + ] + if isinstance(item, list): + out: list[str] = [] + for sub in item: + out.extend(_extract_strs(sub)) + return out + return [] + + +def coerce_str_list(value: Any) -> list[str]: + """Coerce a list-of-strings field to a flat ``list[str]``. + + An LLM may emit a ``list[str]`` field (``acceptance_criteria``, ``notes``, + ``what_this_builds``, a work unit's ``items``) as XML-ish ```` + elements, which the Claude SDK parses into ``[{"item": {"$text": "…"}}, …]``. + A bare dict/scalar is the single-item case. This recurses through the + wrappers and returns only strings — never a dict — so the value is safe for a + ``VARCHAR[]`` column (asyncpg rejects non-str elements with a DataError) and + for markdown rendering (``str(dict)`` would otherwise dump ``"{'item': …}"``). + Anything that cannot be reduced to a string is dropped. + """ + if value is None: + return [] + if isinstance(value, str | dict): + value = [value] + if not isinstance(value, list): + return [] + out: list[str] = [] + for item in value: + out.extend(_extract_strs(item)) + return out diff --git a/roboco/mcp/do_server.py b/roboco/mcp/do_server.py index d2e1c357..ce9ce1ae 100644 --- a/roboco/mcp/do_server.py +++ b/roboco/mcp/do_server.py @@ -196,6 +196,9 @@ def note( what_struggled: str = "", next_steps: list[str] | str | None = None, section: dict[str, Any] | None = None, + done: str = "", + next: str = "", + where_to_look: list[str] | None = None, ) -> dict[str, Any]: """Write a journal entry, or (scope='handoff') your note SECTION. @@ -220,9 +223,13 @@ def note( Other journal scopes (note / learning / struggle) just need ``text``. scope='handoff' writes your dedicated SECTION (dev_notes / quick_context / - auditor_notes) instead of the journal: pass ``section={...}`` with the - section's fields (PM/resumption needs done+next; auditor needs - summary+severity), or just ``text`` for a developer summary. + auditor_notes) instead of the journal. For a PM/coordinator RESUMPTION + section pass the TOP-LEVEL fields ``done`` (what's been done) and ``next`` + (the immediate next step) — these are the required fields and they show + up here as discrete string params; ``where_to_look`` is optional. (Do NOT + pass an empty ``section={}``; the ``section`` dict is the free-form path + for other content types — developer ``{summary, changes}``, auditor + ``{summary, severity}``.) Or just ``text`` for a developer summary. """ return _post( "/api/v1/do/note", @@ -241,6 +248,9 @@ def note( "what_struggled": what_struggled, "next_steps": next_steps, "section": section, + "done": done, + "next": next, + "where_to_look": where_to_look, }, ) diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index 166000bb..56879312 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -18,11 +18,25 @@ import json import os import uuid from pathlib import Path -from typing import Any +from typing import Annotated, Any import httpx import structlog from mcp.server.fastmcp import FastMCP +from pydantic import BeforeValidator + +from roboco.foundation.policy.content.validators import coerce_str_list + +# A ``list[str]`` field that tolerates the Claude SDK's XML-ish tool-input +# parsing: an LLM emitting a bullet list as ```` elements arrives +# as ``[[["…"]]]`` / ``[{"item": {"$text": "…"}}, …]`` — nested arrays / dicts, +# not strings. A bare ``list[str]`` annotation hard-rejects element 1 (a list, +# not a str) at the MCP validation layer BEFORE the verb body runs, surfacing as +# ``1 validation error for i_will_planArguments technical_considerations.1 +# Input should be a valid string``. The ``BeforeValidator`` flattens it to a +# flat ``list[str]`` first (same ``coerce_str_list`` used at the intake→DB +# boundary — see Bug 3 in the MegaTask memory). +StrList = Annotated[list[str], BeforeValidator(coerce_str_list)] ORCHESTRATOR_URL = os.environ.get( "ROBOCO_ORCHESTRATOR_URL", @@ -214,7 +228,7 @@ def i_will_work_on( task_id: str, plan: str | None = None, steps: list[dict[str, str]] | None = None, - technical_considerations: list[str] | None = None, + technical_considerations: StrList | None = None, risks: list[dict[str, str]] | None = None, open_questions: list[dict[str, str | bool]] | None = None, ) -> dict[str, Any]: @@ -333,7 +347,7 @@ def claim_review(task_id: str) -> dict[str, Any]: def pass_review( - task_id: str, notes: str, ac_verdicts: list[str] | None = None + task_id: str, notes: str, ac_verdicts: StrList | None = None ) -> dict[str, Any]: """QA: accept the work. notes >= 80 chars; journal:learning required. @@ -347,7 +361,7 @@ def pass_review( return _post(_role_path("pass"), payload) -def fail_review(task_id: str, issues: list[str]) -> dict[str, Any]: +def fail_review(task_id: str, issues: StrList) -> dict[str, Any]: """QA: reject the work with issues. Each issue should be concrete and actionable.""" return _post(_role_path("fail"), {"task_id": task_id, "issues": issues}) @@ -401,7 +415,7 @@ def claim_doc_task(task_id: str) -> dict[str, Any]: return _post(_role_path("claim_doc_task"), {"task_id": task_id}) -def i_documented(task_id: str, notes: str, files: list[str]) -> dict[str, Any]: +def i_documented(task_id: str, notes: str, files: StrList) -> dict[str, Any]: """Doc: mark documentation complete. files=['', ...].""" return _post( _role_path("i_documented"), @@ -463,7 +477,7 @@ def i_will_plan( plan: str, approach: str = "", sub_tasks: list[dict[str, str]] | None = None, - technical_considerations: list[str] | None = None, + technical_considerations: StrList | None = None, risks: list[dict[str, str]] | None = None, open_questions: list[dict[str, str | bool]] | None = None, ) -> dict[str, Any]: @@ -506,9 +520,9 @@ def delegate( team: str, task_type: str, nature: str, - acceptance_criteria: list[str], + acceptance_criteria: StrList, estimated_complexity: str = "medium", - covers_parent_criteria: list[str] | None = None, + covers_parent_criteria: StrList | None = None, ) -> dict[str, Any]: """PM: create a subtask of parent_task_id. @@ -566,7 +580,7 @@ def pr_pass(task_id: str, notes: str) -> dict[str, Any]: return _post(_role_path("pr_pass"), {"task_id": task_id, "notes": notes}) -def pr_fail(task_id: str, issues: list[str]) -> dict[str, Any]: +def pr_fail(task_id: str, issues: StrList) -> dict[str, Any]: """PR reviewer: fail the assembled PR with concrete issues → needs_revision.""" return _post(_role_path("pr_fail"), {"task_id": task_id, "issues": issues}) diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 44035b3f..6f2c04f0 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -4129,40 +4129,60 @@ class AgentOrchestrator: async def _persist_respawn_record( self, agent_slug: str, task_id: str, record: dict[str, Any] ) -> None: - """Write-through one PM-respawn counter row (delete-then-insert upsert). + """Write-through one PM-respawn counter row (atomic upsert). Best-effort, mirroring ``_persist_waiting_record``: a persistence failure must never gate or un-gate a spawn, so any error is logged and swallowed. The counter stays authoritative in memory regardless. + + Unlike ``_persist_waiting_record`` (inline-awaited, one row per agent), + this is scheduled fire-and-forget per gate mutation, and a respawn loop + fires several persists for the same ``(agent_slug, task_id)`` in quick + succession. A delete-then-insert raced under that concurrency: two + transactions for the same key overlapped, the loser's INSERT hit + ``pk_respawn_tracker`` UniqueViolation, the durable count stuck at the + first INSERT's value, and a restart re-burned the strike threshold — the + exact re-burn this feature was built to stop (2026-06-27 live meltdown). + The single ``ON CONFLICT DO UPDATE`` upsert is race-free: concurrent + upserts on the same key serialize at row level. """ try: from uuid import UUID as _UUID - from sqlalchemy import delete + from sqlalchemy.dialects.postgresql import insert as pg_insert from roboco.db.base import get_session_factory from roboco.db.tables import RespawnTrackerTable tid = _UUID(task_id) + now = datetime.now(UTC) + stmt = pg_insert(RespawnTrackerTable).values( + agent_slug=agent_slug, + task_id=tid, + count=int(record["count"]), + last_status=record.get("last_status"), + last_check=record["last_check"], + tracing_resets=int(record.get("tracing_resets", 0)), + notified=bool(record.get("notified", False)), + updated_at=now, + ) + stmt = stmt.on_conflict_do_update( + index_elements=[ + RespawnTrackerTable.agent_slug, + RespawnTrackerTable.task_id, + ], + set_={ + "count": stmt.excluded.count, + "last_status": stmt.excluded.last_status, + "last_check": stmt.excluded.last_check, + "tracing_resets": stmt.excluded.tracing_resets, + "notified": stmt.excluded.notified, + "updated_at": stmt.excluded.updated_at, + }, + ) session_factory = get_session_factory() async with session_factory() as db: - await db.execute( - delete(RespawnTrackerTable).where( - RespawnTrackerTable.agent_slug == agent_slug, - RespawnTrackerTable.task_id == tid, - ) - ) - db.add( - RespawnTrackerTable( - agent_slug=agent_slug, - task_id=tid, - count=int(record["count"]), - last_status=record.get("last_status"), - last_check=record["last_check"], - tracing_resets=int(record.get("tracing_resets", 0)), - notified=bool(record.get("notified", False)), - ) - ) + await db.execute(stmt) await db.commit() except Exception as e: logger.error( diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 83b04f06..81470810 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -4541,6 +4541,7 @@ class Choreographer: have gotten from the upfront completeness check. """ from roboco.foundation.policy.task_completeness import TaskCompletenessError + from roboco.services.base import ValidationError parent_task_id = parent.id try: @@ -4563,6 +4564,24 @@ class Choreographer: task_id=parent_task_id, verb="delegate", ) + except ValidationError as exc: + # A user-input error from task creation (e.g. delegating past + # MAX_TASK_DEPTH — ``_validate_parent_depth`` raises this with a + # remediation message telling the PM to create a sibling instead of + # a further nested subtask). Without this translator it escaped + # uncaught as a 500 ExceptionGroup; the agent never saw the fix. + # ``invalid_state`` carries the message as the remediation so the + # PM gets a clean, actionable rejection it can act on. + return await self._emit_rejection( + Envelope.invalid_state( + message=exc.message, + remediate=exc.message, + context_briefing=briefing, + ).with_introspection(task=parent, role=role_str), + agent_id=pm_agent_id, + task_id=parent_task_id, + verb="delegate", + ) await self._wire_ux_frontend_dependency(new_task, parent) hint = self._sizing_hint(inputs) if hint is not None: diff --git a/roboco/services/gateway/content_actions.py b/roboco/services/gateway/content_actions.py index 6312f54d..0b0e6cfc 100644 --- a/roboco/services/gateway/content_actions.py +++ b/roboco/services/gateway/content_actions.py @@ -36,6 +36,36 @@ if TYPE_CHECKING: logger = structlog.get_logger() +def _merge_resumption_fields( + section: dict[str, Any] | None, + *, + done: str, + next: str, + where_to_look: list[str] | None, +) -> dict[str, Any] | None: + """Fold the top-level resumption fields into the handoff ``section``. + + ``section: dict[str, Any]`` renders a tool schema with no visible + sub-fields, so a weak model (minimax-m3) emits ``section={}`` and the + resumption gate rejects ``done — Field required`` (the 2026-06-27 PM + respawn-loop meltdown). The top-level ``done`` / ``next`` / + ``where_to_look`` string fields are the LLM-facing contract — they show + up in the tool schema as discrete fields the same model fills fine. Here + they fill any keys the explicit ``section`` omits without overwriting + keys the agent already supplied, so a capable model passing ``section`` + directly is unaffected. Returns ``None`` when nothing was supplied so the + downstream ``{'summary': text}`` fallback + gate remediation still fire. + """ + merged: dict[str, Any] = dict(section) if section else {} + if done and "done" not in merged: + merged["done"] = done + if next and "next" not in merged: + merged["next"] = next + if where_to_look and "where_to_look" not in merged: + merged["where_to_look"] = where_to_look + return merged or None + + # Scope catalog is canonical in foundation.policy.journaling. # Derived here as a string frozenset for the existing call sites that # compare strings rather than the Scope enum. @@ -541,6 +571,9 @@ class ContentActions: task_id: UUID | None = None, structured: dict[str, Any] | None = None, section: dict[str, Any] | None = None, + done: str = "", + next: str = "", + where_to_look: list[str] | None = None, ) -> Envelope: """Write a journal entry, or (scope='handoff') the role's note section. @@ -554,6 +587,17 @@ class ContentActions: - decision: context, options[], chosen, rationale, consequences - reflect: what_done, what_learned, what_struggled, next_steps + For ``scope='handoff'`` the resumption section (PM / coordinator + roles) can be authored two ways: the nested ``section`` dict, OR the + top-level ``done`` / ``next`` / ``where_to_look`` string fields. The + top-level path is the LLM-facing contract — ``section: dict[str, Any]`` + renders a tool schema with no visible sub-fields, so a weak model + (minimax-m3) emits ``section={}`` and the resumption gate rejects + ``done — Field required``; the top-level typed strings show up in the + tool schema as discrete fields the same model fills fine (proven on + the decision scope). Top-level fields fill any keys the explicit + ``section`` omits without overwriting supplied ones. + The note is always recorded. List-typed fields tolerate a lone scalar (wrapped into a one-element list) and missing decision/ reflect narrative fields default to a visible placeholder, so a @@ -571,7 +615,12 @@ class ContentActions: # a journal entry. Content quality is enforced by the content model # (apply_structured_note), so skip the journal-text soup check. return await self._record_section_handoff( - agent_id=agent_id, text=text, task_id=task_id, structured=section + agent_id=agent_id, + text=text, + task_id=task_id, + structured=_merge_resumption_fields( + section, done=done, next=next, where_to_look=where_to_look + ), ) return await self._write_journal_note( agent_id=agent_id, diff --git a/roboco/services/gateway/remediation.py b/roboco/services/gateway/remediation.py index 75911eba..7df37df9 100644 --- a/roboco/services/gateway/remediation.py +++ b/roboco/services/gateway/remediation.py @@ -94,6 +94,7 @@ def hint_for_short_quick_context(*, min_chars: int, task_id: str) -> str: return ( f"your quick_context section is empty or under {min_chars} chars. Before " f"delegate, call note(scope='handoff', task_id='{task_id}', " - "section={'done': '', 'next': ''})" - " to leave a resumption handoff, then retry." + "done='', next='') to leave a " + "resumption handoff (pass done and next as top-level string args, not " + "nested in section), then retry." ) diff --git a/roboco/services/prompter.py b/roboco/services/prompter.py index a64886f7..2d8d5ff0 100644 --- a/roboco/services/prompter.py +++ b/roboco/services/prompter.py @@ -22,6 +22,7 @@ from sqlalchemy import select from roboco.db.tables import AgentTable, TaskTable from roboco.foundation.identity import CELL_TEAMS from roboco.foundation.policy.batch import is_batch_umbrella +from roboco.foundation.policy.content.validators import coerce_str_list from roboco.foundation.policy.sequencing.models import DraftSurface, SequencePlan from roboco.models.base import ( AgentRole, @@ -235,6 +236,27 @@ class PrompterService: message="This task draft is missing acceptance criteria.", field="acceptance_criteria", ) + # Flatten the string-list fields to list[str]. The agent sometimes emits + # these as XML-ish elements the SDK parses into dict + # wrappers ({"item": {"$text": "…"}}); left as-is they crash the + # VARCHAR[] insert (asyncpg: "expected str, got dict") and dump str(dict) + # into the rendered description. Coerce here too — a draft can arrive + # via redraft/localStorage, not only the intake choke point. + draft_data["acceptance_criteria"] = coerce_str_list( + draft_data.get("acceptance_criteria") + ) + draft_data["what_this_builds"] = coerce_str_list( + draft_data.get("what_this_builds") + ) + draft_data["notes"] = coerce_str_list(draft_data.get("notes")) + for unit in draft_data.get("the_work") or []: + if isinstance(unit, dict): + unit["items"] = coerce_str_list(unit.get("items")) + if not draft_data["acceptance_criteria"]: + raise ValidationError( + message="This task draft is missing acceptance criteria.", + field="acceptance_criteria", + ) # Recompose the description from the (possibly edited) structured fields — # the task always carries a freshly-composed, consistent description. draft_data["description"] = compose_description(draft_data) @@ -833,8 +855,12 @@ def derive_scale(the_work: list[Any]) -> str: def _clean_list(value: Any) -> list[str]: - """Trimmed, non-empty string items from a possibly-missing list field.""" - return [str(i).strip() for i in (value or []) if str(i).strip()] + """Trimmed, non-empty string items from a possibly-missing list field. + + Uses :func:`coerce_str_list` so an LLM's dict-wrapped items (``{"$text": …}`` + etc.) are extracted to text rather than dumped as ``str(dict)``. + """ + return coerce_str_list(value) def _text(value: Any) -> str: diff --git a/roboco/services/task.py b/roboco/services/task.py index 91fb1d93..53e771e2 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -713,10 +713,14 @@ class TaskService(BaseService): async def _validate_parent_depth(self, parent_task_id: UUID) -> None: """Enforce MAX_TASK_DEPTH at creation time. - Walks up the parent chain counting ancestors. Raises ValueError if - adding a child under this parent would exceed MAX_TASK_DEPTH. - Previously this was only enforced at branch-name generation time, - so invalid hierarchies could be created and only fail later at claim. + Walks up the parent chain counting ancestors. Raises ValidationError + (a ServiceError the API/gateway translate to a clean 400 / remediation + envelope) if adding a child under this parent would exceed + MAX_TASK_DEPTH. Previously this raised a bare ValueError that escaped + uncaught as a 500 (the message told the agent to create a sibling, but + it never reached the agent as a handled error), and before that it was + only enforced at branch-name generation time, so invalid hierarchies + could be created and only fail later at claim. """ from roboco.templates.git.constants import MAX_TASK_DEPTH @@ -726,19 +730,24 @@ class TaskService(BaseService): while current_id is not None: key = str(current_id) if key in visited: - raise ValueError( - f"Circular reference detected at {key} while validating depth" + raise ValidationError( + f"Circular reference detected at {key} while validating depth", + field="parent_task_id", ) visited.add(key) parent = await self.get(current_id) if parent is None: - raise ValueError(f"Parent task {current_id} not found") + raise ValidationError( + f"Parent task {current_id} not found", + field="parent_task_id", + ) depth += 1 if depth >= MAX_TASK_DEPTH: - raise ValueError( + raise ValidationError( f"Task hierarchy would exceed MAX_TASK_DEPTH={MAX_TASK_DEPTH}. " "Create this work as a sibling of the deepest task instead " - "of a further nested subtask." + "of a further nested subtask.", + field="parent_task_id", ) parent_parent = parent.parent_task_id current_id = UUID(str(parent_parent)) if parent_parent else None diff --git a/roboco/templates/git/branch.py b/roboco/templates/git/branch.py index 1661a547..fe283e14 100644 --- a/roboco/templates/git/branch.py +++ b/roboco/templates/git/branch.py @@ -13,7 +13,8 @@ Uses '--' separator for task hierarchy to avoid git ref conflicts. Git cannot have both 'foo' as a branch AND 'foo/bar' as another branch, so we use '--' instead of '/' for the task hierarchy portion. -Max 3 levels deep (root → subtask → sub-subtask). +Max 4 levels deep — MegaTask adds an umbrella Main-PM layer on top of the +normal flow, so the full path is umbrella → root → cell → dev. """ from typing import TYPE_CHECKING @@ -39,7 +40,7 @@ async def build_branch_name( team: str, task_service: "TaskService", ) -> str: - """Build branch name with ancestor path (max 3 levels, full UUIDs). + """Build branch name with ancestor path (max 4 levels, full UUIDs). Args: task_id: The task to create branch for @@ -48,7 +49,9 @@ async def build_branch_name( task_service: TaskService instance for fetching task hierarchy Returns: - Branch name in format: {type}/{team}/{root}--{sub}--{subsub} + Branch name in format: {type}/{team}/{root}--{sub}--{subsub}--{subsubsub} + (MegaTask: umbrella--root--cell--dev; shorter hierarchies omit the + trailing segments). Raises: BranchNameError: If branch_type invalid, task not found, or hierarchy too deep diff --git a/roboco/templates/git/constants.py b/roboco/templates/git/constants.py index 0fcef34b..9fb189e0 100644 --- a/roboco/templates/git/constants.py +++ b/roboco/templates/git/constants.py @@ -33,8 +33,16 @@ COMMIT_TYPES: Final[frozenset[str]] = frozenset( } ) -# Maximum task hierarchy depth (root → subtask → sub-subtask) -MAX_TASK_DEPTH: Final[int] = 3 +# Maximum task hierarchy depth. MegaTask adds one Main-PM layer on top of the +# normal flow, so the full hierarchy is 4 layers: umbrella (Main PM, depth 0) +# → root-subtask (Main PM, depth 1) → cell task (cell PM, depth 2) → dev +# subtask (depth 3). This was sized at 3 for the pre-MegaTask 3-layer flow +# (root→cell→dev = depths 0,1,2) and never raised when MegaTask shipped, so +# cell-PM delegation of dev subtasks was rejected with MAX_TASK_DEPTH=3 and +# deadlocked the cell PM into a respawn loop (2026-06-27 live meltdown). +# The validator rejects a child whose depth would reach MAX_TASK_DEPTH, so 4 +# permits the dev subtask at depth 3 while still capping a 5th layer. +MAX_TASK_DEPTH: Final[int] = 4 # Git branch name character limit GIT_BRANCH_MAX_LENGTH: Final[int] = 255 diff --git a/tests/integration/test_branch_name_builder.py b/tests/integration/test_branch_name_builder.py index 629b12bf..4450f035 100644 --- a/tests/integration/test_branch_name_builder.py +++ b/tests/integration/test_branch_name_builder.py @@ -174,3 +174,50 @@ async def test_get_root_task_id_too_deep_raises() -> None: ) with pytest.raises(BranchNameError, match="too deep"): await get_root_task_id(uuid4(), fake_svc) + + +@pytest.mark.asyncio +async def test_build_branch_name_allows_four_level_megatask_hierarchy() -> None: + """The MegaTask hierarchy is 4 layers — umbrella (Main PM, depth 0) → + root-subtask (Main PM, depth 1) → cell task (cell PM, depth 2) → dev + subtask (depth 3). MAX_TASK_DEPTH was sized for the 3-layer normal flow + (root→cell→dev = depths 0,1,2) and never raised when MegaTask added the + umbrella layer, so cell-PM delegation of dev subtasks was rejected with + MAX_TASK_DEPTH=3 and the cell PM deadlocked into a respawn loop + (2026-06-27 live meltdown — be-pm on task 9980d0a0). The cap must + accommodate the 4-level MegaTask: a dev subtask's branch name builds as a + 4-segment path (umbrella--root--cell--dev), not raise 'too deep'.""" + umbrella, root, cell, dev = uuid4(), uuid4(), uuid4(), uuid4() + tasks = { + dev: SimpleNamespace(id=dev, parent_task_id=cell), + cell: SimpleNamespace(id=cell, parent_task_id=root), + root: SimpleNamespace(id=root, parent_task_id=umbrella), + umbrella: SimpleNamespace(id=umbrella, parent_task_id=None), + } + fake_svc = AsyncMock() + fake_svc.get.side_effect = lambda task_id: tasks[task_id] + + name = await build_branch_name(dev, "feature", "backend", fake_svc) + + assert name == ( + f"feature/backend/{str(umbrella)[:8]}--{str(root)[:8]}" + f"--{str(cell)[:8]}--{str(dev)[:8]}" + ) + + +@pytest.mark.asyncio +async def test_get_root_task_id_returns_umbrella_for_four_level_hierarchy() -> None: + """Companion to the branch-name test: the root of a 4-level MegaTask chain + is the umbrella, and it must be reachable (not truncated as 'too deep') + under the raised cap.""" + umbrella, root, cell, dev = uuid4(), uuid4(), uuid4(), uuid4() + tasks = { + dev: SimpleNamespace(id=dev, parent_task_id=cell), + cell: SimpleNamespace(id=cell, parent_task_id=root), + root: SimpleNamespace(id=root, parent_task_id=umbrella), + umbrella: SimpleNamespace(id=umbrella, parent_task_id=None), + } + fake_svc = AsyncMock() + fake_svc.get.side_effect = lambda task_id: tasks[task_id] + + assert await get_root_task_id(dev, fake_svc) == umbrella diff --git a/tests/integration/test_task_service_misc.py b/tests/integration/test_task_service_misc.py index f1303ad4..625caf72 100644 --- a/tests/integration/test_task_service_misc.py +++ b/tests/integration/test_task_service_misc.py @@ -181,7 +181,7 @@ async def test_validate_parent_depth_missing_parent_raises( task_setup: dict, ) -> None: svc = task_setup["svc"] - with pytest.raises(ValueError, match="not found"): + with pytest.raises(ValidationError, match="not found"): await svc._validate_parent_depth(uuid4()) @@ -190,14 +190,14 @@ async def test_validate_parent_depth_circular_reference( task_setup: dict, db_session: AsyncSession, ) -> None: - """A self-referential parent loop raises ValueError.""" + """A self-referential parent loop raises ValidationError.""" svc = task_setup["svc"] a = await svc.create(_req(task_setup)) b = await svc.create(_req(task_setup, parent_task_id=a.id)) # Force circular: a.parent_task_id = b.id a.parent_task_id = b.id await db_session.flush() - with pytest.raises(ValueError, match="Circular reference"): + with pytest.raises(ValidationError, match="Circular reference"): await svc._validate_parent_depth(a.id) @@ -205,7 +205,7 @@ async def test_validate_parent_depth_circular_reference( async def test_validate_parent_depth_exceeds_max( task_setup: dict, ) -> None: - """Adding a child past MAX_TASK_DEPTH raises ValueError.""" + """Adding a child past MAX_TASK_DEPTH raises ValidationError.""" svc = task_setup["svc"] # Build a chain of MAX_TASK_DEPTH+1 tasks parent = None @@ -215,7 +215,7 @@ async def test_validate_parent_depth_exceeds_max( ) parent = new assert parent is not None - with pytest.raises(ValueError, match="MAX_TASK_DEPTH"): + with pytest.raises(ValidationError, match="MAX_TASK_DEPTH"): await svc._validate_parent_depth(parent.id) diff --git a/tests/unit/agent_sdk/test_intake_driver.py b/tests/unit/agent_sdk/test_intake_driver.py index d51864c8..41ea82a8 100644 --- a/tests/unit/agent_sdk/test_intake_driver.py +++ b/tests/unit/agent_sdk/test_intake_driver.py @@ -224,6 +224,54 @@ def test_normalize_propose_batch_reports_dropped_count() -> None: assert [d["title"] for d in chunks[0].data["drafts"]] == ["Good"] +def test_normalize_propose_batch_coerces_the_work_scalar_to_list() -> None: + # The agent may emit `the_work` as a lone object (single-cell task) or its + # `items` as a lone string instead of a list. The panel treats `the_work` + # as an array (`(draft.the_work ?? []).map`) and crashes if it isn't — so + # the backend coerces these list-shaped fields before they reach SSE, the + # same graceful single-item wrap the content models apply at confirm time. + msg = AssistantMessage( + [ + ToolUseBlock( + "propose_batch", + { + "drafts": [ + { + "title": "Single-cell with bare the_work", + "acceptance_criteria": "one string not a list", + "the_work": { + "team": "backend", + "summary": "do the thing", + "items": "lone item string", + }, + } + ] + }, + ) + ] + ) + chunks = normalize(msg) + assert [c.kind for c in chunks] == ["batch"] + draft = chunks[0].data["drafts"][0] + assert draft["acceptance_criteria"] == ["one string not a list"] + assert isinstance(draft["the_work"], list) and len(draft["the_work"]) == 1 + unit = draft["the_work"][0] + assert unit["team"] == "backend" + assert unit["items"] == ["lone item string"] + # A non-list, non-dict the_work (e.g. a stray string) is dropped so the + # panel never sees a value it cannot .map over. + msg2 = AssistantMessage( + [ + ToolUseBlock( + "propose_batch", + {"drafts": [{"title": "Bad work", "the_work": "not a list"}]}, + ) + ] + ) + draft2 = normalize(msg2)[0].data["drafts"][0] + assert draft2["the_work"] == [] + + def test_normalize_propose_draft_without_title_is_ignored() -> None: msg = AssistantMessage( [ToolUseBlock("propose_draft", {"draft": {"acceptance_criteria": []}})] diff --git a/tests/unit/api/test_schemas_v1_flow.py b/tests/unit/api/test_schemas_v1_flow.py index 60dcbf02..54a0fe8d 100644 --- a/tests/unit/api/test_schemas_v1_flow.py +++ b/tests/unit/api/test_schemas_v1_flow.py @@ -6,7 +6,11 @@ from uuid import uuid4 import pytest from pydantic import ValidationError -from roboco.api.schemas.v1.flow import DelegateRequest +from roboco.api.schemas.v1.flow import ( + DelegateRequest, + IWillPlanRequest, + IWillWorkOnRequest, +) def test_delegate_request_requires_task_type() -> None: @@ -48,3 +52,85 @@ def test_delegate_request_accepts_explicit_task_type() -> None: acceptance_criteria=["returns 200"], ) assert req.task_type == "code" + + +# --------------------------------------------------------------------------- +# StrList — SDK-nested list-of-strings coercion (Bug A) +# --------------------------------------------------------------------------- + + +def test_i_will_plan_request_flattens_sdk_nested_technical_considerations() -> None: + """The Claude SDK parses XML-ish ```` list-of-strings tool + input into nested arrays (``[[["…"]]]``). A bare ``list[str]`` field + hard-rejects element 1 (a list, not a str) at validation time — the live + ``i_will_plan`` crash: ``technical_considerations.1 Input should be a + valid string``. The ``StrList`` BeforeValidator must flatten it to a flat + ``list[str]`` so the verb body receives clean strings. + """ + req = IWillPlanRequest( + task_id=uuid4(), + plan="Plan narrative describing the approach in full sentences.", + approach=( + "Approach text long enough to clear the 150-character minimum " + "enforced on the plan's Approach field so the Plan tab is fully " + "populated for audit and tracing instead of rendering an empty view." + ), + technical_considerations=[ + [[["Empty state distinct from loaded state, coverage target 80%"]]], + [{"item": {"$text": "Use asyncpg prepared statements"}}], + ], + ) + assert req.technical_considerations == [ + "Empty state distinct from loaded state, coverage target 80%", + "Use asyncpg prepared statements", + ] + + +def test_i_will_work_on_request_flattens_dict_wrapped_technical_considerations() -> ( + None +): + """Same coercion on the developer planning verb — a dict-wrapped string + (``{"item": {"$text": "…"}}``, the SDK's element-text marker) must reduce + to the bare string, not ``str(dict)``.""" + req = IWillWorkOnRequest( + task_id=uuid4(), + technical_considerations=[{"item": {"$text": "Cache the lookup result"}}], + ) + assert req.technical_considerations == ["Cache the lookup result"] + + +def test_delegate_request_flattens_sdk_nested_acceptance_criteria() -> None: + """``delegate``'s ``acceptance_criteria`` is the same list-of-strings shape + the SDK can nest (this is the ``delegate``-verb analogue of the MegaTask + Bug 3 crash). The ``StrList`` field must flatten the nested input so the + VARCHAR[] insert downstream never sees a dict/list element.""" + req = DelegateRequest( + parent_task_id=uuid4(), + title="t", + description="add the new endpoint plus tests", + assigned_to="be-dev-1", + team="backend", + task_type="code", + nature="technical", + estimated_complexity="medium", + acceptance_criteria=[ + [[["returns 200 for valid input"]]], + [{"item": {"$text": "rejects malformed input with 400"}}], + ], + ) + assert req.acceptance_criteria == [ + "returns 200 for valid input", + "rejects malformed input with 400", + ] + + +def test_strlist_drops_non_string_junk_instead_of_crashing() -> None: + """Non-string junk (a bare int, a dict with no string values, whitespace) + is dropped — the field never raises on garbage the SDK might emit; only + real strings survive. An all-junk payload yields an empty list (the + delegate min_length=1 gate then rejects it cleanly, not a 500).""" + req = IWillWorkOnRequest( + task_id=uuid4(), + technical_considerations=[42, {"foo": 123}, [[" "]], "real note"], + ) + assert req.technical_considerations == ["real note"] diff --git a/tests/unit/foundation/policy/content/test_validators.py b/tests/unit/foundation/policy/content/test_validators.py index 3ab740ef..d77c9649 100644 --- a/tests/unit/foundation/policy/content/test_validators.py +++ b/tests/unit/foundation/policy/content/test_validators.py @@ -9,7 +9,7 @@ every whitespace token is a placeholder (``wip wip``, ``tbd / na``). from __future__ import annotations import pytest -from roboco.foundation.policy.content.validators import reject_trivial +from roboco.foundation.policy.content.validators import coerce_str_list, reject_trivial def test_returns_trimmed_value_when_substantive() -> None: @@ -57,3 +57,47 @@ def test_rejects_all_filler_token_string(soup: str) -> None: ) def test_accepts_real_text_containing_a_filler_word(ok: str) -> None: assert reject_trivial(ok, field="reason", min_chars=3) == ok.strip() + + +# --------------------------------------------------------------------------- +# coerce_str_list — flatten an LLM's dict-wrapped list-of-strings to list[str]. +# The agent sometimes emits a list[str] field as XML-ish elements +# the Claude SDK parses into {"item": {"$text": "…"}}; left as dicts they crash +# a VARCHAR[] insert ("expected str, got dict") and dump str(dict) into prose. +# --------------------------------------------------------------------------- + + +def test_coerce_str_list_passes_plain_strings_through() -> None: + assert coerce_str_list(["one", "two"]) == ["one", "two"] + + +def test_coerce_str_list_extracts_sdk_item_text_wrapper() -> None: + # The exact shape from the live crash: [{"item": {"$text": "…"}}, ...]. + assert coerce_str_list( + [{"item": {"$text": "UX-UI delivers a sketch"}}, {"item": "Spec lands first"}] + ) == ["UX-UI delivers a sketch", "Spec lands first"] + + +def test_coerce_str_list_wraps_a_bare_dict() -> None: + assert coerce_str_list({"$text": "only one"}) == ["only one"] + + +def test_coerce_str_list_wraps_a_bare_string() -> None: + assert coerce_str_list("only one") == ["only one"] + + +def test_coerce_str_list_drops_non_string_junk() -> None: + # A non-str, non-dict element is dropped — never passed to a VARCHAR[] column. + assert coerce_str_list(["keep", 7, None, {"text": "nested"}]) == [ + "keep", + "nested", + ] + + +def test_coerce_str_list_recurses_into_nested_lists() -> None: + assert coerce_str_list([["a", {"item": "b"}], "c"]) == ["a", "b", "c"] + + +def test_coerce_str_list_none_and_empty() -> None: + assert coerce_str_list(None) == [] + assert coerce_str_list([]) == [] diff --git a/tests/unit/gateway/test_choreographer_delegate_guards.py b/tests/unit/gateway/test_choreographer_delegate_guards.py index 9d1a83b8..8ef88063 100644 --- a/tests/unit/gateway/test_choreographer_delegate_guards.py +++ b/tests/unit/gateway/test_choreographer_delegate_guards.py @@ -16,6 +16,7 @@ from unittest.mock import AsyncMock, MagicMock from uuid import uuid4 import pytest +from roboco.services.base import ValidationError from roboco.services.gateway.choreographer import ( Choreographer, ChoreographerDeps, @@ -284,3 +285,45 @@ async def test_delegate_blocks_when_parent_quick_context_empty() -> None: assert body["error"] == "tracing_gap" assert "quick_context>=min" in body["missing"] task_svc.create_subtask.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_delegate_past_max_depth_returns_invalid_state_not_500() -> None: + """Bug B: delegating past MAX_TASK_DEPTH raised a bare ValueError that + escaped uncaught as a 500 ExceptionGroup. ``_validate_parent_depth`` now + raises ``ValidationError`` (a ServiceError), and the choreographer + translates it to an ``invalid_state`` envelope whose remediate carries the + "create as a sibling" instruction — so the PM gets a clean, actionable + rejection instead of a crash, and the agent loop doesn't respawn-blind. + """ + pm_id = uuid4() + parent_id = uuid4() + parent = MagicMock( + id=parent_id, + project_id=uuid4(), + status="in_progress", + assigned_to=pm_id, + quick_context="Decomposition planned; cells implement their slice next.", + ) + depth_msg = ( + "Task hierarchy would exceed MAX_TASK_DEPTH=4. Create this work as a " + "sibling of the deepest task instead of a further nested subtask." + ) + task_svc = AsyncMock() + task_svc.get.return_value = parent + task_svc.agent_for.return_value = MagicMock(role="cell_pm", team="backend") + task_svc.get_subtasks.return_value = [] + # create_subtask -> create -> _validate_parent_depth raises ValidationError. + task_svc.create_subtask.side_effect = ValidationError( + depth_msg, field="parent_task_id" + ) + deps = _make_deps(task=task_svc) + c = Choreographer(deps) + + env = await c.delegate(pm_id, parent_id, _delegate_inputs()) + body = env.as_dict() + assert body["error"] == "invalid_state" + assert "MAX_TASK_DEPTH" in body["message"] + # The remediation must reach the agent — the whole point of the fix. + assert "sibling" in body["remediate"] + task_svc.create_subtask.assert_awaited_once() diff --git a/tests/unit/gateway/test_note_handoff.py b/tests/unit/gateway/test_note_handoff.py index 26075314..9cb601c8 100644 --- a/tests/unit/gateway/test_note_handoff.py +++ b/tests/unit/gateway/test_note_handoff.py @@ -130,6 +130,96 @@ async def test_handoff_validation_error_returns_remediation_not_422() -> None: assert "auditor" in body["remediate"] +@pytest.mark.asyncio +async def test_handoff_pm_resumption_from_top_level_done_next() -> None: + """A PM authors the resumption section from TOP-LEVEL ``done``/``next`` + fields, not the nested ``section`` dict. + + Background (2026-06-27 live meltdown): minimax-m3:cloud running the PM + roles emitted ``section={}`` (an empty object) for the handoff note 3 + times identically, ignoring the prose remediate. ``ResumptionNote`` requires + ``done``+``next`` so the gate rejected ``done — Field required``, the + do-server circuit breaker tripped after 3-4 rejections, and the tracing + gate (which obligates a handoff note before ``delegate``) deadlocked the + PM into a respawn loop. Root cause was structural: ``section: dict[str, + Any]`` renders a tool schema with NO visible sub-fields, so a weak model + emits ``{}`` while the SAME model fills the top-level ``string`` decision + fields fine (proven live — those succeeded). The fix follows the + precedent already in ``NoteRequest`` (the decision fields typed ``str = + ""`` so the schema declares ``string``): promote ``done``/``next`` to + top-level typed string params so the tool schema shows them + machine-visible. Passing ``done``+``next`` with no ``section`` must write + the resumption section successfully. + """ + agent_id, task_id = uuid4(), uuid4() + svc = _dev_task_svc(task_id, role="cell_pm") + ca = ContentActions(_make_deps(task=svc)) + + env = await ca.note( + agent_id=agent_id, + text="handoff", + scope="handoff", + done="Planned the decomposition into 3 cell tasks.", + next="Cells implement their slices in wave order.", + where_to_look=["panel/tasks — wave column", "the decomposition note"], + ) + + body = env.as_dict() + assert body["error"] is None + _tid, content_type, payload = svc.record_section_note.call_args.args + assert content_type == "resumption" + assert payload["done"] == "Planned the decomposition into 3 cell tasks." + assert payload["next"] == "Cells implement their slices in wave order." + assert payload["where_to_look"] == [ + "panel/tasks — wave column", + "the decomposition note", + ] + + +@pytest.mark.asyncio +async def test_handoff_top_level_done_next_merge_into_explicit_section() -> None: + """Top-level ``done``/``next`` fill any keys the explicit ``section`` omits + without overwriting keys the agent already supplied — backward compatible + with a capable model that passes ``section`` directly.""" + agent_id, task_id = uuid4(), uuid4() + svc = _dev_task_svc(task_id, role="cell_pm") + ca = ContentActions(_make_deps(task=svc)) + + env = await ca.note( + agent_id=agent_id, + text="handoff", + scope="handoff", + section={"done": "Already-specified done."}, + next="Top-level next fills the omitted next.", + ) + + assert env.as_dict()["error"] is None + _tid, _ctype, payload = svc.record_section_note.call_args.args + assert payload["done"] == "Already-specified done." + assert payload["next"] == "Top-level next fills the omitted next." + + +@pytest.mark.asyncio +async def test_handoff_empty_done_next_empty_section_still_remediates() -> None: + """The backstop holds: with no top-level ``done``/``next`` AND an empty + ``section``, the resumption gate still rejects with the remediation + envelope (never a raw 422) — so a model that ignores both paths still + gets the actionable message rather than tripping a silent crash.""" + agent_id, task_id = uuid4(), uuid4() + svc = _dev_task_svc(task_id, role="cell_pm") + svc.record_section_note.side_effect = ContentValidationError( + "done", "field required" + ) + ca = ContentActions(_make_deps(task=svc)) + + env = await ca.note(agent_id=agent_id, text="handoff", scope="handoff", section={}) + body = env.as_dict() + + assert body["error"] == "invalid_state" + assert "done" in body["message"] + assert "resumption" in body["remediate"] + + @pytest.mark.asyncio async def test_handoff_no_task_to_attach_is_rejected() -> None: """With no active/context task and no explicit task_id, handoff refuses.""" diff --git a/tests/unit/runtime/test_respawn_persistence.py b/tests/unit/runtime/test_respawn_persistence.py index b1831f1a..f65d1459 100644 --- a/tests/unit/runtime/test_respawn_persistence.py +++ b/tests/unit/runtime/test_respawn_persistence.py @@ -20,6 +20,7 @@ from uuid import uuid4 import pytest from roboco.runtime.orchestrator import AgentOrchestrator +from sqlalchemy.dialects import postgresql _SEEDED_COUNT = 3 # a persisted strike count, one below the trip threshold _STRIKE_COUNT = 2 @@ -239,6 +240,47 @@ async def test_persist_record_swallows_db_failure() -> None: ) +@pytest.mark.asyncio +async def test_persist_record_uses_atomic_upsert_no_delete_then_insert() -> None: + """Wedge #3 (2026-06-27 live meltdown): the persist did delete-then-insert in + its own transaction. A respawn loop fires count 1->2->3->4 in quick + succession, scheduling a fire-and-forget persist per increment; two of those + for the same (agent_slug, task_id) race in separate transactions and the + loser's INSERT hit pk_respawn_tracker UniqueViolation, so the durable count + stuck at the first INSERT's value and a restart re-burned the strike + threshold — exactly the re-burn this feature was built to stop. The persist + must be a single atomic ON CONFLICT DO UPDATE upsert (no DELETE, no db.add): + concurrent upserts on the same key serialize at row level and never collide. + """ + orch = _new_orchestrator() + db = AsyncMock() + db.execute = AsyncMock(return_value=MagicMock()) + ctx = MagicMock() + ctx.__aenter__ = AsyncMock(return_value=db) + ctx.__aexit__ = AsyncMock(return_value=False) + factory = MagicMock(return_value=ctx) + with patch("roboco.db.base.get_session_factory", return_value=factory): + await orch._persist_respawn_record( + "be-pm", + str(uuid4()), + { + "count": 4, + "last_status": "pending", + "last_check": datetime.now(UTC), + "tracing_resets": 0, + "notified": True, + }, + ) + # One atomic statement, no ORM add, no delete. + assert db.execute.await_count == 1 + db.add.assert_not_called() + stmt = db.execute.await_args.args[0] + sql = str(stmt.compile(dialect=postgresql.dialect())) + assert "ON CONFLICT" in sql + assert "DO UPDATE" in sql + assert "DELETE" not in sql.upper() + + # --------------------------------------------------------------------------- # # Safety regression + transparency # --------------------------------------------------------------------------- #