Skip to content

fix(#235): add fallback path resolution for companion scripts in post-scripts#273

Open
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/235-companion-fallback-resolution
Open

fix(#235): add fallback path resolution for companion scripts in post-scripts#273
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/235-companion-fallback-resolution

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

When post-fix.sh or post-prioritize.sh runs from a content-addressed cache path (sha256//content), companion files (process-fix-result.py, lib/github-api-csma.sh) are not co-located with the script. The SCRIPT_DIR computed from BASH_SOURCE[0] points to the cache directory where companions do not exist.

Add a fallback: after computing SCRIPT_DIR, check if the companion file exists at that path. If not, fall back to ${FULLSEND_DIR:-.}/scripts where companions are co-located in the standard workspace layout.

Scripts fixed:

  • post-fix.sh: process-fix-result.py resolution
  • post-prioritize.sh: lib/github-api-csma.sh resolution

Closes #235

Post-script verification

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

…-scripts

When post-fix.sh or post-prioritize.sh runs from a
content-addressed cache path (sha256/<hash>/content), companion
files (process-fix-result.py, lib/github-api-csma.sh) are not
co-located with the script. The SCRIPT_DIR computed from
BASH_SOURCE[0] points to the cache directory where companions
do not exist.

Add a fallback: after computing SCRIPT_DIR, check if the
companion file exists at that path. If not, fall back to
${FULLSEND_DIR:-.}/scripts where companions are co-located in
the standard workspace layout.

Scripts fixed:
- post-fix.sh: process-fix-result.py resolution
- post-prioritize.sh: lib/github-api-csma.sh resolution

Closes #235
@github-actions

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 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 12:59 PM UTC · Completed 12:59 PM UTC
Commit: c776c24 · View workflow run →

@guyoron1

Copy link
Copy Markdown
Owner

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:57 PM UTC · Completed 4:09 PM UTC
Commit: d8e3df7 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review — comment

PR: fix(#235): add fallback path resolution for companion scripts in post-scripts
Verdict: Comment — medium-severity findings worth noting but none that should block

Summary

The fix correctly adds fallback path resolution for companion scripts in post-fix.sh and post-prioritize.sh when running from content-addressed cache paths. The core logic is sound:

  • The && short-circuit ensures ${FULLSEND_DIR:?...} only fires when the companion is genuinely missing at SCRIPT_DIR, so no regression in the normal co-located case.
  • The PR correctly identifies the two affected scripts. post-code.sh is not affected (it installs tools inline, no companion file resolution).
  • The fail-closed behavior (FULLSEND_DIR must be set) is a deliberate and defensible design choice.

Findings

Medium

[docs-currency] FULLSEND_DIR not documented in script headers

Both post-fix.sh (lines 31–42) and post-prioritize.sh (lines 7–11) have env var documentation headers listing required and optional variables. The new fallback introduces a conditional dependency on FULLSEND_DIR (hard-exit via :? when companion is missing and FULLSEND_DIR is unset), but FULLSEND_DIR is not listed in either header. Someone debugging a "FULLSEND_DIR must be set" failure would check the header and not find it.

Remediation: Add FULLSEND_DIR to the "Optional environment variables" section in both scripts, noting it is required when running from a content-addressed cache path.

Low

[correctness] post-prioritize.sh checks directory existence instead of file existence at fallback path

The fallback guard checks [ -d "${FULLSEND_DIR:?...}/scripts/lib" ] (directory exists) rather than [ -f "${FULLSEND_DIR:?...}/scripts/lib/github-api-csma.sh" ] (file exists). If the lib/ directory exists but github-api-csma.sh is missing, the fallback activates and the subsequent source fails. This is less precise than the file-existence check used in post-fix.sh. In practice, set -e catches the failure, but the guard should match what it's guarding.

Remediation: Change [ -d "...scripts/lib" ] to [ -f "...scripts/lib/github-api-csma.sh" ] in post-prioritize.sh.

[correctness] post-fix.sh logs fallback path even when companion not found there

The echo "Using fallback companion path: ${SCRIPT_DIR}" runs after SCRIPT_DIR is reassigned but before verifying the companion actually exists at the new path. If the fallback directory exists but lacks process-fix-result.py, the log is misleading. The existing line 322 check catches this gracefully, so it's cosmetic.

[security] GHA workflow command injection hardening on echo output

The echo "Using fallback companion path: ${SCRIPT_DIR}" interpolates SCRIPT_DIR (derived from FULLSEND_DIR). FULLSEND_DIR is infrastructure-controlled (set by action.yml from GITHUB_WORKSPACE) and not attacker-influenced in any current deployment path. However, for defense-in-depth, consider sanitizing :: sequences: echo "Using fallback companion path: ${SCRIPT_DIR//::/__}" in both files.

Verification notes

  • Short-circuit correctness: Verified that ${FULLSEND_DIR:?...} inside [ ... ] && [ ... ] only evaluates when the first condition is true (companion missing). Normal-path scripts never touch the expansion.
  • Scope completeness: Confirmed only post-fix.sh and post-prioritize.sh have companion file dependencies via SCRIPT_DIR. No other post-scripts are affected.
  • Test integrity: No test files were modified. post-prioritize-test.sh exists but tests the normal path where companions are co-located — the fallback is dead code in tests. No test weakening.
  • Security model: The fallback is consistent with post-fix.sh's security model. The trust boundary is unchanged — FULLSEND_DIR/scripts/ is the same trust domain as the original SCRIPT_DIR.
Previous run

Review of PR #273

Verdict: Approve

Summary

Clean, minimal bug fix that adds fallback path resolution to two post-scripts (post-fix.sh and post-prioritize.sh) for companion file lookup when running from the content-addressed cache. The change matches Option A from issue #235 and is correctly scoped — I verified that post-code.sh (mentioned in the issue body) does not have the SCRIPT_DIR companion-file pattern, so it doesn't need this fix.

The fallback logic is straightforward: check if the companion file exists at the BASH_SOURCE-derived path, and if not, try ${FULLSEND_DIR:-.}/scripts. Existing error handling in both scripts correctly handles the case where the companion is not found at either location (post-fix.sh line 322 prints a warning and skips; post-prioritize.sh aborts via set -euo pipefail).

Findings

1. ${FULLSEND_DIR:-.} defaults to cwd when unset — prefer fail-closed

Severity: Low  |  Category: fail-open

Files: post-fix.sh (line 308), post-prioritize.sh (line 18)

The ${FULLSEND_DIR:-.} pattern falls back to . (current working directory) when FULLSEND_DIR is unset. The post-script's cwd is runDir (the agent output directory, e.g., /tmp/fullsend/agent-fix-<id>/), not the workspace root. While FULLSEND_DIR is always set in the standard CI deployment path (action.yml line 369 defaults it to GITHUB_WORKSPACE), the :- default silently resolves to a potentially unintended location rather than failing. Using ${FULLSEND_DIR:?FULLSEND_DIR must be set} would make the fallback fail-closed with a clear error message, at no functional cost to the normal deployment path.

Practical risk is low: the sandboxed agent cannot write to the host's runDir, no existing code creates a scripts/ subdirectory there, and the [ -d ] check acts as a second gate.

2. Fallback trades content-addressed integrity for availability

Severity: Low  |  Category: integrity

File: post-fix.sh (line 308)

The content-addressed cache path (sha256/<hash>/content) guarantees file integrity via the hash. The fallback to ${FULLSEND_DIR}/scripts loses that guarantee — a different version of process-fix-result.py or github-api-csma.sh could be at the fallback path. This is an accepted trade-off (availability over integrity), but consider adding a log line when the fallback activates (e.g., echo "Using fallback companion path: ${SCRIPT_DIR}") to aid debugging.

Scope Verification

  • ✅ Linked to issue [RTK+Ponytail] Post-script companion scripts not accessible when resolved from URL-based cache #235 (type/bug, priority/high)
  • ✅ Fix prefix matches bug-fix tier
  • ✅ No scope creep — only the two scripts with companion-file dependencies are changed
  • ✅ No security-sensitive path changes (permissions, RBAC, secrets)
  • ✅ No exported API or cross-repo contract changes
  • ✅ No documentation staleness introduced
  • ✅ Existing error handling preserved and remains correct after the change

Labels: PR modifies post-script companion resolution in the dispatch/harness scaffold

fullsend-ai-review[bot]

This comment was marked as outdated.

@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 10, 2026
@guyoron1

Copy link
Copy Markdown
Owner

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 10, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 5:38 PM UTC · Completed 5:49 PM UTC
Commit: d8e3df7 · View workflow run →

- Replace ${FULLSEND_DIR:-.} with ${FULLSEND_DIR:?...} so the fallback
  fails with a clear error when FULLSEND_DIR is unset, rather than
  silently resolving to the current working directory.
- Add echo when fallback companion path activates, aiding debugging.

Addresses review feedback on #273
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed both low-severity review findings: (1) replaced fail-open ${FULLSEND_DIR:-.} with fail-closed ${FULLSEND_DIR:?...} in both scripts, (2) added fallback path log line in both scripts.

Fixed (3):

  1. ${FULLSEND_DIR:-.} defaults to cwd when unset — prefer fail-closed (internal/scaffold/fullsend-repo/scripts/post-fix.sh): Replaced ${FULLSEND_DIR:-.} with ${FULLSEND_DIR:?FULLSEND_DIR must be set} so the fallback fails with a clear error when FULLSEND_DIR is unset. Applied to both occurrences in the fallback block.
  2. ${FULLSEND_DIR:-.} defaults to cwd when unset — prefer fail-closed (internal/scaffold/fullsend-repo/scripts/post-prioritize.sh): Same fail-closed change applied to post-prioritize.sh fallback block.
  3. Fallback trades content-addressed integrity for availability (internal/scaffold/fullsend-repo/scripts/post-fix.sh): Added echo logging the fallback companion path when it activates, aiding debugging as suggested. Applied to both post-fix.sh and post-prioritize.sh for consistency.

Tests: passed

Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 5:51 PM UTC · Completed 5:59 PM UTC
Commit: d8e3df7 · View workflow run →

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.

[RTK+Ponytail] Post-script companion scripts not accessible when resolved from URL-based cache

1 participant