test(api): cover tenant settings read and admin mutation - #429
Conversation
Two @app.get decorators were stacked before read_tenant_settings:
@app.get("/healthz")
@app.get("/api/settings", response_model=dict)
async def read_tenant_settings(...):
Both bound to the same handler -- "/healthz" required authentication
(read_tenant_settings depends on get_current_account) and the real
healthz() function below had no route decorator at all, so it was
dead code never reachable by any request. docker-compose.yml's own
backend healthcheck hits "/healthz" with a plain unauthenticated
urllib.request.urlopen call; against this bug it would receive
401/403, fail the healthcheck, and mark the container unhealthy on
every fresh deployment.
Move the decorator onto healthz() where it belongs.
Also add migration 0103_tenant_settings.sql to backend/tests/test_api.py's
seeded_db fixture -- it was never added when the migration shipped, so
the tenant_settings table (and therefore the /api/settings GET/PATCH
endpoints, both previously untested) didn't exist in the test schema
at all.
Tests: test_healthz_is_reachable_without_a_token (the regression this
bug needed) plus three new /api/settings tests (GET returns the seeded
brand name, PATCH requires post_admin, PATCH as admin actually changes
it). uv run --frozen python -m pytest -q: 757 passed, 17 skipped.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
An earlier `if (!accessToken) return` a few hundred lines up already narrows accessToken to string for the rest of the authenticated render tree -- confirmed with a clean tsc build without the guard.
|
Confirmed and fixed: the |
|
Confirmed, thanks -- no action needed. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Same shared-ancestor bug as #418/#415/#426/#427/#429/#431: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(). The authenticated- branch AdminPanel render (line 4693) is untouched here -- an earlier `if (!accessToken) return` already narrows accessToken to string there, so no additional guard is needed (confirmed via a clean tsc build).
|
Confirmed — the login-screen AdminPanel render was unreachable through normal navigation ( |
Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render (accessToken is always undefined pre-auth). This PR's own AdminPanel.test.tsx/.stories.tsx render the component directly, so this doesn't affect its coverage.
Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434/#435/#436: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render (accessToken is always undefined pre-auth). This PR's own diff doesn't touch AdminPanel.
Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434/#435/#436/#437: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render (accessToken is always undefined pre-auth). This PR's own diff doesn't touch AdminPanel.
|
Nudging a fresh scheduler dispatch + Strix run — checks were green except a stale strix failure from before the last push, with no recent scheduler activity picking it back up. |
Pull request was closed
* test(frontend): add Storybook coverage for BuyerNav BuyerNav had a test file but no story, the last remaining gap in frontend/src/components/*.tsx test+story coverage. Adds stories for each destination plus an edge case with an extra tools slot. * fix(frontend): use OIDC return-url helpers on the login button Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434/#435/#436/#437: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render (accessToken is always undefined pre-auth). This PR's own diff doesn't touch AdminPanel. * test(frontend): cover WorkspaceNav in Storybook
|
@opencode-agent independent exact-head review requested. This identity cannot self-approve.
|
|
@opencode-agent independent exact-head review requested. This identity cannot self-approve.
|
|
@opencode-agent @cwl-noema-review current-head review for
|
* test(frontend): cover LineageDag with tests and stories LineageDag renders the git-branch-style multi-thread lineage graph used by both the post-detail popup and the Ask Agent's multi-lineage answer view (ADR 0120), and had zero test or story coverage despite being a core, non-trivial component. Adds tests for the empty state, multi-group branch rendering, group-heading fallback for missing/UUID groups, click and keyboard node selection, the current-post marker, and label truncation with an accessible full-label fallback. Adds stories for empty, single-branch, multi-branch (with an actual fork), ungrouped, and long-label scenarios. * fix(frontend): use OIDC return-url helpers on the login button Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434/#435/#436/#437/#438: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render. * Revert "fix(frontend): use OIDC return-url helpers on the login button" This reverts commit 590c6c3. * fix(frontend): keep lineage edge evidence visible-only * fix(frontend): terminate rooted lineage cycles * chore(frontend): keep shared OIDC repair on #426 * refactor(frontend): remove unreachable cycle guard * test(frontend): cover converging lineage DAGs * fix(frontend): position converging DAG nodes once
* test(frontend): cover FiveW1H component with tests and stories Adds Vitest coverage for loading state, empty-evidence messaging, raw-source-to-label mapping (including the unmapped fallback), and optional evidence-text/ontology-badge rendering, plus a Storybook inventory covering the loading, all-empty, grounded-answer, and unmapped-source scenarios. * fix(frontend): use OIDC return-url helpers on the login button Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434/#435/#436: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render (accessToken is always undefined pre-auth). This PR's own diff doesn't touch AdminPanel. * chore(frontend): keep FiveW1H coverage dependency-correct Remove the duplicated OIDC/login changes owned by #426 so this PR carries only its FiveW1H tests and Storybook inventory.
* test(api): cover POST /api/ask, GET /api/rankings, PATCH /api/me/preferences Found via a systematic route-vs-test cross-reference (every @app.get/ post/patch/put/delete path in backend/app/main.py checked against every test file, not just backend/tests/test_api.py) -- same technique that found the /healthz routing bug earlier this session. All three endpoints had zero test coverage anywhere in the repo: - POST /api/ask: the Ask Agent endpoint itself was never exercised at the HTTP layer, despite its underlying functions (gather_global_chat_sources, cited_post_evidence, ...) being unit-tested. New tests cover the empty-question 422, the no-orchestrator-configured 503 (Null client, matching the existing derive-commitment 503 test's monkeypatch pattern), and the unauthenticated 401/403 case. - GET /api/rankings: covers the real response contract (RankWeave's own "never invent a fused score" fail-closed shape -- status is either "accepted" or "unavailable", never a guessed ranking) plus the unauthenticated case. - PATCH /api/me/preferences: covers persisting a supported locale (round-tripped through GET /api/me) and rejecting an unsupported one (Pydantic's own Literal validation, previously untested). uv run --frozen python -m pytest -q: 760 passed, 17 skipped. * fix(frontend): use OIDC return-url helpers on the login button Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434/#436: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render (accessToken is always undefined pre-auth). * Revert "fix(frontend): use OIDC return-url helpers on the login button" This reverts commit b80628b. * test(api): verify buyer route trust boundaries * fix: remove unused httpx2 dependency * docs: keep API test transport provenance accurate Remove stale httpx2 claims after the look-alike dependency was deleted; the harness uses Starlette TestClient with the official httpx dev dependency. * fix(frontend): repair the inherited login/admin-panel build break Two TypeScript build errors on main (blocking every open PR's "Frontend lint, test, build" check, including this repo's own review bot's ability to approve them): - App.tsx imported rememberOidcReturnUrl/returnUrlFromLocation from oidcReturnUrl.ts but never called them -- the login button built its own unsanitized returnUrl inline instead of using the safe helper (oidcReturnUrl.ts's isSafeReturnUrl guard against an open-redirect- shaped value) or persisting it as the sessionStorage/localStorage fallback restoreOidcReturnUrl (already wired up on the callback side in main.tsx) reads when the OIDC state round-trip drops it. - The unauthenticated login screen unconditionally rendered <AdminPanel accessToken={accessToken} /> when destination === "admin" -- accessToken is string | undefined here (always undefined while unauthenticated), a real type error, and the render was unreachable through normal navigation (destination only changes via the authenticated nav) -- dead code, removed. uv run --frozen python -m pytest -q: 753 passed, 17 skipped. pnpm run test: 140 passed. pnpm run lint / build: clean. * Revert "fix(frontend): repair the inherited login/admin-panel build break" This reverts commit 36164fb.
|
@opencode-agent @cwl-noema-review current-head review for |
* test(admin): cover AdminPanel (previously zero coverage) Found via a systematic component-vs-story-vs-test cross-reference of frontend/src/components/*.tsx -- AdminPanel had neither a .test.tsx nor a .stories.tsx, unlike every other component in the directory. Tests cover: save disabled until the brand name actually changes, a successful save calling updateTenantConfig (backend/app/main.py's PATCH /api/settings, now covered separately in #435) with the right arguments and reporting the new name back to the caller, and a failed save showing the error while leaving the form editable (not stuck disabled). Storybook stories cover the default state and a long brand-name layout edge case. pnpm run test: 143 passed (was 140). pnpm run lint: no new warnings. pnpm run build: the two pre-existing App.tsx errors are the unrelated main break already fixed in #426, not something this PR touches. * fix(frontend): use OIDC return-url helpers on the login button Same shared-ancestor bug as #418/#415/#426/#427/#429/#431/#434: the login button built an unsanitized returnUrl inline instead of returnUrlFromLocation()/rememberOidcReturnUrl(), and removed the unreachable login-screen AdminPanel render (accessToken is always undefined pre-auth). This PR's own AdminPanel.test.tsx/.stories.tsx render the component directly, so this doesn't affect its coverage. * fix(admin): guard no-op saves and announce results * docs(storybook): inventory tenant settings
|
@opencode-agent @cwl-noema-review current-head review for
|
|
@opencode-agent @cwl-noema-review current-head review for /healthz liveness routing fix. Independent OpenCode / Strix / Noema required. This identity cannot self-approve. |
|
Exact-head ping for independent OpenCode/Strix/Noema review on |
|
@opencode-agent @cwl-noema-review exact-head independent review for
|
|
@opencode-agent @cwl-noema-review exact-head independent review for
|
|
Exact-head independent review required on
|
|
@opencode-agent @cwl-noema-review exact-head independent review for
|
|
@opencode-agent @cwl-noema-review exact-head independent review for All product threads resolved. |
|
@opencode-agent @cwl-noema-review exact-head independent review for
|
|
@opencode-agent @cwl-noema-review exact-head independent APPROVE required for
|
…mpose-20260824 # Conflicts: # frontend/src/App.test.tsx
seonghobae
changed the title
…hz-routing' into agent-pr429-compose-20260824 # Conflicts: # backend/tests/test_api.py
5aa5bfa
into
fix/docstring-coverage-and-healthz-routing
* fix: restore /healthz routing and close the docstring-coverage gap
A stray decorator had stacked GET /healthz onto read_tenant_settings,
so the liveness probe silently required auth and hit Postgres instead
of returning {"status": "ok"}; the real healthz() handler had no route
at all. Restored the decorator to the correct handler and added a
regression test.
Also closed the repository-wide docstring-coverage gap: an AST audit
of lineageweave/ and backend/app/ found 35 public functions/classes
missing docstrings (excluding private/dunder names, __init__.py, and
tests). Added them all, plus two leftover "buyer" wording references
from before the terminology rename.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011EP69xAyLaJxa6oaF6D9eq
* fix(frontend): preserve login return destination
* test: enforce public docstring coverage
* test: cover docstring gate failure evidence
* test(api): cover tenant settings read and admin mutation (#429)
* fix(api): route /healthz to the actual liveness probe, not settings
Two @app.get decorators were stacked before read_tenant_settings:
@app.get("/healthz")
@app.get("/api/settings", response_model=dict)
async def read_tenant_settings(...):
Both bound to the same handler -- "/healthz" required authentication
(read_tenant_settings depends on get_current_account) and the real
healthz() function below had no route decorator at all, so it was
dead code never reachable by any request. docker-compose.yml's own
backend healthcheck hits "/healthz" with a plain unauthenticated
urllib.request.urlopen call; against this bug it would receive
401/403, fail the healthcheck, and mark the container unhealthy on
every fresh deployment.
Move the decorator onto healthz() where it belongs.
Also add migration 0103_tenant_settings.sql to backend/tests/test_api.py's
seeded_db fixture -- it was never added when the migration shipped, so
the tenant_settings table (and therefore the /api/settings GET/PATCH
endpoints, both previously untested) didn't exist in the test schema
at all.
Tests: test_healthz_is_reachable_without_a_token (the regression this
bug needed) plus three new /api/settings tests (GET returns the seeded
brand name, PATCH requires post_admin, PATCH as admin actually changes
it). uv run --frozen python -m pytest -q: 757 passed, 17 skipped.
* fix(frontend): use OIDC return-url helpers and guard AdminPanel render
Same shared-ancestor bug as #418/#415/#426/#427: the login button built
an unsanitized returnUrl inline instead of returnUrlFromLocation()/
rememberOidcReturnUrl(), and AdminPanel's accessToken (string, required)
was rendered from a string | undefined at both call sites.
* fix(frontend): drop redundant accessToken guard on AdminPanel render
An earlier `if (!accessToken) return` a few hundred lines up already
narrows accessToken to string for the rest of the authenticated render
tree -- confirmed with a clean tsc build without the guard.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Buyer problem
The liveness routing fix alone does not prove that the adjacent tenant-settings surface still works against the real normalized PostgreSQL schema. A deployment could report healthy while authenticated readers cannot load the brand or admins cannot persist a change.
Unique scope
0103_tenant_settings.sqlin the realistic API database fixture.post_adminrole, persist a synthetic brand change, and read it back through the API.The duplicate
/healthzand OIDC/Admin source changes are owned by parent PR #498 and are absent from this effective diff. The parent already retains the public liveness regression and public-docstring gate.Exact composition
df241b761df2d3a8fb8e8f8a4b461e4a7004e429.2081f7deba6c66edd73713e1566166fb798e469a(fix/docstring-coverage-and-healthz-routing).After #498 lands on protected
main, retarget this PR tomain, refetch exact head/base, and require fresh terminal checks plus independent exact-head approval. Do not transfer stacked-base evidence.Exact validation
uv run pytest backend/tests/test_api.py -k 'healthz or settings' -q: 4 passed, 103 deselected against the reachable local PostgreSQL fixture.uv run pytest -q: 842 passed, 17 skipped, 4 dependency warnings.git diff --check: passed.backend/tests/test_api.py: empty.No real organization or person data is used; all brand values are synthetic.