Skip to content

fix(#197): add fallback path resolution for companion scripts in post-fix.sh#228

Open
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/197-post-script-companion-fallback
Open

fix(#197): add fallback path resolution for companion scripts in post-fix.sh#228
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/197-post-script-companion-fallback

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

When post-fix.sh runs from a content-addressed cache path (sha256//content), the BASH_SOURCE[0] dirname does not contain companion files like process-fix-result.py. This causes the structured output processing to be silently skipped.

Add a fallback: after computing SCRIPT_DIR from BASH_SOURCE[0], check if process-fix-result.py exists there. If not, fall back to ${FULLSEND_DIR:-.}/scripts where companion scripts are co-located in the standard workspace layout. The existing safety net (warning if PROCESS_SCRIPT still does not exist) remains unchanged.


Closes #197

Post-script verification

  • Branch is not main/master (agent/197-post-script-companion-fallback)
  • Secret scan passed (gitleaks — f96750babbed5ada406a9ae04e8068449701d9c7..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

…-fix.sh

When post-fix.sh runs from a content-addressed cache path
(sha256/<hash>/content), the BASH_SOURCE[0] dirname does not
contain companion files like process-fix-result.py. This causes
the structured output processing to be silently skipped.

Add a fallback: after computing SCRIPT_DIR from BASH_SOURCE[0],
check if process-fix-result.py exists there. If not, fall back
to ${FULLSEND_DIR:-.}/scripts where companion scripts are
co-located in the standard workspace layout. The existing
safety net (warning if PROCESS_SCRIPT still does not exist)
remains unchanged.

Closes #197
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review — approve

PR: #228
Title: fix(#197): add fallback path resolution for companion scripts in post-fix.sh

Summary

Small, well-scoped bug fix that adds a fallback path resolution for process-fix-result.py when post-fix.sh runs from a content-addressed cache path where companion files are not co-located. The fix checks ${FULLSEND_DIR:-.}/scripts as a secondary location when the primary BASH_SOURCE[0]-relative path fails.

Analysis

Correctness ✅ — The fallback logic is safe. All edge cases degrade gracefully:

  • When FULLSEND_DIR is unset, the :-. default resolves to ./scripts. This is a weak fallback (relative to REPO_DIR, not the fullsend workspace), but the existing guard at line 322 ([ ! -f "${PROCESS_SCRIPT}" ]) catches the case and emits a warning. No crash or incorrect behavior.
  • When FULLSEND_DIR is set but ${FULLSEND_DIR}/scripts doesn't exist, the -d check prevents the fallback from activating, and the original path is preserved.
  • When the fallback directory exists but process-fix-result.py is absent, the existing safety net still catches it.
  • In the primary CI path, FULLSEND_DIR is always set via action.yml (line 361), so the default is rarely exercised.

Security ✅ — No security concerns. FULLSEND_DIR is set by the GitHub Actions runner (via action.yml input), not by the sandbox or agent. The sandbox is destroyed before the post-script runs. The process-fix-result.py at the fallback path is part of the trusted workspace, not agent-controlled. No injection patterns, Unicode steganography, or bidirectional overrides in the diff.

Intent & Coherence ✅ — The PR correctly traces to issue #197 and implements Option A (minimal post-script fallback). While issue #197 mentions both post-code.sh and a "pre-commit tools" pattern in post-fix.sh, verification shows: (1) post-code.sh has no SCRIPT_DIR-based companion resolution pattern, so it does not need this fix; (2) post-fix.sh's pre-commit handling (section 5) uses pip install and pre-commit run directly without companion script resolution. The only companion resolution in post-fix.sh is for process-fix-result.py, which this PR correctly addresses.

Style ✅ — The added code matches existing conventions: comment style, conditional formatting ([ ] with -f/-d), UPPER_SNAKE_CASE variable naming, and ${VAR:-default} parameter expansion patterns.

Findings (low-severity, non-blocking)

  1. FULLSEND_DIR undocumented in file header — The script now relies on FULLSEND_DIR for fallback resolution, but it is not listed in the "Optional environment variables" section (lines 39-42). Adding it would help operators understand the fallback behavior.

  2. Weak default when FULLSEND_DIR is unset — The ${FULLSEND_DIR:-.} default resolves to . (current working directory), which after the cd "${REPO_DIR}" at line 95 would point inside the target repo — unlikely to contain the companion script. The existing guard prevents any harm, but the fallback is ineffective in this edge case. In CI, FULLSEND_DIR is always set, so this is narrow.


Labels: PR modifies post-fix dispatch script companion resolution.

Previous run

Review — comment

PR: fix(#197): add fallback path resolution for companion scripts in post-fix.sh
Scope: 1 file changed, 7 additions — internal/scaffold/fullsend-repo/scripts/post-fix.sh

Summary

The PR adds a fallback path resolution for process-fix-result.py when post-fix.sh runs from a content-addressed cache path (sha256/<hash>/content) where companion files aren't co-located. The fallback checks ${FULLSEND_DIR:-.}/scripts after the primary BASH_SOURCE-derived path fails. The core logic is sound and the existing safety net at line 322 (elif [ ! -f "${PROCESS_SCRIPT}" ]) continues to handle the case where neither location has the companion script.

Findings

1. ${FULLSEND_DIR:-.} default resolves to target repo — defense-in-depth concern · medium · correctness, security

File: internal/scaffold/fullsend-repo/scripts/post-fix.sh · Line: ~308

The :-. default means that when FULLSEND_DIR is unset, the fallback resolves to ./scripts/process-fix-result.py relative to the current working directory. By line 95 (cd "${REPO_DIR}"), cwd is the target repository. If the target repo happens to have a scripts/process-fix-result.py, the fallback silently picks it up and executes it with GH_TOKEN=PUSH_TOKEN.

Mitigations already in place:

  • The protected-path check (line 164) blocks agent modifications to scripts/ in the target repo
  • FULLSEND_DIR is always set via action.yml in CI (so the . default rarely triggers)
  • The post-script runs after sandbox destruction, so the agent can't create files at runtime

Remediation: Remove the . default so the fallback only activates when FULLSEND_DIR is explicitly set:

if [ ! -f "${PROCESS_SCRIPT}" ] && [ -n "${FULLSEND_DIR:-}" ] && [ -d "${FULLSEND_DIR}/scripts" ]; then
  SCRIPT_DIR="${FULLSEND_DIR}/scripts"
  PROCESS_SCRIPT="${SCRIPT_DIR}/process-fix-result.py"
fi

2. post-prioritize.sh has same companion resolution pattern · medium · incomplete-fix

File: internal/scaffold/fullsend-repo/scripts/post-prioritize.sh · Line: ~15-17

post-prioritize.sh uses the identical SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" pattern to locate lib/github-api-csma.sh. Under the same content-addressed cache scenario, this will fail with a hard error (set -euo pipefail + source on a missing file) rather than the graceful degradation post-fix.sh provides.

Remediation: Apply the same fallback pattern to post-prioritize.sh, or document why it's not affected by the cache scenario. This could be a follow-up issue.

3. FULLSEND_DIR not documented in script header · medium · stale-documentation

File: internal/scaffold/fullsend-repo/scripts/post-fix.sh · Line: 38-41

The script header lists required and optional environment variables (lines 32-41). The new fallback introduces a dependency on FULLSEND_DIR, but it's not listed in the "Optional environment variables" section.

Remediation: Add to the optional env vars section:

#   FULLSEND_DIR      — fullsend directory root (fallback for companion script resolution)

4. No test coverage for post-fix.sh · low · test-adequacy

File: internal/scaffold/fullsend-repo/scripts/post-fix.sh

Sibling scripts have test files (post-code-test.sh, post-triage-test.sh, post-review-test.sh, post-prioritize-test.sh), but post-fix.sh has none. The new fallback logic is untested.

5. post-code.sh does not need the same fix · low · scope-clarification

File: internal/scaffold/fullsend-repo/scripts/post-code.sh

Issue #197 mentions both post-code.sh and post-fix.sh, but post-code.sh doesn't use BASH_SOURCE-relative companion script resolution — its tool dependencies (gitleaks, lychee, uv, pre-commit) are installed to ~/.local/bin via PATH. The PR correctly targets only post-fix.sh.

6. Minor comment spacing inconsistency · low · style

File: internal/scaffold/fullsend-repo/scripts/post-fix.sh · Line: ~304

The multi-line explanatory comment block doesn't follow the file's convention of having a blank separator line before multi-line comments that explain following code blocks.


Labels: PR fixes companion script resolution in the dispatch post-script pipeline

@guyoron1

guyoron1 commented Jul 8, 2026

Copy link
Copy Markdown
Owner

/fs-review

# Locate process-fix-result.py relative to this script.
# When this script runs from a content-addressed cache path
# (sha256/<hash>/content), companion files are not co-located.
# Fall back to ${FULLSEND_DIR}/scripts if the companion is missing.

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] docs-currency

FULLSEND_DIR is now used for fallback companion script resolution but is not listed in the Optional environment variables section of the file header (lines 39-42). Adding it would help operators understand the fallback behavior.

Suggested fix: Add FULLSEND_DIR to the Optional environment variables comment block with a description like: FULLSEND_DIR — path to fullsend workspace directory for companion script fallback resolution (default: current directory).

# Locate process-fix-result.py relative to this script.
# When this script runs from a content-addressed cache path
# (sha256/<hash>/content), companion files are not co-located.
# Fall back to ${FULLSEND_DIR}/scripts if the companion is missing.

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] edge-case

The ${FULLSEND_DIR:-.} default resolves to . (current working directory after cd into REPO_DIR), which is unlikely to contain the companion script. The fallback is ineffective when FULLSEND_DIR is unset, though the existing safety net at line 322 prevents any harm. In CI, FULLSEND_DIR is always set via action.yml.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/dispatch Workflow dispatch and triggers labels Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/dispatch Workflow dispatch and triggers ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Post-script companion scripts not accessible when resolved from URL-based cache

1 participant