Conversation
Two different hazards were sharing one interlock row, and one of them was stopping the source for a condition that is not a source fault. `detector_warmup` watched T12, the second stage of the GM-cooler where the detector is mounted, and held `stop_source` above 10 K. The mock reports 77.5 K there, so the source could never start at all. The owner ruled the row is KEPT and only its action changes: "do not retire warmup detector. we should know if t12 (2nd stage temp) is too high during measurement phase. it just shouldn't turn off source by force" A warm second stage during a measurement is an EVIDENCE problem: it tells the operator the data he is taking may be worthless, and that information is the point. Retiring the row would have thrown it away. So the row keeps its name, its binding and its 10 K threshold, and now raises an operator-facing warning. The equipment hazard he also named — a source driven above room temperature — is a separate new row, `source_overtemp`, because one warns and one stops. Its threshold is 310 K, not "room temperature". A whole-instrument binding thresholded at 295-300 K would trip forever: the mock reports 300.0 K on channel 8 and a real ambient-reference sensor sits near 295 K. 310 K is above any room reading, far below the 350 K emergency tier, and still catches a genuine runaway. An earlier version of this row shipped 299.0 while its own comment said 310 K; that would have replaced one always-on guard with another. The `emergency_off` rows keep their thresholds, bindings and actions unchanged.
|
@codex review Head under review: Please give the threshold your hardest look, because a bad number here is
One thing you should know rather than infer: the lane that wrote this was |
![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: 32472c3385
ℹ️ 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".
| - {instrument_id: "LS218_1", source_key: "input.6.temperature"} | ||
| - {instrument_id: "LS218_1", source_key: "input.7.temperature"} | ||
| - {instrument_id: "LS218_1", source_key: "input.8.temperature"} | ||
| threshold: 310.0 |
There was a problem hiding this comment.
When any bound T1–T4 channel reads in (310, 320] K, or T5–T7 does so during cooldown/warmup, this row performs stop_source even though config/channels.yaml:37-101 explicitly defines those readings as legitimate; the interlock has no phase or source-active predicate and remains latched until acknowledgement. The new 300 K mock test therefore misses the upper 10 K of the repository's accepted range, so 310 K does replace the old guard with another false-trip path unless the threshold is derived above the documented maximum or the binding is narrowed with physical evidence.
AGENTS.md reference: AGENTS.md:L20-L25
Useful? React with 👍 / 👎.
| await _dispatch_alarm_notification( | ||
| context.event_bus, | ||
| context.alarm_dispatch_tasks, | ||
| alarm_id=condition.name, | ||
| level="WARNING", |
There was a problem hiding this comment.
When T12 exceeds 10 K in production, this branch only emits a WARNING alarm_fired event and does not pass a Telegram notifier or register an active annunciation. The current GUI no longer consumes the recent_alarms ring (tests/gui/shell/test_top_watch_bar_annunciation.py:16 explicitly enforces that), while the standalone assistant rejects every sub-CRITICAL alarm at src/cryodaq/agents/assistant/live/agent.py:993-1007; consequently the warning can remain a log/ring artifact rather than reaching the operator. Publish it through the current AlarmStateManager/AnnunciationRegistry path or another verified live operator surface.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
| threshold: 10.0 | ||
| comparison: ">" | ||
| action: "stop_source" | ||
| action: "warning" |
There was a problem hiding this comment.
On a normal warm startup T12 is above 10 K—the new regression itself expects detector_warmup to become TRIPPED—but InterlockEngine._process_reading evaluates only ARMED records and a later below-threshold reading never clears this state. Because no production GUI code invokes the engine's interlock_acknowledge command, cooling the detector leaves this warning permanently disarmed, so a subsequent real warmup during measurement produces no warning at all. Give observational warnings recovery/re-fire semantics, or wire their visible acknowledgement to InterlockEngine.acknowledge, and add a warm→cold→warm regression.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
| - {instrument_id: "LS218_1", source_key: "input.8.temperature"} | ||
| threshold: 310.0 | ||
| comparison: ">" | ||
| action: "stop_source" |
There was a problem hiding this comment.
If any bound channel is already above the chosen threshold while the source is off—for example during startup or warmup—this always-active row immediately executes stop_source and latches source_overtemp as TRIPPED. SafetyManager.request_run does not consult InterlockEngine state, and subsequent readings are ignored for a tripped condition, so the operator can later start the source while the channel remains between this threshold and the 350 K emergency tier with no source_overtemp protection until an explicit interlock acknowledgement occurs. Gate this condition on the active source lifecycle, retain its violation as a RUN blocker, or otherwise ensure an idle trip cannot disarm the later source-on guard.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
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: This round answers your findings on the config change. The coordinator's control Since your last look, something important was learned that bears on this PR and Docs gate green at this head, derived pair regenerated last. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08ec081c6b
ℹ️ 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".
| threshold: 10.0 | ||
| comparison: ">" | ||
| action: "stop_source" | ||
| action: "warning" |
There was a problem hiding this comment.
When an operator consults the repository guidance after T12 exceeds 10 K, docs/alarms_tuning_guide.md:168-189 still says this interlock executes stop_source and that the source must be switched off, while docs/safety-operator.md:293 omits the new source_overtemp interlock entirely. With this line now issuing only a warning, those instructions can make the operator believe a hazardous output was stopped when it remains active; update both operator-facing documents in this slice to describe the new action, threshold, lifecycle gate, and acknowledgement name.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
| if reading.is_usable(): | ||
| for record in protected_matching: | ||
| if ( | ||
| record.condition.action == "warning" | ||
| and record.state == InterlockState.TRIPPED | ||
| and not record.condition.is_triggered(reading.value) | ||
| ): | ||
| await self._recover_warning(record, reading) |
There was a problem hiding this comment.
When T12 oscillates across 10 K within the configured five-second cooldown, this immediate safe-side recovery re-arms the warning, and the next warm sample still invokes the production trip handler even though _trip computed suppress_notification=True. Reproducing warm→cold→warm produces two alarm_fired events, so sensor noise can beep the GUI on every two-second poll despite the cooldown; suppress downstream warning dispatch during cooldown or debounce/hysteretically confirm recovery, with a regression covering this boundary.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
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: This round answers your findings on the previous head. The coordinator verified Context you should have, because it bears on how hard to press here: on a tree So the thing worth your scepticism is the threshold and the binding, not whether Docs gate green at this head; derived pair regenerated as the last commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fccd32ab4
ℹ️ 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".
| if condition.action == "warning": | ||
| if context.event_bus is None or context.experiment_manager is None or context.alarm_state_manager is None: | ||
| raise RuntimeError("interlock warning publication path is unavailable") |
There was a problem hiding this comment.
When detector_warmup warning publication fails—for example, if event_bus.publish() raises—this branch falls into the generic exception handler, which calls SafetyManager.latch_fault() and, with an active source, performs the emergency-OFF path. Thus the new observational warning still acquires actuator authority on a notification-path failure; handle warning failures separately while retaining fail-closed escalation for actual control actions.
AGENTS.md reference: AGENTS.md:L466-L467
Useful? React with 👍 / 👎.
| предупреждение только после 5-секундного cooldown; ручное имя acknowledge — | ||
| `detector_warmup`. |
There was a problem hiding this comment.
When the responsible engineer follows this new detector_warmup acknowledge instruction after T12 has cooled, InterlockEngine.acknowledge() changes only the interlock record to ARMED; warning recovery runs only for TRIPPED records, so subsequent safe readings never call AlarmStateManager.process(..., None, ...) and the GUI annunciation remains active indefinitely. Either clear both state owners in the acknowledge path or remove the unsupported manual-ack instruction and rely on automatic recovery.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
| threshold: 10.0 | ||
| comparison: ">" | ||
| action: "stop_source" | ||
| action: "warning" |
There was a problem hiding this comment.
When the default canonical interlock tests run, this action change makes both affected regressions fail: test_canonical_channel_ids_trip_every_configured_t_interlock includes warning in expected_actions even though warning actions intentionally have no callable, and test_interlock_follows_declared_sensor_binding_after_canonical_id_rename still requires actions_seen == ["stop_source"]. Update those assertions to verify the warning trip and absence of source control so the full suite can pass without restoring the unsafe behavior.
AGENTS.md reference: AGENTS.md:L296-L298
Useful? React with 👍 / 👎.
| "!!! БЛОКИРОВКА СРАБОТАЛА !!! " | ||
| # Защитные действия остаются CRITICAL; наблюдательный warning не | ||
| # должен заявлять, что источник был заблокирован. | ||
| log = logger.warning if condition.action == "warning" else logger.critical |
There was a problem hiding this comment.
Now that the expected warm-detector event is emitted at WARNING, the unchanged scripts/soak_mock_engine.py:40-44 allowlist r"detector_warmup" is no longer needed, but it still suppresses every ERROR or CRITICAL record containing that name. For example, the production warning_recovery_handler failed for interlock 'detector_warmup' ERROR is discarded by scan_log(), allowing the nightly or 72-hour soak to certify a broken warning path as clean; remove the entry or narrow it to the exact former benign record.
AGENTS.md reference: AGENTS.md:L382-L386
Useful? React with 👍 / 👎.
| - name: "source_overtemp" | ||
| channel_pattern: "Т[1-8] .*" |
There was a problem hiding this comment.
When a maintainer uses this newly documented “current configuration” row, it supplies only channel_pattern, but InterlockEngine.load_config() unconditionally calls resolve_interlock_channel_bindings() and rejects any row without a non-empty channel_bindings list. The example therefore fails engine startup and also hides the exact physical bindings this safety row depends on; show the production binding schema or clearly label the block as non-loadable pseudocode.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
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: This round answers your five findings. Coordinator's control: 4 nodes red Two of those four only go green once the round's DOCUMENTATION changes are The round also touches The threshold question stands and is the one I would most like pressed: if any Docs gate green at this head; derived pair regenerated as the last commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f41a42a255
ℹ️ 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".
| return | ||
| if context.alarm_state_manager is None: | ||
| raise RuntimeError("interlock warning recovery path is unavailable") | ||
| context.alarm_state_manager.process(condition.name, None, {}) |
There was a problem hiding this comment.
When T12 returns to the safe side after the cooldown, this removes detector_warmup from AlarmStateManager but never publishes the corresponding alarm_cleared event. The warning's alarm_fired event is relayed to event consumers, and PeriodicAlarmProjection._apply() removes it only upon alarm_cleared, so the assistant projection can continue presenting the recovered warning until its next 240-second snapshot reconciliation. Emit the same clear event used by the normal alarm-v2 recovery path when this state transition succeeds.
Useful? React with 👍 / 👎.
|
Correction to my earlier comments on this pull request. I said that The failure was in MY environment. That test spawns a subprocess to prove there With The operational advice was still right — that node is not this branch's problem |
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: This round answers your remaining finding. Coordinator's control: 1 node red That node is the one worth your attention: the warm-detector row is now a WARNING The threshold question stands and I would rather you pressed it than passed it: Docs gate green at this head; derived pair regenerated as the last commit. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Two different hazards were sharing one interlock row, and one of them was
stopping the source for a condition that is not a source fault.
What was wrong
detector_warmupwatched T12 — the second stage of the GM-cooler, where thedetector is mounted — and held
stop_sourceabove 10 K. The mock reports 77.5 Kthere, so the source could never start at all, and the end-to-end run timed out.
The ruling
A warm second stage during a measurement is an evidence problem: it tells
the operator the data he is taking may be worthless, and that information is the
point. An earlier plan to retire the row would have thrown it away.
A heater running away above ambient is an equipment problem.
One warns. One stops. They are separate rows now.
The change
detector_warmupkeeps its name, itsLS218_2input.4.temperaturebindingand its 10 K threshold. Only its action changes, to an operator warning.
source_overtempis a new row that stops the source above 310 K.Why 310 K and not "room temperature". A whole-instrument binding thresholded
at 295–300 K would trip forever: the mock reports 300.0 K on channel 8, and a
real ambient-reference sensor sits near 295 K. 310 K is above any room reading,
far below the 350 K emergency tier, and still catches a genuine runaway. An
earlier version of this row shipped
threshold: 299.0while its own commentsaid 310 K — that would have replaced one always-on guard with another, which is
the exact failure this row exists to avoid.
The
emergency_offrows keep their thresholds, bindings and actions unchanged.Verification
The lane that produced this was killed before it reported, by a coordinator
error — a
pkill -fpattern matched the brief text inside its command line. Itsfiles survived intact, but its own account of the work did not, so every claim
here was measured by the coordinator from scratch, not carried over.
In a throwaway worktree at the base commit, with the new test present and the
config change absent: 5 of 7 nodes fail. Applying the config change: 7
pass. The failing set covers the row being kept as a T12 warning, the warning
dispatching without a control action, warm mock readings not requesting a source
stop, the new row stopping above threshold, and — the one that matters most —
the new row not tripping at the mock's 300.0 K ambient.
Docs gate green at this head; the derived pair was regenerated as the last commit.
Not determined
Disclosure
Written by an AI lane (
gpt-5.6-sol) under an agent brief. The lane's own reportwas lost, so the verification above is entirely the coordinating agent's.