Tolerate invalid stored proof payloads#336
Closed
yui-stingray wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR consolidates proof payload JSON parsing from scattered ChangesProof payload decoding refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 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_activity.py`:
- Around line 115-164: The test currently only simulates malformed JSON by
setting proof_row.public_json = "{"; add a second malformed-proof case that sets
public_json to a valid non-object JSON (e.g., "[]", "null" or a JSON string) to
exercise the non-object payload skip-path; create a second pay_bounty call
(similar to malformed_proof) or reuse session.get(Proof, ...) to mutate a proof
row for that proof (reference test_activity_views_skip_malformed_proof_payloads,
pay_bounty, Proof, session_scope) and then assert the aggregate API
(/api/v1/activity and /activity) still excludes both malformed proofs and that
totals/recent remain unchanged (ensure the new malformed proof hash is not
present in payload["recent"] and not in page.text).
In `@tests/test_api_mcp.py`:
- Around line 1132-1174: The test
test_mcp_get_proof_rejects_malformed_stored_payload only checks a malformed JSON
string ("{"); update it to also cover non-object JSON stored payloads (e.g.,
proof_row.public_json = "[]" and/or proof_row.public_json = "\"string\"" ) and
assert the same JSON-RPC error response; you can either add a second client
request after setting proof_row.public_json to "[]" (reusing the same
request/response assertions) or parametrize the test to iterate over both
payloads, keeping the rest of the setup (ensure_genesis, create_bounty,
pay_bounty, TestClient call, and expected error) unchanged.
In `@tests/test_bounty_pages.py`:
- Around line 299-345: The test
test_bounty_detail_skips_malformed_award_proof_payloads only mutates a Proof to
malformed JSON "{"; add a second Proof mutated to a non-object JSON payload "[]"
to exercise the non-object payload skip path. In the test create a second
pay_bounty call (e.g., malformed_proof_nonobject), fetch it via
session.get(Proof, malformed_proof_nonobject.hash) and set proof_row.public_json
= "[]", then include assertions similar to the existing ones to ensure that
malformed_proof_nonobject.hash is not present in page.text and not included in
api_detail.json()["accepted_awards"].
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a2e04d61-265a-4325-a3db-ae7f2e6f4fc7
📒 Files selected for processing (5)
app/main.pyapp/serializers.pytests/test_activity.pytests/test_api_mcp.pytests/test_bounty_pages.py
Comment on lines
+115
to
+164
| def test_activity_views_skip_malformed_proof_payloads(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=12, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/12", | ||
| title="Activity malformed proof bounty", | ||
| reward_mrwk="25", | ||
| max_awards=2, | ||
| acceptance="Activity should skip malformed proof rows.", | ||
| ) | ||
| valid_proof = pay_bounty( | ||
| session, | ||
| bounty_id=bounty.id, | ||
| to_account="github:alice", | ||
| submission_url="https://github.com/ramimbo/mergework/pull/12", | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
| malformed_proof = pay_bounty( | ||
| session, | ||
| bounty_id=bounty.id, | ||
| to_account="github:bob", | ||
| submission_url="https://github.com/ramimbo/mergework/pull/13", | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
| proof_row = session.get(Proof, malformed_proof.hash) | ||
| assert proof_row is not None | ||
| proof_row.public_json = "{" | ||
|
|
||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
|
|
||
| response = client.get("/api/v1/activity") | ||
| page = client.get("/activity") | ||
|
|
||
| assert response.status_code == 200 | ||
| assert page.status_code == 200 | ||
| payload = response.json() | ||
| assert payload["totals"] == { | ||
| "accepted_awards": 1, | ||
| "accepted_mrwk": "25", | ||
| "contributors": 1, | ||
| } | ||
| assert [row["proof_hash"] for row in payload["recent"]] == [valid_proof.hash] | ||
| assert malformed_proof.hash not in page.text | ||
|
|
There was a problem hiding this comment.
Cover non-object payloads in the activity skip-path test.
This test only exercises malformed JSON ("{"). The changed behavior also includes non-object JSON payloads; add that case here to guard the aggregate activity path.
Suggested update
+import pytest
...
+@pytest.mark.parametrize("public_json", ["{", "[]"])
-def test_activity_views_skip_malformed_proof_payloads(sqlite_url: str) -> None:
+def test_activity_views_skip_malformed_proof_payloads(
+ sqlite_url: str, public_json: str
+) -> None:
...
- proof_row.public_json = "{"
+ proof_row.public_json = public_jsonAs per coding guidelines tests/**/*.py: Add or update tests for changed behavior.
🤖 Prompt for 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.
In `@tests/test_activity.py` around lines 115 - 164, The test currently only
simulates malformed JSON by setting proof_row.public_json = "{"; add a second
malformed-proof case that sets public_json to a valid non-object JSON (e.g.,
"[]", "null" or a JSON string) to exercise the non-object payload skip-path;
create a second pay_bounty call (similar to malformed_proof) or reuse
session.get(Proof, ...) to mutate a proof row for that proof (reference
test_activity_views_skip_malformed_proof_payloads, pay_bounty, Proof,
session_scope) and then assert the aggregate API (/api/v1/activity and
/activity) still excludes both malformed proofs and that totals/recent remain
unchanged (ensure the new malformed proof hash is not present in
payload["recent"] and not in page.text).
Comment on lines
+1132
to
+1174
| def test_mcp_get_proof_rejects_malformed_stored_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=46, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/46", | ||
| title="Malformed proof payload", | ||
| reward_mrwk="25", | ||
| acceptance="Proof payloads should be valid JSON.", | ||
| ) | ||
| proof = pay_bounty( | ||
| session, | ||
| bounty_id=bounty.id, | ||
| to_account="github:alice", | ||
| submission_url="https://github.com/ramimbo/mergework/pull/46", | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
| proof_row = session.get(Proof, proof.hash) | ||
| assert proof_row is not None | ||
| proof_row.public_json = "{" | ||
|
|
||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
|
|
||
| response = client.post( | ||
| "/mcp", | ||
| json={ | ||
| "jsonrpc": "2.0", | ||
| "id": 4, | ||
| "method": "tools/call", | ||
| "params": {"name": "get_proof", "arguments": {"hash": proof.hash}}, | ||
| }, | ||
| ) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert response.json() == { | ||
| "jsonrpc": "2.0", | ||
| "id": 4, | ||
| "error": {"code": -32602, "message": "invalid tool arguments"}, | ||
| } |
There was a problem hiding this comment.
Add non-object payload coverage to MCP get_proof invalid-stored-payload test.
The new test validates malformed JSON only. Please also cover non-object JSON (e.g., "[]") for this endpoint path.
Suggested update
+@pytest.mark.parametrize("public_json", ["{", "[]"])
-def test_mcp_get_proof_rejects_malformed_stored_payload(sqlite_url: str) -> None:
+def test_mcp_get_proof_rejects_malformed_stored_payload(
+ sqlite_url: str, public_json: str
+) -> None:
...
- proof_row.public_json = "{"
+ proof_row.public_json = public_jsonAs per coding guidelines tests/**/*.py: Add or update tests for changed behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_mcp_get_proof_rejects_malformed_stored_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=46, | |
| issue_url="https://github.com/ramimbo/mergework/issues/46", | |
| title="Malformed proof payload", | |
| reward_mrwk="25", | |
| acceptance="Proof payloads should be valid JSON.", | |
| ) | |
| proof = pay_bounty( | |
| session, | |
| bounty_id=bounty.id, | |
| to_account="github:alice", | |
| submission_url="https://github.com/ramimbo/mergework/pull/46", | |
| accepted_by="maintainer", | |
| verifier_result={"label": "mrwk:accepted"}, | |
| ) | |
| proof_row = session.get(Proof, proof.hash) | |
| assert proof_row is not None | |
| proof_row.public_json = "{" | |
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | |
| response = client.post( | |
| "/mcp", | |
| json={ | |
| "jsonrpc": "2.0", | |
| "id": 4, | |
| "method": "tools/call", | |
| "params": {"name": "get_proof", "arguments": {"hash": proof.hash}}, | |
| }, | |
| ) | |
| assert response.status_code == 200 | |
| assert response.json() == { | |
| "jsonrpc": "2.0", | |
| "id": 4, | |
| "error": {"code": -32602, "message": "invalid tool arguments"}, | |
| } | |
| `@pytest.mark.parametrize`("public_json", ["{", "[]"]) | |
| def test_mcp_get_proof_rejects_malformed_stored_payload( | |
| sqlite_url: str, public_json: 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=46, | |
| issue_url="https://github.com/ramimbo/mergework/issues/46", | |
| title="Malformed proof payload", | |
| reward_mrwk="25", | |
| acceptance="Proof payloads should be valid JSON.", | |
| ) | |
| proof = pay_bounty( | |
| session, | |
| bounty_id=bounty.id, | |
| to_account="github:alice", | |
| submission_url="https://github.com/ramimbo/mergework/pull/46", | |
| accepted_by="maintainer", | |
| verifier_result={"label": "mrwk:accepted"}, | |
| ) | |
| proof_row = session.get(Proof, proof.hash) | |
| assert proof_row is not None | |
| proof_row.public_json = public_json | |
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | |
| response = client.post( | |
| "/mcp", | |
| json={ | |
| "jsonrpc": "2.0", | |
| "id": 4, | |
| "method": "tools/call", | |
| "params": {"name": "get_proof", "arguments": {"hash": proof.hash}}, | |
| }, | |
| ) | |
| assert response.status_code == 200 | |
| assert response.json() == { | |
| "jsonrpc": "2.0", | |
| "id": 4, | |
| "error": {"code": -32602, "message": "invalid tool arguments"}, | |
| } |
🤖 Prompt for 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.
In `@tests/test_api_mcp.py` around lines 1132 - 1174, The test
test_mcp_get_proof_rejects_malformed_stored_payload only checks a malformed JSON
string ("{"); update it to also cover non-object JSON stored payloads (e.g.,
proof_row.public_json = "[]" and/or proof_row.public_json = "\"string\"" ) and
assert the same JSON-RPC error response; you can either add a second client
request after setting proof_row.public_json to "[]" (reusing the same
request/response assertions) or parametrize the test to iterate over both
payloads, keeping the rest of the setup (ensure_genesis, create_bounty,
pay_bounty, TestClient call, and expected error) unchanged.
Comment on lines
+299
to
+345
| def test_bounty_detail_skips_malformed_award_proof_payloads(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=165, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/165", | ||
| title="Malformed proof award history", | ||
| reward_mrwk="50", | ||
| max_awards=2, | ||
| acceptance="Bounty detail pages should skip malformed proof rows.", | ||
| ) | ||
| valid_proof = pay_bounty( | ||
| session, | ||
| bounty_id=bounty.id, | ||
| to_account="github:alice", | ||
| submission_url="https://github.com/ramimbo/mergework/pull/203", | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
| malformed_proof = pay_bounty( | ||
| session, | ||
| bounty_id=bounty.id, | ||
| to_account="github:bob", | ||
| submission_url="https://github.com/ramimbo/mergework/pull/204", | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
| proof_row = session.get(Proof, malformed_proof.hash) | ||
| assert proof_row is not None | ||
| proof_row.public_json = "{" | ||
| bounty_id = bounty.id | ||
|
|
||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
|
|
||
| api_detail = client.get(f"/api/v1/bounties/{bounty_id}") | ||
| page = client.get(f"/bounties/{bounty_id}") | ||
|
|
||
| assert api_detail.status_code == 200 | ||
| assert page.status_code == 200 | ||
| assert [award["proof_hash"] for award in api_detail.json()["accepted_awards"]] == [ | ||
| valid_proof.hash | ||
| ] | ||
| assert malformed_proof.hash not in page.text | ||
|
|
There was a problem hiding this comment.
Also exercise non-object payloads in bounty award-history skip behavior.
This new aggregate-view test mutates only to malformed JSON ("{"). Add a non-object payload case ("[]") to cover the full changed payload contract.
Suggested update
+@pytest.mark.parametrize("public_json", ["{", "[]"])
-def test_bounty_detail_skips_malformed_award_proof_payloads(sqlite_url: str) -> None:
+def test_bounty_detail_skips_malformed_award_proof_payloads(
+ sqlite_url: str, public_json: str
+) -> None:
...
- proof_row.public_json = "{"
+ proof_row.public_json = public_jsonAs per coding guidelines tests/**/*.py: Add or update tests for changed behavior.
🤖 Prompt for 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.
In `@tests/test_bounty_pages.py` around lines 299 - 345, The test
test_bounty_detail_skips_malformed_award_proof_payloads only mutates a Proof to
malformed JSON "{"; add a second Proof mutated to a non-object JSON payload "[]"
to exercise the non-object payload skip path. In the test create a second
pay_bounty call (e.g., malformed_proof_nonobject), fetch it via
session.get(Proof, malformed_proof_nonobject.hash) and set proof_row.public_json
= "[]", then include assertions similar to the existing ones to ensure that
malformed_proof_nonobject.hash is not present in page.text and not included in
api_detail.json()["accepted_awards"].
TateLyman
reviewed
May 26, 2026
Contributor
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Reviewed the invalid proof-payload handling in PR #336. The implementation change looks consistent: aggregate public views now go through the shared proof_public_payload / bounty_payment_payload helpers and skip bad stored proof rows, while direct proof lookup paths return controlled invalid-payload errors instead of leaking JSON decode failures.
One remaining coverage gap is worth fixing before merge: the aggregate skip-path tests only mutate stored public_json to malformed JSON ({), but the helper also intentionally treats valid non-object JSON, such as [], as invalid. Direct proof lookup covers both via test_proof_api_reports_invalid_stored_payload, but activity, bounty award history, and MCP get_proof should get the same non-object regression coverage so the full payload contract is locked down. This matches the three CodeRabbit comments I checked against the current diff.
Validation run locally on this PR head:
- uv run --extra dev python -m pytest tests/test_api_mcp.py::test_account_api_keeps_schema_when_accepted_work_proof_is_malformed tests/test_api_mcp.py::test_mcp_get_proof_rejects_malformed_stored_payload tests/test_activity.py::test_activity_views_skip_malformed_proof_payloads tests/test_bounty_pages.py::test_bounty_detail_skips_malformed_award_proof_payloads tests/test_bounty_pages.py::test_proof_api_reports_invalid_stored_payload -q -> 6 passed
- uv run --extra dev python -m pytest tests/test_api_mcp.py tests/test_activity.py tests/test_bounty_pages.py -q -> 87 passed
- uv run --extra dev python -m pytest tests/test_api_mcp.py tests/test_activity.py tests/test_bounty_pages.py tests/test_serializers.py -q -> 89 passed
- uv run --extra dev ruff check app/main.py app/serializers.py tests/test_activity.py tests/test_api_mcp.py tests/test_bounty_pages.py -> passed
- uv run --extra dev ruff format --check app/main.py app/serializers.py tests/test_activity.py tests/test_api_mcp.py tests/test_bounty_pages.py -> passed
- uv run --extra dev python -m mypy app/main.py app/serializers.py -> success
- uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
- git diff --check origin/main...HEAD -> clean
No secrets, wallet keys, payout credentials, deployment values, private vulnerability details, or MRWK price/cash-out claims were reviewed or exposed.
Owner
|
Maintainer update: holding this with mrwk:needs-info. The behavior is useful, but before merge please cover the non-object stored proof payload path in the aggregate activity/bounty views and MCP/direct proof paths called out in review: #336 (review). No PR bounty payout is recorded while it is held. |
Owner
|
Closing this cleanup item now that #292 is filled. This PR was already held with mrwk:needs-info, and no fresh small-fixes round is being opened from this pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_proof.Repro before fix
A stored bounty payment
Proof.public_jsonvalue like{caused public aggregate views that decode proof rows to raise a server error instead of rendering the remaining valid accepted work. Non-object JSON payloads were also not covered explicitly.Validation
uv run --extra dev python -m pytest tests/test_api_mcp.py::test_account_api_keeps_schema_when_accepted_work_proof_is_malformed tests/test_api_mcp.py::test_mcp_get_proof_rejects_malformed_stored_payload tests/test_activity.py::test_activity_views_skip_malformed_proof_payloads tests/test_bounty_pages.py::test_bounty_detail_skips_malformed_award_proof_payloads tests/test_bounty_pages.py::test_proof_api_reports_invalid_stored_payload -q --capture=no-> 6 passeduv run --extra dev python -m pytest tests/test_api_mcp.py tests/test_activity.py tests/test_bounty_pages.py -q --capture=no-> 87 passeduv run --extra dev ruff check app/main.py app/serializers.py tests/test_api_mcp.py tests/test_activity.py tests/test_bounty_pages.py-> passeduv run --extra dev ruff format --check app/main.py app/serializers.py tests/test_api_mcp.py tests/test_activity.py tests/test_bounty_pages.py-> passeduv run --extra dev python -m mypy app-> successuv run --extra dev python -m pytest -q --capture=no-> 323 passedgit diff --check-> passedBounty
Refs #292
Notes:
app/serializers.py, but only partially overlaps activity/account hardening.No secrets, wallet private keys, payout credentials, deployment values, private vulnerability details, or MRWK price claims are included.
Summary by CodeRabbit
Bug Fixes
Refactor