Adopt structured error codes for plugin UDF control signals#127
Open
kesmit13 wants to merge 14 commits into
Open
Adopt structured error codes for plugin UDF control signals#127kesmit13 wants to merge 14 commits into
kesmit13 wants to merge 14 commits into
Conversation
Per ADR 0001 in the Rust wasm-udf-server, control-signal error
responses are now JSON of the form `{"message": "...", "code": "..."}`,
and consumers read `"message"` instead of `"error"` for human-readable
text. The Python plugin server in `singlestoredb/functions/ext/plugin/`
mirrors this wire protocol and was returning plain text on errors,
which would break consumers updated to the new ADR shape.
Errors from `@@register`, `@@delete`, and unknown `@@`-prefixed signals
now carry a stable code (REGISTER_MISSING_PAYLOAD, REGISTER_INVALID_PAYLOAD,
REGISTER_FUNC_EXISTS, DELETE_MISSING_PAYLOAD, DELETE_INVALID_PAYLOAD,
DELETE_FUNC_NOT_FOUND, DELETE_FUNC_NOT_REGISTERED, UNKNOWN_SIGNAL).
The REGISTER_DISABLED / DELETE_DISABLED codes from the catalog have no
call site here because this server has no enable-register flag.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…code `_register_code_for` was folding "Cannot replace '...': not a dynamically registered function" into REGISTER_FUNC_EXISTS via a too-loose substring match. That misled clients: replace=true cannot fix the built-in case, so a duplicate-registration prompt is wrong advice. Route that path through a new REGISTER_FUNC_NOT_DYNAMIC code (mirrors DELETE_FUNC_NOT_REGISTERED on the register side), and require both "Cannot replace" and "not a dynamically registered function" to match so the genuine `already exists` case still maps to REGISTER_FUNC_EXISTS. ADR 0001 in wasm-udf-server has been updated locally with the new code; the Rust matcher in server.rs:457-462 has the same gap and will be fixed in a follow-up there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The outer except in dispatch_control_signal blanket-mapped any handler exception to UNKNOWN_SIGNAL, which the catalog reserves for unrecognized @@-prefixed signal names. Internal failures (e.g. registry blow-ups in @@functions, post-success pipe write errors in @@register/@@delete) were mislabeled as bad signals. Return INTERNAL_ERROR for that path; the unrecognized-name branch keeps UNKNOWN_SIGNAL.
There was a problem hiding this comment.
Pull request overview
This PR updates the Python plugin UDF control-signal error responses to use a structured JSON payload ({"message": ..., "code": ...}) aligned with ADR 0001 and the Rust wasm-udf-server, and updates the unit tests to assert against the new error shape and stable error codes.
Changes:
- Introduces an ADR 0001 error builder and stable error-code mapping for
@@register/@@delete, plusUNKNOWN_SIGNALandINTERNAL_ERROR. - Routes unknown control signals and unexpected handler exceptions through structured error payloads.
- Updates
singlestoredb/tests/test_plugin.pyto parseControlResult.dataas JSON on failures and validate bothcodeandmessage, adding coverage for new cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
singlestoredb/functions/ext/plugin/control.py |
Implements structured error payloads and maps register/delete failures to stable error codes. |
singlestoredb/tests/test_plugin.py |
Updates assertions to validate structured error responses and adds new test cases for new codes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Expand the module docstring in `control.py` to enumerate every ADR 0001 error code emitted from this module (including the new `REGISTER_FUNC_NOT_DYNAMIC`) and point readers to ADR 0001 in wasm-udf-server as the authoritative catalog. Clarify the `ControlResult.data` field comment to distinguish handler-specific success documents from the ADR 0001 error shape. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace substring-matching of exception messages in _register_code_for/_delete_code_for with typed exceptions raised at the site that knows the semantic intent. Substring matching was fragile because user-supplied function names and dtypes are interpolated into the exception text — a name containing "already exists", "not found", or "not a dynamically registered function" could be misclassified as REGISTER_FUNC_EXISTS, DELETE_FUNC_NOT_FOUND, or *_NOT_DYNAMIC instead of the correct INVALID_PAYLOAD code. FunctionExistsError, FunctionNotDynamicError, and FunctionNotFoundError subclass ValueError to keep existing except ValueError sites compatible. control.py now classifies by exception type, so user input embedded in messages can no longer sway the error code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
SharedRegistry.delete_function previously raised plain ValueError for both "function not found" and "not a dynamically registered function" cases. The @@delete dispatch in control.py classifies failures by exception type, so those failures were falling through to the generic branch and returning DELETE_INVALID_PAYLOAD instead of the intended DELETE_FUNC_NOT_FOUND / DELETE_FUNC_NOT_REGISTERED codes. Raise FunctionNotFoundError and FunctionNotDynamicError to match the underlying FunctionRegistry.delete_function. Add regression tests that exercise @@delete end-to-end against a real SharedRegistry (not a mock) so the typed-code path is covered without mocked side_effects. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Catching all Exception in _handle_register hid genuinely unexpected failures (ImportError, AttributeError, registry bugs) under REGISTER_INVALID_PAYLOAD. Narrow to (ValueError, SyntaxError, TypeError) — the types FunctionRegistry.create_function actually raises for user-supplied input — so unrelated infrastructure failures propagate to the outer dispatch_control_signal handler and surface as INTERNAL_ERROR per ADR 0001. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Catch UnicodeDecodeError alongside JSONDecodeError in @@register and @@delete handlers so non-UTF8 payloads return *_INVALID_PAYLOAD instead of escaping to INTERNAL_ERROR. - Validate that the @@register "replace" field is a bool; reject non-boolean values with REGISTER_INVALID_PAYLOAD. - Update delete_function docstring to enumerate the actual exception types (ValueError, FunctionNotFoundError, FunctionNotDynamicError). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…function hint - @@register and @@delete now reject non-object JSON roots with the appropriate *_INVALID_PAYLOAD code instead of falling through to INTERNAL_ERROR via AttributeError on body.get(). - FunctionRegistry.create_function checks _base_function_names before the generic exists-check, so a base-function name collision raises FunctionNotDynamicError (REGISTER_FUNC_NOT_DYNAMIC) instead of the misleading "use replace=true to overwrite" hint. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…me collision create_function previously raised FunctionNotDynamicError for any collision against a base function, even with replace=False. This violated the ADR-0001 mapping: a first-time @@register colliding with a built-in returned REGISTER_FUNC_NOT_DYNAMIC instead of the correct REGISTER_FUNC_EXISTS. Reorder the guards so without replace any name collision (including base functions) raises FunctionExistsError, and NOT_DYNAMIC is reserved for replace=true against a built-in. Update the exception's docstring to reflect that it covers both replace and delete of non-dynamic functions, and add a regression test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reorder the prechecks in FunctionRegistry.create_function so a collision with a base (built-in) function name raises FunctionNotDynamicError — mapped to REGISTER_FUNC_NOT_DYNAMIC — regardless of the replace flag. Previously, a first-time @@register colliding with a base name and replace=False produced FunctionExistsError with the hint "use replace=true to overwrite", but the next guard rejected replace=True against base names, so the suggested remedy never worked. The user had to make two trips to discover that base names are unavailable. The hint is no longer reachable for base names; replace=True against a genuine dynamic-name collision still works as before. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9339c1d. Configure here.
…IC docs The trailing `except ValueError` in `_handle_delete` only catches internal `json.loads` failures on stored signature data — client-side payload checks emit `DELETE_INVALID_PAYLOAD` themselves earlier in the handler. Map these internal failures to `INTERNAL_ERROR` to match the cross-cutting fallback in `dispatch_control_signal`. Also broaden the docstrings for `FunctionNotDynamicError` and `REGISTER_FUNC_NOT_DYNAMIC` to cover registration-time base-name collisions, not just replace/delete (commits 68d1627 / 9339c1d added that case). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Several client-shape errors were falling through to INTERNAL_ERROR because the exceptions raised deep in code generation (KeyError, AttributeError) were not in the dispatcher's typed catch list: - args/returns items missing 'dtype' raised KeyError - 'body' field as a non-string raised AttributeError on .splitlines() - 'name' field as a non-string raised TypeError on dict membership Add up-front isinstance(str) guards in control.py for name (register and delete) and body (register), mirroring the existing 'replace' boolean check. Also pre-validate the shape of args/returns items in FunctionRegistry.create_function so signature errors surface as ValueError -> REGISTER_INVALID_PAYLOAD instead of INTERNAL_ERROR. Adds regression tests covering each path.
Restore the previous field name for backward compatibility with consumers that parse the 'error' key from control-signal error responses.
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.
Summary
{\"message\": ..., \"code\": ...}) to control-signal error responses in the Python plugin UDF server, matching the Rust wasm-udf-server.@@registerand@@deleteto a stable code:REGISTER_MISSING_PAYLOAD,REGISTER_INVALID_PAYLOAD,REGISTER_FUNC_EXISTS,DELETE_MISSING_PAYLOAD,DELETE_INVALID_PAYLOAD,DELETE_FUNC_NOT_FOUND,DELETE_FUNC_NOT_REGISTERED, plusUNKNOWN_SIGNALfor unrecognised@@-prefixed signals.REGISTER_DISABLED/DELETE_DISABLEDfrom the ADR catalog have no call site here because this server has no enable-register flag — noted in the module docstring.Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Breaking change for clients that parsed plain-text control errors; behavior of register/delete against built-in names is stricter and error classification changed.
Overview
Plugin UDF control-signal failures now return ADR 0001 JSON (
{"error":"...","code":"..."}) instead of plain text, via a shared_err()helper incontrol.py, aligned with the Rust wasm-udf-server.@@register/@@deletemap registry and validation failures to stable codes (REGISTER_*,DELETE_*,UNKNOWN_SIGNAL,INTERNAL_ERROR). Handlers add stricter payload checks (JSON object, stringname/body, booleanreplace,UnicodeDecodeErroron parse). Registry work addsFunctionExistsError,FunctionNotDynamicError, andFunctionNotFoundError, validatesargs/returnsshape increate_function, and rejects any registration whose name collides with a built-in (not onlyreplace=true).SharedRegistry.delete_functionraises those types so delete errors no longer misclassify asDELETE_INVALID_PAYLOAD.Tests assert parsed
code/errorand add integration coverage for end-to-end delete error codes.Reviewed by Cursor Bugbot for commit 0cd3190. Bugbot is set up for automated code reviews on this repo. Configure here.