Repository navigation
W5-04b: Not-Aus in der App mit 10-Sekunden-Frist - #125
Conversation
Adds estop::enforce (injectable clock): kill every PTY session, re-signal survivors, and report success only once an empty fleet is observed; a fleet that cannot be counted or is alive at the deadline is an error (fail closed). Tauri commands get_emergency_stop/set_emergency_stop write the W5-04a barrier first, then enforce. Settings gets a small Not-Aus control. Test-First: src-tauri/src/estop.rs::tests::a_fleet_alive_at_the_deadline_is_reported_not_stopped Test-First: src-tauri/src/estop.rs::tests::a_fleet_that_cannot_be_counted_is_never_reported_stopped Test-First: src/components/EmergencyStop.test.tsx::keeps the stop up and shows the error when the end was not confirmed Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 1761f9866a
ℹ️ 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".
| tauri::async_runtime::spawn_blocking(move || { | ||
| estop::enforce(&manager, &estop::SystemClock, estop::DEADLINE) | ||
| }) |
There was a problem hiding this comment.
After this polling call returns successfully, the persistent barrier still does not prevent the existing spawn_pty, create_worker, or respawn_worker paths from reaching PtyManager::reserve_session, which does not consult emergency-stop state; the database triggers only protect queue/continuous transitions. A direct start immediately after the last empty-fleet observation therefore survives indefinitely while the UI says the global stop is active. Tie PTY admission to a persistent, race-free stop latch rather than relying only on this ten-second kill loop.
Useful? React with 👍 / 👎.
| {active | ||
| ? "Not-Aus ist aktiv: keine neuen Aufgaben, alle Agenten sind beendet." | ||
| : "Beendet binnen 10 Sekunden alle laufenden Agenten und stoppt neue Aufgaben."} |
There was a problem hiding this comment.
When activation rejects because a session remains alive after ten seconds, line 34 intentionally sets active to true, but this branch then announces that all agents have ended; the same false assertion appears when the initial state read fails. These are precisely the cases where termination is unconfirmed, so the safety UI gives contradictory reassurance alongside the error. Track barrier state and termination confirmation separately and reserve this message for a successful observation.
Useful? React with 👍 / 👎.
| ipc.setEmergencyStop.mockReset(); | ||
| }); | ||
|
|
||
| it("raises the stop and shows it active", async () => { |
There was a problem hiding this comment.
This source-changing commit has no recognized Test-First, Regression-For, or No-Test trailer (git log -1 --format='%(trailers)' 1573c006 is empty), despite the report claiming a trailer exists, so the mandatory red-first proof cannot accept this commit. Add the appropriate test trailers to the commit metadata.
AGENTS.md reference: AGENTS.md:L155-L158
Useful? React with 👍 / 👎.
| @@ -0,0 +1,165 @@ | |||
| //! App side of the global emergency stop (W5-04b). The store half (W5-04a) | |||
There was a problem hiding this comment.
The commit adds 318 lines across runtime code and tests with no deletions, exceeding the repository's hard 300-diff-line package maximum; the commit report also acknowledges this overage. Split the change before dispatch rather than landing this oversized package.
AGENTS.md reference: AGENTS.md:L13-L15
Useful? React with 👍 / 👎.
| fleet.kill_all(); | ||
| let remaining = fleet.live().map_err(|error| { |
There was a problem hiding this comment.
When kill_all races a session between reserve_session and spawn_with_id, it changes the registry entry from Reserved to CancelledReservation; begin_spawn then rejects it, but the ordinary spawn and spawn_bound failure paths do not call cancel_reservation, while live_session_ids continues counting every registry key. This loop therefore waits ten seconds and reports a live session forever even though no child was spawned, and later stop attempts encounter the same stale entry. Ensure cancelled reservations are removed or acknowledged by their failed spawn path.
Useful? React with 👍 / 👎.
| manager: State<'_, PtyManager>, | ||
| active: bool, | ||
| ) -> Result<(), String> { | ||
| store.set_emergency_stop(active, "app").await?; |
There was a problem hiding this comment.
The ten-second timer is created only after store.set_emergency_stop completes, but this store uses a five-second SQLite busy timeout. When another writer holds the database, the command can wait about five seconds before it even signals the fleet and then allow another ten seconds, so an agent may remain alive for roughly fifteen seconds after the user presses Not-Aus; if the store operation fails, no termination is attempted at all. Measure the deadline from command entry and bound the barrier/termination sequence by the remaining time.
Useful? React with 👍 / 👎.
| return ( | ||
| <div className="settings-field" data-testid="emergency-stop"> | ||
| <span className="settings-label">Not-Aus</span> |
There was a problem hiding this comment.
This adds a visible safety control to the settings screen, but the commit report explicitly says no screenshot was taken; component tests do not establish that the button, warning state, error, spacing, or contrast are actually usable in the rendered application. Capture and inspect the new UI before treating the visual change as verified.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
|
Queued — the merge queue status continues in this comment ↓. |
Test-First: src/components/EmergencyStop.test.tsx::keeps the stop up and shows the error when the end was not confirmed Test-First: src-tauri/src/pty.rs::kill_all_blocks_new_sessions_until_the_stop_is_cleared Test-First: src-tauri/src/main.rs::emergency_stop_changes_are_serialized
Regression-For: a1f9d7c
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 11 minutes 34 seconds in the queue, including 11 minutes 23 seconds running CI. ReasonThe merge conditions cannot be satisfied due to failing checks
Failing checks: HintYou may have to fix your CI before adding the pull request to the queue again. Waiting for
All conditions
Requeued — the merge queue status continues in this comment ↓. |
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 22 minutes 27 seconds in the queue, including 12 minutes 15 seconds running CI. Required conditions to merge
|
Die App hat jetzt einen globalen Not-Aus in den allgemeinen Einstellungen. Die Prüfung hat zwei Sicherheitslücken geschlossen: Nach einem Fehler behauptet die Anzeige keinen bestätigten Stillstand mehr, und nach dem Stop können keine neuen PTY-Sitzungen zwischen Prüfung und Rückmeldung starten. Zwei unabhängige Reviewer anderer Anbieter als der Claude-Autor haben den Kandidaten und anschließend den Sicherheits-Fix geprüft; es ist kein hoher Befund offen.
Report
What changed
src-tauri/src/estop.rs: bounded polling with an injectable clock, repeated kill signals, and success only after an empty fleet is observed.src-tauri/src/main.rs: Tauri commands persist the global barrier before enforcing the stop; raise/clear operations are serialized, and startup restores the fail-closed PTY latch from the persistent state.src-tauri/src/pty.rs:kill_alllatches session admission closed; every reservation checks the latch, and only a successful persistent clear reopens admission.src/components/EmergencyStop.tsx: the UI distinguishes an active barrier from confirmed process stillness and announces failures as alerts.a1f9d7c, fixes in0038025.Evidence
npx vitest run src/components/EmergencyStop.test.tsx --reporter=verbose-> Exit 0 (3 passed).cargo test --manifest-path src-tauri/Cargo.toml --bin projecta kill_all_blocks_new_sessions_until_the_stop_is_cleared-> Exit 0.cargo test --manifest-path src-tauri/Cargo.toml --bin projecta emergency_stop_changes_are_serialized-> Exit 0.bash scripts/ci/gates.sh lane prepushat0038025-> Exit 0: secret self-test, fmt, typecheck, lint, 395 frontend tests, 502 HQ tests, clippy, and 1710 Rust tests green.git push -u origin claude/w5-04b-app-estopran the same prepush hook again -> Exit 0; remote head verified separately withgit ls-remote.gh pr checks 125 --watch-> Exit 0: Linux gates, Windows PR stub, red-first, CodeQL, and GitGuardian green; latest completedmainCI run37081881377was green before the queue request.real_native_provider_exit_before_input_delivery_reconciles_as_exited; the unchanged candidate was requeued once, draft merge queue: checking #125 + #145 together on main (4e7ecc5) #150 passed the full Linux/Windows/red-first conditions, and Mergify merged commit1276b045b55bf84ad802808372da916d0ea06b85.NICHT ABGEDECKT
#[cfg(unix)]tests or Linux clippy arms (KNOWN_ISSUES KI-7). The Linux half requires the WSL2 ext4 lane.Review disposition
Candidate round 1:
1761f9866a74c41928eda5a7fa5f4aad14a62978(Claude author family).C1- Codex - high -src/components/EmergencyStop.tsx:31-60: every activation error set the UI to active and then claimed all agents had ended, including a failed Store write or a 10-second enforcement failure. Accepted: red-first testa1f9d7c; fixed by confirmed/unconfirmed UI state and alert semantics in0038025.C2- Codex - high -src-tauri/src/pty.rs:728-738,src-tauri/src/estop.rs:51-65:kill_alldid not block a later reservation, so a session could appear after the final empty observation. Accepted: red-first testa1f9d7c; PTY admission latch, startup restore, and clear path in0038025.G1- Ollama requestglm-5.2:cloud, response modelglm-5.2- medium -src-tauri/src/main.rs:1151-1166: a concurrent clear could lift the barrier while enforcement was still running. Accepted: serialized backend state changes in0038025, pinned bya1f9d7c.G2- GLM - medium -src-tauri/src/main.rs:3702-3709: alleged missing Tauri capability entries. Rejected: ProjectA app commands are registered throughgenerate_handler!; the capability file scopes plugin permissions, and existing custom commands use the same path without per-command entries.G3- GLM - low -src/components/EmergencyStop.tsx:14-22: one-time state read can become stale after another writer changes the barrier. Rejected for this candidate: there is no second merged app/CLI writer; a future W5-04c integration must add synchronization when it introduces that writer.G4- GLM - low -src/components/EmergencyStop.tsx:62: failure text was not announced to assistive technology. Accepted:role="alert"plus a component assertion ina1f9d7c/0038025.Round 1 result after counterproof: all surviving findings accepted and fixed; no high finding left open.
Candidate round 2, delta only:
1761f98..0038025.D1- Ollama requestglm-5.2:cloud, response modelglm-5.2- low -src-tauri/src/main.rs:4507-4519: the serialization regression is a source-wiring test rather than a behavioral Tokio mutex test. Rejected as a product defect: the test deliberately pins the Tauri command seam; mutual exclusion behavior belongs to Tokio, while the live statement was also inspected in review.D2- GLM - low -src-tauri/src/main.rs:1162-1177: clear waits behind the at-most-10-second enforcement operation. Rejected: waiting is the intentional safety property that prevents reopening admission while the old stop still kills or verifies sessions.Review prompts and raw responses were kept outside the repository and were not committed.
Open points