refactor: remove GitHub token routing, fix op inject stdout leak

[smartwatermelon]

Summary

• Remove github-token.sh sourcing from wrapper; GH_TOKEN now comes from shell startup (~/.config/bash/1password.sh, Automation vault service account)
• Fix op inject leaking resolved temp-file path to stdout (changed 2>/dev/null → &>/dev/null)
• Update CLAUDE.md architecture + headroom patterns to match current secrets model
• Test: add fail-fast guard requiring wrapper context; accept classic ghp_ PATs alongside github_pat_ fine-grained

Follow-ups (filed, non-blocking)

• README.md still documents removed github-token.sh / gh-token-router.sh as active modules #36 — README.md still documents removed modules as active
• docs/SECRETS.md still documents per-org token env var convention routed by gh-token-router #37 — docs/SECRETS.md still documents per-org routing

Test plan

• bash tests/test-wrapper.sh passes
• bash tests/test-gh-token-permissions.sh passes inside wrapper session
• CI green

🤖 Generated with Claude Code

[claude]

PR #38 Review: Remove github-token.sh from wrapper, service-account 1Password flow

Summary

This PR removes lib/github-token.sh from the wrapper's execution path, shifting GH_TOKEN injection from the wrapper itself to shell startup (~/.config/bash/1password.sh). It also suppresses stdout on op inject, adds a session-guard to the token-permissions test, and broadens PAT-format acceptance to include classic ghp_ tokens.

---

Blocking Issues

None found.

---

Non-Blocking Observations

1. Stale error message in test after token-injection responsibility moves — test-gh-token-permissions.sh:143

fail "GH_TOKEN is not set — wrapper did not inject token"

The wrapper no longer injects GH_TOKEN, so this message will mislead anyone who encounters the failure. It should say something like "GH_TOKEN is not set — check that ~/.config/bash/1password.sh is sourced at shell startup". Low severity (test-only), but it's likely to cause confusion the next time this test fails, since the diagnostic points at the wrong layer.

2. Session-guard error message inaccuracy — test-gh-token-permissions.sh:101

echo "GIT_AUTHOR_NAME is not set — wrapper environment is not active."

The guard condition on line 95 is != "Claude Code Bot", so the message fires even when GIT_AUTHOR_NAME is set to some other value (e.g., the runner's real name in a direct shell invocation). The message should read "GIT_AUTHOR_NAME is not 'Claude Code Bot'" to avoid confusion.

3. op inject stdout suppression — lib/secrets-loader.sh:325

The change from 2>/dev/null to &>/dev/null is safe because op inject writes resolved content to --out-file, not stdout. No functional impact. Worth noting in case a future version of op starts emitting structured output to stdout for diagnostics.

4. CLAUDE.md module-dependency chain omits last two sourced modules

The updated CLAUDE.md step 2 lists:

logging.sh → permissions.sh → path-security.sh → git-identity.sh → secrets-loader.sh → binary-discovery.sh

bin/claude-wrapper lines 13-35 source them in this order but also include pre-launch.sh and remote-session.sh after binary-discovery.sh. The doc omits both. Minor, but the "Execution flow" table is the first thing someone reads to understand load order.

VERDICT: PASS

[claude]

PR #38 Review — refactor: remove GitHub token routing, fix op inject stdout leak

Summary

Clean architectural refactor that moves GH_TOKEN injection out of the wrapper and into shell startup, along with a small security fix to op inject. Changes are coherent, well-documented, and internally consistent.

---

bin/claude-wrapper

Removal of github-token.sh / load_github_token — The old call sequence was: inject 1Password secrets → load_github_token (reads from flat files after env vars are populated). Removing both is consistent with the new model where GH_TOKEN arrives pre-set from ~/.config/bash/1password.sh. No dangling call sites remain in the diff.

One implicit assumption: any consuming code that previously relied on per-org GH_TOKEN_<OWNER> routing (from gh-token-router.sh) is also now satisfied by the single GH_TOKEN from the Automation vault. The PR description and CLAUDE.md both confirm this is intentional. No regression risk in the diff itself.

---

lib/secrets-loader.sh — line 324

op inject --out-file writes resolved secrets to a file, not stdout. Suppressing stdout (&> vs 2>) closes the theoretical path where op emits a warning or progress line to stdout that could leak the resolved temp-file path. The fix is correct. Error detection is preserved because if ! checks exit status, not output.

---

tests/test-gh-token-permissions.sh

Guard check: Gating on GIT_AUTHOR_NAME == "Claude Code Bot" is pragmatic — git-identity.sh sets this only when the wrapper is active. The guard prevents confusing failures when the test is run without wrapper context. The error message is clear and actionable.

Classic PAT branch: Adding ghp_* as a passing case alongside github_pat_* is correct given that GH_TOKEN now comes from the Automation vault and the token type is determined upstream. The original else/fail branch is preserved for unrecognized prefixes.

ShellCheck suppresses: Both are well-justified — SC2329 on cleanup (invoked via trap), SC2312 on date (non-fatal in a test-label string).

---

CLAUDE.md

Documentation is now accurate with the code:

• Module sourcing order adds pre-launch.sh and remote-session.sh (were always sourced, just not listed).
• github-token.sh and gh-token-router.sh correctly marked legacy.
• Config table rows for gh-token* and global secrets.op removed.
• Follow-ups (README.md still documents removed github-token.sh / gh-token-router.sh as active modules #36, docs/SECRETS.md still documents per-org token env var convention routed by gh-token-router #37) appropriately filed as non-blocking.

---

Non-blocking observations

1. opp alias in CLAUDE.md patterns but not defined in this repo — Useful note, but a future reader won't find where it's defined. Low maintenance impact; no action needed in this PR.

2. Test guard hard-codes "Claude Code Bot" — If git-identity.sh ever renames the bot, the guard silently breaks on a valid wrapper session. Consider [[ -n "${GIT_AUTHOR_NAME:-}" ]] as a more resilient check. Stylistic; current value matches git-identity.sh.

VERDICT: PASS

[claude]

PR #38 Review: Remove Multi-Org GitHub Token Routing

Summary

This PR removes the lib/github-token.sh and lib/gh-token-router.sh modules and all associated machinery, replacing the wrapper-managed GitHub token system with the expectation that GH_TOKEN is set in the shell environment at startup (e.g., via ~/.config/bash/1password.sh). It also makes a minor improvement to op inject output suppression in secrets-loader.sh.

---

Code Quality & Correctness

lib/secrets-loader.sh line 525 — &>/dev/null vs 2>/dev/null

The change from 2>/dev/null to &>/dev/null is correct and an improvement. Previously, any stdout from op inject could leak 1Password reference paths. The error case is already handled by log_error on the next line, so silencing op inject's stdout poses no diagnostic loss.

tests/test-gh-token-permissions.sh — wrapper session guard (lines 550–558)

The guard condition [[ "${GIT_AUTHOR_NAME:-}" != "Claude Code Bot" ]] is a fragile sentinel — it couples the test guard to the exact string value set by git-identity.sh, meaning a git identity config change could silently break the guard. This is low impact for now (the value is intentional and documented), but worth noting as a future maintenance concern.

Internal consistency — module removal

The removal is internally consistent across all changed files:

• bin/claude-wrapper — source github-token.sh and load_github_token call both removed
• tests/test-wrapper.sh — assert_file_exists github-token.sh and all token router test cases removed
• tests/test-gh-token-permissions.sh — Section 5 (Cross-Org Token Routing) removed, Section 6 renumbered to 5
• Docs (CLAUDE.md, README.md, docs/SECRETS.md) — all updated to reflect new architecture

tests/test-gh-token-permissions.sh line 568 — classic PAT recognition

Adding ghp_* as a passing prefix is correct given that GH_TOKEN is now sourced externally and could be any PAT type. The updated failure message on line 575 accurately reflects the new setup.

---

Reliability Regressions

No regressions introduced. The removed code path (flat-file token loading + per-org routing via CLAUDE_GH_TOKEN_ROUTER) is intentionally abandoned. The gh CLI wrapper at ~/.local/bin/gh only sourced gh-token-router.sh when CLAUDE_GH_TOKEN_ROUTER was set; since load_github_token no longer sets that env var, the gh wrapper will never attempt to source the now-deleted file.

---

Security

No security concerns. The &>/dev/null change reduces information leakage. The deleted modules had their own inline permission checks — those checks are no longer needed since the wrapper no longer manages token files at all.

---

Non-Blocking Observations (by maintenance impact)

1. test-gh-token-permissions.sh guard fragility — The GIT_AUTHOR_NAME == "Claude Code Bot" guard will silently pass (wrong direction) if someone runs the test in an environment where that var happens to be set for an unrelated reason, or silently block if the identity string changes. A more explicit sentinel (e.g., a dedicated CLAUDE_WRAPPER_ACTIVE=1 env var set by the wrapper) would be more robust.

2. CLAUDE.md source order doc inconsistency — The updated source order in step 2 lists pre-launch.sh and remote-session.sh as sourced libs, but the Execution Flow steps 5 and 7 still describe them as runtime behaviors. Minor doc inconsistency that could confuse readers about when these modules are loaded vs. invoked.

VERDICT: PASS

[smartwatermelon]

Thanks for the reviews. Summary of responses to the observations across this PR's iterations:

Addressed (iteration 1, 6e1397c):

• Stale error message in test-gh-token-permissions.sh:143 → now points at shell startup, not wrapper
• Guard message at test-gh-token-permissions.sh:101 → now says 'not Claude Code Bot' instead of 'not set'
• CLAUDE.md execution flow → added pre-launch.sh and remote-session.sh to the sourced-module list

Addressed (iteration 2, 190a3bf) — responding to sentry[bot]'s HIGH finding:

• Deleted lib/github-token.sh, lib/gh-token-router.sh (no longer sourced; no per-org routing consumers remain)
• Removed test-wrapper.sh Section 6 (9 router tests) and test-gh-token-permissions.sh Section 5 (cross-org)
• Rewrote README.md GitHub Token section and docs/SECRETS.md GitHub Token Integration section
• Closed follow-up README.md still documents removed github-token.sh / gh-token-router.sh as active modules #36 and docs/SECRETS.md still documents per-org token env var convention routed by gh-token-router #37

Acknowledged, not changing:

• Guard hard-coding GIT_AUTHOR_NAME == "Claude Code Bot" — stylistic. Current value matches git-identity.sh and any rename there would produce a visible test-skip on next manual run. Can be revisited whenever git-identity.sh is next touched.
• CLAUDE.md pre-launch.sh/remote-session.sh appearing in both 'source libs' and runtime steps — text is accurate, reflects that modules are sourced once and invoked later.