Repository navigation
fix(ci): guard requirements-ci drift and ansible-vs-CI system packages (#14550, #14551) - #14578
Conversation
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
Review — BLOCKThe measurements are right and I confirmed them independently: production 100 packages, CI mirror 69, missing 54 — and the baseline file's 54 entries are byte-identical to my own computed missing set, zero extra either way. The system-package count of 13 feature packages after excluding the toolchain set also checks out. Two structural problems block, and the first is the sharpest kind: the guard cannot fire on the change it exists to catch. HIGH — the required check is skipped for exactly these inputs
The only mentions of So a PR that removes the This is the epic's own failure shape relocated one layer up into the CI wiring. Add both paths to the filter, and add a test asserting the filter covers every file the guards read — otherwise the next input added to a guard silently escapes the same way. HIGH — the cited precedent has not landed, and this races another open PRThe guard docstrings and PR body say they mirror the All three exist only in the still-open #14510 / #14568. So the guard cites a precedent that does not exist yet, and your Pick one: either drop HIGH — 9 of 54 baseline rationales are factually wrongI asked for this audit specifically, and it matters more than the count. The baseline's stated justification is "no CI-scoped test exercising the real library." For these, a real gate exists today and is silently skipping because the package is absent — the exact vacuous-pass shape #13885 describes and this PR exists to end:
I spot-verified The count being accurate is not the issue. A future maintainer deciding whether a new drift is safe to allowlist will read these rationales and trust them, and 9 of them are false. That is a worse failure than the drift, because it is self-reinforcing. Also unresolved and worth naming: MEDIUM
Verified correctStale-allowlist detection works in both directions and is tested. The ffmpeg canary is real: it asserts only under CI, Fix the paths-filter, resolve the |
Response to BLOCK reviewAll three HIGH findings fixed, plus both MEDIUM items and one severe issue the fix itself surfaced. Summary, with commits: 1. Paths-filter gap (5d77f81)Added Added a third guard, 2. Unlanded precedent + #14510 collision (8bf59dd, fc04f68)Dropped 3. Nine false baseline rationales, plus two more of the same shape (8bf59dd)All 9 (scikit-learn, ldap3, weasyprint, faiss-cpu, datasketch, tree-sitter-python, trafilatura, yt-dlp, ddgs) mirrored into the matching Auditing
Net: baseline shrank from 54 to 44 entries; MEDIUM — both fixed (8bf59dd)
Unplanned but necessary (fc04f68)Installing ffmpeg in CI let Final numbers
All three |
Re-review — block cleared, APPROVEAll three blockers are addressed, and the work turned up something that justifies the whole issue. The finding that proves the thesis
Verified against base: That is this issue's argument demonstrated rather than asserted. "The tests for that capability have been passing vacuously" was the hypothesis; a completely non-functional production method is the instance. Worth remembering the next time a skip looks harmless. The three blockers1. Paths-filter gap — fixed, and generalised. The three inputs are added to both 2. The 3. The nine false rationales — fixed, and the audit found two more. All nine mirrored with per-test citations, and applying the same method to Baseline 54 → 44, matching 46/100 → 56/100. The number moved because real omissions got fixed rather than better justified — which is the distinction I was asking for. Correcting The mediumsThe missing One note for the recordThe ffmpeg fix is a production behaviour change riding in a CI-hygiene PR. That is justified here — you cannot enable a test and leave it red, and the alternative was shipping a guard that immediately fails — but call it out plainly in the body so a future bisect knows audio extraction changed in this commit. Same for the 841-file scanning correction: both are load-bearing changes that a reader skimming for "CI config" would miss. Good work. The guard-reach checker in particular is the kind of fix that stops the class rather than the instance. |
python-suite failures (shards 2, 5, 11, 12) — all root-caused and fixedHypothesis confirmed for shards 2/5: it is the same class as the ffmpeg finding — mirroring a package revealed a code path that was never really exercised. Two more genuine, distinct defects surfaced by the same mechanism (shard 11), and one CI-infra regression from this PR's own ffmpeg fix (shard 12). None were pre-existing; none are fixed by unmirroring anything. Shards 2 + 5 —
|
Re-review — block cleared, pending the CI runFour shards, four distinct causes, all correctly root-caused. Verified the fixes at head. Shards 2 and 5 — the hypothesis was right, and the reason is better than I guessed. Shard 11 — a genuine test bug, and a good catch. Shard 12 — not a cascade, a timeout. The bonus find is one this repo keeps producing. Your own checker mis-tokenised words out of its explanatory comment — which contains the literal phrase "apt-get install" — as installed package names. A guard tripping over its own prose about the pattern it guards is a shape we have hit before with lint fixtures; stripping full-line comments before scanning is the right fix. And the pre-existing check answers the question properly. Those three tests pass on #14589's 12/12-green run only because the packages are absent and every real-library path is Everything from the earlier rounds still stands — the guard-reach checker, the self-correcting I will merge once the required contexts and all twelve shards confirm green. Given the history on this PR, I am checking failed shards by name rather than counting successes. |
Fixed — composite-action schema break (3f4480c)Confirmed the diagnosis: Fixed by bounding the command instead of the step, exactly as suggested: export DEBIAN_FRONTEND=noninteractive
timeout 180 sudo apt-get update -qq -o DPkg::Lock::Timeout=60
timeout 180 sudo apt-get install -y --no-install-recommends -o DPkg::Lock::Timeout=60 ffmpeg
The two things beyond the immediate fixStated in the commit, and here: "verified clean locally at HEAD" was true and did not cover this — no local check parses Filed #14632 rather than guessing at scope: it asks whether In the meantime, landed a verifiable, no-external-tool-needed version in this PR: All 65 local |
Re-review — block cleared, pending greenThe fix is right and the response to it is better than the fix.
Three things you did beyond the fix, and the third is the one that matters. You verified And then you closed the gap in this PR anyway, without waiting for that answer: That is the right instinct. A filed issue is a promise; a guard in the required check is the thing that stops the next one. Filing and closing it is better than either alone — and mutation-testing it against the exact incident shape means it demonstrably catches the defect that motivated it, not merely something adjacent. The honesty note is worth as much as the code. Documenting that "verified clean locally" never covered this class — because nothing local parses Everything from the earlier rounds remains verified and in place: the explicit Merging once all twelve shards and the required contexts confirm green — checked by failed shard name, not by counting successes. |
Fixed — right diagnosis, evidence-checked before choosing the fix (b677521)Confirmed the mechanism before picking a number, rather than guessing bigger. Checked Conclusion: the applicable case is your second one — twelve cold shards still race. What they share isn't a resource, it's a risk: each VM independently rolls the dice against its own background Added the The actual fix is more headroom, sized to evidence:
Considered and rejected: hoisting the install to run once via Canary confirmed unweakened: All four |
Correction: my diagnosis was wrong, and you were right to check itI told you twelve shards were contending for one runner's dpkg lock. That is not what is happening, and you established it rather than accepting it: So the real mechanism is twelve independent rolls against each VM's own That is the right way to take a coordinator's hypothesis. I would rather be corrected than have you build on my wrong model. And you were honest about what your own fix does. Saying plainly that the Sizing it to the documented range Canary confirmed unweakened: Handling the auto-pushed merge commit by fast-forwarding cleanly rather than rewriting was also correct. Merging once all twelve shards confirm green — by failed-shard name, not by counting successes. |
|
Cross-link: the path-filter defect found here is one of three in a single session, now filed as a class in #14659 — the other two were |
Retry loop in place (f9d1e0c)Agreed with the framing — raising the timeout again would have been guessing, not reasoning. Implemented the retry exactly as suggested, both for `apt-get update` and `apt-get install`, with a modest linear backoff (15s/30s/45s): ```bash apt_retry timeout 300 sudo apt-get update -qq -o DPkg::Lock::Timeout=120 Exhausting the retries still fails the step — didn't just reason about this, verified it empirically with an isolated two-mode harness (a fake command standing in for apt, one variant succeeding on the 2nd attempt, one always failing) before trusting the real commands to it: ``` === always_fail === The mechanism: `apt_retry`'s final `return 1` is a bare top-level statement, not wrapped in an `if`/`&&`/`||` — so `set -e` propagates it as the step's own exit status. Confirmed the "always_fail" path never reaches the line after the call. Canary confirmed untouched — `git diff --stat` against this commit shows zero changes to `ffmpeg_service_test.py`. `test_ci_actually_has_the_ffmpeg_toolchain` still asserts `shutil.which("ffmpeg") is not None` unconditionally under `CI=true`, independent of how (or whether) the retry loop succeeded. On the lazy-install alternative — considered it, staying with retry. `pytest-split` assigns shards dynamically from a duration file, so which of the 12 shards ends up with `ffmpeg_service_test.py` isn't knowable statically. Making the install lazy would mean running a `--collect-only` dry pass matched exactly to the real run's `--splits`/`--group`, in the composite action, before the real venv/test run — any mismatch between that dry pass and the real split would silently reintroduce the vacuous-skip bug this whole PR exists to close. It would also only cover ffmpeg specifically; `tools/lint/check_ci_system_package_provisioning.py` already generalizes this pairing across every ansible-installed binary a test might need, and a per-test lazy-install path would need re-deriving for the next one. Retry doesn't need to know which shard needs what, is much smaller, and is the one I'd trust not to introduce a new silent-failure mode. All four `--audit` guards and 65 local tests pass at the pushed HEAD (`f9d1e0c18`); `repo_tests/` collection clean at 959 tests. |
Rebased onto origin/Dev_new_gui, conflict resolved, and the guard caught something real (7687863)Rebase resolved cleanly for 11 of 12 commits; the one conflict was exactly where you said — `.github/workflows/code-quality.yml`'s `changes.filters.backend` block, where #14650 and this PR both add entries. Took both sets: #14650's corrected role `files/` paths + the two config files, and this PR's ansible task path + composite-action glob + `repo_tests/`. Nothing dropped from either side. Ran `check_code_quality_guard_reach.py --audit` immediately after, per your instruction to trust it over the manual resolution — it failed. Not because the merge was wrong (the YAML is valid, all 19 patterns are present and correct, confirmed with `yaml.safe_load`) but because the checker's own line-oriented re-parser had a latent bug the merge was the first thing to expose: `backend_filter_patterns()` treated any non-bullet line as the end of the list once it had collected at least one bullet. #14650 is the first PR to ever put a multi-line comment inside that literal `filters: |` block (previously bare bullets only) — its comment sat between two bullets, and the parser silently truncated right there, reading everything after it, including this PR's own three guarded paths, as uncovered. Fixed `backend_filter_patterns()` to skip blank lines and full-line `#` comments instead of treating them as terminators — matching what dorny/paths-filter itself does (it parses this block as real YAML and is unaffected by comments). Added `test_backend_filter_patterns_survives_a_comment_mid_list`, and didn't just trust it: reverted the fix locally, confirmed the test fails with `['before/'] == ['before/', 'after/**']` against the old parser, restored the fix, confirmed it passes. All four `--audit` guards clean, 66 local tests pass (14 skip, no local faiss/ffmpeg install), `repo_tests/` collection clean at 968 tests. PR `mergeable` is now `MERGEABLE` (confirmed via the API) and CI has started running at the pushed HEAD (`7687863c9`) — will report the shard results once they land. |
CI results after the rebase: 10/12 shards — the 2 failures are pre-existing on base, already trackedAll 10 required contexts green, `mergeable: MERGEABLE` confirmed. `python-suite`: 10/12 shards pass. Shards 2 and 7 fail, but neither is caused by this PR — checked rather than assumed: Both failures are the same defect, in code this PR never touches: ``` shard 7: autobot-slm-backend/tests/services/test_wizard_inventory_parity_14286.py Both are about `role_llm_active`/node-role provisioning logic — nothing in `#14550`/`#14551`'s scope (CI-drift guards, ffmpeg, tree-sitter, faiss, composite-action step keys) touches that code at all. Traced it to a specific base commit, not a guess: `2ba36f1cb5` (#14567) — "derive the deploy gate over the role map so siblings are covered" — is one of the 8 commits between my old merge-base and `origin/Dev_new_gui` that this rebase pulled in. It widened `_DECLARED_ONLY_GROUPS` to cover `role_llm_active` without updating `role_active_facts.yml` to gate that fact on declared-only roles, so the fact still reads the raw union. Already found and filed, independently, before I looked: #14666, opened 2026-08-21T08:09:35Z — same two test names, same root cause, explicitly states "failing on base... within hours." This is the fifth round of this same shape per that issue's own history (#14513, #14560/#14589, #14594/#14637, #14638/#14649, now #14666) — a pre-existing, already-tracked base defect, not something this rebase introduced or something in scope for #14550/#14551 to fix. Not attempting a fix here: it's provisioning/ansible-role logic, a different owner and a different issue, and #14550/#14551's job is CI-drift guards. Fixing it in this PR would be exactly the kind of scope creep the "split by severity" rule warns against — flagging it with evidence is the correct action, not silently working around it. 10/10 required checks green, 10/12 python-suite shards green (not required), and the 2 remaining failures are a confirmed pre-existing base issue already tracked at #14666 — evidence above, not assumption. |
…act (#14550) autobot-backend/requirements.txt and autobot-slm-backend/requirements.txt declare production; requirements-ci.txt is a hand-mirrored copy setup-python-suite actually installs, and nothing checked the two stayed in sync. pytesseract sat declared in production and undeclared in CI for months (#13885) while every real OCR test skipped or ran against a stub. Adds tools/lint/check_requirements_ci_drift.py, which compares the parsed package sets and fails on any production package missing from CI that is not recorded in the new repo_tests/requirements_ci_drift_baseline.txt allowlist -- seeded from today's actual 55-package diff, minus pytesseract (fixed here by mirroring it into requirements-ci/automation.txt instead of allowlisting the exact incident this guard exists for). The allowlist only ever shrinks: an entry CI now installs, or that production no longer declares, is a hard error too. Wired into code-quality.yml (a required check) with repo_tests/requirements_ci_drift_test.py exercising the same functions the guard calls.
…#14550) ansible installs apt packages on every backend host (GUI/VNC, audio, TTS, OCR, pg_dump) that setup-python-suite never provisions -- a test gated on one of those binaries via shutil.which(...) has been silently skipping in CI, indistinguishable from a pass in the job summary. media/audio/ffmpeg_service_test.py::test_real_audio_extraction was the one live instance found: skip-gated on shutil.which("ffmpeg") is None, and ffmpeg was never installed on any CI runner, so it skipped "FFmpeg not installed" on every run. Fixed by installing ffmpeg in .github/actions/setup-python-suite/action.yml, and pinned with a new test_ci_actually_has_the_ffmpeg_toolchain that asserts the binary is present under CI=true instead of allowing another silent skip, mirroring the pattern #13896 introduced for the OCR toolchain. Adds tools/lint/check_ci_system_package_provisioning.py, which scans the test tree for shutil.which(...) skip-gates on an ansible-installed binary and fails if the matching apt package is not installed by setup-python-suite -- so a future regression (or a new gated test added without the matching install) fails the required code-quality check instead of skipping quietly. repo_tests/ci_system_package_provisioning_test.py exercises the same functions the guard calls.
) code-quality only does real work when its changes job's dorny/paths-filter reports a backend path touched -- otherwise the job is SKIPPED, and branch protection treats a skipped required job the same as a passing one. Neither autobot-slm-backend/ansible/roles/backend/tasks/main.yml (the system-package guard's own input) nor .github/actions/setup-python-suite/action.yml (the file both new guards exist to protect) was covered by that filter, so a PR touching only one of them -- e.g. deleting the ffmpeg apt line #14550 just added -- would skip code-quality entirely and neither --audit step would ever run. Adds both paths (and repo_tests/**, which already held baseline files no entry covered either) to both the on.push.paths list and the changes job's filters.backend list. Adds tools/lint/check_code_quality_guard_reach.py, a meta-guard: each checker now declares its own GUARD_INPUT_PATHS, and this module re-derives the path filter's actual reach from code-quality.yml (parsed without a YAML library, since code-quality installs linters only) and fails if any guarded path is not covered. Wired into code-quality.yml itself, which is always in scope. repo_tests/code_quality_guard_reach_test.py exercises the same functions, including a regression test pinning the `**/foo` root-level-match bug this checker's own development caught in itself.
#14551) Code review found the requirements-ci-drift baseline claiming "no CI-scoped test exercising the real library" for 9 packages that had a real pytest.importorskip / importlib.util.find_spec gate, silently skipping -- the exact vacuous-pass shape this guard exists to end. Mirrors each into the matching requirements-ci/*.txt file instead of leaving it baselined: scikit-learn, ldap3, weasyprint, faiss-cpu, datasketch, tree-sitter-python, trafilatura, yt-dlp, ddgs. Auditing autobot_shared/ (the highest-risk shared surface, reached by both backends) the same way found two more of the same class that were not gated by a skip at all: bcrypt and pydantic-settings are unconditionally imported by autobot_shared/auth/jwt_core.py and autobot_shared/ssot_config.py respectively -- both declared in autobot_shared/pyproject.toml but never installed from there by CI (the same shape as the pre-existing PyJWT[crypto] comment in requirements-ci/security.txt), so importing either module, and therefore nearly anything that imports config or auth, was liable to fail collection outright. Mirrored rather than baselined. Resolves the pytesseract/#14510 collision: #14510 (open) already adds pytesseract to requirements-ci/document.txt with its own OCR-fallback rationale. Dropped the automation.txt addition from this PR and allowlisted pytesseract instead, pointing at #14510 -- the audit will force removal of that line the moment either PR lands it for real. Corrected the opencv-python-headless rationale: it is not actually an omission. requirements-ci/automation.txt already declares plain opencv-python, which installs the identical `cv2` import module computer_vision/template_matching_test.py's importorskip("cv2") needs. Widened tools/lint/check_ci_system_package_provisioning.py's detection past the literal shutil.which("x") shape: pytest.importorskip( is now also a recognised skip marker (the style scikit-learn/ldap3/etc. use), and a small PROBE_CALL_TO_BINARY table catches indirect probes like pytesseract.get_tesseract_version() that never call shutil.which at all -- so a real tesseract gate landing later is caught the day it appears instead of reading as "no real test, out of scope". Also fixed _test_files() to match pytest.ini's own `test_*.py` prefix convention, not only `*_test.py` suffixes -- the guard was under-scanning relative to what pytest actually collects. Reworded the docstring in check_ci_system_package_provisioning.py: it cited test_ci_actually_has_the_ocr_toolchain as an established #13896 pattern, but #13896 is still open (#14510/#14568) as of this writing -- corrected to describe the pattern without citing unlanded code.
Enabling ffmpeg in CI (this PR, #14550) let test_real_audio_extraction run for the first time instead of permanently skipping -- and it failed. FFmpegService.extract_audio() passed `-allowed_extensions` as a global ffmpeg CLI flag ahead of `-i`, but that option is a private AVOption of the dash and hls DEMUXERS only (`ffmpeg -h protocol=file` lists no such option for a plain file input; `ffmpeg -h full` shows it scoped to "dash"/"hls"). With no demuxer context that defines it, ffmpeg rejected the whole command with "Option allowed_extensions not found" -- extract_audio() has been completely non-functional for every real audio file, on every ffmpeg version, since Issue #9214 added it. Invisible until now because the only test that ever invoked real ffmpeg was itself silently skipping in CI for want of the binary: a skip masking a failure masking another failure. The actual extension enforcement already happens at the application level, before ffmpeg is ever invoked (`if input_ext not in ALLOWED_EXTENSIONS: raise ValueError(...)`), and is sufficient on its own -- removes the broken flag rather than trying to make it work, since ffmpeg has no mechanism to honour it for a plain file input regardless of version. Also corrects ffmpeg_service_test.py's new test_ci_actually_has_the_ffmpeg_toolchain docstring, which cited test_ci_actually_has_the_ocr_toolchain as an established #13896 precedent; #13896 is still open as of this writing (code review caught this racing an unlanded PR) -- reworded to describe the pattern without citing unlanded code. Verified locally: test_real_audio_extraction failed against the real fix before this change (RuntimeError: Option allowed_extensions not found) and passes after it; all 9 tests in ffmpeg_service_test.py pass under CI=true.
…4550) ci_installed_packages() excluded a fixed list of known apt flags (-y, --yes, -qq, sudo) but not --no-install-recommends (added earlier in this PR's own setup-python-suite change), so it briefly reported that flag as an "installed package" alongside ffmpeg -- harmless to the audit result today (nothing checks a flag against BINARY_TO_PACKAGE), but the next unrecognised flag would have been just as silently wrong. Excludes any token starting with `-` generically instead of enumerating flags by hand.
…ght it (#14551) Shard 2 and shard 5 of python-suite went red on the pushed PR: shared_identity_test.py, code_indexer_test.py, code_indexer_extensions_test.py and impact_analysis_test.py all failed with "the graph extractor produced no ids at all" / empty node sets, root-caused to "ERROR services.knowledge.code_indexer: tree-sitter-python not installed — AST indexing disabled: No module named 'tree_sitter'". tree-sitter-python was mirrored into requirements-ci/document.txt on the assumption it declares `tree-sitter` as a runtime dependency (the baseline comment said so). It does not -- the two ship as siblings, not a dependency pair, and code_indexer.py's `from tree_sitter import Language, Parser` (after `import tree_sitter_python` succeeds) failed on the real runner. Local review never caught this because the checker's own audit only proves "declared in some requirements-ci/*.txt file", never resolves the actual dependency graph -- an assumption about transitive resolution is unverified until CI actually installs it. Mirrors `tree-sitter>=0.26.0` explicitly into requirements-ci/document.txt, removes it from the allowlist, and corrects the file's own comment (which claimed torch's SAME transitive-resolution pattern applied here) to record what was actually checked for torch versus what was merely assumed for tree-sitter. Reproduced the exact bug and fix with an isolated minimal repro (a two-line module + mock.patch, independent of both faiss and tree-sitter) before trusting the diagnosis.
…ope (#14551) Shard 11 of python-suite went red on the pushed PR: test_index_file_written _after_train failed asserting a persisted index file existed, when faiss-cpu was mirrored into CI (#14551) and pytest.importorskip("faiss") stopped skipping this test for the first time. FAISSIVFPQBuilder._index_path() embeds the module-level IVFPQ_NLIST constant in the filename, read as a bare global at call time (not captured once at __init__). The test's `with patch("...IVFPQ_NLIST", 8): ...` block wrapped the training call, which wrote the file under the patched value (n8), but the two assertions sat AFTER the block had already exited -- they computed the path with the real, unpatched IVFPQ_NLIST=1024, checking a filename that was never written. Production is unaffected: IVFPQ_NLIST is never mutated at runtime outside this test, so the same real path is always used to write and read. Fixed by keeping the assertions inside the same patch scope as the write, which is what the test always intended to check. Verified with an isolated two-line reproduction (module-level constant + mock.patch, no faiss involved) showing the exact same false-vs-true result before and after the fix, since faiss-cpu is not installed in this environment and this PR does not pip install.
… shard (#14550) Shard 12 of python-suite showed `cancelled` on the pushed PR. Its setup-python-suite step ran for exactly 60 minutes (19:21:40 to 20:21:35) before being cut off -- ci.yml's python-suite job has `timeout-minutes: 60`, so the job-level timeout fired while the shard was stuck inside "Install CI system packages", the composite-action step this PR's ffmpeg fix added. The bare `apt-get install` had no bound on how long it would wait for the dpkg lock, almost certainly held by the runner image's background apt-daily / unattended-upgrades timer -- the same class of GitHub-hosted-runner flakiness ansible's own apt-get update is already hardened against (roles/nginx/tasks/main.yml, #6719). One stuck shard took every test in it down, not just the apt step. Adds `-o DPkg::Lock::Timeout=60` to both apt-get invocations (apt's own documented option for waiting on the lock instead of an unbounded hang) and `DEBIAN_FRONTEND=noninteractive` (rules out any interactive prompt as a second hang source), plus a step-level `timeout-minutes: 3` as the second line of defense -- if the lock is still held past that, this ONE step fails fast with a clear error instead of silently spending the whole job's budget. Auditing this change against tools/lint/check_ci_system_package_provisioning.py found the checker's own `ci_installed_packages()` mis-tokenizing the fix: its `apt(-get)? install` regex matches anywhere in the file, including inside a prose comment EXPLAINING an apt-get command -- this PR's own explanatory comment about the incident, containing the literal phrase "apt-get install", leaked words from that sentence into the installed-package set. Fixed by stripping full-line comments before scanning, and excludes any `key=value` /`Key::Path=value` token (an apt `-o` option's value, not a package name) as a second layer. Regression-tested with the exact failure shape (a synthetic action.yml whose only "apt-get install" occurs inside a comment).
… one (#14550) `timeout-minutes` on the "Install CI system packages" step (added to bound the apt lock-wait, previous commit) is not a valid key on a composite action's own step -- that key exists on a job, and on a workflow step, but `.github/actions/*/action.yml`'s `runs.steps` accept only `run`, `shell`, `working-directory`, `env`, `id`, `if`, `name` (plus `uses`/`with`). GitHub's template validator rejected the WHOLE action.yml before a single step ran: Unexpected value 'timeout-minutes' GitHub.DistributedTask.ObjectTemplating.TemplateValidationException: The template is not valid. Every python-suite shard using the action failed identically -- not only the one shard the change was meant to protect. Nothing local caught it: no local check parses action.yml the way GitHub's server-side template engine does, so a schema error in a composite action is invisible until CI actually loads it -- true of the checks in this PR too, and worth being explicit about rather than letting local "all clean" stand in for that class of coverage. Fixed by bounding the COMMAND instead of the step: coreutils `timeout 180` around each apt-get invocation. `DPkg::Lock::Timeout=60` and `DEBIAN_FRONTEND=noninteractive` were correct and stay; only the step-level key had to go. A `timeout` exit (124) is a real failure here, deliberately -- a wedged apt means the toolchain this PR exists to provision is absent, and the gated test would pass vacuously again. Adds tools/lint/check_composite_action_step_keys.py: a stdlib-only, required-check guard (wired into code-quality.yml) that re-implements GitHub's small, stable composite-action step-key allowlist and checks every `.github/actions/*/action.yml` in the repo -- not only the one this incident hit. Widens code-quality.yml's path filter from the one composite action to `.github/actions/**` (all three that exist today) to match. Verified the checker rejects the exact incident shape and accepts the fixed one; repo_tests/composite_action_step_keys_test.py carries both, plus the nested-`with:`-block and embedded-heredoc parsing edge cases that a first pass at the regex got wrong (missed an inline `- key: value` on the dash line itself, and a naive substring check against this file's own explanatory prose about the incident, containing the literal text "timeout-minutes: 60"). Filed #14632 to confirm whether actionlint -- present in this repo but triggered only on `.github/workflows/**` -- validates composite-action step schemas at all, and if so, to widen its trigger and consider promoting it to a required check. That is a broader, more general form of the same coverage this commit adds narrowly and verifiably without depending on an external tool being available.
…old VMs (#14550) 6 of 12 python-suite shards failed after the previous fix, one with exit 124 -- `timeout 180` killed `apt-get` before it finished; the command never failed on its own. Checked the actual mechanism rather than assuming a shared lock: ci.yml's own comment on the python-shard job says it runs on `ubuntu-latest`, "NOT the singleton [self-hosted, Linux, X64] pool" -- so each of the 12 shards is a SEPARATE, ephemeral GitHub-hosted VM. There is no dpkg lock literally shared BETWEEN shards, and no warm state carried over run-to-run either (hosted runners are single-use). What twelve independent VMs share is the same risk, not the same resource: each rolls the dice against its own background apt-daily/unattended-upgrades timer independently, and with twelve simultaneous trials several are expected to land inside that timer's window. 180s was not enough headroom for that. Adds the suggested `command -v ffmpeg` short-circuit -- free, and correct defensive practice against a future base image or runner pool where it would actually help -- but confirms explicitly that it does NOT fix today's failures by itself: ffmpeg is absent from ubuntu-latest today, which is this whole PR's own starting point (`shutil.which("ffmpeg")` returning None is what made test_real_audio_extraction silently skip in the first place), so every shard still takes the apt path regardless. The actual fix is more headroom, sized to evidence rather than guessed: `-o DPkg::Lock::Timeout` 60s to 120s (the well-documented range apt-daily typically holds the lock on a freshly booted Ubuntu VM), and the outer `timeout` 180s to 300s per command -- 10 minutes worst-case across both apt-get calls, a small fraction of the job's 60-minute budget. Considered and rejected: hoisting the install to run once and sharing it across shards via actions/cache. Feasible in principle (cache the ffmpeg binary and its shared-library closure, restore without touching apt at all), but meaningfully more complex than a timeout increase, and carries its own risk -- a cached binary built against a different base-image's library versions failing in a more confusing way than an apt timeout does. Not pursued given the simpler fix addresses the actual failure. test_ci_actually_has_the_ffmpeg_toolchain is untouched and unweakened: it asserts `shutil.which("ffmpeg") is not None` under CI=true regardless of whether ffmpeg arrived via the short-circuit or a fresh install, and still fails loudly if provisioning genuinely fails.
…meout (#14550) 11 of 12 shards passed after the previous headroom increase; one still hit exit 124. Raising the timeout again would be guessing -- a single attempt losing to a background lock timer says nothing about whether a second attempt would succeed, which is exactly the case a retry is for. Wraps both `apt-get update` and `apt-get install` in a 3-attempt retry with a modest, deliberately short linear backoff (15s/30s/45s) -- long enough for `apt-daily`/`unattended-upgrades` to release the lock on its own schedule, short enough not to burn meaningful job budget. Exhausting all 3 attempts still fails the step: `apt_retry`'s own `return 1` is a bare top-level statement (not wrapped in an `if`/`&&`/`||`), so `set -e` propagates it as the step's exit status -- verified empirically with an isolated two-mode harness (a fake command that succeeds on the 2nd attempt, and one that always fails), confirming both the "retry then continue" path and the "exhaust retries then abort" path do exactly what was intended before trusting the real apt commands to it. Considered making the install lazy -- only the one shard whose pytest-split group happens to contain ffmpeg_service_test.py would install it, cutting exposure from twelve VMs to one. Concluded against it: pytest-split assigns shards dynamically from a duration file, so which shard gets that test is not knowable statically without running a `--collect-only` dry pass matched exactly to the real run's --splits/ --group -- a mismatch there would silently re-introduce the vacuous-skip bug this PR exists to close, and the pairing this generalizes for EVERY ansible-installed binary already lives in tools/lint/check_ci_system_package_provisioning.py; a per-test lazy-install path would only cover ffmpeg and need re-deriving for the next one. Retry is the safer default and does not require knowing which shard needs what. test_ci_actually_has_the_ffmpeg_toolchain is unmodified (confirmed via `git diff --stat` against this commit): it stays the backstop that fails loudly under CI=true if the retry logic itself is ever wrong.
#14550) Rebasing onto origin/Dev_new_gui merged #14650's addition to the SAME code-quality.yml filter block this PR edits -- both PRs add entries to the `changes` job's `filters.backend` list, and #14650's is the first commit to ever put a multi-line comment INSIDE that literal `filters: |` string block (previously bare bullets only). The merge itself was correct: both sets of additions are present, valid YAML, all 19 patterns real. check_code_quality_guard_reach.py's own re-parser was not correct against that shape, though: backend_filter_patterns() treated any line that was not a `- 'pattern'` bullet as the end of the list, once at least one bullet had been collected. The inserted comment block satisfied that "first non-bullet line" condition and silently truncated the parse right after `autobot-infrastructure/shared/scripts/**` -- reading every entry after it, including this PR's own three guarded paths, as uncovered. Running the checker immediately after the rebase (per review guidance: trust it over the manual resolution) is what caught this. dorny/paths-filter itself parses this block as real YAML and is unaffected by comments -- the bug was specific to this checker's line-oriented mirror, not the workflow. Fixed by skipping blank lines and full-line `#` comments instead of treating them as terminators, so only a genuine non-bullet, non-comment line ends the block. repo_tests/code_quality_guard_reach_test.py::test_backend_filter_patterns_survives_a_comment_mid_list reproduces the exact shape (a comment between two bullets) and was confirmed to fail against the pre-fix parser before being trusted: reverted the fix locally, watched it fail with `['before/**'] == ['before/**', 'after/**']`, restored the fix, watched it pass.
12/12 python-suite shards green, code-quality green, rebase clean (b654327)Rebased onto `origin/Dev_new_gui` a second time, now including `3e78d7438` (#14664, reverts the deploy-gate closure) and `4ec406d25` (#14665, formatting fix) — no conflicts this time, all 13 commits applied cleanly. Result:
Confirmed the discrimination test for the guard-reach parser bug (`test_backend_filter_patterns_survives_a_comment_mid_list`) is committed in the PR, not just scratch-verified — it's part of `b654327` (the same commit as the fix), and re-ran it standalone after this rebase to confirm it still passes against the current parser. All four `--audit` guards clean, 66 local tests pass (14 skip, no local faiss/ffmpeg install), `repo_tests/` collection clean at 968 tests — all reverified at the pushed HEAD after this rebase, not carried over from the previous round. |
1 participant
Closes #14550, #14551
Thinking Path
Both issues describe the same shape of bug: the CI environment can diverge
from what production actually runs, and nothing notices — once as Python
packages (#14551), once as apt system packages (#14550). #13885 already
showed the concrete cost once:
pytesseractsat declared in productionrequirements and absent from CI for months, every OCR test either skipped or
ran against a mock, and the suite stayed green throughout.
Measured the drift before changing anything, then a code-reviewer pass on
the first version of this PR found the guards' reach was itself the
weaker link — a required check that can be skipped for the exact files it
protects, a baseline with 9 false rationales, an unlanded precedent cited as
established, and (once fixed) a genuinely broken production code path the
fix exposed. All of that is folded into what's described below; see the
review thread for the point-by-point response.
#14551 (Python packages): parsed
autobot-backend/requirements.txt+autobot-slm-backend/requirements.txt(production) againstrequirements-ci.txt→requirements-ci/*.txt(CI). Final state:56/100 matching. 44 packages are genuinely absent and allowlisted with a
verified rationale each (SLM-only auth/SSO libs the CI-scoped suite mocks
around, OpenTelemetry instrumentation reached by no collected test,
torch/tree-sitterresolved transitively, LangChain/LlamaIndex extras,optional lazy-imported libraries). 11 more were found to have a real
skip gate and were mirrored into CI instead of allowlisted:
scikit-learn,ldap3,weasyprint,faiss-cpu,datasketch,tree-sitter-python,trafilatura,yt-dlp,ddgs,bcrypt,pydantic-settings.pytesseract(the original #13885 incident) ended up allowlisted for a different
reason — see below.
#14550 (system packages): the backend ansible role installs 13 feature
system packages (GUI/VNC, audio, TTS, OCR,
pg_dump; excluding thePython-build toolchain, which CI never needs since
actions/setup-pythonships a prebuilt interpreter). Cross-referencing every
shutil.which(...)/pytest.importorskip(...)/ known indirect-probe skip-gate in the test treeagainst those 13 packages found exactly one live instance:
ffmpeg_service_test.py::test_real_audio_extraction, silently skipping onevery CI run — 0/1 provisioned before the fix, 1/1 after. Enabling
it surfaced a second, more serious problem: the function it exercises was
completely broken (see "Unplanned but necessary" below). The other 12
ansible packages have no CI-scoped test needing the real binary today.
Decided against merging the two dependency sets (explicitly out of scope
per #14551: the role-based CI split is deliberate, and CI legitimately omits
heavy production deps like
torch, which arrives transitively). The fix isa guard against silent divergence, not a one-time sync — seeded from the
real diff, with a policy for what a fresh diff means:
(mirror into CI). Never allowlist an actual gap — that was the mistake in
round one of this PR, corrected after review.
package's real import name for a skip-gate or an unguarded top-level
import in
autobot_shared/→ record it explicitly inrepo_tests/requirements_ci_drift_baseline.txt, in the style ofsys_modules_leak_baseline.txt/extension_import_baseline.txt. A staleentry (CI now installs it, or production no longer declares it) is a hard
error.
different reasons → don't race it.
pytesseractcollides with the still-openfeat(document): rasterize-then-OCR fallback for PDFs with no text layer (#13896) #14510 (adds it to
requirements-ci/document.txtfor the OCR-fallbackfeature); allowlisted here with a pointer at feat(document): rasterize-then-OCR fallback for PDFs with no text layer (#13896) #14510 instead, and the audit
will force that line's removal the moment either PR lands it for real.
What Changed
tools/lint/check_requirements_ci_drift.py(#14551) — parses productionvs CI requirement sets (stdlib only, following
-rincludes), diffs themagainst
repo_tests/requirements_ci_drift_baseline.txt, and fails on aproduction package missing from CI and not allowlisted, or a stale
allowlist entry. Asserts both parsed sets are non-empty before comparing.
repo_tests/requirements_ci_drift_baseline.txt— 44 entries, grouped bycategory with a verified rationale per group (and per-package where the
category alone doesn't establish it).
requirements-ci/{ai-ml,document,networking,security,framework}.txt—mirror the 11 packages above, each with a comment naming the exact test(s)
that were silently skipping without it.
tools/lint/check_ci_system_package_provisioning.py(#14550) — derivesthe ansible role's feature-package set (excluding a fixed, documented
Python-build-toolchain set), scans the test tree — both
*_test.pyandtest_*.py, matchingpytest.ini's own collection patterns — for a knownbinary reached via
shutil.which(...)or a known indirect probe call(
PROBE_CALL_TO_BINARY), gated byskipif/pytest.skip(/importorskip(,and fails when the matching apt package isn't installed by
.github/actions/setup-python-suite/action.yml. Asserts floors on both theansible package count and the number of gated binaries found.
tools/lint/check_code_quality_guard_reach.py— a third guard: eachchecker declares
GUARD_INPUT_PATHS; this one re-derivescode-quality.yml's actual path-filter reach (parsed without a YAMLlibrary —
code-qualityinstalls linters only) and fails if any guardedpath isn't covered. Closes the gap where a PR touching only the ansible
task file or the composite action skipped
code-qualityentirely — askipped required job satisfies branch protection the same as a passing one.
.github/workflows/code-quality.yml— wires all three checkers in with--audit; addsautobot-slm-backend/ansible/roles/backend/tasks/**,.github/actions/setup-python-suite/**, andrepo_tests/**to bothon.push.pathsand thechangesjob'sfilters.backendlist..github/actions/setup-python-suite/action.yml— installsffmpeg.Composite action referenced by local path (
uses: ./.github/actions/...);its steps execute from the checkout of this PR's own merge commit, so this
takes effect on this PR's own CI run, not only after promotion to
main.autobot-backend/media/audio/ffmpeg_service.py— removes a broken-allowed_extensionsglobal ffmpeg flag (a private AVOption of thedash/hlsdemuxers only; ffmpeg rejects the whole command for a plainfile input on every version).
extract_audio()has been non-functionalfor every real audio file since Issue #9214 added it — invisible because
the only test that ever invoked real ffmpeg was itself silently skipping.
Extension enforcement already happens at the application level above it.
autobot-backend/media/audio/ffmpeg_service_test.py— addstest_ci_actually_has_the_ffmpeg_toolchain, asserting (underCI=trueonly) that
ffmpegis onPATH.test_real_audio_extractionitself isunchanged; it now runs for real instead of skipping.
repo_tests/requirements_ci_drift_test.py,repo_tests/ci_system_package_provisioning_test.py,repo_tests/code_quality_guard_reach_test.py— import the checkermodules by path, exercise the invariant against synthetic input,
discrimination against a synthetic on-disk tree, the anti-empty-result
floors in both directions, the live tree (mirrored packages present and
not allowlisted, the pytesseract/#14510 pointer, ffmpeg provisioned, the
probe-call and prefix-file-glob widenings), that
code-quality.ymlactually invokes each
--audit, and that none of the three checkersimports anything beyond the stdlib.
Required-check status, stated explicitly: all three guards run via
python3 tools/lint/check_*.py --auditinsidecode-quality.yml, whichis one of the 10 required contexts on
Dev_new_gui.python-suite,which the
repo_tests/*_test.pycopies also run under, is notrequired — those are the informational mirror; enforcement is the
--auditinvocation incode-quality.yml.Verification
Mutation evidence, each: mutate → red → revert → green, verified this
session against the pushed HEAD:
pytesseractfromrequirements-ci/automation.txt(reproducing fix(deps): pytesseract binding never installed — every OCR surface is dormant #13885) → 1 problem, exit 1 → reverted → clean. Also removed
scikit-learn(one of the 9 corrected entries) → same shape, confirmingthe fix (not just the original seed) is load-bearing.
ffmpegapt-install line → 2 problems(both call sites), exit 1 → reverted → clean.
reviewer's exact finding) → 1 problem naming both unfiltered paths, exit 1
→ reverted → clean.
Anti-empty-result, both directions, against a synthetic tree with the
requirements-ci file / ansible task file absent entirely — both report a
non-empty problem rather than a clean scan of nothing.
repo_tests/requirements_ci_drift_test.py(17 tests),repo_tests/ci_system_package_provisioning_test.py(14 tests),repo_tests/code_quality_guard_reach_test.py(12 tests),autobot-backend/media/audio/ffmpeg_service_test.py(9 tests, includingtest_real_audio_extractionnow passing for real underCI=true) — 49passed, 1 skipped (the CI-only toolchain assertion, correctly skipping
outside CI), 0 failed. Full
repo_tests/tree still collects cleanly (920tests, no collection errors).
Not run:
ansible, any deployment script,pip install, or a venv — everycheck above is read-only text parsing or a self-contained pytest run
executed directly against the checked-out tree.
Model Used
Claude Sonnet 5