From d1058e2d3fb10d3066bef8ca7b5ed11a69404c85 Mon Sep 17 00:00:00 2001 From: swethasukumarr Date: Mon, 13 Jul 2026 13:52:17 -0400 Subject: [PATCH 1/6] RDKEMW-19315 : Add coding guidelines for firebolt-cpp-client --- .../coding-guidelines.instructions.md | 535 ++++++++++++++++++ 1 file changed, 535 insertions(+) create mode 100644 .github/instructions/coding-guidelines.instructions.md diff --git a/.github/instructions/coding-guidelines.instructions.md b/.github/instructions/coding-guidelines.instructions.md new file mode 100644 index 0000000..a481d78 --- /dev/null +++ b/.github/instructions/coding-guidelines.instructions.md @@ -0,0 +1,535 @@ +--- +applyTo: "**/*.h,**/*.cpp,**/CMakeLists.txt" +--- + +# firebolt-cpp-client — Coding Guidelines + +**Scope:** This document governs code generation and modification for the `firebolt-cpp-client` repository. +It is intended for use by both AI agents (Copilot, openspec) and human developers. + +**How to read this document:** +- **Current practice** — observed consistently in the codebase; enforce as-is. +- **Recommended going forward** — not yet consistent but must be adopted for new/modified code. +- **Anti-pattern** — seen in the repo OR likely to be introduced by AI generation; includes why it is wrong in this specific codebase. + +Markers: +- `[⚑ MAINTAINER]` — item requires maintainer confirmation before being treated as binding. +- `[ASSUMPTION]` — inferred from code patterns where no explicit policy exists. +- `[GAP]` — documented in `.github/copilot-instructions.md` but not yet applied in source. + +--- + +## 1. Architecture and Module Boundaries + +### 1.1 Three-Layer Architecture + +The codebase has exactly three conceptual layers. Do not collapse or skip layers. + +| Layer | Location | Purpose | +|---|---|---| +| Public API | `include/firebolt/.h` | Pure virtual interfaces; the only surface consumers see | +| Implementation | `src/_impl.h` + `src/_impl.cpp` | Concrete logic; hidden from consumers | +| JSON deserialization | `src/json_types/.h` | Wire format ↔ native type adapters; never exposed publicly | + +**Current practice (confirmed across all 13 modules):** +- Every module has exactly one interface (`I`), one impl (`Impl`), and zero or more JSON type adapters. +- Advertising does not have a `json_types/advertising.h` entry in the `src/json_types/` directory — its `JsonData` types live directly in `src/json_types/advertising.h`. Checked: `src/advertising_impl.cpp` includes `"json_types/advertising.h"`. +- The singleton entry point is `Firebolt::IFireboltAccessor::Instance()`, implemented in `src/firebolt.cpp` as a Meyers singleton over `FireboltAccessorImpl`. + +**Anti-patterns:** +- Do not add business logic to `src/json_types/*.h` files. They must only do: field validation, field extraction, enum mapping. +- Do not add `#include .h>` inside another module's public header unless there is a direct type dependency. Cross-module dependencies at the public layer risk coupling the consumer's include graph. `include/firebolt/metrics.h` correctly includes `"firebolt/common_types.h"` because it uses `AgePolicy`. +- Do not add new public `include/firebolt/` headers for internal types. Internal types live in `src/`. + +### 1.2 Module Registration + +Every new module must be wired into `src/firebolt.cpp`: +1. Member variable of type `Module::ModuleImpl` in `FireboltAccessorImpl`. +2. Initialised via `Firebolt::Helpers::GetHelperInstance()` in the constructor initialiser list. +3. Accessor method returning `Module::IModule&` override. +4. `unsubscribeAll()` call in the private `unsubscribeAll()` helper — **only if the module supports subscriptions**. + +Reference: `src/firebolt.cpp` lines 44–110. + +--- + +## 2. Naming Conventions + +### 2.1 Namespaces + +**Current practice (confirmed in every file):** +- Top-level namespace: `Firebolt` +- Module namespace: `Firebolt::` where `` matches the directory/header name with initial capital (e.g., `Firebolt::Device`, `Firebolt::Lifecycle`, `Firebolt::TextToSpeech`). +- JSON adapter namespace: `Firebolt::::JsonData` for module-local types (e.g., `Firebolt::Device::JsonData`, `Firebolt::Accessibility::JsonData`). +- Cross-module shared JSON types: `Firebolt::JsonData` (e.g., `AgePolicyEnum` in `src/json_types/common.h`). +- Every `.h` and `.cpp` file closes with a namespace-end comment: `} // namespace Firebolt::`. + +**Anti-patterns:** +- Do not use `using namespace Firebolt;` or `using namespace Firebolt::;` in header files. +- `using namespace Firebolt::Helpers;` is present in two `.cpp` files (`stats_impl.cpp`, `lifecycle_impl.cpp`) but is not used in the majority of implementation files. `[⚑ MAINTAINER]` Whether to ban this in `.cpp` files uniformly. + +### 2.2 Interfaces and Implementations + +**Current practice:** +- Interface: `I` declared in `include/firebolt/.h` (e.g., `IDevice`, `ILifecycle`, `IActions`). +- Implementation: `Impl` in `src/_impl.h` (e.g., `DeviceImpl`, `LifecycleImpl`). +- Neither the interface nor the impl class is named simply `` — that name is reserved for the namespace. + +### 2.3 Method Names + +**Current practice (confirmed across all modules):** +- Getter methods: lowerCamelCase, no `get` prefix (e.g., `chipsetId()`, `audioDescription()`, `connected()`). +- Subscription methods: `subscribeOn()` returning `Result` (e.g., `subscribeOnHdrChanged`, `subscribeOnCountryChanged`, `subscribeOnIntent`). +- Unsubscription: `unsubscribe(SubscriptionId id)` (universal, module-scoped) and `unsubscribeAll()`. +- Invoke-style (fire-and-forget): verb phrases matching the RPC method (e.g., `ready()`, `signIn()`, `close()`). + +**Anti-patterns:** +- Do not name subscription methods `subscribe()` without the `On` prefix — it violates the established naming scheme. +- Do not use `Get`, `Set`, `Is` prefixes on getter methods. + +### 2.4 RPC Method and Event Name Strings + +**Current practice:** +- Getter/invoke RPC name: `"."` in camelCase (e.g., `"Device.chipsetId"`, `"Metrics.startContent"`). +- Event name: `".on"` (e.g., `"Device.onHdrChanged"`, `"Actions.onIntent"`, `"Lifecycle2.onStateChanged"`). +- `Lifecycle2` is the correct wire name for lifecycle RPCs in this version — do not use `Lifecycle`. +- TextToSpeech event names use lowercase suffixes matching the wire protocol (e.g., `"TextToSpeech.onWillspeak"`, `"TextToSpeech.onSpeechstart"`). These must match the OpenRPC fixture exactly. + +**Anti-patterns:** +- Do not guess RPC or event string names. Always derive them from `docs/openrpc/the-spec/firebolt-open-rpc.json`. + +### 2.5 Enum Names + +**Current practice:** +- C++ enum class names: `SCREAMING_SNAKE_CASE` (e.g., `INITIALIZING`, `ACTIVE`, `KILL_RELOAD`). +- `EnumType` instance variable names: `Enum` (e.g., `LifecycleStateEnum`, `CloseReasonEnum`, `DeviceClassEnum`, `AgePolicyEnum`). +- Wire values in `EnumType` map: lowercase strings matching the OpenRPC fixture (e.g., `{"initializing", ...}`, `{"killReload", ...}`). + +### 2.6 JSON Adapter Classes + +**Current practice:** +- Struct adapters: `class : public Firebolt::JSON::NL_Json_Basic<::>` (e.g., `HDRFormat`, `ClosedCaptionsSettings`, `StateChange`). +- Enum adapter instantiation: `inline const Firebolt::JSON::EnumType Enum({{...}})` at namespace scope. + +### 2.7 Test Class Names + +**Current practice (partial inconsistency — see note):** +- Unit test class: `UTest` inheriting `::testing::Test` and `MockBase` (e.g., `DeviceUTest`, `AccessibilityUTest`). +- Component test class: `CTest` inheriting `::testing::Test` (e.g., `DeviceCTest`, `LifecycleCTest`). +- `ActionsGeneratedUTest` in `test/unit/actionsGeneratedTest.cpp` uses a non-standard name because it is auto-generated. This is acceptable only for auto-generated test files. + +**Recommended going forward:** New manually-authored test files must follow the `UTest` / `CTest` naming. + +--- + +## 3. Header Guards and Include Style + +### 3.1 Header Guards + +**Current practice:** +- Bespoke headers (`include/firebolt/` and most `src/`): use `#pragma once`. +- Auto-generated headers (those with the `// AUTO-GENERATED by fb-gen` banner): use `#ifndef FIREBOLT__H` / `#define` / `#endif` guards. +- Do not mix both guards in the same file (some auto-generated files accidentally include `#pragma once` after `#ifndef`; this is harmless but should not be replicated). + +**Recommended going forward:** All new bespoke headers use `#pragma once` exclusively. + +### 3.2 Include Style + +**Current practice (confirmed across all 14 modules):** +- Includes of public firebolt headers: angle brackets with full path (`#include `, `#include `). +- Includes of implementation-local headers: double quotes without path prefix (`#include "device_impl.h"`, `#include "json_types/device.h"`). +- Includes of the module's own public header from within `_impl.h`: double quotes with full path (`#include "firebolt/device.h"`). + +**Anti-pattern:** Do not `#include ` in `include/firebolt/*.h` public headers. Helpers are an internal abstraction not part of the public API. Consumers must never see `IHelper`. + +### 3.3 Include Hygiene + +**Current practice:** The `.github/copilot-instructions.md` explicitly states: +- Include `` when using `std::move`. +- Remove unused includes such as `` when not used. + +Confirmed observation: `src/actions.h` (auto-generated) includes `` because it uses `std::move` in the default `subscribeOnIntentChanged` method body. + +--- + +## 4. Class Structure and Copy/Move Semantics + +### 4.1 `*Impl` Class Declaration Order + +**Current practice (confirmed in all 13 impl headers):** +``` +class Impl : public I +{ +public: + explicit Impl(Firebolt::Helpers::IHelper& helper); + Impl(const Impl&) = delete; + Impl& operator=(const Impl&) = delete; + ~Impl() override = default; // or override with body when custom cleanup needed + + // method overrides + +private: + Firebolt::Helpers::IHelper& helper_; + Firebolt::Helpers::SubscriptionManager subscriptionManager_; // only if module supports subscriptions +}; +``` + +All `*Impl` constructors take `Firebolt::Helpers::IHelper&` as their only parameter. The majority use `explicit`. Exception: `StatsImpl` does not use `explicit` — `[⚑ MAINTAINER]` whether to enforce `explicit` uniformly. + +### 4.2 Deleted Copy Operations + +**Current practice:** Copy constructor and copy assignment operator are explicitly deleted in **all** 13 `*Impl` classes and in `FireboltAccessorImpl` in `src/firebolt.cpp`. This is mandatory. + +### 4.3 Move Operations + +**[GAP]** `.github/copilot-instructions.md` specifies: +```cpp +ClassName(ClassName&&) = delete; +ClassName& operator=(ClassName&&) = delete; +``` + +**Current state:** No `*Impl` class in `src/` currently deletes move operations. The rule is documented policy but not yet applied. + +**Recommended going forward:** All new and modified `*Impl` classes must explicitly delete move operations immediately after the deleted copy operations. + +**Anti-pattern:** Do not add move constructors or move assignment operators to `*Impl` classes. These classes hold a reference member (`helper_`) and a `SubscriptionManager`; moving them would leave dangling state. + +### 4.4 Destructor + +**Current practice:** Use `~Impl() override = default;` unless custom cleanup is required. `StatsImpl` and `LifecycleImpl` define non-trivial destructors (empty body) — `[⚑ MAINTAINER]` whether these should be `= default`. + +--- + +## 5. Error Handling + +### 5.1 `Result` at the API Boundary + +**Current practice (without exception in 13 modules):** +- Every method in `I` returns `Result` or `Result`. +- `Result` is used for methods that send a command and carry no return value (e.g., `Metrics.ready()`, `Lifecycle.close()`). +- Callers check the result with boolean conversion (`if (result)`) or dereference after assertion (`*result`). + +**Anti-pattern:** Do not throw exceptions from public interface methods. Do not return bare `T` where failure is possible. Do not use `std::optional` as a substitute for `Result` — `optional` cannot carry an error code. + +### 5.2 Error Propagation in Implementations + +**Current practice:** Implementations propagate errors by returning the result of `helper_.get<>()`, `helper_.invoke()`, or `subscriptionManager_.subscribe<>()` directly. No intermediate `try-catch` is present in any `*_impl.cpp` file. + +**Anti-pattern:** Do not add `try-catch` blocks in `*_impl.cpp` for errors the helper/transport already handles. Do not swallow errors silently. + +### 5.3 Error Handling in JSON Adapters + +**Current practice (confirmed in `src/json_types/`):** +- `fromJson()` throws `std::invalid_argument("Missing required fields in JSON")` when required fields are absent. This is caught by the framework and converted to `Result` with `Error::InvalidParams`. +- `EnumType::at()` throws when an unknown wire value is encountered — also caught by the framework. +- Do not use `Result` inside `fromJson()`. Throw only. + +--- + +## 6. JSON Deserialization Layer (`src/json_types/`) + +### 6.1 JSON Type File Rules + +**Current practice:** +- Each `src/json_types/.h` includes its corresponding `include/firebolt/.h` and ``. +- No `.cpp` file exists under `src/json_types/` — all JSON adapter logic is header-only. +- JSON adapter classes are defined in the `Firebolt::::JsonData` namespace. + +### 6.2 Struct Adapters + +**Current practice:** +```cpp +class : public Firebolt::JSON::NL_Json_Basic<::> +{ +public: + void fromJson(const nlohmann::json& json) override + { + if (!checkRequiredFields(json, {"field1", "field2"})) + { + throw std::invalid_argument("Missing required fields in JSON"); + } + field1_ = json["field1"].get(); + field2_ = json["field2"].get(); + } + :: value() const override + { + return ::{field1_, field2_}; + } +private: + CppType field1_; + CppType field2_; +}; +``` + +Reference: `src/json_types/accessibility.h` (`ClosedCaptionsSettings`, `VoiceGuidanceSettings`), `src/json_types/device.h` (`HDRFormat`), `src/json_types/lifecycle.h` (`StateChange`). + +### 6.3 Enum Adapters + +**Current practice:** +```cpp +inline const Firebolt::JSON::EnumType<::> Enum({ + {"wire-string", ::::ENUMERATOR}, + ... +}); +``` +Wire strings are lowercase or camelCase matching the OpenRPC fixture exactly. + +Reference: `src/json_types/lifecycle.h` (`LifecycleStateEnum`, `CloseReasonEnum`), `src/json_types/device.h` (`DeviceClassEnum`), `src/json_types/common.h` (`AgePolicyEnum`). + +### 6.4 Unit Conversion in JSON Adapters + +**Current practice (specific to `Stats` module):** +The wire payload uses `*KiB` field names (e.g., `userMemoryUsedKiB`). The JSON adapter in `src/json_types/stats.h` reads the raw KiB values and the public API returns those values in KiB units. Tests in `test/unit/statsTest.cpp` and `test/component/statsTest.cpp` validate against the fixture's raw KiB values. + +**Anti-pattern:** Do not add unit conversion (e.g., ×1024 for KiB→bytes) inside `fromJson()` without a corresponding change to the public API type, tests, and OpenRPC fixture annotation. See repo memory for the `Stats.memoryUsage` history. + +--- + +## 7. Helper Abstraction Usage + +### 7.1 `IHelper` Injection + +**Current practice:** All `*Impl` constructors accept `Firebolt::Helpers::IHelper&` by reference and store it in `helper_`. This allows the unit test `MockHelper` to be injected without a virtual wrapper on the Impl class itself. + +**Anti-pattern:** Do not accept `IHelper*` (pointer) — the codebase consistently uses references. Do not store a copy of the helper. + +### 7.2 `helper_.get(methodName)` — Getter Methods + +**Current practice:** +- No-parameter getters: `helper_.get("Module.method")` +- Primitive types: use `Firebolt::JSON::String`, `Firebolt::JSON::Boolean`, `Firebolt::JSON::Unsigned`, etc. +- Struct types: use the module's `JsonData` class (e.g., `JsonData::HDRFormat`) +- Array types: use `Firebolt::JSON::NL_Json_Array` (e.g., `Localization.preferredAudioLanguages`) + +Reference: `src/device_impl.cpp`, `src/localization_impl.cpp`. + +### 7.3 `helper_.invoke(methodName, params)` — Fire-and-Forget Methods + +**Current practice:** Used for methods that return `Result`. Parameters are constructed as `nlohmann::json` before the call. Optional parameters are conditionally added. + +Reference: `src/metrics_impl.cpp` (all methods), `src/lifecycle_impl.cpp` (`close()`). + +### 7.4 `subscriptionManager_.subscribe(eventName, notification)` — Subscriptions + +**Current practice:** +- `JsonType` is the JSON adapter class, not the native type. +- `notification` is moved via `std::move()`. +- The `subscriptionManager_` is only present if the module exposes subscription methods. + +Reference: `src/accessibility_impl.cpp`, `src/actions_impl.cpp`, `src/lifecycle_impl.cpp`. + +--- + +## 8. Threading and Async Patterns + +### 8.1 Implementation Files + +**Current practice:** No threading primitives (`std::thread`, `std::mutex`, `std::condition_variable`, `std::atomic`) appear in any `*_impl.cpp` file. All async behaviour is delegated to the transport layer via `IHelper`. + +**Anti-pattern:** Do not introduce thread management in `*Impl` classes. The transport manages its own threading. + +### 8.2 Component Tests + +**Current practice (confirmed in all event-bearing component tests):** +```cpp +class ModuleCTest : public ::testing::Test +{ +protected: + void SetUp() override { eventReceived = false; } + std::condition_variable cv; + std::mutex mtx; + bool eventReceived; +}; +``` +Event delivery uses `cv.wait_for(lock, EventWaitTime, [&] { return eventReceived; })` via `verifyEventReceived()` and `verifyEventNotReceived()` from `test/utils.h`. `EventWaitTime` is `std::chrono::seconds(2)` (defined in `test/utils.cpp`). + +**Current practice — triggering events:** +- String payload: `triggerEvent("Module.onEvent", R"("string_value")")` — note outer double-quotes in JSON +- Object payload: `triggerEvent("Module.onEvent", R"({"field": value})")` +- For `Actions.onIntent`, the payload is a JSON string: `triggerEvent("Actions.onIntent", R"("launch")")`. + +Reference: `test/component/actionsGeneratedTest.cpp`, `test/component/deviceTest.cpp`, `test/component/networkTest.cpp`. + +--- + +## 9. Logging + +**Current practice:** `FIREBOLT_LOG_NOTICE("Client", "Version: %s", Version::String)` appears only in `src/firebolt.cpp` at connection time. No logging macros appear in individual module `*_impl.cpp` files. + +**[ASSUMPTION]** The logging macro originates from the `FireboltTransport` dependency, not from this repo. Individual module implementations intentionally do not log. + +**Anti-pattern:** Do not add `std::cout`, `printf`, or `FIREBOLT_LOG_*` calls to `*_impl.cpp` files. Diagnostic output in component tests uses `std::cout` only — this is test-scoped and acceptable. + +--- + +## 10. Testing Patterns + +### 10.1 Unit Tests + +**Current practice:** +- Location: `test/unit/Test.cpp` +- Uses `MockHelper` (GMock) via `MockBase` from `test/unit/mock_helper.h`. +- Test fixture: `class UTest : public ::testing::Test, protected MockBase`. +- Impl is instantiated directly: `Firebolt::::Impl impl_{mockHelper};` +- OpenRPC fixture is read via `JsonEngine` from `MockBase`. + +**Test case rules (confirmed across all unit test files):** +- Happy path getter: call `mock("Module.method")`, then call impl method, then `ASSERT_TRUE(result)` + value check. +- Negative path (bad wire data): call `mock_with_response("Module.method", )`, then `ASSERT_FALSE(result)`. +- Subscribe test: call `mockSubscribe("Module.onEvent")`, subscribe, assert `ASSERT_TRUE(result)`, then call `unsubscribe` and assert success. +- Enum validation: `validate_enum("EnumName", Firebolt::::JsonData::Enum)` checks the fixture's schema against the code's enum map. + +**Anti-pattern:** Do not test `*Impl` via `IFireboltAccessor::Instance()` in unit tests. Unit tests must isolate the impl with a mock helper, not the full singleton. + +### 10.2 Component Tests + +**Current practice:** +- Location: `test/component/Test.cpp` +- Uses `Firebolt::IFireboltAccessor::Instance()` directly (live transport connection). +- Test fixture: `class CTest : public ::testing::Test` (no `MockBase`). +- Expected values derived from `jsonEngine.get_value("Module.method")` against the OpenRPC fixture. + +**Event delivery tests:** +1. Subscribe with callback that sets `eventReceived = true` and calls `cv.notify_one()`. +2. Call `triggerEvent(...)`. +3. Call `verifyEventReceived(mtx, cv, eventReceived)`. +4. Unsubscribe with `verifyUnsubscribeResult(result)`. + +**Negative event tests (invalid payload):** +1. Subscribe. +2. Call `triggerEvent(...)` with invalid JSON payload. +3. Call `verifyEventNotReceived(mtx, cv, eventReceived)` — callback must NOT fire. +4. Unsubscribe. + +Reference: `test/component/lifecycleTest.cpp` (`subscribeOnState_JSON_RPC_compliant`). + +### 10.3 Pairing Rule + +**Current practice (confirmed across all 13 modules):** Every module has both a unit test file and a component test file. When adding a new module or method: +- Add unit tests in `test/unit/Test.cpp` +- Add component tests in `test/component/Test.cpp` +- Both test files must cover all public API methods +- Each getter/property method must have at minimum: one happy-path test and one bad-response negative test + +### 10.4 Expected Values from OpenRPC + +**Current practice:** Both unit and component tests derive expected values from `jsonEngine.get_value("Module.method")` (the first example in the OpenRPC fixture). Do not hardcode values that duplicate the fixture unless the value requires a type conversion (e.g., enum comparison using `static_cast`). + +Exception: `test/component/actionsGeneratedTest.cpp` hardcodes `"launch"` — this is permissible for auto-generated files and where the fixture specifies that exact value. + +--- + +## 11. OpenRPC Fixture Alignment + +**Current practice:** +- Fixture location: `docs/openrpc/the-spec/firebolt-open-rpc.json` +- Both unit and component test binaries read this file at runtime (path injected via `UT_OPEN_RPC_FILE` define in `test/CMakeLists.txt`). +- When adding or changing a method, the fixture must be updated to include the method, its parameters schema, and at least one example. +- Enum values in code must match `components.schemas..enum` in the fixture — validated by `validate_enum()`. + +**Rule:** When a component test validates against `jsonEngine.get_value("Module.method")`, the fixture's example value must produce the same result as what the live mock-firebolt instance returns. Keep these in sync. + +--- + +## 12. Auto-Generated vs Bespoke Code + +### 12.1 Auto-Generated File Recognition + +Files with the following banner are owned by the `firebolt-sdk-gen` generator tool — do not modify them directly: +``` +// ============================================================================ +// AUTO-GENERATED by fb-gen — DO NOT EDIT +// ============================================================================ +``` + +Confirmed auto-generated files in the repo: +- `include/firebolt/actions.h` +- `src/actions_impl.h` +- `src/actions_impl.cpp` +- `src/json_types/actions.h` +- `test/unit/actionsGeneratedTest.cpp` +- `test/component/actionsGeneratedTest.cpp` + +### 12.2 Modifying Auto-Generated Output + +When a change is generator-owned, use `firebolt-sdk-gen` from the sibling repo: +```bash +./sync-plan-checklist.sh --profile core --module --apply --no-accessor-touchpoints --target-root ../firebolt-cpp-client +``` +Do not hand-edit auto-generated files. If the generated output has a defect, fix the generator. + +### 12.3 Keeping Bespoke and Generated Files Aligned + +When a new bespoke module is added, ensure it follows the same structure as generated modules (`actions`) so the two styles remain similar enough that the generator could own the bespoke code in the future. + +--- + +## 13. CMake and Build + +**Current practice:** +- C++ standard: C++17, required (`CXX_STANDARD 17`, `CXX_STANDARD_REQUIRED YES`). +- Warning flags: `-Wall -Wextra -Wpedantic` are unconditionally applied in `CMakeLists.txt`. +- New `*_impl.cpp` files are picked up automatically via `file(GLOB SOURCES CONFIGURE_DEPENDS *.cpp json_types/*.cpp)` in `src/CMakeLists.txt`. +- New test files are picked up automatically via `file(GLOB UNIT_TESTS CONFIGURE_DEPENDS unit/*Test.cpp)` and `file(GLOB COMPONENT_TESTS CONFIGURE_DEPENDS component/*Test.cpp)`. +- Export macro: `FIREBOLTCLIENT_EXPORT` from the generated `firebolt/client_export.h`. Apply to public symbols in `include/firebolt/firebolt.h`. + +**Anti-pattern:** Do not manually list sources in `src/CMakeLists.txt` — the glob handles this. Do not introduce a new `CMakeLists.txt` inside `src/` or `test/` subdirectories. + +**Formatting enforced by CI:** `clang-format` with the project's `.clang-format` (LLVM-based, column limit 120, 4-space indent, Allman braces, C++17). Running `git ls-files -- '*.cpp' '*.h' | xargs clang-format --dry-run --Werror` is a required CI check. Do not bypass it. + +--- + +## 14. Anti-Patterns Catalogue + +The following patterns are explicitly wrong for this codebase. Each entry notes where the risk originates. + +| # | Anti-Pattern | Why It Is Wrong Here | +|---|---|---| +| AP-1 | Returning `std::optional` instead of `Result` from interface methods | Cannot carry an error code; breaks the uniform error contract used across all 13 modules | +| AP-2 | Throwing exceptions from `*_impl.cpp` method bodies | Consumers expect `Result`; exceptions escape the module boundary unexpectedly | +| AP-3 | Adding `unique_ptr` or `shared_ptr` for module ownership in `FireboltAccessorImpl` | All modules are owned by value in `FireboltAccessorImpl`; smart pointers add indirection with no benefit here | +| AP-4 | Storing `IHelper` by pointer | Consistent reference storage; pointer would allow null and is not the established contract | +| AP-5 | Making `*Impl` classes copyable or movable | They hold a non-owning reference (`helper_`) and a `SubscriptionManager`; copying/moving would silently break subscription ownership tracking | +| AP-6 | Calling `IFireboltAccessor::Instance()` in unit tests | Unit tests must isolate the impl with `MockHelper`; the singleton instantiates real transport | +| AP-7 | Hardcoding JSON field names as magic strings in `*_impl.cpp` | Field names must live in `src/json_types/` only; impl code must not parse JSON directly | +| AP-8 | Adding `nlohmann::json` includes to `include/firebolt/*.h` | Public headers must not expose the JSON library as a transitive dependency | +| AP-9 | Adding logging to `*_impl.cpp` | Logging is intentionally absent in module implementations; all diagnostics go through the transport layer | +| AP-10 | Writing a new module that omits `subscribeOnStateChanged`-style subscription when the OpenRPC spec has `on*` events | Subscriptions are load-bearing API surface; omitting them silently breaks consumer event handling | +| AP-11 | Wrapping the event trigger payload in an extra JSON object in component tests | `triggerEvent("Actions.onIntent", R"("launch")")` — the payload for string events is the JSON-encoded string directly, not `{"value": "launch"}`. Check each module's test file for the correct payload shape | +| AP-12 | Using `#include ` or other heavyweight headers without a direct use | The project compiles with `-Wall -Wextra -Wpedantic`; unused includes produce warnings. The copilot-instructions.md explicitly calls out `` as an example | +| AP-13 | Defining a new public header without `#pragma once` | Bespoke headers require `#pragma once`; `#ifndef` guards are reserved for auto-generated output | +| AP-14 | Modifying auto-generated files by hand | Files with `// AUTO-GENERATED by fb-gen — DO NOT EDIT` must be regenerated via the generator tool | + +--- + +## 15. Assumptions and Open Flags + +### Explicit Assumptions (`[ASSUMPTION]`) + +| ID | Assumption | +|---|---| +| A-1 | `FIREBOLT_LOG_*` macros come from `FireboltTransport`. Individual module impls intentionally omit logging — inferred from the absence of any logging in 12 of 13 impl files. | +| A-2 | The `using namespace Firebolt::Helpers;` pattern in `stats_impl.cpp` and `lifecycle_impl.cpp` is incidental rather than policy — inferred from its absence in the other 11 impl files. | +| A-3 | `StatsImpl`'s non-`explicit` constructor and non-`= default` destructor are legacy remnants, not intentional design. All other impls use `explicit` and `= default`. | +| A-4 | Wire names for TextToSpeech events (`onWillspeak`, `onSpeechstart`, etc.) are lowercase-concatenated because the Firebolt protocol lowercases them — inferred from the pattern in `src/texttospeech_impl.cpp` and the absence of a different naming convention for other modules' events. | + +### Items Requiring Maintainer Confirmation (`[⚑ MAINTAINER]`) + +| ID | Question | +|---|---| +| M-1 | Should move constructor/move assignment be explicitly deleted in all existing `*Impl` classes? (The rule exists in `copilot-instructions.md` but is not yet applied to source code.) | +| M-2 | Should `using namespace Firebolt::Helpers;` at file scope in `.cpp` files be banned, made consistent, or left as-is? | +| M-3 | Should `explicit` be required on all `*Impl` constructors? (`StatsImpl` currently omits it.) | +| M-4 | Should `~StatsImpl()` and `~LifecycleImpl()` become `= default` since their bodies are empty? | +| M-5 | The `friend class ::LifecycleTest;` declaration in `src/lifecycle_impl.h` is the only friend declaration in the codebase. Should this be a policy for test-only access, or should it be removed? | +| M-6 | `src/device_impl.h` uses `u_int32_t` (non-standard POSIX type) for `timeInActiveState` return type while the public interface uses `uint32_t`. Should this be corrected? | + +--- + +## 16. Relationship to Existing Policy Files + +This document **does not supersede** `.github/copilot-instructions.md`. That file contains the authoritative workflow rules (test commands, regeneration instructions, PR hygiene). This document provides the detailed coding-level guidelines that `.github/copilot-instructions.md` references but does not expand. + +Where the two documents conflict, `.github/copilot-instructions.md` takes precedence for workflow/process decisions; this document takes precedence for code structure decisions. + +`CONTRIBUTING.md` governs contribution process. This document governs code shape. From 31ba038e713b3dc31e668effea5ed73539fe23c0 Mon Sep 17 00:00:00 2001 From: swethasukumarr Date: Tue, 14 Jul 2026 13:24:05 -0400 Subject: [PATCH 2/6] RDKEMW-19315 : Correct factual errors and gaps in coding guidelines --- .github/copilot-instructions.md | 84 --------- .../coding-guidelines.instructions.md | 164 +++++++++++++----- 2 files changed, 125 insertions(+), 123 deletions(-) delete mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md deleted file mode 100644 index e80616f..0000000 --- a/.github/copilot-instructions.md +++ /dev/null @@ -1,84 +0,0 @@ -# firebolt-cpp-client Copilot Instructions - -Scope: This file applies to the firebolt-cpp-client repository. - -## Primary goals - -- Preserve API contract correctness across interface, implementation, tests, and OpenRPC fixtures. -- Keep generated surfaces and bespoke conventions aligned. -- Prefer minimal, targeted changes. - -## High-signal workflow - -1. For API-facing changes, update all of the following in one pass: - - `include/firebolt/*.h` - - `src/*_impl.h` and `src/*_impl.cpp` - - `test/unit/*Test.cpp` and `test/component/*Test.cpp` - - `docs/openrpc/the-spec/firebolt-open-rpc.json` when component tests depend on fixture shape. -2. Run component tests after edits. -3. If behavior is generator-owned, patch generator code in sibling repo and regenerate module artifacts. - -## Test commands - -- Local one-shot (current preferred): - - `./run-component-tests-local.sh` - - `./run-component-tests-local.sh --skip-image-build` -- Legacy wrappers may still exist in conversation history; prefer the local script in this repo. -- Unit-only: - - `./run-unit-tests.sh` - -## Actions module rules (important) - -- `Actions.intent` is getter-only: - - takes no parameters - - returns `Result` -- `Actions.onIntent` callback payload is a string value. -- Component event trigger for `Actions.onIntent` should use a string JSON payload (for example `"launch"`), not an object. - -## Generated-code conventions that must be preserved - -- `*Impl` classes should delete copy constructor and copy assignment: - - `ClassName(const ClassName&) = delete;` - - `ClassName& operator=(const ClassName&) = delete;` -- Unless explicitly justified as safe, `*Impl` classes should also delete move operations: - - `ClassName(ClassName&&) = delete;` - - `ClassName& operator=(ClassName&&) = delete;` -- Keep include hygiene strict: - - include `` when using `std::move` - - remove unused includes such as `` when not used -- Keep test names in consistent CamelCase for filtering. - -## Component test expectations - -- Red schema validation lines in logs can be expected for negative-path tests. -- Negative tests must still verify runtime behavior (callbacks not delivered for invalid payloads), not just compile-time surface checks. - -## OpenRPC fixture expectations - -- Module descriptions must match actual API behavior. -- Getter-style methods should carry property tags consistent with the rest of the file (for example `property:readonly` where applicable). -- Keep notifier/subscriber metadata aligned (`x-notifier`, `x-subscriber-for`). - -## Regeneration notes (sibling repo) - -When a change is generator-owned, use `firebolt-sdk-gen` and apply module-scoped output back into this repo. - -Typical flow: - -- From `../firebolt-sdk-gen`: - - `./sync-plan-checklist.sh --profile core --module actions --apply --no-accessor-touchpoints --target-root ../firebolt-cpp-client` - -This keeps migration incremental and avoids unrelated accessor touchpoint churn. - -## CI parity reminders - -- CI uses Dockerized build/test flow and mock-firebolt integration. -- Keep changes compatible with: - - `.github/workflows/ci.yml` - - `.github/scripts/run-component-tests.sh` - -## PR hygiene - -- If a review asks for include-file fixes, prefer precise header/source edits and re-run component tests. -- Do not relax negative tests just to suppress red validation logs. -- Keep commit messages scoped and explicit (example: `fix(actions): address include review comments`). diff --git a/.github/instructions/coding-guidelines.instructions.md b/.github/instructions/coding-guidelines.instructions.md index a481d78..4f0bfa2 100644 --- a/.github/instructions/coding-guidelines.instructions.md +++ b/.github/instructions/coding-guidelines.instructions.md @@ -13,9 +13,7 @@ It is intended for use by both AI agents (Copilot, openspec) and human developer - **Anti-pattern** — seen in the repo OR likely to be introduced by AI generation; includes why it is wrong in this specific codebase. Markers: -- `[⚑ MAINTAINER]` — item requires maintainer confirmation before being treated as binding. - `[ASSUMPTION]` — inferred from code patterns where no explicit policy exists. -- `[GAP]` — documented in `.github/copilot-instructions.md` but not yet applied in source. --- @@ -32,8 +30,7 @@ The codebase has exactly three conceptual layers. Do not collapse or skip layers | JSON deserialization | `src/json_types/.h` | Wire format ↔ native type adapters; never exposed publicly | **Current practice (confirmed across all 13 modules):** -- Every module has exactly one interface (`I`), one impl (`Impl`), and zero or more JSON type adapters. -- Advertising does not have a `json_types/advertising.h` entry in the `src/json_types/` directory — its `JsonData` types live directly in `src/json_types/advertising.h`. Checked: `src/advertising_impl.cpp` includes `"json_types/advertising.h"`. +- Every module has exactly one interface (`I`), one impl (`Impl`), and zero or more JSON type adapters. Four modules (discovery, localization, network, presentation) have no `json_types/` file; they use primitive-type helpers (`Firebolt::JSON::String`, `Boolean`, `Unsigned`) directly. - The singleton entry point is `Firebolt::IFireboltAccessor::Instance()`, implemented in `src/firebolt.cpp` as a Meyers singleton over `FireboltAccessorImpl`. **Anti-patterns:** @@ -51,6 +48,20 @@ Every new module must be wired into `src/firebolt.cpp`: Reference: `src/firebolt.cpp` lines 44–110. +**Cleanup to fix:** `src/firebolt.cpp` — `device_.unsubscribeAll()` is absent from `unsubscribeAll()` (lines 91–99) despite `DeviceImpl` holding a `SubscriptionManager` and `IDevice` exposing `subscribeOnHdrChanged` and `subscribeOnDolbyAtmosExperienceAvailableChanged`. Device subscriptions are never cleaned up on Disconnect. Add `device_.unsubscribeAll();` alongside the other module calls in that method. + +### 1.3 API-Facing Change Discipline + +When making an API-facing change — adding or modifying an interface method, changing a return type, or adding a subscription — update all four artifacts in a single commit: +1. `include/firebolt/.h` +2. `src/_impl.h` and `src/_impl.cpp` +3. `test/unit/Test.cpp` and `test/component/Test.cpp` +4. `docs/openrpc/the-spec/firebolt-open-rpc.json` — when component tests depend on fixture shape. + +Never leave the three layers out of sync after a commit. A compile-passing diff that omits a test update or fixture update is incomplete. + +After the change, run `./run-component-tests-local.sh` to validate all layers together before pushing. + --- ## 2. Naming Conventions @@ -65,8 +76,7 @@ Reference: `src/firebolt.cpp` lines 44–110. - Every `.h` and `.cpp` file closes with a namespace-end comment: `} // namespace Firebolt::`. **Anti-patterns:** -- Do not use `using namespace Firebolt;` or `using namespace Firebolt::;` in header files. -- `using namespace Firebolt::Helpers;` is present in two `.cpp` files (`stats_impl.cpp`, `lifecycle_impl.cpp`) but is not used in the majority of implementation files. `[⚑ MAINTAINER]` Whether to ban this in `.cpp` files uniformly. +- Do not use `using namespace Firebolt::;` in .cpp files either. Use fully-qualified names or targeted `using Firebolt::Helpers::ClassName;` declarations instead. (Applies retroactively to `stats_impl.cpp` and `lifecycle_impl.cpp` — flagged for cleanup.) ### 2.2 Interfaces and Implementations @@ -128,10 +138,11 @@ Reference: `src/firebolt.cpp` lines 44–110. **Current practice:** - Bespoke headers (`include/firebolt/` and most `src/`): use `#pragma once`. -- Auto-generated headers (those with the `// AUTO-GENERATED by fb-gen` banner): use `#ifndef FIREBOLT__H` / `#define` / `#endif` guards. -- Do not mix both guards in the same file (some auto-generated files accidentally include `#pragma once` after `#ifndef`; this is harmless but should not be replicated). +- Auto-generated **interface and impl** headers (e.g., `include/firebolt/actions.h`, `src/actions_impl.h`): use `#ifndef FIREBOLT__H` / `#define FIREBOLT__H` / `#endif` guards — confirmed at `include/firebolt/actions.h:22` and `src/actions_impl.h:22`. +- Auto-generated **json_types** headers (e.g., `src/json_types/actions.h`): use `#pragma once` — consistent with all bespoke json_types headers. +- Do not mix both guards in the same file. -**Recommended going forward:** All new bespoke headers use `#pragma once` exclusively. +**Recommended going forward:** All new bespoke headers use `#pragma once` exclusively. New auto-generated interface/impl headers follow the `#ifndef`/`#define`/`#endif` pattern; new auto-generated json_types headers follow `#pragma once`. ### 3.2 Include Style @@ -144,11 +155,11 @@ Reference: `src/firebolt.cpp` lines 44–110. ### 3.3 Include Hygiene -**Current practice:** The `.github/copilot-instructions.md` explicitly states: +**Current practice:** - Include `` when using `std::move`. - Remove unused includes such as `` when not used. -Confirmed observation: `src/actions.h` (auto-generated) includes `` because it uses `std::move` in the default `subscribeOnIntentChanged` method body. +Confirmed observation: `include/firebolt/actions.h` (auto-generated) includes `` at line 30 because it uses `std::move` in the `subscribeOnIntentChanged` default method body. --- @@ -164,6 +175,8 @@ public: explicit Impl(Firebolt::Helpers::IHelper& helper); Impl(const Impl&) = delete; Impl& operator=(const Impl&) = delete; + Impl(Impl&&) = delete; + Impl& operator=(Impl&&) = delete; ~Impl() override = default; // or override with body when custom cleanup needed // method overrides @@ -174,7 +187,13 @@ private: }; ``` -All `*Impl` constructors take `Firebolt::Helpers::IHelper&` as their only parameter. The majority use `explicit`. Exception: `StatsImpl` does not use `explicit` — `[⚑ MAINTAINER]` whether to enforce `explicit` uniformly. +- All `*Impl` constructors take `Firebolt::Helpers::IHelper&` as their only parameter and must be marked `explicit`. Three impl classes currently lack `explicit` — flagged for cleanup: + - `src/stats_impl.h` — `StatsImpl(Firebolt::Helpers::IHelper&)` at line 30 + - `src/lifecycle_impl.h` — `LifecycleImpl(Firebolt::Helpers::IHelper&)` at line 37 + - `src/localization_impl.h` — `LocalizationImpl(Firebolt::Helpers::IHelper&)` at line 29 +- Method return types and parameter types in `*_impl.h` must exactly match those declared in the corresponding public interface header. Always use `uint32_t` (from ``), never the non-standard POSIX type `u_int32_t`. A type mismatch between interface and override produces an invalid override and fails to compile on non-POSIX targets. + + **Cleanup to fix:** `src/device_impl.h` — `timeInActiveState()` is declared as `u_int32_t`; must be changed to `uint32_t` to match `include/firebolt/device.h`. ### 4.2 Deleted Copy Operations @@ -182,21 +201,44 @@ All `*Impl` constructors take `Firebolt::Helpers::IHelper&` as their only parame ### 4.3 Move Operations -**[GAP]** `.github/copilot-instructions.md` specifies: +All `*Impl` classes and `FireboltAccessorImpl` must explicitly delete the move constructor and move assignment operator, placed immediately after the deleted copy operations: + ```cpp ClassName(ClassName&&) = delete; ClassName& operator=(ClassName&&) = delete; ``` -**Current state:** No `*Impl` class in `src/` currently deletes move operations. The rule is documented policy but not yet applied. - -**Recommended going forward:** All new and modified `*Impl` classes must explicitly delete move operations immediately after the deleted copy operations. +**Rationale:** These classes hold a non-reassignable reference member (`helper_`) and, where applicable, a `SubscriptionManager`. A compiler-generated move would leave the source object with a dangling reference or corrupted subscription state. Explicit deletion makes the intent clear and prevents accidental moves at call sites. -**Anti-pattern:** Do not add move constructors or move assignment operators to `*Impl` classes. These classes hold a reference member (`helper_`) and a `SubscriptionManager`; moving them would leave dangling state. +**Cleanup to fix:** No `*Impl` class currently deletes move operations. The following files must be updated: +- `src/accessibility_impl.h` +- `src/actions_impl.h` +- `src/advertising_impl.h` +- `src/device_impl.h` +- `src/discovery_impl.h` +- `src/display_impl.h` +- `src/lifecycle_impl.h` +- `src/localization_impl.h` +- `src/metrics_impl.h` +- `src/network_impl.h` +- `src/presentation_impl.h` +- `src/stats_impl.h` +- `src/texttospeech_impl.h` +- `src/firebolt.cpp` (`FireboltAccessorImpl`) ### 4.4 Destructor -**Current practice:** Use `~Impl() override = default;` unless custom cleanup is required. `StatsImpl` and `LifecycleImpl` define non-trivial destructors (empty body) — `[⚑ MAINTAINER]` whether these should be `= default`. +Always declare `~Impl() override = default;`. Do not define a destructor with an empty body `{}` — an empty body is not custom cleanup and must be written as `= default`. Define a body only when it performs actual cleanup work (e.g., releasing a resource not managed by RAII). + +**Cleanup to fix:** +- `src/stats_impl.h` + `src/stats_impl.cpp` — destructor is declared non-inline with an empty body; replace the declaration with `~StatsImpl() override = default;` in the header and remove the definition from the `.cpp` file. +- `src/lifecycle_impl.h` + `src/lifecycle_impl.cpp` — same issue; apply the same fix. + +### 4.5 Friend Declarations + +Do not use `friend` declarations to grant test classes access to implementation internals. Design the public interface to be testable. If internal state genuinely must be observed in a test, use a `protected` member with a test-only subclass. Friend declarations break encapsulation without providing a durable or type-safe test seam, and no other class in this codebase uses this pattern. + +**Cleanup to fix:** `src/lifecycle_impl.h` — `friend class ::LifecycleTest;` is the only `friend` declaration in the codebase and must be removed. --- @@ -281,7 +323,7 @@ Reference: `src/json_types/lifecycle.h` (`LifecycleStateEnum`, `CloseReasonEnum` **Current practice (specific to `Stats` module):** The wire payload uses `*KiB` field names (e.g., `userMemoryUsedKiB`). The JSON adapter in `src/json_types/stats.h` reads the raw KiB values and the public API returns those values in KiB units. Tests in `test/unit/statsTest.cpp` and `test/component/statsTest.cpp` validate against the fixture's raw KiB values. -**Anti-pattern:** Do not add unit conversion (e.g., ×1024 for KiB→bytes) inside `fromJson()` without a corresponding change to the public API type, tests, and OpenRPC fixture annotation. See repo memory for the `Stats.memoryUsage` history. +**Anti-pattern:** Do not add unit conversion (e.g., ×1024 for KiB→bytes) inside `fromJson()` without a corresponding change to the public API type, tests, and OpenRPC fixture annotation. --- @@ -346,7 +388,7 @@ Event delivery uses `cv.wait_for(lock, EventWaitTime, [&] { return eventReceived **Current practice — triggering events:** - String payload: `triggerEvent("Module.onEvent", R"("string_value")")` — note outer double-quotes in JSON - Object payload: `triggerEvent("Module.onEvent", R"({"field": value})")` -- For `Actions.onIntent`, the payload is a JSON string: `triggerEvent("Actions.onIntent", R"("launch")")`. +- For `Actions.onIntent`, the payload is a JSON-encoded object: `triggerEvent("Actions.onIntent", R"({"intent":"launch","intentId":1})")`. The callback receives this string and must parse it with `nlohmann::json::parse()`. See §12.4 for the full contract. Reference: `test/component/actionsGeneratedTest.cpp`, `test/component/deviceTest.cpp`, `test/component/networkTest.cpp`. @@ -381,6 +423,8 @@ Reference: `test/component/actionsGeneratedTest.cpp`, `test/component/deviceTest **Anti-pattern:** Do not test `*Impl` via `IFireboltAccessor::Instance()` in unit tests. Unit tests must isolate the impl with a mock helper, not the full singleton. +**Exception for auto-generated unit tests:** `test/unit/actionsGeneratedTest.cpp` directly instantiates `::testing::NiceMock` without inheriting `MockBase`, and uses `EXPECT_CALL(mockHelper, getJson(...))` rather than the `mock()` / `mock_with_response()` convenience wrappers. This pattern is generator-owned. Do not replicate it in bespoke test files. + ### 10.2 Component Tests **Current practice:** @@ -403,6 +447,10 @@ Reference: `test/component/actionsGeneratedTest.cpp`, `test/component/deviceTest Reference: `test/component/lifecycleTest.cpp` (`subscribeOnState_JSON_RPC_compliant`). +**Component test log expectations:** Red schema validation lines in the component test log are expected and normal for negative-path tests — they indicate the transport rejected the invalid payload as intended. Do not treat them as test failures and do not suppress them by weakening the test. + +Negative tests must verify runtime behaviour — specifically that callbacks are not delivered when the payload is invalid. A test that merely asserts the code compiles with an invalid type is insufficient. Always pair with `verifyEventNotReceived`. Do not relax or remove a negative test because it produces red schema validation lines. + ### 10.3 Pairing Rule **Current practice (confirmed across all 13 modules):** Every module has both a unit test file and a component test file. When adding a new module or method: @@ -415,12 +463,14 @@ Reference: `test/component/lifecycleTest.cpp` (`subscribeOnState_JSON_RPC_compli **Current practice:** Both unit and component tests derive expected values from `jsonEngine.get_value("Module.method")` (the first example in the OpenRPC fixture). Do not hardcode values that duplicate the fixture unless the value requires a type conversion (e.g., enum comparison using `static_cast`). -Exception: `test/component/actionsGeneratedTest.cpp` hardcodes `"launch"` — this is permissible for auto-generated files and where the fixture specifies that exact value. +Exception: `test/component/actionsGeneratedTest.cpp` hardcodes `"launch"` (the `intent` field) and `1` (the `intentId` field) from the fixture's `Actions.intent` / `Actions.onIntent` example result — permissible only for auto-generated files. --- ## 11. OpenRPC Fixture Alignment +### 11.1 Fixture Examples and Enum Alignment + **Current practice:** - Fixture location: `docs/openrpc/the-spec/firebolt-open-rpc.json` - Both unit and component test binaries read this file at runtime (path injected via `UT_OPEN_RPC_FILE` define in `test/CMakeLists.txt`). @@ -429,6 +479,14 @@ Exception: `test/component/actionsGeneratedTest.cpp` hardcodes `"launch"` — th **Rule:** When a component test validates against `jsonEngine.get_value("Module.method")`, the fixture's example value must produce the same result as what the live mock-firebolt instance returns. Keep these in sync. +### 11.2 Fixture Metadata Rules + +**Module descriptions:** The `description` field for each module and method in the fixture must accurately describe the module's actual API behaviour. Do not copy-paste descriptions from other modules. + +**Property tags:** Getter-style methods must carry a `property:readonly` tag where other getter methods in the same file use this tag. Before adding a new getter method to the fixture, verify the tagging pattern used by the surrounding methods. + +**Notifier/subscriber metadata:** Subscription event entries must keep `x-notifier` and `x-subscriber-for` fields aligned with the corresponding getter or property. Adding a subscription event without updating both fields is a fixture defect. + --- ## 12. Auto-Generated vs Bespoke Code @@ -462,6 +520,28 @@ Do not hand-edit auto-generated files. If the generated output has a defect, fix When a new bespoke module is added, ensure it follows the same structure as generated modules (`actions`) so the two styles remain similar enough that the generator could own the bespoke code in the future. +### 12.4 Actions Module API Contract + +`Actions.intent` is a getter-only method: it takes no parameters and returns `Result`. Do not add parameters to it and do not change its return type. + +The `std::string` returned by `Actions.intent()` is a JSON-serialized object, not a plain scalar. Callers must parse it (confirmed in `test/component/actionsGeneratedTest.cpp`): +```cpp +auto result = accessor.ActionsInterface().intent(); +ASSERT_TRUE(result); +auto parsed = nlohmann::json::parse(*result); +EXPECT_EQ(parsed.at("intent").get(), "launch"); +EXPECT_EQ(parsed.at("intentId").get(), 1); +``` +Do not treat the return value as a plain scalar string. + +The `Actions.onIntent` callback also delivers a JSON-serialized object string. The component event trigger must use a JSON-encoded object payload (confirmed at `test/component/actionsGeneratedTest.cpp:62`): +```cpp +triggerEvent("Actions.onIntent", R"({"intent":"launch","intentId":1})") +``` +The callback receives the full JSON string and must parse it with `nlohmann::json::parse(intent)`. Do not use a plain string payload such as `R"("launch")"`. + +The OpenRPC fixture confirms: both `Actions.intent` and `Actions.onIntent` example results are `{"intent": "launch", "intentId": 1}` (verified in `docs/openrpc/the-spec/firebolt-open-rpc.json`). + --- ## 13. CMake and Build @@ -477,6 +557,8 @@ When a new bespoke module is added, ensure it follows the same structure as gene **Formatting enforced by CI:** `clang-format` with the project's `.clang-format` (LLVM-based, column limit 120, 4-space indent, Allman braces, C++17). Running `git ls-files -- '*.cpp' '*.h' | xargs clang-format --dry-run --Werror` is a required CI check. Do not bypass it. +**CI compatibility:** All build and source changes must remain compatible with the CI workflow. Do not modify build configuration in a way that passes locally but diverges from the Docker-based environment defined in `.github/workflows/ci.yml` and `.github/scripts/run-component-tests.sh`. + --- ## 14. Anti-Patterns Catalogue @@ -495,41 +577,45 @@ The following patterns are explicitly wrong for this codebase. Each entry notes | AP-8 | Adding `nlohmann::json` includes to `include/firebolt/*.h` | Public headers must not expose the JSON library as a transitive dependency | | AP-9 | Adding logging to `*_impl.cpp` | Logging is intentionally absent in module implementations; all diagnostics go through the transport layer | | AP-10 | Writing a new module that omits `subscribeOnStateChanged`-style subscription when the OpenRPC spec has `on*` events | Subscriptions are load-bearing API surface; omitting them silently breaks consumer event handling | -| AP-11 | Wrapping the event trigger payload in an extra JSON object in component tests | `triggerEvent("Actions.onIntent", R"("launch")")` — the payload for string events is the JSON-encoded string directly, not `{"value": "launch"}`. Check each module's test file for the correct payload shape | -| AP-12 | Using `#include ` or other heavyweight headers without a direct use | The project compiles with `-Wall -Wextra -Wpedantic`; unused includes produce warnings. The copilot-instructions.md explicitly calls out `` as an example | +| AP-11 | Assuming `Actions.onIntent` uses a plain string trigger payload | The actual payload is a JSON-encoded object: `triggerEvent("Actions.onIntent", R"({"intent":"launch","intentId":1})")` (confirmed at `test/component/actionsGeneratedTest.cpp:62`). Always check each module's component test file and the OpenRPC fixture for the correct payload shape before writing event trigger calls | +| AP-12 | Using `#include ` or other heavyweight headers without a direct use | The project compiles with `-Wall -Wextra -Wpedantic`; unused includes produce warnings. `` is the canonical example of a heavyweight header with no use in this codebase | | AP-13 | Defining a new public header without `#pragma once` | Bespoke headers require `#pragma once`; `#ifndef` guards are reserved for auto-generated output | | AP-14 | Modifying auto-generated files by hand | Files with `// AUTO-GENERATED by fb-gen — DO NOT EDIT` must be regenerated via the generator tool | --- -## 15. Assumptions and Open Flags +## 15. Explicit Assumptions -### Explicit Assumptions (`[ASSUMPTION]`) +The following items are inferred from code patterns where no explicit policy documentation existed. They are treated as policy until contradicted. | ID | Assumption | |---|---| | A-1 | `FIREBOLT_LOG_*` macros come from `FireboltTransport`. Individual module impls intentionally omit logging — inferred from the absence of any logging in 12 of 13 impl files. | | A-2 | The `using namespace Firebolt::Helpers;` pattern in `stats_impl.cpp` and `lifecycle_impl.cpp` is incidental rather than policy — inferred from its absence in the other 11 impl files. | -| A-3 | `StatsImpl`'s non-`explicit` constructor and non-`= default` destructor are legacy remnants, not intentional design. All other impls use `explicit` and `= default`. | +| A-3 | `StatsImpl`'s, `LifecycleImpl`'s, and `LocalizationImpl`'s non-`explicit` constructors are legacy remnants — confirmed at `src/stats_impl.h:30`, `src/lifecycle_impl.h:37`, `src/localization_impl.h:29`. `StatsImpl`'s non-`= default` destructor is also a remnant. All other impls use `explicit` and `= default`. | | A-4 | Wire names for TextToSpeech events (`onWillspeak`, `onSpeechstart`, etc.) are lowercase-concatenated because the Firebolt protocol lowercases them — inferred from the pattern in `src/texttospeech_impl.cpp` and the absence of a different naming convention for other modules' events. | -### Items Requiring Maintainer Confirmation (`[⚑ MAINTAINER]`) +--- -| ID | Question | -|---|---| -| M-1 | Should move constructor/move assignment be explicitly deleted in all existing `*Impl` classes? (The rule exists in `copilot-instructions.md` but is not yet applied to source code.) | -| M-2 | Should `using namespace Firebolt::Helpers;` at file scope in `.cpp` files be banned, made consistent, or left as-is? | -| M-3 | Should `explicit` be required on all `*Impl` constructors? (`StatsImpl` currently omits it.) | -| M-4 | Should `~StatsImpl()` and `~LifecycleImpl()` become `= default` since their bodies are empty? | -| M-5 | The `friend class ::LifecycleTest;` declaration in `src/lifecycle_impl.h` is the only friend declaration in the codebase. Should this be a policy for test-only access, or should it be removed? | -| M-6 | `src/device_impl.h` uses `u_int32_t` (non-standard POSIX type) for `timeInActiveState` return type while the public interface uses `uint32_t`. Should this be corrected? | +## 16. Relationship to Existing Policy Files + +This document is the single authoritative source for coding conventions and workflow rules in the `firebolt-cpp-client` repository. It supersedes `.github/copilot-instructions.md`, which has been absorbed into this document and deleted. + +`CONTRIBUTING.md` governs contribution process. This document governs code shape and workflow. --- -## 16. Relationship to Existing Policy Files +## 17. Test Execution Commands -This document **does not supersede** `.github/copilot-instructions.md`. That file contains the authoritative workflow rules (test commands, regeneration instructions, PR hygiene). This document provides the detailed coding-level guidelines that `.github/copilot-instructions.md` references but does not expand. +**Component tests (current preferred, local):** +```bash +./run-component-tests-local.sh +./run-component-tests-local.sh --skip-image-build # reuse existing Docker image +``` -Where the two documents conflict, `.github/copilot-instructions.md` takes precedence for workflow/process decisions; this document takes precedence for code structure decisions. +**Unit tests only:** +```bash +./run-unit-tests.sh +``` -`CONTRIBUTING.md` governs contribution process. This document governs code shape. +Always run component tests after any API-facing change. Component tests run in Docker against mock-firebolt and are the authoritative validation gate. From aaa7c6217e81e7350fffd8449b2bc5f9d54fbae1 Mon Sep 17 00:00:00 2001 From: swethasukumarr Date: Tue, 14 Jul 2026 13:29:33 -0400 Subject: [PATCH 3/6] RDKEMW-19315 : Address copilot comments --- .github/instructions/coding-guidelines.instructions.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/instructions/coding-guidelines.instructions.md b/.github/instructions/coding-guidelines.instructions.md index 4f0bfa2..97698dc 100644 --- a/.github/instructions/coding-guidelines.instructions.md +++ b/.github/instructions/coding-guidelines.instructions.md @@ -146,7 +146,7 @@ After the change, run `./run-component-tests-local.sh` to validate all layers to ### 3.2 Include Style -**Current practice (confirmed across all 14 modules):** +**Current practice (confirmed across all 13 modules):** - Includes of public firebolt headers: angle brackets with full path (`#include `, `#include `). - Includes of implementation-local headers: double quotes without path prefix (`#include "device_impl.h"`, `#include "json_types/device.h"`). - Includes of the module's own public header from within `_impl.h`: double quotes with full path (`#include "firebolt/device.h"`). @@ -553,7 +553,7 @@ The OpenRPC fixture confirms: both `Actions.intent` and `Actions.onIntent` examp - New test files are picked up automatically via `file(GLOB UNIT_TESTS CONFIGURE_DEPENDS unit/*Test.cpp)` and `file(GLOB COMPONENT_TESTS CONFIGURE_DEPENDS component/*Test.cpp)`. - Export macro: `FIREBOLTCLIENT_EXPORT` from the generated `firebolt/client_export.h`. Apply to public symbols in `include/firebolt/firebolt.h`. -**Anti-pattern:** Do not manually list sources in `src/CMakeLists.txt` — the glob handles this. Do not introduce a new `CMakeLists.txt` inside `src/` or `test/` subdirectories. +**Anti-pattern:** Do not manually list sources in `src/CMakeLists.txt` — the glob handles this. Do not introduce new `CMakeLists.txt` files inside nested subdirectories under `src/` or `test/` (e.g., `src/json_types/`, `test/unit/`, `test/component/`). The top-level `src/CMakeLists.txt` and `test/CMakeLists.txt` already exist and must not be replaced. **Formatting enforced by CI:** `clang-format` with the project's `.clang-format` (LLVM-based, column limit 120, 4-space indent, Allman braces, C++17). Running `git ls-files -- '*.cpp' '*.h' | xargs clang-format --dry-run --Werror` is a required CI check. Do not bypass it. @@ -578,7 +578,7 @@ The following patterns are explicitly wrong for this codebase. Each entry notes | AP-9 | Adding logging to `*_impl.cpp` | Logging is intentionally absent in module implementations; all diagnostics go through the transport layer | | AP-10 | Writing a new module that omits `subscribeOnStateChanged`-style subscription when the OpenRPC spec has `on*` events | Subscriptions are load-bearing API surface; omitting them silently breaks consumer event handling | | AP-11 | Assuming `Actions.onIntent` uses a plain string trigger payload | The actual payload is a JSON-encoded object: `triggerEvent("Actions.onIntent", R"({"intent":"launch","intentId":1})")` (confirmed at `test/component/actionsGeneratedTest.cpp:62`). Always check each module's component test file and the OpenRPC fixture for the correct payload shape before writing event trigger calls | -| AP-12 | Using `#include ` or other heavyweight headers without a direct use | The project compiles with `-Wall -Wextra -Wpedantic`; unused includes produce warnings. `` is the canonical example of a heavyweight header with no use in this codebase | +| AP-12 | Using `#include ` or other heavyweight headers without a direct use | Unnecessary includes increase compile time and leak transitive dependencies into consumers' include graphs. `-Wall -Wextra -Wpedantic` do not warn on unused includes; keep includes minimal as a discipline, not for warning suppression. `` is the canonical example of a heavyweight header with no use in this codebase | | AP-13 | Defining a new public header without `#pragma once` | Bespoke headers require `#pragma once`; `#ifndef` guards are reserved for auto-generated output | | AP-14 | Modifying auto-generated files by hand | Files with `// AUTO-GENERATED by fb-gen — DO NOT EDIT` must be regenerated via the generator tool | From 1cd3c9991337619713da614511b8609c78128230 Mon Sep 17 00:00:00 2001 From: swethasukumarr Date: Tue, 14 Jul 2026 13:36:40 -0400 Subject: [PATCH 4/6] RDKEMW-19315 : Address copilot comments --- .github/instructions/coding-guidelines.instructions.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/instructions/coding-guidelines.instructions.md b/.github/instructions/coding-guidelines.instructions.md index 97698dc..e2eeb38 100644 --- a/.github/instructions/coding-guidelines.instructions.md +++ b/.github/instructions/coding-guidelines.instructions.md @@ -44,11 +44,11 @@ Every new module must be wired into `src/firebolt.cpp`: 1. Member variable of type `Module::ModuleImpl` in `FireboltAccessorImpl`. 2. Initialised via `Firebolt::Helpers::GetHelperInstance()` in the constructor initialiser list. 3. Accessor method returning `Module::IModule&` override. -4. `unsubscribeAll()` call in the private `unsubscribeAll()` helper — **only if the module supports subscriptions**. +4. `unsubscribeAll()` call in the private `unsubscribeAll()` helper — **only if the module supports subscriptions** (i.e., the interface exposes any `subscribeOn*` methods). Note: `Device` currently exposes `subscribeOnHdrChanged(...)` but is absent from `FireboltAccessorImpl::unsubscribeAll()` — see Known Issue below. -Reference: `src/firebolt.cpp` lines 44–110. +Reference: `src/firebolt.cpp` (`FireboltAccessorImpl` ctor initializer list and `unsubscribeAll()`). -**Cleanup to fix:** `src/firebolt.cpp` — `device_.unsubscribeAll()` is absent from `unsubscribeAll()` (lines 91–99) despite `DeviceImpl` holding a `SubscriptionManager` and `IDevice` exposing `subscribeOnHdrChanged` and `subscribeOnDolbyAtmosExperienceAvailableChanged`. Device subscriptions are never cleaned up on Disconnect. Add `device_.unsubscribeAll();` alongside the other module calls in that method. +**Known issue:** `FireboltAccessorImpl::unsubscribeAll()` in `src/firebolt.cpp` does not call `device_.unsubscribeAll()` even though `DeviceImpl` supports subscriptions (`subscribeOnHdrChanged`, `subscribeOnDolbyAtmosExperienceAvailableChanged`). Device subscriptions are never cleaned up on Disconnect. Track this as a separate defect. ### 1.3 API-Facing Change Discipline @@ -138,7 +138,7 @@ After the change, run `./run-component-tests-local.sh` to validate all layers to **Current practice:** - Bespoke headers (`include/firebolt/` and most `src/`): use `#pragma once`. -- Auto-generated **interface and impl** headers (e.g., `include/firebolt/actions.h`, `src/actions_impl.h`): use `#ifndef FIREBOLT__H` / `#define FIREBOLT__H` / `#endif` guards — confirmed at `include/firebolt/actions.h:22` and `src/actions_impl.h:22`. +- Auto-generated **interface and impl** headers (e.g., `include/firebolt/actions.h`, `src/actions_impl.h`): use `#ifndef FIREBOLT__H` / `#define FIREBOLT__H` / `#endif` guards — confirmed in `include/firebolt/actions.h` and `src/actions_impl.h`. - Auto-generated **json_types** headers (e.g., `src/json_types/actions.h`): use `#pragma once` — consistent with all bespoke json_types headers. - Do not mix both guards in the same file. @@ -167,7 +167,7 @@ Confirmed observation: `include/firebolt/actions.h` (auto-generated) includes `< ### 4.1 `*Impl` Class Declaration Order -**Current practice (confirmed in all 13 impl headers):** +**Recommended going forward:** ``` class Impl : public I { From a79c58b458501539759f4ef25158042e61edd20a Mon Sep 17 00:00:00 2001 From: swethasukumarr Date: Tue, 14 Jul 2026 13:41:12 -0400 Subject: [PATCH 5/6] RDKEMW-19315 : Address copilot comments --- .github/instructions/coding-guidelines.instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/instructions/coding-guidelines.instructions.md b/.github/instructions/coding-guidelines.instructions.md index e2eeb38..b1983a9 100644 --- a/.github/instructions/coding-guidelines.instructions.md +++ b/.github/instructions/coding-guidelines.instructions.md @@ -76,7 +76,7 @@ After the change, run `./run-component-tests-local.sh` to validate all layers to - Every `.h` and `.cpp` file closes with a namespace-end comment: `} // namespace Firebolt::`. **Anti-patterns:** -- Do not use `using namespace Firebolt::;` in .cpp files either. Use fully-qualified names or targeted `using Firebolt::Helpers::ClassName;` declarations instead. (Applies retroactively to `stats_impl.cpp` and `lifecycle_impl.cpp` — flagged for cleanup.) +- Do not use `using namespace Firebolt::Helpers;` (or `using namespace Firebolt::;`) in `.cpp` files. Prefer fully-qualified names or targeted `using Firebolt::Helpers::ClassName;` declarations instead. (Applies retroactively to `stats_impl.cpp` and `lifecycle_impl.cpp` — both currently have `using namespace Firebolt::Helpers;` at file scope — flagged for cleanup.) ### 2.2 Interfaces and Implementations From 7c1ec9da94c816dd56256f4d064202475e6a5d34 Mon Sep 17 00:00:00 2001 From: swethasukumarr Date: Tue, 14 Jul 2026 13:49:01 -0400 Subject: [PATCH 6/6] RDKEMW-19315 : Address copilot comments --- .../coding-guidelines.instructions.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/instructions/coding-guidelines.instructions.md b/.github/instructions/coding-guidelines.instructions.md index b1983a9..51d451e 100644 --- a/.github/instructions/coding-guidelines.instructions.md +++ b/.github/instructions/coding-guidelines.instructions.md @@ -29,7 +29,7 @@ The codebase has exactly three conceptual layers. Do not collapse or skip layers | Implementation | `src/_impl.h` + `src/_impl.cpp` | Concrete logic; hidden from consumers | | JSON deserialization | `src/json_types/.h` | Wire format ↔ native type adapters; never exposed publicly | -**Current practice (confirmed across all 13 modules):** +**Current practice (confirmed across all existing modules):** - Every module has exactly one interface (`I`), one impl (`Impl`), and zero or more JSON type adapters. Four modules (discovery, localization, network, presentation) have no `json_types/` file; they use primitive-type helpers (`Firebolt::JSON::String`, `Boolean`, `Unsigned`) directly. - The singleton entry point is `Firebolt::IFireboltAccessor::Instance()`, implemented in `src/firebolt.cpp` as a Meyers singleton over `FireboltAccessorImpl`. @@ -146,7 +146,7 @@ After the change, run `./run-component-tests-local.sh` to validate all layers to ### 3.2 Include Style -**Current practice (confirmed across all 13 modules):** +**Current practice (confirmed across all existing modules):** - Includes of public firebolt headers: angle brackets with full path (`#include `, `#include `). - Includes of implementation-local headers: double quotes without path prefix (`#include "device_impl.h"`, `#include "json_types/device.h"`). - Includes of the module's own public header from within `_impl.h`: double quotes with full path (`#include "firebolt/device.h"`). @@ -188,9 +188,9 @@ private: ``` - All `*Impl` constructors take `Firebolt::Helpers::IHelper&` as their only parameter and must be marked `explicit`. Three impl classes currently lack `explicit` — flagged for cleanup: - - `src/stats_impl.h` — `StatsImpl(Firebolt::Helpers::IHelper&)` at line 30 - - `src/lifecycle_impl.h` — `LifecycleImpl(Firebolt::Helpers::IHelper&)` at line 37 - - `src/localization_impl.h` — `LocalizationImpl(Firebolt::Helpers::IHelper&)` at line 29 + - `src/stats_impl.h` — `StatsImpl(Firebolt::Helpers::IHelper&)` + - `src/lifecycle_impl.h` — `LifecycleImpl(Firebolt::Helpers::IHelper&)` + - `src/localization_impl.h` — `LocalizationImpl(Firebolt::Helpers::IHelper&)` - Method return types and parameter types in `*_impl.h` must exactly match those declared in the corresponding public interface header. Always use `uint32_t` (from ``), never the non-standard POSIX type `u_int32_t`. A type mismatch between interface and override produces an invalid override and fails to compile on non-POSIX targets. **Cleanup to fix:** `src/device_impl.h` — `timeInActiveState()` is declared as `u_int32_t`; must be changed to `uint32_t` to match `include/firebolt/device.h`. @@ -246,7 +246,7 @@ Do not use `friend` declarations to grant test classes access to implementation ### 5.1 `Result` at the API Boundary -**Current practice (without exception in 13 modules):** +**Current practice (without exception across all existing modules):** - Every method in `I` returns `Result` or `Result`. - `Result` is used for methods that send a command and carry no return value (e.g., `Metrics.ready()`, `Lifecycle.close()`). - Callers check the result with boolean conversion (`if (result)`) or dereference after assertion (`*result`). @@ -453,7 +453,7 @@ Negative tests must verify runtime behaviour — specifically that callbacks are ### 10.3 Pairing Rule -**Current practice (confirmed across all 13 modules):** Every module has both a unit test file and a component test file. When adding a new module or method: +**Current practice (confirmed across all existing modules):** Every module has both a unit test file and a component test file. When adding a new module or method: - Add unit tests in `test/unit/Test.cpp` - Add component tests in `test/component/Test.cpp` - Both test files must cover all public API methods @@ -567,7 +567,7 @@ The following patterns are explicitly wrong for this codebase. Each entry notes | # | Anti-Pattern | Why It Is Wrong Here | |---|---|---| -| AP-1 | Returning `std::optional` instead of `Result` from interface methods | Cannot carry an error code; breaks the uniform error contract used across all 13 modules | +| AP-1 | Returning `std::optional` instead of `Result` from interface methods | Cannot carry an error code; breaks the uniform error contract used across all modules | | AP-2 | Throwing exceptions from `*_impl.cpp` method bodies | Consumers expect `Result`; exceptions escape the module boundary unexpectedly | | AP-3 | Adding `unique_ptr` or `shared_ptr` for module ownership in `FireboltAccessorImpl` | All modules are owned by value in `FireboltAccessorImpl`; smart pointers add indirection with no benefit here | | AP-4 | Storing `IHelper` by pointer | Consistent reference storage; pointer would allow null and is not the established contract |