diff --git a/docs/webui.md b/docs/webui.md index 14c64c0d..a15baeac 100644 --- a/docs/webui.md +++ b/docs/webui.md @@ -324,6 +324,12 @@ An error rather than a synthetic "cancelled" result says plainly that this clien The seam for a real surface is the `clientRequest` constructor option: `(method, params) => result | Promise`. Its resolved value becomes the JSON-RPC `result`; a throw or rejection becomes an error response carrying the thrown `message` and, when it has one, its `code` (otherwise `-32603`). Nothing in the webui installs a handler yet — routing a decision through to the browser is separate work, and the honest current state is that the webui has no interactive surface to offer. +### Engine stderr in the crash alert + +The engine announces its own failures on stderr and then dies; the crash alert is raised by the webui, not by the engine. `McodeAcpClient` therefore keeps a bounded tail of that stream — the last 2KB and the last 20 lines, cleared at every `start()` so one process's crash text can never be blamed on the next — and the `[mcode-acp.start]` and `[mcode-acp.stream]` error alerts carry it as `data.stderrTail`, prefixed with `[acp stderr truncated, showing the tail]` when anything was dropped. An exit code is not a diagnosis: `mcode acp exited (code=1)` cannot separate a lock the engine could not take from a configuration it refused to parse, while the engine's own line (`agent_name_conflict_migration_failed:lock`) says which. + +`stderrTail` is additive and optional. A silent engine leaves `data` byte-identical to what it was before the field existed, so no consumer of the alert contract has to learn a new required key. The `debug` constructor option keeps its old job — mirroring the stream live to the server's own stderr as it arrives — but all three construction sites in the shipped server pass `debug: false`, so in a running webui the alert's tail is the only channel that stderr has. + ### What this does and does not buy `plan: {}` turns on a **notification**, not a question. A plan review carries a single `approve` option and the Runtime pins `allowOther: true` on every step, so the engine settles it fail-closed through the questionnaire path rather than turning it into a permission request — which is why advertising `plan` is safe for a client that cannot answer anything. The permission-request path is a separate switch the webui never turns on. diff --git a/docs/webui.zh-CN.md b/docs/webui.zh-CN.md index 1f6af167..59d3d70f 100644 --- a/docs/webui.zh-CN.md +++ b/docs/webui.zh-CN.md @@ -324,6 +324,12 @@ ACP 握手是双向的，两个方向都由同一份 `initialize` 载荷决定 留给真实交互界面的接缝是构造函数选项 `clientRequest`：`(method, params) => result | Promise`。它的 resolved 值成为 JSON-RPC 的 `result`；抛错或 reject 变成错误响应，携带抛出的 `message` 与（若有）`code`，否则为 `-32603`。webui 目前没有安装任何处理器——把一次决定真正送到浏览器是另一件事，诚实的现状是 webui 没有可提供的交互界面。 +### 崩溃告警里的引擎 stderr + +引擎把自己的失败写在 stderr 上然后死掉，而崩溃告警是 webui 发的，不是引擎发的。所以 `McodeAcpClient` 会留住这段输出的一个**有界尾部**——最后 2KB 且最后 20 行，并且在每次 `start()` 时清空，免得一个进程的崩溃信息被算到下一个进程头上——`[mcode-acp.start]` 与 `[mcode-acp.stream]` 这两条错误告警把它作为 `data.stderrTail` 带出去；若确实丢掉了内容，前面会加上 `[acp stderr truncated, showing the tail]`。退出码不是诊断：`mcode acp exited (code=1)` 分不清是引擎拿不到锁，还是配置被它拒绝解析；引擎自己那一行（`agent_name_conflict_migration_failed:lock`）才能说明是哪一种。 + +`stderrTail` 是新增的可选字段。引擎若什么都没写，`data` 与这个字段出现之前逐字节相同，所以告警契约的任何消费方都不必学到一个新的必填键。`debug` 构造选项保留它原来的职责——把这段流实时镜像到服务端自己的 stderr——但要清楚：已发布服务端的三个构造点全部传 `debug: false`，因此在一个真正跑起来的 webui 里，告警里的尾部是 stderr 唯一的出口。 + ### 这次拿到了什么、没拿到什么 `plan: {}` 打开的是**通知**，不是提问。计划评审只有一个 `approve` 选项，而运行时把 `allowOther: true` 固定在每一步上，所以引擎会走问卷通道 fail-closed 地了结它，而不会把它变成一次权限请求——这正是「声明 `plan`」对一个什么都答不了的客户端仍然安全的原因。权限请求通道是另一个开关，webui 从不打开它。 diff --git a/packages/webui/acp.mjs b/packages/webui/acp.mjs index d20fe273..94197581 100644 --- a/packages/webui/acp.mjs +++ b/packages/webui/acp.mjs @@ -34,6 +34,20 @@ const DEFAULT_CWD = process.cwd() const JSON_RPC_METHOD_NOT_FOUND = -32601 const JSON_RPC_INTERNAL_ERROR = -32603 +// Bounded tail of the engine subprocess's stderr. +// +// The engine reports its OWN failures on stderr — a failed migration, a +// lock it could not take, a config it refused to parse — and the crash +// alert is raised by the webui, not by the engine. Without a tail the +// whole diagnostic dies with the pipe: the operator sees only +// `mcode acp exited (code=1)` and cannot tell a lock contention from a +// missing binary. Both bounds are needed: bytes alone let one long +// stack trace push the real message out of the window, and lines alone +// let one pathological line carry megabytes. +const STDERR_TAIL_MAX_BYTES = 2048 +const STDERR_TAIL_MAX_LINES = 20 +const STDERR_TRUNCATION_MARKER = '[acp stderr truncated, showing the tail]' + /** * The capabilities this client advertises in `initialize`. * @@ -96,12 +110,31 @@ export class McodeAcpClient extends EventEmitter { // singleton (the previous PR's bug: `_mcodeAcpSingleton.alive` always // undefined) is now actually detected and replaced on the next call. this._alive = false + // Bounded stderr tail (see STDERR_TAIL_MAX_BYTES). Reset per + // `start()` because each start is a different subprocess. + this._stderrTail = '' + this._stderrTruncated = false } get alive() { return this._alive && this.child !== null && this.started === true } + /** + * The engine subprocess's stderr, bounded to the last ~2KB / ~20 lines, + * prefixed with a truncation marker when anything was dropped. + * + * `''` when the engine wrote nothing to stderr — a caller reporting a + * crash omits the field rather than attaching an empty string, so the + * alert it builds keeps the shape it had before this existed. + */ + get stderrTail() { + if (!this._stderrTail) return '' + const lines = this._stderrTail.split('\n') + const kept = lines.slice(-STDERR_TAIL_MAX_LINES).join('\n') + return this._stderrTruncated ? STDERR_TRUNCATION_MARKER + '\n' + kept : kept + } + async start() { if (this.started) return this.capabilities // Windows .cmd shim handling: Node 22+ rejects `spawn('mcode.cmd', { shell:false })` @@ -111,6 +144,10 @@ export class McodeAcpClient extends EventEmitter { // On Linux/macOS, plain `spawn('mcode')` walks PATH. .js/.mjs entries run under // process.execPath on every platform. const resolved = resolveMcodeCmd() + // A new subprocess gets a new tail: a stale line from a previous + // process would misattribute its failure to this one. + this._stderrTail = '' + this._stderrTruncated = false let cmd, args if (/\.(js|mjs)$/i.test(resolved)) { cmd = process.execPath @@ -156,9 +193,7 @@ export class McodeAcpClient extends EventEmitter { this.child.stdout.setEncoding('utf8') this.child.stdout.on('data', (chunk) => this._onData(chunk)) this.child.stderr.setEncoding('utf8') - this.child.stderr.on('data', (c) => { - if (this.debug) process.stderr.write('[acp stderr] ' + c) - }) + this.child.stderr.on('data', (c) => this._onStderr(c)) this.capabilities = await this.request('initialize', { protocolVersion: 1, clientInfo: { name: 'mcode-webui', version: '0.1.0' }, @@ -183,6 +218,22 @@ export class McodeAcpClient extends EventEmitter { this.pending.clear() } + // Record the engine's stderr for the crash alert, and mirror it live + // in debug mode (the dev-loop behavior this handler had before the + // tail existed — unchanged). The tail is kept regardless of `debug`: + // in production nobody is reading the server's own stderr, which is + // precisely why the engine's message has to travel inside the alert. + _onStderr(chunk) { + if (this.debug) process.stderr.write('[acp stderr] ' + chunk) + const next = this._stderrTail + chunk + if (next.length > STDERR_TAIL_MAX_BYTES) { + this._stderrTail = next.slice(-STDERR_TAIL_MAX_BYTES) + this._stderrTruncated = true + } else { + this._stderrTail = next + } + } + _onData(chunk) { this.buf += chunk let nl diff --git a/packages/webui/server/lib/mcode-acp.js b/packages/webui/server/lib/mcode-acp.js index c70a0f00..ccb21b9f 100644 --- a/packages/webui/server/lib/mcode-acp.js +++ b/packages/webui/server/lib/mcode-acp.js @@ -272,6 +272,26 @@ function matchesModelId(recorded, engineCurrent, modelOption) { * `resolveModelId` covers this case before the name-match runs. */ +/** + * The engine's stderr tail, shaped for an alert's `data`. + * + * The engine announces its own failures on stderr and dies; the webui is + * the one that raises the crash alert. Without carrying the tail across, + * every engine failure collapses to `mcode acp exited (code=1)` — an + * operator cannot act on an exit code, only on the engine's line + * (`agent_name_conflict_migration_failed:lock`, a config parse error, a + * missing binary). The client already bounds and truncates it; see + * `McodeAcpClient#stderrTail` in packages/webui/acp.mjs. + * + * Returns `{}` — not `{ stderrTail: "" }` — when the engine said nothing, + * so a silent failure produces byte-identical alert data to what it + * produced before this helper existed. + */ +function acpStderrData(client) { + const tail = client && typeof client.stderrTail === "string" ? client.stderrTail : ""; + return tail ? { stderrTail: tail } : {}; +} + // Exported for unit tests (test/lib/mcode-acp-note.test.js extends to // cover applyRecordedModel's resolution logic). The pre-session model // apply needs to handle three input forms without regressing, so the @@ -431,7 +451,7 @@ export async function runMcodeAcp(content, opts = {}) { src: "mcode-acp", cid: cid || null, sessionId: sid || null, - data: { phase: "start-or-load" }, + data: { phase: "start-or-load", ...acpStderrData(client) }, }); return { status: "failed", @@ -1330,7 +1350,7 @@ function streamAcpPrompt( src: "mcode-acp", cid: cid || null, sessionId: sid || null, - data: { phase: "promise-catch" }, + data: { phase: "promise-catch", ...acpStderrData(client) }, }); finalize(); }); diff --git a/packages/webui/test/lib/acp-stderr-tail.test.js b/packages/webui/test/lib/acp-stderr-tail.test.js new file mode 100644 index 00000000..09d8ea69 --- /dev/null +++ b/packages/webui/test/lib/acp-stderr-tail.test.js @@ -0,0 +1,305 @@ +// webui/test/lib/acp-stderr-tail.test.js +// +// D2 — the engine subprocess's stderr used to reach nobody. +// +// `packages/webui/acp.mjs` forwarded stderr to the server's own stderr +// only under `this.debug`, so a production webui watched the engine die +// with `agent_name_conflict_migration_failed:lock` on the pipe and +// surfaced a single actionable-looking non-action: `mcode acp exited +// (code=1)`. Exit codes do not say which lock; the engine's line does. +// +// The fix carries a bounded tail of that stream inside the failure +// alert's `data.stderrTail`. These assertions run against REAL fake +// engine subprocesses (a stubbed `McodeAcpClient` would prove only that +// the stub's own buffer works) and against the real `runMcodeAcp`, so +// the bytes cross a genuine pipe, a genuine `spawn`, and a genuine +// `pushAlert`. +// +// What is pinned here, and why each half matters: +// * the tail survives `debug: false` — the whole point of the fix; +// * it is BOUNDED and marked when truncated, so a chatty engine cannot +// turn a 200-byte alert into a 200KB one nor hide the failure behind +// its own earlier noise; +// * a silent engine leaves the alert's `data` byte-identical to what it +// was before the field existed (absent, not `""`); +// * a clean code-0 run raises no error alert at all. + +import { test, describe, before, after, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { writeFileSync } from "node:fs"; +import { join, resolve } from "node:path"; +import { pathToFileURL } from "node:url"; + +import { mkTmpDir, rmTmpDir } from "../helpers/tmp.js"; + +const WEBUI_DIR = resolve(import.meta.dirname, "..", ".."); +const absWebuiPath = (rel) => pathToFileURL(resolve(WEBUI_DIR, rel)).href; + +const { McodeAcpClient } = await import(absWebuiPath("acp.mjs")); +const alerts = await import(absWebuiPath("server/lib/alerts.js")); +const mcodeAcp = await import(absWebuiPath("server/lib/mcode-acp.js")); + +// The engine failure the field report actually lost. Kept as a literal +// so a rename on the engine side shows up here as a failing test rather +// than as a silently narrowed assertion. +const ENGINE_FATAL = "agent_name_conflict_migration_failed:lock"; + +// ---------- fake engines ---------- + +// Crashes on startup: the exact shape that produced `code=1` and no +// diagnosis. The noise goes to stderr BEFORE the fatal line, so an +// unbounded implementation would have shipped the last 2KB — mostly +// noise — and dropped the line the operator needed. `process.exitCode` +// (not `process.exit()`) lets the stderr pipe flush before the process +// ends; an explicit exit() truncates piped writes and would make this +// test flaky for the wrong reason. +const CRASH_ENGINE = ` +for (let i = 1; i <= 200; i++) { + process.stderr.write("migrating agent name registry, step " + i + " ...\\n"); +} +process.stderr.write("${ENGINE_FATAL} at ~/.minimax-code/agents.lock\\n"); +process.exitCode = 1; +`; + +// Dies the same way, but says nothing. The reverse half: an engine that +// never spoke must leave the alert's shape untouched. +const SILENT_CRASH_ENGINE = ` +process.exitCode = 1; +`; + +// Completes one full turn and exits 0. A chatty-but-healthy engine is +// the other half: its stderr is retained, yet nothing is an error, so +// `stderrTail` must not become a failure signal of its own. +const CLEAN_ENGINE = ` +const send = (m) => process.stdout.write(JSON.stringify(m) + "\\n"); +process.stderr.write("[mcode] resuming 3 sessions\\n"); +let buf = ""; +process.stdin.setEncoding("utf8"); +process.stdin.on("data", (chunk) => { + buf += chunk; + let nl; + while ((nl = buf.indexOf("\\n")) !== -1) { + const line = buf.slice(0, nl).trim(); + buf = buf.slice(nl + 1); + if (!line) continue; + const msg = JSON.parse(line); + if (msg.method === "initialize") { + send({ jsonrpc: "2.0", id: msg.id, result: { protocolVersion: 1, agentCapabilities: {}, configOptions: [] } }); + } else if (msg.method === "session/new") { + send({ jsonrpc: "2.0", id: msg.id, result: { sessionId: "sess-clean", configOptions: [] } }); + } else if (msg.method === "session/prompt") { + send({ jsonrpc: "2.0", id: msg.id, result: { stopReason: "end_turn" } }); + setTimeout(() => { process.exitCode = 0; process.stdin.pause(); }, 20); + } else if (msg.method && msg.id !== undefined) { + send({ jsonrpc: "2.0", id: msg.id, result: {} }); + } + } +}); +`; + +let dir = null; +const engines = {}; + +before(() => { + dir = mkTmpDir("webui-acp-stderr-"); + for (const [name, source] of Object.entries({ + crash: CRASH_ENGINE, + silent: SILENT_CRASH_ENGINE, + clean: CLEAN_ENGINE, + })) { + engines[name] = join(dir, `${name}-engine.mjs`); + writeFileSync(engines[name], source); + } +}); + +after(async () => { + // Importing lib/mcode-acp.js pulls in lib/acp-client.js, which starts a + // resident engine singleton on module load. Left running it keeps a + // spawned process — and this test file's event loop — alive forever. + const acpClient = await import(absWebuiPath("server/lib/acp-client.js")); + try { + acpClient.shutdownMcodeAcpSingleton(); + } catch { + /* never started, or already gone */ + } + await new Promise((r) => setTimeout(r, 50)); + delete process.env.MCODE_CMD; + if (dir) rmTmpDir(dir); +}); + +beforeEach(() => { + alerts._resetForTests(); +}); + +const running = []; + +afterEach(() => { + while (running.length) running.pop().stop(); + delete process.env.MCODE_CMD; + alerts._resetForTests(); +}); + +/** The `cs` runMcodeAcp reads before the engine is even reached. */ +function makeCs() { + return { + model: { name: "minimax_api/MiniMax-M3" }, + workspace: { dir: dir }, + sessionId: null, + mcodeSessionId: null, + sessionTitle: "Untitled", + chat: [], + usage: {}, + context: { used: 0, limit: 0, percent: 0, tokens: 0 }, + running: { active: false }, + }; +} + +/** The failure alerts runMcodeAcp raised, newest last. */ +function errorAlerts() { + return alerts.getRecentAlerts().filter((a) => a.src === "mcode-acp" && a.level === "error"); +} + +describe("engine stderr reaches the failure alert (D2)", () => { + test("a crash alert carries the truncated stderr tail, with debug off", async () => { + process.env.MCODE_CMD = engines.crash; + + const r = await mcodeAcp.runMcodeAcp("hi", { + label: "test", + cs: makeCs(), + cid: "cid-stderr-crash", + sessionId: null, + }); + + assert.equal(r.status, "failed", "the engine crashed, so the turn fails"); + + const raised = errorAlerts(); + assert.equal(raised.length, 1, `expected exactly one engine error alert, got ${JSON.stringify(raised)}`); + const [alert] = raised; + + // The lost diagnostic is back, and it is the reason the turn failed. + assert.match(alert.data.stderrTail, new RegExp(ENGINE_FATAL)); + assert.match(alert.msg, /mcode acp exited \(code=1/); + }); + + test("the tail is bounded and marked when truncated", async () => { + process.env.MCODE_CMD = engines.crash; + + await mcodeAcp.runMcodeAcp("hi", { + label: "test", + cs: makeCs(), + cid: "cid-stderr-bounded", + sessionId: null, + }); + + const { stderrTail } = errorAlerts()[0].data; + + // 200 lines of ~45 bytes each: an unbounded tail would carry the + // whole 9KB, and a byte-unbounded alert is a log-flooding vector. + assert.ok( + stderrTail.length < 2048 + 200, + `the tail must stay near its 2KB bound, got ${stderrTail.length} chars`, + ); + assert.match(stderrTail, /^\[acp stderr truncated, showing the tail\]/); + // Truncation is stated, not silent — an operator must be able to + // tell "that was all" from "that was the end of what we kept". + assert.doesNotMatch( + stderrTail, + /migrating agent name registry, step 1 /, + "the earliest noise must have been dropped, not carried", + ); + }); + + test("the alert's own shape is unchanged — stderrTail is additive", async () => { + process.env.MCODE_CMD = engines.crash; + + await mcodeAcp.runMcodeAcp("hi", { + label: "test", + cs: makeCs(), + cid: "cid-stderr-shape", + sessionId: null, + }); + + const [alert] = errorAlerts(); + // Every pre-existing field, untouched. Consumers switching on the + // alert contract (SSE /api/alerts, the audit event) must not have + // to learn a new required field. + assert.equal(alert.level, "error"); + assert.equal(alert.src, "mcode-acp"); + assert.equal(alert.cid, "cid-stderr-shape"); + assert.equal(alert.sessionId, null); + assert.equal(alert.data.phase, "start-or-load"); + assert.equal(alert.count, 1); + }); + + test("a silent engine leaves the alert data exactly as it was", async () => { + process.env.MCODE_CMD = engines.silent; + + await mcodeAcp.runMcodeAcp("hi", { + label: "test", + cs: makeCs(), + cid: "cid-stderr-silent", + sessionId: null, + }); + + const raised = errorAlerts(); + assert.equal(raised.length, 1); + // Absent, not `""`: an operator (and a deduped alert diff) should + // not be able to tell this alert from one raised before the fix. + assert.deepEqual(raised[0].data, { phase: "start-or-load" }); + }); + + test("a clean code-0 run raises no failure alert even with stderr output", async () => { + process.env.MCODE_CMD = engines.clean; + + const r = await mcodeAcp.runMcodeAcp("hi", { + label: "test", + cs: makeCs(), + cid: "cid-stderr-clean", + sessionId: null, + }); + + assert.equal(r.status, "succeeded", `clean engine run failed: ${JSON.stringify(r.error)}`); + // The engine did write to stderr. Retaining it must not turn + // ordinary engine chatter into a failure signal. + assert.deepEqual(errorAlerts(), []); + }); +}); + +describe("McodeAcpClient stderr tail", () => { + test("the tail is readable after a crash, and empty before anything is written", async () => { + process.env.MCODE_CMD = engines.crash; + const client = new McodeAcpClient({ debug: false }); + running.push(client); + + assert.equal(client.stderrTail, "", "nothing on the wire yet, so nothing to report"); + + const exited = new Promise((res) => client.once("exit", res)); + await assert.rejects(() => client.start()); + await exited; + + assert.match(client.stderrTail, new RegExp(ENGINE_FATAL)); + assert.match(client.stderrTail, /truncated/); + }); + + test("start() resets the tail so one process cannot be blamed for another's crash", async () => { + process.env.MCODE_CMD = engines.crash; + const client = new McodeAcpClient({ debug: false }); + running.push(client); + + const firstExit = new Promise((res) => client.once("exit", res)); + await assert.rejects(() => client.start()); + await firstExit; + assert.match(client.stderrTail, new RegExp(ENGINE_FATAL)); + + // A restart begins a NEW subprocess, whose stderr starts empty. The + // dead process's crash text must not survive into the next run and + // re-appear on some unrelated failure later. + process.env.MCODE_CMD = engines.clean; + const restarted = client.start(); + // The reset happens before the spawn, so it is observable the moment + // `start()` is called. Whether THIS run goes on to succeed or fail + // is beside the point: the dead process's crash text is already gone. + assert.doesNotMatch(client.stderrTail, new RegExp(ENGINE_FATAL)); + restarted.catch(() => {}); + }); +}); diff --git a/release/public-source.json b/release/public-source.json index 456b5662..21e2f8af 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3588,6 +3588,7 @@ "packages/webui/test/integration/upload-limits.test.js", "packages/webui/test/lib/acp-cache.check.mjs", "packages/webui/test/lib/acp-client-requests.test.js", + "packages/webui/test/lib/acp-stderr-tail.test.js", "packages/webui/test/lib/acp-transport-answer.test.js", "packages/webui/test/lib/acp-turn-message-id.test.js", "packages/webui/test/lib/agent-team-detect.test.js", diff --git a/scripts/test-tmp-leak.check.mjs b/scripts/test-tmp-leak.check.mjs index 0520ba7d..3abfdca1 100644 --- a/scripts/test-tmp-leak.check.mjs +++ b/scripts/test-tmp-leak.check.mjs @@ -261,6 +261,7 @@ const KNOWN_PREFIXES = [ "state-bus-restore-", "webui-acp-answer-", "webui-acp-fake-engine-", + "webui-acp-stderr-", "webui-alerts-audit-", "webui-alerts-check-", "webui-authgate-events-",