Extract account helpers from app.main#359
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 32 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 (3)
✨ 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 account parsing/validation and account-related response shaping into a dedicated app.accounts module, then updates the API to use those helpers while adding unit tests for the new behavior.
Changes:
- Added
app/accounts.pywithnormalize_account,github_login_from_account,transfer_status_for_account, andaccount_summary. - Refactored
/api/v1/accounts/{account}and related helpers inapp/main.pyto use the new accounts module. - Added pytest coverage for normalization and summary shaping in
tests/test_accounts.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_accounts.py | Adds tests for account normalization and API summary shaping. |
| app/main.py | Replaces inline account parsing/summary logic with app.accounts helpers and hardens next_path decoding checks. |
| app/accounts.py | Introduces a single source of truth for account normalization and summary metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
| def _normalize_reserve_account(lower_account: str) -> str: | ||
| reserve_prefix = "reserve:bounty:" | ||
| if not lower_account.startswith(reserve_prefix): | ||
| raise AccountError("reserve account must use reserve:bounty:<id>") | ||
| bounty_id = lower_account.removeprefix(reserve_prefix) | ||
| try: | ||
| normalized_bounty_id = int(bounty_id) if bounty_id.isdigit() else 0 | ||
| except ValueError as exc: | ||
| raise AccountError("reserve bounty id is too large") from exc | ||
| if normalized_bounty_id <= 0: | ||
| raise AccountError("reserve bounty id must be positive") | ||
| if normalized_bounty_id > SQLITE_INTEGER_MAX: | ||
| raise AccountError("reserve bounty id is too large") | ||
| return f"{reserve_prefix}{normalized_bounty_id}" |
| if not account.startswith("github:"): | ||
| return None | ||
| login = account.removeprefix("github:") |
| if normalized_bounty_id > SQLITE_INTEGER_MAX: | ||
| raise AccountError("reserve bounty id is too large") |
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers found on PR #359 at head 14ae298.
Evidence checked:
- Inspected the account-helper extraction from
app/main.pyintoapp/accounts.py;_normalized_account()still maps helperAccountErrorfailures to HTTP 400, and canonicalization of treasury, reserve, GitHub, and mrwk1 wallet accounts is preserved. - Verified
account_summary()preserves the existing/api/v1/accounts/{account}response shape, includingledger_address, GitHub login extraction for normalizedgithub:accounts, balance string, transfer status text, and accepted-work payload. - Checked the OAuth
nextpath hardening added inapp/main.py; percent-decoded//and backslash paths now fall back to/mewhile normal relative paths remain allowed. - Reviewed the Copilot comments: the raw-case
github_login_from_account()concern is not a blocker because public account callers normalize before summary construction, and the reserve upper bound is enforced bySQLITE_INTEGER_MAXeven though an extra explicit unit vector could further document it.
Commands run locally:
./.venv/bin/python -m pytest tests/test_accounts.py tests/test_wallet_api.py -q→ 42 passed./.venv/bin/python -m ruff check app/accounts.py app/main.py tests/test_accounts.py→ passed./.venv/bin/python -m ruff format --check app/accounts.py app/main.py tests/test_accounts.py→ passed./.venv/bin/python -m mypy app→ passedgit diff --check origin/main...HEAD→ passed
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
No blockers found on current head 14ae298.
Evidence checked:
- Inspected
app/accounts.py, account route delegation inapp/main.py, andtests/test_accounts.py. - Verified
normalize_account()preserves the old account boundary behavior for treasury, reserve bounty, GitHub, wallet, and unknown ledger account strings, including lower-casing/canonicalizing known account types and surfacing invalid values through the existing HTTP 400 wrapper. - Verified
account_summary()preserves the/api/v1/accounts/{account}payload shape: ledger address, GitHub login extraction, existence flag, formatted balance, transfer status text, and accepted-work summary. - Rechecked the branch's encoded OAuth
nexthardening so decoded backslash/redirect-like payloads fall back to/mewhile normal relative paths remain allowed.
Validation run locally:
uv run --extra dev python -m pytest tests/test_accounts.py tests/test_api_mcp.py::test_account_api_reports_internal_ledger_account_transfer_status tests/test_security.py::test_oauth_next_path_rejects_external_or_headerlike_paths tests/test_wallet_api.py::test_oauth_next_path_rejects_redirect_ambiguity tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 28 passed.uv run --extra dev python -m mypy app/accounts.py app/main.py-> success.uv run --extra dev python -m ruff check app/accounts.py app/main.py tests/test_accounts.py-> passed.uv run --extra dev python -m ruff format --check app/accounts.py app/main.py tests/test_accounts.py-> 3 files already formatted.git diff --check origin/main...HEAD-> clean.
|
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
Complexity reduced
Tests