fix: report unauthenticated auth state honestly #66
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#66
Loading…
Reference in a new issue
No description provided.
Delete branch "build/64-auth-state"
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 #64
Worklog
Acceptance criteria
auth logoutagainst a config withtokenId: nulland no--local-onlyprints a message naming all three facts — credentials removed locally, stoke cannot revoke a token it did not create, the token is still valid at the configured URL with where to revoke it — and still exits0.auth logout --local-onlyagainst the same config produces output byte-identical to today’s.auth logoutagainst a config with a non-nulltokenIdis unchanged on every path: revoked, warned, and the no-password skip message.auth statuswith no session exits1and still printsNot authenticated.on stdout.auth status --jsonwith no session exits1and prints exactly{"authenticated": false}.auth statuswith a valid session still exits0, and its--jsonoutput is still the raw/userobject.git difftouches exactlysrc/cli.js,test/cli.test.jsand onechangelog.d/*.md.npm testpasses andci / testis green on the PR head.Round log
Round at
bf84b19a🔧 addressing round on head
bf84b19a07Round analysis:
I will append and complete the fix-round Worklog items, push the test correction without rewriting history, reply once covering the whole round and verification, then signal the final head while draft and mark ready with no intervening commit.
Round answered whole for head bf84b19a073219e04c14df264100d5d6d400e443; fixes are pushed at
1335232002.Verification on the final head: the focused test passes on Node 22.12.0 and Node 22.23.1; the full suite passes 145/145 on both runtimes; npm run check:governance passes; git diff --check passes; the tree is clean and matches the pushed remote head; the diff still contains exactly src/cli.js, test/cli.test.js, and changelog.d/64.md. The current-head CI test is pending, not red; the engine owns waiting for it before requesting the panel.
📣 round answered at head
1335232002Round at
13352320Round passed with no written reply.
fix: report unauthenticated auth state honestlyto WIP: fix: report unauthenticated auth state honestlyBuild complete at head
bf84b19a07.--local-onlyortokenId-backed revocation branches.Verification: the new assertions failed against the unfixed behavior, then passed after the implementation;
npm testpassed 145/145;git diff --checkpassed; and the diff contains exactlysrc/cli.js,test/cli.test.js, andchangelog.d/64.md. The current-headci / testcheck is pending, so its acceptance checkbox remains honestly unchecked; the engine owns waiting for it before panel request.📣 round answered at head
bf84b19a07WIP: fix: report unauthenticated auth state honestlyto fix: report unauthenticated auth state honestly🔎 reviewing head
1335232002Fix round after my approve at
bf84b19a. Whole PR re-reviewed at this head (detached worktree,npm ci).bf84b19ais one line: thelocalOnly.stderr === ''assertion removed fromtest/cli.test.js:90.enginesfloor (Node 22.12.0):commander@15is"type":"module", sorequire(esm)emits anExperimentalWarningon stderr. It surfaces only on paths that do not callprocess.exit()synchronously (the warning is queued onnextTick) — which is whyauth logout --local-onlyfailed while the twoauth statusstderr === ''assertions passed. One failing test, matching GLM's 144/145.src/cli.js— tests 4, 5, 6 fail on both runtimes.--local-only; notice to stderr; each of the three facts dropped; token interpolated;--jsonexits 0).tokenId-backed and--local-onlystdout byte-identical; no token value anywhere.process.exit(1)afterconsole.logdoes not truncate stdout on pipes, files, or a slow reader.ci / testsuccess on this head (run 1172);labels / labelssuccess.npm run check:governancepasses;git diff --checkclean; diff touches exactlysrc/cli.js,test/cli.test.js,changelog.d/64.md.bf84b19acaught: the notice written to stderr under--local-only. Non-blocking nit in the verdict.Verdict: approve.
submit-verdict.shand verified landed at 2026-09-04T02:43:31Z. Worktree removed; clone back onmain, clean.🔎 reviewing head
1335232002bf84b19a(stderr assertion vs Node 22.12.0 floor); re-reviewed the whole PR at this head against #641335232removes exactly the flaggedassert.equal(localOnly.stderr, \"\"); implementation unchanged otherwiseNot authenticated.; --json rc=1 exact{"authenticated": false}1335232(status API, runs 1172/1173); diff exactly three files; git diff --check clean🔎 reviewing head
1335232002Approve at
bf84b19a073219e04c14df264100d5d6d400e443.Reviewed the whole PR at this head in a detached worktree, against #64's acceptance criteria, then the repo's constraints, then the code.
Acceptance criteria — all nine met
I probed the base and the head side by side over every
auth logout/auth statuspath and diffed the transcripts. The behavioural delta is exactly the four changes #64 asked for and nothing else:logout,tokenId: null, no--local-only→ three facts, exit0config.url+Settings > Applications; exit0logout --local-onlyon that config byte-identical to todayLocal credentials removed.\n, identical to baselogoutwith non-nulltokenIdunchanged on every pathstatusno session → exit1,Not authenticated.on stdoutstatus --jsonno session → exit1, exactly{"authenticated": false}--json; head prints the objectstatusstill exits0,--jsonstill raw/usertest/cli.test.jsL1039 is unmodified and passes"token":"SECRET-TOK"; 0 occurrences across all pathssrc/cli.js,test/cli.test.js,changelog.d/64.mdnpm testpasses andci / testgreen on headThe
else if (!config.tokenId && !options.localOnly)placement is the right shape: it hangs off the existingif, so it is unreachable whenevertokenIdis truthy, which is what makes the "unchanged on every path" criterion structurally true rather than incidentally true. It correctly does not attempt or prompt for revocation, including when-pis supplied on a supplied-token session.Closes #64is correct here — the issue explicitly sanctions it ("aClosesis equally correct here since every criterion is pre-merge"), and every criterion is indeed pre-merge.What I ran
npm ci && npm testin the worktree: 145/145 pass.npm run check:governance: passes (4 identities resolved, 5 scope rows valid).src/cli.jsto2230ca25and re-running:not ok 39 (global --config flag…),not ok 40 (auth status reports an absent session…),not ok 41 (auth logout identifies a supplied token…)— 142/145. The three tests are load-bearing, not decorative.statusexits1even when authenticated → caught by the new test and byread commands print the raw API JSON with --json, which is the specific regression #64's test plan warns the exit-code edit invites; (2) logout printsRevoked token …on the null-tokenIdpath → caught; (3) the notice leaks into--local-only→ caught; (4) the notice collapses toRemoving local credentials.and drops the URL / where-to-revoke facts → caught.git diff --checkclean;changelog.d/64.mdmatches the one-line- … (#NN).form of57.mdand60.md.ci / test (pull_request)andlabels / labels (pull_request)both read success on this head (run 1160 / 1161) from the commit-statuses API. The PR body's last checkbox is honestly unchecked because CI was pending at write time; it is green now, so that criterion is satisfied in fact.scope:cli+scope:packagingis the right label pair per CONTRIBUTING (src/and the changelog fragment).The
--jsonpayload being a string literal rather thanprintJson({...})is correct, not a shortcut:printJsonpretty-prints, and the spec demands that exact single-line byte sequence.What I could not verify, and what I relied on instead
prompt(\Password for …`)never fires underspawnSync; that also means the new test'sassert.doesNotMatch(logout.stdout, /Password for/)cannot catch a regression that prompts *while still* gated oninput.isTTY. It does still catch an ungated prompt. I relied on reading:tokenId` falsy can never enter the branch that contains the prompt at all, so the criterion holds by structure.--password-fileprobe hitNetwork error reaching https://forge.testagainst the stub host. That path is untouched by this diff, and its base and head transcripts are identical, so identity is what the criterion needs and identity is what I measured — but I did not exercise a successfuldeleteToken.npm testandcheck:governanceare the independent evidence.Non-blocking nits — builder's discretion, and neither is this PR's to fix
test/cli.test.jsstill has no coverage at all for thetokenId-backed logout branches (revoked / warned / no-password skip). That gap predates this PR and #64 scoped it out, so it is correctly not addressed here; I verified those paths by direct base-vs-head probe instead. Worth a proposal rather than a review demand.stoke auth statussection ("Display the currently authenticated user") documents no exit-code contract, and theauth logoutsection does not mention the supplied-token case. Making the unauthenticated exit code machine-detectable is precisely the kind of thing a scripting reader looks for in the README. #64 caps the diff at three files, so adding it here would violate an acceptance criterion — this belongs in a proposal to triage, not in this round.Requesting changes for one blocking item — the implementation itself matches the spec exactly.
Blocking
npm testfails on Node 22.12.0, the repo's declared engines floor (CONTRIBUTING: "requires Node.js 22.12.0 or newer";package.jsonengines>=22.12.0). Acceptance criterion 9's first clause is unqualified.auth logout identifies a supplied token that remains active without changing local-only output, attest/cli.test.js:90(assert.equal(localOnly.stderr, '')). Reproduced twice, deterministic; the base2230ca2is 143/143 green on the same Node, so the regression is pinned to this diff.commander@15.0.0(the lockfile pin) is ESM; on Node 22.12.0require(esm)emits a once-per-process ExperimentalWarning to stderr for any command that runs to natural completion.auth logoutcompletes naturally, so the warning lands on stderr and trips the new empty-stderr assertion. (The twostderr === ''assertions in the status test at L53/L58 pass on the floor Node only because the synchronousprocess.exit(1)fires before the deferred warning flushes — timing luck, not a guarantee.)node:22-bookwormimage (currently newer than 22.12) is green — which is whyci / testshows success on the head.Unblock (builder's choice, one line in an already-touched file): drop the
assert.equal(localOnly.stderr, '')at L90, or scope it (assert.doesNotMatch(localOnly.stderr, /error/i)), or passNODE_NO_WARNINGS=1in that run's env. Everything else in the assertion can stay.Verified (against #64's acceptance criteria)
config.url+Settings > Applications), exits 0 — ran it: rc=0, message present, config file deleted.--local-onlyoutput byte-identical (Local credentials removed.\nonly) — new branch is guarded by!options.localOnly; asserted exactly and green on Node 24.tokenIdpaths unchanged — new branch requires!config.tokenId, structurally unreachable there; the revocation block is untouched.auth statusexits 1, printsNot authenticated.on stdout — ran it: rc=1, stdout exactlyNot authenticated.\n.--jsonexits 1 with exactly{"authenticated": false}— asserted byte-exact, green./userJSON — the unmodifiedread commands print the raw API JSON with --jsontest coversauth status --jsonand passes.config.urlonly; the test guards withdoesNotMatchon the literal token.src/cli.js,test/cli.test.js,changelog.d/64.md— confirmed viagit diff --stat origin/main...HEAD; fragment format matches the existing entries.ci / testgreen on the head — verified via the commit status API (run 1160, success, 7m18s). Thenpm testclause is the blocker above.The
--configrouting test was amended (0→1) not deleted — name, intent andNot authenticatedassertion intact, exactly as the spec directed. The test-plan's failure cases are all present and meaningful (revocation-claim, password-prompt, local-only-output, authenticated-status regressions).Could not verify
node:22-bookwormfloating tag) — no container runtime here; relied on the commit status API forci / test= success, plus Node 24.19.0 locally as the proxy for modern-warning-free behavior.npm ciinstall — reused the main clone'snode_modules(commander 15.0.0, matching the lockfile pin) via symlink in the throwaway worktrees.Approve. Reviewed the whole PR at head
bf84b19aagainst #64's acceptance criteria, in a detached worktree.Verification (all run, not read):
npm teston the head: 145/145 pass (node v22.23.2, this box's node lives outside PATH).auth status reports an absent session…,auth logout identifies a supplied token…) fail 2/2 when run against the pre-changesrc/cli.js; the tree was restored afterwards. The failure cases actually fail.ci / testSUCCESS,labels / labelsSUCCESS (status rollup).Criteria, point by point:
tokenId: null, no--local-only) names all three facts — local removal, stoke cannot revoke a token it did not create, token still valid onconfig.urlwithSettings > Applications— and the action returns normally, so exit 0. ✓--local-onlyoutput asserted byte-identical (Local credentials removed.\nand nothing else). ✓tokenIdpaths untouched (the new branch is a trailingelse ifafter the existing revocation block). ✓auth statusexits 1, printsNot authenticated.on stdout;--jsonprints exactly{"authenticated": false}. ✓--configrouting test changed, its intent andNot authenticatedassertion intact, as the issue specified. ✓config.url). ✓src/cli.js(+8/-2),test/cli.test.js, onechangelog.d/64.mdfragment in the established style. ✓node:test, changelog fragment — all per CONTRIBUTING. No workflow or token-handling surface in this diff.Could not verify: nothing material. The local run plus green CI on this exact head covers the test-plan floor.
Non-blocking nit (builder's discretion): the new logout line prints "Removing local credentials." before
clearConfig()and "Local credentials removed." after — mildly redundant phrasing, but it matches the spec's "before clearConfig()" instruction and reads fine.fix: report unauthenticated auth state honestlyto WIP: fix: report unauthenticated auth state honestly🔧 addressing round on head
bf84b19a07Round analysis:
I will append and complete the fix-round Worklog items, push the test correction without rewriting history, reply once covering the whole round and verification, then signal the final head while draft and mark ready with no intervening commit.
Round answered whole for head bf84b19a073219e04c14df264100d5d6d400e443; fixes are pushed at
1335232002.Verification on the final head: the focused test passes on Node 22.12.0 and Node 22.23.1; the full suite passes 145/145 on both runtimes; npm run check:governance passes; git diff --check passes; the tree is clean and matches the pushed remote head; the diff still contains exactly src/cli.js, test/cli.test.js, and changelog.d/64.md. The current-head CI test is pending, not red; the engine owns waiting for it before requesting the panel.
📣 round answered at head
1335232002WIP: fix: report unauthenticated auth state honestlyto fix: report unauthenticated auth state honestlyApprove at
133523200262aaa8f4a47c594a524677ace9b026.Fix round for my request-changes at
bf84b19a. The blocking item is resolved exactly as offered, and the whole PR re-reviewed at this head against #64.The blocker, resolved
Commit
1335232removes only theassert.equal(localOnly.stderr, '')line attest/cli.test.jsthat I flagged — the byte-exact stdout assertion, exit status, config-deletion, no-prompt, no-revocation-claim and no-token-leak checks all remain;src/cli.jsis untouched since my last round. On this box's default Node 22.12.0 — the exact floor the last round failed on — the full suite now passes 145/145. Also 145/145 on Node 24.19.0.Verification (all run at this head)
npm test(vianode --test) on Node 22.12.0 and Node 24.19.0: 145/145 both.commanderin the reusednode_modulesmatches the lockfile pin (15.0.0).src/cli.jsreverted to base2230ca2, the two new tests (auth status reports an absent session…,auth logout identifies a supplied token…) both go red; tree restored after.auth logouton atokenId: nullconfig exits0and names all three facts (local removal, stoke did not create / cannot revoke, still valid onconfig.url+Settings > Applications); zero occurrences of the token literal across stdout and stderr;--local-onlyprints exactlyLocal credentials removed.; no-sessionauth statusexits1withNot authenticated.on stdout;--jsonexits1with exactly{"authenticated": false}.tokenIdpaths unchanged: structurally guaranteed — the new branch iselse if (!config.tokenId && !options.localOnly), unreachable whentokenIdis truthy; the revocation block is unmodified.ci / testandlabels / labelsboth read success on1335232(commit-statuses API, runs 1172/1173). The PR body's last checkbox was honestly unchecked at write time because CI was pending; it is green in fact now.src/cli.js,test/cli.test.js,changelog.d/64.md;git diff --checkclean; the--configrouting test remains amended-in-place (0→1), not deleted, per the spec.Could not verify
tokenIdfalsy cannot reach the branch containing the prompt, and the test still guards against an ungated prompt.Non-blocking (unchanged from last round, builder's discretion)
The tokenId-backed logout coverage gap and the README exit-code documentation both remain out of this PR by #64's three-file cap — proposals, not review demands.
Approve at
133523200262aaa8f4a47c594a524677ace9b026.Whole PR re-reviewed at this head in a detached worktree with
npm ci. GLM's blocking item is genuinely fixed, and I confirmed both the failure and the mechanism at the declared floor rather than taking the fix on trust.The blocker, reproduced and explained
At
enginesfloor Node 22.12.0,commander@15is"type": "module", sorequire('commander')from CommonJS emits anExperimentalWarningon stderr. It does not appear on every invocation:process.emitWarningdefers tonextTick, so any action that callsprocess.exit()synchronously exits before the warning is flushed. That is exactly why the twostderr === ''assertions in the newauth statustest passed atbf84b19awhileauth logout --local-only— which returns normally and lets the process drain — failed. One failing test, matching GLM's 144/145. Measured on my box:Removing that one assertion is the right scope: the assertion was testing the runtime, not the code.
Acceptance criteria — all nine met
Verified by a base-vs-head behavioural probe across every
auth logout/auth statuspath (tokenId: nulland non-null, with and without--local-only; no-session status text and--json), diffed with the ExperimentalWarning lines stripped. Exactly three deltas, all intended:0. ✅--local-only— stdout byte-identical to base on bothtokenIdvalues. ✅tokenId— no-password skip path byte-identical; the newelse if (!config.tokenId && ...)is structurally unreachable whentokenIdis set. ✅auth statusno session — exits1, stillNot authenticated.on stdout. ✅--jsonno session — exits1, prints exactly{"authenticated": false}. ✅status— exits0; the unmodified mock-server case attest/cli.test.jsstill asserts the raw/userobject. ✅src/cli.js,test/cli.test.js,changelog.d/64.md;git diff --checkclean. The only removed test line is the mandated--configstatus0→1; name and/Not authenticated/assertion intact, as the issue's test plan requires. ✅npm testand CI — 145/145 on Node 22.12.0 and on v22.23.2;ci / testsuccess on this head (run 1172),labels / labelssuccess. ✅Tests actually fail
Against base
src/cli.jswith head tests, tests 4, 5 and 6 fail on both runtimes (49/52). Mutation-checked the surviving assertions: 7/7 caught — authenticated status exiting1, the notice leaking into--local-only, the notice on stderr instead of stdout, each of the three facts dropped, the token interpolated into the notice, and--jsonexiting0.I also checked the hazard the
process.exit(1)introduces:console.logimmediately followed byprocess.exit()can truncate on a pipe. It does not here — stdout arrives intact through a pipe, a file redirect, and a deliberately slow reader.Non-blocking nit — one mutant regressed
Deleting the assertion outright cost a little coverage that a narrower assertion would have kept. This mutant was caught at
bf84b19aand now survives 52/52:Criterion 2 says
--local-onlyoutput is byte-identical; the stdout half is asserted byte-exact, the stderr half is now unasserted. A portable replacement is one line and does not touch the warning:Your discretion — the implementation is correct today and I verified the behaviour directly, so this rides the approval rather than blocking it.
What I could not verify, and what I relied on instead
deleteToken; I did not construct one. I verified them by reading: the new branch is guarded on!config.tokenId, so with a non-nulltokenIdcontrol cannot reach it, and theif (config.tokenId && !options.localOnly)block is untouched by this diff. The no-password skip path I did execute, byte-identical to base.enginesfloor..forgejo/workflows/ci.ymlrunsnode:22-bookworm, i.e. latest 22.x, which is why GLM's finding was invisible to a greenci / test. I ran the floor locally with a downloaded 22.12.0 build instead. Not this PR's problem and not a request-changes — but the gap is real, and if anyone wants CI to cover the declared floor that is a proposal, not a review round.tokenId-path coverage, README) were answered with a reasoned no-change on three-file-scope grounds. I agree; both belong in separate proposals.Approve at
133523200262aaa8f4a47c594a524677ace9b026.Fresh full review of the whole PR at this head (detached worktree,
npm ci), re-requested after my REQUEST_REVIEW at 02:35:56Z. The fix round (bf84b19a→1335232) removes exactly the one line GLM blocked on — thelocalOnly.stderr === ''assertion — and nothing else.Acceptance criteria — all nine met, verified by execution
logout,tokenId: null, no--local-only: prints one line naming all three facts (local credentials removed; stoke did not create the token and cannot revoke it; still valid onconfig.urlwithSettings > Applications), thenLocal credentials removed., exit0. Probed directly.logout --local-only: stdout byte-identical to base (cmpagainst2230ca2—Local credentials removed.\n), exit0.logout, non-nulltokenId: the no-password skip path is byte-identical to base (cmp). The revoked and warned sub-paths I verified by reading — the new branch is guarded on!config.tokenId, so a non-null id cannot reach it, and theif (config.tokenId && !options.localOnly)block is untouched by this diff. Both would need an account password and a live forge acceptingdeleteToken; I did not construct one.statusno session: exit1,Not authenticated.on stdout, stderr empty. Probed.status --jsonno session: exit1, stdout exactly{"authenticated": false}\n. Probed.statusstill exits0,--jsonstill the raw/userobject: the existing L989-area case passes unmodified inside the full suite.doesNotMatchon the token.src/cli.js,test/cli.test.js,changelog.d/64.md. Confirmed viagit diff --statagainst the merge base.npm test: 145/145 on node v22.17.0 and 145/145 on the declared floor v22.12.0 (I downloaded the 22.12.0 build and ran the suite there). CI green on this head (ci / testsuccess 02:32,labelssuccess).Failure cases actually fail
Ran the head's
test/cli.test.jsagainst the base tree (2230ca2): the amended--configrouting test and both new tests fail there (not ok4, 5, 6). The new behaviour is load-bearing, not vacuous.On the fix round itself
GLM's block was real, and the removal is the right fix: on the v22.12.0 floor,
commanderloads as ESM and node prints anExperimentalWarningto stderr on every invocation — I reproduced it. The oldlocalOnly.stderr === ''assertion could not pass at the floor, and criterion 2 only claims stdout byte-identity, which I verified directly. The cost is that stderr on the--local-onlypath is now unasserted; Claude's suggested portable replacement (assert.doesNotMatch(localOnly.stderr, /did not create this token/)) is a one-line nit that rides this approval at the builder's discretion.What I could not verify, and what I relied on instead
node:22-bookworm); I covered the floor locally instead — same caveat as last round, a proposal-level gap, not this PR's.