fix(#4718): stop running pre-code/pre-fix scripts twice per run - #4762
fix(#4718): stop running pre-code/pre-fix scripts twice per run#4762waynesun09 wants to merge 3 commits into
Conversation
reusable-code.yml's inline "Validate inputs" step and pre-fix.sh's equivalent both duplicated the harness pre_script that `fullsend run` already executes right before sandbox creation. For fix, the inline call had no consumer for its output — deleted it; pre-fix.sh continues to run exactly once, as the harness pre_script. For code, the inline call's skipped= output gates four downstream steps (GCP setup, bot identity, agent env, the agent run itself) that all run before `fullsend run` is invoked, so the inline call can't be deleted outright without losing that gate. Instead, gate the existing-human-PR search/label/comment side effects behind a new CODE_SKIP_EXISTING_PR_CHECK env var, set only in harness/code.yaml's forge.github.runner_env — so the second (harness pre_script) invocation skips straight past the GH API search it already ran once, inline, while leaving that inline invocation's behavior (and skipped= output) completely unchanged. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 12:08 PM UTC · Completed 12:27 PM UTC |
PR Summary by QodoDeduplicate pre-code/pre-fix workflow scripts to run once per agent invocation
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
| # Input validation (PR_NUMBER/REPO_FULL_NAME/TRIGGER_SOURCE format, | ||
| # instruction length cap, iteration cap) runs once, as the harness | ||
| # pre_script inside "Run fix agent" below (harness/fix.yaml declares | ||
| # pre_script: scripts/pre-fix.sh, with PR_NUMBER/REPO_FULL_NAME/ | ||
| # TRIGGER_SOURCE/HUMAN_INSTRUCTION/FIX_ITERATION already wired through | ||
| # its runner_env). A duplicate inline call here previously ran the | ||
| # exact same script a second time for no consumer (issue #4718) — | ||
| # unlike the code-agent flow, nothing here gates on its output. |
There was a problem hiding this comment.
I don't think the comment is really necessary, we don't need to keep history of every piece that was at some place.
There was a problem hiding this comment.
Fair — turned out that spot needed to be a real step again anyway (see below), so the comment is moot now; replaced it with the re-added "Validate inputs" step in adbe187.
|
I think you are introducing a feature here with the env var, could you double check that is intended to be here? In that case I disagree. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
ReviewRe-review of e7d15e2 (prior review: adbe187, provenance: app-verified) Approve. The prior review's medium finding — Prior findings — resolution status
Verification summaryCorrectness: All mechanisms verified end-to-end.
Security: No new token scope granted. Tests: 4 new test cases in Execution flow (updated)
Previous runReviewRe-review of adbe187 (prior review: 089dcb2, provenance: app-verified) The overall approach is sound: the fix agent flow is correct (tool install runs exactly once, in the harness FindingsMedium
Low
Execution flow summary
Labels: PR modifies CI workflow files and harness configuration for code and fix agents Previous runReviewFindingsMedium
Low
Previous runReviewRe-review of adbe187 (prior review: 089dcb2, provenance: app-verified) The overall approach is sound: the fix agent flow is correct (tool install runs exactly once, in the harness FindingsMedium
Low
Execution flow summary
Labels: PR modifies CI workflow files and harness configuration for code and fix agents Previous run (2)ReviewFindingsMedium
Low
|
![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.
fullsend-ai-review
Bot
added
the
requires-manual-review
Deleting reusable-fix.yml's inline step outright (previous commit) dropped pre-fix.sh's instruction-length/iteration-cap checks to run only inside "Run fix agent", after GCP setup and agent-env setup — so a capped or malformed fix request now burns that setup cost before failing, instead of failing immediately. Flagged in PR review. Re-add the inline step, but keep it cheap: FIX_SKIP_TOOL_INSTALL defers the pre-commit tool auto-install to the harness pre_script invocation (inside "Run fix agent", where the tools are actually needed for post-fix.sh's pre-commit run), while validation and the iteration cap still run inline for fail-fast behavior. Both invocations still run validation — cheap and idempotent, same tradeoff already made for the code flow. Add pre-fix-test.sh (previously untested) covering validation, instruction-length cap, iteration cap, and the new flag. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
Re: the "introducing a feature with the env var" comment — fair to question, walking through why it's there rather than just removing the inline call outright:
Given the inline call has to stay, If you'd rather see the two responsibilities split into separate scripts instead of toggled by an env var (e.g. a small |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 12:30 PM UTC · Completed 12:46 PM UTC |
|
So changes on harnesses and scripts has no effect here. So given that the pre-code/pre-fix makes sense, I would close PR and issue and open a new one to introduce the mechanisms to avoid some work. Also I would like to see all agents being gated this way for consistency. |
The inline step never set GH_TOKEN, so pre-code.sh's GH_TOKEN guard (added before this branch existed) always short-circuited it straight to skipped=false — the existing-human-PR search, pr-open label, and skip comment never actually ran there. Combined with the second commit's CODE_SKIP_EXISTING_PR_CHECK=true on the harness invocation, that check was running nowhere at all, silently dropping the feature. Caught by fullsend-ai-review[bot] on this PR. Wire GH_TOKEN from steps.app-token.outputs.token, matching how "Resolve bot identity" already sources it, so the check runs where CODE_SKIP_EXISTING_PR_CHECK's own comment claims it does. Also trim the runner_env comment per review feedback (both rh-hemartin and the review bot flagged the multi-line comments in this diff as more history than a workflow/harness file needs). Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Review · ❌ Terminated · Started 3:25 PM UTC · Ended 3:40 PM UTC |
|
Happy to close this and rescope — before I do, could you clarify what mechanism you're picturing? A few things I want to make sure I understand:
Want to make sure whatever gets scoped into the new issue actually matches what you have in mind rather than me guessing again. |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 3:25 PM UTC · Completed 3:40 PM UTC |
I mean that we moved to fullsend-ai/agents the part of harnesses, variables and agents themselves.
Two things revolving this. I would like every agent calling their pre script to gate expensive setups, just for consistency. And then I would like to introduce the env var for all agents as well, so if there is some expensive operator on their pre-script, that can be skipped when running within |
|
Closing per rh-hemartin's request — harness configs, pre/post scripts, and agent definitions have moved to fullsend-ai/agents, so the harness/scripts-level changes here ( Splitting the fix across the correct repos instead:
Keeping #4718 open and will link both PRs there. Per rh-hemartin's broader ask (gating expensive setup consistently across all agents, not just code/fix) — noting that as documented follow-up scope on the issue, not blocking this narrower fix. |
2 participants
Summary
pre-code.shandpre-fix.sheach ran twice per agent invocation — once inline in the reusable workflow, once again as the harnesspre_scriptinsidefullsend run. This removes the duplication: forfix, by deleting the now-redundant inline step; forcode, by gating the expensive/side-effecting part of the duplicated work behind a new env var, since the inline call there can't simply be deleted (itsskipped=output gates four downstream steps that all run beforefullsend runis invoked).Related Issue
Fixes #4718
Changes
.github/workflows/reusable-fix.yml— delete the inline "Validate inputs" step. Nothing in this workflow reads its outputs (unlike the code flow);pre-fix.shcontinues to run exactly once, as the harnesspre_script.PR_NUMBER/REPO_FULL_NAME/TRIGGER_SOURCE/HUMAN_INSTRUCTION/FIX_ITERATIONare already wired into that invocation viaharness/fix.yaml'senv.runner/forge.github.env.runner.internal/scaffold/fullsend-repo/scripts/pre-code.sh— wrap the existing-human-PR check (the GH API search +pr-openlabel + issue comment) behind a newCODE_SKIP_EXISTING_PR_CHECKenv var. Unset (the default, used by the inline workflow step) preserves today's behavior exactly. Input validation and the pre-commit tool auto-install are unaffected and continue to run in both invocations.internal/scaffold/fullsend-repo/harness/code.yaml— setCODE_SKIP_EXISTING_PR_CHECK: "true"inforge.github.runner_env, so only the second (harnesspre_script) invocation ofpre-code.shskips the already-performed check..github/workflows/reusable-code.ymlis unchanged — its inline "Validate inputs" step is still the one that performs the check and produces theskipped=output that gates GCP setup, bot-identity resolution, agent-env setup, and the agent run itself.internal/scaffold/fullsend-repo/scripts/pre-code-test.sh— add coverage for the new flag: bypasses the PR search/label/comment when set, doesn't touchGITHUB_OUTPUT, and existing behavior is byte-for-byte unchanged when unset.Testing
make lintpasses (staged changes)make script-testpasses (pre-code-test.sh26/26, including 4 new cases forCODE_SKIP_EXISTING_PR_CHECK)pre-fetch-prior-review-test.shfails locally on macOS (grep -Punsupported by BSD grep) — confirmed pre-existing onmainbefore this change, unrelated to this PR.Checklist
!for breaking changes)