fix: anchor exclude dirs to absolute paths so bandit honors --exclude#616
Open
elfensky wants to merge 2 commits into
Open
fix: anchor exclude dirs to absolute paths so bandit honors --exclude#616elfensky wants to merge 2 commits into
elfensky wants to merge 2 commits into
Conversation
collect_exclude_dirs() documents itself as returning "absolute paths" but
returned str(scan_root / p), which is relative whenever scan_root is. The
Python security detector derives scan_root from os.path.commonpath of the
file list (scan_root_from_files), so it is Path(".") for any scan launched
with a relative --path.
bandit matches --exclude against the absolute paths it walks, so a relative
entry like ".worktrees" matched nothing and bandit re-scanned excluded
directories — producing thousands of phantom B101 "assert detected" findings
from worktree copies of a repo, even though ".worktrees" was in the exclude
config. The file-discovery detectors were unaffected (they filter an
already-relative file list via matches_exclusion's path-component match), and
ruff tolerates relative excludes; only bandit, which receives a directory
root and does its own walk, surfaced the bug.
Anchor a relative scan_root to its absolute form (matching the .resolve()
bandit already applies to its own scan target). Already-absolute roots are
left byte-stable, so existing callers and the test_returns_absolute_paths
expectation are unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w prompt glob
## The problem
CI has been red on `main` since 2026-04-06, and on every PR since,
because of 5 pre-existing test failures that are unrelated to feature
work. They block green builds without any way to fix them inside a
feature PR. Two distinct root causes:
1. Three bash tests in
`desloppify/tests/lang/common/test_bash_unused_imports.py` call
`detect_unused_imports`, which parses via tree-sitter. When
`tree-sitter-language-pack` is not installed (the case in the
`tests-core` job, but not `tests-full`), the function returns
`[]`. The tests then assert specific findings and fail.
2. Two review tests in
`desloppify/tests/review/test_review_commands.py` and its
integration counterpart use
`list(runs_dir.glob("*/prompts/batch-*.md"))` without sorting.
The glob order is filesystem-dependent: on macOS it happens to
return `batch-1.md` (the batch with `historical_issue_focus`)
first, on Linux it returns `batch-2.md` first. The assertion
`"Previously flagged issues" in prompt_text` then fails on
Linux because that string only appears in `batch-1.md`.
## The solution
Two small fixes:
1. Gate the bash test file on tree-sitter availability with the
existing pattern from
`desloppify/tests/lang/common/test_treesitter.py`:
```python
pytestmark = pytest.mark.skipif(
not is_available(),
reason="tree-sitter-language-pack not installed",
)
```
Without tree-sitter the tests skip cleanly; with it they pass
as before.
2. Wrap the review test's glob in `sorted(...)`:
```python
prompt_files = sorted(runs_dir.glob("*/prompts/batch-*.md"))
```
This makes the test deterministic across operating systems.
`batch-1.md` always sorts first; the assertion that depends on
its content is reliable.
## Why this is in the same PR
These two test fixes are independent of the exclude-dir anchoring in
this PR (peteromallet#616) — they are pre-existing `main` failures that could be
cherry-picked back to `main` separately. Bundling them here lets the PR
actually go green without waiting for a separate maintenance PR. The fix
was originally authored on the framework-support branch; see the
cherry-pick trailer below.
(cherry picked from commit 9baba25)
Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Problem
collect_exclude_dirs()is documented as returning "All exclusion directories as absolute paths, for passing to external tools", but it returnedstr(scan_root / p)— relative wheneverscan_rootis relative.The Python security detector derives
scan_rootfromos.path.commonpath()of the discovered file list (scan_root_from_files), which yieldsPath(".")for any scan launched with a relative--path(the common case).banditis invoked with-r <absolute scan root>and matches--excludeentries against the absolute paths it walks. A relative entry like.worktreestherefore matched nothing, and bandit re-scanned excluded directories anyway.Concrete impact
On a repo that uses git worktrees (
.worktrees/added toexclude), bandit re-scanned every worktree copy of the source tree, producing ~2000 phantomB101"assert detected" findings from the duplicated test files — even though.worktreeswas correctly excluded from every other detector (file count, duplication, smells, …). Security dropped from 100% to ~50% purely on noise.Root cause
desloppify/base/discovery/source.py::collect_exclude_dirsjoins exclusion names onto an unresolved, often-relativescan_root.Only bandit surfaces this:
matches_exclusion's path-component match — they never hand a root to an external tool;--excludetolerates relative entries;Path(dirname).name;--exclude— which was malformed.Verified against bandit 1.9.4: an absolute
--exclude /abs/.worktreesexcludes correctly; a relative.worktreesdoes not.Fix
Anchor a relative
scan_rootto its absolute form before joining the exclusion names — matching the.resolve()bandit already applies to its own scan target. Already-absolute roots are left byte-stable, so existing callers and thetest_returns_absolute_pathsexpectation are unchanged. (7 lines.)Testing
TestCollectExcludeDirs::test_relative_scan_root_yields_absolute_paths— fails before, passes after.TestCollectExcludeDirs, ruff/bandit exclude-flag, and bandit-adapter suites stay green.clean (33 files scanned).Follow-up (intentionally not in this PR)
The per-detector security cache (
review_cache.detectors.security) is keyed by_file_fingerprint, which hashes the file set but not the exclusion config. So on an already-scanned project, changingexcludedoesn't invalidate the cached security result until the file set changes (fresh scans are unaffected). A natural fix is to feed the active exclusions into the existingsalt=parameter of_file_fingerprint— happy to send that as a separate PR if you'd like.🤖 Generated with Claude Code