From 3582cbebb48e57e71c7d6b091341a8def7a915a2 Mon Sep 17 00:00:00 2001 From: fullsend-code Date: Mon, 27 Apr 2026 20:59:33 +0000 Subject: [PATCH 1/6] fix: skip code agent when human PR already exists for issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-code script now checks for existing open PRs linked to the target issue before the sandbox is created. If a human-authored PR is found (not from the fullsend bot), the script applies a "pr-open" label and posts a comment linking the existing PR(s), then exits cleanly — preventing the code agent from creating a duplicate competing PR. The check is best-effort: it only runs when GH_TOKEN is available and can be overridden with CODE_FORCE=true (set when a user comments `/code --force` on the issue). Bot-authored PRs are filtered out so the agent can still update its own previous work. Uses gh's built-in --jq flag for filtering, avoiding a standalone jq dependency. Note: go-test and go-vet could not run (Go not available in sandbox). post-triage-test.sh has pre-existing failures (jq not available in sandbox). pre-code-test.sh passed. Manual verification of Go tests is required. Closes #460 --- .../fullsend-repo/scripts/pre-code-test.sh | 241 ++++++++++++++++++ .../fullsend-repo/scripts/pre-code.sh | 65 +++++ 2 files changed, 306 insertions(+) create mode 100644 internal/scaffold/fullsend-repo/scripts/pre-code-test.sh diff --git a/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh b/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh new file mode 100644 index 000000000..ff146d4fe --- /dev/null +++ b/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh @@ -0,0 +1,241 @@ +#!/usr/bin/env bash +# pre-code-test.sh — Test pre-code.sh with mock gh to verify existing-PR check. +# +# Uses a mock gh command to capture calls without hitting GitHub. +# Run from the repo root: bash internal/scaffold/fullsend-repo/scripts/pre-code-test.sh + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PRE_SCRIPT="${SCRIPT_DIR}/pre-code.sh" +FAILURES=0 + +# Create a temp directory for mock state. +TMPDIR="$(mktemp -d)" +trap 'rm -rf "${TMPDIR}"' EXIT + +# --- Helpers --- + +# build_mock creates a mock gh binary that returns preconfigured responses. +# Arguments: +# $1 — output to return for "gh pr list" calls (tab-separated lines +# matching what gh --jq would produce, or empty for no PRs) +build_mock() { + local pr_list_output="$1" + local mock_bin="${TMPDIR}/bin" + local gh_log="${TMPDIR}/gh-calls.log" + + rm -rf "${mock_bin}" + mkdir -p "${mock_bin}" + > "${gh_log}" + + # Write the pr list output to a file so the mock can read it. + printf '%s' "${pr_list_output}" > "${TMPDIR}/pr-list-output.txt" + + cat > "${mock_bin}/gh" <<'MOCKEOF' +#!/usr/bin/env bash +CALL_LOG="LOGFILE_PLACEHOLDER" +PR_OUTPUT="OUTPUT_PLACEHOLDER" + +echo "gh $*" >> "${CALL_LOG}" + +# Route by subcommand +if [[ "$1" == "pr" && "$2" == "list" ]]; then + cat "${PR_OUTPUT}" +elif [[ "$1" == "label" ]]; then + exit 0 +elif [[ "$1" == "api" ]]; then + exit 0 +elif [[ "$1" == "issue" && "$2" == "comment" ]]; then + # Consume stdin (body-file reads from stdin) + cat > /dev/null + exit 0 +fi +MOCKEOF + + # Patch placeholders with actual paths (avoid sed on source files, + # but this is a generated mock — not repo source code). + local escaped_log="${gh_log//\//\\/}" + local escaped_out="${TMPDIR//\//\\/}\/pr-list-output.txt" + perl -pi -e "s/LOGFILE_PLACEHOLDER/${escaped_log}/g" "${mock_bin}/gh" + perl -pi -e "s/OUTPUT_PLACEHOLDER/${escaped_out}/g" "${mock_bin}/gh" + + chmod +x "${mock_bin}/gh" + + echo "${mock_bin}" +} + +run_test() { + local test_name="$1" + local pr_list_output="$2" + local expected_pattern="$3" + local expect_exit="$4" # 0 = success, 1 = failure + local extra_env="${5:-}" # additional env vars (KEY=VAL KEY2=VAL2) + + local mock_bin + mock_bin="$(build_mock "${pr_list_output}")" + local gh_log="${TMPDIR}/gh-calls.log" + + # Set base env vars for the script. + local env_cmd=( + env + PATH="${mock_bin}:${PATH}" + ISSUE_NUMBER="42" + REPO_FULL_NAME="test-org/test-repo" + GITHUB_ISSUE_URL="https://github.com/test-org/test-repo/issues/42" + GH_TOKEN="fake-token" + ) + + # Add extra env vars if provided. + if [[ -n "${extra_env}" ]]; then + for kv in ${extra_env}; do + env_cmd+=("${kv}") + done + fi + + local exit_code=0 + "${env_cmd[@]}" bash "${PRE_SCRIPT}" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? + + # Check exit code. + if [[ ${exit_code} -ne ${expect_exit} ]]; then + echo "FAIL: ${test_name} — expected exit ${expect_exit}, got ${exit_code}" + cat "${TMPDIR}/stdout.log" + FAILURES=$((FAILURES + 1)) + return + fi + + # Check expected pattern in gh calls (if provided). + if [[ -n "${expected_pattern}" ]]; then + if ! grep -qF "${expected_pattern}" "${gh_log}" 2>/dev/null; then + echo "FAIL: ${test_name} — expected gh call pattern '${expected_pattern}' not found" + echo "Actual calls:" + cat "${gh_log}" 2>/dev/null || echo "(no calls)" + FAILURES=$((FAILURES + 1)) + return + fi + fi + + echo "PASS: ${test_name}" +} + +# Check stdout contains a specific string. +run_test_stdout() { + local test_name="$1" + local pr_list_output="$2" + local expected_stdout="$3" + local expect_exit="$4" + local extra_env="${5:-}" + + local mock_bin + mock_bin="$(build_mock "${pr_list_output}")" + + local env_cmd=( + env + PATH="${mock_bin}:${PATH}" + ISSUE_NUMBER="42" + REPO_FULL_NAME="test-org/test-repo" + GITHUB_ISSUE_URL="https://github.com/test-org/test-repo/issues/42" + GH_TOKEN="fake-token" + ) + + if [[ -n "${extra_env}" ]]; then + for kv in ${extra_env}; do + env_cmd+=("${kv}") + done + fi + + local exit_code=0 + "${env_cmd[@]}" bash "${PRE_SCRIPT}" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? + + if [[ ${exit_code} -ne ${expect_exit} ]]; then + echo "FAIL: ${test_name} — expected exit ${expect_exit}, got ${exit_code}" + cat "${TMPDIR}/stdout.log" + FAILURES=$((FAILURES + 1)) + return + fi + + if ! grep -qF "${expected_stdout}" "${TMPDIR}/stdout.log" 2>/dev/null; then + echo "FAIL: ${test_name} — expected stdout '${expected_stdout}' not found" + echo "Actual stdout:" + cat "${TMPDIR}/stdout.log" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +# --- Test cases --- + +# Tab character for readability. +TAB=$'\t' + +# No existing PRs → agent proceeds (exit 0, no label/comment). +run_test_stdout "no-existing-prs-proceeds" \ + "" \ + "No existing human PRs found" \ + 0 + +# Human PR exists → should apply label and comment, then exit 0. +run_test "human-pr-applies-label" \ + "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ + "gh api repos/test-org/test-repo/issues/42/labels -f labels[]=pr-open --silent" \ + 0 + +run_test "human-pr-posts-comment" \ + "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ + "gh issue comment 42 --repo test-org/test-repo --body-file -" \ + 0 + +run_test_stdout "human-pr-skips-agent" \ + "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ + "Skipping code agent" \ + 0 + +# Bot PR only → gh --jq filters it out, so pr list returns empty → proceeds. +run_test_stdout "bot-pr-does-not-block" \ + "" \ + "No existing human PRs found" \ + 0 + +# CODE_FORCE=true → should skip check even with human PR. +run_test_stdout "force-override-skips-check" \ + "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ + "CODE_FORCE=true" \ + 0 \ + "CODE_FORCE=true" + +# No GH_TOKEN → skips check entirely, exits 0. +run_test_stdout "no-gh-token-skips-check" \ + "" \ + "GH_TOKEN not set" \ + 0 \ + "GH_TOKEN=" + +# Multiple human PRs → should block and apply label. +run_test "multiple-human-prs-block" \ + "50${TAB}dev-a${TAB}https://github.com/test-org/test-repo/pull/50 +51${TAB}dev-b${TAB}https://github.com/test-org/test-repo/pull/51" \ + "gh api repos/test-org/test-repo/issues/42/labels -f labels[]=pr-open --silent" \ + 0 + +run_test_stdout "multiple-human-prs-notice" \ + "50${TAB}dev-a${TAB}https://github.com/test-org/test-repo/pull/50 +51${TAB}dev-b${TAB}https://github.com/test-org/test-repo/pull/51" \ + "Found existing human PR #50 by @dev-a" \ + 0 + +# PR label gets created. +run_test "pr-label-created" \ + "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ + "gh label create pr-open --repo test-org/test-repo" \ + 0 + +# --- Summary --- + +echo "" +if [[ ${FAILURES} -gt 0 ]]; then + echo "${FAILURES} test(s) failed" + exit 1 +fi +echo "All tests passed" diff --git a/internal/scaffold/fullsend-repo/scripts/pre-code.sh b/internal/scaffold/fullsend-repo/scripts/pre-code.sh index eae9c65bd..4ff5bc8da 100755 --- a/internal/scaffold/fullsend-repo/scripts/pre-code.sh +++ b/internal/scaffold/fullsend-repo/scripts/pre-code.sh @@ -50,3 +50,68 @@ echo "Input validation passed:" echo " ISSUE_NUMBER=${ISSUE_NUMBER}" echo " REPO_FULL_NAME=${REPO_FULL_NAME}" echo " GITHUB_ISSUE_URL=${GITHUB_ISSUE_URL}" + +# --------------------------------------------------------------------------- +# Check for existing human PRs linked to this issue +# --------------------------------------------------------------------------- +# Skip if GH_TOKEN is not available (best-effort check). +if [[ -z "${GH_TOKEN:-}" ]]; then + echo "GH_TOKEN not set — skipping existing-PR check" + exit 0 +fi + +# Allow override via CODE_FORCE (set when /code --force is used). +if [[ "${CODE_FORCE:-}" == "true" ]]; then + echo "CODE_FORCE=true — skipping existing-PR check" + exit 0 +fi + +BOT_LOGIN="${FULLSEND_BOT_LOGIN:-fullsend-ai[bot]}" + +echo "Checking for existing open PRs linked to issue #${ISSUE_NUMBER}..." + +# Search for open PRs in the repo that mention the issue number. +# This catches PRs with "Closes #N", "Fixes #N", or "#N" in the body/title. +# Use gh's built-in --jq to filter out bot-authored PRs in one call. +HUMAN_PR_LINES="$(gh pr list --repo "${REPO_FULL_NAME}" --state open \ + --search "${ISSUE_NUMBER} in:body,title" \ + --json number,url,author \ + --jq "[.[] | select(.author.login != \"${BOT_LOGIN}\")] | .[] | \"\(.number)\t\(.author.login)\t\(.url)\"" \ + 2>/dev/null || true)" + +if [[ -n "${HUMAN_PR_LINES}" ]]; then + # Parse the first PR for the notice. + FIRST_PR_NUM="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f1)" + FIRST_PR_AUTHOR="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f2)" + + echo "::notice::Found existing human PR #${FIRST_PR_NUM} by @${FIRST_PR_AUTHOR}" + + # Apply pr-open label to signal work is already underway. + gh label create "pr-open" --repo "${REPO_FULL_NAME}" \ + --description "An open PR already addresses this issue" --color "D4C5F9" \ + --force 2>/dev/null || true + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ + -f "labels[]=pr-open" --silent 2>/dev/null || true + + # Build a markdown list of existing PRs. + PR_LIST_MD="" + while IFS=$'\t' read -r pr_num pr_author pr_url; do + PR_LIST_MD="${PR_LIST_MD} +- #${pr_num} by @${pr_author}" + done <<< "${HUMAN_PR_LINES}" + + COMMENT_BODY="An open PR already addresses this issue — skipping automated implementation. +${PR_LIST_MD} + +To override, comment \`/code --force\` on this issue. + +Posted by fullsend pre-code check" + + printf '%s' "${COMMENT_BODY}" | gh issue comment "${ISSUE_NUMBER}" \ + --repo "${REPO_FULL_NAME}" --body-file - 2>/dev/null || true + + echo "Skipping code agent — existing PR(s) found for issue #${ISSUE_NUMBER}" + exit 0 +fi + +echo "No existing human PRs found — proceeding with code agent" From 71ffe7dfa9e227c48b9587bc51baeb92de08fea9 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Sat, 2 May 2026 11:43:58 -0400 Subject: [PATCH 2/6] fix: use timeline API for PR detection, add input validation and idempotency Replace gh pr list --search with the timeline API to avoid substring false positives (e.g. issue #42 matching PRs mentioning #421). Add BOT_LOGIN regex validation to prevent jq injection, capture API errors instead of silent fail-open, refactor exit-on-missing-token to skip only the check block, and add comment idempotency to avoid duplicate bot comments. Update test suite with 7 new test cases covering API failure handling, bot login validation, comment idempotency, and timeline API usage. Signed-off-by: Wayne Sun --- .../fullsend-repo/scripts/pre-code-test.sh | 135 +++++++++++++----- .../fullsend-repo/scripts/pre-code.sh | 109 ++++++++------ 2 files changed, 169 insertions(+), 75 deletions(-) diff --git a/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh b/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh index ff146d4fe..dc3e31dc1 100644 --- a/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh @@ -18,10 +18,13 @@ trap 'rm -rf "${TMPDIR}"' EXIT # build_mock creates a mock gh binary that returns preconfigured responses. # Arguments: -# $1 — output to return for "gh pr list" calls (tab-separated lines -# matching what gh --jq would produce, or empty for no PRs) +# $1 — output for timeline API calls (JSON lines matching cross-ref events) +# $2 — output for comments API calls (jq-filtered count, default "0") +# $3 — exit code for timeline API calls (default 0) build_mock() { - local pr_list_output="$1" + local timeline_output="$1" + local comments_output="${2:-0}" + local timeline_exit="${3:-0}" local mock_bin="${TMPDIR}/bin" local gh_log="${TMPDIR}/gh-calls.log" @@ -29,36 +32,53 @@ build_mock() { mkdir -p "${mock_bin}" > "${gh_log}" - # Write the pr list output to a file so the mock can read it. - printf '%s' "${pr_list_output}" > "${TMPDIR}/pr-list-output.txt" + printf '%s' "${timeline_output}" > "${TMPDIR}/timeline-output.txt" + printf '%s' "${comments_output}" > "${TMPDIR}/comments-output.txt" + printf '%s' "${timeline_exit}" > "${TMPDIR}/timeline-exit.txt" cat > "${mock_bin}/gh" <<'MOCKEOF' #!/usr/bin/env bash -CALL_LOG="LOGFILE_PLACEHOLDER" -PR_OUTPUT="OUTPUT_PLACEHOLDER" +CALL_LOG="__LOGFILE__" +TIMELINE_OUTPUT="__TIMELINE__" +COMMENTS_OUTPUT="__COMMENTS__" +TIMELINE_EXIT="__TIMELINE_EXIT__" echo "gh $*" >> "${CALL_LOG}" -# Route by subcommand -if [[ "$1" == "pr" && "$2" == "list" ]]; then - cat "${PR_OUTPUT}" +if [[ "$1" == "api" ]]; then + URL="$2" + if [[ "${URL}" == *"/timeline"* ]]; then + ECODE="$(cat "${TIMELINE_EXIT}")" + if [[ "${ECODE}" -ne 0 ]]; then + echo "API error" >&2 + exit "${ECODE}" + fi + # Simulate --paginate --jq by just outputting the pre-filtered content. + cat "${TIMELINE_OUTPUT}" + exit 0 + elif [[ "${URL}" == *"/comments"* ]] && [[ "$*" == *"--jq"* ]]; then + cat "${COMMENTS_OUTPUT}" + exit 0 + elif [[ "${URL}" == *"/labels"* ]]; then + exit 0 + fi elif [[ "$1" == "label" ]]; then exit 0 -elif [[ "$1" == "api" ]]; then - exit 0 elif [[ "$1" == "issue" && "$2" == "comment" ]]; then - # Consume stdin (body-file reads from stdin) cat > /dev/null exit 0 fi MOCKEOF - # Patch placeholders with actual paths (avoid sed on source files, - # but this is a generated mock — not repo source code). + # Patch placeholders. local escaped_log="${gh_log//\//\\/}" - local escaped_out="${TMPDIR//\//\\/}\/pr-list-output.txt" - perl -pi -e "s/LOGFILE_PLACEHOLDER/${escaped_log}/g" "${mock_bin}/gh" - perl -pi -e "s/OUTPUT_PLACEHOLDER/${escaped_out}/g" "${mock_bin}/gh" + local escaped_timeline="${TMPDIR//\//\\/}\/timeline-output.txt" + local escaped_comments="${TMPDIR//\//\\/}\/comments-output.txt" + local escaped_exit="${TMPDIR//\//\\/}\/timeline-exit.txt" + perl -pi -e "s/__LOGFILE__/${escaped_log}/g" "${mock_bin}/gh" + perl -pi -e "s/__TIMELINE__/${escaped_timeline}/g" "${mock_bin}/gh" + perl -pi -e "s/__COMMENTS__/${escaped_comments}/g" "${mock_bin}/gh" + perl -pi -e "s/__TIMELINE_EXIT__/${escaped_exit}/g" "${mock_bin}/gh" chmod +x "${mock_bin}/gh" @@ -67,16 +87,17 @@ MOCKEOF run_test() { local test_name="$1" - local pr_list_output="$2" + local timeline_output="$2" local expected_pattern="$3" - local expect_exit="$4" # 0 = success, 1 = failure - local extra_env="${5:-}" # additional env vars (KEY=VAL KEY2=VAL2) + local expect_exit="$4" + local extra_env="${5:-}" + local comments_output="${6:-0}" + local timeline_exit="${7:-0}" local mock_bin - mock_bin="$(build_mock "${pr_list_output}")" + mock_bin="$(build_mock "${timeline_output}" "${comments_output}" "${timeline_exit}")" local gh_log="${TMPDIR}/gh-calls.log" - # Set base env vars for the script. local env_cmd=( env PATH="${mock_bin}:${PATH}" @@ -86,7 +107,6 @@ run_test() { GH_TOKEN="fake-token" ) - # Add extra env vars if provided. if [[ -n "${extra_env}" ]]; then for kv in ${extra_env}; do env_cmd+=("${kv}") @@ -96,7 +116,6 @@ run_test() { local exit_code=0 "${env_cmd[@]}" bash "${PRE_SCRIPT}" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? - # Check exit code. if [[ ${exit_code} -ne ${expect_exit} ]]; then echo "FAIL: ${test_name} — expected exit ${expect_exit}, got ${exit_code}" cat "${TMPDIR}/stdout.log" @@ -104,7 +123,6 @@ run_test() { return fi - # Check expected pattern in gh calls (if provided). if [[ -n "${expected_pattern}" ]]; then if ! grep -qF "${expected_pattern}" "${gh_log}" 2>/dev/null; then echo "FAIL: ${test_name} — expected gh call pattern '${expected_pattern}' not found" @@ -118,16 +136,17 @@ run_test() { echo "PASS: ${test_name}" } -# Check stdout contains a specific string. run_test_stdout() { local test_name="$1" - local pr_list_output="$2" + local timeline_output="$2" local expected_stdout="$3" local expect_exit="$4" local extra_env="${5:-}" + local comments_output="${6:-0}" + local timeline_exit="${7:-0}" local mock_bin - mock_bin="$(build_mock "${pr_list_output}")" + mock_bin="$(build_mock "${timeline_output}" "${comments_output}" "${timeline_exit}")" local env_cmd=( env @@ -167,10 +186,9 @@ run_test_stdout() { # --- Test cases --- -# Tab character for readability. TAB=$'\t' -# No existing PRs → agent proceeds (exit 0, no label/comment). +# No existing PRs → agent proceeds (exit 0). run_test_stdout "no-existing-prs-proceeds" \ "" \ "No existing human PRs found" \ @@ -192,7 +210,7 @@ run_test_stdout "human-pr-skips-agent" \ "Skipping code agent" \ 0 -# Bot PR only → gh --jq filters it out, so pr list returns empty → proceeds. +# Bot PR only → timeline returns empty → proceeds. run_test_stdout "bot-pr-does-not-block" \ "" \ "No existing human PRs found" \ @@ -231,6 +249,59 @@ run_test "pr-label-created" \ "gh label create pr-open --repo test-org/test-repo" \ 0 +# Timeline API uses correct endpoint. +run_test "timeline-api-called" \ + "" \ + "gh api repos/test-org/test-repo/issues/42/timeline --paginate --jq" \ + 0 + +# gh API failure → warns and proceeds (fail-open with warning). +run_test_stdout "api-failure-warns-and-proceeds" \ + "" \ + "Failed to check for existing PRs" \ + 0 \ + "" \ + "[]" \ + "1" + +run_test_stdout "api-failure-proceeds-with-agent" \ + "" \ + "No existing human PRs found" \ + 0 \ + "" \ + "[]" \ + "1" + +# Invalid FULLSEND_BOT_LOGIN → exits with error. +run_test_stdout "invalid-bot-login-rejected" \ + "" \ + "FULLSEND_BOT_LOGIN contains invalid characters" \ + 1 \ + "FULLSEND_BOT_LOGIN=evil\$(whoami)" + +# Valid custom FULLSEND_BOT_LOGIN → accepted. +run_test_stdout "valid-custom-bot-login" \ + "" \ + "No existing human PRs found" \ + 0 \ + "FULLSEND_BOT_LOGIN=my-bot[bot]" + +# Comment idempotency — existing comment → should skip posting. +run_test_stdout "idempotent-comment-skips-duplicate" \ + "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ + "Skipping duplicate comment" \ + 0 \ + "" \ + "1" + +# Comment idempotency — no existing comment → should post. +run_test "idempotent-comment-posts-new" \ + "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ + "gh issue comment 42 --repo test-org/test-repo --body-file -" \ + 0 \ + "" \ + "0" + # --- Summary --- echo "" diff --git a/internal/scaffold/fullsend-repo/scripts/pre-code.sh b/internal/scaffold/fullsend-repo/scripts/pre-code.sh index 4ff5bc8da..44b2b4354 100755 --- a/internal/scaffold/fullsend-repo/scripts/pre-code.sh +++ b/internal/scaffold/fullsend-repo/scripts/pre-code.sh @@ -54,64 +54,87 @@ echo " GITHUB_ISSUE_URL=${GITHUB_ISSUE_URL}" # --------------------------------------------------------------------------- # Check for existing human PRs linked to this issue # --------------------------------------------------------------------------- -# Skip if GH_TOKEN is not available (best-effort check). +SKIP_PR_CHECK=false + if [[ -z "${GH_TOKEN:-}" ]]; then echo "GH_TOKEN not set — skipping existing-PR check" - exit 0 + SKIP_PR_CHECK=true fi -# Allow override via CODE_FORCE (set when /code --force is used). if [[ "${CODE_FORCE:-}" == "true" ]]; then echo "CODE_FORCE=true — skipping existing-PR check" - exit 0 + SKIP_PR_CHECK=true fi -BOT_LOGIN="${FULLSEND_BOT_LOGIN:-fullsend-ai[bot]}" - -echo "Checking for existing open PRs linked to issue #${ISSUE_NUMBER}..." - -# Search for open PRs in the repo that mention the issue number. -# This catches PRs with "Closes #N", "Fixes #N", or "#N" in the body/title. -# Use gh's built-in --jq to filter out bot-authored PRs in one call. -HUMAN_PR_LINES="$(gh pr list --repo "${REPO_FULL_NAME}" --state open \ - --search "${ISSUE_NUMBER} in:body,title" \ - --json number,url,author \ - --jq "[.[] | select(.author.login != \"${BOT_LOGIN}\")] | .[] | \"\(.number)\t\(.author.login)\t\(.url)\"" \ - 2>/dev/null || true)" - -if [[ -n "${HUMAN_PR_LINES}" ]]; then - # Parse the first PR for the notice. - FIRST_PR_NUM="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f1)" - FIRST_PR_AUTHOR="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f2)" - - echo "::notice::Found existing human PR #${FIRST_PR_NUM} by @${FIRST_PR_AUTHOR}" - - # Apply pr-open label to signal work is already underway. - gh label create "pr-open" --repo "${REPO_FULL_NAME}" \ - --description "An open PR already addresses this issue" --color "D4C5F9" \ - --force 2>/dev/null || true - gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ - -f "labels[]=pr-open" --silent 2>/dev/null || true - - # Build a markdown list of existing PRs. - PR_LIST_MD="" - while IFS=$'\t' read -r pr_num pr_author pr_url; do - PR_LIST_MD="${PR_LIST_MD} +if [[ "${SKIP_PR_CHECK}" != "true" ]]; then + BOT_LOGIN="${FULLSEND_BOT_LOGIN:-fullsend-ai[bot]}" + BOT_LOGIN_RE='^[][a-zA-Z0-9._-]+$' + if [[ ! "${BOT_LOGIN}" =~ ${BOT_LOGIN_RE} ]]; then + echo "::error::FULLSEND_BOT_LOGIN contains invalid characters: '${BOT_LOGIN}'" + exit 1 + fi + + echo "Checking for existing open PRs linked to issue #${ISSUE_NUMBER}..." + + # Use the timeline API to find PRs that reference this issue via + # cross-reference events. This avoids substring false positives from + # gh pr list --search (e.g. issue #42 matching a PR mentioning #421). + PR_LIST_EXIT=0 + HUMAN_PR_LINES="$(gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/timeline" \ + --paginate --jq ' + [.[] + | select(.event == "cross-referenced") + | select(.source.issue.pull_request != null) + | select(.source.issue.state == "open") + | select(.source.issue.user.login != "'"${BOT_LOGIN}"'") + | "\(.source.issue.number)\t\(.source.issue.user.login)\t\(.source.issue.html_url)" + ] | unique | .[]' 2>&1)" || PR_LIST_EXIT=$? + + if [[ ${PR_LIST_EXIT} -ne 0 ]]; then + echo "::warning::Failed to check for existing PRs (exit ${PR_LIST_EXIT}): ${HUMAN_PR_LINES}" + HUMAN_PR_LINES="" + fi + + if [[ -n "${HUMAN_PR_LINES}" ]]; then + FIRST_PR_NUM="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f1)" + FIRST_PR_AUTHOR="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f2)" + + echo "::notice::Found existing human PR #${FIRST_PR_NUM} by @${FIRST_PR_AUTHOR}" + + gh label create "pr-open" --repo "${REPO_FULL_NAME}" \ + --description "An open PR already addresses this issue" --color "D4C5F9" \ + --force 2>/dev/null || true + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ + -f "labels[]=pr-open" --silent 2>/dev/null || true + + PR_LIST_MD="" + while IFS=$'\t' read -r pr_num pr_author pr_url; do + PR_LIST_MD="${PR_LIST_MD} - #${pr_num} by @${pr_author}" - done <<< "${HUMAN_PR_LINES}" + done <<< "${HUMAN_PR_LINES}" - COMMENT_BODY="An open PR already addresses this issue — skipping automated implementation. + COMMENT_BODY="An open PR already addresses this issue — skipping automated implementation. ${PR_LIST_MD} To override, comment \`/code --force\` on this issue. Posted by fullsend pre-code check" - printf '%s' "${COMMENT_BODY}" | gh issue comment "${ISSUE_NUMBER}" \ - --repo "${REPO_FULL_NAME}" --body-file - 2>/dev/null || true + # Check for existing bot comment to avoid duplicates. + EXISTING_COMMENT="$(gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/comments" \ + --jq '[.[] | select(.body | startswith("An open PR already addresses"))] | length' \ + 2>/dev/null || echo "0")" - echo "Skipping code agent — existing PR(s) found for issue #${ISSUE_NUMBER}" - exit 0 -fi + if [[ "${EXISTING_COMMENT}" == "0" ]]; then + printf '%s' "${COMMENT_BODY}" | gh issue comment "${ISSUE_NUMBER}" \ + --repo "${REPO_FULL_NAME}" --body-file - 2>/dev/null || true + else + echo "::notice::Skipping duplicate comment — bot already posted on issue #${ISSUE_NUMBER}" + fi -echo "No existing human PRs found — proceeding with code agent" + echo "Skipping code agent — existing PR(s) found for issue #${ISSUE_NUMBER}" + exit 0 + fi + + echo "No existing human PRs found — proceeding with code agent" +fi From f729f093a186c1e00c2eebb7f6ad22fbc2d2cff0 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Sat, 2 May 2026 13:03:23 -0400 Subject: [PATCH 3/6] feat(cli): migrate pre-code.sh to fullsend gate code command Replace the bash pre-code.sh script with a Go CLI command that runs in the workflow before sandbox creation. This addresses the review findings on PR #473: - Fix skip semantics: write skip=true to GITHUB_OUTPUT and gate all downstream workflow steps with if: steps.gate.outputs.skip != 'true' - Fix dead GH_TOKEN: pass push-token to the gate step env - Fix search false positives: use timeline API instead of text search - Add cross-validation between ISSUE_NUMBER/REPO_FULL_NAME/GITHUB_ISSUE_URL - Add bot-login regex validation against injection Extend forge.Client with ListIssueTimeline, AddIssueComment, EnsureLabel, and AddIssueLabels. Implement on both LiveClient (GitHub API) and FakeClient (test double). Closes #460 Signed-off-by: Wayne Sun --- e2e/admin/admin_test.go | 1 - internal/cli/gate.go | 235 +++++++++++++ internal/cli/gate_test.go | 257 +++++++++++++++ internal/cli/root.go | 1 + internal/forge/fake.go | 93 +++++- internal/forge/forge.go | 13 + internal/forge/github/github.go | 102 ++++++ .../fullsend-repo/.github/workflows/code.yml | 25 +- .../scaffold/fullsend-repo/harness/code.yaml | 7 +- .../fullsend-repo/scripts/pre-code-test.sh | 312 ------------------ .../fullsend-repo/scripts/pre-code.sh | 140 -------- internal/scaffold/scaffold_test.go | 9 +- 12 files changed, 719 insertions(+), 476 deletions(-) create mode 100644 internal/cli/gate.go create mode 100644 internal/cli/gate_test.go delete mode 100644 internal/scaffold/fullsend-repo/scripts/pre-code-test.sh delete mode 100755 internal/scaffold/fullsend-repo/scripts/pre-code.sh diff --git a/e2e/admin/admin_test.go b/e2e/admin/admin_test.go index 6449b4e52..2fc63b1a2 100644 --- a/e2e/admin/admin_test.go +++ b/e2e/admin/admin_test.go @@ -417,7 +417,6 @@ func verifyInstalled(t *testing.T, env *e2eEnv, orgCfg *config.OrgConfig, enable "env/code-agent.env", "env/gcp-vertex.env", "scripts/scan-secrets", - "scripts/pre-code.sh", "scripts/post-code.sh", "scripts/post-triage.sh", "scripts/reconcile-repos.sh", diff --git a/internal/cli/gate.go b/internal/cli/gate.go new file mode 100644 index 000000000..28a2fe9d2 --- /dev/null +++ b/internal/cli/gate.go @@ -0,0 +1,235 @@ +package cli + +import ( + "context" + "fmt" + "net/url" + "os" + "regexp" + "strconv" + "strings" + + "github.com/spf13/cobra" + + "github.com/fullsend-ai/fullsend/internal/forge" + gh "github.com/fullsend-ai/fullsend/internal/forge/github" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +func newGateCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "gate", + Short: "Pre-agent gates that run before sandbox creation", + Long: `Gates validate inputs and check preconditions on the GitHub Actions +runner BEFORE sandbox creation. Each subcommand corresponds to an +agent stage (code, triage, review).`, + } + cmd.AddCommand(newGateCodeCmd()) + return cmd +} + +func newGateCodeCmd() *cobra.Command { + return &cobra.Command{ + Use: "code", + Short: "Gate for the code agent — validates inputs and checks for existing PRs", + Long: `Validates workflow_dispatch inputs and checks whether a human PR +already addresses the target issue. If a human PR exists, applies +the pr-open label, posts an informational comment, and writes +skip=true to GITHUB_OUTPUT so downstream workflow steps can be +conditionally skipped. + +Required environment variables: + ISSUE_NUMBER — positive integer + REPO_FULL_NAME — owner/repo format + GITHUB_ISSUE_URL — valid GitHub issue URL + +Optional: + GH_TOKEN / GITHUB_TOKEN — required for PR check + CODE_FORCE=true — skip PR check + FULLSEND_BOT_LOGIN — bot login to filter (default: fullsend-ai[bot])`, + RunE: func(cmd *cobra.Command, args []string) error { + printer := ui.New(os.Stdout) + + cfg := gateCodeConfig{ + IssueNumber: os.Getenv("ISSUE_NUMBER"), + RepoFullName: os.Getenv("REPO_FULL_NAME"), + IssueURL: os.Getenv("GITHUB_ISSUE_URL"), + BotLogin: os.Getenv("FULLSEND_BOT_LOGIN"), + Force: os.Getenv("CODE_FORCE") == "true", + OutputFile: os.Getenv("GITHUB_OUTPUT"), + } + + token, _ := resolveToken() + if token != "" { + cfg.Client = gh.New(token) + } + + return runGateCode(cmd.Context(), cfg, printer) + }, + } +} + +type gateCodeConfig struct { + IssueNumber string + RepoFullName string + IssueURL string + BotLogin string + Force bool + Client forge.Client + OutputFile string // GITHUB_OUTPUT path for writing step outputs +} + +var ( + issueNumberRe = regexp.MustCompile(`^[1-9][0-9]*$`) + repoFullNameRe = regexp.MustCompile(`^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$`) + issueURLRe = regexp.MustCompile(`^https://github\.com/[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+/issues/[0-9]+$`) + botLoginRe = regexp.MustCompile(`^[][a-zA-Z0-9._-]+$`) +) + +func validateGateCodeInputs(issueNumber, repoFullName, issueURL string) []string { + var errs []string + + if !issueNumberRe.MatchString(issueNumber) { + errs = append(errs, fmt.Sprintf("ISSUE_NUMBER must be a positive integer, got: '%s'", issueNumber)) + } + if !repoFullNameRe.MatchString(repoFullName) { + errs = append(errs, fmt.Sprintf("REPO_FULL_NAME must be owner/repo format, got: '%s'", repoFullName)) + } + if !issueURLRe.MatchString(issueURL) { + errs = append(errs, fmt.Sprintf("GITHUB_ISSUE_URL format invalid, got: '%s'", issueURL)) + } + + if issueURLRe.MatchString(issueURL) && repoFullNameRe.MatchString(repoFullName) { + u, err := url.Parse(issueURL) + if err == nil { + parts := strings.Split(strings.TrimPrefix(u.Path, "/"), "/") + if len(parts) >= 4 { + urlRepo := parts[0] + "/" + parts[1] + urlIssue := parts[3] + if urlRepo != repoFullName { + errs = append(errs, fmt.Sprintf("REPO_FULL_NAME does not match issue URL repo ('%s' vs '%s')", repoFullName, urlRepo)) + } + if urlIssue != issueNumber { + errs = append(errs, fmt.Sprintf("ISSUE_NUMBER does not match issue URL number ('%s' vs '%s')", issueNumber, urlIssue)) + } + } + } + } + + return errs +} + +func runGateCode(ctx context.Context, cfg gateCodeConfig, printer *ui.Printer) error { + printer.Raw(fmt.Sprintf("::notice::Code target: %s\n", cfg.IssueURL)) + + errs := validateGateCodeInputs(cfg.IssueNumber, cfg.RepoFullName, cfg.IssueURL) + if len(errs) > 0 { + for _, e := range errs { + printer.Raw(fmt.Sprintf("::error::%s\n", e)) + } + return fmt.Errorf("input validation failed with %d error(s)", len(errs)) + } + + printer.StepDone("Input validation passed") + printer.KeyValue("ISSUE_NUMBER", cfg.IssueNumber) + printer.KeyValue("REPO_FULL_NAME", cfg.RepoFullName) + printer.KeyValue("GITHUB_ISSUE_URL", cfg.IssueURL) + + if cfg.Client == nil { + printer.StepInfo("GH_TOKEN not set — skipping existing-PR check") + return nil + } + + if cfg.Force { + printer.StepInfo("CODE_FORCE=true — skipping existing-PR check") + return nil + } + + botLogin := cfg.BotLogin + if botLogin == "" { + botLogin = "fullsend-ai[bot]" + } + if !botLoginRe.MatchString(botLogin) { + printer.Raw(fmt.Sprintf("::error::FULLSEND_BOT_LOGIN contains invalid characters: '%s'\n", botLogin)) + return fmt.Errorf("FULLSEND_BOT_LOGIN contains invalid characters: '%s'", botLogin) + } + + parts := strings.SplitN(cfg.RepoFullName, "/", 2) + owner, repo := parts[0], parts[1] + issueNum, _ := strconv.Atoi(cfg.IssueNumber) + + printer.StepStart(fmt.Sprintf("Checking for existing open PRs linked to issue #%d...", issueNum)) + + events, err := cfg.Client.ListIssueTimeline(ctx, owner, repo, issueNum) + if err != nil { + printer.Raw(fmt.Sprintf("::warning::Failed to check for existing PRs: %v\n", err)) + printer.StepInfo("No existing human PRs found — proceeding with code agent") + return nil + } + + var humanPRs []forge.TimelineEvent + for _, e := range events { + if e.PRState == "open" && e.PRAuthor != botLogin { + humanPRs = append(humanPRs, e) + } + } + + if len(humanPRs) == 0 { + printer.StepDone("No existing human PRs found — proceeding with code agent") + return nil + } + + first := humanPRs[0] + printer.Raw(fmt.Sprintf("::notice::Found existing human PR #%d by @%s\n", first.PRNumber, first.PRAuthor)) + + _ = cfg.Client.EnsureLabel(ctx, owner, repo, "pr-open", "An open PR already addresses this issue", "D4C5F9") + _ = cfg.Client.AddIssueLabels(ctx, owner, repo, issueNum, []string{"pr-open"}) + + var prListMD string + for _, pr := range humanPRs { + prListMD += fmt.Sprintf("\n- #%d by @%s", pr.PRNumber, pr.PRAuthor) + } + + commentBody := fmt.Sprintf(`An open PR already addresses this issue — skipping automated implementation. +%s + +To override, comment `+"`/code --force`"+` on this issue. + +Posted by fullsend pre-code check`, prListMD) + + comments, err := cfg.Client.ListIssueComments(ctx, owner, repo, issueNum) + if err != nil { + comments = nil + } + + hasExisting := false + for _, c := range comments { + if strings.HasPrefix(c.Body, "An open PR already addresses") { + hasExisting = true + break + } + } + + if !hasExisting { + _ = cfg.Client.AddIssueComment(ctx, owner, repo, issueNum, commentBody) + } else { + printer.Raw(fmt.Sprintf("::notice::Skipping duplicate comment — bot already posted on issue #%d\n", issueNum)) + } + + writeGateOutput(cfg.OutputFile, "skip", "true") + + printer.StepDone(fmt.Sprintf("Skipping code agent — existing PR(s) found for issue #%d", issueNum)) + return nil +} + +func writeGateOutput(path, key, value string) { + if path == "" { + return + } + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + return + } + defer f.Close() + fmt.Fprintf(f, "%s=%s\n", key, value) +} diff --git a/internal/cli/gate_test.go b/internal/cli/gate_test.go new file mode 100644 index 000000000..78f2c5e16 --- /dev/null +++ b/internal/cli/gate_test.go @@ -0,0 +1,257 @@ +package cli + +import ( + "context" + "fmt" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +func newTestPrinter() *ui.Printer { + return ui.New(&discardWriter{}) +} + +type discardWriter struct{} + +func (d *discardWriter) Write(p []byte) (int, error) { return len(p), nil } + +func validConfig(client forge.Client) gateCodeConfig { + return gateCodeConfig{ + IssueNumber: "42", + RepoFullName: "test-org/test-repo", + IssueURL: "https://github.com/test-org/test-repo/issues/42", + Client: client, + } +} + +func TestValidateGateCodeInputs_Valid(t *testing.T) { + errs := validateGateCodeInputs("42", "test-org/test-repo", "https://github.com/test-org/test-repo/issues/42") + assert.Empty(t, errs) +} + +func TestValidateGateCodeInputs_InvalidNumber(t *testing.T) { + errs := validateGateCodeInputs("0", "test-org/test-repo", "https://github.com/test-org/test-repo/issues/42") + assert.Len(t, errs, 2) // invalid number + cross-validation mismatch +} + +func TestValidateGateCodeInputs_InvalidRepo(t *testing.T) { + errs := validateGateCodeInputs("42", "bad repo!", "https://github.com/test-org/test-repo/issues/42") + require.NotEmpty(t, errs) + assert.Contains(t, errs[0], "REPO_FULL_NAME") +} + +func TestValidateGateCodeInputs_InvalidURL(t *testing.T) { + errs := validateGateCodeInputs("42", "test-org/test-repo", "http://evil.com/issues/42") + require.NotEmpty(t, errs) + assert.Contains(t, errs[0], "GITHUB_ISSUE_URL") +} + +func TestValidateGateCodeInputs_CrossValidationMismatch(t *testing.T) { + errs := validateGateCodeInputs("42", "other-org/other-repo", "https://github.com/test-org/test-repo/issues/42") + assert.Len(t, errs, 1) + assert.Contains(t, errs[0], "does not match") +} + +func TestValidateGateCodeInputs_MultipleErrors(t *testing.T) { + errs := validateGateCodeInputs("", "", "") + assert.True(t, len(errs) >= 3, "expected at least 3 errors, got %d", len(errs)) +} + +func TestRunGateCode_NoToken(t *testing.T) { + cfg := validConfig(nil) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateCode_ForceOverride(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "open", PRAuthor: "human-dev", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + cfg := validConfig(fc) + cfg.Force = true + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + assert.Empty(t, fc.AddedComments) +} + +func TestRunGateCode_HumanPRFound_WritesSkipOutput(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "open", PRAuthor: "human-dev", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + outFile := t.TempDir() + "/github-output" + cfg := validConfig(fc) + cfg.OutputFile = outFile + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + data, err := os.ReadFile(outFile) + require.NoError(t, err) + assert.Contains(t, string(data), "skip=true") +} + +func TestRunGateCode_NoExistingPRs_NoSkipOutput(t *testing.T) { + fc := forge.NewFakeClient() + outFile := t.TempDir() + "/github-output" + cfg := validConfig(fc) + cfg.OutputFile = outFile + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + _, err = os.ReadFile(outFile) + assert.True(t, os.IsNotExist(err), "output file should not exist when no PRs found") +} + +func TestRunGateCode_HumanPRFound_AppliesLabel(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "open", PRAuthor: "human-dev", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + require.Len(t, fc.EnsuredLabels, 1) + assert.Equal(t, []string{"pr-open"}, fc.EnsuredLabels[0].Labels) + require.Len(t, fc.AddedIssueLabels, 1) + assert.Equal(t, []string{"pr-open"}, fc.AddedIssueLabels[0].Labels) + assert.Equal(t, 42, fc.AddedIssueLabels[0].Number) +} + +func TestRunGateCode_HumanPRFound_PostsComment(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "open", PRAuthor: "human-dev", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + require.Len(t, fc.AddedComments, 1) + assert.Contains(t, fc.AddedComments[0].Body, "An open PR already addresses") + assert.Contains(t, fc.AddedComments[0].Body, "#99 by @human-dev") +} + +func TestRunGateCode_BotPRFiltered(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "open", PRAuthor: "fullsend-ai[bot]", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + assert.Empty(t, fc.AddedComments) + assert.Empty(t, fc.EnsuredLabels) +} + +func TestRunGateCode_MultiplePRs(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 50, PRState: "open", PRAuthor: "dev-a", PRURL: "https://github.com/test-org/test-repo/pull/50"}, + {PRNumber: 51, PRState: "open", PRAuthor: "dev-b", PRURL: "https://github.com/test-org/test-repo/pull/51"}, + }, + } + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + require.Len(t, fc.AddedComments, 1) + assert.Contains(t, fc.AddedComments[0].Body, "#50 by @dev-a") + assert.Contains(t, fc.AddedComments[0].Body, "#51 by @dev-b") +} + +func TestRunGateCode_CommentIdempotency_SkipsDuplicate(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "open", PRAuthor: "human-dev", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + fc.IssueComments = map[string][]forge.IssueComment{ + "test-org/test-repo/42": { + {ID: 1, Body: "An open PR already addresses this issue — skipping automated implementation.", Author: "fullsend-ai[bot]"}, + }, + } + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + assert.Empty(t, fc.AddedComments) +} + +func TestRunGateCode_CommentIdempotency_PostsNew(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "open", PRAuthor: "human-dev", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + fc.IssueComments = map[string][]forge.IssueComment{ + "test-org/test-repo/42": {}, + } + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + require.Len(t, fc.AddedComments, 1) +} + +func TestRunGateCode_TimelineAPIFailure(t *testing.T) { + fc := forge.NewFakeClient() + fc.Errors["ListIssueTimeline"] = fmt.Errorf("API error") + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateCode_InvalidBotLogin(t *testing.T) { + fc := forge.NewFakeClient() + cfg := validConfig(fc) + cfg.BotLogin = "evil$(whoami)" + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid characters") +} + +func TestRunGateCode_ValidCustomBotLogin(t *testing.T) { + fc := forge.NewFakeClient() + cfg := validConfig(fc) + cfg.BotLogin = "my-bot[bot]" + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateCode_ClosedPRIgnored(t *testing.T) { + fc := forge.NewFakeClient() + fc.TimelineEvents = map[string][]forge.TimelineEvent{ + "test-org/test-repo/42": { + {PRNumber: 99, PRState: "closed", PRAuthor: "human-dev", PRURL: "https://github.com/test-org/test-repo/pull/99"}, + }, + } + cfg := validConfig(fc) + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) + assert.Empty(t, fc.AddedComments) +} + +func TestRunGateCode_ValidationFailure(t *testing.T) { + cfg := gateCodeConfig{ + IssueNumber: "bad", + RepoFullName: "bad!", + IssueURL: "bad", + } + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "validation failed") +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 5bda415b6..00262a22d 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -20,6 +20,7 @@ func newRootCmd() *cobra.Command { cmd.AddCommand(newScanCmd()) cmd.AddCommand(newPostReviewCmd()) cmd.AddCommand(newPostCommentCmd()) + cmd.AddCommand(newGateCmd()) return cmd } diff --git a/internal/forge/fake.go b/internal/forge/fake.go index 48b4d89f8..cfee26ac2 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -26,6 +26,20 @@ type FileRecord struct { Content []byte } +// CommentRecord records an AddIssueComment call. +type CommentRecord struct { + Owner, Repo string + Number int + Body string +} + +// LabelRecord records an EnsureLabel or AddIssueLabels call. +type LabelRecord struct { + Owner, Repo string + Number int // 0 for EnsureLabel + Labels []string +} + // SecretRecord records a secret creation call. type SecretRecord struct { Owner, Repo, Name, Value string @@ -71,14 +85,18 @@ type FakeClient struct { // Pre-populated data Repos []Repository - FileContents map[string][]byte // key: "owner/repo/path" - WorkflowRuns map[string]*WorkflowRun // key: "owner/repo/workflow" + FileContents map[string][]byte // key: "owner/repo/path" + WorkflowRuns map[string]*WorkflowRun // key: "owner/repo/workflow" AuthenticatedUser string Installations []Installation Secrets map[string]bool // key: "owner/repo/name" PullRequests map[string][]ChangeProposal // key: "owner/repo" TokenScopes []string // scopes returned by GetTokenScopes VariablesExist map[string]bool // key: "owner/repo/name" + IssueComments map[string][]IssueComment // key: "owner/repo/number" + TimelineEvents map[string][]TimelineEvent // key: "owner/repo/number" + PullRequestHeadSHA string + PRReviews map[string][]PullRequestReview // key: "owner/repo/number" // App client IDs for GetAppClientID AppClientIDs map[string]string // key: app slug → client ID @@ -90,15 +108,6 @@ type FakeClient struct { // Error injection: key is method name, value is error to return. Errors map[string]error - // Issue comments for ListIssueComments / UpdateIssueComment. - IssueComments map[string][]IssueComment // key: "owner/repo/number" - - // Pull request head SHA for GetPullRequestHeadSHA. - PullRequestHeadSHA string - - // Pull request reviews for ListPullRequestReviews. - PRReviews map[string][]PullRequestReview // key: "owner/repo/number" - // Call recorders CreatedRepos []Repository CreatedFiles []FileRecord @@ -113,6 +122,9 @@ type FakeClient struct { UpdatedComments []UpdatedCommentRecord MinimizedComments []MinimizedCommentRecord CreatedReviews []ReviewRecord + AddedComments []CommentRecord + EnsuredLabels []LabelRecord + AddedIssueLabels []LabelRecord // internal counters proposalCounter int @@ -683,6 +695,65 @@ func (f *FakeClient) ListPullRequestReviews(_ context.Context, owner, repo strin return nil, nil } +func (f *FakeClient) ListIssueTimeline(_ context.Context, owner, repo string, number int) ([]TimelineEvent, error) { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("ListIssueTimeline"); e != nil { + return nil, e + } + if f.TimelineEvents != nil { + key := fmt.Sprintf("%s/%s/%d", owner, repo, number) + if events, ok := f.TimelineEvents[key]; ok { + return events, nil + } + } + return nil, nil +} + +func (f *FakeClient) AddIssueComment(_ context.Context, owner, repo string, number int, body string) error { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("AddIssueComment"); e != nil { + return e + } + f.AddedComments = append(f.AddedComments, CommentRecord{ + Owner: owner, + Repo: repo, + Number: number, + Body: body, + }) + return nil +} + +func (f *FakeClient) EnsureLabel(_ context.Context, owner, repo, name, description, color string) error { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("EnsureLabel"); e != nil { + return e + } + f.EnsuredLabels = append(f.EnsuredLabels, LabelRecord{ + Owner: owner, + Repo: repo, + Labels: []string{name}, + }) + return nil +} + +func (f *FakeClient) AddIssueLabels(_ context.Context, owner, repo string, number int, labels []string) error { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("AddIssueLabels"); e != nil { + return e + } + f.AddedIssueLabels = append(f.AddedIssueLabels, LabelRecord{ + Owner: owner, + Repo: repo, + Number: number, + Labels: labels, + }) + return nil +} + func (f *FakeClient) MergeChangeProposal(_ context.Context, _, _ string, _ int) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index a50ddcfa2..1be3ac774 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -75,6 +75,15 @@ type PullRequestReview struct { SubmittedAt string } +// TimelineEvent represents a cross-reference event from the issue timeline, +// linking an issue to a pull request. +type TimelineEvent struct { + PRNumber int + PRState string // "open", "closed" + PRAuthor string + PRURL string +} + // Installation represents an app installation on an org. type Installation struct { ID int @@ -157,6 +166,10 @@ type Client interface { CreateIssueComment(ctx context.Context, owner, repo string, number int, body string) (*IssueComment, error) UpdateIssueComment(ctx context.Context, owner, repo string, commentID int, body string) error MinimizeComment(ctx context.Context, nodeID, reason string) error + ListIssueTimeline(ctx context.Context, owner, repo string, number int) ([]TimelineEvent, error) + AddIssueComment(ctx context.Context, owner, repo string, number int, body string) error + EnsureLabel(ctx context.Context, owner, repo, name, description, color string) error + AddIssueLabels(ctx context.Context, owner, repo string, number int, labels []string) error // Pull request operations GetPullRequestHeadSHA(ctx context.Context, owner, repo string, number int) (string, error) diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index c73cc56ab..420a264d3 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1195,6 +1195,108 @@ func (c *LiveClient) ListPullRequestReviews(ctx context.Context, owner, repo str return result, nil } +// ListIssueTimeline returns cross-reference events linking PRs to an issue. +// It paginates through the timeline API and filters for cross-referenced +// events that have an associated pull request. +func (c *LiveClient) ListIssueTimeline(ctx context.Context, owner, repo string, number int) ([]forge.TimelineEvent, error) { + var result []forge.TimelineEvent + seen := make(map[int]bool) + + for page := 1; page <= 100; page++ { + path := fmt.Sprintf("/repos/%s/%s/issues/%d/timeline?per_page=100&page=%d", owner, repo, number, page) + resp, err := c.do(ctx, http.MethodGet, path, nil) + if err != nil { + return nil, fmt.Errorf("list issue timeline page %d: %w", page, err) + } + if err := checkStatus(resp, http.StatusOK); err != nil { + return nil, fmt.Errorf("list issue timeline page %d: %w", page, err) + } + + var events []struct { + Event string `json:"event"` + Source struct { + Issue struct { + Number int `json:"number"` + State string `json:"state"` + HTMLURL string `json:"html_url"` + PullRequest *struct{} `json:"pull_request"` + User struct { + Login string `json:"login"` + } `json:"user"` + } `json:"issue"` + } `json:"source"` + } + if err := decodeJSON(resp, &events); err != nil { + return nil, fmt.Errorf("decode timeline page %d: %w", page, err) + } + + for _, e := range events { + if e.Event != "cross-referenced" || e.Source.Issue.PullRequest == nil { + continue + } + prNum := e.Source.Issue.Number + if seen[prNum] { + continue + } + seen[prNum] = true + result = append(result, forge.TimelineEvent{ + PRNumber: prNum, + PRState: e.Source.Issue.State, + PRAuthor: e.Source.Issue.User.Login, + PRURL: e.Source.Issue.HTMLURL, + }) + } + + if len(events) < 100 { + break + } + } + + return result, nil +} + +// AddIssueComment posts a comment on an issue. +func (c *LiveClient) AddIssueComment(ctx context.Context, owner, repo string, number int, body string) error { + payload := map[string]string{"body": body} + resp, err := c.post(ctx, fmt.Sprintf("/repos/%s/%s/issues/%d/comments", owner, repo, number), payload) + if err != nil { + return fmt.Errorf("add issue comment: %w", err) + } + resp.Body.Close() + return nil +} + +// EnsureLabel creates a label if it doesn't already exist. A 422 response +// (label already exists) is treated as success. +func (c *LiveClient) EnsureLabel(ctx context.Context, owner, repo, name, description, color string) error { + payload := map[string]string{ + "name": name, + "description": description, + "color": color, + } + resp, err := c.do(ctx, http.MethodPost, fmt.Sprintf("/repos/%s/%s/labels", owner, repo), payload) + if err != nil { + return fmt.Errorf("ensure label %s: %w", name, err) + } + defer resp.Body.Close() + // 201 = created, 422 = already exists — both are fine. + if err := checkStatus(resp, http.StatusCreated, http.StatusUnprocessableEntity); err != nil { + return fmt.Errorf("ensure label %s: %w", name, err) + } + return nil +} + +// AddIssueLabels adds labels to an issue. +func (c *LiveClient) AddIssueLabels(ctx context.Context, owner, repo string, number int, labels []string) error { + payload := map[string][]string{"labels": labels} + resp, err := c.post(ctx, fmt.Sprintf("/repos/%s/%s/issues/%d/labels", owner, repo, number), payload) + if err != nil { + return fmt.Errorf("add issue labels: %w", err) + } + resp.Body.Close() + return nil +} + // MergeChangeProposal squash-merges a pull request by number. func (c *LiveClient) MergeChangeProposal(ctx context.Context, owner, repo string, number int) error { resp, err := c.put(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, number), map[string]string{"merge_method": "squash"}) diff --git a/internal/scaffold/fullsend-repo/.github/workflows/code.yml b/internal/scaffold/fullsend-repo/.github/workflows/code.yml index db20dce09..d85416b21 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/code.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/code.yml @@ -94,25 +94,38 @@ jobs: fetch-depth: 0 persist-credentials: false + - name: Install fullsend CLI + run: | + if [[ -f "${GITHUB_WORKSPACE}/bin/fullsend" ]]; then + chmod +x "${GITHUB_WORKSPACE}/bin/fullsend" + echo "${GITHUB_WORKSPACE}/bin" >> "${GITHUB_PATH}" + else + echo "::error::fullsend binary not found; run: fullsend admin install --vendor-fullsend-binary" + exit 1 + fi + - name: Validate inputs + id: gate env: ISSUE_NUMBER: ${{ fromJSON(inputs.event_payload).issue.number }} REPO_FULL_NAME: ${{ inputs.source_repo }} GITHUB_ISSUE_URL: ${{ fromJSON(inputs.event_payload).issue.html_url }} - run: bash scripts/pre-code.sh + GH_TOKEN: ${{ steps.push-token.outputs.token }} + run: fullsend gate code - name: Pre-mask GCP credential file path + if: steps.gate.outputs.skip != 'true' run: echo "::add-mask::${GITHUB_WORKSPACE}/gha-creds-" - name: Authenticate to Google Cloud (WIF) - if: vars.FULLSEND_GCP_AUTH_MODE == 'wif' + if: steps.gate.outputs.skip != 'true' && vars.FULLSEND_GCP_AUTH_MODE == 'wif' uses: google-github-actions/auth@v3 with: workload_identity_provider: ${{ secrets.FULLSEND_GCP_WIF_PROVIDER }} service_account: ${{ secrets.FULLSEND_GCP_WIF_SA_EMAIL }} - name: Authenticate to Google Cloud (SA key) - if: vars.FULLSEND_GCP_AUTH_MODE != 'wif' + if: steps.gate.outputs.skip != 'true' && vars.FULLSEND_GCP_AUTH_MODE != 'wif' uses: google-github-actions/auth@v3 with: credentials_json: ${{ secrets.FULLSEND_GCP_SA_KEY_JSON }} @@ -121,12 +134,13 @@ jobs: # WIF. For non-WIF (SA key), we set it to an empty file to avoid undefined # variable errors in downstream steps that may reference it. - name: Set GCP_OIDC_TOKEN_FILE for non-WIF - if: vars.FULLSEND_GCP_AUTH_MODE != 'wif' + if: steps.gate.outputs.skip != 'true' && vars.FULLSEND_GCP_AUTH_MODE != 'wif' run: | touch "$RUNNER_TEMP/empty-oidc-token" echo "GCP_OIDC_TOKEN_FILE=$RUNNER_TEMP/empty-oidc-token" >> "${GITHUB_ENV}" - name: Mask GCP credential file paths + if: steps.gate.outputs.skip != 'true' run: | for var in GOOGLE_GHA_CREDS_PATH GOOGLE_APPLICATION_CREDENTIALS CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE; do val="${!var:-}" @@ -136,9 +150,11 @@ jobs: done - name: Prepare sandbox credentials + if: steps.gate.outputs.skip != 'true' run: bash scripts/prepare-sandbox-credentials.sh - name: Setup agent environment + if: steps.gate.outputs.skip != 'true' env: AGENT_PREFIX: CODE_ CODE_GH_TOKEN: ${{ steps.sandbox-token.outputs.token }} @@ -149,6 +165,7 @@ jobs: run: bash .github/scripts/setup-agent-env.sh - name: Run code agent + if: steps.gate.outputs.skip != 'true' uses: ./.github/actions/fullsend env: GITHUB_ISSUE_URL: ${{ fromJSON(inputs.event_payload).issue.html_url }} diff --git a/internal/scaffold/fullsend-repo/harness/code.yaml b/internal/scaffold/fullsend-repo/harness/code.yaml index 9d5849342..f4becfb67 100644 --- a/internal/scaffold/fullsend-repo/harness/code.yaml +++ b/internal/scaffold/fullsend-repo/harness/code.yaml @@ -1,7 +1,7 @@ -# harness/code.yaml — code agent with pre/post script pipeline. +# harness/code.yaml — code agent with post-script pipeline. # -# Flow: pre_script → sandbox (agent) → post_script -# pre_script : validates inputs on the runner BEFORE sandbox creation +# Flow: (workflow: fullsend gate code) → sandbox (agent) → post_script +# gate : validates inputs and checks for existing PRs (runs in workflow) # agent : reads the issue, implements, tests, scans, commits locally # post_script : protected-path check, secret scan, push branch, create PR # @@ -12,7 +12,6 @@ model: opus image: ghcr.io/fullsend-ai/fullsend-code:latest policy: policies/code.yaml -pre_script: scripts/pre-code.sh post_script: scripts/post-code.sh host_files: diff --git a/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh b/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh deleted file mode 100644 index dc3e31dc1..000000000 --- a/internal/scaffold/fullsend-repo/scripts/pre-code-test.sh +++ /dev/null @@ -1,312 +0,0 @@ -#!/usr/bin/env bash -# pre-code-test.sh — Test pre-code.sh with mock gh to verify existing-PR check. -# -# Uses a mock gh command to capture calls without hitting GitHub. -# Run from the repo root: bash internal/scaffold/fullsend-repo/scripts/pre-code-test.sh - -set -euo pipefail - -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -PRE_SCRIPT="${SCRIPT_DIR}/pre-code.sh" -FAILURES=0 - -# Create a temp directory for mock state. -TMPDIR="$(mktemp -d)" -trap 'rm -rf "${TMPDIR}"' EXIT - -# --- Helpers --- - -# build_mock creates a mock gh binary that returns preconfigured responses. -# Arguments: -# $1 — output for timeline API calls (JSON lines matching cross-ref events) -# $2 — output for comments API calls (jq-filtered count, default "0") -# $3 — exit code for timeline API calls (default 0) -build_mock() { - local timeline_output="$1" - local comments_output="${2:-0}" - local timeline_exit="${3:-0}" - local mock_bin="${TMPDIR}/bin" - local gh_log="${TMPDIR}/gh-calls.log" - - rm -rf "${mock_bin}" - mkdir -p "${mock_bin}" - > "${gh_log}" - - printf '%s' "${timeline_output}" > "${TMPDIR}/timeline-output.txt" - printf '%s' "${comments_output}" > "${TMPDIR}/comments-output.txt" - printf '%s' "${timeline_exit}" > "${TMPDIR}/timeline-exit.txt" - - cat > "${mock_bin}/gh" <<'MOCKEOF' -#!/usr/bin/env bash -CALL_LOG="__LOGFILE__" -TIMELINE_OUTPUT="__TIMELINE__" -COMMENTS_OUTPUT="__COMMENTS__" -TIMELINE_EXIT="__TIMELINE_EXIT__" - -echo "gh $*" >> "${CALL_LOG}" - -if [[ "$1" == "api" ]]; then - URL="$2" - if [[ "${URL}" == *"/timeline"* ]]; then - ECODE="$(cat "${TIMELINE_EXIT}")" - if [[ "${ECODE}" -ne 0 ]]; then - echo "API error" >&2 - exit "${ECODE}" - fi - # Simulate --paginate --jq by just outputting the pre-filtered content. - cat "${TIMELINE_OUTPUT}" - exit 0 - elif [[ "${URL}" == *"/comments"* ]] && [[ "$*" == *"--jq"* ]]; then - cat "${COMMENTS_OUTPUT}" - exit 0 - elif [[ "${URL}" == *"/labels"* ]]; then - exit 0 - fi -elif [[ "$1" == "label" ]]; then - exit 0 -elif [[ "$1" == "issue" && "$2" == "comment" ]]; then - cat > /dev/null - exit 0 -fi -MOCKEOF - - # Patch placeholders. - local escaped_log="${gh_log//\//\\/}" - local escaped_timeline="${TMPDIR//\//\\/}\/timeline-output.txt" - local escaped_comments="${TMPDIR//\//\\/}\/comments-output.txt" - local escaped_exit="${TMPDIR//\//\\/}\/timeline-exit.txt" - perl -pi -e "s/__LOGFILE__/${escaped_log}/g" "${mock_bin}/gh" - perl -pi -e "s/__TIMELINE__/${escaped_timeline}/g" "${mock_bin}/gh" - perl -pi -e "s/__COMMENTS__/${escaped_comments}/g" "${mock_bin}/gh" - perl -pi -e "s/__TIMELINE_EXIT__/${escaped_exit}/g" "${mock_bin}/gh" - - chmod +x "${mock_bin}/gh" - - echo "${mock_bin}" -} - -run_test() { - local test_name="$1" - local timeline_output="$2" - local expected_pattern="$3" - local expect_exit="$4" - local extra_env="${5:-}" - local comments_output="${6:-0}" - local timeline_exit="${7:-0}" - - local mock_bin - mock_bin="$(build_mock "${timeline_output}" "${comments_output}" "${timeline_exit}")" - local gh_log="${TMPDIR}/gh-calls.log" - - local env_cmd=( - env - PATH="${mock_bin}:${PATH}" - ISSUE_NUMBER="42" - REPO_FULL_NAME="test-org/test-repo" - GITHUB_ISSUE_URL="https://github.com/test-org/test-repo/issues/42" - GH_TOKEN="fake-token" - ) - - if [[ -n "${extra_env}" ]]; then - for kv in ${extra_env}; do - env_cmd+=("${kv}") - done - fi - - local exit_code=0 - "${env_cmd[@]}" bash "${PRE_SCRIPT}" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? - - if [[ ${exit_code} -ne ${expect_exit} ]]; then - echo "FAIL: ${test_name} — expected exit ${expect_exit}, got ${exit_code}" - cat "${TMPDIR}/stdout.log" - FAILURES=$((FAILURES + 1)) - return - fi - - if [[ -n "${expected_pattern}" ]]; then - if ! grep -qF "${expected_pattern}" "${gh_log}" 2>/dev/null; then - echo "FAIL: ${test_name} — expected gh call pattern '${expected_pattern}' not found" - echo "Actual calls:" - cat "${gh_log}" 2>/dev/null || echo "(no calls)" - FAILURES=$((FAILURES + 1)) - return - fi - fi - - echo "PASS: ${test_name}" -} - -run_test_stdout() { - local test_name="$1" - local timeline_output="$2" - local expected_stdout="$3" - local expect_exit="$4" - local extra_env="${5:-}" - local comments_output="${6:-0}" - local timeline_exit="${7:-0}" - - local mock_bin - mock_bin="$(build_mock "${timeline_output}" "${comments_output}" "${timeline_exit}")" - - local env_cmd=( - env - PATH="${mock_bin}:${PATH}" - ISSUE_NUMBER="42" - REPO_FULL_NAME="test-org/test-repo" - GITHUB_ISSUE_URL="https://github.com/test-org/test-repo/issues/42" - GH_TOKEN="fake-token" - ) - - if [[ -n "${extra_env}" ]]; then - for kv in ${extra_env}; do - env_cmd+=("${kv}") - done - fi - - local exit_code=0 - "${env_cmd[@]}" bash "${PRE_SCRIPT}" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? - - if [[ ${exit_code} -ne ${expect_exit} ]]; then - echo "FAIL: ${test_name} — expected exit ${expect_exit}, got ${exit_code}" - cat "${TMPDIR}/stdout.log" - FAILURES=$((FAILURES + 1)) - return - fi - - if ! grep -qF "${expected_stdout}" "${TMPDIR}/stdout.log" 2>/dev/null; then - echo "FAIL: ${test_name} — expected stdout '${expected_stdout}' not found" - echo "Actual stdout:" - cat "${TMPDIR}/stdout.log" - FAILURES=$((FAILURES + 1)) - return - fi - - echo "PASS: ${test_name}" -} - -# --- Test cases --- - -TAB=$'\t' - -# No existing PRs → agent proceeds (exit 0). -run_test_stdout "no-existing-prs-proceeds" \ - "" \ - "No existing human PRs found" \ - 0 - -# Human PR exists → should apply label and comment, then exit 0. -run_test "human-pr-applies-label" \ - "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ - "gh api repos/test-org/test-repo/issues/42/labels -f labels[]=pr-open --silent" \ - 0 - -run_test "human-pr-posts-comment" \ - "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ - "gh issue comment 42 --repo test-org/test-repo --body-file -" \ - 0 - -run_test_stdout "human-pr-skips-agent" \ - "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ - "Skipping code agent" \ - 0 - -# Bot PR only → timeline returns empty → proceeds. -run_test_stdout "bot-pr-does-not-block" \ - "" \ - "No existing human PRs found" \ - 0 - -# CODE_FORCE=true → should skip check even with human PR. -run_test_stdout "force-override-skips-check" \ - "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ - "CODE_FORCE=true" \ - 0 \ - "CODE_FORCE=true" - -# No GH_TOKEN → skips check entirely, exits 0. -run_test_stdout "no-gh-token-skips-check" \ - "" \ - "GH_TOKEN not set" \ - 0 \ - "GH_TOKEN=" - -# Multiple human PRs → should block and apply label. -run_test "multiple-human-prs-block" \ - "50${TAB}dev-a${TAB}https://github.com/test-org/test-repo/pull/50 -51${TAB}dev-b${TAB}https://github.com/test-org/test-repo/pull/51" \ - "gh api repos/test-org/test-repo/issues/42/labels -f labels[]=pr-open --silent" \ - 0 - -run_test_stdout "multiple-human-prs-notice" \ - "50${TAB}dev-a${TAB}https://github.com/test-org/test-repo/pull/50 -51${TAB}dev-b${TAB}https://github.com/test-org/test-repo/pull/51" \ - "Found existing human PR #50 by @dev-a" \ - 0 - -# PR label gets created. -run_test "pr-label-created" \ - "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ - "gh label create pr-open --repo test-org/test-repo" \ - 0 - -# Timeline API uses correct endpoint. -run_test "timeline-api-called" \ - "" \ - "gh api repos/test-org/test-repo/issues/42/timeline --paginate --jq" \ - 0 - -# gh API failure → warns and proceeds (fail-open with warning). -run_test_stdout "api-failure-warns-and-proceeds" \ - "" \ - "Failed to check for existing PRs" \ - 0 \ - "" \ - "[]" \ - "1" - -run_test_stdout "api-failure-proceeds-with-agent" \ - "" \ - "No existing human PRs found" \ - 0 \ - "" \ - "[]" \ - "1" - -# Invalid FULLSEND_BOT_LOGIN → exits with error. -run_test_stdout "invalid-bot-login-rejected" \ - "" \ - "FULLSEND_BOT_LOGIN contains invalid characters" \ - 1 \ - "FULLSEND_BOT_LOGIN=evil\$(whoami)" - -# Valid custom FULLSEND_BOT_LOGIN → accepted. -run_test_stdout "valid-custom-bot-login" \ - "" \ - "No existing human PRs found" \ - 0 \ - "FULLSEND_BOT_LOGIN=my-bot[bot]" - -# Comment idempotency — existing comment → should skip posting. -run_test_stdout "idempotent-comment-skips-duplicate" \ - "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ - "Skipping duplicate comment" \ - 0 \ - "" \ - "1" - -# Comment idempotency — no existing comment → should post. -run_test "idempotent-comment-posts-new" \ - "99${TAB}human-dev${TAB}https://github.com/test-org/test-repo/pull/99" \ - "gh issue comment 42 --repo test-org/test-repo --body-file -" \ - 0 \ - "" \ - "0" - -# --- Summary --- - -echo "" -if [[ ${FAILURES} -gt 0 ]]; then - echo "${FAILURES} test(s) failed" - exit 1 -fi -echo "All tests passed" diff --git a/internal/scaffold/fullsend-repo/scripts/pre-code.sh b/internal/scaffold/fullsend-repo/scripts/pre-code.sh deleted file mode 100755 index 44b2b4354..000000000 --- a/internal/scaffold/fullsend-repo/scripts/pre-code.sh +++ /dev/null @@ -1,140 +0,0 @@ -#!/usr/bin/env bash -# Pre-script: validate workflow_dispatch inputs before the agent runs. -# -# Prevents malformed or malicious event_payload from reaching the sandbox. -# Runs on the GitHub Actions runner BEFORE sandbox creation. -# -# Required environment variables (set by the workflow): -# ISSUE_NUMBER — must be a positive integer -# REPO_FULL_NAME — must be owner/repo format -# GITHUB_ISSUE_URL — must be a valid GitHub issue URL -set -euo pipefail - -echo "::notice::🔗 Code target: ${GITHUB_ISSUE_URL:-}" - -errors=0 - -if [[ ! "${ISSUE_NUMBER:-}" =~ ^[1-9][0-9]*$ ]]; then - echo "::error::ISSUE_NUMBER must be a positive integer, got: '${ISSUE_NUMBER:-}'" - errors=$((errors + 1)) -fi - -if [[ ! "${REPO_FULL_NAME:-}" =~ ^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$ ]]; then - echo "::error::REPO_FULL_NAME must be owner/repo format, got: '${REPO_FULL_NAME:-}'" - errors=$((errors + 1)) -fi - -if [[ ! "${GITHUB_ISSUE_URL:-}" =~ ^https://github\.com/[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+/issues/[0-9]+$ ]]; then - echo "::error::GITHUB_ISSUE_URL format invalid, got: '${GITHUB_ISSUE_URL:-}'" - errors=$((errors + 1)) -fi - -URL_REPO="$(echo "${GITHUB_ISSUE_URL:-}" | sed -E 's|https://github.com/([^/]+/[^/]+)/issues/.*|\1|')" -URL_ISSUE="$(echo "${GITHUB_ISSUE_URL:-}" | sed -E 's|.*/issues/([0-9]+)$|\1|')" - -if [[ -n "${URL_REPO}" && "${URL_REPO}" != "${REPO_FULL_NAME:-}" ]]; then - echo "::error::REPO_FULL_NAME does not match issue URL repo ('${REPO_FULL_NAME:-}' vs '${URL_REPO}')" - errors=$((errors + 1)) -fi -if [[ -n "${URL_ISSUE}" && "${URL_ISSUE}" != "${ISSUE_NUMBER:-}" ]]; then - echo "::error::ISSUE_NUMBER does not match issue URL number ('${ISSUE_NUMBER:-}' vs '${URL_ISSUE}')" - errors=$((errors + 1)) -fi - -if [[ "${errors}" -gt 0 ]]; then - echo "::error::Input validation failed with ${errors} error(s). Aborting." - exit 1 -fi - -echo "Input validation passed:" -echo " ISSUE_NUMBER=${ISSUE_NUMBER}" -echo " REPO_FULL_NAME=${REPO_FULL_NAME}" -echo " GITHUB_ISSUE_URL=${GITHUB_ISSUE_URL}" - -# --------------------------------------------------------------------------- -# Check for existing human PRs linked to this issue -# --------------------------------------------------------------------------- -SKIP_PR_CHECK=false - -if [[ -z "${GH_TOKEN:-}" ]]; then - echo "GH_TOKEN not set — skipping existing-PR check" - SKIP_PR_CHECK=true -fi - -if [[ "${CODE_FORCE:-}" == "true" ]]; then - echo "CODE_FORCE=true — skipping existing-PR check" - SKIP_PR_CHECK=true -fi - -if [[ "${SKIP_PR_CHECK}" != "true" ]]; then - BOT_LOGIN="${FULLSEND_BOT_LOGIN:-fullsend-ai[bot]}" - BOT_LOGIN_RE='^[][a-zA-Z0-9._-]+$' - if [[ ! "${BOT_LOGIN}" =~ ${BOT_LOGIN_RE} ]]; then - echo "::error::FULLSEND_BOT_LOGIN contains invalid characters: '${BOT_LOGIN}'" - exit 1 - fi - - echo "Checking for existing open PRs linked to issue #${ISSUE_NUMBER}..." - - # Use the timeline API to find PRs that reference this issue via - # cross-reference events. This avoids substring false positives from - # gh pr list --search (e.g. issue #42 matching a PR mentioning #421). - PR_LIST_EXIT=0 - HUMAN_PR_LINES="$(gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/timeline" \ - --paginate --jq ' - [.[] - | select(.event == "cross-referenced") - | select(.source.issue.pull_request != null) - | select(.source.issue.state == "open") - | select(.source.issue.user.login != "'"${BOT_LOGIN}"'") - | "\(.source.issue.number)\t\(.source.issue.user.login)\t\(.source.issue.html_url)" - ] | unique | .[]' 2>&1)" || PR_LIST_EXIT=$? - - if [[ ${PR_LIST_EXIT} -ne 0 ]]; then - echo "::warning::Failed to check for existing PRs (exit ${PR_LIST_EXIT}): ${HUMAN_PR_LINES}" - HUMAN_PR_LINES="" - fi - - if [[ -n "${HUMAN_PR_LINES}" ]]; then - FIRST_PR_NUM="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f1)" - FIRST_PR_AUTHOR="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f2)" - - echo "::notice::Found existing human PR #${FIRST_PR_NUM} by @${FIRST_PR_AUTHOR}" - - gh label create "pr-open" --repo "${REPO_FULL_NAME}" \ - --description "An open PR already addresses this issue" --color "D4C5F9" \ - --force 2>/dev/null || true - gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ - -f "labels[]=pr-open" --silent 2>/dev/null || true - - PR_LIST_MD="" - while IFS=$'\t' read -r pr_num pr_author pr_url; do - PR_LIST_MD="${PR_LIST_MD} -- #${pr_num} by @${pr_author}" - done <<< "${HUMAN_PR_LINES}" - - COMMENT_BODY="An open PR already addresses this issue — skipping automated implementation. -${PR_LIST_MD} - -To override, comment \`/code --force\` on this issue. - -Posted by fullsend pre-code check" - - # Check for existing bot comment to avoid duplicates. - EXISTING_COMMENT="$(gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/comments" \ - --jq '[.[] | select(.body | startswith("An open PR already addresses"))] | length' \ - 2>/dev/null || echo "0")" - - if [[ "${EXISTING_COMMENT}" == "0" ]]; then - printf '%s' "${COMMENT_BODY}" | gh issue comment "${ISSUE_NUMBER}" \ - --repo "${REPO_FULL_NAME}" --body-file - 2>/dev/null || true - else - echo "::notice::Skipping duplicate comment — bot already posted on issue #${ISSUE_NUMBER}" - fi - - echo "Skipping code agent — existing PR(s) found for issue #${ISSUE_NUMBER}" - exit 0 - fi - - echo "No existing human PRs found — proceeding with code agent" -fi diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index ca3135479..52dc3b4e1 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -35,7 +35,6 @@ func TestFullsendRepoFilesExist(t *testing.T) { "scripts/post-triage.sh", "scripts/pre-triage.sh", "scripts/scan-secrets", - "scripts/pre-code.sh", "scripts/pre-review.sh", "scripts/post-code.sh", "scripts/reconcile-repos.sh", @@ -137,7 +136,7 @@ func TestWalkFullsendRepo(t *testing.T) { return nil }) require.NoError(t, err) - assert.True(t, len(paths) >= 29, "expected at least 29 files, got %d", len(paths)) + assert.True(t, len(paths) >= 28, "expected at least 28 files, got %d", len(paths)) } func TestTriageWorkflowContent(t *testing.T) { @@ -180,7 +179,7 @@ func TestCodeWorkflowContent(t *testing.T) { s := string(content) assert.Contains(t, s, "workflow_dispatch") assert.Contains(t, s, "FULLSEND_CODER_CLIENT_ID") - assert.Contains(t, s, "pre-code.sh") + assert.Contains(t, s, "fullsend gate code") assert.Contains(t, s, "PUSH_TOKEN") assert.Contains(t, s, "github-app") assert.Contains(t, s, "sandbox-token") @@ -190,6 +189,9 @@ func TestCodeWorkflowContent(t *testing.T) { assert.Contains(t, s, "concurrency:") assert.Contains(t, s, "fullsend-code-") assert.Contains(t, s, "cancel-in-progress: true") + // Verify gate step has id and downstream steps check skip output + assert.Contains(t, s, "id: gate") + assert.Contains(t, s, "steps.gate.outputs.skip") } func TestReviewWorkflowContent(t *testing.T) { @@ -235,7 +237,6 @@ func TestCodeHarnessContent(t *testing.T) { require.NoError(t, err) s := string(content) assert.Contains(t, s, "agents/code.md") - assert.Contains(t, s, "pre_script") assert.Contains(t, s, "post_script") assert.Contains(t, s, "runner_env") assert.Contains(t, s, "PUSH_TOKEN") From 3b5051e7404ac16684c68df63b196ccd2a229b8e Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Sat, 2 May 2026 13:14:47 -0400 Subject: [PATCH 4/6] feat(cli): add gate fix command, address review findings - Add `fullsend gate fix` subcommand migrating pre-fix.sh to Go: input validation, iteration cap enforcement, instruction length check - Address review findings: consolidate AddIssueComment into existing CreateIssueComment, log gate errors as ::warning:: instead of silently discarding - Update fix.yml workflow to use `fullsend gate fix` instead of `bash scripts/pre-fix.sh` - Remove pre_script from fix harness (gate runs on runner before sandbox) - Delete scripts/pre-fix.sh (102 lines of bash replaced by tested Go) - Add 15 new gate fix test cases covering validation, iteration caps, instruction limits, bot vs human paths Signed-off-by: Wayne Sun --- e2e/admin/admin_test.go | 1 - internal/cli/gate.go | 151 ++++++++++++++- internal/cli/gate_test.go | 180 ++++++++++++++++++ internal/forge/fake.go | 21 +- internal/forge/forge.go | 1 - internal/forge/github/github.go | 11 -- .../fullsend-repo/.github/workflows/fix.yml | 12 +- .../scaffold/fullsend-repo/harness/fix.yaml | 8 +- .../scaffold/fullsend-repo/scripts/pre-fix.sh | 101 ---------- internal/scaffold/scaffold_test.go | 4 +- 10 files changed, 351 insertions(+), 139 deletions(-) delete mode 100644 internal/scaffold/fullsend-repo/scripts/pre-fix.sh diff --git a/e2e/admin/admin_test.go b/e2e/admin/admin_test.go index 2fc63b1a2..816a9b619 100644 --- a/e2e/admin/admin_test.go +++ b/e2e/admin/admin_test.go @@ -428,7 +428,6 @@ func verifyInstalled(t *testing.T, env *e2eEnv, orgCfg *config.OrgConfig, enable "policies/fix.yaml", "env/fix-agent.env", "schemas/fix-result.schema.json", - "scripts/pre-fix.sh", "scripts/post-fix.sh", "scripts/process-fix-result.py", "templates/shim-workflow.yaml", diff --git a/internal/cli/gate.go b/internal/cli/gate.go index 28a2fe9d2..14e8a23f9 100644 --- a/internal/cli/gate.go +++ b/internal/cli/gate.go @@ -25,6 +25,7 @@ runner BEFORE sandbox creation. Each subcommand corresponds to an agent stage (code, triage, review).`, } cmd.AddCommand(newGateCodeCmd()) + cmd.AddCommand(newGateFixCmd()) return cmd } @@ -182,8 +183,12 @@ func runGateCode(ctx context.Context, cfg gateCodeConfig, printer *ui.Printer) e first := humanPRs[0] printer.Raw(fmt.Sprintf("::notice::Found existing human PR #%d by @%s\n", first.PRNumber, first.PRAuthor)) - _ = cfg.Client.EnsureLabel(ctx, owner, repo, "pr-open", "An open PR already addresses this issue", "D4C5F9") - _ = cfg.Client.AddIssueLabels(ctx, owner, repo, issueNum, []string{"pr-open"}) + if err := cfg.Client.EnsureLabel(ctx, owner, repo, "pr-open", "An open PR already addresses this issue", "D4C5F9"); err != nil { + printer.Raw(fmt.Sprintf("::warning::Failed to ensure pr-open label: %v\n", err)) + } + if err := cfg.Client.AddIssueLabels(ctx, owner, repo, issueNum, []string{"pr-open"}); err != nil { + printer.Raw(fmt.Sprintf("::warning::Failed to add pr-open label to issue #%d: %v\n", issueNum, err)) + } var prListMD string for _, pr := range humanPRs { @@ -211,7 +216,9 @@ To override, comment `+"`/code --force`"+` on this issue. } if !hasExisting { - _ = cfg.Client.AddIssueComment(ctx, owner, repo, issueNum, commentBody) + if _, err := cfg.Client.CreateIssueComment(ctx, owner, repo, issueNum, commentBody); err != nil { + printer.Raw(fmt.Sprintf("::warning::Failed to post comment on issue #%d: %v\n", issueNum, err)) + } } else { printer.Raw(fmt.Sprintf("::notice::Skipping duplicate comment — bot already posted on issue #%d\n", issueNum)) } @@ -222,6 +229,144 @@ To override, comment `+"`/code --force`"+` on this issue. return nil } +func newGateFixCmd() *cobra.Command { + return &cobra.Command{ + Use: "fix", + Short: "Gate for the fix agent — validates inputs and enforces iteration caps", + Long: `Validates workflow_dispatch inputs for the fix agent and enforces +iteration caps to prevent unbounded fix loops. + +Required environment variables: + PR_NUMBER — positive integer + REPO_FULL_NAME — owner/repo format + TRIGGER_SOURCE — GitHub username that triggered the fix + +Optional: + FIX_ITERATION — current iteration count (default: 1) + ITERATION_CAP — max bot-triggered iterations (default: 5) + ITERATION_CAP_HUMAN — max human-triggered iterations (default: 10) + HUMAN_INSTRUCTION — instruction text (validated for length)`, + RunE: func(cmd *cobra.Command, args []string) error { + printer := ui.New(os.Stdout) + + cfg := gateFixConfig{ + PRNumber: os.Getenv("PR_NUMBER"), + RepoFullName: os.Getenv("REPO_FULL_NAME"), + TriggerSource: os.Getenv("TRIGGER_SOURCE"), + Iteration: os.Getenv("FIX_ITERATION"), + IterationCap: os.Getenv("ITERATION_CAP"), + IterationCapHuman: os.Getenv("ITERATION_CAP_HUMAN"), + HumanInstruction: os.Getenv("HUMAN_INSTRUCTION"), + } + + return runGateFix(cmd.Context(), cfg, printer) + }, + } +} + +type gateFixConfig struct { + PRNumber string + RepoFullName string + TriggerSource string + Iteration string + IterationCap string + IterationCapHuman string + HumanInstruction string +} + +const maxInstructionBytes = 10000 + +func isBotUser(username string) bool { + return strings.HasSuffix(username, "[bot]") +} + +func validateGateFixInputs(prNumber, repoFullName, triggerSource string) []string { + var errs []string + + if !issueNumberRe.MatchString(prNumber) { + errs = append(errs, fmt.Sprintf("PR_NUMBER must be a positive integer, got: '%s'", prNumber)) + } + if !repoFullNameRe.MatchString(repoFullName) { + errs = append(errs, fmt.Sprintf("REPO_FULL_NAME must be owner/repo format, got: '%s'", repoFullName)) + } + if triggerSource == "" { + errs = append(errs, "TRIGGER_SOURCE is required (GitHub username that triggered the fix)") + } + + return errs +} + +func runGateFix(_ context.Context, cfg gateFixConfig, printer *ui.Printer) error { + errs := validateGateFixInputs(cfg.PRNumber, cfg.RepoFullName, cfg.TriggerSource) + if len(errs) > 0 { + for _, e := range errs { + printer.Raw(fmt.Sprintf("::error::%s\n", e)) + } + return fmt.Errorf("input validation failed with %d error(s)", len(errs)) + } + + // Human instruction length cap + if !isBotUser(cfg.TriggerSource) && len(cfg.HumanInstruction) > maxInstructionBytes { + printer.Raw(fmt.Sprintf("::error::HUMAN_INSTRUCTION is %d bytes (max: %d). Truncate the instruction.\n", + len(cfg.HumanInstruction), maxInstructionBytes)) + return fmt.Errorf("HUMAN_INSTRUCTION is %d bytes (max: %d)", len(cfg.HumanInstruction), maxInstructionBytes) + } + + // Parse iteration and caps with defaults + iteration := 1 + if cfg.Iteration != "" { + if v, err := strconv.Atoi(cfg.Iteration); err == nil { + iteration = v + } + } + + botCap := 5 + if cfg.IterationCap != "" { + if v, err := strconv.Atoi(cfg.IterationCap); err == nil { + botCap = v + } + } + + humanCap := 10 + if cfg.IterationCapHuman != "" { + if v, err := strconv.Atoi(cfg.IterationCapHuman); err == nil { + humanCap = v + } + } + + cap := humanCap + if isBotUser(cfg.TriggerSource) { + cap = botCap + } + + if iteration > cap { + if isBotUser(cfg.TriggerSource) { + printer.Raw(fmt.Sprintf("::error::Fix iteration %d exceeds bot cap of %d. Escalating to human.\n", iteration, cap)) + printer.Raw(fmt.Sprintf("::error::The review→fix loop has run %d times without converging.\n", iteration)) + printer.Raw(fmt.Sprintf("::error::A human can still direct the agent with /fix (up to %d total iterations).\n", humanCap)) + } else { + printer.Raw(fmt.Sprintf("::error::Fix iteration %d exceeds human cap of %d.\n", iteration, cap)) + printer.Raw(fmt.Sprintf("::error::The /fix loop has run %d times. Further attempts are blocked.\n", iteration)) + } + return fmt.Errorf("fix iteration %d exceeds cap of %d", iteration, cap) + } + + printer.StepDone("Input validation passed") + printer.KeyValue("PR_NUMBER", cfg.PRNumber) + printer.KeyValue("REPO_FULL_NAME", cfg.RepoFullName) + printer.KeyValue("TRIGGER_SOURCE", cfg.TriggerSource) + printer.KeyValue("FIX_ITERATION", fmt.Sprintf("%d of %d", iteration, cap)) + if !isBotUser(cfg.TriggerSource) && cfg.HumanInstruction != "" { + preview := cfg.HumanInstruction + if len(preview) > 200 { + preview = preview[:200] + "..." + } + printer.KeyValue("HUMAN_INSTRUCTION", preview) + } + + return nil +} + func writeGateOutput(path, key, value string) { if path == "" { return diff --git a/internal/cli/gate_test.go b/internal/cli/gate_test.go index 78f2c5e16..71c19d8ad 100644 --- a/internal/cli/gate_test.go +++ b/internal/cli/gate_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -255,3 +256,182 @@ func TestRunGateCode_ValidationFailure(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "validation failed") } + +// --------------------------------------------------------------------------- +// gate fix tests +// --------------------------------------------------------------------------- + +func TestValidateGateFixInputs_Valid(t *testing.T) { + errs := validateGateFixInputs("42", "test-org/test-repo", "human-dev") + assert.Empty(t, errs) +} + +func TestValidateGateFixInputs_InvalidPRNumber(t *testing.T) { + errs := validateGateFixInputs("0", "test-org/test-repo", "human-dev") + assert.Len(t, errs, 1) + assert.Contains(t, errs[0], "PR_NUMBER") +} + +func TestValidateGateFixInputs_InvalidRepo(t *testing.T) { + errs := validateGateFixInputs("42", "bad repo!", "human-dev") + assert.Len(t, errs, 1) + assert.Contains(t, errs[0], "REPO_FULL_NAME") +} + +func TestValidateGateFixInputs_EmptyTriggerSource(t *testing.T) { + errs := validateGateFixInputs("42", "test-org/test-repo", "") + assert.Len(t, errs, 1) + assert.Contains(t, errs[0], "TRIGGER_SOURCE") +} + +func TestValidateGateFixInputs_MultipleErrors(t *testing.T) { + errs := validateGateFixInputs("", "", "") + assert.True(t, len(errs) >= 3, "expected at least 3 errors, got %d", len(errs)) +} + +func TestRunGateFix_Valid(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateFix_ValidationFailure(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "bad", + RepoFullName: "bad!", + TriggerSource: "", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "validation failed") +} + +func TestRunGateFix_BotIterationCapExceeded(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "review-bot[bot]", + Iteration: "6", + IterationCap: "5", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "exceeds cap") +} + +func TestRunGateFix_BotIterationAtCap(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "review-bot[bot]", + Iteration: "5", + IterationCap: "5", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateFix_HumanIterationCapExceeded(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + Iteration: "11", + IterationCapHuman: "10", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "exceeds cap") +} + +func TestRunGateFix_HumanIterationAtCap(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + Iteration: "10", + IterationCapHuman: "10", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateFix_DefaultIterationCaps(t *testing.T) { + // Bot default cap is 5 + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "review-bot[bot]", + Iteration: "6", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + + // Human default cap is 10 + cfg = gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + Iteration: "11", + } + err = runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) +} + +func TestRunGateFix_HumanInstructionTooLong(t *testing.T) { + longInstruction := strings.Repeat("x", 10001) + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + HumanInstruction: longInstruction, + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "HUMAN_INSTRUCTION") +} + +func TestRunGateFix_HumanInstructionAtLimit(t *testing.T) { + instruction := strings.Repeat("x", 10000) + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + HumanInstruction: instruction, + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateFix_BotIgnoresInstructionLength(t *testing.T) { + longInstruction := strings.Repeat("x", 20000) + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "review-bot[bot]", + HumanInstruction: longInstruction, + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestRunGateFix_DefaultIteration(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.NoError(t, err) +} + +func TestIsBotUser(t *testing.T) { + assert.True(t, isBotUser("fullsend-ai[bot]")) + assert.True(t, isBotUser("review-bot[bot]")) + assert.False(t, isBotUser("human-dev")) + assert.False(t, isBotUser("bot-like-name")) +} diff --git a/internal/forge/fake.go b/internal/forge/fake.go index cfee26ac2..5f072728e 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -588,6 +588,12 @@ func (f *FakeClient) CreateIssueComment(_ context.Context, owner, repo string, n if e := f.err("CreateIssueComment"); e != nil { return nil, e } + f.AddedComments = append(f.AddedComments, CommentRecord{ + Owner: owner, + Repo: repo, + Number: number, + Body: body, + }) f.commentCounter++ comment := IssueComment{ ID: f.commentCounter, @@ -710,21 +716,6 @@ func (f *FakeClient) ListIssueTimeline(_ context.Context, owner, repo string, nu return nil, nil } -func (f *FakeClient) AddIssueComment(_ context.Context, owner, repo string, number int, body string) error { - f.mu.Lock() - defer f.mu.Unlock() - if e := f.err("AddIssueComment"); e != nil { - return e - } - f.AddedComments = append(f.AddedComments, CommentRecord{ - Owner: owner, - Repo: repo, - Number: number, - Body: body, - }) - return nil -} - func (f *FakeClient) EnsureLabel(_ context.Context, owner, repo, name, description, color string) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 1be3ac774..e5114662b 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -167,7 +167,6 @@ type Client interface { UpdateIssueComment(ctx context.Context, owner, repo string, commentID int, body string) error MinimizeComment(ctx context.Context, nodeID, reason string) error ListIssueTimeline(ctx context.Context, owner, repo string, number int) ([]TimelineEvent, error) - AddIssueComment(ctx context.Context, owner, repo string, number int, body string) error EnsureLabel(ctx context.Context, owner, repo, name, description, color string) error AddIssueLabels(ctx context.Context, owner, repo string, number int, labels []string) error diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 420a264d3..7cafa247f 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1255,17 +1255,6 @@ func (c *LiveClient) ListIssueTimeline(ctx context.Context, owner, repo string, return result, nil } -// AddIssueComment posts a comment on an issue. -func (c *LiveClient) AddIssueComment(ctx context.Context, owner, repo string, number int, body string) error { - payload := map[string]string{"body": body} - resp, err := c.post(ctx, fmt.Sprintf("/repos/%s/%s/issues/%d/comments", owner, repo, number), payload) - if err != nil { - return fmt.Errorf("add issue comment: %w", err) - } - resp.Body.Close() - return nil -} - // EnsureLabel creates a label if it doesn't already exist. A 422 response // (label already exists) is treated as success. func (c *LiveClient) EnsureLabel(ctx context.Context, owner, repo, name, description, color string) error { diff --git a/internal/scaffold/fullsend-repo/.github/workflows/fix.yml b/internal/scaffold/fullsend-repo/.github/workflows/fix.yml index b0b0715e5..1ea15373f 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/fix.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/fix.yml @@ -261,6 +261,16 @@ jobs: fi echo "review_file=${REVIEW_FILE}" >> "${GITHUB_OUTPUT}" + - name: Install fullsend CLI + run: | + if [[ -f "${GITHUB_WORKSPACE}/bin/fullsend" ]]; then + chmod +x "${GITHUB_WORKSPACE}/bin/fullsend" + echo "${GITHUB_WORKSPACE}/bin" >> "${GITHUB_PATH}" + else + echo "::error::fullsend binary not found; run: fullsend admin install --vendor-fullsend-binary" + exit 1 + fi + - name: Validate inputs env: PR_NUMBER: ${{ steps.context.outputs.pr_number }} @@ -268,7 +278,7 @@ jobs: TRIGGER_SOURCE: ${{ inputs.trigger_source }} FIX_ITERATION: ${{ steps.context.outputs.iteration }} HUMAN_INSTRUCTION: ${{ steps.context.outputs.instruction }} - run: bash scripts/pre-fix.sh + run: fullsend gate fix - name: Pre-mask GCP credential file path run: echo "::add-mask::${GITHUB_WORKSPACE}/gha-creds-" diff --git a/internal/scaffold/fullsend-repo/harness/fix.yaml b/internal/scaffold/fullsend-repo/harness/fix.yaml index c5ea54875..251cd9a9b 100644 --- a/internal/scaffold/fullsend-repo/harness/fix.yaml +++ b/internal/scaffold/fullsend-repo/harness/fix.yaml @@ -1,7 +1,7 @@ -# harness/fix.yaml — fix agent with pre/post script pipeline. +# harness/fix.yaml — fix agent with post script pipeline. # -# Flow: pre_script → sandbox (agent) → post_script -# pre_script : validates inputs, checks iteration cap +# Flow: (workflow: fullsend gate fix) → sandbox (agent) → post_script +# gate : validates inputs, checks iteration cap (runs on runner) # agent : reads pre-fetched review body, fixes code, tests, scans, commits # post_script : push commit, post summary comment on PR # @@ -12,8 +12,6 @@ model: opus image: ghcr.io/fullsend-ai/fullsend-code:latest policy: policies/fix.yaml -pre_script: scripts/pre-fix.sh - validation_loop: script: scripts/validate-output-schema.sh max_iterations: 2 diff --git a/internal/scaffold/fullsend-repo/scripts/pre-fix.sh b/internal/scaffold/fullsend-repo/scripts/pre-fix.sh deleted file mode 100644 index 090696f55..000000000 --- a/internal/scaffold/fullsend-repo/scripts/pre-fix.sh +++ /dev/null @@ -1,101 +0,0 @@ -#!/usr/bin/env bash -# Pre-script: validate workflow_dispatch inputs before the fix agent runs. -# -# Prevents malformed or malicious event_payload from reaching the sandbox. -# Also enforces the iteration cap — blocks the run if too many fix cycles -# have already occurred on this PR. -# -# Required environment variables (set by the workflow): -# PR_NUMBER — must be a positive integer -# REPO_FULL_NAME — must be owner/repo format -# TRIGGER_SOURCE — GitHub username that triggered the fix (usernames ending in [bot] are bot triggers) -# -# Optional environment variables: -# FIX_ITERATION — current iteration count (default: 1) -# ITERATION_CAP — max bot-triggered iterations (default: 5) -# ITERATION_CAP_HUMAN — max human-triggered iterations (default: 10) -# HUMAN_INSTRUCTION — instruction text (only when TRIGGER_SOURCE doesn't end in [bot]) -set -euo pipefail - -# --------------------------------------------------------------------------- -# Helper: Bot user detection -# --------------------------------------------------------------------------- -is_bot_user() { - [[ "${1:-}" =~ \[bot\]$ ]] -} - -errors=0 - -# --------------------------------------------------------------------------- -# Input validation -# --------------------------------------------------------------------------- -if [[ ! "${PR_NUMBER:-}" =~ ^[1-9][0-9]*$ ]]; then - echo "::error::PR_NUMBER must be a positive integer, got: '${PR_NUMBER:-}'" - errors=$((errors + 1)) -fi - -if [[ ! "${REPO_FULL_NAME:-}" =~ ^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$ ]]; then - echo "::error::REPO_FULL_NAME must be owner/repo format, got: '${REPO_FULL_NAME:-}'" - errors=$((errors + 1)) -fi - -if [[ -z "${TRIGGER_SOURCE:-}" ]]; then - echo "::error::TRIGGER_SOURCE is required (GitHub username that triggered the fix)" - errors=$((errors + 1)) -fi - -if [[ "${errors}" -gt 0 ]]; then - echo "::error::Input validation failed with ${errors} error(s). Aborting." - exit 1 -fi - -# --------------------------------------------------------------------------- -# Human instruction length cap (defense against DoS via oversized input) -# --------------------------------------------------------------------------- -MAX_INSTRUCTION_BYTES=10000 -if ! is_bot_user "${TRIGGER_SOURCE}" && [[ -n "${HUMAN_INSTRUCTION:-}" ]]; then - INSTRUCTION_LEN="${#HUMAN_INSTRUCTION}" - if [[ "${INSTRUCTION_LEN}" -gt "${MAX_INSTRUCTION_BYTES}" ]]; then - echo "::error::HUMAN_INSTRUCTION is ${INSTRUCTION_LEN} bytes (max: ${MAX_INSTRUCTION_BYTES}). Truncate the instruction." - exit 1 - fi -fi - -# --------------------------------------------------------------------------- -# Iteration cap check -# --------------------------------------------------------------------------- -ITERATION="${FIX_ITERATION:-1}" -BOT_CAP="${ITERATION_CAP:-5}" -HUMAN_CAP="${ITERATION_CAP_HUMAN:-10}" - -if is_bot_user "${TRIGGER_SOURCE}"; then - CAP="${BOT_CAP}" -else - CAP="${HUMAN_CAP}" -fi - -if [[ "${ITERATION}" -gt "${CAP}" ]]; then - if is_bot_user "${TRIGGER_SOURCE}"; then - echo "::error::Fix iteration ${ITERATION} exceeds bot cap of ${CAP}. Escalating to human." - echo "::error::The review→fix loop has run ${ITERATION} times without converging." - echo "::error::A human can still direct the agent with /fix (up to ${HUMAN_CAP} total iterations)." - else - echo "::error::Fix iteration ${ITERATION} exceeds human cap of ${CAP}." - echo "::error::The /fix loop has run ${ITERATION} times. Further attempts are blocked." - fi - exit 1 -fi - -# --------------------------------------------------------------------------- -# Summary -# --------------------------------------------------------------------------- -echo "Input validation passed:" -echo " PR_NUMBER=${PR_NUMBER}" -echo " REPO_FULL_NAME=${REPO_FULL_NAME}" -echo " TRIGGER_SOURCE=${TRIGGER_SOURCE}" -echo " FIX_ITERATION=${ITERATION} of ${CAP}" -if ! is_bot_user "${TRIGGER_SOURCE}" && [[ -n "${HUMAN_INSTRUCTION:-}" ]]; then - # Truncate instruction in logs to avoid leaking long user input. - INSTR_PREVIEW="${HUMAN_INSTRUCTION:0:200}" - echo " HUMAN_INSTRUCTION=${INSTR_PREVIEW}..." -fi diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 52dc3b4e1..90147ce4a 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -136,7 +136,7 @@ func TestWalkFullsendRepo(t *testing.T) { return nil }) require.NoError(t, err) - assert.True(t, len(paths) >= 28, "expected at least 28 files, got %d", len(paths)) + assert.True(t, len(paths) >= 27, "expected at least 27 files, got %d", len(paths)) } func TestTriageWorkflowContent(t *testing.T) { @@ -230,6 +230,8 @@ func TestFixWorkflowContent(t *testing.T) { assert.Contains(t, s, "concurrency:") assert.Contains(t, s, "fullsend-fix-") assert.Contains(t, s, "cancel-in-progress: true") + // Verify gate command replaced bash script + assert.Contains(t, s, "fullsend gate fix") } func TestCodeHarnessContent(t *testing.T) { From bab3435a4ec83b85c5934b86d520d76f0ef60c13 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Sat, 2 May 2026 13:28:00 -0400 Subject: [PATCH 5/6] fix(gate): harden input validation and error handling - Tighten botLoginRe to reject leading brackets (e.g. "]evil") - Log resolveToken error instead of discarding it - Move ::notice:: emission after input validation passes - Add TRIGGER_SOURCE format validation via triggerSourceRe - Reject negative and non-numeric iteration/cap values - Extract parsePositiveIntOrDefault helper to reduce duplication - Make writeGateOutput return error with close-error handling - Add newline injection guard to writeGateOutput - Add bot instruction length cap (1 MB) for defense in depth - Add id: gate to fix.yml validate step for parity with code.yml - Add deletion assertion tests for pre-code.sh and pre-fix.sh - Add tests for negative iteration, non-numeric caps, newline rejection, leading bracket bot login, trigger source format Signed-off-by: Wayne Sun --- internal/cli/gate.go | 93 ++++++++++++------- internal/cli/gate_test.go | 88 +++++++++++++++++- .../fullsend-repo/.github/workflows/fix.yml | 1 + internal/scaffold/scaffold_test.go | 12 +++ 4 files changed, 160 insertions(+), 34 deletions(-) diff --git a/internal/cli/gate.go b/internal/cli/gate.go index 14e8a23f9..98e09f926 100644 --- a/internal/cli/gate.go +++ b/internal/cli/gate.go @@ -60,7 +60,10 @@ Optional: OutputFile: os.Getenv("GITHUB_OUTPUT"), } - token, _ := resolveToken() + token, err := resolveToken() + if err != nil { + printer.Raw(fmt.Sprintf("::warning::No GitHub token available: %v\n", err)) + } if token != "" { cfg.Client = gh.New(token) } @@ -84,7 +87,7 @@ var ( issueNumberRe = regexp.MustCompile(`^[1-9][0-9]*$`) repoFullNameRe = regexp.MustCompile(`^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$`) issueURLRe = regexp.MustCompile(`^https://github\.com/[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+/issues/[0-9]+$`) - botLoginRe = regexp.MustCompile(`^[][a-zA-Z0-9._-]+$`) + botLoginRe = regexp.MustCompile(`^[a-zA-Z0-9._-]+(\[bot\])?$`) ) func validateGateCodeInputs(issueNumber, repoFullName, issueURL string) []string { @@ -121,8 +124,6 @@ func validateGateCodeInputs(issueNumber, repoFullName, issueURL string) []string } func runGateCode(ctx context.Context, cfg gateCodeConfig, printer *ui.Printer) error { - printer.Raw(fmt.Sprintf("::notice::Code target: %s\n", cfg.IssueURL)) - errs := validateGateCodeInputs(cfg.IssueNumber, cfg.RepoFullName, cfg.IssueURL) if len(errs) > 0 { for _, e := range errs { @@ -131,6 +132,7 @@ func runGateCode(ctx context.Context, cfg gateCodeConfig, printer *ui.Printer) e return fmt.Errorf("input validation failed with %d error(s)", len(errs)) } + printer.Raw(fmt.Sprintf("::notice::Code target: %s\n", cfg.IssueURL)) printer.StepDone("Input validation passed") printer.KeyValue("ISSUE_NUMBER", cfg.IssueNumber) printer.KeyValue("REPO_FULL_NAME", cfg.RepoFullName) @@ -223,7 +225,9 @@ To override, comment `+"`/code --force`"+` on this issue. printer.Raw(fmt.Sprintf("::notice::Skipping duplicate comment — bot already posted on issue #%d\n", issueNum)) } - writeGateOutput(cfg.OutputFile, "skip", "true") + if err := writeGateOutput(cfg.OutputFile, "skip", "true"); err != nil { + printer.Raw(fmt.Sprintf("::warning::Failed to write GITHUB_OUTPUT: %v\n", err)) + } printer.StepDone(fmt.Sprintf("Skipping code agent — existing PR(s) found for issue #%d", issueNum)) return nil @@ -274,12 +278,31 @@ type gateFixConfig struct { HumanInstruction string } -const maxInstructionBytes = 10000 +const ( + maxInstructionBytes = 10000 + maxBotInstructionBytes = 1048576 // 1 MB — matches review body cap in fix.yml +) func isBotUser(username string) bool { return strings.HasSuffix(username, "[bot]") } +func parsePositiveIntOrDefault(envName, raw string, defaultVal int) (int, error) { + if raw == "" { + return defaultVal, nil + } + v, err := strconv.Atoi(raw) + if err != nil { + return 0, fmt.Errorf("%s must be a valid integer, got: '%s'", envName, raw) + } + if v < 1 { + return 0, fmt.Errorf("%s must be positive, got: %d", envName, v) + } + return v, nil +} + +var triggerSourceRe = regexp.MustCompile(`^[a-zA-Z0-9._-]+(\[bot\])?$`) + func validateGateFixInputs(prNumber, repoFullName, triggerSource string) []string { var errs []string @@ -291,6 +314,8 @@ func validateGateFixInputs(prNumber, repoFullName, triggerSource string) []strin } if triggerSource == "" { errs = append(errs, "TRIGGER_SOURCE is required (GitHub username that triggered the fix)") + } else if !triggerSourceRe.MatchString(triggerSource) { + errs = append(errs, fmt.Sprintf("TRIGGER_SOURCE must be a valid GitHub username, got: '%s'", triggerSource)) } return errs @@ -305,33 +330,33 @@ func runGateFix(_ context.Context, cfg gateFixConfig, printer *ui.Printer) error return fmt.Errorf("input validation failed with %d error(s)", len(errs)) } - // Human instruction length cap - if !isBotUser(cfg.TriggerSource) && len(cfg.HumanInstruction) > maxInstructionBytes { + // Instruction length cap — lower for humans, higher for bots. + instrCap := maxInstructionBytes + if isBotUser(cfg.TriggerSource) { + instrCap = maxBotInstructionBytes + } + if len(cfg.HumanInstruction) > instrCap { printer.Raw(fmt.Sprintf("::error::HUMAN_INSTRUCTION is %d bytes (max: %d). Truncate the instruction.\n", - len(cfg.HumanInstruction), maxInstructionBytes)) - return fmt.Errorf("HUMAN_INSTRUCTION is %d bytes (max: %d)", len(cfg.HumanInstruction), maxInstructionBytes) + len(cfg.HumanInstruction), instrCap)) + return fmt.Errorf("HUMAN_INSTRUCTION is %d bytes (max: %d)", len(cfg.HumanInstruction), instrCap) } - // Parse iteration and caps with defaults - iteration := 1 - if cfg.Iteration != "" { - if v, err := strconv.Atoi(cfg.Iteration); err == nil { - iteration = v - } + iteration, err := parsePositiveIntOrDefault("FIX_ITERATION", cfg.Iteration, 1) + if err != nil { + printer.Raw(fmt.Sprintf("::error::%v\n", err)) + return err } - botCap := 5 - if cfg.IterationCap != "" { - if v, err := strconv.Atoi(cfg.IterationCap); err == nil { - botCap = v - } + botCap, err := parsePositiveIntOrDefault("ITERATION_CAP", cfg.IterationCap, 5) + if err != nil { + printer.Raw(fmt.Sprintf("::error::%v\n", err)) + return err } - humanCap := 10 - if cfg.IterationCapHuman != "" { - if v, err := strconv.Atoi(cfg.IterationCapHuman); err == nil { - humanCap = v - } + humanCap, err := parsePositiveIntOrDefault("ITERATION_CAP_HUMAN", cfg.IterationCapHuman, 10) + if err != nil { + printer.Raw(fmt.Sprintf("::error::%v\n", err)) + return err } cap := humanCap @@ -367,14 +392,20 @@ func runGateFix(_ context.Context, cfg gateFixConfig, printer *ui.Printer) error return nil } -func writeGateOutput(path, key, value string) { +func writeGateOutput(path, key, value string) error { if path == "" { - return + return nil + } + if strings.ContainsAny(key, "\n\r") || strings.ContainsAny(value, "\n\r") { + return fmt.Errorf("GITHUB_OUTPUT key/value must not contain newlines") } f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) if err != nil { - return + return fmt.Errorf("opening GITHUB_OUTPUT: %w", err) + } + _, writeErr := fmt.Fprintf(f, "%s=%s\n", key, value) + if closeErr := f.Close(); writeErr == nil { + writeErr = closeErr } - defer f.Close() - fmt.Fprintf(f, "%s=%s\n", key, value) + return writeErr } diff --git a/internal/cli/gate_test.go b/internal/cli/gate_test.go index 71c19d8ad..93bb320f2 100644 --- a/internal/cli/gate_test.go +++ b/internal/cli/gate_test.go @@ -225,6 +225,15 @@ func TestRunGateCode_InvalidBotLogin(t *testing.T) { assert.Contains(t, err.Error(), "invalid characters") } +func TestRunGateCode_InvalidBotLoginLeadingBracket(t *testing.T) { + fc := forge.NewFakeClient() + cfg := validConfig(fc) + cfg.BotLogin = "]evil" + err := runGateCode(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid characters") +} + func TestRunGateCode_ValidCustomBotLogin(t *testing.T) { fc := forge.NewFakeClient() cfg := validConfig(fc) @@ -407,18 +416,32 @@ func TestRunGateFix_HumanInstructionAtLimit(t *testing.T) { assert.NoError(t, err) } -func TestRunGateFix_BotIgnoresInstructionLength(t *testing.T) { - longInstruction := strings.Repeat("x", 20000) +func TestRunGateFix_BotHigherInstructionCap(t *testing.T) { + // Bot cap is 1 MB, human cap is 10 KB. 20 KB passes for bots. + instruction := strings.Repeat("x", 20000) cfg := gateFixConfig{ PRNumber: "42", RepoFullName: "test-org/test-repo", TriggerSource: "review-bot[bot]", - HumanInstruction: longInstruction, + HumanInstruction: instruction, } err := runGateFix(context.Background(), cfg, newTestPrinter()) assert.NoError(t, err) } +func TestRunGateFix_BotInstructionCapExceeded(t *testing.T) { + instruction := strings.Repeat("x", 1048577) // 1 MB + 1 + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "review-bot[bot]", + HumanInstruction: instruction, + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "HUMAN_INSTRUCTION") +} + func TestRunGateFix_DefaultIteration(t *testing.T) { cfg := gateFixConfig{ PRNumber: "42", @@ -429,6 +452,65 @@ func TestRunGateFix_DefaultIteration(t *testing.T) { assert.NoError(t, err) } +func TestRunGateFix_NegativeIteration(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "review-bot[bot]", + Iteration: "-5", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "must be positive") +} + +func TestRunGateFix_NonNumericIteration(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + Iteration: "abc", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "valid integer") +} + +func TestRunGateFix_NonNumericCap(t *testing.T) { + cfg := gateFixConfig{ + PRNumber: "42", + RepoFullName: "test-org/test-repo", + TriggerSource: "human-dev", + IterationCap: "xyz", + } + err := runGateFix(context.Background(), cfg, newTestPrinter()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "ITERATION_CAP") +} + +func TestValidateGateFixInputs_InvalidTriggerSource(t *testing.T) { + errs := validateGateFixInputs("42", "test-org/test-repo", "evil$(whoami)") + assert.Len(t, errs, 1) + assert.Contains(t, errs[0], "TRIGGER_SOURCE") +} + +func TestWriteGateOutput_Error(t *testing.T) { + err := writeGateOutput("/no-such-dir/no-such-file", "skip", "true") + assert.Error(t, err) +} + +func TestWriteGateOutput_Empty(t *testing.T) { + err := writeGateOutput("", "skip", "true") + assert.NoError(t, err) +} + +func TestWriteGateOutput_RejectsNewlines(t *testing.T) { + outFile := t.TempDir() + "/github-output" + err := writeGateOutput(outFile, "skip\ninjected=pwned", "true") + assert.Error(t, err) + assert.Contains(t, err.Error(), "newlines") +} + func TestIsBotUser(t *testing.T) { assert.True(t, isBotUser("fullsend-ai[bot]")) assert.True(t, isBotUser("review-bot[bot]")) diff --git a/internal/scaffold/fullsend-repo/.github/workflows/fix.yml b/internal/scaffold/fullsend-repo/.github/workflows/fix.yml index 1ea15373f..5de9baa77 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/fix.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/fix.yml @@ -272,6 +272,7 @@ jobs: fi - name: Validate inputs + id: gate env: PR_NUMBER: ${{ steps.context.outputs.pr_number }} REPO_FULL_NAME: ${{ inputs.source_repo }} diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 90147ce4a..138f431ca 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -232,6 +232,8 @@ func TestFixWorkflowContent(t *testing.T) { assert.Contains(t, s, "cancel-in-progress: true") // Verify gate command replaced bash script assert.Contains(t, s, "fullsend gate fix") + // Verify gate step has id for downstream skip checks + assert.Contains(t, s, "id: gate") } func TestCodeHarnessContent(t *testing.T) { @@ -332,3 +334,13 @@ func TestValidateTriageDeleted(t *testing.T) { _, err := FullsendRepoFile("scripts/validate-triage.sh") assert.Error(t, err, "validate-triage.sh should have been deleted") } + +func TestPreCodeShellScriptDeleted(t *testing.T) { + _, err := FullsendRepoFile("scripts/pre-code.sh") + assert.Error(t, err, "pre-code.sh should have been deleted — replaced by fullsend gate code") +} + +func TestPreFixShellScriptDeleted(t *testing.T) { + _, err := FullsendRepoFile("scripts/pre-fix.sh") + assert.Error(t, err, "pre-fix.sh should have been deleted — replaced by fullsend gate fix") +} From 3bd14e111d3a862078a5c0740464e0bfaf618216 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Sat, 2 May 2026 17:49:43 -0400 Subject: [PATCH 6/6] fix(gate): log ListIssueComments error for consistency Add ::warning:: annotation when ListIssueComments fails during the idempotency check, matching how EnsureLabel/AddIssueLabels errors are handled. Without this, repeated API failures would silently cause duplicate comment posting. Signed-off-by: Wayne Sun --- internal/cli/gate.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/cli/gate.go b/internal/cli/gate.go index 98e09f926..0da6c2a33 100644 --- a/internal/cli/gate.go +++ b/internal/cli/gate.go @@ -206,6 +206,7 @@ To override, comment `+"`/code --force`"+` on this issue. comments, err := cfg.Client.ListIssueComments(ctx, owner, repo, issueNum) if err != nil { + printer.Raw(fmt.Sprintf("::warning::Failed to check existing comments on issue #%d: %v\n", issueNum, err)) comments = nil }