Skip to content

v3.2.0: scoped config setters, one identity key - #17

Open
than wants to merge 7 commits into
mainfrom
v3.2.0-config-scopes
Open

v3.2.0: scoped config setters, one identity key#17
than wants to merge 7 commits into
mainfrom
v3.2.0-config-scopes

Conversation

@than

@than than commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Closes #13. Plan: ~/.claude/plans/git-slot-machine-3.2.0-config-scopes.md

Behavior change to call out

sync:disable now silences only the current repo. It previously wrote the global config, silencing every repo at once — the symptom that opened #13. sync:disable --global restores the old behavior. Intended fix, but it will surprise anyone who relied on the old default.

What changed

  • One scoped setter. Each setter previously hardcoded exactly one scope, and the split was arbitrary (sync global-only, privateRepo repo-only). All five now route through a single setValue(key, value, scope) over the existing load/save pairs — no new IO paths. sync:* and privacy:* default to repo, username:set and config:set api-url default to global; every one takes --global/--repo and prints the file it wrote.
  • Identity is one key. playAsUsername and githubUsername were the same concept under two names, which is why getGitHubUsername() needed a bespoke resolver instead of the merge that already handles every other key. Collapsed into githubUsername, resolved through getConfig(). Repo configs migrate on first read — idempotent, no write when there's nothing to migrate (this runs on every post-commit play), and the in-memory result still stands if the write-back fails.
  • api-url stays global-only and rejects --repo. getApiUrl() reads global config only so a .git/slot-machine-config.json can't redirect an authenticated sync; a repo-scoped value would be silently inert, so writing one would be a lie.
  • privateRepo reads through the merge, so a global privateRepo: true means "default all my repos to private". No existing config sets it globally — no-op on upgrade.
  • New privacy:on / privacy:off. Falls out of the scoped setter for free. Privacy mode was only settable during init, so re-running init was the only way to change it.
  • whoami marks which file owns each setting, and names the global value when a repo overrides it.

Fixes found along the way

  • init suppressed the org-credit prompt under privacy mode (!usePrivacyMode in the guard) — the reason ~/Broomfitters/house needed playAsUsername hand-written into its repo config. Privacy hides the repo; the username is sent either way, as init already tells the user.
  • init was reading the obfuscated remote for its own local decisions. With privacy mode already on, getRepoInfo() returns private/private — so a re-run queried api.github.com/repos/private/private and would have offered to credit an org named "private". Added getRemoteRepoInfo() for local-only decisions; anything server-bound still goes through getRepoInfo().
  • Repo-scoped writes outside a git repo reported a raw ENOENT. Now: "not a git repository — use --global".
  • clearPlayAsUsername() deletes both keys. The plan didn't mention the clear path; leaving it on the legacy key would have silently regressed the 3.1.1 hijack fix, since init's "personal credit" branch is the only way back from an org override.

Verification

86 tests (was 71), tsc --noEmit clean, pnpm build clean. 15 new tests cover the repo migration (including the idempotent no-write path, the write-failure path, and token resolution through a migrated identity), scope isolation in both directions, and the clear path.

Live-run against the real configs (backed up first, then restored):

  • ~/Broomfitters/houseplayAsUsername: "broomfitters" became githubUsername: "broomfitters" on first read; whoami output unchanged.
  • sync:disable in house wrote only .git/slot-machine-config.json; global untouched, other repos still enabled.
  • sync:disable --global wrote global and every repo reported disabled.
  • Identity unchanged throughout: global than, house plays as broomfitters.
  • Guards: non-git directory, --global --repo together, api-url --repo.

Not run interactively: the init org-credit prompt under privacy mode (plan verification step 6). init calls the GitHub API and authLoginCommand, so a scripted run would create real leaderboard state. The change is the one-line guard at init.ts plus the raw-remote switch; verified by code review and covered indirectly by the migration tests, not by an interactive run.

Out of scope

hasRepoConfigTarget() returns true when .git is a file (worktrees, submodules), where saveRepoConfig then fails with ENOTDIR. Pre-existing — setPrivateRepo always had it — not a regression from this change.

🤖 Generated with Claude Code

than and others added 2 commits August 6, 2026 15:03
Every setting is now settable at either scope through a single `setValue`
helper, with the default chosen to match what people mean by the bare
command: sync and privacy are about this repo, identity and api-url about
the user. Each command prints the file it wrote.

`sync:disable` previously wrote the global config, silencing every repo at
once; it now silences only the current one, with --global to restore the
old behavior.

`playAsUsername` and `githubUsername` were one concept under two names,
which is why identity needed a bespoke resolver instead of the normal
repo-overrides-global merge. Collapsed into `githubUsername`, with a
one-shot idempotent migration on read.

Also fixes init suppressing the org-credit prompt under privacy mode (the
reason a private org repo had to be hand-edited) and init reading the
obfuscated private/private remote for its own local decisions.

Closes #13

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review round 1 on #17.

Repo config assumed `cwd/.git`, which only resolves at the repo root of an
ordinary checkout. That was survivable while repo scope was reserved for
init-time settings; as of this branch it is the default for sync and
privacy, so it became a setting the CLI writes and then can't find: from a
subdirectory `getRepoConfig()` returned `{}` and the merge fell back to the
global defaults, so a repo with sync disabled synced and a private repo
sent its real owner and name. In a worktree or submodule `.git` is a file,
so `hasRepoConfigTarget()` passed and the write failed with ENOTDIR.

Resolves via `git rev-parse --absolute-git-dir`, cached per cwd, behind a
fast path for the root case that the post-commit hook always hits.

`init` seeded `usePrivacyMode` from nothing, so a re-run with privacy
already on took the public branch, printed "✓ Public repository confirmed"
and claimed the repo URL, owner and name were sent while getRepoInfo() was
still sending private/private.

Also wraps `username:set` in the error handler the other commands have —
its `--repo` path could surface an unhandled rejection.

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

than commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Review round 1 — all 4 findings fixed (8ff6efd)

Three of the four shared one root cause: getRepoConfigPath() assumed cwd/.git.

1 + 2 + 3 — repo config now resolves the real git directory. cwd/.git only resolves at the repo root of an ordinary checkout. That was survivable while repo scope was reserved for init-time settings; as of this branch it's the default for sync and privacy, so it became a setting the CLI writes and then can't find — from a subdirectory getRepoConfig() returned {} and the merge fell back to the global defaults. A repo with sync disabled synced anyway, and a private repo sent its real owner and name. In a worktree or submodule .git is a file, so hasRepoConfigTarget() passed and the write failed with ENOTDIR.

Now: git rev-parse --absolute-git-dir, cached per cwd, behind a statSync().isDirectory() fast path for the root case — which is where the post-commit hook always runs, so the hot path adds no subprocess.

4 — init no longer claims "✓ Public repository confirmed" with privacy on. usePrivacyMode is seeded from isPrivateRepo(), so a re-run skips the visibility check and its prompt, and the closing "what gets sent" summary reports what is actually sent.

Also: username:set got the try/catch the other commands have — its --repo path could surface an unhandled rejection past the guard.

Verification: 89 tests (was 86), tsc --noEmit clean, pnpm build clean. Three new tests pin the resolution: repo config found from a subdirectory, a real git worktree add where .git is a file (isFile() asserted, then a write that previously threw ENOTDIR), and saveRepoConfig throwing "Not a git repository" outside a repo. Confirmed live: sync:disable at the root then whoami from src/commands/ reports disabled (per-repo).

Not changed, as suggested: the double repo-file write when a repo-scoped setValue follows the migration write-back (idempotent), and getRepoConfig() returning {} on corrupt JSON (pre-existing).

Review round 2 on #17.

Round 1 resolved the git dir with `--absolute-git-dir`, which is
per-worktree. Per-repo settings are properties of the repository, not of a
checkout — and hooks live in the common dir, so a hook installed from the
main checkout fires in every worktree. Splitting the config reintroduced
round 1's own bug: `privacy:on` at the main root wrote a config the
worktree couldn't see, so the merge fell back to the global defaults and
the private repo sent its real owner and name. Now `--git-common-dir`,
resolved against cwd because git returns it relative from inside an
ordinary checkout and absolute from a linked worktree.

The round 1 test only asserted where the write landed, so it encoded the
split rather than catching it. It now sets privacy in the main checkout and
reads it from the worktree, and sync the other way.

init's hookPath had the same `cwd/.git` assumption and this branch made the
crash newly reachable: in a worktree the write threw ENOTDIR after init had
already prompted for and persisted the privacy answer.

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

than commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Review round 2 — both findings fixed (c8dd725)

Both were mine from round 1, and the first is round 1's own bug in a new disguise.

1 — repo config now resolves --git-common-dir. I used --absolute-git-dir, which is per-worktree. Per-repo settings are properties of the repository, not of a checkout, and hooks live in the common dir — so a hook installed from the main checkout fires in every worktree and must find the same config. As written, privacy:on at the main root wrote a config the worktree couldn't see, getRepoConfig() returned {}, and the merge fell back to the global defaults: the private repo sending its real owner and name again.

Resolved against cwd via path.resolve, because git returns --git-common-dir relative from inside an ordinary checkout (.git, ../../.git) and absolute from a linked worktree. --path-format=absolute would do it in one step but only on git 2.31+.

Your point about the test was right and worth more than the finding: it only asserted where the write landed, so it encoded the split rather than catching it. It now sets privacy in the main checkout and reads it from the worktree, then sync the other way, and asserts the file is in the main .git.

2 — init installs the hook into the common git dir. Same cwd/.git assumption, and you're right that this branch made the crash newly reachable — the write threw ENOTDIR after init had prompted for and persisted the privacy answer, where previously the config write failed first. getGitCommonDir() is exported now and used for both; mkdirSync(recursive) covers a common dir without a hooks/.

Verification: 89 tests, tsc --noEmit clean, pnpm build clean. Confirmed live on a real git worktree add of this repo: privacy:on at the main checkout, then whoami from the worktree reports Privacy mode: on (per-repo) and This repo: (private — not sent to server). Worktree removed after.

Review round 3 on #17.

`username:set <name> --repo` is new in this branch and lets a repo be
credited to any name. Plays resolve their token through that name, so
setting one you hold no token for makes every commit's sync fail — and the
failure is invisible: play.ts swallows the error, and its "not
authenticated" notice is gated off --small, which is the post-commit hook's
only mode. It now warns where the choice is made, and the README documents
the login step alongside the --repo example.

init's credit prompt keyed on owner-vs-personal, which pre-3.2 was the only
way an override could exist. With --repo it isn't: in `than/my-app`
overridden to `broomfitters`, init skipped the question, authenticated as
than, and left plays credited to broomfitters. The candidate list now
includes the existing override as its own option.

That list moved to utils/credit.ts to be testable — init.ts imports chalk,
which is ESM-only and can't be loaded by this repo's jest transform.

isPrivateRepo() is merged now, so init's "already enabled for this repo"
could be reporting the global default; it says which.

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

than commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Review round 3 — all 3 findings fixed (63d5bd5)

Findings 1 and 2 are two halves of the same hole this branch opened: username:set <name> --repo lets a repo be credited to a name you hold no token for.

2 — username:set now warns when no token is held for that name. Verified the chain you described: play.ts:163-165 swallows the sync error, and the play.ts:116 "not authenticated" notice is gated off --small, which templates/post-commit.ts hardcodes. So the failure really is total silence. Warning at the point the choice is made rather than un-gating the notice — that gating is a deliberate 3.1.1 decision (the hook's output is a single-line contract). README's --repo example now carries the login step.

$ git-slot-machine username:set ghost --repo
Commits in this repo will be credited to ghost

No token held for ghost — plays stay local until you log in.
  git-slot-machine login ghost

1 — init's credit prompt now keys on the candidate set, not owner-vs-personal. You're right that pre-3.2 the old guard was sufficient, because playAsUsername was only ever written as the repo owner. It isn't anymore. The prompt builds its list from personal + owner + existing override, deduped case-insensitively, personal always first so choice 1 stays the reset. In than/my-app overridden to broomfitters it now offers both instead of skipping.

Extracted to utils/credit.ts to make it testable — init.ts imports chalk, which is ESM-only and blows up this repo's jest transform. 7 new tests cover the case that was unreachable before, owner-equals-override dedup, and case-insensitivity.

3 — init says which scope privacy came from. isPrivateRepo() is merged now, so "already enabled for this repo" could be reporting the global default; it distinguishes the two and points at privacy:off either way.

Verification: 96 tests (was 89), tsc --noEmit clean, pnpm build clean, warning confirmed live.

@than than closed this Aug 7, 2026
@than than reopened this Aug 7, 2026
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review round 4 — one real hole, plus small stuff

Rounds 1–3 landed well. --git-common-dir is the right call (per-repo settings are properties of the repository, and hooks already live there), and the round-2 test rewrite — set privacy in the main checkout, read it from the worktree — is the version that would actually catch a regression rather than encode one. Extracting creditCandidates turned an untestable branch into seven tests. The scope model itself reads clean: one setValue over the existing load/save pairs, defaults picked per command, and api-url deliberately outside the model because a repo-scoped value would be inert. getApiUrl() and getApiToken() staying global-only is the correct place to draw that line.

One finding worth fixing before merge.

login <name> after username:set <name> --repo still adopts the name globally

Same shape as round 3's finding — a hole --repo identities opened — but on the far side of the warning you added.

index.ts:82-84:

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

The !globalUsername escape hatch is 3.1.1 behavior and was safe then: the only writer of a per-repo override was init, and init sets the global identity at init.ts:98 before it ever reaches the credit prompt. "A repo override exists but no global identity does" was unreachable.

username:set <name> --repo is a standalone command now, and it writes only the repo config. On a machine that has not run init:

$ git-slot-machine username:set myorg --repo
Commits in this repo will be credited to myorg

No token held for myorg — plays stay local until you log in.
  git-slot-machine login myorg

Following that instruction hits !globalUsernamepersist = truesetGitHubUsername("myorg") at global scope. myorg is now the global identity: every other repo on the machine credits its plays to the org, and a bare logout targets it. That is the identity hijack CONTEXT.md defines, reached by following the remedy this PR prints.

Suggested fix — narrow the escape hatch so it does not cover the one name this repo already routes elsewhere:

const globalUsername = getGlobalConfig().githubUsername;
const repoOverride = getPlayAsUsername();
const persist = globalUsername
  ? globalUsername.toLowerCase() === githubUsername.toLowerCase()
  : repoOverride?.toLowerCase() !== githubUsername.toLowerCase();

"First login establishes you" survives for every other case. Logging in as the name this repo is explicitly credited to stores the token and leaves the global identity unset — which auth.ts:55-61 already prints a note for.

Lower priority

init still prompts before it knows it can write. Round 2 moved the hook write behind getGitCommonDir(), but the guard at init.ts:185 sits below the first repo write: setPrivateRepo(true) at :152 and :170 runs immediately after the privacy prompt and goes through saveRepoConfig, which throws Not a git repository when the path will not resolve. And init is the one action in index.ts without the try/catch login and username:set have (:53-55) — program.parse() is not awaited, so anything thrown in there surfaces as an unhandled-rejection stack instead of the red one-liner every other command prints. A plain EACCES on fs.writeFileSync(hookPath, …) at :204 gets you there too, and that one does not need an exotic setup. Hoisting the gitDir resolution above the privacy block and wrapping the action closes both. Hard to reach for the config path given isGitRepo() upstream, so not a blocker.

The changelog overstates the output. "All of them print which file they wrote" — configSetCommand prints globally / for this repo (commands/config.ts:43), and username:set --global prints GitHub username set to: X with no scope marker at all. Since whoami now exists precisely to answer "which file", either print the resolved path or reword to "which scope".

config:set private-repo <anything> coerces to false. commands/config.ts:106 accepts only true/1, so yes, on, enabled all land as disabled. It does print "Privacy mode disabled", so it is not silent — but for a privacy key, rejecting an unrecognized value beats guessing. sync-enabled has the same shape and is pre-existing.

username:set dynamic import is redundant. index.ts:170 does await import("./config.js") for setGitHubUsername/getApiTokenFor, but index.ts:20 already imports getGlobalConfig statically from that same module.

On round 1 deferred corrupt-JSON item — still fine to leave, but the calculus shifted. setValue reads through getRepoConfig(), which returns {} on a parse failure, so the following saveRepoConfig truncates the file. Pre-3.2 the only repo-scoped writer was init; now sync:disable, privacy:on, and username:set --repo all take that path, so a hand-edited config with a trailing comma gets quietly reset by a routine command. Follow-up issue, not this PR.

Nothing else: no any, relative imports keep their .js extensions, sync stays gated behind isApiAvailable() and swallows failures on both the offline and unauthenticated paths, patterns.ts is untouched so there is no CLI/backend divergence to check, and no new output lands in --small.

I could not run tsc/jest in this sandbox, so the 96-tests-green claim is taken on report; the findings above are from reading.

Review round 4 on #17, from the GitHub Actions reviewer.

The `!globalUsername` escape hatch in login was safe through 3.1.1 because
init was the only writer of a per-repo override and it sets the global
identity first, so "an override exists but no global identity does" was
unreachable. `username:set <name> --repo` is standalone and writes only the
repo config, so on a machine that has never run init, following the "log in
as <name>" hint this branch prints adopted the org globally — every other
repo crediting its plays to it, and a bare logout targeting it. The hijack
two releases have been closing, reached through the remedy for round 3's
finding.

The rule is now a tested function: an established identity still decides, a
first login still establishes you, except for the one name this repo
already routes elsewhere.

Also from that review: config:set rejects unrecognized booleans rather than
coercing to false (`private-repo yes` turned privacy off); init resolves the
git dir before its first prompt, so a failure can't leave a saved privacy
answer with no hook, and its action has the try/catch every other command
has (program.parse() isn't awaited, so a throw was an unhandled rejection);
`username:set --global` names its scope; the changelog no longer claims
commands print the file they wrote when they print the scope.

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

than commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Review round 4 — fixed (a242184)

The finding is right, and it's the sharpest one yet: the hijack is reached by following the remedy round 3 added. username:set myorg --repo prints "log in as myorg", and on a machine that has never run init that login adopted myorg as the global identity.

Your reading of why the escape hatch was safe before is exactly it — init sets the global identity at init.ts:98 before the credit prompt, so "override exists, no global identity" was unreachable until --repo became a standalone command.

Took the suggested shape, extracted to utils/credit.ts as shouldPersistIdentity with 6 tests, including the one that would have caught this and the case it must not break: logging in as the global identity from an overridden repo is a re-auth, not an adoption, and the override must not block it.

Also fixed:

  • config:set rejects unrecognized booleans. Agreed on direction — for a privacy key, guessing is worse than refusing. Both keys now take true/1/yes/on/enabled and their negatives and exit 1 on anything else, listing what's accepted. private-repo yes enables privacy now instead of disabling it.
  • init resolves the git dir before its first prompt, above setPrivateRepo, not at the hook write — you're right that round 2's guard sat below the first repo write, so the failure mode was a saved privacy answer with no hook. Its action also has the try/catch the others do; program.parse() isn't awaited, so a plain EACCES on the hook write was an unhandled-rejection stack.
  • username:set --global says "set globally to". Changelog no longer claims commands print the file they wrote — they print the scope; whoami prints the file.
  • Redundant dynamic import in username:set replaced with the static one.

Deferred, as you suggested: corrupt repo JSON being truncated by setValue. Its blast radius did grow — three commands take that path now, not just init — so it's tracked as a follow-up rather than left implicit.

Verification: 102 tests (was 96), tsc --noEmit clean, pnpm build clean. Boolean rejection and scope wording confirmed live.

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review round 5 — no blockers; the round-4 fix is correct

shouldPersistIdentity is the right shape, and the test that matters is there: shouldPersistIdentity('than', 'broomfitters', 'than') === true, i.e. logging in as your own global identity from an overridden repo is still a re-auth, not blocked by the override. That was the case the narrowing could easily have broken, and it's pinned.

Re-verified the rest of the round-4 items against the tree rather than the description:

  • parseBoolean rejects rather than guesses; private-repo yes now enables privacy.
  • init resolves getGitCommonDir() at init.ts:105, above setPrivateRepo at :152/:170 — the first repo write is now behind the guard, not in front of it — and the action has the try/catch at index.ts:60-68, so an EACCES on the hook write prints the red one-liner instead of an unhandled-rejection stack.
  • The dynamic import in username:set is gone; setGitHubUsername/getApiTokenFor come from the static import at index.ts:20-25.
  • Changelog no longer claims commands print the file they wrote.

Also spot-checked the invariants that matter outside the diff: patterns.ts and patterns.json are untouched, so there is no CLI/backend divergence to check this round; no any anywhere in src; every relative import carries its .js; no new output reaches --small; sync is still gated behind isApiAvailable() and swallows on both the offline and unauthenticated paths.

Everything below is minor.

The changelog claim moved rather than went away. whoami prints (per-repo) / (global) / (default) — scopes, not files. CHANGELOG now says "each accepts --global/--repo ... whoami names the file", and README:125 says "Show which file owns each setting". Neither is true as written. Either reword both to "scope", or have whoami print the resolved getGlobalConfigPath() / getRepoConfigPath() — the latter is genuinely more useful now that .git is not always where you would guess (worktrees resolve to the common dir).

setValue is typed over all of keyof Config. config.ts:274 accepts any key including apiTokens and apiToken, so nothing but convention stops a repo-scoped token write — and "tokens are keyed by identity, not by directory" is an explicit decision in the issue's out-of-scope section. Narrowing the parameter makes the compiler hold it:

type ScopedKey = 'githubUsername' | 'apiUrl' | 'syncEnabled' | 'privateRepo';
function setValue<K extends ScopedKey>(key: K, value: Config[K], scope: Scope): void

All four current callers already pass one of those, so it is a type-only change.

Value validation runs after the repo-target check. commands/config.ts:114-116: config:set sync-enabled maybe outside a git repo reports "not a git repository" and exits, never mentioning that maybe is not a value — so fixing the directory just surfaces a second error. Hoisting parseBoolean above requireRepoScopeTarget reports the argument you actually got wrong first. Same in the private-repo branch at :122-124.

username:set --global does not mention an override that is still in effect. auth.ts:57-67 prints "Note: this repo still credits plays to X" when a login did not adopt the name — the right note. username:set than --global in a repo overridden to myorg prints nothing equivalent, and its token warning at index.ts:180 keys on the name just set, so if you hold a token for than there is no output at all, while plays in this repo still resolve to myorg and fail silently. The same getPlayAsUsername() check auth.ts already does would cover it.

Residual on the round-4 hole, and I think it is acceptable as-is. The narrowing keys on this repo's override, so the hijack is still reachable one directory over: fresh machine, no global identity, username:set myorg --repo in repo A, then login myorg from repo B or from outside a repo — getPlayAsUsername() returns null there, persist is true, and myorg becomes the global identity. Closing it properly would need to know every repo's override, which is not local state, and from repo B's vantage point this is genuinely indistinguishable from a first login. The printed remedy runs in the right directory, so the realistic path is covered. If you want to shrink it further, the cheap move is at the other end: username:set <name> --repo on a machine with no global identity could say to set your personal identity first — a repo override with no global identity behind it is a half-configured machine regardless.

Same caveat as round 4: no network in this sandbox, so node_modules is not installed and I could not run tsc or jest. The 102-tests-green claim is taken on report; everything above is from reading.

Review round 5 on #17 — no blockers, five minor items, all taken.

whoami printed scopes while the changelog and README claimed it named
files. Rather than downgrade the wording, it prints both config paths —
worth having now that repo config resolves to the *common* git dir, which
in a worktree is not the .git next to you.

setValue took all of `keyof Config`, so only convention kept a token out of
a repo config. A `ScopedKey` union makes the compiler hold the line the
issue drew: a token belongs to an identity, not a directory.

parseBoolean runs before the repo-target check, so `sync-enabled maybe`
outside a repo reports the argument you got wrong rather than the directory
and then a second error once you fix it.

`username:set <name> --global` in an overridden repo said nothing, while
its token warning keyed on a name plays here never resolve to — so setting
a name you already hold a token for printed nothing at all. It now prints
the note auth.ts already prints after a non-adopting login.

On the residual round-4 hole the reviewer judged acceptable (login from a
*different* directory can still adopt an org set with --repo elsewhere):
taken the cheap end they suggested — `username:set --repo` with no global
identity says to set a personal one first.

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

than commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Review round 5 — all five minor items taken (0d99e0b)

The "which file" claim. Took the second option — whoami prints both paths now rather than the wording being downgraded. Your reasoning is why: repo config resolves to the common git dir, so in a worktree it isn't the .git next to you, and "which scope" doesn't answer "which file do I edit".

Global config:    /Users/than/.git-slot-machine/config.json
Repo config:      /Users/than/Sites/git-slot-machine/git-slot-machine/.git/slot-machine-config.json

CHANGELOG and README:125 now say "scope" for the marker and describe the paths separately.

setValue over keyof Config. Taken as suggested — ScopedKey union, type-only, all four callers already conform. The issue drew that line in prose and only convention held it; now the compiler does.

Validation order. Both branches hoist parseBoolean above requireRepoScopeTarget.

username:set --global silent about a live override. Good catch, and the interaction with the token warning is the sharp part — keying on the name just set means holding a token for it produced no output while plays here still resolved to the override. It now prints the same note auth.ts:57-67 prints, plus the --repo command to change it.

The residual hole. Agreed it's not closable locally — from repo B there is genuinely nothing to distinguish it from a first login, and inventing a global registry of every repo's override would be worse than the bug. Took your cheap end instead: username:set <name> --repo with no global identity now says to set a personal one, since that's the state that makes the hole reachable at all.

$ git-slot-machine username:set myorg --repo
Commits in this repo will be credited to myorg

No global identity set yet — other repos have nobody to credit.
  git-slot-machine username:set your-personal-username

Verification: 102 tests, tsc --noEmit clean, pnpm build clean. whoami paths and both new notices confirmed live.

Noted on the sandbox caveat — for what it's worth, the test/typecheck numbers in these replies are from local runs, and CI runs tsc, jest and the build on every push to this branch.

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review round 6 — no blockers; one gap the branch’s own hint walks into

The round-5 items all landed, verified against the tree rather than the description:

  • whoami prints Global config / Repo config paths (whoami.ts:91, :96) alongside the scope markers, so the CHANGELOG/README "which file" claim is now true — and it is the more useful of the two options given repo config resolves to the common git dir.
  • ScopedKey (config.ts:277) is the union, and setValue<K extends ScopedKey> (:281) holds it. All four callers pass githubUsername / apiUrl / syncEnabled / privateRepo; a repo-scoped token write no longer typechecks.
  • parseBoolean runs before requireRepoScopeTarget in both branches (commands/config.ts:116-118, :124-126).
  • username:set --global prints the live-override note (index.ts:204-207); username:set --repo with no global identity prints the personal-identity hint (:212-216).

Also re-checked the invariants outside the diff: patterns.ts/patterns.json untouched, so no CLI/backend divergence this round; no any in src; every relative import carries .js; no new output reaches --small (all the new prints are in username:set, config:set, whoami, init); and sendPlayToAPI still gates on isApiAvailable() (api.ts:99:57 = isSyncEnabled() && getApiToken() !== null) and swallows on both the offline and unauthenticated paths — so the headline sync:disable behavior change actually reaches the network layer.

One finding.

Nothing can remove a per-repo override, and the new note recommends creating a permanent one

clearPlayAsUsername() has exactly one caller: init.ts:276, inside the personal branch of the credit prompt. That was proportionate when init was the only writer of an override. This branch makes username:set <name> --repo a standalone creator, so the asymmetry is now load-bearing — and the round-5 note points straight at it:

$ git-slot-machine username:set than --global      # in a repo overridden to myorg
GitHub username set globally to: than
Note: this repo still credits plays to myorg.
  git-slot-machine username:set than --repo

That suggestion does not restore inheritance — it writes a repo githubUsername equal to the current global one. The repo is now pinned: change your global identity later and it silently keeps crediting than, and the same note fires again recommending the same pin. Compare auth.ts:63, which the comment at index.ts:203-206 says it is mirroring — that one suggests git-slot-machine init, which is the actual un-pin path.

And init cannot always undo it:

  • it exits at init.ts:61-71 when getRemoteRepoInfo() is null, so a repo with no GitHub remote (or a non-GitHub one) has no path at all — even though username:set --repo only requires hasRepoConfigTarget(), which such a repo passes;
  • the prompt is gated on candidates.length > 1 (init.ts:252). For a redundant pin created by following the hint above in your own repo — personal than, owner than, override thancreditCandidates dedupes to a single entry, the prompt is skipped, and clearPlayAsUsername() never runs.

So that pin is unreachable from the CLI; hand-editing .git/slot-machine-config.json is the only way out, which is the exact state #13 opened about. It also feeds shouldPersistIdentity(undefined, "than", "than") === false, so on a machine where the pin predates any global identity, login than in that repo will not establish one either.

Cheapest fix is at the hint: point at the clear path instead of the pin. But since --repo is a first-class creator now, the symmetric move is to add the clear — e.g. username:set --repo --clear or username:unset --repo wired to the clearPlayAsUsername() that already exists and is already tested (config.test.ts, "the per-repo override clear path"). Either way the note should stop recommending a value that no longer tracks the global identity.

Credit where due: whoami does surface the state — a redundant pin still prints Playing as than (per-repo override) — so it is discoverable, just not fixable.

Nits

  • The credit prompt resets on any unparseable input. init.ts:270: candidates[Number(choice) - 1] ?? candidates[0], and choice 1 is the branch that clears the override. With two options and a [1] default that reads as intended; with three (now reachable via owner + override) a typo of 4 silently drops an override the user never meant to touch. Treating only empty input as the default and re-asking otherwise would cost one loop. Same shape pre-3.2, so not a regression.
  • config:set api-url X --global --repo reports "api-url is global-only" (commands/config.ts:97) rather than the mutual-exclusion error, because the --repo check runs before resolveScope. Cosmetic.

Pre-existing, worth its own issue

fetchWithFallback (api.ts:66-91) passes no AbortSignal, so fetch inherits the undici default header/body timeouts (~300s), and the loop tries each URL in urlsToTry in sequence. On a network that blackholes rather than refuses — captive portal, VPN, corporate firewall — the post-commit hook can sit for several minutes per commit. It never throws and never corrupts the local play, so it does not violate the swallow-everything rule, but it does block the commit, and AbortSignal.timeout(3000) on the request is a one-liner. Untouched by this PR and not a regression from it; filing it alongside the corrupt-repo-JSON follow-up seems right.

Nothing else. The scope model reads well, the ScopedKey narrowing is the kind of thing that keeps a prose decision from rotting, and the worktree test remains the strongest thing in the diff.

Same sandbox caveat as rounds 4–5: no network here, so pnpm install fails and I could not run tsc or jest. The 102-tests-green claim is taken on report — noted that CI runs all three on every push to this branch. Everything above is from reading.

Review round 6 on #17 — no blockers, one gap worth the fix.

`clearPlayAsUsername()` had exactly one caller: init's credit prompt. That
was proportionate while init was also the only creator of an override. This
branch made `username:set <name> --repo` a standalone creator without a
standalone remover, and init cannot always undo one — it exits when there is
no GitHub remote, and skips the prompt entirely when the candidates dedupe
to one, which is exactly what a redundant pin produces. Hand-editing
.git/slot-machine-config.json was the only way out: the state #13 opened on.

Worse, round 5's own note walked people into it. `username:set than
--global` in an overridden repo suggested `username:set than --repo`, which
does not restore inheritance — it pins the repo to today's global name, so
changing the global identity later silently leaves the repo behind and
fires the same note again recommending the same pin. It now points at the
clear.

Nits from the same review: init's credit prompt re-asks on unparseable
input instead of falling through to choice 1, which is the branch that
*clears* an override — survivable with two options, less so now that owner
plus override can make three. Empty input still takes the default, so a
closed stdin terminates. And `config:set api-url --global --repo` reports
the contradiction rather than the narrower global-only rule.

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

than commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Review round 6 — fixed (07c5bb5)

The finding is right, and the part that stings is that round 5's own note walked people into it. username:set than --repo doesn't restore inheritance — it pins the repo to today's global name, so changing the global identity later silently leaves the repo behind, and the same note fires again recommending the same pin.

Took the symmetric option rather than just rewording the hint, because your two reasons why init can't be the answer are both real: it exits at init.ts:61-71 without a GitHub remote (which username:set --repo never required), and candidates.length > 1 skips the prompt for exactly the redundant pin the hint produced. Hand-editing .git/slot-machine-config.json really was the only way out — the state #13 opened on.

git-slot-machine username:unset — wired to the existing, already-tested clearPlayAsUsername():

$ git-slot-machine username:set than            # in a repo overridden to myorg
GitHub username set globally to: than
Note: this repo still credits plays to myorg.
  git-slot-machine username:unset    (to inherit the global one)

$ git-slot-machine username:unset
Cleared this repo's override (was myorg)
Commits here are credited to than now.

$ git-slot-machine username:unset
This repo has no username override.

It names what it cleared and what the repo inherits now, and says so if there's no global identity to fall back to.

Nits, both taken:

  • The credit prompt re-asks on unparseable input. Agreed on why it matters more now — choice 1 is the branch that clears, and owner + override makes three options reachable. Empty input still takes the default, which is also the EOF answer, so a closed stdin terminates rather than spinning.
  • config:set api-url --global --repo reports the mutual-exclusion error; resolveScope runs before the global-only check.

Filed as follow-ups, not this PR — agreed on both:

  • fetchWithFallback has no AbortSignal, so a blackholing network (captive portal, VPN) can block the post-commit hook for minutes per URL. Pre-existing and untouched here, but AbortSignal.timeout is a one-liner and it blocks the commit.
  • Corrupt repo JSON silently truncated by setValue, from round 4.

Verification: 102 tests, tsc --noEmit clean, pnpm build clean. Pin → note → unset → idempotent-unset confirmed live, plus the flag conflict.

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review round 7 — no blockers; one gap in the new remover

Round 6's items landed, verified against the tree:

  • username:unset (index.ts:245-274) is wired to the already-tested clearPlayAsUsername(), prints what it cleared and what the repo inherits, and no-ops with a message when there is no override. config.test.ts:481-513 covers the 3.2 key, the unmigrated legacy key, and — the one that matters for a standalone remover — that other repo settings survive the write.
  • The round-5 note at index.ts:209-212 now points at username:unset instead of the pin, and the comment above it records why, so the next person doesn't re-suggest username:set <global name> --repo.
  • The credit prompt re-asks (init.ts:270-286): only '' takes the default, and non-numeric input is rejected rather than falling into candidates[0], which is the clearing branch.
  • config:set api-url --global --repo reports the mutual exclusion — resolveScope runs at commands/config.ts:103, above the global-only check at :108.
  • README documents username:unset in both the command list (:126) and the org-credit section (:265).

One finding.

username:unset can move a repo to a tokenless identity without saying so

Round 3 established the rule at the point of choice: username:set <name> --repo warns when no token is held for that name, because the failure downstream is total silence — play.ts:163-165 swallows the sync error, and the "not authenticated" notice at play.ts:116 is gated off --small, the post-commit hook's only mode. index.ts:227-231 still does that, and username:set --global gets it too.

The new remover doesn't. index.ts:262-269:

const inherited = getGlobalConfig().githubUsername;

console.log(chalk.green(`Cleared this repo's override (was ${cleared})`));
console.log(
  inherited
    ? chalk.dim(`Commits here are credited to ${inherited} now.`)
    : chalk.yellow('No global identity set — run: git-slot-machine username:set <name>')
);

The !inherited branch is covered. The inherited branch isn't: it announces the new effective identity without checking whether a token is held for it. Clearing an override is exactly as much a change of effective identity as setting one, and the repo may well have been syncing fine a second earlier under the override's token.

"Global identity set, no token for it" is not exotic — logout clears the token and leaves githubUsername (auth.ts:139; the username is only read, at :120), and username:set <name> --global establishes a name with no token by design. Worse, this branch's own hint chain walks a fresh machine into it:

$ git-slot-machine username:set myorg --repo
No global identity set yet — other repos have nobody to credit.
  git-slot-machine username:set your-personal-username      # index.ts:217-221

$ git-slot-machine username:set than
GitHub username set globally to: than
No token held for than — plays stay local until you log in.   # warned here, good
Note: this repo still credits plays to myorg.
  git-slot-machine username:unset    (to inherit the global one)

$ git-slot-machine username:unset
Cleared this repo's override (was myorg)
Commits here are credited to than now.                        # not warned here

That last step swaps a working sync (token held for myorg) for a silent one, and it is the step the previous command recommended. The next signal the user gets is the leaderboard not moving.

The fix is the check that is already three lines away, and getApiTokenFor is already imported at index.ts:25:

if (inherited && !getApiTokenFor(inherited)) {
  console.log();
  console.log(chalk.yellow(`No token held for ${inherited} — plays stay local until you log in.`));
  console.log(chalk.cyan(`  git-slot-machine login ${inherited}`));
}

Nits

  • The repo-target error tells username:unset users about a flag it doesn't have. requireRepoScopeTarget (commands/config.ts:38) prints "Run this from inside a repo, or use --global to set it for every repo." That is right for sync:* and privacy:*; username:unset (index.ts:251) has no --global, and following the advice gets you commander's unknown option. An optional message argument, or an inline check in username:unset, covers it.

  • auth.ts:62 still routes through init. The note after a non-adopting login offers git-slot-machine init # credit this repo to <name>. That is the pre-3.2 shape, and round 6's argument against relying on init applies here in the other direction: init exits at init.ts:61-71 without a GitHub remote, and the direct route since this branch is username:set <name> --repo. Its sibling note at index.ts:209-212 was updated to name the new command; this one wasn't revisited. Cosmetic, but the two notes now describe different worlds.

  • username:set <name> --repo where <name> is already the global identity still creates the redundant pin round 6 identified — it just isn't a dead end anymore. Since the write happens with both values in hand (globalUsername is read at index.ts:189), a one-line "this pins the repo; it won't follow later changes to your global identity — username:unset to inherit instead" would close the last way in.

  • On the EOF claim at init.ts:276-278. The comment says an empty answer "is also the EOF answer, so a closed stdin terminates the loop". I couldn't run node here to confirm, but as far as I know rl.question's callback is dropped rather than invoked with '' when the stream ends — the promise never settles and the process exits once the loop drains. Same outcome (no spin, which is what matters), different mechanism, so it is the comment worth a second look, not the code.

Re-checked the standing invariants outside the diff: patterns.ts / patterns.json untouched, so no CLI/backend divergence this round; no any in src; every relative import carries .js; the new prints all live in username:unset / username:set / init, none of which run under --small; sendPlayToAPI still gates on isApiAvailable() and swallows on both the offline and unauthenticated paths; and the hot path stays subprocess-free — a post-commit hook runs at the working-tree root, so getGitCommonDir()'s statSync fast path hits for an ordinary checkout, with the execSync fallback (worktrees) inside a try/catch that cannot reach the commit.

Same sandbox caveat as rounds 4–6: no network, so node_modules isn't installed and I could not run tsc or jest. The 102-tests-green claim is taken on report; everything above is from reading.

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.

v3.2.0: scoped config setters, collapse playAsUsername into githubUsername

1 participant