From 554d62e87bb30f7a2a16e16e3c6c8b99993f7185 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Wed, 17 Jun 2026 18:36:41 +0300 Subject: [PATCH 01/12] docs: add initial design for built-in web fetch tool (#344) Add design doc for `internal_file_fetch`, a self-contained internal tool that fetches a web URL. A `save` flag selects between loading content inline (default) and persisting it as a workspace DIAL file for the existing file tools. Reuses ExternalUrlFetcher and the two-tier egress policy; independent of the large-tool-response offload processor. --- docs/designs/web_fetch_tool.md | 275 +++++++++++++++++++++++++++++++++ 1 file changed, 275 insertions(+) create mode 100644 docs/designs/web_fetch_tool.md diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md new file mode 100644 index 00000000..23892381 --- /dev/null +++ b/docs/designs/web_fetch_tool.md @@ -0,0 +1,275 @@ +# Design: Built-in Web Fetch Tool (`internal_file_fetch`) + +- **Status:** Draft +- **Dependencies:** + - [external_url_attachments.md](external_url_attachments.md) — `ExternalUrlFetcher`, `AttachmentService`, and the two-tier egress policy this tool reuses unchanged. + - [dial_files_tools.md](dial_files_tools.md) / [dial_files_search.md](dial_files_search.md) — the `internal_file_*` tool family this tool joins, and the workspace a saved file lands in. + +## Problem Statement + +The agent today has two halves of a capability but not the bridge between them: + +- **It can *pass* an external URL outward.** PR #279 ([external_url_attachments.md](external_url_attachments.md)) made external URLs first-class file references, so the agent can hand `https://…` to a deployment or a REST/MCP tool as an attachment. +- **It can *operate on* files already in its workspace.** The `internal_file_*` family (`list`, `read_lines`, `search`, `find`, …) reads, searches, and edits files that already live in the agent's DIAL workspace. + +What is missing is the ability for the agent to **fetch a web resource for its own use** — read a README from GitHub, a source file, a documentation page — either to consume the content directly *or* to pull it into its workspace for the existing file tools. Issue #344 asks for exactly this: "considering we introduced file tools, it would be logical to introduce a fetch tool as well." + +Without it, an agent that needs the *contents* of a web page has no path: external URLs can only be forwarded to a downstream consumer, never read by the orchestrating model itself. + +## Design Goals + +- **Read in one call by default.** Fetching a text URL returns the content inline in a single tool call — no file written, no follow-up required. +- **Optional persistence under agent control.** A `save` flag lets the agent instead pull the resource into its workspace as a durable DIAL file, so the existing `internal_file_*` tools can operate on it. +- **Self-contained.** The tool owns its own behavior end-to-end. It does **not** depend on the large-tool-response offload processor or any other cross-cutting feature; large/binary content is handled by the agent choosing `save=true`. +- **Bounded output.** In load mode the tool never dumps unbounded content into the LLM context; a hard, tool-owned size guard redirects oversized fetches to `save=true`. +- **Zero new egress surface.** The fetch goes through the existing `ExternalUrlFetcher`, inheriting the two-tier egress policy, host allowlist, SSRF guard, and size/redirect/timeout caps verbatim. +- **Conventional.** Follows the established internal-tool pattern (`StagedBaseTool`, config-driven enablement, auto-generated schema) and joins the `internal_file_*` family by name and placement. + +--- + +## The `save` argument (core design decision) + +The tool exposes a single behavioral switch, `save: bool = false`, which selects between two modes: + +| | `save=false` (default) — **load into context** | `save=true` — **persist to workspace** | +|---|---|---| +| **What the agent gets back** | The fetched text inline in the tool result | The saved workspace-relative path (+ a short text preview when textual) | +| **File written?** | No | Yes — durable DIAL file via `AttachmentService` | +| **Best for** | Reading a page/README/code file once | Large files, binary files, or content the agent will read/search repeatedly with the file tools | +| **Non-text/binary content** | Rejected — parameter error pointing at `save=true` (bytes can't be inlined) | Saved as-is; path returned | +| **Oversized text** | Rejected by the size guard — error pointing at `save=true` | Saved; ranged reading is then `internal_file_read_lines` / `internal_file_search` | + +This switch is what makes the tool self-contained: **`save=true` is the large-content and binary-content story**, so the load path needs no pagination, no offload dependency, and no persistence — only a guard rail. + +--- + +## Use Cases + +### UC-1: Agent fetches a text resource and reads it in one call (default) + +**Trigger:** Agent calls `internal_file_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` (default `save=false`). + +**Behavior:** The tool fetches the bytes through `ExternalUrlFetcher` and, because the content is textual and within the size guard, returns the decoded text inline (code-block wrapped). No file is written. + +**Outcome:** The model sees the README contents immediately. No second tool call, no workspace artifact. + +### UC-2: Agent pulls a resource into the workspace for later use + +**Trigger:** Agent calls `internal_file_fetch(url="https://…/data.py", save=true)`. + +**Behavior:** The tool fetches the bytes and persists them as a DIAL file in the agent's workspace via `AttachmentService`. The result reports the saved workspace-relative path plus a short text preview (for textual content). + +**Outcome:** The model gets the path and a preview, and can now read or search the file with the existing `internal_file_*` tools. + +### UC-3: Agent saves, then chains a file tool + +**Trigger:** After UC-2, agent calls `internal_file_search(path="files/…/data.py", query="def main")`. + +**Behavior:** The search tool operates on the persisted file exactly as it would on any uploaded file. + +**Outcome:** Fetch composes with the rest of the file family with no special-casing. + +### UC-4: Agent loads a large text file without saving + +**Trigger:** Agent calls `internal_file_fetch(url="https://…/huge.log")` with default `save=false`, and the decoded text exceeds the tool's size guard. + +**Behavior:** The tool does **not** truncate-and-guess. It returns a parameter error explaining the content is too large to load inline and instructing the agent to retry with `save=true` (then read ranges via the file tools). + +**Outcome:** The agent gets a clear, actionable next step; the context window is protected. + +### UC-5: Agent fetches non-text / binary content + +**Trigger:** Agent calls `internal_file_fetch` on a URL whose content type is non-textual (image, zip, PDF, …). + +**Behavior:** With `save=false`, the tool returns a parameter error: binary content cannot be loaded into context; retry with `save=true`. With `save=true`, the bytes are saved and the path + content type + size are returned (no inline body, no extraction in phase-1). + +**Outcome:** The model never receives garbled bytes in context; binary is reachable only as a saved file it can forward to a capable deployment/tool. + +### UC-6: External egress is disabled (admin cap or per-app opt-out) + +**Trigger:** Agent calls `internal_file_fetch` on an external URL while `EXTERNAL_URL_FETCH_ENABLED=false` (or the app set `features.external_url_fetch.enabled=false`, or the host is outside the allowlist). + +**Behavior:** `ExternalUrlFetcher.fetch` raises `ExternalFetchDisabledError`; the tool wraps it as a parameter-scoped tool error carrying the existing operator/builder/allowlist message. + +**Outcome:** The model gets a clear, actionable refusal — no new policy, no bypass. + +### UC-7: Agent passes a DIAL URL + +**Trigger:** Agent calls `internal_file_fetch(url="files//doc.md")`. + +**Behavior:** The URL is classified as DIAL (already in the workspace). The tool returns a parameter error pointing the agent at the existing file tools. + +**Outcome:** The fetch tool stays focused on *external* retrieval; in-workspace reads remain the job of `internal_file_read_lines` / `internal_file_search`. + +--- + +## Proposed Design + +```mermaid +flowchart TD + A["LLM: internal_file_fetch(url, save)"] --> B{"classify_url"} + B -->|DIAL| E1["parameter error:
use internal_file_read_lines / _search"] + B -->|unsupported| E2["unsupported-scheme error"] + B -->|EXTERNAL| C["ExternalUrlFetcher.fetch(url)
egress policy + SSRF + size/redirect/timeout"] + C -->|"ExternalFetchDisabledError / ExternalFetchError"| E3["parameter error
(existing messages)"] + C -->|"FetchedBytes: data, content_type, filename"| M{"save?"} + M -->|"false (load)"| L{"textual & within size guard?"} + L -->|no| E4["parameter error:
too large / binary → retry with save=true"] + L -->|yes| LR["ToolCallResult: inline text
(no file written)"] + M -->|"true (persist)"| P["AttachmentService.upload_bytes → workspace file"] + P --> PR["ToolCallResult: saved path
(+ text preview if textual)"] +``` + +### Component 1: The `internal_file_fetch` tool + +- **What:** a new internal tool, `internal_file_fetch`, joining the `internal_file_*` family. It subclasses the shared file-tool base (`_DialFileTool`) so it reuses the family's workspace-path conventions and stage handling; it additionally depends on `ExternalUrlFetcher` (secure fetch) and `AttachmentService` (persist bytes when `save=true`). +- **Owner:** the tool itself orchestrates classify → fetch → (load | persist) → render. It owns no policy logic. +- **Arguments:** + - `url: str` (required) — the http(s) URL to fetch. + - `save: bool = false` (optional) — `false` loads content into context; `true` persists a workspace file. See the table above. +- **Semantics (runtime):** + 1. **Classify** the URL (`classify_url`, reused from the external-fetch machinery). `DIAL` → parameter error (UC-7); unsupported scheme → unsupported-scheme error. + 2. **Fetch** via `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This call is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `ExternalFetchDisabledError` / `ExternalFetchError` are caught and re-raised as parameter-scoped tool errors, matching `FileLoaderService` / `DialFilePromoter`. + 3. **Branch on `save`:** + - **`save=false`:** require textual content (Component 3) within the size guard (Component 2). If either fails → parameter error directing the agent to `save=true` (UC-4/UC-5). Otherwise return the decoded text inline. + - **`save=true`:** persist via `AttachmentService.upload_bytes(data, content_type, filename)` → `FileMetadata`, into the workspace root the file tools address (Component 4). Return the saved path (+ a short text preview when textual). +- **Change:** purely additive; no existing tool is modified. The large-tool-response offload processor is **not** involved — this tool is self-contained by design. + +> **Note on reuse.** `DialFilePromoter.promote(url)` already does "fetch + upload" but discards the bytes and returns only metadata; it also doesn't guarantee the file-family workspace root or produce a preview. The `save=true` path therefore calls `ExternalUrlFetcher.fetch` + `AttachmentService.upload_bytes` directly (the same two steps `promote` performs internally), fetching once and keeping the bytes for the preview. + +### Component 2: Load-mode size guard + +- **What:** a hard, tool-owned upper bound on the bytes the tool will inline when `save=false`. +- **Semantics:** if the fetched content (when `save=false`) exceeds the guard, the tool returns a parameter error — it does **not** silently truncate — telling the agent to retry with `save=true` and read ranges via the file tools. +- **Why a hard guard, not truncation or pagination:** a silent truncation hides data from the model; pagination would duplicate `internal_file_read_lines` / `internal_file_search`, which already provide ranged access on a saved file. `save=true` is the deliberate, agent-visible path to "more than fits inline." +- **Default:** a single server-side byte cap (proposed default ~100 KB), defined as a constant/setting. Independent of the offload feature's threshold. + +### Component 3: Textual vs. non-textual classification + +- **What:** the predicate that decides whether content can be loaded inline (`save=false`) and whether a preview is produced (`save=true`). +- **Semantics:** derived from the fetched `content_type` — `text/*`, `application/json`, `application/xml`, and the common source/markup types are treated as textual (decoded with the response charset, falling back to UTF-8 with replacement). Everything else is non-textual. +- **Change:** a small helper local to the tool; no dependency on content-sniffing libraries in phase-1. + +### Component 4: Workspace placement (applies to `save=true`) + +- **What:** when persisting, the file must be written to the **same workspace root** that the `internal_file_*` tools list and address, and the tool must return the **workspace-relative path** (not an opaque DIAL URL). +- **Why:** the "fetch composes with the file tools" promise (UC-3) depends on `internal_file_search`/`read_lines` finding the file by the path the fetch tool reports. If `AttachmentService.upload_bytes` does not already target that root, the tool must direct the upload there (e.g., a per-conversation/appdata prefix consistent with the file family). +- **Verification:** confirm during implementation where the file family resolves relative paths (`_DialFileTool` path resolution) and ensure the upload targets the same location; add a test that fetches with `save=true` then `internal_file_list`s the returned path. + +### Component 5: Tool config, name, and DI wiring + +- **Name:** add `INTERNAL_FILE_FETCH_TOOL_NAME = "internal_file_fetch"` to `common/tool_names.py`. +- **Config:** an `InternalTool` entry with the OpenAI function schema (`url` string required; `save` boolean default `false`). Enabled via the existing `internal` tool set like the rest of the family — no new tool-set type. +- **DI:** bind the tool at request scope and dispatch it from the file-tooling module's `@multiprovider` by matching the tool name (the established pattern for the family). Nothing new in `app_factory.py` beyond what the file-tooling module already provides. +- **Schema:** run `make dump_app_schema` to regenerate `docs/generated-app-schema.json` and `docs/generated-internal-tools.json`. + +### Component 6: Egress policy (reused, unchanged) + +- No new policy code. The two-tier gate (`EXTERNAL_URL_FETCH_ENABLED` + `features.external_url_fetch.enabled`), host allowlists, and SSRF guard are enforced inside `ExternalUrlFetcher.fetch`. The tool's only responsibility is to surface the resulting errors clearly (UC-6). + +--- + +## Out of Scope + +Deferred from phase-1; each is a clean follow-on, not a rework: + +- **PDF / binary text extraction.** Binary content is save-only (no inline, no extraction) in phase-1. Extraction needs a parser and a preview strategy; future phase. +- **Reliance on the large-tool-response offload processor.** Intentionally avoided to keep the tool self-contained; `save=true` covers the large-content case explicitly. +- **Load-mode pagination / `start_index`.** Unnecessary — `save=true` + `internal_file_read_lines` / `internal_file_search` provide ranged access; load mode is for content that fits inline. +- **Content summarization** (Claude Code-style prompt-over-content). +- **Surfacing the saved file to the user via `propagate_to_choice`** and richer binary metadata (thumbnails, structured metadata). +- **Provenance-based URL allowlisting** (Anthropic-style: only fetch URLs that appeared in conversation context, to harden against model-fabricated exfiltration URLs). A worthwhile future hardening; the existing egress policy + host allowlist already gate destinations today. +- **DIAL-URL retrieval.** A DIAL URL is rejected with guidance (UC-7) rather than fetched; in-workspace reads stay with the file tools. + +--- + +## Configuration / Usage Examples + +### Enabling the tool + +```yaml +tool_sets: + - type: internal + tools: + - type: internal-tool + enabled: true + open_ai_tool: + type: function + function: + name: internal_file_fetch + description: >- + Fetch a file or page from a web URL. By default returns the text content + inline. Set save=true to store it as a workspace file (required for large + or binary content) and read it with the other file tools. + parameters: + type: object + properties: + url: + type: string + description: The http(s) URL to fetch. + save: + type: boolean + description: >- + If true, persist the fetched content as a workspace file and return + its path instead of inlining it. Required for binary or oversized content. + default: false + required: [url] +``` + +### Walkthrough — load into context (UC-1) + +`internal_file_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` +→ returns the README text inline. No file written. + +### Walkthrough — save then search (UC-2 → UC-3) + +1. `internal_file_fetch(url="https://…/data.py", save=true)` + → `saved: files///data.py` + short preview. +2. `internal_file_search(path="files///data.py", query="def main")` + → operates on the persisted file with no re-fetch. + +### Egress disabled (UC-6) + +`internal_file_fetch(url="https://example.com/x")` with `EXTERNAL_URL_FETCH_ENABLED=false` +→ tool error: *"External URL fetching is disabled by operator policy (EXTERNAL_URL_FETCH_ENABLED)."* + +--- + +## Migration + +### Breaking changes + +None. The tool is purely additive and opt-in via app config. + +### Non-breaking changes + +- New tool `internal_file_fetch` appears in the generated schema/manifest after `make dump_app_schema`. +- No change to existing tools, the egress policy, the offload feature, or the config shape beyond the new tool entry. + +## Summary of Changes + +### New files + +- `dial_files_tooling/_fetch_file_tool.py` — the `internal_file_fetch` tool (`_DialFileTool` subclass; depends on `ExternalUrlFetcher` + `AttachmentService`). +- Unit tests under the file-tooling test package. + +### Modified files + +- `common/tool_names.py` — add `INTERNAL_FILE_FETCH_TOOL_NAME`. +- The file-tooling DI module — bind the tool and dispatch it from the existing `@multiprovider`; add the tool's `InternalTool` config definition. +- `docs/generated-app-schema.json`, `docs/generated-internal-tools.json` — regenerated. + +### Tools exposed to the LLM + +- `internal_file_fetch(url, save=false)` — fetch an external resource. Default returns text inline; `save=true` persists a workspace file and returns its path. + +### Tests + +- `save=false`, textual, within guard → full inline content, no file written. +- `save=false`, textual, over size guard → parameter error pointing at `save=true`. +- `save=false`, binary → parameter error pointing at `save=true`. +- `save=true`, textual → persisted, path + preview returned. +- `save=true`, binary → persisted, path + content type + size returned, no inline body. +- `save=true` then `internal_file_list`/`read_lines` on the returned path (workspace-placement guarantee). +- Egress disabled / host not allowed → parameter error with the policy message. +- DIAL URL → parameter error pointing to the file tools. From 48285ee09ec71928b99fb945adf3248da3add92f Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 22 Jun 2026 09:40:33 +0300 Subject: [PATCH 02/12] docs: revise and approve web fetch & file download design (#344) Split the feature into two dedicated tools after review: - internal_web_fetch: load a text resource inline (internal_tooling module, gated by features.web_fetch.enabled, configurable max_inline_size). - internal_file_download: persist any resource into the agent-home workspace via the file-family write path (gated by features.dial_files.enabled_tools). Both feature-gated and preview-gated. Reuses ExternalUrlFetcher + the two-tier egress policy; independent of the offload processor. Status: Approved. --- docs/designs/web_fetch_tool.md | 285 ++++++++++++++++++--------------- 1 file changed, 156 insertions(+), 129 deletions(-) diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md index 23892381..10f59362 100644 --- a/docs/designs/web_fetch_tool.md +++ b/docs/designs/web_fetch_tool.md @@ -1,9 +1,9 @@ -# Design: Built-in Web Fetch Tool (`internal_file_fetch`) +# Design: Built-in Web Fetch & File Download Tools -- **Status:** Draft +- **Status:** Approved - **Dependencies:** - - [external_url_attachments.md](external_url_attachments.md) — `ExternalUrlFetcher`, `AttachmentService`, and the two-tier egress policy this tool reuses unchanged. - - [dial_files_tools.md](dial_files_tools.md) / [dial_files_search.md](dial_files_search.md) — the `internal_file_*` tool family this tool joins, and the workspace a saved file lands in. + - [external_url_attachments.md](external_url_attachments.md) — `ExternalUrlFetcher`, `classify_url`, and the two-tier egress policy both tools reuse unchanged. + - [dial_files_tools.md](dial_files_tools.md) / [dial_files_search.md](dial_files_search.md) — the `internal_file_*` tool family `internal_file_download` joins, its `_DialFileTool` base, and the agent-home workspace a downloaded file lands in. ## Problem Statement @@ -18,36 +18,41 @@ Without it, an agent that needs the *contents* of a web page has no path: extern ## Design Goals -- **Read in one call by default.** Fetching a text URL returns the content inline in a single tool call — no file written, no follow-up required. -- **Optional persistence under agent control.** A `save` flag lets the agent instead pull the resource into its workspace as a durable DIAL file, so the existing `internal_file_*` tools can operate on it. -- **Self-contained.** The tool owns its own behavior end-to-end. It does **not** depend on the large-tool-response offload processor or any other cross-cutting feature; large/binary content is handled by the agent choosing `save=true`. -- **Bounded output.** In load mode the tool never dumps unbounded content into the LLM context; a hard, tool-owned size guard redirects oversized fetches to `save=true`. -- **Zero new egress surface.** The fetch goes through the existing `ExternalUrlFetcher`, inheriting the two-tier egress policy, host allowlist, SSRF guard, and size/redirect/timeout caps verbatim. -- **Conventional.** Follows the established internal-tool pattern (`StagedBaseTool`, config-driven enablement, auto-generated schema) and joins the `internal_file_*` family by name and placement. +- **Read in one call.** Fetching a text URL returns the content inline in a single tool call — no file written, no follow-up required. +- **Persist for the file tools.** A separate tool pulls a resource into the workspace as a durable DIAL file so the existing `internal_file_*` tools can operate on it. +- **One job per tool.** Each tool has a single purpose and a single, predictable return shape — content, or a path. Consistent with the `internal_file_*` family's design (no behavior-changing flags). +- **Independently enableable.** An app can expose read-into-context without allowing file-writing, or vice versa. +- **Self-contained.** Neither tool depends on the large-tool-response offload processor (which nonetheless still post-processes results platform-wide; see Component 4a). Large/binary content is handled by routing the agent to `internal_file_download`. +- **Bounded output.** `internal_web_fetch` never dumps unbounded content into the LLM context; a hard, tool-owned size guard redirects oversized fetches to `internal_file_download`. +- **Zero new egress surface.** Both fetches go through the existing `ExternalUrlFetcher`, inheriting the two-tier egress policy, host allowlist, SSRF guard, and size/redirect/timeout caps verbatim. --- -## The `save` argument (core design decision) +## Two tools (core design decision) -The tool exposes a single behavioral switch, `save: bool = false`, which selects between two modes: +The feature ships as **two dedicated tools** rather than one tool with a mode flag. Each has one job and one stable return contract: -| | `save=false` (default) — **load into context** | `save=true` — **persist to workspace** | +| | `internal_web_fetch(url)` — **load into context** | `internal_file_download(url)` — **persist to workspace** | |---|---|---| -| **What the agent gets back** | The fetched text inline in the tool result | The saved workspace-relative path (+ a short text preview when textual) | -| **File written?** | No | Yes — durable DIAL file via `AttachmentService` | +| **Returns** | The fetched text inline in the tool result | The saved workspace-relative path (+ a short text preview when textual) | +| **File written?** | No | Yes — durable DIAL file under the agent home | +| **Content types** | Textual only | Any (text + binary) | +| **Oversized text** | Rejected by the size guard — error pointing at `internal_file_download` | Saved; ranged reading is then `internal_file_read_lines` / `internal_file_search` | | **Best for** | Reading a page/README/code file once | Large files, binary files, or content the agent will read/search repeatedly with the file tools | -| **Non-text/binary content** | Rejected — parameter error pointing at `save=true` (bytes can't be inlined) | Saved as-is; path returned | -| **Oversized text** | Rejected by the size guard — error pointing at `save=true` | Saved; ranged reading is then `internal_file_read_lines` / `internal_file_search` | +| **Lives in** | the `internal_tooling` module (plain `StagedBaseTool`) | the `internal_file_*` family (`_DialFileTool` subclass) | +| **Enabled by** | `features.web_fetch.enabled` | the short-name `download` in `features.dial_files.enabled_tools` | -This switch is what makes the tool self-contained: **`save=true` is the large-content and binary-content story**, so the load path needs no pagination, no offload dependency, and no persistence — only a guard rail. +**Why two tools, not one flag:** the two behaviors have genuinely different return shapes (inline text vs. a path) and different input domains (textual-only + size-capped vs. any content). A single tool whose output schema and preconditions depend on a boolean is harder for the model to use and inconsistent with the file family, which has no behavior-flagged tools. Splitting on the verb (**fetch** = read it now, **download** = keep it) mirrors the browser mental model and lets each tool be enabled independently. + +The two tools share the fetch/decode/classify logic via a small helper (Component 3); they differ only in what they do with the result. --- ## Use Cases -### UC-1: Agent fetches a text resource and reads it in one call (default) +### UC-1: Agent fetches a text resource and reads it in one call -**Trigger:** Agent calls `internal_file_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` (default `save=false`). +**Trigger:** Agent calls `internal_web_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")`. **Behavior:** The tool fetches the bytes through `ExternalUrlFetcher` and, because the content is textual and within the size guard, returns the decoded text inline (code-block wrapped). No file is written. @@ -55,39 +60,39 @@ This switch is what makes the tool self-contained: **`save=true` is the large-co ### UC-2: Agent pulls a resource into the workspace for later use -**Trigger:** Agent calls `internal_file_fetch(url="https://…/data.py", save=true)`. +**Trigger:** Agent calls `internal_file_download(url="https://…/data.py")`. -**Behavior:** The tool fetches the bytes and persists them as a DIAL file in the agent's workspace via `AttachmentService`. The result reports the saved workspace-relative path plus a short text preview (for textual content). +**Behavior:** The tool fetches the bytes and persists them as a DIAL file under the agent home via the file-family write path. The result reports the saved workspace-relative path plus a short text preview (for textual content). **Outcome:** The model gets the path and a preview, and can now read or search the file with the existing `internal_file_*` tools. -### UC-3: Agent saves, then chains a file tool +### UC-3: Agent downloads, then chains a file tool -**Trigger:** After UC-2, agent calls `internal_file_search(path="files/…/data.py", query="def main")`. +**Trigger:** After UC-2, agent calls `internal_file_search(path="data.py", query="def main")` (the relative path the download returned). **Behavior:** The search tool operates on the persisted file exactly as it would on any uploaded file. -**Outcome:** Fetch composes with the rest of the file family with no special-casing. +**Outcome:** Download composes with the rest of the file family with no special-casing. -### UC-4: Agent loads a large text file without saving +### UC-4: Agent tries to load a large text file into context -**Trigger:** Agent calls `internal_file_fetch(url="https://…/huge.log")` with default `save=false`, and the decoded text exceeds the tool's size guard. +**Trigger:** Agent calls `internal_web_fetch(url="https://…/huge.log")` and the decoded text exceeds the size guard. -**Behavior:** The tool does **not** truncate-and-guess. It returns a parameter error explaining the content is too large to load inline and instructing the agent to retry with `save=true` (then read ranges via the file tools). +**Behavior:** The tool does **not** truncate-and-guess. It returns a parameter error explaining the content is too large to load inline and instructing the agent to use `internal_file_download` (then read ranges via the file tools). **Outcome:** The agent gets a clear, actionable next step; the context window is protected. -### UC-5: Agent fetches non-text / binary content +### UC-5: Agent encounters non-text / binary content -**Trigger:** Agent calls `internal_file_fetch` on a URL whose content type is non-textual (image, zip, PDF, …). +**Trigger:** Agent calls a fetch tool on a URL whose content type is non-textual (image, zip, PDF, …). -**Behavior:** With `save=false`, the tool returns a parameter error: binary content cannot be loaded into context; retry with `save=true`. With `save=true`, the bytes are saved and the path + content type + size are returned (no inline body, no extraction in phase-1). +**Behavior:** `internal_web_fetch` returns a parameter error: binary content cannot be loaded into context; use `internal_file_download`. `internal_file_download` saves the bytes and returns the path + content type + size (no inline body, no extraction in phase-1). **Outcome:** The model never receives garbled bytes in context; binary is reachable only as a saved file it can forward to a capable deployment/tool. ### UC-6: External egress is disabled (admin cap or per-app opt-out) -**Trigger:** Agent calls `internal_file_fetch` on an external URL while `EXTERNAL_URL_FETCH_ENABLED=false` (or the app set `features.external_url_fetch.enabled=false`, or the host is outside the allowlist). +**Trigger:** Agent calls either fetch tool on an external URL while `EXTERNAL_URL_FETCH_ENABLED=false` (or the app set `features.external_url_fetch.enabled=false`, or the host is outside the allowlist). **Behavior:** `ExternalUrlFetcher.fetch` raises `ExternalFetchDisabledError`; the tool wraps it as a parameter-scoped tool error carrying the existing operator/builder/allowlist message. @@ -95,11 +100,19 @@ This switch is what makes the tool self-contained: **`save=true` is the large-co ### UC-7: Agent passes a DIAL URL -**Trigger:** Agent calls `internal_file_fetch(url="files//doc.md")`. +**Trigger:** Agent calls either fetch tool with `url="files//doc.md"`. **Behavior:** The URL is classified as DIAL (already in the workspace). The tool returns a parameter error pointing the agent at the existing file tools. -**Outcome:** The fetch tool stays focused on *external* retrieval; in-workspace reads remain the job of `internal_file_read_lines` / `internal_file_search`. +**Outcome:** The fetch tools stay focused on *external* retrieval; in-workspace reads remain the job of `internal_file_read_lines` / `internal_file_search`. + +### UC-8: App enables only one of the two tools + +**Trigger:** A security-sensitive app exposes `internal_web_fetch` but not `internal_file_download` (or vice versa). + +**Behavior:** The two are controlled by independent feature switches (Component 5) — `internal_web_fetch` by `features.web_fetch.enabled`, `internal_file_download` by the short-name `download` in `features.dial_files.enabled_tools`. Setting one and omitting the other exposes only that tool to the LLM. + +**Outcome:** Read-into-context can be allowed without granting the ability to write fetched files into the workspace — a capability split a single flagged tool could not offer. --- @@ -107,65 +120,83 @@ This switch is what makes the tool self-contained: **`save=true` is the large-co ```mermaid flowchart TD - A["LLM: internal_file_fetch(url, save)"] --> B{"classify_url"} - B -->|DIAL| E1["parameter error:
use internal_file_read_lines / _search"] - B -->|unsupported| E2["unsupported-scheme error"] - B -->|EXTERNAL| C["ExternalUrlFetcher.fetch(url)
egress policy + SSRF + size/redirect/timeout"] - C -->|"ExternalFetchDisabledError / ExternalFetchError"| E3["parameter error
(existing messages)"] - C -->|"FetchedBytes: data, content_type, filename"| M{"save?"} - M -->|"false (load)"| L{"textual & within size guard?"} - L -->|no| E4["parameter error:
too large / binary → retry with save=true"] + subgraph shared["shared fetch helper (Component 3)"] + direction TB + B{"classify_url"} -->|DIAL| E1["parameter error:
use internal_file_read_lines / _search"] + B -->|unsupported| E2["unsupported-scheme error"] + B -->|EXTERNAL| C["ExternalUrlFetcher.fetch(url)
egress + SSRF + size/redirect/timeout"] + C -->|"ExternalFetchDisabledError / ExternalFetchError"| E3["parameter error
(existing messages)"] + end + + WF["internal_web_fetch(url)"] --> B + C -->|"FetchedBytes"| L{"textual & within
size guard?"} + L -->|no| E4["parameter error:
too large / binary → use internal_file_download"] L -->|yes| LR["ToolCallResult: inline text
(no file written)"] - M -->|"true (persist)"| P["AttachmentService.upload_bytes → workspace file"] - P --> PR["ToolCallResult: saved path
(+ text preview if textual)"] + + DL["internal_file_download(url)"] --> B + C -->|"FetchedBytes"| P["resolve agent-home URL +
DialFileService write (file-family pattern)"] + P --> PR["ToolCallResult: saved relative path
(+ text preview if textual)"] ``` -### Component 1: The `internal_file_fetch` tool +### Component 1: `internal_web_fetch` (load into context) + +- **What:** a new internal tool, `internal_web_fetch`, in the existing `internal_tooling` module (alongside the Python interpreter). A plain `StagedBaseTool` (it produces no file, so it does **not** need `_DialFileTool`). Depends on the shared fetch helper (Component 3). Enabled by `features.web_fetch.enabled` (Component 5), not by an `InternalToolSet` entry. +- **Arguments:** `url: str` (required) — the http(s) URL to fetch. +- **Semantics:** run the shared helper to classify + fetch. Then require textual content (Component 3) within the size guard (Component 2). If either fails → parameter error directing the agent to `internal_file_download` (UC-4/UC-5). Otherwise return the decoded text inline (code-block wrapped, consistent with the file-tool stage formatting). +- **Return shape:** always inline text in `ToolCallResult.content`; never an attachment, never a path. + +### Component 2: `internal_file_download` (persist to workspace) -- **What:** a new internal tool, `internal_file_fetch`, joining the `internal_file_*` family. It subclasses the shared file-tool base (`_DialFileTool`) so it reuses the family's workspace-path conventions and stage handling; it additionally depends on `ExternalUrlFetcher` (secure fetch) and `AttachmentService` (persist bytes when `save=true`). -- **Owner:** the tool itself orchestrates classify → fetch → (load | persist) → render. It owns no policy logic. -- **Arguments:** - - `url: str` (required) — the http(s) URL to fetch. - - `save: bool = false` (optional) — `false` loads content into context; `true` persists a workspace file. See the table above. -- **Semantics (runtime):** - 1. **Classify** the URL (`classify_url`, reused from the external-fetch machinery). `DIAL` → parameter error (UC-7); unsupported scheme → unsupported-scheme error. - 2. **Fetch** via `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This call is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `ExternalFetchDisabledError` / `ExternalFetchError` are caught and re-raised as parameter-scoped tool errors, matching `FileLoaderService` / `DialFilePromoter`. - 3. **Branch on `save`:** - - **`save=false`:** require textual content (Component 3) within the size guard (Component 2). If either fails → parameter error directing the agent to `save=true` (UC-4/UC-5). Otherwise return the decoded text inline. - - **`save=true`:** persist via `AttachmentService.upload_bytes(data, content_type, filename)` → `FileMetadata`, into the workspace root the file tools address (Component 4). Return the saved path (+ a short text preview when textual). -- **Change:** purely additive; no existing tool is modified. The large-tool-response offload processor is **not** involved — this tool is self-contained by design. +- **What:** a new internal tool, `internal_file_download`, joining the `internal_file_*` family in `dial_files_tooling/`. Subclasses `_DialFileTool` (`dial_files_tooling/_base_file_tool.py`) to reuse the family's home-path resolution, write path, and stage handling. Depends on the shared fetch helper (Component 3). +- **Arguments:** `url: str` (required) — the http(s) URL to fetch. +- **Semantics:** run the shared helper to classify + fetch (any content type — no textual restriction, no size guard). Then persist via the file-family write path — resolve a target URL under the agent home (`_resolve_appdata_url`), write the bytes through `DialFileService` exactly as `_WriteFileTool` does (Component 4). Return the saved **relative** path (+ a short text preview when textual). +- **Return shape:** always a saved relative path (+ optional preview) in `ToolCallResult.content`; never sets `result.attachments` (see Component 4, "No user-choice propagation"). -> **Note on reuse.** `DialFilePromoter.promote(url)` already does "fetch + upload" but discards the bytes and returns only metadata; it also doesn't guarantee the file-family workspace root or produce a preview. The `save=true` path therefore calls `ExternalUrlFetcher.fetch` + `AttachmentService.upload_bytes` directly (the same two steps `promote` performs internally), fetching once and keeping the bytes for the preview. +### Component 3: Shared fetch helper -### Component 2: Load-mode size guard +- **What:** a small helper shared by both tools, covering the common front half: classify the URL, fetch, decode, and the textual/size predicates. +- **Classification:** `classify_url` (`common/url_classification.py`). `DIAL` → parameter error (UC-7); unsupported scheme → unsupported-scheme error. +- **Fetch:** `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `ExternalFetchDisabledError` / `ExternalFetchError` are caught and re-raised as parameter-scoped tool errors, matching `FileLoaderService` / `DialFilePromoter`. +- **Textual predicate:** derived from `content_type` — `text/*`, `application/json`, `application/xml`, and common source/markup types are textual (decoded with the response charset, falling back to UTF-8 with replacement). Everything else is non-textual. Used by `internal_web_fetch` to gate inlining and by `internal_file_download` to decide whether to attach a preview. +- **Size guard:** a byte cap used **only** by `internal_web_fetch`, **configurable** via `features.web_fetch.max_inline_size` (Component 5). Its default is drawn from the **same env setting that governs the offload threshold's default** (`ToolCallResultOffloadSettings`, 40 KB; `config/dial_files.py`), so out of the box the two are equal and anything `internal_web_fetch` returns inline is below the offloader's trigger — the "Read in one call" and "Bounded output" goals hold without the global processor silently rewriting the result (see Component 4a for the cases where an operator decouples the two). On exceed → parameter error, no silent truncation, no pagination (the download tool + `internal_file_read_lines`/`internal_file_search` are the path to "more than fits inline"). +- **No content-sniffing libraries** in phase-1. -- **What:** a hard, tool-owned upper bound on the bytes the tool will inline when `save=false`. -- **Semantics:** if the fetched content (when `save=false`) exceeds the guard, the tool returns a parameter error — it does **not** silently truncate — telling the agent to retry with `save=true` and read ranges via the file tools. -- **Why a hard guard, not truncation or pagination:** a silent truncation hides data from the model; pagination would duplicate `internal_file_read_lines` / `internal_file_search`, which already provide ranged access on a saved file. `save=true` is the deliberate, agent-visible path to "more than fits inline." -- **Default:** a single server-side byte cap (proposed default ~100 KB), defined as a constant/setting. Independent of the offload feature's threshold. +### Component 4: Workspace placement (applies to `internal_file_download`) -### Component 3: Textual vs. non-textual classification +- **What:** the file must be written under the agent-home root the `internal_file_*` tools address — `files/{appdata}/{agent_home_dir}/…` — and the tool must return the **workspace-relative path** (not an absolute DIAL URL). +- **How:** reuse `_DialFileTool`'s `_resolve_appdata_url` (`dial_files_tooling/_base_file_tool.py:138`) to turn a target filename into the home-prefixed URL, then write via `DialFileService` — the same two steps `_WriteFileTool` performs (`dial_files_tooling/_write_file_tool.py:27`). Report the relative path back via `_to_display_path` (`_base_file_tool.py:164`) so it round-trips into `internal_file_read_lines` / `internal_file_search`. +- **Why not `AttachmentService.upload_bytes`:** it targets the flat bucket root `files/{bucket}/{filename}` (`dial_core_services/attachment_service.py:51`), which the file tools cannot resolve by relative path (they resolve under the agent-home prefix). This is the load-bearing decision for UC-3. +- **Target filename:** derive from `FetchedBytes.filename` (already sanitized from Content-Disposition / URL path / MIME extension by `ExternalUrlFetcher`); on collision, follow the file family's existing overwrite/uniqueness convention. +- **Binary write nuance (implementation):** `DialFileService.write_file` is text-oriented (`_upload_text` encodes a `str`). Persisting **binary** content under the home root needs a bytes-capable write into the *resolved home URL* — confirm `DialFileService` exposes one, or add a thin bytes-write that targets the resolved URL (never the flat bucket path). Covered by the binary-download test. +- **No user-choice propagation:** `internal_file_download` returns the path (+ preview) in `ToolCallResult.content` and deliberately does **not** set `result.attachments`. Note this **diverges from `_WriteFileTool`**, which *does* return `attachments=[attachment]` (`_write_file_tool.py:48-52`); the `StagedBaseTool` choice-propagation path (`staged_base_tool.py:176`) then forwards those whose type matches the tool config's `propagate_types_to_choice`. By leaving `attachments` empty, `download` keeps the fetched file out of the user-visible choice — consistent with deferring `propagate_to_choice` (Out of Scope). Surfacing the downloaded file to the user the way `write_file` does is a deliberate future option, not phase-1. +- **Verification:** add a test that downloads then `internal_file_list`/`read_lines` the returned relative path. -- **What:** the predicate that decides whether content can be loaded inline (`save=false`) and whether a preview is produced (`save=true`). -- **Semantics:** derived from the fetched `content_type` — `text/*`, `application/json`, `application/xml`, and the common source/markup types are treated as textual (decoded with the response charset, falling back to UTF-8 with replacement). Everything else is non-textual. -- **Change:** a small helper local to the tool; no dependency on content-sniffing libraries in phase-1. +### Component 4a: Interaction with the global offload processor -### Component 4: Workspace placement (applies to `save=true`) +- **Context:** when the dial-files offload sub-feature is configured, `ToolCallResultOffloadProcessor` is registered (`DialFilesToolingModule._provide_offload_processors`) and applied to **every** tool result in `ToolExecutor.__process_result` (`core/agent/tool_executor.py:56`) — it is not something a tool opts into. When a result's `content` exceeds the threshold (default 40 KB, `config/dial_files.py`), it is offloaded to a DIAL file and replaced with a notice. If the offload feature is not configured, no such processor exists. +- **Reconciliation:** neither tool **depends on** offload — `internal_file_download` is the explicit large-content path. Out of the box the `web_fetch` size guard and the offload threshold share a default, so there is no overlap. Both thresholds are independently configurable, so an operator can decouple them in two ways — each a documented, opt-in trade-off, never a silent contradiction: + - **Raising `max_inline_size` above the offload threshold:** `web_fetch` results between the two values would be offloaded by the global processor (turned into a file + notice) rather than returned inline. + - **Lowering the offload threshold below `max_inline_size`:** same effect from the other direction — `web_fetch` results between the two would be offloaded. + - **`internal_file_download`** is unaffected either way: it returns a small path + preview, well under any threshold. +- **Decision:** do **not** special-case either tool against offload in phase-1; the shared default removes the overlap, and the schema docs both knobs so an operator who decouples them does so knowingly. (If an operator wants `web_fetch` exempted regardless, the offload config already supports per-tool `excluded_tools`.) -- **What:** when persisting, the file must be written to the **same workspace root** that the `internal_file_*` tools list and address, and the tool must return the **workspace-relative path** (not an opaque DIAL URL). -- **Why:** the "fetch composes with the file tools" promise (UC-3) depends on `internal_file_search`/`read_lines` finding the file by the path the fetch tool reports. If `AttachmentService.upload_bytes` does not already target that root, the tool must direct the upload there (e.g., a per-conversation/appdata prefix consistent with the file family). -- **Verification:** confirm during implementation where the file family resolves relative paths (`_DialFileTool` path resolution) and ensure the upload targets the same location; add a test that fetches with `save=true` then `internal_file_list`s the returned path. +### Component 5: Tool config, names, DI wiring, and gating -### Component 5: Tool config, name, and DI wiring +Both tools are **feature-gated** (enabled through `features.*`, not through an `InternalToolSet` entry) and **preview-gated** — symmetric by design: -- **Name:** add `INTERNAL_FILE_FETCH_TOOL_NAME = "internal_file_fetch"` to `common/tool_names.py`. -- **Config:** an `InternalTool` entry with the OpenAI function schema (`url` string required; `save` boolean default `false`). Enabled via the existing `internal` tool set like the rest of the family — no new tool-set type. -- **DI:** bind the tool at request scope and dispatch it from the file-tooling module's `@multiprovider` by matching the tool name (the established pattern for the family). Nothing new in `app_factory.py` beyond what the file-tooling module already provides. +- **Names:** add `INTERNAL_WEB_FETCH_TOOL_NAME = "internal_web_fetch"` to `common/tool_names.py`; `internal_file_download` follows the family's `INTERNAL_FILE_TOOL_NAME_PREFIX` + short-name `download`. +- **`internal_web_fetch`:** + - New feature config `WebFetchConfig` (`enabled: bool = false`, `max_inline_size: int` defaulting to the offload threshold) added to the `Features` model (`config/application.py:169`) as a `PreviewField` — so configuring it requires `ENABLE_PREVIEW_FEATURES`, matching `dial_files`. + - Provided from the **existing** `InternalToolModule` `@multiprovider`, which gains a branch that builds the tool when `features.web_fetch.enabled` is true (reading `max_inline_size` from the same config). No new module, no `app_factory` change. +- **`internal_file_download` (file family):** + - Enabled via `features.dial_files.enabled_tools` — the short-name `download` (or `"all"`), like every other `internal_file_*` tool. The `dial_files_tooling` module strips the `internal_file_` prefix and checks `short_name in cfg.enabled_tools` (`dial_files_tooling_module.py:123`). + - Add `download` to the `DialFilesToolName` literal (defined near the top of `config/dial_files.py`). Dispatched from the existing `dial_files_tooling` `@multiprovider`; bound at request scope. +- **Preview gating:** `DialFilesToolingModule` is already `@preview_module` (`dial_files_tooling_module.py:46`), so `internal_file_download` is gated by `ENABLE_PREVIEW_FEATURES`. Making `WebFetchConfig` a `PreviewField` gives `internal_web_fetch` the same gating — **both graduate together** without `InternalToolModule` itself needing to be preview-gated (the py-interpreter tool stays unaffected). - **Schema:** run `make dump_app_schema` to regenerate `docs/generated-app-schema.json` and `docs/generated-internal-tools.json`. ### Component 6: Egress policy (reused, unchanged) -- No new policy code. The two-tier gate (`EXTERNAL_URL_FETCH_ENABLED` + `features.external_url_fetch.enabled`), host allowlists, and SSRF guard are enforced inside `ExternalUrlFetcher.fetch`. The tool's only responsibility is to surface the resulting errors clearly (UC-6). +- No new policy code. The two-tier gate (`EXTERNAL_URL_FETCH_ENABLED` + `features.external_url_fetch.enabled`), host allowlists, and SSRF guard are enforced inside `ExternalUrlFetcher.fetch`. Each tool's only responsibility is to surface the resulting errors clearly (UC-6). --- @@ -173,11 +204,11 @@ flowchart TD Deferred from phase-1; each is a clean follow-on, not a rework: -- **PDF / binary text extraction.** Binary content is save-only (no inline, no extraction) in phase-1. Extraction needs a parser and a preview strategy; future phase. -- **Reliance on the large-tool-response offload processor.** Intentionally avoided to keep the tool self-contained; `save=true` covers the large-content case explicitly. -- **Load-mode pagination / `start_index`.** Unnecessary — `save=true` + `internal_file_read_lines` / `internal_file_search` provide ranged access; load mode is for content that fits inline. +- **PDF / binary text extraction.** Binary content is download-only (no inline, no extraction) in phase-1. Extraction needs a parser and a preview strategy; future phase. +- **Special-casing the tools against the global offload processor.** Neither tool relies on offload (`internal_file_download` covers large content), but they also do not exempt themselves from the global post-processor in phase-1; see Component 4a. +- **Load-mode pagination / `start_index` on `internal_web_fetch`.** Unnecessary — `internal_file_download` + `internal_file_read_lines` / `internal_file_search` provide ranged access; web fetch is for content that fits inline. - **Content summarization** (Claude Code-style prompt-over-content). -- **Surfacing the saved file to the user via `propagate_to_choice`** and richer binary metadata (thumbnails, structured metadata). +- **Surfacing the downloaded file to the user via `propagate_to_choice`** and richer binary metadata (thumbnails, structured metadata). - **Provenance-based URL allowlisting** (Anthropic-style: only fetch URLs that appeared in conversation context, to harden against model-fabricated exfiltration URLs). A worthwhile future hardening; the existing egress policy + host allowlist already gate destinations today. - **DIAL-URL retrieval.** A DIAL URL is rejected with guidance (UC-7) rather than fetched; in-workspace reads stay with the file tools. @@ -185,52 +216,40 @@ Deferred from phase-1; each is a clean follow-on, not a rework: ## Configuration / Usage Examples -### Enabling the tool +### Enabling both tools + +Both tools are turned on through `features.*` (no `InternalToolSet` entry for either) and both require `ENABLE_PREVIEW_FEATURES=true` (preview-gated in phase-1): ```yaml -tool_sets: - - type: internal - tools: - - type: internal-tool - enabled: true - open_ai_tool: - type: function - function: - name: internal_file_fetch - description: >- - Fetch a file or page from a web URL. By default returns the text content - inline. Set save=true to store it as a workspace file (required for large - or binary content) and read it with the other file tools. - parameters: - type: object - properties: - url: - type: string - description: The http(s) URL to fetch. - save: - type: boolean - description: >- - If true, persist the fetched content as a workspace file and return - its path instead of inlining it. Required for binary or oversized content. - default: false - required: [url] +features: + # Enables internal_web_fetch and sets its inline size cap (defaults to the + # offload threshold, 40 KB). + web_fetch: + enabled: true + max_inline_size: 40000 + + # Enables internal_file_download via the file-family short-name list. + dial_files: + enabled_tools: [download, read_lines, search] # or "all" ``` +> Neither tool has an `internal-tool` YAML entry. `internal_web_fetch` is selected by `features.web_fetch.enabled`; `internal_file_download` by its short-name in `features.dial_files.enabled_tools`. + ### Walkthrough — load into context (UC-1) -`internal_file_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` +`internal_web_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` → returns the README text inline. No file written. -### Walkthrough — save then search (UC-2 → UC-3) +### Walkthrough — download then search (UC-2 → UC-3) -1. `internal_file_fetch(url="https://…/data.py", save=true)` - → `saved: files///data.py` + short preview. -2. `internal_file_search(path="files///data.py", query="def main")` +1. `internal_file_download(url="https://…/data.py")` + → `saved: data.py` (workspace-relative, under the agent home) + short preview. +2. `internal_file_search(path="data.py", query="def main")` → operates on the persisted file with no re-fetch. ### Egress disabled (UC-6) -`internal_file_fetch(url="https://example.com/x")` with `EXTERNAL_URL_FETCH_ENABLED=false` +`internal_web_fetch(url="https://example.com/x")` with `EXTERNAL_URL_FETCH_ENABLED=false` → tool error: *"External URL fetching is disabled by operator policy (EXTERNAL_URL_FETCH_ENABLED)."* --- @@ -239,37 +258,45 @@ tool_sets: ### Breaking changes -None. The tool is purely additive and opt-in via app config. +None. Both tools are purely additive and opt-in via app config. ### Non-breaking changes -- New tool `internal_file_fetch` appears in the generated schema/manifest after `make dump_app_schema`. -- No change to existing tools, the egress policy, the offload feature, or the config shape beyond the new tool entry. +- New tools `internal_web_fetch` and `internal_file_download` appear in the generated schema/manifest after `make dump_app_schema`. +- No change to existing tools, the egress policy, the offload feature, or the config shape beyond the new tool entries. ## Summary of Changes ### New files -- `dial_files_tooling/_fetch_file_tool.py` — the `internal_file_fetch` tool (`_DialFileTool` subclass; depends on `ExternalUrlFetcher` + `AttachmentService`). -- Unit tests under the file-tooling test package. +- `internal_tooling/_web_fetch_tool.py` — the `internal_web_fetch` tool (plain `StagedBaseTool`; depends on the shared fetch helper). +- `dial_files_tooling/_download_file_tool.py` — the `internal_file_download` tool (`_DialFileTool` subclass; persists via the file-family `DialFileService` write path). +- A shared fetch helper (Component 3) — location shared by both (e.g., `shared/external_fetch/` or a small `common/` helper), reused by both tools. +- Unit tests for both tools. ### Modified files -- `common/tool_names.py` — add `INTERNAL_FILE_FETCH_TOOL_NAME`. -- The file-tooling DI module — bind the tool and dispatch it from the existing `@multiprovider`; add the tool's `InternalTool` config definition. +- `common/tool_names.py` — add `INTERNAL_WEB_FETCH_TOOL_NAME` (the download tool uses the family prefix + short-name `download`). +- `config/application.py` — add `WebFetchConfig` and a `web_fetch` `PreviewField` to the `Features` model (`enabled: bool`, `max_inline_size: int` defaulting to the offload threshold). +- `internal_tooling/internal_tooling_module.py` — extend the `@multiprovider` to build `internal_web_fetch` when `features.web_fetch.enabled`. No `app_factory` change. +- `config/dial_files.py` — add `download` to the `DialFilesToolName` literal so it can appear in `enabled_tools`. +- `dial_files_tooling` DI module — bind `internal_file_download` and dispatch it from the existing `@multiprovider`; add its tool definition to the family. +- `dial_core_services/dial_file_service.py` — if no bytes-capable write exists, add a thin one targeting a resolved home URL (Component 4 binary nuance). - `docs/generated-app-schema.json`, `docs/generated-internal-tools.json` — regenerated. ### Tools exposed to the LLM -- `internal_file_fetch(url, save=false)` — fetch an external resource. Default returns text inline; `save=true` persists a workspace file and returns its path. +- `internal_web_fetch(url)` — fetch a text resource and return it inline (text only; binary/oversize → directs to download). +- `internal_file_download(url)` — download any resource into the workspace (under the agent home) and return its relative path (+ preview when textual). ### Tests -- `save=false`, textual, within guard → full inline content, no file written. -- `save=false`, textual, over size guard → parameter error pointing at `save=true`. -- `save=false`, binary → parameter error pointing at `save=true`. -- `save=true`, textual → persisted, path + preview returned. -- `save=true`, binary → persisted, path + content type + size returned, no inline body. -- `save=true` then `internal_file_list`/`read_lines` on the returned path (workspace-placement guarantee). -- Egress disabled / host not allowed → parameter error with the policy message. -- DIAL URL → parameter error pointing to the file tools. +- **web_fetch:** textual within guard → full inline content, no file written. +- **web_fetch:** textual over size guard → parameter error pointing at `internal_file_download`. +- **web_fetch:** binary → parameter error pointing at `internal_file_download`. +- **download:** textual → persisted under agent home, relative path + preview returned. +- **download:** binary → persisted under agent home, relative path + content type + size returned, no inline body. +- **download** then `internal_file_list`/`read_lines` on the returned relative path (workspace-placement guarantee). +- **both:** egress disabled / host not allowed → parameter error with the policy message. +- **both:** DIAL URL → parameter error pointing to the file tools. +- **config:** enabling only one tool exposes only that tool (UC-8). From 6574f90224d1de7789c3af7570daa9b9df054d17 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Thu, 2 Jul 2026 16:12:39 +0300 Subject: [PATCH 03/12] docs: home internal_web_fetch in a dedicated web_tooling module (#344) Move internal_web_fetch out of the deprecated internal_tooling module into a new self-contained, feature-gated web_tooling module. Drop module-name references to other in-flight work and note the prerequisite PRs (#368, #393) that merge first: the file-family subclass API (_resolve_appdata_url / _to_display_path) that internal_file_download relies on is preserved by #368, and edits to the shared wiring files are additive alongside both PRs. --- docs/designs/web_fetch_tool.md | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md index 10f59362..61aaf7c5 100644 --- a/docs/designs/web_fetch_tool.md +++ b/docs/designs/web_fetch_tool.md @@ -4,6 +4,7 @@ - **Dependencies:** - [external_url_attachments.md](external_url_attachments.md) — `ExternalUrlFetcher`, `classify_url`, and the two-tier egress policy both tools reuse unchanged. - [dial_files_tools.md](dial_files_tools.md) / [dial_files_search.md](dial_files_search.md) — the `internal_file_*` tool family `internal_file_download` joins, its `_DialFileTool` base, and the agent-home workspace a downloaded file lands in. + - **Prerequisite PRs (merge first): #368 and #393.** #344 rebases onto a base that already includes PR #368 (dial-files home-resolution refactor — the `_DialFileTool` subclass API `_resolve_appdata_url` / `_to_display_path` is preserved, and `_WriteFileTool` now sets the real `content_type`) and PR #393 (which adds another preview-gated built-in tool via its own feature flag). #344's edits to the shared wiring files — `config/application.py` (`Features`), `common/tool_names.py`, `app_factory.py`, `src/scripts/dump_internal_tools.py` — are **additive** alongside those PRs' edits to the same files. ## Problem Statement @@ -39,7 +40,7 @@ The feature ships as **two dedicated tools** rather than one tool with a mode fl | **Content types** | Textual only | Any (text + binary) | | **Oversized text** | Rejected by the size guard — error pointing at `internal_file_download` | Saved; ranged reading is then `internal_file_read_lines` / `internal_file_search` | | **Best for** | Reading a page/README/code file once | Large files, binary files, or content the agent will read/search repeatedly with the file tools | -| **Lives in** | the `internal_tooling` module (plain `StagedBaseTool`) | the `internal_file_*` family (`_DialFileTool` subclass) | +| **Lives in** | a new `web_tooling` module (plain `StagedBaseTool`) | the `internal_file_*` family (`_DialFileTool` subclass) | | **Enabled by** | `features.web_fetch.enabled` | the short-name `download` in `features.dial_files.enabled_tools` | **Why two tools, not one flag:** the two behaviors have genuinely different return shapes (inline text vs. a path) and different input domains (textual-only + size-capped vs. any content). A single tool whose output schema and preconditions depend on a boolean is harder for the model to use and inconsistent with the file family, which has no behavior-flagged tools. Splitting on the verb (**fetch** = read it now, **download** = keep it) mirrors the browser mental model and lets each tool be enabled independently. @@ -140,7 +141,7 @@ flowchart TD ### Component 1: `internal_web_fetch` (load into context) -- **What:** a new internal tool, `internal_web_fetch`, in the existing `internal_tooling` module (alongside the Python interpreter). A plain `StagedBaseTool` (it produces no file, so it does **not** need `_DialFileTool`). Depends on the shared fetch helper (Component 3). Enabled by `features.web_fetch.enabled` (Component 5), not by an `InternalToolSet` entry. +- **What:** a new built-in tool, `internal_web_fetch`, in a **new dedicated `web_tooling` module** (`WebToolingModule`, `@preview_module`) — a self-contained, feature-gated module (the standard shape for a standalone built-in tool: `configure` binding + a `@multiprovider` that reads its feature config). A plain `StagedBaseTool` (it produces no file, so it does **not** need `_DialFileTool`). Depends on the shared fetch helper (Component 3). Enabled by `features.web_fetch.enabled` (Component 5). - **Arguments:** `url: str` (required) — the http(s) URL to fetch. - **Semantics:** run the shared helper to classify + fetch. Then require textual content (Component 3) within the size guard (Component 2). If either fails → parameter error directing the agent to `internal_file_download` (UC-4/UC-5). Otherwise return the decoded text inline (code-block wrapped, consistent with the file-tool stage formatting). - **Return shape:** always inline text in `ToolCallResult.content`; never an attachment, never a path. @@ -149,7 +150,7 @@ flowchart TD - **What:** a new internal tool, `internal_file_download`, joining the `internal_file_*` family in `dial_files_tooling/`. Subclasses `_DialFileTool` (`dial_files_tooling/_base_file_tool.py`) to reuse the family's home-path resolution, write path, and stage handling. Depends on the shared fetch helper (Component 3). - **Arguments:** `url: str` (required) — the http(s) URL to fetch. -- **Semantics:** run the shared helper to classify + fetch (any content type — no textual restriction, no size guard). Then persist via the file-family write path — resolve a target URL under the agent home (`_resolve_appdata_url`), write the bytes through `DialFileService` exactly as `_WriteFileTool` does (Component 4). Return the saved **relative** path (+ a short text preview when textual). +- **Semantics:** run the shared helper to classify + fetch (any content type — no textual restriction, no size guard). Then persist via the file-family write path — resolve a target URL under the agent home (the family's home-path resolution; see Component 4), write the bytes through `DialFileService` exactly as `_WriteFileTool` does. Return the saved **relative** path (+ a short text preview when textual). - **Return shape:** always a saved relative path (+ optional preview) in `ToolCallResult.content`; never sets `result.attachments` (see Component 4, "No user-choice propagation"). ### Component 3: Shared fetch helper @@ -164,7 +165,8 @@ flowchart TD ### Component 4: Workspace placement (applies to `internal_file_download`) - **What:** the file must be written under the agent-home root the `internal_file_*` tools address — `files/{appdata}/{agent_home_dir}/…` — and the tool must return the **workspace-relative path** (not an absolute DIAL URL). -- **How:** reuse `_DialFileTool`'s `_resolve_appdata_url` (`dial_files_tooling/_base_file_tool.py:138`) to turn a target filename into the home-prefixed URL, then write via `DialFileService` — the same two steps `_WriteFileTool` performs (`dial_files_tooling/_write_file_tool.py:27`). Report the relative path back via `_to_display_path` (`_base_file_tool.py:164`) so it round-trips into `internal_file_read_lines` / `internal_file_search`. +- **How:** reuse the file family's **home-path resolution** to turn a target filename into the home-prefixed URL, then write via `DialFileService` — the same two steps `_WriteFileTool` performs. Report the relative path back via the family's display-path helper so it round-trips into `internal_file_read_lines` / `internal_file_search`. + - **Subclass API is stable:** a `_DialFileTool` subclass calls `self._resolve_appdata_url` and `self._to_display_path`. PR #368 (merging first) refactors the home-resolution internals but **keeps these subclass-facing methods unchanged**, so the download tool uses them as-is. The behavior is: resolve under `files/{appdata}/{agent_home_dir}/…`, report the relative path. - **Why not `AttachmentService.upload_bytes`:** it targets the flat bucket root `files/{bucket}/{filename}` (`dial_core_services/attachment_service.py:51`), which the file tools cannot resolve by relative path (they resolve under the agent-home prefix). This is the load-bearing decision for UC-3. - **Target filename:** derive from `FetchedBytes.filename` (already sanitized from Content-Disposition / URL path / MIME extension by `ExternalUrlFetcher`); on collision, follow the file family's existing overwrite/uniqueness convention. - **Binary write nuance (implementation):** `DialFileService.write_file` is text-oriented (`_upload_text` encodes a `str`). Persisting **binary** content under the home root needs a bytes-capable write into the *resolved home URL* — confirm `DialFileService` exposes one, or add a thin bytes-write that targets the resolved URL (never the flat bucket path). Covered by the binary-download test. @@ -182,16 +184,16 @@ flowchart TD ### Component 5: Tool config, names, DI wiring, and gating -Both tools are **feature-gated** (enabled through `features.*`, not through an `InternalToolSet` entry) and **preview-gated** — symmetric by design: +Both tools are **feature-gated** (enabled through `features.*`, not through a per-tool tool-set entry) and **preview-gated** — symmetric by design: - **Names:** add `INTERNAL_WEB_FETCH_TOOL_NAME = "internal_web_fetch"` to `common/tool_names.py`; `internal_file_download` follows the family's `INTERNAL_FILE_TOOL_NAME_PREFIX` + short-name `download`. - **`internal_web_fetch`:** - New feature config `WebFetchConfig` (`enabled: bool = false`, `max_inline_size: int` defaulting to the offload threshold) added to the `Features` model (`config/application.py:169`) as a `PreviewField` — so configuring it requires `ENABLE_PREVIEW_FEATURES`, matching `dial_files`. - - Provided from the **existing** `InternalToolModule` `@multiprovider`, which gains a branch that builds the tool when `features.web_fetch.enabled` is true (reading `max_inline_size` from the same config). No new module, no `app_factory` change. + - Provided by a **new `WebToolingModule`** (`web_tooling/web_tooling_module.py`, `@preview_module`) via its own `@multiprovider`, which builds the tool when `features.web_fetch.enabled` is true (reading `max_inline_size` from the same config). Registered in `app_factory.py`. - **`internal_file_download` (file family):** - Enabled via `features.dial_files.enabled_tools` — the short-name `download` (or `"all"`), like every other `internal_file_*` tool. The `dial_files_tooling` module strips the `internal_file_` prefix and checks `short_name in cfg.enabled_tools` (`dial_files_tooling_module.py:123`). - Add `download` to the `DialFilesToolName` literal (defined near the top of `config/dial_files.py`). Dispatched from the existing `dial_files_tooling` `@multiprovider`; bound at request scope. -- **Preview gating:** `DialFilesToolingModule` is already `@preview_module` (`dial_files_tooling_module.py:46`), so `internal_file_download` is gated by `ENABLE_PREVIEW_FEATURES`. Making `WebFetchConfig` a `PreviewField` gives `internal_web_fetch` the same gating — **both graduate together** without `InternalToolModule` itself needing to be preview-gated (the py-interpreter tool stays unaffected). +- **Preview gating:** `DialFilesToolingModule` is already `@preview_module` (`dial_files_tooling_module.py:46`), so `internal_file_download` is gated by `ENABLE_PREVIEW_FEATURES`. `WebToolingModule` is itself `@preview_module`, and `WebFetchConfig` is a `PreviewField` — so `internal_web_fetch` is preview-gated at both the module and config level, and **both tools graduate together**. - **Schema:** run `make dump_app_schema` to regenerate `docs/generated-app-schema.json` and `docs/generated-internal-tools.json`. ### Component 6: Egress policy (reused, unchanged) @@ -218,7 +220,7 @@ Deferred from phase-1; each is a clean follow-on, not a rework: ### Enabling both tools -Both tools are turned on through `features.*` (no `InternalToolSet` entry for either) and both require `ENABLE_PREVIEW_FEATURES=true` (preview-gated in phase-1): +Both tools are turned on through `features.*` (no per-tool tool-set entry for either) and both require `ENABLE_PREVIEW_FEATURES=true` (preview-gated in phase-1): ```yaml features: @@ -269,7 +271,9 @@ None. Both tools are purely additive and opt-in via app config. ### New files -- `internal_tooling/_web_fetch_tool.py` — the `internal_web_fetch` tool (plain `StagedBaseTool`; depends on the shared fetch helper). +- `web_tooling/web_tooling_module.py` — `WebToolingModule` (`@preview_module`; `configure` + `@multiprovider`). +- `web_tooling/_web_fetch_tool.py` — the `internal_web_fetch` tool (plain `StagedBaseTool`; depends on the shared fetch helper). +- `web_tooling/_web_fetch_tool_config.py` — the tool's `InternalTool` config (`WEB_FETCH_TOOL_CONFIG`). - `dial_files_tooling/_download_file_tool.py` — the `internal_file_download` tool (`_DialFileTool` subclass; persists via the file-family `DialFileService` write path). - A shared fetch helper (Component 3) — location shared by both (e.g., `shared/external_fetch/` or a small `common/` helper), reused by both tools. - Unit tests for both tools. @@ -278,7 +282,7 @@ None. Both tools are purely additive and opt-in via app config. - `common/tool_names.py` — add `INTERNAL_WEB_FETCH_TOOL_NAME` (the download tool uses the family prefix + short-name `download`). - `config/application.py` — add `WebFetchConfig` and a `web_fetch` `PreviewField` to the `Features` model (`enabled: bool`, `max_inline_size: int` defaulting to the offload threshold). -- `internal_tooling/internal_tooling_module.py` — extend the `@multiprovider` to build `internal_web_fetch` when `features.web_fetch.enabled`. No `app_factory` change. +- `app_factory.py` — register `WebToolingModule`. - `config/dial_files.py` — add `download` to the `DialFilesToolName` literal so it can appear in `enabled_tools`. - `dial_files_tooling` DI module — bind `internal_file_download` and dispatch it from the existing `@multiprovider`; add its tool definition to the family. - `dial_core_services/dial_file_service.py` — if no bytes-capable write exists, add a thin one targeting a resolved home URL (Component 4 binary nuance). From f4d5d825baf6a3a0498c3d215cb1191f7532a116 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Fri, 3 Jul 2026 16:26:18 +0300 Subject: [PATCH 04/12] docs: rework web fetch design to one tool with save_path (#344) Collapse the two-tool proposal (internal_web_fetch + internal_file_download) into a single internal_web_fetch(url, save_path=None): omit save_path to read text inline, provide it to persist under the agent home. Approved. --- docs/designs/web_fetch_tool.md | 311 +++++++++++++++++---------------- 1 file changed, 164 insertions(+), 147 deletions(-) diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md index 61aaf7c5..cca23290 100644 --- a/docs/designs/web_fetch_tool.md +++ b/docs/designs/web_fetch_tool.md @@ -1,10 +1,13 @@ -# Design: Built-in Web Fetch & File Download Tools +# Design: Built-in Web Fetch Tool -- **Status:** Approved +- **Status:** Approved (reworked — supersedes the earlier two-tool proposal; see [Reworked from](#reworked-from)) - **Dependencies:** - - [external_url_attachments.md](external_url_attachments.md) — `ExternalUrlFetcher`, `classify_url`, and the two-tier egress policy both tools reuse unchanged. - - [dial_files_tools.md](dial_files_tools.md) / [dial_files_search.md](dial_files_search.md) — the `internal_file_*` tool family `internal_file_download` joins, its `_DialFileTool` base, and the agent-home workspace a downloaded file lands in. - - **Prerequisite PRs (merge first): #368 and #393.** #344 rebases onto a base that already includes PR #368 (dial-files home-resolution refactor — the `_DialFileTool` subclass API `_resolve_appdata_url` / `_to_display_path` is preserved, and `_WriteFileTool` now sets the real `content_type`) and PR #393 (which adds another preview-gated built-in tool via its own feature flag). #344's edits to the shared wiring files — `config/application.py` (`Features`), `common/tool_names.py`, `app_factory.py`, `src/scripts/dump_internal_tools.py` — are **additive** alongside those PRs' edits to the same files. + - [external_url_attachments.md](external_url_attachments.md) — `ExternalUrlFetcher`, `classify_url`, and the two-tier egress policy the tool reuses unchanged. + - **PR #368 (already merged, `db4450d`).** The dial-files home-resolution refactor extracted `_HomePathResolver` (`dial_files_tooling/_home_path_resolver.py`) into a standalone, request-scoped **injectable** with `resolve_appdata_url` / `resolve_home_dir` / `to_display_path`. Saving reuses that resolver directly (Component 4) instead of subclassing `_DialFileTool` — that extraction is what makes a single independent tool viable. + +### Reworked from + +An earlier revision of this doc (Status: Approved) shipped this capability as **two dedicated tools** — `internal_web_fetch` (inline text) and `internal_file_download` (persist to the workspace, living in the `internal_file_*` family). This revision collapses them into **one independent tool** whose behavior is selected by a single optional `save_path` argument. The rationale for the reversal is in [One tool, one switch](#one-tool-one-switch-core-design-decision). ## Problem Statement @@ -19,41 +22,45 @@ Without it, an agent that needs the *contents* of a web page has no path: extern ## Design Goals -- **Read in one call.** Fetching a text URL returns the content inline in a single tool call — no file written, no follow-up required. -- **Persist for the file tools.** A separate tool pulls a resource into the workspace as a durable DIAL file so the existing `internal_file_*` tools can operate on it. -- **One job per tool.** Each tool has a single purpose and a single, predictable return shape — content, or a path. Consistent with the `internal_file_*` family's design (no behavior-changing flags). -- **Independently enableable.** An app can expose read-into-context without allowing file-writing, or vice versa. -- **Self-contained.** Neither tool depends on the large-tool-response offload processor (which nonetheless still post-processes results platform-wide; see Component 4a). Large/binary content is handled by routing the agent to `internal_file_download`. -- **Bounded output.** `internal_web_fetch` never dumps unbounded content into the LLM context; a hard, tool-owned size guard redirects oversized fetches to `internal_file_download`. -- **Zero new egress surface.** Both fetches go through the existing `ExternalUrlFetcher`, inheriting the two-tier egress policy, host allowlist, SSRF guard, and size/redirect/timeout caps verbatim. +- **Read in one call.** Fetching a text URL returns the content inline in a single tool call — no file written, no follow-up required — with nothing more than a URL. +- **Persist for the file tools, on demand.** The same tool, given a destination, pulls a resource into the workspace as a durable DIAL file so the existing `internal_file_*` tools can operate on it. +- **One capability, one switch.** A single feature flag exposes the whole fetch capability; the agent chooses read-now vs. keep-it per call by supplying (or omitting) one argument. +- **Self-contained.** The tool depends only on the shared fetch helper and the shared home-path resolver — not on the `internal_file_*` family or the large-tool-response offload processor. +- **Bounded inline output.** Inline reads never dump unbounded content into the LLM context; a hard, tool-owned size guard redirects oversized fetches to a save. +- **Zero new egress surface.** Every fetch goes through the existing `ExternalUrlFetcher`, inheriting the two-tier egress policy, host allowlist, SSRF guard, and size/redirect/timeout caps verbatim. --- -## Two tools (core design decision) +## One tool, one switch (core design decision) -The feature ships as **two dedicated tools** rather than one tool with a mode flag. Each has one job and one stable return contract: +The feature ships as **one dedicated tool, `internal_web_fetch(url, save_path=None)`**, living in its own `web_tooling` module. A single optional argument selects what the tool does with the fetched bytes: -| | `internal_web_fetch(url)` — **load into context** | `internal_file_download(url)` — **persist to workspace** | +| | `save_path` **omitted** — load into context | `save_path="…"` — persist to workspace | |---|---|---| | **Returns** | The fetched text inline in the tool result | The saved workspace-relative path (+ a short text preview when textual) | -| **File written?** | No | Yes — durable DIAL file under the agent home | +| **File written?** | No | Yes — durable DIAL file at `save_path` under the agent home | | **Content types** | Textual only | Any (text + binary) | -| **Oversized text** | Rejected by the size guard — error pointing at `internal_file_download` | Saved; ranged reading is then `internal_file_read_lines` / `internal_file_search` | +| **Oversized text** | Rejected by the size guard — error telling the agent to pass a `save_path` | Saved; ranged reading is then `internal_file_read_lines` / `internal_file_search` | | **Best for** | Reading a page/README/code file once | Large files, binary files, or content the agent will read/search repeatedly with the file tools | -| **Lives in** | a new `web_tooling` module (plain `StagedBaseTool`) | the `internal_file_*` family (`_DialFileTool` subclass) | -| **Enabled by** | `features.web_fetch.enabled` | the short-name `download` in `features.dial_files.enabled_tools` | -**Why two tools, not one flag:** the two behaviors have genuinely different return shapes (inline text vs. a path) and different input domains (textual-only + size-capped vs. any content). A single tool whose output schema and preconditions depend on a boolean is harder for the model to use and inconsistent with the file family, which has no behavior-flagged tools. Splitting on the verb (**fetch** = read it now, **download** = keep it) mirrors the browser mental model and lets each tool be enabled independently. +Both branches share the same front half — classify, egress-gate, fetch, decode (Component 3). They diverge only on the presence of `save_path`. + +**Why one tool, not two (reversing the prior revision):** the prior revision split on the verb (*fetch* vs. *download*) to give each behavior a stable, single-shape return. This revision prefers a single tool because: -The two tools share the fetch/decode/classify logic via a small helper (Component 3); they differ only in what they do with the result. +- **One capability, one switch.** Web retrieval is a single trust decision for an app builder — "may this agent reach the public web?" A single `features.web_fetch.enabled` flag models that directly, instead of one flag plus a file-family short-name entry that a builder must know to also set. +- **Independence.** As one self-contained `web_tooling` tool it no longer half-lives in the `internal_file_*` family (`internal_file_download` was a `_DialFileTool` subclass). Nothing in the file family needs to change, and the tool is enableable on its own. +- **The switch is the destination, not a mode enum.** "Fetch this URL; if you also give me a place to put it, I'll save it there" needs no separate `mode` flag — the presence of `save_path` *is* the choice. There is no redundant `mode`/`path` pair to keep consistent, and no "what does `path` mean in context mode?" ambiguity. +- **The return-shape objection is now cheap.** #368 turned home-path resolution into an injectable, so saving is a few lines in the same tool rather than a second class in another module. The presence-dependent return shape is documented in the tool description and both branches are individually testable. + +**Why an explicit `save_path`, not an auto-derived filename:** the tool never guesses where a saved file lands. If the agent wants to keep the resource, it names the destination; this removes the Content-Disposition / URL-basename / hash-placeholder derivation entirely, keeps saves predictable, and makes the returned path something the agent already knows. --- ## Use Cases -### UC-1: Agent fetches a text resource and reads it in one call +### UC-1: Agent fetches a text resource and reads it in one call (default) -**Trigger:** Agent calls `internal_web_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")`. +**Trigger:** Agent calls `internal_web_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` (no `save_path`). **Behavior:** The tool fetches the bytes through `ExternalUrlFetcher` and, because the content is textual and within the size guard, returns the decoded text inline (code-block wrapped). No file is written. @@ -61,59 +68,51 @@ The two tools share the fetch/decode/classify logic via a small helper (Componen ### UC-2: Agent pulls a resource into the workspace for later use -**Trigger:** Agent calls `internal_file_download(url="https://…/data.py")`. - -**Behavior:** The tool fetches the bytes and persists them as a DIAL file under the agent home via the file-family write path. The result reports the saved workspace-relative path plus a short text preview (for textual content). +**Trigger:** Agent calls `internal_web_fetch(url="https://…/data.py", save_path="analysis/data.py")`. -**Outcome:** The model gets the path and a preview, and can now read or search the file with the existing `internal_file_*` tools. +**Behavior:** The tool fetches the bytes and persists them as a DIAL file at `save_path` under the agent home, via the shared home-path resolver + a bytes write. The result reports the saved workspace-relative path plus a short text preview (for textual content). -### UC-3: Agent downloads, then chains a file tool +**Outcome:** The model gets back the saved path. From there the file is a normal workspace artifact: the agent can hand its link to its own model or to a downstream deployment/tool as an attachment, or operate on it with the existing `internal_file_*` tools (read, search, …). How the saved file is subsequently consumed is out of this tool's scope — it only guarantees the file is durably placed where those mechanisms can address it. -**Trigger:** After UC-2, agent calls `internal_file_search(path="data.py", query="def main")` (the relative path the download returned). +### UC-3: Agent tries to load a large text file into context -**Behavior:** The search tool operates on the persisted file exactly as it would on any uploaded file. +**Trigger:** Agent calls `internal_web_fetch(url="https://…/huge.log")` (no `save_path`) and the decoded text exceeds the size guard. -**Outcome:** Download composes with the rest of the file family with no special-casing. - -### UC-4: Agent tries to load a large text file into context - -**Trigger:** Agent calls `internal_web_fetch(url="https://…/huge.log")` and the decoded text exceeds the size guard. - -**Behavior:** The tool does **not** truncate-and-guess. It returns a parameter error explaining the content is too large to load inline and instructing the agent to use `internal_file_download` (then read ranges via the file tools). +**Behavior:** The tool does **not** truncate-and-guess. It returns a parameter error explaining the content is too large to load inline and instructing the agent to re-call with a `save_path` (then read ranges via the file tools). **Outcome:** The agent gets a clear, actionable next step; the context window is protected. -### UC-5: Agent encounters non-text / binary content +### UC-4: Agent encounters non-text / binary content -**Trigger:** Agent calls a fetch tool on a URL whose content type is non-textual (image, zip, PDF, …). +**Trigger:** Agent calls `internal_web_fetch` on a URL whose content type is non-textual (image, zip, PDF, …). -**Behavior:** `internal_web_fetch` returns a parameter error: binary content cannot be loaded into context; use `internal_file_download`. `internal_file_download` saves the bytes and returns the path + content type + size (no inline body, no extraction in phase-1). +**Behavior:** Without `save_path` → parameter error: binary content cannot be loaded into context; re-call with a `save_path`. With `save_path` → the bytes are saved and the tool returns the path + content type + size (no inline body, no extraction in phase-1). **Outcome:** The model never receives garbled bytes in context; binary is reachable only as a saved file it can forward to a capable deployment/tool. -### UC-6: External egress is disabled (admin cap or per-app opt-out) +### UC-5: External egress is disabled (admin cap or per-app opt-out) -**Trigger:** Agent calls either fetch tool on an external URL while `EXTERNAL_URL_FETCH_ENABLED=false` (or the app set `features.external_url_fetch.enabled=false`, or the host is outside the allowlist). +**Trigger:** Agent calls `internal_web_fetch` on an external URL (with or without `save_path`) while `EXTERNAL_URL_FETCH_ENABLED=false` (or the app set `features.external_url_fetch.enabled=false`, or the host is outside the allowlist). **Behavior:** `ExternalUrlFetcher.fetch` raises `ExternalFetchDisabledError`; the tool wraps it as a parameter-scoped tool error carrying the existing operator/builder/allowlist message. **Outcome:** The model gets a clear, actionable refusal — no new policy, no bypass. -### UC-7: Agent passes a DIAL URL +### UC-6: Agent passes a DIAL URL -**Trigger:** Agent calls either fetch tool with `url="files//doc.md"`. +**Trigger:** Agent calls `internal_web_fetch(url="files//doc.md")`. **Behavior:** The URL is classified as DIAL (already in the workspace). The tool returns a parameter error pointing the agent at the existing file tools. -**Outcome:** The fetch tools stay focused on *external* retrieval; in-workspace reads remain the job of `internal_file_read_lines` / `internal_file_search`. +**Outcome:** The fetch tool stays focused on *external* retrieval; in-workspace reads remain the job of `internal_file_read_lines` / `internal_file_search`. -### UC-8: App enables only one of the two tools +### UC-7: App enables or disables the whole capability -**Trigger:** A security-sensitive app exposes `internal_web_fetch` but not `internal_file_download` (or vice versa). +**Trigger:** An app sets `features.web_fetch.enabled=true` (or omits it). -**Behavior:** The two are controlled by independent feature switches (Component 5) — `internal_web_fetch` by `features.web_fetch.enabled`, `internal_file_download` by the short-name `download` in `features.dial_files.enabled_tools`. Setting one and omitting the other exposes only that tool to the LLM. +**Behavior:** The single feature switch (Component 5) exposes or hides `internal_web_fetch` — read-and-save together. Web retrieval is a single capability gated by a single flag. -**Outcome:** Read-into-context can be allowed without granting the ability to write fetched files into the workspace — a capability split a single flagged tool could not offer. +**Outcome:** "May this agent reach the public web?" is one decision. (The prior two-tool revision could enable read-into-context without save independently; that split is intentionally dropped — see Out of Scope.) --- @@ -121,84 +120,92 @@ The two tools share the fetch/decode/classify logic via a small helper (Componen ```mermaid flowchart TD + WF["internal_web_fetch(url, save_path)"] --> B{"classify_url"} + subgraph shared["shared fetch helper (Component 3)"] direction TB - B{"classify_url"} -->|DIAL| E1["parameter error:
use internal_file_read_lines / _search"] + B -->|DIAL| E1["parameter error:
use internal_file_read_lines / _search"] B -->|unsupported| E2["unsupported-scheme error"] B -->|EXTERNAL| C["ExternalUrlFetcher.fetch(url)
egress + SSRF + size/redirect/timeout"] C -->|"ExternalFetchDisabledError / ExternalFetchError"| E3["parameter error
(existing messages)"] end - WF["internal_web_fetch(url)"] --> B - C -->|"FetchedBytes"| L{"textual & within
size guard?"} - L -->|no| E4["parameter error:
too large / binary → use internal_file_download"] + C -->|"FetchedBytes"| M{"save_path given?"} + + M -->|no| L{"textual & within
size guard?"} + L -->|no| E4["parameter error:
too large / binary → re-call with a save_path"] L -->|yes| LR["ToolCallResult: inline text
(no file written)"] - DL["internal_file_download(url)"] --> B - C -->|"FetchedBytes"| P["resolve agent-home URL +
DialFileService write (file-family pattern)"] + M -->|yes| P["HomePathResolver.resolve_appdata_url(save_path) +
DialFileService.write_bytes (unique on collision)"] P --> PR["ToolCallResult: saved relative path
(+ text preview if textual)"] ``` -### Component 1: `internal_web_fetch` (load into context) +### Component 1: `internal_web_fetch` (the single tool) -- **What:** a new built-in tool, `internal_web_fetch`, in a **new dedicated `web_tooling` module** (`WebToolingModule`, `@preview_module`) — a self-contained, feature-gated module (the standard shape for a standalone built-in tool: `configure` binding + a `@multiprovider` that reads its feature config). A plain `StagedBaseTool` (it produces no file, so it does **not** need `_DialFileTool`). Depends on the shared fetch helper (Component 3). Enabled by `features.web_fetch.enabled` (Component 5). -- **Arguments:** `url: str` (required) — the http(s) URL to fetch. -- **Semantics:** run the shared helper to classify + fetch. Then require textual content (Component 3) within the size guard (Component 2). If either fails → parameter error directing the agent to `internal_file_download` (UC-4/UC-5). Otherwise return the decoded text inline (code-block wrapped, consistent with the file-tool stage formatting). -- **Return shape:** always inline text in `ToolCallResult.content`; never an attachment, never a path. +- **What:** a built-in tool, `internal_web_fetch`, in a dedicated `web_tooling` module (`WebToolingModule`, `@preview_module`). A plain `StagedBaseTool` — it does **not** subclass `_DialFileTool`; for saving it injects the shared `HomePathResolver` (Component 4) instead. Depends on the shared fetch helper (Component 3). Enabled by `features.web_fetch.enabled` (Component 5). +- **Arguments:** + - `url: str` (required) — the http(s) URL to fetch. + - `save_path: str` (optional) — the workspace-relative destination for the fetched file (e.g. `data.py`, `docs/readme.md`). **Its presence is the switch:** omit it to read the content inline; provide it to persist the resource there and get back the saved path. Subdirectories are allowed; DIAL-style (`files/…`) paths are rejected (Component 4). +- **Semantics:** run the shared helper to classify + fetch, then branch on whether `save_path` was given: + - **omitted** → require textual content (Component 3) within the size guard (Component 2). If either fails → parameter error telling the agent to re-call with a `save_path` (UC-3/UC-4). Otherwise return the decoded text inline (code-block wrapped, consistent with the file-tool stage formatting). + - **given** → persist any content type at `save_path` under the agent home (Component 4) and return the saved relative path (+ a short preview when textual). No size guard. +- **Return shape:** without `save_path` → inline text in `ToolCallResult.content`. With `save_path` → the saved relative path (+ optional preview) in `ToolCallResult.content`. Never sets `result.attachments` (see Component 4, "No user-choice propagation"). -### Component 2: `internal_file_download` (persist to workspace) +### Component 2: Inline size guard (no `save_path` only) -- **What:** a new internal tool, `internal_file_download`, joining the `internal_file_*` family in `dial_files_tooling/`. Subclasses `_DialFileTool` (`dial_files_tooling/_base_file_tool.py`) to reuse the family's home-path resolution, write path, and stage handling. Depends on the shared fetch helper (Component 3). -- **Arguments:** `url: str` (required) — the http(s) URL to fetch. -- **Semantics:** run the shared helper to classify + fetch (any content type — no textual restriction, no size guard). Then persist via the file-family write path — resolve a target URL under the agent home (the family's home-path resolution; see Component 4), write the bytes through `DialFileService` exactly as `_WriteFileTool` does. Return the saved **relative** path (+ a short text preview when textual). -- **Return shape:** always a saved relative path (+ optional preview) in `ToolCallResult.content`; never sets `result.attachments` (see Component 4, "No user-choice propagation"). +- **What:** a byte cap applied only on the inline (no-`save_path`) branch, **configurable** via `features.web_fetch.max_inline_size` (Component 5). +- **Default:** drawn from the **same env setting that governs the offload threshold's default** (`ToolCallResultOffloadSettings`, 40 KB; `config/dial_files.py`), so out of the box the two are equal and anything the tool returns inline is below the offloader's trigger — the "Read in one call" and "Bounded inline output" goals hold without the global processor silently rewriting the result (see Component 4a for the cases where an operator decouples the two). +- **Comparison is `>=` (at-or-above rejects).** The guard compares the UTF-8 byte length of the decoded text and rejects when `size >= max_inline_size`, so a *returned* result is always **strictly** below the cap. The global offloader offloads when `size >= size_threshold` (`_offload_processor.py`: it skips only when `size < size_threshold`). So at the default (cap == threshold), a returned result — strictly `< cap == threshold` — is strictly below the offloader's `>= threshold` trigger, with no gap between the two operators. The invariant above is thus provable, not merely asserted. +- **On exceed:** parameter error, no silent truncation, no pagination. Re-calling with a `save_path` + `internal_file_read_lines` / `internal_file_search` are the path to "more than fits inline." -### Component 3: Shared fetch helper +### Component 3: Shared fetch helper (`WebContentFetcher`) -- **What:** a small helper shared by both tools, covering the common front half: classify the URL, fetch, decode, and the textual/size predicates. -- **Classification:** `classify_url` (`common/url_classification.py`). `DIAL` → parameter error (UC-7); unsupported scheme → unsupported-scheme error. -- **Fetch:** `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `ExternalFetchDisabledError` / `ExternalFetchError` are caught and re-raised as parameter-scoped tool errors, matching `FileLoaderService` / `DialFilePromoter`. -- **Textual predicate:** derived from `content_type` — `text/*`, `application/json`, `application/xml`, and common source/markup types are textual (decoded with the response charset, falling back to UTF-8 with replacement). Everything else is non-textual. Used by `internal_web_fetch` to gate inlining and by `internal_file_download` to decide whether to attach a preview. -- **Size guard:** a byte cap used **only** by `internal_web_fetch`, **configurable** via `features.web_fetch.max_inline_size` (Component 5). Its default is drawn from the **same env setting that governs the offload threshold's default** (`ToolCallResultOffloadSettings`, 40 KB; `config/dial_files.py`), so out of the box the two are equal and anything `internal_web_fetch` returns inline is below the offloader's trigger — the "Read in one call" and "Bounded output" goals hold without the global processor silently rewriting the result (see Component 4a for the cases where an operator decouples the two). On exceed → parameter error, no silent truncation, no pagination (the download tool + `internal_file_read_lines`/`internal_file_search` are the path to "more than fits inline"). -- **No content-sniffing libraries** in phase-1. +- **What:** a small helper (`shared/external_fetch/web_content_fetcher.py`, already present on the branch) covering the common front half both branches share. Its entry point is `fetch_external(url, parameter_name="url")`, plus the static `is_textual(content_type)` and `decode(data, content_type)` helpers. +- **Classification:** `classify_url` (`common/url_classification.py`) → `UrlScheme.{DIAL,EXTERNAL,UNSUPPORTED}`. `DIAL` → parameter error (UC-6); unsupported scheme → unsupported-scheme error. +- **Fetch:** wraps `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `fetch_external` already catches `ExternalFetchDisabledError` / `ExternalFetchError` and re-raises them as `InvalidToolCallParameterException` on the given `parameter_name`, matching `FileLoaderService` / `DialFilePromoter`. +- **Textual predicate (`is_textual`):** derived from `content_type` — `text/*`, `application/json`, `application/xml`, and common source/markup types are textual (decoded with the response charset, falling back to UTF-8 with replacement). Everything else is non-textual. Used by the inline branch to gate returning text and by the save branch to decide whether to attach a preview. +- **No content-sniffing libraries** in phase-1. `FetchedBytes.filename` is still produced by `ExternalUrlFetcher` but is unused by this tool (saving always uses the caller's `save_path`). -### Component 4: Workspace placement (applies to `internal_file_download`) +### Component 4: Workspace placement (when `save_path` is given) -- **What:** the file must be written under the agent-home root the `internal_file_*` tools address — `files/{appdata}/{agent_home_dir}/…` — and the tool must return the **workspace-relative path** (not an absolute DIAL URL). -- **How:** reuse the file family's **home-path resolution** to turn a target filename into the home-prefixed URL, then write via `DialFileService` — the same two steps `_WriteFileTool` performs. Report the relative path back via the family's display-path helper so it round-trips into `internal_file_read_lines` / `internal_file_search`. - - **Subclass API is stable:** a `_DialFileTool` subclass calls `self._resolve_appdata_url` and `self._to_display_path`. PR #368 (merging first) refactors the home-resolution internals but **keeps these subclass-facing methods unchanged**, so the download tool uses them as-is. The behavior is: resolve under `files/{appdata}/{agent_home_dir}/…`, report the relative path. -- **Why not `AttachmentService.upload_bytes`:** it targets the flat bucket root `files/{bucket}/{filename}` (`dial_core_services/attachment_service.py:51`), which the file tools cannot resolve by relative path (they resolve under the agent-home prefix). This is the load-bearing decision for UC-3. -- **Target filename:** derive from `FetchedBytes.filename` (already sanitized from Content-Disposition / URL path / MIME extension by `ExternalUrlFetcher`); on collision, follow the file family's existing overwrite/uniqueness convention. -- **Binary write nuance (implementation):** `DialFileService.write_file` is text-oriented (`_upload_text` encodes a `str`). Persisting **binary** content under the home root needs a bytes-capable write into the *resolved home URL* — confirm `DialFileService` exposes one, or add a thin bytes-write that targets the resolved URL (never the flat bucket path). Covered by the binary-download test. -- **No user-choice propagation:** `internal_file_download` returns the path (+ preview) in `ToolCallResult.content` and deliberately does **not** set `result.attachments`. Note this **diverges from `_WriteFileTool`**, which *does* return `attachments=[attachment]` (`_write_file_tool.py:48-52`); the `StagedBaseTool` choice-propagation path (`staged_base_tool.py:176`) then forwards those whose type matches the tool config's `propagate_types_to_choice`. By leaving `attachments` empty, `download` keeps the fetched file out of the user-visible choice — consistent with deferring `propagate_to_choice` (Out of Scope). Surfacing the downloaded file to the user the way `write_file` does is a deliberate future option, not phase-1. -- **Verification:** add a test that downloads then `internal_file_list`/`read_lines` the returned relative path. +- **What:** the file must be written under the agent-home root the `internal_file_*` tools address — `files/{appdata}/{agent_home_dir}/…` — and the tool must return the **workspace-relative path** (not an absolute DIAL URL), so the saved file is addressable by the file tools and shareable to the model / downstream deployments by its link. +- **How:** inject the shared `HomePathResolver` (extracted by #368, promoted to `shared/`; Component 5) and call `resolve_appdata_url(save_path)` → home-prefixed URL, then write the bytes via `DialFileService.write_bytes(url, content, content_type, overwrite=False)`, and report the relative path via `to_display_path(url)` so it round-trips into `internal_file_read_lines` / `internal_file_search`. + - **Why inject the resolver, not subclass `_DialFileTool`:** the tool writes exactly one file and needs only home resolution + a bytes write — not the file family's read/scan/stage machinery. Injecting the standalone resolver keeps `web_tooling` independent of the `_DialFileTool` base class. +- **Why not `AttachmentService.upload_bytes`:** it targets the flat bucket root `files/{bucket}/{filename}` (`dial_core_services/attachment_service.py`), which the file tools cannot resolve by relative path (they resolve under the agent-home prefix). Placing the save under the agent home is what lets the returned path round-trip back into the `internal_file_*` tools. +- **Path validation:** `save_path` is passed to `resolve_appdata_url`, which enforces the family's path rules (non-empty, no newlines, no absolute/traversal escape via `validate_relative_path`) and resolves it under the agent home. **A `files/`-prefixed `save_path` is rejected up front** — `resolve_appdata_url` short-circuits and returns any `files/…` path verbatim *without* home-prefixing or traversal validation (`_home_path_resolver.py`), so such a value would escape the agent home and would not round-trip through `to_display_path`. The tool therefore treats a `save_path` that starts with `files/` (or otherwise classifies as DIAL) as a parameter error on `save_path`, directing the agent to give a home-relative path. Any invalid `save_path` is a parameter error on `save_path`. +- **Collision:** write with `overwrite=False` so an existing workspace file is never clobbered; on an etag clash, uniquify the target with a numeric suffix (`data.py` → `data-1.py`, …), bounded by a retry cap, before failing with a clear error. The uniquified name is what the result reports back. (A future revision could let a caller opt into overwrite; phase-1 always uniquifies.) +- **Binary write:** `DialFileService.write_bytes` writes arbitrary bytes to the *resolved home URL* (never the flat bucket path). Covered by the binary-save test. +- **No user-choice propagation:** the tool returns the path (+ preview) in `ToolCallResult.content` and deliberately does **not** set `result.attachments`. This **diverges from `_WriteFileTool`**, which *does* return `attachments=[attachment]`; the `StagedBaseTool` choice-propagation path (`staged_base_tool.py`) then forwards those whose type matches the tool config's `propagate_types_to_choice`. By leaving `attachments` empty, the save keeps the fetched file out of the user-visible choice — consistent with deferring `propagate_to_choice` (Out of Scope). +- **Verification:** a test that saves then `internal_file_list`/`read_lines` the returned relative path. ### Component 4a: Interaction with the global offload processor -- **Context:** when the dial-files offload sub-feature is configured, `ToolCallResultOffloadProcessor` is registered (`DialFilesToolingModule._provide_offload_processors`) and applied to **every** tool result in `ToolExecutor.__process_result` (`core/agent/tool_executor.py:56`) — it is not something a tool opts into. When a result's `content` exceeds the threshold (default 40 KB, `config/dial_files.py`), it is offloaded to a DIAL file and replaced with a notice. If the offload feature is not configured, no such processor exists. -- **Reconciliation:** neither tool **depends on** offload — `internal_file_download` is the explicit large-content path. Out of the box the `web_fetch` size guard and the offload threshold share a default, so there is no overlap. Both thresholds are independently configurable, so an operator can decouple them in two ways — each a documented, opt-in trade-off, never a silent contradiction: - - **Raising `max_inline_size` above the offload threshold:** `web_fetch` results between the two values would be offloaded by the global processor (turned into a file + notice) rather than returned inline. - - **Lowering the offload threshold below `max_inline_size`:** same effect from the other direction — `web_fetch` results between the two would be offloaded. - - **`internal_file_download`** is unaffected either way: it returns a small path + preview, well under any threshold. -- **Decision:** do **not** special-case either tool against offload in phase-1; the shared default removes the overlap, and the schema docs both knobs so an operator who decouples them does so knowingly. (If an operator wants `web_fetch` exempted regardless, the offload config already supports per-tool `excluded_tools`.) +- **Context:** when the dial-files offload sub-feature is configured, `ToolCallResultOffloadProcessor` is registered (`DialFilesToolingModule._provide_offload_processors`) and applied to **every** tool result in `ToolExecutor.__process_result` (`core/agent/tool_executor.py`) — it is not something a tool opts into. When a result's `content` exceeds the threshold (default 40 KB, `config/dial_files.py`), it is offloaded to a DIAL file and replaced with a notice. If the offload feature is not configured, no such processor exists. +- **Reconciliation:** the tool does not **depend on** offload — saving is the explicit large-content path. Out of the box the inline size guard and the offload threshold share a default, so there is no overlap. Both thresholds are independently configurable, so an operator can decouple them — a documented, opt-in trade-off, never a silent contradiction: + - **Raising `max_inline_size` above the offload threshold**, or **lowering the offload threshold below `max_inline_size`**: inline results between the two values would be offloaded by the global processor (turned into a file + notice) rather than returned inline. + - A save returns a small path + preview, well under any threshold. +- **Decision:** do **not** special-case the tool against offload in phase-1; the shared default removes the overlap, and the schema docs both knobs so an operator who decouples them does so knowingly. (If an operator wants `web_fetch` exempted regardless, the offload config already supports per-tool `excluded_tools`.) ### Component 5: Tool config, names, DI wiring, and gating -Both tools are **feature-gated** (enabled through `features.*`, not through a per-tool tool-set entry) and **preview-gated** — symmetric by design: +The tool is **feature-gated** (enabled through `features.web_fetch`, not through a per-tool tool-set entry) and **preview-gated**: -- **Names:** add `INTERNAL_WEB_FETCH_TOOL_NAME = "internal_web_fetch"` to `common/tool_names.py`; `internal_file_download` follows the family's `INTERNAL_FILE_TOOL_NAME_PREFIX` + short-name `download`. -- **`internal_web_fetch`:** - - New feature config `WebFetchConfig` (`enabled: bool = false`, `max_inline_size: int` defaulting to the offload threshold) added to the `Features` model (`config/application.py:169`) as a `PreviewField` — so configuring it requires `ENABLE_PREVIEW_FEATURES`, matching `dial_files`. - - Provided by a **new `WebToolingModule`** (`web_tooling/web_tooling_module.py`, `@preview_module`) via its own `@multiprovider`, which builds the tool when `features.web_fetch.enabled` is true (reading `max_inline_size` from the same config). Registered in `app_factory.py`. -- **`internal_file_download` (file family):** - - Enabled via `features.dial_files.enabled_tools` — the short-name `download` (or `"all"`), like every other `internal_file_*` tool. The `dial_files_tooling` module strips the `internal_file_` prefix and checks `short_name in cfg.enabled_tools` (`dial_files_tooling_module.py:123`). - - Add `download` to the `DialFilesToolName` literal (defined near the top of `config/dial_files.py`). Dispatched from the existing `dial_files_tooling` `@multiprovider`; bound at request scope. -- **Preview gating:** `DialFilesToolingModule` is already `@preview_module` (`dial_files_tooling_module.py:46`), so `internal_file_download` is gated by `ENABLE_PREVIEW_FEATURES`. `WebToolingModule` is itself `@preview_module`, and `WebFetchConfig` is a `PreviewField` — so `internal_web_fetch` is preview-gated at both the module and config level, and **both tools graduate together**. +- **Name:** `INTERNAL_WEB_FETCH_TOOL_NAME = "internal_web_fetch"` in `common/tool_names.py`. +- **Feature config `WebFetchConfig`** (`config/web_fetch.py`): `enabled: bool = false`, `max_inline_size: int` (validated `gt=0`) defaulting (via `default_factory`) to the offload threshold. Exposed on the `Features` model (`config/application.py`) as a `PreviewField` — so configuring it requires `ENABLE_PREVIEW_FEATURES`. +- **Provider:** `WebToolingModule` (`web_tooling/web_tooling_module.py`, `@preview_module`) via a `@multiprovider` that builds the tool when `features.web_fetch.enabled` is true (passing `max_inline_size` from the same config). Registered in `app_factory.py`. +- **Shared home resolver:** move `_HomePathResolver` (`dial_files_tooling/_home_path_resolver.py`) into a **new `shared/home_path/` package** as public `HomePathResolver`, bound request-scoped by a `HomePathModule` spliced into `shared_module` (`shared/__init__.py`, mirroring the existing `ExternalFetchModule` there, per the CLAUDE.md `shared/` convention). `dial_files_tooling` drops its own binding (`dial_files_tooling_module.py`) and injects the shared type; `_DialFileTool` and the `_AppdataHomePathTransformer` keep working against the same public API. Its constructor deps (`DialFileService`, `DialFilesConfig`) are already globally bound. +- **Preview gating:** `WebToolingModule` is `@preview_module` **and** `WebFetchConfig` is exposed via `PreviewField` — so `internal_web_fetch` is preview-gated at both the module and config level. (This mirrors how the dial-files tooling is preview-gated at the *module* level via `@preview_module`; note the `Features.dial_files` config field itself is a plain `Field`, so the two are not identical — `web_fetch` additionally nullifies its config outside preview.) - **Schema:** run `make dump_app_schema` to regenerate `docs/generated-app-schema.json` and `docs/generated-internal-tools.json`. ### Component 6: Egress policy (reused, unchanged) -- No new policy code. The two-tier gate (`EXTERNAL_URL_FETCH_ENABLED` + `features.external_url_fetch.enabled`), host allowlists, and SSRF guard are enforced inside `ExternalUrlFetcher.fetch`. Each tool's only responsibility is to surface the resulting errors clearly (UC-6). +- No new policy code. The two-tier gate (`EXTERNAL_URL_FETCH_ENABLED` + `features.external_url_fetch.enabled`), host allowlists, and SSRF guard are enforced inside `ExternalUrlFetcher.fetch`. The tool's only responsibility is to surface the resulting errors clearly (UC-5). + +--- + +## Secondary Fixes + +None. The rework is self-contained; it introduces no follow-on changes to unrelated components beyond the shared-resolver relocation (Component 5), which is part of the main design. --- @@ -206,50 +213,46 @@ Both tools are **feature-gated** (enabled through `features.*`, not through a pe Deferred from phase-1; each is a clean follow-on, not a rework: -- **PDF / binary text extraction.** Binary content is download-only (no inline, no extraction) in phase-1. Extraction needs a parser and a preview strategy; future phase. -- **Special-casing the tools against the global offload processor.** Neither tool relies on offload (`internal_file_download` covers large content), but they also do not exempt themselves from the global post-processor in phase-1; see Component 4a. -- **Load-mode pagination / `start_index` on `internal_web_fetch`.** Unnecessary — `internal_file_download` + `internal_file_read_lines` / `internal_file_search` provide ranged access; web fetch is for content that fits inline. +- **A second dedicated tool / splitting read and save apart.** The prior revision's `internal_file_download` (a `_DialFileTool` subclass in the file family) is intentionally folded into the `save_path` branch. If a future need arises to enable saving independently of read-into-context, a separate flag or tool can be reintroduced; phase-1 treats web retrieval as one capability. +- **Auto-derived save filenames.** Saving always uses the caller's `save_path`; the tool never guesses a name from Content-Disposition / URL path / MIME type. A future revision could add an "infer a name" affordance. +- **PDF / binary text extraction.** Binary content is save-only (no inline, no extraction) in phase-1. Extraction needs a parser and a preview strategy; future phase. +- **Special-casing the tool against the global offload processor.** The tool does not rely on offload (saving covers large content) and does not exempt itself from the global post-processor in phase-1; see Component 4a. +- **Pagination / `start_index` on inline reads.** Unnecessary — a save + `internal_file_read_lines` / `internal_file_search` provide ranged access; inline fetch is for content that fits inline. - **Content summarization** (Claude Code-style prompt-over-content). -- **Surfacing the downloaded file to the user via `propagate_to_choice`** and richer binary metadata (thumbnails, structured metadata). -- **Provenance-based URL allowlisting** (Anthropic-style: only fetch URLs that appeared in conversation context, to harden against model-fabricated exfiltration URLs). A worthwhile future hardening; the existing egress policy + host allowlist already gate destinations today. -- **DIAL-URL retrieval.** A DIAL URL is rejected with guidance (UC-7) rather than fetched; in-workspace reads stay with the file tools. +- **Surfacing the saved file to the user via `propagate_to_choice`** and richer binary metadata (thumbnails, structured metadata). +- **Provenance-based URL allowlisting** (Anthropic-style: only fetch URLs that appeared in conversation context). A worthwhile future hardening; the existing egress policy + host allowlist already gate destinations today. +- **DIAL-URL retrieval.** A DIAL URL is rejected with guidance (UC-6) rather than fetched; in-workspace reads stay with the file tools. --- ## Configuration / Usage Examples -### Enabling both tools +### Enabling the tool -Both tools are turned on through `features.*` (no per-tool tool-set entry for either) and both require `ENABLE_PREVIEW_FEATURES=true` (preview-gated in phase-1): +The tool is turned on through `features.web_fetch` (no per-tool tool-set entry) and requires `ENABLE_PREVIEW_FEATURES=true` (preview-gated in phase-1): ```yaml features: - # Enables internal_web_fetch and sets its inline size cap (defaults to the - # offload threshold, 40 KB). + # Enables internal_web_fetch and sets its inline size cap + # (defaults to the offload threshold, 40 KB). web_fetch: enabled: true max_inline_size: 40000 - - # Enables internal_file_download via the file-family short-name list. - dial_files: - enabled_tools: [download, read_lines, search] # or "all" ``` -> Neither tool has an `internal-tool` YAML entry. `internal_web_fetch` is selected by `features.web_fetch.enabled`; `internal_file_download` by its short-name in `features.dial_files.enabled_tools`. +> `internal_web_fetch` has no `internal-tool` YAML entry — it is selected solely by `features.web_fetch.enabled`. ### Walkthrough — load into context (UC-1) `internal_web_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` -→ returns the README text inline. No file written. +→ returns the README text inline (no `save_path`). No file written. -### Walkthrough — download then search (UC-2 → UC-3) +### Walkthrough — save to the workspace (UC-2) -1. `internal_file_download(url="https://…/data.py")` - → `saved: data.py` (workspace-relative, under the agent home) + short preview. -2. `internal_file_search(path="data.py", query="def main")` - → operates on the persisted file with no re-fetch. +`internal_web_fetch(url="https://…/data.py", save_path="analysis/data.py")` +→ `saved: analysis/data.py` (workspace-relative, under the agent home) + short preview. The returned path can then be handed to the model / a deployment as an attachment link, or read/searched with `internal_file_read_lines` / `internal_file_search`. -### Egress disabled (UC-6) +### Egress disabled (UC-5) `internal_web_fetch(url="https://example.com/x")` with `EXTERNAL_URL_FETCH_ENABLED=false` → tool error: *"External URL fetching is disabled by operator policy (EXTERNAL_URL_FETCH_ENABLED)."* @@ -260,47 +263,61 @@ features: ### Breaking changes -None. Both tools are purely additive and opt-in via app config. +None. The tool is purely additive and opt-in via app config. ### Non-breaking changes -- New tools `internal_web_fetch` and `internal_file_download` appear in the generated schema/manifest after `make dump_app_schema`. -- No change to existing tools, the egress policy, the offload feature, or the config shape beyond the new tool entries. +- Only additive: a new `features.web_fetch` entry plus the new `internal_web_fetch` tool in the generated schema after `make dump_app_schema`. No existing tool, policy, or config shape changes. ## Summary of Changes -### New files +> **Baseline.** This inventory is the net change **relative to `development`** (the eventual merge target). Several of these artifacts already exist on the current `feat/344-web-fetch-tool` branch from the superseded two-tool implementation (uncommitted); [Branch reconciliation](#branch-reconciliation) covers how that intermediate state is brought to this end state. +### New files (vs. `development`) + +- `shared/home_path/home_path_resolver.py` + `home_path_module.py` — public `HomePathResolver` (moved from `dial_files_tooling/_home_path_resolver.py`) and `HomePathModule`, spliced into `shared_module`. - `web_tooling/web_tooling_module.py` — `WebToolingModule` (`@preview_module`; `configure` + `@multiprovider`). -- `web_tooling/_web_fetch_tool.py` — the `internal_web_fetch` tool (plain `StagedBaseTool`; depends on the shared fetch helper). -- `web_tooling/_web_fetch_tool_config.py` — the tool's `InternalTool` config (`WEB_FETCH_TOOL_CONFIG`). -- `dial_files_tooling/_download_file_tool.py` — the `internal_file_download` tool (`_DialFileTool` subclass; persists via the file-family `DialFileService` write path). -- A shared fetch helper (Component 3) — location shared by both (e.g., `shared/external_fetch/` or a small `common/` helper), reused by both tools. -- Unit tests for both tools. +- `web_tooling/_web_fetch_tool.py` — the `internal_web_fetch` tool (plain `StagedBaseTool`; injects the shared fetch helper + shared home resolver; branches on `save_path`). +- `web_tooling/_tool_configs.py` — the tool's `InternalTool` config (`WEB_FETCH_TOOL_CONFIG`), with `url` and optional `save_path` parameters. +- `web_tooling/_web_fetch_stage_wrapper.py` — the tool's stage wrapper. +- `config/web_fetch.py` — `WebFetchConfig` (`enabled`, `max_inline_size` with `gt=0`). +- `shared/external_fetch/web_content_fetcher.py` — the shared fetch helper (Component 3), exposing `fetch_external` / `is_textual` / `decode`. +- Unit tests for the tool (both branches) and the fetch helper. -### Modified files +### Modified files (vs. `development`) -- `common/tool_names.py` — add `INTERNAL_WEB_FETCH_TOOL_NAME` (the download tool uses the family prefix + short-name `download`). -- `config/application.py` — add `WebFetchConfig` and a `web_fetch` `PreviewField` to the `Features` model (`enabled: bool`, `max_inline_size: int` defaulting to the offload threshold). +- `common/tool_names.py` — add `INTERNAL_WEB_FETCH_TOOL_NAME`. +- `config/application.py` — add a `web_fetch` `PreviewField` to the `Features` model. - `app_factory.py` — register `WebToolingModule`. -- `config/dial_files.py` — add `download` to the `DialFilesToolName` literal so it can appear in `enabled_tools`. -- `dial_files_tooling` DI module — bind `internal_file_download` and dispatch it from the existing `@multiprovider`; add its tool definition to the family. -- `dial_core_services/dial_file_service.py` — if no bytes-capable write exists, add a thin one targeting a resolved home URL (Component 4 binary nuance). -- `docs/generated-app-schema.json`, `docs/generated-internal-tools.json` — regenerated. +- `dial_core_services/dial_file_service.py` — add `write_bytes` (bytes-capable sibling of `write_file`) targeting a resolved home URL (Component 4). +- `dial_files_tooling/dial_files_tooling_module.py` — drop the local `_HomePathResolver` binding (now provided by `HomePathModule`) and inject the shared `HomePathResolver`. +- `shared/__init__.py` — add `HomePathModule` to `shared_module`. + +### Branch reconciliation + +The current branch already carries a superseded two-tool implementation. Bringing it to the design above means: + +- **Rework `web_tooling/_web_fetch_tool.py` + `_tool_configs.py`:** replace the intermediate `mode`/`path` argument pair (and any `internal_file_download` references in the oversize/binary error messages and the `WEB_FETCH_TOOL_CONFIG` description) with the single `save_path` argument, the `files/`-prefix rejection, and "re-call with a save_path" guidance; fold the save/write-unique logic (from the deleted download tool) into the `save_path` branch. +- **Rewrite `config/web_fetch.py`'s `max_inline_size` description** to reference the `save_path` guidance instead of `internal_file_download`. +- **Delete** `dial_files_tooling/_download_file_tool.py` and its tests. +- **Revert** the `download` short-name in the `DialFilesToolName` literal (`config/dial_files.py`) and its dispatch in the `dial_files_tooling` `@multiprovider` / config. +- **Move** `_HomePathResolver` → `shared/home_path/HomePathResolver` and rewire bindings (Component 5). -### Tools exposed to the LLM +### Tool exposed to the LLM -- `internal_web_fetch(url)` — fetch a text resource and return it inline (text only; binary/oversize → directs to download). -- `internal_file_download(url)` — download any resource into the workspace (under the agent home) and return its relative path (+ preview when textual). +- `internal_web_fetch(url, save_path=None)` — fetch an external resource; without `save_path` it returns text inline (text only; binary/oversize → directs the agent to pass a `save_path`); with `save_path` it persists the resource there under the agent home and returns the relative path (+ preview when textual). ### Tests -- **web_fetch:** textual within guard → full inline content, no file written. -- **web_fetch:** textual over size guard → parameter error pointing at `internal_file_download`. -- **web_fetch:** binary → parameter error pointing at `internal_file_download`. -- **download:** textual → persisted under agent home, relative path + preview returned. -- **download:** binary → persisted under agent home, relative path + content type + size returned, no inline body. -- **download** then `internal_file_list`/`read_lines` on the returned relative path (workspace-placement guarantee). -- **both:** egress disabled / host not allowed → parameter error with the policy message. -- **both:** DIAL URL → parameter error pointing to the file tools. -- **config:** enabling only one tool exposes only that tool (UC-8). +- **inline:** textual within guard → full inline content, no file written. +- **inline:** textual over size guard → parameter error telling the agent to pass a `save_path`. +- **inline:** binary → parameter error telling the agent to pass a `save_path`. +- **save:** textual → persisted at `save_path` under agent home, relative path + preview returned. +- **save:** binary → persisted at `save_path`, relative path + content type + size returned, no inline body. +- **save:** `save_path` with a subdirectory → persisted at that workspace-relative path. +- **save:** invalid `save_path` (absolute / traversal / empty / `files/`-prefixed) → parameter error on `save_path`. +- **save:** `save_path` collision → numeric-suffix uniquification. +- **save** then `internal_file_list`/`read_lines` on the returned relative path (workspace-placement guarantee). +- **both branches:** egress disabled / host not allowed → parameter error with the policy message. +- **both branches:** DIAL URL → parameter error pointing to the file tools. +- **config:** `features.web_fetch.enabled=false` (or omitted) → tool not exposed (UC-7). From 270b548036b7d46df5fd5214dc45f8543b586e0c Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Fri, 3 Jul 2026 17:15:46 +0300 Subject: [PATCH 05/12] feat: built-in internal_web_fetch tool with save_path (#344) Implement the approved one-tool design: internal_web_fetch(url, save_path). Omit save_path to return the fetched text inline (textual, size-guarded); provide it to persist the resource under the agent home via the shared HomePathResolver + DialFileService.write_bytes, returning the workspace path. - Promote HomePathResolver to shared/home_path (HomePathModule in shared_module), injected by both the file tools and the web tool. - Remove the superseded internal_file_download tool and its dial_files wiring. - Gate the tool on external egress at init: when web_fetch is enabled but egress is disabled, the tool is not exposed and a hard ToolInitializationException is surfaced, instead of failing on every call. - Reject files/-prefixed save_path; uniquify on collision (overwrite=False). - Regenerate schemas; dump_internal_tools enables egress so the tool materializes. --- docs/designs/web_fetch_tool.md | 17 +- docs/generated-app-schema.json | 32 +++ docs/generated-internal-tools.json | 17 ++ src/quickapp/app_factory.py | 2 + src/quickapp/common/tool_names.py | 1 + src/quickapp/config/application.py | 9 + src/quickapp/config/web_fetch.py | 32 +++ .../dial_core_services/dial_file_service.py | 43 +++- .../dial_files_tooling/_base_file_tool.py | 4 +- .../_path_argument_transformer.py | 4 +- .../dial_files_tooling_module.py | 5 +- src/quickapp/shared/__init__.py | 2 + .../external_fetch/external_fetch_module.py | 2 + .../external_fetch/web_content_fetcher.py | 138 +++++++++++++ src/quickapp/shared/home_path/__init__.py | 0 .../shared/home_path/home_path_module.py | 16 ++ .../home_path/home_path_resolver.py} | 6 +- src/quickapp/web_tooling/__init__.py | 0 src/quickapp/web_tooling/_tool_configs.py | 50 +++++ .../web_tooling/_web_fetch_stage_wrapper.py | 19 ++ src/quickapp/web_tooling/_web_fetch_tool.py | 186 +++++++++++++++++ .../web_tooling/web_tooling_module.py | 88 ++++++++ src/scripts/dump_internal_tools.py | 10 +- .../unit_tests/dial_files_tooling/_helpers.py | 4 +- .../dial_files_tooling/test_base_file_tool.py | 5 +- .../test_path_argument_transformer.py | 4 +- .../test_home_path_resolver.py | 6 +- .../shared/test_web_content_fetcher.py | 124 ++++++++++++ src/tests/unit_tests/web_tooling/__init__.py | 0 .../web_tooling/test_web_fetch_tool.py | 188 ++++++++++++++++++ .../web_tooling/test_web_tooling_module.py | 94 +++++++++ 31 files changed, 1078 insertions(+), 30 deletions(-) create mode 100644 src/quickapp/config/web_fetch.py create mode 100644 src/quickapp/shared/external_fetch/web_content_fetcher.py create mode 100644 src/quickapp/shared/home_path/__init__.py create mode 100644 src/quickapp/shared/home_path/home_path_module.py rename src/quickapp/{dial_files_tooling/_home_path_resolver.py => shared/home_path/home_path_resolver.py} (93%) create mode 100644 src/quickapp/web_tooling/__init__.py create mode 100644 src/quickapp/web_tooling/_tool_configs.py create mode 100644 src/quickapp/web_tooling/_web_fetch_stage_wrapper.py create mode 100644 src/quickapp/web_tooling/_web_fetch_tool.py create mode 100644 src/quickapp/web_tooling/web_tooling_module.py rename src/tests/unit_tests/{dial_files_tooling => shared}/test_home_path_resolver.py (96%) create mode 100644 src/tests/unit_tests/shared/test_web_content_fetcher.py create mode 100644 src/tests/unit_tests/web_tooling/__init__.py create mode 100644 src/tests/unit_tests/web_tooling/test_web_fetch_tool.py create mode 100644 src/tests/unit_tests/web_tooling/test_web_tooling_module.py diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md index cca23290..3a0aaa6d 100644 --- a/docs/designs/web_fetch_tool.md +++ b/docs/designs/web_fetch_tool.md @@ -92,11 +92,11 @@ Both branches share the same front half — classify, egress-gate, fetch, decode ### UC-5: External egress is disabled (admin cap or per-app opt-out) -**Trigger:** Agent calls `internal_web_fetch` on an external URL (with or without `save_path`) while `EXTERNAL_URL_FETCH_ENABLED=false` (or the app set `features.external_url_fetch.enabled=false`, or the host is outside the allowlist). +**Trigger:** An app sets `features.web_fetch.enabled=true` while external fetching is disabled — `EXTERNAL_URL_FETCH_ENABLED=false` (admin cap) or `features.external_url_fetch.enabled=false` (per-app opt-out). -**Behavior:** `ExternalUrlFetcher.fetch` raises `ExternalFetchDisabledError`; the tool wraps it as a parameter-scoped tool error carrying the existing operator/builder/allowlist message. +**Behavior:** This is a contradictory configuration — the tool cannot function without egress — so it is caught at **initialization**, not per call. `WebToolingModule` gates tool provisioning on the egress policy (`ExternalUrlFetchPolicyResolver.is_enabled()`): the tool is **not exposed**, and a hard `ToolInitializationException` is surfaced in the "Initialization issues" stage explaining that `web_fetch` needs external fetch enabled. (A host-allowlist denial — egress on, but the *specific host* not allowed — is per-URL and cannot be known at init; it remains a runtime parameter error via `ExternalUrlFetcher.fetch`, see Component 3.) -**Outcome:** The model gets a clear, actionable refusal — no new policy, no bypass. +**Outcome:** The builder learns their config is contradictory up front; the model is never offered a tool that would fail on every call. No silent misconfiguration. ### UC-6: Agent passes a DIAL URL @@ -193,13 +193,14 @@ The tool is **feature-gated** (enabled through `features.web_fetch`, not through - **Name:** `INTERNAL_WEB_FETCH_TOOL_NAME = "internal_web_fetch"` in `common/tool_names.py`. - **Feature config `WebFetchConfig`** (`config/web_fetch.py`): `enabled: bool = false`, `max_inline_size: int` (validated `gt=0`) defaulting (via `default_factory`) to the offload threshold. Exposed on the `Features` model (`config/application.py`) as a `PreviewField` — so configuring it requires `ENABLE_PREVIEW_FEATURES`. - **Provider:** `WebToolingModule` (`web_tooling/web_tooling_module.py`, `@preview_module`) via a `@multiprovider` that builds the tool when `features.web_fetch.enabled` is true (passing `max_inline_size` from the same config). Registered in `app_factory.py`. +- **Egress init-gate (fail fast on contradictory config):** the tool is useless without external egress, so `WebToolingModule` injects `ExternalUrlFetchPolicyResolver` and, when `web_fetch.enabled` is set but `is_enabled()` is false (admin cap or per-app opt-out), **does not build the tool** and emits a hard `ToolInitializationException` via a second `@multiprovider -> list[InitializationException]` (aggregated by `_InitializationErrorHandler` into the "Initialization issues" stage). This replaces per-call egress-disabled errors for the on/off case (UC-5); host-allowlist denials stay runtime (Component 3). - **Shared home resolver:** move `_HomePathResolver` (`dial_files_tooling/_home_path_resolver.py`) into a **new `shared/home_path/` package** as public `HomePathResolver`, bound request-scoped by a `HomePathModule` spliced into `shared_module` (`shared/__init__.py`, mirroring the existing `ExternalFetchModule` there, per the CLAUDE.md `shared/` convention). `dial_files_tooling` drops its own binding (`dial_files_tooling_module.py`) and injects the shared type; `_DialFileTool` and the `_AppdataHomePathTransformer` keep working against the same public API. Its constructor deps (`DialFileService`, `DialFilesConfig`) are already globally bound. - **Preview gating:** `WebToolingModule` is `@preview_module` **and** `WebFetchConfig` is exposed via `PreviewField` — so `internal_web_fetch` is preview-gated at both the module and config level. (This mirrors how the dial-files tooling is preview-gated at the *module* level via `@preview_module`; note the `Features.dial_files` config field itself is a plain `Field`, so the two are not identical — `web_fetch` additionally nullifies its config outside preview.) - **Schema:** run `make dump_app_schema` to regenerate `docs/generated-app-schema.json` and `docs/generated-internal-tools.json`. ### Component 6: Egress policy (reused, unchanged) -- No new policy code. The two-tier gate (`EXTERNAL_URL_FETCH_ENABLED` + `features.external_url_fetch.enabled`), host allowlists, and SSRF guard are enforced inside `ExternalUrlFetcher.fetch`. The tool's only responsibility is to surface the resulting errors clearly (UC-5). +- No new policy code. The two-tier gate (`EXTERNAL_URL_FETCH_ENABLED` + `features.external_url_fetch.enabled`), host allowlists, and SSRF guard are enforced inside `ExternalUrlFetcher.fetch`. The tool reuses the same `ExternalUrlFetchPolicyResolver` to gate its own provisioning at init (Component 5, UC-5); at runtime it surfaces any residual errors (host-allowlist denials, SSRF) clearly. --- @@ -254,8 +255,8 @@ features: ### Egress disabled (UC-5) -`internal_web_fetch(url="https://example.com/x")` with `EXTERNAL_URL_FETCH_ENABLED=false` -→ tool error: *"External URL fetching is disabled by operator policy (EXTERNAL_URL_FETCH_ENABLED)."* +`features.web_fetch.enabled=true` with `EXTERNAL_URL_FETCH_ENABLED=false` (or `features.external_url_fetch.enabled=false`) +→ the tool is **not exposed**; a hard initialization error appears in the "Initialization issues" stage: *"internal_web_fetch requires external URL fetching, which is disabled … enable it, or remove features.web_fetch."* --- @@ -289,6 +290,7 @@ None. The tool is purely additive and opt-in via app config. - `common/tool_names.py` — add `INTERNAL_WEB_FETCH_TOOL_NAME`. - `config/application.py` — add a `web_fetch` `PreviewField` to the `Features` model. - `app_factory.py` — register `WebToolingModule`. +- `web_tooling/web_tooling_module.py` — inject `ExternalUrlFetchPolicyResolver`; gate tool provisioning on `is_enabled()` and emit a hard `ToolInitializationException` (via a `list[InitializationException]` `@multiprovider`) when `web_fetch` is enabled but egress is disabled. - `dial_core_services/dial_file_service.py` — add `write_bytes` (bytes-capable sibling of `write_file`) targeting a resolved home URL (Component 4). - `dial_files_tooling/dial_files_tooling_module.py` — drop the local `_HomePathResolver` binding (now provided by `HomePathModule`) and inject the shared `HomePathResolver`. - `shared/__init__.py` — add `HomePathModule` to `shared_module`. @@ -318,6 +320,7 @@ The current branch already carries a superseded two-tool implementation. Bringin - **save:** invalid `save_path` (absolute / traversal / empty / `files/`-prefixed) → parameter error on `save_path`. - **save:** `save_path` collision → numeric-suffix uniquification. - **save** then `internal_file_list`/`read_lines` on the returned relative path (workspace-placement guarantee). -- **both branches:** egress disabled / host not allowed → parameter error with the policy message. +- **both branches:** host not allowed / SSRF → parameter error with the policy message (runtime). - **both branches:** DIAL URL → parameter error pointing to the file tools. - **config:** `features.web_fetch.enabled=false` (or omitted) → tool not exposed (UC-7). +- **egress init-gate:** `web_fetch.enabled=true` but egress disabled → tool not exposed **and** a hard `ToolInitializationException` emitted (UC-5); `enabled=false` + egress disabled → no error. diff --git a/docs/generated-app-schema.json b/docs/generated-app-schema.json index 858dca1c..0badbe99 100644 --- a/docs/generated-app-schema.json +++ b/docs/generated-app-schema.json @@ -1344,6 +1344,19 @@ ], "default": null, "description": "Built-in DIAL files tools (list / read / search / write / edit / delete / copy / move)." + }, + "web_fetch": { + "anyOf": [ + { + "$ref": "#/$defs/WebFetchConfig" + }, + { + "type": "null" + } + ], + "default": null, + "description": "Built-in internal_web_fetch tool: fetch an external resource and return it inline, or persist it to the workspace when a save_path is given (binary / oversized content must be saved rather than read inline).", + "x-preview": true } }, "title": "Features", @@ -3245,6 +3258,25 @@ ], "title": "UserDefinedContextConfig", "type": "object" + }, + "WebFetchConfig": { + "description": "Per-app config for the built-in ``internal_web_fetch`` tool.\n\nToggled by the explicit ``enabled`` flag. ``max_inline_size`` caps how much\ndecoded text the tool returns inline; its default is drawn from the same env\nsetting that governs the offload threshold's default\n(``TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD``), so out of the box the inline\ncap and the global offload threshold are equal — anything returned inline is\nbelow the offloader's trigger. See docs/designs/web_fetch_tool.md (Component\n4a) for the cases where an operator intentionally decouples the two.", + "properties": { + "enabled": { + "default": false, + "description": "Whether to expose the internal_web_fetch tool to the LLM.", + "title": "Enabled", + "type": "boolean" + }, + "max_inline_size": { + "description": "Byte cap on the decoded text internal_web_fetch returns inline. Larger (or binary) resources are rejected with guidance to re-call with a save_path. Defaults to the TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD env var so the inline cap matches the global offload threshold by default.", + "exclusiveMinimum": 0, + "title": "Max Inline Size", + "type": "integer" + } + }, + "title": "WebFetchConfig", + "type": "object" } }, "properties": { diff --git a/docs/generated-internal-tools.json b/docs/generated-internal-tools.json index 7bfae16d..f2eb6d14 100644 --- a/docs/generated-internal-tools.json +++ b/docs/generated-internal-tools.json @@ -351,5 +351,22 @@ "description": "IANA timezone name (e.g. 'Asia/Tokyo'). Defaults to UTC." } } + }, + { + "name": "internal_web_fetch", + "description": "Fetch a resource from an external http(s) URL (e.g. a README, a source file, a documentation page). Without save_path it returns the text inline in a single call — text only: binary content (images, PDFs, archives) or a resource too large to fit inline is rejected, re-call with a save_path instead. With save_path it persists the resource (any content type) at that workspace-relative path under the agent home and returns the saved path (+ a short preview for text), so you can then read it with internal_file_read_lines / internal_file_search. DIAL file paths (files/...) are not fetched here; read them with the file tools.", + "properties": { + "url": { + "type": "string", + "description": "The external http(s) URL to fetch." + }, + "save_path": { + "type": "string", + "description": "Optional workspace-relative destination (e.g. 'data.py' or 'docs/readme.md'). Omit to read the content inline; provide it to save the resource under the agent home and get back the saved path. Must not be an absolute 'files/...' URL." + } + }, + "required": [ + "url" + ] } ] diff --git a/src/quickapp/app_factory.py b/src/quickapp/app_factory.py index a7afbd63..647505b6 100644 --- a/src/quickapp/app_factory.py +++ b/src/quickapp/app_factory.py @@ -28,6 +28,7 @@ from quickapp.skills.skills_module import SkillsModule from quickapp.starters.starters_module import StartersModule from quickapp.timestamp_tooling.timestamp_module import TimestampModule +from quickapp.web_tooling.web_tooling_module import WebToolingModule class AppFactory: @@ -57,6 +58,7 @@ def build_di_modules() -> list[Module]: TimestampModule(), AgentHooksModule(), DialFilesToolingModule(), + WebToolingModule(), ] if FeatureSettings().enable_preview_features: logging.getLogger(__name__).info( diff --git a/src/quickapp/common/tool_names.py b/src/quickapp/common/tool_names.py index 3fdc72fe..e7313077 100644 --- a/src/quickapp/common/tool_names.py +++ b/src/quickapp/common/tool_names.py @@ -9,6 +9,7 @@ INTERNAL_SKILLS_READ_SKILL_TOOL_NAME = "internal_skills_read_skill" INTERNAL_TIMEAWARENESS_CURRENT_TIMESTAMP_TOOL_NAME = "internal_timeawareness_current_timestamp" INTERNAL_CODE_EXECUTION_PYTHON_INTERPRETER_TOOL_NAME = "internal_code_execution_python_interpreter" +INTERNAL_WEB_FETCH_TOOL_NAME = "internal_web_fetch" # DIAL files tools — all share the ``internal_file_`` prefix. INTERNAL_FILE_TOOL_NAME_PREFIX = "internal_file_" diff --git a/src/quickapp/config/application.py b/src/quickapp/config/application.py index f63df0c5..c13af611 100644 --- a/src/quickapp/config/application.py +++ b/src/quickapp/config/application.py @@ -23,6 +23,7 @@ from quickapp.config.starters import ConversationStartersConfig from quickapp.config.timestamp import TimestampConfig, ToolCallTimestampConfig from quickapp.config.toolsets.toolset import ToolSet +from quickapp.config.web_fetch import WebFetchConfig logger = logging.getLogger(__name__) @@ -190,6 +191,14 @@ class Features(BaseModel): default=None, description="Built-in DIAL files tools (list / read / search / write / edit / delete / copy / move).", ) + web_fetch: WebFetchConfig | None = PreviewField( # type: ignore[assignment] + default=None, + description=( + "Built-in internal_web_fetch tool: fetch an external resource and return " + "it inline, or persist it to the workspace when a save_path is given " + "(binary / oversized content must be saved rather than read inline)." + ), + ) class ToolDefaults(BaseModel): diff --git a/src/quickapp/config/web_fetch.py b/src/quickapp/config/web_fetch.py new file mode 100644 index 00000000..eefe9d38 --- /dev/null +++ b/src/quickapp/config/web_fetch.py @@ -0,0 +1,32 @@ +from pydantic import BaseModel, Field + +from quickapp.config.dial_files import ToolCallResultOffloadSettings + + +class WebFetchConfig(BaseModel): + """Per-app config for the built-in ``internal_web_fetch`` tool. + + Toggled by the explicit ``enabled`` flag. ``max_inline_size`` caps how much + decoded text the tool returns inline; its default is drawn from the same env + setting that governs the offload threshold's default + (``TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD``), so out of the box the inline + cap and the global offload threshold are equal — anything returned inline is + below the offloader's trigger. See docs/designs/web_fetch_tool.md (Component + 4a) for the cases where an operator intentionally decouples the two. + """ + + enabled: bool = Field( + default=False, + description="Whether to expose the internal_web_fetch tool to the LLM.", + ) + max_inline_size: int = Field( + default_factory=lambda: ToolCallResultOffloadSettings().size_threshold, + gt=0, + description=( + "Byte cap on the decoded text internal_web_fetch returns inline. " + "Larger (or binary) resources are rejected with guidance to re-call " + "with a save_path. Defaults to the " + "TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD env var so the inline cap " + "matches the global offload threshold by default." + ), + ) diff --git a/src/quickapp/dial_core_services/dial_file_service.py b/src/quickapp/dial_core_services/dial_file_service.py index e6e09e91..d03b7012 100644 --- a/src/quickapp/dial_core_services/dial_file_service.py +++ b/src/quickapp/dial_core_services/dial_file_service.py @@ -118,6 +118,29 @@ async def write_file( self.invalidate_cache(url) return uploaded_url + async def write_bytes( + self, + url: str, + content: bytes, + *, + content_type: str = "application/octet-stream", + overwrite: bool = False, + ) -> str: + """Create or overwrite a file from raw bytes (binary-safe). + + The bytes counterpart of :meth:`write_file` — used for content that is + not UTF-8 text (downloaded binaries). Shares the same ETag semantics: + ``overwrite=False`` uploads with ``If-None-Match: *`` (create only, + raising ``EtagMismatchError`` if the file exists); ``overwrite=True`` + uploads unconditionally. Cache is invalidated on success. + """ + if overwrite: + uploaded_url = await self._upload_bytes(url, content, content_type) + else: + uploaded_url = await self._upload_bytes(url, content, content_type, if_none_match="*") + self.invalidate_cache(url) + return uploaded_url + async def _upload_text( self, url: str, @@ -127,11 +150,27 @@ async def _upload_text( if_none_match: Literal["*"] | None = None, if_match: str | None = None, ) -> str: - encoded = content.encode("utf-8") + return await self._upload_bytes( + url, + content.encode("utf-8"), + content_type, + if_none_match=if_none_match, + if_match=if_match, + ) + + async def _upload_bytes( + self, + url: str, + content: bytes, + content_type: str, + *, + if_none_match: Literal["*"] | None = None, + if_match: str | None = None, + ) -> str: filename = url.split("/")[-1] metadata = await self.__dial_client.files.upload( url=url, - file=(filename, encoded, content_type), + file=(filename, content, content_type), etag_if_none_match=if_none_match, etag_if_match=if_match, ) diff --git a/src/quickapp/dial_files_tooling/_base_file_tool.py b/src/quickapp/dial_files_tooling/_base_file_tool.py index 3c60dabb..1ad4eaca 100644 --- a/src/quickapp/dial_files_tooling/_base_file_tool.py +++ b/src/quickapp/dial_files_tooling/_base_file_tool.py @@ -14,9 +14,9 @@ from quickapp.config.dial_files import DialFilesConfig from quickapp.config.tools.internal import InternalTool from quickapp.dial_core_services.dial_file_service import DialFileService, FolderEntry -from quickapp.dial_files_tooling._home_path_resolver import _HomePathResolver from quickapp.dial_files_tooling._stage_wrapper import _FileStageWrapper from quickapp.dial_files_tooling._utils import is_root_reference +from quickapp.shared.home_path.home_path_resolver import HomePathResolver logger = logging.getLogger(__name__) @@ -36,7 +36,7 @@ def __init__( perf_timer: PerformanceTimer, dial_file_service: DialFileService, dial_files_config: DialFilesConfig, - home_resolver: _HomePathResolver, + home_resolver: HomePathResolver, stage_display_level: StageDisplayLevel = StageDisplayLevel.INFO, argument_transformers: list[ToolArgumentTransformer] | None = None, **kwargs: Any, diff --git a/src/quickapp/dial_files_tooling/_path_argument_transformer.py b/src/quickapp/dial_files_tooling/_path_argument_transformer.py index 9cf4a207..875f7f62 100644 --- a/src/quickapp/dial_files_tooling/_path_argument_transformer.py +++ b/src/quickapp/dial_files_tooling/_path_argument_transformer.py @@ -4,7 +4,7 @@ from injector import inject from quickapp.common.abstract.base_tool_argument_transformer import ToolArgumentTransformer -from quickapp.dial_files_tooling._home_path_resolver import _HomePathResolver +from quickapp.shared.home_path.home_path_resolver import HomePathResolver logger = logging.getLogger(__name__) @@ -27,7 +27,7 @@ class _AppdataHomePathTransformer(ToolArgumentTransformer): resolved to a ``files/...`` URL is relativized too. """ - def __init__(self, home_resolver: _HomePathResolver) -> None: + def __init__(self, home_resolver: HomePathResolver) -> None: self._home_resolver = home_resolver async def transform(self, kwargs: dict[str, Any]) -> dict[str, Any]: diff --git a/src/quickapp/dial_files_tooling/dial_files_tooling_module.py b/src/quickapp/dial_files_tooling/dial_files_tooling_module.py index ce943c7c..93b87d72 100644 --- a/src/quickapp/dial_files_tooling/dial_files_tooling_module.py +++ b/src/quickapp/dial_files_tooling/dial_files_tooling_module.py @@ -20,7 +20,6 @@ from quickapp.dial_files_tooling._delete_file_tool import _DeleteFileTool from quickapp.dial_files_tooling._edit_file_tool import _EditFileTool from quickapp.dial_files_tooling._find_files_tool import _FindFilesTool -from quickapp.dial_files_tooling._home_path_resolver import _HomePathResolver from quickapp.dial_files_tooling._list_files_tool import _ListFilesTool from quickapp.dial_files_tooling._move_file_tool import _MoveFileTool from quickapp.dial_files_tooling._offload_config import ResolvedOffloadConfig @@ -49,9 +48,7 @@ class DialFilesToolingModule(Module): def configure(self, binder: Binder) -> None: binder.bind(_FileStageWrapper, to=_FileStageWrapper, scope=request_scope) - # Request-scoped so the agent home dir is resolved once per request and shared - # between the file tools and the path-argument transformer. - binder.bind(_HomePathResolver, to=_HomePathResolver, scope=request_scope) + # HomePathResolver is bound by shared HomePathModule (shared_module). binder.bind( _AppdataHomePathTransformer, to=_AppdataHomePathTransformer, scope=request_scope ) diff --git a/src/quickapp/shared/__init__.py b/src/quickapp/shared/__init__.py index bec89127..cdba8c85 100644 --- a/src/quickapp/shared/__init__.py +++ b/src/quickapp/shared/__init__.py @@ -2,6 +2,7 @@ from quickapp.shared.config_resolvers.config_resolvers_module import ConfigResolversModule from quickapp.shared.external_fetch.external_fetch_module import ExternalFetchModule +from quickapp.shared.home_path.home_path_module import HomePathModule # Cross-cutting utility DI modules. ``app_factory`` splices this array into its module # list, so future utility modules join by appending here rather than being registered @@ -9,4 +10,5 @@ shared_module: list[Module] = [ ConfigResolversModule(), ExternalFetchModule(), + HomePathModule(), ] diff --git a/src/quickapp/shared/external_fetch/external_fetch_module.py b/src/quickapp/shared/external_fetch/external_fetch_module.py index a3e77220..c73b18ce 100644 --- a/src/quickapp/shared/external_fetch/external_fetch_module.py +++ b/src/quickapp/shared/external_fetch/external_fetch_module.py @@ -6,6 +6,7 @@ ExternalUrlFetchPolicyResolver, ) from quickapp.shared.external_fetch.external_url_fetcher import ExternalUrlFetcher +from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher class ExternalFetchModule(Module): @@ -24,3 +25,4 @@ def configure(self, binder: Binder) -> None: scope=request_scope, ) binder.bind(ExternalUrlFetcher, to=ExternalUrlFetcher, scope=request_scope) + binder.bind(WebContentFetcher, to=WebContentFetcher, scope=request_scope) diff --git a/src/quickapp/shared/external_fetch/web_content_fetcher.py b/src/quickapp/shared/external_fetch/web_content_fetcher.py new file mode 100644 index 00000000..fdea6ee9 --- /dev/null +++ b/src/quickapp/shared/external_fetch/web_content_fetcher.py @@ -0,0 +1,138 @@ +"""Shared front-half for the built-in ``internal_web_fetch`` tool. + +Both of the tool's branches (read inline / save to workspace) need the same +steps before they diverge: classify the URL, reject DIAL / unsupported schemes +with actionable errors, fetch the bytes through the single egress point +(:class:`ExternalUrlFetcher`), and decide whether the payload is textual. This +helper owns exactly that shared half; the tool decides what to do with the +returned :class:`FetchedBytes`. +""" + +import logging +from email.message import Message + +from injector import inject + +from quickapp.common.dial_settings import DialSettings +from quickapp.common.exceptions import InvalidToolCallParameterException +from quickapp.common.url_classification import UrlScheme, classify_url, unsupported_scheme_error +from quickapp.shared.external_fetch.external_url_fetcher import ( + ExternalFetchDisabledError, + ExternalFetchError, + ExternalUrlFetcher, + FetchedBytes, +) + +logger = logging.getLogger(__name__) + +# Non-``text/*`` MIME types that are nevertheless textual and safe to decode / +# inline. Kept small and explicit in phase-1 (no content-sniffing library). +_TEXTUAL_APPLICATION_TYPES = frozenset( + { + "application/json", + "application/xml", + "application/xhtml+xml", + "application/javascript", + "application/ecmascript", + "application/x-yaml", + "application/yaml", + "application/x-www-form-urlencoded", + "application/csv", + } +) + + +def _base_content_type(content_type: str | None) -> str | None: + """Return the lower-cased MIME type without parameters (``; charset=...``).""" + if not content_type: + return None + return content_type.split(";", 1)[0].strip().lower() + + +def _charset(content_type: str | None) -> str | None: + if not content_type: + return None + msg = Message() + msg["content-type"] = content_type + charset = msg.get_param("charset") + return charset if isinstance(charset, str) and charset else None + + +@inject +class WebContentFetcher: + """Classify + fetch + decode helper shared by the built-in web tools. + + A thin wrapper over :class:`ExternalUrlFetcher` that maps its scheme + classification and egress errors into ``InvalidToolCallParameterException`` + (the tool-facing error shape), mirroring ``FileLoaderService``. The egress + policy itself (admin switch, per-app opt-out, host allowlist, SSRF guard, + size / redirect / timeout caps) lives entirely in the fetcher and is reused + verbatim. + """ + + def __init__( + self, + external_fetcher: ExternalUrlFetcher, + dial_settings: DialSettings, + ) -> None: + self.__external_fetcher = external_fetcher + self.__dial_url = dial_settings.url + + async def fetch_external(self, url: str, parameter_name: str = "url") -> FetchedBytes: + """Classify ``url`` and fetch it, or raise a tool-facing parameter error. + + * DIAL URL -> points the agent at the in-workspace file tools (the fetch + tools are for *external* retrieval only). + * Unsupported scheme -> the canonical unsupported-scheme error. + * External -> fetched through :class:`ExternalUrlFetcher`; its + ``ExternalFetchError`` / ``ExternalFetchDisabledError`` (egress + disabled, host not allowed, SSRF, size, timeout, transport) are + re-raised as ``InvalidToolCallParameterException`` carrying the + existing operator/builder messages. + """ + scheme = classify_url(url, self.__dial_url) + if scheme == UrlScheme.DIAL: + raise InvalidToolCallParameterException( + parameter_name, + f"URL {url} already points to a file in DIAL storage. " + "Use internal_file_read_lines or internal_file_search to read it.", + ) + if scheme == UrlScheme.UNSUPPORTED: + raise unsupported_scheme_error(url, parameter_name) + + try: + return await self.__external_fetcher.fetch(url) + except (ExternalFetchError, ExternalFetchDisabledError) as exc: + raise InvalidToolCallParameterException(parameter_name, str(exc)) from exc + + @staticmethod + def is_textual(content_type: str | None) -> bool: + """Whether a payload with this ``content_type`` can be decoded as text. + + Textual: any ``text/*`` type, an explicit allowlisted ``application/*`` + type, or a structured-syntax suffix (``+json`` / ``+xml``). A missing + content type is treated as **non-textual** — phase-1 fails closed rather + than risk decoding binary bytes into the LLM context. + """ + base = _base_content_type(content_type) + if base is None: + return False + if base.startswith("text/"): + return True + if base in _TEXTUAL_APPLICATION_TYPES: + return True + return base.endswith("+json") or base.endswith("+xml") + + @staticmethod + def decode(data: bytes, content_type: str | None) -> str: + """Decode ``data`` using the response charset, defaulting to UTF-8. + + Uses ``errors="replace"`` so an incorrectly-declared charset degrades to + replacement characters rather than raising — the caller has already + decided the payload is textual. + """ + charset = _charset(content_type) or "utf-8" + try: + return data.decode(charset, errors="replace") + except LookupError: + return data.decode("utf-8", errors="replace") diff --git a/src/quickapp/shared/home_path/__init__.py b/src/quickapp/shared/home_path/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/quickapp/shared/home_path/home_path_module.py b/src/quickapp/shared/home_path/home_path_module.py new file mode 100644 index 00000000..7b820483 --- /dev/null +++ b/src/quickapp/shared/home_path/home_path_module.py @@ -0,0 +1,16 @@ +from fastapi_injector import request_scope +from injector import Binder, Module + +from quickapp.shared.home_path.home_path_resolver import HomePathResolver + + +class HomePathModule(Module): + """DI binding for the shared agent-home path resolver. + + Request-scoped so the agent home dir is resolved once per request and shared + between the DIAL-files tools, the path-argument transformer, and the web-fetch + tool. Previously bound inline in ``DialFilesToolingModule``. + """ + + def configure(self, binder: Binder) -> None: + binder.bind(HomePathResolver, to=HomePathResolver, scope=request_scope) diff --git a/src/quickapp/dial_files_tooling/_home_path_resolver.py b/src/quickapp/shared/home_path/home_path_resolver.py similarity index 93% rename from src/quickapp/dial_files_tooling/_home_path_resolver.py rename to src/quickapp/shared/home_path/home_path_resolver.py index d0141dd7..ee2ec2f0 100644 --- a/src/quickapp/dial_files_tooling/_home_path_resolver.py +++ b/src/quickapp/shared/home_path/home_path_resolver.py @@ -7,13 +7,13 @@ @inject -class _HomePathResolver: +class HomePathResolver: """Resolves agent-home-relative paths to absolute DIAL file URLs and back. The agent home prefix (``files/{appdata}/{agent_home_dir}``) is resolved once per request (it may require a ``my_appdata_home()`` API call) and cached. Shared - by the file tools and the path-argument transformer so the home is resolved a - single time per request. + by the file tools, the path-argument transformer, and the web-fetch tool so the + home is resolved a single time per request. """ def __init__( diff --git a/src/quickapp/web_tooling/__init__.py b/src/quickapp/web_tooling/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/quickapp/web_tooling/_tool_configs.py b/src/quickapp/web_tooling/_tool_configs.py new file mode 100644 index 00000000..e1bc2890 --- /dev/null +++ b/src/quickapp/web_tooling/_tool_configs.py @@ -0,0 +1,50 @@ +from quickapp.common.tool_names import INTERNAL_WEB_FETCH_TOOL_NAME +from quickapp.config.tools.base import ( + ConfigurableSchemaSimpleType, + JsonTypeEnum, + OpenAiToolConfig, + OpenAiToolFunction, + OpenAiToolFunctionParameters, +) +from quickapp.config.tools.display.tool import ToolDisplayConfig, ToolStageConfig +from quickapp.config.tools.internal import InternalTool + +WEB_FETCH_TOOL_CONFIG = InternalTool( + open_ai_tool=OpenAiToolConfig( + function=OpenAiToolFunction( + name=INTERNAL_WEB_FETCH_TOOL_NAME, + description=( + "Fetch a resource from an external http(s) URL (e.g. a README, a " + "source file, a documentation page). Without save_path it returns the " + "text inline in a single call — text only: binary content (images, " + "PDFs, archives) or a resource too large to fit inline is rejected, " + "re-call with a save_path instead. With save_path it persists the " + "resource (any content type) at that workspace-relative path under the " + "agent home and returns the saved path (+ a short preview for text), so " + "you can then read it with internal_file_read_lines / " + "internal_file_search. DIAL file paths (files/...) are not fetched " + "here; read them with the file tools." + ), + parameters=OpenAiToolFunctionParameters( + type=JsonTypeEnum.object, + properties={ + "url": ConfigurableSchemaSimpleType( + type=JsonTypeEnum.string, + description="The external http(s) URL to fetch.", + ), + "save_path": ConfigurableSchemaSimpleType( + type=JsonTypeEnum.string, + description=( + "Optional workspace-relative destination (e.g. 'data.py' or " + "'docs/readme.md'). Omit to read the content inline; provide " + "it to save the resource under the agent home and get back " + "the saved path. Must not be an absolute 'files/...' URL." + ), + ), + }, + required=["url"], + ), + ) + ), + display=ToolDisplayConfig(stage=ToolStageConfig(name="Fetch web page")), +) diff --git a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py new file mode 100644 index 00000000..ba66aa33 --- /dev/null +++ b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py @@ -0,0 +1,19 @@ +from typing import Any + +from injector import inject + +from quickapp.common import TimedStageWrapper, ToolCallResult + + +@inject +class _WebFetchStageWrapper(TimedStageWrapper): + + def _get_formatted_parameters(self, parameters: dict[str, Any]) -> str: + url = parameters.get("url") + return f"`{url}`" if url else "" + + def _build_debug_info_from_exception(self, exception: Exception) -> str: + return f"### Exception:\n\r{exception}\n\r" + + def _build_debug_info_from_result(self, result: ToolCallResult) -> str: + return f"### Fetched content:\n\r{result.content}\n\r" diff --git a/src/quickapp/web_tooling/_web_fetch_tool.py b/src/quickapp/web_tooling/_web_fetch_tool.py new file mode 100644 index 00000000..0f0c77b3 --- /dev/null +++ b/src/quickapp/web_tooling/_web_fetch_tool.py @@ -0,0 +1,186 @@ +from pathlib import PurePosixPath +from typing import Any + +from aidial_client._exception import DialException, EtagMismatchError +from injector import AssistedBuilder, inject + +from quickapp.common import StagedBaseTool, ToolCallResult +from quickapp.common.abstract.base_tool_argument_transformer import ToolArgumentTransformer +from quickapp.common.base_stage_wrapper import BaseStageWrapper +from quickapp.common.exceptions import InvalidToolCallParameterException +from quickapp.common.perf_timer.perf_timer import PerformanceTimer +from quickapp.config.application import StageDisplayLevel +from quickapp.config.tools.internal import InternalTool +from quickapp.dial_core_services.dial_file_service import DialFileService +from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher +from quickapp.shared.home_path.home_path_resolver import HomePathResolver +from quickapp.web_tooling._web_fetch_stage_wrapper import _WebFetchStageWrapper + +# Preview shown for textual saves so the agent can decide whether to read the +# full file. Kept short to stay well below any offload threshold. +_PREVIEW_CHARS = 1000 + +# Bound on collision-avoidance retries when the target filename already exists. +_MAX_UNIQUE_ATTEMPTS = 100 + + +@inject +class _WebFetchTool(StagedBaseTool): + """``internal_web_fetch`` — fetch an external resource; read it or save it. + + A single optional ``save_path`` selects the behaviour: + + * **omitted** — the payload must be textual and within ``max_inline_size``; + the decoded text is returned inline. Binary or oversized content is rejected + with guidance to re-call with a ``save_path`` — the tool never truncates and + never offloads. The inline cap defaults to the global offload threshold, so a + returned result is always below the offloader's trigger + (see docs/designs/web_fetch_tool.md, Component 4a). + * **given** — the fetched bytes (any content type) are persisted at + ``save_path`` under the agent home via the shared home-path resolver and a + bytes write, and the saved workspace-relative path (+ a short preview for + textual content) is returned so the ``internal_file_*`` tools can operate on + it. The file is never surfaced to the user choice (``result.attachments`` is + left empty), unlike ``_WriteFileTool``. + """ + + def __init__( + self, + stage_wrapper_builder: AssistedBuilder[_WebFetchStageWrapper], + tool_config: InternalTool, + perf_timer: PerformanceTimer, + web_content_fetcher: WebContentFetcher, + dial_file_service: DialFileService, + home_resolver: HomePathResolver, + max_inline_size: int, + stage_display_level: StageDisplayLevel = StageDisplayLevel.INFO, + argument_transformers: list[ToolArgumentTransformer] | None = None, + **kwargs: Any, + ): + super().__init__( + stage_wrapper_builder=stage_wrapper_builder, # type: ignore[arg-type] + tool_config=tool_config, + perf_timer=perf_timer, + stage_display_level=stage_display_level, + argument_transformers=argument_transformers, + **kwargs, + ) + self.__web_content_fetcher = web_content_fetcher + self.__dial_file_service = dial_file_service + self.__home_resolver = home_resolver + self.__max_inline_size = max_inline_size + + async def _run_in_stage_async( + self, + stage_wrapper: BaseStageWrapper | None = None, + *args: Any, + **kwargs: Any, + ) -> ToolCallResult: + url: str = kwargs["url"] + save_path: str | None = kwargs.get("save_path") + + fetched = await self.__web_content_fetcher.fetch_external(url, parameter_name="url") + + if save_path is None: + result = self.__load_into_context(url, fetched) + else: + result = await self.__save_to_workspace(save_path, fetched) + + if stage_wrapper: + stage_wrapper.add_result(result) + return result + + def __load_into_context(self, url: str, fetched: Any) -> ToolCallResult: + if not WebContentFetcher.is_textual(fetched.content_type): + raise InvalidToolCallParameterException( + "url", + f"URL {url} returned non-text content " + f"(content-type: {fetched.content_type or 'unknown'}), which cannot be " + "loaded into context. Re-call with a save_path to persist it to the " + "workspace instead.", + ) + + text = WebContentFetcher.decode(fetched.data, fetched.content_type) + + # Guard against dumping unbounded content into the LLM context. Reject at or + # above the cap so the returned content stays strictly below it (and, at the + # default where the cap equals the offload threshold, below the offloader's + # trigger too). + content_size = len(text.encode("utf-8")) + if content_size >= self.__max_inline_size: + raise InvalidToolCallParameterException( + "url", + f"URL {url} content is too large to load inline " + f"({content_size} bytes >= {self.__max_inline_size} byte limit). " + "Re-call with a save_path to persist it to the workspace, then read " + "ranges with internal_file_read_lines / internal_file_search.", + ) + + return ToolCallResult( + content=text, + content_type=fetched.content_type or "text/plain", + ) + + async def __save_to_workspace(self, save_path: str, fetched: Any) -> ToolCallResult: + # A ``files/``-prefixed path would be returned verbatim by + # ``resolve_appdata_url`` (bypassing home-prefixing and traversal validation), + # escaping the agent home. Require a home-relative destination. + if save_path.startswith("files/"): + raise InvalidToolCallParameterException( + "save_path", + "save_path must be a workspace-relative path under the agent home " + "(e.g. 'data.py' or 'docs/readme.md'), not an absolute 'files/...' URL.", + ) + + content_type = fetched.content_type or "application/octet-stream" + display_path = await self.__write_unique(save_path, fetched.data, content_type) + + size = len(fetched.data) + content = f"Saved {display_path} ({content_type}, {size} bytes)." + if WebContentFetcher.is_textual(fetched.content_type): + preview = WebContentFetcher.decode(fetched.data, fetched.content_type)[:_PREVIEW_CHARS] + content = f"{content}\n\nPreview:\n{preview}" + + return ToolCallResult(content=content, content_type="text/plain") + + async def __write_unique(self, save_path: str, data: bytes, content_type: str) -> str: + """Write ``data`` at ``save_path`` under the agent home, uniquifying on collision. + + Uses ``overwrite=False`` so an existing file is never clobbered: on an + ``If-None-Match`` failure the name is uniquified (``name-1.ext``, + ``name-2.ext``, …) and retried. Returns the workspace-relative display path + actually written. + """ + for attempt in range(_MAX_UNIQUE_ATTEMPTS): + candidate = save_path if attempt == 0 else _with_suffix(save_path, attempt) + target_url = await self.__home_resolver.resolve_appdata_url(candidate) + display_path = await self.__home_resolver.to_display_path(target_url) + try: + await self.__dial_file_service.write_bytes( + url=target_url, + content=data, + content_type=content_type, + overwrite=False, + ) + return display_path + except EtagMismatchError: + continue + except DialException as e: + if e.status_code == 403: + raise InvalidToolCallParameterException( + "save_path", f"access denied: {display_path}" + ) from e + raise + raise InvalidToolCallParameterException( + "save_path", + f"Could not find a free filename for '{save_path}' after " + f"{_MAX_UNIQUE_ATTEMPTS} attempts; too many saves share this name.", + ) + + +def _with_suffix(path: str, n: int) -> str: + """Insert ``-n`` before the extension: ``data.py`` -> ``data-1.py``.""" + pure = PurePosixPath(path) + suffix = "".join(pure.suffixes) + stem = path[: -len(suffix)] if suffix else path + return f"{stem}-{n}{suffix}" diff --git a/src/quickapp/web_tooling/web_tooling_module.py b/src/quickapp/web_tooling/web_tooling_module.py new file mode 100644 index 00000000..ad87c0a6 --- /dev/null +++ b/src/quickapp/web_tooling/web_tooling_module.py @@ -0,0 +1,88 @@ +import logging + +from fastapi_injector import request_scope +from injector import AssistedBuilder, Binder, Module, multiprovider + +from quickapp.common import StagedBaseTool +from quickapp.common.exceptions import InitializationException, ToolInitializationException +from quickapp.common.preview import preview_module +from quickapp.config.application import ApplicationConfig +from quickapp.config.web_fetch import WebFetchConfig +from quickapp.shared.external_fetch.external_url_fetch_policy_resolver import ( + ExternalUrlFetchPolicyResolver, +) +from quickapp.web_tooling._tool_configs import WEB_FETCH_TOOL_CONFIG +from quickapp.web_tooling._web_fetch_stage_wrapper import _WebFetchStageWrapper +from quickapp.web_tooling._web_fetch_tool import _WebFetchTool + +logger = logging.getLogger(__name__) + +_EGRESS_DISABLED_MESSAGE = ( + "internal_web_fetch requires external URL fetching, which is disabled. Enable it " + "via the admin switch EXTERNAL_URL_FETCH_ENABLED and do not set " + "features.external_url_fetch.enabled=false, or remove features.web_fetch." +) + + +@preview_module +class WebToolingModule(Module): + """Built-in web tools. Currently the single ``internal_web_fetch`` tool. + + Preview-gated at the module level; ``features.web_fetch`` is itself a + ``PreviewField``. Kept separate from the DIAL-files family because it writes + no file (a plain ``StagedBaseTool``, not a ``_DialFileTool``). + + The tool cannot function without external egress, so it is gated on the egress + policy at initialization: if ``features.web_fetch.enabled`` is set while external + fetching is disabled (admin cap or per-app opt-out), the tool is **not** exposed + and a hard ``ToolInitializationException`` surfaces the contradictory config — + rather than exposing a tool that would fail on every call. + """ + + def configure(self, binder: Binder) -> None: + binder.bind(_WebFetchStageWrapper, to=_WebFetchStageWrapper, scope=request_scope) + binder.bind(_WebFetchTool, to=_WebFetchTool, scope=request_scope) + logger.debug("WebToolingModule configuration completed") + + @staticmethod + def _web_fetch_config(app_config: ApplicationConfig) -> WebFetchConfig | None: + features = app_config.features + return features.web_fetch if features else None + + @multiprovider + def _provide_web_fetch_tools( + self, + app_config: ApplicationConfig, + policy: ExternalUrlFetchPolicyResolver, + tool_builder: AssistedBuilder[_WebFetchTool], + ) -> list[StagedBaseTool]: + cfg = self._web_fetch_config(app_config) + # Not requested, or requested while egress is disabled (surfaced as an + # initialization error below): keep the tool unexposed either way. + if cfg is None or not cfg.enabled or not policy.is_enabled(): + return [] + + return [ + tool_builder.build( + tool_config=WEB_FETCH_TOOL_CONFIG, + name=WEB_FETCH_TOOL_CONFIG.open_ai_tool.function.name, + description=WEB_FETCH_TOOL_CONFIG.open_ai_tool.function.description, + max_inline_size=cfg.max_inline_size, + ) + ] + + @multiprovider + def _provide_initialization_exceptions( + self, + app_config: ApplicationConfig, + policy: ExternalUrlFetchPolicyResolver, + ) -> list[InitializationException]: + cfg = self._web_fetch_config(app_config) + if cfg is not None and cfg.enabled and not policy.is_enabled(): + return [ + ToolInitializationException( + _EGRESS_DISABLED_MESSAGE, + tool_name=WEB_FETCH_TOOL_CONFIG.open_ai_tool.function.name, + ) + ] + return [] diff --git a/src/scripts/dump_internal_tools.py b/src/scripts/dump_internal_tools.py index a3e2e060..28b00a6b 100644 --- a/src/scripts/dump_internal_tools.py +++ b/src/scripts/dump_internal_tools.py @@ -39,6 +39,7 @@ from quickapp.config.prompt import CustomSystemPromptConfig from quickapp.config.timestamp import ToolCallTimestampConfig from quickapp.config.tools.const import ALL_MIME_TYPES +from quickapp.config.web_fetch import WebFetchConfig from quickapp.core.agent import OrchestratorCapabilities from quickapp.core.agent._orchestrator_deployment_initializer import ( _OrchestratorDeploymentInitializer, @@ -72,7 +73,11 @@ def build_dump_application_config() -> ApplicationConfig: ), ], tool_sets=[], - features=Features(timestamp=ToolCallTimestampConfig(), dial_files=DialFilesConfig()), + features=Features( + timestamp=ToolCallTimestampConfig(), + dial_files=DialFilesConfig(), + web_fetch=WebFetchConfig(enabled=True), + ), ) @@ -127,6 +132,9 @@ async def _gather_internal_tools_manifest() -> list[dict[str, Any]]: # load_env can inject empty auth vars; set deterministic non-empty fallbacks. _set_env_if_empty("OPENAI_API_KEY", "dump-internal-tools") _set_env_if_empty("OPENAI_ADMIN_KEY", "dump-internal-tools") + # internal_web_fetch is gated on external egress being enabled (WebToolingModule); + # enable the admin cap so the tool materializes in the reference manifest. + _set_env_if_empty("EXTERNAL_URL_FETCH_ENABLED", "true") modules = AppFactory.build_di_modules() injector = Injector(modules) diff --git a/src/tests/unit_tests/dial_files_tooling/_helpers.py b/src/tests/unit_tests/dial_files_tooling/_helpers.py index ec0860a0..8f50446f 100644 --- a/src/tests/unit_tests/dial_files_tooling/_helpers.py +++ b/src/tests/unit_tests/dial_files_tooling/_helpers.py @@ -4,7 +4,7 @@ from quickapp.config.dial_files import DialFilesConfig from quickapp.dial_core_services.dial_file_service import DialFileService -from quickapp.dial_files_tooling._home_path_resolver import _HomePathResolver +from quickapp.shared.home_path.home_path_resolver import HomePathResolver def make_dial_client(appdata: str | None = "appbucket") -> MagicMock: @@ -54,5 +54,5 @@ def make_tool[ToolT]( perf_timer=MagicMock(), dial_file_service=service, dial_files_config=config, - home_resolver=_HomePathResolver(service, config), + home_resolver=HomePathResolver(service, config), ) diff --git a/src/tests/unit_tests/dial_files_tooling/test_base_file_tool.py b/src/tests/unit_tests/dial_files_tooling/test_base_file_tool.py index 4cc519d1..b3a629f5 100644 --- a/src/tests/unit_tests/dial_files_tooling/test_base_file_tool.py +++ b/src/tests/unit_tests/dial_files_tooling/test_base_file_tool.py @@ -7,8 +7,9 @@ from tests.unit_tests.dial_files_tooling._helpers import make_service, make_tool # Home-resolution / display-path / path-validation logic lives in -# `_HomePathResolver` and `_utils` and is covered by test_home_path_resolver.py. -# This module covers tool-level behaviour built on top of them. +# `HomePathResolver` (shared/home_path) and `_utils`, and is covered by +# tests/unit_tests/shared/test_home_path_resolver.py. This module covers +# tool-level behaviour built on top of them. class TestPermissionDeniedWrapper: diff --git a/src/tests/unit_tests/dial_files_tooling/test_path_argument_transformer.py b/src/tests/unit_tests/dial_files_tooling/test_path_argument_transformer.py index 7ff52622..cb1b9370 100644 --- a/src/tests/unit_tests/dial_files_tooling/test_path_argument_transformer.py +++ b/src/tests/unit_tests/dial_files_tooling/test_path_argument_transformer.py @@ -1,14 +1,14 @@ import pytest -from quickapp.dial_files_tooling._home_path_resolver import _HomePathResolver from quickapp.dial_files_tooling._path_argument_transformer import _AppdataHomePathTransformer +from quickapp.shared.home_path.home_path_resolver import HomePathResolver from tests.unit_tests.dial_files_tooling._helpers import make_config, make_service def _make_transformer( appdata: str | None = "appbucket", agent_home_dir: str = "" ) -> _AppdataHomePathTransformer: - resolver = _HomePathResolver( + resolver = HomePathResolver( dial_file_service=make_service(appdata=appdata), dial_files_config=make_config(agent_home_dir=agent_home_dir), ) diff --git a/src/tests/unit_tests/dial_files_tooling/test_home_path_resolver.py b/src/tests/unit_tests/shared/test_home_path_resolver.py similarity index 96% rename from src/tests/unit_tests/dial_files_tooling/test_home_path_resolver.py rename to src/tests/unit_tests/shared/test_home_path_resolver.py index 1c966582..22b990b7 100644 --- a/src/tests/unit_tests/dial_files_tooling/test_home_path_resolver.py +++ b/src/tests/unit_tests/shared/test_home_path_resolver.py @@ -1,13 +1,13 @@ import pytest from quickapp.common.exceptions import InvalidToolCallParameterException -from quickapp.dial_files_tooling._home_path_resolver import _HomePathResolver from quickapp.dial_files_tooling._utils import reject_absolute_path +from quickapp.shared.home_path.home_path_resolver import HomePathResolver from tests.unit_tests.dial_files_tooling._helpers import make_config, make_service -def _resolver(agent_home_dir: str = "", appdata: str | None = "appbucket") -> _HomePathResolver: - return _HomePathResolver( +def _resolver(agent_home_dir: str = "", appdata: str | None = "appbucket") -> HomePathResolver: + return HomePathResolver( dial_file_service=make_service(appdata=appdata), dial_files_config=make_config(agent_home_dir), ) diff --git a/src/tests/unit_tests/shared/test_web_content_fetcher.py b/src/tests/unit_tests/shared/test_web_content_fetcher.py new file mode 100644 index 00000000..92fbfaff --- /dev/null +++ b/src/tests/unit_tests/shared/test_web_content_fetcher.py @@ -0,0 +1,124 @@ +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from quickapp.common.exceptions import InvalidToolCallParameterException +from quickapp.shared.external_fetch.external_url_fetcher import ( + ExternalFetchDisabledError, + ExternalFetchError, + FetchedBytes, +) +from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher + +_DIAL_URL = "https://dial.example.com" + + +def _make_fetcher(fetch_result: FetchedBytes | Exception | None = None) -> WebContentFetcher: + external = MagicMock() + if isinstance(fetch_result, Exception): + external.fetch = AsyncMock(side_effect=fetch_result) + else: + external.fetch = AsyncMock(return_value=fetch_result) + dial_settings = MagicMock() + dial_settings.url = _DIAL_URL + return WebContentFetcher(external_fetcher=external, dial_settings=dial_settings) + + +class TestFetchExternal: + @pytest.mark.asyncio + async def test_external_url_fetched(self): + fetched = FetchedBytes(data=b"hello", content_type="text/plain", filename="a.txt") + fetcher = _make_fetcher(fetched) + result = await fetcher.fetch_external("https://example.com/a.txt") + assert result is fetched + + @pytest.mark.asyncio + async def test_dial_relative_path_rejected(self): + fetcher = _make_fetcher() + with pytest.raises(InvalidToolCallParameterException) as exc: + await fetcher.fetch_external("files/bucket/doc.md") + assert exc.value.parameter_name == "url" + assert "internal_file_read_lines" in exc.value.message + + @pytest.mark.asyncio + async def test_dial_host_url_rejected(self): + fetcher = _make_fetcher() + with pytest.raises(InvalidToolCallParameterException): + await fetcher.fetch_external(f"{_DIAL_URL}/v1/files/x/doc.md") + + @pytest.mark.asyncio + async def test_unsupported_scheme_rejected(self): + fetcher = _make_fetcher() + with pytest.raises(InvalidToolCallParameterException) as exc: + await fetcher.fetch_external("ftp://example.com/x") + assert "scheme not supported" in exc.value.message + + @pytest.mark.asyncio + async def test_egress_disabled_wrapped_as_parameter_error(self): + fetcher = _make_fetcher(ExternalFetchDisabledError(reason="admin", url="https://x.com")) + with pytest.raises(InvalidToolCallParameterException) as exc: + await fetcher.fetch_external("https://x.com") + assert exc.value.parameter_name == "url" + assert "EXTERNAL_URL_FETCH_ENABLED" in exc.value.message + + @pytest.mark.asyncio + async def test_fetch_error_wrapped_as_parameter_error(self): + fetcher = _make_fetcher(ExternalFetchError(reason="size_limit", url="https://x.com")) + with pytest.raises(InvalidToolCallParameterException) as exc: + await fetcher.fetch_external("https://x.com") + assert exc.value.parameter_name == "url" + + @pytest.mark.asyncio + async def test_custom_parameter_name_used(self): + fetcher = _make_fetcher() + with pytest.raises(InvalidToolCallParameterException) as exc: + await fetcher.fetch_external("files/b/x", parameter_name="target") + assert exc.value.parameter_name == "target" + + +class TestIsTextual: + @pytest.mark.parametrize( + "content_type", + [ + "text/plain", + "text/markdown; charset=utf-8", + "text/html", + "application/json", + "application/xml", + "application/vnd.api+json", + "image/svg+xml", + ], + ) + def test_textual(self, content_type): + assert WebContentFetcher.is_textual(content_type) is True + + @pytest.mark.parametrize( + "content_type", + [ + None, + "application/octet-stream", + "image/png", + "application/pdf", + "application/zip", + ], + ) + def test_non_textual(self, content_type): + assert WebContentFetcher.is_textual(content_type) is False + + +class TestDecode: + def test_utf8_default(self): + assert WebContentFetcher.decode("héllo".encode("utf-8"), "text/plain") == "héllo" + + def test_explicit_charset(self): + assert ( + WebContentFetcher.decode("héllo".encode("latin-1"), "text/plain; charset=latin-1") + == "héllo" + ) + + def test_unknown_charset_falls_back_to_utf8(self): + assert WebContentFetcher.decode(b"hi", "text/plain; charset=not-a-charset") == "hi" + + def test_invalid_bytes_replaced_not_raised(self): + # Lone continuation byte is invalid UTF-8; replace rather than raise. + assert WebContentFetcher.decode(b"\xff", "text/plain") == "�" diff --git a/src/tests/unit_tests/web_tooling/__init__.py b/src/tests/unit_tests/web_tooling/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py b/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py new file mode 100644 index 00000000..45b20112 --- /dev/null +++ b/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py @@ -0,0 +1,188 @@ +from unittest.mock import AsyncMock, MagicMock + +import pytest +from aidial_client._exception import DialException, EtagMismatchError + +from quickapp.common.exceptions import InvalidToolCallParameterException +from quickapp.dial_core_services.dial_file_service import DialFileService +from quickapp.shared.external_fetch.external_url_fetcher import FetchedBytes +from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher +from quickapp.shared.home_path.home_path_resolver import HomePathResolver +from quickapp.web_tooling._tool_configs import WEB_FETCH_TOOL_CONFIG +from quickapp.web_tooling._web_fetch_tool import _WebFetchTool + +_HOME = "files/appbucket/agent/" + + +def _make_tool( + fetched: FetchedBytes | Exception | None = None, + *, + max_inline_size: int = 40_000, + write_bytes_side_effect=None, +) -> tuple[_WebFetchTool, MagicMock]: + fetcher = MagicMock(spec=WebContentFetcher) + if isinstance(fetched, Exception): + fetcher.fetch_external = AsyncMock(side_effect=fetched) + else: + fetcher.fetch_external = AsyncMock(return_value=fetched) + + dial_file_service = MagicMock(spec=DialFileService) + dial_file_service.write_bytes = AsyncMock(side_effect=write_bytes_side_effect) + + # Real resolution semantics (home under files/appbucket/agent/), no network. + home_resolver = MagicMock(spec=HomePathResolver) + home_resolver.resolve_appdata_url = AsyncMock(side_effect=lambda p: f"{_HOME}{p}") + home_resolver.to_display_path = AsyncMock(side_effect=lambda u: u.removeprefix(_HOME)) + + tool = _WebFetchTool( + stage_wrapper_builder=MagicMock(), + tool_config=WEB_FETCH_TOOL_CONFIG, + perf_timer=MagicMock(), + web_content_fetcher=fetcher, + dial_file_service=dial_file_service, + home_resolver=home_resolver, + max_inline_size=max_inline_size, + ) + return tool, dial_file_service + + +class TestLoadIntoContext: + @pytest.mark.asyncio + async def test_textual_within_guard_returns_inline_content(self): + fetched = FetchedBytes( + data=b"# Title\nbody", content_type="text/markdown", filename="README.md" + ) + tool, service = _make_tool(fetched) + result = await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/README.md") + assert result.content == "# Title\nbody" + assert result.content_type == "text/markdown" + assert not result.attachments + service.write_bytes.assert_not_awaited() + + @pytest.mark.asyncio + async def test_charset_respected(self): + fetched = FetchedBytes( + data="café".encode("latin-1"), + content_type="text/plain; charset=latin-1", + filename="a.txt", + ) + tool, _ = _make_tool(fetched) + result = await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a.txt") + assert result.content == "café" + + @pytest.mark.asyncio + async def test_binary_rejected_pointing_at_save_path(self): + fetched = FetchedBytes(data=b"\x89PNG\r\n", content_type="image/png", filename="a.png") + tool, _ = _make_tool(fetched) + with pytest.raises(InvalidToolCallParameterException) as exc: + await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a.png") + assert exc.value.parameter_name == "url" + assert "save_path" in exc.value.message + + @pytest.mark.asyncio + async def test_missing_content_type_treated_as_binary(self): + fetched = FetchedBytes(data=b"plain", content_type=None, filename="a") + tool, _ = _make_tool(fetched) + with pytest.raises(InvalidToolCallParameterException) as exc: + await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a") + assert "save_path" in exc.value.message + + @pytest.mark.asyncio + async def test_oversized_text_rejected_pointing_at_save_path(self): + big = "a" * 100 + fetched = FetchedBytes( + data=big.encode("utf-8"), content_type="text/plain", filename="big.txt" + ) + tool, _ = _make_tool(fetched, max_inline_size=50) + with pytest.raises(InvalidToolCallParameterException) as exc: + await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/big.txt") + assert exc.value.parameter_name == "url" + assert "save_path" in exc.value.message + + @pytest.mark.asyncio + async def test_content_at_cap_rejected(self): + # Exactly the cap must be rejected so the returned content stays below the + # offload trigger (which fires at >= threshold). + data = b"a" * 50 + fetched = FetchedBytes(data=data, content_type="text/plain", filename="x.txt") + tool, _ = _make_tool(fetched, max_inline_size=50) + with pytest.raises(InvalidToolCallParameterException): + await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/x.txt") + + @pytest.mark.asyncio + async def test_fetch_error_propagates(self): + tool, _ = _make_tool(InvalidToolCallParameterException("url", "egress disabled")) + with pytest.raises(InvalidToolCallParameterException) as exc: + await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/x") + assert "egress disabled" in exc.value.message + + +class TestSaveToWorkspace: + @pytest.mark.asyncio + async def test_textual_saved_returns_path_and_preview(self): + fetched = FetchedBytes( + data=b"def main():\n pass\n", content_type="text/x-python", filename="data.py" + ) + tool, service = _make_tool(fetched) + result = await tool._run_in_stage_async( + stage_wrapper=None, url="https://x.com/data.py", save_path="analysis/data.py" + ) + service.write_bytes.assert_awaited_once() + kwargs = service.write_bytes.await_args.kwargs + assert kwargs["url"] == f"{_HOME}analysis/data.py" + assert kwargs["overwrite"] is False + assert "analysis/data.py" in result.content + assert "Preview:" in result.content + assert "def main" in result.content + assert not result.attachments + + @pytest.mark.asyncio + async def test_binary_saved_no_inline_body(self): + fetched = FetchedBytes(data=b"\x89PNG\r\n", content_type="image/png", filename="a.png") + tool, service = _make_tool(fetched) + result = await tool._run_in_stage_async( + stage_wrapper=None, url="https://x.com/a.png", save_path="img/a.png" + ) + assert "img/a.png" in result.content + assert "image/png" in result.content + assert "Preview:" not in result.content + service.write_bytes.assert_awaited_once() + + @pytest.mark.asyncio + async def test_files_prefixed_save_path_rejected(self): + fetched = FetchedBytes(data=b"x", content_type="text/plain", filename="x.txt") + tool, service = _make_tool(fetched) + with pytest.raises(InvalidToolCallParameterException) as exc: + await tool._run_in_stage_async( + stage_wrapper=None, url="https://x.com/x.txt", save_path="files/other/x.txt" + ) + assert exc.value.parameter_name == "save_path" + service.write_bytes.assert_not_awaited() + + @pytest.mark.asyncio + async def test_collision_uniquifies_filename(self): + fetched = FetchedBytes(data=b"x", content_type="text/plain", filename="data.py") + # First write collides (etag), second succeeds. + tool, service = _make_tool( + fetched, write_bytes_side_effect=[EtagMismatchError("conflict"), None] + ) + result = await tool._run_in_stage_async( + stage_wrapper=None, url="https://x.com/data.py", save_path="data.py" + ) + assert service.write_bytes.await_count == 2 + # Second (successful) write targeted the uniquified name. + assert service.write_bytes.await_args.kwargs["url"] == f"{_HOME}data-1.py" + assert "data-1.py" in result.content + + @pytest.mark.asyncio + async def test_permission_denied_raises_parameter_error(self): + fetched = FetchedBytes(data=b"x", content_type="text/plain", filename="x.txt") + tool, _ = _make_tool( + fetched, write_bytes_side_effect=DialException("forbidden", status_code=403) + ) + with pytest.raises(InvalidToolCallParameterException) as exc: + await tool._run_in_stage_async( + stage_wrapper=None, url="https://x.com/x.txt", save_path="x.txt" + ) + assert exc.value.parameter_name == "save_path" + assert "access denied" in exc.value.message diff --git a/src/tests/unit_tests/web_tooling/test_web_tooling_module.py b/src/tests/unit_tests/web_tooling/test_web_tooling_module.py new file mode 100644 index 00000000..0a96ff93 --- /dev/null +++ b/src/tests/unit_tests/web_tooling/test_web_tooling_module.py @@ -0,0 +1,94 @@ +from unittest.mock import MagicMock + +from quickapp.common.exceptions import ToolInitializationException +from quickapp.config.web_fetch import WebFetchConfig +from quickapp.web_tooling.web_tooling_module import WebToolingModule + + +def _app_config(web_fetch: WebFetchConfig | None) -> MagicMock: + app_config = MagicMock() + app_config.features = MagicMock() + app_config.features.web_fetch = web_fetch + return app_config + + +def _policy(enabled: bool = True) -> MagicMock: + policy = MagicMock() + policy.is_enabled.return_value = enabled + return policy + + +def _build_tools(app_config: MagicMock, egress_enabled: bool = True) -> tuple: + module = WebToolingModule() + builder = MagicMock() + built = MagicMock() + builder.build.return_value = built + tools = module._provide_web_fetch_tools( + app_config=app_config, policy=_policy(egress_enabled), tool_builder=builder + ) + return tools, builder, built + + +class TestWebToolingModuleGating: + def test_enabled_provides_tool(self): + tools, builder, built = _build_tools(_app_config(WebFetchConfig(enabled=True))) + assert tools == [built] + assert builder.build.call_args.kwargs["max_inline_size"] == WebFetchConfig().max_inline_size + + def test_disabled_provides_nothing(self): + tools, _, _ = _build_tools(_app_config(WebFetchConfig(enabled=False))) + assert tools == [] + + def test_absent_config_provides_nothing(self): + tools, _, _ = _build_tools(_app_config(None)) + assert tools == [] + + def test_no_features_provides_nothing(self): + app_config = MagicMock() + app_config.features = None + tools, _, _ = _build_tools(app_config) + assert tools == [] + + def test_custom_max_inline_size_passed_through(self): + tools, builder, _ = _build_tools( + _app_config(WebFetchConfig(enabled=True, max_inline_size=1234)) + ) + assert builder.build.call_args.kwargs["max_inline_size"] == 1234 + + def test_module_is_preview(self): + from quickapp.common.preview import is_preview_module + + assert is_preview_module(WebToolingModule()) + + +class TestEgressGate: + def test_enabled_but_egress_disabled_provides_nothing(self): + tools, builder, _ = _build_tools( + _app_config(WebFetchConfig(enabled=True)), egress_enabled=False + ) + assert tools == [] + builder.build.assert_not_called() + + def test_enabled_but_egress_disabled_raises_initialization_error(self): + module = WebToolingModule() + errors = module._provide_initialization_exceptions( + app_config=_app_config(WebFetchConfig(enabled=True)), policy=_policy(False) + ) + assert len(errors) == 1 + assert isinstance(errors[0], ToolInitializationException) + assert errors[0].is_hard + assert errors[0].tool_name == "internal_web_fetch" + + def test_enabled_with_egress_enabled_has_no_initialization_error(self): + module = WebToolingModule() + errors = module._provide_initialization_exceptions( + app_config=_app_config(WebFetchConfig(enabled=True)), policy=_policy(True) + ) + assert errors == [] + + def test_disabled_has_no_initialization_error_even_if_egress_disabled(self): + module = WebToolingModule() + errors = module._provide_initialization_exceptions( + app_config=_app_config(WebFetchConfig(enabled=False)), policy=_policy(False) + ) + assert errors == [] From fc7d3ae1779b5d76898beebdec52b5cfa5f8bce1 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 6 Jul 2026 09:02:17 +0300 Subject: [PATCH 06/12] fix(web-fetch): render fetched content as verbatim code block in stage The stage dumped raw fetched content directly, so a fetched Markdown file (e.g. a README) was rendered by the UI as formatted Markdown. Wrap it in a fenced code block to show the text verbatim, label the URL/content sections, and separate them onto their own lines. --- src/quickapp/web_tooling/_web_fetch_stage_wrapper.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py index ba66aa33..f4249507 100644 --- a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py +++ b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py @@ -3,6 +3,7 @@ from injector import inject from quickapp.common import TimedStageWrapper, ToolCallResult +from quickapp.common.utils import fenced_code_block @inject @@ -10,10 +11,15 @@ class _WebFetchStageWrapper(TimedStageWrapper): def _get_formatted_parameters(self, parameters: dict[str, Any]) -> str: url = parameters.get("url") - return f"`{url}`" if url else "" + # Trailing blank line so the result's "content:" header that is appended + # next starts on its own line (otherwise they render inline). + return f"**URL:** `{url}`\n\n" if url else "" def _build_debug_info_from_exception(self, exception: Exception) -> str: return f"### Exception:\n\r{exception}\n\r" def _build_debug_info_from_result(self, result: ToolCallResult) -> str: - return f"### Fetched content:\n\r{result.content}\n\r" + # Fetched content is often itself Markdown/HTML (e.g. a README). Render it + # inside a fenced code block so the stage shows the fetched text verbatim + # instead of the UI rendering it as formatted Markdown. + return f"**Content:**\n{fenced_code_block(result.content)}\n" From 56097379fa97ea4d5e20e69bc0131b6ececd1593 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 6 Jul 2026 12:43:28 +0300 Subject: [PATCH 07/12] refactor(stage-wrappers): extract config-map param rendering to base helper Four stage wrappers (dial_files, web_fetch, py_interpreter, deployment) carried a verbatim copy of the display-config-map parameter loop, and two of them a copy of _order_parameters. Extract both into BaseStageWrapper as protected helpers (_render_config_map_parameters + _order_parameters); each wrapper's _get_formatted_parameters now delegates to the helper. _get_formatted_parameters stays abstract, so a wrapper that forgets to override still fails hard rather than silently rendering config-map output. py_interpreter and deployment gain the stable ordering; behavior-neutral since the default order (0) preserves their existing input order. --- src/quickapp/common/base_stage_wrapper.py | 30 +++++++++++++++++++ .../deployment_stage_wrapper.py | 13 +------- .../dial_files_tooling/_stage_wrapper.py | 22 +------------- .../_py_interpreter_stage_wrapper.py | 13 +------- .../web_tooling/_web_fetch_stage_wrapper.py | 5 +--- 5 files changed, 34 insertions(+), 49 deletions(-) diff --git a/src/quickapp/common/base_stage_wrapper.py b/src/quickapp/common/base_stage_wrapper.py index 06b81636..235406c9 100644 --- a/src/quickapp/common/base_stage_wrapper.py +++ b/src/quickapp/common/base_stage_wrapper.py @@ -136,6 +136,36 @@ def _get_parameter_value( return str(result_value) + def _order_parameters(self, parameters: dict[str, Any]) -> dict[str, Any]: + # Sort by the configured `order` (default 0). sorted() is stable, so params + # sharing an order keep their original position. + def _order(param_name: str) -> int: + display_config = self._parameters_config_map.get(param_name) + return display_config.order if display_config else 0 + + return dict(sorted(parameters.items(), key=lambda item: _order(item[0]))) + + def _render_config_map_parameters(self, parameters: dict[str, Any]) -> str: + """Render parameters using the per-parameter display config map. + + Shared building block for wrappers whose tool advertises `display.stage` + config on its OpenAI parameters. Parameters are ordered by their configured + `order`, then each is formatted via the `_get_parameter_*` helpers (or a + plain `***name:*** value` fallback when it has no display config). + """ + stage_params = "> #### Request:\n\r" + for param_name, param_value in self._order_parameters(parameters).items(): + if display_config := self._parameters_config_map.get(param_name): + if not display_config.ignore: + stage_params += self._get_parameter_name(param_name, display_config) + stage_params += self._get_value_prefix(display_config) + stage_params += self._get_parameter_value(param_value, display_config) + stage_params += self._get_value_sufix(display_config) + stage_params += "\n\r" + else: + stage_params += f"***{param_name}:*** {param_value}\n\r" + return stage_params + def add_result(self, result: ToolCallResult) -> None: debug_info = self._build_debug_info_from_result(result) self.append_stage_content(debug_info) diff --git a/src/quickapp/dial_deployment_tooling/deployment_stage_wrapper.py b/src/quickapp/dial_deployment_tooling/deployment_stage_wrapper.py index 99494b13..c9cd414a 100644 --- a/src/quickapp/dial_deployment_tooling/deployment_stage_wrapper.py +++ b/src/quickapp/dial_deployment_tooling/deployment_stage_wrapper.py @@ -10,18 +10,7 @@ class DeploymentStageWrapper(TimedStageWrapper): def _get_formatted_parameters(self, parameters: dict[str, Any]) -> str: - stage_params = "> #### Request:\n\r" - for param_name, param_value in parameters.items(): - if display_config := self._parameters_config_map.get(param_name): - if not display_config.ignore: - stage_params += self._get_parameter_name(param_name, display_config) - stage_params += self._get_value_prefix(display_config) - stage_params += self._get_parameter_value(param_value, display_config) - stage_params += self._get_value_sufix(display_config) - stage_params += "\n\r" - else: - stage_params += f"***{param_name}:*** {param_value}\n\r" - return stage_params + return self._render_config_map_parameters(parameters) def _build_debug_info_from_exception(self, exception: Exception) -> str: if isinstance(exception, DialException): diff --git a/src/quickapp/dial_files_tooling/_stage_wrapper.py b/src/quickapp/dial_files_tooling/_stage_wrapper.py index 20a66c02..ff3c183d 100644 --- a/src/quickapp/dial_files_tooling/_stage_wrapper.py +++ b/src/quickapp/dial_files_tooling/_stage_wrapper.py @@ -16,27 +16,7 @@ def _get_stage_title_from_params(self, parameters: dict[str, Any]) -> str: return f" `{title}`" def _get_formatted_parameters(self, parameters: dict[str, Any]) -> str: - stage_params = "> #### Request:\n\r" - for param_name, param_value in self._order_parameters(parameters).items(): - if display_config := self._parameters_config_map.get(param_name): - if not display_config.ignore: - stage_params += self._get_parameter_name(param_name, display_config) - stage_params += self._get_value_prefix(display_config) - stage_params += self._get_parameter_value(param_value, display_config) - stage_params += self._get_value_sufix(display_config) - stage_params += "\n\r" - else: - stage_params += f"***{param_name}:*** {param_value}\n\r" - return stage_params - - def _order_parameters(self, parameters: dict[str, Any]) -> dict[str, Any]: - # Sort by the configured `order` (default 0). sorted() is stable, so params - # sharing an order keep their original position. - def _order(param_name: str) -> int: - display_config = self._parameters_config_map.get(param_name) - return display_config.order if display_config else 0 - - return dict(sorted(parameters.items(), key=lambda item: _order(item[0]))) + return self._render_config_map_parameters(parameters) def _build_debug_info_from_exception(self, exception: Exception) -> str: return f"### Exception:\n\r{exception}\n\r" diff --git a/src/quickapp/internal_tooling/py_interpreter_tooling/_py_interpreter_stage_wrapper.py b/src/quickapp/internal_tooling/py_interpreter_tooling/_py_interpreter_stage_wrapper.py index 7491c7e7..aff00482 100644 --- a/src/quickapp/internal_tooling/py_interpreter_tooling/_py_interpreter_stage_wrapper.py +++ b/src/quickapp/internal_tooling/py_interpreter_tooling/_py_interpreter_stage_wrapper.py @@ -9,18 +9,7 @@ class _PyInterpreterStageWrapper(TimedStageWrapper): def _get_formatted_parameters(self, parameters: dict[str, Any]) -> str: - stage_params = "> #### Request:\n\r" - for param_name, param_value in parameters.items(): - if display_config := self._parameters_config_map.get(param_name): - if not display_config.ignore: - stage_params += self._get_parameter_name(param_name, display_config) - stage_params += self._get_value_prefix(display_config) - stage_params += self._get_parameter_value(param_value, display_config) - stage_params += self._get_value_sufix(display_config) - stage_params += "\n\r" - else: - stage_params += f"***{param_name}:*** {param_value}\n\r" - return stage_params + return self._render_config_map_parameters(parameters) def _build_debug_info_from_exception(self, exception: Exception) -> str: return "> #### Exception:\nGeneral exception occurred while calling other DIAL deployment\n" diff --git a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py index f4249507..45aff9b4 100644 --- a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py +++ b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py @@ -10,10 +10,7 @@ class _WebFetchStageWrapper(TimedStageWrapper): def _get_formatted_parameters(self, parameters: dict[str, Any]) -> str: - url = parameters.get("url") - # Trailing blank line so the result's "content:" header that is appended - # next starts on its own line (otherwise they render inline). - return f"**URL:** `{url}`\n\n" if url else "" + return self._render_config_map_parameters(parameters) def _build_debug_info_from_exception(self, exception: Exception) -> str: return f"### Exception:\n\r{exception}\n\r" From d22888e2223a5eba2f8771cbe93e7d760e27d401 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Fri, 10 Jul 2026 10:22:54 +0300 Subject: [PATCH 08/12] docs: reconcile web fetch design with internal_attachments_get_content (#344) Records why the one-tool form stands after examining the overlap with the lazy-on-demand get-content tool: a new Relationship section (comparison table, incidental-capability rationale, rejected download-only alternative), UC-4 cross-guidance, and two Out of Scope entries (binary-attach enrichment, downstream routing via #279). --- docs/designs/web_fetch_tool.md | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md index 3a0aaa6d..a8946888 100644 --- a/docs/designs/web_fetch_tool.md +++ b/docs/designs/web_fetch_tool.md @@ -9,6 +9,8 @@ An earlier revision of this doc (Status: Approved) shipped this capability as **two dedicated tools** — `internal_web_fetch` (inline text) and `internal_file_download` (persist to the workspace, living in the `internal_file_*` family). This revision collapses them into **one independent tool** whose behavior is selected by a single optional `save_path` argument. The rationale for the reversal is in [One tool, one switch](#one-tool-one-switch-core-design-decision). +A subsequent revision re-examined the design against `internal_attachments_get_content` (the lazy-on-demand attachment tool, which also fetches external urls) and against dropping back to a minimal `download` tool in the file family. Both alternatives were rejected — the analysis confirmed the one-tool form unchanged and is recorded in [Relationship to `internal_attachments_get_content`](#relationship-to-internal_attachments_get_content). + ## Problem Statement The agent today has two halves of a capability but not the bridge between them: @@ -56,6 +58,27 @@ Both branches share the same front half — classify, egress-gate, fetch, decode --- +## Relationship to `internal_attachments_get_content` + +The lazy-on-demand attachment strategy ships a tool that superficially overlaps with this one: `internal_attachments_get_content` (`orchestrator_attachment_strategies/lazy_on_demand/`) also accepts external `http(s)` urls and fetches them through the same egress plumbing (`DialFilePromoter` → `ExternalUrlFetcher`, same two-tier policy). The overlap is mechanical, not conceptual — the two tools answer different questions: + +| | `internal_attachments_get_content` | `internal_web_fetch` | +|---|---|---| +| **Purpose** | Deliver a *conversation attachment* to the orchestrator model as multimodal input | *Agent-initiated* retrieval of a web resource | +| **How content reaches the model** | `result.attachments` entry — the deployment must accept the MIME (`orchestrator_accepts_mime_type`); how the model "sees" it is adapter-dependent | Plain text in the tool result — works with any deployment | +| **Availability** | Only when `attachment_strategy: lazy_on_demand` is configured (the default is `None`) **and** the deployment declares non-empty `input_attachment_types` | `features.web_fetch.enabled` — any strategy, any deployment | +| **Where fetched bytes land** | Flat bucket root (`AttachmentService.upload_bytes`), promotion cache is request-scoped; not addressable by the file tools | Agent home at the caller's `save_path` — round-trips into `internal_file_*` | + +`get_content`'s external-url support exists because PR #279 made external urls first-class *user attachments* — "the user attached `https://…/report.pdf`" must be loadable. Arbitrary-url fetching is incidental capability it inherits from that, not designed behavior, and it covers issue #344 only in a configuration corner (lazy strategy + a deployment that accepts the MIME + an adapter that inlines text attachments). The two things it does **not** cover — a universal one-call text read that works on any deployment, and persistence under the agent home for the file tools — are exactly this tool's two branches. (A minimal `download`-only tool in the file family was also re-considered as an alternative and rejected: it leaves the read half config-dependent and forces two-call reads where every glanced-at page becomes a permanently named workspace file.) + +Consequences: + +- **`get_content` is unchanged by this design.** They share fetch plumbing, not code paths that need reconciling. +- **Where both tools are registered, they don't compete** — they divide the labor. Introducing `internal_web_fetch` relieves `get_content` of the fetch role it plays awkwardly today (see the `file:url::` steering fix, `0553f9a`): the model gets a purpose-named tool for the web, and `get_content` stays about attachments. Binary content the deployment accepts (image, PDF) remains `get_content`'s strength — it delivers what this tool's phase-1 punts to save-only. +- **Routing fetched content to downstream tools is neither tool's job.** "The orchestrator can't read this PDF but tool X can" is already covered by #279's outward url-attachment path (capability-aware via `Deployment.features.url_attachments`, with promoter fallback). This tool's save branch adds *durability* (ephemeral/signed urls outlive the link) and a stable workspace identity the model can hand to several tools — not new routing. + +--- + ## Use Cases ### UC-1: Agent fetches a text resource and reads it in one call (default) @@ -88,7 +111,7 @@ Both branches share the same front half — classify, egress-gate, fetch, decode **Behavior:** Without `save_path` → parameter error: binary content cannot be loaded into context; re-call with a `save_path`. With `save_path` → the bytes are saved and the tool returns the path + content type + size (no inline body, no extraction in phase-1). -**Outcome:** The model never receives garbled bytes in context; binary is reachable only as a saved file it can forward to a capable deployment/tool. +**Outcome:** The model never receives garbled bytes in context; binary is reachable only as a saved file it can forward to a capable deployment/tool. (When the url is a conversation attachment and `internal_attachments_get_content` is registered, that tool may deliver the binary to the orchestrator as multimodal input instead — see [Relationship to `internal_attachments_get_content`](#relationship-to-internal_attachments_get_content).) ### UC-5: External egress is disabled (admin cap or per-app opt-out) @@ -217,6 +240,8 @@ Deferred from phase-1; each is a clean follow-on, not a rework: - **A second dedicated tool / splitting read and save apart.** The prior revision's `internal_file_download` (a `_DialFileTool` subclass in the file family) is intentionally folded into the `save_path` branch. If a future need arises to enable saving independently of read-into-context, a separate flag or tool can be reintroduced; phase-1 treats web retrieval as one capability. - **Auto-derived save filenames.** Saving always uses the caller's `save_path`; the tool never guesses a name from Content-Disposition / URL path / MIME type. A future revision could add an "infer a name" affordance. - **PDF / binary text extraction.** Binary content is save-only (no inline, no extraction) in phase-1. Extraction needs a parser and a preview strategy; future phase. +- **Binary-attach enrichment of the inline branch.** A phase-2 candidate: when the fetched content is binary and the orchestrator deployment accepts its MIME (`orchestrator_accepts_mime_type`), return it as a `result.attachments` entry — `get_content`-style multimodal delivery — instead of the "re-call with a `save_path`" error (UC-4). Closes the binary-read gap for capable deployments without touching phase-1's text-only inline contract. +- **Routing fetched content to downstream tools.** Passing an external resource to a deployment/REST/MCP tool is already covered by PR #279's outward url-attachment path (capability-aware, with promoter fallback); this tool deliberately adds only durability and workspace identity on top (see [Relationship to `internal_attachments_get_content`](#relationship-to-internal_attachments_get_content)). - **Special-casing the tool against the global offload processor.** The tool does not rely on offload (saving covers large content) and does not exempt itself from the global post-processor in phase-1; see Component 4a. - **Pagination / `start_index` on inline reads.** Unnecessary — a save + `internal_file_read_lines` / `internal_file_search` provide ranged access; inline fetch is for content that fits inline. - **Content summarization** (Claude Code-style prompt-over-content). From bf54138be72d82d4ccbd991b52df74df38e9ecc7 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Mon, 13 Jul 2026 10:21:14 +0300 Subject: [PATCH 09/12] feat(web-fetch): truncate oversized inline reads and drop file-tool coupling (#344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Oversized textual content is no longer rejected: the inline branch returns a leading truncation notice (total size, content type, save_path guidance) followed by the content head, cut at a line boundary and budgeted so notice + head stay strictly below max_inline_size — an inline result never reaches the global offload trigger. The stage wrapper splits the notice back off and renders it outside the verbatim content block via the shared _truncation module, so the instruction is never mistaken for fetched text. All agent-facing guidance is now tool-neutral: web_fetch and the file tools are independently gated features, so no message names internal_file_* (which may not be enabled). WebContentFetcher gains a request-scoped fetch cache so the save_path re-call after a truncated read does not re-download, and the tool description steers away from HTML viewer pages toward raw endpoints. Recorded as the third design revision in docs/designs/web_fetch_tool.md. --- docs/designs/web_fetch_tool.md | 59 ++++++++---- docs/generated-app-schema.json | 4 +- docs/generated-internal-tools.json | 2 +- src/quickapp/config/web_fetch.py | 14 +-- .../external_fetch/web_content_fetcher.py | 25 +++-- src/quickapp/web_tooling/_tool_configs.py | 17 ++-- src/quickapp/web_tooling/_truncation.py | 45 +++++++++ .../web_tooling/_web_fetch_stage_wrapper.py | 10 +- src/quickapp/web_tooling/_web_fetch_tool.py | 76 ++++++++++----- .../shared/test_web_content_fetcher.py | 32 ++++++- .../web_tooling/test_web_fetch_tool.py | 95 ++++++++++++++++--- 11 files changed, 293 insertions(+), 86 deletions(-) create mode 100644 src/quickapp/web_tooling/_truncation.py diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md index a8946888..0e83888a 100644 --- a/docs/designs/web_fetch_tool.md +++ b/docs/designs/web_fetch_tool.md @@ -11,6 +11,8 @@ An earlier revision of this doc (Status: Approved) shipped this capability as ** A subsequent revision re-examined the design against `internal_attachments_get_content` (the lazy-on-demand attachment tool, which also fetches external urls) and against dropping back to a minimal `download` tool in the file family. Both alternatives were rejected — the analysis confirmed the one-tool form unchanged and is recorded in [Relationship to `internal_attachments_get_content`](#relationship-to-internal_attachments_get_content). +A third revision, driven by field testing, changed the oversized-inline contract from **reject** to **truncate with an explicit notice**, and made every piece of agent-facing guidance **tool-neutral** (no `internal_file_*` name-drops). The prior contract silently assumed the file tools are enabled alongside this one — they are independently gated features, so an app can legitimately run `web_fetch` without them, and the reject-and-save flow then dead-ends (the agent saves a file no enabled tool can read). The rationale and the revised contract are in [Component 2](#component-2-inline-size-cap--truncation-no-save_path-only); a request-scoped fetch cache was added alongside (Component 3) so the save re-call after a truncated read does not re-download. + ## Problem Statement The agent today has two halves of a capability but not the bridge between them: @@ -27,8 +29,8 @@ Without it, an agent that needs the *contents* of a web page has no path: extern - **Read in one call.** Fetching a text URL returns the content inline in a single tool call — no file written, no follow-up required — with nothing more than a URL. - **Persist for the file tools, on demand.** The same tool, given a destination, pulls a resource into the workspace as a durable DIAL file so the existing `internal_file_*` tools can operate on it. - **One capability, one switch.** A single feature flag exposes the whole fetch capability; the agent chooses read-now vs. keep-it per call by supplying (or omitting) one argument. -- **Self-contained.** The tool depends only on the shared fetch helper and the shared home-path resolver — not on the `internal_file_*` family or the large-tool-response offload processor. -- **Bounded inline output.** Inline reads never dump unbounded content into the LLM context; a hard, tool-owned size guard redirects oversized fetches to a save. +- **Self-contained.** The tool depends only on the shared fetch helper and the shared home-path resolver — not on the `internal_file_*` family or the large-tool-response offload processor. This extends to what the tool *says*: every agent-facing message is tool-neutral, because the file tools (or any other content-processing tool) are independently gated and may not be enabled. +- **Bounded inline output.** Inline reads never dump unbounded content into the LLM context; a hard, tool-owned cap truncates oversized text to its head plus an explicit notice. - **Zero new egress surface.** Every fetch goes through the existing `ExternalUrlFetcher`, inheriting the two-tier egress policy, host allowlist, SSRF guard, and size/redirect/timeout caps verbatim. --- @@ -42,7 +44,7 @@ The feature ships as **one dedicated tool, `internal_web_fetch(url, save_path=No | **Returns** | The fetched text inline in the tool result | The saved workspace-relative path (+ a short text preview when textual) | | **File written?** | No | Yes — durable DIAL file at `save_path` under the agent home | | **Content types** | Textual only | Any (text + binary) | -| **Oversized text** | Rejected by the size guard — error telling the agent to pass a `save_path` | Saved; ranged reading is then `internal_file_read_lines` / `internal_file_search` | +| **Oversized text** | Truncated to the head + an explicit notice (total size, pass a `save_path` for the rest) | Saved in full; the agent processes it with whatever tools the app enables | | **Best for** | Reading a page/README/code file once | Large files, binary files, or content the agent will read/search repeatedly with the file tools | Both branches share the same front half — classify, egress-gate, fetch, decode (Component 3). They diverge only on the presence of `save_path`. @@ -99,11 +101,11 @@ Consequences: ### UC-3: Agent tries to load a large text file into context -**Trigger:** Agent calls `internal_web_fetch(url="https://…/huge.log")` (no `save_path`) and the decoded text exceeds the size guard. +**Trigger:** Agent calls `internal_web_fetch(url="https://…/huge.log")` (no `save_path`) and the decoded text reaches the inline cap. -**Behavior:** The tool does **not** truncate-and-guess. It returns a parameter error explaining the content is too large to load inline and instructing the agent to re-call with a `save_path` (then read ranges via the file tools). +**Behavior:** The tool returns an **explicit leading notice** — the total size, the content type, and that re-calling with a `save_path` persists the full content for whatever tools are available — followed by the **head of the content, truncated at a line boundary**. The notice + head together stay strictly below the cap, and the stage renders the notice outside the verbatim content block (Component 2). This is not truncate-and-guess: the notice makes the partiality explicit and impossible to miss. -**Outcome:** The agent gets a clear, actionable next step; the context window is protected. +**Outcome:** The first call is never wasted — for head-answerable questions (summarize, identify, check) the agent is done in one call; otherwise it has a clear next step, and the context window stays protected. Crucially this holds even when no other content-processing tool is enabled in the app. ### UC-4: Agent encounters non-text / binary content @@ -125,9 +127,9 @@ Consequences: **Trigger:** Agent calls `internal_web_fetch(url="files//doc.md")`. -**Behavior:** The URL is classified as DIAL (already in the workspace). The tool returns a parameter error pointing the agent at the existing file tools. +**Behavior:** The URL is classified as DIAL (already in the workspace). The tool returns a parameter error saying the resource is already in DIAL storage and needs no fetching — tool-neutrally, without naming the file tools (which may not be enabled). -**Outcome:** The fetch tool stays focused on *external* retrieval; in-workspace reads remain the job of `internal_file_read_lines` / `internal_file_search`. +**Outcome:** The fetch tool stays focused on *external* retrieval; in-workspace reads remain the job of whatever workspace tools the app enables. ### UC-7: App enables or disables the whole capability @@ -147,7 +149,7 @@ flowchart TD subgraph shared["shared fetch helper (Component 3)"] direction TB - B -->|DIAL| E1["parameter error:
use internal_file_read_lines / _search"] + B -->|DIAL| E1["parameter error:
already in DIAL storage (tool-neutral)"] B -->|unsupported| E2["unsupported-scheme error"] B -->|EXTERNAL| C["ExternalUrlFetcher.fetch(url)
egress + SSRF + size/redirect/timeout"] C -->|"ExternalFetchDisabledError / ExternalFetchError"| E3["parameter error
(existing messages)"] @@ -155,9 +157,11 @@ flowchart TD C -->|"FetchedBytes"| M{"save_path given?"} - M -->|no| L{"textual & within
size guard?"} - L -->|no| E4["parameter error:
too large / binary → re-call with a save_path"] - L -->|yes| LR["ToolCallResult: inline text
(no file written)"] + M -->|no| L{"textual?"} + L -->|no| E4["parameter error:
binary → re-call with a save_path"] + L -->|yes| LT{"within inline cap?"} + LT -->|yes| LR["ToolCallResult: full inline text
(no file written)"] + LT -->|no| LN["ToolCallResult: head + truncation notice
(under the cap, no file written)"] M -->|yes| P["HomePathResolver.resolve_appdata_url(save_path) +
DialFileService.write_bytes (unique on collision)"] P --> PR["ToolCallResult: saved relative path
(+ text preview if textual)"] @@ -170,22 +174,25 @@ flowchart TD - `url: str` (required) — the http(s) URL to fetch. - `save_path: str` (optional) — the workspace-relative destination for the fetched file (e.g. `data.py`, `docs/readme.md`). **Its presence is the switch:** omit it to read the content inline; provide it to persist the resource there and get back the saved path. Subdirectories are allowed; DIAL-style (`files/…`) paths are rejected (Component 4). - **Semantics:** run the shared helper to classify + fetch, then branch on whether `save_path` was given: - - **omitted** → require textual content (Component 3) within the size guard (Component 2). If either fails → parameter error telling the agent to re-call with a `save_path` (UC-3/UC-4). Otherwise return the decoded text inline (code-block wrapped, consistent with the file-tool stage formatting). + - **omitted** → require textual content (Component 3); binary → parameter error telling the agent to re-call with a `save_path` (UC-4). Textual content at or above the inline cap is truncated with a notice (Component 2, UC-3); otherwise the full decoded text is returned inline (code-block wrapped, consistent with the file-tool stage formatting). - **given** → persist any content type at `save_path` under the agent home (Component 4) and return the saved relative path (+ a short preview when textual). No size guard. - **Return shape:** without `save_path` → inline text in `ToolCallResult.content`. With `save_path` → the saved relative path (+ optional preview) in `ToolCallResult.content`. Never sets `result.attachments` (see Component 4, "No user-choice propagation"). -### Component 2: Inline size guard (no `save_path` only) +### Component 2: Inline size cap & truncation (no `save_path` only) -- **What:** a byte cap applied only on the inline (no-`save_path`) branch, **configurable** via `features.web_fetch.max_inline_size` (Component 5). +- **What:** a byte cap applied only on the inline (no-`save_path`) branch, **configurable** via `features.web_fetch.max_inline_size` (Component 5). Text at or above the cap is **truncated with an explicit notice**, never rejected. +- **Why truncate, not reject (third-revision reversal):** the original reject-and-save contract assumed the `internal_file_*` read-back tools exist to consume the saved file. They are an independently gated feature (`features.dial_files`, with per-tool `enabled_tools`), so `web_fetch` can legitimately be the *only* content tool in an app — and the reject flow then dead-ends. It was also wasteful even with file tools present (field-tested: a "summarize this URL" request cost two extra fetches and two dead-end searches): the rejection threw away the downloaded bytes and forced a save + ranged reads for questions the head alone answers. Truncation makes the first call always useful; the offload processor's own precedent (`_REQUIRED_READ_BACK_TOOLS` auto-disable in `dial_files_tooling_module.py`) confirms this dependency class must not be assumed. +- **Truncation shape:** a **leading notice** stating the total byte size, content type, and that re-calling with a `save_path` persists the full content — worded **tool-neutrally** ("use your available tools on it"), never naming `internal_file_*` — followed by a blank line and the UTF-8 head of the decoded text, cut at the last line boundary within budget (falling back to a hard cut when a single line dominates the budget). The notice comes *first* so the truncation is obvious no matter how large the head is, and it deliberately embeds only values known *before* the cut: the head's own byte count is omitted (the model can see how much text it got; the total is what drives the save-or-not decision), which keeps the budget math circularity-free. The format lives in `web_tooling/_truncation.py`, shared with the stage wrapper, which splits the notice back off and renders it **outside** the verbatim content block — the user sees the tool's instruction separated from the fetched text, and fetched text can never pose as a notice (only a notice at position zero, before any fetched byte, is split). - **Default:** drawn from the **same env setting that governs the offload threshold's default** (`ToolCallResultOffloadSettings`, 40 KB; `config/dial_files.py`), so out of the box the two are equal and anything the tool returns inline is below the offloader's trigger — the "Read in one call" and "Bounded inline output" goals hold without the global processor silently rewriting the result (see Component 4a for the cases where an operator decouples the two). -- **Comparison is `>=` (at-or-above rejects).** The guard compares the UTF-8 byte length of the decoded text and rejects when `size >= max_inline_size`, so a *returned* result is always **strictly** below the cap. The global offloader offloads when `size >= size_threshold` (`_offload_processor.py`: it skips only when `size < size_threshold`). So at the default (cap == threshold), a returned result — strictly `< cap == threshold` — is strictly below the offloader's `>= threshold` trigger, with no gap between the two operators. The invariant above is thus provable, not merely asserted. -- **On exceed:** parameter error, no silent truncation, no pagination. Re-calling with a `save_path` + `internal_file_read_lines` / `internal_file_search` are the path to "more than fits inline." +- **The invariant survives truncation (notice budgeting).** Because the notice embeds only up-front values, it is rendered once and the head budget is simply `max_inline_size` minus the notice's exact byte length, minus the separator, minus one — so notice + head together are always **strictly** below the cap. The global offloader offloads when `size >= size_threshold` (`_offload_processor.py`: it skips only when `size < size_threshold`), so at the default (cap == threshold) a returned result — full or truncated — never reaches the offloader's trigger. The invariant is thus provable, not merely asserted. (A pathological cap smaller than the notice itself yields the notice alone; `max_inline_size` is validated `gt=0` and realistic values are KBs.) +- **Comparison is `>=` (at-or-above truncates),** so exactly-at-cap content is truncated too and a returned result is always strictly below the cap. ### Component 3: Shared fetch helper (`WebContentFetcher`) - **What:** a small helper (`shared/external_fetch/web_content_fetcher.py`, already present on the branch) covering the common front half both branches share. Its entry point is `fetch_external(url, parameter_name="url")`, plus the static `is_textual(content_type)` and `decode(data, content_type)` helpers. - **Classification:** `classify_url` (`common/url_classification.py`) → `UrlScheme.{DIAL,EXTERNAL,UNSUPPORTED}`. `DIAL` → parameter error (UC-6); unsupported scheme → unsupported-scheme error. - **Fetch:** wraps `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `fetch_external` already catches `ExternalFetchDisabledError` / `ExternalFetchError` and re-raises them as `InvalidToolCallParameterException` on the given `parameter_name`, matching `FileLoaderService` / `DialFilePromoter`. +- **Request-scoped fetch cache (third revision):** successful fetches are cached by URL for the lifetime of the (request-scoped) instance, so the natural follow-up to a truncated inline read — re-calling the same URL with a `save_path` — does not re-download. Errors are not cached (they may be transient). - **Textual predicate (`is_textual`):** derived from `content_type` — `text/*`, `application/json`, `application/xml`, and common source/markup types are textual (decoded with the response charset, falling back to UTF-8 with replacement). Everything else is non-textual. Used by the inline branch to gate returning text and by the save branch to decide whether to attach a preview. - **No content-sniffing libraries** in phase-1. `FetchedBytes.filename` is still produced by `ExternalUrlFetcher` but is unused by this tool (saving always uses the caller's `save_path`). @@ -243,7 +250,7 @@ Deferred from phase-1; each is a clean follow-on, not a rework: - **Binary-attach enrichment of the inline branch.** A phase-2 candidate: when the fetched content is binary and the orchestrator deployment accepts its MIME (`orchestrator_accepts_mime_type`), return it as a `result.attachments` entry — `get_content`-style multimodal delivery — instead of the "re-call with a `save_path`" error (UC-4). Closes the binary-read gap for capable deployments without touching phase-1's text-only inline contract. - **Routing fetched content to downstream tools.** Passing an external resource to a deployment/REST/MCP tool is already covered by PR #279's outward url-attachment path (capability-aware, with promoter fallback); this tool deliberately adds only durability and workspace identity on top (see [Relationship to `internal_attachments_get_content`](#relationship-to-internal_attachments_get_content)). - **Special-casing the tool against the global offload processor.** The tool does not rely on offload (saving covers large content) and does not exempt itself from the global post-processor in phase-1; see Component 4a. -- **Pagination / `start_index` on inline reads.** Unnecessary — a save + `internal_file_read_lines` / `internal_file_search` provide ranged access; inline fetch is for content that fits inline. +- **Pagination / `start_index` on inline reads.** Considered again in the third revision as the alternative to truncate-with-notice and still deferred: truncation answers head-answerable questions in one call with zero new parameters, and a save covers full-content processing where the app enables tools for it. The accepted trade-off is that an app with `web_fetch` and *no* other content-processing tool cannot reach the tail of an oversized document — deep reads are what enabling a processing tool is for. If that trade-off proves wrong in the field, `start_index` (served from the request-scoped fetch cache, Component 3) is the natural extension. - **Content summarization** (Claude Code-style prompt-over-content). - **Surfacing the saved file to the user via `propagate_to_choice`** and richer binary metadata (thumbnails, structured metadata). - **Provenance-based URL allowlisting** (Anthropic-style: only fetch URLs that appeared in conversation context). A worthwhile future hardening; the existing egress policy + host allowlist already gate destinations today. @@ -273,6 +280,13 @@ features: `internal_web_fetch(url="https://raw.githubusercontent.com/org/repo/main/README.md")` → returns the README text inline (no `save_path`). No file written. +### Walkthrough — oversized text truncated (UC-3) + +`internal_web_fetch(url="https://…/huge.log")` where the log is 425 KB +→ returns +`[Truncated: fetched content is 424767 bytes (text/plain), larger than the inline cap; only the beginning follows. To process the full content, re-call with a save_path to persist it to the workspace, then use your available tools on it.]` +followed by a blank line and the head of the log (cut at a line boundary; notice + head under the 40 KB cap). The stage shows the notice above the verbatim content block. A follow-up `internal_web_fetch(url=…, save_path="huge.log")` is served from the request-scoped fetch cache — no second download. + ### Walkthrough — save to the workspace (UC-2) `internal_web_fetch(url="https://…/data.py", save_path="analysis/data.py")` @@ -306,6 +320,7 @@ None. The tool is purely additive and opt-in via app config. - `web_tooling/_web_fetch_tool.py` — the `internal_web_fetch` tool (plain `StagedBaseTool`; injects the shared fetch helper + shared home resolver; branches on `save_path`). - `web_tooling/_tool_configs.py` — the tool's `InternalTool` config (`WEB_FETCH_TOOL_CONFIG`), with `url` and optional `save_path` parameters. - `web_tooling/_web_fetch_stage_wrapper.py` — the tool's stage wrapper. +- `web_tooling/_truncation.py` — the truncation-notice format, shared by the tool (composes notice + head) and the stage wrapper (splits the notice back off for display outside the verbatim block). - `config/web_fetch.py` — `WebFetchConfig` (`enabled`, `max_inline_size` with `gt=0`). - `shared/external_fetch/web_content_fetcher.py` — the shared fetch helper (Component 3), exposing `fetch_external` / `is_textual` / `decode`. - Unit tests for the tool (both branches) and the fetch helper. @@ -332,13 +347,15 @@ The current branch already carries a superseded two-tool implementation. Bringin ### Tool exposed to the LLM -- `internal_web_fetch(url, save_path=None)` — fetch an external resource; without `save_path` it returns text inline (text only; binary/oversize → directs the agent to pass a `save_path`); with `save_path` it persists the resource there under the agent home and returns the relative path (+ preview when textual). +- `internal_web_fetch(url, save_path=None)` — fetch an external resource; without `save_path` it returns text inline (text only; binary → directs the agent to pass a `save_path`; oversized text → head + truncation notice); with `save_path` it persists the resource there under the agent home and returns the relative path (+ preview when textual). ### Tests -- **inline:** textual within guard → full inline content, no file written. -- **inline:** textual over size guard → parameter error telling the agent to pass a `save_path`. +- **inline:** textual within the cap → full inline content, no file written. +- **inline:** textual at/over the cap → leading truncation notice + head (line-boundary cut, accurate total byte count, `save_path` guidance, no `internal_file_*` name-drops), whole result strictly below the cap, no file written. +- **stage rendering:** truncation notice split off and rendered outside the verbatim content block; non-truncated content untouched; fetched text merely resembling a notice (no composed separator) is not split. - **inline:** binary → parameter error telling the agent to pass a `save_path`. +- **fetch helper:** repeat fetch of the same URL within a request → served from the cache (one download); fetch errors are not cached. - **save:** textual → persisted at `save_path` under agent home, relative path + preview returned. - **save:** binary → persisted at `save_path`, relative path + content type + size returned, no inline body. - **save:** `save_path` with a subdirectory → persisted at that workspace-relative path. diff --git a/docs/generated-app-schema.json b/docs/generated-app-schema.json index 0badbe99..c50ce714 100644 --- a/docs/generated-app-schema.json +++ b/docs/generated-app-schema.json @@ -3260,7 +3260,7 @@ "type": "object" }, "WebFetchConfig": { - "description": "Per-app config for the built-in ``internal_web_fetch`` tool.\n\nToggled by the explicit ``enabled`` flag. ``max_inline_size`` caps how much\ndecoded text the tool returns inline; its default is drawn from the same env\nsetting that governs the offload threshold's default\n(``TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD``), so out of the box the inline\ncap and the global offload threshold are equal — anything returned inline is\nbelow the offloader's trigger. See docs/designs/web_fetch_tool.md (Component\n4a) for the cases where an operator intentionally decouples the two.", + "description": "Per-app config for the built-in ``internal_web_fetch`` tool.\n\nToggled by the explicit ``enabled`` flag. ``max_inline_size`` caps how much\ndecoded text the tool returns inline; larger textual content is truncated to\nfit under the cap (head + notice). The cap's default is drawn from the same\nenv setting that governs the offload threshold's default\n(``TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD``), so out of the box the inline\ncap and the global offload threshold are equal — anything returned inline is\nbelow the offloader's trigger. See docs/designs/web_fetch_tool.md (Component\n4a) for the cases where an operator intentionally decouples the two.", "properties": { "enabled": { "default": false, @@ -3269,7 +3269,7 @@ "type": "boolean" }, "max_inline_size": { - "description": "Byte cap on the decoded text internal_web_fetch returns inline. Larger (or binary) resources are rejected with guidance to re-call with a save_path. Defaults to the TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD env var so the inline cap matches the global offload threshold by default.", + "description": "Byte cap on the decoded text internal_web_fetch returns inline. Larger textual resources are truncated to fit under the cap, with a notice stating the total size and pointing at save_path; binary resources are rejected with guidance to re-call with a save_path. Defaults to the TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD env var so the inline cap matches the global offload threshold by default.", "exclusiveMinimum": 0, "title": "Max Inline Size", "type": "integer" diff --git a/docs/generated-internal-tools.json b/docs/generated-internal-tools.json index f2eb6d14..1d663df5 100644 --- a/docs/generated-internal-tools.json +++ b/docs/generated-internal-tools.json @@ -354,7 +354,7 @@ }, { "name": "internal_web_fetch", - "description": "Fetch a resource from an external http(s) URL (e.g. a README, a source file, a documentation page). Without save_path it returns the text inline in a single call — text only: binary content (images, PDFs, archives) or a resource too large to fit inline is rejected, re-call with a save_path instead. With save_path it persists the resource (any content type) at that workspace-relative path under the agent home and returns the saved path (+ a short preview for text), so you can then read it with internal_file_read_lines / internal_file_search. DIAL file paths (files/...) are not fetched here; read them with the file tools.", + "description": "Fetch a resource from an external http(s) URL (e.g. a README, a source file, a documentation page). Without save_path it returns the text inline in a single call — text only: binary content (images, PDFs, archives) is rejected, re-call with a save_path instead. Text larger than the inline cap is returned truncated to its head with a notice stating the total size; the head is often enough (e.g. to summarize), otherwise re-call with a save_path. With save_path it persists the resource (any content type) at that workspace-relative path under the agent home and returns the saved path (+ a short preview for text), so other available tools can process the full content. Note: URLs of HTML viewer pages (e.g. github.com /blob/ pages) return the whole HTML page; prefer the raw/plain endpoint when one exists. DIAL file paths (files/...) are not fetched here.", "properties": { "url": { "type": "string", diff --git a/src/quickapp/config/web_fetch.py b/src/quickapp/config/web_fetch.py index eefe9d38..ea4b5a52 100644 --- a/src/quickapp/config/web_fetch.py +++ b/src/quickapp/config/web_fetch.py @@ -7,8 +7,9 @@ class WebFetchConfig(BaseModel): """Per-app config for the built-in ``internal_web_fetch`` tool. Toggled by the explicit ``enabled`` flag. ``max_inline_size`` caps how much - decoded text the tool returns inline; its default is drawn from the same env - setting that governs the offload threshold's default + decoded text the tool returns inline; larger textual content is truncated to + fit under the cap (head + notice). The cap's default is drawn from the same + env setting that governs the offload threshold's default (``TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD``), so out of the box the inline cap and the global offload threshold are equal — anything returned inline is below the offloader's trigger. See docs/designs/web_fetch_tool.md (Component @@ -24,9 +25,10 @@ class WebFetchConfig(BaseModel): gt=0, description=( "Byte cap on the decoded text internal_web_fetch returns inline. " - "Larger (or binary) resources are rejected with guidance to re-call " - "with a save_path. Defaults to the " - "TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD env var so the inline cap " - "matches the global offload threshold by default." + "Larger textual resources are truncated to fit under the cap, with a " + "notice stating the total size and pointing at save_path; binary " + "resources are rejected with guidance to re-call with a save_path. " + "Defaults to the TOOL_CALL_RESULT_OFFLOAD__SIZE_THRESHOLD env var so " + "the inline cap matches the global offload threshold by default." ), ) diff --git a/src/quickapp/shared/external_fetch/web_content_fetcher.py b/src/quickapp/shared/external_fetch/web_content_fetcher.py index fdea6ee9..b824a128 100644 --- a/src/quickapp/shared/external_fetch/web_content_fetcher.py +++ b/src/quickapp/shared/external_fetch/web_content_fetcher.py @@ -77,34 +77,47 @@ def __init__( ) -> None: self.__external_fetcher = external_fetcher self.__dial_url = dial_settings.url + # Fetch cache, request-scoped because this class is bound request-scoped: + # a re-call for the same URL within one request — e.g. a save_path call + # after a truncated inline read — must not re-download. + self.__fetched_by_url: dict[str, FetchedBytes] = {} async def fetch_external(self, url: str, parameter_name: str = "url") -> FetchedBytes: """Classify ``url`` and fetch it, or raise a tool-facing parameter error. - * DIAL URL -> points the agent at the in-workspace file tools (the fetch - tools are for *external* retrieval only). + * DIAL URL -> rejected: the resource is already in DIAL storage and the + fetch tools are for *external* retrieval only. The message stays + tool-neutral — which workspace tools exist varies per app. * Unsupported scheme -> the canonical unsupported-scheme error. * External -> fetched through :class:`ExternalUrlFetcher`; its ``ExternalFetchError`` / ``ExternalFetchDisabledError`` (egress disabled, host not allowed, SSRF, size, timeout, transport) are re-raised as ``InvalidToolCallParameterException`` carrying the - existing operator/builder messages. + existing operator/builder messages. Successful fetches are cached for + the lifetime of this (request-scoped) instance, keyed by URL. """ scheme = classify_url(url, self.__dial_url) if scheme == UrlScheme.DIAL: raise InvalidToolCallParameterException( parameter_name, - f"URL {url} already points to a file in DIAL storage. " - "Use internal_file_read_lines or internal_file_search to read it.", + f"URL {url} already points to a file in DIAL storage and does not " + "need fetching. Access it with your available workspace tools.", ) if scheme == UrlScheme.UNSUPPORTED: raise unsupported_scheme_error(url, parameter_name) + cached = self.__fetched_by_url.get(url) + if cached is not None: + return cached + try: - return await self.__external_fetcher.fetch(url) + fetched = await self.__external_fetcher.fetch(url) except (ExternalFetchError, ExternalFetchDisabledError) as exc: raise InvalidToolCallParameterException(parameter_name, str(exc)) from exc + self.__fetched_by_url[url] = fetched + return fetched + @staticmethod def is_textual(content_type: str | None) -> bool: """Whether a payload with this ``content_type`` can be decoded as text. diff --git a/src/quickapp/web_tooling/_tool_configs.py b/src/quickapp/web_tooling/_tool_configs.py index e1bc2890..d6692c87 100644 --- a/src/quickapp/web_tooling/_tool_configs.py +++ b/src/quickapp/web_tooling/_tool_configs.py @@ -17,13 +17,16 @@ "Fetch a resource from an external http(s) URL (e.g. a README, a " "source file, a documentation page). Without save_path it returns the " "text inline in a single call — text only: binary content (images, " - "PDFs, archives) or a resource too large to fit inline is rejected, " - "re-call with a save_path instead. With save_path it persists the " - "resource (any content type) at that workspace-relative path under the " - "agent home and returns the saved path (+ a short preview for text), so " - "you can then read it with internal_file_read_lines / " - "internal_file_search. DIAL file paths (files/...) are not fetched " - "here; read them with the file tools." + "PDFs, archives) is rejected, re-call with a save_path instead. Text " + "larger than the inline cap is returned truncated to its head with a " + "notice stating the total size; the head is often enough (e.g. to " + "summarize), otherwise re-call with a save_path. With save_path it " + "persists the resource (any content type) at that workspace-relative " + "path under the agent home and returns the saved path (+ a short " + "preview for text), so other available tools can process the full " + "content. Note: URLs of HTML viewer pages (e.g. github.com /blob/ " + "pages) return the whole HTML page; prefer the raw/plain endpoint " + "when one exists. DIAL file paths (files/...) are not fetched here." ), parameters=OpenAiToolFunctionParameters( type=JsonTypeEnum.object, diff --git a/src/quickapp/web_tooling/_truncation.py b/src/quickapp/web_tooling/_truncation.py new file mode 100644 index 00000000..34d0d728 --- /dev/null +++ b/src/quickapp/web_tooling/_truncation.py @@ -0,0 +1,45 @@ +"""Truncation notice for oversized inline web fetches. + +Shared by the tool (which composes ``notice + blank line + fetched head``) and +the stage wrapper (which splits the notice back off to render it outside the +verbatim content block). Keeping the format here is what lets the wrapper +separate the tool's instructions from the fetched text without guessing. +""" + +# Deliberately tool-neutral: which tools can process a saved file varies per +# app (file tools, RAG, get_content, ...), and this module must not assume any +# of them is enabled. Embeds only values known before the head is cut (total +# size, content type) so the notice can be rendered once, up front, and its +# exact length reserved from the head budget. +TRUNCATION_NOTICE_TEMPLATE = ( + "[Truncated: fetched content is {total} bytes ({content_type}), larger " + "than the inline cap; only the beginning follows. To process the full " + "content, re-call with a save_path to persist it to the workspace, then " + "use your available tools on it.]" +) + +_NOTICE_PREFIX = "[Truncated: " +_SEPARATOR = "\n\n" + + +def compose_truncated_content(notice: str, head: str) -> str: + return f"{notice}{_SEPARATOR}{head}" + + +def separator_byte_length() -> int: + return len(_SEPARATOR.encode("utf-8")) + + +def split_truncation_notice(content: str) -> tuple[str | None, str]: + """Split ``content`` into ``(notice, fetched text)``. + + Returns ``(None, content)`` unchanged for non-truncated results. Only a + notice composed by :func:`compose_truncated_content` is split off — it sits + at position zero, before any fetched byte, so fetched text can never spoof + its way out of the verbatim block. + """ + if content.startswith(_NOTICE_PREFIX): + notice, sep, body = content.partition(_SEPARATOR) + if sep: + return notice, body + return None, content diff --git a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py index 45aff9b4..83388174 100644 --- a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py +++ b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py @@ -4,6 +4,7 @@ from quickapp.common import TimedStageWrapper, ToolCallResult from quickapp.common.utils import fenced_code_block +from quickapp.web_tooling._truncation import split_truncation_notice @inject @@ -16,7 +17,14 @@ def _build_debug_info_from_exception(self, exception: Exception) -> str: return f"### Exception:\n\r{exception}\n\r" def _build_debug_info_from_result(self, result: ToolCallResult) -> str: + # A truncated read is composed as notice + blank line + fetched head; show + # the notice outside the verbatim block so it stays visible however large + # the head is and is never mistaken for fetched text. + notice, body = split_truncation_notice(result.content) # Fetched content is often itself Markdown/HTML (e.g. a README). Render it # inside a fenced code block so the stage shows the fetched text verbatim # instead of the UI rendering it as formatted Markdown. - return f"**Content:**\n{fenced_code_block(result.content)}\n" + content_block = f"**Content:**\n{fenced_code_block(body)}\n" + if notice is None: + return content_block + return f"**{notice}**\n\n{content_block}" diff --git a/src/quickapp/web_tooling/_web_fetch_tool.py b/src/quickapp/web_tooling/_web_fetch_tool.py index 0f0c77b3..7de9182e 100644 --- a/src/quickapp/web_tooling/_web_fetch_tool.py +++ b/src/quickapp/web_tooling/_web_fetch_tool.py @@ -14,6 +14,11 @@ from quickapp.dial_core_services.dial_file_service import DialFileService from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher from quickapp.shared.home_path.home_path_resolver import HomePathResolver +from quickapp.web_tooling._truncation import ( + TRUNCATION_NOTICE_TEMPLATE, + compose_truncated_content, + separator_byte_length, +) from quickapp.web_tooling._web_fetch_stage_wrapper import _WebFetchStageWrapper # Preview shown for textual saves so the agent can decide whether to read the @@ -30,12 +35,15 @@ class _WebFetchTool(StagedBaseTool): A single optional ``save_path`` selects the behaviour: - * **omitted** — the payload must be textual and within ``max_inline_size``; - the decoded text is returned inline. Binary or oversized content is rejected - with guidance to re-call with a ``save_path`` — the tool never truncates and - never offloads. The inline cap defaults to the global offload threshold, so a - returned result is always below the offloader's trigger - (see docs/designs/web_fetch_tool.md, Component 4a). + * **omitted** — the payload must be textual; the decoded text is returned + inline. Content at or above ``max_inline_size`` is truncated: a leading + notice (total size + how to get the rest) followed by the content head, + sized so the whole result stays strictly below the cap — and, at the + default where the cap equals the global offload threshold, below the + offloader's trigger (see docs/designs/web_fetch_tool.md, Component 4a). + Binary content is rejected with guidance to re-call with a ``save_path``. + Guidance never names other tools — which tools can process a saved file + varies per app. * **given** — the fetched bytes (any content type) are persisted at ``save_path`` under the agent home via the shared home-path resolver and a bytes write, and the saved workspace-relative path (+ a short preview for @@ -97,29 +105,45 @@ def __load_into_context(self, url: str, fetched: Any) -> ToolCallResult: f"URL {url} returned non-text content " f"(content-type: {fetched.content_type or 'unknown'}), which cannot be " "loaded into context. Re-call with a save_path to persist it to the " - "workspace instead.", + "workspace, then use your available tools on it.", ) text = WebContentFetcher.decode(fetched.data, fetched.content_type) - - # Guard against dumping unbounded content into the LLM context. Reject at or - # above the cap so the returned content stays strictly below it (and, at the - # default where the cap equals the offload threshold, below the offloader's - # trigger too). - content_size = len(text.encode("utf-8")) - if content_size >= self.__max_inline_size: - raise InvalidToolCallParameterException( - "url", - f"URL {url} content is too large to load inline " - f"({content_size} bytes >= {self.__max_inline_size} byte limit). " - "Re-call with a save_path to persist it to the workspace, then read " - "ranges with internal_file_read_lines / internal_file_search.", - ) - - return ToolCallResult( - content=text, - content_type=fetched.content_type or "text/plain", - ) + content_type = fetched.content_type or "text/plain" + + # Guard against dumping unbounded content into the LLM context: at or above + # the cap the head is returned with an explicit truncation notice, never an + # error — a truncated read must stay useful even when no other tool that + # could process a saved copy is enabled. + encoded = text.encode("utf-8") + if len(encoded) >= self.__max_inline_size: + text = self.__truncate_with_notice(encoded, content_type) + + return ToolCallResult(content=text, content_type=content_type) + + def __truncate_with_notice(self, encoded: bytes, content_type: str) -> str: + """Return the truncation notice followed by the head of ``encoded``. + + The notice comes *first* so it stays visible however large the head is, + both to the model and — split back off by the stage wrapper — to the + user. It embeds only values known up front, so the head budget is simply + the cap minus the notice's exact length (plus separator), keeping notice + + content strictly below ``max_inline_size`` — an inline result never + reaches the global offload trigger (cap == threshold by default). The + cut prefers the last newline within budget (unless that drops more than + half of it) so the head ends on a whole line. A cap smaller than the + notice itself yields the notice alone. + """ + notice = TRUNCATION_NOTICE_TEMPLATE.format(total=len(encoded), content_type=content_type) + reserved = len(notice.encode("utf-8")) + separator_byte_length() + 1 + budget = max(self.__max_inline_size - reserved, 0) + head = encoded[:budget] + newline_cut = head.rfind(b"\n") + if newline_cut >= budget // 2: + head = head[:newline_cut] + # "ignore" drops only a UTF-8 sequence split by the byte cut; the rest of + # the head is valid by construction. + return compose_truncated_content(notice, head.decode("utf-8", errors="ignore")) async def __save_to_workspace(self, save_path: str, fetched: Any) -> ToolCallResult: # A ``files/``-prefixed path would be returned verbatim by diff --git a/src/tests/unit_tests/shared/test_web_content_fetcher.py b/src/tests/unit_tests/shared/test_web_content_fetcher.py index 92fbfaff..35c62ee4 100644 --- a/src/tests/unit_tests/shared/test_web_content_fetcher.py +++ b/src/tests/unit_tests/shared/test_web_content_fetcher.py @@ -38,7 +38,37 @@ async def test_dial_relative_path_rejected(self): with pytest.raises(InvalidToolCallParameterException) as exc: await fetcher.fetch_external("files/bucket/doc.md") assert exc.value.parameter_name == "url" - assert "internal_file_read_lines" in exc.value.message + assert "DIAL storage" in exc.value.message + # Guidance must stay tool-neutral: no other tool availability is assumed. + assert "internal_file" not in exc.value.message + + @pytest.mark.asyncio + async def test_repeat_fetch_served_from_request_cache(self): + fetched = FetchedBytes(data=b"hello", content_type="text/plain", filename="a.txt") + external = MagicMock() + external.fetch = AsyncMock(return_value=fetched) + dial_settings = MagicMock() + dial_settings.url = _DIAL_URL + fetcher = WebContentFetcher(external_fetcher=external, dial_settings=dial_settings) + first = await fetcher.fetch_external("https://example.com/a.txt") + second = await fetcher.fetch_external("https://example.com/a.txt") + assert second is first + external.fetch.assert_awaited_once() + + @pytest.mark.asyncio + async def test_fetch_errors_not_cached(self): + external = MagicMock() + external.fetch = AsyncMock( + side_effect=ExternalFetchError(reason="size_limit", url="https://x.com") + ) + dial_settings = MagicMock() + dial_settings.url = _DIAL_URL + fetcher = WebContentFetcher(external_fetcher=external, dial_settings=dial_settings) + with pytest.raises(InvalidToolCallParameterException): + await fetcher.fetch_external("https://x.com") + with pytest.raises(InvalidToolCallParameterException): + await fetcher.fetch_external("https://x.com") + assert external.fetch.await_count == 2 @pytest.mark.asyncio async def test_dial_host_url_rejected(self): diff --git a/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py b/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py index 45b20112..05936f59 100644 --- a/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py +++ b/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py @@ -3,12 +3,15 @@ import pytest from aidial_client._exception import DialException, EtagMismatchError +from quickapp.common import ToolCallResult from quickapp.common.exceptions import InvalidToolCallParameterException from quickapp.dial_core_services.dial_file_service import DialFileService from quickapp.shared.external_fetch.external_url_fetcher import FetchedBytes from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher from quickapp.shared.home_path.home_path_resolver import HomePathResolver from quickapp.web_tooling._tool_configs import WEB_FETCH_TOOL_CONFIG +from quickapp.web_tooling._truncation import compose_truncated_content, split_truncation_notice +from quickapp.web_tooling._web_fetch_stage_wrapper import _WebFetchStageWrapper from quickapp.web_tooling._web_fetch_tool import _WebFetchTool _HOME = "files/appbucket/agent/" @@ -88,26 +91,51 @@ async def test_missing_content_type_treated_as_binary(self): assert "save_path" in exc.value.message @pytest.mark.asyncio - async def test_oversized_text_rejected_pointing_at_save_path(self): - big = "a" * 100 + async def test_oversized_text_truncated_with_leading_notice(self): + text = "\n".join(f"line-{i:03d}" for i in range(200)) + total = len(text.encode("utf-8")) fetched = FetchedBytes( - data=big.encode("utf-8"), content_type="text/plain", filename="big.txt" + data=text.encode("utf-8"), content_type="text/plain", filename="big.txt" ) - tool, _ = _make_tool(fetched, max_inline_size=50) - with pytest.raises(InvalidToolCallParameterException) as exc: - await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/big.txt") - assert exc.value.parameter_name == "url" - assert "save_path" in exc.value.message + tool, service = _make_tool(fetched, max_inline_size=500) + result = await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/big.txt") + # The notice leads so it stays visible however large the head is. + notice, head = split_truncation_notice(result.content) + assert notice is not None + assert result.content.startswith(notice) + assert text.startswith(head) + assert f"is {total} bytes" in notice + assert "save_path" in notice + # Guidance must stay tool-neutral: no other tool availability is assumed. + assert "internal_file" not in result.content + service.write_bytes.assert_not_awaited() + + @pytest.mark.asyncio + async def test_truncated_result_stays_below_cap_and_cuts_on_line_boundary(self): + # The whole result (notice + head) must stay strictly below the cap so it + # never reaches the offload trigger (which fires at >= threshold). + text = "\n".join(f"line-{i:03d}" for i in range(200)) + fetched = FetchedBytes( + data=text.encode("utf-8"), content_type="text/plain", filename="big.txt" + ) + tool, _ = _make_tool(fetched, max_inline_size=500) + result = await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/big.txt") + assert len(result.content.encode("utf-8")) < 500 + notice, head = split_truncation_notice(result.content) + assert notice is not None + assert head # a realistic cap leaves room for content, not just the notice + assert head.split("\n")[-1].startswith("line-") # complete last line @pytest.mark.asyncio - async def test_content_at_cap_rejected(self): - # Exactly the cap must be rejected so the returned content stays below the - # offload trigger (which fires at >= threshold). - data = b"a" * 50 + async def test_content_at_cap_truncated(self): + # Exactly the cap must truncate too, so a returned result is always + # strictly below the cap (and thus below the offload trigger). + data = b"a" * 500 fetched = FetchedBytes(data=data, content_type="text/plain", filename="x.txt") - tool, _ = _make_tool(fetched, max_inline_size=50) - with pytest.raises(InvalidToolCallParameterException): - await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/x.txt") + tool, _ = _make_tool(fetched, max_inline_size=500) + result = await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/x.txt") + assert result.content.startswith("[Truncated: ") + assert len(result.content.encode("utf-8")) < 500 @pytest.mark.asyncio async def test_fetch_error_propagates(self): @@ -186,3 +214,40 @@ async def test_permission_denied_raises_parameter_error(self): ) assert exc.value.parameter_name == "save_path" assert "access denied" in exc.value.message + + +class TestSplitTruncationNotice: + def test_composed_content_round_trips(self): + notice = "[Truncated: fetched content is 9 bytes (text/plain). x]" + content = compose_truncated_content(notice, "abc") + assert split_truncation_notice(content) == (notice, "abc") + + def test_non_truncated_content_returned_unchanged(self): + assert split_truncation_notice("# README\nbody") == (None, "# README\nbody") + + def test_fetched_text_resembling_a_notice_needs_the_separator(self): + # A page whose text merely starts like a notice but was not composed by + # the tool (no blank-line separator) is left intact. + content = "[Truncated: some page text without a separator" + assert split_truncation_notice(content) == (None, content) + + +class TestStageRendering: + def _render(self, content: str) -> str: + wrapper = _WebFetchStageWrapper(stage=MagicMock()) + result = ToolCallResult(content=content, content_type="text/plain") + return wrapper._build_debug_info_from_result(result) + + def test_truncation_notice_rendered_outside_verbatim_block(self): + notice = "[Truncated: fetched content is 9 bytes (text/plain). x]" + info = self._render(compose_truncated_content(notice, "abc")) + assert info.startswith(f"**{notice}**") + # Only the fetched head lands inside the verbatim content block. + _, content_block = info.split("**Content:**") + assert "abc" in content_block + assert "[Truncated" not in content_block + + def test_regular_content_rendered_without_notice(self): + info = self._render("# README\nbody") + assert info.startswith("**Content:**") + assert "[Truncated" not in info From e0d5b5d6983d84a4e2fc814ca79f6be87d1d4f7c Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Wed, 22 Jul 2026 19:21:06 +0300 Subject: [PATCH 10/12] fix(web-fetch): reject agent-home-relative paths as already in DIAL storage (#344) Now that get_content classifies bare paths (e.g. reports/img.png) as UrlScheme.DIAL_APPDIR_RELATIVE (#444), the web fetcher treats them like DIAL urls: rejected as already-in-storage rather than falling through to the generic unsupported-scheme error. --- .../shared/external_fetch/web_content_fetcher.py | 9 +++++---- src/tests/unit_tests/shared/test_web_content_fetcher.py | 8 ++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/quickapp/shared/external_fetch/web_content_fetcher.py b/src/quickapp/shared/external_fetch/web_content_fetcher.py index b824a128..ccfd3e10 100644 --- a/src/quickapp/shared/external_fetch/web_content_fetcher.py +++ b/src/quickapp/shared/external_fetch/web_content_fetcher.py @@ -85,9 +85,10 @@ def __init__( async def fetch_external(self, url: str, parameter_name: str = "url") -> FetchedBytes: """Classify ``url`` and fetch it, or raise a tool-facing parameter error. - * DIAL URL -> rejected: the resource is already in DIAL storage and the - fetch tools are for *external* retrieval only. The message stays - tool-neutral — which workspace tools exist varies per app. + * DIAL URL or agent-home-relative path -> rejected: the resource is + already in DIAL storage and the fetch tools are for *external* + retrieval only. The message stays tool-neutral — which workspace + tools exist varies per app. * Unsupported scheme -> the canonical unsupported-scheme error. * External -> fetched through :class:`ExternalUrlFetcher`; its ``ExternalFetchError`` / ``ExternalFetchDisabledError`` (egress @@ -97,7 +98,7 @@ async def fetch_external(self, url: str, parameter_name: str = "url") -> Fetched the lifetime of this (request-scoped) instance, keyed by URL. """ scheme = classify_url(url, self.__dial_url) - if scheme == UrlScheme.DIAL: + if scheme in (UrlScheme.DIAL, UrlScheme.DIAL_APPDIR_RELATIVE): raise InvalidToolCallParameterException( parameter_name, f"URL {url} already points to a file in DIAL storage and does not " diff --git a/src/tests/unit_tests/shared/test_web_content_fetcher.py b/src/tests/unit_tests/shared/test_web_content_fetcher.py index 35c62ee4..0038865e 100644 --- a/src/tests/unit_tests/shared/test_web_content_fetcher.py +++ b/src/tests/unit_tests/shared/test_web_content_fetcher.py @@ -42,6 +42,14 @@ async def test_dial_relative_path_rejected(self): # Guidance must stay tool-neutral: no other tool availability is assumed. assert "internal_file" not in exc.value.message + @pytest.mark.asyncio + async def test_home_relative_path_rejected_as_already_in_storage(self): + fetcher = _make_fetcher() + with pytest.raises(InvalidToolCallParameterException) as exc: + await fetcher.fetch_external("reports/img.png") + assert exc.value.parameter_name == "url" + assert "DIAL storage" in exc.value.message + @pytest.mark.asyncio async def test_repeat_fetch_served_from_request_cache(self): fetched = FetchedBytes(data=b"hello", content_type="text/plain", filename="a.txt") From 2b39f1db961c621da9d8c98d727589ea2686899a Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Wed, 22 Jul 2026 19:42:24 +0300 Subject: [PATCH 11/12] refactor(web-fetch): drop unused logger, annotate fetched as FetchedBytes Address code-review nits on PR #399: remove the unused logger and its logging import from WebContentFetcher, and type the tool's fetched params as FetchedBytes instead of Any. --- src/quickapp/shared/external_fetch/web_content_fetcher.py | 3 --- src/quickapp/web_tooling/_web_fetch_tool.py | 5 +++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/quickapp/shared/external_fetch/web_content_fetcher.py b/src/quickapp/shared/external_fetch/web_content_fetcher.py index ccfd3e10..3fa062b4 100644 --- a/src/quickapp/shared/external_fetch/web_content_fetcher.py +++ b/src/quickapp/shared/external_fetch/web_content_fetcher.py @@ -8,7 +8,6 @@ returned :class:`FetchedBytes`. """ -import logging from email.message import Message from injector import inject @@ -23,8 +22,6 @@ FetchedBytes, ) -logger = logging.getLogger(__name__) - # Non-``text/*`` MIME types that are nevertheless textual and safe to decode / # inline. Kept small and explicit in phase-1 (no content-sniffing library). _TEXTUAL_APPLICATION_TYPES = frozenset( diff --git a/src/quickapp/web_tooling/_web_fetch_tool.py b/src/quickapp/web_tooling/_web_fetch_tool.py index 7de9182e..b7382cdf 100644 --- a/src/quickapp/web_tooling/_web_fetch_tool.py +++ b/src/quickapp/web_tooling/_web_fetch_tool.py @@ -12,6 +12,7 @@ from quickapp.config.application import StageDisplayLevel from quickapp.config.tools.internal import InternalTool from quickapp.dial_core_services.dial_file_service import DialFileService +from quickapp.shared.external_fetch.external_url_fetcher import FetchedBytes from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher from quickapp.shared.home_path.home_path_resolver import HomePathResolver from quickapp.web_tooling._truncation import ( @@ -98,7 +99,7 @@ async def _run_in_stage_async( stage_wrapper.add_result(result) return result - def __load_into_context(self, url: str, fetched: Any) -> ToolCallResult: + def __load_into_context(self, url: str, fetched: FetchedBytes) -> ToolCallResult: if not WebContentFetcher.is_textual(fetched.content_type): raise InvalidToolCallParameterException( "url", @@ -145,7 +146,7 @@ def __truncate_with_notice(self, encoded: bytes, content_type: str) -> str: # the head is valid by construction. return compose_truncated_content(notice, head.decode("utf-8", errors="ignore")) - async def __save_to_workspace(self, save_path: str, fetched: Any) -> ToolCallResult: + async def __save_to_workspace(self, save_path: str, fetched: FetchedBytes) -> ToolCallResult: # A ``files/``-prefixed path would be returned verbatim by # ``resolve_appdata_url`` (bypassing home-prefixing and traversal validation), # escaping the agent home. Require a home-relative destination. From e8818a22f83311de3c68600566976f90766e908b Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Thu, 23 Jul 2026 13:44:31 +0300 Subject: [PATCH 12/12] refactor(web-fetch): address review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - WebContentFetcher: slim to fetch+classify; raise domain WebContentFetchError (and let ExternalFetch*Error propagate) instead of tool exceptions — the shared helper no longer knows about tools. - Text detection: drop the content-type allowlist + charset parsing; decide text vs. binary by attempting a UTF-8 decode in the tool. - Exception taxonomy: a fetch failure on a valid URL (egress denied, timeout, size, non-text body) is now a WebFetchToolErrorException forwarded to the model via fallback_configuration; only a wrong kind of URL (DIAL/unsupported scheme) stays an InvalidToolCallParameterException. - Truncation: drop the newline-boundary logic, keep the strictly-below-cap budget. - Trim the tool description and verbose comments/docstrings; remove a stray binding comment. - Update tests and the design doc; revert doc Status to Approved (not merged). --- docs/designs/web_fetch_tool.md | 50 +++--- docs/generated-internal-tools.json | 2 +- .../dial_files_tooling_module.py | 1 - .../external_fetch/web_content_fetcher.py | 147 ++++-------------- src/quickapp/web_tooling/_tool_configs.py | 20 ++- src/quickapp/web_tooling/_truncation.py | 21 +-- .../web_tooling/_web_fetch_stage_wrapper.py | 9 +- src/quickapp/web_tooling/_web_fetch_tool.py | 139 ++++++++--------- .../_web_fetch_tool_error_exception.py | 12 ++ .../shared/test_web_content_fetcher.py | 130 ++++------------ .../web_tooling/test_web_fetch_tool.py | 64 +++++--- 11 files changed, 232 insertions(+), 363 deletions(-) create mode 100644 src/quickapp/web_tooling/_web_fetch_tool_error_exception.py diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md index 0e83888a..95c1863b 100644 --- a/docs/designs/web_fetch_tool.md +++ b/docs/designs/web_fetch_tool.md @@ -13,6 +13,8 @@ A subsequent revision re-examined the design against `internal_attachments_get_c A third revision, driven by field testing, changed the oversized-inline contract from **reject** to **truncate with an explicit notice**, and made every piece of agent-facing guidance **tool-neutral** (no `internal_file_*` name-drops). The prior contract silently assumed the file tools are enabled alongside this one — they are independently gated features, so an app can legitimately run `web_fetch` without them, and the reject-and-save flow then dead-ends (the agent saves a file no enabled tool can read). The rationale and the revised contract are in [Component 2](#component-2-inline-size-cap--truncation-no-save_path-only); a request-scoped fetch cache was added alongside (Component 3) so the save re-call after a truncated read does not re-download. +A fourth revision, from PR review, made three refinements. **(a) Text detection is a UTF-8 decode attempt**, not a content-type allowlist: the tool decodes the body and treats a `UnicodeDecodeError` as binary — `WebContentFetcher` is slimmed to fetch + scheme-classify and no longer owns `is_textual`/`decode`. **(b) The shared fetcher raises only domain errors** (`WebContentFetchError`, or the egress `ExternalFetchError` / `ExternalFetchDisabledError`), leaving the tool to choose the agent-facing exception — the shared helper knows nothing about tools. **(c) A fetch outcome on a *valid* URL is a tool error, not a parameter error**: an egress failure (disabled, host, SSRF, size, timeout, transport) or a non-text body raises `WebFetchToolErrorException` (a `ToolErrorException`), forwarded to the model via the tool's `fallback_configuration`; only a wrong *kind* of URL (DIAL path, unsupported scheme) is an `InvalidToolCallParameterException`. The oversized-inline head is also simplified to a plain byte cut (no line-boundary trim), preserving only the strictly-below-cap budget. + ## Problem Statement The agent today has two halves of a capability but not the bridge between them: @@ -103,7 +105,7 @@ Consequences: **Trigger:** Agent calls `internal_web_fetch(url="https://…/huge.log")` (no `save_path`) and the decoded text reaches the inline cap. -**Behavior:** The tool returns an **explicit leading notice** — the total size, the content type, and that re-calling with a `save_path` persists the full content for whatever tools are available — followed by the **head of the content, truncated at a line boundary**. The notice + head together stay strictly below the cap, and the stage renders the notice outside the verbatim content block (Component 2). This is not truncate-and-guess: the notice makes the partiality explicit and impossible to miss. +**Behavior:** The tool returns an **explicit leading notice** — the total size, the content type, and that re-calling with a `save_path` persists the full content for whatever tools are available — followed by the **head of the content** (a plain byte cut). The notice + head together stay strictly below the cap, and the stage renders the notice outside the verbatim content block (Component 2). This is not truncate-and-guess: the notice makes the partiality explicit and impossible to miss. **Outcome:** The first call is never wasted — for head-answerable questions (summarize, identify, check) the agent is done in one call; otherwise it has a clear next step, and the context window stays protected. Crucially this holds even when no other content-processing tool is enabled in the app. @@ -111,7 +113,7 @@ Consequences: **Trigger:** Agent calls `internal_web_fetch` on a URL whose content type is non-textual (image, zip, PDF, …). -**Behavior:** Without `save_path` → parameter error: binary content cannot be loaded into context; re-call with a `save_path`. With `save_path` → the bytes are saved and the tool returns the path + content type + size (no inline body, no extraction in phase-1). +**Behavior:** Without `save_path` → the body fails to decode as UTF-8, so it is treated as binary and the tool returns a **tool error** (forwarded to the model via the fallback config): binary content cannot be loaded into context; re-call with a `save_path`. With `save_path` → the bytes are saved and the tool returns the path + content type + size (no inline body, no extraction in phase-1). **Outcome:** The model never receives garbled bytes in context; binary is reachable only as a saved file it can forward to a capable deployment/tool. (When the url is a conversation attachment and `internal_attachments_get_content` is registered, that tool may deliver the binary to the orchestrator as multimodal input instead — see [Relationship to `internal_attachments_get_content`](#relationship-to-internal_attachments_get_content).) @@ -119,7 +121,7 @@ Consequences: **Trigger:** An app sets `features.web_fetch.enabled=true` while external fetching is disabled — `EXTERNAL_URL_FETCH_ENABLED=false` (admin cap) or `features.external_url_fetch.enabled=false` (per-app opt-out). -**Behavior:** This is a contradictory configuration — the tool cannot function without egress — so it is caught at **initialization**, not per call. `WebToolingModule` gates tool provisioning on the egress policy (`ExternalUrlFetchPolicyResolver.is_enabled()`): the tool is **not exposed**, and a hard `ToolInitializationException` is surfaced in the "Initialization issues" stage explaining that `web_fetch` needs external fetch enabled. (A host-allowlist denial — egress on, but the *specific host* not allowed — is per-URL and cannot be known at init; it remains a runtime parameter error via `ExternalUrlFetcher.fetch`, see Component 3.) +**Behavior:** This is a contradictory configuration — the tool cannot function without egress — so it is caught at **initialization**, not per call. `WebToolingModule` gates tool provisioning on the egress policy (`ExternalUrlFetchPolicyResolver.is_enabled()`): the tool is **not exposed**, and a hard `ToolInitializationException` is surfaced in the "Initialization issues" stage explaining that `web_fetch` needs external fetch enabled. (A host-allowlist denial — egress on, but the *specific host* not allowed — is per-URL and cannot be known at init; it remains a runtime tool error via `ExternalUrlFetcher.fetch`, see Component 3.) **Outcome:** The builder learns their config is contradictory up front; the model is never offered a tool that would fail on every call. No silent misconfiguration. @@ -147,18 +149,18 @@ Consequences: flowchart TD WF["internal_web_fetch(url, save_path)"] --> B{"classify_url"} - subgraph shared["shared fetch helper (Component 3)"] + subgraph shared["shared fetch helper (Component 3) — raises domain errors; tool maps them"] direction TB B -->|DIAL| E1["parameter error:
already in DIAL storage (tool-neutral)"] - B -->|unsupported| E2["unsupported-scheme error"] + B -->|unsupported| E2["parameter error:
unsupported scheme"] B -->|EXTERNAL| C["ExternalUrlFetcher.fetch(url)
egress + SSRF + size/redirect/timeout"] - C -->|"ExternalFetchDisabledError / ExternalFetchError"| E3["parameter error
(existing messages)"] + C -->|"ExternalFetchDisabledError / ExternalFetchError"| E3["tool error (forwarded):
existing messages"] end C -->|"FetchedBytes"| M{"save_path given?"} - M -->|no| L{"textual?"} - L -->|no| E4["parameter error:
binary → re-call with a save_path"] + M -->|no| L{"decodes as UTF-8?"} + L -->|no| E4["tool error (forwarded):
binary → re-call with a save_path"] L -->|yes| LT{"within inline cap?"} LT -->|yes| LR["ToolCallResult: full inline text
(no file written)"] LT -->|no| LN["ToolCallResult: head + truncation notice
(under the cap, no file written)"] @@ -174,7 +176,7 @@ flowchart TD - `url: str` (required) — the http(s) URL to fetch. - `save_path: str` (optional) — the workspace-relative destination for the fetched file (e.g. `data.py`, `docs/readme.md`). **Its presence is the switch:** omit it to read the content inline; provide it to persist the resource there and get back the saved path. Subdirectories are allowed; DIAL-style (`files/…`) paths are rejected (Component 4). - **Semantics:** run the shared helper to classify + fetch, then branch on whether `save_path` was given: - - **omitted** → require textual content (Component 3); binary → parameter error telling the agent to re-call with a `save_path` (UC-4). Textual content at or above the inline cap is truncated with a notice (Component 2, UC-3); otherwise the full decoded text is returned inline (code-block wrapped, consistent with the file-tool stage formatting). + - **omitted** → require the body to decode as UTF-8 text (Component 3); a non-decodable (binary) body → a **tool error** (`WebFetchToolErrorException`, forwarded) telling the agent to re-call with a `save_path` (UC-4). Textual content at or above the inline cap is truncated with a notice (Component 2, UC-3); otherwise the full decoded text is returned inline (code-block wrapped, consistent with the file-tool stage formatting). - **given** → persist any content type at `save_path` under the agent home (Component 4) and return the saved relative path (+ a short preview when textual). No size guard. - **Return shape:** without `save_path` → inline text in `ToolCallResult.content`. With `save_path` → the saved relative path (+ optional preview) in `ToolCallResult.content`. Never sets `result.attachments` (see Component 4, "No user-choice propagation"). @@ -182,18 +184,18 @@ flowchart TD - **What:** a byte cap applied only on the inline (no-`save_path`) branch, **configurable** via `features.web_fetch.max_inline_size` (Component 5). Text at or above the cap is **truncated with an explicit notice**, never rejected. - **Why truncate, not reject (third-revision reversal):** the original reject-and-save contract assumed the `internal_file_*` read-back tools exist to consume the saved file. They are an independently gated feature (`features.dial_files`, with per-tool `enabled_tools`), so `web_fetch` can legitimately be the *only* content tool in an app — and the reject flow then dead-ends. It was also wasteful even with file tools present (field-tested: a "summarize this URL" request cost two extra fetches and two dead-end searches): the rejection threw away the downloaded bytes and forced a save + ranged reads for questions the head alone answers. Truncation makes the first call always useful; the offload processor's own precedent (`_REQUIRED_READ_BACK_TOOLS` auto-disable in `dial_files_tooling_module.py`) confirms this dependency class must not be assumed. -- **Truncation shape:** a **leading notice** stating the total byte size, content type, and that re-calling with a `save_path` persists the full content — worded **tool-neutrally** ("use your available tools on it"), never naming `internal_file_*` — followed by a blank line and the UTF-8 head of the decoded text, cut at the last line boundary within budget (falling back to a hard cut when a single line dominates the budget). The notice comes *first* so the truncation is obvious no matter how large the head is, and it deliberately embeds only values known *before* the cut: the head's own byte count is omitted (the model can see how much text it got; the total is what drives the save-or-not decision), which keeps the budget math circularity-free. The format lives in `web_tooling/_truncation.py`, shared with the stage wrapper, which splits the notice back off and renders it **outside** the verbatim content block — the user sees the tool's instruction separated from the fetched text, and fetched text can never pose as a notice (only a notice at position zero, before any fetched byte, is split). +- **Truncation shape:** a **leading notice** stating the total byte size, content type, and that re-calling with a `save_path` persists the full content — worded **tool-neutrally** ("use your available tools on it"), never naming `internal_file_*` — followed by a blank line and the UTF-8 head of the content, cut to the head budget (a plain byte cut; `errors="ignore"` drops only a multibyte sequence split by the boundary). The notice comes *first* so the truncation is obvious no matter how large the head is, and it deliberately embeds only values known *before* the cut: the head's own byte count is omitted (the model can see how much text it got; the total is what drives the save-or-not decision), which keeps the budget math circularity-free. The format lives in `web_tooling/_truncation.py`, shared with the stage wrapper, which splits the notice back off and renders it **outside** the verbatim content block — the user sees the tool's instruction separated from the fetched text, and fetched text can never pose as a notice (only a notice at position zero, before any fetched byte, is split). - **Default:** drawn from the **same env setting that governs the offload threshold's default** (`ToolCallResultOffloadSettings`, 40 KB; `config/dial_files.py`), so out of the box the two are equal and anything the tool returns inline is below the offloader's trigger — the "Read in one call" and "Bounded inline output" goals hold without the global processor silently rewriting the result (see Component 4a for the cases where an operator decouples the two). - **The invariant survives truncation (notice budgeting).** Because the notice embeds only up-front values, it is rendered once and the head budget is simply `max_inline_size` minus the notice's exact byte length, minus the separator, minus one — so notice + head together are always **strictly** below the cap. The global offloader offloads when `size >= size_threshold` (`_offload_processor.py`: it skips only when `size < size_threshold`), so at the default (cap == threshold) a returned result — full or truncated — never reaches the offloader's trigger. The invariant is thus provable, not merely asserted. (A pathological cap smaller than the notice itself yields the notice alone; `max_inline_size` is validated `gt=0` and realistic values are KBs.) - **Comparison is `>=` (at-or-above truncates),** so exactly-at-cap content is truncated too and a returned result is always strictly below the cap. ### Component 3: Shared fetch helper (`WebContentFetcher`) -- **What:** a small helper (`shared/external_fetch/web_content_fetcher.py`, already present on the branch) covering the common front half both branches share. Its entry point is `fetch_external(url, parameter_name="url")`, plus the static `is_textual(content_type)` and `decode(data, content_type)` helpers. -- **Classification:** `classify_url` (`common/url_classification.py`) → `UrlScheme.{DIAL,EXTERNAL,UNSUPPORTED}`. `DIAL` → parameter error (UC-6); unsupported scheme → unsupported-scheme error. -- **Fetch:** wraps `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `fetch_external` already catches `ExternalFetchDisabledError` / `ExternalFetchError` and re-raises them as `InvalidToolCallParameterException` on the given `parameter_name`, matching `FileLoaderService` / `DialFilePromoter`. +- **What:** a small helper (`shared/external_fetch/web_content_fetcher.py`) covering the common front half both branches share: classify the URL, then fetch it. Its only entry point is `fetch_external(url)`. It is **tool-agnostic** — it raises only domain errors and never imports a tool exception. +- **Classification:** `classify_url` (`common/url_classification.py`) → `UrlScheme.{DIAL,DIAL_APPDIR_RELATIVE,EXTERNAL,UNSUPPORTED}`. A DIAL / agent-home-relative URL or an unsupported scheme raises **`WebContentFetchError`** (a plain domain exception defined in this module) carrying a ready, tool-neutral message. The tool maps it to an `InvalidToolCallParameterException` on `url` (UC-6) — a wrong *kind* of URL is a parameter the model should correct. +- **Fetch:** wraps `ExternalUrlFetcher.fetch(url)` → `FetchedBytes{data, content_type, filename}`. This is where the **entire egress policy is enforced** (admin switch, per-app opt-out, host allowlist, SSRF guard, size/redirect/timeout caps). `ExternalFetchDisabledError` / `ExternalFetchError` **propagate unchanged**; the tool catches them and raises `WebFetchToolErrorException` (forwarded to the model via the fallback config) — the URL was valid, so the failure is a tool error, not a bad parameter. - **Request-scoped fetch cache (third revision):** successful fetches are cached by URL for the lifetime of the (request-scoped) instance, so the natural follow-up to a truncated inline read — re-calling the same URL with a `save_path` — does not re-download. Errors are not cached (they may be transient). -- **Textual predicate (`is_textual`):** derived from `content_type` — `text/*`, `application/json`, `application/xml`, and common source/markup types are textual (decoded with the response charset, falling back to UTF-8 with replacement). Everything else is non-textual. Used by the inline branch to gate returning text and by the save branch to decide whether to attach a preview. +- **Text detection lives in the tool, as a UTF-8 decode attempt.** There is no content-type allowlist or charset parsing: the tool calls `data.decode("utf-8")` and treats a success as text and a `UnicodeDecodeError` as binary. The inline branch uses this to gate returning text (binary → tool error, UC-4); the save branch uses it to decide whether to attach a preview. Trade-off: non-UTF-8 text (e.g. a latin-1 page) is read as binary and must be saved — accepted for phase-1 simplicity over the previous content-type heuristic. - **No content-sniffing libraries** in phase-1. `FetchedBytes.filename` is still produced by `ExternalUrlFetcher` but is unused by this tool (saving always uses the caller's `save_path`). ### Component 4: Workspace placement (when `save_path` is given) @@ -223,6 +225,7 @@ The tool is **feature-gated** (enabled through `features.web_fetch`, not through - **Name:** `INTERNAL_WEB_FETCH_TOOL_NAME = "internal_web_fetch"` in `common/tool_names.py`. - **Feature config `WebFetchConfig`** (`config/web_fetch.py`): `enabled: bool = false`, `max_inline_size: int` (validated `gt=0`) defaulting (via `default_factory`) to the offload threshold. Exposed on the `Features` model (`config/application.py`) as a `PreviewField` — so configuring it requires `ENABLE_PREVIEW_FEATURES`. - **Provider:** `WebToolingModule` (`web_tooling/web_tooling_module.py`, `@preview_module`) via a `@multiprovider` that builds the tool when `features.web_fetch.enabled` is true (passing `max_inline_size` from the same config). Registered in `app_factory.py`. +- **Error forwarding:** `WEB_FETCH_TOOL_CONFIG.fallback_configuration` uses `ContinueStrategyModel(forward_tool_error_message=True)`, so `WebFetchToolErrorException` messages (egress denied, non-text body, …) are forwarded to the model, which can then react (e.g. re-call with a `save_path`). The other class of failure — `InvalidToolCallParameterException` (wrong *kind* of URL, bad `save_path`) — is handled by the base tool's separate, guaranteed retry-instruction path (`staged_base_tool.py`), independent of `fallback_configuration`. - **Egress init-gate (fail fast on contradictory config):** the tool is useless without external egress, so `WebToolingModule` injects `ExternalUrlFetchPolicyResolver` and, when `web_fetch.enabled` is set but `is_enabled()` is false (admin cap or per-app opt-out), **does not build the tool** and emits a hard `ToolInitializationException` via a second `@multiprovider -> list[InitializationException]` (aggregated by `_InitializationErrorHandler` into the "Initialization issues" stage). This replaces per-call egress-disabled errors for the on/off case (UC-5); host-allowlist denials stay runtime (Component 3). - **Shared home resolver:** move `_HomePathResolver` (`dial_files_tooling/_home_path_resolver.py`) into a **new `shared/home_path/` package** as public `HomePathResolver`, bound request-scoped by a `HomePathModule` spliced into `shared_module` (`shared/__init__.py`, mirroring the existing `ExternalFetchModule` there, per the CLAUDE.md `shared/` convention). `dial_files_tooling` drops its own binding (`dial_files_tooling_module.py`) and injects the shared type; `_DialFileTool` and the `_AppdataHomePathTransformer` keep working against the same public API. Its constructor deps (`DialFileService`, `DialFilesConfig`) are already globally bound. - **Preview gating:** `WebToolingModule` is `@preview_module` **and** `WebFetchConfig` is exposed via `PreviewField` — so `internal_web_fetch` is preview-gated at both the module and config level. (This mirrors how the dial-files tooling is preview-gated at the *module* level via `@preview_module`; note the `Features.dial_files` config field itself is a plain `Field`, so the two are not identical — `web_fetch` additionally nullifies its config outside preview.) @@ -285,7 +288,7 @@ features: `internal_web_fetch(url="https://…/huge.log")` where the log is 425 KB → returns `[Truncated: fetched content is 424767 bytes (text/plain), larger than the inline cap; only the beginning follows. To process the full content, re-call with a save_path to persist it to the workspace, then use your available tools on it.]` -followed by a blank line and the head of the log (cut at a line boundary; notice + head under the 40 KB cap). The stage shows the notice above the verbatim content block. A follow-up `internal_web_fetch(url=…, save_path="huge.log")` is served from the request-scoped fetch cache — no second download. +followed by a blank line and the head of the log (a plain byte cut; notice + head under the 40 KB cap). The stage shows the notice above the verbatim content block. A follow-up `internal_web_fetch(url=…, save_path="huge.log")` is served from the request-scoped fetch cache — no second download. ### Walkthrough — save to the workspace (UC-2) @@ -318,11 +321,12 @@ None. The tool is purely additive and opt-in via app config. - `shared/home_path/home_path_resolver.py` + `home_path_module.py` — public `HomePathResolver` (moved from `dial_files_tooling/_home_path_resolver.py`) and `HomePathModule`, spliced into `shared_module`. - `web_tooling/web_tooling_module.py` — `WebToolingModule` (`@preview_module`; `configure` + `@multiprovider`). - `web_tooling/_web_fetch_tool.py` — the `internal_web_fetch` tool (plain `StagedBaseTool`; injects the shared fetch helper + shared home resolver; branches on `save_path`). -- `web_tooling/_tool_configs.py` — the tool's `InternalTool` config (`WEB_FETCH_TOOL_CONFIG`), with `url` and optional `save_path` parameters. +- `web_tooling/_tool_configs.py` — the tool's `InternalTool` config (`WEB_FETCH_TOOL_CONFIG`), with `url` and optional `save_path` parameters and a `fallback_configuration` that forwards tool-error messages. - `web_tooling/_web_fetch_stage_wrapper.py` — the tool's stage wrapper. +- `web_tooling/_web_fetch_tool_error_exception.py` — `WebFetchToolErrorException` (a `ToolErrorException`) for fetch failures on a valid URL (egress denied, non-text body, …). - `web_tooling/_truncation.py` — the truncation-notice format, shared by the tool (composes notice + head) and the stage wrapper (splits the notice back off for display outside the verbatim block). - `config/web_fetch.py` — `WebFetchConfig` (`enabled`, `max_inline_size` with `gt=0`). -- `shared/external_fetch/web_content_fetcher.py` — the shared fetch helper (Component 3), exposing `fetch_external` / `is_textual` / `decode`. +- `shared/external_fetch/web_content_fetcher.py` — the shared fetch helper (Component 3): `fetch_external` + the domain error `WebContentFetchError` (fetch + scheme-classify only; tool-agnostic). - Unit tests for the tool (both branches) and the fetch helper. ### Modified files (vs. `development`) @@ -351,18 +355,18 @@ The current branch already carries a superseded two-tool implementation. Bringin ### Tests -- **inline:** textual within the cap → full inline content, no file written. -- **inline:** textual at/over the cap → leading truncation notice + head (line-boundary cut, accurate total byte count, `save_path` guidance, no `internal_file_*` name-drops), whole result strictly below the cap, no file written. +- **inline:** UTF-8 content within the cap → full inline content, no file written; a missing/any content type that still decodes → returned inline. +- **inline:** UTF-8 content at/over the cap → leading truncation notice + head (accurate total byte count, `save_path` guidance, no `internal_file_*` name-drops), whole result strictly below the cap, no file written. - **stage rendering:** truncation notice split off and rendered outside the verbatim content block; non-truncated content untouched; fetched text merely resembling a notice (no composed separator) is not split. -- **inline:** binary → parameter error telling the agent to pass a `save_path`. -- **fetch helper:** repeat fetch of the same URL within a request → served from the cache (one download); fetch errors are not cached. +- **inline:** non-UTF-8 / binary body → tool error (forwarded) telling the agent to pass a `save_path`. +- **fetch helper:** repeat fetch of the same URL within a request → served from the cache; scheme rejections raise `WebContentFetchError`; egress errors propagate unchanged (not cached). - **save:** textual → persisted at `save_path` under agent home, relative path + preview returned. - **save:** binary → persisted at `save_path`, relative path + content type + size returned, no inline body. - **save:** `save_path` with a subdirectory → persisted at that workspace-relative path. - **save:** invalid `save_path` (absolute / traversal / empty / `files/`-prefixed) → parameter error on `save_path`. - **save:** `save_path` collision → numeric-suffix uniquification. - **save** then `internal_file_list`/`read_lines` on the returned relative path (workspace-placement guarantee). -- **both branches:** host not allowed / SSRF → parameter error with the policy message (runtime). -- **both branches:** DIAL URL → parameter error pointing to the file tools. +- **both branches:** host not allowed / SSRF → tool error (forwarded) with the policy message (runtime). +- **both branches:** DIAL URL → tool-neutral parameter error on `url`. - **config:** `features.web_fetch.enabled=false` (or omitted) → tool not exposed (UC-7). - **egress init-gate:** `web_fetch.enabled=true` but egress disabled → tool not exposed **and** a hard `ToolInitializationException` emitted (UC-5); `enabled=false` + egress disabled → no error. diff --git a/docs/generated-internal-tools.json b/docs/generated-internal-tools.json index 44fdf0ea..2535d566 100644 --- a/docs/generated-internal-tools.json +++ b/docs/generated-internal-tools.json @@ -375,7 +375,7 @@ }, { "name": "internal_web_fetch", - "description": "Fetch a resource from an external http(s) URL (e.g. a README, a source file, a documentation page). Without save_path it returns the text inline in a single call — text only: binary content (images, PDFs, archives) is rejected, re-call with a save_path instead. Text larger than the inline cap is returned truncated to its head with a notice stating the total size; the head is often enough (e.g. to summarize), otherwise re-call with a save_path. With save_path it persists the resource (any content type) at that workspace-relative path under the agent home and returns the saved path (+ a short preview for text), so other available tools can process the full content. Note: URLs of HTML viewer pages (e.g. github.com /blob/ pages) return the whole HTML page; prefer the raw/plain endpoint when one exists. DIAL file paths (files/...) are not fetched here.", + "description": "Fetch a resource from an external http(s) URL (e.g. a README, a source file, a documentation page). Without save_path it returns the text inline in a single call — text only: binary content (images, PDFs, archives) is rejected, re-call with a save_path instead. Text larger than the inline cap is returned truncated to its head with a notice stating the total size; the head is often enough, otherwise re-call with a save_path. With save_path it persists the resource (any content type) at that workspace-relative path under the agent home and returns the saved path (+ a short preview for text), so other available tools can process the full content. DIAL file paths (files/...) are not fetched here.", "properties": { "url": { "type": "string", diff --git a/src/quickapp/dial_files_tooling/dial_files_tooling_module.py b/src/quickapp/dial_files_tooling/dial_files_tooling_module.py index 93b87d72..8bdcbfc7 100644 --- a/src/quickapp/dial_files_tooling/dial_files_tooling_module.py +++ b/src/quickapp/dial_files_tooling/dial_files_tooling_module.py @@ -48,7 +48,6 @@ class DialFilesToolingModule(Module): def configure(self, binder: Binder) -> None: binder.bind(_FileStageWrapper, to=_FileStageWrapper, scope=request_scope) - # HomePathResolver is bound by shared HomePathModule (shared_module). binder.bind( _AppdataHomePathTransformer, to=_AppdataHomePathTransformer, scope=request_scope ) diff --git a/src/quickapp/shared/external_fetch/web_content_fetcher.py b/src/quickapp/shared/external_fetch/web_content_fetcher.py index 3fa062b4..898fbcff 100644 --- a/src/quickapp/shared/external_fetch/web_content_fetcher.py +++ b/src/quickapp/shared/external_fetch/web_content_fetcher.py @@ -1,149 +1,62 @@ -"""Shared front-half for the built-in ``internal_web_fetch`` tool. +"""Fetch + scheme-classify helper for the built-in ``internal_web_fetch`` tool. -Both of the tool's branches (read inline / save to workspace) need the same -steps before they diverge: classify the URL, reject DIAL / unsupported schemes -with actionable errors, fetch the bytes through the single egress point -(:class:`ExternalUrlFetcher`), and decide whether the payload is textual. This -helper owns exactly that shared half; the tool decides what to do with the -returned :class:`FetchedBytes`. +Wraps :class:`ExternalUrlFetcher` with URL-scheme classification and a +request-scoped fetch cache. It raises only domain errors — its own +:class:`WebContentFetchError`, or the fetcher's ``ExternalFetchError`` / +``ExternalFetchDisabledError`` — leaving the tool to translate them into its +parameter-error shape. """ -from email.message import Message - from injector import inject from quickapp.common.dial_settings import DialSettings -from quickapp.common.exceptions import InvalidToolCallParameterException -from quickapp.common.url_classification import UrlScheme, classify_url, unsupported_scheme_error -from quickapp.shared.external_fetch.external_url_fetcher import ( - ExternalFetchDisabledError, - ExternalFetchError, - ExternalUrlFetcher, - FetchedBytes, -) - -# Non-``text/*`` MIME types that are nevertheless textual and safe to decode / -# inline. Kept small and explicit in phase-1 (no content-sniffing library). -_TEXTUAL_APPLICATION_TYPES = frozenset( - { - "application/json", - "application/xml", - "application/xhtml+xml", - "application/javascript", - "application/ecmascript", - "application/x-yaml", - "application/yaml", - "application/x-www-form-urlencoded", - "application/csv", - } -) - - -def _base_content_type(content_type: str | None) -> str | None: - """Return the lower-cased MIME type without parameters (``; charset=...``).""" - if not content_type: - return None - return content_type.split(";", 1)[0].strip().lower() +from quickapp.common.url_classification import UrlScheme, classify_url +from quickapp.common.url_sanitization import sanitize_url_for_log +from quickapp.shared.external_fetch.external_url_fetcher import ExternalUrlFetcher, FetchedBytes -def _charset(content_type: str | None) -> str | None: - if not content_type: - return None - msg = Message() - msg["content-type"] = content_type - charset = msg.get_param("charset") - return charset if isinstance(charset, str) and charset else None +class WebContentFetchError(Exception): + """A URL cannot be fetched because it is not an external http(s) resource.""" @inject class WebContentFetcher: - """Classify + fetch + decode helper shared by the built-in web tools. + """Classify + fetch helper shared by the built-in web tools. - A thin wrapper over :class:`ExternalUrlFetcher` that maps its scheme - classification and egress errors into ``InvalidToolCallParameterException`` - (the tool-facing error shape), mirroring ``FileLoaderService``. The egress - policy itself (admin switch, per-app opt-out, host allowlist, SSRF guard, - size / redirect / timeout caps) lives entirely in the fetcher and is reused - verbatim. + Reuses the egress policy of :class:`ExternalUrlFetcher` (admin switch, + per-app opt-out, host allowlist, SSRF guard, size / redirect / timeout caps) + verbatim; adds scheme classification and a per-request fetch cache. """ - def __init__( - self, - external_fetcher: ExternalUrlFetcher, - dial_settings: DialSettings, - ) -> None: + def __init__(self, external_fetcher: ExternalUrlFetcher, dial_settings: DialSettings) -> None: self.__external_fetcher = external_fetcher self.__dial_url = dial_settings.url - # Fetch cache, request-scoped because this class is bound request-scoped: - # a re-call for the same URL within one request — e.g. a save_path call - # after a truncated inline read — must not re-download. + # Request-scoped (this class is bound request-scoped) so a re-call for the + # same URL within one request does not re-download. self.__fetched_by_url: dict[str, FetchedBytes] = {} - async def fetch_external(self, url: str, parameter_name: str = "url") -> FetchedBytes: - """Classify ``url`` and fetch it, or raise a tool-facing parameter error. + async def fetch_external(self, url: str) -> FetchedBytes: + """Classify ``url`` and fetch it, caching successful fetches per request. - * DIAL URL or agent-home-relative path -> rejected: the resource is - already in DIAL storage and the fetch tools are for *external* - retrieval only. The message stays tool-neutral — which workspace - tools exist varies per app. - * Unsupported scheme -> the canonical unsupported-scheme error. - * External -> fetched through :class:`ExternalUrlFetcher`; its - ``ExternalFetchError`` / ``ExternalFetchDisabledError`` (egress - disabled, host not allowed, SSRF, size, timeout, transport) are - re-raised as ``InvalidToolCallParameterException`` carrying the - existing operator/builder messages. Successful fetches are cached for - the lifetime of this (request-scoped) instance, keyed by URL. + Raises :class:`WebContentFetchError` for DIAL / unsupported URLs and + propagates ``ExternalFetchError`` / ``ExternalFetchDisabledError`` for + egress failures. """ scheme = classify_url(url, self.__dial_url) if scheme in (UrlScheme.DIAL, UrlScheme.DIAL_APPDIR_RELATIVE): - raise InvalidToolCallParameterException( - parameter_name, + raise WebContentFetchError( f"URL {url} already points to a file in DIAL storage and does not " - "need fetching. Access it with your available workspace tools.", + "need fetching. Access it with your available workspace tools." ) if scheme == UrlScheme.UNSUPPORTED: - raise unsupported_scheme_error(url, parameter_name) + raise WebContentFetchError( + f"URL scheme not supported: {sanitize_url_for_log(url)}. " + "Only http(s) URLs can be fetched." + ) cached = self.__fetched_by_url.get(url) if cached is not None: return cached - - try: - fetched = await self.__external_fetcher.fetch(url) - except (ExternalFetchError, ExternalFetchDisabledError) as exc: - raise InvalidToolCallParameterException(parameter_name, str(exc)) from exc - + fetched = await self.__external_fetcher.fetch(url) self.__fetched_by_url[url] = fetched return fetched - - @staticmethod - def is_textual(content_type: str | None) -> bool: - """Whether a payload with this ``content_type`` can be decoded as text. - - Textual: any ``text/*`` type, an explicit allowlisted ``application/*`` - type, or a structured-syntax suffix (``+json`` / ``+xml``). A missing - content type is treated as **non-textual** — phase-1 fails closed rather - than risk decoding binary bytes into the LLM context. - """ - base = _base_content_type(content_type) - if base is None: - return False - if base.startswith("text/"): - return True - if base in _TEXTUAL_APPLICATION_TYPES: - return True - return base.endswith("+json") or base.endswith("+xml") - - @staticmethod - def decode(data: bytes, content_type: str | None) -> str: - """Decode ``data`` using the response charset, defaulting to UTF-8. - - Uses ``errors="replace"`` so an incorrectly-declared charset degrades to - replacement characters rather than raising — the caller has already - decided the payload is textual. - """ - charset = _charset(content_type) or "utf-8" - try: - return data.decode(charset, errors="replace") - except LookupError: - return data.decode("utf-8", errors="replace") diff --git a/src/quickapp/web_tooling/_tool_configs.py b/src/quickapp/web_tooling/_tool_configs.py index d6692c87..1bcd3ce7 100644 --- a/src/quickapp/web_tooling/_tool_configs.py +++ b/src/quickapp/web_tooling/_tool_configs.py @@ -8,8 +8,14 @@ ) from quickapp.config.tools.display.tool import ToolDisplayConfig, ToolStageConfig from quickapp.config.tools.internal import InternalTool +from quickapp.config.tools.tool_fallback import ContinueStrategyModel, ToolFallbackConfig WEB_FETCH_TOOL_CONFIG = InternalTool( + # Forward fetch-error messages (egress denied, non-text body, ...) so the model + # sees the reason and can react, e.g. re-call with a save_path. + fallback_configuration=ToolFallbackConfig( + strategies=[ContinueStrategyModel(forward_tool_error_message=True)], + ), open_ai_tool=OpenAiToolConfig( function=OpenAiToolFunction( name=INTERNAL_WEB_FETCH_TOOL_NAME, @@ -19,14 +25,12 @@ "text inline in a single call — text only: binary content (images, " "PDFs, archives) is rejected, re-call with a save_path instead. Text " "larger than the inline cap is returned truncated to its head with a " - "notice stating the total size; the head is often enough (e.g. to " - "summarize), otherwise re-call with a save_path. With save_path it " - "persists the resource (any content type) at that workspace-relative " - "path under the agent home and returns the saved path (+ a short " - "preview for text), so other available tools can process the full " - "content. Note: URLs of HTML viewer pages (e.g. github.com /blob/ " - "pages) return the whole HTML page; prefer the raw/plain endpoint " - "when one exists. DIAL file paths (files/...) are not fetched here." + "notice stating the total size; the head is often enough, otherwise " + "re-call with a save_path. With save_path it persists the resource " + "(any content type) at that workspace-relative path under the agent " + "home and returns the saved path (+ a short preview for text), so " + "other available tools can process the full content. DIAL file paths " + "(files/...) are not fetched here." ), parameters=OpenAiToolFunctionParameters( type=JsonTypeEnum.object, diff --git a/src/quickapp/web_tooling/_truncation.py b/src/quickapp/web_tooling/_truncation.py index 34d0d728..8a5a6ba7 100644 --- a/src/quickapp/web_tooling/_truncation.py +++ b/src/quickapp/web_tooling/_truncation.py @@ -1,16 +1,12 @@ """Truncation notice for oversized inline web fetches. -Shared by the tool (which composes ``notice + blank line + fetched head``) and -the stage wrapper (which splits the notice back off to render it outside the -verbatim content block). Keeping the format here is what lets the wrapper -separate the tool's instructions from the fetched text without guessing. +Shared by the tool (composes ``notice + blank line + head``) and the stage +wrapper (splits the notice back off to render it outside the verbatim block). """ -# Deliberately tool-neutral: which tools can process a saved file varies per -# app (file tools, RAG, get_content, ...), and this module must not assume any -# of them is enabled. Embeds only values known before the head is cut (total -# size, content type) so the notice can be rendered once, up front, and its -# exact length reserved from the head budget. +# Tool-neutral (which tools can process a saved file varies per app) and embeds +# only values known before the head is cut, so its exact length can be reserved +# from the head budget. TRUNCATION_NOTICE_TEMPLATE = ( "[Truncated: fetched content is {total} bytes ({content_type}), larger " "than the inline cap; only the beginning follows. To process the full " @@ -33,10 +29,9 @@ def separator_byte_length() -> int: def split_truncation_notice(content: str) -> tuple[str | None, str]: """Split ``content`` into ``(notice, fetched text)``. - Returns ``(None, content)`` unchanged for non-truncated results. Only a - notice composed by :func:`compose_truncated_content` is split off — it sits - at position zero, before any fetched byte, so fetched text can never spoof - its way out of the verbatim block. + Returns ``(None, content)`` for non-truncated results. Only a notice at + position zero composed by :func:`compose_truncated_content` is split off, so + fetched text cannot spoof its way out of the verbatim block. """ if content.startswith(_NOTICE_PREFIX): notice, sep, body = content.partition(_SEPARATOR) diff --git a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py index 83388174..33937a0f 100644 --- a/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py +++ b/src/quickapp/web_tooling/_web_fetch_stage_wrapper.py @@ -17,13 +17,10 @@ def _build_debug_info_from_exception(self, exception: Exception) -> str: return f"### Exception:\n\r{exception}\n\r" def _build_debug_info_from_result(self, result: ToolCallResult) -> str: - # A truncated read is composed as notice + blank line + fetched head; show - # the notice outside the verbatim block so it stays visible however large - # the head is and is never mistaken for fetched text. + # Render any truncation notice outside the verbatim block so it stays + # visible; render the fetched text (often Markdown/HTML) fenced so the UI + # shows it verbatim instead of formatting it. notice, body = split_truncation_notice(result.content) - # Fetched content is often itself Markdown/HTML (e.g. a README). Render it - # inside a fenced code block so the stage shows the fetched text verbatim - # instead of the UI rendering it as formatted Markdown. content_block = f"**Content:**\n{fenced_code_block(body)}\n" if notice is None: return content_block diff --git a/src/quickapp/web_tooling/_web_fetch_tool.py b/src/quickapp/web_tooling/_web_fetch_tool.py index b7382cdf..3d77fea1 100644 --- a/src/quickapp/web_tooling/_web_fetch_tool.py +++ b/src/quickapp/web_tooling/_web_fetch_tool.py @@ -12,8 +12,15 @@ from quickapp.config.application import StageDisplayLevel from quickapp.config.tools.internal import InternalTool from quickapp.dial_core_services.dial_file_service import DialFileService -from quickapp.shared.external_fetch.external_url_fetcher import FetchedBytes -from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher +from quickapp.shared.external_fetch.external_url_fetcher import ( + ExternalFetchDisabledError, + ExternalFetchError, + FetchedBytes, +) +from quickapp.shared.external_fetch.web_content_fetcher import ( + WebContentFetcher, + WebContentFetchError, +) from quickapp.shared.home_path.home_path_resolver import HomePathResolver from quickapp.web_tooling._truncation import ( TRUNCATION_NOTICE_TEMPLATE, @@ -21,9 +28,9 @@ separator_byte_length, ) from quickapp.web_tooling._web_fetch_stage_wrapper import _WebFetchStageWrapper +from quickapp.web_tooling._web_fetch_tool_error_exception import WebFetchToolErrorException -# Preview shown for textual saves so the agent can decide whether to read the -# full file. Kept short to stay well below any offload threshold. +# Preview of a textual save, short enough to stay below the offload threshold. _PREVIEW_CHARS = 1000 # Bound on collision-avoidance retries when the target filename already exists. @@ -32,25 +39,14 @@ @inject class _WebFetchTool(StagedBaseTool): - """``internal_web_fetch`` — fetch an external resource; read it or save it. - - A single optional ``save_path`` selects the behaviour: - - * **omitted** — the payload must be textual; the decoded text is returned - inline. Content at or above ``max_inline_size`` is truncated: a leading - notice (total size + how to get the rest) followed by the content head, - sized so the whole result stays strictly below the cap — and, at the - default where the cap equals the global offload threshold, below the - offloader's trigger (see docs/designs/web_fetch_tool.md, Component 4a). - Binary content is rejected with guidance to re-call with a ``save_path``. - Guidance never names other tools — which tools can process a saved file - varies per app. - * **given** — the fetched bytes (any content type) are persisted at - ``save_path`` under the agent home via the shared home-path resolver and a - bytes write, and the saved workspace-relative path (+ a short preview for - textual content) is returned so the ``internal_file_*`` tools can operate on - it. The file is never surfaced to the user choice (``result.attachments`` is - left empty), unlike ``_WriteFileTool``. + """``internal_web_fetch`` — fetch an external URL, then read it or save it. + + ``save_path`` omitted: the body must decode as UTF-8 text and is returned + inline (truncated to its head with a leading notice once it reaches + ``max_inline_size``); a binary body is rejected. ``save_path`` given: the + fetched bytes are persisted under the agent home and the saved path (+ a + short preview for text) is returned, without surfacing the file to the user + choice (``result.attachments`` stays empty). """ def __init__( @@ -88,7 +84,7 @@ async def _run_in_stage_async( url: str = kwargs["url"] save_path: str | None = kwargs.get("save_path") - fetched = await self.__web_content_fetcher.fetch_external(url, parameter_name="url") + fetched = await self.__fetch(url) if save_path is None: result = self.__load_into_context(url, fetched) @@ -99,57 +95,50 @@ async def _run_in_stage_async( stage_wrapper.add_result(result) return result + async def __fetch(self, url: str) -> FetchedBytes: + try: + return await self.__web_content_fetcher.fetch_external(url) + except WebContentFetchError as e: + # Wrong kind of URL (DIAL path / unsupported scheme): a parameter the + # model should correct and retry. + raise InvalidToolCallParameterException("url", str(e)) from e + except (ExternalFetchError, ExternalFetchDisabledError) as e: + # The URL is valid; the fetch itself failed (egress policy, size, + # timeout, ...). A tool error, not a bad parameter. + raise WebFetchToolErrorException(self._resolve_tool_name(), str(e)) from e + def __load_into_context(self, url: str, fetched: FetchedBytes) -> ToolCallResult: - if not WebContentFetcher.is_textual(fetched.content_type): - raise InvalidToolCallParameterException( - "url", - f"URL {url} returned non-text content " - f"(content-type: {fetched.content_type or 'unknown'}), which cannot be " - "loaded into context. Re-call with a save_path to persist it to the " - "workspace, then use your available tools on it.", + text = _try_decode_utf8(fetched.data) + if text is None: + raise WebFetchToolErrorException( + self._resolve_tool_name(), + f"URL {url} returned non-text content (content-type: " + f"{fetched.content_type or 'unknown'}); it cannot be loaded into context. " + "Re-call with a save_path to persist it to the workspace, then use your " + "available tools on it.", ) - text = WebContentFetcher.decode(fetched.data, fetched.content_type) content_type = fetched.content_type or "text/plain" - - # Guard against dumping unbounded content into the LLM context: at or above - # the cap the head is returned with an explicit truncation notice, never an - # error — a truncated read must stay useful even when no other tool that - # could process a saved copy is enabled. - encoded = text.encode("utf-8") - if len(encoded) >= self.__max_inline_size: - text = self.__truncate_with_notice(encoded, content_type) - + # Keep unbounded content out of the LLM context: at or above the cap the + # head is returned with a leading truncation notice, never an error. + if len(fetched.data) >= self.__max_inline_size: + text = self.__truncate_with_notice(fetched.data, content_type) return ToolCallResult(content=text, content_type=content_type) - def __truncate_with_notice(self, encoded: bytes, content_type: str) -> str: - """Return the truncation notice followed by the head of ``encoded``. - - The notice comes *first* so it stays visible however large the head is, - both to the model and — split back off by the stage wrapper — to the - user. It embeds only values known up front, so the head budget is simply - the cap minus the notice's exact length (plus separator), keeping notice - + content strictly below ``max_inline_size`` — an inline result never - reaches the global offload trigger (cap == threshold by default). The - cut prefers the last newline within budget (unless that drops more than - half of it) so the head ends on a whole line. A cap smaller than the - notice itself yields the notice alone. - """ - notice = TRUNCATION_NOTICE_TEMPLATE.format(total=len(encoded), content_type=content_type) + def __truncate_with_notice(self, data: bytes, content_type: str) -> str: + # Notice first so it stays visible; reserve its byte length (+ separator + # + 1) from the head budget so notice + head stays strictly below the cap + # and never reaches the offload trigger (cap == threshold by default). + notice = TRUNCATION_NOTICE_TEMPLATE.format(total=len(data), content_type=content_type) reserved = len(notice.encode("utf-8")) + separator_byte_length() + 1 budget = max(self.__max_inline_size - reserved, 0) - head = encoded[:budget] - newline_cut = head.rfind(b"\n") - if newline_cut >= budget // 2: - head = head[:newline_cut] - # "ignore" drops only a UTF-8 sequence split by the byte cut; the rest of - # the head is valid by construction. - return compose_truncated_content(notice, head.decode("utf-8", errors="ignore")) + # "ignore" drops only a UTF-8 sequence split by the byte cut. + head = data[:budget].decode("utf-8", errors="ignore") + return compose_truncated_content(notice, head) async def __save_to_workspace(self, save_path: str, fetched: FetchedBytes) -> ToolCallResult: # A ``files/``-prefixed path would be returned verbatim by - # ``resolve_appdata_url`` (bypassing home-prefixing and traversal validation), - # escaping the agent home. Require a home-relative destination. + # ``resolve_appdata_url``, escaping the agent home. Require a home-relative one. if save_path.startswith("files/"): raise InvalidToolCallParameterException( "save_path", @@ -160,11 +149,10 @@ async def __save_to_workspace(self, save_path: str, fetched: FetchedBytes) -> To content_type = fetched.content_type or "application/octet-stream" display_path = await self.__write_unique(save_path, fetched.data, content_type) - size = len(fetched.data) - content = f"Saved {display_path} ({content_type}, {size} bytes)." - if WebContentFetcher.is_textual(fetched.content_type): - preview = WebContentFetcher.decode(fetched.data, fetched.content_type)[:_PREVIEW_CHARS] - content = f"{content}\n\nPreview:\n{preview}" + content = f"Saved {display_path} ({content_type}, {len(fetched.data)} bytes)." + preview = _try_decode_utf8(fetched.data) + if preview is not None: + content = f"{content}\n\nPreview:\n{preview[:_PREVIEW_CHARS]}" return ToolCallResult(content=content, content_type="text/plain") @@ -172,9 +160,8 @@ async def __write_unique(self, save_path: str, data: bytes, content_type: str) - """Write ``data`` at ``save_path`` under the agent home, uniquifying on collision. Uses ``overwrite=False`` so an existing file is never clobbered: on an - ``If-None-Match`` failure the name is uniquified (``name-1.ext``, - ``name-2.ext``, …) and retried. Returns the workspace-relative display path - actually written. + ``If-None-Match`` failure the name is uniquified (``name-1.ext``, …) and + retried. Returns the workspace-relative display path actually written. """ for attempt in range(_MAX_UNIQUE_ATTEMPTS): candidate = save_path if attempt == 0 else _with_suffix(save_path, attempt) @@ -203,6 +190,14 @@ async def __write_unique(self, save_path: str, data: bytes, content_type: str) - ) +def _try_decode_utf8(data: bytes) -> str | None: + """Return ``data`` decoded as UTF-8, or ``None`` if it is not valid UTF-8.""" + try: + return data.decode("utf-8") + except UnicodeDecodeError: + return None + + def _with_suffix(path: str, n: int) -> str: """Insert ``-n`` before the extension: ``data.py`` -> ``data-1.py``.""" pure = PurePosixPath(path) diff --git a/src/quickapp/web_tooling/_web_fetch_tool_error_exception.py b/src/quickapp/web_tooling/_web_fetch_tool_error_exception.py new file mode 100644 index 00000000..dccd7655 --- /dev/null +++ b/src/quickapp/web_tooling/_web_fetch_tool_error_exception.py @@ -0,0 +1,12 @@ +from quickapp.common.exceptions.tool_error import ToolErrorException + + +class WebFetchToolErrorException(ToolErrorException): + """Raised when a fetch fails for a valid URL (egress denied, size, timeout, + transport, or a non-text body on an inline read). + + Not a bad-parameter case: the URL is correct, so this is a tool error whose + message is forwarded to the model/user via the tool's fallback configuration. + """ + + tool_kind = "Web fetch tool" diff --git a/src/tests/unit_tests/shared/test_web_content_fetcher.py b/src/tests/unit_tests/shared/test_web_content_fetcher.py index 0038865e..a724133c 100644 --- a/src/tests/unit_tests/shared/test_web_content_fetcher.py +++ b/src/tests/unit_tests/shared/test_web_content_fetcher.py @@ -2,13 +2,15 @@ import pytest -from quickapp.common.exceptions import InvalidToolCallParameterException from quickapp.shared.external_fetch.external_url_fetcher import ( ExternalFetchDisabledError, ExternalFetchError, FetchedBytes, ) -from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher +from quickapp.shared.external_fetch.web_content_fetcher import ( + WebContentFetcher, + WebContentFetchError, +) _DIAL_URL = "https://dial.example.com" @@ -35,128 +37,52 @@ async def test_external_url_fetched(self): @pytest.mark.asyncio async def test_dial_relative_path_rejected(self): fetcher = _make_fetcher() - with pytest.raises(InvalidToolCallParameterException) as exc: + with pytest.raises(WebContentFetchError) as exc: await fetcher.fetch_external("files/bucket/doc.md") - assert exc.value.parameter_name == "url" - assert "DIAL storage" in exc.value.message + assert "DIAL storage" in str(exc.value) # Guidance must stay tool-neutral: no other tool availability is assumed. - assert "internal_file" not in exc.value.message + assert "internal_file" not in str(exc.value) @pytest.mark.asyncio async def test_home_relative_path_rejected_as_already_in_storage(self): fetcher = _make_fetcher() - with pytest.raises(InvalidToolCallParameterException) as exc: + with pytest.raises(WebContentFetchError) as exc: await fetcher.fetch_external("reports/img.png") - assert exc.value.parameter_name == "url" - assert "DIAL storage" in exc.value.message - - @pytest.mark.asyncio - async def test_repeat_fetch_served_from_request_cache(self): - fetched = FetchedBytes(data=b"hello", content_type="text/plain", filename="a.txt") - external = MagicMock() - external.fetch = AsyncMock(return_value=fetched) - dial_settings = MagicMock() - dial_settings.url = _DIAL_URL - fetcher = WebContentFetcher(external_fetcher=external, dial_settings=dial_settings) - first = await fetcher.fetch_external("https://example.com/a.txt") - second = await fetcher.fetch_external("https://example.com/a.txt") - assert second is first - external.fetch.assert_awaited_once() - - @pytest.mark.asyncio - async def test_fetch_errors_not_cached(self): - external = MagicMock() - external.fetch = AsyncMock( - side_effect=ExternalFetchError(reason="size_limit", url="https://x.com") - ) - dial_settings = MagicMock() - dial_settings.url = _DIAL_URL - fetcher = WebContentFetcher(external_fetcher=external, dial_settings=dial_settings) - with pytest.raises(InvalidToolCallParameterException): - await fetcher.fetch_external("https://x.com") - with pytest.raises(InvalidToolCallParameterException): - await fetcher.fetch_external("https://x.com") - assert external.fetch.await_count == 2 + assert "DIAL storage" in str(exc.value) @pytest.mark.asyncio async def test_dial_host_url_rejected(self): fetcher = _make_fetcher() - with pytest.raises(InvalidToolCallParameterException): + with pytest.raises(WebContentFetchError): await fetcher.fetch_external(f"{_DIAL_URL}/v1/files/x/doc.md") @pytest.mark.asyncio async def test_unsupported_scheme_rejected(self): fetcher = _make_fetcher() - with pytest.raises(InvalidToolCallParameterException) as exc: + with pytest.raises(WebContentFetchError) as exc: await fetcher.fetch_external("ftp://example.com/x") - assert "scheme not supported" in exc.value.message + assert "scheme not supported" in str(exc.value) @pytest.mark.asyncio - async def test_egress_disabled_wrapped_as_parameter_error(self): - fetcher = _make_fetcher(ExternalFetchDisabledError(reason="admin", url="https://x.com")) - with pytest.raises(InvalidToolCallParameterException) as exc: - await fetcher.fetch_external("https://x.com") - assert exc.value.parameter_name == "url" - assert "EXTERNAL_URL_FETCH_ENABLED" in exc.value.message + async def test_repeat_fetch_served_from_request_cache(self): + fetched = FetchedBytes(data=b"hello", content_type="text/plain", filename="a.txt") + fetcher = _make_fetcher(fetched) + first = await fetcher.fetch_external("https://example.com/a.txt") + second = await fetcher.fetch_external("https://example.com/a.txt") + assert second is first @pytest.mark.asyncio - async def test_fetch_error_wrapped_as_parameter_error(self): + async def test_egress_errors_propagate_and_are_not_cached(self): + # The fetcher no longer wraps egress errors; they propagate unchanged and + # a failed fetch is retried on the next call (not cached). fetcher = _make_fetcher(ExternalFetchError(reason="size_limit", url="https://x.com")) - with pytest.raises(InvalidToolCallParameterException) as exc: + with pytest.raises(ExternalFetchError): + await fetcher.fetch_external("https://x.com") + with pytest.raises(ExternalFetchError): await fetcher.fetch_external("https://x.com") - assert exc.value.parameter_name == "url" @pytest.mark.asyncio - async def test_custom_parameter_name_used(self): - fetcher = _make_fetcher() - with pytest.raises(InvalidToolCallParameterException) as exc: - await fetcher.fetch_external("files/b/x", parameter_name="target") - assert exc.value.parameter_name == "target" - - -class TestIsTextual: - @pytest.mark.parametrize( - "content_type", - [ - "text/plain", - "text/markdown; charset=utf-8", - "text/html", - "application/json", - "application/xml", - "application/vnd.api+json", - "image/svg+xml", - ], - ) - def test_textual(self, content_type): - assert WebContentFetcher.is_textual(content_type) is True - - @pytest.mark.parametrize( - "content_type", - [ - None, - "application/octet-stream", - "image/png", - "application/pdf", - "application/zip", - ], - ) - def test_non_textual(self, content_type): - assert WebContentFetcher.is_textual(content_type) is False - - -class TestDecode: - def test_utf8_default(self): - assert WebContentFetcher.decode("héllo".encode("utf-8"), "text/plain") == "héllo" - - def test_explicit_charset(self): - assert ( - WebContentFetcher.decode("héllo".encode("latin-1"), "text/plain; charset=latin-1") - == "héllo" - ) - - def test_unknown_charset_falls_back_to_utf8(self): - assert WebContentFetcher.decode(b"hi", "text/plain; charset=not-a-charset") == "hi" - - def test_invalid_bytes_replaced_not_raised(self): - # Lone continuation byte is invalid UTF-8; replace rather than raise. - assert WebContentFetcher.decode(b"\xff", "text/plain") == "�" + async def test_egress_disabled_propagates(self): + fetcher = _make_fetcher(ExternalFetchDisabledError(reason="admin", url="https://x.com")) + with pytest.raises(ExternalFetchDisabledError): + await fetcher.fetch_external("https://x.com") diff --git a/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py b/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py index 05936f59..b9d14522 100644 --- a/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py +++ b/src/tests/unit_tests/web_tooling/test_web_fetch_tool.py @@ -4,10 +4,16 @@ from aidial_client._exception import DialException, EtagMismatchError from quickapp.common import ToolCallResult -from quickapp.common.exceptions import InvalidToolCallParameterException +from quickapp.common.exceptions import InvalidToolCallParameterException, ToolErrorException from quickapp.dial_core_services.dial_file_service import DialFileService -from quickapp.shared.external_fetch.external_url_fetcher import FetchedBytes -from quickapp.shared.external_fetch.web_content_fetcher import WebContentFetcher +from quickapp.shared.external_fetch.external_url_fetcher import ( + ExternalFetchDisabledError, + FetchedBytes, +) +from quickapp.shared.external_fetch.web_content_fetcher import ( + WebContentFetcher, + WebContentFetchError, +) from quickapp.shared.home_path.home_path_resolver import HomePathResolver from quickapp.web_tooling._tool_configs import WEB_FETCH_TOOL_CONFIG from quickapp.web_tooling._truncation import compose_truncated_content, split_truncation_notice @@ -63,32 +69,41 @@ async def test_textual_within_guard_returns_inline_content(self): service.write_bytes.assert_not_awaited() @pytest.mark.asyncio - async def test_charset_respected(self): + async def test_utf8_content_returned_inline(self): fetched = FetchedBytes( - data="café".encode("latin-1"), - content_type="text/plain; charset=latin-1", - filename="a.txt", + data="café".encode("utf-8"), content_type="text/plain", filename="a.txt" ) tool, _ = _make_tool(fetched) result = await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a.txt") assert result.content == "café" + @pytest.mark.asyncio + async def test_non_utf8_text_rejected_as_binary(self): + # Text detection is a UTF-8 decode attempt; non-UTF-8 (e.g. latin-1) fails + # closed and is treated as a binary body. + fetched = FetchedBytes( + data="café".encode("latin-1"), content_type="text/plain", filename="a.txt" + ) + tool, _ = _make_tool(fetched) + with pytest.raises(ToolErrorException) as exc: + await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a.txt") + assert "save_path" in exc.value.error_message + @pytest.mark.asyncio async def test_binary_rejected_pointing_at_save_path(self): fetched = FetchedBytes(data=b"\x89PNG\r\n", content_type="image/png", filename="a.png") tool, _ = _make_tool(fetched) - with pytest.raises(InvalidToolCallParameterException) as exc: + with pytest.raises(ToolErrorException) as exc: await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a.png") - assert exc.value.parameter_name == "url" - assert "save_path" in exc.value.message + assert "save_path" in exc.value.error_message @pytest.mark.asyncio - async def test_missing_content_type_treated_as_binary(self): + async def test_missing_content_type_but_decodable_returned_inline(self): fetched = FetchedBytes(data=b"plain", content_type=None, filename="a") tool, _ = _make_tool(fetched) - with pytest.raises(InvalidToolCallParameterException) as exc: - await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a") - assert "save_path" in exc.value.message + result = await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/a") + assert result.content == "plain" + assert result.content_type == "text/plain" @pytest.mark.asyncio async def test_oversized_text_truncated_with_leading_notice(self): @@ -111,7 +126,7 @@ async def test_oversized_text_truncated_with_leading_notice(self): service.write_bytes.assert_not_awaited() @pytest.mark.asyncio - async def test_truncated_result_stays_below_cap_and_cuts_on_line_boundary(self): + async def test_truncated_result_stays_below_cap(self): # The whole result (notice + head) must stay strictly below the cap so it # never reaches the offload trigger (which fires at >= threshold). text = "\n".join(f"line-{i:03d}" for i in range(200)) @@ -124,7 +139,6 @@ async def test_truncated_result_stays_below_cap_and_cuts_on_line_boundary(self): notice, head = split_truncation_notice(result.content) assert notice is not None assert head # a realistic cap leaves room for content, not just the notice - assert head.split("\n")[-1].startswith("line-") # complete last line @pytest.mark.asyncio async def test_content_at_cap_truncated(self): @@ -138,11 +152,21 @@ async def test_content_at_cap_truncated(self): assert len(result.content.encode("utf-8")) < 500 @pytest.mark.asyncio - async def test_fetch_error_propagates(self): - tool, _ = _make_tool(InvalidToolCallParameterException("url", "egress disabled")) + async def test_scheme_error_mapped_to_parameter_error(self): + # A wrong-kind-of-URL domain error becomes a retryable parameter error. + tool, _ = _make_tool(WebContentFetchError("URL files/x is already in DIAL storage.")) with pytest.raises(InvalidToolCallParameterException) as exc: - await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com/x") - assert "egress disabled" in exc.value.message + await tool._run_in_stage_async(stage_wrapper=None, url="files/x") + assert exc.value.parameter_name == "url" + assert "DIAL storage" in exc.value.message + + @pytest.mark.asyncio + async def test_egress_failure_mapped_to_tool_error(self): + # The URL is valid; an egress failure is a tool error, not a bad parameter. + tool, _ = _make_tool(ExternalFetchDisabledError(reason="admin", url="https://x.com")) + with pytest.raises(ToolErrorException) as exc: + await tool._run_in_stage_async(stage_wrapper=None, url="https://x.com") + assert "EXTERNAL_URL_FETCH_ENABLED" in exc.value.error_message class TestSaveToWorkspace: