fix(web): keep escapes, UNC roots, and truncated cwd honest
Four review findings. Two are defects the previous two rounds introduced, which the existing tests did not catch: A backspace erased raw bytes, so one landing after an SGR reset ate part of the escape: `\x1b[31mabc\x1b[0m\b\bXY` left `\x1b[` and repainted the rest of the line with whatever the remainder parsed as. Backspaces now resolve over VISIBLE characters — a CSI sequence is one indivisible unit a backspace steps over on its way to the last printed character, so the surviving text keeps the color its run authored. The cwd normalizer popped a UNC share root: `\\server\share` with a `..` became `/server`, losing the separators too. A UNC path's server and share are its root, and Windows cannot climb above a share, so they are split off and the remainder collapses against that root. The other two are gaps the earlier fixes left: The render-site fallback row still passed the args-derived summary, so any terminal-declaring tool without its own keyed row (`terminal_send`) lost the contract's above-card description. It now prefers the description exactly as BashRow does. A settled call read `call?.cwd`, which cannot tell "the call omitted a cwd" from "the paging window dropped the call head". The second case has no cwd anywhere and the original call may have used an explicit workdir, so it now draws a bare `$` instead of naming the session workspace.
This commit is contained in:
@@ -28,6 +28,7 @@ const VARIANT_ICONS: Record<ToolRowVariant, ReactNode> = {
|
||||
|
||||
export function GenericToolCard({ toolName, block, cwd, openFile }: ToolRowOwnerProps) {
|
||||
const model = toolRowModel(toolName, block, cwd)
|
||||
const terminal = terminalCardModel(block, cwd)
|
||||
const singleFile = model.filePath !== undefined
|
||||
return (
|
||||
<ToolRow
|
||||
@@ -35,10 +36,12 @@ export function GenericToolCard({ toolName, block, cwd, openFile }: ToolRowOwner
|
||||
toolName={toolName}
|
||||
icon={VARIANT_ICONS[model.variant]}
|
||||
title={model.title}
|
||||
summary={model.summary}
|
||||
// A terminal presenter's description is the contract's above-card text, so
|
||||
// it outranks the args-derived summary here exactly as it does in BashRow.
|
||||
summary={terminal?.description ?? model.summary}
|
||||
// Single-file tools never expose an args body — the path link is the only action.
|
||||
body={singleFile ? null : model.body}
|
||||
terminal={terminalCardModel(block, cwd)}
|
||||
terminal={terminal}
|
||||
state={model.state}
|
||||
filePath={model.filePath}
|
||||
onOpenFile={singleFile ? openFile : undefined}
|
||||
|
||||
@@ -68,31 +68,59 @@ function resolveTerminalCwd(viewCwd: string | undefined, sessionCwd: string | un
|
||||
* running, so a joined `/w/app/..` must display as `w`, not as `..`. Separators
|
||||
* are preserved as authored (a Windows path keeps its backslashes) because this
|
||||
* value is only ever displayed; a `..` that would climb past the root is
|
||||
* dropped, which is what a filesystem does with it.
|
||||
* dropped, which is what a filesystem does with it. A UNC path's `server` and
|
||||
* `share` are part of its root, not poppable segments: Windows cannot climb
|
||||
* above a share, so `\\\\server\\share` with a `..` stays there.
|
||||
* @param path - a joined or absolute path, possibly carrying `.`/`..` segments.
|
||||
* @returns the same path with those segments resolved.
|
||||
*/
|
||||
function normalizeSegments(path: string): string {
|
||||
if (!/(?:^|[/\\])\.\.?(?:[/\\]|$)/.test(path)) return path
|
||||
// A UNC path is `\\\\server\\share\\...`: the server and share form the root,
|
||||
// so they are split off here and neither is a segment `..` may pop. Its
|
||||
// separator is fixed to a backslash, since a joined relative part may have
|
||||
// introduced a forward slash that UNC syntax does not use.
|
||||
const unc = /^[/\\]{2}([^/\\]+)[/\\]+([^/\\]+)/.exec(path)
|
||||
if (unc !== null) {
|
||||
// Both groups are mandatory in the pattern, so destructuring types them as
|
||||
// strings without an assertion.
|
||||
const [matched, server, share] = unc
|
||||
const root = `\\\\${String(server)}\\${String(share)}`
|
||||
// Rooted: what follows the share hangs off it, so a `..` at the top is
|
||||
// dropped rather than kept — Windows cannot climb above a share.
|
||||
const rest = collapse(path.slice(matched.length), true)
|
||||
return rest === '' ? root : `${root}\\${rest}`
|
||||
}
|
||||
const backslashed = path.includes('\\') && !path.includes('/')
|
||||
const separator = backslashed ? '\\' : '/'
|
||||
const leading = /^[/\\]/.test(path) ? separator : ''
|
||||
const rooted = /^[/\\]/.test(path)
|
||||
const drive = /^[A-Za-z]:/.exec(path)?.[0] ?? ''
|
||||
const body = collapse(path.slice(drive.length), rooted || drive !== '', separator)
|
||||
const leading = rooted ? separator : ''
|
||||
return drive === '' ? `${leading}${body}` : `${drive}${rooted ? leading : separator}${body}`
|
||||
}
|
||||
|
||||
/**
|
||||
* Collapse the `.`/`..` segments of a path body against a known root state.
|
||||
* @param body - the path after any drive letter or UNC root.
|
||||
* @param rooted - the body hangs off a root, so a `..` at its top is dropped
|
||||
* the way a filesystem drops one; without a root the `..` is kept, since it
|
||||
* stays meaningful against a cwd this function cannot see.
|
||||
* @param separator - separator to rejoin with (default `/`).
|
||||
* @returns the collapsed body, without leading or trailing separators.
|
||||
*/
|
||||
function collapse(body: string, rooted: boolean, separator = '/'): string {
|
||||
const kept: string[] = []
|
||||
for (const segment of path.slice(drive.length).split(/[/\\]/)) {
|
||||
for (const segment of body.split(/[/\\]/)) {
|
||||
if (segment === '' || segment === '.') continue
|
||||
if (segment === '..') {
|
||||
// Nothing to climb from: at a root the segment is dropped, matching the
|
||||
// filesystem; on a relative path the `..` has to stay, since it is still
|
||||
// meaningful against a cwd this function cannot see.
|
||||
if (kept.length > 0 && kept[kept.length - 1] !== '..') kept.pop()
|
||||
else if (leading === '' && drive === '') kept.push(segment)
|
||||
else if (!rooted) kept.push(segment)
|
||||
continue
|
||||
}
|
||||
kept.push(segment)
|
||||
}
|
||||
const body = kept.join(separator)
|
||||
return drive === '' ? `${leading}${body}` : `${drive}${leading === '' ? separator : leading}${body}`
|
||||
return kept.join(separator)
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -146,7 +174,12 @@ export function terminalCardModel(block: ToolCallBlock, sessionCwd?: string): Te
|
||||
// (the presentation contract's replacement-title rule); the call title is
|
||||
// what a result without one keeps.
|
||||
command: result.title ?? call?.title ?? '',
|
||||
cwd: resolveTerminalCwd(call?.cwd, sessionCwd),
|
||||
// Only a PRESENT call view can mean "omitted the cwd, so use the
|
||||
// workspace". When the window dropped the call head there is no cwd
|
||||
// anywhere — the result view carries none — and the original call may
|
||||
// well have used an explicit workdir, so the prompt draws a bare `$`
|
||||
// rather than naming a directory this card cannot know.
|
||||
cwd: call === null ? undefined : resolveTerminalCwd(call.cwd, sessionCwd),
|
||||
output: result.output,
|
||||
exitCode: result.exitCode,
|
||||
signal: result.signal,
|
||||
|
||||
@@ -153,6 +153,32 @@ describe('terminalCardModel', () => {
|
||||
}))?.card.cwd).toBe('../elsewhere')
|
||||
})
|
||||
|
||||
it('keeps a UNC server and share as an unpoppable root', () => {
|
||||
// Windows cannot climb above a share, so `..` from the share root stays put.
|
||||
expect(terminalCardModel(settled({
|
||||
callView: callTerminal({ cwd: '..' }),
|
||||
}), '\\\\server\\share')?.card.cwd).toBe('\\\\server\\share')
|
||||
// Below the share it pops normally, keeping the UNC separators.
|
||||
expect(terminalCardModel(settled({
|
||||
callView: callTerminal({ cwd: '..' }),
|
||||
}), '\\\\server\\share\\app')?.card.cwd).toBe('\\\\server\\share')
|
||||
// Several `..` cannot escape the root either.
|
||||
expect(terminalCardModel(settled({
|
||||
callView: callTerminal({ cwd: '../../..' }),
|
||||
}), '\\\\server\\share\\app')?.card.cwd).toBe('\\\\server\\share')
|
||||
})
|
||||
|
||||
it('draws a bare $ when the window dropped the call head, rather than guessing', () => {
|
||||
// A truncated call carries no cwd anywhere: the result view has none, and
|
||||
// the original call may have used an explicit workdir. Falling back to the
|
||||
// session workspace here would name a directory the card cannot know.
|
||||
expect(terminalCardModel(settled({
|
||||
call: null, callView: null, resultView: resultTerminal({ title: 'ls -la' }),
|
||||
}), '/w/app')?.card.cwd).toBeUndefined()
|
||||
// A present call view that omits its cwd still means the workspace.
|
||||
expect(terminalCardModel(settled(), '/w/app')?.card.cwd).toBe('/w/app')
|
||||
})
|
||||
|
||||
it('carries the call view\'s description, which the contract renders above the card', () => {
|
||||
expect(terminalCardModel(settled())?.description).toBe('List files')
|
||||
expect(terminalCardModel(running())?.description).toBe('List files')
|
||||
@@ -231,6 +257,16 @@ describe('chat row terminal body', () => {
|
||||
expect(view.container.querySelectorAll('[data-terminal] [data-state]')).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('the fallback row shows the presenter description, not the args summary', () => {
|
||||
// Any terminal-declaring tool without its own keyed row lands here, so the
|
||||
// contract's above-card description has to win at this render site as well.
|
||||
const view = render(<GenericToolCard {...ownerProps(settled({
|
||||
callView: callTerminal({ description: 'Terminal 3' }),
|
||||
}))} />)
|
||||
expect(view.getByText('Terminal 3')).toBeTruthy()
|
||||
expect(view.queryByText('List files')).toBeNull()
|
||||
})
|
||||
|
||||
it('a running terminal call expands to the prompt line with no output yet', () => {
|
||||
const view = render(<GenericToolCard {...ownerProps(running())} />)
|
||||
fireEvent.click(view.container.querySelector('button')!)
|
||||
|
||||
@@ -114,14 +114,49 @@ function applyCarriageReturns(text: string): string {
|
||||
*/
|
||||
function applyBackspaces(text: string): string {
|
||||
if (!text.includes('\u0008')) return text
|
||||
return text.split('\n').map((line) => {
|
||||
const kept: string[] = []
|
||||
for (const char of line) {
|
||||
if (char === '\u0008') kept.pop()
|
||||
else kept.push(char)
|
||||
return text.split('\n').map(applyBackspacesToLine).join('\n')
|
||||
}
|
||||
|
||||
/**
|
||||
* One line's backspaces, resolved over VISIBLE characters only. A CSI sequence
|
||||
* moves no cursor, so it must survive intact: erasing its bytes would corrupt
|
||||
* the sequence and repaint the rest of the output with whatever the mangled
|
||||
* remainder parses as. The sequences are therefore held as indivisible units
|
||||
* that a backspace steps over on its way to the last printed character, and a
|
||||
* unit already erased stays erased so a run's own color still applies to what
|
||||
* remains of it.
|
||||
* @param line - one output line, still carrying its CSI sequences.
|
||||
* @returns the line with each backspace applied to the character before it.
|
||||
*/
|
||||
function applyBackspacesToLine(line: string): string {
|
||||
if (!line.includes('\u0008')) return line
|
||||
const units: { text: string; visible: boolean }[] = []
|
||||
// Same shape anser splits on: CSI ... final byte. Matched here so a sequence
|
||||
// is one unit rather than a run of erasable characters.
|
||||
const csi = /\u001b\[[\u0030-\u003f]*[\u0020-\u002f]*[\u0040-\u007e]/g
|
||||
let at = 0
|
||||
for (const match of line.matchAll(csi)) {
|
||||
for (const char of line.slice(at, match.index)) units.push({ text: char, visible: true })
|
||||
units.push({ text: match[0], visible: false })
|
||||
at = match.index + match[0].length
|
||||
}
|
||||
for (const char of line.slice(at)) units.push({ text: char, visible: true })
|
||||
|
||||
const kept: { text: string; visible: boolean }[] = []
|
||||
for (const unit of units) {
|
||||
if (unit.visible && unit.text === '\u0008') {
|
||||
// Walk back past any escapes to the last printed character and drop it,
|
||||
// keeping those escapes so the surviving text stays styled as authored.
|
||||
for (let index = kept.length - 1; index >= 0; index--) {
|
||||
if (kept[index]?.visible !== true) continue
|
||||
kept.splice(index, 1)
|
||||
break
|
||||
}
|
||||
continue
|
||||
}
|
||||
return kept.join('')
|
||||
}).join('\n')
|
||||
kept.push(unit)
|
||||
}
|
||||
return kept.map(unit => unit.text).join('')
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -184,6 +184,24 @@ describe('parseAnsiLines: backspaces', () => {
|
||||
])
|
||||
})
|
||||
|
||||
it('steps over an SGR sequence instead of erasing its bytes', () => {
|
||||
// `abc` reset then two backspaces then `XY`: erasing the reset's bytes would
|
||||
// corrupt it and repaint the rest of the line with whatever the remainder
|
||||
// parses as. The visible result is `aXY`, still red, with the reset intact.
|
||||
expect(parseAnsiLines(`${sgr('31', 'abc')}${BS}${BS}XY`)).toEqual([[
|
||||
{ text: 'a', style: { color: 'var(--dsw-alias-state-error-primary)' } },
|
||||
{ text: 'XY', style: undefined },
|
||||
]])
|
||||
})
|
||||
|
||||
it('erases across a style boundary without dropping the styles between', () => {
|
||||
// The backspace reaches back past the reset to the last printed character.
|
||||
expect(parseAnsiLines(`${sgr('32', 'ok')}${ESC}[31m${BS}bad`)).toEqual([[
|
||||
{ text: 'o', style: { color: 'var(--dsw-alias-state-success-primary)' } },
|
||||
{ text: 'bad', style: { color: 'var(--dsw-alias-state-error-primary)' } },
|
||||
]])
|
||||
})
|
||||
|
||||
it('applies the overwrite after a carriage-return redraw, not before', () => {
|
||||
// The redraw wins first; the backspace then erases inside what survived.
|
||||
expect(onlySpan(`old\rnew${BS}`)).toEqual({ text: 'ne', style: undefined })
|
||||
|
||||
Reference in New Issue
Block a user