Repository navigation
Conversation
… it on three leftovers (#16912, #16513, #16494, #16704) (#17321) * fix(frontend-tooling): give a worktree a resolvable node_modules and stop a skipped check reading as a pass (#16912) A frontend change made in a worktree had no local verification at all: no node_modules to resolve, so neither vitest nor vue-tsc could run, and pre-push said "skipping" and pushed anyway. Four causes, all four fixed: 1. post-checkout now links autobot-frontend/ and autobot-slm-frontend/ node_modules to the main checkout's install when it runs in a linked worktree -- the same shape as the backend symlink it already restores. Nothing is installed, and a real install in the worktree always wins. 2. autobot-slm-frontend/.gitignore pinned `node_modules/`, a directory-only pattern that does not match a symlink, so the link above showed up as an untracked file. Verified with git status before and after. 3. pre-push reports both frontend checks through could_not_run() when node_modules does not resolve, so "could not look" is no longer spelled the same way as "looked and found nothing" (MEASUREMENT_DISCIPLINE). The vitest branch had no else at all and fell through silently. 4. dependency-floor-reporter.ts could never load: it imported createLogger from @/utils/debugUtils, which re-exports from @autobot/ui, whose index pulls in .vue SFCs that fail to compile in the reporter-loading context, and it default-exported an instance where vitest calls `new`. Either one takes the whole run down at startup, and vitest.config.ts only registers this reporter when CI is unset -- so it broke local runs exclusively. Both fixed; the banner now prints for the first time. Evidence, all from inside a worktree: vitest 4.1.9 and vue-tsc 5.9.3 resolve; `npx vitest run` (the exact invocation pre-push uses) starts and passes 13/13; vue-tsc --noEmit over autobot-frontend reports 0 errors; the hook was run for real with the links removed first and recreated both. * fix(frontend): clear the three leftovers from the settings-path and SecretVault retirements (#16513) - useAsyncOperation.examples.md showed api.saveSettings() in two examples, an API #16484 removed (api.ts:247 records the removal). Both now use apiClient.saveSettings() from utils/ApiClient, which is live, and the AFTER example gains the import a reader would copy. - security.secretsManager.scopeUser / scopeSession went unreferenced when #16486 dropped those scopes from the Scope select to match ChatSecretScope. Removed from all 11 locales, brace-matched to the secretsManager block so the unrelated secrets.vault.scopeSession survives -- asserted per file, as is each file re-parsing as JSON. No dynamic key construction reaches them: every reference in the component is a literal $t('...scopeGeneral') form. - SecretsManager's virtual-list wiring had no test. The new spec covers the seam that was uncovered -- the FILTERED set reaching displayedSecrets, list mode positioning each row from the virtual item's offset, grid mode rendering the same set unpositioned -- not useVirtualList's own maths. 3/3 pass locally (worktree vitest, now that #16912 makes that possible). * fix(frontend): gate the three admin-only surfaces the UI still offered everyone (#16494) Each of these let a non-admin see and attempt an action the backend already refuses. Verified against the backend before changing anything, because it sets the severity: api/feature_flags.py has Depends(require_admin) on 8 of 8 routes, POST /api/settings/telemetry has Depends(check_admin_permission) (the GET is public on purpose), and api/themes.py gates install and uninstall the same way. So these are confusing dead-ends, not privilege escalation -- no data was reachable, only 403s. - SettingsView: the Feature Flags tab button and its section, gated on userStore.isAdmin. - TelemetrySettingsPanel: the toggle only. The state it shows comes from the public GET, so everyone keeps seeing whether telemetry is on; only an admin is offered the control that changes it. - ThemeManagerView: upload and uninstall. The listing GETs are open, so the installed-theme list stays visible. The issue's fourth item needed no change: #16442 merged and #16426 is closed, and SecretsManager.vue already gates both the infrastructure-hosts category and its quick-add button on userStore.isAdmin. v-if with the existing userStore.isAdmin pattern, matching App.vue and UsageView; no v-permission, since each of these is a plain admin/non-admin binary. No new UI strings, so no locale changes. vue-tsc --noEmit over autobot-frontend: 0 errors. * refactor(slm-frontend): finish #16239's consolidation — move ToolsView onto useFleetTools (#16704) ToolsView is the view FleetToolsTab was extracted from, and it never adopted the composable its own extraction produced: #16702's merge-conflict reconciliation kept main's independent copy of this one file. The split was an artifact, not a decision -- nothing anywhere records an intent for these two views to diverge on shared logic. - activeTool/loading/error/result, the tool-specific state, nodes and selectedNodeDetails, selectTool/closeTool now come from useFleetTools(), with the two names this view has always used renamed at the destructure (shellCommand: ansibleCommand, runShellCommand: runAnsibleCommand) so FleetToolsTab and the composable are untouched. - The local runRedisCommand/runAnsibleCommand bodies are gone; they were the composable's own implementations inlined, down to the redis-node fallback and the message keys. - runNetworkTest, runHealthCheck, serviceAction and getServiceLogs stay here -- this view alone offers them -- rebuilt on the shared runTool/requireInput the way FleetToolsTab already builds getServiceLogs. 666 -> 545 lines. The three remaining rawRequest calls are exactly the three tools this view alone offers. serviceAction keeps a local normalisation: its fallback message interpolates {action} and runTool's fallback key cannot take a parameter, so a non-Error throw still reads as it did before. Verification, stated exactly: NOT run locally. autobot-slm-frontend's node_modules in this machine's main checkout is mode drw-r--r-- -- no execute bit, so nothing inside it is reachable and neither vitest nor vue-tsc can run for the SLM app from any tree. The existing src/views/ToolsView.test.ts and FleetToolsTab.logsError.test.ts therefore run in CI only for this change. The SLM duplication count drops with this change. SLM_MAX_DUP_LINES ('2945') lives in .github/workflows/duplication-guard.yml, which this PR does not own; the gate passes when under the pin and prints the figure to lower it to. * fix(frontend-tooling): select changed .spec.ts files for the pre-push vitest run too (#16912) A fifth cause of the same defect, found while pushing this branch: the hook selected changed test files with `grep -E '\.test\.ts$'`, so a changed `.spec.ts` was never run. 51 of autobot-frontend's 246+51 test files are named that way -- including the SecretsManager virtual-list spec added in this branch, which is exactly how it surfaced. A test file that is silently not selected is the same failure as a check that could not run: nothing in the output distinguishes it from a clean pass. * docs(changelog): add the fragment for the admin-only UI gates (#16494) * test(frontend): mount ThemeManagerView with its store, and cover the admin gate it gained (#16494, #16912) The #16494 gate made this view call useUserStore(), and its existing test mounted it with no pinia active: "[pinia]: getActivePinia() was called but there was no active Pinia". That is my regression and CI caught it -- but it was reproduced locally in seconds rather than through a CI round trip, which is what #16912 in this same branch is for. The store is mocked with a per-test admin flag rather than backed by a real pinia, matching the pattern the SecretsManager spec uses, and the regression is turned into the coverage the gate did not have: the listing stays visible to a non-admin while the file input and the uninstall button do not, and an admin sees both. Full autobot-frontend suite after this fix: 297 files, 3408 passed, 1 skipped, 0 failed, in 149s -- run in the worktree.
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ion each (#16415, #13579) (#17314) * tech-debt(env-registry): state the registration contract once (#16415) Seven env_registry_* siblings restated the same import-as-side-effect contract, and env_registry_llc.py already pointed at a sibling's copy of it while naming #16415 as the issue that would consolidate it. The contract could not move INTO env_registry.py -- that file is at its grandfathered size ceiling and may not grow (#14236) -- but it did not need to: env_registry.py already states it from the importer's side, beside the imports it performs. The siblings were restating it from the other side. So this is deletion plus a reference, and nothing grows. MEASURED, because the first attempt did nothing. Replacing the paragraph with a four-line reference took the clone pairs from 24/18/16 lines down to 23/17/15 and eliminated NONE of them: jscpd's threshold is 8 lines AND 70 tokens, and the clone spans the whole docstring tail plus an irreducible code tail -- the closing quotes, `from __future__`, the registry import, and the opening `register_env_var(EnvVarSpec(` that every sibling must have. Shortening prose only shortens the clone. What works is removing enough shared PROSE that the remaining identical run falls under the 70-token floor, which the code tail alone does not reach. The reference is now one line, and the #15710 provenance paragraph that backend_services restated verbatim from agent_runtime is collapsed to a reference too. autobot_shared scope: 9 clones -> 4 clones env_registry pairs: 5 -> 0 (the acceptance criterion) SLM scope total: 2918 -> 2831 duplicated lines No behaviour change, asserted rather than assumed: base and branch both import to 257 registered variables with identical names. Pin lowered 2945 -> 2831. Recorded alongside it: the same local run measured origin/main at 2918, so the 2945 pin had carried 27 lines of slack since 2026-09-11. The gate exits 0 when a count is UNDER its pin, so a fall in duplication is never noticed and a later PR can spend it -- that is #16324's exact shape, observed here rather than argued. Closes #16415 Refs #16324 * fix(ci): land the durations this workflow only ever measured (#17313) `test-durations.yml` was named "Measure and store per-test durations". Its last step was upload-artifact. Nothing committed anything, so every weekly run did 2-3h of real work and threw the result away: .test_durations last committed 2026-08-17 (37 days) by #14413 .test_durations_slm last committed 2026-08-03 (51 days) by #13314 Both last written by ordinary feature PRs, never by the workflow that owns them. Across the last 100 workflow runs in this repository it has no completed run at all. Not a breakage -- pytest-split assigns the mean duration to any test it has no timing for, so the split decays rather than failing. The cost is gradual shard imbalance across 12 python-suite and 12 coverage shards, growing with every test added since August. It did NOT cause the shard 11/12 and 12/12 failures on #17303; those were a stale fixture dependency list and a reach floor, both separately diagnosed. A SEPARATE `land` job, not `contents: write` on the existing one. The obvious fix would put a repository-write token in the same job that runs the entire test suite, within reach of any test, fixture or transitive import. `store-durations` stays read-only and hands over an artifact; `land` never runs a test. It fails closed on the way in: an empty file, or one with fewer than 100 timings, is refused rather than committed over good data -- pytest-split would then give every test the mean, which is worse than the stale file it replaced. And it distinguishes the two states the old workflow could not: "completed, nothing changed" now says so, instead of looking identical from outside to "never landed anything". The measuring job is renamed to "Measure per-test durations", because that is what it does. Guards, each mutation-verified: landing job deleted -> 4 FAILED no longer runs on the cron -> FAILED ..._runs_on_the_scheduled_refresh suite job handed a write token -> FAILED ..._never_holds_a_write_token emptiness check removed -> FAILED ..._empty_or_truncated_..._refused The age check uses a 90-day rot floor rather than a tight one: a threshold that fires on ordinary lateness gets muted, and a muted guard is worse than none. The automation is pinned alongside it, because an age check on its own would take three months to notice the landing job had been deleted. Refs #17313 * chore(slm): build the filtered-install command once, not five times (#17166) Five roles hand-wrote the same `build-filtered-requirements.sh` invocation in role_registry.py. These are deploy commands: nothing executes them until a real deploy, so a wrong character survives every test in the suite. They now come from one `_filtered_pip_install` helper. PROVEN unchanged, not asserted. All ten `post_sync_cmd` values were extracted from the AST before and after -- evaluating the f-strings, and on the after side actually calling the helper -- and diffed: IDENTICAL -- all 10 post_sync_cmd values byte-for-byte unchanged The five strings are pinned as literals in a test, so the refactor is an identity the suite enforces rather than one this message claims. `tag` is an argument rather than derived from the working directory: the backend role writes /tmp/requirements-filtered-slm.txt while slm-backend writes ...-slm-backend.txt. That reads like a typo and is not one; deriving it -- the obvious simplification -- silently changes two live deploy commands. A test pins the oddity so the next reader does not "fix" it. Mutation-verified: tag derived from workdir -> 2 FAILED a sixth hand-written call -> FAILED ..._no_role_still_hand_writes_... The new tests are their own module: test_role_registry.py is grandfathered at 846 lines and may not grow, and it owns 100 lines of sys.modules surgery to get the real registry past conftest's stubs. The new module imports that bootstrap rather than copying it, which would have been this issue's own defect in the test tree. role_registry.py came out at 713 against a 715 ceiling, so the ceiling is lowered to 713 in both places that record it -- the ratchet only turns down, and an unlowered ceiling re-licenses the lines just cut. AC3 OF THE ISSUE CANNOT BE TICKED, and that is the finding. It asks for the duplication-guard pin to be "lowered in the same PR to reflect the reduction". There is no reduction to reflect. Measured on the SLM scope with the workflow's exact flags, before and after: 2831 duplicated lines -> 2831 duplicated lines The five blocks were never clones. They differ in working directory and temp-file name, so no 8-line/70-token identical run existed and jscpd never counted them. Five near-copies of one command, invisible to the guard whose job is duplication -- the same shape as #16415, where prose had to be cut past the token floor before the pin moved at all, and the general case filed as #17312. So the criterion is left unticked with its measurement rather than ticked to tidy the issue, and this says Refs, not Closes. Refs #17166, #17312 * fix(security): one translation from a rejected path to an HTTP refusal (#13579) Six call sites turned a path-validator ValueError into an HTTP error by hand, and disagreed: three 400, two 403, one 404. The status a caller saw for the same rejection depended on which endpoint they hit. The issue said five. It is six -- api/data_storage.py was not on its list. Counted by walking every module that calls either validator and matching `except ValueError` windows that raise, rather than by trusting the enumeration. THE STATUS IS 400, and the reasoning is not new. THREAT_MODEL already records the ruling for session ownership (#14012): creating over an existing id returns 409 identically for "owned by someone else" and "no recorded owner", because a 403 on the first would confirm who owns it. Same shape here. `validate_path` refuses for several reasons -- decoding failure, `..`, absolute, drive qualifier, containment -- and deliberately does not say which; containment is the sole authority and the rest are defence in depth. 403 breaks that: it asserts "a real target you may not have", which a lexical validator never established, and it lets a caller separate "outside the roots" from "malformed" and map the boundary one request at a time. 400 is true of every refusal reason and distinguishes none of them. It was already the majority, three of six. The detail is a fixed "Invalid path". The offending value is logged and returned to nobody: it reaches the client, and echoing it answers the probe it was sent to make. api/files.py was returning 403 "Path outside allowed directories", which said precisely which boundary had been hit. ONE SITE IS NOT CONVERGED, deliberately, and this is the interesting half. transcriber/routes/recordings.py answers 404 -- and it was already right. The next line returns 404 for a missing file, so answering anything else for an out-of-bounds path would separate "not yours" from "not there" and leak the upload directory's shape. 404 for both is strictly MORE opaque than the shared default, so folding it in would have been a regression. It keeps its own handler with the reason written at the call site, rather than the helper growing a status-code parameter that would re-open the per-endpoint decision the helper exists to close. The helper lives beside path_validator, not in it: path_validator has no framework import and should keep none, or every consumer of validation takes a FastAPI dependency. Verified: hand-written translations 6 -> 1 (the documented 404) status flipped to 403 -> FAILED test_the_shared_status_is_not_403 detail echoes the path -> FAILED 4 tests pyflakes on all 5 touched modules -> clean Both files that shrank have their ceilings lowered in the same change -- files.py 1368 -> 1365, logs.py 1029 -> 1027 -- because an unlowered ceiling re-licenses the lines just cut. One test corrected mid-flight rather than shipped wrong: `~/secrets` is NOT rejected, and should not be. `~` is shell expansion, not a path feature, so it is an ordinary directory name that stays inside the base. `resolve_within_sandbox` does forbid it -- a different function with a stricter contract -- and the distinction is easy to misread as a gap. Closes #13579 * test(api): fail when two modules register the same route (#16908) Registration order was the only thing deciding which of two implementations served a shared (method, path), and nothing checked it. FastAPI matches in order, so the first wins and the second is unreachable -- dead code that reads as a live endpoint. The issue's table is STALE, which is the first finding. All four pairs it lists are resolved; feature_routers.py:552 records the scheduler shadowing being deleted under this issue. Re-measuring rather than trusting the table found a different collision the issue does not mention: POST /api/voice/realtime/tools/call api.voice:realtime_router (core, registered first -- serves) api.realtime_session:router (feature -- dead) Two scanner bugs had to be fixed before that verdict meant anything, and both would otherwise have been reported as fact: * Attribution by FILE, not by router OBJECT. `from api.voice import realtime_router as voice_realtime_router` registers realtime_router, not the module's `router`. api.voice registers BOTH at /voice, so attributing every decorator in the file to each registration made the module collide with itself -- noise that buried the real cross-module collision. * `api.redis_mcp` is a package, not a module file. The scan could not resolve it and said so; counting it as "no routes" would have been the familiar defect of reading "could not look" as "found nothing". The guard now fails on an unresolvable module rather than skipping it. before: 1522 pairs, 1 unresolved, 1 duplicate (the wrong one) after: 1518 pairs, 0 unresolved, 1 duplicate (real) THE COLLISION IS RECORDED, NOT RESOLVED, and that is deliberate. The two handlers are different implementations and they differ in DECLARED AUTH: the winner takes `Depends(get_current_user)` and checks `is_admin_role` (#12717); the dead one declares neither and passes a `session_id` through `realtime_mcp_bridge`, which the winner has no equivalent for. Deleting the dead one drops a capability; re-pathing it makes a handler with no declared auth reachable. That is a judgement about a voice feature's security surface, not a cleanup, so it is asked on the issue rather than decided here. Stated precisely: I could not establish from app_factory.py whether other middleware would cover the dead handler, so "no declared auth" is what the decorators say, not a claim that it is unauthenticated. Same accidental-correctness shape the issue already noted for knowledge_search: the safe implementation wins, and nothing makes it win. Mutation-proved, as the issue asks: a planted duplicate route -> FAILED, naming BOTH modules: "GET /api/voice/realtime/tools registered by api.voice:realtime_router, api.voice:router" a stale baseline entry -> FAILED ..._only_lists_duplicates_that_still_exist The baseline is shrink-only: entries leave when fixed and none may be added. Refs #16908 * fix(ci): build the durations PR body with a heredoc, not printf (#17313) actionlint failed the branch: SC2016, "expressions don't expand in single quotes", twice. The lines it names carry Markdown backticks -- `test-durations.yml`, `.test_durations` -- and shellcheck reads a backtick inside single quotes as command substitution that will not expand. It is right about the shell and wrong about the intent, and the intent is what has to change: printf with per-line quoting is the wrong tool for prose that contains shell metacharacters. A quoted heredoc has no expansion at all, so the body can hold backticks, and the one dynamic value is a __RUN_URL__ placeholder substituted after. Two details that had to be measured rather than assumed, because both fail silently into a malformed PR body: * The first attempt put the heredoc at column 0, which terminates the YAML block scalar. The file stopped parsing -- caught locally, not in CI. * The body is indented to sit inside that block and the indent is stripped back off. `sed 's/^ //'` was wrong: the heredoc lines carry 14 raw spaces against the block's base of 10, so 4 survive parsing and 2 would have remained on every line. Verified by rendering the body and checking the four required headings match exactly, rather than by reading the widths off the screen. Refs #17313 * docs(control): point the Phase 8 endpoints at the prefix they are served on (#16843) The doc advertised 32 endpoints under /api/control/. No router registers /control; api.advanced_control is registered at /advanced-control (feature_routers.py:380), so every documented path 404s as written. Verified per path rather than by search-and-replace confidence: all 18 distinct sub-paths in the doc were checked against the `@router.<verb>` decorators in api/advanced_control.py before the rewrite and all 18 resolve; re-checked after, and all 32 rewritten occurrences resolve to a declared route. This is #16843's third acceptance criterion. Its first -- populate `affected_tasks` from the task tracker -- is already satisfied on main: services/emergency_stop.py:49 reads `get_task_tracker().get_active_tasks().keys()`. That landed via the v0.9.1 vehicle consolidation (#17194, 245c1af), which is why the issue's own PR #16854 shows as closed-unmerged. The second criterion -- wire real task execution to check the paused-task set -- is NOT addressed here and the issue stays open for it. Refs #16843 * fix(ci): terminate the durations heredoc and drop a duplicate fi (#17313) The heredoc rewrite two commits ago shipped a script that does not parse. `bash -n` on the extracted run block: warning: here-document at line 40 delimited by end-of-file (wanted `BODY') syntax error: unexpected end of file Two bugs. The BODY terminator sat at 12 spaces against the block scalar's base indent of 10, so after YAML dedents the block it still carried two leading spaces and `<<'BODY'` (no dash) never matched it. And the `if gh pr view` was closed twice -- if/fi counted 2 against 3. Not cosmetic. The step dies at runtime on the schedule and dispatch paths, which are the only paths that matter here: the durations PR would never have opened, and the symptom would have been exactly the silence #17313 exists to fix -- a workflow that appears to run and lands nothing. Everything already in this file's guard inspects the workflow AS DATA: the YAML parses, the job is wired to the cron, the body renders with its four headings. None of that executes a shell, so all of it passed on a script bash refuses. The guard now runs `bash -n` on the landing script, and the mutation confirms it: restoring the 12-space terminator fails test_the_landing_script_is_valid_shell and nothing else. Found by peer session autobot-ai-63 reviewing the branch, and verified here independently before applying -- the diagnosis named both bugs, the line numbers and the mechanism, and all three checked out. Refs #17313 * fix(ci): repair five guards this branch's own changes tripped (#13579, #17166, #17313) Shards 7/12 and 12/12. Five failures, all caused by changes on this branch, and two of them are the guards working exactly as designed. 1. THE EXTRACTION BROKE A GUARD THAT READS SOURCE, NOT BEHAVIOUR (#17166). test_code_sync_preserves_install_14275 AST-parses role_registry.py and reads `post_sync_cmd` string literals. Moving the build-filtered-requirements.sh invocation into `_filtered_pip_install` left a Call node where a Constant was, so the guard found the rewrite in zero entries and reported its own rule vacuous -- "only [] route through the rewrite". The commands are byte-for-byte unchanged; the guard's method was what no longer reached them. It now executes the module's helpers and evaluates the call, so the check runs against the string the role will actually execute. That is strictly stronger than matching a literal: it would catch a helper that builds the WRONG command, which a literal match never could. Rule 7 says grep the behaviour, not the symbol, on an extraction PR -- this is the guard side of that same rule. 2. MY NAMING POLLUTED A DOCUMENTED AUDIT (#13579). schema_validator_names_13518 requires `grep "def validate_path"` to match exactly one thing -- the canonical containment helper -- because #13518 exists over false hits on that grep making a reviewer read schema validators as containment. `validate_path_or_http` matched it. The guard was right and the name was wrong, so the helpers are now `require_contained_path` / `require_contained_relative_path`, which say what they enforce and stay out of those results. 3. MY NEW GUARD WAS OUTSIDE THE FILTER THAT RUNS IT (#17313). durations_are_landed_and_fresh_17313_test.py reads .github/workflows/test-durations.yml by concrete literal, and python-paths.yml did not cover it -- so a change confined to that workflow computes `python != 'true'`, the required-context shim publishes green, and the guard never runs. Covered rather than recorded as uncovered: MAX_UNCOVERED_READS stays 42 because the read is now genuinely covered, not because it was excused. 4 & 5. TWO REACH FLOORS. hooks-path-override 6531 -> 6536 (measured 6936) and import-hermeticity 601 -> 602 (measured 662). This branch adds four counting files and one module respectively; main had spent the rest of both allowances before the branch existed. That is the fifteenth re-pin of the first one and the sixth in two days -- see #17142, which is about the allowance and not the floor. 206 passed, 1 skipped (reach_declarations + import_hermeticity) 15 passed (code_sync_preserves_install) 16 passed (schema_validator_names + path_http) 13 passed (python_filter_covers_its_guards) Refs #13579, #17166, #17313, #17142 * test(api): pin the duplicate-route guard's blind spot instead of hiding it (#16908, #17325) My guard reads `@router.<verb>` decorators out of the AST, so it cannot see a route mounted with `router.include_router(other)`. Sixteen non-test modules under autobot-backend/api/ compose that way -- agent_terminal.py:218, auth.py:302 and fourteen others. Its "1518 pairs, 1 duplicate" is a FLOOR, not a total, and reporting it as a total would be the exact defect this repository governs: a count the guard cannot establish. So the blind spot is now measured and asserted. If a seventeenth module starts composing that way the guard fails and says the gap grew, rather than the gap growing unobserved. I FOUND THIS BY AUDITING LEFTOVER BRANCHES, and what it turned up is worse than the blind spot. Commit f4e2d6b on the abandoned branch `issue-16908-duplicate-routes` had already rebuilt this guard on REAL router objects -- load_core_routers()/load_optional_routers(), effective_routes() resolving nested include_router() with an inner router's own prefix, and a population-collapse assertion so a broken import cannot pass as "no duplicates". 228 lines, complete, removed from that branch by a later commit ("split guard out of #16918 per review") and never landed. So I rewrote the project's own prior work, worse, because I did not look for it first. That is rule 1, and the remedy is to reuse it -- filed as #17325 with f4e2d6b named as the starting point rather than a third implementation. Not adopted here because I could not run it once: importing real routers pulls llc.scheduler.base, which refuses on Python 3.11-minus and this machine is 3.10. CI is 3.14 and would execute it. Shipping 228 unverified lines is not an improvement on shipping a lesser guard I did run, so the lesser one ships with its limit written down. The pin is 16 and was 77 in my first draft -- that grep counted `*_test.py`. The test computes the number and asserts it rather than trusting the shell, which is how the miscount surfaced within a minute instead of becoming a comment nobody rechecks. Mutation-verified: a planted module containing include_router fails the pin. Refs #16908, #17325
… gate (#17010) (#17344) #17010's first criterion asked for a router-level Depends(get_current_user). That cannot be written here: router dependencies apply to the WebSocket route too, and get_current_user takes a Request, which FastAPI has none of for /{operation_id}/progress -- the socket would break. So the gate stays per-route, and per-route gating does not hold itself: a route added without one is anonymous. Nothing else would catch that. The repo-wide sweep cannot see this router at all -- router_auth_enumerator.REGISTRY reads only core_routers.py while this router is registered as a string tuple in feature_routers.py (#16375, measured and posted there: 99 of 102 string-registered routers invisible, 17 of them ungated by the guard's own detector). This adds the missing check, scoped to the module it can defend. The behavioural half of #17010's fourth criterion is NOT delivered, and the reason is recorded at the place the next person will look for the test. testkit/auth_middleware_stub.py replaces the whole auth_middleware module for backend tests, and its check_admin_permission takes no Request and refuses nothing: an unauthenticated POST to /codebase/index answers 501 from the route body because the gate never ran. A test asserting "not 200" would pass for that reason and read as security coverage. Filed as #17343 with an opt-in real-auth fixture as the recommendation. Verified: 16 tests pass. Mutation -- add `@router.get("/mutation/probe")` with no gate -- turns the new guard red naming `mutation_probe`, and it passes again once reverted. The guard carries a floor of 10 routes so a walk that matches nothing fails instead of passing. Local passes are not CI (#17144). Refs #17010
…t carries (#17317) (#17318) * fix(provision): stop a long ansible line from replacing the failure it carries (#17317) Reported from a live provisioning run: backend : Install filtered backend requirements fatal: [...]: FAILED! => {"changed": false, "cmd": [".../pip3", "install", ...], "msg": "\n:stderr: ER Error: Separator is found, but chunk is longer than limit Two failures stacked and the second hid the first. `pip install` failed, and ansible reported it the way it reports everything: ONE `fatal:` line holding the whole task result as JSON, with pip's entire stderr inside it. That line was past asyncio's default 64 KiB StreamReader limit, so `readline()` raised `ValueError: Separator is found, but chunk is longer than limit`. Nothing caught it, the generator died mid-run, and that ValueError stood in for the pip error it had just swallowed. The operator was shown the reader's failure instead of the provisioning failure, and the real cause was never written down anywhere. THE PART THAT DECIDES THE FIX: `readline()` DESTROYS the line before raising. CPython's implementation clears `self._buffer` on LimitOverrunError and re-raises as a bare ValueError, so catching it recovers nothing however careful the handler is -- verified directly, `read()` afterwards returns 0 bytes. My first attempt did exactly that and its own test caught it. So the reader now uses `readuntil(b"\n")`, which raises LimitOverrunError with the buffer INTACT and `consumed` pointing at how much is readable. The head is kept, the remainder of that line is discarded so the next read starts on a clean boundary, and the yielded line carries a marker so a cut line is never mistaken for a whole one. An over-long line is truncated and reported -- never dropped, never fatal. A line too big to read is still evidence. Also: the spawn now passes `limit=PIPE_LINE_LIMIT` (10 MiB, env-backed and clamped). Raising the constant without passing it to create_subprocess_exec would have left the 64 KiB default in place, which is the version of this fix that looks right and changes nothing. Mutation-verified, each against the real stdlib rather than a mock -- the bug lives in what asyncio raises when its buffer is exceeded, so the buffer has to be exceeded: back to readline() -> 4 FAILED drop limit= from the spawn -> FAILED ..._spawned_with_that_limit skip the discard -> FAILED ..._no_fragment_..._leaks_as_its_own_line That last one is worth its own note. Removing the discard does not crash and does not spin -- it yields the unread tail as an extra blank line between the truncated line and the next real one, quietly feeding the progress parser a line ansible never emitted. Only a shape assertion catches it, and my first pass at these tests did not. WHAT THIS DOES NOT DO: it does not fix the pip failure. That error is still unknown, because it was destroyed before anyone could read it. This change is what makes the next run say what actually went wrong. The reader moved to its own module, services/playbook_output_stream.py. playbook_executor.py is at its grandfathered ceiling and may not grow, and the rule is split rather than raise -- it came out at 1402 against a 1406 ceiling, so the ceiling drops to 1402 in both places that record it. It is also the better seam: this is stream decoding and the executor is orchestration. Closes #17317 Refs #17038 * chore(ratchets): re-pin four counters against the merged tree (#17317, #17142, #16324) Rebased onto 3131039. Three merges landed within the hour (#17314, #17321, #17328) and moved every one of these. REACH FLOORS. hooks-path-override 6536 -> 6538 (measured 6938) and audio-extension-allowlist 5450 -> 5752 (measured 6152). The first is the sixteenth re-pin and 6536 was measured correctly barely an hour ago -- it did not survive three merges. The second had never tripped before today and trips now for the same reason. That is #17142's argument as a data point rather than an argument: a 400-file allowance on a ~6900-file tree is spent faster than a branch can be reviewed, so a floor pinned correctly at measurement time is already stale at merge time. DUPLICATION, MAIN SCOPE. 11723 -> 11707. Sixteen lines of slack, and the gate exits 0 while it sits there, so a fall in duplication is invisible and a later PR can spend it. That is #16324. DUPLICATION, SLM SCOPE: DELIBERATELY UNCHANGED, and this is the part worth reading. A figure of 2918 was reported for it from the guard's run on #17321's merged head. That run predated #17314, which had already lowered the pin from 2945 to 2831. Applying the reported number would have moved the pin from 2831 to 2918 -- RAISING a ratchet, which is the one direction it must never go, and it would have re-licensed 87 lines of duplication while looking like housekeeping. Re-measured here instead, on the actual merged tree: main scope: 11707 duplicated lines, 672 clones, 4288 files -> pin 11707 SLM scope: 2831 duplicated lines, 123 clones, 798 files -> pin 2831, exact Both figures come from running the detector with the workflow's own flags, not from reading a report. The report was accurate about its own tree and wrong about this one, which is the entire hazard of carrying a measurement across a merge. Refs #17317, #17142, #16324 * fix(provision): route the second playbook reader through the same guard (#17317) Review of this PR found the extraction had left a live twin. api/infrastructure.py `_stream_process_output` ran the identical unguarded `readline()` against a pipe spawned without a `limit=`, so the default 64 KiB applied. It is reached from POST /api/execute and runs the same pip-heavy provisioning playbooks -- setup-ai-stack.yml, setup-npu-worker.yml, provision-fleet-roles.yml -- that produce the oversized `fatal:` JSON line this PR exists to survive. Its failure mode was the worse of the two. `readline` clears the buffer and raises a bare ValueError; `_run_playbook`'s broad `except Exception` catches it and reports "Internal server error". So the endpoint answered with strictly less than the original bug report, which at least surfaced the ValueError text. Both halves are needed and neither is sufficient: the shared iterator cannot salvage a line the spawn already capped at 64 KiB, and a raised limit changes nothing while the reader still uses `readline`. Both are asserted, and the reader assertion is written against the BEHAVIOUR (`process.stdout.readline()` absent, `iter_pipe_lines` present) rather than the helper's name, so renaming `_stream_process_output` cannot silently retire the check. Also corrects two stale references to the pre-extraction name `_iter_pipe_lines` left in a comment and a test docstring. * fix(ratchets): pin the hooks reach floor mid-window, not at its bottom (#17317, #17142) Sixteen re-pins of this floor, six in the last two days, and #17142 concluded the growth allowance is too small. The measurement says the allowance is not the cause and raising it would not have helped. The floor has two bounds. verify_floor needs population - floor <= skips + growth (401), so floor >= 6535. completed() needs floor <= what the guard finishes, and skips=1 is this file alone, so that ceiling is population - 1 = 6935. The legal window is 6535..6935, four hundred wide. Every previous re-pin used floor = population - growth, which lands on the very bottom of that window. Slack is then always exactly growth against an allowance of growth + 1, leaving ONE file of headroom no matter what growth is set to -- doubling growth to 800 would re-pin the floor 400 lower and leave the same single file. That is the treadmill, and it is a property of the formula rather than of the number. 6736 is population - 200: 201 files of headroom before the allowance is breached, 199 files of shrink before completed() is. It is also a stricter floor than 6538 rather than a looser one, because the floor asserts how much of the tree the guard actually reached; only the gap check cares about the distance. Main measures 6936, confirmed by two independent branches rather than asserted: #17323 adds 3 counted files and CI read 6939, #17330 adds 2 and read 6938. Counted additions in flight total +22, and #17327 alone (+14) would have breached the previous 6538 four times over.
…tions (#16908) (#17347) #16908 AC4 asks for the guard to be proved by mutation: plant a duplicate and confirm it goes red naming both modules. The tests that existed proved the EXPANSION attributes routes to the right router object -- correct, and a different claim. Nothing proved the guard FIRES. Proved here against collisions() itself, with REG/API pointed at a synthetic checkout, rather than by reimplementing the comparison in the test -- which would only prove the test can find a duplicate. Two directions, and the second is what makes the first mean anything: - a duplicate planted where the AST detector reads (a decorator on the REGISTERED router object) reddens the guard and names BOTH modules; - the same (GET, /api/alpha/dup) mounted through include_router does NOT redden it, because the detector reads decorators on the registered object only (#17325's blind spot). "I planted a duplicate and it went red" is worthless unless the plant was in the reached region, so the miss is asserted as behaviour rather than described in a comment. When #17325 adopts real-router enumeration, that test is the one that must change, and it says so by failing. Verified against the live tree before writing either test, then reverted: planted `@router.get("/admin/orphan-storage")` in api/run_jwt_router.py -- registered, and NOT one of the 16 include_router modules, checked with the guard's own _modules_using_include_router() -- and the guard failed with "GET /api/admin/orphan-storage registered by api.admin_orphan_storage:router, api.run_jwt_router:router". Both sides named. planted `@me_router.post("/login")` in api/auth.py, inserted BEFORE `router.include_router(me_router)` so it is a real runtime duplicate of POST /login (neither router carries a prefix) -- 4 passed. Silent. The new tests were themselves mutation-tested: making collisions() name one side instead of two fails them with "the failure must name BOTH sides"; the detector restored, 6 pass.
…RED case (#17348) (#17350) The guard merged in #17344 matched names and keywords rather than what they were bound to, and its proof that it can go red lived only in the PR body. - _declares_a_gate now reads VALUES. A `dependencies=` list satisfies it only when it contains Depends(get_current_user) or Depends(check_admin_permission), and a parameter satisfies it only when its DEFAULT is one of those -- which also drops the dependence on the parameter being spelled `current_user`. `dependencies=_ADMIN` resolves through the module's own top-level list bindings, because the router spells its admin gate as a shared list rather than repeating the literal five times, and a guard that only understood list literals would have read those five as ungated. - The RED case is committed. A synthetic router carries one route per gating shape plus three that must FAIL: anonymous, dependencies=[Depends(unrelated)], and current_user: str = Query(...). The last two are exactly the holes this issue reported, now asserted rather than described. - The real test and the meta-tests drive one function, _ungated_routes, so the RED case exercises the logic the guard uses instead of a paraphrase of it. - _AUTH_DEPENDENCIES is checked against what the module actually imports, so renaming a dependency breaks the guard loudly rather than narrowing it to a name that can no longer appear -- which would make every route read as gated. _MIN_ROUTES_SEEN stays at 10, re-counted on this base: 10 route decorators, exact. Verified: 24 tests pass. The real-file mutation still works end to end -- append an ungated route to the router, the guard fails naming mutation_probe, revert, green. Local passes are not CI (#17144). Closes #17348 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…validate the action at creation (#17315) (#17323) * refactor(approvals): require a proposer declaration when registering a post-approval action (#17315) Partial, unpushed WIP: register_post_approval_action() now takes a required proposed_by= naming the production module that creates such an approval, plus is_registered_action()/registered_action_proposers() for the creation-time check. The one production call site and the existing tests still pass no proposed_by -- handing over before that half landed. * refactor(approvals): complete the proposer declaration through every call site (#17315) Finishes the half-landed commit before it: the production registration now declares PROPOSED_BY = "api.admin_orphan_storage", and the seven test call sites pass one too. The registry fixture resets _PROPOSERS alongside _REGISTRY, so the fake actions these tests register cannot leak into the guard that checks every registered action names a real production proposer. * feat(approvals): add the missing proposer for orphan-storage deletion (#17315) The #17038 chain -- detect, propose, human approves, execute -- had both ends and no middle. The executor was registered at a mounted router's import and unit-tested; the detector was exposed by the one GET this module offered; and nothing in production ever created an approval naming the action, so an admin could list orphan candidates and could never propose one. The registered handler was unreachable, and every test passed. POST /api/admin/orphan-storage/deletion-requests is that middle. It creates a PENDING approval whose context is the shape orphan_storage_cleanup_action documents and dispatches on, and it does nothing else: - it deletes nothing, and cannot: delete_candidate stays reachable only from the post-approval executor, after a human decides (#17038 rule 1); - admin-gated by the router's own require_role, like the listing; - an unregistered provider is refused with 404 at proposal time, so a reviewer is never asked to decide on something that could not execute; - whether the candidate is still an orphan past its grace period is re-checked at approval, not now -- a record that reappears in between is refused. Tests: the route (non-admin refused, unknown provider refused, the context asserted key by key, nothing deleted) and the chain #17315 AC2 asks for -- propose -> approve -> delete through the real proposer, the real approve() and the real dispatcher, with only the session and the detector faked. Rejecting deletes nothing, which is the same rule's other half. * fix(security): reject an unregistered approval action at creation, not after a human approves (#17315) POST /approval-gates passed body.context through verbatim, so any authenticated caller could create an approval whose context.action named a string no handler was ever registered for. On approve, the dispatcher recorded a durable anomaly for it -- an ApprovalComment plus an approval.post_action_failed audit entry -- which approval_execution's own docstring describes as "a real defect, not a no-op". Two consequences: a user could mint defect records at will, and a reviewer was asked to decide on a proposal that could never execute. Creation is the only point where the request can simply be refused; by approval time the only option left is telling someone who already said yes that nothing happened. The 400 does not echo the caller's value back and does not list the registry -- the caller supplied the value, and the set of live actions is not theirs to enumerate. An approval with no action key stays valid, which is the ordinary gate (compose_tool_handler creates exactly that) and the dispatcher's early return. * test(approvals): guard that every registered post-approval action has a production proposer (#17315) #17315 AC4. The registry shipped holding one action that was registered, unit-tested, wired into a mounted router and impossible to reach, while every existing test passed -- each covered one end of a chain whose middle was missing. This guard closes the shape, not the instance. It reads the tree for production call sites of register_post_approval_action, imports the modules that WIRE each one (importing the handler module alone registers nothing -- register() is called by the router, which is the subtlety that makes this worth a guard rather than a grep), and then checks each registered action against the module it declared as proposer: it must import, must name the action -- literal or imported constant, since a proposer that imports ACTION is as valid as one spelling it out -- and must actually call create_approval. It fails loudly when it finds no registration sites at all, rather than passing with nothing to check: a guard that cannot distinguish "nothing registered" from "did not look" is the defect it exists to catch. Negative control, run before committing: with the new proposer route removed from api/admin_orphan_storage.py -- the exact pre-#17315 state -- the guard fails with "action 'orphan_storage_delete' declares proposer 'api.admin_orphan_storage', but that module never calls create_approval()". Restored, 3 passed. * docs(changelog): add the fragment for the orphan-storage proposer (#17315) * chore(types): regenerate generated API types from OpenAPI schema(s) Auto-regenerated (py3.14) to match the backend and/or SLM backend schema so the required verify-generated-types gate(s) pass. Triggered by auto-fix-generated-types.yml. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ve degraded verifier replies by policy (#17305, #17306) (#17322) * chore: claim worktree for #17305, #17306 * fix(llm,security): wire structured output to every provider and resolve degraded verifier replies by policy (#17305, #17306) #17305 — `structured_output=True` reached exactly one provider. `providers/ollama.py:113` turned it into `format: "json"`; the hosted providers never read the flag and no file in `llm_shared/providers/` emitted `response_format`, `json_schema` or `json_object`. Callers that set it and then parsed prose — the judges, the contradiction detector, the extraction cognifiers — were exposed to drift that native schema mode makes impossible. `llm_shared/structured_output.py` builds each dialect's fragment once and declares what a provider can keep: the OpenAI-compatible family gets `response_format` in the one shared `_build_params`, Anthropic gets `output_config["format"]` merged beside any `effort` tier, Mistral gets the same clamp, vLLM gets `guided_json` threaded into guided decoding. `LLMRequest.json_schema` carries the schema. A provider that declares only `json_object` drops a schema deliberately rather than sending one that would 400, and `BaseProvider.chat_completion` records the applied mode on the request and warns when the flag is a no-op — so a caller can tell "constrained" from "hoping" (#17319 tracks the three providers that still declare NONE). #17306 — the verifier's degraded paths produced security outcomes by numeric coincidence. A parse miss returned 0.5, which with a `>=` compare blocked deploy/mutate/exec/default and passed network mutations at 0.6; raising any `VERIFIER_THRESHOLD_*` moved that class to fail-open silently. An error returned 0.0, so "the verifier passed it" and "the verifier never ran" were the same PASS. `autobot_shared/verifier_degradation.py` makes a degradation a named state instead: parse misses return None, `VerifierObservation` carries the cause, and `resolve_verdict` routes degraded passes through an explicit policy that no threshold participates in. `VERIFIER_ON_UNPARSEABLE` (default block) and `VERIFIER_FAIL_CLOSED` (default open, so a broken verifier still does not brick the agent) are the switches; allowing a degraded response through is SKIP, never PASS, and `VerifierResult.degradation` plus `evidence_label()` keep the two apart at the dispatch seam and in the loop. Two files were split to stay under the 600-line ceiling rather than raising it: `providers/anthropic_request.py` (593→504) and `autobot_shared/verifier_prompt.py`, both re-exported from their old homes so existing importers are untouched. Also fixed in scope: `vllm_base._chat_completion_impl` accessed `msg.role` on dicts and handed a coroutine function to `run_in_executor` with a positional kwargs dict — it raised before reaching vLLM on every call. Closes #17305 Closes #17306 * docs(llm): state the exact line counts behind the anthropic_request split (#17305) * test(llm): real-load the new llm_shared modules in the backend conftest (#17305) * test(guards): re-pin four reach floors this branch's file count moves past (#17305, #17306) Arithmetic, not a regression. Main at 3131039 leaves all four floors sitting at EXACTLY their declared allowance, so any branch that adds a python file trips them -- the #17142 shape the hooks-path-override comment block already records thirteen times, and this is the sharpest instance: zero headroom on four floors at once. Attribution, by counting both trees with the same globs rather than adding to what CI last reported: 6905 sh/py/yml files on origin/main, 6912 here. hooks-path-override 6536 -> 6543 (population 6943, growth 400) audio-extension-allowlist 5450 -> 5757 (population 6157, growth 400) import-hermeticity 602 -> 604 (population 664, growth 60) prompt-injection-detector-strict-mode 2956 -> 2958 (population 3258, growth 300) Each pinned at `population - growth`, the stricter direction for a reach floor: it asserts the sweep must reach MORE, unlike a size ceiling, which may only come down. * fix(llm): declare Ollama's real structured-output capability, and its two exceptions (#17305) Review catch on #17322, and the inverse of the defect this PR exists to prevent. `providers/ollama_provider.OllamaProvider` inherited StructuredOutputMode.NONE, so `note_structured_output` logged "not honoured natively by provider=ollama" and recorded structured_output_mode_applied=none on every structured Ollama call. Both false: its delegate (providers/ollama.py:113) sends `format: "json"`, and the PR body itself says Ollama was the one provider that honoured the flag before this work. A metadata contract lying about what happened is exactly the class of bug the flag was added to remove. Declared JSON_OBJECT, with `applied_structured_output_mode` reporting NONE on the two paths where the format really is dropped rather than over-claiming: - a request carrying metadata["chat_template"] is rendered and POSTed to /api/generate by _chat_completion_impl, a payload with no `format` field (#4525); - ollama.py suppresses format=json when tools will be sent, because Ollama rejects the combination (#7911). Its `_model_supports_tools` predicate is imported rather than re-approximated -- two copies of "will tools be sent" is how the two answers drift apart, and the suppression is conditional on the model, so the report has to be too. Six tests pin the declaration to the payload, including the conditional case (tools on a model that is NOT tool-capable still reports json_object). The payload half was already pinned by ollama_test.py's two format tests, which these point at rather than duplicate.
…marker that bricks pip (#17331, #17332, #17246) (#17338) * fix(deploy): a generated manifest must name paths the node has, not the controller's checkout (#17331, #17246) #17242 moved the generation of the filtered backend manifest onto the controller. It did not move the path that generation writes INTO the file, so provisioning still aborted on every non-manager host -- one task later, inside pip: ERROR: Could not open constraint file: [Errno 2] No such file or directory: '<base_dir>/code_source/constraints/shared.txt' Confirmed from a two-host run, not inferred. pip resolves a nested `-c` against the directory of the file containing it, so the path the rewrite bakes in is the path pip opens on the target -- where code_source does not exist. - #17331 build-filtered-requirements.sh takes the rewrite root as its own argument, separate from the source root. Omitting it keeps both equal, which is correct for services/role_registry.py's post_sync_cmd (reads and installs on the manager). The five delegated ansible generations pass the node's base_dir; the three GPU manifests had no relative includes to rewrite, so that was latent rather than failing. - #17246 six of the seven baselined pip sites now read a staged constraints/shared.txt. The seventh (npu-worker) reads its whole manifest from the checkout and that manifest editable-installs autobot_shared, which nothing else puts in that venv; routing it through the filter script would strip that line and silently drop the dependency. Filed as #17334 with the decision it needs, and it stays as the single remaining baseline entry. - roles/_shared/tasks/stage_shared_manifests.yml stages constraints/shared.txt and the root requirements.txt onto the node, guarded by a per-host fact and tagged `always` for the reason update-all-nodes.yml gives at its own staging task. Each role includes it, so a role carries its own precondition whatever playbook ran it. - A new guard covers the generated-CONTENT case the #17243 guard structurally cannot see: that one reads task payloads, and this path only exists inside a file a shell script writes at run time. Both mutations named in its docstring were executed and both went red. - #17242's false invariant comment ("code_source ... always exists as the git checkout") is corrected -- it is the sentence that hid this half. Verified: 32 ansible guards and 15 rewrite tests pass locally; reach_declarations_test 185 passed. Local passes are not CI (#17144). * fix(deploy): record the new manifest-path guard's declared glob (#17331) repo_tests/glob_declared_reads_15900_test.py keeps a bidirectional record of which guards declare each uncovered glob. A new guard that reads *.yml must appear in it, or the record is wrong in a way a maintainer would trust. * fix(deploy): keep the staging task's comment out of the code_source_dir scan (#17331) repo_tests/ansible_shared_tasks_code_source_dir_default_16750_test.py scans every _shared/tasks/ file for a templated code_source_dir with no | default(...), and it reads comments as well as tasks. The prose describing the defect being fixed quoted the bad pattern verbatim, which the scan cannot distinguish from a real template. * fix(update): stop the venv provenance marker outliving its install, and clear the husks it left (#17332) The #15067 marker is written inside a distribution's dist-info and was not listed in that distribution's RECORD. pip deletes the paths RECORD names and then the directory if nothing is left, so every upgrade of a stamped package left a dist-info holding one file. Python then reports a second distribution for that name at version None and the next pip run that must uninstall it aborts: error: uninstall-no-record-file x Cannot uninstall cachetools None -- no RECORD file was found Twelve packages on one host were already in that state, found in tonight's provisioning run. pip stops at the first, so each failed deploy reveals one. - write_provenance_marker now lists the marker in RECORD, which makes the module's central claim -- "a reinstall erases the marker" -- true instead of assumed. Empty hash and size columns, which is what pip writes for RECORD itself. Idempotent, because the reconciler re-stamps every declared package on every run. - clear_provenance_husks repairs a whole venv in one pass before pip runs, so a host with twelve does not need twelve failed deploys. On #17038: what it removes is tool-authored debris, admitted only when the directory describes no installed distribution AND its sole content is this module's own marker. It cannot reach a real installation, an operator's files, or another tool's, and each removal is logged. - has_tool_provenance refuses a husk. It used to answer "ours" for a distribution that is not installed, which would let a removal be authorised by the debris of an earlier one. - The module docstring's false invariant is corrected at the sentence that states it. Three existing fixtures built a dist-info with mkdir() alone -- shaped exactly like the husk -- so they are now realistic installs via one shared helper. _StubPath grows `parents` modelled on pathlib's sequence rather than a single plausible value. Verified: 48 venv tests and 98 code-sync tests pass; the mutation (drop the RECORD listing) turns three of the new tests red, including the upgrade sequence itself. The new suite carries a contrast test asserting the modelled uninstall DOES produce a husk without the fix, so the passing tests cannot be satisfied by a simulation that never removes anything. pip's behaviour is read from its source, not assumed: req_uninstall.uninstallation_paths yields dist.iter_declared_entries() and raises UninstallMissingRecord when RECORD is absent. Local passes are not CI (#17144). Refs #17332 * test(slm): scope the multipart import stub so it stops leaking into later files (#17332) The sys.modules leak guard blocked the push, and the leak was not in the new file it was reporting against: test_worker_component_resolve_12450.py installed `multipart` and `multipart.multipart` stubs at module import and never removed them, so they outlived it and were whatever the next test file to import happened to inherit. The guard names the file that SEES the leak, which is why this surfaced only once the venv tests ran in the same selection. The stubs are needed while `api.code_sync` is imported and are dead weight afterwards, since the module object is cached. `_load_code_sync()` installs them, imports, and removes them in one try/finally, restoring whatever was there before. The ten call sites now go through it. Verified: 117 tests pass across test_worker_component_resolve_12450.py, test_code_sync_deploy_bugs.py and test_venv_provenance.py. Nothing was added to repo_tests/sys_modules_leak_baseline.txt -- that list only shrinks. Refs #17332 * docs(update): correct the husk count — 13 across two venvs, not 12 in one (#17332) The count was measured in autobot-backend/venv alone and written as "twelve packages on one host". A one-venv measurement reported with host-wide language, which is the shape of error this repo treats as serious: a reader greps that venv, confirms twelve, and stops. Re-measured across every venv under the install tree, read-only: autobot-backend/venv 12 husks autobot-slm-backend/venv 1 husk psycopg2_binary-2.9.12.dist-info the other five venvs 0 The thirteenth matters beyond arithmetic. It is in the SLM's OWN venv -- the component that runs the builtin updater, which is the only path clear_provenance_husks is wired into (#17339). Its live sibling psycopg2_binary-2.9.13.dist-info carries a 46-line RECORD while the husk holds one file, so it is the same upgrade-leak mechanism and not a second defect. Found by autobot-ai-f8, who swept every venv rather than the one the issue named, and verified independently here before changing the text. Refs #17332 * fix(guards): adopt main's reach floors on rebase and record what they leave (#17331, #17332) #17318 landed hooks-path-override 6736 and audio-extension-allowlist 5752 while this branch carried 6800 and 5700. Both of main's values are inside the windows measured here, so this branch takes them rather than re-deriving competing numbers for one measurement -- first to land wins, the others adopt. Re-measured after the rebase to confirm they hold WITH this branch's files, which is the half that adopting a number normally skips: audio : population 6154 completed 5767 floor 5752 gap 402/700 headroom 15 hooks : population 6941 completed 6940 floor 6736 gap 205/401 headroom 204 Both pass. Audio's 15 is thin and is recorded at the site: that declaration's loop skips `repo_tests` outright, so its `completed` ceiling FALLS as guards are added while population rises -- the two bounds close from opposite directions, and only the gap one is ever measured. Not re-pinning it here; one branch widening a floor another just set is how two correct numbers become a conflict. Carried to #17142 instead. Refs #17142
…mmary, and carry a pin (#16816) (#17341) * fix(ci): make the dead-surface report actually emit, reach the job summary, and carry a pin (#16816) #16816's first half landed in e4fc546: the three-way split, the Company OS subtotal and tests for the classifier. Two acceptance criteria were still open, and verifying the landed half found a third problem that made the rest moot. THE REPORT EMITTED NOTHING. report_dead_surface() logged through a module logger, and audit_api_wiring.py configures no logging at all -- it prints everywhere else -- so every line went to a root logger at WARNING and vanished. CI passed --dead-surface, the job printed no section, and nothing failed. The measurement #16816 exists to obtain was being taken and thrown away, which is the issue's own defect shape one level further in. Measured, not inferred: the pre-fix module emits 0 characters for a one-path input; the post-fix module emits 345 and the section assertion holds. Why the classifier tests did not catch it: they test classify_dead_surface(), a pure function, and pass. AC6 asked for a test that the MODE EMITS THE SECTION, which is a different claim, and the one that would have caught this. Six new cases cover it -- the section, the split reaching stdout rather than only the return value, both pin directions, the summary file, and the local case where no summary exists. AC1, the job summary: scripts/dead_surface.py appends the table, the Company OS subtotal and the GUI-unreachable list to GITHUB_STEP_SUMMARY itself when CI sets it. Deliberately not a `... | tee` in the workflow: a pipeline between the auditor and its own exit code is how a failing audit reads as a passing job. AC2, the ratchet: scripts/dead_surface_baseline.txt holds the pinned GUI-gap count and the comparison prints on every run -- over by N, or "lower it to N to keep the ratchet tight", the same shape the duplication gate uses. It is seeded UNMEASURED because measuring it means importing the backend to dump its OpenAPI schema, which is running codebase code outside CI; the first real figure comes from the job, gets triaged, and is written there. AC3 is respected: the comparison never touches the exit code, and says so where a reader will see it. * fix(ci): keep the dead-surface report out of print()'s way — summary file, not stdout (#16816, #16008) Making the report emit turned scripts/dead_surface.py into a library module that calls print(), and #16008's guard failed it by name: these library modules under scripts/ call print(), so the CLI-entry-point property does not describe the exempt set: ['scripts/dead_surface.py'] The guard is right, and its exemption is a PROPERTY it tests rather than a list it is told -- so adding an entry, or giving this module a __main__ block it has no use for, would defeat what #16008 built. Logging is what emitted nothing in the first place. The obvious fix, printing from the auditor (which IS an entry point and prints everywhere else), is not available: audit_api_wiring.py cannot be staged at all. It carries two pre-existing `:8001` findings under scripts/, which HV_SCAN_DIRS does not walk, so they can never be baselined -- #17329's exact catch-22, in a third directory -- and a `main` of 106 body lines against a 65-line threshold, while sitting at exactly its 895-line ceiling. All three are pre-existing and none is this issue's to fix. So the report goes where it needs nobody's cooperation: the GITHUB_STEP_SUMMARY file, which is what #16816 AC1 asks for. The lines are returned too, so printing them to the job log is a one-line change at the call site the moment #17330 makes that file editable. Stated in the workflow comment rather than left as a silent gap. 17 passed, including #16008's guard, which now passes on this branch.
…m for code-consumed verdicts (#17307, #17308) (#17327) * chore: claim worktree for #17307, #17308 * fix(judges,agent-seam): one validated loop and one typed-decision seam for code-consumed verdicts (#17307, #17308) #17307 — a judge parse failure approved the workflow step it was asked to gate. `judges/__init__.py` indexed `overall_score`/`recommendation`/`confidence` out of a single-shot reply, any key or enum drift raised, `make_judgment` turned that into an error judgment, and `step_evaluator._check_judge_errors` turned THAT into `should_proceed: True`. The security judge failing to parse therefore approved the command it was assessing. The retry-and-validate loop `structured_ops.extract()` already had now lives in `llm_shared/validated_llm.py` behind a *completer*, so one loop serves every transport: `llm_service` for the judges, the claim verifier and the autoresearch scorer, a direct local Ollama call for `rlm/evaluator.py`, and whatever backend the decision seam is given. It also sends the schema to the provider as `json_schema` (#17305), so native schema mode constrains the reply before the retry is needed. The gate is now a named policy: `AUTOBOT_JUDGE_FAIL_CLOSED` (default open, as #1464 chose), and the response carries `judge_available: False` plus a `degradation` code, counted through the existing approval and error counters — `approved_judge_unavailable` is a different metric label from `approved`. `_build_evaluation_error_response` and `workflow_step_judge.quick_approval_check` had the same hard-coded approval and now follow the same policy. Two parse-miss defaults of the #17306 shape went with it: `rlm/evaluator._extract_float` returned `0.5` when the SCORE line was missing (compared against `quality_threshold` immediately after), and `scorers._parse_rating` fell back to a regex that would find "7" inside a refusal. Both are gone; an unreadable reply is an error result or the existing INDETERMINATE verdict, never a number nobody produced. #17308 — `llm_shared/decisions.py` is the seam: `decide(state, questions)` with choice, score and boolean primitives, answered in one round trip against one state, validated against a generated schema. `claim_verifier.classify_agreement` and the autoresearch scorer are migrated with their bespoke parsers deleted; the judges go through the same loop with `JUDGMENT_SCHEMA` because their payload carries per-dimension scores the three primitives cannot express. The backend is pluggable and defaults to the local small-model path, so the seam needs no new outbound dependency and none is added. Probabilities are reported as `Calibration.SELF_REPORTED`, not calibrated: nobody here has measured them. `scripts/benchmark_decision_backends.py` plus a labelled sample is the gate that would change that — it reports accuracy, p50/p95 latency and the separation between mean probability on right and wrong answers, and promotes nothing. The exclusion list is a test, not a comment: `decisions_exclusions_test.py` asserts that tier routing, the fast-path classifiers, reranking, injection detection and the pre-action verifier gate do not import the seam. `services/claim_verifier.py` shrank 835 -> 830, so both ratchet copies are lowered. `AUTOBOT_JUDGE_FAIL_CLOSED` is registered in `env_registry_agent_runtime.py` with `ENV_VARS.md` regenerated, as the env-var hook requires — both are hub files #17314 also touches. Closes #17307 Closes #17308 * fix(eval,autoresearch): update the doubles the validated verdict contract replaced (#17307, #17308) The five failures on #17327 that were not the shared CI floor, all one cause: test doubles still speaking the format the schema replaced. - eval/tests (harness, candidate_wiring, hardening): the RLM evaluator stub returned the three-line SCORE:/CRITIQUE:/HINT: reply, so every scored trajectory now failed validation and classified as 'unmeasured' instead of 'unchanged'. The stub speaks JSON. - tests/test_autoresearch_m3.py: the judge double still returned {"rating": 7}, and its two sibling doubles had a truthy MagicMock .error, which the completer correctly reads as a failed call. - pipeline-scripts/hardcoded_values_baseline.txt: five entries went stale because the "system"/"user" role literals left structured_ops.py, scorers.py and claim_verifier.py when those call sites moved onto the shared completer's CategoryDefaults. Pruned with --prune-baseline (removal-only) and re-audited clean. * test(guards): re-pin four reach floors this stack's file count moves past (#17307, #17308) Arithmetic, not a regression, and measured rather than assumed. Main at 3131039 leaves all four floors sitting at EXACTLY their declared allowance, so any branch that adds files trips them -- the #17142 shape the hooks-path-override comment block already records thirteen times. Attribution, by counting both trees with the same globs rather than adding to what CI last reported: `git ls-tree -r --name-only` gives 6905 sh/py/yml files on origin/main and 6919 on this HEAD, and 6150 vs 6164 python files. The +14 is this stack's own new modules and tests. hooks-path-override 6536 -> 6550 (population 6950, growth 400) audio-extension-allowlist 5450 -> 5764 (population 6164, growth 400) import-hermeticity 602 -> 604 (population 664, growth 60) prompt-injection-detector-strict-mode 2956 -> 2960 (population 3260, growth 300) Each pinned at `population - growth`, which is the stricter direction for a reach floor: it asserts the sweep must reach MORE, unlike a size ceiling, which may only come down. * test(guards): give the decision-seam exclusion guard a positive control (#17308) Found by applying tonight's rule to my own branch: would this green test still pass if the thing under test were deleted? It would. Every case in `test_excluded_module_does_not_use_the_decision_seam` asserts an ABSENCE. `path.is_file()` catches a renamed target and `test_the_repo_root_resolves_to_this_checkout` catches a wrong root, but nothing proved `_imports()` can DETECT a seam import. A changed AST node type, a silent exception or a walk that stops early would have made all nine cases pass while reporting "nothing found" for "did not look" -- the exact conflation MEASUREMENT_DISCIPLINE.md opens with. The new case asserts the detector finds the seam import in `services/claim_verifier.py`, a production caller migrated onto the seam in #17308 rather than a fixture that could drift out of the shape it stands for. Mutation-verified rather than asserted: with `_imports` blinded to return an empty set, the suite goes 1 failed / 11 passed, and the one failure is this new control. Before it, the same blinding produced 11 green. * fix(research): update the agreement double the typed-decision contract replaced (#17307, #17308) The red CI found and my own consumer sweep missed. `services/research/orchestrator_test.py` stubs the LLM with an `AGREEMENT:` line, so with `classify_agreement` on the decision seam every source came back UNRELATED, no fact was corroborated, and two promotion tests failed on `update_fact` never being awaited. Why the sweep missed it, because the selector is the lesson: I grepped for the migrated SYMBOLS (`classify_agreement`, `LLMJudgeScorer`, `make_judgment`, ...) across test files. This file never names any of them -- it drives `ResearchOrchestrator`, which calls `ClaimVerifier` two layers down. A grep for the symbol can only find direct namers, which is the "query narrower than its reading" shape MEASUREMENT_DISCIPLINE.md opens with. The instrument that would have found it is a grep for the REPLY FORMAT rather than the caller: `AGREEMENT:`, `SCORE:`, `{"rating": N}`. Re-run that way, the remaining hits are all unrelated (marketplace ratings, skill-health fixtures, a judge prompt template), so this was the last one. Two failures, both real assertions rather than flakes -- exactly what the shape said: a named failing step at 9.3 minutes, not an empty step list at the 60 minute ceiling.
… not a bypass (#17060) (#17352) * fix(security): the task-ownership store fails closed, so an outage is not a bypass (#17060) verify_task_owner returned True on any Redis error. /steer and /answer have no other authorization, so while the store was down ANY signed-in user could steer or answer ANY running agent task -- the control absent at exactly the moment nobody is watching it. register_task_owner had the same shape: it returned True on a write failure, telling its caller ownership was recorded when nothing was stored. Both now deny and log at error level. The admin bypass is checked BEFORE any Redis call, so an operator still reaches a stuck task during an outage -- which is what makes denying non-admins acceptable rather than an outage of its own. Matches #16411/#16387. This is #17060's third criterion only, and the issue does NOT close. The other three are blocked, and on what: - "verify_task_owner never registers the caller; with no recorded owner only an admin passes" cannot ship alone. NOTHING registers owners today -- `git grep -rln register_task_owner` on main returns the module and its own test, nothing else -- so denying unowned tasks would refuse every non-admin steer and answer, which is a functional outage wearing a security fix's clothes. - "Every agent task records its verified creator at creation" has no site to hook: no route mints a task_id (every one in api/agent_terminal.py receives it), and the loop that builds the TaskContext holds no authenticated principal. - The discovery half -- agent_loop/loop.py:1787 publishes APPROVAL_REQUIRED with task_id to the "global" channel, which api/live_events.py:236 admits every authenticated client to -- is a one-line change to a file PR #17327 currently holds. Same-file rule: not taken here. So the outage bypass is closed and the steady-state hole is not. Stating it rather than implying the issue is handled. Verified: 11 tests pass. Mutation -- restore the `return True` in verify_task_owner's except -- turns test_redis_outage_denies_a_non_admin red and nothing else, then green on revert. The admin-during-outage and failed-registration cases are new tests, not rewordings of the old one. Local passes are not CI (#17144). Refs #17060 * fix(security): deny where a real outage actually lands, not where an exception would (#17060) Review found the first version of this fix does not cover the scenario in the issue title, and that its tests passed by exercising a path production never takes. Verified every link before rewriting: 1. task_owner.py `if existing is None: register(); return True` 2. autobot_shared/redis_client.py redis_get: `client = await get_async_redis_client()` `if client: return await client.get(key)` / `return None` 3. get_async_redis_client docstring "or None if Redis is disabled or the circuit breaker is open" -- it does not raise 4. the old tests patched redis_get to RAISE ConnectionError So a real outage returns None, lands on "no owner recorded", registers the caller and GRANTS -- for every task at once. The `except Exception` deny blocks were real but only reachable in a much narrower window: a command failing after a good connection. The tests injected an exception at a layer that, in production, swallows exceptions and returns None. Green suite, surviving vulnerability, on the authorization control itself. The fix distinguishes the two states the wrapper collapses. `_owner_of` acquires the client directly and returns a `_STORE_UNAVAILABLE` sentinel when there is none, so "could not determine owner" can no longer read as "verified absent" -- which was the exact distinction this issue's first criterion rests on. - verify_task_owner denies on `_STORE_UNAVAILABLE` and, separately, on a mid-command failure. Two branches because they are two different facts; both deny. - register_task_owner has the mirror fix: the same None collapse fell through to `return True`, so a failed write reported an established owner -- contradicting its own docstring. Not independently exploitable today (nothing reads that return), but it would mislead #17060's steady-state half the moment something does. - The module docstring's "If Redis is unavailable the function degrades gracefully (logs a warning, allows the call)" was untouched by the first diff and contradicted both the new behaviour and the PR body. Corrected. Tests now mock `get_async_redis_client` returning None -- the production failure shape -- rather than a raise. Added: an admin still admitted during an outage, a mid-command failure denied, and a positive control that an unowned task is still adopted when the store is healthy, without which "denies" could just mean "always denies". The in-memory fixture moved from the `redis_get` wrapper to a fake client, because faking the wrapper is what hid this. Verified: 12 tests pass. Mutation -- collapse `_STORE_UNAVAILABLE` back into the unowned branch, i.e. reintroduce exactly the original defect -- fails `test_a_real_outage_denies_a_non_admin` and nothing else; green on revert. That mutation is the proof the first version could not produce. Local passes are not CI (#17144). Found by autobot-ai-f8, who traced all four links rather than trusting a green suite. Refs #17060
…broadcasting them (#17354) (#17358) * fix(security): scope workflow events to their own channel instead of broadcasting them (#17354) api/live_events.py admits every authenticated client to the `global` channel, under a comment asserting the channel "carries no per-tenant payload of its own". It was not a property: api/workflow.py had seven publishes on it carrying an operator's `user_input`, a workflow's `user_message`, step descriptions and error text. Every signed-in account holding a /ws/live connection received them. The publisher is admin-gated; the audience was not. All seven now publish to `workflow:{workflow_id}`, which `_authorize_resource_channel` gates on the caller holding `view` on that workflow via WorkflowPermissionService.check_permission. The workflow id was already in hand at every site, so this is the channel argument and nothing else. Checked that this does not break the feature it protects, because the obvious version of this fix does exactly that on the neighbouring seam: - WorkflowLiveDashboard.vue:312 subscribes to `global` and filters for workflow_completed. - It forwards the payload with `emit('workflow-update', ...)`. - That emit is NOT BOUND at the only mount site (WorkflowBuilderView.vue:575-584 binds @refresh, @refresh-agents and @select-workflow, and nothing else), so the payload is discarded today. - Nothing in autobot-frontend/src subscribes to a `workflow:` channel at all. So the move is observable-behaviour-neutral, and finishing that dashboard feature means subscribing to `workflow:{id}` -- the correct channel for it either way. Recorded here rather than discovered later. The comment is now a requirement on publishers rather than an assertion about the channel, and repo_tests/global_channel_carries_no_tenant_payload_17354_test.py enforces it: a comment cannot hold an invariant that any call site can break. The guard's limits are stated, not implied. It reads literal dict payloads, so six pre-existing sites carrying `task_id`/`workflow_id` are baselined (shrink-only, with a staleness test) rather than swept in with the disclosure that prompted this. Eleven more pass an opaque payload -- api/agent.py:197 is a helper that hardcodes "global", so none of its callers can scope itself -- and those are COUNTED under a shrink-only ratchet, so the blind spot cannot grow quietly even though it cannot be inspected. Verified: 4 guard tests pass; 62 event-bus and workflow-notification tests pass; 49 live-events authz and guard meta-tests pass. Mutation -- move one publish back to "global", which is literally the original defect on the production path -- fails test_no_new_global_publish_carries_a_tenant_key naming `api/workflow.py:553 ['step_id', 'user_input', 'workflow_id']`, and passes again on revert. Local passes are not CI (#17144). Refs #17354 * fix(security): make the live-event admin bypass read the payload auth actually builds (#17359, #17354) Scoping the workflow events to `workflow:{id}` in the previous commit made a dormant defect load-bearing, so it ships here rather than after: the admin bypass on every channel authorizer read `user_payload.get("roles", [])`, and authentication does not produce a `roles` list. `auth_middleware.authenticate_websocket` builds `{"username": ..., "role": ...}` -- singular, a string -- and nothing normalises between them. The bypass was inert on every real connection. Left alone, the previous commit would have been a silent feature kill: every route in api/workflow.py is admin-gated, nothing creates a WorkflowPermission row on workflow creation, so non-admins hold no row AND admins would not get the bypass. The seven events would have reached nobody -- a correct-looking security fix that switches the feature off. That is the trap 63 found on the agent_loop seam, arriving from the other direction: there the seam carried no identity, here the identity was under a different key than the reader expected. - One `_is_admin` helper replacing four copies of the rule, using autobot_shared.auth.permissions.is_admin_role -- the predicate the rest of the codebase uses. It admits `superadmin`, which the literal `"admin" in roles` refused even where the key existed (#12786, #14944), and is case-insensitive. - A `roles` list is still honoured, so any caller that does supply one keeps working. Why the suite did not catch it: api/live_events_authz_review_test.py constructs its own payloads carrying `roles`, a shape production never produces. Green tests over a branch production cannot reach. Four tests added against the real shape, including a positive control -- whose first draft asserted a non-admin is denied `agent:u1` while that user IS u1, so the self-scope rule correctly returned True. The control caught my test, which is what a control is for; it now names a case the rule genuinely refuses. Also in this commit: three comment line-number citations in the new guard replaced with symbol names (`_publish_event_safe`, `_try_publish`), per #15877 -- a line number fails silently INTO whatever later occupies the line rather than failing loudly. Verified: 16 authz tests pass, 13 across the guard, the citation checker and the event-bus fanout suite. Mutation -- restore the inert `"admin" in roles` check -- fails exactly the two new production-shape tests and nothing else, then green on revert. Local passes are not CI (#17144). Refs #17359 Refs #17354
…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>
…C4 only) (#17346) * refactor(optimization): one lazy-import primitive, and the accelerate fork closed (#13049) C4 ONLY, AND THE ISSUE SAYS SO. #13049's Ordering section reads: "Land them after the #13030 P0 defects, so the enum work is done against a module whose behaviour is verifiable. C4 is independent and can go any time." All eight #13030 children are still open, including the P0 defects #13031/#13032/#13033, so C1 (architecture-family taxonomies), C2 (dtype vocabularies) and C3 (compression validation) are not touched here. Those blockers are now recorded as native blocked_by edges on #13049 rather than left as prose in an Ordering paragraph nobody queries. C4: _import_accelerate() was defined twice in llm_shared/optimization/ -- meta_eviction.py and model_inspector.py -- same package, same optional dependency. The issue calls them "character-for-character in intent"; they had in fact already drifted on the part that matters. meta_eviction caught (ImportError, RuntimeError) and re-raised BOTH as ImportError; model_inspector caught ImportError alone and let RuntimeError through. That divergence decided the design rather than being papered over. torch_loader had already ruled on exactly this question (#12714): a custom error_message applies only when the dependency is genuinely ABSENT, while a RuntimeError -- installed but failed to initialise -- is re-raised unchanged, because rewording it as "Install with: pip install X" sends a reader to fix something that is already there. The rule is inherited, not invented. It changes one behaviour, narrowly: meta_eviction's RuntimeError-to-ImportError conversion is gone. Its public entry point evict_layer_to_meta documents "ImportError: If quantizer is provided but accelerate is not installed" -- the not-installed case, which still raises ImportError. The only call site, _evict_quantized_layer, is unguarded, and nothing upstream catches it, so no handler changes meaning. NOT a third copy of the machinery. Rather than clone torch_loader's caching and double-checked locking into an accelerate_loader beside it, the primitive moved to llm_shared/lazy_import.py and both loaders are thin wrappers over it. lazy_torch keeps its exact signature and behaviour; its 18 call sites are untouched, and nothing outside the module ever referenced the _torch/_torch_error globals it used to own (checked before moving them). Call-site impact, which the symbol rename made load-bearing: 9 existing tests patched "...meta_eviction._import_accelerate" / "...model_inspector. _import_accelerate" by name. Retargeted to lazy_accelerate in the same module namespaces, which is where the imported symbol lives. Grepping the behaviour rather than the symbol is rule 7 for exactly this reason. lazy_import_test.py pins the asymmetry that had no test while the rule lived in torch_loader -- and which the two forks had already drifted apart on: an ImportError takes the call site's wording, a RuntimeError is re-raised unchanged and ignores error_message. Both driven through the real import path rather than by seeding the cache, so they also prove RuntimeError is among the exceptions caught. Also pinned: the failure is cached rather than retried, and eight threads racing first use all receive one module object -- the race #12714 found in 8 of its 9 copies. Refs #13049, #12714, #13030 * fix(optimization): put the accelerate loader where the harness can see it (#13049) The first attempt added llm_shared/lazy_import.py and llm_shared/accelerate_loader.py at the package root and made torch_loader delegate to the shared primitive. The pre-push suite refused it, and the reason is worth recording rather than routing around. A module at the llm_shared ROOT is invisible to the test harness until it is named in autobot-backend/conftest.py. That package's __init__.py eagerly imports .adapters (reaching autobot_shared and the provider stack), so conftest installs a stub whose __path__ is [] and real-loads only the submodules tests need -- torch_loader among them, added by #12714 when it created that module. Two more entries were the obvious fix. conftest.py cannot take them. It sits at EXACTLY its recorded 1505-line ceiling on origin/main (measured: git show origin/main:autobot-backend/conftest.py | wc -l = 1505, baseline 1505 in both python_file_size_known_large.py and python_file_size_ratchet_baseline.py). A grandfathered file may not grow, and raising a ceiling is not allowed -- so this is a second zero-headroom ceiling on main, alongside the audio-extension-allowlist reach floor, and it blocks ANY new llm_shared root module for everyone, not just this change. WHAT CHANGED INSTEAD. llm_shared.optimization is given a REAL __path__ by that same conftest, and both callers of the loader live there. So the loader is llm_shared/optimization/accelerate_loader.py, imported as `from .accelerate_loader import lazy_accelerate`, and needs no registration at all. conftest.py and torch_loader.py are now byte-identical to origin/main -- verified with `git diff origin/main` on both, empty. That is not free, and the cost is stated rather than hidden: the locking-and-caching machinery now exists twice, here and in torch_loader. Extracting it into one primitive requires a conftest entry, so it is blocked on that ceiling and filed separately. The alternative -- hiding a generic LazyModule class inside a module named torch_loader purely to dodge a size limit -- would be a workaround shaped by the constraint rather than by the design, so it was not taken. The consolidation #13049 C4 asks for is delivered either way: two _import_accelerate() copies become one lazy_accelerate(), and the raise policy they had drifted apart on is inherited from torch_loader's recorded ruling rather than re-decided -- error_message applies to a genuine ImportError, a RuntimeError is re-raised unchanged. Verified on this tree: accelerate_loader_test + meta_eviction_test + model_inspector_test . 56 passed llm_shared/ (excluding pricing/) ................. 683 passed, 128 skipped llm_shared/pricing/sync_cache_scheduler_test.py fails to collect here and is NOT caused by this change: "llc.scheduler.base requires asyncio.Task.cancelling() (Python 3.11+)", identical on the untouched origin/main checkout. This machine runs 3.10; CI runs 3.14. Refs #13049, #12714 * chore(ratchets): re-pin the two floors main left at zero headroom (#13049, #17142) Rebased onto origin/main 3f0d4b3. Two declarations were red, both because main sits at EXACTLY its allowance and this branch adds counting files. hooks-path-override 6536 -> 6800 population 6938 (6907 glob hits for *.sh/*.py/*.yml/*.yaml, plus 31 extensionless files whose shebang says shell), completed 6937 (`read` counts opened files, minus the one EXEMPT). At 6536: slack 402 against growth 400 + skips 1 = 401. One over. Window [6537, 6937]. prompt-injection-detector-strict-mode 2956 -> 3100 population 3257 non-test .py under autobot-backend/ (origin/main: 3256, slack exactly 300 = the whole growth allowance, skips defaults to 0). This branch adds accelerate_loader.py, so 301. Window [2957, 3257]; no completed() call on this declaration, so verify_floor is the only bound. audio-extension-allowlist needed nothing: already pinned to 5700 on this branch, population 6152 against window [5452, 5766]. NEITHER IS PINNED TO THE MINIMUM, and that is the point. hooks-path-override has been re-pinned SIXTEEN times, and its own comment block records why each time: every re-pin took the bottom of the window, so the next file main added was another red, and the branch paying for it was never the branch that caused it. 6800 and 3100 are mid-window -- slack 138 and 157 against allowances of 401 and 300 -- which buys ~260 and ~143 files of headroom instead of zero. The reasoning is autobot-ai-58's, from #17338. 6800 is deliberately 58's value rather than a fourth number for one measurement: if #17338 lands first this line is identical and there is nothing to resolve. autobot-ai-f8 proposed 6736 for the post-#17318 tree, also inside the window; 6800 leaves more headroom. Rule agreed between the three sessions: first to land wins, the others adopt rather than re-derive. 3100 is new -- no peer has a re-pin of that declaration in flight. Verified with the declaring module explicitly in the session: pytest autobot-backend/security/prompt_injection_detector_strict_mode_test.py \ repo_tests/reach_declarations_test.py -> 192 passed That invocation is not incidental. The declaration's own docstring warns it reaches the sweep only when something else has already imported its module: running repo_tests/ alone drops the parameter count from 36 to 35 and reports a clean pass over a floor it never checked. 5.8%, and sixteen re-pins is the evidence that the allowance is what is wrong. Refs #13049, #17142, #17338 * chore(ratchets): adopt main's floors, re-pin the one it left one file short (#13049, #17142) Rebased onto origin/main e8c7bf9. Main moved three floors in the 40 minutes since the previous rebase; all three are ADOPTED, and one of them is red here. audio-extension-allowlist 5752 -> 5757 on main adopted, green import-hermeticity 602 -> 604 on main adopted, green hooks-path-override 6736 unchanged adopted, green prompt-injection-... 2956 -> 2958 on main adopted, RED -> 3100 Measured on this tree: audio pop 6164 completed 5778 pin 5757 slack 407/700 margin 21 hooks pop 6950 completed 6949 pin 6736 slack 214/401 import-herm pop 664 pin 604 slack 60/60 ZERO headroom prompt-inj pop 3259 pin 2958 slack 301/300 ONE OVER prompt-injection-detector-strict-mode is re-pinned 2958 -> 3100, mid-window of [2959, 3259]. Main pinned 2958 from its own population of 3258 -- slack exactly 300, the whole allowance, zero headroom. This branch adds ONE non-test module under autobot-backend/ and that is enough. THE PATTERN IS NOW THE FINDING, not any individual pin. This is the third time tonight a floor pinned at `population - growth` has gone red on the next branch to touch its roots: hooks-path-override, then this declaration at 2956, then this declaration again at 2958. Each time main pins from its own population, each time the next branch adds a file, and each time the branch that pays for the re-pin is not the branch that caused it -- which is what sixteen comment blocks in hooks_path_override_15961_test.py already say, one per re-pin. 3100 buys ~159 files of headroom so the fourth branch does not repeat it. The alternative -- adopting 2958 to honour first-to-land -- would be adopting a value that does not pass here, which the rule was never meant to require. import-hermeticity is adopted at main's 604 and sits at slack 60 against an allowance of 60: passing, zero headroom, and red for whoever next adds a module under api/ or autobot_shared/. Not re-pinned here because this branch does not add one; #17342 is the branch that does and carries 633. Refs #13049, #17142 * chore(ratchets): keep 3100 over main's 2960, and stop putting measurements in prose (#13049, #17142) Rebased onto origin/main e575e3c. KEEPING 3100 RATHER THAN ADOPTING, which inverts this branch's usual rule and is agreed rather than unilateral. #17327 merged carrying prompt-injection-detector-strict-mode at 2960 -- `population - growth`, so main sits at slack exactly 300 of a 300 allowance and the next branch adding one non-test backend module turns it red. This is that branch. autobot-ai-63 held back an identical 3100 so it would resolve here rather than race, and autobot-ai-f8 asked for it to be kept. First-to-land-wins presumes the landed value is VALID. 2960 is not, for any tree with one more file than main's, which is what "zero headroom by construction" means. Measured on this tree (in this message, not in the file -- see below): prompt-inj population 3261 pin 3100 slack 161/300 hooks population 6960 pin 6736 slack 224/401 audio population 6173 completed 5786 pin 5764 margin 22 import-herm population 664 pin 604 slack 60/60 zero headroom, untouched here hooks-path-override adopts main's 6736 unchanged: unlike the prompt-injection pin it is not at the bottom of its window, so this branch's files do not exhaust it. NO MEASURED NUMBERS IN EITHER COMMENT NOW, and that is the durable fix rather than a third cleanup. Both notes previously cited a population and a slack figure; both were stale one rebase later -- the prompt-injection block still said "main re-pinned this to 2958 ... this branch 3259" after main reached 2960 and the tree reached 3261, and the hooks note claimed slack ~204 against an actual 224. That is the same mechanism that produced four orphaned narratives in these files tonight: a rebase resolves the VALUE and nothing resolves the SENTENCE, because prose lands as a pure addition in a different commit. Deleting each one as it is found is a treadmill. Prose that cites no measurement cannot go stale, so both notes now state the DECISION and the METHOD and send the reader to re-derive. The arithmetic lives here, in a commit message, which cannot drift from the tree it describes. The prompt-injection declaration is also rebuilt on main's current comment rather than carrying this branch's copy of an older one, so main's own account of 2955 -> 2956 -> 2958 -> 2960 is intact and unedited above the note. Refs #13049, #17142, #17356
…e measured floor (#17324) (#17366) * fix(ci): make the frontend coverage gate enforceable and pin it to the measured floor (#17324) The four numbers were nested under a `global` key. That is not a threshold key: vitest treats an unrecognised key under `thresholds` as a GLOB pattern for per-file thresholds, so "global" matched no file and the gate enforced nothing. CI has been passing Unit & Integration Tests on ~50% coverage against a declared 70% ever since -- a check that cannot fail, which reads in CI output exactly like a check that passed. Shape confirmed against the CURRENT upstream `interface Thresholds` rather than the locally installed copy, because this machine has vitest 4.1.9 while package.json declares ^5.0.0: the accepted keys are 100, perFile, autoUpdate, statements, functions, branches and lines, and the type carries an explicit `[glob: string]` index signature for per-file patterns. Both versions agree, so the corrected shape is valid for each. FLOORS, AND WHERE THEY COME FROM. Originally taken from CI's own coverage table on merged main at f72de7e: statements 49.91, branches 34.98, functions 38.45, lines 51.01. Thirteen merges landed before this could be committed, so they were RE-MEASURED rather than trusted: 50.07 / 35.11 / 38.54 / 51.15 on the new base. Coverage moved by hundredths, every pin still sits below the measurement, and the tightest margin is functions at 0.54. Pinned 49 / 34 / 38 / 50 -- each the greatest integer at least 0.4 points below the measurement, because repeated runs of the SAME tree jitter by roughly +/-0.2 points and a pin at the measurement would red a PR that changed nothing. These are FLOORS, not targets. 70% is the target and the comment says so. The pin only ratchets UP: a PR that raises coverage raises the floor in the same PR, and it is never lowered to clear a red -- which is how MAX_DUP_LINES and the reach floors accumulated their slack. src/test/coverage-thresholds.test.ts stops the silent shape from returning: it reads the config as text and fails when any top-level threshold key is not one vitest reads. A coverage run cannot catch this -- an inert gate looks exactly like a satisfied one. This could not be committed until #17330 landed. autobot-frontend/vitest.config.ts carried a pre-existing `:3000` in a directory the repo-wide scan did not walk, so it could be neither edited through the hook nor absorbed by the baseline. Running that same hook against this staged change now reports "No new violations found!". Verified: guard 2/2; full suite with the corrected thresholds in place, 298 files, 3410 passed, 1 skipped, exit 0. Local is Python-independent here but the node toolchain is below the declared vitest floor (4.1.9 vs ^5.0.0), so CI remains the authority on the numbers. * fix(test): make the coverage-threshold guard read the parsed block, not its text (#17324) The guard's second test regexed the thresholds block's RAW text, so it matched a metric that had been commented out exactly as happily as one vitest enforces. Commenting out `// statements: 49,` left the literal `statements: 49` in the file: the first test dropped it (it parses top-level keys and strips comments, so no stray glob key appeared) and the second still found it. Both green, metric unenforced -- the precise evasion these tests exist to catch. A guard that greps text cannot tell "configured" from "mentioned". Both tests now read one parsed structure, `topLevelEntries()`: comment-stripped, depth-0 only, key -> value. A commented-out metric has no entry and fails "no floor pinned". A per-file glob's nested `statements: 95` is at depth > 0 and can no longer satisfy a top-level metric. Three adjacent holes in the same class, closed while here: * quoted keys are now captured whole, so a glob key ('src/**') is reported as a non-threshold key instead of being skipped for not being an identifier -- it was the key SHAPE that made the old `global:` block inert; * /* */ comments are stripped from the source, so the evasion cannot simply switch comment syntax; * the value must MATCH a number rather than parse as one, and must exceed zero. `Number('')` is 0, so a bare `statements:` would otherwise have read as a pinned floor -- and a floor of 0 is enforced by vitest and passed by every run, which is an inert gate wearing a number. Mutation reasoning is STATIC, by construction: proving it by running vitest locally would require pushing a mutated config, and a local run is not CI's run. CI is the authority on the numbers. `ci.yml:605` still claims a 70% threshold when the enforced floors are 49/34/38/50. That fix is NOT here: touching ci.yml pulls 32 workflow guard tests into the pre-push selection, which then exceeds its budget and leaves the push unverified. Filed separately rather than trading a verified push for a comment. Found by review of PR #17366, not by me.
…sources (#14881, #14631) (#17342) * refactor(enums): one definition per enum, and a guard that reads the tree (#14881) GROUP 1 -- the API schema hand-copied four enums it already imported from. api/schemas_analytics.py redeclared CheckSeverity, CheckCategory, ReviewSeverity and ReviewCategory with members identical to the code_intelligence originals, while api/analytics_precommit.py imported BUILTIN_CHECKS from that same module. The only real difference was the `str` mixin the schema copies carried for Pydantic. The domain classes carry it now, so one class serves the engine and the wire format, and the four copies are gone. The `CheckSeverity(x.value)` conversions at the seam still hold -- they became idempotent rather than breaking. No cycle: code_intelligence does not import api.schemas_analytics, and both modules pull only stdlib plus autobot_shared. schemas_analytics 3575 -> 3544 lines, re-pinned in both size baselines since a shrink has to be locked in (#14498). GROUP 2 -- one copy was laziness, five were not, and the issue's prediction about them was inverted. api/marketplace_422_test.py was the lazy one, and its enforcement was worse than absent. TestEnumParity was titled "Verify our local enums exactly match the real marketplace enums" and asserted this file's copy against a THIRD hardcoded literal set a few lines below it -- it never imported api/marketplace.py, so no change to the real enum could ever fail it. The real enums are imported now (tests/api/test_marketplace.py already did, so the import is proven in this environment, not assumed), `_make_app()` validates against the same class the real endpoint does, and the parity class is deleted rather than repaired: with the import, parity is structural. The other five are harness scaffolding -- enums injected into sys.modules stubs so a route module imports without its dependency graph, and FastAPI rejects anything but a real str-enum as a Query type, so a Mock cannot serve. Telling those tests to "just import it" would defeat the isolation they exist for, so they keep their copies and declare them. What the census found instead of the predicted drift: the copies carried members production never had. BatchJobType stubs declared ai_task and report. Production has neither, and has file_conversion, report_generation, backup and custom, which the stubs omitted. Two files. _ErrorCategory stubs declared CLIENT_ERROR. Production's ErrorCategory has 18 members and that is not one. Two files. No test referenced any invented member, which is why nothing failed and why only a census found them. All four stubs now mirror production. repo_tests/enum_hand_copy_guard_test.py enforces it, derived from the tree rather than a list, and asymmetric on purpose: a SUBSET is fine (a stub needs only what its test uses -- _SSOProviderType declares 5 of 9), an EXTRA member fails (a test exercising it asserts against a value the system cannot produce), and an exact copy fails unless declared scaffolding. _DISTINCT_CONCEPTS is the declared escape for a genuine name collision, empty today because the sweep found none. Measured: 6,150 files, 436 production enum names, 14 test-local, 0 invented, 0 undeclared exact copies, 0 stale exemptions, 0 unreadable. The guard caught one of my own changes while I wrote it: aligning BatchJobType to production made it an exact copy, which is only legitimate because it is the same stub BatchJobStatus is. Declared, not exempted by widening the rule. Closes #14881 * refactor(mcp): derive the bridge lists from disk instead of hand-maintaining them (#14631) Registering manual_mcp as the twelfth bridge (#14586) meant editing three separate hand-maintained lists of "every governed bridge", and the fourth caught the omission only by accident: test_discover_bridges_known_names happened to assert == against a literal set rather than a subset, so a shard failure rather than a design decision is what surfaced it. The filesystem scan is the SSOT because it is the only one of the four that cannot silently disagree with reality -- it reads the files on disk rather than a name someone remembered to type in a second place. LIST 4 IS GONE. test_discover_bridges_known_names no longer carries twelve literal names; it compares discover_bridges() against {bridge_name(p) for p in bridge_files()}, with a non-empty assertion first so an empty scan cannot make the comparison vacuous. A thirteenth bridge now needs no edit there, and a bridge that stops being discovered still fails. LIST 3 IS GUARDED. floor_membership_gaps() in the required lint check reports a governed bridge with no PER_BRIDGE_DISCOVERY_FLOOR entry, and a floor entry naming no bridge. This closes a real hole rather than a theoretical one: bridge_discovery_gaps() iterates PER_BRIDGE_DISCOVERY_FLOOR.items(), so a bridge absent from that dict was never checked by it, and the aggregate DISCOVERY_FLOOR could not catch the omission either because a new bridge only ever RAISES the sum. It runs before bridge_discovery_gaps(), which can only speak about bridges the dict already names. LIST 2 IS GUARDED, and measured as fully removable. repo_tests/mcp_bridge_list_consistency_14631_test.py pins the membership of both _BRIDGE_MODULE_REGISTRY and PER_BRIDGE_DISCOVERY_FLOOR against the scan, in both directions. It imports bridge_files() rather than re-globbing -- re-deriving the scan inside the test would make it a fifth copy of the thing it checks -- and reads the two lists by AST, since neither api/mcp_registry.py's dependency graph nor the lint script's sys.path manipulation is needed to read a literal. The investigate item is answered, and more strongly than #14631 expected. It assumed _BRIDGE_MODULE_REGISTRY carries endpoint URLs and feature tags the glob cannot supply, so only a partial derivation was possible. Measured: all 12 bridges declare MANIFEST = MCPBridgeManifest(...) carrying name, endpoint AND features, and both endpoint and features agree with the registry's tuples 12/12. It holds no information that is not already on the bridge it describes. Filed as #17340 rather than done here: it feeds discover_bridges()'s fallback, so it changes a discovery path rather than a literal, and wants its own review pass. Verified on this tree: scan finds 12 bridges; registry 12, floor 12, all three sets identical. The exactness rule is pinned on five synthetic cases including the #14586 shape -- a list that is a strict SUBSET of disk, which a subset assertion would pass and this one fails. Closes #14631 * fix(enums): move the shared vocabularies to autobot_shared, not into the API (#14881) CI caught a real defect in the first version of this PR, and the fix is the design argument rather than a patch over it. WHAT BROKE. startup-import-smoke went red: PydanticSchemaGenerationError: Unable to generate pydantic-core schema for <MagicMock id=...> Pointing api/schemas_analytics.py at code_intelligence.precommit_analyzer and code_intelligence.code_review_engine made every Pydantic model annotated with CheckSeverity/CheckCategory/ReviewSeverity/ReviewCategory depend on that package being real at import time. autobot-backend/conftest.py stubs the whole code_intelligence package -- its __init__ carries imports the smoke environment cannot satisfy -- so the enums resolved to MagicMock and the schemas could not be built. I checked for an import CYCLE before the first version and found none; I did not check whether the package is importable in every harness, which is the question that actually decided it. WHAT IT MEANS. A wire-format vocabulary the API cannot import without dragging in an analysis engine is not owned by that engine. These four are shared between the engines and the API, so they belong to neither. They now live in autobot_shared/code_review_enums.py -- the same move #11290 made collapsing SecretScope/VisibilityLevel/ScopeLevel onto one shared ScopeLevel. Both engines re-export their pair, so every existing `from code_intelligence.precommit_analyzer import CheckSeverity` keeps working: what moved is the definition, not the name. The `str` mixin stays, for FastAPI. The duplication #14881 exists to close is still closed -- one definition per enum, four copies gone -- and the definition now sits where both consumers can actually reach it. Sizes, all shrinking and all locked in per #14498 (both baselines): api/schemas_analytics.py 3544 -> 3543 code_intelligence/precommit_analyzer.py 731 -> 720 code_intelligence/code_review_engine.py 1235 -> 1219 `from enum import Enum` removed from both engines; it became unused. Verified on this tree: precommit_analyzer_test + analytics_code_review_test + marketplace_422_test 73 passed tests/test_startup_imports.py -- the exact job that failed: this branch 7 failed, 414 passed origin/main 7 failed, 414 passed (identical) PydanticSchemaGenerationError / MagicMock occurrences: 0 The 7 are the pre-existing Python-3.10 platform guard ("llc.scheduler.base requires asyncio.Task.cancelling()"); this machine runs 3.10, CI runs 3.14, so the delta against baseline is what carries the information here, not the count. Refs #14881, #11290 * fix(guards,ratchets): route the new guard through the one enumeration, re-pin two floors (#14881, #14631) Three failures, diagnosed from the run log rather than inferred. The log only became readable once the WHOLE run finished -- a job's log is not served while any shard of its run is still going, which is #17351. shard 10 one_git_enumeration_15926_test::test_the_direct_invocation_count_only_shrinks 34 direct `git ls-files` invocations in repo_tests/, MAX says 33 shard 12 reach_declarations_test[hooks-path-override] floor 6536 sits 403 below population 6939, allowance 401 shard 12 reach_declarations_test[import-hermeticity] floor 602 sits 61 below population 663, allowance 60 THE FIRST ONE IS MINE, and it is the one worth recording. enum_hand_copy_guard shelled out to `git ls-files` for itself. MAX_DIRECT_INVOCATIONS only ever shrinks, so that is a regression even though the guard worked -- a new fork of the repo's enumeration helper, added in a PR whose subject is hand-copied definitions. Routed through tools.lint._scan_helpers.tracked_paths, which also gets the exclusion right: `exclude` becomes a git `:(exclude)` pathspec so git does the matching, rather than a second matcher in Python that can disagree with the first (#15510). subprocess and scrubbed_git_env are no longer imported. THE OTHER TWO ARE MAIN'S, and this branch pays for them: hooks-path-override 6536 -> 6800. Population 6939, completed 6938, window [6537, 6938]. Main had spent 401 of its 401 allowance before this branch existed. Same 6800 as autobot-ai-58's #17338 and this session's #13049 branch -- one measurement should not produce three numbers, and if any two land the line is identical. import-hermeticity 602 -> 633. A FOURTH floor at zero headroom on main that nobody had named: 662 non-test modules against floor 602 is slack 60, exactly the allowance (skips=0). This branch adds autobot_shared/code_review_enums.py -- the shared home the four hand-copied enums moved to -- so 663, one over. 633 is mid-window of [603, 663]; the declaration's own comment records this population growing ~52 modules per 30 days, so the minimum 603 would be red again within three weeks of ordinary work. Measured through the declaration's own discover (repo_tests._import_hermeticity_scope.entries), not a re-derived glob. NEITHER FLOOR IS PINNED TO ITS MINIMUM. hooks-path-override has been re-pinned sixteen times and its own comment block records why every time: pinned at the bottom of the window, the next file main adds is another red, and the branch that pays is never the branch that caused it. #17142 is the real fix -- a 400-file growth allowance on a ~6900-file tree is 5.8%. Verified on this tree, rebased onto origin/main 3f0d4b3: one_git_enumeration + enum_hand_copy_guard + mcp_bridge_list_consistency 50 passed prompt_injection_detector_strict_mode_test + reach_declarations_test 192 passed The second invocation is not incidental: reach_declarations_test only sees the prompt-injection declaration when something else in the session has imported its module, so running repo_tests/ alone drops the sweep from 36 parameters to 35 and reports a clean pass over a floor it never checked. Refs #14881, #14631, #17142, #17351 * chore(ratchets): record that this branch adopted main's floors, not its own (#14881, #14631) Empty commit, and deliberately so: the change is already in the rebase conflict resolutions, but the commits carrying them still SAY 6800 and 5700 in their messages because that is what they proposed when written. Rewriting four commit messages to fix a two-line outcome is worse than stating it once. Rebased onto origin/main 19006b3. Both floors this branch re-pinned had landed on main first, so both were adopted: hooks-path-override proposed 6800 -> adopted main's 6736 (#17318) audio-extension-allowlist proposed 5700 -> adopted main's 5752 (#17317) First to land wins, so one measurement does not produce four numbers. Re-derived on this tree rather than taken on trust: audio pop 6155 completed 5767 pin 5752 slack 403/700 margin 15 hooks pop 6941 completed 6940 pin 6736 slack 205/401 margin 204 prompt-inj pop 3256 pin 2956 slack 300/300 ZERO headroom import-herm pop 663 pin 633 slack 30/60 import-hermeticity stays at this branch's own 633: main is still 602 and this is the branch that adds a module under the counted roots (autobot_shared/code_review_enums.py), so nothing to adopt. TWO NUMBERS WORTH CARRYING TO #17142 rather than acting on here: audio's margin is 15, not the 402 its pin implies. 5752 is population - growth, the GAP bound, while the bound that fires on this guard is completed() -- its loop continues past the canonical file and all of repo_tests/ before reading anything, so completed sits ~388 below population and falls every time anyone adds a guard. Five sessions did tonight. This branch measures 15; the #13049 branch measures 16. Each new repo_tests guard spends one. prompt-injection-detector-strict-mode is at slack 300 against an allowance of 300 -- passing only because this branch adds no non-test module under autobot-backend/. It reds for whoever next does. Verified on this tree: prompt_injection_detector_strict_mode + reach_declarations + one_git_enumeration + enum_hand_copy_guard + mcp_bridge_list_consistency ............................. 242 passed Refs #14881, #14631, #17142, #17317, #17318 * fix(guards): migrate the enum-census floor to declare(), re-measured not carried (#14881, #15928) This is the shard-8 failure on this branch, and the second instance of it this session: FAILED repo_tests/reach_floor_migration_test.py::test_no_new_hand_rolled_floor_remains_unmigrated hand-rolled reach-sized floor(s) not migrated to repo_tests._reach.declare: repo_tests/enum_hand_copy_guard_test.py: ['_MIN_SOURCE_FILES'] #15928's rule: a floor >=100 that binds a tree sweep goes through declare(), because a number chosen by feel decays silently. Both of this session's new guards had one, and both had already decayed: enum_hand_copy_guard_test 3000 against a population of 6155 -- 51% below redaction_concept_census_test 1500 against a population of 3393 -- 56% below (migrated on the #16688 branch) Either would have passed over a sweep that lost HALF the codebase and still reported clean. Both were written by feel, hours apart, inside guards about measurement discipline, in PRs about hand-copied constants, by the same author who had spent the evening flagging this class of error in other sessions' work. One instance is carelessness; two is a property of hand-rolled floors, which is #15928's argument. Evidence posted there rather than left in a postmortem. declare("enum-hand-copy-census", floor=5755, growth=400, what="tracked python files") Re-measured from scratch with a throwaway script -- 6,155 tracked .py files excluding .worktrees/ -- rather than carried across, which is what #15928 requires. growth=400 matches the sibling audio-extension-allowlist declaration over the same population. REACH.completed(parsed) is wired in, which the bare constant had no way to express: the floor now binds what the sweep actually PARSED, not what it listed. Without it the guard could enumerate 6,155 files, fail to parse 6,000 of them, and report the same clean as a tree with no hand-copied enums in it -- the exact distinction the guard exists to make. discover() takes a root and returns [] on EmptyEnumeration rather than letting it escape: reach_declarations_test drives every declaration against an empty repository and demands ReachFloorError specifically, so letting EmptyEnumeration through means the declaration fails loudly while saying nothing about whether its floor binds. Diagnosed by autobot-ai-f8 from the shard-8 log, retrieved via the jobs API after `gh run view --log-failed` refused mid-run. Verified on this tree: reach_floor_migration + enum_hand_copy_guard + reach_declarations ...................................... 206 passed Refs #14881, #14631, #15928 * chore(ratchets): re-pin two floors mid-window, including one I pinned at the minimum an hour ago (#14881, #14631) Rebased onto origin/main e8c7bf9. Main's floors adopted where they hold; two did not. import-hermeticity 604 -> 633. Main pinned 604 for its own population of 664 -- slack exactly 60, the whole allowance (skips=0), zero headroom. This branch adds one module under the counted roots (autobot_shared/code_review_enums.py) so 665, slack 61. Second time tonight this declaration has gone red one file over a minimum pin. 633 is mid-window of [605, 665]. enum-hand-copy-census 5755 -> 5965, AND THIS ONE IS MINE. I created that declaration earlier tonight, migrating a hand-rolled floor under MINIMUM OF THE WINDOW. It went red on the very next rebase: main gained ten files, population reached 6165, slack 410 against an allowance of 400. Ten files. That is all the tolerance a minimum pin has, because it has none by construction. The timing is the point rather than an embarrassment to bury. I had just filed a comment on #17142 arguing that `population - growth` is not a floor but a snapshot with zero tolerance, and that every re-pin taking the bottom of the window is why hooks-path-override has sixteen of them. The declaration I wrote an hour earlier did exactly that. Knowing the rule did not prevent it; reach_declarations_test did, which is the entire case for the rule being mechanical rather than a review item. Re-measured on this tree rather than adjusted: 6,165 tracked .py files excluding .worktrees/. Window [5765, 6165]; 5965 is mid-window and buys ~200 files of headroom instead of ten. Measured after both re-pins: audio pop 6165 completed 5777 pin 5757 slack 408/700 margin 20 hooks pop 6951 pin 6736 slack 215/401 prompt-inj pop 3258 pin 2958 slack 300/300 ZERO headroom import-herm pop 665 pin 633 slack 32/60 enum-census pop 6165 pin 5965 slack 200/400 prompt-injection-detector-strict-mode is adopted at main's 2958 and left alone: it is green here because this branch adds no non-test module under autobot-backend/. #17346 is the branch that does, and carries 3100. Verified on this tree: reach_declarations + reach_floor_migration + enum_hand_copy_guard .................................... 206 passed Refs #14881, #14631, #17142, #15928 * chore(ratchets): re-pin import-hermeticity, and remove every measurement from floor prose (#14881, #14631) Rebased onto origin/main e575e3c. Main's floors adopted where they hold; one did not, and all the prose changed shape. import-hermeticity 604 -> 635. Main's 604 is `population - growth`, so main sits at exactly its allowance and one added module under the counted roots is enough to red it. This branch adds autobot_shared/code_review_enums.py, the shared home the four hand-copied enums moved to. Second time this declaration has gone red that way tonight. 635 is mid-window. audio, hooks, prompt-injection and enum-hand-copy-census all pass on main's values or this branch's existing ones and are left alone. Measured on this tree (here, not in the files -- see below): audio population 6174 completed 5785 pin 5764 margin 21 hooks population 6961 pin 6736 slack 225/401 prompt-inj population 3260 pin 2960 slack 300/300 import-herm population 665 pin 635 slack 30/60 enum-census population 6174 pin 5965 slack 209/400 NO MEASURED NUMBERS REMAIN IN ANY FLOOR COMMENT THIS PR TOUCHES, and that is the change worth reading. Verified: git diff origin/main...HEAD | grep '^+' \ | grep -cE 'slack [0-9]+|population [0-9]+|completed [0-9]+' -> 0 Tonight these files accumulated several stale narratives -- a comment claiming "5700 leaves 64" beside a shipped 5757, a note claiming "slack ~204" against an actual 225, a block describing main's pin as 2958 after main reached 2960. None was wrong when written. Every one went stale at a rebase, because a rebase resolves the VALUE and nothing resolves the SENTENCE: prose lands as a pure addition in a different commit, and no mechanism connects the two. Deleting each as it is found is a treadmill -- this branch did it three times. Prose that cites no measurement cannot go stale. So every floor comment here now states the DECISION and the METHOD and tells the reader to re-derive; the arithmetic lives in commit messages, which cannot drift from the tree they describe because they are not in it. That includes this branch's own self-report on enum-hand-copy-census, which was itself citing a population that had moved twice since it was written -- a comment about the hazard of stale measurements, stale. Verified on this tree: reach_declarations + reach_floor_migration + enum_hand_copy_guard + prompt_injection_detector_strict_mode .................... 213 passed Refs #14881, #14631, #17142, #17356
…oning (#17339) (#17360) * chore: claim worktree for #17339 * fix(deploy): make the venv husk repair reachable from ansible provisioning (#17339) #17332 stopped the provenance marker creating dist-info husks and repaired the existing ones -- but only on the path that runs `api/venv_reconcile.py`, which the builtin updater calls. A host reached ONLY by ansible never executes it, so its husks survive and every pip step that must upgrade a husked package aborts with `uninstall-no-record-file`. pip stops at the first, so a host with several needs several failed deploys to find them all. Option 1 of the three the issue lists: run the real module on the node, with the venv's own interpreter. - `venv_provenance.main()` -- a CLI over `clear_venv_husks()`, which is the whole-venv loop `_run_pip_install` already performed inline and now shares, so ansible and the reconciler have one loop as well as one classification. - `roles/_shared/tasks/clear_venv_husks.yml` -- stages the module with `copy:` (read on the controller, written to the node: the #17242/#17331 invariant that nothing syncs `code_source` to a fleet node) and runs it with `{{ husk_venv_dir }}/bin/python`. No `shell:` step, no `find -name`: the classification that decides a deletion is not re-expressed in YAML. - Included by `roles/backend` and `roles/slm_manager`, each BEFORE its first pip task. That target set is exact rather than convenient: `api/code_sync._COMPONENT_PIP_PATHS` names `autobot-backend` and `autobot-slm-backend` and nothing else writes a marker, so no other role can have a husk to clear. The module's one non-stdlib import (`autobot_shared.time_utils.utc_timestamp`) is now lazy, inside the marker-writing function that uses it. A staged copy has to import on stdlib alone, and only the write half needs it. The bootstrap question 58 raised and nobody had answered -- can the repair run when the venv it repairs is the broken one? -- is answered by test rather than by argument: a husk is a dist-info directory, not a broken interpreter, so a husked venv repairs itself. It matters because `autobot-slm-backend/venv` runs the builtin updater, the only other path to this repair. 15 new tests: the stdlib-only import in a subprocess, the CLI over a husked venv, a real installation and another tool's leftovers both untouched, an absent venv and a healthy one as success, every site-packages reached, the self-repair property as a subprocess, and four on the ansible wiring including that the include precedes the first pip task in both roles. Not covered, stated rather than implied: AC2's "the pip step that previously aborted completes" is verified here at the level of its precondition -- the husk is gone -- not by running pip against a real venv, which needs a package index CI does not have. The reported 13 husks live on the owner's host under /opt/autobot, where removal is the owner's call (#17038), so the end-to-end run is theirs rather than something this PR can assert. * fix(deploy): add the third marker-capable venv, and make both guards able to fail (#17339) Review found two defects, both the vacuous-guard shape, and the second-order one is the more interesting. **1. The target set was wrong, and the test could not say so.** `autobot-ai-stack` reaches `reconcile_component` through `code_sync._WORKER_COMPONENT_PIP` and is NOT in `EXPLICIT_LIST_COMPONENTS`, so its venv gets a marker too -- and `roles/ai-stack` has its own pip steps against it. A host code-synced before #17332 and later provisioned by ansible has exactly this PR's gap in a third venv. The include now runs before its first pip step. The test that missed it was parametrised over the two roles I had claimed, so it verified my claim against itself. `COMPONENT_ROLES` is now checked against a set DERIVED from source with `ast` -- both dicts that reach `reconcile_component`, minus the explicit-list components -- so a new marker-capable component fails this file until someone maps it. A positive control asserts the derivation finds `autobot-ai-stack` and excludes `autobot-npu-worker`. **2. The single-source-of-truth guard could not catch its own worked example.** It matched `-name '*dist-info` without the dot, while real directories are `pkg-1.0.dist-info`, so `ansible.builtin.command: find {{ dir }} -name '*.dist-info' -delete` -- the exact reimplementation it exists to stop -- passed every assertion. The task file being clean made it report a true negative, which is how a matcher that cannot match looks correct. Markers now carry the dot, and `test_the_detector_catches_its_own_worked_example` feeds the guard that very line. The scan reads the YAML round-trip rather than raw text, so the file can keep explaining the rule it forbids in prose -- the #16750 shape, where a scan that reads comments makes the comment the defect. Mutation-verified, each one applied and confirmed to apply before its result was read: a find -name '*.dist-info' STEP added -> 1 failed ai-stack's include removed -> 2 failed a new marker-capable component in code_sync -> 1 failed clean tree -> 20 passed The first attempt at the second mutation edited an indent that does not exist in that file and changed nothing; it 'passed', which proved nothing. Checking that a mutation mutates is part of the check. 30 passed across both husk suites, 227 passed / 1 xfailed on repo_tests -k ansible. * fix(provision): run the husk repair before the first pip step, and make the guard see it (#17339) Pre-merge review of #17360 found the repair ordered AFTER the first pip invocation in `slm_manager` -- the one venv this task's own comment calls critical, because it runs the builtin updater and so a husk there blocks its own repair. The ordering guard did not catch it, and the reason is the more important half. `test_the_repair_precedes_the_first_pip_step` located the first pip step as: first_pip_at = rendered.index("ansible.builtin.pip:") `slm_manager` reaches pip first through `ansible.builtin.command:` with `{{ slm_backend_dir }}/venv/bin/pip install -e ...`, which that index cannot see. The assertion was true of what it measured and false of what its name claims -- it answered an adjacent question and passed. Changes: - `slm_manager/tasks/main.yml`: the husk-clear include now precedes the editable `autobot_shared` install. `ai-stack` and `backend` already had no pip step before their includes. - The same edit had orphaned the `#16641` comment from the task it explains (`virtualenv_command: python3.14 -m venv`); it is reattached. - The guard matches every shape pip is spelled in these roles, and raises rather than returning a sentinel when nothing matches, so "the matcher went blind" cannot read as "pip comes later". - A positive control (`test_the_matcher_sees_a_command_shaped_pip_invocation`) pins the widening, so narrowing it back makes the suite redder rather than greener -- which is how the gap survived the first pass. - `clear_venv_husks.yml`: the header claimed only two venvs can carry a marker and only two roles need the include. This PR wires three; `_WORKER_COMPONENT_PIP` makes `autobot-ai-stack` marker-capable. The list is derived by `test_the_role_map_covers_every_marker_capable_component`, so the comment no longer restates it. Mutation proof, run on the two YAML revisions rather than asserted: BEFORE repair@15468 old matcher: first pip@15633 PASS new: @14628 FAIL AFTER repair@14818 old matcher: first pip@15633 PASS new: @15058 PASS The old matcher is green on the defect and on the fix alike. Refs #17339 * fix(tests): take the pip-ordering offsets from the comment-stripped task content (#17339) The widened matcher from the previous commit matched PROSE. `roles/backend/tasks/main.yml:348` describes its own deploy window as "apt/pip install, code sync, alembic migrations" -- 24k characters before that role's first real pip task -- so `test_the_repair_precedes_the_first_pip_step` failed `backend` for a sentence while the role was correctly ordered. Both offsets now come from `_task_content()`, the YAML round-trip this file already uses for the same reason (#16750): a scan that reads comments makes the comment the defect. Mutation proof, both directions, on the real file rather than a fixture: defect restored (include moved back after the editable install) FAIL: "roles/slm_manager/tasks/main.yml reaches pip first via '/bin/pip ' at offset 8029, repair at 8363" fix in place 21 passed The mutation was confirmed applied before the verdict was read -- the include moved from line 373 to 388, past the editable install at 370. Refs #17339 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ons test (#16261) (#17379) Line 19640 of api_endpoint_migrations_test.py asserted that inspect.getsource(frontend_config.get_frontend_config) contains "hosts". host-list disclosure, #15745), so the check goes stale once it merges. The whole file is skip-marked (#5359/#15173), so nothing caught it. Delete the one assertion; the method's other nine checks (fallback structure, config sections) still exercise real, currently-true behavior. Editing the file put it in no-local-schemas' (#6056) scan, which flagged four pre-existing test-fixture BaseModel subclasses that predate the hook. Exempt *_test.py/test_*.py from that endpoint-schema-placement hook -- a model a test defines inline is a fixture, not a production schema -- the same exemption made independently on #16298. Lower both file-size ratchet ceilings (scripts/python_file_size_known_large.py, repo_tests/python_file_size_ratchet_baseline.py) to the file's new actual line count, 31039, so the deleted line cannot be re-spent elsewhere. Not done here, per the issue's own scope: other getsource substring checks in this parked file may also be stale (e.g. "default_config" is not currently present in get_frontend_config's source either) -- that drift belongs to #15173, not this fix.
…own hang (#17355) (#17367) _write_loop called input_queue.get_nowait() and `continue`d on queue.Empty with no sleep, no timeout and no blocking get anywhere in its body. An idle session re-entered the loop immediately and burned 100% of a core -- one per LIVE terminal, with no leak required. Measured on the pre-fix loop against a counting queue: 845,748 calls in 0.4 seconds. After: under 50. Its docstring said "Wait for input without timeout". That is what the loop was meant to do and the exact opposite of what get_nowait does, which is why the defect survived review -- a reader checking "does this block properly?" found a comment saying it did. THE OBVIOUS FIX IS A TRAP, and the tests here exist mostly to catch it. A plain blocking get() looks correct against cleanup(), which does put a None sentinel. It parks forever on three paths that stop this loop without one: * the sentinel's put sits inside a try/except that swallows to logger.debug, AFTER running = False is already set; * an abandoned session never calls cleanup() at all; * the loop's second condition, `master_fd is not None`, is signalled by nothing, and a parked thread re-checks neither term. So the wait is BOUNDED: get(timeout=_WRITE_WAIT_TIMEOUT_S), from TimingConstants.MICRO_DELAY rather than a literal. It is not a latency budget -- get() returns the moment input is queued -- it bounds only how often an idle loop wakes and how long after running=False the thread notices. cleanup()'s two `pass` statements claimed the threads exit on the running flag. That was true ONLY because the writer spun: the spin was load-bearing for a documented shutdown contract nobody had decided to rely on. The comments now say what actually happens -- both threads exit within their own bounded wait, and cleanup() does not join them. No join is added deliberately: cleanup() is reached from async request paths and a blocking join there would stall an event loop to save milliseconds of thread lifetime. Three tests, each mutation-tested: * idle loop does not spin -- fails at 845,748 calls with get_nowait restored; * loop stops on running=False with NO sentinel -- fails with a blocking get(); * loop stops on master_fd going None -- fails with a blocking get(). Restored, 3 pass. The fake queue models a real blocking get() (never returns when nothing is queued) so the blocking-fix mutation runs against the committed fixture rather than being described in a comment. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
… (#17378) * guard(size): give .sh the size ratchet .py has had since #5060 (#17353) Shell had no size gate of any kind -- no checker, no baseline, no ratchet test. Python has had one since #5060 and a ratchet since #14236, so a .py file cannot grow past a recorded ceiling. Meanwhile scripts/lib/hardcoded-value-rules.sh -- the rule set BOTH the repo-wide scan and the pre-commit hook read -- went 787 to 842 lines in one PR with nothing to say about it. The gate that can say no shapes the change; the one that does not exist does not. Measured first: 216 tracked .sh files, 10 over the 600-line ceiling. Not the 509 the Python gate had to grandfather, which is why the ceilings sit inline rather than in a sibling data module. SCOPE IS A DELIBERATE DIVERGENCE, and it is the decision in this change. check_python_file_size.py excludes autobot-infrastructure/. Inheriting that here would drop 6 of the 10 oversized files -- including four of the five that motivated the issue -- and the gate would ship green having checked almost nothing, which is the vacuous-gate failure MEASUREMENT_DISCIPLINE.md names. Python can afford that exclusion because little Python lives there. Shell cannot: infrastructure is WHERE SHELL LIVES, and deployment scripts are the code with the least type checking and the most operational consequence. So this gate excludes .worktrees/ only, and a test pins that divergence so a later "align the two gates" change fails and reads why. Same three-way ratchet as the Python gate, including the branch that looks wrong: a file UNDER its ceiling fails until the ceiling is lowered, because an unlowered ceiling re-licenses the lines just cut. Ceilings are kept in two copies that a test holds equal (#14498) -- the gap between a lowered hook and an unlowered mirror is exactly those lines, and is spendable. An unreadable file is a violation, not a skip: None from count_lines means "never measured", which is a different claim from "within the limit", and only the second is what exit 0 may report. NOT extracted into a shared engine with the Python gate, deliberately, though ~90% of the logic is common. repo_tests/python_file_size_ratchet_test.py monkeypatches hook.KNOWN_LARGE, hook.repo_root and hook.tracked_python_files as MODULE ATTRIBUTES; an engine taking a config object would stop honouring those patches and the tests would keep passing while asserting nothing. Consolidating means rewriting that 590-line test's patching strategy, which is a bigger and riskier change than this issue asks for. Filed as #17377. Wired into code-quality.yml beside the Python audit, so an entry whose file shrank, moved or was deleted is caught repo-wide rather than waiting for somebody to stage it. * fix(guard): the shell ratchet's CI audit did not run for 4 of the 10 files it pins (#17353) Review found the gate shipping green while not running in the case it exists for -- the vacuous-gate failure this PR is supposed to prevent, in the PR that adds it. `--audit-ceilings` lives in the `code-quality` job, which is gated on `needs.changes.outputs.backend`. That filter carries a generic `'**/*.py'` and had NO generic `.sh` entry -- only three specific shell prefixes. Measured against the ten grandfathered paths, four fell outside every pattern: install.sh (1265) scripts/pr-preflight.sh (818) autobot-slm-backend/ansible/deploy.sh (718) autobot-infrastructure/autobot-slm-backend/scripts/bootstrap-slm.sh (652) So a PR editing ONLY one of those skipped `code-quality` entirely and the audit never ran. The well-behaved case hid it: lowering a ceiling touches `repo_tests/**`, which IS in the filter, so a correct change self-triggers the job. The misbehaving case -- shrink the file, forget the ceiling -- is the one that skipped. This PR's own run hid it too, for the same reason. Fixed by adding `'**/*.sh'` beside `'**/*.py'`, in the canonical filter and in code-quality.yml's `on.push.paths` copy, which `check_workflow_path_filters.py` polices for exactly this drift. Re-measured: 0 of 10 uncovered. Two more from the same review: * the exclude-mirror test asserted CONTAINMENT, not equality, so widening the yaml to `^(\.worktrees/|autobot-infrastructure/)` would still have passed -- '.worktrees/' is a substring of it. That widening is the "align the two gates" mistake the neighbouring test exists to refuse, and it would have silently stopped pre-commit gating infrastructure shell files. Now set equality, with the alternation parsed rather than string-matched. * the tree walk `continue`d past an unreadable file, contradicting this module's own docstring that an unmeasured file is a violation and not a skip. It now reports one, and still does not count it toward `reached`, so it cannot prop up the floor check without having been ruled on. Plus a boundary test pinning that an unlisted file at exactly MAX_LINES passes and 601 fails: a gate that trips AT the limit and one that trips past it read identically in review.
…census (#16688) (#17335) * refactor(redaction): name the redaction boundary and pin the concept census (#16688) The issue asks which of four parallel redaction implementations is canonical. Measuring first changed the question twice. There are seven, not four. A census rule run over all 3,392 tracked production sources finds three more modules that independently decide what a secret looks like and that no issue had counted: chat_workflow/cot_events.py (_SENSITIVE_KEY_FRAGMENTS, 13 nouns), llc/services/portability.py (_SECRET_LIKE_KEYS, 13) and services/config_revision_service.py (_SECRET_SUBSTRINGS, 5). A fifth, autobot_shared/security/redaction.py, also declares itself the one canonical redactor (#12242). And one of the four is not an implementation at all. security/chat_message_safety.py declares no detector; it composes pii_pipeline and the injection detector. It is the shape the other seven should converge on, so the guard must not count it. There is therefore no single canonical redactor and there should not be one. What was missing is the boundary: one module per SHAPE of the problem. docs/developer/REDACTION_BOUNDARY.md states it, with a which-one-do-I-call table, and each of the modules points at it -- except portability.py, which sits on its grandfathered 936-line ceiling and would be pushed over by the note. Two behaviour changes, both strictly widening: - secret_redaction's JWT pattern required "eyJ" on the payload as well as the header, making it narrower than a2a/pii_pipeline.py's. A payload of {} encodes to e30 and was matched there and missed here -- in the module llm_shared.credential_redaction delegates log redaction to. Anchored on the header alone, as pii_pipeline already was. - credential_redaction.SENSITIVE_KEYS no longer hand-copies the credential nouns; it derives from secret_redaction.CREDENTIAL_SUFFIXES. Deliberately a UNION: that vocabulary carries no authorization terms, so a straight replacement would have stopped redacting Authorization, auth_header and x_auth. A redaction merge that narrows is a silent under-redaction, which is why both directions are now tested. repo_tests/redaction_concept_census_test.py pins the seven. It counts implementations of a concept rather than duplicated text, because jscpd finds zero clone lines across these modules -- they are forks, and a fork shares no 8-line run, so MAX_DUP_LINES neither rises when an eighth lands nor falls when one is collapsed (#17312). The rule is proven on a planted fork that shares no text with any real module, and on the two shapes that must NOT count: pure composition, and a vocabulary with no redaction entry point. The two autobot_shared redactors disagree on 7 of 10 sampled field names -- suffix matching vs substring, each a recorded ruling in its own file. Merging them would reverse one, so this documents the split and leaves it open. Refs #16688, #17312 * fix(redaction): correct the JWT divergence example to a token both rules size (#16688) The pre-push suite caught the previous commit's fixture asserting the wrong thing. `{}` base64url-encodes to `e30`, three characters -- below the `{8,}` this scanner requires for a segment and below the `{10,}` a2a/pii_pipeline.py requires, so NEITHER rule matched it. The test passed its "something matched" sibling only because the high-entropy rule claimed the span; the precise assertion (`pattern == "jwt"`) is what surfaced it, which is the argument for writing it that way. The divergence itself is real; the example was not. A payload starts "eyJ" only when its JSON begins exactly '{"'. A serializer emitting a space after the brace gives "eyAi" and one padding before it gives "IHsi" -- both long enough, both matched by pii_pipeline, both missed by this scanner before the widening. Fixture replaced with such a token, decoding to { "sub": "1234567890", "name": "A" }, verified matched by the new rule and unmatched by the old one. The source comment carried the same wrong example and is corrected with it. Refs #16688 * chore(ratchets): re-pin audio-extension-allowlist, red at zero headroom on main (#16688, #16724) origin/main sits at EXACTLY zero headroom on this floor: population 6150, floor 5450, and 6150 - 5450 = 700 = skips 300 + growth 400. Any branch adding one tracked .py file turns it red. This branch adds one test file, measures 6151, slack 701 -- one over. The floor is stale, not the branch. Measured on this rebased tree, two methods, not carried from another session: git ls-files "*.py" = 6151 (origin/main 6150) git ls-tree -r HEAD = 6151 (origin/main 6150) Re-pinned to 5700, NOT the 5751 that population - growth gives. The binding constraint is completed(), not verify_floor(): the guard's loop continues past the canonical file and everything under repo_tests/ before reading anything, so completed = 6151 - 386 - 1 = 5764. A 5751 floor leaves 13 files of margin against a number that FALLS as repo_tests/ grows -- and this branch grows it. 5700 leaves 64, comparable to the 47 the 5450 pin carried against its measured completed of 5497, which held from #16702 until now. verify_floor: 6151 - 5700 = 451 <= 300 + 400 = 700 ok completed: 5764 >= 5700, margin 64 ok skips and growth unchanged: neither is short, the floor was stale. #16724 already tracks re-measuring this from CI's own log rather than from a tree walk. Checked for the duplicated `floor=` keyword a conflict resolution produced elsewhere today: one occurrence. Also measured, not changed: hooks-path-override is green here but at ZERO headroom (population 6937 incl. 31 extensionless shell scripts, floor 6536, slack 401, allowance 401); origin/main has exactly 1. The next tracked .sh/.py/.yml/.yaml file anywhere turns it red. Left alone deliberately -- it passes, it is a hub file four sessions are touching tonight, and #17142 already tracks it. Refs #16688, #16724, #17142 * fix(docs): follow the four THREAT_MODEL anchors this branch moved (#16688) python-suite shards 5, 6, 8 and 10 were red on threat_model_anchors_resolve_test. The cause is in this branch's own diff, not in base: the boundary pointer added to autobot_shared/security/redaction.py's module docstring inserted five lines above every definition in the file, and the citations in docs/developer/THREAT_MODEL.md still pointed at the old numbers. Base agrees with itself, checked before assuming: git show origin/main:autobot_shared/security/redaction.py \ | grep -n "def redact_provider_error" -> 140 git show origin/main:docs/developer/THREAT_MODEL.md \ | grep -n redact_provider_error -> 116: ... (:140) FOUR anchors moved, not one. The failing test names only the anchor its shard reached first, so fixing that one would have left three and produced another red run. Every citation in the document was re-resolved with the test's own rule (AST definition lookup, not a line regex) rather than only the reported case: redact_text :97 -> :102 redact_mapping :109 -> :114 redact_cloud_identifiers :126 -> :131 redact_provider_error :140 -> :145 The two loader.py anchors that a definition-only check flags are NOT broken: import_module and spec_from_file_location are stdlib CALL sites, and the test has an explicit branch for anchors with no definition in the file. A first pass listed them as failures; that was the probe being wrong, not the doc. The other seven modules this branch adds a docstring pointer to are unaffected -- the sweep covered all 38 anchors, not just redaction.py, and they are either unanchored or anchored above the insertion point. Verified: repo_tests/threat_model_anchors_resolve_test.py -- 41 passed. THREAT_MODEL.md stays at 140 lines, which is its own ceiling (test_document_exists_and_is_short_enough_to_read_every_review asserts <= 140); this change edits line numbers in place and adds none. Root-caused by autobot-ai-f8 while sweeping, including the base-vs-branch attribution check; the widening from one anchor to four is this commit. Refs #16688 * fix(guards,ratchets): route the census through the one enumeration, pre-empt a zero-headroom floor (#16688) Both changes are pre-emptive: this branch was GREEN when they were made. It was green against a base that has since moved, which is the whole point. THE CENSUS GUARD WAS A SECOND FORK OF THE ENUMERATION. redaction_concept_census_test shelled out to `git ls-files` for itself, exactly as enum_hand_copy_guard did on the #14881 branch. one_git_enumeration_15926_test::test_the_direct_invocation_count_only_shrinks caught that one in CI; rebasing this branch onto origin/main 3f0d4b3 reproduces it here -- 34 invocations against a MAX of 33 -- because main reached 33 after this branch's last run. Found by predicting it and checking, not by a red. A census guard forking the repo's enumeration, in a PR whose subject is forks, is the joke writing itself. Routed through tools.lint._scan_helpers.tracked_paths, whose `exclude` becomes a git `:(exclude)` pathspec so git does the matching -- rather than the second Python-side matcher this had, which is how #15510 fired an exclusion against absolute paths and matched every file. subprocess and scrubbed_git_env are no longer imported. HOOKS-PATH-OVERRIDE 6536 -> 6800 WHILE PASSING. Measured on this rebased tree: population 6937 (glob hits for *.sh/*.py/*.yml/*.yaml plus 31 extensionless files whose shebang says shell), completed 6936, slack 401 against growth 400 + skips 1 = exactly 401. That is not "passing", it is "passing until the next file anyone lands", and four sessions moved main in the hour this branch sat in review. The cost of pre-empting is one line on a hub file three other PRs already change to the same value; the cost of not is an hour of CI and a round trip on a predictable red. 6800 is mid-window of [6536, 6936], not the minimum -- fifteen previous re-pins took the bottom, which is why there were fifteen. It is deliberately the same value as autobot-ai-58's #17338 and this session's #17342 and #17346: one measurement should not produce four numbers, and identical lines cannot conflict. First to land wins, the rest adopt on rebase. The other three floors were measured here and left alone, because this branch does not move their populations: audio-extension-allowlist 6151/5764 against the 5700 pinned earlier on this branch (margin 64), prompt-injection-detector-strict-mode 3256 (no new backend non-test module), import-hermeticity 662 (no new module under api/ or autobot_shared/). Two of those sit at exactly zero headroom on main and will need re-pinning by whichever branch next adds to their roots -- see #17142. Verified on this tree: one_git_enumeration + redaction_concept_census + threat_model_anchors + prompt_injection_detector_strict_mode + reach_declarations ... 274 passed reach_declarations_test is run with the prompt-injection module in the same session on purpose: its declaration reaches the sweep only when something else has imported it, so repo_tests/ alone silently checks 35 of 36 floors. Refs #16688, #15926, #17142 * chore(ratchets): record that this branch adopted main's floors, not its own (#16688) Empty commit: the change is in the rebase conflict resolutions, but the commits carrying them still SAY 6800 and 5700 in their messages because that is what they proposed when written. Stating it once beats rewriting four messages. Rebased onto origin/main 19006b3. Both floors adopted, first-to-land wins: hooks-path-override proposed 6800 -> adopted main's 6736 (#17318) audio-extension-allowlist proposed 5700 -> adopted main's 5752 (#17317) Re-derived on this tree rather than taken on trust: audio pop 6153 completed 5766 pin 5752 slack 401/700 margin 14 hooks pop 6939 completed 6938 pin 6736 slack 203/401 margin 202 prompt-inj pop 3256 pin 2956 slack 300/300 ZERO headroom import-herm pop 662 pin 602 slack 60/60 ZERO headroom The pre-emptive 6800 on this branch was still the right call when made -- it sat at slack 401 against an allowance of 401, one file from red, while passing -- but main moved first and one measurement should not produce four numbers. FOR #17142, the measurement that took three branches to produce. audio's pin is population - growth, the GAP bound, while the bound that fires on this guard is completed(): its loop continues past the canonical file and all of repo_tests/ before reading anything, so completed sits ~387 below population and falls every time anyone adds a guard. Main pinned 5752 with an apparent 402 of headroom. The real figure, measured on this session's three branches: issue-13049-lazy-import margin 16 issue-14881-enum-hand-copies margin 15 issue-16688-fork-collapse margin 14 Three ordinary branches, each adding one or two repo_tests guards, took it from 16 to 14. That is the allowance being wrong, not the floors, and it is why this guard has a re-pin history at all. Verified on this tree: prompt_injection_detector_strict_mode + reach_declarations + one_git_enumeration + redaction_concept_census + threat_model_anchors ................................... 274 passed Refs #16688, #17142, #17317, #17318 * fix(guards): migrate the census floor to declare(), re-measured not carried (#16688, #15928) reach_floor_migration_test::test_no_new_hand_rolled_floor_remains_unmigrated failed: redaction_concept_census_test declared `_MIN_SOURCE_FILES` as a hand-rolled reach-sized floor. #15928's rule is that a floor >=100 which binds a tree sweep goes through _reach.declare(), because a number chosen by feel decays silently. IT HAD ALREADY DECAYED, IN THE FOUR HOURS SINCE I WROTE IT. The constant was 1500. Re-measured with a throwaway script, as #15928 requires rather than carrying the old value across: the real population is 3,393 tracked production .py files (excluding .worktrees/, docs/, repo_tests/ and test files). 1500 sat 56% BELOW the tree it claimed to bind -- it would have passed over a sweep that had lost more than half the codebase and still reported "no new redaction implementations". Which is the argument #15928 makes, demonstrated on a guard whose own docstring is about not confusing "nothing found" with "did not look". I wrote the number by feel in a file about measurement discipline. declare("redaction-concept-census", floor=3093, growth=300, what="tracked production python files") growth=300 matches the sibling declaration in prompt_injection_detector_strict_mode_test over a population of similar size. Also wired REACH.completed(parsed), which the hand-rolled version had no way to express: the floor now binds what the census actually PARSED, not merely what it listed. Without it the sweep could enumerate 3,393 files, fail to parse 3,000 of them, and report the same clean as a tree with nothing in it -- the exact distinction this guard exists to make. discover() takes a root and returns [] on EmptyEnumeration rather than letting it escape: reach_declarations_test drives every declaration against an empty repository and demands ReachFloorError specifically, so letting EmptyEnumeration through means the declaration fails loudly while saying nothing about whether its floor binds. Found by autobot-ai-f8 reading the shard-8 log on the sibling #17342 branch, whose enum_hand_copy_guard_test.py carries the identical defect. Confirmed here by running the migration guard rather than assuming the sibling's diagnosis transferred. Verified on this tree: reach_floor_migration + redaction_concept_census + reach_declarations ...................................... 206 passed Refs #16688, #15928, #17312 * chore(ratchets): adopt main's floors and re-pin the census I set at the minimum (#16688, #17142) Rebased onto origin/main e8c7bf9. Main's floors adopted where they hold; the one this branch created did not. audio-extension-allowlist adopted main's 5757 (NOT 5752 -- superseded) hooks-path-override adopted main's 6736 prompt-injection-... adopted main's 2958, green here import-hermeticity adopted main's 604, green here redaction-concept-census 3093 -> 3247, MINE, and it was red THE CENSUS FLOOR WAS MY OWN MINIMUM PIN, AND IT IS THE SECOND ONE TONIGHT. I created that declaration earlier in this branch, migrating a hand-rolled floor under #15928, and pinned it at 3093 -- population 3393 minus growth 300, which is the MINIMUM OF THE WINDOW. It went red on this rebase: population reached 3397, slack 304 against an allowance of 300. Four files. The sibling declaration on the #14881 branch (enum-hand-copy-census, pinned 5755 at population 6155) went red the same way at 6165. Ten files. Both were committed AFTER I filed the comment on #17142 arguing that `population - growth` is not a floor but a snapshot with zero tolerance by construction. So: wrote the argument, made the mistake twice, noticed neither time until reach_declarations_test failed. That is the case for the check being mechanical rather than a review item, and it is self-reported on #17142 with timestamps because "people who understand the rule will follow it" is the objection it removes. Re-measured on this tree rather than adjusted: 3,397 tracked production .py files using the guard's own filter. Window [3097, 3397]; 3247 is mid-window and buys ~150 files of headroom instead of four. Measured after the re-pin: audio pop 6163 completed 5776 pin 5757 slack 406/700 margin 19 hooks pop 6949 pin 6736 slack 213/401 prompt-inj pop 3258 pin 2958 slack 300/300 ZERO headroom import-herm pop 664 pin 604 slack 60/60 ZERO headroom census pop 3397 pin 3247 slack 150/300 The two at zero headroom are adopted unchanged because this branch adds nothing to their roots. #17346 carries prompt-injection at 3100 and #17342 carries import-hermeticity at 633 -- those are the branches that do. audio is 5757, not the 5752 quoted to me during review: #17317's value was superseded while that message was in flight. Read by AST at e8c7bf9 rather than taken from the message, which is the same discipline this session has been applying all night -- a value quoted by name is already a snapshot. Verified on this tree: reach_declarations + reach_floor_migration + redaction_concept_census + threat_model_anchors .......... 247 passed Refs #16688, #17142, #15928 * fix(docs): delete a floor comment narrating a re-pin this branch does not ship (#16688) Third instance of the same defect, found by checking rather than by review. autobot-ai-f8 caught it on #17342 (fixed as acbfbe3) and #17346 (67f977f); this branch is the third of the three that re-pinned this declaration tonight, so it was checked before anyone had to report it. git diff origin/main...HEAD -- repo_tests/audio_extension_allowlist_test.py \ | grep -c 'floor=' -> 0 the file ships -> floor=5757 the block this branch ADDED said -> "Ratcheted 5450 -> 5700 (#16688)" An earlier revision of this branch did propose 5700 and the block described it correctly then. #17317 landed 5752, later 5757, and the rebase adopted main's value; the `floor=` hunk was resolved and the prose, living in a different commit, survived as a pure addition. The branch ended up ADDING a precise narrative about a change it no longer makes. Deleted rather than corrected, matching the #17346 fix: main's own comment sits immediately below explaining 5757, so a correction block would leave two voices on one floor -- which is the defect, not its remedy. The diff no longer touches this file. Note this differs from the #17342 fix, which rewrote its block instead. That was the right call there and the wrong one to repeat: at the time I had not seen that main already carried a true account a few lines down, so the correction became a second voice too. It is deleted there as well. WHY THIS FILE AND NO OTHER, which is f8's observation: three branches re-pinned this one declaration tonight. A floor whose comment block is append-only across concurrent branches accumulates contradictory narratives even when every individual re-pin is correct, because the rebase resolves the value and nothing resolves the sentence. The durable record of the mechanism belongs in commit messages and on #17142 -- not as competing prose in a hub file. Refs #16688, #17142, #17356 * chore(ratchets): ship main's floor files unmodified, and drop measurements from prose (#16688) Two distinct problems, both found by grepping this branch's own diff for prose naming a measurement -- which is the check autobot-ai-f8 asked for after the orphaned-narrative round. 1. THIS BRANCH WAS SHIPPING TWO STALE FLOORS AND TWO SUPERSEDED COPIES OF MAIN'S OWN COMMENTS. audio-extension-allowlist carried 5757 with main's older narrative while main is at 5764 with a rewritten one; prompt-injection-detector-strict-mode carried 2958's text while main is at 2960. Neither needs a branch-specific value -- measured on this tree, main's floors pass here -- so both files are now byte-identical to origin/main. That is worse than an orphaned note of my own: a superseded copy of ANOTHER branch's comment silently misattributes a stale account to them, and reads as authoritative because it is written in their voice. 2. EVERY MEASUREMENT IS OUT OF THE PROSE THIS BRANCH ADDS. The census declaration's migration note, its re-pin note, the hooks adoption note and one line of REDACTION_BOUNDARY.md all cited a population or a slack figure. Each was correct when written and stale one rebase later. The last one is the instructive case: "enumerate 3,393 files, fail to parse 3,000 of them" was an ILLUSTRATION, not a claim -- and an illustration carrying a real population goes stale exactly like a measurement while reading as more authoritative for being precise. Now "enumerate the whole tree, fail to parse most of it", which stays true. Prose that cites no measurement cannot go stale. The arithmetic lives in commit messages, which cannot drift from the tree they describe because they are not in it. Verified on this tree: reach_declarations + reach_floor_migration + redaction_concept_census ................................ 206 passed Refs #16688, #17142, #17356 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…in and post the evidence (#17090) (#17382) * feat(agents): verify an issue's acceptance criteria against merged main and post the evidence (#17090) Part of #16698. Agents spend much of their effort answering "is this criterion actually met in origin/main" before closing or ticking anything. That answer is mechanical enough to produce and far too consequential to guess: a wrong "met" closes an issue on work that does not exist. So every path is built so the cheapest outcome is CAN'T TELL. - A criterion whose wording asks about a running system never reaches a model. It is needs_host_evidence by rule, from the criterion's own text, and the test asserts the decision backend was not called -- a verdict that happened to agree would pass a weaker test while leaving the criterion answerable as "met from code alone" on the next model. - A met or not-met verdict must carry a file:line that EXISTS. Citations are parsed out of the model's own reasoning and opened with `git show origin/main:<path>`; a line past EOF, a missing file, or a blank line is rejected, and a verdict whose citations are all rejected is downgraded to can't tell with the rejections recorded. #17090's last criterion asks for a fabricated file:line to be rejected rather than noted, because an evidence-shaped string is the failure mode that gets acted on. - A decision that never arrives is can't tell. `decisions.decide` raises rather than manufacturing an answer; that raise is caught and reported as what it is. - Nothing ticks a box. The GitHub surface has one write verb, `issue comment`, and TestItNeverTicksABox parses the module to assert it -- a rule held only by "nobody has written that call yet" is not held. - An unreadable issue raises instead of verifying an empty body, so "nobody read it" can never be published as "no criteria found". Reading is deliberately model-free (ac_criteria.py): what the issue says and what merged code contains are facts, and a guess would be indistinguishable from either. Its stated boundary is that a prose amendment in a comment cannot be parsed -- comments that read like one are reported as unread in the posted comment rather than left to make a stale list look complete. The vault-owned credential path (#13859) moved out of workers/audit_tasks.py into services/github_service_credential.py rather than being copied: the audit worker was its first caller and this is the second, and a forked copy of a secrets path is the duplication that matters -- the two drift and the one nobody watches keeps working after a revocation. Behaviour is unchanged, including the class-name-only logging and the cache-don't-re-derive rule; gh_env takes an optional resolver so the worker's own patch point still governs its subprocesses, which is why its 87 tests pass untouched. audit_tasks.py 1085 -> 991, re-pinned. Refs #17090 * chore(guards): re-pin the tracked-file floor mid-window (#17126, #17090) `tracked-key-material` counts every tracked file of any extension -- the broadest population any floor here watches -- and it was pinned at 10_000 with growth 800, a ceiling of 10_800. Main sat at 10_796: four files of headroom on a population that moves on every merge, whatever the language. This PR's five modules and their tests were the first to cross it. Arithmetic, measured on this rebased tree by the guard itself rather than by `git ls-files` (which counts differently): live population 10_805 previous floor 10_000 (ceiling 10_800 -- breached by 5) valid range 10_005 .. ~10_805 new floor 10_400 10_400 is mid-window on purpose, not `population - growth`. The bottom of the range is a snapshot rather than a floor: it has zero headroom by construction, so the next branch to add a file re-reddens it -- which is how this one arrived, and #17142 records the same pattern on two other declarations. 10_400 leaves ~395 files of growth before the next re-pin and ~405 of deletion before the floor itself bites, which is the symmetry this population deserves. The comment above the declaration now states the floor's PURPOSE (catching a collapsed enumeration, not tracking growth) and carries no measured numbers, per #17384: a population written into a comment survives the merge that changes it, and one paragraph per re-pin is how four floor files ended up describing branches that no longer exist. The arithmetic lives here instead. Refs #17090
…tation (#17054) (#17396) Three fixtures in one PR hit the same defect and twice the suggested repair would have deleted the coverage it appeared to fix. When a change adds an authorization gate upstream, every test whose fixture predates it short-circuits before reaching its own subject. The failure reads as `assert <new> == <old>` with a plausible new value, which is indistinguishable in a CI log from an expectation that has simply drifted. The obvious repair -- update the assertion -- goes green and removes the test's real subject. Observed on #17054's desktop-authz gate: - two VNC handshake tests whose fake user carried no `role` began reporting the gate's `close(1008)` where the test asserted the handshake's `1011`; 1011 was never a changed code, it was a code no longer REACHED - a third, `role: "user"`, stopped reaching `accept()` at all Re-pinning those expectations would have removed the only coverage of the handshake-failure path and of the accept path, and left the gate's own headline case -- a signed-in account without the permission -- covered by nothing. The tell recorded with it is the cheap one: if the "corrected" expectation duplicates what the new gate's own suite already asserts, the test has stopped testing its own subject. Docs only; no code or test behaviour changes. Refs #17054
…17392) `master_fd` is non-blocking (`os.set_blocking(..., False)` in `_spawn`), and the write loop called `os.write(self.master_fd, data)` once, discarding the return value, under a broad `except Exception: log; break`. That loses user input two ways, both silent: * `os.write` returns how many bytes it TOOK, and on a non-blocking descriptor that can be fewer than given. The remainder was dropped, so a paste larger than the PTY buffer arrived truncated -- at a byte boundary that can split a multi-byte UTF-8 character. * A full buffer raises `BlockingIOError`, which means "not now", not "failed". The broad handler caught it, logged, and `break` -- ending the write thread for the life of the session. The read loop keeps streaming output, so the terminal looks healthy while accepting no input at all. `_write_all` loops on the remainder and treats EAGAIN as backpressure. THE BOUND IS TIME SINCE LAST PROGRESS, NOT TOTAL ELAPSED, and the difference is a bug I shipped into the first version of this fix. A total budget looks equivalent, passes every small-input test, and truncates exactly the large write the method exists to deliver: a 1 MiB paste needs many buffer-fills and blows a 5s cap. My own test caught it -- it is the one test whose payload is large enough to span multiple fills, which is why the payload size is now pinned in its docstring. The clock resets on every byte accepted, so the budget detects a child that has stopped reading rather than capping how long a legitimate write may take. On exhaustion the BlockingIOError is re-raised deliberately: a child that has not accepted a byte in that long is wedged, and failing loudly beats retrying forever. Two smaller defects in the same file, from the same review: * cleanup() closed the descriptor and set `master_fd = None` afterwards, leaving a window where the write loop could use a closed fd -- and, once the number was reused, one belonging to something else. It now publishes None first and closes the fd it took. * `TimingConstants.MICRO_DELAY` served both the write loop's idle bound and the post-SIGTERM grace period. Unrelated timings sharing one constant means tuning either silently moves the other, so the grace period is now `_PROC_TERM_GRACE_S`. Same SSOT value today, separate names. Tests run against a REAL non-blocking pipe, not a patched `os.write`: a fake reproduces the semantics someone believed, while a pipe whose buffer is actually full produces the real short write and the real EAGAIN and cannot drift from the kernel. It also avoids patching a global pytest itself writes through. Mutation checked: replacing `_write_all`'s body with a single `os.write` loses the tail in the short-write test and raises in the backpressure test. Closes #17381
…17126) (#17388) * fix(ci): a skipped smoke-test must not satisfy the required context (#17126) Owner rule, 2026-09-19: nothing lands on main without passing the Docker smoke test. `smoke-test` is a required status check published by two mutually exclusive jobs -- the real build in docker-smoke-test.yml and the path-filtered shim in docker-smoke-required-context.yml. Both jobs `needs: changes`, so a `changes` job that failed or was cancelled SKIPPED both of them. GitHub counts a skipped check run as satisfying a required status check, so the requirement was met with no smoke test run at all. Both files carried a comment asserting the opposite: "if `changes` errors, `needs` skips this job rather than reporting green ... which blocks the pull request". Correct about the skip, wrong about what a skip means, and written in the one place that cannot be falsified. Same shape as #17354's `global` channel comment and #17384's floor prose -- a claim parked where nothing can test it. The shim now runs on `always()` and REFUSES when its own detector did not answer, before reporting anything. The real build now also runs when its own `changes` did not succeed: two `changes` jobs exist, one per workflow, and the shim can only speak for its own. When nothing can say whether an image could have changed, the honest answer is to build it -- a full run on a rare infrastructure failure is the correct direction for a gate nothing is allowed to land without. The three cases stay mutually exclusive, so exactly one check run named `smoke-test` ever carries a conclusion: changes ok, docker != true shim succeeds (legitimate green) changes ok, docker == true shim skips, real build reports changes NOT ok shim fails; the real workflow builds `repo_tests/docker_smoke_required_context_17126_test.py` moves the claim somewhere it can fail: 14 tests over both workflows' `if:` expressions, the shim's refusal ORDER (an `exit 1` after the success echo would report green first), and the env-var seam. With positive controls, because every assertion is a substring check against a live file and those pass for two different reasons. Mutation proof, each fix reverted in turn with the mutation confirmed applied: shim `if:` back to the pre-#17126 form -> 3 tests red by name refusal moved after the success echo -> the order test red real build's `if:` back -> 1 test red by name all reverted -> 14 passed Closes #17126 * fix(ci): claim the invariant the smoke gate has, not a stronger one (#17126) A pre-merge review found the comment asserting something the code does not do, in the one place a future reader would trust it -- on a PR whose entire subject is a gate that reported green while checking nothing. The claim was "the three cases are mutually exclusive, so exactly one check run named `smoke-test` ever carries a conclusion". The first two are exclusive. The third is not a case: there are TWO `changes` jobs, one per workflow, and each job can only see its own. When both detectors fail, the shim reports failure AND the real build runs and reports whatever the build does -- two check runs, one context, possibly opposite, resolved by whichever finished last. The narrower invariant is the one #17126 needs and it does hold: THE SHIM CANNOT REPORT GREEN UNLESS ITS OWN DETECTOR SAID NO DOCKER PATHS CHANGED. So a disagreement between the two detectors can block a PR whose image build passed -- conservative, self-resolving on a re-run -- and can never admit one no build covered. Both comments now say that, and two tests assert it: the refusal precedes any success message, and it is conditioned on the shim's own result. The complement assumption the two detectors rest on is #17391's subject, named rather than left implicit. Refs #17126 * test(ci): pin the smoke gate's condition whole, and make its control read the file (#17126) A pre-merge review found two weaknesses in this PR's own guard, both of the class the guard exists to catch. THE CONNECTIVE WAS NEVER ASSERTED. Every check was `"clause" in condition`, so swapping `||` for `&&` between the disjuncts would make each job's `if` false in exactly the case it must run -- reopening #17126 -- while every assertion kept passing, because each token is still present verbatim. The condition is now pinned WHOLE for both workflows. Mutation proof, applied to the real file and confirmed by `git diff --stat` before the verdict was read: `||` -> `&&` in the shim's if: FAILED test_the_condition_is_pinned_whole_not_clause_by_clause[...] 1 failed, 18 passed <- the substring tests all still passed reverted: 19 passed That the older assertions stayed green under the mutation is the finding, not a side note. THE always() CONTROL COULD NOT FAIL. It asserted against two string literals defined in the test file and never read the workflow, so it reported coverage of the exact bug class #17126 fixed while exercising nothing. It now also checks the live condition. Also documented: the `always()` widening reaches every trigger, so a `changes` failure on `merge_group` now runs the full build where it previously skipped -- a real cost at the last gate before main, and still the right direction. The shim's trigger is pull_request only, so nothing double-reports there. Refs #17126 * docs(ci): state the four-case detector table, not a safety claim it does not carry (#17126) I corrected this comment once already -- it claimed the three cases were mutually exclusive, which is false -- and the correction then claimed the shim's green 'can never admit one where no build ran', which is ALSO false. Both were overstatements in the one place a reader would trust them, on a PR about a gate that reported green while checking nothing. Derived from the two live conditions, on a pull request with both detectors succeeding (S = shim's detector, R = the real build's): S.docker R.docker publishers true true real only correct false false shim only correct true false NEITHER no context is created at all false true BOTH shim green + the build's result Row 3 is #17126's hole by another door: a required check satisfies the gate by never existing, with nothing failed and nothing cancelled. Row 4 is worse than conservative -- the shim's green does not mean a build ran, and completion order decides the context. Neither row is a defect in either condition; each is correct for the value its own detector computed. One question, two detectors, an assumed agreement. Both files now carry the table, and the walk-through with proposed criteria is on #17391 -- the single-publisher problem, not closable in either condition alone. Refs #17126, #17391 * test(ci): the connective control never read the file it claimed to guard (#17126) Two review findings on #17388, both fixed here. `test_swapping_the_connective_would_be_caught` compared two of this module's own literals -- `_EXPECTED_CONDITIONS[_SHIM].replace("||", "&&")` against `_EXPECTED_CONDITIONS[_SHIM]` -- which is a tautology the moment the pin contains an `||`, and it never opened a workflow file. It also sat ABOVE the positive-controls banner, so its name and its position both read as a live guard while it was asserting nothing about the live condition. Renamed to say what it does, moved down with the other controls, and pointed at `_condition(_job(_SHIM))`. Reading the live file is what makes it non-vacuous: if the shim's condition ever stopped joining its disjuncts with `||`, the swap becomes a no-op and the inequality fails. That is the property being claimed -- that the whole-condition pin can see a connective where a substring check cannot. This is the same class as the finding the previous round fixed in `test_the_always_check_rejects_the_shape_that_caused_17126`: a control that never reads its subject reads as coverage and is not. Finding it twice in one file says the first fix was applied to the instance rather than to the pattern. `DOCKER_CHANGED` was declared in the step's `env:` and never read -- wired into both log paths instead of dropped, so a refusal now names the filter's answer alongside the detector's result, which is what a reader debugging a red shim needs and had to go to the job summary for. Closes #17126
…rusts an unshown citation (#17090) (#17397) * fix(agents): the AC verifier no longer truncates a criteria list or trusts an unshown citation (#17090) A code-reviewer pass on the MERGED #17090 found four defects. I merged that PR without a review pass because base was red and it carried the one-line floor fix; this is the deferred review's output, and the first finding is the worst failure that feature can have. TRUNCATION, reproduced before fixing. `_section_lines` ended the acceptance-criteria section at ANY line matching a heading pattern, unanchored and unlevelled: "## Acceptance criteria / - [ ] one / - [ ] two / ### Notes / ..." -> 2 of 4 criteria, no note "## Acceptance criteria / - [ ] Do X / **Note:** remember Y / - [ ] Do Z" -> 1 of 2, because `^\s*` let an INDENTED bold note match Both reported what they had read as the whole list. A reviewer seeing "2 met, 0 not met" would believe the issue's criteria were checked. Now only a heading at column zero, at the same level or higher than the one that opened the section, closes it -- and `criteria_after_the_section()` counts checkboxes below that heading so "no more criteria" and "stopped looking" cannot read the same (MEASUREMENT_DISCIPLINE). The count lands in the posted comment's notes. AN UNSHOWN CITATION IS NOT EVIDENCE. The citation check proved a `file:line` exists and is non-blank; it never checked the line had anything to do with the criterion. A citation to any real line in the tree survived. The reviewer proved it with this suite's own fixture -- line 2 reads "line two has content", which supports nothing. A citation must now also appear in the evidence block the model was shown. Existence is still checked FIRST, so a fabricated path reports "no such file" rather than the vaguer reason. THE CREDENTIAL CACHE. `gh_env()`'s cache is module-level and shared with the audit worker, which resets it as the first statement of every task for the reason #13859 records. This caller inherited the cache and not the reset, so a token revoked between runs kept being used and `vault_backed=True` was reported for a run that never touched the vault. `verify_and_post` now resets first. DOUBLE POSTING. `gh issue comment` is not idempotent, and a failed post cannot be told from an unobserved success, so a retry posted twice. The comments are already in hand from the fetch, so the check is free: an existing verification block means skip, with `force=True` as the explicit escape hatch. 13 new tests across the four files, each reproducing its finding before the fix. Refs #17090 * fix(agents): a citation to line 1 no longer rides in on the evidence for line 12 (#17090) Review findings on #17397, both fixed here rather than in a follow-up. HIGH -- the evidence check was `str(citation) not in evidence` over the whole blob, and `Citation.__str__` is `path:line` with no terminator. So `...:1` is a substring of `...:12`, `...:100` and `...:1002`, and `a.py:1` is a substring of `ba.py:12`. A fabricated citation to line 1 passed whenever the evidence quoted that file at any higher line -- and the lower the line number, the likelier the collision, which made line 1 the cheapest citation to invent inside the check whose entire purpose is to reject invented citations. `_quoted_in` now matches per evidence line on a `path:line:` prefix after stripping the two-space indent `code_searcher` joins its hits with. The trailing colon is the load-bearing character: it is what stops `:1` matching `:12`. The test that shipped with the original fix passed for a reason unrelated to what it asserted -- line 1 against evidence showing line 2, where no digit overlap exists. Three tests replace it: the same-path collision, a positive control on the two-digit line actually shown, and the path-tail collision (`ac_verifier.py:12` cited against `autobot-backend/agents/ac_verifier.py:12`) with BOTH paths readable, so the citation cannot be rejected for not existing and pass without reaching the evidence check. MEDIUM -- the CRITICAL's repair had no end-to-end test. `criteria_after_the_section` was unit-tested and the wiring in `verify_issue` read correctly, but a mutation deleting that wiring passed every test in the PR, because nothing drove the count through to the comment a human reads. Surfacing the truncation IS the fix, so three tests now drive body -> `verify_issue` -> `render_comment`: the note and its count reach the rendered comment, only the one criterion inside the section is verified, and a body with nothing after the section produces no note -- the negative control, without which the first two pass equally well against a `render_comment` that emits the note unconditionally. Refs #17090
…e-bound token is not a login (#17054, #17049) (#17369) * fix(security): the desktop socket requires desktop-control, not merely a login (#17054) api/vnc_proxy's websockify route calls enforce_ws_desktop_auth, whose signature names DESKTOP_VIEW and DESKTOP_INPUT. Those apply to a paired device. An ordinary browser session presents no device credential, so enforce_ws_remote_control_auth delegated it to enforce_ws_authentication -- which asks only whether the caller is somebody. Any signed-in account, whatever its role, got full view AND input of the managed desktop. The capability names were in the signature the whole time and never reached the path most callers take. A user credential now has to hold mcp.desktop.control for its role: admin and operator today. Everyone else is refused in the handshake with 1008, before any RFB byte is proxied. `control` rather than `read` because the socket cannot grant one without the other -- _forward_client_to_vnc forwards the client's KeyEvent and PointerEvent frames verbatim, so admitting on mcp.desktop.read would be a view-only grant that is not view-only. A real view-only tier needs RFB message-level filtering, which this gate does not attempt. Answered through role_has_permission, NOT is_admin_role. #13854 deliberately removed the administrative short-circuit from that function because it made a predicate the most permissive permission source in the system; re-adding it at this call site would reverse that ruling one file at a time. The consequence is deliberate and asserted rather than discovered: ROLE_PERMISSIONS[Role.SUPERADMIN] is empty by that same decision, so a SUPERADMIN IS REFUSED the desktop. If that is wrong the fix is superadmin's ROLE_PERMISSIONS entries -- one place -- not a bypass in the gate. Two tests pin it: one asserts the table's answer, one asserts the socket honours it, so they cannot drift. The paired-device path is untouched and asserted (#15146). Without that test this fix would close the desktop to the callers that feature was built for, which is the shape of a fix that breaks what it protects. THREAT_MODEL.md gains section 5 with the policy, the control-not-read invariant, the permission-source rule and the superadmin consequence. Tests patch the two credential seams the existing capability test uses -- _resolve_ws_device_credential and enforce_ws_authentication -- so the real enforce_ws_remote_control_auth and the real gate run. Patching the function under the gate would have tested it on a path production does not take, which is how an earlier fix in this repo passed with its vulnerability intact. Verified: 13 new tests, 60 across the new file plus the device-capability and ws-auth suites. Mutation -- restore the original "any signed-in user" behaviour -- fails 7 tests (the four ordinary roles, both closed-before-RFB cases, and superadmin) while the admin, operator and paired-device cases still pass, so the mutation moves only what it should. Local passes are not CI (#17144). AC4 (frontend hides the entry, i18n in 11 locales) is NOT delivered and is filed separately: several entry points and a different risk envelope from a backend authz gate. Refs #17054 * fix(security): a token minted for a purpose is not a login (#17049) The backend accepted ANY HS256 token signed with the platform key as its subject's ordinary login, whatever it was minted for. _extract_user_from_jwt runs FIRST in get_user_from_request, ahead of the run-JWT and device-JWT readers and their path allow-lists, so it answered before the guarded readers saw the token: a device JWT authenticated as its owning user on every endpoint (bypassing GH#9493's /api/devices/ allow-list and its read scope), an SLM MFA-pending temp token was a full login BEFORE the second factor, and a service-to-service token resolved to a role-user login. THE ISSUE'S OWN PROPOSED FIX WOULD HAVE LOGGED OUT EVERY SLM USER, and that is the whole shape of this change. "Return None when is_login_token(token_data) is false" requires a positive token_type: login claim -- minted only by this service (auth_middleware.py). Tokens from autobot-slm-backend carry identity in `sub` and NO token_type at all, and #12135 established those must keep working. The issue flags this itself ("Check before changing anything"); I verified it rather than trusted it: backend login (token_type: login) is_login_token=True is_purpose_bound=False SLM login (sub only) is_login_token=False is_purpose_bound=False device / mfa_pending / service is_login_token=False is_purpose_bound=True token_type: device is_login_token=False is_purpose_bound=True So the rule is a NEGATIVE check, in one shared predicate next to the positive one rather than a second spelling at the call site. Its second arm -- a token_type that is not `login` -- covers the device-token-service shape, which carries no NON_LOGIN_CLAIMS member and was the row #17049 marked "not verified". Applied to BOTH extraction paths. authenticate_websocket had the identical defect, and it matters to the desktop gate in the previous commit: a device JWT resolves to its OWNING USER, so it would have carried that user's role into a role-gated socket. Refusing is a fall-through, not a dead end -- get_user_from_request tries this, then the run-JWT reader, then the device-JWT reader -- so a device JWT now reaches its own guarded path instead of being answered as a user first. Both gates are expressed by EXTENDING existing conditions rather than adding lines, because auth_middleware.py sits at exactly its recorded ceiling of 1088 and a grandfathered file may not grow (#14236). The pre-commit hook caught the first attempt at 1119. The rationale lives in is_purpose_bound's docstring, which is where it belongs -- one rule, one place, rather than a paraphrase at each call site. I tried putting the socket gate in api/ws_security.py to avoid the ceiling and REVERTED it: the user dict `authenticate_websocket` returns drops the purpose claims, so the check would have been inert -- a gate that reads the wrong object and passes. It has to run where token_data is still in scope. Tests load auth_middleware.py BY PATH to bypass testkit/auth_middleware_stub.py, which replaces the module and has no _extract_user_from_jwt; an ordinary import would have asserted against a stand-in (#17343 tracks that harness gap). Verified: 33 tests pass across the new suite, the desktop gate and the #12135 auth-me regressions. Mutation -- implement is_purpose_bound as `not is_login_token(claims)`, i.e. EXACTLY the fix this issue proposed -- fails both SLM-login tests and nothing else, then green on revert. That is the check that makes this version trustworthy: the plausible wrong fix is the one the tests catch. Local passes are not CI (#17144). Closes #17049 * fix(tests): reconcile 11 CI failures with the two gates, one by fixture not expectation (#17054, #17049) Review pulled the shard-2 log: 11 failures, in two groups. Both were the new gates behaving correctly while older tests pinned the older shape -- but "the test disagrees because my code is better" is what a genuine regression also sounds like, so each was traced to its cause before anything was edited. One of the two groups was NOT what it looked like. GROUP 2 (2 failures, vnc_proxy_handshake_wiring_test) -- NOT an expectation change. The suggestion was to update 1011 -> 1008 and note that 1008 is the authz code. That would have been wrong. The tests patch `authenticate_websocket` to return {"username": "alice"} -- a user with NO role -- so the #17054 desktop gate refuses it with 1008 BEFORE the VNC handshake runs. The 1011 those tests assert is not a changed code; it is a code that is no longer reached, because the test's own subject (the handshake leg) is skipped. Updating the assertion would have made both tests assert authz -- already covered elsewhere -- and silently deleted the only coverage of the handshake-failure path. Fixed at the FIXTURE instead: alice gets a role that holds mcp.desktop.control, so the tests reach their subject and 1011 stays correct. GROUP 1 (9 failures, test_every_decision_path_refuses_each_non_human_credential) -- an expectation change, and one of the three kinds was mis-predicted. Every one of the nine is still REFUSED; #17049 moves the refusal earlier. Two kinds (slm_mfa_pending_token, slm_service_token) are now refused at authentication with 401. The third, device_jwt_on_platform_key, does NOT land on 401: the device reader recognises it and its allow-list refuses it with 403 "Device JWT not permitted on this endpoint" -- so with the fallback secret it now behaves exactly like a properly-keyed device JWT, which is the outcome #17049 asked for and better than a bare 401. THE COST, NAMED RATHER THAN ABSORBED. Those three no longer exercise the #17042 human decision check, because they never reach it. The gate keeps four kinds (pre_17042_login_jwt, internal_service_key, dev_header, auth_disabled), and `test_the_human_decision_check_is_still_exercised` now fails if that count drops below four -- so the gate cannot quietly stop being tested by everything while each individual move looks like an improvement. A SECOND table existed and the first run did not reveal it. test_approval_human_decider_17042 carries its own _LLC_EXPECTED for the LLC route, with the same three kinds. Updated the same way -- with one difference that matters: on that route the sync get_current_user never extends to the device reader, so device_jwt_on_platform_key lands on 401 there rather than the device allow-list's 403. THE COVERAGE FLOOR CAUGHT MY OWN ASSUMPTION. I added one shared floor of 4 and the LLC assertion failed at 3: internal_service_key is refused at resolution on that route and never reached the human check even before #17049. So the tables genuinely differ and a single shared number would have been wrong for one of them. Two named constants now, with the reason recorded beside them. The pre-commit `name-tests-test` hook then blocked the harness edit: `credential_harness.py does not match pattern "test_.*\.py"`. Pre-existing -- the hook fires for ANYONE editing that file, so it was effectively frozen. Renamed to `_credential_harness.py`, which is the convention the hook's own exclude pattern documents (`_*.py` under a tests dir is a shared helper pytest does not collect). Not an added exclusion: #16230's comment records that enumerating filenames there was itself the bug. Also fixed, pre-existing and surfaced rather than caused by this branch: the 401 rendered as "Authentication required: Authentication required". The catalog's AUTH_0002 message already IS that string, and both call sites passed it again as context. Dropping the redundant argument shortens the line rather than adding one, so the file stays at its 1088 ceiling. Verified: 157 tests pass across the two security suites, plus the handshake and desktop-gate suites. Local passes are not CI (#17144). Refs #17054 Refs #17049 * fix(security): the JWT refusal names which predicate fired (#17049) Review finding. The gate merges two causes into one condition -- "no identity claim" and "minted for another purpose" both mean "not a login", so one branch is right -- but it also merged them into one log line, and that line is the only observable an operator has. "Why was my device JWT refused" and "why is this token missing a subject" are different investigations, and the merged message sends the second one hunting a malformed token that is fine. This gate is also what makes people start asking, so it owns the answer. One line rather than two branches, because auth_middleware.py is at exactly its 1088 ceiling and a grandfathered file may not grow (#14236). The first attempt used a `why = ...` local and came out one line over; folding the conditional into the logger call keeps three lines and lands at 117 characters against the 120 limit. The issue numbers leave the log line to buy that room. They are in `is_purpose_bound`'s docstring, which is where the full reasoning already lives -- a log line names the cause, it is not the place to carry citations. Verified: 13 tests pass. New test asserts each cause produces ITS OWN message and not the other's, in both directions. Mutation -- restore the merged line -- fails exactly that test, then green on revert. Local passes are not CI (#17144). Refs #17049 * docs(threat-model): compress the desktop section, then raise a ceiling that was never a measurement (#17054) THREAT_MODEL.md sits at exactly its 140-line ceiling, so adding a fifth trust boundary broke it. Unlike the population floors hit twice today, raising this one is not automatically correct: the guard's name states its purpose -- "short enough to read every review" -- so a thoughtless bump retires the property instead of fixing anything. Did the cheap thing first. Section 5 was the LONGEST in the document at 38 lines; it is now 20, the second-shortest of the five. What came out was reasoning that already exists in `enforce_ws_desktop_auth`'s docstring, which is where a reader who wants it already is. What stayed is what a reviewer cannot re-derive from a diff: the boundary, the one gate, and the two invariants that get violated silently -- `control` never `read`, and the permission table rather than `is_admin_role`. Did NOT trim sections 1-4 to make room. Main's 140 lines carry 21 blanks and no padding; cutting someone else's security content so mine fits would degrade the document to satisfy my own addition, which is the opposite of what the ceiling is for. That leaves 160, so the ceiling moves -- deliberately, with the argument written where the next person will copy it. 140 was never a readability measurement; it was the document's size when the guard was written. What it protects is per-boundary cost, because a reviewer reads the preamble, the cross-cutting section and the one section their diff touches: before 140 / 4 boundaries = 35.0 lines each after 160 / 5 boundaries = 32.0 lines each The doc got denser, not baggier. The constant now carries that arithmetic and one rule: MOVE IT ONLY FOR A NEW BOUNDARY. Prose growth inside existing sections still hits the wall, which is the property being kept -- raising the number for prose is what would retire it. Also fixed: the preamble said "these four subsystems". My section made that false the moment it landed, independently of the line count. Verified: 41 tests pass, including the anchor-resolution and link checks over the rewritten section. Local passes are not CI (#17144). Refs #17054 * test(api): restore a third VNC test's subject, and cover the case the gate introduces (#17054) Third instance of one fixture problem, in a file the earlier two fixes did not reach. `vnc_proxy_websocket_auth_test.py:78` builds `{"username": "alice", "role": "user"}`, and `user` does not hold mcp.desktop.control -- only admin and operator do -- so the desktop gate refuses before the handshake and `accept` is never awaited. As with the two in vnc_proxy_handshake_wiring_test.py, the test's SUBJECT became unreachable; the expectation is not wrong and is not weakened. Fixed with `operator`, deliberately not `admin`: operator is the least-privileged role that still reaches the accept, so the test exercises a gate it must pass rather than an admin bypassing everything on the way. THE SECOND HALF IS THE MORE USEFUL ONE, and it is a gap this PR created. That file tested the UNAUTHENTICATED refusal (#14959) and nothing else. The case this gate actually introduces -- a real signed-in account WITHOUT the permission, which is the most likely real-world refusal -- had no test. It was being exercised only by accident, by the fixture above, and the accident failed rather than asserted. Fixing the fixture alone would have left it uncovered. So: a parametrised sibling asserting user/editor/analyst/readonly/superadmin are refused with `accept` never awaited and close(1008). `superadmin` is in that list deliberately -- ROLE_PERMISSIONS[SUPERADMIN] is empty by #13854, so the role that sounds highest is refused, and if that is ever ruled wrong the fix is that table and the parameter moves to the permitted test. One place, both sides asserted. Verified: 7 tests in that file, 22 across the three VNC/desktop suites. Mutation -- remove the gate, restoring "any signed-in user" -- fails all five new refusal cases and leaves the permitted one passing, which is the correct split: an operator is admitted either way. Local passes are not CI (#17144). Refs #17054 * docs(guards): state the delimiter behind the _MAX_LINES rationale (#17054) The comment justifying the 140 -> 160 raise called the new section "the second-shortest of the five" without measuring it. Measured heading-to- heading with every `## ` line delimiting, the boundaries run 20/25/31/31/33 and section 5 is the 20 -- the shortest. The original adjective was wrong, and so was my first correction to "second-longest". That first correction is the point worth recording. It came from an awk delimiter of `^## [0-9]+\.`, which cannot see the unnumbered "Cross-cutting" heading, so section 5 absorbed it and measured 32 -- a real number, honestly produced, of a span that is not the section. An adjective derived from a measurement still carries the measurement's definition, so the definition is now in the comment beside it, where the next reader can tell a disagreement about the text from a disagreement about the ruler. The arithmetic the adjective sits next to was never in doubt and re-verifies against current origin/main after the rebase: still 140 lines / 4 boundaries = 35.0 each, so 160 / 5 = 32.0 stands and the ceiling stays 160. * test(auth): pin the websocket purpose-bound guard, and correct what it is worth (#17054) A review could not substantiate this branch's earlier claim that the WebSocket path carried the "identical defect" as the HTTP one, and stated that as a gap rather than resolving it either way. It was right, and the claim was overstated. Correcting it here rather than leaving the stronger version standing. The two paths differ: - HTTP: _extract_user_from_jwt falls back `username or sub or user_id`, and a device JWT carries user_id. It authenticated as its owning user on every endpoint. That is the real defect (#17049). - WS: authenticate_websocket builds its result with DIRECT subscripts, token_data["username"], and no purpose-bound minter emits username. services/device_token_service.py:43-50 is the only minter setting a token_type and its payload is device_id/user_id/scope/platform/iat. So a device JWT already died there on a KeyError swallowed by `except Exception` -- rejected, but by accident. Rejecting by accident and rejecting on purpose are the same outcome and different guarantees. The accidental one stops holding the moment some minter adds a username claim, a change with no obvious connection to authentication, and nothing in the suite would fail when it does. So the guard stays and is now pinned by the shape no minter produces yet: a purpose-bound token that also carries username and role. Mutation-checked -- replacing the WS call site's `token_data and not is_purpose_bound(token_data)` with `token_data` fails exactly this test, and the failure shows the device token authenticating as a full user dict (role=user, auth_method=jwt_websocket). 1 failed, 14 passed. Before this test the same mutation reddened nothing, which is precisely the reviewer's finding. A positive control is included, because "refuses a device token" is otherwise satisfied by refusing everyone. No production code changed; auth_middleware.py stays at its ceiling. Refs #17054, #17049
…k tree (#17410) (#17415) * fix(hooks): stop the Stop orphan-check exiting non-zero outside a work tree (#17410) The Stop hook resolved its script with $(git rev-parse --show-toplevel), which collapses to /scripts/... when the session cwd is not in a work tree (exit 127). It now uses $CLAUDE_PROJECT_DIR like every other project hook. The script's own guard, cd "$(git_repo_root)" || exit 0, never fired either: an empty substitution makes it cd "", a no-op success, and a later git call died under set -e with 128. verify-done.sh and lint-conventions.sh carried the same guard; outside a repo they failed closed further down (exit 1) instead of exiting 2 with "not a git repo". * fix(hooks): quote $CLAUDE_PROJECT_DIR in every project hook command (#17410) Hook commands run through sh -c, so an unquoted $CLAUDE_PROJECT_DIR splits on a space in the checkout path and the hook exits 127 -- the same symptom as the cwd route fixed in the previous commit, reached by word splitting. Fixed as a class (one defect, one fix): the Stop hook line this PR added and the three pre-existing PreToolUse hooks. A class-wide test fails on any unquoted hook command, so a new hook copying a neighbour cannot reintroduce it. * fix(guards): the hook scan went blind when the expansion was quoted (#17410) Quoting the hook commands made `_SCRIPT_REFERENCE` match nothing. Its pattern required `$CLAUDE_PROJECT_DIR` immediately followed by `/`, and the quoted form puts a `"` between them, so the extractor found zero script references in a settings file holding nine hook commands. before 0 of 9 commands matched after 4 of 9 -- block-dangerous-commands, protect-files, scan-secrets, session-stop-orphan-check Measured in all three spellings, because a widening that shrinks the population is a narrowing wearing a widening's syntax: quoted "$CLAUDE_PROJECT_DIR"/x.sh 0 -> 4 unquoted $CLAUDE_PROJECT_DIR/x.sh still matches git-rev $(git rev-parse --show-toplevel)/x.sh still matches MUTATION: drop the `"?` again FAILED test_the_command_scan_finds_the_script_references_that_exist 1 failed, 11 passed <- fails for its own reason, not a neighbour's with the fix 12 passed The guard caught this itself -- `AssertionError: the extractor found only []` is a reachability check reporting that its own scan went blind, which is the one failure mode this file exists to make loud. Without it, quoting the hooks would have silently disabled the scan and every future hook would have gone unchecked. Refs #17410
…r read (#17571, #14785) (#17608) * fix(canonical): the audit harness reported clean over a tree it never read (#17571, #14785) `scripts/canonical_check.mjs --all` printed `0 violations` and exited 0 while the same rule, handed the same file directly, found one. The flag satisfied the "pass --files or --all" check and then filtered `args.files`, which `--all` never populated. The walk was marked "a Wave 3 task", so the mode existed, reported success, and had never opened anything. That is the defect the canonical rules exist to catch, inside the tool meant to catch it: the rule existed and the tree was never asked. Four mechanisms were missing, all verified by running it: --all now walks each rule's TARGETS. 51 violations where it reported 0. TARGETS is honoured. runRules ran every rule against every file, so the declaration was decorative: a rule scoped to one tree silently inspected another. A rule may also declare EXTENSIONS, which is what lets a CSS rule exist without the console.log rule scanning stylesheets. A TARGETS entry naming a missing path is reported and fails the run. It previously made its rule inspect nothing while reporting the same clean result as a rule that found nothing. An empty file set refuses rather than printing a clean run. Adds the first design-system rule the harness has carried: ds-unloaded-authority blocks on a stylesheet that claims to be the canonical token source and is imported by nothing. It fires on assets/tokens.css, which opens "Canonical CSS Design Tokens", declares no tokens at all, and appears in no import statement. Severity `block`, so the run exits 1 -- the shipped smoke rule is `warn`, and before this nothing could fail. Three bugs of my own on the way, each the shape this harness is for: The rule first needed repo context passed in; check() takes one argument, so it returned [] forever -- dead on arrival. Its import-graph needle was an unanchored substring, so looking for `tokens.css` matched `design-tokens.css`, which IS imported. The unloaded file read as loaded, and telling those two names apart is the rule's entire job. Anchored on a path boundary. I labelled it SEVERITY "error". makeDiagnostic rejects anything outside [block, warn, audit], so that crashes the run rather than passing quietly -- which also corrects my earlier claim on #17571 that "error does not fail". A rule cannot opt into failing by inventing a level; it says block or it is advisory. Nine tests added, covering the walk, TARGETS and EXTENSIONS scoping, an unreachable target, a prefix that is not a tree match (`autobot-frontend/srcx`), and that a block diagnostic exits 1. 14 pass, up from 5. The CI workflow still has no failing step, so this is enforceable rather than enforced; that is the remaining half of #17571. * feat(canonical): wire the frontend audit into CI and let a blocking rule fail it (#17571, #14785) The audit generated, uploaded and summarised, and had no failing step at all -- so a `block` diagnostic landed in an artifact nobody opened. A rule that cannot fail anything is a report, and that is precisely why the design system has been advisory. The frontend audit is now actually run. Its step previously wrote a placeholder saying `--all` was unimplemented, which was true: the flag was accepted and walked nothing, reporting a clean tree it had never opened. The exit code is captured rather than allowed to abort the generate step, and the gate runs AFTER the upload. Under `bash -e` a blocking diagnostic would otherwise kill the job before the artifact and job summary existed -- publishing nothing is how people stop reading a gate. Only `block` fails. The vocabulary is block | warn | audit and makeDiagnostic rejects anything else, so a rule cannot opt into failing by inventing a level. The 50 existing console.log findings are `warn` and do not fail. Removes `autobot-frontend/src/assets/tokens.css`, the one blocking violation, so this gate lands green instead of reddening main on arrival. Deleting it loses nothing, and that was measured rather than assumed: 310 lines, ZERO non-comment lines, zero declarations, and no import statement anywhere names it. Of the 128 token names it mentions in prose, 113 are declared in loaded files and the other 15 are wildcard prefixes (`--color-`, `--spacing-`), not names. It opened "Canonical CSS Design Tokens ... serves as the primary reference", so the file asserting authority was the one nothing loaded. The rule's tests moved off the live tree onto fixtures in the same change. Asserting "the repo contains a blocking violation" would have passed only until the backlog drained and then failed for the best possible reason -- RATCHET_BASELINES.md names this, and removing the single instance is what surfaced it. Three fixtures now: a claiming-and-unimported stylesheet blocks, an ordinary one does not, and the waiver is honoured. One live assertion remains, stated as the outcome: the tree has no blocking violations. Verified: 17 harness tests pass, up from 5; `--all` exits 0 with 50 warn diagnostics; the workflow parses. * fix(canonical): refuse a run that inspects nothing, and resolve import paths (#17571, #14785) Three review findings, all correct, and all the same class this PR is about -- a check reporting clean over something it did not examine. 1. A run with files but no COVERAGE exited 0. `--files` naming only paths outside every rule's TARGETS, or with an extension no rule accepts, left every rule skipped and still reported success. Having files is not having coverage. It now exits 2 and says how many files were given and that none were in scope. That is the `--all` bug one level in, and I had added the empty-list refusal without noticing the covered-but-skipped case. 2. Read failures became a clean audit. Both walkers caught and continued, so an unreadable directory silently shrank the sweep while the run still printed "0 violations". Fixing that surfaced a distinction I had collapsed: an ABSENT tree is not an UNREADABLE one. ENOENT on a top-level target means the tree does not exist, which `unreachableTargets` already reports, so continuing is not a silent loss. Anything else -- a permission error, or any failure on a directory the walk itself just discovered -- is a directory that exists and could not be read, and that now throws. Three test fixtures caught this by failing on a temp root with no `autobot-slm-frontend`. 3. The rule matched import paths by pattern instead of resolving them, and I had already got this wrong twice in opposite directions: - unanchored, `tokens.css` matched `design-tokens.css`, which IS imported, so the unloaded file read as loaded -- and telling those two apart is this rule's whole job; - anchored on a preceding `/`, it then missed a legitimate same-directory `@import "tokens.css"` and could still match a same-named file elsewhere. Specifiers are now resolved against the importing file's directory and compared as absolute paths, which has neither failure. The keyword is captured because it decides what a BARE specifier means: in CSS `@import "tokens.css"` is relative to the stylesheet, in JavaScript a bare specifier is a package. Treating them alike is what produced the miss. Five tests added, 22 passing, up from 17: a file outside every TARGETS exits 2, a same-directory `@import` counts as loaded, a same-named file in another directory does not, and a prefixed name is not a match. Verified: `--all` exits 0 over a clean tree; an uncovered `--files` exits 2; a covered file exits 0. A fourth thing, found by the fix breaking its own commit: the refusal above was too blunt. The pre-commit hook runs `canonical_check.mjs --files` over every changed `autobot-frontend/**/*.{ts,vue,mjs,js}`, which includes this harness, which no rule targets -- so exit 2 failed the very commit implementing it. The two modes differ in what "nothing in scope" means. `--all` chose the files itself, so nothing matched means it inspected nothing and success would be a false clean bill: it refuses. `--files` is a filter and the caller chose the files: it reports what it skipped and exits 0. That keeps the reviewer's point -- an audit cannot report clean over an empty inspection -- without breaking a per-file hook whose job is to skip what it does not cover. * fix(ci): a deleted CSS file made stylelint check nothing at all (#17571) The stylelint job builds its file list with `git diff --name-only`, which reports DELETED paths, and hands that list to `npx stylelint`. stylelint answers NoFilesFoundError for a path not in the checkout and exits 123 *before* linting any of the files that do exist -- so a PR removing one CSS file received zero lint coverage on the CSS it actually changed. `continue-on-error: true` then rendered that as an ordinary red badge, which is the reading the guard doctrine exists to forbid: "ran and found problems" and "never opened a file" produced the same signal. This PR hit it by deleting `autobot-frontend/src/assets/tokens.css`. `--diff-filter=d` on both project diffs. Not a new idea in this repo -- `import-hermeticity-sweep.yml:123` already carries the flag with the same justification, so stylelint-tokens was the one workflow out of step, not a defect class needing a sweep. The guard runs the workflow's own `run:` script in a throwaway repo rather than grepping it for the flag, per `prepush_diff_range_16637_test.py`: a flag can be present and the pipeline still wrong. Three behaviours are pinned separately, because a script emitting nothing would satisfy the deletion assertion while checking even less than the bug did -- the deleted path is absent, the added and modified paths are still listed, and `frontend_count` (which gates whether the lint step runs at all) matches the filtered list. Verified against a throwaway repo before pushing: listed `['src/added.vue', 'src/kept.css']`, `frontend_count=2`, deleted path absent. * fix: restore 32 files my own `git add -A` deleted (#17571) Not a change I made or intended. `517b1aee97` shows 37 files changed and 16,590 deletions; three of those files are this PR's subject and the other 34 are collateral -- `.dockerignore`, `cosign.pub`, `.vscode/settings.json`, `autobot-infrastructure/.../redis.conf`, every `data/system_knowledge/` tool, procedure and workflow, `data/file_manager_root/`, the `.superpowers/` reports. Mechanism: those paths are TRACKED and also matched by `.gitignore` (`.gitignore:345` `.dockerignore`, `:93` `*.pub`, `:434` `**/.vscode/`). They were absent from this worktree, and `git add -A` stages a deletion for a tracked file missing from the working tree -- .gitignore only ever governs whether an UNTRACKED file is added, never whether a tracked one is removed. So a bare `git add -A` in any worktree missing a tracked-but-ignored file commits its removal silently. The "Detect Mass Deletions (#4111)" pre-commit hook reported Passed for all 34. This is also the cause of most of this PR's red, which had been read as one guard objecting to the deliberate `assets/tokens.css` removal: - `.dockerignore` gone -> all 6 of test_dockerignore_test_file_coverage_14127 (FileNotFoundError, not an assertion) - data/system_knowledge/... -> the #17089 phase-criteria path guard - redis.conf, .superpowers/ -> the glob-declaration guards Restored from `origin/main` and verified byte-identical per path. TWO FILES ARE NOT RESTORED HERE, and this is a blocker I am reporting rather than working around. `data/file_manager_root/test.py` (`print("Hello World")`) and `data/file_manager_root/test.js` (`console.log("test")`) are one-line file-manager UPLOAD FIXTURES whose content is the point of them. Restoring them is refused by the `No print()/console.* in Production Code (#1082)` pre-commit hook, which scans staged files and does not know these are data: COMMIT BLOCKED: 2 print()/console.* violation(s) found The exemptions available are `# noqa: print` / `// noqa: console`, which would change the fixtures' contents so they no longer match `origin/main`, and a path allowlist, which that hook's own comments (#16008) argue against at length as "an exemption with no allowlist, no baseline and no comment". Neither is mine to choose unilaterally, so the two files stay deleted and stated rather than quietly patched. `pre-commit-no-print-console_test.py::TestScanCostAndRepoWideResult` is expected to stay red until they are back. Refs #17571 * fix: restore the five remaining files the same `git add -A` deleted (#17571) The tail of the previous commit's restore. These five needed `git checkout origin/main -- <path>` rather than `git add -A -- <path>`, because they were absent from the working tree entirely, so there was nothing for `add` to stage: autobot-infrastructure/autobot-database/templates/redis.conf data/system_knowledge/procedures/evidence_handling.yaml data/system_knowledge/workflows/image_forensics.yaml data/system_knowledge/workflows/incident_response.yaml data/system_knowledge/workflows/network_analysis.yaml Those absences are what the glob-declaration guards were reporting during the pre-push run -- `slm_frontend_atomic_publish_15610_test.py` names the redis.conf template and `generate_service_keys_export_dir_16348_test.py` names evidence_handling.yaml. I read that output as an informational listing of affected guards. It was the deletion being announced, one path per line, and I walked past it twice. 32 of 34 now restored and byte-identical to `origin/main`. The two upload fixtures carrying a `print(` and a `console.log(` remain deleted and are reported in the previous commit rather than worked around. Refs #17571 * fix(repo_tests): a fixture path became a real file, and main is green without noticing (#17571) `prepush_frontend_projects_17574_test.py` feeds sample paths to the pre-push hook's filter. They must NOT resolve: `python_filter_covers_its_guards_test` records any quoted path that resolves as a tree this guard depends on, and the remedy for that is an entry in a shrink-only record which nothing can later remove. The docstring already carried two constraints pulling against each other. A third arrived from the tree rather than from a guard. The paths were `Chat.vue`, `useThing.ts`, `RolesView.test.ts`, `Button.vue` -- ordinary names in ordinary directories. #17616 created `autobot-slm-frontend/src/views/RolesView.test.ts` and `.../composables/usePerformanceMonitoring.test.ts` for entirely unrelated reasons, and two fixtures became real. That breaks this guard AND pushes the uncovered-reads record from 45 to 47. MAIN IS CARRYING THIS BREAK RIGHT NOW AND ITS BADGE IS GREEN. #17616 changed only `autobot-slm-frontend/**`, which the python-paths filter does not cover, so `python-suite` was SKIPPED on `bf6ee5583` -- every shard, reported as `skipped`, run conclusion `success`. The bypass that let it through is the exact one `python_filter_covers_its_guards_test` exists to name: "a change confined to one takes the required-context shim's green while the guard never runs." The guard predicted the hole, the change went through the hole, and what it landed on was the guard. Nothing was wrong with either; the two never met, because the suite holding one of them did not run. Fixed here rather than in a new PR because this PR was already red on it and unblocking base is worth more than tidy attribution. - Every fixture path now carries the reserved `__prepush_fixture__` segment. Project prefix and extension are unchanged, because those are what the hook's filter routes on; only the basename moves. - `test_every_fixture_path_is_reserved_by_name` makes not-resolving a property of the NAME. `test_no_fixture_path_names_a_real_file` checks the tree as it stands, which is only ever a statement about the present -- it passed for months and then someone added a file. The reserved segment is independent of what anyone adds next. - The composable-sibling test builds its sibling under `tmp_path`, so its name tracks the input for matching only; nothing there touches the real tree. Also: two reach floors, MEASURED in this tree rather than copied from #17610 which ratchets the same two: blanket-skip-test-module-sweep 2540 -> 2741 test modules collectable-modules-inert-on-import 2527 -> 2728 matching modules Ratchet baselines are hub files and exempt from one-PR-one-owner; whichever of the two PRs merges second resolves a one-line conflict. Verified: 318 passed, 0 failed across prepush_frontend_projects_17574_test.py, python_filter_covers_its_guards_test.py and reach_declarations_test.py. Refs #17571 * fix(hooks): data/ is application data, so the print()/console guard stops scanning it (#17623) * fix: restore the last two deleted files — the guard's domain was wrong, not its rule (#17623, #17571) Completes the #17622 restore: 34 of 34 back. 33 are byte-identical to `origin/main`; `data/file_manager_root/test.js` differs by one trailing newline, which the `fix end of files` pre-commit hook adds and which I am not going to bypass a hook to avoid. Saying "byte-identical" of all 34 would have been the easier sentence and a false one. The two upload fixtures could not be re-committed because the `No print()/console.* in Production Code (#1082)` hook refused them. I read that as a policy question for a human -- `# noqa` markers that would change fixtures which must match base, or a path allowlist that the hook's own #16008 comments argue against. Both of those were the wrong framing. The rule is right; the DOMAIN is wrong. `data/` is application data, not production code -- `file_manager_root` is a registered data path via `PathsManager.get_data_path("file_manager_root")` (`paths_manager_test.py:49`). The whole tracked tree holds exactly two source-like files under `data/`: $ git ls-files 'data/**/*.py' 'data/**/*.ts' 'data/**/*.vue' 'data/**/*.js' data/file_manager_root/test.js data/file_manager_root/test.py Both one-line upload fixtures whose `print("Hello World")` and `console.log("test")` ARE the sample being uploaded. This is not the exemption #16008 removed. That one was invisible -- it lived in the gap between the scan's scope and nobody editing those lines. This excludes a DIRECTORY for what it is, beside `autobot-infrastructure/`, `code_analysis/`, `ansible/` and `tools/lint/`, which the same two greps already exclude with their reasons inline; and it hides nothing, because those two files are all there is to hide. Credit to f8 for the reframing. Two things worth recording for whoever meets this next: * The fixtures miss every filename exemption by one character. `test.py` is not `_test.py` and not `test_*.py`; `test.js` is not `*-test.js`. * Re-adding them needed `git add -f`. They are `.gitignore`-matched, and once the deletion landed they stopped being tracked, so a plain `add` refuses them. That is #17622's mechanism running backwards: the same ignore rule that let them be deleted without notice now stands in the way of putting them back. RE-MEASURED, and the arithmetic does not close: declared before 301 minus the two fixtures that left the domain -2 expected 299 MEASURED 300 The extra one is not from this branch: no file it touches violates, and the 32 files it restored are yaml/json/config this scan never reads. So base has been one violation short of its own pin, and nothing reported it -- `python-suite` is SKIPPED on any PR touching no python path and the run still says `success` (#16087). That is the third thing today to reach `main` through that gap. Pinned to the measured 300, not to the expected 299. Verified: 86 passed, 0 failed across pre-commit-no-print-console_test.py, python_filter_covers_its_guards_test.py, glob_declared_reads_15900_test.py and prepush_frontend_projects_17574_test.py -- the last two because changing a scanned domain can move another guard's population, which it did not. Closes #17623 Refs #17622 Refs #17571 * fix(canonical): three review findings, all the defect this harness exists to catch (#17571) Every one of them is a clean result standing in for an inspection that did not happen, inside the harness built to make that impossible. A fourth turned up while fixing them, and it is the worst of the four. 1. ds_unloaded_authority.check() SWALLOWED ITS OWN READ FAILURE. `readFile` throwing returned `[]`, which means "this stylesheet is fine" -- for a stylesheet nobody read. The two traversal paths a few lines up already throw for exactly this condition; the function that does the actual reading did not. It throws now. 2. THREE TESTS SKIPPED THEMSELVES IF THE EXPORT VANISHED. `if (!isImportedForTest) return;` meant that dropping the export would make all three pass without creating a fixture or resolving a single specifier -- a green suite over an uninspected rule. Asserted now, not guarded. 3. THE RESERVED-NAME CHECK HAD NO CONTRAST CASE. It only ever sees paths this file chose, so it would pass identically if the rule were `assert True`. Added the name that actually broke things -- the real `RolesView.test.ts` #17616 created -- so the assertion can fail. 4. AND THE ONE I FOUND WHILE VERIFYING: `runner.test.mjs` HAS NEVER RUN. It imports from vitest and asserts on the harness, but vitest's `include` was `src/**` only and the file lives under `scripts/`: $ npx vitest run scripts/canonical/__tests__/runner.test.mjs No test files found, exiting with code 1 198 lines of tests that read as coverage in a diff and were executed by no job, local or CI. I had run them earlier by another route and recorded "22 tests pass", which is true and was never a statement about CI. `include` now also matches `scripts/**`; one file matches it today, and the point is that the next one is collected rather than ignored. Findings 1-3 from review on #17608. Finding 4 is the same family one level out: 1 is a rule reporting clean without reading, 4 is a suite reporting nothing without running. Verified: 22 passed in runner.test.mjs -- now through the config rather than around it -- and 36 passed in prepush_frontend_projects_17574_test.py. Refs #17571 * fix(repo_tests): the contrast case put the defect back that its own rule removes (#17571) CI red on `python_filter_covers_its_guards_test.py`, both tests, and the cause was the fix I wrote for review thread 3 on this PR. That thread was right: `test_every_fixture_path_is_reserved_by_name` only ever saw paths this file chose, so it would pass identically if the rule were `assert True`. I added a contrast case, and reached for the most rhetorically satisfying string available -- the real path that broke things: assert "__prepush_fixture__" not in "autobot-slm-frontend/src/views/RolesView.test.ts" A quoted path that RESOLVES is exactly what `python_filter_covers_its_guards` records as a tree this guard reads. #17616 created that file, so the literal resolves, so the uncovered record went back up -- 45 to 46, one short of the 47 the original break caused. The contrast case for the sentinel rule reintroduced the defect the sentinel rule exists to remove, in the same commit that introduced the rule. The real name now stays in prose, unquoted, and the assertion uses a path that resolves to nothing. The property under test is whether a name carries the sentinel; it never needed the name to be real. That was the whole point of the sentinel and I did not apply it to my own new line. MERGE RESOLUTION. #17615 landed its own repair of the same file, renaming the two slm fixtures to `ExampleOnlyView.test.ts` / `useExampleOnly.test.ts`. Resolved toward the sentinel, which is a difference in kind rather than degree: `ExampleOnly` is a name nobody has created YET and a sentinel cannot ever be a real path. All six fixtures carry it, not just the two that happened to break. FLOORS: MEASURED, NOT RECONCILED, AND NOT CHANGED. blanket-skip-test-module-sweep population 2744, floor 2743, headroom 1 collectable-modules-inert-on-import population 2731, floor 2730, headroom 1 allowance 200 on both Taken from `len(REACH.discover())` in each declaration, not from a grep. #17615 merged 2743/2730 into base measured against a tree with its two new modules; this branch adds one more, so each population sits one above base's floor and 200 below the allowance. The floors already cover it, so they are left alone -- raising them would be churn that collides with every other branch measuring the same pair. My earlier 2741/2728 described a tree that no longer exists. Verified: 49 passed, 0 failed across python_filter_covers_its_guards_test.py and prepush_frontend_projects_17574_test.py; reach_declarations_test.py green on both declarations. Refs #17571 --------- Co-authored-by: t <t@example.invalid> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
bsidian graph
…guard that could not see them (#17646) (#17647) * fix(async): four production blocking calls off the event loop, and a guard that could not see them (#17646) `tools/lint/check_no_blocking_io_in_async.py` knew two vocabularies, `requests.*` and `Path.read/write_text/bytes`, and passed clean over every file in this diff. The codebase-analytics index reported 8 high-severity `performance_blocking_io_in_async` findings in those same approved files. Four production sites, all on the event loop: - the VNC quality-metrics endpoint held three blocking probes -- a `pgrep` at `timeout=5`, a socket connect at `settimeout(2)`, another `pgrep` at `timeout=5` -- so one request could stall every other request for ~12s - the VNC clipboard endpoint ran `Popen(...).communicate(timeout=5)` - the SLM agent's event sync ran three `sqlite3.connect` calls and held one connection open across an `await` on a 30-second POST, so a slow admin held both the loop and a database handle for the whole timeout Both endpoints' blocking work moves into `api/vnc_blocking_probes.py`, whose functions block by contract and import no `asyncio`; the agent's moves into `slm/agent/event_buffer.py`, where a connection cannot outlive one call and so cannot be held across an await. Both host files sat at their frozen size ceiling, so this splits rather than raising it: vnc_manager 1699 -> 1653, agent.py 675 -> 631, and both baselines ratchet down. The guard now matches `subprocess.run/call/check_call/check_output/Popen`, `sqlite3.connect`, `time.sleep`, `os.system` on `(module, attr)` -- a bare `engine.connect()` or `task.run()` stays clean, which is what keeps this a gate rather than a nuisance. The stdlib group is not enforced in test files, where a blocked loop is the test's own; that narrowing is *counted and printed*, because a silent exemption and a clean tree read identically. Production stdlib count is now 0 of 0; the 192 remaining are the pre-existing `Path.*` backlog, unchanged. What the widening does NOT reach, recorded at both sites rather than left implicit: `is_vnc_running` is a *sync* function called from async, and the visitor resets its async depth inside `def` bodies -- correctly, since a sync helper may have sync callers. Blocking work one frame below an `async def` is invisible however wide the call list gets. That needs reachability, not matching. Found while moving the agent code: the buffer prune reached a `@staticmethod` whose `db_path` defaulted to `DEFAULT_BUFFER_DB` and was invoked with no argument, so an agent with a configured `buffer_db` pruned the default file and let its own buffer grow past the cap unbounded. Prune now takes the path with no default, proved by a two-database test. Tests: 24 new guard tests (each stdlib kind flagged in async, ignored in sync, ignored one frame down in a sync helper, the module qualifier, the noqa, and all three test-path spellings) and 9 event-buffer tests. Mutation-proved: dropping `Popen` from the table, matching on attr alone, narrowing the test-path predicate to `*_test.py`, relaxing `requests.*` into the exempt set, restoring the prune default, and dropping the empty-ids guard each fail a named test. The empty-ids test watches `sqlite3.connect` rather than the row count, because SQLite accepts `IN ()` and a row-count assertion passes with the guard removed. Closes #17646 * fix(async): resolve the receiver binding, and unblock the two buffer paths a call-site never reached (#17646) Three Major findings from the #17647 review. All three confirmed against the code, none dismissed. 1. The widened matcher compared the *name* `subprocess` to a literal, not the module. `import subprocess as sp` then `sp.run(...)` in an `async def` walked straight through the guard that exists to catch it, and an argument named `time` with a `sleep` method was falsely flagged. That is the same "detector sees less than it names" class this PR documents, introduced by the widening itself. `imported_modules()` now maps every name an `import` binds -- including aliases, and including function-local imports, where a blocking call tends to be written -- and Pattern 3 resolves through that map before it classifies. A receiver the file never imported is not a module and is skipped, which removes the false positive by construction rather than by exception. Re-measured after the change: the tree is unchanged at 192 + 18, so no aliased blocking call exists today -- a measurement, not an assumption. 2. `buffer_event` was called inline from the `aiohttp.ClientError` handler in `async def _send_heartbeat_request`: a synchronous sqlite insert on the event loop, on the buffer's hot path, running on *every* failed heartbeat -- that is, exactly when the admin is unreachable. Offloaded at the async entry point, not inside `buffer_event`, because `_process_code_change` is a sync caller and must stay one. This site is the blind spot this PR describes, found by review rather than by the guard, which is the evidence that widening a call list does not reach it. 3. `MAX_BUFFERED_EVENTS` was enforced at the top of `sync_buffered_events`, and the run loop calls that only `if success`. So during an admin outage every failed heartbeat appended a row and nothing ever trimmed: the cap was dead in the one situation it exists for. Prune moves to `prune_event_buffer()`, called unconditionally each cycle; only the upload stays gated on connectivity. Not enforced inside `append` as suggested, because that puts a `COUNT(*)` on every insert and gives the cap two enforcement points that can disagree -- the reasoning is recorded on `event_buffer.prune`. Tests: 8 binding-resolution tests (each kind under an alias, a local shadowing a module name, a function-local import, a dotted import binding its root, and the map itself) and 4 call-site tests over both agent copies. The call-site pair is asserted against the AST on purpose: a behavioural test passes either way, since prune trims correctly whether or not the loop reaches it and the insert succeeds whether or not the loop was free. Reachability and scheduling live at the call site, so that is where they are pinned. Mutation-proved: reverting to literal-name matching fails 5 tests; re-gating the prune under `if success` fails its test in the copy mutated; calling `buffer_event` inline again fails its test. agent.py 631 -> 630, re-pinned in all three baseline entries (canonical, ansible mirror, and vnc_manager unchanged at 1651). Mirror pairs 15/15 byte-identical. Refs #17646 * fix(async): offload buffer_event's second async caller and the two bare VNC probes (#17646) Review found `buffer_event` has **two** async callers and the previous push offloaded one. `handle_code_change` -- an aiohttp request handler -- called `_process_code_change` bare, which writes a version file, clears a cache and appends a row through `sqlite3.connect`. A fix that covers one of two paths is not a fix, and this one was reachable from an HTTP endpoint. The hop wraps the whole of `_process_code_change` rather than the `buffer_event` call inside it: three wrappers buy nothing over one for three sequential blocking operations, the same reasoning the quality endpoint uses. `buffer_event` itself stays synchronous because `_process_code_change` is a synchronous caller, and the rule now lives on `event_buffer.append` where both paths can see it. Two more sites of the same shape in `vnc_manager.py`, in the file this PR already owns: `get_vnc_status` and `ensure_vnc_running` called `is_vnc_running()` bare -- a `pgrep` at `timeout=5` on the event loop -- while the quality endpoint and the clipboard endpoint wrapped theirs. Both now offload. That is three of one defect fixed once in one place rather than rediscovered per endpoint. Also corrected, because this PR's own argument applies to it: the `sync_buffered_events` docstring opened "Every sqlite call goes through `asyncio.to_thread`", which was true of the method and false of the module while the code-change path existed. A sentence that reads as a module-wide guarantee while one path bypasses it is the same failure as a guard reporting clean over part of its class. It now says what it covers, and after this commit the module-wide form is true. Tests: the agent call-site pin extends to both async callers, and a new `vnc_blocking_probes_call_sites_test.py` asserts that no `async def` in `vnc_manager.py` reaches any of the three sync probes without a `to_thread` hop -- with a companion test that fails if the probes stop being referenced at all, so an empty result cannot read as a clean one. Both pins are AST assertions on purpose: a bare `is_vnc_running()` returns the same boolean as an offloaded one, and the code-change insert succeeds either way. What differs is whether the loop was free while it happened, and only the call site records that. This is also the instrument the widened guard cannot be: these are sync functions reached from async, one frame below the depth reset. Mutation-proved: un-offloading either `is_vnc_running` caller fails the vnc pin; un-offloading `_process_code_change` fails the agent pin. agent.py stays at its 630 pin -- the reasoning moved into `event_buffer`, which is not size-capped, rather than the ceiling moving. vnc_manager unchanged at 1651. Mirror pairs 15/15 byte-identical. Refs #17646 * test(async): the non-vacuity check was itself vacuous (#17646) The companion test added alongside the VNC call-site sweep asked whether each probe's name appeared anywhere in `vnc_manager.py`. An `import` line satisfies that. So with every async call to `is_vnc_running` deleted and only the import left, the check passed while its name promised the probe was reached from async -- which is the exact name-claims-more-than-mechanism defect the sweep it guards exists to prevent, written into the guard for the sweep. Proved rather than argued. Mutating both call sites to constants leaves the name present once, in `from api.vnc_blocking_probes import is_vnc_running`: OLD substring predicate on the mutated file: PASSES name still present only via: ['is_vnc_running,'] NEW predicate: 1 failed, 5 passed It now walks the AST and requires each probe to be *called* from at least one `async def`, offloaded or not, so the sweep above it cannot go quietly vacuous: a probe that is renamed, moved, or no longer reached from async fails this test and the pin gets re-pointed instead of silently satisfied. Worth stating in the file rather than only in the log, because a non-vacuity check written vacuously buys nothing and reads like coverage -- the same reason this PR counts its skipped test-file hits instead of omitting them. Raised in review of #17647 by a peer applying this PR's own standard to its tests. That standard is the right one: a sweep asserting "no bad call sites" is satisfied by a file with no call sites. Refs #17646 * fix(slm): the buffer cap and batch size are configuration, not literals (#17646, #16056) Found by asking which open issues this PR incidentally touches. #16056's AC5 asks for `max_events` to be env-var-backed "consistent with `DEFAULT_BUFFER_DB`", and the extraction had carried both numbers across as bare literals -- so a fleet-wide buffer cap could still only be changed by shipping code. That is the project's no-hardcoding rule, and the violation was introduced by this PR's own new module, not inherited. `SLM_MAX_BUFFERED_EVENTS` and `SLM_SYNC_BATCH_SIZE`, defaults unchanged at 500 and 100, following the `os.getenv` shape `DEFAULT_BUFFER_DB` already uses one file over. This settles the literal and nothing else in #16056: that issue's substance is that a node reports neither its buffer depth nor its cumulative drops, and the SLM persists and surfaces neither. None of that is in scope here and it stays open. Recorded on the constant so the next reader does not mistake one ticked criterion for the issue. Tests: each limit is read from its env var (via a module reload, which is how an import-time `getenv` is observable at all, with a teardown reload so no later test inherits a 7-row cap), and the defaults are pinned unchanged so env-backing cannot quietly alter behaviour for existing nodes. Mutation-proved: reverting either constant to a literal fails its test. Refs #17646 Refs #16056 * fix(slm): a heartbeat timeout skipped the cap, and two pins certified inert fixes (#17646) Round 3 of #17647's review. One Major and two labelled Trivial; the second Trivial is not trivial and the label is the only thing wrong with it. **The cap could still be skipped.** Moving prune out of `if success:` stopped a *rejected* heartbeat from skipping it. A heartbeat that times out raises `asyncio.TimeoutError`, which is not an `aiohttp.ClientError` -- verified against aiohttp 3.14.3, `issubclass(...) == False` -- so it escapes the heartbeat's own handler, lands in the run loop's catch-all, and skips a prune placed after it. Narrower than "an outage" and worth stating precisely: on a timeout the heartbeat path appends nothing either, because `buffer_event` sits in the ClientError handler that also does not run. The growth comes from the other path -- `handle_code_change`, the HTTP endpoint, appends on every git-hook post independently of heartbeats. So the failing case is a node whose admin times out while commits keep arriving, and it grows unbounded. Prune now runs first in the cycle, ahead of anything that can raise past it. `finally` was the suggestion and would also survive the timeout, but a prune raising inside a `finally` escapes the catch-all that exists to keep the loop alive (#9965); ordering costs nothing and cannot fail that way. **Two pins that passed on fixes doing nothing.** `asyncio.to_thread(f)` without `await` returns a coroutine that never runs, so `f` does not execute at all. Both offload assertions checked only that the callable was *passed* to `to_thread`. Proved rather than argued -- mutate the `await` away and both original assertions still pass: CURRENT pin on an inert (unawaited) fix: no-direct-call assertion: passes handed-to-to_thread assertion: passes => PIN PASSES ON A NO-OP FIX Worse than a missing pin: it would have certified a change that silently stopped processing code-change events altogether. Both now require the `to_thread` call to be the operand of an `await`. The prune pin asserted unconditional, which is not per-cycle: a call placed before `while self.running` satisfied it while pruning once at startup. It now requires containment in the loop *and* precedence over the heartbeat. That makes three pins in this PR that claimed more than they enforced -- the substring companion, and these two -- in the PR whose subject is detectors that do exactly that. Each was found by someone applying the standard back to my tests, which is the argument for the standard. Mutation-proved, each failing a named test: prune after the heartbeat, prune once at startup, and an unawaited `to_thread`. agent.py stays at its 630 pin; the timeout reasoning lives on `event_buffer.prune`, which is not size-capped. Mirror pairs 15/15 byte-identical. Refs #17646 * test(vnc): the probe pin caught one of three defective forms, and its exclusion hid the worst (#17646) Round four strengthened the agent offload pin and left its sibling untouched. `ast.Await` appeared zero times in `vnc_blocking_probes_call_sites_test.py`, so the fix covered one of the two files it already spanned. Checked against every form rather than against the code it was written for: 1 await to_thread(probe) [correct] passes correct 2 bare probe() [blocks] FLAGGED correct 3 to_thread(probe) unawaited [never runs] passes MISS 4 await to_thread(probe()) [blocks on loop] passes MISS Form 4 is the shape `check_no_blocking_io_in_async_test.py` has an explicit test for under `Path.read_text()`. So this PR contained a test for that shape in one file and a pin concealing it in another. The concealment was mechanical, not incidental. `_bare_calls` built an exclusion set from `id(arg)` over every `to_thread` argument, so a probe handed off would not also count as called: - correct form `to_thread(probe)` -- the argument is an `ast.Name`, no `Call` node exists, the exclusion never fires. Dead. - defective form `to_thread(probe())` -- the argument **is** a `Call`, lands in the exclusion, and is hidden. Its only live effect was to hide the one shape where the blocking work runs on the loop. Removed rather than corrected: the correct form passes a function object and produces no `Call` node for the probe, so any `Call` to a probe in async context is a violation and no exception is needed. Nested sync helpers are excluded by walking around nested `def` bodies, mirroring the depth reset in the guard this pin compensates for -- and that shape is now tested too, since it must keep passing or the canonical fix cannot be written. Three sweeps now, one per defect: executed on the loop (forms 2 and 4), offloaded but never awaited (form 3), and a non-vacuity check across all three so an empty file cannot read as a clean one. Verified against all five forms: 2, 3 and 4 flagged; 1 and 5 pass. This is the third pin in this PR to claim more than it enforced. All three were found by a reviewer applying the PR's own standard to its tests, none by a tool, which is the finding worth more than the individual pins. Refs #17646 * fix(slm): env-backing must not be a bare cast, and a stale comment cost three diagnoses (#17646) CI red at `d58af1f124f3`, both failures mine and one cause: the env-backing two commits ago used `int(os.getenv(...))`, which `repo_tests/env_var_bare_cast_test.py` forbids and its shrink-only ceiling counted (7 -> 11). The guard is right and the reason is worse for an agent than for a service: a bare cast raises `ValueError` at **import**, so a typo in one node's environment stops that agent from starting rather than degrading one setting. I introduced an import-time crash while satisfying the no-hardcoding rule. Both now go through `env_utils.env_int_clamped`, which also fixes something the literals never had: `min_v=1`. A cap of 0 deletes every row on each prune, a negative cap makes `count - max_events` exceed the table, and a batch size of 0 reads nothing forever while the buffer fills. The clamp is the substantive part; satisfying the guard is a side effect. I did not raise the ceiling or add an allowlist entry. A ratchet that only shrinks does not get widened to admit new work -- the same rule this PR applied twice to the file-size pins. Also in this commit, and the reason it is worth appending rather than filing: `autobot-backend/utils/chromadb_client.py:46` read "Host remains os.getenv-based: empty string = use local PersistentClient (dev mode)" directly above `_CHROMADB_HOST = _ssot_config.vm.chromadb`, whose default is `127.0.0.1`. The comment described an earlier revision and inverted the default. Three sessions diagnosing empty analytics panels today read it, observed that no host appears in any config file, and concluded the backend was opening an empty local store. The backend has been on the HTTP client throughout -- the deployed log says `ChromaDB HTTP client connected to 127.0.0.1:8100`. A stale comment about a mechanism outranks the mechanism for anyone who stops at the comment, which is the same failure as a guard whose name claims more than it enforces, in prose. Tests: a malformed value falls back instead of raising at import, and `0` clamps to 1, both per limit. Mutation-proved -- restoring the bare cast fails the local pins *and* the repo guard, which is the pairing worth having: the guard catches the class anywhere, the pin catches it here with the reason attached. Refs #17646 * fix(async): from-imports bypassed the guard, a prune fault silenced the node, and two matchers were untested (#17646) Round five of #17647's review: six threads, four real, two already fixed by the previous push. **A prune fault now costs the heartbeat.** Round four moved the prune ahead of the heartbeat so a timeout could not skip it, and the run loop's catch-all wraps both -- so an unhandled buffer fault (full disk, corrupt sqlite) skipped health reporting and a node went dark while otherwise healthy. I traded one silent failure for another. `prune_event_buffer` now swallows and logs its own failure, which makes *both* orderings safe rather than picking the lesser one; `finally` was rejected for the mirror-image reason and this is why neither position was the answer. **`from time import sleep` walked through the widened guard.** Those calls are `ast.Name` nodes and `_maybe_record` returns early on anything that is not an `ast.Attribute`, so the module-qualified matcher could not see them at all. The alias fix two pushes ago covered `import x as y` and left `from x import y` -- one spelling of the class it named, which is this hook's own subject matter one level up. `imported_callables()` resolves those bindings to `(module, attr)` pairs, so `from subprocess import run as child_run` is caught and an unrelated local `run()` still is not. Re-measured: 192 + 18, unchanged -- this hole was empty too, so it is a proven-safe closure rather than a migration. **Two matchers accepted the target in any argument position.** `to_thread(func, *args)` calls only its first argument, so `await asyncio.to_thread(lambda unused: None, self.buffer_event)` satisfied both offload pins while never calling `buffer_event`. Fixed in the agent pin and the VNC pin. **And the matchers themselves had no tests**, which is why that mattered: `_offloaded` was widened twice on this PR -- awaited-only, then first-argument-only -- and each time I changed the matcher without pinning the property, so reverting either widening passed the whole suite. I found this by mutation, after fixing the finding: the fix was in place and the mutation still passed. Both matchers are now asserted directly against the shapes they must separate. **Test isolation.** The env teardown did `delenv` + reload, so on a machine that already sets a limit the module kept the default while pytest restored the variable, and every later test saw a module disagreeing with its environment. The helper now restores the original value; the default-value assertion skips where a limit is configured, because an environment that sets one cannot answer a question about the default. The first version of that helper returned the *module* rather than the value -- and `importlib.reload` returns the same object it mutates, so the teardown reload rewrote what the assertion then read. Mutation-proved, each failing a named test: prune propagating again, dropping from-import resolution, and either matcher accepting a later argument. agent.py stays at its 630 pin -- the round-five reasoning moved into `event_buffer`, which is not size-capped. Mirror 15/15. Refs #17646 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…sm (#17365) (#17664) * test(frontend): wait for the emission instead of assuming one tick (#17365) #17365 reports two tests failing only under `--coverage`, and attributes it to v8 instrumentation slowing those two assertions. Measured before fixing, and both halves of that are wrong in ways that change the fix. THE NAMED TESTS PASS UNDER COVERAGE IN ISOLATION. vitest run --coverage <the two named files> 14 passed So instrumentation-on-those-files is not the variable. THE FULL SUITE UNDER COVERAGE FAILED A THIRD TEST, not either named one: Tests 1 failed | 3417 passed | 1 skipped FAIL WorkflowCanvas.contextMenu.test.ts > re-anchors and re-targets when opened again on a different node AssertionError: expected [ 'n1' ] to deeply equal [ 'n2' ] `n1` is the PREVIOUS emission. The menu had re-anchored to `n2`, but `.at(-1)` read the old value because the new one had not happened yet. Nothing about the assertion is wrong; it was made too early. So the MECHANISM in the issue is right -- an assertion racing the component's reaction -- and the SCOPE is not. It is not those two tests. It is whichever timing-sensitive assertion loses the race in a given run, which is why #10323 fixed this once per-test and it came back in different places. WHAT CHANGED. `await trigger(...)` and `firePointer` each flush exactly ONE Vue tick. Every assertion that reads `emitted()` for its LATEST value straight afterwards is racing. There are eight such assertions in `components/workflow/__tests__` (of 64 emission assertions across the frontend suite; the other 56 assert that something was emitted at all, which does not race the same way). All eight now wait for the emission first. `emitTestUtils.ts` waits on TICKS, not on time, and that distinction is the point: a `setTimeout` or a fixed delay trades one race for another -- passing on an idle machine and failing on a loaded one, which is the behaviour being removed. Fifty microtask flushes cost microseconds when idle and are still fifty under load. The bound exists to fail a broken component, not to bound a slow one. The helper is itself tested, 8 cases, including that it THROWS when the state never arrives. Without that, a helper returning immediately would leave every call site passing exactly as today and the flake would return with no failing test to say the guard had stopped guarding. WHAT THIS DOES NOT FIX, and cannot. One of the two originally-named tests is a NEGATIVE assertion -- "does not refetch and keeps showing the pre-edit name". There is no state to wait for, and waiting longer makes a negative assertion MORE likely to fail, not less. That half of the class needs a deterministic flush and then an assertion, which is a different technique; left alone and recorded on the issue rather than papered over. A PASS IS NOT PROOF HERE. The flake is load-dependent and rare, so a green run does not demonstrate it is gone. What is demonstrated: the racing construct is absent from all eight sites, and the helper behaves under test. I would rather state that than claim a fix I cannot evidence. the three affected suites 31 passed emitTestUtils.test.ts 8 passed Refs #17365, #10323 * test(frontend): fix the two tests #17365 actually names, and share the mechanism (#17365) The branch carrying this had one commit from 2026-09-25 and no PR. What it contained was good and did not close the issue: it built the `emitTestUtils` helper and applied it to `WorkflowCanvas.contextMenu`, `.marquee` and `.undoRedo` -- three tests the issue does not mention -- while BOTH tests it does name were untouched. WorkflowCanvas.panClick.test.ts > does not swallow the click after a pan OrgChart.contactEdit.test.ts > does not refetch and keeps showing ... That helper's own docstring says why it stopped there: a negative assertion has no state to wait for, and waiting longer makes a false pass MORE likely rather than less. Both named tests read as negatives, so both were left. One of them is not actually a negative, and the other has a positive hiding inside it. PANCLICK IS A POSITIVE WEARING A NEGATIVE TITLE. "does not swallow the click" asserts `expect(selections(w)).toEqual([['ceo']])` -- an emission that must HAVE happened. `waitForEmitCount(w, 'node-selected', 1)` applies directly. The assertion is not weakened: the count is still exactly one, and a component that genuinely swallows the click never reaches it and fails on the bound. ORGCHART IS A REAL NEGATIVE, SO ANCHOR IT. Its two assertions fail for opposite reasons when the flush is short: the error text is a positive that has not rendered yet, and `involvedCallCount()` is a negative that would PASS wrongly if a refetch were still in flight. Waiting cannot fix the second -- but the first is the signal that the failure path RAN TO COMPLETION. Wait for the rendered error, and "no refetch happened" becomes a settled fact about a finished path rather than a race against an unfinished one. That pattern is the structural half the issue asks about (ask 2), so it does not live in one view's test folder. `waitForTicks` moves to `src/test/utils/waitForState.ts` alongside a named `waitForSettled`, and `emitTestUtils` re-exports it so the three tests already using it are unchanged. Fixing each instance where it appeared is what let the class recur through #10323, then #17365 in two more places. Ticks and not `vi.waitFor`: that, `setTimeout` and a fixed delay are all interval-based, so they pass on an idle machine and fail on a loaded one, which is the property being removed. Flushing microtasks is load-independent. VERIFIED UNDER THE CONDITION THAT FAILS, not just under the tests in isolation: npm run test:coverage -> 305 files, 3467 passed, 1 skipped, 0 FAILED The issue reported 2 failed / 3408 passed for this same command. One thing that run also exposed, filed as #17638 rather than fixed here: it exits 1 with zero failing tests, because `MockWebSocket` leaks its open-timer past teardown and vitest counts the resulting `ReferenceError: WebSocket is not defined` as `Errors 1 error`. Same class -- scheduling decides the outcome -- different file and different mechanism, and it is a shared mock many suites use. Not caused by this change, which touches nothing WebSocket-related. Refs #17365 * fix(test): drop the unused import that red-lit two checks, and put it back under the header (#17365) `oxlint . -D correctness` failed before vitest started, so no test ran: WorkflowCanvas.contextMenu.test.ts:1:10 Identifier 'waitForEmitCount' is imported but never used. Left over from migrating this file to `emitTestUtils`: `waitForEmitCount` occurs exactly once in the file, the import itself. One cause, two red checks -- `frontend-tests` and `Unit & Integration Tests` both run the lint step. Also moved the import BELOW the licence header. It was at line 1, above the copyright, unlike every other file in the tree -- the signature of an automated edit inserting at line 1. 87's observation is why it survived review: nobody reads line 1 when the file appears to start at line 2. Checked the other two files the same migration touched; both have the header first, so this was isolated. Not the #17365 flake class, on three grounds: no test executed, the shape is a static diagnostic at a fixed line, and it fails identically every run. Verified: oxlint clean, and 16 passed on three consecutive runs of this file. The first of those runs failed once on `Cannot call trigger on an empty DOMWrapper` -- the flake class, in a single-file run, which is worth recording because I had previously only observed it in the full suite. Refs #17365 * fix(test): the timeout diagnostic reported the state from before the wait (#17365) Review on #17664, and it is a real defect in the helper this PR adds. Both messages were plain template literals, so they were built at the call to `waitForTicks` -- before the first tick. When an emission changes during polling but never matches, the timeout reported the payload present BEFORE the wait rather than the last one actually seen. These helpers exist to make an instrumentation-timing failure legible. A diagnostic that describes the pre-wait state names the one moment that is never the interesting one -- the same shape as the class this PR is about, in the tool built to explain it. `waitForTicks` now takes `string | (() => string)` and resolves it at throw time. Plain strings still work, so no caller is forced to change; the two emit helpers pass thunks. MY FIRST TEST FOR THIS WAS VACUOUS, and the mutation run is the only reason I know. Its stub pushed a new emission on EVERY read, so the eager build also observed a changed value and the test passed against the bug it targeted. The stub now holds its first answer stable and changes only afterwards, which is what makes eager and lazy distinguishable at all. Verified: baseline 10 passed; reverting the `waitForLastEmit` thunk to a plain template literal fails exactly 1 of 10 and names it; restore clean. vue-tsc 0 errors, oxlint clean. Refs #17365 * fix(test): the late-emission fixture made its own clock, so it proved nothing (#17365) The second review thread on #17664, filed Trivial. It is not trivial — it is the same vacuity as the diagnostic test in the commit before this one, in the fixture that one inherited. `lateEmitter.wrapper.emitted()` incremented `ticks` and pushed the emission during each READ, and `advance` -- the function that would have driven the clock independently -- was never called. Zero occurrences. So reading was what made time pass. A `waitForTicks` implemented as a tight loop calling `emitted()` with no `await nextTick()` would have reached the emission and passed, because the poll IS the clock. The test claimed to prove the helper yields to `nextTick` and could not distinguish yielding from spinning. Reads are pure now, and `advance()` runs concurrently on real ticks. If the helper does not yield, `advance` never progresses and the helper times out. Verified by mutation, which is the only thing that could have shown this: removing `await nextTick()` from `waitForTicks` now fails 1 of 10. Against the old fixture the same mutation passed. That is two vacuous tests in this file in one evening, found by two different routes -- one by my own mutation run, one by a reviewer. Both were correct assertions in a world constructed so the answer could not differ. Reading the assertion finds neither. Refs #17365
…#17666) * fix(workflows): record the owner, then scope control to them (#17014) `ActiveWorkflow.owner_id` and the WebSocket's `may_control` both existed and both were inert: no creation path set an owner, so every workflow answered `owner_id is None`. An owner check against None admits nobody, which made the WebSocket admin-only, while REST `control_workflow` had no check at all -- any signed-in user could pause, cancel or approve any workflow. Recording and checking now share one identity function, because a check that compares a different claim than the one recorded silently never matches. - `workflow_ownership.py` (new): `caller_id`, `is_owner_or_admin`, `require_owner_or_admin`, `find_workflow`. An absent owner means admin-only, not open -- fail closed. - Owner recorded on every creation path that has a caller: the REST route, chat-derived (via the manager), templates, and the advanced-workflow coordinator. `workflow_serializer` already passed one. - Scoped to owner-or-admin: `control_workflow`, `start_workflow`, `workflow_status`, `present_plan`, `approve_plan`, `pending_approval`. - `approve_plan`'s docstring claimed authorization "by checking session_id matches the workflow" -- client-supplied, so it authorised whoever asked, and deferred the real check to a gateway that does not perform it. - Executor no longer resolves secrets under `workflow.session_id`. A session id is not a user id; an unowned workflow now resolves no user secrets rather than another namespace's, and creation logs that it will. - `except HTTPException: raise` on the routes gaining a 403. Without it the route's `except Exception` returned 500 for a refusal -- and already did for the pre-existing 404, so "workflow not found" was an Internal Server Error. - `approval_support.py` (new): `routes.py` was at 599 of its 600-line ceiling, so the two approval helpers moved out rather than the ceiling moving up. 599 -> 573. Refs #17009, #2304, #16628 * fix(workflows): present_plan swallowed its own 403 into a 500 (#17014) Found by enumerating the routes whose `except Exception` converts an `HTTPException` raised in the try body into 500. `present_plan` is one of five, and it is the one this change had just given an ownership check -- so its refusal would have surfaced as "Internal server error". The cross-user test did not cover `/present_plan`, which is why the gap survived the first pass: an unlisted route is an unchecked route. It is in the parametrized list now, and an AST assertion over the file confirms no route carrying an ownership check still swallows its 403. The remaining four swallows are pre-existing and got no 403 from this change, so they stay out of scope and are filed separately. * chore(types): regenerate generated API types from OpenAPI schema(s) Auto-regenerated (py3.14) to match the backend and/or SLM backend schema so the required verify-generated-types gate(s) pass. Triggered by auto-fix-generated-types.yml. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…l queried rows that could not match (#17651) (#17672) * fix(analytics): the index request never sent source_id, so every panel queried rows that could not match (#17651) Thirteen-plus codebase-analytics panels render empty. The data is there, healthy, and unreachable. POST /api/analytics/codebase/index useSourceRegistry.ts:231 body: { root_path } <- no source_id indexing.py:109 if request.source_id: -> falsy, stays None ...threaded correctly as None through three layers... chromadb_storage.py:300 if source_id: metadata["source_id"] = source_id <- OMITTED -> 11,241 rows written with no source_id declarations.py:35 filter {"source_id": source_id} -> 0 rows Every link threads `source_id` correctly. Nothing in the plumbing is broken. The top of the chain never supplied one and the bottom treated its absence as acceptable, so the GUI indexed globally while the panels queried per-source. Measured on the running deployment: `autobot_code` holds 11,241 rows whose complete metadata key set is type, suggestion, severity, problem_type, line_number, file_path, file_category, description. `source_id` is absent from all 11,241. Problems Report has every one of those rows waiting behind a filter that cannot match them. THE FIX 1. `useSourceRegistry.ts` sends `source_id`. It is the ONLY index trigger in the frontend, and `sourceIdQuery` -- the accessor the other #1710 calls already use -- was right there. It yields `{}` when no source is selected, so the root_path-only mode the endpoint supports is unchanged. 2. A contract guard, because this is the FOURTH instance in one day of a consumer requiring what a producer does not emit (#17643 was the third). Both sides are always correct in isolation; the defect lives only in the gap, so no test of either side can see it. THE GUARD HAD THE DEFECT IT CATCHES. Its first version inspected `_prepare_problem_document` and passed, while four sibling preparers -- function, class, import, stats -- carry the identical conditional and were never examined. A guard that names "the writer" and sees one of five is the thing it exists to prevent. It now discovers all five by AST rather than naming one. WHAT I GOT WRONG FIRST, recorded because the wrong version was reported as measured and nearly acted on: I claimed the backend read an empty ChromaDB store. It does not -- `AUTOBOT_CHROMADB_HOST` is unset so the SSOT default 127.0.0.1 applies, and the server log shows live queries against the correct collection. I had misread `config.yaml`'s `memory.chromadb:` -- a nested mapping header -- as `vm.chromadb` with an empty value. Changing that config would have broken a working deployment. Verified: guard baseline 2 passed; mutation removing `source_id` from `_prepare_class_document` fails it and names that function; restore clean. vue-tsc 0 errors, eslint 0 problems. DELIBERATELY NOT IN THIS PR. A runtime warning when an unscoped write happens belongs at `chromadb_storage.py`, and that file is grandfathered at exactly 1470 lines: "a grandfathered file may not grow -- the exemption freezes the size it was granted for" (#14236). Adding 15 lines of safety code took it to 1486 and the guard refused. I am not refactoring an unrelated 1470-line file to make room inside a fix for something else. Filed separately. ALSO NOT FIXED: the 11,241 existing rows stay invisible until re-indexed -- owner has chosen re-index over backfill. The 504 on the call-graph endpoint is unexplained by any of this and stays open. Refs #17651 * test(analytics): tripwire for the metadata builder nobody has written yet (#17651) 87's review. The discovery is exhaustive today and exhaustive BY SPELLING: `startswith("_prepare_") and endswith("_document")` finds functions named a particular way, not functions that build metadata for a store write. 87 checked and the gap is future, not present -- the five it finds are the five that exist. But "the five that exist" is a fact about now, and the guard's own first version failed for exactly this reason at one level down. Added a second discovery that asks what a function DOES: assigns a dict to a variable literally named `metadata`. The two sets must agree. A PROPERTY, not an allowlist. I tried 87's suggested key-based tripwire first -- flag any non-preparer dict containing `file_path` -- and measured it before writing it: it fires on `make_problem_dict` (the upstream problem record) and `_delete_source_documents` (a filter). Both legitimate, so that version needs an allowlist of exceptions, which goes stale exactly like the thing it guards. Binding `metadata` separates them without one: _prepare_problem_document -> metadata (emitter) _prepare_function_document -> metadata (emitter) _prepare_class_document -> metadata (emitter) _prepare_import_document -> metadata (emitter) _prepare_stats_document -> metadata (emitter) _recreate_chromadb_collection -> collection_meta (not a row) make_problem_dict -> no binding (upstream record) _delete_source_documents -> no binding (filter) Verified by planting preparer six rather than by reading: a `_build_widget_metadata` appended to the file fails the new test and NAMES it; baseline 3 passed; restore clean. Refs #17651 * test(analytics): four review findings, and one of them is my own argument aimed at me (#17651) All four Majors on #17672 were right. Detectors extracted to `repo_tests/_analytics_metadata_detect.py` so each can be run against synthetic source, and every one now has a contrast pair. :80 -- DISCOVER FILTER KEYS WITHOUT A FIXED ALLOWLIST. The detector regex-matched dict keys and then intersected with six hardcoded names, so an endpoint filtering on a NEW key had it silently discarded and the contract passed even though no writer emitted it. That is precisely the objection I raised an hour earlier against a key-based tripwire -- "an allowlist that goes stale exactly like the thing it guards" -- and I had written one into the other half of the same file. Keys now come from the filter expressions themselves: dicts assigned to a `*filter*`/`where*` name or passed as `where=`, with the Mongo-style operators excluded as structure rather than field names. :62 -- COLLECT KEYS FROM THE EMITTED `metadata` ONLY. The old collector took every dict key and constant subscript anywhere in a preparer, so a preparer that stopped assigning `source_id` to `metadata` but mentioned the name in some other dict still passed. It now follows the binding: `metadata = {...}`, `metadata["k"] = ...`, `metadata.update({...})`. :110 -- DISCOVER METADATA AT WRITE SITES. A future writer can pass `metadatas=[{"source_id": x}]` inline and escape both name-based and binding-based discovery. Third population added. It is empty today by construction rather than by luck, which is exactly why it needs its own contrast fixture. :114 -- CONTRAST FIXTURES FOR EACH DETECTOR. This is the standard I have been holding others to all evening and had not met here. A detector exercised only against the live tree is exercised against the one input it is guaranteed to agree with; it can be narrowed later with nothing failing. Each detector now has a fixture it must recognise and one it must reject, including a filter key -- tenant_slug -- that nobody enumerated. Verified by mutation, not by reading: planting a "tenant_slug" key into declarations.py's where-filter fails the contract and NAMES the key. The allowlist version discarded it silently. Baseline 7 passed, restore clean. Refs #17651 * test(analytics): the guard examined some readers of the contract it asserts (#17651) e5's review of the older head. `_filtered_keys()` globbed `endpoints/*.py`. The writer's collection is `autobot_code`, and modules outside that directory obtain it too -- `analytics_code.py`, `analytics_debt.py` and `analytics_quality.py` all filter on it. So a guard asserting a writer-reader contract was examining only some readers: the same "sees less than it names" defect it was written to catch, one directory along, and the fifth instance of that shape in a day. The population is now discovered by OBTAINING THE COLLECTION rather than by location. That is also what keeps it honest in the other direction, which is the trap in doing this widening at all: `bug_predictor.py:1181` and `detector.py:1004` both carry `source_id` where-filters and are NOT in scope, because they query `bug_pattern_vectors` and `cross_language_patterns`. `detector.py`'s own comment says its filter "mirrors the dependencies.py source_id where-filter", which is precisely what makes it look in-scope. A `source_id` filter is not evidence of this contract; the collection is. Measured, because a widening that finds nothing is indistinguishable from one that works: 11 reader modules, 4 of them outside `endpoints/`. All their filter keys are already emitted, so there is no live gap -- e5 predicted that and it is now checked rather than assumed. Also verified against e5's other warning: dropping the old regex removed an `[a-z_]+` constraint that silently excluded camelCase and digit-bearing keys. The AST version has no name-shape filter at all -- `filter_keys` on a fixture containing `tenantSlug2` and `snake_key` returns both. Refs #17651 * test(analytics): the vacuity check measured matches, not reach (#17651) Last Major on #17672. `assert filtered` proves the matcher fired somewhere. It does not prove the sweep read anything: a run that reached two files and recognised one filter satisfies it, and reports a clean contract over a population it never opened. That is the repo's reach-floor discipline (RATCHET_BASELINES.md) and this guard did not have it -- in a file whose entire subject is a check that passes while seeing less than it names. Added a floor on FILES REACHED, kept `assert filtered` beside it. They fail for different reasons and neither detects the other: reached >= 17 the sweep collapsed (measured today: 19) assert filtered the matcher went silent across files it did read Floor at 17 against a live 19, matching the growth-allowance convention rather than pinning to the exact count, so adding or removing one reader module does not fail a guard that is not about module count. Verified by mutation: renaming the accessor to `get_code_collection_typo` collapses the sweep to zero modules, and the floor fails with "only 0 modules referencing get_code_collection_typo were read". Under `assert filtered` alone that mutation would have passed -- `filtered` would simply be empty and the old assertion was the only thing looking, which is the exact failure being guarded against. Baseline 7 passed, restore clean. Refs #17651 * test(analytics): the two-population check ran in one direction only (#17651) #17672 review, filed Trivial. `by_behaviour - by_name` catches a metadata builder the naming convention misses. It does not catch the opposite: a named preparer that stops binding a literal dict to `metadata` drops out of `by_behaviour`, the difference stays empty, and the guard passes while that preparer emits nothing this file can see. A one-directional check on two populations is half a check, and the half it omits is the one that goes wrong during ordinary refactoring -- switching `metadata = {...}` to `dict(...)` or building it through a helper is the kind of change nobody would think to re-verify a guard against. Both directions asserted now, with separate messages because they mean different things: one says discovery is too narrow, the other says a discovered function has gone silent. Verified by mutation: rewriting `_prepare_problem_document` to build its metadata with `dict(type="problem", ...)` instead of a literal fails the guard. Before this commit the same mutation passed. Baseline 7 passed, restore clean. Refs #17651
…17667) * fix(kb): run ECL ingestion through the declared sanitizer (#17033) Patch authored by session autobot-ai-87, which wrote the complete change and posted it on #17033 as a reviewable patch but could not push: the freeze lift had reached it only as a relay, and it declined on the principle that a relay cannot carry authority for that -- including a relay asserting the owner named the relayer, since that claim arrives through the channel whose authority is in question. Applied by a session holding the authorisation directly. Its analysis, comments and tests are carried as written except where noted below. The ECL pipeline redacted credentials but reached no injection sanitizer, and the load stage writes straight to the stores (chromadb_loader, sqlite_loader, redis_graph_loader), so nothing on this path passes store_fact's chokepoint at knowledge/facts.py:819. An "ignore previous instructions" in an ingested document landed in ChromaDB and returned through retrieval into a later prompt. The fix calls `sanitize_fact_content` (knowledge/ingest_sanitize.py:96) -- the declared single rule -- rather than its two halves in a different order, so the standalone `redact_content` call is removed rather than joined by a parallel one. `INJECTION_ROUTE` is set explicitly because `_route_of` reads it first; without it the route label falls back to `unspecified` and neither the metric nor the warning says which writer was sanitized. One correction to the patch as posted: its first assertion required the payload to be ABSENT from each chunk. The sanitizer neutralises rather than deletes -- it wraps the span as `[ESCAPED:...]`, which the patch's own prose says and its assertion contradicted. The assertion now mirrors the contract the repo already fixes for this exact payload at knowledge/test_query_sanitizer.py:346: the span is present wrapped, and absent unwrapped. Mutation check, run here because 87 had no branch to run it on: - removing the `sanitize_fact_content` call fails 4 tests -- all three new ones AND the pre-existing credential test, which is the proof 87 asked for that the combined call preserves the credential behaviour - removing only the `INJECTION_ROUTE` line fails exactly one, on the label - restored: 18 passed Refs #17033, #17649, #16770, #13708 * fix(guards): the redaction guard matched a spelling, not the property (#17033) CI caught `kb_content_redaction_chokepoint_guard_test.py ::test_the_ecl_pipeline_redacts_before_the_extract_stage` red on this branch. The guard was a source-text match on one literal call: assert "input_data = redact_content(input_data)" in source It fired on a change that STRENGTHENED the pipeline: #17667 replaced that call with `sanitize_fact_content`, which runs an injection pass and then the same redaction (ingest_sanitize.py:120-122). The guard could not tell a stronger implementation from a removed one. And it could not have caught the failure its own name describes. A source-text match knows nothing about position, so moving the redaction AFTER the extract loop -- the exact regression "redacts before the extract stage" is about -- would have left it passing. Replaced with an AST assertion scoped to `_run_extract_stage`: some call that redacts must reassign `input_data`, and it must be positioned before the loop over extract tasks. The set of redacting primitives stays a named list, because "does this call redact" cannot be answered without following the callee across files -- but it lists *primitives*, not call sites, and the assertion is about position rather than spelling. Mutation-checked, all four: - remove the redacting call -> fails - move it after the extract loop -> fails, naming both line numbers (the old guard PASSED this) - revert to the old `redact_content` -> passes; either spelling is accepted - as committed -> passes Eleventh witness to #15826: a name claiming a property, a mechanism enforcing a spelling. Pointed because this is the redaction chokepoint's own guard. Refs #17033, #15826, #13708 * docs(guards): the primitive list is safe only while the check is positional (#17033) The replacement guard keeps a named set of redacting primitives because "does this call redact" cannot be answered without following the callee across files. That is tolerable for a reason worth writing down: the guard's real claim is POSITIONAL, so the list only answers a narrow sub-question. Strip the before-the-loop assertion and keep the list, and the guard silently becomes a membership test -- "is one of these two names mentioned" -- which is exactly the shape it was rewritten to escape. Refs #17033, #15826 * fix(docs): cite symbols, not line numbers, in the comments this branch adds (#17033) CI red on `comment_line_number_citations_test.py`, and the guard is right. My comment cited `knowledge/ingest_sanitize.py:120`, and its message is the argument I have been making about other people's guards all evening: a line number "is invalidated by edits to code it does not describe, and fails silently INTO whatever now occupies the line rather than failing loudly." Five citations replaced with symbol references across the three files this branch touches -- four of them in comments carried over from 87's patch, one mine. `ingest_sanitize.py:120` -> `sanitize_fact_content`; `facts.py:819` -> `knowledge/facts.py`'s `store_fact`; `test_query_sanitizer.py:346` -> the file; `chromadb_loader.py:96` -> the file. Refs #17033, #15877 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…3 of 4 'Potential Bugs' were documentation (#17668) (#17670) * fix(analytics): the call scanner read JSDoc examples as live calls (#17668) Three of the four findings on the live "Missing Endpoints — Potential Bugs" panel were documentation. Verified line by line before changing anything: useFileDownload.ts:11 inside the /** block opening at 6, closing at 17 FALSE useFileDownload.ts:16 same block, "Or inline:" example FALSE apiErrorHandler.ts:222 inside the ```typescript fence opening at 212 FALSE ThemeManagerView.vue:35 real call, and no backend route matches TRUE The scanner decided "is this a comment?" per line, by prefix: stripped = line.strip() if stripped.startswith("//") or stripped.startswith("/*"): That knows two spellings and the class has at least three. A JSDoc example lives on a `*` continuation line, which starts with neither -- all three false positives. A `//` trailing real code is scanned too, because the line does not *start* with it. And any line inside a `/* */` block that has no leading star. "Is this position inside a comment" is not a property of one line, so a per-line prefix test cannot answer it. `blank_comments` carries block state across lines and replaces comment bodies with **spaces**, so every `file:line` the scanner reports still points at the same place. It lives in its own module because the scanner is at its frozen size ceiling; the scanner shrinks 1445 -> 1444 and both baselines ratchet down. String awareness is the part that is not optional: `'https://example.test'` contains `//`, and a stripper without it blanks the rest of that line -- silencing a real call and reporting nothing, which is the failure direction that looks like success. Vue `<!-- -->` is handled too, since a commented-out template call is the same defect in a different syntax. Verified against the three real files rather than synthetic input: useFileDownload.ts /api/ visible before [11, 16] -> after [] apiErrorHandler.ts /api/ visible before [222] -> after [] ThemeManagerView.vue /api/ visible before [35, 49] -> after [35, 49] The false positives go, the true positive stays. `POST /api/themes` is a theme upload that can only 404 and was previously indistinguishable from noise. Tests are organised by failure *direction*, because only one direction is visible: a false positive shows up as a wrong finding, a false negative as a clean panel. A non-vacuity test fails on an identity implementation, since every "this call survives" assertion passes when nothing is blanked at all. Mutation-proved, each verified to have applied before its result was read: identity fails 6, deleting rather than blanking fails 5, dropping string awareness fails 1, not entering block state fails 4. Two of those first read as "not caught" because a shell heredoc mangled the replacement and the edit never landed -- a mutation that did not apply is not evidence that a test is weak, and the harness now asserts the file changed before running anything. `blank_comments` is split into small state consumers rather than one loop: the function-length gate caught it at 67 body lines, and the split is better anyway -- each consumer answers one question and the driver is a five-branch dispatch. One limit stated in the module rather than left to be rediscovered: a JS regex literal containing a slash pair is not distinguished from a line comment. The scanner's input is API-path string literals where that shape does not arise, and the fix would be a tokenizer rather than another special case. Closes #17668 * test(analytics): settle the fourth finding with the scanner, not a proxy (#17668) Review condition on #17670: the PR showed three findings silenced and argued the fourth survives from structural proxies -- "code after a closed block is still scanned", "a plain call is untouched". Those show the mechanism is *capable* of keeping a call, not that it kept *this* call. The corollary is what makes it non-optional: **a fix that silenced all four would look identical to a correct one** if only the three were checked. So this runs `FrontendAPICallScanner._parse_api_call` over the same three files with comments intact and with them blanked, and asserts both directions: useFileDownload.ts before [(11, GET /api/export), (16, GET /api/export)] after [] apiErrorHandler.ts before [(222, GET /api/users)] after [] ThemeManagerView.vue before [(35, POST /api/themes), after [(35, POST /api/themes), (49, DELETE /api/themes/${id})] (49, DELETE /api/themes/${id})] The real finding is named rather than implied: **`POST /api/themes` at `ThemeManagerView.vue:35`**, a theme upload with no matching backend route. A second real call at `:49` (`DELETE /api/themes/${id}`) survives too and was not in the panel's four -- worth noting, since it shares the missing route. Three tests, one per direction plus one against vacuity: if the pre-fix scan found nothing, every "absent after" assertion would be trivially satisfied, so the comparison asserts at least four pre-fix findings exist. Mutation-proved by the mutation that matters here -- making the blanker blank *code* as well as comments, i.e. the fix that silences everything -- fails 2 of 5. That is the assertion the proxies could not make. The pre-fix side is reconstructed by scanning with comments left intact, which is what the prefix test did for every shape not starting with a marker. That keeps both sides comparable without depending on the old revision, and both sides name how they were produced. Refs #17668 * test(reach): re-pin the one-claim-registry floor to its live population (#17668) CI red after merging main into this PR: the floor described a tree two merges old. [one-claim-registry] floor 2976 sits 51 below its live population of 3027 Python modules swept for claim-or-lock primitives, which exceeds the declared allowance of 50 (skips=0 + growth=50). Re-pinned to 3,002 -- mid-window, which is the convention this declaration documents for itself and explains: pinning at `population - growth` leaves zero headroom, so the next module anyone adds reds the meta-test (#17142). 3,002 leaves 25 before the next bump is due. The allowance is untouched. Only **one** of those 51 modules came from this PR. The sweep skips `_test.py`, so of the three files added here -- `comment_blanking.py` and two test modules -- one is swept. The other 50 arrived with base. The measurement is worth recording because I got it wrong first. I measured `len(REACH.discover())` in my worktree and got **3025** against CI's **3027**, and the two-module gap was not a discrepancy to explain: my worktree was still at `495c4811f4` and had never pulled the `update-branch` merge. I was measuring a tree two merges older than the one CI tested, which is the same shape as reading one side of a diff from the working tree and the other from a revision. Fast-forwarded to `b2f41ad3c694` and re-measured: **3027**, matching CI exactly. So both numbers in this commit name their revision: 3,027 live at `b2f41ad3c694`, by `len(REACH.discover())`, not by grep. Mutation-proved the meta-test still does its job: setting the floor to 2,900 fails `test_every_declared_floor_is_pinned_to_its_population`. A re-pin that quietly broke the check it satisfies would look identical to a correct one. 289 tests pass across `reach_declarations_test.py` and the registry guard. Refs #17668 * fix(analytics): verify through the production scanner, and treat ${} as code (#17668) Two review findings, batched into one push. The one rated Trivial is the serious one. **The test reimplemented what it verified.** `_scan` applied `blank_comments` and the call patterns itself rather than calling `FrontendAPICallScanner._scan_file`, so it would have passed with the production blanking removed entirely. Proved rather than argued -- deleting the `blank_comments` call from `_scan_file`: before this commit: both suites pass <- the hole after this commit: scanner test 2 failed, blanker test 17 passed The blanker test correctly stays green there, because the blanker is fine; only the wiring broke. That split is the signal the old arrangement could not produce. This is the same defect as verifying with a private regex instead of the scanner's own patterns -- which this PR already fixed once, one level down. I replaced the regex and kept a parallel copy of the loop around it. A verification that reimplements its subject tests its own copy, and the copy cannot drift from itself. The two sides are now named for what they are: `_scan_production` calls `_scan_file`, and `_scan_prefix_era` is an explicit reconstruction of the pre-fix behaviour, kept separate because it can no longer be obtained from the production path. **A `${...}` interpolation is code, not template text.** `_consume_string` copies a backtick literal verbatim so that a URL's slash pair survives -- correct for the text, wrong for interpolations, which can contain comments. Interpolations now go back through `blank_comments` recursively, with brace matching so a nested `{}` does not end one early. Length is preserved, so positions still hold. comment inside ${} hidden -> blanked nested braces in ${} hidden -> blanked `/api/items/${id}` kept <- template text is still text Mutation-proved: disabling interpolation handling fails 2. 25 tests across the two files. All functions under the 30-line gate; the scanner stays at its re-pinned 1444. Refs #17668 * fix(analytics): count interpolation braces in code only (#17668) `_matching_brace` counted raw braces, so a `}` inside a comment or a string ended the interpolation early and everything after it was copied out as template text. Verified before fixing -- both shapes leaked a path the scanner would then report: const x = `${/* } api.get('/api/hidden') */ ''}` leaked /api/hidden const y = `${ m['}'] /* api.get('/api/gone') */ }` leaked /api/gone The review named the comment case; the string case has the identical cause and would have survived a fix aimed only at comments. A brace inside either is not a delimiter. The counting view comes from `blank_comments` itself, with an internal flag that blanks string bodies as well, rather than from a second brace scanner. That is the point: a separate scanner could disagree with this one about where a string ends, and then the two would drift -- which is the defect this module was written to fix, appearing one level down inside the fix. Whatever the real pass treats as a comment or a string is exactly what the counter refuses to count. Blanking preserves length, so indices in the view are indices in the source. Recursion is broken by not processing interpolations in the counting view: a brace inside a nested template is not a delimiter of the interpolation being measured, so the whole literal blanks and `_matching_brace` cannot re-enter itself. Two regression tests, one per shape. Mutation-proved: reverting the counter to raw characters fails both. 27 tests across the two files, function-length gate clean. This is the fourth finding on this PR that was the same class as the PR's own subject -- private regex, parallel scan loop, interpolation contents, and now brace counting -- each one a level further in. Every one was found by review, none by a tool. Refs #17668
#17673, #17650) (#17678) Both are the #15826 class: a claim about structure enforced by something that cannot express structure. #17673 -- the versioning redaction assertion was the sibling of the one #17667 replaced, and survived because CI only flagged the one my change broke. Its property is NOT the runner path's: that one is positional within a function, this one is aliasing. `_apply_version_to_fact` receives `target_version` from its caller and must write the redacted value back through a subscript on that dict, because `revert_to_version` reads the same dict afterwards to record the revert. A local-only redaction reads as correct and leaves the second write raw -- the regression #13708's review found. So two assertions rather than a mirror of the runner's one: the write goes through the caller's dict, AND the apply call precedes the record call. Mutation table, new guard vs the substring it replaces: local-only redaction (the real regression) new FAIL old FAIL redaction removed new FAIL old FAIL record before applying (order swapped) new FAIL old PASS equivalent rewrite via a temp variable new PASS old FAIL Wrong in both directions on two of four -- blind to a regression it names, loud about a refactor that preserved the property. #17650 -- `test_the_uncovered_record_only_shrinks` pins equality. The equality is correct and documented: RATCHET_BASELINES.md holds exact congruence in both directions up as the one exemplar satisfying staleness AND blindness, and headroom under a ceiling is room for a new bypass to appear with nothing failing. The missing half is the rest of the stated convention -- "and only ever goes down" -- which was prose. Equality makes MAX track the count in EITHER direction, so a new bypass is made green by raising the number: 27 -> 38 -> 39 -> 41 -> 42 -> 45. Added the direction check with the escape #17650's own criterion asks for (justified, not forbidden): MAX may not exceed the base revision's value unless `ACKNOWLEDGED_RAISE` names exactly the current value and cites an issue. Checked in both directions -- a record that no longer matches MAX fails as stale, so lowering MAX cannot leave a standing permission behind it. Verified across five states: bare raise fails, matching record passes, stale record fails, record without an issue reference fails, restored passes. Neither RATCHET_BASELINES.md nor ARCHITECTURE_EXCEPTIONS.md defines an allowance path for a baseline raise, so this test carries the record itself. The preferred route is still the one the ratchet exists to force: widen the filter until the read is covered, as #16442 did. A git-based check that cannot resolve the base SKIPS with a stated reason rather than passing -- building a guard that reports clean having examined nothing, inside the fix for a guard that could not hold its own direction, would be a pointed own goal. `kb_content_redaction_chokepoint_guard_test.py` reached 629 lines, over the 600 ceiling, so the placement helpers moved to `repo_tests/_redaction_placement.py` rather than the ceiling moving: 629 -> 488, plus a 176-line module. Refs #17673, #17650, #15826, #17667, #13708, #16442
…eady had (#17680) (#17685) Reported together as "I can create a project but cannot do anything with it". Three of the causes are unrelated to each other; this lands the three that are defects in code that exists. `GET /api/llc/backlog` has accepted and honoured `project_id` since it was written, plumbed through to `list_backlog`. `BacklogView` was its only caller and sent `company_id` alone, so every project's backlog was the whole company's. The project dropdown the view already had belongs to the bulk-assign modal and never touched the request. Filtered server-side rather than in `filteredItems` with the other filters, because the route pages at `limit=50` by default: narrowing an already-truncated page returns an arbitrary subset of a project rather than the project. The project list is now loaded on mount, since `loadProjects` was reachable only by opening the assign modal. This is the same defect as #17651, fixed hours earlier: a parameter that exists, is honoured, is tested on the producer, and is omitted by its one caller. Both sides stay internally consistent, so no test on either side can see the gap -- which is why the new tests assert the request rather than the rendered list. Delete looked broken on a new project because the archive-before-delete rule was expressed only by the button's absence. Both disposal routes refuse a non-archived project with a 409 carrying an actionable reason, and every failure in `deleteProject` collapsed to one string, so a lifecycle violation, a permission refusal and a network fault read identically. The precondition is now stated, and 409 and 403 get their own messages. "Findings scanning is disabled" was shown both when scanning was disabled and when the policy could not be read. Failing closed stays -- an action that could only 403 must not be offered -- but a server fault reported as a deliberate setting is a fault nobody investigates. The policy is now four states, with `loading` rendering nothing so the note does not flash on the way in. A 403 from the scan action no longer claims the policy is off, since the policy was read separately and said otherwise or the button would not be there. Every behaviour change is mutation-tested: omitting `project_id`, collapsing `unavailable` back to `disabled`, dropping the 409 mapping, and rendering the hint unconditionally are each CAUGHT by a named test. Five new i18n keys across all 11 locales. Not fixed here, and filed instead: #17681 (projects cannot be edited -- PATCH exists and nothing calls it, a form is a different risk) and #17682 (no project-level membership exists on either side, and it needs an owner ruling on whether that is its own relation or project-scoped role holding before a second source of truth gets built). Closes #17680 Closes #17683 Closes #17684 Refs #17651 Refs #17681 Refs #17682
…ublishes (#17643) (#17679) `WORKFLOW_EVENT_TYPES` held `workflow_status_update` and a bare `step_completed`. No backend publishes either under any spelling, so a workflow that failed or was cancelled never left the active list without a manual click, and nothing moved mid-run. Only completion got through, and it had done since 2026-04-01. The names now come off the publish sites in `api/workflow.py`, split by what the event means for the list rather than handled alike: a terminal event changes WHICH workflows are active and refetches, a progress event moves one card and is applied in place. That is also the answer to #17477's per-step refetch cost -- the cost was real, and it goes away by routing progress events away from the refetch instead of by dropping them. Binding `@workflow-update` completes #17364's fourth criterion. The emit was written, declared, and never bound; `applyWorkflowProgress` patches the matching workflow by id using `step_index` / `total_steps` / `step_id`, which is exactly what the card's `stepOf` renders. No payload change and no invented event -- the data was already being published to nobody. The second consumer is on a different transport and was reconciled against its own producer, not this one. `useWorkflowBuilder.handleWebSocketMessage` reads `data.type` off `workflow_ws/{session_id}`, fed by `workflow_automation/*` `send_json`; three of its four cases named messages that producer never sends, including `approval_required`, which gates `pendingApproval` -- so plan approval never reached the UI on that path. It is now `workflow_plan_presented`, with the request under `plan`. `workflow_status_update` is removed rather than repointed: it carried an in-place update of a whole workflow object and nothing on either transport sends one. The capability is delivered by `applyWorkflowProgress`, on the transport that does carry the fields. The guard is per transport pair. #17643's original criterion asked that the frontend's names be a subset of "the backend's published set", which has no referent -- there are two producers, and a union of them would certify `workflow_paused` for the dashboard, a name that can never arrive there. The amendment is recorded on the issue. The overlap between the two vocabularies is pinned at exactly `workflow_completed` and `workflow_cancelled` (#17676), so a third shared name fails rather than widening quietly. One test changed because it was certifying the defect: `WorkflowLiveDashboard.subscriptions.test.ts` fired `workflow_status_update` and asserted a refresh. It passed, always had, and was a green assertion about an event that cannot occur. Closes #17643 Closes #17364 Refs #17477 Refs #17676 Refs #17677
…) (#17687) `KbPushButton.vue` imported no `useI18n` and called no `t()`. Seven locale keys now cover the six strings, across all 11 locales. The six sat in three different template positions, which matters for why nothing caught them: three were text nodes, one was a `placeholder` attribute, and two were string literals inside an interpolation. The repo's bare-literal detector reads text nodes and strips interpolations wholesale before looking for prose, so pointed at this file it reports three of six. It is not pointed at this file either -- `slm_frontend_bare_ui_literals_test.py` scans `autobot-slm-frontend/src` only. Both gaps are measured and filed rather than widened here, since widening the scan is a ratchet against 800-odd literals and not a change this PR can honestly carry. The tests assert rendered output against the shipped locale files, so all six are covered by one mechanism whatever position each occupied. One of them mounts in GERMAN, and that is the only test here that can fail on a reverted literal: every English assertion compares against `en.json`, which says exactly what the removed literals said, so reverting `t('...push')` to `Push to KB` leaves them all green. I wrote the English ones first and found this while mutation-testing. Also surfaced, in the same file: `push()` logged its failure and told the user nothing. The form closed on success and stayed open on failure, so the only difference a user could see was a control that did not move. Fixed here rather than filed, because it is four lines in a file this change already rewrites and a second PR would collide on it. Reuses the global `.field-error-msg` rather than adding a class -- the component has no `<style>` block, and its existing `kb-pushed-badge` / `kb-push-form` classes are defined nowhere. Every behaviour change is mutation-tested: reverting the text label, the placeholder attribute, the interpolated pair, or the error surfacing is each CAUGHT by a named test. Closes #17535
…17370) (#17691) #17054's fourth criterion. The socket gate itself landed in #17369; this is the UI agreeing with it. THE ISSUE'S OWN CRITERION WOULD HAVE CREATED THE DISAGREEMENT IT FORBIDS. It said gating on `hasPermission('mcp.desktop.control')` would agree with the socket "automatically, including on [superadmin]". It would not: `usePermissions.hasPermission` short-circuits `if (userStore.isAdmin) return true`, and `isAdmin` admits superadmin -- its own test says so. But `ROLE_PERMISSIONS[superadmin]` is EMPTY by #13854's ruling, so the socket refuses a superadmin. Gating on `hasPermission` would have offered a superadmin a desktop the handshake then closes with 1008. That is #13854 reappearing one layer up. PR #17369 refused to write `is_admin_role(role) or role_has_permission(...)` in the backend gate for exactly this reason; `hasPermission` is that short-circuit, alive in the frontend composable. So `usePermissions` gains `holdsPermission` -- the role table, no administrative override, the same question `role_has_permission` answers -- documented with why a caller matching a backend gate must not reach for `hasPermission`. `hasPermission` is NOT changed: its override is relied on by call sites this did not audit, and altering it across the app is a different risk envelope. Filed separately. Four render sites, not one. Hiding the tab is cosmetic on its own: a bookmarked `/chat/novnc` sets `activeTab` from route meta, so `ChatTabContent` renders the viewer with no tab present. `/slm/tools/novnc` carries `requiresAuth: true` and nothing else, and a bookmarked URL is activation. `DesktopView` mounts the viewer and is currently unreachable behind a redirect -- gated anyway, because an unreachable mounted viewer is unfinished work and the redirect is one line from being removed. One `DesktopAccessDenied` component rather than the explanation copied four times. It is not the boundary and says so in its own comment. The two entry points the issue left unassessed were checked, not assumed: `AdvancedControlView` is a capability READOUT (`novnc_available ? yes : no`), not an entry point, and gating it would hide diagnostics from the people who need them. `DesktopView` is an entry point and is gated. All five tab labels go to i18n, not the one the issue named. They were one `withDefaults` expression, and fixing one of five while editing the line is the shape the one-defect-one-fix rule exists to stop. A computed rather than a default factory, so labels follow a locale change. Every behaviour change is mutation-tested. The swap that matters -- `holdsPermission` back to `hasPermission` -- is CAUGHT by exactly one assertion, the superadmin case, which is why that case is in the file. Closes #17370 Refs #17054 Refs #13854
…ready existed (#17681) (#17695) `PATCH /api/llc/projects/{id}` (`llc/api/sprints.py:685`) has worked since it was written, with its IDOR guard, and no frontend called it. Projects were write-once at creation. The owner reported this as "I can create a project but cannot edit it", which was exactly what the code did. The field set is read off `ProjectUpdate`, not off `ProjectResponse` -- the response returns more than the route accepts. Five of its eight fields are in the form; the three left out, each for a reason rather than by omission: lead_agent_id / lead_user_id a person-picker here would settle #17682's open ruling on how people attach to a project, one field at a time env free-form JSONB; a textarea of raw JSON is a worse editor than none `status` is a select over the five members of the `projectstatus` DB enum, not a text field: `ProjectUpdate.status` is an unvalidated `str` while the column is an enum, so a plausible typo like "in-progress" is refused by Postgres as a 500 rather than by FastAPI as a 422. Filed as #17694 with the other half of that finding -- `exclude_none=True` means no field can be cleared back to null, so the form cannot offer a "clear" control for the target date and does not pretend to. The request carries only the fields the user actually changed. Sending everything also works, and quietly rewrites values nobody touched, so the tests assert the body rather than the rendered card -- a card-only assertion passes against either. Not gated on `lifecycle_state`: renaming is not a lifecycle operation, and an archived project with a wrong name is exactly the one that needs fixing. A 404 reports as "no longer available". The IDOR guard answers 404 for a project outside the caller's org as well as for one that does not exist, so the message does not claim a permission verdict it cannot distinguish. Thirteen keys across all 11 locales. Every behaviour change is mutation-tested: sending every field instead of a diff, prefilling from blanks, collapsing the 404, dropping a status option, and skipping the no-change early return are each CAUGHT by a named test. Closes #17681 Refs #17682 Refs #17694 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…tem CSS, stale SLM locales (#17698) (#17699) * fix(frontend): unblock base — undefined tokens, duplicated design-system CSS, stale SLM locales (#17698) Three failures on `origin/main` at 290fae3, all from my #17691 and #17695. Nothing else in this commit. 1. FIVE UNDEFINED TOKEN REFERENCES. `--color-text-primary` (x2) and `--color-text-secondary` (x3) are not tokens; the real names are `--text-primary` / `--text-secondary`. No fallback added -- the ratchet's own message says why: a fallback hard-codes a colour that then ignores the theme. The rest of `ProjectBrowserView.vue` already used the correct spelling on twenty surrounding lines, which is what makes this a slip rather than a misunderstanding. 2. A SECOND IMPLEMENTATION OF AN EXISTING DESIGN-SYSTEM COMPONENT. `DesktopAccessDenied.vue` hand-rolled `.desktop-denied{,-icon,-title,-desc}` -- the exact four-part structure `assets/css/components.css:649` already ships as `.empty-state{,-icon,-title,-desc}`. Likewise `.lifecycle-hint` duplicated `.field-label-hint` and `.create-checkbox` duplicated `.checkbox-label`/`.checkbox-input`. That is a canonical-design-system violation on its own, and it is also exactly what put three fragmentation ratchets over: +1 components_declaring_styles, +6 css_rule_declarations, +6 distinct_class_names. Attributed by counting, not assumed -- four rules and four classes from the new component plus two of each from `ProjectBrowserView`, which is 1/6/6 exactly. Fixed by using the canonical classes, so `DesktopAccessDenied.vue` now has no `<style>` block at all. The ratchets return to their pins by REMOVAL rather than re-pinning, which is what a shrink-only ratchet is for. `inline_generics` was NOT over on this tree -- it passes here at 563, so I have left it alone. 3. STALE GENERATED SLM LOCALES -- and this was the actual red check. `SLM Frontend Check` failed at `Locales match the lifting rule`, not at the ratchets. `lift_locale_translations.py` lifts a translation into `autobot-slm-frontend` for every key whose English source appears verbatim in `autobot-frontend`'s locales. The twenty keys I added in #17691 and #17695 gave the rule sources it did not have, so five locales (fa, he, lv, pl, ur) stopped being what the rule produces. Regenerated. Verified rather than trusted: 35,123 keys before and after, ZERO lost and ZERO added, 36 replacements only. They are strings that were stuck in English and now carry the translations added in #17695 -- `"Completed"` becomes `"Pabeigts"` in lv, and so on. This is 36 SLM strings becoming translated, not merely a checksum satisfied. Closes #17698 is NOT claimed: the instrument hole that let two green PRs break base is f8's to fix and is tracked there. This commit only removes the damage. * fix(guards): restate the css-var reach floor, drop the inline generic that broke the contract pin (#17698) Two more failures on the same base, both of which only surface on a PR that actually runs `python-suite` -- #17698's mechanism. 1. REACH FLOOR, restated rather than nudged. `undefined-css-var-scan` declared `floor=1400, growth=60` against a measurement of 1452 files. The tree is now **1463**, so the slack reached 63 against an allowance of 60 and `verify_floor` refused the DECLARATION -- the scan itself was fine. Re-measured and restated at **1433**, not 1403. The old pin sat 52 of its 60 allowance below its own measurement, leaving room for 8 more files before a bump came due; 11 arrived. 1433 puts the slack at 30, half the allowance, so the next bump is due after 30 more files. `population - growth` would put the slack exactly at the allowance and go red on the next file anyone adds, which is the state this just came out of. 2. INLINE GENERIC -- and this one is MINE, not pre-existing base debt. `inline_generics` went 562 -> 563 because #17695's edit form named a response type at the call site in `saveProject`. Same story as the CSS tokens: merged green, invisible to a frontend-only PR. I had told the coordinator this pin "passes at 563". That was wrong and worth naming: I had run `frontend_fragmentation_ratchet_test.py`, which contains the string `inline_generics` **zero times** -- the pin lives in `frontend_api_contract_ratchet_test.py`. A correct measurement of a different question, and I quoted a number I had not measured. The pin is shrink-only and does not move. Fixed by not asserting a shape at the call site: the response is neither typed nor read, and the list is reloaded from the endpoint that already types it -- what `createProject` and `deleteProject` in this file already do after a mutation. The guard's own advice is that naming a response type at the call site is a claim TypeScript cannot check, and a refetch removes the claim rather than relocating it. The explaining comment deliberately does NOT contain the offending syntax: the detector reads comments, so writing it there to explain it would be counted. My first attempt did exactly that and left the count at 563. The edit test now asserts the REFETCH and not merely the rendered name -- a handler that read the response would also end up showing the new name, so the two are indistinguishable by output alone. Mutation-tested: restoring the inline generic is CAUGHT by both the contract ratchet and the edit test. Refs #17698 Refs #17552 Refs #12363
…it claims (#16817) (#17696) Three of #16817's ACs. The release of what a run held is #16818's and is not touched here. AC4 -- a stalled run is distinguishable, not inferred. The sweep wrote `LLCRunStatus.TIMEOUT`, which four sites set: http_adapter, subprocess_base, heartbeat_scheduler and the sweep itself. So "which runs stalled?" meant matching a formatted, human-readable `error` string -- inference, and it breaks on a re-wording. `STALLED` is a new member; `error` still carries the detail. This reverses a decision recorded in the sweep's own docstring, so the reason is recorded too. The original reused TIMEOUT because that "keeps the state machine as it is". That cost is close to zero and was probably overestimated: `status` is `sa.String(32)` rather than a native enum, so no migration, and `is_terminal` is a deny-list over {QUEUED, RUNNING}, so a new terminal member needs no edit there. The thing traded away was what AC4 asks for. AC5 -- `FOR UPDATE SKIP LOCKED`. Two beat workers overlapping today would write the same terminal status, so the race is invisible. #16818 adds the release of claims, assignments and workspace leases to this path, and the same race then releases each holding twice. The lock belongs here BEFORE the side effects that need it: a defect armed by its own dependency is cheaper to prevent than to diagnose. The scan -- the sweep loaded EVERY non-terminal run and compared ages in Python. The existing comment's NULL reasoning was right and the implementation did not follow it; `COALESCE(started_at, created_at) <= cutoff` is that same reasoning expressed where an index can serve it. Not a reversal of a documented decision -- the decision was already this, in prose. The query moved into `_stalled_candidates()` so both properties are assertable without a database. Mutation-checked, each failing exactly one test and the right one: write TIMEOUT again -> the sweep's behavioural test fails drop SKIP LOCKED -> the lock test fails drop locking entirely -> the lock test fails age filter back in Python -> the in-SQL test fails Two notes on the tests themselves. The lock assertion compiles against `postgresql.dialect()`, because the default dialect renders FOR UPDATE and silently drops SKIP LOCKED -- asserting on the generic rendering would have reported a lock clause production never gets. And `_async_sweep` is now driven through a fake session: every other AC4 test passes while the sweep writes whatever it likes, because the enum member existing is not the sweep using it. `test_the_error_says_the_sweep_decided_it` kept its name and lost its premise -- it used to state that the error was "the only place that distinguishes them", which was true and was the defect. Refs #16817, #16818 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Contributor
|
Important Review skippedToo many files! This PR contains 728 files, which is 628 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (728)
You can disable this status message by setting the 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 |
…el (#17020) (#17697) * feat(canvas): make Live Canvas live — publish cell changes on a channel (#17020) "Live Canvas — agent-driven visual canvas workspace" (#7425) loaded once and received nothing. Measured by call site, not by name: useCanvasWebSocket.ts:15 opened /api/canvas/{id}/ws -- api/canvas.py has five REST routes and no websocket get_canvas_cell_event :325 ZERO callers (one occurrence: its own def) register_canvas_streaming_task ZERO callers unregister_... ZERO callers CanvasView.vue :83 onMounted(loadCanvas), no polling A REGISTRATION IS NOT A CALL. `_format_canvas_cell` sits in MESSAGE_TYPE_FORMATTERS under the key "canvas_cell", so every reader who checked for the name found it, in a dispatch map, and stopped. That is why this change is verified by call sites and why the guard asserts them. CHANNELS, NOT ROUTES. The issue's criterion said to wire the client to "the endpoint that actually carries canvas events"; there was none, and the alternative -- remove it as superseded by the shared bus -- was also false, since the bus had no canvas_cell publisher either. Serving the bespoke socket would have made a third parallel delivery system, which EVENT_STATE_DOCTRINE principle 1 names as a state this codebase has already been in, and principle 5 forbids directly: new event types extend the channel grammar, they do not add an endpoint. The doctrine's anti-goals do give binary and high-volume streams their own sockets -- VNC, audio, PTY, log tails -- and a JSON cell delta is not in that carve-out. So: `canvas` joins _VALID_PREFIXES, `_authorize_canvas_channel` gates it on the same owner check every REST route on the resource applies, and the three routes that write a cell publish canvas_cell after committing. Publishing before the commit would announce a write a rollback discards; publishing inside the persistence step would make delivery its side effect (principle 4). PersistStrategy.NONE deliberately -- the durable fact is the cell row, and a client that was away re-reads it with GET (principle 2). Agent DELTA streaming still has no producer. Nothing writes cell content except those three REST routes, so `seq`/`delta` carry a version and the full content rather than an incremental chunk. What this delivers is every real cell change reaching every subscriber, which is what "does a second connected client see the same thing as the first" asks. An agent streaming path publishes to the same channel when it exists, with no new transport. api/canvas.py was grandfathered at 637 lines in a shrink-only ratchet and the call sites pushed it over, so rich-payload validation is extracted to canvas/rich_payload.py -- split rather than raise, per the rule. The file is now 584 lines, under MAX_LINES, so its KNOWN_LARGE and RATCHET_BASELINE entries are DELETED: the tool's own message says an entry naming a compliant file exempts nothing while looking authoritative. The consumer's message handling is unchanged, conflict branch included. It was written for exactly this event and had simply never been reached. Guard mutation-tested, six ways: removing a publish, publishing before the commit, serving a @router.websocket on the canvas router, dropping the canvas prefix, making the authorizer fail open, and unhooking it from the dispatcher are each CAUGHT. Closes #17020 Refs #7425 Refs #17009 * fix(canvas): cell ids collided within a millisecond, so the conflict branch overwrote the cell it protects (#17020) The conflict test added in this PR failed in CI and passed locally. The test was right; `addCell` was wrong. id: `cell-${Date.now()}` Millisecond resolution, and every operation in this store addresses a cell BY id -- `deleteCell`, `moveCell`, `upsertStreamCell`, and the template's `:key`. Two cells created in the same tick shared one. That is what broke the conflict branch specifically. It adds an agent cell and then addresses "the last one": store.addCell('agent') const newId = store.cells[store.cells.length - 1].id store.upsertStreamCell({ ...msg, cellId: newId }) With a duplicate id, `newId` equals the conflicted cell's id, so the delta landed straight back on the cell the user was resolving -- the one thing that branch exists to prevent. WHY IT WAS NEVER CAUGHT BEFORE. That branch had never executed. Making it run for the first time is what #17020 is for, and a latent collision in id generation is precisely the kind of defect only a first execution can reveal. Better here than after merge. Reproduced deterministically with a frozen clock rather than by a fast loop -- a loop only collides when the machine is quick enough, which is why it passed on mine and failed in CI. The three new store tests use `vi.useFakeTimers()`, and one asserts the CONSEQUENCE (`deleteCell` removes the cell asked for, not whichever `find` reaches first) so it stays meaningful if the id scheme changes again. `crypto.randomUUID()` matches the five existing call sites, including the same shape at `useAppStore.ts:195`. FIXED AS A CLASS, NOT AN INSTANCE. `views/CustomDashboard.vue:516` had the identical defect -- `id: \`widget-${Date.now()}\`` -- and dashboards address a widget by id for layout, removal and `:key`. Both are fixed here rather than one fixed and one left for whichever PR next trips over it. A sweep for `id: \`...${Date.now()}\`` across the frontend now returns nothing. Mutation-tested: reverting either id to `Date.now()` is CAUGHT -- by all three store tests and by the CI conflict test.
Contributor
AutoBot Phase Validation ResultsSystem Maturity: undefined% Phase Status:FAIL Phase 1: Core Infrastructure: undefined% Recommendations:
|
This was referenced Sep 28, 2026
mrveiss
added a commit
that referenced
this pull request
Sep 28, 2026
…e class-1 cause (#17717) Two corrections to the first commit on this branch, both because the original was wrong about a fact it asserted. ALL NINE ARE NOW BASELINED, by explicit owner ruling. The first commit carried five and held four; the ruling reached both this session and the coordinator's and now agrees. The class-2 paragraph is unchanged in substance -- it still says a `Co-authored-by: Claude` trailer is the thing the gate exists to reject, is exempted only because `main` cannot be rewritten, and is NOT a precedent -- and it now also says its source is open, which makes it worse than class 1 rather than merely different. CLASS 1 WAS NOT A MISCONFIGURED USER IDENTITY. The first commit said the source was closed because no worktree config and no global config resolves `user.name` to `t`. That measurement was correct and about the wrong thing. `t <t@example.invalid>` is this repository's TEST-FIXTURE identity -- ten files carry it, eight run `git commit`, two of those under the pre-push hook. The reason it reaches the real repository is that an inherited `GIT_DIR` OUTRANKS `-C` and `cwd=`; `hooks_path_override_15961_test.py:450` records learning that the hard way. So no config override was ever there to find, and checking config is how you fail to find it. Three of the fixture files write the identity with `git config` rather than a per-invocation `-c`. Worktrees share one config through the common dir, so a single unscrubbed write sets it for every worktree at once -- which is the shape of five commits across five branches in one afternoon. WHAT I VERIFIED RATHER THAN RELAYED: every one of those sites currently scrubs. All eight committing tests pass `scrubbed_git_env()`, and all three config writers unset the ambient variables -- checked per file, not inferred from the helper existing. So no current test leaks, and the header says that instead of claiming the source is closed. The open window is a NEW fixture that commits or writes config, run before its scrub is added. That is what happened here: 969c195 is the commit that introduced one such test. Source is #15353, open; detection gap #16124; release-facing home #16268, which already carries 18 from an earlier promotion. So this baseline is the third containment of one root cause -- 22, then 18, now 9 -- and the header says so, because whoever picks up #15353 should be able to find the cost of not doing it in one place. Gate parse verified against the file as committed: 31 entries, all nine present. Refs #17717 Refs #13654 Refs #15353
…v0.10.0 (#17717) (#17718) * ci(release): baseline five squash-stamped identity trailers blocking v0.10.0 (#17717) #17717 promotes main -> release and fails "No commit trailers" on commits already landed. Per-PR scans passed -- including #17696's own -- because the gate compares BASE..HEAD and the trailer does not exist until GitHub's squash-merge creates it. Only a promotion re-scans the landed range, so the check runs on an object no PR ever showed it. DERIVED AND VERIFIED, NOT TAKEN FROM A LIST. The run log reports ten "Offending commit:" lines and nine unique SHAs. The tenth is the workflow's own printf template -- `Offending commit: %s\n' "${bad[@]}"` -- echoed into the log. Nine real commits, and a count taken from the line count would have been the measurement's artefact rather than the population. The nine are TWO classes, established by reading each commit's trailers rather than assuming one cause: five Co-authored-by: t <t@example.invalid> four Co-authored-by: Claude <noreply@anthropic.com> ONLY THE FIVE ARE BASELINED. They are the same cause as the existing `test <test@test>` entries, a second time: a local user.name/user.email override shadowed the correct global identity, branch commits carried it, and the squash stamped it onto the result. `t <t@example.invalid>` is not a person and claims no attribution. The source is CLOSED -- no worktree config and no global config resolves user.name to `t`, checked rather than assumed. That check is what makes the class bounded at five, and the header says so, because the previous instance's header claimed the override was "since corrected" and a different one appeared anyway. The claim was true and insufficient; the check is the thing. THE FOUR ARE HELD, AND THE HEADER SAYS WHY. `Co-authored-by: Claude` is a real attribution claim -- exactly what no-commit-trailers.yml (#13654) exists to reject -- not a misconfigured identity. The reasoning that justifies the five does not reach it, so folding it in would exempt the thing the rule is for on a ruling about something else. A reader of this file must not conclude that a `Claude` co-author is baselinable. Its source is also not closed, which is why it is a separate question rather than a longer list: the agent harness instructs sessions to append that trailer while this project's rules forbid it and this gate enforces them. Until that is resolved at the source, new instances keep landing and a baseline is treating the symptom. Filed separately. THE TOP RULE IS NARROWED, NOT SUSPENDED. It said entries may only be removed. It now names the one class an addition is permitted for -- a trailer no branch commit carried, created by the squash, found only at promotion, on a history that cannot be rewritten -- and says explicitly that a trailer which WAS on a branch commit does not qualify, because that one was fixable and the fix is to fix it. Verified the gate's own parse: of the nine, exactly the five appear in `sed 's/#.*//' | tr -d '[:blank:]'`, and the four named in the header comment do not. The workflow is untouched. Refs #17717 Refs #13654 * ci(release): baseline the four Claude-trailer commits, and correct the class-1 cause (#17717) Two corrections to the first commit on this branch, both because the original was wrong about a fact it asserted. ALL NINE ARE NOW BASELINED, by explicit owner ruling. The first commit carried five and held four; the ruling reached both this session and the coordinator's and now agrees. The class-2 paragraph is unchanged in substance -- it still says a `Co-authored-by: Claude` trailer is the thing the gate exists to reject, is exempted only because `main` cannot be rewritten, and is NOT a precedent -- and it now also says its source is open, which makes it worse than class 1 rather than merely different. CLASS 1 WAS NOT A MISCONFIGURED USER IDENTITY. The first commit said the source was closed because no worktree config and no global config resolves `user.name` to `t`. That measurement was correct and about the wrong thing. `t <t@example.invalid>` is this repository's TEST-FIXTURE identity -- ten files carry it, eight run `git commit`, two of those under the pre-push hook. The reason it reaches the real repository is that an inherited `GIT_DIR` OUTRANKS `-C` and `cwd=`; `hooks_path_override_15961_test.py:450` records learning that the hard way. So no config override was ever there to find, and checking config is how you fail to find it. Three of the fixture files write the identity with `git config` rather than a per-invocation `-c`. Worktrees share one config through the common dir, so a single unscrubbed write sets it for every worktree at once -- which is the shape of five commits across five branches in one afternoon. WHAT I VERIFIED RATHER THAN RELAYED: every one of those sites currently scrubs. All eight committing tests pass `scrubbed_git_env()`, and all three config writers unset the ambient variables -- checked per file, not inferred from the helper existing. So no current test leaks, and the header says that instead of claiming the source is closed. The open window is a NEW fixture that commits or writes config, run before its scrub is added. That is what happened here: 969c195 is the commit that introduced one such test. Source is #15353, open; detection gap #16124; release-facing home #16268, which already carries 18 from an earlier promotion. So this baseline is the third containment of one root cause -- 22, then 18, now 9 -- and the header says so, because whoever picks up #15353 should be able to find the cost of not doing it in one place. Gate parse verified against the file as committed: 31 entries, all nine present. Refs #17717 Refs #13654 Refs #15353
This was referenced Sep 28, 2026
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
releaseis at3421377aa1, the v0.9.0 promotion merge from 2026-09-23.mainis 98 commits ahead — five days of merged, reviewed work. The v0.9.0 milestone closed today with 0 open and 331 closed after its remaining issues were re-scoped intov0.9.1–v0.9.6and four parking milestones; per #16801's recorded ruling, the milestone is a planning artifact and a release ships what is onmain, so this PR promotes what is finished.What Changed
Promotes
maintorelease. No source edits in this PR — it is the merge.Since v0.9.0 the range carries 70 fix · 8 feat · 7 test · 4 docs · 3 refactor · 3 chore · 1 guard · 1 ci, no breaking changes. The 8
featcommits meangit-cliff --bumped-versionwill produce a minor bump: the tag will be v0.10.0, not a v0.9.x patch. That is the workflow's own rule, not a choice made here.Four changelog fragments are in
changelog/unreleased/and become the release-notes headline: admin-only UI gates (#16494), apt repo-add retry (#17230), orphan-storage deletion proposer (#17315), agent-terminal conversation binding (#17422).Why this shape
release.ymltriggers on push toreleaseand cuts the tag, the per-version changelog file and the GitHub Release itself.releaseis a protected branch; a promotion is a PR frommain, merged, never a direct push.What is different about this run
This is the first release run since "Allow GitHub Actions to create and approve pull requests" was enabled (#15167). Two consequences, both to watch rather than assume:
gh pr createfor the first time instead of classifying its own permission failure. Success is no::notice::and no::warning::from that step, plus a PR with basereleaseand headchangelog-v0.10.0. A green step alone is not evidence — it exits 0 either way by design.paths-ignore: [CHANGELOG.md, changelog/**]loop guard (release.yml self-triggers on its own changelog commit → spurious release (latent infinite loop once #11853 lands) #11855) becomes reachable for the first time. When the changelog PR is merged, there must be no secondrelease.ymlrun triggered by it.Verification
mainat80b6200d4a: one advisory failure,AutoBot Phase Validation(bug(ci): the phase-validation report is a hand-written key whitelist — a gate reading a new top-level key sees 0, and the file's own note predicted it (#7496, now #17089) #17674, a gate reading a key the report never contains — not one of the required contexts).releaseprotection requires exactly two checks: no open blocks-merge issues, no commit trailers.RELEASE_WORKFLOW.md: after merge, confirm the "Check if release needed" step printedNext version: v0.10.0. A green run alone does not mean a release happened.Refs #15167
Refs #11855
Refs #16801
🤖 Generated with Claude Code