fix(#2305): treat 401/403 comment-posting errors as non-fatal in post-retro.sh - #2306
Conversation
…-retro.sh
The retro post-script previously treated all comment-posting failures as
fatal under set -euo pipefail, causing the entire workflow run to fail
even when the retro agent succeeded and proposal issues were filed. A
403 ("Resource not accessible by integration") is a permanent permission
error — retrying won't help, and the summary comment is informational.
Wrap the gh api comment-posting call in error handling that captures
the exit code and response. If the response contains HTTP 401 or 403,
log a GitHub Actions warning and continue. All other HTTP errors remain
fatal. This prevents permission-gated repos from artificially inflating
the failure rate.
Add post-retro-test.sh with 8 test cases covering: happy path with and
without proposals, 403/401 non-fatal behavior, 500/422 remaining fatal,
and edge cases.
Note: pre-commit could not run in sandbox (shellcheck-py failed to
download due to network restrictions). The post-script runs an
authoritative pre-commit check on the runner.
Closes #2305
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
Site previewPreview: https://41d255a8-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 9:23 PM UTC · Completed 9:34 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Looks good to me ReviewFindingsLow
Info
Previous runLooks good to me ReviewFindingsLow
Info
Previous runLooks good to me ReviewFindingsLow
Info
Previous run (2)Looks good to me ReviewFindingsLow
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| if [[ ${COMMENT_EXIT} -ne 0 ]]; then | ||
| # Treat 401/403 as non-fatal — the token lacks permission to comment on | ||
| # this repo, but the core deliverables (analysis + proposal issues) are | ||
| # already complete. See #2305. |
There was a problem hiding this comment.
[low] injection
COMMENT_RESPONSE is interpolated unsanitized into the ::warning:: GHA workflow command. Although the dangerous commands (::set-env, ::add-path) were disabled by GitHub in 2020 and attacker control is limited, remaining injectable commands (::set-output, ::save-state) could still be exploited.
Suggested fix: Sanitize COMMENT_RESPONSE before interpolating into the ::warning:: directive — strip :: sequences and %0A/%0D URL-encoded newlines.
|
|
||
| if [[ ${COMMENT_EXIT} -ne 0 ]]; then | ||
| # Treat 401/403 as non-fatal — the token lacks permission to comment on | ||
| # this repo, but the core deliverables (analysis + proposal issues) are |
There was a problem hiding this comment.
[low] error handling fragility
The grep pattern grep -qE "HTTP (401|403)" depends on the exact error format emitted by the gh CLI. If a future gh version changes the format, the match would fail and the error would be treated as fatal.
|
|
||
| echo "Posting summary comment on ${ORIGINATING_REPO}#${ORIGINATING_NUMBER}" | ||
| jq -nc --arg body "${COMMENT}" '{body: $body}' | gh api \ | ||
| COMMENT_RESPONSE="" |
There was a problem hiding this comment.
[low] variable-naming-convention
COMMENT_RESPONSE deviates from the _OUTPUT naming convention used elsewhere (e.g., PUSH_OUTPUT in post-code.sh). Consider renaming to COMMENT_OUTPUT.
| # this repo, but the core deliverables (analysis + proposal issues) are | ||
| # already complete. See #2305. | ||
| if echo "${COMMENT_RESPONSE}" | grep -qE "HTTP (401|403)"; then | ||
| echo "::warning::Could not post summary comment to ${ORIGINATING_REPO}#${ORIGINATING_NUMBER}: insufficient permissions (${COMMENT_RESPONSE}). Skipping." |
There was a problem hiding this comment.
[low] error-message-formatting
The fatal error message omits target repository context. Compare with line 101 in the same file which includes ${TARGET_REPO}.
| fi | ||
| done | ||
|
|
||
| echo "gh $*" >> "${GH_LOG}" |
There was a problem hiding this comment.
[low] test coverage gap
The happy-path-one-proposal test does not assert that gh issue create was called — it only checks the comment-posting endpoint appeared in the log.
| # Treat 401/403 as non-fatal — the token lacks permission to comment on | ||
| # this repo, but the core deliverables (analysis + proposal issues) are | ||
| # already complete. See #2305. | ||
| if echo "${COMMENT_RESPONSE}" | grep -qE "HTTP (401|403)"; then |
There was a problem hiding this comment.
[low] design-coherence
The existing github-api-csma.sh library handles 403 secondary rate limits. While CSMA addresses a different concern (retries vs. graceful degradation), documenting why inline handling is preferred here would aid future maintainers.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 7:33 AM UTC · Completed 7:39 AM UTC |
- Sanitize COMMENT_OUTPUT before interpolating into ::warning:: GHA workflow command to prevent injecting ::set-output/::save-state - Rename COMMENT_RESPONSE → COMMENT_OUTPUT to match _OUTPUT naming convention used in other post-scripts (e.g. PUSH_OUTPUT) - Add comment explaining fail-closed behavior if gh CLI error format changes in the future - Include repo context in fatal error message for parity with other error messages in the script - Add happy-path-issue-created test asserting gh issue create was called - Document why inline 401/403 handling is used instead of github-api-csma.sh (different intent: graceful degradation vs retry) Addresses review feedback on #2306
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 6 low-severity review findings: sanitized GHA workflow command interpolation, renamed variable to match convention, documented fail-closed behavior and CSMA design rationale, added repo context to error message, and added test coverage for issue creation. Fixed (6):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 7:43 AM UTC · Completed 7:54 AM UTC |
| SAFE_OUTPUT="${SAFE_OUTPUT//%0D/}" | ||
| echo "::warning::Could not post summary comment to ${ORIGINATING_REPO}#${ORIGINATING_NUMBER}: insufficient permissions (${SAFE_OUTPUT}). Skipping." | ||
| else | ||
| echo "ERROR: failed to post summary comment on ${ORIGINATING_REPO}#${ORIGINATING_NUMBER}: ${COMMENT_OUTPUT}" |
There was a problem hiding this comment.
[low] injection
In the else branch (non-401/403 errors), COMMENT_OUTPUT is interpolated unsanitized into a plain echo statement. In GitHub Actions, stdout lines matching ::command:: patterns are processed as workflow commands. Although exit 1 follows immediately, an attacker could inject ::save-state:: or ::set-output:: directives that execute before the exit.
Suggested fix: Apply the same SAFE_OUTPUT sanitization before echoing in the else branch.
| if echo "${COMMENT_OUTPUT}" | grep -qE "HTTP (401|403)"; then | ||
| # Sanitize before interpolating into GHA workflow command to prevent | ||
| # injecting ::set-output or ::save-state directives via crafted responses. | ||
| SAFE_OUTPUT="${COMMENT_OUTPUT//::/}" |
There was a problem hiding this comment.
[low] sanitization-completeness
The sanitization handles only uppercase %0A and %0D. The established pattern in extract-transcript-error.sh handles both cases (%0A/%0a, %0D/%0d).
Suggested fix: Add lowercase variant handling: SAFE_OUTPUT="${SAFE_OUTPUT//%0a/}" and SAFE_OUTPUT="${SAFE_OUTPUT//%0d/}"
| fi | ||
| done | ||
|
|
||
| echo "gh $*" >> "${GH_LOG}" |
There was a problem hiding this comment.
[low] test coverage gap
The happy-path-one-proposal test does not assert that gh issue create was called — it only checks the comment-posting endpoint appeared in the log.
Suggested fix: Add an assertion that verifies gh issue create appears in the GH_LOG with expected arguments.
| # Sanitize before interpolating into GHA workflow command to prevent | ||
| # injecting ::set-output or ::save-state directives via crafted responses. | ||
| SAFE_OUTPUT="${COMMENT_OUTPUT//::/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0A/}" |
There was a problem hiding this comment.
[low] error handling fragility
The grep -qE "HTTP (401|403)" pattern depends on the exact error format emitted by the gh CLI. The inline comment acknowledges this fail-closed behavior.
| @@ -124,8 +124,34 @@ else | |||
| fi | |||
There was a problem hiding this comment.
[low] architecture-coherence
The inline 401/403 handling introduces a third distinct error-handling pattern for GitHub API permission errors across post-* scripts. The inline comment explains the design rationale.
fullsend-ai-review
Bot
added
ready-for-merge
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 8:16 AM UTC · Completed 8:23 AM UTC |
…URL-encoding variants Apply the same ::, %0A/%0D sanitization to the else branch (fatal errors) to prevent GHA workflow command injection via crafted gh CLI stderr output. Add lowercase %0a/%0d variants to match the established pattern in extract-transcript-error.sh. Addresses review feedback on #2306
🔧 Fix agent — iteration 2 (human-triggered)Fixed 2 of 5 review findings: sanitized COMMENT_OUTPUT in the fatal error branch and added lowercase %0a/%0d URL-encoding variants. The remaining 3 findings were informational or already addressed in iteration 1. Fixed (2):
Disagreed (3):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 8:26 AM UTC · Completed 8:37 AM UTC |
|
|
||
| echo "Posting summary comment on ${ORIGINATING_REPO}#${ORIGINATING_NUMBER}" | ||
| jq -nc --arg body "${COMMENT}" '{body: $body}' | gh api \ | ||
| # Note: we handle 401/403 inline rather than relying on github-api-csma.sh |
There was a problem hiding this comment.
[low] architectural-consistency
The inline 401/403 handling bypasses the existing lib/github-api-csma.sh infrastructure. CSMA already distinguishes retryable rate-limit 403s from bare permission 403s, but does not distinguish between rate-limit exhaustion and non-retryable permission error in its return code. The inline approach is simpler but diverges from post-prioritize.sh pattern.
|
|
||
| if [[ ${COMMENT_EXIT} -ne 0 ]]; then | ||
| # Treat 401/403 as non-fatal — the token lacks permission to comment on | ||
| # this repo, but the core deliverables (analysis + proposal issues) are |
There was a problem hiding this comment.
[low] duplication
The sanitization code (5 lines replacing ::, %0A, %0a, %0D, %0d) is duplicated identically in both the if and else branches.
Suggested fix: Hoist the sanitization outside the conditional — perform it once on COMMENT_OUTPUT before the if statement.
| # Sanitize before interpolating into GHA workflow command to prevent | ||
| # injecting ::set-output or ::save-state directives via crafted responses. | ||
| SAFE_OUTPUT="${COMMENT_OUTPUT//::/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0A/}" |
There was a problem hiding this comment.
[low] error-handling-consistency
The fatal error path uses plain echo ERROR: while the warning path uses GHA annotation syntax (::warning::). Other post-scripts use ::error:: for fatal errors. Pre-existing inconsistency in this file.
Suggested fix: Change to echo ::error::failed to post summary comment on ... for consistency with post-code.sh.
| fi | ||
| done | ||
|
|
||
| echo "gh $*" >> "${GH_LOG}" |
There was a problem hiding this comment.
[low] test coverage gap
The happy-path-one-proposal test name implies it validates the full happy path but only checks the comment-posting endpoint. A separate test covers gh issue create, but the test naming is misleading.
Suggested fix: Rename to happy-path-comment-posted, or add the gh issue create assertion.
fullsend-ai-review
Bot
added
ready-for-merge
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 9:07 AM UTC · Completed 9:12 AM UTC |
🔧 Fix agent — iteration 3 (human-triggered)No changes made. The human triggered /fs-fix without providing an instruction. The PR is already approved by the review bot with no outstanding findings, all CI checks pass, and all tests pass. There is nothing actionable to fix. Disagreed (1):
Tests: passed Updated by fullsend fix agent |
rh-hemartin
added
the
ok-to-test
|
🤖 Finished Retro · ✅ Success · Started 10:03 AM UTC · Completed 10:16 AM UTC |
Retro: PR #2306 — treat 401/403 comment-posting errors as non-fatal in post-retro.shOverall assessment: workflow performed well. The full pipeline (retro→triage→code→review→fix→merge) completed successfully. The code agent produced a correct, tested implementation in ~18 minutes from issue filing. All 13 workflow runs succeeded. The core fix was right from the start — all rework was low-severity polish. Timeline
What went well
Improvement opportunities (mostly already tracked)
Proposals filed
|
1 participant
The retro post-script previously treated all comment-posting failures as fatal under set -euo pipefail, causing the entire workflow run to fail even when the retro agent succeeded and proposal issues were filed. A 403 ("Resource not accessible by integration") is a permanent permission error — retrying won't help, and the summary comment is informational.
Wrap the gh api comment-posting call in error handling that captures the exit code and response. If the response contains HTTP 401 or 403, log a GitHub Actions warning and continue. All other HTTP errors remain fatal. This prevents permission-gated repos from artificially inflating the failure rate.
Add post-retro-test.sh with 8 test cases covering: happy path with and without proposals, 403/401 non-fatal behavior, 500/422 remaining fatal, and edge cases.
Note: pre-commit could not run in sandbox (shellcheck-py failed to download due to network restrictions). The post-script runs an authoritative pre-commit check on the runner.
Closes #2305
Post-script verification
agent/2305-retro-403-non-fatal)f119a205ffb44da8687d9ff1673b6b2d2de2e927..HEAD)