Skip to content

feat(credentials): load every owner's PAT at launch, failing closed per token - #127

Merged
twistedmelonman merged 3 commits into
mainfrom
claude/feat-load-all-owner-tokens-c4e8e106
Sep 18, 2026
Merged

twistedmelonman merged 3 commits into
mainfrom
claude/feat-load-all-owner-tokens-c4e8e106

Conversation

@twistedmelonman

Copy link
Copy Markdown
Member

Why

A fine-grained PAT is bound to one resource owner at creation, so no single token covers the fleet. Reading an org's repos with the personal token fails 403 on branch protection and 404 on the repo itself — whether that repo is public or private, because the boundary is ownership, not visibility.

Selecting one token per session from the launch directory therefore guarantees the wrong credential for any cross-owner work, which fleet probes and rollouts are by definition. The documented workaround was env -u GH_TOKEN, which falls back to the keyring OAuth token and defeats the per-owner routing the gh wrapper enforces.

What

Load all three PATs at launch as GH_TOKEN_SWM / GH_TOKEN_NOS / GH_TOKEN_TWM.

Additive. GH_TOKEN keeps its launch-directory selection, so nothing that reads it changes. This is option 1 from the token-architecture decision; option 3 (gh-wrapper routes per invocation) is the eventual target and is what #126 tracks.

Fetched once at launch rather than per use: a 42-repo probe would otherwise mean dozens of vault reads, making the correct path slower than the fallback it replaces — which is how workarounds get entrenched.

Also extracts _creds_read_token_ref() from _load_gh_token(). The retry/backoff/timeout handling was already correct and is now shared rather than duplicated. It prints to stdout, returns 1 on failure, and never exports or logs a value. _load_gh_token's behavior is unchanged.

Failing closed

Each token fails closed independently. A failed fetch exports the invalid sentinel, never an empty string — an empty value would let gh fall through to the user's keyring OAuth token (repo/workflow/admin:org), silently widening the agent's access on a transient vault failure. A partial failure poisons only its own variable.

Verification

This is the part that was previously blocked. The fail-closed path is the test that leaked tokens in an earlier session, so it was left unverified.

It is now covered safely: every assertion compares against the fixed sentinel literal or a stub-supplied value. The stub op never returns a real credential. A test that must echo a live token to prove itself is the test that leaks it.

8 new cases in test-credentials.sh (32/32), with a known-bad gate first:

Case Asserts
No OP_SERVICE_ACCOUNT_TOKEN nothing exported, op never called (known-bad gate)
All refs resolve each var loaded from its own ref
All refs fail every var gets the sentinel
One ref fails only that var gets the sentinel
Failure warns degraded session is visible
Already set preserved, and its ref never read

Verified by mutation, not just by passing — a green suite over broken code has burned this repo before:

Mutation Result
Fail open (empty instead of sentinel) 2 failures
Drop the already-set guard 3 failures
Point every var at one ref 3 failures

Full suite 133/133 across test-credentials, test-launch-dir-check, test-remote-session, test-wrapper. shellcheck -S info clean.

Pre-existing failure, not from this change

test-gh-token-permissions.sh reports Token authentication failed (got: 'twistedmelonman'). It reproduces identically on a clean tree (verified by stashing).

That failure is #126: GH_TOKEN resolves from the launch directory, so a session started in a smartwatermelon repo still authenticates as twistedmelonman. This PR supplies the per-owner tokens that fix will select among; it deliberately does not change GH_TOKEN selection.

Advances #126

https://claude.ai/code/session_017s2qrmkQaV54fbcMzB2KRm

…er token

A fine-grained PAT is bound to one resource owner at creation, so no single
token covers the fleet. Reading an org's repos with the personal token fails
403 on branch protection and 404 on the repo itself -- whether that repo is
public or private, because the boundary is ownership, not visibility.
Selecting one token per session from the launch directory therefore
guarantees the wrong credential for any cross-owner work, which fleet probes
and rollouts are by definition.

Load all three as GH_TOKEN_SWM/NOS/TWM at launch. Additive: GH_TOKEN keeps
its launch-directory selection, so nothing that reads it changes.

Fetched once at launch rather than per use. A 42-repo probe would otherwise
mean dozens of vault reads, making the correct path slower than the keyring
fallback it replaces -- which is how workarounds get entrenched.

Also extracts _creds_read_token_ref() from _load_gh_token(): the retry,
backoff and timeout handling were already correct and are now shared rather
than duplicated. It prints to stdout, returns 1 on failure, and never exports
or logs a value. _load_gh_token's behavior is unchanged.

Each token fails closed independently. A failed fetch exports the invalid
sentinel, never an empty string: an empty value would let gh fall through to
the user's keyring OAuth token (repo/workflow/admin:org), silently widening
the agent's access on a transient vault failure. A partial failure poisons
only its own variable.

Tests: 8 new cases in test-credentials.sh (32/32), covering the success path,
the all-fail and partial-fail closed paths, the warning, and the already-set
guard. A known-bad gate runs first. Every assertion compares against the
fixed sentinel literal or a stub-supplied value -- none prints a credential,
because a test that must echo a live token to prove itself is the test that
leaks it.

Verified by mutation, not just by passing:
  - fail open (empty instead of sentinel) -> 2 failures
  - drop the already-set guard             -> 3 failures
  - point every var at one ref             -> 3 failures

Full suite: 133/133 across test-credentials, test-launch-dir-check,
test-remote-session and test-wrapper. shellcheck -S info clean.

test-gh-token-permissions.sh's "Token authentication failed (got:
twistedmelonman)" is PRE-EXISTING -- it reproduces identically on a clean
tree. That failure is #126 itself: GH_TOKEN resolves from the launch
directory, so a session started in a smartwatermelon repo still authenticates
as twistedmelonman. This commit supplies the per-owner tokens that fix needs;
it does not change GH_TOKEN selection.

Advances #126

Claude-Session: https://claude.ai/code/session_017s2qrmkQaV54fbcMzB2KRm
@claude

This comment has been minimized.

…ading

The "op read succeeds on first attempt -> called exactly once" case shares one
call log with everything the sourced file does. Adding _load_owner_gh_tokens
added three more reads to that log, so the assertion saw 4 where it expects 1.

Preset the per-owner vars for this case so _load_owner_gh_tokens takes its
already-set path and reads nothing. The assertion then counts only
_load_gh_token's calls, which is what it was written to measure. Presetting
rather than filtering the log keeps the count exact instead of merely
plausible.

This passed locally and failed in CI, which is the whole lesson: the
developer environment already exports GH_TOKEN_SWM/NOS/TWM, so the already-set
path was being taken by accident. The test was green for a reason unrelated to
what it asserts. Verified the fix by reproducing CI's environment --
`env -u GH_TOKEN_SWM -u GH_TOKEN_NOS -u GH_TOKEN_TWM -u GH_TOKEN` -- where the
unfixed version fails 31/32 and the fixed version passes 32/32.

No change to lib/credentials.sh; the code under test was correct.

Claude-Session: https://claude.ai/code/session_017s2qrmkQaV54fbcMzB2KRm
@claude

This comment has been minimized.

Same defect as the previous commit, in the other suite. test-wrapper.sh's
"op read attempted exactly once (no retry loop without timeout)" shares one
call log with everything sourcing credentials.sh does, so
_load_owner_gh_tokens' three reads were attributed to _load_gh_token and the
count read 4.

Preset the per-owner vars for this case so the already-set path is taken and
nothing is read. The assertion then measures what it was written to measure:
that a missing `timeout` produces one attempt rather than a retry loop.

I should have found this with the first fix instead of after a second red CI
run. Both suites were checked for the pattern this time -- these are the only
two call-count assertions in the repo.

Verified in a CI-equivalent environment
(`env -u GH_TOKEN_SWM -u GH_TOKEN_NOS -u GH_TOKEN_TWM -u GH_TOKEN`):
without the preset this fails 1 with Actual: 4, matching CI exactly; with it
all four suites pass -- test-credentials 32/32, test-launch-dir-check 6/6,
test-remote-session 26/26, test-wrapper 69/69.

shellcheck -S info clean. No change to lib/credentials.sh.

Claude-Session: https://claude.ai/code/session_017s2qrmkQaV54fbcMzB2KRm
@claude

This comment has been minimized.

@twistedmelonman
twistedmelonman merged commit 3ed0d39 into main Sep 18, 2026
4 checks passed
@twistedmelonman
twistedmelonman deleted the claude/feat-load-all-owner-tokens-c4e8e106 branch September 18, 2026 05:05
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