Extract admin payout helpers#360
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extracts payout-related helper functions into a dedicated app/payouts.py module and adds unit tests to validate payout response payloads and duplicate payout detection. It also hardens _safe_next_path against additional encoded open-redirect/path abuse cases.
Changes:
- Added
app/payouts.pywithpayout_response_from_proofandexisting_payout_proof_for_submission. - Added
tests/test_payouts.pyto cover payout payload shaping and duplicate-proof lookup behavior. - Updated
app/main.pyto use extracted helpers and expanded_safe_next_pathvalidation usingunquote.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/test_payouts.py | New tests validating payout response payloads and duplicate payout detection. |
| app/payouts.py | New module extracted from main.py for payout-response shaping and duplicate-proof lookup. |
| app/main.py | Replaces in-file helpers with imports; expands _safe_next_path validation with decoded checks. |
| app/ledger/service.py | Adjusts typing around session.execute(...update...) results by removing cast/CursorResult. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_payout_response_from_proof_shapes_admin_api_payload(sqlite_url: str) -> None: | ||
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| bounty = create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=320, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/320", | ||
| title="Payout helper extraction", | ||
| reward_mrwk="15", | ||
| acceptance="Payout API response helpers should be independently testable.", | ||
| ) | ||
| proof = pay_bounty( | ||
| session, | ||
| bounty_id=bounty.id, | ||
| to_account="github:alice", | ||
| submission_url="https://github.com/ramimbo/mergework/pull/320", | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
|
|
||
| payload = payout_response_from_proof(proof, status="paid") | ||
|
|
||
| assert payload == { | ||
| "status": "paid", | ||
| "bounty_id": bounty.id, | ||
| "to_account": "github:alice", | ||
| "submission_id": proof.submission_id, | ||
| "submission_url": "https://github.com/ramimbo/mergework/pull/320", | ||
| "ledger_sequence": proof.ledger_sequence, | ||
| "ledger_url": f"/ledger/{proof.ledger_sequence}", | ||
| "proof_hash": proof.hash, | ||
| "proof_url": f"/proofs/{proof.hash}", | ||
| } |
| def test_existing_payout_proof_for_submission_finds_duplicate_proof(sqlite_url: str) -> None: | ||
| create_schema(sqlite_url) | ||
| submission_url = "https://github.com/ramimbo/mergework/pull/321" | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| bounty = create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=321, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/321", | ||
| title="Duplicate payout helper", | ||
| reward_mrwk="15", | ||
| acceptance="Duplicate payout lookup should find the existing proof.", | ||
| ) | ||
| proof = pay_bounty( | ||
| session, | ||
| bounty_id=bounty.id, | ||
| to_account="github:alice", | ||
| submission_url=submission_url, | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
|
|
||
| found = existing_payout_proof_for_submission(session, bounty.id, submission_url) | ||
| missing = existing_payout_proof_for_submission( | ||
| session, bounty.id, "https://github.com/ramimbo/mergework/pull/999" | ||
| ) | ||
|
|
||
| assert found is not None | ||
| assert found.hash == proof.hash | ||
| assert missing is None |
|
|
||
|
|
||
| def payout_response_from_proof(proof: Proof, *, status: str) -> dict[str, Any]: | ||
| data = json.loads(proof.public_json) |
| from fastapi import HTTPException | ||
| from sqlalchemy import select | ||
| from sqlalchemy.orm import Session | ||
|
|
||
| from app.models import Proof, Submission | ||
|
|
||
|
|
||
| def payout_response_from_proof(proof: Proof, *, status: str) -> dict[str, Any]: | ||
| data = json.loads(proof.public_json) | ||
| if not isinstance(data, dict) or data.get("kind") != "bounty_payment": | ||
| raise HTTPException(status_code=500, detail="invalid proof payload") |
| from fastapi import HTTPException | ||
| from sqlalchemy import select | ||
| from sqlalchemy.orm import Session | ||
|
|
||
| from app.models import Proof, Submission | ||
|
|
||
|
|
||
| def payout_response_from_proof(proof: Proof, *, status: str) -> dict[str, Any]: | ||
| data = json.loads(proof.public_json) | ||
| if not isinstance(data, dict) or data.get("kind") != "bounty_payment": | ||
| raise HTTPException(status_code=500, detail="invalid proof payload") |
| claimed: Any = session.execute( | ||
| update(Bounty) | ||
| .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) | ||
| .values( | ||
| awards_paid=Bounty.awards_paid + 1, | ||
| status=case( | ||
| (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), | ||
| else_="open", | ||
| ), | ||
| ) | ||
| ) | ||
| if claimed.rowcount != 1: |
bitdamii
left a comment
bitdamii
left a comment
There was a problem hiding this comment.
Reviewed PR #360 for the MRWK #340 review bounty.
Evidence checked:
- Inspected
app/payouts.py,app/main.py,app/ledger/service.py, andtests/test_payouts.pyat head88997109b3c2008ffbd66a3d6f9b502a4bfd0bfb. - Verified
payout_response_from_proofpreserves the admin payout payload/proof URL fields and thatexisting_payout_proof_for_submissionstill backs the duplicate-submission 409 path inapi_pay_bounty. - Checked the ledger
pay_bounty/close_bountyrowcount guards remain intact after the SQLAlchemy typing cleanup. - Confirmed the added payout helper tests cover payload shaping and existing-proof lookup.
Local validation:
uv run --extra dev python -m pytest tests/test_payouts.py tests/test_security.py::test_admin_payout_api_returns_existing_proof_for_duplicate_submission tests/test_security.py::test_admin_payout_api_requires_admin_token_not_cookie_auth tests/test_security.py::test_admin_payout_api_omits_blank_note_from_public_proof tests/test_ledger.py -q-> 35 passed.uv run --extra dev ruff check app/main.py app/ledger/service.py app/payouts.py tests/test_payouts.py-> passed.uv run --extra dev ruff format --check app/main.py app/ledger/service.py app/payouts.py tests/test_payouts.py-> passed.uv run --extra dev python -m mypy app-> success.git diff --check origin/main...HEAD-> clean.
No blockers found.
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
No blockers found on current head 8899710.
Evidence checked:
- Inspected
app/payouts.py, the admin payout route wiring inapp/main.py, the rowcount typing cleanup inapp/ledger/service.py, andtests/test_payouts.py. - Verified
payout_response_from_proof()preserves the admin API response contract for paid and already-paid responses: bounty id, recipient account, submission id/url, ledger sequence/url, and proof hash/url all still come from the persisted proof/public payload. - Verified
existing_payout_proof_for_submission()remains scoped by both bounty id and exact submission URL before returning an existing bounty-payment proof, so duplicate payout retries return the original proof instead of issuing a second payment. - Rechecked the encoded OAuth
nexthardening added on this branch: decoded//, backslash, and control-character payloads fall back to/mewhile normal relative paths stay allowed.
Validation run locally:
uv run --extra dev python -m pytest tests/test_payouts.py tests/test_security.py::test_admin_payout_api_returns_existing_proof_for_duplicate_submission tests/test_security.py::test_oauth_next_path_rejects_external_or_headerlike_paths tests/test_wallet_api.py::test_oauth_next_path_rejects_redirect_ambiguity tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 18 passed.uv run --extra dev python -m mypy app/payouts.py app/main.py app/ledger/service.py-> success.uv run --extra dev python -m ruff check app/main.py app/payouts.py tests/test_payouts.py app/ledger/service.py-> passed.uv run --extra dev python -m ruff format --check app/main.py app/payouts.py tests/test_payouts.py app/ledger/service.py-> 4 files already formatted.git diff --check origin/main...HEAD-> clean.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
|
Evidence: Inspected app/main.py, app/payouts.py, app/ledger/service.py, and tests/test_payouts.py. |
Refs #320
Summary
app/main.pyintoapp/payouts.py.mypy appand by sanitizing percent-decoded OAuthnextpaths covered by the existing OAuth regression test.Complexity reduced
app/main.pyno longer owns payout proof lookup or admin payout response formatting. The pay route now delegates those responsibilities to a small payout module with direct tests, leaving the route focused on request parsing, authorization, and orchestration.Tests
python -m pytest -q-> 337 passedpython -m pytest tests/test_payouts.py tests/test_security.py::test_admin_payout_api_returns_existing_proof_for_duplicate_submission tests/test_security.py::test_admin_payout_api_requires_admin_token_not_cookie_auth tests/test_security.py::test_admin_payout_api_omits_blank_note_from_public_proof tests/test_ledger.py -q-> 35 passedpython -m ruff format --check app/main.py app/ledger/service.py app/payouts.py tests/test_payouts.pypython -m ruff check app/main.py app/ledger/service.py app/payouts.py tests/test_payouts.pypython -m mypy app