fix(cloud): ship prebuilt frontend and fail closed on missing dist - #2188
fix(cloud): ship prebuilt frontend and fail closed on missing dist#2188coozgan wants to merge 1 commit into
Conversation
95a853c to
5c54856
Compare
|
Update on the failing checks from the first CI run — all traced, all addressed: Root cause. The earlier failures were not from this change. The branch had fallen 41 commits behind Fix. Rebased onto the latest Verification (rebased branch, Python 3.12):
One maintainer action needed. After the force-push, GitHub held this fork's CI at Happy to make any changes the reviews raise. |
5c54856 to
5f3bd0b
Compare
|
Second CI failure — root-caused and fixed ( Root cause — a test-isolation bug, not this PR's change. The Fix (commit Verification:
This should also fix the same flake for every other open PR — the failing test ships on |
9800379 to
7ea7da7
Compare
|
Synced to latest Notes on the rebase:
Verification on the rebased branch (Python 3.12): The current |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands. BLOCKING -- src/kiro_crew/frontend.py:398 -- Untrusted build scripts execute on the operator host Adjudication (Opus 4.8) — is blocking on each finding proportionate?F1 (fenced) — The build runs on the operator host: 🏷️ Fenced finding(s) machine-flagged as likely edge caseThe security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)
|
|
🤖 Kiro Crew [operator: bolichen97] Round 7 — head GPT BLOCKING (source.py:459, dist files bypass the hardened read gate) — fixed, and this is the structural close of the dist-read hazard class. The finding is legitimate: a HARDLINK planted in dist aliasing Why the class is now closed, not just this instance: the read path no longer trusts anything derived from the directory walk — exclusion rules run on the arcname, and admission runs on the opened descriptor itself (inode type, link count, real path, sensitivity). There is no remaining bare GPT FINDING (cli_cloud.py:125 function-local import) — standing rebuttal, 4th occurrence: the module-scope form fails Local gates on |
|
|
🤖 Kiro Crew [operator: bolichen97] GPT 5.6 has now produced blocking findings in
Every fix so far was legitimate and landed, but the surface keeps yielding, and this round GPT's own prescription for (a) is "Revert the untracked dist-tree injection hunk" — that is no longer a bug report, it is a challenge to the PR's core design (ship the prebuilt dist so the box never runs npm). That call belongs to the maintainer and the PR author (@coozgan), not to this loop. The decision:
My recommendation: option 1 — it preserves the author's design, and the allowlist is the type-level analogue of the read-gate fix that closed round 7's class. Current state: head |
8105b44 to
0ffeef8
Compare
|
🤖 Kiro Crew [operator: bolichen97] Round 9 — head GPT BLOCKING (a) — gitignored secrets ship via GPT BLOCKING (b) — failure paths not atomic — fixed; shipping is now atomic over the bundle.
Docs ( Local gates on |
|
|
🤖 Kiro Crew [operator: bolichen97] Round 10 — head
No functional delta to round 9's allowlist + atomicity fixes. Local gates: 247 tests ✅ isort/flake8/mypy ✅ brand ✅. |
|
🤖 Kiro Crew [operator: bolichen97] The operator (bolichen97) ruled option 1 of the previous escalation and it shipped in round 9 ( GPT 5.6 has now produced its 5th consecutive blocking round on the same surface (
Each is strictly lower-severity than what rounds 4–9 already fixed, and the marginal prescription is now "content-scan every asset byte" — the endpoint of this chain is indistinguishable from GPT's round-8 prescription to revert the feature. Per the stall discipline this loop committed to (and the operator's ruling that the design stands), no further patches will be pushed on this chain by this loop. Requested from the maintainer / @coozgan: either
Findings 2 and 3, if a maintainer wants them, are one-line hardenings a follow-up can carry without blocking this PR. Current state: head |
|
Updated head: This implements the bounded hardening requested in the final escalation while preserving dist injection:
The governing cloud spec and regression tests are updated in the same commit. The PR currently shows |
|
Updated head: The approved Fixed narrowly in The replacement |
|
Final-head CI follow-up: the previous run on I therefore rebased the patch unchanged onto current The replacement CI/Build/review workflows for |
|
Rebased Provenance Hardening:
Local Gate Verification on
Workflows are currently held by GitHub at |
|
🤖 Kiro Crew [operator: zejiangg] Adopted by the drive-to-green reconciler (no triage spec) — diagnosed from live CI. Head Two rounds of work. Round 1 fixed both GPT blockers and removed the maintainer-only workflow gate; round 2 rebased to pick up main's own repairs. Round 1 — the two GPT 5.6 blockers, plus the fork-workflow guardGPT BLOCKING ( GPT FINDING ( Fork workflow-change guard — resolved by removing the One rebase repair: Round 2 — the remaining reds were main's, and main has since fixed themAfter round 1 the only failures were Backend Tests shards 2 and 4 (Linux 3.10/3.12 + Windows). Neither is reachable from this diff:
Both issues are now CLOSED, and my rebase base predated the repairs. Rebased onto Verification on
|
|
🤖 Kiro Crew [operator: zejiangg] Round 12 — head
Verification on |
|
🤖 Kiro Crew [operator: zejiangg] Answering the advisory
|
|
🤖 Kiro Crew [operator: zejiangg] Answering the advisory
|
|
🤖 Kiro Crew [operator: zejiangg] Round 13 — head
Verification on |
|
🤖 Kiro Crew [operator: zejiangg] Round 14 — head
Verification on |
|
🤖 Kiro Crew [operator: zejiangg]
All 67 of 69 checks pass. Design, Opus, First Principles and UX are all PASS on this head; 0 unresolved threads; Blocker 1 —
|
| Round | Exemption keyed on | How it was defeated |
|---|---|---|
| 12 | run length ≠ 40 chars | raw secret parked as a data: payload |
| 13 | position (any data: payload) |
same, position was enough to launder |
| 14 | content (payload decodes to media magic) | valid PNG with the secret appended — magic is at the start, so the whole run masks |
I verified round 15's variant before writing this: _data_uri_payload_is_media(PNG + SECRET) returns True, the literal key is present in the shipped CSS, and the actionable-warning list comes back empty. GPT is right.
Per this repo's own same-span discipline, the next move is the invariant, not a fourth patch — and the invariant question is a product call:
GPT's prescription is "remove the masking exemption; treat every bare-secret warning as actionable." That is airtight, and it is implementable in one line. Its cost is the thing only you can price: this repo's built editor CSS inlines a 231-char PNG data URI that trips the generic entropy heuristic, so with the exemption gone, admission refuses and every launch takes the _AbortInjection path — the original archive ships and the box builds with npm. That is the safe, documented fallback, not a breakage. But it also means the bundle would in practice never ship for this repo, which removes the PR's reason to exist. The author's test_real_bundled_data_uri_does_not_false_positive currently asserts the opposite, so it would have to be inverted.
So the choice is:
- Drop the exemption (GPT's fix). Scan is airtight; dist injection effectively never fires for this repo; invert that test. The PR shrinks to the health check,
install.shcompleteness,connectprobe, and doctor. - Keep the exemption and override the GPT lane on this head, accepting the residual: a credential deliberately appended to an inlined-media payload can ship to the operator's own private S3 and own token-gated box. Opus assessed this exact class on an earlier head and dropped it, on the grounds that these bytes derive only from operator-trusted source and cross no trust boundary — the operator's own gateway already serves them to any browser.
- Narrow further — mask only the maximal valid media prefix and scan the remainder. Kills the append class properly, but it is a fourth iteration on a span that has produced a new variant every round, and I would not ship it without your call given that track record.
My recommendation: option 2, on Opus's reasoning, with option 1 as the choice if you want the scan airtight and are willing to give up the shipping optimization. I am not taking either unilaterally — option 1 guts the PR's purpose and option 2 needs a human to accept a documented residual.
State
Head b123f25a, rebased onto main 61c89d45, single commit, @coozgan preserved with prior operators' co-author trailers intact. 325 tests pass locally; isort/flake8/mypy clean; black gate passes with the baseline untouched. Every finding across rounds 12-14 has a recorded disposition. Adding needs-human.
Also still open and unanswered by me only because it needs your call, from the First Principles lane: whether the two troubleshooting doc entries unrelated to the dist mechanism ride along or move to a docs PR.
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
…ssing dist The cloud launch flow builds the stock frontend from the exact filtered source archive in an isolated temporary root and appends only its admitted byte snapshot. A residual gitignored checkout bundle is never read. Every build or admission failure -- including an npm that resolves but cannot be executed, whose spawn error would otherwise escape as a traceback -- leaves the original archive unchanged so the EC2 box uses its required npm-build fallback. install.sh skips the on-box npm build only when the shipped bundle is COMPLETE (index.html plus every /assets/ chunk it references, the same completeness signal frontend._incomplete_bundle_reason uses) and fails closed when no frontend can be produced; a torn or partial staging falls through to the npm rebuild. The CloudFormation health check rejects the dashboard-less HTTP 200 guidance page, and cloud connect probes the forwarded dashboard before opening it. Dist admission scans text assets for credential-shaped content. The generic bare-secret entropy heuristic is exempted only for a data-URI payload that actually DECODES to a recognized media container, because inlined media is the one shape built assets false-positive on. Keying that exemption on content rather than position matters: `data:image/png;base64,` must not become a laundering prefix for a raw key. Every other bare-secret run is actionable -- including runs longer than 40 characters, which is what a genuine key glued to adjacent base64 characters looks like and precisely what security._contains_bare_secret exists to catch. Distinctive and encoded-credential matches stay actionable everywhere. Baselined files (cli_cloud.py, cloud/ec2.py, frontend.py, test_frontend_dist_resolve.py) keep their pre-existing formatting so the black baseline is untouched and the PR carries no .github/ change. Original author: Joshyfruit (coozgan). Co-authored-by: Bolin Chen <bolichen@amazon.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
|
Rebased onto main Conflicts resolved:
One rebase-induced fix: main's reboot-resume bootstrap grew UserData, so your WaitCondition health-check pushed the worst-case expansion to 14687 bytes, over Gates run locally on changed files: black (baseline-clean), isort, flake8, and Please review the resolution. A maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed. Reply if anything looks wrong. |
Problem / Motivation
kirocrew cloud launchships the local checkout to EC2 as an S3 tarball built from git-tracked files only (git archive HEAD/git ls-files). The built frontendsrc/kiro_crew/static/dist/is git-ignored and name-excluded ("dist"in_EXCLUDE_DIRS), so it never reaches the box — every launch must run the fullnpm ci && vite buildon the instance. That on-box build is the least reliable step of the bootstrap (npm registry flakes, dnf/NodeSource races, memory pressure on smaller tiers): when it fails, today's gates correctly fail the stack (KIROCREW_REQUIRE_FRONTEND=1+ the bootstrap's dist check), but the user still waits ~20 minutes for a rollback whose root cause was avoidable entirely.Why it matters
The dashboard IS the product on a cloud crew. Making the box's npm build unnecessary (instead of merely fail-closed) turns the common failure mode — on-box build breakage, ~20 minutes to a rollback — into a non-event.
The trade is reliability, not speed. The unreliable remote build is removed, but an equivalent cold
npm ci+ build is added on the laptop, where a failure is immediate, visible, and costs nothing but a retry. Wall-clock launch time is not claimed to improve.What changed
The frontend that ships is built from the packaged source on the laptop, not copied from the checkout's own
static/dist. That distinction is the whole provenance story: a gitignored checkout bundle has no verifiable relationship to the source being shipped, so no marker could safely bind the two. Rebuilding from the exact archived bytes makes the shipped JavaScript derive only from source that is actually in the tarball.cloud/source.py—_inject_dist()extracts the already-filteredwebsite/bytes out of the exact source archive into an isolated temporary root, runs a lockfile-exactnpm ci --ignore-scripts --no-audit --no-fundfollowed bynpm run buildwith edition-composition env vars removed, then appends only the admitted output. The checkout's ownstatic/distandwebsite/distare never read or touched. Admission has three gates — a hardened nolink read gate, a build-artifact extension allowlist (_DIST_ALLOWED_SUFFIXES, so an untracked non-asset file cannot ride along by name), and a credential content scan of text assets — and is atomic: any refusal raises_AbortInjection, the original archive ships unchanged, and the box falls back to its required npm build._exclude_filtergains an opt-inallow_dist_underexact-prefix param exempting only the literaldistcomponent; every other excluded dir (.aws,.ssh, …) and all credential-name/suffix checks still apply to injected members, and symlinks are never shipped. DefaultNonekeeps every existing caller byte-identical.data:<type>;base64,payload, because inlined media is the one shape real built assets false-positive on (a 231-char PNG run from this project's editor CSS does). Every other bare-secret run is actionable, including runs longer than 40 characters — that is what a genuine key glued to adjacent base64 characters looks like, and precisely whatsecurity._contains_bare_secretexists to catch. Distinctive and encoded-credential matches stay actionable everywhere, so the exemption cannot launder a key into an inlined image.cloud/ec2.py—deploy()packages the archived-source build described above.kirocrew cloud doctorreports whether a launch will ship a frontend (it checks the prerequisites —website/,package-lock.json,npm— not a pre-existing bundle).install.sh— skips the on-box npm build only when the shipped bundle is complete:index.htmlplus every/assets/chunk it references, the same completeness signalfrontend._incomplete_bundle_reasonapplies on the Python side.index.htmlalone does not prove a usable bundle, and an install retry that trusted it would skip the rebuild and serve a shell whose every chunk 404s. An incomplete tree falls through to the npm rebuild; cloud fail-closed behavior is unchanged and stays owned by the existing gates; local installs stay non-fatal.kirocrew-ec2.yaml— the WaitCondition health check now rejects theDashboard HTML not foundguidance page instead of accepting any 200 (a backstop at the serving layer, catching a box whose assets vanish after install succeeded).cli_cloud.py/cli_server.py—connectprobes the tunnel's local port with the existing_probe_dashboard_health()and warns on the marker page.remote-crew-on-ec2.md, and a spec sync incloud.md. The walkthrough's prerequisite is npm on the laptop, and it states that the launcher builds the frontend itself from packaged source; it does not ask the user to prebuild a bundle, since the launcher would ignore it.Declared cost
The laptop now runs
npm ci+vite buildon every launch (in a temp tree, so it never disturbs the checkout the local gateway serves). That is a deliberate trade: a bounded local cost in exchange for removing the flakiest remote step. Every gate failure — including--ignore-scriptsbreaking a dependency that needs a lifecycle script — falls back to the on-box npm build, which is the pre-existing behavior and is fail-closed at the stack level.Tests
test_cloud_source.py: dist members injected; no-dist unchanged; symlinks never shipped; dist-root symlink outside the checkout refused (inside allowed); incomplete bundle (missing hashed chunks) refused;.env/.pemand.aws/.sshinside dist still filtered; siblingdistprefixes stay excluded; non-asset extensions refused by the allowlist while typical Vite output ships; a refused read-gate object or refused index-referenced chunk aborts the whole injection; an exact 40-char bare AWS secret and anAKIAkey id abort; a secret glued to adjacent base64 chars aborts (4 parametrized shapes); an encoded credential inside adata:URI aborts; a real bundled PNG data URI does not false-positive.test_cloud_cli.py: connect probes the local port; doctor reports frontend-build prerequisites.test_frontend_dist_resolve.pycovers the archived-source build helper. All cloud test files plus the CLI lazy-import ratchet pass; isort/flake8/mypy clean on touched modules; black gate passes with the baseline untouched.The two glued-secret pins are mutation-verified: restoring the previous length-keyed exemption fails all four cases.
Manual verification
Launch-path behaviors (S3 upload, CFn health check) are exercised via the mocked deploy tests, and the
install.shcompleteness check was verified against four cases (partial bundle refused, complete accepted, missing index refused,/manifest.js-only index accepted since that route is gateway-served rather than a bundle chunk). A full live launch is environment-dependent and covered by the existing cloud launch flow; the isolatednpm ci+ build path is not exercised end-to-end in CI, so a first real launch is where that step gets its live proof.Why no screenshot: backend/CLI/infra change only — no rendered UI delta.
no linked issue: fixes a launch-reliability gap found in operation, no tracked issue exists.