fix(scripts): add pre-commit tool resolver and installer - #58
Conversation
Copy the shared pre-commit tool chain from the fullsend scaffold so pre-code/post-code and pre-fix/post-fix can auto-install hook dependencies (for example lychee) on the GitHub Actions runner. Fixes #57 Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 2:12 PM UTC · Completed 2:23 PM UTC |
PR Summary by QodoAdd pre-commit tool resolver + installer chain for runner-side hook deps
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewVerdict: Comment — one medium-severity finding on unchanged code; none blocking This PR restores the pre-commit tool auto-install chain that existing pre/post scripts ( The second commit ( The resolver, installer, and registry are well-structured. The JSON manifest contract between resolver and installer is consistent. SHA256 checksums are verified for binary downloads, FindingsMedium1. [security · GHA-command-injection] Manifest-derived values not sanitized in workflow commands The script sanitizes resolver warnings for Remediation: Create a sanitization helper (strip Severity unchanged from prior review — code unchanged. Low2. [security · supply-chain] pyyaml auto-install without hash verification The script auto-installs Downgraded from medium to low — consistent with existing codebase convention. 3. [security · network] The 4. [security · input-validation] Manifest-derived NAME used in command execution
5. [correctness · test-coverage] No regression test for exclude + match_entry interaction The 6. [correctness · test-coverage] Non-local repo hook matching untested All 7. [correctness · test-coverage] Language-based warning paths untested The 8. [correctness · test-coverage] skip_install flag path untested The gitleaks 9. [correctness · maintainability] Duplicated uv install block The uv install definition (version, checksums, url_template, strip_prefix, binary_name, extra_binaries) is duplicated verbatim between the Prior review finding status
Previous runReviewVerdict: Comment — medium-severity findings worth noting; none blocking This PR restores the pre-commit tool auto-install chain that existing scripts ( The resolver, installer, and registry are well-structured. The JSON manifest contract between resolver and installer is consistent. SHA256 checksums are verified for binary downloads, FindingsMedium1. [security] Manifest-derived values not sanitized in GHA workflow commands The script correctly sanitizes resolver warnings for Remediation: Create a helper function that strips 2. [security] The Remediation: Add 3. [security] pyyaml auto-install without hash verification The script auto-installs Remediation: Either add 4. [correctness] Default GOARCH for x86_64 is The default Remediation: Change 5. [correctness] When excluding an upstream entry, the code reads Remediation: Read Low6. [correctness] Test coverage gaps The test suite covers merge logic and Remediation: Add test cases for these paths. 7. [correctness] Duplicated uv install block The uv install definition (version, checksums, url_template, strip_prefix, binary_name, extra_binaries) is duplicated verbatim between the Remediation: Add a comment on the second entry noting it must be kept in sync, or restructure the registry to support shared install definitions. 8. [security] The Remediation: Add Labels: PR fixes missing pre-commit tool chain scripts (bug fix per issue #57) |
![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
requires-manual-review
- Use amd64 as default GOARCH for x86_64 (standard Go release naming) - Add --no-absolute-names to tar extraction - Clear match_entry_owners from upstream entry on registry exclude Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 2:40 PM UTC · Ended 2:51 PM UTC |
|
Replying to the top-level review findings not covered by inline threads: 1. GHA workflow-command sanitization for manifest fields — Deferred. Manifest values come from controlled registries and base-branch 6. Test coverage gaps — Deferred to a follow-up 7. Duplicated 8. |
|
Re: finding 5 (::warning:: vars unsanitized) — Deferred. Resolver warnings are partially sanitized today; manifest-derived fields (NAME, VERSION, etc.) come from controlled YAML registries, not untrusted PR-head input. Same pattern as fullsend scaffold. A shared sanitizer helper is reasonable follow-up work in fullsend, then sync. |
| fi | ||
|
|
||
| if [ -z "${TRIPLE}" ]; then | ||
| echo "::warning::Cannot install ${NAME} — unsupported architecture" |
There was a problem hiding this comment.
[medium] GHA-command-injection
Manifest-derived variables NAME, VERSION, TYPE, BINARY_NAME are interpolated unsanitized into 15 GHA workflow commands (::warning:: and ::error::) throughout the script. The sanitization applied to the warnings array (lines 58-64) is not applied to these variables. While the manifest comes from controlled registries (upstream in this repo + base-branch-only local override), consistent sanitization would eliminate a class of GHA command injection risk.
Suggested fix: Create a sanitization helper that strips ::, %0A/%0a, %0D/%0d and apply it to all manifest-derived values before interpolating into workflow command strings.
| import yaml | ||
| except ImportError: | ||
| try: | ||
| subprocess.check_call( |
There was a problem hiding this comment.
[low] supply-chain
pyyaml==6.0.2 is auto-installed via pip at import time without --require-hashes. The version pin and --no-deps provide integrity assurance. This matches the existing codebase convention (post-code.sh installs pre-commit==4.5.1 the same way).
Suggested fix: Add --require-hashes with the known SHA256 hash, or pre-install pyyaml in the CI runner image.
| DL_TMPDIR="$(mktemp -d)" | ||
| TARBALL="${DL_TMPDIR}/${NAME}.tar.gz" | ||
|
|
||
| if ! curl -fsSL "${URL}" -o "${TARBALL}"; then |
There was a problem hiding this comment.
[low] network-security
curl -fsSL follows redirects without --proto '=https'. Mitigated by SHA256 checksum verification. Matches existing codebase pattern in post-code.sh gitleaks download.
| binary) | ||
| VERSION="$(echo "${entry}" | jq -r '.version')" | ||
| if command -v "${NAME}" >/dev/null 2>&1; then | ||
| INSTALLED_VERSION="$("${NAME}" --version 2>&1 | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 || true)" |
There was a problem hiding this comment.
[low] input-validation
Manifest-derived NAME controls which binary is executed (line 91) and which packages are installed via apt/pip/npm (lines 205, 217, 230). Values are properly quoted. Base-branch-only sourcing is the primary trust control.
| # ── uv / uvx (Python package manager, needed for ty check) ─────── | ||
| # Two match entries: hooks may use "uvx <tool>" or "uv run <tool>". | ||
| # Both resolve to the same install (dedup via seen_names on "uv"). | ||
| - hook_id: ty |
There was a problem hiding this comment.
[low] maintainability
The uv install block is duplicated verbatim between the ty (line 120) and uv-run (line 135) entries. Both must be updated in lockstep when uv is bumped. YAML anchors could eliminate this duplication.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 2:40 PM UTC · Completed 2:51 PM UTC |
|
Drafted this after reading fullsend-ai/fullsend#3393 I'm wondering why we don't just move those companion files to this agents repo directly like in this PR. |
|
Dug into how the pre/post scripts are actually loaded at runtime, and I don't think this PR can fix the failure it targets — the premise that restoring sibling files makes the How the scripts actually execute in production The fullsend CLI resolves the agent harness and downloads each referenced resource ( That sha256 is an exact match for this repo's What actually closed the The workspace fallback (#41 here, plus the same fix in the fullsend scaffold via fullsend-ai/fullsend#3393) redirects the cached script to Where I think this should go instead The real underlying question — where shared, workflow-loaded tooling should live now that agents are extracted — has a few durable answers, each better than maintaining a second copy here that the runtime can't reach:
Given the fallback already covers both per-org and per-repo modes, I'd suggest closing or parking this PR and capturing option 1/2/3 as a design issue, rather than taking on a permanent keep-in-sync burden for ~1,000 lines the current runtime never reads. |
|
@waynesun09 Agreed — this PR can't fix the failure it targets. We should park/close it rather than keep a ~1k-line sync copy the runtime never reads. The workspace fallback (#41 + fullsend-ai/fullsend#3393) is the right bridge for now. Scripts run from isolated cache blobs ( The durable fix is already tracked upstream: fullsend-ai/fullsend#3640 — extend skill-style directory-level fetching to pre/post scripts so companions land with the entrypoint. That covers option 1 (and the third recurrence of this class after #2705 / #3182 / #3069). Options 2 ( |
|
🤖 Finished Retro · ✅ Success · Started 9:54 PM UTC · Completed 10:06 PM UTC |
Retro: PR #58 — fix(scripts): add pre-commit tool resolver and installerOutcome: Closed without merge after 12 days. A human reviewer identified that the PR's fundamental premise was architecturally unsound — the review agent missed this entirely. Timeline
Key findingThe review agent produced high-quality code-level reviews but did not question whether the PR's approach could actually work. It accepted the premise ("copy sibling scripts so This is a structural gap in the correctness sub-agent's "runtime mechanism checklist," which traces mechanisms within the diff but does not extend to verifying that the deployment model supports the proposed fix mechanism. Existing issues with new evidence from this retro
Proposals filed
|
2 participants
Summary
Bug fix: restores the pre-commit tool auto-install chain that was lost during agent extraction to this repo. Without these files,
pre-code.sh/post-code.sh/pre-fix.sh/post-fix.shsilently skip runner-side tool installation, and the authoritative post-script pre-commit gate fails with errors likeExecutable lychee not found— blocking code/fix agent PRs even when sandbox work completed successfully.This PR copies the shared tooling from the fullsend scaffold (
resolve-precommit-tools.py,install-precommit-tools.sh,.pre-commit-tools.yaml, tests) so the existingSCRIPT_DIR-relative lookups in pre/post scripts work again.Fixes #57
Problem this prevents
Observed on code agent runs against
fullsend-ai/fullsend(e.g..fullsendruns 28933432433, 28904209250):fullsend-codeimage haslychee).lycheebecause auto-install never ran ([ -f "${RESOLVE_SCRIPT}" ]guard failed — companion scripts were never migrated).BLOCKED — pre-commit hooks failed on agent's changes.Changes
scripts/resolve-precommit-tools.py— hook → install manifest resolver (from fullsend scaffold)scripts/install-precommit-tools.sh— runner-side binary/tool installer (from fullsend scaffold)scripts/.pre-commit-tools.yaml— pinned tool registry includinglychee(from fullsend scaffold)scripts/resolve-precommit-tools-test.py— unit tests (from fullsend scaffold)Makefile/README.md— include resolver tests inmake script-test869a19c):GOARCH=amd64for x86_64 (wasx64, wrong for Go release URLs without per-tool override)tar --no-absolute-nameson archive extractionmatch_entry_ownerscleanup when excluding upstream registry entriesTesting
make script-test(all suites pass)Notes
fullsend postrunsubcommands instead of sibling scripts; that approach depends on an unreleased fullsend change. This PR is self-contained and unblocks thelycheefailure immediately.internal/scaffold/fullsend-repo/scripts/in fullsend; keep both repos in sync when changing this tooling.