-
Notifications
You must be signed in to change notification settings - Fork 94
fix(#2393): add diagnostic stderr output to post-script failure paths #2395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
12b47a9
f01e246
e972b2c
36186df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,7 @@ REPO_DIR="${REPO_DIR:-repo}" | |
|
|
||
| if [ "${REPO_DIR}" != "." ]; then | ||
| if [ ! -d "${REPO_DIR}" ]; then | ||
| echo "::error::Extracted repo not found at ${REPO_DIR}" | ||
| echo "::error::Extracted repo not found at ${REPO_DIR}" >&2 | ||
| exit 1 | ||
| fi | ||
| cd "${REPO_DIR}" | ||
|
|
@@ -215,9 +215,9 @@ echo "Secret scan passed — no leaks in agent's commit(s)" | |
| # --------------------------------------------------------------------------- | ||
| echo "Checking for Signed-off-by trailers in agent's commit(s)..." | ||
| if git log --format='%b' "${SCAN_RANGE}" | grep -q '^Signed-off-by:'; then | ||
| echo "::error::BLOCKED — agent commit contains a Signed-off-by trailer" | ||
| echo "::error::Agents must not use 'git commit -s' or append Signed-off-by trailers." | ||
| echo "::error::DCO is a human attestation; the DCO app waives the check for bots." | ||
| echo "::error::BLOCKED — agent commit contains a Signed-off-by trailer" >&2 | ||
| echo "::error::Agents must not use 'git commit -s' or append Signed-off-by trailers." >&2 | ||
| echo "::error::DCO is a human attestation; the DCO app waives the check for bots." >&2 | ||
| exit 1 | ||
| fi | ||
| echo "Signed-off-by scan passed — no trailers in agent's commit(s)" | ||
|
|
@@ -231,7 +231,7 @@ if ! command -v lychee >/dev/null 2>&1; then | |
| case "$(uname -m)" in | ||
| x86_64) LY_TRIPLE="x86_64-unknown-linux-gnu"; LY_SHA="${LYCHEE_SHA256_AMD64}" ;; | ||
| aarch64) LY_TRIPLE="aarch64-unknown-linux-gnu"; LY_SHA="${LYCHEE_SHA256_ARM64}" ;; | ||
| *) echo "::error::Unsupported architecture for lychee: $(uname -m)"; exit 1 ;; | ||
| *) echo "::error::Unsupported architecture for lychee: $(uname -m)" >&2; exit 1 ;; | ||
| esac | ||
| curl -fsSL \ | ||
| "https://github.com/lycheeverse/lychee/releases/download/lychee-v${LYCHEE_VERSION}/lychee-${LY_TRIPLE}.tar.gz" \ | ||
|
|
@@ -279,9 +279,9 @@ if [ -f .pre-commit-config.yaml ]; then | |
| if pre-commit run --files "${changed_array[@]}"; then | ||
| echo "Pre-commit passed — all hooks clean" | ||
| else | ||
| echo "::error::BLOCKED — pre-commit hooks failed on agent's changes" | ||
| echo "::error::The agent's code does not pass the repo's pre-commit hooks." | ||
| echo "::error::Fix the issues and re-run, or update the pre-commit config." | ||
| echo "::error::BLOCKED — pre-commit hooks failed on agent's changes" >&2 | ||
| echo "::error::The agent's code does not pass the repo's pre-commit hooks." >&2 | ||
| echo "::error::Fix the issues and re-run, or update the pre-commit config." >&2 | ||
| exit 1 | ||
| fi | ||
| else | ||
|
|
@@ -334,7 +334,8 @@ if [ "${PUSH_RC}" -ne 0 ]; then | |
| echo "::warning::Plain push failed (non-fast-forward) — retrying with --force-with-lease" | ||
| git push --force-with-lease -u origin -- "${BRANCH}" 2>&1 | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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::. |
||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| PR_CREATE_STDERR=$(mktemp) | ||
| if ! PR_URL=$(gh pr create \ | ||
| --repo "${REPO_FULL_NAME}" \ | ||
| --head "${BRANCH}" \ | ||
| --base "${TARGET_BRANCH}" \ | ||
| --title "${PR_TITLE}" \ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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>"${PR_CREATE_STDERR}"); then | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| echo "::error::Failed to create PR for ${REPO_FULL_NAME} (head: ${BRANCH}, base: ${TARGET_BRANCH})" >&2 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| [ -s "${PR_CREATE_STDERR}" ] && cat "${PR_CREATE_STDERR}" >&2 | ||
| rm -f "${PR_CREATE_STDERR}" | ||
| exit 1 | ||
| fi | ||
| rm -f "${PR_CREATE_STDERR}" | ||
|
|
||
| echo "PR created: ${PR_URL}" | ||
| echo "pr_url=${PR_URL}" >> "${GITHUB_OUTPUT:-/dev/null}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ set -euo pipefail | |
| : "${REVIEW_TOKEN:?REVIEW_TOKEN is required}" | ||
| : "${PR_NUMBER:?PR_NUMBER is required}" | ||
| if ! [[ "${PR_NUMBER}" =~ ^[0-9]+$ ]]; then | ||
| echo "::error::PR_NUMBER must be a positive integer" | ||
| echo "::error::PR_NUMBER must be a positive integer" >&2 | ||
| exit 1 | ||
| fi | ||
| : "${REPO_FULL_NAME:?REPO_FULL_NAME is required}" | ||
|
|
@@ -97,7 +97,7 @@ DOWNGRADED=false | |
| if [ "${ACTION}" = "approve" ]; then | ||
| 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 (gh pr view --json files)" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
|
|
@@ -177,6 +177,7 @@ ${REDISPATCH_MARKER}" || echo "::warning::Failed to post re-dispatch comment" | |
| # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| exit "${POST_REVIEW_EXIT}" | ||
| fi | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.