Skip to content

Share query guard helpers#886

Open
xiefuzheng713-alt wants to merge 1 commit into
ramimbo:mainfrom
xiefuzheng713-alt:codex/query-guard-helper-846
Open

Share query guard helpers#886
xiefuzheng713-alt wants to merge 1 commit into
ramimbo:mainfrom
xiefuzheng713-alt:codex/query-guard-helper-846

Conversation

@xiefuzheng713-alt
Copy link
Copy Markdown

@xiefuzheng713-alt xiefuzheng713-alt commented Jun 4, 2026

Refs #846 / Bounty #846.

Summary

  • Adds tiny internal helpers in app/query_validation.py for reading raw query values and rejecting control characters.
  • Reuses those helpers across the control-char, canonical integer, canonical boolean, and repeated-parameter guards.
  • Adds focused unit coverage for shared control-character rejection plus canonical/repeated boundary behavior.

Duplicate check

I searched open PRs for query guard helpers, _query_param_values, _reject_control_char_value, and reject_control_char_query_param cleanup 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 tree ace7e128bee0f82180b3e6265755c0a8d2f5990d.

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

    • Added comprehensive test coverage for query parameter validation, including control character rejection and format enforcement.
  • Refactor

    • Improved query parameter validation code structure for better maintainability and consistency across validators.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4dfc6add-4604-4a8e-b893-1d45b4885b72

📥 Commits

Reviewing files that changed from the base of the PR and between d4d0e48 and 48fde3b.

📒 Files selected for processing (2)
  • app/query_validation.py
  • tests/test_query_validation.py

📝 Walkthrough

Walkthrough

The PR refactors query validation by centralizing repeated request.query_params.getlist calls and control-character rejection logic into shared helpers, then applies these helpers across all four validator functions. Tests verify that control characters are rejected consistently and that canonical format and repetition boundaries are enforced.

Changes

Query validation refactoring

Layer / File(s) Summary
Centralized helpers for query parameter validation
app/query_validation.py
_query_param_values(request, name) and _reject_control_char_value(value, name) reduce duplication by consolidating value retrieval and control-character exception logic. reject_control_char_query_param and reject_noncanonical_int_query_param are refactored to use these helpers.
Apply shared helpers to all validators
app/query_validation.py
reject_noncanonical_bool_query_param and reject_repeated_query_param are updated to call the shared _query_param_values helper instead of directly invoking request.query_params.getlist.
Test control-character rejection and format boundaries
tests/test_query_validation.py
A test helper constructs Request objects with encoded query strings. Parametrized tests assert that control characters are rejected with guard-specific error messages, and separate tests verify canonical integer format, boolean value validity, and parameter repetition are enforced via 400 HTTP responses.

Possibly related PRs

  • ramimbo/mergework#824: Wires the refactored query validation guards into admin routes and adds integration tests asserting their HTTP 400 behavior.
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Bounty Pr Focus ❓ Inconclusive Branch name suggests bounty #846, file changes match claims, but commit message lacks explicit bounty/issue reference in conventional format. Check if actual GitHub PR #886 includes "Bounty #846" or "Refs #846" in description/body.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Share query guard helpers' directly names the changed surface and reflects the core refactoring: extracting and sharing internal helper functions across query validation guards.
Description check ✅ Passed The description is complete with all required template sections: summary, evidence (confusion/bug addressed, file scope, PR size, out-of-scope statement), test evidence checklist, and bounty/issue reference.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed No investment claims, price claims, cash-out claims, fabricated payout claims, or private security details found in PR code, docstrings, or description.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@mauricemohr88-debug mauricemohr88-debug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 existing request.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 as tests/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 tree ace7e128bee0f82180b3e6265755c0a8d2f5990d.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants