feat(aws-control): move the library removal onto the cloud listing - #7810
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Removal now targets the object the bucket listing observed instead of a ledger inference — root-cause placement, residual label defect honestly scoped to #6987. [DESIGN-REVIEWED] f908b2f |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Solid, well-evidenced move of a destructive control; two copy/friction trades deserve a human eye before merge. Watch
Suggestions
[UX-REVIEWED] f908b2f |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All counts are run and the repository verification is complete. Here is the review. First-Principles-Verdict: CONCERNS The removal relocation is a derived, well-aimed fix; the Drive Download move and the shared confirm's Cancel-disable ride along on consistency-only and undeclared grounds. What this change shipsIntent: let a user remove a cloud copy from the folder that lists it — a FIX (the deferred item #7026 named).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] f908b2f |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @chenmingwei23 overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo findings. The sole candidate — [OPUS-REVIEWED] f908b2f Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
04abc05 to
14940f7
Compare
14940f7 to
2dfb3b9
Compare
2dfb3b9 to
72ad64b
Compare
72ad64b to
e3d51d8
Compare
|
On the two Remove items at The render path. Both items are in the Library folder's cloud listing, not the picker: one in What the delete targets. The listing is a delimited read of the bucket: The concession, and it is the part of my earlier reasoning that does not hold. I previously argued the confirm naming Nothing ledger-derived can close it either. Why the remedy is declined. Rendering Remove only when What would actually close it is identity read from the bucket rather than the ledger. I have checked whether that data is already available: it is not. The Library listing is delimited, so each slug arrives as a |
4e03a66 to
2e596a4
Compare
A removal offered on the "Add from Artifacts" picker cannot know which cloud copy it is deleting. The picker's rows are LOCAL artifacts joined to a ledger keyed `account -> slug`; `ArtifactStore.delete` does not prune that ledger and a new artifact starts at version 1, so pushing A, deleting A locally and creating a B that takes A's slug leaves B's card wearing A's push record. Removing from B empties `artifacts/<slug>/`, which is A's copy, under B's name. No predicate available on that card separates the two. The Library folder beside it lists the real bucket prefix, so a row there IS a cloud folder and removing it empties the object the reader was shown. The control moves there, into the same overflow menu the Files folder's own cards and rows use, and is offered on every listed copy including one no local artifact backs -- which is what makes a copy pushed from another machine removable rather than stranded, the promise `remoteOnly` already carries. Drive's Download moves into that same menu, in both views, so a folder offers one grammar for "act on this item" rather than a bare button beside an overflow.
2e596a4 to
f908b2f
Compare
|
/ai-review override gpt f908b2f: Zero-delta vs base: main's PickerCard:1034 calls the same libraryRemove(slug) and empties the same artifacts/S/ bytes under the replacement's name; main's own comment at :1045 concludes it cannot prove ownership without the pushed meta.json. This PR relocates the control and does not move that harm -- #6987 owns the cause. Gating on !local would hide Remove whenever a local copy exists, which with the picker's Remove gone leaves no path to delete a cloud copy. Residual documented in the body. |
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. |
buluoray
left a comment
There was a problem hiding this comment.
Verdict: 0 blocking, 3 non-blocking. Reviewed at head f908b2fbad46ecc786956cd3a57f482b705d4167.
What I verified
- Delete target is the row, not ambient state.
removeMutcallsawsControlApi.libraryRemove(account, slug)whereslugis the folder enumerated by the bucket listing (driveList->pg.folders), threaded per-card/per-row (onSelect={() => askRemove(slug)}/onAskRemove). Thelocal = bySlug.get(slug)join supplies only the card's label/thumbnail. Verified there is exactly onelibraryRemovecaller on HEAD (DrivePage.tsx:336); the picker's old caller is gone, no orphaned callers. - Old removal path fully migrated. The picker's Remove control and confirm strip are deleted; the test
does not offer removal from the pickerasserts bothlibrary-removeandlibrary-moreare absent inside the "Add from Artifacts" dialog (DrivePage.test.tsx,queryByTestId('library-more')null). No dead entry point left behind. - Server-side authorization unchanged.
libraryRemoveis a pre-existing backend endpoint; this PR relocates the UI caller only and does not weaken any server guard. - AUTOSDE blocking rules (website/AUTOSDE.yaml).
frontend-security: noinnerHTML/dangerouslySetInnerHTML/template-literal HTML added.accessible-interactive-elements+icon-buttons-need-labels: both newlibrary-moretriggers carryaria-label={i18nT('apps.awsControl.console.library_actions')}(DrivePage.tsx:697,913), and a test asserts the label + that the trigger sits OUTSIDE the<Link>(no<button>in<a>).use-lucide-icons/no-emoji-as-icons: uses lucideMoreHorizontal, no emoji.page-layout-patternregex applies tosrc/pages/*.tsxonly — N/A here. Remove is aDropdownMenuItembehind the overflow, not a visible danger button. - i18n. 4 new keys (
library_actions,library_remove_action,library_removing,library_remove_failed) added toen.json+ 11 catalogs +en-XAregenerated +en.context.json; all user-facing strings route through the catalog. - Spec is in-commit.
docs/system-specs/features/aws-control.mdmodified in the same commit (AGENTS.md maps this surface to that spec). - Reuse-slug residual (#6987) is pre-existing, not introduced. The delete target is the enumerated bucket prefix
artifacts/<slug>/(a test pins it namesartifacts/, notlibrary/); the residual is the borrowed label, identical in bytes/request/target to main's picker path. The GPT 5.6 finding on this was human-overridden with a sound zero-delta rationale. Correctly scoped out. - Tests pin behavior. New tests drive through the real menu (
chooseFromMenu), assertlibrary-removecount 0 at rest, cover the no-local-twin copy, the reuse-slug boundary in grid AND list, and the per-slug pending/failed sets. I did not re-run the suite; CI (Frontend Tests/Lint/Type Check, Coverage Gate) is green at this SHA.
Findings
website/src/i18n/locales/en.json:3709—library_remove_failedoverclaims on partial failure (non-blocking). Text is "Remove failed — the cloud copy is unchanged. Try again."libraryRemoveis adelete_prefixsweep over multiple S3 objects, so a mid-sweep failure can leave the prefix partially emptied while this asserts nothing changed — misinformation on an irreversible action. Consequence: a user skips re-verifying and believes a partially-deleted copy is intact. This is inherited framing from the olddelete_failedand is recoverable (retry stays available), so non-blocking. Suggestion: drop the state claim ("Remove failed. Try again.") unless the backend confirms zero deletions. (Matches the UX lane's CONCERNS.)DrivePage.tsxDriveDownloadrelocation (drive-grid-download/drive-download) — rides along on consistency grounds (non-blocking). Download moves from a one-click bare button to an overflow item in both views; the core Remove-relocation fix stands without it, and no failing user is named for the Download change. Consequence: the most frequent Files action costs a second click for habituated users. Suggestion: a human ratifies (or defers) this scope extension rather than inheriting it. (Matches First Principles + UX advisories.)DrivePage.tsx:~1706,1774—TileConfirmdisabled={pending}on Cancel now reaches the two Files-folder deletes (non-blocking). A behavior change on a surface outside this PR's stated scope. Opus falsified it as a defect (the feeding mutations always resolve and re-enable Cancel; Cancel only closes the strip and never aborts the request), so it is safe. The in-code rationale comment ("this strip is the ONLY place the outcome can render") is inaccurate for the library caller, whose failure also renders on the card viafailedSlugs. Suggestion: correct or scope that comment so a later editor does not propagate it.
What I could not verify
- I did not execute the 730-test suite or the screenshot harness locally; I relied on green CI at this SHA and read the test assertions statically.
- The backend
delete_prefixpartial-failure behavior (whether it reports objects-deleted on error) I inferred from the diff comment ("adelete_prefixsweep over several S3 objects"); I did not openstorage.py/routes.py, so finding #1's exact partial-failure surface is described from the PR's own text plus the client-side message. - Runtime rendering of the overflow menu (portal/z-index/focus behavior) was checked via tests and static reading only, not in a live browser.
Audit note — part of this has already landed; the rest has notThis PR is not a duplicate and is not finished by anything on Already landed#7026 CONFIRMED MERGED: the issue/PR reference check row Which parts
|
Problem / Motivation
What is on
maintoday. The picker-based removal shipped with #7026 (90f708efb). Its only entry point is a per-card control inside a dialog labelled "Add from Artifacts", so a user standing in the Library folder -- looking at the very copies they are paying to store -- has no affordance to remove one. Raymond reviewed that placement and directed the move onto the folder's own listing, behind an overflow menu.That is not a change of mind about a working design. The UX lane on #7026 reached the same conclusion independently, on the merged head, and stated it as its verdict:
#7026 did not dispute the mechanism. It dispositioned the finding as disproportionate for its own scope and named this exact change as the deferred fix: "Smallest fix is the PR's own deferred item: Remove on
LibraryCloudCard." This PR is that deferred item.Why the placement is not merely undiscoverable but unsound. The picker cannot host this control at all. Its rows are local artifacts joined to a ledger keyed
account -> slug.ArtifactStore.delete(src/kiro_crew/artifacts.py) does not prune that ledger, and a newly created artifact starts atversion = 1. So:slug -> {version: 1, pushedAt}list_pushablejoins B to A's recordB's tile now renders as synced, Remove appears on it, and clicking it empties
artifacts/<slug>/-- A's cloud copy, under B's name. No predicate available on that card separates the two:syncedis the inherited record, andpushedVersion === versionis satisfied by a never-pushed B at v1 against A's pushed v1. #7026 mitigated this by naming the bucket folder in the confirm, which narrows the blast radius but cannot fix it: the reader is still asked to vouch for an identity this machine cannot establish.The inversion is the cleanest statement of why the placement is wrong: a copy pushed from another machine could not be removed at all, while a locally reused slug could remove the wrong one.
Why it matters
library_remove's own docstring already expects the cloud listing to be its caller:That is the backend describing a UI that walks the bucket, not the local store. The spec says the same thing about
remoteOnly: a copy pushed from another machine "would otherwise be unreachable from the console that must be able to remove it". Today's placement gets that exactly backwards. This PR is not a redesign; it finishes the wiring the backend was written for.What changed (motivation -> approach -> change)
Removal moves to
LibrarySection's listing. Its rows come fromdriveList, so a row is a cloud folder and removing its slug empties the object on screen. Offered on every listed copy, gated on nothing, including one no local artifact backs.It is hidden behind the same overflow menu the Files folder uses. Same
MoreHorizontaltrigger, sameDropdownMenu/DropdownMenuTrigger/DropdownMenuContent align="end"primitives, sameTileConfirmfollow-up, on cards and on list rows. A visible danger button on every card of a browse surface was the rejected design; a reader moving between the two folders met two grammars for "act on this item".The trigger sits outside the
<Link>. The card shell became a<div>with the link inside it. A<button>inside an<a>is invalid content, and a trigger that had topreventDefaultout of the surrounding navigation is the nested-control hijack this page avoids elsewhere. cmd-click and middle-click still open the artifact.Confirm state lives on the section, not the card: one
confirmSlugplus oneuseMutation, withremoveMut.reset()on open. This removes the state two review findings lived in rather than resetting it. There is nosyncedflag to fall out of (a card exists exactly while its object is listed, and success invalidates the listing so card and strip unmount together), and one card's failure cannot render inside another's confirm. The drive's own confirms carry that second wart; this deliberately does not copy it.Deliberate scope extension, on Raymond's instruction: Drive's
Downloadmoves into the same overflow in both views. It was a bare<button>beside the trigger while Share and Delete were inside it, the same inconsistency one component over, and shipping a new Library menu while leaving it would recreate what was just rejected. The non-obvious part is popup survival:download()callswindow.open('', '_blank')and must stay inside the user gesture or a blocker eats it. Radix dispatchesonSelectsynchronously from the item's own click handler, so it does; the assertion that proves it is unchanged.Four new i18n keys --
library_actions,library_remove_action,library_removing,library_remove_failed-- acrossen.jsonplus 11 translated catalogs,en-XAregenerated. Four, not seven:mainalready carrieslibrary_remove,library_remove_confirmandlibrary_remove_confirm_slugfrom #7026.library_remove_failedreplacesdelete_failed, whose "Delete failed - the file is unchanged" is the wrong verb and the wrong noun for removing an artifact's cloud copy.Kept from #7026 rather than reverted: the confirm stays a single
<Trans>with a<folder>tag. That is better than a lead-in string plus a sibling<code>, because a translator can reorder around the monospace chip. Only its contrast changed, totext-textfromtext-muted, per UX below.Scope limit -- what this does NOT fix
Relocating fixes the delete target: the object deleted is the object the bucket listing enumerated, not one inferred from the ledger. It does not fix the borrowed title.
bySlugis built from the same slug-keyed ledger (if (a.pushedAt !== null) bySlug.set(a.slug, a)), so a reused slug can still lend a card the wrong artifact's name, kind and preview. That is #6987's cause-level fix -- read the pushedmeta.jsonsidecar instead of inferring from local state -- and it is unchanged here.Being exact about the residual, because an earlier draft of this section was not. I previously argued that naming
artifacts/<slug>/in the confirm reduced this to a display defect. It does not. The prefix is derived from the slug, and the slug is precisely what the second artifact reuses, soartifacts/S/reads identically whether the bytes are A's or B's. The reader is given nothing they can check. So the honest statement is: the delete target is correct and never a ledger inference, and the displayed identity can belong to a different artifact, on an irreversible action. Both halves are true at once.Measured against base, this is zero-delta. On
origin/maintoday,data-testid="library-remove"sits atDrivePage.tsx:1034insidePickerCard(defined at:927) and calls the samelibraryRemove(account, slug)at:749. So in the reuse case on the current release a reader opens "Add from Artifacts", sees row B, clicks Remove, andartifacts/S/-- A's bytes -- is emptied under B's name. After this change the same reader sees a card labelled B in the Library folder and the same call empties the same prefix. Same slug, same request, same bytes, same false belief. Main's own comment at:1045already describes this mechanism and concludes it "cannot prove the copy is this artifact's without reading the pushed meta.json sidecar". The relocation moves where the control lives and does not move this harm in either direction, so it is a pre-existing hole owned by #6987 -- not applicable to this PR, rather than a false positive. The finding is real.Why the proposed remedy is refused. Rendering Remove only when
localis undefined would hide it in the normal case -- you pushed a copy and still hold the original. Combined with this change removing the picker's Remove, the product would then have no path at all to remove a cloud copy in that case, which is worse than main, where the picker at least offered one. It leaves the harm intact and deletes the operation.The next step, deliberately not in this diff. A per-slug rollup -- object count, bytes, and newest last-modified, grouped by the first path segment after
artifacts/-- would let the confirm show something a reader can actually check ("2 files, 1.4 MB, last modified 12 Aug" is visibly not an artifact created minutes ago). The whole-bucket undelimited listingusage()already performs is the natural source; it projectsContents[].{Key, Size}today and would needLastModifiedadded. That is a new capability acrossstorage.py,routes.pyandtypes.ts, so it belongs in its own change rather than bolted onto a converged relocation.Review advisories from #7026, and what answers each
delete_failedvocabulary: the newlibrary_remove_failedkey.text-texton the sentence and its path chip. It names the bucket folder the delete will empty, which is the identity check the whole rationale rests on; styling it as an aside undercut the argument.TileConfirm": satisfied structurally. The rebuild reusesTileConfirm, widened only for aReactNodelabel and a test-id base.LibraryCloudCard, one in the list row -- andlocalcan indeed be defined at both, so that half of the finding is accurate. It is not what makes identity provable. The listing enumerates the bucket (driveList(account, 'library')->pg.folders), so a row exists if and only ifartifacts/<slug>/is in the cloud, and the removal empties exactly the prefix the row was listed from: the target is the object that was observed.localsupplies the card's label and thumbnail only. On the picker the row IS a local artifact and no cloud object has been observed at all, so there the target would be inferred from a slug-keyed ledger -- which is why that surface has no removal, pinned by its own test. Under slug reuse the residual is the LABEL, not the target, and the confirm's operative sentence already names the folder it empties rather than the label. GPT's remedy -- render only whenlocalis undefined -- is declined: most cloud copies still have their local original, so it would remove the affordance from nearly every card and restore the gap this PR exists to close. The rebuttal is now enforced rather than argued: a new test pins the boundary in both views, and applying GPT's remedy to either site reds it.onSuccessclearedconfirmSlugunconditionally. Round two:removeMut.reset()discarded an in-flight removal's state, so a failure had nowhere to report. Round three: the slug-keyed failure was itself one slot, so of two overlapping failures the later hid the earlier -- GPT'sSetis the right answer and is taken, extended to the pending state as well, which had the identical defect one round ahead of being reported. Its prescription in the first two rounds -- return early fromaskRemovewhileisPending-- remains declined: it makes the Remove item a dead click with no feedback whenever any sibling removal is in flight, trading these defects for a worse one. See the class note under Tests.Tests
vitest run src/apps/aws-control/DrivePage.test.tsx src/i18n/gives 730 pass across 43 files.tsc -bandeslintclean. Counts are not the argument, though -- what follows is.Mutation-verified: what each guard is worth
Each row below was checked by breaking the code and confirming a named test goes red. No mutant survived, across eleven rows. Earlier drafts of this table did have survivors, and each one was the useful signal: once it showed a guard was genuinely redundant (it was deleted), once it showed a test was missing (it was written), and twice it showed my own mutant was broken and never reached the code -- which is now checked before any conclusion is drawn from a survival.
failedSlugscollapsed to a single slugkeeps both failures when two removals failpendingSlugscollapsed toisPendingonSettlednever clears pendingreports a failed removal on its card and retires it on cancelonSuccessclears the confirm unconditionallydoes not close one card's confirm when a different removal completesdoes not erase an existing failure when another confirm opensshows the in-flight label only on the card being removedlibrary/<slug>/instead ofartifacts/<slug>/removes from the cloud listing, naming the artifacts/ prefix it emptiesdoes not offer removal from the picker, where identity is unprovableoffers removal on a listed copy with a local twin, and targets the listed prefixDownloadno longer inside the grid overflowgrid mode carries the same actions the table rows carryOne class, three layers, and why the third fix changed shape
Three review rounds each found a real, reachable defect here, and the first two fixes were each one layer deep. Recording all three, because the pattern is the point:
confirmSlugwas cleared unconditionally, so a completing removal closed a different card's confirm.removeMut.reset()on open discarded an in-flight removal's state, so a failure that arrived afterwards had nowhere to report.variablesholds only the most recent slug, so an in-flight label could appear on a card nothing was happening to.All three are one defect: per-card state held in a value shared across cards. Patching them individually was chasing shapes of the same mistake, and layer four was already implied by the code before it was reported. So the third fix changes the shape instead: N cards get N slots.
pendingSlugsandfailedSlugsare sets, every per-card indicator is derived from membership, and nothing per-card consults a shared value. The check is mechanical rather than a promise -- the only member this section reads off the mutation observer is.mutate.confirmSlugdeliberately stays a scalar: only one confirm strip is open at a time, which is a constraint on the surface, not a shared slot standing in for N.This shape was not invented here. The picker 600 lines down has always held its per-card push state as
addingSlugs/failedSlugs; the regression was mine, in moving the affordance across without carrying that lesson with it. The copy-on-write helpers are now hoisted to module scope and used by both, so there is one spelling of the idea rather than two.What each test would let through if deleted
removes from the cloud listing, naming the artifacts/ prefix it emptiesalso assertsnot.toHaveTextContent('library/notes/'), and drives the control through the menu. Delete it and both a visible danger button on every browse card and a removal keyed to local state pass again. The fixture is deliberately the reused-slug shape: the bucket holdsnoteswhile the local artifact of that slug reportspushedVersion: null.removes a cloud copy that has no local artifact behind it: the local library is empty and the removal still fires. Delete it and gating removal onsyncedpasses again, which strands every copy pushed from a second machine.does not offer removal from the picker, where identity is unprovable: both fixture cards are synced, the state that used to reveal the control, and bothlibrary-removeandlibrary-moreare asserted absent inside the dialog, so the unsound placement cannot return as a button or as a menu.offers removal on a listed copy with a local twin, and targets the listed prefix: the complement of the picker guard -- lists a cloud object whose local twin is a different artifact wearing its slug, and asserts the control stays and targets the listed prefix, in grid AND list view.keeps both failures when two removals fail: starts two removals, fails the earlier one first, and asserts both cards carry their own error -- a single slot would end up holding only the later.shows the in-flight label only on the card being removed: holds A in flight, opens B, and asserts B's strip offers its action and a usable Cancel rather than A's progress.does not close one card's confirm when a different removal completes: holds A's removal open, opens B while it is in flight, asserts B's own controls are not wearing A's pending state, then resolves A and asserts B's strip survives.keeps a failure on the card that owns it when another confirm is opened: A fails while B's strip is the open one, and the failure reports on A's card, not on B and not nowhere.does not erase an existing failure when another confirm opens: the reverse order -- A's failure is already on screen, then B's confirm opens and B is cancelled, and A's failure survives both.reports a failed removal on its card and retires it on cancel: the strip stays open so a retry is one click, and Cancel retires that card's failure with the attempt it describes.carries the removal in list view too: the view choice persists per section, so a control the grid has and rows do not is one a reader can never reach again.The screenshot harness carries 13 assertions of its own. The load-bearing one is
library-removecount 0 at rest: the item is portaled, so its absence from the document is the proof that the destructive path is not on display.Manual verification
Captured against the real built
website/distwith the API layer answered from fixtures, full app window at 2360x1640 (deviceScaleFactor 2), no crop. A pod cannot show this feature: with no AWS profile, no S3 consent and no bucket, the Library folder renders empty and the control has nothing to attach to.Screenshots / video
The listing at rest, one card per object in the bucket, each with the overflow trigger, no destructive control on display. The middle card,
from-another-machine, has no local artifact behind it and is the case the old placement could not reach:The menu open. A deliberately hidden control needs one frame that shows it:
The confirm, naming both the item and the prefix it will empty, at full contrast:
Related Issues
Follows up #7026. Leaves #6987's cause-level sidecar read open; see the scope limit above.
Pattern harvest
Two findings worth carrying beyond this PR.
A screenshot harness without assertions cannot tell a stale bundle from a broken page.
capture-aws-control-library-remove.mjs'sDRIVEfixture originally omittedusage.sections, whichDriveUsagerequires, soStorageMeterdereferencedusage.sections.driveonundefinedand the harness photographed the page's error boundary while exiting 0. It surfaced only because this script'sexpectCountassertions fail the run.mainhas since fixed this file's fixture, and #7801 fixes the siblingcapture-aws-control.mjsthe same way, so nothing is changed here for it. The class is still worth naming: an evidence harness needs assertions or it will publish "Something went wrong" as a feature demo.Reading
headRefOidwithoutstatemakes a merged PR look open.gh pr viewpopulatesheadRefOidon merged PRs, so a check keyed to it reported #7026 as open at its squashed pre-merge head and nearly produced a force-push onto a deleted ref, from a base 440 files behindmain.Any other suggestions
The
library_remove_confirm_slugkey resolves fromen.manual.jsonrather thanen.json. That is legitimate and the key-reference gate passes correctly, but it means a grep ofen.jsonalone reports the key missing, which is worth knowing before anyone concludes the confirm is broken.