feat(safety): Emergency Stop for desktop automation (#4255)#4600
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… slice 1) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nyhumansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nyhumansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…inyhumansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nyhumansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nsai#4255) Add HaltState serde type and EmergencyStop OnceLock singleton mirroring the ApprovalGate install pattern. Types: engaged flag, reason, timestamp, source. State: init_global/try_global, engage/clear/snapshot, is_engaged_global. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…inyhumansai#4255) Register all_emergency_registered_controllers and all_emergency_controller_schemas in src/core/all.rs (next to approval). Install EmergencyStop::init_global() in jsonrpc.rs boot sequence next to ApprovalGate::init_global. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ile halted (tinyhumansai#4255) Also introduces a shared crate-visible EMERGENCY_TEST_GUARD in state.rs so all tests touching the process-global EmergencyStop (ops, middleware, screen intelligence) serialize against the same mutex under parallel execution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tinyhumansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…event (tinyhumansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…up (tinyhumansai#4255) - EmergencyStopButton: call emergencyStop() with no argument so the halt reason stays undefined and the banner falls back to safety.haltedBody instead of displaying the literal string "user". Error-path setHalt no longer hardcodes reason: 'user'. onClick renamed to handleClick and wrapped in void to avoid passing an async fn as the event handler. - socketService: add automation_halt handler tests (engaged=true → setHalt, engaged=false → clearHalt, malformed payload → silent drop). - Extracted hydrateEmergencyState helper from AppShellDesktop boot effect for testability; App effect is now a one-liner. Tests cover success, not-engaged, and thrown emergencyStatus paths. - Removed unused safety.stopConfirm key from en.ts and all 13 locale files; pnpm i18n:check exits 0 with no missing or extra keys. - AutomationHaltedBanner.test: replaced brittle getByText('Automation halted') with role+data-analytics-id assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nyhumansai#4255) The bottom-right FAB overlapped the chat composer's Quick/Reasoning controls. Pin it to the top-right (clear of the composer and sidebar; macOS traffic lights sit top-left) and hide it while halted so the AutomationHaltedBanner's Resume is the sole control then. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a process-global emergency-stop control spanning Rust state, RPC, enforcement, websocket events, Redux synchronization, desktop safety UI, localized messages, tests, and design documentation. ChangesEmergency Stop Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Desktop
participant CoreRPC
participant EmergencyStop
participant EventBus
participant Socket
participant Redux
Desktop->>CoreRPC: emergency_stop(reason)
CoreRPC->>EmergencyStop: engage and deny pending activity
EmergencyStop->>EventBus: publish AutomationHalted
EventBus->>Socket: emit automation_halt
Socket->>Redux: dispatch setHalt
Redux->>Desktop: render halted banner
Desktop->>CoreRPC: emergency_resume()
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
feature
![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: a5cbd50c26
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
app/src/services/api/emergencyApi.ts (1)
12-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding debug logging for this new RPC flow.
emergencyStop/emergencyResume/emergencyStatusintroduce a new call path but log nothing themselves (callers likeEmergencyStopButton/AutomationHaltedBanneronly log on the error path). A stable-prefixed debug log on invocation (method + reason) would aid correlating a stop/resume attempt with the resulting core state across the boot/hydrate/socket flows in this feature.As per path instructions, "On new or changed flows, add verbose debug logging with stable prefixes, correlation fields, and no secrets or full PII; incomplete changes are those lacking logging."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/services/api/emergencyApi.ts` around lines 12 - 25, Add stable-prefixed debug logging to the new RPC helpers in emergencyApi so each invocation is visible even on success. In emergencyStop, emergencyResume, and emergencyStatus, log the method being called and the reason when present before calling callCoreRpc, using a consistent prefix that can be correlated with EmergencyStopButton and AutomationHaltedBanner flows. Keep the log verbose enough for tracing across boot/hydrate/socket paths, but avoid secrets or full PII.Source: Path instructions
app/src/components/safety/AutomationHaltedBanner.tsx (1)
22-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider richer logging for this new safety flow.
Only the failure path logs (single line, no correlation id). Per path instructions, new/changed flows should have verbose debug logging with correlation fields.
As per path instructions: "On new or changed flows, add verbose debug logging with stable prefixes, correlation fields, and no secrets or full PII; incomplete changes are those lacking logging."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/safety/AutomationHaltedBanner.tsx` around lines 22 - 30, The onResume emergency flow in AutomationHaltedBanner only logs a terse failure message and lacks the verbose, correlated debug logging required for new/changed safety paths. Update the onResume callback around emergencyResume() to add stable-prefixed debug logs for the attempt, success, and failure paths, and include correlation fields already available in this component/context (without secrets or full PII); keep clear context in the existing error log or replace it with structured logging using the same identifiers.Source: Path instructions
app/src/components/safety/EmergencyStopButton.tsx (2)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing verbose/correlated debug logging for a new safety flow.
The failure path logs only
console.error('[emergency] stop failed', err). As per path instructions, "On new or changed flows, add verbose debug logging with stable prefixes, correlation fields, and no secrets or full PII; incomplete changes are those lacking logging." Consider adding a correlation id (e.g., a per-click token) and logging both the attempt and outcome (success/failure) with that stable prefix, to make it easier to trace emergency-stop attempts in support/debugging scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/safety/EmergencyStopButton.tsx` around lines 26 - 30, The EmergencyStopButton safety flow only logs the failure, so add verbose debug logging around the full emergency-stop attempt with the existing [emergency] prefix. In EmergencyStopButton, generate a per-click correlation token and log both the attempt and the outcome in the try/catch path, including stable fields like the token and source while avoiding secrets or full PII. Update the current console.error in the catch block and add a matching success log so support/debugging can trace a single stop request end to end.Source: Path instructions
22-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo guard against duplicate concurrent stop requests.
handleClickhas no in-flight/disabled state, so rapid clicks before the Reduxhaltedflag flips (and the button unmounts) could fire multipleemergencyStop()calls. The backend is likely idempotent for engage, but consider disabling the button (or ignoring re-entrant clicks) while the request is pending to avoid redundant RPCs and console noise.💡 Proposed guard
+ const [pending, setPending] = useState(false); const handleClick = useCallback(async () => { + if (pending) return; + setPending(true); try { const state = await emergencyStop(); dispatch(setHalt({ reason: state.reason, source: state.source, since: state.engaged_at_ms })); } catch (err) { dispatch(setHalt({ source: 'user' })); console.error('[emergency] stop failed', err); + } finally { + setPending(false); } - }, [dispatch]); + }, [dispatch, pending]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/safety/EmergencyStopButton.tsx` around lines 22 - 31, The EmergencyStopButton handleClick flow allows duplicate concurrent emergencyStop() requests because there is no in-flight guard before Redux setHalt unmounts the button. Update EmergencyStopButton to track a pending state around handleClick and either disable the button or ignore re-entrant clicks until the request settles, while keeping the existing dispatch(setHalt(...)) and error handling behavior intact.app/src/lib/i18n/es.ts (1)
7030-7037: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent register: "Reanude" uses formal
ustedimperative, rest of file uses informaltú.The rest of the file consistently uses informal
túimperative forms (e.g., "Confirma", "Guarda cambios", "Envía").haltedBodyuses "Reanude" (formalustedimperative of reanudar), whileresumecorrectly uses "Reanudar" (infinitive, register-neutral). This is an inconsistency that will stand out to Spanish-speaking users.🐛 Proposed fix
- 'safety.haltedBody': 'Toda la automatización de escritorio está detenida. Reanude cuando esté listo.', + 'safety.haltedBody': 'Toda la automatización de escritorio está detenida. Reanuda cuando estés listo.',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/lib/i18n/es.ts` around lines 7030 - 7037, Update the Spanish copy in the i18n mapping for safety.haltedBody so its wording matches the file’s informal tú register used elsewhere. The issue is the formal “Reanude” wording; change the sentence to use an informal imperative/phrasing consistent with keys like safety.resume, safety.emergencyStop, and the other translations in this file. Keep the meaning the same while aligning tone across the safety strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/safety/EmergencyStopButton.tsx`:
- Around line 8-16: The emergency stop flow in EmergencyStopButton currently
treats a failed `emergency_stop` RPC the same as a confirmed halt by calling
`setHalt({ source: 'user' })` in the catch path. Update the button’s click
handling and the related safety slice state so a requested halt is tracked
separately from a confirmed `automation_halt` from the core, using a distinct
unconfirmed/error state or retry indicator instead of marking the system halted
locally on RPC failure. Keep the Resume banner logic tied only to confirmed halt
state so Stop and Resume are not hidden prematurely.
In `@app/src/services/socketService.ts`:
- Around line 467-491: The automation_halt handler in socketService should fail
closed for ambiguous payloads instead of treating a missing engaged field as
false. Update the this.socket.on('automation_halt', ...) logic to require a real
boolean engaged before calling setHalt or clearHalt, and drop/ignore objects
like {} or {reason: 'x'} the same way invalid shapes are handled. Add or extend
coverage in socketService.events.test.ts for the object-without-engaged case so
the handler does not silently dispatch clearHalt().
In `@docs/superpowers/plans/2026-07-06-emergency-stop.md`:
- Around line 21-22: The locale path references in the i18n guidance are
incorrect and point to a non-existent locales directory. Update the plan text to
reference the actual translation files under app/src/lib/i18n, using the
existing locale module names such as en.ts, ar.ts, de.ts, and the rest of the
listed language files. Keep the rest of the i18n requirement intact, and make
sure the path wording in the emergency-stop doc matches the repo’s real
structure so implementers can find the right symbols and files.
- Around line 726-743: The emergency-stop fast-fail in wrap_tool currently
returns before the approval audit trail is updated, so halted tool calls are not
recorded as aborted. Update the existing halt check in wrap_tool to also write
the audit-trail entry when crate::openhuman::emergency_stop::is_engaged_global()
is true, using the same aborted/blocked path that ApprovalGate::try_global()
handling uses, and keep the fail-closed return behavior unchanged.
In `@docs/superpowers/specs/2026-07-06-emergency-stop-design.md`:
- Line 27: The web bridge reference is pointing to the wrong module path, which
will send implementers to the wrong place. Update the guidance to reference the
actual bridge module in the web provider stack, and keep the surrounding context
tied to the controller registry and approval-surface event bridging so readers
can still locate the right RPC/event flow. Use the existing symbols like
controller registry and ApprovalSurfaceSubscriber to preserve the implementation
trail, but ensure the bridge path points to the correct web event bus module.
In `@src/openhuman/emergency_stop/ops.rs`:
- Around line 56-78: The cascade-deny work in cascade_deny_pending is using
synchronous ApprovalGate::list_pending and ApprovalGate::decide calls, which can
block a Tokio worker thread. Move this best-effort approval draining into a
blocking context such as spawn_blocking or block_in_place, and keep the existing
error handling/logging while preserving the same behavior for pending approvals.
In `@src/openhuman/emergency_stop/state.rs`:
- Around line 53-70: The `EmergencyStopState::engage` and
`EmergencyStopState::clear` updates to `engaged` and `info` are not atomic as a
pair, so concurrent calls can leave `snapshot()` seeing inconsistent state. Fix
this by making the state transition in each method happen under the same
mutex-protected critical section used for `info`, and update `engaged` and
`info` together in `engage`, `clear`, and any related read path in `snapshot()`
to preserve a consistent pair of values.
In `@src/openhuman/screen_intelligence/ops.rs`:
- Around line 325-374: The emergency-stop tests can leak the process-global
engaged state if they panic before reaching the final clear call. In both
input_action_blocked_while_emergency_engaged and
panic_stop_passes_even_while_emergency_engaged, add a Drop-based cleanup guard
immediately after EmergencyStop::init_global() so stop.clear() always runs even
on assertion failure or unwrap panic. Keep the existing test logic intact, but
ensure the guard owns the cleanup for the EmergencyStop state.
In `@src/openhuman/tinyagents/middleware.rs`:
- Around line 3935-3946: Make the emergency-stop test cleanup panic-safe by
ensuring the process-global state is always reset even if an assertion fails.
Update emergency_guard_blocks_when_engaged to use a small Drop-based guard
around EmergencyStop::init_global()/engage so stop.clear() runs during
unwinding, rather than relying on the final explicit clear call after the
assertions.
In `@tests/json_rpc_e2e.rs`:
- Around line 1195-1261: The emergency-stop roundtrip test leaves the
process-global EmergencyStop engaged if any assertion panics before the final
resume call. Update json_rpc_emergency_stop_roundtrip_over_rpc to use an
RAII/scoped cleanup guard (or equivalent defer-style cleanup) around the
openhuman.emergency_stop/openhuman.emergency_resume sequence so resume is
guaranteed to run even on failure; place the cleanup near the test setup and
keep the existing stop/status/resume assertions in the same function.
---
Nitpick comments:
In `@app/src/components/safety/AutomationHaltedBanner.tsx`:
- Around line 22-30: The onResume emergency flow in AutomationHaltedBanner only
logs a terse failure message and lacks the verbose, correlated debug logging
required for new/changed safety paths. Update the onResume callback around
emergencyResume() to add stable-prefixed debug logs for the attempt, success,
and failure paths, and include correlation fields already available in this
component/context (without secrets or full PII); keep clear context in the
existing error log or replace it with structured logging using the same
identifiers.
In `@app/src/components/safety/EmergencyStopButton.tsx`:
- Around line 26-30: The EmergencyStopButton safety flow only logs the failure,
so add verbose debug logging around the full emergency-stop attempt with the
existing [emergency] prefix. In EmergencyStopButton, generate a per-click
correlation token and log both the attempt and the outcome in the try/catch
path, including stable fields like the token and source while avoiding secrets
or full PII. Update the current console.error in the catch block and add a
matching success log so support/debugging can trace a single stop request end to
end.
- Around line 22-31: The EmergencyStopButton handleClick flow allows duplicate
concurrent emergencyStop() requests because there is no in-flight guard before
Redux setHalt unmounts the button. Update EmergencyStopButton to track a pending
state around handleClick and either disable the button or ignore re-entrant
clicks until the request settles, while keeping the existing
dispatch(setHalt(...)) and error handling behavior intact.
In `@app/src/lib/i18n/es.ts`:
- Around line 7030-7037: Update the Spanish copy in the i18n mapping for
safety.haltedBody so its wording matches the file’s informal tú register used
elsewhere. The issue is the formal “Reanude” wording; change the sentence to use
an informal imperative/phrasing consistent with keys like safety.resume,
safety.emergencyStop, and the other translations in this file. Keep the meaning
the same while aligning tone across the safety strings.
In `@app/src/services/api/emergencyApi.ts`:
- Around line 12-25: Add stable-prefixed debug logging to the new RPC helpers in
emergencyApi so each invocation is visible even on success. In emergencyStop,
emergencyResume, and emergencyStatus, log the method being called and the reason
when present before calling callCoreRpc, using a consistent prefix that can be
correlated with EmergencyStopButton and AutomationHaltedBanner flows. Keep the
log verbose enough for tracing across boot/hydrate/socket paths, but avoid
secrets or full PII.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a6e55f0-e5f4-4da5-aafb-49ed61ace5b3
📒 Files selected for processing (47)
app/src/App.tsxapp/src/components/safety/AutomationHaltedBanner.test.tsxapp/src/components/safety/AutomationHaltedBanner.tsxapp/src/components/safety/EmergencyStopButton.test.tsxapp/src/components/safety/EmergencyStopButton.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/services/__tests__/socketService.events.test.tsapp/src/services/api/emergencyApi.test.tsapp/src/services/api/emergencyApi.tsapp/src/services/safety/hydrateEmergencyState.test.tsapp/src/services/safety/hydrateEmergencyState.tsapp/src/services/socketService.tsapp/src/store/index.tsapp/src/store/safetySlice.test.tsapp/src/store/safetySlice.tsapp/src/test/test-utils.tsxdocs/superpowers/plans/2026-07-06-emergency-stop.mddocs/superpowers/specs/2026-07-06-emergency-stop-design.mdsrc/core/all.rssrc/core/event_bus/events.rssrc/core/event_bus/events_tests.rssrc/core/jsonrpc.rssrc/openhuman/channels/providers/web/event_bus.rssrc/openhuman/channels/providers/web/mod.rssrc/openhuman/channels/runtime/startup.rssrc/openhuman/emergency_stop/mod.rssrc/openhuman/emergency_stop/ops.rssrc/openhuman/emergency_stop/schemas.rssrc/openhuman/emergency_stop/state.rssrc/openhuman/emergency_stop/types.rssrc/openhuman/mod.rssrc/openhuman/screen_intelligence/ops.rssrc/openhuman/tinyagents/middleware.rstests/json_rpc_e2e.rs
…ansai#4255) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tomic state, coverage (tinyhumansai#4255) - socketService: drop automation_halt payloads lacking a boolean engaged (a malformed broadcast can no longer silently clear an active halt). - EmergencyStopButton: do not mark halted locally when the stop RPC fails (no false sense of safety); log + leave button for retry. - state.rs: write the engaged flag inside the info mutex so snapshot never sees an inconsistent (flag, info) pair; is_engaged() stays lock-free. - ops.rs: run the blocking cascade-deny via spawn_blocking off the worker. - Extract emergency_halt_denial helper + unit test, and schemas handler/arm test — cover the changed lines the >=80% Rust diff-cover gate flagged. - Panic-safe RAII cleanup guards in the middleware, screen-intel, and e2e tests. - Add [emergency] debug logging (button/banner/api); es.ts informal tú; doc paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
coderabbitai
Bot
added
the
agent
AutomationHaltedBanner.test.tsx queries the retry indicator via
getByRole('status', { name: /could not resume/i }), but the ARIA `status`
role is name-from-author — its accessible name comes from aria-label/
aria-labelledby, not text content — so a `<span role="status">` with only
text has no accessible name and the query fails (Frontend Checks red). Add
aria-label={t('safety.resumeFailed')} so the live region is properly named
for assistive tech and the test resolves it.
|
Maintainer fix pass: CI — Frontend Checks (was RED): CI — Rust Core Coverage (was RED): the sole failure was CodeRabbit review (14:15 CHANGES_REQUESTED) — both actionable items already fixed in current HEAD:
Letting PR CI re-verify (no local matrix run). |
|
@coderabbitai review Both actionable items from the last review are already resolved in current HEAD: the stale- |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== AutomationHaltedBanner.tsx ==="
fd AutomationHaltedBanner.tsx app/src/components/safety --exec cat -n {}Length of output: 4840 🏁 Script executed: #!/bin/bash
rg -n "resumeFailed" app/src/lib/i18n/en.tsLength of output: 285
Kicking off a fresh review pass against current HEAD. ✅ Action performedReview finished.
|
|
@coderabbitai review Status update for this fix pass:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== AutomationHaltedBanner.tsx ==="
fd AutomationHaltedBanner.tsx app/src/components/safety --exec cat -n {}Length of output: 4840
The unrelated ✅ Action performedReview finished.
|
Resolve i18n conflicts in 13 non-English locale files: main added the privacy status/disclosure keys (tinyhumansai#4437/S3) at the same tail position where this PR adds the Emergency Stop safety keys (tinyhumansai#4255). Both key blocks are additive and in disjoint namespaces (safety.* vs privacy.*), so keep BOTH — a union merge. Verified each locale matches en.ts key-for-key (67 safety+privacy keys, no missing/extra).
Review (tinyhumansai#4600): the ops.rs unit tests engaged the process-global kill switch and cleaned up via a manual emergency_resume() at the end. An assertion panic between engage and resume would leak the engaged switch into later tests in the same binary. Add the RAII ClearEmergencyOnDrop guard (already used in schemas.rs) right after the EMERGENCY_TEST_GUARD lock so the switch is cleared on drop, including on panic unwind. Mirrors the guard M3gA-Mind/CodeRabbit asked for on the sibling middleware/screen_intelligence tests.
|
Maintainer fix pass — merged fresh Merge conflicts resolved (13 i18n locale files): Fixed by me:
Verified already addressed in the current head (resolving these stale threads):
Replies (no code change):
Requesting a CodeRabbit re-review below to clear the stale |
|
@coderabbitai review — the audit-trail Major is reconciled per your second option (spec Task 7 now carries the explicit deferred-audit note, so spec ↔ code are consistent); the panic-safe test guard is added; and the halt-broadcast/registration/cron/resume items are already fixed in the current head (details above). |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== spec Task 7 audit-trail note ==="
fd "2026-07-06-emergency-stop-design.md" docs --exec grep -n -i "audit" -A5 -B5 {}
echo "=== plan line ~745 ==="
fd "2026-07-06-emergency-stop.md" docs --exec sed -n '720,760p' {}
echo "=== ops.rs guard usage ==="
fd ops.rs src/openhuman/emergency_stop --exec cat -n {}
echo "=== socketService.ts halt handler ==="
fd socketService.ts app/src/services --exec sed -n '440,510p' {}
echo "=== startup.rs registration ==="
fd startup.rs src/openhuman/channels/runtime --exec cat -n {}
echo "=== jsonrpc.rs registration ==="
rg -n "register_automation_halt_subscriber" src/core/jsonrpc.rs
echo "=== cron scheduler halt check ==="
rg -n "is_engaged_global" src/openhuman/cron/scheduler.rs
echo "=== AutomationHaltedBanner.tsx resume onClick ==="
fd AutomationHaltedBanner.tsx app/src/components/safety --exec cat -n {}Length of output: 87478
Everything checks out. Kicking off a fresh review pass against current HEAD. ✅ Action performedReview finished.
|
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc56bf59a1
ℹ️ 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".
# Conflicts: # app/src/lib/i18n/en.ts
The locale coverage test (app/src/lib/i18n/__tests__/coverage.test.ts) forbids U+2014 in every locale value, including en. The emergency-stop safety.stopFailed / safety.resumeFailed strings used em dashes across 12 locale files; rephrase each into natural two-sentence copy with locale-appropriate punctuation. No key or meaning changes.
|
Maintainer sync + conflict resolution (this PR had gone stale for ~10 days; Merge: merged current
Semantic re-check of the auto-merges (a clean textual merge can still be semantically broken): I verified the three emergency-stop enforcement chokepoints are still correctly wired after
Genuine failure fixed (NOT inherited from the old red Inherited #4948 breakage: gone — the sync pulled in Pushed 2 commits to the fork head: the merge commit + |
…above webviews cron: the emergency-stop guard ran once before the retry loop, so a user who engaged Emergency Stop during a backoff sleep could still have the next retry execute a Shell/Flow/Agent job. Re-check is_engaged_global() at the top of each retry attempt and return the same fail-closed denial (Codex P1, tinyhumansai#4255). UI: AutomationHaltedBanner rendered in normal flow, so the provider WebviewHost overlay (absolute inset-0 z-30) covered the halt banner and its Resume button whenever a provider account was active. Make the banner sticky top-0 z-40 so it stays visible and reachable above the webview, below the settings modal (z-50), per the app stacking convention (Codex P2).
|
Maintainer re-review (PR was untouched for ~10 days; reviewing it current, and clearing the outstanding bot items). Re-review of the feature — looks solidThe emergency-stop design is well-built and genuinely fail-closed:
Fixes I pushed
Bot items already addressed by the author (verified against current code)
RecommendationMerge once CI is green, pending a quick visual check of the halt-banner z-index on desktop (item 2). No open correctness concerns from me; the safety enforcement is fail-closed at every path I traced. |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Approving. CI is fully green (18 lanes: 14 pass, 4 skipped by changed-area detection, zero pending, zero failing) — including Frontend Checks, PR CI Gate, Rust Quality (fmt/clippy), Rust Feature-Gate Smoke (gates off), and Rust Core Coverage. mergeable=MERGEABLE; no standing CHANGES_REQUESTED (CodeRabbit's was dismissed).
This PR sat at review-complete for ~10 days and had drifted. Bringing it current surfaced real issues, all now fixed:
Conflict with main — resolved (understood both sides, no blanket --ours/--theirs).
Frontend Checks was a genuine failure, not inherited. Worth stating plainly, since main was separately red today (#4948/#4949): this PR's failure was its own. safety.stopFailed / safety.resumeFailed used em dashes (U+2014) across 12 locale files, which the i18n rule forbids in every locale including en. Each was rephrased into natural two-sentence copy with locale-appropriate punctuation (period; full-width comma for zh-CN), preserving meaning and keys. de/es already used en-dash/colon and were left alone. Zero U+2014 remain. PR CI Gate was only a rollup of that failure and cleared with it.
Two Codex findings fixed — one is a real safety defect:
- [P1] Emergency Stop could be bypassed during retry backoff. The kill-switch guard ran once before the retry loop, so engaging the stop during a backoff sleep let the next attempt execute a Shell/Flow/Agent job anyway. Now re-checked before every attempt, fail-closed. In a feature whose entire purpose is to halt execution, this mattered.
- [P2] Halt banner was rendering behind provider webviews — it sat in normal flow under the
absolute inset-0 z-30WebviewHost. Nowsticky top-0 z-40.
Bot items: every CodeRabbit and Codex item is fixed or verified already-addressed by the author (stale-resumeFailed useEffect reset, empty-string reason fallback, e2e panic cleanup via ResumeOnDrop RAII, cron dispatch guard, preserve-halt-on-failed-resume). None left unaddressed.
Assessment: the state machine is fail-closed, atomic, and headless-safe; enforcement covers the middleware plus both non-middleware job paths and desktop input, with panic_stop correctly exempt. No open correctness concerns.
One caveat the maintainer should know before merging: the P2 banner z-index fix is not visually verified — it was made and reasoned about headless, and no one has confirmed the banner actually renders above the CEF webview in the running desktop app. The logic is sound and the risk is low, but this is a safety-critical affordance: if the halt banner is invisible, the user cannot tell the system is stopped. Recommend a quick desktop look at that banner before or shortly after merge.
Note for the merger: the head (e340a8be8) carries maintainer commits pushed unsigned (51da322, e5bcf46, e340a8b) — re-sign before merging if signature enforcement applies.
|
@coderabbitai review Requesting a re-review of the current head ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR metadata ==="
gh pr view 4600 --repo tinyhumansai/openhuman --json headRefOid,baseRefName,headRefName,mergeable,state,statusCheckRollup \
--jq '{headRefOid, baseRefName, headRefName, mergeable, state, checks: [.statusCheckRollup[] | {name, conclusion, status}]}'
echo "=== Changed files (merge-base diff) ==="
git diff --name-status origin/main...HEAD
echo "=== Diff stat ==="
git diff --stat origin/main...HEAD
echo "=== Targeted AST outlines ==="
ast-grep outline src/openhuman/cron/scheduler.rs --items all --type function --match 'execute_job_with_retry|is_engaged_global' || true
ast-grep outline app/src/components/safety/AutomationHaltedBanner.tsx --items all || true
ast-grep outline app/src/services/socketService.ts --items all --match 'connect|socket' || true
ast-grep outline src/openhuman/emergency_stop --items all || trueLength of output: 9912 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current focused implementations ==="
sed -n '1,240p' src/openhuman/emergency_stop/state.rs
sed -n '1,260p' src/openhuman/emergency_stop/ops.rs
sed -n '450,610p' src/openhuman/cron/scheduler.rs
sed -n '1,150p' app/src/components/safety/AutomationHaltedBanner.tsx
sed -n '430,530p' app/src/services/socketService.ts
echo "=== Controller/event registration and bridge ==="
sed -n '1,220p' src/openhuman/emergency_stop/schemas.rs
rg -n -C 4 'emergency_stop|register_automation_halt_subscriber' src/core/all.rs src/core/jsonrpc.rs src/openhuman/channels/runtime/startup.rs src/openhuman/channels/providers/web/mod.rs
ast-grep outline src/openhuman/channels/providers/web/event_bus.rs --items all --match 'AutomationHalt|register_automation_halt_subscriber'
rg -n -C 5 'AutomationHaltSubscriber|automation_halt|register_automation_halt_subscriber' src/openhuman/channels/providers/web/event_bus.rs
echo "=== Enforcement guards ==="
rg -n -C 10 'emergency_halt_denial|is_engaged_global|panic_stop' src/openhuman/tinyagents/middleware.rs src/openhuman/screen_intelligence/ops.rs
echo "=== Frontend API, state, boot integration ==="
cat -n app/src/services/api/emergencyApi.ts
cat -n app/src/store/safetySlice.ts
rg -n -C 6 'hydrateEmergencyState|AutomationHaltedBanner|EmergencyStopButton' app/src/App.tsx
echo "=== Recent-change patches ==="
git diff origin/main...HEAD -- src/openhuman/cron/scheduler.rs app/src/components/safety/AutomationHaltedBanner.tsxLength of output: 74842 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Cron command execution and retry/backoff boundaries ==="
rg -n -C 12 'async fn run_job_command|Command::new|for attempt in|sleep\(|backoff_ms|execute_job_with_retry' src/openhuman/cron/scheduler.rs
echo "=== Emergency-stop cron regression tests ==="
rg -n -C 12 'emergency|halted|retry' src/openhuman/cron/scheduler_tests.rs
echo "=== UI/API/state regression tests ==="
sed -n '1,180p' app/src/components/safety/AutomationHaltedBanner.test.tsx
sed -n '1,140p' app/src/components/safety/EmergencyStopButton.test.tsx
sed -n '1,150p' app/src/services/__tests__/socketService.events.test.ts
sed -n '1,120p' app/src/services/safety/hydrateEmergencyState.test.ts
sed -n '1,120p' app/src/services/api/emergencyApi.test.ts
echo "=== Runtime installation branch and complete core diff context ==="
sed -n '2310,2395p' src/core/jsonrpc.rs
git diff --check origin/main...HEAD
git diff --unified=12 origin/main...HEAD -- src/core/jsonrpc.rs src/openhuman/screen_intelligence/ops.rs src/openhuman/tinyagents/middleware.rs tests/json_rpc_e2e.rsLength of output: 79535
One remaining correctness concern:
|
6 participants
Summary
accessibility_input_actionrefuses desktop input (panic_stopexempt).emergency_stopdomain: a process-globalOnceLockswitch, RPCsopenhuman.emergency_{stop,resume,status}, boot install; engaging also stops the accessibility session and cascade-denies pending approvals (best-effort).AutomationHalted/AutomationResumedevents bridged to anautomation_haltsocket broadcast (client_id: "system"); React UI: ReduxsafetySlice,emergencyApi, a top-right Emergency Stop button (hidden while halted) + an Automation halted banner with Resume, boot-time hydration, and i18nsafety.*across all locales.Problem
Desktop control can click, type, delete, send, and purchase on the user's behalf. Non-technical users need one always-available way to instantly stop everything and be sure nothing else runs until they say so. This implements the emergency-stop acceptance criterion of #4255: "Emergency stop can cancel pending actions and prevents further queued actions from running until the user resumes."
Solution
EmergencyStopmirrorsApprovalGate's install idiom:try_global() → None(CLI/headless, not installed) never blocks — fail-open; engaged → fail-closed.emergency_stop(reason, source)sets theAtomicBoolbefore any side effects; the a11y-session stop and cascade-deny are best-effort and can never un-set the flag or fail the RPC.SeqCstordering with a fail-closed torn-read fallback (reviewed).is_engaged_global(). The approval middleware is installed unconditionally, so the tool refusal holds even withOPENHUMAN_APPROVAL_GATE=0.emergency_statushydration, and theautomation_haltsocket broadcast. The Stop button hides while halted so the banner's Resume is the sole control (no dual Stop+Resume).Design decisions / tradeoffs: in-memory halt (resets on restart) for this slice; no confirmation dialog before stopping (a kill switch shouldn't be gated); backend (
backend-alphahuman) intentionally unchanged — this is a local Tauri-core control.Submission Checklist
emergency_stop_roundtrip_over_rpc); Vitest for the slice, API client,automation_haltsocket handler, boot hydration, button (incl. hide-when-halted), and banner. Failure paths covered (RPC throws → local halt; malformed socket payload dropped; fail-open when uninstalled).handle). The CIdiff-covergate is authoritative; final number verified there.feature-ids.jsonmatrix-sync mapping (epic Enforce unit tests, feature tests, and documentation for core foundation #773).## Related— new feature; see Related.Closes #NNN— N/A: intentionally non-closing — first slice of epic Desktop automation safety previews, confirmations, history, and emergency stop #4255, referenced as "Part of Desktop automation safety previews, confirmations, history, and emergency stop #4255" so the epic stays open (see Related).Impact
AtomicBoolload at the two chokepoints.Related
src/openhuman/about_app/capability-catalog entry for the new controlwrap_tooldeny-branch integration test (current test togglesis_engaged_global(); deny path verified by inspection + the RPC/E2E proof)docs/TEST-COVERAGE-MATRIX.mdrow +docs/RELEASE-MANUAL-SMOKE.mdstepsAI Authored PR Metadata
Linear Issue
Commit & Branch
feat/desktop-safety-4255a5cbd50c2Validation Run
pnpm --filter openhuman-app format:check— prettier applied to all changed frontend files (style(emergency)commits)pnpm typecheck— changed files typecheck clean; repo-wide has pre-existing unrelated failures (@xyflow/react,rehype-highlight) not from this PRpnpm test src/store/safetySlice src/services/api/emergencyApi src/components/safety(all green);cargo test -p openhuman emergency(15 pass, parallel)cargo fmt --check+cargo checkcleanapp/src-taurichangesValidation Blocked
command:full desktop GUI enforcement of the tool-middleware chokepointerror:staging account's AI provider key is unset, so the agent can't invoke a tool in-chatimpact:low — enforcement proven at the RPC layer (screen_intelligence_input_actionreturnsblocked, reason:"emergency_stop"only while halted; clears on resume) and by unit testsBehavior Changes
Parity Contract
panic_stopbypasses the guard.Duplicate / Superseded PR Handling
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Closes : #4255