Honor accepted-work API limit#865
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 (1)
📝 WalkthroughWalkthroughAdds an optional ChangesAccepted-Work Limit Feature
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
laughlife
left a comment
There was a problem hiding this comment.
Reviewed current head 18d1f9cb4e8cef56cced09c79ce4952c8a62aae3 as a non-author.
Scope checked:
app/accounts.py:/api/v1/accounts/{account}/accepted-worknow accepts a boundedlimitquery parameter, rejects repeatedlimit, rejects non-canonical integer spellings, and passes the parsed limit intoaccount_accepted_work_context().app/serializers.py:accepted_work_for_account()now applies the optional limit at the SQL query level before serializing proof-backed rows, so the API does not fetch the full account history whenlimit=1is requested.tests/test_account_routes.py: covers the production report behavior for the API: full response remains uncapped by default,limit=1returns one latest row while preserving full summary totals, andlimit=01fails closed.
Local validation on this head:
uv run --extra dev pytest tests/test_account_routes.py::test_account_accepted_work_api_honors_canonical_limit -q-> 1 passed, 1 existing Starlette/httpx warning.uv run --extra dev pytest tests/test_account_routes.py -q-> 8 passed, 1 existing Starlette/httpx warning.uv run --extra dev ruff check app/accounts.py app/serializers.py tests/test_account_routes.py-> passed.uv run --extra dev ruff format --check app/accounts.py app/serializers.py tests/test_account_routes.py-> 3 files already formatted.uv run --extra dev mypy app/accounts.py app/serializers.py-> success.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.git merge-tree origin/main HEAD-> clean tree7fb1b2541ddfeb1b3655e87b5f2dd4a4de39f295.
This PR is a focused API-path fix. It does not cover the public account page row limit or docs wording that PR #864 covers, while PR #864 does not push the API limit down into the SQL query. Maintainers may want to choose one direction or consolidate the API query-level limit with the broader page/docs coverage. CodeRabbit was still pending at review time and was not counted as completed evidence.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6855597-94ad-4dc7-a158-6242b4b2608b
📒 Files selected for processing (3)
app/accounts.pyapp/serializers.pytests/test_account_routes.py
|
Follow-up for the CodeRabbit boundary coverage warning. Added assertions in
Local validation on new head
|
laughlife
left a comment
There was a problem hiding this comment.
Current-head review for bd41893191e954ff6f82d44cc04ffe6633f7c295.
I rechecked this after the boundary-test update made my earlier review stale. This PR cleanly limits the accepted-work API at query level via accepted_work_for_account(..., limit=...), preserves the default unbounded API shape when limit is omitted, and keeps summary totals based on the full account history while capping only the returned accepted_work rows.
Validation I ran locally:
uv run --extra dev pytest tests/test_account_routes.py::test_account_accepted_work_api_honors_canonical_limit -q-> 1 passed, 1 existing Starlette/httpx warning.uv run --extra dev pytest tests/test_account_routes.py -q-> 8 passed, 1 existing Starlette/httpx warning.uv run --extra dev ruff check app/accounts.py app/serializers.py tests/test_account_routes.py-> passed.uv run --extra dev ruff format --check app/accounts.py app/serializers.py tests/test_account_routes.py-> passed.uv run --extra dev mypy app/accounts.py app/serializers.py-> passed.uv run --extra dev python scripts/docs_smoke.py-> passed.git diff --check origin/main...HEAD-> clean.git merge-tree origin/main HEAD-> clean tree943dd339efbed34059bc019651c54e95b789dbab.
The hosted Quality/readiness/docs check is successful. CodeRabbit was still pending at review time, so I am not counting that as completed evidence. This overlaps with PR #864 only around the accepted-work limit theme; this version is specifically the API/query-level implementation.
xiefuzheng713-alt
left a comment
There was a problem hiding this comment.
Reviewed current head bd41893191e954ff6f82d44cc04ffe6633f7c295 for the accepted-work API limit fix.
Checked changed files:
app/accounts.pyapp/serializers.pytests/test_account_routes.py
What I verified:
/api/v1/accounts/{account}/accepted-worknow accepts an optional boundedlimit=1..200query value.- Repeated and non-canonical
limitvalues are rejected through the existing query validation helpers. account_accepted_work_context()forwards the limit only to the accepted-work row query.account_accepted_summary()remains uncapped, so summary totals still describe the full account history.accepted_work_for_account()applies SQL-level.limit(limit)only when the caller provides a limit.- Regression coverage verifies full output when omitted,
limit=1returning the newest row,limit=200, 422 range errors, repeated limit rejection, andlimit=01rejection.
GitHub state checked before review: mergeStateStatus=CLEAN, hosted Quality/readiness/docs/image check passed, and CodeRabbit status passed. I did not see regressions in the touched API behavior.
tudorian95
left a comment
There was a problem hiding this comment.
Reviewed current head bd41893191e954ff6f82d44cc04ffe6633f7c295 for Bounty #838.
What I checked:
- Inspected the focused diff in
app/accounts.py,app/serializers.py, andtests/test_account_routes.py. - Confirmed the live production before-state is still reproducible with harmless GETs:
GET /api/v1/accounts/github:likeacloud7/accepted-workreturned 8 accepted-work rows, andGET /api/v1/accounts/github:likeacloud7/accepted-work?limit=1also returned 8 rows while preserving summary totals. I also confirmedlimit=01and repeatedlimit=1&limit=2currently return HTTP 200 on production. - In Docker/uv on a fresh checkout of this PR head:
uv run --extra dev pytest tests/test_account_routes.py::test_account_accepted_work_api_honors_canonical_limit -q-> 1 passed, 1 existing Starlette/httpx warning.uv run --extra dev pytest tests/test_account_routes.py tests/test_serializers.py tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account tests/test_api_mcp.py::test_account_api_keeps_schema_when_accepted_work_proof_is_malformed -q-> 21 passed, 1 existing warning.uv run --extra dev ruff check app/accounts.py app/serializers.py tests/test_account_routes.py-> passed.uv run --extra dev ruff format --check app/accounts.py app/serializers.py tests/test_account_routes.py-> passed.uv run --extra dev mypy app/accounts.py app/serializers.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 HEADsucceeded with tree943dd339efbed34059bc019651c54e95b789dbab.- GitHub reports the current head as
MERGEABLE.
The implementation limits only the returned accepted-work rows while leaving account summary totals uncapped, which preserves the API shape and expected accounting semantics. I did not find blockers.
Bounty #799
Source report: #798 (comment)
This PR implements a focused fix for the public account accepted-work API silently ignoring
limitquery values:/api/v1/accounts/{account}/accepted-work?limit=1now returns one accepted-work row while keeping the summary totals uncapped.limitis bounded with the same 1..200 shape used by nearby public list endpoints.limit=01are rejected before FastAPI coercion.Validation:
uv run --extra dev pytest tests/test_account_routes.py::test_account_accepted_work_api_honors_canonical_limit -q-> 1 passed, 1 existing Starlette/httpx warning.uv run --extra dev pytest tests/test_account_routes.py tests/test_serializers.py tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account tests/test_api_mcp.py::test_account_api_keeps_schema_when_accepted_work_proof_is_malformed -q-> 21 passed, 1 existing Starlette/httpx warning.uv run --extra dev ruff check app/accounts.py app/serializers.py tests/test_account_routes.py-> passed.uv run --extra dev ruff format --check app/accounts.py app/serializers.py tests/test_account_routes.py-> 3 files already formatted.uv run --extra dev mypy app/accounts.py app/serializers.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 tree7fb1b2541ddfeb1b3655e87b5f2dd4a4de39f295.Scope: public account accepted-work row limiting only. No payout execution, treasury mutation, wallet mutation, ledger mutation, admin-token behavior, private data, secrets, bridge/exchange/cash-out behavior, or MRWK price behavior changed.
Summary by CodeRabbit
limitquery parameter (1–200) to the accepted-work endpoint; requests with non-canonical formatting are rejected with appropriate validation errors.