diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 4e29ed9af..9863dcd46 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -2,4 +2,4 @@ # the repo. Unless a later match takes precedence, # @global-owner1 and @global-owner2 will be requested for # review when someone opens a pull request. -* @rdkcentral/rdke_ghec_tr69_maintainer @rdkcentral/rdke_ghec_tr69_admin +* @rdkcentral/tr69hostif-maintainers diff --git a/.github/README.md b/.github/README.md new file mode 100644 index 000000000..7e54a8d66 --- /dev/null +++ b/.github/README.md @@ -0,0 +1,453 @@ +# tr69hostif — TR-069 Host Interface Manager + +[![License](https://img.shields.io/badge/License-Apache%202.0-blue.svg)](LICENSE) +[![Version](https://img.shields.io/badge/version-1.3.6-green.svg)](CHANGELOG.md) + +## Overview + +`tr69hostif` is the **TR-069 Host Interface Manager** for RDK-based devices. It acts as the central broker between remote management infrastructure (ACS, WebPA/Parodus) and the TR-181 data model implemented on the device. Any component that needs to read or write TR-181 parameters — including the CWMP stack, WebPA gateway, and RFC override system — routes its requests through `tr69hostif`. + +The daemon runs as a persistent systemd service, initializes all TR-181 profile handlers at startup, and then services get/set requests over multiple IPC channels simultaneously. + +## Architecture + +### High-Level Component Diagram + +```mermaid +graph TB + subgraph Remote["Remote Callers"] + ACS[ACS / CWMP Stack] + WebPA[WebPA / parodus] + RBUS[RBUS Clients] + end + + subgraph tr69hostif["tr69hostif Daemon"] + IARM[IARM-Bus IPC Handler] + JSON[JSON Request Handler\nPort 10999] + RBUS_P[RBUS DML Provider] + PAR[Parodus PAL\nlibpd] + UPD[Update Handler\nValue Change Events] + MSG[Message Dispatcher\nhostIf_msgHandler] + + subgraph Profiles["TR-181 Profile Handlers"] + DEV[DeviceInfo] + WIFI[WiFi] + ETH[Ethernet] + IP[IP] + MOCA[MoCA] + TIME[Time] + DHCP[DHCPv4] + STBS[STBService\nDS Profile] + STOR[StorageService] + INTF[InterfaceStack] + end + + subgraph RFC["RFC / Bootstrap"] + RFC_S[RFC Store\nXRFCStorage] + BS_S[Bootstrap Store\nXBSStore] + end + end + + ACS -->|IARM RPC| IARM + WebPA-->|msgpack/WRP| PAR + RBUS -->|rbus API| RBUS_P + JSON -->|HTTP JSON| MSG + + IARM --> MSG + PAR --> MSG + RBUS_P --> MSG + MSG --> Profiles + MSG --> RFC + UPD -->|ValueChanged| IARM + UPD -->|ValueChanged| PAR +``` + +### Request Flow + +```mermaid +sequenceDiagram + participant Caller as Caller (IARM/RBUS/WebPA) + participant MSG as Message Dispatcher + participant PROF as Profile Handler + participant HAL as Platform HAL / OS + + Caller->>MSG: Get/Set paramName + value + MSG->>MSG: Route by prefix (mgrlist.conf) + MSG->>PROF: handler->handleGetMsg() / handleSetMsg() + PROF->>HAL: Read device state / write config + HAL-->>PROF: Raw value + PROF-->>MSG: Populated HOSTIF_MsgData_t + MSG-->>Caller: Response + faultCode +``` + +### Startup Sequence + +```mermaid +sequenceDiagram + participant main as main() + participant CFG as ConfigManager + participant IARM as IARM-Bus + participant DM as DataModel XML + participant THR as Threads + + main->>CFG: hostIf_initalize_ConfigManger() + main->>IARM: hostIf_IARM_IF_Start() + main->>DM: mergeDataModel() + loadDataModel() + main->>THR: json_if_handler_thread (GLib) + main->>THR: http_server_thread (optional, legacy RFC) + main->>THR: updateHandler::Init() (value-change polling) + main->>THR: libpd_client_mgr() (Parodus, if enabled) + main->>THR: initWebConfigTask() (WebConfig, if enabled) + main->>main: init_rbus_dml_provider() + main->>main: sd_notify(READY=1) + main->>main: g_main_loop_run() +``` + +## Key Components + +### Core Daemon (`src/hostif/src/`) + +| File | Purpose | +|------|---------| +| `hostIf_main.cpp` | `main()` entry point: argument parsing, signal handling, thread lifecycle, GLib main loop | +| `hostIf_utils.cpp` | Utility helpers: type conversion, reset state machine, gateway connectivity | +| `IniFile.cpp` | INI file parser used by RFC and Bootstrap stores | + +### Request Handlers (`src/hostif/handlers/`) + +| Handler | IARM Bus Manager Token | TR-181 Subtree | +|---------|----------------------|----------------| +| `hostIf_DeviceClient_ReqHandler` | `deviceMgr` | `Device.DeviceInfo.*` | +| `hostIf_EthernetClient_ReqHandler` | `ethernetMgr` | `Device.Ethernet.*` | +| `hostIf_IPClient_ReqHandler` | `ipMgr` | `Device.IP.*` | +| `hostIf_MoCAClient_ReqHandler` | `mocaMgr` | `Device.MoCA.*` | +| `hostIf_TimeClient_ReqHandler` | `timeMgr` | `Device.Time.*` | +| `hostIf_DHCPv4Client_ReqHandler` | `dhcpv4Mgr` | `Device.DHCPv4.*` | +| `hostIf_dsClient_ReqHandler` | `dsMgr` | `Device.Services.STBService.*` | +| `hostIf_StorageSrvc_ReqHandler` | `storageSrvcMgr` | `Device.Services.StorageService.*` | +| `hostIf_InterfaceStackClient_ReqHandler` | `intfStackMgr` | `Device.InterfaceStack.*` | +| `hostIf_rbus_Dml_Provider` | — | Exposes all registered params over RBUS | +| `hostIf_updateHandler` | — | Polls profiles for value changes; publishes IARM events | +| `hostIf_NotificationHandler` | — | Queues value-change notifications to Parodus | + +All handlers inherit from the abstract `msgHandler` base class. The `hostIf_msgHandler.cpp` dispatcher instantiates each handler at startup and routes requests by matching the parameter name prefix against the manager map loaded from `tr69hostIf.conf`. + +### TR-181 Profiles (`src/hostif/profiles/`) + +Each subdirectory implements one or more TR-181 objects. Profiles contain the business logic: they read HAL APIs (IARM Device Settings, wifihal, platform sysfs, etc.) and translate results to/from `HOSTIF_MsgData_t`. + +| Profile Directory | TR-181 Object | Key Dependencies | +|-------------------|---------------|-----------------| +| `DeviceInfo/` | `Device.DeviceInfo` | IARM, rfcapi, rfcdefaults, partners\_defaults.json | +| `Ethernet/` | `Device.Ethernet` | sysfs, IARM | +| `IP/` | `Device.IP` | netlink / sysfs | +| `moca/` | `Device.MoCA` | IARM mocaMgr | +| `Time/` | `Device.Time` | NTP daemon, chrony | +| `DHCPv4/` | `Device.DHCPv4` | udhcpc / dnsmasq | +| `STBService/` | `Device.Services.STBService` | IARM Device Settings (DS) | +| `StorageService/` | `Device.Services.StorageService` | sysfs block devices | +| `InterfaceStack/` | `Device.InterfaceStack` | sysfs | +| `Device/` | `Device.*` (root object) | — | + +### RFC & Bootstrap Subsystem (`src/hostif/profiles/DeviceInfo/`) + +| Class | File | Purpose | +|-------|------|---------| +| `XRFCStorage` | `XrdkCentralComRFC.cpp` | Persists RFC override values in an INI file under `/opt/secure/RFC/` | +| `XBSStore` | `XrdkCentralComBSStore.cpp` | Loads per-partner bootstrap defaults from `partners_defaults.json`; owns the background partner-ID resolution thread | +| `XBSStoreJournal` | `XrdkCentralComBSStoreJournal.cpp` | Append-only journal for bootstrap value changes | + +RFC parameter precedence (highest to lowest): + +``` +RFC Override (/opt/secure/RFC/) > WebPA Set > Bootstrap Default > Firmware Default +``` + +### Parodus / WebPA Client (`src/hostif/parodusClient/pal/`) + +| File | Purpose | +|------|---------| +| `libpd.cpp` | Connects to `parodus` process; manages the recv-wait thread | +| `webpa_adapter.cpp` | Translates libparodus WRP messages to `HOSTIF_MsgData_t` | +| `webpa_parameter.cpp` | GetParam / SetParam over WebPA | +| `webpa_attribute.cpp` | GetAttr / SetAttr over WebPA | +| `webpa_notification.cpp` | Pushes value-change events back to parodus | + +### HTTP Server (`src/hostif/httpserver/`) + +An optional Mongoose-based HTTP server (disabled when `NEW_HTTP_SERVER_DISABLE` is defined or when the Legacy RFC feature flag is active). Provides a local REST endpoint used during RFC migration. Controlled at runtime by `/opt/RFC/.RFC_LegacyRFCEnabled.ini`. + +## Threading Model + +| Thread | Name | How Created | Purpose | +|--------|------|------------|---------| +| Main | `main` | OS | Init, GLib main loop | +| Shutdown | `shutdown_thread` | `pthread_create` | Waits on semaphore; calls `exit_gracefully()` on signal | +| JSON Handler | `json_if_handler_thread` | `g_thread_try_new` | Services JSON-over-socket requests | +| HTTP Server | `http_server_thread` | `g_thread_try_new` | Optional legacy HTTP RFC endpoint | +| Update Handler | `updateHandler` | `g_thread_try_new` | Polls profiles for value changes; fires IARM / Parodus events | +| Parodus Init | `parodus_init_tid` | `pthread_create` | Connects to parodus daemon, starts recv loop | +| WebConfig | `webconfig_threadId` | `pthread_create` | Handles WebConfig Lite document processing | +| Partner ID | `partnerIdThread` | `std::thread` (inside `XBSStore`) | Resolves partner ID asynchronously at boot | + +### Synchronization + +```c +// Signal → shutdown path +sem_t shutdown_thread_sem; // Main signals shutdown thread +pthread_mutex_t graceful_exit_mutex; // Protects shutdown sequence + +// HTTP server startup handshake +std::mutex mtx_httpServerThreadDone; +std::condition_variable cv_httpServerThreadDone; + +// Bootstrap store +static recursive_mutex XBSStore::mtx; // Guards m_dict cache +static mutex XBSStore::mtx_stopped; +static condition_variable XBSStore::cv; + +// Notification queue (lock-free) +GAsyncQueue* NotificationHandler::notificationQueue; +``` + +**Lock ordering**: No nested lock acquisitions exist across manager threads; each subsystem owns its own mutex. The GLib `GAsyncQueue` is used for the notification path to avoid blocking the update handler. + +## Data Structures + +### `HOSTIF_MsgData_t` — the universal request/response envelope + +```c +typedef struct _HostIf_MsgData_t { + char paramName[4096]; // Full TR-181 parameter path + char paramValue[4096]; // Value as string + char *paramValueLong; // Heap buffer for values > 4096 bytes + char transactionID[256]; // Correlation ID (WebPA / CWMP) + short paramLen; // Byte length of paramValue + short instanceNum; // Object instance number + HostIf_ParamType_t paramtype; // String/Int/Bool/DateTime/ULong + HostIf_ReqType_t reqType; // GET / SET / GETATTRIB / SETATTRIB + faultCode_t faultCode; // TR-069 fault code (0 = success) + HostIf_Source_Type_t requestor; // WEBPA / RFC / IARM / DEFAULT + HostIf_Source_Type_t bsUpdate; // Bootstrap source level + bool isLengthyParam; // true → use paramValueLong +} HOSTIF_MsgData_t; +``` + +### Fault Codes + +| Code | Name | Meaning | +|------|------|---------| +| 0 | `fcNoFault` | Success | +| 9000 | `fcMethodNotSupported` | RPC not implemented | +| 9001 | `fcRequestDenied` | Access denied | +| 9002 | `fcInternalError` | Unexpected internal failure | +| 9003 | `fcInvalidArguments` | Bad arguments | +| 9004 | `fcResourcesExceeded` | Resource limit hit | +| 9005 | `fcInvalidParameterName` | Unknown parameter | +| 9006 | `fcInvalidParameterType` | Type mismatch | +| 9007 | `fcInvalidParameterValue` | Value out of range or invalid | +| 9008 | `fcAttemptToSetaNonWritableParameter` | Read-only parameter | + +## Configuration + +### `conf/tr69hostIf.conf` + +```ini +[HOSTIF_DM_PROFILE_MGR] +Device.DeviceInfo=deviceMgr +Device.Services.STBService=dsMgr +Device.Services.StorageService=storageSrvcMgr +Device.MoCA=mocaMgr +Device.Ethernet=ethernetMgr +Device.IP=ipMgr +Device.Time=timeMgr + +[HOSTIF_JSON_CONFIG] +PORT=10999 + +[HOSTIF_CONFIG] +REBOOT_SCR="/rebootNow.sh -s tr69hostIfReset" +RDK_SCR_PATH=/lib/rdk +NTP_FILE_NAME=/opt/persistent/firstNtpTime +FW_DWN_FILE_PATH=/opt/fwdnldstatus.txt +``` + +The `[HOSTIF_DM_PROFILE_MGR]` section defines which manager handles each TR-181 subtree prefix. The dispatcher matches incoming parameter names against these prefixes to route requests. + +### Runtime Feature Flags (RFC) + +| Path | Feature | +|------|---------| +| `/opt/RFC/.RFC_LegacyRFCEnabled.ini` | Enable legacy HTTP server instead of new HTTP server | +| `/opt/secure/RFC/.RFC_.ini` | General RFC feature toggles (created by `XRFCStorage`) | +| `/opt/debug.ini` | RDK logger configuration | + +### Build-Time Feature Flags (`configure.ac`) + +| Configure Flag | Preprocessor Define | Effect | +|----------------|--------------------|----| +| `--enable-parodus` | `PARODUS_ENABLE` | Enable WebPA/Parodus client | +| `--disable-new-http-server` | `NEW_HTTP_SERVER_DISABLE` | Remove internal HTTP server | +| `--enable-webpa-rfc` | `WEBPA_RFC_ENABLED` | Guard service on RFC flag | +| `--enable-rbus` | *(rbus linkage)* | Enable RBUS DML provider | +| `--enable-t2` | `T2_EVENT_ENABLED` | Telemetry 2.0 markers | +| `--enable-webconfig` | `WEB_CONFIG_ENABLED` | WebConfig multipart support | +| `--enable-webconfig-lite` | `WEBCONFIG_LITE_ENABLE` | WebConfig Lite | +| `--enable-moca` | *(moca linkage)* | MoCA profile handlers | + +## Build & Install + +### Prerequisites + +| Dependency | Minimum Version | Notes | +|------------|----------------|-------| +| GCC / G++ | 7+ | C++17 required | +| GLib 2 | 2.32+ | GThread, GMainLoop, GAsyncQueue | +| libcurl | 7.65+ | Used by DeviceInfo utilities | +| IARM Bus | — | RDK platform IPC | +| libparodus | — | Required with `--enable-parodus` | +| rbus | — | Required with `--enable-rbus` | +| safec | — | Safe string functions (`strcpy_s`, etc.) | +| cJSON | — | JSON parsing | +| OpenSSL | 1.1.1+ | TLS for HTTP server | + +### Build Steps + +```bash +# Generate build system +autoreconf -iv + +# Configure (example for a typical RDK broadband build) +./configure \ + --enable-parodus \ + --enable-rbus \ + --enable-moca \ + --enable-t2 + +# Build +make -j$(nproc) + +# Install +make install +``` + +### Run + +```bash +# Typical invocation (as managed by systemd) +/usr/bin/tr69hostIf -c /etc/tr69hostIf.conf -p 10000 + +# Options +# -c Configuration file path +# -p IARM listen port +# -s HTTP server port (legacy mode only) +# -l Log file path +# -h Show usage +``` + +The provided systemd unit files are: +- `tr69hostif.service` — standard deployment +- `tr69hostif_no_new_http_server.service` — deployment with `NEW_HTTP_SERVER_DISABLE` + +## Testing + +### Unit Tests + +```bash +# Build and run unit tests +./run_ut.sh +``` + +Unit tests live under `src/unittest/` and `src/hostif/**/gtest/`. They use **Google Test** and rely on stub headers under `src/unittest/stubs/` to isolate the daemon from IARM, DS, and other platform dependencies. + +Key test areas: + +| Test Suite | Location | Coverage | +|------------|----------|----------| +| RFC Store | `profiles/DeviceInfo/gtest/` | `XRFCStorage` get/set/clear | +| Bootstrap Store | `profiles/DeviceInfo/gtest/` | `XBSStore` partner loading | +| JSON Handler | `handlers/src/gtest/` | Request parsing and routing | +| IARM Handler | `handlers/src/gtest/` | IARM RPC dispatch | +| IniFile | `src/gtest/` | INI parser correctness | + +### Integration / L2 Tests + +```bash +# Run L2 integration tests (requires Docker) +./run_l2.sh +``` + +L2 tests live under `src/integrationtest/` (configuration fixtures) and `test/functional-tests/` (Behave BDD scenarios). They exercise the full daemon end-to-end against mock IARM and RFC infrastructure. + +## Directory Reference + +``` +tr69hostif/ +├── configure.ac # Autoconf top-level +├── Makefile.am # Top-level Automake +├── conf/ # Runtime configuration +│ ├── tr69hostIf.conf # Manager-to-prefix mapping +│ ├── mgrlist.conf # Manager list +│ └── rfcdefaults/ +│ └── tr69hostif.ini # RFC default values +├── src/ +│ ├── backgroundrun.c # Helper to run scripts in background +│ └── hostif/ +│ ├── src/ # Core daemon source +│ ├── include/ # Core public headers +│ ├── handlers/ # Request dispatching layer +│ ├── profiles/ # TR-181 object implementations +│ ├── parodusClient/ # WebPA / Parodus PAL +│ ├── httpserver/ # Optional HTTP server +├── test/ +│ └── functional-tests/ # BDD integration tests (Behave) +└── scripts/ + └── validateDataModel.py # Data model XML validation utility +``` + +## Logging + +tr69hostif uses the RDK Logger (`rdk_debug.h`). Log levels map to standard RDK levels: `FATAL`, `ERROR`, `WARN`, `NOTICE`, `INFO`, `DEBUG`, `TRACE1/2`. + +The log category is `LOG_TR69HOSTIF`. To enable verbose logging at runtime, add the following to `/opt/debug.ini`: + +```ini +LOG.RDK.TR69HOSTIF = DEBUG +``` + +Telemetry 2.0 markers (when `T2_EVENT_ENABLED` is defined) are emitted via `t2_event_s()` / `t2_event_d()` for key lifecycle events. + +## Platform Notes + +### RDKB (Broadband Gateway) +- Uses IARM-Bus for all cross-process communication. +- WiFi parameters delegate to the `wifihal` abstraction layer. +- RFC overrides stored under `/opt/secure/RFC/`. +- Bootstrap defaults loaded from `/etc/partners_defaults.json` or `/opt/partners_defaults.json`. + +### RDKV (Video/STB) +- `RDKV_TR69` compile flag activates STB-specific code paths. +- DS (Device Settings) profile enabled; STBService provides HDMI, FPD, audio, and video object support. +- Base data model file: `/etc/data-model.xml` merged with device-type overlays at startup. + +### General Constraints +- Minimum 64 MB RAM recommended. +- ARMv7 or better CPU. +- GLib 2 event loop required (no bare POSIX event loop replacement). + +## Contributing + +See [CONTRIBUTING.md](CONTRIBUTING.md). All contributions require signing the RDK Contributor License Agreement. + +## License + +Licensed under the [Apache License, Version 2.0](LICENSE). + +Copyright 2016 RDK Management. + +## See Also + +- [CHANGELOG](CHANGELOG.md) — Release history +- [conf/tr69hostIf.conf](conf/tr69hostIf.conf) — Runtime configuration reference +- [run_ut.sh](run_ut.sh) — Unit test runner +- [run_l2.sh](run_l2.sh) — L2 integration test runner diff --git a/.github/agents/embedded-programmer.agent.md b/.github/agents/embedded-programmer.agent.md new file mode 100644 index 000000000..1993bd4a1 --- /dev/null +++ b/.github/agents/embedded-programmer.agent.md @@ -0,0 +1,177 @@ +--- +name: 'Embedded Programming Expert' +description: 'Expert in embedded C++ development with focus on resource constraints, memory safety, and platform independence for tr69hostif / TR-069 host interface systems' +tools: ['codebase', 'search', 'edit', 'runCommands', 'runTests', 'problems', 'web'] +--- + +# Embedded C++ Development Expert + +You are an expert embedded systems C++ developer specializing in resource-constrained environments. You have deep knowledge of: + +- Memory management without garbage collection +- Platform-independent C/C++ programming +- Real-time and embedded systems constraints +- RDK (Reference Design Kit) architecture +- TR-069/TR-181 data model management and CWMP protocol +- tr69hostif architecture: handlers, profiles, WebPA/parodus integration, RFC parameter management + +## Your Expertise + +### Memory Management +- RAII patterns in C using cleanup functions +- Memory pools and custom allocators +- Fragmentation prevention strategies +- Stack vs heap tradeoffs +- Valgrind and memory leak detection + +### Thread Safety and Concurrency +- Lightweight synchronization primitives (atomic operations, simple mutexes) +- Deadlock prevention (lock ordering, timeouts) +- Minimal thread memory configuration (pthread attributes) +- Lock-free patterns for embedded systems +- Thread pool design to prevent fragmentation +- Race condition detection and prevention + +### Resource Optimization +- Minimal CPU usage patterns +- Code size reduction techniques +- Static memory allocation strategies +- Efficient data structures for embedded systems +- Zero-copy techniques + +### Platform Independence +- POSIX compliance +- Endianness handling +- Type size portability (stdint.h) +- Build system abstractions +- Hardware abstraction layers + +### Code Quality +- Static analysis (cppcheck, scan-build) +- Unit testing with gtest/gmock from C +- Coverage analysis +- Defensive programming +- Error handling patterns + +## Your Approach + +### When Reviewing Code +1. Check for memory leaks (every malloc needs a free) +2. Verify error handling (all return values checked) +3. Validate resource cleanup (files, mutexes, etc.) +4. Ensure platform independence (no assumptions) +5. Look for buffer overflows and bounds checking +6. Verify thread safety if multi-threaded +7. Check for proper synchronization (no race conditions, no deadlocks) +8. Validate thread creation uses minimal stack attributes +9. Ensure lock-free patterns used where appropriate + +### When Writing Code +1. Start with function signature and error handling +2. Document ownership and lifetime of pointers +3. Use single exit point pattern for cleanup +4. Add bounds checking and validation +5. Write corresponding tests +6. Run valgrind to verify no leaks + +### When Refactoring +1. Don't change behavior (verify with tests) +2. Reduce memory footprint when possible +3. Improve error handling and logging +4. Extract common patterns into functions +5. Maintain backward compatibility +6. Update tests to match changes + +## Guidelines + +### Memory Safety +- Always check malloc/calloc return values +- Free memory in reverse order of allocation +- Use goto for cleanup in complex error paths +- NULL pointers after free to catch double-free +- Use const for read-only data +- Prefer stack allocation for small, fixed-size data + +### Performance +- Profile before optimizing (measure, don't guess) +- Cache frequently accessed data +- Minimize system calls +- Use atomic operations instead of locks when possible +- Keep critical sections minimal +- Use efficient algorithms (avoid O(n²)) +- Consider memory vs speed tradeoffs +- Know your platform's cache sizes + +### Maintainability +- Follow existing code style +- Use meaningful variable names +- Comment non-obvious logic (why, not what) +- Keep functions small and focused +- Avoid premature optimization +- Write self-documenting code + +### Platform Independence +- Use stdint.h for fixed-width types +- Use stdbool.h for boolean +- Handle endianness explicitly +- Don't assume structure packing +- Use configure checks for platform features +- Abstract platform-specific code + +## Anti-Patterns to Avoid + +```c +// Never assume malloc succeeds +char* buf = malloc(size); +strcpy(buf, input); // Crash if malloc failed! + +// Never ignore return values +fwrite(data, size, 1, file); // Did it succeed? + +// Never use magic numbers +if (size > 1024) { ... } // What is 1024? + +// Never leak on error paths +FILE* f = fopen(path, "r"); +if (error) return -1; // Leaked f! + + +// Never create threads with default stack size +pthread_create(&t, NULL, func, arg); // Wastes 8MB! + +// Never use inconsistent lock ordering +pthread_mutex_lock(&lock_a); +pthread_mutex_lock(&lock_b); // OK in func1 +// But in func2: +pthread_mutex_lock(&lock_b); +pthread_mutex_lock(&lock_a); // DEADLOCK! + +7. Use thread sanitizer for concurrent code +8. Test for race conditions with helgrind +9. Verify no deadlocks under load +// Never use heavy locks for simple operations +pthread_rwlock_wrlock(&lock); +counter++; // Use atomic_int instead! +pthread_rwlock_unlock(&lock); +// Never assume integer sizes +long timestamp; // 32 or 64 bits? +``` + +## Testing Focus + +For every change: +1. Write tests that verify the behavior +2. Run tests under valgrind to catch leaks +3. Verify tests pass on target platform +4. Check code coverage (aim for >80%) +5. Run static analysis tools +6. Test error paths and edge cases + +## Communication Style + +- Be direct and specific +- Explain memory implications +- Point out potential issues proactively +- Suggest platform-independent alternatives +- Reference specific line numbers +- Provide complete, working code examples diff --git a/.github/agents/l2-test-runner.agent.md b/.github/agents/l2-test-runner.agent.md new file mode 100644 index 000000000..67492605a --- /dev/null +++ b/.github/agents/l2-test-runner.agent.md @@ -0,0 +1,266 @@ +--- +name: 'L2 Test Runner' +description: 'Runs tr69hostif L2 integration tests in Docker containers, reports failures with root-cause analysis, and identifies untested areas. Prefers locally cached container images; asks before pulling or building new ones.' +tools: ['codebase', 'runCommands', 'search', 'edit', 'problems'] +--- + +# L2 Integration Test Runner + +You are a CI/test-execution specialist for the tr69hostif project. Your job is to run the L2 +functional integration test suite locally using Docker containers, exactly as the GitHub Actions +workflow `.github/workflows/L2-tests.yml` does, interpret results, and guide the developer to fix +any failures. + +## Responsibilities + +1. **Run L2 tests** inside the correct Docker containers on the developer's machine. +2. **Prefer local images** — check `docker images` before pulling anything from GHCR. +3. **Never pull or build images without user confirmation** when a pull is required or when + the local image is incompatible. +4. **Report failures** with a triage summary: failing test, assertion text, likely root cause, + and a suggested fix. +5. **Identify untested areas**: after every run, list functional areas with no L2 test coverage. + +--- + +## Container Images + +| Image name | GHCR path | Purpose | +|------------|-----------|---------| +| `mockxconf` | `ghcr.io/rdkcentral/docker-device-mgt-service-test/mockxconf:latest` | Mock XConf / WebPA server | +| `native-platform` | `ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest` | Build host + test runtime | +| `docker-rdk-ci` | `ghcr.io/rdkcentral/docker-rdk-ci:latest` | Results upload to Automatics | + +Container source: **https://github.com/rdkcentral/docker-device-mgt-service-test** + +--- + +## Workflow + +### Step 1 — Check local Docker images + +```bash +docker images --format "table {{.Repository}}:{{.Tag}}\t{{.ID}}\t{{.CreatedAt}}" | grep -E "mockxconf|native-platform" +``` + +- If **both images exist locally** → proceed directly to Step 3. +- If **one or both are missing** → ask the user: + + > "Image `` is not found locally. Should I pull it from GHCR (`docker pull ...`)? + > If the host architecture is incompatible with the pre-built image, I can also guide you + > to build it from source at https://github.com/rdkcentral/docker-device-mgt-service-test + > (requires your approval)." + + **Do not run `docker pull` or `docker build` without explicit user approval.** + +### Step 2 (conditional) — Authenticate, then pull or build + +Only after user approval. Before pulling, attempt GHCR login automatically using the +`rdkcentral` credentials stored in `~/.netrc`: + +```bash +# Extract token from ~/.netrc for ghcr.io +NETRC_TOKEN=$(awk '/machine ghcr.io/{getline; if ($1=="password") print $2}' ~/.netrc) +NETRC_USER=$(awk '/machine ghcr.io/{getline; if ($1=="login") print $2}' ~/.netrc) + +if [ -n "$NETRC_TOKEN" ]; then + echo "$NETRC_TOKEN" | docker login ghcr.io -u "$NETRC_USER" --password-stdin +else + echo "No ghcr.io entry found in ~/.netrc — login skipped." +fi +``` + +If `docker login` fails (exit code ≠ 0), **stop immediately** and show the user this prompt: + +> **GHCR login failed.** To authenticate manually: +> 1. Create a GitHub Personal Access Token (PAT) with `read:packages` scope at +> https://github.com/settings/tokens +> 2. Add it to `~/.netrc`: +> ``` +> machine ghcr.io +> login +> password +> ``` +> 3. Or log in directly: +> ```bash +> echo "" | docker login ghcr.io -u --password-stdin +> ``` +> Re-run the agent once you have authenticated. + +Do not attempt the pull until login succeeds. + +```bash +docker pull ghcr.io/rdkcentral/docker-device-mgt-service-test/mockxconf:latest +docker pull ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest +``` + +If the image architecture is incompatible with the host (e.g., `exec format error`), present this +prompt to the user instead of retrying the pull: + +> "The pre-built image is not compatible with your host architecture. +> To build compatible images from source, clone +> https://github.com/rdkcentral/docker-device-mgt-service-test and run: +> ```bash +> docker build -t mockxconf -f Dockerfile.mockxconf . +> docker build -t native-platform -f Dockerfile.native-platform . +> ``` +> Shall I proceed with the build?" + +### Step 3 — Handle existing containers + +First check whether `mockxconf` or `native-platform` containers are already running: + +```bash +docker ps --filter "name=mockxconf" --filter "name=native-platform" --format "table {{.Names}}\t{{.Status}}\t{{.CreatedAt}}" +``` + +If **either container exists** (running or stopped), **always ask the user** before removing it: + +> "Found existing container(s): ``. These may be left over from a +> previous test session. Should I stop and remove them to start a clean run? +> (If you are debugging a previous failure, you may want to keep them.)" + +**Do not run `docker rm` or `docker stop` without explicit user approval.** Proceed to +Step 4 only after confirmation. + +### Step 4 — Start mock XConf container + +```bash +docker run -d --name mockxconf \ + -p 50050:50050 -p 50051:50051 -p 50052:50052 -p 50053:50053 \ + -v "$(pwd)":/mnt/L2_CONTAINER_SHARED_VOLUME \ + mockxconf:latest # use local tag, fall back to ghcr.io/… if pulled +``` + +### Step 5 — Start native-platform container + +```bash +docker run -d --name native-platform \ + --link mockxconf \ + -v "$(pwd)":/mnt/L2_CONTAINER_SHARED_VOLUME \ + native-platform:latest +``` + +### Step 6 — Build and run tests + +Run the build and tests as **two separate `docker exec` calls** so that a build failure +can be detected and reported before the test runner is invoked. + +**6a — Build:** +```bash +docker exec -i native-platform /bin/bash -c \ + "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh cov_build.sh" +``` + +If the build exits with a non-zero code: +1. Capture the last 60 lines of compiler output. +2. Present a **Build Failure Summary**: + + ``` + ## Build Failure Summary + + **Exit code:** + + **First error:** + :: error: + + **Compiler output (last 60 lines):** + + + **Next step:** Fix the compiler error above and re-run the agent. + No further build or test steps will be attempted. + ``` +3. **Stop immediately.** Do not retry the build, do not proceed to Step 6b. + +**6b — Run tests** (only if 6a succeeded): +```bash +docker exec -i native-platform /bin/bash -c \ + "export LD_LIBRARY_PATH=\$LD_LIBRARY_PATH:/usr/lib/x86_64-linux-gnu:/lib/aarch64-linux-gnu:/usr/local/lib && \ + cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh run_l2.sh" +``` + +### Step 7 — Collect results + +```bash +docker cp native-platform:/tmp/l2_test_report /tmp/L2_TEST_RESULTS +``` + +### Step 8 — Analyse and report + +Parse JSON reports in `/tmp/L2_TEST_RESULTS/` and produce the outputs described below. + +--- + +## Output Format + +### A. Test Run Summary + +| Suite | Total | Passed | Failed | Errors | +|-------|-------|--------|--------|--------| +| bootup_sequence | N | N | N | N | +| handlers_communications | N | N | N | N | +| deviceip | N | N | N | N | +| webpa | N | N | N | N | + +### B. Failure Analysis (one entry per failed test) + +``` +## FAIL: [.json] + +**Assertion:** + + +**Likely cause:** +<2–3 sentence root-cause hypothesis based on test code and source> + +**Suggested fix:** + +``` + +### C. Untested Functionality + +After each run, audit `src/hostif/` against the test suites and list areas with no L2 coverage. +Always check these areas at minimum: + +| Area | Source path | L2 coverage? | +|------|------------|-------------| +| Bootstrap sequence / daemon startup | `src/hostif/src/hostIf_main.cpp` | ✅ | +| TR-181 Device.IP parameter handlers | `src/hostif/profiles/IP/` | ✅ | +| WebPA/parodus GET/SET request handling | `src/hostif/parodusClient/pal/` | ✅ | +| RFC parameter retrieval and override | `src/hostif/handlers/src/` — rfcapi path | ❌ | +| Device.Time parameter handlers | `src/hostif/profiles/Time/` | ❌ | +| STBService profile handlers | `src/hostif/profiles/STBService/` | ❌ | +| DeviceInfo firmware update status | `src/hostif/profiles/DeviceInfo/` — fwdnld handlers | partial | +| Ethernet interface handlers | `src/hostif/profiles/Ethernet/` | ❌ | +| moca profile handlers | `src/hostif/profiles/moca/` | ❌ | +| WiFi profile handlers | `src/hostif/profiles/wifi/` | ❌ | + +Update this table with actual results from each run (`✅` / `❌` / `partial`). + +--- + +## Rules and Constraints + +- **Never** run `docker pull` or `docker build` without explicit user approval. +- **Never** remove or stop `mockxconf` or `native-platform` containers without asking the user, + even if they look stale — they may be intentionally kept for debugging. +- **Never** stop or remove any container other than `mockxconf` / `native-platform` under any + circumstances. +- **Never** modify source files as part of a test run — only suggest edits. +- **Always** attempt GHCR login from `~/.netrc` before any `docker pull`; if login fails, show + the credential steps prompt and stop. +- **Always** clean up (`docker rm -f mockxconf native-platform`) at the end of a successful run, + unless the user asks to keep containers for debugging. +- If `build_inside_container.sh` fails: capture output, show the Build Failure Summary, and stop. + **Do not retry the build.** Do not attempt any workaround or source patch. +- If architecture incompatibility is detected, present the build-from-source prompt (see Step 2) + and wait for user approval before doing anything else. + +--- + +## Example Invocations + +- "Run the L2 tests and tell me what failed." +- "Run L2 tests using the images I already have." +- "Which parts of the xconf-client are not covered by L2 tests?" +- "L2 tests failed on `test_xconf_connection_with_empty_url` — what should I fix?" diff --git a/.github/agents/legacy-refactor-specialist.agent.md b/.github/agents/legacy-refactor-specialist.agent.md new file mode 100644 index 000000000..dcd7a39bc --- /dev/null +++ b/.github/agents/legacy-refactor-specialist.agent.md @@ -0,0 +1,263 @@ +--- +name: 'Legacy Code Refactoring Specialist' +description: 'Expert in safely refactoring legacy C/C++ code while preventing regressions and maintaining API compatibility' +tools: ['codebase', 'search', 'edit', 'runCommands', 'runTests', 'problems', 'usages'] +--- + +# Legacy Code Refactoring Specialist + +You are a specialist in working with legacy embedded C/C++ code. You follow Michael Feathers' "Working Effectively with Legacy Code" principles adapted for embedded systems. + +## Your Mission + +Improve code quality, reduce technical debt, and enhance maintainability while: +- **Zero regressions**: All existing tests must continue to pass +- **API stability**: Maintain backward compatibility +- **Resource constraints**: Don't increase memory footprint +- **Production safety**: Code ships to millions of devices + +## Your Process + +### 1. Understand Before Changing +- Read and analyze the existing code thoroughly +- Identify all entry points and dependencies +- Map data flow and control flow +- Document current behavior with tests +- Find all callers using search tools + +### 2. Establish Safety Net +- Write characterization tests for existing behavior +- Run tests before ANY changes +- Use static analysis tools (cppcheck, valgrind) +- Create test coverage baseline +- Document any undefined behavior found + +### 3. Make Changes Incrementally +- One small change at a time +- Run full test suite after each change +- Verify memory usage hasn't increased +- Check for new static analysis warnings +- Commit frequently with clear messages + +### 4. Refactoring Patterns + +#### Extract Function +```c +// BEFORE: Long function with mixed concerns +int process_data(const char* input) { + // 200 lines of code doing multiple things + // Parsing, validation, transformation, storage +} + +// AFTER: Extracted, focused functions +static int validate_input(const char* input); +static int parse_data(const char* input, data_t* out); +static int store_data(const data_t* data); + +int process_data(const char* input) { + data_t data; + + if (validate_input(input) != 0) return -1; + if (parse_data(input, &data) != 0) return -1; + if (store_data(&data) != 0) return -1; + + return 0; +} +``` + +#### Introduce Seam (for testing) +```c +// BEFORE: Hard to test due to tight coupling +void process() { + FILE* f = fopen("/etc/config", "r"); + // ... process file ... + fclose(f); +} + +// AFTER: Dependency injection +typedef struct { + FILE* (*open_file)(const char* path); + // ... other dependencies ... +} dependencies_t; + +void process_with_deps(const dependencies_t* deps) { + FILE* f = deps->open_file("/etc/config"); + // ... process file ... + fclose(f); +} + +// Production code +FILE* real_open(const char* path) { return fopen(path, "r"); } +dependencies_t prod_deps = { .open_file = real_open }; + +void process() { + process_with_deps(&prod_deps); +} + +// Test code can inject mocks +``` + +#### Reduce God Object +```c +// BEFORE: Huge structure with everything +typedef struct { + char config_path[256]; + int config_version; + FILE* log_file; + void* data_buffer; + size_t buffer_size; + // ... 50 more fields ... +} context_t; + +// AFTER: Separate concerns +typedef struct { + char path[256]; + int version; +} config_t; + +typedef struct { + FILE* file; +} logger_t; + +typedef struct { + void* buffer; + size_t size; +} data_buffer_t; + +// Compose only what's needed +typedef struct { + config_t* config; + logger_t* logger; + data_buffer_t* buffer; +} context_t; +``` + +### 5. Memory Optimization Patterns + +#### Replace Heap with Stack +```c +// BEFORE: Unnecessary heap allocation +char* format_message(const char* fmt, ...) { + char* buf = malloc(256); + // ... format into buf ... + return buf; // Caller must free +} + +// AFTER: Use stack (if size is known and reasonable) +#define MSG_MAX_SIZE 256 + +int format_message(char* buf, size_t size, const char* fmt, ...) { + // ... format into buf ... + return strlen(buf); +} + +// Caller: +char msg[MSG_MAX_SIZE]; +format_message(msg, sizeof(msg), "Error: %d", code); +``` + +#### Memory Pool for Frequent Allocations +```c +// BEFORE: Frequent malloc/free causing fragmentation +for (int i = 0; i < 1000; i++) { + event_t* e = malloc(sizeof(event_t)); + process_event(e); + free(e); +} + +// AFTER: Pre-allocated pool +#define EVENT_POOL_SIZE 10 + +typedef struct { + event_t events[EVENT_POOL_SIZE]; + bool used[EVENT_POOL_SIZE]; +} event_pool_t; + +event_t* event_pool_acquire(event_pool_t* pool); +void event_pool_release(event_pool_t* pool, event_t* event); + +// Usage +event_pool_t pool = {0}; +for (int i = 0; i < 1000; i++) { + event_t* e = event_pool_acquire(&pool); + process_event(e); + event_pool_release(&pool, e); +} +``` + +## Regression Prevention + +### Before Any Refactoring +1. Ensure all existing tests pass +2. Run valgrind (no leaks in current code) +3. Measure memory footprint baseline +4. Document current behavior + +### During Refactoring +1. Make one logical change at a time +2. Run tests after EVERY change +3. Use git to create checkpoint commits +4. Monitor memory usage + +### After Refactoring +1. All tests still pass +2. No new memory leaks (valgrind) +3. Memory footprint same or better +4. No new compiler warnings +5. Static analysis clean +6. Code review by human + +## Communication + +### When Proposing Changes +- Explain the problem being solved +- Show before/after comparison +- Highlight safety measures +- Document any risks +- Estimate memory impact + +### When Blocked +- Explain what's preventing progress +- Suggest alternatives +- Ask for clarification on requirements +- Note any missing tests + +### Code Review Focus +- Point out missing error handling +- Identify memory leak risks +- Note API compatibility concerns +- Suggest additional test cases +- Highlight complexity that could be simplified + +## Emergency Procedures + +If tests start failing: +1. **STOP** immediately +2. Review the last change +3. Use git diff to see what changed +4. Revert if cause isn't obvious +5. Fix the issue before continuing + +If memory leaks detected: +1. **STOP** the refactoring +2. Run valgrind to identify leak +3. Fix the leak +4. Verify fix with valgrind +5. Resume refactoring + +If API breaks: +1. **REVERT** the breaking change +2. Find alternative approach +3. Use wrapper functions if needed +4. Maintain old API alongside new + +## Success Criteria + +You've succeeded when: +- All tests pass +- No memory leaks (valgrind clean) +- Code is more maintainable +- No API breaks +- Memory footprint same or improved +- Complexity metrics improved +- Test coverage maintained or improved diff --git a/.github/instructions/build-system.instructions.md b/.github/instructions/build-system.instructions.md new file mode 100644 index 000000000..17121156d --- /dev/null +++ b/.github/instructions/build-system.instructions.md @@ -0,0 +1,137 @@ +--- +applyTo: "**/Makefile.am,**/configure.ac,**/*.ac,**/*.mk" +--- + +# Build System Standards (Autotools) + +## Autotools Best Practices + +### configure.ac +- Check for required headers and functions +- Provide clear error messages for missing dependencies +- Support cross-compilation +- Allow feature toggles + +```autoconf +# GOOD: Check for required features +AC_CHECK_HEADERS([pthread.h], [], + [AC_MSG_ERROR([pthread.h is required])]) + +AC_CHECK_LIB([pthread], [pthread_create], [], + [AC_MSG_ERROR([pthread library is required])]) + +# GOOD: Optional features with clear naming +AC_ARG_ENABLE([gtest], + AS_HELP_STRING([--enable-gtest], [Enable Google Test support]), + [enable_gtest=$enableval], + [enable_gtest=no]) + +AM_CONDITIONAL([WITH_GTEST_SUPPORT], [test "x$enable_gtest" = "xyes"]) +``` + +### Makefile.am +- Use non-recursive makefiles when possible +- Minimize intermediate libraries +- Support parallel builds +- Link only what's needed + +```makefile +# GOOD: Minimal linking +bin_PROGRAMS = tr69hostif + +tr69hostif_SOURCES = src/hostif/src/hostIf_main.cpp +tr69hostif_CXXFLAGS = -DFEATURE_SUPPORT_RDKLOG +tr69hostif_LDADD = \ + $(top_builddir)/src/hostif/handlers/libhandlers.la \ + $(top_builddir)/src/hostif/profiles/libprofiles.la \ + -lpthread -ldl + +# GOOD: Conditional compilation +if WITH_GTEST_SUPPORT +SUBDIRS += src/unittest +endif +``` + +## Cross-Compilation Support + +### Platform Detection +```autoconf +# Support different target platforms +case "$host" in + *-linux*) + AC_DEFINE([PLATFORM_LINUX], [1], [Linux platform]) + ;; + *-arm*) + AC_DEFINE([PLATFORM_ARM], [1], [ARM platform]) + ;; +esac +``` + +### Compiler Flags +```makefile +# Platform-specific optimizations +if TARGET_ARM +AM_CFLAGS += -march=armv7-a -mfpu=neon +endif + +# Debug vs Release +if DEBUG_BUILD +AM_CFLAGS += -g -O0 -DDEBUG +else +AM_CFLAGS += -O2 -DNDEBUG +endif +``` + +## Dependency Management + +### Package Config +```autoconf +# Use pkg-config for external dependencies +PKG_CHECK_MODULES([DBUS], [dbus-1 >= 1.6]) +AC_SUBST([DBUS_CFLAGS]) +AC_SUBST([DBUS_LIBS]) +``` + +### Header Organization +```makefile +# Include paths +AM_CPPFLAGS = -I$(top_srcdir)/src/hostif/include \ + -I$(top_srcdir)/src/hostif/handlers/include \ + -I$(top_srcdir)/src/hostif/profiles \ + $(DBUS_CFLAGS) +``` + +## Build Performance + +### Parallel Builds +- Support `make -j` +- Avoid circular dependencies +- Use order-only prerequisites when appropriate + +### Incremental Builds +- Proper dependency tracking +- Don't force full rebuilds unless necessary +- Use libtool for shared libraries + +## Testing Integration + +```makefile +# Test targets +check-local: + @echo "Running memory leak tests..." + @for test in $(TESTS); do \ + valgrind --leak-check=full \ + --error-exitcode=1 \ + ./$$test || exit 1; \ + done + +# Code coverage +if ENABLE_COVERAGE +AM_CFLAGS += --coverage +AM_LDFLAGS += --coverage +endif + +coverage: check + $(LCOV) --capture --directory . --output-file coverage.info + $(GENHTML) coverage.info --output-directory coverage +``` diff --git a/.github/instructions/c-embedded.instructions.md b/.github/instructions/c-embedded.instructions.md new file mode 100644 index 000000000..1ef2a9812 --- /dev/null +++ b/.github/instructions/c-embedded.instructions.md @@ -0,0 +1,693 @@ +--- +applyTo: "**/*.c,**/*.h" +--- + +# C Programming Standards for Embedded Systems + +## Memory Management + +### Allocation Rules +- **Prefer stack allocation** for fixed-size, short-lived data +- **Use malloc/free** only when necessary; always pair them +- **Check all allocations**: Never assume malloc succeeds +- **Free in reverse order** of allocation to reduce fragmentation +- **Use memory pools** for frequent same-size allocations +- **Zero memory after free** to catch use-after-free bugs in debug builds + +```c +// GOOD: Stack allocation for fixed-size data +char buffer[256]; + +// GOOD: Checked heap allocation with cleanup +char* data = malloc(size); +if (!data) { + log_error("Failed to allocate %zu bytes", size); + return ERR_NO_MEMORY; +} +// ... use data ... +free(data); +data = NULL; // Prevent double-free + +// BAD: Unchecked allocation +char* data = malloc(size); +strcpy(data, input); // Crash if malloc failed +``` + +### Memory Leak Prevention +- Every function that allocates must document ownership transfer +- Use goto for single exit point in complex error handling +- Implement cleanup functions for complex structures +- Use valgrind regularly during development + +```c +// GOOD: Single exit point with cleanup +int process_data(const char* input) { + int ret = 0; + char* buffer = NULL; + FILE* file = NULL; + + buffer = malloc(BUFFER_SIZE); + if (!buffer) { + ret = ERR_NO_MEMORY; + goto cleanup; + } + + file = fopen(input, "r"); + if (!file) { + ret = ERR_FILE_OPEN; + goto cleanup; + } + + // ... processing ... + +cleanup: + free(buffer); + if (file) fclose(file); + return ret; +} +``` + +## Resource Constraints + +### Code Size Optimization +- Avoid inline functions unless proven beneficial +- Share common code paths +- Use function pointers for conditional logic in tables +- Strip debug symbols in release builds + +### CPU Optimization +- Minimize system calls +- Cache frequently accessed data +- Use efficient algorithms (prefer O(n) over O(n²)) +- Avoid floating point on devices without FPU +- Profile before optimizing (don't guess) + +### Memory Optimization +- Use bitfields for boolean flags +- Pack structures to minimize padding +- Use const for read-only data (goes in .rodata) +- Prefer static buffers with maximum sizes when bounds are known +- Implement object pools for frequently created/destroyed objects + +```c +// GOOD: Packed structure +typedef struct __attribute__((packed)) { + uint8_t flags; + uint16_t id; + uint32_t timestamp; + char name[32]; +} telemetry_event_t; + +// GOOD: Const data in .rodata +static const char* const ERROR_MESSAGES[] = { + "Success", + "Out of memory", + "Invalid parameter", + // ... +}; +``` + +## Platform Independence + +### Never Assume +- Pointer size (use uintptr_t for pointer arithmetic) +- Byte order (use htonl/ntohl for network data) +- Structure packing (use __attribute__((packed)) or #pragma pack) +- Integer sizes (use int32_t, uint64_t from stdint.h) +- Boolean type (use stdbool.h) + +```c +// GOOD: Platform-independent types +#include +#include + +typedef struct { + uint32_t id; // Always 32 bits + uint64_t timestamp; // Always 64 bits + bool enabled; // Standard boolean +} config_t; + +// GOOD: Endianness handling +uint32_t network_value = htonl(host_value); + +// BAD: Assumptions +int id; // Size varies by platform +long timestamp; // 32 or 64 bits depending on platform +``` + +### Abstraction Layers +- Use platform abstraction for OS-specific code +- Isolate hardware dependencies +- Use configure.ac to detect platform capabilities + +## Error Handling + +### Return Value Convention +- Return 0 for success, negative for errors +- Use errno for system call failures +- Define error codes in header files +- Never ignore return values + +```c +// GOOD: Consistent error handling +typedef enum { + T2ERROR_SUCCESS = 0, + T2ERROR_FAILURE = -1, + T2ERROR_INVALID_PARAM = -2, + T2ERROR_NO_MEMORY = -3, + T2ERROR_TIMEOUT = -4 +} T2ERROR; + +T2ERROR init_telemetry() { + if (!validate_config()) { + return T2ERROR_INVALID_PARAM; + } + + if (allocate_resources() != 0) { + return T2ERROR_NO_MEMORY; + } + + return T2ERROR_SUCCESS; +} +``` + +### Logging +- Use severity levels appropriately +- Log errors with context (function, line, errno) +- Avoid logging in hot paths +- Make logging configurable at runtime +- Never log sensitive data + +```c +// GOOD: Contextual error logging +if (ret != 0) { + T2Error("%s:%d Failed to initialize: %s (errno=%d)", + __FUNCTION__, __LINE__, strerror(errno), errno); + return T2ERROR_FAILURE; +} +``` + +## Thread Safety and Concurrency + +### Critical Principles + +- **Minimize synchronization overhead**: Use lightweight primitives +- **Prevent deadlocks**: Establish lock ordering, use timeouts +- **Avoid memory fragmentation**: Configure thread stack sizes appropriately +- **Reduce contention**: Design for lock-free patterns where possible +- **Document thread safety**: Mark functions as thread-safe or not + +### Thread Creation with Minimal Memory + +Always create threads with attributes that specify required memory: + +```c +// GOOD: Thread with minimal stack size +#include + +#define THREAD_STACK_SIZE (64 * 1024) // 64KB instead of default (often 8MB) + +pthread_t thread; +pthread_attr_t attr; + +// Initialize attributes +pthread_attr_init(&attr); + +// Set minimal stack size (reduces memory fragmentation) +pthread_attr_setstacksize(&attr, THREAD_STACK_SIZE); + +// Detached threads free resources immediately when done +pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); + +// Create thread +int ret = pthread_create(&thread, &attr, thread_function, arg); +if (ret != 0) { + T2Error("Failed to create thread: %s", strerror(ret)); + pthread_attr_destroy(&attr); + return T2ERROR_FAILURE; +} + +// Clean up attributes +pthread_attr_destroy(&attr); + +// BAD: Default thread (wastes memory) +pthread_create(&thread, NULL, thread_function, arg); // Uses 8MB stack! +``` + +### Lightweight Synchronization + +Prefer lightweight synchronization primitives to avoid deadlocks and overhead: + +```c +// GOOD: Simple mutex with minimal overhead +typedef struct { + pthread_mutex_t lock; + int counter; +} thread_safe_counter_t; + +int init_counter(thread_safe_counter_t* c) { + // Use default attributes (lightest weight) + pthread_mutex_init(&c->lock, NULL); + c->counter = 0; + return 0; +} + +void increment_counter(thread_safe_counter_t* c) { + pthread_mutex_lock(&c->lock); + c->counter++; + pthread_mutex_unlock(&c->lock); +} + +void cleanup_counter(thread_safe_counter_t* c) { + pthread_mutex_destroy(&c->lock); +} + +// GOOD: Use atomic operations when possible (no locks needed) +#include + +typedef struct { + atomic_int counter; // Lock-free! +} lockfree_counter_t; + +void increment_lockfree(lockfree_counter_t* c) { + atomic_fetch_add(&c->counter, 1); // No mutex overhead +} +``` + +### Deadlock Prevention + +Follow strict rules to prevent deadlocks: + +```c +// GOOD: Consistent lock ordering +typedef struct { + pthread_mutex_t lock_a; + pthread_mutex_t lock_b; + // ... data ... +} resource_t; + +// RULE: Always acquire locks in alphabetical order (a, then b) +void multi_lock_operation(resource_t* r) { + pthread_mutex_lock(&r->lock_a); // First: lock_a + pthread_mutex_lock(&r->lock_b); // Second: lock_b + + // ... critical section ... + + pthread_mutex_unlock(&r->lock_b); // Release in reverse order + pthread_mutex_unlock(&r->lock_a); +} + +// GOOD: Use trylock with timeout to avoid indefinite blocking +#include + +int safe_lock_with_timeout(pthread_mutex_t* lock, int timeout_ms) { + struct timespec ts; + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += timeout_ms / 1000; + ts.tv_nsec += (timeout_ms % 1000) * 1000000; + + int ret = pthread_mutex_timedlock(lock, &ts); + if (ret == ETIMEDOUT) { + T2Error("Lock timeout - potential deadlock detected"); + return -1; + } + return ret; +} + +// BAD: Different lock order in different functions (DEADLOCK RISK!) +void bad_function_1(resource_t* r) { + pthread_mutex_lock(&r->lock_a); + pthread_mutex_lock(&r->lock_b); // Order: a, b + // ... +} + +void bad_function_2(resource_t* r) { + pthread_mutex_lock(&r->lock_b); + pthread_mutex_lock(&r->lock_a); // Order: b, a - DEADLOCK! + // ... +} +``` + +### Avoid Heavy Synchronization + +Heavy synchronization causes performance issues and fragmentation: + +```c +// BAD: Reader-writer lock for simple counter (overkill) +pthread_rwlock_t heavy_lock; +int counter; + +void heavy_increment() { + pthread_rwlock_wrlock(&heavy_lock); // Too heavy! + counter++; + pthread_rwlock_unlock(&heavy_lock); +} + +// GOOD: Use appropriate synchronization level +atomic_int light_counter; // Lock-free for simple operations + +void light_increment() { + atomic_fetch_add(&light_counter, 1); // No lock overhead +} + +// BAD: Fine-grained locking everywhere (lock thrashing) +typedef struct { + pthread_mutex_t lock; + int value; +} each_field_locked_t; // Don't do this! + +// GOOD: Coarse-grained locking for related data +typedef struct { + pthread_mutex_t lock; + int value_a; + int value_b; + int value_c; // All protected by one lock +} properly_locked_t; +``` + +### Lock-Free Patterns + +Use lock-free patterns to avoid synchronization overhead: + +```c +// GOOD: Lock-free flag +#include + +typedef struct { + atomic_bool shutdown_requested; +} thread_control_t; + +void request_shutdown(thread_control_t* ctrl) { + atomic_store(&ctrl->shutdown_requested, true); +} + +bool should_shutdown(thread_control_t* ctrl) { + return atomic_load(&ctrl->shutdown_requested); +} + +// GOOD: Lock-free queue for single producer, single consumer +typedef struct { + atomic_int read_index; + atomic_int write_index; + void* buffer[256]; +} spsc_queue_t; + +bool spsc_enqueue(spsc_queue_t* q, void* item) { + int write = atomic_load(&q->write_index); + int next_write = (write + 1) % 256; + + if (next_write == atomic_load(&q->read_index)) { + return false; // Queue full + } + + q->buffer[write] = item; + atomic_store(&q->write_index, next_write); + return true; +} +``` + +### Minimize Critical Sections + +Keep locked sections as short as possible: + +```c +// BAD: Long critical section +void bad_process(data_t* shared) { + pthread_mutex_lock(&shared->lock); + + // Heavy computation while holding lock (BAD!) + for (int i = 0; i < 1000000; i++) { + compute_something(); + } + + shared->value = result; + pthread_mutex_unlock(&shared->lock); +} + +// GOOD: Minimal critical section +void good_process(data_t* shared) { + // Do heavy computation WITHOUT lock + int result = 0; + for (int i = 0; i < 1000000; i++) { + result += compute_something(); + } + + // Lock only for the update + pthread_mutex_lock(&shared->lock); + shared->value = result; + pthread_mutex_unlock(&shared->lock); +} +``` + +### Thread-Safe Initialization + +Use pthread_once for thread-safe initialization: + +```c +// GOOD: Thread-safe singleton initialization +static pthread_once_t init_once = PTHREAD_ONCE_INIT; +static config_t* global_config = NULL; + +static void init_config_once(void) { + global_config = malloc(sizeof(config_t)); + // ... initialize config ... +} + +config_t* get_config(void) { + pthread_once(&init_once, init_config_once); + return global_config; +} + +// BAD: Double-checked locking (broken in C without memory barriers) +static pthread_mutex_t init_lock; +static config_t* config = NULL; + +config_t* bad_get_config(void) { + if (config == NULL) { // First check (no lock) + pthread_mutex_lock(&init_lock); + if (config == NULL) { // Second check + config = malloc(sizeof(config_t)); // Race condition! + } + pthread_mutex_unlock(&init_lock); + } + return config; +} +``` + +### Thread Safety Documentation + +Always document thread safety expectations: + +```c +// GOOD: Clear thread safety documentation + +/** + * Process telemetry event + * @param event Event to process + * @return 0 on success, negative on error + * + * Thread Safety: This function is thread-safe and may be called + * from multiple threads concurrently. + */ +int process_event(const event_t* event) { + // Uses internal locking +} + +/** + * Initialize event processor + * @return 0 on success, negative on error + * + * Thread Safety: NOT thread-safe. Must be called once during + * initialization before any worker threads start. + */ +int init_event_processor(void) { + // No locking - initialization only +} + +/** + * Get current statistics + * @param stats Output buffer for statistics + * + * Thread Safety: Caller must hold stats_lock before calling. + * Use get_stats_safe() for automatic locking. + */ +void get_stats_unlocked(stats_t* stats) { + // Assumes caller holds lock +} +``` + +### Memory Fragmentation Prevention + +Configure thread pools to prevent fragmentation: + +```c +// GOOD: Thread pool with pre-allocated threads +#define THREAD_POOL_SIZE 4 +#define WORK_QUEUE_SIZE 256 + +typedef struct { + pthread_t threads[THREAD_POOL_SIZE]; + pthread_attr_t thread_attr; + // ... work queue ... +} thread_pool_t; + +int init_thread_pool(thread_pool_t* pool) { + // Configure thread attributes once + pthread_attr_init(&pool->thread_attr); + pthread_attr_setstacksize(&pool->thread_attr, THREAD_STACK_SIZE); + pthread_attr_setdetachstate(&pool->thread_attr, PTHREAD_CREATE_JOINABLE); + + // Create fixed number of threads (no dynamic allocation) + for (int i = 0; i < THREAD_POOL_SIZE; i++) { + int ret = pthread_create(&pool->threads[i], &pool->thread_attr, + worker_thread, pool); + if (ret != 0) { + // Cleanup already created threads + cleanup_partial_pool(pool, i); + return -1; + } + } + + return 0; +} + +// BAD: Creating threads dynamically (causes fragmentation) +void bad_handle_request(request_t* req) { + pthread_t thread; + pthread_create(&thread, NULL, handle_one_request, req); + pthread_detach(thread); // New thread for each request! +} +``` + +### Testing Thread Safety + +```c +// GOOD: Test for race conditions +#include + +TEST(ThreadSafety, ConcurrentIncrement) { + thread_safe_counter_t counter = {0}; + init_counter(&counter); + + const int NUM_THREADS = 10; + const int INCREMENTS_PER_THREAD = 1000; + pthread_t threads[NUM_THREADS]; + + // Create multiple threads + for (int i = 0; i < NUM_THREADS; i++) { + pthread_create(&threads[i], NULL, + increment_n_times, &counter); + } + + // Wait for all threads + for (int i = 0; i < NUM_THREADS; i++) { + pthread_join(threads[i], NULL); + } + + // Verify no race conditions + EXPECT_EQ(counter.counter, NUM_THREADS * INCREMENTS_PER_THREAD); + + cleanup_counter(&counter); +} +``` + +### Static Analysis for Concurrency + +```bash +# Use thread sanitizer to detect race conditions +gcc -g -fsanitize=thread source.c -o program +./program + +# Use helgrind (valgrind) to detect synchronization issues +valgrind --tool=helgrind ./program + +# Check for deadlocks +valgrind --tool=helgrind --track-lockorders=yes ./program +``` + +## Code Style + +### Naming Conventions +- Functions: `snake_case` (e.g., `init_telemetry`) +- Types: `snake_case_t` (e.g., `telemetry_event_t`) +- Macros/Constants: `UPPER_SNAKE_CASE` (e.g., `MAX_BUFFER_SIZE`) +- Global variables: `g_` prefix (avoid when possible) +- Static variables: `s_` prefix + +### File Organization +- One .c file per module +- Corresponding .h file for public interface +- Internal functions marked static +- Header guards in all .h files + +```c +// GOOD: header guard +#ifndef TELEMETRY_INTERNAL_H +#define TELEMETRY_INTERNAL_H + +// ... declarations ... + +#endif /* TELEMETRY_INTERNAL_H */ +``` + +## Testing Requirements + +### Unit Tests +- Test all public functions +- Test error paths and edge cases +- Use mocks for external dependencies +- Verify resource cleanup (no leaks) +- Run tests under valgrind + +### Memory Testing +```bash +# Run with memory checking +valgrind --leak-check=full --show-leak-kinds=all \ + --track-origins=yes ./test_binary + +# Static analysis +cppcheck --enable=all --inconclusive source/ +``` + +## Anti-Patterns to Avoid + +```c +// BAD: Magic numbers +if (size > 1024) { ... } + +// GOOD: Named constants +#define MAX_PACKET_SIZE 1024 +if (size > MAX_PACKET_SIZE) { ... } + +// BAD: Unchecked allocation +char* buf = malloc(size); +strcpy(buf, input); + +// GOOD: Checked with cleanup +char* buf = malloc(size); +if (!buf) return ERR_NO_MEMORY; +strncpy(buf, input, size - 1); +buf[size - 1] = '\0'; + +// BAD: Memory leak in error path +FILE* f = fopen(path, "r"); +if (condition) return -1; // Leaked f +fclose(f); + +// GOOD: Cleanup on all paths +FILE* f = fopen(path, "r"); +if (!f) return -1; +if (condition) { + fclose(f); + return -1; +} +fclose(f); +return 0; +``` + +## References + +- Project follows RDK coding standards +- See `src/hostif/include/` for tr69hostif API header documentation +- Review existing code in `src/hostif/` for patterns +- Check `src/unittest/` directory for testing examples diff --git a/.github/instructions/cpp-testing.instructions.md b/.github/instructions/cpp-testing.instructions.md new file mode 100644 index 000000000..0e1bcf82b --- /dev/null +++ b/.github/instructions/cpp-testing.instructions.md @@ -0,0 +1,178 @@ +--- +applyTo: "src/unittest/**/*.cpp,src/unittest/**/*.h,src/hostif/**/gtest/**/*.cpp,src/hostif/**/gtest/**/*.h" +--- + +# C++ Testing Standards (Google Test) + +## Test Framework + +Use Google Test (gtest) and Google Mock (gmock) for all C++ test code. + +## Test Organization + +### File Structure +- One test file per source file: `foo.c` → `test/FooTest.cpp` +- Test fixtures for complex setups +- Mocks in separate files when reusable + +```cpp +// GOOD: Test file structure +// filepath: src/unittest/hostIf_utils_Test.cpp + +extern "C" { +#include "hostIf_utils.h" +#include "IniFile.h" +} + +#include +#include + +class HostIfUtilsTest : public ::testing::Test { +protected: + void SetUp() override { + // Initialize test resources + } + + void TearDown() override { + // Clean up test resources + } +}; + +TEST_F(HostIfUtilsTest, IniFileReadWriteRoundTrip) { + IniFile ini; + ini.load("/tmp/test.ini"); + // verify read back value matches written value + ASSERT_EQ(ini.get("key"), "value"); +} +``` + +## Testing Patterns + +### Test C Code from C++ +- Wrap C headers in `extern "C"` blocks +- Use RAII in tests for automatic cleanup +- Mock C functions using gmock when needed + +```cpp +extern "C" { +#include "hostIf_main.h" +#include "hostIf_tr69ReqHandler.h" +} + +#include + +class HostIfHandlerTest : public ::testing::Test { +protected: + void SetUp() override { + // Initialize handler stubs + } + + void TearDown() override { + // Clean up + } +}; + +TEST_F(HostIfHandlerTest, GetParamValueReturnsExpected) { + HOSTIF_MsgData_t msgData = {}; + strncpy(msgData.paramName, "Device.DeviceInfo.Manufacturer", sizeof(msgData.paramName) - 1); + msgData.reqType = HOSTIF_GET; + // verify handler returns success and populates paramValue +} +``` + +### Memory Leak Testing +- All tests must pass valgrind +- Use RAII wrappers for C resources +- Verify cleanup in TearDown + +```cpp +// GOOD: RAII wrapper for C resource +class FileHandle { + FILE* file_; +public: + explicit FileHandle(const char* path, const char* mode) + : file_(fopen(path, mode)) {} + + ~FileHandle() { + if (file_) fclose(file_); + } + + FILE* get() const { return file_; } + bool valid() const { return file_ != nullptr; } +}; + +TEST(FileTest, ReadConfig) { + FileHandle file("/tmp/config.json", "r"); + ASSERT_TRUE(file.valid()); + // file automatically closed when test exits +} +``` + +### Mocking External Dependencies + +```cpp +// GOOD: Mock for handler dependencies +class MockIniFile { +public: + MOCK_METHOD(std::string, get, (const std::string& key)); + MOCK_METHOD(bool, set, (const std::string& key, const std::string& value)); +}; + +TEST(HandlerTest, GetParamUsesIniFile) { + MockIniFile mock; + + EXPECT_CALL(mock, get("Device.DeviceInfo.Manufacturer")) + .WillOnce(testing::Return("TestVendor")); + + std::string result = mock.get("Device.DeviceInfo.Manufacturer"); + EXPECT_EQ("TestVendor", result); +} +``` + +## Test Quality Standards + +### Coverage Requirements +- All public functions must have tests +- Test both success and failure paths +- Test boundary conditions +- Test error handling + +### Test Naming +```cpp +// Pattern: TEST(ComponentName, BehaviorBeingTested) + +TEST(Vector, CreateReturnsNonNull) { ... } +TEST(Vector, DestroyHandlesNull) { ... } +TEST(Vector, PushIncrementsSize) { ... } +TEST(Utils, ParseConfigInvalidJson) { ... } +``` + +### Assertions +- Use `ASSERT_*` when test can't continue after failure +- Use `EXPECT_*` when subsequent checks are still valuable +- Provide helpful failure messages + +```cpp +// GOOD: Informative assertions +ASSERT_NE(nullptr, ptr) << "Failed to allocate " << size << " bytes"; +EXPECT_EQ(expected, actual) << "Mismatch at index " << i; +EXPECT_TRUE(condition) << "Context: " << debug_info; +``` + +## Running Tests + +### Build Tests +```bash +./configure --enable-gtest +make check +``` + +### Memory Checking +```bash +valgrind --leak-check=full --show-leak-kinds=all \ + ./src/unittest/tr69hostif_gtest +``` + +### Test Output +- Use `GTEST_OUTPUT=xml:results.xml` for CI integration +- Check return code: 0 = all passed diff --git a/.github/instructions/shell-scripts.instructions.md b/.github/instructions/shell-scripts.instructions.md new file mode 100644 index 000000000..a25a2c69b --- /dev/null +++ b/.github/instructions/shell-scripts.instructions.md @@ -0,0 +1,179 @@ +--- +applyTo: "**/*.sh" +--- + +# Shell Script Standards for Embedded Systems + +## Platform Independence + +### Use POSIX Shell +- Use `#!/bin/sh` not `#!/bin/bash` +- Avoid bashisms (use shellcheck to verify) +- Test on busybox ash (common in embedded) + +```bash +#!/bin/sh +# GOOD: POSIX compliant + +# BAD: Bash-specific +if [[ $var == "value" ]]; then # Use [ ] instead + array=(1 2 3) # Arrays not in POSIX +fi + +# GOOD: POSIX compliant +if [ "$var" = "value" ]; then + set -- 1 2 3 # Use positional parameters +fi +``` + +## Resource Awareness + +### Minimize Process Spawning +- Use shell builtins when possible +- Avoid pipes when not necessary +- Batch operations to reduce forks + +```bash +# BAD: Multiple processes +cat file | grep pattern | wc -l + +# GOOD: Fewer processes +grep -c pattern file + +# BAD: Loop with external commands +for file in *.txt; do + cat "$file" >> output +done + +# GOOD: Single cat invocation +cat *.txt > output +``` + +### Memory Usage +- Avoid reading entire files into variables +- Process streams line by line +- Clean up temporary files + +```bash +# BAD: Loads entire file into memory +content=$(cat large_file.log) +echo "$content" | grep ERROR + +# GOOD: Stream processing +grep ERROR large_file.log + +# GOOD: Line-by-line processing +while IFS= read -r line; do + process_line "$line" +done < large_file.log +``` + +## Error Handling + +### Always Check Exit Codes +```bash +# GOOD: Check critical operations +if ! mkdir -p /tmp/telemetry; then + logger -t telemetry "ERROR: Failed to create directory" + exit 1 +fi + +# GOOD: Use set -e for fail-fast +set -e # Exit on any error +set -u # Exit on undefined variable +set -o pipefail # Catch errors in pipes + +# GOOD: Trap for cleanup +cleanup() { + rm -f "$TEMP_FILE" +} +trap cleanup EXIT INT TERM + +TEMP_FILE=$(mktemp) +# ... use temp file ... +# cleanup happens automatically +``` + +## Script Quality + +### Defensive Programming +```bash +# GOOD: Quote all variables +rm -f "$file_path" # Not: rm -f $file_path + +# GOOD: Use -- to separate options from arguments +grep -r -- "$pattern" "$directory" + +# GOOD: Check variable is set +: "${CONFIG_FILE:?CONFIG_FILE must be set}" + +# GOOD: Validate inputs +if [ -z "$1" ]; then + echo "Usage: $0 " >&2 + exit 1 +fi +``` + +### Logging +```bash +# Use logger for syslog integration +log_info() { + logger -t telemetry -p user.info "$*" +} + +log_error() { + logger -t telemetry -p user.error "$*" + echo "ERROR: $*" >&2 +} + +# Usage +log_info "Starting telemetry collection" +if ! start_service; then + log_error "Failed to start service" + exit 1 +fi +``` + +## Testing Scripts + +### Use shellcheck +```bash +# Run shellcheck on all scripts +shellcheck script.sh + +# In CI +find . -name "*.sh" -exec shellcheck {} + +``` + +### Test on Target Platform +- Test on actual embedded device or emulator +- Verify with busybox tools +- Check resource usage (memory, CPU) + +## Anti-Patterns + +```bash +# BAD: Unquoted variables +for file in $FILES; do # Word splitting! + +# GOOD: Quoted +for file in "$FILES"; do + +# BAD: Parsing ls output +for file in $(ls *.txt); do + +# GOOD: Use glob +for file in *.txt; do + +# BAD: Useless use of cat +cat file | grep pattern + +# GOOD: grep can read files +grep pattern file + +# BAD: Not checking if file exists +rm /tmp/file # Error if doesn't exist + +# GOOD: Check or use -f +rm -f /tmp/file # Or: [ -f /tmp/file ] && rm /tmp/file +``` diff --git a/.github/prompts/opsx-apply.prompt.md b/.github/prompts/opsx-apply.prompt.md new file mode 100644 index 000000000..e23ec64d1 --- /dev/null +++ b/.github/prompts/opsx-apply.prompt.md @@ -0,0 +1,149 @@ +--- +description: Implement tasks from an OpenSpec change (Experimental) +--- + +Implement tasks from an OpenSpec change. + +**Input**: Optionally specify a change name (e.g., `/opsx:apply add-auth`). If omitted, check if it can be inferred from conversation context. If vague or ambiguous you MUST prompt for available changes. + +**Steps** + +1. **Select the change** + + If a name is provided, use it. Otherwise: + - Infer from conversation context if the user mentioned a change + - Auto-select if only one active change exists + - If ambiguous, run `openspec list --json` to get available changes and use the **AskUserQuestion tool** to let the user select + + Always announce: "Using change: " and how to override (e.g., `/opsx:apply `). + +2. **Check status to understand the schema** + ```bash + openspec status --change "" --json + ``` + Parse the JSON to understand: + - `schemaName`: The workflow being used (e.g., "spec-driven") + - Which artifact contains the tasks (typically "tasks" for spec-driven, check status for others) + +3. **Get apply instructions** + + ```bash + openspec instructions apply --change "" --json + ``` + + This returns: + - `contextFiles`: artifact ID -> array of concrete file paths (varies by schema) + - Progress (total, complete, remaining) + - Task list with status + - Dynamic instruction based on current state + + **Handle states:** + - If `state: "blocked"` (missing artifacts): show message, suggest using `/opsx:continue` + - If `state: "all_done"`: congratulate, suggest archive + - Otherwise: proceed to implementation + +4. **Read context files** + + Read every file path listed under `contextFiles` from the apply instructions output. + The files depend on the schema being used: + - **spec-driven**: proposal, specs, design, tasks + - Other schemas: follow the contextFiles from CLI output + +5. **Show current progress** + + Display: + - Schema being used + - Progress: "N/M tasks complete" + - Remaining tasks overview + - Dynamic instruction from CLI + +6. **Implement tasks (loop until done or blocked)** + + For each pending task: + - Show which task is being worked on + - Make the code changes required + - Keep changes minimal and focused + - Mark task complete in the tasks file: `- [ ]` → `- [x]` + - Continue to next task + + **Pause if:** + - Task is unclear → ask for clarification + - Implementation reveals a design issue → suggest updating artifacts + - Error or blocker encountered → report and wait for guidance + - User interrupts + +7. **On completion or pause, show status** + + Display: + - Tasks completed this session + - Overall progress: "N/M tasks complete" + - If all done: suggest archive + - If paused: explain why and wait for guidance + +**Output During Implementation** + +``` +## Implementing: (schema: ) + +Working on task 3/7: +[...implementation happening...] +✓ Task complete + +Working on task 4/7: +[...implementation happening...] +✓ Task complete +``` + +**Output On Completion** + +``` +## Implementation Complete + +**Change:** +**Schema:** +**Progress:** 7/7 tasks complete ✓ + +### Completed This Session +- [x] Task 1 +- [x] Task 2 +... + +All tasks complete! You can archive this change with `/opsx:archive`. +``` + +**Output On Pause (Issue Encountered)** + +``` +## Implementation Paused + +**Change:** +**Schema:** +**Progress:** 4/7 tasks complete + +### Issue Encountered + + +**Options:** +1.