feat: AWS Control - account portal + S3-backed cloud drive - #5517
Conversation
github-actions
Bot
added
readiness: checking
1df91fb to
4ecfc7d
Compare
github-actions
Bot
added
readiness: checking
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of I have enough for the verdict. The design is thorough — spec-first, reuses the deploy engine chokepoint, consent fails closed, two-call confirm, owner-pinned transfers. One genuine design gap survives the kill-filter: the storage-lifecycle story. Design-Verdict: CONCERNS Sound, spec-first design — but it creates an unboundedly growing billable resource with no reclamation path or retention, in an app selling cost transparency. Watch
Suggestions
[DESIGN-REVIEWED] 4c2cf69 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Most console actions fail in silence — upload, push, backup, restore, and share creation render no error state, on a surface whose whole premise is credentials that expire. Watch
Suggestions
[UX-REVIEWED] 4c2cf69 |
4ecfc7d to
52b4b03
Compare
github-actions
Bot
added
readiness: checking
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence is gathered; the counts I needed are verified (existing deploy profile control plane at First-Principles-Verdict: CONCERNS The portal quietly ships a second profile-registration surface the description never mentions, duplicating the deploy control plane that already lists and registers the same registry. What this change shipsIntent: give users one surface to see/repair their AWS accounts and a consented S3 drive/library/backup — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 4c2cf69 |
52b4b03 to
3fcbf05
Compare
github-actions
Bot
added
readiness: checking
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates fail the falsification bar at requirement (a) — a concrete input that occurs in practice. Candidate 1 ( Candidate 2 ( Neither survives; nothing in the diff neighboring these two files rises to the grounded bar on my own read. No findings. [OPUS-REVIEWED] 4c2cf69 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
3fcbf05 to
12da3e3
Compare
chenmingwei23
added
the
posix-only-approved
github-actions
Bot
removed
the
readiness: action required
Round 20 disposition - head a538fe1Two findings, plus two Windows test failures of my own making. One finding 1. Backup authorization temporaries remain agent-writable - real, and the The entry is now the whole DIRECTORY, Verified against the live classifier, not the list: 2. Parent-directory swaps escape backup and restore roots - real residual, What the code does have on that path: the walk refuses to descend into a Windows Backend Tests (1) - two failures, both mine, and the first exposed a
Gates: 73 aws-control backend tests, 42 security-posture tests, 637 security |
Round 21 disposition - head a58c838Three findings, all real, all fixed. The descriptor-pinned-traversal class did 1. GovCloud drives are never rediscovered (BLOCKING) - real, and the worst Discovery now anchors on 2. Download loses Safari user activation (FINDING) - real, fixed. 3. Restore is denied under the rendered policy (FINDING) - third raise Two self-inflicted items worth recording. The Italian style gate caught Gates: 78 aws-control backend tests, 42 security-posture tests, 748 tests across |
Round 22 disposition - head e54a154Three findings. Two fixed. The third is the fourth raise of the 1. Nightly uploads omit invocation outcome auditing - real, fixed, and the 2. App shutdown does not stop the active backup - real, fixed as far as the What is closeable is closed. Teardown now sets a The residual, stated plainly: an 3. Session backup can traverse a swapped directory symlink - fourth raise of The disposition stands: closing it means a traversal pinned on directory Backend Tests (Windows) - one failure, mine: the round-19 fixture used Gates: 82 aws-control backend tests, 42 security-posture tests, black / isort / |
Round 23 disposition - head 1f07cb6Two findings. One fixed, and it was a good catch. The other is the FIFTH raise of 1. Windows junctions escape the session backup tree - real, fixed, and the Pinned by making the predicate itself the thing under test rather than asserting 2. Drive uploads and backups bypass enterprise publish governance - fifth The disposition has not changed and this is the last time it is restated. The Gates: 84 aws-control backend tests, 42 security-posture tests, black / isort / |
Round 24 disposition - head d3211c4Two findings, both real, both fixed. The second is the sixth raise of the 1. Truncated AWS errors can bypass credential redaction - real, and a Both construction sites now go through 2. Session backup follows swapped ancestor links - real, and now closed The archive descent is now descriptor-pinned end to end. Each directory level is Windows has neither Pinned by making the property the test, not the platform: Gates: 89 aws-control backend tests, 42 security-posture tests, 932 tests across |
Follow-up on round 24 - head b358a86One red on the previous head, no new reviewer findings.
Gates: 89 aws-control backend tests, 42 security-posture tests, isort / flake8 / |
Coverage Gate - head 39ec790The only red on the previous head, and a legitimate one: five of this app's new
142 tests added across five new files, one per module, so the suites stay These are contract tests, not line-count filler. What they actually pin:
Three notes worth recording rather than hiding. Every FIFO fixture carries an |
Round 26 disposition - head 1371f1eTwo findings plus a Windows red. One finding fixed, one is the sixth raise of a 1. Final consent check ignores the granted account - real, fixed, and a good The chain is exact: a backup for account A starts, the same profile is repointed 2. Windows fallback permits credential-path traversal - real residual, Sixth raise of the descriptor-pinned-traversal class. Round 24 CLOSED its POSIX The remedy is to refuse session backups whenever Backend Tests (Windows) - a real source defect, found by one of the new Gates: 236 aws-control tests across the six suites, 42 security-posture tests, |
Correction to the round-26 dispositionThe round-26 comment says, of the Windows-fallback traversal finding, "Override Correcting it rather than quietly posting the override now, because the timing
So the correct order is: let the lane review this head, and if the Nothing about the code or the gates changes with this correction. The two |
|
/ai-review override gpt 1371f1e: Sixth raise of a claim adjudicated in rounds 8, 10, 11, 18 and 23 with the same remedy and no new evidence. The publish gate's contract is bytes becoming reachable OUTSIDE the box. A write into the owner's own bucket, with block public access on and SSE on and no public policy, creates no external reachability. The only chokepoint is a presign; both routes that mint one have been gated since rounds 5 and 8, and backup-section presigns are refused outright. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Round 27 disposition - head 1371f1eOne finding, and for the first time on this PR it is cleared by override rather S3 exports bypass publish governance - sixth raise (rounds 8, 10, 11, 18, 23, The reasoning, stated once more only because this is the comment the marker The publish gate's contract is that bytes become reachable OUTSIDE the box. A For the record on how this PR handled its review load: 27 rounds, roughly 70 No code changed for this round, deliberately: pushing after an override voids it. |
Frontend Coverage Gate - head 9e34473The backend half of the per-file gate went green on the previous head; the red
52 cases added: a new
Two real defects came out of writing these, both fixed: The download handler left an UNHANDLED PROMISE REJECTION. Round 21's fix opened
|
Account discovery + two UI fixes - head 1bf9b1cThree operator-reported problems on the Accounts page, fixed here rather than 1. The portal could not see most of the operator's accounts, and offered no way Two endpoints close it, both built on primitives the deploy path already
The refusals matter more than the happy path. A name must be one 2. The paid-services consent cards rendered under a "no results" empty state. 3. Those cards are account-scoped but read as page-level. Bundle Size Gate was the one red on the previous head: the all-language i18n Gates: 242 aws-control backend tests (6 new pinning the discovery/registration One i18n note worth recording rather than hiding: the first pass wrote the new |
GPT round on the new surface - head a6d2a64Two findings, both real, both fixed. Plus an attribution of the other reds, 1. Bootstrap authorized outside the lock, so account identity could drift Both checks now re-run inside the lock, immediately before 2. Per the HTML standard, The isolation is kept without the feature: the tab is opened without Worth recording WHY the existing tests could not see this. They mocked
Attribution of the remaining reds. Four of the seven on the previous head are
Main's own last three CI runs are failing, which is the simplest confirmation Gates: 245 aws-control backend tests, the CI per-file coverage script reporting |
Five reds, one real - head b1076a7Real and mine: Worth recording how it reached CI, because the miss was in my own tooling rather Infra, not findings:
Neither gate ever evaluated this branch, so neither red carries information. Two Stale, and main's rather than this branch's: No behaviour changed in this round. Gates: flake8 / mypy clean, 245 aws-control |
GPT round - head d42490fTwo findings, both real, both fixed. Both are correctness bugs on the surface 1. The bill was not scoped to the selected account. The query now carries 2. Registration discarded the region the profile declares about itself. The The two values are not alike. The account is whatever a live probe resolves, so A new Four pin tests: the CE filter is present and exact; a profile declaring Gates: flake8 / mypy clean, 249 aws-control backend tests, security-posture and |
Round - head 10b674dGPT: "Account authorization is racy" - REAL, and the previous fix was necessary but not sufficientThis is the seventh time the bootstrap-race theme has come back, and this raise is This raise names what remains after that, and it is right: What it is fixed with. The only way to learn which account a bucket is in is Why not the suggested fix. "Bind credentials to the verified account" means Why nothing is deleted. On mismatch this raises and leaves the bucket. A Five pin tests: the head-bucket call carries the verified account and precedes GPT: function-local imports violate
|
Round - head 11c498dGPT: Windows fallback permits raced junction traversal - accepted, fail closedThis one is not a missed check, and the code did not pretend otherwise: the walk So the finding is really a judgement about that documented residual, and on Changes:
Tests: the two that existed only to exercise the fallback are gone with it (the Windows shard 3 red is not this branch's
The phantom reds were self-inflicted, and the procedure is fixedFour cancelled-run "failures" over the last cycles ( From this push on, the body is re-pinned BEFORE pushing, using the SHA the local Gates: flake8 / mypy clean, 252 aws-control backend tests, security-posture and |
Round - head a9d0bdaGPT: drive discovery drops the verified account binding - REAL, and the theme is now closed structurallySame class as the create-side finding accepted last round, on the READ path: Rather than patch this one site, the binding is now enforced by S3 itself in two At the choke point. At each operation. Which calls accept the flag was established by asking the CLI (
Three pins on discovery: a foreign-owned bucket raises with the account named, the The Windows shard red was mine, from last round's fail-closed changeSeven tests exercised CI had reported six of the seven; the seventh lives in another shard and had not Gates: flake8 / mypy clean, 254 aws-control backend tests, security-posture and |
Round - head 6c0ea27Two findings, both real, both fixed. The first is a defect I introduced myself last Stderr truncated before redaction - my own regression
Fixed by calling S3 transfers were not bound to the verified bucket ownerLast round I documented Transfers now use the low-level operations that DO accept the guard -
Least privilege followed the change: Four pins on the transfers: put-object carries bucket, key, body and the verified Gates: flake8 / mypy clean, 256 aws-control backend tests, security-posture and |
Round - head fb81909GPT: malformed profile registry crashes AWS Control - REAL, fixed at the parse rather than at the call sitesThe finding is right and it is not narrow: Two facts worth putting on the record, because they change what the right fix is: The defect is in **The prescribed remedy - "catch malformed-shape errors at each new registry read"
The behaviour chosen is deliberate: valid JSON of the wrong shape now degrades Eight pins in Gates: flake8 / mypy clean, 256 aws-control backend tests, 193 deploy-profiles and |
|
/ai-review override gpt 6de5b20: Maintainer call. Findings since the rebase are nitpicks under the reachable-trigger test: the hardlink path needs an actor who can already read and exfiltrate the credential file; the upload drift needs three conjunctive conditions and the write is already owner-pinned; the registry shape needs a hand-corrupted file for a recoverable 500 in main's own code. Each push re-rolls this non-deterministic lane, so this is non-convergence, not a defect queue. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
One surface over the user's AWS accounts: an Accounts page aggregating the deploy profile registry by resolved account (one health light each, reconnect guidance matched to the profile's auth mechanism), and an Account Console with Library / Drive / Backup / Bill / Access sections on one private versioned bucket per account (three prefixes, stateless- by-tag discovery, deploy-engine hardening + CLI chokepoint). Guards: owner-only surface including reads; s3/ce join the aws-usage- consent enum (keystone leaf, fail-closed, drift-revoked); two-call confirm on bucket creation; restricted-session refusal + SEL audit on every mutation; presigned shares with a metadata-only ledger (URLs are never persisted); a self-contained 'drive' IAM tier pinned to kirocrew-drive-*. Spec: docs/system-specs/features/aws-control.md. Closes #5496
3 participants
What is the problem?
KiroCrew touches the user's AWS accounts from several disconnected corners -- deploy-web publishes sites, voice features bill Polly/Transcribe behind consent, agents run read-only CLI calls -- but there is no single surface that answers: which accounts can KiroCrew use, do their credentials still work, what has KiroCrew created in them, and what does that cost this month. And the gateway's own data (artifacts, sessions, memory, workspace) has no durable cloud home: no backup, and no way to hand a file to someone who does not run KiroCrew.
Why this issue matters to the user
How our fix solves it
One builtin app,
aws-control, spec atdocs/system-specs/features/aws-control.md(two-page design signed off before implementation):aws configure getreads, never by touching credential files).kirocrew-drive-<12hex>, BPA + SSE + BucketOwnerEnforced via the deploy engine's own hardening, PLUS versioning -- the drive's deliberate delta from deploy-web), discovered stateless-by-tag with ambiguity failing loud. Three prefixes serve three console sections:artifacts/(Library),drive/(Drive),backup/(Backup). All AWS access stays behind the deploy engine's singlerun_awsCLI chokepoint, gateway-side.s3,ce) join the existing aws-usage-consent enum -- grants stay in the one keystone leaf, re-verified against a live identity probe, failing closed; bucket creation is a two-call confirm (preview, then explicit confirm); every mutation refuses restricted sessions and is SEL-audited. A new self-containeddriveIAM tier renders the least-privilege policy pinned tokirocrew-drive-*for the user to apply (the gateway never writes IAM).What tests we did
test/test_aws_control_app.py): route inventory as a table (a route added without a gate fails the inventory), disabled/non-owner refusal across every route, restricted-session refusal across every mutation, consent-refused-before-any-AWS-call, two-call confirm (preview creates nothing), hostile object-key refusal, presign expiry clamp, tag-discovery ambiguity + foreign-naming refusal, share ledger never storing URLs, costs cache freshness + stale-serve-on-missing-consent, backup kind/key validation, nightly due logic, and the drive IAM tier's scoping (no deploy-web statements leak in either direction).tsc --noEmit, eslint with zero new warnings,check-app-manifest-sync(manifest prose byte-identical to the catalog),lint:i18n, and the fulli18n:checkrunner. Locale style findings (Hindi formal-pronoun register, Japanese long-vowel marks) were fixed at the source, not by moving baselines.Any other suggestions on the work
Closes #5496
Screenshots (pod e2e, SYNTHETIC staged data -- no real account identifiers)
Accounts page after the design rework -- dense ~40px rows, each leading with the account name and the FULL 12-digit account id, a single quiet aggregate line plus client-side search instead of the old stat-card strip. All API responses are staged with synthetic identifiers (the real-credential e2e run is described in the Tests section; its screenshots are not published):
Account detail page -- grouped General card (name, full id with copy, region, connection, keys) and a Connections section with one row per key (kind badge, region, health). Reconnect now lives here for failing keys. Below: settled stats (em dash with a hint where the test account has Cost Explorer disabled -- the 502 settles instead of skeletoning), the S3 setup confirmation card, ghost cards:
An UNRESOLVED account row cannot open a console, so clicking it expands inline Reconnect guidance instead of being a dead row (command classified gateway-side, display-only with copy):