From 25633ece33d90aa6e331bc2f9f7bf0d270864452 Mon Sep 17 00:00:00 2001 From: Mike Lyons Date: Sat, 10 Oct 2026 12:52:02 -0400 Subject: [PATCH] Teach a table-safe image size syntax, and only to agents that can use it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems with how the screenshot-embed behaviour was prompted. The documented size syntax broke inside tables. A pipe in a GFM table row is a column delimiter, so `![cap|300](path)` is split into `![cap` and `300](path)` before any inline parsing happens — no image node is produced at all and the cells render as broken literal text. The pipe form was the one form we taught, so this fired exactly when an agent did what it was told. Every prompt surface now teaches the `#w=` fragment instead: `![cap](path#w=500)`, `#w=50%`, `#w=400x300`. It's part of the URL, so it survives table cells, lists and blockquotes untouched. The pipe and `=title` forms still work for agents that guess them; the pipe is now documented only as the thing to avoid. Fragments also needed percent-decoding — markdown encodes the URL on the way through, so an agent's `#w=50%` arrives as `#w=50%25`. The tool description also over-promised. The MCP bridge is shared between Chat and Terminal tabs, but inline rendering only exists in Chat; a terminal-tab agent was told "Ness renders the screenshot inline in the chat" and would emit markdown into an xterm, where it's literal text. CallerScope gains rendersMarkdown (true for 'json-claude' tabs), /scope carries it, and the bridge strips the embed sentence from the tool description and the hint from the result line when it's false — mirroring how stripForkAffordance already works. The path itself is still reported either way. Defaults permissive when /scope can't say. src/renderer/chat-image-markdown.test.ts pins the actual failure: it renders each syntax through ReactMarkdown + remark-gfm inside a table cell and asserts the pipe form yields no image while the fragment does. Co-Authored-By: Claude Opus 5 --- resources/mcp-bridge.js | 51 +++++++++-- src/main/control-server.test.ts | 25 +++++- src/main/control-server.ts | 5 ++ src/main/index.ts | 11 ++- src/main/persistence.ts | 2 +- src/renderer/chat-image-markdown.test.ts | 103 +++++++++++++++++++++++ src/shared/chat-image-src.test.ts | 14 +++ src/shared/chat-image-src.ts | 10 ++- 8 files changed, 207 insertions(+), 14 deletions(-) create mode 100644 src/renderer/chat-image-markdown.test.ts diff --git a/resources/mcp-bridge.js b/resources/mcp-bridge.js index 9f5d5fc2..089a92cd 100644 --- a/resources/mcp-bridge.js +++ b/resources/mcp-bridge.js @@ -166,6 +166,20 @@ function truncateDom(html, maxBytes) { const FORK_DESCRIPTION_SENTENCE = ' The new tab normally starts as a blank conversation seeded with initialPrompt; set forkConversation to instead hand it a copy of THIS conversation to continue from.' +// Appended to screenshot_tab's description, and removed again by +// stripEmbedAffordance for callers whose transcript isn't rendered markdown. +// A terminal-tab agent that follows this advice writes markdown into an +// xterm, where it is literal text. Same keep-it-in-one-constant reasoning as +// FORK_DESCRIPTION_SENTENCE above. +// +// The documented size syntax is the `#w=` fragment rather than a `|` spec: +// a pipe inside a markdown table cell is a column delimiter, so GFM splits +// `![cap|300](path)` into two cells before any image is parsed and the embed +// renders as the literal text `![cap` / `300](path)`. The fragment survives +// table cells, lists, and blockquotes untouched. +const EMBED_DESCRIPTION_SENTENCE = + ' Also returns the path the capture was saved to: embed that path as markdown (`![caption](path)`) in your reply and Ness renders the screenshot inline in the chat, which is the clearest way to show a UI change you just made. Append a `#w=` fragment to set its displayed width — `![login form](path#w=500)` for 500px, `#w=50%` to fill half the column, `#w=400x300` for both dimensions; with no fragment it renders at a default size. Prefer the fragment over a `|` size spec: a pipe breaks the embed when it lands inside a markdown table cell.' + const TOOLS = [ { name: 'create_worktree', @@ -350,7 +364,8 @@ const TOOLS = [ { name: 'screenshot_tab', description: - "Take a screenshot of a browser tab in this worktree at the viewport's CSS-pixel dimensions — so screenshot coords can be passed straight to click_tab. Returns a JPEG (quality 70) by default for context-efficiency; ask for PNG only when lossless matters. Screenshots are for visual verification, not for finding click targets — prefer get_tab_clickables for interaction. Also returns the path the capture was saved to: embed that path as markdown (`![](path)`) in your reply and Ness renders the screenshot inline in the chat, which is the clearest way to show the user a UI change you just made. Set the display size with a spec after a pipe in the alt text — `![login form|500](path)` for 500px wide, `![x|50%](path)` to fill half the column, `![x|400x300](path)` for both dimensions; with no spec it renders at a default size. Tab id comes from list_browser_tabs.", + "Take a screenshot of a browser tab in this worktree at the viewport's CSS-pixel dimensions — so screenshot coords can be passed straight to click_tab. Returns a JPEG (quality 70) by default for context-efficiency; ask for PNG only when lossless matters. Screenshots are for visual verification, not for finding click targets — prefer get_tab_clickables for interaction. Tab id comes from list_browser_tabs." + + EMBED_DESCRIPTION_SENTENCE, inputSchema: { type: 'object', properties: { @@ -683,6 +698,15 @@ async function getConversationForkEnabled() { return s.conversationFork ? s.conversationFork.enabled === true : false } +/** Whether the caller's transcript renders markdown (a Chat tab). Defaults + * to true when /scope can't say — same permissive-on-failure posture as the + * browser gates, and over-advertising costs a terminal agent a wasted + * sentence where under-advertising hides the feature from a chat agent. */ +async function getRendersMarkdown() { + const s = await getScopeInfo() + return s.scope ? s.scope.rendersMarkdown !== false : true +} + function filterToolsByPerms(tools, perms) { return tools.filter((t) => { if (t.name === 'send_message') return perms.messaging.enabled @@ -697,6 +721,14 @@ function filterToolsByPerms(tools, perms) { // Strip every trace of forking from create_worktree when it's disabled, rather // than advertising a parameter whose only outcome is a rejection. +function stripEmbedAffordance(tools) { + return tools.map((t) => + t.name === 'screenshot_tab' + ? { ...t, description: t.description.replace(EMBED_DESCRIPTION_SENTENCE, '') } + : t + ) +} + function stripForkAffordance(tools) { return tools.map((t) => { if (t.name !== 'create_worktree') return t @@ -856,17 +888,21 @@ async function handleToolCall(name, args) { // One terse line, not a paragraph: this rides along with every // screenshot (context cost + it shows in the tool card). How to use // the path lives in the tool description, which is in the system - // prompt anyway. + // prompt anyway. The path itself is worth having either way — it's + // the embed hint that's Chat-tab-only. + const embeddable = await getRendersMarkdown() content.push({ type: 'text', text: 'Saved to ' + r.path + - ' — embed as ![](' + - r.path + - ') to show it in your reply, or ![|600](' + - r.path + - ') to set its width.' + (embeddable + ? ' — embed as ![](' + + r.path + + ') to show it in your reply, or ![](' + + r.path + + '#w=600) to set its width.' + : '') }) } return { content } @@ -1067,6 +1103,7 @@ async function handle(msg) { const perms = await getToolPerms() let tools = filterToolsByPerms(TOOLS, perms) if (!(await getConversationForkEnabled())) tools = stripForkAffordance(tools) + if (!(await getRendersMarkdown())) tools = stripEmbedAffordance(tools) return { jsonrpc: '2.0', id, result: { tools } } } if (method === 'tools/call') { diff --git a/src/main/control-server.test.ts b/src/main/control-server.test.ts index ca837aad..e297cd32 100644 --- a/src/main/control-server.test.ts +++ b/src/main/control-server.test.ts @@ -45,7 +45,8 @@ const scope: CallerScope = { terminalId: CALLER_TERMINAL, worktreePath: CALLER_WORKTREE, repoRoot: '/repo', - isMain: false + isMain: false, + rendersMarkdown: true } /** Mutable so a test can flip the setting off without restarting the server — @@ -128,12 +129,14 @@ let baseUrl: string let token: string async function get( - path: string + path: string, + extraHeaders?: Record ): Promise<{ status: number; json: Record }> { const res = await fetch(`${baseUrl}${path}`, { headers: { Authorization: `Bearer ${token}`, - 'X-Harness-Terminal-Id': CALLER_TERMINAL + 'X-Harness-Terminal-Id': CALLER_TERMINAL, + ...extraHeaders } }) const text = await res.text() @@ -534,6 +537,22 @@ describe('control-server /messages endpoint', () => { }) }) +describe('control-server /scope endpoint', () => { + // The bridge gates screenshot_tab's "embed this in your reply" guidance + // on this flag — a terminal tab renders markdown as literal text, so it + // must not be told the affordance exists. + it('carries rendersMarkdown through to the caller', async () => { + const r = await get('/scope') + expect(r.status).toBe(200) + expect((r.json.scope as CallerScope).rendersMarkdown).toBe(true) + }) + + it('400s without a terminal id, rather than guessing', async () => { + const r = await get('/scope', { 'X-Harness-Terminal-Id': '' }) + expect(r.status).toBe(400) + }) +}) + describe('control-server /browser/screenshot endpoint', () => { beforeAll(() => { browserEnabled = true diff --git a/src/main/control-server.ts b/src/main/control-server.ts index 45d86864..6d6198db 100644 --- a/src/main/control-server.ts +++ b/src/main/control-server.ts @@ -128,6 +128,11 @@ export interface CallerScope { worktreePath: string repoRoot: string isMain: boolean + /** True when the caller's transcript is rendered as markdown (a Chat + * tab). Terminal tabs are an xterm, so affordances that only exist in + * the rendered transcript — embedding a screenshot in a reply — must + * not be advertised to them. */ + rendersMarkdown: boolean } export interface BrowserPerms { diff --git a/src/main/index.ts b/src/main/index.ts index 6550db88..8cad8785 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -237,10 +237,13 @@ function resolveCallerScope(terminalId: string) { if (!terminalId) return null const panes = store.getSnapshot().state.terminals.panes let worktreePath: string | null = null + let tabType: TerminalTab['type'] | null = null for (const [wtPath, tree] of Object.entries(panes)) { for (const leaf of getLeaves(tree)) { - if (leaf.tabs.some((t) => t.id === terminalId)) { + const tab = leaf.tabs.find((t) => t.id === terminalId) + if (tab) { worktreePath = wtPath + tabType = tab.type break } } @@ -255,7 +258,11 @@ function resolveCallerScope(terminalId: string) { terminalId, worktreePath, repoRoot: wt.repoRoot, - isMain: wt.isMain + isMain: wt.isMain, + // Chat tabs render their transcript as markdown; terminal tabs are an + // xterm, where an embedded image is just literal text. Tools that + // advertise rendering affordances gate on this. + rendersMarkdown: tabType === 'json-claude' } } diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 37178c92..f5bbca03 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -447,7 +447,7 @@ Click targeting workflow: **prefer get_tab_clickables → match by role + name Prefer these over blind curl/fetch — or shelling out to \`open \`, which launches the user's default browser outside Ness where you can't see the result — when you need to verify rendered UI, inspect a dev server, debug a page the user is looking at, or confirm your changes actually work in the browser. -Show your work with screenshots. Every screenshot_tab result comes back with the path it was saved to, and embedding an absolute image path as markdown (\`![](/path/to/shot.png)\`) renders it inline in the chat — so when you've changed something visual, end your reply with the screenshot that proves it rather than only describing it. This works for any image file on this machine, not just screenshot_tab captures. Size it with a spec after a pipe in the alt text: \`![caption|500](path)\` is 500px wide, \`![caption|50%](path)\` fills half the column, \`![caption|400x300](path)\` sets both dimensions, and no spec means a default size. Be selective: embed the one or two shots that show the result, not every frame you took while finding your way around. +Show your work with screenshots — this applies in Chat tabs, whose transcript is rendered markdown; a terminal tab is an xterm and will show the markdown as literal text. Every screenshot_tab result comes back with the path it was saved to, and embedding an absolute image path as markdown (\`![caption](/path/to/shot.png)\`) renders it inline — so when you've changed something visual, end your reply with the screenshot that proves it rather than only describing it. This works for any image file on this machine, not just screenshot_tab captures. Append a \`#w=\` fragment to set the displayed width: \`![caption](path#w=500)\` is 500px, \`#w=50%\` fills half the column, \`#w=400x300\` sets both dimensions, and no fragment means a default size. Use the fragment rather than a \`|\` size spec in the alt text — inside a markdown table a pipe is a column delimiter, so the cell splits and the embed renders as broken literal text. Be selective: embed the one or two shots that show the result, not every frame you took while finding your way around. Ness also exposes shell tabs for long-running processes — anything that wouldn't naturally exit within a few seconds (dev servers, watchers, \`tail -f\`, REPL-style tools, long builds). Drive them via the ness-control shell tools (scoped to this worktree only): diff --git a/src/renderer/chat-image-markdown.test.ts b/src/renderer/chat-image-markdown.test.ts new file mode 100644 index 00000000..1f470bd5 --- /dev/null +++ b/src/renderer/chat-image-markdown.test.ts @@ -0,0 +1,103 @@ +// What the markdown parser actually hands the chat's component, and +// what our src/alt parsers make of it. The size syntax we document to +// agents lives or dies here: a spec that the parser mangles before the +// component ever sees it can't be fixed downstream. +import { describe, it, expect } from 'vitest' +import { createElement } from 'react' +import { renderToStaticMarkup } from 'react-dom/server' +import ReactMarkdown from 'react-markdown' +import remarkGfm from 'remark-gfm' +import { + parseImageAlt, + parseImageTitle, + resolveLocalImageSrc, + type ImageSizeHint +} from '../shared/chat-image-src' + +interface Embed { + path: string | null + size: ImageSizeHint | null +} + +/** Render markdown the way the chat does and report every image that came + * out, resolved through the same parsers MarkdownImage uses. */ +function embeds(md: string): Embed[] { + const out: Embed[] = [] + const Img = (props: { src?: string; alt?: string; title?: string }): null => { + const local = resolveLocalImageSrc(props.src) + out.push({ + path: local?.path ?? null, + size: + parseImageAlt(props.alt).size ?? + parseImageTitle(props.title).size ?? + local?.size ?? + null + }) + return null + } + renderToStaticMarkup( + createElement( + ReactMarkdown, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + { remarkPlugins: [remarkGfm], components: { img: Img } } as any, + md + ) + ) + return out +} + +const TABLE = (cell: string): string => + ['| shot | note |', '| --- | --- |', `| ${cell} | x |`].join('\n') + +describe('chat image embeds — prose', () => { + it('sizes via a #w= fragment', () => { + expect(embeds('![cap](/tmp/a.png#w=300)')).toEqual([ + { path: '/tmp/a.png', size: { width: 300 } } + ]) + expect(embeds('![cap](/tmp/a.png#w=50%)')[0].size).toEqual({ widthPercent: 50 }) + expect(embeds('![cap](/tmp/a.png#w=400x300)')[0].size).toEqual({ + width: 400, + height: 300 + }) + }) + + it('still honours the alt-pipe and =title forms', () => { + expect(embeds('![cap|300](/tmp/a.png)')).toEqual([ + { path: '/tmp/a.png', size: { width: 300 } } + ]) + expect(embeds('![cap](/tmp/a.png "=300x")')).toEqual([ + { path: '/tmp/a.png', size: { width: 300 } } + ]) + }) + + it('renders unsized when no spec is given', () => { + expect(embeds('![cap](/tmp/a.png)')).toEqual([{ path: '/tmp/a.png', size: null }]) + }) +}) + +describe('chat image embeds — inside a GFM table cell', () => { + // The bug this file exists for. A pipe in a table row is a column + // delimiter, so GFM splits `![cap|300](path)` into `![cap` and + // `300](path)` before inline parsing — no image node is produced at all + // and the cells render as broken literal text. That's why the documented + // syntax is the fragment, not the pipe. + it('the pipe form produces NO image — this is why we stopped teaching it', () => { + expect(embeds(TABLE('![cap|300](/tmp/a.png)'))).toEqual([]) + }) + + it('the fragment form survives, sized', () => { + expect(embeds(TABLE('![cap](/tmp/a.png#w=300)'))).toEqual([ + { path: '/tmp/a.png', size: { width: 300 } } + ]) + }) + + it('the =title form survives too', () => { + expect(embeds(TABLE('![cap](/tmp/a.png "=300x")'))[0].size).toEqual({ width: 300 }) + }) + + it('an escaped pipe survives, for markdown that already uses it', () => { + expect(embeds(TABLE('![cap\\|300](/tmp/a.png)'))).toEqual([ + { path: '/tmp/a.png', size: { width: 300 } } + ]) + }) +}) diff --git a/src/shared/chat-image-src.test.ts b/src/shared/chat-image-src.test.ts index 78348822..b9e6299e 100644 --- a/src/shared/chat-image-src.test.ts +++ b/src/shared/chat-image-src.test.ts @@ -178,3 +178,17 @@ describe('resolveLocalImageSrc — size fragment', () => { }) }) }) + +describe('resolveLocalImageSrc — percent-encoded fragment', () => { + it('decodes before parsing, since markdown encodes % in the URL', () => { + expect(resolveLocalImageSrc('/tmp/a.png#w=50%25')?.size).toEqual({ + widthPercent: 50 + }) + }) + + it('tolerates a malformed escape in the fragment', () => { + expect(resolveLocalImageSrc('/tmp/a.png#w=50%')?.size).toEqual({ + widthPercent: 50 + }) + }) +}) diff --git a/src/shared/chat-image-src.ts b/src/shared/chat-image-src.ts index 82ae417b..484511b7 100644 --- a/src/shared/chat-image-src.ts +++ b/src/shared/chat-image-src.ts @@ -72,7 +72,15 @@ export function resolveLocalImageSrc(src: string | null | undefined): LocalImage if (hash > 0) { const head = path.slice(0, hash) if (imageMediaType(head)) { - const parsed = parseImageSizeSpec(path.slice(hash + 1)) + // The markdown parser percent-encodes the URL on the way through, so + // a `#w=50%` written by an agent arrives as `#w=50%25`. + let tail = path.slice(hash + 1) + try { + tail = decodeURIComponent(tail) + } catch { + /* malformed escape — parse what was written */ + } + const parsed = parseImageSizeSpec(tail) if (parsed) { path = head size = parsed