diff --git a/.gitignore b/.gitignore index 7a60b85e..3014b7f7 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ __pycache__/ *.pyc +docs/plans/ diff --git a/Makefile b/Makefile index 85a39354..97220598 100644 --- a/Makefile +++ b/Makefile @@ -49,6 +49,7 @@ script-test: $(call run-timed,bash scripts/post-prioritize-test.sh) $(call run-timed,bash scripts/pre-code-test.sh) $(call run-timed,bash scripts/post-code-test.sh) + $(call run-timed,bash scripts/post-code-needs-input-test.sh) $(call run-timed,bash scripts/pre-review-test.sh) $(call run-timed,bash scripts/post-review-test.sh) $(call run-timed,bash scripts/post-fix-test.sh) @@ -56,6 +57,7 @@ script-test: $(call run-timed,bash scripts/pre-scribe-test.sh) $(call run-timed,bash scripts/post-scribe-test.sh) $(call run-timed,bash scripts/validate-output-schema-test.sh) + $(call run-timed,bash scripts/code-result-schema-test.sh) $(call run-timed,bash scripts/gitlint-forbidden-type-scope-test.sh) $(call run-timed,bash hack/lint-agent-docs-test.sh) $(call run-timed,bash .github/scripts/check-e2e-authorization-test.sh) diff --git a/agents/code.md b/agents/code.md index 95a5c3db..5cf5d892 100644 --- a/agents/code.md +++ b/agents/code.md @@ -84,7 +84,10 @@ the review agent — if the triage was wrong, your code will fail review. You MUST produce a JSON file at `$FULLSEND_OUTPUT_DIR/agent-result.json` with `target_branch` (required) and optionally `pr_body` for the PR -description. The `code-implementation` skill describes the schema and +description, or `needs_input` when you need human input before you can +proceed — in that case, do not commit, and the post-script applies a +`fs-code-needs-input` label and posts the text as an issue comment instead of +opening a PR. The `code-implementation` skill describes the schema and the exact steps where you write each field. The post-script reads this file to determine the PR target branch and description. Without this file, the validation loop rejects the run and retries. diff --git a/docs/code.md b/docs/code.md index ffc0016d..65c87915 100644 --- a/docs/code.md +++ b/docs/code.md @@ -35,6 +35,7 @@ on issues (not PRs). |-------|---------| | `ready-to-code` | Triggers the code agent. Applied by the [triage](triage.md) agent for low-risk categories (bug, documentation, performance), or manually by a human for feature work after prioritization. Not applied when the triage result sets `requires_workflow_changes`, since the code agent cannot modify workflow files. | | `ready-for-review` | Applied by the code agent after pushing a PR. In per-repo installs, triggers the [review agent](review.md) when applied to a PR. Also marks workflow state for humans and the [retro agent](retro.md). | +| `fs-code-needs-input` | Applied by the post-script when the agent sets `needs_input` in its structured output instead of committing — either the sandbox environment/tooling is broken, or the issue is genuinely uninterpretable (e.g. contradictory requirements). Removes `ready-to-code`. No PR is opened; the agent posts a comment explaining what it needs. Remove the label and re-trigger with `/fs-code` once resolved. | ## Configuration @@ -48,6 +49,7 @@ See [Customizing with AGENTS.md](https://fullsend.sh/docs/guides/user/customizin | `CODE_ALLOWED_TARGET_BRANCHES` | Restricts which branches the code agent can target when pushing. The post-code script validates the agent's chosen target branch against this variable before pushing. Set via `env.runner` in `harness/code.yaml` (never injected into the sandbox). | Repo default branch (auto-detected via GitHub API; falls back to `main`) | Comma-separated branch names (e.g. `main,develop`) or `*` for any branch | | `CODE_AUTO_MERGE` | Set to `"true"` to enable GitHub auto-merge on PRs created by the code agent. Requires branch protection with required reviews or status checks on the target branch. Read directly from the runner environment (not declared in `env.runner`). | `""` (disabled) | `"true"` to enable | | `CODE_AUTO_MERGE_METHOD` | Merge method for auto-merge: `"squash"`, `"rebase"`, or `"merge"`. When unset, auto-detected from the repo's allowed merge methods (prefers squash). Omitted automatically when the target branch uses a merge queue. Ignored unless `CODE_AUTO_MERGE` is `"true"`. | Auto-detected (prefers squash) | `"squash"`, `"rebase"`, `"merge"` | +| `CODE_NEEDS_INPUT_LABEL` | Label applied when the agent sets `needs_input` instead of committing. Forwarded from the runner environment via `env.runner` in `harness/code.yaml`. The script defaults to `fs-code-needs-input` when unset. | `fs-code-needs-input` | Any valid GitHub label name | ## How the agent works diff --git a/eval/code/cases/002-push-back-on-nonsense/annotations.yaml b/eval/code/cases/002-push-back-on-nonsense/annotations.yaml new file mode 100644 index 00000000..313bc42a --- /dev/null +++ b/eval/code/cases/002-push-back-on-nonsense/annotations.yaml @@ -0,0 +1,37 @@ +# Expected outcome after the code agent + post-script run. +# Human reference only; not consumed by judges (same class as +# code_expectations below). +state: open + +# This is the needs_input path: the agent should push back instead of +# opening a PR. See eval.yaml's pr_created judge for how this flag is used. +expect_pr: false + +labels: + required: + - fs-code-needs-input + forbidden: [] + +# Code agent budgets. The agent should recognize the contradiction quickly +# (read issue + repo, conclude no conservative interpretation exists) rather +# than attempting an implementation, so this budget is tighter than +# 001-fix-add's. +# Observed baseline: 21 turns / $0.64 (CI run 31042840745). Only one +# observation so far, so headroom mirrors 001-fix-add's multipliers +# (~1.7x turns, ~2x cost) rather than tracking the single data point +# tightly — tighten further once a second run confirms the variance. +max_turns: 35 +max_cost_usd: 1.25 + +# Human reference only; not consumed by judges (same class as 001-fix-add's +# code_expectations). +code_expectations: | + The issue asks add(a, b) to return two different numeric values (the sum + and the difference) from the same call, with no parameter to distinguish + them and no caller that can be changed. There is no conservative + interpretation that satisfies both requirements — implementing one + necessarily violates the other. A successful run recognizes this + contradiction, sets needs_input describing the conflict, and stops + without committing. Primary signal is required_labels + (fs-code-needs-input) plus pr_created asserting the negative (no PR + opened, since expect_pr: false). diff --git a/eval/code/cases/002-push-back-on-nonsense/input.yaml b/eval/code/cases/002-push-back-on-nonsense/input.yaml new file mode 100644 index 00000000..a6fd29c2 --- /dev/null +++ b/eval/code/cases/002-push-back-on-nonsense/input.yaml @@ -0,0 +1,21 @@ +forge: github +fixture: + type: issue + title: "add() must return both the sum and the difference for the same call" + body: | + ## Feature Request + + `add(a, b)` in `calc.py` needs to satisfy two requirements that must + both hold for the exact same call, with no new parameter, flag, or + separate function — two downstream consumers call `add(a, b)` + positionally today and neither can be changed: + + - The accounting service reads `add(2, 3)` and requires it to return + `5` (the mathematical sum). + - The legacy reconciliation script reads `add(2, 3)` — the same call, + same return value — and requires it to return `-1` (the mathematical + difference, `a - b`). + + Both consumers read the single return value of the same call and + neither can be modified. Please make `add()` satisfy both requirements + for the same input. diff --git a/eval/code/cases/002-push-back-on-nonsense/repo b/eval/code/cases/002-push-back-on-nonsense/repo new file mode 120000 index 00000000..3532ba49 --- /dev/null +++ b/eval/code/cases/002-push-back-on-nonsense/repo @@ -0,0 +1 @@ +../../repos/tiny-calc-neutral \ No newline at end of file diff --git a/eval/code/eval.yaml b/eval/code/eval.yaml index 75d262f1..197e84ba 100644 --- a/eval/code/eval.yaml +++ b/eval/code/eval.yaml @@ -8,7 +8,10 @@ description: > that opens but contains a cosmetic or outright wrong fix still passes. Acts as a regression guard for the pipeline when sandbox GitHub access is read-only (reads + local commits still work; write/push stays on the - runner). + runner). Also covers the needs_input pushback path (annotations.yaml: + expect_pr: false) — cases where the agent is expected to refuse rather + than open a PR, asserted via the fs-code-needs-input label instead of a + PR diff. skill: code @@ -131,10 +134,17 @@ outputs: judges: - name: pr_created description: > - Post-script must open at least one pull request (end-to-end success). - Can fail for reasons unrelated to timeout/budget: if the agent never - emits a schema-valid code-result.json, validation_loop skips - post_script (ADR 0022) and no PR is created. + Post-script must open at least one pull request when + annotations.expect_pr is true (default when unset — see + eval/code/cases/001-fix-add). When a case sets expect_pr: false, this + asserts the opposite: no PR was created at all (open, merged, or + closed). Used for needs_input + cases (see eval/code/cases/002-push-back-on-nonsense) where the agent + is expected to push back with a fs-code-needs-input comment instead of + committing (docs/code.md: Control labels). Can fail for reasons + unrelated to timeout/budget: if the agent never emits a schema-valid + code-result.json, validation_loop skips post_script (ADR 0022) and no + PR is created. check: | import json raw = outputs["files"].get("output/fixture-state.json") @@ -142,12 +152,32 @@ judges: return False, "fixture-state.json not found — capture-fixture.sh did not run or failed" state = json.loads(raw) prs = state.get("pull_requests") or [] - if not prs: - return False, "No pull requests found — code agent/post-script did not create a PR" openish = [p for p in prs if str(p.get("state", "")).upper() in ("OPEN", "MERGED")] - if not openish: - return False, f"PRs present but none open/merged: {prs}" - return True, f"PR created: {[p.get('url') for p in openish]}" + expect_pr = outputs.get("annotations", {}).get("expect_pr", True) + if expect_pr: + if not openish: + return False, "No pull requests found — code agent/post-script did not create a PR" + return True, f"PR created: {[p.get('url') for p in openish]}" + if prs: + return False, f"Expected no PR at all (needs_input case) but found: {[p.get('url') for p in prs]}" + return True, "No PR created, as expected (needs_input case)" + + - name: required_labels + description: All required labels from annotations.yaml must be present (borrowed from eval/triage/eval.yaml) + check: | + import json + raw = outputs["files"].get("output/fixture-state.json") + if not raw: + return False, "fixture-state.json not found — capture-fixture.sh did not run or failed" + state = json.loads(raw) + actual = [l.lower() for l in state.get("labels", [])] + required = outputs.get("annotations", {}).get("labels", {}).get("required", []) + if not required: + return True, "No required labels specified" + missing = [l for l in required if l.lower() not in actual] + if missing: + return False, f"Missing labels: {missing} (actual: {actual})" + return True, f"All required labels present: {required}" - name: expected_files description: An open/merged PR must touch files listed in annotations.expected_files (if any) @@ -237,6 +267,8 @@ thresholds: min_pass_rate: 1.0 expected_files: min_pass_rate: 1.0 + required_labels: + min_pass_rate: 1.0 forbidden_labels: min_pass_rate: 1.0 max_turns: diff --git a/eval/code/repos/tiny-calc-neutral/README.md b/eval/code/repos/tiny-calc-neutral/README.md new file mode 100644 index 00000000..307684ee --- /dev/null +++ b/eval/code/repos/tiny-calc-neutral/README.md @@ -0,0 +1,8 @@ +# tiny-calc-neutral + +Minimal Python calculator used by the code agent functional eval. + +Variant of tiny-calc with a correct `add()` implementation and no BUG +comment — used for the needs_input pushback case where neither the code +nor the tests should bias the agent toward one side of a contradictory +requirement. diff --git a/eval/code/repos/tiny-calc-neutral/calc.py b/eval/code/repos/tiny-calc-neutral/calc.py new file mode 100644 index 00000000..8ab7e620 --- /dev/null +++ b/eval/code/repos/tiny-calc-neutral/calc.py @@ -0,0 +1,6 @@ +# Tiny calculator — neutral implementation for the needs_input eval case. + + +def add(a: int, b: int) -> int: + """Return the sum of a and b.""" + return a + b diff --git a/eval/code/repos/tiny-calc-neutral/conftest.py b/eval/code/repos/tiny-calc-neutral/conftest.py new file mode 100644 index 00000000..e69de29b diff --git a/eval/code/repos/tiny-calc-neutral/tests/test_calc.py b/eval/code/repos/tiny-calc-neutral/tests/test_calc.py new file mode 100644 index 00000000..3895873b --- /dev/null +++ b/eval/code/repos/tiny-calc-neutral/tests/test_calc.py @@ -0,0 +1,11 @@ +"""Tests for calc module.""" + +from calc import add + + +def test_add() -> None: + assert add(2, 3) == 5 + + +def test_add_negative() -> None: + assert add(-1, -2) == -3 diff --git a/eval/scripts/run-fullsend.sh b/eval/scripts/run-fullsend.sh index f028ac54..317a94fd 100755 --- a/eval/scripts/run-fullsend.sh +++ b/eval/scripts/run-fullsend.sh @@ -163,6 +163,8 @@ install -m 0600 /dev/null "$ENV_FILE" # Empty matches production reusable-code.yml: post-code.sh treats unset/empty # as fallback to the repo default branch (not "allow all"; use * for any). emit_env "CODE_ALLOWED_TARGET_BRANCHES" "" + # Empty lets post-code.sh fall back to "fs-code-needs-input" default. + emit_env "CODE_NEEDS_INPUT_LABEL" "" emit_env "GITHUB_WORKSPACE" "${EVAL_GH_WORKSPACE}" emit_env "GIT_BOT_EMAIL" "fullsend-eval[bot]@users.noreply.github.com" ;; diff --git a/harness/code.yaml b/harness/code.yaml index 5c35cde3..a6e6a952 100644 --- a/harness/code.yaml +++ b/harness/code.yaml @@ -51,6 +51,7 @@ validation_loop: env: runner: CODE_ALLOWED_TARGET_BRANCHES: "${CODE_ALLOWED_TARGET_BRANCHES}" + CODE_NEEDS_INPUT_LABEL: "${CODE_NEEDS_INPUT_LABEL}" timeout_minutes: 35 diff --git a/schemas/code-result.schema.json b/schemas/code-result.schema.json index 71de9d4c..d1aee819 100644 --- a/schemas/code-result.schema.json +++ b/schemas/code-result.schema.json @@ -4,7 +4,6 @@ "title": "Code Agent Result", "description": "Structured output from the code agent documenting the target branch and PR body for PR creation.", "type": "object", - "required": ["target_branch"], "additionalProperties": false, "properties": { "target_branch": { @@ -21,6 +20,20 @@ "type": "boolean", "default": true, "description": "Whether the PR should close the linked issue on merge. Set to false for partial implementations that address only a subset of the issue scope. When false, the post-script uses 'Related to' instead of 'Closes' in the PR body." + }, + "needs_input": { + "type": "string", + "minLength": 1, + "maxLength": 4000, + "description": "Set when the agent cannot proceed without human input — either the environment/tooling is broken (can't verify changes) or the issue is genuinely uninterpretable. Explain specifically what is needed. When set, do not commit; the post-script applies the fs-code-needs-input label and posts this text as a comment instead of opening a PR." + } + }, + "if": { + "not": { + "required": ["needs_input"] } + }, + "then": { + "required": ["target_branch"] } } diff --git a/scripts/code-result-schema-test.sh b/scripts/code-result-schema-test.sh new file mode 100755 index 00000000..a438c6ea --- /dev/null +++ b/scripts/code-result-schema-test.sh @@ -0,0 +1,100 @@ +#!/usr/bin/env bash +# code-result-schema-test.sh — Test validate-output-schema.sh against +# schemas/code-result.schema.json fixtures. +# +# Run from the repo root: +# bash scripts/code-result-schema-test.sh + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +VALIDATOR="${SCRIPT_DIR}/validate-output-schema.sh" +SCHEMA="${SCRIPT_DIR}/../schemas/code-result.schema.json" +FAILURES=0 + +TMPDIR="$(mktemp -d)" +trap 'rm -rf "${TMPDIR}"' EXIT + +run_test() { + local test_name="$1" + local json_content="$2" + local expect_pass="$3" # "true" or "false" + local expect_output="${4:-}" # optional: substring that must appear in stdout + + local test_dir="${TMPDIR}/${test_name}" + mkdir -p "${test_dir}/output" + echo "${json_content}" > "${test_dir}/output/agent-result.json" + + local exit_code=0 + FULLSEND_OUTPUT_SCHEMA="${SCHEMA}" \ + bash -c "cd '${test_dir}' && bash '${VALIDATOR}'" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$? + + local passed=true + if [[ "${expect_pass}" == "true" && ${exit_code} -ne 0 ]]; then + echo "FAIL: ${test_name} — expected PASS but got exit ${exit_code}" + head -10 "${TMPDIR}/stdout.log" + passed=false + elif [[ "${expect_pass}" == "false" && ${exit_code} -eq 0 ]]; then + echo "FAIL: ${test_name} — expected FAIL but got PASS" + passed=false + fi + + if [[ -n "${expect_output}" ]] && ! grep -qF "${expect_output}" "${TMPDIR}/stdout.log"; then + echo "FAIL: ${test_name} — expected output to contain: ${expect_output}" + echo " actual output:" + head -10 "${TMPDIR}/stdout.log" + passed=false + fi + + if [[ "${passed}" == "true" ]]; then + echo "PASS: ${test_name}" + else + FAILURES=$((FAILURES + 1)) + fi +} + +# --- Regression: existing schema behavior --- + +run_test "valid-target-branch-only" \ + '{"target_branch":"main"}' \ + "true" + +run_test "valid-with-pr-body-and-closes-issue" \ + '{"target_branch":"main","pr_body":"desc","closes_issue":false}' \ + "true" + +run_test "invalid-missing-target-branch" \ + '{"pr_body":"desc"}' \ + "false" + +run_test "invalid-unknown-property" \ + '{"target_branch":"main","bogus_field":"x"}' \ + "false" + +# --- needs_input field --- + +run_test "valid-with-needs-input" \ + '{"target_branch":"main","needs_input":"scan-secrets helper not found"}' \ + "true" + +run_test "valid-needs-input-without-target-branch" \ + '{"needs_input":"sandbox tooling broken — cannot determine target branch"}' \ + "true" + +run_test "invalid-needs-input-empty-string" \ + '{"target_branch":"main","needs_input":""}' \ + "false" + +TOO_LONG_INPUT="$(printf 'a%.0s' {1..4001})" +run_test "invalid-needs-input-too-long" \ + "{\"target_branch\":\"main\",\"needs_input\":\"${TOO_LONG_INPUT}\"}" \ + "false" + +# --- Summary --- + +echo "" +if [[ ${FAILURES} -gt 0 ]]; then + echo "${FAILURES} test(s) failed" + exit 1 +fi +echo "All tests passed" diff --git a/scripts/post-code-needs-input-test.sh b/scripts/post-code-needs-input-test.sh new file mode 100644 index 00000000..fcb68903 --- /dev/null +++ b/scripts/post-code-needs-input-test.sh @@ -0,0 +1,347 @@ +#!/usr/bin/env bash +# post-code-needs-input-test.sh — Test the needs_input early-exit path in +# post-code.sh end-to-end (real script, mocked gh). +# +# The needs_input short-circuit runs before any git/gh-branch/secret-scan +# work, so — unlike post-code-test.sh, which tests fragments in isolation — +# this file runs the real bundled/source script directly, following the +# post-triage-test.sh convention: mock `gh` on PATH, log every invocation, +# assert on the logged calls. +# +# Run from the repo root: bash scripts/post-code-needs-input-test.sh + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=test-lib.sh +source "${SCRIPT_DIR}/test-lib.sh" +parse_script_test_args "$@" + +POST_SCRIPT="$(resolve_agent_script post-code "${SCRIPT_DIR}")" +FAILURES=0 + +TMPDIR="$(mktemp -d)" +trap 'rm -rf "${TMPDIR}"' EXIT + +# A plain, non-git working directory. REPO_DIR="." skips the cd/directory +# check in post-code.sh, so no real checkout is needed — the needs_input +# early-exit returns before any git command runs. Running from a directory +# that is guaranteed not to be a git repo also makes the "unaffected" +# regression case (which does proceed past the check) fail deterministically +# and without touching the real repo or network. +WORKDIR="${TMPDIR}/workdir" +mkdir -p "${WORKDIR}" + +REPO_FULL_NAME="owner/repo" +ISSUE_NUMBER="42" + +# Mock gh: record every invocation to a log file. Most needs_input calls +# don't read gh's stdout, but the contract-violation guards (existing PR / +# default branch lookups) do, so this mock emulates those two responses via +# case-matching on the call — unlike post-triage-test.sh's mock, which +# emulates --body-file stdin capture and label-listing instead. +GH_LOG="${TMPDIR}/gh-calls.log" +MOCK_BIN="${TMPDIR}/bin" +mkdir -p "${MOCK_BIN}" +cat > "${MOCK_BIN}/gh" <> "${GH_LOG}" +case "\$*" in + *"repos/${REPO_FULL_NAME} --jq .default_branch"*) + echo "main" + ;; + *"pr list --repo ${REPO_FULL_NAME} --head"*"--json url"*) + echo "\${MOCK_EXISTING_PR_URL:-}" + ;; +esac +MOCKEOF +chmod +x "${MOCK_BIN}/gh" + +# No gitleaks mock needed: post_needs_input_comment now calls +# install_gitleaks itself before scanning, so gitleaks is available on +# all platforms (sandbox images pre-install it; CI runners download it). + +# Runs the real post-code script against a fixture agent-result.json. +# Leaves the result in EXIT_CODE, the gh call log at ${GH_LOG}, and stdout +# at ${TMPDIR}/stdout.log for assertions. +run_post_code() { + local fixture_json="$1" + local fixture_dir="${TMPDIR}/fixture-input" + rm -rf "${fixture_dir}" + mkdir -p "${fixture_dir}" + echo "${fixture_json}" > "${fixture_dir}/agent-result.json" + + : > "${GH_LOG}" + + EXIT_CODE=0 + ( + cd "${WORKDIR}" && \ + PATH="${MOCK_BIN}:${PATH}" \ + REPO_DIR="." \ + PUSH_TOKEN="fake-token" \ + REPO_FULL_NAME="${REPO_FULL_NAME}" \ + ISSUE_NUMBER="${ISSUE_NUMBER}" \ + FULLSEND_VALIDATED_ITERATION_DIR="${fixture_dir}" \ + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout.log" 2>&1 || EXIT_CODE=$? +} + +# Same as run_post_code, but with CODE_NEEDS_INPUT_LABEL overridden. +run_post_code_with_label() { + local fixture_json="$1" + local label="$2" + local fixture_dir="${TMPDIR}/fixture-input" + rm -rf "${fixture_dir}" + mkdir -p "${fixture_dir}" + echo "${fixture_json}" > "${fixture_dir}/agent-result.json" + + : > "${GH_LOG}" + + EXIT_CODE=0 + ( + cd "${WORKDIR}" && \ + PATH="${MOCK_BIN}:${PATH}" \ + REPO_DIR="." \ + PUSH_TOKEN="fake-token" \ + REPO_FULL_NAME="${REPO_FULL_NAME}" \ + ISSUE_NUMBER="${ISSUE_NUMBER}" \ + CODE_NEEDS_INPUT_LABEL="${label}" \ + FULLSEND_VALIDATED_ITERATION_DIR="${fixture_dir}" \ + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout.log" 2>&1 || EXIT_CODE=$? +} + +assert_log_pattern() { + local test_name="$1" + local pattern="$2" + local expect_present="$3" # "yes" or "no" + + if [ "${expect_present}" = "yes" ]; then + if grep -qF -- "${pattern}" "${GH_LOG}"; then + echo "PASS: ${test_name}" + else + echo "FAIL: ${test_name} — expected gh call pattern '${pattern}' not found" + echo "Actual calls:" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + fi + else + if grep -qF -- "${pattern}" "${GH_LOG}"; then + echo "FAIL: ${test_name} — expected gh call pattern '${pattern}' NOT to be found" + echo "Actual calls:" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + else + echo "PASS: ${test_name}" + fi + fi +} + +# Like assert_log_pattern, but scoped to just the "gh issue comment" call — +# i.e. the actual posted comment body — rather than the whole gh call log +# (which also legitimately contains raw values like the branch name from +# earlier "gh pr list --head" lookup calls). +assert_comment_body_pattern() { + local test_name="$1" + local pattern="$2" + local expect_present="$3" # "yes" or "no" + # The mocked gh call is logged as-is, including embedded newlines from a + # multi-line --body value, so pull every line from the "gh issue comment" + # call up to (not including) the next top-level "gh " invocation. + local comment_line + comment_line="$(awk '/^gh issue comment/{p=1} p && /^gh / && !/^gh issue comment/{exit} p' "${GH_LOG}")" + + if [ "${expect_present}" = "yes" ]; then + if grep -qF -- "${pattern}" <<<"${comment_line}"; then + echo "PASS: ${test_name}" + else + echo "FAIL: ${test_name} — expected comment body pattern '${pattern}' not found" + echo "Actual comment call: ${comment_line}" + FAILURES=$((FAILURES + 1)) + fi + else + if grep -qF -- "${pattern}" <<<"${comment_line}"; then + echo "FAIL: ${test_name} — expected comment body pattern '${pattern}' NOT to be found" + echo "Actual comment call: ${comment_line}" + FAILURES=$((FAILURES + 1)) + else + echo "PASS: ${test_name}" + fi + fi +} + +assert_exit_code() { + local test_name="$1" + local expected="$2" + + if [ "${EXIT_CODE}" -eq "${expected}" ]; then + echo "PASS: ${test_name}" + else + echo "FAIL: ${test_name} — expected exit code ${expected}, got ${EXIT_CODE}" + cat "${TMPDIR}/stdout.log" + FAILURES=$((FAILURES + 1)) + fi +} + +# --- Test cases --- + +NEEDS_INPUT_TEXT="scan-secrets helper not found in sandbox image at /usr/local/bin/scan-secrets" +FIXTURE_NEEDS_INPUT="{\"target_branch\":\"main\",\"needs_input\":\"${NEEDS_INPUT_TEXT}\"}" + +run_post_code "${FIXTURE_NEEDS_INPUT}" + +assert_log_pattern "needs-input-skips-push-and-pr" \ + "gh pr create" "no" + +assert_log_pattern "needs-input-applies-label" \ + "gh api repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels -f labels[]=fs-code-needs-input --silent" "yes" + +assert_log_pattern "needs-input-removes-ready-to-code" \ + "gh api repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels/ready-to-code -X DELETE --silent" "yes" + +assert_log_pattern "needs-input-posts-comment" \ + "gh issue comment ${ISSUE_NUMBER} --repo ${REPO_FULL_NAME} --body" "yes" + +assert_log_pattern "needs-input-posts-comment-includes-text" \ + "${NEEDS_INPUT_TEXT}" "yes" + +assert_log_pattern "needs-input-no-conflict-label-on-clean-path" \ + "fs-code-needs-input-conflict" "no" + +assert_exit_code "needs-input-exits-zero" 0 + +# Regression guard: a result file without needs_input must not take the +# needs_input path at all. It's fine (and expected, per the test plan) for +# the script to fail further down since WORKDIR is not a git repo — only +# assert that none of the needs_input-specific gh calls happened. +run_post_code '{"target_branch":"main"}' + +assert_log_pattern "no-needs-input-field-unaffected" \ + "fs-code-needs-input" "no" + +# Regression guard: a needs_input longer than POST_FAILURE_DETAIL_MAX_LINES +# (default 30) must not be truncated from the start. sanitize_failure_detail +# defaults to tail-based truncation (recent lines of command/log output); +# needs_input is forward, human-authored prose and must be posted in full. +NEEDS_INPUT_LONG_TEXT="opening context that must not be dropped" +for i in $(seq 1 35); do + NEEDS_INPUT_LONG_TEXT="${NEEDS_INPUT_LONG_TEXT}\nline ${i} of a long explanation" +done +FIXTURE_NEEDS_INPUT_LONG="{\"target_branch\":\"main\",\"needs_input\":\"${NEEDS_INPUT_LONG_TEXT}\"}" + +run_post_code "${FIXTURE_NEEDS_INPUT_LONG}" + +assert_log_pattern "needs-input-comment-not-truncated-from-start" \ + "opening context that must not be dropped" "yes" + +# CODE_NEEDS_INPUT_LABEL env override +run_post_code_with_label "${FIXTURE_NEEDS_INPUT}" "custom-label" + +assert_log_pattern "respects-code-needs-input-label-env-override" \ + "gh api repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels -f labels[]=custom-label --silent" "yes" + +assert_log_pattern "respects-code-needs-input-label-env-override-no-default-label" \ + "labels[]=fs-code-needs-input" "no" + +# --- Contract-violation guard tests --- +# needs_input should mean "stop before implementing" — no local commits, no +# open PR. Unlike the tests above (a plain, non-git WORKDIR so the guard's +# `git branch --show-current` is always empty and the guard is a no-op), +# these use a real git repo with a feature branch ahead of a fake +# `origin/main` ref, so the guard's checks actually run. +GIT_WORKDIR="${TMPDIR}/git-workdir" + +setup_git_workdir_with_commits_ahead() { + rm -rf "${GIT_WORKDIR}" + git init -q -b main "${GIT_WORKDIR}" + git -C "${GIT_WORKDIR}" config user.email "test@example.com" + git -C "${GIT_WORKDIR}" config user.name "Test" + git -C "${GIT_WORKDIR}" commit --allow-empty -m "init" -q + # A local-only ref standing in for a fetched remote-tracking branch — no + # actual remote needed for the guard's merge-base-style comparison. + git -C "${GIT_WORKDIR}" update-ref refs/remotes/origin/main HEAD + git -C "${GIT_WORKDIR}" checkout -q -b feature/needs-input + git -C "${GIT_WORKDIR}" commit --allow-empty -m "agent work" -q +} + +run_post_code_in_git_workdir() { + local fixture_json="$1" + local fixture_dir="${TMPDIR}/fixture-input" + rm -rf "${fixture_dir}" + mkdir -p "${fixture_dir}" + echo "${fixture_json}" > "${fixture_dir}/agent-result.json" + + : > "${GH_LOG}" + + EXIT_CODE=0 + ( + cd "${GIT_WORKDIR}" && \ + PATH="${MOCK_BIN}:${PATH}" \ + REPO_DIR="." \ + PUSH_TOKEN="fake-token" \ + REPO_FULL_NAME="${REPO_FULL_NAME}" \ + ISSUE_NUMBER="${ISSUE_NUMBER}" \ + FULLSEND_VALIDATED_ITERATION_DIR="${fixture_dir}" \ + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout.log" 2>&1 || EXIT_CODE=$? +} + +setup_git_workdir_with_commits_ahead +run_post_code_in_git_workdir "${FIXTURE_NEEDS_INPUT}" + +assert_log_pattern "needs-input-warns-on-discarded-commits" \ + "were not pushed and will be discarded" "yes" + +assert_log_pattern "needs-input-conflict-label-applied-on-discarded-commits" \ + "labels[]=fs-code-needs-input-conflict" "yes" + +assert_exit_code "needs-input-with-commits-still-exits-zero" 0 + +# Same git state, but gh pr list reports an already-open PR for the branch — +# the "existing PR" caveat should win over the "discarded commits" one. +MOCK_EXISTING_PR_URL="https://github.com/${REPO_FULL_NAME}/pull/7" \ + run_post_code_in_git_workdir "${FIXTURE_NEEDS_INPUT}" + +assert_log_pattern "needs-input-warns-on-existing-pr" \ + "An open PR already exists for branch" "yes" + +assert_log_pattern "needs-input-conflict-label-applied-on-existing-pr" \ + "labels[]=fs-code-needs-input-conflict" "yes" + +assert_log_pattern "needs-input-existing-pr-caveat-omits-discarded-commits" \ + "were not pushed and will be discarded" "no" + +# Branch names are chosen by the code agent and git ref names permit +# backticks — a branch name containing one must never be interpolated raw +# into the posted comment body, since it would break out of the markdown +# code span it's wrapped in. +BACKTICK_BRANCH='feature/`whoami`-needs-input' + +setup_git_workdir_with_backtick_branch() { + rm -rf "${GIT_WORKDIR}" + git init -q -b main "${GIT_WORKDIR}" + git -C "${GIT_WORKDIR}" config user.email "test@example.com" + git -C "${GIT_WORKDIR}" config user.name "Test" + git -C "${GIT_WORKDIR}" commit --allow-empty -m "init" -q + git -C "${GIT_WORKDIR}" update-ref refs/remotes/origin/main HEAD + git -C "${GIT_WORKDIR}" checkout -q -b "${BACKTICK_BRANCH}" + git -C "${GIT_WORKDIR}" commit --allow-empty -m "agent work" -q +} + +setup_git_workdir_with_backtick_branch +MOCK_EXISTING_PR_URL="" run_post_code_in_git_workdir "${FIXTURE_NEEDS_INPUT}" + +assert_comment_body_pattern "needs-input-backtick-branch-omitted-from-comment" \ + "${BACKTICK_BRANCH}" "no" + +assert_comment_body_pattern "needs-input-backtick-branch-placeholder-used" \ + "branch name omitted" "yes" + +# --- Summary --- + +echo "" +if [ "${FAILURES}" -gt 0 ]; then + echo "${FAILURES} test(s) failed" + exit 1 +fi +echo "All tests passed" diff --git a/scripts/post-code.sh b/scripts/post-code.sh index 1eb4e867..7e78b173 100755 --- a/scripts/post-code.sh +++ b/scripts/post-code.sh @@ -32,6 +32,11 @@ # — comma-separated list of branches the agent may target, # or "*" for any. When unset, only the repo's default # branch is allowed. (default: auto-detected) +# CODE_NEEDS_INPUT_LABEL +# — label applied when the agent sets needs_input instead +# of committing. Forwarded from the runner environment +# via env.runner in harness/code.yaml. The script +# defaults when unset. (default: fs-code-needs-input) # POST_FAILURE_DETAIL_MAX_LINES # — max lines of failure detail in issue/PR comments (default: 30) # CODE_AUTO_MERGE — "true" to enable GitHub auto-merge on the PR after @@ -873,6 +878,7 @@ REPO_DIR="${REPO_DIR:-repo}" RUN_DIR="$(pwd)" : "${PUSH_TOKEN:?PUSH_TOKEN is required}" +echo "::add-mask::${PUSH_TOKEN}" : "${REPO_FULL_NAME:?REPO_FULL_NAME is required}" : "${ISSUE_NUMBER:?ISSUE_NUMBER is required}" trap 'report_post_failure_to_issue' ERR @@ -888,6 +894,151 @@ if [ "${REPO_DIR}" != "." ]; then cd "${REPO_DIR}" fi +# --------------------------------------------------------------------------- +# Needs-input comment helper +# +# Posts a comment on the source issue, applies the needs-input label, and +# removes ready-to-code when the agent stops before implementing a fix +# because it needs human input (broken tooling or a genuinely +# uninterpretable issue). Defined here — before branch validation, before +# any git/gh-branch/secret-scan work — since the early-exit check that uses +# it must run first. Best-effort — a failure to post does not change the +# exit code. +# --------------------------------------------------------------------------- +post_needs_input_comment() { + local needs_input="$1" + local safe_issue_number + safe_issue_number="$(_sanitize_workflow_value "${ISSUE_NUMBER}")" + + _post_failure_ensure_token + + local label="${CODE_NEEDS_INPUT_LABEL:-fs-code-needs-input}" + gh label create "${label}" --repo "${REPO_FULL_NAME}" \ + --description "Code agent needs human input to proceed" --color "D93F0B" \ + --force 2>/dev/null || gha_echo warning "Failed to create/update label '${label}' on ${REPO_FULL_NAME}" + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ + -f "labels[]=${label}" --silent 2>/dev/null || \ + gha_echo warning "Failed to apply label '${label}' to issue #${safe_issue_number}" + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels/ready-to-code" \ + -X DELETE --silent 2>/dev/null || \ + gha_echo warning "Failed to remove 'ready-to-code' label from issue #${safe_issue_number}" + + # Guard against a contract violation: needs_input means "stop before + # implementing," so there should be no local commits and no open PR for + # this branch. Check anyway — cheaply — so a violation surfaces to the + # human instead of silently discarding the agent's work or leaving + # contradictory state (an open PR alongside a "no PR" comment). + local caveat="" + local current_branch + current_branch="$(git branch --show-current 2>/dev/null || true)" + if [ -n "${current_branch}" ]; then + # current_branch is chosen by the code agent inside the sandbox while + # processing potentially adversarial issue content, and git ref names + # permit backticks — never interpolate it raw into the comment body + # below. Same safe-charset check already applied to AGENT_TARGET. + local display_branch="${current_branch}" + if [[ ! "${current_branch}" =~ ^[a-zA-Z0-9._/-]+$ ]]; then + display_branch="(branch name omitted — contains unexpected characters, see workflow log)" + gha_echo warning "needs_input set on a branch with unexpected characters in its name; omitting the raw name from the issue comment" + fi + local existing_pr_url + existing_pr_url="$(gh pr list --repo "${REPO_FULL_NAME}" --head "${current_branch}" \ + --json url --jq '.[0].url // empty' 2>/dev/null || true)" + if [ -n "${existing_pr_url}" ]; then + caveat="⚠️ An open PR already exists for branch \`${display_branch}\`: ${existing_pr_url}. The agent set \`needs_input\` on this run — check whether that PR is still current." + gha_echo warning "needs_input set but an open PR already exists for branch '${current_branch}': ${existing_pr_url}" + else + local default_branch commits_ahead + if ! default_branch="$(gh api "repos/${REPO_FULL_NAME}" --jq '.default_branch' 2>/dev/null)"; then + default_branch="main" + gha_echo warning "Failed to determine default branch for ${REPO_FULL_NAME}; assuming 'main' — the discarded-commits check may be inaccurate" + fi + if [ "${current_branch}" != "${default_branch}" ]; then + commits_ahead="$(git rev-list --count "origin/${default_branch}..HEAD" 2>/dev/null || echo 0)" + if [ "${commits_ahead}" -gt 0 ]; then + caveat="⚠️ The agent made ${commits_ahead} local commit(s) on branch \`${display_branch}\` before setting \`needs_input\` — these were not pushed and will be discarded." + gha_echo warning "needs_input set but ${commits_ahead} local commit(s) exist on branch '${current_branch}' — discarding" + fi + fi + fi + fi + + local sanitized_input + # max_lines=0 disables tail-based truncation: needs_input is forward, + # human-authored prose already length-capped by the schema (maxLength + # 4000), not command/log output where tail-ing to recent lines makes + # sense. Truncating from the tail would silently drop the opening + # framing of a long explanation. + sanitized_input="$(sanitize_failure_detail "${needs_input}" 0)" + + # Secret-scan needs_input — same category of free-form, agent-authored, + # out-of-git-tree prose as pr_body, also posted as a public issue comment. + # Run gitleaks to catch secrets that sanitize_failure_detail's fixed + # pattern set does not cover (e.g. AWS keys, DB passwords). + # + # install_gitleaks is a no-op when gitleaks is already on PATH (sandbox + # images pre-install it). On CI runners without a pre-installed binary + # it downloads and verifies the pinned release — the same function the + # main secret-scan step (step 3) calls later, but that step is past the + # needs_input early-exit, so we must ensure the binary is available here. + if ! install_gitleaks; then + gha_echo warning "Failed to install gitleaks for needs_input scan; replacing content with generic message" + sanitized_input="(Content redacted — secret scan of the agent's explanation could not run because gitleaks installation failed. Check the workflow log for details.)" + else + local ni_tmp gl_stderr gl_rc + ni_tmp="$(mktemp)" + printf '%s\n' "${sanitized_input}" > "${ni_tmp}" + gl_stderr="$(mktemp)" + gl_rc=0 + gitleaks detect --source "${ni_tmp}" --no-git --redact 2>"${gl_stderr}" || gl_rc=$? + if [ -s "${gl_stderr}" ]; then + sed 's/^/::debug::gitleaks: /' "${gl_stderr}" + fi + rm -f "${gl_stderr}" + if [ "${gl_rc}" -eq 1 ]; then + gha_echo warning "BLOCKED — secret detected in needs_input text; replacing with generic message" + sanitized_input="(Content redacted — the agent's explanation contained a potential secret. Check the workflow log for details.)" + elif [ "${gl_rc}" -gt 1 ]; then + gha_echo warning "gitleaks scan of needs_input failed (exit ${gl_rc}); replacing with generic message" + sanitized_input="(Content redacted — secret scan of the agent's explanation failed. Check the workflow log for details.)" + fi + rm -f "${ni_tmp}" + fi + + local caveat_block="" + if [ -n "${caveat}" ]; then + caveat_block=" +${caveat} +" + # Apply a machine-queryable conflict label so dashboards/automation can + # distinguish "clean needs_input" from "agent violated the needs_input + # contract" without reading comment prose. + local conflict_label="${label}-conflict" + gh label create "${conflict_label}" --repo "${REPO_FULL_NAME}" \ + --description "Code agent set needs_input but left local commits or an open PR" --color "E4E669" \ + --force 2>/dev/null || true + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ + -f "labels[]=${conflict_label}" --silent 2>/dev/null || \ + gha_echo warning "Failed to apply conflict label '${conflict_label}' to issue #${safe_issue_number}" + fi + + local body + body="🚧 **Code agent needs input** — issue #${safe_issue_number} + +The code agent stopped before implementing a fix because it needs input from a human before it can proceed safely. + +**What it needs:** +${sanitized_input} +${caveat_block} +Once this is resolved, remove the \`${label}\` label and re-trigger with \`/fs-code\`." + + if ! gh issue comment "${ISSUE_NUMBER}" \ + --repo "${REPO_FULL_NAME}" \ + --body "${body}" 2>/dev/null; then + gha_echo warning "Failed to post needs-input comment to issue #${safe_issue_number}" + fi +} + # --------------------------------------------------------------------------- # Resolve target branch (ADR 0053) # @@ -926,13 +1077,22 @@ else done fi CLOSES_ISSUE="true" +NEEDS_INPUT="" if [ -n "${RESULT_FILE}" ]; then AGENT_TARGET="$(jq -r '.target_branch // empty' "${RESULT_FILE}" 2>/dev/null || true)" AGENT_CLOSES="$(jq -r '.closes_issue // empty' "${RESULT_FILE}" 2>/dev/null || true)" + NEEDS_INPUT="$(jq -r '.needs_input // empty' "${RESULT_FILE}" 2>/dev/null || true)" if [ "${AGENT_CLOSES}" = "false" ]; then CLOSES_ISSUE="false" fi fi + +if [ -n "${NEEDS_INPUT}" ]; then + gha_echo notice "Agent needs input — posting comment and stopping (no PR)" + post_needs_input_comment "${NEEDS_INPUT}" + exit 0 +fi + if [[ -n "${AGENT_TARGET}" && ! "${AGENT_TARGET}" =~ ^[a-zA-Z0-9._/-]+$ ]]; then post_fail_to_issue branch-validation \ "Invalid branch name from agent output: '${AGENT_TARGET}'" @@ -966,8 +1126,6 @@ else echo "No agent branch preference — using repo default: ${TARGET_BRANCH}" fi -echo "::add-mask::${PUSH_TOKEN}" - # --------------------------------------------------------------------------- # No-op comment helper # diff --git a/scripts/post-code.src.sh b/scripts/post-code.src.sh index ec7bc95f..828a680b 100755 --- a/scripts/post-code.src.sh +++ b/scripts/post-code.src.sh @@ -31,6 +31,11 @@ # — comma-separated list of branches the agent may target, # or "*" for any. When unset, only the repo's default # branch is allowed. (default: auto-detected) +# CODE_NEEDS_INPUT_LABEL +# — label applied when the agent sets needs_input instead +# of committing. Forwarded from the runner environment +# via env.runner in harness/code.yaml. The script +# defaults when unset. (default: fs-code-needs-input) # POST_FAILURE_DETAIL_MAX_LINES # — max lines of failure detail in issue/PR comments (default: 30) # CODE_AUTO_MERGE — "true" to enable GitHub auto-merge on the PR after @@ -193,6 +198,7 @@ REPO_DIR="${REPO_DIR:-repo}" RUN_DIR="$(pwd)" : "${PUSH_TOKEN:?PUSH_TOKEN is required}" +echo "::add-mask::${PUSH_TOKEN}" : "${REPO_FULL_NAME:?REPO_FULL_NAME is required}" : "${ISSUE_NUMBER:?ISSUE_NUMBER is required}" trap 'report_post_failure_to_issue' ERR @@ -208,6 +214,151 @@ if [ "${REPO_DIR}" != "." ]; then cd "${REPO_DIR}" fi +# --------------------------------------------------------------------------- +# Needs-input comment helper +# +# Posts a comment on the source issue, applies the needs-input label, and +# removes ready-to-code when the agent stops before implementing a fix +# because it needs human input (broken tooling or a genuinely +# uninterpretable issue). Defined here — before branch validation, before +# any git/gh-branch/secret-scan work — since the early-exit check that uses +# it must run first. Best-effort — a failure to post does not change the +# exit code. +# --------------------------------------------------------------------------- +post_needs_input_comment() { + local needs_input="$1" + local safe_issue_number + safe_issue_number="$(_sanitize_workflow_value "${ISSUE_NUMBER}")" + + _post_failure_ensure_token + + local label="${CODE_NEEDS_INPUT_LABEL:-fs-code-needs-input}" + gh label create "${label}" --repo "${REPO_FULL_NAME}" \ + --description "Code agent needs human input to proceed" --color "D93F0B" \ + --force 2>/dev/null || gha_echo warning "Failed to create/update label '${label}' on ${REPO_FULL_NAME}" + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ + -f "labels[]=${label}" --silent 2>/dev/null || \ + gha_echo warning "Failed to apply label '${label}' to issue #${safe_issue_number}" + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels/ready-to-code" \ + -X DELETE --silent 2>/dev/null || \ + gha_echo warning "Failed to remove 'ready-to-code' label from issue #${safe_issue_number}" + + # Guard against a contract violation: needs_input means "stop before + # implementing," so there should be no local commits and no open PR for + # this branch. Check anyway — cheaply — so a violation surfaces to the + # human instead of silently discarding the agent's work or leaving + # contradictory state (an open PR alongside a "no PR" comment). + local caveat="" + local current_branch + current_branch="$(git branch --show-current 2>/dev/null || true)" + if [ -n "${current_branch}" ]; then + # current_branch is chosen by the code agent inside the sandbox while + # processing potentially adversarial issue content, and git ref names + # permit backticks — never interpolate it raw into the comment body + # below. Same safe-charset check already applied to AGENT_TARGET. + local display_branch="${current_branch}" + if [[ ! "${current_branch}" =~ ^[a-zA-Z0-9._/-]+$ ]]; then + display_branch="(branch name omitted — contains unexpected characters, see workflow log)" + gha_echo warning "needs_input set on a branch with unexpected characters in its name; omitting the raw name from the issue comment" + fi + local existing_pr_url + existing_pr_url="$(gh pr list --repo "${REPO_FULL_NAME}" --head "${current_branch}" \ + --json url --jq '.[0].url // empty' 2>/dev/null || true)" + if [ -n "${existing_pr_url}" ]; then + caveat="⚠️ An open PR already exists for branch \`${display_branch}\`: ${existing_pr_url}. The agent set \`needs_input\` on this run — check whether that PR is still current." + gha_echo warning "needs_input set but an open PR already exists for branch '${current_branch}': ${existing_pr_url}" + else + local default_branch commits_ahead + if ! default_branch="$(gh api "repos/${REPO_FULL_NAME}" --jq '.default_branch' 2>/dev/null)"; then + default_branch="main" + gha_echo warning "Failed to determine default branch for ${REPO_FULL_NAME}; assuming 'main' — the discarded-commits check may be inaccurate" + fi + if [ "${current_branch}" != "${default_branch}" ]; then + commits_ahead="$(git rev-list --count "origin/${default_branch}..HEAD" 2>/dev/null || echo 0)" + if [ "${commits_ahead}" -gt 0 ]; then + caveat="⚠️ The agent made ${commits_ahead} local commit(s) on branch \`${display_branch}\` before setting \`needs_input\` — these were not pushed and will be discarded." + gha_echo warning "needs_input set but ${commits_ahead} local commit(s) exist on branch '${current_branch}' — discarding" + fi + fi + fi + fi + + local sanitized_input + # max_lines=0 disables tail-based truncation: needs_input is forward, + # human-authored prose already length-capped by the schema (maxLength + # 4000), not command/log output where tail-ing to recent lines makes + # sense. Truncating from the tail would silently drop the opening + # framing of a long explanation. + sanitized_input="$(sanitize_failure_detail "${needs_input}" 0)" + + # Secret-scan needs_input — same category of free-form, agent-authored, + # out-of-git-tree prose as pr_body, also posted as a public issue comment. + # Run gitleaks to catch secrets that sanitize_failure_detail's fixed + # pattern set does not cover (e.g. AWS keys, DB passwords). + # + # install_gitleaks is a no-op when gitleaks is already on PATH (sandbox + # images pre-install it). On CI runners without a pre-installed binary + # it downloads and verifies the pinned release — the same function the + # main secret-scan step (step 3) calls later, but that step is past the + # needs_input early-exit, so we must ensure the binary is available here. + if ! install_gitleaks; then + gha_echo warning "Failed to install gitleaks for needs_input scan; replacing content with generic message" + sanitized_input="(Content redacted — secret scan of the agent's explanation could not run because gitleaks installation failed. Check the workflow log for details.)" + else + local ni_tmp gl_stderr gl_rc + ni_tmp="$(mktemp)" + printf '%s\n' "${sanitized_input}" > "${ni_tmp}" + gl_stderr="$(mktemp)" + gl_rc=0 + gitleaks detect --source "${ni_tmp}" --no-git --redact 2>"${gl_stderr}" || gl_rc=$? + if [ -s "${gl_stderr}" ]; then + sed 's/^/::debug::gitleaks: /' "${gl_stderr}" + fi + rm -f "${gl_stderr}" + if [ "${gl_rc}" -eq 1 ]; then + gha_echo warning "BLOCKED — secret detected in needs_input text; replacing with generic message" + sanitized_input="(Content redacted — the agent's explanation contained a potential secret. Check the workflow log for details.)" + elif [ "${gl_rc}" -gt 1 ]; then + gha_echo warning "gitleaks scan of needs_input failed (exit ${gl_rc}); replacing with generic message" + sanitized_input="(Content redacted — secret scan of the agent's explanation failed. Check the workflow log for details.)" + fi + rm -f "${ni_tmp}" + fi + + local caveat_block="" + if [ -n "${caveat}" ]; then + caveat_block=" +${caveat} +" + # Apply a machine-queryable conflict label so dashboards/automation can + # distinguish "clean needs_input" from "agent violated the needs_input + # contract" without reading comment prose. + local conflict_label="${label}-conflict" + gh label create "${conflict_label}" --repo "${REPO_FULL_NAME}" \ + --description "Code agent set needs_input but left local commits or an open PR" --color "E4E669" \ + --force 2>/dev/null || true + gh api "repos/${REPO_FULL_NAME}/issues/${ISSUE_NUMBER}/labels" \ + -f "labels[]=${conflict_label}" --silent 2>/dev/null || \ + gha_echo warning "Failed to apply conflict label '${conflict_label}' to issue #${safe_issue_number}" + fi + + local body + body="🚧 **Code agent needs input** — issue #${safe_issue_number} + +The code agent stopped before implementing a fix because it needs input from a human before it can proceed safely. + +**What it needs:** +${sanitized_input} +${caveat_block} +Once this is resolved, remove the \`${label}\` label and re-trigger with \`/fs-code\`." + + if ! gh issue comment "${ISSUE_NUMBER}" \ + --repo "${REPO_FULL_NAME}" \ + --body "${body}" 2>/dev/null; then + gha_echo warning "Failed to post needs-input comment to issue #${safe_issue_number}" + fi +} + # --------------------------------------------------------------------------- # Resolve target branch (ADR 0053) # @@ -246,13 +397,22 @@ else done fi CLOSES_ISSUE="true" +NEEDS_INPUT="" if [ -n "${RESULT_FILE}" ]; then AGENT_TARGET="$(jq -r '.target_branch // empty' "${RESULT_FILE}" 2>/dev/null || true)" AGENT_CLOSES="$(jq -r '.closes_issue // empty' "${RESULT_FILE}" 2>/dev/null || true)" + NEEDS_INPUT="$(jq -r '.needs_input // empty' "${RESULT_FILE}" 2>/dev/null || true)" if [ "${AGENT_CLOSES}" = "false" ]; then CLOSES_ISSUE="false" fi fi + +if [ -n "${NEEDS_INPUT}" ]; then + gha_echo notice "Agent needs input — posting comment and stopping (no PR)" + post_needs_input_comment "${NEEDS_INPUT}" + exit 0 +fi + if [[ -n "${AGENT_TARGET}" && ! "${AGENT_TARGET}" =~ ^[a-zA-Z0-9._/-]+$ ]]; then post_fail_to_issue branch-validation \ "Invalid branch name from agent output: '${AGENT_TARGET}'" @@ -286,8 +446,6 @@ else echo "No agent branch preference — using repo default: ${TARGET_BRANCH}" fi -echo "::add-mask::${PUSH_TOKEN}" - # --------------------------------------------------------------------------- # No-op comment helper # diff --git a/skills/code-implementation/SKILL.md b/skills/code-implementation/SKILL.md index ea8eacd1..1aa0b8c7 100644 --- a/skills/code-implementation/SKILL.md +++ b/skills/code-implementation/SKILL.md @@ -39,7 +39,12 @@ The `scan-secrets` helper is pre-installed in the sandbox image at command -v scan-secrets ``` -If missing, **STOP**. Do not improvise a replacement or skip scanning. +If missing, this is an environment blocker, not a silent stop. Write +`needs_input` to the result file describing the missing helper and path +(e.g. "scan-secrets helper not found in sandbox image at +/usr/local/bin/scan-secrets — cannot verify changes are free of secrets"), +validate structured output (step 11), and stop — no commit. **Do not +improvise a replacement or skip scanning.** Two modes: @@ -400,6 +405,21 @@ uninterpretable" (no viable path forward). For vague-but-actionable issues, implement the most conservative interpretation and note your assumptions in the commit message. +For genuinely uninterpretable issues, do not guess. Write `needs_input` to +the result file explaining specifically what's ambiguous and why no +conservative interpretation is safe, skip implementation entirely, go +straight to step 11 (validate structured output), and stop. No commit. + +```bash +needs_input=$(cat <<'NEEDSINPUT' + +NEEDSINPUT +) +jq --arg ni "$needs_input" '. + {needs_input: $ni}' \ + "${FULLSEND_OUTPUT_DIR}/agent-result.json" > "${FULLSEND_OUTPUT_DIR}/agent-result.json.tmp" \ + && mv "${FULLSEND_OUTPUT_DIR}/agent-result.json.tmp" "${FULLSEND_OUTPUT_DIR}/agent-result.json" +``` + Do not start writing code until you can articulate: what you will change, why, and how you will verify it works. @@ -609,16 +629,15 @@ failures. **If tests or linters fail due to missing tools or infrastructure** (not due to your code): try the Makefile's setup targets first (`make deps`, -`make setup`, etc.). If the tool genuinely cannot be installed in the -sandbox, note this in your commit message body so reviewers know what was -not verified: - -> Note: tests could not run (). -> tests passed. Manual verification of is required. +`make setup`, etc.) — a reasonable number of attempts (typically one, +more only if the failure looks transient). If the tool still cannot run, +write `needs_input` to the result file describing exactly what's missing +(the tool name, the command that failed, and the error), validate +structured output (step 11), and stop — do NOT commit. **Do NOT silently skip tests or linters and commit as if everything -passed.** If you cannot run the relevant test suite or lint command, you -must disclose that. +passed.** If you cannot run the relevant test suite or lint command, use +`needs_input` as described above — do not commit. **If tests or linters fail due to your code:** @@ -636,8 +655,7 @@ must disclose that. The retry limit is read from the `MAX_RETRIES` environment variable (default: 1 if unset). The harness may also enforce a hard timeout independently — if the harness kills the session, your retry count is -irrelevant. Prefer committing with a disclosed issue over burning time -on additional retry iterations. +irrelevant. If the retry limit is reached and tests or linters still fail, do not commit. Validate structured output, then stop: @@ -883,7 +901,7 @@ cat "${FULLSEND_OUTPUT_DIR}/agent-result.json" ``` The file must be valid JSON with `target_branch` (required) and -optionally `pr_body` and `closes_issue`: +optionally `pr_body`, `closes_issue`, and `needs_input`: ```json { @@ -893,8 +911,8 @@ optionally `pr_body` and `closes_issue`: ``` **Schema compliance:** The schema uses `additionalProperties: false`. -Only `target_branch`, `pr_body`, and `closes_issue` are allowed. Any -other fields will cause validation to fail. +Only `target_branch`, `pr_body`, `closes_issue`, and `needs_input` are +allowed. Any other fields will cause validation to fail. Validate the output against the schema: