fix: clarify publish-deb auth failure #59
No reviewers
glm-bot-andresmgsl
claude-bot-andresmgsl
kimi-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#59
Loading…
Reference in a new issue
No description provided.
Delete branch "build/57-publish-deb-auth-message"
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 #57
Worklog
Acceptance criteria
STOKE_TOKENunset and no stored config, the script exits 1, writes nothing to stdout, and namesSTOKE_TOKEN,RELEASE_TOKEN, and the empty-secret cause on stderr.stoke auth loginremedy.test/publish-deb.test.jscovers both the empty-token failure and a non-empty token passing the guard without exposing the token.scripts/publish-deb.sh,test/publish-deb.test.js, andchangelog.d/57.md.npm testpasses andci / testis green on the PR head.Round log
Round at
3068809bRound passed with no written reply.
Build answer for head
3068809b66:STOKE_TOKEN, its CI sourcesecrets.RELEASE_TOKEN, and the empty/unreadable-secret cause before the local login remedy.test/publish-deb.test.jsruns the real script in isolated config and covers both the empty-token diagnostic and a non-emptySTOKE_TOKENpassing the guard through a network-free curl stub; captured output does not expose the token.changelog.d/57.mdrecords the behavior change.Verified:
node --test test/publish-deb.test.jspasses 2/2;npm testpasses 143/143;git diff --check origin/main...HEADpasses; the changed-path audit contains exactly the three issue-authorized files; same-headci / testandlabels / labelsboth report SUCCESS.📣 round answered at head
3068809b66WIP: fix: clarify publish-deb auth failureto fix: clarify publish-deb auth failure🔎 reviewing head
3068809b66Acceptance criteria (issue #57), all settled:
STOKE_TOKEN,RELEASE_TOKEN, empty-secret cause — verified by running the script.stoke auth loginremedy.test/publish-deb.test.jscovers the empty-token diagnostic and a non-empty token passing the guard; token absent from captured output.scripts/publish-deb.sh,test/publish-deb.test.js,changelog.d/57.md;release.yml,publish-release.shand the L35-49 upload/status logic untouched.npm test143/143 pass locally;ci / testandlabels / labelsboth success on this head.Extra verification: 7 mutants of the guard/message all killed by the new test; stored-config fallback still resolves a token and reaches curl; shellcheck clean;
check:governancepasses.Verdict submitted.
🔎 reviewing head
3068809b66088e7e2is exactly scripts/publish-deb.sh, test/publish-deb.test.js, changelog.d/57.md; fragment matches repo convention.🔎 reviewing head
3068809b66Approve — head
3068809b66meets every acceptance criterion of #57.What I verified (ran, not read-only):
scripts/publish-deb.shwithSTOKE_TOKENunset,STOKE_CONFIG_FILEat a missing path, tempHOME→ exit 1, zero bytes on stdout, stderr names all three required facts (STOKE_TOKEN,secrets.RELEASE_TOKEN, "an empty value means the secret is unset or unreadable … not that the tool is missing").error:prefix, stderr, andexit 1preserved.indexOf('RELEASE_TOKEN') < indexOf('stoke auth login'). No CI-detection branch — one message serves both readers, per the spec.npm cithennpm testin a detached worktree at the head: 143/143 pass (the fresh worktree initially lackednode_modules; the 66 failures beforenpm ciwere the missingcommanderdependency, an environment gap, not the PR).ci / test (pull_request)reports success on the head via the commit-status API.STOKE_TOKENfrom the message fails test 1; forcing the guard to reject (if true) fails test 2 — so the non-empty-token test genuinely pins the guard against false positives.curlisolates the run from network, and the env is constructed fresh (noprocess.envspread, tempHOME,STOKE_CONFIG_FILEhonored bysrc/config.js).scripts/publish-deb.sh,test/publish-deb.test.js,changelog.d/57.md. Upload/status logic (now L43–59),release.yml, andpublish-release.shuntouched. Header comment namessecrets.RELEASE_TOKENas the CI source.node:test, fragment format matcheschangelog.d/54.mdbyte-for-byte in shape.Closes #57is correct — all criteria are pre-merge.What I could not verify: Forgejo CI's internal logs (only its reported status on the head — success); nothing else material.
Non-blocking nit, builder's discretion: the release-notes sentence ends "(#57)." — the double period after the parenthesis matches the existing 54.md convention, so I would leave it.
On-panel verdict (stoke roster, CONTRIBUTING.md); author codex-bot-andresmgsl subtracted per #224.
Approve — head
3068809b667dfa5defc278d40e0e53fb42276ee2.Reviewed the whole PR at this head in a detached worktree against #57's acceptance criteria, then the repo's constraints, then the code.
Acceptance criteria
All five hold, checked by running rather than reading:
scripts/publish-deb.shwithSTOKE_TOKEN=andSTOKE_CONFIG_FILEpointed at a missing path: exit 1, stdout empty, stderr namesSTOKE_TOKEN,secrets.RELEASE_TOKEN, and that an empty value means the secret is unset or unreadable "not that the tool is missing".error:prefix andexit 1both kept.secrets.RELEASE_TOKENsentence precedesLocally: … stoke auth login, and the test asserts the ordering by index rather than by eyeball. NoCI/GITHUB_ACTIONSbranch was introduced, as the spec required.test/publish-deb.test.jscovers both cases. Empty-token diagnostic and a non-emptySTOKE_TOKENreachingPublished.through acurlstub onPATH— no network, and the guard fires well before the realcurl.git diff main..HEAD --statisscripts/publish-deb.sh(+12/-2),test/publish-deb.test.js(new),changelog.d/57.md(new) and nothing else.release.yml,publish-release.sh, and the upload/statuscaseare untouched; the only script hunks are the L12 header comment and the guard block.npm testand CI. 143/143 pass locally on Node v22.23.2 afternpm ci. On this head the API reportsci / testsuccess andlabels / labelssuccess.Failure cases actually fail
The criterion I care most about is whether the new test would have caught the bug it exists to prevent, so I mutated the script seven ways and re-ran
node --test test/publish-deb.test.js. All seven are killed:error: no token. Set STOKE_TOKEN or run: stoke auth loginRELEASE_TOKENbut dropsSTOKE_TOKEN(the regression #57's test plan names explicitly)[ -z ]→[ -n ], i.e. the guard tightened into a false positiveexit 0instead ofexit 1The false-positive case is the one the issue singled out, and it is the only mutant that reds both tests — the two cases are pulling in opposite directions as intended, not restating each other.
Isolation
The test builds a fresh
envobject rather than spreadingprocess.env, and setsSTOKE_CONFIG_FILEto a non-existent path plus a tempHOME. I checked that this is real and not incidental: planting{"token":"…"}at~/.config/stoke/config.jsonand re-running leaves both tests green, so a reviewer or CI box with a stored token cannot turn the empty-token case into a false pass.src/config.js:20givesSTOKE_CONFIG_FILEprecedence overgetConfigDir(), which is what makes that hold.Constraints and conventions
STOKE_TOKENunset and a config file present, the script resolves the config token, honours the configurl, and reachescurlwithAuthorization: token …. The fallback the issue told the builder to keep is intact.changelog.d/57.mdmatcheschangelog.d/<issue>.md, is a single-bullet well under 300 characters, and ends(#57).with nothing after — byte-identical in shape to54.md,48.md,43.md. No headings, consistent with every prior fragment in this repo; grouping happens at release assembly.CHANGELOG.mdcorrectly untouched.shellcheck scripts/publish-deb.shis clean andbash -nparses; the quoted<<'EOF'heredoc is safe underset -euo pipefailand the backticks aroundstoke auth loginare not expanded.npm run check:governancepasses:test/*.test.jsis inUNSCOPED_PATHS(scripts/check-governance.js:10), so the new test file needs no scope mapping, andscope:packagingcorrectly coversscripts/**andchangelog.d/**.Closes #57is right here: #57's task list explicitly sanctions either keyword because every criterion is pre-merge, so theRefs-body adjacency rule does not apply.What I could not verify
ci / testandlabels / labelsas success fromcommits/<sha>/statusand reproducednpm ci && npm test && npm run check:governancelocally, which is exactly what.forgejo/workflows/ci.ymlruns.curland the status handling out of scope, and the test contract forbids reaching the network.Non-blocking nits
Neither gates this approval; take or leave them.
assert.doesNotMatch(result.stdout/stderr, TOKEN)is close to vacuous as written — the script never echoes the token, so those two lines pass no matter what happens tocurl's argv. The siblingtest/publish-release.test.js:103gets real value from the same idea because its stub logsprocess.argv; a two-lineprintf '%s\n' "$*" >> "$LOG"in thecurlstub would let this file assert the token reached theAuthorizationheader and stayed out of anything captured. Worth having whenever the upload path is next touched.publish-debtest file to live in but still no case in it. Out of scope for #57 — noting it so it is a deliberate gap rather than an assumed one.Approve. Reviewed the whole PR at head
3068809bin a detached worktree against the acceptance criteria of #57.Acceptance criteria — all five verified:
scripts/publish-deb.shnow exits 1, writes nothing to stdout, and the stderr message namesSTOKE_TOKEN,secrets.RELEASE_TOKEN, and that an empty value means the secret — not the tooling — is the fault.stoke auth loginremedy in the message.test/publish-deb.test.jscovers both the empty-token failure and a non-emptySTOKE_TOKENpassing the guard, and asserts the token value appears in neither stdout nor stderr. It stubscurland never reaches the network.git diffagainst the merge base (088e7e2) touches exactlyscripts/publish-deb.sh,test/publish-deb.test.js, andchangelog.d/57.md— no change torelease.yml,publish-release.sh, or the upload/status logic.npm testpasses: 143/143 on this box (see verification note), and the commit-status rollup on the head is success.Failure cases actually fail (probed, then restored): the new empty-token test fails when run against the pre-change script, and fails when the message drops
RELEASE_TOKEN— so the test guards the regression it claims to, not just the happy path.Conventions: the
changelog.d/57.mdfragment matches the repo's existing fragment style (- <sentence>. (#N).); the header-comment update landed in the same commit as the message, per the issue's tasks;scope:packaginglabel is correct for the touched paths;npm run check:governanceis green.What I could not verify and what I relied on instead: this box ships without
node, so I could not use the system toolchain. I ran the suite with a standalone Node v22.20.0 tarball (matching the CI imagenode:22-bookwormandengines >=22.12.0) extracted inside the duty trees directory, with dependencies fromnpm ci. Everything above was executed, not read. The only thing I did not run myself is CI'sci / testjob; I relied on the green commit-status rollup on the head plus my local full-suite pass.