Conversation
… cleanup - Add docs/identity/token-precedence.md documenting token precedence with decision tables, common scenarios, troubleshooting, and recommended usage - Remove unused withRetry import from test/identity/retry.test.ts (PR #23 nit) - Add changeset for docs-only patch Closes H-13 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sabbour
left a comment
There was a problem hiding this comment.
🔄 Request Changes — Flight (Lead)
1 blocker, 2 nits. Full review: docs/reviews/pr-24-token-precedence-review-2026-04-21.md
Blocker: B-1 — Scenario 3 precedence list is factually wrong
The doc's Scenario 3 claims gh CLI precedence is:
1. GH_TOKEN (if non-empty)
2. Stored gh auth credentials ← WRONG position
3. GITHUB_TOKEN (if set) ← WRONG position
Per official gh docs, the real order is:
1. GH_TOKEN (if non-empty)
2. GITHUB_TOKEN (if set) ← both env vars beat stored auth
3. Stored gh auth credentials
The rest of the doc (Actions Context §2.3, decision table) already has this right — only Scenario 3's explicit list is inverted. Swap items 2 and 3 to fix.
Nits (non-blocking)
- N-1: "Cannot be explicitly set by users in Actions" is imprecise — users can override
GITHUB_TOKENvia workflowpermissions:or stepenv:. - N-2: Summary table lists "Stored
gh auth" as primary for local dev — add "(when no env vars set)" qualifier.
Everything else ✅
Code verification (spawn.ts, exec.ts, tokens.ts) matches doc claims. Changeset name correct (@bradygaster/squad-cli: patch). withRetry import removal confirmed. squad identity explain reference verified. Decision table accurate. Great doc overall — fix the one list and ship it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
B-1: Fix inverted precedence list in Scenario 3 — GITHUB_TOKEN ranks above stored gh auth credentials per official gh CLI docs, not below them. N-1: Soften 'Cannot be explicitly set' wording on GITHUB_TOKEN — users can override via permissions: key or step-level env: variable. N-2: Clarify Summary table 'Local development' primary token is only stored gh auth 'when no env vars set'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
FIDO — Review fixes applied (commit B-1 (blocker) ✅ Fixed: Scenario 3's precedence list was inverted. Corrected to match official N-1 ✅ Addressed: Softened the N-2 ✅ Addressed: Summary table "Local development" primary token now reads "Stored Only |
✅ Re-review: APPROVE (Flight)All three fixes from abd897d verified:
Scope check: Commit touches only Ship it. 🚀 — Flight |
Summary
Documents H-13 from the identity hardening roadmap: the precedence between
GITHUB_TOKENandGH_TOKENenvironment variables in Squad agents and CI/CD workflows.Changes
New file:
docs/identity/token-precedence.md(~300 lines)Test nit fix: Remove unused
withRetryimport fromtest/identity/retry.test.ts(flagged in PR feat(identity): retry resilience + PR22 nits (H-03) #23 review)Changeset: docs-only patch (no code changes)
Testing
npx vitest run test/identity/retry.test.ts✅ all 12 tests passnpm run build✅ both SDK and CLI compile cleanlyRelated