diff --git a/.changeset/cli-agent-review-fixes.md b/.changeset/cli-agent-review-fixes.md new file mode 100644 index 0000000000..c34b449493 --- /dev/null +++ b/.changeset/cli-agent-review-fixes.md @@ -0,0 +1,25 @@ +--- +"@runfusion/fusion": patch +--- + +Fix a batch of CLI Agent Executor review defects: + +- **Schema-version gate**: bump `SCHEMA_VERSION` to 110 so a DB already at 109 + runs migration 110 and gains the `chat_sessions.cliExecutorAdapterId` column + (it was previously short-circuited). Add the column to the compat-fingerprint + `MIGRATION_ONLY_TABLE_SCHEMAS.chat_sessions` entry so the fingerprint matches. +- **Generic adapter double-wrap**: `formatInjection` no longer re-wraps injected + text in bracketed-paste markers when `bracketedPasteActive`; the session + manager's security path is the sole wrapper, so the generic adapter (like every + native one) only appends a carriage return. +- **Output-filter cross-boundary bypass**: thread one carry buffer across the + scrollback→live seam in the CLI session WS bridge so a dangerous escape (e.g. + OSC 52) split across the seam is fully neutralized instead of the held + introducer being flushed verbatim into the scrollback frame. +- **Output-filter overflow leak**: when an over-length carry begins with a + recognized dangerous introducer (OSC `ESC ]` / DCS `ESC P`), drop the + introducer instead of flushing it as literal, so it cannot recombine with a + later terminator at the client. +- **Follow-up never resolves**: `followUp()` now drives the authoritative state + machine `done→busy` before injecting, so the re-armed result promise resolves + on the next positive `done` instead of hanging on an idempotent done. diff --git a/packages/core/src/__tests__/db-migrate.test.ts b/packages/core/src/__tests__/db-migrate.test.ts index 30407179a4..2adac04a10 100644 --- a/packages/core/src/__tests__/db-migrate.test.ts +++ b/packages/core/src/__tests__/db-migrate.test.ts @@ -715,7 +715,7 @@ describe("schema migration", () => { const row = db.prepare("SELECT deletedAt FROM tasks WHERE id = 'FN-legacy'").get() as { deletedAt: string | null }; expect(row.deletedAt).toBeNull(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -748,7 +748,7 @@ describe("schema migration", () => { { id: "WS-001", mode: "prompt", gateMode: "advisory" }, { id: "WS-002", mode: "script", gateMode: "advisory" }, ]); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -798,7 +798,7 @@ describe("schema migration", () => { reviewerContextRetryCount: 0, reviewerFallbackRetryCount: 0, }); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -827,7 +827,7 @@ describe("schema migration", () => { const columns = db.prepare("PRAGMA table_info(milestones)").all() as Array<{ name: string }>; expect(columns.map((column) => column.name)).toContain("acceptanceCriteria"); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -868,7 +868,7 @@ describe("schema migration", () => { const missionColumns = db.prepare("PRAGMA table_info(missions)").all() as Array<{ name: string }>; expect(missionColumns.map((column) => column.name)).toContain("autoMerge"); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -902,7 +902,7 @@ describe("schema migration", () => { { id: "WS-002", mode: "script", enabled: 1, gateMode: "advisory" }, { id: "WS-003", mode: "prompt", enabled: 0, gateMode: "advisory" }, ]); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -939,7 +939,7 @@ describe("schema migration", () => { const indexes = db.prepare("PRAGMA index_list(mission_goals)").all() as Array<{ name: string }>; expect(indexes.some((index) => index.name === "idxMissionGoalsGoalId")).toBe(true); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -1000,7 +1000,7 @@ describe("schema migration", () => { expect(customFieldsColumn).toBeDefined(); expect(customFieldsColumn?.dflt_value).toBe("'{}'"); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -1053,7 +1053,39 @@ describe("schema migration", () => { expect(indexNames).toContain("idx_cli_sessions_chatSessionId"); expect(indexNames).toContain("idx_cli_sessions_project_state"); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); + db.close(); + }); + + it("adds cliExecutorAdapterId to chat_sessions when migrating from schema version 109", () => { + const db = new Database(fusionDir); + db.exec("CREATE TABLE IF NOT EXISTS __meta (key TEXT PRIMARY KEY, value TEXT)"); + db.exec("INSERT INTO __meta (key, value) VALUES ('schemaVersion', '109')"); + db.exec("INSERT INTO __meta (key, value) VALUES ('lastModified', '1000')"); + db.exec(` + CREATE TABLE IF NOT EXISTS chat_sessions ( + id TEXT PRIMARY KEY, + agentId TEXT NOT NULL, + title TEXT, + status TEXT NOT NULL DEFAULT 'active', + projectId TEXT, + modelProvider TEXT, + modelId TEXT, + createdAt TEXT NOT NULL, + updatedAt TEXT NOT NULL, + cliSessionFile TEXT, + inFlightGeneration TEXT + ) + `); + + db.init(); + + const columns = db + .prepare("PRAGMA table_info(chat_sessions)") + .all() as Array<{ name: string }>; + expect(columns.map((column) => column.name)).toContain("cliExecutorAdapterId"); + + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -1063,7 +1095,7 @@ describe("schema migration", () => { const tables = db.prepare("SELECT name FROM sqlite_master WHERE type='table'").all() as Array<{ name: string }>; expect(tables.map((row) => row.name)).toContain("cli_sessions"); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); }); diff --git a/packages/core/src/__tests__/db.test.ts b/packages/core/src/__tests__/db.test.ts index 191b67591a..974e0be1c2 100644 --- a/packages/core/src/__tests__/db.test.ts +++ b/packages/core/src/__tests__/db.test.ts @@ -334,7 +334,7 @@ describe("Database", () => { }); it("seeds schema version", () => { - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); }); it("includes tokenUsageCacheWriteTokens on freshly initialized tasks table", () => { @@ -393,7 +393,7 @@ describe("Database", () => { it("is idempotent - calling init() twice does not fail", () => { expect(() => db.init()).not.toThrow(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); }); it("does not overwrite existing config on re-init", () => { // Update the config @@ -1463,7 +1463,7 @@ describe("schema migrations", () => { db.init(); // Verify version bumped to 29 (includes v1→v2 through v26→v29) - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); // Verify new columns exist and existing data is intact const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; @@ -1488,11 +1488,11 @@ describe("schema migrations", () => { const db = new Database(fusionDir); db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); // Re-init should not fail db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); db.close(); }); @@ -1527,7 +1527,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; expect(cols.map((col) => col.name)).toContain("priority"); @@ -1568,7 +1568,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; const colNames = cols.map((col) => col.name); @@ -1640,7 +1640,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; const colNames = cols.map((col) => col.name); @@ -1880,7 +1880,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); const cols = db.prepare("PRAGMA table_info(chat_messages)").all() as Array<{ name: string }>; expect(cols.map((col) => col.name)).toContain("attachments"); @@ -1954,7 +1954,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); const tables = db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'agentRatings'").all() as Array<{ name: string }>; expect(tables).toEqual([{ name: "agentRatings" }]); @@ -1978,7 +1978,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); const tables = db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'mission_events'").all() as Array<{ name: string }>; expect(tables).toEqual([{ name: "mission_events" }]); @@ -2082,7 +2082,7 @@ describe("schema migrations", () => { db.init(); // Verify version bumped to 29 - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); // Verify new columns exist and existing data is intact const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; @@ -2301,7 +2301,7 @@ describe("schema migrations", () => { localDb.init(); - expect(localDb.getSchemaVersion()).toBe(109); + expect(localDb.getSchemaVersion()).toBe(110); const columns = localDb.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; expect(columns.map((column) => column.name)).toContain("tokenUsageCacheWriteTokens"); @@ -2612,7 +2612,7 @@ describe("createDatabase factory", () => { const db = createDatabase(fusionDir); db.init(); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); expect(db.getLastModified()).toBeGreaterThan(0); db.close(); @@ -2766,7 +2766,7 @@ describe("migration v77 task token budget columns", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(109); + expect(migrated.getSchemaVersion()).toBe(110); const rows = migrated.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; const names = new Set(rows.map((row) => row.name)); expect(names.has("tokenBudgetSoftAlertedAt")).toBe(true); @@ -2797,7 +2797,7 @@ describe("migration v106 adds tasks.transitionPending (FN-1417)", () => { const fresh = new Database(fusion); try { fresh.init(); - expect(fresh.getSchemaVersion()).toBe(109); + expect(fresh.getSchemaVersion()).toBe(110); const names = new Set( (fresh.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>).map((r) => r.name), ); @@ -2825,7 +2825,7 @@ describe("migration v106 adds tasks.transitionPending (FN-1417)", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(109); + expect(migrated.getSchemaVersion()).toBe(110); const names = new Set( (migrated.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>).map((r) => r.name), ); @@ -2851,7 +2851,7 @@ describe("migration v107 adds workflow_run_branches + index (FN-1417)", () => { const fresh = new Database(fusion); try { fresh.init(); - expect(fresh.getSchemaVersion()).toBe(109); + expect(fresh.getSchemaVersion()).toBe(110); const table = fresh .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'workflow_run_branches'") .get() as { name: string } | undefined; @@ -2885,7 +2885,7 @@ describe("migration v107 adds workflow_run_branches + index (FN-1417)", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(109); + expect(migrated.getSchemaVersion()).toBe(110); const table = migrated .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'workflow_run_branches'") .get() as { name: string } | undefined; @@ -2926,7 +2926,7 @@ describe("migration v67 drops orphan project auth tables", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(109); + expect(migrated.getSchemaVersion()).toBe(110); const tables = migrated .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name LIKE 'project_auth_%'") .all() as Array<{ name: string }>; @@ -2953,7 +2953,7 @@ describe("migration v67 drops orphan project auth tables", () => { try { fresh.init(); - expect(fresh.getSchemaVersion()).toBe(109); + expect(fresh.getSchemaVersion()).toBe(110); const tables = fresh .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name LIKE 'project_auth_%'") .all() as Array<{ name: string }>; diff --git a/packages/core/src/__tests__/goals-schema.test.ts b/packages/core/src/__tests__/goals-schema.test.ts index 03398b9515..73d1e70d50 100644 --- a/packages/core/src/__tests__/goals-schema.test.ts +++ b/packages/core/src/__tests__/goals-schema.test.ts @@ -91,6 +91,6 @@ describe("goals schema", () => { }); it("reports schema version 101", () => { - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); }); }); diff --git a/packages/core/src/__tests__/insight-store.test.ts b/packages/core/src/__tests__/insight-store.test.ts index 5d8ac9b47a..e6d5b4bcb1 100644 --- a/packages/core/src/__tests__/insight-store.test.ts +++ b/packages/core/src/__tests__/insight-store.test.ts @@ -1000,7 +1000,7 @@ describe("Migration: pre-33 DB upgrade", () => { // Step 1: Create a fresh database at v33 (runs all migrations up to 33) const db1 = createDatabase(legacyDir); db1.init(); - expect(db1.getSchemaVersion()).toBe(109); + expect(db1.getSchemaVersion()).toBe(110); db1.close(); // Step 2: Manually downgrade to version 32 and drop insight tables @@ -1035,7 +1035,7 @@ describe("Migration: pre-33 DB upgrade", () => { expect(tableNamesBefore).not.toContain("project_insight_runs"); // Now run init — this triggers the v32→v33 migration db3.init(); - expect(db3.getSchemaVersion()).toBe(109); + expect(db3.getSchemaVersion()).toBe(110); // Step 4: Verify insight tables exist after migration const tablesAfter = db3.prepare( @@ -1066,12 +1066,12 @@ describe("Migration: pre-33 DB upgrade", () => { try { const db1 = createDatabase(testDir); db1.init(); - expect(db1.getSchemaVersion()).toBe(109); + expect(db1.getSchemaVersion()).toBe(110); db1.close(); const db2 = createDatabase(testDir); expect(() => db2.init()).not.toThrow(); - expect(db2.getSchemaVersion()).toBe(109); + expect(db2.getSchemaVersion()).toBe(110); db2.close(); } finally { rmSync(testDir, { recursive: true, force: true }); @@ -1085,7 +1085,7 @@ describe("Migration: pre-33 DB upgrade", () => { // Step 1: Create a fresh DB and run migrations const db1 = createDatabase(compatDir); db1.init(); - expect(db1.getSchemaVersion()).toBe(109); + expect(db1.getSchemaVersion()).toBe(110); // Step 2: Strip lifecycle and cancelledAt columns by recreating the // table without them. This simulates a DB that was created before the diff --git a/packages/core/src/__tests__/merge-request-record.test.ts b/packages/core/src/__tests__/merge-request-record.test.ts index 64dc82817b..193e76bf7e 100644 --- a/packages/core/src/__tests__/merge-request-record.test.ts +++ b/packages/core/src/__tests__/merge-request-record.test.ts @@ -38,7 +38,7 @@ describe("TaskStore merge request record + completion handoff marker", () => { .all() as Array<{ name: string }>; expect(tableRows).toEqual([{ name: "completion_handoff_markers" }, { name: "merge_requests" }]); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); }); it("upserts merge request records", async () => { diff --git a/packages/core/src/__tests__/mission-store.test.ts b/packages/core/src/__tests__/mission-store.test.ts index 2ded53b090..18830ed7c9 100644 --- a/packages/core/src/__tests__/mission-store.test.ts +++ b/packages/core/src/__tests__/mission-store.test.ts @@ -3746,7 +3746,7 @@ describe("MissionStore", () => { describe("Loop State & Validator Run Schema (v31)", () => { it("schema version is 101 after migration", () => { - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); }); it("mission_features table has loop state columns", () => { diff --git a/packages/core/src/__tests__/run-audit.test.ts b/packages/core/src/__tests__/run-audit.test.ts index ca428b6914..0b4a7864d3 100644 --- a/packages/core/src/__tests__/run-audit.test.ts +++ b/packages/core/src/__tests__/run-audit.test.ts @@ -584,7 +584,7 @@ describe("Run Audit", () => { }); it("schema version is bumped to 40", () => { - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); }); }); }); diff --git a/packages/core/src/__tests__/store-merge-queue.test.ts b/packages/core/src/__tests__/store-merge-queue.test.ts index 96a9ea905b..ae8bdd7c66 100644 --- a/packages/core/src/__tests__/store-merge-queue.test.ts +++ b/packages/core/src/__tests__/store-merge-queue.test.ts @@ -60,7 +60,7 @@ describe("TaskStore merge queue", () => { expect.arrayContaining(["idx_mergeQueue_lease_ready", "idx_mergeQueue_leaseExpiresAt"]), ); - expect(store.getDatabase().getSchemaVersion()).toBe(109); + expect(store.getDatabase().getSchemaVersion()).toBe(110); }); it("migrates a legacy v88 database and preserves task rows", async () => { diff --git a/packages/core/src/__tests__/task-documents.test.ts b/packages/core/src/__tests__/task-documents.test.ts index 63e5eebfad..33417c08a4 100644 --- a/packages/core/src/__tests__/task-documents.test.ts +++ b/packages/core/src/__tests__/task-documents.test.ts @@ -51,7 +51,7 @@ describe("TaskStore task documents", () => { expect(tableNames.has("task_documents")).toBe(true); expect(tableNames.has("task_document_revisions")).toBe(true); - expect(db.getSchemaVersion()).toBe(109); + expect(db.getSchemaVersion()).toBe(110); const index = db .prepare( diff --git a/packages/core/src/db.ts b/packages/core/src/db.ts index fe49c728ff..c4c824f90a 100644 --- a/packages/core/src/db.ts +++ b/packages/core/src/db.ts @@ -149,7 +149,7 @@ export function probeFts5(db: DatabaseSync): boolean { // ── Schema Definition ──────────────────────────────────────────────── -const SCHEMA_VERSION = 109; +const SCHEMA_VERSION = 110; export { SCHEMA_VERSION }; @@ -1232,6 +1232,7 @@ export const MIGRATION_ONLY_TABLE_SCHEMAS: Record updatedAt: "TEXT NOT NULL", cliSessionFile: "TEXT", inFlightGeneration: "TEXT", + cliExecutorAdapterId: "TEXT", }, cli_sessions: { id: "TEXT PRIMARY KEY", diff --git a/packages/dashboard/src/__tests__/cli-session-output-filter.test.ts b/packages/dashboard/src/__tests__/cli-session-output-filter.test.ts index 8aaafba4b0..e04c9b9e30 100644 --- a/packages/dashboard/src/__tests__/cli-session-output-filter.test.ts +++ b/packages/dashboard/src/__tests__/cli-session-output-filter.test.ts @@ -116,6 +116,23 @@ describe("neutralizeTerminalOutput", () => { expect(r.carry.length).toBeLessThanOrEqual(MAX_CARRY_LENGTH); }); + it("drops (not flushes) an overflowing OSC 52 prefix so it cannot reconstruct across chunks", () => { + // An unterminated OSC 52 grows past MAX_CARRY. The dangerous introducer must + // NOT be emitted as literal — otherwise a terminator in the next chunk would + // recombine at the client into a working OSC 52 clipboard write. + const huge = `${ESC}]52;c;` + "A".repeat(MAX_CARRY_LENGTH + 100); + const r1 = neutralizeTerminalOutput(huge, ""); + // Nothing reconstructable was emitted, and the carry was dropped. + expect(r1.output).not.toContain(`${ESC}]`); + expect(r1.output).not.toContain("52;"); + expect(r1.carry).toBe(""); + // The terminator arriving next has no held introducer to recombine with. + const r2 = neutralizeTerminalOutput(`${BEL}visible`, r1.carry); + const combined = r1.output + r2.output; + expect(combined).not.toContain(`${ESC}]52;`); + expect(r2.output).toContain("visible"); + }); + it("neutralizes a stream with OSC 52, OSC 8 js link, and a DSR query together", () => { const stream = `start${ESC}]52;c;ZXZpbA==${BEL}` + diff --git a/packages/dashboard/src/__tests__/cli-session-ws.test.ts b/packages/dashboard/src/__tests__/cli-session-ws.test.ts index bea912955c..4f1da19587 100644 --- a/packages/dashboard/src/__tests__/cli-session-ws.test.ts +++ b/packages/dashboard/src/__tests__/cli-session-ws.test.ts @@ -472,6 +472,51 @@ describe("cli-session WS attach", () => { ws.close(); }); + it("does not leak an OSC 52 split across the scrollback→live seam", async () => { + const ESC = "\x1b"; + const BEL = "\x07"; + // The OSC 52 introducer lands at the very TAIL of scrollback (unterminated); + // its terminator arrives in the first LIVE chunk. The carry must thread + // across the seam so the neutralizer sees the full sequence and strips it — + // the scrollback frame must NOT flush the held introducer verbatim. + h = await startHarness(); + h.manager.scrollbackById.set("cli-1", `prior-output${ESC}]52;c;ZXZ`); + const t = await mintTicket("cli-1"); + const ws = connect(h.port, { sessionId: "cli-1", ticket: t }); + + const scrollback = await nextMessage(ws, (m) => m.type === "scrollback"); + const scrollText = decode(scrollback.data); + // Normal scrollback still renders; the unterminated tail is withheld (carry). + expect(scrollText).toContain("prior-output"); + expect(scrollText).not.toContain("52;"); + expect(scrollText).not.toContain(`${ESC}]`); + + // Deliver the terminator in the first live chunk. + h.manager.broadcast("cli-1", `pbA==${BEL}after`); + let collected = scrollText; + await vi.waitFor( + () => + new Promise((resolve, reject) => { + const onMsg = (raw: Buffer) => { + const m = JSON.parse(raw.toString()); + if (m.type === "data") collected += decode(m.data); + if (collected.includes("after")) { + ws.off("message", onMsg); + resolve(); + } + }; + ws.on("message", onMsg); + setTimeout(() => reject(new Error("timeout")), 1000); + }), + ); + // The full sequence, reassembled across the seam, was neutralized. + expect(collected).not.toContain("52;"); + expect(collected).not.toContain(`${ESC}]52`); + expect(collected).toContain("prior-output"); + expect(collected).toContain("after"); + ws.close(); + }); + it("read-only session rejects input with an error frame (server-side)", async () => { h = await startHarness({ sessions: [makeSession({ id: "cli-ro", purpose: "validator" })], diff --git a/packages/dashboard/src/cli-session-output-filter.ts b/packages/dashboard/src/cli-session-output-filter.ts index c39391f1ec..6449f2d8d0 100644 --- a/packages/dashboard/src/cli-session-output-filter.ts +++ b/packages/dashboard/src/cli-session-output-filter.ts @@ -220,10 +220,19 @@ export function neutralizeTerminalOutput(chunk: string, carry = ""): NeutralizeR } let newCarry = input.slice(i); - // Bound the carry: if a "sequence" never terminates, don't buffer forever — - // flush it as literal output (it isn't a recognized hazard if it's this long). + // Bound the carry: if a "sequence" never terminates, don't buffer forever. if (newCarry.length > MAX_CARRY_LENGTH) { - out += newCarry; + // If the overflowing carry begins with a recognized dangerous introducer + // (OSC `ESC ]` or DCS `ESC P`), do NOT flush it as literal — emitting the + // raw `ESC ]52;…` prefix would let it recombine with a terminator that + // arrives in a later chunk and reconstruct the hazardous sequence at the + // client. Drop the introducer (and everything held with it) so it can never + // be reassembled. Harmless overflow (anything else) is flushed as before. + if (newCarry.startsWith(`${ESC}]`) || newCarry.startsWith(`${ESC}P`)) { + // Strip the dangerous prefix entirely. + } else { + out += newCarry; + } newCarry = ""; } else if (newCarry.length > 0 && !isIncompleteSequence(newCarry)) { // The residual isn't actually a growing prefix — emit it. diff --git a/packages/dashboard/src/cli-session-ws.ts b/packages/dashboard/src/cli-session-ws.ts index ab578223dc..65a3ec31e1 100644 --- a/packages/dashboard/src/cli-session-ws.ts +++ b/packages/dashboard/src/cli-session-ws.ts @@ -208,13 +208,20 @@ export function setupCliSessionWebSocket( { const scrollText = Buffer.from(attachment.scrollback).toString("utf8"); const result = neutralizeTerminalOutput(scrollText, ""); - // Flush carry into the scrollback frame (replay is a complete snapshot). - const full = result.output + flushTerminalOutput(result.carry); + // Thread the carry across the scrollback→live seam. Do NOT flush the + // scrollback carry verbatim: if a dangerous sequence (e.g. OSC 52) is + // split so its introducer lands at the tail of scrollback and its + // terminator arrives in the first live chunk, flushing the held prefix + // here would let it recombine at the client and reconstruct the hazard. + // Instead we hand the unterminated tail to the live `sendData` carry so + // the neutralizer sees the full sequence and strips it. Only the safe, + // fully-neutralized prefix is sent in the scrollback frame. + carry = result.carry; try { ws.send( JSON.stringify({ type: "scrollback", - data: Buffer.from(full, "utf8").toString("base64"), + data: Buffer.from(result.output, "utf8").toString("base64"), }), ); } catch { @@ -235,6 +242,25 @@ export function setupCliSessionWebSocket( } catch { /* stream error — close below */ } + // Flush any residual carry at true stream end. The held bytes are an + // unterminated tail; no further chunk can arrive to recombine with them, + // so emitting them as literal is safe and avoids losing trailing output. + if (!streamClosed && ws.readyState === ws.OPEN && carry.length > 0) { + const tail = flushTerminalOutput(carry); + carry = ""; + if (tail.length > 0) { + try { + ws.send( + JSON.stringify({ + type: "data", + data: Buffer.from(tail, "utf8").toString("base64"), + }), + ); + } catch { + /* ignore */ + } + } + } if (!streamClosed && ws.readyState === ws.OPEN) { try { ws.send(JSON.stringify({ type: "exit" })); diff --git a/packages/engine/src/cli-agent/__tests__/task-session.test.ts b/packages/engine/src/cli-agent/__tests__/task-session.test.ts index 8e8b12f1ae..89d4d01750 100644 --- a/packages/engine/src/cli-agent/__tests__/task-session.test.ts +++ b/packages/engine/src/cli-agent/__tests__/task-session.test.ts @@ -316,6 +316,20 @@ describe("CliTaskSession (U7)", () => { await new Promise((r) => setTimeout(r, 0)); expect(pty().written.length).toBeGreaterThan(writesBefore); expect(pty().written.some((w) => w.includes("follow-up"))).toBe(true); + + // The follow-up drove the machine done→busy, so the re-armed result promise + // must resolve on the NEXT positive done (it would hang forever if the + // machine were left parked in `done`, since signalDone-from-done is a no-op). + let resolved = false; + const next = session.result().then((o) => { + resolved = true; + return o; + }); + await new Promise((r) => setTimeout(r, 0)); + expect(resolved).toBe(false); + hub.ingest(session.sessionId, { kind: "done" }); + const outcome = await next; + expect(outcome.kind).toBe("success"); }); it("follow-up returns false when the adapter does not support resume (caller launches fresh)", async () => { diff --git a/packages/engine/src/cli-agent/adapters/__tests__/generic.test.ts b/packages/engine/src/cli-agent/adapters/__tests__/generic.test.ts index 3aa91f8f46..67d3455e90 100644 --- a/packages/engine/src/cli-agent/adapters/__tests__/generic.test.ts +++ b/packages/engine/src/cli-agent/adapters/__tests__/generic.test.ts @@ -32,6 +32,20 @@ describe("GenericCliAdapter capabilities", () => { const asInterface: CliAgentAdapter = adapter; expect(asInterface.buildResume).toBeUndefined(); }); + + it("formatInjection only appends CR and never wraps bracketed paste (no double-wrap)", () => { + // Bracketed-paste wrapping is the session manager's responsibility; the + // adapter must not also wrap, otherwise the payload double-wraps. + expect(adapter.formatInjection("hello", { bracketedPasteActive: false })).toEqual({ + payload: "hello\r", + }); + expect(adapter.formatInjection("hello", { bracketedPasteActive: true })).toEqual({ + payload: "hello\r", + }); + const wrapped = adapter.formatInjection("multi\nline", { bracketedPasteActive: true }); + expect(wrapped.payload).not.toContain("\x1b[200~"); + expect(wrapped.payload).not.toContain("\x1b[201~"); + }); }); // ── buildLaunch / env ──────────────────────────────────────────────────────── diff --git a/packages/engine/src/cli-agent/adapters/generic.ts b/packages/engine/src/cli-agent/adapters/generic.ts index 2792de1294..2bc331e08a 100644 --- a/packages/engine/src/cli-agent/adapters/generic.ts +++ b/packages/engine/src/cli-agent/adapters/generic.ts @@ -388,14 +388,12 @@ export class GenericCliAdapter implements CliAgentAdapter { return new GenericReadinessDetector(); } - formatInjection(text: string, opts: { bracketedPasteActive: boolean }): CliInjectionFormat { - // Submit with a carriage return. Bracketed-paste wrapping is handled by the - // session manager's security path; the generic adapter only decides submit - // semantics. When the child negotiated bracketed paste, wrap so multi-line - // text is delivered atomically before the submit CR. - if (opts.bracketedPasteActive) { - return { payload: `\x1b[200~${text}\x1b[201~\r` }; - } + formatInjection(text: string, _opts: { bracketedPasteActive: boolean }): CliInjectionFormat { + // Submit with a carriage return only. Bracketed-paste wrapping is handled + // exclusively by the session manager's security path; if we also wrapped + // here when bracketedPasteActive, the payload would be double-wrapped with + // \x1b[200~...\x1b[201~. Like every native adapter, generic ignores the + // bracketedPasteActive hint and only appends \r. return { payload: `${text}\r` }; } diff --git a/packages/engine/src/cli-agent/task-session.ts b/packages/engine/src/cli-agent/task-session.ts index aeec588b7a..c9c59b5099 100644 --- a/packages/engine/src/cli-agent/task-session.ts +++ b/packages/engine/src/cli-agent/task-session.ts @@ -397,6 +397,22 @@ export class CliTaskSession { // Re-arm the result promise so the next done resolves it again. if (this.settled) this.rearm(); this.subscribe(); + // Drive the authoritative state machine done→busy BEFORE injecting. If we + // leave it parked in `done`, the next native done is idempotent (no state + // change emitted), so onMachineState("done") never fires and the re-armed + // result promise never resolves. The hub swallows signalBusy-from-done, so + // we transition via the machine's followUp()/injectPrompt directly. + const machine = this.hub.getStateMachine(this.sessionId); + if (machine) { + try { + if (machine.getState() === "done") machine.followUp(); + else if (machine.getState() === "ready" || machine.getState() === "resuming") { + machine.injectPrompt(); + } + } catch { + // best-effort transition + } + } await this.manager.inject(this.sessionId, prompt); this.log(`cli-task-session ${this.sessionId}: follow-up injected (live resume)`); return true;