Setting up the initial copilot instructions#2069
Setting up the initial copilot instructions#2069sramani-metro wants to merge 7 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial GitHub Copilot instruction documents for the Thunder framework, including a repo-wide overview and layer-scoped guidance that applies automatically to Source/** subtrees.
Changes:
- Added repo-wide
.github/copilot-instructions.mdcovering architecture, workflows, and cross-cutting C++/Thunder conventions. - Added scoped instruction files under
.github/instructions/forcore/,com/,plugins/, andSource/Thunder/. - Documented macOS-specific portability pitfalls (notably cross-dylib
dynamic_castissues with template typeinfo).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/instructions/thunder-runtime.instructions.md |
Adds runtime/daemon-layer guidance for Source/Thunder/** (Server/ServiceMap/Controller/config/lifecycle). |
.github/instructions/plugins.instructions.md |
Adds plugin framework API rules for Source/plugins/** (IPlugin, IShell lifetime, JSONRPC patterns). |
.github/instructions/core.instructions.md |
Adds Source/core/** rules for dependencies, threading, sync, JSON, IPC, portability. |
.github/instructions/com.instructions.md |
Adds Source/com/** rules for COM-RPC interfaces, proxy/stub patterns, and macOS IPC casting caveats. |
.github/copilot-instructions.md |
Adds top-level repository + style conventions and links to scoped instruction files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/copilot-instructions.md
Outdated
| ### Include Guards | ||
| Use the `__UPPER_CASE_H__` double-underscore pattern — **not** `#pragma once`: | ||
| ```cpp | ||
| #ifndef __MYPLUGIN_H__ | ||
| #define __MYPLUGIN_H__ | ||
| ... | ||
| #endif // __MYPLUGIN_H__ | ||
| ``` |
There was a problem hiding this comment.
This file mandates include guards and forbids #pragma once, but the codebase uses both patterns today (e.g. Source/com/com.h:20 uses #pragma once, while Source/plugins/IPlugin.h:20 uses #ifndef __IPLUGIN_H__). To avoid confusing contributors/Copilot, consider either allowing both or documenting where each is preferred, rather than an absolute prohibition.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ## `IPlugin::IPluginExtended` and `IPlugin::ICompositPlugin` | ||
| - `IPluginExtended`: adds `Inbound(Web::Request&)` and `Process(const Web::Request&)` for HTTP handling. Also known as the `IWeb` pattern (prefer `IWeb` from the exchange interfaces if available). | ||
| - `ICompositPlugin`: for plugins that aggregate sub-plugins — rare, check existing examples before implementing. | ||
|
|
There was a problem hiding this comment.
IPluginExtended/ICompositPlugin are not nested under IPlugin (they’re PluginHost::IPluginExtended and PluginHost::ICompositPlugin), and IPluginExtended does not add Inbound()/Process() for HTTP handling. In Source/plugins/IPlugin.h, HTTP handling is provided by PluginHost::IWeb (Inbound(Web::Request&), Process(const Web::Request&)), while IPluginExtended only adds Attach()/Detach() for channel lifecycle. Please update this section header and bullets to match the actual interfaces to avoid misleading plugin authors.
| ## `IPlugin::IPluginExtended` and `IPlugin::ICompositPlugin` | |
| - `IPluginExtended`: adds `Inbound(Web::Request&)` and `Process(const Web::Request&)` for HTTP handling. Also known as the `IWeb` pattern (prefer `IWeb` from the exchange interfaces if available). | |
| - `ICompositPlugin`: for plugins that aggregate sub-plugins — rare, check existing examples before implementing. | |
| ## `PluginHost::IPluginExtended` and `PluginHost::ICompositPlugin` | |
| - `PluginHost::IPluginExtended`: extends `IPlugin` with `Attach()` / `Detach()` for channel lifecycle handling (called when a channel connects/disconnects). | |
| - HTTP handling is provided by `PluginHost::IWeb`, which adds `Inbound(Web::Request&)` and `Process(const Web::Request&)`. Prefer `PluginHost::IWeb` (or the corresponding exchange interface) when exposing HTTP endpoints. | |
| - `PluginHost::ICompositPlugin`: for plugins that aggregate sub-plugins — rare, check existing examples before implementing. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@sramani-metro I've opened a new pull request, #2070, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `Service` wraps the loaded plugin library and its `IPlugin` implementation. | ||
| - Activation path: `Service::Activate()` → loads `.so` → calls `Initialize()` → transitions to `ACTIVATED`. | ||
| - Deactivation path: `Service::Deactivate()` → calls `Deinitialize()` → unloads `.so`. | ||
| - Reason codes for deactivation (`REQUESTED`, `AUTOMATIC`, `FAILURE`, `SHUTDOWN`, `CONDITIONS`, `WATCHDOG`) are passed to `IPlugin::INotification::Deactivated()` — use the correct reason when triggering deactivation. |
There was a problem hiding this comment.
This says deactivation reason codes are passed to IPlugin::INotification::Deactivated(), but that callback’s signature is only (callsign, IShell*) (no reason parameter). The reason is available via IShell::Reason() (e.g. used in Source/Thunder/Controller.h). Please update this guidance, and also align the reason names with the actual PluginHost::IShell::reason enum (e.g. WATCHDOG_EXPIRED, INITIALIZATION_FAILED, etc.).
| - Reason codes for deactivation (`REQUESTED`, `AUTOMATIC`, `FAILURE`, `SHUTDOWN`, `CONDITIONS`, `WATCHDOG`) are passed to `IPlugin::INotification::Deactivated()` — use the correct reason when triggering deactivation. | |
| - Deactivation reasons are represented by `PluginHost::IShell::reason` (e.g. `WATCHDOG_EXPIRED`, `INITIALIZATION_FAILED`, `SHUTDOWN`, `REQUESTED`) and are available inside `IPlugin::INotification::Deactivated(const string& callsign, PluginHost::IShell* shell)` via `shell->Reason()` — always set and interpret the correct reason when triggering or handling deactivation. |
| - Activation path: `Service::Activate()` → loads `.so` → calls `Initialize()` → transitions to `ACTIVATED`. | ||
| - Deactivation path: `Service::Deactivate()` → calls `Deinitialize()` → unloads `.so`. |
There was a problem hiding this comment.
The activation/deactivation paths mention loading/unloading a .so, but Thunder also targets macOS where the runtime library extension is .dylib. Consider wording this as “shared library” (or “.so/.dylib”) to avoid encoding a Linux-only assumption.
| - Activation path: `Service::Activate()` → loads `.so` → calls `Initialize()` → transitions to `ACTIVATED`. | |
| - Deactivation path: `Service::Deactivate()` → calls `Deinitialize()` → unloads `.so`. | |
| - Activation path: `Service::Activate()` → loads the plugin shared library (`.so`/`.dylib`) → calls `Initialize()` → transitions to `ACTIVATED`. | |
| - Deactivation path: `Service::Deactivate()` → calls `Deinitialize()` → unloads the plugin shared library (`.so`/`.dylib`). |
.github/copilot-instructions.md
Outdated
| ### Execution Modes (plugin `configuration.root.mode`) | ||
| | Mode | Meaning | | ||
| |------|---------| | ||
| | `Off` | In-process (default if no `root` block) | |
There was a problem hiding this comment.
The execution mode table describes Off as “In-process”, but in-process is the default when there is no root block. In code, RootConfig::ModeType::OFF is treated as an invalid/unexpected value (ASSERT(false) in Source/plugins/Shell.cpp), so documenting it as an in-process mode is incorrect.
| | `Off` | In-process (default if no `root` block) | | |
| | `Off` | Invalid / reserved — do not use; in-process is the implicit default when no `root` block is present | |
| - **Acquire `_adminLock` as briefly as possible** — never call plugin code (`Initialize`, `Deinitialize`, JSON-RPC handlers) while holding it. | ||
| - `_pluginHandling` (per-`Service`) guards per-plugin interface pointers — separate from `_adminLock`. Never hold both simultaneously. | ||
| - Lookup by callsign: `ServiceMap::FromLocator()` resolves a URL segment to a `Service*`. | ||
| - Plugin states are `UNAVAILABLE → DEACTIVATED → PRECONDITION → ACTIVATED` (plus `SUSPENDED`/`HIBERNATED`). Only the framework transitions states — never force-set `_state` directly. |
There was a problem hiding this comment.
The plugin state list is incomplete/misleading. In this repo PluginHost::IShell::state includes additional lifecycle states like ACTIVATION, DEACTIVATION, and DESTROYED (see Source/plugins/IShell.h), so describing the states as only UNAVAILABLE → DEACTIVATED → PRECONDITION → ACTIVATED is inaccurate.
| - Plugin states are `UNAVAILABLE → DEACTIVATED → PRECONDITION → ACTIVATED` (plus `SUSPENDED`/`HIBERNATED`). Only the framework transitions states — never force-set `_state` directly. | |
| - Plugin states follow `PluginHost::IShell::state` (see `Source/plugins/IShell.h`), including `UNAVAILABLE`, `DEACTIVATED`, `ACTIVATION`, `PRECONDITION`, `ACTIVATED`, `DEACTIVATION`, `SUSPENDED`, `HIBERNATED`, and `DESTROYED`. Only the framework transitions states — never force-set `_state` directly. |
|
|
||
| Thunder is a **plugin-based device abstraction framework** for embedded platforms (primarily ARM/MIPS running Linux). It is developed by Metrological, a Comcast company, and is the backbone of RDK (Reference Design Kit) deployments worldwide. | ||
|
|
||
| Thunder provides: |
There was a problem hiding this comment.
perhaps mention something that it also provides general abstrtactions to plugins, e.g. OS things like threads, time, low level network stuff, files etc.?
| | `Thunder/` | Core daemon + libraries (`core/`, `com/`, `plugins/`, `Source/Thunder/`) | 2nd | | ||
| | `ThunderInterfaces/` | Shared COM-RPC interface headers (`interfaces/I*.h`) + generated JSON-RPC wrappers (`jsonrpc/`) | 3rd | | ||
| | `ThunderNanoServices/` | Metrological-maintained plugins (not used in RDK deployments) | 4th | | ||
| | `rdkservices/` | RDK-specific plugins consuming ThunderInterfaces | 4th (parallel with NanoServices) | |
There was a problem hiding this comment.
Don't know if it is important but that is old RDKV. Newer code is in all the enterservices- repo's
|
|
||
| | Repo | Role | Build Order | | ||
| |------|------|-------------| | ||
| | `ThunderTools/` | Code generators: `ProxyStubGenerator`, `JsonGenerator`, `ConfigGenerator`, `LuaGenerator` (all Python) | 1st — must be installed before any other build | |
There was a problem hiding this comment.
ThunderExtensions repo is missinf(should we explain inhere what that is)
Also ThunderClient libraries, ThunderNanoServicesRDK, ThunderTools,
|
|
||
| **The build order is mandatory.** Each repo's installed artifacts are required by the next. Never attempt to build out of order. | ||
|
|
||
| ## Key Binary and Library Outputs |
There was a problem hiding this comment.
As it mentions key gues does not have to be complete
| ┌──────────────────────────────────────────────┐ | ||
| │ rdkservices / ThunderNanoServices │ Plugin implementations | ||
| ├──────────────────────────────────────────────┤ | ||
| │ Source/Thunder/ │ Daemon runtime: Server, ServiceMap, Controller |
There was a problem hiding this comment.
Don't know if it is important, but Thunder is more next to plugins
| ## Required Elements in Every Interface Header | ||
|
|
||
| ```cpp | ||
| #ifndef __IMYFEATURE_H__ |
| | Primitive | Class | When to use | | ||
| |-----------|-------|------------| | ||
| | Mutual exclusion | `Core::CriticalSection` | Protecting shared mutable state | | ||
| | RAII lock guard | `Core::SafeSyncType<CriticalSection>` | Preferred over manual Lock/Unlock | |
There was a problem hiding this comment.
It is not preferred over manual (okay now stating Pierre's opinion :) )
Only for very small functions we use RAII over manual. But we can discuss in the CCB. But for now this should be the rule to be consistent with existing code
| @@ -0,0 +1,621 @@ | |||
| --- | |||
There was a problem hiding this comment.
Isnn't that the same as this PR: #2072 ?
See my remark there, can't we point to the PSG somehow ? It has all options ad is reviewed
| @@ -0,0 +1,245 @@ | |||
| --- | |||
There was a problem hiding this comment.
This whole section is hugely incorrect. Just remove it?
|
|
||
| ## `Core::IUnknown` — The Base of All COM Interfaces | ||
| - Every COM interface must inherit **`virtual public Core::IUnknown`** — virtual inheritance is mandatory to avoid diamond-problem reference count collisions. | ||
| - `Core::IUnknown` provides `AddRef()`, `Release()`, and `QueryInterface(id)`. |
There was a problem hiding this comment.
Mhmm that is strange, that is not even completely correct (addref and release come in via IReferenceCounted :)
sebaszm
left a comment
sebaszm
left a comment
There was a problem hiding this comment.
Thanks for putting this together! That was a lot to go through!
Thus I have the impression this is too verbose for what it's supposed to address. I expected a list of brief guidelines, rather than such detailed description, whch is hard to review and maintain being correct. I even think some sections like how to build, run and test are redundant completely from a pull-request reviewer perspective.
Many do's and don'ts are repeated multiple times (like not using a vector, disregarding it being incorrect), some are not applicable (like don't edit generated files - this can't happen in a PR), some should be already obvious for AI (like what asserts are for).
And there seems to be much focus towards macOS (it hardly being a main target platform).
I left some comments on statements that I believe are straight incorrect. I also hope with so much text it's not a TL;DR sort of thing for Copilot. :)
| | `libThunderCOM.so/.dylib` | `install/lib/` | COM-RPC infrastructure — proxy/stub registration, communicator | | ||
| | `libThunderPlugins.so/.dylib` | `install/lib/` | Plugin framework API — `IPlugin`, `IShell`, `JSONRPC` | | ||
| | `libThunderCOMProcessStubs.so/.dylib` | `install/lib/` | Core COM proxy/stub library | | ||
| | Plugin `.so`/`.dylib` files | `install/lib/thunder/plugins/` | Individual plugin implementations | |
.github/copilot-instructions.md
Outdated
|
|
||
| These rules apply to **every file** in every repo. No exceptions. | ||
|
|
||
| 1. **All COM interface methods must return `Core::hresult`** — `Core::ERROR_NONE` (0) on success. |
There was a problem hiding this comment.
That would be true for JSON-RPC. For COM-RPC, while this would be preferred, I don't think this is a concrete rule. Certainly it's supported that the return value is not hresult.
.github/copilot-instructions.md
Outdated
|
|
||
| 1. **All COM interface methods must return `Core::hresult`** — `Core::ERROR_NONE` (0) on success. | ||
| 2. **COM interfaces are immutable once published** — methods must never be changed or removed; only append new methods at the end. | ||
| 3. **No STL containers across COM boundaries** — use `RPC::IIteratorType<T, ID>` iterators. Never pass `std::vector`, `std::list`, or `std::map` across an interface. |
There was a problem hiding this comment.
Not completely true. We do support std::vecotor in COM-RPC (with mandatory (at)restrict limitation).
|
|
||
| ## Include Guards | ||
|
|
||
| Thunder does **not** use `#pragma once`. Every header must use the double-underscore guard pattern: |
There was a problem hiding this comment.
I think it's the opposite? We endorese #pragma once, but old code still uses header guards, no point in changing it now.
| ## Virtual Methods and `override` | ||
|
|
||
| - Every method that overrides a virtual base **must** have the `override` specifier. Never rely on implicit override. | ||
| - Interface destructors must be declared `= default` with `override` — never define a body: |
There was a problem hiding this comment.
Why they are expected to be declared? the interface already inherits a virtual destructor from IUnknown, and re-declaring it in interface has zero functional purpose and only clutters the contract :) (the header).
| ``` | ||
|
|
||
| **Rules**: | ||
| - Never pass `std::vector`, `std::list`, `std::map`, or any STL container across a COM interface. |
There was a problem hiding this comment.
Neither list or map will even work. But vector will.
|
|
||
| ## `Core::IUnknown` — The Reference Counting Contract | ||
|
|
||
| All COM objects in Thunder inherit from `Core::IUnknown`, which provides: |
There was a problem hiding this comment.
technically Iunknown only provides queryInterface :)
| Core::ProxyType<InvokeMessage> invoke(base); // dynamic_cast fails on macOS (see below) | ||
| ``` | ||
|
|
||
| ## macOS `dynamic_cast` Failure for Template Types |
There was a problem hiding this comment.
I think this section will be romoved once the issue described here is resolved.
| - `enum { ID = RPC::ID_XXX }` — unique registered ID; see `06-comrpc-fundamentals.md` for ID allocation | ||
| - `~IMyFeature() override = default` — destructor must be `= default`, never defined with a body | ||
| - `EXTERNAL` on the struct — required for cross-dylib visibility | ||
| - All methods return `Core::hresult` — no exceptions to this rule |
There was a problem hiding this comment.
again, I don't think this is a no-exception rule
| namespace { static Plugin::Metadata<Plugin::MyPlugin> metadata(1, 0, 0, {}, {}, {}); } | ||
| ``` | ||
|
|
||
| ## JSON-RPC Method Registration |
There was a problem hiding this comment.
This is typically autogenerated, no need for calling these manually.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| - `Core::ERROR_UNAVAILABLE`, `Core::ERROR_ILLEGAL_STATE`, `Core::ERROR_BAD_REQUEST` for common failure modes. | ||
| - Use `Core::ErrorToString(code)` when logging errors. | ||
| - Check for transport vs plugin errors: `if (result & COM_ERROR) { /* transport failure */ }`. | ||
| - `Core::ERROR_TIMEDOUT` (== `Core::ERROR_ASYNC_ABORTED` in some contexts) indicates a COM-RPC call exceeded `CommunicationTimeOut`. |
There was a problem hiding this comment.
The error-code note says Core::ERROR_TIMEDOUT is equal to Core::ERROR_ASYNC_ABORTED “in some contexts”, but these are distinct values in Source/core/Errors.h (TIMEDOUT=11, ASYNC_ABORTED=4). This equivalence statement is likely incorrect and could cause people to write the wrong comparisons/handling; suggest removing the equality claim or describing the actual scenario being referred to (e.g. specific APIs translating one to the other).
| - `Core::ERROR_TIMEDOUT` (== `Core::ERROR_ASYNC_ABORTED` in some contexts) indicates a COM-RPC call exceeded `CommunicationTimeOut`. | |
| - `Core::ERROR_TIMEDOUT` indicates a COM-RPC call exceeded `CommunicationTimeOut`. Do not assume it is equal to `Core::ERROR_ASYNC_ABORTED`; handle them as distinct error codes. |
|
|
||
| ## Hot-Reload Details | ||
|
|
||
| - **Plugin config hot-reload**: `FileObserver` watches the `configs` directory. On change, `ConfigObserver` calls `Service::ConfigUpdated()` → plugin is deactivated, config updated, re-activated (if autostart). This is non-atomic — there is a brief window where the plugin is inactive. |
There was a problem hiding this comment.
The “Plugin config hot-reload” description doesn’t match the current runtime behavior in Source/Thunder/PluginServer.h: ConfigObserver::Updated() calls ServiceMap::ConfigReload(), which iterates *.json files and calls Insert(...); Insert ignores duplicate callsigns rather than deactivating/reactivating an existing plugin. Consider updating this section to reflect what actually happens today (and/or documenting the current limitations) so this file doesn’t teach a hot-reload mechanism that isn’t implemented.
| - **Plugin config hot-reload**: `FileObserver` watches the `configs` directory. On change, `ConfigObserver` calls `Service::ConfigUpdated()` → plugin is deactivated, config updated, re-activated (if autostart). This is non-atomic — there is a brief window where the plugin is inactive. | |
| - **Plugin config “hot-reload” (current behavior is limited)**: `FileObserver` watches the `configs` directory. On change, `ConfigObserver::Updated()` calls `ServiceMap::ConfigReload()`, which rescans `*.json` files and calls `Insert(...)` for each. `Insert` **ignores duplicate callsigns**, so existing plugins are *not* deactivated, reconfigured, or re-activated — only newly added configs for previously unknown callsigns are picked up. To apply config changes for an existing plugin, you must currently deactivate/activate it (for example via `Controller`) or restart the daemon. Treat this as **additive-only discovery**, not full hot-reload. |
.github/instructions/constraints.md
Outdated
|
|
||
| ### Comments | ||
|
|
||
| Keep comments minimal — code must be self-explanatory. A comment must add context that cannot be read from the code itself. Write comments in correct English. Place comments on their own line immediately before or after the relevant code — never on the same line as a statement. |
There was a problem hiding this comment.
The rule "Place comments ... never on the same line as a statement" contradicts existing Thunder code style (there are many inline trailing comments in Source/, e.g. Source/Thunder/PluginHost.cpp:84 and Source/com/Administrator.h:37). Consider relaxing this to a preference (e.g. avoid trailing comments when possible) rather than a hard prohibition, so contributors/Copilot aren’t pushed into a style that the repo doesn’t follow.
| Keep comments minimal — code must be self-explanatory. A comment must add context that cannot be read from the code itself. Write comments in correct English. Place comments on their own line immediately before or after the relevant code — never on the same line as a statement. | |
| Keep comments minimal — code must be self-explanatory. A comment must add context that cannot be read from the code itself. Write comments in correct English. Prefer placing comments on their own line immediately before or after the relevant code; avoid trailing comments where possible, but they are acceptable when they significantly improve clarity or match the surrounding file’s style. |
.github/instructions/constraints.md
Outdated
| - **Files**: PascalCase — `MyModule.h`, `MyModule.cpp`. | ||
| - **Namespaces and classes**: PascalCase — `MyNameSpace`, `MyClass`. | ||
| - **Class member methods**: PascalCase — `GetValue()`, `SetActive()`. | ||
| - **Class member variables**: `_` prefix + camelCase — `_myVariable`, `_adminLock`. | ||
|
|
There was a problem hiding this comment.
This section states that all files should be PascalCase, but the repository contains many established lowercase headers (e.g. Source/core/core.h, Source/plugins/plugins.h, Source/com/com.h). To avoid generating renames or inconsistent guidance, consider scoping this to “new feature/module files” or documenting exceptions/legacy naming patterns.
|
|
||
| ## COM-RPC Debugging | ||
| - Switch `communicator` to TCP (`"127.0.0.1:62000"`) to capture traffic. | ||
| - Run `Thunder/GenerateLua.sh Thunder/ ThunderInterfaces/` to produce the Wireshark dissector data file. |
There was a problem hiding this comment.
This section references Thunder/GenerateLua.sh, but that script does not exist in this repository (no GenerateLua.sh found). If the intent is cross-repo documentation, consider either linking to the correct script/path (if it lives elsewhere) or removing this step to avoid a dead-end for contributors.
| - Run `Thunder/GenerateLua.sh Thunder/ ThunderInterfaces/` to produce the Wireshark dissector data file. |
.github/instructions/navigation.md
Outdated
|
|
||
| # Thunder — Navigation | ||
|
|
||
| Use this file to find where detailed documentation or canonical examples live. No rules are defined here — rules are in `copilot-instructions.md` and `constraints.md`. For anti-patterns, see `common-mistakes.md`. |
There was a problem hiding this comment.
This file says “No rules are defined here — rules are in copilot-instructions.md and constraints.md”, but the repo also contains several other rule-bearing instruction files (e.g. the scoped core.instructions.md, com.instructions.md, plugins.instructions.md, thunder-runtime.instructions.md). Consider rewording to avoid implying that only those two files contain rules.
| Use this file to find where detailed documentation or canonical examples live. No rules are defined here — rules are in `copilot-instructions.md` and `constraints.md`. For anti-patterns, see `common-mistakes.md`. | |
| Use this file to find where detailed documentation or canonical examples live. This file does not define new rules — consult `copilot-instructions.md`, `constraints.md`, and other `*.instructions.md` files for rules. For anti-patterns, see `common-mistakes.md`. |
.github/copilot-instructions.md
Outdated
|
|
||
| 1. **Plugin isolation** — the daemon is infrastructure only. All domain logic lives in plugins behind COM interfaces. Never add business logic to `Source/Thunder/` or `Source/core/`. | ||
| 2. **COM-RPC over everything** — inter-plugin calls go through COM interface pointers. The framework short-circuits in-process calls to direct vtable calls automatically. Never use JSON-RPC, raw sockets, or function pointers to call between plugins. | ||
| 3. **No RTTI, no exceptions, no STL allocations across ABI boundaries** — Thunder targets embedded Linux (`-fno-exceptions`, `-fno-rtti`). COM ref-counting (`AddRef`/`Release`/`Core::ProxyType`) replaces smart pointers at interface boundaries. |
There was a problem hiding this comment.
This sentence states Thunder targets -fno-rtti, but the build configuration in this repo does not appear to disable RTTI (e.g. cmake/common/CompileSettings.cmake sets -fno-exceptions but not -fno-rtti), and the codebase uses dynamic_cast in multiple places. Consider removing the -fno-rtti claim or rephrasing it to the actual constraint being enforced (e.g. avoid RTTI-dependent patterns across dylib boundaries).
| 3. **No RTTI, no exceptions, no STL allocations across ABI boundaries** — Thunder targets embedded Linux (`-fno-exceptions`, `-fno-rtti`). COM ref-counting (`AddRef`/`Release`/`Core::ProxyType`) replaces smart pointers at interface boundaries. | |
| 3. **No RTTI, no exceptions, no STL allocations across ABI boundaries** — Thunder targets embedded Linux and is built with `-fno-exceptions`. Treat RTTI-dependent patterns (`dynamic_cast`, `typeid`, etc.) as forbidden across COM/ABI boundaries so deployments can safely opt into `-fno-rtti`. COM ref-counting (`AddRef`/`Release`/`Core::ProxyType`) replaces smart pointers at interface boundaries. |
|
|
||
| ### Include Guards | ||
|
|
||
| Prefer `#pragma once` for new header files. Existing files that use classic `#ifndef` guards may keep them — there is no need to convert. Never mix both styles in the same file. |
There was a problem hiding this comment.
This file says to prefer #pragma once for new headers, but core.instructions.md later says “use classic guards … (double-underscore prefix/suffix)” for new headers. These two instruction sources conflict and will cause inconsistent output; consider making them agree (either prefer #pragma once everywhere for new headers, or scope classic guards to specific subtrees / legacy files).
| Prefer `#pragma once` for new header files. Existing files that use classic `#ifndef` guards may keep them — there is no need to convert. Never mix both styles in the same file. | |
| For new header files, use classic `#ifndef` / `#define` / `#endif` include guards, with a double-underscore prefix and suffix in the macro name (for example, `__MYMODULE_H__`). Existing files that already use `#pragma once` or classic guards may keep them — there is no need to convert. Never mix both styles in the same file. |
| `Source/core/` is a **provider of abstractions** — it must never depend on upper layers. Upper layers (`com/`, `plugins/`, `Thunder/`) implement the abstractions. Concretely: | ||
| - `core/` defines `IWorkerPool`, `IResource`, `IDispatch` — it does not implement a `WorkerPool` that knows about plugins. | ||
| - `com/` and `Thunder/` provide the concrete `WorkerPool` and `ResourceMonitor` implementations that are injected at runtime. | ||
| - If a new abstraction in `core/` needs a concrete implementation, define an interface in `core/` and provide the implementation in `Thunder/` or `com/`. |
There was a problem hiding this comment.
The “Dependency Inversion Rule” claims com/ or Thunder/ provide the concrete WorkerPool / ResourceMonitor implementations, but in this repo they are implemented in Source/core/ (e.g. Source/core/WorkerPool.h defines Core::WorkerPool). This is likely to mislead contributors about where to add/modify threading/event-loop behavior; please update this section to reflect the actual layering used here.
| `Source/core/` is a **provider of abstractions** — it must never depend on upper layers. Upper layers (`com/`, `plugins/`, `Thunder/`) implement the abstractions. Concretely: | |
| - `core/` defines `IWorkerPool`, `IResource`, `IDispatch` — it does not implement a `WorkerPool` that knows about plugins. | |
| - `com/` and `Thunder/` provide the concrete `WorkerPool` and `ResourceMonitor` implementations that are injected at runtime. | |
| - If a new abstraction in `core/` needs a concrete implementation, define an interface in `core/` and provide the implementation in `Thunder/` or `com/`. | |
| `Source/core/` is a **provider of abstractions and platform primitives** — it must never depend on upper layers. Upper layers (`com/`, `plugins/`, `Thunder/`) configure, own, and wrap these primitives, but do not introduce dependencies back into `core/`. Concretely: | |
| - `core/` defines interfaces such as `IWorkerPool`, `IResource`, and `IDispatch`, and provides their generic implementations (for example, `Core::WorkerPool` in `Source/core/WorkerPool.h` and `Core::ResourceMonitor` in `Source/core/ResourceMonitor.h`). These implementations must remain independent of plugins and upper-layer concepts. | |
| - Threading, job-scheduling, and file-descriptor readiness behavior (event loop, `poll`/`epoll` integration, etc.) should be added or modified in `Source/core/` (typically in the `WorkerPool*` and `ResourceMonitor*` files), not in `com/` or `Thunder/`. | |
| - Upper layers (`com/`, `Thunder/`, `plugins/`) may own instances of `Core::WorkerPool` / `Core::ResourceMonitor`, wrap them, and register their own `IResource`/`IDispatch`/job types, but must not create new dependencies from `Source/core/` back into these layers. | |
| - If a new abstraction in `core/` needs a concrete implementation that depends on plugins, IPC, or other upper-layer types, define an interface in `core/` and provide the implementation in `Thunder/` or `com/`, accessed via dependency injection or callbacks. |
| - **Unregister timing**: `Unregister()` may be called from the `Handle()` callback itself (re-entrant safe), but the resource must not be destroyed until `Unregister()` returns. | ||
|
|
||
| ## New File Checklist | ||
| - Include guard for new headers: use classic guards `#ifndef __MYFILE_H__` / `#define __MYFILE_H__` / `#endif` (double-underscore prefix and suffix); existing headers that already use `#pragma once` may remain unchanged. |
There was a problem hiding this comment.
This “New File Checklist” says new headers should use classic include guards with double-underscore identifiers, but (a) constraints.md instructs to prefer #pragma once for new headers, and (b) many core headers already use #pragma once (e.g. Source/core/WorkerPool.h, Source/core/Errors.h). Consider aligning this checklist with the repo’s current practice and the global constraints file to avoid conflicting guidance.
| - Include guard for new headers: use classic guards `#ifndef __MYFILE_H__` / `#define __MYFILE_H__` / `#endif` (double-underscore prefix and suffix); existing headers that already use `#pragma once` may remain unchanged. | |
| - Include guard for new headers: prefer `#pragma once` (as per `constraints.md`). Existing headers that already use classic include guards may remain unchanged, but do not introduce new double-underscore identifiers in include guards. |
| - `ERROR_COMPOSIT_OBJECT` returned from `AddRef()` signals a composit (delegate-lifetime) object — `Administrator::RecoverySet` handles this correctly; do not special-case it elsewhere. | ||
|
|
||
| ## Collections Across COM Boundaries | ||
| - **Never** pass `std::vector`, `std::list`, `std::map`, or any STL container across a COM interface. |
There was a problem hiding this comment.
This no longer holds for std::vector ;)
| ## `Core::ProxyType<T>` — Dynamic Cast Internals | ||
| - `Core::ProxyType<T>` uses `dynamic_cast` internally when converting between types (e.g. `ProxyType<IIPC>` → `ProxyType<InvokeMessage>`). | ||
| - **Never** rely on `ProxyType<T>(base_proxy)` conversion for `IPCMessageType<>` specialisations — `dynamic_cast` on template types is not reliable across dylib boundaries. Use `IIPC::Label()` + `static_cast` instead. | ||
| - This does NOT affect `ProxyType` conversions for non-template types (e.g. `ProxyType<IPCChannel>`) — those work correctly. |
There was a problem hiding this comment.
Or inside the same library
There was a problem hiding this comment.
Perhaps we should be careful with the rather detailed part. I do not want AI start generating code everywhere using labes or adding own RTTI info and static cast.
|
|
||
| ## `SmartInterfaceType<T>` — COM-RPC Client Helper | ||
| For external tools/clients connecting to Thunder via COM-RPC, `SmartInterfaceType<T>` provides automatic connection management: | ||
| ```cpp |
There was a problem hiding this comment.
Perhaps also mention the other ones, like for Controller and Plugin?
| @@ -0,0 +1,157 @@ | |||
| --- | |||
| name: Thunder-Core-Library | |||
There was a problem hiding this comment.
Should we perhaps mention generated code should use code form core over what is available in the C++ standard library?
| - `ProcessShutdown` (background timer) manages graceful shutdown of multiple OOP processes. | ||
| - `RemoteConnectionMap::Closed()` is called when a channel disconnects — notifies `IRemoteConnection::INotification` observers, then calls `Terminate()`. | ||
|
|
||
| ### CLI Arguments for ThunderPlugin |
There was a problem hiding this comment.
This should not be needed at all.
This process should always be spawned from Thunder itself
| - Modifying the signature, return type, or parameter list of an existing published COM interface method — always extend via a new versioned interface | ||
| - Process-global mutable state outside of `Core::Singleton<T>` | ||
|
|
||
| --- |
There was a problem hiding this comment.
Scanning through this I think I also would have expected a section on coding guidelines etc.
No description provided.