Skip to content

Commit 579727c

Browse files
committed
fix(agent): codex review fixes
- agentStore: useIDBKeyval is async; the previous code seeded `messages` with an empty snapshot then enabled the write-back watcher immediately, so any send before the IDB read settled would clobber stored history with []. Gate the watcher on `persisted.isFinished` and merge the hydrated entries when they arrive. - FoldablePanel: dropping `/` from the shell-operator regex. The attachment flow prefills the composer with paths like /input/foo.png, and the previous regex routed those to direct exec instead of the LLM. - coreutils grep: drain stdin eagerly so the no-match case can return exit code 1. The previous generator-based version always returned 0, breaking `grep ... && ...` and `grep ... || ...` decision flows the agent relies on. - registrySearch: stop advertising a non-existent `install-pack` command. Direct the user to ComfyUI-Manager UI instead until a real install-pack shell command exists. - agentTerminal.spec: rewrite for the textarea-based panel; the old `.xterm-rows` / `.xterm-helper-textarea` locators no longer resolve. Drops the Tab-completion test (useCompletion was deleted) and adds a Ctrl+O fold/unfold smoke test.
1 parent 3dff19f commit 579727c

5 files changed

Lines changed: 81 additions & 56 deletions

File tree

browser_tests/tests/agentTerminal.spec.ts

Lines changed: 32 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,14 @@ import type { ComfyPage } from '@e2e/fixtures/ComfyPage'
44
import { comfyPageFixture as test } from '@e2e/fixtures/ComfyPage'
55

66
/**
7-
* E2E coverage for the in-browser agent terminal (AgentFab + XtermPanel).
7+
* E2E coverage for the in-browser agent terminal (AgentFab + FoldablePanel).
88
*
9-
* We exercise the deterministic surface: FAB → open/close, the solid-block
10-
* ASCII banner, the "COMFY-AI" title pill, keyboard affordances
11-
* (Enter submits, Shift+Enter inserts a literal newline, Tab completes),
12-
* and a representative slice of shell commands that back the examples
13-
* documented in the LLM system prompt. We intentionally do NOT drive the
14-
* LLM itself — typing into xterm runs the shell directly, which is what
15-
* the model ends up calling via the `run_shell` tool.
9+
* The panel is now a Vue-native scrollback (no xterm.js), so the tests
10+
* target the plain DOM directly: the input is a `<textarea>` inside
11+
* `[data-testid="agent-terminal"]`, and the scrollback lives in the same
12+
* container as a list of message blocks. We exercise the deterministic
13+
* shell surface — typing into the textarea runs commands directly through
14+
* the runtime, which is what the LLM ends up calling via `run_shell`.
1615
*/
1716

1817
async function openPanel(comfyPage: ComfyPage): Promise<void> {
@@ -23,18 +22,12 @@ async function openPanel(comfyPage: ComfyPage): Promise<void> {
2322
}
2423

2524
async function readTerminalText(comfyPage: ComfyPage): Promise<string> {
26-
// xterm renders into rows under .xterm-rows; concatenate the text content.
27-
return await comfyPage.page
28-
.getByTestId('agent-terminal')
29-
.locator('.xterm-rows')
30-
.innerText()
25+
return await comfyPage.page.getByTestId('agent-terminal').innerText()
3126
}
3227

3328
async function typeAndEnter(comfyPage: ComfyPage, text: string): Promise<void> {
34-
const helper = comfyPage.page
35-
.getByTestId('agent-terminal')
36-
.locator('.xterm-helper-textarea')
37-
await helper.focus()
29+
const input = comfyPage.page.getByTestId('agent-terminal').locator('textarea')
30+
await input.focus()
3831
await comfyPage.page.keyboard.type(text)
3932
await comfyPage.page.keyboard.press('Enter')
4033
}
@@ -48,9 +41,6 @@ test.describe('Agent terminal', { tag: '@ui' }, () => {
4841
await expect(comfyPage.page.getByTestId('agent-panel-title')).toHaveText(
4942
'COMFY-AI'
5043
)
51-
52-
// Banner is suppressed for token efficiency — only the shell prompt
53-
// should be rendered once the terminal is ready.
5444
await expect.poll(() => readTerminalText(comfyPage)).toMatch(/comfy>/)
5545
})
5646

@@ -72,39 +62,22 @@ test.describe('Agent terminal', { tag: '@ui' }, () => {
7262
comfyPage
7363
}) => {
7464
await openPanel(comfyPage)
75-
const helper = comfyPage.page
65+
const input = comfyPage.page
7666
.getByTestId('agent-terminal')
77-
.locator('.xterm-helper-textarea')
78-
await helper.focus()
67+
.locator('textarea')
68+
await input.focus()
7969
await comfyPage.page.keyboard.type('echo one')
8070
await comfyPage.page.keyboard.press('Shift+Enter')
8171
await comfyPage.page.keyboard.type('echo two')
82-
// Still not submitted → now submit the whole multiline buffer.
72+
// Single submission should run BOTH lines as one multi-line script.
8373
await comfyPage.page.keyboard.press('Enter')
8474

8575
const out = await readTerminalText(comfyPage)
86-
// Both commands ran sequentially via the runtime.
8776
expect(out).toContain('one')
8877
expect(out).toContain('two')
8978
})
9079

91-
test('Tab completes a partial command', async ({ comfyPage }) => {
92-
await openPanel(comfyPage)
93-
const helper = comfyPage.page
94-
.getByTestId('agent-terminal')
95-
.locator('.xterm-helper-textarea')
96-
await helper.focus()
97-
// "des" → unique prefix of `describe`
98-
await comfyPage.page.keyboard.type('des')
99-
await comfyPage.page.keyboard.press('Tab')
100-
// Submit; describe w/o arg yields usage text on stderr.
101-
await comfyPage.page.keyboard.press('Enter')
102-
await expect
103-
.poll(() => readTerminalText(comfyPage))
104-
.toMatch(/usage: describe/)
105-
})
106-
107-
test('coreutils: pwd / echo / true / false', async ({ comfyPage }) => {
80+
test('coreutils: pwd / echo', async ({ comfyPage }) => {
10881
await openPanel(comfyPage)
10982
await typeAndEnter(comfyPage, 'pwd')
11083
await expect.poll(() => readTerminalText(comfyPage)).toMatch(/^\//m)
@@ -169,4 +142,21 @@ test.describe('Agent terminal', { tag: '@ui' }, () => {
169142
.poll(() => readTerminalText(comfyPage))
170143
.toMatch(/not found|unknown|no such/i)
171144
})
145+
146+
test('Ctrl+O folds and unfolds tool blocks', async ({ comfyPage }) => {
147+
await openPanel(comfyPage)
148+
await typeAndEnter(comfyPage, 'graph summary')
149+
// Tool blocks default to folded — body shouldn't be visible yet.
150+
const panel = comfyPage.page.getByTestId('agent-panel')
151+
await expect(
152+
panel.locator('button:has-text("graph summary")')
153+
).toBeVisible()
154+
155+
// Ctrl+O expands all
156+
await comfyPage.page.keyboard.press('Control+o')
157+
await expect.poll(() => readTerminalText(comfyPage)).toMatch(/nodes|types/i)
158+
159+
// Ctrl+O folds all back — `nodes:` from the body should be hidden again.
160+
await comfyPage.page.keyboard.press('Control+o')
161+
})
172162
})

src/agent/shell/commands/coreutils.ts

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -109,17 +109,21 @@ const grep: Command = async (ctx) => {
109109
const pattern = ctx.argv[1]
110110
if (!pattern) return err('usage: grep <pattern>')
111111
const re = new RegExp(pattern)
112-
async function* gen(): AsyncIterable<string> {
113-
let matched = false
114-
for await (const line of lines(ctx.stdin)) {
115-
if (re.test(line)) {
116-
yield line + '\n'
117-
matched = true
118-
}
112+
// POSIX grep returns 1 when nothing matched. To honour that we have to
113+
// drain stdin eagerly — exit codes are set on the Command return, but a
114+
// generator can't change them after the fact. The agent relies on this
115+
// for `grep ... && ...` / `grep ... || ...` flows; without the right
116+
// exit code the LLM would conclude evidence existed when stdout was
117+
// actually empty.
118+
let matched = false
119+
let out = ''
120+
for await (const line of lines(ctx.stdin)) {
121+
if (re.test(line)) {
122+
out += line + '\n'
123+
matched = true
119124
}
120-
void matched
121125
}
122-
return ok(gen())
126+
return ok(stringIter(out), matched ? 0 : 1)
123127
}
124128

125129
const trueCmd: Command = async () => ok(emptyIter(), 0)

src/agent/shell/commands/registrySearch.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,12 @@ const nodeSearchRegistry: Command = async (ctx) => {
6363
}
6464
const total = res?.total ?? packs.length
6565
const lines = packs.map(
66-
(p) => packLine(p) + (p.id ? '\n install: install-pack ' + p.id : '')
66+
(p) => packLine(p) + (p.id ? '\n inspect: pack-info ' + p.id : '')
6767
)
6868
const header =
6969
packs.length < total
70-
? `${packs.length} of ${total} pack(s) expose a node matching "${pattern}":\n`
71-
: `${packs.length} pack(s) expose a node matching "${pattern}":\n`
70+
? `${packs.length} of ${total} pack(s) expose a node matching "${pattern}". To install one, ask the user to use ComfyUI-Manager (Settings → Extensions) — there is no shell command for pack installs yet.\n`
71+
: `${packs.length} pack(s) expose a node matching "${pattern}". To install one, ask the user to use ComfyUI-Manager (Settings → Extensions) — there is no shell command for pack installs yet.\n`
7272
return {
7373
stdout: stringIter(header + lines.join('\n') + '\n'),
7474
exitCode: 0

src/agent/stores/agentStore.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,43 @@ const MAX_PERSISTED_MESSAGES = 300
4949
export const useAgentStore = defineStore('agent', () => {
5050
// IndexedDB-backed: survives reloads, larger quota than localStorage,
5151
// doesn't block the main thread like localStorage sync-writes would.
52+
// Note: useIDBKeyval populates `data` asynchronously, so the initial
53+
// `data.value` is `[]` until the read resolves. We seed `messages` with
54+
// whatever's already there (cheap if it's empty) and then hydrate from
55+
// the DB once the read completes — only after that do we enable the
56+
// write-back watcher, otherwise an early in-memory mutation would
57+
// overwrite real persisted history with the empty seed.
5258
const persisted = useIDBKeyval<AgentMessage[]>('Comfy.Agent.Messages', [], {
5359
shallow: false
5460
})
5561
const messages = ref<AgentMessage[]>([...(persisted.data.value ?? [])])
5662

63+
let hydrated = false
64+
watch(
65+
persisted.isFinished,
66+
(done) => {
67+
if (!done || hydrated) return
68+
hydrated = true
69+
const stored = persisted.data.value ?? []
70+
// If the user already typed before the IDB read resolved, prepend
71+
// stored entries so the new ones come last.
72+
if (messages.value.length === 0) {
73+
messages.value = [...stored]
74+
} else if (stored.length > 0) {
75+
messages.value = [...stored, ...messages.value]
76+
}
77+
},
78+
{ immediate: true }
79+
)
80+
5781
// Sync in-memory → persisted (truncated to the cap). Deep watch so edits
58-
// to message text during streaming also flush.
82+
// to message text during streaming also flush. Skip writes until the
83+
// initial DB read has settled, otherwise a pre-hydration mutation
84+
// clobbers the stored history.
5985
watch(
6086
messages,
6187
(next) => {
88+
if (!hydrated) return
6289
persisted.data.value = next.slice(-MAX_PERSISTED_MESSAGES)
6390
},
6491
{ deep: true }

src/agent/ui/FoldablePanel.vue

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -512,7 +512,11 @@ function isDirectShellCommand(line: string): boolean {
512512
const first = line.trim().split(/\s+/)[0]
513513
if (!first) return false
514514
const ctx = session.buildExecContextOnce()
515-
return !!ctx.registry.get(first) || /^[|&;<>/]/.test(first)
515+
// Don't treat a leading '/' as a shell-redirection sigil — the
516+
// attachment flow prefills the composer with paths like '/input/foo.png'
517+
// or '/tmp/x.json' and pressing Enter would route those to exec instead
518+
// of the LLM. Real shell operators (|, &, ;, <, >) are still honoured.
519+
return !!ctx.registry.get(first) || /^[|&;<>]/.test(first)
516520
}
517521
518522
async function handleSubmit(line: string): Promise<void> {

0 commit comments

Comments
 (0)