mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-04 06:29:42 +02:00
fix(response-viewer): role on full-context blocks, divider ReDoS, pi mode (#326 follow-up)
Three post-merge fixes for the external-CLI response viewer:
- ?context=full blocks now carry role ('user' for prompts, 'assistant'
for response/status/tool). The frontend's loadFullContext() renders
via msg.role, so the roleless blocks lost the "You" badge and every
turn rendered as the agent. kind/label/text are unchanged and the
frontend needs no change.
- normalizeDividerStatusLine() dropped its backtracking regex
(/^[─-]+\s*(.+?)\s*[─-]{3,}$/): the lazy middle went catastrophic on
a long dash run without a 3-dash tail (measured 15.5s at 4,000 chars,
minutes at 10,000), and pane text is agent-controlled with buffers up
to 32MB. Replaced by a linear counter walk with the identical accept
set and captured content, pinned char-for-char against the old regex
by a brute-force corpus test plus a hostile-input regression test
that fails by timeout with the RegExp version (same approach as the
glob-matcher hardening in 68ae9a8).
- 'pi' joins EXTERNAL_CLI_MODES: pi sessions had the identical
empty-viewer symptom the transcript branch exists to fix. The list
stays a local duplicate of isExternalCliMode() (importing session.ts
would drag node-pty into the pure module); a new exhaustive parity
test asserts the two mode sets can no longer drift.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,11 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { getLastTranscriptResponse, parseExternalCliTranscript } from '../src/web/response-viewer-transcript.js';
|
||||
import {
|
||||
getLastTranscriptResponse,
|
||||
isExternalCliTranscriptMode,
|
||||
parseExternalCliTranscript,
|
||||
} from '../src/web/response-viewer-transcript.js';
|
||||
import { isExternalCliMode } from '../src/session.js';
|
||||
import type { SessionMode } from '../src/types.js';
|
||||
|
||||
describe('response viewer transcript parser', () => {
|
||||
it('extracts structured COD transcript blocks and keeps labeled status/tool entries', () => {
|
||||
@@ -46,6 +52,15 @@ gpt-5.4 medium · kb · main · Ready · Context 92% left
|
||||
expect(blocks[3]?.label).toBe('Tool');
|
||||
expect(blocks[4]?.text).toContain('reduces the risk and effort involved');
|
||||
expect(blocks[4]?.text).not.toContain('reduces\nthe risk');
|
||||
|
||||
// The frontend's loadFullContext() renders via msg.role: 'user' gets the
|
||||
// "You" badge, everything else the agent badge. A block without role
|
||||
// rendered every prompt as the agent.
|
||||
expect(blocks[1]?.role).toBe('user');
|
||||
expect(blocks[5]?.role).toBe('user');
|
||||
expect(blocks[0]?.role).toBe('assistant');
|
||||
expect(blocks[3]?.role).toBe('assistant');
|
||||
expect(blocks[4]?.role).toBe('assistant');
|
||||
});
|
||||
|
||||
it('returns the most recent completed response when the last block is a new prompt', () => {
|
||||
@@ -320,4 +335,128 @@ Here is the assistant answer at column zero.
|
||||
expect(getLastTranscriptResponse(blocks)).toBe('Here is the assistant answer at column zero.');
|
||||
});
|
||||
});
|
||||
|
||||
// The divider-status detector used to be /^[─-]+\s*(.+?)\s*[─-]{3,}$/, whose
|
||||
// lazy middle backtracked catastrophically on a long dash run that does not
|
||||
// end in 3+ dashes (measured >2min at 8,000 chars). Pane text is
|
||||
// agent-controlled and buffers reach 32MB, so the pattern was replaced by a
|
||||
// linear counter walk. Same approach as the glob-matcher ReDoS fix (68ae9a8):
|
||||
// the hostile input below fails by timeout with the RegExp version.
|
||||
describe('divider-status ReDoS hardening', () => {
|
||||
it('classifies a hostile 10k dash run in linear time', () => {
|
||||
const hostile = '-'.repeat(10_000) + '>';
|
||||
const started = Date.now();
|
||||
const blocks = parseExternalCliTranscript(`› q\n${hostile}`, 'opencode');
|
||||
const elapsed = Date.now() - started;
|
||||
|
||||
expect(elapsed).toBeLessThan(2_000);
|
||||
// No 3-dash tail, so the line is not a status divider — it stays prose.
|
||||
expect(blocks.some((b) => b.kind === 'response' && b.text.includes('>'))).toBe(true);
|
||||
expect(blocks.some((b) => b.kind === 'status')).toBe(false);
|
||||
});
|
||||
|
||||
// The counter walk must keep the EXACT accept set and captured content of
|
||||
// the old regex (including its backtracking quirks on all-dash lines), so
|
||||
// brute-force both against a corpus. The old regex is safe here: probes are
|
||||
// capped at 24 chars, far below the blowup threshold.
|
||||
it('matches the old regex char-for-char on a brute-force corpus', () => {
|
||||
const oldNormalize = (line: string): string | null => {
|
||||
const trimmed = line.trim();
|
||||
const matched = trimmed.match(/^[─-]+\s*(.+?)\s*[─-]{3,}$/);
|
||||
if (!matched) return null;
|
||||
return matched[1]?.trim() || null;
|
||||
};
|
||||
|
||||
// Observe the private normalizeDividerStatusLine through the parser: after
|
||||
// a prompt, a column-zero probe from this alphabet is a status block iff
|
||||
// the divider detector matched, and the status text is its return value.
|
||||
// (The alphabet triggers no other status arm; letters are limited to 'x'.)
|
||||
const observedStatusText = (probe: string): string | null => {
|
||||
const blocks = parseExternalCliTranscript(`› q\n${probe}`, 'opencode');
|
||||
const status = blocks.find((b) => b.kind === 'status');
|
||||
return status ? status.text : null;
|
||||
};
|
||||
|
||||
const directed = [
|
||||
'──── Status ────',
|
||||
'─ Worked for 1m 51s ───────',
|
||||
'---',
|
||||
'----',
|
||||
'-----',
|
||||
'------',
|
||||
'-------',
|
||||
'---- ---',
|
||||
'- ---',
|
||||
'--x',
|
||||
'x---',
|
||||
'----x',
|
||||
'----x--',
|
||||
'----x---',
|
||||
'─x───',
|
||||
'- x ---',
|
||||
'--- x -',
|
||||
'─── ──',
|
||||
'-\tx\t---',
|
||||
'--->----',
|
||||
'─-─- x x ─-─',
|
||||
'- x x x ----',
|
||||
];
|
||||
|
||||
// Deterministic LCG so a failure reproduces byte-identically.
|
||||
let seed = 0x2f6e2b1;
|
||||
const rand = () => {
|
||||
seed = (seed * 1103515245 + 12345) & 0x7fffffff;
|
||||
return seed / 0x80000000;
|
||||
};
|
||||
const alphabet = ['-', '-', '─', '─', ' ', ' ', '\t', 'x', '>'];
|
||||
const probes = [...directed];
|
||||
for (let i = 0; i < 3000; i += 1) {
|
||||
const len = Math.floor(rand() * 25);
|
||||
let probe = '';
|
||||
for (let j = 0; j < len; j += 1) {
|
||||
probe += alphabet[Math.floor(rand() * alphabet.length)];
|
||||
}
|
||||
probes.push(probe);
|
||||
}
|
||||
|
||||
for (const raw of probes) {
|
||||
const probe = raw.trim();
|
||||
// Divider-only lines (8+ dash/whitespace chars) are consumed by
|
||||
// isDividerOnlyLine before the status detector ever runs, in both the
|
||||
// old and new worlds — the detector is unobservable there.
|
||||
if (!probe || /^[\s─-]{8,}$/.test(probe)) continue;
|
||||
expect(observedStatusText(probe), `probe: ${JSON.stringify(probe)}`).toBe(oldNormalize(probe));
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// EXTERNAL_CLI_MODES is a local duplicate of isExternalCliMode() in
|
||||
// src/session.ts (importing session.ts would drag node-pty and the whole
|
||||
// session layer into the pure transcript module). This pin is what keeps the
|
||||
// two from drifting — 'pi' was missing here while isExternalCliMode() had it,
|
||||
// which left pi sessions with the exact empty-viewer symptom the transcript
|
||||
// branch exists to fix.
|
||||
describe('mode parity with isExternalCliMode()', () => {
|
||||
// Exhaustive by construction: adding a SessionMode without deciding its
|
||||
// transcript behavior fails to compile here.
|
||||
const ALL_SESSION_MODES: Record<SessionMode, true> = {
|
||||
claude: true,
|
||||
shell: true,
|
||||
opencode: true,
|
||||
codex: true,
|
||||
gemini: true,
|
||||
antigravity: true,
|
||||
pi: true,
|
||||
};
|
||||
|
||||
it('agrees with isExternalCliMode() for every SessionMode', () => {
|
||||
for (const mode of Object.keys(ALL_SESSION_MODES) as SessionMode[]) {
|
||||
expect(isExternalCliTranscriptMode(mode), `mode: ${mode}`).toBe(isExternalCliMode(mode));
|
||||
}
|
||||
});
|
||||
|
||||
it('covers pi', () => {
|
||||
expect(isExternalCliTranscriptMode('pi')).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4,12 +4,14 @@
|
||||
* Uses app.inject() — no real HTTP ports needed.
|
||||
* Port: N/A (app.inject doesn't open ports)
|
||||
*
|
||||
* OpenCode / Gemini / Antigravity render their own TUIs and never write a Claude
|
||||
* transcript under ~/.claude/projects, so before this branch existed the handler
|
||||
* fell through to the Claude scan, found nothing, and the response viewer was
|
||||
* permanently empty for those modes. These tests pin:
|
||||
* OpenCode / Gemini / Antigravity / Pi render their own TUIs and never write a
|
||||
* Claude transcript under ~/.claude/projects, so before this branch existed the
|
||||
* handler fell through to the Claude scan, found nothing, and the response viewer
|
||||
* was permanently empty for those modes. These tests pin:
|
||||
* - the pane buffer is segmented and the LAST response is returned
|
||||
* - ?context=full carries the parsed blocks, and the short form omits them
|
||||
* - ?context=full blocks carry role — the frontend renders via msg.role, so a
|
||||
* block without it lost the "You" badge on prompts
|
||||
* - a pane that has produced no output reports hasContext: false rather than 404ing
|
||||
* - Claude mode still takes the Claude path (regression guard)
|
||||
*/
|
||||
@@ -100,7 +102,7 @@ describe('GET /api/sessions/:id/last-response — external CLI panes', () => {
|
||||
return { res, body: JSON.parse(res.body) };
|
||||
}
|
||||
|
||||
for (const mode of ['opencode', 'gemini', 'antigravity'] as const) {
|
||||
for (const mode of ['opencode', 'gemini', 'antigravity', 'pi'] as const) {
|
||||
it(`returns the last assistant response from the ${mode} pane buffer`, async () => {
|
||||
session.mode = mode;
|
||||
session.terminalBuffer = PANE;
|
||||
@@ -131,6 +133,12 @@ describe('GET /api/sessions/:id/last-response — external CLI panes', () => {
|
||||
const prompts = body.data.messages.filter((b: { kind: string }) => b.kind === 'prompt');
|
||||
expect(prompts).toHaveLength(2);
|
||||
expect(prompts[1].text).toContain('now document it');
|
||||
|
||||
// loadFullContext() renders via msg.role — without it every block got the
|
||||
// agent badge and the user's own prompts lost their "You" attribution.
|
||||
for (const block of body.data.messages as Array<{ kind: string; role: string }>) {
|
||||
expect(block.role).toBe(block.kind === 'prompt' ? 'user' : 'assistant');
|
||||
}
|
||||
});
|
||||
|
||||
it('reports hasContext false for a pane that has produced no output', async () => {
|
||||
|
||||
Reference in New Issue
Block a user