Skip to content

fix(#4718): wire GH_TOKEN and re-add fail-fast validation for pre-code/pre-fix #5013

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.

Sign up for GitHub

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

Open
waynesun09 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(#4718): wire GH_TOKEN and re-add fail-fast validation for pre-code/pre-fix #5013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/reusable-code.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ jobs:
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"
Comment on lines +163 to 165

@qodo-code-review qodo-code-review Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Comment on lines 160 to 165

@qodo-code-review qodo-code-review Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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


Expand Down
1 change: 1 addition & 0 deletions .github/workflows/reusable-fix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ jobs:
TRIGGER_SOURCE: ${{ inputs.trigger_source }}
FIX_ITERATION: ${{ steps.context.outputs.iteration }}
HUMAN_INSTRUCTION: ${{ steps.context.outputs.instruction }}
FIX_SKIP_TOOL_INSTALL: "true"

@fullsend-ai-review fullsend-ai-review Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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_DIR: ${{ inputs.install_mode == 'per-repo' && '.fullsend' || '' }}
run: bash "${FULLSEND_DIR:+$FULLSEND_DIR/}scripts/pre-fix.sh"
Comment on lines +376 to 378

@qodo-code-review qodo-code-review Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Comment on lines 373 to 378

@qodo-code-review qodo-code-review Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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


Expand Down
Loading