refactor: extract path parameter validators#375
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughA new ChangesPath Validation Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR centralizes validation/parsing logic for several API path/query parameters into a dedicated module and adds unit tests for the new utilities.
Changes:
- Added
app/path_params.pywith shared helpers for bounded SQLite ints, positive IDs/sequences, and proof hash normalization. - Updated
app/main.pyto use the shared path param helpers instead of local/private duplicates. - Added
tests/test_path_params.pyto cover the new path/query parsing behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_path_params.py | Adds unit tests for issue number parsing, positive ID/sequence validation, and proof-hash normalization. |
| app/path_params.py | Introduces shared validation/parsing helpers (SQLite int bounds, proof hash, positive IDs). |
| app/main.py | Switches endpoints/CLI code paths to use the shared helpers and removes duplicated private implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except (TypeError, ValueError) as exc: | ||
| raise HTTPException(status_code=400, detail=f"{field} must be an integer") from exc | ||
| if parsed < -SQLITE_INTEGER_MAX - 1 or parsed > SQLITE_INTEGER_MAX: | ||
| raise HTTPException(status_code=400, detail=f"{field} is too large") |
| from fastapi import HTTPException | ||
|
|
||
| from app.path_params import ( | ||
| SQLITE_INTEGER_MAX, | ||
| issue_number_search_value, | ||
| positive_bounty_id, | ||
| positive_ledger_sequence, | ||
| proof_hash_from_path, | ||
| ) |
| try: | ||
| parsed = int(value) | ||
| except (TypeError, ValueError) as exc: | ||
| raise HTTPException(status_code=400, detail=f"{field} must be an integer") from exc |
CharlieLZ
left a comment
CharlieLZ
left a comment
There was a problem hiding this comment.
Thanks for the focused extraction. I found one behavior-preservation issue to fix before this lands.
app/path_params.py introduces parse_sqlite_int(), but the PR does not use it anywhere (rg parse_sqlite_int only finds its definition) and it does not match the existing integer-parser semantics in app.main: parse_sqlite_int(True, field="bounty_id") returns 1, while the existing _parse_int() / MCP integer parser explicitly reject booleans as invalid integers. Since this PR is a boundary-preserving extraction, adding an unused public helper with looser validation creates a future footgun and is not covered by the new tests.
Please either remove parse_sqlite_int() from this extraction, or make it reject booleans and add regression coverage before introducing it as a shared helper.
Validation I ran:
uv run --extra dev python -m pytest tests/test_path_params.py tests/test_bounty_pages.py tests/test_api_mcp.py -q-> 87 passeduv run --extra dev ruff check app/main.py app/path_params.py tests/test_path_params.py-> passeduv run --extra dev ruff format --check app/main.py app/path_params.py tests/test_path_params.py-> passeduv run --extra dev python -m mypy app/main.py app/path_params.py-> passedgit diff --check origin/main...HEAD-> clean
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_path_params.py (1)
21-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd tests for
parse_sqlite_intchanged behavior.The new validator module introduces
parse_sqlite_int, but this suite does not cover its success/bounds/error cases. Please add tests for valid ints, overflow, and non-integer inputs.As per coding guidelines,
tests/**/*.py: Add or update tests for changed behavior.🤖 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_path_params.py` around lines 21 - 52, Add unit tests covering the new parse_sqlite_int behavior: create tests that assert parse_sqlite_int("340") returns 340 and parse_sqlite_int(str(SQLITE_INTEGER_MAX)) returns SQLITE_INTEGER_MAX, assert parse_sqlite_int("")/parse_sqlite_int(" 340")/parse_sqlite_int("340a")/parse_sqlite_int(str(SQLITE_INTEGER_MAX + 1)) return None (or use assert_bad_request where appropriate), and also add explicit tests for positive_bounty_id/positive_ledger_sequence to ensure they still enforce bounds; reference the function name parse_sqlite_int and the constant SQLITE_INTEGER_MAX when adding the test cases so they mirror existing patterns in test_issue_number_search_value_rejects_non_numeric_or_overflow_query and test_positive_bounty_id_and_ledger_sequence_validate_bounds.
🤖 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 `@app/path_params.py`:
- Around line 48-55: parse_sqlite_int currently calls int(value) which accepts
booleans and truncates floats; update parse_sqlite_int to explicitly reject bool
and float inputs and only accept either an actual int (but not bool) or a
numeric string that matches an integer pattern (e.g. ^-?\d+$), then convert the
validated string with int(...) and keep the existing bounds check against
SQLITE_INTEGER_MAX; reference parse_sqlite_int and SQLITE_INTEGER_MAX when
making the change.
---
Outside diff comments:
In `@tests/test_path_params.py`:
- Around line 21-52: Add unit tests covering the new parse_sqlite_int behavior:
create tests that assert parse_sqlite_int("340") returns 340 and
parse_sqlite_int(str(SQLITE_INTEGER_MAX)) returns SQLITE_INTEGER_MAX, assert
parse_sqlite_int("")/parse_sqlite_int("
340")/parse_sqlite_int("340a")/parse_sqlite_int(str(SQLITE_INTEGER_MAX + 1))
return None (or use assert_bad_request where appropriate), and also add explicit
tests for positive_bounty_id/positive_ledger_sequence to ensure they still
enforce bounds; reference the function name parse_sqlite_int and the constant
SQLITE_INTEGER_MAX when adding the test cases so they mirror existing patterns
in test_issue_number_search_value_rejects_non_numeric_or_overflow_query and
test_positive_bounty_id_and_ledger_sequence_validate_bounds.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4055f8e6-7264-4c2a-894e-acdf8db14faa
📒 Files selected for processing (3)
app/main.pyapp/path_params.pytests/test_path_params.py
| def parse_sqlite_int(value: Any, *, field: str) -> int: | ||
| try: | ||
| parsed = int(value) | ||
| except (TypeError, ValueError) as exc: | ||
| raise HTTPException(status_code=400, detail=f"{field} must be an integer") from exc | ||
| if parsed < -SQLITE_INTEGER_MAX - 1 or parsed > SQLITE_INTEGER_MAX: | ||
| raise HTTPException(status_code=400, detail=f"{field} is too large") | ||
| return parsed |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Python coercion behavior and repository usage of parse_sqlite_int (read-only).
python - <<'PY'
print("int(True) ->", int(True))
print("int(False) ->", int(False))
print("int(1.9) ->", int(1.9))
PY
rg -n -C2 '\bparse_sqlite_int\s*\('Repository: ramimbo/mergework
Length of output: 311
Reject bool/float coercion in parse_sqlite_int (Line 50).
int(value) coerces True/False to 1/0 and truncates floats (e.g., 1.9 → 1), so non-integer inputs can pass parsing and only be validated by the SQLite bounds check.
💡 Proposed fix
def parse_sqlite_int(value: Any, *, field: str) -> int:
- try:
- parsed = int(value)
- except (TypeError, ValueError) as exc:
- raise HTTPException(status_code=400, detail=f"{field} must be an integer") from exc
+ if isinstance(value, bool):
+ raise HTTPException(status_code=400, detail=f"{field} must be an integer")
+ if isinstance(value, int):
+ parsed = value
+ elif isinstance(value, str):
+ clean = value.strip()
+ if not clean or not clean.lstrip("+-").isdigit():
+ raise HTTPException(status_code=400, detail=f"{field} must be an integer")
+ try:
+ parsed = int(clean)
+ except ValueError as exc:
+ raise HTTPException(status_code=400, detail=f"{field} must be an integer") from exc
+ else:
+ raise HTTPException(status_code=400, detail=f"{field} must be an integer")
if parsed < -SQLITE_INTEGER_MAX - 1 or parsed > SQLITE_INTEGER_MAX:
raise HTTPException(status_code=400, detail=f"{field} is too large")
return parsed🤖 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 `@app/path_params.py` around lines 48 - 55, parse_sqlite_int currently calls
int(value) which accepts booleans and truncates floats; update parse_sqlite_int
to explicitly reject bool and float inputs and only accept either an actual int
(but not bool) or a numeric string that matches an integer pattern (e.g.
^-?\d+$), then convert the validated string with int(...) and keep the existing
bounds check against SQLITE_INTEGER_MAX; reference parse_sqlite_int and
SQLITE_INTEGER_MAX when making the change.
|
Addressed the behavior-preservation review in b1cdf7b: removed the unused |
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
Thanks for the focused extraction. I found one behavior-preservation problem before this helper becomes shared.
parse_sqlite_int() currently accepts values the existing request/MCP integer parsers reject. In a local checkout of this PR, this probe:
from app.path_params import parse_sqlite_int
for value in [True, False, 1.9, b"7"]:
print(repr(value), "->", parse_sqlite_int(value, field="bounty_id"))returns:
True -> 1
False -> 0
1.9 -> 1
b'7' -> 7
The current integer parsing logic rejects booleans explicitly and only accepts actual ints or signed numeric strings. Since this PR is presented as a behavior-preserving path-parameter extraction, introducing an unused helper with looser coercion creates a future route/MCP footgun. Please either remove parse_sqlite_int() from this extraction, or align it with the existing strict integer semantics and add regression coverage for bool/float/bytes inputs.
Validation I ran on current head 75ae35e:
uv run --extra dev python -m pytest tests/test_path_params.py tests/test_bounty_pages.py tests/test_api_mcp.py -q-> 87 passeduv run --extra dev python -m ruff check app/main.py app/path_params.py tests/test_path_params.py-> passeduv run --extra dev python -m ruff format --check app/main.py app/path_params.py tests/test_path_params.py-> 3 files already formatted
CharlieLZ
left a comment
CharlieLZ
left a comment
There was a problem hiding this comment.
Follow-up on b1cdf7b: the behavior-preservation issue I requested changes for is resolved. The unused parse_sqlite_int() helper has been removed, so the extraction no longer introduces a looser boolean/int parsing path than the existing validators.\n\nValidation on current head b1cdf7b:\n- uv run --extra dev python -m pytest tests/test_path_params.py tests/test_bounty_pages.py tests/test_api_mcp.py -q -> 87 passed\n- uv run --extra dev ruff check app/main.py app/path_params.py tests/test_path_params.py -> passed\n- uv run --extra dev ruff format --check app/main.py app/path_params.py tests/test_path_params.py -> passed\n- uv run --extra dev python -m mypy app/main.py app/path_params.py -> success\n- git diff --check origin/main...HEAD -> clean
Refs #320
Bounty #320
Summary
app.mainintoapp.path_params.Test Plan
./.venv/bin/python -m pytest -q tests/test_path_params.py tests/test_bounty_pages.py tests/test_api_mcp.py./.venv/bin/python -m ruff check app/main.py app/path_params.py tests/test_path_params.py./.venv/bin/python -m ruff format --check app/main.py app/path_params.py tests/test_path_params.pySummary by CodeRabbit
Refactor
Tests