fix(#230): detect self-review 422 and degrade gracefully when REVIEW_TOKEN missing#271
fix(#230): detect self-review 422 and degrade gracefully when REVIEW_TOKEN missing#271fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
…TOKEN missing When REVIEW_TOKEN is undefined, post-review.sh hard-failed before reaching fullsend post-review. When the Go CLI fell back to GITHUB_TOKEN, the review submission hit a 422 because GitHub rejects self-reviews (PR author = token identity). Changes: - post-review.sh: fall back to GITHUB_TOKEN with a warning instead of hard-failing, so the agent's sticky comment and label work still completes - postreview.go: warn when falling back to GITHUB_TOKEN; detect self-review 422 errors and surface an actionable message pointing operators to set REVIEW_TOKEN via the mint-token action - Add isSelfReviewError helper with tests Closes #230
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
|
🤖 Finished Review · ❌ Failure · Started 12:58 PM UTC · Completed 12:59 PM UTC |
|
/fs-review |
|
🤖 Finished Review · ❌ Failure · Started 3:57 PM UTC · Completed 4:09 PM UTC |
ReviewVerdict: comment — medium-severity findings worth noting; none blocking. This is a well-scoped bug fix for a real production problem (issue #230: 9+ failures across 9 days). The approach — soft fallback from Findings1.
|
| Severity | medium |
| Category | logic-error |
| File | internal/cli/postreview.go — isSelfReviewError |
The matching logic uses a disjunctive OR:
return strings.Contains(combined, "your own") ||
strings.Contains(combined, "can not") ||
strings.Contains(combined, "cannot")The "can not" and "cannot" legs match independently of the self-referential "your own" check. GitHub's review creation API returns other 422 errors containing "cannot" that are not self-review errors — for example, "You cannot request changes on a draft pull request". These would be wrapped with the misleading message about token identity, causing operators to chase a minted-token fix for an unrelated issue.
Impact: The review still fails either way (no data loss or security issue), but the error message is misleading. Operators may waste time on incorrect remediation.
Suggested fix: Require "your own" as the primary signal. Use "cannot"/"can not" only conjunctively:
return strings.Contains(combined, "your own")Or, if broader matching is desired:
hasSelfRef := strings.Contains(combined, "your own")
hasDenial := strings.Contains(combined, "can not") || strings.Contains(combined, "cannot")
return hasSelfRef && hasDenial2. Missing test case for the false-positive scenario
| Severity | medium |
| Category | test-adequacy |
| File | internal/cli/postreview_test.go — TestIsSelfReviewError |
The 5 test cases cover the happy paths well (self-review detection, non-422, non-API error, unrelated 422), but none exercises a 422 with "cannot" that is not a self-review error. Adding this case would expose the false-positive bug described above:
{
name: "422 draft PR cannot request changes",
err: &gh.APIError{StatusCode: 422, Message: "You cannot request changes on a draft pull request"},
want: false, // Would FAIL with current implementation
},3. Header comment says REVIEW_TOKEN is "Required" but it's now optional
| Severity | medium |
| Category | stale-documentation |
| File | internal/scaffold/fullsend-repo/scripts/post-review.sh |
| Line | 12 |
The file header (line 11–12) still reads:
# Required environment variables:
# REVIEW_TOKEN — token with pull-requests:write on the target repo
The PR makes REVIEW_TOKEN optional (falls back to GITHUB_TOKEN), but the header comment was not updated. Since this comment is in the same file being modified, it should be updated to reflect the new behavior.
Suggested fix: Change line 11–12 to something like:
# Environment variables:
# REVIEW_TOKEN — (optional) token with pull-requests:write; falls back to GITHUB_TOKEN
4. User guide references REVIEW_TOKEN without explaining it's optional
| Severity | low |
| Category | incomplete-documentation |
| File | docs/guides/user/running-agents-locally.md |
| Line | 149 |
The "Review agent" section shows REVIEW_TOKEN={github-pat} in the env file example without noting it's now optional. Users following this guide may think REVIEW_TOKEN is mandatory.
What looks good
- Authorization & scope: PR traces directly to issue [RTK+Ponytail] post-review.sh fails with 422 when REVIEW_TOKEN undefined after PR #2389 #230; scope matches the issue's authorization (soft fallback + self-review detection). No scope creep.
- Forge abstraction respected: Go code uses
client.CreatePullRequestReview— no forge violations. - Scaffold is the right place:
internal/scaffold/fullsend-repo/scripts/post-review.shis the upstream template; no other copies need updating per the layered content model. - Security clean: The
::warning::message is a static string (no variable interpolation, no GHA workflow command injection). Token masking works correctly after fallback. No secrets leaked in error messages. - Existing tests preserved: Import reordering in
postreview_test.gois purely cosmetic (fixes Go import grouping). No existing test logic or assertions were weakened. - Error wrapping chain correct:
errors.Ascorrectly unwraps through%wchains fromCreatePullRequestReview. - Shell script fallback correct:
${REVIEW_TOKEN:-}/${GITHUB_TOKEN:?...}pattern handles all cases (set, empty, unset).
Labels: PR fixes a bug in the review post-script and CLI error handling
Previous run
Review
Verdict: comment — one medium finding worth noting; no blocking issues.
This PR addresses issue #230 by converting the hard REVIEW_TOKEN guard in post-review.sh to a soft fallback to GITHUB_TOKEN, and adding isSelfReviewError detection in the Go CLI to surface clear error messages when the review submission hits a 422 self-review rejection. The approach is sound — it prevents the hard crash, preserves the preferred minting path, and gives operators actionable guidance when the fallback token triggers a self-review 422.
The test coverage is adequate (5 cases covering wrapped errors, alternate wording, non-422, unrelated 422, and non-API errors), the tokenFallback logic has no edge-case gaps, and the shell script changes are clean (static ::warning:: string, ::add-mask:: still runs correctly after fallback).
Findings
1. isSelfReviewError matching is overly broad — false positive risk
Severity: medium · Category: false-positive-logic · File: internal/cli/postreview.go
The function matches any 422 APIError whose combined message contains "can not" or "cannot" (case-insensitive). These are extremely generic English words. While the call site is narrow (only invoked when CreatePullRequestReview fails), GitHub could return other 422 errors from that endpoint containing "cannot" — e.g., a hypothetical "Cannot submit review on a closed pull request" — which would be misidentified as a self-review error, producing a misleading message about token identity.
The "your own" match alone is sufficient for the known GitHub self-review messages ("Can not approve your own pull request", "You cannot review your own pull request"). The bare "can not" and "cannot" matches add false-positive surface area without additional correct-detection value.
For comparison, the existing isBranchProtectionError in github.go uses domain-specific terms ("protected", "required status", "rule violation") that are unlikely to appear in unrelated errors. "cannot" is generic English.
Suggested fix: Remove the standalone "can not" and "cannot" matches. Keep "your own", or use tighter compound matches like strings.Contains(combined, "can not approve") or strings.Contains(combined, "cannot review").
2. Script header comment is stale
Severity: low · Category: stale-documentation · File: internal/scaffold/fullsend-repo/scripts/post-review.sh, lines 11–12
The file header still lists REVIEW_TOKEN under "Required environment variables" but the implementation now treats it as optional with a fallback to GITHUB_TOKEN. The header should reflect the new behavior.
Suggested fix: Update the header to mark REVIEW_TOKEN as optional and document the GITHUB_TOKEN fallback.
3. Error helper placement diverges from established pattern
Severity: low · Category: code-organization · File: internal/cli/postreview.go
The existing is*Error helpers (isBranchProtectionError, isAlreadyExistsError) live in internal/forge/github/github.go alongside the APIError type, using msg as the variable name and taking *APIError directly. The new isSelfReviewError lives in internal/cli/postreview.go, takes error (using errors.As), and uses combined as the variable name.
The different interface (error vs *APIError) is defensible since the function is called from consumer code. But placing it alongside its siblings in the github package (exported or with the same *APIError signature) would be more consistent.
Suggested fix: Consider moving to internal/forge/github/github.go with the same *APIError parameter signature as the existing helpers, and calling errors.As at the call site in postreview.go instead.
|
/fs-review |
|
🤖 Finished Review · ❌ Failure · Started 4:30 PM UTC · Completed 4:40 PM UTC |
1 participant
Summary
REVIEW_TOKENguard with a soft fallback toGITHUB_TOKENplus a warning. The agent's sticky comment, label logic, and stale-review cleanup still run even without a minted token — only the formal review submission may fail.GITHUB_TOKEN. Detects self-review 422 errors fromCreatePullRequestReviewand surfaces an actionable message pointing operators to setREVIEW_TOKENvia themint-tokenaction.isSelfReviewErrorhelper with 5 test cases covering GitHub's known error phrasings, unrelated 422s, non-422 errors, and non-API errors.Root cause
When
REVIEW_TOKENis undefined (e.g., mint step failure or missing workflow config),post-review.shhard-failed at line 21 before reachingfullsend post-review. If the guard was bypassed and the Go CLI fell back toGITHUB_TOKEN, the review submission hit a 422 because GitHub rejects self-reviews when the token identity matches the PR author.Test plan
go test -race -run "Review|SelfReview" ./internal/cli/...— 73 tests passgo vet ./...— cleanshellcheckon modifiedpost-review.sh— cleanpost-review-test.sh— all label tests passscan-secrets --staged— cleangitlint --commit HEAD— cleanCloses #230