Skip to content

fix(cloud): ship prebuilt frontend and fail closed on missing dist - #2188

Open
coozgan wants to merge 1 commit into
kirodotdev:mainfrom
coozgan:fix/cloud-launch-prebuilt-frontend
Open

fix(cloud): ship prebuilt frontend and fail closed on missing dist#2188
coozgan wants to merge 1 commit into
kirodotdev:mainfrom
coozgan:fix/cloud-launch-prebuilt-frontend

Conversation

@coozgan

@coozgan coozgan commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

kirocrew cloud launch ships the local checkout to EC2 as an S3 tarball built from git-tracked files only (git archive HEAD / git ls-files). The built frontend src/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 full npm ci && vite build on 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-filtered website/ bytes out of the exact source archive into an isolated temporary root, runs a lockfile-exact npm ci --ignore-scripts --no-audit --no-fund followed by npm run build with edition-composition env vars removed, then appends only the admitted output. The checkout's own static/dist and website/dist are 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_filter gains an opt-in allow_dist_under exact-prefix param exempting only the literal dist component; every other excluded dir (.aws, .ssh, …) and all credential-name/suffix checks still apply to injected members, and symlinks are never shipped. Default None keeps every existing caller byte-identical.
  • Credential scan scope — the generic bare-secret entropy heuristic is exempted only inside a declared 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 what security._contains_bare_secret exists to catch. Distinctive and encoded-credential matches stay actionable everywhere, so the exemption cannot launder a key into an inlined image.
  • cloud/ec2.pydeploy() packages the archived-source build described above. kirocrew cloud doctor reports 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.html plus every /assets/ chunk it references, the same completeness signal frontend._incomplete_bundle_reason applies on the Python side. index.html alone 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 the Dashboard HTML not found guidance 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.pyconnect probes the tunnel's local port with the existing _probe_dashboard_health() and warns on the marker page.
  • Docs — beginner walkthrough + three troubleshooting entries in remote-crew-on-ec2.md, and a spec sync in cloud.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 build on 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-scripts breaking 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/.pem and .aws/.ssh inside dist still filtered; sibling dist prefixes 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 an AKIA key id abort; a secret glued to adjacent base64 chars aborts (4 parametrized shapes); an encoded credential inside a data: 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.py covers 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.sh completeness 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 isolated npm 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.

@coozgan
coozgan requested a review from a team as a code owner August 8, 2026 08:20
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 8, 2026
@coozgan
coozgan force-pushed the fix/cloud-launch-prebuilt-frontend branch from 95a853c to 5c54856 Compare August 8, 2026 14:40
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 8, 2026
@coozgan

coozgan commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

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 main, so the PR's merge ref pulled in newer commits, including a known-flaky test_knowledge_kiroignore.py pair (see #2184, which fixed them once already). Those two tests failed across the Linux 3.10/3.12 and Windows shard-2 lanes; the Coverage Gate and PR Readiness then failed as knock-ons. The Automated Rule Check failure ("use lucide-react icons") flagged inline SVGs that came from those same newer frontend commits in the merge ref — this PR touches no .tsx/.jsx and adds no SVGs.

Fix. Rebased onto the latest main (906557d1, which carries the #2184 kiroignore fix). Now 0 commits behind.

Verification (rebased branch, Python 3.12):

  • test_cloud_source.py, test_cloud_cli.py, test_cloud_connect.py — the files covering this change: 97/97 pass.
  • test_knowledge_kiroignore.py (the tests that failed in CI): 32/32 pass.
  • Combined targeted run: 129/129 pass.
  • isort / flake8 / mypy clean on the touched files (the handful of full-tree findings pre-exist on main).

One maintainer action needed. After the force-push, GitHub held this fork's CI at action_required (first-time-contributor workflow approval). I can't approve it (403 — must have admin rights). Could a maintainer click "Approve and run" on the held workflows? Once they run, the backend suites and the AI-review lanes (Opus 5 / GPT 5.6 / Design / UX) should complete and the readiness gate can settle.

Happy to make any changes the reviews raise.

@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 Aug 8, 2026
@coozgan
coozgan force-pushed the fix/cloud-launch-prebuilt-frontend branch from 5c54856 to 5f3bd0b Compare August 8, 2026 15:49
@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 Aug 8, 2026
@coozgan

coozgan commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Second CI failure — root-caused and fixed (test_cli.py::TestDoctorEmbeddings::test_doctor_names_the_missing_native_libs on the Windows shard-1 lane, and earlier on Linux 3.12 shard 1).

Root cause — a test-isolation bug, not this PR's change. The TestDoctorEmbeddings._run_doctor harness stubs _load_llama_class with a _load() that mimics the real loader via a raw os.environ.setdefault("LLAMA_CPP_LIB_PATH", …). monkeypatch can't undo a raw os.environ mutation, so when test_doctor_does_not_mistake_the_loaders_own_setdefault_for_an_override runs first on the same xdist worker, the var leaks into the next test. cli_doctor reads that var before loading (cli_doctor.py:769) and treats a non-empty value as an operator override → it skips verify_vendored_libs() → the "Missing native libs for macos_arm64: …" line is never printed → the assertion fails. Reproduced locally by running the two tests in that order on one worker (-n 0): 1 failed, 1 passed. It passes in isolation, which is why it was green locally and only flaked on specific CI shards (test distribution differs per OS/Python).

Fix (commit 9800379d, in this PR): route the stub through monkeypatch.setenv instead of raw os.environ.setdefault, so the side effect is undone at teardown and can't leak. 5 lines changed in test/test_cli.py only — no production code touched.

Verification:

  • The exact failing order now passes: 2 passed.
  • Whole TestDoctorEmbeddings class under xdist: 9 passed.
  • Full test_cli.py + test_vendored_llama_payload.py: 258 passed, 2 skipped.
  • isort / flake8 clean on the touched file.

This should also fix the same flake for every other open PR — the failing test ships on main.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 10, 2026
@coozgan
coozgan force-pushed the fix/cloud-launch-prebuilt-frontend branch from 9800379 to 7ea7da7 Compare August 10, 2026 10:28
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention labels Aug 10, 2026
@coozgan

coozgan commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Synced to latest main (adbec83b, +160 commits) — rebased, 0 behind.

Notes on the rebase:

  • One real conflict: install.sh (upstream bumped the Node.js guidance to "22+ / 24 LTS" on a line this PR also touched). Resolved taking upstream's message, keeping this PR's _frontend_failed fail-closed wiring.
  • The test-isolation commit I added yesterday (9800379d, the LLAMA_CPP_LIB_PATH xdist leak in TestDoctorEmbeddings) is dropped — upstream main now carries an equivalent fix in the same harness, so there was nothing left to apply.

Verification on the rebased branch (Python 3.12): test_cloud_source.py + test_cloud_cli.py + test_cloud_connect.py + test_cli.py::TestDoctorEmbeddings106/106 pass; isort / flake8 / mypy clean on the touched files; install.sh passes bash -n.

The current PR Readiness: fail — 3 blocking readiness item(s) is GitHub's first-time-contributor hold, not a test failure: the CI, Build, and review workflows are queued at action_required and the gate counts "not run" as blocking. Could a maintainer click "Approve and run" on the held workflows for this PR? They should go green from there.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed c803a364da80364c6670fb64becc809bdc3b69d5 via the fork AI-review pipeline; updated in place on each push.

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
["run", "build"],
Malicious website/package.json -> cloud launch packaging -> npm run build -> arbitrary commands execute with operator filesystem and credential access.
Anchor: residual/security
Fix: Execute the archived frontend build in a filesystem-confined sandbox with credentials hidden, environment scrubbed, and build-time network restricted.
[BLOCK-MERGE] c803a36
[GPT-REVIEWED] c803a36

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

F1 (fenced) — The build runs on the operator host: source.py:599 calls frontend.build_stock_frontend, which spawns npm ci/npm run build (diff frontend.py _run_npm_step). But the source built is the operator's OWN editable checkout — repo_root() returns "the installed package's repo" (source.py:102-113), and _inject_dist builds website/ from that exact archive. That identical build script already executed on the operator's host at install time: install.sh:414-428 runs npm ci && npm run build on the same website/ source. For website/package.json to be "malicious," the operator's own installed source tree must already be compromised — a pre-existing self-compromise this PR does not newly expose, and the new npm ci --ignore-scripts even narrows the dependency-lifecycle-script surface install.sh's plain npm ci runs. Recovery/rarity: the precondition is a trusted input the operator supplies from their own filesystem and which has already run; no new trust boundary is crossed. This meets the FLAG bar — the finding is real but the reaching condition is one the system's own trusted writer produces only if already compromised, which a human would plausibly accept.

[ADJUDICATION] c803a364da80364c6670fb64becc809bdc3b69d5 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] c803a364da80364c6670fb64becc809bdc3b69d5
[ADJUDICATION-FENCED] c803a364da80364c6670fb64becc809bdc3b69d5 fenced=1 flagged=1
FLAG F1 src/kiro_crew/frontend.py:398 -- The build source is the operator's OWN trusted checkout (repo_root, source.py:102), whose identical `npm run build` already ran on the same host at install time (install.sh:414-428), so no untrusted input reaches this build.
[GPT-ADJUDICATED-FENCED] c803a364da80364c6670fb64becc809bdc3b69d5

🏷️ Fenced finding(s) machine-flagged as likely edge case

The 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.)

  • F1 src/kiro_crew/frontend.py:398 — The build source is the operator's OWN trusted checkout (repo_root, source.py:102), whose identical npm run build already ran on the same host at install time (install.sh:414-428), so no untrusted input reaches this build.

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]

Round 7 — head 8105b442 (was ecd1fea2), push iteration 7/10.

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 ~/.aws/credentials has an in-checkout path and is not a symlink, so both the is_symlink() skip and the resolve fence pass it, and the bare open() reads the credential. This was the 3rd blocking round in the dist-shipping chain, so per our own stall discipline this round replaces the hand-rolled fences with the codebase's hardened primitive rather than adding another patch: every member is now read through hooks.safe_read_file_bytes_nolink(within_root=<resolved dist>) — O_NOFOLLOW open + fstat on the SAME descriptor rejects hardlinked inodes (st_nlink > 1) and non-regular types race-free, and the fd-real-path containment check subsumes the manual resolve fence (closing its lstat-then-open TOCTOU window as a bonus). This is the exact keystone AUTOSDE.yaml's backend-security-controls names for file reads. Regression test: test_hardlinked_credential_in_dist_refused (hardlinked credential never ships; legit members still do).

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 open()/stat() in the injection path for a planted filesystem object to race.

GPT FINDING (cli_cloud.py:125 function-local import) — standing rebuttal, 4th occurrence: the module-scope form fails test_cli_lazy_imports on 3 CI shards (issue #3504); cli.py defers every cli_server import identically. Recorded in the ledger each round.

Local gates on 8105b442: 171 tests ✅ isort/flake8/mypy ✅ bash -n ✅ brand ✅.

@bolichen97

Copy link
Copy Markdown
Collaborator
  • fixed — Dist files bypass the hardened read gate (src/kiro_crew/cloud/source.py:459)

All dist member reads now go through hooks.safe_read_file_bytes_nolink(within_root=): O_NOFOLLOW + same-descriptor fstat rejects hardlinks (st_nlink>1) and non-regular types race-free; fd-real-path containment subsumes the manual resolve fence and closes its TOCTOU window. Structural fix per the backend-security-controls keystone, not another point fence. Pinned by test_hardlinked_credential_in_dist_refused. Fixed in 8105b44.

  • rebutted — function-local from kiro_crew.cli_server import _probe_dashboard_health (src/kiro_crew/cli_cloud.py:125)

Standing rebuttal, 4th occurrence (span d7e442e5752d): module-scope form fails test_cli_lazy_imports on 3 CI shards (cli_server → dashboard.state → vector_memory → numpy, issue #3504); cli.py defers every cli_server import identically. The ratchet test outranks the style rule.

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

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]

⚠️ Escalation — needs a maintainer decision. Pausing the drive-to-green loop (7 push iterations used).

GPT 5.6 has now produced blocking findings in source.py:_inject_dist in 4 consecutive review rounds (6 distinct blockers), each round attacking a new residual angle of the same surface — shipping the untracked static/dist tree:

Round Head Blocker Fixed by
4 5c1ad131 dist-root symlink escape; staging-transaction bypass resolve fence + staging lock + completeness check
6 d7ef9621 in-launch build mutates the live source-install frontend removed in-launch build (reversed round 5's gate)
7 ecd1fea2 hardlinked credential bypasses the fences all reads via hooks.safe_read_file_bytes_nolink (structural round)
8 8105b442 (a) gitignored secrets: Vite copies website/public/* into dist verbatim, so an untracked public/secrets.yaml ships to S3; (b) failure paths not atomic: a refused member still ships the index that references it paused here

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:

  1. Keep dist injection, constrain the surface — add a Vite-output extension allowlist (.html .js .css .map .svg .png .ico .woff2 .json …) so only build-artifact-typed files ship; a stray secrets.yaml/.pem/anything else in public/ is refused by type. Plus the (b) atomicity fix (on lock/member-read failure discard the injected archive and ship the original, so a partial dist never rides). Both are small, tested, and I can push them within one iteration — but a 5th round on this surface without a design ruling is throwing good rounds after bad.
  2. Adopt GPT's prescription — revert dist injection entirely. The box always builds with npm (today's behavior); the PR shrinks to the health-check + doctor + python3.12 changes. Gut-renovates the PR's stated purpose, so it needs @coozgan's sign-off.
  3. Accept the residual risk as-is — argue website/public/ contents are already served verbatim by the user's own local gateway, so shipping them to the user's own private S3 + token-gated box crosses no new trust boundary. Defensible but weaker than option 1, and GPT will keep blocking on it.

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 8105b442, all CI green except GPT 5.6 Review (the two blockers above); Design fresh-pass, Opus pending re-stamp. Single squashed commit, author preserved, co-author trailer intact. Work dir /tmp/kc-drive-2188 left in place.

@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 Aug 15, 2026
@bolichen97
bolichen97 force-pushed the fix/cloud-launch-prebuilt-frontend branch from 8105b44 to 0ffeef8 Compare August 16, 2026 08:47
@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]

Round 9 — head 0ffeef89b (was 8105b442), push iteration 8/10. Implements option 1 from the escalation (operator ruling: keep the author's dist-injection design, constrain the shipping surface by type, make failure atomic).

GPT BLOCKING (a) — gitignored secrets ship via website/public/ — fixed with a type allowlist. Injected members now pass a third gate, _DIST_ALLOWED_SUFFIXES: only build-artifact extensions (.html .js .mjs .cjs .css .map .json .webmanifest .wasm + image/font types) are admitted. Vite copies website/public/* into dist verbatim, so an untracked secrets.yaml, id_rsa_backup, or notes.txt parked there is refused by type, independent of the name/suffix denylists — an allowlist cannot be extended by naming a file cleverly, where a denylist can be sidestepped. This is the type-level analogue of round 7's read-gate fix. Pins: test_non_asset_extension_refused_by_allowlist (secrets.yaml / extensionless / .txt refused, legit members ship), test_allowlist_admits_typical_vite_output (hashed chunks, sourcemaps, webmanifest, fonts, images, wasm, vendor/*.mjs — the full shape of this repo's real dist — all ship).

GPT BLOCKING (b) — failure paths not atomic — fixed; shipping is now atomic over the bundle. _inject_dist restructured into admit-then-write under the staging lock:

  • A read-gate rejection aborts the whole injection (new semantics): a hardlink/non-regular object has no legitimate shape inside a Vite build tree, so instead of shipping the bundle around it, nothing ships and the box builds with npm. test_hardlinked_credential_in_dist_refused updated to pin the abort.
  • After admission, every /assets/ chunk index.html references must be in the admitted set (frontend._index_asset_refs, the same extraction _incomplete_bundle_reason uses, now shared). A referenced chunk refused admission — e.g. a symlinked chunk, which the old code silently skipped while still shipping the index that references it — aborts the injection. New pin: test_refused_referenced_chunk_aborts_injection.
  • Both aborts return the ORIGINAL archive unchanged (_AbortInjection → unlink temp, log, fall back), so a partial dist never rides and install.sh never sees an index whose chunks 404.

Docs (docs/system-specs/modules/cloud.md) updated to state the three admission gates and the atomicity invariant.

Local gates on 0ffeef89b: 272 tests across cloud/frontend/lazy-import suites ✅ (53 in test_cloud_source.py incl. 3 new pins) isort/flake8/mypy ✅ bash -n ✅ brand ✅ inclusive-language diff 0 hits ✅.

@bolichen97

Copy link
Copy Markdown
Collaborator
  • fixed — gitignored secrets in website/public/ ship to S3 via dist injection (src/kiro_crew/cloud/source.py)

Type allowlist added (_DIST_ALLOWED_SUFFIXES): only build-artifact extensions are admitted, so an untracked secrets.yaml/extensionless credential Vite copied from website/public/ is refused by TYPE, independent of the name/suffix denylists. Operator ruled option 1 of the escalation (keep the author's dist-injection design, constrain the surface) over the "revert the injection hunk" prescription. Pinned by test_non_asset_extension_refused_by_allowlist and test_allowlist_admits_typical_vite_output. Fixed in 0ffeef8.

  • fixed — failure paths not atomic: a refused member still ships the index that references it (src/kiro_crew/cloud/source.py)

_inject_dist restructured to admit-then-write under the staging lock: a read-gate rejection aborts the WHOLE injection (planted objects have no legitimate shape in a build tree), and every index-referenced /assets/ chunk must be in the admitted set (shared frontend._index_asset_refs) or the injection aborts — the original archive ships unchanged and the box builds with npm. Pinned by test_refused_referenced_chunk_aborts_injection and the updated test_hardlinked_credential_in_dist_refused. Fixed in 0ffeef8.

  • rebutted — function-local from kiro_crew.cli_server import _probe_dashboard_health (src/kiro_crew/cli_cloud.py:125)

Standing rebuttal, 5th occurrence (span d7e442e5752d): module-scope form fails test_cli_lazy_imports on 3 CI shards (cli_server → dashboard.state → vector_memory → numpy, issue #3504); cli.py defers every cli_server import identically. The ratchet test outranks the style rule.

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]

Round 10 — head 819394806 (was 0ffeef89b), push iteration 9/10. Two CI failures on the previous head, both resolved:

  • Brand Name Gate (real, mine): a comment added in round 9 spelled the product name joined ("a KiroCrew writer", source.py:480). Reworded to "a Kiro Crew writer". Local gate rerun clean.
  • Automated Rule Check (base-drift leak, no code change): the model-selection rule flagged "cc_model": "claude-sonnet-4.6" at a diff line this PR never touched — it is main's feat(models): configurable per-role model + reasoning effort for background and sub-agent work #1600 (per-role model config) drift showing up in the base.sha..HEAD two-dot scan, same class as the round-4 Inclusive Language leak. Verified: 0 hits in the true merge-base diff. Cleared by rebasing onto current main (64060f3f).

No functional delta to round 9's allowlist + atomicity fixes. Local gates: 247 tests ✅ isort/flake8/mypy ✅ brand ✅.

@bolichen97

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: bolichen97]

⚠️ Final escalation — the drive-to-green loop is stopping here (9 push iterations used). Maintainer ruling requested.

The operator (bolichen97) ruled option 1 of the previous escalation and it shipped in round 9 (0ffeef89b, brand-fix rebased as 819394806): a Vite-output type allowlist (_DIST_ALLOWED_SUFFIXES) so untracked non-artifact files in website/public/ are refused by type, plus atomic shipping (any read-gate rejection or index-referenced chunk refused admission aborts the whole injection; the original archive ships unchanged). Both pinned by regression tests. All CI is green on this head; Design, UX, and First Principles all PASS (FP explicitly verified the three admission gates "each trace to a real constraint").

GPT 5.6 has now produced its 5th consecutive blocking round on the same surface (_inject_dist / dist shipping), 9 distinct blockers cumulative. This round's three:

  1. Credential-bearing text inside legit asset types (config.js with a token baked in) — this is no longer a planted-object or file-type hazard; it demands content scanning of asset bytes. The user's own gateway serves these same bytes to any browser today; shipping them to the user's own private S3 + token-gated box crosses no new trust boundary.
  2. _incomplete_bundle_reason reads index.html via read_text() — a symlinked index is already refused by the walk (never shipped); this read is a completeness probe of a tree the same function then fences member-by-member through the hardened gate.
  3. ANSI escapes in a diagnostic string — the "missing asset" name comes from the user's own index.html on their own machine, printed to their own terminal.

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

  • /ai-review override for the GPT check on 819394806c9906b14f2159898f50cb6ca0be1231 (Design/UX/FP all pass; CI fully green), or
  • rule that finding 1 warrants a content-scan gate (a bounded add — reuse the repo's credential-pattern scan over decoded text-type bytes at admission time) and hand it to a fresh session, or
  • rule for reverting the injection design (GPT's standing prescription).

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 819394806, CI green, GPT Review the sole red check. Single squashed commit, author coozgan preserved, co-author trailer intact. Work dir /tmp/kc-drive-2188 retained.

@coozgan

coozgan commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Updated head: beef5984d627bda25b8f647d0e6daadbd2d09f9c, rebased onto current main 59b5e00b (0 behind).

This implements the bounded hardening requested in the final escalation while preserving dist injection:

  • text-type frontend assets are read once through safe_read_file_bytes_nolink, decoded as strict UTF-8, and checked with the shared credential detector without mutating bundle bytes; canonical/plaintext and encoded credentials plus exact 40-character bare AWS secret keys abort injection;
  • longer generic bare-secret warnings are ignored because real built editor/Mermaid assets contain minified/data-URI runs that trigger that heuristic (a regression fixture pins this);
  • index.html is parsed from the same hardened byte snapshot that is scanned and written, eliminating the path-based read bypass;
  • untrusted filenames and HTML asset references are repr-escaped in diagnostics, preventing terminal control-character injection;
  • any refusal remains atomic: the original archive is unchanged and the EC2 box falls back to its fail-closed npm build.

The governing cloud spec and regression tests are updated in the same commit. The PR currently shows PR Readiness: 3 blocking item(s) because all substantive fork workflows (CI, Build, and review lanes) are held at action_required; no jobs have run on this head yet. A maintainer must click Approve and run on the held workflows. Once approved, I will monitor every current-head gate and address any real failure.

@coozgan

coozgan commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Updated head: c0944efbeff90e80a05388a206e5ec4bff997fe3, rebased onto current main 2e500e6d7b76794b76cb219a559f16afcd689589 (0 behind).

The approved beef5984 CI run found one deterministic failure in Linux 3.10 shard 3, Linux 3.12 shard 3, and Windows shard 3: cloud/source.py called redact_credentials but was not classified by the security-posture omission gate.

Fixed narrowly in security_posture.NON_EGRESS_REDACTION_MODULES: cloud/source.py uses the scanner only as an admission detector, discards the redacted output, and either archives the original bytes unchanged or aborts dist injection. It is not registered as an output sink, and the omission test was not weakened.

The replacement CI, Build, and review workflows are again held at action_required; a maintainer must click Approve and run for head c0944efb. Local Python execution remains unavailable in the contributor harness, so the approved GitHub run is the executable validation source.

@coozgan

coozgan commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Final-head CI follow-up: the previous run on 13f3125e had one infrastructure-style failure in Backend Tests (3.10, 4). At 54%, xdist reported gw1 as "Not properly terminated", replaced it, reached 99%, then stalled until GitHub canceled the 30-minute job. No pytest timeout or assertion failure was reported; the equivalent upstream shard completed normally, and every other backend, Windows, frontend, E2E, lint/type, security, CloudFormation, and build lane passed. Fork contributors cannot rerun failed upstream jobs (Must have admin rights).

I therefore rebased the patch unchanged onto current main fce64a94 and force-pushed the single patch-identical commit as 1cdcc008 (range-diff exact, 0 behind / 1 ahead). Docs-lint and diff checks pass locally; other local Python commands remain blocked before process start by the contributor harness classifier outage, so GitHub CI is the executable validator.

The replacement CI/Build/review workflows for 1cdcc008 are now held at action_required. Could a maintainer click Approve and run? I will inspect the complete replacement matrix, including Python 3.10 shard 4 and Coverage Gate.

@coozgan

coozgan commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Rebased fix/cloud-launch-prebuilt-frontend onto current main (69b60c3b) and force-pushed head 8271764e. Single squashed commit, co-author trailers preserved.

Provenance Hardening:

  • Replaced untracked checkout-dist reading with an archive-bound isolated build (_build_archived_dist).
  • Extracts only filtered website/ source bytes from the exact tarball into an isolated temporary tree.
  • Runs lockfile-exact npm ci --ignore-scripts --no-audit --no-fund followed by npm run build with edition composition env vars removed.
  • Validates the resulting dist root containment and runs all admitted assets through the existing nolink/content/completeness admission gates.
  • Failed or refused builds leave the original source archive unchanged to use the required on-box npm fallback.

Local Gate Verification on 8271764e:

  • test_cloud_source.py, test_cloud_cli.py, test_cloud_ec2.py, test_frontend_dist_resolve.py, test_cli_lazy_imports.py: 259/259 pass.
  • flake8 and mypy clean on touched production modules.
  • Formatter/isort clean on touched files.
  • bash -n install.sh clean.
  • Brand name gate (scripts/check_brand_name.py) clean.

Workflows are currently held by GitHub at action_required for first-time contributor approval. Maintainers can click "Approve and run" to run CI and AI review suites.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]

Adopted by the drive-to-green reconciler (no triage spec) — diagnosed from live CI. Head 7f3c7b43, rebased onto current main 6b9dcd78.

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 guard

GPT BLOCKING (install.sh:373) — incomplete frontend bypasses the required rebuild — fixed. The skip branch tested only -f .../static/dist/index.html, so a failed or torn Vite build that left an index behind would make an install RETRY skip the npm rebuild and serve a shell whose every chunk 404s. The skip now requires the bundle to be complete: index.html plus every /assets/*.js|.css it references present on disk — the same completeness signal frontend._incomplete_bundle_reason already applies on the Python side. An incomplete tree falls through to the npm rebuild, so the fail-closed behavior the gates provide is preserved rather than bypassed. Verified against four cases: partial bundle refused, complete bundle accepted, missing index refused, and a /manifest.js-only index accepted (that route is gateway-served, not a bundle chunk, and must not read as missing).

GPT FINDING (remote-crew-on-ec2.md:40) — guide contradicts the implementation — fixed. Prerequisite 3 told users to cd website && npm ci && npm run build because "the launcher ships your checkout's existing static/dist bundle". The shipped code deliberately does the opposite: _inject_dist never reads the checkout's dist and rebuilds from the archived website/ source in an isolated temp root. Rewritten to describe the actual mechanism (npm on the laptop is the prerequisite; the build is automatic; the checkout's own dist is never read or touched). This also resolves the matching Design Review and First Principles CONCERNS, both of which flagged this same description↔code divergence as their headline item.

Fork workflow-change guard — resolved by removing the .github/ change entirely. The PR was pruning four paths from .github/black-baseline.txt because it had reformatted those files. That made it a fork PR touching .github/**, which trips an anti-spoofing guard no code fix can clear — it needs a maintainer label. Since the reformats were incidental to the feature, I separated them out: for each of cli_cloud.py, cloud/ec2.py, frontend.py, and test_frontend_dist_resolve.py I computed the functional patch as black(main) → PR and replayed it onto main's original unformatted file. All four files keep their pre-existing formatting, the baseline is untouched, and the PR no longer edits .github/** at all (17 files → 16). Equivalence was verified mechanically, not by eye: formatting each reconstructed file reproduces the PR's version byte-for-byte, so no functional edit was lost. The black gate passes with the baseline unchanged.

One rebase repair: test_cloud_cli.py monkeypatched shutil.which with lambda name:, but main's deploy/engine.py now calls it with path=, so the shim raised TypeError. Widened to lambda name, **kw:.

Round 2 — the remaining reds were main's, and main has since fixed them

After 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 6b9dcd78 (20 commits) and confirmed locally: test_trust_reads.py + test_kiro_usage_api.py now 196 passed at this base. Notably, GPT 5.6 Review had also stopped reporting a verdict — the fork review lanes gate on workflow_run.conclusion == 'success', so red CI leaves their stamps pinned to an older head regardless of the diff's merits.

Verification on 7f3c7b43

272 passed across test_cloud_source.py, test_cloud_cli.py, test_cloud_ec2.py, test_frontend_dist_resolve.py, test_spawn_audit.py, test_cli_lazy_imports.py. isort / flake8 / mypy clean on all touched production modules; black gate passes with the baseline unchanged; brand-name gate and docs-lint clean; bash -n install.sh clean.

No design, approach, or architecture change: the isolated archived-source build, the three admission gates, and the atomicity invariant are all the author's, untouched. Author @coozgan preserved; prior operators' co-author trailers kept.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]

Round 12 — head 021c9ddf (was 7f3c7b43), rebased onto current main 056e768f. One finding fixed, two rebutted with evidence.

  • fixed — longer AWS secret runs bypass admission (src/kiro_crew/cloud/source.py:471)

Legitimate and reachable, and the Opus lane independently re-derived the same mechanism on this surface. security._contains_bare_secret exists specifically to catch a genuine 40-char key glued to adjacent base64 characters (X+key, key+ABC, key+X+key), all of which report a 41+ char run — so keying the filter on the exact (40 chars) string discarded precisely the signal the upstream helper is built to produce. Fixed by removing the false-positive SOURCE instead of exempting the signal: declared data:<type>;base64, payloads are masked before the scan, and every remaining bare-secret warning is actionable. That satisfies this finding's literal prescription ("treat every bare-secret warning as actionable") while keeping test_real_bundled_data_uri_does_not_false_positive green — a blunt "all warnings actionable" alone would abort on the real 231-char PNG run in this project's built editor CSS. The exemption is also narrowed to the entropy heuristic ONLY: distinctive and encoded-credential matches stay actionable everywhere, so a key cannot be laundered by base64-ing it into an inlined image. That last case was caught by self-review and is a gap BOTH the original filter and a mask-the-whole-scan version would have shipped. Pinned by test_glued_bare_aws_secret_in_text_asset_aborts_injection (4 parametrized cases) and test_encoded_credential_inside_data_uri_aborts_injection; mutation-verified — restoring the old exemption fails all 4 glued cases.

  • rebutted — cloud launch executes untrusted checkout scripts (src/kiro_crew/frontend.py:398)

Two independent reasons, and the prescription is out of scope for this loop. First, no new trust boundary: origin/main's own frontend.py:85-86 documents that the runtime rebuild (POST /api/update, kirocrew update, and the gateway's auto-apply) already shells npm run build in the SAME checkout — so the capability exists on main today, reachable from an HTTP endpoint, whereas this PR runs the same command with a narrower environment (edition vars stripped, npm ci --ignore-scripts for install so lifecycle scripts never run). A package.json that can attack the operator already has execution on a host whose gateway runs from that very checkout. Second, the prescription is "Revert the local archived-frontend build and retain the on-box fallback" — that is a request to revert this PR's core design, not a defect fix, and it is the same revert prescription this lane issued at round 8. Reverting the author's approach is a maintainer/product call, and this drive is explicitly scoped to making the author's existing change pass CI and review without altering its design. Flagging for the maintainer rather than acting on it.

  • rebutted — inline from kiro_crew.cli_server import _probe_dashboard_health violates top-level-imports (src/kiro_crew/cli_cloud.py:126)

Standing rebuttal, 6th occurrence of span d7e442e5752d, now verified empirically rather than argued from code reading. I applied this finding's own prescription (moved the dependency to file scope) and ran the ratchet: test_cli_lazy_imports.py::test_every_deferred_dispatch_import_resolves fails with module-scope import of kiro_crew.cli loaded deferred module...kiro_crew.cli_server is an explicit entry in that test's forbidden-at-import-time set (its dashboard.state → vector_memory → numpy edge is the measured cost, issue #3504). The alternative prescription ("remove the inline probe") would delete the dashboard-health warning, i.e. a feature. So neither prescription is applicable, the deferred form is load-bearing, and cli.py defers every cli_server import identically. Advisory (FINDING), so non-blocking either way.

Verification on 021c9ddf: 278 passed across the cloud/frontend/lazy-import/spawn-audit suites; isort/flake8/mypy clean on touched modules; black gate passes with the baseline untouched.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]

Answering the advisory CONCERNS on head 7f3c7b43. Both items were legitimate; neither needed a code change, so this is prose-only and the diff is unchanged.

  • fixed — description ↔ diff drift: the "What changed" bullets described a design the code rejects

Confirmed against the code, not just the report. The body claimed _inject_dist() "snapshots under frontend._staging_lock" (no such call exists in the shipped function), that deploy() "packages the checkout's existing eligible dist (it never rebuilds the frontend in-launch)" (the opposite of what _build_archived_dist does), and that the template "prefers python3.12 (AL2023.3+) with python3.11 fallback" (the yaml hunks touch only the health check and a timeout comment). All three are gone. The body now leads with the provenance rationale — the shipped frontend is built from the archived source precisely BECAUSE a gitignored checkout bundle has no verifiable relationship to the source being shipped — and states plainly that the checkout's own static/dist / website/dist are never read or touched. The same stale story was also live in the user-facing guide (remote-crew-on-ec2.md prerequisite 3 told users to prebuild a bundle the launcher ignores); that was corrected in the code change on head 7f3c7b43, which is what makes the docs and the description agree now.

  • fixed — hides a real per-launch cost

Also legitimate: npm ci + vite build now run on the laptop on every launch, and the previous body implied a build-once-ship-many model. The body carries a dedicated Declared cost section stating the per-launch build explicitly, noting it runs in a temp tree so it never disturbs the checkout the local gateway serves, and naming it as a deliberate trade — a bounded local cost for removing the flakiest remote step.

  • accepted-and-deferred — the benefit can silently evaporate; nothing proves the real isolated build succeeds

Legitimate and unfixed, deliberately. Every gate failure (including --ignore-scripts breaking a dependency that needs a lifecycle script) falls back to the on-box npm build, surfaced only as a logger.warning, and all tests mock _run_npm_step / _build_archived_dist — so doctor can report prerequisites as ready while no bundle ever ships. I have not fixed it here because both remedies you name are new surface rather than corrections to this diff: a doctor --deep flag that actually runs the isolated build is a new CLI capability, and a real end-to-end build in CI is a new job. This drive is scoped to making the author's existing change pass CI and review without widening it. The Manual-verification section now says outright that the isolated build path is not exercised end-to-end in CI and that a first real launch is where it gets live proof, so the gap is declared rather than implied. Worth doing as a follow-up; flagging for @coozgan and the maintainer rather than filing an issue that presumes which of the two remedies is wanted.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]

Answering the advisory CONCERNS on head 7f3c7b43. The three Watch items are fixed in the description; the three Subtractions are answered individually below.

  • fixed — description/diff gap: the body described the staging-lock snapshot design the diff replaced

Verified against the code rather than taken on report: _staging_lock appears nowhere in the shipped _inject_dist, and _build_archived_dist runs npm ci --ignore-scripts + npm run build per launch. Your framing of the risk is the reason this mattered most — "reviewers approving from the description approve the wrong mechanism." The body now leads with the provenance rationale and states that the checkout's static/dist / website/dist are never read.

  • fixed — phantom claim: python3.12/python3.11 dnf preference is not in the diff

Correct, and confirmed by re-reading the hunks: kirocrew-ec2.yaml contains only the health-check grep and a timeout comment. No hunk was missing, so the claim was removed rather than the code added.

  • fixed — "cuts launch time" is unsubstantiated

Also correct, and this one survived my first rewrite until you named it. The on-box build is removed but an equivalent cold npm ci of the same module graph is added on the laptop, so the wall-clock claim was not supported. The body now states the trade explicitly as reliability, not speed, and says outright that launch time is not claimed to improve.

  • rebutted — subtraction: make allow_dist_under a boolean instead of Optional[str]

The observation is accurate (grep confirms one non-test caller, always _DIST_PREFIX) and as a matter of API shape you are right. I am declining it here on proportionality, not correctness: _exclude_filter is the security fence that decides what may enter the tarball, this file has been the subject of eleven prior blocking review rounds, and the change buys no behavioral improvement — it trades a real (if small) risk of perturbing that fence for a cosmetic signature simplification. The exact-prefix string form also fails safe in a way a boolean does not: it names which component is exempt at the call site, so a future second caller cannot silently inherit a different exemption. Fair follow-up once this lands; not worth a signature change to the admission fence in this PR.

  • rebutted — subtraction: drop the npm parameter of frontend.build_stock_frontend

The count is right (no production caller passes it) but the parameter is a deliberate test seam, not dead generality: it is what lets the build tests inject a fake npm without monkeypatching shutil.which globally. Removing it does not reduce real surface — it relocates the injection into the tests as patching, which is the more fragile of the two. build_stock_frontend already resolves via shutil.which when the argument is omitted, which is exactly what every production call does.

  • needs-a-decision — subtraction: defer the two troubleshooting entries unrelated to the dist mechanism

Your scope point is legitimate: the pip-flake and WaitCondition-timeout entries are riders in a fix-typed PR and do not describe this mechanism. I am not deleting them unilaterally, because they are the author's own documentation of real failures they hit during this work, and removing them destroys information that is useful even though it is off-topic here. This is a maintainer/author call rather than a defect: @coozgan and the maintainer should say whether they ride along or move to a docs PR. Flagging it here rather than filing an issue, since the question is which of two acceptable outcomes is wanted and only they can answer it.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]

Round 13 — head 0b2087b6 (was 021c9ddf), rebased onto current main 9dbdcd2d. One finding fixed, two rebutted.

  • fixed — data URI masking permits raw AWS secrets (src/kiro_crew/cloud/source.py:500)

Correct, and this one was my own regression from round 12, not a pre-existing defect — worth stating plainly. Round 12 masked data-URI payloads by POSITION, so writing a raw 40-char key where a payload goes (data:image/png;base64,wJalr…KEY) discarded the bare-secret warning and shipped it: data:image/png;base64, had become a laundering prefix. Fixed by keying the exemption on CONTENT instead: _mask_inlined_media masks a payload only when it base64-decodes to bytes carrying a recognized container magic number (PNG/JPEG/GIF/WEBP/WOFF/WOFF2/TTF/OTF/ICO/SVG). A genuine inlined asset has one; a bare key does not, so it stays visible to the scan and aborts. I did not take the literal prescription ("remove payload masking") because it re-breaks test_real_bundled_data_uri_does_not_false_positive — a real 231-char PNG run from this project's built editor CSS trips the generic heuristic, which is the false positive the exemption exists for. Content-keying satisfies the finding's intent (no raw secret can hide in a data URI) without reintroducing that abort. Pinned by test_bare_secret_parked_as_a_data_uri_payload_aborts (exact-40 and glued); mutation-verified — restoring position-keyed masking fails both.

  • rebutted — source-controlled build executes with laptop credentials (src/kiro_crew/frontend.py:382)

Same span, third consecutive round, same prescription: revert the local build. Evidence unchanged and still decisive: origin/main's own frontend.py documents at lines 85-86 that the runtime rebuild (POST /api/update, kirocrew update, gateway auto-apply) already shells npm run build in the SAME checkout with the same inherited environment — so env = dict(os.environ) is not a capability this PR introduces, it is main's existing behaviour, and main's path is reachable from an HTTP endpoint while this one requires the operator to run a launch. This PR is strictly narrower: npm ci --ignore-scripts means lifecycle scripts never execute, and the edition-composition vars are stripped. A package.json able to exfiltrate here already has execution on a host whose gateway builds from that very checkout. Per this repo's own stall discipline, three rounds of new angles on one span with an unchanged revert-the-design prescription is the point to stop patching and put it to a human: reverting the author's approach is a maintainer/product call, and this drive is scoped to making the author's existing change pass CI and review without altering its design.

  • rebutted — function-local from kiro_crew.cli_server import _probe_dashboard_health violates top-level-imports (src/kiro_crew/cli_cloud.py:126)

Standing rebuttal, 7th occurrence of span d7e442e5752d, verified empirically. This round's prescription escalated from "move it to file scope" to "remove the deferred import and probe integration" — i.e. delete the feature. I tested the earlier form: moving the import to file scope fails test_cli_lazy_imports.py with module-scope import of kiro_crew.cli loaded deferred module..., because kiro_crew.cli_server is an explicit entry in that ratchet's forbidden-at-import-time set (dashboard.state → vector_memory → numpy, issue #3504). Deleting the probe instead would remove the dashboard-health warning that stops a user opening a known-broken dashboard — a feature deletion, out of scope for a drive-to-green. cli.py defers every cli_server import identically, so the deferred form is the established idiom, not an exception. Advisory (FINDING), non-blocking.

Verification on 0b2087b6: 324 passed across the cloud/frontend/lazy-import/spawn-audit/security-posture suites; isort/flake8/mypy clean on touched modules; black gate passes with the baseline untouched.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]

Round 14 — head b123f25a (was 0b2087b6), rebased onto current main 61c89d45. One finding fixed, two rebutted.

  • fixed — npm spawn errors bypass the documented fallback (src/kiro_crew/cloud/source.py:649)

Legitimate, reachable, and a genuine crash-class defect — thank you for it. Verified in the code: _run_npm_step calls subprocess.Popen unguarded, and build_stock_frontend only reports a missing npm (it returns None after shutil.which fails). An npm that resolves but cannot be executed — non-executable file, broken symlink, unlinked between the check and the spawn, EPERM — raises OSError from the spawn, which passes _build_archived_dist untouched and hits _inject_dist's except BaseException: raise, terminating the launch with a traceback. That directly contradicts this feature's own guarantee that a failed build leaves the archive unchanged so the box builds with npm. Fixed exactly as prescribed: the spawn error is converted into _AbortInjection, so it takes the same original-archive fallback every other build failure already takes. Scoped deliberately to this PR's path rather than to the shared _run_npm_step, because that helper is also main's POST /api/update rebuild path and widening the blast radius there is not this PR's business. Pinned by test_unexecutable_npm_refuses_instead_of_raising, placed in TestBuildArchivedDist (the TestInjectDist class stubs _build_archived_dist out, so a test there could never reach the spawn). Mutation-verified: removing the guard fails the test with a raw PermissionError: [Errno 13] — the precise escape you described.

  • rebutted — cloud launch executes untrusted checkout code on the operator host (src/kiro_crew/cloud/source.py:595)

Fourth consecutive round on this one concern, third distinct line cited (frontend.py:398frontend.py:382 → now source.py:595), with the prescription unchanged every time: revert the local build. The evidence has not changed either, and it is decisive: origin/main's frontend.py states at lines 85-86 that the runtime rebuild (POST /api/update, kirocrew update, gateway auto-apply) already shells npm run build in the same checkout with the same inherited environment. So arbitrary-host-code-execution-from-package.json is main's existing behaviour, reachable today from an HTTP endpoint, whereas this path requires the operator to invoke a launch. This PR is strictly narrower: --ignore-scripts means lifecycle scripts never run, and edition vars are stripped. Per this repo's own stall discipline — ≥3 rounds of new angles on one span with an unchanged revert-the-design prescription — I am stopping the patch cycle here rather than spending a fifth round. Reverting the author's approach is a maintainer and author decision; this drive is scoped to making the existing change pass CI and review without altering its design. Maintainer ruling requested (see the summary comment following this one).

  • rebutted — function-local from kiro_crew.cli_server import _probe_dashboard_health violates top-level-imports (src/kiro_crew/cli_cloud.py:126)

Standing rebuttal, 8th occurrence of span d7e442e5752d, verified empirically rather than argued. I applied the module-scope prescription and ran the ratchet: test_cli_lazy_imports.py fails with module-scope import of kiro_crew.cli loaded deferred module..., because kiro_crew.cli_server is an explicit entry in that test's forbidden-at-import-time set (dashboard.state → vector_memory → numpy, issue #3504). The alternative — remove the probe — deletes the warning that stops a user opening a known-broken dashboard, i.e. a feature, which is out of scope for a drive-to-green. cli.py defers every cli_server import identically, so this is the established idiom. Advisory (FINDING), non-blocking either way.

Verification on b123f25a: 325 passed across the cloud/frontend/lazy-import/spawn-audit/security-posture suites; isort/flake8/mypy clean on touched modules; black gate passes with the baseline untouched.

@iamwhatever

Copy link
Copy Markdown
Collaborator

🤖 Kiro Crew [operator: zejiangg]

⚠️ Escalation — stopping the drive-to-green loop at head b123f25a. Two maintainer decisions needed.

All 67 of 69 checks pass. Design, Opus, First Principles and UX are all PASS on this head; 0 unresolved threads; Fork workflow-change guard clears. GPT 5.6 Review is the only red, and both of its blockers have hit a wall that more patching will not clear.

Blocker 1 — frontend.py local npm build (needs a design ruling)

Five consecutive rounds, three different cited lines (frontend.py:398:382source.py:595 → back to frontend.py:398), one unchanging prescription: revert the local build. I have rebutted it three times on the same evidence, which I still believe is correct:

origin/main's own frontend.py (lines 85-86) documents that the runtime rebuild — POST /api/update, kirocrew update, and the gateway auto-apply — already shells npm run build in the same checkout with the same inherited environment. So "a modified website/package.json executes with operator credentials" describes main's existing behaviour, reachable today from an HTTP endpoint. This PR's path is strictly narrower: npm ci --ignore-scripts (lifecycle scripts never run) and edition vars stripped.

I cannot resolve this by patching, because the only fix GPT accepts is reverting the PR's core mechanism — a design call belonging to the maintainer and @coozgan, and explicitly outside this drive's scope.

Blocker 2 — the credential-scan exemption (needs a product ruling)

This one is mine, and I want to be plain about it: three rounds running, each fix I made produced a new narrower laundering variant of the same exemption.

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:

  1. 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.sh completeness, connect probe, and doctor.
  2. 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.
  3. 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.

@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 #3355. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #2188: CONTINUE_DEVELOPMENT. Independent features in the same module; both can land, with only ordinary same-file rebase churn in the docs and cloud tests. Files: src/kiro_crew/cloud/templates/kirocrew-ec2.yaml. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #4095. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #2188: CONTINUE_DEVELOPMENT. Both edit the same ~15 lines of install.sh, so whichever lands second takes a textual conflict there; the two changes should be sequenced deliberately, and 4095's recovery ladder should sit inside the elif arm 2188 creates rather than be re-derived. Files: install.sh.
  • This PR is OVERLAPPING with PR #4913. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #2188: CONTINUE_DEVELOPMENT. Different mechanisms and different files; the only shared file is a benign-spawn allowlist with disjoint entries. Worth noting to whoever lands both that cloud-shipped bundles will not be freshness-verifiable unless the commit is threaded into the isolated build. Files: src/kiro_crew/cloud/source.py.

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>
@bolichen97

Copy link
Copy Markdown
Collaborator

Rebased onto main 6c24f116e by a maintainer as part of the 2026-09-08 open-PR audit (was 1851 commits behind, mergeable_state=dirty).

Conflicts resolved:

  • docs/system-specs/modules/cloud.md — module table: kept main's new connect.py is_launched_instance() sentence and your rewritten source.py row. Provisioning section: kept main's reboot-resilience and printable-ASCII paragraphs, with your "healthy = answers AND lacks the Dashboard HTML not found marker" definition folded into the opening sentence.

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 test_cloud_ec2.py::TestUserDataSize's 14336 ceiling. Applied the remedy that test prescribes — moved your 6-line explanation out of UserData into the template's "Bootstrap script rationale" block as a "Dashboard health" entry. Script logic is unchanged.

Gates run locally on changed files: black (baseline-clean), isort, flake8, and test_cloud_source.py test_cloud_cli.py test_cloud_ec2.py test_frontend_dist_resolve.py test_spawn_audit.py — 256 passed.

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.

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

Labels

drive-to-green PR claimed by drive-to-green pipeline fork Pull request from a fork (external contributor) needs-human PR flagged for human review by drive-to-green pipeline readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants