Uh oh!
There was an error while loading. Please reload this page.
refactor(api): enforce type annotations on whitelisted methods - #303
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
e991297 to
2d5d39bCompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
2d5d39b to
9448943Compare681ebb1 to
2d93bc3CompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
9448943 to
ac5d02cCompare2d93bc3 to
9999c82CompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
ac5d02c to
ece5d45Compare9999c82 to
2b651a7CompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
2b651a7 to
8d2c94aCompareece5d45 to
11da4dbCompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
11da4db to
b1796daCompare8d2c94a to
eb3aad8CompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
eb3aad8 to
705f754Compareb1796da to
f98f005CompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Turn on require_type_annotated_api_methods so Frappe rejects any whitelisted method whose arguments are not annotated. No annotations had to be added: typing_validations.py skips the first parameter when it is self/cls, and every whitelisted controller method in the app takes only self. This is runtime cover for what CI already blocks statically. The Linters workflow runs frappe/semgrep-rules, whose missing-argument-type-hint rule (rules/security/whitelisted.yml) fails the build on an unannotated whitelisted argument. The hook catches what a syntactic rule cannot see, and unlike semgrep it cannot be silenced with a nosemgrep comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BuzzEvent.after_insert was whitelisted, which made a lifecycle hook remotely re-runnable through run_doc_method. Re-running it duplicates the default sponsorship tiers and ticket types and then calls save(ignore_permissions=True). Nothing calls it remotely. Replace the last two json.loads calls with frappe.parse_json. It already returns a frappe._dict for dict payloads, so payments.py collapses to a single call and its function-local import goes with it. In event_booking.py the except clause widens to (ValueError, TypeError): orjson.JSONDecodeError subclasses json.JSONDecodeError so the old clause still caught it, but naming json there was the last reason to keep the module imported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f98f005 to
30f3175CompareThere was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Successfully created backport PR for |
Stacked on #302 — read that first.
Turns on
require_type_annotated_api_methodsfor buzz, the last step of the APIrefactor, plus the dead surface the plan parked here.
The plan expected this PR to annotate eight whitelisted controller methods. It
annotates none:
typing_validations.py:114-115skips the first parameter when itis
self/cls, and every one of those methods takes onlyself. The twomodule-level ones were already typed. An AST pass and a live
frappe.whitelistedwalk both found zero unannotated arguments, so the hook flip is inert on today's
code — which is the point.
frappe/semgrep-rules'missing-argument-type-hint(rules/security/whitelisted.yml) is inside the pathlinter.yml:38-41already scans. Verified: it catches an unannotated argumentplain, behind an intervening decorator, and alongside
*args/**kwargs, andsemgrep ciexits 1 with the findings marked blocking. The hook is the runtimehalf — it sees what a syntactic rule cannot, and
# nosemgrepdoesn't silence it.A test asserting the same thing was written and then dropped as redundant.
typing_validations.py:31,44splits
func.__module__), so setting it inbuzz/hooks.pybindsbuzz.*only.It checks argument presence, never return annotations, and skips
*args/**kwargs.frappe.whitelistwrapper at call time(
frappe/__init__.py:609,apply_condition=_in_request_or_test), so it is inertoutside a request or test.
BuzzEvent.after_insertloses its whitelist — it made a lifecycle hook re-runnablevia
run_doc_method, duplicating default tiers and ticket types.frappe.parse_jsonalready returns afrappe._dict(frappe/utils/data.py:2661),so
payments.pycollapses to one call.orjson.JSONDecodeErrorsubclassesjson.JSONDecodeError, so wideningevent_booking.py's except toValueErrorisa no-op that lets the
jsonimport go.payments.mark_payment_as_receivedearly-returns onfrappe.in_test(payments.py:126), so itsparse_jsonswap has no test. Makingthat path testable is refactor(payments): clean up buzz/payments.py and make the settlement path testable #299.
buzz.api.download_ticketblock inTicketDetails.vue, was already gone.Checks: 354 Python tests green (
run-tests --app buzz, 2 crm-gated skips); ruffcheck and format clean; semgrep clean; Playwright 81/85 locally, the 4 failures
reproduced identically with this PR stashed (2 are the known guest-OTP
frappe.in_testgap, 2 are UI-login specs Harsh asked to ignore).🤖 Generated with Claude Code