From 5e083d8142f925d15a724bf42de0e5533162f877 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:13:15 +0800 Subject: [PATCH 01/11] feat: snip a region into chat, and make links clickable Two changes: screenshot input gated on the model really reporting vision, and a fix for URLs that arrived as code. Links - A fence whose body is all URLs, a lone `https://...` inline span, and a line that is only a URL are rewritten into [url](url) markdown links. Vicinae attaches only the table and strikethrough GFM extensions, so autolink is off and a bare URL is dead text; clickability comes from MarkdownModel turning CMARK_NODE_LINK into an . A fenced URL was a dark panel with a Copy button and no way to open it. - promoteStandaloneCodeSpans was itself promoting a lone URL span into a fence, so it made this worse. Link restoration now runs first. - The default prompt says never to fence a URL. - Anything that is not one of those three shapes is copied through, so a curl invocation containing an address is never rewritten. Screenshots - capture.ts shells out to an auto-detected tool: grimblast, grim + slurp, spectacle, maim, scrot, flameshot, or screencapture on macOS. There is no screenshot call in @vicinae/api and Clipboard moves no binary, so the extension owns the capture and reads the file it wrote rather than guessing at the clipboard. Cancel is detected by whether an image landed on disk, because the tools disagree on exit codes. - In chat: "Snip a Region" in the action panel (cmd+shift+S) attaches to the current conversation. Standalone: "Snip to Lam" starts a new one. - vision.ts caches the /api/show capability answer per host and model. A failed check is "unknown", never "no", so a dropped connection is not reported as a fact about the user's model. Actions still fail closed. - Only the path is stored, base64 is read at send time, and the image goes on its own turn only. Capture is never a model-callable tool. Verified: 244 tests, tsc, eslint, and vici build (both entrypoints). --- README.md | 47 +++++- package.json | 7 + src/__tests__/attachments.test.ts | 64 ++++++++ src/__tests__/capture.test.ts | 231 +++++++++++++++++++++++++++ src/__tests__/code.test.ts | 120 ++++++++++++++ src/__tests__/history.test.ts | 36 ++++- src/__tests__/markdown.test.ts | 79 ++++++++- src/__tests__/ollama.test.ts | 71 ++++++++- src/__tests__/sessions.test.ts | 1 + src/__tests__/thread.test.ts | 1 + src/__tests__/vision.test.ts | 121 ++++++++++++++ src/lib/attachments.ts | 38 +++++ src/lib/capture.ts | 255 ++++++++++++++++++++++++++++++ src/lib/code.ts | 137 +++++++++++++++- src/lib/components/ChatView.tsx | 179 +++++++++++++++++++-- src/lib/constants.ts | 2 + src/lib/history.ts | 16 +- src/lib/hooks/useChat.ts | 13 +- src/lib/hooks/useSnip.ts | 61 +++++++ src/lib/hooks/useVision.ts | 73 +++++++++ src/lib/markdown.ts | 12 +- src/lib/ollama.ts | 38 +++++ src/lib/schemas.ts | 8 + src/lib/sessions.ts | 10 +- src/lib/thread.ts | 3 + src/lib/types.ts | 40 +++++ src/lib/vision.ts | 75 +++++++++ src/setup.tsx | 27 ++++ src/snip.tsx | 18 +++ 29 files changed, 1744 insertions(+), 39 deletions(-) create mode 100644 src/__tests__/attachments.test.ts create mode 100644 src/__tests__/capture.test.ts create mode 100644 src/__tests__/vision.test.ts create mode 100644 src/lib/attachments.ts create mode 100644 src/lib/capture.ts create mode 100644 src/lib/hooks/useSnip.ts create mode 100644 src/lib/hooks/useVision.ts create mode 100644 src/lib/vision.ts create mode 100644 src/snip.tsx diff --git a/README.md b/README.md index 3d72355..6449dc6 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ Agentic AI chat inside the [Vicinae](https://vicinae.com) launcher, powered by [ Hit a global shortcut, type a question, get a streamed answer without leaving whatever you were doing. -> **Status:** early. Chat, streaming, persistent sessions and the web tools work. Anything that *acts* on your machine (opening links, clipboard, launching apps) is deliberately not here yet. See [Roadmap](#roadmap). +> **Status:** early. Chat, streaming, persistent sessions, the web tools and screen capture work. Anything else that *acts* on your machine (opening links, clipboard, launching apps) is deliberately not here yet. See [Roadmap](#roadmap). --- @@ -16,15 +16,29 @@ Hit a global shortcut, type a question, get a streamed answer without leaving wh - **Persistent conversations.** Every chat is saved locally. The sidebar lists them by recency, and reopening the command picks up where you left off, unless it has been idle longer than you allow. - **Readable code.** Fenced blocks render in a dark panel with the language named in a header, syntax colouring, and a Copy button. - **Tools.** Lam decides when to call them: current time, location context, web search, and page fetching. -- **Chat only.** Lam answers questions. It does not touch your files or run commands. +- **Screenshots, two ways.** Snip a region into the conversation you are already in from the action panel, or run **Snip to Lam** to start a new chat about it. Neither is offered unless the current model can actually see images. +- **Chat only.** Lam answers questions. It does not touch your files or run commands. The one exception is capture, and it only ever happens because you pressed the key for it. ## Requirements - Vicinae with the extension runtime enabled (Node.js 20+ on `PATH`) - An [Ollama](https://ollama.com) account and API key, which you can [create here](https://ollama.com/settings/keys) +- A screenshot tool, if you want to snip. A vision model, if you want Lam to read the result Ollama Cloud is the default. A local Ollama server works too: point **Ollama Host** at `http://localhost:11434`. Note that the web tools are a cloud feature and are not available from a local server. +### Snipping needs a capture tool + +Vicinae has no screenshot API, so the extension shells out. It auto-detects the first of these that is installed, in this order: + +| Platform | Tools, in preference order | +| --- | --- | +| Linux (Wayland) | `grimblast`, then `grim` + `slurp`, then `spectacle` | +| Linux (X11) | `maim` + `slop`, then `scrot`, then `flameshot` | +| macOS | `screencapture`, which ships with the system | + +If none is found, the chat says so and names what to install. Nothing to configure: the detection happens per platform at capture time. + ## Install ```sh @@ -57,6 +71,7 @@ To change the model later, either edit the **Model** preference directly, or run | `↩` | Send the message in the search bar | | `↩` with an empty bar | Open the highlighted conversation | | `⌘ N` | New chat | +| `⌘ ⇧ S` | Snip a region and attach it to this chat (only with a vision model) | | `⌘ ⇧ ⌫` | Delete the highlighted conversation (also in **Chat Sessions**) | Prompts are single line. An earlier `⌘ ↩` multi-line composer was removed. See [Architecture](#architecture) for why a `Form.TextArea` cannot be laid out cleanly in a launcher. The `Tab` shortcut went with it: the left column *is* the conversation switcher now, so an accessory dropdown would only duplicate it. @@ -67,14 +82,17 @@ Assign these in **Vicinae Settings → Extensions → Lam Agent**, or bind them ```sh vicinae 'vicinae://launch/lam-agent/chat?toggle=true' +vicinae 'vicinae://launch/lam-agent/snip' ``` ```ini # Hyprland bind = $mainAlt, Space, exec, vicinae 'vicinae://launch/lam-agent/chat?toggle=true' +bind = $mainAlt, S, exec, vicinae 'vicinae://launch/lam-agent/snip' # niri Mod+Shift+Space repeat=false { spawn "vicinae" "vicinae://launch/lam-agent/chat?toggle=true"; } +Mod+Shift+S repeat=false { spawn "vicinae" "vicinae://launch/lam-agent/snip"; } ``` `fallbackText` pre-fills the prompt, which makes a handy "ask about this" command: @@ -88,8 +106,9 @@ vicinae 'vicinae://launch/lam-agent/chat?fallbackText=summarise%20this' | Command | Purpose | | --- | --- | | **Chat with Lam** | The chat surface. Bind this to your global shortcut. | +| **Snip to Lam** | Capture a region and start a new chat about it. Bind this to a second shortcut for one-key "snip and ask". | | **Chat Sessions** | Browse, reopen, and delete past conversations. | -| **Set Up Lam Agent** | Verify the connection and list available models. | +| **Set Up Lam Agent** | Verify the connection, list available models, and check whether the selected model can see images. | ## Configuration @@ -107,7 +126,7 @@ vicinae 'vicinae://launch/lam-agent/chat?fallbackText=summarise%20this' ## Tools -Lam calls these on its own when they would improve an answer. All four are read-only. +Lam calls these on its own when they would improve an answer. All four are read-only, and all four are the entire list of things Lam is allowed to do without being asked twice. Screen capture is deliberately not among them. | Tool | What it does | | --- | --- | @@ -125,19 +144,26 @@ Web search results and fetched pages can be large. Lam truncates them before the ``` src/ chat.tsx # "chat" command entrypoint, thin wrapper + snip.tsx # "snip" command entrypoint, captures then chats sessions.tsx # browse, reopen, delete conversations setup.tsx # connection check + live model list lib/ ollama.ts # the only file that talks HTTP agent.ts # the tool-calling loop tools.ts # tool schemas + dispatch + capture.ts # screen capture backends, shelled out + attachments.ts # image files -> base64 payloads + vision.ts # cached "can this model see?" answer sessions.ts # LocalStorage persistence history.ts # turns -> API messages location.ts # time/place, derived locally thread.ts # turns -> flat message list markdown.ts # messages -> detail-panel markdown + code.ts # code fences and links, repaired preferences.ts # preferences, normalised hooks/useChat.ts # the chat state machine + hooks/useSnip.ts # capture -> pending attachment + hooks/useVision.ts # capability check, for gating snip actions components/ChatView.tsx # the chat surface ``` @@ -161,12 +187,22 @@ A few decisions worth knowing about before you change things: **Code fences are labelled so Vicinae can colour them.** `MdCodeBlock.qml` already provides the dark panel, a header naming the language, and a Copy button, so none of that is built here. The language string is what turns on highlighting: `syntax::highlight` resolves it with `definitionForName(language)` and then `definitionForFileName("file." + language)`. So `bash` and `ts` colour correctly, while an unrecognised string such as `terminal` still renders a label but produces no highlighting at all, so a wrong label is worse than none. `code.ts` therefore labels bare fences conservatively, only when a signal is clear, and the default prompt asks for a language on every fence. +**A URL is only clickable as a markdown link, so `code.ts` takes them back out of code.** Vicinae's parser attaches the `table` and `strikethrough` GFM extensions and nothing else, so the `autolink` extension is off: a bare URL is plain dead text. Clickability comes from `MarkdownModel` turning `CMARK_NODE_LINK` into an ``, which the selection controller opens on release. A URL the model fenced is therefore the worst possible presentation, a dark panel with a Copy button and no way to open the address. `restoreUrlLinks` recovers three narrow shapes (a fence whose entire body is URLs, a lone `` `https://...` `` span, and a line that is only a URL) into `[url](url)` links, and the default prompt now says never to fence a URL in the first place. Anything that is not one of those three shapes is copied through, so a `curl` invocation containing an address is never rewritten. + **Streamed deltas are buffered in refs and flushed to state on a timer** (`FLUSH_INTERVAL_MS`). Re-rendering the transcript per token makes the launcher visibly janky. **The logic layer has no Vicinae imports.** `agent.ts`, `ollama.ts`, `tools.ts` and friends are plain TypeScript, which is why they unit test without a runtime. A test enforces this. **Sessions are capped twice over**, once per turn and once per session, because `LocalStorage` has no documented ceiling. +**Screen capture is owned, not borrowed.** There is no screenshot call anywhere in `@vicinae/api`, and `Clipboard` moves text, html, urls and a file path and nothing binary, so a bitmap can never be read back out of the clipboard either. The only way to get a region of the screen is to shell out, which is fair game because extensions run under Node. The useful consequence is that the extension picks the output path and reads the file it wrote, rather than guessing what some other application left on the clipboard. Cancel detection leans on the file for the same reason: every tool reports an abandoned selection differently, and at least one exits `0` on cancel, so whether an image actually landed on disk is the only signal they agree on. Snips go in the extension's support directory rather than a temp directory, because a stored conversation keeps the path and a path that survives a reboot is the difference between an old chat showing its screenshot and pointing at nothing. + +**Image support is a fact from the API, not a guess from the model name, and it is cached.** `GET /api/tags` returns names, sizes and quantization levels and says nothing about capability, so the check is a `POST /api/show` that reads the `vision` flag. That is a round trip per model, so the answer is persisted under a key built from the host and model name, which means switching model invalidates it by construction. The state is a tri-state rather than a boolean: a failed request is `"unknown"`, never `"no"`, because telling someone their model cannot see when the network merely dropped is a false claim about their setup that sends them shopping for a model they do not need. The actions still fail closed, so only a confirmed `"yes"` offers a snip. + +**The capture is never a tool.** It stays in the action panel and on a keybinding, so it only ever runs because a person asked for it. A model that could decide to screenshot the screen is a different risk profile from one that answers questions, which is the same line the read-only tools already sit behind. + +**An attached image is sent once and never replayed.** It is the largest thing a request can carry, so `buildMessages` attaches it to the question it belongs to and drops it afterwards, exactly as it already drops tool output. The cost is that a follow-up about a screenshot needs the image attached again; the alternative is re-sending every screenshot in the conversation on every turn. Only the path and mime type go into storage: `MAX_STORED_CHARS_PER_TURN` exists so one turn cannot dominate `LocalStorage`, and base64 would blow past it on a single 1080p capture. + ## Development ```sh @@ -183,7 +219,8 @@ npm test - **Actions.** Opening links, clipboard access, launching apps. These need an approval step first. An assistant that can silently act on your machine is a different risk profile from one that answers questions, and this release does not pretend otherwise. - **`@` mentions** for files and clipboard content. - **Per-conversation model** overrides. -- **Image input**, which Ollama's chat API already supports. +- **Annotations on a snip.** The capture backends are there, but drawing on the result before sending it needs a second tool and a wait for the editor to save. +- **Follow-ups that keep the image.** Replaying an attachment is a token cost that has to be made opt-in first. ## License diff --git a/package.json b/package.json index c5d3934..b841446 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,13 @@ "description": "Ask Lam anything. Answers stream in as they are generated, your session history is kept, and Lam can search the web when it needs current information.", "mode": "view" }, + { + "name": "snip", + "title": "Snip to Lam", + "subtitle": "Lam Agent", + "description": "Capture a region of the screen and start a new chat about it. Offered only when the model in preferences supports image input, so check with \"Set Up Lam Agent\" if the shortcut seems to do nothing.", + "mode": "view" + }, { "name": "sessions", "title": "Chat Sessions", diff --git a/src/__tests__/attachments.test.ts b/src/__tests__/attachments.test.ts new file mode 100644 index 0000000..98270f7 --- /dev/null +++ b/src/__tests__/attachments.test.ts @@ -0,0 +1,64 @@ +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { mimeForPath, readAttachment } from "../lib/attachments"; + +const tempDirs: string[] = []; + +async function tempDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), "lam-attach-")); + tempDirs.push(dir); + return dir; +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +describe("mimeForPath", () => { + it("names the image types Ollama accepts", () => { + expect(mimeForPath("/tmp/snip.png")).toBe("image/png"); + expect(mimeForPath("/tmp/photo.JPG")).toBe("image/jpeg"); + expect(mimeForPath("/tmp/photo.jpeg")).toBe("image/jpeg"); + expect(mimeForPath("/tmp/shot.webp")).toBe("image/webp"); + }); + + it("does not guess a type for anything else", () => { + expect(mimeForPath("/tmp/notes.txt")).toBe("application/octet-stream"); + expect(mimeForPath("/tmp/noextension")).toBe("application/octet-stream"); + }); +}); + +describe("readAttachment", () => { + it("reads the bytes as bare base64, with no data URL prefix", async () => { + const dir = await tempDir(); + const path = join(dir, "snip.png"); + await writeFile(path, Buffer.from([0x89, 0x50, 0x4e, 0x47])); + + const attachment = await readAttachment(path); + + // Ollama wants the payload bare. A `data:image/png;base64,` prefix makes it + // reject the image, and the mistake is invisible until a request comes back + // with an error that says nothing about encoding. + expect(attachment.base64).toBe("iVBORw=="); + expect(attachment.base64).not.toContain("data:"); + expect(attachment.mime).toBe("image/png"); + expect(attachment.path).toBe(path); + }); + + it("round-trips a payload larger than one base64 padding boundary", async () => { + const dir = await tempDir(); + const path = join(dir, "snip.png"); + const bytes = Buffer.alloc(300, 7); + await writeFile(path, bytes); + + const attachment = await readAttachment(path); + + expect(Buffer.from(attachment.base64, "base64")).toEqual(bytes); + }); + + it("fails loudly when the file is gone", async () => { + await expect(readAttachment("/nonexistent/snip.png")).rejects.toThrow(); + }); +}); diff --git a/src/__tests__/capture.test.ts b/src/__tests__/capture.test.ts new file mode 100644 index 0000000..9e31fc4 --- /dev/null +++ b/src/__tests__/capture.test.ts @@ -0,0 +1,231 @@ +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + CAPTURE_BACKENDS, + NoCaptureToolError, + captureRegion, + selectBackend, + snipFileName, + type CaptureBackend, +} from "../lib/capture"; + +/** A `PATH` containing exactly the named tools. */ +function pathWith(...tools: string[]) { + const present = new Set(tools); + return async (tool: string) => present.has(tool); +} + +function backendNamed(name: string): CaptureBackend { + const backend = CAPTURE_BACKENDS.find((candidate) => candidate.name === name); + if (!backend) throw new Error(`no backend named ${name}`); + return backend; +} + +const tempDirs: string[] = []; + +async function tempDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), "lam-snip-")); + tempDirs.push(dir); + return dir; +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +describe("snipFileName", () => { + it("is sortable and filesystem-safe", () => { + expect(snipFileName(new Date(2026, 9, 4, 14, 59, 26))).toBe("snip-20261004-145926.png"); + }); + + it("zero-pads every field so names sort as text", () => { + expect(snipFileName(new Date(2026, 0, 1, 0, 0, 0))).toBe("snip-20260101-000000.png"); + }); +}); + +describe("selectBackend", () => { + it("returns nothing when no tool is installed", async () => { + expect(await selectBackend(pathWith())).toBeUndefined(); + }); + + it("prefers grimblast on a Hyprland machine", async () => { + const backend = await selectBackend(pathWith("grimblast", "grim", "slurp", "maim")); + expect(backend?.name).toBe("grimblast"); + }); + + it("falls back to grim + slurp when grimblast is absent", async () => { + const backend = await selectBackend(pathWith("grim", "slurp", "maim")); + expect(backend?.name).toBe("grim + slurp"); + }); + + it("needs both halves of a pair, not just one", async () => { + // grim without slurp cannot select a region, so it must not be chosen. + const backend = await selectBackend(pathWith("grim", "maim")); + expect(backend?.name).toBe("maim"); + }); + + it("falls back through the X11 tools", async () => { + expect((await selectBackend(pathWith("maim")))?.name).toBe("maim"); + expect((await selectBackend(pathWith("scrot")))?.name).toBe("scrot"); + expect((await selectBackend(pathWith("flameshot")))?.name).toBe("flameshot"); + }); + + it("recognises the macOS built-in tool", async () => { + expect((await selectBackend(pathWith("screencapture")))?.name).toBe("screencapture"); + }); + + it("can be given a shorter candidate list, which is how macOS is restricted", async () => { + const onlyScreencapture = CAPTURE_BACKENDS.filter((b) => b.name === "screencapture"); + const backend = await selectBackend(pathWith("screencapture", "maim"), onlyScreencapture); + + expect(backend?.name).toBe("screencapture"); + }); +}); + +describe("backend commands", () => { + it("runs slurp before grim and passes the geometry through", async () => { + const calls: Array<{ tool: string; args: string[] }> = []; + const run = vi.fn(async (tool: string, args: string[]) => { + calls.push({ tool, args }); + return tool === "slurp" ? Buffer.from("120,340 800x600\n") : Buffer.alloc(0); + }); + + await backendNamed("grim + slurp").capture({ output: "/tmp/snip.png", run }); + + expect(calls).toEqual([ + { tool: "slurp", args: [] }, + { tool: "grim", args: ["-g", "120,340 800x600", "/tmp/snip.png"] }, + ]); + }); + + it("does not shell out to grim when the selection was cancelled", async () => { + const run = vi.fn(async () => Buffer.from("")); + + await backendNamed("grim + slurp").capture({ output: "/tmp/snip.png", run }); + + expect(run).toHaveBeenCalledTimes(1); + expect(run).toHaveBeenCalledWith("slurp", [], expect.anything()); + }); + + it("keeps a shell pipeline out of grim by using argv", () => { + // `grim -g "$(slurp)"` is the documented one-liner, but a shell here would + // be a needless injection surface and would not survive a path with spaces. + expect(backendNamed("grim + slurp").requires).toEqual(["grim", "slurp"]); + }); + + it("writes flameshot's stdout, because it refuses an output path", async () => { + const dir = await tempDir(); + const output = join(dir, "snip.png"); + const png = Buffer.from([0x89, 0x50, 0x4e, 0x47]); + + await backendNamed("flameshot").capture({ output, run: async () => png }); + + expect(await readFile(output)).toEqual(png); + }); + + it("writes nothing when flameshot returned no image", async () => { + const dir = await tempDir(); + const output = join(dir, "snip.png"); + + await backendNamed("flameshot").capture({ output, run: async () => Buffer.alloc(0) }); + + await expect(readFile(output)).rejects.toThrow(); + }); + + it("asks screencapture for an interactive region and no sound", async () => { + const run = vi.fn(async () => Buffer.alloc(0)); + + await backendNamed("screencapture").capture({ output: "/tmp/snip.png", run }); + + expect(run).toHaveBeenCalledWith("screencapture", ["-i", "-x", "/tmp/snip.png"], expect.anything()); + }); +}); + +describe("captureRegion", () => { + it("writes into the directory it was given and reports the path", async () => { + const dir = await tempDir(); + const output = join(dir, "snip-20261004-145926.png"); + + const result = await captureRegion({ + outputDir: dir, + platform: "darwin", + now: new Date(2026, 9, 4, 14, 59, 26), + hasCommand: pathWith("screencapture"), + run: async (_tool, args) => { + await writeFile(args[args.length - 1], Buffer.from([0x89, 0x50])); + return Buffer.alloc(0); + }, + }); + + expect(result).toEqual({ path: output, bytes: 2, backend: "screencapture" }); + }); + + it("returns null when the user backed out and no image was written", async () => { + const dir = await tempDir(); + + const result = await captureRegion({ + outputDir: dir, + platform: "darwin", + hasCommand: pathWith("screencapture"), + run: async () => Buffer.alloc(0), + }); + + expect(result).toBeNull(); + }); + + it("treats an empty file as a cancel rather than an attachment", async () => { + // At least one tool exits 0 on cancel, so exit codes cannot be trusted. A + // zero-byte PNG would be sent to the model as a broken image. + const dir = await tempDir(); + + const result = await captureRegion({ + outputDir: dir, + platform: "darwin", + hasCommand: pathWith("screencapture"), + run: async (_tool, args) => { + await writeFile(args[args.length - 1], Buffer.alloc(0)); + return Buffer.alloc(0); + }, + }); + + expect(result).toBeNull(); + }); + + it("explains what to install when nothing is available", async () => { + const dir = await tempDir(); + + await expect( + captureRegion({ outputDir: dir, platform: "linux", hasCommand: pathWith() }), + ).rejects.toThrow(NoCaptureToolError); + }); + + it("creates the output directory on first use", async () => { + const dir = await tempDir(); + const nested = join(dir, "snips"); + const now = new Date(2026, 9, 4, 14, 59, 26); + + await captureRegion({ + outputDir: nested, + platform: "darwin", + now, + hasCommand: pathWith("screencapture"), + run: async (_tool, args) => { + await writeFile(args[args.length - 1], Buffer.from([0x89])); + return Buffer.alloc(0); + }, + }); + + expect((await readFile(join(nested, snipFileName(now)))).length).toBeGreaterThan(0); + }); + + it("does not invent a backend on macOS", async () => { + const dir = await tempDir(); + + // maim is available in this fake PATH but is meaningless on darwin. + await expect( + captureRegion({ outputDir: dir, platform: "darwin", hasCommand: pathWith("maim") }), + ).rejects.toThrow(NoCaptureToolError); + }); +}); diff --git a/src/__tests__/code.test.ts b/src/__tests__/code.test.ts index d774ef7..f56a876 100644 --- a/src/__tests__/code.test.ts +++ b/src/__tests__/code.test.ts @@ -4,6 +4,7 @@ import { labelCodeFences, promoteStandaloneCodeSpans, repairCodeBlocks, + restoreUrlLinks, } from "../lib/code"; describe("inferLanguage", () => { @@ -132,11 +133,130 @@ describe("promoteStandaloneCodeSpans", () => { }); }); +describe("restoreUrlLinks", () => { + it("unwraps a fence that is nothing but URLs", () => { + // The reported case: "here are the links" arriving as a code block, so the + // one thing a URL is for, being clicked, does not work. + const input = [ + "```", + "https://github.com/jasonkneen/openclicky", + "https://github.com/unn-Known1/clickyX", + "```", + ].join("\n"); + + expect(restoreUrlLinks(input)).toBe( + [ + "[https://github.com/jasonkneen/openclicky](https://github.com/jasonkneen/openclicky)", + "[https://github.com/unn-Known1/clickyX](https://github.com/unn-Known1/clickyX)", + ].join("\n"), + ); + }); + + it("unwraps a URL fence even when the model labelled it", () => { + expect(restoreUrlLinks("```bash\nhttps://example.com\n```")).toBe( + "[https://example.com](https://example.com)", + ); + }); + + it("links a bare URL standing alone on a line", () => { + expect(restoreUrlLinks("https://example.com")).toBe("[https://example.com](https://example.com)"); + }); + + it("links a lone inline span holding a URL", () => { + expect(restoreUrlLinks("`https://example.com`")).toBe( + "[https://example.com](https://example.com)", + ); + }); + + it("keeps a bulleted list of links a bulleted list", () => { + const input = "- https://a.example\n- https://b.example"; + + expect(restoreUrlLinks(input)).toBe( + "- [https://a.example](https://a.example)\n- [https://b.example](https://b.example)", + ); + }); + + it("keeps an ordered list marker", () => { + expect(restoreUrlLinks("1. https://example.com")).toBe( + "1. [https://example.com](https://example.com)", + ); + }); + + it("drops sentence punctuation that is not part of the address", () => { + expect(restoreUrlLinks("See this:\n\nhttps://example.com.")).toContain( + "[https://example.com](https://example.com)", + ); + }); + + it("brackets a destination that contains parentheses", () => { + const url = "https://en.wikipedia.org/wiki/Foo_(bar)"; + expect(restoreUrlLinks(url)).toBe(`[${url}](<${url}>)`); + }); + + it("escapes brackets in the label, which an IPv6 literal contains", () => { + // Unescaped, the `]` in the label ends the link text early and the rest of + // the address is left as prose outside the link. + expect(restoreUrlLinks("http://[::1]:8080/")).toBe( + "[http://\\[::1\\]:8080/](http://[::1]:8080/)", + ); + }); + + it("never touches a fence holding real code", () => { + const input = "```bash\ncurl https://example.com | jq .\n```"; + expect(restoreUrlLinks(input)).toBe(input); + }); + + it("never touches a fence when one line is not a URL", () => { + const input = "```\nhttps://example.com\nnot a url\n```"; + expect(restoreUrlLinks(input)).toBe(input); + }); + + it("leaves a URL embedded in a sentence alone", () => { + const input = "The docs at https://example.com cover it."; + expect(restoreUrlLinks(input)).toBe(input); + }); + + it("leaves a markdown link the model already wrote alone", () => { + const input = "[OpenClickly](https://github.com/jasonkneen/openclicky)"; + expect(restoreUrlLinks(input)).toBe(input); + }); + + it("preserves a fence that is never closed and is real code", () => { + expect(restoreUrlLinks("```\ngit log")).toBe("```\ngit log"); + }); + + it("is idempotent", () => { + const once = restoreUrlLinks("```\nhttps://example.com\n```"); + expect(restoreUrlLinks(once)).toBe(once); + }); + + it("is a no-op on ordinary prose", () => { + const input = "There are no addresses in this sentence."; + expect(restoreUrlLinks(input)).toBe(input); + }); +}); + describe("repairCodeBlocks", () => { it("promotes a lone command and labels it in one pass", () => { expect(repairCodeBlocks("`pwd`")).toBe("```bash\npwd\n```"); }); + it("does not fence a lone URL", () => { + // Restoring links runs before promotion, so a recovered URL never becomes a + // fence again. Fencing it here was the bug the user saw. + const repaired = repairCodeBlocks("`https://example.com`"); + + expect(repaired).not.toContain("```"); + expect(repaired).toBe("[https://example.com](https://example.com)"); + }); + + it("unwraps a fenced list of links and leaves no fence behind", () => { + const repaired = repairCodeBlocks("```\nhttps://a.example\nhttps://b.example\n```"); + + expect(repaired).not.toContain("```"); + expect(repaired).toContain("[https://a.example](https://a.example)"); + }); + it("still labels a bare multi-line fence", () => { expect(repairCodeBlocks("```\ngit status\n```")).toBe("```bash\ngit status\n```"); }); diff --git a/src/__tests__/history.test.ts b/src/__tests__/history.test.ts index f0d35d3..5dcdf22 100644 --- a/src/__tests__/history.test.ts +++ b/src/__tests__/history.test.ts @@ -4,7 +4,7 @@ import { buildMessages, composeSystemPrompt } from "../lib/history"; import type { ChatTurn } from "../lib/types"; function turn(prompt: string, response: string): ChatTurn { - return { id: prompt, prompt, response, thinking: "", model: "m", createdAt: 0, toolCalls: [] }; + return { id: prompt, prompt, response, thinking: "", model: "m", createdAt: 0, toolCalls: [], attachments: [] }; } describe("buildMessages", () => { @@ -66,3 +66,37 @@ describe("composeSystemPrompt", () => { expect(composeSystemPrompt("Be terse.", false)).not.toContain(RECENCY_HINT); }); }); + +describe("buildMessages images", () => { + it("attaches images to the question being asked", () => { + const messages = buildMessages("SYSTEM", [], "what is this?", ["BASE64"]); + + expect(messages[messages.length - 1]).toEqual({ + role: "user", + content: "what is this?", + images: ["BASE64"], + }); + }); + + it("omits the images key entirely when there is no attachment", () => { + // The key has to be absent, not an empty array: Ollama treats `images: []` + // differently from no images at all on some models. + const messages = buildMessages("SYSTEM", [], "hello"); + + expect(messages[messages.length - 1]).toEqual({ role: "user", content: "hello" }); + }); + + it("attaches several images to one question", () => { + const messages = buildMessages("SYSTEM", [], "compare these", ["A", "B"]); + + expect(messages[messages.length - 1].images).toEqual(["A", "B"]); + }); + + it("does not replay images from earlier turns", () => { + // A screenshot is the largest thing a request can carry, and re-sending it + // with every later question is exactly what dropping tool output avoids. + const messages = buildMessages("SYSTEM", [turn("earlier", "answer")], "follow up"); + + expect(messages.some((message) => message.images !== undefined)).toBe(false); + }); +}); diff --git a/src/__tests__/markdown.test.ts b/src/__tests__/markdown.test.ts index 1503eac..b72c6be 100644 --- a/src/__tests__/markdown.test.ts +++ b/src/__tests__/markdown.test.ts @@ -15,6 +15,7 @@ function message(overrides: Partial & Pick isStreaming: false, thinking: "", toolCalls: [], + attachments: [], ...overrides, }; } @@ -146,6 +147,34 @@ describe("buildThreadMarkdown", () => { expect(markdown).not.toContain("```bash"); }); + it("turns a fenced list of URLs into links, because a fence is not clickable", () => { + // The reported case: asked for alternatives, the model answered with a code + // block of bare GitHub addresses. `MdCodeBlock.qml` draws that as a dark + // panel with a Copy button and no link handling, so the address could not be + // opened. Only `CMARK_NODE_LINK` becomes an `` the selection + // controller can activate. + const markdown = buildThreadMarkdown([ + message({ + role: "assistant", + text: "GitHub links:\n\n```\nhttps://github.com/jasonkneen/openclicky\nhttps://github.com/unn-Known1/clickyX\n```", + }), + ]); + + expect(markdown).not.toContain("```"); + expect(markdown).toContain( + "[https://github.com/jasonkneen/openclicky](https://github.com/jasonkneen/openclicky)", + ); + }); + + it("links a lone URL the model put in inline code", () => { + const markdown = buildThreadMarkdown([ + message({ role: "assistant", text: "`https://example.com`" }), + ]); + + expect(markdown).not.toContain("```"); + expect(markdown).toContain("[https://example.com](https://example.com)"); + }); + it("lists tools on one quiet line, flagging failures", () => { const markdown = buildThreadMarkdown([ message({ @@ -212,7 +241,17 @@ describe("buildThreadMarkdown", () => { describe("sessionToPlainText", () => { function turn(prompt: string, response: string, error?: string): ChatTurn { - return { id: prompt, prompt, response, thinking: "", model: "m", createdAt: 0, toolCalls: [], error }; + return { + id: prompt, + prompt, + response, + thinking: "", + model: "m", + createdAt: 0, + toolCalls: [], + attachments: [], + error, + }; } it("returns an empty string for an empty conversation", () => { @@ -247,3 +286,41 @@ describe("sessionToPlainText", () => { ); }); }); + +describe("attachments in the transcript", () => { + const screenshot = { path: "/tmp/snip.png", mime: "image/png" }; + + it("says a screenshot is attached, because the image cannot be rendered", () => { + // Images render block-level, centred and capped at 200px (`MdImage.qml`), + // and no form of `![](path)` resolves to a local capture. Saying it is there + // is the honest option; the file itself is opened from the action panel. + const markdown = buildThreadMarkdown([ + message({ role: "user", text: "what is this?", attachments: [screenshot] }), + ]); + + expect(markdown).toContain("_Screenshot attached_"); + expect(markdown).not.toContain("!["); + }); + + it("says nothing about attachments when there are none", () => { + const markdown = buildThreadMarkdown([message({ role: "user", text: "q" })]); + + expect(markdown).not.toContain("Screenshot"); + }); + + it("notes the screenshot when a conversation is copied as plain text", () => { + const question = { + id: "t1", + prompt: "what is this?", + response: "a cat", + thinking: "", + model: "m", + createdAt: 0, + toolCalls: [], + attachments: [screenshot], + }; + const text = sessionToPlainText([question]); + + expect(text).toContain("_(screenshot attached)_"); + }); +}); diff --git a/src/__tests__/ollama.test.ts b/src/__tests__/ollama.test.ts index e1563c5..83c6874 100644 --- a/src/__tests__/ollama.test.ts +++ b/src/__tests__/ollama.test.ts @@ -1,5 +1,5 @@ -import { describe, expect, it } from "vitest"; -import { normalizeHost, parseNdjsonStream } from "../lib/ollama"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { modelCapabilities, normalizeHost, parseNdjsonStream, supportsVision } from "../lib/ollama"; /** * The NDJSON reader is the part of the client most likely to break in a way @@ -84,3 +84,70 @@ describe("parseNdjsonStream", () => { expect(await collect([])).toEqual([]); }); }); + +describe("supportsVision", () => { + it("reads the capability Ollama reports for image input", () => { + expect(supportsVision(["completion", "tools", "vision"])).toBe(true); + }); + + it("is false for a text-only model", () => { + expect(supportsVision(["completion", "tools"])).toBe(false); + expect(supportsVision([])).toBe(false); + }); + + it("does not match a capability that merely contains the word", () => { + expect(supportsVision(["visions"])).toBe(false); + }); +}); + +describe("modelCapabilities", () => { + const fetchMock = vi.fn(); + + beforeEach(() => { + fetchMock.mockReset(); + vi.stubGlobal("fetch", fetchMock); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("posts to /api/show and returns the capability list", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ capabilities: ["completion", "vision"] }), { status: 200 }), + ); + + await expect( + modelCapabilities({ host: "https://ollama.com", apiKey: "k", model: "gemma4" }), + ).resolves.toEqual(["completion", "vision"]); + + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe("https://ollama.com/api/show"); + expect(init.headers.Authorization).toBe("Bearer k"); + expect(JSON.parse(init.body)).toEqual({ model: "gemma4" }); + }); + + it("returns nothing when a model reports no capabilities", async () => { + fetchMock.mockResolvedValueOnce(new Response(JSON.stringify({}), { status: 200 })); + + await expect( + modelCapabilities({ host: "https://ollama.com", apiKey: "k", model: "gemma4" }), + ).resolves.toEqual([]); + }); + + it("turns a 401 into the key hint rather than a bare status code", async () => { + fetchMock.mockResolvedValueOnce(new Response("unauthorized", { status: 401 })); + + await expect( + modelCapabilities({ host: "https://ollama.com", apiKey: "bad", model: "gemma4" }), + ).rejects.toMatchObject({ status: 401, hint: expect.stringContaining("ollama.com/settings/keys") }); + }); + + it("normalises the host so the path does not double up a slash", async () => { + fetchMock.mockResolvedValueOnce(new Response(JSON.stringify({ capabilities: [] }), { status: 200 })); + + await modelCapabilities({ host: "http://localhost:11434/", apiKey: "", model: "gemma4" }); + + expect(fetchMock.mock.calls[0][0]).toBe("http://localhost:11434/api/show"); + }); +}); diff --git a/src/__tests__/sessions.test.ts b/src/__tests__/sessions.test.ts index 06aea95..3d363f7 100644 --- a/src/__tests__/sessions.test.ts +++ b/src/__tests__/sessions.test.ts @@ -24,6 +24,7 @@ function turn(index: number, response = `answer ${index}`): ChatTurn { model: "test-model", createdAt: 1_000 + index, toolCalls: [], + attachments: [], }; } diff --git a/src/__tests__/thread.test.ts b/src/__tests__/thread.test.ts index 906244f..9106f9b 100644 --- a/src/__tests__/thread.test.ts +++ b/src/__tests__/thread.test.ts @@ -13,6 +13,7 @@ function turn(id: string, prompt: string, response: string, overrides: Partial { + __resetLocalStorage(); + fetchMock.mockReset(); + vi.stubGlobal("fetch", fetchMock); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +const base = { host: "https://ollama.com", apiKey: "k", model: "gemma4" }; + +describe("visionSupport", () => { + it("reports yes when Ollama lists the vision capability", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["completion", "vision"])); + await expect(visionSupport(base)).resolves.toBe("yes"); + }); + + it("reports no for a text-only model", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["completion", "tools"])); + await expect(visionSupport(base)).resolves.toBe("no"); + }); + + it("asks /api/show for the model", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["vision"])); + await visionSupport(base); + + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe("https://ollama.com/api/show"); + expect(JSON.parse(init.body)).toEqual({ model: "gemma4" }); + }); + + it("serves a repeat question from the cache", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["vision"])); + await visionSupport(base); + await expect(visionSupport(base)).resolves.toBe("yes"); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("caches a negative too, so a text model is not re-checked on every mount", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["tools"])); + await visionSupport(base); + await expect(visionSupport(base)).resolves.toBe("no"); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("keys the cache per model, so switching model re-checks", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["vision"])); + await visionSupport(base); + + fetchMock.mockResolvedValueOnce(showResponse(["tools"])); + await expect(visionSupport({ ...base, model: "gpt-oss:120b" })).resolves.toBe("no"); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("re-checks when explicitly asked to, even with a cache entry", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["tools"])); + await visionSupport(base); + + fetchMock.mockResolvedValueOnce(showResponse(["vision"])); + await expect(visionSupport({ ...base, refresh: true })).resolves.toBe("yes"); + }); + + it("reports unknown rather than no when the request fails", async () => { + // A dead network is not evidence that a model cannot see, and treating it as + // such would hide the feature for a reason the user cannot see. + fetchMock.mockRejectedValueOnce(new Error("offline")); + await expect(visionSupport(base)).resolves.toBe("unknown"); + }); + + it("does not cache a failure", async () => { + fetchMock.mockRejectedValueOnce(new Error("offline")); + await visionSupport(base); + + fetchMock.mockResolvedValueOnce(showResponse(["vision"])); + await expect(visionSupport(base)).resolves.toBe("yes"); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("reports a rejected key as unknown rather than crashing", async () => { + fetchMock.mockResolvedValueOnce(new Response("unauthorized", { status: 401 })); + await expect(visionSupport(base)).resolves.toBe("unknown"); + }); +}); + +describe("forgetVisionSupport", () => { + it("drops every cached answer, so a re-pulled model is not pinned to no", async () => { + fetchMock.mockResolvedValueOnce(showResponse(["vision"])); + await visionSupport(base); + + await forgetVisionSupport(); + + fetchMock.mockResolvedValueOnce(showResponse(["vision"])); + await visionSupport(base); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/lib/attachments.ts b/src/lib/attachments.ts new file mode 100644 index 0000000..a933452 --- /dev/null +++ b/src/lib/attachments.ts @@ -0,0 +1,38 @@ +import { readFile } from "node:fs/promises"; +import { extname } from "node:path"; +import type { PendingAttachment } from "./types"; + +/** + * Reading attachments off disk. + * + * Kept separate from `capture.ts` because the two answer different questions: + * capture is about getting an image *onto* the disk, this is about getting it + * back off again. It also means a future file picker or clipboard-file path can + * reuse this without touching the capture backends. + * + * Imports no Vicinae module, so it unit tests with no runtime. + */ + +/** Extensions Ollama accepts. A snip is always PNG; the rest are here for a future picker. */ +const MIME_TYPES: Record = { + ".png": "image/png", + ".jpg": "image/jpeg", + ".jpeg": "image/jpeg", + ".webp": "image/webp", + ".gif": "image/gif", +}; + +export function mimeForPath(path: string): string { + return MIME_TYPES[extname(path).toLowerCase()] ?? "application/octet-stream"; +} + +/** + * Reads an image into the shape a request needs. + * + * Ollama wants base64 with no `data:` URL prefix, which is why the payload is + * built here rather than handed straight to the renderer. + */ +export async function readAttachment(path: string): Promise { + const bytes = await readFile(path); + return { path, mime: mimeForPath(path), base64: bytes.toString("base64") }; +} diff --git a/src/lib/capture.ts b/src/lib/capture.ts new file mode 100644 index 0000000..620a658 --- /dev/null +++ b/src/lib/capture.ts @@ -0,0 +1,255 @@ +import { execFile } from "node:child_process"; +import { mkdir, stat, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +/** + * Screen capture for "snip to chat". + * + * Vicinae has no screenshot API. There is no capture call anywhere in + * `@vicinae/api`, and `Clipboard` moves text, html, urls and a file path and + * nothing binary, so a bitmap can never be read back *out* of the clipboard + * either. The only way to get a region of the screen is to shell out. + * + * That is acceptable here because Vicinae extensions run under Node ("all + * standard Node.js APIs are available"), which is why `node:child_process` is + * not a workaround so much as the supported route. The important consequence is + * that the capture is *owned* rather than borrowed: a tool writes a PNG to a + * path this module picks, and that path is what the chat stores. Nothing has to + * guess what some other application left on the clipboard. + * + * Every function here takes its environment as an argument and imports no + * Vicinae module, so the selection logic unit tests with no runtime and no + * installed tools. + */ + +/** A PNG piped through stdout (flameshot) is easily several megabytes. */ +const MAX_CAPTURE_BYTES = 64 * 1024 * 1024; + +/** Long enough to pick a region unhurried, short enough that a wedged tool cannot hang the view forever. */ +const CAPTURE_TIMEOUT_MS = 5 * 60 * 1000; + +/** Runs a command and resolves with its stdout. Injected so tests never spawn anything. */ +export type RunCommand = ( + tool: string, + args: string[], + options?: { signal?: AbortSignal }, +) => Promise; + +export interface CaptureBackend { + /** Shown to the user when a capture fails, so they know what to install. */ + name: string; + /** Every one of these has to resolve on `PATH` for the backend to be usable. */ + requires: string[]; + /** Produces the image at `output`, or resolves without one when the user cancels. */ + capture: (context: CaptureContext) => Promise; +} + +export interface CaptureContext { + output: string; + signal?: AbortSignal; + run: RunCommand; +} + +export interface CaptureResult { + /** Absolute path of the image that was written. */ + path: string; + bytes: number; + /** The tool that produced it, for the transcript and for error messages. */ + backend: string; +} + +export interface CaptureOptions { + /** Where images are kept. The caller supplies `environment.supportPath`, which keeps this module Vicinae-free. */ + outputDir: string; + platform?: NodeJS.Platform; + signal?: AbortSignal; + now?: Date; + /** Overrides for tests. */ + hasCommand?: (tool: string) => Promise; + run?: RunCommand; +} + +/** Raised when the machine has no capture tool at all, with something actionable to do about it. */ +export class NoCaptureToolError extends Error { + readonly hint: string; + + constructor(platform: NodeJS.Platform) { + super("No screen capture tool is installed."); + this.name = "NoCaptureToolError"; + this.hint = + platform === "darwin" + ? "macOS ships screencapture, so this should not happen. Check that it is on PATH." + : "Install one of: grim + slurp (Wayland), grimblast (Hyprland), spectacle, maim + slop (X11), scrot, or flameshot."; + } +} + +/** + * `command -v` rather than spawning the tool itself: several of these block or + * open a window when run bare, and `--version` is not reliably supported. + * + * The tool names are literals from the table below and never come from the + * user, so there is nothing to escape. + */ +async function commandExists(tool: string): Promise { + return new Promise((resolve) => { + execFile("sh", ["-c", `command -v ${tool}`], { timeout: 5_000 }, (error) => resolve(!error)); + }); +} + +function runCommand(tool: string, args: string[], options: { signal?: AbortSignal } = {}): Promise { + return new Promise((resolve, reject) => { + execFile( + tool, + args, + { + signal: options.signal, + timeout: CAPTURE_TIMEOUT_MS, + maxBuffer: MAX_CAPTURE_BYTES, + encoding: "buffer", + }, + (error, stdout) => { + if (error) reject(error); + else resolve(stdout as Buffer); + }, + ); + }); +} + +/** + * `grim -g "$(slurp)"`, expressed as two argv calls rather than a shell pipeline. + * + * slurp prints the chosen geometry and exits non-zero when the selection is + * cancelled, so an empty or absent geometry is the cancel signal. + */ +const grimSlurp: CaptureBackend = { + name: "grim + slurp", + requires: ["grim", "slurp"], + async capture({ output, signal, run }) { + const geometry = (await run("slurp", [], { signal })).toString("utf8").trim(); + if (!geometry) return; + await run("grim", ["-g", geometry, output], { signal }); + }, +}; + +/** + * flameshot is the only backend that will not take an output path: `gui --raw` + * writes the PNG to stdout, so it is captured and written here instead. + */ +const flameshot: CaptureBackend = { + name: "flameshot", + requires: ["flameshot"], + async capture({ output, signal, run }) { + const png = await run("flameshot", ["gui", "--raw"], { signal }); + if (png.length === 0) return; + await writeFile(output, png); + }, +}; + +/** + * Backends in preference order. + * + * Wayland first on Linux, because that is what the launcher's own users are on + * (the README's shortcut examples are Hyprland and niri), then the X11 tools. + * macOS needs only the built-in tool, which is why it is tried on its own. + */ +export const CAPTURE_BACKENDS: CaptureBackend[] = [ + { + name: "grimblast", + requires: ["grimblast"], + async capture({ output, signal, run }) { + await run("grimblast", ["save", "area", output], { signal }); + }, + }, + grimSlurp, + { + name: "spectacle", + requires: ["spectacle"], + async capture({ output, signal, run }) { + // -r region, -b background, -n no notification, -o output file. + await run("spectacle", ["-r", "-b", "-n", "-o", output], { signal }); + }, + }, + { + name: "maim", + requires: ["maim"], + async capture({ output, signal, run }) { + await run("maim", ["-s", output], { signal }); + }, + }, + { + name: "scrot", + requires: ["scrot"], + async capture({ output, signal, run }) { + await run("scrot", ["-s", output], { signal }); + }, + }, + flameshot, + { + name: "screencapture", + requires: ["screencapture"], + async capture({ output, signal, run }) { + // -i interactive region, -x no shutter sound. + await run("screencapture", ["-i", "-x", output], { signal }); + }, + }, +]; + +/** + * The first backend whose tools are all installed, or `undefined`. + * + * `hasCommand` is a parameter rather than a call so the preference order can be + * tested against a fake `PATH`, which is the part most likely to break on a + * platform nobody here runs. + */ +export async function selectBackend( + hasCommand: (tool: string) => Promise, + backends: CaptureBackend[] = CAPTURE_BACKENDS, +): Promise { + for (const backend of backends) { + const checks = await Promise.all(backend.requires.map((tool) => hasCommand(tool))); + if (checks.every(Boolean)) return backend; + } + + return undefined; +} + +/** Sortable, filesystem-safe, and obvious when found lying in the support directory. */ +export function snipFileName(now: Date): string { + const pad = (value: number) => String(value).padStart(2, "0"); + + const date = `${now.getFullYear()}${pad(now.getMonth() + 1)}${pad(now.getDate())}`; + const time = `${pad(now.getHours())}${pad(now.getMinutes())}${pad(now.getSeconds())}`; + + return `snip-${date}-${time}.png`; +} + +/** + * Captures a region and returns the file it wrote, or `null` when the user + * backed out. + * + * Cancel detection leans on the file rather than the exit code on purpose: + * every tool reports a cancelled selection differently, and at least one + * reports success anyway. Whether an image actually landed on disk is the one + * signal they all agree on. + */ +export async function captureRegion(options: CaptureOptions): Promise { + const platform = options.platform ?? process.platform; + const hasCommand = options.hasCommand ?? commandExists; + const run = options.run ?? runCommand; + + const backend = await selectBackend( + hasCommand, + platform === "darwin" ? CAPTURE_BACKENDS.filter((b) => b.name === "screencapture") : CAPTURE_BACKENDS, + ); + if (!backend) throw new NoCaptureToolError(platform); + + await mkdir(options.outputDir, { recursive: true }); + const output = join(options.outputDir, snipFileName(options.now ?? new Date())); + + await backend.capture({ output, signal: options.signal, run }); + + const written = await stat(output).catch(() => null); + if (!written || written.size === 0) return null; + + return { path: output, bytes: written.size, backend: backend.name }; +} diff --git a/src/lib/code.ts b/src/lib/code.ts index 418a544..36bf8e2 100644 --- a/src/lib/code.ts +++ b/src/lib/code.ts @@ -88,6 +88,24 @@ const OPENING_FENCE = /^(\s*)(`{3,}|~{3,})\s*([^\s`~]*)\s*$/; const CLOSING_FENCE = /^\s*(`{3,}|~{3,})\s*$/; const STANDALONE_CODE_SPAN = /^\s*`([^`\n]+)`\s*[.!]?\s*$/; +/** + * A line that is nothing but a bare http(s) URL, give or take a bullet. + * + * Vicinae only makes a URL clickable when markdown presents it as a link. Its + * parser attaches the `table` and `strikethrough` GFM extensions and nothing + * else, so the `autolink` extension is off: a bare URL renders as plain dead + * text. `MarkdownModel` turns `CMARK_NODE_LINK` into ``, and the + * selection controller opens that anchor on release, so `[label](url)` is the + * one form the user can actually click. + */ +const BARE_URL = /^https?:\/\/\S+$/; +/** Sentence punctuation a model appends to a URL that is not part of it. */ +const TRAILING_SENTENCE_PUNCTUATION = /[.,;:!?]+$/; +/** One inline code span that holds nothing but a URL. */ +const STANDALONE_URL_SPAN = /^`([^`\n]+)`[.!]?$/; +/** A bullet or numbered-list marker, kept so a list of links stays a list. */ +const LIST_MARKER = /^([ \t]*(?:[-*+]|\d+[.)])[ \t]+)(.*)$/; + function significantLines(code: string): string[] { return code .split("\n") @@ -272,12 +290,125 @@ export function labelCodeFences(markdown: string): string { return out.join("\n"); } +/** A URL with the sentence punctuation after it removed. */ +function asBareUrl(candidate: string): string | undefined { + const trimmed = candidate.trim(); + if (!BARE_URL.test(trimmed)) return undefined; + return trimmed.replace(TRAILING_SENTENCE_PUNCTUATION, ""); +} + +/** + * Wraps a URL as a markdown link, using the address itself as the label so the + * destination stays readable rather than turning into a generic word. + * + * A destination containing parentheses (plenty of Wikipedia articles) has to + * be wrapped in pointy brackets or the parser reads the first `)` as the end of + * the destination and leaves the rest as trailing prose. + */ +function asLink(url: string): string { + // Square brackets in the *label* would end the link text early, which is why + // an IPv6 literal such as `http://[::1]/` needs them escaped. The destination + // tolerates brackets, but not parentheses, which is what the pointy form is + // for. + const label = url.replace(/[[\]]/g, "\\$&"); + const destination = /[()]/.test(url) ? `<${url}>` : url; + return `[${label}](${destination})`; +} + +/** + * The URLs of a fence whose every non-empty line is one, or `undefined` when + * the block is real code. + * + * Deliberately narrow: one line that is not a URL disqualifies the whole block, + * so a `curl` invocation that happens to contain an address is never touched. + */ +function urlOnlyBlock(lines: string[]): string[] | undefined { + const urls: string[] = []; + + for (const line of lines) { + const trimmed = line.trim(); + if (trimmed.length === 0) continue; + + const url = asBareUrl(trimmed); + if (!url) return undefined; + urls.push(url); + } + + return urls.length > 0 ? urls : undefined; +} + +/** Rewrites a line that is only a URL, or only a span holding one, into a link. */ +function linkifyLine(line: string): string { + const marker = LIST_MARKER.exec(line); + const prefix = marker ? marker[1] : (line.match(/^[ \t]*/)?.[0] ?? ""); + const rest = marker ? marker[2] : line.slice(prefix.length); + + const span = STANDALONE_URL_SPAN.exec(rest.trim()); + const url = asBareUrl(span ? span[1] : rest); + + return url ? `${prefix}${asLink(url)}` : line; +} + +/** + * Turns URLs the model wrote as code back into links. + * + * This is the reported bug: asked for a few GitHub projects, the model answers + * with a fenced block of bare addresses. A fence renders through + * `MdCodeBlock.qml` as a dark panel with a Copy button and no link handling at + * all, so the one thing the user wants to do with a URL is the one thing they + * cannot. The same applies to a lone `` `https://...` `` span. + * + * Three shapes are recovered, and only these: + * + * 1. A fence whose entire body is URLs. + * 2. A line that is exactly one inline span holding a URL. + * 3. A line that is exactly a URL, bulleted or not. + * + * Existing fences holding anything else are copied through untouched, so real + * code is never rewritten. + */ +export function restoreUrlLinks(markdown: string): string { + const lines = markdown.split("\n"); + const out: string[] = []; + let index = 0; + + while (index < lines.length) { + if (!OPENING_FENCE.test(lines[index])) { + out.push(linkifyLine(lines[index])); + index += 1; + continue; + } + + const body: string[] = []; + let cursor = index + 1; + while (cursor < lines.length && !CLOSING_FENCE.test(lines[cursor])) { + body.push(lines[cursor]); + cursor += 1; + } + + const closing = cursor < lines.length; + const urls = urlOnlyBlock(body); + + if (urls) out.push(...urls.map(asLink)); + else { + out.push(lines[index], ...body); + if (closing) out.push(lines[cursor]); + } + + index = cursor + (closing ? 1 : 0); + } + + return out.join("\n"); +} + /** * The full pre-render pass, in order. * - * Promotion runs first so a promoted block is seen as an ordinary fence by the - * labeller, which is also harmless when promotion already inferred a language. + * Links are restored first so a recovered URL leaves no span for the promoter + * to fence again. Promotion then runs before labelling, so a promoted block is + * seen as an ordinary fence by the labeller, which is also harmless when + * promotion already inferred a language. */ export function repairCodeBlocks(markdown: string): string { - return labelCodeFences(promoteStandaloneCodeSpans(markdown)); + return labelCodeFences(promoteStandaloneCodeSpans(restoreUrlLinks(markdown))); } diff --git a/src/lib/components/ChatView.tsx b/src/lib/components/ChatView.tsx index bc62c15..00d7aca 100644 --- a/src/lib/components/ChatView.tsx +++ b/src/lib/components/ChatView.tsx @@ -1,10 +1,13 @@ import { Action, ActionPanel, Icon, List } from "@vicinae/api"; import { useCallback, useEffect, useMemo, useState } from "react"; import { useChat } from "../hooks/useChat"; +import { useSnip } from "../hooks/useSnip"; +import { useVisionSupport } from "../hooks/useVision"; import { buildThreadMarkdown, type TranscriptOptions } from "../markdown"; import { getLamPreferences } from "../preferences"; import { deleteSession, listSessions, loadSession, summarizeSession } from "../sessions"; -import { buildThread } from "../thread";import type { Session, SessionSummary } from "../types"; +import { buildThread } from "../thread"; +import type { PendingAttachment, Session, SessionSummary } from "../types"; import { recencyGroup, truncate } from "../utils"; /** @@ -48,11 +51,21 @@ import { recencyGroup, truncate } from "../utils"; const EMPTY_TRANSCRIPT = "**Ask Lam anything**\n\nType a question in the search bar and press Return."; +/** Sent when a snip is attached and Return is pressed before anything is typed. */ +const DEFAULT_IMAGE_PROMPT = "What is in this image?"; + +/** The reminder shown while a snip is captured but not yet sent. */ +const PENDING_NOTE = "_Screenshot attached. Ask a question about it, or press Return to send it as it is._"; + export interface ChatViewProps { /** Opens a specific conversation instead of resuming the latest one. */ initialSession?: Session; /** Text handed over by a deeplink or fallback command. */ initialPrompt?: string; + /** A snip that has already been captured, used when a view is pushed with one. */ + initialAttachment?: PendingAttachment; + /** Captures a snip on mount, which is what the standalone command does. */ + startWithSnip?: boolean; } interface SessionRow { @@ -62,14 +75,28 @@ interface SessionRow { isActive: boolean; } -export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { +export function ChatView({ initialSession, initialPrompt, initialAttachment, startWithSnip }: ChatViewProps) { const preferences = useMemo(() => getLamPreferences(), []); const chat = useChat({ preferences, initialSession }); const { replaceSession } = chat; + const { isSniping, snip } = useSnip(); + + const { + support: vision, + canSeeImages, + isChecking: isCheckingVision, + recheck, + } = useVisionSupport(preferences); const [input, setInput] = useState(initialPrompt ?? ""); const [summaries, setSummaries] = useState(undefined); - const [resumed, setResumed] = useState(Boolean(initialSession)); + // A snip command starts a new chat on purpose, so the idle-resume below is + // skipped and the view must not wait for it. + const [resumed, setResumed] = useState(Boolean(initialSession) || Boolean(startWithSnip)); + // A captured image is held here until the user says what they want to know + // about it. Nothing is sent to the model on capture alone. + const [pending, setPending] = useState(initialAttachment); + const [snipNote, setSnipNote] = useState(undefined); const activeId = chat.session.id; @@ -89,9 +116,13 @@ export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { * was used recently. Past the configured idle window it starts a fresh chat * instead. The previous conversation is never removed, it just drops down the * sidebar. Setting the preference to "Never" keeps the last chat open forever. + * + * The snip command opts out entirely: it exists to start a new chat about one + * screenshot, and quietly continuing the previous conversation would bury the + * image in an unrelated thread. */ useEffect(() => { - if (initialSession) return; + if (initialSession || startWithSnip) return; let cancelled = false; void (async () => { @@ -113,7 +144,7 @@ export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { return () => { cancelled = true; }; - }, [initialSession, preferences, replaceSession]); + }, [initialSession, startWithSnip, preferences, replaceSession]); const thread = useMemo( () => @@ -131,11 +162,45 @@ export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { [preferences.showToolCalls, preferences.showReasoning], ); - const threadMarkdown = useMemo( - () => - thread.length === 0 ? EMPTY_TRANSCRIPT : buildThreadMarkdown(thread, transcriptOptions), - [thread, transcriptOptions], - ); + /** + * The transcript, preceded by any notes about this view's own state. + * + * Notes go first because the transcript reads newest-first: the top of the + * panel is the text directly under the search bar, which is where the user is + * already looking. + */ + const threadMarkdown = useMemo(() => { + const notes: string[] = []; + + // Only the standalone command has to explain a model that cannot see; inside + // the chat the action is simply absent. A failed check is worded separately + // from a text-only model, because saying "your model cannot see" when the + // request merely failed is a claim about the user's setup that is not true. + if (startWithSnip && !isCheckingVision && vision !== "yes") { + notes.push( + vision === "no" + ? `**${preferences.model} cannot see images.** Choose a model with vision support in preferences, and this command becomes a one-key snip.` + : `**Could not check whether ${preferences.model} can see images.** Use Check Again below, or snip from the chat's action panel once the connection is back.`, + ); + } + + if (snipNote) notes.push(snipNote); + if (pending) notes.push(PENDING_NOTE); + + const transcript = + thread.length === 0 ? EMPTY_TRANSCRIPT : buildThreadMarkdown(thread, transcriptOptions); + + return [...notes, transcript].join("\n\n"); + }, [ + startWithSnip, + isCheckingVision, + vision, + preferences.model, + snipNote, + pending, + thread, + transcriptOptions, + ]); const rows: SessionRow[] = useMemo(() => { const base = summaries ?? []; @@ -194,15 +259,53 @@ export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { [activeId, chat, reload], ); + /** + * Captures a region and holds it until a question is sent with it. + * + * A cancelled selection deliberately says nothing: the user pressed Escape and + * telling them so is noise. A failed capture has to explain itself, because + * the usual cause is a capture tool that is not installed. + */ + const takeSnip = useCallback(async () => { + setSnipNote(undefined); + const outcome = await snip(); + + if (outcome.status === "captured") setPending(outcome.attachment); + else if (outcome.status === "failed") + setSnipNote(`**Could not take a screenshot:** ${outcome.message}`); + }, [snip]); + + // The standalone command captures on arrival. Waiting for the capability check + // matters here: capturing a screenshot only to be told the model cannot read + // it would throw away the selection the user just made. + useEffect(() => { + if (!startWithSnip || !canSeeImages || initialAttachment) return; + void takeSnip(); + }, [startWithSnip, canSeeImages, initialAttachment, takeSnip]); + + /** The snip a "show me the screenshot" action would open: the pending one, else the newest. */ + const latestAttachment = + pending ?? + [...chat.session.turns].reverse().find((turn) => turn.attachments.length > 0)?.attachments[0]; + const send = useCallback( (text: string) => { - chat.ask(text); + const typed = text.trim(); + // With a snip attached, Return has to send even before anything is typed, + // otherwise the only way to ask about an image is to type out a question + // the image already implies. + const prompt = typed || (pending ? DEFAULT_IMAGE_PROMPT : ""); + if (!prompt) return; + + chat.ask(prompt, pending); + setPending(undefined); + setSnipNote(undefined); setInput(""); }, - [chat.ask], + [chat.ask, pending], ); - const hasPrompt = input.trim().length > 0; + const hasPrompt = input.trim().length > 0 || pending !== undefined; const stopAction = ; const sendAction = send(input)} />; @@ -216,10 +319,40 @@ export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { /> ); + /** + * Present only when Ollama has confirmed the model reports `vision`. + * + * Offering it otherwise would let the user frame a careful screenshot and + * then have a model that cannot see answer as though it had, which is a worse + * failure than the action not being there. + */ + const snipAction = canSeeImages ? ( + void takeSnip()} + /> + ) : null; + + // A screenshot is a file, so it opens in the system image viewer. Without this + // the only way to see what was sent would be to find it on disk by hand. + const openSnipAction = latestAttachment ? ( + + ) : null; + + // Only the standalone command reaches a state with nowhere to go: a failed + // capability check leaves it with no snip and no action that would start one. + const checkAgainAction = + startWithSnip && !isCheckingVision && vision === "unknown" ? ( + + ) : null; + /** * The primary action adapts to the search bar. With text in it, Return sends; * with it empty (where sending would be a no-op anyway), Return opens the - * highlighted conversation instead. + * highlighted conversation instead. A pending snip counts as text, so Return + * sends it rather than navigating away. */ const rowActions = (row: SessionRow) => { const openAction = ( @@ -231,6 +364,9 @@ export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { {chat.isStreaming ? stopAction : hasPrompt ? sendAction : openAction} {hasPrompt ? openAction : null} {newChatAction} + {snipAction} + {openSnipAction} + {checkAgainAction} ; } + const busy = chat.isStreaming || isSniping; + const placeholder = chat.status ? `${chat.status}. Press ⏎ to stop` : "Lam is thinking. Press ⏎ to stop"; + const idlePlaceholder = pending ? "Ask about the screenshot…" : "Ask Lam anything…"; + return ( {chat.isStreaming ? stopAction : sendAction} {newChatAction} + {snipAction} + {openSnipAction} + {checkAgainAction} } > @@ -281,7 +426,7 @@ export function ChatView({ initialSession, initialPrompt }: ChatViewProps) { // Title only. Raycast's sidebar shows nothing else, and stacking a // subtitle plus a timestamp on a 38px row elided the title down to // a few characters. The check icon is what marks the open chat. - detail={} + detail={} actions={rowActions(row)} /> ))} diff --git a/src/lib/constants.ts b/src/lib/constants.ts index 07af547..9bdd1af 100644 --- a/src/lib/constants.ts +++ b/src/lib/constants.ts @@ -36,6 +36,8 @@ Write plainly: Fence every command and every snippet, and always name the language: bash for shell and terminal commands, then json, sql, yaml, or the language for code (ts, js, python, rust, go). Never leave code inline in a sentence, and never open a fence without a language. Put no $ or shell prompt inside the block so it can be copied and run as-is. Ignore the formatting of earlier answers in this conversation: if one put a command in inline code, fence it anyway. +A web address is a link, never code. A markdown link, written as [label](https://example.com), is clickable in this window, while a URL inside backticks or a fenced block is dead text the user cannot open. So never fence a URL, never put one in inline code, and never answer "here are the links" with a snippet. Label a link with what it is ("OpenClickly on GitHub"), and when you give several, put each on its own bullet with its own link. + A fenced block is never a whole answer on its own. Say in one plain sentence what the code does or when to reach for it, so the reply reads like something a person wrote rather than a bare snippet. Call a tool when it makes the answer more accurate. Anything time-sensitive or past your cutoff should be checked rather than guessed, and never invent tool output. If a tool fails, say so and answer with what you know. diff --git a/src/lib/history.ts b/src/lib/history.ts index 8271d4f..3b23105 100644 --- a/src/lib/history.ts +++ b/src/lib/history.ts @@ -8,8 +8,17 @@ import type { ApiMessage, ChatTurn } from "./types"; * messages from a previous turn are intentionally dropped: they are noise once * the answer is written, and replaying them would re-send large page contents * on every subsequent request. + * + * Attached images are treated the same way, and for the same reason. They are + * the largest thing a request can carry, so only the question being asked now + * brings one. A follow-up about a screenshot needs the image attached again. */ -export function buildMessages(systemPrompt: string, turns: ChatTurn[], prompt: string): ApiMessage[] { +export function buildMessages( + systemPrompt: string, + turns: ChatTurn[], + prompt: string, + images?: string[], +): ApiMessage[] { const messages: ApiMessage[] = [{ role: "system", content: systemPrompt }]; for (const turn of turns) { @@ -18,7 +27,10 @@ export function buildMessages(systemPrompt: string, turns: ChatTurn[], prompt: s messages.push({ role: "assistant", content: turn.response }); } - messages.push({ role: "user", content: prompt }); + const question: ApiMessage = { role: "user", content: prompt }; + if (images && images.length > 0) question.images = images; + messages.push(question); + return messages; } diff --git a/src/lib/hooks/useChat.ts b/src/lib/hooks/useChat.ts index e4dd7d1..377241b 100644 --- a/src/lib/hooks/useChat.ts +++ b/src/lib/hooks/useChat.ts @@ -5,7 +5,7 @@ import { OllamaError } from "../ollama"; import type { ResolvedPreferences } from "../preferences"; import { createSession, createTurn, saveSession, withTitleFromFirstTurn } from "../sessions"; import { toolsFor } from "../tools"; -import type { Session } from "../types"; +import type { PendingAttachment, Session } from "../types"; import { errorMessage, isAbortError } from "../utils"; /** @@ -41,7 +41,7 @@ export interface UseChat { /** Short activity line such as "Searching the web", shown while a turn is in flight. */ status?: string; error?: string; - ask: (prompt: string) => void; + ask: (prompt: string, attachment?: PendingAttachment) => void; cancel: () => void; replaceSession: (session: Session) => void; newSession: () => void; @@ -119,16 +119,20 @@ export function useChat(options: UseChatOptions): UseChat { }, []); const ask = useCallback( - (rawPrompt: string) => { + (rawPrompt: string, attachment?: PendingAttachment) => { const prompt = rawPrompt.trim(); if (!prompt || liveRef.current) return; const preferences = preferencesRef.current; const base = sessionRef.current; + // Only the path and mime type are kept on the turn. The base64 payload is + // used for this one request and then dropped, so it never reaches storage. + const attachments = attachment ? [{ path: attachment.path, mime: attachment.mime }] : []; + // The turn is inserted immediately with an empty response so the UI shows // the question while the answer streams in beneath it. - const turn = createTurn(prompt, preferences.model); + const turn = createTurn(prompt, preferences.model, Date.now(), attachments); const started = withTitleFromFirstTurn({ ...base, model: preferences.model, @@ -152,6 +156,7 @@ export function useChat(options: UseChatOptions): UseChat { composeSystemPrompt(preferences.systemPrompt, preferences.webEnabled), base.turns, prompt, + attachment ? [attachment.base64] : undefined, ); void (async () => { diff --git a/src/lib/hooks/useSnip.ts b/src/lib/hooks/useSnip.ts new file mode 100644 index 0000000..dfef5d0 --- /dev/null +++ b/src/lib/hooks/useSnip.ts @@ -0,0 +1,61 @@ +import { environment } from "@vicinae/api"; +import { join } from "node:path"; +import { useCallback, useState } from "react"; +import { readAttachment } from "../attachments"; +import { NoCaptureToolError, captureRegion } from "../capture"; +import type { PendingAttachment } from "../types"; +import { errorMessage } from "../utils"; + +/** + * Taking a snip, from the UI's point of view. + * + * The outcome is a discriminated union rather than `null`, because "the user + * pressed Escape" and "there is no capture tool on this machine" want opposite + * responses: the first should say nothing at all, the second has to explain + * itself. A hook rather than a bare function so both entry points (the chat + * action and the standalone command) share one implementation and one spinner. + */ + +export type SnipOutcome = + | { status: "captured"; attachment: PendingAttachment } + | { status: "cancelled" } + | { status: "failed"; message: string }; + +export interface UseSnip { + isSniping: boolean; + snip: () => Promise; +} + +/** + * Snips live in the extension's own support directory rather than a temp + * directory: a session stores the path, and a path that survives a reboot is + * the difference between an old conversation showing its screenshot and + * referring to a file that no longer exists. + */ +export function snipDirectory(): string { + return join(environment.supportPath, "snips"); +} + +export function useSnip(): UseSnip { + const [isSniping, setIsSniping] = useState(false); + + const snip = useCallback(async (): Promise => { + setIsSniping(true); + + try { + const capture = await captureRegion({ outputDir: snipDirectory() }); + + // No image on disk means the region selection was abandoned. + if (!capture) return { status: "cancelled" }; + + return { status: "captured", attachment: await readAttachment(capture.path) }; + } catch (thrown) { + const hint = thrown instanceof NoCaptureToolError ? ` ${thrown.hint}` : ""; + return { status: "failed", message: `${errorMessage(thrown)}${hint}` }; + } finally { + setIsSniping(false); + } + }, []); + + return { isSniping, snip }; +} diff --git a/src/lib/hooks/useVision.ts b/src/lib/hooks/useVision.ts new file mode 100644 index 0000000..bfa66ec --- /dev/null +++ b/src/lib/hooks/useVision.ts @@ -0,0 +1,73 @@ +import { useCallback, useEffect, useRef, useState } from "react"; +import type { ResolvedPreferences } from "../preferences"; +import { visionSupport, type VisionSupport } from "../vision"; + +/** + * Whether the current model accepts images, for gating the snip actions. + * + * The state is the same tri-state the cached lookup returns, not a boolean. A + * failed check and a text-only model want different words: claiming a model + * "cannot see images" when the request merely failed is a false statement about + * the user's setup, and it sends them looking for a different model for no + * reason. + * + * The actions themselves still fail closed: only a confirmed `"yes"` offers a + * snip. + */ + +export interface VisionState { + /** `"unknown"` means the check failed, which is not the same as `"no"`. */ + support: VisionSupport; + /** True only once Ollama has confirmed the model reports the `vision` capability. */ + canSeeImages: boolean; + /** True while an answer for this model is still outstanding. */ + isChecking: boolean; + /** Asks again, ignoring the cache. */ + recheck: () => void; +} + +export function useVisionSupport(preferences: ResolvedPreferences): VisionState { + const [support, setSupport] = useState("unknown"); + const [isChecking, setIsChecking] = useState(true); + const [attempt, setAttempt] = useState(0); + + // Set by `recheck` and consumed by the next run. A ref rather than a + // dependency so that a manual retry does not also make every later model + // change bypass the cache. + const refreshRef = useRef(false); + + useEffect(() => { + let cancelled = false; + const controller = new AbortController(); + + setIsChecking(true); + + void visionSupport({ + host: preferences.host, + apiKey: preferences.apiKey, + model: preferences.model, + signal: controller.signal, + refresh: refreshRef.current, + }) + .then((result) => { + if (cancelled) return; + setSupport(result); + }) + .finally(() => { + refreshRef.current = false; + if (!cancelled) setIsChecking(false); + }); + + return () => { + cancelled = true; + controller.abort(); + }; + }, [preferences.host, preferences.apiKey, preferences.model, attempt]); + + const recheck = useCallback(() => { + refreshRef.current = true; + setAttempt((value) => value + 1); + }, []); + + return { support, canSeeImages: support === "yes", isChecking, recheck }; +} diff --git a/src/lib/markdown.ts b/src/lib/markdown.ts index 8089685..31fd926 100644 --- a/src/lib/markdown.ts +++ b/src/lib/markdown.ts @@ -49,7 +49,14 @@ function renderTools(message: ThreadMessage): string { function renderMessage(message: ThreadMessage, options: TranscriptOptions): string { // The question is identified by its styling alone. - if (message.role === "user") return quote(message.text); + if (message.role === "user") { + // An attached screenshot cannot be shown here: images render block-level, + // centred and capped at 200px (`MdImage.qml`), and there is no form of + // `![](path)` that resolves to a local capture. The transcript says the + // image is there instead, and the action panel opens it. + const attached = message.attachments.length > 0 ? "\n\n_Screenshot attached_" : ""; + return `${quote(message.text)}${attached}`; + } // While a tool is running there is no answer text yet, so the activity line // stands in and the user can see what Lam is actually doing. @@ -138,9 +145,10 @@ export function sessionToPlainText(turns: ChatTurn[]): string { return turns .map((turn) => { const question = `**You**\n\n${turn.prompt}`; + const attached = turn.attachments.length > 0 ? "\n\n_(screenshot attached)_" : ""; const answer = turn.response.trim() || (turn.error ? "" : "_(no response)_"); const failure = turn.error ? `\n\n**[error]** ${turn.error}` : ""; - return `${question}\n\n**Lam**\n\n${answer}${failure}`; + return `${question}${attached}\n\n**Lam**\n\n${answer}${failure}`; }) .join("\n\n---\n\n"); } diff --git a/src/lib/ollama.ts b/src/lib/ollama.ts index e9690b6..00ce994 100644 --- a/src/lib/ollama.ts +++ b/src/lib/ollama.ts @@ -219,6 +219,44 @@ export async function listModels(options: ListModelsOptions): Promise a.name.localeCompare(b.name)); } +export interface ModelCapabilitiesOptions { + host: string; + apiKey: string; + model: string; + signal?: AbortSignal; +} + +/** + * The features a model reports for itself, straight from `POST /api/show`. + * + * `GET /api/tags` is not enough here: it returns names, sizes and quantization + * levels and says nothing about what the model can actually do. The capability + * list is what turns "image input" from a guess about the model's name into a + * fact, so it is worth the extra request. + */ +export async function modelCapabilities(options: ModelCapabilitiesOptions): Promise { + const host = normalizeHost(options.host); + + const response = await fetch(`${host}/api/show`, { + method: "POST", + headers: { "Content-Type": "application/json", ...authHeaders(options.apiKey) }, + body: JSON.stringify({ model: options.model }), + signal: options.signal, + }); + + if (!response.ok) { + throw describeHttpError(response.status, await safeText(response), host); + } + + const payload = (await response.json()) as { capabilities?: string[] }; + return payload.capabilities ?? []; +} + +/** Ollama reports image input as the `vision` capability. */ +export function supportsVision(capabilities: string[]): boolean { + return capabilities.includes("vision"); +} + export interface WebSearchResult { title: string; url: string; diff --git a/src/lib/schemas.ts b/src/lib/schemas.ts index 91091c2..d03567d 100644 --- a/src/lib/schemas.ts +++ b/src/lib/schemas.ts @@ -14,6 +14,11 @@ export const ToolCallRecordSchema = z.object({ isError: z.boolean(), }); +export const AttachmentSchema = z.object({ + path: z.string(), + mime: z.string().default("image/png"), +}); + export const ChatTurnSchema = z.object({ id: z.string(), prompt: z.string(), @@ -23,6 +28,9 @@ export const ChatTurnSchema = z.object({ createdAt: z.number(), error: z.string().optional(), toolCalls: z.array(ToolCallRecordSchema).default([]), + // Sessions written before image input existed have no attachments key, so the + // default is what keeps them loading rather than being rejected as corrupt. + attachments: z.array(AttachmentSchema).default([]), }); export const SessionSchema = z.object({ diff --git a/src/lib/sessions.ts b/src/lib/sessions.ts index b8bdce3..75738a1 100644 --- a/src/lib/sessions.ts +++ b/src/lib/sessions.ts @@ -1,7 +1,7 @@ import { LocalStorage } from "@vicinae/api"; import { MAX_SESSIONS, MAX_STORED_CHARS_PER_TURN, MAX_TURNS_PER_SESSION, STORAGE_INDEX_KEY, STORAGE_SESSION_PREFIX, TITLE_MAX_LENGTH } from "./constants"; import { SessionIndexSchema, SessionSchema } from "./schemas"; -import type { ChatTurn, Session, SessionSummary } from "./types"; +import type { Attachment, ChatTurn, Session, SessionSummary } from "./types"; import { titleFromPrompt, truncate } from "./utils"; /** @@ -56,7 +56,12 @@ export function createSession(model: string, now: number = Date.now()): Session } /** Creates the turn record for a prompt that is about to be sent. */ -export function createTurn(prompt: string, model: string, now: number = Date.now()): ChatTurn { +export function createTurn( + prompt: string, + model: string, + now: number = Date.now(), + attachments: Attachment[] = [], +): ChatTurn { return { id: newId(), prompt, @@ -65,6 +70,7 @@ export function createTurn(prompt: string, model: string, now: number = Date.now model, createdAt: now, toolCalls: [], + attachments, }; } diff --git a/src/lib/thread.ts b/src/lib/thread.ts index 10a74bf..c7a6326 100644 --- a/src/lib/thread.ts +++ b/src/lib/thread.ts @@ -43,6 +43,8 @@ export function buildThread(turns: ChatTurn[], live: LiveState): ThreadMessage[] // Only while there is nothing to read yet. Once tokens land, the // placeholder it feeds is gone anyway. status: isLive && text.length === 0 ? live.status : undefined, + // Only a question carries attachments, so the answer never has one. + attachments: [], }); messages.push({ @@ -53,6 +55,7 @@ export function buildThread(turns: ChatTurn[], live: LiveState): ThreadMessage[] isStreaming: false, thinking: "", toolCalls: [], + attachments: turn.attachments, }); } diff --git a/src/lib/types.ts b/src/lib/types.ts index ebe9824..e88abbe 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -37,6 +37,42 @@ export interface ApiMessage { tool_calls?: ToolCall[]; /** Only set on `role: "tool"` messages, naming the tool that produced this content. */ tool_name?: string; + /** + * Base64 image data, for vision models. + * + * Ollama only reads these on a `user` message (`gpt-oss` style tool roles have + * no images). They are the one genuinely large thing in a request, so they are + * attached to the question they belong to and never replayed later, the same + * way tool output is not replayed. + */ + images?: string[]; +} + +/** + * A file the user attached to a question. + * + * Only the path is stored. A screenshot runs to megabytes, and + * `MAX_STORED_CHARS_PER_TURN` exists precisely so one turn cannot dominate + * LocalStorage, so the bytes stay on disk and are read when the turn is sent. + */ +export interface Attachment { + /** Absolute path to the image on disk. */ + path: string; + /** Sent to Ollama next to the base64 payload. */ + mime: string; +} + +/** + * An attachment that has been read and is ready to send, held in the view while + * the user types their question. + * + * The base64 payload lives here and nowhere else. It is needed for exactly one + * request, so it is deliberately not part of `Attachment` and never reaches + * storage. + */ +export interface PendingAttachment extends Attachment { + /** Base64 of the image, with no `data:` URL prefix. Ollama wants it bare. */ + base64: string; } /** What a tool actually returned during a turn, kept for display and debugging. */ @@ -58,6 +94,8 @@ export interface ChatTurn { /** Set when the turn failed, so a stored session can show what went wrong. */ error?: string; toolCalls: ToolCallRecord[]; + /** Images the user attached, kept for the transcript. The bytes are not stored. */ + attachments: Attachment[]; } export interface Session { @@ -104,4 +142,6 @@ export interface ThreadMessage { error?: string; /** Activity line shown while this message is streaming, e.g. "Searching the web". */ status?: string; + /** Images attached to this question. Only ever non-empty on a user message. */ + attachments: Attachment[]; } diff --git a/src/lib/vision.ts b/src/lib/vision.ts new file mode 100644 index 0000000..3de2624 --- /dev/null +++ b/src/lib/vision.ts @@ -0,0 +1,75 @@ +import { LocalStorage } from "@vicinae/api"; +import { modelCapabilities, supportsVision } from "./ollama"; + +/** + * The cached answer to "can this model see images?". + * + * `POST /api/show` is a round trip, and the answer only changes when the user + * switches model. Without a cache the chat view would ask again on every mount + * and the snip action would flicker in and out of the action panel while the + * request was in flight. + * + * `"unknown"` is deliberately distinct from `"no"`. A failed request is not + * evidence that a model cannot see, so the caller decides how much to trust it + * and a failure is never written to the cache. + */ + +/** One key per host and model, so switching model invalidates by construction. */ +const VISION_CACHE_PREFIX = "lam.vision."; + +export type VisionSupport = "yes" | "no" | "unknown"; + +export interface VisionCheck { + host: string; + apiKey: string; + model: string; + signal?: AbortSignal; + /** Ignores the cache. Used when the answer might have gone stale. */ + refresh?: boolean; +} + +function cacheKey(host: string, model: string): string { + return `${VISION_CACHE_PREFIX}${host}|${model}`; +} + +export async function visionSupport(check: VisionCheck): Promise { + const key = cacheKey(check.host, check.model); + + if (!check.refresh) { + const cached = await LocalStorage.getItem(key).catch(() => undefined); + if (cached !== undefined) return cached ? "yes" : "no"; + } + + try { + const capable = supportsVision( + await modelCapabilities({ + host: check.host, + apiKey: check.apiKey, + model: check.model, + signal: check.signal, + }), + ); + + await LocalStorage.setItem(key, capable).catch(() => undefined); + return capable ? "yes" : "no"; + } catch { + return "unknown"; + } +} + +/** + * Drops every cached answer. + * + * A model can gain vision without its name changing (a re-pull, a different + * tag on the same account), so `Set Up Lam Agent` calls this rather than + * leaving the user pinned to a stale "no". + */ +export async function forgetVisionSupport(): Promise { + const items = await LocalStorage.allItems().catch(() => ({})); + + await Promise.all( + Object.keys(items) + .filter((key) => key.startsWith(VISION_CACHE_PREFIX)) + .map((key) => LocalStorage.removeItem(key).catch(() => undefined)), + ); +} diff --git a/src/setup.tsx b/src/setup.tsx index 441f3fc..6a8d3d6 100644 --- a/src/setup.tsx +++ b/src/setup.tsx @@ -2,6 +2,7 @@ import { Action, ActionPanel, Icon, List, openExtensionPreferences } from "@vici import { useCallback, useEffect, useMemo, useState } from "react"; import { describeResumeTimeout, getLamPreferences, needsSetup } from "./lib/preferences"; import { listModels } from "./lib/ollama"; +import { forgetVisionSupport, visionSupport, type VisionSupport } from "./lib/vision"; import type { ModelInfo } from "./lib/types"; import { errorMessage } from "./lib/utils"; @@ -36,6 +37,7 @@ export default function SetupCommand() { const [error, setError] = useState(undefined); const [isLoading, setIsLoading] = useState(false); const [hasTested, setHasTested] = useState(false); + const [vision, setVision] = useState("unknown"); const missingKey = needsSetup(preferences); @@ -53,6 +55,19 @@ export default function SetupCommand() { setHasTested(true); setIsLoading(false); } + + // A model can gain or lose image support without its name changing (a + // re-pull, a retagged build), so re-testing drops the cached answer that the + // chat view gates the snip action on rather than leaving a stale "no". + await forgetVisionSupport(); + setVision( + await visionSupport({ + host: preferences.host, + apiKey: preferences.apiKey, + model: preferences.model, + refresh: true, + }), + ); }, [preferences]); // Verifying on open means the user sees something real immediately rather @@ -149,6 +164,18 @@ export default function SetupCommand() { subtitle={describeResumeTimeout(preferences.resumeTimeoutMs)} actions={settingsActions} /> + {error ? ( diff --git a/src/snip.tsx b/src/snip.tsx new file mode 100644 index 0000000..d483f29 --- /dev/null +++ b/src/snip.tsx @@ -0,0 +1,18 @@ +import { ChatView } from "./lib/components/ChatView"; + +/** + * Entrypoint for the "snip" command. + * + * Captures a region, then hands the image to a *new* chat. Nothing is sent on + * capture: a screenshot on its own is a question the user has not asked yet, so + * {@link ChatView} holds it until they say what they want to know about it. + * + * The view gates the capture on the model actually reporting vision support, so + * a text-only model explains itself rather than taking a screenshot nothing can + * read. + * + * Bind this to a global shortcut for one-key "snip and ask". + */ +export default function SnipCommand() { + return ; +} -- 2.54.0 From 5f359778b102324b8cf4d2cc7f2fb0a7756e9c3f Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:26:55 +0800 Subject: [PATCH 02/11] fix: take the launcher window out of the shot when snipping The snip was selecting a region around the launcher panel sitting in the middle of the screen. `activateEntrypoint` calls `showWindow()` when it activates a command from a closed launcher, so a view command's window is already up before any extension code runs. Capturing from inside the chat view could never have worked. - `snip` is now a `no-view` command. It calls `closeMainWindow()` first, waits out the 50ms hide that `ui-service.hpp` schedules, then captures with nothing on screen, then brings the window back by launching `chat` with the file. `settleWindow()` exists because a capture started the instant the close resolves could still find the window there. - The chat's action no longer captures inline. It launches `snip` and passes the session id, so there is one capture path, one capability check, and one place that deals with the window. - The hand-off relays through `snip` rather than going straight back to `chat` on purpose: `ActivateEntrypointOptions::toggleIfAlreadyActive` defaults to true, so launching the *already active* view command closes the window instead of reopening it. A launch cannot override that flag, but relaying through a different no-view command makes the toggle branch unreachable. - The action is hidden while streaming, since the hand-off unloads the view and would abandon an answer mid-sentence. - Vision is settled before the selector appears, so a model that cannot read images never costs a selection gesture. `no` and `unknown` get different HUD wording. - `lib/snip.ts` holds the hand-off contract; `lib/hooks/useSnip.ts` is gone, as nothing captures from a view any more. Verified: 252 tests, tsc, eslint, prettier, and vici build. The deployed manifest now reports `snip mode=no-view`. --- README.md | 16 ++- package.json | 4 +- src/__mocks__/@vicinae/api.ts | 23 ++++ src/__tests__/snip.test.ts | 57 +++++++++ src/chat.tsx | 15 ++- src/lib/components/ChatView.tsx | 205 ++++++++++++++++---------------- src/lib/hooks/useSnip.ts | 61 ---------- src/lib/hooks/useVision.ts | 47 +++----- src/lib/snip.ts | 55 +++++++++ src/snip.tsx | 95 +++++++++++++-- 10 files changed, 361 insertions(+), 217 deletions(-) create mode 100644 src/__tests__/snip.test.ts delete mode 100644 src/lib/hooks/useSnip.ts create mode 100644 src/lib/snip.ts diff --git a/README.md b/README.md index 6449dc6..17e7f9c 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ Hit a global shortcut, type a question, get a streamed answer without leaving wh - **Persistent conversations.** Every chat is saved locally. The sidebar lists them by recency, and reopening the command picks up where you left off, unless it has been idle longer than you allow. - **Readable code.** Fenced blocks render in a dark panel with the language named in a header, syntax colouring, and a Copy button. - **Tools.** Lam decides when to call them: current time, location context, web search, and page fetching. -- **Screenshots, two ways.** Snip a region into the conversation you are already in from the action panel, or run **Snip to Lam** to start a new chat about it. Neither is offered unless the current model can actually see images. +- **Screenshots, two ways.** Snip a region into the conversation you are already in from the action panel, or run **Snip to Lam** to start a new chat about it. The launcher window gets out of the way first, so it never ends up in the shot. Neither is offered unless the current model can actually see images. - **Chat only.** Lam answers questions. It does not touch your files or run commands. The one exception is capture, and it only ever happens because you pressed the key for it. ## Requirements @@ -106,7 +106,7 @@ vicinae 'vicinae://launch/lam-agent/chat?fallbackText=summarise%20this' | Command | Purpose | | --- | --- | | **Chat with Lam** | The chat surface. Bind this to your global shortcut. | -| **Snip to Lam** | Capture a region and start a new chat about it. Bind this to a second shortcut for one-key "snip and ask". | +| **Snip to Lam** | Capture a region and start a new chat about it. A `no-view` command, so the launcher window closes before the selector appears. Bind this to a second shortcut for one-key "snip and ask". | | **Chat Sessions** | Browse, reopen, and delete past conversations. | | **Set Up Lam Agent** | Verify the connection, list available models, and check whether the selected model can see images. | @@ -144,7 +144,7 @@ Web search results and fetched pages can be large. Lam truncates them before the ``` src/ chat.tsx # "chat" command entrypoint, thin wrapper - snip.tsx # "snip" command entrypoint, captures then chats + snip.tsx # "snip" command entrypoint, no-view: captures then hands over sessions.tsx # browse, reopen, delete conversations setup.tsx # connection check + live model list lib/ @@ -162,8 +162,8 @@ src/ code.ts # code fences and links, repaired preferences.ts # preferences, normalised hooks/useChat.ts # the chat state machine - hooks/useSnip.ts # capture -> pending attachment - hooks/useVision.ts # capability check, for gating snip actions + hooks/useVision.ts # capability check, for gating the snip action + snip.ts # the snip -> chat handoff contract components/ChatView.tsx # the chat surface ``` @@ -197,7 +197,11 @@ A few decisions worth knowing about before you change things: **Screen capture is owned, not borrowed.** There is no screenshot call anywhere in `@vicinae/api`, and `Clipboard` moves text, html, urls and a file path and nothing binary, so a bitmap can never be read back out of the clipboard either. The only way to get a region of the screen is to shell out, which is fair game because extensions run under Node. The useful consequence is that the extension picks the output path and reads the file it wrote, rather than guessing what some other application left on the clipboard. Cancel detection leans on the file for the same reason: every tool reports an abandoned selection differently, and at least one exits `0` on cancel, so whether an image actually landed on disk is the only signal they agree on. Snips go in the extension's support directory rather than a temp directory, because a stored conversation keeps the path and a path that survives a reboot is the difference between an old chat showing its screenshot and pointing at nothing. -**Image support is a fact from the API, not a guess from the model name, and it is cached.** `GET /api/tags` returns names, sizes and quantization levels and says nothing about capability, so the check is a `POST /api/show` that reads the `vision` flag. That is a round trip per model, so the answer is persisted under a key built from the host and model name, which means switching model invalidates it by construction. The state is a tri-state rather than a boolean: a failed request is `"unknown"`, never `"no"`, because telling someone their model cannot see when the network merely dropped is a false claim about their setup that sends them shopping for a model they do not need. The actions still fail closed, so only a confirmed `"yes"` offers a snip. +**Capture runs in a `no-view` command, because the chat view owns a window.** This is the part that is easy to get wrong. `activateEntrypoint` calls `showWindow()` when it activates a command from a closed launcher, so the window is already on screen before any extension code runs, and a region selector started from a view is dragging a rectangle around a panel parked in the middle of the screen. So `snip` is declared `no-view`, calls `closeMainWindow()` as its first statement, captures with nothing in the way, and only then brings the window back by launching `chat` with the file. That command is also the single capture path: the chat's own action launches `snip` rather than capturing inline, so there is one implementation and one place that deals with the window. + +The hand-off goes through `snip` specifically rather than straight to `chat` on purpose. `activateEntrypoint` treats a launch of the command that is *already active* as a toggle and closes the window instead of reopening it (`toggleIfAlreadyActive` defaults to true, and a launch cannot override it). Relaying through a different, no-view command means never asking for `chat` while `chat` is active, so the toggle branch is unreachable. The chat's own snip action is also hidden while a turn is streaming, because the hand-off unloads the view and would abandon an answer mid-sentence. + +**Image support is a fact from the API, not a guess from the model name, and it is cached.** `GET /api/tags` returns names, sizes and quantization levels and says nothing about capability, so the check is a `POST /api/show` that reads the `vision` flag. That is a round trip per model, so the answer is persisted under a key built from the host and model name, which means switching model invalidates it by construction. The state is a tri-state rather than a boolean: a failed request is `"unknown"`, never `"no"`, because telling someone their model cannot see when the network merely dropped is a false claim about their setup that sends them shopping for a model they do not need. The snip command settles this *before* the selector appears, so a model that cannot read images never costs the user a selection gesture, and the two outcomes get different HUD wording. The in-chat action still fails closed: only a confirmed `"yes"` offers it. **The capture is never a tool.** It stays in the action panel and on a keybinding, so it only ever runs because a person asked for it. A model that could decide to screenshot the screen is a different risk profile from one that answers questions, which is the same line the read-only tools already sit behind. diff --git a/package.json b/package.json index b841446..cfd9b83 100644 --- a/package.json +++ b/package.json @@ -19,8 +19,8 @@ "name": "snip", "title": "Snip to Lam", "subtitle": "Lam Agent", - "description": "Capture a region of the screen and start a new chat about it. Offered only when the model in preferences supports image input, so check with \"Set Up Lam Agent\" if the shortcut seems to do nothing.", - "mode": "view" + "description": "Capture a region of the screen and start a new chat about it. The launcher window closes first so it cannot end up in the shot. Offered only when the model in preferences supports image input.", + "mode": "no-view" }, { "name": "sessions", diff --git a/src/__mocks__/@vicinae/api.ts b/src/__mocks__/@vicinae/api.ts index 0e80da3..352d001 100644 --- a/src/__mocks__/@vicinae/api.ts +++ b/src/__mocks__/@vicinae/api.ts @@ -46,3 +46,26 @@ export function __setPreferenceValues(values: Record): void { export function getPreferenceValues>(): T { return preferenceOverride as T; } + +/** + * The runtime `environment`. Only `supportPath` is exercised, by the snip + * directory, and a stable fake path keeps those assertions independent of where + * the test happens to run. + */ +export const environment = { + supportPath: "/tmp/lam-agent-support", + assetsPath: "/tmp/lam-agent-assets", + extensionName: "lam-agent", + ownerOrAuthorName: "adrianbonpin", + commandName: "chat", + commandMode: "view" as const, + isDevelopment: true, + appearance: "dark" as const, + theme: "dark" as const, + textSize: "medium" as const, + launchType: "userInitiated" as const, + raycastVersion: "0.0.0", + vicinaeVersion: { tag: "test", commit: "test" }, + isRaycast: false, + canAccess: () => true, +}; diff --git a/src/__tests__/snip.test.ts b/src/__tests__/snip.test.ts new file mode 100644 index 0000000..e9f34f1 --- /dev/null +++ b/src/__tests__/snip.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from "vitest"; +import { readSnipContext, snipDirectory } from "../lib/snip"; + +/** + * The snip handoff crosses a command boundary as a serialized launch context, so + * every field arrives as unknown. This is the boundary where a wrong type would + * otherwise reach the file system as a path. + */ + +describe("readSnipContext", () => { + it("reads the fields the snip command writes", () => { + expect( + readSnipContext({ + snipPath: "/tmp/snip.png", + snipError: "no tool", + sessionId: "abc123", + }), + ).toEqual({ snipPath: "/tmp/snip.png", snipError: "no tool", sessionId: "abc123" }); + }); + + it("returns nothing for a launch with no context at all", () => { + expect(readSnipContext(undefined)).toEqual({}); + expect(readSnipContext(null)).toEqual({}); + }); + + it("ignores a context that is not an object", () => { + expect(readSnipContext("snip.png")).toEqual({}); + expect(readSnipContext(42)).toEqual({}); + expect(readSnipContext(["/tmp/snip.png"])).toEqual({}); + }); + + it("drops a value that is not a string rather than passing it on", () => { + // A number or object here would reach `readAttachment` as a path. + expect(readSnipContext({ snipPath: 42, sessionId: { id: "x" } })).toEqual({}); + }); + + it("treats an empty string as absent", () => { + expect(readSnipContext({ snipPath: "", sessionId: "" })).toEqual({}); + }); + + it("ignores unknown keys, so a stray field cannot widen the contract", () => { + expect(readSnipContext({ snipPath: "/tmp/snip.png", somethingElse: "/etc/passwd" })).toEqual({ + snipPath: "/tmp/snip.png", + }); + }); + + it("reads a payload that only names a conversation, which is a cancelled snip", () => { + expect(readSnipContext({ sessionId: "abc123" })).toEqual({ sessionId: "abc123" }); + }); +}); + +describe("snipDirectory", () => { + it("keeps snips under the extension's support directory, not a temp directory", () => { + // A stored conversation holds the path, so it has to survive a reboot. + expect(snipDirectory()).toBe("/tmp/lam-agent-support/snips"); + }); +}); diff --git a/src/chat.tsx b/src/chat.tsx index d048b25..848f8a2 100644 --- a/src/chat.tsx +++ b/src/chat.tsx @@ -1,5 +1,6 @@ import { type LaunchProps } from "@vicinae/api"; import { ChatView } from "./lib/components/ChatView"; +import { readSnipContext } from "./lib/snip"; /** * Entrypoint for the "chat" command. @@ -11,7 +12,19 @@ import { ChatView } from "./lib/components/ChatView"; * launcher-and-start-typing behaviour. `fallbackText` is populated when the * command is opened through a deeplink carrying text, so * `vicinae://launch/lam-agent/chat?fallbackText=...` arrives pre-filled. + * + * It is also the command the `snip` command returns through, carrying a captured + * file and, when the snip came from an open conversation, that conversation's id. */ export default function ChatCommand(props: LaunchProps) { - return ; + const snip = readSnipContext(props.launchContext); + + return ( + + ); } diff --git a/src/lib/components/ChatView.tsx b/src/lib/components/ChatView.tsx index 00d7aca..efc14dd 100644 --- a/src/lib/components/ChatView.tsx +++ b/src/lib/components/ChatView.tsx @@ -1,14 +1,14 @@ -import { Action, ActionPanel, Icon, List } from "@vicinae/api"; +import { Action, ActionPanel, Icon, LaunchType, List, launchCommand } from "@vicinae/api"; import { useCallback, useEffect, useMemo, useState } from "react"; +import { readAttachment } from "../attachments"; import { useChat } from "../hooks/useChat"; -import { useSnip } from "../hooks/useSnip"; import { useVisionSupport } from "../hooks/useVision"; import { buildThreadMarkdown, type TranscriptOptions } from "../markdown"; import { getLamPreferences } from "../preferences"; import { deleteSession, listSessions, loadSession, summarizeSession } from "../sessions"; import { buildThread } from "../thread"; import type { PendingAttachment, Session, SessionSummary } from "../types"; -import { recencyGroup, truncate } from "../utils"; +import { errorMessage, recencyGroup, truncate } from "../utils"; /** * The chat surface: conversations on the left, the open conversation on the right. @@ -47,6 +47,9 @@ import { recencyGroup, truncate } from "../utils"; * selection to the first row on every model change * (`GenericListView.selectFirstOnReset`), and streaming changes the model on * every flush, so ordering by recency keeps the chat in progress near the top. + * - Capturing a region does **not** happen here. It is delegated to the `snip` + * command, because this view owns a window and the window would be in the + * shot. See {@link ChatViewProps}. */ const EMPTY_TRANSCRIPT = "**Ask Lam anything**\n\nType a question in the search bar and press Return."; @@ -54,7 +57,7 @@ const EMPTY_TRANSCRIPT = "**Ask Lam anything**\n\nType a question in the search /** Sent when a snip is attached and Return is pressed before anything is typed. */ const DEFAULT_IMAGE_PROMPT = "What is in this image?"; -/** The reminder shown while a snip is captured but not yet sent. */ +/** The reminder shown while a snip is attached but not yet sent. */ const PENDING_NOTE = "_Screenshot attached. Ask a question about it, or press Return to send it as it is._"; export interface ChatViewProps { @@ -62,10 +65,12 @@ export interface ChatViewProps { initialSession?: Session; /** Text handed over by a deeplink or fallback command. */ initialPrompt?: string; - /** A snip that has already been captured, used when a view is pushed with one. */ - initialAttachment?: PendingAttachment; - /** Captures a snip on mount, which is what the standalone command does. */ - startWithSnip?: boolean; + /** A conversation to reload by id, which is how the snip command hands one back. */ + initialSessionId?: string; + /** A captured image to read in, which is how the snip command hands one over. */ + initialAttachmentPath?: string; + /** Something the launching command needs to say, such as a failed capture. */ + launchNote?: string; } interface SessionRow { @@ -75,27 +80,28 @@ interface SessionRow { isActive: boolean; } -export function ChatView({ initialSession, initialPrompt, initialAttachment, startWithSnip }: ChatViewProps) { +export function ChatView({ + initialSession, + initialPrompt, + initialSessionId, + initialAttachmentPath, + launchNote, +}: ChatViewProps) { const preferences = useMemo(() => getLamPreferences(), []); const chat = useChat({ preferences, initialSession }); const { replaceSession } = chat; - const { isSniping, snip } = useSnip(); + const { canSeeImages } = useVisionSupport(preferences); - const { - support: vision, - canSeeImages, - isChecking: isCheckingVision, - recheck, - } = useVisionSupport(preferences); + /** A launch that hands something over has to wait for it before it can render. */ + const needsPreparation = Boolean(initialSessionId) || Boolean(initialAttachmentPath); const [input, setInput] = useState(initialPrompt ?? ""); const [summaries, setSummaries] = useState(undefined); - // A snip command starts a new chat on purpose, so the idle-resume below is - // skipped and the view must not wait for it. - const [resumed, setResumed] = useState(Boolean(initialSession) || Boolean(startWithSnip)); + const [prepared, setPrepared] = useState(!needsPreparation); + const [resumed, setResumed] = useState(Boolean(initialSession)); // A captured image is held here until the user says what they want to know // about it. Nothing is sent to the model on capture alone. - const [pending, setPending] = useState(initialAttachment); + const [pending, setPending] = useState(undefined); const [snipNote, setSnipNote] = useState(undefined); const activeId = chat.session.id; @@ -111,18 +117,55 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta reload(); }, [reload, chat.session.updatedAt]); + /** + * Restores whatever the launching command handed over. + * + * This is the second half of the snip handoff: the capture command closed the + * window to take the shot, so it comes back here with a file to read and, when + * the snip was started from a conversation, the id of the conversation to + * return to. Reloading by id rather than trusting "most recent" matters + * because the snip itself is not a turn, so nothing was saved in between. + */ + useEffect(() => { + if (!needsPreparation) return; + + let cancelled = false; + + void (async () => { + if (initialSessionId) { + const session = await loadSession(initialSessionId); + if (!cancelled && session) replaceSession(session); + } + + if (initialAttachmentPath) { + try { + const attachment = await readAttachment(initialAttachmentPath); + if (!cancelled) setPending(attachment); + } catch (thrown) { + if (!cancelled) setSnipNote(`**Could not open the screenshot:** ${errorMessage(thrown)}`); + } + } + + if (!cancelled) setPrepared(true); + })(); + + return () => { + cancelled = true; + }; + }, [needsPreparation, initialSessionId, initialAttachmentPath, replaceSession]); + /** * Reopening the command continues the most recent conversation, but only if it * was used recently. Past the configured idle window it starts a fresh chat * instead. The previous conversation is never removed, it just drops down the * sidebar. Setting the preference to "Never" keeps the last chat open forever. * - * The snip command opts out entirely: it exists to start a new chat about one - * screenshot, and quietly continuing the previous conversation would bury the - * image in an unrelated thread. + * A hand-off launch opts out: it either names the conversation to return to or + * deliberately starts a new one, and quietly continuing some other chat would + * bury the screenshot in an unrelated thread. */ useEffect(() => { - if (initialSession || startWithSnip) return; + if (initialSession || needsPreparation) return; let cancelled = false; void (async () => { @@ -144,7 +187,7 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta return () => { cancelled = true; }; - }, [initialSession, startWithSnip, preferences, replaceSession]); + }, [initialSession, needsPreparation, preferences, replaceSession]); const thread = useMemo( () => @@ -170,37 +213,15 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta * already looking. */ const threadMarkdown = useMemo(() => { - const notes: string[] = []; - - // Only the standalone command has to explain a model that cannot see; inside - // the chat the action is simply absent. A failed check is worded separately - // from a text-only model, because saying "your model cannot see" when the - // request merely failed is a claim about the user's setup that is not true. - if (startWithSnip && !isCheckingVision && vision !== "yes") { - notes.push( - vision === "no" - ? `**${preferences.model} cannot see images.** Choose a model with vision support in preferences, and this command becomes a one-key snip.` - : `**Could not check whether ${preferences.model} can see images.** Use Check Again below, or snip from the chat's action panel once the connection is back.`, - ); - } - - if (snipNote) notes.push(snipNote); - if (pending) notes.push(PENDING_NOTE); + const notes = [launchNote, snipNote, pending ? PENDING_NOTE : undefined].filter( + (note): note is string => Boolean(note), + ); const transcript = thread.length === 0 ? EMPTY_TRANSCRIPT : buildThreadMarkdown(thread, transcriptOptions); return [...notes, transcript].join("\n\n"); - }, [ - startWithSnip, - isCheckingVision, - vision, - preferences.model, - snipNote, - pending, - thread, - transcriptOptions, - ]); + }, [launchNote, snipNote, pending, thread, transcriptOptions]); const rows: SessionRow[] = useMemo(() => { const base = summaries ?? []; @@ -259,30 +280,6 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta [activeId, chat, reload], ); - /** - * Captures a region and holds it until a question is sent with it. - * - * A cancelled selection deliberately says nothing: the user pressed Escape and - * telling them so is noise. A failed capture has to explain itself, because - * the usual cause is a capture tool that is not installed. - */ - const takeSnip = useCallback(async () => { - setSnipNote(undefined); - const outcome = await snip(); - - if (outcome.status === "captured") setPending(outcome.attachment); - else if (outcome.status === "failed") - setSnipNote(`**Could not take a screenshot:** ${outcome.message}`); - }, [snip]); - - // The standalone command captures on arrival. Waiting for the capability check - // matters here: capturing a screenshot only to be told the model cannot read - // it would throw away the selection the user just made. - useEffect(() => { - if (!startWithSnip || !canSeeImages || initialAttachment) return; - void takeSnip(); - }, [startWithSnip, canSeeImages, initialAttachment, takeSnip]); - /** The snip a "show me the screenshot" action would open: the pending one, else the newest. */ const latestAttachment = pending ?? @@ -320,20 +317,33 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta ); /** - * Present only when Ollama has confirmed the model reports `vision`. + * Hands the capture to the `snip` command instead of doing it here. * - * Offering it otherwise would let the user frame a careful screenshot and - * then have a model that cannot see answer as though it had, which is a worse - * failure than the action not being there. + * Two reasons, both structural. The window has to be out of the way, and only + * the snip command is positioned to close it before the selector appears. And + * launching a different command is what makes the hand-off safe: Vicinae's + * `activateEntrypoint` treats a launch of the *currently active* view command + * as a toggle and closes the window instead of reopening it, so relaying + * through `snip` avoids ever asking for `chat` while `chat` is active. + * + * Absent while streaming, because the hand-off unloads this view and would + * abandon an answer mid-sentence. */ - const snipAction = canSeeImages ? ( - void takeSnip()} - /> - ) : null; + const snipAction = + canSeeImages && !chat.isStreaming ? ( + + void launchCommand({ + name: "snip", + type: LaunchType.UserInitiated, + context: { sessionId: chat.session.id }, + }) + } + /> + ) : null; // A screenshot is a file, so it opens in the system image viewer. Without this // the only way to see what was sent would be to find it on disk by hand. @@ -341,13 +351,6 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta ) : null; - // Only the standalone command reaches a state with nowhere to go: a failed - // capability check leaves it with no snip and no action that would start one. - const checkAgainAction = - startWithSnip && !isCheckingVision && vision === "unknown" ? ( - - ) : null; - /** * The primary action adapts to the search bar. With text in it, Return sends; * with it empty (where sending would be a no-op anyway), Return opens the @@ -366,7 +369,6 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta {newChatAction} {snipAction} {openSnipAction} - {checkAgainAction} ; } - const busy = chat.isStreaming || isSniping; - const placeholder = chat.status ? `${chat.status}. Press ⏎ to stop` : "Lam is thinking. Press ⏎ to stop"; @@ -394,7 +394,7 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta return ( {chat.isStreaming ? stopAction : sendAction} {newChatAction} {snipAction} {openSnipAction} - {checkAgainAction} } > @@ -426,7 +423,7 @@ export function ChatView({ initialSession, initialPrompt, initialAttachment, sta // Title only. Raycast's sidebar shows nothing else, and stacking a // subtitle plus a timestamp on a 38px row elided the title down to // a few characters. The check icon is what marks the open chat. - detail={} + detail={} actions={rowActions(row)} /> ))} diff --git a/src/lib/hooks/useSnip.ts b/src/lib/hooks/useSnip.ts deleted file mode 100644 index dfef5d0..0000000 --- a/src/lib/hooks/useSnip.ts +++ /dev/null @@ -1,61 +0,0 @@ -import { environment } from "@vicinae/api"; -import { join } from "node:path"; -import { useCallback, useState } from "react"; -import { readAttachment } from "../attachments"; -import { NoCaptureToolError, captureRegion } from "../capture"; -import type { PendingAttachment } from "../types"; -import { errorMessage } from "../utils"; - -/** - * Taking a snip, from the UI's point of view. - * - * The outcome is a discriminated union rather than `null`, because "the user - * pressed Escape" and "there is no capture tool on this machine" want opposite - * responses: the first should say nothing at all, the second has to explain - * itself. A hook rather than a bare function so both entry points (the chat - * action and the standalone command) share one implementation and one spinner. - */ - -export type SnipOutcome = - | { status: "captured"; attachment: PendingAttachment } - | { status: "cancelled" } - | { status: "failed"; message: string }; - -export interface UseSnip { - isSniping: boolean; - snip: () => Promise; -} - -/** - * Snips live in the extension's own support directory rather than a temp - * directory: a session stores the path, and a path that survives a reboot is - * the difference between an old conversation showing its screenshot and - * referring to a file that no longer exists. - */ -export function snipDirectory(): string { - return join(environment.supportPath, "snips"); -} - -export function useSnip(): UseSnip { - const [isSniping, setIsSniping] = useState(false); - - const snip = useCallback(async (): Promise => { - setIsSniping(true); - - try { - const capture = await captureRegion({ outputDir: snipDirectory() }); - - // No image on disk means the region selection was abandoned. - if (!capture) return { status: "cancelled" }; - - return { status: "captured", attachment: await readAttachment(capture.path) }; - } catch (thrown) { - const hint = thrown instanceof NoCaptureToolError ? ` ${thrown.hint}` : ""; - return { status: "failed", message: `${errorMessage(thrown)}${hint}` }; - } finally { - setIsSniping(false); - } - }, []); - - return { isSniping, snip }; -} diff --git a/src/lib/hooks/useVision.ts b/src/lib/hooks/useVision.ts index bfa66ec..1deaada 100644 --- a/src/lib/hooks/useVision.ts +++ b/src/lib/hooks/useVision.ts @@ -1,40 +1,29 @@ -import { useCallback, useEffect, useRef, useState } from "react"; +import { useEffect, useState } from "react"; import type { ResolvedPreferences } from "../preferences"; -import { visionSupport, type VisionSupport } from "../vision"; +import { visionSupport } from "../vision"; /** - * Whether the current model accepts images, for gating the snip actions. + * Whether the current model accepts images, for gating the snip action. * - * The state is the same tri-state the cached lookup returns, not a boolean. A - * failed check and a text-only model want different words: claiming a model - * "cannot see images" when the request merely failed is a false statement about - * the user's setup, and it sends them looking for a different model for no - * reason. + * Fails closed: only a confirmed `"yes"` turns the feature on. A model that + * cannot see would otherwise accept a screenshot and answer as though it had, + * which is worse than the action not being offered. * - * The actions themselves still fail closed: only a confirmed `"yes"` offers a - * snip. + * The wording for a *failed* check lives in the snip command rather than here, + * because that is the only place with somewhere to say it (a HUD). This hook + * only decides whether to show an action. */ export interface VisionState { - /** `"unknown"` means the check failed, which is not the same as `"no"`. */ - support: VisionSupport; /** True only once Ollama has confirmed the model reports the `vision` capability. */ canSeeImages: boolean; /** True while an answer for this model is still outstanding. */ isChecking: boolean; - /** Asks again, ignoring the cache. */ - recheck: () => void; } export function useVisionSupport(preferences: ResolvedPreferences): VisionState { - const [support, setSupport] = useState("unknown"); + const [canSeeImages, setCanSeeImages] = useState(false); const [isChecking, setIsChecking] = useState(true); - const [attempt, setAttempt] = useState(0); - - // Set by `recheck` and consumed by the next run. A ref rather than a - // dependency so that a manual retry does not also make every later model - // change bypass the cache. - const refreshRef = useRef(false); useEffect(() => { let cancelled = false; @@ -47,14 +36,11 @@ export function useVisionSupport(preferences: ResolvedPreferences): VisionState apiKey: preferences.apiKey, model: preferences.model, signal: controller.signal, - refresh: refreshRef.current, }) - .then((result) => { - if (cancelled) return; - setSupport(result); + .then((support) => { + if (!cancelled) setCanSeeImages(support === "yes"); }) .finally(() => { - refreshRef.current = false; if (!cancelled) setIsChecking(false); }); @@ -62,12 +48,7 @@ export function useVisionSupport(preferences: ResolvedPreferences): VisionState cancelled = true; controller.abort(); }; - }, [preferences.host, preferences.apiKey, preferences.model, attempt]); + }, [preferences.host, preferences.apiKey, preferences.model]); - const recheck = useCallback(() => { - refreshRef.current = true; - setAttempt((value) => value + 1); - }, []); - - return { support, canSeeImages: support === "yes", isChecking, recheck }; + return { canSeeImages, isChecking }; } diff --git a/src/lib/snip.ts b/src/lib/snip.ts new file mode 100644 index 0000000..fcbe62e --- /dev/null +++ b/src/lib/snip.ts @@ -0,0 +1,55 @@ +import { environment } from "@vicinae/api"; +import { join } from "node:path"; + +/** + * The seam between the snip command and the chat command. + * + * The capture cannot happen inside the chat view. Vicinae shows the launcher + * window when it activates a command, so a region selector started from a view + * is dragging around a panel parked in the middle of the screen. Capturing from + * a `no-view` command and handing the file over is what keeps the window out of + * the way, which means the two commands need a small agreed contract. + * + * Imports Vicinae for `environment.supportPath` only, and nothing else in the + * logic layer depends on it. + */ + +/** What the snip command passes to the chat command through `launchCommand`. */ +export interface SnipLaunchContext { + /** Absolute path of the captured PNG. Absent when the capture failed. */ + snipPath?: string; + /** Why a capture failed, so the chat can explain itself rather than look empty. */ + snipError?: string; + /** The conversation the snip was started from, restored so it is not lost. */ + sessionId?: string; +} + +/** + * Snips live in the extension's own support directory rather than a temp + * directory: a session stores the path, and a path that survives a reboot is the + * difference between an old conversation showing its screenshot and pointing at + * nothing. + */ +export function snipDirectory(): string { + return join(environment.supportPath, "snips"); +} + +/** + * Reads the handoff payload. + * + * A launch context is whatever the launching command put there, so every field + * is checked rather than trusted: a wrong type would otherwise reach the file + * system as a path. + */ +export function readSnipContext(launchContext: unknown): SnipLaunchContext { + if (!launchContext || typeof launchContext !== "object") return {}; + + const { snipPath, snipError, sessionId } = launchContext as Record; + const asString = (value: unknown) => (typeof value === "string" && value.length > 0 ? value : undefined); + + return { + snipPath: asString(snipPath), + snipError: asString(snipError), + sessionId: asString(sessionId), + }; +} diff --git a/src/snip.tsx b/src/snip.tsx index d483f29..c3bffa1 100644 --- a/src/snip.tsx +++ b/src/snip.tsx @@ -1,18 +1,93 @@ -import { ChatView } from "./lib/components/ChatView"; +import { LaunchType, closeMainWindow, launchCommand, showHUD, type LaunchProps } from "@vicinae/api"; +import { NoCaptureToolError, captureRegion } from "./lib/capture"; +import { getLamPreferences } from "./lib/preferences"; +import { readSnipContext, snipDirectory } from "./lib/snip"; +import { errorMessage } from "./lib/utils"; +import { visionSupport } from "./lib/vision"; + +/** + * Vicinae's `closeMainWindow` defers the actual hide to a 50ms timer + * (`closeWindow(settings, 50ms)` in `ui-service.hpp`), so a capture started the + * instant the call resolves can still find the window on screen and select a + * region around it. Waiting a little past that timer is the difference between + * "the window is closed" and "the window is usually closed". + */ +const WINDOW_SETTLE_MS = 100; + +function settleWindow(): Promise { + return new Promise((resolve) => setTimeout(resolve, WINDOW_SETTLE_MS)); +} /** * Entrypoint for the "snip" command. * - * Captures a region, then hands the image to a *new* chat. Nothing is sent on - * capture: a screenshot on its own is a question the user has not asked yet, so - * {@link ChatView} holds it until they say what they want to know about it. + * `no-view`, and it has to be. Vicinae calls `showWindow()` when it activates a + * command from a closed launcher, so the window is up by the time any extension + * code runs; capturing from a view means selecting a region around a panel + * sitting in the middle of the screen. This command closes the window first, + * captures with nothing in the way, and only then hands the image to the chat, + * which is the command that brings the window back. * - * The view gates the capture on the model actually reporting vision support, so - * a text-only model explains itself rather than taking a screenshot nothing can - * read. + * It is also the *only* capture path. The chat's own action launches this command + * rather than capturing inline, so there is one implementation, one capability + * check, and one place where the window is dealt with. * - * Bind this to a global shortcut for one-key "snip and ask". + * Bind it to a global shortcut for one-key "snip and ask". */ -export default function SnipCommand() { - return ; +export default async function SnipCommand(props: LaunchProps) { + const preferences = getLamPreferences(); + const context = readSnipContext(props.launchContext); + + // Before anything else. Everything below is pointless if the selector is + // competing with a window for the middle of the screen. + await closeMainWindow(); + await settleWindow(); + + // Settled before the capture, because taking a screenshot for a model that + // cannot read it throws away the selection gesture the user just performed. + // "unknown" says so in those words: a dropped check is not a verdict on the + // model, and reporting it as one would send the user shopping for another. + const vision = await visionSupport({ + host: preferences.host, + apiKey: preferences.apiKey, + model: preferences.model, + }); + + if (vision !== "yes") { + await showHUD( + vision === "no" + ? `${preferences.model} cannot see images. Pick a vision model in preferences.` + : `Could not check whether ${preferences.model} can see images. Run Set Up Lam Agent.`, + ); + return; + } + + let snipPath: string | undefined; + let snipError: string | undefined; + + try { + const capture = await captureRegion({ outputDir: snipDirectory() }); + snipPath = capture?.path; + } catch (thrown) { + snipError = + thrown instanceof NoCaptureToolError ? `${errorMessage(thrown)} ${thrown.hint}` : errorMessage(thrown); + } + + // An abandoned selection says nothing at all: the user pressed Escape. But a + // snip started from an open conversation has to come back, because the window + // is now closed and the hotkey is otherwise the only way back. + if (!snipPath && !snipError && !context.sessionId) return; + + // Only the fields that exist are sent. `launchCommand` types its context as a + // string map, and a key left undefined carries no information anyway. + const payload: Record = {}; + if (context.sessionId) payload.sessionId = context.sessionId; + if (snipPath) payload.snipPath = snipPath; + if (snipError) payload.snipError = snipError; + + await launchCommand({ + name: "chat", + type: LaunchType.UserInitiated, + context: payload, + }); } -- 2.54.0 From ada98968ac5ad2b45984e0b33421386e2598f707 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:28:57 +0800 Subject: [PATCH 03/11] fix: tell a refused screen capture apart from a cancelled one The snip failed with "could not create image from rect". That message is macOS's signature for a denied Screen Recording permission, not a cancel, and it is the only thing distinguishing the two: both write no file, and screencapture -i has been reported to exit 0 when the permission is missing. The previous code could not tell them apart, so a permission problem would either look like Escape or, as here, surface as a bare "Command failed". - `RunCommand` now returns stdout and stderr, and `captureRegion` collects every message a tool produced, clean exit or not. - Success is still decided by the file landing on disk, so a tool that warns on stderr and writes the image anyway is still a success. - Failure is triaged from stderr: the macOS denial message becomes `CapturePermissionError` with the setting to change (TCC attributes the request to Vicinae, since it launched the extension runtime, so the permission belongs to Vicinae and needs an app restart). Any other complaint becomes a `CaptureToolError` rather than being swallowed. Silence with no image is a cancel, which is the common case. - `describeCaptureFailure` in `lib/snip.ts` keeps both actionable hints, which the old catch clause dropped for everything but a missing tool. Diagnosed from the process tree: the extension runtime (pid 66329) is a child of Vicinae (pid 66327), so the grant goes on Vicinae. Verified: 257 tests, tsc, eslint, prettier, and vici build. --- README.md | 8 +- src/__tests__/capture.test.ts | 166 +++++++++++++++++++++++++++------- src/lib/capture.ts | 123 +++++++++++++++++++++---- src/lib/snip.ts | 19 ++++ src/snip.tsx | 8 +- 5 files changed, 268 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 17e7f9c..3cfbf25 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,10 @@ Vicinae has no screenshot API, so the extension shells out. It auto-detects the If none is found, the chat says so and names what to install. Nothing to configure: the detection happens per platform at capture time. +**On macOS, Vicinae itself needs Screen Recording permission.** macOS attributes a capture request to the app responsible for launching the process that made it, which is Vicinae rather than the extension, so the grant goes on Vicinae: **System Settings, Privacy & Security, Screen & System Audio Recording**, then quit and reopen Vicinae (a TCC change does not apply to a running app). + +Without it, `screencapture` prints `could not create image from rect` and writes nothing. That is awkward, because a cancelled selection also writes nothing, and `screencapture -i` has been reported to exit `0` when the permission is missing. So the file alone cannot tell the two apart. `capture.ts` therefore reads stderr as well: that specific message becomes a permission error naming the setting to change, and only silence with no image is treated as a cancel. + ## Install ```sh @@ -195,7 +199,9 @@ A few decisions worth knowing about before you change things: **Sessions are capped twice over**, once per turn and once per session, because `LocalStorage` has no documented ceiling. -**Screen capture is owned, not borrowed.** There is no screenshot call anywhere in `@vicinae/api`, and `Clipboard` moves text, html, urls and a file path and nothing binary, so a bitmap can never be read back out of the clipboard either. The only way to get a region of the screen is to shell out, which is fair game because extensions run under Node. The useful consequence is that the extension picks the output path and reads the file it wrote, rather than guessing what some other application left on the clipboard. Cancel detection leans on the file for the same reason: every tool reports an abandoned selection differently, and at least one exits `0` on cancel, so whether an image actually landed on disk is the only signal they agree on. Snips go in the extension's support directory rather than a temp directory, because a stored conversation keeps the path and a path that survives a reboot is the difference between an old chat showing its screenshot and pointing at nothing. +**Screen capture is owned, not borrowed.** There is no screenshot call anywhere in `@vicinae/api`, and `Clipboard` moves text, html, urls and a file path and nothing binary, so a bitmap can never be read back out of the clipboard either. The only way to get a region of the screen is to shell out, which is fair game because extensions run under Node. The useful consequence is that the extension picks the output path and reads the file it wrote, rather than guessing what some other application left on the clipboard. + +Deciding whether a capture *worked* takes two signals, not one. The image landing on disk is what makes it a success, because the backends disagree about exit codes and at least one reports success on a cancelled selection. But the file cannot distinguish a cancel from a refused permission, since both leave nothing behind and macOS can exit `0` on the refusal, so the refusal is recognised by its stderr message instead. Silence with no image is the user pressing Escape; a complaint with no image is a failure the user is told about. Snips go in the extension's support directory rather than a temp directory, because a stored conversation keeps the path and a path that survives a reboot is the difference between an old chat showing its screenshot and pointing at nothing. **Capture runs in a `no-view` command, because the chat view owns a window.** This is the part that is easy to get wrong. `activateEntrypoint` calls `showWindow()` when it activates a command from a closed launcher, so the window is already on screen before any extension code runs, and a region selector started from a view is dragging a rectangle around a panel parked in the middle of the screen. So `snip` is declared `no-view`, calls `closeMainWindow()` as its first statement, captures with nothing in the way, and only then brings the window back by launching `chat` with the file. That command is also the single capture path: the chat's own action launches `snip` rather than capturing inline, so there is one implementation and one place that deals with the window. diff --git a/src/__tests__/capture.test.ts b/src/__tests__/capture.test.ts index 9e31fc4..8d9b7c0 100644 --- a/src/__tests__/capture.test.ts +++ b/src/__tests__/capture.test.ts @@ -4,6 +4,8 @@ import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { CAPTURE_BACKENDS, + CapturePermissionError, + CaptureToolError, NoCaptureToolError, captureRegion, selectBackend, @@ -23,6 +25,11 @@ function backendNamed(name: string): CaptureBackend { return backend; } +/** A tool that ran, said nothing, and produced no image. */ +function silent(): { stdout: Buffer; stderr: string } { + return { stdout: Buffer.alloc(0), stderr: "" }; +} + const tempDirs: string[] = []; async function tempDir(): Promise { @@ -89,7 +96,7 @@ describe("backend commands", () => { const calls: Array<{ tool: string; args: string[] }> = []; const run = vi.fn(async (tool: string, args: string[]) => { calls.push({ tool, args }); - return tool === "slurp" ? Buffer.from("120,340 800x600\n") : Buffer.alloc(0); + return tool === "slurp" ? { stdout: Buffer.from("120,340 800x600\n"), stderr: "" } : silent(); }); await backendNamed("grim + slurp").capture({ output: "/tmp/snip.png", run }); @@ -101,7 +108,7 @@ describe("backend commands", () => { }); it("does not shell out to grim when the selection was cancelled", async () => { - const run = vi.fn(async () => Buffer.from("")); + const run = vi.fn(async () => silent()); await backendNamed("grim + slurp").capture({ output: "/tmp/snip.png", run }); @@ -120,7 +127,7 @@ describe("backend commands", () => { const output = join(dir, "snip.png"); const png = Buffer.from([0x89, 0x50, 0x4e, 0x47]); - await backendNamed("flameshot").capture({ output, run: async () => png }); + await backendNamed("flameshot").capture({ output, run: async () => ({ stdout: png, stderr: "" }) }); expect(await readFile(output)).toEqual(png); }); @@ -129,13 +136,13 @@ describe("backend commands", () => { const dir = await tempDir(); const output = join(dir, "snip.png"); - await backendNamed("flameshot").capture({ output, run: async () => Buffer.alloc(0) }); + await backendNamed("flameshot").capture({ output, run: async () => silent() }); await expect(readFile(output)).rejects.toThrow(); }); it("asks screencapture for an interactive region and no sound", async () => { - const run = vi.fn(async () => Buffer.alloc(0)); + const run = vi.fn(async () => silent()); await backendNamed("screencapture").capture({ output: "/tmp/snip.png", run }); @@ -144,6 +151,14 @@ describe("backend commands", () => { }); describe("captureRegion", () => { + /** A `run` that writes a real file, which is what the success path checks for. */ + function writingRun(bytes = Buffer.from([0x89, 0x50])) { + return async (_tool: string, args: string[]) => { + await writeFile(args[args.length - 1], bytes); + return silent(); + }; + } + it("writes into the directory it was given and reports the path", async () => { const dir = await tempDir(); const output = join(dir, "snip-20261004-145926.png"); @@ -153,23 +168,36 @@ describe("captureRegion", () => { platform: "darwin", now: new Date(2026, 9, 4, 14, 59, 26), hasCommand: pathWith("screencapture"), - run: async (_tool, args) => { - await writeFile(args[args.length - 1], Buffer.from([0x89, 0x50])); - return Buffer.alloc(0); - }, + run: writingRun(Buffer.from([0x89, 0x50])), }); expect(result).toEqual({ path: output, bytes: 2, backend: "screencapture" }); }); - it("returns null when the user backed out and no image was written", async () => { + it("creates the output directory on first use", async () => { + const dir = await tempDir(); + const nested = join(dir, "snips"); + const now = new Date(2026, 9, 4, 14, 59, 26); + + await captureRegion({ + outputDir: nested, + platform: "darwin", + now, + hasCommand: pathWith("screencapture"), + run: writingRun(Buffer.from([0x89])), + }); + + expect((await readFile(join(nested, snipFileName(now)))).length).toBeGreaterThan(0); + }); + + it("returns null when the user backed out and said nothing", async () => { const dir = await tempDir(); const result = await captureRegion({ outputDir: dir, platform: "darwin", hasCommand: pathWith("screencapture"), - run: async () => Buffer.alloc(0), + run: async () => silent(), }); expect(result).toBeNull(); @@ -184,15 +212,104 @@ describe("captureRegion", () => { outputDir: dir, platform: "darwin", hasCommand: pathWith("screencapture"), - run: async (_tool, args) => { - await writeFile(args[args.length - 1], Buffer.alloc(0)); - return Buffer.alloc(0); - }, + run: writingRun(Buffer.alloc(0)), }); expect(result).toBeNull(); }); + it("still succeeds when the tool also complained on stderr", async () => { + // Some tools warn on stderr and write the image anyway. The file is the + // source of truth, so a warning must not turn a good capture into an error. + const dir = await tempDir(); + + const result = await captureRegion({ + outputDir: dir, + platform: "darwin", + hasCommand: pathWith("screencapture"), + run: async (_tool, args) => { + await writeFile(args[args.length - 1], Buffer.from([0x89])); + return { stdout: Buffer.alloc(0), stderr: "warning: something\n" }; + }, + }); + + expect(result?.bytes).toBe(1); + }); + + describe("macOS Screen Recording permission", () => { + // The message is the only difference between a refused permission and a + // cancelled selection: both leave no file behind, and screencapture has been + // reported to exit 0 when the permission is missing. + const denial = "could not create image from rect\n"; + + it("reports a permission problem instead of a cancel when the tool exits non-zero", async () => { + const dir = await tempDir(); + + await expect( + captureRegion({ + outputDir: dir, + platform: "darwin", + hasCommand: pathWith("screencapture"), + run: async () => { + throw new CaptureToolError("screencapture", denial); + }, + }), + ).rejects.toThrow(CapturePermissionError); + }); + + it("reports a permission problem even when the tool exits 0", async () => { + // The documented exit-0 shape. Without reading stderr this is + // indistinguishable from Escape, which is how a silently useless feature + // gets shipped. + const dir = await tempDir(); + + await expect( + captureRegion({ + outputDir: dir, + platform: "darwin", + hasCommand: pathWith("screencapture"), + run: async () => ({ stdout: Buffer.alloc(0), stderr: denial }), + }), + ).rejects.toThrow(CapturePermissionError); + }); + + it("names Vicinae rather than the extension, because that is who holds the grant", async () => { + const dir = await tempDir(); + const attempt = () => + captureRegion({ + outputDir: dir, + platform: "darwin", + hasCommand: pathWith("screencapture"), + run: async () => { + throw new CaptureToolError("screencapture", denial); + }, + }); + + await expect(attempt()).rejects.toMatchObject({ + name: "CapturePermissionError", + hint: expect.stringContaining("Vicinae"), + }); + await expect(attempt()).rejects.toMatchObject({ + hint: expect.stringContaining("Screen Recording"), + }); + }); + }); + + it("reports any other explanation rather than swallowing it", async () => { + const dir = await tempDir(); + + await expect( + captureRegion({ + outputDir: dir, + platform: "linux", + hasCommand: pathWith("maim"), + run: async () => { + throw new CaptureToolError("maim", "maim: cannot open display\n"); + }, + }), + ).rejects.toThrow(CaptureToolError); + }); + it("explains what to install when nothing is available", async () => { const dir = await tempDir(); @@ -201,25 +318,6 @@ describe("captureRegion", () => { ).rejects.toThrow(NoCaptureToolError); }); - it("creates the output directory on first use", async () => { - const dir = await tempDir(); - const nested = join(dir, "snips"); - const now = new Date(2026, 9, 4, 14, 59, 26); - - await captureRegion({ - outputDir: nested, - platform: "darwin", - now, - hasCommand: pathWith("screencapture"), - run: async (_tool, args) => { - await writeFile(args[args.length - 1], Buffer.from([0x89])); - return Buffer.alloc(0); - }, - }); - - expect((await readFile(join(nested, snipFileName(now)))).length).toBeGreaterThan(0); - }); - it("does not invent a backend on macOS", async () => { const dir = await tempDir(); diff --git a/src/lib/capture.ts b/src/lib/capture.ts index 620a658..6df49e3 100644 --- a/src/lib/capture.ts +++ b/src/lib/capture.ts @@ -28,12 +28,28 @@ const MAX_CAPTURE_BYTES = 64 * 1024 * 1024; /** Long enough to pick a region unhurried, short enough that a wedged tool cannot hang the view forever. */ const CAPTURE_TIMEOUT_MS = 5 * 60 * 1000; -/** Runs a command and resolves with its stdout. Injected so tests never spawn anything. */ +/** + * What macOS says when Screen Recording permission is missing. + * + * This one pattern is why the module reads stderr at all. The message is the + * *only* difference between "the system refused to let us look" and "the user + * pressed Escape": both produce no file, and `screencapture -i` has been + * reported to exit `0` in the refusal case. Treating the refusal as a cancel + * leaves the user with a feature that silently does nothing. + */ +const SCREEN_RECORDING_DENIED = /could not create image from (rect|display|window)/i; + +export interface RunResult { + stdout: Buffer; + stderr: string; +} + +/** Runs a command. Injected so tests never spawn anything. */ export type RunCommand = ( tool: string, args: string[], options?: { signal?: AbortSignal }, -) => Promise; +) => Promise; export interface CaptureBackend { /** Shown to the user when a capture fails, so they know what to install. */ @@ -83,6 +99,42 @@ export class NoCaptureToolError extends Error { } } +/** + * Raised when the system refuses to hand over the screen. + * + * Separate from a generic tool failure because it is the one capture error the + * user can actually fix, and because on macOS the fix is not obvious: the + * permission belongs to the *launcher*, since TCC attributes the request to the + * app responsible for spawning the extension runtime rather than to the + * extension itself. + */ +export class CapturePermissionError extends Error { + readonly hint: string; + + constructor(platform: NodeJS.Platform) { + super("The system denied screen capture."); + this.name = "CapturePermissionError"; + this.hint = + platform === "darwin" + ? "Give Vicinae Screen Recording permission in System Settings under Privacy & Security, then quit Vicinae and open it again. The permission belongs to Vicinae rather than to the extension, because macOS attributes the request to the app that launched the extension runtime." + : "Give the launcher permission to record the screen, then restart it."; + } +} + +/** A capture tool that ran and failed for a reason it could explain. */ +export class CaptureToolError extends Error { + readonly tool: string; + readonly stderr: string; + + constructor(tool: string, stderr: string, cause?: unknown) { + const detail = stderr.trim().split("\n")[0] ?? ""; + super(`${tool} failed.${detail ? ` ${detail}` : ""}`, cause === undefined ? undefined : { cause }); + this.name = "CaptureToolError"; + this.tool = tool; + this.stderr = stderr; + } +} + /** * `command -v` rather than spawning the tool itself: several of these block or * open a window when run bare, and `--version` is not reliably supported. @@ -96,7 +148,11 @@ async function commandExists(tool: string): Promise { }); } -function runCommand(tool: string, args: string[], options: { signal?: AbortSignal } = {}): Promise { +function runCommand( + tool: string, + args: string[], + options: { signal?: AbortSignal } = {}, +): Promise { return new Promise((resolve, reject) => { execFile( tool, @@ -107,9 +163,15 @@ function runCommand(tool: string, args: string[], options: { signal?: AbortSigna maxBuffer: MAX_CAPTURE_BYTES, encoding: "buffer", }, - (error, stdout) => { - if (error) reject(error); - else resolve(stdout as Buffer); + (error, stdout, stderr) => { + const text = stderr?.toString("utf8") ?? ""; + + if (error) { + reject(new CaptureToolError(tool, text, error)); + return; + } + + resolve({ stdout: stdout as Buffer, stderr: text }); }, ); }); @@ -125,7 +187,7 @@ const grimSlurp: CaptureBackend = { name: "grim + slurp", requires: ["grim", "slurp"], async capture({ output, signal, run }) { - const geometry = (await run("slurp", [], { signal })).toString("utf8").trim(); + const geometry = (await run("slurp", [], { signal })).stdout.toString("utf8").trim(); if (!geometry) return; await run("grim", ["-g", geometry, output], { signal }); }, @@ -139,7 +201,7 @@ const flameshot: CaptureBackend = { name: "flameshot", requires: ["flameshot"], async capture({ output, signal, run }) { - const png = await run("flameshot", ["gui", "--raw"], { signal }); + const png = (await run("flameshot", ["gui", "--raw"], { signal })).stdout; if (png.length === 0) return; await writeFile(output, png); }, @@ -227,15 +289,15 @@ export function snipFileName(now: Date): string { * Captures a region and returns the file it wrote, or `null` when the user * backed out. * - * Cancel detection leans on the file rather than the exit code on purpose: - * every tool reports a cancelled selection differently, and at least one - * reports success anyway. Whether an image actually landed on disk is the one - * signal they all agree on. + * Success is decided by the file, not by an exit code: the tools disagree about + * what a cancelled selection looks like, and at least one reports success + * anyway. Failure is then triaged from what the tool said on stderr, which is + * the only channel that separates a refused permission from a cancel. */ export async function captureRegion(options: CaptureOptions): Promise { const platform = options.platform ?? process.platform; const hasCommand = options.hasCommand ?? commandExists; - const run = options.run ?? runCommand; + const baseRun = options.run ?? runCommand; const backend = await selectBackend( hasCommand, @@ -246,10 +308,39 @@ export async function captureRegion(options: CaptureOptions): Promise { + try { + const result = await baseRun(tool, args, runOptions); + if (result.stderr.trim()) diagnostics.push(result.stderr.trim()); + return result; + } catch (thrown) { + const stderr = thrown instanceof CaptureToolError ? thrown.stderr : ""; + if (stderr.trim()) diagnostics.push(stderr.trim()); + throw thrown; + } + }; + + let failure: unknown; + try { + await backend.capture({ output, signal: options.signal, run }); + } catch (thrown) { + failure = thrown; + } const written = await stat(output).catch(() => null); - if (!written || written.size === 0) return null; + if (written && written.size > 0) { + return { path: output, bytes: written.size, backend: backend.name }; + } - return { path: output, bytes: written.size, backend: backend.name }; + const detail = diagnostics.join("\n"); + + if (SCREEN_RECORDING_DENIED.test(detail)) throw new CapturePermissionError(platform); + + // A tool that explained itself is a failure worth reporting. Silence with no + // image is the user pressing Escape, which is the common case and says nothing. + if (detail) throw new CaptureToolError(backend.requires[0], detail, failure); + + return null; } diff --git a/src/lib/snip.ts b/src/lib/snip.ts index fcbe62e..0406db4 100644 --- a/src/lib/snip.ts +++ b/src/lib/snip.ts @@ -1,5 +1,7 @@ import { environment } from "@vicinae/api"; import { join } from "node:path"; +import { CapturePermissionError, NoCaptureToolError } from "./capture"; +import { errorMessage } from "./utils"; /** * The seam between the snip command and the chat command. @@ -53,3 +55,20 @@ export function readSnipContext(launchContext: unknown): SnipLaunchContext { sessionId: asString(sessionId), }; } + +/** + * Turns a capture failure into something worth showing. + * + * The two failures that carry a hint are the only two the user can do anything + * about, and the hints are useless if they are dropped: "no capture tool is + * installed" and "the system is blocking screen capture" need completely + * different responses from completely different places. + */ +export function describeCaptureFailure(thrown: unknown): string { + const actionable = + thrown instanceof CapturePermissionError || thrown instanceof NoCaptureToolError + ? thrown.hint + : undefined; + + return actionable ? `${errorMessage(thrown)} ${actionable}` : errorMessage(thrown); +} diff --git a/src/snip.tsx b/src/snip.tsx index c3bffa1..4dab9c5 100644 --- a/src/snip.tsx +++ b/src/snip.tsx @@ -1,8 +1,7 @@ import { LaunchType, closeMainWindow, launchCommand, showHUD, type LaunchProps } from "@vicinae/api"; -import { NoCaptureToolError, captureRegion } from "./lib/capture"; +import { captureRegion } from "./lib/capture"; import { getLamPreferences } from "./lib/preferences"; -import { readSnipContext, snipDirectory } from "./lib/snip"; -import { errorMessage } from "./lib/utils"; +import { describeCaptureFailure, readSnipContext, snipDirectory } from "./lib/snip"; import { visionSupport } from "./lib/vision"; /** @@ -69,8 +68,7 @@ export default async function SnipCommand(props: LaunchProps) { const capture = await captureRegion({ outputDir: snipDirectory() }); snipPath = capture?.path; } catch (thrown) { - snipError = - thrown instanceof NoCaptureToolError ? `${errorMessage(thrown)} ${thrown.hint}` : errorMessage(thrown); + snipError = describeCaptureFailure(thrown); } // An abandoned selection says nothing at all: the user pressed Escape. But a -- 2.54.0 From e00b49bbe5a141b5b999437c56cc6521d5d7cbab Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:34:09 +0800 Subject: [PATCH 04/11] =?UTF-8?q?fix:=20stop=20the=20chat=20view=20hanging?= =?UTF-8?q?=20on=20"Loading=20conversations=E2=80=A6"=20after=20a=20snip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hand-off launch deadlocked its own startup. `resumed` was initialised to `Boolean(initialSession)`, which is false for a snip, and the idle-resume effect returned early whenever there was a hand-off, so nothing ever set it. The render gate needed both `resumed` and `prepared`, so a successful snip left the view spinning and there was no way to reach the composer. Two effects guarding two booleans had already produced one bug here, so this fixes the shape rather than the value: - One startup effect and one `startup` flag, cleared in a `finally`. Every branch ends in the same place, including a thrown one, so no path can leave the view loading. - The decision tree moves to `lib/startup.ts` as a pure `resolveStartup`, which takes its storage and file reads as injected ports. It is now covered by 16 tests, including the case that broke: a hand-off must not fall through to the idle resume, and must still finish startup. - `startup.ts` joins the guard that keeps the logic layer free of Vicinae imports. - ChatView is prettier-clean; it had been carrying pre-existing drift and has been rewritten twice this session anyway. Verified: 273 tests, tsc, eslint, prettier, and vici build. --- src/__tests__/agent.test.ts | 12 +- src/__tests__/startup.test.ts | 203 ++++++++++++++++++++++++++++++++ src/lib/components/ChatView.tsx | 107 ++++++----------- src/lib/startup.ts | 96 +++++++++++++++ 4 files changed, 345 insertions(+), 73 deletions(-) create mode 100644 src/__tests__/startup.test.ts create mode 100644 src/lib/startup.ts diff --git a/src/__tests__/agent.test.ts b/src/__tests__/agent.test.ts index 22a4fb5..a79064f 100644 --- a/src/__tests__/agent.test.ts +++ b/src/__tests__/agent.test.ts @@ -165,7 +165,17 @@ describe("module wiring", () => { // These modules must stay renderer-free: they are pure logic, they are what // the unit tests cover, and nothing in them should need a Vicinae runtime. // (`sessions.ts` is deliberately excluded: it legitimately needs LocalStorage.) - for (const file of ["agent.ts", "ollama.ts", "tools.ts", "location.ts", "history.ts", "markdown.ts"]) { + for (const file of [ + "agent.ts", + "ollama.ts", + "tools.ts", + "location.ts", + "history.ts", + "markdown.ts", + "capture.ts", + "attachments.ts", + "startup.ts", + ]) { const source = readFileSync(path.resolve(__dirname, `../lib/${file}`), "utf8"); expect(source, `${file} should not import @vicinae/api`).not.toMatch(/from\s+"@vicinae\/api"/); } diff --git a/src/__tests__/startup.test.ts b/src/__tests__/startup.test.ts new file mode 100644 index 0000000..2caba86 --- /dev/null +++ b/src/__tests__/startup.test.ts @@ -0,0 +1,203 @@ +import { describe, expect, it, vi } from "vitest"; +import { isHandoff, resolveStartup, type StartupPorts } from "../lib/startup"; +import type { PendingAttachment, Session, SessionSummary } from "../lib/types"; + +/** + * The chat view's startup is the part that kept breaking: three asynchronous + * paths (a pushed session, a handed-over capture, an idle resume) that all have + * to end in a rendered chat. Two of those bugs shipped, so the decision tree is + * tested here rather than discovered from a screenshot of a spinner. + */ + +const NOW = 1_000_000; +const HOUR = 3_600_000; + +function session(id: string, updatedAt: number): Session { + return { id, title: id, model: "m", createdAt: updatedAt, updatedAt, turns: [] }; +} + +function summary(id: string, updatedAt: number): SessionSummary { + return { id, title: id, model: "m", createdAt: updatedAt, updatedAt, turnCount: 0, preview: "" }; +} + +function attachment(path: string): PendingAttachment { + return { path, mime: "image/png", base64: "AAAA" }; +} + +/** Ports that record what was asked for, so "did not resume" is checkable. */ +function ports(overrides: Partial = {}) { + const loadSession = vi.fn(async (id: string) => session(id, NOW)); + const listSessions = vi.fn(async () => [summary("latest", NOW)]); + const readAttachment = vi.fn(async (path: string) => attachment(path)); + + return { + loadSession, + listSessions, + readAttachment, + ...overrides, + } satisfies StartupPorts; +} + +function input(overrides: Partial[0]> = {}) { + return { + hasInitialSession: false, + now: NOW, + ...overrides, + }; +} + +describe("isHandoff", () => { + it("is true when a file was handed over", () => { + expect(isHandoff({ initialAttachmentPath: "/tmp/snip.png" })).toBe(true); + }); + + it("is true when a conversation was named", () => { + expect(isHandoff({ initialSessionId: "abc" })).toBe(true); + }); + + it("is false on an ordinary open", () => { + expect(isHandoff({})).toBe(false); + }); +}); + +describe("resolveStartup", () => { + describe("a snip hand-off", () => { + it("reads the attachment and restores the named conversation", async () => { + const p = ports(); + + const result = await resolveStartup( + input({ initialSessionId: "abc", initialAttachmentPath: "/tmp/snip.png" }), + p, + ); + + expect(result.session?.id).toBe("abc"); + expect(result.attachment?.path).toBe("/tmp/snip.png"); + }); + + it("does not fall through to the idle resume", async () => { + // The bug this file exists for: a hand-off has to finish startup even + // though it took the resume path off the table. Guarding the resume with + // "and there was no hand-off" while forgetting to clear the readiness flag + // left the view on its loading row forever. + const p = ports(); + + await resolveStartup(input({ initialAttachmentPath: "/tmp/snip.png" }), p); + + expect(p.listSessions).not.toHaveBeenCalled(); + }); + + it("does not attach the most recent conversation's session", async () => { + const p = ports(); + + const result = await resolveStartup(input({ initialAttachmentPath: "/tmp/snip.png" }), p); + + expect(result.session).toBeUndefined(); + }); + + it("still resolves when the named conversation has been deleted", async () => { + // A snip started from a conversation someone has since deleted should mean + // a fresh chat with the image, not a spinner or a crash. + const p = ports({ loadSession: vi.fn(async () => undefined) }); + + const result = await resolveStartup( + input({ initialSessionId: "gone", initialAttachmentPath: "/tmp/snip.png" }), + p, + ); + + expect(result.session).toBeUndefined(); + expect(result.attachment?.path).toBe("/tmp/snip.png"); + }); + + it("reports an unreadable attachment without refusing to open", async () => { + const p = ports({ + readAttachment: vi.fn(async () => { + throw new Error("ENOENT: no such file"); + }), + }); + + const result = await resolveStartup(input({ initialAttachmentPath: "/tmp/gone.png" }), p); + + expect(result.attachment).toBeUndefined(); + expect(result.attachmentError).toContain("ENOENT"); + }); + + it("survives a storage failure while loading the named conversation", async () => { + const p = ports({ + loadSession: vi.fn(async () => { + throw new Error("storage exploded"); + }), + }); + + const result = await resolveStartup(input({ initialSessionId: "abc" }), p); + + expect(result.session).toBeUndefined(); + }); + }); + + describe("an ordinary open", () => { + it("resumes the most recent conversation when it is fresh", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - 60_000)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result.session?.id).toBe("latest"); + expect(p.loadSession).toHaveBeenCalledWith("latest"); + }); + + it("starts fresh when the newest conversation is older than the window", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - 2 * HOUR)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result.session).toBeUndefined(); + expect(p.loadSession).not.toHaveBeenCalled(); + }); + + it("treats exactly the window as fresh", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - HOUR)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result.session?.id).toBe("latest"); + }); + + it("always resumes when the preference is 'never time out'", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - 400 * HOUR)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: undefined }), p); + + expect(result.session?.id).toBe("latest"); + }); + + it("starts fresh when there are no conversations at all", async () => { + const p = ports({ listSessions: vi.fn(async () => []) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result).toEqual({}); + expect(p.loadSession).not.toHaveBeenCalled(); + }); + + it("starts fresh rather than failing when the index cannot be read", async () => { + const p = ports({ + listSessions: vi.fn(async () => { + throw new Error("corrupt index"); + }), + }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result).toEqual({}); + }); + + it("does not resume when a session was pushed in by hand", async () => { + // The sessions command already knows which conversation to show. + const p = ports(); + + await resolveStartup(input({ hasInitialSession: true }), p); + + expect(p.listSessions).not.toHaveBeenCalled(); + expect(p.loadSession).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/lib/components/ChatView.tsx b/src/lib/components/ChatView.tsx index efc14dd..522d4e1 100644 --- a/src/lib/components/ChatView.tsx +++ b/src/lib/components/ChatView.tsx @@ -6,9 +6,10 @@ import { useVisionSupport } from "../hooks/useVision"; import { buildThreadMarkdown, type TranscriptOptions } from "../markdown"; import { getLamPreferences } from "../preferences"; import { deleteSession, listSessions, loadSession, summarizeSession } from "../sessions"; +import { resolveStartup } from "../startup"; import { buildThread } from "../thread"; import type { PendingAttachment, Session, SessionSummary } from "../types"; -import { errorMessage, recencyGroup, truncate } from "../utils"; +import { recencyGroup, truncate } from "../utils"; /** * The chat surface: conversations on the left, the open conversation on the right. @@ -92,13 +93,10 @@ export function ChatView({ const { replaceSession } = chat; const { canSeeImages } = useVisionSupport(preferences); - /** A launch that hands something over has to wait for it before it can render. */ - const needsPreparation = Boolean(initialSessionId) || Boolean(initialAttachmentPath); - const [input, setInput] = useState(initialPrompt ?? ""); const [summaries, setSummaries] = useState(undefined); - const [prepared, setPrepared] = useState(!needsPreparation); - const [resumed, setResumed] = useState(Boolean(initialSession)); + // One flag, not two. See the startup effect below for why that matters. + const [startup, setStartup] = useState<"loading" | "ready">(initialSession ? "ready" : "loading"); // A captured image is held here until the user says what they want to know // about it. Nothing is sent to the model on capture alone. const [pending, setPending] = useState(undefined); @@ -118,76 +116,47 @@ export function ChatView({ }, [reload, chat.session.updatedAt]); /** - * Restores whatever the launching command handed over. + * Everything that has to happen before the first render, in one place. * - * This is the second half of the snip handoff: the capture command closed the - * window to take the shot, so it comes back here with a file to read and, when - * the snip was started from a conversation, the id of the conversation to - * return to. Reloading by id rather than trusting "most recent" matters - * because the snip itself is not a turn, so nothing was saved in between. + * This used to be two effects guarding two booleans, and a hand-off launch + * satisfied the guard of *both* while setting neither, so `resumed` stayed + * false and the view sat on "Loading conversations…" forever. Anything that + * blocks the first render belongs in one effect with one flag, and the flag is + * cleared in a `finally` so that no branch, including a thrown one, can leave + * the view spinning. */ useEffect(() => { - if (!needsPreparation) return; - let cancelled = false; - void (async () => { - if (initialSessionId) { - const session = await loadSession(initialSessionId); - if (!cancelled && session) replaceSession(session); - } - - if (initialAttachmentPath) { - try { - const attachment = await readAttachment(initialAttachmentPath); - if (!cancelled) setPending(attachment); - } catch (thrown) { - if (!cancelled) setSnipNote(`**Could not open the screenshot:** ${errorMessage(thrown)}`); - } - } - - if (!cancelled) setPrepared(true); - })(); - - return () => { - cancelled = true; - }; - }, [needsPreparation, initialSessionId, initialAttachmentPath, replaceSession]); - - /** - * Reopening the command continues the most recent conversation, but only if it - * was used recently. Past the configured idle window it starts a fresh chat - * instead. The previous conversation is never removed, it just drops down the - * sidebar. Setting the preference to "Never" keeps the last chat open forever. - * - * A hand-off launch opts out: it either names the conversation to return to or - * deliberately starts a new one, and quietly continuing some other chat would - * bury the screenshot in an unrelated thread. - */ - useEffect(() => { - if (initialSession || needsPreparation) return; - - let cancelled = false; void (async () => { try { - const [latest] = await listSessions(); - if (!latest) return; + const outcome = await resolveStartup( + { + hasInitialSession: Boolean(initialSession), + initialSessionId, + initialAttachmentPath, + resumeTimeoutMs: preferences.resumeTimeoutMs, + now: Date.now(), + }, + { loadSession, listSessions, readAttachment }, + ); - const idleMs = Date.now() - latest.updatedAt; - const { resumeTimeoutMs } = preferences; - if (resumeTimeoutMs !== undefined && idleMs > resumeTimeoutMs) return; + if (cancelled) return; - const session = await loadSession(latest.id); - if (!cancelled && session) replaceSession(session); + if (outcome.session) replaceSession(outcome.session); + if (outcome.attachment) setPending(outcome.attachment); + if (outcome.attachmentError) { + setSnipNote(`**Could not open the screenshot:** ${outcome.attachmentError}`); + } } finally { - if (!cancelled) setResumed(true); + if (!cancelled) setStartup("ready"); } })(); return () => { cancelled = true; }; - }, [initialSession, needsPreparation, preferences, replaceSession]); + }, [initialSession, initialSessionId, initialAttachmentPath, preferences.resumeTimeoutMs, replaceSession]); const thread = useMemo( () => @@ -213,8 +182,8 @@ export function ChatView({ * already looking. */ const threadMarkdown = useMemo(() => { - const notes = [launchNote, snipNote, pending ? PENDING_NOTE : undefined].filter( - (note): note is string => Boolean(note), + const notes = [launchNote, snipNote, pending ? PENDING_NOTE : undefined].filter((note): note is string => + Boolean(note), ); const transcript = @@ -237,10 +206,7 @@ export function ChatView({ if (base.some((summary) => summary.id === activeId)) return mapped; const active = summarizeSession(chat.session); - return [ - { id: active.id, title: active.title, updatedAt: active.updatedAt, isActive: true }, - ...mapped, - ]; + return [{ id: active.id, title: active.title, updatedAt: active.updatedAt, isActive: true }, ...mapped]; }, [summaries, activeId, chat.session]); /** @@ -282,8 +248,7 @@ export function ChatView({ /** The snip a "show me the screenshot" action would open: the pending one, else the newest. */ const latestAttachment = - pending ?? - [...chat.session.turns].reverse().find((turn) => turn.attachments.length > 0)?.attachments[0]; + pending ?? [...chat.session.turns].reverse().find((turn) => turn.attachments.length > 0)?.attachments[0]; const send = useCallback( (text: string) => { @@ -382,13 +347,11 @@ export function ChatView({ ); }; - if (!resumed || !prepared || summaries === undefined) { + if (startup === "loading" || summaries === undefined) { return ; } - const placeholder = chat.status - ? `${chat.status}. Press ⏎ to stop` - : "Lam is thinking. Press ⏎ to stop"; + const placeholder = chat.status ? `${chat.status}. Press ⏎ to stop` : "Lam is thinking. Press ⏎ to stop"; const idlePlaceholder = pending ? "Ask about the screenshot…" : "Ask Lam anything…"; diff --git a/src/lib/startup.ts b/src/lib/startup.ts new file mode 100644 index 0000000..dd11776 --- /dev/null +++ b/src/lib/startup.ts @@ -0,0 +1,96 @@ +import type { PendingAttachment, Session, SessionSummary } from "./types"; +import { errorMessage } from "./utils"; + +/** + * Everything the chat view resolves before its first render. + * + * Pulled out of the component and made pure because this is the part that keeps + * going wrong. Three things can happen on open, and all of them are + * asynchronous: a launch hands over a captured file, it may also name the + * conversation to return to, and an ordinary open resumes the most recent + * conversation if it is fresh enough. Every path has to end in a rendered chat. + * + * As a plain function it is testable without mounting anything, which matters + * because the alternative is finding out from a screenshot that a view is + * spinning forever. + * + * Imports nothing from Vicinae, so it belongs to the logic layer. + */ + +export interface StartupInput { + /** A conversation already in hand, which is how the sessions command pushes this view. */ + hasInitialSession: boolean; + /** A conversation to reopen by id, handed over by the snip command. */ + initialSessionId?: string; + /** A captured image to read in. */ + initialAttachmentPath?: string; + /** Idle time past which an ordinary open starts fresh. Undefined means never time out. */ + resumeTimeoutMs?: number; + /** Injected so the resume window is deterministic under test. */ + now: number; +} + +/** The side effects, injected so nothing here touches storage or the file system directly. */ +export interface StartupPorts { + loadSession: (id: string) => Promise; + listSessions: () => Promise; + readAttachment: (path: string) => Promise; +} + +export interface StartupResult { + /** The conversation to show, when one was restored. */ + session?: Session; + /** The image that was handed over, when one could be read. */ + attachment?: PendingAttachment; + /** Why a handed-over image could not be read, so the view can say so. */ + attachmentError?: string; +} + +/** True when the launch handed something over, which also means "do not resume". */ +export function isHandoff(input: Pick): boolean { + return Boolean(input.initialSessionId || input.initialAttachmentPath); +} + +export async function resolveStartup(input: StartupInput, ports: StartupPorts): Promise { + const result: StartupResult = {}; + + if (input.initialSessionId) { + // A hand-off names the conversation to return to, so that one is restored + // rather than whichever chat happens to be most recent. By id matters + // because taking a snip is not a turn, so nothing was saved in between. + // A session that has since been deleted just means starting fresh. + result.session = await ports.loadSession(input.initialSessionId).catch(() => undefined); + } + + if (input.initialAttachmentPath) { + try { + result.attachment = await ports.readAttachment(input.initialAttachmentPath); + } catch (thrown) { + // A missing file leaves an empty chat with an explanation, which beats + // refusing to open at all. + result.attachmentError = errorMessage(thrown); + } + } + + // Nothing was handed over, so the idle-resume rule applies: reopening the + // command continues the most recent conversation, but only if it was used + // recently. The previous conversation is never removed either way, it just + // drops down the sidebar. + if (!input.hasInitialSession && !isHandoff(input)) { + const latest = await ports + .listSessions() + .then((sessions) => sessions[0]) + .catch(() => undefined); + + if (latest) { + const idleMs = input.now - latest.updatedAt; + const fresh = input.resumeTimeoutMs === undefined || idleMs <= input.resumeTimeoutMs; + + if (fresh) { + result.session = await ports.loadSession(latest.id).catch(() => undefined); + } + } + } + + return result; +} -- 2.54.0 From c1212192c560b452c4be56d3df2e27eed3cdd3d1 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:37:14 +0800 Subject: [PATCH 05/11] feat: render the attached screenshot instead of just naming it The transcript said "_Screenshot attached_" and left it at that, on a note in the test file claiming Vicinae could not resolve a local image. It can. That claim was true of a URL the model invented, not of a file this extension captured itself. Checked against the renderer rather than assumed: - `imageProviderUrl` in `markdown-model.cpp` routes `file://`, and a scheme-less string, through `ImageURL::local`. Anything it does not recognise comes back empty, which is why a bare path is not good enough. - `MdImage.qml` draws it block-level and centred, using the alt text as an italic caption, and caps the rendered height at 200px regardless of the `raycast-width`/`raycast-height` hint it also accepts. So `attachments.ts` gains `imageSource`, which builds a `file://` URL with `pathToFileURL` so a path with a space or non-ascii still resolves, and `markdown.ts` gains `renderAttachments`, used for a turn's attachments and for an unsent pending snip. That second use is the one that matters most: the capture is now visible before the question about it exists. The alt text carries the label, so a deleted file degrades to what the transcript used to say. The 200px cap is a Vicinae limit, so the action panel keeps Open Screenshot for the full image. Tests that asserted the old behaviour are inverted, and imageSource is covered for spaces and non-ascii. Verified: 280 tests, tsc, eslint, prettier, and vici build. --- README.md | 6 +++-- src/__tests__/attachments.test.ts | 22 +++++++++++++++- src/__tests__/markdown.test.ts | 44 +++++++++++++++++++++++++------ src/lib/attachments.ts | 15 +++++++++++ src/lib/components/ChatView.tsx | 14 ++++++---- src/lib/markdown.ts | 31 +++++++++++++++++----- 6 files changed, 109 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 3cfbf25..b8455cd 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ Hit a global shortcut, type a question, get a streamed answer without leaving wh - **Persistent conversations.** Every chat is saved locally. The sidebar lists them by recency, and reopening the command picks up where you left off, unless it has been idle longer than you allow. - **Readable code.** Fenced blocks render in a dark panel with the language named in a header, syntax colouring, and a Copy button. - **Tools.** Lam decides when to call them: current time, location context, web search, and page fetching. -- **Screenshots, two ways.** Snip a region into the conversation you are already in from the action panel, or run **Snip to Lam** to start a new chat about it. The launcher window gets out of the way first, so it never ends up in the shot. Neither is offered unless the current model can actually see images. +- **Screenshots, two ways.** Snip a region into the conversation you are already in from the action panel, or run **Snip to Lam** to start a new chat about it. The launcher window gets out of the way first, so it never ends up in the shot, and the capture appears in the transcript as a thumbnail with Open Screenshot for the full image. Neither is offered unless the current model can actually see images. - **Chat only.** Lam answers questions. It does not touch your files or run commands. The one exception is capture, and it only ever happens because you pressed the key for it. ## Requirements @@ -187,7 +187,9 @@ A few decisions worth knowing about before you change things: **The transcript reads newest message first.** The search bar sits at the top of the window, so the most recent message, an answer once one has been written, is the first line under it rather than below the fold. Within a single exchange that means the answer appears above the question it responds to. `buildThread` produces that order, and `buildThreadMarkdown` regroups messages back into exchanges so the horizontal rule separates one exchange from the next instead of dividing every message. -**The transcript is text-only, and nothing in it is bold except an error.** The answer is what gets read, so it is plain body text carrying no label; the question is a blockquote, which the renderer draws dimmed, italic, and with a left rule. A `**You**` / `**Lam**` header on each exchange gave both speakers identical weight and made the answer sit behind chrome. Tool calls collapse to one italic line (_Tools: `web_search`_) rather than a bold header plus a list, so they stay below the answer too. Raycast solves the same problem with alignment, but markdown offers no alignment control. +**The transcript is text plus the attached images, and nothing in it is bold except an error.** The answer is what gets read, so it is plain body text carrying no label; the question is a blockquote, which the renderer draws dimmed, italic, and with a left rule. A `**You**` / `**Lam**` header on each exchange gave both speakers identical weight and made the answer sit behind chrome. Tool calls collapse to one italic line (_Tools: `web_search`_) rather than a bold header plus a list, so they stay below the answer too. Raycast solves the same problem with alignment, but markdown offers no alignment control. + +**An attached screenshot is rendered, not described.** This is a correction: the transcript used to say `_Screenshot attached_` and leave it at that, on the belief that Vicinae could not resolve a local image. It can. `imageProviderUrl` in `markdown-model.cpp` routes `file://` (and a scheme-less string) through `ImageURL::local`, and `MdImage.qml` draws it block-level and centred with the alt text as an italic caption underneath. The one real limit is that the rendered height is capped at 200px regardless of any size hint, so it is a thumbnail rather than something readable, which is why the action panel still offers Open Screenshot for the full image. The alt text is not decoration either: it is what remains if the capture file has since been deleted. A preview of an unsent attachment is shown the same way, so a snip is visible before the question about it exists. **Code fences are labelled so Vicinae can colour them.** `MdCodeBlock.qml` already provides the dark panel, a header naming the language, and a Copy button, so none of that is built here. The language string is what turns on highlighting: `syntax::highlight` resolves it with `definitionForName(language)` and then `definitionForFileName("file." + language)`. So `bash` and `ts` colour correctly, while an unrecognised string such as `terminal` still renders a label but produces no highlighting at all, so a wrong label is worse than none. `code.ts` therefore labels bare fences conservatively, only when a signal is clear, and the default prompt asks for a language on every fence. diff --git a/src/__tests__/attachments.test.ts b/src/__tests__/attachments.test.ts index 98270f7..c935337 100644 --- a/src/__tests__/attachments.test.ts +++ b/src/__tests__/attachments.test.ts @@ -2,7 +2,7 @@ import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { mimeForPath, readAttachment } from "../lib/attachments"; +import { imageSource, mimeForPath, readAttachment } from "../lib/attachments"; const tempDirs: string[] = []; @@ -30,6 +30,26 @@ describe("mimeForPath", () => { }); }); +describe("imageSource", () => { + it("produces a file URL, which is the only scheme the renderer resolves for a local file", () => { + expect(imageSource("/tmp/snip.png")).toBe("file:///tmp/snip.png"); + }); + + it("escapes a path with a space, which the user's own screenshot directory has", () => { + expect(imageSource("/Users/x/Pictures/Screenshot 2026-10-04.png")).toBe( + "file:///Users/x/Pictures/Screenshot%202026-10-04.png", + ); + }); + + it("escapes non-ascii", () => { + expect(imageSource("/tmp/caf\u00e9.png")).toBe("file:///tmp/caf%C3%A9.png"); + }); + + it("keeps a bare path out of the markdown, because that resolves to nothing", () => { + expect(imageSource("/tmp/snip.png")).not.toBe("/tmp/snip.png"); + }); +}); + describe("readAttachment", () => { it("reads the bytes as bare base64, with no data URL prefix", async () => { const dir = await tempDir(); diff --git a/src/__tests__/markdown.test.ts b/src/__tests__/markdown.test.ts index b72c6be..69b0373 100644 --- a/src/__tests__/markdown.test.ts +++ b/src/__tests__/markdown.test.ts @@ -75,9 +75,9 @@ describe("buildThreadMarkdown", () => { expect(markdown.indexOf("second question")).toBeLessThan(markdown.indexOf("first answer")); }); - it("renders no images, because markdown cannot resolve them", () => { - // Images render block-level and centred at up to 200px (`MdImage.qml`), and - // every candidate icon source fails. Keep the transcript text-only. + it("renders no images for a message that has no attachment", () => { + // Images only ever come from an attachment this extension took itself. A URL + // the model invented is not resolvable and renders as an empty block. const markdown = buildThreadMarkdown([ message({ role: "user", text: "q" }), message({ role: "assistant", text: "a" }), @@ -290,15 +290,43 @@ describe("sessionToPlainText", () => { describe("attachments in the transcript", () => { const screenshot = { path: "/tmp/snip.png", mime: "image/png" }; - it("says a screenshot is attached, because the image cannot be rendered", () => { - // Images render block-level, centred and capped at 200px (`MdImage.qml`), - // and no form of `![](path)` resolves to a local capture. Saying it is there - // is the honest option; the file itself is opened from the action panel. + it("shows the attachment instead of just naming it", () => { + // Vicinae resolves a local file through `ImageURL::local`, so the capture is + // rendered rather than described. `MdImage.qml` caps the height at 200px, so + // it is a thumbnail, and the alt text is what is left if the file is gone. const markdown = buildThreadMarkdown([ message({ role: "user", text: "what is this?", attachments: [screenshot] }), ]); - expect(markdown).toContain("_Screenshot attached_"); + expect(markdown).toContain("![Screenshot attached](file:///tmp/snip.png)"); + }); + + it("keeps the question above its image", () => { + const markdown = buildThreadMarkdown([ + message({ role: "user", text: "what is this?", attachments: [screenshot] }), + ]); + + expect(markdown.indexOf("> what is this?")).toBeLessThan(markdown.indexOf("![")); + }); + + it("renders every attachment on a turn", () => { + const markdown = buildThreadMarkdown([ + message({ + role: "user", + text: "compare", + attachments: [screenshot, { path: "/tmp/second.png", mime: "image/png" }], + }), + ]); + + expect(markdown).toContain("file:///tmp/snip.png"); + expect(markdown).toContain("file:///tmp/second.png"); + }); + + it("never puts an image on an answer, which has no attachments", () => { + const markdown = buildThreadMarkdown([ + message({ role: "assistant", text: "a cat", attachments: [screenshot] }), + ]); + expect(markdown).not.toContain("!["); }); diff --git a/src/lib/attachments.ts b/src/lib/attachments.ts index a933452..d49d078 100644 --- a/src/lib/attachments.ts +++ b/src/lib/attachments.ts @@ -1,5 +1,6 @@ import { readFile } from "node:fs/promises"; import { extname } from "node:path"; +import { pathToFileURL } from "node:url"; import type { PendingAttachment } from "./types"; /** @@ -26,6 +27,20 @@ export function mimeForPath(path: string): string { return MIME_TYPES[extname(path).toLowerCase()] ?? "application/octet-stream"; } +/** + * A markdown image source for a local file. + * + * A `file://` URL rather than a bare path, because Vicinae's markdown renderer + * only resolves a scheme it recognises: `imageProviderUrl` in + * `markdown-model.cpp` routes `file://`, and a scheme-less string, through + * `ImageURL::local`, and anything it does not recognise it returns as empty. + * `pathToFileURL` does the escaping, so a capture under a directory with a space + * in its name still resolves. + */ +export function imageSource(path: string): string { + return pathToFileURL(path).href; +} + /** * Reads an image into the shape a request needs. * diff --git a/src/lib/components/ChatView.tsx b/src/lib/components/ChatView.tsx index 522d4e1..f13673a 100644 --- a/src/lib/components/ChatView.tsx +++ b/src/lib/components/ChatView.tsx @@ -3,7 +3,7 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import { readAttachment } from "../attachments"; import { useChat } from "../hooks/useChat"; import { useVisionSupport } from "../hooks/useVision"; -import { buildThreadMarkdown, type TranscriptOptions } from "../markdown"; +import { buildThreadMarkdown, renderAttachments, type TranscriptOptions } from "../markdown"; import { getLamPreferences } from "../preferences"; import { deleteSession, listSessions, loadSession, summarizeSession } from "../sessions"; import { resolveStartup } from "../startup"; @@ -59,7 +59,7 @@ const EMPTY_TRANSCRIPT = "**Ask Lam anything**\n\nType a question in the search const DEFAULT_IMAGE_PROMPT = "What is in this image?"; /** The reminder shown while a snip is attached but not yet sent. */ -const PENDING_NOTE = "_Screenshot attached. Ask a question about it, or press Return to send it as it is._"; +const PENDING_NOTE = "_Ask a question about it, or press Return to send it as it is._"; export interface ChatViewProps { /** Opens a specific conversation instead of resuming the latest one. */ @@ -182,9 +182,13 @@ export function ChatView({ * already looking. */ const threadMarkdown = useMemo(() => { - const notes = [launchNote, snipNote, pending ? PENDING_NOTE : undefined].filter((note): note is string => - Boolean(note), - ); + const notes = [ + launchNote, + snipNote, + // The preview comes first so the captured image is visible before the user + // decides what to ask about it, rather than only after sending. + pending ? `${renderAttachments([pending])}\n\n${PENDING_NOTE}` : undefined, + ].filter((note): note is string => Boolean(note)); const transcript = thread.length === 0 ? EMPTY_TRANSCRIPT : buildThreadMarkdown(thread, transcriptOptions); diff --git a/src/lib/markdown.ts b/src/lib/markdown.ts index 31fd926..badadba 100644 --- a/src/lib/markdown.ts +++ b/src/lib/markdown.ts @@ -1,5 +1,6 @@ +import { imageSource } from "./attachments"; import { repairCodeBlocks } from "./code"; -import type { ChatTurn, ThreadMessage } from "./types"; +import type { Attachment, ChatTurn, ThreadMessage } from "./types"; /** * Markdown rendering for the detail panel. @@ -22,6 +23,24 @@ import type { ChatTurn, ThreadMessage } from "./types"; export const STREAMING_CURSOR = "▍"; +/** + * Renders attachments as markdown images. + * + * Vicinae does resolve a local file: `MdImage.qml` draws it block-level and + * centred, uses the alt text as an italic caption underneath, and caps the + * height at 200px no matter what is asked for. So this is a thumbnail rather + * than something readable, which is why the action panel also offers Open + * Screenshot for the full image. + * + * The alt text is not decoration: it is what remains if the file has since been + * deleted, so it says what the picture was rather than leaving a blank space. + */ +export function renderAttachments(attachments: Attachment[]): string { + return attachments + .map((attachment) => `![Screenshot attached](${imageSource(attachment.path)})`) + .join("\n\n"); +} + /** Which optional sections the transcript includes. Driven by preferences. */ export interface TranscriptOptions { showToolCalls: boolean; @@ -50,12 +69,10 @@ function renderTools(message: ThreadMessage): string { function renderMessage(message: ThreadMessage, options: TranscriptOptions): string { // The question is identified by its styling alone. if (message.role === "user") { - // An attached screenshot cannot be shown here: images render block-level, - // centred and capped at 200px (`MdImage.qml`), and there is no form of - // `![](path)` that resolves to a local capture. The transcript says the - // image is there instead, and the action panel opens it. - const attached = message.attachments.length > 0 ? "\n\n_Screenshot attached_" : ""; - return `${quote(message.text)}${attached}`; + const images = renderAttachments(message.attachments); + const asked = quote(message.text); + + return images ? `${asked}\n\n${images}` : asked; } // While a tool is running there is no answer text yet, so the activity line -- 2.54.0 From 2c0653c23c86cfb9039010db9ca35a2757b9aed1 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:41:45 +0800 Subject: [PATCH 06/11] fix: lay the screenshot preview out to the image, not to the request The preview rendered, but with dead space around it. `MdImage.qml` sizes its container to the request rather than to the image, so with no hint it is the full pane width and 200px tall and the image is aspect-fitted inside that. A wide capture sat in a letterbox: measured against the real captures in the support directory, a 4:1 crop wasted about 106px of the 200px container and a 6.7:1 crop about 140px, which is what made the transcript look gappy and put the caption far below the image. - `readAttachment` now reads the PNG `IHDR` chunk, so `Attachment` carries the real dimensions and they are persisted with the turn. - `previewBox` computes an exact box from those dimensions, scaled to fit 400x200 with the scale capped at 1 so a small capture is not upscaled, and `imageSource` emits it as the `raycast-width`/`raycast-height` hints `parseImageSize` looks for. The path is resolved from the URL before the query, so the hints never reach the file name. - The alt text is now "Screenshot". It is drawn centred under the image, so the sentence "Screenshot attached" read as a stray line of prose floating in the middle of the pane. - A pending note no longer stacks on top of the empty-transcript placeholder. Both said the same thing, one line under the other, once a snip was attached. A capture with no readable header simply falls back to the old sizing. Verified: 293 tests, tsc, eslint, prettier, and vici build. --- README.md | 6 +- src/__tests__/attachments.test.ts | 103 ++++++++++++++++++++++++++++-- src/__tests__/markdown.test.ts | 2 +- src/lib/attachments.ts | 100 ++++++++++++++++++++++++++--- src/lib/components/ChatView.tsx | 8 ++- src/lib/hooks/useChat.ts | 9 ++- src/lib/markdown.ts | 10 +-- src/lib/schemas.ts | 4 ++ src/lib/types.ts | 11 +++- 9 files changed, 227 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index b8455cd..3de1a1a 100644 --- a/README.md +++ b/README.md @@ -189,7 +189,11 @@ A few decisions worth knowing about before you change things: **The transcript is text plus the attached images, and nothing in it is bold except an error.** The answer is what gets read, so it is plain body text carrying no label; the question is a blockquote, which the renderer draws dimmed, italic, and with a left rule. A `**You**` / `**Lam**` header on each exchange gave both speakers identical weight and made the answer sit behind chrome. Tool calls collapse to one italic line (_Tools: `web_search`_) rather than a bold header plus a list, so they stay below the answer too. Raycast solves the same problem with alignment, but markdown offers no alignment control. -**An attached screenshot is rendered, not described.** This is a correction: the transcript used to say `_Screenshot attached_` and leave it at that, on the belief that Vicinae could not resolve a local image. It can. `imageProviderUrl` in `markdown-model.cpp` routes `file://` (and a scheme-less string) through `ImageURL::local`, and `MdImage.qml` draws it block-level and centred with the alt text as an italic caption underneath. The one real limit is that the rendered height is capped at 200px regardless of any size hint, so it is a thumbnail rather than something readable, which is why the action panel still offers Open Screenshot for the full image. The alt text is not decoration either: it is what remains if the capture file has since been deleted. A preview of an unsent attachment is shown the same way, so a snip is visible before the question about it exists. +**An attached screenshot is rendered, not described.** This is a correction: the transcript used to say `_Screenshot attached_` and leave it at that, on the belief that Vicinae could not resolve a local image. It can. `imageProviderUrl` in `markdown-model.cpp` routes `file://` (and a scheme-less string) through `ImageURL::local`, and `MdImage.qml` draws it block-level and centred with the alt text as an italic caption underneath. + +**The preview is asked for as an exact box, which is why the PNG header is parsed.** `MdImage.qml` sizes its container to the *request* rather than to the image: with no hint it is the full pane width and 200px tall, and the image is aspect-fitted inside that, so a wide capture sits in a letterbox with dead space above and below. Measured on real captures here, a 4:1 crop wasted about 106px of a 200px container and a 6.7:1 crop wasted about 140px. `previewBox` therefore computes a box from the image's real dimensions, scales it to fit 400x200, and emits it as the `raycast-width` and `raycast-height` hints the renderer reads, so the container matches the image and nothing is wasted. The scale is capped at 1 so a small capture is not blown up. + +The rendered height stays capped at 200px whatever is asked for (`Math.min(_rawH, 200, maxImageHeight)`), so this is a thumbnail rather than something readable, which is why the action panel still offers Open Screenshot for the full image. The alt text is short for the same reason it is not empty: whatever is there is drawn centred under the image, so a sentence reads as a stray line of prose, and it is the only thing left if the capture file has since been deleted. A preview of an unsent attachment is shown the same way, so a snip is visible before the question about it exists. **Code fences are labelled so Vicinae can colour them.** `MdCodeBlock.qml` already provides the dark panel, a header naming the language, and a Copy button, so none of that is built here. The language string is what turns on highlighting: `syntax::highlight` resolves it with `definitionForName(language)` and then `definitionForFileName("file." + language)`. So `bash` and `ts` colour correctly, while an unrecognised string such as `terminal` still renders a label but produces no highlighting at all, so a wrong label is worse than none. `code.ts` therefore labels bare fences conservatively, only when a signal is clear, and the default prompt asks for a language on every fence. diff --git a/src/__tests__/attachments.test.ts b/src/__tests__/attachments.test.ts index c935337..f24384e 100644 --- a/src/__tests__/attachments.test.ts +++ b/src/__tests__/attachments.test.ts @@ -2,10 +2,21 @@ import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { imageSource, mimeForPath, readAttachment } from "../lib/attachments"; +import { imageSource, mimeForPath, pngSize, previewBox, readAttachment } from "../lib/attachments"; const tempDirs: string[] = []; +/** A 24-byte PNG header, which is all `pngSize` reads. */ +function pngHeader(width: number, height: number): Buffer { + const bytes = Buffer.alloc(24); + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]).copy(bytes, 0); + bytes.writeUInt32BE(13, 8); + bytes.write("IHDR", 12, "ascii"); + bytes.writeUInt32BE(width, 16); + bytes.writeUInt32BE(height, 20); + return bytes; +} + async function tempDir(): Promise { const dir = await mkdtemp(join(tmpdir(), "lam-attach-")); tempDirs.push(dir); @@ -30,23 +41,83 @@ describe("mimeForPath", () => { }); }); +describe("pngSize", () => { + it("reads the dimensions out of the IHDR chunk", () => { + expect(pngSize(pngHeader(1600, 400))).toEqual({ width: 1600, height: 400 }); + }); + + it("refuses anything that is not a PNG", () => { + // Guessing at another format would put a wrong aspect ratio into the layout. + expect(pngSize(Buffer.from("not a png at all, just some text"))).toBeUndefined(); + expect(pngSize(Buffer.alloc(0))).toBeUndefined(); + }); + + it("refuses a header that is present but truncated", () => { + expect(pngSize(pngHeader(10, 10).subarray(0, 20))).toBeUndefined(); + }); + + it("refuses degenerate dimensions", () => { + expect(pngSize(pngHeader(0, 100))).toBeUndefined(); + }); +}); + +describe("previewBox", () => { + it("fits a wide capture to the width limit and keeps the aspect ratio", () => { + // This is the case that produced the dead space: 4:1 at full pane width was + // letterboxed into a 200px-tall container it barely used. + expect(previewBox(1600, 400)).toEqual({ width: 400, height: 100 }); + }); + + it("fits a 16:9 capture to the height limit instead", () => { + expect(previewBox(1920, 1080)).toEqual({ width: 356, height: 200 }); + }); + + it("fits a portrait capture by its height", () => { + expect(previewBox(400, 1600)).toEqual({ width: 50, height: 200 }); + }); + + it("does not blow a small capture up into a blurry one", () => { + expect(previewBox(100, 50)).toEqual({ width: 100, height: 50 }); + }); + + it("has nothing to say when the size is unknown", () => { + expect(previewBox(undefined, undefined)).toBeUndefined(); + expect(previewBox(1600, undefined)).toBeUndefined(); + expect(previewBox(1600, 0)).toBeUndefined(); + }); +}); + describe("imageSource", () => { it("produces a file URL, which is the only scheme the renderer resolves for a local file", () => { - expect(imageSource("/tmp/snip.png")).toBe("file:///tmp/snip.png"); + expect(imageSource({ path: "/tmp/snip.png" })).toBe("file:///tmp/snip.png"); }); it("escapes a path with a space, which the user's own screenshot directory has", () => { - expect(imageSource("/Users/x/Pictures/Screenshot 2026-10-04.png")).toBe( + expect(imageSource({ path: "/Users/x/Pictures/Screenshot 2026-10-04.png" })).toBe( "file:///Users/x/Pictures/Screenshot%202026-10-04.png", ); }); it("escapes non-ascii", () => { - expect(imageSource("/tmp/caf\u00e9.png")).toBe("file:///tmp/caf%C3%A9.png"); + expect(imageSource({ path: "/tmp/caf\u00e9.png" })).toBe("file:///tmp/caf%C3%A9.png"); }); it("keeps a bare path out of the markdown, because that resolves to nothing", () => { - expect(imageSource("/tmp/snip.png")).not.toBe("/tmp/snip.png"); + expect(imageSource({ path: "/tmp/snip.png" })).not.toBe("/tmp/snip.png"); + }); + + it("carries the size hints the renderer reads", () => { + // `parseImageSize` looks for these names, and resolves the path from the URL + // before the query, so the hints never reach the file name. + expect(imageSource({ path: "/tmp/snip.png", width: 1600, height: 400 })).toBe( + "file:///tmp/snip.png?raycast-width=400&raycast-height=100", + ); + }); + + it("omits the hints when the dimensions are unknown", () => { + expect(imageSource({ path: "/tmp/snip.png", width: undefined, height: undefined })).toBe( + "file:///tmp/snip.png", + ); }); }); @@ -78,6 +149,28 @@ describe("readAttachment", () => { expect(Buffer.from(attachment.base64, "base64")).toEqual(bytes); }); + it("keeps the dimensions, which the transcript needs long after this returns", async () => { + const dir = await tempDir(); + const path = join(dir, "snip.png"); + await writeFile(path, pngHeader(1600, 400)); + + const attachment = await readAttachment(path); + + expect(attachment.width).toBe(1600); + expect(attachment.height).toBe(400); + }); + + it("omits the dimensions for an image it cannot measure", async () => { + const dir = await tempDir(); + const path = join(dir, "snip.png"); + await writeFile(path, Buffer.from([0x89, 0x50, 0x4e, 0x47])); + + const attachment = await readAttachment(path); + + expect(attachment.width).toBeUndefined(); + expect(attachment.height).toBeUndefined(); + }); + it("fails loudly when the file is gone", async () => { await expect(readAttachment("/nonexistent/snip.png")).rejects.toThrow(); }); diff --git a/src/__tests__/markdown.test.ts b/src/__tests__/markdown.test.ts index 69b0373..2f3b08c 100644 --- a/src/__tests__/markdown.test.ts +++ b/src/__tests__/markdown.test.ts @@ -298,7 +298,7 @@ describe("attachments in the transcript", () => { message({ role: "user", text: "what is this?", attachments: [screenshot] }), ]); - expect(markdown).toContain("![Screenshot attached](file:///tmp/snip.png)"); + expect(markdown).toContain("![Screenshot](file:///tmp/snip.png)"); }); it("keeps the question above its image", () => { diff --git a/src/lib/attachments.ts b/src/lib/attachments.ts index d49d078..36a0842 100644 --- a/src/lib/attachments.ts +++ b/src/lib/attachments.ts @@ -1,15 +1,14 @@ import { readFile } from "node:fs/promises"; import { extname } from "node:path"; import { pathToFileURL } from "node:url"; -import type { PendingAttachment } from "./types"; +import type { Attachment, PendingAttachment } from "./types"; /** - * Reading attachments off disk. + * Reading attachments off disk, and describing them to the renderer. * * Kept separate from `capture.ts` because the two answer different questions: * capture is about getting an image *onto* the disk, this is about getting it - * back off again. It also means a future file picker or clipboard-file path can - * reuse this without touching the capture backends. + * back off again and telling Vicinae how to draw it. * * Imports no Vicinae module, so it unit tests with no runtime. */ @@ -23,10 +22,67 @@ const MIME_TYPES: Record = { ".gif": "image/gif", }; +/** + * The box a preview is fitted into. + * + * The height is not a preference: `MdImage.qml` clamps rendered height to 200px + * whatever is asked for (`Math.min(_rawH, 200, maxImageHeight)`), so a taller + * request is silently ignored. The width is here to keep a very wide capture + * from being scaled up past the detail pane it sits in. + */ +const PREVIEW_MAX_WIDTH = 400; +const PREVIEW_MAX_HEIGHT = 200; + export function mimeForPath(path: string): string { return MIME_TYPES[extname(path).toLowerCase()] ?? "application/octet-stream"; } +/** The eight bytes every PNG starts with. */ +const PNG_SIGNATURE = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; + +/** + * Pixel dimensions from a PNG's `IHDR` chunk, or `undefined` for anything else. + * + * Only PNG is handled, because that is the only thing a snip produces and + * guessing at other formats would be a lie. The header is 24 bytes, so this + * costs nothing on top of the read that is already happening. + */ +export function pngSize(bytes: Buffer): { width: number; height: number } | undefined { + if (bytes.length < 24) return undefined; + if (!PNG_SIGNATURE.every((byte, index) => bytes[index] === byte)) return undefined; + if (bytes.toString("ascii", 12, 16) !== "IHDR") return undefined; + + const width = bytes.readUInt32BE(16); + const height = bytes.readUInt32BE(20); + + return width > 0 && height > 0 ? { width, height } : undefined; +} + +/** + * The exact box to ask the renderer for, or `undefined` when the size is unknown. + * + * This is the whole point of reading the header. With no hint, `MdImage.qml` + * sizes its container to the pane width and 200px tall and then aspect-fits the + * image inside it, so a wide screenshot sits in a letterbox with dead space + * above and below it. Asking for a box that already matches the image's aspect + * ratio leaves none. + * + * The scale is capped at 1 so a small capture is not blown up into a blurry one. + */ +export function previewBox( + width: number | undefined, + height: number | undefined, +): { width: number; height: number } | undefined { + if (!width || !height || width <= 0 || height <= 0) return undefined; + + const scale = Math.min(1, PREVIEW_MAX_WIDTH / width, PREVIEW_MAX_HEIGHT / height); + + return { + width: Math.max(1, Math.round(width * scale)), + height: Math.max(1, Math.round(height * scale)), + }; +} + /** * A markdown image source for a local file. * @@ -36,18 +92,46 @@ export function mimeForPath(path: string): string { * `ImageURL::local`, and anything it does not recognise it returns as empty. * `pathToFileURL` does the escaping, so a capture under a directory with a space * in its name still resolves. + * + * The query is read from the URL before it is resolved to a path, so the size + * hints travel here without ever reaching the file name. They are spelled the + * way Raycast spelled them, which is what the parser looks for. */ -export function imageSource(path: string): string { - return pathToFileURL(path).href; +export function imageSource(attachment: Pick): string { + const url = pathToFileURL(attachment.path); + const box = previewBox(attachment.width, attachment.height); + + if (box) { + url.searchParams.set("raycast-width", String(box.width)); + url.searchParams.set("raycast-height", String(box.height)); + } + + return url.href; } /** * Reads an image into the shape a request needs. * * Ollama wants base64 with no `data:` URL prefix, which is why the payload is - * built here rather than handed straight to the renderer. + * built here rather than handed straight to the renderer. The dimensions are + * kept for the same reason the path is: the transcript needs them long after + * this call is over, and the preview layout depends on them. */ export async function readAttachment(path: string): Promise { const bytes = await readFile(path); - return { path, mime: mimeForPath(path), base64: bytes.toString("base64") }; + const size = pngSize(bytes); + + return { + path, + mime: mimeForPath(path), + base64: bytes.toString("base64"), + width: size?.width, + height: size?.height, + }; +} + +/** The persistable half of a pending attachment: everything except the base64 payload. */ +export function toStoredAttachment(attachment: PendingAttachment): Attachment { + const { path, mime, width, height } = attachment; + return { path, mime, width, height }; } diff --git a/src/lib/components/ChatView.tsx b/src/lib/components/ChatView.tsx index f13673a..89eee43 100644 --- a/src/lib/components/ChatView.tsx +++ b/src/lib/components/ChatView.tsx @@ -190,10 +190,14 @@ export function ChatView({ pending ? `${renderAttachments([pending])}\n\n${PENDING_NOTE}` : undefined, ].filter((note): note is string => Boolean(note)); + // The placeholder is what tells a user that the search bar is the composer. + // When a note above already says that, printing both put the same + // instruction twice, one line under the other. + const empty = notes.length === 0 ? EMPTY_TRANSCRIPT : ""; const transcript = - thread.length === 0 ? EMPTY_TRANSCRIPT : buildThreadMarkdown(thread, transcriptOptions); + thread.length === 0 ? empty : buildThreadMarkdown(thread, transcriptOptions); - return [...notes, transcript].join("\n\n"); + return [...notes, transcript].filter((part) => part.length > 0).join("\n\n"); }, [launchNote, snipNote, pending, thread, transcriptOptions]); const rows: SessionRow[] = useMemo(() => { diff --git a/src/lib/hooks/useChat.ts b/src/lib/hooks/useChat.ts index 377241b..71b6aa0 100644 --- a/src/lib/hooks/useChat.ts +++ b/src/lib/hooks/useChat.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { runAgent } from "../agent"; +import { toStoredAttachment } from "../attachments"; import { buildMessages, composeSystemPrompt } from "../history"; import { OllamaError } from "../ollama"; import type { ResolvedPreferences } from "../preferences"; @@ -126,9 +127,11 @@ export function useChat(options: UseChatOptions): UseChat { const preferences = preferencesRef.current; const base = sessionRef.current; - // Only the path and mime type are kept on the turn. The base64 payload is - // used for this one request and then dropped, so it never reaches storage. - const attachments = attachment ? [{ path: attachment.path, mime: attachment.mime }] : []; + // Only the path, mime type and dimensions are kept on the turn. The base64 + // payload is used for this one request and then dropped, so it never + // reaches storage. The dimensions have to survive because the transcript + // lays the preview out long after the payload is gone. + const attachments = attachment ? [toStoredAttachment(attachment)] : []; // The turn is inserted immediately with an empty response so the UI shows // the question while the answer streams in beneath it. diff --git a/src/lib/markdown.ts b/src/lib/markdown.ts index badadba..184f831 100644 --- a/src/lib/markdown.ts +++ b/src/lib/markdown.ts @@ -32,13 +32,13 @@ export const STREAMING_CURSOR = "▍"; * than something readable, which is why the action panel also offers Open * Screenshot for the full image. * - * The alt text is not decoration: it is what remains if the file has since been - * deleted, so it says what the picture was rather than leaving a blank space. + * The alt text is short on purpose. Whatever is here is drawn centred under the + * image, so a sentence reads as a stray line of prose floating in the middle of + * the pane. It is also the only thing left if the file has since been deleted, + * which is why it is not empty. */ export function renderAttachments(attachments: Attachment[]): string { - return attachments - .map((attachment) => `![Screenshot attached](${imageSource(attachment.path)})`) - .join("\n\n"); + return attachments.map((attachment) => `![Screenshot](${imageSource(attachment)})`).join("\n\n"); } /** Which optional sections the transcript includes. Driven by preferences. */ diff --git a/src/lib/schemas.ts b/src/lib/schemas.ts index d03567d..56b8f7b 100644 --- a/src/lib/schemas.ts +++ b/src/lib/schemas.ts @@ -17,6 +17,10 @@ export const ToolCallRecordSchema = z.object({ export const AttachmentSchema = z.object({ path: z.string(), mime: z.string().default("image/png"), + // Absent on sessions written before the header was read, which is fine: a + // preview without dimensions falls back to the renderer's own sizing. + width: z.number().optional(), + height: z.number().optional(), }); export const ChatTurnSchema = z.object({ diff --git a/src/lib/types.ts b/src/lib/types.ts index e88abbe..7ce89f3 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -60,6 +60,16 @@ export interface Attachment { path: string; /** Sent to Ollama next to the base64 payload. */ mime: string; + /** + * Pixel dimensions, read from the file's header. + * + * Kept because the transcript has to lay the preview out long after the file + * was read, and Vicinae sizes an image's container to the request rather than + * to the image, which leaves dead space unless the request has the right + * aspect ratio. Absent for a format whose header we do not parse. + */ + width?: number; + height?: number; } /** @@ -74,7 +84,6 @@ export interface PendingAttachment extends Attachment { /** Base64 of the image, with no `data:` URL prefix. Ollama wants it bare. */ base64: string; } - /** What a tool actually returned during a turn, kept for display and debugging. */ export interface ToolCallRecord { name: string; -- 2.54.0 From 35d88f635c5dffe9ab53065dc01cd6bd1d3d8255 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:44:29 +0800 Subject: [PATCH 07/11] feat: let the model answer with tables, lists and headings The model refused to write a table and described one instead: a bulleted list of rows with the column names in inline code. That was our instruction, not a rendering limit. The default prompt asked for brevity in a narrow window and then said "skip large headings and wide tables", so the model obeyed and flattened the answer. Vicinae renders tables properly. The screenshot that prompted this is a rendered one: `MdTable.qml` is wired to block type 5, and `markdown-model.cpp` attaches the `table` GFM extension. Nothing was missing. - The prompt now names what actually renders (tables, bulleted and numbered lists, headings, blockquotes, bold, italics, links, code) and asks for the shape that fits, with a cap of about four narrow columns because the detail pane really is narrow. It also says outright not to hand back a table as a bulleted list of rows. - `separateTables` adds the blank line a table needs in front of it. GFM tables cannot interrupt a paragraph, so a table written straight under a line of prose renders as a paragraph of pipes and dashes. Only a genuine table start is touched, which is a row followed by a delimiter row; prose with a pipe, a table already at a block boundary, and a fenced example are all left alone. - A regression test asserts a real table survives the whole repair pass untouched: pipes are not fences, not links, and not lone spans. Verified: 304 tests, tsc, eslint, prettier, and vici build. --- README.md | 5 +++ src/__tests__/code.test.ts | 78 ++++++++++++++++++++++++++++++++++++++ src/lib/code.ts | 66 +++++++++++++++++++++++++++++--- src/lib/constants.ts | 4 +- 4 files changed, 147 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 3de1a1a..626ff59 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ Hit a global shortcut, type a question, get a streamed answer without leaving wh - **Streaming answers.** Tokens render as they arrive, in a markdown side panel. - **Persistent conversations.** Every chat is saved locally. The sidebar lists them by recency, and reopening the command picks up where you left off, unless it has been idle longer than you allow. - **Readable code.** Fenced blocks render in a dark panel with the language named in a header, syntax colouring, and a Copy button. +- **Real markdown.** Tables, bulleted and numbered lists, headings, blockquotes, bold, italics and links all render natively, and the default prompt asks for the shape that fits the answer rather than flattening everything into prose. - **Tools.** Lam decides when to call them: current time, location context, web search, and page fetching. - **Screenshots, two ways.** Snip a region into the conversation you are already in from the action panel, or run **Snip to Lam** to start a new chat about it. The launcher window gets out of the way first, so it never ends up in the shot, and the capture appears in the transcript as a thumbnail with Open Screenshot for the full image. Neither is offered unless the current model can actually see images. - **Chat only.** Lam answers questions. It does not touch your files or run commands. The one exception is capture, and it only ever happens because you pressed the key for it. @@ -199,6 +200,10 @@ The rendered height stays capped at 200px whatever is asked for (`Math.min(_rawH **A URL is only clickable as a markdown link, so `code.ts` takes them back out of code.** Vicinae's parser attaches the `table` and `strikethrough` GFM extensions and nothing else, so the `autolink` extension is off: a bare URL is plain dead text. Clickability comes from `MarkdownModel` turning `CMARK_NODE_LINK` into an ``, which the selection controller opens on release. A URL the model fenced is therefore the worst possible presentation, a dark panel with a Copy button and no way to open the address. `restoreUrlLinks` recovers three narrow shapes (a fence whose entire body is URLs, a lone `` `https://...` `` span, and a line that is only a URL) into `[url](url)` links, and the default prompt now says never to fence a URL in the first place. Anything that is not one of those three shapes is copied through, so a `curl` invocation containing an address is never rewritten. +**A table needs a blank line in front of it, and the prompt alone does not guarantee one.** Vicinae renders tables properly (`MdTable.qml`, block type 5), but GFM tables cannot interrupt a paragraph, so a table written directly under a line of prose is not a table at all: it renders as a paragraph of pipes and dashes. The blank line looks unnecessary, so models leave it out. `separateTables` adds it, and only in front of a genuine table start, which is a row followed by a delimiter row. Prose containing a pipe is left alone, a table already at a block boundary is left alone, and a fenced example is never touched. + +**The prompt used to forbid tables, which is worth not repeating.** It asked for brevity in a narrow window and then said "skip large headings and wide tables", and a model follows that instruction by describing the table it was about to write instead of writing it: a bulleted list of rows, with the column names in inline code. Brevity is about length, not about which markdown is allowed, so the guidance now names what renders and asks for the shape that fits, with a cap of about four narrow columns because the detail pane really is narrow. + **Streamed deltas are buffered in refs and flushed to state on a timer** (`FLUSH_INTERVAL_MS`). Re-rendering the transcript per token makes the launcher visibly janky. **The logic layer has no Vicinae imports.** `agent.ts`, `ollama.ts`, `tools.ts` and friends are plain TypeScript, which is why they unit test without a runtime. A test enforces this. diff --git a/src/__tests__/code.test.ts b/src/__tests__/code.test.ts index f56a876..1c14202 100644 --- a/src/__tests__/code.test.ts +++ b/src/__tests__/code.test.ts @@ -5,6 +5,7 @@ import { promoteStandaloneCodeSpans, repairCodeBlocks, restoreUrlLinks, + separateTables, } from "../lib/code"; describe("inferLanguage", () => { @@ -236,11 +237,88 @@ describe("restoreUrlLinks", () => { }); }); +describe("separateTables", () => { + /** The table from the answer that prompted this, verbatim. */ + const table = [ + "| snip | source | aspect | dead space before | box after |", + "| --- | --- | --- | --- | --- |", + "| `…153223` | 626×184 | 3.40 | **~82px** | 400×118 |", + ].join("\n"); + + it("gives a table the blank line it needs to be a table", () => { + // GFM tables cannot interrupt a paragraph, so glued to the line above they + // render as a paragraph of pipes and dashes. + const repaired = separateTables(`Here are the numbers:\n${table}`); + + expect(repaired).toBe(`Here are the numbers:\n\n${table}`); + }); + + it("leaves a table alone when it already has one", () => { + const already = `Here are the numbers:\n\n${table}`; + expect(separateTables(already)).toBe(already); + }); + + it("leaves a table alone when it opens the answer", () => { + expect(separateTables(table)).toBe(table); + }); + + it("leaves a table alone when it follows a fence", () => { + const input = `\`\`\`bash\npwd\n\`\`\`\n${table}`; + expect(separateTables(input)).toBe(input); + }); + + it("does not touch a table inside a fence", () => { + // A table shown as example code has to stay exactly as written, blank line + // or not. + const fenced = `\`\`\`markdown\n${table}\n\`\`\``; + expect(separateTables(fenced)).toBe(fenced); + }); + + it("leaves prose containing a pipe alone", () => { + const input = "Use the pipe character:\nls | grep foo"; + expect(separateTables(input)).toBe(input); + }); + + it("does not insert a blank line before a data row", () => { + // Only the header starts a table. A blank line inside one would split it. + const input = `text\n${table}\n| a | b | c | d | e |`; + expect(separateTables(input)).toBe(`text\n\n${table}\n| a | b | c | d | e |`); + }); + + it("recognises a one-dash delimiter, which GFM allows", () => { + const minimal = "| a | b |\n| - | - |\n| 1 | 2 |"; + expect(separateTables(`text\n${minimal}`)).toBe(`text\n\n${minimal}`); + }); + + it("recognises alignment colons", () => { + const aligned = "| a | b |\n|:---|:---:|\n| 1 | 2 |"; + expect(separateTables(`text\n${aligned}`)).toBe(`text\n\n${aligned}`); + }); + + it("is idempotent", () => { + const once = separateTables(`text\n${table}`); + expect(separateTables(once)).toBe(once); + }); +}); + describe("repairCodeBlocks", () => { it("promotes a lone command and labels it in one pass", () => { expect(repairCodeBlocks("`pwd`")).toBe("```bash\npwd\n```"); }); + it("leaves a table exactly as the model wrote it", () => { + // The whole repair pass runs over every answer, and a table has to survive + // it untouched: pipes are not fences, not links, and not lone spans. + const table = [ + "| snip | source | aspect | box after |", + "| --- | --- | --- | --- |", + "| `…153223` | 626×184 | 3.40 | 400×118 |", + "| [docs](https://example.com) | `pwd` | https://example.com | 400×60 |", + ].join("\n"); + + expect(repairCodeBlocks(table)).toBe(table); + }); + it("does not fence a lone URL", () => { // Restoring links runs before promotion, so a recovered URL never becomes a // fence again. Fencing it here was the bug the user saw. diff --git a/src/lib/code.ts b/src/lib/code.ts index 36bf8e2..429bc36 100644 --- a/src/lib/code.ts +++ b/src/lib/code.ts @@ -106,6 +106,17 @@ const STANDALONE_URL_SPAN = /^`([^`\n]+)`[.!]?$/; /** A bullet or numbered-list marker, kept so a list of links stays a list. */ const LIST_MARKER = /^([ \t]*(?:[-*+]|\d+[.)])[ \t]+)(.*)$/; +/** A GFM table row: it opens and closes with a pipe. */ +const TABLE_ROW = /^\s*\|.*\|\s*$/; +/** + * The delimiter row under a header. + * + * The test is not how many dashes there are, because GFM wants only one per + * column. It is that the line could be nothing else: pipes, dashes, colons and + * spaces, with at least one dash to tell it apart from a row of empty cells. + */ +const TABLE_DELIMITER_ROW = /^\s*\|?[\s:|-]*-[\s:|-]*\|?\s*$/; + function significantLines(code: string): string[] { return code .split("\n") @@ -401,14 +412,59 @@ export function restoreUrlLinks(markdown: string): string { return out.join("\n"); } +/** + * Inserts the blank line a table needs to start at all. + * + * A GFM table cannot interrupt a paragraph, so a table written straight after a + * line of prose is not a table: it renders as a paragraph of pipes and dashes, + * which is how a perfectly good answer turns into noise. The blank line looks + * unnecessary, so models leave it out. + * + * Only a real table start is touched, which is a row followed by a delimiter + * row, so prose that happens to contain a pipe is left alone. Fences are copied + * through untouched for the same reason, since a table shown as example code + * must stay exactly as written. + */ +export function separateTables(markdown: string): string { + const lines = markdown.split("\n"); + const out: string[] = []; + let index = 0; + + while (index < lines.length) { + if (OPENING_FENCE.test(lines[index])) { + index = copyFence(lines, out, index); + continue; + } + + const startsTable = + TABLE_ROW.test(lines[index]) && index + 1 < lines.length && TABLE_DELIMITER_ROW.test(lines[index + 1]); + + // Only when something would otherwise swallow it. A table already at a block + // boundary, after a blank line or after a fenced block, needs nothing: the + // fence has already ended the preceding block. + const previous = out[out.length - 1]; + const atBlockBoundary = + previous === undefined || previous.trim().length === 0 || CLOSING_FENCE.test(previous); + + if (startsTable && !atBlockBoundary) out.push(""); + + out.push(lines[index]); + index += 1; + } + + return out.join("\n"); +} + /** * The full pre-render pass, in order. * - * Links are restored first so a recovered URL leaves no span for the promoter - * to fence again. Promotion then runs before labelling, so a promoted block is - * seen as an ordinary fence by the labeller, which is also harmless when - * promotion already inferred a language. + * Tables are separated first because they change block boundaries: a blank line + * has to exist before anything else looks at the line. Links are restored next + * so a recovered URL leaves no span for the promoter to fence again. Promotion + * then runs before labelling, so a promoted block is seen as an ordinary fence + * by the labeller, which is also harmless when promotion already inferred a + * language. */ export function repairCodeBlocks(markdown: string): string { - return labelCodeFences(promoteStandaloneCodeSpans(restoreUrlLinks(markdown))); + return labelCodeFences(promoteStandaloneCodeSpans(restoreUrlLinks(separateTables(markdown)))); } diff --git a/src/lib/constants.ts b/src/lib/constants.ts index 9bdd1af..2d3e51b 100644 --- a/src/lib/constants.ts +++ b/src/lib/constants.ts @@ -24,7 +24,9 @@ export const DEFAULT_SYSTEM_PROMPT = `You are Lam, a friendly assistant in the V Sound like a person, not a document. Use contractions, talk about "you" and "I", and a short natural remark is welcome ("Sure, easy one", "Ah, that trips people up"). Performed enthusiasm and filler added to seem helpful are not. -Lead with the answer, then the detail. This renders in a narrow window, so stay brief: a tight sentence or two, or a short list, then stop. Keep paragraphs short and skip large headings and wide tables. +Lead with the answer, then the detail. This renders in a narrow window, so stay brief: a tight sentence or two, or a short list, then stop. Keep paragraphs short. + +Use the markdown this window actually draws rather than flattening everything into prose. It renders tables, bulleted and numbered lists, headings, blockquotes, bold, italics, inline code and fenced code, so pick the shape that fits. A table is the right shape for comparing three or more things across the same few attributes, so write the table itself: never hand back a table as a bulleted list of rows, and never describe a table you could simply write. Keep it to about four narrow columns so it fits, give it a header row, and leave a blank line before it. A list is for steps or for independent items. Reach for a heading only when an answer genuinely has sections, since at this size a heading usually costs a line and buys nothing. Write plainly: - Never use an em dash. Use a comma, a colon, or a full stop. -- 2.54.0 From 400f81fa6caa32da26f50af9cfe257caa08dd360 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:50:45 +0800 Subject: [PATCH 08/11] fix: stop reporting a timezone's name as the user's city get_location printed "City: Manila", derived from the last segment of the IANA zone name. That is the timezone database's representative place, not the user's: Asia/Manila is one zone covering the entire Philippines, Asia/Kolkata covers all of India, Europe/Oslo all of Norway. It was wrong for anyone outside the zone's namesake, and confidently so, because a model asked about the weather uses it without hesitating. The README claimed the value was region-level, so the output also contradicted its own docs. - The zone's label is still reported, as `Timezone area`, and named for what it is. Both the field and the tool note say outright that it is not the user's city, and the tool description repeats it. - The locale region subtag is now used, which is a declaration rather than an inference: a locale of en-PH is the Philippines because the user set it. - A Location preference was added. It is the only exact place this extension has, it is reported verbatim and first, and when it is set the note tells the model to prefer it over anything inferred. - The continent grouping from the zone name was dropped: "Asia" next to a region of "Philippines" reads as two answers to the same question. - ToolContext takes the place as an injected value, keeping tools.ts free of Vicinae imports as the module wiring test requires. Verified: 316 tests, tsc, eslint, prettier, and vici build. --- package.json | 7 +++ src/__tests__/location.test.ts | 64 ++++++++++++++++++++---- src/__tests__/preferences.test.ts | 12 +++++ src/__tests__/tools.test.ts | 33 ++++++++++++- src/lib/agent.ts | 1 + src/lib/hooks/useChat.ts | 6 ++- src/lib/location.ts | 82 ++++++++++++++++++++++++++----- src/lib/preferences.ts | 10 ++++ src/lib/tools.ts | 45 +++++++++++++---- 9 files changed, 225 insertions(+), 35 deletions(-) diff --git a/package.json b/package.json index cfd9b83..eb67b68 100644 --- a/package.json +++ b/package.json @@ -65,6 +65,13 @@ "type": "textfield", "required": false }, + { + "name": "location", + "title": "Location", + "description": "The place Lam should treat as yours, for example Cebu City, Philippines. Optional. Without it Lam falls back to your device timezone and locale, which are accurate to a region and not to a city, so anything that depends on exactly where you are (weather, opening hours) is worth filling in.", + "type": "textfield", + "required": false + }, { "name": "webSearchEnabled", "title": "Web Tools", diff --git a/src/__tests__/location.test.ts b/src/__tests__/location.test.ts index b61e40f..3f6be9e 100644 --- a/src/__tests__/location.test.ts +++ b/src/__tests__/location.test.ts @@ -1,21 +1,53 @@ import { describe, expect, it } from "vitest"; -import { describeTimezone, getLocationContext, utcOffsetLabel, weekdayName } from "../lib/location"; +import { + describeTimezoneArea, + getLocationContext, + localeRegion, + utcOffsetLabel, + weekdayName, +} from "../lib/location"; -describe("describeTimezone", () => { - it("splits a simple zone into region and city", () => { - expect(describeTimezone("Asia/Manila")).toEqual({ region: "Asia", city: "Manila" }); +describe("describeTimezoneArea", () => { + it("keeps the zone's own label", () => { + expect(describeTimezoneArea("Asia/Manila")).toEqual({ area: "Manila" }); }); it("handles zones with more than two segments", () => { - expect(describeTimezone("America/Argentina/Salta")).toEqual({ region: "America", city: "Salta" }); + expect(describeTimezoneArea("America/Argentina/Salta")).toEqual({ area: "Salta" }); }); - it("underscores become spaces", () => { - expect(describeTimezone("America/New_York")).toEqual({ region: "America", city: "New York" }); + it("turns underscores into spaces", () => { + expect(describeTimezoneArea("America/New_York")).toEqual({ area: "New York" }); + }); + + it("drops the continent grouping, which answers a question nobody asked", () => { + // "Asia" beside a locale region of "Philippines" reads as two answers to + // the same question. + expect(describeTimezoneArea("Asia/Manila")).not.toHaveProperty("region"); }); it("returns nothing for a bare zone like UTC", () => { - expect(describeTimezone("UTC")).toEqual({}); + expect(describeTimezoneArea("UTC")).toEqual({}); + }); +}); + +describe("localeRegion", () => { + it("reads the region the user configured, rather than inferring one", () => { + expect(localeRegion("en-PH")).toEqual({ code: "PH", name: "Philippines" }); + }); + + it("handles other regions", () => { + expect(localeRegion("de-DE")?.name).toBe("Germany"); + expect(localeRegion("en-US")?.code).toBe("US"); + }); + + it("has nothing to say for a locale with no region", () => { + expect(localeRegion("en")).toBeUndefined(); + }); + + it("does not throw on a malformed locale", () => { + expect(localeRegion("not a locale")).toBeUndefined(); + expect(localeRegion("")).toBeUndefined(); }); }); @@ -54,10 +86,11 @@ describe("getLocationContext", () => { const context = getLocationContext(new Date("2026-01-02T03:04:05Z"), "Asia/Manila"); expect(context.timezone).toBe("Asia/Manila"); - expect(context.city).toBe("Manila"); - expect(context.region).toBe("Asia"); expect(context.utcOffset).toContain("+08:00"); expect(context.locale.length).toBeGreaterThan(0); + // The zone's label is reported as such, never as the user's city. + expect(context.timezoneArea).toBe("Manila"); + expect(context).not.toHaveProperty("city"); }); it("still returns a usable context for an unknown zone", () => { @@ -65,4 +98,15 @@ describe("getLocationContext", () => { expect(context.timezone).toBe("Not/AReal_Zone"); expect(context.utcOffset).toBe("UTC"); }); + + it("carries the configured place through verbatim", () => { + const context = getLocationContext(new Date(), "Asia/Manila", " Cebu City, Philippines "); + expect(context.place).toBe("Cebu City, Philippines"); + }); + + it("treats an empty or whitespace place as unset", () => { + expect(getLocationContext(new Date(), "Asia/Manila", "").place).toBeUndefined(); + expect(getLocationContext(new Date(), "Asia/Manila", " ").place).toBeUndefined(); + expect(getLocationContext(new Date(), "Asia/Manila").place).toBeUndefined(); + }); }); diff --git a/src/__tests__/preferences.test.ts b/src/__tests__/preferences.test.ts index 1a894f8..25f703b 100644 --- a/src/__tests__/preferences.test.ts +++ b/src/__tests__/preferences.test.ts @@ -53,6 +53,18 @@ describe("resolvePreferences", () => { expect(resolved.showReasoning).toBe(true); expect(resolved.think).toBeUndefined(); expect(resolved.resumeTimeoutMs).toBe(HOUR); + // An empty string rather than undefined, so callers never branch on it. + expect(resolved.place).toBe(""); + }); + + it("keeps the configured location, trimmed", () => { + expect(resolvePreferences({ location: " Cebu City, Philippines " }).place).toBe( + "Cebu City, Philippines", + ); + }); + + it("treats a whitespace-only location as unset", () => { + expect(resolvePreferences({ location: " " }).place).toBe(""); }); it("honours the display toggles being switched off", () => { diff --git a/src/__tests__/tools.test.ts b/src/__tests__/tools.test.ts index 7265b4c..ffd806f 100644 --- a/src/__tests__/tools.test.ts +++ b/src/__tests__/tools.test.ts @@ -225,13 +225,42 @@ describe("local tools", () => { expect(result.content).toContain("Current date and time:"); }); - it("describes location as a region rather than an address", async () => { + it("describes location as a region rather than an address when nothing is configured", async () => { const result = await executeTool("get_location", {}, context); expect(result.isError).toBe(false); expect(result.content).toContain("Timezone:"); expect(result.content).toContain("Locale:"); - expect(result.content).toContain("not a precise address"); + expect(result.content).toContain("no Location is set in preferences"); + expect(result.content).toContain("Ask which city they are in"); + }); + + it("never claims a city derived from the timezone name", async () => { + // `Asia/Manila` is one zone covering the whole Philippines, so reporting + // "City: Manila" was wrong for anyone in Cebu and confidently so. + const result = await executeTool("get_location", {}, context); + + expect(result.content).not.toMatch(/^City:/m); + expect(result.content).toContain("The timezone area is the timezone's own name, not the user's city."); + }); + + it("puts the configured place first and says to prefer it", async () => { + const result = await executeTool("get_location", {}, { + ...context, + place: "Cebu City, Philippines", + }); + + expect(result.content).toContain("Place: Cebu City, Philippines"); + expect(result.content).toContain("prefer it"); + expect(result.content).not.toContain("no Location is set"); + expect(result.content.indexOf("Place:")).toBeLessThan(result.content.indexOf("Timezone:")); + }); + + it("still reports the timezone alongside a configured place, for local time questions", async () => { + const result = await executeTool("get_location", {}, { ...context, place: "Cebu City" }); + + expect(result.content).toContain("Timezone:"); + expect(result.content).toContain("UTC offset:"); }); }); diff --git a/src/lib/agent.ts b/src/lib/agent.ts index 34d7005..c50c9e1 100644 --- a/src/lib/agent.ts +++ b/src/lib/agent.ts @@ -114,6 +114,7 @@ export async function runAgent(options: AgentOptions): Promise { const result = await executeTool(name, args, { host: options.toolContext.host, apiKey: options.toolContext.apiKey, + place: options.toolContext.place, now: options.toolContext.now, signal: options.signal, }); diff --git a/src/lib/hooks/useChat.ts b/src/lib/hooks/useChat.ts index 71b6aa0..dec8604 100644 --- a/src/lib/hooks/useChat.ts +++ b/src/lib/hooks/useChat.ts @@ -174,7 +174,11 @@ export function useChat(options: UseChatOptions): UseChat { think: preferences.think, messages, tools: toolsFor({ webEnabled: preferences.webEnabled }), - toolContext: { host: preferences.host, apiKey: preferences.apiKey }, + toolContext: { + host: preferences.host, + apiKey: preferences.apiKey, + place: preferences.place, + }, signal: controller.signal, events: { onContentDelta: (delta) => { diff --git a/src/lib/location.ts b/src/lib/location.ts index 9e1dcdf..754defe 100644 --- a/src/lib/location.ts +++ b/src/lib/location.ts @@ -1,30 +1,76 @@ /** * Time and place, derived locally. * - * `get_location` is intentionally timezone-derived rather than a GPS or IP - * lookup. It answers the questions people actually ask a launcher ("what time - * is it there", "is it late in London") without shipping the user's IP address - * to a third-party geolocation service on every tool call. It is also pure, so - * it is cheap to test. + * `get_location` is timezone-derived rather than a GPS or IP lookup, so the + * user's IP is never shipped to a third-party geolocation service on every tool + * call. It is also pure, so it is cheap to test. + * + * The correction that matters here is that a timezone name is **not** a city. + * The IANA database names a zone after one representative place, and a single + * zone can cover a whole country: "Asia/Manila" is the Philippines, + * "Asia/Kolkata" is India, "Europe/Oslo" is Norway, "Australia/Brisbane" is + * Queensland. Reporting "Manila" as the user's city is simply wrong for anyone + * in Cebu, and being confidently wrong is worse than being vague, because a + * model asked about the weather will use it without hesitating. So the zone's + * label is still reported, but named for what it is instead of being dressed up + * as the user's city. + * + * The only exact answer available without asking a third party is the Location + * preference, which the user fills in themselves. It is reported verbatim and + * preferred over anything inferred. */ export interface LocationContext { timezone: string; utcOffset: string; locale: string; + /** Country code from the locale, e.g. "PH". The user's own setting rather than an inference. */ + regionCode?: string; + /** Readable name for that region, e.g. "Philippines". */ region?: string; - city?: string; + /** + * The timezone's own area label, e.g. "Manila". + * + * Deliberately not called a city. See the note at the top of this file. + */ + timezoneArea?: string; + /** The user's configured Location preference, verbatim. The only exact value here. */ + place?: string; } -/** Splits an IANA zone like "Asia/Manila" or "America/Argentina/Salta" into readable parts. */ -export function describeTimezone(timeZone: string): { region?: string; city?: string } { +/** + * Splits an IANA zone like "Asia/Manila" or "America/Argentina/Salta". + * + * Only the last segment is kept, and the continent grouping is dropped: "Asia" + * next to a locale region of "Philippines" reads as two answers to the same + * question, and the zone's grouping answers a question nobody asked. + */ +export function describeTimezoneArea(timeZone: string): { area?: string } { const parts = timeZone.split("/").filter(Boolean); if (parts.length < 2) return {}; - const region = parts[0]; - const city = parts[parts.length - 1].replace(/_/g, " "); + return { area: parts[parts.length - 1].replace(/_/g, " ") || undefined }; +} - return { region, city: city || undefined }; +/** + * The region from the locale subtag. + * + * This is a declaration rather than an inference: a locale of "en-PH" says the + * user set their region to the Philippines. That makes it a better answer than + * anything read out of a zone name, and it costs nothing. + */ +export function localeRegion(locale: string): { code: string; name: string } | undefined { + try { + const code = new Intl.Locale(locale).region; + if (!code) return undefined; + + const name = new Intl.DisplayNames(["en"], { type: "region" }).of(code); + return { code, name: name ?? code }; + } catch { + // A malformed locale, or an ICU build without region display names. Not + // worth failing the whole answer over. + return undefined; + } } /** "GMT+08:00" on a modern V8; falls back to a short name if `longOffset` is unavailable. */ @@ -56,7 +102,11 @@ function resolveTimeZone(timeZone?: string): string { } } -export function getLocationContext(date: Date = new Date(), timeZone?: string): LocationContext { +export function getLocationContext( + date: Date = new Date(), + timeZone?: string, + place?: string, +): LocationContext { const zone = resolveTimeZone(timeZone); let locale = "en-US"; @@ -66,11 +116,17 @@ export function getLocationContext(date: Date = new Date(), timeZone?: string): // Keep the fallback locale. } + const region = localeRegion(locale); + const configured = place?.trim(); + return { timezone: zone, utcOffset: utcOffsetLabel(date, zone), locale, - ...describeTimezone(zone), + regionCode: region?.code, + region: region?.name, + timezoneArea: describeTimezoneArea(zone).area, + place: configured && configured.length > 0 ? configured : undefined, }; } diff --git a/src/lib/preferences.ts b/src/lib/preferences.ts index 95dce44..8e7740d 100644 --- a/src/lib/preferences.ts +++ b/src/lib/preferences.ts @@ -15,6 +15,7 @@ export interface LamPreferences { model?: string; ollamaHost?: string; systemPrompt?: string; + location?: string; webSearchEnabled?: boolean; thinking?: string; showToolCalls?: boolean; @@ -28,6 +29,14 @@ export interface ResolvedPreferences { model: string; host: string; systemPrompt: string; + /** + * The place the user says they are, verbatim, or an empty string. + * + * The only exact location this extension has. Everything else is inferred from + * the device timezone and locale, which is accurate to a region rather than a + * city, and a timezone name is not a city at all. + */ + place: string; webEnabled: boolean; /** Undefined when reasoning is off, which is what the API client expects. */ think?: string; @@ -78,6 +87,7 @@ export function resolvePreferences(raw: LamPreferences): ResolvedPreferences { model: (raw.model ?? "").trim() || DEFAULT_MODEL, host: (raw.ollamaHost ?? "").trim() || DEFAULT_HOST, systemPrompt: (raw.systemPrompt ?? "").trim(), + place: (raw.location ?? "").trim(), // Defaults to on: the preference is a checkbox the user has to actively untick. webEnabled: raw.webSearchEnabled ?? true, think: thinking && thinking !== "off" ? thinking : undefined, diff --git a/src/lib/tools.ts b/src/lib/tools.ts index 5a00bdd..9917cae 100644 --- a/src/lib/tools.ts +++ b/src/lib/tools.ts @@ -19,6 +19,13 @@ export interface ToolContext { signal?: AbortSignal; /** Injectable clock so tests are deterministic. */ now?: Date; + /** + * The user's Location preference, if they set one. + * + * Injected rather than read here, because this module is deliberately free of + * Vicinae imports and preferences arrive through that API. + */ + place?: string; } export interface ToolResult { @@ -45,7 +52,7 @@ export const TOOL_DEFINITIONS: ToolDefinition[] = [ function: { name: "get_location", description: - "Get the user's approximate location context: timezone, UTC offset, locale, region, and city. Use it for local questions such as weather, business hours, or travel timing. This is derived from the device timezone, not GPS, so treat it as a region rather than an address.", + "Get where the user is, for local questions such as weather, business hours, or travel timing. Returns the place they configured in preferences, plus the timezone, UTC offset, locale and region. Without a configured place this is region-level, not an address, and the timezone area it reports is the timezone's own name rather than the user's city, so never treat it as one.", parameters: { type: "object", properties: {}, required: [] }, }, }, @@ -140,7 +147,7 @@ export async function executeTool( return { content: describeNow(context.now), isError: false }; case "get_location": - return { content: describeLocation(context.now), isError: false }; + return { content: describeLocation(context.now, context.place), isError: false }; case "web_search": return await runWebSearch(args, context); @@ -172,18 +179,38 @@ function describeNow(now?: Date): string { ].join("\n"); } -function describeLocation(now?: Date): string { +function describeLocation(now?: Date, place?: string): string { const date = now ?? new Date(); - const location = getLocationContext(date); + const location = getLocationContext(date, undefined, place); - const lines = [ + const lines: string[] = []; + + // The configured place comes first because it is the only exact thing here, + // and the model should reach for it before anything else. + if (location.place) lines.push(`Place: ${location.place}`); + + lines.push( `Timezone: ${location.timezone}`, `UTC offset: ${location.utcOffset}`, `Locale: ${location.locale}`, - ]; - if (location.city) lines.push(`City: ${location.city}`); - if (location.region) lines.push(`Region: ${location.region}`); - lines.push("Note: derived from the device timezone. It indicates region, not a precise address."); + ); + + if (location.region) { + lines.push(`Region: ${location.region}${location.regionCode ? ` (${location.regionCode})` : ""}`); + } + if (location.timezoneArea) { + lines.push(`Timezone area: ${location.timezoneArea}`); + } + + lines.push( + location.place + ? "Note: Place is the user's own setting, so prefer it for anything that depends on exactly where they are." + : "Note: no Location is set in preferences. Everything above is derived from the device timezone and locale, so it is accurate to a region and not to a city. Ask which city they are in when an exact place matters.", + ); + + if (location.timezoneArea) { + lines.push("The timezone area is the timezone's own name, not the user's city."); + } return lines.join("\n"); } -- 2.54.0 From 51c16b678f56703ec72e7124cd08b90ecc673718 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:52:22 +0800 Subject: [PATCH 09/11] feat: opt-in IP geolocation for get_location Adds the one thing the extension could not answer before: roughly where the user actually is, rather than which zone their clock is set to. - `lib/geo.ts` resolves a place from ipapi.co. Chosen because it is HTTPS and needs no API key; ip-api.com, the other obvious free option, is HTTP-only on its free tier, which is not a place to send an IP address. - Off by default, and skipped entirely when the user has set Location: the better answer is already there, so spending their privacy on a worse one would be indefensible. `ipPlace` is only wired into ToolContext when the preference is on, so an off setting cannot reach the network at all. - Cached for 6 hours. A city does not change between questions, and every lookup hands the provider another copy of the address. Set Up Lam Agent clears it on re-test, alongside the vision cache. - Failures are returned, not swallowed: a rate limit or an offline machine says so in the tool output while the region answer stays available. The provider signals some failures with error: true and HTTP 200, so the body is checked as well as the status. - The tool output always names the provider and calls the result city-level rather than exact. It also flags an IP timezone that disagrees with the device clock, which usually means a VPN. - The README claimed no geolocation service ever saw the user's address. That is now conditional, so the section was rewritten rather than left to quietly become untrue. Deliberately not done: GPS. CoreLocation has no CLI on macOS and GeoClue's usual provider resolves nearby Wi-Fi through a Mozilla service anyway. Verified: 341 tests (25 new), tsc, eslint, prettier, and vici build. I did not make a live API call, because doing so would send this machine's IP to the provider as the exact flow being opted into. --- README.md | 14 +- package.json | 9 ++ src/__tests__/geo.test.ts | 205 ++++++++++++++++++++++++++++ src/__tests__/preferences.test.ts | 8 ++ src/__tests__/tools.test.ts | 77 +++++++++++ src/lib/geo.ts | 220 ++++++++++++++++++++++++++++++ src/lib/hooks/useChat.ts | 4 + src/lib/preferences.ts | 6 + src/lib/tools.ts | 57 ++++++-- src/setup.tsx | 17 +++ 10 files changed, 604 insertions(+), 13 deletions(-) create mode 100644 src/__tests__/geo.test.ts create mode 100644 src/lib/geo.ts diff --git a/README.md b/README.md index 626ff59..d79085b 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,8 @@ vicinae 'vicinae://launch/lam-agent/chat?fallbackText=summarise%20this' | **Ollama API Key** | None | Required. Stored by Vicinae as a password preference. | | **Model** | None | Required. Any model your key can reach. | | **Ollama Host** | `https://ollama.com` | Set to `http://localhost:11434` for a local server. | +| **Location** | None | The place Lam should treat as yours, for example `Cebu City, Philippines`. Optional. The only exact location available, and the only one that stops anything being inferred. | +| **IP Geolocation** | off | Off by default. When on, and only when **Location** is empty, your IP is sent to ipapi.co to resolve a rough city. | | **System Prompt** | Lam's default | Replaces the built-in prompt entirely. The default asks for plain prose: no em dashes, no generated-text tells, no restating the question. | | **Web Tools** | on | Enables `web_search` and `web_fetch`. | | **Reasoning Effort** | Off | `low`/`medium`/`high` for models that support thinking. | @@ -136,11 +138,19 @@ Lam calls these on its own when they would improve an answer. All four are read- | Tool | What it does | | --- | --- | | `get_datetime` | Current local date, time, weekday, timezone, and UTC offset. | -| `get_location` | Timezone, UTC offset, locale, region, and city. | +| `get_location` | The place you configured, or a rough city from your IP if you enabled that, plus timezone, UTC offset, locale and region. | | `web_search` | Searches the web; returns titles, URLs, and snippets. | | `web_fetch` | Fetches one page and returns its content as markdown. | -`get_location` is derived from your device timezone rather than GPS or an IP lookup, so no third-party geolocation service sees your address. Treat it as "which region am I in", not "where am I". +`get_location` reports the **Location** you set, if you set one, because that is the only answer here that is actually about where you are. Everything else is inferred from your device, and the inference is coarser than it looks. + +**A timezone is not a city.** This is the trap the tool used to fall into: it reported `City: Manila`, taken from the last segment of the IANA zone name. That name is the database's representative place, not yours, and a single zone is often a whole country. `Asia/Manila` is the Philippines, `Asia/Kolkata` is India, `Europe/Oslo` is Norway, `Australia/Brisbane` is Queensland. Reporting the zone's namesake as your city was wrong for anyone outside it, and confidently so, because a model asked about the weather will use it without hesitating. The zone's label is still reported, as `Timezone area`, and named for what it is. + +The **region** comes from your locale, which is a declaration rather than an inference: a locale of `en-PH` says the Philippines because that is what you set. It costs nothing and it cannot be wrong the way a zone name can. + +If you turn on **IP Geolocation**, and only then, `get_location` resolves a rough city from your IP address via [ipapi.co](https://ipapi.co). That is a third party learning approximately where you are, so it is off by default, it is skipped entirely whenever you have set a Location (the better answer is already there, and there is no reason to spend your privacy on a worse one), and the tool output names the provider and says the result is city-level rather than exact. The answer is cached for 6 hours rather than looked up per question, because a city does not change between questions and every lookup hands over another copy of your address. `Set Up Lam Agent` clears both that cache and the capability cache when you re-test. + +This extension does not use GPS. That would mean CoreLocation on macOS, which has no command-line interface, or GeoClue on Linux, whose usual provider resolves nearby Wi-Fi networks through a Mozilla service anyway. Neither is worth it for "is it late where you are". Web search results and fetched pages can be large. Lam truncates them before they reach the model, and gives up after 5 rounds of tool calls rather than looping. diff --git a/package.json b/package.json index eb67b68..4bc40cc 100644 --- a/package.json +++ b/package.json @@ -72,6 +72,15 @@ "type": "textfield", "required": false }, + { + "name": "ipGeolocation", + "title": "IP Geolocation", + "label": "Look up my city from my IP address", + "description": "Off by default. When this is on, and the Location preference above is empty, Lam sends your IP address to ipapi.co to work out your city. That is a third party learning roughly where you are, which is why it is opt-in, and why setting Location makes it unnecessary.", + "type": "checkbox", + "required": false, + "default": false + }, { "name": "webSearchEnabled", "title": "Web Tools", diff --git a/src/__tests__/geo.test.ts b/src/__tests__/geo.test.ts new file mode 100644 index 0000000..f9b1231 --- /dev/null +++ b/src/__tests__/geo.test.ts @@ -0,0 +1,205 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { __resetLocalStorage } from "../__mocks__/@vicinae/api"; +import { GEO_TTL_MS, buildPlace, forgetGeoPlace, ipPlace } from "../lib/geo"; + +/** + * The only code in the extension that sends anything about the user to a third + * party, so the parts worth pinning down are that it caches (rather than leaking + * the address on every question), that it never throws, and that a failure is + * reported instead of being served as a coarser answer. + */ + +const fetchMock = vi.fn(); + +function answer(payload: unknown, status = 200): Response { + return new Response(JSON.stringify(payload), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +const CEBU = { + city: "Cebu City", + region: "Central Visayas", + country_name: "Philippines", + latitude: 10.3167, + longitude: 123.8907, + timezone: "Asia/Manila", +}; + +beforeEach(() => { + __resetLocalStorage(); + fetchMock.mockReset(); + vi.stubGlobal("fetch", fetchMock); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("buildPlace", () => { + it("joins the parts that exist", () => { + expect(buildPlace({ city: "Cebu City", region: "Central Visayas", country: "Philippines" })).toBe( + "Cebu City, Central Visayas, Philippines", + ); + }); + + it("drops repeats, which providers return for city-states", () => { + // "Singapore, Singapore, Singapore" reads like a bug rather than a place. + expect(buildPlace({ city: "Singapore", region: "Singapore", country: "Singapore" })).toBe("Singapore"); + }); + + it("ignores repeats that differ only in case", () => { + expect(buildPlace({ city: "Manila", region: "manila" })).toBe("Manila"); + }); + + it("drops empty and whitespace parts", () => { + expect(buildPlace({ city: " ", region: "", country: "Philippines" })).toBe("Philippines"); + }); + + it("returns nothing when there is nothing", () => { + expect(buildPlace({})).toBe(""); + }); +}); + +describe("ipPlace", () => { + it("resolves a place from a provider answer", async () => { + fetchMock.mockResolvedValueOnce(answer(CEBU)); + + const result = await ipPlace({ now: 1_000 }); + + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + + expect(result.place.place).toBe("Cebu City, Central Visayas, Philippines"); + expect(result.place.latitude).toBe(10.3167); + expect(result.place.timezone).toBe("Asia/Manila"); + expect(result.place.source).toBe("ipapi.co"); + }); + + it("asks for JSON with no cache-busting that a cache would defeat", async () => { + fetchMock.mockResolvedValueOnce(answer(CEBU)); + await ipPlace({ now: 1_000 }); + + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe("https://ipapi.co/json/"); + expect(init.headers.Accept).toBe("application/json"); + }); + + it("serves a repeat from the cache instead of re-sending the address", async () => { + fetchMock.mockResolvedValueOnce(answer(CEBU)); + + await ipPlace({ now: 1_000 }); + const second = await ipPlace({ now: 2_000 }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(second.status).toBe("ok"); + }); + + it("re-fetches once the cached place is stale", async () => { + fetchMock.mockResolvedValueOnce(answer(CEBU)); + await ipPlace({ now: 1_000 }); + + fetchMock.mockResolvedValueOnce(answer({ ...CEBU, city: "Davao City" })); + const later = await ipPlace({ now: 1_000 + GEO_TTL_MS + 1 }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(later.status === "ok" && later.place.city).toBe("Davao City"); + }); + + it("treats exactly the TTL as still fresh", async () => { + fetchMock.mockResolvedValueOnce(answer(CEBU)); + await ipPlace({ now: 1_000 }); + + await ipPlace({ now: 1_000 + GEO_TTL_MS }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("ignores the cache when asked to refresh", async () => { + fetchMock.mockResolvedValueOnce(answer(CEBU)); + await ipPlace({ now: 1_000 }); + + fetchMock.mockResolvedValueOnce(answer({ ...CEBU, city: "Davao City" })); + const refreshed = await ipPlace({ now: 1_001, refresh: true }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(refreshed.status === "ok" && refreshed.place.city).toBe("Davao City"); + }); + + it("reports a rate limit by status rather than pretending to know the place", async () => { + fetchMock.mockResolvedValueOnce(new Response("slow down", { status: 429 })); + + const result = await ipPlace({ now: 1_000 }); + + expect(result).toEqual({ status: "failed", message: "ipapi.co answered HTTP 429" }); + }); + + it("reports a provider that answers with error: true", async () => { + // ipapi.co signals failure in the body while still answering 200. + fetchMock.mockResolvedValueOnce(answer({ error: true, reason: "RateLimited", message: "Too many" })); + + const result = await ipPlace({ now: 1_000 }); + + expect(result).toEqual({ status: "failed", message: "Too many" }); + }); + + it("reports a body that is not JSON", async () => { + fetchMock.mockResolvedValueOnce(new Response("nope", { status: 200 })); + + const result = await ipPlace({ now: 1_000 }); + + expect(result.status).toBe("failed"); + }); + + it("reports an answer with no usable place", async () => { + fetchMock.mockResolvedValueOnce(answer({ city: null, region: null, country_name: null })); + + const result = await ipPlace({ now: 1_000 }); + + expect(result).toEqual({ status: "failed", message: "ipapi.co returned no usable place" }); + }); + + it("survives the network being down without throwing", async () => { + fetchMock.mockRejectedValueOnce(new Error("offline")); + + const result = await ipPlace({ now: 1_000 }); + + expect(result.status).toBe("failed"); + }); + + it("does not cache a failure", async () => { + fetchMock.mockRejectedValueOnce(new Error("offline")); + await ipPlace({ now: 1_000 }); + + fetchMock.mockResolvedValueOnce(answer(CEBU)); + const second = await ipPlace({ now: 1_001 }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(second.status).toBe("ok"); + }); + + it("ignores a corrupt cache entry rather than failing", async () => { + const { LocalStorage } = await import("../__mocks__/@vicinae/api"); + await LocalStorage.setItem("lam.geo", "{not json"); + + fetchMock.mockResolvedValueOnce(answer(CEBU)); + + await expect(ipPlace({ now: 1_000 })).resolves.toMatchObject({ status: "ok" }); + }); +}); + +describe("forgetGeoPlace", () => { + it("drops the cached place so a move is not pinned for hours", async () => { + fetchMock.mockResolvedValueOnce(answer(CEBU)); + await ipPlace({ now: 1_000 }); + + await forgetGeoPlace(); + + fetchMock.mockResolvedValueOnce(answer({ ...CEBU, city: "Davao City" })); + const after = await ipPlace({ now: 1_001 }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(after.status === "ok" && after.place.city).toBe("Davao City"); + }); +}); diff --git a/src/__tests__/preferences.test.ts b/src/__tests__/preferences.test.ts index 25f703b..ab83d66 100644 --- a/src/__tests__/preferences.test.ts +++ b/src/__tests__/preferences.test.ts @@ -57,6 +57,14 @@ describe("resolvePreferences", () => { expect(resolved.place).toBe(""); }); + it("leaves IP geolocation off unless it was switched on", () => { + // The one setting that sends something about the user to a third party is + // opted into, never defaulted into. + expect(resolvePreferences({}).ipGeolocation).toBe(false); + expect(resolvePreferences({ ipGeolocation: true }).ipGeolocation).toBe(true); + expect(resolvePreferences({ ipGeolocation: false }).ipGeolocation).toBe(false); + }); + it("keeps the configured location, trimmed", () => { expect(resolvePreferences({ location: " Cebu City, Philippines " }).place).toBe( "Cebu City, Philippines", diff --git a/src/__tests__/tools.test.ts b/src/__tests__/tools.test.ts index ffd806f..4aeb59b 100644 --- a/src/__tests__/tools.test.ts +++ b/src/__tests__/tools.test.ts @@ -264,6 +264,83 @@ describe("local tools", () => { }); }); +describe("get_location with IP geolocation", () => { + const resolved = { + status: "ok" as const, + place: { + place: "Cebu City, Central Visayas, Philippines", + city: "Cebu City", + region: "Central Visayas", + country: "Philippines", + timezone: "Asia/Manila", + source: "ipapi.co", + fetchedAt: 1_000, + }, + }; + + it("reports the resolved place and says how approximate it is", async () => { + const ipPlace = vi.fn(async () => resolved); + + const result = await executeTool("get_location", {}, { ...context, ipPlace }); + + expect(result.isError).toBe(false); + expect(result.content).toContain("Place: Cebu City, Central Visayas, Philippines"); + expect(result.content).toContain("city-level rather than exact"); + expect(result.content).toContain("ipapi.co"); + }); + + it("does not look the address up at all when the user has said where they are", async () => { + // The preference is both more accurate and free, so sending the IP anyway + // would be a privacy cost for nothing. + const ipPlace = vi.fn(async () => resolved); + + const result = await executeTool("get_location", {}, { + ...context, + place: "Cebu City, Philippines", + ipPlace, + }); + + expect(ipPlace).not.toHaveBeenCalled(); + expect(result.content).toContain("Place: Cebu City, Philippines"); + expect(result.content).not.toContain("ipapi.co"); + }); + + it("surfaces a failed lookup instead of quietly serving the region answer", async () => { + const ipPlace = vi.fn(async () => ({ status: "failed" as const, message: "ipapi.co answered HTTP 429" })); + + const result = await executeTool("get_location", {}, { ...context, ipPlace }); + + expect(result.content).toContain("IP geolocation is switched on but did not answer"); + expect(result.content).toContain("HTTP 429"); + // The region answer is still there, so the tool stays useful. + expect(result.content).toContain("Timezone:"); + }); + + it("flags an IP timezone that disagrees with the clock, which usually means a VPN", async () => { + const ipPlace = vi.fn(async () => ({ + status: "ok" as const, + place: { ...resolved.place, timezone: "America/New_York" }, + })); + + const result = await executeTool("get_location", {}, { ...context, ipPlace }); + + expect(result.content).toContain("differs from the device timezone"); + }); + + it("says nothing about a mismatch when the timezones agree", async () => { + const ipPlace = vi.fn(async () => resolved); + const result = await executeTool("get_location", {}, { ...context, ipPlace }); + + // The test host's own zone varies, so this asserts the absence of the note + // only for the case where the provider echoed the same value. + if (result.content.includes("IP-reported timezone")) { + expect(result.content).toContain("differs"); + } else { + expect(result.content).not.toContain("IP-reported timezone"); + } + }); +}); + describe("dispatch", () => { it("reports an unknown tool instead of throwing", async () => { const result = await executeTool("delete_everything", {}, context); diff --git a/src/lib/geo.ts b/src/lib/geo.ts new file mode 100644 index 0000000..b8df71d --- /dev/null +++ b/src/lib/geo.ts @@ -0,0 +1,220 @@ +import { LocalStorage } from "@vicinae/api"; +import { z } from "zod"; +import { errorMessage } from "./utils"; + +/** + * IP geolocation, opt-in. + * + * This is the one place in the extension that tells a third party anything about + * the user. The README said no geolocation service ever saw the user's address, + * and this is what makes that conditional, so it is off unless asked for, it is + * skipped entirely when the user has already said where they are, and the tool + * output names the provider so the model can qualify what it repeats. + * + * The answer is cached for hours rather than looked up per call. A city does not + * change between questions, and every lookup hands the provider another copy of + * the user's IP address. + * + * Imports Vicinae for `LocalStorage` only, which is the same footing as + * `sessions.ts` and `vision.ts`. Everything else about it is plain `fetch`, so + * the parsing and the cache are unit tested without a runtime. + */ + +/** Where the lookup goes. Swapping providers means changing this and the parser below. */ +export const GEO_PROVIDER = "https://ipapi.co/json/"; +export const GEO_PROVIDER_NAME = "ipapi.co"; + +/** How long a resolved place is trusted. Long enough to not re-leak the IP on every question. */ +export const GEO_TTL_MS = 6 * 60 * 60 * 1000; + +/** An IP lookup is a nice-to-have, so it must never hold up an answer. */ +const GEO_TIMEOUT_MS = 8_000; + +const STORAGE_KEY = "lam.geo"; + +export interface GeoPlace { + /** Ready to show, e.g. "Cebu City, Central Visayas, Philippines". */ + place: string; + city?: string; + region?: string; + country?: string; + latitude?: number; + longitude?: number; + timezone?: string; + /** Named in the tool output, so the model can say where this came from. */ + source: string; + fetchedAt: number; +} + +export type GeoLookup = { status: "ok"; place: GeoPlace } | { status: "failed"; message: string }; + +export interface GeoOptions { + signal?: AbortSignal; + /** Injected so the cache is deterministic under test. */ + now?: number; + /** Ignores a cached answer. */ + refresh?: boolean; +} + +const CachedSchema = z.object({ + place: z.string(), + city: z.string().optional(), + region: z.string().optional(), + country: z.string().optional(), + latitude: z.number().optional(), + longitude: z.number().optional(), + timezone: z.string().optional(), + source: z.string(), + fetchedAt: z.number(), +}); + +/** The subset of ipapi.co's response this needs, with every field optional so a shape change degrades instead of throwing. */ +const ResponseSchema = z.object({ + error: z.unknown().optional(), + reason: z.string().optional(), + message: z.string().optional(), + city: z.string().nullable().optional(), + region: z.string().nullable().optional(), + country_name: z.string().nullable().optional(), + latitude: z.number().nullable().optional(), + longitude: z.number().nullable().optional(), + timezone: z.string().nullable().optional(), +}); + +/** + * Joins the parts of a place, dropping duplicates. + * + * Providers often return the city twice (once as `city`, once as `region`) for + * city-states and single-city regions, and "Singapore, Singapore, Singapore" + * reads like a bug. + */ +export function buildPlace(parts: { city?: string; region?: string; country?: string }): string { + const seen = new Set(); + const kept: string[] = []; + + for (const part of [parts.city, parts.region, parts.country]) { + const trimmed = part?.trim(); + const key = trimmed?.toLowerCase(); + if (!trimmed || !key || seen.has(key)) continue; + + seen.add(key); + kept.push(trimmed); + } + + return kept.join(", "); +} + +/** Aborts on either the caller's signal or a timeout, whichever comes first. */ +async function fetchWithTimeout( + url: string, + signal: AbortSignal | undefined, + timeoutMs: number, +): Promise { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + const forward = () => controller.abort(); + + signal?.addEventListener("abort", forward, { once: true }); + + try { + return await fetch(url, { signal: controller.signal, headers: { Accept: "application/json" } }); + } finally { + clearTimeout(timer); + signal?.removeEventListener("abort", forward); + } +} + +async function readCache(now: number): Promise { + try { + const raw = await LocalStorage.getItem(STORAGE_KEY); + if (!raw) return undefined; + + const parsed = CachedSchema.safeParse(JSON.parse(raw)); + if (!parsed.success) return undefined; + if (now - parsed.data.fetchedAt > GEO_TTL_MS) return undefined; + + return parsed.data; + } catch { + return undefined; + } +} + +async function writeCache(place: GeoPlace): Promise { + await LocalStorage.setItem(STORAGE_KEY, JSON.stringify(place)).catch(() => undefined); +} + +/** + * Resolves the user's rough place from their IP address. + * + * Never throws: a lookup failure leaves the region-level answer intact, and the + * message is passed on so the model can say what went wrong rather than quietly + * presenting a coarser answer as though it were the same thing. + */ +export async function ipPlace(options: GeoOptions = {}): Promise { + const now = options.now ?? Date.now(); + + if (!options.refresh) { + const cached = await readCache(now); + if (cached) return { status: "ok", place: cached }; + } + + let response: Response; + try { + response = await fetchWithTimeout(GEO_PROVIDER, options.signal, GEO_TIMEOUT_MS); + } catch (thrown) { + return { status: "failed", message: `could not reach ${GEO_PROVIDER_NAME} (${errorMessage(thrown)})` }; + } + + if (!response.ok) { + return { status: "failed", message: `${GEO_PROVIDER_NAME} answered HTTP ${response.status}` }; + } + + let payload: unknown; + try { + payload = await response.json(); + } catch { + return { status: "failed", message: `${GEO_PROVIDER_NAME} did not answer with JSON` }; + } + + const parsed = ResponseSchema.safeParse(payload); + if (!parsed.success) { + return { status: "failed", message: `${GEO_PROVIDER_NAME} answered in an unexpected shape` }; + } + + const data = parsed.data; + if (data.error) { + return { + status: "failed", + message: data.message ?? data.reason ?? `${GEO_PROVIDER_NAME} refused the lookup`, + }; + } + + const city = data.city ?? undefined; + const region = data.region ?? undefined; + const country = data.country_name ?? undefined; + const place = buildPlace({ city, region, country }); + + if (!place) { + return { status: "failed", message: `${GEO_PROVIDER_NAME} returned no usable place` }; + } + + const resolved: GeoPlace = { + place, + city, + region, + country, + latitude: data.latitude ?? undefined, + longitude: data.longitude ?? undefined, + timezone: data.timezone ?? undefined, + source: GEO_PROVIDER_NAME, + fetchedAt: now, + }; + + await writeCache(resolved); + return { status: "ok", place: resolved }; +} + +/** Drops the cached place, so a move or a provider change is not pinned for hours. */ +export async function forgetGeoPlace(): Promise { + await LocalStorage.removeItem(STORAGE_KEY).catch(() => undefined); +} diff --git a/src/lib/hooks/useChat.ts b/src/lib/hooks/useChat.ts index dec8604..0722d95 100644 --- a/src/lib/hooks/useChat.ts +++ b/src/lib/hooks/useChat.ts @@ -1,6 +1,7 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { runAgent } from "../agent"; import { toStoredAttachment } from "../attachments"; +import { ipPlace } from "../geo"; import { buildMessages, composeSystemPrompt } from "../history"; import { OllamaError } from "../ollama"; import type { ResolvedPreferences } from "../preferences"; @@ -178,6 +179,9 @@ export function useChat(options: UseChatOptions): UseChat { host: preferences.host, apiKey: preferences.apiKey, place: preferences.place, + // Only wired up when the user switched it on, so an off setting + // cannot reach the network for a location at all. + ipPlace: preferences.ipGeolocation ? ipPlace : undefined, }, signal: controller.signal, events: { diff --git a/src/lib/preferences.ts b/src/lib/preferences.ts index 8e7740d..2ec2c99 100644 --- a/src/lib/preferences.ts +++ b/src/lib/preferences.ts @@ -16,6 +16,7 @@ export interface LamPreferences { ollamaHost?: string; systemPrompt?: string; location?: string; + ipGeolocation?: boolean; webSearchEnabled?: boolean; thinking?: string; showToolCalls?: boolean; @@ -37,6 +38,8 @@ export interface ResolvedPreferences { * city, and a timezone name is not a city at all. */ place: string; + /** Whether to resolve a rough place from the IP address. Off unless asked for. */ + ipGeolocation: boolean; webEnabled: boolean; /** Undefined when reasoning is off, which is what the API client expects. */ think?: string; @@ -88,6 +91,9 @@ export function resolvePreferences(raw: LamPreferences): ResolvedPreferences { host: (raw.ollamaHost ?? "").trim() || DEFAULT_HOST, systemPrompt: (raw.systemPrompt ?? "").trim(), place: (raw.location ?? "").trim(), + // Defaults to off: this is the one setting that sends something about the + // user to a third party, so it is a deliberate choice rather than a default. + ipGeolocation: raw.ipGeolocation ?? false, // Defaults to on: the preference is a checkbox the user has to actively untick. webEnabled: raw.webSearchEnabled ?? true, think: thinking && thinking !== "off" ? thinking : undefined, diff --git a/src/lib/tools.ts b/src/lib/tools.ts index 9917cae..f76f626 100644 --- a/src/lib/tools.ts +++ b/src/lib/tools.ts @@ -1,5 +1,6 @@ import { formatNow, getLocationContext, weekdayName } from "./location"; import { webFetch, webSearch } from "./ollama"; +import type { GeoLookup } from "./geo"; import type { ToolDefinition } from "./types"; import { errorMessage } from "./utils"; @@ -26,6 +27,14 @@ export interface ToolContext { * Vicinae imports and preferences arrive through that API. */ place?: string; + /** + * Resolves a rough place from the user's IP address, when that is switched on. + * + * Injected for the same reason as `place`, and absent when IP geolocation is + * off, which is what keeps this module from being able to reach the network + * for a location on its own. + */ + ipPlace?: (options: { signal?: AbortSignal }) => Promise; } export interface ToolResult { @@ -147,7 +156,7 @@ export async function executeTool( return { content: describeNow(context.now), isError: false }; case "get_location": - return { content: describeLocation(context.now, context.place), isError: false }; + return { content: await describeLocation(context), isError: false }; case "web_search": return await runWebSearch(args, context); @@ -179,14 +188,22 @@ function describeNow(now?: Date): string { ].join("\n"); } -function describeLocation(now?: Date, place?: string): string { - const date = now ?? new Date(); - const location = getLocationContext(date, undefined, place); +async function describeLocation(context: ToolContext): Promise { + const date = context.now ?? new Date(); + const configured = context.place?.trim() || undefined; + + // A place the user typed wins outright, and while it is set there is no reason + // to hand their IP address to anyone. + const lookup = + configured || !context.ipPlace ? undefined : await context.ipPlace({ signal: context.signal }); + const ip = lookup?.status === "ok" ? lookup.place : undefined; + + const location = getLocationContext(date, undefined, configured ?? ip?.place); const lines: string[] = []; - // The configured place comes first because it is the only exact thing here, - // and the model should reach for it before anything else. + // The place comes first because it is the only answer that is about where the + // user actually is, rather than about which zone the clock is set to. if (location.place) lines.push(`Place: ${location.place}`); lines.push( @@ -202,11 +219,29 @@ function describeLocation(now?: Date, place?: string): string { lines.push(`Timezone area: ${location.timezoneArea}`); } - lines.push( - location.place - ? "Note: Place is the user's own setting, so prefer it for anything that depends on exactly where they are." - : "Note: no Location is set in preferences. Everything above is derived from the device timezone and locale, so it is accurate to a region and not to a city. Ask which city they are in when an exact place matters.", - ); + // A mismatch between the clock and the IP is worth surfacing: it usually means + // a VPN or a device still set to somewhere the user has left. + if (ip?.timezone && ip.timezone !== location.timezone) { + lines.push(`IP-reported timezone: ${ip.timezone}, which differs from the device timezone`); + } + + if (configured) { + lines.push( + "Note: Place is the user's own setting, so prefer it for anything that depends on exactly where they are.", + ); + } else if (ip) { + lines.push( + `Note: Place came from an IP address lookup via ${ip.source}, so it is city-level rather than exact, and it can be out of date if the user has moved since.`, + ); + } else { + lines.push( + "Note: no Location is set in preferences. Everything above is derived from the device timezone and locale, so it is accurate to a region and not to a city. Ask which city they are in when an exact place matters.", + ); + } + + if (lookup?.status === "failed") { + lines.push(`IP geolocation is switched on but did not answer: ${lookup.message}.`); + } if (location.timezoneArea) { lines.push("The timezone area is the timezone's own name, not the user's city."); diff --git a/src/setup.tsx b/src/setup.tsx index 6a8d3d6..aae6273 100644 --- a/src/setup.tsx +++ b/src/setup.tsx @@ -2,6 +2,7 @@ import { Action, ActionPanel, Icon, List, openExtensionPreferences } from "@vici import { useCallback, useEffect, useMemo, useState } from "react"; import { describeResumeTimeout, getLamPreferences, needsSetup } from "./lib/preferences"; import { listModels } from "./lib/ollama"; +import { forgetGeoPlace } from "./lib/geo"; import { forgetVisionSupport, visionSupport, type VisionSupport } from "./lib/vision"; import type { ModelInfo } from "./lib/types"; import { errorMessage } from "./lib/utils"; @@ -60,6 +61,10 @@ export default function SetupCommand() { // re-pull, a retagged build), so re-testing drops the cached answer that the // chat view gates the snip action on rather than leaving a stale "no". await forgetVisionSupport(); + + // Same reasoning for the IP-derived place: a re-test is the moment to stop + // trusting an answer that was resolved hours ago, possibly in another city. + await forgetGeoPlace(); setVision( await visionSupport({ host: preferences.host, @@ -176,6 +181,18 @@ export default function SetupCommand() { } actions={settingsActions} /> + {error ? ( -- 2.54.0 From af325b4a6201b9f3db4d300bd87452fa9189e773 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:59:09 +0800 Subject: [PATCH 10/11] fix: ipapi.co stopped answering, so the lookup never resolved Turning IP Geolocation on changed nothing: every lookup failed, and the tool output still said "no Location is set in preferences", which is a different problem and sent the user looking in the wrong place. ipapi.co no longer serves an unauthenticated caller. Verified directly: no User-Agent -> HTTP 429 {"reason":"RateLimited", "message":"Please sign up for a paid plan"} browser UA -> HTTP 403, a Cloudflare "Just a moment..." challenge So the provider is now ipwho.is, which answers 200 without a key. It signals refusal with success: false on an HTTP 200, so the body is checked as well as the status, and it nests the timezone under an object, which the parser now accepts alongside a bare string. A named User-Agent is sent: node's default is bare, and "who is asking" is the one thing a geolocation provider has a fair interest in. The failure branch is also fixed. A failed lookup and an unset preference are different problems, and the wording no longer conflates them. The note tells the model to pass the failure on, because the model is the only channel that reaches the user and it had been paraphrasing the failure away. Accuracy is documented honestly rather than as "city-level". An IP locates the ISP's egress, not the person: measured from one machine, an IPv4 query resolved to Cebu City and an IPv6 query moments later to Talisay in Calabarzon, about 500km apart. That is now in the README, the preference description and the tool output, and it is the argument for setting Location instead. Verified live with the exact request and field access the extension uses: HTTP 200, "Talisay, Calabarzon (Region IV-A), Philippines", Asia/Manila. 344 tests, tsc, eslint, prettier, and vici build. --- README.md | 10 +++++++-- package.json | 2 +- src/__tests__/geo.test.ts | 37 +++++++++++++++++++++---------- src/__tests__/tools.test.ts | 32 ++++++++++++++++++++++----- src/lib/geo.ts | 44 +++++++++++++++++++++++++++---------- src/lib/tools.ts | 14 +++++++----- src/setup.tsx | 2 +- 7 files changed, 102 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index d79085b..4016947 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,7 @@ vicinae 'vicinae://launch/lam-agent/chat?fallbackText=summarise%20this' | **Model** | None | Required. Any model your key can reach. | | **Ollama Host** | `https://ollama.com` | Set to `http://localhost:11434` for a local server. | | **Location** | None | The place Lam should treat as yours, for example `Cebu City, Philippines`. Optional. The only exact location available, and the only one that stops anything being inferred. | -| **IP Geolocation** | off | Off by default. When on, and only when **Location** is empty, your IP is sent to ipapi.co to resolve a rough city. | +| **IP Geolocation** | off | Off by default. When on, and only when **Location** is empty, your IP is sent to ipwho.is to resolve a rough area. | | **System Prompt** | Lam's default | Replaces the built-in prompt entirely. The default asks for plain prose: no em dashes, no generated-text tells, no restating the question. | | **Web Tools** | on | Enables `web_search` and `web_fetch`. | | **Reasoning Effort** | Off | `low`/`medium`/`high` for models that support thinking. | @@ -148,10 +148,16 @@ Lam calls these on its own when they would improve an answer. All four are read- The **region** comes from your locale, which is a declaration rather than an inference: a locale of `en-PH` says the Philippines because that is what you set. It costs nothing and it cannot be wrong the way a zone name can. -If you turn on **IP Geolocation**, and only then, `get_location` resolves a rough city from your IP address via [ipapi.co](https://ipapi.co). That is a third party learning approximately where you are, so it is off by default, it is skipped entirely whenever you have set a Location (the better answer is already there, and there is no reason to spend your privacy on a worse one), and the tool output names the provider and says the result is city-level rather than exact. The answer is cached for 6 hours rather than looked up per question, because a city does not change between questions and every lookup hands over another copy of your address. `Set Up Lam Agent` clears both that cache and the capability cache when you re-test. +If you turn on **IP Geolocation**, and only then, `get_location` resolves a rough area from your IP address via [ipwho.is](https://ipwho.is). That is a third party learning approximately where you are, so it is off by default, it is skipped entirely whenever you have set a Location (the better answer is already there, and there is no reason to spend your privacy on a worse one), and the tool output names the provider and calls the result a rough guess rather than a city. + +**It is coarser than "city-level" suggests, and that is worth knowing before trusting it.** An IP address identifies the point where the traffic left the ISP, not the person. Measured from one machine on Globe Telecom, an IPv4 lookup resolved to Cebu City and an IPv6 lookup moments later resolved to Talisay in Calabarzon, roughly 500km apart. If you want a place you can rely on, set **Location** and turn this off. + +The answer is cached for 6 hours rather than looked up per question, because an area does not change between questions and every lookup hands over another copy of your address. `Set Up Lam Agent` clears that cache, and the capability cache, when you re-test. This extension does not use GPS. That would mean CoreLocation on macOS, which has no command-line interface, or GeoClue on Linux, whose usual provider resolves nearby Wi-Fi networks through a Mozilla service anyway. Neither is worth it for "is it late where you are". +**The provider has to be one that answers an unauthenticated caller.** The first choice was ipapi.co, which no longer does: a request with no User-Agent now returns `HTTP 429` with `"Please sign up for a paid plan"`, and adding a browser User-Agent gets an `HTTP 403` Cloudflare bot challenge instead. ipwho.is answers without a key and signals failures with `success: false` on an `HTTP 200`, so the body is checked as well as the status. `GEO_PROVIDER` is a single constant, and swapping it means changing the parser beside it. + Web search results and fetched pages can be large. Lam truncates them before they reach the model, and gives up after 5 rounds of tool calls rather than looping. ## Architecture diff --git a/package.json b/package.json index 4bc40cc..d0c56ef 100644 --- a/package.json +++ b/package.json @@ -76,7 +76,7 @@ "name": "ipGeolocation", "title": "IP Geolocation", "label": "Look up my city from my IP address", - "description": "Off by default. When this is on, and the Location preference above is empty, Lam sends your IP address to ipapi.co to work out your city. That is a third party learning roughly where you are, which is why it is opt-in, and why setting Location makes it unnecessary.", + "description": "Off by default. When this is on, and the Location preference above is empty, Lam sends your IP address to ipwho.is to work out roughly where you are. That is a third party learning approximately where you are, which is why it is opt-in, and why setting Location makes it unnecessary. It is coarser than a city: an IP address locates the point where traffic left your ISP, which can be hundreds of kilometres away.", "type": "checkbox", "required": false, "default": false diff --git a/src/__tests__/geo.test.ts b/src/__tests__/geo.test.ts index f9b1231..abc86ab 100644 --- a/src/__tests__/geo.test.ts +++ b/src/__tests__/geo.test.ts @@ -18,13 +18,15 @@ function answer(payload: unknown, status = 200): Response { }); } +/** A real ipwho.is payload, trimmed. Note the nested timezone and `country`. */ const CEBU = { + success: true, city: "Cebu City", region: "Central Visayas", - country_name: "Philippines", + country: "Philippines", latitude: 10.3167, longitude: 123.8907, - timezone: "Asia/Manila", + timezone: { id: "Asia/Manila", utc: "+08:00" }, }; beforeEach(() => { @@ -74,16 +76,26 @@ describe("ipPlace", () => { expect(result.place.place).toBe("Cebu City, Central Visayas, Philippines"); expect(result.place.latitude).toBe(10.3167); expect(result.place.timezone).toBe("Asia/Manila"); - expect(result.place.source).toBe("ipapi.co"); + expect(result.place.source).toBe("ipwho.is"); }); - it("asks for JSON with no cache-busting that a cache would defeat", async () => { + it("names itself to the provider rather than arriving anonymously", async () => { fetchMock.mockResolvedValueOnce(answer(CEBU)); await ipPlace({ now: 1_000 }); const [url, init] = fetchMock.mock.calls[0]; - expect(url).toBe("https://ipapi.co/json/"); + expect(url).toBe("https://ipwho.is/"); expect(init.headers.Accept).toBe("application/json"); + expect(init.headers["User-Agent"]).toContain("lam-agent"); + }); + + it("reads a timezone handed back as a plain string too", async () => { + // Providers differ here: ipwho.is nests it, others return a bare zone name. + fetchMock.mockResolvedValueOnce(answer({ ...CEBU, timezone: "Asia/Manila" })); + + const result = await ipPlace({ now: 1_000 }); + + expect(result.status === "ok" && result.place.timezone).toBe("Asia/Manila"); }); it("serves a repeat from the cache instead of re-sending the address", async () => { @@ -132,16 +144,17 @@ describe("ipPlace", () => { const result = await ipPlace({ now: 1_000 }); - expect(result).toEqual({ status: "failed", message: "ipapi.co answered HTTP 429" }); + expect(result).toEqual({ status: "failed", message: "ipwho.is answered HTTP 429" }); }); - it("reports a provider that answers with error: true", async () => { - // ipapi.co signals failure in the body while still answering 200. - fetchMock.mockResolvedValueOnce(answer({ error: true, reason: "RateLimited", message: "Too many" })); + it("reports a provider that refuses in the body while answering 200", async () => { + // ipwho.is signals failure with success: false rather than a status code, + // so checking the status alone would read a refusal as a success. + fetchMock.mockResolvedValueOnce(answer({ success: false, message: "Reserved range" })); const result = await ipPlace({ now: 1_000 }); - expect(result).toEqual({ status: "failed", message: "Too many" }); + expect(result).toEqual({ status: "failed", message: "Reserved range" }); }); it("reports a body that is not JSON", async () => { @@ -153,11 +166,11 @@ describe("ipPlace", () => { }); it("reports an answer with no usable place", async () => { - fetchMock.mockResolvedValueOnce(answer({ city: null, region: null, country_name: null })); + fetchMock.mockResolvedValueOnce(answer({ success: true, city: null, region: null, country: null })); const result = await ipPlace({ now: 1_000 }); - expect(result).toEqual({ status: "failed", message: "ipapi.co returned no usable place" }); + expect(result).toEqual({ status: "failed", message: "ipwho.is returned no usable place" }); }); it("survives the network being down without throwing", async () => { diff --git a/src/__tests__/tools.test.ts b/src/__tests__/tools.test.ts index 4aeb59b..2451d81 100644 --- a/src/__tests__/tools.test.ts +++ b/src/__tests__/tools.test.ts @@ -273,7 +273,7 @@ describe("get_location with IP geolocation", () => { region: "Central Visayas", country: "Philippines", timezone: "Asia/Manila", - source: "ipapi.co", + source: "ipwho.is", fetchedAt: 1_000, }, }; @@ -285,8 +285,8 @@ describe("get_location with IP geolocation", () => { expect(result.isError).toBe(false); expect(result.content).toContain("Place: Cebu City, Central Visayas, Philippines"); - expect(result.content).toContain("city-level rather than exact"); - expect(result.content).toContain("ipapi.co"); + expect(result.content).toContain("rough guess"); + expect(result.content).toContain("ipwho.is"); }); it("does not look the address up at all when the user has said where they are", async () => { @@ -302,20 +302,40 @@ describe("get_location with IP geolocation", () => { expect(ipPlace).not.toHaveBeenCalled(); expect(result.content).toContain("Place: Cebu City, Philippines"); - expect(result.content).not.toContain("ipapi.co"); + expect(result.content).not.toContain("ipwho.is"); }); it("surfaces a failed lookup instead of quietly serving the region answer", async () => { - const ipPlace = vi.fn(async () => ({ status: "failed" as const, message: "ipapi.co answered HTTP 429" })); + const ipPlace = vi.fn(async () => ({ status: "failed" as const, message: "ipwho.is answered HTTP 429" })); const result = await executeTool("get_location", {}, { ...context, ipPlace }); - expect(result.content).toContain("IP geolocation is switched on but did not answer"); + expect(result.content).toContain("IP geolocation is switched on but the lookup failed"); expect(result.content).toContain("HTTP 429"); // The region answer is still there, so the tool stays useful. expect(result.content).toContain("Timezone:"); }); + it("does not blame an unset preference when the lookup itself failed", async () => { + // These are different problems and send the user to different places. The + // original wording said "no Location is set in preferences" for both, which + // is exactly what made a broken lookup look like a missing setting. + const ipPlace = vi.fn(async () => ({ status: "failed" as const, message: "ipwho.is answered HTTP 429" })); + + const result = await executeTool("get_location", {}, { ...context, ipPlace }); + + expect(result.content).not.toContain("no Location is set in preferences"); + }); + + it("tells the model to pass a failed lookup on, since it is the only channel to the user", async () => { + const ipPlace = vi.fn(async () => ({ status: "failed" as const, message: "offline" })); + + const result = await executeTool("get_location", {}, { ...context, ipPlace }); + + expect(result.content).toContain("Tell the user"); + expect(result.content).toContain("Setting Location in preferences avoids the lookup"); + }); + it("flags an IP timezone that disagrees with the clock, which usually means a VPN", async () => { const ipPlace = vi.fn(async () => ({ status: "ok" as const, diff --git a/src/lib/geo.ts b/src/lib/geo.ts index b8df71d..ca7cb47 100644 --- a/src/lib/geo.ts +++ b/src/lib/geo.ts @@ -11,6 +11,13 @@ import { errorMessage } from "./utils"; * skipped entirely when the user has already said where they are, and the tool * output names the provider so the model can qualify what it repeats. * + * How accurate is it? Less than the name suggests, and the honest answer is that + * an IP address locates the point where the traffic left the ISP, not the person. + * Measured from one machine on Globe Telecom, an IPv4 query resolved to Cebu City + * and an IPv6 query, moments later, to Talisay in Calabarzon: about 500km apart. + * The cache is therefore not just a privacy measure, and the tool output calls + * the result a rough guess rather than a city. + * * The answer is cached for hours rather than looked up per call. A city does not * change between questions, and every lookup hands the provider another copy of * the user's IP address. @@ -21,8 +28,8 @@ import { errorMessage } from "./utils"; */ /** Where the lookup goes. Swapping providers means changing this and the parser below. */ -export const GEO_PROVIDER = "https://ipapi.co/json/"; -export const GEO_PROVIDER_NAME = "ipapi.co"; +export const GEO_PROVIDER = "https://ipwho.is/"; +export const GEO_PROVIDER_NAME = "ipwho.is"; /** How long a resolved place is trusted. Long enough to not re-leak the IP on every question. */ export const GEO_TTL_MS = 6 * 60 * 60 * 1000; @@ -68,17 +75,20 @@ const CachedSchema = z.object({ fetchedAt: z.number(), }); -/** The subset of ipapi.co's response this needs, with every field optional so a shape change degrades instead of throwing. */ +/** The subset of the provider's response this needs, with every field optional so a shape change degrades instead of throwing. */ const ResponseSchema = z.object({ - error: z.unknown().optional(), - reason: z.string().optional(), + success: z.boolean().optional(), message: z.string().optional(), city: z.string().nullable().optional(), region: z.string().nullable().optional(), - country_name: z.string().nullable().optional(), + country: z.string().nullable().optional(), latitude: z.number().nullable().optional(), longitude: z.number().nullable().optional(), - timezone: z.string().nullable().optional(), + // ipwho.is nests the timezone; some providers hand back a plain string. + timezone: z + .union([z.string(), z.object({ id: z.string().optional() })]) + .nullable() + .optional(), }); /** @@ -117,7 +127,15 @@ async function fetchWithTimeout( signal?.addEventListener("abort", forward, { once: true }); try { - return await fetch(url, { signal: controller.signal, headers: { Accept: "application/json" } }); + return await fetch(url, { + signal: controller.signal, + headers: { + Accept: "application/json", + // Named rather than anonymous. Node's default UA is bare, and "who is + // asking" is the one thing a geolocation provider has a fair interest in. + "User-Agent": "lam-agent-vicinae", + }, + }); } finally { clearTimeout(timer); signal?.removeEventListener("abort", forward); @@ -182,22 +200,24 @@ export async function ipPlace(options: GeoOptions = {}): Promise { } const data = parsed.data; - if (data.error) { + if (data.success === false) { return { status: "failed", - message: data.message ?? data.reason ?? `${GEO_PROVIDER_NAME} refused the lookup`, + message: data.message ?? `${GEO_PROVIDER_NAME} refused the lookup`, }; } const city = data.city ?? undefined; const region = data.region ?? undefined; - const country = data.country_name ?? undefined; + const country = data.country ?? undefined; const place = buildPlace({ city, region, country }); if (!place) { return { status: "failed", message: `${GEO_PROVIDER_NAME} returned no usable place` }; } + const timezone = typeof data.timezone === "string" ? data.timezone : data.timezone?.id; + const resolved: GeoPlace = { place, city, @@ -205,7 +225,7 @@ export async function ipPlace(options: GeoOptions = {}): Promise { country, latitude: data.latitude ?? undefined, longitude: data.longitude ?? undefined, - timezone: data.timezone ?? undefined, + timezone: timezone ?? undefined, source: GEO_PROVIDER_NAME, fetchedAt: now, }; diff --git a/src/lib/tools.ts b/src/lib/tools.ts index f76f626..40836df 100644 --- a/src/lib/tools.ts +++ b/src/lib/tools.ts @@ -231,7 +231,15 @@ async function describeLocation(context: ToolContext): Promise { ); } else if (ip) { lines.push( - `Note: Place came from an IP address lookup via ${ip.source}, so it is city-level rather than exact, and it can be out of date if the user has moved since.`, + `Note: Place came from an IP address lookup via ${ip.source}, so treat it as a rough guess rather than the user's city. An IP address locates where the request left the ISP's network, which can be hundreds of kilometres away, and it can be out of date if they have moved since.`, + ); + } else if (lookup?.status === "failed") { + // Deliberately not the "no Location is set" wording below. A failed lookup + // and an unset preference are different problems, and reporting the wrong + // one sends the user looking in the wrong place. This is also the only + // channel that reaches them: the model paraphrases, so it is told to say so. + lines.push( + `Note: IP geolocation is switched on but the lookup failed (${lookup.message}), so no place could be resolved. Tell the user this plainly. Setting Location in preferences avoids the lookup and the network entirely. Everything below is derived from the device timezone and locale, so it is region-level.`, ); } else { lines.push( @@ -239,10 +247,6 @@ async function describeLocation(context: ToolContext): Promise { ); } - if (lookup?.status === "failed") { - lines.push(`IP geolocation is switched on but did not answer: ${lookup.message}.`); - } - if (location.timezoneArea) { lines.push("The timezone area is the timezone's own name, not the user's city."); } diff --git a/src/setup.tsx b/src/setup.tsx index aae6273..5443ca2 100644 --- a/src/setup.tsx +++ b/src/setup.tsx @@ -188,7 +188,7 @@ export default function SetupCommand() { preferences.place ? `${preferences.place} (from preferences)` : preferences.ipGeolocation - ? "Rough city, resolved from the IP address via ipapi.co" + ? "Rough area, resolved from the IP address via ipwho.is" : "Region only, from the timezone and locale" } actions={settingsActions} -- 2.54.0 From 1ae44adc7b2eb7b4e692a897a46abdb278eb22e4 Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 16:08:26 +0800 Subject: [PATCH 11/11] fix: end a spent tool budget in an answer, not a notice about the loop Asked for the fish in every Valheim biome, the turn researched for five rounds and then displayed "Stopped after 5 rounds of tool calls without a final answer." Three things compounded: - MAX_TOOL_ITERATIONS of 5 is too few for a question that needs several searches and a few pages. It is a safety net against looping, not a budget anyone chose. - The loop discarded everything. `content` was scoped to the iteration, so every preamble the model wrote was thrown away and the turn ended with an app-generated message about its own control flow. - The model was never told it was running out, so it kept searching until the hard stop instead of answering with what it had. The budget is now spent on a closing round with the tools taken away and an instruction to answer from what was gathered, naming anything it could not confirm. So the worst case is a partial answer that says what is missing. If the closing round produces nothing, the last preamble is used, and only then the notice. The cap is 8 because the extra round is not more searching, it is the answer. Verified: 349 tests, tsc, eslint, prettier, and vici build. --- README.md | 6 ++- src/__tests__/agent.test.ts | 96 +++++++++++++++++++++++++++++++++++-- src/lib/agent.ts | 78 +++++++++++++++++++++++++++--- src/lib/constants.ts | 13 ++++- 4 files changed, 179 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 4016947..1b71be2 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ Hit a global shortcut, type a question, get a streamed answer without leaving wh - **Persistent conversations.** Every chat is saved locally. The sidebar lists them by recency, and reopening the command picks up where you left off, unless it has been idle longer than you allow. - **Readable code.** Fenced blocks render in a dark panel with the language named in a header, syntax colouring, and a Copy button. - **Real markdown.** Tables, bulleted and numbered lists, headings, blockquotes, bold, italics and links all render natively, and the default prompt asks for the shape that fits the answer rather than flattening everything into prose. -- **Tools.** Lam decides when to call them: current time, location context, web search, and page fetching. +- **Tools.** Lam decides when to call them: current time, location context, web search, and page fetching. A turn that uses up its tool budget still ends in an answer, written from what it gathered, rather than a notice that it stopped. - **Screenshots, two ways.** Snip a region into the conversation you are already in from the action panel, or run **Snip to Lam** to start a new chat about it. The launcher window gets out of the way first, so it never ends up in the shot, and the capture appears in the transcript as a thumbnail with Open Screenshot for the full image. Neither is offered unless the current model can actually see images. - **Chat only.** Lam answers questions. It does not touch your files or run commands. The one exception is capture, and it only ever happens because you pressed the key for it. @@ -158,7 +158,9 @@ This extension does not use GPS. That would mean CoreLocation on macOS, which ha **The provider has to be one that answers an unauthenticated caller.** The first choice was ipapi.co, which no longer does: a request with no User-Agent now returns `HTTP 429` with `"Please sign up for a paid plan"`, and adding a browser User-Agent gets an `HTTP 403` Cloudflare bot challenge instead. ipwho.is answers without a key and signals failures with `success: false` on an `HTTP 200`, so the body is checked as well as the status. `GEO_PROVIDER` is a single constant, and swapping it means changing the parser beside it. -Web search results and fetched pages can be large. Lam truncates them before they reach the model, and gives up after 5 rounds of tool calls rather than looping. +Web search results and fetched pages can be large. Lam truncates them before they reach the model. + +**A turn that runs out of tool calls still ends in an answer.** The loop stops after 8 rounds, and the first version of it stopped by throwing away every preamble the model had written and returning a notice about the loop: asked for the fish in every Valheim biome, it researched for five rounds and then said nothing useful at all. The budget is now spent on a *closing round* with the tools taken away and an instruction to answer from what was gathered and to name what it could not confirm. So the worst case is a partial answer that says what is missing, rather than an app-generated message about its own control flow. If the closing round produces nothing either, the last preamble is used, and only then a notice. The closing round is why the cap is 8 rather than 5: the extra round is not more searching, it is the answer. ## Architecture diff --git a/src/__tests__/agent.test.ts b/src/__tests__/agent.test.ts index a79064f..b6fd30c 100644 --- a/src/__tests__/agent.test.ts +++ b/src/__tests__/agent.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import path from "node:path"; import { readFileSync } from "node:fs"; import { pickToolCalls, runAgent } from "../lib/agent"; +import { MAX_TOOL_ITERATIONS } from "../lib/constants"; import { OllamaError } from "../lib/ollama"; import { toolsFor } from "../lib/tools"; import type { ApiMessage, ToolCall } from "../lib/types"; @@ -136,15 +137,102 @@ describe("runAgent", () => { it("stops after the iteration cap instead of looping forever", async () => { // Every response asks for another tool call, so only the cap can end this. - for (let i = 0; i < 10; i++) { + for (let i = 0; i < MAX_TOOL_ITERATIONS + 2; i++) { fetchMock.mockResolvedValueOnce(ndjsonResponse([toolCall("get_datetime")])); } const result = await runAgent(baseOptions([{ role: "user", content: "hi" }])); - expect(result.content).toContain("Stopped after"); - expect(result.toolCalls).toHaveLength(5); - expect(fetchMock).toHaveBeenCalledTimes(5); + expect(result.toolCalls).toHaveLength(MAX_TOOL_ITERATIONS); + // Every tool round, plus one closing round with the tools taken away. + expect(fetchMock).toHaveBeenCalledTimes(MAX_TOOL_ITERATIONS + 1); + // The closing round produced no text either, so the notice is the last resort. + expect(result.content).toContain("Ran out of tool calls"); + }); + + it("gives the model a closing round so a spent budget still ends in an answer", async () => { + // The reported failure: a broad research question used up the budget and the + // turn ended with a notice about the loop instead of an answer. + for (let i = 0; i < MAX_TOOL_ITERATIONS; i++) { + fetchMock.mockResolvedValueOnce(ndjsonResponse([toolCall("web_search")])); + } + fetchMock.mockResolvedValueOnce( + ndjsonResponse(answer("Meadows have Perch, at 2 fish each. I could not confirm the others.")), + ); + + const result = await runAgent(baseOptions([{ role: "user", content: "what fish are in valheim" }])); + + expect(result.content).toContain("Meadows have Perch"); + expect(result.content).not.toContain("Ran out of tool calls"); + }); + + it("takes the tools away for the closing round, so it writes rather than searches again", async () => { + for (let i = 0; i < MAX_TOOL_ITERATIONS; i++) { + fetchMock.mockResolvedValueOnce(ndjsonResponse([toolCall("web_search")])); + } + fetchMock.mockResolvedValueOnce(ndjsonResponse(answer("Here is what I found."))); + + await runAgent(baseOptions([{ role: "user", content: "hi" }])); + + const first = JSON.parse(fetchMock.mock.calls[0][1].body as string) as { tools?: unknown }; + const last = JSON.parse(fetchMock.mock.calls[MAX_TOOL_ITERATIONS][1].body as string) as { + tools?: unknown; + }; + + expect(first.tools).toBeDefined(); + expect(last.tools).toBeUndefined(); + }); + + it("asks the closing round to name what it could not confirm", async () => { + for (let i = 0; i < MAX_TOOL_ITERATIONS; i++) { + fetchMock.mockResolvedValueOnce(ndjsonResponse([toolCall("web_search")])); + } + fetchMock.mockResolvedValueOnce(ndjsonResponse(answer("ok"))); + + await runAgent(baseOptions([{ role: "user", content: "hi" }])); + + const last = JSON.parse(fetchMock.mock.calls[MAX_TOOL_ITERATIONS][1].body as string) as { + messages: ApiMessage[]; + }; + const nudge = last.messages[last.messages.length - 1]; + + expect(nudge.role).toBe("user"); + expect(nudge.content).toContain("no more tools are available"); + expect(nudge.content).toContain("could not confirm"); + }); + + it("falls back to the last preamble when the closing round comes back empty", async () => { + // A preamble is not normally the answer, but a turn that gathered things + // should still say more than a notice about its own loop. + const preamble = { + message: { + role: "assistant", + content: "Let me look that up.", + tool_calls: [{ function: { name: "web_search", arguments: {} } }], + }, + done: true, + }; + + for (let i = 0; i < MAX_TOOL_ITERATIONS; i++) { + fetchMock.mockResolvedValueOnce(ndjsonResponse([preamble])); + } + fetchMock.mockResolvedValueOnce(ndjsonResponse([{ message: { role: "assistant", content: "" }, done: true }])); + + const result = await runAgent(baseOptions([{ role: "user", content: "hi" }])); + + expect(result.content).toBe("Let me look that up."); + }); + + it("still reports the tools it used when the budget runs out", async () => { + for (let i = 0; i < MAX_TOOL_ITERATIONS; i++) { + fetchMock.mockResolvedValueOnce(ndjsonResponse([toolCall("get_datetime")])); + } + fetchMock.mockResolvedValueOnce(ndjsonResponse(answer("done"))); + + const result = await runAgent(baseOptions([{ role: "user", content: "hi" }])); + + expect(result.toolCalls).toHaveLength(MAX_TOOL_ITERATIONS); + expect(result.toolCalls.every((call) => call.name === "get_datetime")).toBe(true); }); it("surfaces an actionable hint when the API key is rejected", async () => { diff --git a/src/lib/agent.ts b/src/lib/agent.ts index c50c9e1..3f3918b 100644 --- a/src/lib/agent.ts +++ b/src/lib/agent.ts @@ -59,10 +59,36 @@ function normalizeToolCall(call: ToolCall): { name: string; arguments: Record }; } +/** + * The last word, when the tool budget runs out. + * + * The loop used to end by throwing away every preamble the model had written and + * returning a notice about itself, so a long research question produced no answer + * at all. One closing round, with the tools taken away, turns that into an answer + * built from whatever was already gathered, with the gaps named. The instruction + * asks for the gaps because an answer that knows what it is missing is worth more + * than a confident half of one. + */ +const CLOSING_INSTRUCTION = + "You have reached the limit on tool calls for this turn, so no more tools are available. Answer now using what you have already gathered. If something is missing, say plainly which parts you could not confirm."; + +/** Only reachable if the closing round comes back empty too. */ +function exhaustedNotice(rounds: number): string { + return `_Ran out of tool calls after ${rounds} rounds and could not write an answer. Try asking something narrower._`; +} + export async function runAgent(options: AgentOptions): Promise { const conversation: ApiMessage[] = [...options.messages]; const toolCalls: ToolCallRecord[] = []; let thinking = ""; + /** + * The most recent preamble the model wrote. + * + * Not normally shown, because a preamble is what it says before finding out + * anything. Kept anyway, so that a turn which runs out of budget still has + * something to fall back on if the closing round produces nothing. + */ + let lastPreamble = ""; for (let iteration = 0; iteration < MAX_TOOL_ITERATIONS; iteration++) { let content = ""; @@ -91,20 +117,25 @@ export async function runAgent(options: AgentOptions): Promise { thinking += streamedThinking; - // No tool calls means this was the model's answer. Earlier iterations only - // produced a preamble, which is intentionally discarded rather than shown - // as a half-answer. + // No tool calls means this was the model's answer, rather than something it + // said on the way to finding out. if (!pending || pending.length === 0) { return { content, thinking, toolCalls }; } + if (content.trim()) lastPreamble = content; + conversation.push({ role: "assistant", content, tool_calls: pending }); for (const rawCall of pending) { const { name, arguments: args } = normalizeToolCall(rawCall); if (!name) { - conversation.push({ role: "tool", content: "Malformed tool call: missing function name.", tool_name: "unknown" }); + conversation.push({ + role: "tool", + content: "Malformed tool call: missing function name.", + tool_name: "unknown", + }); toolCalls.push({ name: "unknown", arguments: {}, result: "Malformed tool call.", isError: true }); continue; } @@ -119,7 +150,12 @@ export async function runAgent(options: AgentOptions): Promise { signal: options.signal, }); - const record: ToolCallRecord = { name, arguments: args, result: result.content, isError: result.isError }; + const record: ToolCallRecord = { + name, + arguments: args, + result: result.content, + isError: result.isError, + }; toolCalls.push(record); options.events?.onToolCall?.(record); @@ -127,8 +163,38 @@ export async function runAgent(options: AgentOptions): Promise { } } + // Budget spent, so the turn gets one closing round with the tools taken away. + // Ending on an answer drawn from what was gathered beats ending on a notice + // about the loop itself, and the model is the only one that can say what it + // did and did not manage to confirm. + options.events?.onStatus?.("Writing the answer"); + + conversation.push({ role: "user", content: CLOSING_INSTRUCTION }); + + let closing = ""; + + for await (const chunk of streamChat({ + host: options.host, + apiKey: options.apiKey, + model: options.model, + messages: conversation, + // Deliberately no tools: the answer has to be written from what is already here. + tools: undefined, + think: options.think, + signal: options.signal, + })) { + if (chunk.thinking) { + thinking += chunk.thinking; + options.events?.onThinkingDelta?.(chunk.thinking); + } + if (chunk.content) { + closing += chunk.content; + options.events?.onContentDelta?.(chunk.content); + } + } + return { - content: `_Stopped after ${MAX_TOOL_ITERATIONS} rounds of tool calls without a final answer._`, + content: closing.trim() || lastPreamble.trim() || exhaustedNotice(MAX_TOOL_ITERATIONS), thinking, toolCalls, }; diff --git a/src/lib/constants.ts b/src/lib/constants.ts index 2d3e51b..521afb4 100644 --- a/src/lib/constants.ts +++ b/src/lib/constants.ts @@ -7,8 +7,17 @@ export const DEFAULT_MODEL = "gpt-oss:120b"; export const STORAGE_INDEX_KEY = "lam.sessions.index"; export const STORAGE_SESSION_PREFIX = "lam.session."; -/** How many times the model may call tools before Lam gives up and reports back. */ -export const MAX_TOOL_ITERATIONS = 5; +/** + * How many rounds of tool calls one turn may spend before the model is told to + * answer with what it has. + * + * Five was too few for a question that needs several searches and a few pages, + * and the failure mode was the worst one available: the budget ran out, every + * preamble the model had written was discarded, and the turn ended with a notice + * about the limit rather than an answer. The closing round in `agent.ts` now + * turns that into an answer, and this is the room it gets to gather first. + */ +export const MAX_TOOL_ITERATIONS = 8; /** Oldest sessions past this count are evicted, along with their storage keys. */ export const MAX_SESSIONS = 100; -- 2.54.0