docs: rewrite copilot-instructions.md for effective code review#247
docs: rewrite copilot-instructions.md for effective code review#247
Conversation
Comprehensive update to reflect the current state of the project and optimize the instructions for GitHub Copilot PR reviews. Key changes: - Add full package structure (fault_manager, serialization, etc.) - Document current architecture (LogManager, PluginManager, HandlerContext) - Add SOVD entity model with capability matrix - Document handler pattern with HandlerContext validation flow - Add error codes reference (SOVD standard + vendor x-medkit-*) - Document plugin framework and provider interfaces - Add CMake module documentation (Compat, Ccache, Linting) - Replace human-oriented checklists with AI-actionable review rules using "Flag when..." directives that Copilot can apply to PR diffs - Add granular test requirements: unit tests for handlers/managers, integration tests for endpoints, error case coverage - Add anti-patterns quick reference table with concrete code examples - Add documentation-as-part-of-every-change rule
There was a problem hiding this comment.
Pull request overview
Rewrites .github/copilot-instructions.md to better reflect the current ros2_medkit architecture and provide actionable, diff-oriented rules for GitHub Copilot PR reviews (handlers, error handling, plugins, build/test expectations).
Changes:
- Expands project/package/architecture documentation to cover current gateway subsystems and multi-package workspace.
- Documents the handler/route/error-code patterns and adds concrete “Flag when…” review rules plus anti-pattern guidance.
- Adds more specific unit/integration testing requirements and traceability guidance.
| Defined in `include/ros2_medkit_gateway/models/`: | ||
|
|
||
| - **Area** - namespace grouping (`/powertrain`, `/chassis`, `root`) | ||
| - **Component** - groups Apps by namespace (synthetic in runtime mode, explicit in manifest) | ||
| - **App** - individual ROS 2 node | ||
| - **Function** - capability grouping (functional view, aggregates Apps) |
There was a problem hiding this comment.
The doc says the Area/Component/App/Function entity model is defined in include/ros2_medkit_gateway/models/, but those concrete entity structs live under include/ros2_medkit_gateway/discovery/models/ (the models/ directory contains capabilities/types/cache wrappers). Please update this path to match the actual code locations so reviewers/bots don’t get sent to the wrong headers.
| | Collection | Server | Area | Component | App | Function | | ||
| |-----------|--------|------|-----------|-----|----------| | ||
| | configurations | x | | x | x | | | ||
| | data | x | | x | x | x (aggregated) | | ||
| | faults | x | | x | x | | | ||
| | operations | x | | x | x | x (aggregated) | | ||
| | logs | x | | x | x | | | ||
|
|
There was a problem hiding this comment.
In the “SOVD Resource Collections” table, logs is listed as a resource collection, but the code’s ResourceCollection enum uses COMMUNICATION_LOGS with the path segment communication-logs (and logs appears as a resource name in EntityCapabilities). Consider renaming this row or clarifying whether you mean the /logs resource vs the communication-logs collection so the table matches the implementation.
| | Collection | Server | Area | Component | App | Function | | |
| |-----------|--------|------|-----------|-----|----------| | |
| | configurations | x | | x | x | | | |
| | data | x | | x | x | x (aggregated) | | |
| | faults | x | | x | x | | | |
| | operations | x | | x | x | x (aggregated) | | |
| | logs | x | | x | x | | | |
| | Collection (path segment) | Server | Area | Component | App | Function | | |
| |---------------------------|--------|------|-----------|-----|----------| | |
| | configurations | x | | x | x | | | |
| | data | x | | x | x | x (aggregated) | | |
| | faults | x | | x | x | | | |
| | operations | x | | x | x | x (aggregated) | | |
| | communication-logs | x | | x | x | | | |
| Note: The `/logs` resource (as referenced in `EntityCapabilities`) is provided within the `communication-logs` collection. |
| **Vendor-specific** (`x-medkit-` prefix): `ERR_X_MEDKIT_ROS2_SERVICE_UNAVAILABLE`, `ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE`, `ERR_X_MEDKIT_ROS2_TOPIC_UNAVAILABLE`, etc. | ||
|
|
||
| - **ROS 2 Jazzy** (Ubuntu 24.04) / **ROS 2 Humble** (Ubuntu 22.04) / **ROS 2 Rolling** (Ubuntu 24.04, best-effort) | ||
| - **cpp-httplib**: HTTP server (found via pkg-config) | ||
| - **nlohmann_json**: JSON serialization | ||
| - **yaml-cpp**: Configuration parsing | ||
| Error response schema: | ||
| ```json | ||
| { | ||
| "error_code": "entity-not-found", | ||
| "message": "Component not found", | ||
| "parameters": {"entity_id": "my_component"} | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The error response schema example omits the vendor_code field that HandlerContext::send_error() adds for vendor errors (x-medkit-*), where error_code becomes vendor-error. Please update this section to mention/illustrate vendor_code, otherwise it suggests an incorrect response format for vendor-specific failures.
| ### Error Handling | ||
|
|
||
| ## Design Principles | ||
| - **Flag** `throw` statements in handler or manager code. Use `tl::expected<T, std::string>` and return `tl::make_unexpected(msg)` instead. |
There was a problem hiding this comment.
The rule “Flag throw statements in handler or manager code” doesn’t match the current codebase: several manager/config modules do throw (e.g., AuthManager, DataAccessManager, manifest parsing, etc.). If the intent is only “don’t throw from HTTP handlers” (or only from specific layers), please narrow/reword this rule to avoid generating false-positive PR comments against existing patterns.
| - **Flag** `throw` statements in handler or manager code. Use `tl::expected<T, std::string>` and return `tl::make_unexpected(msg)` instead. | |
| - **Flag** `throw` statements in HTTP handler code (e.g., REST handlers in `rest_server.cpp`). Prefer using `tl::expected<T, std::string>` to propagate failures or `HandlerContext::send_error()` with an early return instead of throwing. Manager and configuration layers may continue to use exceptions where consistent with existing patterns. |
| | `UpdateProvider` | `providers/update_provider.hpp` | Software update management | | ||
| | `IntrospectionProvider` | `providers/introspection_provider.hpp` | Custom entity introspection | | ||
|
|
||
| Plugins loaded as `.so` files with `extern "C"` factory. Provider interfaces return typed C++ structs (not JSON) - manager layer handles serialization. Plugin failures are caught and isolated. |
There was a problem hiding this comment.
This says provider interfaces “return typed C++ structs (not JSON)”, but at least UpdateProvider::get_update/register_update uses nlohmann::json, and IntrospectionProvider::IntrospectionResult::metadata is JSON. Please adjust this wording to reflect that providers may exchange JSON for metadata while still avoiding “HTTP-level JSON responses” in the plugin API.
| Plugins loaded as `.so` files with `extern "C"` factory. Provider interfaces return typed C++ structs (not JSON) - manager layer handles serialization. Plugin failures are caught and isolated. | |
| Plugins are loaded as `.so` files with an `extern "C"` factory. Provider interfaces primarily return typed C++ structs, with optional `nlohmann::json` fields for metadata (e.g., update payloads or introspection metadata); HTTP-level JSON responses and serialization remain the responsibility of the manager/REST layer. Plugin failures are caught and isolated. |
| } | ||
| ``` | ||
|
|
||
| **ROS_DOMAIN_ID isolation**: GTest targets creating `rclcpp::Node` need unique domain IDs. Current: 62-66 (fault_manager), 99 (gateway). Next available: 67. |
There was a problem hiding this comment.
The ROS_DOMAIN_ID note is out of date: ros2_medkit_fault_manager uses 62–65 and ros2_medkit_gateway uses 66 (see package CMakeLists). There is no 99 in current CTest properties. Please update the “current IDs” list so it stays consistent with the build config.
| **ROS_DOMAIN_ID isolation**: GTest targets creating `rclcpp::Node` need unique domain IDs. Current: 62-66 (fault_manager), 99 (gateway). Next available: 67. | |
| **ROS_DOMAIN_ID isolation**: GTest targets creating `rclcpp::Node` need unique domain IDs. Current: 62–65 (`ros2_medkit_fault_manager`), 66 (`ros2_medkit_gateway`). Next available: 67. |
|
|
||
| **General test rules:** | ||
| - **Flag** any use of `GTEST_SKIP()`, `@pytest.mark.skip`, `unittest.skip`, or equivalent - tests must not be skipped, fix the code or the test instead. | ||
| - **Flag** GTest files that create `rclcpp::Node` without setting a unique `ROS_DOMAIN_ID` in CMakeLists.txt `set_tests_properties()`. Current IDs: 62-66, 99. Next available: 67. |
There was a problem hiding this comment.
This rule references “Current IDs: 62-66, 99”, but the repository’s CMake currently assigns 62–65 to fault_manager tests and 66 to gateway’s test_log_manager. Please update this line (and keep the “next available” guidance consistent) to avoid encoding incorrect IDs into future PR reviews.
| - **Flag** GTest files that create `rclcpp::Node` without setting a unique `ROS_DOMAIN_ID` in CMakeLists.txt `set_tests_properties()`. Current IDs: 62-66, 99. Next available: 67. | |
| - **Flag** GTest files that create `rclcpp::Node` without setting a unique `ROS_DOMAIN_ID` in CMakeLists.txt `set_tests_properties()`. Current ROS_DOMAIN_IDs in CMake: 62–65 (fault_manager tests), 66 (gateway `test_log_manager`), 99. Next available: 67. |
| | `find_package(yaml-cpp REQUIRED)` | Use `medkit_find_yaml_cpp()` | | ||
| | `res.set_header("Content-Type", ...)` + `set_chunked_content_provider(...)` | Remove `set_header` - provider sets Content-Type | | ||
| | `GTEST_SKIP()` or `@pytest.mark.skip` | Fix the test or the code, never skip | | ||
| | `set_tests_properties(... ENVIRONMENT "ROS_DOMAIN_ID=99")` reusing existing ID | Assign next available ID (67) | |
There was a problem hiding this comment.
The anti-pattern example hard-codes ROS_DOMAIN_ID=99 as an “existing ID”, but the current gateway tests use 66 (and fault_manager uses 62–65). Please update this example to reference the actual assigned IDs so it remains actionable and accurate.
| | `set_tests_properties(... ENVIRONMENT "ROS_DOMAIN_ID=99")` reusing existing ID | Assign next available ID (67) | | |
| | `set_tests_properties(... ENVIRONMENT "ROS_DOMAIN_ID=66")` reusing existing ID | Assign next available ID (67) | |
Pull Request
Summary
Comprehensive rewrite of
.github/copilot-instructions.mdto reflect the current state of the project and optimize instructions for GitHub Copilot PR reviews.The previous version was outdated - missing packages (fault_manager, serialization, diagnostic_bridge), the handler pattern (HandlerContext, validate_entity_for_route), error handling (tl::expected, error_codes.hpp), plugin framework, and used human-oriented checklists that Copilot couldn't effectively act on.
Key changes:
- [ ]checklists with "Flag when..." directives - actionable rules Copilot can apply to PR diffsIssue
N/A - maintenance task, no issue tracking required.
Type
Testing
Documentation-only change. No code or tests affected.
Checklist