fix(drivers): close Lua host holes for command, default mode, and fingerprint - #1199
Conversation
…gerprint A missing driver_command is an error instead of a silent success. A non-read-only driver that declares controls or a battery/EV/V2X/heat-pump command path must implement driver_default_mode before it starts. Fingerprint probes deny mutating host verbs even for bundled drivers. The catalog parser now reads auth_post_path.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_341bb692-9c52-4531-b9ce-afb4f5eee825) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79d180bc6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Tightens the Lua driver host contract after the device-drivers audit (companion PR srcfl/device-drivers#112). Three real safety gaps closed:
Command()silently succeeded whendriver_commandwas undefined — now returns an error instead of pretending the write happened. This was the sharpest one: a caller had no way to know a command was dropped on the floor.RunFingerprintnow setsProbeReadOnly, so a passive fingerprint probe can't callmodbus_write/mqtt_pub/http_post/http_patcheven on bundled drivers (whereallowWritewas previously a no-op without a signed policy).- Legacy drivers that can be commanded + declare an actuation capability (battery/EV/V2X/heatpump) or catalog
controlsnow requiredriver_default_modeat start, with read-only and telemetry-only drivers correctly exempted.
Test coverage matches the logic well — TestLegacyBatteryCommandDriverRequiresDefaultMode, TestReadOnlyBatteryMayOmitDefaultMode, TestBatteryTelemetryDriverWithoutCommandMayOmitDefaultMode, TestFingerprintProbeCannotWrite, TestLuaDriverMissingCommandIsError all cover the new branches, not just the happy path. CI green (Core Linux binaries, Windows config, go test + vet, Device Support driver contract).
Minor: this is a behavior change for any driver relying on the old "missing driver_command = silent success" shape, but that's exactly the bug being fixed, and the changeset documents it.
Safe to merge from my read.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Good hardening pass on the Lua host contract, three real holes closed:
Command()on a driver with nodriver_commandused to returnnil(silent success) — now an explicit error. That's a real "commanded a device, nothing happened, no one noticed" bug closed.RunFingerprintnow setsHostEnv.ProbeReadOnly, andallowWritechecks it before falling through toRuntimePolicy == nil → allow. Previously a bundled driver with no signed policy couldmodbus_write/http_postduring a passive fingerprint probe — that's the actuation-during-probe hole, now closed at the host layer regardless of policy state.legacyDriverRequiresDefaultModewidens the startup gate from "declarescontrols" to "hasdriver_commandAND an actuation capability (battery/EV/V2X/heatpump)" — broader and more honest given the audit found no bundled driver even declarescontrols.
Well tested: missing-command error, probe-cannot-write (asserts zero writes reached the mock), and all three default-mode branches (must-have / read-only-may-omit / telemetry-only-may-omit).
One question before merge, not a blocker on the code itself: the PR body says this is host-only and the companion device-drivers fix (making bundled Lua drivers comply) hasn't landed yet — bundled pin unchanged. Point 3 above is strictly stricter than today's gate (hasCommand && actuationCapability, not just controls). Is there a CI check that loads all 38 currently-bundled drivers through Registry.Add / legacyDriverRequiresDefaultMode to confirm none of them currently have a command path + battery/EV/etc capability without driver_default_mode? If not, merging this ahead of the companion PR risks a currently-shipped driver failing to start. Worth a human confirming that sequencing before merge — the code change itself looks correct and well-scoped.
Security screen: no secrets, no new deps; the interesting security surface here (fingerprint write-gating, command-contract) is the point of the PR and looks correctly tightened.
#112) Driver audit against the FTW host contract (companion: srcfl/ftw#1199). - fronius_smart_meter 2.1.2, zuidwijk_p1 1.1.1: stop fabricating 0 W/0-value telemetry on a failed/missing read; declare read_only - tesla_vehicle 0.2.2: cache replay sets soc_fresh = false - pixii 2.1.4: serial probe no longer gated on an unset `ok` - easee_cloud 1.3.1: config_secrets = {"password"}; driver_default_mode pauses and writes 0 A - ferroamp 2.1.2: host.set_sn(config.serial) when set - Telemetry-only hybrids/meters (sonnen, goodwe, growatt, sofar, kostal, sma, victron, fronius, pixii_pv, solis_string, tibber) declare read_only = true - zaptec_cloud 0.1.0 and tesla_wall_connector 0.1.0 promoted from FTW testdata Pre-merge review found zaptec_cloud shipped control-capable with no hardware verification and a phase-order bug in ev_set_current (computed amps from the old phase count before applying a same-call phase change). Fixed by keeping it read-only/no-op like tesla_wall_connector until a real charger verifies the control path end-to-end; DCO sign-off added to both commits. make check: 4404 passed, 1069 skipped.
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
…ract Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
…ract Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_37f824ad-1a59-442f-ace4-3366f82d5d3a) |
2 participants
Core could report a successful command when a Lua hook was missing, accept a controllable read-only driver without a safe default, and omit PV-only control from the default-mode gate. Missing commands now fail, both Registry and Lua reject read-only commands before the hook runs, and PV control requires a default. Fingerprint probes cannot use mutating host calls; the catalog reads auth_post_path.
Move the recovery pin to device-drivers #112, including its telemetry fixes and safe-default/read-only declarations. Include the read-only Zaptec Cloud and Tesla Wall Connector drivers and regenerate the shipped inventory. Keep the exact reviewed Ferroamp source hashes: the new version changes only version and configured serial identity; its PV limit and release code is unchanged. Unknown source bytes still receive no PV control proof.
Validation:
make verifypasses on current master including #1170; driver and EV-cloud packages pass; the version check across the pin passes for all 40 bundled drivers. A new test loads each pinned Lua file and applies the same default-hook gate as Registry startup. Tests cover refused read-only commands, missing hooks, PV-only actuators, probe write denial, PV proof identity and revocation. This proves host contract compatibility and simulated recovery paths, not safe-default behavior on every physical device. Native bundle integrity passes; native worker execution is unavailable on this Mac. No UI changes, release or install.