feat(gateway): surface tool output previews and structured rendering#2571
feat(gateway): surface tool output previews and structured rendering#2571serrrfirat wants to merge 6 commits intostagingfrom
Conversation
…in web UI Tools in the engine v2 path showed "0.0s" duration and no output preview because ActionExecuted events lacked output data and forward_event_to_channel never emitted ToolResult events. Changes: - Add output_preview field to EventKind::ActionExecuted, populated at all tool execution sites (orchestrator, structured, scripting) with 500-char truncated preview - Emit ToolResult from both thread_event_to_app_events (SSE path) and forward_event_to_channel (channel path) when output_preview is present - Frontend: parse server-side duration from "Nms" parameters, show "< 1ms" for sub-millisecond tools instead of "0.0s" - Frontend: detect JSON in tool output — render arrays as tables, objects as key-value pairs instead of raw text - Add AppEvent::MissionCreated with dedicated card rendering (name, status badge, cadence, project ID) following the PlanUpdate pattern - Wire SSE manager into EffectBridgeAdapter for structured event broadcast Closes #2537 Closes #2545 [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix XSS in renderMissionCreatedCard: replace innerHTML with textContent for all user-supplied data (mission_id, cadence, project_id) - Add unit tests for truncate_output_preview: null, empty, short, long, UTF-8 emoji boundaries, CJK character boundaries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new "Approval Panel" in the web UI to manage self-improvement mission proposals and behavior changes. It adds structured event types for mission creation and change resolution, implements a daily reset for mission thread budgets, and enhances tool output display with truncated previews and structured table/key-value rendering. Additionally, it ensures learning missions are initialized for users upon request and improves attachment persistence tracking. Feedback focuses on improving security by avoiding innerHTML for dynamic data, ensuring consistent use of the apiFetch wrapper, and refining the display of object data in tooltips.
| fetch('/api/chat/change/resolve', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': 'Bearer ' + authToken | ||
| }, | ||
| body: JSON.stringify({ request_id: requestId, resolution: resolution }) | ||
| }).then(function(r) { | ||
| if (!r.ok) throw new Error('Failed to resolve change'); |
There was a problem hiding this comment.
The sendChangeResolution function uses the native fetch API directly instead of the apiFetch wrapper used elsewhere in the codebase (e.g., line 2157). apiFetch likely handles base URL configuration, authentication headers, and global error reporting consistently. Bypassing it can lead to maintenance issues and inconsistent behavior if the API configuration changes.
apiFetch('/api/chat/change/resolve', {
method: 'POST',
body: JSON.stringify({ request_id: requestId, resolution: resolution })
}).then(function(r) {| if (data.mission_id) { | ||
| var idRow = document.createElement('div'); | ||
| idRow.className = 'mission-card-detail'; | ||
| var idLabel = document.createElement('span'); |
There was a problem hiding this comment.
Using innerHTML with dynamic data from the server poses a potential Cross-Site Scripting (XSS) risk. While mission_id is expected to be a UUID, it is safer to use textContent for dynamic values to ensure they are treated as plain text and not parsed as HTML.
var idRow = document.createElement('div');
idRow.className = 'mission-card-detail';
var label = document.createElement('span');
label.className = 'mission-detail-label';
label.textContent = 'ID';
var value = document.createElement('span');
value.className = 'mission-detail-value';
value.textContent = data.mission_id.substring(0, 8);
idRow.appendChild(label);
idRow.appendChild(value);
details.appendChild(idRow);| idValue.className = 'mission-detail-value'; | ||
| idValue.textContent = data.mission_id.substring(0, 8); | ||
| idRow.appendChild(idLabel); | ||
| idRow.appendChild(idValue); |
There was a problem hiding this comment.
Using innerHTML with data.cadence is unsafe as it could contain malicious HTML if the server response is compromised or manipulated. Prefer using textContent for the dynamic value.
var cadenceRow = document.createElement('div');
cadenceRow.className = 'mission-card-detail';
var label = document.createElement('span');
label.className = 'mission-detail-label';
label.textContent = I18n.t('missions.cadence');
var value = document.createElement('span');
value.className = 'mission-detail-value';
value.textContent = data.cadence;
cadenceRow.appendChild(label);
cadenceRow.appendChild(value);
details.appendChild(cadenceRow);| // Truncate long cell values | ||
| if (td.textContent.length > 120) { | ||
| td.textContent = td.textContent.substring(0, 117) + '...'; | ||
| td.title = String(arr[r][keys[c]]); |
There was a problem hiding this comment.
When setting the title attribute for a table cell, using String(val) on an object will result in the unhelpful string "[object Object]". Since this code already handles JSON objects for the cell content, it should also stringify them for the tooltip to provide a useful preview of the full data.
| td.title = String(arr[r][keys[c]]); | |
| td.title = (typeof val === 'object' && val !== null) ? JSON.stringify(val) : String(val); |
- Use apiFetch wrapper instead of raw fetch in sendChangeResolution for consistent auth handling and OIDC proxy support - Fix table cell tooltip: use JSON.stringify for object values instead of String() which produces unhelpful "[object Object]" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review feedback addressedAll 4 comments from @gemini-code-assist resolved:
Also added 7 unit tests for |
henrypark133
left a comment
There was a problem hiding this comment.
Review: current head does not compile
The gateway/event UX additions are substantial, but the current head is blocked by a test that does not compile against the checked-in router test harness.
Critical: the new router attachment test references stale test APIs and breaks cargo clippy --all --all-features
File: src/bridge/router.rs:5862
The added handle_with_engine_persists_attachment_files_and_indexes_them test uses symbols and fields that do not exist in this branch: CWD_TEST_LOCK, CurrentDirGuard, EngineState.project_root, and IncomingAttachment.local_path (see the same block through src/bridge/router.rs:5887). CI is failing on this exact test code, so the PR cannot merge in its current form.
Suggested fix: Rewrite the test against the current router test helpers and current IncomingAttachment / EngineState shapes, or drop the stale assertions from this PR.
|
Addressed review feedback:
Note: |
Add the HTML skeleton for the new right-hand approval side panel and tab-bar badge that the JS in this PR references. Without these elements the top-level listener registrations for `approval-badge` and `approval-panel-close` threw TypeError on page load, halting all subsequent init and breaking the web UI. - Add `#approval-badge` button in the tab-bar (hidden until cards queue). - Add `#approval-panel` aside as a sibling of `.chat-container` inside `#tab-chat` with header/body/footer matching the CSS and JS contract (`#approval-panel-count`, `#approval-panel-close`, `#approval-panel-body`, `#approval-badge-count`). - Reuse existing i18n keys added in this PR (`approvalPanel.*`, `approval.pressY`). Resolves findings #1 (Critical), #2 (High), #3 (High) surfaced by the pr-fix-loop review pass.
Summary
output_previewfield toActionExecutedevents, emittingToolResultfrom both the SSE and channel paths.duration_msfrom theparametersfield.AppEvent::MissionCreatedwith dedicated frontend card (name, status badge, cadence, project) following thePlanUpdatepattern.Closes #2537
Closes #2545
Test plan
cargo check— compiles cleancargo test -p ironclaw_common— 23 tests pass (includes newMissionCreatedvariant inevent_type_matches_serde_type_field)mission_create, verify styled card appears instead of raw JSONmission_list, verify table rendering in tool output🤖 Generated with Claude Code