From e1581046f14cb69e8b0ab5040dd6d72c15a4a922 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Sat, 25 Jul 2026 11:52:39 +0800 Subject: [PATCH 1/8] fix(delegation): parse Codex's live MCP result envelope in the status cards codex-acp forwards every MCP tool call's outcome to the ACP wire as `rawOutput = { result: , error: }`, one layer above the shapes the delegation parsers read. Nothing peeled it, so with Codex as the main agent a `get_delegation_status` poll resolved to no report at all: a batch of tasks collapsed into a single anonymous row that rendered the raw envelope JSON as its result, carried no task id or duration, and fell back to the tool-call lifecycle for its badge -- reporting "done" for tasks that were still running. Codex's persisted rollout carries the bare `Wall time:.../ Output:` wrapper instead, which already parsed, so only the live path was affected. Add `peelMcpResultEnvelope`, which strips that envelope plus the `{Ok}` serde variant Codex writes into its rollout, and run it in `delegation-status.ts` (both the single-report and batch paths) and `delegation-card.ts` (`parseToolOutput` and `parseDelegateTaskId`, where the same envelope hid a running ack's child conversation id). An envelope whose call failed outright now surfaces its own error string instead of the raw JSON. The peel is positively identified on both ends so a child agent's arbitrary output is never mistaken for a companion result: a wrapper key is only followed when the value under it is a real CallToolResult (carries `content` or `structuredContent`), and the host error string is only read from `{result: null, error: "..."}`. A payload that merely owns a `result` or `error` field of its own is left exactly as it was. Parsing happens at render time, so existing transcripts pick this up on reopen. --- src/lib/delegation-card.test.ts | 89 +++++++++++++++++- src/lib/delegation-card.ts | 46 ++++++++- src/lib/delegation-status.test.ts | 150 ++++++++++++++++++++++++++++++ src/lib/delegation-status.ts | 64 +++++++++++-- src/lib/mcp-result-envelope.ts | 108 +++++++++++++++++++++ 5 files changed, 444 insertions(+), 13 deletions(-) create mode 100644 src/lib/mcp-result-envelope.ts diff --git a/src/lib/delegation-card.test.ts b/src/lib/delegation-card.test.ts index d04461633..9492a7dfc 100644 --- a/src/lib/delegation-card.test.ts +++ b/src/lib/delegation-card.test.ts @@ -1,7 +1,12 @@ import { describe, expect, it } from "vitest" import { ALL_AGENT_TYPES } from "@/lib/types" -import { parseDelegationMeta, parseInput } from "./delegation-card" +import { + parseDelegateTaskId, + parseDelegationMeta, + parseInput, + parseToolOutput, +} from "./delegation-card" describe("parseInput wrapper peeling", () => { it("reads top-level delegation args", () => { @@ -50,6 +55,88 @@ describe("parseInput wrapper peeling", () => { }) }) +describe("codex live-wire result envelope", () => { + /** + * codex-acp forwards every MCP call's outcome as + * `rawOutput = { result: , error: }` + * (`createMcpRawOutput`) — one layer above the shapes the parsers read. + */ + function codexLive(callToolResult: Record): string { + return JSON.stringify({ + error: null, + result: { meta: null, ...callToolResult }, + }) + } + + const runningAck = { + agent_type: "codex", + child_conversation_id: 2781, + status: "running", + task_id: "8cb72a7c-1a96-44aa-9c26-d4356862c9c2", + message: + "Delegation successful. task_id=8cb72a7c-1a96-44aa-9c26-d4356862c9c2.", + } + + it("reads a running ack through the wrapper (ack, not a terminal outcome)", () => { + const parsed = parseToolOutput( + codexLive({ + content: [{ type: "text", text: runningAck.message }], + structuredContent: runningAck, + }) + ) + expect(parsed).toEqual({ kind: "ack", childConversationId: 2781 }) + }) + + // Note: this one already passed pre-fix via the `task_id=` text scan — + // it guards that the structured path doesn't regress that resolution. + it("resolves the task id through the wrapper", () => { + const output = codexLive({ + content: [{ type: "text", text: "Delegation successful." }], + structuredContent: runningAck, + }) + expect(parseDelegateTaskId(output, null)).toBe(runningAck.task_id) + }) + + it("surfaces a failed envelope's error string instead of the raw JSON", () => { + const parsed = parseToolOutput( + JSON.stringify({ error: "mcp server disconnected", result: null }) + ) + expect(parsed).toEqual({ + kind: "outcome", + text: "mcp server disconnected", + isError: true, + childConversationId: null, + }) + }) + + it("leaves a child's nested {status, task_id} payload alone", () => { + // Peeling on the `result` KEY alone would turn opaque child output into a + // failed delegation outcome and hand out `child-job` as the task id. + const childOutput = JSON.stringify({ + result: { status: "failed", task_id: "child-job", message: "domain" }, + }) + expect(parseToolOutput(childOutput)).toEqual({ + kind: "outcome", + text: + "```json\n" + + JSON.stringify(JSON.parse(childOutput), null, 2) + + "\n```", + isError: false, + childConversationId: null, + }) + expect(parseDelegateTaskId(childOutput, null)).toBeNull() + }) + + it("does NOT treat a child's own `error` field as a host failure", () => { + // No `result` key ⇒ not codex-acp's failure envelope. Must stay a + // non-error outcome rendered as-is. + const childOutput = JSON.stringify({ error: "domain validation", rows: [] }) + const parsed = parseToolOutput(childOutput) + expect(parsed).toMatchObject({ kind: "outcome", isError: false }) + expect(parsed).not.toMatchObject({ text: "domain validation" }) + }) +}) + describe("parseDelegationMeta task fields", () => { it("surfaces the broker-stamped task_preview and task_id", () => { // The persisted Cursor shape: raw_input is "{}" forever, so the meta the diff --git a/src/lib/delegation-card.ts b/src/lib/delegation-card.ts index 424d82a5c..9231dd06d 100644 --- a/src/lib/delegation-card.ts +++ b/src/lib/delegation-card.ts @@ -11,6 +11,7 @@ */ import { extractEmbeddedJsonObject } from "@/lib/embedded-json" +import { peelMcpResultEnvelope } from "@/lib/mcp-result-envelope" import { ALL_AGENT_TYPES, type AgentType } from "@/lib/types" import { type DelegationBinding, @@ -372,6 +373,22 @@ function interpretMcpContentArray( return null } +/** Whether `obj` is already one of the shapes [`parseToolOutput`] reads — a + * report (`status`), a legacy outcome (`kind`), or an MCP `CallToolResult`. + * Stops the host-envelope peel at a result that itself happens to carry a + * `result` key. (A child's arbitrary payload is guarded on the other side too: + * `peelMcpResultEnvelope` only ever peels TO a real `CallToolResult`.) */ +function isResolvableDelegateResult(obj: Record): boolean { + return ( + typeof obj.status === "string" || + typeof obj.kind === "string" || + Array.isArray(obj.content) || + (typeof obj.structuredContent === "object" && + obj.structuredContent !== null && + !Array.isArray(obj.structuredContent)) + ) +} + /** * Best-effort parse of the `delegate_to_agent` tool output into a * `ParsedToolOutput`. Mirrors the old unwrapping chain (direct JSON → @@ -379,6 +396,10 @@ function interpretMcpContentArray( * `companion.rs::render_task_report`) but yields the ack/outcome tagged union * so a running ack is never rendered as a result. `forceError` is set when * parsing the tool's `errorText` channel. + * + * A host envelope around the MCP result — Codex's live wire sends + * `{result: , error: null}` — is peeled first, so the chain + * below only ever faces the result itself. */ export function parseToolOutput( raw: string | null | undefined, @@ -415,6 +436,9 @@ export function parseToolOutput( } } + const peel = peelMcpResultEnvelope(obj, isResolvableDelegateResult) + obj = peel.obj + // MCP `CallToolResult` envelope: `{ content: [...], structuredContent?, isError? }`. if (Array.isArray(obj.content)) { const inner = @@ -470,6 +494,17 @@ export function parseToolOutput( return interpreted } + // A host envelope that failed outright carries no result to render — its own + // error string is the whole story, and beats dumping the envelope JSON. + if (peel.hostError) { + return { + kind: "outcome", + text: peel.hostError, + isError: true, + childConversationId: null, + } + } + // Unrecognized JSON — pretty-print so we don't surface raw braces. return { kind: "outcome", @@ -507,12 +542,19 @@ export function parseDelegateTaskId( obj = extractEmbeddedJsonObject(trimmed) } if (obj) { - const sc = obj.structuredContent + // Peel Codex's live `{result, error}` wrapper so `structuredContent` is + // reachable; a bare `task_id` at this level already ends the walk. + const { obj: result } = peelMcpResultEnvelope( + obj, + (o) => typeof o.task_id === "string" || isResolvableDelegateResult(o) + ) + const sc = result.structuredContent if (sc && typeof sc === "object" && !Array.isArray(sc)) { const id = (sc as Record).task_id if (typeof id === "string" && id) return id } - if (typeof obj.task_id === "string" && obj.task_id) return obj.task_id + if (typeof result.task_id === "string" && result.task_id) + return result.task_id } // Live wire: the ack message text embeds `task_id=`. const m = trimmed.match(/task_id[=:]\s*"?([A-Za-z0-9][\w-]*)"?/) diff --git a/src/lib/delegation-status.test.ts b/src/lib/delegation-status.test.ts index a86b1b7bb..1bfeabab7 100644 --- a/src/lib/delegation-status.test.ts +++ b/src/lib/delegation-status.test.ts @@ -24,6 +24,20 @@ function envelope(report: Record, isError = false): string { }) } +/** + * codex's LIVE wire shape. codex-acp forwards every MCP call's outcome as + * `rawOutput = { result: , error: }` + * (`createMcpRawOutput`), so the card sees the CallToolResult one level down — + * unlike codex's persisted rollout, which carries the bare + * `Wall time:…\nOutput:\n` text. + */ +function codexLiveEnvelope(callToolResult: string): string { + return JSON.stringify({ + error: null, + result: { meta: null, ...JSON.parse(callToolResult) }, + }) +} + describe("parseTaskId", () => { it("reads a plain task_id", () => { expect(parseTaskId(JSON.stringify({ task_id: "abc12345" }))).toBe( @@ -170,6 +184,49 @@ describe("parseStatusReports", () => { }) }) + it("peels codex's live {result, error} wrapper around a batch", () => { + // Codex as the MAIN agent: the whole envelope used to fall through as + // opaque text, so one anonymous row rendered the raw JSON instead of one + // row per task. + const reports = parseStatusReports( + codexLiveEnvelope( + batchEnvelope([ + { + task_id: "8cb72a7c", + status: "running", + message: "Running.\nLatest sub-agent reply: thinking", + }, + { + task_id: "9c232105", + status: "completed", + text: "done", + duration_ms: 42, + }, + ]) + ), + null + ) + expect(reports).toHaveLength(2) + expect(reports[0]).toMatchObject({ status: "running", taskId: "8cb72a7c" }) + expect(reports[1]).toMatchObject({ + status: "completed", + taskId: "9c232105", + text: "done", + durationMs: 42, + }) + }) + + it("peels codex's rollout {Ok} serde variant around a batch", () => { + const output = JSON.stringify({ + Ok: JSON.parse( + batchEnvelope([{ task_id: "t1", status: "completed", text: "ok" }]) + ), + }) + const reports = parseStatusReports(output, null) + expect(reports).toHaveLength(1) + expect(reports[0]).toMatchObject({ status: "completed", taskId: "t1" }) + }) + it("does NOT treat a tasks array of non-reports as a batch", () => { // A child whose own output is {tasks:[...]} of non-report objects must not // be misread — it falls back to the single-report path. @@ -221,6 +278,66 @@ describe("parseStatusReport", () => { expect(report.durationMs).toBe(1234) }) + it("peels codex's live {result, error} wrapper around a single report", () => { + const report = parseStatusReport( + codexLiveEnvelope( + envelope({ + task_id: "abc12345", + status: "completed", + text: "All done.", + duration_ms: 1234, + }) + ), + null + ) + expect(report.status).toBe("completed") + expect(report.taskId).toBe("abc12345") + expect(report.durationMs).toBe(1234) + expect(report.text).toBe("All done.") + }) + + it("shows a failed host envelope's error string, not the envelope JSON", () => { + // codex-acp sets `error` and leaves `result` null when the MCP call itself + // failed — there is no result to peel to, so the error text is all there is. + const report = parseStatusReport( + JSON.stringify({ error: "mcp server disconnected", result: null }), + null + ) + expect(report.text).toBe("mcp server disconnected") + }) + + it("renders a child's own `result`-shaped payload verbatim", () => { + // The peel finds no CallToolResult under `result`, so nothing is recovered + // and the child's output still displays as-is. + const childOutput = JSON.stringify({ result: { rows: 3 }, error: null }) + const report = parseStatusReport(childOutput, null) + expect(report.status).toBeNull() + expect(report.taskId).toBeNull() + expect(report.text).toBe(childOutput) + }) + + it("does NOT read a child's nested {status, task_id} as a report", () => { + // The nastiest shape: peeling on the `result` KEY alone would surface a + // report-looking object and repaint opaque child output as a failed task. + // Only a real CallToolResult (content / structuredContent) may be peeled to. + const childOutput = JSON.stringify({ + result: { status: "failed", task_id: "child-job", message: "domain" }, + }) + const report = parseStatusReport(childOutput, null) + expect(report.status).toBeNull() + expect(report.taskId).toBeNull() + expect(report.text).toBe(childOutput) + }) + + it("does NOT treat a child's own `error` field as a host failure", () => { + // No `result` key ⇒ not codex-acp's failure envelope, so the payload stays + // whole instead of collapsing to the error string. + const childOutput = JSON.stringify({ error: "domain validation", rows: [] }) + const report = parseStatusReport(childOutput, null) + expect(report.status).toBeNull() + expect(report.text).toBe(childOutput) + }) + it("does NOT treat a child's own JSON-with-status as a report (no task_id)", () => { const childOutput = JSON.stringify({ status: "failed", message: "child" }) const report = parseStatusReport(childOutput, null) @@ -453,6 +570,39 @@ describe("buildDelegationTaskRows", () => { expect(rows[0].taskId).toBe("t1") }) + it("builds one row per task from a codex live-wire batch poll", () => { + // End-to-end shape of the reported bug: with codex as the main agent the + // `{result, error}` wrapper hid the batch, collapsing both tasks into one + // anonymous row whose body was the raw envelope JSON and whose badge fell + // back to "ok" (output-available) even for the still-running task. + const rows = buildDelegationTaskRows([ + statusPoll({ + toolCallId: "p1", + input: JSON.stringify({ task_ids: ["t1", "t2"], wait_ms: 60000 }), + output: codexLiveEnvelope( + batchEnvelope([ + { task_id: "t1", status: "running", message: "Running." }, + { + task_id: "t2", + status: "completed", + text: "review done", + duration_ms: 3000, + }, + ]) + ), + }), + ]) + expect(rows).toHaveLength(2) + expect(rows[0].taskId).toBe("t1") + // A poll that RETURNED while the task ran is a settled snapshot, not a + // spinner — and definitely not "ok". + expect(rows[0].badge.status).toBe("checked") + expect(rows[1].taskId).toBe("t2") + expect(rows[1].badge.status).toBe("ok") + expect(rows[1].results).toEqual(["review done"]) + expect(rows[1].report.durationMs).toBe(3000) + }) + it("drops a live arg-less in-flight orphan poll (no id, nothing to show)", () => { const rows = buildDelegationTaskRows([ statusPoll({ input: "{}", output: null, state: "input-available" }), diff --git a/src/lib/delegation-status.ts b/src/lib/delegation-status.ts index c744c7f4f..28f5d50b5 100644 --- a/src/lib/delegation-status.ts +++ b/src/lib/delegation-status.ts @@ -17,9 +17,15 @@ * - hosts that surface only `CallToolResult.content` text — e.g. Claude Code * via claude-agent-acp — give no structured fields, so the badge is derived * from the tool-call state / `is_error`, with no duration. + * + * On top of that, a host may wrap the result in an envelope of its own — Codex's + * live wire sends `{result: , error: null}`. `parseResultObject` + * peels those first (see `@/lib/mcp-result-envelope`), so the shapes below only + * ever face the result itself. */ import { extractEmbeddedJsonObject } from "@/lib/embedded-json" +import { peelMcpResultEnvelope } from "@/lib/mcp-result-envelope" import { isUnsettledToolCall } from "@/lib/tool-call-lifecycle" import type { AdaptedToolCallPart, @@ -131,6 +137,33 @@ function firstContentText(envelope: Record): string | null { return first ? str(first, "text") : null } +/** Whether `obj` is already one of the shapes the resolution below reads — a + * report, a batch, or an MCP content envelope. Stops the host-envelope peel at + * a result that itself happens to carry a `result` key. (A child's arbitrary + * payload is guarded on the other side too: `peelMcpResultEnvelope` only ever + * peels TO a real `CallToolResult`.) */ +function isResolvableResult(obj: Record): boolean { + return ( + validStatus(obj) !== null || + Array.isArray(obj.tasks) || + Array.isArray(obj.content) || + asObject(obj.structuredContent) !== null + ) +} + +type ParsedResult = { + obj: Record | null + /** codex-acp's `rawOutput.error` — the only text worth showing when the MCP + * call itself failed and carried no result. */ + hostError: string | null +} + +const NO_RESULT: ParsedResult = { obj: null, hostError: null } + +function peeled(obj: Record | null): ParsedResult { + return obj ? peelMcpResultEnvelope(obj, isResolvableResult) : NO_RESULT +} + /** * Parse a tool-result string into its envelope/report object, peeling one layer * of double-encoding (JSON-of-JSON). Some hosts re-stringify the RESULT text @@ -138,24 +171,33 @@ function firstContentText(envelope: Record): string | null { * yields a *string* rather than the payload object. Re-parse that once — falling * back to the embedded-JSON scanner so a re-stringified Codex "Wall time:… * Output:" wrap still resolves. A parse that yields a non-string non-object - * (array, number) is null, matching the prior inline behavior. Shared by - * [`parseStatusReport`] and [`parseStatusReports`] so both honor the same shapes. + * (array, number) is null, matching the prior inline behavior. + * + * The parsed object is then stripped of any host envelope around the MCP result + * (`peelMcpResultEnvelope`) — codex's LIVE wire hands us + * `{result: , error: null}`, which otherwise resolves to no + * report at all and paints the raw envelope JSON into the card. (Codex's + * PERSISTED rollout carries the bare `Wall time:…\nOutput:\n` instead, so + * only the live path was affected.) Shared by [`parseStatusReport`] and + * [`parseStatusReports`] so both honor the same shapes. */ -function parseResultObject(raw: string): Record | null { +function parseResultObject(raw: string): ParsedResult { let parsed: unknown try { parsed = JSON.parse(raw) } catch { - return extractEmbeddedJsonObject(raw) + return peeled(extractEmbeddedJsonObject(raw)) } if (typeof parsed === "string") { try { - return asObject(JSON.parse(parsed)) ?? extractEmbeddedJsonObject(parsed) + return peeled( + asObject(JSON.parse(parsed)) ?? extractEmbeddedJsonObject(parsed) + ) } catch { - return extractEmbeddedJsonObject(parsed) + return peeled(extractEmbeddedJsonObject(parsed)) } } - return asObject(parsed) + return peeled(asObject(parsed)) } // Wrapper keys hosts use to nest the actual tool arguments (mirrors @@ -312,7 +354,7 @@ export function parseStatusReport( const raw = (output ?? errorText ?? "").trim() if (!raw) return empty - const obj = parseResultObject(raw) + const { obj, hostError } = parseResultObject(raw) // Plain text (no recoverable JSON) — the historical content-only shape. The // only structured hint left is the backend's running sentinel sentence. if (!obj) return { ...empty, status: textRunningStatus(raw), text: raw } @@ -350,7 +392,9 @@ export function parseStatusReport( // Parsed JSON but no report (e.g. a content envelope stripped of // structuredContent) — still honor the running sentinel in the display text. - const fallbackText = contentText ?? raw + // A host envelope that failed outright carries no result to show, so its own + // error string beats dumping the envelope JSON. + const fallbackText = contentText ?? hostError ?? raw return { ...empty, status: textRunningStatus(fallbackText), @@ -412,7 +456,7 @@ export function parseStatusReports( ): StatusReport[] { const raw = (output ?? errorText ?? "").trim() if (raw) { - const obj = parseResultObject(raw) + const { obj } = parseResultObject(raw) if (obj) { const found = findTasksArray(obj) if ( diff --git a/src/lib/mcp-result-envelope.ts b/src/lib/mcp-result-envelope.ts new file mode 100644 index 000000000..babdde9f6 --- /dev/null +++ b/src/lib/mcp-result-envelope.ts @@ -0,0 +1,108 @@ +/** + * Peel the host envelopes that wrap an MCP `CallToolResult` on its way to a + * tool card. + * + * codex-acp forwards EVERY MCP tool call's outcome to the ACP wire as + * `rawOutput = { result: | null, error: | null }` + * (its `createMcpRawOutput`), and codex's own rollout tags the same result under + * a serde `{ Ok: … }` variant. Neither layer is part of the result the codeg-mcp + * companion actually returned, so a card that reads a companion result has to + * strip them first — otherwise the whole envelope falls through as opaque text + * and the card renders raw JSON instead of the report inside it. + * + * The peel is deliberately narrow: a tool result is only ever a *child agent's* + * arbitrary payload away from being mangled, so both the destination and the + * failure case are positively identified (a `CallToolResult` under the wrapper + * key; a `result`-bearing envelope for the error string) rather than pattern- + * matched on key names alone. A payload that merely happens to own a `result` or + * `error` key is left exactly as it was. + * + * `ask-question.ts::parseOutcomeJson` peels the same layers inline for its own + * shapes; this module is the reusable form for the delegation cards. + */ + +/** + * Keys a host uses to nest the actual `CallToolResult`. `result` is codex-acp's + * live-wire envelope; `Ok`/`ok` are the serde-tagged `Result` variant codex + * writes into its rollout. + */ +const RESULT_ENVELOPE_KEYS = ["result", "Ok", "ok"] as const + +/** Depth cap — one host layer plus a serde tag is the deepest shape seen. */ +const MAX_PEEL_DEPTH = 3 + +export type PeeledMcpResult = { + /** The `CallToolResult` reached by peeling, or the input unchanged when no + * host envelope was positively identified. */ + obj: Record + /** codex-acp's `rawOutput.error`, read ONLY from an envelope that carries a + * `result` key with nothing in it — i.e. the MCP call failed outright and + * that string is all there is to show. Null in every other case, including a + * payload that just happens to have an `error` field. */ + hostError: string | null +} + +/** + * Whether `value` is an MCP `CallToolResult` — the only thing worth peeling TO. + * Requiring this of the destination (not just the wrapper key's presence) is + * what keeps a child's own `{result: {...}}` payload from being unwrapped and + * then misread as a report by the caller. + */ +function isCallToolResult(value: unknown): value is Record { + if (!value || typeof value !== "object" || Array.isArray(value)) return false + const obj = value as Record + const structured = obj.structuredContent + return ( + Array.isArray(obj.content) || + (typeof structured === "object" && + structured !== null && + !Array.isArray(structured)) + ) +} + +/** + * The error string of a host FAILURE envelope — `{result: null, error: "…"}`. + * codex-acp always emits both keys, so requiring a present-but-empty `result` + * alongside the string is what separates a failed MCP call from a child payload + * that merely has an `error` field of its own. + */ +function hostFailureError(obj: Record): string | null { + if (!("result" in obj)) return null + if (obj.result !== null && obj.result !== undefined) return null + const err = obj.error + return typeof err === "string" && err.trim() ? err : null +} + +/** + * Strip host `{ result, error }` / `{ Ok }` layers from `obj`. + * + * `isResolvable` is the caller's own "I can already read this shape" predicate + * (a report, a batch, an MCP content envelope, …). Peeling stops as soon as it + * holds, so a `CallToolResult` that itself owns a `result` key is never unwrapped + * out from under the caller. + */ +export function peelMcpResultEnvelope( + obj: Record, + isResolvable: (candidate: Record) => boolean +): PeeledMcpResult { + let current = obj + for ( + let depth = 0; + depth < MAX_PEEL_DEPTH && !isResolvable(current); + depth++ + ) { + let next: Record | null = null + for (const key of RESULT_ENVELOPE_KEYS) { + const value = current[key] + if (isCallToolResult(value)) { + next = value + break + } + } + // Nothing peelable left: this is either the payload itself or a host + // envelope whose call failed before producing a result. + if (!next) return { obj: current, hostError: hostFailureError(current) } + current = next + } + return { obj: current, hostError: null } +} From df5ee401c94683a25d2a2f1147d01950b4572219 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Sat, 25 Jul 2026 14:30:08 +0800 Subject: [PATCH 2/8] fix(acp): correct the fs write policy for Windows builds and tests - `temp_roots` pushed the shared unix temp dirs from inside a `#[cfg(unix)]` block, leaving the `mut` binding unused on Windows, where `-D warnings` turns that into a compile error. The pushes now sit behind `if cfg!(unix)`; the resolved roots are unchanged on every platform. - The policy tests built their fixtures from unix path literals such as `/tmp/codeg-xdg`. Those carry a root but no drive prefix, so they are relative on Windows and the module's fail-closed guards correctly dropped them, leaving the assertions with an empty root list. Fixtures now come from a platform-prefixed helper, with the environment value and the expected root derived from the same call. - The assertions that a padded value never widens to the filesystem root could not fail on Windows, since `/` is never produced there. They now use the platform's own root, and the write-gate check compares against the canonical form so the verbatim `\\?\C:\` that Windows canonicalizes to is still caught. --- src-tauri/src/acp/file_system_runtime.rs | 97 +++++++++++++++++------- 1 file changed, 68 insertions(+), 29 deletions(-) diff --git a/src-tauri/src/acp/file_system_runtime.rs b/src-tauri/src/acp/file_system_runtime.rs index 2524f1479..d7ae37133 100644 --- a/src-tauri/src/acp/file_system_runtime.rs +++ b/src-tauri/src/acp/file_system_runtime.rs @@ -226,8 +226,11 @@ fn extra_write_roots(runtime_env: &BTreeMap) -> Vec { /// — hence `canonical_root` on every entry). fn temp_roots() -> Vec { let mut roots = vec![std::env::temp_dir()]; - #[cfg(unix)] - { + // The shared unix temp dirs are extra roots on top of the per-user one; + // Windows has none to add. Gated with a runtime `cfg!` rather than + // `#[cfg(unix)]` so the binding is still mutated on Windows — a `#[cfg]` + // block leaves `mut` unused there, which `-D warnings` rejects. + if cfg!(unix) { roots.push(PathBuf::from("/tmp")); roots.push(PathBuf::from("/var/tmp")); } @@ -946,6 +949,24 @@ mod tests { path } + /// Drive prefix that makes a rooted path ABSOLUTE on this platform. On + /// Windows `\tmp\x` is rooted but not absolute (it is relative to the + /// current drive), so a literal unix path would be dropped by the very + /// relative-path guards these tests exercise. + #[cfg(windows)] + const ABS_PREFIX: &str = "C:"; + #[cfg(not(windows))] + const ABS_PREFIX: &str = ""; + + /// An absolute path for the HOST platform, built from unix-style segments. + /// `absolute_path("")` is the filesystem root itself (`/`, or `C:/`). + /// + /// None of these paths are ever opened — they only have to be absolute, so + /// the synthetic drive letter needs no counterpart on disk. + fn absolute_path(segments: &str) -> PathBuf { + PathBuf::from(format!("{ABS_PREFIX}/{segments}")) + } + #[tokio::test(flavor = "current_thread")] async fn read_honors_line_and_limit() { let workspace = temp_workspace(); @@ -1297,7 +1318,7 @@ mod tests { /// fallback) are covered by `relocation_suffixes_match_the_agents_own_resolvers`. #[test] fn agent_data_roots_honor_runtime_env_relocation() { - let relocated = PathBuf::from("/tmp/codeg-relocated-agent-home"); + let relocated = absolute_path("tmp/codeg-relocated-agent-home"); let cases = [ (AgentType::Grok, "GROK_HOME"), (AgentType::ClaudeCode, "CLAUDE_CONFIG_DIR"), @@ -1327,7 +1348,7 @@ mod tests { /// writable, which for `GEMINI_CLI_HOME=$HOME` is the whole home directory. #[test] fn relocation_suffixes_match_the_agents_own_resolvers() { - let base = PathBuf::from("/tmp/codeg-relocation-base"); + let base = absolute_path("tmp/codeg-relocation-base"); let cases = [ (AgentType::Gemini, "GEMINI_CLI_HOME", ".gemini"), (AgentType::OpenCode, "XDG_DATA_HOME", "opencode"), @@ -1407,7 +1428,7 @@ mod tests { /// inactive tree holds config and credentials, so it must not stay writable. #[test] fn relocation_excludes_the_inactive_default_root() { - let relocated = PathBuf::from("/tmp/codeg-isolated-profile"); + let relocated = absolute_path("tmp/codeg-isolated-profile"); let cases = [ (AgentType::Codex, "CODEX_HOME"), (AgentType::Grok, "GROK_HOME"), @@ -1504,14 +1525,18 @@ mod tests { /// not to the masked key's inherited value. #[test] fn blank_runtime_value_falls_through_to_the_next_candidate() { + let xdg = absolute_path("tmp/codeg-xdg"); let roots = agent_data_roots( AgentType::Cursor, &BTreeMap::from([ ("CURSOR_CONFIG_DIR".to_string(), String::new()), - ("XDG_CONFIG_HOME".to_string(), "/tmp/codeg-xdg".to_string()), + ( + "XDG_CONFIG_HOME".to_string(), + xdg.to_string_lossy().to_string(), + ), ]), ); - assert_eq!(roots, vec![PathBuf::from("/tmp/codeg-xdg/cursor")]); + assert_eq!(roots, vec![xdg.join("cursor")]); } /// `child_env_value` must distinguish the ways a var reaches (or fails to @@ -1723,16 +1748,17 @@ mod tests { /// Same hazard through the user-facing knob. #[test] fn relative_extra_roots_are_dropped() { + let absolute = absolute_path("tmp/codeg-abs-extra"); let runtime_env = BTreeMap::from([( FS_EXTRA_ROOTS_ENV.to_string(), - std::env::join_paths([Path::new("."), Path::new("/tmp/codeg-abs-extra")]) + std::env::join_paths([Path::new("."), absolute.as_path()]) .expect("join paths") .to_string_lossy() .to_string(), )]); let roots = extra_write_roots(&runtime_env); - assert_eq!(roots, vec![PathBuf::from("/tmp/codeg-abs-extra")]); + assert_eq!(roots, vec![absolute]); } /// The extra-roots list must be read VERBATIM. `split_paths` does not trim @@ -1740,20 +1766,22 @@ mod tests { /// `" / "` into `/` and hand out the entire filesystem. #[test] fn whitespace_padded_extra_root_is_not_trimmed_into_filesystem_root() { - for padded in [" / ", "\t/", "/ "] { - let runtime_env = - BTreeMap::from([(FS_EXTRA_ROOTS_ENV.to_string(), padded.to_string())]); + let root = absolute_path(""); + let raw = root.to_string_lossy().to_string(); + + for padded in [format!(" {raw} "), format!("\t{raw}"), format!("{raw} ")] { + let runtime_env = BTreeMap::from([(FS_EXTRA_ROOTS_ENV.to_string(), padded.clone())]); let roots = extra_write_roots(&runtime_env); assert!( - !roots.contains(&PathBuf::from("/")), + !roots.contains(&root), "{padded:?} must not become the filesystem root, got {roots:?}" ); } - // An entry that IS exactly `/` stays honored — widening is this knob's - // declared purpose, so that is the user's explicit choice. - let runtime_env = BTreeMap::from([(FS_EXTRA_ROOTS_ENV.to_string(), "/".to_string())]); - assert_eq!(extra_write_roots(&runtime_env), vec![PathBuf::from("/")]); + // An entry that IS exactly the filesystem root stays honored — widening + // is this knob's declared purpose, so that is the user's explicit choice. + let runtime_env = BTreeMap::from([(FS_EXTRA_ROOTS_ENV.to_string(), raw)]); + assert_eq!(extra_write_roots(&runtime_env), vec![root]); } /// Trimming a relocation value can WIDEN the policy, not just fail to match: @@ -1761,27 +1789,34 @@ mod tests { /// filesystem root and would make everything writable. #[test] fn whitespace_padded_root_never_becomes_the_filesystem_root() { + let root = absolute_path(""); + let padded = format!(" {} ", root.to_string_lossy()); + for (agent_type, key) in [ (AgentType::Codex, "CODEX_HOME"), (AgentType::Grok, "GROK_HOME"), (AgentType::Pi, "PI_CODING_AGENT_DIR"), ] { - let runtime_env = BTreeMap::from([(key.to_string(), " / ".to_string())]); + let runtime_env = BTreeMap::from([(key.to_string(), padded.clone())]); let roots = agent_data_roots(agent_type, &runtime_env); assert!( - !roots.contains(&PathBuf::from("/")), - "{agent_type:?}: {key}=\" / \" must not resolve to the filesystem root, \ - got {roots:?}" + !roots.contains(&root), + "{agent_type:?}: {key}={padded:?} must not resolve to the filesystem \ + root, got {roots:?}" ); } - // And the same value must not slip through the write gate either. + // And the same value must not slip through the write gate either. Compared + // against the CANONICAL root because that is the form `permissive` stores — + // on Windows `canonicalize("C:/")` is the verbatim `\\?\C:\`, so comparing + // the raw spelling would make this assertion unfalsifiable there. let workspace = temp_workspace(); - let runtime_env = BTreeMap::from([("CODEX_HOME".to_string(), " / ".to_string())]); + let runtime_env = BTreeMap::from([("CODEX_HOME".to_string(), padded)]); let policy = FsAccessPolicy::permissive(&workspace, AgentType::Codex, &runtime_env); assert!( - !policy.write_roots.contains(&PathBuf::from("/")), - "write roots must not include /: {:?}", + !policy.write_roots.contains(&canonical_root(&root)), + "write roots must not include {}: {:?}", + root.display(), policy.write_roots ); @@ -1853,16 +1888,20 @@ mod tests { /// first match must win rather than both being admitted. #[test] fn cursor_relocation_respects_resolver_precedence() { + let config_dir = absolute_path("tmp/codeg-cursor"); let runtime_env = BTreeMap::from([ ( "CURSOR_CONFIG_DIR".to_string(), - "/tmp/codeg-cursor".to_string(), + config_dir.to_string_lossy().to_string(), + ), + ( + "XDG_CONFIG_HOME".to_string(), + absolute_path("tmp/codeg-xdg").to_string_lossy().to_string(), ), - ("XDG_CONFIG_HOME".to_string(), "/tmp/codeg-xdg".to_string()), ]); let roots = agent_data_roots(AgentType::Cursor, &runtime_env); - assert_eq!(roots, vec![PathBuf::from("/tmp/codeg-cursor")]); + assert_eq!(roots, vec![config_dir]); } /// pi relocates its session store independently of its agent home, so a @@ -1870,7 +1909,7 @@ mod tests { /// roots on its own — the process-env-only resolver cannot see it. #[test] fn pi_session_dir_relocation_is_honored() { - let sessions = PathBuf::from("/tmp/codeg-pi-sessions-elsewhere"); + let sessions = absolute_path("tmp/codeg-pi-sessions-elsewhere"); let runtime_env = BTreeMap::from([( "PI_CODING_AGENT_SESSION_DIR".to_string(), sessions.to_string_lossy().to_string(), From adc567608dffafb76c29e13d46a82903e36cbb58 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Sat, 25 Jul 2026 14:54:52 +0800 Subject: [PATCH 3/8] feat(git-log): cap a commit's message body with a show more toggle An expanded commit in the aux panel's Commits tab rendered its message in full, so a long release message pushed the file tree and containing branches far off the bottom of the panel. The body is now capped at 12rem with a bottom fade and a Show more / Show less toggle that appears only once the text is actually clipped, matching how a chat user message behaves. The clamp-and-measure plumbing behind those user messages moves into a shared useCollapsibleOverflow hook that both call sites use, and the fade class it applies is renamed collapsed-user-message-fade -> collapsed-content-fade to match. Show more / Show less are added under Folder.gitLogTab in all ten locales, reusing each locale's existing chat wording. --- src/app/globals.css | 8 +- .../layout/aux-panel-git-log-tab.tsx | 17 ++- .../layout/git-log-commit-message.test.tsx | 111 ++++++++++++++++++ .../layout/git-log-commit-message.tsx | 68 +++++++++++ .../message/collapsible-user-message.test.tsx | 6 +- .../message/collapsible-user-message.tsx | 34 +----- src/hooks/use-collapsible-overflow.ts | 58 +++++++++ src/i18n/messages/ar.json | 2 + src/i18n/messages/de.json | 2 + src/i18n/messages/en.json | 2 + src/i18n/messages/es.json | 2 + src/i18n/messages/fr.json | 2 + src/i18n/messages/ja.json | 2 + src/i18n/messages/ko.json | 2 + src/i18n/messages/pt.json | 2 + src/i18n/messages/zh-CN.json | 2 + src/i18n/messages/zh-TW.json | 2 + 17 files changed, 277 insertions(+), 45 deletions(-) create mode 100644 src/components/layout/git-log-commit-message.test.tsx create mode 100644 src/components/layout/git-log-commit-message.tsx create mode 100644 src/hooks/use-collapsible-overflow.ts diff --git a/src/app/globals.css b/src/app/globals.css index 93dae2428..0018d1228 100644 --- a/src/app/globals.css +++ b/src/app/globals.css @@ -1123,10 +1123,10 @@ box-shadow: inset 0 0 0 1px var(--border); } -/* 折叠态用户消息底部渐隐：遮罩内容本身而非叠加层，跟 .qa-marquee-viewport / - .browser-tab-label 同技术，天然适配纯色 bg-secondary 或开工作区背景图后的 - 半透明 ws-msg-secondary。不属于下面的 ws- 门控家族，始终生效。 */ -.collapsed-user-message-fade { +/* 折叠态内容底部渐隐（会话用户消息、提交 tab 的提交信息…）：遮罩内容本身而非叠加层，跟 + .qa-marquee-viewport / .browser-tab-label 同技术，天然适配纯色 bg-secondary 或开工作区 + 背景图后的半透明 ws-msg-secondary。不属于下面的 ws- 门控家族，始终生效。 */ +.collapsed-content-fade { -webkit-mask-image: linear-gradient( to bottom, #000 calc(100% - 2.5rem), diff --git a/src/components/layout/aux-panel-git-log-tab.tsx b/src/components/layout/aux-panel-git-log-tab.tsx index 2883fc941..d291bd7f1 100644 --- a/src/components/layout/aux-panel-git-log-tab.tsx +++ b/src/components/layout/aux-panel-git-log-tab.tsx @@ -84,6 +84,7 @@ import { } from "@/components/ui/command" import { Skeleton } from "@/components/ui/skeleton" import { AuxPanelNoFolderEmpty } from "@/components/layout/aux-panel-no-folder-empty" +import { GitLogCommitMessage } from "@/components/layout/git-log-commit-message" import { subscribe } from "@/lib/platform" import { useActiveFolder } from "@/contexts/active-folder-context" import { useWorkspaceActions } from "@/contexts/workspace-context" @@ -2296,16 +2297,12 @@ export function GitLogTab() { -
-

- {entry.message} -

- -
+ {/* Long release messages are capped with a + Show more / Show less toggle so the file + tree below stays reachable. */} + {/* File changes load lazily on expand (the list query runs with withFiles=false). CommitFilesTree renders its own "Files" diff --git a/src/components/layout/git-log-commit-message.test.tsx b/src/components/layout/git-log-commit-message.test.tsx new file mode 100644 index 000000000..0db1b77bb --- /dev/null +++ b/src/components/layout/git-log-commit-message.test.tsx @@ -0,0 +1,111 @@ +import { type ReactElement } from "react" +import { fireEvent, render, screen } from "@testing-library/react" +import { NextIntlClientProvider } from "next-intl" +import { afterEach, describe, expect, it } from "vitest" + +import { GitLogCommitMessage } from "./git-log-commit-message" +import enMessages from "@/i18n/messages/en.json" + +function renderWithIntl(ui: ReactElement) { + return render( + + {ui} + + ) +} + +const MESSAGE = "feat: add a thing\n\nWith a body." + +// jsdom does no layout: scrollHeight/clientHeight both default to 0, which +// already reads as "not overflowing" for the short-message case. The overflow +// cases patch both onto Element.prototype *before* rendering so the +// synchronous mount-time measurement picks them up (the global ResizeObserver +// stub in test-setup.ts never invokes its callback). Mirrors +// collapsible-user-message.test.tsx. +function mockScrollMetrics(scrollHeight: number, clientHeight: number) { + const scrollHeightDescriptor = Object.getOwnPropertyDescriptor( + Element.prototype, + "scrollHeight" + ) + const clientHeightDescriptor = Object.getOwnPropertyDescriptor( + Element.prototype, + "clientHeight" + ) + Object.defineProperty(Element.prototype, "scrollHeight", { + configurable: true, + get: () => scrollHeight, + }) + Object.defineProperty(Element.prototype, "clientHeight", { + configurable: true, + get: () => clientHeight, + }) + return () => { + if (scrollHeightDescriptor) { + Object.defineProperty( + Element.prototype, + "scrollHeight", + scrollHeightDescriptor + ) + } + if (clientHeightDescriptor) { + Object.defineProperty( + Element.prototype, + "clientHeight", + clientHeightDescriptor + ) + } + } +} + +describe("GitLogCommitMessage", () => { + let restoreMetrics: (() => void) | null = null + + afterEach(() => { + restoreMetrics?.() + restoreMetrics = null + }) + + it("renders a short message clamped but with no toggle", () => { + renderWithIntl() + + const content = screen.getByTestId("git-log-commit-message-content") + expect(content).toHaveTextContent("feat: add a thing") + expect( + screen.queryByTestId("git-log-commit-message-toggle") + ).not.toBeInTheDocument() + expect(content).not.toHaveClass("collapsed-content-fade") + }) + + it("shows a Show more toggle when the message overflows the cap", () => { + restoreMetrics = mockScrollMetrics(900, 192) + + renderWithIntl() + + const content = screen.getByTestId("git-log-commit-message-content") + const toggle = screen.getByTestId("git-log-commit-message-toggle") + expect(toggle).toHaveAttribute("aria-expanded", "false") + expect(toggle).toHaveTextContent("Show more") + expect(toggle).toHaveAttribute("aria-controls", content.id) + expect(content).toHaveClass("max-h-48", "collapsed-content-fade") + }) + + it("expands to Show less on click and removes the clamp", () => { + restoreMetrics = mockScrollMetrics(900, 192) + + renderWithIntl() + + fireEvent.click(screen.getByTestId("git-log-commit-message-toggle")) + + const toggle = screen.getByTestId("git-log-commit-message-toggle") + expect(toggle).toHaveAttribute("aria-expanded", "true") + expect(toggle).toHaveTextContent("Show less") + const content = screen.getByTestId("git-log-commit-message-content") + expect(content).not.toHaveClass("max-h-48") + expect(content).not.toHaveClass("collapsed-content-fade") + + // Clicking again re-collapses. + fireEvent.click(toggle) + expect(toggle).toHaveAttribute("aria-expanded", "false") + expect(toggle).toHaveTextContent("Show more") + }) +}) diff --git a/src/components/layout/git-log-commit-message.tsx b/src/components/layout/git-log-commit-message.tsx new file mode 100644 index 000000000..198a7280f --- /dev/null +++ b/src/components/layout/git-log-commit-message.tsx @@ -0,0 +1,68 @@ +"use client" + +import { memo } from "react" +import { useTranslations } from "next-intl" +import { ChevronDown, ChevronUp } from "lucide-react" + +import { CommitCopyButton } from "@/components/ai-elements/commit" +import { useCollapsibleOverflow } from "@/hooks/use-collapsible-overflow" +import { cn } from "@/lib/utils" + +/** + * An expanded commit's full message body. Release commits can run to dozens of + * lines, which would push the file tree and branch list far off the bottom of + * the (narrow) aux panel, so the body is capped and gets a "Show more"/"Show + * less" toggle once it's actually clipped — same treatment as a chat user + * message (see `CollapsibleUserMessage`), shared via `useCollapsibleOverflow`. + */ +export const GitLogCommitMessage = memo(function GitLogCommitMessage({ + message, +}: { + message: string +}) { + const t = useTranslations("Folder.gitLogTab") + const { contentRef, contentId, isOverflowing, expanded, toggle } = + useCollapsibleOverflow(message) + + const clipped = !expanded + + return ( +
+

+ {message} +

+ {isOverflowing && ( + + )} + +
+ ) +}) diff --git a/src/components/message/collapsible-user-message.test.tsx b/src/components/message/collapsible-user-message.test.tsx index b7266e935..497e75871 100644 --- a/src/components/message/collapsible-user-message.test.tsx +++ b/src/components/message/collapsible-user-message.test.tsx @@ -76,7 +76,7 @@ describe("CollapsibleUserMessage", () => { ).not.toBeInTheDocument() expect( screen.getByTestId("collapsible-user-message-content") - ).not.toHaveClass("collapsed-user-message-fade") + ).not.toHaveClass("collapsed-content-fade") }) it("shows a Show more toggle when content overflows the collapsed height", () => { @@ -89,7 +89,7 @@ describe("CollapsibleUserMessage", () => { expect(toggle).toHaveAttribute("aria-expanded", "false") expect(toggle).toHaveTextContent("Show more") expect(toggle).toHaveAttribute("aria-controls", content.id) - expect(content).toHaveClass("max-h-60", "collapsed-user-message-fade") + expect(content).toHaveClass("max-h-60", "collapsed-content-fade") }) it("expands to Show less on click and removes the clamp", () => { @@ -104,7 +104,7 @@ describe("CollapsibleUserMessage", () => { expect(toggle).toHaveTextContent("Show less") const content = screen.getByTestId("collapsible-user-message-content") expect(content).not.toHaveClass("max-h-60") - expect(content).not.toHaveClass("collapsed-user-message-fade") + expect(content).not.toHaveClass("collapsed-content-fade") // Clicking again re-collapses. fireEvent.click(toggle) diff --git a/src/components/message/collapsible-user-message.tsx b/src/components/message/collapsible-user-message.tsx index 28a73a380..f39603852 100644 --- a/src/components/message/collapsible-user-message.tsx +++ b/src/components/message/collapsible-user-message.tsx @@ -1,11 +1,12 @@ "use client" -import { memo, useEffect, useId, useRef, useState } from "react" +import { memo } from "react" import { useTranslations } from "next-intl" import { ChevronDown, ChevronUp } from "lucide-react" import { cn } from "@/lib/utils" import type { AdaptedContentPart } from "@/lib/adapters/ai-elements-adapter" +import { useCollapsibleOverflow } from "@/hooks/use-collapsible-overflow" import { ContentPartsRenderer } from "./content-parts-renderer" @@ -22,31 +23,8 @@ export const CollapsibleUserMessage = memo(function CollapsibleUserMessage({ parts: AdaptedContentPart[] }) { const t = useTranslations("Folder.chat.messageList") - const contentRef = useRef(null) - const [isOverflowing, setIsOverflowing] = useState(false) - const [expanded, setExpanded] = useState(false) - const contentId = useId() - - useEffect(() => { - // Nothing useful to redetect once expanded: the clamp class below is - // removed, so clientHeight === scrollHeight trivially and this would - // misreport `false`, dropping the "Show less" toggle. Freeze the last - // known value instead. - if (expanded) return - const el = contentRef.current - if (!el) return - const measure = () => { - // Both reads are on this same, currently `max-h-60`-clamped node: no - // numeric threshold duplicated from CSS. clientHeight is capped by the - // class below; scrollHeight always reports the untruncated height. - setIsOverflowing(el.scrollHeight > el.clientHeight + 1) - } - measure() // Synchronous initial read — doesn't depend on the - // ResizeObserver callback firing (the jsdom test stub never invokes it). - const observer = new ResizeObserver(measure) - observer.observe(el) - return () => observer.disconnect() - }, [parts, expanded]) + const { contentRef, contentId, isOverflowing, expanded, toggle } = + useCollapsibleOverflow(parts) const clipped = !expanded @@ -59,7 +37,7 @@ export const CollapsibleUserMessage = memo(function CollapsibleUserMessage({ className={cn( "min-w-0", clipped && "max-h-60 overflow-hidden", - clipped && isOverflowing && "collapsed-user-message-fade" + clipped && isOverflowing && "collapsed-content-fade" )} > @@ -68,7 +46,7 @@ export const CollapsibleUserMessage = memo(function CollapsibleUserMessage({