test(drivers): let a driver report metrics, and a topic carry a configurable prefix - #59
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>
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".
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 |
miravoss26
left a comment
There was a problem hiding this comment.
Read the whole diff — two test-contract loosenings, both well-argued.
test_calls_emit_in_pollnow acceptshost.emit_metric(alongsidehost.emit(. Right call: a heat-pump driver has no DER reading type to emit yet, so metrics are its honest output — the alternative was "emit nothing or claim to be a battery." The docstring captures the why.test_subscription_topics_are_staticswaps the literal-only rule for literal-or-concatenation-of-file-local-names. There.fullmatchcorrectly rejects runtime-derived topics (index/field/call) while allowingPREFIX .. "/status".
One note, not a blocker: the check is syntactic — a bare identifier passes even if it held runtime data. In practice the sandbox keeps those file-local so this matches intent; just flagging that the guarantee is "looks static," not "is constant."
Security screen: no secrets, no new deps, no network — this tightens a subscribe-injection guard, doesn't widen it. CI green. Safe to merge from my read; leaving the merge to a human (device-drivers isn't on my auto-merge allowlist).
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
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