refactor: shared session load path for session and export APIs#125
Merged
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds shared session-resolution helpers, refactors session and export routes to use them, and updates error-propagation tests. It also centralizes session-load exception classification in a shared constant reused by export error handling. ChangesSession handler consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant sessions.py
participant api._session_handlers
participant get_cached_session
participant compute_stats_or_error
Client->>sessions.py: request session or stats
sessions.py->>api._session_handlers: resolve_loaded_session(project, session_id)
api._session_handlers->>get_cached_session: load cached session
get_cached_session-->>api._session_handlers: session or exception
api._session_handlers-->>sessions.py: LoadedSession or error response
sessions.py->>api._session_handlers: compute_stats_or_error(session, session_id)
api._session_handlers-->>sessions.py: stats dict or error response
sessions.py-->>Client: JSON response or error response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
timon0305
approved these changes
Jul 9, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_error_propagation.py (1)
204-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport endpoint test coverage only covers the exclusion-raise path, not the parse-failure path.
TestExportSessionErrorBodyonly exercisesis_session_excludedraising. The sibling classes (TestGetSessionErrorBody,TestGetSessionStatsErrorBody) each also test aget_cached_sessionraise (KeyError/ValueError) to confirm no leakage on parse failures. Adding an equivalent test for the export endpoint would keep coverage symmetric across all three routes sharingresolve_loaded_session.Suggested additional test
def test_500_on_parse_failure_does_not_leak_class_name( self, tmp_path, export_client, monkeypatch ): _write_session(tmp_path, "proj", "abc", "{}") def _boom(*args, **kwargs): raise KeyError("internal_secret_field_id") monkeypatch.setattr("api._session_handlers.get_cached_session", _boom) resp = export_client.get("/api/export/session/proj/abc") assert resp.status_code == 500 body = resp.get_json() assert body.get("error") == "Failed to parse session" assert "internal_secret_field_id" not in json.dumps(body) _assert_no_class_name_leak(json.dumps(body))🤖 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 `@tests/test_error_propagation.py` around lines 204 - 226, Add a parse-failure coverage test for ExportSessionErrorBody to match the other session error-body suites. In TestExportSessionErrorBody, add a case that patches get_cached_session to raise a parsing-related error (for example via api._session_handlers.get_cached_session) and verifies /api/export/session/proj/abc still returns 500 with the same sanitized error body. Keep the existing exclusion-raise test, and ensure the new test checks that sensitive details and class names do not leak from resolve_loaded_session/export_bp handling.
🤖 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 `@tests/test_error_propagation.py`:
- Around line 204-226: Add a parse-failure coverage test for
ExportSessionErrorBody to match the other session error-body suites. In
TestExportSessionErrorBody, add a case that patches get_cached_session to raise
a parsing-related error (for example via
api._session_handlers.get_cached_session) and verifies
/api/export/session/proj/abc still returns 500 with the same sanitized error
body. Keep the existing exclusion-raise test, and ensure the new test checks
that sensitive details and class names do not leak from
resolve_loaded_session/export_bp handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6d532b1-ae15-4104-ae77-74c670efe76b
📒 Files selected for processing (3)
api/_session_handlers.pytests/test_error_propagation.pyutils/session_errors.py
💤 Files with no reviewable changes (1)
- utils/session_errors.py
🚧 Files skipped from review as they are similar to previous changes (1)
- api/_session_handlers.py
wpak-ai
approved these changes
Jul 9, 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
3 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.
Closes #119
get_session, get_session_stats, and export_session all repeated the same steps: resolve the projects dir, safe_join the JSONL path, check the file exists, load through the cache, run exclusion rules, and map parse failures to structured ErrorCode responses. The exception tuple was also defined twice (_PARSE_ERRORS in api/sessions.py and EXPORT_ERRORS in utils/export_engine.py), so any change to that list had to be kept in sync by hand.
This branch pulls that flow into api/_session_handlers.py (resolve_loaded_session and compute_stats_or_error) and defines SESSION_LOAD_ERRORS once in utils/session_errors.py. export_engine.EXPORT_ERRORS now aliases that tuple. The three route handlers are thin wrappers around the shared helpers. HTTP status codes, error messages, and the issue #25 no-leakage behavior are unchanged.
To sanity-check: run pytest tests/test_error_propagation.py tests/test_api_routes.py tests/test_export_api_bulk.py, then full pytest, mypy -p api -p utils -p models, and ruff check ..
Summary by CodeRabbit