fix(#4718): wire GH_TOKEN and re-add fail-fast validation for pre-code/pre-fix - #5013
fix(#4718): wire GH_TOKEN and re-add fail-fast validation for pre-code/pre-fix#5013waynesun09 wants to merge 1 commit into
Conversation
pre-code.sh and pre-fix.sh (now in fullsend-ai/agents) grew CODE_SKIP_EXISTING_PR_CHECK and FIX_SKIP_TOOL_INSTALL flags so their harness pre_script invocation (inside `fullsend run`) can skip the work already done by these workflows' inline pre-run steps, instead of repeating it. This is the fullsend-ai/fullsend half of that split: - reusable-code.yml: pass GH_TOKEN to the inline "Validate inputs" step. It never had one, so pre-code.sh's existing-human-PR check (GH API search, pr-open label, skip comment) was silently a no-op there — the harness invocation was the only place it ever ran. Once CODE_SKIP_EXISTING_PR_CHECK is set on that harness invocation, the check needs to actually work on this side. - reusable-fix.yml: re-add a lightweight inline "Validate inputs" step with FIX_SKIP_TOOL_INSTALL=true, so PR_NUMBER/instruction/ iteration-cap validation still fails fast before GCP/agent-env setup, without re-running the pre-commit tool auto-install that only the harness invocation needs. Depends on the matching fullsend-ai/agents change for the flags themselves to exist. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 1:49 PM UTC · Completed 1:57 PM UTC |
PR Summary by QodoFix reusable workflows: pass GH_TOKEN and restore fail-fast pre-* validation
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Jobs are now inlined, you need to rebase and update reusable-dispatch.yml as well |
Code Review by Qodo
1. pre-code.sh runs twice
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| GH_TOKEN: ${{ steps.app-token.outputs.token }} | ||
| FULLSEND_DIR: ${{ inputs.install_mode == 'per-repo' && '.fullsend' || '' }} | ||
| run: bash "${FULLSEND_DIR:+$FULLSEND_DIR/}scripts/pre-code.sh" |
There was a problem hiding this comment.
1. pre-code.sh runs twice 📎 Requirement gap ☼ Reliability
The reusable code workflow still runs scripts/pre-code.sh inline in Validate inputs, and then runs fullsend run code, which executes the same script again via harness/code.yaml pre_script. This violates the requirement to avoid double-running the pre-script and can cause duplicated side effects and wasted CI time.
Agent Prompt
## Issue description
`.github/workflows/reusable-code.yml` directly invokes `scripts/pre-code.sh` in the inline `Validate inputs` step, but `fullsend run code` will also execute `scripts/pre-code.sh` because `internal/scaffold/fullsend-repo/harness/code.yaml` defines it as `pre_script`. This results in the pre-script being executed twice in a single workflow run.
## Issue Context
The Fullsend composite action runs `fullsend run <agent>` (see `action.yml`), and `fullsend run` executes `pre_script` when present (per harness config). The compliance requirement for #4718 specifically disallows redundant direct invocation from the reusable workflow.
## Fix Focus Areas
- .github/workflows/reusable-code.yml[156-166]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| FIX_SKIP_TOOL_INSTALL: "true" | ||
| FULLSEND_DIR: ${{ inputs.install_mode == 'per-repo' && '.fullsend' || '' }} | ||
| run: bash "${FULLSEND_DIR:+$FULLSEND_DIR/}scripts/pre-fix.sh" |
There was a problem hiding this comment.
2. pre-fix.sh runs twice 📎 Requirement gap ☼ Reliability
The reusable fix workflow still runs scripts/pre-fix.sh inline in Validate inputs, and then runs fullsend run fix, which executes the same script again via harness/fix.yaml pre_script. This violates the requirement to avoid double-running the pre-script and can cause duplicated work/side effects.
Agent Prompt
## Issue description
`.github/workflows/reusable-fix.yml` directly invokes `scripts/pre-fix.sh` in the inline `Validate inputs` step, but `fullsend run fix` will also execute `scripts/pre-fix.sh` because `internal/scaffold/fullsend-repo/harness/fix.yaml` defines it as `pre_script`. This causes double execution of the same pre-script.
## Issue Context
The Fullsend composite action runs `fullsend run <agent>` (see `action.yml`). The harness definition for `fix` includes `pre_script: scripts/pre-fix.sh`, so the pre-script will run during `fullsend run` even if it already ran earlier in the workflow.
## Fix Focus Areas
- .github/workflows/reusable-fix.yml[369-379]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| TRIGGER_SOURCE: ${{ inputs.trigger_source }} | ||
| FIX_ITERATION: ${{ steps.context.outputs.iteration }} | ||
| HUMAN_INSTRUCTION: ${{ steps.context.outputs.instruction }} | ||
| FIX_SKIP_TOOL_INSTALL: "true" | ||
| FULLSEND_DIR: ${{ inputs.install_mode == 'per-repo' && '.fullsend' || '' }} | ||
| run: bash "${FULLSEND_DIR:+$FULLSEND_DIR/}scripts/pre-fix.sh" |
There was a problem hiding this comment.
3. Unused tool-skip flag 🐞 Bug ➹ Performance
The reusable fix workflow sets FIX_SKIP_TOOL_INSTALL=true before running scripts/pre-fix.sh, but pre-fix.sh never reads that variable and always executes the pre-commit tool auto-install section. Since harness/fix.yaml also declares scripts/pre-fix.sh as pre_script, this can lead to redundant tool installation and longer/flakier runs.
Agent Prompt
### Issue description
`reusable-fix.yml` sets `FIX_SKIP_TOOL_INSTALL=true` for the inline `Validate inputs` call to `scripts/pre-fix.sh`, but `pre-fix.sh` does not check this flag and always runs the pre-commit tool auto-install block. This makes the “lightweight” validation step still perform heavy work and can duplicate work when the harness `pre_script` also runs.
### Issue Context
- The workflow explicitly exports `FIX_SKIP_TOOL_INSTALL` in the validate step.
- The shipped `pre-fix.sh` documents optional vars but not `FIX_SKIP_TOOL_INSTALL`, and contains no conditional around the auto-install section.
- The harness config also uses `scripts/pre-fix.sh` as `pre_script`, so tool installation can happen more than once per job.
### Fix Focus Areas
- internal/scaffold/fullsend-repo/scripts/pre-fix.sh[13-18]
- internal/scaffold/fullsend-repo/scripts/pre-fix.sh[103-166]
- internal/scaffold/fullsend-repo/harness/fix.yaml[22-27]
### Suggested change
- Add `FIX_SKIP_TOOL_INSTALL` to the documented optional env vars.
- Guard the auto-install section with something like:
- if `FIX_SKIP_TOOL_INSTALL == "true"`, emit a `::notice::` and skip the entire install block (still allow the earlier validation/iteration-cap checks to run).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| REPO_FULL_NAME: ${{ inputs.source_repo }} | ||
| GITHUB_ISSUE_URL: ${{ fromJSON(inputs.event_payload).issue.html_url }} | ||
| COMMENT_BODY: ${{ fromJSON(inputs.event_payload).comment.body }} | ||
| GH_TOKEN: ${{ steps.app-token.outputs.token }} | ||
| FULLSEND_DIR: ${{ inputs.install_mode == 'per-repo' && '.fullsend' || '' }} | ||
| run: bash "${FULLSEND_DIR:+$FULLSEND_DIR/}scripts/pre-code.sh" |
There was a problem hiding this comment.
4. Silent gh failures 🐞 Bug ☼ Reliability
pre-code.sh discards stderr and ignores failures from gh pr list, so even with GH_TOKEN now provided by reusable-code.yml, API/auth/rate-limit errors can silently turn the existing-PR check into a no-op and allow duplicate code-agent runs without any diagnostic signal.
Agent Prompt
### Issue description
`pre-code.sh` uses `gh pr list ... 2>/dev/null || true`, which suppresses both errors and exit status. If GitHub API calls fail (auth/scopes/rate limiting), the script treats the result as “no PRs found” and continues, undermining the duplicate-PR prevention the workflow now depends on.
### Issue Context
This PR wires `GH_TOKEN` into the inline `Validate inputs` step, making the existing-PR check path active. But the script still hides failures, making this logic brittle and hard to debug when the check doesn’t work.
### Fix Focus Areas
- internal/scaffold/fullsend-repo/scripts/pre-code.sh[75-85]
- internal/scaffold/fullsend-repo/scripts/pre-code.sh[122-124]
### Suggested change
- Capture `gh pr list` exit status and stderr.
- On failure, emit a `::warning::` (include a brief reason) and default to `skipped=false` (fail-open), rather than silently proceeding as if there are no PRs.
- Avoid redirecting stderr to `/dev/null` for this call (or only suppress known-noise while preserving actionable errors).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review — fix(#4718): wire GH_TOKEN and re-add fail-fast validation for pre-code/pre-fixVerdict: approve SummaryTwo-line change to CI workflow files that (1) fixes a silently broken existing-PR check in the code agent workflow and (2) forward-wires a skip flag for the fix agent's tool auto-install step. Analysis
Correct and needed. The token is used for
The env var is set to Security: The Scope & intent: The PR is tightly scoped to issue #4718 (deduplicate pre-script calls). Changes are limited to the two workflow files identified in the issue body. The companion PR relationship is clearly documented. The Findings
Labels: PR modifies CI workflow files for code and fix agents |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| TRIGGER_SOURCE: ${{ inputs.trigger_source }} | ||
| FIX_ITERATION: ${{ steps.context.outputs.iteration }} | ||
| HUMAN_INSTRUCTION: ${{ steps.context.outputs.instruction }} | ||
| FIX_SKIP_TOOL_INSTALL: "true" |
There was a problem hiding this comment.
[low] mechanism-readiness
FIX_SKIP_TOOL_INSTALL=true is a no-op — pre-fix.sh does not check this env var. The tool auto-install section (lines 103–166) runs unconditionally. Harmless today; the companion PR (fullsend-ai/agents#175) is expected to add script-side support.
fullsend-ai-review
Bot
added
requires-manual-review
Records the pattern introduced in #5013/fullsend-ai/agents#175 so the next agent that needs to gate expensive workflow setup behind a fast pre-check (per rh-hemartin's request for consistency across agents, one: an {AGENT}_SKIP_{THING} env var, set on whichever of the two pre-script call sites should skip the redundant/expensive part. Distinguishes this from ADR 0049's agent-configuration env vars — same {AGENT}_{SETTING} syntax, but an internal invocation-context signal rather than a user-facing behavioral knob, so it belongs in script/harness comments, not docs/agents/<agent>.md. Scopes the decision to agents that actually need workflow-level gating (code, fix today) rather than mandating it retroactively for agents that don't currently duplicate pre-script execution. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
We need to decrease the differences between the reusable* workflows if we ever want to be able to port the agents to use the generic harness triggers, this is a step in the wrong direction.
Records the pattern introduced in #5013/fullsend-ai/agents#175 so the next agent that needs to gate expensive workflow setup behind a fast pre-check (per rh-hemartin's request for consistency across agents, one: an {AGENT}_SKIP_{THING} env var, set on whichever of the two pre-script call sites should skip the redundant/expensive part. Distinguishes this from ADR 0049's agent-configuration env vars — same {AGENT}_{SETTING} syntax, but an internal invocation-context signal rather than a user-facing behavioral knob, so it belongs in script/harness comments, not docs/agents/<agent>.md. Scopes the decision to agents that actually need workflow-level gating (code, fix today) rather than mandating it retroactively for agents that don't currently duplicate pre-script execution. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The skip-flag convention this ADR originally recorded was withdrawn after review (#5013 and agents#175 closed as superseded). Rewrite the ADR — still unmerged, so immutability does not apply — to record the decision that replaced it: pre-scripts run exactly once, inside fullsend run, signalling skips via the pre-script output protocol shipped in #5737, with the field-level contract in docs/normative/prescript-output/v1. Addresses the review feedback on this PR: options are now one paragraph each with the decision and its reasons in the Decision section; the gating no longer lives in GH workflow YAML at all; and no variables pass between invocations, dissolving the portability concern rather than specifying a mechanism for it. The non-template References section is folded into inline links. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
Closing as superseded, per the agreed direction on #4718: the skip-flag approach was withdrawn in favor of the pre-script output protocol, which shipped as #5737 (protocol in |
|
🤖 Finished Retro · ✅ Success · Started 7:26 PM UTC · Completed 7:36 PM UTC |
Retro: PR #5013 — wire GH_TOKEN and re-add fail-fast validation for pre-code/pre-fixWhat happenedPR #5013 was a human-authored 2-line change by @waynesun09 to fix issue #4718 (redundant pre-script execution). It was the second attempt (after closed PR #4762) and a companion to Timeline:
Findings — all covered by existing issues1. Review agent approved a PR that a human rejected on architectural grounds. The review agent correctly assessed code-level correctness, security, and the 2. Code agent wasted 16 min on unpushable workflow file changes. The code agent modified 3. Triage agent labeled What went well
No new proposals — all identified improvements are tracked by existing issues. |
3 participants
Summary
Companion PR to fullsend-ai/agents#175. That PR adds
CODE_SKIP_EXISTING_PR_CHECKandFIX_SKIP_TOOL_INSTALLflags topre-code.sh/pre-fix.shandharness/code.yamlso the harnesspre_scriptinvocation (insidefullsend run) can skip the work these workflows' inline pre-run steps already did, instead of repeating it. This PR is the fullsend-ai/fullsend half — the two only work together.Related Issue
Fixes #4718
Changes
.github/workflows/reusable-code.yml— passGH_TOKENto the inlineValidate inputsstep. It never had one, sopre-code.sh's existing-human-PR check (GH API search,pr-openlabel, skip comment) was silently a no-op there — the harness invocation was the only place it ever ran. OnceCODE_SKIP_EXISTING_PR_CHECKis set on that harness invocation (fix(#4718): stop running pre-code/pre-fix scripts twice per run agents#175), the check needs to actually work on this side, or the feature runs nowhere at all. (Caught byfullsend-ai-review[bot]on the original fix(#4718): stop running pre-code/pre-fix scripts twice per run #4762.).github/workflows/reusable-fix.yml— re-add a lightweight inlineValidate inputsstep withFIX_SKIP_TOOL_INSTALL=true, soPR_NUMBER/instruction/iteration-cap validation still fails fast before GCP/agent-env setup, without re-running the pre-commit tool auto-install that only the harness invocation needs.Testing
make lintpasses (staged changes)python3 -c "import yaml; yaml.safe_load(...)")History
This supersedes #4762, which edited
internal/scaffold/fullsend-repo/{harness,scripts}/*directly — those moved to fullsend-ai/agents and no longer affect runtime behavior (per discussion on #4762 with @rh-hemartin). This PR carries forward only the parts that still belong in this repo.