docs(tasks): make the two open decisions pickable without me in the room - #659
Conversation
Both carried their evidence and neither carried a recommendation, so whoever picked them up would have had to redo the investigation before they could choose. 3.10 now says what is actually dead because no category declares `electrical_generation` — the source type, four branches in the data service, `totalProductionKwh` and the `netKwh` that subtracts it, the timeseries series — and notes that nothing looks broken because the panel guards its solar pill on `hasProduction`. Three options with their costs, a recommendation to leave it unless solar is near-term, and the one move to avoid: grafting the channel onto an unrelated category, which would let a device declare a channel nothing on it can produce. Controller support had seven paragraphs of constraints and no order of work. It now says which decision unblocks the others — where the actuator mapping lives, because it decides the schema — which categories to take in which order and why they differ, what to read first (Home Assistant's generic_thermostat, which separates hvac_mode from hvac_action exactly as this section argues for), and what the deliverable is: a design spec, then one task per phase rather than one task for all of it. Also corrects the open follow-up count, which #654 and #655 left behind.
![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: 972a7a6f6d
ℹ️ 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".
…learn Decision 1 — an exclusive, durable mapping that sweeps can see — is very nearly the problem the energy claim solved this week, and none of what it cost was written down anywhere the next person would look. So the section now carries the shape that worked (a nullable FK column with a partial unique index and ON DELETE SET NULL, and the invariant that makes the promotion write cheap), and the four things that were not obvious until they broke: release on every exit path of the gate, promote only to a successor that could have earned it, sweep for the paths that cannot name what they released, and condition the write so a lost race is a no-op rather than a contradiction. Plus the traps beside it. `DeviceStructureLockService` covers create and update and not remove, which is why the claim leans on the constraint rather than the lock. CI builds the schema from entity decorators while installations upgrade through migrations, so a constraint declared once is missing exactly where the tests run. Core reaches a plugin's private knowledge through registries rather than columns, twice now. The wizard and the write path read one function, so a preview cannot offer what the create refuses. And this kind of change is tested by driving real components against sqlite, because a mock at any of those seams tests the mock. 3.10 gains a smaller correction: the `electrical_generation` channel is already specified with production, power, active and fault, so option 2's missing piece is only the device category.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62af7233ed
ℹ️ 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".
Generation is not dead code, which is the one that mattered. The category table governs what can be *offered* — the wizard builds from it, and `DeviceValidationService.isChannelAllowedForDevice()` is its only reader — not what can be persisted: `ChannelsService.create()` never consults that check, and the ingestion classifies from the persisted channel/property pair. So an integration that creates an `electrical_generation` channel is ingested today, and the option to delete the mapping would have removed working support. Ruled out rather than listed. What is actually unreachable is generation through anything the specification drives, which is an asymmetry rather than dead code — and the fact that the category table is advisory at persistence for every channel is a bigger question than solar, so it is noted as its own. The interlock is not the last phase's problem: `air_conditioner` requires `cooler` and declares `heater` optional, so a mapped heater gives it the same two opposed actuators a thermostat has. Saying otherwise contradicted the paragraph above it, which already says the interlock applies by shape rather than by category name. What is particular to `thermostat` is that both actuator channels are optional. And a synthesized `status` follows the actuator's confirmed state, never the loop's decision — the decision list steered toward exactly the failure mode the safety invariant beside it exists to prevent.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cb9977039
ℹ️ 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".
Two corrections to the precedent block, both about carrying a lesson across a boundary where it changes. Exclusivity was stated in the direction a foreign key already gives: one actuator per controlled channel still leaves a relay free to be bound by several channels, which is the case that matters. The invariant is at most one controlled channel per *actuator* — the direction the FK does not give, and what the partial unique index is for. And promotion inverts. A released meter must find a new claimant or it stops being counted, so promotion is the safe answer there; a released relay must stay unclaimed until an operator maps it, because promotion would hand a control loop hardware nobody chose. Attribution is bookkeeping and can be repaired silently; energising a relay cannot. The sweep keeps its job and changes its verb: report what a released relay left behind rather than adopt it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2688467217
ℹ️ 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".
The opening still said the channel cannot appear on any device while the paragraph below established that an integration can persist one. What the category table actually forecloses is specification-driven creation — the wizard's slots and structural validation — which is also the honest form of the sentence about the cross-type guard beside it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91238bc2db
ℹ️ 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".
"At most one controlled channel per actuator" stops a second controller and nothing else, and the relay has other writers. This design deliberately lets one source property feed several virtual devices, so a virtual switch can project the same relay's `on` and expose it as a toggle; and a stored scene action commands the physical property directly, since `@ValidateDeviceNotHidden` refuses only new references and leaves existing ones executing. Either bypasses the loop's serialisation, which means either can energise a relay the controller believes it released, or start a cooler while its heater runs — the interlock defeated from outside the mechanism enforcing it. So the claim either withholds the property from every other writable reference and command path, or those paths are mediated, and the trade is stated rather than assumed: withholding breaks the one-source-many-devices principle for one kind of property, mediating makes the controller a command path other modules have to know about. Serialisation moves to 5 with a note that it only orders what goes through it, which is what makes this a prerequisite. Also strikes a claim in the same document that the code no longer supports: the energy guard does not skip projections wholesale any more — a projected meter has one accountable claimant and a second projection is refused at persistence.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd7db3b9a7
ℹ️ 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".
The energy claim lives on a projection row and names the property that row projects, so "null, or equal to sourcePropertyId" is meaningful there. An actuator is explicitly not a slot of the virtual device — that is the whole reason it has nowhere to live yet — so there is no projection row for it and no sourcePropertyId to equal. Carrying the invariant across would either force an unrelated sensor or setpoint projection to become the relay's, or make the mapping unstorable. Uniqueness and ON DELETE SET NULL still transfer. The actuator's property id stays independent, on the controlled channel or on a mapping entity of its own, and the conditional write names that row's own mapping. Second place the precedent stops, after promotion. Both are marked in the list rather than left for the implementer to hit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a29cc69a1
ℹ️ 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".
Withholding a property from other references, or mediating the paths that reach it, bounds only what this application does. A relay imported from Home Assistant stays commandable by HA automations and the HA UI — the plugin subscribes to `state_changed` and learns about a toggle after it happened — and the same holds for a Shelly running its own schedule or a wall switch wired across the relay. The loop does react, since an actuator value event is already one of its triggers. What it cannot do is prevent the window in between, and for two opposed actuators that window is a heater and a cooler running together. So the design owes an upstream-writer boundary as well, and the honest options are narrower: a hardware interlock, which makes the contradiction impossible rather than corrected; exclusive upstream control, which can be required and documented but not enforced; or refusing the opposed-actuator configurations where neither can be guaranteed. A single-actuator heater is unaffected — an external write there costs a cycle rather than a conflict.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 504acf0b16
ℹ️ 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".
Serialising each evaluation preserves nothing where the platform acknowledges asynchronously. One evaluation dispatches heater=on and releases; the confirming value event has not arrived; the next trigger takes the lock, reads the heater as off, and energises the cooler. Both outputs on, with the stated serialisation satisfied throughout. The invariant that state follows the actuator is what makes it invisible: committing on confirmation is right for reporting, and it leaves an in-flight command nowhere the next evaluation can see it. So the boundary is confirmation. Either hold serialisation through the round trip — which needs a policy for an actuator that never confirms, or the device stops evaluating — or represent outstanding commands as interlock state, so a pending heater=on counts as active while `status` still waits. The second keeps the reporting invariant and costs an explicit answer for how long an unconfirmed command stays outstanding.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc578d63fd
ℹ️ 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".
Two more, both from review. The `status` requirement is on the heater and cooler *channels*, not on the three thermal device categories — which is the same by-shape-not-by-name distinction the interlock turns on. A thermostat's selected heater or cooler channel requires one exactly as `heating_unit`'s does, so scoping the decision by category left the last phase without an answer and its devices failing structural validation. And a timed-out energising command must stay interlock-blocking. The question a lifetime answers is not how long the record lives but what is assumed when it runs out, and a command that timed out can still complete late — the platform was slow, not silent. Expiring the record and treating the output as off is how both actuators end up on with every stated rule obeyed. A timeout drives reconciliation against the actuator, and only its answer releases the interlock.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fa76f499e
ℹ️ 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".
…t safe by itself Two more from review, both places where I left an option unconstrained. A mapped `status` was governed by nothing. Mappings are deliberately cross-device and reusable, so an unrelated read-only boolean satisfies the slot's type and permissions perfectly while reporting a unit that is not this one — and `SpaceClimateStateService` consumes whatever lands there as the unit's actual activity. Synthesized status was already required to follow the actuator's confirmed state; mapped status now carries the same requirement, because both routes end in the same claim and only the relay can make it. And a single actuator is not unaffected by upstream writers, which was a tidy sentence rather than a true one. An automation or a wall switch that energises a lone heater after the user disabled it heats a room nobody asked to heat, the loop learns after actuation, and its corrective release can fail or arrive late — while hysteresis and minimum-cycle protection are bypassed outright, and minimum-cycle protects the hardware rather than the comfort. Exclusive upstream ownership or an equivalent fail-safe is owed for every actuator; hardware interlocking or refusal sits on top of that, for opposed pairs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fd5417d67
ℹ️ 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".
Two more, both about the in-flight command the previous round introduced and neither closed by it. Reading the actuator back does not prove a timed-out `heater=on` can no longer take effect: the readback can happen while it is still queued upstream, and answering "off" releases the interlock in front of a command that then lands. What proves the output safe is that the earlier command cannot take effect — a cancellation the platform honours, ordering or correlation showing it drained, or an explicit off whose confirmation arrived after it. A bare observation of "off" proves only that it had not arrived yet. And a process that dies between dispatch and confirmation takes the lock or the pending record with it, so a fresh start reads the heater as off, energises the cooler and receives the delayed command afterwards — the same failure by way of a deployment. Outstanding energising commands are durable, or evaluation stays blocked until startup reconciliation shows nothing from before the restart is in flight. Which is what the cycle state above already requires, for the reason already written there: protection kept only in memory is no protection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ded97fd2a4
ℹ️ 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".
A relay that has confirmed ON stays on until something turns it off. If the backend crashes, loses power or sits partitioned from the device, nothing does — the tick cannot run, and the restart rules only help if the service comes back. Exclusive ownership makes this worse rather than better: it is precisely the arrangement in which no other writer will intervene either. A control loop cannot be the only thing between a heating element and an indefinite ON. So the answer has to live outside this process: an actuator-local maximum-on or watchdog timeout, fail-closed hardware, or refusing configurations where neither is available. Where the platform supports it, prefer a dead-man's switch — a relay with its own auto-off timer that the loop re-arms while it wants the output — because the controller disappearing then releases the actuator instead of stranding it. That inverts the failure mode rather than shortening it, and it is the only one of the three that does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdcf5c45ae
ℹ️ 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".
`SetpointOrderConstraintValidator` checks heating < cooling only when both arrive together — by design, "if either is not provided, no cross-field validation needed" — and `executeSetpointSetIntent()` writes only the bound it was given. Two successive single-bound updates therefore persist a crossed pair: cooling 24, then heating 25. Inert in v1, where nothing acts on a setpoint. Under a loop it is an instruction to hold a room above 25 and below 24 at once, and the answer is changeover oscillation that minimum-cycle protection then masks as slow cycling rather than reporting as a contradictory configuration. So the command path validates the merged persisted pair rather than its payload — and ordering alone is not enough, since heating 22.0 with cooling 22.2 oscillates just as surely under ±0.3 hysteresis. The gap has to be at least the band the loop uses, which makes the deadband a property of the pair and something both the wizard and the intent path have to be able to state.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7f0809725
ℹ️ 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".
Two more, both undercutting things written a round earlier. The dead-man's switch I recommended is renewed by the loop, and the loop is driven by the sensor: a sensor stuck low while still publishing keeps the freshness check happy, so the loop goes on wanting heat, goes on re-arming, and the timer never fires — a water heater energised indefinitely with every rule here obeyed. Whatever bounds the output has to be underivable from the same sensor decision: an absolute maximum-on the loop cannot renew, or an independent high-limit measurement, which is what a real thermal cut-out is. A configuration with neither is one to refuse. And minimum-cycle protection has a direction nobody had stated. It protects hardware from short cycling, and applied indiscriminately it delays the one thing that must never wait — an explicit OFF, or a release demanded by stale or faulted input. An implementation could satisfy every cycle rule while continuing to heat after somebody said stop. Cycle timing gates energising and non-safety changeovers; releases are dispatched immediately and retried until confirmed.
1 participant
Both remaining virtual-devices decisions carried their evidence and neither carried a recommendation, so whoever picked one up would have had to redo the investigation before they could choose. This adds the missing half.
3.10 —
electrical_generationhas no homeAdds what is actually dead because 0 of 32 device categories declare the channel:
EnergySourceType.GENERATION_PRODUCTION, its row inSOURCE_TYPE_MAP, four branches inenergy-data.service.ts,totalProductionKwhand thenetKwhthat subtracts it, and theproduction_delta_kwhseries — a promise the API makes and cannot keep. Nothing looks broken, because the panel guards its solar pill behindhasProduction.Three options with their costs (leave it / give the channel a home / delete the mapping), and a recommendation: leave it unless solar is near-term, in which case it belongs to that feature rather than to a follow-up. Deleting buys accuracy at the price of a breaking API change that would be undone the moment an inverter integration lands.
Also states the one move to avoid — grafting the channel onto an unrelated category — which would let a device declare a channel nothing on it can produce and start the energy module classifying readings that mean something else.
Controller support
Seven paragraphs of constraints, no order of work. Adds:
unhideAbandonedSourcesdoes not read is auto-unhidden on the next start;statusis, how a command reaches the setpoint, and what serialises around what;heating_unit/water_heaterfirst (one actuator, booleanstatus),air_conditionernext (same shape, inverted), the humidity pair after (enumstatusneeds a vocabulary),thermostatlast (the only one with two opposed actuators, and the only one needing the interlock);generic_thermostat, whosemin_cycle_duration, tolerances,keep_aliveandac_modeare the shape of the protections described, and whose climate entity separateshvac_modefromhvac_actionexactly as this section argues for, arrived at independently;What the energy claim cost to learn
Decision 1 — an exclusive, durable mapping that sweeps can see — is very nearly the problem
BUG-ENERGY-VIRTUAL-ROOM-ATTRIBUTIONsolved this week, and none of what it cost was recorded where the next person would look. The section now carries:ON DELETE SET NULL, plus the invariant (null, or equal tosourcePropertyId) the promotion write conditions on;DeviceStructureLockService's hole — it coverscreate(:403) andupdate(:510) and notremove(:556), which is why the claim leans on the constraint rather than the lock, and why an actuator leaning on the lock alone would inherit the race;3.10 gains a smaller correction: the
electrical_generationchannel is already specified withproduction,power,activeandfault, so option 2's missing piece is only the device category.Also
Corrects the open follow-up count on the roadmap, which #654 and #655 left behind — 15 of 38 done, 23 open. Supersedes #658, which did only that.
Docs only.
🤖 Generated with Claude Code