Fix 5 bugs (2 access-control leaks) and add test coverage for reports, search, subscriptions#241
Merged
Conversation
A query token consisting only of apostrophes or other punctuation survived sanitization and reached the raw tsquery, raising ProgrammingError (HTTP 500) - a trivial denial of service on the search box. Drop tokens with no letter/digit/mark, quote terms (doubling embedded apostrophes), and prune empty operator nodes. Adds search-engine tests including hostile inputs.
NoteEditView.get_object filtered by report_id only, bypassing the owner-scoped queryset, so a user could read, overwrite, hijack ownership of, or delete another user's note for the same report. Scope the lookup to the request user. Adds cross-user isolation tests.
processors: the accept path passed filter_fields_results= but the SubscribedItem field is 'answers', so every matched report raised TypeError and no item was ever created - subscription matching was wholly non-functional. forms: clean() subscripted cleaned_data[...] and raised KeyError (HTTP 500) when an age failed validation; use .get(). Adds subscription processor, task, and form tests.
- reports REST API: create/update/upsert (201 vs 200), bulk upsert, document_id dedup, M2M and metadata, IsAdminUser authorization - patient_age generated field (leap years, age at study time) - chats async views and extractions LLM plumbing - collections XLSX export contents
pgsearch _build_filter_query ignored the group filter that SearchView already passes (group=active_group.pk), so search returned reports from groups the user is not in - a cross-group data leak. Scope results to reports whose groups M2M includes the filter group, matching the access model used by the reports list/detail views. Adds search end-to-end tests (the leak regression plus a same-group positive case) and threads the group through seeded reports in existing provider tests.
to_internal_value raised ValidationError with a bare-string detail inside a nested serializer, which became an uncaught ValueError (HTTP 500) instead of 400; use dict-shaped details. update() used Language.objects.get() where create() uses get_or_create(), so PUTting a new language onto an existing report raised DoesNotExist (500); align them. Adds serializer unit tests.
- reports bulk upsert: partial-failure rollback, document_id uniqueness, cascade behavior - radis-client request/response shape contract
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR fixes several bugs in notes, search, reports, subscriptions, and settings, removes nest-asyncio2 test patching, and adds broad new test coverage for client, chats, extractions, collections, reports, search, and subscriptions. ChangesNotes Ownership Fix
PGSearch Query Sanitization
Reports Serializer/API Fixes and Tests
Subscriptions Fixes and Tests
Settings and Infra
New Test Coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves test coverage across multiple modules (chats, collections, extractions, notes, pgsearch, reports, search, and subscriptions) and resolves several critical bugs, including cross-user isolation leaks in note editing, crashes in full-text search query parsing, and field name mismatches in subscription processing. The reviewer suggests copying the input data dictionary in to_internal_value of ReportSerializer to prevent side-effects and avoid crashes when handling immutable QueryDict objects.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
133
to
137
| def to_internal_value(self, data: Any) -> Any: | ||
| if "language" in data: | ||
| if not isinstance(data["language"], str): | ||
| raise ValidationError("Invalid language type.") | ||
| raise ValidationError({"language": "Invalid language type."}) | ||
| data["language"] = {"code": data["language"]} |
There was a problem hiding this comment.
Mutating the input data dictionary in-place inside to_internal_value is a side-effect that can lead to unexpected behavior for the caller if they reuse the payload dictionary. Furthermore, if data is an immutable QueryDict (e.g., from a standard Django form submission), attempting to mutate it in-place will raise an AttributeError or UnsupportedOperation and crash the request.
To avoid this, create a copy of the data dictionary before performing any mutations.
Suggested change
| def to_internal_value(self, data: Any) -> Any: | |
| if "language" in data: | |
| if not isinstance(data["language"], str): | |
| raise ValidationError("Invalid language type.") | |
| raise ValidationError({"language": "Invalid language type."}) | |
| data["language"] = {"code": data["language"]} | |
| def to_internal_value(self, data: Any) -> Any: | |
| data = data.copy() if hasattr(data, "copy") else dict(data) | |
| if "language" in data: | |
| if not isinstance(data["language"], str): | |
| raise ValidationError({"language": "Invalid language type."}) | |
| data["language"] = {"code": data["language"]} |
Member
There was a problem hiding this comment.
Fixed in 2db757c — to_internal_value now copies the payload before reshaping (shallow copy suffices; only top-level keys are reassigned), with a regression test asserting the input stays unmutated. The round-trip test that had explicitly pinned the old in-place mutation was updated to the new contract.
The shared testing helpers are now event-loop independent (thread-based run_worker_once, fork-safe DaphneProcess) and terminate leaked test-database sessions before teardown, so nest_asyncio and its global asyncio patching are no longer needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
channels' database_sync_to_async closes the thread's database connections around each call, so the tests no longer accumulate open connections in asgiref's shared executor thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Django deprecated (name, address) pairs in ADMINS; use a list of plain email address strings. The registration admin-approval flow keeps working through REGISTRATION_ADMINS, which django-registration-redux checks first and which still expects pairs (its fallback to ADMINS indexes admin[1], which would silently pick the second character of a plain string). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g it The wire-format reshaping rewrote the caller's payload dict in place, which leaks the transformation to the caller and crashes with an immutable QueryDict (e.g. a form-encoded request via the browsable API). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test suite has grown considerably and runs in a single pytest process with coverage, which no longer fits in 15 minutes on a GitHub runner. pytest-timeout still fails any individual hanging test after 60 seconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
radis/reports/tests/test_patient_age.py (1)
32-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parametrizing the near-duplicate age-boundary tests.
test_patient_age_basic_completed_years,test_patient_age_day_before_birthday,test_patient_age_on_birthday,test_patient_age_day_after_birthday, and the leap-year variants all follow the identical build→refresh→assert shape. Apytest.mark.parametrizetable of(birth_date, study_dt, expected_age)would reduce duplication while keeping each case's intent documented via the parameter id.♻️ Example consolidation
-@pytest.mark.django_db -def test_patient_age_basic_completed_years(): - report = _make_report( - date(1980, 1, 15), - datetime(2024, 3, 15, 10, 30, tzinfo=UTC), - ) - report.refresh_from_db() - assert report.patient_age == 44 - - -@pytest.mark.django_db -def test_patient_age_day_before_birthday(): - ... +@pytest.mark.django_db +@pytest.mark.parametrize( + ("birth_date", "study_dt", "expected_age"), + [ + pytest.param(date(1980, 1, 15), datetime(2024, 3, 15, 10, 30, tzinfo=UTC), 44, id="basic"), + pytest.param(date(1990, 6, 10), datetime(2024, 6, 9, 8, 0, tzinfo=UTC), 33, id="day-before-birthday"), + pytest.param(date(1990, 6, 10), datetime(2024, 6, 10, 0, 1, tzinfo=UTC), 34, id="on-birthday"), + pytest.param(date(1990, 6, 10), datetime(2024, 6, 11, 8, 0, tzinfo=UTC), 34, id="day-after-birthday"), + ], +) +def test_patient_age_boundaries(birth_date, study_dt, expected_age): + report = _make_report(birth_date, study_dt) + report.refresh_from_db() + assert report.patient_age == expected_age🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radis/reports/tests/test_patient_age.py` around lines 32 - 124, The age-boundary tests in test_patient_age.py are nearly identical and should be consolidated. Refactor the repeated build→refresh→assert cases in test_patient_age_basic_completed_years, test_patient_age_day_before_birthday, test_patient_age_on_birthday, test_patient_age_day_after_birthday, and the leap-year variants into a single pytest.mark.parametrize-driven test using _make_report and report.refresh_from_db, with parameter ids preserving each scenario’s intent. Keep the existing recomputation and same-day-zero tests separate if they exercise different behavior.radis/notes/tests/test_views.py (1)
278-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "KNOWN BUG" comments contradict the already-fixed lookup.
These docstrings/section header describe
NoteEditView.get_objectas keying notes byreport_idonly with no owner filter, butradis/notes/views.py:55already filters by bothreport_idandowner. The tests pass precisely because the bug is fixed — leaving these comments as-is will confuse future readers into thinking the vulnerability is still open.📝 Example fix for the section header (apply similarly to each per-test docstring)
# --------------------------------------------------------------------------- # Cross-user isolation on the edit/update path. # # A user must not be able to read, overwrite, or take ownership of another -# user's note on the same report. ``NoteEditView`` keys notes by ``report_id`` -# only, so all of the assertions below probe that ownership boundary. +# user's note on the same report. ``NoteEditView.get_object`` scopes notes by +# both ``report_id`` and ``owner``; the assertions below guard that boundary. # ---------------------------------------------------------------------------Also applies to: 292-295, 317-320, 347-350, 374-376
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radis/notes/tests/test_views.py` around lines 278 - 284, Update the stale section header and per-test docstrings in test_views.py so they match the current behavior of NoteEditView.get_object. The comments still describe note lookup as report_id-only and imply a missing owner filter, but the view in radis/notes/views.py already filters by both report_id and owner. Rewrite those comments to describe the cross-user isolation being verified, not a known bug, and keep the wording consistent across the affected test blocks.radis/collections/tests/test_exporters.py (1)
74-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit
strict=tozip().Ruff (B905) flags this as missing an explicit
strictparameter. SinceEXPECTED_HEADERandroware expected to have the same length,strict=Truewould surface a mismatch immediately instead of silently truncating.🧹 Proposed fix
- by_col = dict(zip(EXPECTED_HEADER, row)) + by_col = dict(zip(EXPECTED_HEADER, row, strict=True))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radis/collections/tests/test_exporters.py` at line 74, The test in test_exporters.py uses dict(zip(EXPECTED_HEADER, row)) without an explicit strict argument, which Ruff flags. Update the zip call in the by_col assignment to pass strict=True so the EXPECTED_HEADER and row lengths are validated immediately instead of truncating silently.Source: Linters/SAST tools
radis-client/tests/test_client_contract.py (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the
clientfixture to avoid shadowing pytest-django's built-in fixture.pytest-django ships a
clientfixture (Django testClient) by default. Overriding it here with aRadisClientinstance works, but the name collision could confuse readers/maintainers expecting the standard Django client, especially if this test module grows or other fixtures referenceclient.♻️ Suggested rename
`@pytest.fixture` -def client(live_server: LiveServer) -> RadisClient: +def radis_client(live_server: LiveServer) -> RadisClient: _user, _group, token = create_admin_with_group_and_token() return RadisClient(live_server.url, token)(and update all
client: RadisClientparameters throughout the file toradis_client: RadisClient)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radis-client/tests/test_client_contract.py` around lines 42 - 45, The `client` fixture in `test_client_contract.py` is shadowing pytest-django’s built-in Django `client` fixture, which can be confusing. Rename the fixture in the `client` function to `radis_client` and update every test parameter that currently references `client: RadisClient` to use `radis_client: RadisClient` so the local fixture name is unambiguous.radis/pgsearch/tests/test_providers.py (1)
271-279: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPotential flakiness: offset test relies on tie-broken rank ordering.
All three seeded reports share the identical body
"pneumonia case", so they will tie onts_rank.providers.search()only sorts.order_by("-rank")with no secondary key, so the relative order of tied rows isn't guaranteed to be stable across two independent query executions (fullvsskipped). In practice Postgres will likely return the same order for identical re-executed queries against unchanged data, but this is not guaranteed behavior to depend on in a test.Consider giving each report distinct, rank-differentiating body text (as other tests in this file already do) to make the ordering assertion deterministic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radis/pgsearch/tests/test_providers.py` around lines 271 - 279, The offset test in test_offset_skips_leading_results is flaky because all seeded reports have identical text and can tie on rank, making the ordering from providers.search() nondeterministic across separate calls. Update the test data in test_offset_skips_leading_results to use distinct body text that produces a stable rank order, similar to the other tests in this file, and keep the assertions against full and skipped results unchanged.radis/reports/tests/test_api.py (1)
34-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated payload-builder helpers.
make_payload()here is nearly identical towire_payload()inradis/reports/tests/test_serializers.py(lines 25-52) andmake_payload()inradis/reports/tests/test_data_integrity.py(lines 66-87) — same field set, only a few literal values differ. Consider extracting a single shared fixture/helper (e.g. in aradis/reports/tests/factories.pyorconftest.py) that all three files import, matching the "shared test-helper adoption" direction already mentioned for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radis/reports/tests/test_api.py` around lines 34 - 63, The payload builder is duplicated across multiple test modules, so extract the shared report payload helper into one reusable test fixture/helper and have test_api, test_serializers, and test_data_integrity import it. Keep the common field construction in the shared helper (the make_payload/wire_payload shape) and allow each test to override only the few differing literals via kwargs or a small wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radis/chats/tests/test_views.py`:
- Around line 240-273: `chat_update_view` currently fetches the chat with
`Chat.objects...aget(pk=pk, owner=request.user)`, which raises
`Chat.DoesNotExist` and surfaces as a 500 for non-owners. Update this view to
use `aget_object_or_404` (matching `chat_detail_view` and `chat_delete_view`) so
unauthorized access returns a consistent 404 instead of an unhandled exception.
Keep the ownership check tied to the existing `chat_update_view` flow and
preserve the “LLM not consulted” behavior for blocked requests.
In `@radis/extractions/tests/unit/test_llm_extraction.py`:
- Around line 97-99: The test in Schema should not use a broad Exception catch,
since that can hide unrelated failures; update the pytest.raises assertion
around the Schema(finding="x") call to target the specific pydantic validation
error raised by this model. Use the Schema symbol in test_llm_extraction to
locate the assertion and narrow the expected exception type so the test only
passes for the intended validation failure.
In `@radis/reports/tests/test_data_integrity.py`:
- Around line 324-344: The test is mutating module-level handler state by
assigning empty lists to viewsets.reports_created_handlers and
viewsets.reports_deleted_handlers, which can leak into later tests. Update
test_api_delete_removes_report_and_dependents to override those globals with
monkeypatch or restore them with a cleanup/finalizer, keeping the change scoped
to this test only. Use the viewsets module symbols directly so the original
handlers are preserved after the test finishes.
- Around line 202-274: BulkUpsertOnCommitTests is mutating the global handler
registries and leaving them as empty lists, which affects later tests. Update
the test setup/cleanup around viewsets.reports_created_handlers and
viewsets.reports_updated_handlers to restore the original registries after each
test instead of assigning [] permanently; use the same approach in
test_api_delete_removes_report_and_dependents for
viewsets.reports_created_handlers and viewsets.reports_deleted_handlers, or
switch to monkeypatch so the site-registered handlers are preserved for
subsequent tests.
---
Nitpick comments:
In `@radis-client/tests/test_client_contract.py`:
- Around line 42-45: The `client` fixture in `test_client_contract.py` is
shadowing pytest-django’s built-in Django `client` fixture, which can be
confusing. Rename the fixture in the `client` function to `radis_client` and
update every test parameter that currently references `client: RadisClient` to
use `radis_client: RadisClient` so the local fixture name is unambiguous.
In `@radis/collections/tests/test_exporters.py`:
- Line 74: The test in test_exporters.py uses dict(zip(EXPECTED_HEADER, row))
without an explicit strict argument, which Ruff flags. Update the zip call in
the by_col assignment to pass strict=True so the EXPECTED_HEADER and row lengths
are validated immediately instead of truncating silently.
In `@radis/notes/tests/test_views.py`:
- Around line 278-284: Update the stale section header and per-test docstrings
in test_views.py so they match the current behavior of NoteEditView.get_object.
The comments still describe note lookup as report_id-only and imply a missing
owner filter, but the view in radis/notes/views.py already filters by both
report_id and owner. Rewrite those comments to describe the cross-user isolation
being verified, not a known bug, and keep the wording consistent across the
affected test blocks.
In `@radis/pgsearch/tests/test_providers.py`:
- Around line 271-279: The offset test in test_offset_skips_leading_results is
flaky because all seeded reports have identical text and can tie on rank, making
the ordering from providers.search() nondeterministic across separate calls.
Update the test data in test_offset_skips_leading_results to use distinct body
text that produces a stable rank order, similar to the other tests in this file,
and keep the assertions against full and skipped results unchanged.
In `@radis/reports/tests/test_api.py`:
- Around line 34-63: The payload builder is duplicated across multiple test
modules, so extract the shared report payload helper into one reusable test
fixture/helper and have test_api, test_serializers, and test_data_integrity
import it. Keep the common field construction in the shared helper (the
make_payload/wire_payload shape) and allow each test to override only the few
differing literals via kwargs or a small wrapper.
In `@radis/reports/tests/test_patient_age.py`:
- Around line 32-124: The age-boundary tests in test_patient_age.py are nearly
identical and should be consolidated. Refactor the repeated build→refresh→assert
cases in test_patient_age_basic_completed_years,
test_patient_age_day_before_birthday, test_patient_age_on_birthday,
test_patient_age_day_after_birthday, and the leap-year variants into a single
pytest.mark.parametrize-driven test using _make_report and
report.refresh_from_db, with parameter ids preserving each scenario’s intent.
Keep the existing recomputation and same-day-zero tests separate if they
exercise different behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1b6b6c8-3ce5-43c9-9452-f9a2e69c78f6
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
pyproject.tomlradis-client/tests/test_client_contract.pyradis/chats/tests/test_views.pyradis/collections/tests/test_exporters.pyradis/conftest.pyradis/extractions/tests/unit/test_llm_extraction.pyradis/notes/tests/test_views.pyradis/notes/views.pyradis/pgsearch/providers.pyradis/pgsearch/tests/test_providers.pyradis/reports/api/serializers.pyradis/reports/tests/test_api.pyradis/reports/tests/test_data_integrity.pyradis/reports/tests/test_patient_age.pyradis/reports/tests/test_serializers.pyradis/search/tests/test_views_e2e.pyradis/search/utils/query_parser.pyradis/settings/base.pyradis/subscriptions/forms.pyradis/subscriptions/processors.pyradis/subscriptions/tests/test_forms.pyradis/subscriptions/tests/test_processors.pyradis/subscriptions/tests/test_tasks_build.py
💤 Files with no reviewable changes (1)
- radis/conftest.py
chat_update_view fetched the chat with a bare aget(pk, owner=user), so a non-owner (or a stale tab posting to a deleted chat) got an unhandled Chat.DoesNotExist instead of the 404 that chat_detail_view and chat_delete_view already return via get_object_or_404. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Assert pydantic's ValidationError instead of a blind Exception for the required-field check of the dynamic extraction schema. - Restore the reports handler registries to their original values after the on-commit tests instead of resetting them to empty lists (the registries are currently empty extension points, so this is hygiene, not a bug fix). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mhumzaarain
added a commit
that referenced
this pull request
Jul 6, 2026
Tests from main (#241) mocked parse() without extra_body and asserted a system-role message, matching the old ChatClient. The new core LLMClient always passes extra_body and sends the prompt as a user message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
samuelvkwong
added a commit
that referenced
this pull request
Jul 7, 2026
* docs: design for shared LLM client with built-in rate limiting Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: extract_data uses user message and extra_body Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: implementation plan for shared LLM client with rate limiting Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): add LLM rate-limit settings * test: add LLM rate-limit error builders * feat(core): add rate-limit gate with sync and async helpers * fix(core): use clamped pause for gate budget decision run_through_gate(_async) armed the gate with the clamped pause but made the defer-vs-wait decision against the raw, unclamped Retry-After. When a server sent Retry-After above header_ceiling, this caused a spurious RateLimited even though the clamped window fit the caller's budget. Drop the divergent early-raise and let the loop's wait_until_open(_async) decide from the armed (clamped) _blocked_until, as it already did correctly. * feat(core): add throttled LLMClient and AsyncChatClient * refactor: use core LLMClient with built-in rate limiting across apps * fix(core): give RateLimited a default message; clarify test * docs: design for proactive LLM_MAX_RPM request cap Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: implementation plan for LLM_MAX_RPM request cap Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(core): add LLM_MAX_RPM setting * feat(core): add RpmLimiter and wire it into run_through_gate * feat(core): cap LLM requests per minute via shared RpmLimiter * Clean up: Added few comments and cleaned up specs files * Fix: few minor issue that were suggested by Gemini and Coderabbitai * fix(core): make RpmLimiter a sliding-window to match provider quota * refactor(core): remove proactive RPM limiter Different LLM providers use different RPM quotas, so a single proactive per-process cap is awkward to configure meaningfully. Reactive rate limiting (the shared 429 backoff gate plus transient retries) already handles provider limits regardless of their configuration, so drop the proactive RpmLimiter entirely. Removes the RpmLimiter class, the rpm parameter on run_through_gate/ run_through_gate_async (and the redundant post-acquire gate re-check), the _LLM_RPM_LIMITER global, the LLM_MAX_RPM setting/env var, and all related tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(core): route absurd Retry-After to exponential backoff Rename LLM_RATE_LIMIT_FALLBACK_MAX_SECONDS to LLM_RATE_LIMIT_BACKOFF_MAX_SECONDS (and the gate's fallback_max param/attr to backoff_max) to better describe what it caps. Change the header ceiling from a clamp into a sanity threshold: a Retry-After >= LLM_RATE_LIMIT_HEADER_CEILING_SECONDS is now treated as absurd and ignored, falling back to the shared exponential backoff ladder instead of blocking the gate for the full ceiling. * test: adapt merged-in LLM mocks to new client signature Tests from main (#241) mocked parse() without extra_body and asserted a system-role message, matching the old ChatClient. The new core LLMClient always passes extra_body and sends the prompt as a user message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Samuel Kwong <samvictor232@gmail.com>
samuelvkwong
added a commit
that referenced
this pull request
Jul 9, 2026
Main dropped this in #241 when adopting adit-radis-shared 0.25.0, whose test helpers are event-loop independent; the merge back into this branch kept it. The package is no longer a declared dependency and only imports via ipykernel's transitive lock entry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At a glance
🔒 Security fixes (please read first)
Two live data-exposure issues, both now closed and proven shut by regression tests:
Both now follow the exact access model already used across the rest of the app.
Other bugs fixed
What's covered now
Reports REST API (create / update / bulk upsert, dedup, permissions) · the search engine (term / phrase / AND / OR / NOT, ranking, group-scoping, hostile-input safety) · LLM chat & extraction plumbing (via a controllable fake client) · subscriptions · notes · export · the
radis-clientcontract. Coverage → ~92%.Risk & rollback
Additive test coverage plus five tightly-scoped fixes. No schema or API changes. Fully reversible by reverting the branch.
Engineering detail on the five fixes
NoteEditView.get_objectfiltered byreport_idonly, bypassing the owner-scoped queryset. Now scoped to the request user.pgsearch._build_filter_queryignored thegroupfilter thatSearchViewalready passes. Now scoped viaQ(report__groups=filters.group), matching the reports views' access model.filter_fields_resultsvs the model'sanswers), so every match raisedTypeErrorand noSubscribedItemwas created.KeyErrorinclean()on a non-multiple-of-10 age; now surfaces the field error.ValidationErrorin a nested serializer became an uncaughtValueError(500); and PUTting a new language usedget()instead ofget_or_create()(500). Both now return correct responses.Reviewer checklist
🤖 Generated with Claude Code
Summary by CodeRabbit