-
Notifications
You must be signed in to change notification settings - Fork 15
feat(#677): add needs_input pushback for the code agent #682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0a34758
a20426b
d321257
c2738cf
d6288d5
d6c18c4
c49344d
7790f92
59d2c99
b98e021
bdb839f
ff2f3a3
eae7c54
c9f17f6
a3abe70
4b23170
bc153b9
d198257
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ 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. | | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM]
Suggestion: add a Control-labels row for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, 👍 |
||
| | `<CODE_NEEDS_INPUT_LABEL>-conflict` | Applied alongside `fs-code-needs-input` (or the configured label) when the agent set `needs_input` but also left local commits, uncommitted files, or an already-open PR — i.e. the agent violated the `needs_input` contract. The label name derives from `CODE_NEEDS_INPUT_LABEL` with `-conflict` appended (default: `fs-code-needs-input-conflict`). Machine-queryable signal for dashboards to distinguish a clean pushback from a contract violation. | | ||
|
|
||
| ## Configuration | ||
|
|
||
|
|
@@ -49,6 +51,7 @@ See [Customizing with AGENTS.md](https://fullsend.sh/docs/guides/user/customizin | |
| | `FULLSEND_FORGE` | Forge platform. Set automatically by the harness `forge.<platform>.env` section. | (set by harness) | `"github"`, `"gitlab"` | | ||
| | `CODE_AUTO_MERGE` | Set to `"true"` to enable auto-merge on PRs/MRs created by the code agent. On GitHub, uses `gh pr merge --auto`; on GitLab, uses `merge_when_pipeline_succeeds`. 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. Hardcoded to `fs-code-needs-input` in `harness/code.yaml`; override by editing that file (or via `base:` composition). The script's `${CODE_NEEDS_INPUT_LABEL:-fs-code-needs-input}` fallback is unreachable in production since the harness always sets the value. Also determines the conflict label name (`<value>-conflict`). | `fs-code-needs-input` | Any valid GitHub/GitLab label name (safe charset: `[a-zA-Z0-9._:/ -]+`) | | ||
|
|
||
| ## How the agent works | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # 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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Eval case 002 does not forbid the fs-code-needs-input-conflict label, letting a dirty pushback pass
Suggestion: add |
||
| - ready-to-code | ||
|
|
||
| # 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 | ||
|
ralphbean marked this conversation as resolved.
|
||
| # 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). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../../repos/tiny-calc-neutral |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
|
|
@@ -136,23 +139,52 @@ 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") | ||
| if not raw: | ||
| 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: | ||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
| if not prs: | ||
| return False, "No pull requests found — code agent/post-script did not create a PR" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Confirmed against this PR's diff for if not prs:
return False, "No pull requests found — code agent/post-script did not create a PR"
openish = [...]
if not openish:
return False, f"PRs present but none open/merged: {prs}"After this PR both branches collapse into one: if expect_pr:
if not openish:
return False, "No pull requests found — code agent/post-script did not create a PR"So when PRs exist for the branch but all are CLOSED, the eval now reports "No pull requests found", which is factually wrong and drops the PR list from the message. That is the 001-style failure this judge was written to make diagnosable, and it now requires a re-run with extra logging to distinguish. Suggestion: restore the two-message split inside the Dedup note: the existing |
||
| if not openish: | ||
| return False, f"PRs present but none open/merged: {[p.get('url') for p in prs]}" | ||
| return True, f"PR created: {[p.get('url') for p in openish]}" | ||
| if prs: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Verified at head if prs:
return False, f"Expected no PR at all (needs_input case) but found: ..."
return True, "No PR created, as expected (needs_input case)"It returns
The asymmetry is the problem. For The fix pattern already exists in the same script: Note on novelty: distinct from the existing comment at Suggested fix: Have |
||
| 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) | ||
|
|
@@ -162,7 +194,12 @@ judges: | |
| if not raw: | ||
| return False, "fixture-state.json not found — capture-fixture.sh did not run or failed" | ||
| state = json.loads(raw) | ||
| expect_pr = outputs.get("annotations", {}).get("expect_pr", True) | ||
| expected = outputs.get("annotations", {}).get("expected_files") or [] | ||
| if not expect_pr: | ||
| if expected: | ||
| return False, "expected_files declared but expect_pr is false — contradictory annotation" | ||
| return True, "expected_files not checked when expect_pr is false (needs_input case)" | ||
| if not expected: | ||
| return True, "No expected_files declared" | ||
| # Filter to OPEN/MERGED like pr_created — a closed/superseded PR's | ||
|
|
@@ -242,6 +279,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: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| """Tests for calc module — signature checks only. | ||
|
|
||
| Behavioral assertions (specific return values) are deliberately omitted: | ||
| this fixture is used by eval case 002-push-back-on-nonsense, which presents | ||
| a genuinely contradictory requirement. Neither the implementation nor the | ||
| tests should favor one side of the contradiction. | ||
| """ | ||
|
|
||
| from calc import add | ||
|
|
||
|
|
||
| def test_add_callable() -> None: | ||
| """add() accepts two int arguments and returns an int.""" | ||
| result = add(0, 0) | ||
| assert isinstance(result, int) | ||
|
|
||
|
|
||
| def test_add_negative_args() -> None: | ||
| """add() accepts negative arguments without raising.""" | ||
| result = add(-1, -2) | ||
| assert isinstance(result, int) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| #!/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 "valid-needs-input-with-pr-body-without-target-branch" \ | ||
| '{"needs_input":"Environment broken — pip not available","pr_body":"partial context"}' \ | ||
| "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" |
Uh oh!
There was an error while loading. Please reload this page.