Skip to content

fix(scripts): guard GITHUB_WORKSPACE fallback against empty/unset values #67

Description

@fullsend-ai-retro

What happened

PR #41 (#41) added a workspace-directory fallback for companion script lookup in post-code.sh, post-fix.sh, pre-code.sh, and pre-fix.sh. The fallback iterates over ${GITHUB_WORKSPACE:-}/scripts and ${GITHUB_WORKSPACE:-}/.fullsend/scripts. When GITHUB_WORKSPACE is unset or empty, ${GITHUB_WORKSPACE:-} expands to an empty string, producing absolute paths /scripts and /.fullsend/scripts. The Qodo review bot flagged this as finding #3 ('Fallback can probe /scripts') on post-code.sh line 287. The fullsend review agent did not catch this — its observations focused on GHA workflow command sanitization and code duplication. The human reviewer approved silently without addressing the Qodo finding. The PR was merged with the bug present. The same pattern exists in the upstream fullsend repo via PR fullsend-ai/fullsend#3393.

What could go better

The fallback loop should only run when GITHUB_WORKSPACE is non-empty. On GitHub Actions runners, GITHUB_WORKSPACE is always set by the runner infrastructure, so the practical risk is low — the -f test on /scripts/resolve-precommit-tools.py is extremely unlikely to find anything. However, these scripts execute under set -euo pipefail in a security-sensitive context (push tokens are active), and defensively validating environment assumptions is standard practice for this codebase. Confidence: high that the fix is correct and low-risk. Uncertainty: whether the team considers this worth fixing given the low practical risk.

Proposed change

In all four scripts (scripts/post-code.sh, scripts/post-fix.sh, scripts/pre-code.sh, scripts/pre-fix.sh), wrap the workspace fallback loop in a non-empty guard:

if [ ! -f "${RESOLVE_SCRIPT}" ] || [ ! -f "${INSTALL_SCRIPT}" ]; then
  if [ -n "${GITHUB_WORKSPACE:-}" ]; then
    for _ws_candidate in "${GITHUB_WORKSPACE}/scripts" "${GITHUB_WORKSPACE}/.fullsend/scripts"; do
      # ... existing loop body ...
    done
  fi
fi

The same fix should be applied to the upstream scaffold copies in fullsend-ai/fullsend (internal/scaffold/fullsend-repo/scripts/). This change also applies to the process-fix-result.py fallback in post-fix.sh (lines 350-357).

Validation criteria

After the fix: (1) bash -n syntax check passes on all four scripts. (2) With GITHUB_WORKSPACE unset, the fallback loop does not execute and no paths under / are probed (verify with bash -x). (3) Existing scripts/post-code-test.sh and scripts/post-fix-test.sh test suites continue to pass.


Generated by retro agent from #41

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingready-to-codeTriggers code agent dispatch

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions