feat: upload release assets #37
No reviewers
glm-bot-andresmgsl
kimi-bot-andresmgsl
claude-bot-andresmgsl
andres
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:ci
scope:cli
scope:docs
scope:manifests
scope:packaging
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/stoke#37
Loading…
Reference in a new issue
No description provided.
Delete branch "build/25-release-assets"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #25
Acceptance criteria
release create --asset a --asset bcreates the release and attaches both;release view --tag v1lists both with correct sizesrelease createprints the numeric release id on successrelease upload --tag <existing> --asset fattaches to a release created earlier, without recreating it--asset-namerenames a single asset, and is rejected with a usage error and non-zero exit when two or more--assetare givenheavy-duty/stokeWorklog
--asset-namecardinality before network workchangelog.d/25.mdRound log
Round at
8293c835🔧 addressing round on head
8293c83531Round analysis:
release uploadaccepts zero--assetvalues and exits successfully: agree. I will add explicit non-empty cardinality validation before config/network access and a regression test proving the usage error and non-zero exit.UPLOAD_TIMEOUT_MSlacks a why-comment: agree. I will document why uploads use their own ten-minute budget.I will update the PR Worklog with these fix steps, make all changes while the PR is draft, run focused and full verification, post one whole-round reply, then signal the exact complete head before marking ready. I will not request reviewers; the engine owns that transition.
Round 1 answered whole at head
3c0709189e.Changes pushed in
3c07091:release uploadnow explicitly rejects an empty asset list before config/client/network access; the regression test proves exit 1, a usage diagnostic, and no authentication fallback.UPLOAD_TIMEOUT_MSnow explains why release assets use a separate ten-minute budget.Points answered without code changes:
Fresh verification on
3c0709189e:npm test: 108/108 passed.npm run check:governance: 4 identities resolved; 5 scope rows valid.npm pack --dry-run, release create/upload help smokes, andgit diff --check: passed.origin/build/25-release-assets; same-head Forge CI and labels checks report success.origin/mainatc09943ea32andgit merge-treeis clean; the transient draft-only conflict label is not supported by Git.📣 round answered at head
3c0709189eRound at
3c070918Round passed with no written reply.
Build complete at head
8293c83531.Implemented:
Verified on this exact head:
📣 round answered at head
8293c83531WIP: feat: upload release assetsto feat: upload release assets🔎 reviewing head
3c0709189eProgress:
3c070918in detached worktree /home/dev/duty/trees/heavy-duty__stoke/review-37.Approved.
Reviewed against #25 acceptance criteria:
release create --asset <a> --asset <b>creates the release, uploads both assets via multipart, prints the numeric release id, andrelease view --taglists assets with sizes.release upload --tag <existing> --asset <f>resolves the tag once and attaches without recreating.--asset-nameoverrides the filename for a single asset and is rejected (exit 1) when paired with multiple--asset.fs.openAsBlob/FormDatarather than buffering.Verified:
node --test: 106/106 passed (afternpm installin the worktree).npm run check:governance: 4 identities, 5 scope rows valid.git diff --checkclean.npm pack --dry-runsucceeds.release create,release upload, andrelease viewoptions.Unverified / relied on reading:
build/25-release-assets).No load-bearing constraints from this repo's CONTRIBUTING.md (none present) or ceremony's doctrine are violated.
🔎 reviewing head
3c0709189eRound 2 (re-request 11:17Z postdates my request-changes on
8293c83). Whole PR re-reviewed at this head in a detached worktree.release uploadnow exits 1 with a usage error and makes zero requests (also with no config file at all)8293c83, green hererelease viewlists both with sizesrelease createprints the numeric idrelease uploadattaches to an existing tag, exactly one lookup--asset-namerenames one asset; rejected before network with 2+Verdict: approve, with non-blocking nits.
🔎 reviewing head
3c0709189e3c07091in detached worktreerelease uploadnow exit 1 + usage error + zero requests before config (probe); ENOENT names full path (probe); UPLOAD_TIMEOUT_MS why-comment present8293c83src, pass on3c07091Approved at head
8293c83531.Reviewed the whole PR (4 commits, 6 files) against issue #25's acceptance criteria:
release create+release viewsizes: the suite pins the exact request sequence (POST release, then one assets POST per file, multipart) and view's asset lines; I also ranrelease view --tag 0.1.0against the live forge (heavy-duty/crew) — the real asset renders with correct size and download URL.Release id: 42).release uploadresolves the tag once (single GET releases/tags/{tag}) and never POSTs /releases: pinned by test.--asset-name: renames a single asset (test + stub probe); with two or more (and with zero)--assetit exits 1 with a usage error before config or network — verified by hand, including the before-config ordering the test asserts via the absence of "Not authenticated".Could not verify / relied on instead: the live write path (
release create/release uploadagainst the real forge) — review is read-plus-comment work, so I did not create releases on shared repos; I relied on the issue's sanctioned stubbed-HTTP test plan (106/106 tests pass,check:governanceclean), my own local stub probes, and a read-only liverelease view. A literal >30 s wall-clock upload was not run; relied on the pinned timeout test plus reading the mechanism (fs.openAsBlob + FormData streamed by fetch, no manual Content-Type, no whole-file JSON.stringify).Non-blocking nits, at the builder's discretion: a missing local asset file is only reported after the release exists (the spec decides only cardinality validation before network work, so this is within spec — a pre-flight existence check would be friendlier);
UPLOAD_TIMEOUT_MSlacks the why-comment its siblingMIGRATE_TIMEOUT_MScarries; the ENOENT asset message ("Unable to open file as blob") is opaque.Same-repo branch
build/25-release-assetson heavy-duty/stoke confirmed (not a fork). Changelog line present (changelog.d/25.md); README documents --asset, --asset-name, release upload and the printed id, matching the behavior I probed.Request changes — one blocking defect at
8293c835313d0f2eda354c1954c6e258c3db7863Reviewed the whole PR at this head in a detached worktree. Six of the seven
acceptance criteria on #25 hold and I verified each of them by running the
code; the seventh (
--asset-namecardinality) holds forrelease createbutthe sibling
--assetrequirement on the newrelease uploadis not enforcedat all, and its failure mode is a silent zero-exit success.
Blocking:
release upload --assetis declared and documented as required, but is notsrc/cli.js:1149Commander's mandatory check is
getOptionValue(name) === undefined. The[]default satisfies it, so the option is never actually required.
--helpsaysso out loud on this head:
Measured against a stub forge (config pointed at a local server, exit status
and requests captured):
Exit 0, no output at all, one wasted tag lookup, nothing uploaded. The README
this PR adds states the opposite contract:
Why this blocks rather than rides an approval: #25 exists because release
mirroring had to be finished by hand, and the shape it is replacing is a
script. A typo (
--assets), or a glob that expands to nothing, now reportssuccess and attaches nothing — the one outcome a calling script cannot detect,
in the command whose entire job is attaching files. It is also the only
requiredOptioninsrc/cli.jscarrying a default; the other 40-odd areclean, so this reads as an accident rather than a decision.
What unblocks: make the emptiness fail. Either drop the
[]default soCommander's mandatory check fires (the collector then needs
(v, prev) => (prev || []).concat(v)), or keep.option(...)and reject anempty list in
validateAssetOptionsalongside the--asset-namerule. Eitherway with a test asserting non-zero exit and that no request is made — the
REQSline above is the part that must go to[].Non-blocking — the "does not buffer the whole file" claim is only half proven
The head does satisfy the criterion's operative half: the file is read lazily
and under backpressure, not slurped. I confirmed that separately from the
suite — with a deliberately slow receiver (~10 MB/s) uploading a 300 MB asset,
bytes were read from disk no faster than the server consumed them.
But resident memory still scales 1:1 with the asset. Sampling the uploader as
a child process (so the stub server's own buffers are excluded), forcing
global.gc()every second,arrayBuffersclimbs monotonically and tracksbytes already sent:
Live after a forced GC means retained, not uncollected. A 400 MB asset peaked
at 476 MB RSS.
I chased this to make sure it was not a defect in the chosen shape, and it is
not. Cross-process, on this Node (v22.23.2), all three fetch body strategies
cost the same for a 200 MB file:
So
fs.openAsBlob+FormDatais the right call and the residency is belowthe line this PR draws — escaping it means leaving
fetchforhttp.request,which is not this issue's scope. Nothing to change here. Two smaller notes:
changelog.d/25.md("stream asset uploads") and the build comment ("does notbuffer the whole file") both read as stronger than what is proven, and at
#25's motivating sizes (29 MB / 50 MB) the cost is immaterial. If constant
memory ever matters, that is a discussion, not a fix in this round.
Non-blocking nits
release uploadhas no test for--asset-namewith two assets; onlyrelease createcovers it. I ran the case by hand and it does reject(
status=1,--asset-name requires exactly one --asset., no request made),so this is missing coverage of working behaviour, not a bug.
--assetsurfaces asAsset failed: <dir>: Network error reaching <url>: fetch failed— theEISDIRcause is dropped. That wrapper shape is inherited from the existingrequest(), so it is pre-existing, not introduced here.?name=is built withURLSearchParams, which encodes a space as+(the suite asserts
name=custom+name.bin). That is correctform-urlencoded and Forgejo should decode it, but
encodeURIComponentwould be unambiguous. See the unverified list below.
What I verified, and how
Ran on this exact head, in a detached worktree, after
npm ci:npm test— 106/106 pass.npm run check:governance— 4 identities, 5 scope rows, pass.ci / testandlabels / labelsboth green.release create(both assets attached,
Release id: 42printed, multipart content type,bodies carry the file bytes);
release uploadonto an existing tag (exactlyone
GET .../releases/tags/v1, then the asset POST);--asset-namewith twoassets on
createand onupload(both exit 1 before any network call);--asset-namewith zero assets; a missing asset path (release kept, idprinted, exit 1); a directory as
--asset;release createwith no assetsat all (unchanged, exit 0); and the no-
--assetrelease uploadcase above.Closes #25against the issue: #25 carries no post-merge criterion, sothe closing keyword is the right shape here.
.github/labels.confpanel=lists me and the author iscodex-bot-andresmgsl, so this verdict is a gate, not advisory.Could not verify, and what I relied on instead:
http.createServerstub. So the acceptance criterion's live half —release view --tag v1listing both assets with sizes the forge reports,and the forge accepting
?name=first+second.binwith a+— rests on thestub and on reading the endpoint contract in #25, not on observation. #25's
manual proof block against
heavy-duty/scratchis still owed by somebodywith write access.
suite asserts
AbortSignal.timeoutreceives600000on the upload path, andI confirmed a 400 MB upload completes. No upload here actually ran past 30 s
on loopback.
memory numbers above are this version's.
Everything else in the round is in good shape — the partial-failure semantics
match the spec exactly (release kept, every asset still attempted after a
failure, both lists named, non-zero exit), and the failure-first test genuinely
fails for the right reason. Fix the
--assetrequiredness and I expect toapprove.
🔧 addressing round on head
8293c83531Round analysis:
release uploadaccepts zero--assetvalues and exits successfully: agree. I will add explicit non-empty cardinality validation before config/network access and a regression test proving the usage error and non-zero exit.UPLOAD_TIMEOUT_MSlacks a why-comment: agree. I will document why uploads use their own ten-minute budget.I will update the PR Worklog with these fix steps, make all changes while the PR is draft, run focused and full verification, post one whole-round reply, then signal the exact complete head before marking ready. I will not request reviewers; the engine owns that transition.
feat: upload release assetsto WIP: feat: upload release assetsRound 1 answered whole at head
3c0709189e.Changes pushed in
3c07091:release uploadnow explicitly rejects an empty asset list before config/client/network access; the regression test proves exit 1, a usage diagnostic, and no authentication fallback.UPLOAD_TIMEOUT_MSnow explains why release assets use a separate ten-minute budget.Points answered without code changes:
Fresh verification on
3c0709189e:npm test: 108/108 passed.npm run check:governance: 4 identities resolved; 5 scope rows valid.npm pack --dry-run, release create/upload help smokes, andgit diff --check: passed.origin/build/25-release-assets; same-head Forge CI and labels checks report success.origin/mainatc09943ea32andgit merge-treeis clean; the transient draft-only conflict label is not supported by Git.WIP: feat: upload release assetsto feat: upload release assetsfeat: upload release assetsto WIP: feat: upload release assets📣 round answered at head
3c0709189eWIP: feat: upload release assetsto feat: upload release assetsApproved at head
3c0709189e(re-review of the whole PR, 5 commits / 6 files; my previous approval covered8293c83).Round-1 follow-ups verified at this head:
release uploadwith zero--assetnow exits 1 with a usage error before config or any network work — I probed it against a stub forge (status=1,At least one --asset is required., zero requests) and ran the new test red/green: it fails on 8293c83's src and passes on3c07091.Could not read asset file <full-supplied-path>: Unable to open file as blob(api test + hand probe on both create and upload paths), andUPLOAD_TIMEOUT_MScarries its why-comment.Acceptance criteria of #25 re-checked at this head, each by test plus my own stub-forge probes:
release create+release viewsizes/URLs: suite pins the exact request sequence (POST release, then one assets POST per file, multipart with boundary, file bytes on the wire) and view's asset lines.Release id: 42).release uploadresolves the tag once (exactly oneGET .../releases/tags/{tag}), never POSTs the release.--asset-namerenames a single asset; with two or more--assetit is a usage error before config/network on both create (test) and upload (probe); on upload, zero assets is now likewise rejected before anything.UPLOAD_TIMEOUT_MSpinned instead of the 30 s JSON budget, 8 MiB upload driven through a paused/resumed receiver (backpressure). Same-repo branchbuild/25-release-assetson heavy-duty/stoke confirmed from the PR API.Also ran at this head: full suite 108/108,
npm run check:governanceclean,git diff --checkclean; forge checksci / testandlabels / labelsgreen on this exact head.Could not verify / relied on instead: the live-forge write path (
release create/release uploadagainst the real instance) — review is read-plus-comment work and I did not create releases on shared repos; I relied on the issue's sanctioned stubbed-HTTP test plan, my own local stub probes, and last round's read-only liverelease view(heavy-duty/crew 0.1.0). A literal >30 s wall-clock upload was not run; relied on the pinned-timeout test, the backpressure test, and reading the mechanism (fs.openAsBlob + FormData streamed by fetch, no manual Content-Type, no whole-file buffering in the client). Local Node is v22.12.0; same-head forge CI is the cross-check.Non-blocking nits, at the builder's discretion:
release upload's.requiredOption('--asset <path>', …, [])still carries its[]default, so help shows(default: [])and Commander's own required-machinery stays inert — the explicitvalidateAssetOptionscheck is what enforces it (correctly; consider plain.optionto avoid the misleading help); still no dedicated test for--asset-namewith two assets onrelease upload(I probed it: exit 1, usage error, no requests).Changelog line present (changelog.d/25.md); README documents
--asset,--asset-name,release upload, and the printed id, matching probed behavior. Panel:.github/labels.confpanel=includes me; author is codex-bot-andresmgsl — on-panel, so this verdict is a gate, not advisory.Approved at head
3c0709189e.Reviewed against issue #25 acceptance criteria:
release create --asset <a> --asset <b>creates the release and uploads both via multipart;release view --taglists assets with sizes.release createprints the numeric release id.release upload --tag <existing> --asset <f>resolves the tag once and attaches without recreating.--asset-nameoverrides the filename for a single asset and rejects with exit 1 when paired with multiple--asset(release createis covered by the suite;release uploadsharesvalidateAssetOptionsand I probed it by hand).fs.openAsBlob+FormDatawithout forcingapplication/json.Round-1 follow-up verified:
release uploadwith zero--assetnow exits 1 with a usage error before config or any network work.Verification run on this head in a detached worktree:
npm ci+npm test: 108/108 pass.shellcheck scripts/*.sh: clean.npm run check:governance: 4 identities resolved; 5 scope rows valid.release create,release upload, andrelease view.Could not verify against the live forge; I relied on stubbed HTTP tests and code reading for upload semantics and endpoint shapes.
Approve — the round-1 blocker is fixed, and I closed my own verification gap
Reviewed the whole PR at
3c0709189e9ad3bf2983206820b922c7052150dcin adetached worktree, not just the fix commit. All seven acceptance criteria on
#25 now hold and I verified each by running the code.
The blocking finding is genuinely fixed
release uploadno longer accepts an empty asset list.validateAssetOptionsgained a
requireAssetgate that runs beforeloadConfig:REQSis empty — the wasted tag lookup I measured on8293c83is gone. Ire-ran it with
STOKE_CONFIG_FILEpointing at a nonexistent file and got thesame exit 1 and the same message, so the check really does precede config and
client construction rather than happening to short-circuit on a loaded config.
Both new tests fail for the right reason. Checking out
8293c83'ssrc/underthis head's
test/:Restored to this head, 108/108 pass.
I closed the 30-second gap I named last round
Last round I could only prove the upload timeout structurally — the suite
asserts
AbortSignal.timeoutreceives600000, but nothing had actually runpast 30 s. I built the case this time. A stub receiver that takes the whole
40 MB body and then withholds its response for 33 s:
Control on the same stub, through the ordinary JSON client:
The JSON path dies at 30.0 s; the upload path returns at 33.2 s. The criterion
is now proven by elapsed time, not only by an asserted constant.
The other two round-1 points, as answered
UPLOAD_TIMEOUT_MSwhy-comment — present, and it says why (assets arelarger than JSON payloads) rather than what.
Could not read asset file /tmp/.../missing.bin: Unable to open file as blob. The full supplied path is named, and therelease is still kept with the remaining assets attempted. The
assert.matchon an escaped absolute path in the api test is the right shape.
author's disposition on both. Preflighting local files would contradict the
criterion that a failed upload leaves the release in place with every later
asset still attempted, and I said last round that the residency is inherent
to
fetchon Node 22 rather than to this shape.Acceptance criteria, each exercised against a stub forge
multipart/form-data, file bytes on thewire,
Release id: 42printed;release viewlists both with sizes and URLs.release upload --tag v1: exactly oneGET .../releases/tags/v1, then onePOST per asset.
--asset-namewith one asset renames (?name=renamed.bin); with two itexits 1 before any request, on both
createandupload.good.binstill attempted afterbad.binfailed, both named, exit 1.
release does not exist+HTTP status: 404with noupload attempted;
release createwith no assets is unchanged (exit 0).build/25-release-assetsonheavy-duty/stoke, not a fork.Closes #25is the right keyword: #25 carries no post-merge criterion.changelog.d/25.mdis present.git diff --checkclean. Forge checks on thishead:
ci / testandlabels / labelsboth success. Panel membership from.github/labels.confincludes me and the author iscodex-bot-andresmgsl, sothis verdict is a gate, not advisory.
Non-blocking nits — the builder's discretion
release uploadstill declares.requiredOption('--asset <path>', …, collectOption, []).The
[]default defeats Commander's mandatory check, so therequiredpartis dead weight — the enforcement is entirely
validateAssetOptions, which isfine and better tested. But
--helpprints--asset <path> asset to upload (repeatable) (default: [])while the README block calls it(required, repeatable). Demoting it to.option(...)would make thedeclaration honest without changing a single behaviour.
--assetstill reportsAsset failed: <dir>: Network error reaching <url>: fetch failed— theEISDIRcause is dropped, becauseopenAsBlobsucceeds on a directory andthe failure only surfaces when
fetchreads the body. The new try/catchcan't catch this one. Pre-existing wrapper shape; not introduced here.
release uploadhas no test for--asset-namewith two assets (onlyrelease createdoes). I ran it and it rejects correctly, so this ismissing coverage of working behaviour, not a bug.
?name=is built withURLSearchParams, so a space encodes as+. Correctform-urlencoded, and the suite pins
name=custom+name.bin;encodeURIComponentwould be unambiguous. See below.What I could not verify, and what I relied on instead
http.createServerstub. So the live half of criterion 1 — the forgereporting asset sizes back through
release view, and Forgejo decoding?name=first+second.binwith a+— rests on the stub and on the endpointcontract written into #25, not on observation. #25's manual proof block
against
heavy-duty/scratchis still owed by somebody with write access; Idid not run it, because a reviewer mutating a shared repository is not mine
to do.
pinned 600 s value end-to-end. That constant is covered by the unit test only.
and memory behaviour above is this version's.
The
state:addressinglabel is stale now that the round is answered; thereconciler owns that, not me. Nice work on the fix — the zero-asset case fails
exactly where it should, before anything is touched.