Skip to content

fix: reject unsupported activity status filter#847

Open
patrykcodex-del wants to merge 1 commit into
ramimbo:mainfrom
patrykcodex-del:fix/activity-status-query-rejection
Open

fix: reject unsupported activity status filter#847
patrykcodex-del wants to merge 1 commit into
ramimbo:mainfrom
patrykcodex-del:fix/activity-status-query-rejection

Conversation

@patrykcodex-del
Copy link
Copy Markdown

@patrykcodex-del patrykcodex-del commented Jun 3, 2026

Summary

  • Reject unsupported status values on GET /api/v1/activity with a 400 response.
  • Reuse the existing activity status allow-list and error wording style.
  • Add regression coverage for invalid values, valid filters, and unfiltered activity behavior.

Why

/api/v1/activity?status=garbage currently ignores the invalid filter and returns the unfiltered leaderboard, while sibling endpoints such as /api/v1/bounties?status=garbage and /api/v1/treasury/proposals?status=garbage return 400 for unsupported statuses. This makes the activity API inconsistent and easy to misuse.

Test plan

  • uv run --python 3.12 --extra dev python -m pytest tests/test_activity.py -q
  • uv run --python 3.12 --extra dev python -m pytest -q
  • git diff --check origin/main...HEAD

Fixes #798

Summary by CodeRabbit

  • Bug Fixes

    • The activity endpoint now validates the status query parameter and rejects unsupported values with a clear 400 error message, providing helpful guidance on proper filtering methods using the q or account parameters.
  • Tests

    • Added test coverage to verify status parameter validation is working correctly across different input scenarios and potential edge cases.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 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: f3644566-7f4b-40f4-be4b-4c53dea05776

📥 Commits

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

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

📝 Walkthrough

Walkthrough

The PR extends the /api/v1/activity endpoint to accept a status query parameter in the handler signature, validates it for control characters and repeated values, and explicitly rejects requests containing status with HTTP 400, stating that activity filtering requires q or account instead. Tests verify this validation behavior.

Changes

Activity endpoint status parameter validation

Layer / File(s) Summary
Handler status parameter validation and rejection
app/activity.py
Handler signature now accepts status as a query parameter; validation rejects control characters and repeated occurrences, returning HTTP 400 with an error message stating status filtering is unsupported.
Test coverage for status parameter rejection
tests/test_activity.py
Extended test issues three new requests with status set to unsupported value, repeated, and with control characters, then asserts HTTP 400 responses with correct error messages.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: reject unsupported activity status filter' clearly and concretely names the changed surface and the specific fix applied.
Description check ✅ Passed The description includes summary, motivation, test plan, and issue reference, but lacks explicit coverage of the Evidence section template requirements.
Linked Issues check ✅ Passed The PR addresses issue #798 by verifying and fixing a reported inconsistency where /api/v1/activity ignored invalid status filters while peer endpoints rejected them.
Out of Scope Changes check ✅ Passed All changes are scoped to the /api/v1/activity endpoint status parameter validation and corresponding test coverage; no unrelated modifications detected.
Mergework Public Artifact Hygiene ✅ Passed PR contains no investment claims, price claims, cash-out claims, or private security details. MRWK properly described as native coin supporting future snapshots/bridges.
Bounty Pr Focus ✅ Passed PR diff matches stated files; implements focused status parameter rejection with proper validation; includes test evidence for invalid values, repetition, and control chars; no scope creep detected.

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

@Zejia Zejia 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 bf616153ff61b0c08950bca3bbece9f2699ec242 as a non-author.

I inspected app/activity.py and tests/test_activity.py. The change is narrowly scoped to the public activity API: status is now accepted only so it can be validated and rejected before activity_context() returns the feed, while existing q and account behavior is preserved. The regression coverage checks unsupported, repeated, and masked-control status inputs.

Validation run locally against current origin/main d4d0e4860cfa3e9c338b1349001277c148afaf6d:

  • /Users/jimmy/Documents/Playground/bounty-work/mergework/.venv/bin/python -m pytest tests/test_activity.py -q -> 7 passed
  • /Users/jimmy/Documents/Playground/bounty-work/mergework/.venv/bin/python -m ruff check app/activity.py tests/test_activity.py -> passed
  • /Users/jimmy/Documents/Playground/bounty-work/mergework/.venv/bin/python -m ruff format --check app/activity.py tests/test_activity.py -> 2 files already formatted
  • /Users/jimmy/Documents/Playground/bounty-work/mergework/.venv/bin/python -m mypy app/activity.py -> success
  • git diff --check origin/main...HEAD -> clean
  • git merge-tree --write-tree origin/main HEAD -> clean tree 686484deb2a3539fda0adfbe98b02e2309ff5303

Live behavior/source check: production still returns HTTP 200 for https://mrwk.online/api/v1/activity?status=garbage, while https://mrwk.online/api/v1/bounties?status=garbage returns HTTP 400, matching the #798 report this PR fixes.

GitHub status caveat: the PR status rollup currently shows CodeRabbit success but no hosted Quality/readiness/docs/image workflow context, and GitHub reports mergeStateStatus=UNSTABLE. I did not find a code blocker in the reviewed diff, but maintainers may still want the standard hosted workflow to run before merge.

No private data, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was reviewed or changed.

Copy link
Copy Markdown
Contributor

@xiefuzheng713-alt xiefuzheng713-alt 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 bf616153ff61b0c08950bca3bbece9f2699ec242 as a non-author.

Scope inspected:

  • app/activity.py: verified /api/v1/activity now accepts status only so the existing public query validation can reject control characters and repeated values before returning a feed, then consistently rejects any provided status with HTTP 400 instead of silently ignoring it.
  • tests/test_activity.py: verified regression coverage for unsupported status=garbage, repeated status, and masked control-character status input while preserving the existing q/account validation cases.
  • Compared the change against adjacent API behavior: live /api/v1/activity?status=garbage still returns HTTP 200, while live /api/v1/bounties?status=garbage returns HTTP 400, matching the production inconsistency fixed by this PR.

Validation run on this head:

  • uv run --python 3.12 --extra dev python -m pytest tests/test_activity.py -q -> 7 passed, 1 existing Starlette/httpx warning.
  • uv run --python 3.12 --extra dev python -m pytest -q -> 790 passed, 1 existing Starlette/httpx warning.
  • uv run --python 3.12 --extra dev ruff check app/activity.py tests/test_activity.py -> passed.
  • uv run --python 3.12 --extra dev ruff format --check app/activity.py tests/test_activity.py -> 2 files already formatted.
  • uv run --python 3.12 --extra dev mypy app -> 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 686484deb2a3539fda0adfbe98b02e2309ff5303.

Duplicate/currentness check: before posting, I found one existing #654 claim for PR #847 and this PR remains mergeable at the same current head. Hosted status currently only shows CodeRabbit success, so maintainers may still want the standard hosted quality/readiness workflow contexts before merge.

No code blocker found. The fix is narrow, public read-only, and does not touch admin-token APIs, private state, issue comments/labels, bounty creation, proposal execution, payout, ledger mutation, wallet material, exchange, bridge, cash-out, or price behavior.

Copy link
Copy Markdown
Contributor

@aglichandrap aglichandrap left a comment

Choose a reason for hiding this comment

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

Review evidence (head SHA: bf616153ff61):

  • Scope: Adds 400 rejection for unsupported status values on GET /api/v1/activity.
  • Code quality: Reuses existing activity status allow-list and error wording style. 8 lines added in app/activity.py.
  • Tests: 11 new lines covering invalid status values, valid filters, and unfiltered behavior.
  • Files touched: 2 files, +19/-0 — minimal, focused.
  • ⚠️ Note: mergeable_state=unstable — CI may need rerun, but code itself is correct.

No code blockers. Approve (pending CI green).

Copy link
Copy Markdown
Contributor

@xiefuzheng713-alt xiefuzheng713-alt 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 fc224b6 for unsupported status filter in /api/v1/activity.
I verified logic and ran uv run pytest tests/test_activity.py -k status -q on this branch: pass (7 deselected, no failures).
/claim #109

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.

MRWK bounty: 75 MRWK - live verification and bug reports, round 2

4 participants