Extract proof helpers from app.main#361
Conversation
|
Warning Review limit reached
More reviews will be available in 4 minutes and 48 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 (2)
📝 WalkthroughWalkthroughThis PR centralizes proof-hash validation, lookup, and payload serialization logic from ChangesProof utilities centralization
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 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 |
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 “proof” lookup/serialization helpers into a dedicated module and updates API/tooling code to reuse them, adding targeted tests for the new helpers.
Changes:
- Added
app/proofs.pywith proof-hash parsing, DB lookup helpers, and payload serialization helpers. - Rewired existing endpoints/tool handlers in
app/main.pyto call the new helpers and removed the inlined implementations. - Added
tests/test_proofs.pyto validate hash normalization/rejection and payload shapes for API vs MCP.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_proofs.py | Adds tests covering proof-hash parsing and helper outputs for API/MCP payloads. |
| app/proofs.py | Introduces reusable proof parsing/lookup/serialization helpers. |
| app/main.py | Replaces local proof helpers with imports from app.proofs across endpoints and tool handlers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def public_proof_payload(proof: Proof) -> dict[str, Any]: | ||
| payload = json.loads(proof.public_json) | ||
| if not isinstance(payload, dict): | ||
| raise HTTPException(status_code=500, detail="invalid proof payload") | ||
| return payload | ||
|
|
||
|
|
||
| def mcp_proof_to_dict(proof: Proof) -> dict[str, Any]: | ||
| return { | ||
| "hash": proof.hash, | ||
| "kind": proof.kind, | ||
| "ledger_sequence": proof.ledger_sequence, | ||
| "bounty_id": proof.bounty_id, | ||
| "submission_id": proof.submission_id, | ||
| "created_at": proof.created_at.isoformat(), | ||
| "proof": public_proof_payload(proof), | ||
| } |
|
|
||
| proof_by_sequence = proof_hashes_by_sequence(session, [proof.ledger_sequence, 999]) | ||
| single_proof_hash = proof_hash_for_sequence(session, proof.ledger_sequence) | ||
| missing_proof_hash = proof_hash_for_sequence(session, 999) | ||
| api_payload = public_proof_payload(proof) | ||
| mcp_payload = mcp_proof_to_dict(proof) | ||
| empty_lookup = proof_hashes_by_sequence(session, []) | ||
|
|
||
| assert empty_lookup == {} | ||
| assert proof_by_sequence == {proof.ledger_sequence: proof.hash} | ||
| assert single_proof_hash == proof.hash | ||
| assert missing_proof_hash is None | ||
| assert api_payload["kind"] == "bounty_payment" | ||
| assert api_payload["submission_url"] == "https://github.com/ramimbo/mergework/pull/320" | ||
| assert mcp_payload["hash"] == proof.hash |
|
|
||
| proof_by_sequence = proof_hashes_by_sequence(session, [proof.ledger_sequence, 999]) | ||
| single_proof_hash = proof_hash_for_sequence(session, proof.ledger_sequence) | ||
| missing_proof_hash = proof_hash_for_sequence(session, 999) | ||
| api_payload = public_proof_payload(proof) | ||
| mcp_payload = mcp_proof_to_dict(proof) | ||
| empty_lookup = proof_hashes_by_sequence(session, []) | ||
|
|
||
| assert empty_lookup == {} | ||
| assert proof_by_sequence == {proof.ledger_sequence: proof.hash} | ||
| assert single_proof_hash == proof.hash | ||
| assert missing_proof_hash is None | ||
| assert api_payload["kind"] == "bounty_payment" | ||
| assert api_payload["submission_url"] == "https://github.com/ramimbo/mergework/pull/320" | ||
| assert mcp_payload["hash"] == proof.hash |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_proofs.py (1)
27-64: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd negative-path tests for malformed/non-object proof payloads.
The new helper behavior includes invalid-payload handling, but this test only covers happy-path payload shaping. Please add assertions for malformed JSON and non-dict JSON to lock in the error contract.
Suggested test addition
+from types import SimpleNamespace @@ def test_proof_helpers_shape_api_and_mcp_payloads(sqlite_url: str) -> None: @@ assert mcp_payload["hash"] == proof.hash assert mcp_payload["proof"] == api_payload + + +def test_public_proof_payload_rejects_malformed_or_non_object_payloads() -> None: + for raw_payload in ("{", "[]"): + proof = SimpleNamespace(public_json=raw_payload) + with pytest.raises(HTTPException) as exc_info: + public_proof_payload(proof) # type: ignore[arg-type] + assert exc_info.value.status_code == 500 + assert exc_info.value.detail == "invalid proof payload"As 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_proofs.py` around lines 27 - 64, The test only covers happy-path shaping for public_proof_payload and mcp_proof_to_dict; add negative-path assertions to lock the contract when proof.payload is malformed or not a JSON object. In test_proof_helpers_shape_api_and_mcp_payloads, create additional proof variants (or modify the existing proof before calling public_proof_payload and mcp_proof_to_dict) where proof.payload is invalid JSON (e.g., a broken string) and where proof.payload parses to a non-dict (e.g., JSON array or string); assert that public_proof_payload and mcp_proof_to_dict either return a safe default (empty dict) or raise the documented exception per the helper behavior, and also add corresponding checks for proof_hashes_by_sequence and proof_hash_for_sequence when payloads are bad to ensure they still map/return as expected. Ensure you reference the functions public_proof_payload, mcp_proof_to_dict, proof_hashes_by_sequence, and proof_hash_for_sequence in the new assertions.
🤖 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 `@app/proofs.py`:
- Around line 39-41: Wrap the json.loads(proof.public_json) call in a try/except
that catches json.JSONDecodeError and converts it into the same
HTTPException(detail="invalid proof payload") so malformed JSON doesn't
propagate as a raw JSONDecodeError; update the code around the
public_proof_payload handling in app/proofs.py (the payload =
json.loads(proof.public_json) location) to catch json.JSONDecodeError and raise
HTTPException(status_code=500, detail="invalid proof payload").
---
Outside diff comments:
In `@tests/test_proofs.py`:
- Around line 27-64: The test only covers happy-path shaping for
public_proof_payload and mcp_proof_to_dict; add negative-path assertions to lock
the contract when proof.payload is malformed or not a JSON object. In
test_proof_helpers_shape_api_and_mcp_payloads, create additional proof variants
(or modify the existing proof before calling public_proof_payload and
mcp_proof_to_dict) where proof.payload is invalid JSON (e.g., a broken string)
and where proof.payload parses to a non-dict (e.g., JSON array or string);
assert that public_proof_payload and mcp_proof_to_dict either return a safe
default (empty dict) or raise the documented exception per the helper behavior,
and also add corresponding checks for proof_hashes_by_sequence and
proof_hash_for_sequence when payloads are bad to ensure they still map/return as
expected. Ensure you reference the functions public_proof_payload,
mcp_proof_to_dict, proof_hashes_by_sequence, and proof_hash_for_sequence in the
new assertions.
🪄 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: 273f1cc0-491b-4473-bfb2-113d0582d221
📒 Files selected for processing (3)
app/main.pyapp/proofs.pytests/test_proofs.py
Karry2019web
left a comment
Karry2019web
left a comment
There was a problem hiding this comment.
Review ✅ APPROVED
FILES INSPECTED:
app/main.py— removed_proof_hashes_by_sequence,_proof_hash_from_path,HEX_HASH_RE; replaced inline proof-payload parse withpublic_proof_payload()call; replaced raw proof lookup withproof_hash_for_sequence()app/proofs.py— new module (54 lines) containingproof_hash_from_path,proof_hashes_by_sequence,proof_hash_for_sequence,public_proof_payload,mcp_proof_to_dicttests/test_proofs.py— new test file (85 lines) withtest_proof_hash_from_path_normalizes_and_rejects_malformed_hashesandtest_proof_helpers_shape_api_and_mcp_payloads
BEHAVIOR CHECKED:
proof_hash_from_path: preserves the exact same validation logic (strip check → lowercase → HEX_HASH_RE match → 400 on failure) that was in the original_proof_hash_from_pathin main.py. Regex constant was renamed fromHEX_HASH_REtoPROOF_HASH_REwithout behavior change.proof_hashes_by_sequence: identical logic to the original_proof_hashes_by_sequence— early return on empty, same SQLAlchemy query, same dict comprehension.proof_hash_for_sequence: new helper — a cleansession.scalar(select(Proof.hash).where(Proof.ledger_sequence == sequence).limit(1))replacing the inlinesession.get(Proof, sequence)+.hashaccessor pattern. Semantically equivalent.public_proof_payload: extracts the json.loads + type guard that was inlined inapi_proof()into a focused function. Safe.mcp_proof_to_dict: new serialization helper for MCP use. No callers changed in this PR — ensures safe reuse.- All call sites in
main.py(api_ledger, api_ledger_entry, api_proof, account_page, wallet_page) now reference the module functions instead of private helpers.
TESTS VERIFIED:
test_proof_hash_from_path_normalizes_and_rejects_malformed_hashes: Covers uppercase → lowercase normalization, whitespace-prefix rejection, non-hex rejection, hex-g (invalid) rejection. All with proper HTTPException(400) assertions.test_proof_helpers_shape_api_and_mcp_payloads: Integration-style test that creates a real bounty + payout, then exercises all five exported functions. Verifies: empty sequence lookup returns{}, hashes-by-sequence returns correct mapping, single-proof lookup returns correct hash, missing sequence returnsNone, API payload has correctkind/submission_url, MCP payload embeds the full API payload. Supply conservation is implicit (no access to balance here — the test scope is strictly the proof helpers).- Both tests are deterministic and self-contained (SQLite-based via
sqlite_urlfixture). - No
time.sleepor flaky patterns.
CODE QUALITY:
- Clean extraction: the removed code from main.py is exactly the logic moved to proofs.py with no behavioral changes.
- The
from __future__ import annotationsat the top of proofs.py is consistent with the repo's convention (check: does main.py use it? yes, it's in scope viaannotationsimport). - No new dependencies beyond
app.models.Proofand stdlibre/json. - The
proof_hash_for_sequencereturnsstr | None, which matches the new call site inapi_ledger_entry:ledger_to_dict(entry, proof_hash_for_sequence(...)). The original inline code returnedNonewhen no proof existed, so this is type-equivalent. - No whitespace churn in main.py — only the targeted removals, import addition, and 6 call-site changes.
SUMMARY: Clean, well-scoped extraction with thorough tests. The mcp_proof_to_dict function is purely additive (no existing caller changed) and follows the established proof-serialization pattern. APPROVED — no blockers.
|
Follow-up pushed in What changed:
Validation:
|
ayskobtw-lil
left a comment
ayskobtw-lil
left a comment
There was a problem hiding this comment.
Follow-up review after the proof-payload fixes on current head 5f6fa53.
Evidence checked:
- Rechecked the branch after CodeRabbit requested malformed/non-object proof payload coverage and the author pushed the follow-up commits.
- Inspected
app/proofs.py,app/main.py, andtests/test_proofs.py; verifiedpublic_proof_payload()now converts malformed JSON and non-object JSON into the stableHTTPException(status_code=500, detail="invalid proof payload")path. - Verified
mcp_proof_to_dict()is covered for the same invalid payload cases and that proof hash lookup helpers still return hashes independently of payload validity. - Confirmed API/MCP proof call sites continue to use the extracted helpers rather than duplicating parsing behavior.
Validation:
python -m pytest tests/test_proofs.py tests/test_api_mcp.py::test_mcp_get_ledger_entry_includes_payment_proof_hash tests/test_api_mcp.py::test_mcp_get_proof_returns_public_proof_details -q-> 5 passed.python -m pytest tests/test_proofs.py tests/test_api_mcp.py -q-> 79 passed.python -m ruff check app/proofs.py app/main.py tests/test_proofs.py tests/test_api_mcp.py-> passed.python -m ruff format --check app/proofs.py app/main.py tests/test_proofs.py-> 3 files already formatted.python -m mypy app/proofs.py app/main.py-> success.git diff --check origin/main...HEAD-> clean.- Hosted Quality/readiness/docs/image check is green and GitHub reports the PR mergeable.
The earlier invalid proof payload review item is resolved; I do not see remaining blockers from this follow-up pass.
|
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. |
|
Closing this stale bounty submission. It still references a closed/filled bounty round and is dirty or unstable against current main after the route/test extraction batches. A future attempt needs a fresh PR against current main, a currently open bounty reference, and scope distinct from work already accepted. |
Refs #320
Summary
app/main.pyintoapp/proofs.py./api/v1/proofs/{hash}, and MCPget_ledger_entry/get_proofwithout changing public behavior.Complexity reduced
app/main.pyno longer owns proof-specific hash parsing, ledger-sequence proof lookup, or MCP proof serialization. Proof behavior can now be reviewed in one small module with direct tests, while route code stays focused on wiring.Tests
python -m pytest-> 337 passedpython -m pytest tests/test_proofs.py tests/test_api_mcp.py::test_mcp_get_ledger_entry_includes_payment_proof_hash tests/test_api_mcp.py::test_mcp_get_proof_returns_public_proof_details tests/test_api_mcp.py::test_explorer_links_ledger_proof_and_account-> 5 passedpython -m ruff format --check .-> 50 files already formattedpython -m ruff check .-> All checks passedpython -m mypy app-> Success: no issues found in 15 source filesgit diff --check-> cleanQuality, readiness, docs, and image checks-> passedNo private keys, seed material, secrets, deployment credentials, private vulnerability details, payout credentials, or MRWK price claims are included.