fix(#4718)!: remove inline pre-script calls and scaffold script copies - #5739
Conversation
|
🤖 Finished Review · ✅ Success · Started 9:06 PM UTC · Completed 9:22 PM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewReason: stale-head The review agent reviewed commit Previous run (3)ReviewFindingsHigh
Medium
Low
Labels: PR modifies CI workflow files under .github/workflows/ and scaffold scripts 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.
6301ca4 to
1a5a227
Compare
PR Summary by QodoRemove inline pre-script validation and scaffolded pre-code/pre-fix scripts
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
|
🤖 Finished Review · ✅ Success · Started 9:32 PM UTC · Completed 9:47 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
1a5a227 to
841a670
Compare
|
/fs-review |
Site previewPreview: https://3548bf17-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · |
841a670 to
0b5579c
Compare
|
🤖 Finished Review · ✅ Success · Started 9:59 PM UTC · Completed 10:20 PM UTC |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
The inline Validate inputs steps ran pre-code.sh/pre-fix.sh on the bare runner while the harness pre_script ran the agents-repo copy inside fullsend run — two executions per run from two independently maintained copies (#4718, #5667). With the pre-script output protocol in fullsend run, the CLI owns the skip decision, so: - reusable-code.yml, reusable-fix.yml, and both inlined stages in reusable-dispatch.yml lose their Validate inputs steps and every steps.validate.outputs.skipped gate, converging toward the generic harness-run job shape (ADR 0061). That job does not yet carry the code/fix pre-script env contract, so the shapes are not yet interchangeable. - COMMENT_BODY moves onto the Run code agent step env so the pre-script inside fullsend run can read it (env-quoted, never interpolated into run: blocks). Its --force override is not end-to-end yet: the route job's own Check for existing PRs step gates stage=code first and does not honor --force. - The scaffold copies of pre-code.sh, pre-fix.sh, and pre-code-test.sh are deleted; fullsend-ai/agents becomes the single source (closes the #5667 scope). Companion scripts (install-precommit-tools.sh, resolve-precommit-tools.py) stay — the agents-repo pre-fix.sh resolves them via the workspace fallback. Fix-side enforcement (input validation, the 10,000-byte HUMAN_INSTRUCTION cap, and the bot-5/human-10 iteration caps) is exit-code-based in the agents-repo pre-fix.sh, so it is version-independent and unaffected by the protocol rollout. It does lose its fail-fast position: those checks now run inside fullsend run, after GCP setup and token mint, rather than before them. BREAKING CHANGE: the scaffold embed no longer ships scripts/pre-code.sh or scripts/pre-fix.sh. Installations referencing these paths directly must use the fullsend-ai/agents copies via harness pre_script. Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
0b5579c to
fe88c04
Compare
|
🤖 Finished Review · ✅ Success · Started 5:30 PM UTC · Completed 5:51 PM UTC |
The force override matched --force as a substring anywhere in the
comment body, so a comment merely mentioning it ("please don't use
--force here", --forceful, or a pasted log line) silently disabled the
existing-PR check. Parse the first line and honor --force only as the
command's flag token (/fs-code --force), mirroring the dispatch
router's first-line tokenization. fullsend-ai/fullsend#5739 wires
COMMENT_BODY into the in-run pre-script for the first time, so this
branch is about to become reachable in production.
Test hardening:
- Assert GITHUB_OUTPUT stays empty on every path, locking in the
removal of the legacy skipped= writes that would collide with
fullsend run's protocol relay (last-write-wins).
- Run the script with FULLSEND_PRESCRIPT_OUTPUT, CODE_FORCE and
COMMENT_BODY explicitly unset so the caller's environment cannot
leak into the cases that depend on their absence.
- Add three regression tests for the --force anchoring; verified by
mutation (substring match fails exactly those three, legacy
GITHUB_OUTPUT writes fail the new assertion on all paths).
Assisted-by: Claude (review and fix)
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Retro · ✅ Success · Started 6:49 PM UTC · Completed 7:07 PM UTC |
Retro: PR #5739 — Remove inline pre-script calls and scaffold script copiesTimeline
Workflow AssessmentReview quality was strong. All findings across 5 review runs were valid and well-calibrated. Severity appropriately decreased from High to Medium as the PR evolved (dependency on #5737 was documented as a release gate). The review agent correctly identified removed security controls (input validation, iteration caps, instruction length caps) and stale documentation. Main waste areas:
Evidence for Existing Issues
No Novel ProposalsAll improvement opportunities identified in this retro are covered by existing open issues. The workflow performed well overall — the review agent produced valid, actionable findings, severity calibration was appropriate, and the human reviewer made an informed approval decision accepting documented known gaps. |
When an open human PR already addresses the issue, pre-code.sh now writes skipped=true plus a reason to the file named by FULLSEND_PRESCRIPT_OUTPUT (fullsend docs/normative/prescript-output/v1), so fullsend run stops before creating the sandbox instead of posting the "skipping" comment and then running the agent anyway. The GITHUB_OUTPUT skip writes are removed — they served the reusable workflows' inline invocation, which fullsend-ai/fullsend#5739 deletes. The write is guarded on the variable being present: under a CLI that predates the protocol the script behaves exactly as before (fails open, per the protocol's version-skew contract). Proceed paths write nothing — an absent skipped key means proceed. pre-fix.sh needs no protocol change: the fix agent has no skip semantics, and its double tool-install disappears when the inline invocation is removed (fullsend-ai/fullsend#5739). Adds pre-code-test.sh (ported from the fullsend scaffold copy that fullsend-ai/fullsend#5739 deletes, adapted to assert the protocol file instead of GITHUB_OUTPUT) and wires it into make script-test. Part of fullsend-ai/fullsend#4718. Supersedes fullsend-ai#175. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
2 participants
Summary
Phase 2 of the #4718 plan: with the pre-script output protocol in
fullsend run(#5737), the inlineValidate inputssteps are redundant — and they were the last consumer of the scaffold copies ofpre-code.sh/pre-fix.sh. This PR removes the inline calls and gates from all three reusable workflows and deletes the scaffold script copies, makingfullsend-ai/agentsthe single source (the #5667 scope, closed as duplicate into #4718).#5737 has merged (
306acf0a), so this now targetsmaindirectly.⛔ Merge/release gates
The binding constraint is the release cut, not the merge. The CLI resolves agents content from
fullsend-ai/agentsattags/v0(tryAgentsRepoFallback). fullsend'srelease.ymlmoves only fullsend's ownv0and pushes the release tag to the agents repo at itsmainHEAD; agentsv0is then moved by the agents repo's ownrelease.ymlin a separate, asynchronous run triggered by that tag push (which skips pre-releases). fullsend neither performs nor verifies that second move, so the ordering must be enforced by process:fullsend-ai/agents@mainbefore the fullsend release carrying this commit is tagged. Merging it to agentsmainafterwards changes nothing at runtime until the next release.skipped=true/reasonviaFULLSEND_PRESCRIPT_OUTPUTper the v1 contract, and remove their existing directskipped=writes to$GITHUB_OUTPUT—prescript.Relaynow writes to that same file, so leaving both in place makes the two collide (last-write-wins).If a release ships this commit while agents
v0still points at the old scripts, the in-run skip is a no-op for every@v0install until the following release — not a merge window measured in hours.Post-release verification:
gh api repos/fullsend-ai/agents/git/ref/tags/v0must resolve to a commit whosescripts/pre-code.shcontainsFULLSEND_PRESCRIPT_OUTPUT; then re-run one code dispatch on an issue with an open human PR.Current state: latest release
v0.32.0(2026-07-20) predates the protocol. fullsend-ai/agents#536 — the protocol adoption inpre-code.sh(shared-lib composed, with the--forceoverride anchored to the command's flag token) — is approved and enqueued to agentsmain; the next full release's tag push then recuts agentsv0with it. fullsend-ai/agents#175 is still open with the withdrawn skip-flag design and should be closed rather than rebased.Scope of the interim regression
Narrower than "no duplicate-PR gating", but contradictory rather than silent. The route job's own
Check for existing PRsstep (reusable-dispatch.yml) and the per-org dispatcher both run an equivalent search before the code stage and are untouched here. On the cases that do reach the pre-script (search-index timing, non-routed paths): the pre-protocol agentsv0script still runs authenticated insidefullsend run(mint setsGH_TOKENbefore the pre-script), so it does post the "an open PR already addresses this issue" comment and apply thepr-openlabel — but itsskipped=truegoes to$GITHUB_OUTPUT, which nothing consumes, andprescript.Relaythen writesskipped=falselast (last-write-wins). The run proceeds and opens a duplicate bot PR immediately after commenting that it would skip. The recut delivers the skip itself; the label and comment were never the gap.Fix-side enforcement is unaffected either way: input validation, the 10,000-byte
HUMAN_INSTRUCTIONcap, and the bot-5/human-10 iteration caps all terminate withexit 1in the agents-repopre-fix.sh, and a non-zero pre-script exit is a hard failure on every CLI version.Related Issue
Part of #4718. Closes the #5667 scope. Continues #5588's scaffold cleanup.
Changes
reusable-code.yml+reusable-dispatch.yml(inlined code stage): removeValidate inputsand all foursteps.validate.outputs.skippedgates; addCOMMENT_BODYto the run step env (env-quoted) so the in-run pre-script can read it.reusable-fix.yml+reusable-dispatch.yml(inlined fix stage): removeValidate inputs.internal/scaffold/fullsend-repo/scripts/{pre-code.sh,pre-fix.sh,pre-code-test.sh}; drop theexecutableFilesentry and the Makefilescript-testline. Companion scripts (install-precommit-tools.sh,resolve-precommit-tools.py) intentionally remain — the agents-repopre-fix.shresolves them via the workspace fallback.GH_TOKENneeds no workflow wiring:fullsend runmints the agent token (os.Setenv) before the pre-script executes, so the existing-PR check insidefullsend runis authenticated — which the old inline step wasn't (itsenv:had noGH_TOKEN, so it always short-circuited and emittedskipped=false; the gates it fed were effectively dead code, which is the silent breakage fix(#4718): wire GH_TOKEN and re-add fail-fast validation for pre-code/pre-fix #5013 tried to patch).executableFilesentry fromcli-internals.md, and note incustomizing-agents.mdthatpre_scriptpaths resolve against the harness's own repo — a locally defined harness must ship its own copies.Known gaps, deliberately not addressed here
pre-code-test.sh(26 cases) is deleted and has no counterpart in the agents repo, which has nopre-code-test.sh/pre-fix-test.sh. The recut should port it and extend it to assert theFULLSEND_PRESCRIPT_OUTPUTwrites.fullsend run, after GCP setup and token mint, rather than before them. A capped-out fix loop therefore burns WIF auth plus a mint per attempt and surfaces as an agent-run ❌ rather than a cheap step failure.--forceis not end-to-end. The route job'sCheck for existing PRsgate does not honor--force, so/fs-code --forceis still stopped before the pre-script seesCOMMENT_BODY. The wiring here is correct but only helps on non-routing-gated paths.action.ymlexposesoutputs.skipped(empty ⇒ CLI predates the protocol) but nothing consumes it. A repo whoseuses:ref contains this commit whilefullsend_versionresolves to a pre-protocol release loses the code skip silently.Testing
make lintpasses, including actionlint on all three workflowssteps.validatereferences anywhere in.github/,internal/,e2e/go build ./...,go test ./internal/scaffold/(embed/file-mode sync tests pass post-deletion)fullsend-ai/agents: the agents copies ofpre-code.sh/pre-fix.share line-for-line equivalent to the deleted scaffold copies apart from comments and an addedGITHUB_WORKSPACEguard;PATH/pre-commit-tool install is re-done independently bypost-code.sh/post-fix.sh, so no regression theremake script-test: only pre-existing local failures inpre-fetch-prior-review-test.sh(reproduced identically on the base commit; untouched by this PR)Checklist
!for breaking changes)