Skip to content

feat(agents): template selector becomes the definition panel's header with private fork-on-edit - #8307

Open
xuejinT wants to merge 1 commit into
mainfrom
feat/agent-template-pane
Open

feat(agents): template selector becomes the definition panel's header with private fork-on-edit#8307
xuejinT wants to merge 1 commit into
mainfrom
feat/agent-template-pane

Conversation

@xuejinT

@xuejinT xuejinT commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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_pane feature 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_owned marker 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.

  • Backend (dashboard/handlers/agents.py, routes): api_agent_fork (create the private copy, record lineage, rebind the crew) and api_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.
  • Lineage (agent_state.py): a sidecar records forked_from / private_to per fork; agent_discovery.list_agents enriches rows with it in one sidecar read per scan.
  • Refresh semantics (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_templates composition is the security-relevant seam of this diff — worth a close read.
  • Frontend (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.
  • i18n: 37 new keys across 13 catalogs; the four destructive-confirm keys glyph-quote their interpolated operands per locale convention and are pinned in destructiveConfirm.test.ts.
  • Review-round hardening (from the first CI review round): both endpoints refuse with 409 stale_binding when 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-startup rebuild_agent_config() call is offloaded via asyncio.to_thread (the fork refresh made the synchronous rebuild heavier); every new error response carries a machine-readable code; the customized header keeps to two action controls (Reset lives inside the change popover) and the popover trigger uses a Lucide ChevronDown; the feature map's Crews row names the new endpoints and panel component.
  • Write discipline (third review round, replacing the earlier point fixes with one invariant): every template-spec mutation — fork create, publish create, publish cleanup, the agent-detail PATCH overwrite, and each step of the background fork refresh — runs off-loop under one cross-process advisory lock (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 through update_config_locked as a binding-only delta with the stale-binding check re-run inside the critical section, replacing the full-snapshot cfg.save() that could silently revert concurrent config writes. Detail-pane errors render through the shared ErrorNotice component (the publish-dialog variant deliberately opts out of the ask-agent hand-off, which would destroy the unsaved name draft).
  • Fourth review round (extending the same discipline to the two stores it had not yet covered): the fork-refresh writer now runs the two governance passes (_apply_allowed_tools_ceiling, _strip_ungoverned_auto_approve) before every fork write — allowedTools/autoApprove never reach the PreToolUse gate, so a fork carrying grants the ceiling later tightened against must be re-filtered on refresh (regression-tested); and the agent_state sidecar'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 through ErrorNotice with 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 — Reviewer vs existing reviewer.json — matching case-insensitive filesystem semantics), reserved-name refusal, owner gating, idempotency, stale_binding refusals 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=True preserves fork-owned fields while applying managed security fields.
  • Frontend component tests for the panel states (bound / customized / private-copy), confirm flows, and the popover.
  • destructiveConfirm.test.ts pins 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

Template-bound agent — selector as panel header

Customized agent — private copy with origin name, change count popover (holding Reset), Save as template

Changed-fields popover — detail on demand

More states

Template dropdown
Own template selected
Built-in, lower panel
Own, lower panel
Own copy, lower panel

Related Issues

no linked issue: this implements the Agents-page redesign agreed in design review; no tracking issue was filed for it.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

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, 500 rebind_failed) instead of leaking an orphan template, and every ErrorNotice site carries an explicit hand-off decision (askAgent or 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), 500 bookkeeping_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), then sanitize_agent_config_governance immediately 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_pane flag is now modeled end-to-end (field, load, save, resolution allow-list — the connections_ui shape), so agent_template_pane: true in config actually reaches the browser and enables the panel; and the DELETE handler's unlink+prune run in one agents_spec_lock hold 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_PATHS mechanism; 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.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5.1) — 🟡 CONCERNS

Premise-level review of 3ba55ffe284fd1227a7a08968fbfacacacdf416e — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The temp-screenshots/ PNGs follow a documented repo convention (1245 already committed; .gitignore:97 names them "committed deliverables"), so I'm not flagging them. Here's the review.

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 False value no caller ever passes.

What this change ships

Intent: make per-crew agent customization safe so editing one crew's template stops silently mutating every crew bound to it. This is an ADDITION (feat), framed honestly.

  1. Fork-on-first-edit: editing a template-bound agent auto-creates a private per-crew copy — justified (DERIVED: shared-template mutation data loss).
  2. "Save as new template" (publish a private copy) — justified.
  3. Scoped "Reset" to origin template — justified.
  4. Template selector becomes the panel header with a "Customized" state — flagged move, declared.
  5. Changed-field-count popover — part of feature.
  6. Private copies hidden from other crews' dropdowns — justified.
  7. Dropdown rows gain source badges — rides along, ships un-flagged to all users, declared.
  8. Agent Templates tab badge → kirocrew_owned neutral marker — relabel, un-flagged, declared.
  9. agent_template_pane config key (default off, soak agent_template_pane: settings-UI registration, then default-on after soak #8567) — justified gate.
  10. Fork-governance spawn gate + sandbox read-only seal on agent_model_state.json(.lock) — justified (DERIVED: allowedTools/autoApprove bypass the PreToolUse gate). [more items exist; these are the 10 most noticeable.]

Subtractions

  • rebuild_agent_config(refresh_forks: bool | Literal["defer"]) (agent.py:334): only True (default) and "defer" are ever passed (grepped patch: 2 "defer" sites, 0 False). Drop the False branch — a defer: bool covers both live states.

[FIRST-PRINCIPLES-REVIEWED] 3ba55ff

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5.1) — 🟡 CONCERNS

Design-level review of 3ba55ffe284fd1227a7a08968fbfacacacdf416e — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 deny_unknown_fields constraint, and the locking/compensation is proportionate. No blocker. One design-owned risk survives: the load-bearing security invariant is captured only in code comments, and the owning spec was not updated.

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

  • The whole seam's soundness rests on one invariant — no code path writes an agent spec from sidecar lineage without corroborating the binding against config.json — enforced today only by convention across fork/publish/reset/_refresh_forked_templates and dense GPT round-NN comments. A future reader who trusts forked_from/private_to alone reintroduces the forged-sidecar → shared-template-mutation path that _WRITE_PROTECTED_HOME_PATHS + corroboration jointly close. This is exactly the class of security invariant that must be pinned by a test and stated in a spec, not left implicit.
  • No docs/system-specs/ file was updated, yet this adds persisted sidecar keys, three public endpoints (fork/publish/reset), a new security-relevant write-protection, and new crew-binding semantics (fork rebinds the crew). AGENTS.md mandates the owning spec (crew-mode.md) be updated in the same commit; the feature-map row alone doesn't capture the invariant or the data model. Docs checklist is unchecked.

Suggestions

  • Add a golden/invariant test asserting every spec-writing path passes through a binding-corroboration check — that converts the code-comment discipline above into an enforced contract and is the cheapest durable protection for the seam.

[DESIGN-REVIEWED] 3ba55ff

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5.1) — 🟡 CONCERNS

UX-level review of 3ba55ffe284fd1227a7a08968fbfacacacdf416e — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 (AgentTemplateDetail.tsx:464-482) cross-fades two differently-keyed motion.spans with different text ("Built-in" → "Customized") rather than one continuous element — borderline, and the PR states a reason + implements an animation, so CONCERNS not BLOCK.

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

  • Chip swap isn't one continuous element. AgentTemplateDetail.tsx:464-482 cross-fades key="source" ("Built-in") out and key="customized" ("Customized") in — two elements, different text, no shared layoutId. Reads as "the badge vanished, a new one appeared" at the fork moment rather than a transformation; frequency low (first edit only) × moderate impact × persistent per fork. Fix: animate one motion.span via layoutId with the source name truncating into the customized state, or accept the cross-fade and state it as the reason.

Evidence gaps

  • Publish dialog ("Save as new template…" → name input + publish action, AgentTemplateDetail.tsx:691-736) appears in no committed screenshot — add a capture of the open dialog.
  • Reset and Switch-template confirm dialogs (handleSelect/resetToOrigin, the "listing what is discarded" copy) appear in no screenshot — add captures of each confirm.
  • Built-in→Customized chip transition has no committed recording (recordings list empty) — needs a .gif/.webm/.mp4 showing the swap; a human must watch it to confirm continuity.

[UX-REVIEWED] 3ba55ff

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3ba55ffe284fd1227a7a08968fbfacacacdf416e and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3ba55ff

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3ba55ffe284fd1227a7a08968fbfacacacdf416e: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3ba55ffe284fd1227a7a08968fbfacacacdf416e — this comment is updated in place on each push.

Review details

The 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 agent_model_state.json and its lock, the write-protection path entry, the strict/lenient _read split with cross-process locking, and the fork-governance write funnel through _write_spec_filesanitize_agent_config_governance — confirms they are internally consistent and fail closed. The removed work-ledger masks correspond to the deleted work-ledger subsystem. The strict all_fork_info() callers are correctly wrapped (roster degrades to {}, fork-refresh fails closed). No candidate survived and I derived no new grounded defect at the required bar.

No findings.

[OPUS-REVIEWED] 3ba55ff

Verdict parsed from the review's SHA-scoped output markers for commit 3ba55ffe284fd1227a7a08968fbfacacacdf416e.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 3ba55ffe284fd1227a7a08968fbfacacacdf416e: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@xuejinT
xuejinT force-pushed the feat/agent-template-pane branch from 0701ce3 to f071f4a Compare September 4, 2026 01:03
@xuejinT

xuejinT commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • stale requests overwrite newer crew bindings — span=d77e9c4049d0 — fixed in f071f4a.

api_agent_fork now refuses with 409 stale_binding (under the config lock) unless cfg.agents[crew].kiro_agent is the template being forked (file stem or declared name), and api_agent_publish carries the same guard after its private-copy validation. Covered by test_fork_stale_binding_409 and test_publish_stale_binding_409.

@xuejinT

xuejinT commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • failed cleanup exposes a private copy — span=d77e9c4049d0 — fixed in f071f4a.

agent_state.prune(source_name) now runs only after the superseded file is actually gone (unlink(missing_ok=True) succeeded); a failed unlink keeps the fork lineage, so the lingering file stays recorded as the crew's private copy instead of surfacing as a shared template. Covered by test_publish_keeps_lineage_when_delete_fails.

@xuejinT

xuejinT commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • new dashboard handlers are missing from the feature map — span=d77e9c4049d0 — fixed in f071f4a.

docs/feature-map/README.md's Crews row now names components/crew/AgentTemplateDetail.tsx (behind agent_template_pane) and the POST /api/agents/detail/{name}/fork / POST /api/agents/detail/{name}/publish endpoints.

@xuejinT

xuejinT commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • fork refresh blocks the event loop — span=954823a62c41 — fixed in f071f4a.

The one call site that ran rebuild_agent_config() directly on the event loop (slack/gateway.py _init_services) now awaits it via asyncio.to_thread, so the rebuild — including the fork refresh this PR adds to it — runs off-loop at startup. Every other caller already offloaded via to_thread.

@xuejinT

xuejinT commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • customized header renders three action controls (origin: validation) — span=297ff4d5c4f6 — fixed in f071f4a.

Reset moved into the changes popover (with the list it undoes); the header row now holds exactly two action controls: the change-count popover trigger and Save-as-new-template. Component tests and the capture-harness self-checks updated; PR screenshots re-captured on the new head.

@xuejinT

xuejinT commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • dropdown affordance bypasses the icon system (origin: validation) — span=297ff4d5c4f6 — fixed in f071f4a.

The text glyph is replaced by a Lucide <ChevronDown className="h-3 w-3" aria-hidden /> inside the trigger.

@xuejinT

xuejinT commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • "agent_template_pane" is removed from GET /api/config/kirocrew — span=dae9cf662ed3 — rebutted.

The flag was never a modelled config field this diff removed — it is an operator-set top-level key. KiroCrewConfig.load() captures unknown top-level sections into _extra_sections and to_dict() re-emits them (src/kiro_crew/config/loader.py:3655), and the GET handler returns _masked_config_dict(cfg) over exactly that dict, so {"agent_template_pane": true} in config.json reaches the frontend and the gate is reachable. useAgentTemplatePane.ts deliberately resolves absent/failed/non-true to false (flag off by default).

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • This PR is OVERLAPPING with PR #2383. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8307: MERGE_DISCUSSION. Two open PRs are independently building 'copy a template so you can edit it', with different naming models (invisible auto-named fork vs explicit Clone), different validation stories (no screening vs a single screened chokepoint that refuses managed specs), and overlapping hunks in handlers/agents.py, AgentsPage.tsx and 12 locale catalogs. Both can ship, but the authoring model and the write chokepoint should be agreed before either lands. Files: src/kiro_crew/dashboard/handlers/agents.py, website/src/pages/AgentsPage.tsx, website/src/i18n/locales/zh-CN.json.
  • This PR is OVERLAPPING with PR #7181. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8307: MERGE_DISCUSSION. Different features (portable project bundles vs per-crew template forks) that collide on three specific code sites, including one assertion whose merged form must account for the new boolean field. Keep both; sequence the merges and fix the shared assertion in whichever lands second. Files: test/test_agent_discovery.py, src/kiro_crew/agent_discovery.py, src/kiro_crew/agent.py.
  • This PR is OVERLAPPING with PR #7443. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8307: MERGE_DISCUSSION. Unrelated user goals sharing the crew editor's file and pane structure. Both should land; expect conflicts in KiroCrewAgentsPage.tsx and handlers/agents.py. Files: website/src/pages/KiroCrewAgentsPage.tsx, src/kiro_crew/dashboard/handlers/agents.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — private copies can be deleted while another crew still references them — fixed in 340f2a8.

Owner binds crew B to crew A's private copy -> A publishes or resets -> cleanup deletes B's template -> B's sessions fail with "Mode not found."

Legitimate, and both halves of the fix line are implemented. (1) Cross-crew private-copy bindings are now rejected with 409 foreign_private_copy at every binding write the API exposes: the binding-only fast path, the generic PUT's kiro_agent assignment, and crew creation (_foreign_private_copy_owner, lenient sidecar read — the spawn gate re-reads strict, so an unreadable sidecar degrades to an allowed bind, not an ungoverned session). The dashboard UI never offered foreign private copies (they are filtered from the shared catalog), so no UX behavior changes. (2) Belt-and-braces for bindings pre-dating the guard: publish's _cleanup_superseded and reset's _delete_copy re-check all crew bindings after the caller's own rebind persisted and SKIP the unlink — file AND lineage kept, so the copy stays surfaced as private — when any other crew still resolves the copy; an unreadable config fails closed (file kept). Tests: test_binding_update_rejects_foreign_private_copy (409 + owner self-bind still allowed), test_publish_keeps_copy_bound_by_another_crew, test_reset_keeps_copy_bound_by_another_crew (operation commits, copy survives with lineage). 330 tests green across the touched suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Alias bindings still permit deletion of an in-use private copy — fixed in 1329358.

Crew B binds the file stem -> owner publishes or resets its declared-name alias -> cleanup deletes B's active template.

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 source_path.stem and reset's includes the resolved copy file's stem. Tests: test_publish_keeps_copy_bound_by_file_stem and test_reset_keeps_copy_bound_by_file_stem seed a copy whose stem differs from its declared name, bind a second crew by the stem, and assert the file and lineage survive the operation. 66 tests green across the three touched suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Reference check races private-copy deletion — fixed in 41f2a7a, as a restructure rather than another patch (this span's third round, per the same-span stall rule).

Concurrent crew binding -> reference check passes -> binding commits -> cleanup deletes its active template.

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: _copy_still_referenced is replaced by _unlink_copy_unless_referenced, which runs the reference scan AND the unlink as one critical section inside update_config_locked's mutate (the same advisory lock every binding-delta write holds), with the spec lock nested inside for the file operation — lock ordering checked: no existing spec-lock section takes the config lock, so the config-outer/spec-inner nesting introduces no deadlock. Both cleanup paths (publish, reset) now call it; a queued binding writer blocks until the check-and-delete completes, and the check reads the config state as of the critical section, not a pre-lock snapshot. Unreadable config still fails closed (file kept); mutate returns None so the lock is held for isolation, never for a config write. Test: test_publish_cleanup_checks_and_unlinks_under_config_lock wraps the locked mutate and asserts the copy file transitions present→deleted INSIDE the callback. Known residual, unchanged by this PR: writers in the cfg.save() family bypass the advisory lock by design (documented in update_config_locked); the bind-time foreign_private_copy guard covers those paths' private-copy bindings before they save. 91 tests green including the config-writer ratchet suite.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Private-copy ownership check races the binding write (origin: validation) — fixed in 8a15878.

Concurrent fork and template switch -> ownership is checked before lineage is recorded -> another crew binds the private copy and receives its later edits.

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 _get_config_lock() around _rebind_crew_locked (which takes it again inside update_config_locked), the ownership check MOVED INTO _rebind_crew_locked's mutate — the same critical section as the binding write, mirroring the staleness check that already re-runs there for exactly this reason. A lineage record landing after any pre-validation is now seen by the in-lock check, which raises _ForeignPrivateCopy(owner); the fast path maps it to the same 409 foreign_private_copy, and its now-redundant racy pre-check was removed so the locked check is the single source. The existing test_binding_update_rejects_foreign_private_copy now exercises the in-lock path (with the pre-check gone, the 409 can only come from inside the mutate) and passes, along with 319 tests across the touched suites. Fork/publish/reset callers rebind to their own copy or a public name, so the new check cannot fire on their legitimate flows. The generic PUT and crew-create paths keep their pre-checks; they write via the documented cfg.save() family that bypasses the advisory lock, so an in-lock check has no lock to ride there — unchanged residual, same as noted in round-32. The Analyze (javascript-typescript) red on this head was a cancelled checkout step (infra), re-running on this push.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Crew creation races private-fork ownership — fixed in d454e9c.

Concurrent create + fork -> pre-lock check sees no lineage -> create binds the newly private copy -> later edits silently affect both crews.

Correct, and fixed exactly per the fix line: the crew-create handler already holds _get_config_lock() around its load-validate-save block, so the ownership check MOVED INSIDE that block, immediately before cfg.agents[name] is assigned; the racy pre-lock check was removed so the in-lock check is the single source, the same structure as round-33's in-mutate check on the rebind path. With this, all three API binding writes check ownership inside their respective write locks: the binding fast path (in-mutate under the cross-process advisory lock), the generic PUT (its kiro_agent assignment was already inside its _get_config_lock() block), and creation (this fix). Test: test_create_rejects_foreign_private_copy (409 foreign_private_copy, no entry persisted). 68 tests green across the touched suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Stem aliases bypass private-copy ownership — fixed in 152e46d.

Stem binding -> lineage lookup misses declared-name entry -> second crew binds and shares the owner's mutable private spec.

Correct — the bind-side twin of round-31's stem gap on the cleanup side. Fixed per the fix line: _foreign_private_copy_owner now resolves a target that has no lineage entry through _load_template_specs (the same declared-name-or-stem resolver the reset path uses) and re-queries lineage under the resolved DECLARED name, so a stem binding to a copy whose stem differs from its declared name is refused with the same 409. Ambiguous names and unreadable directories land in the existing lenient posture (allowed bind; the spawn gate re-reads strict), unchanged. Because the check runs inside the locked rebind mutate (round-33) and inside the create/PUT lock blocks (round-34), the stem resolution inherits the same atomicity. Test: test_binding_update_rejects_foreign_copy_via_stem (spec file owner-copy-file.json declaring owner-copy, lineage recorded under the declared name as the fork endpoint writes it, stem binding → 409 and the on-disk binding untouched). 69 tests green across the touched suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=954823a62c41 — Stem aliases bypass fork governance (origin: validation) — fixed in d059a64.

Private fork bound by file stem -> lineage lookup misses its declared-name entry -> stale auto-approvals execute after policy tightening.

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, require_fork_governance resolves the name through agent_spec_path (the module's existing declared-name-wins resolver, ambiguity raising) and re-queries lineage strict under the resolved DECLARED name; that effective name then also keys the project-shadow check (both spellings checked — the backend resolves either against the project dir) and the recorded-failure check (the refresh records failures under declared names). Resolution errors and ambiguity land in the existing fail-closed branch — a fork the gate cannot rule out does not pass. Test: test_spawn_gate_resolves_stem_bindings_to_declared_names (a failure recorded under the declared name blocks the stem binding; a clean refresh passes it; a stem resolving to a non-fork stays fast-path). 332 tests green across the agent suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=2a024926c4bd — Agent can replace the advisory-lock inode — fixed in 845901a.

Sandboxed interpreter unlinks/recreates the unsealed lock -> concurrent writers lock different inodes -> stale replacement erases private-copy lineage.

Correct, and fixed exactly per the fix line: agent_model_state.json.lock is added to both sandbox seal lists — _CREW_READONLY_LEAVES (macOS seatbelt / Linux mount seal for a present file) and _CREW_PRECREATE_READONLY_FILE_LEAVES (the lock is created on first use, so without pre-creation a sandbox spawned before any lineage write finds it absent and creatable — a sandbox-created lock IS the replaced-inode attack). No sandboxed process legitimately takes this lock: every locker (agent_state's mutators via the dashboard and CLI) runs unsandboxed, so nothing breaks. An empty pre-created lock is absent-equivalent by definition — its content is never read, only its identity is locked. Assertions added to test_agent_state_write_protection.py; 984 tests green across the seal suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=954823a62c41 — Agent can replace the template-spec lock inode — fixed in 4352d60.

Runtime-built path -> sandboxed agent replaces the unsealed lock between concurrent writers -> different inodes are locked and one template edit is silently overwritten.

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: .kirocrew-agents.lock lives in the KIRO AGENTS tree (~/.kiro/agents/), not the crew home, so it cannot ride the crew leaf lists. It gets its own home-relative readonly target (consumed by both the Linux mount seal and the macOS seatbelt, which deny writes and link-minting on it) and its own pre-create entry resolved through kiro_agents_dir() in _sealable_absent_ceilings — the lock is created on first use, so without pre-creation a sandbox spawned before any template write finds it absent and creatable. Every legitimate locker (fork/publish/reset handlers, the fork refresh) runs unsandboxed, so nothing breaks; an empty pre-created lock is absent-equivalent since only its identity is locked, never its content. Test: test_template_spec_lock_is_sealed_by_the_sandbox (readonly target present, pre-create file list carries the resolved lock path). 1186 tests green across the seal-consumer suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=fa7ab943e399 — The sandbox seals only the lock, not the governed fork specs — fixed in 14c63bf.

Runtime-built spec path -> sandboxed agent rewrites allowedTools/autoApprove after refresh -> next spawn executes forged grants.

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 .kiro/agents entry rides _CREW_READONLY_TARGETS (Linux mount seal + macOS seatbelt, with the seatbelt's file-link deny stopping a writable alias), a new _resolved_kiro_agents_targets() covers relocated data homes per spawn at both consumption sites (same normpath/never-raise contract as _relocated_crew_targets), and _sealable_absent_ceilings pre-creates the directory so the Linux bind has a target on an install where it does not exist yet — gated on the crew data home existing, preserving the absent-data-home no-scaffolding contract its test pins. Reads stay open (kiro-cli resolves its own spec there); every legitimate writer (fork/publish/reset handlers, the fork refresh, CLI setup) runs unsandboxed. The lock file is subsumed by the directory seal, so its file-level entries were folded in. Test updated to the directory contract; 1186 tests green across the seal-consumer suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=596f8a56c6ac — switching away from an own-copy does not delete the superseded copy or prune its lineage — rebutted: deliberate retention, same design as the round-26 ruling on this behavior.

switching away from an own-copy runs only if (ok) onSelect(v) (→ persistTemplateSwitch, a bare binding PUT), so unlike resetToOrigin/publish it never deletes the superseded <crew>.json copy or prunes its private_to lineage; the file is filtered from kiroAgentOptions

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 private_to lineage keeps it out of the shared catalog (kiroAgentOptions filters it), the bind-time guard (rounds 30/33/34/36) refuses any other crew binding it, the spawn gate governs it, and the refresh keeps re-sanitizing it — and if the owner rebinds to it later, the copy-on-first-edit contract resumes where they left off. An orphan-sweep for copies whose owner switched away permanently is a plausible follow-up outside this PR's scope, but deletion-on-switch would be a data-loss behavior, not a cleanup.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=2b21c22f22ad — one ErrorNotice renders a value fed by both a client-side validation hint and a server rejection — fixed in 70aa43d.

publishError is set from a pure validation hint at setPublishError(i18nT('components.agentTemplateDetail.publish_name_rule')) … AND from a caught server rejection … dressing a hint as an error — the exact shape the rule names

Correct — a blocking: true errors-use-error-notice violation, fixed exactly per the fix line: the state is split into publishNameHint (set by the TEMPLATE_NAME_RE validation branch, rendered as plain muted hint text — nothing failed yet, the name merely does not meet the rule) and publishError (set only by the catch around the publish call, still flowing through ErrorNotice with the existing no-hand-off decision unchanged). Typing in the name field clears both. Test: renders the name rule as a hint, not an ErrorNotice submits -bad, asserts the hint text renders with NO role="alert" in the dialog and no publish call fires. tsc + 61 frontend tests green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=0dfda262228b — Fork governance only guards the unused legacy Kiro spawn path — fixed in b4a8884.

Private fork -> AcpProvider.start() -> AcpRuntime.spawn() -> stale or project-shadowed grants execute.

Correct — the round-29 gate was wired into acp/client.py's spawn while the live path (AcpProviderAcpRuntime) builds its own --agent argv in runtime.py with ensure_agent_materialized but no governance gate. Fixed per the fix line: AcpRuntime's argv builder now runs require_fork_governance(agent, work_dir) immediately after materialization — NOT best-effort, mapped to AcpRuntimeError so the spawn aborts, with the work dir passed so the project-shadow refusal applies here too. Sibling table, per the branch-narrowing rule: (1) runtime --agent spawn — gated by this fix; (2) runtime KAS path — gated in the same push: the KAS projection reads the on-disk spec and transmits its grants into the session over the wire, so _build now calls the gate before materialization and a ForkGovernanceUnresolved fails the session loudly alongside the existing translation error, never silently; (3) legacy client spawn — keeps its round-29 gate; (4) claude-backend runtimes take no --agent from this tree, so no fork spec is consumed. 1276 tests green across the agent + ACP suites; the one red (test_runtime_spawn_passes_installed_path_through_exact_wrappers) fails identically on the pristine pre-change tree — an environment-dependent pass_fds assertion inherited from the base, not introduced here.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=297ff4d5c4f6 — Malformed template fields crash the definition panel — fixed in 1fb7a67.

Non-array tools in a template spec -> raw detail response -> Chips calls shown.map -> panel crashes.

Correct — the spec file is user-editable and the detail response is raw, so detail?.tools || [] passes any truthy non-array to .map. Fixed per the fix line, across the full sibling table of raw-detail consumers in the pane rather than just the flagged line: tools, allowedTools, and deniedCommands normalize through Array.isArray; mcpServers takes Object.keys only for a plain object (an array or string yields no keys rather than index keys); prompt requires a string; and the skills / unmanaged_skills props handed to AgentSkillsEditor (which .maps both) normalize at the pass-through, preserving the existing skills === undefined load-failure notice. Test: renders a malformed spec without crashing feeds a detail response with every list field malformed (string, object, number, null) and asserts the pane settles and renders. tsc + 40 pane/editor tests + the page suites green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Concurrent refresh can block a newly forked template — fixed in 91f5c97.

Fork during deferred refresh -> refresh sees lineage before binding and records failure -> rebind succeeds without clearing it -> future agent spawns abort.

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 _refresh_forked_templates(), which re-runs corroboration with the binding now persisted and rebuilds the failure set from scratch, so the just-forked copy passes the spawn gate. Best-effort for the RESPONSE only: the fork is committed either way, and a refresh failure leaves the gate fail-closed — the correct posture — rather than turning a committed fork into a 500 (same principle as the publish sidecar note). Test: test_fork_clears_a_stale_refresh_failure seeds the interleave's outcome (the copy pre-recorded in the failure set), forks, and asserts the failure is cleared and require_fork_governance passes. 303 tests green across the fork/publish/reset/agent suites.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Publish rollback can delete another crew's active template (origin: validation) — fixed in 06cf4c4.

Publish creates destination -> another crew binds it while owner binding changes -> stale rollback deletes destination -> bound crew fails with "Mode not found."

Correct — the ROLLBACK path still used a bare dest.unlink() while the forward cleanup had been reference-checked since round 32. Fixed per the fix line: _undo_publish now goes through the same reference-aware locked helper (check and unlink as one critical section under the config advisory lock, stem included). The helper's return was widened from a bool to the outcome kind because the rollback must treat the two retention reasons differently: deleted → prune lineage as before; referenced → the destination is in live use as a shared template by the crew that bound it, so it stays exactly that — deleting it breaks that crew and marking it private would misattribute it; error (unlink/config failure) → keep the pre-existing fail-closed behavior of marking it private to the publishing crew, so a file carrying the crew's customizations cannot surface as shared. The two forward-cleanup call sites were updated to the new return type with identical behavior. Test: test_publish_rollback_keeps_destination_bound_by_another_crew forces a rebind failure after a concurrent crew binds the destination, and asserts the file survives with NO private lineage. 339 tests green across the endpoint + agent suites; this push also carries a rebase onto moved main (13 locale files re-merged, pseudolocale regenerated).

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=902a82841f6e — Fork governance fails OPEN in the boot deferral on a thread-start failure — fixed in efee7e8.

Under thread exhaustion (RuntimeError: can't start new thread) at gateway boot, _run_deferred never runs … the except sets _fork_refresh_settled and re-raises … a later fork-backed spawn … returns without raising — the one fail-OPEN path among siblings that all set frozenset({"*"}).

Correct, and fixed exactly per the fix line: the thread-start-failure except now records _fork_refresh_failed = frozenset({"*"}) (with the global declaration the enclosing scope needed) BEFORE setting the settled event, so a boot whose deferred refresh never ran holds every fork-backed spawn at the gate instead of releasing them over an empty failure set — matching the fail-closed posture of the deferred runner and the wrapper. Note: this is the same path this lane's round-27 review analyzed and dismissed as below the falsification bar; the fix costs one assignment and aligns all four sibling paths, so fixing beats relitigating. Verified by the existing gate tests (a "*" entry blocks every fork) plus the full agent suite — 344 tests green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=583f63beb6ea — function-local sanitize_agent_config_governance imports violate top-level-imports — fixed in efee7e8.

Fix: import it once at module scope and remove both local imports.

Fixed as suggested: kiro_crew.platform.governance imports nothing from the dashboard tree (verified — its kiro_crew imports are config.paths and platform.* only), so no cycle prevented the hoist. Imported once at module scope; both function-local imports removed; the two tests that patched the source-module symbol were repointed at the handlers-module binding the hoist creates. 344 tests green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=f963f5452093 — function-local file_lock import violates top-level-imports — rebutted: the lazy import is deliberate and load-bearing.

Fix: move the import to module scope. (origin: validation)

The import site carries its own rationale comment: platform_compat pulls in executors, and agent_state's near-leaf import contract is what keeps it out of the agent ↔ config.loader cycle. Hoisting it to module scope would re-introduce exactly the import cycle the lazy import exists to break — the rule's intent (no hidden import-time side effects scattered through call paths) is honored by documenting the exception at the site, which is the codebase's established pattern for cycle-breaking lazy imports. The code stays as-is.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=954823a62c41 — Custom prompt paths containing /kiro_crew/ are overwritten — fixed in 9fc7506.

Custom file:///workspace/kiro_crew/prompt.md -> fork refresh -> managed URI replacement -> irreversible configuration loss.

Correct — the bare /kiro_crew/ spelling, meant to cover the installed package, also matches a source CHECKOUT of this repo, where prompt.md is a user's custom file. Fixed per the fix line: the stale-managed-location spellings are now the two crew data homes plus the concrete installed-package roots (/site-packages/kiro_crew/, /dist-packages/kiro_crew/) — places the managed prompt has actually lived — and never a bare package-name segment. The current == managed_uri exact-match branch is untouched, so a live dev-mode pointer still heals through that path. Test: test_source_checkout_prompt_preserved_but_site_packages_healed refreshes two forks side by side — a checkout-path prompt survives verbatim while a stale wheel-path pointer heals to the managed URI. 268 agent-suite tests green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=297ff4d5c4f6 — Origin-detail failures are silently treated as no changes — fixed in 9fc7506.

Origin request failure -> originDetail remains undefined -> changes become empty -> Reset and change details disappear.

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 ErrorNotice rendered in the pill's slot, with the hand-off decision made explicitly — askAgent is ON, the same decision as the pane's detail-load notice, because this is a read failure with no draft to lose. The deleted-origin case keeps its designed quiet-hide: a 404 is classified through the api layer's existing isNotFoundError (absent vs failed) and shows no notice. New localized message added to all 12 catalogs with the pseudolocale regenerated. tsc + 117 frontend tests (pane, editor, catalog parity) green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=297ff4d5c4f6 — Malformed fields still crash customized templates — fixed in 97b7098.

Malformed template -> fork-on-edit -> sectionValues calls .join on a non-array -> panel crashes.

Correct — round 43 hardened the render sites but missed sectionValues, the origin-diff helper, which still joined skills/tools/allowedTools/deniedCommands behind a || [] that a truthy non-array sails past, and keyed mcpServers without an object check. Fixed per the fix line: every diffed field is now type-checked before joining or rendering — arrays via Array.isArray, mcpServers via a non-array object check, model/prompt via typeof === 'string' — degrading to empty exactly like the render sites. Test: diffs a malformed copy against a malformed origin without crashing feeds the round-43 malformed fixture through the own-copy diff path and asserts the pane settles with the correct change count. tsc + 28 pane tests green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Fork rollback can delete another crew's active template — fixed in 53a86b6.

Generated copy exposed before lineage -> concurrent crew binds it and owner switches -> stale fork rollback deletes the bound spec.

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 _undo_fork then bare-unlinked the spec that crew was bound to. Fixed per the fix line: _undo_fork now routes through _unlink_copy_unless_referenced — the same reference-check-and-unlink critical section (config lock outer, spec lock inner) the publish and reset cleanups use — so a referenced file is KEPT and only the failed fork's lineage is pruned (the kept file equals its source content, so it lists as an ordinary shared template); an unremovable file keeps its lineage, unchanged fail-closed direction. Test: test_fork_rollback_keeps_a_foreign_bound_copy seeds a second crew bound to the generated name, fails the sidecar write, and asserts the file survives with the lineage pruned. 87 endpoint-suite tests green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Fork naming can capture another crew's dangling binding — fixed in 4e0cdd3.

Existing crew bound to missing alpha -> crew alpha forks -> alpha.json is created -> both crews silently share the private copy.

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: _create_copy now runs its whole choose-and-create under update_config_locked (config lock outer, spec lock inner — the same nesting the cleanup paths use) and reserves every current kiro_agent binding read from the locked config, so the name suffixes past a dangling binding and no binding write can land between the reservation scan and the file create. Tests: test_fork_name_skips_another_crews_dangling_binding (a dangling alpha binding forces alpha-2), and the round-50 rollback test was restaged to inject its foreign bind DURING the create-to-lineage window, since a pre-existing binding can no longer reach the rollback (the reservation catches it first). 342 backend tests green.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Publish can capture another crew's dangling binding — fixed in 2973815.

Crew B bound to missing alpha -> Crew A publishes as alpha -> Crew B silently executes Crew A's template.

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: _create_published now runs under update_config_locked (config lock outer, spec lock inner) and REJECTS a name any crew binding currently references — publish never suffixes, so the request 409s with code name_bound before the file exists, and the lock hold means no binding write can land between the check and the create. The publishing crew's own binding cannot false-positive: it points at the source, whose file exists and is already caught by the name_taken pre-check. Test: test_publish_rejects_a_name_a_crew_binding_dangles_at — dangling foreign binding at the requested name → 409, nothing created, the private copy and lineage untouched. 89 endpoint-suite tests green.

Note on the two red CI checks on this head: Backend Tests (Windows) failed with WinError 10055 (runner socket-buffer exhaustion) in test_session_more_coverage.py, a file this PR does not touch, and the offline E2E i18n gate flagged file-explorer/TabStrip.tsx, also untouched by this PR — both re-run on this push; if the E2E finding reproduces deterministically I will apply its suggested one-line min-w-0 fix next round.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Fork is exposed before ownership is recorded / Reset can rebind to a concurrently deleted origin / Fork and publish can capture the global fallback — all three fixed in 16fbc1e.

Concurrent crew bind -> lineage records another owner -> later edits silently affect both crews.

All three correct, fixed per each fix line:

  1. Fork single lock hold_create_record_bind now performs staleness check, name choice, file create, sidecar lineage recording, AND the binding delta inside ONE update_config_locked mutate (spec lock inner), so no locked bind can land between the file appearing and its ownership existing. The staleness check moved FIRST, so nothing is created for a moved binding. The in-lock unwind for a sidecar failure keeps the round-50 posture (a defensive config-file re-read guards against the documented legacy cfg.save() lock-bypassing writer family before unlinking). The old post-create _record_fork_lineage / _rebind_crew_locked / _undo_fork blocks are gone.
  2. Reset origin revalidation_rebind_crew_locked gained require_path: the target's spec file is rechecked for existence INSIDE the critical section under the spec lock; a vanished origin raises and reset 409s origin_missing with the copy kept. Test: test_rebind_refuses_a_vanished_target_file.
  3. Global fallback reservation — the binding scans in fork and publish are now the shared _reserved_binding_names, which folds in agent.default_agent (the legacy global fallback, a resolvable spec reference). Tests: test_fork_name_skips_the_legacy_default_agent_fallback (suffixes past), test_publish_rejects_the_legacy_default_agent_fallback_name (409 name_bound).

346 backend tests green (fork/publish/reset/config/agent suites), flake8/black/isort clean.

@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=6e26220b5826 — Closing drops failures from instant saves — fixed in 16fbc1e.

Model or skill edit -> close before its PATCH settles -> failure renders only in the unmounted pane and the edit is silently lost.

Correct — requestClose held for the template-switch write but not for the pane's shared instant-save chain. Fixed per the fix line: the pane's chain ref is now a notifying box (its .current setter reports every growth through a new onSaveChain prop — covering BOTH writers, the model patch and the skills editor's direct assignment), the parent tracks the latest chain promise in instantSaveInflight, and requestClose defers on it exactly like templateSwitchInflight: the close re-evaluates after the PATCH settles, so a failure renders in the still-mounted pane. Behavior-preserving otherwise — no visual or flow change when no save is in flight. tsc 0; 28 pane tests + the page's discard-guard/private-forks/template-source suites green.

… 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
@xuejinT

xuejinT commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • span=d77e9c4049d0 — Unreadable lineage fails open during private-copy binding — fixed in 3ba55ff.

Corrupt sidecar → create/rebind skips ownership check → after recovery, another crew executes the private definition.

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: _foreign_private_copy_owner now reads lineage with strict=True (both the direct read and the stem-resolution read), any read/resolution failure raises the new _UnverifiableLineage, and every binding writer maps it to 409 lineage_unverifiable — the locked rebind's callers (binding switch, publish with rollback, reset with the copy kept), the generic PUT's fast path, and crew create. Test: test_binding_update_rejects_unverifiable_lineage — a raising sidecar read 409s and the on-disk binding is untouched. 93 endpoint/config tests green.

@bolichen97

Copy link
Copy Markdown
Collaborator

@xuejinT This PR and your own #8497 change the same component in website/src/pages/KiroCrewAgentsPage.tsx, so whichever merges second will conflict textually.

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 mergeable_state dirty, a merge conflict label, and 51 commits behind. #8497 will almost certainly land first.

Suggestion: land #8497 first, then rebase this PR and merge the two prop additions into one TemplateField signature carrying both provenance and editLaterNote, and drop the stale SessionColorField context. While rebasing, please regenerate the locale catalogs from the new base and re-add only this feature's keys; the current catalog diff carries roughly 800 lines of unrelated key reordering, which is where the merge conflict comes from.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants