feat: SOVD diagnostic scripts with plugin-extensible ScriptProvider#286
Open
feat: SOVD diagnostic scripts with plugin-extensible ScriptProvider#286
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds SOVD (ISO 17978-3 §7.15) “diagnostic scripts” support to the gateway with a pluggable ScriptProvider backend, exposing CRUD + execution lifecycle REST endpoints for Apps and Components.
Changes:
- Introduces
ScriptProvider/ScriptManagerplus a built-inDefaultScriptProvider(filesystem + subprocess execution with timeouts/limits). - Registers
/scriptsREST routes (apps/components), updates discovery capabilities/links, and extends plugin framework to allow script backends. - Adds unit + integration tests and updates docs/configuration to describe and enable the feature.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/test/features/test_scripts_api.test.py | End-to-end tests for scripts CRUD + execution and 501 behavior when disabled |
| src/ros2_medkit_gateway/test/test_script_manager.cpp | Unit tests for ScriptManager backend pass-through |
| src/ros2_medkit_gateway/test/test_default_script_provider.cpp | Unit tests for DefaultScriptProvider CRUD + subprocess execution behavior |
| src/ros2_medkit_gateway/test/demo_nodes/scripts/echo_params.py | Demo script used by provider execution tests |
| src/ros2_medkit_gateway/test/demo_nodes/scripts/echo_params.sh | Demo script used by provider execution tests |
| src/ros2_medkit_gateway/test/demo_nodes/scripts/echo_params.bash | Demo script used by provider execution tests |
| src/ros2_medkit_gateway/src/script_manager.cpp | Implements the manager that delegates to the configured backend |
| src/ros2_medkit_gateway/src/default_script_provider.cpp | Implements built-in filesystem storage + subprocess execution backend |
| src/ros2_medkit_gateway/src/http/handlers/script_handlers.cpp | Adds REST handlers for scripts CRUD/execution endpoints |
| src/ros2_medkit_gateway/src/http/rest_server.cpp | Registers scripts routes (apps/components) when handlers are present |
| src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp | Advertises scripts link/capability when scripts backend is enabled |
| src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp | Exposes “scripts” capability flag on root endpoint |
| src/ros2_medkit_gateway/src/http/handlers/capability_builder.cpp | Adds SCRIPTS capability name mapping |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Declares scripts parameters; wires plugin provider or default provider into ScriptManager |
| src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp | Tracks first ScriptProvider among loaded plugins and exposes accessor |
| src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp | Loads get_script_provider symbol across dlopen boundary |
| src/ros2_medkit_gateway/src/auth/auth_config.cpp | Adds RBAC allowlist entries for scripts endpoints per role |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/script_provider.hpp | New provider interface + request/response structs + backend error model |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/script_manager.hpp | New manager API for scripts backend delegation |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/default_script_provider.hpp | New built-in provider declarations/config types |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/script_handlers.hpp | New handler class interface |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp | Exposes ScriptHandlers in consolidated handlers header |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp | Stores ScriptHandlers in REST server |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/capability_builder.hpp | Adds SCRIPTS capability enum |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/error_codes.hpp | Adds script-related error code constants |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_types.hpp | Bumps PLUGIN_API_VERSION to 4 |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp | Adds ScriptProvider plumbing to plugin manager |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_loader.hpp | Extends load result + docs for get_script_provider |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp | Exposes get_script_manager() and stores scripts members |
| src/ros2_medkit_gateway/design/index.rst | Updates architecture/design diagram and component list to include scripts |
| src/ros2_medkit_gateway/config/gateway_params.yaml | Documents scripts configuration parameters |
| src/ros2_medkit_gateway/CMakeLists.txt | Adds scripts sources to gateway_lib and registers new unit tests |
| docs/requirements/specs/scripts.rst | Updates requirements list (removes executions-list requirement) |
| docs/api/rest.rst | Documents scripts REST endpoints and behavior |
| README.md | Marks scripts feature as available in the project feature table |
src/ros2_medkit_integration_tests/test/features/test_scripts_api.test.py
Outdated
Show resolved
Hide resolved
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/error_codes.hpp
Outdated
Show resolved
Hide resolved
src/ros2_medkit_gateway/include/ros2_medkit_gateway/default_script_provider.hpp
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Adds SOVD (ISO 17978-3 §7.15) “Scripts” support to ros2_medkit_gateway via a pluggable ScriptProvider backend, exposing CRUD + execution lifecycle endpoints for Apps and Components, with a built-in filesystem/subprocess implementation and plugin override support.
Changes:
- Introduces
ScriptProvider+ScriptManager, plus built-inDefaultScriptProvider(filesystem uploads + subprocess execution with timeouts / concurrency limits). - Adds
ScriptHandlersand registers scripts routes in the REST server; updates discovery/health outputs and capability mapping. - Extends plugin framework + auth route permissions, and adds unit + integration test coverage plus docs updates.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/test/features/test_scripts_api.test.py | End-to-end launch/integration coverage for scripts enabled/disabled + CRUD + executions |
| src/ros2_medkit_gateway/test/test_script_manager.cpp | Unit tests for ScriptManager pass-through behavior |
| src/ros2_medkit_gateway/test/test_script_handlers.cpp | Handler-level unit tests for 501 guard + error/status mapping + success paths |
| src/ros2_medkit_gateway/test/test_default_script_provider.cpp | Unit tests for DefaultScriptProvider CRUD + subprocess execution behaviors |
| src/ros2_medkit_gateway/test/demo_nodes/scripts/echo_params.sh | Demo script used by provider execution tests (sh) |
| src/ros2_medkit_gateway/test/demo_nodes/scripts/echo_params.py | Demo script used by provider execution tests (python) |
| src/ros2_medkit_gateway/test/demo_nodes/scripts/echo_params.bash | Demo script used by provider execution tests (bash) |
| src/ros2_medkit_gateway/src/script_manager.cpp | ScriptManager implementation (backend delegator) |
| src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp | Tracks first ScriptProvider plugin and exposes getter |
| src/ros2_medkit_gateway/src/plugins/plugin_loader.cpp | Loads get_script_provider symbol from plugins and stores provider pointer |
| src/ros2_medkit_gateway/src/http/rest_server.cpp | Instantiates ScriptHandlers and registers scripts routes for apps/components |
| src/ros2_medkit_gateway/src/http/handlers/script_handlers.cpp | HTTP handlers for script CRUD and execution lifecycle + error mapping |
| src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp | Advertises scripts availability in root/health capabilities |
| src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp | Adds scripts link + capability when scripts backend is enabled |
| src/ros2_medkit_gateway/src/http/handlers/capability_builder.cpp | Maps new SCRIPTS capability enum to "scripts" |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Declares scripts params; wires plugin/default script provider into ScriptManager |
| src/ros2_medkit_gateway/src/default_script_provider.cpp | Built-in provider: manifest scripts + filesystem uploads + subprocess exec |
| src/ros2_medkit_gateway/src/auth/auth_config.cpp | Adds scripts endpoint permissions across roles |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/script_manager.hpp | ScriptManager public API |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/script_provider.hpp | ScriptProvider interface + request/response structs + backend error enum |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_types.hpp | Bumps PLUGIN_API_VERSION to 4 |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp | Declares ScriptProvider support and getter |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_loader.hpp | Extends load result with ScriptProvider pointer + docs |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp | Adds ScriptHandlers member |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/script_handlers.hpp | ScriptHandlers class declaration |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp | Exposes ScriptHandlers in aggregated handlers header |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/capability_builder.hpp | Adds SCRIPTS to capability enum |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/error_codes.hpp | Adds vendor x-medkit-* error codes for scripts |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp | Adds get_script_manager() and stores script manager/provider members |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/default_script_provider.hpp | DefaultScriptProvider API + config structs + ExecutionState |
| src/ros2_medkit_gateway/design/index.rst | Updates architecture diagram/docs to include ScriptManager |
| src/ros2_medkit_gateway/config/gateway_params.yaml | Adds scripts configuration section and defaults |
| src/ros2_medkit_gateway/CMakeLists.txt | Adds scripts sources to gateway_lib and registers new gtests |
| docs/requirements/specs/scripts.rst | Updates scripts requirements list (removes executions list requirement) |
| docs/api/rest.rst | Adds REST API documentation for scripts endpoints + enablement notes |
| README.md | Marks Scripts feature as available |
src/ros2_medkit_gateway/include/ros2_medkit_gateway/default_script_provider.hpp
Show resolved
Hide resolved
src/ros2_medkit_integration_tests/test/features/test_scripts_api.test.py
Show resolved
Hide resolved
mfaferek93
requested changes
Mar 18, 2026
src/ros2_medkit_gateway/include/ros2_medkit_gateway/providers/script_provider.hpp
Show resolved
Hide resolved
…meout Replace stubbed execution methods in DefaultScriptProvider with real POSIX fork/exec/waitpid subprocess execution. Features: concurrent execution with configurable limit, positional/named/flag argument passing from manifest config, JSON-on-stdin fallback for scripts without args config, environment variable injection, timeout with SIGTERM/SIGKILL escalation using process groups, and stdout/stderr capture with JSON output parsing.
Three test scenarios with 27 tests total: - TestScriptsNotConfigured: verify all 8 script endpoints return 501 when scripts_dir is not configured - TestScriptsCRUD: upload/list/get/delete lifecycle, metadata, error cases (9 tests) - TestScriptsExecution: Python and shell execution with status polling, termination, delete rejection while running, timestamps (9 tests)
… atomic counters Bug 1 - Deadlock: delete_execution held exec_mutex_ while joining the monitor thread, which itself acquires exec_mutex_ to update status. Fix: extract the ExecutionState from the map under the lock, release the lock, then join threads outside the lock. Bug 2 - Concurrency race: start_execution checked the concurrency limit under exec_mutex_ but incremented active_execution_count_ ~150 lines later in a separate lock scope, allowing two concurrent calls to both pass the check. Fix: increment (reserve slot) inside the check lock scope and decrement on all error paths before final registration. Bug 3 - Data race: id_counter_ and exec_id_counter_ were plain int but incremented without synchronization from start_execution. Fix: change to std::atomic<int>.
…e non-SOVD REQ_045
…nct error codes - Set res.status and Location header before send_json in upload/start - Add "scripts" capability to handle_root, conditional on backend - Make scripts capability conditional in component/app discovery - Replace duplicate "invalid-request" error codes with distinct x-medkit-script-* vendor codes for each script error case - Update integration test to match new error code string
- Mark Scripts feature as Available in README feature table - Add Scripts section to REST API reference with all 8 SOVD endpoints - Add scripts to SOVD compliance matrix - Add ScriptManager to gateway architecture diagram and component list
…ams, output, failure, lifecycle
…g scripts - Validate entity_id and script_id match in get/control/delete_execution to prevent cross-entity access via guessed execution IDs (security fix) - Reorder mutex declarations before the data they protect to avoid UB on shutdown when ExecutionState destructors join threads - Check for running executions before allowing script deletion (SOVD 409)
…, remove hardcoded size limit - Add validate_collection_access(SCRIPTS) after validate_entity_for_route in all 8 handler methods, matching the pattern used by bulk-data and other handlers - Remove duplicate error constants ERR_SCRIPT_NOT_FOUND and ERR_SCRIPT_UNSUPPORTED_TYPE that shadowed existing ERR_RESOURCE_NOT_FOUND and ERR_INVALID_PARAMETER - Remove hardcoded kMaxScriptSizeBytes (10 MB) and handler-level size check; the provider's configurable max_file_size_mb already validates this and returns FileTooLarge
…l-based test waits
…th, document AlreadyExists
…TooLarge, NotRunning Add 12 new tests covering: - Adversarial is_valid_resource_id inputs via handler (path traversal, shell metacharacters, overlong IDs, valid special chars) - Control execution validation (missing action, bogus action, invalid JSON, empty action) - NotRunning error code mapping to 409 - FileTooLarge rejection with max_file_size_mb=0 - ControlCompletedExecutionReturnsNotRunning (stop after completion)
Completed/failed/terminated executions now evict oldest entries when count exceeds configurable max_execution_history (default 100). Stdin JSON parameters are rejected if larger than 64 KB (POSIX pipe buffer). Also adds millisecond precision to ISO 8601 timestamps.
…nks to collection - Validate Content-Type: multipart/form-data before accessing req.files in upload handler - Extract duplicated entity type ternary into static entity_type_from_path() helper - Add _links with self/parent to list_scripts collection response (consistent with discovery handlers) - Add test for Content-Type rejection
…ty, thread docs - Validate manifest script paths at construction time and log warnings - Replace sequential counter IDs with timestamp-prefixed IDs - Fix TOCTOU race in delete_script by holding exec_mutex_ through fs ops - Add pid safety check before kill probe in timeout grace period - Add Doxygen thread safety docs and param/return docs to ScriptProvider
The RBAC permission patterns only covered components/* paths. Routes exist for all four entity types (areas, apps, components, functions), so viewers/operators/configurators were denied access to apps, areas, and functions sub-resources. Also missing: logs, bulk-data, cyclic-subscriptions, locks, updates, data-categories/groups, discovery relationship endpoints, and global faults. Added comprehensive patterns for all entity types and collections to each role, matching the actual routes in rest_server.cpp.
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.
Pull Request
Summary
Implement SOVD-compliant diagnostic scripts feature (ISO 17978-3, §7.15) with a plugin-extensible architecture. Clients can upload, manage, and execute diagnostic scripts on Components and Apps entities. The gateway ships with a built-in DefaultScriptProvider (manifest + filesystem + subprocess), but plugins can replace the entire backend via dlopen (same pattern as UpdateProvider).
Key components:
SOVD endpoints implemented:
POST /{entity}/scripts- Upload script (201 + Location)GET /{entity}/scripts- List scriptsGET /{entity}/scripts/{id}- Get metadataDELETE /{entity}/scripts/{id}- Delete script (204, 409 for manifest)POST /{entity}/scripts/{id}/executions- Start execution (202 + Location)GET /{entity}/scripts/{id}/executions/{eid}- Poll statusPUT /{entity}/scripts/{id}/executions/{eid}- Terminate (stop/forced_termination)DELETE /{entity}/scripts/{id}/executions/{eid}- Remove execution (204)Issue
Type
Testing
Unit tests (34 new):
Integration tests (27 new):
Run:
./scripts/test.sh(unit) and./scripts/test.sh integ(integration)Checklist