feat(scaffold): auto-detect and install pre-commit tool dependencies - #1055
Conversation
Site previewPreview: https://499874db-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsMedium
Low
Resolved from prior review
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsLow
Previous run (8)ReviewFindingsMedium
Low
Labels: PR modifies scaffold harness scripts (pre/post-code.sh, pre/post-fix.sh) and adds new runner-side tooling Previous runReviewFindingsMedium
Low
Info
Resolved from prior review
Labels: PR modifies scaffold harness scripts (pre/post-code.sh, pre/post-fix.sh) and adds new runner-side tooling. Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Info
Previous run (7)ReviewFindingsLow
Previous run (8)ReviewFindingsMedium
Low
|
fullsend-ai-review
Bot
added
the
requires-manual-review
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
|
||
| # ── gitleaks (secret scanning) ──────────────────────────────────── | ||
| # Post-scripts install gitleaks independently as a security gate. | ||
| # This entry exists only so the resolver recognizes gitleaks hooks |
There was a problem hiding this comment.
[low] correctness
The strip_prefix for lychee is set to "lychee-{triple}", but the original hardcoded install extracted the binary from the tarball root. If the tarball has no subdirectory, the find fallback triggers with a spurious warning on every install.
Suggested fix: Verify the lychee tarball structure and remove strip_prefix if the binary is at the root.
| # Also build entry-based lookup for local hooks matched by entry content | ||
| repo_hook_map = {} | ||
| entry_match_map = {} | ||
| for tool in registry_tools: |
There was a problem hiding this comment.
[low] correctness
PyYAML is installed with an unpinned version floor (pyyaml>=6.0). Every other dependency in this PR uses pinned versions with SHA256 checksums. A compromised PyYAML could influence tool selection, though tool binaries are checksum-verified.
Suggested fix: Pin PyYAML to a specific version (e.g., pyyaml==6.0.2) for consistency with the supply-chain posture.
Review follow-upsCreated follow-up issues for actionable non-blocking review findings:
|
fullsend-ai-review
Bot
added
ready-for-merge
There was a problem hiding this comment.
Shouldn't this be top level? Now it will be distributed/
There was a problem hiding this comment.
Good question — moved it from tools/ to scripts/ in a follow-up push but forgot to reply here.
The registry lives under scripts/ because resolve-precommit-tools.py looks it up relative to its own directory (os.path.join(script_dir, "precommit-tools.yaml")), so co-locating them keeps the lookup simple with no extra path configuration.
Also, scripts/ is a layered directory in scaffold.go — it gets distributed at runtime via reusable workflows, not installed into .fullsend repos. Putting it at the top level would mean it gets scaffolded into every org's config repo, which isn't the intent.
There was a problem hiding this comment.
But having it as a layered directory does the same as being installed into repositories from the practical point of view, no? The script will install OUR tools for all repositories using fullsend. We need to move this to toplevel, so the script will get layered, but the definition of OUR tools won't. Users of fullsend can take advantage of this creating its own precommit-tools.yaml at the root level.
There was a problem hiding this comment.
I see what you mean — since scripts/ is layered, the registry does travel with the scripts at runtime. But I think that's the right behavior for this PR's scope, and here's why:
What this PR replaces: The hardcoded LYCHEE_VERSION/LYCHEE_SHA256_AMD64/UV_VERSION/UV_SHA256 blocks that were previously inline in post-code.sh and post-fix.sh. Those were also distributed via the same layered scripts/ path. The registry is the same data externalized into a structured file — it doesn't change what gets distributed, just how it's organized.
What the registry is: Fullsend's knowledge of which pre-commit hook repos need which system tools. It's infrastructure knowledge, not user configuration. Users don't need to know (or care) that lychee needs a specific binary download with a specific checksum — that's fullsend's job.
User-provided overrides are #1270 territory. The resolver already takes registry_path as a parameter (not hardcoded in resolve()), so adding a merge step later — load fullsend's built-in registry, overlay a user-provided precommit-tools.yaml from the target repo — is straightforward. But that's a different feature with different requirements (merge semantics, conflict resolution, validation).
This PR needs to land so the follow-up work can proceed — #1270 (registry expansion), #836 (shared tool install logic), #850 (pre-flight checks), and #1056/#1057 (review findings) are all blocked on it. I'd rather not widen the scope further.
There was a problem hiding this comment.
What about shellcheck and actionlint? Those are in the file, but they weren't on post-code. They are currently compiled by golang
There was a problem hiding this comment.
Good call — you're right that shellcheck and actionlint weren't in the old hardcoded blocks.
shellcheck: removed. Both shellcheck-py/shellcheck-py (language: python) and koalaman/shellcheck-precommit (language: docker_image) are self-managed by pre-commit — it installs shellcheck-py via pip/venv or pulls the Docker image. The apt-installed system shellcheck was wasted work and could cause version skew with the pip-bundled binary. Dropped both entries.
actionlint: kept with rationale comment. The hook uses language: golang, so pre-commit CAN compile it from source, but that takes ~2 minutes on GHA runners. The registry downloads the pre-built binary in ~3 seconds. It's a performance optimization, not a correctness fix. Added an explicit comment making this trade-off visible. If you'd rather let pre-commit handle it natively and accept the build time, I can drop it too.
Also added a comment to the registry header documenting when entries are appropriate (only for hooks pre-commit can't self-serve) and how to customize (place .pre-commit-tools.yaml in customized/scripts/ for full replacement). Additive merge (adding/suppressing individual entries without replacing the whole file) is follow-up territory (#1270).
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
Hm.. I don't like this. It is mixing bash and Python in a weird way, create a Python file and use the bash to forward all the arguments if needed.
|
This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the |
39bd65a to
70151ea
Compare
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
|
🤖 Review · |
70151ea to
029b537
Compare
|
🤖 Review · |
029b537 to
cec8f15
Compare
|
🤖 Review · |
cec8f15 to
054f17e
Compare
|
🤖 Review · |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Add a pre-commit hook that runs `pinact run --fix=false --no-api` to verify all GitHub Actions references use full-length commit SHAs. The --no-api flag ensures the check is offline-only (syntactic SHA presence) so it won't break when new action versions are released. Also install pinact in the CI lint workflow so the hook passes there. Depends-on: #1055 (auto-detect pre-commit tool dependencies) Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
waynesun09
added
the
ok-to-test
Add a pre-commit hook that runs `pinact run --fix=false --no-api` to verify all GitHub Actions references use full-length commit SHAs. The --no-api flag ensures the check is offline-only (syntactic SHA presence) so it won't break when new action versions are released. Also install pinact in the CI lint workflow so the hook passes there. Depends-on: #1055 (auto-detect pre-commit tool dependencies) Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add a pre-commit hook that runs `pinact run --fix=false --no-api` to verify all GitHub Actions references use full-length commit SHAs. The --no-api flag ensures the check is offline-only (syntactic SHA presence) so it won't break when new action versions are released. Also install pinact in the CI lint workflow so the hook passes there. Depends-on: #1055 (auto-detect pre-commit tool dependencies) Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add a pre-commit hook that runs `pinact run --fix=false --no-api` to verify all GitHub Actions references use full-length commit SHAs. The --no-api flag ensures the check is offline-only (syntactic SHA presence) so it won't break when new action versions are released. Also install pinact in the CI lint workflow so the hook passes there. Depends-on: #1055 (auto-detect pre-commit tool dependencies) Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add a pre-commit hook that runs `pinact run --fix=false --no-api` to verify all GitHub Actions references use full-length commit SHAs. The --no-api flag ensures the check is offline-only (syntactic SHA presence) so it won't break when new action versions are released. Also install pinact in the CI lint workflow so the hook passes there. Depends-on: fullsend-ai#1055 (auto-detect pre-commit tool dependencies) Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
831a6bb to
733f6a3
Compare
Add a registry-based system for resolving and installing pre-commit hook tool dependencies at runtime, replacing hardcoded tool installs baked into OpenShell container images. New files: - tools/precommit-tools.yaml: registry mapping hook repos/IDs to system tools with pinned versions and SHA256 checksums - scripts/resolve-precommit-tools.py: standalone Python resolver that parses .pre-commit-config.yaml against the registry - scripts/resolve-precommit-tools.sh: bash wrapper ensuring PyYAML is available before invoking the Python resolver - scripts/install-precommit-tools.sh: installs tools from the JSON manifest (binary/apt/pip/npm) with architecture detection Modified pre/post scripts (pre-code, pre-fix, post-code, post-fix) to call the resolver and installer instead of hardcoding tool versions. Removes LYCHEE_VERSION/UV_VERSION constants from post-code.sh and post-fix.sh. Supply-chain hardening: - Binary downloads use pinned versions + SHA256 checksums - pip installs use --no-deps to prevent transitive dependency attacks - npm installs use --ignore-scripts to prevent install-time RCE - jq architecture lookups use --arg binding (not shell interpolation) - PyYAML pinned to ==6.0.2 - Pre-scripts write to GITHUB_PATH for cross-step persistence Closes #1270 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
733f6a3 to
228b9e5
Compare
|
🤖 Finished Retro · ✅ Success · Started 1:33 PM UTC · Completed 1:40 PM UTC |
Retro: PR #1055 — feat(scaffold): auto-detect and install pre-commit tool dependenciesTimeline
Workflow Quality AssessmentReview quality: Good, with complementary human/bot coverage. The review bot excelled at implementation correctness and security (PyYAML supply chain, injection vulnerabilities, edge cases). The human reviewer focused on architectural design (file placement in layered directories, bash/Python separation, which tools belong in the registry). Neither fully substituted for the other. Rework rate: Moderate. The PR went through 8 head SHAs, but much of this was driven by substantive design feedback from the human reviewer, not bot-driven churn. Token cost: Elevated. 9 review runs (5 cancelled) represents significant waste. The 5 cancellations on SHA Notable bot behavior: The bot demonstrated good self-correction by catching its own false "resolved" marking for PyYAML Existing Issue CoverageMost improvement opportunities identified are already tracked by open issues:
ProposalsOne novel improvement identified (see below). All other potential proposals were filtered as duplicates of existing open issues listed above. Proposals filed
|
2 participants
Summary
.pre-commit-tools.yaml) that maps pre-commit hook repos/IDs to the system tools they require, with pinned versions and SHA256 checksumsresolve-precommit-tools.pyto parse a target repo's.pre-commit-config.yamland produce a JSON manifest of needed toolsinstall-precommit-tools.shto install tools from the manifest with architecture detection and checksum verificationpre-code.sh,pre-fix.sh) so tools are installed before the sandbox runspost-code.sh,post-fix.sh) with the same auto-resolve mechanism as a fallbackMotivation
Post-scripts run authoritative pre-commit hooks after the sandbox exits. If a target repo's
.pre-commit-config.yamlrequires tools not baked into the sandbox image (e.g., lychee, shellcheck, actionlint), the hooks fail and block the push. The previous approach of manually adding tool installs to post-scripts doesn't scale — every new tool needs a code change.This auto-detection system reads the target repo's hook config and installs what's needed, with the same supply-chain security (pinned versions, SHA256 checksums) as the hardcoded installs it replaces.
Related: #1270 (expanding registry coverage — follow-up work after this lands)
Test plan
resolve-precommit-tools.pycorrectly parses fullsend's own.pre-commit-config.yamland produces valid JSON manifestinstall-precommit-tools.shinstalls binary tools with checksum verification.pre-commit-config.yamlexistsLYCHEE_VERSION/UV_VERSIONmake lint— passes