close
Skip to content

feat(cloud): durable launch jobs + owner-only API for EC2 crew setup - #2059

Merged
iamwhatever merged 1 commit into
mainfrom
feat/cloud-provisioning-api
Aug 10, 2026
Merged

feat(cloud): durable launch jobs + owner-only API for EC2 crew setup#2059
iamwhatever merged 1 commit into
mainfrom
feat/cloud-provisioning-api

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Problem

Creating a remote crew on EC2 is CLI-only, and the CLI drives an interactive
device login. The dashboard therefore had no way to offer it: a web request cannot
sit and wait while a human approves a code in a browser, and an HTTP handler that
blocks for the minutes a CloudFormation stack takes would be dead on arrival. So
the gateway could list remote crews but never create one.

Why it matters

The only path to a remote crew was "read a guide, run CLI commands, and get the AWS
prerequisites right on the first try". Everything needed already exists in cloud/
— it just had no second caller. Users who want a bigger machine for parallel
sub-agents drop out at the CLI step.

Fix (symptom → root cause → change)

Symptom: the dashboard can list crews but not create one.
Root cause: two properties of the CLI path are incompatible with a
request/response cycle — a multi-minute provision, and a human-in-the-loop device
code.
Change: move the long-running work off the request and make the device code a
piece of durable state the UI can read.

  • cloud/launch_job.py — a launch is a durable job persisted after every state
    transition, so progress survives navigating away. The device code is job state,
    not a blocking prompt.
  • cloud/launch_engine.pyRealLaunchEngine binds to the existing ec2.deploy /
    login.start_device_login / connect.register_instance. No new AWS logic; the
    engine is injected so tests never touch AWS.
  • dashboard/handlers_cloud.py — ten owner-only /api/cloud/* routes (rejecting
    Slack origin, app tokens, and non-POSIX hosts). Every error response carries a
    machine-readable code.
  • settings/RemoteCrewPanel.tsx — one Remote Crew page, two tabs: Your crews
    (manage, default) and Set up a new one (prerequisite checklist, size picker,
    launch progress with the device code).

Making a launched crew actually reachable and accounted for

Manual testing surfaced several ways a paid instance could end up outside the
dashboard's control. Each is fixed here:

  • Per-crew gateway port. The tunnel forces local_port == remote_port (the
    remote gateway only trusts Origins on its own configured port) and hard-fails when
    that port is busy. Registering every crew on the default 5476 produced a crew that
    could never be connected — the operator's own gateway usually owns 5476. The
    launcher now allocates a port (deterministically, skipping ports the registry
    already uses) and applies it to both ends: a new DashboardPort stack
    parameter sets KIROCREW_PORT in the systemd unit and the bootstrap health
    probe, and the same value is recorded as the crew's remote_port.
  • Source shipping degrades instead of failing. source.repo_root() fails closed
    when there is no checkout (it must never tar up site-packages), which made
    one-click setup impossible from a wheel/app install. A non-raising
    find_repo_root() now decides: checkout → ship local source; otherwise install by
    cloning the public repo, which is the fallback the template already carried.
  • Cancel rolls back. Cancellation is observed between steps and the crew is
    registered last, so a cancel after provisioning left a running, billing instance
    that never appeared in the crew list. It now deletes the stack it created, acking
    the cancelled state first and only reporting "Removed" once AWS confirms
    a delete that lands in DELETE_FAILED says so instead.
  • Registration failures are not silent. connect.register_instance is
    best-effort by contract (returns None), so a registry failure used to mark the
    launch done with no crew. It now fails the job and names the instance.
  • The device sign-in code is stored behind the sensitive-path floor. A job
    that is awaiting sign-in persists the device-login URL and code — a credential
    that completes the Kiro sign-in. It was written under <config_dir>/cloud/, which
    is NOT on the shared sensitive-path floor, so a prompt-injected same-UID agent
    could read and exfiltrate it via ordinary file tools (0600 perms do not help — the
    agent runs as the same user). Jobs now live under <config_dir>/run/cloud-launch-jobs/;
    run/ is on security._SENSITIVE_HOME_DIRS, so agent file tools refuse it, while
    the gateway's own writer opens the path directly and is unaffected.
  • The launch POST no longer does blocking disk I/O on the event loop. The
    POST /api/cloud/launch handler persisted the new job with a synchronous
    store.create() (mkdir + temp-write + os.replace) directly on the aiohttp
    loop, while its sibling list call right above it was already offloaded via
    _in_executor -- so on a slow disk the create could stall every other gateway
    request and the heartbeat behind it. create() now runs in the executor too
    (still under the single-launch lock). Regression: test_create_persists_off_the_event_loop.
  • The SSM-plugin install hint no longer uses a predictable /tmp path. The
    copy-pasteable install command we show for the local host downloaded the AWS
    package to a fixed /tmp/session-manager-plugin.{pkg,deb} and then ran
    sudo installer/sudo dpkg -i on it. On a shared host a local user could
    preplant or swap that path and have the privileged install execute their scripts
    as root. The command now downloads into a private mktemp -d (0700, owner-only)
    and removes it afterward; the rpm (dnf install <url>) and Homebrew branches
    never touched a temp file and are unchanged.
  • An over-long launch-job id can no longer 500 the API. _path() validated the
    id charset but not its length, so a charset-valid but over-long {id} on
    GET/cancel/signin /api/cloud/launch/{id} (e.g. 300 hex chars) reached
    Path.exists() and raised ENAMETOOLONG -> HTTP 500. The store now requires the
    exact generated shape (12 hex chars) before any filesystem access, so a bad id is
    a clean 404 (get() maps the ValueError to None -> the handler's not-found path).
  • The build failure is diagnosable even on the default clone-of-main launch. The
    retry/exposure above lives in install.sh, which travels with the CLONED source --
    so on the default KirocrewRef=main launch (no local checkout shipped) the crew
    runs main's non-fatal install.sh and the real npm error was lost. The template's
    dist-check (which travels with the launching gateway) now greps the setup log for
    the npm/vite/tsc error and folds it into the WaitCondition reason, so the failure
    names its cause regardless of which install.sh ran.
  • A transient frontend-build failure now retries, and a persistent one is diagnosable.
    install.sh treated a build failure as a non-fatal warning (exit 0), so the cloud
    bootstrap's retry never fired for it and the real npm/vite error was swallowed
    behind "legacy fallback". The bootstrap now sets KIROCREW_REQUIRE_FRONTEND=1,
    making install.sh exit non-zero on a failed build: the existing install retry
    re-runs it on the warm box (first-boot contention -- the common cause -- self-heals),
    and if it still fails the build-log tail is printed into the WaitCondition reason
    instead of a bare "build failed". Local CLI installs stay non-fatal.
  • A crew that never built its dashboard fails the launch instead of shipping dead.
    install.sh treats a frontend-build failure as a non-fatal warning (legacy
    fallback) -- right for a local CLI user, wrong for a cloud crew whose whole point
    is the remote dashboard. Without the built SPA the gateway serves a ~782-byte
    "not built" stub that still returns HTTP 200, so the bootstrap health probe
    (curl 127.0.0.1:/) passed and the stack reached CREATE_COMPLETE while the
    crew's pane could never load -- a green launch handing over a dead, billing box.
    The bootstrap now verifies src/kiro_crew/static/dist/index.html exists after
    install and fails the WaitCondition (folding the build log into the reason) when
    it does not, so a failed build rolls the stack back with a real error.
  • A failed provision rolls back its stack. ec2.deploy creates the stack and
    then blocks until healthy, so a transient failure after the stack exists (e.g. a
    post-create DescribeStacks error) marked the job FAILED while the instance kept
    running, unregistered and invisible. A STEP_PROVISION failure now best-effort
    tears the stack down (mirroring the cancel path) before recording the failure;
    scoped to that step so a later-step failure — where the crew is created and named
    for recovery — is never deleted out from under the user.
  • A failed or reaped launch surfaces after a restart. The setup card fell back
    only to jobs that still carried a sign-in prompt, so a launch that FAILED or was
    reaped on restart (no prompt) rendered nothing after a reload — hiding its error
    and the "check your crews, it may still be running" warning for a possibly-billing
    stack. It now falls back to the newest persisted job.
  • Owner-only means the owner, not any authenticated caller. The guard rejected
    Slack-origin and app tokens but then admitted any request carrying a user with an
    empty app — and a dashboard session token is also minted for every allowed Slack
    user
    (!dashboard), which is exactly that shape. A non-owner could therefore reach
    a control plane that creates, stops and terminates billable AWS resources on the
    owner's account. The guard now matches the configured owner via the shared
    is_owner_dashboard_request predicate (the same definition ask_question and the
    source-provider routes use); a single-owner setup with no owner configured is
    unaffected (the owner's own local token still matches).
  • An unconfirmable sign-in no longer strands the crew. A transient SSM failure
    while starting or resuming the device login used to propagate out of the
    sign-in handle, failing the job before registration and leaving a paid instance
    absent from the crew list. Both the start_device_login call in the handle's
    constructor and the daemon resume in wait() now degrade to "sign-in
    unconfirmed": the crew still registers, and the device code (when there is one) is
    preserved so the user can finish from the dashboard.
  • An unconfirmed sign-in is now finishable from the UI. The gateway keeps the
    device code alive when sign-in could not be confirmed (it clears it only once
    sign-in succeeds), but the card gated that block on status === 'awaiting_signin'
    and the remount fallback only adopted in-progress jobs — so the surviving code was
    unreachable the moment the job went terminal, making "finish it from the dashboard"
    a dead end. The card now shows the code (with an explicit "could not confirm"
    message) whenever a terminal job still carries a prompt.
  • Stop/Start report progress on the button you clicked. The busy key interpolated
    the whole {tag, coords} variables object, yielding stop:[object Object] — a key no
    row could match, so the label never changed for the duration of the request. It stayed
    invisible because the row still disables (that only tests !!busy). Start is also
    wired into the same computation, which it was missing entirely.
  • A restart terminalizes orphans rather than leaving a card that can never
    advance.
  • Delete shows it is working. The delete endpoint only requests the teardown
    (cleanup: "pending"); the registry row is dropped minutes later by the background
    watcher once AWS confirms DELETE_COMPLETE. The row now shows a Deleting…
    state and the crew list polls until it disappears, instead of reappearing
    unchanged so the click read as a no-op.

Set-up tab usability (found by manual testing)

  • Account inputs live inside the prerequisites card, above the rows they produce,
    and the card names the profile and region it probed — showing the verdict first and
    the inputs later inverted cause and effect.
  • Profile/region persist across navigation (usePersistedString); losing them meant
    the next probe silently tested the AWS CLI default profile.
  • Re-check reports progress via isFetching (with isLoading, a refetch of an
    already-populated query showed no feedback at all).
  • The session-manager-plugin install command is served by the backend, which
    knows the gateway host's OS (macOS pkg/brew, Linux deb/rpm) — a hardcoded
    Homebrew line was unusable on every Linux gateway.
  • Sizes re-laddered to 16 / 32 (default) / 64 GB, described by how many parallel
    sub-agents each supports, because the cap is CPU-bound (floor(vCPU × 0.8)). Note
    this makes --size balanced resolve to a larger, pricier instance; the tier keys
    are deliberately unchanged.

Tests

Backend (197): job lifecycle and durability; cancel-rollback including an
unconfirmed delete not being reported as removed; orphan reaping vs a freshly
created job; owner-only guards on all ten routes — including a non-owner allowed Slack
user
(not only an app token) being rejected; instance_id derived server-side
rather than trusted from the caller; the allocated gateway port reaching both the
stack and the registry, and skipping ports already in use; source-mode selection
with and without a checkout; the plugin install command per platform/arch; an
unconfirmable sign-in returning "not signed in" instead of raising — from both
the daemon resume and a failure starting the device login; the
error-code contract ratchet; and a size-table parity gate asserting the panel's
SIZE_TIERS/X86_TIERS facts match cloud/sizes.py tier-for-tier, so the two
sources of truth can no longer drift silently.

Frontend (19 panel tests): cloud vs hand-added vs unidentifiable crews and which
destructive action each may offer; the device code surviving a remount; the crew list
refreshing when a launch finishes; the profile surviving a remount and driving the
first probe; Re-check showing progress; the server-supplied install command
rendering; size-card interpolation; a delete showing a Deleting… state once the
request is accepted; and copy guards that the progress card promises
only navigation persistence and that inputs precede their checks.

Manual verification

Found via live debugging of a launched crew (kc-90a9df): the box was healthy and
served HTTP 200, but the journal showed "Dashboard dist/ not found" -- the
public-clone launch had not built the SPA, so the pane timed out. End-to-end
validation is a fresh cloud launch from a gateway running this branch: the crew now
either comes up with a working dashboard, or fails the launch with the explicit
"dashboard frontend build missing" error and rolls back.

Built and run from the feature worktree against a real AWS account (116101834266).
The prerequisite checklist correctly reported expired credentials, went green after
aws login, and correctly stayed red for a missing session-manager-plugin. That
session is what surfaced the input ordering, the profile-persistence trap, the
invisible Re-check, the macOS-only install command, and the source-shipping failure
— all fixed here. cfn-lint passes on the template.

Screenshots

Your crews

AWS prerequisites — account inputs above the checks they produce

Device-code sign-in

Launch flow (10.7s)

Full launch sequence

03-setup-sizes.png is omitted: it predates the input reordering and would
misrepresent the current layout. It needs a fresh capture.

Progress running

Progress survives navigation

Notes for reviewers

  • config/infra signal: the flagged file is
    cloud/templates/kirocrew-ec2.yaml — a new DashboardPort parameter (default
    5476, so a direct deploy is unchanged) wired into the systemd unit's
    KIROCREW_PORT and the bootstrap health probe.
  • Which code a crew installs. With a checkout it installs your source; from an
    app/wheel install it clones the public repo at main. That means a DMG user's crew
    tracks upstream main, not the version inside their app — deliberate for now.
    Selectable stable/nightly/own-S3 channels are a follow-up.
  • feat(dashboard): one-page Remote Crew settings with guided EC2 setup #2060 was squash-merged into this branch rather than into main, so this single
    PR carries the whole feature (backend + frontend + evidence).

@iamwhatever
iamwhatever requested a review from a team as a code owner August 7, 2026 18:01
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of f35e099c2e6967062080e93b4b8ad58b04644238 — updated in place on each push; does not block merge.

I have enough to render a verdict. The design is thorough — durable job store, injected engine, owner-only routes, extensive rollback paths. The remaining risks cluster around billing-visible edges.

Design-Verdict: CONCERNS

Sound durable-job design; the residual risks are billing-shaped: a restart-orphaned stack has no dashboard recovery path, and stable size keys silently repriced.

Watch

  • Restart mid-provision strands a paid stack with no dashboard recovery. reap_orphans() terminalizes the job with "check your crews — it may still be running", but registration is the last step (STEP_CONNECT), so a stack orphaned before it never appears in the crew list and the reap does no reconcile — the one "paid instance outside the dashboard's control" case this PR's own cancel/failure rollbacks exist to prevent is recreated by a gateway restart, recoverable only via CLI/console.
  • --size balanced (the default) now provisions a ~2.4× pricier instance (t4g.xlarge → m7g.2xlarge) under an unchanged key, so an existing saved config or scripted --size balanced silently costs more per hour. A new key (e.g. development) with balanced aliased or deprecated would make the reprice opt-in.
  • Wheel/app launches clone public main unpinned ("tracks upstream main, not the version inside their app — deliberate for now"): any breakage on upstream main bricks every new launch for non-git users, and gateway↔crew version skew is unversioned and untested until the channels follow-up lands.

Suggestions

  • On reap of a tagged orphan, best-effort describe the stack and either register a CREATE_COMPLETE crew or surface a delete action on the failed card — closing the one gap in the otherwise-complete "no invisible billing" story.

[DESIGN-REVIEWED] f35e099

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f35e099

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

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

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

The change is well-scoped and heavily tested. I verified the security- and correctness-sensitive paths:

  • run/ is genuinely on security._SENSITIVE_HOME_DIRS (security.py:4331), so the device-code job files are behind the read+write floor as claimed; the store's own writer opens paths directly and is unaffected.
  • The owner-only guard (handlers_cloud._guard) rejects Slack origin → unauthenticated (401) → non-owner via the shared is_owner_dashboard_request predicate → Windows (400), in deny-by-default order; the app-token and allowed-Slack-user bypasses are correctly closed.
  • Job-id validation (_path) bounds both charset and exact length before any filesystem access, so a malformed {id} is a clean 404, not an ENAMETOOLONG 500.
  • All blocking store calls (create, list, get, reap_orphans) run via _in_executor, honoring the no-blocking-on-event-loop rule; the single-launch lock spans check→create→start.
  • RealLaunchEngine memoizes the allocated port across provision/register, so DashboardPort (systemd KIROCREW_PORT, health probe) and the recorded remote_port agree; base 7778 avoids the operator's 5476.
  • Cancel/failure rollback is scoped to STEP_PROVISION only (post-provision failures keep the named crew), teardown reports "Removed" only on confirmed deletion, and register fails loudly on a None return.
  • find_repo_root is the non-raising split of repo_root; the wheel-install path degrades to the public-clone fallback rather than raising mid-provision.
  • Referenced signatures (register_instance(remote_port=), ec2.deploy(dashboard_port=), wait_until_logged_in(attempts=), atomic_write(mode=)) all match; the new dashboard_port override is only appended when non-zero, so direct deploys keep the template default.
  • The size re-ladder retires 8 GB while preserving the light/balanced/power keys, and the UI-parity test guards drift between sizes.py and the frontend tables.

[OPUS-REVIEWED] f35e099

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

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

@iamwhatever
iamwhatever force-pushed the feat/cloud-provisioning-api branch from 1493c63 to d4c8cf5 Compare August 7, 2026 20:09
@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 7, 2026
@iamwhatever
iamwhatever force-pushed the feat/cloud-provisioning-api branch from d4c8cf5 to 56ca7c3 Compare August 7, 2026 21:16
@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 7, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Dispositions for the two blocking findings on d4c8cf5b9 — both fixed in 56ca7c365.

1. handlers_cloud.py:66 — app tokens bypass the owner-only guard — FIXED.
Confirmed legitimate before fixing: token_auth.py:1396-1397 sets request["user"]
and request["app"] together, so a user check alone admits an app token, and
app_token_path_allowed() will grant /api/cloud/* to an app whose manifest declares
it. These routes create, stop and terminate billable EC2, so that is the wrong caller
class entirely. _guard() now rejects any non-empty request.get("app") with
403 cloud_owner_only — outright, not via path scope, because "owner-only" here means
the human at the dashboard: the deliberate click is the consent, and an app acting
unattended has none. Covered by test_app_token_rejected.

2. handlers_cloud.py:96 — orphan reaping blocks the event loop — FIXED.
Legitimate, and self-inflicted by the previous round's restart fix. Split the accessor:
_store() now only constructs (no disk), and a new async _astore() awaits
reap_orphans() through _in_executor, setting the once-per-process flag before
awaiting so a burst of concurrent requests triggers exactly one reap. Handler-side
store reads and writes are offloaded too — get, save and list in the launch
list / get / create / cancel / signin paths all go through the executor now. The only
remaining synchronous store use is inside _start_worker, which already runs on its
own thread.

Two follow-on test corrections worth flagging, since they came out of the fix rather
than the review: the reaper legitimately terminalized jobs that tests had hand-written
as running without ever claiming them, which is exactly what it should do to a job no
worker owns. Those fixtures now call store.adopt(...) the way _start_worker does,
and the cancel test split in two — one asserting that a restart-orphaned job is already
terminal by the time cancel runs, one covering the backstop branch where a non-terminal
unowned job is reached after the reap has run.

Local gates on 56ca7c365: 51 cloud/contract tests, flake8, isort, mypy and the
brand gate all clean.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Dispositions for the advisory Design Review — 🟡 CONCERNS on this PR. Taking them in
order, since two are now fixed and one is a deliberate product call.

1. "Durable-on-disk but not durable-in-control" — FIXED, and the claim corrected.
This was the sharpest observation on the PR and it was right: nothing re-adopted a
non-terminal job at startup, so a restart mid-provision left a job running forever
while CloudFormation finished independently — a zombie progress card, and a
post-restart cancel that found ev is None, changed nothing, audited "success" and
returned 200. LaunchJobStore.reap_orphans() now terminalizes non-terminal jobs that no
worker in this process owns, ownership is tracked via adopt() so a live process never
reaps its own in-flight work, and cancel terminalizes rather than signalling a thread
that no longer exists. The reap runs off the event loop.

I also removed the "survives a gateway restart" claim from the PR body and from
docs/system-specs/modules/instances.md, because with terminalize-on-load a launch
genuinely does not resume — the honest contract is "survives navigating away and a
reload; a restart terminalizes with an interrupted status". The failure message points
the user at their crew list precisely because, as you noted, the stack may well exist in
AWS. Resuming the CloudFormation poll is the better end state but a materially larger
change; it is not in this PR.

2. No already-running guard on POST /api/cloud/launch — FIXED.
409 launch_already_running, returning the in-flight job so a client can surface it
instead of guessing. Two jobs meant two tags, two stacks and two billed instances with
no server-side backstop, which is not something a client-side disable can be trusted to
prevent. Covered by two tests (refused while in flight, allowed once terminal).

3. sizes.py re-binds stable keys to costlier shapes — ACCEPTED, deliberate.
Not fixed, and I want to be explicit rather than quietly disagree. The re-ladder is the
point of the change: the sub-agent cap is CPU-bound (floor(vCPU × 0.8), clamped ≥3),
so the retired 8 GB / 2 vCPU shape could not host the parallel sub-agents that are the
reason to run a remote crew at all — it was a tier that looked available and did not
work. Keeping light/balanced/power was chosen over new keys so that existing
--size values and saved configs keep resolving instead of erroring; the cost of that
choice is exactly what you identified, a silent ~2.4× on balanced.

Your framing of the tradeoff is fair and I am not rebutting the arithmetic. What this PR
does about it: the UI in the follow-up PR states plainly that the instance is billed to
the user's account and links the AWS Pricing Calculator rather than printing a figure
that would be wrong for their region and discounts. A release note for CLI users is the
gap that remains, and it belongs with the release rather than in this diff.

@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 7, 2026
@iamwhatever
iamwhatever force-pushed the feat/cloud-provisioning-api branch from 56ca7c3 to 0cb2d2a Compare August 7, 2026 22:08
@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 7, 2026
@iamwhatever
iamwhatever force-pushed the feat/cloud-provisioning-api branch from 0cb2d2a to c1f3e5b Compare August 7, 2026 22:39
@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 7, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Round 5 dispositions for 0cb2d2a83 → now c1f3e5baf.

SAST (Semgrep) insecure-file-permissions on os.chmod(self._root, 0o700) — REBUTTED, suppressed in place with the reasoning inline.
The rule calls 0o700 "widely permissive" and recommends 0o644. That is backwards for a
directory: 0o644 would drop owner-execute — making the directory untraversable — and
add world-read to the folder holding a short-lived device code. 0o700 is the restrictive
mode, and it exists in this diff only because the previous round's GPT finding (correctly)
asked for owner-only permissions on exactly this path. Fixing one gate by satisfying the
other literally would have re-opened the vulnerability the earlier round closed.

Suppressed with a rule-scoped # nosemgrep on the flagged line plus the justification
above it in the source, following the repo's existing convention
(mcp_core.py:2819, _process_group_supervisor.py:195). Nothing was weakened.

Backend Tests (Windows) shard 1 — FIXED. Mine, and a real portability defect in the
tests rather than the code: the two new permission assertions encode POSIX semantics, and
Windows reported 0o666 / 0o777 because it does not enforce mode bits. The class is now
skipif(not platform_compat.IS_POSIX) with the reason stated — honest, because the feature
is POSIX-only by design (handlers_cloud._guard rejects win32 with posix_host_required),
so there is nothing to assert on Windows.

Also replaced my hand-rolled atomic write with the repo's atomic_write(..., mode=0o600).
Not a review finding — I noticed while fixing the above that the helper already does exactly
what I had written by hand (mkstemp + cleanup + os.replace), and its replace path carries a
Windows sharing-violation retry that my version lacked. Less code, repo convention, and
better on the platform that just failed.

GPT 5.6 Review — "review incomplete", no verdict produced for 0cb2d2a83. Not a finding
and nothing to fix; it needs a re-run against the new SHA, which this push triggers.

Local gates on c1f3e5baf: 55 tests, flake8, isort, mypy and the brand gate all clean.

@github-actions github-actions Bot removed the readiness: checking Automated validation is still running label Aug 7, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Round 11 dispositions for 82402925b → now 6f3c8e149. Both findings validated in code; both
addressed — the second by correcting a false claim rather than changing behaviour.

1. handlers_cloud.py:303 — stale cancellation can overwrite a completed job — VALID, FIXED as
prescribed.
Confirmed the ordering: the job snapshot is read across an await, and a worker that
finishes in that gap saves its result and then pops its cancel event in finally. So reaching the
"no event" branch does not prove the job is still active, and the branch wrote the stale snapshot
back with status = cancelled.

One consequence worth adding, because it makes this more than a mislabelled row: if the snapshot was
taken before provisioning finished it carries no instance_id, and the dashboard builds its
instance→tag map from launch jobs. Overwriting with that snapshot therefore erases the only link
between a live crew and its stack, so the crew renders as unidentified — which is precisely the
row that PR #2060's companion finding shows offers a one-click unregister. A cancel racing a
successful launch could hand the user a billing instance the dashboard no longer knows how to delete.

Fixed as suggested: re-read the job after finding no event and terminalize only that fresh snapshot.
Covered by test_cancel_after_a_restart_reports_a_terminal_job (unchanged) plus the existing
no-worker backstop test; the race window itself is now unreachable rather than merely narrower.

2. launch_engine.py:82 — cancelling during ec2.deploy leaves the job provisioning — VALID as a
documentation defect. Fixed by correcting the documentation, not by wiring cancellation through.

The behaviour report is accurate: ec2.deploy blocks until the stack settles, so a cancel during
provisioning is not observed until it returns. What is actually wrong is that my own text promised
otherwise
launch_job.py said cancellation is honored "between/inside steps" and
instances.md said "between and inside steps". True of the sign-in wait (~5s granularity), not of
provisioning. Both now state the real contract.

I did not pass the cancel signal into provision to terminate the deploy via proc_sink. Killing
the local AWS CLI does not stop CloudFormation — the stack keeps building server-side — so on its own
that would make the job look cancelled while the stack survived, reintroducing the abandoned-stack
leak fixed in round 9. The leak is already closed the other way: a cancel that lands after
provisioning rolls the stack back via LaunchEngine.teardown. So the remaining gap is
responsiveness (the user waits out the deploy), not correctness or cost — and the honest fix for a
docs-vs-behaviour mismatch is to correct the docs. Happy to add true in-step cancellation as a
follow-up if you want the responsiveness, but it needs to be proc_sink plus the rollback, not
proc_sink alone.

Local gates on 6f3c8e149: 68 tests, flake8, mypy clean.

Note on the brand gate: my local run flagged code_review_sage/sage_lib/pipeline.py:43, which this
change does not touch (git diff BASE...HEAD lists 0 matching files) — stale local base resolution,
and the gate is green in CI. Flagging it only so the line is not mistaken for mine.

@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
@iamwhatever
iamwhatever force-pushed the feat/cloud-provisioning-api branch from 6f3c8e1 to fb94d7a Compare August 8, 2026 06:21
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 8, 2026
@iamwhatever
iamwhatever enabled auto-merge (squash) August 8, 2026 07:57
@iamwhatever
iamwhatever requested a review from a team August 8, 2026 08:00
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

Advisory UX-level review of f35e099c2e6967062080e93b4b8ad58b04644238 — updated in place on each push; does not block merge.

I have everything I need — diff, strings, screenshots, and sibling code. Composing the review.

UX-Verdict: CONCERNS

Solid flow overall, but the sign-in recovery button lies about its action, and the armed "Confirm delete" never disarms — both on billable paths.

Watch

  • "Open sign-in page" doesn't open a page when no code exists. In LaunchProgressCard the no-signin branch wires that label to onSignin(job.id) — a POST that only fetches the code — and signinMutation.isPending is never passed, so the click shows nothing until the poll repaints. This is the sole recovery path for an unconfirmed sign-in (rare × task-blocking × every occurrence). Relabel "Get sign-in code" and show a pending state.
  • "Confirm delete" stays armed forever. confirmDeleteTag is only cleared on mutate or re-target; switch tabs and return minutes later and the row's danger button still terminates the instance in one click (rare × destructive-irreversible). Auto-disarm on timeout, blur, or tab change.
  • Stop and Start are both always shown with no power-state indicator (CrewRow renders both unconditionally), next to Disconnect — a newcomer can't tell Stop (EC2, stops billing CPU) from Disconnect (tunnel), nor which of Stop/Start currently applies (frequent × friction). Show the one that applies, or surface the instance's power state on the row.
  • The done-state badge reads "Connect" (Badge variant="ok" reusing instancesPanel.connect) — an action verb on a non-interactive status chip; users will click it. Use "Ready".

Suggestions

  • more_sizes: "Smaller and x86_64 sizes" — the disclosure contains no smaller sizes (x86 lane is the same 16/32/64 GB); label it "x86_64 sizes".
  • Add a copy button to your_code in the sign-in block — the device code must be retyped into another window otherwise.
  • The "Your crews" tab count (instances.length) excludes Setting-up rows, so screenshot 06 shows "2" over four rows; include in-progress launches.

[UX-REVIEWED] f35e099

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Dispositions for the findings raised across 45bfabc4bdcd652c58, now all on
4da791a5e (rebased onto current main). Every one was validated against the code
before being touched.

1. launch_engine.py:166 — cancellation records rollback before deletion completes
— VALID, FIXED, with a deliberate deviation.
Correct: destroy(..., wait=False)
only means the request was accepted, yet the job unconditionally wrote
"Removed {tag} after cancellation." A stack reaching DELETE_FAILED therefore left
the user believing billing had stopped.

I did not take "wait for deletion and raise" literally, because waiting before
persisting would leave the card showing running for the minutes a CFN delete takes
— the user clicked Cancel and would see nothing change. Instead it is ack-then-confirm
(the same shape this PR already uses for the destroy route): persist cancelled +
"Removing …" immediately, then confirm. teardown now returns bool, and three
outcomes are distinguishable where there was one — confirmed removed / request raised
/ requested-but-unconfirmed. The last names the stack and says it may still be
billing. Covered by test_an_unconfirmed_delete_is_not_reported_as_removed.

2. launch_engine.py:97 — runtime import violates the top-level-imports rule —
VALID, FIXED.
Agreed, and I had already complied with that rule elsewhere in this PR
before reintroducing a lazy import here. Checked for a cycle first: registry.py
imports only atomic_write + config.loader, and instances.port_allocator was
already imported at module scope, so it moves up safely. The try/except stays around
the registry read, which is the part that can genuinely fail.

Moving it broke two of my own tests, which is worth recording: they patched
sys.modules["kiro_crew.instances.registry"], a no-op once the name is bound at
module scope. They now patch le.InstancesRegistry where it is actually looked up.

3. launch_engine.py:48 — sign-in resume errors strand provisioned crews — VALID,
FIXED, and widened.
Confirmed the chain: resume_login_daemon sat outside the
try: … except AWSError: return False wrapping the poll loop, so a transient SSM
failure propagated out of wait(), hit run_launch's outer except Exception, and
returned before STEP_CONNECT — leaving a provisioned, billing instance that was
never registered and so never appeared in the crew list. That handler exists precisely
to treat an unconfirmable sign-in as "not signed in"; the call was simply on the wrong
side of it.

Widened beyond the prescribed fix: except AWSErrorexcept Exception. These
helpers shell out to the AWS CLI, so an exec/sandbox failure arrives as an unrelated
type and would have escaped the handler and stranded the crew anyway — the same trap
that bit ec2.describe earlier in this PR. An exc_info log keeps the real cause
diagnosable rather than swallowed. Covered by two tests (AWSError and a non-AWS
RuntimeError both returning False).

Pattern worth flagging to a human reviewer: three separate findings on this PR have
now been the same shape — an error path that skips registration while AWS keeps
billing (cancel-abandons-stack, register-returns-None, signin-resume-raises). Each is
fixed individually. The durable fix, if a fourth appears, is to register the instance
immediately after provisioning instead of as the final step; I have deliberately NOT
done that here because it changes what "registered" means mid-launch and deserves its
own change.

Local gates on 4da791a5e: 190 backend tests, flake8 / isort / mypy / brand clean,
cfn-lint clean; frontend tsc -b, eslint (under the 1116 warning cap), jscpd, all
13 i18n:check gates, 39 test files / 615 tests.

Disclosure about the local review gate: this session has no spawn_run facility,
so the two model-pinned local reviewers (gpt-5.6-sol / claude-opus-5 mirrors) did not
run. The above is a prompt-driven self-review against each contract plus the full
deterministic gate set — weaker than the real local gate, so the server lanes are the
authoritative check on this SHA.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Answering both advisory CONCERNS verdicts on dcd652c58, now addressed on
fe2e5ca9f. One disposition per concern.

UX Review 🟡 CONCERNS

1. "Stop is a one-way door" — FIXED. Verified and correct: api.cloudStart
existed in the client (and was even mocked in the panel tests) but no call site
existed
— grep found cloudStop in RemoteCrewPanel.tsx and cloudStart only in
client.ts and the test mock. A stopped crew therefore had no dashboard path back to
running while its EBS volume kept billing. Added a Start action beside Stop on
cloud rows with a matching startMutation, plus two new i18n keys across all 13
catalogs. Covered by offers Start so Stop is not a one-way door, which asserts
api.cloudStart is actually invoked.

2. "Disconnect vs Stop side by side are indistinguishable" — ACCEPTED-AND-DEFERRED.
The observation is fair: one drops a tunnel, the other halts a billable instance, and
they read alike on a cold pass. I am not redesigning the row's action set in this PR —
distinguishing them properly means an affordance change (grouping, or a destructive
tint, or moving lifecycle actions into an overflow) that deserves its own mockup pass
rather than being improvised at the end of a large change. Worth noting the asymmetry
is now smaller, since Stop is at least reversible from the same row.

3. "The success badge says Connect" — ACCEPTED-AND-DEFERRED. Correct that
LaunchProgressCard reuses instancesPanel.connect as the done state chip, so an
imperative verb renders as a non-clickable status. Cosmetic, no functional
consequence, and it needs a new key rather than a reshuffle — folding it in now would
mean another 13-catalog edit for a label. Filed mentally with the same follow-up as
concern 5.

4. "'Smaller and x86_64 sizes' breaks its promise" — REBUTTED (copy is accurate),
with the wording conceded.
The disclosure genuinely contains no smaller tier because
the 8 GB / 2 vCPU tier was deliberately retired in this PR: the sub-agent cap is
CPU-bound (floor(vCPU × 0.8), clamped ≥3), so that tier could not host sub-agents at
all. The hint's own text already says "16 GB is the smallest size", so it does not
misstate the facts — but I agree the label "Smaller and x86_64 sizes" sets an
expectation the body then withdraws. Renaming the disclosure is a one-key change I did
not take here for the same reason as 3.

5. "Screenshot 03-setup-sizes.png is stale" — VALID, and I have removed it rather
than ship misleading evidence.
It shows the AWS profile/Region inputs inside the
"New cloud crew" card, which this PR moved into the prerequisites card — so it
contradicts both the code and the PR's own claim. I re-captured 02-setup-prereqs.png
against an isolated pod earlier but did not re-shoot 03. It is now out of the body; the
inline prereq shot shows the current layout. Re-capturing 03 needs another pod run and
is the one piece of evidence still owed.

Design Review 🟡 CONCERNS

6. "A restart recreates the same harm: reap_orphans marks the job failed but a
restart before STEP_CONNECT strands a billing instance" — VALID, ACCEPTED-AND-DEFERRED
with reasoning.
This is real and I want to be precise about what is and is not
covered. Fixed in this PR: cancel-after-provision (rolls back), registration returning
None (fails loudly), and an unconfirmable sign-in (registers anyway). NOT fixed: a
hard process death between provision and register. reap_orphans terminalizes the
job and points at the crew list, but the instance was never registered, so the crew
list cannot show it.

I am not fixing that here because the honest fix changes the launch's ordering
contract — register immediately after provisioning, then reconcile the rest — and that
alters what "registered" means mid-launch (a half-configured crew appearing as
connectable). That is a design change deserving its own PR, not an addition to a
41-file one. Three findings on this PR have now been this same shape, which is the
argument for doing it properly rather than patching a fourth instance.

7. "Reused tier keys silently reprice ~2.4×" — ACCEPTED, deliberately, and now
stated in the PR body.
Keeping light / balanced / power stable was an explicit
decision (the user chose it when the re-ladder was designed) so saved configs and
scripts keep working. The cost consequence is real and is called out in the Fix
section. A release note is the right vehicle for warning existing users; that is
follow-up, not a code change.

8. "Cloud-crew identity derives from correlating registry entries against launch-job
files — state nothing prunes" — REBUTTED in part, accepted in part.
The correlation
is deliberate and its failure mode is now safe: an SSM row with no matching launch job
is treated as possibly-cloud (honest attribution, confirm-gated Remove) rather than
as hand-added, precisely so a CLI-launched crew is never offered a one-click
unregister. So a missing job no longer causes harm. The unbounded growth of
launch-jobs/ is a fair observation and unaddressed — jobs are small JSON files and
nothing prunes them; a retention sweep belongs with the durable-teardown work in
concern 6.

Local gates on fe2e5ca9f: 190 backend tests; frontend tsc -b, eslint, jscpd, all
13 i18n:check gates, 39 files / 19 panel tests among 615 frontend tests.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Dispositions for fda0920dfa5bce3cb4 (also rebased onto current main, which
carries the ko-catalog fix — see the CI note at the end).

GPT 5.6 — RemoteCrewPanel.tsx:433

"An unconfirmed sign-in reaches done with its prompt preserved, but
status === 'awaiting_signin' hides it and the remount fallback excludes terminal
jobs" — VALID, FIXED. This is the best finding on the PR so far.

It caught an incoherence I introduced myself. An earlier round made the gateway treat
an unconfirmable sign-in as non-fatal: the crew still registers and job.signin is
deliberately preserved, with this comment in launch_job.py:

Keep the prompt when the wait ran out … the message below tells them to finish from
the dashboard — which is only possible if the dashboard still has the URL and code to
show. Clearing it here is what made "finish it from the dashboard" a dead end.

The backend then made exactly that promise, and the frontend broke it in two places —
both of which GPT named precisely:

  1. the sign-in block rendered only while status === 'awaiting_signin', so the code
    vanished the instant the job went terminal; and
  2. effectiveLaunchId fell back only to inProgress[0], so a terminal-but-unconfirmed
    job was never adopted after a remount — the card did not render at all, meaning
    fixing only (1) would have left the bug reachable.

Both are fixed. job.signin surviving on a terminal job is precisely the
unconfirmed signal (the gateway clears it on confirmed sign-in, and on reap), so the
card now renders the code for that case with a distinct "we could not confirm the
sign-in" line rather than the generic in-progress hint. Regression test: still shows the device code when a finished launch never confirmed sign-in — it drives the real
tab and would have failed on either half alone.

Design Review — four concerns

1. "Two sources of truth for size tiers" — VALID, FIXED, though not the way you'd
expect.
Correct, and your evidence is the strongest part: the re-ladder in this very
PR proves these numbers move. RemoteCrewPanel.tsx renders from literal
SIZE_TIERS/X86_TIERS while the gateway launches from sizes.py, so a one-sided
edit would show "32 GB · 8 vCPU" while provisioning something else, with nothing red.

I did not serve the catalog over HTTP. That adds a runtime API surface for data
that is compile-time constant, and it would not actually collapse the duplication —
the tier copy is translated and must stay in the frontend catalogs either way. What
the concern is really about is drift, so I made drift unmergeable instead:
test/test_cloud_sizes_ui_parity.py parses the panel's literal tables and asserts
instance type / vCPU / RAM / disk / arch match sizes.py tier-for-tier, with a
guard-the-guard test so a shape change that defeats the parser fails loudly rather
than passing vacuously. Same pattern this repo already uses for i18n catalog parity.
7 tests, one per tier plus the parse guard.

2. "Existing --size keys silently got pricier (~2.4×)" — ACCEPTED, deliberately.
Keeping light/balanced/power stable was an explicit product decision so saved
configs and scripts keep working; the alternative (new keys) breaks every existing
invocation to avoid a surprise. The repricing is called out in the PR body's Fix
section. A release note is the right vehicle for warning existing users and is
follow-up, not a code change.

3. "~3 MB of temp-screenshots/ binaries committed to the source tree" — REBUTTED
on the convention, with the underlying point conceded.
temp-screenshots/ is this
repo's own documented mechanism for PR evidence (temp-screenshots/README.md): a
deliberately top-level, periodically-pruned directory that exists because it sits
outside every packaged path, so review media never rides into the wheel, sdist, or
desktop DMG — which is what happens if screenshots go under docs/ or
src/kiro_crew/**. Commit-SHA-pinned raw URLs are the documented embedding recipe
precisely because branch-pinned URLs die when the branch is deleted on merge and
external hosts are camo-blocked for private repos. You are right that git history
keeps the blobs forever; that is a known, accepted cost of the convention, and
changing it is a repo-wide policy decision, not something to relitigate in this PR.

4. "Wheel/DMG installs clone public main, so a crew can run newer code than the
gateway tunneling to it" — ACCEPTED-AND-DEFERRED, and now explicit in the PR body.

Real and correctly reasoned: an Origin/API contract change on main would break crews
launched from an older app. It is deliberate for now (this is OSS and the alternative
is shipping no one-click path at all from an app install), and the PR body states
plainly that a DMG user's crew tracks upstream main rather than the version inside
their app. Selectable stable/nightly/own-S3 channels are the follow-up that closes it.

CI note

The two red checks on fda0920df had a single root cause that was not this PR:
Frontend Tests failed on catalogParity > ko (5 missing
components.sendToInstanceSubmenu.* keys that arrived with the session-teleport merge
on main), and Coverage Gate fails closed whenever frontend-test is not
success, so it was a cascade, not an independent failure. I verified the gap existed
on pristine origin/main and that this diff never references that key namespace. The
fix has since landed on main; this SHA is rebased onto it.

Local gates on a5bce3cb4: 197 backend tests, flake8 / isort / mypy / brand clean;
frontend tsc -b, eslint, jscpd, all 13 i18n:check gates, 20/20 panel tests and 597
frontend tests.

Disclosure: this session has no spawn_run facility, so the two model-pinned local
reviewers did not run — the above is a prompt-driven self-review against each contract
plus the full deterministic gate set. The server lanes remain the authoritative check
on this SHA.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Dispositions for the advisory verdicts on a5bce3cb4, now addressed on ad1076daf
(also rebased onto current main). One disposition per concern.

UX Review 🟡 CONCERNS

1. "Start gives zero feedback; the Stop key interpolates an object" — VALID, FIXED,
and the sharpest finding in this round.
Verified exactly as described:
`stop:${stopMutation.variables}` interpolated the whole {tag, coords} object,
producing the literal key stop:[object Object], which no row could ever match against
`stop:${cloudTag}`. So Stop's in-flight label never changed — and that bug
predates the Start button, it was simply never visible because the row still disables
(disabled={!!busy} only tests truthiness, so the disable worked while the label was
dead). startMutation was missing from the computation altogether, so you were right on
both halves. Now keyed on .variables?.tag with a startMutation branch, and covered by
shows progress on the button that was clicked, which asserts the clicked button reports
progress while the request is in flight and fails on the old key.

2. "Stop and Start render side-by-side with no instance-state indicator" —
ACCEPTED-AND-DEFERRED.
Fair, and I want to be precise about why I am not fixing it
here: the row genuinely cannot know EC2 machine state today. It renders from the
instances registry plus launch history, neither of which carries a
running/stopped/stopping field — surfacing real state means polling
ec2:DescribeInstances per cloud row on an interval, which is a new AWS call path with
its own cost, throttling and failure-mode questions. That is a feature, not a label fix.
Until then both actions stay available and idempotent-ish (starting a running instance
is a no-op at the API), which is the honest behaviour for "we don't know".

3. "Finished launch is badged 'Connect'" — ACCEPTED-AND-DEFERRED. Confirmed: the
done state chip reuses instancesPanel.connect, so an imperative verb renders as a
status. Cosmetic and non-functional; the fix is a new i18n key across 13 catalogs plus a
copy decision about what the terminal state should say ("Ready"? "Set up"?), which is
worth doing deliberately rather than as a drive-by at the end of a 42-file PR.

4. "'Smaller and x86_64 sizes' contains nothing smaller" — REBUTTED on the facts,
label conceded.
The disclosure contains no smaller tier because the 8 GB / 2 vCPU tier
was deliberately retired in this PR: the sub-agent cap is floor(vCPU × 0.8) clamped
≥ 3, so that shape could not host the sub-agents the tier is sold on. The hint's own body
already states "16 GB is the smallest size", so nothing in it is false — but I agree the
label promises something the body withdraws, and renaming it is the right fix. Grouped
with 3 as a copy pass rather than done piecemeal.

5. "Armed 'Confirm delete' has no way to back out" — ACCEPTED-AND-DEFERRED, and the
one I would fix next.
Correct: onRequestDelete arms the confirm state and only
deleting clears it. The mitigation today is that the armed state is per-row and visibly
distinct, and navigating away unmounts it — but "the only way out of a destructive prompt
is to complete it" is a genuine trap and I am not defending it, only sequencing it.

6. "Tab badge counts instances.length only, while the list also shows setting-up
rows" — VALID, ACCEPTED-AND-DEFERRED.
Reproduced in screenshot 06 (badge "2", four
rows). A one-line fix, but it changes what the badge means, and the honest version
probably distinguishes "crews" from "in progress" rather than summing them.

7. "unverified_cloud_note is jargon" — ACCEPTED-AND-DEFERRED. Your rewrite is
better than mine and says the consequence instead of the epistemics. Copy pass with 3
and 4.

Design Review 🟡 CONCERNS

8. "Serve the size ladder from GET /api/cloud/sizes instead of duplicating the
tables — this deletes the parity test rather than guarding the duplication" — REBUTTED,
with the disagreement stated plainly.
This is a real architectural preference and I
considered it before choosing the gate, so here is the reasoning rather than a deferral:

  • It does not delete the duplication, only relocates part of it. Tier copy is
    translated and must live in the 13 i18n catalogs regardless, so the frontend keeps a
    per-key table either way; the endpoint moves five numbers and leaves the naming,
    ordering and "recommended" presentation client-side.
  • It converts compile-time constants into a runtime dependency. The size picker would
    then have a loading state, an error state, and a failure mode (picker unusable because
    an owner-only API call failed) for data that changes only when someone edits
    sizes.py — while the parity gate's failure mode is a red CI check on the PR that
    caused it.
  • The i18n catalogs in this repo are guarded the same way — duplicated data, parity
    enforced by test — so the gate is the established pattern, not an ad-hoc dodge.

If a maintainer prefers the endpoint I will build it; I am not claiming the gate is
strictly better, only that it closes the drift risk you identified at materially lower
cost and risk, which is what the concern was about.

9. "Restart recovery contradicts the PR's own goal — a mid-provision restart strands a
billing stack" — ACCEPTED-AND-DEFERRED (third raise, standing answer, and I think you are
right that it is the most consequential item left).
To be exact about coverage: cancel
after provisioning, registration returning None, and an unconfirmable sign-in are all
fixed in this PR. What remains is a hard process death between provision and
registerreap_orphans terminalizes the job and says "check your crews", but the
instance was never registered, so the crew list cannot show it.

I am still not fixing it here, for one reason: the durable fix is to register immediately
after provisioning and reconcile afterwards, which changes what "registered" means
mid-launch (a half-configured crew becomes visible and apparently connectable) and
touches the ordering contract the whole job model is built on. Four findings across this
PR have now been this same shape, which is the argument for doing it as its own change
with its own tests, not as a late addition to a 42-file PR.

10. "light/balanced/power keys silently resolve to ~2× pricier shapes" —
ACCEPTED, deliberately, product decision.
balanced: t4g.xlarge → m7g.2xlarge is real
and is stated in the PR body. Keeping the keys stable was chosen so existing configs and
scripts keep resolving; renaming would break every existing invocation to avoid a
surprise. A release note is the right vehicle for warning existing users and is
follow-up, not a code change.

Summary of what is deliberately left unfixed

Items 2, 3, 5, 6, 7 are a UI/copy pass (state indicator, terminal-state badge,
cancellable confirm, badge semantics, note wording); items 9 and 10 are a durable
teardown change and a release note. None of them is disputed except 8 and the factual
half of 4. Nothing here is silently dropped.

Local gates on ad1076daf: 74 targeted backend tests plus the full cloud suite green,
brand clean; frontend tsc -b, eslint, jscpd, all 13 i18n:check gates, 39 test files /
618 tests including 21 panel tests.

Disclosure: this session has no spawn_run facility, so the two model-pinned local
reviewers did not run — the above is a prompt-driven self-review against each contract
plus the full deterministic gate set. The server lanes remain authoritative for this SHA.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Dispositions for the verdicts on ad1076dafaa833a494, plus a UX fix I
folded in from manual testing.

GPT 5.6 — launch_engine.py:37 — "Initial sign-in failures strand the provisioned crew" — VALID, FIXED. Correct, and it is the last hole in a family this PR has been closing all along.

An earlier round hardened _RealSigninHandle.wait() so a transient SSM failure while
resuming the login daemon degrades to "sign-in unconfirmed" instead of failing the
job before register(). GPT caught that the constructor still had the same defect
one call earlier: login.start_device_login(...) also shells out to SSM, so a transient
failure there raised straight out of begin_signin → the launch worker, failing the
job before registration and leaving a provisioned, billing instance absent from the
crew list — the exact stranding the wait() comment says it exists to prevent.

Fix: the constructor now catches that failure (broad, on purpose — an exec/sandbox
failure arrives as an unrelated type, same reasoning as wait()) and continues with an
empty, unconfirmed prompt, so the launch still reaches register() and the crew becomes
visible and deletable. close() tolerates the absent prompt. Regression test: test_a_failure_starting_device_login_does_not_strand_the_crew constructs the handle with start_device_login raising and asserts it does not raise and comes back empty/unconfirmed.

GPT 5.6 (advisory) — RemoteCrewPanel.tsx:719 — non-blocking, and it did not survive GPT's own falsification pass. activeJob = launchStatusQuery.data ?? inProgress[0] is backed by the terminal-unconfirmed handling added last round (still shows the device code when a finished launch never confirmed sign-in): a failed detail poll falls back to the list copy, and the card renders the code for a terminal job that still carries a prompt. Not changing it.

Delete UX — folded in (manual testing)

Deleting an installed crew looked like a no-op: after re-auth + Confirm, the row sat
there unchanged. Root cause is a frontend gap, not the delete itself — DELETE /api/cloud/{tag} only requests the teardown (cleanup: "pending"); the registry row
is dropped minutes later by the background watcher once AWS confirms DELETE_COMPLETE.
The panel refetched the list immediately (the row was still present) and then never
polled again, so the row never cleared until an unrelated refetch. Fix: the accepted
tag is marked, its row shows a disabled Deleting… state, and the instances list
polls (4s) until the row disappears — then polling stops. New i18n key
remoteCrewPanel.deleting across all 12 authored catalogs (+ regenerated pseudolocale).
Panel test: shows a Deleting… state after the delete is accepted.

Local gates on aa833a494: 33 launch-job + 42 cloud-handler/ui backend tests, flake8 /
isort / mypy clean on the engine; tsc -b, eslint, the full i18n:check chain, 597
i18n vitest + 22 panel tests green.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition for aa833a4948a35b7d40.

GPT 5.6 — handlers_cloud.py:69 — "Non-owner dashboard sessions pass the owner-only gate" — VALID, FIXED. Not too strict for a personal tool: this is a cross-principal escalation, not the owner acting on their own machine.

I checked whether it was a false positive first, and it is not. The gate rejected
Slack-origin (X-Session-Key: slack:) and app tokens, then admitted any request with a
non-empty user and empty app. But token_auth mints a dashboard session token
for every allowed Slack user via !dashboard, and that token has exactly that shape —
user set to their subject, app empty — and does not carry the slack: session
key, so neither existing check catches it. The sibling _deny_non_owner in
ask_question.py documents this same hole in its docstring almost verbatim; the cloud
guard simply hadn't adopted the fix. These routes run real AWS mutations
(launch/stop/start/destroy) billed to the owner's account, and the module's own
docstring already claims "owner-only, never reachable via Slack" — so the code did not
match its stated contract.

Why "personal tool" doesn't make it a non-issue: KiroCrew explicitly supports allowed
Slack users who are not the owner (is_owner vs is_allowed_user), and hands them
dashboard tokens. That is a different principal from the owner — the self-authorizing
"deliberate click on your own machine" argument covers the owner, not a third party who
can spend the owner's money or delete their crews.

Fix: the app-only check is replaced by is_owner_dashboard_request(request) — the one
shared owner definition (exact owner_id match, or a signed local bootstrap subject when
no owner is configured), already used by ask_question and the source-provider routes.
It subsumes the app-token case (non-empty app → not owner) and, crucially, does not
lock out a genuine single-owner setup: with no owner configured, the owner's own local
token (local-app/local-startup) still matches. The Slack-origin fast-reject and the
auth_required 401 for a truly unauthenticated caller are kept for their distinct codes.

Regression test: test_non_owner_dashboard_user_rejected poses as an allowed Slack
user's !dashboard token (user set, empty app, not the owner) and asserts 403
cloud_owner_only; the existing app-token, Slack-origin, unauthenticated and
POSIX-guard tests still hold. The test harness now mirrors production (string subject +
empty app + configured owner_id).

Local gates on 8a35b7d40: 36 cloud-handler tests (incl. the new one) + 33 launch-job
tests, flake8 / isort / mypy clean on the handler.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Dispositions for 5946343a811d0a4058.

GPT 5.6 — launch_job.py:225 — "Device-code jobs bypass the sensitive-path floor" — VALID, FIXED. Verified against the floor, not just accepted.

A job that is AWAITING_SIGNIN persists the device-login URL and code — a credential
that completes the Kiro sign-in on the new crew. The store rooted at
config_dir()/"cloud"/"launch-jobs", and "cloud" is not in
security._SENSITIVE_HOME_DIRS, whereas "run" is (I confirmed both in
security.py). The floor is the shared read gate for agent file tools, so a
prompt-injected, same-UID agent could read that job file and exfiltrate the code. The
existing 0600 perms do not help here — agent tools run as the same user; the
protection is the application-level path floor, not the mode bits.

Fix: the default store root is now config_dir()/"run"/"cloud-launch-jobs". run/ is
classified read+write sensitive, and the gateway's own writers (this store) open the
path directly and do not route through the gate, so persistence is unaffected. Updated
the class docstring and docs/system-specs/modules/instances.md. Regression test
test_default_store_root_is_on_the_sensitive_path_floor asserts
is_sensitive_path(store.root) is True for the default root and a job file under it —
so a future move off the floor fails CI.

GPT 5.6 (advisory) — RemoteCrewPanel.tsx effectiveLaunchId — "excludes reaped/failed jobs without prompts" — VALID, FIXED (advisory, folded in).

Correct: the remount fallback was ... ?? unconfirmed[0]?.id, where unconfirmed
required !!j.signin. A launch that FAILED or was reaped on restart has no prompt, so
after a reload its card — including the "check your crews, it may still be running"
warning for a possibly-billing stack — rendered nothing. Changed the fallback to the
newest persisted job (launches[0]?.id; the store returns jobs created_at-descending,
confirmed in launch_job.list()), which surfaces failed/reaped launches while still
covering the in-progress and unconfirmed-sign-in cases. The existing
still shows the device code when a finished launch never confirmed sign-in panel test
still passes (that job is launches[0]).

Local gates on 11d0a4058: 34 launch-job + 35 cloud-handler backend tests, flake8 /
mypy clean; tsc -b, eslint, 22 panel tests green.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition for 11d0a4058503ad59d8 (also rebased onto current main).

GPT 5.6 — launch_job.py:574 — "Provision failures can orphan a billing stack" — VALID, FIXED. This is the exact residual the earlier strand fixes predicted, on the one path still uncovered.

ec2.deploy creates the CloudFormation stack and then blocks until it is healthy, so a transient failure after the stack exists (the post-create DescribeStacks GPT names, or anything else raising inside provision) unwound to except Exception → status=FAILED with the instance running, never registered, and invisible to the crew list — the same orphaned-billing shape the cancel path already rolls back and the four prior strand fixes each closed on their own step.

Fix: in the failure handler, when the failed step is STEP_PROVISION and a tag exists, _rollback_failed_provision best-effort tears the stack down (via the existing engine.teardown, confirm-then-report, mirroring _rollback_cancelled_stack) before the job is saved FAILED. It augments the recorded error rather than replacing it, so the original cause stays visible, and it is safe when no stack was created (deleting an absent stack is a no-op). Deliberately scoped to STEP_PROVISION: a later-step failure means the crew IS created — register even raises a message naming the instance for manual recovery — so tearing it down there would delete a live crew out from under the user.

Tests: test_provision_failure_marks_failed now asserts teardown is called and the stack-removed note is appended; new test_a_failure_after_provisioning_does_not_tear_down_the_crew proves a register failure leaves STEP_PROVISION DONE and calls no teardown.

Local gates on 503ad59d8: 35 launch-job + 36 cloud-handler backend tests, flake8 / mypy clean; tsc -b green.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Round update on 503ad59d8 -> e263cc921 (found via live debugging, not a bot finding).

A launched crew (kc-90a9df) sat on "Pane failed to load" for hours. On-box: the gateway
was up and serving HTTP 200, but journalctl showed Dashboard dist/ not found -- the dashboard will show the 'not built' guidance page. Root cause: the public-git-clone
launch path installs the Python backend, but the frontend build did not produce
static/dist, and install.sh treats that as a non-fatal warning (legacy fallback).
The gateway then serves a ~782-byte stub that returns 200 -- so the bootstrap health probe
passed and the stack reached CREATE_COMPLETE with a dashboard that can never load.

Fix (cloud/templates/kirocrew-ec2.yaml): after install, verify
src/kiro_crew/static/dist/index.html exists and fail the WaitCondition otherwise,
folding the build output into the signal reason so the stack rolls back with a real error
instead of shipping a dead, billing crew. install.sh stays graceful for local CLI users
(unchanged); the strictness lives in the cloud bootstrap where the dashboard is the whole
point. Regression test test_bootstrap_verifies_dashboard_built_before_success mirrors
the existing kiro-cli guard.

End-to-end validation is a fresh cloud launch from a gateway running this branch (the
launch ships this template + install.sh): the crew now either comes up with a working
dashboard, or fails the launch with the explicit error and rolls back.

Local gates on e263cc921: 72 cloud-ec2 template tests (incl. the new guard) + 35
launch-job + 36 handler tests, flake8/mypy clean; tsc -b green. CloudFormation Lint runs
in CI.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Follow-up on the dashboard-build guard (surfaced by a real launch on this branch).

The guard worked: a fresh crew detected the missing SPA, failed the launch, and rolled the stack back so it stops billing -- no more silent dead crew. But it exposed two gaps the guard alone didn't close:

  1. No retry for the frontend build. install.sh treats a build failure as a non-fatal warning and exits 0, so the bootstrap's existing "retry install.sh once on the warm box" (the documented cure for first-boot contention) never fired for it.
  2. The real error was swallowed. The npm/vite failure was logged only as "legacy fallback", and the rollback destroyed the instance log -- the WaitCondition reason showed only install.sh's success banner, not why the build failed.

Fix: the cloud bootstrap now sets KIROCREW_REQUIRE_FRONTEND=1. install.sh honors it by making a failed/absent build fatal (exit non-zero) and dumping the build-log tail first. That (a) engages the existing retry so a transient first-boot failure self-heals on the warm box, and (b) puts the real npm/vite error into the failure reason if it persists. Local CLI installs are unaffected (the flag is unset there, so the build stays non-fatal with the legacy fallback). Regression test test_bootstrap_requires_the_frontend_build.

Local gates on 886a2f934: 144 cloud backend tests, bash -n install.sh, tsc -b green.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition for f63811fdd458f9f774.

GPT 5.6 — launch_job.py _path() — "Unbounded job IDs crash lookup" — VALID, FIXED.

Confirmed reachable: _path validated the id charset but not its length, and job ids are the {id} path param on GET/cancel/signin /api/cloud/launch/{id}. A charset-valid but over-long id (e.g. 300 hex chars) passed the guard, so self._root / f"{job_id}.json"Path.exists() raised ENAMETOOLONG (an OSError) → HTTP 500 instead of a clean not-found. My earlier get() retry made this slightly worse by calling path.exists() outside a try.

Fix: _path now requires the exact generated shapelen(job_id) == 12 hex chars (_JOB_ID_LEN, also used by _new_job_id) — before touching the filesystem. A malformed/over-long id raises ValueError, which get() already maps to None, so the cancel/signin/status handlers return their existing 404 (launch_job_not_found) rather than 500. No new AWS or handler code. Regression test: test_bad_job_id_cannot_escape_store now also asserts a 300-char id yields get()==None and _path raises.

Local gates on 458f9f774: 72 launch-job + cloud-handler tests, flake8 / mypy clean; tsc -b green.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition for 458f9f77474fe6a58d.

GPT 5.6 — ssm.py session_manager_plugin_install_command() — "predictable temporary package paths permit root-code substitution" — VALID, FIXED.

Confirmed the class: the copy-pasteable install hint we render for the local host downloaded the AWS package to a fixed /tmp/session-manager-plugin.{pkg,deb} and then ran sudo installer -pkg / sudo dpkg -i on it. In world-writable sticky /tmp, a local unprivileged user can preplant the path or win a TOCTOU swap between the curl and the sudo step, so the privileged install runs attacker-controlled package scripts as root — a local privilege escalation on a shared host (e.g. a shared dev desktop, which the remote-crew flow targets).

Not rebutted despite being owner-facing: our "a user's action on their own machine is self-authorizing" principle governs gating the owner's own action; it does not cover a different, unprivileged local principal hijacking root via a predictable path. Same distinction as the earlier owner-gate finding.

Fix: both affected branches (macOS .pkg, Debian .deb) now download into a private mktemp -d (0700, owner-only — no preplant, no sticky-dir swap) and rm -rf it afterward. The rpm branch (sudo dnf install -y <url>, no temp file) and the Homebrew branch are unaffected. Tests test_macos_without_brew_falls_back_to_aws_own_package and test_debian_family_gets_a_deb now assert mktemp -d is used and no /tmp/session-manager-plugin literal remains.

Local gates on 74fe6a58d: 97 cloud handler + ssm + launch-job tests, flake8 / mypy clean.

Creating a remote crew on EC2 was CLI-only, and the CLI drives an interactive
device login — so the gateway could list remote crews but never create one.

Backend: durable launch jobs (`cloud/launch_job.py`) that persist every state
transition to disk, so progress survives navigating away, plus ten owner-only
`/api/cloud/*` routes and a `RealLaunchEngine` that binds to the existing
`ec2` / `login` / `connect` modules rather than adding new AWS logic. A cancel
after provisioning rolls the stack back so a half-built crew cannot bill
silently, and a restart terminalizes orphaned jobs instead of leaving a card
that can never advance.

Frontend: one Remote Crew settings page with two tabs — "Your crews" (manage,
default) and "Set up a new one" (AWS prerequisite checklist + size picker +
launch progress with the device code). The account inputs sit inside the
prerequisites card, above the rows they produce, and the card names the profile
and region it probed.

Sizes are re-laddered to 16 / 32 (default) / 64 GB, described by the number of
parallel sub-agents each supports, because the cap is CPU-bound.
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition for 51ddfaeb3f35e099c2 (also rebased onto current main).

GPT 5.6 BLOCKING — handlers_cloud.py:289 — "launch persistence blocks the event loop" — VALID, FIXED.

Confirmed against the file's own convention, not just accepted. api_cloud_launch_create is an async aiohttp handler. Every other store touch in this module is deliberately pushed off the loop through _in_executor — including the list call on the line directly above the flagged one, and the reap in _astore, whose docstring spells out the exact hazard ("must not run on the event loop — the first cloud request after a restart would otherwise stall every other request and the heartbeat behind it"). The one exception was store.create(...), which runs mkdir + a temp-file write + os.replace synchronously on the loop. On a slow/contended disk that blocks the whole gateway (all requests + heartbeat) for the duration — the very failure mode the surrounding code already guards against. So this is the project's own stated standard applied consistently, not a speculative "slow FS" hypothetical.

Fix: create() now runs via _in_executor (using functools.partial, since the helper takes positional args and create is keyword-only), reusing the single store = await _astore(state) already fetched for the list check. It stays inside the _launch_lock so the check-create-start critical section (the double-submit / double-stack guard) is unchanged; the KeyError-for-unknown-size path still maps to 400. Regression test test_create_persists_off_the_event_loop records the thread create() runs on and asserts it is not the loop thread.

Local gates on f35e099c2: 171 cloud backend tests (handlers + launch-job + ec2 + ssm), flake8 / mypy clean.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants