Skip to content

fix: github-account hook converges the active account instead of blocking#99

Merged
levifig merged 4 commits into
mainfrom
github-account-autoswitch
Jul 7, 2026
Merged

fix: github-account hook converges the active account instead of blocking#99
levifig merged 4 commits into
mainfrom
github-account-autoswitch

Conversation

@levifig

@levifig levifig commented Jul 7, 2026

Copy link
Copy Markdown
Owner

What & Why

The github-account guard (shipped in v2.0.0-alpha.5) blocked every gh command on account mismatch β€” including the gh auth switch command its own error message prescribed as the remedy. In agent sessions, where hooks intercept all gh invocations, that's a deadlock: the state being checked can only be changed by the command class being blocked. Hit live on 2026-07-07 while opening PR #97.

Redesign per direction: the guard's job is to converge the environment on the configured account, not dead-end on it. On mismatch (or authenticated-but-no-active-account) the check now runs gh auth switch --hostname github.com --user <configured> itself, re-checks that the switch took, and passes with a visible warning naming the mutation β€” never silently. Blocking remains for the genuinely unrecoverable cases: gh not installed, status probe failure, or a failed switch (typically the account isn't authenticated) β€” the latter surfacing gh's own stderr plus gh auth login guidance.

Design notes:

  • Mismatch is a typed state (classifyGitHubAccount / resolveGitHubAccount with injected status/switch runners), not a parsed diagnostic string.
  • The read-only release-readiness report path deliberately keeps its non-mutating diagnostic β€” reporting surfaces state; enforcement converges it.
  • Hook timeout doubled (5s β†’ 10s) for the switch + re-check round-trips.

Review focus

  • The six behavioral branches and their tests in check_test.go: match β†’ no switch attempted; no-config β†’ no gh execution; mismatch β†’ switch β†’ pass-with-warning (asserts switch args and the re-check); switch-fails β†’ block with login guidance; gh missing / probe failure β†’ block unchanged.
  • The warning wording: switched active gh account to "<expected>" (was "<actual>") for this project β€” this mutation is global gh state, so visibility is the safety mechanism.
  • Known limitation, deliberately out of scope: the switch mutates the global active account, so concurrent sessions in other projects can race it (observed live β€” a parallel session flipped the account back between check and API call). The endgame is per-invocation identity (GH_TOKEN resolved per command), which is skill-guidance territory, not enforceable by a check hook β€” tracked for the hooks re-evaluation.

Verification

go test ./internal/cli -run "GitHubAccount|Check" -count=1
npm run test && npm run build
# live: run any gh command in a repo configured for the non-active account β€”
# it switches, warns, and proceeds; with an unauthenticated account it blocks with login guidance

Migration / breaking changes

Behavior change to the guard: gh commands under a mismatched account now proceed after an automatic switch (with warning) instead of failing. Projects relying on the hard block as an identity firewall should note the block now only fires when convergence is impossible.

levifig added 2 commits July 7, 2026 02:49
…king

The guard blocked every gh command on account mismatch β€” including the
gh auth switch it prescribed as the remedy, deadlocking agent sessions.
It now runs gh auth switch --hostname github.com --user <configured>
itself on mismatch (or when no account is active), re-checks, and passes
with a visible warning naming the switch. Blocking remains for the
unrecoverable cases: gh missing, status probe failure, or a failed
switch β€” the latter with gh's own stderr and gh auth login guidance.
Mismatch is a typed state, not a parsed diagnostic; the read-only
release-readiness report keeps its non-mutating wording. Hook timeout
doubled for the switch+recheck round-trips; hook docs updated.
levifig added 2 commits July 7, 2026 19:50
External review round 1 found the retargeting hazard: under a mismatch,
gh auth logout would first converge the global pointer to the configured
account and then log THAT account out, not the one the user targeted β€”
same misdirection class for gh auth switch and refresh. Identity
administration is the user's domain: when every gh invocation in the
command is an auth subcommand, the hook passes through with no probe and
no switch; compounds mixing auth with resource-touching gh usage still
converge. Docs sharpen the residual-risk disclosure per the same review:
convergence writes the shared pointer on every mismatched call β€” reads
included β€” raising collision frequency versus the old pure-read check;
the window's shape is unchanged, its trigger rate is not.
@levifig

levifig commented Jul 7, 2026

Copy link
Copy Markdown
Owner Author

External review β€” round 1 (Codex, off-PR) β€” findings and dispositions

Fresh-thread adversarial review of a fail-closed guard's behavior change; reviewer ran all tests, verified the six behavioral branches, reproduced the original deadlock from the old code, and confirmed the tier layering with PR #100. Verdict: approve, no blockers. Both should-fixes landed in d49b823a + 2cddd852.

# Severity Finding Disposition
1 Should-fix gh auth * retargeting hazard: under a mismatch, gh auth logout would first converge the pointer to the configured account and then log that account out β€” same misdirection for switch/refresh. Untested, undisclosed. Confirmed and fixed β€” identity administration now passes through untouched (no probe, no switch) when every gh invocation in the command is an auth subcommand; compounds mixing auth with resource-touching usage still converge. Five new test cases including the logout scenario.
2 Should-fix Residual-risk disclosure understated: convergence writes the shared global pointer on every mismatched call (the old check was a pure reader), so concurrent multi-identity sessions collide more often β€” the window's shape is unchanged, its trigger rate is not. Confirmed and fixed β€” hook-system.md and the git-workflow hook row now state the frequency increase explicitly, consistent with how PR #100 characterizes the same residual.
3 Nit Convergence fires for read-only commands (gh pr view) too β€” global mutation to satisfy a read. Accepted, documented in the same disclosure (by-design under the always-right-identity intent; one more reason tier 3 exists).

Reviewer also confirmed: the release-flow partition stays correct (enforcement converges, reporting stays read-only, release create's in-process pre-check is the smallest possible window), the 2Γ— timeout matches the 2Γ— probe cost, all doc mirrors rebuilt with zero drift, and no conflict with PR #100's layering β€” which demotes this convergence to tier-1 fallback by design.

@levifig
levifig merged commit c1c5459 into main Jul 7, 2026
1 check passed
@levifig
levifig deleted the github-account-autoswitch branch July 7, 2026 20:35
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