fix(#2393): add diagnostic stderr output to post-script failure paths - #2395
Conversation
All exit 1 paths across the 6 post-scripts (post-triage, post-code, post-review, post-retro, post-fix, post-prioritize) now emit a clear error message to stderr before exiting. This addresses three categories of issues: 1. Silent exit paths: post-review.sh exited with the fullsend post-review exit code but produced no diagnostic message. post-fix.sh exited silently when process-fix-result.py failed with bad input. Both now emit descriptive stderr messages. 2. Stdout-only errors: All echo "ERROR:..." and echo "::error::..." messages now include >&2 to ensure they appear on stderr, making them visible in GitHub Actions logs regardless of stdout buffering. 3. Missing context: HTTP-related failures now include the endpoint or command that failed. The add_label function in post-triage.sh captures and reports the gh API error output. Push failures in post-code.sh include the push output. PR creation failures include the head/base branch info. post-prioritize.sh errors include project and org context. Closes #2393
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. |
Site previewPreview: https://8d7477d9-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 5:33 PM UTC · Completed 5:45 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Info
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
|
ralphbean
left a comment
There was a problem hiding this comment.
/fs-fix A couple things inline.
| echo "::error::Failed to create PR: see above for details" | ||
| --body "${PR_BODY}" 2>&1); then | ||
| echo "::error::Failed to create PR for ${REPO_FULL_NAME} (head: ${BRANCH}, base: ${TARGET_BRANCH})" >&2 | ||
| [[ -n "${PR_CREATE_OUTPUT}" ]] && echo "::error::${PR_CREATE_OUTPUT}" >&2 |
There was a problem hiding this comment.
[critical] The 2>&1 here merges stderr into the capture, so on success PR_URL ends up with any progress messages gh prints alongside the actual URL. The original code only captured stdout, which was correct for the success path.
Could we redirect stderr to a temp file so stdout stays clean for PR_URL? Something like:
if ! PR_URL=$(gh pr create \
--repo "${REPO_FULL_NAME}" \
--head "${BRANCH}" \
--base "${TARGET_BRANCH}" \
--title "${PR_TITLE}" \
--body "${PR_BODY}" 2>/tmp/pr_create_stderr); then
echo "::error::Failed to create PR for ${REPO_FULL_NAME} (head: ${BRANCH}, base: ${TARGET_BRANCH})" >&2
[[ -s /tmp/pr_create_stderr ]] && cat /tmp/pr_create_stderr >&2
exit 1
fi| PR_FILES=$(gh pr view "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" --json files --jq '.files[].path') | ||
| if [ -z "${PR_FILES}" ]; then | ||
| echo "::error::Failed to fetch PR files or PR has no changed files — refusing to approve" | ||
| echo "::error::Failed to fetch PR files or PR has no changed files — refusing to approve (GET repos/${REPO_FULL_NAME}/pulls/${PR_NUMBER}/files)" >&2 |
There was a problem hiding this comment.
[minor] The diagnostic says GET repos/.../pulls/.../files but the actual command is gh pr view --json files, which hits GraphQL. Might send someone down the wrong path — maybe (gh pr view --json files) instead?
|
/fs-fix see inline comments above |
|
🤖 Finished Fix · ✅ Success · Started 8:15 PM UTC · Completed 8:23 PM UTC |
- post-code.sh: redirect gh pr create stderr to temp file instead of merging into stdout with 2>&1, keeping PR_URL clean on success - post-review.sh: fix diagnostic message to reference the actual command (gh pr view --json files) instead of the REST API endpoint Addresses review feedback on #2395
🔧 Fix agent — iteration 1 (human-triggered)Addressed both review findings: fixed stderr capture in post-code.sh PR creation and corrected diagnostic message in post-review.sh. Fixed (2):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 8:26 PM UTC · Completed 8:37 PM UTC |
| else | ||
| echo "::error::Push failed with unexpected error" | ||
| echo "::error::Push failed with unexpected error (git push origin ${BRANCH})" >&2 | ||
| echo "::error::Push output: ${PUSH_OUTPUT}" >&2 |
There was a problem hiding this comment.
[low] injection
PUSH_OUTPUT is interpolated unsanitized into a ::error:: GHA workflow command. PUSH_OUTPUT captures stdout/stderr of git push, which could contain ::set-env:: sequences if a server-side git hook or proxy injects them.
Suggested fix: Sanitize PUSH_OUTPUT by stripping :: sequences before interpolating, or use a plain ERROR: prefix instead.
| @@ -411,8 +412,9 @@ if ! PR_URL=$(gh pr create \ | |||
| --head "${BRANCH}" \ | |||
| --base "${TARGET_BRANCH}" \ | |||
| --title "${PR_TITLE}" \ | |||
There was a problem hiding this comment.
[low] error-handling
gh pr create stderr is redirected to a hardcoded path (2>/tmp/pr_create_stderr). Using mktemp would be safer against parallel invocation, and the temp file is never cleaned up.
Suggested fix: Use mktemp to create the temp file and add cleanup in a trap.
| --body "${PR_BODY}"); then | ||
| echo "::error::Failed to create PR: see above for details" | ||
| --body "${PR_BODY}" 2>/tmp/pr_create_stderr); then | ||
| echo "::error::Failed to create PR for ${REPO_FULL_NAME} (head: ${BRANCH}, base: ${TARGET_BRANCH})" >&2 |
There was a problem hiding this comment.
[low] pattern-consistency
The conditional [[ -s /tmp/pr_create_stderr ]] uses [[ ]] test syntax while the rest of post-code.sh consistently uses [ ] for conditionals (20+ instances, zero prior [[ ]] uses).
Suggested fix: Change [[ -s /tmp/pr_create_stderr ]] to [ -s /tmp/pr_create_stderr ] to match the file's convention.
| # appear as a failure. | ||
| exit 0 | ||
| elif [ "${POST_REVIEW_EXIT}" -ne 0 ]; then | ||
| echo "ERROR: fullsend post-review failed with exit code ${POST_REVIEW_EXIT} (PR #${PR_NUMBER} in ${REPO_FULL_NAME})" >&2 |
There was a problem hiding this comment.
[info] diagnostic-completeness
The fullsend post-review command's stderr is not captured, so the actual failure output is lost. Consider capturing stderr and including it in the error message.
Suggested fix: Capture stderr from the fullsend post-review call and include it in the error message.
ralphbean
left a comment
There was a problem hiding this comment.
/fs-fix These two lines should use ::error:: instead of ERROR: — see inline.
| python3 "${PROCESS_SCRIPT}" "${RESULT_FILE}" "${REPO_FULL_NAME}" "${PR_NUMBER}" || PROCESS_EXIT=$? | ||
| if [ "${PROCESS_EXIT}" -eq 1 ]; then | ||
| exit 1 # hard failure (bad input) | ||
| echo "ERROR: process-fix-result.py failed with exit code 1 (bad input) for PR #${PR_NUMBER} in ${REPO_FULL_NAME}" >&2 |
There was a problem hiding this comment.
[minor] This uses ERROR: where the rest of the file uses ::error::. The ::error:: prefix renders as a red annotation in the Actions UI, which is the whole point of this PR — making failures visible. Same thing on post-review.sh:180.
::error:: works fine with >&2, so we get both benefits.
|
/fs-fix see inline comments above |
|
🤖 Finished Fix · ✅ Success · Started 8:46 PM UTC · Completed 8:56 PM UTC |
- post-fix.sh, post-review.sh: change ERROR: prefix to ::error:: so failures render as red annotations in the Actions UI (per reviewer) - post-code.sh: use mktemp instead of hardcoded /tmp/pr_create_stderr, clean up temp file on both success and failure paths, and switch from [[ ]] to [ ] for pattern consistency with the rest of the file Addresses review feedback on #2395
🔧 Fix agent — iteration 2 (human-triggered)Addressed 3 findings: changed ERROR: to ::error:: in post-fix.sh and post-review.sh per human reviewer, and improved post-code.sh to use mktemp with cleanup and consistent [ ] syntax per bot reviewer. Declined 1 low-severity bot finding (PUSH_OUTPUT sanitization) as out of scope. Fixed (3):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 8:59 PM UTC · Completed 9:10 PM UTC |
| else | ||
| echo "::error::Push failed with unexpected error" | ||
| echo "::error::Push failed with unexpected error (git push origin ${BRANCH})" >&2 | ||
| echo "::error::Push output: ${PUSH_OUTPUT}" >&2 |
There was a problem hiding this comment.
[low] injection
PUSH_OUTPUT is interpolated unsanitized into a ::error:: GHA workflow command. PUSH_OUTPUT captures the combined stdout/stderr of git push, which could contain sequences like ::set-env:: if a server-side git hook or proxy injects them. The >&2 redirect does not mitigate the injection vector.
Suggested fix: Sanitize PUSH_OUTPUT by stripping :: sequences before interpolating, or use a plain ERROR: prefix instead of ::error::.
| --title "${PR_TITLE}" \ | ||
| --body "${PR_BODY}"); then | ||
| echo "::error::Failed to create PR: see above for details" | ||
| --body "${PR_BODY}" 2>"${PR_CREATE_STDERR}"); then |
There was a problem hiding this comment.
[low] injection
The stderr output of gh pr create is written to the runner via cat without sanitization. The GHA runner scans both stdout and stderr for workflow commands. If the GitHub API error response echoes back user-controlled data containing :: sequences, those would be interpreted as workflow commands.
Suggested fix: Pipe through sed to neutralize workflow commands before output.
| @@ -406,15 +407,19 @@ Closes #${ISSUE_NUMBER} | |||
| - [x] Pre-commit hooks passed (authoritative run on runner) | |||
| - [x] Tests ran inside sandbox" | |||
|
|
|||
There was a problem hiding this comment.
[info] error-handling
The temp file created by PR_CREATE_STDERR=$(mktemp) is cleaned up in both branches but could leak on unexpected exit. Negligible impact in an ephemeral GHA runner environment.
| # appear as a failure. | ||
| exit 0 | ||
| elif [ "${POST_REVIEW_EXIT}" -ne 0 ]; then | ||
| echo "::error::fullsend post-review failed with exit code ${POST_REVIEW_EXIT} (PR #${PR_NUMBER} in ${REPO_FULL_NAME})" >&2 |
There was a problem hiding this comment.
[info] diagnostic-completeness
The new error message provides useful context (exit code, PR number, repo) but the fullsend post-review command stderr is not captured, so the actual failure output is lost.
|
🤖 Finished Review · ❌ Failure · Started 3:35 PM UTC · Completed 3:49 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 4:01 PM UTC · Completed 4:16 PM UTC |
Retro: PR #2395 — add diagnostic stderr output to post-script failure pathsTimeline
What went well
Improvement areas (all already tracked)All systemic issues observed in this workflow are covered by existing open issues:
No new proposals filed — all improvement opportunities are already tracked. |
All exit 1 paths across the 6 post-scripts (post-triage, post-code, post-review, post-retro, post-fix, post-prioritize) now emit a clear error message to stderr before exiting. This addresses three categories of issues:
Silent exit paths: post-review.sh exited with the fullsend
post-review exit code but produced no diagnostic message.
post-fix.sh exited silently when process-fix-result.py failed
with bad input. Both now emit descriptive stderr messages.
Stdout-only errors: All echo "ERROR:..." and echo "::error::..."
messages now include >&2 to ensure they appear on stderr, making
them visible in GitHub Actions logs regardless of stdout buffering.
Missing context: HTTP-related failures now include the endpoint
or command that failed. The add_label function in post-triage.sh
captures and reports the gh API error output. Push failures in
post-code.sh include the push output. PR creation failures include
the head/base branch info. post-prioritize.sh errors include
project and org context.
Closes #2393
Post-script verification
agent/2393-post-script-diagnostic-errors)25d4659c9a0f620899e379ebe0894d45c0836016..HEAD)