Skip to content

feat(web): render conductor fleet calls as action cards - #323

Open
saucam wants to merge 2 commits into
mainfrom
feat/p5-1-fleet-action-cards
Open

feat(web): render conductor fleet calls as action cards#323
saucam wants to merge 2 commits into
mainfrom
feat/p5-1-fleet-action-cards

Conversation

@saucam

@saucam saucam commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Second slice of P5.1 (conductor-frontends-design.md §5). Follows #320.

Why

The conductor drives the whole fleet through mcp__codeoid_fleet__* tools, and they render like any other tool — a long prefixed name and a collapsed blob of raw JSON:

mcp__codeoid_fleet__fleet_spawn   [completed]
  ▸ input  {"workdir":"/home/me/Workspace/codeoid","task":"Read README…","shape":"scout",…}

That's the right default for an arbitrary tool and the wrong one here. These few verbs are the conductor's vocabulary — "which session did it pick, what did it send, where did it spawn" is what you open the transcript to find out.

Now:

Spawn scout in codeoid   [act]  [completed]        fleet_spawn · a1b2c3
  shape    scout
  workdir  /home/me/Workspace/codeoid
  backend  claude · opus
  task     Read README.md and report

Shape

Split so the part worth testing needs no reactive root — same as lib/fleet.ts. lib/fleet-cards.ts is pure classification and field extraction; MessageRow renders the result. Ordinary tools are untouched: the classifier returns null and the existing path runs unchanged.

Three decisions that carry the value

Unknown verbs fail safe. The read/send split is security-relevant and enforced daemon-side (FLEET_SEND_TOOL_NAMES — send-class can never be auto-approved). The web can't import daemon code, so the vocabulary is duplicated here and can drift. An unrecognised verb is therefore classified unknown, never observe — so a send-class verb added daemon-side can never render here as a harmless read. Pinned by a test.

input is model-generated and typed unknown. Every field is narrowed, not cast. A wrong-typed or hallucinated field is omitted rather than rendered — an invalid shape produces no shape claim at all. Asserted against missing / null / string / number / array inputs.

The input is read off the STATE while awaiting approval. A call at waiting_confirmation carries its complete input on state.input, not tool.input — and that is exactly the card that must be readable, since it's the approval prompt where you decide whether a dispatch runs. Reading only tool.input would have blanked it.

streaming is deliberately not consulted: partialInput is a half-generated fragment, and a card built from it would show a workdir the model hasn't finished writing.

A guess I caught

Field names are taken from the daemon's own zod schemas, not assumed. My first draft had fleet_panel as targets/prompt; it's actually sessions/message, and fleet_send uses session, not name. Near-miss aliases are still tolerated for single-target verbs, because a blank target on an approval prompt is worse than a tolerated alias.

Verification

  • 17 new tests, 180 web tests total — including a hostile/malformed-input block
  • tsc -b, eslint, and production vite build clean
  • The two remaining MessageRow lint warnings are pre-existing on main (verified by linting the stashed baseline)

Component rendering itself still isn't locally testable (the Node <20.19 jsdom gap from #321), but CI runs those suites — as #320 demonstrated.

Next

The Conductor ⇄ Sessions toggle (§3.A) completes P5.1, then P5.2's docked conductor surface with state/fleet.ts over the fleet.subscribe contract.

🤖 Generated with Claude Code

Second slice of P5.1 (docs/conductor-frontends-design.md §5, §12).

The conductor drives the whole fleet through `mcp__codeoid_fleet__*`
tools, and they rendered like any other tool: a long prefixed name and a
collapsed blob of raw JSON. That is the right default for an arbitrary
tool and the wrong one here, because these few verbs ARE the conductor's
vocabulary — "which session did it pick, what did it send, where did it
spawn" is the thing you open the transcript to find out.

Splits the work so the part worth testing needs no reactive root, the
same shape `lib/fleet.ts` uses for grouping: `lib/fleet-cards.ts` holds
classification and field extraction as pure functions, and MessageRow
renders the result. Ordinary tools are untouched — the classifier
returns null and the existing path runs unchanged.

Three decisions carry most of the value.

**Unknown verbs fail safe.** The read/send split is security-relevant and
enforced daemon-side (`FLEET_SEND_TOOL_NAMES`); the web cannot import
daemon code, so the vocabulary is duplicated and can drift. An
unrecognised verb is therefore classified `unknown`, never `observe`, so
a send-class verb added daemon-side can never render here as a harmless
read. Pinned by a test.

**Input is model-generated and typed `unknown`.** Every field is narrowed
rather than cast, and a wrong-typed or hallucinated field is omitted
rather than rendered — an invalid `shape` produces no shape claim at all.
Asserted against missing/null/string/number/array inputs.

**The input is read off the STATE while awaiting approval.** A call at
`waiting_confirmation` carries its complete input on `state.input`, not
`tool.input` — and that is exactly the card that has to be readable,
since it is the approval prompt where the owner decides whether a
dispatch runs. Reading only `tool.input` would have blanked it. A
`streaming` phase is deliberately NOT consulted: `partialInput` is a
half-generated fragment, and a card built from it would show a workdir
the model has not finished writing.

Field names were taken from the daemon's own zod schemas rather than
guessed — `fleet_panel` uses `sessions`/`message`, not `targets`/`prompt`,
which the first draft had wrong. Near-miss aliases are still tolerated
for the target of single-target verbs, because a blank target on an
approval prompt is worse than a tolerated alias.

The card's left border carries the read/act distinction, and send-class
calls get an `act` badge. `unknown` deliberately does not borrow a colour
it has not earned.

17 new tests (180 web tests total), typecheck, lint and build clean; the
two remaining MessageRow lint warnings are pre-existing on main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@highflame-oracle highflame-oracle Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔮 Oracle Review

🎯 Start Here

web/src/components/transcript/MessageRow.tsx (~15 min) — Logic changes in MessageRow.tsx


📋 PR Summary

What this PR does: Renders conductor fleet tool calls (mcp__codeoid_fleet__*) as structured action cards in the web transcript instead of collapsed raw JSON, via a new pure classification module (lib/fleet-cards.ts) consumed by MessageRow. Ordinary tools are untouched — the classifier returns null for non-fleet tools and the existing render path runs unchanged.

Key changes:

  • New pure classification/extraction module web/src/lib/fleet-cards.ts that maps fleet tool calls to card data (verb, title, fields) with no reactive dependencies, mirroring the lib/fleet.ts split
  • MessageRow renders fleet calls as action cards showing shape, workdir, backend, task, and tool/session identifiers
  • Fail-safe verb classification: unrecognized verbs are classified 'unknown', never 'observe', so a send-class verb added daemon-side can never render as a harmless read
  • All model-generated input fields are narrowed rather than cast; wrongly-typed or hallucinated fields are omitted rather than rendered
  • Input resolution reads state.input while a call is at waiting_confirmation, ensuring approval prompts display the complete dispatch input
  • Field names sourced from the daemon's zod schemas, with near-miss aliases tolerated for single-target verbs to avoid blank approval prompts

Areas affected: web transcript rendering (MessageRow), fleet tool call presentation layer, conductor frontend (P5.1)

Testing notes: 17 new tests (180 web tests total) including a hostile/malformed-input block covering missing, null, string, number, and array inputs; tsc, eslint, and production vite build clean. Component rendering remains untestable locally due to the Node <20.19 jsdom gap from #321, but CI runs those suites; the two remaining MessageRow lint warnings were verified as pre-existing on main.


🔍 Code Review

This is a carefully engineered slice with unusually thoughtful security reasoning — fail-safe classification of unknown verbs, strict narrowing of model-generated input, and reading input from state during approval all demonstrate a clear understanding of where these cards matter most. The pure-module split makes the security-critical logic testable without a reactive root, and verification discipline (schema-sourced field names, baseline lint comparison) is evident throughout. The remaining inline comments are hardening opportunities rather than corrections.

What's good:

  • ✨ Fail-safe classification of unknown verbs as 'unknown' rather than defaulting to 'observe' — correctly treating the daemon/web vocabulary duplication as a drift risk on a security-relevant boundary and pinning it with a test
  • ✨ Narrowing every model-generated field instead of casting, so hallucinated or wrongly-typed input is omitted rather than rendered as fact
  • ✨ Reading input from state.input during waiting_confirmation, recognizing that the approval prompt is exactly where card readability matters most
  • ✨ Deliberately excluding partialInput while streaming to avoid rendering half-generated fields as if they were decisions the model made
  • ✨ Pure-module split (no reactive root) enabling the valuable logic to be tested directly, consistent with the lib/fleet.ts pattern
  • ✨ Sourcing field names from the daemon's zod schemas rather than assuming them, and transparently documenting the near-miss that was caught

Review Stats: suggestion:3


Generated by Oracle - Highflame's AI Code Reviewer

Comment thread web/src/lib/fleet-cards.ts Outdated
* workdir that the model has not finished writing.
*/
function resolveToolInput(tool: ToolInfo): unknown {
if (tool.input !== undefined) return tool.input;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion

tool.input !== undefined blocks the state fallback when input is null

ToolInfo.input is typed unknown, so null is a representable value — and explicit nulls survive JSON round-trips while missing keys become undefined. If a call at waiting_confirmation ever arrives with input: null rather than the key absent, this check returns the null, isRecord rejects it, and the card renders fieldless — which is exactly the approval-prompt card this fallback exists to protect. The != null form costs nothing and removes the dependency on how the protocol spells 'no input'. If you've verified the wire format never emits null here, a one-line note in the doc comment would save the next reader from re-litigating it.

Suggested fix:

Suggested change
if (tool.input !== undefined) return tool.input;
function resolveToolInput(tool: ToolInfo): unknown {
if (tool.input != null) return tool.input;
return tool.state.phase === "waiting_confirmation" ? tool.state.input : undefined;
}

Related: web/src/protocol/types.ts

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real bug — fixed in cdbf59c, taking your != null verbatim.

You're right that the wire format doesn't settle it: input is typed unknown, so null is representable, and a JSON round-trip preserves an explicit null while turning a missing key into undefined. The old check returned that null, isRecord rejected it, and the card rendered fieldless — the exact approval-prompt card the fallback exists to protect.

Added the doc note you suggested plus a regression test that sets input: null alongside a populated state.input and asserts the fields still render.

Comment thread web/src/lib/fleet-cards.ts Outdated
* Send-class verbs — act on the fleet, and never auto-approved daemon-side.
* Mirrors `FLEET_SEND_TOOL_NAMES`.
*/
const SEND_VERBS = [

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion

Make the daemon↔web verb mirror a CI assertion, not just a documented risk

The header correctly names drift as the standing risk and the fail-safe default contains the blast radius — but the containment is passive. A send-class verb added daemon-side renders as unknown here forever, and nobody notices unless they're reading transcripts closely. The module can't import daemon code (browser bundle), but a test can — tests don't ship. If the current 'pinned' test only pins the unknown-never-observe behavior for a fixed verb list, consider adding a mirror test that imports FLEET_SEND_TOOL_NAMES from src/daemon/fleet.ts and asserts that no daemon send-class verb ever classifies as a read here. That turns the worst drift direction into a red build on the PR that introduces it. If the daemon module isn't importable from the vitest setup (node-only side effects at import time), a tiny shared constants module both sides consume is the alternative.

Suggested fix:

Suggested change
const SEND_VERBS = [
// fleet-cards.test.ts — tests don't ship to the browser, so they CAN import daemon code
import { FLEET_SEND_TOOL_NAMES } from "../../../src/daemon/fleet";
import { classifyFleetTool } from "./fleet-cards";
it("no daemon send-class verb ever renders as a read here", () => {
for (const verb of FLEET_SEND_TOOL_NAMES) {
const card = classifyFleetTool(makeFleetTool(`mcp__codeoid_fleet__${verb}`));
// `unknown` is acceptable (fail-safe); an observe/read classification is the bug
expect(card === null || card.sendClass || card.kind === "unknown").toBe(true);
}
});

Related: src/daemon/fleet.ts

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took the second option you offered — the shared constants module — rather than the mirror test, in cdbf59c.

I tried the test first. It does import cleanly under vitest, but it fails web's tsc -b: reaching src/daemon/fleet.ts drags bun:sqlite and the Bun global into the web project's typecheck, so web-check goes red for the wrong reason.

The deciding argument was the weaker one though: a tripwire only detects drift, and still only after someone opens a PR that trips it. FLEET_READ_TOOLS / FLEET_SEND_TOOLS / FLEET_TOOL_PREFIX now live in @highflame/codeoid-protocol and both sides import them, so there is no second copy to fall out of date. It sits beside CAPABILITIES, which is already a shared runtime constant there for the same reason. The daemon keeps its existing export names as aliases, so its providers and src/tests/fleet.test.ts are untouched.

The fail-safe stays and is still tested: a verb neither list names — an older client meeting a newer daemon — classifies as unknown, never observe.

Worth noting your suggested test did earn its keep before being replaced. I wrote it, then verified it actually tripped by deleting fleet_panel from the web copy and watching the mirror go red — which is what made it obvious the duplication was the thing to remove rather than police.


export interface FleetCardField {
label: string;
value: string;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion

Pin the plain-text rendering contract on the card model

summary and value are model-generated, and tool inputs can carry text the model lifted from untrusted repo content — prompt injection puts attacker-chosen strings into task/message fields. This module is safe by construction because it only produces strings, but the safety then lives entirely in MessageRow interpolating them as text (Solid's default) rather than through innerHTML or a raw-HTML markdown pass. One doc line on the model stating 'plain text, render via text interpolation only' makes the contract explicit for the rendering slice and for anyone who later adds a rich-text affordance to these cards. Not a fire — Solid escapes by default — just cheap insurance on the boundary.

Suggested fix:

Suggested change
value: string;
/** Plain text, model-generated. Render via text interpolation only — never innerHTML. */
value: string;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in cdbf59c, on both value and summary.

Agreed on the framing — the module is safe by construction because it only emits strings, so the guarantee lives entirely at the render site, and that's exactly the kind of invariant that gets quietly broken later by someone adding a rich-text affordance.

I expanded the note slightly to say why it matters rather than just what to do: the content is doubly untrusted — model-generated, and frequently lifted from repo content the model just read, which is the path a prompt injection takes into a task or message field.

Review follow-up on #323 (Oracle suggestions 1-3).

**The verb lists move to @highflame/codeoid-protocol.** Oracle asked for
a CI assertion that the web's duplicated copy still mirrors the daemon's
`FLEET_SEND_TOOL_NAMES`, since the fail-safe only CONTAINS drift — a
send-class verb added daemon-side would render as `unknown` here
indefinitely and nobody would notice.

A mirror test turned out to be the weaker of the two options Oracle
offered. Tests can import daemon code, but doing so drags `bun:sqlite`
and the `Bun` global into web's `tsc -b`, which fails the web-check job.
More importantly, a tripwire only DETECTS drift. Both sides now import
`FLEET_READ_TOOLS` / `FLEET_SEND_TOOLS` / `FLEET_TOOL_PREFIX` from the
shared protocol package, so there is no second copy to fall out of date.
It sits beside `CAPABILITIES`, which is already a shared runtime constant
there for the same reason.

The daemon keeps its existing export names as aliases, so its providers
and tests are untouched. The fail-safe stays: a verb NEITHER list names —
an older client meeting a newer daemon — still classifies as `unknown`
rather than `observe`.

**`resolveToolInput` now tests `!= null`, not `!== undefined`.** A real
bug. `input` is typed `unknown`, so `null` is representable, and a JSON
round-trip preserves an explicit `null` while turning a missing key into
`undefined`. The old check accepted that `null`, `isRecord` then rejected
it, and the card rendered fieldless — precisely the approval-prompt card
the state fallback exists to protect. Covered by a test.

**The card model now states its rendering contract.** `summary` and
`value` are plain text, to be rendered by text interpolation only, never
`innerHTML` or a raw-HTML markdown pass. The content is doubly untrusted:
model-generated, and often lifted from repo content the model just read,
which is the path a prompt injection takes into a `task` or `message`
field. Solid escapes by default so today's renderer is safe; the note is
for whoever later adds a rich-text affordance.

Web 184 tests, daemon 2434, typecheck and lint clean on both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants