Skip test and distro suites on docs-only changes

[singlerider]

Summary

Every pull request currently runs the full test suite across 5 Python versions and the distro suite across 6 containers, no matter what changed. A README typo costs the same as a change to the scan engine.

This adds a changes job that classifies the diff, and gates test and test-distros on its result. Docs-only pull requests skip both.

Two things that make this less trivial than it looks

Required status checks are matrix contexts. The ruleset on dev requires test (3.10) through test (3.13). The obvious implementation, a workflow-level paths-ignore, means those jobs never run and therefore never report, and GitHub blocks the PR on permanently pending checks. A skipped job reports success, so the gate has to be a job-level if: instead. That is the only reason this is a changes job rather than five lines of paths-ignore.

Pushes always run the suite. The skip is scoped to pull_request. A push to dev or stable feeds publish_code, and skipping the suite there would publish a release whose tests never ran. publish_code keeps its plain success() dependency on test.

Filter behavior

The filter lists what counts as documentation and negates it, so a new top-level directory runs the suite by default rather than being silently skipped:

code:
  - '!{docs/**,**/*.md,*.md,mkdocs.yml,LICENSE,.gitignore}'

Braces rather than a !(a|b) extglob, which matters. paths-filter matches with picomatch, and picomatch treats !(...) as a single-path-segment extglob: the docs/** inside it never applies to nested paths. An earlier version of this PR used that form, and it classified docs/scanning/index.md as code while classifying bbot/README.md as docs — close to the exact inverse of the intent. *.md is listed alongside **/*.md because picomatch does not match root-level files with a leading **/.

Verified directly against picomatch 2.3.1, the version paths-filter pins:

changed files	result
docs/scanning/index.md	skip
docs/assets/diagram.png	skip
README.md, CONTRIBUTING.md	skip
bbot/README.md	skip
mkdocs.yml, LICENSE, .gitignore	skip
bbot/modules/foo.py	run
bbot/modules/foo.py + docs/index.md	run
pyproject.toml, uv.lock	run
.github/workflows/tests.yml	run
a new whatever/thing.py	run

Mixed code+docs PRs run everything, which is the important case: the filter can only skip when nothing outside the docs set changed.

Scope

About 7% of recent merged PRs (4 of the last 60) were docs-only, so this is not a huge fraction of CI spend. It is a clean win on those, and it costs one small job on everything else.

The larger cost is the matrices themselves: 5 Python versions and 6 distros on every code PR. Trimming those on PRs while keeping the full set on merge to dev would save considerably more, but that is a policy call about coverage rather than a mechanical change, so it is not in this PR.

Testing

The filter patterns are verified against picomatch 2.3.1 directly, case by case, per the table above.

I cannot exercise the required-status-check interaction from a fork, since the ruleset applies to dev in the upstream repo. The skip-satisfies-required-check behavior is worth confirming on a real docs-only PR before this is relied on.

If #3356 lands first, the test matrix passed job there is what should be required, and it is already configured to accept a skipped test.

I re-verified every row of the table above against picomatch 2.3.1 directly, including the broken !(a|b) extglob form, which does classify docs/scanning/index.md as code and bbot/README.md as docs. The braces form in this PR is correct.

Conflicts

This and #3356 both insert a new job at the top of tests.yml and will conflict textually. Whichever lands second needs a trivial rebase; the two changes are independent in substance.

Known gaps

• dorny/paths-filter@v3 is a mutable tag. Every other third-party action in this repo is pinned the same way, so this is consistent with existing practice rather than a new risk, but it is worth noting given Add a test-matrix status check, name every job #3356 argues for pinning the gate action to a sha. Happy to pin this one too if preferred.
• The changes job relies on the default GITHUB_TOKEN permissions. If the repo default is ever tightened, this job needs pull-requests: read declared explicitly.

CI status

test (3.10) failed here on test_manager_scope_accuracy_correct:

assert 1 == len([e for e in all_events_nodups
                 if e.type == "OPEN_TCP_PORT" and e.data == "127.0.0.33:8889" ...])
E       assert 1 == 0

This branch changes only workflow YAML, no Python at all, so it cannot be the cause. I reproduced it on unmodified dev serially: 1 failure in 9 runs.

The test discovers 127.0.0.222:8889 and 127.0.0.33:8889 from links on the same page and then asserts on the deduplicated event set. Which of the pair survives dedup depends on arrival order, so occasionally the .33 OPEN_TCP_PORT is not the one kept and the assertion sees zero.

Pre-existing flake on dev, unrelated to this PR and out of scope for it, but worth a separate issue. The other four Python versions passed, and the distro suite passed.

The changes job resolved code=true on this PR, which is correct: it touches workflow files, not just docs, so the full suite ran as intended.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90%. Comparing base (cccc263) to head (40b415b).
⚠️ Report is 14 commits behind head on dev.

Additional details and impacted files

@@          Coverage Diff          @@
##             dev   #3355   +/-   ##
=====================================
- Coverage     90%     90%   -0%     
=====================================
  Files        450     450           
  Lines      46327   46327           
=====================================
- Hits       41587   41576   -11     
- Misses      4740    4751   +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[liquidsec]

The goal is right and the glob is correct. I re-verified every row of your table against picomatch 2.3.1 with {dot: true}, the options paths-filter actually uses, and confirmed from src/filter.ts that a negation-only rule under the default some quantifier behaves the way you describe. The braces-vs-extglob catch is real and well documented.

My concerns are all about the gating mechanism, not the pattern.

Blocking

1. The gate fails open on its own failure.

Your premise is that a skipped job reports success to a required status check. That is exactly what makes this dangerous. test's if: contains no status-check function, so GitHub ANDs an implicit success() over needs. If the changes job fails for any reason (a pulls.listFiles 5xx, a node runtime crash, the token scope tightening), test is skipped, all four required contexts report skipped, and the ruleset is satisfied. A real code PR merges with zero tests run, and the only red mark is a check that isn't required.

The filter needs to fail toward running:

if: always() && (github.event_name != 'pull_request' || needs.changes.outputs.code != 'false')

!= 'false' rather than == 'true' so an empty or missing output runs the suite instead of skipping it. Same change needed at distro_tests.yml.

2. The release path is now coupled to the filter job on push.

The PR says pushes always run the suite, and the if: does scope the skip to pull_request, but needs: changes applies on every event. On push, paths-filter doesn't use the API at all: it defaults base to the repository default branch (stable), computes a merge-base with git over the fetch-depth: 1 checkout, and deepens 100 commits at a time to find it. If that fetch fails, changes fails, test is skipped by the implicit success(), and publish_code and tag_commit go with it. No PyPI dev release, no :dev image, no version tag, and no failing job to signal it since test reads as skipped.

Cleanest fix is if: github.event_name == 'pull_request' on the changes job itself, plus the always() above so test doesn't inherit its status.

3. The skip set includes the only inputs to the test that validates them.

mkdocs.yml and docs/** are what bbot/test/test_step_1/test_docs.py exercises. It calls update_docs(), which does mkdocs_yaml["nav"] at bbot/scripts/docs.py:297 and walks every **/*.md under the repo root. A PR that reorders nav, drops the nav: key, or adds an unregistered !!python/name: tag currently fails that test; after this it merges clean and surfaces on somebody else's unrelated PR, in the nightly docs_updater.yml, or in mike deploy. docs/data/chord_graph/*.json is generated by gen_chord_data() in the same call path and is likewise unguarded.

Simplest resolution is to drop mkdocs.yml from the docs set, and either drop docs/** too or keep it and accept that the docs test no longer gates docs changes.

4. permissions:, listed as a known gap, is more than a forward-looking note.

gh api repos/blacklanternsecurity/bbot/actions/permissions/workflow returns default_workflow_permissions: "write". So today the job takes a write-scoped GITHUB_TOKEN, actions/checkout@v7 persists it into .git/config by default, and dorny/paths-filter@v3 then runs in that same job off a mutable tag. benchmark.yml and cla.yml in this repo already declare explicit permissions:, and the action's own README example uses permissions: pull-requests: read. Adding permissions: {contents: read, pull-requests: read} closes it and also makes the tightening scenario in your known-gaps list a non-issue.

While you're in there, the checkout can go entirely for the PR case. paths-filter's getChangedFilesFromApi never touches the working tree, and the README notes as much. On push it's worse than unnecessary: it's what forces the ~100-commit deepening from stable described in item 2.

Non-blocking

distro_tests.yml doesn't need the job. The whole justification for a changes job over paths-ignore: is the required-status-check interaction, and test-distros isn't in required_status_checks for dev (the ruleset lists only test (3.10) through test (3.13)). That workflow triggers on pull_request only, so a workflow-level paths-ignore: does the same work with no extra runner. benchmark.yml already has the five-line form.

The two copies have already drifted. distro_tests.yml:10-21 and tests.yml:14-25 are byte-identical, but the if: lines differ, and the distro copy has no event guard. If that workflow ever gains a push: trigger, it silently stops running on push. filters: accepts a file path as well as inline YAML, so a single .github/filters.yml referenced from both fixes it. Worth noting codeql.yml also runs unfiltered on docs-only PRs and would want the same file rather than a third copy.

The pattern list depends on an unstated quantifier. With one negated pattern the default some is correct. Add a second exclusion later (say - '!vendor/**') and the rule becomes NOT(docs) OR NOT(vendor), true for every file that isn't both, so code is permanently 'true' and the skip quietly stops working with no error anywhere. Either a comment or an explicit predicate-quantifier would save the next person.

.gitignore and LICENSE are build inputs. hatchling's sdist file selection honours VCS ignore rules, and under PEP 639 license = "AGPL-3.0" pairs with a default license-files glob of LICEN[CS]E*. Both can change what uv build produces in publish_code. The classification is also uneven: .gitignore is docs but sub/.gitignore is code, and LICENSE.md is docs only incidentally.

Truncation fails closed. pulls.listFiles caps at 3000 files. A mass rename or a vendored drop past that cap gets a truncated list, and if the returned slice happens to be all markdown, both matrices skip with no warning in the run.

On the CI failure

Agreed that test_manager_scope_accuracy_correct is unrelated to this PR, and the ordering analysis matches what we've seen. It's tracked separately.

Minor

The branch is based on dev from before #3353, so tests.yml here still shows the pre-xdist pytest invocation. The diff doesn't touch that line so a merge won't revert it, but a rebase would make the file easier to read against current dev.

[liquidsec]

Posted the whole AI review, but my biggest concern is the "fail-passed" nature of it