Repository navigation
chore(vehicle): land 4 small branches (#16249, #16394, CodeQL fixes, dependabot uv) - #17116
Merged
Merged
Conversation
python-shard shared one venv between autobot-backend and autobot-slm-backend's pytest invocations, built from requirements-ci.txt -- autobot-backend/requirements.txt's langgraph-sdk caps websockets<16, while autobot-slm-backend/requirements.txt declares websockets>=17.1,<18 (SLM's real production version). SLM's tests always ran against 15.0.1 as a result, and #16391 carried a named exemption in check_dependency_floors.py's KNOWN_CROSS_VENV_EXEMPTIONS pending a real fix. setup-python-suite/action.yml now builds a second, parallel venv from autobot-slm-backend/requirements.txt + requirements-ci-test.txt, exposed via a new `slm-venv` output (not activated onto $GITHUB_PATH, reached explicitly so it coexists with the backend venv). ci.yml points the slm-backend pytest invocation at that venv's own interpreter, and splits the strict dependency-floor check into two --roots-scoped calls (one per venv, mirroring scripts/setup-ci-parity-env.sh's existing pattern) so each service's floors are judged against what its own venv actually installs. The KNOWN_CROSS_VENV_EXEMPTIONS entry is removed and pinned empty by a test.
…16249) `pipeline-scripts/check-pre-commit-hook-pr.sh` is run by two python-suite tests, and no pattern in `.github/filters/python-paths.yml` matched it. A change confined to the wrapper computed `python != 'true'`, the required-context shim reported `python-suite` green, and the tests that exercise the change never ran — a guard bypassable by touching the one file it exists to watch. Six paths, not the one reported: four wrappers with sibling tests (check-pre-commit-hook-pr.sh, check_baseline_no_growth.sh, detect-hardcoded-values.sh, pr-queue-open-list.sh) and the two libraries they source (scripts/lib/git-scope.sh, scripts/lib/hardcoded-value-rules.sh). Both extra findings came from the new guard failing against my own enumeration, before review rather than after: * I paired scripts to tests by NAME, which crosses a hyphen/underscore boundary silently — pr-queue-open-list.sh is tested by pr_queue_open_list_test.py. The guard pairs them by CONTENT and named it. * my source-line pattern matched the quoted argument as `[^"]+`, which stops at the first INNER quote. The portable spelling nests them — `source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/../x.sh"` — so it yielded `$(cd `, ended in no `.sh`, and was dropped as if the line sourced nothing. Only the simple `"${REPO_ROOT}/..."` spelling parsed, which is how hardcoded-value-rules.sh — sourced by two of the four — surfaced at all, and how git-scope.sh then read as absent. The extractor now matches the `.sh` argument itself, last one on the line. Both variables are assigned `$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)`, checked in each script rather than inferred from the name. The transitive closure was checked too, per the issue's "not verified" note: neither library sources anything, though both look like they might — git-scope.sh contains the word `source` four times and hardcoded-value-rules.sh six, and every occurrence is prose in a comment. An anchored grep returned zero for the first; counting the unanchored word is what made that zero trustworthy. Why the existing coverage guard cannot catch this, which the issue did not record. `python_filter_covers_its_guards_test.py` exists for exactly this class and two independent things stop it: it sweeps `repo_tests/` only, and `pipeline-scripts`, `scripts` and `tools` sit in its `_NOT_A_READ` set — whose stated premise, "trees whose contents no guard reads directly", is false, since check-pre-commit-hook-pr_test.py reads its own `.sh`. Sweeping those directories would still discard the finding. Widening it was measured, not guessed. The sweep was reimplemented independently and validated against the recorded baseline (38 uncovered reads, equal to MAX_UNCOVERED_READS and to the 38 recorded entries) before any option was judged: today 38 also sweep pipeline-scripts/ 58 ...and drop pipeline-scripts from _NOT_A_READ 63 ...and drop scripts too 77 Every option raises a count whose own rule is that it may only fall, and the 25 newly surfaced reads are mostly workflow and config files — a different population from this defect. So the narrow invariant is asserted directly in `repo_tests/python_filter_covers_tested_shell_wrappers_test.py`: a wrapper that python-suite tests must itself be able to trigger python-suite. Exact, and it costs the ratchet nothing — re-measured after the filter change, still 38, so the equality rule holds in both directions. That guard imports the matcher from the existing one rather than restating it. Two matchers drifting apart would disagree silently and certify coverage the real gate does not grant; a rename breaks the import loudly instead. Four of its six tests are controls: the sweep must find at least four tested wrappers and name the reported one; the extractor is pinned to exact sourced sets for all four real wrappers, including the empty one, which is what would catch an extractor that started matching prose; an argument that does not resolve to a repository file is REPORTED rather than skipped, so an unreadable source line cannot look like a script that sources nothing; and the imported matcher is asserted to say no to an absent path, without which both findings tests pass vacuously. Closes #16249
`python-suite shard 8/12` failed on `glob_declared_reads_15900_test.py::test_the_record_names_exactly_the_guards_that_declare_each_glob`: declaring-but-not-recorded: ['repo_tests/python_filter_covers_tested_shell_wrappers_test.py'] The guard added by this PR sweeps `pipeline-scripts/*.sh`, and #15900's record names every guard that declares each tracked glob so a sweep cannot quietly appear or vanish. A new declarer has to be added in the same change — which is the record doing its job, not an obstacle. Only `*.sh` needed recording. The guard also calls `glob("*_test.py")`, and that pattern is not tracked: the record carries `*_test.sh` but no `*_test.py`. Checked the two other guards this session added for the same trap — `command_position_scan_test.py` and `hook_declarations_are_reachable_test.py` — and neither globs at all; they read a module's `dir()` and the router's exported routes respectively. So this is the only entry needed.
…al (#16394) Giving autobot-slm-backend's tests their own venv (this same PR) surfaced 3 ModuleNotFoundErrors across every shard's SLM test run -- numpy, jinja2, and aiosqlite were never declared in autobot-slm-backend/requirements.txt, only ever present because CI's OLD shared venv installed them transitively for the backend's own needs (aiosqlite>=0.22.1 is autobot-backend/requirements.txt's own pin; numpy/jinja2 arrived from some other backend dependency's own closure, never declared directly there either). tests/test_bi_dashboard.py needs real numpy (documented in its own comment, no stub) and falls back to a jinja2 stub only when the real package is genuinely absent; migrations/migrate_system_secrets_to_vault_test.py and user_management/services/sso_e2e_test.py need the real `sqlite+aiosqlite://` SQLAlchemy driver for in-memory async DB tests. numpy pinned via the existing `-c ../constraints/shared.txt` convention (autobot-backend/requirements.txt:9's same mechanism), not a local version -- this dev box's Python 3.10.12 cannot resolve constraints/shared.txt's numpy>=2.5.3 floor (needs 3.11+), so that one is unverifiable locally; CI runs the declared 3.14.7. jinja2 and aiosqlite verified installable in an isolated venv on this box (jinja2 3.1.6, aiosqlite 0.22.1).
Bumps the uv group with 2 updates in the /autobot-infrastructure/shared/mcp/tools/knowledge-base-mcp directory: [anyio](https://github.com/agronholm/anyio) and [httpx2](https://github.com/pydantic/httpx2). Updates `anyio` from 4.12.1 to 4.14.2 - [Release notes](https://github.com/agronholm/anyio/releases) - [Commits](agronholm/anyio@4.12.1...4.14.2) Updates `httpx2` from 2.10.0 to 2.12.0 - [Release notes](https://github.com/pydantic/httpx2/releases) - [Changelog](https://github.com/pydantic/httpx2/blob/main/src/httpx2/CHANGELOG.md) - [Commits](pydantic/httpx2@v2.10.0...v2.12.0) --- updated-dependencies: - dependency-name: anyio dependency-version: 4.14.2 dependency-type: indirect dependency-group: uv - dependency-name: httpx2 dependency-version: 2.12.0 dependency-type: indirect dependency-group: uv ... Signed-off-by: dependabot[bot] <support@github.com>
…_test.py (#16249) pyflakes F821: Path is used in 3 type annotations (dict[Path, list[str]], def _sourced_paths(script: Path)) but never imported. from __future__ import annotations defers evaluation at runtime, but pyflakes still resolves annotation strings statically. Pure import addition, no logic change.
…le regexes CodeQL flagged 3 high-severity alerts introduced by the #17077 merge: - py/clear-text-logging-sensitive-data (npu_client.py:471): the error log on embedding-generation failure logged a raw prefix of caller-supplied text. generate_embedding_with_fallback() is a generic low-level client called from several places (work_intent_similarity, semantic_search, vector_search_engine, code_intelligence) that don't all redact before embedding, so a failed call could log real secret material. Now redacts via autobot_shared.secret_redaction.redact_content() before truncating. - py/polynomial-redos x2 (secret_redaction.py:325, :347): the PEM-block and data-URI matchers used unbounded quantifiers ([\s\S]*? and [^,\s]+) whose lazy/greedy scan restarts at every occurrence of the anchor literal in the input, giving O(n^2) worst case on adversarial content (many "-----BEGIN ... PRIVATE KEY-----" or "data:" occurrences with no closing marker) -- ironic since this module itself scans arbitrary KB/fact content. Bounded both quantifiers to generous but finite limits that comfortably cover real PEM key bodies (up to ~8192-bit RSA) and real MIME types, verified linear (not quadratic) scaling and full behavioral equivalence against every true-positive/false-positive fixture in secret_redaction_test.py.
…og path (#17104 review) redact_content(text)[:80] ran every redaction regex over the full caller-supplied text (up to ~100 KB) on an unconditional logger.error in the embedding-failure path. Slice to 512 chars before redacting -- well over the final 80-char prefix, so a credential crossing that boundary is still masked, but the regex cost is now bounded.
mrveiss
added
the
land-next
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change updates one secret baseline explanation. It replaces an obsolete line-number reference with a description of the comment above ChangesSecret baseline update
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This documentation-only update does not alter runtime behavior and is ready to merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation For Resolution Provide reviewable diff or test evidence for Full details: Out of Scope Changes checkExplanation The summary identifies CodeQL fixes, Dependabot dependency upgrades, and a secret-redaction baseline-reason change in addition to the work for
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 19, 2026
This was referenced Sep 19, 2026
Owner
Author
|
#17116 update: member #16889 is BLOCKED on a CRITICAL regression in the builtin updater. #17104's fix is approved at #16889 adds
The other ACs of #16394 are met: the SLM venv is really used (the shard log shows websockets 17.1 under the SLM interpreter), both Non-blocking: the dependency-floor banner now also fires on the SLM shard. It never gates, so a follow-up is optional. |
…uirements.sh (#16889 review) #16394 gave autobot-slm-backend/requirements.txt its own sibling-relative -c ../constraints/shared.txt include, which the deployed directory can't resolve. A bare pip install -r aborted the sync, and the && silently skipped alembic upgrade head (#11069/#14272) -- the same failure mode already fixed for the backend and ai-stack roles. Routes through the same canonical script so all three deploy paths share one implementation.
…ame (#16889 review) The previous fix copied the backend role's post_sync_cmd pattern verbatim, including its /tmp/requirements-filtered-slm.txt output path -- already used by the backend role itself. code_sync runs sync jobs as concurrent tasks, so on a node hosting both roles the two jobs can interleave and one filter's output gets pip-installed into the other role's venv: a silent wrong-deps install, not a visible failure. Adds a regression test asserting every role's filtered-output /tmp path is unique, verified to catch the exact collision by reverting locally and confirming it fails, then restoring.
This was referenced Sep 19, 2026
…ason - role_registry.py: 727 -> 715 lines. Collapses the slm-backend #16889 comment from 9 lines to 3 and merges it with the adjacent pre-existing #14275 comment, and tightens the post_sync_cmd string layout -- no behavior change, back under the grandfathered 715-line ceiling (#14236). - secrets_baseline_reasons.py: the Basic Auth Credentials entry for autobot_shared/secret_redaction.py cited a bare "line 242", which drifts whenever a regex rewrite upstream (like this vehicle's own #17104 member) moves the comment -- reworded to cite the comment above `_BASIC_AUTH_URL_RE` by name instead. - glob_declared_reads_15900_test.py: the main merge below brought in #15317's chromadb_bind_not_hardcoded_15317_test.py, whose *.service, *.service.j2 and docker/*.yml glob declarations were undetected by that guard's own coverage sweep -- recorded all three (plus adding the guard to the pre-existing *.sh entry's set) so the pre-push hook's glob-declared-reads check passes. Also merges origin/main (143 commits) to bring this code-touching PR to exactly 0-behind, per the owner's merge-safety rule.
mrveiss
added a commit
that referenced
this pull request
Sep 19, 2026
…globs (#17124) chromadb_bind_not_hardcoded_15317_test.py (landed via #17047) declares *.service, *.service.j2 and docker/*.yml root-relative globs that GLOB_DECLARED_UNCOVERED never recorded, flipping glob_declared_reads_15900_test.py red on main -- a second, independent combination red alongside #17124's own KB-visibility guard, folded into this base-fix PR per the current queue rather than opened separately. Same fix c0 wrote for #17116's vehicle (verified via `git diff 2acfb8b^1 2acfb8b -- repo_tests/glob_declared_reads_15900_test.py` against a fresh check that the test fails on current main without it); applied here as an identical diff, confirmed via `git diff` against that reference before committing.
9 tasks done
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
This was referenced Sep 19, 2026
mrveiss
added a commit
that referenced
this pull request
Sep 19, 2026
…globs (#17124) chromadb_bind_not_hardcoded_15317_test.py (landed via #17047) declares *.service, *.service.j2 and docker/*.yml root-relative globs that GLOB_DECLARED_UNCOVERED never recorded, flipping glob_declared_reads_15900_test.py red on main -- a second, independent combination red alongside #17124's own KB-visibility guard, folded into this base-fix PR per the current queue rather than opened separately. Same fix c0 wrote for #17116's vehicle (verified via `git diff 2acfb8b^1 2acfb8b -- repo_tests/glob_declared_reads_15900_test.py` against a fresh check that the test fails on current main without it); applied here as an identical diff, confirmed via `git diff` against that reference before committing.
mrveiss
removed
the
land-next
#17116) 347 behind. Blob comparison against origin/main shows 13 of the 21 files this branch touched are already byte-identical on main -- #16249 and #16394 landed via #17149, and the CI action/workflow payload with them. Three conflicts, all resolved toward main so no later fix is reversed: - services/role_registry.py: branch only reworded a comment and re-wrapped a string to hold the 715-line ceiling; main is already 715. Took main. - repo_tests/secrets_baseline_reasons.py: main extracted the reason table into secrets_baseline_reason_entries.py (#17134); the branch still carried it inline. Taking the branch here would have reverted that refactor. Took main and PORTED the branch's one real change into the new module -- a reason that cited 'line 242' for _BASIC_AUTH_URL_RE, which now sits at line 262. - repo_tests/python_filter_covers_tested_shell_wrappers_test.py: add/add, and the two sides differ only in black's line-wrapping. Took main. Net residue actually carried by this PR: the dependabot uv bump (anyio 4.12.1->4.14.2, httpcore2/httpx2 2.10.0->2.12.0), pypdf 6.18.1->6.19.0 in two manifests, and the ported reason reword. All upgrades; no downgrade.
Owner
Author
Branch verdict — salvaged, now mergeableOwner ruling was rebase and salvage, not close. Done: merged current What was already landedBlob comparison of all 21 touched files against Conflicts, all resolved toward main
The ported change is a reason string that cited Net residue this PR still carriesFour files, +12/−12:
All upgrades; no downgrade in the set. Note CI re-running on the new head. |
6 tasks
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
1 participant
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.
Thinking Path
Owner direction 2026-09-19: every branch with 5 or fewer commits goes into a vehicle, one per owner and scope, at most 5 members each. This carries my 4 remaining small branches, all ≤5 commits, no shared files between them.
What Changed
Server-side merge (
POST /merges) of 4 heads ontomain, one member per commit, no conflicts.fix-codeql-clear-text-log-and-redosd2d277b92ed2d277b92e(ledger-verified, matches carried head exactly)issue-16394-slm-own-venvdb0d234e3a(updated: was blocked CRITICAL at0fd349ea77—post_sync_cmdbypassed-c ../constraints/shared.txt, so every SLM self-update aborted at pip and silently skippedalembic upgrade head; fixed by routing throughbuild-filtered-requirements.sh, folded in as a 6th vehicle commit)issue-16249-filter-gapf21f3a796ef21f3a796e(ledger-verified, matches carried head exactly)dependabot/uv/uv-6c3fb3b3e80f732a9c1e0f732a9c1e(ledger-verified, matches carried head exactly)Verification
Verdicts above are cited from the ledger's actual recorded review at the exact carried head, not assumed from "the PR was reviewed at some point" — an earlier version of this table claimed "approved" for all 4 without checking this, which was wrong for #16889 (approval was at a different SHA) and unverified for #17104 (the CodeQL fix). This PR's own delta beyond the 4 original server-side merges is one more commit fixing #17104's outstanding CodeQL finding.
Single-issue rationale
N/A — this is the batch vehicle itself; each member's own issue is listed above. #17104 and #17066 close no issue (a security fix responding to a CI finding, and a dependabot bump respectively).
Closes #16394
Closes #16249
Model Used
Claude Sonnet 5
Summary by CodeRabbit