From 7d4269e1ad17633af25c3108ecfc5de520051aca Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 4 Jul 2026 18:33:13 +0000 Subject: [PATCH 1/2] fix(agent): render subagent tool failures + harden memory lock symlinks Follow-up to the #4504 review: - Extract ToolFailureLines into a shared component and render it in the sub-agent renderers (SubagentDrawer rows + ToolTimelineBlock inline rows), so a failed child tool actually SHOWS the why/next copy instead of only storing it on the transcript item (#4459). - update_memory_md: reject a symlinked `.memory-write.lock` and open it O_NOFOLLOW on Unix, so a project-controlled symlink can't redirect the cross-process flock outside the containment-checked workspace (#4458). Claude-Session: https://claude.ai/code/session_019j5TLsRLHsM3kqAYFyH4hR --- .../components/ProcessingTranscriptView.tsx | 55 +----------------- .../components/SubagentDrawer.tsx | 6 ++ .../components/ToolFailureLines.tsx | 56 ++++++++++++++++++ .../components/ToolTimelineBlock.tsx | 57 +++++++++++-------- .../tools/impl/filesystem/update_memory_md.rs | 26 +++++++-- 5 files changed, 117 insertions(+), 83 deletions(-) create mode 100644 app/src/pages/conversations/components/ToolFailureLines.tsx diff --git a/app/src/pages/conversations/components/ProcessingTranscriptView.tsx b/app/src/pages/conversations/components/ProcessingTranscriptView.tsx index 7e303f7e63..321d57ab52 100644 --- a/app/src/pages/conversations/components/ProcessingTranscriptView.tsx +++ b/app/src/pages/conversations/components/ProcessingTranscriptView.tsx @@ -1,7 +1,6 @@ import { useT } from '../../../lib/i18n/I18nContext'; import type { ProcessingTranscriptItem, - ToolFailureExplanation, ToolTimelineEntry, ToolTimelineEntryStatus, } from '../../../store/chatRuntimeSlice'; @@ -12,6 +11,7 @@ import { stripToolCallEnvelopes, type ToolCategory, } from '../../../utils/toolTimelineFormatting'; +import { ToolFailureLines } from './ToolFailureLines'; /** * The Hermes-style "View processing" body: the agent's narration and hidden @@ -113,29 +113,6 @@ function ToolGroupBlock({ summary, entries }: { summary: string; entries: ToolTi ); } -/** - * The failure classes the UI has localized copy for (#4254 / #4459), keyed by the - * camelCase form of the wire's PascalCase `class`. Any class not in this set - * falls back to the English `causePlain` / `nextAction` carried on the payload. - */ -const LOCALIZED_FAILURE_CLASSES: ReadonlySet = new Set([ - 'missingPermission', - 'missingApp', - 'serviceUnavailable', - 'badCredentials', - 'blockedByPolicy', - 'modelConnection', - 'timeout', - 'denied', - 'approvalExpired', - 'unknown', -]); - -/** Lowercase the first character: `MissingPermission` → `missingPermission`. */ -function toCamelClass(cls: string): string { - return cls.length > 0 ? cls[0].toLowerCase() + cls.slice(1) : cls; -} - /** One tool step: type icon + human sentence + contextual detail chip. */ function ToolRow({ entry }: { entry: ToolTimelineEntry }) { const { title, detail } = formatTimelineEntry(entry); @@ -159,36 +136,6 @@ function ToolRow({ entry }: { entry: ToolTimelineEntry }) { ); } -/** - * The "why + what to do next" pair rendered under a failed tool row (#4254). - * Copy is resolved by failure class from i18n, falling back to the English - * `causePlain` / `nextAction` carried on the wire when the class is one the UI - * hasn't localized. The failure line is coral to match the error glyph. - */ -function ToolFailureLines({ failure }: { failure: ToolFailureExplanation }) { - const { t } = useT(); - const camel = toCamelClass(failure.class); - const known = LOCALIZED_FAILURE_CLASSES.has(camel); - const cause = known - ? t(`conversations.toolFailure.${camel}.cause`, failure.causePlain) - : failure.causePlain; - const next = known - ? t(`conversations.toolFailure.${camel}.next`, failure.nextAction) - : failure.nextAction; - return ( - - - {t('conversations.toolFailure.whyLabel')}: {cause} - - - {t('conversations.toolFailure.nextLabel')}: {next} - - - ); -} - /** Compact terminal status glyph for the group's "Done" line. */ function StatusGlyph({ status }: { status: ToolTimelineEntryStatus }) { if (status === 'error') { diff --git a/app/src/pages/conversations/components/SubagentDrawer.tsx b/app/src/pages/conversations/components/SubagentDrawer.tsx index 698311e7ee..c1c9cac8b1 100644 --- a/app/src/pages/conversations/components/SubagentDrawer.tsx +++ b/app/src/pages/conversations/components/SubagentDrawer.tsx @@ -11,6 +11,7 @@ import type { import type { ThreadMessage } from '../../../types/thread'; import { stripToolCallEnvelopes } from '../../../utils/toolTimelineFormatting'; import { BubbleMarkdown } from './AgentMessageBubble'; +import { ToolFailureLines } from './ToolFailureLines'; /** * Rebuild a renderable transcript from a worker sub-thread's persisted @@ -480,6 +481,11 @@ function ToolCallRow({ item }: { item: SubagentToolItem }) { {formatElapsed(item.elapsedMs)} ) : null} + {item.status === 'error' && item.failure ? ( +
+ +
+ ) : null} {expandable && expanded ? (
{argsText != null ? ( diff --git a/app/src/pages/conversations/components/ToolFailureLines.tsx b/app/src/pages/conversations/components/ToolFailureLines.tsx new file mode 100644 index 0000000000..5ac2de97f9 --- /dev/null +++ b/app/src/pages/conversations/components/ToolFailureLines.tsx @@ -0,0 +1,56 @@ +import { useT } from '../../../lib/i18n/I18nContext'; +import type { ToolFailureExplanation } from '../../../store/chatRuntimeSlice'; + +/** + * The failure classes the UI has localized copy for (#4254 / #4459), keyed by + * the camelCase form of the wire's PascalCase `class`. Any class not in this + * set falls back to the English `causePlain` / `nextAction` on the payload. + */ +const LOCALIZED_FAILURE_CLASSES: ReadonlySet = new Set([ + 'missingPermission', + 'missingApp', + 'serviceUnavailable', + 'badCredentials', + 'blockedByPolicy', + 'modelConnection', + 'timeout', + 'denied', + 'approvalExpired', + 'unknown', +]); + +/** Lowercase the first character: `MissingPermission` → `missingPermission`. */ +function toCamelClass(cls: string): string { + return cls.length > 0 ? cls[0].toLowerCase() + cls.slice(1) : cls; +} + +/** + * The "why + what to do next" pair rendered under a failed tool row (#4254 / + * #4459). Copy resolves by failure class from i18n, falling back to the English + * `causePlain` / `nextAction` carried on the wire when the class is one the UI + * hasn't localized. Shared by the parent processing transcript and the + * sub-agent renderers so a failed child tool shows the same why/next copy. + */ +export function ToolFailureLines({ failure }: { failure: ToolFailureExplanation }) { + const { t } = useT(); + const camel = toCamelClass(failure.class); + const known = LOCALIZED_FAILURE_CLASSES.has(camel); + const cause = known + ? t(`conversations.toolFailure.${camel}.cause`, failure.causePlain) + : failure.causePlain; + const next = known + ? t(`conversations.toolFailure.${camel}.next`, failure.nextAction) + : failure.nextAction; + return ( + + + {t('conversations.toolFailure.whyLabel')}: {cause} + + + {t('conversations.toolFailure.nextLabel')}: {next} + + + ); +} diff --git a/app/src/pages/conversations/components/ToolTimelineBlock.tsx b/app/src/pages/conversations/components/ToolTimelineBlock.tsx index e4545c9252..8ed00fa8ad 100644 --- a/app/src/pages/conversations/components/ToolTimelineBlock.tsx +++ b/app/src/pages/conversations/components/ToolTimelineBlock.tsx @@ -2,6 +2,7 @@ import WorktreeActions from '../../../components/worktree/WorktreeActions'; import { useT } from '../../../lib/i18n/I18nContext'; import type { SubagentActivity, + ToolFailureExplanation, ToolTimelineEntry, ToolTimelineEntryStatus, } from '../../../store/chatRuntimeSlice'; @@ -14,6 +15,7 @@ import { import { parseWorkerThreadRef } from '../utils/workerThreadRef'; import { BubbleMarkdown } from './AgentMessageBubble'; import { agentNameTone, AgentTimelineRail } from './AgentTimelineRail'; +import { ToolFailureLines } from './ToolFailureLines'; import { WorkerThreadRefCard, type WorkerThreadStatus } from './WorkerThreadRefCard'; /** @@ -96,39 +98,44 @@ function ToolCallRow({ displayName?: string; /** Server-computed contextual detail (path / recipient / query). */ detail?: string; + /** Structured why/next explanation for a FAILED child tool call (#4459). */ + failure?: ToolFailureExplanation; }; }) { const tone = toolCallTone(call.status); return ( -
- - • - - - {call.displayName ?? formatToolName(call.toolName)} - - {/* The contextual arg (path / recipient / query) can be long, so it +
+
+ + • + + + {call.displayName ?? formatToolName(call.toolName)} + + {/* The contextual arg (path / recipient / query) can be long, so it truncates to a single line and absorbs the row's spare width — the full value stays available on hover — instead of wrapping into a multi-line box that knocks the name and status out of alignment. */} - {call.detail ? ( - - {call.detail} - - ) : null} - {/* Status reads as a tinted "Done" / "Failed" / "Running" tag. */} - - - - {call.elapsedMs != null && call.status !== 'running' ? ( - - {call.elapsedMs >= 1000 - ? `${(call.elapsedMs / 1000).toFixed(1)}s` - : `${call.elapsedMs}ms`} + {call.detail ? ( + + {call.detail} + + ) : null} + {/* Status reads as a tinted "Done" / "Failed" / "Running" tag. */} + + - ) : null} + {call.elapsedMs != null && call.status !== 'running' ? ( + + {call.elapsedMs >= 1000 + ? `${(call.elapsedMs / 1000).toFixed(1)}s` + : `${call.elapsedMs}ms`} + + ) : null} +
+ {call.status === 'error' && call.failure ? : null}
); } diff --git a/src/openhuman/tools/impl/filesystem/update_memory_md.rs b/src/openhuman/tools/impl/filesystem/update_memory_md.rs index 098d000318..c26685342c 100644 --- a/src/openhuman/tools/impl/filesystem/update_memory_md.rs +++ b/src/openhuman/tools/impl/filesystem/update_memory_md.rs @@ -61,10 +61,28 @@ async fn acquire_cross_process_write_lock(workspace_dir: &Path) -> anyhow::Resul if let Some(parent) = lock_path.parent() { let _ = std::fs::create_dir_all(parent); } - let file = std::fs::OpenOptions::new() - .create(true) - .write(true) - .truncate(false) + // Reject a symlinked lock file: a project-controlled `.memory-write.lock` + // symlink (including a dangling one) could otherwise redirect this + // create/open/lock to a path OUTSIDE the already-containment-checked + // workspace, bypassing the symlink hardening applied to MEMORY.md / + // SKILL.md. If it exists it must be a regular file. + if let Ok(meta) = std::fs::symlink_metadata(&lock_path) { + if meta.file_type().is_symlink() { + return Err(anyhow::anyhow!( + "workspace lock file {lock_path:?} is a symlink; refusing to follow it" + )); + } + } + let mut opts = std::fs::OpenOptions::new(); + opts.create(true).write(true).truncate(false); + #[cfg(unix)] + { + // O_NOFOLLOW closes the TOCTOU window: if a symlink is swapped in + // after the check above, the open fails (ELOOP) rather than follows. + use std::os::unix::fs::OpenOptionsExt; + opts.custom_flags(libc::O_NOFOLLOW); + } + let file = opts .open(&lock_path) .map_err(|e| anyhow::anyhow!("open workspace lock file {lock_path:?}: {e}"))?; file.lock_exclusive() From 8ce421dbacf056cfd9f33df7bb89ceaed3d68f9e Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 4 Jul 2026 18:35:50 +0000 Subject: [PATCH 2/2] test(agent): assert subagent failed tool renders why/next copy (#4459) --- .../__tests__/SubagentDrawer.test.tsx | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/app/src/pages/conversations/components/__tests__/SubagentDrawer.test.tsx b/app/src/pages/conversations/components/__tests__/SubagentDrawer.test.tsx index c5ecdc8f04..e74f9a25ac 100644 --- a/app/src/pages/conversations/components/__tests__/SubagentDrawer.test.tsx +++ b/app/src/pages/conversations/components/__tests__/SubagentDrawer.test.tsx @@ -65,6 +65,37 @@ describe('SubagentDrawer', () => { expect(texts[1].textContent).toContain('The answer is'); }); + it('renders the why/next explanation for a failed child tool call (#4459)', () => { + render( + {}} + /> + ); + const failure = screen.getByTestId('processing-tool-failure'); + expect(failure).toHaveTextContent('You declined this action.'); + expect(failure).toHaveTextContent('Ask again if you change your mind.'); + }); + it('opens with the parent delegation prompt as a chat bubble', () => { render(