Skip to content

v3.1.0: per-identity API tokens, whoami, vendor gitignore - #14

Merged
than merged 7 commits into
mainfrom
v3.1.0-identity-and-hardening
Aug 6, 2026
Merged

v3.1.0: per-identity API tokens, whoami, vendor gitignore#14
than merged 7 commits into
mainfrom
v3.1.0-identity-and-hardening

Conversation

@than

@than than commented Jul 29, 2026

Copy link
Copy Markdown
Owner

What

Org logins no longer hijack the global identity. init keeps githubUsername personal and stores the org in per-repo playAsUsername; API tokens are now keyed by username via an apiTokens map, with the legacy single token auto-migrated. Adds git-slot-machine whoami to show the resolved identity for the current repo.

Security review

I ran a security review over this diff before opening the PR. Reviewer: I'd particularly like your read on the one open finding below.

Open — MEDIUM, deliberately not fixed here

src/config.ts:22-32 and :106-109mkdirSync/writeFileSync pass no mode, so ~/.git-slot-machine/ lands at 0755 and config.json at 0644. Verified on a real machine.

That file now holds the plaintext apiTokens map, so this diff changes the blast radius: a single readable file used to yield one bearer token, and now yields one per identity you've logged in as. Tokens are long-lived — nothing expires or rotates them.

Reachable by any other local UID, a co-tenant on a shared CI box, or an npm postinstall running under a different user.

Proposed fix, not yet applied:

fs.mkdirSync(configDir, { recursive: true, mode: 0o700 });
fs.writeFileSync(configPath, data, { mode: 0o600 });
// plus a one-time chmodSync in saveGlobalConfig — `mode` is ignored when the file already exists

Is the one-time chmodSync migration the right call here, or would you rather see it gated behind a version check?

Fixed here — LOW

vendor/ is now gitignored. 3,898 vendored PHP files (laravel/boost, guzzle, symfony) were untracked with no ignore rule, so git add . would have committed all of them. npm was never exposed — the files allowlist in package.json already excluded vendor/. composer.json/composer.lock stay tracked.

Checked and clean

  • Command injection — every execSync call (config.ts:232, utils/git.ts, balance.ts:20, amendDetector.ts:18) uses a constant string. The only interpolation is amendDetector.ts:18's numeric ${timeWindowMinutes}. Commit hashes and usernames never reach a shell.
  • Dependencies — none added; package.json only bumps the version. No typosquats in composer.lock.
  • Prototype pollutionapiTokens is built with object spread, so a __proto__ key from JSON.parse stays an own property.
  • Net improvementgetApiToken (config.ts:136-156) now reads global config only and keys by exact username, so a repo-local config can no longer inject an apiToken.
  • No eval/Function, no TLS bypass, no path traversal, no secret logging (auth.ts:105 prints a 10-char fingerprint only).

Pre-existing, out of scope for this PR

Server-side, tracked separately: POST /api/play is unauthenticated and trusts github_username from the body; play.ts computes payout, balance_after and the suspicious flag client-side and POSTs them, so all three are forgeable.

Verification

  • npm run build — clean
  • npm test — 58 passed, 2 suites

🤖 Generated with Claude Code

Org logins no longer hijack the global identity. `init` keeps
`githubUsername` personal and stores the org in per-repo
`playAsUsername`; API tokens are now keyed by username via an
`apiTokens` map, with the legacy single token auto-migrated.

Adds `git-slot-machine whoami` to show the resolved identity for the
current repo.

Security review of this diff (see PR body) turned up one hardening
item fixed here: `vendor/` is now gitignored, so the 3,898 vendored
PHP files pulled in by laravel/boost can't be staged by `git add .`.
`composer.json`/`composer.lock` stay tracked. npm was never exposed —
the `files` allowlist in package.json already excluded vendor/.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review

The core idea is right and the getApiToken() rewrite is a genuine improvement — reading global-only and keying by exact username closes the repo-config injection path cleanly, and authLoginCommand(authUsername, authUsername === githubUsername) is the correct seam for "authenticate as the org without adopting its identity." whoami is a good addition and leaks nothing (usernames only, no token material).

But the change is incomplete: three call sites still assume the old single-token world, and one of them hands the user the exact command that re-creates the bug this PR exists to fix.


Blocking

a) login still hijacks the global identity — and play.ts now tells users to run it

src/index.ts:68-70 calls authLoginCommand(githubUsername) with persistGlobalUsername defaulted to true, so it always calls setGitHubUsername(). Meanwhile the new notice at src/commands/play.ts:116 prints:

git-slot-machine login ${githubUsername}

where githubUsername is getGitHubUsername() — i.e. the resolved identity, which is playAsUsername in an org repo. So: commit in an org repo with no org token → CLI suggests git-slot-machine login acme-corp → user runs it → setGitHubUsername('acme-corp') → global identity is now the org. That is the hijack, reachable in two steps through the PR's own new hint.

init already computes the right answer; login should use the same rule:

.action(async (githubUsername: string) => {
  await authLoginCommand(githubUsername, getPlayAsUsername() !== githubUsername);
});

b) The new unauthenticated notice fires in --small mode, on every commit

src/commands/play.ts:112-118 is not gated on options.small, but --small is the only mode the post-commit hook uses (src/templates/post-commit.ts:9,11). Two consequences:

  • animateSmallMode deliberately ends without a newline (slotMachine.ts:161-162) so the caller completes the single line. This block writes console.log() straight after it, so the hash gets orphaned on its own line and the result renders detached — the one-line contract is broken.
  • It emits five extra lines on every single commit under CI/Claude Code, which is exactly the "only the final result line, save LLM context tokens" property the 2.x changelog entry calls out.

Gate it on !options.small at minimum. Given it repeats forever until the user acts, consider showing it once per repo rather than per commit. (The pre-existing "No GitHub remote" warning at :103-110 has the same flaw; not yours, but it's right above and worth fixing in the same pass.)

c) config get all will report "Has Token: no" for every 3.1 user

src/commands/config.ts:25 still reads config.apiToken, which migrateLegacyToken deletes on first read. It also reads the merged config, so a repo-local file can influence the answer. Should be getApiToken() ? 'yes' : 'no'.

d) init's auth-failure fallback is unreachable

src/commands/init.ts:226-236 wraps authLoginCommand in try/catch, but authLoginCommand calls process.exit(1) on a null token (auth.ts:25) and again in its own catch (:48). The process dies; the catch never runs. This was latent before, but this PR routes org logins through it, where createToken failing is much more likely (org name isn't a GitHub user the API will mint a token for, etc.). Failure now aborts init after setPlayAsUsername and the hook are already written, and the user never sees the "commits will work locally" message or the data-sent summary.

Have authLoginCommand throw (or return boolean) and let the top-level .action() in index.ts own the exit code.


On the file-permission finding you flagged

Ship the 0o700/0o600 fix — the reasoning in your PR body is sound, and the blast-radius argument is the right framing: one file now yields N long-lived bearer tokens instead of one.

To your question — unconditional chmodSync on every save, not a version gate. A version gate runs once and then stops being true: it won't repair a file whose mode drifted, won't cover a config written by an older binary still on PATH, and adds a persisted flag you have to reason about forever. chmodSync on an already-0600 file is one cheap syscall on a file you're writing anyway, and it self-heals.

One thing to get right, though: wrap it in try/catch. mode is ignored on Windows, and chmodSync can throw EPERM/ENOTSUP on bind mounts, 9p/WSL, and some CI volumes. That matters more than usual here because of the next item — saveGlobalConfig is now reachable from a read path, so an unguarded throw doesn't just fail the chmod, it blanks the config.

export function saveGlobalConfig(config: Config): void {
  const configPath = getGlobalConfigPath();
  fs.writeFileSync(configPath, JSON.stringify(config, null, 2), { mode: 0o600 });
  try { fs.chmodSync(configPath, 0o600); } catch { /* best effort */ }
}

Same treatment for the mkdirSync at config.ts:28.


Should fix

e) migrateLegacyToken writes from inside a read, and a failed write silently blanks the config

config.ts:94 calls saveGlobalConfig from within getGlobalConfig's try. If that write throws (read-only $HOME, ENOSPC, a container with an unwritable home — all real in CI), control lands in catch { return {}; } at :69-71 and the caller gets an empty config despite a perfectly valid file on disk. Downstream that reads as "no username, no token, sync default-on" — so the user's commit prints the not-authenticated nag from (b) while their config is fine.

Migration shouldn't be able to destroy a read:

try {
  saveGlobalConfig(migrated);
} catch {
  // couldn't persist; the in-memory migration still stands for this run
}
return migrated;

f) Token keys are case-sensitive; GitHub usernames are not

apiTokens is keyed by the exact string given. init is self-consistent (owner from the remote URL flows into both setPlayAsUsername and setApiToken), but the manual paths aren't: git-slot-machine login netflix stores under "netflix", while playAsUsername from the remote is "Netflix"config.apiTokens?.[username] misses → silently local-only, with no way for the user to tell why. username:set has the same hazard, and getApiToken's legacy fallback at :151 compares with === too. Normalize with .toLowerCase() on both write and lookup; keep display casing separate if you want it.

Related: the fallback branch at config.ts:150-153 is dead in practice. If githubUsername is set, migration already moved the token and deleted apiToken; if it isn't, getGitHubUsername() returns null and we've returned at :141. Also, the early return at :85-87 (token already present for that owner) leaves the legacy apiToken in the file forever — delete it there too.


Smaller notes

  • CHANGELOG.md got mangled by a formatter. Every heading is now ## \[3.0.0\], and two lines are actually corrupted: - **Artisan command: play:remove** became - **Artisan command:&#x20;** followed by the code span, which renders with a leaking HTML entity and broken bold. Suggest git checkout main -- CHANGELOG.md and adding just a hand-written ## [3.1.0] entry — which is also missing, despite package.json bumping to 3.1.0 and this repo keeping an entry per release.
  • composer.json + composer.lock (5,179 lines) are new file in this diff, not pre-existing — the PR body describes them as already tracked. If checking in the laravel/boost dev dependency is deliberate, fine (files in package.json keeps it out of the tarball), but it should be its own commit, not folded into an identity fix.
  • Split the Config type. apiTokens/apiToken sit on the same interface used for repo config, so the global/repo trust boundary this PR establishes is enforced by convention only — getConfig() will happily merge an apiTokens key out of .git/slot-machine-config.json. A GlobalConfig / RepoConfig split makes the compiler enforce what getApiToken() currently enforces by hand.
  • No tests for any of this. The 58 passing tests are all in patterns.test.ts / patterns.contract.test.ts; nothing exercises migration, per-identity lookup, or clearApiToken. The legacy-token migration in particular is one-shot and destructive — it deletes apiToken — so a bug there strands users with a config they can't recover from. Worth a small suite over a temp HOME covering: legacy migrate, idempotent re-read, playAsUsername miss, and logout-as-org leaving the personal token intact.
  • clearAllApiTokens (config.ts:179) is exported but never called. Wire it to a logout --all or drop it.
  • whoami hides "Playing as" when getRepoInfo() returns null (whoami.ts:30). playAsUsername lives in .git/ and applies regardless of whether a GitHub remote parses, so in a repo with no remote the command silently omits the very thing it exists to report.

patterns.ts is untouched, so no CLI/server payout divergence here. Nothing in the diff logs token material — auth.ts:105 is a 10-char prefix, and whoami prints usernames only.

Blocking items:
- login computes persistGlobalUsername from the repo's playAsUsername
  (same rule as init), so the hint play.ts prints can no longer re-create
  the org hijack this PR exists to fix
- Both play-time notices (no-remote, unauthenticated) gate on !--small:
  that's the post-commit hook's only mode and its single-line contract
- config get all reads token state via getApiToken(), not the removed
  legacy field through merged config
- authLoginCommand throws instead of process.exit, so init's catch is
  reachable and org-token failures degrade to local play as designed;
  the top-level CLI action owns the exit code

Security (the PR-body question, per review guidance):
- Global config dir 0700, file 0600, plus unconditional best-effort
  chmodSync in saveGlobalConfig — self-heals drifted modes, safe where
  chmod throws (Windows, bind mounts, CI volumes)

Should-fix:
- migrateLegacyToken persists inside try/catch: a failed write-back
  (read-only HOME, ENOSPC) keeps the in-memory migration instead of
  blanking a valid on-disk config to {}
- Token keys lowercased on write and lookup (GitHub usernames are
  case-insensitive); legacy fallbacks in getApiToken and
  getAuthenticatedUsernames removed as dead post-migration; lingering
  legacy apiToken deleted even when the username already has a token
- logout --all wires the orphaned clearAllApiTokens
- whoami shows the per-repo override even when no GitHub remote parses
- CHANGELOG restored from main (formatter mangling reverted) with a
  hand-written 3.1.0 entry

Tests: new config suite over a mocked homedir + temp cwd — legacy
migration, idempotent re-read, playAs token miss, org logout keeping the
personal token, case-insensitive lookup, failed write-back not blanking
the read, 0600 mode. 66 tests pass.

composer.json/lock stay tracked (deliberate, for the enumeration
tooling) — noted per review; happy to split on request.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review — round two

Most of round one landed cleanly. Confirming what's now correct:

  • authLoginCommand throws instead of process.exit(1), and both callers own the exit code — init's fallback path (init.ts:230-235) is reachable for real now, which is the whole point of routing org logins through it.
  • login computes persistGlobalUsername from getPlayAsUsername() (index.ts:73-75), and does it case-insensitively.
  • Both notices in play.ts are gated on !options.small, so the hook's single-line contract survives.
  • config get all goes through getApiToken(); config.apiToken has no readers left outside config.ts (grepped).
  • Case-insensitive keys on write and lookup, migration write-back can no longer blank a valid config, whoami shows playAs without a parseable remote.
  • CHANGELOG.md is un-mangled and has a real 3.1.0 entry. clearAllApiTokens is wired to logout --all. Tests exist and cover the destructive migration.

patterns.ts/patterns.json are byte-identical to main, so there's no CLI/server payout divergence in this diff.

One hole is still open, and it's the same hijack.


Blocking — init re-run still adopts the org as the global identity

init.ts:70 resolves the personal identity through getGitHubUsername(), which returns playAsUsername || githubUsername. In a repo that's already configured to play as an org, that returns the org, and every downstream decision in init is made against it:

  1. Repo acme-corp/api, user than, previously init'd with choice 2.git/slot-machine-config.json has playAsUsername: "acme-corp".
  2. User re-runs git-slot-machine init (hook got clobbered, upgrade, core.hooksPath change — all routine).
  3. :70githubUsername = "acme-corp".
  4. Truthy, so the setGitHubUsername block at :91 is skipped.
  5. :199repoOwner.toLowerCase() !== githubUsername.toLowerCase() is "acme-corp" !== "acme-corp"false. The personal-vs-org prompt never renders, and authUsername stays "acme-corp".
  6. :227authLoginCommand("acme-corp", "acme-corp" === "acme-corp")persistGlobalUsername: truesetGitHubUsername("acme-corp").

Global identity is now the org, and every unrelated repo on the machine plays as acme-corp. This is reachable for every existing 3.0 user with an org repo the first time they re-run init after upgrading, because 3.0's init already wrote playAsUsername.

whoami.ts:22 already has the right accessor for this — getGlobalConfig().githubUsername. init should use the same one:

let githubUsername = getGlobalConfig().githubUsername || null;

With that, an org repo falls into the !githubUsername branch, detects/prompts for the personal name, persists it at :91, and the credit prompt at :199 fires as designed. Worth a test in config.test.ts's style: config with playAsUsername set and no global githubUsername, assert the global identity is unchanged after the org login path.


Should fix

auth.ts:110 points at a command that doesn't exist. You edited this line to interpolate the username, but the command itself is wrong:

git-slot-machine auth login than

There is no auth subcommand — index.ts registers login, logout, and status at the top level. That's error: unknown command 'auth'. :129 has the same string. Your new hint in play.ts:119 gets it right (git-slot-machine login ${githubUsername}); make these two match.

apiLogout()'s return value is discarded at both call sites (auth.ts:62 and :78). It returns boolean and swallows network errors internally, so an offline logout prints Successfully logged out and then deletes the only copy of a token that is still live on the server — and, as your PR body notes, nothing expires or rotates it. The user now has an unrevocable bearer token in the wild and no way to know.

const revoked = await apiLogout();
clearApiToken();
if (!revoked) {
  console.log(chalk.yellow('Could not reach the server — the token may still be valid. Revoke it at gitslotmachine.com.'));
}

Related, --all (auth.ts:62-65): the code comment is honest that only the active identity is revoked server-side, but the user-facing string says Logged out all identities: than, acme-corp, which reads as "all revoked." Say what actually happened — cleared locally for all, revoked server-side for the active one.


On the file-permission question you asked

The implementation is right and matches what round one argued for: unconditional best-effort chmodSync, no version gate, wrapped in try/catch so a throwing chmod on Windows/bind mounts can't take down saveGlobalConfig — which matters because migrateLegacyToken now reaches it from a read path. { mode: 0o600 } on the write plus the chmod covers both new and drifted files. Ship it.

One gap: the directory doesn't self-heal the way the file does. config.ts:27-29 only passes mode: 0o700 inside if (!fs.existsSync(configDir)), so every pre-3.1 user keeps their 0755 directory forever. It's not a token-disclosure path on its own — 0755 denies write to others, so they can't swap the file, and the file itself is 0600 — but the CHANGELOG claims "Global config is written 0600 in a 0700 directory," which isn't true for upgraders. Either add the symmetric best-effort chmod:

try { fs.chmodSync(configDir, 0o700); } catch { /* best effort */ }

or soften the changelog line to describe new installs only.


Notes

  • The write-back-failure test can go false-green. config.test.ts:113 relies on chmod 0o400 blocking the write, but root ignores file permission bits, so in any root container CI the write succeeds and the test stops exercising the path it names — silently, because the assertions hold either way. Make it deterministic with jest.spyOn(fs, 'writeFileSync').mockImplementationOnce(() => { throw new Error('EROFS'); }), and assert the on-disk file still has apiToken afterward.
  • The Config split is still worth doing, and there's a sharper reason than "convention": getApiUrl() reads the merged config (config.ts:139), so a repo-local .git/slot-machine-config.json containing apiUrl wins over global — and getHeaders() attaches Authorization: Bearer <token> to whatever host that names. Anyone who can write into your .git/ can redirect every sync to their own server with the token attached. The bar is high (.git/ isn't cloned, and write access there is mostly game-over already), but getApiToken() now defends this boundary by hand while getApiUrl() doesn't. Pinning apiUrl to global-only, or a GlobalConfig/RepoConfig split that makes the compiler enforce it, closes the class rather than the instance.
  • composer.json + composer.lock (5,184 lines) are still folded into this diff as new files. Same note as last round — the files allowlist keeps them out of the tarball so there's no publish risk, but they don't belong in an identity fix.
  • Nit: getAuthenticatedUsernames() returns the lowercased map keys, so someone who ran login Netflix sees netflix in whoami and status. Harmless, just slightly off from what they typed.
  • PR body still says npm test — 58 passed, 2 suites; there are 3 suites now. Worth refreshing before merge.

I reviewed this statically — dependency install wasn't available in my environment, so I did not execute npm run build or npm test. Everything above is from reading the source, and the init trace in particular is worth confirming by hand: init a repo as an org, then re-run init and check whoami.

than and others added 2 commits August 5, 2026 23:20
3.1.0 stored apiTokens keys with whatever casing the user typed
(login Broomfitters -> key 'Broomfitters'); this branch lowercases
every lookup, so those tokens would silently miss and plays would go
local-only with no visible cause. Normalize keys on read, first
writer winning a casing collision to match pre-3.1.1 lookup behavior.
Found live: a real 3.1.0 config held 'Broomfitters'/'PlaydownApp'/
'MunksApp' keys that the new lookup missed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rl pinned

- init resolves the personal identity from getGlobalConfig()
  .githubUsername, not getGitHubUsername() — resolving playAsUsername
  first meant re-running init in an org repo adopted the org globally
  (skipped prompt, persistGlobalUsername true), reachable for every
  3.0 upgrader with an org repo; test pins the rule
- status hints name the real command (login, not the nonexistent
  auth login)
- logout surfaces a failed server revocation instead of printing
  success while deleting the only local copy of a still-live token;
  logout --all says what actually happens (cleared locally for all,
  revoked server-side for the active identity)
- config dir chmod 0o700 self-heals pre-3.1 0755 dirs, best-effort,
  matching the file treatment and the CHANGELOG claim
- getApiUrl reads global config only: getHeaders() attaches the bearer
  token to whatever host apiUrl names, so a repo-local override could
  redirect syncs, token attached
- write-back-failure test uses a mocked writeFileSync throw instead of
  chmod 0o400 (root ignores permission bits) and asserts the on-disk
  file still holds the legacy token

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review — round three

One commit since round two (bc19e52), and it fixes a real problem that neither of the earlier rounds caught: round two's lowercasing landed on the lookup side without normalizing keys already on disk, so any 3.1.0 config holding Broomfitters/PlaydownApp keys would have started missing silently and dropping plays to local-only with no visible cause. Catching that on a live config rather than in review is the good outcome. migrateLegacyToken is idempotent, the early return keeps it from rewriting the file on every read once normalized, and the write-back is still inside the try/catch from round two, so a failed normalize can't blank a valid config. The new test pins the behavior.

patterns.ts and patterns.json are still byte-identical to main — no CLI/server payout divergence in this diff. Sync remains correctly gated (sendPlayToAPI returns early on !isApiAvailable(), and the call site in play.ts:154-166 swallows). The hook's single-line contract survives.

But round two's blocking item is untouched, and the CHANGELOG in this diff now asserts it's fixed.


Blocking — init re-run still adopts the org as the global identity

src/commands/init.ts:70 is unchanged:

let githubUsername = getGitHubUsername();   // playAsUsername || githubUsername

The trace from round two still runs end to end. In a repo already configured to play as an org, :70 resolves to the org, the truthy branch skips the setGitHubUsername block at :187, the credit prompt at :199 compares "acme-corp" !== "acme-corp" and never renders, authUsername stays the org, and :227 calls authLoginCommand(org, org === org)persistGlobalUsername: truesetGitHubUsername(org). The global identity becomes the org for every repo on the machine.

This is not a hypothetical tail case: 3.0 already wrote playAsUsername, so it fires for any existing org user the first time they re-run init after upgrading — and re-running init is routine (clobbered hook, core.hooksPath change, upgrade).

That matters more this round because CHANGELOG.md:20 now says:

Org logins no longer hijack the global identity. init keeps githubUsername personal…

init does keep it personal on a first run. On a re-run it does the opposite. Shipping the claim without the fix is worse than shipping neither.

The fix is still one line, and whoami.ts:22 already uses the right accessor:

let githubUsername = getGlobalConfig().githubUsername || null;

An org repo then falls into the !githubUsername branch, detects or prompts for the personal name, persists it, and the credit prompt fires as designed. Your new config.test.ts is the right place for the regression: global githubUsername: 'than' plus repo playAsUsername: 'acme-corp', run the org login path, assert getGlobalConfig().githubUsername is still than.

If you've decided against this — say so and I'll drop it — but nothing in the branch or the commit messages engages with it, so I read it as missed rather than declined.


New this round — on the mixed-case migration itself

a) Collision resolution is insertion-order-dependent, and picks the older token.

config.ts:79 resolves { Broomfitters: A, broomfitters: B } by ??= over Object.entries, so the first key in file order wins. 3.1.0's setApiToken didn't lowercase, so login Broomfitters then login broomfitters leaves the newer token (B) appended second — migration keeps A and deletes B from disk.

The comment's rationale ("matching pre-3.1.1 lookup behavior") holds for the common shape, where playAsUsername carries GitHub's canonical casing and exact-match lookup was finding A. But it holds by accident of key order, not by intent: flip the two keys in the file and you get the other token. If the server invalidates a prior token when it mints a new one, the survivor is the dead one, and the live one is now unrevocable.

Resolve by identity rather than order — prefer the key that exactly matches the current playAsUsername/githubUsername, then fall back:

const preferred = new Set([config.playAsUsername, config.githubUsername].filter(Boolean));
for (const [key, value] of Object.entries(config.apiTokens || {})) {
  const lower = key.toLowerCase();
  if (apiTokens[lower] === undefined || preferred.has(key)) apiTokens[lower] = value;
}

b) The collision case is the one thing the new test doesn't cover. config.test.ts:96-116 uses three distinct keys, so ??= never actually arbitrates. The behavior the commit exists to choose is untested — add a case with both casings of the same name and assert which token survives.

c) package.json says 3.1.0; the migration code says it's fixing 3.1.0. config.ts:70-73, the test name at :96, and the commit message all describe normalizing keys "written by 3.1.0" with "3.1.1 lowercases every lookup" — but this PR is the 3.1.0 bump (package.json:3, 3.0.0 → 3.1.0), and the CHANGELOG entry is ## [3.1.0] - 2026-07-29.

Two possibilities, and they want different things:

  • 3.1.0 is already published from an earlier cut of this branch → the version bump here needs to be 3.1.1, and the CHANGELOG needs a 3.1.1 section, otherwise you're republishing a version number.
  • 3.1.0 is unreleased → the only configs with mixed-case keys are on machines that ran this branch (which matches "found live" in the commit message). The migration is still worth keeping as cheap insurance, but the comments should say so rather than implying a shipped release, and the dates should move off 2026-07-29.

Worth resolving before merge either way — I couldn't check the registry from here.

d) An unattributable legacy token is deleted without being revoked. If apiToken is set but githubUsername isn't, owner is undefined, nothing lands in apiTokens, and config.ts:97 deletes the field and persists — a live bearer token gone from disk with no way left to revoke it. The state is close to unreachable (old authLoginCommand wrote both), so this is a one-line guard, not a redesign: skip the delete when there's no owner to attribute it to.


Still open from round two

Status only — the reasoning is in the previous comment and none of it has changed:

  • auth.ts:110 and :129 still print git-slot-machine auth login. There is no auth subcommand; index.ts registers login/logout/status at top level. Both lines produce error: unknown command 'auth' if a user copies them. :110 was edited in this PR to interpolate the username, so it's live code in this diff.
  • apiLogout()'s return value is still discarded (auth.ts:62, :78). Offline logout prints success and deletes the local copy of a token that's still live server-side.
  • logout --all still says Logged out all identities: … while revoking only the active one server-side (auth.ts:60-65). The code comment is honest; the user-facing string isn't.
  • The config directory still doesn't self-heal. config.ts:27-29 only applies mode: 0o700 inside if (!fs.existsSync(configDir)), so upgraders keep 0755 — while CHANGELOG.md:23 now states "in a 0700 directory" as fact. Either add the symmetric best-effort chmodSync you applied to the file, or soften the line. (The file-side implementation is right; that part is settled.)
  • config.test.ts:113 can go false-green under root CIchmod 0o400 doesn't stop root, so the write succeeds and the test silently stops exercising the write-back-failure path while its assertions still pass. jest.spyOn(fs, 'writeFileSync').mockImplementationOnce(() => { throw new Error('EROFS'); }) makes it deterministic. This matters more now that migration does more work.
  • GlobalConfig/RepoConfig split, with the getApiUrl()-reads-merged-config argument from round two.
  • composer.json + composer.lock still folded into an identity fix. No publish risk; still doesn't belong here.
  • PR body still says "58 passed, 2 suites." It's 3 suites now, and the body's open-question section is stale — the permissions finding it asks about was implemented two commits ago.

Same caveat as round two: dependency install isn't available in my environment, so I did not run npm run build or npm test. Everything above is from reading the source. The init trace is the one to confirm by hand — init a repo as an org, re-run init, then whoami in an unrelated repo.

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review — round three

Round two's blocking item is genuinely closed. Confirming the fixes I can verify by reading:

  • init.ts:73 now resolves the personal identity from getGlobalConfig().githubUsername, so the org-repo re-run trace from last round terminates at the !githubUsername branch instead of adopting the org. The test at config.test.ts:156-167 pins the rule.
  • auth.ts:117 / :129 name login, not the nonexistent auth login.
  • Single logout now surfaces a failed revocation (auth.ts:88-92), and --all's wording no longer claims every identity was revoked server-side.
  • getApiUrl() is global-only (config.ts:164), closing the repo-local-apiUrl-redirects-the-bearer-token path.
  • Directory chmod self-heals pre-3.1 0755 dirs (config.ts:30-34), matching the CHANGELOG claim.
  • The write-back-failure test uses a mocked writeFileSync throw and asserts the on-disk legacy token survives — no longer false-green under root CI.
  • patterns.ts, patterns.json, and __fixtures__/golden-vectors.json are untouched in this diff, so there is no CLI/server payout divergence.

The composer.json/composer.lock note is settled — your reasoning for tracking them is fine, and the files allowlist keeps them out of the tarball. Dropping it.

One real defect left, and it's in code this PR adds.


Should fix — logout --all reports a revocation that never happened

api.ts:217-220 early-returns true when there is no active token:

export async function logout(): Promise<boolean> {
  if (!getApiToken()) {
    return true;   // "nothing to revoke" is indistinguishable from "revoked"
  }

The single-logout path is safe because it guards first (auth.ts:76-79, if (!token) ... return). The --all path (auth.ts:52-70) has no such guard — it gates on getAuthenticatedUsernames().length, which counts all identities, not the active one. So:

  1. Repo has playAsUsername: "acme-corp"; you hold only a personal than token.
  2. getApiToken() resolves the active identity → acme-corp → no token → null.
  3. apiLogout() returns true without making a request.
  4. clearAllApiTokens() deletes the than token.
  5. Output: Cleared local tokens for: than + The active identity's token was revoked on the server; the others remain valid there.

Nothing was revoked. The user's only token is now deleted locally and live on the server indefinitely — and they have been told the opposite. Same reachability from the plain sequence logout (clears than) then logout --all.

This is the exact failure mode you fixed for single logout last round; it just leaks back in through logout()'s early return. Minimal fix — don't let "no token" masquerade as success:

const activeToken = getApiToken();
const revoked = activeToken ? await apiLogout() : false;

and word the false branch as "could not be revoked / may still be valid" rather than "could not reach the server," since both causes land there.

Worth considering the fuller fix while you are in here: verifyToken(token) already shows that the API accepts an explicit bearer, so logout() could take an optional token and --all could loop over every held token and actually revoke each one. That turns "the others remain valid there" from a caveat into a non-issue. It needs an accessor that returns the token map rather than just the key names.


Smaller items

a) 3.1.0 vs 3.1.1 — the code and the release metadata disagree. package.json and the only new CHANGELOG entry say 3.1.0, but config.ts:81, config.ts:92, and config.test.ts:102-104 all describe normalizing configs written by 3.1.0, and commit bc19e52 says it found a real 3.1.0 config holding Broomfitters/PlaydownApp keys. Both can't be true of one release. If 3.1.0 is already on npm, this needs a 3.1.1 bump plus a CHANGELOG entry for the key normalization — npm publish over an existing version fails, and 3.1.0 users are the ones who need that fix described. If 3.1.0 never shipped and that config came from a local build of this branch, the "pre-3.1.1" framing in the comments is misleading and should say something like "configs written by earlier builds of 3.1." I couldn't check the registry from this environment, so please confirm which it is.

b) migrateLegacyToken silently destroys an unattributable legacy token. config.ts:97 only re-homes apiToken when config.githubUsername is set, but :113 deletes it unconditionally. A global config with apiToken and no githubUsername loses its only credential on first read, with no message — the user just becomes local-only. I think this is unreachable in practice (3.0's authLoginCommand always wrote both), which is presumably your reasoning in the getAuthenticatedUsernames comment. But the cost of being wrong is a destroyed credential, versus the cost of being right being one stale key, so I'd only delete migrated.apiToken when it was actually re-homed or is a confirmed duplicate.

c) The directory chmod runs on every config read, including inside the post-commit hook. getGlobalConfigPath() is called by every getGlobalConfig(), and a single --small play calls it about eight times (getGitHubUsernamegetConfig; isSyncEnabled; getApiToken twice, each of which re-enters via getGitHubUsername; getApiUrl; getHeadersgetApiToken again). That's ~8 chmodSync syscalls per commit to fix a mode that can only change on a write. Irrelevant on local disk; on an NFS/SMB home directory each one is a round trip, and .claude/release.md is explicit that the hook path stays fast. Moving the directory chmod into saveGlobalConfig alongside the file chmod gets the same self-healing without putting it on the read path.

d) authStatusCommand gates the identity list inconsistently. auth.ts:113 shows Tokens held for: at length > 0, :126 at length > 1. Minor, but the authenticated branch is where a user is most likely to be checking which identities they hold.

e) The GlobalConfig/RepoConfig split is still worth doing, but it's no longer urgent. Both concrete holes are closed now that getApiToken() and getApiUrl() read global-only. What's left is getGitHubUsername() and isSyncEnabled() reading the merged config — neither is an escalation (playAsUsername is the intended per-repo identity knob, and you can only use a token you already hold), so this is fine as a follow-up rather than a blocker on this PR.


Nothing in the diff logs token material: auth.ts:124 is a 10-char prefix, whoami prints usernames only, and the apiTokens map never reaches stdout. The --small gating on both play.ts notices holds, so the hook's single-line contract survives.

One caveat on my verification: dependency install required approval in this environment, so I did not execute npm run build or npm test — everything above is from reading the source, same as the previous rounds. The logout --all trace in particular is worth confirming by hand: set playAsUsername to an org you hold no token for, then run logout --all and watch it claim a revocation.

- api.logout takes an optional explicit bearer and returns false when
  there is no token — 'nothing to revoke' must not masquerade as
  'revoked'. logout --all loops every held identity's token through a
  real server revocation and names the ones that failed; the previous
  wording claimed a revocation that never happened when the active
  identity held no token (reachable via logout then logout --all)
- Single-logout failure wording covers both causes (unreachable server,
  rejected request), not just network
- migrateLegacyToken keeps an unattributable legacy token instead of
  destroying the config's only credential; no write-back when there is
  nothing to migrate
- Directory chmod moves off the read path into saveGlobalConfig — modes
  only change on writes, and the hook path was paying ~8 chmod syscalls
  per play
- status shows 'Tokens held for' consistently at length > 0
- Version 3.1.1 (3.1.0 is live on npm and contains the bugs this branch
  fixes); CHANGELOG split into an accurate 3.1.0 entry and a 3.1.1
  entry covering the review rounds

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review — round four

logout --all is properly fixed. logout(token?) (api.ts:220-225) no longer treats "no token" as a vacuous success, the --all loop pulls each identity's token via the new getApiTokenFor and actually revokes it (auth.ts:64-70), and the output now names the identities that failed rather than asserting a blanket revocation. That closes the round-three trace end to end. Version metadata is consistent too — package.json is 3.1.1, the CHANGELOG has both a 3.1.0 and a 3.1.1 section, and the "written by 3.1.0" comments in config.ts:74-78 now line up with a version that isn't this one. (I couldn't reach the registry from here; the only thing left to confirm is whether 3.1.0 actually shipped, since that decides whether the 3.1.0 section is history or fiction.)

Also re-verified: patterns.ts, patterns.json, and __fixtures__/golden-vectors.json are byte-identical to main, so there's no CLI/server payout divergence in this diff. sendPlayToAPI still short-circuits on !isApiAvailable() and the play.ts:155-165 call site swallows. Both notices remain gated on !options.small, so the hook's single-line contract holds. Nothing in the diff logs token material.

One new defect, introduced by round three's own fix.


Blocking — choosing "personal" in init silently doesn't take effect

init.ts:220-224, the else branch of the credit prompt, prints the confirmation but never clears playAsUsername:

} else {
  // Play as personal username (default)
  console.log(chalk.green(`✓ Commits in this repo will be credited to ${githubUsername}`));
  console.log();
}

Only the choice === '2' branch writes (:214). There is no clearPlayAsUsername anywhere in config.ts, so an existing override in .git/slot-machine-config.json survives.

That branch was unreachable in this state before this PR. On main, :70 was getGitHubUsername(), which resolves playAsUsername first — so in an org repo repoOwner.toLowerCase() !== githubUsername.toLowerCase() was false and the prompt never rendered. Your :73 change to getGlobalConfig().githubUsername is correct and is exactly what makes it render. Trace:

  1. Repo configured with playAsUsername: "acme-corp"; global githubUsername: "than".
  2. Re-run init (routine — clobbered hook, core.hooksPath change, upgrade). :73than.
  3. :202"acme-corp" !== "than" → prompt fires. User picks 1.
  4. Prints ✓ Commits in this repo will be credited to than. authUsername = "than", global identity correctly untouched.
  5. playAsUsername is still acme-corp. Next commit: getGitHubUsername()acme-corp, getApiToken() → the org's token. Plays keep going to the org.

So the one UI affordance for moving a repo back to personal credit does nothing, and tells the user it worked. Fix is symmetric with :214:

// config.ts
export function clearPlayAsUsername(): void {
  const config = getRepoConfig();
  delete config.playAsUsername;
  saveRepoConfig(config);
}

called in the else. Worth deciding the privacy-mode path in the same pass: :202 skips the whole prompt when usePrivacyMode, so enabling privacy on a repo that already plays as an org leaves the override in place with no way to see or change it short of whoami plus hand-editing. Your config.test.ts is the right home for the regression — set playAsUsername, run the personal branch, assert getPlayAsUsername() is null.


Should fix

login <org> still overwrites an established global identity when no override exists yet

index.ts:73-75 gates on getPlayAsUsername(), so the hijack is closed for every path that sets the override first — init's org branch, its auth-failure hint at :237, the play.ts:119 and auth.ts:129 hints. Good. But the first org login in a repo has no playAsUsername yet:

  • Global githubUsername: "than", repo owner acme-corp, no override.
  • git-slot-machine login acme-corpisPerRepoIdentity false → persistGlobalUsername: truesetGitHubUsername("acme-corp").

Global identity is now the org, machine-wide, with no prompt and no output line saying so. That's the exact class CONTEXT.md names ("never changed as a side effect of acting for one").

The argument for fixing it rather than calling it user intent is that you already ship a dedicated command for the deliberate case — username:set (index.ts:128-135). So login never needs to clobber an established identity:

const globalUsername = getGlobalConfig().githubUsername;
const persist = !globalUsername || globalUsername.toLowerCase() === githubUsername.toLowerCase();
await authLoginCommand(githubUsername, persist);

First-time users still get their identity set; everyone else gets a token stored under the new identity and their global one left alone. If you'd rather keep the overwrite, CHANGELOG.md:17 needs rewording either way — "Both resolve the personal identity from the global config" describes init accurately but not login, which only consults playAsUsername. Round three's point about claims outrunning fixes applies.

Re-authenticating orphans the previous token server-side

auth.ts:33 overwrites unconditionally, and setApiToken (config.ts:202-206) spreads over the existing map. Run login than twice — or re-run init on an org repo, which is the flow this PR made routine — and the first token is gone from disk while staying live on the server forever. That's the same failure you fixed on the logout side, arriving through the mint path instead. logout(token?) now takes an explicit bearer, so it's cheap:

const previous = getApiTokenFor(githubUsername);
setApiToken(token, githubUsername);
if (previous && previous !== token) {
  await apiLogout(previous);   // best effort; don't fail the login on it
}

Offline logout --all destroys tokens it couldn't revoke

auth.ts:72 runs clearAllApiTokens() regardless of the loop's outcome. Offline with three identities: all three revocations fail, all three are correctly named as unrevoked — and then all three are deleted locally. Unlike the single-logout case there's no retry, because the bearers needed to revoke them are gone. Keeping the unrevoked ones (and saying "re-run when you're back online") preserves the ability to finish the job; if you want the current behavior available, put it behind --force.


Smaller items

  • auth.ts:48 prints a command that doesn't exist: git-slot-machine config set sync-enabled false. It's registered as config:set (hidden) — sync:disable is the discoverable one. Same class as the auth login strings you fixed this round, and it's the last line of the function this PR rewrote. Pre-existing text, one-word fix.
  • The casing-collision branch is still untested. config.test.ts:110-130 uses three distinct names, so ??= at config.ts:85 never arbitrates — the one behavior that comment exists to choose is the one the suite doesn't pin. A case with both Netflix and netflix present, asserting which token survives, costs four lines. (I'm not re-litigating the first-writer-wins choice; the comment states it and round three dropped it.)
  • logout --all makes N sequential revocations with no timeout, each trying the configured URL then the fallback domain. fetchWithFallback sets no AbortSignal, so against a black-holing network (rather than a fast DNS failure) each one waits out the OS connect timeout. Manual command, not the hook path, so low stakes — but Promise.all over the identities plus AbortSignal.timeout(5000) would bound it. The same missing timeout on sendPlayToAPI is pre-existing and worth its own issue, since that one is on the post-commit path.
  • getAuthenticatedUsernames() returning lowercased keys is unchanged from round two's nit — still cosmetic.

Same caveat as the previous rounds: dependency install needs approval in this environment, so I did not run npm run build or npm test — everything above is from reading the source. The init trace is the one to confirm by hand: init a repo choosing the org, re-run init and choose personal, then cat .git/slot-machine-config.json.

- clearPlayAsUsername() + call in init's personal branch: the round-3
  identity fix made the credit prompt render on re-runs, but choosing
  '1) personal' printed success while the org override survived —
  plays kept crediting the org
- login adopts the name globally only when no identity is established
  or it matches the established one; a first org login (no override
  yet) previously overwrote the global identity silently. The
  deliberate change stays username:set
- Re-authenticating best-effort revokes the token it replaces
  (overwriting without revoking left the old bearer live forever)
- logout --all keeps tokens it couldn't revoke so a re-run can finish
  the job, instead of destroying the only credential able to
- clearAllApiTokens deleted (orphaned by the above); sync hint names
  the real sync:disable command; tests for the override clear and the
  casing-collision first-writer rule (71 pass)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review — round five

Round four's blocking item is closed and the two "should fix" items landed. Confirming what I can verify by reading:

  • clearPlayAsUsername() exists (config.ts:262-266) and init's personal branch calls it (init.ts:224), so choosing 1) personal on a re-run actually moves credit back. Pinned by config.test.ts:112-120.
  • login no longer clobbers an established global identity (index.ts:76-79) — the first-org-login case now stores a token and leaves githubUsername alone.
  • Re-authenticating best-effort revokes the token it replaces (auth.ts:32-43), and apiLogout can't throw, so it can't fail the login.
  • logout --all clears only the identities it actually revoked (auth.ts:74-89).
  • clearAllApiTokens is gone with no dangling references (grepped), and auth.ts:55 names the real sync:disable.
  • The casing-collision branch is now tested (config.test.ts:122-131).

patterns.ts, patterns.json, and __fixtures__/golden-vectors.json are byte-identical to main (git diff --stat against 03c3269 is empty for those paths), so there's no CLI/server payout divergence in this diff. Both play.ts notices are still gated on !options.small, the sync path still short-circuits on !isApiAvailable() and swallows at the call site, no any was introduced, relative imports keep their .js extensions, and nothing logs token material.

Three things worth addressing, one of which is a side effect of round four's own fix.


Should fix

a) logout --all can never clear a token the server permanently rejects

auth.ts:76 keeps an identity locally whenever apiLogout returns false, and fetchWithFallback (api.ts:82-91) collapses "network unreachable" and "server returned 401/404" into the same null. So logout() returns false for both.

For a token that's already dead server-side — revoked from the web, user deleted, DB reset — that means:

  1. logout --all → revocation is rejected → identity is kept.
  2. Output: Could not revoke: acme-corp — their tokens are kept locally so you can re-run logout --all when the server is reachable.
  3. Re-run, online, forever: same result. There is no run that succeeds.

The only escapes are hand-editing ~/.git-slot-machine/config.json, or making that identity the active one in some repo and running plain logout, which clears unconditionally (auth.ts:105). Round four's suggestion was "keep the unrevoked ones, and put the old behavior behind --force" — the keep half landed, the escape hatch didn't.

Either add --force at index.ts:89, or distinguish the two failures: have fetchWithFallback return the last response instead of null so logout() can report rejected separately from unreachable, and clear locally on a definitive 4xx.

While you're here, the two paths now disagree on the same failure: single logout deletes a token it couldn't revoke (with a warning), --all keeps it. Both are defensible; having both is the part that's hard to explain.

b) login <name> can store a token that does nothing, and the output doesn't say so

The persist rule at index.ts:76-79 is right. But auth.ts:45-47 prints the same block regardless of persistGlobalUsername:

Successfully authenticated!
GitHub Username: acme-corp

Two reachable cases where that's actively misleading:

  • Org repo, global identity than, no playAsUsername yet. git-slot-machine login acme-corp stores a token under acme-corp — and nothing credits acme-corp. Plays still resolve to than. setPlayAsUsername is called from exactly one place (init.ts:214), so there is no command that sets the override; the user's only route is re-running init. Before this round the command "worked" by hijacking; now it is a silent no-op with a success message on top.
  • Correcting a typo'd identity. Init'd as thann, run login than to fix it → token stored under than, global identity stays thann, plays keep crediting thann. username:set is the right command and nothing points at it.

The rule is correct; it just needs to be legible. Thread the decision into the output — when !persistGlobalUsername:

Token stored for acme-corp. This repo still credits plays to than.
  git-slot-machine init          # credit this repo to acme-corp
  git-slot-machine username:set  # change your identity

c) Confirm the server revokes only the presented bearer before shipping the re-auth revocation

auth.ts:41-42 now calls apiLogout(previous) immediately after minting and storing the replacement. That is correct if DELETE /api/auth/token is currentAccessToken()->delete(). If it is $request->user()->tokens()->delete() — a common shape — then revoking with bearer A also destroys the token B you just wrote to disk, and the user is left holding a dead bearer with no error: sendPlayToAPI swallows the resulting 401 and every play goes local-only. That is the same silent-degradation class this PR keeps closing, arriving through the mint path.

Reordering does not help: createToken has already run by then. If the endpoint is user-wide, the call has to go entirely. The backend is public at than/gitslotmachine.com; I could not fetch it from this environment, so please confirm the DELETE handler's scope.


Smaller items

  • clearApiToken() with no argument and no resolvable identity deletes config.apiToken (config.ts:216-218) — the unattributable legacy token that migrateLegacyToken goes out of its way to preserve (config.ts:109-114). It is unreachable today, because auth.ts:96 guards on getApiToken(), which is null in exactly that state. Still, it is the one place that undoes the round-three preservation; a guard would keep the two consistent by construction rather than by call-site luck.
  • init's privacy-mode path still skips the credit prompt (init.ts:202), so a repo that was public when first init'd as an org and later went private keeps an invisible override — whoami is the only way to see it. Raised in round four, nothing in the branch engages with it. Narrow (you cannot create the override in a private repo), fine to leave, but say so if it is deliberate.
  • logout --all still makes N sequential revocations with no AbortSignal (api.ts:68-95), each trying the configured URL then the fallback domain. Unchanged from round four, still low stakes — manual command, not the hook path. The same missing timeout on sendPlayToAPI is on the hook path and is pre-existing; worth its own issue.
  • The [3.1.0] CHANGELOG entry overstates what shipped. It credits "org logins via init no longer hijack the global identity" to 3.1.0, but the init re-run hijack was still live in that commit and was fixed in round three. The 3.1.1 entry above it is accurate and supersedes it; if 3.1.0 is on npm, the history reads as if the fix shipped a release earlier than it did.
  • The PR body is stale. It still presents the file-permission finding as "Open — deliberately not fixed here" and asks the chmodSync-vs-version-gate question that was answered in round one and implemented four commits ago, and it reports 58 passed, 2 suites (the round-five commit message says 71 across 3). The body is what anyone reading this PR later sees first.

Same caveat as the previous rounds: dependency install requires approval in this environment, so I did not run npm run build or npm test — everything above is from reading the source. The one to confirm by hand is (a): login an identity, revoke its token from the web, then run logout --all twice and watch it never converge.

…an token

- logout gets --force (both paths): a server rejection is
  indistinguishable from an unreachable server through
  fetchWithFallback, so a token already revoked elsewhere could never
  be cleared — every re-run kept it forever. Single logout and --all
  now share one rule: keep on failed revocation, --force to clear
  locally anyway
- login says when a stored token changes nothing: token minted for a
  name that neither is the global identity nor this repo's override
  now prints who the repo actually credits, pointing at init /
  username:set instead of a bare success message
- Re-auth revocation confirmed safe against the real backend:
  DELETE /auth/token is currentAccessToken()->delete(), so revoking
  the replaced bearer can't kill the fresh one
- clearApiToken no longer deletes the unattributable legacy token when
  no target resolves — the one call site that could undo
  migrateLegacyToken's preservation

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@than
than merged commit a7ed7f5 into main Aug 6, 2026
1 check passed
@than
than deleted the v3.1.0-identity-and-hardening branch August 6, 2026 03:51
@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review — round six (post-merge)

This landed (merged 03:51:37, ~27s after aa5808a), so everything below is follow-up material rather than a merge gate.

Round five's three items are closed, and I can verify all of them by reading:

  • --force escape hatch exists (index.ts:90, auth.ts:100-102, :132), so an identity whose token the server permanently rejects can finally be cleared. The two paths also agree on the default now — both keep an unrevoked token — which resolves the "having both is the part that's hard to explain" note.
  • No-op logins are legible (auth.ts:52-65). The note fires exactly when it should: init's org branch sets playAsUsername before calling in, so creditsThisName suppresses it there, and the standalone login acme-corp case gets the "this repo still credits plays to than" block plus the two commands that actually change something.
  • clearApiToken no longer undoes the round-three preservation (config.ts:216-221). The guard makes it unreachable-by-construction rather than by call-site luck, which was the point.
  • The DELETE scope question is answered in-code (auth.ts:42-43) — currentAccessToken()->delete(), so apiLogout(previous) can't kill the token just minted. I can't reach the backend from here to confirm independently, but it's now documented where the next reader will look.

Re-verified for this round: patterns.ts, patterns.json, and __fixtures__/golden-vectors.json are untouched by the diff (gh pr diff 14 --name-only), so no CLI/server payout divergence. sendPlayToAPI still short-circuits on !isApiAvailable() and the play.ts:155-165 call site swallows. Both notices stay gated on !options.small, so the hook's single-line contract holds. No any, relative imports keep .js, nothing logs token material.

One new defect, introduced by the --force fix itself.


Should fix — --force re-opens the false-revocation claim this PR spent four rounds closing

auth.ts:100-102 pushes force-cleared identities into revokedIdentities, and :109 drops the qualifier for the whole batch:

} else if (options.force) {
  clearApiToken(identity);
  revokedIdentities.push(identity);   // never revoked
}
...
const suffix = options.force ? '' : ' (revoked on the server)';
console.log(chalk.green(`Logged out: ${revokedIdentities.join(', ')}${suffix}.`));

Offline, holding than and acme-corp, logout --all --force:

  1. Both apiLogout calls return false (unreachable — not rejected).
  2. Both fall to the --force branch, both are cleared locally, both land in revokedIdentities.
  3. unrevoked is empty, so the warning at :112 never prints.
  4. Output: Logged out: acme-corp, than.

Nothing was revoked. Two bearer tokens that never expire server-side are now live with their only local copies deleted — and the user was told they logged out. That is round three's logout --all trace verbatim, arriving through the new flag instead of logout()'s early return.

--force genuinely has two outcomes and they aren't interchangeable, so track them separately:

const revoked: string[] = [];
const forced: string[] = [];
const kept: string[] = [];
for (const identity of identities) {
  const token = getApiTokenFor(identity);
  if (token && (await apiLogout(token))) { clearApiToken(identity); revoked.push(identity); }
  else if (options.force) { clearApiToken(identity); forced.push(identity); }
  else kept.push(identity);
}

and report forced as cleared locally — may still be valid on the server; revoke at gitslotmachine.com. This also fixes the case the flag was actually designed for: --force against a mix of reachable and already-rejected tokens currently tells the user nothing about which was which.

Single logout --force has the same shape at :132-141 — a failed revocation falls straight through to Successfully logged out as than. with the round-two warning suppressed. --force should mean "clear it anyway," not "stop telling me."


Smaller items

a) The login note covers one branch of the same asymmetry. auth.ts:55 gates on !persistGlobalUsername, but the confusing case exists on the other side too: global than, repo playAsUsername: acme-corp, run git-slot-machine login than. persist is true, so the note is skipped, and the user gets Successfully authenticated! / GitHub Username: than while every commit in that repo credits acme-corp. One rule covers both:

const effective = getGitHubUsername();
if (effective && effective.toLowerCase() !== githubUsername.toLowerCase()) { /* note */ }

b) initCommand is the one async action with no error handling, and this PR added a write to it. index.ts:160 is program.parse(), not parseAsync(), so commander never awaits the action's promise — a rejection is unhandled, which on Node 22 is a crash with a stack trace rather than the red one-liner every other command prints. Every other action guards internally; initCommand (index.ts:47-49) doesn't. clearPlayAsUsername() at init.ts:224 is a new uncaught writeFileSync on that path — it throws ENOTDIR in a linked worktree or a submodule, where .git is a file, not a directory. Either wrap the init action the way the login one at :70-84 is wrapped, or switch to parseAsync() with a top-level handler.

Minor, same call: clearPlayAsUsername() runs unconditionally, so choosing personal credit in a repo with no .git/slot-machine-config.json creates one containing {}.

c) init's privacy-mode path still skips the credit prompt (init.ts:202), so a repo first init'd as an org and later gone private keeps an invisible override that only whoami reveals. Raised in rounds four and five; nothing in the branch engages with it. It's narrow — the override can't be created in a private repo — so "deliberate, leaving it" is a fine answer, it just hasn't been said.

d) logout --all still makes N sequential revocations with no AbortSignal (api.ts:68-95), each trying the configured URL then the fallback domain. Unchanged, still low stakes — manual command, not the hook path. The missing timeout on sendPlayToAPI is on the hook path and is pre-existing; still worth its own issue.

e) The [3.1.0] CHANGELOG entry still credits the init hijack fix to 3.1.0, where it was live and only got fixed in round three. The 3.1.1 entry above it is accurate and supersedes it; this only matters if 3.1.0 is on npm.

f) The PR body was never refreshed. It still presents the file-permission finding as "Open — deliberately not fixed here" and asks the chmodSync-vs-version-gate question that was answered in round one and implemented five commits ago, and it reports 58 passed, 2 suites. Now that this is merged, the body is the permanent record of the change for anyone reading back.


Same caveat as every prior round: dependency install requires approval in this environment, so I did not run npm run build or npm test — everything above is from reading the source. The one to confirm by hand is the --force trace: hold two tokens, disconnect, run logout --all --force, and watch it report a logout that didn't happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant