feat(storage): browse, download, and delete a bucket's objects - #90
Conversation
Adds an `insta storage` group over the three new platform object routes: insta storage list [--prefix <p>] [--cursor <c>] [--limit <n>] [--service <n>] [--branch <b>] [--json] insta storage get <key> [-o <file>] [--service <n>] [--branch <b>] [--json] insta storage delete <key> [-y] [--service <n>] [--branch <b>] [--json] `get` resolves a short-lived presigned URL and pulls the bytes straight from the provider, so nothing large streams through the control plane; it writes to the key's last segment unless `-o` names a file. `delete` is irreversible and runs immediately (a data operation, not staged infrastructure), so on a terminal it confirms first — `-y` skips the prompt and a non-TTY proceeds. `resolveComputeServiceId` is generalized into `resolveSoleService` so storage gets the same named-or-sole resolution with identical messages.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Adds a well-scoped insta storage {list,get,delete} command group that wraps the three platform object routes from insta-platform#215, with pure/DI'd seams, strong test coverage, and behaviour consistent with the rest of the CLI.
Requirements context
No spec/plan exists in this repo (docs/superpowers/ and docs/specs/ are both absent). The design of record lives in the insta-cloud repo at docs/superpowers/specs/2026-08-12-storage-file-browser-design.md (referenced in the PR body) and the wire contract is defined by insta-platform#215, which is not yet on the platform's main. I assessed against the PR description, the referenced dependency, and existing CLI conventions. Verified locally: npm run typecheck is clean and npx vitest run passes 27 files / 259 tests (matches the PR's stated numbers).
Findings
Critical
(none)
Suggestion
- Functionality —
res.bodyis dereferenced without a null guard on success paths.src/commands/storage.ts:61(res.body.objects ?? []),:66(res.body.nextCursor) and:106(saveObject(res.body.url, …)). The API layer parses an empty response body tonull(src/api.ts:79-82), so a2xxwith an empty/non-JSON body would throw a rawCannot read properties of nullrather than a clean CLI error. Low blast radius — the list/download routes are expected to return JSON objects — but a defensive(res.body ?? {}).objects/ an explicitif (!res.body?.url) throw new Error(...)would degrade gracefully. (Thedeletenon-JSON path at:120is already safe since it doesn't touchres.body.)
Information
- Functionality — merge-order and server-side contract. The PR correctly documents that it must merge after insta-platform#215. Two things this CLI cannot verify on its own and that depend on that PR landing as-designed: (1) the presign/list/delete routes at
objects/objects/download, and (2) the gate action namesstorage.read/storage.deletenewly advertised in thepolicy sethelp text (src/index.ts:273).policySetpasses the action straight through to the platform (src/commands/govern.ts:46-50), so if the platform names those gates differently the help text becomes misleading (not broken). Worth a final cross-check against the merged platform PR. - Functionality — default
storage getoverwrites silently.outputPath(src/commands/storage.ts:72-76) writes the key's last segment into cwd, so two keys sharing a basename (a/logo.png,b/logo.png) or a re-run clobber the earlier file without warning. This matches typical download-tool behaviour (curl -O) and-ois the escape hatch, so it's fine as-is — just noting it.
Notes on the four dimensions
- Software engineering — Strong. 21 new unit tests exercise the pure builders (encoding of
&/#/space/non-ASCII keys, cursor round-trip, fixed-width alignment, the deliberate—for omitted fields), the limit validator's range + junk rejection, traversal safety, and the fetch→write core (including the "write nothing on failure" case). TheresolveComputeServiceId → resolveSoleService<T>generalisation preserves the exact existing error messages and the pre-existingresolveComputeServiceIdtests still pass, so compute is regression-covered.fmtBytesis reused fromdb.tsrather than copied, and import style (.jssuffixes,node:prefix) is consistent. - Security — No concerns. Default output path uses only the key's last segment, so a hostile key (
../../etc/passwd,/etc/passwd) is neutralised topasswd— tested. Keys travel URL-encoded viaURLSearchParams, never as path segments (verified for both the DELETE and download routes), so no path/query injection. Bytes are pulled straight from the provider's presigned URL, never proxied through the platform. No secrets are logged; the--jsonURL surfacing is an intentional, documented escape hatch mirroringinsta secrets --json. No new dependencies. Unprompteddeleteis consistent with every other destructive command here and leans on the governance gate + help-text warning. - Performance — No concerns. Each command is one
serviceslist call plus one route call; no N+1, no unbounded loops, downloads stream from the provider rather than through the control plane.
Verdict
approved (informational — a human still gives the explicit GitHub approval via the approve flow). No Critical findings; the one Suggestion (null-body guarding) and the Information notes are non-blocking. Please just confirm the platform gate action names / routes line up once insta-platform#215 merges.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ters Six findings from review, all valid. `outputPath` split on `/` only, so a key holding backslashes kept them and, on Windows, `..\..\Windows\...\hosts` would have escaped the working directory. It now splits on both separators — the traversal guard has to cover the platform the CLI actually runs on. Downloads buffered the whole object through `arrayBuffer()` before writing, which kills the process rather than producing a file once an object outgrows memory. `streamPresignedTo` pipes the provider's body straight to disk and counts bytes on the way through, and removes the partial file if the stream fails — a truncated file must not pass for a finished download. The DI seam moves from fetch-bytes/write to stream-to-path accordingly. The "next page" hint printed a bare `--cursor`, so following it after `--prefix docs/` paged through a different set of objects. `nextPageCommand` repeats every filter that shaped the page. Also: `res.body` is dereferenced defensively now that an empty 2xx body parses to null; `--json` no longer requires a key it can infer a filename from, since it writes nothing; and the test's `mkdtempSync` directory is removed rather than leaked once per run. `policy set` help lists `storage.write`, which the merged platform now advertises alongside read and delete.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Both from review, and the first one is a regression the streaming change introduced. `createWriteStream(out)` truncates on open, so `-o important.pdf` on a download that then failed left the cleanup deleting the user's existing file. The buffered version it replaced never touched the target until it had all the bytes, so the streaming fix traded a truncated file for data loss. Bytes now land in a `.insta-part-<rand>` file beside the target and are renamed over it only once the pipeline completes; the part file is what gets removed on failure. Same directory, so the rename is atomic rather than a cross-device copy. Two tests pin it: an existing target still holds its original contents after a mid-stream failure with no part file left behind, and a successful download replaces it. `nextPageCommand` interpolated values raw, so a prefix holding a space or `&` printed something that is not a runnable command. Values a shell would reinterpret are single-quoted now, embedded quotes included.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…rl-C Two more from review, both consequences of writing to a part file and renaming. The part file is created at the umask default, so renaming it over a `0600` target silently widened that file to `0644`. It now inherits the target's mode before the rename, when a target exists. A test pins it: replacing a `0600` file leaves it `0600`. Ctrl-C kills the process without unwinding, so the `catch` never ran and a `.insta-part-*` stayed behind in the user's directory — worse than a temp dir because that is where they are working. SIGINT and SIGTERM now remove it synchronously and exit 130; the handlers are detached in a `finally` so repeated downloads do not stack listeners.
|
On the PowerShell quoting finding — fixed the other two, deliberately not this one. Generating the hint with the target shell's rules means detecting the shell, and that detection is the unreliable part: Worth noting where the bar actually is: this repo's existing suggested-command renderer, If cross-shell hints are wanted, the right shape is one shared helper for every command that prints one, not a second convention inside |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Both from review. The sweep exited 130 for SIGTERM as well as SIGINT, so a supervisor that reads 143 as terminated and 130 as interrupted would misreport a download killed by systemd or CI as someone pressing Ctrl-C. The handler is now per-signal and exits 128 plus the signal number. The mode-preservation test asserts POSIX permission bits. Windows chmod only toggles the read-only attribute and stat reports 0o666 for any writable file, so that assertion cannot hold there — and CI is ubuntu, so it would only ever fail on a contributor's machine while this CLI ships a Windows binary. Skipped off POSIX rather than weakened, since the behaviour it pins is real where modes exist.
The page hint quoted POSIX-style, so a prefix holding an apostrophe printed Bash's '\'' escape — which PowerShell cannot parse. Per-shell quoting was the obvious fix and the wrong one: detecting the shell is the unreliable part, and a wrong guess prints something broken in a different way. So the hint no longer needs quoting. Every value it would interpolate is checked against a conservative shell-safe pattern; if any of them fails, the hint becomes a sentence naming the cursor instead of a command. Base64 cursors carry only `+ / =`, which are safe unquoted, so the copy-pasteable form survives for the common case — a prefix with a space or quote is what trades it for prose.
2 participants
What
Adds an
insta storagecommand group so a bucket's objects are reachable from the CLI, not only from the console.Until now
insta services add storage <name>handed you a bucket and stopped there: nothing in the CLI could list what was inside it, fetch a file, or remove one. That gap is why the platform work exists — see the design atinsta-cloud/docs/superpowers/specs/2026-08-12-storage-file-browser-design.md.Depends on InsForge/insta-platform#215, which adds the three endpoints these wrap. They do not exist on the platform's main yet, so merge this after that.
How
src/commands/storage.ts(new) — the three commands, with the pure/injected seams split out so they can be unit-tested without a platform:objectsPath,objectDownloadPath,parseObjectLimit,objectListLine,outputPath,fetchPresigned,saveObject.src/index.ts— registers the group afterdb, and addsstorage.read/storage.deleteto thepolicy setaction list.src/commands/services.ts— generalisesresolveComputeServiceIdintoresolveSoleService(services, type, name?)with identical messages; compute now delegates to it, so storage gets named-or-sole resolution without a second copy.src/util.ts— no new prompt helpers; see the delete note below.Details worth reviewing:
storage getwrites the key's last segment, never a path. That is what makes a hostile key harmless —../../etc/passwdsaves aspasswd, and nothing can escape the working directory.-ooverrides.--jsonprints{url, expiresAt}and downloads nothing, mirroringinsta secrets --json, which also gives you ajq -r .url | curlescape hatch.storage deletedoes not prompt. No destructive command in this CLI does —project delete,branch delete,services removeandcompute volume --deleteall execute immediately and lean on the governance gate instead. A prompt here would have made this the one command that behaves differently, and a TTY-only prompt would have split behaviour between humans and the agent/CI path this CLI is built for. The help text carries the warning, ascompute volume --deletedoes.--limitparses through a throwing validator. A bareNumber()would sendNaNto the server; the repo already learned that fromparseCpu.--service, not--group.insta dbuses--groupfor the same job, but "group" is a Fly machine term with no meaning for a bucket.Command surface is mirrored in
insta-skills(insta/cli-reference.md) per this repo's non-negotiable #4 — that is a separate PR in that repo.Verify
23 new tests in
test/storage.test.tscover the path builders (including keys with&,#, spaces and non-ASCII), the limit validator's range and junk rejection, the listing row's fixed-width alignment and its refusal to fake a zero for a field the platform omitted, output-path defaulting and traversal safety, and the fetch→write core with a failure that must write nothing.Because the platform routes are still in review, the commands were also driven end to end against a throwaway fake platform on localhost with
INSTA_API_URL+INSTA_PROJECT_ID, confirming the real wire paths:and the error paths: the 202 gate printing the approvals hint, a 403 on an expired presigned URL naming the ~60s TTL as the likely cause,
--limit 9999rejected locally before any request, and an unknown--servicenaming the missing service.Summary by cubic
Adds an
insta storagecommand group to browse, download, and delete a bucket’s objects from the CLI. Previously you could only create buckets; now you can list contents, fetch objects safely, and remove them with governance gates.insta storage list [--prefix <p>] [--cursor <c>] [--limit <n>] [--service <name>] [--branch <b>] [--json],insta storage get <key> [-o <file>] [--service <name>] [--branch <b>] [--json],insta storage delete <key> [--service <name>] [--branch <b>] [--json]..insta-part-*file and are renamed over the target only on success; partial files are removed on failure and also swept on Ctrl‑C/SIGTERM (process exits 128+signo). If replacing an existing file, its mode (e.g., 0600) is preserved.getwrites to the key’s last segment by default (slashes and backslashes treated as separators to prevent traversal).--jsonprints{url, expiresAt}without downloading.deleteexecutes immediately without a prompt; governance gates apply.--limitvalidates 1..1000 locally. The “next page” hint repeats all filters and prints a copy‑pasteable command only when values are shell‑safe; otherwise it falls back to a sentence instructing you to re‑run with--cursor.--serviceresolves a named or sole storage service on the branch.resolveComputeServiceIdis generalized toresolveSoleService; compute delegates to it (messages unchanged).policy sethelp now includesstorage.read,storage.write, andstorage.delete.InsForge/insta-platform#215. No migrations required.Written for commit 97ce6d1. Summary will update on new commits.