feat(debug-panel): expand Activity tab coverage with CodeAct + warnings - #2850
Conversation
The Activity tab was missing most event types: CodeAct runs showed only lossy chat summaries, WARN/ERROR logs only landed in server stdout, and tool entries hid their parameters on success. - Emit AppEvent::CodeExecuted (verbose-only) with raw code, stdout, and return value from the engine orchestrator so observers see what the model actually wrote. - Bridge WARN/ERROR tracing into AppEvent::Warning via spawn_warning_bridge, scoped by owner_id in multi-tenant mode to prevent cross-tenant log bleed. - Backfill params_summary on ActionExecuted/ActionFailed events from structured + scripting executors so the Activity tab shows tool args immediately (not just on failure) without waiting for tool_completed. - Wire debug-panel.js to render code_executed, warning, gate_required, gate_resolved, approval_needed, skill_activated, plan_update, thread_state_changed, child/mission_thread_spawned, onboarding_state, image_generated, suggestions, and the full sandbox-job event family. - Extract shared on(name, handler) wrapper to dedupe ~25 copies of the JSON-parse + reconnect-counter housekeeping and keep lastEventTime bookkeeping consistent across listeners. - Add i18n strings (en/ko/zh-CN) and CSS icon colors for the new activity types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: channel/web
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the diagnostic capabilities of the debug inspector by introducing detailed execution traces and a server-side warning bridge. Key changes include the addition of CodeExecuted and Warning events, which allow the debug panel to surface raw code, stdout, and filtered log entries. The frontend has been refactored to use a more robust event-handling pattern and now provides comprehensive visibility into tool parameters, gate resolutions, and sandbox job lifecycles. Review feedback suggests extending the use of the new event-handling helper for consistency and ensuring that structured parameters in gate events are properly stringified for the UI.
| function on(name, handler) { | ||
| es.addEventListener(name, function (e) { | ||
| try { handler(JSON.parse(e.data)); } catch (_) { /* ignore */ } | ||
| lastEventTime = Date.now(); totalEventsReceived++; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The on helper function is a great addition for deduplicating SSE event handling. However, several existing handlers (e.g., status at line 275 and turn_cost at line 359) were not migrated to use it. To ensure consistent bookkeeping of lastEventTime and totalEventsReceived, and to fully realize the deduplication benefits mentioned in the PR description, these should also be refactored to use the on() wrapper where they parse JSON.
References
- Maintain style consistency with existing code and ensure new patterns are applied uniformly across the file.
There was a problem hiding this comment.
Fixed in aae97be. Migrated the remaining JSON-parsing listeners — status, thinking, tool_started, tool_completed, tool_result, turn_cost, response, error, turn_metrics, tool_result_full — to use the on() wrapper. Left stream_chunk on the raw addEventListener path since it doesn't parse JSON, with a short comment explaining why.
| on('gate_required', function (data) { | ||
| var body = [ | ||
| data.gate_name ? 'gate: ' + data.gate_name : '', | ||
| data.tool_name ? 'tool: ' + data.tool_name : '', | ||
| data.description || '', | ||
| data.parameters ? 'params: ' + data.parameters : '', | ||
| ].filter(Boolean).join('\n'); | ||
| addActivity('gate', t('debug.activityGateRequired'), timeNow(), 'pending', body, { labelKey: 'debug.activityGateRequired' }); | ||
| }); |
There was a problem hiding this comment.
In the gate_required and approval_needed handlers, data.parameters is appended to the body string. If the backend sends these as structured objects rather than pre-summarized strings, they will render as [object Object]. Consider using JSON.stringify for these fields, similar to how data.input is handled in the job_tool_use listener at line 567.
| on('gate_required', function (data) { | |
| var body = [ | |
| data.gate_name ? 'gate: ' + data.gate_name : '', | |
| data.tool_name ? 'tool: ' + data.tool_name : '', | |
| data.description || '', | |
| data.parameters ? 'params: ' + data.parameters : '', | |
| ].filter(Boolean).join('\n'); | |
| addActivity('gate', t('debug.activityGateRequired'), timeNow(), 'pending', body, { labelKey: 'debug.activityGateRequired' }); | |
| }); | |
| on('gate_required', function (data) { | |
| var params = typeof data.parameters === 'string' ? data.parameters : JSON.stringify(data.parameters, null, 2); | |
| var body = [ | |
| data.gate_name ? 'gate: ' + data.gate_name : '', | |
| data.tool_name ? 'tool: ' + data.tool_name : '', | |
| data.description || '', | |
| params ? 'params: ' + params : '', | |
| ].filter(Boolean).join('\n'); | |
| addActivity('gate', t('debug.activityGateRequired'), timeNow(), 'pending', body, { labelKey: 'debug.activityGateRequired' }); | |
| }); |
References
- Maintain style consistency with existing code. The suggestion uses 'var' to match the existing codebase style.
There was a problem hiding this comment.
Not changing — this looks like a false positive. The backend wire contract for both gate_required and approval_needed types parameters as a String (see crates/ironclaw_common/src/event.rs:267 for ApprovalNeeded and :298 for GateRequired), so the UI can safely string-concat. The data.input field on job_tool_use is genuinely an object (it's the Claude-bridge tool arguments), which is why that handler uses JSON.stringify — different wire shape.
There was a problem hiding this comment.
Pull request overview
Expands the web debug panel “Activity” tab by emitting and rendering additional backend events (notably CodeAct traces and WARN/ERROR log forwarding), and by improving tool-parameter visibility via params_summary propagation.
Changes:
- Add new engine/common event variants (
EventKind::CodeExecuted,AppEvent::{CodeExecuted, Warning}) and wire them into the bridge/SSE flow. - Populate
params_summaryin structured + scripting executors and surface it in tool activity entries. - Extend debug-panel frontend (JS/CSS + i18n) to display many more activity event types, with shared listener wiring.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/support/replay_outcome.rs | Recognize CodeExecuted in event-kind naming for replay/testing utilities. |
| src/channels/web/mod.rs | Start warning→SSE forwarding bridge during gateway startup (scoped in multi-tenant mode). |
| src/channels/web/log_layer.rs | Add spawn_warning_bridge to forward WARN/ERROR logs into SSE as AppEvent::Warning. |
| src/bridge/router.rs | Backfill tool parameters from params_summary and emit AppEvent::CodeExecuted for engine v2 thread events. |
| crates/ironclaw_gateway/static/i18n/en.js | Add i18n strings for new Activity event types. |
| crates/ironclaw_gateway/static/i18n/ko.js | Add i18n strings for new Activity event types. |
| crates/ironclaw_gateway/static/i18n/zh-CN.js | Add i18n strings for new Activity event types. |
| crates/ironclaw_gateway/static/debug-panel.js | Add unified on() listener wrapper; render CodeAct, warnings, gate lifecycle, plan/thread/job events; show params earlier. |
| crates/ironclaw_gateway/static/debug-panel.css | Add icon colors for newly rendered activity types. |
| crates/ironclaw_engine/src/types/event.rs | Introduce EventKind::CodeExecuted and retain CodeAct stdout/code for observers. |
| crates/ironclaw_engine/src/executor/structured.rs | Compute and attach params_summary for tool call events (success + failure). |
| crates/ironclaw_engine/src/executor/scripting.rs | Compute and attach params_summary for preflight-denied scripting tool calls. |
| crates/ironclaw_engine/src/executor/orchestrator.rs | Emit EventKind::CodeExecuted after CodeAct execution (success or failure) with code/stdout. |
| crates/ironclaw_common/src/event.rs | Add AppEvent::{CodeExecuted, Warning} and mark them verbose-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut rx = broadcaster.subscribe(); | ||
| tokio::spawn(async move { | ||
| loop { | ||
| match rx.recv().await { | ||
| Ok(entry) => { | ||
| if entry.level != "WARN" && entry.level != "ERROR" { | ||
| continue; | ||
| } | ||
| let event = AppEvent::Warning { | ||
| source: entry.target, | ||
| message: entry.message, | ||
| thread_id: None, | ||
| }; | ||
| match &owner_id { | ||
| Some(uid) => sse.broadcast_for_user(uid, event), | ||
| None => sse.broadcast(event), | ||
| } |
There was a problem hiding this comment.
spawn_warning_bridge broadcasts AppEvent::Warning (verbose-only), but it currently does so regardless of whether any verbose/debug SSE subscribers exist. Because the SSE broadcast channel is shared by all clients, flooding it with verbose-only warnings can still cause lag/reconnect pressure for non-verbose subscribers (they must drain and filter the stream). Consider checking sse.has_verbose_receivers() before constructing/broadcasting the event (and skipping work when false).
There was a problem hiding this comment.
Fixed in aae97be. spawn_warning_bridge now short-circuits on sse.has_verbose_receivers() before constructing the AppEvent, so when no debug/admin subscriber is connected the warning is dropped instead of flowing through the shared SSE broadcast channel.
| EventKind::CodeExecuted { | ||
| code, | ||
| stdout, | ||
| return_value, | ||
| duration_ms, | ||
| .. | ||
| } => vec![AppEvent::CodeExecuted { | ||
| code: code.clone(), | ||
| stdout: stdout.clone(), | ||
| return_value: return_value.clone(), | ||
| duration_ms: *duration_ms, | ||
| thread_id: Some(thread_id.into()), | ||
| }], |
There was a problem hiding this comment.
AppEvent::CodeExecuted is marked verbose-only, but this mapping eagerly clones potentially large code/stdout into an SSE event. In await_thread_outcome these AppEvents are broadcast unconditionally, so verbose-only CodeExecuted payloads can still fill the shared broadcast buffer and increase lag/reconnects for non-verbose subscribers. Consider gating generation/broadcast of this event on SseManager::has_verbose_receivers() (e.g., check before calling thread_event_to_app_events, or plumb an include_verbose flag) to avoid unnecessary cloning and broadcast load.
There was a problem hiding this comment.
Fixed in aae97be. Added the gate inside await_thread_outcome — after thread_event_to_app_events returns, any AppEvent whose is_verbose_only() is true is skipped when sse.has_verbose_receivers() is false. Mirrors the existing send_status short-circuit. This covers CodeExecuted, Warning, and any future verbose-only variant without per-event plumbing. (And CodeExecuted's payload is now capped at 8_000 chars before the clone — see the other thread on orchestrator.rs.)
| error: reason.clone(), | ||
| duration_ms: 0, | ||
| params_summary: None, | ||
| params_summary: crate::types::event::summarize_params(action_name, params), |
There was a problem hiding this comment.
High: this new params_summary path looks like it can bypass our normal sensitive-param redaction. summarize_params() falls back to the first string value for unknown tools, and this PR now threads that summary into ActionFailed/ActionExecuted events that reach SSE/debug-panel. For MCP or built-in tools whose first string field is something like token, api_key, or password, we would now surface up to 50 chars here. Could we redact before summarizing (using tool.sensitive_params()) or restrict the fallback to an explicit allow-list of safe keys instead of obj.values().find_map(...)?
There was a problem hiding this comment.
Fixed in aae97be. summarize_params's generic fallback now filters out sensitive-looking keys (token/secret/password/api_key/auth/credential/bearer + _key suffix / standalone key) before picking the first string value, so MCP or unknown-tool calls with a secret as the leading field no longer surface it into ActionExecuted/ActionFailed events or debug-panel SSE. Went with a denylist rather than a hard allow-list so legitimate unknown-tool params still render a summary; the engine crate can't reach the host's Tool::sensitive_params() directly. Added two regression tests in types::event::tests.
| EventKind::CodeExecuted { | ||
| step_id: exec_ctx.step_id, | ||
| code: code.clone(), | ||
| stdout: result.stdout.clone(), |
There was a problem hiding this comment.
Medium: this clones the full stdout payload into ThreadEvent::CodeExecuted on every CodeAct run, even when there are no verbose/debug subscribers. Because there is no truncation/cap here, a step that prints a large blob can now bloat stored thread events and SSE payloads significantly. Was that retention cost intentional? I think this should either be capped before persistence or gated behind a verbose-only check so debug instrumentation does not become an unbounded storage/memory path.
There was a problem hiding this comment.
Fixed in aae97be. code and stdout are now tail-truncated to 8_000 chars (reusing the tail_chars helper already used by CodeExecutionFailed's error text, and matching scripting::OUTPUT_TRUNCATE_LEN) before the event is constructed, so persisted thread events and SSE payloads stay bounded regardless of subscriber count. The broadcast path is also gated on has_verbose_receivers() now (see the Copilot thread on router.rs) so non-debug tabs don't see the event at all.
| data.resolution ? 'resolution: ' + data.resolution : '', | ||
| data.message || '', | ||
| ].filter(Boolean).join('\n'); | ||
| var status = STATUS_TO_ACTIVITY[data.resolution] || 'success'; |
There was a problem hiding this comment.
Medium: defaulting unknown gate_resolved values to 'success' looks wrong. The backend already emits resolution: "expired" when the target thread disappears during resume (src/bridge/router.rs), so that failure path will render as a green success badge in Activity. Could we mirror the neighboring handlers and use || null (or an explicit failure mapping for expired) instead of treating every unmapped resolution as success?
There was a problem hiding this comment.
Fixed in aae97be. Added a dedicated GATE_RESOLUTION_STATUS map with explicit entries for the six known resolutions (approved/credential_provided/external_callback → success, denied/cancelled/expired → failure) and a fallback to null, so expired now renders as a failure and any future unknown resolution renders neutral instead of green. Kept STATUS_TO_ACTIVITY for jobs/plans/onboarding where success is a sensible default for completed/ready.
- summarize_params generic fallback: skip sensitive-looking parameter keys (token/secret/password/api_key/auth/credential/bearer) so MCP and unknown-tool calls can't surface secret values into ActionExecuted events or debug-panel SSE. Adds two regression tests. - Cap CodeExecuted code/stdout at 8_000 chars (tail-last) before emission so a step that prints a large blob can't bloat persisted thread events. Matches the existing scripting OUTPUT_TRUNCATE_LEN. - await_thread_outcome: skip broadcasting verbose-only AppEvents when no debug subscriber is connected — mirrors the send_status gate and keeps CodeExecuted off the shared SSE broadcast buffer for normal browser tabs. - spawn_warning_bridge: same short-circuit on has_verbose_receivers. - debug-panel.js: introduce GATE_RESOLUTION_STATUS so `expired` (a failure path from router.rs) no longer renders as a green success badge; shared STATUS_TO_ACTIVITY map is kept for jobs/ plans/onboarding where `success` is the right default. - debug-panel.js: migrate the remaining legacy listeners to the shared on() wrapper so lastEventTime / totalEventsReceived bookkeeping stays consistent across every activity listener. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| // owner so per-request log context (thread ids, paths, emails) | ||
| // doesn't bleed across tenants via admin debug panels. | ||
| if let Some(log_broadcaster) = self.state.log_broadcaster.as_ref() { | ||
| let owner_scope = self |
There was a problem hiding this comment.
High Severity
This scopes the warning bridge once at gateway startup to the static gateway owner_id, not to the user who triggered the warning-producing request. In multi-tenant mode that means WARN/ERROR log lines from tenant A's request can be delivered to the owner/admin account instead of tenant A, while the originating tenant never sees the warning in their own debug panel.
That avoids broadcasting warnings to everyone, but it still misroutes per-request diagnostics across accounts. I think this needs request/user-aware scoping at the point where the warning event is emitted (or the warning bridge should be disabled for multi-tenant mode until that provenance exists), rather than binding everything to the owner up front.
There was a problem hiding this comment.
Fixed in 1d499f8. Disabled the warning bridge entirely when multi_tenant_mode is true — matching the second option you suggested ("disable for multi-tenant mode until provenance exists"). The tracing layer captures log context at the global subscriber scope, so there's no way to scope warnings per-request today without threading tenant provenance through every warn!/error! call site, which is well outside this PR. Single-user gateways still get the bridge with no owner_scope since there's only one consumer.
| success: true, | ||
| error: None, | ||
| parameters: None, | ||
| parameters: params_summary.clone(), |
There was a problem hiding this comment.
High Severity
Routing params_summary into ToolCompleted.parameters makes the existing summary policy user-visible on the debug SSE path, and that policy still returns raw http/web_fetch URLs and raw shell commands. That means a successful tool call can now expose query-string secrets, signed URLs, or inline auth headers (for example curl -H "Authorization: Bearer ...") directly in the debug panel.
I don't think the generic MCP-key filtering added in this PR is enough here, because these known-tool branches bypass it entirely. Please either redact these summaries before broadcasting them (for example with the leak detector / safety layer) or make the per-tool summaries strip sensitive parts such as URL query strings and auth-bearing shell arguments.
There was a problem hiding this comment.
Fixed in 1d499f8. Tightened summarize_params per-tool branches in crates/ironclaw_engine/src/types/event.rs:
http/web_fetch:strip_url_sensitive_partsdrops the query string, fragment, anduser[:pass]@userinfo before truncation, so signed URLs / query-string API keys / embedded credentials no longer reachToolCompleted.parameters.shell:redact_shell_command_for_displaymasks values of-H/--header/-u/--user/--token/--api-key/--password/--auth/--bearer(quoted or unquoted), masksAuthorization:/X-Api-Key:/X-Auth-Token:/Bearer:values inside any argument, and strips query strings from any URL embedded in the command.
Kept this in the engine crate (no new dependency on ironclaw_safety) since the leak shape is specific to how these known-tool summaries are constructed. Regression tests: summarize_params_http_strips_query_string_and_userinfo, summarize_params_http_preserves_non_url_strings, summarize_params_shell_redacts_auth_headers_and_query_strings. The generic MCP-key filter from aae97be still covers the unknown-tool path.
| serde_json::Value::Null => None, | ||
| other => Some(other.clone()), | ||
| }; | ||
| let code_executed_event = ThreadEvent::new( |
There was a problem hiding this comment.
Medium Severity
CodeExecuted now persists and broadcasts raw CodeAct code, stdout, and return_value, but unlike normal external tool output this path never goes through the safety/leak-detection layer before reaching SSE. A snippet that prints a secret, echoes sensitive tool output, or returns a credential-shaped JSON blob will therefore surface that raw material to every verbose subscriber and keep it in the thread event log as well.
Given the IronClaw convention that external output should cross a safety boundary before leaving the runtime, I think this event either needs redaction/scrubbing before emission or a narrower payload that can't carry arbitrary sensitive output verbatim.
There was a problem hiding this comment.
Fixed in 1d499f8. AppEvent::CodeExecuted now passes through the leak detector at the bridge boundary before reaching SSE. Added SafetyLayer::leak_detector() and EffectBridgeAdapter::safety() accessors, then redact_code_executed_secrets in src/bridge/router.rs::await_thread_outcome scrubs code, stdout, and return_value (recursively for nested JSON) via a new redact_leaks_in_string helper.
One important subtlety: LeakDetector::scan_and_clean / scan().redacted_content only redact Redact-action matches, leaving Block-action matches (OpenAI/Anthropic/GitHub/AWS/Stripe/NEAR patterns — the majority of the default set) untouched. For a verbose-only observability event, keeping any leak-carrying field verbatim is never appropriate, so the helper iterates scan().matches directly and redacts every match range regardless of action. Regression test redact_code_executed_scrubs_secrets_from_code_stdout_and_return_value covers the code / stdout / nested-JSON-return-value cases with a GitHub-token pattern. Keeping the engine crate safety-agnostic (no ironclaw_safety dep) — scrubbing stays at the adapter boundary where the rest of the safety pipeline already lives.
…ty-trace # Conflicts: # crates/ironclaw_common/src/event.rs # crates/ironclaw_gateway/static/debug-panel.js # src/bridge/router.rs
…scoping - Warning bridge (`src/channels/web/mod.rs`): disable entirely in multi-tenant mode. The `tracing` layer captures log context at the global subscriber scope, not at request scope, so scoping the bridge to the gateway `owner_id` misroutes tenant A's WARN/ERROR log lines to the admin account (and prevents tenant A from ever seeing them). Per-request provenance would need threading through every `warn!` / `error!` call site — out of scope for this PR — so the safe move is to keep the bridge off until that lands. - `summarize_params` (`crates/ironclaw_engine/src/types/event.rs`): strip URL query strings / fragments / userinfo for `http` and `web_fetch`, and redact auth-bearing flag values (`-H`, `--header`, `-u`, `--user`, `--token`, `--api-key`, `--password`, `--auth`, `--bearer`) plus embedded URL query strings inside `shell` commands. Signed URLs, inline `Authorization: Bearer …` headers, and query- string API keys no longer reach `ToolCompleted.parameters` on the debug SSE stream. Six regression tests added. - `CodeExecuted` redaction (`src/bridge/router.rs`): apply the leak detector to `code` / `stdout` / `return_value` at the bridge boundary before SSE broadcast. The engine crate has no dependency on `ironclaw_safety`, so scrubbing lives here. Adds `SafetyLayer::leak_detector()` and `EffectBridgeAdapter::safety()` accessors. Handles both `Redact` and `Block`-action matches (scan_and_clean's `redacted_content` is `None` for Block-only matches, which would have passed bearer tokens / API keys through unchanged). Regression test covers string and nested-JSON cases.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.state.multi_tenant_mode { | ||
| tracing::debug!( | ||
| "warning bridge disabled in multi-tenant mode: \ | ||
| WARN/ERROR log forwarding to debug panel requires \ | ||
| per-request tenant provenance that is not yet \ | ||
| wired through the tracing layer" | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
The PR description says the warning bridge is tenant-scoped in multi-tenant mode, but this code disables the bridge entirely when multi_tenant_mode is true (and therefore no AppEvent::Warning will ever reach the debug panel in that configuration). Please either update the PR description/test plan to match this behavior, or implement the described tenant-scoped forwarding once per-request provenance is available.
There was a problem hiding this comment.
Fixed — PR description updated to state the warning bridge is disabled (not tenant-scoped) in multi-tenant mode. Both the Summary bullet and the Test plan's multi-tenant check now reflect the actual behavior shipped in 1d499f8. Re-enabling the bridge in multi-tenant deployments is blocked on per-request provenance being threaded through every warn!/error! call site, which is outside this PR's scope.
| Some(uid) => sse.broadcast_for_user(uid, event), | ||
| None => sse.broadcast(event), |
There was a problem hiding this comment.
New sse.broadcast* call sites are subject to the repo’s PROJECTION safety check (scripts/pre-commit-safety.sh). These added broadcast_for_user / broadcast lines need an inline // projection-exempt: <category>, <detail> annotation (or be routed through the projection bridge) to avoid failing the safety gate.
| Some(uid) => sse.broadcast_for_user(uid, event), | |
| None => sse.broadcast(event), | |
| Some(uid) => sse.broadcast_for_user(uid, event), // projection-exempt: logs, scrubbed warning event for verbose SSE delivery | |
| None => sse.broadcast(event), // projection-exempt: logs, scrubbed warning event for verbose SSE delivery |
There was a problem hiding this comment.
Fixed in 9f43803. Both sse.broadcast_for_user and sse.broadcast sites in spawn_warning_bridge now carry // projection-exempt: log source, WARN/ERROR tracing bridge → AppEvent::Warning annotations — the warning bridge is a typed source log in the sense of .claude/rules/gateway-events.md (every AppEvent::Warning on SSE projects from exactly one LogEntry produced by WebLogLayer), but that source log isn't yet in the rule's fixed table of three. Added a comment block above the match explaining why the annotation is there. Pre-commit safety script now passes clean on this branch.
| // Cap code and stdout at `CODE_EXECUTED_MAX_CHARS` each before | ||
| // emission so a step that prints a large blob cannot bloat | ||
| // persisted thread events or flood the SSE broadcast buffer. | ||
| // Same cap as `scripting::OUTPUT_TRUNCATE_LEN` (8_000). | ||
| const CODE_EXECUTED_MAX_CHARS: usize = 8_000; | ||
| let return_value = match &result.return_value { | ||
| serde_json::Value::Null => None, | ||
| other => Some(other.clone()), | ||
| }; | ||
| let code_executed_event = ThreadEvent::new( | ||
| thread.id, | ||
| EventKind::CodeExecuted { | ||
| step_id: exec_ctx.step_id, | ||
| code: tail_chars(&code, CODE_EXECUTED_MAX_CHARS), | ||
| stdout: tail_chars(&result.stdout, CODE_EXECUTED_MAX_CHARS), |
There was a problem hiding this comment.
CodeExecuted is intended to be size-bounded, but tail_chars currently walks the entire string (chars().count()), so very large code/stdout payloads still incur O(n) work. Also return_value is cloned without any size cap, so it can still bloat persisted events/SSE frames. Consider truncating from the end using byte indices + UTF-8 boundary checks, and applying a similar size limit (or omission) for return_value.
| // Cap code and stdout at `CODE_EXECUTED_MAX_CHARS` each before | |
| // emission so a step that prints a large blob cannot bloat | |
| // persisted thread events or flood the SSE broadcast buffer. | |
| // Same cap as `scripting::OUTPUT_TRUNCATE_LEN` (8_000). | |
| const CODE_EXECUTED_MAX_CHARS: usize = 8_000; | |
| let return_value = match &result.return_value { | |
| serde_json::Value::Null => None, | |
| other => Some(other.clone()), | |
| }; | |
| let code_executed_event = ThreadEvent::new( | |
| thread.id, | |
| EventKind::CodeExecuted { | |
| step_id: exec_ctx.step_id, | |
| code: tail_chars(&code, CODE_EXECUTED_MAX_CHARS), | |
| stdout: tail_chars(&result.stdout, CODE_EXECUTED_MAX_CHARS), | |
| // Cap code/stdout and return_value before emission so a step that | |
| // produces a large payload cannot bloat persisted thread events or | |
| // flood the SSE broadcast buffer. | |
| // Same cap as `scripting::OUTPUT_TRUNCATE_LEN` (8_000). | |
| fn tail_utf8_bytes(s: &str, max_bytes: usize) -> String { | |
| if s.len() <= max_bytes { | |
| return s.to_owned(); | |
| } | |
| let mut start = s.len() - max_bytes; | |
| while start < s.len() && !s.is_char_boundary(start) { | |
| start += 1; | |
| } | |
| s[start..].to_owned() | |
| } | |
| fn bounded_return_value( | |
| value: &serde_json::Value, | |
| max_bytes: usize, | |
| ) -> Option<serde_json::Value> { | |
| match value { | |
| serde_json::Value::Null => None, | |
| serde_json::Value::String(s) => { | |
| Some(serde_json::Value::String(tail_utf8_bytes(s, max_bytes))) | |
| } | |
| other => match serde_json::to_vec(other) { | |
| Ok(buf) if buf.len() <= max_bytes => Some(other.clone()), | |
| _ => None, | |
| }, | |
| } | |
| } | |
| const CODE_EXECUTED_MAX_BYTES: usize = 8_000; | |
| let return_value = | |
| bounded_return_value(&result.return_value, CODE_EXECUTED_MAX_BYTES); | |
| let code_executed_event = ThreadEvent::new( | |
| thread.id, | |
| EventKind::CodeExecuted { | |
| step_id: exec_ctx.step_id, | |
| code: tail_utf8_bytes(&code, CODE_EXECUTED_MAX_BYTES), | |
| stdout: tail_utf8_bytes(&result.stdout, CODE_EXECUTED_MAX_BYTES), |
There was a problem hiding this comment.
Fixed in 9f43803. At the CodeExecuted emission site, tail_chars is replaced with:
tail_utf8_bytes(s, max_bytes)— byte-based, O(1) start + ≤3-byte UTF-8 boundary walk. Used forcodeandstdout.bounded_return_value(v, max_bytes)—Null⇒None,String⇒ tail-truncated, structuredArray/Object/Number/Boolserialized and either preserved intact if≤ max_bytesor dropped toNone. Truncating arbitrary JSON would be unparseable on the frontend so dropping is the conservative choice — observers seeNoneand know the payload was omitted for size.
tail_chars is kept for its other callers (OUTPUT_TRUNCATE_LEN / 500-char error slices) whose inputs are already pre-bounded and where chars-vs-bytes semantics matter for character-count messaging. Seven regression tests added covering ASCII, multi-byte emoji boundary, Null, small struct, oversized struct, and large-string paths.
Code Review — PR #2850
|
- `src/channels/web/log_layer.rs`: annotate `spawn_warning_bridge`'s `sse.broadcast_for_user` / `sse.broadcast` sites with `// projection-exempt: log source, WARN/ERROR tracing bridge → AppEvent::Warning` so the PROJECTION safety check (#9 in `scripts/pre-commit-safety.sh`) recognises the tracing `LogBroadcaster` as a typed source log. Added a comment block explaining why the source-log category isn't yet in `.claude/rules/gateway-events.md`'s table. - `crates/ironclaw_engine/src/executor/orchestrator.rs`: replace `tail_chars` (O(n) via `chars().count()`) with a local `tail_utf8_bytes` helper for the `CodeExecuted` emission path. Byte based so it stays O(1) + ≤3-byte UTF-8 boundary walk for arbitrarily large `code`/`stdout`. Also add `bounded_return_value` so a CodeAct snippet returning a 50 MB JSON value doesn't bloat persisted thread events — strings are tail-truncated; structured values that serialize past 8 KiB are dropped to `None` (rather than truncated into unparseable JSON). Seven regression tests cover ASCII / emoji boundary / null / small struct / oversized struct / large-string paths. `tail_chars` is kept unchanged for its existing callers, whose inputs are already bounded (`OUTPUT_TRUNCATE_LEN`, 500-char error slices).
…gs (nearai#2850) * feat(debug-panel): expand Activity tab coverage with CodeAct + warnings The Activity tab was missing most event types: CodeAct runs showed only lossy chat summaries, WARN/ERROR logs only landed in server stdout, and tool entries hid their parameters on success. - Emit AppEvent::CodeExecuted (verbose-only) with raw code, stdout, and return value from the engine orchestrator so observers see what the model actually wrote. - Bridge WARN/ERROR tracing into AppEvent::Warning via spawn_warning_bridge, scoped by owner_id in multi-tenant mode to prevent cross-tenant log bleed. - Backfill params_summary on ActionExecuted/ActionFailed events from structured + scripting executors so the Activity tab shows tool args immediately (not just on failure) without waiting for tool_completed. - Wire debug-panel.js to render code_executed, warning, gate_required, gate_resolved, approval_needed, skill_activated, plan_update, thread_state_changed, child/mission_thread_spawned, onboarding_state, image_generated, suggestions, and the full sandbox-job event family. - Extract shared on(name, handler) wrapper to dedupe ~25 copies of the JSON-parse + reconnect-counter housekeeping and keep lastEventTime bookkeeping consistent across listeners. - Add i18n strings (en/ko/zh-CN) and CSS icon colors for the new activity types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(debug-panel): address review feedback on activity-trace PR - summarize_params generic fallback: skip sensitive-looking parameter keys (token/secret/password/api_key/auth/credential/bearer) so MCP and unknown-tool calls can't surface secret values into ActionExecuted events or debug-panel SSE. Adds two regression tests. - Cap CodeExecuted code/stdout at 8_000 chars (tail-last) before emission so a step that prints a large blob can't bloat persisted thread events. Matches the existing scripting OUTPUT_TRUNCATE_LEN. - await_thread_outcome: skip broadcasting verbose-only AppEvents when no debug subscriber is connected — mirrors the send_status gate and keeps CodeExecuted off the shared SSE broadcast buffer for normal browser tabs. - spawn_warning_bridge: same short-circuit on has_verbose_receivers. - debug-panel.js: introduce GATE_RESOLUTION_STATUS so `expired` (a failure path from router.rs) no longer renders as a green success badge; shared STATUS_TO_ACTIVITY map is kept for jobs/ plans/onboarding where `success` is the right default. - debug-panel.js: migrate the remaining legacy listeners to the shared on() wrapper so lastEventTime / totalEventsReceived bookkeeping stays consistent across every activity listener. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(debug-panel): address PR nearai#2850 follow-up review on leak / tenant scoping - Warning bridge (`src/channels/web/mod.rs`): disable entirely in multi-tenant mode. The `tracing` layer captures log context at the global subscriber scope, not at request scope, so scoping the bridge to the gateway `owner_id` misroutes tenant A's WARN/ERROR log lines to the admin account (and prevents tenant A from ever seeing them). Per-request provenance would need threading through every `warn!` / `error!` call site — out of scope for this PR — so the safe move is to keep the bridge off until that lands. - `summarize_params` (`crates/ironclaw_engine/src/types/event.rs`): strip URL query strings / fragments / userinfo for `http` and `web_fetch`, and redact auth-bearing flag values (`-H`, `--header`, `-u`, `--user`, `--token`, `--api-key`, `--password`, `--auth`, `--bearer`) plus embedded URL query strings inside `shell` commands. Signed URLs, inline `Authorization: Bearer …` headers, and query- string API keys no longer reach `ToolCompleted.parameters` on the debug SSE stream. Six regression tests added. - `CodeExecuted` redaction (`src/bridge/router.rs`): apply the leak detector to `code` / `stdout` / `return_value` at the bridge boundary before SSE broadcast. The engine crate has no dependency on `ironclaw_safety`, so scrubbing lives here. Adds `SafetyLayer::leak_detector()` and `EffectBridgeAdapter::safety()` accessors. Handles both `Redact` and `Block`-action matches (scan_and_clean's `redacted_content` is `None` for Block-only matches, which would have passed bearer tokens / API keys through unchanged). Regression test covers string and nested-JSON cases. * fix(debug-panel): address PR nearai#2850 Copilot follow-up review - `src/channels/web/log_layer.rs`: annotate `spawn_warning_bridge`'s `sse.broadcast_for_user` / `sse.broadcast` sites with `// projection-exempt: log source, WARN/ERROR tracing bridge → AppEvent::Warning` so the PROJECTION safety check (#9 in `scripts/pre-commit-safety.sh`) recognises the tracing `LogBroadcaster` as a typed source log. Added a comment block explaining why the source-log category isn't yet in `.claude/rules/gateway-events.md`'s table. - `crates/ironclaw_engine/src/executor/orchestrator.rs`: replace `tail_chars` (O(n) via `chars().count()`) with a local `tail_utf8_bytes` helper for the `CodeExecuted` emission path. Byte based so it stays O(1) + ≤3-byte UTF-8 boundary walk for arbitrarily large `code`/`stdout`. Also add `bounded_return_value` so a CodeAct snippet returning a 50 MB JSON value doesn't bloat persisted thread events — strings are tail-truncated; structured values that serialize past 8 KiB are dropped to `None` (rather than truncated into unparseable JSON). Seven regression tests cover ASCII / emoji boundary / null / small struct / oversized struct / large-string paths. `tail_chars` is kept unchanged for its existing callers, whose inputs are already bounded (`OUTPUT_TRUNCATE_LEN`, 500-char error slices). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
AppEvent::CodeExecuted(verbose-only) andAppEvent::Warningso the debug panel's Activity tab can surface raw CodeAct code/stdout and WARN/ERROR logs that were previously only visible in server stdout.params_summaryonActionExecuted/ActionFailedevents from the structured and scripting executors so tool args are visible immediately, even on success.summarize_paramshardens thehttp/web_fetch/shellbranches and the generic MCP fallback so query-string secrets, signed-URL tokens, inlineAuthorization:headers, and sensitively-keyed MCP fields are stripped before they reachToolCompleted.parameters.AppEvent::CodeExecutedcode / stdout / return_value throughLeakDetectorat the bridge boundary before SSE broadcast, so a model-authored Python snippet that prints a bearer token or returns a credential-shaped JSON blob does not reach verbose subscribers verbatim. Covers bothLeakAction::RedactandLeakAction::Blockpatterns (the latter were previously surfaced byscan()without redacting).code/stdouttail-truncated to 8 KiB via a UTF-8-boundary-safe helper;return_valuestrings tail-truncated, structured return_values <= 8 KiB preserved and larger ones dropped toNone(truncated JSON would be unparseable on the frontend).on()wrapper that dedupes ~25 copies of the JSON-parse + reconnect-counter housekeeping.multi_tenant_modeistrue. Thetracinglayer captures log context at the global subscriber scope, not at request scope, so scoping the whole bridge to a singleowner_idin multi-tenant mode would deliver tenant A's warnings to the admin/owner account instead of tenant A. Re-enabling the bridge in multi-tenant deployments is gated on per-request provenance being threaded through everywarn!/error!call site — out of scope for this PR. Single-user gateways get the bridge unchanged.Test plan
cargo fmt --checkcargo clippy --all --benches --tests --examples --all-features— cleancargo test --lib— 5474 pass; one pre-existing MCP flake (extensions::manager::tests::inject_mcp_client_partitions_cache_by_user) reproducible on cleanorigin/staging, unrelated to this PRtracing::warn!from a non-verbose code path; confirm it lands in the Activity tab only for verbose subscribers in single-user mode, and does not appear at all whenmulti_tenant_modeis on[REDACTED]/ dropped return value rather than verbatim secrets or bloated payloads🤖 Generated with Claude Code