feat(scan): make armis-cli usable as a pre-commit framework hook - #312
feat(scan): make armis-cli usable as a pre-commit framework hook#312closerforever wants to merge 2 commits into
Conversation
`scan repo` was `MaximumNArgs(1)`, so the only way to scan a subset of a
repository was `--include-files`, a comma-separated flag value. That makes the
command undrivable by any tool that appends selected filenames to a fixed command
line -- notably the pre-commit framework, whose `pass_filenames: true` produces:
armis-cli scan repo . src/a.py src/b.py
Error: accepts at most 1 arg(s), received 3
Arguments after the path are now files to scan, merged with `--include-files`
because both name the same thing. Three edges are handled deliberately:
- `--changed` derives its own file list, so combining it with trailing files is
rejected rather than silently discarding one of the two, mirroring the existing
`--include-files` / `--changed` mutual exclusion.
- A selection larger than `MaxFiles` falls back to a whole-repository scan with a
warning. That analyses a superset, so nothing the caller asked about goes
unexamined; erroring out would turn a large `pre-commit run --all-files` into
no scan at all.
- A file as the first argument now says that the first argument is the repository
path, since that is the mistake the new form invites.
Git-hint detection stays off for a trailing-file scan, for the same reason it is
off for `--include-files`: a partial upload is not a whole-repo snapshot, so a
baseline diff against it would be meaningless.
armis-cli already has a `.pre-commit-config.yaml`, but that only configures the linters that run against armis-cli's own source. It does not make the repository usable on the right-hand side of a `repo:` entry in someone else's config -- that requires a `.pre-commit-hooks.yaml` manifest, which did not exist. The only pre-commit story available until now was `armis-cli hook init`, a raw `.git/hooks/pre-commit` that the framework cannot see and that CI, which runs `pre-commit run --all-files` on a fresh checkout, never invokes. Three hooks, all `language: golang` so pre-commit fetches a pinned toolchain and `go install`s this repository into its own cache -- no packaging change needed: - `armis-scan` scans the files pre-commit selected, via the trailing-file form added in the previous commit. This is the hook for `pre-commit run --all-files`, the command both developers and CI invoke. - `armis-scan-staged` scans the git index, for the local commit path. Documented as unsuitable for CI: a fresh checkout has an empty index, so it would report success without scanning anything. - `armis-scan-base-ref` scans against a base branch, for pull-request gating. Credentials have to be ambient (`ARMIS_CLIENT_ID` / `ARMIS_CLIENT_SECRET`, or a completed SSO login), because a hook runs in an isolated environment that never sees the consumer project's .env file. The manifest header says so.
There was a problem hiding this comment.
🟡 Changes recommended
The new pre-commit manifest’s pinned rev example is incorrect for the newly introduced file and a newly surfaced error message is misleading for positional file usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes armis-cli consumable as a first-class pre-commit hook repository by allowing scan repo to accept trailing file arguments (for pass_filenames: true) and by publishing a .pre-commit-hooks.yaml manifest that defines supported hooks.
Changes:
- Extend
armis-cli scan repoto accept trailing positional file paths and merge them with--include-files, while rejecting conflicts with--changedand falling back to whole-repo scan when exceedingrepo.MaxFiles. - Add
.pre-commit-hooks.yamlwith three hook definitions (armis-scan,armis-scan-staged,armis-scan-base-ref) usinglanguage: golang. - Update/extend command and behavior tests to cover the new argument handling.
File summaries
| File | Description |
|---|---|
| internal/cmd/scan_test.go | Makes scan subcommand discovery resilient to usage-string changes by matching on cmd.Name(). |
| internal/cmd/scan_repo.go | Implements trailing file args handling for scan repo (pre-commit pass_filenames support) and related validation/fallback behavior. |
| internal/cmd/scan_repo_test.go | Adds tests covering trailing file args behavior, conflicts with --changed, traversal validation, and MaxFiles fallback. |
| .pre-commit-hooks.yaml | Publishes pre-commit hook manifest enabling this repo to be used as a repo: hook source. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # # in a consumer project's .pre-commit-config.yaml | ||
| # - repo: https://github.com/ArmisSecurity/armis-cli | ||
| # rev: v1.22.0 | ||
| # hooks: | ||
| # - id: armis-scan |
| fileList, err := repo.ParseFileList(absPath, selectedFiles) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid --include-files: %w", err) | ||
| } |
yiftach-armis
left a comment
There was a problem hiding this comment.
Automated review findings (see inline comments for details). Two additional findings that fall outside this diff's changed lines:
- internal/cmd/scan_repo.go (the
--changedhandling, further down in the file, untouched by this diff): it still hits the sameMaxFilesbound viaGitChangedFiles/ParseFileListbut hard-errors on overflow, while the new positional-args/--include-filespath (see inline comment on theif len(selectedFiles) > repo.MaxFilescheck) now silently falls back to a full-repo scan. Same limit, inconsistent behavior depending on which path selects the files. - README.md: still documents
armis-cli scan repo [path]and doesn't mention the new[path] [file...]trailing-file form this PR adds.
| // trailing form exists so a tool that appends selected filenames to a fixed | ||
| // command line -- pre-commit with `pass_filenames: true`, xargs, a git hook -- | ||
| // can drive `scan repo` without knowing about --include-files. | ||
| Args: cobra.ArbitraryArgs, |
There was a problem hiding this comment.
Correctness: Switching from cobra.MaximumNArgs(1) to cobra.ArbitraryArgs means malformed multi-argument invocations that used to be rejected at the Cobra Args stage with zero side effects now reach RunE and can trigger a live auth network round-trip (JWT token exchange) before argument validation. E.g. armis-cli scan repo /repo ../../etc/passwd now runs os.Stat then getAuthProvider/GetTenantID before ParseFileList (further down) ever rejects the traversal path — previously this was rejected instantly.
| // Trailing arguments are files, merged with --include-files rather than | ||
| // conflicting with it: both name the same thing, a subset of the repository | ||
| // to analyse. | ||
| selectedFiles := make([]string, 0, len(includeFiles)+len(args)) |
There was a problem hiding this comment.
Correctness: includeFiles and trailing positional files are concatenated with no de-duplication before the MaxFiles check and ParseFileList. A pre-commit config with args: [--include-files=a.py,b.py,...] plus pass_filenames: true will double-count the same files, potentially tripping the MaxFiles fallback below at roughly half the real file count.
| // MarkFlagsMutuallyExclusive covers --include-files vs --changed; positional | ||
| // files need the same guard for the same reason: --changed derives its own | ||
| // file list, so a second, contradictory one would be silently discarded. | ||
| if len(args) > 1 && cmd.Flags().Changed("changed") { |
There was a problem hiding this comment.
Correctness: This mutual-exclusion guard covers --changed + positional args, but there's a broader inconsistency: the --changed path (elsewhere in this file, not touched by this diff) still hard-errors when its own file list exceeds MaxFiles, while the positional-args/--include-files selection below now silently falls back to a full-repo scan on the same overflow. Two callers of the same limit, two different behaviors.
| // Falling back to the whole repository analyses a superset, so nothing the | ||
| // caller asked about goes unexamined -- the alternative turns a large | ||
| // `pre-commit run --all-files` into no scan at all. | ||
| if len(selectedFiles) > repo.MaxFiles { |
There was a problem hiding this comment.
Correctness: This silently converts a pure --include-files overflow (no trailing positional args at all) from a hard error into a whole-repo scan. E.g. armis-cli scan repo . --include-files=<1500 paths> used to fail with invalid --include-files: too many files: maximum 1000 files allowed; now it exits 0 after silently scanning the entire repo — a behavior/contract change for existing --include-files users, unrelated to the new pre-commit feature.
Also: this counts raw slice length, not the post-empty-filter count ParseFileList/addFile would use — stray empty CSV entries (trailing/double commas) can push len(selectedFiles) over 1000 and trigger this fallback even when the real file count is well under the cap.
| // caller asked about goes unexamined -- the alternative turns a large | ||
| // `pre-commit run --all-files` into no scan at all. | ||
| if len(selectedFiles) > repo.MaxFiles { | ||
| fmt.Fprintf(os.Stderr, |
There was a problem hiding this comment.
Correctness: This warning is printed before the repository path is validated to exist/be a directory (see os.Stat below), so it can be shown even when no scan happens at all — e.g. path doesn't exist → this warning prints, immediately followed by a path does not exist error and nonzero exit.
| return fmt.Errorf("cannot access path %s: %w", repoPath, err) | ||
| } | ||
| if !info.IsDir() { | ||
| if len(args) > 1 { |
There was a problem hiding this comment.
Usability: This hint only fires when len(args) > 1. A single-file-only invocation (e.g. a script forwarding one changed filename: armis-cli scan repo only_changed_file.py) hits len(args) == 1 and gets the plain, unhelpful message instead — exactly the confusion this hint was meant to address, just at n=1.
| fileList, err := repo.ParseFileList(absPath, includeFiles) | ||
| fileList, err := repo.ParseFileList(absPath, selectedFiles) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid --include-files: %w", err) |
There was a problem hiding this comment.
Correctness: Errors from ParseFileList are always framed as "invalid --include-files" even when the failing path came solely from a new trailing positional argument. E.g. armis-cli scan repo . ../../etc/passwd (no --include-files used) returns invalid --include-files: absolute path "../../etc/passwd" is outside repository root, pointing at a flag the user never used. TestScanRepoRunE_TrailingFilesGetPathValidation bakes this wrong wording in as expected.
| language: golang | ||
| pass_filenames: true | ||
| types_or: [python, jupyter, shell, bash, sql, yaml, terraform, go, javascript, ts, tsx, java, scala] | ||
| require_serial: true |
There was a problem hiding this comment.
Performance: require_serial: true on this pass_filenames: true hook forces pre-commit's own internal split of a large filename list (argv-length batching) to run fully serially instead of using pre-commit's worker pool. On pre-commit run --all-files with many changed files (or on Windows, with a much smaller argv limit), this multiplies wall-clock time by the number of batches, each paying a full tarball-upload-and-poll round trip.
What this is
Two commits that make
armis-cliconsumable as a pre-commitframework hook, found while piloting an Armis AppSec scan
as a default commit gate in an internal Azure Databricks project template.
Measured against
armis-cliv1.22.0 and the live API, not inferred from readingthe code.
The gap
armis-cliships a.pre-commit-config.yaml, but that only configures the lintersthat run against
armis-cli's own source. It does not make this repositoryusable on the right-hand side of a
repo:entry in someone else's config — thatneeds a
.pre-commit-hooks.yamlmanifest, which does not exist onmain.The only pre-commit story available today is
armis-cli hook init, which writes araw
.git/hooks/pre-commit. Two problems with that, both reproduced:InstallPreCommit(
internal/install/precommit.go:69) appends the Armis section to an existing.git/hooks/pre-commit. pre-commit's generated hook ends inexec pre-commit hook-impl …, so in the order every project documents —pre-commit installfirst — the appended Armis section is unreachable deadcode. The CLI still prints
Pre-commit hook installed (fail-closed)andIsPreCommitInstalledreturns true, and nothing ever scans. The reverse orderis not a fix either:
pre-commit installmoves the file topre-commit.legacyandhook-impldoes run it, but outside the framework —no file selection, no unstaged stash, invisible to
pre-commit run --all-files, andhook init --removecan no longer find it, so uninstallno-ops.
pre-commit run --all-fileson afresh checkout; a hook the framework cannot see is not part of that run.
And the obvious manifest — one pointing at
scan repo . --changed=staged— is avacuous gate in exactly that CI shape: a fresh checkout has an empty index, so
repo.ErrNoChangedFilesprintsNo changed files found - nothing to scan.andreturns
nil→ rc=0 (internal/cmd/scan_repo.go:165). It passes without scanning.--changed=mainfails the otherway: the ref must resolve locally or
ErrRefNotFoundis a hard non-zero, i.e. aspurious block rather than a finding.
The change
1.
scan repoaccepts file paths as trailing arguments. It wasMaximumNArgs(1), sopass_filenames: trueproduced:Trailing arguments are now files to scan, merged with
--include-filessince bothname the same thing. Three edges handled deliberately: combining with
--changedis rejected (mirroring the existing
--include-files/--changedexclusion); aselection larger than
MaxFilesfalls back to a whole-repository scan with awarning (a superset, so nothing asked about goes unexamined — erroring out would
turn a large
pre-commit run --all-filesinto no scan); and a file as the firstargument now says the first argument is the repository path.
2.
.pre-commit-hooks.yaml, three hooks, alllanguage: golang— pre-commitfetches a pinned toolchain and
go installs this repository into its own cache, sono packaging change is needed:
armis-scanpass_filenames: true)pre-commit run --all-filesin CI — same binary, same flags at botharmis-scan-stagedarmis-scan-base-refarmis-scanis the one that matters: it is faithful at both enforcement points,which is what makes "green locally ⇒ green in CI" hold.
Credentials must be ambient (
ARMIS_CLIENT_ID/ARMIS_CLIENT_SECRET, or acompleted SSO login) because a hook runs in an isolated environment that never sees
the consumer project's
.env. The manifest header says so.Why
pass_filenamesand not just--changedBesides the vacuous-gate problem above, it is the only mitigation we could measure
for scan latency. On one internal repository the same whole-tree scan took 37 s,
then 511 s, then >600 s; a one-line commit costs 27–80 s. Narrowing the tarball to
the selected files is the lever a hook has. (The fixed per-job cost remains — see
the ask below.)
Verification
go build ./...,go vet ./...,gofmt -lclean,go test ./...— 22/22 packagesok, exit 0. New tests:internal/cmd/scan_repo_test.gocovers the trailing-filemerge, the
--changedconflict, theMaxFilesfallback and the file-as-first-argmessage.
Two related asks, not in this PR
ErrNoChangedFilesneeds to be distinguishable from "scanned, clean" for anygate use — today both are rc=0 with no machine-readable difference.
--scan-timeouthas a 1-minute floor (default 60,internal/cmd/scan.go:176; the floor is validated at:113). A local hook cannot bound the 511 s queue tail below 60 s, and a timeoutexits non-zero — a spurious block on a slow queue. A sub-minute floor and/or
--on-timeout=warnwould make this pilotable.