feat(apps): add Design Tweak builtin (visual select-to-edit) - #1122
Conversation
github-actions
Bot
added
readiness: checking
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/apps/design-tweak/DesignTweakPage.tsx:470 -- 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 detailsArbiter-Verdict: PASS No sub-threshold finding meets the long-term-impact bar. Scope note: GPT 5.6's two findings (unbounded
Suggested follow-ups (open as issues — non-blocking)
[ARBITER-REVIEWED] 44deb6f False positive or not applicable? A repository writer can comment: For a broader accepted-risk deferral, apply |
UX Review (Fable 5) — 🟡 CONCERNSAdvisory UX-level review of No committed screenshots in the diff (the PR image is a GitHub-hosted asset, not a file under the repo), so lens 12 doesn't apply. My findings come from the code and strings. One notable issue: all failure feedback funnels into a single de-emphasized, truncated, unannounced status line. UX-Verdict: CONCERNS Every error — send, delete, capture, follow-up, add — lands in one 11px muted truncated status line with no aria-live, so a failed edit-send can go unnoticed and its "what to do next" gets cut off. Watch
Suggestions
[UX-REVIEWED] 5ef6f79 |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS A real graduation problem, solved in the sibling-builtin shape, with the riskiest piece (startup auto-migration) deliberately cut and every residual risk given a named, tested disposition. [DESIGN-REVIEWED] 5ef6f79 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
6fb2d01 to
5a97098
Compare
github-actions
Bot
added
readiness: checking
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBased on my review: the candidate list contained no candidates to falsify (Step 1 is trivially satisfied), and my independent Step 2 examination of the two highest-risk surfaces — the stdlib HTTP backend ( No findings. [OPUS-REVIEWED] 5ef6f79 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
5a97098 to
0c02de9
Compare
github-actions
Bot
added
readiness: checking
0c02de9 to
114f6ee
Compare
github-actions
Bot
added
readiness: checking
114f6ee to
a76ff1c
Compare
|
🤖 Kiro Crew [operator: iamwhatever]: This PR has been inactive for 7+ days. I reviewed the blockers but they require your input:
When you've addressed these (rebased onto main / resolved conflicts), the pipeline will re-assess on its next cycle. |
|
Picked this back up and force-pushed a rebase — here's what was going on and what changed. Root cause of the stall: the branch wasn't just behind main, it had regressed. Comparing the force-push history, What this push does:
Verified before push: full backend pytest + frontend vitest (18236 tests) green, isort/flake8/mypy/tsc/eslint clean, |
First Principles Review (Fable 5) — 🟡 CONCERNSAdvisory premise-level review of First-Principles-Verdict: CONCERNS A well-cased graduation with the migration correctly deleted — but it re-implements pid→cwd beside What this change shipsIntent: let users install the visual select-to-edit tool from the App Store instead of side-loading an external app — an ADDITION (graduation), with a named cause (
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 5ef6f79 |
|
Two more rounds while getting CI green: Fixed — GPT 5.6 finding. Not fixed here — pre-existing, unrelated main breakage. |
|
Another GPT round, 3 findings this time — all fixed:
Local gates still all green (pytest 244/244 on the touched file, tsc/eslint/i18n-check clean). |
|
One more round, 2 findings — both fixed:
|
|
Round against Fixed. The dropdown trigger button (my own addition from an earlier round) reused the Rebutted — base64 credential scan. Rebutted — delivery.ts truncated-history duplicate. The suggested fix ("return /ai-review override gpt 2e46e7c: base64-scan trade-off is pre-existing, documented, and deliberate — see |
|
Fixed the Brand Name Gate too — a comment I wrote used the joined |
|
Scope reduced: the automatic-takeover migration is dropped. The reason it is unnecessary is that two mechanisms already on
So the upgrade path is: uninstall the external app once from Apps → Library → Uninstall, and the builtin registers into that same directory on the next gateway start, inheriting the preserved queue and history. Now documented in the app's README. Automating those two clicks is what required moving user-owned directories during gateway startup — the largest failure surface in the change and the source of most blocking findings this PR accumulated (crash-interrupted renames, symlinked This also resolves the two blocking findings from the last GPT round: the Gates re-run after the removal: full frontend vitest 19,906 passed, design_tweak backend suite green, isort / flake8 / mypy / tsc -b / eslint-at-baseline / brand-name gate / |
|
Local pre-push review dispositions — reviewed SHA Two model-pinned local reviewers ran the CI contracts against this diff before the push (a 1. The claim was that The residual — the child process can read files as the host user — is not a boundary this diff crosses. The child is the project's own 2. Both reviewers agree the mechanism is real, and it is worth stating plainly: cookies are host-scoped, not port-scoped, so nothing in the sandbox tokens stops a framed project script from navigating itself (or fetching with Not treated as blocking, because it grants no capability the actor lacks: the only party positioned to receive that cookie is the previewed project's own dev server, which per finding 1 already runs unsandboxed as the host user and can read the dashboard token off disk directly. It is also conditional on cookie-auth being active at all. What was wrong and is now fixed: the safety comment in 3. Confirmed by both reviewers and by the file's own contract. Fixed by moving the Gates on the pushed commit: backend |
|
Answering the First Principles Review (Fable 5) 🟡 CONCERNS — it advises rather than blocks, and no earlier round replied to it. It should have; it found real over-granted privilege. Now on Subtraction 1 — Verified each claim before acting. Worth noting for the record that Subtraction 2 — the One correction to the finding: Subtraction 3 — bare This one is more than a consistency nit, which is why it got fixed rather than deferred: I checked the opposite failure mode before calling it done, since a pin can silently disable a working feature: Watch item — sibling counts → ACCEPTED-AND-DEFERRED. Agreed and not attempted here. This is the 3rd macOS folder-picker copy ( Gates on |
|
Dispositions against 1. Confirmed. Fixed with the repo's existing primitive for exactly this direction, Tests: 2. Confirmed, and it was worse than the body reads alone: none of the three handlers ( Three changes:
I checked the WebSocket relay for the opposite failure mode, since a connection timeout could tear down a legitimately idle HMR socket: Tests: Gates on Process note for the maintainer, not a disposition. By the |
|
Answering the UX Review (Fable 5) 🟡 CONCERNS on Headline — every outcome lands in one muted, truncated 11px status line → ACCEPTED, valid, and the one I would fix first. The observation is accurate: Not fixed in this PR, and I want to be honest about the reason rather than dress it up: it is the largest of the UX items — an error-styled, wrapping variant plus relocating feedback next to each acting control touches every call site that sets Dimensions dropdown has no Escape / outside-click dismissal → ACCEPTED-AND-DEFERRED (cheapest of the three). Confirmed asymmetry: the project dropdown implements both and "Connected" button reads as a status badge but disconnects on click → ACCEPTED-AND-DEFERRED. Valid — the destructive outcome lives only in a hover tooltip, which is invisible to touch and to keyboard users. Deferred because the fix needs a new label string across 14 locale catalogs plus Suggestions:
None of the six is a one-way door: no wire format, schema, or persisted contract is involved, so each is fixable in an ordinary follow-up without a migration. That is why I am comfortable deferring rather than blocking — but they are deferred decided, not dismissed, and I would rather the maintainer tell me to spend one more round on the dropdown dismissal and the status-line styling than have them ship as-is by default. |
|
Dispositions against 1. The report was right about the consequence and slightly off about the mechanism, so worth stating precisely. Validation already existed one line below the quoted snippet ( A Tests: 2. Confirmed, including the claim that an endpoint already exists.
3. Coverage Gate — FIXED, I owe a correction here: I previously reported this PR as "only GPT blocking". That was wrong. Five new sibling test files, one per subsystem, all asserting behavior rather than merely executing lines:
Verified with the repo's own gate rather than my own arithmetic: On the four extra backend failures a reviewer may notice: the full suite reports 43 failures with these five files present and 39 without, but the delta is xdist re-sharding, not regression. All five extra tests pass in isolation, one different test ( Gates on |
|
Dispositions against 1. Legitimate, and the repo already had the right primitive. The read now goes through it with Tests: 2. Confirmed, and worse than disk alone: every append rewrites the WHOLE record, so an unbounded thread is quadratic rewrite work on top of unbounded storage. Added One deliberate carve-out worth naming: a text-free status update ( Tests: 3. Confirmed, and the reported fix was the only correct shape.
Tests: 2 in Gates on One unrelated red a reviewer may see: |
|
Dispositions against 1. Legitimate, and the reachability argument is stronger than "an injected agent": the bundled What makes it worth blocking rather than shrugging at is the blast radius:
Tests: 2. Confirmed.
One implementation detail that is load-bearing and not obvious from the diff: the teardown runs outside Tests: Gates on |
|
Disposition against
Correct, and it is the mirror image of the read bound added last round. Each inbound payload is under Fixed as an invariant rather than a second number: Two implementation choices that go beyond the reported fix:
Surfaced at all six transaction boundaries as Tests: Gates on |
|
Disposition against
This is the same finding, on the same span, with the same proposed remedy ("remove automatic project-script spawning") that was already raised and adjudicated earlier in this PR's review history. A second model reviewing the same SHA ruled it a false positive as framed. Re-stating the evidence rather than re-litigating it:
The one genuinely new angle, stated honestly: this round adds "rewrites governance files → policy tampering," which the earlier rounds did not. At the OS level that is true — a child running as the user can write What is a fair question, and the reason this goes to the maintainer: whether an app-initiated spawn should be held to a stricter standard than a user-typed one, even when the operator picked the target. If the answer is yes, the fix is not in this app — it is sandboxing the dev-server spawn (a Two ways forward, maintainer's call:
I am deliberately not posting the override myself: it asserts a maintainer judgment about the app's threat model, and nobody has asked me to make that call. Round count, for context on why this is escalating rather than iterating. This is the 11th review round on this PR. The last three rounds each produced legitimate findings that were fixed (symlink-swap read, unbounded thread growth, resend button state, unbounded queue read, project-removal resource leak, and a write/read ceiling mismatch the round before it introduced). This round produced a repeat of an already-rebutted finding whose remedy is a feature deletion. That is the signal to stop patching: the remaining disagreement is about the app's threat model, not about a defect. Everything else on this head is green: design-tweak backend suite 528 passed, per-file coverage 85% against an 80% floor, and isort / flake8 / mypy / tsc / eslint / brand-name / i18n:check all clean. |
|
Rebased onto latest Fixed — The test files added for the coverage work made platform assumptions that only showed up on the Windows shard:
The sixth Windows failure, Newly diagnosed — I owe a correction here. I previously reported this gate as fixed after taking Backend coverage is 90.38% and frontend 90.32% project-wide, so the project floors pass; this is purely the per-file floor on the three new frontend files, none of which is baselined (correctly — they are new, so they have to meet 80% outright).
I am flagging it rather than starting it, because it is a real chunk of work and the maintainer may prefer a different route — baselining the page and covering the two small files, splitting the page into testable units first, or landing the coverage as its own follow-up. Everything else on |
|
Frontend per-file coverage is fixed. Pushed All three files that the Coverage Gate flagged now clear the 80% floor, verified with the repo's own gate against a full-suite cobertura report rather than my own arithmetic:
Why the page was at 1.8% despite having tests. The three pre-existing
210 tests across the nine design-tweak files, all passing. Two problems found while integrating, both worth recording because neither showed up in isolation: Fake timers versus Radix opens on One test removed rather than left broken. A collapse/expand test would not drive the row toggle under happy-dom. After four attempts against the same mechanism I stopped and deleted it, leaving a comment saying why: the visibility mechanism it asserted is already covered by Three files the gate still flags, none of them in this diff (confirmed against Gates on |
|
Disposition against 1. Reproduced the exact pair from the finding: Replaced with a 128-bit digest: FNV-1a over four lanes with distinct offset bases, base36 per lane. Both bytes of every UTF-16 code unit are mixed, so The regression test asserts the documented pair separates, and then pins the whole structural class: all 1,296 paths differing only in their last two characters yield 1,296 distinct keys. 2. Traced it end to end before changing anything. The finding's remedy is right, but the order is the whole fix, and the obvious reading of it is inert: Resume is now the first call, before any creation, and doubles as the read since it returns the same 3. Not disputing it. Verified the mechanism: It is being held for a follow-up rather than folded in here, to keep this diff to the two client-side defects. Flagging plainly that this is a deliberate deferral of a real defect, not a rebuttal, so the next review round sees it as known. Local gates on One thing worth a separate issue, outside this diff: |
|
Follow-up on the third finding, now fixed in
Reproduced the mechanism rather than taking it on the label. Fixed as a read/write pair, because a boundary check alone is not sufficient here:
That pairing is deliberate and it is the same shape as the Both layers are mutation-verified independently: removing the boundary guard fails Gates on All three findings from this round are now closed. The |
|
Disposition against
The mechanism is real and I am not disputing it. Why the suggested fix is not the one I applied. "If What I did instead, in 3 lines of behaviour: return if (transcriptHasRequest(t, req.id)) return 'delivered'
return t.hasMore ? 'unknown' : 'missing'
Mutation-verified in both halves, because either one alone fails silently:
Two tests guard the obvious over-correction: Gates on A process note, offered rather than asked for. This is round 14 on this PR, and this finding was created by the fix in round 13 — which was itself created by the fix in round 10. Each has been real and each has been narrower than the last. That is a healthy trend, but the remaining items are now bounded, reachable only through multi-step operator sequences, and confined to one client-side verification helper whose worst case is a duplicated edit the user can see and undo. I would rather land this and open a follow-up for anything further than keep the diff open across another rebase of a fast-moving |
|
Disposition against
Verified against the file's own invariant rather than reasoned about in the abstract. Every other The finding's fix is right, including the part that says to keep One thing added beyond the prescription. Both halves of the ordering are mutation-verified, because each fails silently on its own:
The test observes lock state by substituting a depth-tracking context manager for Gates on Round 15, and I want to be straight about the pattern rather than just keep patching. Five consecutive rounds have each produced one real, narrow, correctly-reported defect in a different corner of this app — read/write ceiling pairing, resume ordering, element field types, transcript truncation, and now registry serialization. None has been a false positive and none has recurred, which says the review is working; but they are being found one per round in a 3,700-line file, and the diff has now been rebased across a fast-moving |
The "Design Tweak" visual select-to-edit tool only existed as an externally-installed app from a separate repo. Graduates it to a first-party builtin so it's discoverable in the App Store, versioned with the gateway, and held to the repo's own quality gates: a compiled React UI page (website/src/apps/design-tweak/) plus a gateway-managed subprocess backend (src/kiro_crew/apps/builtins/design_tweak/backend/server.py) behind an HMAC-verified proxy. Security properties of the backend: every filesystem path resolves through one canonical _contained() barrier, the dev proxy re-validates its loopback allowlist at the sink, upstream response headers are sanitised before echo, and spawned dev scripts inherit neither the proxy HMAC secret, PORT, nor SSH credentials (SSH_AUTH_SOCK / GIT_SSH_COMMAND / GIT_SSH) -- an untrusted project dev script must not be able to authenticate as the operator. Each previewed project gets its OWN loopback listener, so two projects never share a browser-storage origin: same-origin is scheme+host+port, and a shared listener differentiated only by URL path would have let project B's page read project A's localStorage. GET /latest applies the same _redact_thread() floor /queue and /history apply, so agent-written credential text in the queue JSON cannot be served verbatim. NO automatic takeover of the external install. An earlier revision of this branch carried ~1167 lines that renamed a user-installed app directory during gateway startup and moved its data/ forward. That is dropped deliberately: register_builtin_apps() already stands down on a directory it did not write (_builtin_owns_install(), pre-existing), and uninstall_app() already keeps data/ by default -- so a user coming from the external app uninstalls it once and the builtin registers into that same directory on the next start, inheriting the preserved queue and history. Automating that bought two clicks for the few users who have the external app, at the cost of moving user-owned directories at startup for everyone -- a failure surface (crash-interrupted renames, symlinked data/, enabled-state carry-over) far larger than the convenience. The path is documented in the app's README instead. Tests: full frontend vitest (19906) and the design_tweak backend suite pass; isort, flake8, mypy, tsc -b, eslint at the repo baseline, brand-name gate, and I18N_BASE_REF=origin/main i18n-check all clean.
|
Disposition against
Not a threat-model question, and I am not going to argue it: this is the app's own documented invariant applied to one field of a dict and missed on its sibling. In the same returned object, line 518 was
Every word of that applies to the comment text. Fixed by extracting the order into one helper rather than adding a second inline copy of the two calls. I also checked that this closes the field set rather than one instance of it: everything else Mutation-verified: removing the comment redaction fails three of the new tests; flipping the order to credentials-first fails the ordering test, which now guards the shared helper for the thread path too. Two of my own test assertions were wrong before they were right, and the corrections are worth recording because they are easy to get backwards:
Gates on Escalation, per the rule I set for myself last round. Rounds 11 through 16 have now each produced exactly one new, real, correctly-reported defect, each in a different corner of this app: record-size read/write pairing, resume ordering, element field types, transcript truncation, registry serialization, and now output redaction. Zero false positives, zero recurrences. The review is genuinely working — and it is also not converging, because the arrival rate has not fallen: one per round, six rounds running, in a 3,700-line file, with a Two of those six rounds also shipped a suggested fix that would have been inert if applied literally (round 13's create-then-resume, round 14's "unpaginated detail"), which is the part that makes further rounds expensive rather than merely slow: each one needs full source verification before it can be trusted, and one round's fix has twice become the next round's finding. The remaining surface is uniformly: concurrent or multi-step operator sequences, in a default-disabled builtin app, worst case a visible and recoverable data loss or a redaction gap on the user's own screen. I am recommending this lands now and that anything further is taken as follow-up issues against merged code, where a fix costs one small PR instead of re-arming eleven review bots and re-racing Also still open and deliberately out of scope, worth its own issue: |
5 participants
Problem
The "Design Tweak" visual select-to-edit tool only existed as an
externally-installed KiroCrew app (
poke-and-prose, from a separate GitHubrepo). It should ship as a first-party builtin so it's discoverable in the App
Store, versioned with the gateway, and held to the repo's own quality gates.
Why it matters
External apps run out-of-tree with a federated ESM UI bundle, which forces
origin=localand blocks the gateway from spawning their backend. Graduatingthe tool to a builtin makes it a supported, in-repo feature (compiled UI page +
gateway-managed subprocess backend) instead of a side-loaded app.
Fix (symptom → root cause → change)
Symptom: the app couldn't be a builtin — a shipped
ui.entryESM bundle flipsorigintolocal, andapps/backend.pyrefuses to spawn a subprocess backendunless
origin == "builtin". Root cause: the UI must be a compiled React page(no
ui.entry), not a federated.mjs. Change — graduatepoke-and-prose→builtin
design-tweak:src/kiro_crew/apps/builtins/design_tweak/backend/server.py):stdlib HTTP server, binds loopback, reads
PORT, exposes/health, andverifies the gateway's
X-KiroCrew-ProxyHMAC on every non-health route(CWE-306). Manifest
backend.entryPointis the dotted modulekiro_crew.apps.builtins.design_tweak.backend.server. The external repo's/self-updateroute and legacy env fallbacks are removed (a builtin isversioned with the gateway, not self-updated from an external repo).
.mjsis converted to a compiled React pagewebsite/src/apps/design-tweak/DesignTweakPage.tsx(+api.ts,types.ts),registered in
website/src/apps/builtinRegistry.tsunder/design-tweak. Thethree host-module hooks are rewired to SPA-native equivalents
(
react-routeruseNavigate, reduxsetPendingInput, a plain-fetchapi).The header's update-check and GitHub-repo buttons are removed, and the panels
are aligned to the sidebar.
skills/visual-editand the App Store icon at/app-assets/design-tweak/icon.svg.test/test_spawn_audit.pyallowlists three benign,fixed-argv / operator-configured subprocess spawns (
_lsof_fields,_h_pick_folder,_start_dev_proc) with justification, matching the existingcode_reviewer/git.pyprecedent (routing a long-lived dev server is noted asa follow-up hardening candidate).
setup.cfgadds the builtin'sinject/dir topackage_data.The external app repo (
kc-app-design-tweak) is not modified.Tests
pytest(14796 passed) — includes the twotest_spawn_auditgates, which pass with the merged allowlist.
vitest(3832 passed, 3 pre-existing skips).isort/flake8/mypy(411 files) clean;tsc -bandeslintclean.Manual verification
Brought up an isolated pod, enabled the app from the App Store, and confirmed:
the
/design-tweakpage renders in the sidebar, and the spawned backend answers/healthand/queuewith 200 through the HMAC-signed proxy.Screenshots
No automatic takeover of the external install (was: a migration)
An earlier revision of this branch carried ~1167 lines in
apps/manager.py(+531) andtest_app_manager.py(+645) that renamed auser-installed app directory during gateway startup and moved its
data/forward, so users of the external app would be auto-upgraded to the builtin.
That is now dropped, and
apps/manager.pyis untouched by this PR.Two mechanisms that already exist on
mainmake it unnecessary:register_builtin_apps()already stands down on an app directory it didnot write (
_builtin_owns_install()), so a user-installeddesign-tweakisnever clobbered.
uninstall_app()already keepsdata/by default — it movesdata/aside, removes the app dir, then moves
data/back.So the upgrade path for someone coming from the external app is: uninstall it
once from Apps → Library → Uninstall, and the builtin registers into that
same directory on the next gateway start, inheriting the preserved queue and
history. Documented in the app's README.
Why dropping it is the right call. Automating those two clicks required
moving user-owned directories at gateway startup — the single largest failure
surface in the change (crash-interrupted renames, symlinked
data/,enabled-state carry-over ordering), and the source of most of the blocking
review findings this PR accumulated. The audience is only those who installed
the external app from its own repo, so the convenience bought is small and the
risk imposed on every other user is not. If auto-migration is wanted later it
belongs in its own small PR, where its risk gets review attention proportionate
to it rather than riding inside a 12.8k-line app change.
Security hardening in this round
Each item has a regression test:
_contained()barrier(realpath +
startswith), replacing barriers CodeQL could not model. Thistook the PR from 21 open CodeQL alerts to 1.
sink rather than trusting module state set elsewhere (closes a
py/full-ssrfcritical).(
py/http-response-splitting).this backend's only authentication, so an untrusted project dev script could
otherwise forge signed calls straight to the loopback backend and bypass the
gateway entirely.
PORTis stripped too, so the child does not try to bindthe backend's own port.
CookieandAuthorizationon both the HTTP relay andthe WebSocket handshake replay. Cookies are host-scoped but port-agnostic,
so with the dashboard on
127.0.0.1the browser was attaching itsSameSite=Laxsession cookie to proxied requests and it was being relayedverbatim into the user's own
npm run devprocess.message-type allowlist, and learns the panel's origin instead of posting to
"*".Internationalisation
The page is authored in JSX (not
createElement) so it matches every siblingapp and satisfies the i18n extraction gate. All user-facing strings are behind
i18nT('apps.designTweak.…')with real translations in all nine target localesplus the regenerated
en-XApseudolocale; the send-button count uses i18nextplurals registered in
pluralKeys.json.Known dispositions
py/path-injectionin_valid_root— dismissed as false positive.The sink is a read-only
p.is_dir()check, and naming an arbitrary folder topreview is the feature, so there is no enclosing base to contain the path
in.
realpath+expanduserremoves all traversal first, then the resolvedpath is rejected if any component is in
_DENIED_ROOT_PARTS(
.ssh/.aws/.gnupg/.kube/.docker) oris_sensitive_path()matches.Same disposition as alerts 331–332 / 369–371 in
file_explorer/server.py.no linked issue: this ships a new builtin app that no filed issue requested — it graduates a previously side-loaded external app into a first-party builtin, so there is no bug or feature request for it to close. Recorded explicitly so the omission reads as deliberate rather than a forgotten closing keyword.