Repository navigation
chore(vehicle): rest consolidation train — #17107, #17119, #17122, #17117, #17129 in one CI run (#17128) - #17133
Merged
Merged
Conversation
…ecretsManager (#16429) SecretAuditLog.vue, SecretVault.vue and ShareSecretDialog.vue had zero callers outside their own stories files. Triaged each individually rather than deleting on sight: - SecretAuditLog: genuine gap, not a duplicate. useSecretsAuditApi reads a real GET /api/audit/logs (admin-only, api/audit.py), which api/secrets.py already writes to on every secret op. Wired into the Secrets page as a new admin-only tab (views/secrets/AuditLogView.vue, secrets-audit-log route), matching the LLM API Keys tab's existing admin gate. - SecretVault: same backend as SecretsManager.vue (secretsApiClient, same /api/secrets/ routes) -- a superseded parallel implementation. Its one net-new capability, virtual scrolling for 100+ items (#4037), is ported into SecretsManager.vue's list view (grid view keeps plain rendering -- CSS grid's auto-fill column count depends on runtime container width, which useVirtualList's fixed-row model doesn't represent, and SecretVault itself never had a grid mode to port a solution from). Retired in this PR now that parity is shown. - ShareSecretDialog: depends on useSessionCollaboration, which has 5 sibling components in components/collaboration/ that are themselves unwired -- a full real-time collaborative-session UI (#608 phases 5-7, closed / #874 phase 6) built end-to-end and never connected to any view. Filed as its own implementation gap, #16443 (sub-issue of #16425, blocked_by edge from #16429). Left unticked here. Also fixed while mapping SecretVault's scope vocabulary onto the backend's canonical values: the create/edit form's Scope select offered user/session/ shared options that api/schemas_system.py's SecretCreateRequest.scope (typed ChatSecretScope) would reject with a 422 -- only general/chat were ever valid. Filed #16450 for a larger, separate finding from the same investigation: the Visibility/Organization/Team/Shared-With controls in the same form have no server-side effect at all (SecretCreateRequest doesn't carry them onto the persisted model).
… changed test_*.py runs (#16711)
…est never re-imports the real package over it (#16722)
…tes, so Package.setup doesn't read an invented pytest_plugins (#16722) CI at 9251f0d failed shards 5 and 7 with 59 setup errors across the services/ tests, all "pytest.UsageError: Plugins may be specified as a sequence ... Got: <MagicMock name='mock.pytest_plugins'>". The cause: now that the stub is kept, Package.setup imports services/__init__.py through importtestmodule, and importlib mode returns the stub already in sys.modules. pytest then reads pytest_plugins (via consider_module) and setUpModule, setup_module, tearDownModule and teardown_module off it, and MagicMock invents all five. The stub now carries what a plain module without them gives: [] and None. The regression test drives importtestmodule itself.
…k with a __path__ (#16722) Code review of 52a3638 (HIGH): a MagicMock given a __path__ still invents every attribute. So `from services import manifest_loader`, `jwks_verifier`, `frontend_bundle_health` and the rest bound invented mocks instead of importing the real submodule. And tests/services/conftest.py's _ensure_real_pkg guard now short-circuited on that __path__, so the hollow-package swap it used to do never happened. The package-level UsageError in the last red run hid those tests. The root conftest now installs a hollow types.ModuleType("services") over the real directory, the shape tests/services/conftest.py already uses, and binds each child stub onto it. It invents nothing, so pytest's Package.setup attribute reads and `from services import` both behave as they would on a real package, and the explicit pytest_plugins and xunit-hook attributes are no longer needed. The regression test checks the shape instead of MagicMock identity: tests/services modules install their own hollow package at import time. It also checks that `from services import purge_playbook`, an unstubbed submodule, loads the real file.
…gisters onto the parent, so the two never diverge (#16722)
…hem (#16784) `extract_document` dispatched pdf and docx and fell through to `extract_plain_text` for everything else, so a spreadsheet, presentation or OpenDocument file was decoded as if its ZIP bytes were text. The result was binary noise presented as document text, flowing onward into the knowledge base and into prompts through retrieval. Pre-existing rather than a regression: before #16773 these formats were detected as "text" and took the same branch, which is why nothing looked wrong. `utils/document_parser.py` already owns working parsers for all six formats, so this delegates rather than forking a second implementation. The import is function-local on purpose: that module imports FROM this one, so a module-level import would be a cycle. Failure raises rather than returning empty text, so the pipeline's existing handling names the format instead of reporting success with nothing in it. A missing parser library is checked with find_spec BEFORE delegating, because the shared sync parser catches every exception per candidate and reports one "extraction failed" string — without the check, a deployment gap would be reported as a bad upload, the exact conflation the pipeline's two error paths exist to prevent. The tests assert a NEGATIVE property: for these formats extract_document must never return format="text". A suite covering only healthy documents would still pass if the fall-through came back, so every case uses a file that cannot be parsed and asserts it raises rather than degrades. extraction.py is left at 590 of its 600-line limit; six lines came back by reflowing over-wrapped comments so the file is not parked at the edge.
…t kinds (#16947) Discovery today is four unconnected registries (#6828), none of them a live named "who is here and are they busy" view: AgentHealthRegistry tracks AI-stack health not activity, DistributedAgentManager tracks distributed task assignment, agent-terminal's SessionManager holds live sessions with no shared identity shape, and AgentOrgNode's heartbeat state is a poll, not live. Sessions had no registry at all. - protocols/agent_kind.py: AgentKind (COMPANY_OS, AI_STACK, SESSION, EXTERNAL), split from agent_communication.py to avoid growing a file already at its size-ratchet ceiling, and to avoid a circular import with the new presence module. - protocols/agent_communication.py: AgentIdentity gains kind/name/tenant_id (the shared identity model from #16946) -- additive, defaults preserve every existing caller's behavior exactly. Net zero growth against the 805-line ratchet ceiling: three restating-the-obvious comments removed (two in the CLI demo block, one duplicate `# noqa: E402`) to offset the new fields. - protocols/agent_presence.py: AgentPresenceRegistry, in-memory and TTL-bounded (AUTOBOT_AGENT_PRESENCE_TTL_SECONDS, default 90s) -- (kind, tenant_id, name) -> busy/idle. report() rejects a different instance_id claiming a live name (squat protection, #16946 §2) but lets the same instance heartbeat freely, and lets a new instance reclaim a name once the old one goes stale. EXTERNAL is refused outright -- owner decision 3: external A2A peers get identity for attribution only, never discovery. - protocols/agent_presence_feeds.py: pull adapters for each kind's own authoritative source (never a caller-declared blob, per #16946 §2) -- Company OS from AgentOrgNode + latest heartbeat run status (mirrors llc/api/agents.py's own join), AI-stack from AgentHealthRegistry (busy honestly reported as unknown/idle -- DistributedAgentManager's active_tasks would refine this, left as a follow-up rather than guessed), sessions from agent-terminal's SessionManager via has_running_task(). - 19 new tests: collision rejection, same-instance heartbeat, stale-name reclaim, TTL expiry (entries actually pruned from the live table, not just filtered at read time), EXTERNAL refusal, deregister ownership, and each feed adapter against a fake of its own source. Lands the identity type and the registry interface first, as instructed -- #16948 and #16949 (both blocked_by this) can now build against a real interface. The AI-stack busy signal and any push-based (non-pull) feed wiring are explicitly left as follow-ups, not silently dropped. Refs #16947
#16947) Review found tenant_id=None doing two jobs: shared infrastructure (correct for AI-stack) and unknown (wrong for a Company OS node with no company_id, and for every session unconditionally) -- an agent whose tenant couldn't be determined was becoming visible to every tenant's query. - New UNKNOWN_TENANT sentinel, distinct from None. Never returned by list_live() under any argument, including the sentinel itself passed directly -- fails closed rather than becoming a backdoor to unknowns. - sync_company_os_presence: a null company_id is UNKNOWN_TENANT, not shared. - sync_session_presence: UNKNOWN_TENANT unconditionally. Investigated the conversation_id -> owner -> org path asked for and it does not exist in code -- chat_history's session/conversation mixins (base.py, session.py, security.py) carry no user_id/org_id anywhere, and the owner username threaded through SessionManager.create_session is never persisted onto AgentTerminalSession for a later lookup to recover. Reporting this back rather than picking something else, per the review's own instruction. The real fix is threading tenant through at session-creation time, not deriving it after the fact from a field that cannot carry it. - list_live(tenant_id=None) -- returns {tenant_id} union {shared}, never unknown. No unscoped "every tenant" query on purpose, per the review: a caller that needed one would be reimplementing the leak this replaces. - Logged both rejection paths (collision, EXTERNAL) and the TTL env fallback -- were silent before. - Module docstring now states report()/deregister() enforce no caller identity of their own; the authoritative-source guarantee holds only because the three feed adapters are the sole intended callers. - 6 new tests: the negative control (tenant X doesn't see tenant Y), shared entries included in a tenant query, no-arg defaults to shared-only, an UNKNOWN_TENANT entry never surfaces under any query, a null-company_id node is unknown not shared, sessions are UNKNOWN_TENANT and never listed. Refs #16947
A terminal session had no tenant, and none could be recovered afterwards -- presence (#16947) had to report every session as UNKNOWN_TENANT. The route that looked like it might work does not exist: conversation_id is a log- linkage id only, chat_history's session/conversation stores carry no user/tenant field, and the owner username threaded through SessionManager.create_session was never persisted for a later lookup to recover. - AgentTerminalSession gains an explicit tenant_id field. - Threaded through SessionManager.create_session -> AgentTerminalService.create_session -> the one real call site with authenticated context: POST /api/agent-terminal/sessions, which already resolves `owner` from current_user.get("username") for the WebSocket ownership gate (#14989/#14960) -- tenant_id is current_user.get("org_id") alongside it, the JWT claim only. TerminalCreateSessionRequest has no org_id/tenant_id field, so a caller cannot supply one through this route. - sync_session_presence now reads session.tenant_id, falling back to UNKNOWN_TENANT only when it's unset -- a pre-#16975 session, or one created via a path with no authenticated context (e.g. the chat-tool internal session creation in tools/terminal_tool.py, which passes neither owner nor tenant_id today and is left alone: no code touches those paths in this PR, so they correctly keep reporting UNKNOWN_TENANT rather than a guess). - 6 new tests: SessionManager.create_session threads tenant_id onto the session (and leaves it unset without one); presence reports a session under its real tenant and it is invisible to a different tenant's query (the AC's own discoverability negative control); a session with no tenant stays UNKNOWN_TENANT and is never listed. - service.py's ratchet lowered 962 -> 959 (landed below ceiling after trimming its own docstring to make room for the new parameter). Refs #16975
Review found to_persist_dict() left tenant_id out, and get_session()'s
Redis-reload branch rebuilt AgentTerminalSession without it -- a session
stamped with its tenant at creation lost the stamp on any reload (a
restart, an eviction, another worker) and reappeared as UNKNOWN_TENANT.
Fails closed, so nothing leaked, but it broke AC1 across a reload.
- to_persist_dict() now includes tenant_id.
- get_session()'s Redis-reload branch restores it via
session_data.get("tenant_id").
- The pending-approval rebuild path is left as UNKNOWN_TENANT deliberately
-- it predates this PR and never guessing is correct there.
- New test drives the real persist -> drop from memory -> get_session
round trip (not the two methods in isolation), then confirms the
reloaded session is discoverable through the real presence registry in
its own tenant only.
Refs #16975, #16978 review
…16947) AC2 says busy/idle is live -- both stubs were not. has_running_task() always False (running_command_task was declared but never assigned anywhere outside models.py), and sync_ai_stack_presence hard-coded busy=False and called it a follow-up. - services/agent_terminal/service.py: new _run_tracked() helper wraps both PTY-execution call sites (_execute_auto_approved_command, _run_approved_command_body) so running_command_task is actually set for the command's real span and cleared in a finally, success or exception. - chat_workflow/manager.py: ChatWorkflowManager gains _active_streams (incremented/decremented around process_message_stream's real try/ finally, not a copy of it) and is_processing(). No persistent per-role AI-stack process exists, so this is "any stream in flight right now" for the whole registry, not attributed to one role -- documented as the honest simplification, not a guess. - protocols/agent_presence_feeds.py: sync_ai_stack_presence takes an optional chat_workflow_manager and reports its is_processing() as busy for every role; omitting it reports idle, same as before. - AUTOBOT_AGENT_PRESENCE_TTL_SECONDS registered in autobot_shared/env_registry_agent_runtime.py -- the code-quality red: it was documented by hand in ENV_VARS.md but never registered, so check_env_var_registry.py flagged it as read-but-unregistered. docs/developer/ENV_VARS.md regenerated from the registry (byte-identical to the hand-written version once regenerated). - 7 new tests: is_processing() true only for a stream's real span, across overlapping streams, and clearing on exception; has_running_task() true only for a command's real span and clearing on exception. - File-size ratchet: manager.py and service.py both landed below their ceiling (redundant-comment trims offsetting the new lines) and are lowered to match, not left with slack. Refs #16947
#16947) duplication-guard: sync_company_os_presence's docstring said it "mirrors llc/api/agents.py::list_agents's own latest-run-per-agent join" -- it did, literally, 15 lines of the exact same SQLAlchemy query pasted into both files. jscpd correctly caught it (9 lines over the whole-tree pin; main alone measures 11718, 5 under it, so this diff's own +14 was the entire overage). agent_org_nodes_with_latest_heartbeat(company_id) in llc/api/agents.py is now the one definition; list_agents and sync_company_os_presence both call it. Behavior-preserving -- same subquery, same outer joins, same where clause; list_agents adds its own .order_by(AgentOrgNode.name) after. Verified with the same jscpd invocation CI runs (autobot-backend + autobot-frontend/src, -k 70 -l 8 --skip-comments): 11718 duplicated lines, exactly main's own clean baseline, no longer 11732. Refs #16947
… issue-16947-agent-presence
… issue-16975-session-tenant
mrveiss
removed
the
land-next
This was referenced Sep 19, 2026
Open
Merged
mrveiss
added a commit
that referenced
this pull request
Sep 24, 2026
…y are real reads (#17329) python_filter_covers_its_guards_test.py went red at 46 against MAX_UNCOVERED_READS = 42: the two guards this branch adds name four paths the python filter does not cover. They are not the same kind of thing, and the file's own trade-off rule says to treat them differently. .github/filters/frontend-paths.yml is a REAL read -- frontend_path_filter_ mirror_test.py parses it and compares it to frontend-test.yml's push paths, so its verdict changes when that file changes. Covered in the filter, which is what makes the guard re-run when its input moves. The other three -- autobot-frontend/vitest.config.ts, autobot-slm-frontend/vitest.config.ts and autobot-slm-frontend/src/App.vue -- are PARAMETERS to hv_path_in_scan_dirs, a pure string predicate. Nothing opens them and no content of theirs can change the verdict. Recorded as bypasses and MAX raised 42 -> 45 with the reason, which is exactly the precedent this file already carries for CLAUDE.md (#17129), README.md (#17133) and SSHTerminal.vue (#17020): "covering the real path in the filter would run twelve shards on every edit for a guard that does not depend on its content. Recorded, not covered, per this file's own trade-off rule." Raising is allowed for a new bypass and forbidden for a denominator correction -- the three existing RAISED entries say so in those words. This is the former: the count went up because this branch introduced three new literals, not because anything was excluded to silence a failure. Recursive, and worth noting: the branch widens the hardcoded-value scope to the frontend config roots, adds a guard asserting that new scope, and that guard names frontend config paths -- which a different ratchet counts. The widening is what makes the guard name them. 13 passed on the ratchet, 35 across it and the three guards it judges.
Merged
7 tasks done
mrveiss
added a commit
that referenced
this pull request
Sep 24, 2026
…ory scope (#17329) (#17330) * fix(hooks): give the hardcoded-values hook and scan one shared directory scope (#17329) The two entry points share a rule set and disagreed about which files it applies to. detect-hardcoded-values.sh walked a list it owned privately; the pre-commit hook scanned whatever was staged and never saw that list. So any staged file outside those trees was blocked on commit, invisible to the scan, and impossible to baseline -- the baseline is keyed to what the scan finds and --audit-baseline fails on an entry the scan cannot reproduce. The only way to commit such a file was --no-verify, which switches off every other hook to get past one that was never going to pass. The list moves to scripts/lib/hardcoded-value-rules.sh as HV_SCAN_DIRS, beside the rules both callers already share, with hv_path_in_scan_dirs() as the predicate. The scan consumes it instead of its own literal, and the hook applies it alongside hv_file_in_scope. Same six directories, byte-identical -- verified by diffing the new array against the old literal from origin/main -- so nothing the scan covered yesterday is uncovered today. What this does NOT do is add coverage: each frontend's project-root configs sit outside */src and remain unscanned by both sides. The comments at both the predicate and the hook's filter say so and name the issue, because a gate that is silent about what it does not look at is the failure this repo keeps closing. Option 2 -- widening the list -- is measured on #17329 rather than guessed at: 24 findings across 8 files, mostly dev/test ports (:8001, :5173, :3000) in e2e and playwright configs plus documentation URLs in comments. Verification: - the file that could not be committed now passes. Staging #17324's vitest.config.ts change and running the hook exactly as pre-commit does: "No in-scope files staged for commit", exit 0. - repo_tests/hardcoded_values_scope_agreement_test.py, 14 cases, pins both halves: the list is defined once, each caller reads it, and the predicate agrees with the declared directories. - negative controls, both run: delete the hook's scope call -> 1 failed; re-fork a literal list into the scan -> 1 failed; restored -> 14 passed. - the guard's own first version was WRONG and the control caught it. It asserted `"hv_path_in_scan_dirs" in body`, which still matched the comment describing the call after the call itself was deleted -- green on the exact regression it exists to catch. It now strips comment lines before any containment check, and that reasoning is recorded in the helper's docstring. * fix(ci): restore the two frontend path-filter mirrors the hook had made uneditable (#16260) #15002 needed `autobot-plugins/terminal/**` in the frontend path lists so a plugin-only edit still runs the redactor drift guard. The entry landed in .github/filters/frontend-paths.yml -- the authoritative PR gate -- and was DROPPED from both mirrors, because staging either workflow file tripped the hardcoded-values hook on three pre-existing, unbaselinable URLs. The commit in this branch removes that blocker, so the mirrors can be restored: - frontend-test.yml `on.push.paths`: post-merge runs. Without the entry a plugin-only push did not run the frontend suite after merge. - ci.yml's inline `frontend` filter: gates that workflow's own frontend-tests job (ci.yml:545), which was skipped for the same changes. The note in frontend-paths.yml saying the entry was "not yet mirrored" is corrected rather than left behind -- a stale comment asserting a gap that no longer exists is its own small defect. repo_tests/frontend_path_filter_mirror_test.py pins the mirror that declares itself one: frontend-test.yml's push paths must equal the authoritative list exactly, in both directions. Drift here is silent by construction -- both files parse, both workflows run, and the only symptom is a suite that quietly does not execute -- which is why it survived months. Negative control run: drop the restored entry and it fails with "A change matching these patterns would NOT run the suite after merge: ['autobot-plugins/terminal/**']". Verified end-to-end rather than by reading: staging ci.yml, frontend-test.yml and the filter file and running the hook exactly as pre-commit does now reports "No in-scope files staged for commit". Both workflow files and the embedded filter block parse as YAML, and the parsed filter now carries the entry. Not touched: ci.yml's inline list holds 3 of the authoritative 8 patterns. That is a narrower gate for a different job, so whether it should mirror the full list is a coverage decision rather than drift, and it is filed separately with the exact missing set instead of being changed here. * fix(repo-tests): make the new guard's nosec annotations reach bandit (#17329, #13521) I wrote `# nosec B404 - fixed argv, no shell`. Bandit reads every word after the directive as a test id, so `-`, `fixed`, `argv` and `no` became ids it does not know and the suppression silenced nothing -- an annotation that looks like a considered decision and has no effect, which is the exact shape #13521's guard exists to catch. It caught mine. Correct form, `# nosec <IDS> # <prose>`, on both sites. Verified twice rather than by eye: scripts/check_nosec_format.py now reports nothing across the repository, and bandit on the file reports "Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 3" -- B404, B603 and B607 are actually attributed and suppressed. The 8 remaining B101 assert_used findings are ordinary for a test module. * fix(tests): point the changed-lines fixtures at a scanned directory (#17329) My own change broke these. TestChangedLinesOnly plants its fixture at the temp repo ROOT, and this branch makes the hardcoded-values hook judge staged paths against HV_SCAN_DIRS, so the fixture fell out of scope and the hook looked at nothing: a pre-existing violation read as "hidden rather than reported" and a newly added one "passed" -- the same symptom as the guard being switched off. Verified as mine rather than assumed, same command both sides: 11 passed on base, 3 failed on this branch. The fixture path becomes autobot-backend/m.py, which is inside the scanned set and says so by being that path. Deliberately line-neutral: this file sits at its recorded size ceiling of 747, and a grandfathered file may not grow (#14236) -- so the explanation belongs in this message and the PR body rather than in a comment block that would raise it. The scope reduction itself is asserted in repo_tests/hardcoded_values_scope_ agreement_test.py, which pins hv_path_in_scan_dirs against paths inside and outside the scanned set, including project-root configs. An end-to-end version at this hook's level needs the harness that lives here, so it waits for this file to be split rather than growing it. * feat(hooks): widen the hardcoded-value scope to the config roots and scripts/ (#17329) Option 2, measured rather than guessed, and triaged rather than seeded. The previous commits made the hook and the scan agree on scope; this one buys back the coverage that agreement gave up, for the three directories where the gap was costing something. HV_SCAN_DIRS gains each frontend's project ROOT in place of its */src, plus scripts/. Those trees held files that could be neither committed through the hook nor absorbed by the baseline -- the catch-22 this issue exists for, with three known victims: autobot-frontend/vitest.config.ts (#17324), .github/workflows (#16260, already unblocked here) and scripts/, which blocked the correct fix for #16816 and forced a worse one. The walk prunes node_modules and friends. _HV_EXCLUDE_RE already dropped those paths from the findings, but with the project roots in scope the grep would read every matching file inside a ~790MB tree on the way to discarding it. Widening surfaced 34 findings. They are not 34 baseline rows: 3 stopped being findings. _HV_COMMENT_RE lists `#`, `//` and a continuation `*` and had simply missed the block OPENER `/*`, so one-line doc comments were scanned as code. Measured: zero stranded baseline entries. 2 are FIXED here. emergency-rollback.sh's REMOTE_ENV_FILE and sync-to-admin.sh's REMOTE_PATH now name their remote root through an env var with today's value as the default. Deliberately NOT AUTOBOT_BASE_DIR: both scripts already resolve a LOCAL root, and pointing a remote path at a locally-overridden one conflates two roots that must differ. 29 are baselined, each with its own `# reviewed: #17329 <why>` line. One of those reasons matters more than the others. scripts/check_ansible_file_ references.py's "/opt/autobot/src/" LOOKS like the class this gate exists for and is not: :200 tests whether a resolved ansible VALUE starts with it and :204 strips it to recover a repo path, so it describes what this repo's ansible files say. Deriving it from AUTOBOT_BASE_DIR would make every real deployed path fail that startswith() wherever the var is set, and the checker would report clean while checking nothing. Baselined with that reasoning rather than "fixed" into a silent guard. A refinement was measured and REJECTED: adding `process.env` to _HV_CONFIG_RE would have dropped 34 findings to 21, and would have stranded 10 tracked entries including two internal IPs, because `process.env.X || 'http://<ip>'` reads config AND hardcodes a fallback. That is #14198's defect exactly, so the playwright fallbacks stay findings with reasons instead. Verified: detect-hardcoded-values.sh reports status=pass, 0 violations, and --audit-baseline reports no stale entries. 65 tests pass across the scope guard, the mirror guard and the pre-commit hook suite. * fix(tests): source HV_SCAN_DIRS instead of text-parsing the wrapper (#17329) _scan_dirs() read `SCAN_DIRS=(` out of detect-hardcoded-values.sh and split it by hand. This branch changed that line to `SCAN_DIRS=("${HV_SCAN_DIRS[@]}")` when the list moved to the shared rule library, so the parse found exactly one "directory": the literal ${HV_SCAN_DIRS[@]}. Four tests failed with "SCAN_DIRS parse found ['${HV_SCAN_DIRS[@]}']". The function's own docstring said it read the list "from the script itself, so this file cannot drift from it" -- the intent was right and the location moved underneath it. That is the same shape as a test selected by a name that resembles the failing one: A TEXT PARSE READS WHERE A VALUE IS WRITTEN, and the value moved. Sourcing the library and reading the array asks the same question the script asks, so a future move cannot silently answer it wrong -- it fails to source instead of finding a plausible wrong answer. 4 passed. * fix(tests): repair the two sibling suites this branch's scope change broke (#17329) Review found the same defect class I had already fixed twice on this branch, in two files I had not touched: 35 failed, 71 passed before this commit. pre-commit-hardcoded-values_test.py (67 tests): its fixtures staged files at `src/...` and bare root names, none of which falls under any HV_SCAN_DIRS prefix, so the hook filtered every one out, printed "No in-scope files staged for commit" and exited 0 -- roughly forty tests asserting returncode != 0 got 0. Every fixture now stages inside a scanned tree, line-neutral: the path itself carries the requirement. The four workflow cases needed judgement rather than a prefix. This branch deliberately makes .github/ unscanned -- that is what unblocked #16260 -- so two of them asserted behaviour this PR removes and two would have passed VACUOUSLY, which is worse than failing. They now stage under autobot-infrastructure/.github/workflows/, which the exemption's own pattern (`*/.github/workflows/*`) still matches, so the vendor-URL rule stays under test inside a scanned tree. A new case pins the scope change itself: an IP that would be blocked inside a scanned tree is not judged at the repo root. detect-hardcoded-values_test.py (25 tests): its _SCAN_DIRS was the stale six-entry list and its drift guard TEXT-PARSED `SCAN_DIRS=(`, which now reads `SCAN_DIRS=("${HV_SCAN_DIRS[@]}")` -- so the parse returned that literal as its one directory, while _hermetic_repo never created `scripts/` and left the script FATALing on a directory it could not find. The list is sourced from the library now, and the drift guard asserts the script still DELEGATES rather than re-declaring, because "a second list exists at all" is the drift that matters. One assertion widened honestly: `scripts` is both a scan directory and where the rule library lives, so removing it trips the "cannot load" FATAL rather than the missing-directory check. Both are the same refusal, and the test now asserts the refusal rather than which guard reached it first. Written to fit, not past, the gate: that file is grandfathered at 653 lines and this leaves it at exactly 653, so the reasoning above lives here rather than in comments that would have raised a ceiling. 68 passed and 41 passed respectively. * fix(guards): split this branch's four new guard inputs by whether they are real reads (#17329) python_filter_covers_its_guards_test.py went red at 46 against MAX_UNCOVERED_READS = 42: the two guards this branch adds name four paths the python filter does not cover. They are not the same kind of thing, and the file's own trade-off rule says to treat them differently. .github/filters/frontend-paths.yml is a REAL read -- frontend_path_filter_ mirror_test.py parses it and compares it to frontend-test.yml's push paths, so its verdict changes when that file changes. Covered in the filter, which is what makes the guard re-run when its input moves. The other three -- autobot-frontend/vitest.config.ts, autobot-slm-frontend/vitest.config.ts and autobot-slm-frontend/src/App.vue -- are PARAMETERS to hv_path_in_scan_dirs, a pure string predicate. Nothing opens them and no content of theirs can change the verdict. Recorded as bypasses and MAX raised 42 -> 45 with the reason, which is exactly the precedent this file already carries for CLAUDE.md (#17129), README.md (#17133) and SSHTerminal.vue (#17020): "covering the real path in the filter would run twelve shards on every edit for a guard that does not depend on its content. Recorded, not covered, per this file's own trade-off rule." Raising is allowed for a new bypass and forbidden for a denominator correction -- the three existing RAISED entries say so in those words. This is the former: the count went up because this branch introduced three new literals, not because anything was excluded to silence a failure. Recursive, and worth noting: the branch widens the hardcoded-value scope to the frontend config roots, adds a guard asserting that new scope, and that guard names frontend config paths -- which a different ratchet counts. The widening is what makes the guard name them. 13 passed on the ratchet, 35 across it and the three guards it judges. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 of 4 tasks
mrveiss
added a commit
that referenced
this pull request
Oct 4, 2026
…the wrong population (#17930) Four defects found in review of the guard added earlier on this branch, all confirmed with a control before being fixed. 1. FALSE POSITIVE ON COMMENT BLOCKS. `undefined_tags("{% comment %}{% citation %} {% endcomment %}")` returned `['citation']`. Liquid's `comment` block overrides `unknown_tag` to do nothing, so Jekyll accepts that Markdown and the guard rejected it. Comment regions are now stripped like raw regions -- AFTER them, because a `{% comment %}` quoted inside a raw block would otherwise open a region that swallows every violation up to an unrelated `{% endcomment %}`. Fixtures cover both directions and the ordering. 2. HOLLOW ON BALANCE. `{% endif %}` alone, `{% else %}` alone, `{% endfor %}` alone, an unclosed `{% if %}` and an unclosed `{% raw %}` all returned `[]`, and every one is a Liquid::SyntaxError built entirely from WHITELISTED names -- checking names could not see any of them. `unbalanced_tags` adds a block/end stack over the same stripped text, with a fixture per case and seven well-formed contrasts so it cannot degenerate into rejecting all templating. The sibling fix riding this branch (#17932) is itself a stray `{% endif %}`. 3. THE POPULATION WAS MISLABELLED (#17844). `what="Jekyll-processed Markdown docs"` was false: Jekyll runs Liquid only on files WITH front matter, and 131 of the 739 scanned files have it. The other 608 are copied verbatim, can never break the build, and could only ever yield false positives -- and the floor was sized over them. Verified first that `docs/_config.yml` does not load jekyll-optional-front-matter (its `plugins:` has one entry, docs/Gemfile three gems, pages.yml builds with plain `jekyll build`), and a new test pins that premise in both places it could change. The floor is RE-MEASURED, not carried across: 125 with growth 50, against a live 131. Both numbers come from counting the front-matter population at past revisions of main -- 102 at 120d, 114 at 110d/100d/80d, 116 at 70d, 120 at 50d, 119 at 40d, 117 at 20d, 131 now, i.e. +0.24 files/day with a largest drawdown of 3. floor sits 6 below today (twice that drawdown); the ceiling at 175 is 44 above it, ~180 days of headroom. The previous pair (650/150) left 61, about six weeks. The `{% raw %}` wrap added to docs/superpowers/plans/2026-05-29-resource-policy.md is reverted: that file has no front matter, is never rendered, and the wrap only added literal noise visible on GitHub. The research doc that actually broke the build does have front matter and keeps its fix. min_fraction was reconsidered rather than dismissed, because the earlier note was too strong -- `reference=` is overridable, and against the non-excluded docs count the ratio is stable to within 0.165..0.182 across nine revisions. Not taken, for a measured reason recorded in the file: 0.15 of 739 is 110, so a fraction tolerates losing 21 files where floor=125 fires on the 7th, and the staleness it buys does not apply to a population growing at a quarter of a file a day. 4. THE GUARD WAS NOT COVERED, AND THE FIRST ATTEMPT REVERSED A RULING. `.github/filters/ python-paths.yml` reached only `docs/research/**` and one guide, so a docs-only PR elsewhere never ran repo_tests. That was "fixed" by appending to `repo_tests/_glob_declared_uncovered.py`, whose own header forbids exactly that ("Never add one to make a new uncovered dependency pass"). Reverted. The filter now covers `docs/**/*.md` and `docs/_config.yml`, following the #17893 widening beside it, and the guard declares `docs/**/*.md` instead of a bare `*.md` so the coverage checker can match the two. The walk stays rooted at `docs/`: a `**` glob from the repository root is what repo_root_walks_use_git_15955_test exists to reject. Ripple, stated because it reverses a second recorded trade: seven docs entries in `python_filter_uncovered_reads.py` are no longer bypasses, so MAX_UNCOVERED_READS drops 45 -> 38. One of them (#17133's GITHUB_FILING_CREDENTIAL_ROTATION.md) was accepted on the grounds that covering it ALONE would cost twelve shards; the tree is now covered for a stronger reason, so that trade has nothing left to buy. Mutation-proved, each in a fresh interpreter because `_reach._MEASURED` memoises the population and an in-process mutation would appear to survive: removing the comment strip fails 2, swapping the strip order fails 1, removing the raw strip fails 5, a clean `unbalanced_tags` fails 6, dropping the stray-end check 2, the unclosed check 2, the inner-tag check 1; `has_front_matter` forced true fails 3 and false fails 6, dropping the front-matter filter fails 3, floor=1 fails 1 and floor=900 fails 5, enabling optional-front-matter in _config.yml fails 1. Planted in a real page: a foreign tag, a stray `{% endif %}`, an unclosed `{% if %}` and reverting the research doc's raw guard each fail the tree assertion; the same foreign tag planted in a front-matter-less file correctly does not. 36 pass in this guard, 526 across it plus every reach and filter meta-test. Refs #17930, #17844
mrveiss
added a commit
that referenced
this pull request
Oct 4, 2026
… the python filter (#17930) Owner decision on the CI-spend question the previous commit flagged: narrow it. `docs/**/*.md` and `docs/_config.yml` are OUT of `.github/filters/python-paths.yml` again, so a documentation edit no longer starts the twelve-shard python suite. `repo_tests/python_filter_uncovered_reads.py` is restored byte for byte -- MAX_UNCOVERED_READS back to 45 and the seven docs entries back, including `docs/developer/GITHUB_FILING_CREDENTIAL_ROTATION.md`, whose #17133 trade is left intact rather than reversed by a widening it never asked for. THE GUARD STILL HAS TO RUN, and `pages.yml` cannot be the thing that runs it: it is `push: branches: [main]` only, so it reports a dark site after the merge that darkened it. The whole point is to fail on the pull request. So `.github/workflows/docs-liquid-tags.yml` -- `pull_request` + `push: main`, gated on `docs/**/*.md` and `docs/_config.yml`, one job, one test file. Modelled on `ansible-file-references.yml`, which is the lightest workflow of this shape here. The dependency set is MEASURED, not assumed: `pyyaml pytest pytest-asyncio` in a bare venv runs this guard from the repo rootdir -- root conftest, both its plugins and `repo_tests/conftest.py` included -- in 0.45s, 36 passed. Same three packages `ansible-file-references.yml` already proves in CI. NOT A `_glob_declared_uncovered` ENTRY, which is the part worth reading. That record's header has always had two exits: an entry leaves when the filter covers its tree, "or when a cheaper route runs that guard on its own trigger". The second exit had no mechanism, so taking it would have meant writing a sentence -- and a record whose entries are sentences is the exemption the same header forbids. There is no existing registry for "runs on its own trigger"; I looked, and say so rather than implying one was reused. The smallest honest mechanism instead: a second record, `GLOB_RUN_BY_A_DEDICATED_WORKFLOW`, every entry of which is VERIFIED against the workflow's parsed YAML rather than believed -- * the named workflow exists; * its `pull_request` `paths:` fire on the tree the glob names, checked with the SAME matcher the python filter is checked with, so the two cannot drift; * a `run:` step in it invokes each guard recorded against the glob; * and the staleness direction: an entry whose tree the filter now covers, or that no guard declares any more, FAILS rather than lapsing into decoration. Parsed, never grepped. A comment naming the guard, a `name:` quoting it or a path mentioned in prose satisfies a text search while running nothing, and that exact shape -- a check keyed on a sentence that resembles the thing it looks for -- bit this branch twice today. `test_a_workflow_that_only_MENTIONS_the_guard_does_not_count` is the in-suite control for it, and `test_the_trigger_block_is_read_through_yamls_boolean_on_key` is the control for the other way this check could be vacuous: YAML 1.1 folds a bare `on:` key to the boolean True, so a helper looking up the string finds nothing and every path assertion downstream passes while asserting about `{}`. ONE filter line is added, and it is the workflow file itself -- `.github/workflows/docs-liquid-tags.yml` -- because the record now names it by concrete literal and a change confined to it is precisely the change that can falsify the record's claim. Without that line MAX_UNCOVERED_READS goes to 46 and the equality fails; with it the count is 45, unchanged. Same shape as the `auto-fix-generated-types.yml` and `stylelint-tokens.yml` entries beside it. `DOCS_GLOB = "docs/**/*.md"` and the `docs.rglob(leaf)` walk are unchanged: the glob has to be the one the workflow's `paths:` are compared against, and the walk stays rooted at `docs/` because `repo_root_walks_use_git_15955_test.py:136` rejects a `**` glob taken from the repository root. NOT A REQUIRED STATUS CONTEXT. New workflows are not required by default and branch protection is an owner-only change, so this reports and does not block. Promoting it is a separate, deliberate step. Mutation-proved, eight ways, all killing `test_a_dedicated_workflow_entry_really_runs_that_guard`: dropping `docs/**/*.md` from the workflow's paths, deleting the pytest step, downgrading that step to a comment plus a matching `name:` (the case above), removing the `pull_request` trigger entirely so it takes `pages.yml`'s shape, pointing the record at a nonexistent workflow, naming a guard that does not declare the glob, renaming the glob so no guard declares it (also fails `test_every_uncovered_glob_declaration_is_recorded`), and widening the filter to `docs/**/*.md` after all (also fails `test_the_uncovered_record_only_shrinks`). 85 pass across the Liquid guard and the two filter meta-tests; 219 across the workflow, required-context and CI-wiring guards. Also filed: #17940, a sub-issue of #15826 -- `excluded_dirs()` reads `repo_root()` rather than the `root` its caller was handed, so the empty-tree mutation passes for the wrong reason. Not fixed here: the obvious repair introduces a silent-empty path in which an unreadable `docs/_config.yml` means "nothing is excluded". Seven candidate guards listed there, one confirmed. Refs #17930, #17844, #17940
mrveiss
added a commit
that referenced
this pull request
Oct 4, 2026
…is backup produced nothing for ~125 days, and Jekyll broke on quoted template syntax (#17897, #17932, #17930) * fix(ansible): presence detection counted apt sources apt never reads (#17897) A clean-machine install died at step 4 of 7 with "No package matching 'grafana' is available". Two defects, one cause and its missing detector. The cause. The failing node carried /etc/apt/sources.list.d/grafana.list.distUpgrade -- correct content, correct keyring, and inert: sources.list(5) has apt parse only names ending .list or .sources. do-release-upgrade renames third-party sources that way and the suffix had been baked into the device image. The helper's recursive grep found the match in that inert file, reported PRESENT, preserved a repository apt had never read, and the canonical grafana.list was never written. The same host showed the contrast that proves this is the suffix and not the content: nodesource.list.distUpgrade was equally inert, nodesource.sources sat beside it, and nodesource installed fine. Detection is now scoped to the two suffixes apt parses; matching semantics, directory and reach are otherwise unchanged. The detector. The verdict short-circuits to 'usable' when no caller names a package -- defensible per call site, wrong as a fleet property. Six of seven call sites inherited that untested trust; only postgresql named a package. Each of the six now names the package its own role installs after adding the repo, derived from the role rather than guessed: slm_manager/grafana grafana (its own apt task) monitoring/grafana grafana (its own apt task) docker/main docker-ce (the engine; the distribution archive ships docker.io, never docker-ce) redis/main redis-stack-server (its own apt task) python_interpreter/main python_interpreter_packages | first (derived, not restated, so check and install cannot disagree) agent_config/openvino openvino-{{ openvino_version }} (the only unconditional install from that repo) Either fix alone unblocks the install; together the detection is honest and the trust is declared. A new guard turns the default into a decision: every include_tasks of the helper must pass a non-empty apt_repo_verify_package, with a documented shrink-only exemption list (empty, ceiling 0) for a role that genuinely cannot name one. The guard fails if it resolves zero call sites, so "found nothing" cannot read as "all compliant", and the detection tests execute the real shell against a fixture rebuilt from the failing host. Refs #17897 * fix(ansible): the nightly Redis backup produced no archive for 125 days (#17932) Refs #17932 `roles/redis/templates/redis-backup.sh.j2` has written zero `.tar.gz` since at least 2026-05-31 while logging a success line on every run. Two defects, plus the property that let them survive. 1. trim_blocks. Ansible renders with `trim_blocks=True`, which eats the newline after a block tag. The template ended the `tar` line with a block tag and began the next line with `rm`, so the deployed script carried one fused command with `-f` twice; tar refused with "Multiple archive files require '-M'" and `set -e` aborted before the retention sweep, which has therefore never run. Fixed structurally rather than with another newline: the template now contains no block tags at all. Persistence is passed in as a rendered value and every condition is decided in shell, so no whitespace setting can move a command. 2. The AOF layout. The guard tested `$REDIS_DATA_DIR/appendonly.aof`, which Redis 7 does not have -- append-only data lives in `appendonlydir/` (the `appenddirname` default). It was false on every run, so no AOF was ever archived despite `appendonly yes`. Both layouts are now handled, directory first. Same premise found in `ansible/utils/backup.sh`, fixed here as one class rather than one instance, and a third time in `playbooks/data-migration.yml`, whose source Redis version cannot be established from this repository -- filed as #17933 with a documented, shrink-only exemption in the guard. 3. Silent success. The first line the script logs is a success line, so an abort read as a completed backup. An EXIT trap now reports a non-zero status and a final FAILED line; the completion line states what the archive holds (`aof=appendonlydir|appendonly.aof|missing|disabled`). The archive's existence is checked rather than assumed, and the BGSAVE wait now watches LASTSAVE advance instead of comparing it to `date +%s`, which exited on its first iteration and allowed a stale dump into the archive. That wait is bounded by an env-var-backed constant, and the install task gains `validate: /bin/bash -n %s` -- which would NOT have caught this defect, since the fused line parsed fine, but stops a future render that does not parse from reaching /usr/local/bin. The orphaned `dump-*.rdb` backlog on affected nodes is deliberately NOT swept: the retention sweep stays scoped to this script's own archives, because disposing of accumulated data needs an owner decision, not a script. Guard: `repo_tests/redis_backup_template_renders_runnable_17932_test.py` renders the template with Ansible's settings and executes it against a fake Redis host. Its central assertion is differential -- rendering with and without `trim_blocks` must produce identical output -- so it names no command pair and catches the class. 10 of its 14 tests fail against the template as it stands on main. * fix(docs): a Liquid tag quoted as prose broke the site build, and nothing caught it (#17930) The docs site stopped building at `125c083ca0` and nobody noticed for days: Liquid syntax error (line 64): Unknown tag 'citation' (Liquid::SyntaxError) Neither file was making a templating mistake. Both QUOTE another system's tag syntax as content: `docs/research/private-tenant-chat-reference-app.md:71` describes Markdoc's `citation` tag in prose, and `docs/superpowers/plans/2026-05-29-resource-policy.md:512` shows an Ansible/Jinja2 template in a fenced block. Jekyll runs Liquid over the raw Markdown BEFORE rendering, so neither a code span nor a fence protects the literal -- Liquid reaches it and fails on a tag it does not define. TWO files, not one. The reported failure named only `citation`, because Jekyll stops at the first error. Enumerating every `{% tag %}` in the tree found `set`, `macro` and `endmacro` as well -- Jinja2 spellings Liquid has no equivalent for. `macro`/`endmacro` are in `docs/archives/`, which `docs/_config.yml` excludes from processing, so they are inert; `set` is in `docs/superpowers/`, which is NOT excluded, so it was the next failure waiting behind this one. Fixing only the reported file would have produced a second identical red. Both wrapped in a raw guard. The two obvious alternatives are both wrong and are recorded in the issue: defining `citation` as a Liquid plugin would invent a tag to satisfy a line quoting a different templating system -- and there are zero files under `docs/_plugins` -- while deleting the literal would delete the content, since the syntax is what the paragraph is about. The explanatory comment names those tags in PROSE rather than writing them out, because an HTML comment is not protected either. My first version of it spelled them literally and would have reintroduced the defect inside its own explanation. The guard is the half that matters. `repo_tests/docs_liquid_tags_are_defined_17930_test.py` compares every tag in the processed tree against the set Liquid and Jekyll define. Three design points: * It strips raw-guarded regions FIRST. Without that it would reject the fix for this very defect -- mutation-proved: removing the strip fails 3 tests including the raw-guarded case. * Its contrast case accepts DEFINED tags (`if`, `for`, `assign`, `highlight`, `post_url`, `raw`), so it cannot degenerate into "no `{% %}` in docs" -- which would pass this issue's acceptance test while forbidding all legitimate templating. * `EXCLUDED_DIRS` is read from `docs/_config.yml`'s own `exclude:` rather than hand-copied, with a test pinning that `archives` is still excluded -- a hand-copied list drifts from the renderer's, and `archives` is the live case where text and build disagree. Reach floor at 200 processed docs, because this one discovers its inputs and a glob that stops matching would otherwise report clean having scanned nothing. Why it hid for days: `build` runs only when `docs/**` is in the changeset, so it is absent from most PRs and present on docs PRs. Five consecutive `main` commits showed it failing on three and absent on two, which reads as flake and is a path filter. Mutation-proved four ways: reverting either file's raw guard fails the tree assertion, removing the raw-stripping fails 3, and an unsatisfiable reach floor fails. 18 pass. Closes #17930 Refs #17835 * test(guards): register the Liquid guard's *.md glob in the declared-reads record (#17930) glob_declared_reads_15900_test asserts the record names exactly the guards declaring each glob. The new docs guard declares *.md and was absent, so the set comparison failed on push -- the record is hand-kept by design, which is what makes it catch a new declarer rather than drift with one. Refs #17930 * test(docs): migrate the Liquid-tag guard's reach floor to _reach.declare (#17930) The hand-rolled MIN_DOCS_SCANNED = 200 sat 3.7x below the real population of 739, so the floor would have gone on passing after the glob lost three quarters of the tree -- which is why reach_floor_migration_test refuses a carried-across constant (#15928). Re-measured rather than converted. Deliberately absolute, not a min_fraction against all *.md under docs/ (846, ratio 0.874): archives/ is excluded and only ever grows, so every archived doc leaves the numerator and stays in the denominator, walking the fraction down into a false fire. The window is wide for the same reason -- archiving shrinks this population without the glob breaking. * fix(guards): the Liquid guard was hollow on structure and sized over the wrong population (#17930) Four defects found in review of the guard added earlier on this branch, all confirmed with a control before being fixed. 1. FALSE POSITIVE ON COMMENT BLOCKS. `undefined_tags("{% comment %}{% citation %} {% endcomment %}")` returned `['citation']`. Liquid's `comment` block overrides `unknown_tag` to do nothing, so Jekyll accepts that Markdown and the guard rejected it. Comment regions are now stripped like raw regions -- AFTER them, because a `{% comment %}` quoted inside a raw block would otherwise open a region that swallows every violation up to an unrelated `{% endcomment %}`. Fixtures cover both directions and the ordering. 2. HOLLOW ON BALANCE. `{% endif %}` alone, `{% else %}` alone, `{% endfor %}` alone, an unclosed `{% if %}` and an unclosed `{% raw %}` all returned `[]`, and every one is a Liquid::SyntaxError built entirely from WHITELISTED names -- checking names could not see any of them. `unbalanced_tags` adds a block/end stack over the same stripped text, with a fixture per case and seven well-formed contrasts so it cannot degenerate into rejecting all templating. The sibling fix riding this branch (#17932) is itself a stray `{% endif %}`. 3. THE POPULATION WAS MISLABELLED (#17844). `what="Jekyll-processed Markdown docs"` was false: Jekyll runs Liquid only on files WITH front matter, and 131 of the 739 scanned files have it. The other 608 are copied verbatim, can never break the build, and could only ever yield false positives -- and the floor was sized over them. Verified first that `docs/_config.yml` does not load jekyll-optional-front-matter (its `plugins:` has one entry, docs/Gemfile three gems, pages.yml builds with plain `jekyll build`), and a new test pins that premise in both places it could change. The floor is RE-MEASURED, not carried across: 125 with growth 50, against a live 131. Both numbers come from counting the front-matter population at past revisions of main -- 102 at 120d, 114 at 110d/100d/80d, 116 at 70d, 120 at 50d, 119 at 40d, 117 at 20d, 131 now, i.e. +0.24 files/day with a largest drawdown of 3. floor sits 6 below today (twice that drawdown); the ceiling at 175 is 44 above it, ~180 days of headroom. The previous pair (650/150) left 61, about six weeks. The `{% raw %}` wrap added to docs/superpowers/plans/2026-05-29-resource-policy.md is reverted: that file has no front matter, is never rendered, and the wrap only added literal noise visible on GitHub. The research doc that actually broke the build does have front matter and keeps its fix. min_fraction was reconsidered rather than dismissed, because the earlier note was too strong -- `reference=` is overridable, and against the non-excluded docs count the ratio is stable to within 0.165..0.182 across nine revisions. Not taken, for a measured reason recorded in the file: 0.15 of 739 is 110, so a fraction tolerates losing 21 files where floor=125 fires on the 7th, and the staleness it buys does not apply to a population growing at a quarter of a file a day. 4. THE GUARD WAS NOT COVERED, AND THE FIRST ATTEMPT REVERSED A RULING. `.github/filters/ python-paths.yml` reached only `docs/research/**` and one guide, so a docs-only PR elsewhere never ran repo_tests. That was "fixed" by appending to `repo_tests/_glob_declared_uncovered.py`, whose own header forbids exactly that ("Never add one to make a new uncovered dependency pass"). Reverted. The filter now covers `docs/**/*.md` and `docs/_config.yml`, following the #17893 widening beside it, and the guard declares `docs/**/*.md` instead of a bare `*.md` so the coverage checker can match the two. The walk stays rooted at `docs/`: a `**` glob from the repository root is what repo_root_walks_use_git_15955_test exists to reject. Ripple, stated because it reverses a second recorded trade: seven docs entries in `python_filter_uncovered_reads.py` are no longer bypasses, so MAX_UNCOVERED_READS drops 45 -> 38. One of them (#17133's GITHUB_FILING_CREDENTIAL_ROTATION.md) was accepted on the grounds that covering it ALONE would cost twelve shards; the tree is now covered for a stronger reason, so that trade has nothing left to buy. Mutation-proved, each in a fresh interpreter because `_reach._MEASURED` memoises the population and an in-process mutation would appear to survive: removing the comment strip fails 2, swapping the strip order fails 1, removing the raw strip fails 5, a clean `unbalanced_tags` fails 6, dropping the stray-end check 2, the unclosed check 2, the inner-tag check 1; `has_front_matter` forced true fails 3 and false fails 6, dropping the front-matter filter fails 3, floor=1 fails 1 and floor=900 fails 5, enabling optional-front-matter in _config.yml fails 1. Planted in a real page: a foreign tag, a stray `{% endif %}`, an unclosed `{% if %}` and reverting the research doc's raw guard each fail the tree assertion; the same foreign tag planted in a front-matter-less file correctly does not. 36 pass in this guard, 526 across it plus every reach and filter meta-test. Refs #17930, #17844 * fix(redis): the nightly backup sent no credential, so an error reply read as a timestamp (#17932) CodeRabbit's finding on redis-backup.sh.j2:66-69, confirmed from the codebase (no production access was used or attempted). `roles/redis/templates/redis-stack.conf.j2` gives this role's own server a `requirepass` directive whenever the role has a password, so an auth-enabled host is a rendered configuration of this very role rather than a hypothetical. The backup script's three `redis-cli` calls sent nothing at all -- no password, no username, no environment variable -- while the role's own readiness probe in tasks/main.yml has always sent both. The reason this is not a nit is the exit code. Without `-e`, redis-cli PRINTS a server error reply on stdout and still exits 0, so `LASTSAVE` returned the literal string `NOAUTH Authentication required.`; the wait loop then compared that string against itself, and the run either spun out its five-minute budget or archived a stale dump.rdb. That is the same "logs a success line, produces no usable archive" failure #17932 exists to end, arriving by a second route -- so closing the issue on the current fix would have closed it on a fleet that still produced zero archives. TWO FIXES, NOT ONE. * The credential is SENT. `REDISCLI_AUTH` from the environment rather than `-a`, so it never reaches the process list, plus `--user` to mirror the readiness probe (the adopted SLM credential authenticates as a username, and a bare password is refused). * An error reply is REFUSED. `LASTSAVE` is validated by shape -- a reply that is not a bare integer ends the run naming what came back. Shape rather than `-e`: the flag exists in current redis-cli, but this repository does not establish the fleet's version, and a shape check is version-independent and strictly broader. The BGSAVE reply is logged, not gated: Redis answers an in-progress save with an error, and rejecting that would abort a run about to succeed. WHERE THE CREDENTIAL LIVES. Not in the script, and that is a ruling rather than a style choice. The script is deployed 0755 and is re-rendered on the code-only update path, where `redis_password` resolves from `vault_redis_password` -- which no inventory here defines. The "DELIBERATELY EXCLUDED" note at the top of roles/redis/tasks/code_only.yml keeps every artifact whose content depends on that variable off the update path for exactly that reason, so substituting it into the script would have reversed that ruling twice: a secret in a world-readable file, and a secret an update silently empties. The script therefore carries only the PATH; `redis-backup.env.j2` carries the value, 0600 root, written from the provisioning path only, and parsed with sed rather than sourced so a credential is never executed. The wait loop is also an explicit assignment now: a command substitution inside a `while` condition is one of the places `set -e` does not abort, so a LASTSAVE that started failing mid-wait would have been re-reported every two seconds instead of ending the run. ALSO, the same review's second finding: the "Move aside" task still used a bare `grep -rlsZ` while #17897 scoped the DETECT step to `*.list`/`*.sources`, so an `unusable` verdict displaced the inert `.distUpgrade`/`.save`/`.bak` siblings too -- files outside the population the verdict was computed over. Scoped identically. While there, the detect step gained `-F`, which the move step always had: `apt_repo_match` is a URL whose dots are regex wildcards, and two steps in one decision must not read the pattern in two dialects. TESTS. The existing e2e tests shadow redis-cli, so they could not see any of this; the fake now records argv and REDISCLI_AUTH per call and can answer with a server error. Six new assertions, in a new guard because the old file reached the 600-line ceiling (#5060) -- the harness is split out to repo_tests/_redis_backup_harness.py so both guards drive one render rather than two that can diverge. The harness also pins the BGSAVE budget short, so a run that WAITS instead of failing is observable rather than killing the subprocess. Mutation-proved: reverting the LASTSAVE calls to bare fails 2, the BGSAVE call 2, exporting REDISCLI_AUTH blank 1, always sending --user 1, removing the shape check 1 (and it then takes the full wait budget), never reading the credential file 1, substituting the credential into the script 14, `-a` at a call site 1, `-a` added to the shared argument array 2, and an env template that emits no password 2. On the apt helper: reverting the move to a recursive grep, dropping its `.sources` name test, and removing `-F` from detect each fail the new agreement test. 21 pass in the two redis guards, 18 in the apt helper's, 5150 across repo_tests (the three failures and 59 errors are this box's Python 3.10 against a tree that needs 3.11+, all pre-existing and none in a file this branch touches). Refs #17932, #17897 * fix(ci): run the Liquid guard on a docs-gated job instead of widening the python filter (#17930) Owner decision on the CI-spend question the previous commit flagged: narrow it. `docs/**/*.md` and `docs/_config.yml` are OUT of `.github/filters/python-paths.yml` again, so a documentation edit no longer starts the twelve-shard python suite. `repo_tests/python_filter_uncovered_reads.py` is restored byte for byte -- MAX_UNCOVERED_READS back to 45 and the seven docs entries back, including `docs/developer/GITHUB_FILING_CREDENTIAL_ROTATION.md`, whose #17133 trade is left intact rather than reversed by a widening it never asked for. THE GUARD STILL HAS TO RUN, and `pages.yml` cannot be the thing that runs it: it is `push: branches: [main]` only, so it reports a dark site after the merge that darkened it. The whole point is to fail on the pull request. So `.github/workflows/docs-liquid-tags.yml` -- `pull_request` + `push: main`, gated on `docs/**/*.md` and `docs/_config.yml`, one job, one test file. Modelled on `ansible-file-references.yml`, which is the lightest workflow of this shape here. The dependency set is MEASURED, not assumed: `pyyaml pytest pytest-asyncio` in a bare venv runs this guard from the repo rootdir -- root conftest, both its plugins and `repo_tests/conftest.py` included -- in 0.45s, 36 passed. Same three packages `ansible-file-references.yml` already proves in CI. NOT A `_glob_declared_uncovered` ENTRY, which is the part worth reading. That record's header has always had two exits: an entry leaves when the filter covers its tree, "or when a cheaper route runs that guard on its own trigger". The second exit had no mechanism, so taking it would have meant writing a sentence -- and a record whose entries are sentences is the exemption the same header forbids. There is no existing registry for "runs on its own trigger"; I looked, and say so rather than implying one was reused. The smallest honest mechanism instead: a second record, `GLOB_RUN_BY_A_DEDICATED_WORKFLOW`, every entry of which is VERIFIED against the workflow's parsed YAML rather than believed -- * the named workflow exists; * its `pull_request` `paths:` fire on the tree the glob names, checked with the SAME matcher the python filter is checked with, so the two cannot drift; * a `run:` step in it invokes each guard recorded against the glob; * and the staleness direction: an entry whose tree the filter now covers, or that no guard declares any more, FAILS rather than lapsing into decoration. Parsed, never grepped. A comment naming the guard, a `name:` quoting it or a path mentioned in prose satisfies a text search while running nothing, and that exact shape -- a check keyed on a sentence that resembles the thing it looks for -- bit this branch twice today. `test_a_workflow_that_only_MENTIONS_the_guard_does_not_count` is the in-suite control for it, and `test_the_trigger_block_is_read_through_yamls_boolean_on_key` is the control for the other way this check could be vacuous: YAML 1.1 folds a bare `on:` key to the boolean True, so a helper looking up the string finds nothing and every path assertion downstream passes while asserting about `{}`. ONE filter line is added, and it is the workflow file itself -- `.github/workflows/docs-liquid-tags.yml` -- because the record now names it by concrete literal and a change confined to it is precisely the change that can falsify the record's claim. Without that line MAX_UNCOVERED_READS goes to 46 and the equality fails; with it the count is 45, unchanged. Same shape as the `auto-fix-generated-types.yml` and `stylelint-tokens.yml` entries beside it. `DOCS_GLOB = "docs/**/*.md"` and the `docs.rglob(leaf)` walk are unchanged: the glob has to be the one the workflow's `paths:` are compared against, and the walk stays rooted at `docs/` because `repo_root_walks_use_git_15955_test.py:136` rejects a `**` glob taken from the repository root. NOT A REQUIRED STATUS CONTEXT. New workflows are not required by default and branch protection is an owner-only change, so this reports and does not block. Promoting it is a separate, deliberate step. Mutation-proved, eight ways, all killing `test_a_dedicated_workflow_entry_really_runs_that_guard`: dropping `docs/**/*.md` from the workflow's paths, deleting the pytest step, downgrading that step to a comment plus a matching `name:` (the case above), removing the `pull_request` trigger entirely so it takes `pages.yml`'s shape, pointing the record at a nonexistent workflow, naming a guard that does not declare the glob, renaming the glob so no guard declares it (also fails `test_every_uncovered_glob_declaration_is_recorded`), and widening the filter to `docs/**/*.md` after all (also fails `test_the_uncovered_record_only_shrinks`). 85 pass across the Liquid guard and the two filter meta-tests; 219 across the workflow, required-context and CI-wiring guards. Also filed: #17940, a sub-issue of #15826 -- `excluded_dirs()` reads `repo_root()` rather than the `root` its caller was handed, so the empty-tree mutation passes for the wrong reason. Not fixed here: the obvious repair introduces a silent-empty path in which an unreadable `docs/_config.yml` means "nothing is excluded". Seven candidate guards listed there, one confirmed. Refs #17930, #17844, #17940 * fix(guards): the credential guard read a comment EXPLAINING the ruling as a breach of it (#17932) test_code_only_renders_nothing_that_carries_the_redis_password matched "redis_password" anywhere in a rendered template. redis-backup.sh.j2 documents, in a shell comment, that the credential is deliberately NOT substituted into it -- so the guard failed CI for documenting its own fix. The #15771 shape, catalogued as a class in #17941; sixth witness today. The question it should ask is whether rendering can EMIT the value, not whether the file mentions the name. Only a {{ ... }} or {% ... %} can, so that is what it now matches, after stripping {# ... #} regions. Shell comments are deliberately NOT stripped, which is the half that is easy to get wrong: Jinja has no notion of shell syntax, so "# {{ redis_password }}" still renders and lands the credential in the file. Stripping them would have traded a false positive for a real hole. Four controls pin the distinction, including that redis-stack.conf.j2 -- a genuine offender -- is still caught, so the narrowing did not cost the guard its subjects. * test(redis): the bare-call detector read physical lines, so a continued call was invisible (#17932) `_command_lines` splits on newlines, so `redis-cli\` followed by its arguments produces no line containing `redis-cli `. The detector walked past it while the remaining calls still cleared the count floor -- a number reported, the miss invisible. It was measuring the lines it could see, not the calls that run. Backslash-newline is now joined first, which is what the shell does before executing. Latent today: no call in the template is continued. Fixed anyway, because what the guard is wrong about is an unauthenticated call shipping unnoticed, which is the defect this whole issue is. Adds the contrast pair that was missing -- bare and authenticated, each in plain and continued form. Without the continued cases the detector cannot be shown to catch anything the line-oriented version missed, and that version passed the real template happily while blind to exactly that shape.
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
2 participants
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
The owner, 2026-09-19: "fix all red, review and merge, check if any are consolidation candidates", with "merge the existing merge trains before producing new ones". After #17048 turned
maingreen, the approved non-security trains had to land. Landing them one at a time would cost one CI cycle each through the FIFO python-suite lanes, so they are merged server-side into this one branch instead: one CI run proves the union. The migrations (#17072, then #17125) stay separate as their own risk class. The security trains ridevehicle-v090-2026-09-19-security2.What Changed
This branch was cut from
mainafter #17048. Each member was merged server-side at its carried head, and all five merges were clean.vehicle-v090-2026-09-19-7b74dc31be94vehicle-v090-2026-09-19-77e1533c96c1vehicle-v090-2026-09-19-bb4ecfcd132eissue-16310-unshallow9fc53562aabatching-check-advisory75878df3dfOn top of that there is one commit,
760c54bd0b(#17128): the owner rule "finish what you started — append before you open" added toCLAUDE.md, next to the same-file rule.#17122 wrote its closing keywords in the comma form (
Closes #13859, #16784, #16985), which links only the first issue. The two it missed are listed here on their own lines; their ACs were reviewed on #17103 (#16784) and #17016 (#16985).Closes #13859
Closes #15603
Closes #16020
Closes #16711
Closes #16712
Closes #16722
Closes #16947
Closes #16949
Closes #16975
Closes #16986
Closes #17006
Closes #17038
Closes #17039
Closes #17128
Closes #16784
Closes #16985
Refs #16948
Refs #16429
Refs #16310
Verification
mainate1533c96c1, plus the sandbox fix inpre_push_open_pr_cap_17006_test.py) and theCLAUDE.mdcommit. Those get their own review on this PR.Model Used
Claude Opus 5 (coordinator)