Extract ledger view helpers#366
Merged
ramimbo merged 3 commits intoMay 26, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR extracts ledger query and serialization logic into a new dedicated module, refactors main.py endpoints and pages to use reusable helpers, hardens OAuth redirect validation with URL decoding, and adds comprehensive test coverage for the changes. ChangesLedger Views Refactoring & OAuth Enhancements
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 centralizes ledger view serialization (including proof-hash attachment) into reusable helpers, updates API/page endpoints to use them, and adds tests to validate correct behavior.
Changes:
- Added
app/ledger_views.pyhelpers for recent entries, per-entry lookup, and account transaction lists with consistentproof_hashhandling. - Refactored
app/main.pyledger endpoints/pages to use the new helpers and tightened_safe_next_pathagainst encoded path tricks. - Added focused tests covering ledger view helpers (proof hash attachment + account-side filtering).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_ledger_views.py | Adds tests validating proof-hash attachment and account transaction filtering behavior. |
| app/main.py | Replaces inline ledger/proof query logic with ledger_views helpers and improves _safe_next_path decoding checks. |
| app/ledger_views.py | Introduces centralized query/serialization helpers for ledger-related views. |
| app/ledger/service.py | Simplifies SQLAlchemy update execution by removing cast(...) usage around update results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
276
to
290
| def _safe_next_path(next_path: str | None) -> str: | ||
| decoded_next_path = unquote(next_path) if next_path else "" | ||
| if ( | ||
| not next_path | ||
| or not next_path.startswith("/") | ||
| or next_path.startswith("//") | ||
| or len(next_path) > 2048 | ||
| or "\\" in next_path | ||
| or decoded_next_path.startswith("//") | ||
| or "\\" in decoded_next_path | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in next_path) | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in decoded_next_path) | ||
| ): | ||
| return "/me" | ||
| return next_path |
Comment on lines
659
to
671
| claimed = 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: | ||
| raise LedgerError("bounty already paid") |
| detail = ledger_entry_to_dict(session, proof.ledger_sequence) | ||
| missing = ledger_entry_to_dict(session, 9999) | ||
|
|
||
| payment_row = next(row for row in rows if row["sequence"] == proof.ledger_sequence) |
liangtovi-debug
suggested changes
May 26, 2026
liangtovi-debug
left a comment
liangtovi-debug
left a comment
There was a problem hiding this comment.
Thanks for the focused extraction. I found one merge-blocking issue: the PR removes the CursorResult casts around the two SQLAlchemy update() calls in app/ledger/service.py, but the code still reads claimed.rowcount afterward. Under this project's strict mypy settings, session.execute(update(...)) is inferred as Result[Any], which does not expose rowcount, so the hosted quality check is failing.
Repro from PR head 9935752:
uv run --extra dev python -m mypy app/ledger/service.py app/main.py app/ledger_views.py
app/ledger/service.py:670: error: "Result[Any]" has no attribute "rowcount" [attr-defined]
app/ledger/service.py:741: error: "Result[Any]" has no attribute "rowcount" [attr-defined]
Found 2 errors in 1 file (checked 3 source files)
The focused behavior checks pass (tests/test_ledger_views.py, the MCP ledger proof hash test, the bounty proof page test, and the OAuth next-path regression: 5 passed), and ruff check/format pass for touched files. Restoring the typed CursorResult[Any] handling, or another typed equivalent, should unblock the PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ledger/service.py`:
- Around line 736-741: The mypy error is caused by session.execute(...) being
inferred as Result[Any] which lacks rowcount; restore a precise type so claimed
exposes rowcount. Wrap or annotate the call to session.execute in
app/ledger/service.py (the expression assigning claimed from
session.execute(update(Bounty)...)) with a cast/annotation to
sqlalchemy.engine.Result (e.g., cast(Result, session.execute(...)) or using a
typed Session.execute return like Session.execute[...]) and ensure you import
typing.cast and sqlalchemy.engine.Result so mypy recognizes claimed.rowcount.
- Around line 659-670: The mypy failure comes from losing the typed Result that
exposes rowcount when removing the cast around session.execute; restore a proper
type annotation/cast so `claimed` is typed as SQLAlchemy Result (the same typed
Result used previously) before checking `claimed.rowcount` in the update(Bounty)
block, i.e. reintroduce the cast/annotation around the call to
session.execute(...) that assigns to `claimed` so mypy knows `claimed` has a
`rowcount` attribute.
🪄 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: 93633b00-3b0a-4fe2-9ffa-3c24b98aa1ea
📒 Files selected for processing (4)
app/ledger/service.pyapp/ledger_views.pyapp/main.pytests/test_ledger_views.py
Contributor
Author
|
Follow-up pushed in What changed:
Validation:
|
Contributor
Author
|
Follow-up pushed in What changed:
Validation:
|
Contributor
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers from my review of current head 715503f.
Evidence checked:
- Inspected the
app.ledger_viewsextraction and theapp.maincall sites for/api/v1/ledger,/api/v1/ledger/{sequence}, account/wallet pages, and the MCPget_ledger_entrypath. - Verified proof hashes are still attached through a single helper path, missing ledger entries still return
Nonebefore the API turns them into 404s, and account transactions still include both sender and recipient entries in descending sequence order. - Confirmed the current diff no longer includes the earlier
app/ledger/service.pyrowcount typing issue mentioned by another review.
Validation run locally on this PR head:
./.venv/bin/python -m pytest tests/test_ledger_views.py tests/test_wallet_api.py tests/test_api_mcp.py -q— 109 passed./.venv/bin/python -m ruff check app/ledger_views.py app/main.py tests/test_ledger_views.py tests/test_wallet_api.py— passed./.venv/bin/python -m ruff format --check app/ledger_views.py app/main.py tests/test_ledger_views.py tests/test_wallet_api.py— passed./.venv/bin/python -m mypy app— passedgit diff --check origin/main...HEAD— passed
Owner
|
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. |
newmattock
approved these changes
May 26, 2026
Contributor
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
No blockers from my pass on current head 715503f.
Evidence checked:
- Inspected
app/ledger_views.py, the route call sites inapp/main.py, andtests/test_ledger_views.py. - Verified
recent_ledger_entries()andledger_entry_to_dict()share the same proof-hash attachment path, so/api/v1/ledger,/api/v1/ledger/{sequence},/ledger, and MCPget_ledger_entrykeep bounty-payment proof hashes consistent. - Verified missing ledger entries still return
Nonefrom the helper before the API converts that to the existing 404 behavior. - Verified
account_ledger_transactions()still matches bothfrom_accountandto_account, orders newest-first, honors the default account/wallet page limit, and leaves non-proof ledger rows withproof_hash: None. - Rechecked that the earlier
app/ledger/service.pyrowcount typing regression is no longer present in the current diff.
Validation:
uv run --extra dev python -m pytest tests/test_ledger_views.py tests/test_api_mcp.py::test_mcp_get_ledger_entry_includes_payment_proof_hash tests/test_bounty_pages.py::test_ledger_and_proof_pages_make_bounty_payments_scannable tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows -q-> 5 passed.uv run --extra dev python -m mypy app/ledger_views.py app/main.py-> success.uv run --extra dev python -m ruff check app/ledger_views.py app/main.py tests/test_ledger_views.py tests/test_wallet_api.py-> passed.uv run --extra dev python -m ruff format --check app/ledger_views.py app/main.py tests/test_ledger_views.py tests/test_wallet_api.py-> 4 files already formatted.git diff --check origin/main...HEAD-> clean.
ramimbo
added
mrwk:accepted
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
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.
Refs #320
Summary
app/ledger_views.py.get_ledger_entrypath soapp/main.pyno longer owns duplicate ledger/proof query shaping.CursorResulttyping for rowcount checks and by sanitizing percent-decoded OAuthnextpaths covered by the existing OAuth regression test.Complexity reduced
Ledger display/API/MCP code now shares one query boundary for proof-backed ledger rows. Route handlers stay focused on HTTP status, auth, and template selection, while transaction lookup and proof hash attachment are directly testable outside
app/main.py.Tests
python -m pytest -q-> 337 passed, 1 Windows asyncio warningpython -m pytest tests/test_ledger_views.py tests/test_api_mcp.py::test_mcp_get_ledger_entry_includes_payment_proof_hash tests/test_bounty_pages.py::test_ledger_and_proof_pages_make_bounty_payments_scannable tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 5 passedpython -m ruff format --check app/main.py app/ledger/service.py app/ledger_views.py tests/test_ledger_views.pypython -m ruff check app/main.py app/ledger/service.py app/ledger_views.py tests/test_ledger_views.pypython -m mypy appSummary by CodeRabbit
Bug Fixes
Tests