Repository navigation
Conversation
…ous callers owning every recording (#15758) Every transcriber route resolved its caller with a local `_user_id(request)` that returned `request.state.user.id`, or DEFAULT_USER ("default") when unset. Nothing in production set `request.state.user`, so every caller -- with or without credentials -- was "default", and could read, edit and delete every row stamped with that owner. `can_access` already carried the #9968 fix, which is correct given a real caller; the defect was that it never received one. The fix is the production half the tests already assumed. Their fixtures set `request.state.user` from a header; nothing did that for real requests. transcriber/deps.py authenticate() router-level dependency: runs get_current_user, records the resolved id and whether the caller is an admin caller_id_of() replaces six copied _user_id forks; fails closed with 401, never falls back can_access() unchanged for users; admins may reach DEFAULT_USER rows (repo owner ruling) resolve_user_id() moved here from api/transcripts.py so there is one resolver, not two All seven sub-routers now mount `authenticate` -- including providers.py, which had no _user_id but exposes PATCH /providers. The enumerator judges an aggregator only by what it mounts, so gating api/transcriber.py itself would have authenticated everything while the guard still reported UNGATED. Existing recordings. Because no route ever resolved a caller, every recording ever written is stamped DEFAULT_USER (reasoned from code, not checked against data). #9968's rule -- no real user may reach DEFAULT_USER rows -- would make the whole dataset unreachable, so the repo owner ruled that admins keep access. The can_access docstring is rewritten to record that ruling rather than silently contradict the one it carried. Admin listings include DEFAULT_USER projects; without that an admin could reach a row by id but never find one. `_require_recording_owner` compared owners itself, forking the policy that can_access's own docstring forbids (#9863). It now goes through can_access, which is also what carries the admin path to its seven call sites. Tests. Two suites set no identity at all and passed only because every caller became "default" -- test_recordings_api.py:222 built a DEFAULT_USER project and reached it anonymously, i.e. it tested the vulnerability. They now use a fixed test caller. The five suites whose middleware sets identity get a strict no-op override of authenticate, not a fallback identity, so any test that sends no identity and expects success fails rather than passing on the old path. New tests: every sub-router carries authenticate; caller_id_of is a 401 with no identity; authenticate records the admin flag; an admin reaches DEFAULT_USER rows and no other row it does not own. Not in this change: - PATCH /providers changes a process-wide setting from a control the UI treats as a personal preference. Gating it admin-only would break AsrProviderSelector for every non-admin, so it is authenticated here and the design question is #16254. - resolve_user_id prefers user_id and falls back to username, so a principal with no user_id whose username is literally "default" would own every legacy row. Not verified whether any real principal lacks a user_id. Verified before push: enumerator reads api.transcriber GATED (7/7 mounted routers gated), its ungated set equals KNOWN_UNGATED, every control passes; flake8 F401/F821, isort and black clean; no _user_id or DEFAULT_USER fallback left in any route. The no-blocking-io-in-async hook (#7444) flagged three pre-existing synchronous file reads/writes inside async tests in the two test files this touches (test_export_api.py, test_recordings_api.py). Wrapped with asyncio.to_thread as the hook recommends, rather than suppressed with its noqa, which it reserves for code that genuinely runs in a thread.
…uthenticates (#15758) test_legacy_default_rows_not_accessible_to_other_users created its DEFAULT_USER row with an anonymous POST. Under #15758 that POST returns 401, so the test read a missing id. The row is now seeded through a db fixture; the #9968 assertions are unchanged. The orchestrator upload test made header-less requests under a no-op authenticate override, which would also return 401. It now uses the same stand-in identity as test_projects_api. This corrects the previous commit's claim that no test relied on the anonymous path. Two did: CI's authz step caught the first, and a scan of every test client call found the second.
… that it is gated (#15758) core_router_auth_guard_test.py records the transcriber in _TRACKED_BY_OTHER_ISSUES as ungated. This PR gates every transcriber sub-router with Depends(authenticate), so test_every_recorded_exemption_is_still_ungated would fail on the entry the way it failed on #16240 four routers. It comes off as a consequence of the gate, as knowledge_cognition did when #15759 landed.
…ders admin-only, and test the gates through the routes (#15758) Closes the #16259 review gaps. test_route_authz_15758.py adds cross-user 404s for ai/ask, kb/push and kb/status (with an owner contrast on kb/status), and a real anonymous 401 from one route of every sub-router with authenticate left in place (every other fixture overrides it), plus a signed-in contrast. The review question: user management accepts "default" as a username (^[a-zA-Z0-9_]+$), and resolve_user_id falls back to username when a principal has no user_id, so a real account could have owned every pre-#15758 row. resolve_user_id now refuses a principal that resolves to DEFAULT_USER with 403, which also covers any account already so named. PATCH /providers switched the process-wide cloud ASR provider for any signed-in user, so one user could route everyone audio to another provider. Owner ruling 2026-09-11: admin-only, via a new require_admin dependency; GET stays login-only (metadata, never keys).
…s, and reserve "default" at account creation (#15758) Review of #16259. PATCH /providers became admin-only, but TranscriberLayout.vue offered its settings surface (whose only control is AsrProviderSelector, which calls PATCH /providers) to every user, so a non-admin got a raw 403. The settings toggle and panel now render for admins only (useUserStore().isAdmin); no new strings. The layout existing hardcoded labels are #16335. UserCreate and UserUpdate now refuse the reserved username "default" through one shared check, so the clash with the transcriber legacy owner id surfaces when the account is created or renamed, not on first transcriber use; resolve_user_id keeps its request-time refusal for any account that already exists. test_reserved_owner_id.py pins that DEFAULT_USER stays in RESERVED_USERNAMES.
… email (#15758) someone@example.test failed EmailStr validation, because email-validator rejects the special-use .test domain. That broke the ordinary-username contrast test, and it also meant the "reserved" tests passed partly for the wrong reason: pydantic reported the email error alongside the username error. The tests now use example.com, as users_duplicate_conflict_test.py does, so each refusal is the reservation alone.
…ername fixture (#15758) The whole-tree Secret Detection check (added by #16300) flagged a Secret Keyword in user_reserved_username_test.py: its fixture carried "password": "Str0ngPassw0rd". UserCreate.password is optional (SSO accounts) and these tests exercise only the username, so the fixture now omits it. No .secrets.baseline change, keeping clear of the #16343/#16349 line churn.
…t adds the backend key (#16374)
…otocol, never the URL (#16374) Coordinator round-2 review of 3d73ef5: High (JWT in the WebSocket URL, persisted to the vhost access log and browser history) and Low (the query fallback authenticating every /autobot-api/ request, not just the stream). - Frontend: `new WebSocket(url, ['bearer', token])` replaces `?slm_ws_token=` (ProcessMonitorTab.vue + test) -- JWT characters are valid RFC 7230 tokens. - nginx (autobot-slm.conf.j2): the stream path gets its own regex location with a dedicated auth_request target that reads Authorization from Sec-WebSocket-Protocol (`map $http_sec_websocket_protocol ...`), strips the subprotocol down to "bearer" before proxying, and never forwards the query string. The general /autobot-api/ auth_request goes back to reading Authorization directly, with no $arg_ fallback. - Backend (process_management.py): stream_process_logs echoes the "bearer" subprotocol on accept() (RFC 6455 4.2.2), matching the convention already used by autobot-slm-backend/api/websocket.py; new test covers both offered and no-offer cases. - Guard (nginx_internal_api_key_auth_gate_test.py): new find_query_string_derived_credentials assertion -- no auth map or auth_request target may derive a credential from $arg_*, with planted cases for both the map and the direct-header shapes.
…gin (#16374) Coordinator round-2 review of 3d73ef5, second High: /_slm_session_check and /_slm_stream_session_check gated the /autobot-api/ internal-key injection on GET /api/auth/me, which proves only that a token is a *valid* SLM session. Backend login tokens are valid SLM sessions by design (epic #10193), so a read-only or non-admin backend user's own token passed the gate and nginx attached the admin-equivalent X-Internal-API-Key on their behalf -- #16386 turned "any visitor becomes admin" into "any backend user becomes admin". - SLM backend: new GET /api/auth/proxy-check (204, no body), gated by the existing require_admin dependency (services/auth.py). Its upstream HTTPBearer() already answers a missing Authorization header with 401 (not 403, checked against the installed fastapi release), and get_current_user raises 401 for an invalid/expired token, so require_admin only ever adds the "authenticated but not admin" 403 -- no new dependency needed. - nginx (autobot-slm.conf.j2): both internal check locations now proxy_pass to /api/auth/proxy-check instead of /api/auth/me. - Tests (test_auth_proxy_check.py): real get_current_user/require_admin round-tripped against real minted JWTs -- admin passes, readonly/user get 403, garbage/expired tokens get 401, a missing Authorization header (real fastapi HTTPBearer, not mocked) gets 401. The endpoint itself is extracted via an identity-decorator exec of api/auth.py and asserted to return None for an admin caller, plus a static pin on its status_code=204 declaration. - Guard (nginx_internal_api_key_auth_gate_test.py): new find_session_only_auth_request_targets assertion -- no auth_request target gating a key injection may proxy to /api/auth/me, with planted cases for the defect, the fix, and a to-be-ignored unrelated /api/auth/me location. PR note: non-admin SLM users lose every /autobot-api/-backed feature -- the SLM's admin Settings pages (cache, prompts, NPU workers, user management, log forwarding, backend settings, LLM fallbacks, budget audit) and admin Tools (agents, batch, browser, voice, vision, MCP, file browser, advanced control), plus the agent Process Monitor/Config History/Org Chart tabs, the Redis service panel, and Shared Links -- since all of them read the main backend through useAutobotApi.ts's /autobot-api/ proxy. The SLM is already an admin console, so this is the intended narrowing, not a regression.
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.
…filesystem walk (#16374)
…gacy admin flag (#16374)
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.
…16501) sync_config and update_hardware_priority in api/settings.py now depend on require_settings_admin (from api.settings_config) instead of check_admin_permission directly, and pass its returned actor identity to create_revision's created_by -- matching the fix settings_config.py's own routes already got under #16278, whose audit-trail criterion this sub-issue completes. The new import pushed the grandfathered settings.py from 882 to 883 lines; ceiling raised to match in both size-ceiling files.
…maDB-backed semantic cache (#16528) SemanticLLMCache (llm_shared/semantic_cache.py) had zero production callers — referenced only from llm_shared/__init__.py's re-export and its own test. The live semantic-cache path is services/semantic_query_cache.py (SemanticQueryCache), wired into services/rag_service.py and api/cache_management.py. Checked whether SemanticLLMCache's numpy-cosine fallback was worth porting in case ChromaDB is unavailable: SemanticQueryCache already degrades gracefully in that case (_get_or_create_collection returns None, _initialized stays False, lookup()/store() short-circuit to None/False on the first check) with no exception and no lost correctness, only the cache-hit optimization. Wiring the semantic cache into LLMService.chat() was itself evaluated and explicitly not adopted when the exact-cache path was wired in #10619 (closing #10597) — this is settled supersession, not neglected work, so there is no real capability to merge forward. Deleting is the correct resolution. Removed llm_shared/semantic_cache.py and its dedicated test (tests/test_semantic_llm_cache.py), dropped the SemanticLLMCache export from llm_shared/__init__.py, and removed the now-dead conftest.py stub-wiring block that loaded it for tests. conftest.py shrank below its grandfathered file-size ceiling, so the ratchet in scripts/python_file_size_known_large.py and its mirror in repo_tests/python_file_size_ratchet_baseline.py are lowered from 1580 to 1570 lines to match.
send_to_browser_vm already runs a DNS-resolving public-address check (a7afad8); services/playwright_service.py::capture_screenshot did not, and posts a caller-supplied url straight to the container's /test-frontend endpoint. Guard it with the same autobot_shared.url_safety.is_public_url_async check. test_frontend/send_test_message stay unguarded on purpose: their frontend_url defaults to the internal frontend service and is meant to reach it, matching the admin-surface exemption pattern already documented in api/browser_mcp.py. search_web takes a query string, not a url, so it was never an instance of this gap.
update_agent_model, enable_agent and disable_agent were gated by check_admin_permission but discarded which admin (or the internal-service key) it admitted before writing the audit revision as the literal created_by="admin" -- the same defect #16278/#16501 fixed in api/settings.py. Reuses require_settings_admin, already generic and already reused across files for exactly this.
…ks (#15143) Four of five branches named an env file that does not exist (.env.wsl-docker-desktop, .env.linux-native x2, .env.distributed), so the script always fell through to "using defaults" silently regardless of which environment it detected. wsl-docker-desktop already had the right file under a wrong name -- .env.localhost matches its own "using localhost for service access". The other three (linux-native, wsl-native-docker, containerized) have no unambiguous match among the three real modes this repo tracks (localhost, native-vm -- a genuinely distributed multi-VM deployment, network): guessing one would point services at the wrong host silently, so they fail loudly instead, naming the tracked set. AUTOBOT_ENVIRONMENT now prefers an already-set value over detecting one, so a test can force a branch without faking /proc/version, /.dockerenv or the docker socket.
…enamed Dev_new_gui (#16528) The base branch was renamed Dev_new_gui -> main, but docker-smoke-test.yml and its required-context shim (docker-smoke-required-context.yml) still triggered only on `branches: [Dev_new_gui]`. Since `smoke-test` is a required status check on main, every PR targeting main (including this one) was structurally unable to get it to report at all -- not a failure, an absence -- which left mergeStateStatus stuck on BLOCKED regardless of how green everything else was. A broader repo-wide rename PR (#16487, unmerged) already carries the identical one-line fix for these two files among 204 others. Applying it here too, narrowly, unblocks this PR immediately without waiting on that unrelated PR's review/merge timeline, and converges to the exact same content either way -- so it cannot conflict when #16487 eventually lands.
…actor' into issue-16547-agent-config-created-by
…o guard (#16541) Merged in #16504's fix (require_settings_admin wiring for api/settings.py's sync_config and update_hardware_priority) so this branch carries both halves of the same defect class. Neither route had a test pinning the recorded actor -- only the shared require_settings_admin dependency was covered, through a different route (settings_config_test.py's POST /). Added settings_sync_hardware_priority_created_by_test.py mirroring that file's exact pattern (force the internal-service-key branch via verify_internal_api_key, read back create_revision's created_by kwarg). Added repo_tests/no_created_by_admin_literal_test.py: an AST-based guard (not grepped, so it can't flag its own docstring) over every tracked non-test .py file under autobot-backend/ and autobot-slm-backend/, failing on any created_by="admin" keyword argument. Zero occurrences remain as of this commit -- this keeps a third from landing the same way #16278/#16501 and #16547 each did.
#15163) gpu_vector_search.py's GPU-accelerated similarity search (#387) has been unreachable since introduction: FAISS_GPU_AVAILABLE depends on faiss.StandardGpuResources, which only a faiss-gpu build provides, and every automated install path only ever installed faiss-cpu. requirements.txt's own comment pointed at conda, which no deploy path uses. PyPI's faiss-gpu 1.15.0 (Aug 2026) is a real pip-installable wheel with CUDA/ROCm support, so this follows requirements-gpu-torch.txt's exact shape (#15162): new requirements-gpu-faiss.txt, installed by the ansible backend role whenever backend_gpu_available is true, independent of the vLLM opt-in. Unlike torch (one package name, CPU vs CUDA build selected by index-url), faiss-cpu and faiss-gpu are two different package names that both provide the top-level `faiss` module -- installing faiss-gpu on top of an existing faiss-cpu risks their files conflicting, so faiss-cpu is explicitly removed first, order-tested. gpu_vector_search.py's initialize() now logs distinctly when GPU was requested but faiss-gpu isn't installed, rather than falling silently back to CPU indistinguishable from "no GPU present" or "GPU not requested" -- _initialize_gpu_index already logs the third case (GPU attempted, no CUDA device) via its existing exception handler. New repo_tests/backend_gpu_faiss_gating_test.py mirrors backend_gpu_torch_gating_test.py's (#15162) exact pattern: parses the real ansible task list and requirement pins, never raw substring matches against the whole file.
…#16541) Review found: the guard declared floor=3000 against a population of 3514, and Reach.verify_floor requires population - floor <= skips + growth -- 514 > growth(300), so reach_declarations_test.py's own meta-test would fail in CI. Pinned floor=3514 (the exact population), matching sibling declarations: growth absorbs ordinary churn, it is not a second safety margin under the floor. Also adds the internal-service-key case to agent_config_created_by_test.py for all three agent-config routes (#16541 AC2), mirroring settings_config_test.py's TestAnAdmittedWriteRecordsItsAuthor pattern -- previously only the admin-session branch was covered.
…to tmp-merge-16259 # Conflicts: # repo_tests/router_auth_coverage_test.py
…racked_paths (#16547) 37's sweep: this guard's raw subprocess git-ls-files call was the 34th direct invocation in repo_tests/ against one_git_enumeration_15926_test.py's MAX_DIRECT_INVOCATIONS = 33. Migrated to tools.lint._scan_helpers.tracked_paths, catching EmptyEnumeration and returning [] instead of letting it propagate -- that specific exception type would otherwise fail reach_declarations_test.py's empty-tree case, which requires ReachFloorError specifically (from Reach's own floor check), not whatever exception discovery happens to raise. This is the same tension one_git_enumeration_15926_test.py's own docstring names for audio_extension_allowlist_test/env_var_bare_cast_test, solved here by catching the one exception tracked_paths raises for "listed nothing" rather than leaving the call unmigrated. Verified directly: the real-tree population is still exactly 3514 (unchanged, same files), the reach_declarations_test empty-tree behavior is unaffected (still returns [] before Reach's floor check runs), and one_git_enumeration_15926_test's own AST count is back to exactly 33.
) 37's sweep: _tracked_env_files() had no Reach declaration, so a shrunken .glob('.env*') population (a moved/renamed file, a typo in the pattern) would still pass test_the_tracked_set_is_not_empty as long as it stayed above zero -- the boolean check can't tell "6 files" from "1 file" the way a floor can. Gave discover a root parameter (defaulting to project_root(), matching what reach_declarations_test needs to drive it against an empty directory) and declared it with floor=6, the live population, growth=2 for ordinary churn. Path.glob() returns an empty generator on a tree with nothing to match, never raises, so no EmptyEnumeration handling is needed here -- verified directly against both the real tree (6) and a tmp empty directory ([]).
…it in the python filter (#15163) 37's sweep, two findings on this branch's new manifest: 1. ansible_manifest_resolution.py's SITE_MANIFESTS didn't name the new file: roles/backend/tasks/main.yml now installs it into the same venv the existing (BACKEND, GPU_TORCH, GPU) entry already covers. Added a GPU_FAISS constant and extended that entry's tuple, so derived_bindings() (which re-reads the ansible tree for what it actually installs) has a declared match instead of drifting the table away from the tree it describes. 2. requirements-gpu-faiss.txt is a bare repo-root filename with no slash. backend_gpu_faiss_gating_test.py's REPO_ROOT / "requirements-gpu-faiss.txt" composition (and this same file's own GPU_FAISS constant) both resolve to that bare name, and `**/requirements*.txt` cannot match it: python_filter_covers_its_guards_test.py's own matcher rewrites `**/` to `*/` before calling fnmatch, which still requires a literal `/` before "requirements" -- a root-level file has none. Added an explicit filter entry, verified directly against the actual rewritten-pattern match, not assumed from the glob syntax alone. Per the sweep's own instruction: fixed via a filter entry, not an UNCOVERED_READS allowlist addition -- that file's own rule is shrink-only, and adding a new bypass there is exactly what it exists to refuse.
…15163) Pre-push hook caught this: roles/backend's new "Remove CPU-only faiss before installing the GPU build" task (state: absent, name: faiss-cpu) made _names_a_component_package() record an INLINE_SOURCE edge for the backend venv, which already has a MANIFEST_SOURCE edge from its existing requirements: installs -- so test_no_venv_is_provisioned_from_more_than_one_source_shape read it as #15671's multi-source shape. It is not: a state: absent task names a package to remove, not a second source of what fills the venv. The manifest still fully determines the venv's contents; removing a known-conflicting package first (faiss-cpu and faiss-gpu are different package names sharing the `faiss` import) is orthogonal to that. _record_task now skips INLINE_SOURCE for state: absent tasks. Rewrapped the pre-existing 4-line comment above _record_task to 3 lines to make room within the 600-line hard cap -- this file carried no headroom at all (exactly 600 before this fix), so the fix itself could not fit without it. New test pins both directions: the absent case records no source edge, and a same-shaped install (no state:, or state: present) still does -- without that second half this fix could pass by silently never recording INLINE_SOURCE at all. Verified directly against the real tree: /opt/autobot/autobot-backend/venv now resolves to frozenset({'manifest'}) instead of {'manifest', 'inline'}.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
7 tasks done
Contributor
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
This was referenced Sep 13, 2026
mrveiss
force-pushed
the
train/2026-09-13-batch2a
branch
from
September 13, 2026 05:11
2ed60c8 to
b3df041
Compare
Closed
3 tasks done
Owner
Author
|
Batch 2a landed: all 7 members merged individually onto main (final SHA 0abe0d4). Tree diff against this vehicle branch is empty. Closing without merging. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Test vehicle only, not for merging. Combines #16413, #16259, #16386, #16536, #16550, #16553, #16560, #16544 onto post-batch-1 main (995e135) for one shared CI run.
What Changed
Merge of 8 already-reviewed PRs, no new content of its own.
Verification
The required-check suite running on this PR is the verification. No local test execution as evidence — CI is authoritative. (The local pre-push hook did run as a side effect of pushing and passed, but that's not cited as merge evidence.)
Model Used
Claude Sonnet 5 (claude-sonnet-5)
Issue Link
None — vehicle PR, closes nothing.