Share query guard helpers#886
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors query validation by centralizing repeated ChangesQuery validation refactoring
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
mauricemohr88-debug
left a comment
There was a problem hiding this comment.
Reviewed current head 48fde3bccca6470e853284f4b792dd31ce7e30a8.
Approved. I inspected app/query_validation.py and tests/test_query_validation.py and verified this is a behavior-preserving query-validation refactor:
_query_param_values()centralizes the existingrequest.query_params.getlist(name)access used by all scalar query guards;_reject_control_char_value()preserves the same 400 response and detail text for control-character rejection;- canonical integer and boolean validation still reject control characters before format aliases;
- repeated-parameter rejection still checks the same value list length and preserves the same response detail;
- the new unit tests cover shared control-character rejection plus canonical integer, canonical boolean, and repeated-parameter boundaries;
- PR body includes
Refs #846 / Bounty #846, which satisfies the bounty reference despite CodeRabbit's commit-message-only focus note; - no route behavior, ledger, wallet, treasury, payout, proposal execution, admin-token API, bridge, exchange, cash-out, MRWK price, private-data, credential, or secret-handling behavior is changed.
Validation on this exact head:
uv run --python 3.12 --extra dev python -m pytest tests/test_query_validation.py tests/test_admin_routes.py tests/test_bounty_attempts.py tests/test_api_mCP.py::test_ledger_api_rejects_noncanonical_limit tests/test_api_mcp.py::test_ledger_api_rejects_repeated_limit_before_using_later_value tests/test_api_mcp.py::test_mcp_rejects_noncanonical_integer_string_arguments -q-> 44 passed, 1 existing Starlette/httpx warning. (Command typo corrected in actual shell astests/test_api_mcp.py; result above is from the executed command.)uv run --python 3.12 --extra dev ruff check app/query_validation.py tests/test_query_validation.py tests/test_admin_routes.py tests/test_bounty_attempts.py tests/test_api_mcp.py-> passed.uv run --python 3.12 --extra dev ruff format --check app/query_validation.py tests/test_query_validation.py tests/test_admin_routes.py tests/test_bounty_attempts.py tests/test_api_mcp.py-> 5 files already formatted.uv run --python 3.12 --extra dev mypy app/query_validation.py-> success.uv run --python 3.12 --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD-> clean treeace7e128bee0f82180b3e6265755c0a8d2f5990d.
GitHub state checked before approval: mergeStateStatus=CLEAN; hosted Quality, readiness, docs, and image checks and CodeRabbit statuses are successful on this head. I found no prior human review and no existing #838 claim for PR #886 / this head before posting this review.
Refs #846 / Bounty #846.
Summary
app/query_validation.pyfor reading raw query values and rejecting control characters.Duplicate check
I searched open PRs for
query guard helpers,_query_param_values,_reject_control_char_value, andreject_control_char_query_paramcleanup before opening this. I did not find an open PR covering this helper extraction. This is distinct from the open padded-filter hardening PRs and from bounty URL/availability cleanup PRs.Validation
uv run --extra dev python -m pytest tests/test_query_validation.py tests/test_admin_routes.py tests/test_bounty_attempts.py tests/test_api_mcp.py::test_ledger_api_rejects_noncanonical_limit tests/test_api_mcp.py::test_ledger_api_rejects_repeated_limit_before_using_later_value tests/test_api_mcp.py::test_mcp_rejects_noncanonical_integer_string_arguments -q-> 44 passed, 1 existing Starlette/httpx warning.uv run --extra dev ruff check app/query_validation.py tests/test_query_validation.py-> passed.uv run --extra dev ruff format --check app/query_validation.py tests/test_query_validation.py-> 2 files already formatted.uv run --extra dev mypy app/query_validation.py-> success.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD-> clean treeace7e128bee0f82180b3e6265755c0a8d2f5990d.Scope
Focused maintainability cleanup only. It does not change route behavior, ledger, wallet, treasury, payout, proposal execution, bridge, exchange, cash-out, MRWK price, private data, credentials, or secrets.
Summary by CodeRabbit
Tests
Refactor