feat(apps): Spec Builder builtin — spec-driven development surface - #518
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
1ed6500 to
01be8c5
Compare
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: |
Arbiter — ✅ no blocking findingsArbiter found no unresolved long-term items that require action before merging Second-order review for Review detailsBoth input files read. The line-level reviewers (Opus 5, GPT 5.6) reported no Medium/Low findings, so the pool to judge is the design reviewer's two Watch items + one suggestion and the UX reviewer's two Watch items + two suggestions. None of them clears the narrow bar:
Arbiter-Verdict: PASS No sub-threshold finding meets the long-term-impact bar. Suggested follow-ups (open as issues — non-blocking)
[ARBITER-REVIEWED] eed0e11 False positive or not applicable? A repository writer can comment: For a broader accepted-risk deferral, apply |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound shape — reuses core chat slots, ChatEmbed, and autonudge instead of a private runner — but the description claims a core fix the diff doesn't contain. Watch
Suggestions
[DESIGN-REVIEWED] 14f96a9 |
01be8c5 to
1de2ea7
Compare
bdfffdc to
a41cfcf
Compare
github-actions
Bot
added
the
readiness: checking
Round 2 disposition — all 5 HIGHs fixed (
|
github-actions
Bot
added
readiness: action required
a41cfcf to
311fbeb
Compare
github-actions
Bot
added
readiness: checking
311fbeb to
646626e
Compare
github-actions
Bot
added
readiness: checking
f7efdcf to
f2f2db3
Compare
github-actions
Bot
removed
the
readiness: action required
|
Round 65 — rebase only, no code change. GPT 5.6 on Why the new SHA. Gates re-run on the rebased tree:
Screenshot URLs in the PR body re-pinned to
|
|
Round 66 — GPT's blocking finding on BLOCKING — routes.py:1491 — "Pause and Delete can relaunch queued work" → FIXEDLegitimate, and confirmed in the upstream code.
So a Pause or Delete that only stopped the turn handed the agent its next prompt: it kept editing the user's spec files after the click, and for Delete it kept writing into a directory the request was about to archive. Fix — one chokepoint, two call sites. Added Deviations from the suggested remedy, both deliberate:
Not absorbed: the gateway's own hard-kill path ( Revert-verified — five independent mutations, each failing a NAMED test:
The source guard asserts on the call order ( Gates: pytest Frontend gates are reused from One test-hygiene note: the two behavioural tests initially passed alone but failed in a full-file run. Cause was mine, not the fix — Screenshot URLs re-pinned to |
|
Round 67 — GPT's blocking finding on BLOCKING — routes.py:2493 — "stale create can delete a replacement spec's worktree" → FIXEDLegitimate, and the blast radius is larger than files. The window is real: between the Fix. Scope — deliberately only this one of four Revert-verified — three mutations, each syntactically valid, each failing a NAMED test:
Two of the three new tests are behavioural (they call Gates: pytest Frontend gates are reused from Screenshot URLs re-pinned to |
|
Round 68 — three findings fixed, and the first round where the local review gate ran before the push rather than after it. Head is now 1. BLOCKING — routes.py:1139 — "Slot identity is recomputed after an await" → FIXEDLegitimate. There were two windows, not one. Beyond the restore await the finding named, Fix: resolve the key once before any await, use that local throughout (including 2. BLOCKING (found locally, pre-push) — routes.py:1390 + :2853 — directory-only identity pins → FIXEDBoth sites pinned on
Worth recording: the local Opus reviewer examined the Three mutations revert-verified (including one that only moves the capture to after the await it must survive). A class guard, 3. Advisory (local Opus) — SpecBuilderPage.tsx:30 — client allowlist wider than the manifest → FIXED
Rebase —
|
|
Rebased onto 25 commits of Two decisions a reviewer shouldn't have to infer1. Main's version exports an identical public API — This is the second round in a row where main independently shipped something this branch carried (round 68 was 2. Reverted an out-of-scope i18n register change. The resolver flagged that this branch had modified An audit script now checks every catalog for keys this PR changed outside its own namespaces ( The mechanical sixteen
One resolution error was caught by the gates and fixed: Gates on
|
|
merge conflicts |
Rebase onto main (48 commits) — head
|
| mutation | named test that fails |
|---|---|
| rollback removed | test_sync_rehydrate_leaves_no_partial_slot, test_async_rehydrate_leaves_no_partial_slot |
preexisting_restricted guard dropped |
test_a_preexisting_restricted_key_is_never_discarded |
adopt_closed ignored on the sync path |
the archived-session restore assertion |
Main's 7 test_rehydrate_async.py tests pass unchanged.
2. i18n-strict:all-caps-const — main's #1099 (84b4ea0d0) landed in these 48 commits
The strict config now inspects literals inside ALL-CAPS module constants, which the
per-file ceilings never counted. Every line in a new file counts as written by this branch,
so all 20 such literals in the Spec Builder fired at once under two zero-tolerance gates.
They are three categories and are treated differently:
- 14 are user-visible copy → catalog keys resolved with
i18nTat the render site,
mirroringPHASE_LABEL_KEY/phaseLabel()incode-review-sage, including its
hasOwnPropertyguard (the phase arrives on a backend payload, sotoStringwould
otherwise resolve to an inherited prototype member and hand a function to i18next).
20 keys × 10 locales. - 2 are CSS values (
color-mix(...)) → moved intoinlineStyles.ts, already exempt by
path for exactly this reason, and re-exported fromshared.tsxso the five importers are
untouched. - 2 are prompt text sent TO the agent (
ADVANCE[].msg, consumed by
messageMutation.mutate) → moved intoprompts.ts, already exempt via main's
src/apps/*/prompts.tsglob. Localising them would change the instruction the model
receives per user locale.
No new eslint exemptions were added: both non-copy categories moved into modules that are
already exempt for the right reason. For the same reason this branch's explicit
src/apps/spec-builder/prompts.ts entry was dropped rather than unioned — main's glob
now covers it, and re-adding it would leave dead config.
Register is per namespace, not per repo: apps.specBuilder is uniformly formal in de
(16/0) and uniformly informal in fr (15/0), and each locale follows its own siblings.
Verification
26871 pytest passed; the 9 failures all reproduce on pristine origin/main
(3× test_embedding_space_reconcile, test_embedding_model_apply,
test_app_manager::test_declares_backend_helper, 3× test_dashboard_origin, and
test_browser_recording_skill which needs Node ≥18). 8290 vitest passed; the one failure
is src/i18n/format.test.ts, inherited — this host's Node 24 has Intl.DurationFormat
and the test asserts its absence. tsc -b, eslint (0 errors), isort, flake8, mypy
(637 files) and all 11 i18n checks clean.
One self-inflicted error caught before pushing: the first pass at the catalog edits
alphabetised every sibling group, ~1400 lines of churn per file. Beyond the noise it would
have been actively wrong — [added-lines] counts a moved line as newly written, so it would
have reported every re-ordered pre-existing untranslated literal as this branch's debt.
Redone as an in-place insert: 28 lines per catalog, 0 existing values changed, 0 keys lost.
Round 71 — head
|
| candidate | _valid_name |
_usable_name |
outcome before the fix |
|---|---|---|---|
AKIAIOSFODNN7EXAMPLE |
yes | no | created, then dropped by _load_index |
ghp_ + 36 alnum |
yes | no | created, then dropped |
xoxb-… |
yes | no | created, then dropped |
my-normal-spec-name |
yes | yes | fine |
3 of 7 shapes passed create and were discarded by the very next load, orphaning the
directory, worktree and session the handler had already built. Your diagnosis and your
remedy were both right.
Widened: the same class was in two more write-side paths
_load_index admits a key only when _usable_name(k) holds — the grammar and
survival of _redact unchanged. Three paths that put a name INTO the index gated on
_valid_name, the grammar half alone. You named one; the other two are the same bug:
create(line 2426) — the site you found._discover_folder_specs(line 2274) — writesindex[name]for an adopted
on-disk directory. A credential-shaped directory name was re-added on every list
poll and dropped on every load, so it was rediscovered and re-saved indefinitely._ensure_worker_slot's name re-assert (line 1144) — its own comment claimed it
"re-assert[s] the same grammar creation and discovery enforce", which would have
become false the moment the other two moved.
All three now use _usable_name, so the index has exactly one admission predicate and
the write side cannot disagree with the read side.
Deliberately not changed: _owns_slot_key (line 760). Its name always originates
from an index key that already passed _usable_name, so the two predicates agree there,
and it carries legacy slot-key compatibility worth leaving alone. The class guard
allowlists it by name with that reasoning.
The error message now says the name "must not look like a credential" alongside the
grammar. code stays invalid_name — same category, and it avoids churning the
enumerated error-code baseline for a message change.
Tests — revert-verified, each mutation left syntactically valid so a NAMED test fails
| mutation | named test that fails |
|---|---|
create back on the grammar half |
test_create_refuses_a_name_the_loader_would_discard |
| discovery back on the grammar half | test_discovery_does_not_adopt_a_name_the_loader_would_discard |
| slot re-assert back on the grammar half | test_no_index_write_path_admits_on_the_grammar_alone |
Four tests: two behavioural (create returns 400 and leaves no spec directory; discovery
adopts the two ordinary siblings and refuses the credential-shaped one), one fixture
guard (test_the_probe_name_is_the_shape_this_class_is_about pins that the probe is
grammar-valid but not admissible, so the others cannot pass for the wrong reason), and
one class guard that walks every function in routes.py and fails if any but the
allowlisted _owns_slot_key gates on _valid_name. That last one is the one that keeps
a future caller from reintroducing the split — it asserts on the calls, not on a comment.
Verification
26871 pytest passed; the 9 failures all reproduce on pristine origin/main
(3× test_embedding_space_reconcile, test_embedding_model_apply,
test_app_manager::test_declares_backend_helper, 3× test_dashboard_origin,
test_browser_recording_skill needing Node ≥18). 250/250 Spec Builder tests pass.
isort, flake8 and mypy (637 files) clean. Backend-only change, so the frontend
gates are unaffected from 2eb8ba251, where they were green.
Not rebased: main has moved 3 commits but the PR is not CONFLICTING and the branch
protection does not require up-to-date, so rebasing would only restart the hour-long
gates.
Round 73 — head
|
| mutation | named test that fails |
|---|---|
| guard removed | test_safe_dir_refuses_a_relative_working_dir, test_absoluteness_is_checked_before_realpath |
guard moved back AFTER realpath (the original dead-code shape) |
both of the above |
Three tests: the rejection itself over ., .., relative/path, ./sub and ""
with the cwd moved into a tmp dir; a companion that pins the two legitimately-absolute
forms (plain absolute, and ~/…) still pass, so the guard cannot be "fixed" by
rejecting everything; and a source guard asserting os.path.isabs appears before
os.path.realpath, because the ordering is the entire defect and a comment cannot
enforce it.
Verification
27604 pytest passed; the 9 failures all reproduce on pristine origin/main. One run
showed a 10th failure that did not reproduce on an immediate re-run (27603 vs 27604
passed) — a flake, and outside this diff's reach either way, which touches only
_safe_dir and a Node capture script. 253/253 Spec Builder tests. tsc -b, eslint
(0 errors), node --check on the script, isort, flake8, mypy (661 files) clean.
Not this PR: De-Amazon Scrub Lint is failing main-wide
That check is red on this head, and none of it is this branch's:
src/kiro_crew/artifact_source.py:28
src/kiro_crew/artifacts.py:356
src/kiro_crew/artifacts.py:1070
test/test_artifact_source.py:299
All four occurrences are present on pristine origin/main, and none of those files
appear in this PR's diff. Fixing them here would mean absorbing an unrelated main-wide
breakage into a feature PR, so it belongs in its own change. Flagging rather than
silently carrying it, because it will hold PR Readiness red for every open PR until it
is fixed at the source.
Round 74 — head
|
| mutation | named test that fails |
|---|---|
| identity gate removed from the clear (claimless hole reopens) | test_prepare_handoff_refuses_to_clear_when_the_identity_moved |
| claim check physically moved back after the clear | test_handoff_captures_its_identity_before_the_await_and_pins_on_both |
Worth recording how the second was verified: neutering the check in place
(if False and …) did not fail the guard, because the guard tests ORDER and the text
was still present. That is the guard behaving correctly and my first mutation being
unrepresentative, so it was redone as a physical move of the block — which fails, with
the message "moved after the sentinel clear it is meant to gate".
Three new behavioural tests (refuse-on-moved-identity, still-clears-for-the-matching-identity,
unpinned-still-works) plus repairs to four existing tests the reordering touched:
- Two source guards anchored on
"asyncio.to_thread(_prepare_handoff"as a single-line
literal, which the wrapped call broke. They are now whitespace-insensitive regexes —
a guard a reformat can silently disarm is not a guard. - The order guard additionally pins the pre-clear claim check, so the new invariant is
enforced and not just the old one. test_no_handler_reads_the_index_on_the_event_loopgains_prepare_handoff, which
now reads the index synchronously. That is legitimate under the guard's own stated
rule — it only ever runs viaasyncio.to_thread— but rather than just appending a
name I tied the allowance to the contract: the test now asserts each off-loop
allowee's docstring still declaresBLOCKING, so the allowlist cannot be widened to
admit a function that actually runs on the loop.
Verification
27604 pytest passed; the 9 failures all reproduce on pristine origin/main.
256/256 Spec Builder tests. isort, flake8, mypy (661 files) clean. Backend-only,
so the frontend gates carry over from 15bb990f6, where tsc -b, eslint and all 11
i18n checks were green.
De-Amazon Scrub Lint is green again on this head — the /workplace/nrb references
I flagged last round as main-wide were fixed upstream, so that is no longer a blocker
for this PR.
Round 75 — head
|
| mutation | named tests that fail |
|---|---|
| queued append back to the raw message | test_queued_append_is_redacted_before_it_is_broadcast, test_no_broadcast_eligible_append_passes_raw_caller_text |
Three tests:
- Behavioural — a credential-shaped token in a queued message must not survive into
the appended (broadcast) content, while the queue still carries it intact. - Class guard — walks the AST for every
slot.append(...)in the module and fails
if any call whose role is NOT in the host's skip set passes content that did not go
through_redact. That is what stops the next broadcast-eligible role from
reintroducing this. - Fixture guard — asserts
_ChatSlot.appendstill contains
role not in ("chunk", "done", "user"). The rule above is derived from that skip
set, so if the host changes it the guard fails loudly instead of silently protecting
the wrong roles.
Verification
27604 pytest passed; the 9 failures all reproduce on pristine origin/main.
259/259 Spec Builder tests. isort, flake8, mypy (661 files) clean. Backend-only,
so the frontend gates carry over from 632c72df0.
Rebase onto main (15 commits) — head
|
Round 77 — head
|
| mutation | named tests that fail |
|---|---|
| settings egress back to the raw value | test_get_settings_redacts_an_agent_written_base_path, test_every_handler_that_returns_settings_redacts_it |
Three tests:
- Behavioural — a credential-shaped token written into
base_pathvia
_save_settingsmust not appear in theGET /settingsresponse. - A companion that pins the ordinary path unchanged. This one matters: redaction here
must not mangle a normal path, or the picker would display a scrubbed value and a
round-trip save would corrupt the setting. Without it, "redact everything" would pass
the first test while breaking the feature. - Class guard — walks every
_handle_*in the module and fails if one reads
_load_settingsand returns ajson_responsewithout_redact.
On the re-review
This diff passed GPT on de6a30fe4 and this finding appeared only after the rebase to
25f6cd617, whose only delta was the chat_persistence.py conflict resolution — nothing
touching _handle_get_settings. So the finding was latent and surfaced on a fresh roll
rather than being introduced. It is legitimate either way; noting it only so the history
is not read as a regression.
Verification
27718 pytest passed. Ten failures, all accounted for: the 9 that reproduce on pristine
origin/main, plus test_terminal_handler::test_ws_ctrl_c_delivers_sigint@pty_integration,
which is order-dependent under xdist and passes 167/167 when that file runs alone.
262/262 Spec Builder tests. isort, flake8, mypy (663 files) clean. Backend-only, so
the frontend gates carry over from 25f6cd617 where tsc -b and all 12 i18n checks were
green.
Round 78 — head
|
Round 79 disposition — reviewed SHA
|
| test | pre-fix |
|---|---|
..._releases_a_reservation_left_by_a_dead_process |
fails — "still hides the spec and reserves its name" |
..._releases_a_legacy_bare_timestamp_reservation |
fails — pre-upgrade bare-float marker not released |
..._keeps_a_reservation_this_process_still_owns |
passes (ordinary case, guards the blanket-clear regression) |
..._released_reservation_is_persisted_by_the_next_mutation |
fails — stale marker still on disk after a mutation |
The third is deliberately the one that passes both before and after: it exists to fail if someone later "simplifies" this into clearing every reservation. My first attempt at it asserted on the new helper and so failed pre-fix with AttributeError — the wrong reason — so I cut that assertion and left it purely behavioural.
The legacy case is a real migration path, not a hypothetical: an index written by the previous build stores a bare float, which carries no owner and therefore reads as foreign. That is the correct answer, since this process demonstrably did not write it.
Also in this push (unrelated to your finding): the Brand Name Gate landed on main while this branch was idle and flagged 3 prose lines spelling KiroCrew rather than Kiro Crew; fixed, and the gate's own self-test plus the diff check now pass locally. The rebase carried 16 commits of main, resolving chat_persistence.py (main evolved the same function this branch wraps in its rollback) plus the ten locale catalogs.
Not this PR's: Backend Tests (Windows) (3) failed on test_mochi_mcp_server.py::TestQueueMutationLock::test_concurrent_appends_are_not_lost (assert 15 == 16) — a concurrency test for main's own mochi app, in no file this diff touches.
Verification: 30,045 pytest pass; isort/flake8 clean; mypy 715 files clean; tsc -b clean; eslint 0 errors; the full 12-check i18n chain green; brand gate green; 8,936 vitest pass with two known-inherited failures (format.test.ts, which asserts the absence of Intl.DurationFormat that this host's Node 24 has, and the load-sensitive WindowCalculator benchmark, 23/23 in isolation — neither subject is in this diff).
Round 80 disposition — reviewed SHA
|
| test | pre-fix result |
|---|---|
..._arms_under_the_same_lock_as_the_identity_check |
fails — "arm is outside the _INDEX_LOCK block" |
..._halt_execution_writes_the_sentinel_off_the_loop |
fails — write not on the identity-pinned wrapper |
..._stop_write_is_refused_for_a_replaced_spec |
AttributeError (subject does not exist pre-fix) |
..._no_handler_reads_the_index_on_the_event_loop |
AttributeError (same) |
Being straight about the last two: they fail for the wrong reason pre-fix, because a test of a new function cannot run before the function exists. That is unavoidable, not a claim of behavioural coverage. The ordinary case stays pinned by the pre-existing handoff tests, which pass unchanged before and after. The first test is structural on purpose — the race is a thread interleaving, so timing it would be flaky, while the property that forbids it (the arm sits inside the locked block, and nowhere outside it) is exactly stated in the source.
I also made the off-loop guard whitespace-insensitive. It matched "asyncio.to_thread(_write_stop_sentinel" as a single-line literal, which my multi-line call broke — a guard a reformat can silently disarm is not a guard, and its sibling test_handoff_does_no_filesystem_work_on_the_loop had already learned this.
Separately in this push: 14 Windows failures in this suite, now recorded as a tracked platform boundary. Worth stating plainly because it is not a paper-over. _CAN_PIN_DIR is False on Windows (no O_NOFOLLOW, no dir_fd), so _write_stop_sentinel and _clear_stop_sentinel fail closed by design — they refuse to operate by path, exactly as the source comment explains, because the agent could swap the directory for a junction between check and write. The failing tests assert the pinned POSIX behaviour, so on Windows they assert a capability the product deliberately declines to have.
A new tests/conftest.py skips them by name on Windows, mirroring test/conftest.py's burn-down convention (whose hook is rooted at test/ and so never covered this path). It is deliberately not a whole-file collect_ignore: simulating Windows locally gives 253 passed, 15 skipped of 268, so Windows keeps almost all of this suite, including the coverage of its own no-sentinel halt path. Anything unlisted still fails the job, and I verified every listed name resolves to a real test so a typo cannot silently protect nothing.
Why it surfaced only now, since these tests are byte-identical to the previous head: CI has no committed .test_durations, so pytest-split falls back to an even split by test count. The rebase over 16 main commits changed the suite size, moving whole blocks between shards — Windows shard 4 went from 6,512 tests to 6,974. This was latent debt any rebase would eventually expose.
Verification: 30,048 pytest pass with the same 7 pre-existing failures as the previous head (5 documented inherited, plus 2 in test_weixin_qr that are an artifact of the venv I borrowed lacking qrcode — it is a declared dependency in setup.cfg, so CI has it). isort / flake8 clean; mypy clean across 716 files. Backend-only change, so the frontend gates carry over from 2bf1b57bf, where tsc, eslint, the 12-check i18n chain and the brand gate were all green.
Ports the external kiro-specs app into a native builtin: backend routes with
_require_enabled gating, a TSX page suite, and the spec-workflow skill.
Adheres to the frontend style guide rather than re-implementing dashboard UI.
Every fix below deleted code instead of adding it:
- shared SegmentedControl for the doc tabs (the per-file status dot rides its
icon slot, and responsive collapse comes free) — replaces two hand-rolled
button rows
- shared Modal for settings — supplies role=dialog, aria-modal, Escape and
backdrop dismissal, scroll lock and a labelled close button
- shared Btn / Input / SearchInput / EmptyState / Clickable throughout; all 11
raw <button> elements are gone, and the local pill button is now a thin
wrapper over the host Btn instead of 18 hand-written style props
- lucide-inline icons: dropped all 26 forbidden size={N} props, the inline-flex
IconText wrapper, and the text glyphs used as icons
- Framer Motion pulse instead of a CSS @Keyframes block
- design system variables only; typography normalised onto the 14/13/12/11
scale, including one 9.5px value below the hard floor
- aria-labels on every icon-only control, aria-live on the streaming/working
regions, role=alert on the error banner, keyboard-operable spec rows, and an
APG window-splitter (role=separator + arrow-key resize) for the divider
Gates: tsc clean, eslint 0 errors 0 warnings on the app (was 6 warnings),
vitest 4554 passed, backend 20 passed, flake8 + isort clean. Four new
accessibility regression tests, the strictest one revert-verified.
Security: adds a single _safe_dir() chokepoint for every caller-supplied
directory (working_dir, settings base_path, browse path). Previously only the
browse endpoint applied the sensitive-path test, so a direct create call could
name a credential directory as its working_dir and get a spec tree — and an
agent cwd — inside it. The chokepoint expands ~, resolves symlinks BEFORE
checking (so a planted symlink can't smuggle its target), requires absolute,
and denies is_sensitive_path; with must_exist=False it also validates the
nearest existing ancestor so a not-yet-created dir under a credential
directory can't slip through on a stat miss. Resolved spec dirs additionally
get an explicit _contained() assertion instead of relying on the name regex
defined three functions away. 7 new security tests, revert-verified.
CX alignment with the Issue Radar builtin (the reference triage app), so an
embedded builtin does not invent its own conventions:
- loading is a SKELETON THAT HOLDS THE LAYOUT, not a spinner: shimmer rows in
the rail and a document-shaped skeleton in the doc pane, reusing the shared
animate-shimmer utility. The sr-only role="status" sits OUTSIDE the
aria-hidden subtree, or it would never be announced.
- the empty state is centred with an icon instead of a sentence pinned to the
top-left, and no longer flashes before the first fetch resolves.
- the rail STAYS MOUNTED in every state and ends in app identity + version with
Settings reachable from it. It previously unmounted on the empty state, so a
first-run user could not reach settings at all — caught by the screenshot
harness asserting the footer before it shot.
- detail mounts only for a spec present in the list: a stale localStorage
selection used to surface a raw "not found" banner before the list reconciled.
- the splitter matches Issue Radar's (slim, hover-accent) and locks text
selection while dragging.
Screenshots captured against a REAL isolated instance of this branch (own port,
own KIROCREW_HOME, no crons — never the live plane) via
website/scripts/capture-spec-builder.mjs, which asserts each state rendered
before shooting.
The git helper is routed through sandboxed_spawn_argv with a scrubbed env and
the resource-limit preexec (mirroring git_coord._git) rather than added to the
spawn-audit benign allowlist: its working directory is caller-supplied and the
branch name derives from a spec name, so it is agent-influenced.
Review round 2 — all five reported HIGHs:
1. Spec-dir files are untrusted even though the dir passed validation: reads and
the STOP write go through _spec_file/_read_spec_text/_write_stop_sentinel,
which refuse symlinks, require realpath containment, refuse sensitive
targets, and write atomically with O_NOFOLLOW + os.replace (which destroys a
planted link instead of writing through it).
2. Autonomous execution routes through authorize_and_add_nudge instead of
svc.add, so it inherits the shared ownership checks, message limit,
sensitive-sentinel refusal and SEL audit; fails closed by revoking trust and
returning 403. max_cycles 0 (infinite) -> 60. The auto-approve grant now
expires via _TRUST_TTL_SECS and is revoked on Stop through one _halt_execution
chokepoint (Stop previously left trust set). A missing start timestamp fails
closed.
3. .spec-state.json is projected onto the documented schema: unknown keys
dropped, types enforced, lists capped, and KEYS redacted as well as values
(a credential in an object key previously bypassed the value-only scrub).
Malformed entries such as decisions:[null] are dropped, not forwarded.
4. Create returns 409 when the target already holds Kiro markdown, so an
IDE/CLI-authored spec is never handed to an agent by accident; adopting is
explicit via import_existing.
5. Containment regression fixed: a worktree is a SIBLING of the checkout, so
after creating one it is re-validated and becomes the containment root.
Previously every worktree-mode create failed containment and orphaned the
worktree; failures now roll it back via _remove_worktree.
14 new tests (34 total in the builtin), each guard revert-verified.
Embedded chats can now action a pending tool approval. ChatMessageList only
renders Approve/Reject when an onApprove handler is supplied and ChatEmbed
supplied none, so a permission prompt raised mid-turn showed a dead "Approval
needed" label and the worker blocked until the runner's timeout auto-rejected
it. ChatEmbed now resolves decisions via POST /api/approvals/{id}/{action}
through the scoped app API, and the builtin grants '/api/approvals' alongside
'/api/chat' (the external app's manifest already did).
Embedded tool calls now read like a main session. ChatEmbed's inline
ToolCallPill rendered EVERY state as one accent-purple spinning Wrench
(animate-spin, 2s) with the raw command hard-truncated to 80 chars, so a
finished call was indistinguishable from an in-flight one and the transcript
looked permanently busy. It now uses state-aware icons (LoaderCircle running /
CircleDot done / CircleSlash rejected / Lock awaiting approval) with matching
status colours, prefers the backend-stamped meta.purpose over the raw command,
and offers the same file affordance as the main chat via the pure
extractToolFilePath/isSafePath helpers (no Redux — ToolCallLine itself depends
on store state the embed's prop-driven session never populates, which is why the
pill was brought up to parity rather than swapped out). The spinner is gated on
the session's running flag, so a tool call orphaned by a dropped turn stops
spinning instead of looking busy forever. 5 parity tests, revert-verified.
Review round 3:
- The auto-approval TTL now ages EVERY grant, not just an executing run. Trust
is stamped on three paths (create / message / execute) but the TTL opened with
a status=="executing" gate, so both planning grants were unexpiring: create a
spec, stay in planning, and the worker auto-approved tools forever. Grants now
record trust_granted + trust_at; expiry halts an executing run as before and
revokes a planning grant in place. A source guard fails if a future grant site
forgets to record its timestamp.
- The browse scan moved off the event loop. sorted(os.scandir()) plus a realpath
and sensitive-path test PER ENTRY ran inline in the aiohttp handler, stalling
chat streaming and heartbeats on large directories; it now runs via
asyncio.to_thread and is bounded at 500 entries.
- is_sensitive_path is imported at module scope (four function-local copies
removed) with a fail-closed fallback: callers use it to decide whether a path
may be read, written or browsed, so an unavailable security module must deny.
Review round 4:
- Spec reads are descriptor-pinned. _spec_file validated the path and then
read_text() opened it BY NAME; the agent writes into that same directory and
the UI polls every 2.5s, so the check-to-open window was real and repeatable.
Reads now go through safe_read_file_bytes_nolink(within_root=spec_dir), which
opens O_NOFOLLOW first and validates the descriptor it read, and are capped at
1 MiB.
- The app no longer grants worker trust at all. The TTL was enforced from the
status-poll handler, so closing the browser tab stopped enforcement while the
grant survived — not a bound. All three slot._trust grants and the whole TTL
machinery are removed. The grant existed only because approval prompts were
invisible in the embedded chat, and this PR fixed that, so the decision now
belongs to the user through core's own trust mechanism where it is auditable
as their choice. Stop and the authz-failure path no longer clear trust either:
if the user granted it, this app must not silently undo it. Source guards keep
both the grant and the TTL from coming back.
- The specs list uses useQuery instead of useState + setInterval; two
overlapping manual polls could resolve out of order and overwrite fresh server
state with stale data.
Review round 5 (rebased onto current main):
- The two POLLED endpoints no longer do filesystem work on the event loop. The
detail endpoint (polled every 2.5s during a build) stat-ed three phase files,
read up to three 1 MiB documents and read .spec-state.json inline; the list
endpoint walked every known project root's .kiro/specs. Both froze the gateway
loop, chat streaming and heartbeats included. Detail now makes ONE thread hop
through _collect_spec_documents (bundled so a future edit can't reintroduce an
inline read); discovery goes through asyncio.to_thread. Round 3 fixed this
class only at the browse endpoint and missed both siblings.
- Deleting a spec now tears down its worker slot. Previously it removed the nudge
loop and the index entry but left the in-flight turn ALIVE, so the agent kept
editing the user's files after the spec was deleted, and re-creating the same
name resurrected the old transcript (get_or_create_slot keys off the name).
_teardown_worker_slot mirrors the gateway's own order: pop from the registry
before any await, cancel, await under a bounded shield, then save as closed.
It refuses slots this app does not own.
4 participants
What
Ports the external
kiro-specsapp into a native builtin: Spec Builder, a spec-driven development surface. Describe a feature in chat, an embedded agent drives Requirements → Design → Tasks → Execution with phase gates you approve, and the artifacts land as plain markdown in<project>/.kiro/specs/<name>/so they stay compatible with the Kiro IDE and CLI.Three columns: a collapsible specs rail, the native chat (
ChatEmbed), and a docs card with selection-to-comment review, batched feedback, and phase-gated approval actions. Structured agent state (.spec-state.json) surfaces as DECISIONS / BLOCKING / CONTEXT cards.Style-guide adherence
The external app hand-rolled a lot of UI. A review against
website/AUTOSDE.yamlandwebsite/AGENTS.mdfound 12 findings, and this port closes all of them. Notably, every fix removed code rather than adding it — each one replaced a local reinvention with the shared component that already existed:SegmentedControl— the per-file status dot rides itsiconslot, and responsive full→compact→dropdown collapse comes free (it sits in a user-resizable column)Modal— suppliesrole="dialog",aria-modal, Escape + backdrop dismissal, scroll lock, labelled close<button>elementsBtn; the local pill button is now a thin wrapper over it instead of 18 hand-written style propssize={N}icon props,inline-flexwrappers, text glyphs as iconsclassName="lucide-inline"throughout;IconTextdeleted;✕ › ←→X/ChevronRight/ArrowLeft<div onClick>/<span onClick>Clickable(role, tabIndex, Enter/Space) for spec rows, folder rows, decision options, type cardsaria-labelon icon-only controlsaria-liveon streaming regionsrole="alert"on the error banner@keyframesPULSE_MOTION)text-xsrole="separator"+aria-valuenow/min/max)The two eslint warnings the splitter trips are a linter blind spot —
no-noninteractive-tabindexandno-noninteractive-element-interactionsdon't model the APG splitter pattern, which is interactive once focusable. Suppressed at the element with the reasoning written inline rather than reshaped into a<button>, which would announce the wrong role and lose the value semantics.Verification
tsc -bclean (the roottsconfig.jsonis references-only, sotsc --noEmit -p tsconfig.jsonchecks nothing —tsc -bis the real gate)eslint src/apps/spec-builder— 0 errors, 0 warnings (was 6 warnings)vitest— 5321 passedpytest— 141 app tests + the full 19212-test suite;mypy(509 files),flake8,isortcleanaria-labeland confirming it fails.Note on a pre-existing failure
src/test/KiroGhostMark.test.tsxfails 2mask-imageassertions. This is not from this branch — I reproduced it with these changes stashed. Main's happy-dom test-environment migration broke a test written for jsdom, so it currently fails on any PR. Flagging rather than fixing here to keep this diff scoped.Changes outside the app directory
Two core files carry fixes this app surfaced, kept here because the app is the
thing that exposes them:
src/kiro_crew/autonudge.py—remove()fsynced on the event loop. All fourwrite sites now go through
_write_payload_locked, which offloads and, oncancellation, drains to completion so a later write cannot land before an
earlier one.
src/kiro_crew/dashboard/chat_persistence.py— addedarehydrate_slot_from_history, an async twin of the existing per-slot resumewhose transcript read runs in a worker thread. Slots live in memory, so a
gateway restart left an app's chat column empty with the whole conversation
still on disk; app backends resolving a cold worker slot need to restore it on
the request path without stalling the loop on a multi-megabyte session file.
Follow-ups deliberately not in scope
kiro-specs-appstill carries its own copy of these issues (notably a light-mode-only phase pill). It is superseded by this builtin; retiring it is a separate decision.ChatEmbedgainedframelessandstartAtBottomprops here, which let the app drop CSS!importantoverrides.onApproveis now wired through the embed too (an embedded agent that hit a permission prompt previously rendered a dead "Approval needed" label with no buttons).Screenshots
Captured against a real isolated instance of this branch — its own port, its
own
KIROCREW_HOME,--no-crons, live gateway untouched — viawebsite/scripts/capture-spec-builder.mjs(committed). The harness asserts eachstate rendered before it shoots, so a blank page or an auth failure fails the run
rather than producing a plausible-looking empty image.
Collapsed rail
Dragging the rail past its minimum collapses it to a bordered strip that keeps
Settings and the app identity reachable.
Spec selected — chat | docs split
The main surface: rail with two specs, the embedded agent session, and a rendered
requirements.mdwith phase tabs (sharedSegmentedControl, status dot in itsicon slot), the phase-gated
Approve → Tasksaction, and the CONTEXT panel.Light palette — the app is design-token driven, so both palettes are meaningful
variants rather than a cosmetic duplicate:
First run
Centred, icon-paired empty state. The rail stays mounted — it previously
unmounted here, taking the app-identity footer and the Settings entry point with
it (the screenshot assertion is what caught that).
More surfaces — rail populated, new-spec view, project picker, light first run
Rail with both specs, nothing selected:
The conversational creator — spec-type cards are
Clickabletoggle buttons witharia-pressed, not bare<div onClick>:Project picker — folder browser with recents, keyboard-operable rows:
First run, light palette:
Honest scope note on the fixtures: the two specs in these shots are fixtures
written to disk in the isolated instance's spec dirs, not the output of a live
agent run. They exercise the real read path, rendering and layout, but they are
not evidence that the Requirements → Design → Tasks agent loop works end to end.
The skeleton states are covered by unit tests rather than screenshots, since they
resolve too quickly to capture reliably.