feat(#677): add needs_input pushback for the code agent - #682
Conversation
|
🤖 Finished Review · ✅ Success · Started 8:11 PM UTC · Completed 8:28 PM UTC |
PR Summary by QodoAdd needs_input pushback path for the code agent (label + comment, no PR)
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsMedium
Low
Previous run (11)ReviewFindingsMedium
Low
Previous run (12)ReviewFindingsMedium
Previous run (13)ReviewFindingsHigh
Medium
Low
Labels: PR implements the needs_input pushback feature for the code agent, modifying agent definitions, harness config, post-scripts, skills, and eval infrastructure. Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional finding (no line in this PR's diff to anchor it to — schemas/code-result.schema.json line 7 isn't within the changed hunk):
[MEDIUM] target_branch kept unconditionally required, untested for the broken-tooling needs_input scenario — schemas/code-result.schema.json:7-8
The schema keeps required: ["target_branch"] unconditional even when needs_input is set. The PR's own design doc (docs/plans/code-agent-needs-input.md) justifies this only as "per current design the agent always writes target_branch regardless" and explicitly lists it under "Open items to watch during implementation" as an unconfirmed assumption, not a verified guarantee. The PR's stated motivation for needs_input is two-fold — (1) a genuinely uninterpretable issue and (2) broken sandbox tooling/environment — but only scenario (1) got an eval case (eval/code/cases/002-push-back-on-nonsense/); there is no case exercising a broken-environment run where the agent's normal means of determining target_branch (git/gh calls) might also fail. If that happens, agent-result.json fails schema validation, validation_loop skips post_script per ADR 0022, and the needs_input signal this feature exists to produce is lost silently — regressing to the pre-PR generic no-op.
Suggestion: Either add an eval case simulating broken tooling (unrelated to git/gh) to confirm target_branch is still reliably produced, or relax the schema so target_branch is optional when needs_input is set (e.g. via oneOf/if-then), since no push/PR happens on the needs_input path regardless of target_branch's value.
|
Re: #682 (comment) Good catch on the schema-compliance line in SKILL.md step 11 — it still said only On the protected-path note: intentional — this feature has to touch scripts/, harness/, and skills/ to exist at all. The schema-compatibility point (optional field + additionalProperties: false being backward-incompatible for a stale CLI copy of the schema) is a real question but not one I can resolve unilaterally — flagging it for a human to confirm how the fullsend CLI resolves this schema at runtime. |
|
Re: #682 (comment) These four findings are the same ones raised inline — handled there: protected-path note dismissed as intentional, the |
- Fix wrong label name (needs-input -> fs-code-needs-input) in agents/code.md and the needs_input schema description. - Close a pr_created judge loophole: fail on any PR at all (open, merged, or closed), not just open/merged, when expect_pr is false. - SKILL.md: needs_input is now listed among the allowed output fields (step 11), and the stale "you must disclose that" line (step 9c) now points at needs_input instead of the old disclosure flow. - Remove docs/plans/code-agent-needs-input.md and ignore docs/plans/ going forward -- planning scratch files aren't meant to be committed. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Warn (instead of silently swallowing) label create/apply/remove failures in post_needs_input_comment, matching the existing comment-post failure pattern. - Stop truncating the needs_input comment from the tail -- it's forward, human-authored prose already length-capped by the schema (maxLength 4000), not command/log output where tail-ing makes sense. Truncating from the tail dropped the opening context of longer explanations. - Guard against a needs_input contract violation: warn (in both the workflow log and the posted comment) if the agent committed local work before setting needs_input, since that work is silently discarded, and check for an already-open PR on the branch to avoid posting a "no PR" comment alongside a real one. Adds a regression test for the truncation fix and two git-repo-backed tests for the new contract-violation guards. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
Re: #682 (comment) Following up on the schema-compatibility point — the schema ships bundled with this agent, not the CLI, so an old pinned CLI paired with the new agent would indeed reject |
max_turns/max_cost_usd were plausibility-based guesses. Update them using the one CI run we have (21 turns / $0.64, run 31042840745), applying the same headroom multipliers as 001-fix-add (~1.7x turns, ~2x cost) since we only have a single observation so far. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 10:09 PM UTC · Completed 10:23 PM UTC |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
post_needs_input_comment's discarded-commits check silently fell back to "main" when the gh api call for the repo's default branch failed. If the actual default branch differs, the subsequent git rev-list comparison silently reports zero commits ahead, dropping the discarded-commits caveat this check exists to surface. Now it warns via gha_echo when the API call fails, so the inaccuracy is visible in the workflow log. Addresses review feedback from waynesun09 on PR #682. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
"one attempt only" for Makefile setup-target retries was an unsubstantiated specific number. Softened to "a reasonable number of attempts (typically one, more only if the failure looks transient)" per review feedback, so the agent has room to judge transient vs. persistent failures rather than following a hardcoded count with no cited basis. Addresses review feedback from waynesun09 on PR #682. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 4:21 PM UTC · Completed 4:37 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional review findings
[HIGH] scripts/post-code.src.sh:248 — PUSH_TOKEN log-masking is registered after the needs_input path already makes several PUSH_TOKEN-authenticated calls
(Not attachable inline: this line is unchanged code, outside the PR's diff hunks.)
echo "::add-mask::${PUSH_TOKEN}" at line 248 only executes after target-branch resolution, which is well past the if [ -n "${NEEDS_INPUT}" ]; then post_needs_input_comment "${NEEDS_INPUT}"; exit 0; fi block (lines 209-213). post_needs_input_comment calls _post_failure_ensure_token (exports GH_TOKEN=PUSH_TOKEN when unset) and then issues gh label create, two gh api .../labels calls, gh pr list, gh api repos/.../ --jq .default_branch, and gh issue comment, then exit 0s — none of these ever pass through the GitHub Actions log-mask registration for the token, since the exit happens before line 248 is reached. This script's own header explicitly calls out token handling as the reason it is "the most security-sensitive component in the pipeline," and needs_input is a normal, expected, exit-0 outcome this whole PR builds a workflow around — not a rare edge case — so this is now a routine, frequently-exercised path missing the log-redaction layer.
Suggestion: Move echo "::add-mask::${PUSH_TOKEN}" to immediately after : "${PUSH_TOKEN:?PUSH_TOKEN is required}" near the top of the script, before the ERR trap and before any code path (including post_needs_input_comment and the pre-existing early post_fail_to_issue calls that share the same gap) can use the token.
…ame in comments harness/code.yaml hardcoded CODE_NEEDS_INPUT_LABEL to the default label instead of passing through the runner env var, silently defeating the operator override the script and its tests expect. post-code.src.sh interpolated the raw current_branch (chosen by the code agent while processing potentially adversarial issue content) into a public GitHub comment wrapped only in backticks. Git ref names permit backticks, so a malicious branch name could break out of the markdown code span and inject content into a comment posted with the bot's write token. Validate current_branch against the same safe-charset regex used for AGENT_TARGET and substitute a redacted placeholder when it fails, while still using the real branch name for the underlying gh/git checks. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 7:12 PM UTC · Completed 7:29 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The passthrough broke CI: fullsend validates env.runner strictly and errors if a referenced host variable is entirely unset, not just empty. CODE_ALLOWED_TARGET_BRANCHES avoids this because eval/scripts/run-fullsend.sh explicitly emits it (even empty). CODE_NEEDS_INPUT_LABEL has no such emitter here, and no external reusable workflow forwards it into the runner env either, so treating it as an operator-configurable env var would break every production run of the code agent, not just eval. The label is still configurable the same way CODE_NEEDS_INPUT_LABEL plumbing exists for at all: operators fork/edit harness/code.yaml directly to change the literal value. The script's own :-fs-code-needs-input fallback and its env-override test are unrelated to the harness and remain valid on their own. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
The post-code-needs-input-test.sh script runs the real post-code.src.sh which calls gitleaks inside post_needs_input_comment() — but gitleaks is installed later by install_gitleaks(), well past the needs_input early-exit path. On CI runners where gitleaks is not pre-installed, the scan fails with exit 127 and the content is replaced with a generic redacted message, breaking test assertions that check for the original needs_input text. - Add a mock gitleaks binary (exit 0, no secrets) to MOCK_BIN in the test, matching the existing gh mock pattern - Add CODE_NEEDS_INPUT_LABEL to eval/scripts/run-fullsend.sh env file for code/fix agents (empty = use default), matching the pattern of CODE_ALLOWED_TARGET_BRANCHES Addresses review feedback on #682
post_needs_input_comment() calls gitleaks detect to scan the needs_input text, but install_gitleaks was only called in step 3 — after the needs_input early-exit. On CI runners without a pre-installed gitleaks binary, the scan failed with exit 127 and the content was silently replaced with a generic redacted message. Fix: call install_gitleaks inside post_needs_input_comment before the gitleaks detect call (no-op when already on PATH). Remove the gitleaks mock from post-code-needs-input-test.sh since the script now properly ensures the binary is available. Addresses review feedback on #682
…ebase The rebase onto main picked up the multi-forge change (PR #813) which requires FULLSEND_FORGE to be set. The needs-input test was missing this env var, causing all test invocations to fail with "invalid FULLSEND_FORGE" at script load time. Also resolves merge conflicts from the rebase: - harness/code.yaml: kept both CODE_NEEDS_INPUT_LABEL (PR) and sandbox env vars (main) - scripts/post-code.sh, post-code.src.sh: merged GITLAB_TOKEN masking (main) with early PUSH_TOKEN masking (PR) Addresses review feedback on #682
- Validate existing_pr_url through _sanitize_workflow_value and a safe-charset regex before interpolating into the needs_input issue comment body (defense-in-depth, per waynesun09 review finding). - Short-circuit expected_files eval judge when expect_pr is false so a future needs_input case accidentally declaring expected_files produces a clear error instead of a confusing PR-files failure. - Add schema test for needs_input combined with pr_body but without target_branch, exercising the conditional if/then logic. Addresses review feedback on #682
- Remove the current_branch != default_branch gate so commits_ahead is checked even when the agent stays on the default branch - Replace || echo 0 with explicit git failure detection so a rev-list error is logged instead of silently reporting 0 commits - Add git status --porcelain check for uncommitted working-tree changes that would be silently discarded - Validate CODE_NEEDS_INPUT_LABEL against a safe-charset regex before use in API calls and comment body interpolation - Add gitleaks mock to needs-input tests so they pass on CI runners without a pre-installed gitleaks binary - Fix long-text test fixture to use real newlines (via jq) instead of relying on JSON \n escape interpretation through the bash→jq chain Addresses review feedback on #682
🔧 Fix agent — iteration 9 (human-triggered)Fixed validation failure caused by Signed-off-by trailers in 8 of 16 PR commits. The pre-commit gate checks all commits in the merge-base..HEAD range for Signed-off-by trailers and rejects them for agent branches. Used git filter-branch with a sed-based msg-filter to remove all Signed-off-by lines from the commit messages while preserving the rest of the commit history and content. Fixed (1):
Tests: passed Decision points
Next steps:
|
1d6862f to
4b23170
Compare
|
🤖 Review · ❌ Terminated · Started 8:11 PM UTC · Ended 8:29 PM UTC Commit: |
|
/fs-fix address comments from @waynesun09 |
|
🤖 Finished Fix · ✅ Success · Started 8:21 PM UTC · Completed 8:40 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.13 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 8:11 PM UTC · Completed 8:29 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.67 |
- Convert post_needs_input_comment to use forge helpers (forge_create_label, forge_add_label, forge_remove_label, forge_list_prs_for_branch, forge_get_pr_url, forge_post_issue_comment, forge_get_default_branch) so the needs_input path works on both GitHub and GitLab instead of silently no-oping on GitLab via direct gh calls. - Add forge_remove_label to both github-code-ops.lib.sh and gitlab-code-ops.lib.sh following the forge_add_label pattern. - Use forge_list_prs_for_branch (with headRepositoryOwner / source_project_id filter) for the existing-PR check, fixing the cross-fork false-positive. - Use agent's target_branch from RESULT_FILE as the comparison base for discarded-commits guard instead of always using the repo default branch, fixing false counts on non-default targets. - Use git status --porcelain --untracked-files=no so untracked files (build artifacts from make setup) don't trigger the conflict label. - Add fs-code-needs-input-conflict label to docs/code.md Control labels table. - Update CODE_NEEDS_INPUT_LABEL docs to reflect it's hardcoded in harness/code.yaml, not forwarded from runner env. - Remove dead emit_env CODE_NEEDS_INPUT_LABEL from eval runner. - Add ready-to-code to forbidden labels in case 002 annotations. - Neutralize test_calc.py assertions (signature-only checks) so the fixture doesn't bias toward one side of the contradiction. - Restore two-message split in pr_created judge for the expect_pr:true branch (distinguishes "no PRs" from "PRs present but none open/merged"). - Add untracked-files-only test case to confirm no false conflict. - Update test mock to handle forge helper gh call patterns. Addresses review feedback on #682
🔧 Fix agent — iteration 9 (human-triggered)Addressed 9 remaining review findings from @waynesun09. Key changes: converted post_needs_input_comment from direct gh calls to forge helpers (works on both GitHub and GitLab), added forge_remove_label to both forge libs, fixed cross-fork owner filter, used agent's target_branch for discarded-commits comparison, excluded untracked files from dirty-tree check, documented the conflict label, corrected CODE_NEEDS_INPUT_LABEL docs, added ready-to-code to forbidden labels, neutralized fixture test assertions, and restored the pr_created judge's two-message diagnostic. Disagreed with 1 finding (capture-fixture fallback) as out of PR scope. Fixed (9):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:42 PM UTC · Completed 9:00 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.06 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 9:24 PM UTC · Completed 9:36 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.74 |
- Remove docs/plans/ from .gitignore (scope-creep: unrelated to #677) - Clarify SKILL.md step 11: target_branch is required unless needs_input is set, matching the schema's if/then clause - URL-encode label in forge_remove_label() to handle spaces in label names (github-code-ops.lib.sh + rebuilt bundles) Addresses review feedback on #682
🔧 Fix agent — iteration 10 (human-triggered)Addressed 3 of 5 low-severity review findings: removed scope-creep .gitignore entry, clarified conditional target_branch requirement in SKILL.md, and URL-encoded labels in forge_remove_label(). Disagreed with 2 findings (bundled script artifact, mock pattern fragility) as not actionable. Fixed (3):
Disagreed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 9:37 PM UTC · Completed 10:14 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.76 |
| echo "main" | ||
| ;; | ||
| *"pr list --repo ${REPO_FULL_NAME} --head"*"--json number,headRepositoryOwner"*) | ||
| # forge_list_prs_for_branch — return PR number |
There was a problem hiding this comment.
[low] test-coverage
The mock gh script matches forge_list_prs_for_branch calls using a fixed argument pattern. If arguments are reordered in a future refactor, the mock silently stops matching and existing-PR test assertions pass vacuously. Inherent to pattern-based mocking and acceptable, but a regression risk.
| fi | ||
| } | ||
|
|
||
| forge_remove_label() { |
There was a problem hiding this comment.
[low] scope-creep
forge_remove_label() is defined in pre-code.sh (both GitHub and GitLab sections) but never called there. Build-system artifact from bundling the shared lib files, not intentional scope creep.
|
/fs-fix address findings with obvious resolution |
|
🤖 Finished Fix · ❌ Failure · Started 1:36 AM UTC · Completed 1:36 AM UTC Commit: Effort: high |
|
/fs-fix |
|
🤖 Finished Fix · ❌ Failure · Started 12:02 PM UTC · Completed 12:02 PM UTC Commit: Effort: high |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
[MEDIUM] validate-code-output.src.sh has no needs_input awareness, so a contract-violation pushback can be swallowed by the pre-commit retry loop instead of surfacing (scripts/validate-code-output.src.sh:113, not in this PR's diff so posting here instead of inline)
needs_input does not appear anywhere in scripts/validate-code-output.src.sh (confirmed by grep at head) and the file is untouched by this PR's diff — the needs_input feature this PR adds was never wired into this consumer. This is the validation_loop.script for the code harness; per ADR 0022 (referenced elsewhere in this same PR's own eval.yaml judge docstring), the post-script — where post_needs_input_comment lives — only runs after validation_loop passes. If the agent leaves the working tree in a state that fails the pre-commit gate (dirty/committed changes that don't pass hooks) while also setting needs_input in agent-result.json (schema-valid: needs_input is accepted alone per schemas/code-result.schema.json's if/then, target_branch not required), Part 2 of this script still runs the pre-commit gate against TARGET_REPO_DIR, fails, and returns exit 1 with a generic "FAIL: pre-commit-blocked" message that feeds back into another retry iteration — the needs_input signal is never read, so the human never sees the pushback and the run burns turns/cost instead of surfacing the exact contract-violation scenario this PR was built to catch. The clean/no-work path is unaffected (no changes -> CHANGED_FILES empty -> gate skipped).
Suggestion: have validate-code-output.src.sh read needs_input from RESULT_FILE (already parsed for target_branch) early in Part 2 and soft-pass (skip) the pre-commit gate when it's non-empty, so the post-script always runs and post_needs_input_comment's own contract-violation handling gets a chance to fire.
| sed 's/^/::debug::gitleaks: /' "${gl_stderr}" | ||
| fi | ||
| rm -f "${gl_stderr}" | ||
| if [ "${gl_rc}" -eq 1 ]; then |
There was a problem hiding this comment.
[MEDIUM] needs_input secret-scan misattributes generic gitleaks errors as "secret detected"
post_needs_input_comment() branches gl_rc -eq 1 -> "BLOCKED — secret detected" vs gl_rc -gt 1 -> "scan failed" (this line through 401). Empirically verified against the pinned gitleaks v8.30.1 binary (built via this repo's own scripts/lib/gitleaks-install.lib.sh): gitleaks detect --source /nonexistent/path --no-git --redact — a plain file-stat error, not a leak — exits 1, identically to a genuine finding; only an unknown-flag error exits 126. So a transient tmpfile/permissions/internal gitleaks error on this path will almost always land in the -eq 1 branch and post "the agent's explanation contained a potential secret" to the issue when no secret exists. Content is redacted either way (no security regression), but the diagnostic is misleading.
Note: this is not a new deviation invented by this PR — the pre-existing pr_body scan later in the same file (around line 867, same GL_RC eq-1/gt-1 split) does the identical thing, so this new code faithfully copied a pre-existing misclassification rather than introducing a new one.
Suggestion: either inspect gitleaks' JSON report (--report-format json) to positively confirm a finding rather than inferring from exit code, or soften the exit-1 message to acknowledge it could be either a finding or a scan error. Consider applying the same fix to the pre-existing pr_body scan for consistency, since it has the identical ambiguity.
| labels: | ||
| required: | ||
| - fs-code-needs-input | ||
| forbidden: |
There was a problem hiding this comment.
[MEDIUM] Eval case 002 does not forbid the fs-code-needs-input-conflict label, letting a dirty pushback pass
labels.forbidden is [ready-to-code] only. The case requires fs-code-needs-input and asserts expect_pr: false, but never forbids <CODE_NEEDS_INPUT_LABEL>-conflict (default fs-code-needs-input-conflict). An agent that leaves an uncommitted-only partial fix (which skips the pre-commit gate entirely since validate-code-output's CHANGED_FILES check only sees committed diffs) and sets needs_input would trip post_needs_input_comment's own dirty-tree caveat, get the -conflict label applied, open no PR, keep the required fs-code-needs-input label — and pass this eval case regardless, since neither pr_created, required_labels, nor forbidden_labels inspects the conflict label. This is distinct from the existing thread on this same file (the one about ready-to-code never being asserted-removed) — that thread does not mention the conflict label.
Suggestion: add fs-code-needs-input-conflict (or <CODE_NEEDS_INPUT_LABEL>-conflict) to labels.forbidden in this case's annotations.yaml so a contract-violating pushback fails the eval instead of passing.
3 participants
Summary
needs_inputfield to the code-result schema so the agent can refuse to open a PR (broken sandbox tooling or a genuinely uninterpretable issue) and instead post an explanatory comment +fs-code-needs-inputlabel.eval/code/eval.yaml'spr_createdjudge to assert the negative whenannotations.expect_pr: false, add arequired_labelsjudge, and add eval case002-push-back-on-nonsensecovering the pushback path.Test plan
make check-bundlemake testCloses #677
Assisted-by: Claude Opus 4.6 noreply@anthropic.com