feat: Phase 18 - Universal RBAC and Frontend UX fixes - #229
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OIDC/JWKS RS256 verification and role helpers; canonicalizes RBAC; adds DB-backed CalDAV/WebDAV models and writeback-intent API; implements runner websocket dispatch; centralizes telemetry; updates frontend nav/UX and tests; and tightens CI/Strix model gating to GPT-5.4+. ChangesMulti-Feature and Infrastructure Update
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
PR governance metadata gate is not ready for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/api/auth.py (1)
21-27:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRuntime role allowlist still uses pre-migration names.
Line 21-27 and Line 223-224 still enforce
platform_admin/organization_admin, so validsystem_admin/tenant_admintokens are rejected (matches the CI failures at Line 220).Suggested fix
-RoleName = Literal["platform_admin", "organization_admin", "group_admin", "member"] +RoleName = Literal["system_admin", "tenant_admin", "group_admin", "member"] ALLOWED_ROLES: set[str] = { - "platform_admin", - "organization_admin", + "system_admin", + "tenant_admin", "group_admin", "member", } @@ - if role != "platform_admin" and organization_id is None: + if role != "system_admin" and organization_id is None: raise _authentication_error()Also applies to: 223-224
🤖 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 `@backend/api/auth.py` around lines 21 - 27, Update the role allowlist to use the post-migration role names so tokens with system_admin and tenant_admin are accepted: change the RoleName Literal and the ALLOWED_ROLES set (symbols: RoleName and ALLOWED_ROLES) to include "system_admin" and "tenant_admin" (and optionally keep the old names if backward compatibility is required), and ensure any runtime role checks that reference the same allowlist/values (the checks around the current enforcement of platform_admin/organization_admin) use these updated names so validations no longer reject migrated tokens.backend/tests/test_access_policy.py (1)
100-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix access-policy role migration for
system_admin/tenant_admin
backend/services/access_policy.pydefinesPolicyRoleNameas"platform_admin" | "organization_admin" | "group_admin" | "member"(no"system_admin"/"tenant_admin"), so RBAC-permitted checks for these roles won’t match.evaluate_access()only bypasses the organization mismatch whenrequest.role == "platform_admin"; with"system_admin"it will fall through and returnorganization_denied, which conflicts withbackend/tests/test_access_policy.py.🤖 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 `@backend/tests/test_access_policy.py` around lines 100 - 145, The tests fail because role names were migrated but code still checks legacy names: update evaluate_access in backend/services/access_policy.py (and any RBAC checks that reference PolicyRoleName) to treat legacy aliases by mapping "system_admin" -> "platform_admin" and "tenant_admin" -> "organization_admin" (or accept both names) when evaluating permitted_roles and when doing the organization-bypass check; ensure AccessRequest.role is normalized before comparisons against ResourcePolicy.permitted_roles and the organization-bypass clause so RBAC and org-bypass behave consistently with the tests.backend/tests/test_webdav_api.py (1)
9-24:⚠️ Potential issue | 🟠 MajorAdd real PostgreSQL bootstrap/smoke coverage for
/api/webdav/writeback-intent
backend/tests/test_webdav_api.pyonly exercises a mocked fast path: the autousestub_webdav_servicemonkeypatcheswebdav_service.get_connected_accountsandwebdav_service.get_project_folders, and the/api/webdav/writeback-intenttests (lines ~49-67) rely on those stubs.- Search shows
/api/webdav/writeback-intentis only tested in this file (no additional live/real-PostgreSQL smoke bootstrap path for this endpoint).- Add a PostgreSQL-backed bootstrap/smoke test path for this API slice (not only monkeypatched behavior) per the DB-slice guidelines.
🤖 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 `@backend/tests/test_webdav_api.py` around lines 9 - 24, The current autouse fixture stub_webdav_service monkeypatches webdav_service.get_connected_accounts and get_project_folders so tests for the /api/webdav/writeback-intent endpoint only exercise a mocked fast path; add a new PostgreSQL-backed smoke/bootstrap test that seeds the real DB with the required connected account and project folder rows (instead of relying on stub_webdav_service) and calls the endpoint to verify behavior end-to-end. Concretely, add a test function in backend/tests/test_webdav_api.py that temporarily disables or bypasses the autouse fixture (or uses a marker) and uses the project DB fixture (e.g., the test DB session/engine already used in other DB-slice tests) to insert the account (account_id, server_url, username, user_id "alice") and folders (folder_id, project_name, webdav_path), then invoke the API client against /api/webdav/writeback-intent and assert the expected response; follow existing DB-slice patterns for setup/teardown and use the same table/ORM models that the webdav service reads so the endpoint is exercised against real PostgreSQL data.
🧹 Nitpick comments (2)
backend/core/telemetry.py (1)
14-14: 💤 Low valueComment doesn't match implementation logic.
The comment states "Only setup if OTEL is enabled or endpoint is provided", but the logic only checks
ENABLE_OTEL. SinceOTEL_EXPORTER_OTLP_ENDPOINThas a default value of"http://localhost:4317", the endpoint is always "provided" and the comment's condition is misleading.Update the comment to accurately reflect the actual gating logic.
📝 Proposed fix to clarify comment
- # Only setup if OTEL is enabled or endpoint is provided + # Only setup if OTEL is enabled via ENABLE_OTEL environment variable otel_endpoint = os.getenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://localhost:4317")🤖 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 `@backend/core/telemetry.py` at line 14, The comment above the OTEL setup is misleading: it says "Only setup if OTEL is enabled or endpoint is provided" but the code only checks ENABLE_OTEL (OTEL_EXPORTER_OTLP_ENDPOINT has a default and isn't used for gating). Update that comment to accurately state that the setup is conditioned solely on ENABLE_OTEL (i.e., "Only set up tracing if ENABLE_OTEL is true") and, if desired, note that OTEL_EXPORTER_OTLP_ENDPOINT is configured separately rather than used to gate setup.backend/tests/test_webdav_api.py (1)
49-67: ⚡ Quick winAdd explicit target-account selection regression tests.
Current coverage validates default selection and no-account error, but not the explicit
target_account_idcontract (valid and invalid IDs).Suggested test additions
@@ def test_get_webdav_writeback_intent(auth_client): @@ assert body["provenance"] == "server-authoritative" + +def test_get_webdav_writeback_intent_with_target_account(auth_client, monkeypatch): + monkeypatch.setattr( + webdav_service, + "get_connected_accounts", + lambda user_id: [ + {"account_id": 1, "server_url": "https://webdav.naruon.net", "username": "demo_user"}, + {"account_id": 2, "server_url": "https://alt-webdav.naruon.net", "username": "demo_user"}, + ] if user_id == "alice" else [], + ) + response = auth_client.post("/api/webdav/writeback-intent", json={"target_account_id": 2}) + assert response.status_code == 200, response.text + assert response.json()["source_id"] == 2 + +def test_get_webdav_writeback_intent_with_unknown_target_account(auth_client): + response = auth_client.post("/api/webdav/writeback-intent", json={"target_account_id": 999}) + assert response.status_code == 422🤖 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 `@backend/tests/test_webdav_api.py` around lines 49 - 67, Add two tests exercising explicit target_account_id selection for the /api/webdav/writeback-intent endpoint: one that posts {"target_account_id": <valid_id>} using auth_client and asserts 200 plus that response.json()["source_id"] equals the provided id and other fields (intent, requires_if_match, server_url, provenance) are present; and one that posts {"target_account_id": <invalid_id>} (e.g., non-existent id) using a TestClient with no accounts and/or an auth context and asserts a 4xx response (422) and that the JSON error detail indicates the selected account was not found/invalid. Place them alongside test_get_webdav_writeback_intent and test_get_webdav_writeback_intent_no_accounts, naming them e.g., test_get_webdav_writeback_intent_with_target_account and test_get_webdav_writeback_intent_with_invalid_target_account.
🤖 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 `@backend/api/auth.py`:
- Around line 199-206: The code currently skips legacy metadata checks based
only on settings.OIDC_ISSUER_URL, which incorrectly bypasses ver/iss/aud
validation when a token falls back to legacy HS256 verification; update the
logic so the ver/iss/aud checks (payload.get("ver"), payload.get("iss") vs
SESSION_ISSUER, and payload.get("aud") vs SESSION_AUDIENCE) are enforced unless
the token was actually validated by OIDC (e.g., track a verification result/flag
from the OIDC jwt.decode path or detect OIDC-validated tokens), i.e., only skip
these checks when an explicit oidc-validated flag is true (not merely when
settings.OIDC_ISSUER_URL is set).
In `@backend/api/ontology.py`:
- Around line 59-63: The update branch currently unconditionally sets
rel.parent_sender_email = req.parent_sender_email which can overwrite existing
data with None from older clients; change the update logic in the handler that
updates rel (the code that sets rel.relationship_type, rel.confidence_score,
rel.parent_sender_email) to only assign parent_sender_email when the incoming
request actually provides it (e.g., check if hasattr(req, "parent_sender_email")
or if req.parent_sender_email is not None) so existing parent_sender_email is
preserved when absent from the request.
In `@backend/core/config.py`:
- Around line 43-46: The new OIDC settings (OIDC_ISSUER_URL, OIDC_CLIENT_ID,
OIDC_JWKS_URL) can be partially set causing ambiguous auth behavior—add a
coherence check in the configuration initialization (e.g., in the Config class
__post_init__ / validate or module-level startup check) that verifies either all
three are set or none are set; if a partial set is detected raise a clear
startup error (ValueError/SystemExit) with a message referencing
OIDC_ISSUER_URL, OIDC_CLIENT_ID and OIDC_JWKS_URL so the app fails fast and
informs the operator which variables are inconsistent.
In `@backend/core/telemetry.py`:
- Around line 15-16: The telemetry module currently enables OTEL by default by
setting otel_endpoint = os.getenv("OTEL_EXPORTER_OTLP_ENDPOINT",
"http://localhost:4317") and enable_otel = os.getenv("ENABLE_OTEL",
"true").lower() == "true", which differs from main.py behavior and causes tests
to attempt connections; change the gating to require explicit configuration by
default: set ENABLE_OTEL default to "false" and treat an unset
OTEL_EXPORTER_OTLP_ENDPOINT as disabled (i.e., only enable telemetry when
enable_otel is true AND OTEL_EXPORTER_OTLP_ENDPOINT is provided/non-empty),
updating the logic around otel_endpoint and enable_otel in telemetry.py (and
keep behavior consistent with main.py).
- Line 33: The OTLPSpanExporter is hardcoded with insecure=True; change this to
read an environment variable (e.g., OTEL_EXPORTER_OTLP_INSECURE) and default to
False so TLS is enabled by default. In telemetry.py, parse the env var into a
boolean (accepting common truthy values like "1", "true", "yes") and pass that
boolean as the insecure argument when constructing OTLPSpanExporter (the line
that creates otlp_exporter, which currently uses otel_endpoint). Ensure the
variable name for the parsed flag is clear (e.g., otlp_insecure) and used in the
OTLPSpanExporter(...) call so production defaults to secure connections.
- Around line 22-39: Wrap the entire OpenTelemetry setup (creation of Resource
and TracerProvider, trace.set_tracer_provider(provider), OTLPSpanExporter
instantiation, BatchSpanProcessor creation and
provider.add_span_processor(span_processor), and
FastAPIInstrumentor.instrument_app(app)) in a try/except block so failures don't
crash startup; catch broad exceptions, log the error via logger.error including
the exception details, and skip adding the exporter/processor or instrumentation
on failure (i.e., do not re-raise) so the app can continue running without
telemetry.
- Around line 13-39: The setup_telemetry(app) function is never invoked while
backend/main.py still performs inline OTEL initialization (including
FastAPIInstrumentor.instrument_app(app)), so the centralized initializer is dead
code and risks double-instrumentation; fix by removing the inline OTEL setup
from backend/main.py and instead call setup_telemetry(app) during the FastAPI
startup sequence (e.g., in the startup event handler or right after app
creation), or vice‑versa consolidate into setup_telemetry and ensure only that
function performs TracerProvider/trace.set_tracer_provider, OTLPSpanExporter and
BatchSpanProcessor setup and calls FastAPIInstrumentor.instrument_app to
guarantee a single initialization. Ensure the
ENABLE_OTEL/OTEL_EXPORTER_OTLP_ENDPOINT guards remain in setup_telemetry and, if
needed, add an idempotent guard (module-level flag) inside setup_telemetry to
prevent multiple initializations.
In `@backend/runner/connector.py`:
- Around line 82-85: The _handle_send_smtp handler logs and uses
payload.get('account') without validating required fields; update
_handle_send_smtp to validate that payload contains a non-empty 'account' (and
any other required fields like 'to', 'subject', 'body' if applicable) before
proceeding, and if validation fails call self.send_response with an error status
and descriptive message (e.g., missing/invalid account) instead of continuing;
ensure validation mirrors the pattern used in _handle_fetch_imap (raise or
return early and send error response) so the gateway receives a clear failure
instead of a None account in logs.
- Around line 77-80: In _handle_fetch_imap validate that payload contains a
non-empty 'account' before using payload.get('account'): if missing/empty, log a
clear error (avoid logging "account: None"), send a failure response via
self.send_response (e.g.,
{"status":"error","action":"fetch_imap","error":"missing account"}) and return
early; otherwise proceed with the existing IMAP logic and success response.
Ensure checks use explicit condition (e.g., if not payload.get('account')) and
keep references to _handle_fetch_imap, payload.get('account'), and
self.send_response to locate the code.
- Around line 64-76: Move the json import to the top of the module and remove
the duplicate import in send_response; in the message handler (the method
containing json.loads(message) that currently calls _handle_fetch_imap and
_handle_send_smtp) validate that the parsed payload is a dict before calling
payload.get(), and on error cases (JSONDecodeError, non-dict payload, or unknown
action) call the existing send_response(...) helper to return an error payload
to the gateway (include an error message and a status field), while keeping the
normal calls to await self._handle_fetch_imap(payload) and await
self._handle_send_smtp(payload) for valid actions.
- Around line 61-85: Add unit tests for SelfHostedConnector.handle_message
covering successful dispatch and error branches: write tests that (1) send a
JSON message with action "fetch_imap" and assert _handle_fetch_imap is awaited
(mock or spy the method) and that send_response is called with the expected
dict; (2) send a JSON message with action "send_smtp" and assert
_handle_send_smtp is awaited and send_response invoked with expected message_id;
(3) send invalid JSON and assert json.JSONDecodeError path is exercised (mock
logger.error or capture logs); and (4) send a JSON with an unknown action and
assert logger.warning is called and no handler is invoked. Use asyncio test
utilities (pytest-asyncio or similar) to await handle_message, patch
SelfHostedConnector._handle_fetch_imap, _handle_send_smtp, and send_response to
observe calls and validate response formatting.
In `@backend/services/caldav_service.py`:
- Line 21: The debug log currently prints raw CalDAV server_url via
logger.debug(f"Syncing CalDAV account {account.id} at {account.server_url}")
which can leak embedded credentials; update the logging to avoid raw URLs by
stripping any userinfo before logging: parse account.server_url (e.g., using
urllib.parse.urlparse) and reconstruct a sanitized URL or log only the
host/netloc without username/password, then call logger.debug with account.id
and the sanitized value instead of account.server_url.
In `@backend/services/webdav_service.py`:
- Around line 77-81: The current account selection loop (using variables
target_account_id, accounts, selected_account) silently falls back to the first
account when a provided target_account_id isn't found; change it so that after
iterating accounts you explicitly detect "not found" and reject the request
(e.g., raise a ValueError/HTTPException or return an error) instead of
proceeding with the default, ensuring any caller is informed the
target_account_id was invalid.
In `@backend/tests/test_ontology_api.py`:
- Around line 9-13: Tests for the ontology API are missing assertions that
verify the new parent_sender_email field on the mock Relationship DTO; update
the relevant tests (e.g., the POST and GET relationship tests in
backend/tests/test_ontology_api.py) to assert that response JSON includes
"parent_sender_email" and that its value equals the mock object's
parent_sender_email (the constructor shown with sender_email, relationship_type,
confidence_score, parent_sender_email). Add the same assertion after creating a
relationship (POST) and when fetching it (GET) to ensure the new contract is
validated.
In `@backend/tests/test_runner_config_api.py`:
- Around line 70-71: The RBAC checks still rely on old role names so requests
using headers "X-User-Role": "tenant_admin" / "system_admin" are being denied;
update the runtime authorization logic for the runner-config endpoints (the GET
handler and the rotate handler) to recognize the new role taxonomy by accepting
"tenant_admin" where tenant-level admin is expected and "system_admin" for
system-level admin, adjust any deny-vs-allow ordering that short-circuits
authorization checks, and update the denial message path to reflect the new role
names; also ensure the unit tests at the other referenced locations (lines
~90-91, 100-111, 186-188, 202-203) use the same header values so tests and
implementation match.
In `@backend/tests/test_tasks_api.py`:
- Around line 52-53: The signed-session auth path is rejecting the renamed JWT
role claim "tenant_admin"; update the signed-session parsing/normalization so
the auth layer accepts the new role name. Modify the function that
parses/validates signed sessions (e.g., parse_signed_session or
validate_signed_session) to map or accept "tenant_admin" as an admin-equivalent
role, and update the role-normalization utility (e.g., normalize_role or
Authenticator.normalize_role) to treat legacy names (like "admin" or
"org_admin") and "tenant_admin" as equivalent admin roles so SignedSession-based
auth returns 200 for tenant_admin claims.
In `@backend/tests/test_tenant_config_api.py`:
- Around line 211-212: The endpoint /api/config/global still checks for the
deprecated "tenant_admin" role; locate the route handler or auth middleware used
by that endpoint (e.g., the function handling get_global_config or the
middleware named require_admin_role / check_global_config_admin) and update its
role check to the new taxonomy (accept "organization_admin" as the admin role
for this endpoint). Replace the old literal "tenant_admin" check with the new
role string (or map the migrated role to the new name) so the test using
X-User-Role: tenant_admin passes under the migrated taxonomy.
In `@frontend/src/components/WorkspaceHome.tsx`:
- Around line 153-174: The settings popup button and menu lack ARIA wiring;
update the button that toggles isSettingsOpen (the onClick that calls
setIsSettingsOpen) to include aria-expanded={isSettingsOpen}, aria-controls
pointing to a unique id (e.g., "workspace-settings-menu"), and
aria-haspopup="menu"; add that same id attribute to the popup container div (the
element rendered when isSettingsOpen is true) and ensure the div has role="menu"
(and each option/button can get role="menuitem") so assistive tech can detect
open/closed state and associate the control with the menu; keep using the
existing setWorkspaceStartupView handlers.
In `@frontend/tests/e2e/mobile-hamburger.spec.ts`:
- Around line 23-25: The test writes a fixed screenshot path which can clash
across parallel workers; update the screenshot call to use
testInfo.outputPath(...) so the file is unique per worker/run (replace the
current page.screenshot path with a path generated via
testInfo.outputPath("mobile-hamburger-open.png")); if the test callback doesn't
accept testInfo, add it to the test function signature (e.g., ({ page, testInfo
})) so you can call testInfo.outputPath before awaiting page.screenshot,
preserving fullPage: false.
In `@frontend/tests/e2e/scrolling.spec.ts`:
- Around line 24-28: The current check uses scrollContainer.evaluate to assert
overflowY style only (isScrollable) which can be true even if content doesn't
actually scroll; update the test to instead verify real scrollability by using
the scrollContainer handle (e.g., scrollContainer.evaluate or
scrollContainer.evaluateHandle) to assert that node.scrollHeight >
node.clientHeight and then programmatically perform a scroll (e.g., change
node.scrollTop or call node.scrollTo) and assert that scrollTop changes
accordingly; keep references to the existing scrollContainer and isScrollable
usage so you replace the style-only assertion with these
scrollHeight/clientHeight and scrollTop-change assertions.
---
Outside diff comments:
In `@backend/api/auth.py`:
- Around line 21-27: Update the role allowlist to use the post-migration role
names so tokens with system_admin and tenant_admin are accepted: change the
RoleName Literal and the ALLOWED_ROLES set (symbols: RoleName and ALLOWED_ROLES)
to include "system_admin" and "tenant_admin" (and optionally keep the old names
if backward compatibility is required), and ensure any runtime role checks that
reference the same allowlist/values (the checks around the current enforcement
of platform_admin/organization_admin) use these updated names so validations no
longer reject migrated tokens.
In `@backend/tests/test_access_policy.py`:
- Around line 100-145: The tests fail because role names were migrated but code
still checks legacy names: update evaluate_access in
backend/services/access_policy.py (and any RBAC checks that reference
PolicyRoleName) to treat legacy aliases by mapping "system_admin" ->
"platform_admin" and "tenant_admin" -> "organization_admin" (or accept both
names) when evaluating permitted_roles and when doing the organization-bypass
check; ensure AccessRequest.role is normalized before comparisons against
ResourcePolicy.permitted_roles and the organization-bypass clause so RBAC and
org-bypass behave consistently with the tests.
In `@backend/tests/test_webdav_api.py`:
- Around line 9-24: The current autouse fixture stub_webdav_service
monkeypatches webdav_service.get_connected_accounts and get_project_folders so
tests for the /api/webdav/writeback-intent endpoint only exercise a mocked fast
path; add a new PostgreSQL-backed smoke/bootstrap test that seeds the real DB
with the required connected account and project folder rows (instead of relying
on stub_webdav_service) and calls the endpoint to verify behavior end-to-end.
Concretely, add a test function in backend/tests/test_webdav_api.py that
temporarily disables or bypasses the autouse fixture (or uses a marker) and uses
the project DB fixture (e.g., the test DB session/engine already used in other
DB-slice tests) to insert the account (account_id, server_url, username, user_id
"alice") and folders (folder_id, project_name, webdav_path), then invoke the API
client against /api/webdav/writeback-intent and assert the expected response;
follow existing DB-slice patterns for setup/teardown and use the same table/ORM
models that the webdav service reads so the endpoint is exercised against real
PostgreSQL data.
---
Nitpick comments:
In `@backend/core/telemetry.py`:
- Line 14: The comment above the OTEL setup is misleading: it says "Only setup
if OTEL is enabled or endpoint is provided" but the code only checks ENABLE_OTEL
(OTEL_EXPORTER_OTLP_ENDPOINT has a default and isn't used for gating). Update
that comment to accurately state that the setup is conditioned solely on
ENABLE_OTEL (i.e., "Only set up tracing if ENABLE_OTEL is true") and, if
desired, note that OTEL_EXPORTER_OTLP_ENDPOINT is configured separately rather
than used to gate setup.
In `@backend/tests/test_webdav_api.py`:
- Around line 49-67: Add two tests exercising explicit target_account_id
selection for the /api/webdav/writeback-intent endpoint: one that posts
{"target_account_id": <valid_id>} using auth_client and asserts 200 plus that
response.json()["source_id"] equals the provided id and other fields (intent,
requires_if_match, server_url, provenance) are present; and one that posts
{"target_account_id": <invalid_id>} (e.g., non-existent id) using a TestClient
with no accounts and/or an auth context and asserts a 4xx response (422) and
that the JSON error detail indicates the selected account was not found/invalid.
Place them alongside test_get_webdav_writeback_intent and
test_get_webdav_writeback_intent_no_accounts, naming them e.g.,
test_get_webdav_writeback_intent_with_target_account and
test_get_webdav_writeback_intent_with_invalid_target_account.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ffb64ea-02aa-4fab-8770-bb95472a0813
⛔ Files ignored due to path filters (3)
backend/tests/live/__pycache__/conftest.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/live/__pycache__/mail_smoke_test.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pycbackend/tests/live/__pycache__/test_live_api_sequence.cpython-310-pytest-9.0.3.pycis excluded by!**/*.pyc
📒 Files selected for processing (33)
.coderabbit.yaml.github/workflows/app-ci.yml.github/workflows/strix.ymlAGENTS.mdbackend/api/auth.pybackend/api/ontology.pybackend/api/webdav.pybackend/core/config.pybackend/core/telemetry.pybackend/db/models.pybackend/requirements.txtbackend/runner/connector.pybackend/services/caldav_service.pybackend/services/webdav_service.pybackend/tests/conftest.pybackend/tests/test_access_policy.pybackend/tests/test_auth_real.pybackend/tests/test_calendar_api.pybackend/tests/test_llm_providers_api.pybackend/tests/test_ontology_api.pybackend/tests/test_ontology_pipeline.pybackend/tests/test_prompts_api.pybackend/tests/test_runner_config_api.pybackend/tests/test_tasks_api.pybackend/tests/test_tenant_config_api.pybackend/tests/test_threading_pipeline.pybackend/tests/test_webdav_api.pyfrontend/playwright.config.tsfrontend/src/components/DashboardLayout.tsxfrontend/src/components/SettingsLayout.tsxfrontend/src/components/WorkspaceHome.tsxfrontend/tests/e2e/mobile-hamburger.spec.tsfrontend/tests/e2e/scrolling.spec.ts
💤 Files with no reviewable changes (2)
- frontend/src/components/DashboardLayout.tsx
- .coderabbit.yaml
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/DashboardLayout.tsx (1)
82-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActive-state matching drops query params, so all
/mail?folder=*shortcuts can appear active at once.On
Line 84,splitHref()removes query strings, andLine 440uses that logic for mobile mail shortcuts. SinceusePathname()isaria-current="page".💡 Suggested fix
-import { usePathname } from 'next/navigation'; +import { usePathname, useSearchParams } from 'next/navigation'; function splitHref(href: string) { - const [path, hash = ''] = href.split('#'); - return { path: path.split('?')[0] || '/', hash: hash ? `#${hash}` : '' }; + const [pathWithQuery, hash = ''] = href.split('#'); + const [path = '/', query = ''] = pathWithQuery.split('?'); + return { path: path || '/', query, hash: hash ? `#${hash}` : '' }; } -function isActivePath(pathname: string | null, href: string, currentHash = '') { +function isActivePath( + pathname: string | null, + href: string, + currentHash = '', + currentSearchParams?: URLSearchParams +) { if (!pathname) return false; - const { path, hash } = splitHref(href); + const { path, query, hash } = splitHref(href); if (hash) { return pathname === path && currentHash === hash; } - return path === '/' + const pathMatch = path === '/' ? pathname === '/' - : pathname === path || pathname.startsWith(`${path}/`); + : pathname === path || pathname.startsWith(`${path}/`); + if (!pathMatch) return false; + if (!query || !currentSearchParams) return true; + const target = new URLSearchParams(query); + for (const [k, v] of target.entries()) { + if (currentSearchParams.get(k) !== v) return false; + } + return true; } export function DashboardLayout({ children, }: { children: React.ReactNode; }) { const pathname = usePathname(); + const searchParams = useSearchParams(); ... - const active = isActivePath(pathname, href); + const active = isActivePath(pathname, href, '', new URLSearchParams(searchParams.toString()));Also applies to: 437-446
🤖 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 `@frontend/src/components/DashboardLayout.tsx` around lines 82 - 96, splitHref currently strips the query string, so isActivePath (used by the mail shortcuts) only compares pathname and hash and treats all /mail?folder=* links as the same active path; update the matching to include the URL search/query when determining active state. Modify splitHref to preserve and return the search portion and adjust isActivePath to compare pathname + search (or include search in the equality/startsWith checks) in addition to hash/currentHash so links like /mail?folder=inbox vs /mail?folder=archive are distinguished (refer to splitHref and isActivePath functions and the usePathname consumer logic).backend/api/auth.py (1)
154-169:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove JWKS retrieval off the auth hot path
backend/api/auth.py(lines 154-169) callsjwks_client.get_signing_key_from_jwt(token)during request authentication. In PyJWT,PyJWKClientperforms a synchronous HTTP fetch to the JWKS URI on cache miss, so IdP latency/outages can block worker threads. Preload/refresh JWKS outside the request path (startup/periodic background refresh) and/or configure explicit HTTP timeout plus JWKS/key caching behavior, with graceful failure handling.🤖 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 `@backend/api/auth.py` around lines 154 - 169, The current request-path code calls jwks_client.get_signing_key_from_jwt(token) which can perform a blocking HTTP fetch; change this so JWKS are fetched/updated off the auth hot path and requests use a cached key set: add startup initialization (and/or a periodic background refresher) that calls jwks_client.get_jwks() or equivalent to populate an in-memory cache, ensure the refresher uses explicit HTTP timeouts and backoff and stores last-known keys, then modify the authentication flow (the block around settings.OIDC_ISSUER_URL, jwks_client.get_signing_key_from_jwt, and jwt.decode) to read the signing key from that cache and fall back to a cached key or return a graceful authentication error on cache miss rather than performing network I/O; also ensure exceptions from the background fetch are logged and do not block request threads.
🧹 Nitpick comments (2)
backend/tests/test_runner_connector.py (1)
67-76: ⚡ Quick winAdd a non-object JSON payload test.
handle_message’s contract includes rejecting decoded JSON that is not an object, but that path is not asserted here yet. A small test for[](or"text") would close this gap and prevent regressions.Suggested test addition
`@pytest.mark.asyncio` async def test_handle_fetch_imap_requires_account(): connector = SelfHostedConnector("ws://gateway.example/api/runner/ws", "token") connector.send_response = AsyncMock() await connector.handle_message(json.dumps({"action": "fetch_imap"})) connector.send_response.assert_awaited_once_with( {"status": "error", "action": "fetch_imap", "error": "missing account"} ) + + +@pytest.mark.asyncio +async def test_handle_message_reports_non_object_payload(): + connector = SelfHostedConnector("ws://gateway.example/api/runner/ws", "token") + connector.send_response = AsyncMock() + + await connector.handle_message(json.dumps(["not", "an", "object"])) + + connector.send_response.assert_awaited_once_with( + {"status": "error", "action": None, "error": "invalid payload"} + )🤖 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 `@backend/tests/test_runner_connector.py` around lines 67 - 76, Add a new async test in backend/tests/test_runner_connector.py that verifies SelfHostedConnector.handle_message rejects decoded JSON that is not an object: instantiate SelfHostedConnector("ws://gateway.example/api/runner/ws", "token"), replace connector.send_response with an AsyncMock, call await connector.handle_message(json.dumps([])) (or json.dumps("text")), and assert connector.send_response was awaited once and that the sent response is a mapping with "status" == "error" and contains an "error" field (you can check response.get("status") == "error" and "error" in response) to cover the non-object JSON path.scripts/ci/test_strix_quick_gate.sh (1)
83-95: ⚡ Quick winAdd a semantic negative test for
openai/gpt-5rejection.These checks validate workflow text, but they won’t catch glob logic that still accepts
openai/gpt-5. Add a gate behavior test case that expects failure for plainopenai/gpt-5and success foropenai/gpt-5.4.As per coding guidelines, “Strix Security Scan must use an OpenAI GPT-5.4-or-newer model...”.
🤖 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 `@scripts/ci/test_strix_quick_gate.sh` around lines 83 - 95, Add a semantic negative/positive test that actually runs the gate logic against a workflow snippet to ensure plain "openai/gpt-5" is rejected while "openai/gpt-5.4" is accepted: update the test harness in scripts/ci/test_strix_quick_gate.sh to create two temporary workflow inputs (one containing provider "openai/gpt-5" and one "openai/gpt-5.4") and assert the gate run fails for the former and succeeds for the latter (use the existing helper/assert functions such as assert_file_contains/assert_file_not_contains and the workflow_file variable to locate the content, and the gate invocation/exit-code checks already used elsewhere in this script). Ensure the new assertions check exit status or gate output (e.g., expecting non-zero/zero exit codes or specific failure message) so glob-acceptance logic is exercised rather than just substring checks.
🤖 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 `@backend/services/access_policy.py`:
- Around line 53-70: _role_allowed currently intersects equivalence sets
(ROLE_EQUIVALENTS via _equivalent_roles) which misses hierarchical privilege
(e.g., tenant_admin should satisfy member); change it to first canonicalize
aliases using _equivalent_roles, then consult the canonical role hierarchy in
backend.core.rbac (import the role-to-level or comparator function defined
there) and return true if any request role's level is >= the permitted role's
level (one-way comparison) rather than using set intersection; update
_role_allowed to iterate permitted_roles and compare levels after alias
canonicalization.
In `@frontend/src/components/WorkspaceHome.tsx`:
- Line 153: The span in WorkspaceHome.tsx currently renders a hard-coded
timestamp string; replace it with a real, dynamically formatted time value
(e.g., compute a formatted Date string) and render that instead. Implement this
by producing the formatted time in a client-only hook or prop (for example,
useState + useEffect to set formattedTime on mount or accept a server-generated
prop) and render formattedTime inside the span that currently has
suppressHydrationWarning and className "break-keep text-sm font-medium
text-muted-foreground"; ensure the formatting uses your project's date utility
(or Intl/Date) and preserves the suppressHydrationWarning to avoid SSR hydration
mismatches.
---
Outside diff comments:
In `@backend/api/auth.py`:
- Around line 154-169: The current request-path code calls
jwks_client.get_signing_key_from_jwt(token) which can perform a blocking HTTP
fetch; change this so JWKS are fetched/updated off the auth hot path and
requests use a cached key set: add startup initialization (and/or a periodic
background refresher) that calls jwks_client.get_jwks() or equivalent to
populate an in-memory cache, ensure the refresher uses explicit HTTP timeouts
and backoff and stores last-known keys, then modify the authentication flow (the
block around settings.OIDC_ISSUER_URL, jwks_client.get_signing_key_from_jwt, and
jwt.decode) to read the signing key from that cache and fall back to a cached
key or return a graceful authentication error on cache miss rather than
performing network I/O; also ensure exceptions from the background fetch are
logged and do not block request threads.
In `@frontend/src/components/DashboardLayout.tsx`:
- Around line 82-96: splitHref currently strips the query string, so
isActivePath (used by the mail shortcuts) only compares pathname and hash and
treats all /mail?folder=* links as the same active path; update the matching to
include the URL search/query when determining active state. Modify splitHref to
preserve and return the search portion and adjust isActivePath to compare
pathname + search (or include search in the equality/startsWith checks) in
addition to hash/currentHash so links like /mail?folder=inbox vs
/mail?folder=archive are distinguished (refer to splitHref and isActivePath
functions and the usePathname consumer logic).
---
Nitpick comments:
In `@backend/tests/test_runner_connector.py`:
- Around line 67-76: Add a new async test in
backend/tests/test_runner_connector.py that verifies
SelfHostedConnector.handle_message rejects decoded JSON that is not an object:
instantiate SelfHostedConnector("ws://gateway.example/api/runner/ws", "token"),
replace connector.send_response with an AsyncMock, call await
connector.handle_message(json.dumps([])) (or json.dumps("text")), and assert
connector.send_response was awaited once and that the sent response is a mapping
with "status" == "error" and contains an "error" field (you can check
response.get("status") == "error" and "error" in response) to cover the
non-object JSON path.
In `@scripts/ci/test_strix_quick_gate.sh`:
- Around line 83-95: Add a semantic negative/positive test that actually runs
the gate logic against a workflow snippet to ensure plain "openai/gpt-5" is
rejected while "openai/gpt-5.4" is accepted: update the test harness in
scripts/ci/test_strix_quick_gate.sh to create two temporary workflow inputs (one
containing provider "openai/gpt-5" and one "openai/gpt-5.4") and assert the gate
run fails for the former and succeeds for the latter (use the existing
helper/assert functions such as assert_file_contains/assert_file_not_contains
and the workflow_file variable to locate the content, and the gate
invocation/exit-code checks already used elsewhere in this script). Ensure the
new assertions check exit status or gate output (e.g., expecting non-zero/zero
exit codes or specific failure message) so glob-acceptance logic is exercised
rather than just substring checks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e0ce81e-7ef0-4ba9-9a85-91eb2c0c3435
📒 Files selected for processing (30)
.github/workflows/strix.ymlAGENTS.mdbackend/api/auth.pybackend/api/calendar.pybackend/api/llm_providers.pybackend/api/ontology.pybackend/api/runner_config.pybackend/api/tenant_config.pybackend/core/config.pybackend/core/rbac.pybackend/core/telemetry.pybackend/db/models.pybackend/runner/connector.pybackend/services/access_policy.pybackend/services/caldav_service.pybackend/services/webdav_service.pybackend/tests/conftest.pybackend/tests/test_auth_real.pybackend/tests/test_dav_sync.pybackend/tests/test_ontology_api.pybackend/tests/test_runner_connector.pybackend/tests/test_tenant_config_api.pybackend/tests/test_webdav_api.pyfrontend/playwright.config.tsfrontend/src/components/DashboardLayout.tsxfrontend/src/components/WorkspaceHome.tsxfrontend/tests/e2e/dashboard-branding.spec.tsfrontend/tests/e2e/mobile-hamburger.spec.tsfrontend/tests/e2e/scrolling.spec.tsscripts/ci/test_strix_quick_gate.sh
✅ Files skipped from review due to trivial changes (2)
- frontend/tests/e2e/dashboard-branding.spec.ts
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/tests/conftest.py
- backend/core/telemetry.py
- backend/core/config.py
- frontend/playwright.config.ts
- backend/services/caldav_service.py
- backend/api/ontology.py
- backend/runner/connector.py
- frontend/tests/e2e/mobile-hamburger.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@backend/tests/test_webdav_api.py`:
- Around line 186-188: Replace the broad except Exception block that disposes
engine and calls pytest.skip with a targeted handler: catch only explicit
connectivity/bootstrap errors (e.g., psycopg2.OperationalError,
asyncpg.exceptions.CannotConnectNow or a custom PostgresConnectionError) and
call pytest.skip with a static non-secret message like "PostgreSQL smoke path
unavailable" (do not interpolate exc); for any other exception re-raise so
schema/SQL regressions fail the test; ensure engine.dispose is called in a
finally block (or before re-raising) so cleanup always happens.
- Around line 132-157: The test currently deletes any webdav_accounts rows for
user_id "alice", which can interfere with other tests; update the setup and
cleanup SQL in the test that manipulates the webdav_accounts table so it uses a
test-unique identifier (e.g., a UUID-based user_id or a unique account_id
constant) and only deletes the exact row(s) created by this test. Specifically,
change the DELETE statements that target webdav_accounts (the statements using
parameters {"account_id": 8871, "user_id": "alice"} and the similar blocks later
around the other occurrences) to pass and match the unique user_id/account_id
you generate for this test and use those same identifiers when inserting and
cleaning up to ensure isolation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f91fe22-5e9b-4003-9ec5-d938963c3c08
📒 Files selected for processing (12)
backend/api/ontology.pybackend/api/webdav.pybackend/core/config.pybackend/core/telemetry.pybackend/main.pybackend/runner/connector.pybackend/services/webdav_service.pybackend/tests/test_apm_observability.pybackend/tests/test_config.pybackend/tests/test_ontology_api.pybackend/tests/test_runner_connector.pybackend/tests/test_webdav_api.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/core/config.py
- backend/api/ontology.py
- backend/runner/connector.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/services/access_policy.py (1)
69-76: ⚡ Quick winUse deterministic role canonicalization in
_role_allowed(avoidnext(iter(frozenset))).
_role_allowedcurrently picks an arbitrary representative from the role equivalencefrozensetvianext(iter(...)). While the current RBAC implementation assigns the same hierarchy rank to each alias pair (tenant_admin/organization_adminboth 2,system_admin/platform_adminboth 3), this is brittle and can cause future regressions if the hierarchy or equivalence logic changes.💡 Suggested fix
ROLE_EQUIVALENTS: dict[str, frozenset[str]] = { @@ } +CANONICAL_ROLE: dict[str, str] = { + "system_admin": "system_admin", + "platform_admin": "system_admin", + "tenant_admin": "tenant_admin", + "organization_admin": "tenant_admin", + "group_admin": "group_admin", + "member": "member", +} + +def _canonical_role(role: str) -> str: + return CANONICAL_ROLE.get(role, role) + def _equivalent_roles(role: str) -> frozenset[str]: return ROLE_EQUIVALENTS.get(role, frozenset({role})) @@ def _role_allowed(role: str, permitted_roles: tuple[PolicyRoleName, ...]) -> bool: if _is_system_admin_role(role): return any(_is_system_admin_role(item) for item in permitted_roles) - request_role = next(iter(_equivalent_roles(role))) + request_role = _canonical_role(role) return any( - check_tenant_access(request_role, next(iter(_equivalent_roles(item)))) + check_tenant_access(request_role, _canonical_role(item)) for item in permitted_roles )🤖 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 `@backend/services/access_policy.py` around lines 69 - 76, _role_allowed uses next(iter(_equivalent_roles(role))) which picks an arbitrary member from a frozenset; make canonicalization deterministic by selecting a stable representative instead (e.g., implement a small helper _canonical_role(role) that returns a deterministic member from _equivalent_roles(role) using sorted(...) or a fixed mapping) and use that in _role_allowed for both the request role and each permitted role; keep the existing checks (_is_system_admin_role and check_tenant_access) but replace next(iter(...)) calls with the new deterministic _canonical_role to avoid non-deterministic behavior.
🤖 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.
Nitpick comments:
In `@backend/services/access_policy.py`:
- Around line 69-76: _role_allowed uses next(iter(_equivalent_roles(role)))
which picks an arbitrary member from a frozenset; make canonicalization
deterministic by selecting a stable representative instead (e.g., implement a
small helper _canonical_role(role) that returns a deterministic member from
_equivalent_roles(role) using sorted(...) or a fixed mapping) and use that in
_role_allowed for both the request role and each permitted role; keep the
existing checks (_is_system_admin_role and check_tenant_access) but replace
next(iter(...)) calls with the new deterministic _canonical_role to avoid
non-deterministic behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41d5a8e3-2375-4113-8459-0e1613e12f6b
📒 Files selected for processing (10)
backend/api/auth.pybackend/main.pybackend/services/access_policy.pybackend/tests/test_access_policy.pybackend/tests/test_auth_real.pybackend/tests/test_webdav_api.pyfrontend/src/components/DashboardLayout.test.tsxfrontend/src/components/DashboardLayout.tsxfrontend/src/components/WorkspaceHome.tsxscripts/ci/test_strix_quick_gate.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/WorkspaceHome.tsx
- backend/tests/test_webdav_api.py
- backend/main.py
seonghobae
enabled auto-merge
May 27, 2026 01:19
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/test_auth_real.py (1)
678-714: 💤 Low valueConsider using
monkeypatch.setattrfor settings to simplify teardown.The manual save/restore pattern for
settings.OIDC_ISSUER_URL,settings.OIDC_CLIENT_ID, etc. is repeated across all three OIDC tests. Usingmonkeypatch.setattr(settings, "OIDC_ISSUER_URL", ...)would auto-restore on test exit, eliminating the try/finally blocks.♻️ Example simplification
async def test_signed_bearer_session_with_oidc(monkeypatch): import jwt - - previous_issuer_url = settings.OIDC_ISSUER_URL - previous_client_id = settings.OIDC_CLIENT_ID - previous_secret = settings.AUTH_SESSION_HMAC_SECRET - settings.OIDC_ISSUER_URL = "http://localhost:8081/realms/naruon" - settings.OIDC_CLIENT_ID = "naruon-api" - settings.AUTH_SESSION_HMAC_SECRET = SecretStr(TEST_SESSION_HMAC_SECRET) + + monkeypatch.setattr(settings, "OIDC_ISSUER_URL", "http://localhost:8081/realms/naruon") + monkeypatch.setattr(settings, "OIDC_CLIENT_ID", "naruon-api") + monkeypatch.setattr(settings, "AUTH_SESSION_HMAC_SECRET", SecretStr(TEST_SESSION_HMAC_SECRET)) # ... rest of test setup ... - try: - token = _signed_session_token(...) - context = await get_auth_context(authorization=f"Bearer {token}") - finally: - settings.OIDC_ISSUER_URL = previous_issuer_url - settings.OIDC_CLIENT_ID = previous_client_id - settings.AUTH_SESSION_HMAC_SECRET = previous_secret + token = _signed_session_token(...) + context = await get_auth_context(authorization=f"Bearer {token}")Also applies to: 725-754, 761-785
🤖 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 `@backend/tests/test_auth_real.py` around lines 678 - 714, Replace the manual save/restore pattern around settings in the OIDC tests by using monkeypatch.setattr to set settings.OIDC_ISSUER_URL, settings.OIDC_CLIENT_ID and settings.AUTH_SESSION_HMAC_SECRET (e.g. monkeypatch.setattr(settings, "OIDC_ISSUER_URL", "...") and similarly for the others) so pytest will auto-restore them; remove the try/finally block that restores previous_issuer_url/previous_client_id/previous_secret and apply the same replacement in the other two OIDC test blocks that touch those same symbols (the test code calling _signed_session_token and get_auth_context should remain unchanged).
🤖 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.
Nitpick comments:
In `@backend/tests/test_auth_real.py`:
- Around line 678-714: Replace the manual save/restore pattern around settings
in the OIDC tests by using monkeypatch.setattr to set settings.OIDC_ISSUER_URL,
settings.OIDC_CLIENT_ID and settings.AUTH_SESSION_HMAC_SECRET (e.g.
monkeypatch.setattr(settings, "OIDC_ISSUER_URL", "...") and similarly for the
others) so pytest will auto-restore them; remove the try/finally block that
restores previous_issuer_url/previous_client_id/previous_secret and apply the
same replacement in the other two OIDC test blocks that touch those same symbols
(the test code calling _signed_session_token and get_auth_context should remain
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0a59bf2-6a2b-4f1a-9a09-1c990095df01
📒 Files selected for processing (7)
backend/core/telemetry.pybackend/services/access_policy.pybackend/tests/test_access_policy.pybackend/tests/test_apm_observability.pybackend/tests/test_auth_real.pybackend/tests/test_ontology_api.pyfrontend/src/components/DashboardLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/test_apm_observability.py
- backend/tests/test_access_policy.py
- backend/tests/test_ontology_api.py
This was referenced May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
This PR implements Phase 18 and ensures all required frontend layouts and backend components exist.
Summary by CodeRabbit
New Features
Improvements
Chores
Tests