fix(#511): add GH_TOKEN validation to pre-retro.sh

[fullsend-ai-coder]

Add a gh auth status check after URL validation in pre-retro.sh to fail fast when GH_TOKEN is invalid. This prevents wasting compute on sandbox creation and model inference when the token cannot authenticate.

Behavior:

• GH_TOKEN set and valid: logs success, continues
• GH_TOKEN set and invalid: exits 1 with ::error::
• GH_TOKEN unset: logs ::warning::, continues

Add pre-retro-test.sh with mock gh to verify all three cases.

---

Closes #511

Post-script verification

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

[github-actions]

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]

🤖 Finished Review · ✅ Success · Started 5:37 PM UTC · Completed 5:47 PM UTC
Commit: b776cf6 · View workflow run →

[fullsend-ai-review]

Review

Verdict: request-changes

Summary

The intent is sound — fail fast when GH_TOKEN is invalid so the retro agent doesn't waste compute. However, gh auth status is the wrong tool for this job. It rejects tokens that are fully functional for API access (verified on this runner: gh api /rate_limit returns 4972 remaining while gh auth status returns exit 1 with "The token in GH_TOKEN is invalid"). This would block valid retro runs in production.

A secondary issue: GH_TOKEN is treated as optional (warn and continue when unset), but post-retro.sh hard-fails on missing GH_TOKEN. The warning path wastes the entire agent run.

The test file is well-structured and follows existing *-test.sh patterns. The scope is clean — no scope creep beyond issue #511.

---

Findings

1. gh auth status produces false negatives — rejects working tokens · HIGH · correctness

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

gh auth status does not reliably validate whether a token works for API access. Verified on this runner (gh 2.93.0): the current GH_TOKEN successfully authenticates gh api /rate_limit (4972 remaining), yet gh auth status returns exit code 1 with "The token in GH_TOKEN is invalid." This happens with certain token types (e.g., fine-grained PATs, GitHub App installation tokens) that gh auth status does not fully recognize despite being functional.

This directly violates validation criterion 4 from issue #511: "no false positives (valid tokens incorrectly rejected)." In production, this would block all retro runs that use these token types.

Remediation: Replace gh auth status with a lightweight authenticated API call:

if ! gh api /rate_limit >/dev/null 2>&1; then
  echo "::error::GH_TOKEN is invalid — retro agent requires GitHub API access"
  exit 1
fi

This tests what actually matters: can the token make API calls?

2. GH_TOKEN treated as optional but post-retro.sh requires it · MEDIUM · contract-mismatch

File: internal/scaffold/fullsend-repo/scripts/pre-retro.sh, line 12 (header) and line 30 (else branch)

The script lists GH_TOKEN as "Optional" and emits a ::warning:: when it's unset, allowing execution to continue. But post-retro.sh line 17 has : "${GH_TOKEN:?GH_TOKEN is required}" — it hard-fails if GH_TOKEN is absent. The retro agent will launch, consume sandbox/model resources, produce output, and then post-retro.sh will fail. This defeats the "fail fast" goal of issue #511.

Remediation: Make GH_TOKEN required in pre-retro.sh to match the downstream contract:

: "${GH_TOKEN:?GH_TOKEN is required for the retro agent}"

3. 2>/dev/null suppresses diagnostic stderr · LOW · error-handling

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

Redirecting stderr to /dev/null on the validation call makes it impossible to distinguish "invalid token" from "network failure" or "DNS resolution error." When the validation check fails, operators see only the generic ::error:: message with no root cause.

Remediation: Let stderr through to the step log (it appears below the ::error:: annotation), or capture it and include it in the error message.

4. Test "no-token-warns" does not verify script completion · LOW · test-adequacy

File: internal/scaffold/fullsend-repo/scripts/pre-retro-test.sh, lines 97-101

The test checks for the warning message and exit 0, but unlike "valid-token-continues-to-completion" it does not verify that "Pre-retro validation complete." appears. If the warning branch silently aborted, this test would still pass.

5. Missing edge case test: whitespace-only GH_TOKEN · LOW · test-adequacy

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

GH_TOKEN=" " (whitespace-only) passes the -n check and would attempt token validation. No test covers this edge case.

---

Labels: PR modifies sandbox pre-script infrastructure for the retro agent.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[high] correctness

gh auth status produces false negatives — rejects tokens that work for API access. Verified: gh api /rate_limit succeeds (4972 remaining) while gh auth status returns exit 1 with 'The token in GH_TOKEN is invalid' on this runner (gh 2.93.0). This would block valid retro runs, violating issue #511 validation criterion 4.

Suggested fix: Replace gh auth status with a lightweight API call: if ! gh api /rate_limit >/dev/null 2>&1; then echo '::error::GH_TOKEN is invalid'; exit 1; fi

[fullsend-ai-review]

[medium] contract-mismatch

GH_TOKEN is listed as 'Optional' and the else branch warns-and-continues when unset. But post-retro.sh line 17 hard-fails with : ${GH_TOKEN:?GH_TOKEN is required}. The warning path wastes compute — sandbox + model inference runs then post-retro fails.

Suggested fix: Make GH_TOKEN required in pre-retro.sh with : "${GH_TOKEN:?GH_TOKEN is required}" to match the post-retro.sh contract and fail fast.

[fullsend-ai-review]

[low] error-handling

2>/dev/null on the validation call suppresses diagnostic stderr. Network failures, DNS errors, and TLS issues are indistinguishable from invalid tokens — operators see only the generic ::error:: message.

Suggested fix: Let stderr through to the step log, or capture and include in the error message on failure.

[fullsend-ai-review]

[low] test-adequacy

Test 'no-token-warns' checks for warning message and exit 0 but does not verify the script runs to completion (unlike 'valid-token-continues-to-completion' which checks for 'Pre-retro validation complete.').

[fullsend-ai-review]

[low] test-adequacy

Missing edge case: whitespace-only GH_TOKEN (e.g., ' ') passes the -n check and would attempt token validation. No test covers this.