Expose bounty finalization evidence in public rows#849
Conversation
📝 WalkthroughWalkthroughThis PR adds end-to-end support for serializing finalization evidence on bounties. It extends the bounty serializers and attempt-summary helpers to accept and thread finalization evidence extracted from executed treasury proposals, exposes the evidence in API responses and documentation, and validates the behavior through route and unit tests. ChangesFinalization Evidence Serialization
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65fcfeec-548d-4b21-8002-58916b4c4ef6
📒 Files selected for processing (6)
app/bounty_attempts.pyapp/serializers.pydocs/api-examples.mdtests/test_bounty_api_routes.pytests/test_docs_public_urls.pytests/test_serializers.py
| lookup_session = None if pending_proposals is not None else session | ||
| bounty_data = bounty_to_dict( | ||
| bounty, | ||
| session=session, | ||
| session=lookup_session, | ||
| pending_proposals=pending_proposals, | ||
| attempt_summary={}, | ||
| finalization_evidence=finalization_evidence, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Warnings path triggers a redundant finalization-evidence scan.
_bounty_attempt_warnings_for_count never reads finalization_evidence (it only consumes effective_awards_remaining, status, availability_state, availability_note). But via bounty_attempt_warnings (Line 139-144) lookup_session is the session and finalization_evidence is None, so bounty_to_dict auto-computes _bounty_finalization_evidence — a LIKE/contains scan over treasury_proposals — whose result is thrown away. api_create_bounty_attempt calls bounty_attempt_warnings on every response path (Lines 316, 334, 370, 378), so this adds one wasted scan per call on a request thread.
Short-circuit the auto-compute since the value is unused here:
Proposed fix
bounty_data = bounty_to_dict(
bounty,
session=lookup_session,
pending_proposals=pending_proposals,
attempt_summary={},
- finalization_evidence=finalization_evidence,
+ finalization_evidence=finalization_evidence if finalization_evidence is not None else {},
)| return [bounty_to_dict(bounty) for bounty in bounties] | ||
|
|
||
| pending_by_bounty = _pending_bounty_proposals_by_bounty_id(session) | ||
| finalization_by_bounty = _create_bounty_finalization_evidence_by_bounty_id(session) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Bulk preload scans every executed create_bounty proposal, not just the listed bounties.
_create_bounty_finalization_evidence_by_bounty_id(session) ignores the bounties being serialized and loads/parses all executed create_bounty results, then discards entries for bounties not in the page. On a public list route this grows O(total bounties ever created) per request, independent of page size.
Consider scoping the preload to the bounty ids in bounties (e.g. an or_ of _result_bounty_id_filter(id) markers, or filtering after fetch) so list serialization cost tracks the page, not lifetime bounty count.
| def test_bounty_api_omits_finalization_evidence_when_not_recorded(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=75, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/75", | ||
| title="Older bounty without finalization result", | ||
| reward_mrwk="25", | ||
| acceptance="Older rows should stay compatible.", | ||
| ) | ||
| bounty_id = bounty.id | ||
|
|
||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
|
|
||
| detail = client.get(f"/api/v1/bounties/{bounty_id}").json() | ||
| listed = client.get("/api/v1/bounties?repo=ramimbo%2Fmergework&issue_number=75").json()[0] | ||
|
|
||
| assert "finalization_evidence" not in detail | ||
| assert "finalization_evidence" not in listed |
There was a problem hiding this comment.
Exercise the executed-proposal omission path.
This test creates the bounty directly, so it never reaches the serializer branch that omits finalization_evidence for an executed create_bounty proposal with missing/non-public github_issue_finalization. A regression in app/serializers.py:265-292 would still pass here. Please add a case that executes a proposal and then records either no finalization field or a field without public URLs, and assert the key stays absent.
As per coding guidelines, tests/**/*.py: Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.
| def test_bounties_to_dict_exposes_preloaded_finalization_evidence(sqlite_url: str) -> None: | ||
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| proposal = propose_treasury_action( | ||
| session, | ||
| action="create_bounty", | ||
| payload={ | ||
| "repo": "ramimbo/mergework", | ||
| "issue_number": 324, | ||
| "issue_url": "https://github.com/ramimbo/mergework/issues/324", | ||
| "title": "Finalization evidence serializer", | ||
| "reward_mrwk": "25", | ||
| "max_awards": 1, | ||
| "acceptance": "Serializer should expose stored finalization evidence.", | ||
| }, | ||
| proposed_by="maintainer", | ||
| ) | ||
| proposal_id = proposal.id | ||
| proposal.executes_after = utc_now() - timedelta(seconds=1) | ||
| session.flush() | ||
| executed = execute_treasury_proposal( | ||
| session, proposal_id=proposal_id, executed_by="maintainer" | ||
| ) | ||
| bounty_id = int(json.loads(executed.result_json)["bounty"]["id"]) | ||
| record_proposal_result_field( | ||
| session, | ||
| proposal_id=proposal_id, | ||
| field="github_issue_finalization", | ||
| value={ | ||
| "status": "updated", | ||
| "label": "mrwk:bounty", | ||
| "bounty_url": "https://api.mrwk.online/bounties/324", | ||
| "comment_url": ("https://github.com/ramimbo/mergework/issues/324#issuecomment-1"), | ||
| "issue_body_status": "updated", | ||
| }, | ||
| ) | ||
| bounty = session.get(Bounty, bounty_id) | ||
| assert bounty is not None | ||
| treasury_selects: list[str] = [] | ||
|
|
||
| def count_treasury_selects( | ||
| conn, | ||
| cursor, | ||
| statement, | ||
| parameters, | ||
| context, | ||
| executemany, | ||
| ) -> None: | ||
| del conn, cursor, parameters, context, executemany | ||
| if "treasury_proposals" in statement.lower() and statement.lstrip().upper().startswith( | ||
| "SELECT" | ||
| ): | ||
| treasury_selects.append(statement) | ||
|
|
||
| bind = session.get_bind() | ||
| event.listen(bind, "before_cursor_execute", count_treasury_selects) | ||
| try: | ||
| serialized = bounties_to_dict([bounty], session=session) | ||
| finally: | ||
| event.remove(bind, "before_cursor_execute", count_treasury_selects) | ||
|
|
||
| assert serialized[0]["finalization_evidence"] == { | ||
| "create_bounty_proposal_id": proposal_id, | ||
| "create_bounty_proposal_url": f"/api/v1/treasury/proposals/{proposal_id}", | ||
| "status": "updated", | ||
| "public_bounty_url": "https://api.mrwk.online/bounties/324", | ||
| "claims_open_comment_url": ( | ||
| "https://github.com/ramimbo/mergework/issues/324#issuecomment-1" | ||
| ), | ||
| "label": "mrwk:bounty", | ||
| "issue_body_status": "updated", | ||
| } | ||
| assert len(treasury_selects) == 2 |
There was a problem hiding this comment.
Use more than one bounty to prove list preloading.
bounties_to_dict([bounty], session=session) does not distinguish bulk preload from per-bounty lookup; an N+1 regression would still pass with a single-element list. Add at least one more bounty and keep len(treasury_selects) == 2 so this test actually locks in the preload behavior from app/serializers.py:122-150.
As per coding guidelines, tests/**/*.py: Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.
xiefuzheng713-alt
left a comment
xiefuzheng713-alt
left a comment
There was a problem hiding this comment.
Bounty #654 current-head review for PR #849.
Reviewed head aab6f79e43cc563f4662dec746bb831f8ef05c51 against base d4d0e4860cfa3e9c338b1349001277c148afaf6d as a non-author. I inspected app/serializers.py, app/bounty_attempts.py, tests/test_serializers.py, tests/test_bounty_api_routes.py, tests/test_docs_public_urls.py, and docs/api-examples.md.
Blocking finding: bounties_to_dict() now calls _create_bounty_finalization_evidence_by_bounty_id(session) for every list serialization, but that helper selects every executed create_bounty proposal without restricting the scan to the bounty rows being serialized. A filtered list/page with one bounty therefore still fetches and parses all historical create-bounty proposal results, so public bounty list cost grows with lifetime proposal history instead of with the returned page. The current preload test only asserts that there are two treasury SELECTs; it does not catch that the finalization SELECT is unbounded.
Current-head reproduction:
- Created 3 executed
create_bountyproposals with finalization evidence in a temporary SQLite DB. - Serialized only the first bounty via
bounties_to_dict([listed], session=session). - Observed
parsed_proposal_ids_during_single_row_serialize = [1, 2, 3]even thoughlisted_bounty_id = 1. - The finalization SELECT was:
WHERE treasury_proposals.status = ? AND treasury_proposals.action = ? ORDER BY treasury_proposals.id ASC, params('executed', 'create_bounty'), with noresult_jsonbounty-id filter.
Suggested fix: pass the listed bounty ids into the finalization preload helper and apply _result_bounty_id_filter()/equivalent filtering for those ids before fetching rows, then add a regression with at least one unrelated executed create_bounty proposal that is not fetched/parsed for the serialized list.
Validation on the reviewed head:
uv run --extra dev pytest tests/test_serializers.py::test_bounties_to_dict_exposes_preloaded_finalization_evidence tests/test_bounty_api_routes.py::test_bounty_api_exposes_create_bounty_finalization_evidence -q-> 2 passed, 1 existing Starlette/httpx warning.uv run --extra dev pytest tests/test_serializers.py tests/test_bounty_api_routes.py tests/test_docs_public_urls.py -q-> 70 passed, 1 existing warning.uv run --extra dev ruff check app/serializers.py app/bounty_attempts.py tests/test_serializers.py tests/test_bounty_api_routes.py docs/api-examples.md tests/test_docs_public_urls.py-> passed.uv run --extra dev ruff format --check app/serializers.py app/bounty_attempts.py tests/test_serializers.py tests/test_bounty_api_routes.py-> passed.uv run --extra dev mypy app-> success for 42 source files.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 tree9f9a2ce605d6ffc661550851446cafab47f03f76.
No private data, secrets, wallet material, payout execution, treasury mutation, ledger mutation, exchange, bridge, cash-out, price behavior, or fabricated payout claim was used.
aglichandrap
left a comment
aglichandrap
left a comment
There was a problem hiding this comment.
Review evidence (head SHA: aab6f79e43cc):
- ✅ Scope: Passes
finalization_evidencethroughbounty_attempt_warnings_for_countandbounty_attempt_summary_from_count. - ✅ Code quality: Adds optional
finalization_evidenceparam withNonedefault — backwards compatible. - ✅ Session optimization:
lookup_session = None if pending_proposals is not None else session— avoids redundant DB lookups when data is pre-fetched. - ✅ Files: 6 files, +335/-6 — larger PR but well-structured across serializers, bounty_attempts, routes, tests, docs.
- ✅ Mergeable: state=clean.
No blockers. Approve.
3 participants
Related bounty and source report
Refs #799
Source report: #774
Summary
finalization_evidenceon public bounty row serialization when an executedcreate_bountyproposal has stored public GitHub finalization evidenceEvidence
Source report #774 identifies that public bounty rows expose status and capacity but not the public evidence that made a row live: the executed create-bounty proposal and claims-open
Reserved on MergeWorkcomment.The implementation reuses data already persisted in executed treasury proposal
result.github_issue_finalization. It does not crawl GitHub on read, mutate GitHub, change claim acceptance, change payout behavior, change treasury execution, or add private/admin state.Tests
/Users/dom/bounty-work/mergework/.venv/bin/python -m pytest tests/test_bounty_api_routes.py tests/test_serializers.py tests/test_docs_public_urls.py -q-> 70 passed, 1 warning/Users/dom/bounty-work/mergework/.venv/bin/python -m pytest -q-> 794 passed, 1 warning/Users/dom/bounty-work/mergework/.venv/bin/ruff check .-> all checks passed/Users/dom/bounty-work/mergework/.venv/bin/ruff format --check .-> 111 files already formatted/Users/dom/bounty-work/mergework/.venv/bin/python scripts/docs_smoke.py-> docs smoke ok/Users/dom/bounty-work/mergework/.venv/bin/mypy app-> success, no issues found in 42 source filesgit diff --check-> cleanOut of scope
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests