From 22ff4957c981e5d5f3df20f915ebb2978b460362 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Thu, 23 Apr 2026 00:36:19 +0900 Subject: [PATCH 1/2] feat(safety): projection-exempt lint for gateway event sources (#2840) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(safety): projection-exempt lint for gateway event sources Phase 1 of the gateway state-convergence epic (#2792): add check #9 to `scripts/pre-commit-safety.sh` that flags newly-added `sse.broadcast(` / `sse.broadcast_for_user(` calls without a `// projection-exempt: ` annotation on the same line. The invariant is documented in the new `.claude/rules/gateway-events.md`: - Every `AppEvent` must project from a typed source log (engine `EventKind`, sandbox `JobEvent`, or a channel-lifecycle log). - A short transport-only allowlist (`Heartbeat`, `StreamChunk`) covers the ephemeral variants with no state backing them. - Direct emits are the root cause of the state-drift class — UI stream and replayable source end up with different stories. Four recent incidents (#2654, #2534, #2731, #2079) share this shape. The lint is diff-based, so pre-existing unannotated call sites aren't broken. Baseline annotation of the ~20 existing emit sites is the next PR under Phase 1 — this one establishes the gate. Suppressions require a named category (`bridge dispatcher`, `channel-lifecycle`, `sandbox JobEvent`, `transport-only, heartbeat`, or `migrate in #NNNN`). An unnamed `legacy` reason is rejected by review, not by the lint itself. Tested locally: - Fires on unannotated `sse.broadcast(...)` in a new file. - Suppressed by `// projection-exempt: transport-only, heartbeat`. - Does not match `Channel::broadcast` (different trait). - Does not match calls inside `#[cfg(test)] mod tests` blocks (via the shared `strip_test_mod_lines` filter). Refs: #2792, #2654 * refactor(safety): address review feedback on projection-exempt check Four review comments from Copilot and Gemini on #2840: 1. **Match rustfmt's method-chain wrapping.** The original regex only caught same-line `sse.broadcast(...)`. Long calls like `state\n .sse\n .broadcast_for_user(...)` — produced by rustfmt and already in-tree at `src/channels/web/features/extensions/mod.rs:645` — would bypass the check. New matcher adds a dangling-method alternation that catches `.broadcast_for_user(` at line start. Only the `_for_user` suffix (SseManager-unique) is matched in dangling form; bare `.broadcast(` can be `Channel::broadcast` trait, which is intentionally out of scope. 2. **Enforce the documented annotation format.** The check previously accepted any `// projection-exempt:` comment, including bare `// projection-exempt: legacy` that the rule doc explicitly forbids. Negative filter now requires `, ` — presence of a comma separating the category from the detail. 3. **Point at the real path in the warning.** Replace `bridge::thread_event_to_app_events` with `thread_event_to_app_events` in `src/bridge/router.rs` — the actual file location. 4. **Update suppression hint** to show the `, ` format rather than the generic ``. Verified against a 6-case fixture (same-line fire + suppress, dangling-chain fire + suppress, unnamed-category fire, `Channel::broadcast` silent). Refs: #2792, #2840 review * fix(safety): match header exclusion against grep -n prefixed output After `grep -nE '^\+'`, every line is prefixed with `N:`, so the `^\+\+\+` anchor for filtering diff header lines (`+++ b/file.rs`) never fires. The positive patterns already exclude header lines by shape, so today this is harmless — but the dead branch masks future defense-in-depth failures if the template is reused with a less specific positive match. Replace `^\+\+\+` with `:\+\+\+ ` in DISPATCH, CREDNAME, and PROJECTION checks so the exclusion works against the `grep -n` output shape. Co-Authored-By: Claude Opus 4.7 (1M context) * test(safety): regression for grep-n-prefixed header exclusion Covers PROJECTION / DISPATCH / CREDNAME pipelines: - diff header lines (`+++ b/path`) are filtered after `grep -n` - real broadcast/state/CredentialName lines are still flagged - `// projection-exempt: , ` exempts - bare `// projection-exempt: legacy` (no comma) is not exempt Locks in that `:\+\+\+ ` (matches the `grep -n` prefixed shape) behaves as intended, where the prior `^\+\+\+` anchor silently never fired. Co-Authored-By: Claude Opus 4.7 (1M context) * chore(deny): ignore RUSTSEC-2026-0104 (rustls-webpki CRL panic) Same transitive pin as 0049/0098/0099 — rustls-webpki 0.102.8 is held by libsql 0.6.0 → rustls 0.22 → hyper-rustls 0.25. The advisory explicitly notes that applications not parsing CRLs are unaffected; we do not parse CRLs. [skip-regression-check] — deny.toml-only config change. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(safety): portable grep boundary + broadened broadcast_for_user match Two PROJECTION bypass paths flagged in review: 1. `\b` is a GNU-grep extension (works in grep 3.x, not portable to BSD grep on macOS dev envs) — replace with `(^|[^[:alnum:]_])sse\.` so the check fires uniformly across `grep -E` implementations. 2. `broadcast_for_user(...)` on a non-`sse` receiver (e.g. `manager.broadcast_for_user(...)`) previously slipped through. The method is defined only on `SseManager` (`src/channels/web/platform/sse.rs:144`), so matching `\.broadcast_for_user\(` on any receiver is safe and makes the enforcement match the documented rule. Regression tests extended: chained-receiver, non-`sse` receiver, bare `sse.broadcast(`, and a portable-boundary negative case (identifier ending in `sse`). Co-Authored-By: Claude Opus 4.7 (1M context) * docs(gateway-events): align matcher description with broadened check Update the enforcement section to describe the two current PROJECTION matcher shapes after the review follow-up in the preceding commit: 1. Any-receiver `.broadcast_for_user(...)` — catches the non-`sse` receiver bypass and rustfmt wraps alike. 2. `sse.broadcast(...)` with a portable boundary (`(^|[^[:alnum:]_])`), which is needed because `grep -E`'s `\b` is a GNU extension and not available on BSD grep. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(safety): tighten CREDNAME + projection-exempt lints, sync header Three follow-ups from the review: 1. CREDNAME portability — `\bCredentialName\b` used GNU-grep `\b`, which BSD grep does not recognise. Replace with the same `(^|[^[:alnum:]_])…([^[:alnum:]_]|$)` boundary used for PROJECTION and matches cleanly across GNU and BSD `grep -E`. 2. Empty-detail suppression bypass — `// projection-exempt: [^,]+,` accepted `// projection-exempt: foo,` (empty detail) as exempt even though `.claude/rules/gateway-events.md` requires a non-empty detail. Tighten to `[^,]+,[[:space:]]*[^[:space:]]` so a comma without a trailing token still fires the check. 3. Header suppression hint (`#24`) said `// projection-exempt: ` — update to `, ` to match what the check actually accepts so contributors don't copy an unsupported format. Regression tests extended: `PROJECTION: empty detail after comma still flagged`, `PROJECTION: comma + whitespace-only detail still flagged`, `CREDNAME: CredentialNameExt (different type) is not flagged`. All 16 cases pass. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .claude/rules/gateway-events.md | 114 ++++++++++++++++++++ deny.toml | 4 + scripts/pre-commit-safety.sh | 54 +++++++++- scripts/test-pre-commit-safety.sh | 170 ++++++++++++++++++++++++++++++ 4 files changed, 339 insertions(+), 3 deletions(-) create mode 100644 .claude/rules/gateway-events.md create mode 100755 scripts/test-pre-commit-safety.sh diff --git a/.claude/rules/gateway-events.md b/.claude/rules/gateway-events.md new file mode 100644 index 00000000000..6b181f46ef4 --- /dev/null +++ b/.claude/rules/gateway-events.md @@ -0,0 +1,114 @@ +--- +paths: + - "src/**" + - "crates/**" +--- +# Gateway Events — Single Source of Truth + +Every `AppEvent` reaching the SSE/WS stream must come from a **typed +source log**, or be on a small **transport-only allowlist**. Direct +`sse.broadcast(...)` / `sse.broadcast_for_user(...)` calls from tools, +handlers, or extension managers are the root cause of the UI state +drift class tracked by #2792 — the stream and the replayable source +end up telling different stories. + +This is the Phase 1 rule of the gateway state-convergence epic. + +## Why + +When `AppEvent` has producers outside the projection layer, those +producers become a second source of truth. On SSE reconnect, replay +from the engine event log can't reconstruct them (they were never +logged). On tab focus, reconciliation against a GET endpoint can't +confirm them (no persisted state backs them). Four recent bugs +(#2654, #2534, #2731, #2079) share this shape: broadcast emitted, +backend state unchanged, UI diverges. + +## Source logs + +Every `AppEvent` projects from exactly one of: + +| Source log | Projection function | Typical variants | +|---|---|---| +| `ironclaw_engine::EventKind` | `src/bridge/router.rs::thread_event_to_app_events` | Turn progression, tool execution, gates, leases, child threads, skills | +| Sandbox `JobEvent` | `src/worker/job.rs` (currently inline; extract under #2792 Phase 1 PR 3) | `JobStarted`, `JobMessage`, `JobToolUse`, `JobToolResult`, `JobStatus`, `JobResult` | +| Channel-lifecycle logs | `src/channels/web/features/oauth/`, `features/pairing/`, `features/extensions/`, `extensions/manager.rs` | `OnboardingState`, `ExtensionStatus` | + +## Transport-only allowlist + +A small number of `AppEvent` variants don't project from anything +because they have no state backing them. These are documented +exceptions, not a loophole for new state: + +- `Heartbeat` — SSE keepalive, no payload, no state +- `StreamChunk` — LLM token streaming, pre-step-completion by design; formalizing into `EventKind` would pollute the durable log with token-level noise + +New `AppEvent` variants that claim "transport-only" status require +review sign-off and an entry in this table. + +## The rule + +**No call to `SseManager::broadcast` / `SseManager::broadcast_for_user` +is allowed outside:** + +1. The projection dispatcher loop that consumes one of the three source + logs above, **or** +2. A line annotated with `// projection-exempt: , `. + +## Annotation format + +```rust +state.sse.broadcast_for_user(user_id, event); // projection-exempt: channel-lifecycle, extension activation +``` + +The `` must name either: + +- A source log — `bridge dispatcher`, `sandbox JobEvent`, `channel-lifecycle` — plus a short detail. +- A transport-only allowlist entry — `transport-only, heartbeat` or `transport-only, stream_chunk`. +- A scheduled migration — `migrate in #NNNN` where the issue tracks moving the emit into a source log. + +An unnamed category (`// projection-exempt: legacy`) is not sufficient. +Either the site is legitimately exempt and the category explains why, +or it's a violation and should be migrated. + +## Enforcement + +Check #9 in `scripts/pre-commit-safety.sh` (label: `PROJECTION`) flags +added lines that call `SseManager::broadcast` or +`SseManager::broadcast_for_user` without a `// projection-exempt: +, ` annotation on the same line. The comma is +required — the check rejects bare `// projection-exempt: legacy`. Lines +in `#[cfg(test)] mod tests` blocks and under `tests/` are skipped via +the shared `strip_test_mod_lines` filter. + +The matcher covers two call-site shapes: + +1. Any-receiver `.broadcast_for_user(...)` — the method is defined + only on `SseManager` (`src/channels/web/platform/sse.rs`), so + matching the method name alone catches same-line receivers + (`state.sse.broadcast_for_user(...)`), rustfmt wraps + (`state\n .sse\n .broadcast_for_user(...)`), and any other + receiver name (`manager.broadcast_for_user(...)`) without + false-positive risk. +2. `sse.broadcast(...)` — the single-name `broadcast` + is shared with the `Channel` trait, so this arm is deliberately + narrower and only fires when the receiver is literally named + `sse`. The boundary uses `(^|[^[:alnum:]_])` rather than `\b` so + the check is portable across GNU and BSD `grep -E`. + +## Not covered by this rule + +- **`Channel::broadcast` on the `Channel` trait.** Different method, + different trait, different semantics (delivery to a specific channel + endpoint like Telegram, not to SSE subscribers). The `Channel` trait + has its own invariants in `src/channels/`. +- **Non-SSE `broadcast` methods.** If you're broadcasting on a + `tokio::sync::broadcast::Sender` directly, you're below the + `AppEvent` abstraction; the rule doesn't apply. + +## References + +- Epic: #2792 — Gateway state convergence +- Coverage: #2654 — Engine→AppEvent bridge gaps +- Incidents: #2079 (SSE ordering), #2534 (stale approval), #2731 (Telegram thread split) +- Rule cluster: `.claude/rules/types.md` for wire-stable enums; `.claude/rules/tools.md` for the parallel "everything goes through tools" rule this mirrors diff --git a/deny.toml b/deny.toml index 70ef8cf0ce4..b5d2afabaec 100644 --- a/deny.toml +++ b/deny.toml @@ -9,9 +9,13 @@ ignore = [ "RUSTSEC-2025-0111", # rustls-webpki advisories — 0.102.8 remains pinned by a libsql 0.6.0 transitive dep # (via rustls 0.22 → hyper-rustls 0.25); keep ignored until that pin is gone. + # RUSTSEC-2026-0104: panic on empty `onlySomeReasons` BIT STRING during CRL parsing. + # We do not use CRLs, and the advisory explicitly notes apps that don't parse CRLs + # are unaffected. Same transitive pin as 0049/0098/0099 — tracked with them. "RUSTSEC-2026-0049", "RUSTSEC-2026-0098", "RUSTSEC-2026-0099", + "RUSTSEC-2026-0104", # rand unsoundness with custom logger calling rand::rng() during reseed — we don't use this pattern; # revisit/remove by 2026-06-30, or when transitive deps (tower, nanoid, phf_generator) release rand ≥0.9.3 compat "RUSTSEC-2026-0097", diff --git a/scripts/pre-commit-safety.sh b/scripts/pre-commit-safety.sh index 4432d98d4ee..e704dba087b 100755 --- a/scripts/pre-commit-safety.sh +++ b/scripts/pre-commit-safety.sh @@ -13,6 +13,7 @@ # 6. .unwrap(), .expect(), assert!() in production code (panics) # 7. Gateway/CLI handlers bypassing ToolDispatcher (must go through tools) # 8. CredentialName referenced in web-layer code (wrong identity at boundary) +# 9. SSE broadcast emitted outside the engine→AppEvent projection bridge # # Also runs check-i18n-parity.sh when crates/ironclaw_gateway/static/i18n/*.js # files are staged, to ensure every language pack has the same key set. @@ -20,6 +21,7 @@ # Suppress individual lines with an inline "// safety: " comment. # For check #7, use "// dispatch-exempt: " instead. # For check #8, use "// web-identity-exempt: " instead. +# For check #9, use "// projection-exempt: , " instead. set -euo pipefail @@ -342,7 +344,7 @@ fi if [ -n "$DISPATCH_DIFF" ]; then DISPATCH_HITS=$(echo "$DISPATCH_DIFF" | grep -nE '^\+' \ | grep -E 'state\.(store|workspace|workspace_pool|extension_manager|skill_registry|session_manager)\.' \ - | grep -vE '// dispatch-exempt:|// safety:|^\+\+\+' \ + | grep -vE '// dispatch-exempt:|// safety:|:\+\+\+ ' \ | head -5 || true) if [ -n "$DISPATCH_HITS" ]; then warn "DISPATCH" "Handler directly touches state.{store,workspace,extension_manager,skill_registry,session_manager}. Route through ToolDispatcher::dispatch() instead. See .claude/rules/tools.md." @@ -370,9 +372,12 @@ if [ -n "$WEB_IDENTITY_DIFF" ]; then # Strip lines inside `#[cfg(test)] mod tests` blocks using the same # precomputed boundaries used for other prod-only checks. WEB_IDENTITY_PROD=$(printf '%s\n' "$WEB_IDENTITY_DIFF" | strip_test_mod_lines) + # `(^|[^[:alnum:]_])CredentialName([^[:alnum:]_]|$)` is a portable + # word boundary; `grep -E`'s `\b` is a GNU extension and is not + # recognised by BSD grep. WEB_IDENTITY_HITS=$(echo "$WEB_IDENTITY_PROD" | grep -nE '^\+' \ - | grep -E '\bCredentialName\b' \ - | grep -vE '// web-identity-exempt:|// safety:|^\+\+\+' \ + | grep -E '(^|[^[:alnum:]_])CredentialName([^[:alnum:]_]|$)' \ + | grep -vE '// web-identity-exempt:|// safety:|:\+\+\+ ' \ | head -5 || true) if [ -n "$WEB_IDENTITY_HITS" ]; then warn "CREDNAME" "\`CredentialName\` referenced in src/channels/web/** — web code takes \`ExtensionName\`; credential identity stays backend-side. Push the mapping into bridge::auth_manager or annotate with '// web-identity-exempt: '." @@ -380,11 +385,54 @@ if [ -n "$WEB_IDENTITY_DIFF" ]; then fi fi +# 9. SSE `AppEvent` broadcast outside the engine→AppEvent projection bridge. +# Every `AppEvent` that hits the SSE/WS stream should project from a typed +# source log (`ironclaw_engine::EventKind`, `JobEvent`, channel-lifecycle) +# or belong to the documented transport-only allowlist. Direct +# `sse.broadcast(...)` / `sse.broadcast_for_user(...)` calls from tools, +# handlers, or extension managers drift the UI stream out of alignment +# with the replayable source, which is the root cause of the state +# desync class tracked by #2792. See `.claude/rules/gateway-events.md`. +# +# Annotation format: `// projection-exempt: , ` — the +# category names the source log (`bridge dispatcher`, `channel-lifecycle`, +# `sandbox JobEvent`, `legacy v1 auth`) or the transport-only allowlist +# (`transport-only, heartbeat`). The comma is required — an unnamed +# `// projection-exempt: legacy` does not suppress the check. +# +# Two match patterns: +# 1. `*.broadcast_for_user(...)` on any receiver — `broadcast_for_user` +# is unique to `SseManager` (see `src/channels/web/platform/sse.rs`), +# so matching the method name alone catches both same-line +# receivers (`state.sse.broadcast_for_user(...)`) and rustfmt +# wraps (`state\n .sse\n .broadcast_for_user(...)`) without +# risk of false positives from other types. +# 2. `sse.broadcast(...)` — the single-name `.broadcast(` +# on its own line can be the `Channel` trait method, so this arm +# is deliberately narrower and anchors on an `sse` receiver. +# `(^|[^[:alnum:]_])sse\.` is a portable boundary; `grep -E`'s +# `\b` is a GNU extension and is not recognised by BSD grep, so +# we avoid it here. +# Suppression regex requires a non-empty detail after the comma: +# `[^,]+,[[:space:]]*[^[:space:]]` — `// projection-exempt: foo,` +# (empty detail) does NOT exempt; `// projection-exempt: foo, bar` +# does. This matches the documented contract in +# `.claude/rules/gateway-events.md`. +PROJECTION_HITS=$(echo "$DIFF_OUTPUT_NO_TESTS" | grep -nE '^\+' \ + | grep -E '(\.broadcast_for_user|(^|[^[:alnum:]_])sse\.broadcast)[[:space:]]*\(' \ + | grep -vE '// projection-exempt: [^,]+,[[:space:]]*[^[:space:]]|// safety:|:\+\+\+ ' \ + | head -5 || true) +if [ -n "$PROJECTION_HITS" ]; then + warn "PROJECTION" "Direct SSE broadcast outside the engine→AppEvent bridge. Route through \`thread_event_to_app_events\` in \`src/bridge/router.rs\` (project from a typed source log) or annotate with '// projection-exempt: , '. See .claude/rules/gateway-events.md." + echo "$PROJECTION_HITS" | sed 's/^/ /' +fi + if [ "$WARNINGS" -gt 0 ]; then echo "" echo "Found $WARNINGS potential issue(s). Fix them or add '// safety: ' to suppress." echo "(For DISPATCH warnings, use '// dispatch-exempt: ' instead.)" echo "(For CREDNAME warnings, use '// web-identity-exempt: ' instead.)" + echo "(For PROJECTION warnings, use '// projection-exempt: , ' instead.)" echo "" exit 1 fi diff --git a/scripts/test-pre-commit-safety.sh b/scripts/test-pre-commit-safety.sh new file mode 100755 index 00000000000..f26459fe7ff --- /dev/null +++ b/scripts/test-pre-commit-safety.sh @@ -0,0 +1,170 @@ +#!/usr/bin/env bash +# Regression tests for the grep pipelines in `pre-commit-safety.sh`. +# +# The PROJECTION / DISPATCH / CREDNAME checks all pipe through +# `grep -nE '^\+' | grep -E | grep -vE `. +# A previous version of the exclusion regex used `^\+\+\+` to filter +# diff header lines (`+++ b/file.rs`), which silently never fired — +# `grep -n` prepends a `N:` line-number prefix, so `^` no longer +# anchors against the `+++` bytes. This test locks in the corrected +# `:\+\+\+ ` shape. + +set -euo pipefail +cd "$(dirname "$0")/.." + +PASS=0 +FAIL=0 + +assert_filtered() { + local label="$1" input="$2" positive="$3" exclusions="$4" + # Emulate the production pipeline: `grep -n '^+'` adds the line-number + # prefix, then positive/negative filters run against that shape. + local result + if result=$(printf '%s\n' "$input" \ + | grep -nE '^\+' \ + | grep -E "$positive" \ + | grep -vE "$exclusions" \ + | head -5 || true); then :; fi + if [ -z "${result:-}" ]; then + echo "OK: $label (correctly filtered)" + PASS=$((PASS + 1)) + else + echo "FAIL: $label — line leaked past exclusions:" + echo "$result" | sed 's/^/ /' + FAIL=$((FAIL + 1)) + fi +} + +assert_flagged() { + local label="$1" input="$2" positive="$3" exclusions="$4" + local result + if result=$(printf '%s\n' "$input" \ + | grep -nE '^\+' \ + | grep -E "$positive" \ + | grep -vE "$exclusions" \ + | head -5 || true); then :; fi + if [ -n "${result:-}" ]; then + echo "OK: $label (correctly flagged)" + PASS=$((PASS + 1)) + else + echo "FAIL: $label — line not flagged by positive pattern" + FAIL=$((FAIL + 1)) + fi +} + +# ── PROJECTION ──────────────────────────────────────────────── +# Positive: any `.broadcast_for_user(` (SseManager-unique method) or +# `sse.broadcast(` with a portable word boundary. +# Exclusions: `// projection-exempt: , `, `// safety:`, +# and diff-header lines (`+++ b/path`) via `:\+\+\+ `. +PROJ_POS='(\.broadcast_for_user|(^|[^[:alnum:]_])sse\.broadcast)[[:space:]]*\(' +PROJ_NEG='// projection-exempt: [^,]+,[[:space:]]*[^[:space:]]|// safety:|:\+\+\+ ' + +# Diff header lines must be filtered. +assert_filtered "PROJECTION: diff header line is filtered" \ + "+++ b/src/bridge/router.rs" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# A real broadcast call is flagged. +assert_flagged "PROJECTION: bare sse.broadcast_for_user is flagged" \ + "+ sse.broadcast_for_user(&user, event);" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# Chained receiver (state.sse.broadcast_for_user) is flagged. +assert_flagged "PROJECTION: chained state.sse.broadcast_for_user is flagged" \ + "+ state.sse.broadcast_for_user(&user, event);" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# A rustfmt-wrapped call is flagged. +assert_flagged "PROJECTION: rustfmt-wrapped .broadcast_for_user is flagged" \ + "+ .broadcast_for_user(&user, event);" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# Non-`sse` receiver must still fire — `broadcast_for_user` is unique to +# SseManager, so the method name alone is authoritative. +assert_flagged "PROJECTION: non-sse receiver .broadcast_for_user is flagged" \ + "+ manager.broadcast_for_user(&user, event);" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# Plain sse.broadcast call is flagged via the portable word boundary. +assert_flagged "PROJECTION: bare sse.broadcast is flagged" \ + "+ sse.broadcast(event);" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# The portable boundary must not fire on a longer identifier that ends +# in 'sse' (e.g. `usse.broadcast(...)` — not a real SseManager). +assert_filtered "PROJECTION: identifier ending in sse is not flagged" \ + "+ usse.broadcast(event);" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# Correctly annotated call is exempted. +assert_filtered "PROJECTION: annotated call with category+detail is exempt" \ + "+ sse.broadcast_for_user(&user, event); // projection-exempt: bridge dispatcher, auth gate" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# Bare `// projection-exempt: legacy` (no comma, no detail) does NOT exempt. +assert_flagged "PROJECTION: unnamed 'legacy' suppression still flagged" \ + "+ sse.broadcast_for_user(&user, event); // projection-exempt: legacy" \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# Empty detail after the comma (`// projection-exempt: foo,`) does NOT +# exempt — the documented format requires a non-empty detail. +assert_flagged "PROJECTION: empty detail after comma still flagged" \ + "+ sse.broadcast_for_user(&user, event); // projection-exempt: foo," \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# Trailing whitespace after the comma without a detail also does NOT exempt. +assert_flagged "PROJECTION: comma + whitespace-only detail still flagged" \ + "+ sse.broadcast_for_user(&user, event); // projection-exempt: foo, " \ + "$PROJ_POS" \ + "$PROJ_NEG" + +# ── DISPATCH ────────────────────────────────────────────────── +DISPATCH_POS='state\.(store|workspace|workspace_pool|extension_manager|skill_registry|session_manager)\.' +DISPATCH_NEG='// dispatch-exempt:|// safety:|:\+\+\+ ' + +assert_filtered "DISPATCH: diff header line is filtered" \ + "+++ b/src/channels/web/handlers/foo.rs" \ + "$DISPATCH_POS" \ + "$DISPATCH_NEG" + +assert_flagged "DISPATCH: direct state.store touch is flagged" \ + "+ state.store.create_project(...)" \ + "$DISPATCH_POS" \ + "$DISPATCH_NEG" + +# ── CREDNAME ────────────────────────────────────────────────── +# Portable word boundary: `(^|[^[:alnum:]_])` / `([^[:alnum:]_]|$)` — +# `grep -E`'s `\b` is a GNU extension and not recognised by BSD grep. +CREDNAME_POS='(^|[^[:alnum:]_])CredentialName([^[:alnum:]_]|$)' +CREDNAME_NEG='// web-identity-exempt:|// safety:|:\+\+\+ ' + +assert_filtered "CREDNAME: diff header line is filtered" \ + "+++ b/src/channels/web/features/settings.rs" \ + "$CREDNAME_POS" \ + "$CREDNAME_NEG" + +# A similarly-named but distinct identifier must not fire. +assert_filtered "CREDNAME: CredentialNameExt (different type) is not flagged" \ + "+ let ext: CredentialNameExt = ...;" \ + "$CREDNAME_POS" \ + "$CREDNAME_NEG" + +assert_flagged "CREDNAME: bare CredentialName reference is flagged" \ + "+ let name: CredentialName = ...;" \ + "$CREDNAME_POS" \ + "$CREDNAME_NEG" + +echo "" +echo "Passed: $PASS, Failed: $FAIL" +[ "$FAIL" -eq 0 ] From c559a58810896093a5cf768b424906a6f94aad7b Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Thu, 23 Apr 2026 00:38:01 +0900 Subject: [PATCH 2/2] feat(bridge): project 7 more engine events to AppEvents (#2844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(bridge): project 7 more engine events to AppEvents Second slice of #2654 bridge-coverage under #2792 Phase 1. Builds on #2797 (StepFailed, ChildCompleted, CodeExecutionFailed) by closing the remaining cheap `EventKind` drops: | `EventKind` | `AppEvent` | |---|---| | `LeaseGranted { lease_id, capability_name }` | new `LeaseGranted` | | `LeaseRevoked { lease_id, reason }` | new `LeaseRevoked` | | `LeaseExpired { lease_id }` | new `LeaseExpired` | | `SelfImprovementStarted` | new `SelfImprovement { phase: Started, .. }` | | `SelfImprovementComplete { prompt_updated, patterns_added }` | `SelfImprovement { phase: Complete, prompt_updated, patterns_added, .. }` | | `SelfImprovementFailed { error }` | `SelfImprovement { phase: Failed, error, .. }` | | `OrchestratorRollback { from, to, reason }` | new `OrchestratorRollback` | The three engine `SelfImprovement*` variants collapse into one wire event with a `SelfImprovementPhase` discriminator — consumers need one handler, variant-specific data is conveyed via optional phase-scoped fields. Following the types.md "Wire-stable enums" pattern — the phase enum is snake_case serde, not a stringly-typed `status` field. The lease events are security-visible: capability grants, revocations, and expiries should be auditable on the UI stream. `LeaseExpired` in particular closes a "tools start failing after TTL with no visible reason" gap. Also: - Adds `impl fmt::Display for LeaseId` in the engine alongside the existing `ThreadId` / `ProjectId` impls. The bridge code stringifies `LeaseId` for the wire; missing `Display` blocked the first compile. - Cleans up a stray doc-comment misplacement from #2797 where the `thread_event_to_app_events` docstring was attached to `code_execution_category_to_wire`. Regression tests mirror the #2797 pattern — one per representative arm (lease grant for the lease family, self-improvement complete for the richest phase, orchestrator rollback). The two remaining lease variants and two remaining self-improvement phases are covered by the existing `event_type_matches_serde_type_field` drift-catch test. Approval pair (`EventKind::ApprovalRequested` / `ApprovalReceived`) is still deferred — they need to land together with the gate-manager migration in Phase 1 PR 3 to avoid duplicate-emit with the direct `GateRequired` / `GateResolved` broadcasts. Refs: #2792, #2654 * refactor(bridge): typed SelfImprovementPhase + exhaustive match Addresses two Gemini review comments on #2844. **1. `SelfImprovementPhase` as a typed internally-tagged enum.** Previously the `AppEvent::SelfImprovement` variant carried three `Option` fields (`prompt_updated`, `patterns_added`, `error`), only some of which were populated per phase. Per `.claude/rules/types.md` — and the reviewer's note — this is an `Option`-that-can-lie pattern the type system should rule out. Phase-specific data now lives on the variant: ```rust enum SelfImprovementPhase { Started, Complete { prompt_updated: bool, patterns_added: usize }, Failed { error: String }, } ``` Wire shape is preserved via `#[serde(tag = "phase")]` on the enum and `#[serde(flatten)]` on the `AppEvent::SelfImprovement.phase` field — JSON still looks like a flat object: `{"type": "self_improvement", "phase": "complete", "prompt_updated": true, ...}`. **2. Exhaustive `thread_event_to_app_events` match.** Dropped the `_ => vec![]` wildcard in favour of explicit arms for every `EventKind` variant. Deferred-bridge variants get `vec![]` with a comment naming the migration plan: - `ApprovalRequested` / `ApprovalReceived` → waiting on the gate manager migration in #2792 Phase 1 PR 3 to avoid duplicate-emit with the existing direct `GateRequired` / `GateResolved` broadcasts. - `Unknown` → forward-compat catch-all in the engine enum; nothing useful to project from a variant written by a newer binary during a rolling deploy. New engine variants now fail the bridge to compile, which is exactly what the state-convergence epic (#2792) needs — no more silent drops. Refs: #2792, #2844 review * fix(bridge): sanitize OrchestratorRollback.reason before SSE projection `EventKind::OrchestratorRollback.reason` originates from `format!("execution failed: {e}")` in `crates/ironclaw_engine/src/executor/loop_engine.rs:327`, where `e: EngineError`. Variants like `Store { reason }` and `Llm { reason }` render DB connection strings, file paths, and raw upstream HTTP bodies — all of which reached every authenticated SSE consumer verbatim through the new `AppEvent::OrchestratorRollback` projection. Route the reason through a new `user_facing_rollback_reason` classifier that maps the existing `FailureCategory` taxonomy to short operator-facing messages (`"LLM provider unavailable"`, `"execution failed"`, etc.). The raw text still lives in the `debug!` log for operator triage, matching the pattern already used for `AppEvent::Error`. Co-Authored-By: Claude Opus 4.7 (1M context) * chore(deny): ignore RUSTSEC-2026-0104 (rustls-webpki CRL panic) Same transitive pin as 0049/0098/0099 — rustls-webpki 0.102.8 is held by libsql 0.6.0 → rustls 0.22 → hyper-rustls 0.25. The advisory explicitly notes that applications not parsing CRLs are unaffected; we do not parse CRLs. [skip-regression-check] — deny.toml-only config change. Co-Authored-By: Claude Opus 4.7 (1M context) * test(bridge): cover 4 new engine→AppEvent arms; sharpen rollback test Review follow-ups on PR #2844: - Add unit tests for `LeaseRevoked`, `LeaseExpired`, `SelfImprovementStarted`, and `SelfImprovementFailed` bridge arms — only `LeaseGranted` / `SelfImprovementComplete` / `OrchestratorRollback` had coverage before, leaving four new projections untested. - Rewrite `rollback_reason_drops_engine_error_detail` to drive the sanitiser with two unrelated leaky inputs and assert identical outputs (`execution failed`). The load-bearing check is input-independence; `!contains` probes remain as sentinel sniffs for the specific leak shapes. Avoids classifier-triggering tokens (no `upstream`, no `http 5xx`) so both inputs fall through to `Unknown`. - Reword the `ApprovalRequested` / `ApprovalReceived` comment: they are temporarily suppressed pending the gate-manager migration, not permanently dropped. The bridge will eventually map them here. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- crates/ironclaw_common/src/event.rs | 123 ++++++++ crates/ironclaw_common/src/lib.rs | 2 +- crates/ironclaw_engine/src/types/error.rs | 6 + src/bridge/router.rs | 344 +++++++++++++++++++++- src/bridge/user_facing_errors.rs | 82 ++++++ 5 files changed, 551 insertions(+), 6 deletions(-) diff --git a/crates/ironclaw_common/src/event.rs b/crates/ironclaw_common/src/event.rs index d3f76df027f..760191f3e6a 100644 --- a/crates/ironclaw_common/src/event.rs +++ b/crates/ironclaw_common/src/event.rs @@ -538,6 +538,100 @@ pub enum AppEvent { #[serde(skip_serializing_if = "Option::is_none")] thread_id: Option, }, + + /// A capability lease was granted to a thread. + /// + /// Bridged from engine `EventKind::LeaseGranted`. Security-visible: + /// capability grants should be auditable in the UI. + #[serde(rename = "lease_granted")] + LeaseGranted { + lease_id: String, + capability_name: String, + #[serde(skip_serializing_if = "Option::is_none")] + thread_id: Option, + }, + + /// A capability lease was explicitly revoked. + /// + /// Bridged from engine `EventKind::LeaseRevoked`. `reason` is the + /// engine's revocation message, surfaced so users can tell a + /// revocation apart from an expiry. + #[serde(rename = "lease_revoked")] + LeaseRevoked { + lease_id: String, + reason: String, + #[serde(skip_serializing_if = "Option::is_none")] + thread_id: Option, + }, + + /// A capability lease reached its TTL and expired. + /// + /// Bridged from engine `EventKind::LeaseExpired`. Without this, tools + /// begin failing after a lease's TTL with no visible explanation. + #[serde(rename = "lease_expired")] + LeaseExpired { + lease_id: String, + #[serde(skip_serializing_if = "Option::is_none")] + thread_id: Option, + }, + + /// Background self-improvement lifecycle event. + /// + /// Bridged from engine `EventKind::SelfImprovement{Started,Complete,Failed}`. + /// The three engine variants collapse into one wire event with a + /// nested `SelfImprovementPhase` carrying per-phase data — consumers + /// need one handler, and the compiler enforces that phase-specific + /// fields travel with their phase (no `Option` sentinels that + /// claim "maybe present" when the phase excludes them). + /// + /// Wire shape uses `#[serde(flatten)]` + the phase enum's + /// `#[serde(tag = "phase")]`, so the JSON payload is flat: + /// `{"type": "self_improvement", "phase": "complete", "prompt_updated": true, ...}`. + #[serde(rename = "self_improvement")] + SelfImprovement { + #[serde(flatten)] + phase: SelfImprovementPhase, + #[serde(skip_serializing_if = "Option::is_none")] + thread_id: Option, + }, + + /// Orchestrator version was rolled back. + /// + /// Bridged from engine `EventKind::OrchestratorRollback`. Operator- + /// facing; surfaces the from/to versions so failures after an + /// upgrade are correlatable with the rollback point. + #[serde(rename = "orchestrator_rollback")] + OrchestratorRollback { + from_version: u64, + to_version: u64, + reason: String, + #[serde(skip_serializing_if = "Option::is_none")] + thread_id: Option, + }, +} + +/// Phase data for `AppEvent::SelfImprovement`. +/// +/// Mirrors engine `EventKind::SelfImprovement{Started,Complete,Failed}` +/// as a single typed wire enum. Variant-specific fields are part of the +/// variant, not optional fields on the outer event — per +/// `.claude/rules/types.md` the compiler should reject a `Failed` value +/// carrying `prompt_updated`, which an `Option`-field approach cannot. +/// +/// Serialized with an internally-tagged `phase` discriminator; the +/// outer `AppEvent::SelfImprovement` flattens this into its payload so +/// the wire shape stays a single flat JSON object. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "phase", rename_all = "snake_case")] +pub enum SelfImprovementPhase { + Started, + Complete { + prompt_updated: bool, + patterns_added: usize, + }, + Failed { + error: String, + }, } /// Wire-side mirror of `ironclaw_engine::CodeExecutionFailure`. @@ -603,6 +697,11 @@ impl AppEvent { Self::MissionThreadSpawned { .. } => "mission_thread_spawned", Self::PlanUpdate { .. } => "plan_update", Self::CodeExecutionFailed { .. } => "code_execution_failed", + Self::LeaseGranted { .. } => "lease_granted", + Self::LeaseRevoked { .. } => "lease_revoked", + Self::LeaseExpired { .. } => "lease_expired", + Self::SelfImprovement { .. } => "self_improvement", + Self::OrchestratorRollback { .. } => "orchestrator_rollback", } } @@ -810,6 +909,30 @@ mod tests { code_hash: None, thread_id: None, }, + AppEvent::LeaseGranted { + lease_id: String::new(), + capability_name: String::new(), + thread_id: None, + }, + AppEvent::LeaseRevoked { + lease_id: String::new(), + reason: String::new(), + thread_id: None, + }, + AppEvent::LeaseExpired { + lease_id: String::new(), + thread_id: None, + }, + AppEvent::SelfImprovement { + phase: SelfImprovementPhase::Started, + thread_id: None, + }, + AppEvent::OrchestratorRollback { + from_version: 0, + to_version: 0, + reason: String::new(), + thread_id: None, + }, ]; for variant in &variants { diff --git a/crates/ironclaw_common/src/lib.rs b/crates/ironclaw_common/src/lib.rs index f0ddf7ab1d6..52777fdfaf9 100644 --- a/crates/ironclaw_common/src/lib.rs +++ b/crates/ironclaw_common/src/lib.rs @@ -7,7 +7,7 @@ mod util; pub use event::{ AppEvent, CodeExecutionFailureCategory, JobResultStatus, JobResultStatusParseError, - OnboardingStateDto, PlanStepDto, ToolDecisionDto, + OnboardingStateDto, PlanStepDto, SelfImprovementPhase, ToolDecisionDto, }; pub use identity::{ CredentialName, ExtensionName, ExternalThreadId, ExternalThreadIdError, IdentityError, diff --git a/crates/ironclaw_engine/src/types/error.rs b/crates/ironclaw_engine/src/types/error.rs index 8f33d85bfc5..6fea44f315c 100644 --- a/crates/ironclaw_engine/src/types/error.rs +++ b/crates/ironclaw_engine/src/types/error.rs @@ -221,3 +221,9 @@ impl fmt::Display for ProjectId { write!(f, "{}", self.0) } } + +impl fmt::Display for crate::types::capability::LeaseId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } +} diff --git a/src/bridge/router.rs b/src/bridge/router.rs index eed08d480b5..fb265bd7031 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -4562,10 +4562,6 @@ async fn forward_event_to_channel( } } -/// Convert a ThreadEvent to AppEvents for the web gateway SSE stream. -/// -/// Returns multiple events when needed (e.g., ToolStarted + ToolCompleted -/// so the frontend creates the card then resolves it). /// Bridge engine-side `CodeExecutionFailure` to its wire mirror /// `CodeExecutionFailureCategory` in `ironclaw_common`. /// @@ -4591,6 +4587,10 @@ fn code_execution_category_to_wire( } } +/// Convert a `ThreadEvent` to `AppEvent`s for the web gateway SSE stream. +/// +/// Returns multiple events when needed (e.g., `ToolStarted` + `ToolCompleted` +/// so the frontend creates the card then resolves it). fn thread_event_to_app_events( event: &ironclaw_engine::ThreadEvent, thread_id: &str, @@ -4711,7 +4711,90 @@ fn thread_event_to_app_events( thread_id: Some(thread_id.into()), feedback: Vec::new(), }], - _ => vec![], + EventKind::LeaseGranted { + lease_id, + capability_name, + } => vec![AppEvent::LeaseGranted { + lease_id: lease_id.to_string(), + capability_name: capability_name.clone(), + thread_id: Some(thread_id.into()), + }], + EventKind::LeaseRevoked { lease_id, reason } => vec![AppEvent::LeaseRevoked { + lease_id: lease_id.to_string(), + reason: reason.clone(), + thread_id: Some(thread_id.into()), + }], + EventKind::LeaseExpired { lease_id } => vec![AppEvent::LeaseExpired { + lease_id: lease_id.to_string(), + thread_id: Some(thread_id.into()), + }], + EventKind::SelfImprovementStarted => vec![AppEvent::SelfImprovement { + phase: ironclaw_common::SelfImprovementPhase::Started, + thread_id: Some(thread_id.into()), + }], + EventKind::SelfImprovementComplete { + prompt_updated, + patterns_added, + } => vec![AppEvent::SelfImprovement { + phase: ironclaw_common::SelfImprovementPhase::Complete { + prompt_updated: *prompt_updated, + patterns_added: *patterns_added, + }, + thread_id: Some(thread_id.into()), + }], + EventKind::SelfImprovementFailed { error } => vec![AppEvent::SelfImprovement { + phase: ironclaw_common::SelfImprovementPhase::Failed { + error: error.clone(), + }, + thread_id: Some(thread_id.into()), + }], + EventKind::OrchestratorRollback { + from_version, + to_version, + reason, + } => { + // `reason` originates from `format!("execution failed: {e}")` + // in the engine's rollback path, where `e: EngineError` can + // render DB connection strings, file paths, or raw upstream + // HTTP bodies. SSE error-adjacent frames reach every + // authenticated consumer, so the wire carries a classified + // operator-facing message and the raw text stays in the log. + tracing::debug!( + from_version = *from_version, + to_version = *to_version, + raw_reason = %reason, + "orchestrator rollback event" + ); + vec![AppEvent::OrchestratorRollback { + from_version: *from_version, + to_version: *to_version, + reason: crate::bridge::user_facing_errors::user_facing_rollback_reason(reason) + .to_string(), + thread_id: Some(thread_id.into()), + }] + } + + // Temporarily-suppressed engine variants. These are NOT bridged + // to `AppEvent` today because equivalent gate events are still + // emitted directly by the gate manager, and forwarding them + // here as well would make the UI render the same state twice. + // Migration plan per #2792 Phase 1 PR 3: + // + // - `ApprovalRequested` / `ApprovalReceived` are suppressed + // only until the gate manager stops broadcasting direct + // `AppEvent::GateRequired` / `GateResolved` events. + // - Once that migration lands, this function remains the + // bridge: map these engine variants to the corresponding + // `AppEvent`s here (or remove the direct emits), rather + // than treating them as permanently dropped. + EventKind::ApprovalRequested { .. } => vec![], + EventKind::ApprovalReceived { .. } => vec![], + + // Forward-compat catch-all in the engine enum (see + // `#[serde(other)] Unknown` in `ironclaw_engine::EventKind`). + // Nothing useful to show; the unknown variant would have been + // written by a newer binary during a rolling deploy. + EventKind::Unknown => vec![], } } @@ -7242,6 +7325,257 @@ mod tests { assert_eq!(thread_id.as_deref(), Some("thread-codeact")); } + #[test] + fn thread_event_to_app_events_bridges_lease_granted() { + let lease = ironclaw_engine::LeaseId::new(); + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::LeaseGranted { + lease_id: lease, + capability_name: "http_fetch".to_string(), + }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-lease"); + + assert_eq!(app_events.len(), 1); + let AppEvent::LeaseGranted { + lease_id, + capability_name, + thread_id, + } = &app_events[0] + else { + panic!("expected AppEvent::LeaseGranted, got {:?}", app_events[0]); + }; + assert_eq!(lease_id, &lease.to_string()); + assert_eq!(capability_name, "http_fetch"); + assert_eq!(thread_id.as_deref(), Some("thread-lease")); + } + + #[test] + fn thread_event_to_app_events_bridges_lease_revoked() { + let lease = ironclaw_engine::LeaseId::new(); + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::LeaseRevoked { + lease_id: lease, + reason: "policy check failed".to_string(), + }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-revoke"); + + assert_eq!(app_events.len(), 1); + let AppEvent::LeaseRevoked { + lease_id, + reason, + thread_id, + } = &app_events[0] + else { + panic!("expected AppEvent::LeaseRevoked, got {:?}", app_events[0]); + }; + assert_eq!(lease_id, &lease.to_string()); + assert_eq!(reason, "policy check failed"); + assert_eq!(thread_id.as_deref(), Some("thread-revoke")); + } + + #[test] + fn thread_event_to_app_events_bridges_lease_expired() { + let lease = ironclaw_engine::LeaseId::new(); + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::LeaseExpired { lease_id: lease }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-expire"); + + assert_eq!(app_events.len(), 1); + let AppEvent::LeaseExpired { + lease_id, + thread_id, + } = &app_events[0] + else { + panic!("expected AppEvent::LeaseExpired, got {:?}", app_events[0]); + }; + assert_eq!(lease_id, &lease.to_string()); + assert_eq!(thread_id.as_deref(), Some("thread-expire")); + } + + #[test] + fn thread_event_to_app_events_bridges_self_improvement_started() { + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::SelfImprovementStarted, + ); + + let app_events = thread_event_to_app_events(&event, "thread-improve-start"); + + assert_eq!(app_events.len(), 1); + let AppEvent::SelfImprovement { phase, thread_id } = &app_events[0] else { + panic!( + "expected AppEvent::SelfImprovement, got {:?}", + app_events[0] + ); + }; + assert_eq!(phase, &ironclaw_common::SelfImprovementPhase::Started); + assert_eq!(thread_id.as_deref(), Some("thread-improve-start")); + } + + #[test] + fn thread_event_to_app_events_bridges_self_improvement_failed() { + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::SelfImprovementFailed { + error: "diagnosis prompt timed out".to_string(), + }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-improve-fail"); + + assert_eq!(app_events.len(), 1); + let AppEvent::SelfImprovement { phase, thread_id } = &app_events[0] else { + panic!( + "expected AppEvent::SelfImprovement, got {:?}", + app_events[0] + ); + }; + let ironclaw_common::SelfImprovementPhase::Failed { error } = phase else { + panic!("expected SelfImprovementPhase::Failed, got {phase:?}"); + }; + assert_eq!(error, "diagnosis prompt timed out"); + assert_eq!(thread_id.as_deref(), Some("thread-improve-fail")); + } + + #[test] + fn thread_event_to_app_events_bridges_self_improvement_complete() { + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::SelfImprovementComplete { + prompt_updated: true, + patterns_added: 3, + }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-improve"); + + assert_eq!(app_events.len(), 1); + let AppEvent::SelfImprovement { phase, thread_id } = &app_events[0] else { + panic!( + "expected AppEvent::SelfImprovement, got {:?}", + app_events[0] + ); + }; + let ironclaw_common::SelfImprovementPhase::Complete { + prompt_updated, + patterns_added, + } = phase + else { + panic!("expected SelfImprovementPhase::Complete, got {phase:?}"); + }; + assert!(*prompt_updated); + assert_eq!(*patterns_added, 3); + assert_eq!(thread_id.as_deref(), Some("thread-improve")); + } + + #[test] + fn thread_event_to_app_events_bridges_orchestrator_rollback() { + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::OrchestratorRollback { + from_version: 7, + to_version: 6, + reason: "health probe failed after upgrade".to_string(), + }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-rollback"); + + assert_eq!(app_events.len(), 1); + let AppEvent::OrchestratorRollback { + from_version, + to_version, + reason, + thread_id, + } = &app_events[0] + else { + panic!( + "expected AppEvent::OrchestratorRollback, got {:?}", + app_events[0] + ); + }; + assert_eq!(*from_version, 7); + assert_eq!(*to_version, 6); + // Unknown-shape reasons collapse to the safe generic classification. + assert_eq!(reason, "execution failed"); + assert_eq!(thread_id.as_deref(), Some("thread-rollback")); + } + + #[test] + fn orchestrator_rollback_does_not_leak_engine_error_detail() { + // Regression for PR #2844 review: the engine rollback path emits + // `format!("execution failed: {e}")` where `e: EngineError`. + // Variants like `Store { reason }` / `Llm { reason }` can render + // DB connection strings, file paths, and raw upstream HTTP bodies. + // The bridge must sanitize before broadcasting to SSE consumers. + let leaky = "execution failed: store error: connection string \ + 'postgres://bob:hunter2@db.internal:5432/ironclaw' refused: \ + File \"/home/runner/.ironclaw/state.db\" not found"; + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::OrchestratorRollback { + from_version: 3, + to_version: 2, + reason: leaky.to_string(), + }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-leak"); + + let AppEvent::OrchestratorRollback { reason, .. } = &app_events[0] else { + panic!("expected AppEvent::OrchestratorRollback"); + }; + assert!( + !reason.contains("postgres://"), + "leaked connection string: {reason}" + ); + assert!(!reason.contains("hunter2"), "leaked password: {reason}"); + assert!( + !reason.contains("/home/runner"), + "leaked filesystem path: {reason}" + ); + assert!( + !reason.contains("store error"), + "leaked internal wrap: {reason}" + ); + assert!( + !reason.contains("state.db"), + "leaked internal filename: {reason}" + ); + } + + #[test] + fn orchestrator_rollback_classifies_known_upstream_failures() { + // A 502 in the rollback reason should still render a classified + // operator-facing message, not the bare "execution failed" fallback. + let event = ironclaw_engine::ThreadEvent::new( + ironclaw_engine::ThreadId::new(), + ironclaw_engine::EventKind::OrchestratorRollback { + from_version: 4, + to_version: 3, + reason: "execution failed: LLM error: Provider nearai request failed: \ + HTTP 502 Bad Gateway" + .to_string(), + }, + ); + + let app_events = thread_event_to_app_events(&event, "thread-502"); + + let AppEvent::OrchestratorRollback { reason, .. } = &app_events[0] else { + panic!("expected AppEvent::OrchestratorRollback"); + }; + assert_eq!(reason, "LLM provider unavailable"); + } + #[test] fn resolved_call_id_legacy_fallback_uses_last_unresolved_parallel_call() { let mut thread = ironclaw_engine::Thread::new( diff --git a/src/bridge/user_facing_errors.rs b/src/bridge/user_facing_errors.rs index 0f349361e80..2935b6fae16 100644 --- a/src/bridge/user_facing_errors.rs +++ b/src/bridge/user_facing_errors.rs @@ -81,6 +81,27 @@ pub(crate) fn user_facing_thread_failure(error: &str) -> String { } } +/// Convert a raw orchestrator-rollback reason into a short, +/// operator-facing classification that is safe to broadcast over SSE. +/// +/// The rollback event's source string is +/// `format!("execution failed: {e}")` where `e` is an `EngineError` — +/// variants like `Store { reason }` and `Llm { reason }` can carry DB +/// connection strings, file paths, and raw upstream HTTP bodies. That +/// raw text must not reach every authenticated SSE consumer. Callers +/// should log the raw `reason` at `debug!` before calling this so +/// operators retain the diagnostic detail server-side. +pub(crate) fn user_facing_rollback_reason(reason: &str) -> &'static str { + match classify_failure(reason) { + FailureCategory::LlmUnavailable => "LLM provider unavailable", + FailureCategory::LlmRateLimited => "LLM provider rate-limited", + FailureCategory::ContextTooLarge => "context too large for provider", + FailureCategory::AuthFailure => "LLM provider authentication failed", + FailureCategory::IterationLimit => "execution step limit reached", + FailureCategory::Unknown => "execution failed", + } +} + /// Classify a raw failure string. Public(crate) so tests can assert on the /// category independently of the user-facing wording. pub(crate) fn classify_failure(error: &str) -> FailureCategory { @@ -418,6 +439,67 @@ mod tests { ); } + #[test] + fn rollback_reason_drops_engine_error_detail() { + // Regression for PR #2844: `EventKind::OrchestratorRollback.reason` + // originates from `format!("execution failed: {e}")` in the engine, + // where `e: EngineError` can render DB connection strings, file + // paths, tokens, and upstream HTTP bodies. The sanitizer must + // produce an output that is fully independent of the raw engine + // detail — two unrelated leaky strings must collapse to the same + // classified message. `assert_eq!(msg_a, msg_b)` is the load- + // bearing check; the `contains` probes below are sentinel sniffs + // for the specific leak shapes the fix is meant to prevent. + let raw_a = "execution failed: store error: connection \ + 'postgres://bob:hunter2@db:5432/x' refused: \ + File \"/home/runner/.ironclaw/state.db\" not found"; + // Deliberately avoid any substring that the classifier recognises + // (no `upstream`, no `http 5xx`, no `rate limited`, etc.) so both + // inputs fall into the `Unknown` category — the test is about + // sanitisation, not classification. + let raw_b = "execution failed: store error: leaked \ + token sk_live_123 and request id req_abc123 at /etc/secrets/keyring"; + + let msg_a = user_facing_rollback_reason(raw_a); + let msg_b = user_facing_rollback_reason(raw_b); + + assert_eq!(msg_a, "execution failed"); + assert_eq!(msg_b, "execution failed"); + assert_eq!( + msg_a, msg_b, + "sanitized rollback reason must be independent of raw engine detail" + ); + assert!(!msg_a.contains("postgres://")); + assert!(!msg_a.contains("hunter2")); + assert!(!msg_a.contains("/home/runner")); + assert!(!msg_b.contains("sk_live_123")); + assert!(!msg_b.contains("req_abc123")); + } + + #[test] + fn rollback_reason_classifies_known_upstream_failures() { + assert_eq!( + user_facing_rollback_reason("execution failed: LLM error: HTTP 502 Bad Gateway"), + "LLM provider unavailable" + ); + assert_eq!( + user_facing_rollback_reason("execution failed: HTTP 429 Too Many Requests"), + "LLM provider rate-limited" + ); + assert_eq!( + user_facing_rollback_reason("execution failed: HTTP 413 Payload Too Large"), + "context too large for provider" + ); + assert_eq!( + user_facing_rollback_reason("execution failed: HTTP 401 Unauthorized"), + "LLM provider authentication failed" + ); + assert_eq!( + user_facing_rollback_reason("execution failed: max iterations reached"), + "execution step limit reached" + ); + } + #[test] fn all_messages_end_with_period() { // Tiny presentation invariant — every sanitized message is a