fix(#3069): fall back to workspace scripts dir for script companions#3393
Conversation
|
🤖 Finished Review · ✅ Success · Started 5:54 PM UTC · Completed 6:08 PM UTC |
PR Summary by QodoAdd
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Site previewPreview: https://7162f8e8-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
Context used✅ Tickets:
🎫 Post-script companion scripts not accessible when resolved from URL-based cache 🎫 Post-script pre-commit tool auto-install should warn when companion scripts are not found✅ Compliance rules (platform):
61 rules✅ Skills:
writing-user-docs, writing-adrs 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Review — approveThis PR fixes companion-script resolution in the four harness scripts ( Implementation pivotThe prior review (SHA AssessmentThe change is safe and well-scoped:
Low-severity observations (non-blocking)
VerdictApprove. The shell-only fallback is simpler than the prior Go CLI approach, introduces no new dependencies or release sequencing concerns, and correctly fixes the root cause. The low-severity observations are non-blocking style concerns suitable for a future refactoring pass. Reviewed by fullsend review agent · fullsend-ai/fullsend Previous runReview — approveThis PR ports three Python/shell companion scripts ( Changes since prior reviewThe fixup commit (
AssessmentThe change is well-crafted and safe to merge:
Low-severity observations (non-blocking)
VerdictApprove. The fixup commit strengthened the code across multiple dimensions (validation, forge interface, token fallback, GHA sanitization). The remaining observations are non-blocking, defense-in-depth improvements suitable for follow-up work. Reviewed by fullsend review agent · fullsend-ai/fullsend Previous runReview — approveThis PR ports three Python/shell companion scripts ( AssessmentThe change is well-crafted and safe to merge:
Notes
VerdictApprove. The PR fixes a real, high-priority bug with a clean architectural approach, comprehensive tests, and faithful behavioral parity. The notes above are non-blocking observations for future improvement. Reviewed by fullsend review agent · fullsend-ai/fullsend Labels: PR adds new CLI commands under internal/cli/ and modifies harness post-scripts under internal/scaffold/ Previous runReview — approveThis PR ports three Python/shell companion scripts ( Changes since prior reviewThe fixup commit (
AssessmentThe change is well-crafted and safe to merge:
Low-severity observations (non-blocking)
VerdictApprove. The fixup commit strengthened the code across multiple dimensions (validation, forge interface, token fallback, GHA sanitization). The remaining observations are non-blocking, defense-in-depth improvements suitable for follow-up work. Reviewed by fullsend review agent · fullsend-ai/fullsend Previous run (2)Review — approveThis PR ports three Python/shell companion scripts ( AssessmentThe change is well-crafted and safe to merge:
Notes
VerdictApprove. The PR fixes a real, high-priority bug with a clean architectural approach, comprehensive tests, and faithful behavioral parity. The notes above are non-blocking observations for future improvement. Reviewed by fullsend review agent · fullsend-ai/fullsend Labels: PR adds new CLI commands under internal/cli/ and modifies harness post-scripts under internal/scaffold/ |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
@waynesun09 I'm not sure I am on-page with the approach of converting all the sibling scripts into fullsend CLI sub-commands. It might make sense for a limited set of things but not across the board. We should have probably taken #3182 as a warning that we've taken a bad turn. I think we should try packaging the sets of scripts as dummy skills. That can probably work now. The only issue I see with it is that we then end up with the scripts inside the sandbox. I think we can live with that for now. The next step will be to probably allow applying the same directory fetching logic we have for skill for per/post scripts as well. |
|
🤖 Finished Review · ✅ Success · Started 6:30 PM UTC · Completed 6:41 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
@ifireball you're right, and digging into it further changed my mind on the approach entirely — not just the "single-use vs shared" split I was originally going to propose. Where this actually breaks: Why a CLI subcommand is the wrong fix: the reusable workflow ( So the real fix is much smaller than what I originally posted: add a fallback in Thanks for pushing on this — will update shortly. |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
LGTM. One note inline.
precommit-install is solid agent-agnostic infrastructure. fix-summary is more coupled to fix-result.json's schema though — action types, decision points, the full rendering. If we're keeping third-party agents decoupled from the core binary, could the fix agent render its own markdown and a generic "post comment from file" subcommand handle the rest?
| if len(body) > maxFixSummaryCommentLength { | ||
| truncationNotice := "\n\n*[truncated — output exceeded 32KB]*" | ||
| stepWarn(printer, fmt.Sprintf("comment body truncated from %d to %d chars", len(body), maxFixSummaryCommentLength)) | ||
| body = body[:maxFixSummaryCommentLength-len(truncationNotice)] + truncationNotice |
There was a problem hiding this comment.
[minor] body[:maxFixSummaryCommentLength-len(truncationNotice)] slices by byte offset — could split a multi-byte UTF-8 character if the cut lands mid-codepoint (the header's wrench emoji alone is 4 bytes). Non-blocking, but walking backward to a rune boundary would be cleaner.
pre-code.sh, pre-fix.sh, post-code.sh, and post-fix.sh locate their companion files (resolve-precommit-tools.py, install-precommit-tools.sh, process-fix-result.py) next to BASH_SOURCE. That breaks when the script itself is resolved from a URL base: the harness fetches scripts as single flat files into the content-addressed cache with no siblings, so the lookups silently miss and pre-commit tool auto-install (and the fix-agent summary comment) are skipped. The companions never needed to travel with the script: the reusable workflows' "Prepare workspace" step always materializes the full scripts/ directory (with L1 org overrides applied) at GITHUB_WORKSPACE/scripts (per-org) or GITHUB_WORKSPACE/.fullsend/scripts (per-repo). Fall back to those absolute paths when the BASH_SOURCE-relative lookup misses. GITHUB_WORKSPACE is used instead of FULLSEND_DIR because post-scripts run with cwd=runDir, where the per-repo relative FULLSEND_DIR value would mis-resolve. Executing the workspace copy also means resolve-precommit-tools.py finds the L1-overridden .pre-commit-tools.yaml next to itself, so org registry overrides keep working. Also warn instead of silently skipping when the companions are missing everywhere (#3070), so the failure no longer surfaces only as a confusing "Executable X not found" at the pre-commit gate. Fixes #3069 Fixes #3070 Assisted-by: Claude (fix and verification) Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
postrun-command
branch
from
cd9ff41 to
fa96e95
Compare
waynesun09
changed the title
|
🤖 Review · ❌ Terminated · Started 8:25 PM UTC · Ended 8:37 PM UTC |
|
Rework pushed — the PR is now the minimal workspace-fallback fix described above: +114/−1 across the four scaffold scripts, no Go changes, Two notes from re-verifying against #3069's triage:
@ralphbean your two comments on the Go files (sanitizer reuse, byte-offset truncation) are moot now — those files no longer exist in this PR. |
| # 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 |
There was a problem hiding this comment.
[low] naming-convention
Loop variable _ws_candidate uses underscore prefix, which matches the codebase convention for temporary variables (_BASE_BR, _TOKEN) but not the convention for loop variables (label, file, dir). Borderline — consistent with one convention, inconsistent with another.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 8:25 PM UTC · Completed 8:37 PM UTC |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
🤖 Finished Retro · ✅ Success · Started 2:18 PM UTC · Completed 2:27 PM UTC |
|
PR #3393 fixed companion-script resolution for pre/post-code/fix scripts by adding a workspace-directory fallback when BASH_SOURCE-relative lookups fail. The workflow involved a significant approach pivot: the initial submission was a Go CLI subcommand approach ( Proposals filed
|
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>
4 participants
Summary
pre-code.sh/pre-fix.sh/post-code.sh/post-fix.share resolved from a URL base (e.g.fullsend-ai/agents) instead of the local scaffold checkout.fullsend postrunsubcommand approach).Executable X not foundat the pre-commit gate.Related Issue
Fixes #3069
Fixes #3070
Root cause
The scripts locate their companions (
resolve-precommit-tools.py,install-precommit-tools.sh,process-fix-result.py) next toBASH_SOURCE[0]. That worked while scripts lived in the local scaffold checkout, but the harness fetches URL-based scripts as single flat files into the content-addressed cache (fetchBaseFileininternal/harness/compose.go) with no sibling-file concept — so oncepost-code.sh/post-fix.shmoved to URL resolution, the sibling lookups silently missed and the whole install step was skipped.The companions were never really "sibling files of an agent script": the reusable workflows' "Prepare workspace" step unconditionally materializes the full
scripts/directory (with L1 org overrides applied) at a known workspace path on every job —${GITHUB_WORKSPACE}/scripts/(per-org) or${GITHUB_WORKSPACE}/.fullsend/scripts/(per-repo). Only theBASH_SOURCE-derived guess at where that copy lives was broken.Changes
internal/scaffold/fullsend-repo/scripts/{pre-code,pre-fix,post-code,post-fix}.sh:BASH_SOURCE-relative lookup, fall back to${GITHUB_WORKSPACE}/scripts/then${GITHUB_WORKSPACE}/.fullsend/scripts/(5 lookup sites: 4 pre-commit-install sections +process-fix-result.pyinpost-fix.sh). This is issue Post-script companion scripts not accessible when resolved from URL-based cache #3069's Option A, with one correction:GITHUB_WORKSPACE(absolute, always in the post-script env —childScriptEnvlayersrunner_envover the full process environment) instead ofFULLSEND_DIR, whose per-repo value is the relative path.fullsendand would mis-resolve from the post-script's cwd (runDir).::warning::when.pre-commit-config.yamlexists but the companions are missing after both lookups (Post-script pre-commit tool auto-install should warn when companion scripts are not found #3070's proposed change).Because the fallback executes the workspace copy,
resolve-precommit-tools.pyfinds the L1-overridden.pre-commit-tools.yamlnext to itself (__file__), so org registry overrides keep working from the cache path too.Testing
bash -nandshellcheck(viamake lint) clean on all four scriptspre-code-test.sh/post-code-test.sh/post-fix-test.shsuites pass$GITHUB_WORKSPACE/scripts), URL-cache + per-repo (resolves$GITHUB_WORKSPACE/.fullsend/scripts), companions missing everywhere (warning fires, graceful skip),GITHUB_WORKSPACEunset for local runs (safe via:-, no error)Checklist
Note: this rework removes the sequencing dependency on a
fullsendrelease — the companionfullsend-ai/agentsPR no longer needs to wait for a new subcommand to ship, sincepost-code.sh/post-fix.shfetched fromfullsend-ai/agentswill find the companions via the workspace fallback.