Extract accepted activity helpers#328
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR hardens proof JSON parsing in activity and accepted-work serializers by centralizing validation into a new ChangesProof payload validation hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 |
shootingallday
left a comment
shootingallday
left a comment
There was a problem hiding this comment.
No blockers from my review on current head 5132409b3f0a856cd5f184f27fb59b5dc1133210.
Evidence checked:
- Inspected
app/activity.py, the correspondingapp/main.pyimports/call sites, andtests/test_activity_helpers.py. - Compared the moved activity/account accepted-work helper logic against
origin/main; the row shaping, query filtering, contributor aggregation, account summary, accepted-work list, and fallback helpers appear preserved behind the newapp.activitymodule boundary. - Verified the route layer still calls the same helper behavior for
/api/v1/activity, account API accepted-work summaries, account accepted-work details, and the account page fallback paths.
Validation run locally:
uv run --extra dev python -m pytest tests/test_activity_helpers.py tests/test_activity.py tests/test_api_mcp.py -q-> 72 passed.uv run --extra dev ruff check app/activity.py app/main.py tests/test_activity_helpers.py-> passed.uv run --extra dev ruff format --check app/activity.py app/main.py tests/test_activity_helpers.py-> passed.uv run --extra dev python -m mypy app-> success.python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.
Residual risk is limited to the existing broad except Exception fallback behavior, which this PR moves unchanged rather than expanding.
![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/activity.py`:
- Around line 17-20: Wrap the json.loads(proof.public_json) call in
_activity_row in a try/except that catches json.JSONDecodeError and TypeError
(and any safe parsing errors), and defensively return None if parsing fails or
the result is not a dict; apply the same defensive parsing pattern to the other
helper that decodes proof.public_json (the helper around the other json.loads
usage) so malformed proof.public_json values are skipped instead of raising.
In `@tests/test_activity_helpers.py`:
- Around line 11-20: Add a DB-failure assertion for
safe_account_accepted_summary: in the
test_activity_helper_fallbacks_keep_account_schema function, after the existing
assertions, call safe_account_accepted_summary(BrokenSession(), "github:alice")
and assert it equals empty_accepted_summary(); this mirrors the existing
safe_accepted_work_for_account(BrokenSession(), ...) check and verifies the
fallback schema for safe_account_accepted_summary.
🪄 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: dfac8525-c585-4648-a5a7-ace7f9dec52d
📒 Files selected for processing (3)
app/activity.pyapp/main.pytests/test_activity_helpers.py
|
Fixed the actionable review feedback in commit 2da5b66. Changed:
Validation:
|
|
Holding this for a rebase/retarget after merged PR #330. The accepted serializer extraction now owns the overlapping accepted-work serialization boundary in app/serializers.py, so this needs an updated diff that avoids duplicating or moving the same helpers. |
…elpers # Conflicts: # app/main.py
|
Resolved the merge conflict after #329/#330 landed on What changed in the conflict resolution:
Verification after the merge:
|
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers found.
Evidence checked on head ee15d046933272d5d645a5c2b5f9d16a3b7ea15d:
- The new
_proof_payload()helper centralizes bounty-payment proof JSON parsing and returnsNonefor malformed/non-bounty payloads before both activity-feed and account accepted-work serializers use the data. _activity_row()now reuses the same payload guard asaccepted_work_for_account(), so a malformedProof.public_jsonrow is skipped consistently instead of breaking/activityor account accepted-work surfaces.- The regression mutates a real paid proof row to malformed JSON and verifies all three public serializer paths preserve stable empty shapes:
activity_to_dict(),account_accepted_summary(), andaccepted_work_for_account(). - The safe wrapper fallback test still confirms database failures return the documented empty accepted-work shapes.
Validation I ran locally:
./.venv/bin/python -m pytest tests/test_serializers.py -q
# 4 passed
./.venv/bin/python -m ruff check app/serializers.py tests/test_serializers.py
# All checks passed
./.venv/bin/python -m mypy app
# Success: no issues found in 13 source files
git diff --check origin/main...HEAD
# clean
This is a focused code-health hardening of accepted-work/activity serialization with no public shape changes beyond avoiding malformed-proof crashes.
Karry2019web
left a comment
Karry2019web
left a comment
There was a problem hiding this comment.
PR #328 Review — Extract accepted activity helpers
Scope: Inline code health improvements in app/serializers.py + test coverage for fallback behavior.
Files Inspected
| File | Lines | Role |
|---|---|---|
app/serializers.py |
127-316, proposed diff +14/-4 | Central change: extract _proof_payload() helper, use it in _activity_row() and accepted_work_for_account() |
app/main.py |
1092-1095 (api_activity route) |
Verifies the route calls activity_to_dict(session, q) — no change needed |
app/activity.py |
Not in main branch — PR claims extraction target | But the actual diff only touches serializers.py. The app/activity.py extraction referenced in the PR body does not appear in the diff. |
tests/test_serializers.py |
+56/-1 diff | New tests for BrokenSession fallback and malformed proof skipping |
tests/test_activity.py |
223 lines (existing) | Existing route-level tests — confirms activity_to_dict behavior is unchanged |
Correctness Analysis
1. _proof_payload() consolidation — correct
The original code in _activity_row():
data = json.loads(proof.public_json)
if not isinstance(data, dict) or data.get("kind") != "bounty_payment":
return NoneAnd in accepted_work_for_account():
data = json.loads(proof.public_json)
if not isinstance(data, dict) or data.get("kind") != "bounty_payment":
continueThe new _proof_payload() wraps both calls:
def _proof_payload(proof: Proof) -> dict[str, Any] | None:
try:
data = json.loads(proof.public_json)
except (TypeError, json.JSONDecodeError):
return None
if not isinstance(data, dict) or data.get("kind") != "bounty_payment":
return None
return data✅ The behavior is preserved with an improvement: TypeError and JSONDecodeError are caught (the original code would crash on None or malformed public_json).
2. _activity_row() behavior — identical output where input is valid
The call sites use _proof_payload(proof) and check if data is None: return None — same logic as if data is None: continue in the second call site. Output shape is preserved.
3. Tests — adequate for the change
test_activity_serializer_fallbacks_keep_account_schema: Uses aBrokenSessionthat raises on any database access — verifiessafe_*wrappers return empty fallback shapes. ✅test_activity_serializers_skip_malformed_public_proofs: Creates a real bounty, pays with a malformed proof, then checksaccepted_work_for_accountandaccount_accepted_summaryskip it gracefully. ✅
Minor Concern
The PR body claims it extracts code into app/activity.py, but the actual diff only changes app/serializers.py and tests/test_serializers.py. The app/activity.py extraction either:
- Was already done in a prior commit on this branch
- Or is aspirational but not included
The diff as presented is self-contained and correct — the _proof_payload() extraction is a genuine code health improvement that reduces duplication. The body/filename mismatch is harmless but worth noting.
Verdict
APPROVE — no blockers. The change is a focused, safe consolidation that:
- Removes duplicated
json.loads+ type-check code - Adds graceful handling of edge cases (
None, malformed JSON) - Has regression tests for both the fallback path and real malformed data
- Preserves all existing behavior and public API shapes
The dirty mergeable state is a separate concern (the branch needs rebasing onto main) and does not reflect on the code quality of the change itself.
s0584273828-ctrl
left a comment
s0584273828-ctrl
left a comment
There was a problem hiding this comment.
Requesting changes on PR #328 after rechecking current head ee15d046933272d5d645a5c2b5f9d16a3b7ea15d against current origin/main.
The serializer helper extraction itself looks focused, and the focused local checks below pass. The blocker is integration readiness: the branch is currently not mergeable with origin/main.
Evidence:
gh api repos/ramimbo/mergework/pulls/328reportsmergeable: falseandmergeable_state: dirty.git merge-tree --write-tree origin/main HEADexits 1 and reports a content conflict intests/test_serializers.py.- The command also auto-merges
app/serializers.py, so the observed blocker is specifically the test-file conflict.
Validation on the current PR head:
uv run --extra dev python -m pytest tests/test_serializers.py -q-> 4 passeduv run --extra dev ruff check app/serializers.py tests/test_serializers.py-> all checks passeduv run --extra dev python -m mypy app/serializers.py-> no issues in 1 source file
Patch direction: rebase or merge current origin/main, resolve tests/test_serializers.py, and keep both the current-main serializer assertions and this PR's malformed-proof fallback regression coverage.
6 participants
Summary
app/main.pyinto focusedapp/activity.pyhelpers.Evidence
app/main.pycarried the accepted-work activity row shaping, search filtering, contributor aggregation, account summary, and safe fallback logic inline with route wiring. Moving that coherent activity subsystem intoapp/activity.pymakes public account/activity behavior easier to review independently from OAuth, admin, MCP, wallet, and page routing code.tests/test_activity_helpers.pyfirst and observed it fail withModuleNotFoundError: No module named 'app.activity'before extracting the helpers.Test Evidence
python -m pytest tests/test_activity_helpers.py tests/test_activity.py tests/test_api_mcp.py -q-> 72 passed.python -m ruff check app/activity.py app/main.py tests/test_activity_helpers.py-> All checks passed.python -m ruff format --check app/activity.py app/main.py tests/test_activity_helpers.py-> 3 files already formatted.python -m mypy app-> success.python scripts/docs_smoke.py-> docs smoke ok.git diff --check-> clean.MRWK
Refs #320
No secrets, wallet private keys, private payout details, deployment values, private vulnerability details, or MRWK price claims are included.
Summary by CodeRabbit
Bug Fixes
Tests