Add search limit validation, typed models, and mypy strict CI#38
Add search limit validation, typed models, and mypy strict CI#38clean6378-max-it wants to merge 6 commits into
Conversation
- Return 400 with a clear JSON error when /api/search limit is invalid instead of crashing with ValueError on bad input (abc, 1.5, etc.) - Add models/ TypedDict shapes and annotate parser/API boundaries - Enable mypy --strict on api, utils, and models; add CI mypy job - Add tests/test_search.py for limit validation cases
|
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 TypedDict models, a shared Flask response helper, validates search ChangesType Safety and Search Validation Overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/export_api.py (1)
279-306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard single-session export parse/render errors with a stable JSON 500 response.
export_sessioncurrently letsparse_session(...),compute_stats(...), and render steps raise uncaught exceptions. This can break API response consistency on failures.Proposed fix
fmt = request.args.get("format", "md") - session = parse_session(filepath) - rules = current_app.config.get("EXCLUSION_RULES") or [] - if is_session_excluded(rules, session, project_name): - return json_ok({"error": "Session not found"}), 404 - stats = compute_stats(session) - title_slug = slugify(session["title"], default="session") + try: + session = parse_session(filepath) + rules = current_app.config.get("EXCLUSION_RULES") or [] + if is_session_excluded(rules, session, project_name): + return json_ok({"error": "Session not found"}), 404 + stats = compute_stats(session) + title_slug = slugify(session["title"], default="session") + except Exception: + current_app.logger.exception("Failed to export session %s", session_id) + return json_ok({"error": "Failed to export session"}), 500🤖 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 `@api/export_api.py` around lines 279 - 306, The export_session route currently calls parse_session, compute_stats, session_to_json and session_to_markdown without error handling; wrap the parse/render/stat computation block in a try/except that catches Exception, logs the exception (e.g., current_app.logger.exception) and returns a stable JSON 500 response (body like {"error": "Internal server error exporting session"}) instead of letting exceptions bubble; ensure you guard the calls to parse_session, compute_stats, session_to_json/session_to_markdown and slugify/ send_file decision logic so any failure returns that JSON 500 (optionally include error details only when app.debug is true).
🧹 Nitpick comments (2)
pyproject.toml (1)
5-9: 💤 Low valueConsider removing redundant test exclusion override.
The
exclude = ["tests/"]at line 5 already prevents mypy from analyzing the tests directory. The override block at lines 7–9 that ignores errors fortests.*modules is redundant unless checked packages explicitly import from tests (which would be unusual).♻️ Simplify configuration by removing override
[tool.mypy] python_version = "3.12" strict = true packages = ["api", "utils", "models"] exclude = ["tests/"] - -[[tool.mypy.overrides]] -module = "tests.*" -ignore_errors = true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 5 - 9, Remove the redundant mypy override that ignores errors for tests by deleting the [[tool.mypy.overrides]] block that sets module = "tests.*" and ignore_errors = true, since exclude = ["tests/"] already prevents analysis of the tests directory; ensure only the exclude setting remains to keep configuration simple and avoid masking issues.utils/session_path.py (1)
56-63: 💤 Low valueConsider removing the redundant fallback check.
After updating
_get_display_nameto acceptnameas the fallback parameter (line 58), the check at lines 62-63 is now redundant. When extraction fails,_get_display_namereturnsnamedirectly (viafallback or ""), soif not display_name:will never be true (assumingnameis non-empty).The current code works correctly, but you could remove lines 62-63 to eliminate the redundant check.
🤖 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 `@utils/session_path.py` around lines 56 - 63, Remove the redundant fallback assignment: since _get_display_name(os.path.join(project_dir, jf), name) already returns name when extraction fails, drop the post-loop check that reassigns display_name = name; specifically update the loop in which display_name is set (references: jsonl_files, project_dir, name, and _get_display_name) to rely on the function's fallback and remove the subsequent if not display_name block.
🤖 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 @.github/workflows/ci.yml:
- Around line 69-70: Update the pip cache dependency path so it includes both
requirements-dev.txt and requirements.txt (the same pattern used in the pytest
job) to ensure cache invalidation when either file changes; locate the cache
block that sets "cache: pip" and "cache-dependency-path" in the CI workflow and
change the dependency path value to list both files (requirements-dev.txt and
requirements.txt).
In `@api/sessions.py`:
- Around line 55-58: The stats endpoint currently parses the session and returns
compute_stats without applying exclusion rules; modify the get_session_stats
handler to call is_session_excluded(...) (the same check used by get_session)
after obtaining the session (or reuse get_session to centralize logic), and if
the session is excluded return the same error/response (e.g., 404 or json_error)
as get_session instead of json_ok(stats); ensure you reference parse_session,
compute_stats and is_session_excluded so the exclusion check is applied before
returning stats.
In `@requirements-dev.txt`:
- Around line 3-4: Remove the obsolete types-Flask entry from
requirements-dev.txt (the package name "types-Flask" from the diff) because
Flask 3.1.3 provides native type stubs, and bump the mypy version string from
"mypy==1.15.0" to the current stable release (e.g., "mypy==2.1.0") in
requirements-dev.txt so the dev requirements use the up-to-date type checker.
---
Outside diff comments:
In `@api/export_api.py`:
- Around line 279-306: The export_session route currently calls parse_session,
compute_stats, session_to_json and session_to_markdown without error handling;
wrap the parse/render/stat computation block in a try/except that catches
Exception, logs the exception (e.g., current_app.logger.exception) and returns a
stable JSON 500 response (body like {"error": "Internal server error exporting
session"}) instead of letting exceptions bubble; ensure you guard the calls to
parse_session, compute_stats, session_to_json/session_to_markdown and slugify/
send_file decision logic so any failure returns that JSON 500 (optionally
include error details only when app.debug is true).
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 5-9: Remove the redundant mypy override that ignores errors for
tests by deleting the [[tool.mypy.overrides]] block that sets module = "tests.*"
and ignore_errors = true, since exclude = ["tests/"] already prevents analysis
of the tests directory; ensure only the exclude setting remains to keep
configuration simple and avoid masking issues.
In `@utils/session_path.py`:
- Around line 56-63: Remove the redundant fallback assignment: since
_get_display_name(os.path.join(project_dir, jf), name) already returns name when
extraction fails, drop the post-loop check that reassigns display_name = name;
specifically update the loop in which display_name is set (references:
jsonl_files, project_dir, name, and _get_display_name) to rely on the function's
fallback and remove the subsequent if not display_name block.
🪄 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: 371cc1f8-ed5c-4963-b8bf-2da316d77719
📒 Files selected for processing (24)
.github/workflows/ci.ymlapi/_flask_types.pyapi/export_api.pyapi/projects.pyapi/search.pyapi/sessions.pymodels/__init__.pymodels/errors.pymodels/export.pymodels/project.pymodels/search.pymodels/session.pymodels/stats.pypyproject.tomlrequirements-dev.txttests/test_search.pyutils/exclusion_rules.pyutils/export_day_filter.pyutils/export_state_store.pyutils/json_exporter.pyutils/jsonl_parser.pyutils/md_exporter.pyutils/session_path.pyutils/session_stats.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/session_path.py (1)
92-93: ⚡ Quick winConsider narrowing exception handling or adding logging.
The broad
except Exception: passsilently swallows all errors, including unexpected ones, making debugging harder. Since this is a defensive helper that reads JSONL files, consider catching specific exceptions (OSError,json.JSONDecodeError,UnicodeDecodeError) or adding minimal logging for visibility.♻️ Proposed refinement
- except Exception: - pass + except (OSError, json.JSONDecodeError, UnicodeDecodeError): + # Failed to extract display name; caller will use fallback + pass🤖 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 `@utils/session_path.py` around lines 92 - 93, The broad "except Exception: pass" in utils/session_path.py silently swallows file/read/parse errors; replace it by catching specific exceptions (e.g., OSError, json.JSONDecodeError, UnicodeDecodeError) around the JSONL read/parse logic (the block containing the except) and log the error instead of passing—use the module or passed-in logger (e.g., logger.error or logger.exception) with a concise message including the filename and error to aid debugging while still allowing other unexpected exceptions to surface.
🤖 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 `@utils/session_path.py`:
- Around line 55-62: The loop that determines display_name now breaks on the
first JSONL file because _get_display_name is being called with name as the
fallback, making failures return a truthy value; revert to passing None (or
otherwise detect a true extraction) so failures return None and the loop
continues: change the call to _get_display_name(os.path.join(project_dir, jf),
None) (or keep name but only break when candidate is not None and candidate !=
name) so display_name is only set/breaks when a real extracted candidate is
returned; this fixes the unintended early exit from the jsonl_files loop
(symbols: display_name, jsonl_files, _get_display_name, project_dir, name).
---
Nitpick comments:
In `@utils/session_path.py`:
- Around line 92-93: The broad "except Exception: pass" in utils/session_path.py
silently swallows file/read/parse errors; replace it by catching specific
exceptions (e.g., OSError, json.JSONDecodeError, UnicodeDecodeError) around the
JSONL read/parse logic (the block containing the except) and log the error
instead of passing—use the module or passed-in logger (e.g., logger.error or
logger.exception) with a concise message including the filename and error to aid
debugging while still allowing other unexpected exceptions to surface.
🪄 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: 9442bfc9-cf96-4ea4-a3c9-25f07f5d9b11
📒 Files selected for processing (6)
.github/workflows/ci.ymlapi/export_api.pyapi/sessions.pypyproject.tomlrequirements-dev.txtutils/session_path.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- api/export_api.py
- api/sessions.py
When I click every conversation, it is showing like strange.
|
|
in light mode, when I reload project, it is showing dark and then showing light again |
|
Why hamburger button is needed? Is this clickable? |
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 `@static/js/search.js`:
- Around line 31-35: The click handler currently assumes e.target is an Element
and proceeds even if data-session-id is missing; update the guard so you first
check that e.target is an instanceof Element before calling closest, then
require both attributes (e.g., result.getAttribute('data-project') and
result.getAttribute('data-session-id')) to be non-null/ non-empty before
updating window.location.hash (use encodeURIComponent as before); reference the
variables result, project, sessionId and the window.location.hash assignment
when locating where to add these checks.
In `@static/js/sessions.js`:
- Around line 102-104: The click handler attached to sidebar calls
e.target.closest without ensuring e.target is an Element, which can throw for
non-Element event targets; update the sidebar.addEventListener callback to first
guard that e.target is an Element (e.g., check instanceof Element or that
closest exists) and return early if not, then call
e.target.closest('button.sidebar-item[data-session-id]') to assign btn and
continue as before.
🪄 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: a5c44ad7-86d0-43f8-8214-db7df0681ca8
📒 Files selected for processing (2)
static/js/search.jsstatic/js/sessions.js
| @@ -1,2 +1,3 @@ | |||
| -r requirements.txt | |||
| pytest==9.0.2 | |||
| mypy==2.1.0 | |||
There was a problem hiding this comment.
PR description says mypy==1.15.0 and types-Flask==1.1.6. The diff actually adds mypy==2.1.0 and no types-Flask. Either the description is stale or the diff doesn't match what was tested locally. Mypy 2.x is a major version with notable behavioural differences from 1.x — worth confirming which version was used for "mypy clean on 23 source files" in the test plan and aligning the description/pin.
| result.append(cast(ProjectSessionRowDict, { | ||
| **s, | ||
| "title": parsed["title"], | ||
| "models": meta["models_used"], | ||
| "models": sorted(models) if isinstance(models, set) else list(models), | ||
| "tokens": meta["total_input_tokens"] + meta["total_output_tokens"], | ||
| "tool_calls": meta["total_tool_calls"], | ||
| "first_timestamp": meta["first_timestamp"], | ||
| "last_timestamp": meta["last_timestamp"], | ||
| }) | ||
| })) |
There was a problem hiding this comment.
result.append(cast(ProjectSessionRowDict, {
**s,
"title": parsed["title"],
...
}))
cast is a runtime no-op that silences mypy without checking. The whole point of TypedDict is compile-time checking; the cast removes it.
| filepath = safe_join(base, project_name, f"{session_id}.jsonl") | ||
| except ValueError: | ||
| return jsonify({"error": "Invalid path"}), 400 | ||
| return json_ok({"error": "Invalid path"}), 400 |
There was a problem hiding this comment.
return json_ok({"error": "Invalid path"}), 400
return json_ok({"error": f"Session {session_id} not found"}), 404
return json_ok({"error": "Session not found"}), 404
return json_ok({"error": "Failed to parse session"}), 500
json_ok is used for both success and error responses. The name says "success", the call sites add a non-2xx status tuple. Rename to json_response() / as_json(), or add a json_error(message, status) sibling that returns the (Response, int) pair in one call. As-is, every error site has to remember the trailing , 404 — confusing.
|
|
||
| if not os.path.isfile(filepath): | ||
| return jsonify({"error": f"Session {session_id} not found"}), 404 | ||
| return json_ok({"error": f"Session {session_id} not found"}), 404 |
| if is_session_excluded(rules, session, project_name): | ||
| return jsonify({"error": "Session not found"}), 404 | ||
| return jsonify(session) | ||
| return json_ok({"error": "Session not found"}), 404 |
| python_version = "3.12" | ||
| strict = true | ||
| packages = ["api", "utils", "models"] | ||
| exclude = ["tests/"] |
There was a problem hiding this comment.
Mypy strict skips the whole tests/ tree. Test code can drift (untyped helpers, broken exclusion regexes) without the gate catching it. Either include tests/ with a [[tool.mypy.overrides]] block for strict = false, or at minimum run a non-strict pass over tests/.
|
|
||
| mypy: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.12" | ||
| cache: pip | ||
| cache-dependency-path: | | ||
| requirements.txt | ||
| requirements-dev.txt | ||
| - name: Install dev dependencies | ||
| run: pip install -r requirements-dev.txt | ||
|
|
||
| - name: Run mypy (strict) |
There was a problem hiding this comment.
Job added but the diff doesn't show whether it's listed as a required status check on the branch protection. A failing mypy job in CI doesn't block merge unless the gate is enforced — verify with whoever owns the repo settings.
|
|
||
| from flask import Response, jsonify | ||
|
|
||
| FlaskReturn = Union[Response, tuple[Response, int]] |
There was a problem hiding this comment.
Misses Flask's other valid return shapes: tuple[Response, int, dict[str, str]] (with headers), tuple[str, int] (plain text), etc. For now the codebase only emits the two listed shapes — fine — but add a one-line comment noting the narrow scope so the next contributor knows to widen if they add a header response.
| def _parse_limit(raw: str | None, default: int = _DEFAULT_LIMIT) -> int: | ||
| """Parse a positive integer limit from a query string value.""" | ||
| if raw is None or raw.strip() == "": | ||
| return default | ||
| try: | ||
| value = int(raw) | ||
| except ValueError: | ||
| raise ValueError("Invalid limit: must be a positive integer") from None | ||
| if value < 1: | ||
| raise ValueError("Invalid limit: must be a positive integer") | ||
| return value |
There was a problem hiding this comment.
def _parse_limit(raw: str | None, default: int = _DEFAULT_LIMIT) -> int:
if raw is None or raw.strip() == "":
return default
try:
value = int(raw)
except ValueError:
raise ValueError("Invalid limit: must be a positive integer") from None
if value < 1:
raise ValueError("Invalid limit: must be a positive integer")
return value
| resp = client.get("/api/search?q=searchable&limit=10") | ||
| assert resp.status_code == 200 | ||
| data = resp.get_json() | ||
| assert isinstance(data, list) | ||
| assert len(data) >= 1 | ||
|
|
||
| def test_limit_float_string_returns_400(self, client): | ||
| resp = client.get("/api/search?q=test&limit=1.5") | ||
| assert resp.status_code == 400 | ||
| body = resp.get_json() | ||
| assert "error" in body | ||
| assert "limit" in body["error"].lower() | ||
|
|
||
| def test_limit_non_numeric_returns_400(self, client): | ||
| resp = client.get("/api/search?q=test&limit=abc") | ||
| assert resp.status_code == 400 | ||
| body = resp.get_json() | ||
| assert "error" in body | ||
| assert "limit" in body["error"].lower() | ||
|
|
||
| def test_limit_default_when_omitted(self, client, tmp_path): | ||
| _write_searchable_session(tmp_path, "proj-a", "sess-1", "findme keyword here") | ||
| resp = client.get("/api/search?q=findme") | ||
| assert resp.status_code == 200 | ||
| assert isinstance(resp.get_json(), list) | ||
|
|
||
| def test_empty_query_returns_empty_list(self, client): |
There was a problem hiding this comment.
Cover integer / float / non-numeric / default / empty query. Missing two edge cases worth adding: limit=0 → 400 (per value < 1 at api/search.py:26) and limit=-5 → 400. Both hit the same branch as limit=abc but they're the corner cases someone empirically passes through a UI bug.
2 participants
Closes #37
Add search limit validation, typed models, and mypy strict CI
Summary
Week 3 Monday type-safety foundations (9 pt) for
claude-code-chat-browser:/api/searchlimitand return 400 for bad input (no more 500 crashes).models/withTypedDictshapes at public API/parser boundaries.mypy --strictonapi,utils, andmodelsplus a CI job.No intentional JSON or UI behavior changes.
Problem
limit—int(request.args.get("limit", 50))raisedValueErrorforabc,1.5, etc., surfacing HTTP 500.dicts with no shared types.Changes
Block A — Search limit validation (1 pt)
api/search.py:_parse_limit()validateslimit.{"error": "Invalid limit: must be a positive integer"}.limit→ default 50.tests/test_search.py: 5 tests (integer string, float, non-numeric, default, empty query).Block B — Typed models (5 pt)
New
models/package:session.pySessionDict,MessageDict,SessionMetadataDict,QuickSessionInfoDictproject.pyProjectDict,SessionListItemDict,ProjectSessionRowDictsearch.pySearchHitDictstats.pySessionStatsDict,FilesTouchedDicterrors.pyErrorResponseexport.pyExportStateDictAnnotated boundaries in
utils/jsonl_parser.py,utils/session_path.py,utils/session_stats.py,api/*, exporters, and exclusion helpers.Block C — Mypy strict + CI (3 pt)
pyproject.toml—strict = trueforapi,utils,modelsrequirements-dev.txt—mypy==1.15.0,types-Flask==1.1.6.github/workflows/ci.yml— newmypyjobapi/_flask_types.py—FlaskReturn,json_ok()Out of scope
ErrorCodeon error responsesTest plan
pytest tests/test_search.py -v— 5 passedpytest -q— 234 passedmypy— clean on 23 source filescurl.exe "http://127.0.0.1:5000/api/search?q=test&limit=abc"→ 400 JSONcurl.exe "http://127.0.0.1:5000/api/search?q=test&limit=5"→ 200/#searchunchangedNotes for reviewers
scripts/export.pyis not in the mypy package list; sharedutils/+models/are typed at boundaries the CLI uses.{"error": "..."}only;codefields come in a follow-up PR.api/search.pymanually.Related
Summary by CodeRabbit
Tests
Bug Fixes
Chores
UI
Exports