fix: fall back to workspace scripts dir for script companions - #41
Conversation
Companion scripts (resolve-precommit-tools.py, install-precommit-tools.sh, process-fix-result.py) are located via SCRIPT_DIR-relative sibling lookups. That works when scripts are resolved from a local scaffold checkout, but silently fails here: this repo's post-code.sh/post-fix.sh/ pre-code.sh/pre-fix.sh are fetched by fullsend as single files from a URL base, and the harness fetch pipeline has no sibling-file mechanism for scripts (only for skills) — so those companion scripts were never present next to the fetched pre/post scripts in this repo. Effect: pre-commit tool auto-install silently skipped (surfaces downstream as a confusing "Executable X not found" pre-commit failure), and fix-agent PR summary comments never posted. Requires fullsend-ai/fullsend#3393 to be merged and released first — this PR calls `fullsend postrun precommit-install` / `fullsend postrun fix-summary`, which do not exist in any fullsend release yet. Fixes fullsend-ai/fullsend#3069 Fixes fullsend-ai/fullsend#3070 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:54 PM UTC · Completed 6:03 PM UTC |
Review — approvePR: #41 — Clean, well-scoped fix. The companion scripts ( The approach is the correct one: the existing Re-review note: The prior review (SHA Observations1.
|
| Severity | low |
| Category | GHA-workflow-command-injection |
| Files | scripts/post-code.sh, scripts/post-fix.sh, scripts/pre-code.sh, scripts/pre-fix.sh |
The ::warning::Expected ${RESOLVE_SCRIPT} and ${INSTALL_SCRIPT} lines interpolate path variables into GHA workflow commands without sanitizing for :: sequences or %0A/%0D URL-encoded newlines. These paths are derived from ${GITHUB_WORKSPACE} (runner-controlled infrastructure, not attacker-controllable) concatenated with hardcoded path segments, so the practical risk is negligible. This is an established pattern in the codebase — post-code.sh:54, post-fix.sh:71,337 already emit ::error:: with unsanitized path variables. A separate sanitization pattern exists at post-code.sh:419-423 for untrusted git output, but that addresses a meaningfully different threat profile (external process output vs. infrastructure-set paths). No action required unless the team decides to remediate all call sites together.
2. Fallback block duplicated across four scripts
| Severity | low |
| Category | code-duplication |
| Files | scripts/post-code.sh, scripts/post-fix.sh, scripts/pre-code.sh, scripts/pre-fix.sh |
The 20-line fallback-and-warning block is repeated identically across all four scripts (with minor variation: post-scripts check .pre-commit-config.yaml in CWD, pre-scripts check ${TARGET_REPO}/.pre-commit-config.yaml). This follows the established codebase convention — the pre-commit tool installation block is already duplicated across these four files with explicit # SYNC: comments acknowledging the intentional parallelism. Extracting a shared helper could improve maintainability but would be a style change inconsistent with how the rest of the scripts are structured.
Dimensions reviewed
| Dimension | Result |
|---|---|
| Correctness | ✅ Fallback logic is correct; edge cases handled; existing guards preserved |
| Security | ✅ 1 low (unsanitized paths in workflow commands — pre-existing pattern) |
| Intent & coherence | ✅ Scope matches authorized work; approach mirrors merged fullsend#3393 |
| Style & conventions | ✅ 1 low (duplication follows established codebase pattern) |
| Documentation | ✅ No stale references |
| Cross-repo contracts | ⏭ Skipped — no exported interfaces modified |
📋 Review by fullsend — PR #41 · head 8e5ab3c
Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.Protected files in this PR:
scripts/post-code.shscripts/post-fix.shscripts/pre-code.shscripts/pre-fix.sh
Labels: PR modifies CI/CD pipeline scripts for pre-commit tool dependency resolution
Previous run
Review — comment
PR: #41 — fix: call fullsend postrun for pre-commit tools and fix summaries
Author: waynesun09 · Draft: yes · Base: main · Files: 4 · Δ: +24 / −86
Clean simplification — replaces dead sibling-script lookups (resolve-precommit-tools.py, install-precommit-tools.sh, process-fix-result.py) with fullsend postrun subcommands that ship inside the already-present fullsend binary. Net deletion of 62 lines. Scope is tight — only the 4 pre/post scripts are touched, and the PR body clearly documents the dependency on fullsend-ai/fullsend#3393.
One medium finding worth addressing before this leaves draft:
Findings
1. fullsend postrun precommit-install runs unguarded under set -euo pipefail — soft-failure → hard-failure regression
| Severity | medium |
| Category | error-handling |
| Files | scripts/post-code.sh, scripts/post-fix.sh, scripts/pre-code.sh, scripts/pre-fix.sh |
The old code wrapped the tool-resolution step in an explicit guard:
if python3 "${RESOLVE_SCRIPT}" ... > "${MANIFEST}"; then
...
else
echo "::warning::Pre-commit tool resolution failed — continuing without auto-install"
fiA failure emitted a GitHub Actions warning and the script continued to the authoritative pre-commit check, push, and PR-creation steps.
The new code runs the replacement bare:
fullsend postrun precommit-install "${PRECOMMIT_INSTALL_ARGS[@]}"Under set -e, any non-zero exit from this command terminates the entire script. In the post-scripts this blocks branch push and PR creation; in the pre-scripts it prevents the agent from running at all. Pre-commit tool installation is a best-effort optimization — a transient failure (network timeout, binary version mismatch, unexpected input) should not be fatal.
Other fullsend CLI calls in this codebase follow the exit-code-capture pattern (e.g., fullsend post-review ... || POST_REVIEW_EXIT=$? in post-review.sh:314). The fullsend postrun fix-summary call in this same PR correctly preserves the pattern.
Remediation: Guard the call in all four scripts, e.g.:
fullsend postrun precommit-install "${PRECOMMIT_INSTALL_ARGS[@]}" \
|| echo "::warning::fullsend postrun precommit-install failed — continuing without auto-install"Alternatively, if fullsend postrun precommit-install is designed to always exit 0 (handling errors internally), add a comment documenting that contract so future maintainers know the bare call is intentional.
2. Token passed as CLI argument (--token) — visible in process table
| Severity | low |
| Category | secrets-handling |
| File | scripts/post-fix.sh |
The new fix-summary invocation passes the token on the command line:
fullsend postrun fix-summary --result "${RESULT_FILE}" --repo "${REPO_FULL_NAME}" \
--pr "${PR_NUMBER}" --token "${GH_TOKEN}"This exposes the token in /proc/<pid>/cmdline (readable by same-UID processes). The old python3 call inherited GH_TOKEN from the environment, keeping it out of the process argument list.
Mitigating factors: This is an established pattern in the codebase — post-review.sh, post-triage.sh, and post-prioritize.sh all pass tokens via --token. The token is already masked via ::add-mask:: (line 83). The practical risk on an ephemeral single-user CI runner is low. No action required unless the team decides to remediate all call sites together.
Dimensions reviewed
| Dimension | Result |
|---|---|
| Correctness | 1 medium (error-handling regression) |
| Security | 1 low (token on CLI — established pattern) |
| Intent & coherence | ✅ Scope matches authorized work; architectural direction is sound |
| Style & conventions | ✅ Consistent with existing patterns |
| Documentation | ✅ No stale references found |
| Cross-repo contracts | ✅ Internal scripts, not public API — temporal dependency on fullsend#3393 is acknowledged via draft status |
📋 Review by fullsend — PR #41 · head 7d82582
fullsend-ai-review
Bot
added
the
requires-manual-review
fullsend-ai/fullsend#3393 dropped the `fullsend postrun` subcommand approach (per review feedback there) in favor of a workspace-directory fallback for companion-script lookup, so the previous commit on this branch now calls subcommands that were never shipped and never will be. Rework to mirror 3393's actual fix instead. post-code.sh, post-fix.sh, pre-code.sh, pre-fix.sh: after the BASH_SOURCE-relative lookup for resolve-precommit-tools.py / install-precommit-tools.sh / process-fix-result.py misses (it always does here, since those companions were never migrated into this repo during the ADR 0058 extraction), fall back to ${GITHUB_WORKSPACE}/scripts/ then ${GITHUB_WORKSPACE}/.fullsend/scripts/ — the path the reusable workflow's "Prepare workspace" step materializes fullsend's own scaffold scripts/ directory at on every job. Warn instead of silently skipping when the companions are missing everywhere. Fixes fullsend-ai/fullsend#3069 Fixes fullsend-ai/fullsend#3070 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
changed the title
PR Summary by QodoFix companion script lookup via GITHUB_WORKSPACE fallback
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Review · ❌ Terminated · Started 2:58 PM UTC · Ended 3:09 PM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Protected scripts/ files modified
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Fallback: these companion scripts were never migrated into this repo | ||
| # during the ADR 0058 extraction, so the BASH_SOURCE-relative lookup above | ||
| # always misses. The reusable workflow's "Prepare workspace" step always | ||
| # materializes the full scripts/ directory (from fullsend's own scaffold) | ||
| # at ${GITHUB_WORKSPACE}/scripts/ (per-org) or ${GITHUB_WORKSPACE}/.fullsend/scripts/ | ||
| # (per-repo). Try those paths when the BASH_SOURCE-relative lookup misses. | ||
| if [ ! -f "${RESOLVE_SCRIPT}" ] || [ ! -f "${INSTALL_SCRIPT}" ]; then | ||
| for _ws_candidate in "${GITHUB_WORKSPACE:-}/scripts" "${GITHUB_WORKSPACE:-}/.fullsend/scripts"; do | ||
| if [ -f "${_ws_candidate}/resolve-precommit-tools.py" ] \ | ||
| && [ -f "${_ws_candidate}/install-precommit-tools.sh" ]; then | ||
| RESOLVE_SCRIPT="${_ws_candidate}/resolve-precommit-tools.py" | ||
| INSTALL_SCRIPT="${_ws_candidate}/install-precommit-tools.sh" | ||
| break | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| # Warn instead of silently skipping when the repo needs the auto-install but | ||
| # the companions are missing everywhere — a silent skip here surfaces later | ||
| # as a confusing "Executable X not found" pre-commit failure. | ||
| if [ -f .pre-commit-config.yaml ] \ | ||
| && { [ ! -f "${RESOLVE_SCRIPT}" ] || [ ! -f "${INSTALL_SCRIPT}" ]; }; then | ||
| echo "::warning::Pre-commit tool auto-install skipped: companion scripts not found" | ||
| echo "::warning::Expected ${RESOLVE_SCRIPT} and ${INSTALL_SCRIPT}" | ||
| echo "::warning::Pre-commit hooks requiring system tools (e.g. lychee) may fail" | ||
| fi | ||
|
|
There was a problem hiding this comment.
2. Protected scripts/ files modified 📜 Skill insight § Compliance
This PR modifies files under the protected governance/infrastructure path scripts/, which must not be auto-approved and requires explicit review handling per policy. Even with justification, this must be surfaced as a protected-path change.
| if [ ! -f "${RESOLVE_SCRIPT}" ] || [ ! -f "${INSTALL_SCRIPT}" ]; then | ||
| for _ws_candidate in "${GITHUB_WORKSPACE:-}/scripts" "${GITHUB_WORKSPACE:-}/.fullsend/scripts"; do | ||
| if [ -f "${_ws_candidate}/resolve-precommit-tools.py" ] \ | ||
| && [ -f "${_ws_candidate}/install-precommit-tools.sh" ]; then | ||
| RESOLVE_SCRIPT="${_ws_candidate}/resolve-precommit-tools.py" | ||
| INSTALL_SCRIPT="${_ws_candidate}/install-precommit-tools.sh" | ||
| break | ||
| fi | ||
| done |
There was a problem hiding this comment.
3. Fallback can probe /scripts 🐞 Bug ☼ Reliability
The fallback candidates are built as "${GITHUB_WORKSPACE:-}/scripts" and
"${GITHUB_WORKSPACE:-}/.fullsend/scripts"; if GITHUB_WORKSPACE is unset/empty this becomes the
absolute paths /scripts and /.fullsend/scripts, so the scripts may accidentally discover companions
outside the workspace. In post-code/post-fix this can result in running whatever is found there via
python3/bash under a security-sensitive token-enabled context.
Agent Prompt
### Issue description
The workspace fallback uses `${GITHUB_WORKSPACE:-}` which expands to an empty string when `GITHUB_WORKSPACE` is unset/empty, producing absolute fallback paths (`/scripts`, `/.fullsend/scripts`). If those exist, the scripts may pick up companion files from outside the intended workspace; if they don’t exist, it still causes confusing probing of root paths.
### Issue Context
These scripts later execute the resolved companions via `python3`/`bash`, so unintended discovery can matter even if it’s unlikely on GitHub-hosted runners.
### Fix Focus Areas
- scripts/post-code.sh[279-288]
- scripts/post-fix.sh[191-200]
- scripts/post-fix.sh[350-357]
- scripts/pre-code.sh[139-148]
- scripts/pre-fix.sh[119-128]
### Suggested fix
- Only attempt the workspace fallback when `GITHUB_WORKSPACE` is non-empty, e.g.:
- `if [ -n "${GITHUB_WORKSPACE:-}" ]; then ... fi`
- and use `${GITHUB_WORKSPACE}/scripts` (no `:-` inside the concatenation).
- Optionally also ensure the candidate is a directory (`[ -d "${_ws_candidate}" ]`) before checking files.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| if [ -f .pre-commit-config.yaml ] \ | ||
| && { [ ! -f "${RESOLVE_SCRIPT}" ] || [ ! -f "${INSTALL_SCRIPT}" ]; }; then | ||
| echo "::warning::Pre-commit tool auto-install skipped: companion scripts not found" | ||
| echo "::warning::Expected ${RESOLVE_SCRIPT} and ${INSTALL_SCRIPT}" |
There was a problem hiding this comment.
[low] GHA-workflow-command-injection
::warning:: messages interpolate ${RESOLVE_SCRIPT} and ${INSTALL_SCRIPT} without sanitizing for :: sequences or %0A/%0D. These paths derive from ${GITHUB_WORKSPACE} (runner-controlled) + hardcoded segments, so practical risk is negligible. Pre-existing pattern in codebase (post-code.sh:54, post-fix.sh:71,337).
Suggested fix: No immediate action required. If the team decides to remediate all call sites, apply the sanitization pattern from post-code.sh:419-423 before interpolating paths into workflow commands.
| # Fallback: these companion scripts were never migrated into this repo | ||
| # during the ADR 0058 extraction, so the BASH_SOURCE-relative lookup above | ||
| # always misses. The reusable workflow's "Prepare workspace" step always | ||
| # materializes the full scripts/ directory (from fullsend's own scaffold) |
There was a problem hiding this comment.
[low] code-duplication
The 20-line fallback-and-warning block is repeated across all four scripts. This follows the established codebase convention — the pre-commit tool install block is already duplicated with explicit SYNC comments acknowledging the pattern.
Suggested fix: Consider extracting a shared helper function if the team decides to consolidate the existing duplication pattern across these scripts.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 2:58 PM UTC · Completed 3:09 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 3:43 PM UTC · Completed 3:51 PM UTC |
Retro: PR #41 — workspace fallback for script companionsTimeline
What went well
Review quality observations
Stale issues
Proposals filed
|
…yword - Extend GitLab closing-keyword regex to match comma-separated issue lists (e.g., "Closes #41, #42") by adding a repeating group for preceding issue refs separated by commas - Remove implement(?:s|ed|ing)? from keyword list — GitLab only recognizes close/fix/resolve variants as closing keywords - Update stale block comment describing the old search strategy - Update SKILL.md to replace stale gh pr list --search pattern with closedByPullRequestsReferences GraphQL query Addresses review feedback on #887
…yword - Extend GitLab closing-keyword regex to match comma-separated issue lists (e.g., "Closes #41, #42") by adding a repeating group for preceding issue refs separated by commas - Remove implement(?:s|ed|ing)? from keyword list — GitLab only recognizes close/fix/resolve variants as closing keywords - Update stale block comment describing the old search strategy - Update SKILL.md to replace stale gh pr list --search pattern with closedByPullRequestsReferences GraphQL query Addresses review feedback on #887
2 participants
Summary
post-code.sh,post-fix.sh,pre-code.sh, andpre-fix.shlocate companion scripts (resolve-precommit-tools.py,install-precommit-tools.sh,process-fix-result.py) via aSCRIPT_DIR-relative sibling lookup. Those companion scripts were never migrated into this repo during extraction, so the lookup has always silently failed here.Executable X not foundfailure at the authoritative pre-commit gate), and fix-agent PR summary comments never post.SCRIPT_DIR-relative lookup misses (it always will here), fall back to${GITHUB_WORKSPACE}/scripts/then${GITHUB_WORKSPACE}/.fullsend/scripts/— the path the reusable workflow's "Prepare workspace" step materializes fullsend's own scaffoldscripts/directory at on every job.Executable X not foundat the pre-commit gate.Supersedes this PR's original approach. The first commit on this branch called two
fullsend postrunsubcommands that don't exist in any shippedfullsendbinary — that CLI-surface approach was dropped in fullsend-ai/fullsend#3393 per review feedback there in favor of the workspace-fallback approach this PR now uses instead. No dependency on afullsendrelease remains.Related Issue
Fixes fullsend-ai/fullsend#3069
Fixes fullsend-ai/fullsend#3070
Changes
scripts/post-code.sh,scripts/pre-code.sh,scripts/pre-fix.sh: after theSCRIPT_DIR/TARGET_REPO-relative lookup forresolve-precommit-tools.py+install-precommit-tools.shmisses, fall back to${GITHUB_WORKSPACE}/scripts/then${GITHUB_WORKSPACE}/.fullsend/scripts/; warn if still missing.scripts/post-fix.sh: same fallback for the pre-commit auto-install block, plus the same fallback forprocess-fix-result.py.Testing
bash -nsyntax check on all four modified scriptsscripts/post-code-test.shandscripts/post-fix-test.shsuites pass unchanged${GITHUB_WORKSPACE}/scripts