feat(#256): add GH_TOKEN validation to pre-retro.sh by fullsend-ai-coder[bot] · Pull Request #262 · guyoron1/fullsend

fullsend-ai-coder · 2026-07-10T12:49:48Z

Validate GH_TOKEN with gh auth status before the sandbox starts. When the token is invalid, exit 1 with an ::error:: annotation, saving the cost of a full agent run (model inference, sandbox creation, teardown). When GH_TOKEN is unset, emit a ::warning:: and continue with limited functionality.

This follows the pattern from pre-review.sh which performs token-dependent pre-flight checks.

Closes #256

Post-script verification

Branch is not main/master (agent/256-gh-token-validation-retro)
Secret scan passed (gitleaks — c776c248ac9a91f10998ca7107e974a6c9d76514..HEAD)
Pre-commit hooks passed (authoritative run on runner)
Tests ran inside sandbox

Validate GH_TOKEN with gh auth status before the sandbox starts. When the token is invalid, exit 1 with an ::error:: annotation, saving the cost of a full agent run (model inference, sandbox creation, teardown). When GH_TOKEN is unset, emit a ::warning:: and continue with limited functionality. This follows the pattern from pre-review.sh which performs token-dependent pre-flight checks. Closes #256

github-actions · 2026-07-10T12:50:56Z

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

fullsend-ai-review · 2026-07-10T12:55:19Z

🤖 Finished Review · ❌ Failure · Started 12:55 PM UTC · Completed 12:55 PM UTC
Commit: c776c24 · View workflow run →

guyoron1 · 2026-07-10T15:56:23Z

/fs-review

fullsend-ai-review · 2026-07-10T15:57:05Z

🤖 Finished Review · ✅ Success · Started 3:57 PM UTC · Completed 4:05 PM UTC
Commit: d8e3df7 · View workflow run →

fullsend-ai-review · 2026-07-10T16:05:39Z

Review — Approve

The change correctly addresses issue #256 by adding GH_TOKEN validation to pre-retro.sh before the sandbox starts. When the token is unset or invalid, the script exits with a clear ::error:: annotation and exit code 1, saving the cost of a full agent run. The implementation is sound: the empty check gates ::add-mask:: (avoiding the empty-mask footgun), the mask is applied before gh auth status (preventing token leakage), and the ponytail comment honestly documents the validation ceiling (auth only, not scopes).

The test file covers the key scenarios (valid token, invalid token, absent token, on-demand trigger, automatic trigger) using a mock gh binary and env -i for environment isolation. The env -i approach is stricter than sibling tests (which use env with overrides) but correctly passes PATH, HOME, and ORIGINATING_URL explicitly.

post-retro.sh already requires GH_TOKEN via ${GH_TOKEN:?} — this PR catches the failure earlier, before sandbox creation.

Findings

Low — PR body description mismatch (pre-retro.sh): The PR description states "When GH_TOKEN is unset, emit a ::warning:: and continue with limited functionality," but the implementation exits with ::error:: and exit code 1. The implementation matches the issue's recommendation ("The error should be fatal"), so the code is correct — the PR body is just misleading.

Low — gh auth status stderr suppressed (pre-retro.sh, line 37): Both stdout and stderr are redirected to /dev/null, hiding diagnostics that distinguish token expiry from revocation from wrong host. Consider capturing stderr for a more informative error message: if ! auth_err=$(gh auth status 2>&1); then echo "::error::GH_TOKEN validation failed — ${auth_err}"; exit 1; fi

Low — Pattern divergence (pre-retro.sh): This is the only pre-script that treats GH_TOKEN as mandatory. Other pre-scripts (pre-review.sh, pre-code.sh) skip token-dependent checks when the token is absent (exit 0). The divergence is well-motivated — the retro agent cannot produce meaningful output without API access — and is documented by the ponytail comment, but worth noting as a deliberate architectural choice.

Low — ::add-mask::${GH_TOKEN} injection surface (pre-retro.sh, line 36): The echo "::add-mask::${GH_TOKEN}" pattern is theoretically vulnerable to workflow command injection if the token value contains newlines or :: sequences. Risk is negligible: the token is infrastructure-controlled (OIDC mint), and this pattern is already used identically in post-retro.sh (line 18), post-review.sh, post-fix.sh, post-code.sh, and prepare-sandbox-credentials.sh.

Labels: PR modifies retro agent pre-script under internal/scaffold/fullsend-repo/scripts/

Previous run

Review — request-changes

PR: feat(#256): add GH_TOKEN validation to pre-retro.sh
Scope: 1 file changed, +14 lines

The change adds a GH_TOKEN validation block to pre-retro.sh that checks gh auth status when the token is set and warns when absent. The goal — fail fast on invalid tokens before burning a 30-minute sandbox — is sound and directly addresses issue #256. However, the absent-token path has a gap that defeats the fail-fast promise.

Findings

🔴 High — Fail-fast gap on absent GH_TOKEN

File: internal/scaffold/fullsend-repo/scripts/pre-retro.sh, lines 33–35

When GH_TOKEN is unset, the script emits ::warning:: and exits 0 (success). The harness then launches the 30-minute sandbox. After the sandbox completes, post-retro.sh runs and immediately hard-fails on line 17:

: "${GH_TOKEN:?GH_TOKEN is required}"

The entire sandbox run is wasted — model inference, compute, teardown — on a run that can never post results. This is the exact failure mode issue #256 aimed to prevent, and it occurs on the most expensive path (absent token → full sandbox → guaranteed post-script failure).

Remediation: Change the else branch to fail hard:

else
  echo "::error::GH_TOKEN is not set — retro agent requires GitHub API access"
  exit 1
fi

Update the header comment to move GH_TOKEN from "Optional" to "Required env vars."

🟡 Medium — Missing test file

File: internal/scaffold/fullsend-repo/scripts/pre-retro-test.sh (absent)

pre-review.sh has a corresponding pre-review-test.sh with a mock gh binary covering its branching logic. The new pre-retro.sh validation has three code paths (valid token, invalid token, absent token) but no test coverage. The mock-gh pattern from pre-review-test.sh is directly reusable.

🟡 Medium — Plan document staleness

File: docs/superpowers/plans/2026-05-04-retro-agent.md, line 226

The plan document lists GH_TOKEN as a Required env var for pre-retro.sh, but the implementation treats it as optional with a warning. If the high finding above is resolved (making it required), the plan document is already correct and no update is needed.

🔵 Low — `gh auth status` validates auth, not scopes

File: internal/scaffold/fullsend-repo/scripts/pre-retro.sh, line 28

gh auth status confirms the token authenticates against the GitHub API but does not verify it has the scopes (issues:write, pull_requests:write) required by post-retro.sh. A fine-grained PAT with only contents:read would pass validation but fail during post-processing. This is still a strict improvement over no validation — the specific failure case in issue #256 (invalid token) is caught. Worth a comment noting the limitation.

🔵 Low — No `::add-mask::` defense-in-depth

File: internal/scaffold/fullsend-repo/scripts/pre-retro.sh

post-retro.sh (line 18) masks the token with echo "::add-mask::${GH_TOKEN}". The pre-script doesn't. If any future edit accidentally echoes the token value, it would appear unmasked in CI logs. Consider adding echo "::add-mask::${GH_TOKEN}" before the validation block.

🔵 Low — `gh auth status` stdout not suppressed

File: internal/scaffold/fullsend-repo/scripts/pre-retro.sh, line 28

gh auth status 2>/dev/null suppresses stderr but not stdout. gh auth status outputs authentication metadata (username, token type) to stdout, which will appear in CI logs. Consider >/dev/null 2>&1 to suppress both streams.

What looks good

Authorization: Change traces cleanly to issue [RTK+Ponytail] Add GH_TOKEN validation to pre-retro.sh to fail fast on invalid tokens #256 with matching scope. No scope creep.
Security: No user-controlled values interpolated into GHA workflow commands. All ::error:: and ::warning:: strings are hardcoded literals.
Style: Comment banner, variable quoting, and bash idioms match established patterns in sibling scripts.
Header comment updated: GH_TOKEN documented in the env vars section.

Labels: PR modifies retro agent scaffold pre-script

guyoron1 · 2026-07-10T17:37:35Z

/fs-fix

fullsend-ai-coder · 2026-07-10T17:38:25Z

🤖 Finished Fix · ✅ Success · Started 5:38 PM UTC · Completed 5:47 PM UTC
Commit: d8e3df7 · View workflow run →

- Fail hard (exit 1) when GH_TOKEN is unset instead of warning and continuing — prevents wasting a full sandbox run that will always fail in post-retro.sh (closes the fail-fast gap from issue #256) - Move GH_TOKEN from optional to required in header comment - Add ::add-mask:: to redact token from CI logs (defense-in-depth) - Suppress gh auth status stdout (>/dev/null 2>&1) - Add comment noting gh auth status checks auth, not scopes - Add pre-retro-test.sh covering valid/invalid/absent token paths Addresses review feedback on #262

fullsend-ai-coder · 2026-07-10T17:47:33Z

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 6 review findings. Core fix: GH_TOKEN is now required (exit 1 if absent), closing the fail-fast gap from #256. Added ::add-mask::, suppressed gh auth stdout, added scope-limitation comment, and created pre-retro-test.sh with 5 test cases. Plan doc already correct — no change needed.

Fixed (6):

Fail-fast gap on absent GH_TOKEN (internal/scaffold/fullsend-repo/scripts/pre-retro.sh): Changed else branch to exit 1 with ::error:: annotation when GH_TOKEN is unset, preventing a wasted 30-minute sandbox run that would always fail in post-retro.sh. Moved GH_TOKEN from optional to required in header comment.
Missing test file (internal/scaffold/fullsend-repo/scripts/pre-retro-test.sh): Created pre-retro-test.sh following the mock-gh pattern from pre-review-test.sh. Covers three code paths: valid token (exit 0), invalid token (exit 1), absent token (exit 1), plus retro comment variants.
Plan document staleness (docs/superpowers/plans/2026-05-04-retro-agent.md): No change needed — the plan document already lists GH_TOKEN as required. With the code fix above making GH_TOKEN required, implementation and plan are now consistent.
gh auth status validates auth, not scopes (internal/scaffold/fullsend-repo/scripts/pre-retro.sh): Added ponytail comment explaining that gh auth status checks authentication but not token scopes, noting this still catches the [RTK+Ponytail] Add GH_TOKEN validation to pre-retro.sh to fail fast on invalid tokens #256 failure mode and the upgrade path if scope checks are needed.
No ::add-mask:: defense-in-depth (internal/scaffold/fullsend-repo/scripts/pre-retro.sh): Added echo ::add-mask::${GH_TOKEN} before the validation block, matching the pattern from post-retro.sh.
gh auth status stdout not suppressed (internal/scaffold/fullsend-repo/scripts/pre-retro.sh): Changed 2>/dev/null to >/dev/null 2>&1 to suppress both stdout and stderr from gh auth status.

Tests: passed

_{Updated by fullsend fix agent}

fullsend-ai-review · 2026-07-10T17:50:17Z

🤖 Finished Review · ✅ Success · Started 5:50 PM UTC · Completed 5:59 PM UTC
Commit: d8e3df7 · View workflow run →

Superseded by updated review

fullsend-ai-review · 2026-07-10T17:58:59Z

+  exit 1
+fi
+echo "::add-mask::${GH_TOKEN}"
+if ! gh auth status >/dev/null 2>&1; then


[low] observability

gh auth status stderr suppressed (>/dev/null 2>&1) hides diagnostics that distinguish token expiry from revocation. Consider capturing stderr for a more informative error annotation.

Suggested fix: Replace with: if ! auth_err=$(gh auth status 2>&1); then echo "::error::GH_TOKEN validation failed — ${auth_err}"; exit 1; fi

fullsend-ai-review · 2026-07-10T17:58:59Z

+  echo "::error::GH_TOKEN is not set — retro agent requires GitHub API access"
+  exit 1
+fi
+echo "::add-mask::${GH_TOKEN}"


[low] workflow-command-injection

echo "::add-mask::${GH_TOKEN}" is theoretically vulnerable to newline-based workflow command injection. Risk is negligible: token is OIDC-minted (infrastructure-controlled), and this pattern is already used in post-retro.sh, post-review.sh, post-fix.sh, post-code.sh, and prepare-sandbox-credentials.sh.

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added the component/sandbox label Jul 10, 2026

fullsend-ai-review Bot approved these changes Jul 10, 2026

View reviewed changes

fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jul 10, 2026

Conversation

fullsend-ai-coder Bot commented Jul 10, 2026

Post-script verification

Uh oh!

github-actions Bot commented Jul 10, 2026

E2E tests did not run

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guyoron1 commented Jul 10, 2026

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review — Approve

Findings

Review — request-changes

Findings

🔴 High — Fail-fast gap on absent GH_TOKEN

🟡 Medium — Missing test file

🟡 Medium — Plan document staleness

🔵 Low — gh auth status validates auth, not scopes

🔵 Low — No ::add-mask:: defense-in-depth

🔵 Low — gh auth status stdout not suppressed

What looks good

Uh oh!

This comment was marked as outdated.

Uh oh!

guyoron1 commented Jul 10, 2026

Uh oh!

fullsend-ai-coder Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-coder Bot commented Jul 10, 2026

🔧 Fix agent — iteration 1 (human-triggered)

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading

🔵 Low — `gh auth status` validates auth, not scopes

🔵 Low — No `::add-mask::` defense-in-depth

🔵 Low — `gh auth status` stdout not suppressed

fullsend-ai-coder Bot commented Jul 10, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading