Repository navigation
chore(train): vehicle 2e — approved PRs (#16700, #16483, #16721, #16689, #16703, #16691, #16706, #16705, #16696) - #16746
Merged
Merged
Conversation
…nd route are hidden from non-admins (#16666)
…dependency, not the stubbed module (#16666)
… caller's access (#16665, #16654) api/knowledge_search.py's canonical search endpoint never applied ownership/visibility filtering -- any signed-in caller could read another user's PRIVATE fact through it. Threads user_id/user_org_id/ user_group_ids/is_admin from the route's current_user through all three search paths (RAG, aistack/advanced, basic+reranking) down to the three call sites that actually fetch raw results from the KB (_execute_kb_search, _search_with_all_queries, _aistack_search's three branches), applying knowledge.search_filters.filter_search_results_by_permission before any result reaches the caller. Per the owner's decision on #16654: this is an explicit read API, so an admin (is_admin_role(current_user.get("role"))) reads any fact, same as knowledge_ownership.py/knowledge_collaboration.py's existing correct callers. Test (real KnowledgeOwnership.check_access, not mocked, so it proves the route's wiring and not just that a mock was called): - user A's PRIVATE fact is not returned to user B - is returned to A - is returned to an admin via an explicit read Extracted the three analytics endpoints (search_analytics, record_click, expand_query -- none of which ever return KB facts) into their own api/knowledge_search_analytics.py: the added filtering code pushed knowledge_search.py past its grandfathered file-size ceiling (#14236, 851 lines), and that exemption freezes the size it was granted for rather than licensing more. Registered the new router in core_routers.py under the same /knowledge_base prefix, so every route keeps its existing URL. Lowered the grandfather ceiling to the file's new real size (820), since the ratchet only turns down. Shrinks 3 entries from repo_tests/kb_read_visibility_allowlist.py (_aistack_search, _execute_kb_search, _search_with_all_queries), MAX_ALLOWLISTED 84 -> 81. Refs #16665, #16654
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.
…re and cleanup; pin the admin read bypass to explicit read APIs (#16662)
… 401 anonymous, 403 user, 200 admin (#16666)
…x its form styling (#16695) Watch Folders was a standalone top-level route reached via a lone "Automation" nav group, and its Add Watch Folder dialog hand-rolled its own overlay/inputs against tokens that don't exist in design-tokens.css instead of using BaseModal and the real design tokens like every sibling form. Moved it into KnowledgeEntries.vue as a third manageTab tab (readable via ?tab=watchFolders), redirect the old route, switch the dialog to BaseModal, and i18n every string across all 11 locales.
…helper until the explicit callers land (#16662)
…atchet on the detected reads, not the allowlist's own length (#16667)
….css classes (#16695) CI's duplication-guard flagged WatchFoldersPanel.vue's local .stat-card/.stat-label/.stat-value block as a near-verbatim clone of AuditStatistics.vue's own local copy of the same pattern. Both are redeclaring classes assets/css/components.css already defines globally (loaded via main.ts) with the same design tokens, so drop the local override and let the shared rule apply. Renamed the per-folder stat rows from .stat-item/.stat-label/.stat-value to .detail-row/.detail-label/.detail-value (they already shared identical styling with the folder's other detail rows) so the global .stat-label rule's uppercase/tracking treatment, meant for the overview tiles, doesn't bleed into those inline rows too.
…ults by the caller's access (#16665, #16654) api/knowledge_search_aggregator.py's /multi-source/search, /context and /graph routes never applied ownership/visibility filtering. Threads user_id/user_org_id/user_group_ids/is_admin from each route's current_user through to every call site that fetches raw facts or relation content from the KB: - _search_facts, _get_facts_for_graph: filter_search_results_by_permission on the raw fact list (get_facts_in_category's flat fact dicts are wrapped with a "metadata" view first, since that shape differs from kb.search()'s already-nested results). - _expand_fact_relations, _process_relations_for_citations: a related fact reached via the relation graph (target_fact/source_fact) is a KB fact like any other -- filtered per-item via the new _related_fact_is_accessible helper, since a relation is not an access grant. - get_llm_context: filters the fact-search results feeding the LLM prompt itself, not just the display list. - _get_fact_relations_for_graph: not touched -- reclassified in the allowlist as SCOPED rather than removed. It receives fact_ids already filtered by _get_facts_for_graph, both ends of every returned edge are re-checked against that same set, and include_fact_details=False means it never fetches fact content -- safe by construction, not by an active check. Extracted the documentation-search subsystem (indexed AutoBot docs, not KB facts -- no visibility concern) into a new api/knowledge_search_documentation.py: the added filtering code pushed this file past its grandfathered ceiling (#14236, 967 lines). Registered the new router in core_routers.py under the same /knowledge_base prefix -- every URL unchanged. Lowered the ceiling to the file's new real size (937), since the ratchet only turns down. Updated the 3 existing tests in knowledge_search_aggregator_test.py that called the modified functions directly with placeholder user_id/is_admin values (empty kb.search() results, nothing for the filter to act on). New test (knowledge_search_aggregator_visibility_16665_test.py, real KnowledgeOwnership.check_access, not mocked) covers the /search route: user A's PRIVATE fact hidden from user B, visible to an explicit admin read. Shrinks 5 entries from repo_tests/kb_read_visibility_allowlist.py (_expand_fact_relations, _get_facts_for_graph, _process_relations_for_citations, _search_facts, get_llm_context) and reclassifies 1 (_get_fact_relations_for_graph, _T3 -> SCOPED), MAX_ALLOWLISTED 81 -> 76. Refs #16665, #16654
… routers (#16665) Both api/knowledge_search_analytics.py and api/knowledge_search_documentation.py imported KnowledgeDocumentationSearchResponse/KnowledgeDocumentationStatsResponse/ RecordClickResponse/ExpandQueryResponse/SearchAnalyticsResponse from knowledge.schemas, a different, unrelated package (knowledge/schemas/, which exports things like KnowledgeSearchResponse) -- these classes actually live in api/schemas_knowledge.py, matching the original knowledge_search_aggregator.py import this code was extracted from. Broke module import entirely (core_routers.py imports both routers directly, so this would have failed app startup), caught by the pre-push hook's own pytest collection failing on knowledge_search_aggregator_test.py rather than by me tracing it manually -- CI is still the authority here, this is the fix it asked for.
…9 headroom (#15026) llm_shared/quota_headroom.py — Redis-backed QuotaHeadroomStore, mirroring provider_degradation.py's pattern (in-process fallback when Redis is down, never raises into a caller). Key: (provider, account_id, window). account_id defaults to "default" so a future multi-account pool (#15029) can extend this without a schema change. Absence is a first-class state — no entry means "no signal yet," never fabricated as zero. llm_shared/rate_limit_backoff.py — raise_if_rate_limited() now persists a remaining=0 / resets_at reading on every 429 before raising, fire-and-forget so a store hiccup can never affect the existing raise-and-retry path. Behaviour for every caller is unchanged (all 15 pre-existing tests pass untouched) — this is additive. autobot-backend/conftest.py — real-loads llm_shared.quota_headroom (the llm_shared stub's existing per-submodule real-load list, same mechanism provider_degradation/rate_limit_backoff/base_provider already use). autobot_shared/env_registry_ai.py — registers AUTOBOT_LLM_QUOTA_HEADROOM_TTL_SECONDS. Scope note (see PR body): only the error/429 harvest path is wired here. Success-path header harvesting needs each provider's own SDK-specific raw- response access (e.g. Anthropic's with_raw_response) — deliberately not touched in this PR to avoid an unverified change to the live completion path of the most-used provider; filed as its own follow-up.
…, stop one bad entry sinking the whole store (#16483) Review: three except Exception: handlers around the Redis calls caught everything, not just Redis being unreachable, and the list/scan path's handler logged nothing at all -- one corrupt stored entry made every Redis-held entry vanish from all_entries(), indistinguishable from Redis being down. - record()/get()/all_entries() now catch only (redis.exceptions.ConnectionError, redis.exceptions.TimeoutError), matching conversation_file_manager.py's existing convention for the same client. Anything else -- a real bug -- surfaces instead of silently degrading to the in-process fallback. - all_entries()'s fallback path now logs at debug, matching the other two. - A shared _decode_entry() catches json.JSONDecodeError/TypeError/ ValueError per entry, logs a warning naming the key, and returns None for that one entry instead of failing the whole call -- used by both get() (returns None, the same "absent" state a missing key already produces) and all_entries()'s scan loop (skips just that key). The three existing no-Redis-fallback tests mocked _get_redis to raise the builtin ConnectionError, which is unrelated to redis.exceptions.ConnectionError (a RedisError subclass, not a builtin-OSError subclass) -- the narrowed catch would not have caught it. Fixed to raise the real redis exception, which is what actually simulates a Redis outage. Added two corrupt-entry tests: get() returns None (not a raise) and logs, and all_entries() drops just the corrupt key and keeps the rest.
…uteError (#16483) Review (37): get_async_redis_client() returns None -- not an exception -- when Redis is disabled or its circuit breaker is open. The previous commit's narrowed except (RedisConnectionError, RedisTimeoutError) no longer catches the AttributeError that follows from calling a method on that None, so record()/get()/all_entries() would raise instead of falling back in exactly the case (breaker open) the fallback exists for -- silently losing headroom recorded on the 429 fire-and-forget path. _get_redis() now raises RedisConnectionError itself when the underlying client is None, so all three methods share the one fallback path already there for connection/timeout errors, rather than each needing its own None check. Added a None-client test for each of the three methods, asserting the in-process fallback is used.
…omFrom429's mock (#15026) CI's shard 8/12 red: TestPersistHeadroomFrom429's mock for "Redis unavailable" raised a plain RuntimeError, but quota_headroom.py's get() narrows its except to _REDIS_UNAVAILABLE = (RedisConnectionError, RedisTimeoutError) -- a deliberate review outcome, not something to widen back open. The test's mock was simulating the wrong exception type; fixed to raise redis.exceptions.ConnectionError, matching the established pattern in test_quota_headroom.py's own mocks.
…act metadata before filtering (#16665) Two real bugs from the code-reviewer pass on #16691, both confirmed against source (and the import one against this PR's own CI): 1. knowledge_search_analytics.py's schema import went the wrong direction in the earlier "fix" commit -- SearchAnalyticsResponse/ RecordClickResponse/ExpandQueryResponse actually live in knowledge.schemas (matching knowledge_search.py's own still-correct KnowledgeSearchResponse import), not api.schemas_knowledge. CI's autofix-types job confirmed: "cannot import name 'ExpandQueryResponse' from 'api.schemas_knowledge'". 2. _get_facts_for_graph's category-filter branch and _related_fact_is_accessible wrapped the raw Redis fact hash's "metadata" field directly as the dict filter_search_results_by_permission checks -- but knowledge/facts.py writes that field as json.dumps(metadata), a STRING, not a dict. check_access() read owner_id=None off the un-decoded string every time, defaulting every fact to PRIVATE-with-no-owner-match and denying it to everyone, including the fact's own owner (fail-closed, not a leak, but a silent functional regression on /multi-source/graph's category filter and every relation-graph fact). Added _decode_fact_metadata to parse the field properly, matching how api/knowledge_ownership.py's already-correct kb.get_fact() path handles the same shape. New regression tests exercise both previously-untested paths directly: _decode_fact_metadata's own JSON parsing, and _get_facts_for_graph with a real category_filter proving the owner sees their own fact again. Lowered knowledge_search_aggregator.py's file-size ceiling to its final real size (951) rather than the earlier intermediate value (937) set before this fix's lines were added -- still well under the original grandfathered 967, and the ratchet's invariant is against what's merged on main, not against an unmerged PR's own intermediate commits.
…not line numbers (#16686)
Main gained a new AUTOBOT_* var since this branch's own env_registry_ai.py addition landed, so the autogenerated table (and its count line, 225 -> 226) had drifted behind pipeline-scripts/check_env_var_registry.py's live count.
…lections, each classified, ceiling re-frozen 84 -> 101 (#16667)
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.
…D routes from the T3 allowlist (#16665, #16654) _get_or_compute_category_counts cached one unfiltered fact count per category behind a single global Redis key, shared by every caller: a private/shared/group fact's existence leaked into a count every other authenticated user would read from that same cache entry. Restrict the counted facts to those visible to every authenticated user (the access_level bypass, else SYSTEM/PUBLIC visibility) so the one shared cache entry is a valid answer for any caller, matching #910's existing promise that the 3 top-level categories are non-sensitive public metadata. Extracted the count computation/caching (already-extracted style, Issue #185/#209 precedent) into knowledge_category_counts.py to keep knowledge.py under its #14236 grandfathered ceiling (3477 -> 3420, lowered to the file's new true size). The other 11 PR2-scope allowlist entries (knowledge_categories.py, knowledge_collections.py, knowledge_metadata.py, knowledge_relations.py, knowledge_tags.py, knowledge_verification.py, and knowledge.py's search_man_pages) are reclassified SCOPED, not filtered: every one of those routes already requires Depends(check_admin_permission) (403 for non-admins), either at the router level or per-route, and KnowledgeOwnership.check_access grants admins unconditional read regardless of fact filtering -- so they carry no T3 gap. MAX_ALLOWLISTED: 84 -> 83 (one real removal; the SCOPED reclassifications keep their keys, per the guard's own reclassification rule).
…ly on (#16665) repo_tests/kb_read_visibility_allowlist.py reclassifies 11 knowledge-CRUD functions as SCOPED rather than filtered, on the claim that every one of their routes already requires Depends(check_admin_permission). That claim was true when read, but nothing kept it true. Add one test per route, walking the real resolved Dependant tree (#15737 method, same as api/settings_route_posture_test.py) to assert check_admin_permission is still reachable -- so removing the gate later fails CI here instead of silently re-opening the T3 gap the allowlist entry waved through.
Contributor
|
Warning Review limit reachedNext included review available in 56 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (90)
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 |
Contributor
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
3 tasks
This was referenced Sep 14, 2026
Closed
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
A batch vehicle off current
origin/mainmerging 9 already-approved PR heads unchanged,each at its ledger-recorded approve SHA (verified equal to the PR's live
headRefOidbefore merging).
#16728was left out:ledger holdsshowed an activepr:16728hold("paused for the week ... lift next week"), which is the documented exclusion condition.
Every merge is
git fetch origin pull/<N>/head+git merge --no-ff <pinned-sha>; nomember's own commits were rebased, amended or edited. Conflicts are resolved in the merge
commit itself; two follow-up fix commits handle effects that only appear once several
members are combined (see below).
What Changed
Nine approved PRs merged unchanged into
vehicle/2e-2026-09-14:#16728intentionally not included — held (pr:16728, "paused for the week ... lift next week").Conflict resolutions
docs/developer/CLAUDE_RULES.md(#16483 vs. main)ENV_VARS.md; keptmain's split-doc structure (CLAUDE_RULES.mdpointer), added #16483's one new row (AUTOBOT_LLM_QUOTA_HEADROOM_TTL_SECONDS) todocs/developer/ENV_VARS.mdat its alphabetical position — the registration itself (env_registry_ai.py) merged cleanly on its own.repo_tests/python_file_size_ratchet_baseline.py,scripts/python_file_size_known_large.py(#16483)conftest.pyentry (1568 vs. 1507); realwc -lon the merged tree is 1505 (≤ both) — used that.repo_tests/kb_read_visibility_allowlist.py(#16691 then #16706)_EXPLORER/_IMPL_*/etc. from #16703 plus_SCOPED_ADMIN_ROUTERfrom #16691); droppedMAX_ALLOWLISTEDper #16703's own documented redesign (superseded byUNFILTERED_READ_CEILING, checked against the detector's findings rather than the allowlist's own length — the file's own comment explains why the old pattern let both rise together). Against #16706: #16706's branch pre-dates #16691's removal of severalknowledge_search.py/knowledge_search_aggregator.pyTRACKED_GAP entries, so its diff still carried the old (already-removed) lines; per "an entry a member removed stays removed unless deliberately re-added with a SCOPED reason," those stayed removed, while #16706's own reclassifications (knowledge_relations.py,knowledge_tags.py,knowledge_verification.py, etc. →_SCOPED_ADMIN_ROUTER) were kept. Verified post-merge: 97 entries, no duplicate keys, valid syntax.autobot-frontend/src/i18n/locales/*.json(all 11, #16696)en.json's key count (7911) exceeds both parents (7881, 7868) — no key dropped.repo_tests/frontend_fragmentation_ratchet_test.py(css_rule_declarations, #16696)_measure()against the merged tree: 9388, below both parents (9398 main, 9391 #16696) — used the real count.repo_tests/i18n_untranslated_ratchet_test.py(BASELINE, #16696)_untranslated()against the merged tree and setBASELINEto those exact values — all ≤ both parents.repo_tests/kb_read_visibility_guard_test.py(UNFILTERED_READ_CEILING, follow-up fix commit)repo_tests/kb_admin_read_bypass_guard_test.py(EXPLICIT_READ_APIS, follow-up fix commit)is_adminthroughknowledge_search.py/knowledge_search_aggregator.py; #16705's new guard (same batch) only allowlisted 3 other files, so the combination failed pre-push. Filed #16745 rather than guessing on security code. Owner decision (2026-09-14, #16745): these two files are themselves explicit, caller-driven read APIs (canonical search + multi-source aggregation), not a chat/agent/RAG hop, so #16691'sis_adminbypass there is the intended #16654 behaviour — same category as the three files already listed. Added both toEXPLICIT_READ_APISin a dedicated commit. Chat/RAG/agent paths remain unlisted and stay blocked.Refs #16745 (owner decision that unblocked the
kb_admin_read_bypass_guard_test.pyconflict above).Verification
headRefOidviagh pr view --json headRefOidbefore merging.git worktree-local targeted pytest (design aid only) on every touched ratchet/guard:frontend_fragmentation_ratchet_test.py,i18n_untranslated_ratchet_test.py,kb_read_visibility_guard_test.py,kb_admin_read_bypass_guard_test.py— all pass locally.git pushpre-push hook (pytest + hardcoded-value + size/canonical guards) passed clean.Model Used
Sonnet 5