Conversation
Landed by the batch lander. The lane's own report and the coordinator's verification are recorded on the pull request.
Landed by the batch lander. The lane's own report and the coordinator's verification are recorded on the pull request.
|
@codex review Head under review: The verification is in the pull request body; the short form is that reverting production with What I want you to attack, in order of how badly it would hurt the operator.
Scope note: enable/disable only. Thresholds and channel patterns are deliberately not editable Disclosure: this change was produced and verified with AI assistance. |
![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: a71c92e873
ℹ️ 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".
| "cmd": "interlock_set_enabled", | ||
| "interlock_name": name, | ||
| "enabled": enabled, | ||
| "operator": self._operator_edit.text().strip(), | ||
| "request_id": uuid.uuid4().hex, |
There was a problem hiding this comment.
When a disable command sent here succeeds, the production SafetyManager adds a software_interlock_disabled readiness blocker; the next typed OperatorSnapshot makes MainWindowV2 call KeithleyPanel.set_safety_ready(False), which disables every per-channel Start button and Start A+B. test_toggle_does_not_disable_any_control misses this because its command stub never delivers the resulting typed snapshot, so the exact operator scenario called out in the review still greys out Start.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| if name == "interlocks": | ||
| widget.interlock_committed.connect(self._apply_interlock_commit) | ||
| stale = self._last_safety_observed_at is None or self._last_safety_observed_at < datetime.now( | ||
| UTC | ||
| ) - timedelta(seconds=_SAFETY_MAX_SOURCE_AGE_S) | ||
| widget.set_interlocks(self._last_interlocks, stale=stale) |
There was a problem hiding this comment.
On a normal launch this always passes None: production SafetyManager._publish_state() publishes only disabled_interlocks, while the complete rows are available only from the interlock_status command, which neither this panel nor the window requests. Consequently _last_interlocks is never populated, _rows stays empty, and the operator sees no controls after startup or Engine reconnect; the tests hide this by manually adding engine.get_operator_state() to synthetic safety telemetry, a condition production never supplies.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| def _on_toggle_result(self, name: str, result: dict) -> None: | ||
| self._last_results[name] = dict(result) if type(result) is dict else {} | ||
| state = result.get("interlock") if type(result) is dict else None | ||
| if result.get("ok") is True and type(state) is dict: |
There was a problem hiding this comment.
When interlock_set_enabled persists and applies the transition but later experiment-provenance settlement fails, the engine deliberately returns ok: false, committed: true with interlock_toggle_reconciliation_failed. This branch treats that as an ordinary rejection and neither updates nor re-queries state, so a successfully disabled interlock can remain displayed as enabled and the operator may retry under a false premise; committed/unknown outcomes need reconciliation against engine-owned state instead of retaining the old row.
AGENTS.md reference: AGENTS.md:L490-L491
Useful? React with 👍 / 👎.
| worker = ZmqCommandWorker(command, parent=self) | ||
| self._register_worker(worker, lambda result, target=name: self._on_toggle_result(target, result)) |
There was a problem hiding this comment.
During ordinary command latency the row keeps its old enabled state, so a double-click starts two workers with different request IDs and both request enabled=False. With an active experiment the first transition creates the open provenance interval, but the second same-state transition replaces the engine's persisted disable_receipt while record_interlock_operator_state() leaves the existing interval bound to the first receipt; on restart, sync_interlock_operator_provenance() cannot find an interval for the second receipt and Engine startup fails. Coalesce an in-flight same-row action or make the command/state owner reject same-state transitions without creating a new receipt.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| "channel_ids": tuple(canonical_channels), | ||
| "enabled": enabled, | ||
| "operator_disableable": disableable, | ||
| "action": action, | ||
| } |
There was a problem hiding this comment.
The engine state already includes comparison and threshold, but canonicalization drops both, so the panel shows channels and eventual action without the boundary whose enforcement is being suppressed. The shipped descriptions do not supply those values—for example the operator cannot see that detector_warmup is the 10 K guard or distinguish the 320 K and 350 K overheat guards—yet Disable executes in one click; displaying these fields read-only would preserve the intended non-editable scope while making the reduction in protection understandable.
AGENTS.md reference: AGENTS.md:L529-L536
Useful? React with 👍 / 👎.
| command = { | ||
| "cmd": "interlock_set_enabled", | ||
| "interlock_name": name, | ||
| "enabled": enabled, | ||
| "operator": self._operator_edit.text().strip(), | ||
| "request_id": uuid.uuid4().hex, |
There was a problem hiding this comment.
After an Engine disconnect the window deliberately retains these rows as stale and leaves their buttons actionable, but this command binds only the interlock name and desired boolean. If the reconnected Engine reuses that name with changed channels, threshold, or protective action, clicking the retained row applies the new policy while the operator is still looking at the old consequence; transport mutation compatibility does not bind the displayed policy. Include the displayed policy fingerprint and Engine generation in the request and reject a mismatch without greying out the control.
AGENTS.md reference: AGENTS.md:L533-L536
Useful? React with 👍 / 👎.
| updated = tuple({**row, "enabled": enabled} if row["name"] == name else dict(row) for row in snapshot) | ||
| self._set_interlock_presentation(updated, stale=False) |
There was a problem hiding this comment.
This applies the command reply without advancing any state revision or timestamp high-water mark. Because command replies and analytics/safety_state arrive over independent ZMQ paths, a fresh-but-pre-command telemetry packet can be processed afterward; _dispatch_safety_evidence() compares it only with the last telemetry timestamp and will overwrite the newly disabled row with its older enabled snapshot. Preserve changed_at or a monotonic state revision from the accepted reply and reject older snapshots so a confirmed OFF state cannot transiently render as ON.
AGENTS.md reference: AGENTS.md:L490-L491
Useful? React with 👍 / 👎.
|
#119's five Windows reds are inherited from its base, not caused by this branch. This is stacked on The first is already fixed on #113's newer head The second is a genuine open defect on #113, measured this turn: the bar is 960 px with only the Nothing here is this pull request's own defect, and I am not going to rebase it onto a The interlock control itself remains verified as posted: production reverted gives 7 failed / |
1 participant
The owner asked for this today, in these words:
an hour after describing why he had left the program entirely:
The gap this closes.
#113makes every interlock operator-optional in the engine — receipts,survival across restart, the disabled interval carried as run provenance — and adds a status bar
that displays which are disabled. Measured on
origin/master,git grep -l interlock -- src/cryodaq/guireturns nothing: there is no interlock surface in theinterface at all. So the ruling was implemented where he could not reach it. He could see that
an interlock was off; he had no control to turn one off.
This adds that control:
src/cryodaq/gui/shell/overlays/interlock_panel.py, reachable fromnavigation and the tool rail, driving the engine-side path #113 already provides rather than a
second mechanism.
Scope, stated so it can be corrected. "Configurable" is implemented as ENABLE/DISABLE per
interlock. Editing thresholds and channel patterns from the interface is deliberately NOT
included — he did not clearly ask for it and it is a much larger surface. One word from him adds
it. The panel shows, for each interlock, which channels it watches and whether it cuts the
source or triggers an emergency shutdown, because those differ sharply in consequence.
Verification, run by the coordinator on Ubuntu 22.04, not taken from the lane's report.
Production reverted with the new tests kept:
The owner's standing ruling is encoded as an executable guard, not a promise.
test_toggle_does_not_disable_any_controlexists because his rule is that nothing may grey outa control — a source may be turned off, but he must never lose the ability to act, since when he
does he drives the hardware by hand outside this program and nothing is recorded at all. A
direct search of the new panel for
setEnabled(False)/setDisabled(True)returns nothing.docs/CLAIM_CORRECTIONS.mdmoves its moving candidate count from 717 to 719, read out of thefreshness guard rather than counted, naming the two paths that entered.
Related but deliberately separate: a sibling branch is making a railed sensor read as a
faulty sensor rather than as 380 K, so that the interlock is never fed a broken reading in the
first place. This pull request is about giving him the switch; that one is about not needing it.
Opened as a draft. Disclosure: this change was produced and verified with AI assistance.