feat: mini tool call hardening

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

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."}

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", "<unknown>"),
                 )
                 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",

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<HTMLDivElement>(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<string, string> = {
-      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);
 

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(
+        <MarkdownAnswer
+          content={content}
+          artifacts={artifacts}
+          onOpenArtifact={onOpenArtifact}
+        />,
+      );
+    });
+    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('');
+  });
+});

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 (
+      <Button
+        type="button"
+        variant="link"
+        onClick={() =>
+          onOpenArtifact({
+            id: artifact.id,
+            toolName: artifact.toolName ?? '',
+          })
+        }
+        /* Sits mid-sentence: no pill background, no fixed height, and it must
+           wrap with the surrounding text. */
+        className="text-primary h-auto w-auto bg-transparent p-0 whitespace-normal underline underline-offset-2"
+        title={artifact.label}
+      >
+        {content}
+      </Button>
+    );
+  };
+
   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 (
-                      <Button
-                        type="button"
-                        variant="link"
-                        onClick={() =>
-                          onOpenArtifact({
-                            id: artifact.id,
-                            toolName: artifact.toolName ?? '',
-                          })
-                        }
-                        /* Sits mid-sentence: no pill background, no fixed
-                           height, and it must wrap with the surrounding text. */
-                        className="text-primary h-auto w-auto bg-transparent p-0 whitespace-normal underline underline-offset-2"
-                        title={artifact.label}
-                      >
-                        {children}
-                      </Button>
-                    );
+                    return renderArtifactChip(sandboxLink.artifact, children);
                   }
                   if (href?.startsWith('#cite-')) {
                     const num = href.replace('#cite-', '');
@@ -169,6 +178,26 @@ export default function MarkdownAnswer({
                     </a>
                   );
                 },
+                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 <img src={source} alt={alt} className="max-w-full" />;
+                },
                 code(props) {
                   const { children, className, node, ref, ...rest } = props;
                   const match = /language-(\w+)/.exec(className || '');

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',
+    ]);
+  });
 });

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<string, string> = {
+    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,
+      }));
+    });
+}

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',
+    });
+  });
+});

frontend/src/conversation/sandboxLinks.ts

@@ -13,6 +13,12 @@ import { defaultUrlTransform } from 'react-markdown';
  *   sandbox:/tmp/<filename>       sandbox:/<filename>
  *   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<T>(
+  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/<filename>``, 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;
 }

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 == {}

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

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]