fix: global extension bugs — list_extensions, catch-all handler, tests#902
Merged
fix: global extension bugs — list_extensions, catch-all handler, tests#902
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The APIManager.list_extensions() method defaulted to querying @internal when no room was specified, hiding @global and room-scoped extensions. Now falls back to self.room_id so the server returns all visible extensions (global + internal + room-scoped). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add InternalServerError import and catch-all @app.exception_handler(Exception) - Logs full traceback server-side, returns application/problem+json - Refactor _validation_exception_handler to delegate to problem_exception_handler instead of building its own JSONResponse with model_dump Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- test_list_extensions_includes_global: verify list_extensions() returns @global extensions - test_unhandled_exception_returns_problem_json: verify InternalServerError is registered - test_append_frame_with_nested_info_dict: regression test for frames with nested dict+numpy in atoms.info Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 (4)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAdds an RFC 9457 InternalServerError problem type and global FastAPI exception handler (returns problem+json and logs trace), fixes APIManager.list_extensions() to default to the client's room_id so Changes
Sequence Diagram(s)(omitted) Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
The catch-all exception handler receives `exc` from FastAPI, not from an `except` block, so ruff LOG014 is a false positive. Renamed test to accurately reflect it checks problem type registration, not handler behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <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: 5
🧹 Nitpick comments (2)
tests/zndraw/test_problems.py (1)
53-61: Exercise the actual exception path here.This only checks the documentation page for
/v1/problems/internal-server-error;_unhandled_exception_handler()could regress and this test would still pass. Please hit a route that really raises and assert the500application/problem+jsonbody.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_problems.py` around lines 53 - 61, The current test only checks the documentation endpoint and can miss regressions in the actual handler; update or add a test (e.g., in test_unhandled_exception_returns_problem_json) that calls a real route which raises an unhandled exception (create or reuse a test-only endpoint that raises, or trigger the existing handler that throws) and assert the response has HTTP 500, Content-Type "application/problem+json", and a problem JSON body containing "title": "Internal Server Error" and "status": 500; ensure this exercises the framework handler (_unhandled_exception_handler) rather than the docs page.tests/zndraw/test_routes_frames.py (1)
921-961: Assert the nestedcube_datapayload, not just the top-level key.Right now a serializer that stores an empty or malformed
info.cube_datavalue would still pass. Please unpackframes[0][b"info.cube_data"]and verify the nested keys/shapes or representative values so the Isosurface round-trip is actually covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/zndraw/test_routes_frames.py` around lines 921 - 961, The test test_append_frame_with_nested_info_dict currently only asserts the presence of the top-level key b"info.cube_data" but not its contents; modify the test to unpack frames[0][b"info.cube_data"] (after decode_msgpack_response) and assert the nested payload contains expected keys and representative values/types — e.g. verify presence of "grid","origin","cell", that "grid" has the expected shape (10x10x10) or length, "origin" has three elements, and "cell" is a 3x3 array (or appropriate shapes/types) to ensure the numpy arrays round-trip correctly from atoms.info["cube_data"] through atoms_to_json_dict and the frames endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-01-global-extension-bugs.md`:
- Line 166: The fenced code block currently missing a language identifier should
be updated to include the appropriate language tag (e.g., ```bash, ```js,
```json, etc.) so markdownlint MD040 is satisfied; locate the fenced block that
lacks a language specifier and add the correct language identifier matching the
block's contents.
- Around line 205-219: The handler _unhandled_exception_handler currently
returns InternalServerError.exception(detail=str(exc)), which can leak sensitive
internals; change it to return a generic, client-safe detail (e.g., "Internal
server error" or similar) while keeping the full exception/traceback in logs via
logger.error(exc_info=True). Update the call to
problem_exception_handler/InternalServerError.exception so the detail is the
generic message and do not include str(exc) in the response payload; keep
logging the actual exc via logger.error for debugging.
- Around line 241-259: The test test_unhandled_exception_returns_problem_json in
tests/zndraw/test_problems.py only hits the problems documentation endpoint and
doesn't exercise the actual catch-all exception handler
(`@app.exception_handler`(Exception)); update the test to call a real endpoint
that raises an unhandled Exception (or inject/mock a handler to raise) so the
request triggers the global exception handler at runtime, then assert the
response status_code is 500, the Content-Type is application/problem+json, and
the JSON body contains the expected problem fields (status: 500, title:
"Internal Server Error" and any problem-type identifier like
"internal-server-error"); keep or separately assert that the registry endpoint
(/v1/problems/internal-server-error) still returns the docs if needed.
In `@docs/superpowers/specs/2026-04-01-global-extension-bugs-design.md`:
- Around line 166-170: Add a language tag "text" to the fenced block containing
the "room" description so the opening fence becomes ```text; this fixes
markdownlint MD040 for the block that reads: "room\n Room scope. Defaults to
``\"@global\"`` when called directly.\n ``ZnDraw.register_job()`` resolves
*None* to the client's room." — update that specific fenced code block
surrounding the "room" paragraph to start with ```text and keep the existing
content unchanged.
In `@src/zndraw/app.py`:
- Around line 75-89: The handler _unhandled_exception_handler should not echo
raw exception text to clients; keep detailed trace in logs but return a generic
500 detail. Update the return to call problem_exception_handler(request,
InternalServerError.exception(detail="Internal server error")) (or similar
generic message) instead of using str(exc), while leaving the logger.error(...,
exc_info=True) unchanged so full traceback remains in logs; ensure the function
still returns the JSONResponse from problem_exception_handler.
---
Nitpick comments:
In `@tests/zndraw/test_problems.py`:
- Around line 53-61: The current test only checks the documentation endpoint and
can miss regressions in the actual handler; update or add a test (e.g., in
test_unhandled_exception_returns_problem_json) that calls a real route which
raises an unhandled exception (create or reuse a test-only endpoint that raises,
or trigger the existing handler that throws) and assert the response has HTTP
500, Content-Type "application/problem+json", and a problem JSON body containing
"title": "Internal Server Error" and "status": 500; ensure this exercises the
framework handler (_unhandled_exception_handler) rather than the docs page.
In `@tests/zndraw/test_routes_frames.py`:
- Around line 921-961: The test test_append_frame_with_nested_info_dict
currently only asserts the presence of the top-level key b"info.cube_data" but
not its contents; modify the test to unpack frames[0][b"info.cube_data"] (after
decode_msgpack_response) and assert the nested payload contains expected keys
and representative values/types — e.g. verify presence of
"grid","origin","cell", that "grid" has the expected shape (10x10x10) or length,
"origin" has three elements, and "cell" is a 3x3 array (or appropriate
shapes/types) to ensure the numpy arrays round-trip correctly from
atoms.info["cube_data"] through atoms_to_json_dict and the frames endpoint.
🪄 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: 428ae66a-6029-45d2-b8be-faccad32d076
📒 Files selected for processing (9)
docs/superpowers/plans/2026-04-01-global-extension-bugs.mddocs/superpowers/specs/2026-04-01-global-extension-bugs-design.mdsrc/zndraw/app.pysrc/zndraw/client/api.pysrc/zndraw/exceptions.pysrc/zndraw_joblib/client.pytests/zndraw/test_problems.pytests/zndraw/test_routes_frames.pytests/zndraw/worker/test_global.py
| - [ ] **Step 3: Refactor `_validation_exception_handler` to delegate** | ||
|
|
||
| Replace the existing `_validation_exception_handler` (lines 58-71): | ||
|
|
There was a problem hiding this comment.
Add explicit language to the fenced code block at Line 166
markdownlint flags this as MD040. Add a language identifier to keep docs lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-01-global-extension-bugs.md` at line 166, The
fenced code block currently missing a language identifier should be updated to
include the appropriate language tag (e.g., ```bash, ```js, ```json, etc.) so
markdownlint MD040 is satisfied; locate the fenced block that lacks a language
specifier and add the correct language identifier matching the block's contents.
Comment on lines
+205
to
+219
| @app.exception_handler(Exception) | ||
| async def _unhandled_exception_handler( | ||
| request: Request, exc: Exception | ||
| ) -> JSONResponse: | ||
| """Catch-all for unhandled exceptions — log and return RFC 9457.""" | ||
| logger.error( | ||
| "Unhandled %s on %s %s", | ||
| type(exc).__name__, | ||
| request.method, | ||
| request.url.path, | ||
| exc_info=True, | ||
| ) | ||
| return await problem_exception_handler( | ||
| request, InternalServerError.exception(detail=str(exc)) | ||
| ) |
There was a problem hiding this comment.
Avoid returning raw exception messages in 500 responses
At Line 218, using detail=str(exc) can leak sensitive internal data to clients. Keep traceback/details in logs and return a generic client-safe detail in the RFC 9457 payload.
Safer handler snippet
return await problem_exception_handler(
- request, InternalServerError.exception(detail=str(exc))
+ request,
+ InternalServerError.exception(
+ detail="An unexpected error occurred while processing the request."
+ ),
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-01-global-extension-bugs.md` around lines 205
- 219, The handler _unhandled_exception_handler currently returns
InternalServerError.exception(detail=str(exc)), which can leak sensitive
internals; change it to return a generic, client-safe detail (e.g., "Internal
server error" or similar) while keeping the full exception/traceback in logs via
logger.error(exc_info=True). Update the call to
problem_exception_handler/InternalServerError.exception so the detail is the
generic message and do not include str(exc) in the response payload; keep
logging the actual exc via logger.error for debugging.
Comment on lines
+241
to
+259
| - [ ] **Step 1: Write the test** | ||
|
|
||
| Add at the end of `tests/zndraw/test_problems.py`: | ||
|
|
||
| ```python | ||
| @pytest.mark.asyncio | ||
| async def test_unhandled_exception_returns_problem_json(client: AsyncClient) -> None: | ||
| """Unhandled exceptions return application/problem+json with status 500.""" | ||
| # Hit a URL that doesn't exist as a route — FastAPI returns 404. | ||
| # Instead, trigger via an invalid room ID that passes URL parsing | ||
| # but causes an internal error. We verify the catch-all by checking | ||
| # that the registry now includes internal-server-error. | ||
| response = await client.get("/v1/problems/internal-server-error") | ||
| assert response.status_code == 200 | ||
| content = response.text | ||
| assert "# InternalServerError" in content | ||
| assert "**Status:** 500" in content | ||
| assert "**Title:** Internal Server Error" in content | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Task 5 test does not actually verify the catch-all exception handler
At Line 247-259, the test only verifies the problem-type docs endpoint (/v1/problems/internal-server-error, status 200). It does not exercise @app.exception_handler(Exception) or assert a 500 application/problem+json runtime response, so the planned regression can pass even if the catch-all handler is broken.
Proposed plan adjustment
-async def test_unhandled_exception_returns_problem_json(client: AsyncClient) -> None:
- """Unhandled exceptions return application/problem+json with status 500."""
- response = await client.get("/v1/problems/internal-server-error")
- assert response.status_code == 200
- content = response.text
- assert "# InternalServerError" in content
- assert "**Status:** 500" in content
- assert "**Title:** Internal Server Error" in content
+async def test_unhandled_exception_returns_problem_json(client: AsyncClient) -> None:
+ """Unhandled exceptions return application/problem+json with status 500."""
+ response = await client.get("/v1/test/raise-unhandled")
+ assert response.status_code == 500
+ assert response.headers["content-type"].startswith("application/problem+json")
+ body = response.json()
+ assert body["status"] == 500
+ assert body["title"] == "Internal Server Error"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-01-global-extension-bugs.md` around lines 241
- 259, The test test_unhandled_exception_returns_problem_json in
tests/zndraw/test_problems.py only hits the problems documentation endpoint and
doesn't exercise the actual catch-all exception handler
(`@app.exception_handler`(Exception)); update the test to call a real endpoint
that raises an unhandled Exception (or inject/mock a handler to raise) so the
request triggers the global exception handler at runtime, then assert the
response status_code is 500, the Content-Type is application/problem+json, and
the JSON body contains the expected problem fields (status: 500, title:
"Internal Server Error" and any problem-type identifier like
"internal-server-error"); keep or separately assert that the registry endpoint
(/v1/problems/internal-server-error) still returns the docs if needed.
docs/superpowers/specs/2026-04-01-global-extension-bugs-design.md
Outdated
Show resolved
Hide resolved
Comment on lines
+75
to
89
| @app.exception_handler(Exception) | ||
| async def _unhandled_exception_handler( | ||
| request: Request, exc: Exception | ||
| ) -> JSONResponse: | ||
| """Catch-all for unhandled exceptions — log and return RFC 9457.""" | ||
| logger.error( | ||
| "Unhandled %s on %s %s", | ||
| type(exc).__name__, | ||
| request.method, | ||
| request.url.path, | ||
| exc_info=True, | ||
| ) | ||
| return await problem_exception_handler( | ||
| request, InternalServerError.exception(detail=str(exc)) | ||
| ) |
There was a problem hiding this comment.
Don't echo raw exception messages back to clients.
str(exc) can expose internal details such as SQL errors, file paths, or other implementation state. Keep the traceback in the server log, but return a generic 500 detail instead.
🛡️ Suggested change
return await problem_exception_handler(
- request, InternalServerError.exception(detail=str(exc))
+ request,
+ InternalServerError.exception(
+ detail="An unexpected error occurred while processing the request."
+ ),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @app.exception_handler(Exception) | |
| async def _unhandled_exception_handler( | |
| request: Request, exc: Exception | |
| ) -> JSONResponse: | |
| """Catch-all for unhandled exceptions — log and return RFC 9457.""" | |
| logger.error( | |
| "Unhandled %s on %s %s", | |
| type(exc).__name__, | |
| request.method, | |
| request.url.path, | |
| exc_info=True, | |
| ) | |
| return await problem_exception_handler( | |
| request, InternalServerError.exception(detail=str(exc)) | |
| ) | |
| `@app.exception_handler`(Exception) | |
| async def _unhandled_exception_handler( | |
| request: Request, exc: Exception | |
| ) -> JSONResponse: | |
| """Catch-all for unhandled exceptions — log and return RFC 9457.""" | |
| logger.error( | |
| "Unhandled %s on %s %s", | |
| type(exc).__name__, | |
| request.method, | |
| request.url.path, | |
| exc_info=True, | |
| ) | |
| return await problem_exception_handler( | |
| request, | |
| InternalServerError.exception( | |
| detail="An unexpected error occurred while processing the request." | |
| ), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zndraw/app.py` around lines 75 - 89, The handler
_unhandled_exception_handler should not echo raw exception text to clients; keep
detailed trace in logs but return a generic 500 detail. Update the return to
call problem_exception_handler(request,
InternalServerError.exception(detail="Internal server error")) (or similar
generic message) instead of using str(exc), while leaving the logger.error(...,
exc_info=True) unchanged so full traceback remains in logs; ensure the function
still returns the JSONResponse from problem_exception_handler.
Add language tag to unfenced code block in design spec. Assert nested cube_data keys (grid, origin, cell) exist after round-trip, not just the top-level key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #902 +/- ##
==========================================
- Coverage 91.72% 91.65% -0.07%
==========================================
Files 245 245
Lines 22791 22839 +48
==========================================
+ Hits 20904 20933 +29
- Misses 1887 1906 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Summary
list_extensions()defaults to actual room instead of@internal—@globaland room-scoped extensions were hidden from the Python client and CLI becauseAPIManager.list_extensions()hardcoded@internalas fallback. Now usesself.room_idso the server returns all visible extensions (matching frontend behavior).Internal Server Errorplain text with no diagnostics. Now logs full traceback server-side and returnsapplication/problem+jsonvia newInternalServerErrorproblem type. Also refactors_validation_exception_handlerto delegate toproblem_exception_handlerinstead of building its ownJSONResponse.atoms.info— TheIsosurfacefeature depends on storing nested dicts with numpy arrays in frame info. This test ensures the append+read round-trip works.JobManager.register()room default — Docstring said "Defaults to@global" butZnDraw.register_job()resolvesNoneto the client's room.Test plan
uv run pytest tests/zndraw/test_problems.py— 4 passeduv run pytest tests/zndraw/test_routes_frames.py— all passed (including new nested dict test)uv run pytest tests/zndraw/test_cli_agent/test_extensions.py— all passeduv run pytest tests/zndraw/worker/test_global.py— all passed (including new list_extensions test)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests