feat(agents): template selector becomes the definition panel's header with private fork-on-edit - #8307
feat(agents): template selector becomes the definition panel's header with private fork-on-edit#8307xuejinT wants to merge 1 commit into
Conversation
First Principles Review (Fable 5.1) — 🟡 CONCERNSPremise-level review of The First-Principles-Verdict: CONCERNS Sound fix for a real cross-crew data-loss defect; the one soft spot is a three-state refresh flag whose What this change shipsIntent: make per-crew agent customization safe so editing one crew's template stops silently mutating every crew bound to it. This is an ADDITION (
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 3ba55ff |
Design Review (Fable 5.1) — 🟡 CONCERNSDesign-level review of Design assessment complete. The backend analysis confirms the security seam is sound (governance re-projected on every fork refresh, fail-closed spawn gate, lineage never trusted without config corroboration), fork-on-first-edit is the correct model given kiro-cli's Design-Verdict: CONCERNS Sound copy-on-write design with a correctly re-projected governance ceiling; but its load-bearing safety invariant lives only in code comments, and no owning spec was updated. Watch
Suggestions
[DESIGN-REVIEWED] 3ba55ff |
UX Review (Fable 5.1) — 🟡 CONCERNSUX-level review of I have enough to adjudicate. Let me confirm the two flagged surfaces against the diff: the publish dialog and confirm dialogs, and the chip transition mechanism. Reconciliation summary: the PR's added controls are the selector-as-header, the "Customized" chip, the "N changes" popover pill, "Reset my changes" (inside the popover), "Save as new template…", the publish dialog, dropdown source badges, and the Reset/Switch confirm dialogs. Blind reader identified the core controls correctly (selector, Customized tag, changes pill, Save-as-new). No primary control was misread or "no idea"-rated. Reset was "would not dare," but the reader understood it correctly and it's a guarded destructive action — appropriate caution, not a comprehension failure. Two evidence problems: the publish dialog and the two destructive confirm dialogs appear in no committed screenshot, and the Built-in→Customized chip transition (lens 13) has no recording (recordings file is empty). The chip transition ( UX-Verdict: CONCERNS Core new controls read correctly, but the fork/publish dialog and both destructive confirms were never screenshotted, and the Built-in→Customized swap is an unrecorded cross-fade of two different chips. Watch
Evidence gaps
[UX-REVIEWED] 3ba55ff |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe candidate list was empty (discovery found nothing), and my independent review of the load-bearing security changes — the sandbox read-only/pre-create sealing of No findings. [OPUS-REVIEWED] 3ba55ff Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
0701ce3 to
f071f4a
Compare
|
|
|
|
|
|
|
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Legitimate, and both halves of the fix line are implemented. (1) Cross-crew private-copy bindings are now rejected with 409 |
Correct: the round-30 reference check passed only declared names, so a binding by the file STEM (where it differs from the declared name) resolved the same file yet did not block the unlink. Fixed exactly per the fix line: publish's check now includes |
Correct: the check loaded the config and the unlink ran later, so a binding write through the locked delta writer could land in between. Restructured per the fix line: |
Correct — the same TOCTOU shape as round-32's cleanup fix, at the bind-time guard. Fixed equivalently to the fix line but deadlock-free: instead of holding the loader-internal |
Correct, and fixed exactly per the fix line: the crew-create handler already holds |
Correct — the bind-side twin of round-31's stem gap on the cleanup side. Fixed per the fix line: |
Correct — the spawn-gate instance of the stem-vs-declared-name gap closed at the bind guard in round 36. Fixed per the fix line: when the raw-name lineage lookup finds nothing, |
Correct, and fixed exactly per the fix line: |
Correct — the template-spec sibling of round-38's model-state lock, and fixed per the fix line with one structural difference the fix had to account for: |
Correct — the round-39 lock seal protected the lock's identity but left the specs it serializes writable, and the specs ARE what governance sanitizes. Fixed per the fix line: the whole kiro agents tree is now sealed read-only in every sandbox backend — the home-relative |
Retention on switch-away is intentional: a template switch is a non-destructive, instantly reversible navigation action, and deleting the crew's private copy on it would destroy the user's customizations the moment they try a different template — switching back would silently land them on the pristine shared template instead of the copy they edited. The two destructive paths are deliberately explicit gestures with their own confirmation flows: reset (discard my copy, return to origin) and publish (promote my copy to a shared template); both run the copy cleanup, which since round 32 is also the locked, reference-checked deletion path. The retained copy leaks nothing meanwhile: its |
Correct — a |
Correct — the round-29 gate was wired into |
Correct — the spec file is user-editable and the detail response is raw, so |
Correct — an availability race the fail-closed design itself created: the deliberate lineage-before-rebind ordering means a refresh pass interleaving between the two sees an uncorroborated fork and records it in the failure set, and nothing clears it after the rebind lands. Fixed per the fix line: after the successful rebind (outside the endpoint's spec lock — the refresh pass takes it per fork), the endpoint awaits |
Correct — the ROLLBACK path still used a bare |
Correct, and fixed exactly per the fix line: the thread-start-failure except now records |
Fixed as suggested: |
The import site carries its own rationale comment: |
Correct — the bare |
Correct — a transient failure and a deleted origin were indistinguishable, and both silently hid the change pill and its Reset. Fixed per the fix line: the origin query's error is now surfaced through an inline |
Correct — round 43 hardened the render sites but missed |
Correct — the copy FILE is created before its lineage is recorded, so inside that window the bind-time foreign-copy guard sees no private marking and a concurrent crew can legitimately bind the name; the fork's |
Correct — the name chooser avoided existing files, declared names, and reserved basenames, but not names a crew binding currently points at with no file behind it; creating such a file makes the dangling binding resolve to the new private copy. Fixed per the fix line: |
Correct — round 51 closed this on the fork path; publish had the same hazard at its user-chosen destination name. Fixed per the fix line: Note on the two red CI checks on this head: |
All three correct, fixed per each fix line:
346 backend tests green (fork/publish/reset/config/agent suites), flake8/black/isort clean. |
Correct — |
… with private fork-on-edit
Blueprint semantics for the Agents page: editing a crew's template definition
forks a private copy on first edit instead of mutating the shared template.
The template selector becomes the definition panel's header bar, with a
Customized state, a live change-count popover, a scoped 'Reset my changes',
and 'Save as new template'. All behind the agent_template_pane feature flag.
- fork endpoint (POST /api/agents/detail/{name}/fork) + publish endpoint
- fork lineage sidecar (forked_from/private_to) in agent_model_state.json
- sync loop skips auto-creating private copies; fork refresh keeps copies
aligned with owned-template dynamic fields without clobbering user edits
- panel UI: header selector, source badges, Customized tag, changes popover,
scoped reset, save-as-new-template dialog, switch-away confirm
- i18n for 11 locales; 96 backend + 36 frontend tests; capture harness
Correct — the lenient posture was documented as safe because "the spawn gate re-reads strict", but the spawn gate validates GOVERNANCE (refresh, shadowing), not OWNERSHIP: a bind that slipped through the corrupt window persists, and once the sidecar recovers the foreign crew's sessions run the private definition unimpeded. Fixed per the fix line: |
|
@xuejinT This PR and your own #8497 change the same component in What overlaps:
What differs: #8497 declutters the create and edit modal, this PR turns the template selector into the definition panel's header with source badges and private fork-on-edit. Neither implements the other's behaviour, so both should land. Which side is further along: #8497 is +167/-254, mergeable, and 32 commits behind main. This PR is 84 files with Suggestion: land #8497 first, then rebase this PR and merge the two prop additions into one Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
Problem / Motivation
On the Agents page, a crew's agent template is a shared object: editing the prompt, tools, or model of a template while configuring one crew silently mutates that template for every other crew bound to it. There is no safe way to customize an agent for one crew, no visibility that a crew's agent has diverged from the template it came from, and no path to promote a good customization back into a reusable template.
Why it matters
Shared-mutation editing is a silent cross-crew data-loss class: a user tuning one crew's agent breaks the others without any warning, and there is no way back because the original template text is overwritten in place. It also blocks the common workflow of "start from a built-in template, tweak it for this crew".
What changed (motivation → approach → change)
Goal: make per-crew customization safe while keeping templates reusable — blueprint semantics, where a template is a starting point and edits never propagate backward.
Approach chosen: private fork-on-first-edit with the template selector as the definition panel's header (over alternatives of explicit "duplicate template" buttons or an edit-lock on shared templates, both of which push bookkeeping onto the user). The first edit to a template-bound agent transparently creates an auto-named private copy scoped to that crew; the panel header then shows a "Customized" state with the origin template's name.
What was built (behind the
agent_template_panefeature flag):Two small pieces ship un-flagged for all users, deliberately: template dropdown rows gain source badges (built-in / custom / private copy), and the Agent Templates tab's badges move to the shared
kirocrew_ownedmarker with a neutral tone — both are display-only relabels with no behavior change, and gating pure labels would fork the render path for no benefit.dashboard/handlers/agents.py, routes):api_agent_fork(create the private copy, record lineage, rebind the crew) andapi_agent_publish(promote a private copy to a named user-level template). Both owner-gated, config-lock-serialized, path-confined to the agents dir, idempotent.agent_state.py): a sidecar recordsforked_from/private_toper fork;agent_discovery.list_agentsenriches rows with it in one sidecar read per scan.agent.py):_refresh_dynamic_fields(..., fork=True)keeps a fork's own prompt/model instead of overwriting from the template, while security hooks, managed MCP, and the data-home pin still apply;_refresh_forked_templates()(cycle-guarded) keeps forks' managed fields current on setup. Reviewer note: the hooks-refresh /_refresh_forked_templatescomposition is the security-relevant seam of this diff — worth a close read.AgentTemplateDetail.tsx,KiroCrewAgentsPage.tsx): the selector IS the panel header; Customized badge with origin template name; changed-field count in a detail-on-demand popover; scoped Reset (back to origin, with a destructive confirm listing what is discarded); Switch template (same confirm shape); Save as new template. Private copies are filtered out of other crews' template dropdowns.destructiveConfirm.test.ts.409 stale_bindingwhen the crew is no longer bound to the named template (a stale or racing request cannot clobber a newer binding); publish prunes fork lineage only after the superseded file is actually removed (a locked file stays recorded as private instead of surfacing as shared); the gateway-startuprebuild_agent_config()call is offloaded viaasyncio.to_thread(the fork refresh made the synchronous rebuild heavier); every new error response carries a machine-readablecode; the customized header keeps to two action controls (Reset lives inside the change popover) and the popover trigger uses a LucideChevronDown; the feature map's Crews row names the new endpoints and panel component.agents_spec_lock, a sidecar lockfile in the agents dir); spec files are only ever created with exclusive-create (open('x')), so a raced or differing-case destination is refused instead of truncated; template names are reserved case-insensitively (matching APFS/NTFS defaults); and crew rebinds go throughupdate_config_lockedas a binding-only delta with the stale-binding check re-run inside the critical section, replacing the full-snapshotcfg.save()that could silently revert concurrent config writes. Detail-pane errors render through the sharedErrorNoticecomponent (the publish-dialog variant deliberately opts out of the ask-agent hand-off, which would destroy the unsaved name draft)._apply_allowed_tools_ceiling,_strip_ungoverned_auto_approve) before every fork write —allowedTools/autoApprovenever reach the PreToolUse gate, so a fork carrying grants the ceiling later tightened against must be re-filtered on refresh (regression-tested); and theagent_statesidecar's mutating read-modify-writes hold a cross-process file lock (agent_model_state.json.lock), so a dashboard fork racing a CLI model-state write can no longer erase fork lineage via a stale whole-file replacement. Model/reset failures in the pane surface inline throughErrorNoticewith the hint copy declaring "saved as you go" in all 13 catalogs; the fork/publish spec-scan closures are collapsed into one shared loader (call-site attribution ratchet updated to its sanctioned forward form).Tests
test/test_agent_fork_endpoint.py+test/test_agent_publish_endpoint.py(26 tests): fork creates the copy + lineage + rebinding, publish renames/promotes, name-collision 409 (including a differing-case name —Reviewervs existingreviewer.json— matching case-insensitive filesystem semantics), reserved-name refusal, owner gating, idempotency,stale_bindingrefusals for both endpoints, and lineage preserved when the superseded copy cannot be deleted.test/test_agent_discovery_fork_info.py: lineage enrichment and cache invalidation on fork/publish.test/test_agent_refresh_fork.py:fork=Truepreserves fork-owned fields while applying managed security fields.destructiveConfirm.test.tspins the four new confirm keys to the quoted-operand contract in every catalog.Manual verification
Verified with the repo's scripted-Playwright capture harness (
website/scripts/capture-agent-template-pane.mjs) against an isolated instance: template-bound, customized, and private-copy states, the change popover, and the template dropdown. Frames below are the same evidence.Screenshots / video
More states
Related Issues
no linked issue: this implements the Agents-page redesign agreed in design review; no tracking issue was filed for it.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Fifth review round: the shared spec writer now runs
sanitize_agent_config_governance(the sanctioned whole-config funnel) before every fork/publish write, both endpoints compensate on any rebind failure (locked undo, 500rebind_failed) instead of leaking an orphan template, and everyErrorNoticesite carries an explicit hand-off decision (askAgentor a no-hand-off comment). Two regression tests pin the governance filter and the compensation.Sixth review round: every sidecar mutation reachable from an async handler (fork lineage, publish model tracking, the PATCH model branch, DELETE prune) is offloaded with
asyncio.to_thread, so the cross-process sidecar lock never blocks the event loop.Seventh review round: the fork prompt refresh preserves live custom
file://prompts (only the managed pointer and dangling stale pointers are rewritten), and a sidecar bookkeeping failure after fork/publish spec creation now compensates like a rebind failure — failure-resilient undo (unlink runs even if the sidecar prune fails), 500bookkeeping_failed. Five regression tests pin the prompt-guard branches and both compensations.Eighth review round: the PATCH overwrite is now a full read-merge-write inside
agents_spec_lock— fresh re-read, merge of only this patch's delta (a concurrent refresh's changes to untouched keys survive), thensanitize_agent_config_governanceimmediately before persisting. This closes the governance funnel across all four whole-spec writers (PUT, fork, publish, PATCH). Two regression tests pin the sanitized write and the concurrent-merge semantics.Ninth review round: spec writes are durable and portable — a failed exclusive create is unlinked (no truncated name-squatter), the PATCH overwrite is atomic (tmp+replace), fork and publish re-read the source inside the spec lock (no stale copies), post-commit lineage cleanup is non-throwing, and Windows-reserved basenames (CON, NUL, COM1-9…) are refused on publish and suffixed past on fork. Five regression tests.
Tenth review round: publish's superseded-copy cleanup unlinks the RESOLVED source file (a fork whose stem differs from its declared name is no longer missed), and the failed-create unlink runs after the handle closes (Windows sharing violation) and never on FileExistsError (a concurrent creator's file is never ours to remove).
Eleventh review round: the
agent_template_paneflag is now modeled end-to-end (field, load, save, resolution allow-list — theconnections_uishape), soagent_template_pane: truein config actually reaches the browser and enables the panel; and the DELETE handler's unlink+prune run in oneagents_spec_lockhold off the event loop, so a concurrent fork refresh can no longer resurrect a deleted private copy as a shared template.Twelfth review round: the lineage sidecar (and its lock/temp siblings) is write-fenced from agent tools via the existing
_WRITE_PROTECTED_HOME_PATHSmechanism; reset is a server-side transaction (POST /api/agents/detail/{name}/reset— origin and binding validated before any mutation, copy deleted only after the rebind persisted) and the panel calls it; rollback keeps or grants private lineage when a file cannot be removed; ambiguous template names are refused; the sheet footer is hidden while the pane is active (one save model per surface, hint names the reset path in all 13 locales); the Built-in→Customized chip transition is animated with a reduced-motion fallback.