From ccd539fb33043ea9f71998e8202f1a326ac7d5e4 Mon Sep 17 00:00:00 2001 From: Kaede Akino Date: Fri, 28 Aug 2026 22:56:19 +0800 Subject: [PATCH] Let a webpage land in the scratch workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fetch returns a page through the conversation, bounded at 256 KB. So a page larger than that could not be read at all — the tail was truncated and there was no second call that would reach it — and a page you meant to refer back to was paid for twice, once to read and again to quote into a file. scratch_pull already solved this for hosts: it lands a remote file in the workspace and returns a size and a digest, which is what lifts sftp_read's inline ceiling. The network had no equivalent. scratch_fetch is that, and its bound is the workspace's own 8 MB per-file limit rather than what fits in a reply — so a large page arrives whole and is read a window at a time with scratch_read. A separate tool rather than a scratch_path on fetch, on the same grounds scratch_pull and scratch_push were split out of sftp_read and sftp_write. The flag would decide the tool's Safety — locallyReadOnly without it, locallyContained with it — and one descriptor meaning two things is harder for a model to choose correctly and harder to audit. fetch is unchanged, byte for byte, so nothing already in a conversation's history changes meaning. locallyContained is the sum of its halves: reading a page, public or private-network, is what fetch already does un-gated and never escalates for, and the landing site is storage the app owns. It therefore needs no approval and stays available while planning, which is right — staging the documentation you are planning against is planning. The result carries the title, size and digest and no page text; the title is enough to confirm the right page arrived. Both paths are proven in preflight, so a bad URL or a destination that escapes the scope costs nothing rather than a page fetched and dropped. An empty extraction fails instead of staging a zero-byte file the model would then try to read. 851 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- Termind/App/AppModel.swift | 9 +- .../Application/Agent/AgentScratchTools.swift | 144 +++++++++++++-- .../Application/Agent/AgentToolDetail.swift | 1 + .../AgentBuiltInToolAvailabilityTests.swift | 8 +- TermindTests/AgentScratchTests.swift | 170 ++++++++++++++++++ TermindTests/AgentToolPresentationTests.swift | 33 ++++ 6 files changed, 350 insertions(+), 15 deletions(-) diff --git a/Termind/App/AppModel.swift b/Termind/App/AppModel.swift index d172eff5..f221369d 100644 --- a/Termind/App/AppModel.swift +++ b/Termind/App/AppModel.swift @@ -378,19 +378,24 @@ final class AppModel { // being keyed by conversation, so one chat can never read what another // pulled off a host. let scratch = AgentScratchWorkspace(conversationID: conversation.id) + // One client for both, rather than two default-constructed ones: `fetch` + // and `scratch_fetch` read the same web the same way and differ only in + // where the page ends up. + let web = ScrubberWebClient() providers.append( AgentStaticToolProvider( AgentBuiltInTools.make( hosts: agentHosts, connections: agentConnections, authentication: agentAuthentication, - background: agentBackgroundCommands, search: searchPlan.local, + background: agentBackgroundCommands, web: web, + search: searchPlan.local, includesTerminalSuggestion: includesTerminalSuggestion ))) providers.append( AgentStaticToolProvider( AgentScratchTools.make( workspace: scratch, connections: agentConnections, - authentication: agentAuthentication + authentication: agentAuthentication, web: web ))) // Both write through the one ordered channel the rest of the run // uses, rather than reaching into the conversation directly: a tool diff --git a/Termind/Application/Agent/AgentScratchTools.swift b/Termind/Application/Agent/AgentScratchTools.swift index 9521666b..fb175ec3 100644 --- a/Termind/Application/Agent/AgentScratchTools.swift +++ b/Termind/Application/Agent/AgentScratchTools.swift @@ -7,14 +7,15 @@ import Foundation /// the user's disk. That containment is what earns `.locallyContained` on the three /// that write: they change something, but only storage the app owns. /// -/// `scratch_pull` and `scratch_push` are the exception and the reason the rest is -/// worth having. They move bytes between a host and the workspace *without the -/// bytes passing through the conversation*, which is what lifts `sftp_read`'s 128 KB -/// inline ceiling and stops an edit costing two copies of the file in tokens. They -/// are kept separate from `sftp_read` / `sftp_write` rather than folded in as -/// options: a tool that means two different things depending on which optional -/// argument is present is harder for a model to choose correctly and harder for a -/// reader to audit, and only one of the two directions needs approval. +/// The three that reach outside — `scratch_pull`, `scratch_push` and +/// `scratch_fetch` — are the exception and the reason the rest is worth having. +/// They move bytes between a host or the web and the workspace *without the bytes +/// passing through the conversation*, which is what lifts `sftp_read`'s and +/// `fetch`'s inline ceilings and stops an edit costing two copies of the file in +/// tokens. They are kept separate from `sftp_read` / `sftp_write` / `fetch` rather +/// than folded in as options: a tool that means two different things depending on +/// which optional argument is present is harder for a model to choose correctly and +/// harder for a reader to audit, and only one of the three needs approval. /// /// Everything except `scratch_push` is `.parallel`. The workspace is an actor, so it /// is already the per-path mutation queue Pi builds explicitly — a read and a write @@ -22,12 +23,14 @@ import Foundation /// taking the whole turn serial. nonisolated enum AgentScratchTools: AgentToolSchemaBuilding { /// Connection and authentication services travel together. If either is - /// absent no transfer tools are registered, rather than exposing a path - /// that fails on first use or bypasses Agent credential interaction. + /// absent no host transfer tools are registered, rather than exposing a path + /// that fails on first use or bypasses Agent credential interaction. `web` + /// governs `scratch_fetch` on the same terms. static func make( workspace: AgentScratchWorkspace, connections: AgentConnectionPool? = nil, - authentication: AgentSSHAuthenticationRegistry? = nil + authentication: AgentSSHAuthenticationRegistry? = nil, + web: ScrubberWebClient? = nil ) -> [AnyAgentTool] { let transfers: [AnyAgentTool] if let connections, let authentication { @@ -41,7 +44,7 @@ nonisolated enum AgentScratchTools: AgentToolSchemaBuilding { return [ list(workspace), read(workspace), write(workspace), edit(workspace), diff(workspace), delete(workspace), - ] + transfers + ] + transfers + (web.map { [fetch(workspace, $0)] } ?? []) } private static let pathBytes = AgentScratchWorkspace.Limits.pathBytes @@ -447,6 +450,84 @@ nonisolated enum AgentScratchTools: AgentToolSchemaBuilding { ) } + /// Network → workspace. + /// + /// `.locallyContained`, which is the sum of its two halves on exactly the terms + /// `scratch_pull` argues: reading a page — public or private-network — is what + /// `fetch` already does un-gated and never escalates for, and the landing site is + /// storage the app owns. So it stays available while planning, which is right: + /// staging the documentation you are planning against is planning. + /// + /// The point of it is the ceiling. `fetch` bounds what it returns at 256 KB + /// because that is what crosses the context window; here nothing does, so the + /// bound becomes the workspace's own 8 MB per-file limit and a page far larger + /// than `fetch` can show lands whole, to be read a window at a time. + private static func fetch( + _ workspace: AgentScratchWorkspace, _ web: ScrubberWebClient + ) -> AnyAgentTool { + let tool = descriptor( + "scratch_fetch", + """ + Read an HTTP(S) webpage, including private-network pages, straight into the \ + scratch workspace. Returns its title, size and digest only — the page does not \ + enter this conversation, and there is no inline size ceiling, so a page far \ + larger than fetch can return lands whole. Prefer fetch to read a page once; \ + prefer this when the page is large, when you will refer back to it, or when it \ + is going to a host. Markdown by default, or the original HTML when format is \ + original. + """, + properties: [ + "url": string(max: 8_192), + "scratch_path": string(max: pathBytes), + "format": enumeration(["markdown", "original"]), + ], + required: ["url", "scratch_path"], target: .network, + safety: .locallyContained, concurrency: .parallel, + presentation: .init( + symbol: "globe.badge.chevron.backward", + activity: .semanticArgument(key: "url", fallback: .url), + output: .json, detailPresenterID: "builtin.scratch_fetch", + actionKind: .fetch + ) + ) + return AnyAgentTool( + descriptor: tool, + preflight: { invocation in + let arguments = try Arguments(invocation) + _ = try ScrubberWebClient.validatedURL(arguments.string("url")) + // Proven here so a malformed destination costs nothing, rather than a + // page fetched over the network and then dropped on the floor. + _ = try AgentScratchWorkspace.normalize(arguments.string("scratch_path")) + return AgentToolPreflight(invocation: invocation, safety: .locallyContained) + }, + execute: { invocation, _ in + let arguments = try Arguments(invocation) + let document = try await web.document(for: arguments.string("url")) + let format = arguments.optionalString("format") ?? "markdown" + let selected = format == "original" ? document.original : document.markdown + // An empty representation means extraction failed, not that the page is + // empty. Saying so beats spending one of the workspace's entries on a + // zero-byte file the model will then try to read. + guard !selected.isEmpty else { throw ScrubberWebError.emptyDocument } + let written = try await workspace.write( + arguments.string("scratch_path"), data: Data(selected.utf8) + ) + return result(invocation, .object([ + "url": .string(document.url.absoluteString), + "title": .string(document.title), + "format": .string(format), + "scratch_path": .string(written.path), + "bytes": .number(Double(written.bytes)), + "sha256": .string(written.sha256), + "lines": .number(Double( + UnifiedDiff.split(LineEnding.normalizedToLF(selected)).lines.count + )), + "total_bytes": .number(Double(written.usage.bytes)), + ])) + } + ) + } + /// What the approval card and Security Review see in place of the content. /// /// Without this, `scratch_push` would be an approval that showed a filename where @@ -514,6 +595,9 @@ nonisolated extension AgentScratchTools { static let pushDetailPresenter = AgentToolDetailPresenter( id: "builtin.scratch_push", present: presentPushDetail ) + static let fetchDetailPresenter = AgentToolDetailPresenter( + id: "builtin.scratch_fetch", present: presentFetchDetail + ) private static func presentListDetail( _ input: AgentToolDetailInput @@ -730,4 +814,40 @@ nonisolated extension AgentScratchTools { items.append(F.field("Result", F.localized("Written", locale: input.locale), locale: input.locale)) return items } + + /// The page's identity and where it landed, since its content is deliberately + /// not in the result. `presentPullDetail` with a URL in place of a host path. + private static func presentFetchDetail( + _ input: AgentToolDetailInput + ) -> [AgentToolDetail.Item] { + typealias F = AgentToolDetailFormatting + guard let object = input.result.objectValue else { + return F.genericItems(input.result, locale: input.locale) + } + var items: [AgentToolDetail.Item] = [] + if let title = F.nonempty(object["title"]?.stringValue) { + items.append(F.field("Title", title, locale: input.locale)) + } + if let url = F.nonempty(object["url"]?.stringValue) { + items.append(.field(.init( + label: F.localized("URL", locale: input.locale), value: url, + isMonospaced: true, url: url + ))) + } + F.appendField(&items, "Scratch path", object["scratch_path"]?.stringValue, locale: input.locale, monospaced: true) + items.append(F.field( + "Content type", + object["format"]?.stringValue == "original" + ? F.localized("Original HTML", locale: input.locale) + : F.localized("Markdown", locale: input.locale), + locale: input.locale + )) + F.appendBytes(&items, "Size", object["bytes"], locale: input.locale) + if let lines = object["lines"]?.integerValue { + items.append(F.field("Lines", String(lines), locale: input.locale)) + } + F.appendField(&items, "SHA-256", object["sha256"]?.stringValue, locale: input.locale, monospaced: true) + F.appendBytes(&items, "Workspace usage", object["total_bytes"], locale: input.locale) + return items + } } diff --git a/Termind/Application/Agent/AgentToolDetail.swift b/Termind/Application/Agent/AgentToolDetail.swift index 0cd1b9d9..94d2e8ae 100644 --- a/Termind/Application/Agent/AgentToolDetail.swift +++ b/Termind/Application/Agent/AgentToolDetail.swift @@ -116,6 +116,7 @@ nonisolated enum AgentBuiltInToolDetailPresenters { AgentScratchTools.deleteDetailPresenter, AgentScratchTools.pullDetailPresenter, AgentScratchTools.pushDetailPresenter, + AgentScratchTools.fetchDetailPresenter, AgentPlanTools.presentPlanDetailPresenter, AgentPlanTools.manageTasksDetailPresenter, ] diff --git a/TermindTests/AgentBuiltInToolAvailabilityTests.swift b/TermindTests/AgentBuiltInToolAvailabilityTests.swift index 31a9a4ce..ea719cb6 100644 --- a/TermindTests/AgentBuiltInToolAvailabilityTests.swift +++ b/TermindTests/AgentBuiltInToolAvailabilityTests.swift @@ -50,7 +50,13 @@ struct AgentBuiltInToolAvailabilityTests { let workspace = AgentScratchWorkspace( conversationID: UUID(), base: FileManager.default.temporaryDirectory ) - let scratch = AgentScratchTools.make(workspace: workspace) + let scratch = AgentScratchTools.make( + workspace: workspace, + web: ScrubberWebClient( + document: { _ in throw ScrubberWebError.emptyDocument }, + search: { _, _ in [] } + ) + ) let planning = AgentPlanTools.make( workspace: workspace, tasks: AgentTaskList { _ in }, plans: AgentPlanRecorder { _ in } diff --git a/TermindTests/AgentScratchTests.swift b/TermindTests/AgentScratchTests.swift index da9e73d6..819a65a1 100644 --- a/TermindTests/AgentScratchTests.swift +++ b/TermindTests/AgentScratchTests.swift @@ -460,6 +460,31 @@ struct AgentScratchTests { transfer.first { $0.descriptor.name == "scratch_push" }?.descriptor.concurrency == .sequential ) + + // Same terms again for the web: absent without a client, and when present a + // network read landing in contained storage — no approval, may overlap. + #expect(!local.contains { $0.descriptor.name == "scratch_fetch" }) + let withWeb = AgentScratchTools.make(workspace: workspace, web: Fixture.web()) + let fetch = try #require(withWeb.first { $0.descriptor.name == "scratch_fetch" }) + #expect(fetch.descriptor.target == .network) + #expect(fetch.descriptor.safety == .locallyContained) + #expect(fetch.descriptor.concurrency == .parallel) + } + + /// `fetch` is a separate tool, not a new mode of `fetch`. The same promise + /// `theSFTPToolsAreUnchanged` makes about `sftp_read` / `sftp_write`. + @Test @MainActor + func theFetchToolIsUnchanged() { + let tools = AgentBuiltInTools.make( + hosts: AgentHostResolver(store: HostLibraryModel()), + connections: Fixture.pool(StubTransferSFTPClient()) + ) + let fetch = tools.first { $0.descriptor.name == "fetch" }?.descriptor + let properties = fetch?.inputSchema.objectValue?["properties"]?.objectValue ?? [:] + #expect(properties["scratch_path"] == nil) + #expect(properties.keys.sorted() == ["format", "url"]) + #expect(fetch?.safety == .locallyReadOnly) + #expect(!tools.contains { $0.descriptor.name.hasPrefix("scratch_") }) } /// The built-in SFTP tools were deliberately left alone: `scratch_pull` and @@ -637,6 +662,139 @@ struct AgentScratchTests { ) #expect(await client.writes["/opt/copy.bin"] == binary) } + + // MARK: - Web + + @Test + func fetchLandsThePageWithoutPuttingItInTheResult() async throws { + let workspace = Fixture.workspace() + let page = (1...40).map { "line \($0)" }.joined(separator: "\n") + "\n" + let tool = try #require( + AgentScratchTools.make(workspace: workspace, web: Fixture.web(markdown: page)) + .first { $0.descriptor.name == "scratch_fetch" } + ) + + let result = try await tool.execute( + Fixture.invocation("scratch_fetch", [ + "url": .string("https://example.com/docs"), + "scratch_path": .string("docs/page.md"), + ]), + context: Fixture.context() + ) + + #expect(!result.isError) + #expect(!result.content.contains("line 1")) + #expect(!result.content.contains("raw-marker")) + #expect(result.content.contains("\"lines\":40")) + #expect(result.content.contains("\"title\":\"Example\"")) + #expect(result.content.contains(AgentScratchWorkspace.digest(Data(page.utf8)))) + // The bytes are in the workspace, byte for byte. + #expect(try await workspace.data(at: "docs/page.md").data == Data(page.utf8)) + } + + /// The reason the tool exists. `fetch` truncates at 256 KB because that is what + /// crosses the context window; nothing does here, so the page arrives whole. + @Test + func fetchIsNotBoundedByTheInlineCeilingThatBoundsTheResultOfFetch() async throws { + let workspace = Fixture.workspace() + let oversized = String(repeating: "x", count: 256 * 1_024 + 100) + let tool = try #require( + AgentScratchTools.make(workspace: workspace, web: Fixture.web(markdown: oversized)) + .first { $0.descriptor.name == "scratch_fetch" } + ) + + let result = try await tool.execute( + Fixture.invocation("scratch_fetch", [ + "url": .string("https://example.com/big"), "scratch_path": .string("big.md"), + ]), + context: Fixture.context() + ) + + #expect(!result.isTruncated) + #expect(result.content.contains("\"bytes\":\(oversized.utf8.count)")) + #expect(try await workspace.data(at: "big.md").bytes == oversized.utf8.count) + } + + @Test + func fetchWritesTheOriginalHTMLWhenAsked() async throws { + let workspace = Fixture.workspace() + let tool = try #require( + AgentScratchTools.make(workspace: workspace, web: Fixture.web()) + .first { $0.descriptor.name == "scratch_fetch" } + ) + + let result = try await tool.execute( + Fixture.invocation("scratch_fetch", [ + "url": .string("https://example.com/"), "scratch_path": .string("page.html"), + "format": .string("original"), + ]), + context: Fixture.context() + ) + + #expect(result.content.contains("\"format\":\"original\"")) + #expect(!result.content.contains("raw-marker")) + let staged = try await workspace.data(at: "page.html") + #expect(String(data: staged.data, encoding: .utf8)?.contains("raw-marker") == true) + } + + @Test + func fetchValidatesTheURLAndTheDestinationBeforeReachingTheNetwork() async throws { + let loaded = Counter() + let web = ScrubberWebClient( + document: { url in + await loaded.increment() + return ScrubbedWebDocument( + title: "Example", url: url, markdown: "# Example", original: "" + ) + }, + search: { _, _ in [] } + ) + let tool = try #require( + AgentScratchTools.make(workspace: Fixture.workspace(), web: web) + .first { $0.descriptor.name == "scratch_fetch" } + ) + + for arguments in [ + ["url": "file:///etc/passwd", "scratch_path": "leak.txt"], + ["url": "https://user:pass@example.com/", "scratch_path": "creds.md"], + ["url": "https://example.com/", "scratch_path": "../escape.md"], + ["url": "https://example.com/", "scratch_path": "/etc/passwd"], + ] { + await #expect(throws: (any Error).self) { + try await tool.preflight(Fixture.invocation( + "scratch_fetch", arguments.mapValues(AgentJSONValue.string) + )) + } + } + #expect(await loaded.count == 0) + } + + /// An empty representation means extraction failed, not that the page is empty. + @Test + func fetchRefusesToStageAnEmptyExtraction() async throws { + let workspace = Fixture.workspace() + let tool = try #require( + AgentScratchTools.make(workspace: workspace, web: Fixture.web(markdown: "")) + .first { $0.descriptor.name == "scratch_fetch" } + ) + + await #expect(throws: (any Error).self) { + try await tool.execute( + Fixture.invocation("scratch_fetch", [ + "url": .string("https://example.com/js-only"), + "scratch_path": .string("empty.md"), + ]), + context: Fixture.context() + ) + } + #expect(try await workspace.usage().entries == 0) + } +} + +/// Counts calls from a `@Sendable` closure without a data race. +private actor Counter { + private(set) var count = 0 + func increment() { count += 1 } } // MARK: - Fixtures @@ -670,6 +828,18 @@ private enum Fixture { ) } + static func web(markdown: String = "# Example") -> ScrubberWebClient { + ScrubberWebClient( + document: { url in + ScrubbedWebDocument( + title: "Example", url: url, markdown: markdown, + original: "raw-marker" + ) + }, + search: { _, _ in [] } + ) + } + @MainActor static func authentication() -> AgentSSHAuthenticationRegistry { AgentSSHAuthenticationRegistry( diff --git a/TermindTests/AgentToolPresentationTests.swift b/TermindTests/AgentToolPresentationTests.swift index c30c55ce..0b8542d0 100644 --- a/TermindTests/AgentToolPresentationTests.swift +++ b/TermindTests/AgentToolPresentationTests.swift @@ -630,6 +630,39 @@ struct AgentToolPresentationTests { } } + /// Separate from the table above, which builds cards without a presentation and + /// so resolves through `legacyPresenter`. `scratch_fetch` has never shipped, so + /// it deliberately has no legacy name and is reached only by presenter ID. + @Test + func scratchFetchDetailIsStructuredWithoutJSONSyntax() throws { + var card = makeCard( + name: "scratch_fetch", target: .network, + arguments: ["url": .string("https://example.com/docs")], + presentation: .init( + symbol: "globe.badge.chevron.backward", + activity: .semanticArgument(key: "url", fallback: .url), + output: .json, detailPresenterID: "builtin.scratch_fetch", + actionKind: .fetch + ) + ) + card.result = AgentToolResult( + callID: "call", + content: AgentJSONValue.object([ + "url": .string("https://example.com/docs"), "title": .string("Docs"), + "format": .string("markdown"), "scratch_path": .string("docs.md"), + "bytes": .number(4), "sha256": .string("abcd"), "lines": .number(1), + "total_bytes": .number(4), + ]).encodedString + ) + + let detail = try #require(AgentToolPresentation.detail(for: card)) + let rendered = String(describing: detail) + #expect(!rendered.contains("{")) + #expect(!rendered.contains("}")) + #expect(rendered.contains("docs.md")) + #expect(rendered.contains("Docs")) + } + @Test func finishedBatchSummaryCountsRepeatedActivitiesAndPreservesFailures() { let questionPresentation = AgentBuiltInTools.requestUserInput().descriptor.presentation