Skip to content

Expose admin payout award state#288

Merged
ramimbo merged 1 commit into
ramimbo:mainfrom
TateLyman:codex/admin-payout-observability
May 25, 2026
Merged

Expose admin payout award state#288
ramimbo merged 1 commit into
ramimbo:mainfrom
TateLyman:codex/admin-payout-observability

Conversation

@TateLyman
Copy link
Copy Markdown
Contributor

@TateLyman TateLyman commented May 25, 2026

Refs #283

Summary

  • add maintainer-visible award state to admin payout API responses (bounty_status, awards_paid, awards_remaining)
  • return the accepted submission_url, proof_url, and ledger_sequence alongside the proof hash
  • cover duplicate admin payout handling so a repeated submission URL reports submission already paid without incrementing awards or creating a second payment

Validation

  • uv run pytest tests/test_security.py -q
  • uv run pytest tests/test_webhooks.py tests/test_activity.py -q
  • uv run ruff check app/main.py tests/test_security.py
  • uv run ruff format --check app/main.py tests/test_security.py
  • git diff --check

Summary by CodeRabbit

  • New Features

    • Payment endpoint now returns richer payout details: submission URL, proof URL, ledger sequence, and updated award counters/status.
  • Bug Fixes

    • Returns a clear 400 error when post-payment data cannot be retrieved or when duplicate payouts are submitted.
  • Tests

    • Added tests covering duplicate-submission protection and verification of award state, counts, and payout fields.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7eea8adc-4bb7-48f8-91e0-5bafeae55798

📥 Commits

Reviewing files that changed from the base of the PR and between 97190af and a3e89e4.

📒 Files selected for processing (2)
  • app/main.py
  • tests/test_security.py

📝 Walkthrough

Walkthrough

The PR enhances the bounty payout API to return detailed award-state information and payment metadata after processing a bounty payment. The endpoint now fetches and includes the updated bounty status, award counts, submission source, and proof metadata. A comprehensive test validates the enriched response, verifies database state, and exercises duplicate-submission protection.

Changes

Payout Flow Enhancement

Layer / File(s) Summary
Enhanced payout endpoint response
app/main.py
After pay_bounty succeeds, the handler fetches the updated Bounty row and extracts submission_url from proof.public_json, returning bounty_status, awards_paid, awards_remaining, submission_url, proof_url, and ledger_sequence instead of only to_account and proof_hash.
Award-state reporting and duplicate-submission test
tests/test_security.py
New test creates a bounded bounty, posts admin payout and validates the enriched response fields, verifies database award counters, rejects duplicate submissions with a 400 error, and confirms the second payout updates the remaining award count.

🎯 3 (Moderate) | ⏱️ ~20 minutes


🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Expose admin payout award state' directly and clearly summarizes the main change: adding maintainer-visible award state fields to the admin payout API response.
Description check ✅ Passed The description covers all major changes with specific field names, includes validation steps that were executed with results, and properly references issue #283 using the template's MRWK section.
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.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_security.py`:
- Around line 318-322: Change the duplicate-submission assertion to ensure
deduping is based solely on submission_url: after the original first_payload,
send a second payload that reuses the same "submission_url" but changes
"to_account" (e.g., "github:bob") and assert the response indicates "submission
already paid"/duplicate; update the analogous duplicate check section around the
later block (the other payload at 338-340) the same way so both tests verify
URL-only dedupe.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5167b5d8-cdc2-401c-b359-7d9ef3640124

📥 Commits

Reviewing files that changed from the base of the PR and between 1eb4c77 and 3425ab2.

📒 Files selected for processing (2)
  • app/main.py
  • tests/test_security.py

Comment thread tests/test_security.py
@TateLyman
Copy link
Copy Markdown
Contributor Author

Addressed the duplicate-submission coverage note in 97190af.

The duplicate admin payout call now reuses the same submission_url but changes to_account to github:carol, so the test verifies URL-only dedupe rather than a full-payload duplicate.

Re-validation:

  • uv run pytest tests/test_security.py -q -> 37 passed.
  • uv run ruff check tests/test_security.py -> passed.
  • uv run ruff format --check tests/test_security.py -> passed.
  • git diff --check -> passed.

@TateLyman TateLyman force-pushed the codex/admin-payout-observability branch from 97190af to a3e89e4 Compare May 25, 2026 19:58
@TateLyman
Copy link
Copy Markdown
Contributor Author

Rebased this branch onto current main and preserved the duplicate-submission coverage fix.

Re-validation after conflict resolution:

  • uv run pytest tests/test_security.py tests/test_webhooks.py tests/test_activity.py -q -> 68 passed.
  • uv run ruff check app/main.py tests/test_security.py -> passed.
  • uv run ruff format --check app/main.py tests/test_security.py -> passed.
  • git diff --check -> passed.

Copy link
Copy Markdown
Contributor

@Karry2019web Karry2019web left a comment

Choose a reason for hiding this comment

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

Review PR #288: Expose admin payout award state

Evidence

Files inspected:

  • app/main.pyapi_pay_bounty() response enriched with bounty_status, awards_paid, awards_remaining, submission_url, proof_url, ledger_sequence
  • tests/test_security.pytest_admin_payout_api_reports_award_state_and_blocks_duplicate_submission

Behavior checked:

  • After payment, response now includes bounty_status: 'open', awards_paid: 1, awards_remaining: 1 (for max_awards=2 bounty) — reads from existing bounty_to_dict()
  • submission_url comes from proof.public_json payload, properly parsed
  • proof_url and ledger_sequence from the Proof model
  • Duplicate submission on same submission_url returns 400 as expected
  • CI: all checks pass (CodeRabbit: success)

Result: LGTM, no blockers. Clean enhancement — adds observability without changing behavior.

Copy link
Copy Markdown
Contributor

@Karry2019web Karry2019web left a comment

Choose a reason for hiding this comment

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

Review ✅ APPROVED

FILES INSPECTED: app/main.py, tests/test_security.py

BEHAVIOR CHECKED:

  • Admin payout endpoint now returns: bounty_status, awards_paid, awards_remaining, submission_url, proof_url, ledger_sequence alongside existing proof_hash.
  • bounty_state = bounty_to_dict(bounty) and proof_payload = json.loads(proof.public_json) provide the source data.

TESTS VERIFIED:

  • test_admin_payout_api_reports_award_state_and_blocks_duplicate_submission — comprehensive 3-scenario test:
    1. First payout: asserts status=paid, bounty_status=open, awards_paid=1, awards_remaining=1, correct to_account, submission_url, proof_hash, proof_url, ledger_sequence (int).
    2. Duplicate submission: returns 400 with "submission already paid", bounty awards_paid NOT incremented (remains 1), balance unchanged.
    3. Second distinct payout: fills remaining slot, bounty_status=paid, awards_paid=2, awards_remaining=0.

SECURITY: Duplicate submission prevention is critical for ledger integrity.

Excellent PR for #283.

@ramimbo ramimbo merged commit 6ff699f into ramimbo:main May 25, 2026
2 checks passed
@ramimbo ramimbo added mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded labels May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants