From 4a4f4857818af8101586153abb35446345492ec6 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 14:29:40 +0100 Subject: [PATCH] feat: mini tool call hardening --- application/agents/tool_executor.py | 95 ++++++++++++++----- .../agents/tools/artifact_generator.py | 5 + .../agents/tools/tool_action_parser.py | 25 +++-- .../src/conversation/ConversationBubble.tsx | 36 +------ .../MarkdownAnswer.sandbox.test.tsx | 91 ++++++++++++++++++ frontend/src/conversation/MarkdownAnswer.tsx | 71 ++++++++++---- .../src/conversation/artifactChips.test.ts | 59 ++++++++---- frontend/src/conversation/artifactChips.ts | 55 +++++++++++ .../src/conversation/sandboxLinks.test.ts | 75 +++++++++++++++ frontend/src/conversation/sandboxLinks.ts | 64 +++++++++++-- tests/agents/test_tool_action_parser.py | 47 +++++++++ ...st_tool_executor_duplicate_registration.py | 79 +++++++++++++++ .../test_tool_executor_hallucinated_calls.py | 69 ++++++++++++++ 13 files changed, 653 insertions(+), 118 deletions(-) create mode 100644 frontend/src/conversation/artifactChips.ts create mode 100644 tests/agents/test_tool_executor_duplicate_registration.py diff --git a/application/agents/tool_executor.py b/application/agents/tool_executor.py index 4af19063..477cc8cf 100644 --- a/application/agents/tool_executor.py +++ b/application/agents/tool_executor.py @@ -10,6 +10,7 @@ from sqlalchemy.exc import IntegrityError from application.agents.default_tools import ( BUILTIN_AGENT_TOOLS, is_headless_excluded_tool, + is_synthesized_tool_id, resolve_tool_by_id, synthesized_default_tools, ) @@ -56,6 +57,18 @@ def _dedupable_tool_names() -> frozenset: return frozenset(BUILTIN_AGENT_TOOLS) | frozenset(getattr(settings, "DEFAULT_CHAT_TOOLS", None) or []) +def _requires_approval(tool: Dict, action: Dict) -> bool: + """Effective approval gate for one action of a tool row. + + Both sources live on the row: the cached ``actions[].require_approval`` + snapshot and the deployment-level ``config.require_approval`` that + ``code_executor`` reads. + """ + if bool(action.get("require_approval")): + return True + return bool((tool.get("config") or {}).get("require_approval")) + + def _sanitize_tool_prefix(tool_name: Optional[str]) -> str: """Reduce a tool name to characters allowed in function-call names.""" return re.sub(r"[^a-zA-Z0-9_-]+", "_", str(tool_name or "")).strip("_") @@ -385,8 +398,10 @@ class ToolExecutor: self.attachments: List[Dict] = [] self.client_tools: Optional[List[Dict]] = None self._name_to_tool: Dict[str, Tuple[str, str]] = {} - # Signatures of unresolvable calls that already failed this turn, so an - # invented tool name cannot be retried until MAX_TOOL_ITERATIONS. + # Per-signature failure counts for invented tool names this turn. After + # ``UNRESOLVABLE_CALL_LIMIT`` the model is handed a directive message + # instead of the generic error; this is a prompt-level nudge, not a hard + # bound — the loop bound remains ``MAX_TOOL_ITERATIONS``. self._unresolvable_calls: Dict[str, int] = {} self._tool_to_name: Dict[Tuple[str, str], str] = {} # Filled by the LLMHandler.handle_tool_calls headless loop. @@ -549,15 +564,23 @@ class ToolExecutor: # copies with mangled names (``artifact_generator_create_artifact`` + # ``…_1``). The two rows are NOT byte-identical — a stored row has been # through ``transform_actions``, which stamps ``active``/``filled_by_llm`` - # onto every action — so the key is (tool name, action name) and the - # first occurrence wins, which is the stored row. Safe only for - # builtins: they carry no per-row config - # (``get_config_requirements() == {}``), while two MCP rows can - # legitimately share a name and must stay distinct. - seen_actions: set = set() + # onto every action — so the key is (tool name, action name). + # + # Which copy survives is load-bearing: a stored builtin row CAN carry + # per-row config even though ``get_config_requirements() == {}``, and + # ``code_executor`` keeps its approval gate there. Insertion order is + # the agent's ``tool_ids`` click order on the agent paths, so it must + # not decide this — stored rows are considered before synthesized ones, + # and a row that requires approval always beats one that does not. Safe + # only for builtins; two MCP rows can legitimately share a name and must + # stay distinct. + seen_actions: Dict[Tuple[str, str, bool], Tuple[int, bool]] = {} dedupable = _dedupable_tool_names() - for tool_id, tool in tools_dict.items(): + # Stable sort: preserves the caller's order within each group. + ordered_tools = sorted(tools_dict.items(), key=lambda kv: is_synthesized_tool_id(kv[0])) + + for tool_id, tool in ordered_tools: is_api = tool["name"] == "api_tool" is_client = tool.get("client_side", False) @@ -574,13 +597,32 @@ class ToolExecutor: tool_name = tool.get("name", "") if tool_name in dedupable: fingerprint = (tool_name, action["name"], is_client) - if fingerprint in seen_actions: - logger.debug( - "duplicate_tool_registration_collapsed", - extra={"tool_name": tool_name, "action_name": action["name"]}, - ) + requires_approval = _requires_approval(tool, action) + kept = seen_actions.get(fingerprint) + if kept is not None: + kept_index, kept_approval = kept + if requires_approval and not kept_approval: + # Never let a duplicate registration drop an + # approval gate the user configured. + entries[kept_index] = ( + tool_id, + tool_name, + action["name"], + action, + is_client, + ) + seen_actions[fingerprint] = (kept_index, True) + logger.debug( + "duplicate_tool_registration_promoted", + extra={"tool_name": tool_name, "action_name": action["name"]}, + ) + else: + logger.debug( + "duplicate_tool_registration_collapsed", + extra={"tool_name": tool_name, "action_name": action["name"]}, + ) continue - seen_actions.add(fingerprint) + seen_actions[fingerprint] = (len(entries), requires_approval) entries.append((tool_id, tool_name, action["name"], action, is_client)) name_counts[action["name"]] += 1 @@ -845,7 +887,7 @@ class ToolExecutor: ) return True - def _available_tool_names(self, tools_dict: Dict) -> str: + def _available_tool_names(self, tools_dict: Dict, exclude: Optional[str] = None) -> str: """Render the names the model can actually call, for a correctable error. Prefer the LLM-visible action names assigned by @@ -859,7 +901,8 @@ class ToolExecutor: names = sorted( {(tool or {}).get("name") or tool_id for tool_id, tool in (tools_dict or {}).items()} ) - return ", ".join(str(name) for name in names) if names else "(none available)" + names = [str(name) for name in names if name != exclude] + return ", ".join(names) if names else "(none available)" @staticmethod def _call_signature(llm_name: str, call_args: Any) -> str: @@ -883,16 +926,23 @@ class ToolExecutor: unresolvable = tool_id is None or action_name is None or tool_id not in tools_dict # A tool the model invented will never resolve, so re-running it just - # burns the turn's iteration budget on an identical failure. Answer the - # third attempt from here, without touching the tool layer. - if unresolvable: - signature = self._call_signature(llm_name, call_args) + # burns the turn's iteration budget on an identical failure. From the + # third attempt on, hand back a directive message instead of the generic + # error. Scoped to an unknown *name*: a registered tool whose arguments + # merely failed to decode is recoverable, and refusing it would strand a + # working tool for the rest of the turn. Keyed on the raw payload, so two + # different malformed bodies stay two different signatures. + if unresolvable and llm_name not in self._name_to_tool: + signature = self._call_signature(llm_name, getattr(call, "arguments", None)) failures = self._unresolvable_calls.get(signature, 0) + # Count before refusing, so the message and the log escalate rather + # than freezing at the limit for the rest of the turn. + self._unresolvable_calls[signature] = failures + 1 if failures >= self.UNRESOLVABLE_CALL_LIMIT: repeated = ( f"'{llm_name}' has already failed {failures} times with these arguments and " f"will keep failing. Stop calling it and either use a different tool " - f"({self._available_tool_names(tools_dict)}) or answer without one." + f"({self._available_tool_names(tools_dict, exclude=llm_name)}) or answer without one." ) logger.warning( "tool_call_repeated_failure", @@ -926,7 +976,6 @@ class ToolExecutor: yield {"type": "tool_call", "data": {**tool_call_data, "status": "error"}} self.tool_calls.append(tool_call_data) return repeated, call_id - self._unresolvable_calls[signature] = failures + 1 if tool_id is None or action_name is None: # Say which half actually failed. Reporting a name problem for a diff --git a/application/agents/tools/artifact_generator.py b/application/agents/tools/artifact_generator.py index 6e1264e8..4aee4298 100644 --- a/application/agents/tools/artifact_generator.py +++ b/application/agents/tools/artifact_generator.py @@ -804,6 +804,11 @@ class ArtifactGeneratorTool(Tool): def _render(self, kind: str, spec: Any) -> Dict[str, Any]: """Run the fixed renderer in the sandbox and return the produced file bytes.""" + # ``_validate`` coerces a stringified spec internally but returns only an + # error payload, so validating and rendering the same value is the + # caller's job. Coerce here too: a caller that skipped it would otherwise + # pass validation and then write a JSON *string* to spec.json. + spec = self._coerce_spec(spec) session_id = self._resolve_session_id() if session_id is None: return {"error": "artifact_generator requires a conversation_id or workflow_run_id."} diff --git a/application/agents/tools/tool_action_parser.py b/application/agents/tools/tool_action_parser.py index ee6b94ca..28ce8658 100644 --- a/application/agents/tools/tool_action_parser.py +++ b/application/agents/tools/tool_action_parser.py @@ -1,8 +1,14 @@ import json import logging +from typing import Any logger = logging.getLogger(__name__) +# An ``arguments`` payload that is not decodable JSON at all. Distinct from a +# payload that decodes to a non-object (``[]``, ``5``), which keeps its resolved +# tool name so the executor can report the real type against the real tool. +_MALFORMED_ARGUMENTS: Any = object() + class ToolActionParser: def __init__(self, llm_type, name_mapping=None): @@ -24,7 +30,7 @@ class ToolActionParser: return None @staticmethod - def _decode_arguments(raw): + def _decode_arguments(raw: Any) -> Any: """Decode a tool call's ``arguments`` payload into a dict. A zero-parameter action (``note_view``, ``note_delete``, the todo/ @@ -39,8 +45,8 @@ class ToolActionParser: empty/None for a call that takes no parameters. Returns: - The decoded dict, or ``None`` when the payload is genuinely - malformed. + The decoded value — ``{}`` for an empty payload — or + ``_MALFORMED_ARGUMENTS`` when ``raw`` is not decodable JSON. """ if raw is None: return {} @@ -50,11 +56,10 @@ class ToolActionParser: if not raw.strip(): return {} try: - decoded = json.loads(raw) + return json.loads(raw) except (TypeError, ValueError): - return None - return decoded if isinstance(decoded, dict) else None - return None + return _MALFORMED_ARGUMENTS + return _MALFORMED_ARGUMENTS def _parse_openai_llm(self, call): try: @@ -62,9 +67,9 @@ class ToolActionParser: # treating it as one used to discard the (valid, registered) name # along with it. Only genuinely malformed arguments fail here. call_args = self._decode_arguments(call.arguments) - if call_args is None: + if call_args is _MALFORMED_ARGUMENTS: logger.error( - "Error parsing OpenAI LLM call: arguments are not a JSON object (%s)", + "Error parsing OpenAI LLM call: arguments are not decodable JSON (%s)", getattr(call, "name", ""), ) return None, None, None @@ -104,7 +109,7 @@ class ToolActionParser: # fall back to an empty dict on malformed input so downstream # ``call_args.items()`` doesn't crash the stream. call_args = self._decode_arguments(call.arguments) - if call_args is None: + if not isinstance(call_args, dict): logger.warning( "Google call.arguments was not a JSON object; " "falling back to empty args for %s", diff --git a/frontend/src/conversation/ConversationBubble.tsx b/frontend/src/conversation/ConversationBubble.tsx index 99de4a00..f1afef5f 100644 --- a/frontend/src/conversation/ConversationBubble.tsx +++ b/frontend/src/conversation/ConversationBubble.tsx @@ -30,6 +30,7 @@ import { import { isToolCallRunning } from '../utils/streamingStatusUtils'; import AnswerFlow from './AnswerFlow'; import { AnswerSegment } from './answerSegments'; +import { deriveArtifactChips } from './artifactChips'; import { FEEDBACK, MESSAGE_TYPE, ResearchState } from './conversationModels'; import MarkdownAnswer from './MarkdownAnswer'; import ResearchProgress from './ResearchProgress'; @@ -111,40 +112,7 @@ const ConversationBubble = forwardRef< const editableQueryRef = useRef(null); const [isQuestionCollapsed, setIsQuestionCollapsed] = useState(true); - const formatToolName = (toolName: string | undefined): string => { - if (!toolName) return ''; - // Display-name overrides for tools whose label differs from the formatted key. - const overrides: Record = { - artifact_generator: 'Artifact', - }; - if (overrides[toolName]) return overrides[toolName]; - return toolName - .split('_') - .map((word) => word.charAt(0).toUpperCase() + word.slice(1).toLowerCase()) - .join(' '); - }; - - // One entry per artifact, not per tool call: a single call (``run_code``) - // can write several files, and only the first was reachable before. - const completedArtifacts = (toolCalls ?? []) - .filter((toolCall) => toolCall.status === 'completed') - .flatMap((toolCall) => { - const produced = toolCall.artifacts?.length - ? toolCall.artifacts - : toolCall.artifact_id - ? [{ id: toolCall.artifact_id, filename: undefined, ref: undefined }] - : []; - return produced.map((artifact) => ({ - id: artifact.id, - ref: artifact.ref ?? undefined, - // The file's own name is what the user recognises; the tool that made - // it ("Code Executor") tells them nothing about which file this is. - label: - artifact.filename || formatToolName(toolCall.tool_name) || 'Artifact', - toolName: toolCall.tool_name, - callId: toolCall.call_id, - })); - }); + const completedArtifacts = deriveArtifactChips(toolCalls); useOutsideAlerter(editableQueryRef, () => setIsEditClicked(false), [], true); diff --git a/frontend/src/conversation/MarkdownAnswer.sandbox.test.tsx b/frontend/src/conversation/MarkdownAnswer.sandbox.test.tsx index 19ec2f62..4c2d1a50 100644 --- a/frontend/src/conversation/MarkdownAnswer.sandbox.test.tsx +++ b/frontend/src/conversation/MarkdownAnswer.sandbox.test.tsx @@ -128,3 +128,94 @@ describe('MarkdownAnswer sandbox: links', () => { expect(container.querySelector('a')?.getAttribute('href')).toBe(''); }); }); + +describe('MarkdownAnswer sandbox: images and scheme-less paths', () => { + let container: HTMLDivElement; + let root: Root; + + beforeEach(() => { + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + }); + + function render(content: string, onOpenArtifact = vi.fn()) { + act(() => { + root.render( + , + ); + }); + return onOpenArtifact; + } + + // `![chart](sandbox:...)` is how models announce a plot. urlTransform runs + // over `src` too, so without an `img` renderer the scheme reached the DOM. + it('turns a sandbox image into the chip instead of a broken image', () => { + const onOpenArtifact = render( + '![the sweep](sandbox:/mnt/data/summer_sweep_up.pdf)', + ); + + expect(container.querySelector('img')).toBeNull(); + const button = container.querySelector('button'); + expect(button?.textContent).toBe('the sweep'); + + act(() => { + button?.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); + expect(onOpenArtifact).toHaveBeenCalledWith({ + id: '9d28fc58-f02f-4d78-a45d-143f60d580e4', + toolName: 'artifact_generator', + }); + }); + + it('drops an unresolvable sandbox image rather than rendering a broken one', () => { + render('![missing](sandbox:/mnt/data/nope.png)'); + + expect(container.querySelector('img')).toBeNull(); + expect(container.textContent).toContain('missing'); + }); + + it('leaves a real image URL alone', () => { + render('![logo](https://example.com/logo.png)'); + + expect(container.querySelector('img')?.getAttribute('src')).toBe( + 'https://example.com/logo.png', + ); + }); + + it('resolves a bare /mnt/data path to the chip', () => { + render('[the report](/mnt/data/summer_sweep_up.pdf)'); + + expect(container.querySelector('a')).toBeNull(); + expect(container.querySelector('button')?.textContent).toBe('the report'); + }); + + it('resolves a file: URL to the chip instead of a blank href', () => { + render('[the report](file:///mnt/data/summer_sweep_up.pdf)'); + + expect(container.querySelector('a')).toBeNull(); + expect(container.querySelector('button')?.textContent).toBe('the report'); + }); + + it('leaves an ordinary relative link untouched', () => { + render('[docs](/docs/getting-started)'); + + const anchor = container.querySelector('a'); + expect(anchor?.getAttribute('href')).toBe('/docs/getting-started'); + }); + + it('still blocks javascript: URLs', () => { + render('[click](javascript:alert(1))'); + + expect(container.querySelector('a')?.getAttribute('href')).toBe(''); + }); +}); diff --git a/frontend/src/conversation/MarkdownAnswer.tsx b/frontend/src/conversation/MarkdownAnswer.tsx index 696f23d6..cafb1274 100644 --- a/frontend/src/conversation/MarkdownAnswer.tsx +++ b/frontend/src/conversation/MarkdownAnswer.tsx @@ -1,6 +1,6 @@ import 'katex/dist/katex.min.css'; -import { Fragment, useMemo } from 'react'; +import { Fragment, type ReactNode, useMemo } from 'react'; import ReactMarkdown from 'react-markdown'; import { Prism as SyntaxHighlighter } from 'react-syntax-highlighter'; import { @@ -85,6 +85,34 @@ export default function MarkdownAnswer({ [content], ); + // Shared by the `a` and `img` renderers: a generated file is already on the + // turn as a download chip, so both point at the chip rather than at a URL no + // browser can open. + const renderArtifactChip = ( + artifact: SandboxArtifact, + content: ReactNode, + ) => { + if (!onOpenArtifact) return <>{content}; + return ( + + ); + }; + return ( <> {contentSegments.map((segment, index) => ( @@ -109,26 +137,7 @@ export default function MarkdownAnswer({ return <>{children}; } if (sandboxLink.kind === 'artifact') { - const { artifact } = sandboxLink; - if (!onOpenArtifact) return <>{children}; - return ( - - ); + return renderArtifactChip(sandboxLink.artifact, children); } if (href?.startsWith('#cite-')) { const num = href.replace('#cite-', ''); @@ -169,6 +178,26 @@ export default function MarkdownAnswer({ ); }, + img({ src, alt }) { + // `![chart](sandbox:/mnt/data/chart.png)` is how a model + // announces a plot it produced. react-markdown runs + // urlTransform over `src` too, so without this the invented + // scheme survives and renders a broken-image box beside the + // chip that opens the very same file. + const source = typeof src === 'string' ? src : undefined; + const sandboxLink = resolveSandboxLink(source, artifacts); + if (sandboxLink.kind === 'artifact') { + const { artifact } = sandboxLink; + return renderArtifactChip( + artifact, + alt || artifact.label || 'Open file', + ); + } + if (sandboxLink.kind === 'plain') { + return <>{alt ?? ''}; + } + return {alt}; + }, code(props) { const { children, className, node, ref, ...rest } = props; const match = /language-(\w+)/.exec(className || ''); diff --git a/frontend/src/conversation/artifactChips.test.ts b/frontend/src/conversation/artifactChips.test.ts index d6a8895c..dca77e27 100644 --- a/frontend/src/conversation/artifactChips.test.ts +++ b/frontend/src/conversation/artifactChips.test.ts @@ -6,29 +6,16 @@ * to drop ``artifacts``, so the fallback was the only path on reload: three * files became one chip labelled with the tool's name. The payload below is * captured verbatim from ``GET /api/get_single_conversation``. + * + * This exercises ``deriveArtifactChips`` itself — the function the component + * calls. An earlier version of this file reimplemented the derivation locally, + * so dropping ``artifacts`` from the component left the suite green. */ import { describe, expect, it } from 'vitest'; +import { deriveArtifactChips } from './artifactChips'; import type { ToolCallsType } from './types'; -/** Mirrors the derivation in ConversationBubble.tsx. */ -function chipsFor(toolCalls: ToolCallsType[]) { - return toolCalls - .filter((toolCall) => toolCall.status === 'completed') - .flatMap((toolCall) => { - const produced = toolCall.artifacts?.length - ? toolCall.artifacts - : toolCall.artifact_id - ? [{ id: toolCall.artifact_id, filename: undefined, ref: undefined }] - : []; - return produced.map((artifact) => ({ - id: artifact.id, - ref: artifact.ref ?? undefined, - label: artifact.filename || 'Code Executor', - })); - }); -} - const reloadedTurn = [ { tool_name: 'code_executor', @@ -53,16 +40,20 @@ const reloadedTurn = [ describe('artifact chips after reload', () => { it('renders one chip per file, labelled by filename', () => { - expect(chipsFor(reloadedTurn)).toEqual([ + expect(deriveArtifactChips(reloadedTurn)).toEqual([ { id: '7c342ac8-20d3-4658-9f6a-0ac88f665037', ref: 'A1', label: 'monthly_sales.csv', + toolName: 'code_executor', + callId: 'c1', }, { id: '5b9bea99-38ac-4102-a838-09cac7a42f1a', ref: 'A2', label: 'monthly_sales_bar_chart.png', + toolName: 'code_executor', + callId: 'c1', }, ]); }); @@ -73,8 +64,36 @@ describe('artifact chips after reload', () => { const stripped = [ { ...reloadedTurn[0], artifacts: undefined }, ] as unknown as ToolCallsType[]; - const chips = chipsFor(stripped); + const chips = deriveArtifactChips(stripped); expect(chips).toHaveLength(1); expect(chips[0].label).toBe('Code Executor'); }); + + it('skips calls that have not completed', () => { + const running = [ + { ...reloadedTurn[0], status: 'pending' }, + ] as unknown as ToolCallsType[]; + expect(deriveArtifactChips(running)).toEqual([]); + }); + + it('falls back to the tool label, then to Artifact', () => { + const unnamed = [ + { + tool_name: 'artifact_generator', + call_id: 'c2', + status: 'completed', + artifacts: [{ id: 'x1' }], + }, + { + tool_name: undefined, + call_id: 'c3', + status: 'completed', + artifacts: [{ id: 'x2' }], + }, + ] as unknown as ToolCallsType[]; + expect(deriveArtifactChips(unnamed).map((chip) => chip.label)).toEqual([ + 'Artifact', + 'Artifact', + ]); + }); }); diff --git a/frontend/src/conversation/artifactChips.ts b/frontend/src/conversation/artifactChips.ts new file mode 100644 index 00000000..b6c35ce3 --- /dev/null +++ b/frontend/src/conversation/artifactChips.ts @@ -0,0 +1,55 @@ +import type { ToolCallsType } from './types'; + +/** One chip per file a completed tool call produced. */ +export type ArtifactChip = { + id: string; + ref?: string; + label: string; + toolName: string; + callId: string; +}; + +/** Display name for a tool, used when an artifact reported no filename. */ +export function formatToolName(toolName: string | undefined): string { + if (!toolName) return ''; + // Display-name overrides for tools whose label differs from the formatted key. + const overrides: Record = { + artifact_generator: 'Artifact', + }; + if (overrides[toolName]) return overrides[toolName]; + return toolName + .split('_') + .map((word) => word.charAt(0).toUpperCase() + word.slice(1).toLowerCase()) + .join(' '); +} + +/** + * One entry per artifact, not per tool call: a single call (``run_code``) can + * write several files, and only the first was reachable before. + * + * Lives here rather than inside ``ConversationBubble`` so the regression test + * exercises this function instead of a copy of it. + */ +export function deriveArtifactChips( + toolCalls: ToolCallsType[] | undefined, +): ArtifactChip[] { + return (toolCalls ?? []) + .filter((toolCall) => toolCall.status === 'completed') + .flatMap((toolCall) => { + const produced = toolCall.artifacts?.length + ? toolCall.artifacts + : toolCall.artifact_id + ? [{ id: toolCall.artifact_id, filename: undefined, ref: undefined }] + : []; + return produced.map((artifact) => ({ + id: artifact.id, + ref: artifact.ref ?? undefined, + // The file's own name is what the user recognises; the tool that made + // it ("Code Executor") tells them nothing about which file this is. + label: + artifact.filename || formatToolName(toolCall.tool_name) || 'Artifact', + toolName: toolCall.tool_name, + callId: toolCall.call_id, + })); + }); +} diff --git a/frontend/src/conversation/sandboxLinks.test.ts b/frontend/src/conversation/sandboxLinks.test.ts index 350e42b0..99ca02d6 100644 --- a/frontend/src/conversation/sandboxLinks.test.ts +++ b/frontend/src/conversation/sandboxLinks.test.ts @@ -221,3 +221,78 @@ describe('resolveSandboxLink', () => { } }); }); + +describe('duplicate filenames within one turn', () => { + // A second `run_code` that rewrites `report.pdf` creates a second artifact + // row with the same filename. The model's prose means the file as it stands + // after the last write, so the newer copy is the one to open. + const twoWrites = [ + { id: 'stale', label: 'report.pdf', ref: 'A1' }, + { id: 'fresh', label: 'report.pdf', ref: 'A2' }, + ]; + + it('resolves a bare filename to the most recent copy', () => { + const resolved = resolveSandboxLink( + 'sandbox:/mnt/data/report.pdf', + twoWrites, + ); + expect(resolved).toEqual({ + kind: 'artifact', + artifact: { id: 'fresh', label: 'report.pdf', ref: 'A2' }, + }); + }); + + it('still honours an explicit ref over the filename', () => { + const resolved = resolveSandboxLink('artifact:A1', twoWrites); + expect(resolved).toEqual({ + kind: 'artifact', + artifact: { id: 'stale', label: 'report.pdf', ref: 'A1' }, + }); + }); +}); + +describe('scheme-less and file: hrefs', () => { + const available = [{ id: 'a1', label: 'report.pdf', ref: 'A1' }]; + + it.each([ + '/mnt/data/report.pdf', + 'report.pdf', + './report.pdf', + '/tmp/report.pdf', + 'file:///mnt/data/report.pdf', + ])('resolves %s to the artifact', (href) => { + expect(resolveSandboxLink(href, available)).toEqual({ + kind: 'artifact', + artifact: available[0], + }); + }); + + it('leaves an unresolvable relative link as an ordinary link', () => { + expect(resolveSandboxLink('/docs/intro', available)).toEqual({ + kind: 'external', + }); + }); + + it('leaves in-page anchors alone', () => { + expect(resolveSandboxLink('#cite-2', available)).toEqual({ + kind: 'external', + }); + }); + + it('leaves other schemes alone', () => { + expect( + resolveSandboxLink('https://example.com/report.pdf', available), + ).toEqual({ + kind: 'external', + }); + expect(resolveSandboxLink('mailto:a@b.com', available)).toEqual({ + kind: 'external', + }); + }); + + it('renders an unresolvable file: href as plain text, never a dead href', () => { + expect(resolveSandboxLink('file:///mnt/data/nope.pdf', available)).toEqual({ + kind: 'plain', + }); + }); +}); diff --git a/frontend/src/conversation/sandboxLinks.ts b/frontend/src/conversation/sandboxLinks.ts index 63a7a308..dafc0cb7 100644 --- a/frontend/src/conversation/sandboxLinks.ts +++ b/frontend/src/conversation/sandboxLinks.ts @@ -13,6 +13,12 @@ import { defaultUrlTransform } from 'react-markdown'; * sandbox:/tmp/ sandbox:/ * artifact:A1 * + * The same path also arrives with no scheme at all (``/mnt/data/report.pdf``, + * ``report.pdf``) or as ``file:///mnt/data/report.pdf``. Those are the same + * mistake wearing different clothes, so they resolve the same way — except that + * an unresolvable scheme-less href stays an ordinary link, since a relative URL + * is something a model may legitimately write. + * * Left alone the result is worse than a no-op: react-markdown's * ``defaultUrlTransform`` blanks any unknown protocol, so the anchor renders as * ``href=""`` and clicking it opens the current page again in a new tab — which @@ -31,7 +37,14 @@ import { defaultUrlTransform } from 'react-markdown'; * own short ref (``artifact:A1``) leaking into prose. react-markdown blanks * both, so both have to be intercepted. */ -export const GENERATED_FILE_SCHEMES = ['sandbox:', 'artifact:'] as const; +export const GENERATED_FILE_SCHEMES = [ + 'sandbox:', + 'artifact:', + // Browsers refuse to navigate to `file:` from an https page, so a `file:` + // href is dead whatever we do with it; routing it here at least turns it into + // the chip or plain text instead of react-markdown's blank `href=""`. + 'file:', +] as const; export type SandboxArtifact = { id: string; @@ -78,6 +91,24 @@ function matchedScheme(href: string | undefined | null): string | null { /** Short refs the artifact tool hands back, e.g. ``A1`` is the first artifact. */ const SHORT_REF = /^a(\d+)$/i; +/** Any URL scheme, so ``https:`` and ``mailto:`` are left alone. */ +const ANY_SCHEME = /^[a-z][a-z0-9+.-]*:/i; + +/** + * Last match wins. Within one turn the model's prose means the file as it + * stands after the *last* write, so a rerun that overwrites ``report.pdf`` + * should open the new copy rather than the one it replaced. + */ +function findLast( + items: T[], + predicate: (item: T) => boolean, +): T | undefined { + for (let index = items.length - 1; index >= 0; index -= 1) { + if (predicate(items[index])) return items[index]; + } + return undefined; +} + function decodeSegment(segment: string): string { try { return decodeURIComponent(segment); @@ -103,16 +134,27 @@ export function resolveSandboxLink( href: string | undefined | null, artifacts: SandboxArtifact[] | undefined, ): SandboxLinkResolution { + if (typeof href !== 'string' || !href) return { kind: 'external' }; const scheme = matchedScheme(href); - if (!scheme || typeof href !== 'string') return { kind: 'external' }; + // A scheme-less href may be a genuine relative link or in-page anchor, so it + // only ever *upgrades* to a chip on a hit and is otherwise left untouched. + if (!scheme && (href.startsWith('#') || ANY_SCHEME.test(href))) { + return { kind: 'external' }; + } + const miss: SandboxLinkResolution = scheme + ? { kind: 'plain' } + : { kind: 'external' }; const available = artifacts ?? []; // Strip the scheme, any leading slashes, and a trailing query/fragment. - const path = href.slice(scheme.length).replace(/^\/+/, '').split(/[?#]/)[0]; - if (!path) return { kind: 'plain' }; + const path = href + .slice(scheme?.length ?? 0) + .replace(/^\/+/, '') + .split(/[?#]/)[0]; + if (!path) return miss; const segments = path.split('/').filter(Boolean).map(decodeSegment); - if (segments.length === 0) return { kind: 'plain' }; + if (segments.length === 0) return miss; const last = segments[segments.length - 1]; @@ -120,12 +162,13 @@ export function resolveSandboxLink( // matched against the artifact's own ``ref``, never by position: ``A2`` is the // conversation's second artifact, which on a later turn is not this turn's // second one. - const byId = available.find((artifact) => artifact.id === last); + const byId = findLast(available, (artifact) => artifact.id === last); if (byId) return { kind: 'artifact', artifact: byId }; if (SHORT_REF.test(last)) { const wanted = last.toUpperCase(); - const byRef = available.find( + const byRef = findLast( + available, (artifact) => (artifact.ref ?? '').toUpperCase() === wanted, ); if (byRef) return { kind: 'artifact', artifact: byRef }; @@ -134,11 +177,12 @@ export function resolveSandboxLink( // Fabricated paths (``/mnt/data/…``, ``/tmp/…``, a bare filename) — and // ``/artifact/``, which the model also writes — usually still name // a file that exists as an artifact on this turn. - const byName = available.find( + const byName = findLast( + available, (artifact) => - artifact.label && artifact.label.toLowerCase() === last.toLowerCase(), + !!artifact.label && artifact.label.toLowerCase() === last.toLowerCase(), ); if (byName) return { kind: 'artifact', artifact: byName }; - return { kind: 'plain' }; + return miss; } diff --git a/tests/agents/test_tool_action_parser.py b/tests/agents/test_tool_action_parser.py index 7a97a88b..16cfecb3 100644 --- a/tests/agents/test_tool_action_parser.py +++ b/tests/agents/test_tool_action_parser.py @@ -358,3 +358,50 @@ class TestZeroArgumentToolCalls: call.arguments = "invalid json" assert parser.parse_args(call) == (None, None, None) + + +@pytest.mark.unit +class TestNonObjectArguments: + """Valid JSON of the wrong type must keep the resolved tool name. + + ``[]`` or ``5`` decodes cleanly, so the name is knowable; only the payload + is wrong. Collapsing it into the malformed-JSON sentinel journalled the call + as ``tool_name: "unknown"`` with no ``tool_id``, which under-attributes the + per-tool analytics and fed the call into the invented-name throttle. The + executor's own non-dict guard reports it with the real tool instead. + """ + + MAPPING = {"read_document": ("42", "read_document")} + + @pytest.mark.parametrize("arguments", ['[]', 'null', '5', '"a string"', '["a", "b"]']) + def test_the_name_survives_a_non_object_payload(self, arguments): + parser = ToolActionParser("OpenAILLM", name_mapping=self.MAPPING) + call = Mock() + call.name = "read_document" + call.arguments = arguments + + tool_id, action_name, _call_args = parser.parse_args(call) + + assert tool_id == "42" + assert action_name == "read_document" + + def test_genuinely_malformed_arguments_still_discard_the_call(self): + parser = ToolActionParser("OpenAILLM", name_mapping=self.MAPPING) + call = Mock() + call.name = "read_document" + call.arguments = "{not json" + + assert parser.parse_args(call) == (None, None, None) + + @pytest.mark.parametrize("arguments", ['[]', '{not json', 'null']) + def test_google_still_falls_back_to_empty_args(self, arguments): + """Gemini's branch must keep handing downstream a dict to walk.""" + parser = ToolActionParser("GoogleLLM", name_mapping=self.MAPPING) + call = Mock() + call.name = "read_document" + call.arguments = arguments + + tool_id, action_name, call_args = parser.parse_args(call) + + assert (tool_id, action_name) == ("42", "read_document") + assert call_args == {} diff --git a/tests/agents/test_tool_executor_duplicate_registration.py b/tests/agents/test_tool_executor_duplicate_registration.py new file mode 100644 index 00000000..098b2158 --- /dev/null +++ b/tests/agents/test_tool_executor_duplicate_registration.py @@ -0,0 +1,79 @@ +"""A duplicate builtin registration must not drop the row that carries the gate. + +``code_executor`` can arrive twice — the user's stored ``user_tools`` row and the +synthesized default — and the picker labels both "Code Executor". Collapsing them +by insertion order meant the agent's click order decided which survived, and the +survivor is the only row consulted for approval, so a gated row landing second +was silently dropped and ``run_code`` ran unapproved. +""" + +from __future__ import annotations + +import pytest + +from application.agents.default_tools import default_tool_id +from application.agents.tool_executor import ToolExecutor + +SYNTHESIZED_ID = default_tool_id("code_executor") +STORED_ID = "9f1d4c2e-0000-4000-8000-000000000001" + + +def _row(*, action_approval=False, config_approval=False): + return { + "name": "code_executor", + "actions": [ + {"name": "run_code", "active": True, "require_approval": action_approval} + ], + "config": {"require_approval": True} if config_approval else {}, + } + + +def _survivor(tools_dict): + executor = ToolExecutor() + schemas = executor.prepare_tools_for_llm(tools_dict) + assert len(schemas) == 1, schemas + tool_id, _action = executor._name_to_tool["run_code"] + return tool_id + + +@pytest.mark.unit +class TestDuplicateBuiltinRegistration: + def test_gated_action_snapshot_survives_when_listed_second(self): + tools = { + SYNTHESIZED_ID: _row(), + STORED_ID: _row(action_approval=True), + } + assert _survivor(tools) == STORED_ID + + def test_gated_deployment_config_survives_when_listed_second(self): + tools = { + SYNTHESIZED_ID: _row(), + STORED_ID: _row(config_approval=True), + } + assert _survivor(tools) == STORED_ID + + def test_stored_row_wins_over_the_synthesized_copy(self): + """Neither carries a gate: the stored row is still the one with config.""" + tools = { + SYNTHESIZED_ID: _row(), + STORED_ID: _row(), + } + assert _survivor(tools) == STORED_ID + + def test_gate_survives_between_two_stored_rows(self): + other = "9f1d4c2e-0000-4000-8000-000000000002" + tools = { + STORED_ID: _row(), + other: _row(action_approval=True), + } + assert _survivor(tools) == other + + def test_still_collapses_to_a_single_callable_name(self): + tools = { + SYNTHESIZED_ID: _row(), + STORED_ID: _row(action_approval=True), + } + executor = ToolExecutor() + schemas = executor.prepare_tools_for_llm(tools) + names = [s["function"]["name"] for s in schemas] + assert names == ["run_code"], names diff --git a/tests/agents/test_tool_executor_hallucinated_calls.py b/tests/agents/test_tool_executor_hallucinated_calls.py index 5a9afee4..f472ae24 100644 --- a/tests/agents/test_tool_executor_hallucinated_calls.py +++ b/tests/agents/test_tool_executor_hallucinated_calls.py @@ -118,3 +118,72 @@ class TestErrorNamesWhatTheModelCanCall: executor.prepare_tools_for_llm(tools_dict) _events, (result, _call_id) = _drain(executor, _call("make_a_pdf"), tools=tools_dict) assert "create_artifact" in result + + +@pytest.mark.unit +class TestThrottleScope: + """The throttle must fire on invented names only, and per distinct payload.""" + + def test_registered_tool_with_bad_arguments_is_never_refused(self): + """Three malformed bodies for a real tool must not strand it for the turn. + + Truncated ``code``/``spec`` payloads are the common shape here, and they + differ every time — collapsing them into one signature refused a working + tool and named it as its own alternative. + """ + executor = ToolExecutor() + tools = _tools_dict() + executor._name_to_tool = {"memory_view": ("t1", "memory_view")} + bodies = ['{"a": 1', '{"b": 2', '{"c": 3', '{"d": 4'] + + for index, body in enumerate(bodies): + _events, (result, _call_id) = _drain( + executor, _call("memory_view", arguments=body, call_id=f"c{index}"), tools=tools + ) + assert "has already failed" not in result, (index, result) + assert "arguments were not a valid JSON object" in result, (index, result) + assert executor._unresolvable_calls == {} + + def test_invented_name_is_refused_on_the_third_attempt(self): + executor = ToolExecutor() + for index in range(2): + _events, (result, _call_id) = _drain( + executor, _call("note_view", call_id=f"c{index}") + ) + assert "has already failed" not in result, (index, result) + + _events, (result, _call_id) = _drain(executor, _call("note_view", call_id="c2")) + assert "has already failed 2 times" in result, result + + def test_the_refusal_does_not_suggest_the_tool_it_refuses(self): + """``memory`` is a real tool; refusing it must not offer it as the way out.""" + executor = ToolExecutor() + tools = _tools_dict() + executor._tool_to_name = {("t1", "memory_view"): "memory_view"} + for index in range(3): + _events, (result, _call_id) = _drain( + executor, _call("memory_view", call_id=f"c{index}"), tools=tools + ) + assert "has already failed" in result, result + assert "(none available)" in result, result + + def test_distinct_payloads_for_an_invented_name_stay_distinct(self): + executor = ToolExecutor() + for index, body in enumerate(['{"a": 1}', '{"b": 2}', '{"c": 3}']): + _events, (result, _call_id) = _drain( + executor, _call("note_view", arguments=body, call_id=f"c{index}") + ) + assert "has already failed" not in result, (index, result) + assert len(executor._unresolvable_calls) == 3 + + def test_the_failure_count_keeps_escalating(self): + """A count frozen at the limit makes the message and the ops log useless.""" + executor = ToolExecutor() + results = [] + for index in range(5): + _events, (result, _call_id) = _drain( + executor, _call("note_view", call_id=f"c{index}") + ) + results.append(result) + assert "has already failed 2 times" in results[2], results[2] + assert "has already failed 4 times" in results[4], results[4]