Repository navigation
feat(files): add drag-and-drop and picker upload to the Files panel - #9749
amadsalmon wants to merge 1 commit into
Conversation
github-actions
Bot
added
fork
cf10b5f to
7884f55
Compare
github-actions
Bot
added
readiness: checking
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS No design-level concerns. The new create-only endpoint reuses the composer's admission gates and the existing path gate, re-checks the full destination, and changes no default behavior of existing helpers. Notes
[DESIGN-REVIEWED] 07b80f5 Description read[DESCRIPTION-READ] 9e5c8b8fffdaaaa4c55855230052a5cdb12fa44e936182fd8d247f149dfd0f07 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
UX Review (Opus 5.5, fork) — ✅ PASSUX-level review of UX-Verdict: PASS Nothing blocks. The screenshots show every new control and state (row-menu "Upload files…", drop overlay, progress, success, collision error, refused-type error), but this fork lane has no blind read, so no first-time reader has judged them. Notes
[UX-REVIEWED] 07b80f5 Description read[DESCRIPTION-READ] c6b32195322b59e014d160f95a30162e7868e1bd2737bf6c4905ee736d17534d That sha256 names everything this verdict read: the PR description, plus the 10 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Additive capability beside existing ones; no default moved, no existing control relocated. One fail-closed branch the author calls unreachable. Notes
What this change shipsInventory (8 items) — 7 justifiedIntent: let a user put a file from their computer into the workspace from the Files panel. ADDITION.
[FIRST-PRINCIPLES-REVIEWED] 07b80f5 Description read[DESCRIPTION-READ] 01db23aa6f51d8c9950045054d6e8233b3647f902321b444272308927fa058b5 That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
github-actions
Bot
added
readiness: action required
97e81e3 to
ea7d9f9
Compare
github-actions
Bot
added
readiness: checking
83feb68 to
c7e055c
Compare
github-actions
Bot
added
readiness: checking
c7e055c to
febd887
Compare
|
|
|
|
Holding at
— crew-pr-reviewer (r116) |
|
|
On the hold at
Sent by an AI agent on behalf of Amad |
|
|
|
@bolichen97 @chenmingwei23 this is ready for review. CI is fully green on @bolichen97, the owner gate from your 2026-09-25 review is in place: Thanks! Sent by an AI agent on behalf of Amad |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed head 3a209e9ba9.
My earlier change request is resolved. api_directory_upload now calls require_owner_dashboard_request(request, "upload.directory") as its first statement, before the query parse, the directory probe and the multipart read. That is the same order api_file_write uses on main, and test_directory_upload_refuses_a_non_owner covers it. I found no new blocking issue in the handler: the full destination is re-gated through _validate_dashboard_path, the create is O_EXCL so an existing name always answers 409 name_collision, and the extension allowlist and magic-byte check match api_upload_file.
One thing blocks the merge: the branch conflicts with main. The only conflicting file is docs/feature-map/README.md; the locale files and learn-cron-dashboard.md merge cleanly. Please rebase onto current main, resolve that row, and push. When the new head is green with readiness: passed, I will take a final look and replace this change request.
What I checked on this head: PR Readiness is green, every AI review lane passes, the linked issue is a T2, and the owner gate and its test are present.
|
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Re-reviewed the change since 335d208 at 5cbfb99. Besides the rebase onto main, the only change is in useDirectoryUpload: a successful batch now also invalidates the rail filename search, the folder tab listing and the folder tab search, by key prefix because those are keyed by roots the hook cannot know. I checked that the three keys match the ones FileBrowserRail.tsx and FolderPanel.tsx read on main, and the updated test seeds each key and asserts it is invalidated. No new findings.
This stays a comment, not an approval: the PR adds a new write route and edits docs/feature-map/README.md, so the approval belongs to a human maintainer, and readiness and the test jobs are still running on this head.
|
HOLD at 88da04a for maintainer review. The items from our r116 hold are addressed: the Opus comment wording is fixed, @bolichen97's change request is dismissed, and the root-target upload button and accepted-types message are now items 9 and 10 of #12124. All AI lanes pass and every check is green on this head. I found no defect in what I read: What still needs a person:
I read the route, the handler and the lane bodies, not every test file. I did not run tests. — crew-pr-reviewer (r119) |
|
Human design review is required before this PR can merge. Workspace upload adds a new remote write path. A maintainer must approve destination scope, file types, collision behavior, and replacement rules. This comment records the design gate only; it is not a code defect. |
|
@bolichen97 thanks, understood: this needs a maintainer's design sign-off. To make that review quick, here are the four rules as the PR implements them today:
Whoever reviews it: if you'd rather any of these be different, tell me which one and I'll change it. |
— Sent by an AI Agent on behalf of Amad |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Re-reviewed at bd6a8cc.
What changed since 818556e: frontend only. The Files rail now shows an "Uploading ..." status while a batch runs and a dismissible "Uploaded ... to ..." line after it, a second drop of a name still uploading into the same folder is skipped instead of sent, and the new strings are in every locale with plural keys registered. The backend route is unchanged.
The delta reads correctly to me. The in-flight set is keyed by the normalized folder plus name, keys are released when the batch ends, a project switch clears the notices, and StatusNotice keeps both lines as polite role="status" updates. The two new tests cover the progress line, the success line and the skipped re-drop.
One item on unchanged backend code, inline below: the GPT lane now blocks on the Windows create path in _write, and the Design and First Principles lanes point at the same spot. Routing that branch through one pinned_fs by-name create would clear the block and remove the duplicate sequence in one change.
Status: this still needs a maintainer's design sign-off on the new write route (destination scope, file types, create-only collision rule), so I am leaving a comment rather than an approval.
Checked: interdiff 818556e to bd6a8cc, the Windows branch of _write against write_new_file_pinned at this head, CI and lane results on this head. Not run locally.
| os.close(fd) | ||
| except OSError: | ||
| pass | ||
| if created is not None: |
There was a problem hiding this comment.
🟡 Windows rollback skips the second identity probe
The Windows branch runs its own create, write and rollback instead of the pinned_fs sequence. When os.fstat(fd) fails, created stays None, so the rollback only closes the descriptor and the empty file stays under the O_EXCL name. The upload answers 500, and every retry of that name then answers 409 until the user deletes the empty file by hand. write_new_file_pinned handles this case by asking the descriptor for its identity a second time before closing it.
if created is not None:
pinned_fs.unlink_verified_by_name(
Path(target_dir), safe_name, (created.st_dev, created.st_ino)
)Suggestion: add a by-name variant of the pinned_fs create (same steps, os.open(dest, ...) instead of dir_fd) and call it here, so both platforms share one sequence. That clears the GPT lane block on this line and the duplicate-code note from the Design and First Principles lanes. If that is too big for this PR, at least retry os.fstat(fd) before the close when created is None.
Rule: one owner for each filesystem transaction; a rollback must not leave behind a name that the request created.
— Sent by an AI Agent on behalf of Amad |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Re-review at f1767ed.
Delta since bd6a8cc: one backend change in dashboard/file_api/uploads.py plus one regression test in test/test_handlers_files_directory_upload.py. Everything else in the PR is unchanged; the locale files differ only by what main added.
Previous finding (Windows rollback after a failed first os.fstat): resolved. On the by-name branch the rollback now re-asks the identity through the still-open descriptor when the first probe failed, closes, and unlinks only through unlink_verified_by_name with that identity. This is step for step the rollback contract of pinned_fs.write_new_file_pinned, including leaving the name in place when both probes fail (nothing was written by then, so only an empty file can remain). The new test forces the first probe to fail on the by-name path, checks the 500 and that the file is gone, and checks that a retry of the same name returns 200 with the right bytes.
What I verified: read the delta at this head, compared the new rollback with write_new_file_pinned on main, and read the new test. I did not run the suite locally. The AI review lanes have not run on this head yet, so the earlier GPT block still points at the old head.
Optional, not blocking: write_new_file_pinned needs a dir_fd, so the by-name copy is still a second spelling of the same sequence. A by-name mode in pinned_fs would let a future fix land once. Fine to leave for a follow-up.
No new findings. This is a feature that still needs a maintainer to agree to the design (destination scope, allowed file types, create-only collision rule), so this is a comment, not an approval.
— Sent by an AI Agent on behalf of Amad |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Re-review at 0a69a16 (previous review was on f1767ed).
What changed since f1767ed (the base also moved to current main, which did not touch any file in this PR):
- Windows upload now pins the target with
pin_directory(target_dir, links_only=True), so a OneDrive or other cloud-files folder (a reparse directory whose tag is not a name surrogate) is pinned instead of refused with 403. A link or junction at the name is still refused. This addresses the blocking Opus finding on f1767ed. pin_directorygains a keyword-onlylinks_only(default False, so every other caller is unchanged). It reuses the existing_win_reparse_refused/win_fd_is_linkhelpers thatopen_file_no_reparsealready uses, so a tag that cannot be read still fails closed as a link.unlink_verified_by_nameforwardslinks_only, and the upload rollback passes it, so the rollback pin and the create pin agree on the same folder.- A pin refused for a reason other than a link (sharing violation, EACCES) now propagates and answers 500
upload_failedinstead of 403symlink_refused. OnlyNotADirectoryErrorandELOOPstill map tosymlink_refused. The final path check after the pin is unchanged. - New tests: the four
pin_directory(links_only=...)cases on the stubbed Windows branch (default refuses cloud-files, links_only pins it, a junction and a file stay refused), plus handler tests for a cloud-files upload landing, a non-link pin refusal answering 500, and rollback in a cloud-files folder.
Verified: read the full delta between the two heads; confirmed main did not move any touched file between the two bases; ran test/test_handlers_files_directory_upload.py and test/test_platform_compat_filesystem.py at this head on Linux: 106 passed, 5 skipped. The Windows branch is exercised only through stubs here; no Windows host was run.
Findings: none new. The previous review's note still stands as an optional follow-up: the Windows create sequence is a second spelling of write_new_file_pinned, now behavior-equivalent including the cloud-files case.
The AI review lanes are still running on this head, so their verdicts are not yet known. This is a feature that still needs a maintainer design sign-off on destination scope, allowed file types and the create-only collision rule, so this is a comment, not an approval.
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Re-review at 1ff3664.
Delta since 0a69a16: a rebase onto current main plus one test edit. test_windows_failed_write_into_a_cloud_files_folder_still_rolls_back now patches pin_directory through the kiro_crew.pinned_fs facade instead of importing pinned_fs_files directly. The facade forwards a patch to every owner module that holds the name, so the rollback in unlink_verified_by_name still sees the stub, and the test still asserts the rollback pin used links_only. No production code changed. The rebase hunks in the locale files match the previous head.
What I verified: the range diff between the two heads, that the facade forwards writes to pinned_fs_files, and test/test_handlers_files_directory_upload.py at this head against this head's source on Linux (59 passed). Windows paths are stubbed in that file, not run on Windows.
No new findings. The AI review lanes are still running on this head.
This is a comment, not an approval. The feature still needs a maintainer to sign off the design: the destination scope, the allowed file types and the create-only rule for name collisions.
Add POST /api/directory-upload so the Files panel's drag-and-drop, drop overlay and "Upload files..." action can write a file into a directory the panel already shows. Reuses api_upload_file's admission gates (extension allowlist, size cap, magic-byte content check) and the dashboard path gate, with platform-aware ancestor pinning. Upload is create-only. On POSIX the file is created through pinned_fs.write_new_file_pinned, the shared O_EXCL create under the pinned directory with an identity-checked rollback; on Windows the same O_EXCL create runs by name under the held pin_directory handle. An existing name is never replaced: a name collision, including one lost to a concurrent upload, answers 409 name_collision, and the panel shows "<name> already exists in this folder. Rename your file, or remove the existing one, then upload again." A failed write removes the partly written file and answers 500 upload_failed. A new file gets atomic_write's default mode. The handler opens with the dashboard owner gate (require_owner_dashboard_request), matching the sibling /api/file-write route: a non-owner holding a channel-created dashboard session cannot use it to plant steering docs, SKILL.md or the MCP config. The gate runs before the query parse, path probe and multipart read, so a non-owner also cannot learn whether a path exists. Tests: non-owner refused (403 owner_only) before the probe; owner positive control; an existing name answers 409 name_collision with the original bytes unchanged, including for a request that sends overwrite=1; a rival created just before the create wins and the upload gets 409; a write that fails mid-body leaves no file on both the pinned and the by-name branch.
Problem / Motivation
Goal: let a user put a file from their computer into the workspace from the Files panel, by drag-and-drop or a file picker.
The Files panel can move files out of the workspace but not in. Download exists (
components.fileRenderers.download,apps.fileExplorer.fileViewer.download); the completeapps.fileExplorer.*key set contains no upload, drop, or new-file string, and the sidebar tree's row menu holds exactly one entry (Add to chat). There is no way to get a file into the workspace from the panel that reads and writes it.Four other surfaces in the same dashboard already accept a dropped file — the chat composer (
components.chatInput.drop_to_attach), Knowledge Base (knowledge.sourcesList.drop_files_here_or_click_to_upload), Design Critique, and the AWS Control file drive (#7983) — so this is a consistency gap on one surface, not a new capability for the product.Why it matters
For anyone running the dashboard against a remote machine (Cloud Desktop, dev box, pod), the Files panel is the only file tool that already works, because it reads and writes through the gateway rather than the local filesystem. Every alternative for getting a file onto that machine is a separate stack with separate credentials. Kiro IDE's Remote-SSH explorer accepts a dropped file and uploads it into the remote workspace; the dashboard's Files panel looks like that control and sits in the same position, but cannot do that one thing.
Not a goal
file_sendgate protects content leaving the workspace, the opposite direction; the reasoning is in the handler docstring.What changed (motivation → approach → change)
The Files panel gets an upload path that reuses the composer's gates instead of a second upload stack: drop files anywhere on the panel to upload them into the project root, which the drop overlay names, or pick them with Upload files… on a folder's row menu to upload into that folder. The new endpoint is a sibling of
api_upload_filethat differs only in its destination, a caller-named directory instead of the chat-attachment scratch dir.src/kiro_crew/dashboard/file_api/uploads.py(re-exported by thehandlers/files.pyfacade, routed inroutes/taskrunner.py)POST /api/directory-upload?dir=<path>. Same extension allowlist and magic-byte check asapi_upload_file; the per-file cap isdashboard.upload_max_mbfor every type, video included. Upload never replaces a file: an existing name always answers409 name_collision.website/src/api/client/files.ts,website/src/pages/chat/useDirectoryUpload.tsxStatusNotice,role="status"), then "Uploaded report.md to docs." until dismissed or the next batch; a second drop of a name still uploading into that folder is skipped, not sent. A collision shows "already exists in this folder" in the panel'sErrorNotice.website/src/pierre/PierreWorkspaceTreeImpl.tsx,tree.tsx,website/src/pages/chat/FileBrowserRail.tsx,website/src/components/ChatDropOverlay.tsxErrorNotice.website/src/i18n/locales/*,docs/feature-map/README.md,docs/system-specs/pages.chat.fileBrowserRail.*keys in every locale, the feature-map row, and thelearn-cron-dashboard.mdandmemory-skills-hooks.mdspecs.How the write stays safe.
_validate_dashboard_path/is_sensitive_pathgate/api/file-writeuses, before any body byte is read, so a part namedsecurity_policy.jsoncannot reach the crew home's protected files.pinned_fs.write_new_file_pinned, theO_EXCLcreate the steering, prompt and skill creates already share, so exactly one lands and the other getsname_collision. On Windows, which has no directory descriptor, the sameO_EXCLcreate runs by name whilepin_directoryholds the folder and its parents.pin_directory(..., links_only=True)(platform_compat_links.py, also used by the rollback inpinned_fs_files.py): a link or junction is refused 403, a OneDrive folder is pinned, any other pin error is 500.500withcode: "upload_failed", never a bare text 500.What upload refuses on purpose.
api_upload_fileadmits, the same extension allowlist and magic-byte check, so a file with no extension (Makefile,Dockerfile) or an unlisted one (.toml,.sql) answersunsupported_file_type. The workspace is where agents read and run files, and Define the Files-panel upload allowlist scope (extensionless workspace files are refused today) #13038 is deciding what the Files panel should admit there; until then this route admits nothing an existing upload route does not..,..or a name with a control character answersinvalid_filenameeverywhere. On a Windows gateway, so does a name Windows cannot store (<>:"\|?*, a trailing dot or space, a device name such asCON), before any write; a Linux or macOS gateway keeps such a name as typed, as every other tool writing into the workspace does.Backwards compatibility
Compatible: a new endpoint, new i18n keys and new optional props on the tree and rail components.
Removes nothing: no route, key, keyword or behavior main has is removed or narrowed.
Tests
test/test_handlers_files_directory_upload.py: successful write; an existing name answering409 name_collisionwith the original unchanged, including for a request that sendsoverwrite=1; the allowlist, size, magic-byte, empty-payload, invalid and sensitive-destination refusals; a path-traversal multipart name; a concurrent same-name create landing only the winner; a fresh file's mode matching what a new file in that folder gets;EACCES/ENOSPCanswering500 upload_failed, a write that fails halfway leaving no file, on both the pinned and the Windows branch; and on the Windows branch, a failed first identity probe still rolling back the empty file so a retry is not refused; on the Windows branch, a cloud-files folder accepting an upload and rolling back a failed one, a link refused 403, and an unrelated pin error answering 500.test/test_platform_compat_filesystem.pycoverspin_directory'slinks_only(default still refuses a cloud-files folder;links_onlypins it and still refuses a junction or non-directory).useDirectoryUpload(a successful batch refreshes every listing and search query that can gain the new file, including keys built from the backend's realpath; a collision shows the exists notice and never retries; errors; a later success clearing an earlier failure notice; the in-flight and success lines, their clearing on dismiss, next batch and project change; a second drop of a name still uploading sends no request),PierreWorkspaceTreeImpl.contextMenu.test.tsx(the menu item on folder rows only) andFileBrowserRail.test.tsx(the overlay names the root; a drop released over a folder row or a file row uploads into the root; the picker).FileBrowserRail.test.tsxanduseDirectoryUpload.test.tsxpass 3 repeats and 3 shuffled orders (--sequence.shuffle, seeds 11, 22, 33).Manual verification
Verified against an isolated gateway from this worktree using the offline harness (
spawn_feature_gateway+ the packaged fake ACP backend — no model, no credentials, no network), driving the real UI. The row-menu and post-upload screenshots below are from that run; the drop-overlay, name-collision and upload-failure frames are gateway-free captures of the realFileBrowserRailwith/api/**stubbed,/api/directory-uploadanswering the backend's real refusal shapes.Screenshots / video
Drop overlay active, naming the project root a drop uploads into (real
FileBrowserRail, light theme), then the directory row menu with the new action, and the tree after a successful upload:Post-upload tree
While an upload runs, and after it finishes (real
FileBrowserRail, light theme, through thedocsrow menu's Upload files…,/api/directory-uploadheld pending, then answering 200):A name collision (
409 name_collision): the panel names the file and says what to do. RealFileBrowserRail, light theme, triggered through the row menu's Upload files… with the backend's 409 stubbed:The upload-failure state (
ErrorNoticeabove the tree, naming the file and the backend refusal, with dismiss and agent hand-off):The collision frame is light mode; the other four are dark mode (the fixture's default theme).
Related Issues
Fixes #9666
Downloadburied in the viewer's overflow menu). Together these two make the Files panel bidirectional.feat(aws-control): drag and drop for the file drive, the closest precedent: same gesture, different surface.feat(files): add "Add to chat" to the file-tree row menu, the precedent for extending the row menu.Checklist
docs/system-specs/modules/learn-cron-dashboard.md,memory-skills-hooks.md)