Skip to content

Limit activity query length#858

Open
xiefuzheng713-alt wants to merge 2 commits into
ramimbo:mainfrom
xiefuzheng713-alt:codex/activity-q-length-limit
Open

Limit activity query length#858
xiefuzheng713-alt wants to merge 2 commits into
ramimbo:mainfrom
xiefuzheng713-alt:codex/activity-q-length-limit

Conversation

@xiefuzheng713-alt
Copy link
Copy Markdown
Contributor

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

Summary:

  • caps the public activity q filter at 500 characters for both /api/v1/activity and /activity;
  • keeps the existing control-character and repeated-parameter behavior intact;
  • adds regression coverage for oversized activity searches on the JSON API and HTML page.

Bounty #799

Source report: #798 (comment)

Production evidence before fix:

  • GET https://api.mrwk.online/api/v1/activity?q=<10000 a characters> -> HTTP 200, response size 10188 bytes.
  • The request is accepted and processed instead of being rejected with a bounded validation error.

Duplicate/current check:

Scope:

  • This PR intentionally stays on public activity search query validation.
  • It does not change activity result semantics for valid queries, account normalization, MCP behavior, ledger mutation, bounty creation, payout execution, treasury mutation, wallet behavior, private data, secrets, bridge/exchange/cash-out behavior, or MRWK price behavior.

Validation:

  • uv run --extra dev pytest tests/test_activity.py -q -> 7 passed, 1 warning.
  • uv run --extra dev pytest tests/test_activity.py tests/test_activity_routes.py -q -> 8 passed, 1 warning.
  • uv run --extra dev ruff check app/activity.py tests/test_activity.py tests/test_activity_routes.py -> passed.
  • uv run --extra dev ruff format --check app/activity.py tests/test_activity.py tests/test_activity_routes.py -> 3 files already formatted.
  • uv run --extra dev mypy app/activity.py -> success.
  • uv run --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 e6fc1d0efb5d75ca010137dca3d2ae3817ad3382.

Summary by CodeRabbit

  • Bug Fixes

    • Activity query input now rejects control characters and enforces a 500-character maximum, returning clear 400 errors for invalid input.
  • Tests

    • Validation tests updated to confirm valid 500-character queries succeed and oversized or control-character queries are rejected with appropriate error messages.

@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: 3baa4fe0-fd33-4f77-b9c0-37d34e057f60

📥 Commits

Reviewing files that changed from the base of the PR and between b11cc9e and cfbad48.

📒 Files selected for processing (1)
  • tests/test_activity.py

📝 Walkthrough

Walkthrough

A 500-character maximum length limit is added to the q query parameter in the activity endpoints. Requests with q values exceeding 500 characters now return HTTP 400 with a specific validation error message. Existing control-character validation remains in place.

Changes

Activity query length validation

Layer / File(s) Summary
Query length validation
app/activity.py
Module-level ACTIVITY_QUERY_MAX_LENGTH = 500 added; activity_context now rejects q parameters longer than 500 characters with HTTP 400 and error detail q must be at most 500 characters, keeping the control-character check.
Validation test coverage
tests/test_activity.py
test_activity_query_rejects_control_characters extended to assert that a 500-character q succeeds (HTTP 200) and a 501-character q is rejected (HTTP 400) on both /api/v1/activity and /activity, with the exact message q must be at most 500 characters.

Possibly related PRs

  • ramimbo/mergework#753: Modifies activity_context to reject control characters in q; this PR adds an additional length constraint.
  • ramimbo/mergework#733: Also changes how the activity q query is handled; related to activity filtering/validation.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Limit activity query length' clearly and directly names the changed surface—the activity query parameter length constraint introduced in the changeset.
Description check ✅ Passed The description covers all required template sections: summary, evidence (including confusion addressed, duplicate check, scope, and validation evidence), and test evidence with full CI validation results.
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 README correctly describes MRWK as native coin supporting future snapshots/bridges. Code changes contain no problematic claims or security details.
Bounty Pr Focus ✅ Passed PR scope matches bounty: only app/activity.py and tests/test_activity.py modified; validates query length with tests at boundary (500) and overage (501); no unrelated file changes.

✏️ 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

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d5a75f9d-918d-443f-b2bd-6c5f29f7d2bd

📥 Commits

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

📒 Files selected for processing (2)
  • app/activity.py
  • tests/test_activity.py

Comment thread tests/test_activity.py
Copy link
Copy Markdown

@NiXouuuu NiXouuuu left a comment

Choose a reason for hiding this comment

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

Approved current head b11cc9e6af4bd301ce8efd5020bb226afb23747e after validating the focused public activity query length cap.

Source report checked: #798 (comment)

Production evidence before fix:

  • GET https://api.mrwk.online/api/v1/activity?q=<10000 a characters> -> HTTP 200, response size 10188 bytes.

Files inspected:

  • app/activity.py
  • tests/test_activity.py

Review notes:

  • ACTIVITY_QUERY_MAX_LENGTH = 500 is enforced inside activity_context(), which is shared by /api/v1/activity and /activity.
  • Existing control-character and repeated-query-parameter validation remains unchanged.
  • The new regression coverage checks oversized q rejection for both the JSON API and HTML page.

Validation on current head:

  • uv run --extra dev pytest tests/test_activity.py tests/test_activity_routes.py -q -> 8 passed, 1 existing Starlette/httpx warning.
  • uv run --extra dev ruff check app/activity.py tests/test_activity.py tests/test_activity_routes.py -> passed.
  • uv run --extra dev ruff format --check app/activity.py tests/test_activity.py tests/test_activity_routes.py -> 3 files already formatted.
  • uv run --extra dev mypy app/activity.py -> success.
  • uv run --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 e6fc1d0efb5d75ca010137dca3d2ae3817ad3382.

GitHub state before review: mergeable=MERGEABLE, hosted Quality, readiness, docs, and image checks SUCCESS, CodeRabbit PENDING, and no current-head human reviews existed. scripts/review_bounty_candidates.py --repo ramimbo/mergework --reviewer NiXouuuu --format text reports PR #858 as candidate_for_fresh_review.

Scope: public activity query validation only. No admin-token APIs, labels/comments from app code, payout execution, treasury mutation, ledger mutation, wallet material, private data, secrets, bridge, exchange, cash-out, or MRWK price behavior was used or changed.

Copy link
Copy Markdown

@laughlife laughlife left a comment

Choose a reason for hiding this comment

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

Approved current head cfbad488b30da1c469f306bc2409a2f43fc53412 after validating the follow-up boundary coverage for the public activity query length cap.

Source report checked: #798 (comment)

Production evidence before fix:

  • GET https://api.mrwk.online/api/v1/activity?q=<10000 a characters> -> HTTP 200, response size 10188 bytes.

Files inspected:

  • app/activity.py
  • tests/test_activity.py

Review notes:

  • ACTIVITY_QUERY_MAX_LENGTH = 500 is enforced in shared activity_context(), so the cap applies to both /api/v1/activity and /activity.
  • Existing control-character and repeated-query-parameter validation remains unchanged.
  • The latest commit addresses the stale CodeRabbit boundary concern by proving exactly 500 characters is still accepted and 501 characters is rejected.
  • The change stays scoped to public activity query validation and does not alter account normalization, result serialization, MCP behavior, ledger mutation, bounty creation, payout execution, treasury mutation, wallet behavior, private data, secrets, bridge/exchange/cash-out behavior, or MRWK price behavior.

Validation on current head:

  • uv run --extra dev pytest tests/test_activity.py tests/test_activity_routes.py -q -> 8 passed, 1 existing Starlette/httpx warning.
  • uv run --extra dev ruff check app/activity.py tests/test_activity.py tests/test_activity_routes.py -> passed.
  • uv run --extra dev ruff format --check app/activity.py tests/test_activity.py tests/test_activity_routes.py -> 3 files already formatted.
  • uv run --extra dev mypy app/activity.py -> success.
  • uv run --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 a90470f5e30e49fc311b33bfa5c8539f1cb67c93.

GitHub state before review: mergeStateStatus=CLEAN, mergeable=MERGEABLE; hosted Quality, readiness, docs, and image checks and CodeRabbit are successful on this head. The only prior human review was on stale head b11cc9e6af4bd301ce8efd5020bb226afb23747e, and scripts/review_bounty_candidates.py --repo ramimbo/mergework --reviewer laughlife --format text reports PR #858 as candidate_for_fresh_review because the latest useful human review is stale.

Copy link
Copy Markdown
Contributor

@alan747271363-art alan747271363-art 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 cfbad488b30da1c469f306bc2409a2f43fc53412 against current origin/main d7e9b530fffec7bd774da7708597648096a37393.

Scope inspected:

  • app/activity.py
  • tests/test_activity.py
  • supporting route coverage in tests/test_activity_routes.py

The branch-local fix is still focused: ACTIVITY_QUERY_MAX_LENGTH = 500 is enforced in the shared activity context used by both /api/v1/activity and /activity, boundary coverage confirms 500 characters is accepted and 501 is rejected, and the change does not alter account normalization, result serialization, MCP behavior, ledger mutation, bounty creation, payout execution, treasury mutation, wallet behavior, private data, or secrets handling.

Validation on this exact head:

  • .\.venv\Scripts\python.exe -m pytest tests\test_activity.py tests\test_activity_routes.py -q -> 8 passed, 1 existing Starlette/httpx warning.
  • .\.venv\Scripts\python.exe -m ruff check app\activity.py tests\test_activity.py tests\test_activity_routes.py -> passed.
  • .\.venv\Scripts\python.exe -m ruff format --check app\activity.py tests\test_activity.py tests\test_activity_routes.py -> 3 files already formatted.
  • .\.venv\Scripts\python.exe -m mypy app\activity.py -> success.
  • .\.venv\Scripts\python.exe scripts\docs_smoke.py -> docs smoke ok.
  • git diff --check origin/main...HEAD -> clean.

Current blocker: mergeability has changed. GitHub now reports mergeStateStatus=DIRTY / conflicting, and git merge-tree --write-tree origin/main HEAD exits non-zero with content conflicts in app/activity.py and tests/test_activity.py. This needs a current-main rebase/conflict resolution before merge.

No private data, credentials, wallet material, production mutation, payout execution, treasury execution, ledger mutation, exchange, bridge, cash-out, price behavior, or fabricated payout claims were used.

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.

4 participants