Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions client/src/components/cos/tabs/AgentCard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,15 @@ export default function AgentCard({ agent, onPause, onKill, onDelete, onResume,
), [inactive, fullOutput, liveOutput, agent.output]);
const lastOutput = output.length > 0 ? output[output.length - 1]?.line : null;

// Why this run has NO "Open Shell" link. Scoped to the one case where the user
// configured a TUI provider and got no shell anyway: a public-review stage is
// forced headless regardless (`spawnHeadless = publicReview || !isTui`). An
// ordinary headless CLI agent gets no chip — nobody expected a shell there, and
// one on every card would be the noise this exists to remove.
const noShellReason = !agent.metadata?.tuiSessionId && agent.metadata?.publicReviewPosture
? 'No shell: a public-review stage always runs headless, even when you configure it onto a TUI provider — the screened PR content stays inside the sandboxed child. Watch the live output below to see what it is doing.'
: null;

// Extract recent tool activity (last few tool lines) for live display
const recentActivity = useMemo(() => {
if (inactive || output.length === 0) return [];
Expand Down Expand Up @@ -616,6 +625,15 @@ export default function AgentCard({ agent, onPause, onKill, onDelete, onResume,
<span className="font-mono text-[10px] text-port-on-success">{agent.metadata.tuiSessionId.slice(0, 6)}</span>
</Link>
)}
{!inactive && noShellReason && (
<span
className="flex items-center gap-1 px-2 py-0.5 rounded bg-port-border/40 text-gray-400 whitespace-nowrap"
title={noShellReason}
>
<Terminal size={10} aria-hidden="true" className="shrink-0" />
<span>No shell</span>
</span>
)}
{!remote && (
<button
onClick={openPromptModal}
Expand Down
48 changes: 48 additions & 0 deletions client/src/components/cos/tabs/AgentCard.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -368,3 +368,51 @@ describe('AgentCard task description (#4170)', () => {
expect(screen.queryByRole('button', { name: /Show more/ })).not.toBeInTheDocument();
});
});

describe('AgentCard missing shell explanation', () => {
const running = (metadata) => ({
...agent,
status: 'running',
completedAt: null,
metadata: { ...agent.metadata, ...metadata },
});

const renderCard = (a) => render(
<MemoryRouter>
<AgentCard agent={a} />
</MemoryRouter>
);

it('says why a public-review stage has no shell instead of leaving the card silent', () => {
// These stages are forced headless even on a TUI provider, so the "Open
// Shell" link never appears. Silence made a slow run look wedged with
// nowhere to look.
renderCard(running({ publicReviewPosture: 'sandboxed-actions', executionMode: 'direct' }));

expect(screen.queryByText('Open Shell')).not.toBeInTheDocument();
expect(screen.getByText('No shell')).toBeInTheDocument();
expect(screen.getByTitle(/public-review stage always runs headless/)).toBeInTheDocument();
});

it('stays silent on a TUI run that has not registered its session yet', () => {
// `executionMode` is stamped at registration but `tuiSessionId` only lands
// once the PTY attaches, so every healthy TUI spawn passes through this
// state — a chip here would put the "looks wedged" noise straight back.
renderCard(running({ executionMode: 'tui', phase: 'initializing' }));

expect(screen.queryByText('No shell')).not.toBeInTheDocument();
});

it('adds no chip to an ordinary headless CLI agent — none was ever expected', () => {
renderCard(running({ executionMode: 'direct' }));

expect(screen.queryByText('No shell')).not.toBeInTheDocument();
});

it('links to the live shell, with no explanation chip, once a session exists', () => {
renderCard(running({ executionMode: 'tui', tuiSessionId: 'sess-abcdef123' }));

expect(screen.getByText('Open Shell')).toBeInTheDocument();
expect(screen.queryByText('No shell')).not.toBeInTheDocument();
});
});
25 changes: 21 additions & 4 deletions server/cos-runner/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { commandExists } from '../lib/commandExists.js';
import { adoptNpmGlobalBinDir } from '../lib/npmGlobalBin.js';
import { findCommandOnPath } from '../lib/processEnv.js';
import { createCodexStderrFormatter } from '../lib/codexCliOutput.js';
import { createStreamingAnsiStripper } from '../lib/ansiStrip.js';
import { createStreamJsonParser } from './streamJsonParser.js';
import { loadState, saveState, withState } from './runnerState.js';
import { getProcessStats, checkProcessRunning } from './processStats.js';
Expand Down Expand Up @@ -504,6 +505,13 @@ app.post('/spawn', async (req, res) => {
rawStreamBuffer: '',
streamParser,
codexStderrFormatter,
// Per-stream ANSI strippers, mirroring spawnDirectly's headless path — the
// same providers land here when the run is dispatched through the runner, so
// without these the `[stderr] [0m` noise reproduces on that half of the
// dispatch matrix. Stateful: each buffers an escape split across two chunks.
// A stream-json stdout is NDJSON with no raw ESC byte, so it skips the scan.
stripStdoutAnsi: isStreamJson ? (text) => text : createStreamingAnsiStripper(),
stripStderrAnsi: createStreamingAnsiStripper(),
workspacePath: cwd
});

Expand All @@ -518,8 +526,8 @@ app.post('/spawn', async (req, res) => {

// Handle stdout
claudeProcess.stdout.on('data', (data) => {
const text = data.toString();
const agent = activeAgents.get(agentId);
const text = agent?.stripStdoutAnsi ? agent.stripStdoutAnsi(data.toString()) : data.toString();

if (agent?.streamParser) {
// Parse stream-json and emit extracted text lines (cap buffer at 512KB for error analysis)
Expand All @@ -533,7 +541,11 @@ app.post('/spawn', async (req, res) => {
emitToServer('agent:output', { agentId, text: line + '\n' });
}
} else {
// Non-stream providers: emit raw stdout as before
// Non-stream providers: emit stdout as-is once decolored. A chunk that was
// purely terminal control has nothing left to show. Unlike stderr below,
// whitespace is NOT dropped here — a blank line is legitimate formatting
// when it isn't wearing an `[stderr]` tag.
if (!text) return;
if (agent) {
agent.outputBuffer += text;
}
Expand All @@ -544,16 +556,21 @@ app.post('/spawn', async (req, res) => {
// Handle stderr
claudeProcess.stderr.on('data', (data) => {
const agent = activeAgents.get(agentId);
const decolored = agent?.stripStderrAnsi ? agent.stripStderrAnsi(data.toString()) : data.toString();
if (agent?.codexStderrFormatter) {
const lines = agent.codexStderrFormatter.processChunk(data.toString());
const lines = agent.codexStderrFormatter.processChunk(decolored);
for (const line of lines) {
agent.outputBuffer += line + '\n';
emitToServer('agent:output', { agentId, text: line + '\n' });
}
return;
}

const text = `[stderr] ${data.toString()}`;
// A chunk that decolors down to whitespace was pure terminal control
// (`opencode run` emits a bare reset per progress redraw). Tagging it
// `[stderr]` would add one blank noise line to the tail per redraw.
if (!decolored.trim()) return;
const text = `[stderr] ${decolored}`;
if (agent) agent.outputBuffer += text;
emitToServer('agent:output', { agentId, text });
});
Expand Down
37 changes: 37 additions & 0 deletions server/cos-runner/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,40 @@ describe('cos-runner durable TUI ownership (#3202)', () => {
expect(RUNNER_SRC).toMatch(/io\.emit\('tui:exit',[\s\S]{0,350}?\.\.\.\(outputTail \? \{ outputTail \} : \{\}\)/);
});
});

// The runner is the second headless spawner: the same providers land here when a
// task is dispatched with `useRunner`, and its stdout/stderr handlers are a twin
// of spawnDirectly's. `opencode run` colors its progress line, and those raw
// escapes reached the agent card as `[stderr] [0m` noise — a working agent read
// as a wedged one. The behavioral coverage lives in agentCliSpawning.test.js
// against the real handlers; this pins that the runner twin was not left behind.
describe('cos-runner output — ANSI decoloring parity with spawnDirectly', () => {
it('imports the shared streaming stripper rather than rolling its own regex', () => {
expect(RUNNER_SRC).toMatch(
/import\s*\{[^}]*\bcreateStreamingAnsiStripper\b[^}]*\}\s*from\s*'\.\.\/lib\/ansiStrip\.js';/
);
});

it('gives each agent its own stdout/stderr strippers, skipping NDJSON stdout', () => {
// Per agent, not per module: the stripper buffers an escape split across two
// chunks, so one shared instance would interleave two agents' tails.
expect(RUNNER_SRC).toMatch(
/stripStdoutAnsi:\s*isStreamJson\s*\?\s*\(text\)\s*=>\s*text\s*:\s*createStreamingAnsiStripper\(\)/
);
expect(RUNNER_SRC).toMatch(/stripStderrAnsi:\s*createStreamingAnsiStripper\(\)/);
});

it('decolors both streams before they reach the buffer, formatter, or socket', () => {
expect(RUNNER_SRC).toMatch(/agent\?\.stripStdoutAnsi\s*\?\s*agent\.stripStdoutAnsi\(data\.toString\(\)\)/);
expect(RUNNER_SRC).toMatch(/agent\?\.stripStderrAnsi\s*\?\s*agent\.stripStderrAnsi\(data\.toString\(\)\)/);
// The codex formatter must see the decolored text too — it matches prose that
// an SGR pair would otherwise split.
expect(RUNNER_SRC).toMatch(/codexStderrFormatter\.processChunk\(decolored\)/);
// And the `[stderr]` tag is built from the decolored chunk, never the raw one.
expect(RUNNER_SRC).toMatch(/const text = `\[stderr\] \$\{decolored\}`/);
});

it('drops a chunk that was only terminal control instead of emitting a blank line', () => {
expect(RUNNER_SRC).toMatch(/if \(!decolored\.trim\(\)\) return;/);
});
});
35 changes: 30 additions & 5 deletions server/services/agentCliSpawning.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { normalizeReviewers } from '../lib/validation.js';
import { resolveReviewLoopOptions } from './codeReview.js';
import { safeJSONParse, PATHS } from '../lib/fileUtils.js';
import { createCodexStderrFormatter } from '../lib/codexCliOutput.js';
import { createStreamingAnsiStripper } from '../lib/ansiStrip.js';
import { PROVIDER_TYPES } from '../lib/aiToolkit/constants.js';
import { createImmediateFallbackSignalDetector } from '../lib/aiToolkit/errorDetection.js';
import { prepareCliPrompt } from '../lib/cliProviderArgs.js';
Expand Down Expand Up @@ -533,6 +534,18 @@ export async function spawnDirectly({
const isStreamJson = cliConfig.streamFormat === 'stream-json';
const streamParser = isStreamJson ? createStreamJsonParser() : null;
const codexStderrFormatter = provider.id === 'codex' ? createCodexStderrFormatter(prompt) : null;
// A headless CLI still colors its progress output: `opencode run` emits bare
// `\x1B[0m` resets around its status line. Those bytes reached output.txt and
// the live tail verbatim, and the browser drops only the ESC itself — so the
// card rendered `[stderr] [0m` and a working agent was indistinguishable from
// a wedged one. Strip per stream (each stripper is stateful, buffering an
// escape split across two `data` chunks) BEFORE the fallback-signal detector,
// which matches on provider text that color codes would otherwise break up.
// A stream-json stdout is NDJSON, which escapes an ESC into its six-character
// JSON form and never carries the raw byte — so the busiest stdout stream in
// the system skips the scan entirely.
const stripStdoutAnsi = isStreamJson ? (text) => text : createStreamingAnsiStripper();
const stripStderrAnsi = createStreamingAnsiStripper();
let immediateFallbackAnalysis = null;
const detectImmediateFallbackSignal = createImmediateFallbackSignalDetector();

Expand Down Expand Up @@ -610,15 +623,18 @@ export async function spawnDirectly({

handleStdout = (data) => {
try {
const text = data.toString();
const raw = data.toString();
const text = stripStdoutAnsi(raw);
// Detect fallback signals SYNCHRONOUSLY, before enqueuing any transcript
// mutation — a blocked earlier write must never delay killing the provider
// on a usage-limit signal (#2384).
stopForImmediateFallbackSignal(text);
// Serialize the transcript body so two `data` events can't interleave their
// awaits and reorder output.txt / the batched live tail.
enqueueTranscriptWrite(async () => {
await recordFirstOutput('cli-stdout', text.length);
// Raw length, not stripped: a chunk that was ONLY color codes is still
// proof the child is alive, and reporting 0 would read as "no output yet".
await recordFirstOutput('cli-stdout', raw.length);
if (!hasStartedWorking) {
hasStartedWorking = true;
await updateAgent(agentId, { metadata: { phase: 'working' } });
Expand All @@ -636,7 +652,11 @@ export async function spawnDirectly({
outputBatcher.push(lines);
await writeFile(outputFile, outputBuffer).catch(() => {});
} else {
// Non-stream providers: emit raw stdout as before
// Non-stream providers: emit stdout as-is once decolored. A chunk that
// was purely terminal control has nothing left to show. Unlike stderr
// below, whitespace is NOT dropped here — a blank line is legitimate
// formatting when it isn't wearing an `[stderr]` tag.
if (!text) return;
outputBuffer += text;
await writeFile(outputFile, outputBuffer).catch(() => {});
outputBatcher.push(text);
Expand All @@ -649,11 +669,12 @@ export async function spawnDirectly({

handleStderr = (data) => {
try {
const text = data.toString();
const raw = data.toString();
const text = stripStderrAnsi(raw);
// Synchronous fallback detection before the serialized write (see stdout).
stopForImmediateFallbackSignal(`[stderr] ${text}`);
enqueueTranscriptWrite(async () => {
await recordFirstOutput('cli-stderr', text.length);
await recordFirstOutput('cli-stderr', raw.length);
// Codex stderr: show thinking + tool names, skip config dump and command output
if (codexStderrFormatter) {
const lines = codexStderrFormatter.processChunk(text);
Expand All @@ -662,6 +683,10 @@ export async function spawnDirectly({
await writeFile(outputFile, outputBuffer).catch(() => {});
return;
}
// A chunk that decolors down to whitespace was pure terminal control
// (`opencode run` emits a bare reset per progress redraw). Tagging it
// `[stderr]` would add one blank noise line to the tail per redraw.
if (!text.trim()) return;
outputBuffer += `[stderr] ${text}`;
await writeFile(outputFile, outputBuffer).catch(() => {});
outputBatcher.push(`[stderr] ${text}`);
Expand Down
101 changes: 101 additions & 0 deletions server/services/agentCliSpawning.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,107 @@ describe('stream error containment', () => {
expect(usageIdx).toBeGreaterThan(firstIdx);
});

// A headless CLI still colors its progress feed. `opencode run` wraps its
// status line in bare SGR resets, and those bytes used to reach the transcript
// verbatim — the browser drops only the ESC, so the agent card rendered
// `[stderr] [0m` and a working agent read as a wedged one.
describe('ANSI decoloring of headless CLI output', () => {
const textArgs = () => ({
...minimalArgs,
cliConfig: { command: 'opencode', args: ['run'], stdinMode: 'prompt', streamFormat: 'text' },
});

const emittedLines = () =>
agentStateMocks.appendAgentOutputLines.mock.calls.flatMap(([, batch]) => batch);

// The shared mock accumulates across the whole file; these assertions are
// about the exact transcript ONE run produced.
beforeEach(() => agentStateMocks.appendAgentOutputLines.mockClear());

it('strips color codes from stdout and stderr instead of leaking them into the transcript', async () => {
const spawnPromise = spawnDirectly(textArgs());
await new Promise((r) => setTimeout(r, 10));

fakeProcess.stdout.emit('data', Buffer.from('\x1B[32mbuilding\x1B[0m\n'));
fakeProcess.stderr.emit('data', Buffer.from('\x1B[0m> build · some-model\n'));
await new Promise((r) => setTimeout(r, 30));

fakeProcess.emit('close', 0);
await spawnPromise.catch(() => {});

const lines = emittedLines();
expect(lines).toContain('building\n');
expect(lines).toContain('[stderr] > build · some-model\n');
expect(lines.join('')).not.toMatch(/\x1B|\[0m|\[32m/);
});

it('reassembles an escape sequence split across two chunks rather than leaking its tail', async () => {
const spawnPromise = spawnDirectly(textArgs());
await new Promise((r) => setTimeout(r, 10));

// The reset straddles the chunk boundary: a per-chunk strip would emit `2m…`.
fakeProcess.stdout.emit('data', Buffer.from('ready\x1B['));
await new Promise((r) => setTimeout(r, 20));
fakeProcess.stdout.emit('data', Buffer.from('32mgreen\n'));
await new Promise((r) => setTimeout(r, 20));

fakeProcess.emit('close', 0);
await spawnPromise.catch(() => {});

expect(emittedLines().join('')).toBe('readygreen\n');
});

it('drops a chunk that was ONLY terminal control rather than tagging a blank [stderr] line', async () => {
const spawnPromise = spawnDirectly(textArgs());
await new Promise((r) => setTimeout(r, 10));

fakeProcess.stderr.emit('data', Buffer.from('\x1B[0m'));
fakeProcess.stdout.emit('data', Buffer.from('\x1B[0m'));
await new Promise((r) => setTimeout(r, 30));

fakeProcess.emit('close', 0);
await spawnPromise.catch(() => {});

expect(emittedLines()).not.toContain('[stderr] ');
expect(emittedLines().some((line) => line.trim() === '')).toBe(false);
});

it('still records a colors-only chunk as run output — it is proof the child is alive', async () => {
// Counting the DECOLORED length would report zero bytes and file a run that
// was steadily redrawing its progress line as having produced nothing.
appendRunEvent.mockClear();
const spawnPromise = spawnDirectly(textArgs());
await new Promise((r) => setTimeout(r, 10));

fakeProcess.stderr.emit('data', Buffer.from('\x1B[0m'));
await new Promise((r) => setTimeout(r, 30));

const outputs = appendRunEvent.mock.calls.map(([e]) => e).filter((e) => e.kind === 'run.output');
expect(outputs).toHaveLength(1);
expect(outputs[0].data).toMatchObject({ source: 'cli-stderr' });

fakeProcess.emit('close', 0);
await spawnPromise.catch(() => {});
});

it('detects a fallback signal that the CLI printed with color codes inside it', async () => {
// The detector matches provider prose; an SGR pair mid-sentence used to
// split the phrase and let a usage-limit signal through unnoticed.
killProcessTree.mockClear();
const spawnPromise = spawnDirectly(textArgs());
await new Promise((r) => setTimeout(r, 10));

fakeProcess.stdout.emit('data', Buffer.from('\x1B[33mNow using \x1B[1mextra usage\x1B[0m\n'));

expect(killProcessTree).toHaveBeenCalledTimes(1);
fakeProcess.killed = true;

await new Promise((r) => setTimeout(r, 20));
fakeProcess.emit('close', 143);
await spawnPromise.catch(() => {});
});
});

it('routes the CLI command through prepareCliSpawn and spawns its resolved+wrapped result (#2243)', async () => {
// The reported bug: on Windows a bare `opencode`/`claude` .cmd shim can't be
// spawned directly under shell:false → ENOENT (-4058) → startup-failure.
Expand Down
Loading