Repository navigation
Conversation
… route left open (#15745) Four of the nine routers in KNOWN_UNGATED exposed configuration writes or operational control with no authentication of any kind. Each now takes the house admin gate -- `auth_middleware.check_admin_permission`, used by 129 routers, raising 401 unauthenticated / 403 non-admin -- per the authorization model decided and recorded on #15745. api/redis.py router-level POST /config (Redis config write) + 3 api/developer.py router-level POST /config (dev-mode config write) + 3 services/knowledge_sync_service.py router-level daemon start/stop, manual sync + 3 api/wake_word.py per-route 13 of 14; POST /check stays open wake_word cannot take a router-level gate: the repo owner ruled that POST /check stays open, because the local voice client calls it before any user session exists. So the other thirteen are gated per-route and the module docstring states the exception and its reason, which is what #15745 AC1 asks for -- "gated, or intentionally open with the reason stated in the file". That docstring is load-bearing, not decoration. The enumerator's `gated` is ANY-route: `classify()` appends a mechanism the moment it finds one gate name. So a router gated on 13 of 14 routes reads as fully GATED and the open route becomes invisible. The marker makes the verdict GATED+intentional -- the same state `api/system` is in -- which is accurate rather than hole-hiding. KNOWN_UNGATED only shrinks, and now does by exactly these four. Deliberately NOT in this change: - api/frontend_config. It was ruled "intentionally open -- document it" on the premise that the SPA needs it before login. It returns `NetworkConstants.get_host_configs()` (autobot_shared/network_constants.py:226), which lists every VM's internal address and port. A marker saying "open by design" over that would record a disclosure as a decision, so it stays in KNOWN_UNGATED until the decision is revisited with that fact. - api/transcriber (#15758) and the three knowledge routers: different risk. The first needs its DEFAULT_USER fallback replaced with an ownership check; the others need tenant scoping, not just authentication (#15745 AC4). Separate PRs. - #15738: user CRUD is authenticated; its defect is missing role/ownership checks. Authz, not ungated. Verified before commit: - enumerator on this tree: redis/developer/knowledge_sync GATED (router-level), wake_word GATED+intentional; its ungated set equals the new KNOWN_UNGATED exactly, so the stale-entry and new-entry checks both hold - all four files: `check_admin_permission` and `Depends` imported (checked by AST, not by eye), parse, isort --check-only and black --check clean - no import cycle: auth_middleware imports nothing from api/ or services/, and four services/ modules already import it - zero frontend callers for redis, developer, wake_word; knowledge/sync appears only in the generated OpenAPI types, not a call site - knowledge_sync_service.py 567 lines against MAX_LINES 600; no ceiling needed
…point at modules with headroom (#16178) The hook read each changed file whole. So editing one unrelated line of api/auth.py for #15757 made the commit answerable for a local BaseModel that was on base at :240 and belonged to another commit, and the instruction it printed named seven schemas_*.py modules that are all frozen at their size ceiling. The rule and its remedy could not both be satisfied. Scoping. The hook takes --changed-lines-only. With no --base it reads the staged diff: the local commit stage, which is where #15757 was blocked and which #13950's CI-only precedent deliberately left whole-file. With --base R it reads R..HEAD, for the PR stage. A model counts only if its class line was added. A pre-existing model is listed as "not counted" instead of hidden. A git failure fails the run instead of reading as "added nothing". The staged/range hunk parsing is added_lines() in tools/lint/_scan_helpers.py, next to the git plumbing that already runs with a scrubbed environment. No Python hunk parser existed anywhere in the repo. Wrapper defect. check-pre-commit-hook-pr.sh accepted --changed-lines-only together with --python and silently dropped it: validator mode never scoped, so a caller asking for scoping got a whole-file run with no sign of it. It now passes --changed-lines-only --base <resolved base> to the validator. A validator that cannot scope rejects the unknown flag, which is loud. Guidance. The fixed list of destinations is replaced by every schemas_*.py module's headroom, computed from the size guard's own MAX_LINES, KNOWN_LARGE and count_lines loaded by path, never copied. Frozen modules are marked: add a companion schemas_<domain>_<topic>.py instead. That is the owner's ruling on AC3, and the hook's docstring now records it as the convention. Tests. The scoping is driven through run() against a real throwaway git repo, because what is under test is the diff: an unrelated staged edit passes, a newly added model fails, the same pair holds in --base mode, and a missing repository fails closed. The wrapper's pass-through tests live in a companion file, because check-pre-commit-hook-pr_test.py is at its 747-line ceiling; its repo helper is loaded by path, not copied. Without --changed-lines-only the hook behaves exactly as before apart from the message. The .pre-commit-config.yaml and code-quality.yml changes that switch it on are held back pending the coordinator's OK, since both are shared files outside the scope boundary set for this issue.
…ommit and PR stage (#16178) .pre-commit-config.yaml gives the no-local-schemas entry --changed-lines-only, so the commit stage judges only the models a commit added, read from the staged diff. That is the stage where #15757 was blocked. A direct run of the hook still reads whole files. code-quality.yml gets the hook's first CI step, through the wrapper with --changed-lines-only --python. The wrapper passes the base it resolved and the hook scopes itself. Before this the hook never ran in CI at all. Path-filter coverage was checked rather than assumed. The step sits in the code-quality job, which is gated by code-quality-paths.yml's backend filter. That filter already covers everything the step reads: **/*.py (the hook, _scan_helpers, the size guard and its ceiling data, api/), pipeline-scripts/** (the wrapper), tools/lint/**, and scripts/lib/** (the git-scope.sh the wrapper sources). No python_filter_uncovered_reads.py record is needed: that record covers repo_tests guards, and this adds none. Both files were approved by the coordinator for this issue. They are shared files, and no other in-flight branch touches either.
…eet's addresses (#15745)
…it carries (#16178) added_lines() diffed only the file being checked, and a diff limited to one path cannot pair a rename with its source. A `git mv` of an API file therefore read as wholly new, and every pre-existing model in it was reported as introduced by the change. That broke #16178's first criterion: a pre-existing violation must not fail a change that did not introduce it. The rename source is now found from the whole change (--name-status -M -z, so paths containing spaces or quotes survive). The pair is then diffed together, so a move adds only the lines it actually changed. Below git's rename-similarity threshold the file still reads as new, which at that point is what it is. The two git calls share _git_diff(), which runs in the scrubbed environment and raises on failure, so a scoped run still fails closed and never reads a git error as "nothing added". Tests: a pure move passes; a move that also adds a model still fails, so following the rename cannot hide one; and the same rename holds in a committed range.
…-ref and --all-files (#16178) enforce-precommit.yml runs every pre-commit hook in CI: with --from-ref HEAD^1 --to-ref HEAD^2 on pull requests, and --all-files on pushes. Findings there are warnings only. Neither mode stages anything. So the --changed-lines-only this PR added to the hook's pre-commit entry would have diffed an empty staged change in both, and reported every model, new ones included, as not counted. That is a scoped run passing everything against nothing, the failure this PR exists to remove. The blocking code-quality step passes --base and still caught new models, so the damage was limited to the warning-only job. It was still wrong. Two rules close it: - A base pre-commit provides is used. It exports the range as PRE_COMMIT_FROM_REF (pre-commit 4.6.0, commands/run.py:391), so the hook diffs FROM_REF..HEAD, which describes the checked-out tree it reads. An explicit --base still wins. - In staged mode, a file with no staged change at all is not part of the commit, so there is no change to scope it to. It is judged whole-file and reported as such, never as "nothing added". staged_paths() in _scan_helpers answers that with one git call, through the same scrubbed _git_diff() the rest of the scoping uses. Found from #16250, which noted that enforce-precommit runs every hook. That also means this PR's claim that the hook never ran in CI was false: it always ran there as a warning. I had searched the workflows for the hook's name, and a job that runs all hooks names none. The code-quality.yml comment is corrected to say it had no *blocking* CI step. Tests: an unstaged file is judged whole and says so; PRE_COMMIT_FROM_REF becomes the base, an explicit --base overrides it, and with neither the staged diff is used.
#15745) Review of 74a5abd found the docstring's reason false. It said the fleet host list is "served by the admin-gated /system/frontend-config instead, which is where the terminal reads it". api/system.py contains no `hosts` and no get_host_configs at all -- that endpoint never carried a host list. What actually happened: the terminal calls getBackendConfig(), which fetches /system/frontend-config and, on any non-OK response or exception, returns `this.config` -- the merged PUBLIC payload from /api/frontend-config (AppConfig.js). So admins always got DEFAULT_HOSTS, and non-admin callers got the backend host list through that fallback. After 74a5abd they get DEFAULT_HOSTS too. Nothing breaks, but it is a behaviour change, and I had claimed nothing consumed the field: I traced useTerminalStore to getBackendConfig and stopped, without reading the fallback in its body. - the false sentence is rewritten; the marker phrase the enumerator matches ("no authentication required by design") is unchanged - the two `# Issue #372 ... host configs` comments orphaned by the removal go Not done here: api_endpoint_migrations_test.py still asserts assertIn("hosts", source), which 74a5abd made false. That file is skipped at collection, so the assertion never runs -- and editing it trips no-local-schemas on four pre-existing test-fixture BaseModels, which do not belong in production schema files. Filed separately rather than fixed with the wrong move. Whether DEFAULT_HOSTS itself ships internal addresses in the anonymous bundle depends on the build's VITE_*_HOST values -- #16258.
…hemas-changed-lines
…t it (#16178) Review nit on #16241, applied as the reviewer specified it. _resolve_base trusted any PRE_COMMIT_FROM_REF in the environment, so a value left exported in a developer's shell would re-scope a plain run to a stale range. pre-commit always sets PRE_COMMIT=1 for its hooks, and the range is now read only when that is present. An explicit --base still wins. The failure it closes was 'scoped to the wrong range', not a silent pass: a git error on a bad ref still fails closed. It is cheap enough to close anyway. Test: a PRE_COMMIT_FROM_REF with no PRE_COMMIT=1 is ignored.
…d refuse writes under a lapsed claim (#15950) AC4 and part of AC6, per the owner's rulings on #15950. AC4: progress-gated renewal. A run that was alive but hung renewed its claims forever, because "alive" only meant "the coroutine has not returned". Renewal now stops once the run has reported no progress for the stall window, the run is marked lapsed, and its claim expires at TTL. A renew that finds the claim already gone lapses it the same way. Progress is recorded at checkpoints every run passes through, and none of them is in a frozen file: - BaseProvider.chat_completion, where every provider the registry builds finishes a model call. Ollama and vLLM reach it too, through the ollama_provider and vllm_base subclasses that wrap the standalone classes. services/llm_service.py (1263/1263) and llm_shared/provider_registry.py (788/788) are both at their size ceilings and are not touched. - ToolSDKRegistry.execute, after every SDK tool run. - require_held, since a write that passes the check is progress. The legacy tool handler table inside the frozen tool_registry.py is not instrumented. An agent loop still returns to the model each turn, so only a single legacy tool call longer than the whole window can lapse a working run. The window is max(AUTOBOT_WORK_CLAIM_STALL_INTERVALS x renewal interval, 2 x AUTOBOT_LLM_REQUEST_TIMEOUT). It is tied to the configured timeout, not a number, because a model call reports progress only when it finishes: a window shorter than one call would lapse a run doing exactly what it should. The per-run record is autobot_shared/coordination/run_progress.py, a ContextVar. That makes it reachable from the provider and tool layers, which must not import from agents. Tasks created inside a run copy the context, and nested runs pass progress outward. AC6, partly: require_held() at a declared write site, per the owner's table: held -> allow; lapsed or lost -> refuse (ClaimNotHeld); degraded (registry unavailable) -> allow and log; no claimed run at all -> allow with a named warning. The last row exists because only the agent_client path holds declared scopes. workflow_runner and the LLC adapter call process_request directly (#16269). Refusing there would break live writes, and the warnings measure which paths really carry them. The KB librarian's add_knowledge, the only declaring write site, calls it first. Also corrects the module docstring's claim that cancelling the renewer in `finally` frees a hung executor's scope. A hung run never reaches `finally`, which is why AC4 exists. Tests: a progressing run keeps its claim; a silent one lapses and stops renewing; progress from a spawned task and from a nested run reaches the outer run; the require_held table row by row; and the guard AC6 names. The KB librarian refuses add_knowledge under a lapsed claim and writes under a live one. AUTOBOT_WORK_CLAIM_STALL_INTERVALS is registered, and its CLAUDE_RULES.md row matches the generator's format (count 219 -> 220). AC1 is blocked by #16173 (NPU's file is at its ceiling), AC3 by #16174, and the rest of AC6 by #16269.
… exemptions (#15745) core_router_auth_guard_test.py keeps its own exemption list, separate from KNOWN_UNGATED, and test_every_recorded_exemption_is_still_ungated failed on redis, developer, wake_word and knowledge_sync: this PR gated all four, so their entries recorded violations that no longer exist. They come off as a consequence of the gates, the way #15794 removed agent_org. frontend_config stays ungated and moves to _INTENTIONALLY_OPEN, because the owner ruled it open by design: it now records a decision rather than a pending fix.
…bound the stall window by the token wait too (#15950)
…e summary, direct-mode and fail-closed tests (#16191)
…ord who wrote config (#16278) Nine routes in api/settings.py (GET and POST /, /settings, /backend, /config, and POST /clear-cache) carried no auth dependency. /api/settings is in the service-auth EXEMPT_PATHS, so nothing covered them: the deployed backend answered an anonymous GET /api/settings/ with 200 and the full merged config, and POST wrote config, recording every write as created_by="admin". settings.py is frozen at its file-size ceiling, so the nine routes move to api/settings_config.py (the password_change.py precedent, #15743), which settings.py includes. Every path is unchanged. Each route is gated by require_settings_admin, a declared dependency on check_admin_permission that returns who a revision records: the admin username, or "internal-service" for the #1145 key. settings.py drops from 1105 to 882 lines and both ratchet ceilings follow. settings_route_posture_test.py pins every settings route posture from the Dependant tree (the #15737 method). The router-level guards counted this router as gated because its rbac routes are. settings_config_test.py proves the wiring: refused callers (401, 403) never reach config, and an admitted write records its author. The agent registry Settings tab reads and writes the same config, so only admins see it. The chat startup read of /api/settings/ has no consumer and already falls back to defaults. Owner ruling 2026-09-11: admin-only for read and write.
…ke /my-teams reachable (#16276, #16277, #15738) Teams (#16276): no route in teams.py checked a role, and TeamService checks none, so any logged-in org member could create, rename and delete teams and change membership. Every route but /my-teams is now admin-only, matching the SLM backend ADMIN_USERS_WRITE gate on /api/autobot-teams (owner ruling 2026-09-11). On list and create the admin gate runs before require_org_context, so a refusal opens no session (#15805). /my-teams stays login-only: it returns only the caller own teams. Route order (#16277): GET /{team_id} was declared before GET /my-teams, and {team_id} matches the segment my-teams, so every /my-teams request failed UUID parsing with a 422. /my-teams is now declared first. Users (#15738): create, delete, assign-role and revoke-role are admin-only; get and update are self-or-admin through the new dependencies.require_self_or_admin, which authorize_password_change (#15743) now delegates to rather than keeping a second copy of the rule. The posture table changes as a consequence of the gates, as its guardrail asked. user_management_gates_test.py drives each gate through FastAPI: a member gets the platform-admin 403 on all 13 admin-only routes and on another user account, while the owner, an admin, and /my-teams pass. teams_route_order_test.py asks Starlette which route GET /teams/my-teams reaches.
…ch (#16279) search_users_for_sharing needed no login and searched through a hand-built TenantContext(org_id=None, is_platform_admin=True), so an anonymous caller could list names from every organisation; the deployed backend answered it with 200. It now depends on require_org_context (login plus an org, 400 without one) and searches through get_user_service, whose apply_tenant_filter confines the query to the caller org. Owner ruling 2026-09-11. The posture table records it as authenticated, which leaves no open user-management route, so the open-routes test becomes a check that none is open. user_management_gates_test.py covers the anonymous (401) and no-org (400) refusals, that the search runs under the caller org context, and that the user query binds the caller org.
…ory root, where it would pass unexamined (#16191)
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.
…hemas-changed-lines
… guard can resolve them (#16191)
…ue-15745-auth-gates
…_routes, not .routes (#15745) router_routes_traversal_test failed #16240 on shard 7. #16278 gave the settings router its first include_router (api/settings_config.py), and the user-management router already includes its child routers. On the FastAPI CI pins, include_router defers: a parent .routes holds one wrapper per child, so settings_telemetry_test._registered_deps and the new teams_route_order_test would walk wrappers and find nothing -- vacuous, or silently wrong. Both now walk effective_routes(), the one shared traversal (#15093). teams_route_order_test also strips each route known_prefix before Route.matches: on the deferred shape a child route path lacks the including router prefix. effective_routes descends into children in place, so its order is Starlette dispatch order and "first match" keeps its meaning.
…x, set once for every image (#16291)
…ers before detection so they cannot split a refused phrase (#15700)
…ort 'could not check' apart from advisories (#16337) The Security Scan step called npm's audit service twice; the gate was the second call, so an endpoint failure read exactly like a high advisory. pipeline-scripts/npm_audit_gate.py makes one call (npm 11, which has no audits/quick fallback), retries it while the service is unavailable, and ends in passed / advisories found / could not check, each named in the job summary and annotation. Fixture tests cover all three.
…e that under-matches (#16334) hv_stale_baseline_entries now emits claimed|found|key, and --audit-baseline lists 'matched 0 of N: delete this entry' separately from 'matched k of N: lower it to k, do not delete'. Calling both 'no longer match anything' made #16298 delete an entry that still covered a live finding.
…check' (#16337) Review of #16357: an undecodable byte in npm's output raised UnicodeDecodeError and any uncaught exception exited 1, the 'advisories found' code, with no summary; a missing or non-numeric severity count read as 0 and passed. npm output is decoded with errors='replace', an exception inside the gate is reported as unavailable (exit 2) with its traceback, and every severity count must be a non-negative int. Fallback tests cover all three env constants.
…e framing, and refuse forget all previous (#15700)
…word the audit's two cases apart (#16334) 4b7c40c (#15732) C-sorted the baseline's 48 header lines, scrambling the rules the file carries; the same 48 lines are put back in their 0c3a04a order, body untouched. The header and HARDCODING_PREVENTION.md now describe --audit-baseline as it reports: delete an entry that matches nothing, lower one that under-matches.
…ode, and that crashes reach stderr (#16337) Review of 9174a08: real OSErrors (report into a missing directory, summary path a directory) leave passed/found/unavailable at 0/1/2; the crash test asserts the traceback reaches stderr; a real subprocess emitting a non-UTF-8 byte beside an audits/quick line is decoded (errors='replace') and reads as unavailable.
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Contributor
Owner
Author
|
Train-a red at
Both read as genuinely deliberate-time sites, not guard-dodging. Fix: add both to ("autobot-backend/tests/agents/test_scope_enforcement.py", "test_a_run_that_keeps_reporting_progress_keeps_its_claim"): 1, # elapsed time past the stall window is the subject
("autobot-backend/tests/agents/test_scope_enforcement.py", "test_a_run_that_goes_silent_lapses_and_stops_renewing"): 1, # a renewal interval must pass with nothing happeningDropped from this train; the other 8 members are unaffected. |
Owner
Author
|
Red on #16273's own defect (root cause above). Rebuilding without it as train-a2. |
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.
Test vehicle, do not merge. This PR runs CI once over the combined state of the train's PRs. It is closed once they have landed.
Thinking Path
Owner-approved manual merge train (#15937, comment 5632692837). After a session restart, the coordinator (autobot-ai-a7, formerly "Master — issues scope and solution order") re-established the standing PR-watcher/reviewer/merger role and asked to land the overnight green backlog. This is the first train from that sweep: 9 PRs, each independently re-confirmed green at its current head, checked pairwise conflict-free via
git merge-tree.What Changed
Branch
train/2026-09-12-a, built fromDev_new_guiataffea4b38athrough the repository merges API. All 9 merged without conflict.241060e99403cf17e0efee4a16a88dfdfab4dcf9d2db9ee40dd5f1bfae22fdb5d936865536b73ee2cf119a4cb5Held out, per the coordinator: #16284 (real duplication-guard clone, fix in progress), #16413/#16386/#16351 (fixes in progress, new SHAs pending). #16199/#16362 rebase on #16349's baseline once it lands, so they ride the next train.
Verification
This PR's own CI run is the verification. The merge burst refuses to start unless the four-layer pre-flight passes on this tip and base is still
affea4b38a. It then squash-merges each member in table order, with--match-head-commitand an explicit trailer-free message, and requiresDev_new_gui's tree to equal this tip's tree. Nothing was run locally.Model Used
Claude Opus 5 (the merge-owner session).
Refs #15937
Refs #16349
Refs #16273
Refs #16241
Refs #16358
Refs #16365
Refs #16357
Refs #16240
Refs #16345
Refs #16356
🤖 Generated with Claude Code