charles/agent-hooks

Fork 0

fe: roster-assistant proposal preview + accept handler (#1215) #1222

Merged

charles merged 1 commit from code-lead/1215 into main

2026-05-15 21:10:20 +00:00

code-lead commented

2026-05-15 20:58:16 +00:00

Collaborator

Summary

Replaces the JSON debug block in the roster AI assistant sidebar (#1213) with a structured, editable preview, and wires the accept-to-roster handler.

Per-agent card (proposed-agent-card.tsx) with editable name / role / system_prompt / model_tier (+ reason) / skills / tool_permissions / mcp_servers / rationale. Sensitive tools surface an inline warning; MCP chips with setup_required: true get a "setup needed" badge.
Fleet summary header + select-all/none + per-card checkboxes (proposal-preview.tsx). Accept button is disabled when nothing is selected.
Card-level diff against the previous proposal in the same session (proposal-diff.ts): new (green border + badge), removed (ghost card with strike-through + badge), changed (yellow border + field-level "·" markers). "Show changes only" filter collapses unchanged cards.
Catalog-drift warning when the live catalog hash differs from envelope.catalog_hash (non-blocking).
Accept handler (proposal-accept.ts) walks the selected agents and writes each through the manual editor's endpoints: POST /api/config/agent-types (clones a template), PUT /agent-config/system-prompts/:type, per-skill POST /agents/:type/skills, per-MCP PUT /agent-config/mcp/:id. Per-agent failure surfaces inline with a retry; full success closes the sidebar and invalidates roster query caches.
New i18n strings (en + fr).

Tests

proposal-diff.test.ts — pure-logic unit tests for the diff (new/removed/changed/unchanged + rename = remove+add + rationale-drift ignored).
proposal-preview.test.tsx — Vitest browser-mode component tests covering the deliverables: edit doesn't mutate the original envelope, diff highlights changed/new cards, accept passes only the selected drafts, partial failure surfaces the per-card error, catalog-drift warning, select-all + changes-only filter behaviour.
Existing roster-assistant-sidebar.test.tsx adapted to assert the new roster-assistant-preview testid in place of the retired JSON debug block.

Test plan

just qa — typecheck / lint / format / 3417 server tests + 1233 web tests all pass.
Manual: open the sidebar, send a brief, refine once, edit a field, uncheck one agent, accept — verify only the checked + edited agents land in the roster and the sidebar closes.
Manual: simulate a server-side write failure for one agent — verify partial success keeps the failed card visible with an inline error and the others stay written.

Closes #1215.

🤖 Generated with Claude Code

## Summary Replaces the JSON debug block in the roster AI assistant sidebar (#1213) with a structured, editable preview, and wires the accept-to-roster handler. - Per-agent card (`proposed-agent-card.tsx`) with editable `name` / `role` / `system_prompt` / `model_tier` (+ reason) / `skills` / `tool_permissions` / `mcp_servers` / `rationale`. Sensitive tools surface an inline warning; MCP chips with `setup_required: true` get a "setup needed" badge. - Fleet summary header + select-all/none + per-card checkboxes (`proposal-preview.tsx`). Accept button is disabled when nothing is selected. - Card-level diff against the previous proposal in the same session (`proposal-diff.ts`): new (green border + badge), removed (ghost card with strike-through + badge), changed (yellow border + field-level "·" markers). "Show changes only" filter collapses unchanged cards. - Catalog-drift warning when the live catalog hash differs from `envelope.catalog_hash` (non-blocking). - Accept handler (`proposal-accept.ts`) walks the selected agents and writes each through the manual editor's endpoints: `POST /api/config/agent-types` (clones a template), `PUT /agent-config/system-prompts/:type`, per-skill `POST /agents/:type/skills`, per-MCP `PUT /agent-config/mcp/:id`. Per-agent failure surfaces inline with a retry; full success closes the sidebar and invalidates roster query caches. - New i18n strings (en + fr). ## Tests - `proposal-diff.test.ts` — pure-logic unit tests for the diff (new/removed/changed/unchanged + rename = remove+add + rationale-drift ignored). - `proposal-preview.test.tsx` — Vitest browser-mode component tests covering the deliverables: edit doesn't mutate the original envelope, diff highlights changed/new cards, accept passes only the selected drafts, partial failure surfaces the per-card error, catalog-drift warning, select-all + changes-only filter behaviour. - Existing `roster-assistant-sidebar.test.tsx` adapted to assert the new `roster-assistant-preview` testid in place of the retired JSON debug block. ## Test plan - [x] `just qa` — typecheck / lint / format / 3417 server tests + 1233 web tests all pass. - [ ] Manual: open the sidebar, send a brief, refine once, edit a field, uncheck one agent, accept — verify only the checked + edited agents land in the roster and the sidebar closes. - [ ] Manual: simulate a server-side write failure for one agent — verify partial success keeps the failed card visible with an inline error and the others stay written. Closes #1215. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added the

area:agents

area:dashboard

type:user-story

labels

2026-05-15 20:58:17 +00:00

code-lead added 1 commit

2026-05-15 20:58:17 +00:00

feat(web): roster-assistant proposal preview + accept handler (#1215 )

qa / sql-layer-check (pull_request) Successful in 13s

Details

qa / dockerfile (pull_request) Successful in 13s

Details

qa / i18n-string-check (pull_request) Successful in 14s

Details

qa / db-schema (pull_request) Successful in 19s

Details

qa / qa-1 (pull_request) Successful in 3m18s

Details

qa / qa (pull_request) Successful in 0s

Details

52bc12e27d

Replaces the JSON debug block in the roster AI assistant sidebar with a
structured, editable preview: one card per `ProposedAgent` with name, role,
system prompt, model tier (+ reason), skills, tool permissions, MCP servers,
and rationale. Adds select-all / select-none, per-card checkboxes, a card-
level diff against the previous proposal (new / removed / changed kinds +
"show changes only" filter), and a non-blocking catalog-drift warning that
fires when the live catalog hash has shifted since the proposal was
generated.

Accept handler walks the selection and writes each agent through the same
roster endpoints the manual editor uses: `POST /api/config/agent-types`
(clones a template), `PUT /agent-config/system-prompts/:type` for the
system prompt, `POST /agents/:type/skills` per skill from the catalog
snapshot, and `PUT /agent-config/mcp/:id` per MCP. Per-agent failures
surface as inline errors so the operator can fix one and retry without
re-doing the rest. Full success closes the sidebar and invalidates the
roster query caches.

Closes #1215

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead requested review from reviewer

2026-05-15 21:04:19 +00:00

reviewer approved these changes

2026-05-15 21:08:16 +00:00

reviewer left a comment

Review: fe: roster-assistant proposal preview + accept handler (#1215)

CI ✅ green. Mergeable ✅. Read 100% of the diff.

Overview

Replaces the JSON debug <pre> block with a real, editable, diffable proposal preview and wires the complete accept-to-roster path. Five new files (~1 000 lines of product code + ~300 lines of tests), all well-documented with module-level JSDoc. The feature covers every deliverable from the issue: edit isolation, diff highlighting, partial-failure retry, catalog-drift warning, select-all, changes-only filter.

What's good

Draft immutability is solid. makeDrafts creates a Map that's re-keyed from the props envelope. Field edits produce { ...agent, [key]: value } spreads, so the original ProposalEnvelope reference is never mutated. The test asserts this directly.
computeProposalDiff is clean. The canonical() helper correctly sorts primitive arrays (so skill reordering doesn't flag as changed) while leaving object arrays ordered (tool_permissions priority is meaningful). Excluding rationale / model_tier_reason from the compared fields is the right call — those drift every generation.
Rename handling is correct. onCardChange re-keys both drafts and selected when next.name !== name, and onAcceptClick checks selected.has(draft.name) || selected.has(original.name) to catch the mid-edit case.
lastProposalRef pattern is correct. Using a ref to capture the prior proposal before the state update batches is the right way to carry previousProposal forward in runPropose.
Promise.all(tasks) ordering in acceptOne. Step 1 (createAgentType) is awaited before tasks are built, so steps 2–4 can safely parallelise against an already-created type.
Test coverage. proposal-diff.test.ts covers all diff kinds + rename + skills reorder + rationale-drift immunity. proposal-preview.test.tsx covers all six issue deliverables. The sidebar test adapter is minimal and correct.

Issues / notes

1. MCP upsert writes a bare stub even for configured entries — verify nulls are inert (medium)

// proposal-accept.ts  lines ~127-143
tasks.push(
    upsertAgentConfigRow("mcp", mcp.id, {
        scope: "agent_type",
        agent_type: agent.name,
        body: {
            transport: "stdio",
            command: null,
            args: [],
            env: {},
            url: null,
            enabled,
        },
        comment: "roster-assistant proposal accept",
    }),
);

The module header says "we copy them from the catalog's view of the global row when configured", but CatalogSnapshot.mcp_servers only carries { id, name, configured } — no transport details. So every write, even for enabled=true MCPs, sends command: null / url: null.

The manual editor uses the same PUT /agent-config/mcp/:id scope=agent_type endpoint and works fine, so the server presumably doesn't clobber the global row's transport details with per-type nulls. But the comment is misleading and if someone later adds transport fields to CatalogSnapshot they'll expect the accept handler to use them. Please update the comment to say explicitly that the per-type row only sets enabled and the global row's transport is inherited unchanged.

2. Indeterminate checkbox uses inline callback ref — reactive but non-obvious

ref={(el) => {
    if (el) el.indeterminate = !allSelected && someSelected;
}}

This works because React calls inline callback refs on every render (new function reference → old ref called with null, new ref called with the element). It updates indeterminate correctly but the pattern is non-obvious and slightly wasteful. A useEffect + useRef would be cleaner:

const selectAllRef = useRef<HTMLInputElement>(null);
useEffect(() => {
    if (selectAllRef.current) {
        selectAllRef.current.indeterminate = !allSelected && someSelected;
    }
}, [allSelected, someSelected]);
// <input ref={selectAllRef} ...

Not a blocker, but worth tidying.

3. "Show changes only" toggle is visible even when `diff.hasChanges === false`

The control is gated on previous !== null but not on diff.hasChanges. If the operator refines and the model returns the same proposal, the toggle is visible but checking it collapses every card (all unchanged) and shows the empty-state string. Disabling the checkbox when !diff.hasChanges would avoid that confusion:

disabled={!diff.hasChanges}

Minor UX nit; not blocking.

4. No unit tests for `acceptOne` itself

The write path is exercised indirectly (the preview test mocks onAccept at the preview boundary, not at acceptOne). Given partial-failure isolation is a deliverable, a direct test for acceptOne with mocked API functions would be a good addition in a follow-up — particularly for the skill-skip-if-unknown branch and the enabled derivation logic.

Verdict

APPROVED. The core logic is correct, the immutability contract holds, the diff algorithm is sound, and the tests cover the stated deliverables. CI is green. The MCP comment needs a wording fix (item 1) — please land that as a follow-up commit or squash it in before merge; the other items can be tracked as tech-debt. Nothing here blocks shipping.

## Review: fe: roster-assistant proposal preview + accept handler (#1215) CI ✅ green. Mergeable ✅. Read 100% of the diff. --- ### Overview Replaces the JSON debug `<pre>` block with a real, editable, diffable proposal preview and wires the complete accept-to-roster path. Five new files (~1 000 lines of product code + ~300 lines of tests), all well-documented with module-level JSDoc. The feature covers every deliverable from the issue: edit isolation, diff highlighting, partial-failure retry, catalog-drift warning, select-all, changes-only filter. --- ### What's good - **Draft immutability is solid.** `makeDrafts` creates a `Map` that's re-keyed from the props envelope. Field edits produce `{ ...agent, [key]: value }` spreads, so the original `ProposalEnvelope` reference is never mutated. The test asserts this directly. - **`computeProposalDiff` is clean.** The `canonical()` helper correctly sorts primitive arrays (so skill reordering doesn't flag as changed) while leaving object arrays ordered (tool_permissions priority is meaningful). Excluding `rationale` / `model_tier_reason` from the compared fields is the right call — those drift every generation. - **Rename handling is correct.** `onCardChange` re-keys both `drafts` and `selected` when `next.name !== name`, and `onAcceptClick` checks `selected.has(draft.name) || selected.has(original.name)` to catch the mid-edit case. - **`lastProposalRef` pattern is correct.** Using a ref to capture the prior proposal before the state update batches is the right way to carry `previousProposal` forward in `runPropose`. - **`Promise.all(tasks)` ordering in `acceptOne`.** Step 1 (`createAgentType`) is awaited before tasks are built, so steps 2–4 can safely parallelise against an already-created type. - **Test coverage.** `proposal-diff.test.ts` covers all diff kinds + rename + skills reorder + rationale-drift immunity. `proposal-preview.test.tsx` covers all six issue deliverables. The sidebar test adapter is minimal and correct. --- ### Issues / notes #### 1. MCP upsert writes a bare stub even for configured entries — verify nulls are inert (medium) ```ts // proposal-accept.ts lines ~127-143 tasks.push( upsertAgentConfigRow("mcp", mcp.id, { scope: "agent_type", agent_type: agent.name, body: { transport: "stdio", command: null, args: [], env: {}, url: null, enabled, }, comment: "roster-assistant proposal accept", }), ); ``` The module header says *"we copy them from the catalog's view of the global row when configured"*, but `CatalogSnapshot.mcp_servers` only carries `{ id, name, configured }` — no transport details. So every write, even for `enabled=true` MCPs, sends `command: null / url: null`. The manual editor uses the same `PUT /agent-config/mcp/:id scope=agent_type` endpoint and works fine, so the server presumably doesn't clobber the global row's transport details with per-type nulls. But the comment is misleading and if someone later adds transport fields to `CatalogSnapshot` they'll expect the accept handler to use them. **Please update the comment** to say explicitly that the per-type row only sets `enabled` and the global row's transport is inherited unchanged. #### 2. Indeterminate checkbox uses inline callback ref — reactive but non-obvious ```tsx ref={(el) => { if (el) el.indeterminate = !allSelected && someSelected; }} ``` This works because React calls inline callback refs on every render (new function reference → old ref called with `null`, new ref called with the element). It updates `indeterminate` correctly but the pattern is non-obvious and slightly wasteful. A `useEffect` + `useRef` would be cleaner: ```tsx const selectAllRef = useRef<HTMLInputElement>(null); useEffect(() => { if (selectAllRef.current) { selectAllRef.current.indeterminate = !allSelected && someSelected; } }, [allSelected, someSelected]); // <input ref={selectAllRef} ... ``` Not a blocker, but worth tidying. #### 3. "Show changes only" toggle is visible even when `diff.hasChanges === false` The control is gated on `previous !== null` but not on `diff.hasChanges`. If the operator refines and the model returns the same proposal, the toggle is visible but checking it collapses every card (all unchanged) and shows the empty-state string. Disabling the checkbox when `!diff.hasChanges` would avoid that confusion: ```tsx disabled={!diff.hasChanges} ``` Minor UX nit; not blocking. #### 4. No unit tests for `acceptOne` itself The write path is exercised indirectly (the preview test mocks `onAccept` at the preview boundary, not at `acceptOne`). Given partial-failure isolation is a deliverable, a direct test for `acceptOne` with mocked API functions would be a good addition in a follow-up — particularly for the skill-skip-if-unknown branch and the `enabled` derivation logic. --- ### Verdict APPROVED. The core logic is correct, the immutability contract holds, the diff algorithm is sound, and the tests cover the stated deliverables. CI is green. The MCP comment needs a wording fix (item 1) — please land that as a follow-up commit or squash it in before merge; the other items can be tracked as tech-debt. Nothing here blocks shipping.