diff --git a/docs/designs/web_fetch_tool.md b/docs/designs/web_fetch_tool.md new file mode 100644 index 00000000..3a0aaa6d --- /dev/null +++ b/docs/designs/web_fetch_tool.md @@ -0,0 +1,326 @@ +# Design: Built-in Web Fetch Tool + +- **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 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 + +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.** 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. + +--- + +## One tool, one switch (core design decision) + +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: + +| | `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 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` | +| **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`. + +**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: + +- **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 (default) + +**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. + +**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_web_fetch(url="https://…/data.py", save_path="analysis/data.py")`. + +**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). + +**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. + +### 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. + +**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-4: Agent encounters non-text / binary content + +**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). + +**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-5: External egress is disabled (admin cap or per-app opt-out) + +**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.) + +**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 + +**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 tool stays focused on *external* retrieval; in-workspace reads remain the job of `internal_file_read_lines` / `internal_file_search`. + +### UC-7: App enables or disables the whole capability + +**Trigger:** An app sets `features.web_fetch.enabled=true` (or omits it). + +**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:** "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.) + +--- + +## Proposed Design + +```mermaid +flowchart TD + WF["internal_web_fetch(url, save_path)"] --> B{"classify_url"} + + subgraph shared["shared fetch helper (Component 3)"] + direction TB + 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 + + 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 -->|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` (the single tool) + +- **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: Inline size guard (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). +- **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 (`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`. +- **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 (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), 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`) — 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 + +The tool is **feature-gated** (enabled through `features.web_fetch`, not through a per-tool tool-set entry) and **preview-gated**: + +- **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 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. + +--- + +## 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. + +--- + +## Out of Scope + +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. +- **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 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 the tool + +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). + web_fetch: + enabled: true + max_inline_size: 40000 +``` + +> `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 `save_path`). No file written. + +### Walkthrough — save to the workspace (UC-2) + +`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-5) + +`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."* + +--- + +## Migration + +### Breaking changes + +None. The tool is purely additive and opt-in via app config. + +### Non-breaking changes + +- 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 + +> **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`; 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 (vs. `development`) + +- `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`. + +### 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). + +### 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). + +### 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:** 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:** 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 == []