feat: disarm scan — the one API built for scanning, pointed at files (#704) - #944
Merged
Conversation
…704) `inspect_anomalies` has always returned everything a scanner needs: a kind, a span, evidence and a plain-language reason. There was no way to run it over a file. Every CLI subcommand took text from an argument or stdin; none took a path. That single absence is most of why third-party tools in this space exist as separate projects rather than thin wrappers: the detection is the hard part and disarm has it, the file plumbing is the easy part and disarm had none of it. Python rather than a Rust binary, which is #704's own recommendation and its reasoning: plumbing over an API that already exists ships now and can be measured against real repositories before anyone commits to a binary per tag. The four rules #704 takes from `juriku/untrace`, each asserted: git's ignore rules come from THREE sources — `.gitignore` in the scanned directory and every parent, `.git/info/exclude`, and `core.excludesFile`. A scanner reading only the nearest file makes `scan src/` and `scan .` disagree on one tree, which users report as flakiness. This delegates to `git check-ignore` rather than reimplementing the rule, so the two cannot disagree. A test scans the same tree from the root and from a subdirectory and asserts they agree. The skip list is defensible, not convenient. `node_modules`, `__pycache__`, `.venv`, `.terraform` hold no hand-written source. `build`, `dist`, `out`, `target`, `bin` and `vendor` are NOT skipped — hand-written in some projects, and a scanner that skips them by name reports clean on a tree it never read. Both halves are parametrized tests. Symlinks are never followed, directory or file. A directory link could walk out of the tree or loop; a file link would report a finding against a path whose content lives somewhere else. Something found is not something failed to read. Exit 0 clean or found without `--fail`, 1 found with `--fail`, 3 a path could not be read; 2 stays argparse's, since `docs/cli.md` already assigns it. The first version got the third code wrong twice, and the tests caught both: a missing root fell through `root.is_file()` into `os.walk`, which yields nothing and raises nothing for a path that does not exist — so a typo'd path "scanned cleanly" with exit 0; and an unlistable subdirectory was swallowed by `os.walk`'s default `onerror=None`. Both now surface as unreadable. Silence looks like clean, and the contract exists so that it does not. `--json` converts the library's byte spans to 1-based line and CHARACTER column, which is what an editor's gutter shows. Pinned with a non-ASCII prefix where byte and character columns diverge — the same trap #940's spans set. Binary, non-UTF-8 and oversize files are skipped without error: they are not text, and `decode_to_utf8` is a different job. Inline suppression is deliberately absent (#704 item 5): there is nothing to suppress until there is a scanner. Closes #704 Assisted-by: Claude Opus 5 (1M context) Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
📄 Docs preview: https://345fe78e.disarm-docs.pages.dev |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current implementation violates the “never follow symlinks” contract for symlink root paths, and a new test uses os.geteuid() in a way that breaks imports on non-POSIX platforms.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new Python CLI subcommand, disarm scan, that walks files/directories and runs inspect_anomalies over UTF-8 text files to surface bidi/invisible/smuggling findings with a stable location (1-based line + character column) and a CI-friendly exit-code contract.
Changes:
- Introduces
python/disarm/scan.pyimplementing file iteration (gitignore-aware viagit check-ignore), scanning, JSON/plain output, and exit codes (0/1/3). - Extends the Python CLI (
python/disarm/__main__.py) withscan/scplus--fail,--json, and--no-gitignore. - Adds comprehensive unit + argv-level tests and updates docs/CHANGELOG to document the new command and exit-code behavior.
File summaries
| File | Description |
|---|---|
| tests/test_scan.py | New unit tests for gitignore behavior, skip list, symlink handling, exit-code contract, and JSON shape. |
| tests/test_cli.py | New subprocess/argv-level coverage for disarm scan behavior and CLI contract. |
| python/disarm/scan.py | New scanning implementation (walk + gitignore delegation + reporting/exit codes). |
| python/disarm/main.py | Registers the scan/sc subcommand and wires flags to disarm.scan.run. |
| docs/cli.md | Documents scan usage, options, and exit codes; updates global exit-code table. |
| CHANGELOG.md | Announces the new disarm scan command and its behavior/contract. |
Review details
- Files reviewed: 6/6 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.
) Copilot review on #944, both real. The contract says symlinks are never followed, and the walk refused symlinked entries under a root — but `Path.is_file()` and `is_dir()` resolve links, so a root that was itself a symlink was followed. `disarm scan <link>` walked whatever it pointed at, outside the tree it was pointed at. Refused now, and REPORTED through `on_error` rather than skipped: a root the scanner declined must not look like a root that scanned clean, which is the same rule the missing-path and unlistable-directory cases follow. Exit 3. `os.geteuid()` in a `skipif` expression evaluates at import, and the function does not exist on Windows — so the test module raised before any test could be skipped. `hasattr` first; on Windows the test is skipped, which is right, since `chmod 000` does not lock a directory there either. Regression test for the root link covers both halves — a link to a directory and a link to a file — and asserts the refusal is reported, not silent. Refs #704 Assisted-by: Claude Opus 5 (1M context) Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
inspect_anomalieshas always returned everything a scanner needs — a kind, a span, evidence and a plain-language reason — and there was no way to run it over a file. Every CLI subcommand took text from an argument or stdin; none took a path.Python rather than a Rust binary — #704's own recommendation: plumbing over an API that already exists ships now and can be measured against real repositories before anyone commits to a binary per tag.
The four rules, each asserted
git's ignore rules, all three sources.
.gitignorein the scanned directory and every parent,.git/info/exclude, andcore.excludesFile. A scanner reading only the nearest file makesscan src/andscan .disagree on one tree. This delegates togit check-ignorerather than reimplementing the rule, so the two cannot disagree by construction. A test scans the same tree from the root and from a subdirectory and asserts they agree.A defensible skip list.
node_modules,__pycache__,.venv,.terraformhold no hand-written source.build,dist,out,target,bin,vendorare not skipped — a scanner that skips them by name reports clean on a tree it never read. Both halves parametrized.Symlinks never followed, directory or file.
Something found ≠ something failed to read.
0clean or found-without---fail,1found with--fail,3a path could not be read.2stays argparse's —docs/cli.mdalready assigns it.What the tests caught
The first version got the read-error code wrong twice:
root.is_file()intoos.walk, which yields nothing and raises nothing for a nonexistent path — so a typo'd path scanned cleanly with exit 0.os.walk's defaultonerror=None.Both now surface as
3. Silence looks like clean, and the contract exists so that it doesn't.--jsonByte spans become 1-based line and character column — what an editor's gutter shows. Pinned with a non-ASCII prefix where byte and character columns diverge (the trap #940's spans set).
Binary, non-UTF-8 and oversize files are skipped without error. Inline suppression is deliberately absent (#704 item 5): nothing to suppress until there is a scanner.
Verification
ruff check/format,mypy— cleanpytest -n 8 --dist loadfile— 7,234 passed, 42 skipped (38 new: 31 unit, 7 argv-level)mkdocs build --strict,run_doc_tests.py41/41 — clean--help,--fail→ 1, missing path → stderr + 3Closes #704
🤖 Generated with Claude Code