test(drivers): let a driver report metrics, and a topic carry a configurable prefix - #59
test(drivers): let a driver report metrics, and a topic carry a configurable prefix#59frahlg wants to merge 1 commit into
Conversation
…gurable prefix Two catalog conventions only held because every driver that breaks them is byte-identical to its FTW baseline and therefore skipped. Editing one for any reason fails on a rule it never had to meet, and neither rule was right. test_calls_emit_in_poll required host.emit, which carries a DER reading and needs a DER type the host understands. VALID_DERS has had `heatpump` since the FTW promotion, but no host has a heat-pump reading type, so a heat-pump driver has nothing to emit and reports through host.emit_metric. The rule left an author two options: emit nothing, or claim to be a battery. test_subscription_topics_are_strings required a literal, while heishamon and ctek_hybrid both build the topic from a file-local set in driver_init — the broker prefix is the operator's to choose. The check now asks for what it actually wants: a topic decidable without running the driver. Literals and file-local names concatenate; an index, a field or a call does not, which is how a driver ends up subscribing to whatever a message told it to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
![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: 79132d07a7
ℹ️ 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".
| code = read_driver(driver_name) | ||
| assert 'host.emit(' in code, \ | ||
| f"{driver_name}: should call host.emit() in driver_poll" | ||
| assert 'host.emit(' in code or 'host.emit_metric(' in code, \ |
There was a problem hiding this comment.
When a meter, PV, battery, or other schema-backed driver accidentally removes its host.emit() call but retains any diagnostic host.emit_metric() call, this assertion now passes even though the driver emits no DER telemetry. spec/driver-contract.md requires polling to emit telemetry via host.emit(), while spec/host-api.md explicitly defines metrics as diagnostics outside the DER telemetry schema, so the metric-only exception should be limited to heat-pump/schema-less drivers rather than weakening the contract for every driver.
Useful? React with 👍 / 👎.
| r'(?:"[^"]*"|[A-Za-z_]\w*)' | ||
| r'(?:\s*\.\.\s*(?:"[^"]*"|[A-Za-z_]\w*))*', |
There was a problem hiding this comment.
When a driver assigns runtime data to a temporary, such as local topic = msg.topic, and later calls host.mqtt_subscribe(topic), this regex accepts the call because every bare identifier is treated as static; concatenations containing that identifier pass as well. The test therefore does not enforce its stated protection against message-controlled subscriptions and should verify the origin of each accepted name (or otherwise restrict the allowed configurable-prefix pattern).
Useful? React with 👍 / 👎.
frahlg
left a comment
frahlg
left a comment
There was a problem hiding this comment.
Automated maintenance pass.
(Can't formally approve — this PR's author is the same account this pass posts as.)
Reviewed against AGENTS.md and CONTRIBUTING.md. This is a test-convention fix only — no driver source or manifest touched, so no version bump or CHANGELOG entry is owed here. Both changes are narrowing/correcting existing rules rather than loosening safety-relevant checks:
test_calls_emit_in_pollnow acceptshost.emit_metricalongsidehost.emit, which matches reality:heatpumphas been inVALID_DERSsince the FTW promotion but no host has a heat-pumphost.emitreading type yet, so a heat-pump driver reporting throughemit_metricwas never actually wrong.test_subscription_topics_are_static(renamed fromtest_subscription_topics_are_strings) still rejects a topic built from runtime data (index/field/call) — it only additionally accepts concatenation of string literals and file-local names, which is what heishamon and ctek_hybrid already do indriver_init. Reviewable, not a relaxation of the security-relevant property (topic decidable without running the driver).
All 6 CI checks are green (dco, packages, signed channel accepts this tree, history-secret-scan, drivers, public-boundary). Looks ready to merge.
Flagging rather than merging: the merge API refuses with 403 Merging stacked PRs via this API is not supported. Use the web interface instead because #60 is stacked on this branch. A maintainer needs to merge this one from the web UI.
Generated by Claude Code
|
Automated maintenance pass: reviewed and this looks ready.
I attempted to merge this but the GitHub API refused: "Merging stacked PRs via this API is not supported. Use the web interface instead." — because #60 is branched from this PR's branch. A maintainer will need to merge this one from the web UI. Once it's in, #60's diff will collapse down to just its own heishamon commit automatically. Generated by Claude Code |
1 participant
Two catalog conventions only held because every driver that breaks them is byte-identical to its FTW baseline and therefore skipped. Editing one for any reason fails on a rule it never had to meet. Neither rule was right, so this fixes the rules rather than adding an exemption.
test_calls_emit_in_pollrequiredhost.emit, which carries a DER reading and needs a DER type the host understands.VALID_DERShas hadheatpumpsince the FTW promotion, but no host has a heat-pump reading type yet, so a heat-pump driver has nothing to emit and reports temperatures and power throughhost.emit_metric. The rule left an author two options: emit nothing, or claim to be a battery. It now accepts either call.test_subscription_topics_are_stringsrequired a literal, while heishamon and ctek_hybrid both build the topic from a file-local set indriver_init— the broker prefix is the operator's to choose. Renamed totest_subscription_topics_are_staticand rewritten to ask for what it actually wants: a topic decidable without running the driver. Literals and file-local names concatenate; an index, a field or a call does not, which is how a driver ends up subscribing to whatever a message told it to.make checkis green: 3148 passed, 1323 skipped.Found while making heishamon report
hp_power_w(srcfl/ftw#520). That change is #NEXT, based on this branch.🤖 Generated with Claude Code