diff --git a/docs/add-project-preflight.md b/docs/add-project-preflight.md index 8cadfc6fb..2804c996f 100644 --- a/docs/add-project-preflight.md +++ b/docs/add-project-preflight.md @@ -111,9 +111,13 @@ The allowlist covers (paths are relative to `/.bobbit/`): `state/sessions.json`, `state/projects.json`. - TLS / DNS challenge state — `state/tls/`, `state/desec.json`. - Restart caches — `state/tool-docs/`, `state/mcp-tool-docs/`. -- Per-session scratch — `state/preview/`, `state/tool-guard/`, - `state/mcp-extensions/`, `state/html-snapshots/`, `state/proposal-drafts/`, - `state/sessions/`, `state/session-prompts/`, `state/system-project/`, and +- Per-session scratch and generated extension state — `state/preview/`, + `state/preview-artifacts/`, `state/tool-guard/`, `state/provider-bridge/`, + `state/google-code-assist/`, `state/openai-orphan-tool-result/`, + `state/mcp-extensions/`, `state/html-snapshots/`, `state/gate-diagnostics/`, + `state/proposal-drafts/`, `state/pr-walkthrough/`, `state/sessions/`, + `state/session-prompts/`, `state/sandbox-agent-auth/`, + `state/system-project/`, `state/marketplace-cache/`, and `state/model-name-*` files. When `bobbit.gateway-owned` is `pass` (the common case — the gateway runs diff --git a/docs/debugging.md b/docs/debugging.md index bac958cfd..85c0e9305 100644 --- a/docs/debugging.md +++ b/docs/debugging.md @@ -105,6 +105,13 @@ The pill strip above the composer (`AgentInterface._renderPillStrip`, `_measureP - Tool-call messages stay in streaming until the next message starts. - See [docs/internals.md — Reducer ordering invariant](internals.md#reducer-ordering-invariant) for the single-sort-key contract. +## OpenAI/Codex session stuck on orphan tool results + +- **Symptom**: an OpenAI Responses or Codex session fails every later turn after restore/compaction/abort because the request contains a `function_call_output` without a matching retained `function_call`. +- **Where to look**: restore-boundary repair in `transcript-sanitizer.ts`; request preflight in `openai-orphan-tool-result-extension.ts`; sandbox mount policy in `docker-args.ts`. +- **Key detail**: valid tool-call/result pairs stay byte-identical, but results following aborted/errored assistant tool-call rows, compaction boundaries, or missing calls are dropped/filtered with bounded count-only diagnostics. +- **Reference**: [Orphan tool-result hardening](orphan-tool-result-hardening.md). + ## Blob stuck idle while streaming (zzz visible with stop button) - **Symptom**: chat blob shows the desaturated idle sprite with floating `zzz` while the agent is actively streaming (stop button visible, tool calls running). Stays wrong until the next `isStreaming` transition. Most reproducible by sending a new message immediately after the previous turn ends. @@ -1308,5 +1315,5 @@ Pinned by `tests/error-modal-call-sites.test.ts` (enumerates every modal call si - **Symptom**: sending a prompt with only an image/attachment and no typed text fails with `Validation error: the text field in the ContentBlock at messages … is blank.` The session then stays broken — even retries that include text re-fail with the same error. - **Cause**: the model API rejects a user message whose `ContentBlock` has a blank `text` field (next to an image, or standalone). The blank block was committed to the agent's `.jsonl` transcript, so every later turn replayed it. See [docs/image-attachment-only-prompts.md](image-attachment-only-prompts.md) for the full design. - **Source-prevention fix**: `synthesizeAttachmentText` (`rpc-bridge.ts`) substitutes the synthetic body `ATTACHMENT_ONLY_TEXT` (`"Attachments:"`) when text is blank/whitespace AND an image/attachment is present. Applied at the dispatch boundary in `SessionManager.enqueuePrompt` so direct/queued/recovery/retry paths all inherit valid text; backstopped in `RpcBridge.prompt` for the image case. -- **Recovery fix (already-broken sessions)**: `isBlankContentBlockError` detects the poison; the transcript sanitizer (`transcript-sanitizer.ts`) rewrites blank-text user messages to `"Attachments:"` at the rehydration boundary (it never touches `tool_result` user messages and hardens the write path against symlink/traversal); `_recoverBlankTextPoison` respawns the live agent so it rehydrates from the clean transcript. -- **Pinning tests**: `tests/synthesize-attachment-text.test.ts`, `tests/image-only-prompt-dispatch.test.ts`, `tests/image-only-prompt-unstick-recovery.test.ts`, `tests/transcript-sanitizer.test.ts`. +- **Recovery fix (already-broken sessions)**: `isBlankContentBlockError` detects the poison; the transcript sanitizer (`transcript-sanitizer.ts`) rewrites blank-text user messages to `"Attachments:"` at the rehydration boundary, preserves valid matched `tool_result` user messages, and separately drops/filters orphan tool results with bounded repair-count diagnostics; `_recoverBlankTextPoison` respawns the live agent so it rehydrates from the clean transcript. See [docs/orphan-tool-result-hardening.md](orphan-tool-result-hardening.md) for the orphan repair contract. +- **Pinning tests**: `tests/synthesize-attachment-text.test.ts`, `tests/image-only-prompt-dispatch.test.ts`, `tests/image-only-prompt-unstick-recovery.test.ts`, `tests/transcript-sanitizer.test.ts`, `tests/openai-orphan-tool-result-guard.test.ts`. diff --git a/docs/image-attachment-only-prompts.md b/docs/image-attachment-only-prompts.md index e0abc83fe..d842bad96 100644 --- a/docs/image-attachment-only-prompts.md +++ b/docs/image-attachment-only-prompts.md @@ -103,11 +103,18 @@ prior turn was poisoned by a blank block specifically (vs. any other API error). leading text block is inserted), and non-string/non-array content. Every other line is left **byte-identical**, including trailing-newline shape, so re-running is a no-op. -- **Tool results are never touched.** Tool results are also persisted as - `role:"user"` messages with a `tool_result` / `toolResult` block and no text — - that is valid history. Rewriting them to `"Attachments:"` would corrupt - tool-call history and break tool-result ordering, so any user message - carrying a tool-result block is left byte-identical. +- **Valid matched tool results are never rewritten as attachment text.** Tool + results are also persisted as `role:"user"` messages with a `tool_result` / + `toolResult` block and no text — that is valid history when the retained + transcript still contains the matching assistant tool call. Rewriting a valid + matched tool result to `"Attachments:"` would corrupt tool-call history and + break tool-result ordering, so those rows stay byte-identical. +- **Orphan tool results are repaired before blank-text rewriting.** The same + sanitizer now drops message-level tool-result rows and filters user-message + result blocks whose tool-call id is not present in retained valid assistant + history. If filtering leaves ordinary blank user content, the attachment-only + rewrite still applies. See [orphan-tool-result-hardening.md](orphan-tool-result-hardening.md) + for the orphan repair contract and diagnostics. `sanitizeAgentTranscriptFile(...)` wraps the pure function with I/O, called at the **rehydration boundary** in `session-setup.ts` — just before the @@ -165,7 +172,7 @@ invalid content. | `synthesizeAttachmentText` rule (blank/whitespace/text/no-attachment) | `tests/synthesize-attachment-text.test.ts` | | Dispatch boundary: empty/whitespace text + image → non-blank dispatch; stuck-session retry preserves image | `tests/image-only-prompt-dispatch.test.ts` | | Stuck-session recovery via respawn + sanitized rehydrate; legacy non-image case; non-poison errors unaffected | `tests/image-only-prompt-unstick-recovery.test.ts` | -| Sanitizer correctness, idempotency, tool-result protection, path-safety guards | `tests/transcript-sanitizer.test.ts` | +| Sanitizer correctness, idempotency, valid matched tool-result preservation, orphan tool-result repair, path-safety guards | `tests/transcript-sanitizer.test.ts` | ## Related diff --git a/docs/internals.md b/docs/internals.md index a4e465054..45c4a945c 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -2286,7 +2286,7 @@ The existing session restore path (on gateway restart) also benefits from worktr ### Security summary -- Container sees `/workspace`, `/workspace-wt/`, `/agent-modules` (ro), `/tools` (ro), `/bobbit-state/{sessions,tool-guard,html-snapshots}/` (selective mounts - the host gateway token, TLS keys, and other sensitive state files are not mounted), and `/bobbit/preview` (per-session bind-mount of `/preview//` - or `/bobbit/preview-root` for the per-project shared parent; see [`docs/preview-architecture.md`](preview-architecture.md)) +- Container sees `/workspace`, `/workspace-wt/`, `/agent-modules` (ro), `/tools` (ro), `/bobbit-state/{sessions,tool-guard,html-snapshots}/`, read-only `/bobbit-state/{google-code-assist,openai-orphan-tool-result}/` generated-extension mounts (selective mounts - the host gateway token, TLS keys, and other sensitive state files are not mounted), and `/bobbit/preview` (per-session bind-mount of `/preview//` - or `/bobbit/preview-root` for the per-project shared parent; see [`docs/preview-architecture.md`](preview-architecture.md)) - Runs as `node` user (uid=1000), no Docker socket - Mount paths validated against blocklist (`/proc`, `/sys`, `/.ssh`, `/.aws`, etc.) - Credential keys sanitized (`^[A-Za-z_][A-Za-z0-9_]*$`) diff --git a/docs/orphan-tool-result-hardening.md b/docs/orphan-tool-result-hardening.md new file mode 100644 index 000000000..acd5ad75f --- /dev/null +++ b/docs/orphan-tool-result-hardening.md @@ -0,0 +1,107 @@ +# Orphan tool-result hardening + +Bobbit persists agent transcripts as `.jsonl` and later rehydrates them into provider requests. A valid tool result is only valid when the retained context also contains the assistant tool call that produced it. If compaction, abort, or an errored turn leaves a `toolResult` / `tool_result` / OpenAI `function_call_output` without that matching call, OpenAI Responses and Codex can reject every later request and wedge the session. + +This note documents the two-layer repair path that prevents that failure while preserving normal tool history byte-for-byte. + +## Where the fix sits + +There are two defensive boundaries: + +1. **Restore-boundary transcript sanitizer** — `src/server/agent/transcript-sanitizer.ts` repairs persisted agent `.jsonl` before restore / `switch_session` rehydrates the transcript. +2. **OpenAI Responses request guard** — `src/server/agent/openai-orphan-tool-result-extension.ts` writes a generated pi extension that runs immediately before a provider request and filters orphan OpenAI `function_call_output` items from `payload.input`. + +The sanitizer fixes durable history. The OpenAI guard is a final provider-specific preflight for anything still orphaned at request construction time. Together they cover restored sessions, force-abort respawns, compaction boundaries, and late tool results from discarded turns. + +## Restore-boundary sanitizer behavior + +`sanitizeTranscriptContent()` walks the transcript once from oldest to newest and keeps a set of retained, valid tool-call ids. + +It adds assistant tool-call ids only from assistant rows that are not terminally bad: + +- `stopReason: "aborted"` is ignored. +- `stopReason: "error"` is ignored. +- Non-aborted / non-errored assistant rows contribute tool-call ids from the supported shapes (`toolCall`, `tool_use`, and pi-shaped `toolCallId`). + +Then it repairs result shapes against that retained set: + +- Message-level `role: "toolResult"` rows are kept only when `toolCallId` is in the retained set; otherwise the whole row is dropped. +- User-message content blocks with `type: "tool_result"` or `type: "toolResult"` are filtered block-by-block using `tool_use_id`, `toolCallId`, or `id`. +- If filtering removes every content block from a user row, the row is dropped. +- If filtering leaves non-tool content, the existing blank-user-message sanitizer still runs so image/attachment-only prompts are repaired in the same pass. + +Compaction uses the exact retained range when Pi provides one. If an inline `type: "compaction"` entry has a resolvable `firstKeptEntryId`, the sanitizer resolves that id against parsed JSONL entry ids and drops only tracked tool-call ids whose originating assistant line is before that first-kept entry. Tool calls at or after the first-kept entry remain valid even when they appear before the inline marker. If `firstKeptEntryId` is absent or cannot be resolved, the sanitizer uses the legacy marker fallback and clears tracked tool-call ids at the compaction marker, so later result rows cannot match calls from the discarded pre-marker history. + +## Valid-pair preservation and idempotence + +Valid tool-call/tool-result pairs are deliberately not normalized. When an assistant tool call is retained and its result follows it, the sanitizer leaves both rows byte-identical, including blank text that is part of a valid tool-result user message. + +The sanitizer is idempotent: + +- A clean transcript returns `changed: false` and unchanged content. +- A repaired transcript becomes clean after one pass. +- Re-running on repaired output is a no-op. +- Non-JSON lines, non-message records, and unrelated message rows are preserved. + +This matters because the restore path can run more than once across restart, refresh, and respawn flows; repeated restores must not keep rewriting history. + +## Aborted and errored assistant tool calls + +An aborted or errored assistant row may still contain a tool-call-looking block. Bobbit does not treat those ids as valid producers because the provider turn did not complete normally. Any late result for those ids is dropped on restore. + +This handles the common stuck-session shape: + +1. Assistant starts a tool call. +2. The turn is aborted or errors. +3. A late tool result is appended after the assistant row. +4. Restore would otherwise rehydrate a result whose producing call is not valid retained history. + +The assistant row itself is preserved for UI/user history; only the invalid result side is removed. + +## OpenAI Responses / Codex request guard + +OpenAI Responses represents tool calls as `function_call` items and tool results as `function_call_output` items in `payload.input`. The generated guard extension registers pi's `before_provider_request` hook and performs a payload-local scan: + +- Track `function_call.call_id` values already seen in `payload.input`. +- Keep a `function_call_output` only if it has a string `call_id` already seen in that same input array. +- Drop outputs with missing/non-string ids or outputs that appear before their call. +- Preserve all other items, valid pairs, and ordering. +- No-op for non-object payloads, payloads without array `input`, and non-Responses-shaped requests. + +The extension is added during normal session setup and mirrored in restore / role-reassignment / force-abort respawn paths, so OpenAI and Codex sessions keep the same preflight guard across lifecycle transitions. + +## Bounded diagnostics + +Repairs are not silent, but logs are bounded and do not include raw tool output. + +- Transcript sanitizer success logs include counts such as dropped orphan tool-result rows and filtered orphan result blocks. +- Transcript sanitizer read/write/path failures log warnings and remain non-fatal, so a defensive repair failure does not prevent session restore. +- The OpenAI guard logs only the number of dropped `function_call_output` items with the `[bobbit-openai-orphan-guard]` prefix. + +This gives operators enough signal to spot transcript repair without leaking large or sensitive tool-result payloads into logs. + +## Dedicated read-only sandbox mount for the OpenAI guard + +The OpenAI guard is generated source loaded by sandboxed agents via `--extension`. It is content-addressed under: + +```text +.bobbit/state/openai-orphan-tool-result//extension.ts +``` + +and exposed inside sandbox containers as: + +```text +/bobbit-state/openai-orphan-tool-result//extension.ts +``` + +It intentionally does **not** live under the existing `tool-guard` state directory. `tool-guard` must stay writable for other session-scoped guard material, while this provider guard is always loaded and its generated source can be reused by later sessions. If a compromised sandbox could write to that reused extension source, it could tamper with future provider requests. + +For that reason `docker-args.ts` mounts generated-extension state subdirectories, including `openai-orphan-tool-result`, read-only (`:ro`). Bobbit still avoids mounting the full state directory, which contains secrets such as gateway tokens and TLS material. The writer also revalidates cached file contents before reuse as defense-in-depth. + +## Regression coverage + +The behavior is pinned by focused unit coverage: + +- `tests/transcript-sanitizer.test.ts` — valid pair preservation, idempotence, aborted/errored assistant tool-call handling, compaction-boundary orphan result dropping, and mixed user-message result-block filtering. +- `tests/openai-orphan-tool-result-guard.test.ts` — OpenAI payload guard behavior, bounded diagnostics, generated extension source, and dedicated state subdir placement. +- `tests/docker-args.test.ts` and `tests/container-path-translation.test.ts` — sandbox mount creation, read-only generated-extension mounts, and container/host path translation. diff --git a/src/server/agent/bobbit-archive.ts b/src/server/agent/bobbit-archive.ts index fee3beb41..13415279a 100644 --- a/src/server/agent/bobbit-archive.ts +++ b/src/server/agent/bobbit-archive.ts @@ -56,6 +56,7 @@ export const GATEWAY_OWNED_FILES: readonly string[] = [ "state/tool-guard/", // src/server/agent/tool-activation.ts "state/provider-bridge/", // src/server/agent/provider-bridge-extension.ts "state/google-code-assist/", // src/server/agent/google-code-assist-provider-extension.ts + "state/openai-orphan-tool-result/", // src/server/agent/openai-orphan-tool-result-extension.ts "state/mcp-extensions/", // src/server/agent/tool-activation.ts, rpc-bridge.ts "state/html-snapshots/", // server.ts "state/gate-diagnostics/", // src/server/agent/gate-diagnostics-cleanup.ts diff --git a/src/server/agent/docker-args.ts b/src/server/agent/docker-args.ts index 75ece6bce..c7a273ad7 100644 --- a/src/server/agent/docker-args.ts +++ b/src/server/agent/docker-args.ts @@ -205,30 +205,25 @@ export function buildDockerRunArgs(config: DockerRunConfig): string[] { // Bind mount ONLY specific state subdirectories — never the full state dir, // which contains the host gateway token, TLS keys, sessions.json, etc. // - // `google-code-assist` holds the generated pi-coding-agent provider - // extension (`/provider.ts`, written by - // google-code-assist-provider-extension.ts) that registers the - // `google-code-assist` API provider inside the spawned agent. Sandboxed - // sessions load it via `--extension`, whose host path remapArgsForContainer - // rewrites to `/bobbit-state/google-code-assist/...`; that container path - // only resolves if the subdir is bind-mounted here. The dir contains only - // generated extension source (no secrets — the runtime token is fetched - // per-request from the gateway), so mounting it is safe. + // `google-code-assist` and `openai-orphan-tool-result` hold generated + // pi-coding-agent extensions loaded by sandboxed sessions via `--extension`. + // Host paths are remapped to `/bobbit-state//...` by + // remapArgsForContainer, so those container paths only resolve if the subdirs + // are bind-mounted here. They contain generated extension source only — no + // secrets are baked into either file. // - // `google-code-assist` is mounted READ-ONLY (`:ro`): the sandboxed agent - // only ever *loads* the generated `provider.ts` via `--extension`, never - // writes it. A writable mount would let a compromised sandbox tamper with - // the generated extension source, which is then reused (content-addressed, - // shared across sessions) for a subsequent run — a sandbox-escape vector. - // The `:ro` flag closes that hole at the kernel mount level; the gateway - // also revalidates cached contents before reuse as defense-in-depth - // (see google-code-assist-provider-extension.ts). + // Both generated-extension subdirs are mounted READ-ONLY (`:ro`): sandboxed + // agents only ever *load* them. A writable mount would let a compromised + // sandbox tamper with content-addressed source reused by later sessions. + // The gateway also revalidates cached contents before reuse as + // defense-in-depth (see the corresponding extension writer modules). if (stateDir) { const sandboxStateDirs: Array<{ sub: string; readOnly?: boolean }> = [ { sub: "sessions" }, { sub: "tool-guard" }, { sub: "html-snapshots" }, { sub: "google-code-assist", readOnly: true }, + { sub: "openai-orphan-tool-result", readOnly: true }, ]; for (const { sub, readOnly } of sandboxStateDirs) { const hostPath = path.join(stateDir, sub); diff --git a/src/server/agent/openai-orphan-tool-result-extension.ts b/src/server/agent/openai-orphan-tool-result-extension.ts new file mode 100644 index 000000000..a9994fd5c --- /dev/null +++ b/src/server/agent/openai-orphan-tool-result-extension.ts @@ -0,0 +1,181 @@ +import { createHash } from "node:crypto"; +import fs from "node:fs"; +import path from "node:path"; +import { bobbitStateDir } from "../bobbit-dir.js"; + +export const OPENAI_ORPHAN_TOOL_RESULT_STATE_SUBDIR = "openai-orphan-tool-result"; + +export interface OpenAiOrphanToolResultGuardResult { + payload: unknown; + dropped: number; + changed: boolean; +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null; +} + +function isFunctionCallItem(item: unknown): item is { type: "function_call"; call_id: string } { + return isRecord(item) && item.type === "function_call" && typeof item.call_id === "string"; +} + +function isFunctionCallOutputItem(item: unknown): item is { type: "function_call_output"; call_id?: unknown } { + return isRecord(item) && item.type === "function_call_output"; +} + +/** + * Drop OpenAI Responses `function_call_output` items that do not have a prior + * matching `function_call` in the same payload input array. + */ +export function dropOrphanFunctionCallOutputsFromPayload(payload: unknown): OpenAiOrphanToolResultGuardResult { + if (!isRecord(payload) || !Array.isArray(payload.input)) { + return { payload, dropped: 0, changed: false }; + } + + if (!payload.input.some(isFunctionCallOutputItem)) { + return { payload, dropped: 0, changed: false }; + } + + const seenCallIds = new Set(); + const filteredInput: unknown[] = []; + let dropped = 0; + + for (const item of payload.input) { + if (isFunctionCallItem(item)) { + seenCallIds.add(item.call_id); + filteredInput.push(item); + continue; + } + + if (isFunctionCallOutputItem(item)) { + const callId = item.call_id; + if (typeof callId !== "string" || !seenCallIds.has(callId)) { + dropped++; + continue; + } + } + + filteredInput.push(item); + } + + if (dropped === 0) { + return { payload, dropped: 0, changed: false }; + } + + return { + payload: { ...payload, input: filteredInput }, + dropped, + changed: true, + }; +} + +export function generateOpenAiOrphanToolResultExtension(): string { + return `function isRecord(value) { + return typeof value === "object" && value !== null; +} + +function isFunctionCallItem(item) { + return isRecord(item) && item.type === "function_call" && typeof item.call_id === "string"; +} + +function isFunctionCallOutputItem(item) { + return isRecord(item) && item.type === "function_call_output"; +} + +function dropOrphanFunctionCallOutputsFromPayload(payload) { + if (!isRecord(payload) || !Array.isArray(payload.input)) { + return { payload, dropped: 0, changed: false }; + } + + if (!payload.input.some(isFunctionCallOutputItem)) { + return { payload, dropped: 0, changed: false }; + } + + const seenCallIds = new Set(); + const filteredInput = []; + let dropped = 0; + + for (const item of payload.input) { + if (isFunctionCallItem(item)) { + seenCallIds.add(item.call_id); + filteredInput.push(item); + continue; + } + + if (isFunctionCallOutputItem(item)) { + const callId = item.call_id; + if (typeof callId !== "string" || !seenCallIds.has(callId)) { + dropped++; + continue; + } + } + + filteredInput.push(item); + } + + if (dropped === 0) { + return { payload, dropped: 0, changed: false }; + } + + return { + payload: { ...payload, input: filteredInput }, + dropped, + changed: true, + }; +} + +export default function(pi) { + pi.on("before_provider_request", (event) => { + const result = dropOrphanFunctionCallOutputsFromPayload(event && event.payload); + if (!result.changed) return undefined; + console.warn("[bobbit-openai-orphan-guard] Dropped " + result.dropped + " orphan function_call_output item(s)"); + return result.payload; + }); +} +`; +} + +let cachedCode: string | undefined; +let cachedPath: string | undefined; + +/** Write the OpenAI Responses orphan-output guard extension and return its path. */ +export function writeOpenAiOrphanToolResultExtension(): string | undefined { + if (!cachedCode) cachedCode = generateOpenAiOrphanToolResultExtension(); + if (cachedPath) { + try { + if (fs.readFileSync(cachedPath, "utf-8") === cachedCode) return cachedPath; + } catch { /* missing/unreadable — fall through to rewrite */ } + cachedPath = undefined; + } + + try { + // Content-addressed under its own sandbox-visible state subdir, mounted + // READ-ONLY by docker-args.ts. Do not place this always-loaded provider + // guard under writable tool-guard/, or a compromised sandbox could tamper + // with extension source reused by later sessions. + const baseDir = path.join(bobbitStateDir(), OPENAI_ORPHAN_TOOL_RESULT_STATE_SUBDIR); + const hash = createHash("sha256").update(cachedCode).digest("hex").slice(0, 12); + const extDir = path.join(baseDir, hash); + fs.mkdirSync(extDir, { recursive: true }); + + const filePath = path.join(extDir, "extension.ts"); + try { + if (fs.readFileSync(filePath, "utf-8") === cachedCode) { + cachedPath = filePath; + return filePath; + } + } catch { /* file does not exist yet */ } + fs.writeFileSync(filePath, cachedCode, "utf-8"); + cachedPath = filePath; + return filePath; + } catch { + // Non-fatal: session setup should not fail because a defensive provider + // preflight guard could not be written. + return undefined; + } +} + +export function resetOpenAiOrphanToolResultExtensionCache(): void { + cachedCode = undefined; + cachedPath = undefined; +} diff --git a/src/server/agent/project-sandbox.ts b/src/server/agent/project-sandbox.ts index e3056aeda..b9ef9ed7e 100644 --- a/src/server/agent/project-sandbox.ts +++ b/src/server/agent/project-sandbox.ts @@ -740,7 +740,7 @@ export class ProjectSandbox { // Ensure the state directory and sandbox-visible subdirectories exist for bind mounts const stateDir = path.join(this.options.projectDir, ".bobbit", "state"); fs.mkdirSync(stateDir, { recursive: true }); - for (const sub of ["sessions", "tool-guard", "html-snapshots"]) { + for (const sub of ["sessions", "tool-guard", "html-snapshots", "google-code-assist", "openai-orphan-tool-result"]) { fs.mkdirSync(path.join(stateDir, sub), { recursive: true }); } diff --git a/src/server/agent/rpc-bridge.ts b/src/server/agent/rpc-bridge.ts index fbe8c1ba7..c788de942 100644 --- a/src/server/agent/rpc-bridge.ts +++ b/src/server/agent/rpc-bridge.ts @@ -810,9 +810,10 @@ function buildMountTable(builtinToolsDir?: string): MountMapping[] { { containerPrefix: "/bobbit-state/sessions", hostPath: path.join(stateDir, "sessions") }, { containerPrefix: "/bobbit-state/tool-guard", hostPath: path.join(stateDir, "tool-guard") }, { containerPrefix: "/bobbit-state/html-snapshots", hostPath: path.join(stateDir, "html-snapshots") }, - // Generated google-code-assist provider extension (provider.ts) — bind-mounted - // by docker-args.ts so sandboxed pi-coding-agent can load it via --extension. + // Generated pi-coding-agent extensions — bind-mounted read-only by + // docker-args.ts so sandboxed sessions can load them via --extension. { containerPrefix: "/bobbit-state/google-code-assist", hostPath: path.join(stateDir, "google-code-assist") }, + { containerPrefix: "/bobbit-state/openai-orphan-tool-result", hostPath: path.join(stateDir, "openai-orphan-tool-result") }, { containerPrefix: "/tools", hostPath: TOOLS_DIR }, ]; diff --git a/src/server/agent/session-manager.ts b/src/server/agent/session-manager.ts index c9c920f07..073795021 100755 --- a/src/server/agent/session-manager.ts +++ b/src/server/agent/session-manager.ts @@ -49,6 +49,7 @@ import type { ToolManager } from "./tool-manager.js"; import { computeToolActivationArgs, writeMcpProxyExtensions, writeToolGuardExtension, computeEffectiveAllowedTools, tagAllowedTool, type EffectiveTool } from "./tool-activation.js"; import { hasProviderBridgeHooks, writeProviderBridgeExtension } from "./provider-bridge-extension.js"; import { writeGoogleCodeAssistProviderExtension } from "./google-code-assist-provider-extension.js"; +import { writeOpenAiOrphanToolResultExtension } from "./openai-orphan-tool-result-extension.js"; import { discoverSlashSkills, type SkillMarketContext } from "../skills/slash-skills.js"; import { getProjectRoot } from "../bobbit-dir.js"; import { shouldSkipRemotePush, shouldSkipRemoteGitForTests, detectPrimaryBranch, isGitRepo, getRepoRoot, isUnresolvedHeadWorktreeError } from "../skills/git.js"; @@ -1952,6 +1953,14 @@ export class SessionManager { } } + // OpenAI Responses preflight guard. Mirrors session setup so restore, + // role-reassignment, and force-abort respawn paths keep dropping orphan + // function_call_output items before provider requests are sent. + const openAiOrphanGuardPath = writeOpenAiOrphanToolResultExtension(); + if (openAiOrphanGuardPath) { + args.push("--extension", openAiOrphanGuardPath); + } + // Google account (Code Assist) provider extension. Mirrors // session-setup.ts::resolveToolActivation so respawn/restore paths keep the // provider registered and `google-gemini-cli/*` models stay runnable after a diff --git a/src/server/agent/session-setup.ts b/src/server/agent/session-setup.ts index e471ae591..59b53e023 100644 --- a/src/server/agent/session-setup.ts +++ b/src/server/agent/session-setup.ts @@ -42,6 +42,7 @@ import { buildReattemptContext } from "./goal-assistant.js"; import { computeToolActivationArgs, writeMcpProxyExtensions, writeToolGuardExtension, computeEffectiveAllowedTools, type EffectiveTool } from "./tool-activation.js"; import { hasProviderBridgeHooks, writeProviderBridgeExtension } from "./provider-bridge-extension.js"; import { writeGoogleCodeAssistProviderExtension } from "./google-code-assist-provider-extension.js"; +import { writeOpenAiOrphanToolResultExtension } from "./openai-orphan-tool-result-extension.js"; import { createWorktree, cleanupWorktree, isUnresolvedHeadWorktreeError } from "../skills/git.js"; import { isWorktreePathReferencedByLiveSession, type WorktreeReferenceRecord } from "./worktree-reference-guard.js"; @@ -852,6 +853,14 @@ function _resolveToolActivation(plan: SessionSetupPlan, ctx: PipelineContext): v } } + // OpenAI Responses preflight guard: remove orphan function_call_output items + // that would otherwise wedge Codex/OpenAI sessions. Provider-safe no-op for + // non-Responses payloads. + const openAiOrphanGuardPath = writeOpenAiOrphanToolResultExtension(); + if (openAiOrphanGuardPath) { + plan.bridgeOptions.args.push("--extension", openAiOrphanGuardPath); + } + // Register the Google account (Code Assist) provider INSIDE the agent process // so `google-gemini-cli/*` models can run as session models. Written // UNCONDITIONALLY (not credential-gated): a session spawned BEFORE Google diff --git a/src/server/agent/transcript-sanitizer.ts b/src/server/agent/transcript-sanitizer.ts index 38cda4fdb..524993ddf 100644 --- a/src/server/agent/transcript-sanitizer.ts +++ b/src/server/agent/transcript-sanitizer.ts @@ -1,6 +1,6 @@ /** - * Transcript sanitizer — un-poison persisted agent `.jsonl` transcripts whose - * `user` messages carry a blank text body. + * Transcript sanitizer — repair persisted agent `.jsonl` transcripts at the + * restore boundary. * * Background: the model API rejects a user message whose ContentBlock has a * blank `text` field (next to an image block, or as a standalone empty text @@ -12,7 +12,7 @@ * transcript is to repair the `.jsonl` Bobbit owns at the rehydration boundary * (before `switch_session`). * - * This module is a pure, idempotent, one-pass sanitizer: it rewrites a + * This module is a pure, idempotent sanitizer: it rewrites a * persisted `user` message whose effective text is blank/whitespace-only to the * synthetic `ATTACHMENT_ONLY_TEXT` ("Attachments:"), covering BOTH the * image-adjacent case (`[{text:""},{image}]`) AND the standalone empty/blank @@ -20,11 +20,10 @@ * a user message with only non-text content / no text block at all). * * IMPORTANT: tool results are also represented as `role:"user"` messages whose - * content is a `tool_result`/`toolResult` block with no text. Those are normal, - * valid history — rewriting them to "Attachments:" would corrupt tool-call - * history and break tool-result ordering. So a user message that carries ANY - * tool_result/toolResult block is left byte-identical. It leaves every other - * line byte-identical too, so re-running it is a no-op. + * content is a `tool_result`/`toolResult` block with no text. Valid tool-result + * history is left byte-identical. Orphan results (no prior retained, + * non-aborted/non-errored assistant tool call) are repaired at this restore + * boundary so providers are not rehydrated with invalid tool-call history. */ import { ATTACHMENT_ONLY_TEXT } from "./rpc-bridge.js"; @@ -53,17 +52,48 @@ function effectiveText(content: unknown): string { return parts.join(""); } +/** Detect whether a content block is a tool-result block. */ +function isToolResultBlock(block: unknown): boolean { + return !!block && typeof block === "object" && + ((block as any).type === "tool_result" || (block as any).type === "toolResult"); +} + /** * Detect whether a message `content` carries a tool-result block. Tool results * are persisted as `role:"user"` messages with a `tool_result` (or `toolResult`) - * content block and no text — they are valid history and MUST NOT be rewritten. + * content block and no text — they are valid history only when matched to a + * prior retained assistant tool call. */ function hasToolResultBlock(content: unknown): boolean { if (!Array.isArray(content)) return false; - return content.some( - (b) => b && typeof b === "object" && - ((b as any).type === "tool_result" || (b as any).type === "toolResult"), - ); + return content.some(isToolResultBlock); +} + +function stringField(value: unknown): string | null { + return typeof value === "string" && value.length > 0 ? value : null; +} + +function toolCallIdFromAssistantBlock(block: unknown): string | null { + if (!block || typeof block !== "object") return null; + const b = block as any; + // Tolerant id resolution matching src/server/extension-host/action-guard.ts: + // typed `toolCall`/`tool_use` blocks carry `id`, but some variants persist + // `toolCallId` or `tool_use_id` instead. Accept any of the three so a valid + // assistant tool call is never misclassified as orphaned. + if (b.type === "toolCall" || b.type === "tool_use" || typeof b.toolCallId === "string") { + return stringField(b.id ?? b.toolCallId ?? b.tool_use_id); + } + return null; +} + +function toolResultIdFromBlock(block: unknown): string | null { + if (!block || typeof block !== "object") return null; + const b = block as any; + return stringField(b.tool_use_id) ?? stringField(b.toolCallId) ?? stringField(b.id); +} + +function messageStopReason(entry: any): unknown { + return entry?.message?.stopReason ?? entry?.stopReason; } /** @@ -96,10 +126,14 @@ function rewriteBlankUserContent(content: unknown): unknown { export interface SanitizeResult { /** The (possibly unchanged) JSONL content. */ content: string; - /** Whether any line was rewritten. */ + /** Whether any line was rewritten or dropped. */ changed: boolean; - /** Number of transcript records rewritten. */ + /** Number of blank user-message transcript records rewritten. */ rewritten: number; + /** Number of message-level orphan tool-result rows dropped. */ + droppedToolResultRows: number; + /** Number of orphan tool-result content blocks filtered from user messages. */ + filteredToolResultBlocks: number; } export interface RebaseTranscriptCwdMetadataOptions { @@ -116,20 +150,143 @@ export interface RebaseTranscriptCwdMetadataOptions { * yields `changed:false`. */ export function sanitizeTranscriptContent(content: string): SanitizeResult { - return transformTranscriptJsonl(content, (entry) => { - if (!entry || entry.type !== "message" || !entry.message) return false; - if (entry.message.role !== "user") return false; + if (!content) return emptySanitizeResult(content); - // Tool-result user messages are valid history (no text by design) — - // never touch them, or tool-call history/ordering is corrupted. - if (hasToolResultBlock(entry.message.content)) return false; + // Preserve the exact line structure when no rows are dropped: split on \n, + // keep empty segments, and rejoin with \n so unchanged lines stay byte-for-byte + // identical. Dropped orphan result rows are omitted from the output. + const lines = content.split("\n"); + const outputLines: string[] = []; + const lineIndexByEntryId = new Map(); + for (let lineIndex = 0; lineIndex < lines.length; lineIndex++) { + const trimmed = lines[lineIndex].trim(); + if (!trimmed) continue; + try { + const parsed = JSON.parse(trimmed); + const id = stringField(parsed?.id); + if (id && !lineIndexByEntryId.has(id)) lineIndexByEntryId.set(id, lineIndex); + } catch { + // non-JSON line — no entry id to index + } + } + const seenToolCallIds = new Map(); + let changed = false; + let rewritten = 0; + let droppedToolResultRows = 0; + let filteredToolResultBlocks = 0; - const text = effectiveText(entry.message.content); - if (text.trim() !== "") return false; // already valid — leave byte-identical + for (let lineIndex = 0; lineIndex < lines.length; lineIndex++) { + const raw = lines[lineIndex]; + const trimmed = raw.trim(); + if (!trimmed) { + outputLines.push(raw); + continue; + } - entry.message.content = rewriteBlankUserContent(entry.message.content); - return true; - }); + let entry: any; + try { + entry = JSON.parse(trimmed); + } catch { + outputLines.push(raw); // non-JSON line — leave untouched + continue; + } + + if (!entry || entry.type !== "message" || !entry.message) { + if (entry?.type === "compaction") { + const firstKeptEntryId = stringField(entry.firstKeptEntryId); + const firstKeptLineIndex = firstKeptEntryId ? lineIndexByEntryId.get(firstKeptEntryId) : undefined; + if (firstKeptLineIndex === undefined) { + // Legacy fallback: without a resolvable exact retained-range boundary, + // the marker itself is the only safe split point. + seenToolCallIds.clear(); + } else { + // Pi's `firstKeptEntryId` can name any retained entry (user, + // assistant-without-tools, or assistant-with-tools). Drop only tool-call + // ids whose originating assistant line is before that retained range. + for (const [id, originLineIndex] of seenToolCallIds) { + if (originLineIndex < firstKeptLineIndex) seenToolCallIds.delete(id); + } + } + } + outputLines.push(raw); + continue; + } + + const message = entry.message; + if (message.role === "assistant") { + const stopReason = messageStopReason(entry); + if (stopReason !== "aborted" && stopReason !== "error" && Array.isArray(message.content)) { + for (const block of message.content) { + const id = toolCallIdFromAssistantBlock(block); + if (id) seenToolCallIds.set(id, lineIndex); + } + } + outputLines.push(raw); + continue; + } + + if (message.role === "toolResult") { + const id = stringField(message.toolCallId); + if (!id || !seenToolCallIds.has(id)) { + changed = true; + droppedToolResultRows++; + continue; + } + outputLines.push(raw); + continue; + } + + if (message.role !== "user") { + outputLines.push(raw); + continue; + } + + if (hasToolResultBlock(message.content)) { + const filteredContent = (message.content as unknown[]).filter((block) => { + if (!isToolResultBlock(block)) return true; + const id = toolResultIdFromBlock(block); + const keep = !!id && seenToolCallIds.has(id); + if (!keep) filteredToolResultBlocks++; + return keep; + }); + + if (filteredContent.length !== (message.content as unknown[]).length) { + changed = true; + if (filteredContent.length === 0) continue; + entry.message.content = filteredContent; + message.content = filteredContent; + if (hasToolResultBlock(filteredContent)) { + outputLines.push(JSON.stringify(entry)); + continue; + } + } else { + // Valid tool-result user messages are valid history (no text by design) — + // never rewrite them, or tool-call history/ordering is corrupted. + outputLines.push(raw); + continue; + } + } + + const text = effectiveText(message.content); + if (text.trim() !== "") { + outputLines.push(raw); // already valid — leave byte-identical + continue; + } + + entry.message.content = rewriteBlankUserContent(message.content); + outputLines.push(JSON.stringify(entry)); + changed = true; + rewritten++; + } + + if (!changed) return emptySanitizeResult(content); + return { + content: outputLines.join("\n"), + changed: true, + rewritten, + droppedToolResultRows, + filteredToolResultBlocks, + }; } /** @@ -144,7 +301,7 @@ export function rebaseTranscriptCwdMetadataContent( ): SanitizeResult { const oldCwds = new Set(options.oldCwds.filter((cwd): cwd is string => typeof cwd === "string" && cwd.length > 0)); if (!content || !options.newCwd || oldCwds.size === 0) { - return { content, changed: false, rewritten: 0 }; + return emptySanitizeResult(content); } return transformTranscriptJsonl(content, (entry) => { @@ -163,11 +320,15 @@ function isRebasableRuntimeCwdMetadataRecord(entry: any): entry is { type: "sess return entry.subtype === "init" || !hasSubtype; } +function emptySanitizeResult(content: string): SanitizeResult { + return { content, changed: false, rewritten: 0, droppedToolResultRows: 0, filteredToolResultBlocks: 0 }; +} + function transformTranscriptJsonl( content: string, mutateEntry: (entry: any) => boolean, ): SanitizeResult { - if (!content) return { content, changed: false, rewritten: 0 }; + if (!content) return emptySanitizeResult(content); // Preserve the exact line structure: split on \n, keep empty segments, and // rejoin with \n so the only bytes that change are the rewritten JSON lines. @@ -193,8 +354,8 @@ function transformTranscriptJsonl( rewritten++; } - if (!changed) return { content, changed: false, rewritten: 0 }; - return { content: lines.join("\n"), changed: true, rewritten }; + if (!changed) return emptySanitizeResult(content); + return { content: lines.join("\n"), changed: true, rewritten, droppedToolResultRows: 0, filteredToolResultBlocks: 0 }; } /** True iff `target` is `root` itself or strictly nested inside it. */ @@ -303,7 +464,7 @@ function writeFileNoFollow(realPath: string, data: string): void { * container-path is translated to its host path and written there (visible * inside the container via the mount). * - * @returns the number of user messages rewritten (0 when nothing changed). + * @returns the number of transcript repairs performed (0 when nothing changed). */ export async function sanitizeAgentTranscriptFile( ctx: SessionFsContext, @@ -316,7 +477,14 @@ export async function sanitizeAgentTranscriptFile( sandboxManager, sanitizeTranscriptContent, "sanitize", - (file, rewritten) => `Un-poisoned ${rewritten} blank-text user message(s) in ${file}`, + (file, result) => { + const parts: string[] = []; + if (result.rewritten) parts.push(`un-poisoned ${result.rewritten} blank-text user message(s)`); + if (result.droppedToolResultRows) parts.push(`dropped ${result.droppedToolResultRows} orphan tool result row(s)`); + if (result.filteredToolResultBlocks) parts.push(`filtered ${result.filteredToolResultBlocks} orphan tool result block(s)`); + return `${parts.join(", ")} in ${file}`; + }, + (result) => result.rewritten + result.droppedToolResultRows + result.filteredToolResultBlocks, ); } @@ -339,7 +507,7 @@ export async function rebaseAgentTranscriptCwdMetadataFile( sandboxManager, (content) => rebaseTranscriptCwdMetadataContent(content, options), "cwd metadata rebase", - (file, rewritten) => `Rebased ${rewritten} runtime cwd metadata record(s) in ${file}`, + (file, result) => `Rebased ${result.rewritten} runtime cwd metadata record(s) in ${file}`, ); } @@ -349,7 +517,8 @@ async function transformAgentTranscriptFile( sandboxManager: SandboxManager | null, transform: (content: string) => SanitizeResult, operation: string, - logMessage: (filePath: string, rewritten: number) => string, + logMessage: (filePath: string, result: SanitizeResult) => string, + returnCount: (result: SanitizeResult) => number = (result) => result.rewritten, ): Promise { try { // Resolve the host-side path. Non-sandboxed: filePath is already a host @@ -384,8 +553,8 @@ async function transformAgentTranscriptFile( } writeFileNoFollow(realPath, result.content); - console.log(`[transcript-sanitizer] ${logMessage(filePath, result.rewritten)}`); - return result.rewritten; + console.log(`[transcript-sanitizer] ${logMessage(filePath, result)}`); + return returnCount(result); } catch (err) { console.warn(`[transcript-sanitizer] Failed to ${operation} ${filePath} (non-fatal):`, err); return 0; diff --git a/src/server/auth/session-secret.ts b/src/server/auth/session-secret.ts index 0426202bf..10245516a 100644 --- a/src/server/auth/session-secret.ts +++ b/src/server/auth/session-secret.ts @@ -28,9 +28,10 @@ import crypto from "node:crypto"; * `SandboxTokenStore` exactly (see `sandbox-token.ts`): nothing is persisted, so * there is zero disk surface an agent sandbox could mount and read. Critically, * the gateway never bind-mounts the `.bobbit/state` root into agent containers — - * only specific subdirectories (`sessions/`, `tool-guard/`, `html-snapshots/` — - * see `docker-args.ts`), and `sessions.json` lives at the state ROOT and is also - * never mounted. Storing the secret on disk (even in `sessions.json`) was + * only specific subdirectories (`sessions/`, `tool-guard/`, `html-snapshots/`, + * generated read-only extension dirs — see `docker-args.ts`), and `sessions.json` + * lives at the state ROOT and is also never mounted. Storing the secret on disk + * (even in `sessions.json`) was * therefore unnecessary; keeping it purely in-memory is strictly safer. * * RESTART-SAFE diff --git a/tests/container-path-translation.test.ts b/tests/container-path-translation.test.ts index 05be27ce8..4f1b5bf3a 100644 --- a/tests/container-path-translation.test.ts +++ b/tests/container-path-translation.test.ts @@ -34,7 +34,7 @@ describe("containerPathToHost (bind-mount fallback)", () => { }); it("translates /bobbit-state subdirectory paths", () => { - // Only specific subdirs are mounted (sessions/, tool-guard/, html-snapshots/) + // Only specific state subdirs are mounted; never the state root. const containerPath = "/bobbit-state/sessions/abc-123/2026-01-01.jsonl"; const hostPath = containerPathToHost(containerPath); const expected = process.platform === "win32" @@ -43,16 +43,23 @@ describe("containerPathToHost (bind-mount fallback)", () => { assert.equal(hostPath, expected); }); - it("translates the google-code-assist provider extension path", () => { - // The generated provider.ts lives under /google-code-assist// - // and is bind-mounted at /bobbit-state/google-code-assist so sandboxed - // pi-coding-agent can load it via --extension. - const containerPath = "/bobbit-state/google-code-assist/abc123def456/provider.ts"; - const hostPath = containerPathToHost(containerPath); - const expected = process.platform === "win32" - ? "C:\\Users\\test\\project\\.bobbit\\state\\google-code-assist\\abc123def456\\provider.ts" - : "/home/test/project/.bobbit/state/google-code-assist/abc123def456/provider.ts"; - assert.equal(hostPath, expected); + it("translates generated extension paths", () => { + for (const { containerPath, expectedHost } of [ + { + containerPath: "/bobbit-state/google-code-assist/abc123def456/provider.ts", + expectedHost: process.platform === "win32" + ? "C:\\Users\\test\\project\\.bobbit\\state\\google-code-assist\\abc123def456\\provider.ts" + : "/home/test/project/.bobbit/state/google-code-assist/abc123def456/provider.ts", + }, + { + containerPath: "/bobbit-state/openai-orphan-tool-result/def456abc123/extension.ts", + expectedHost: process.platform === "win32" + ? "C:\\Users\\test\\project\\.bobbit\\state\\openai-orphan-tool-result\\def456abc123\\extension.ts" + : "/home/test/project/.bobbit/state/openai-orphan-tool-result/def456abc123/extension.ts", + }, + ]) { + assert.equal(containerPathToHost(containerPath), expectedHost); + } }); it("does NOT translate /bobbit-state root files (not mounted)", () => { diff --git a/tests/docker-args.test.ts b/tests/docker-args.test.ts index 8bd4d6230..f5479d48c 100644 --- a/tests/docker-args.test.ts +++ b/tests/docker-args.test.ts @@ -63,23 +63,24 @@ describe("buildDockerRunArgs", () => { ); }); - it("mounts the google-code-assist state subdir so sandboxed agents can load the provider extension", () => { - const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "bobbit-docker-gca-")); + it("mounts generated-extension state subdirs so sandboxed agents can load them", () => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "bobbit-docker-generated-ext-")); try { const args = buildDockerRunArgs({ image: "test", workspaceDir: "/tmp/test", stateDir, }); const mounts = args.filter((a, i) => args[i - 1] === "-v"); - assert.ok( - mounts.some((m) => m.includes(":/bobbit-state/google-code-assist")), - `expected a /bobbit-state/google-code-assist mount, got: ${JSON.stringify(mounts)}`, - ); - // The mount must be a subdir (never the full state dir) and created on disk. - assert.ok( - fs.existsSync(path.join(stateDir, "google-code-assist")), - "google-code-assist subdir should be created before mounting", - ); + for (const sub of ["google-code-assist", "openai-orphan-tool-result"]) { + assert.ok( + mounts.some((m) => m.includes(`:/bobbit-state/${sub}`)), + `expected a /bobbit-state/${sub} mount, got: ${JSON.stringify(mounts)}`, + ); + assert.ok( + fs.existsSync(path.join(stateDir, sub)), + `${sub} subdir should be created before mounting`, + ); + } assert.ok( !mounts.some((m) => m.endsWith(":/bobbit-state")), "must never mount the full state dir", @@ -89,20 +90,22 @@ describe("buildDockerRunArgs", () => { } }); - it("mounts the google-code-assist state subdir READ-ONLY so a compromised sandbox cannot tamper with the generated provider extension", () => { - const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "bobbit-docker-gca-ro-")); + it("mounts generated-extension state subdirs READ-ONLY so a compromised sandbox cannot tamper with reused source", () => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "bobbit-docker-generated-ext-ro-")); try { const args = buildDockerRunArgs({ image: "test", workspaceDir: "/tmp/test", stateDir, }); const mounts = args.filter((a, i) => args[i - 1] === "-v"); - const gca = mounts.find((m) => m.includes(":/bobbit-state/google-code-assist")); - assert.ok(gca, `expected a google-code-assist mount, got: ${JSON.stringify(mounts)}`); - assert.ok( - gca!.endsWith(":/bobbit-state/google-code-assist:ro"), - `google-code-assist mount must be read-only (:ro), got: ${gca}`, - ); + for (const sub of ["google-code-assist", "openai-orphan-tool-result"]) { + const mount = mounts.find((m) => m.includes(`:/bobbit-state/${sub}`)); + assert.ok(mount, `expected a ${sub} mount, got: ${JSON.stringify(mounts)}`); + assert.ok( + mount!.endsWith(`:/bobbit-state/${sub}:ro`), + `${sub} mount must be read-only (:ro), got: ${mount}`, + ); + } // The writable state subdirs must NOT have picked up :ro. for (const sub of ["sessions", "tool-guard", "html-snapshots"]) { const m = mounts.find((x) => x.includes(`:/bobbit-state/${sub}`)); diff --git a/tests/e2e/orphan-tool-results.spec.ts b/tests/e2e/orphan-tool-results.spec.ts new file mode 100644 index 000000000..c50467496 --- /dev/null +++ b/tests/e2e/orphan-tool-results.spec.ts @@ -0,0 +1,247 @@ +/** + * API E2E — orphan tool-result lifecycle repair. + * + * Exercises the real restore/rehydration path that runs before `switch_session`: + * a persisted transcript containing an aborted assistant tool call plus a late + * matching toolResult is repaired on disk, restored without wedging the session, + * and remains clean on a second restore. Also exercises the generated OpenAI + * Responses guard extension without external credentials by invoking its local + * before_provider_request hook. + */ +import { test, expect } from "./in-process-harness.js"; +import { + agentEndPredicate, + connectWs, + createSession, + deleteSession, + statusPredicate, +} from "./e2e-setup.js"; +import { pollUntil } from "./test-utils/cleanup.js"; +import fs from "node:fs"; +import path from "node:path"; + +interface CapturedConsole { + logs: string[]; + warns: string[]; +} + +const ORPHAN_SECRET = "ORPHAN_RESULT_SECRET_SHOULD_NOT_SURVIVE"; +const VALID_SECRET = "VALID_RESULT_SHOULD_SURVIVE"; +const OPENAI_ORPHAN_GUARD_STATE_SUBDIR = "openai-orphan-tool-result"; + +test.describe.configure({ mode: "serial" }); +test.setTimeout(60_000); + +function jsonlLine(entry: unknown): string { + return JSON.stringify(entry); +} + +function craftedTranscript(): string { + return [ + jsonlLine({ type: "message", id: "user-1", message: { role: "user", content: [{ type: "text", text: "hello before restore" }] } }), + jsonlLine({ + type: "message", + id: "aborted-assistant", + message: { + role: "assistant", + stopReason: "aborted", + content: [{ type: "toolCall", id: "late-aborted-call", name: "team_spawn", arguments: { task: "discarded" } }], + }, + }), + jsonlLine({ + type: "message", + id: "orphan-result", + message: { + role: "toolResult", + toolCallId: "late-aborted-call", + toolName: "team_spawn", + isError: false, + content: [{ type: "text", text: ORPHAN_SECRET }], + }, + }), + jsonlLine({ + type: "message", + id: "valid-assistant", + message: { + role: "assistant", + content: [{ type: "toolCall", id: "valid-call", name: "bash", arguments: { command: "echo ok" } }], + }, + }), + jsonlLine({ + type: "message", + id: "valid-result", + message: { + role: "toolResult", + toolCallId: "valid-call", + toolName: "bash", + isError: false, + content: [{ type: "text", text: VALID_SECRET }], + }, + }), + ].join("\n") + "\n"; +} + +function readTranscriptMessages(filePath: string): any[] { + return fs.readFileSync(filePath, "utf-8") + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + .map((line) => JSON.parse(line)) + .filter((entry) => entry?.type === "message") + .map((entry) => entry.message); +} + +async function captureConsoleDuring(fn: () => Promise): Promise { + const originalLog = console.log; + const originalWarn = console.warn; + const logs: string[] = []; + const warns: string[] = []; + console.log = (...args: unknown[]) => { + logs.push(args.map(String).join(" ")); + originalLog(...args); + }; + console.warn = (...args: unknown[]) => { + warns.push(args.map(String).join(" ")); + originalWarn(...args); + }; + try { + await fn(); + } finally { + console.log = originalLog; + console.warn = originalWarn; + } + return { logs, warns }; +} + +async function simulateRestore(gateway: any, sessionId: string): Promise { + const sm = gateway.sessionManager; + const liveSession = sm.sessions.get(sessionId); + if (liveSession) { + try { liveSession.unsubscribe?.(); } catch { /* best-effort */ } + try { await liveSession.rpcClient?.stop?.(); } catch { /* already stopped */ } + sm.sessions.delete(sessionId); + } + return captureConsoleDuring(async () => { + await sm.restoreSessions(); + }); +} + +async function persistedAgentSessionFile(gateway: any, sessionId: string): Promise { + const sm = gateway.sessionManager; + return pollUntil(async () => { + const file = sm.getPersistedSession(sessionId)?.agentSessionFile; + return file && fs.existsSync(file) ? file : null; + }, { timeoutMs: 10_000, intervalMs: 100, label: "agent session file persisted" }); +} + +function removeOpenAiGuardDir(bobbitDir: string): void { + fs.rmSync(path.join(bobbitDir, "state", OPENAI_ORPHAN_GUARD_STATE_SUBDIR), { recursive: true, force: true }); +} + +function openAiGuardExtensionFiles(bobbitDir: string): string[] { + const root = path.join(bobbitDir, "state", OPENAI_ORPHAN_GUARD_STATE_SUBDIR); + if (!fs.existsSync(root)) return []; + const files: string[] = []; + const stack = [root]; + while (stack.length > 0) { + const dir = stack.pop()!; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) stack.push(full); + else if (entry.isFile() && entry.name === "extension.ts") files.push(full); + } + } + return files; +} + +test.describe("orphan tool-result restore lifecycle", () => { + test("restore repairs persisted orphan tool results, is idempotent, and leaves the session usable", async ({ gateway }) => { + const sessionId = await createSession(); + let conn: Awaited> | undefined; + try { + conn = await connectWs(sessionId); + conn.send({ type: "prompt", text: "prime orphan lifecycle file" }); + await conn.waitFor(agentEndPredicate(), 15_000); + await conn.waitFor(statusPredicate("idle"), 10_000).catch(() => {}); + conn.close(); + conn = undefined; + + const agentSessionFile = await persistedAgentSessionFile(gateway, sessionId); + expect(agentSessionFile, "test must use a real agent sessions transcript path").toContain(`${path.sep}agent${path.sep}sessions${path.sep}`); + + fs.writeFileSync(agentSessionFile, craftedTranscript(), "utf-8"); + expect(fs.readFileSync(agentSessionFile, "utf-8")).toContain(ORPHAN_SECRET); + + const { resetOpenAiOrphanToolResultExtensionCache } = await import("../../dist/server/agent/openai-orphan-tool-result-extension.js"); + resetOpenAiOrphanToolResultExtensionCache(); + removeOpenAiGuardDir(gateway.bobbitDir); + + const firstRestore = await simulateRestore(gateway, sessionId); + const repaired = fs.readFileSync(agentSessionFile, "utf-8"); + expect(repaired).not.toContain(ORPHAN_SECRET); + expect(repaired).toContain(VALID_SECRET); + expect(readTranscriptMessages(agentSessionFile).filter((m) => m.role === "toolResult").map((m) => m.toolCallId)).toEqual(["valid-call"]); + + const repairLog = [...firstRestore.logs, ...firstRestore.warns].join("\n"); + expect(repairLog).toContain("[transcript-sanitizer]"); + expect(repairLog).toContain("dropped 1 orphan tool result row(s)"); + expect(repairLog).not.toContain(ORPHAN_SECRET); + expect(openAiGuardExtensionFiles(gateway.bobbitDir).length, "restore setup should install the OpenAI orphan guard extension").toBeGreaterThan(0); + + const beforeSecondRestore = fs.readFileSync(agentSessionFile, "utf-8"); + const secondRestore = await simulateRestore(gateway, sessionId); + expect(fs.readFileSync(agentSessionFile, "utf-8"), "second restore must be idempotent").toBe(beforeSecondRestore); + expect([...secondRestore.logs, ...secondRestore.warns].join("\n")).not.toContain("orphan tool result"); + + conn = await connectWs(sessionId); + conn.send({ type: "prompt", text: "orphan lifecycle follow-up" }); + await conn.waitFor(agentEndPredicate(), 15_000); + await conn.waitFor(statusPredicate("idle"), 10_000).catch(() => {}); + } finally { + conn?.close(); + await deleteSession(sessionId).catch(() => {}); + } + }); + + test("generated OpenAI Responses guard hook drops only orphan function_call_output items", async ({ gateway }) => { + const { resetOpenAiOrphanToolResultExtensionCache, writeOpenAiOrphanToolResultExtension } = await import("../../dist/server/agent/openai-orphan-tool-result-extension.js"); + resetOpenAiOrphanToolResultExtensionCache(); + removeOpenAiGuardDir(gateway.bobbitDir); + + const extensionPath = writeOpenAiOrphanToolResultExtension(); + expect(extensionPath).toBeTruthy(); + expect(extensionPath).toContain(path.join(gateway.bobbitDir, "state", OPENAI_ORPHAN_GUARD_STATE_SUBDIR)); + expect(extensionPath).not.toContain(path.join(gateway.bobbitDir, "state", "tool-guard")); + + const source = fs.readFileSync(extensionPath!, "utf-8"); + const install = new Function(source.replace("export default function(pi)", "return function(pi)")) as () => (pi: { on: (event: string, cb: (event: any) => unknown) => void }) => void; + let beforeProviderRequest: ((event: any) => unknown) | undefined; + install()({ + on(event, cb) { + if (event === "before_provider_request") beforeProviderRequest = cb; + }, + }); + expect(beforeProviderRequest).toBeTruthy(); + + const payload = { + model: "gpt-test", + input: [ + { type: "function_call_output", call_id: "missing", output: "RAW_ORPHAN_OUTPUT" }, + { type: "message", role: "user", content: [{ type: "input_text", text: "hello" }] }, + { type: "function_call", call_id: "valid", name: "bash", arguments: "{}" }, + { type: "function_call_output", call_id: "valid", output: "VALID_OUTPUT" }, + ], + }; + + const captured = await captureConsoleDuring(async () => { + const next = beforeProviderRequest!({ payload }); + expect(next).toEqual({ + ...payload, + input: payload.input.slice(1), + }); + }); + const guardLog = [...captured.logs, ...captured.warns].join("\n"); + expect(guardLog).toContain("[bobbit-openai-orphan-guard] Dropped 1 orphan function_call_output item(s)"); + expect(guardLog).not.toContain("RAW_ORPHAN_OUTPUT"); + }); +}); diff --git a/tests/e2e/sandbox-pentest.spec.ts b/tests/e2e/sandbox-pentest.spec.ts index 826e40000..fb50f68af 100644 --- a/tests/e2e/sandbox-pentest.spec.ts +++ b/tests/e2e/sandbox-pentest.spec.ts @@ -87,6 +87,7 @@ test.describe("Sandbox Pen-test Regression", () => { expect(containerPaths).toContain("/bobbit-state/tool-guard"); expect(containerPaths).toContain("/bobbit-state/html-snapshots"); expect(containerPaths).toContain("/bobbit-state/google-code-assist"); + expect(containerPaths).toContain("/bobbit-state/openai-orphan-tool-result"); // Verify host paths point to the correct subdirectories // toDockerPath converts "C:\foo" to "/c/foo" on Windows, so normalize both @@ -145,7 +146,7 @@ test.describe("Sandbox Pen-test Regression", () => { const stateMounts = mounts.filter(m => m.container.startsWith("/bobbit-state")); // All /bobbit-state* mounts must point to specific subdirectories - const allowedSubdirs = ["sessions", "tool-guard", "html-snapshots", "google-code-assist"]; + const allowedSubdirs = ["sessions", "tool-guard", "html-snapshots", "google-code-assist", "openai-orphan-tool-result"]; for (const mount of stateMounts) { const subdir = mount.container.replace("/bobbit-state/", ""); expect( @@ -162,12 +163,13 @@ test.describe("Sandbox Pen-test Regression", () => { const hasRootMount = stateMounts.some(m => m.container === "/bobbit-state"); expect(hasRootMount, "No mount should expose /bobbit-state root (would leak token and other sensitive files)").toBe(false); - // The generated google-code-assist provider extension is loaded (never - // written) by the sandbox; mounting it writable would let a compromised - // sandbox tamper with the content-addressed source reused across sessions. - const gca = stateMounts.find(m => m.container === "/bobbit-state/google-code-assist"); - expect(gca, "google-code-assist must be mounted").toBeDefined(); - expect(gca!.flags, "google-code-assist mount must be read-only (:ro)").toBe("ro"); + // Generated extensions are loaded (never written) by the sandbox; mounting + // them writable would let a compromised sandbox tamper with reused source. + for (const sub of ["google-code-assist", "openai-orphan-tool-result"]) { + const mount = stateMounts.find(m => m.container === `/bobbit-state/${sub}`); + expect(mount, `${sub} must be mounted`).toBeDefined(); + expect(mount!.flags, `${sub} mount must be read-only (:ro)`).toBe("ro"); + } } finally { cleanup(); } @@ -224,6 +226,7 @@ test.describe("Sandbox Pen-test Regression", () => { expect(containerPaths).toContain("/bobbit-state/tool-guard"); expect(containerPaths).toContain("/bobbit-state/html-snapshots"); expect(containerPaths).toContain("/bobbit-state/google-code-assist"); + expect(containerPaths).toContain("/bobbit-state/openai-orphan-tool-result"); } finally { fs.rmSync(stateDir, { recursive: true, force: true }); fs.rmSync(workspaceDir, { recursive: true, force: true }); @@ -237,10 +240,9 @@ test.describe("Sandbox Pen-test Regression", () => { try { // Subdirs should not exist before the call - expect(fs.existsSync(path.join(stateDir, "sessions"))).toBe(false); - expect(fs.existsSync(path.join(stateDir, "tool-guard"))).toBe(false); - expect(fs.existsSync(path.join(stateDir, "html-snapshots"))).toBe(false); - expect(fs.existsSync(path.join(stateDir, "google-code-assist"))).toBe(false); + for (const sub of ["sessions", "tool-guard", "html-snapshots", "google-code-assist", "openai-orphan-tool-result"]) { + expect(fs.existsSync(path.join(stateDir, sub))).toBe(false); + } buildDockerRunArgs({ image: "bobbit-sandbox:latest", @@ -249,10 +251,9 @@ test.describe("Sandbox Pen-test Regression", () => { }); // After the call, subdirs should exist (created by mkdirSync) - expect(fs.existsSync(path.join(stateDir, "sessions"))).toBe(true); - expect(fs.existsSync(path.join(stateDir, "tool-guard"))).toBe(true); - expect(fs.existsSync(path.join(stateDir, "html-snapshots"))).toBe(true); - expect(fs.existsSync(path.join(stateDir, "google-code-assist"))).toBe(true); + for (const sub of ["sessions", "tool-guard", "html-snapshots", "google-code-assist", "openai-orphan-tool-result"]) { + expect(fs.existsSync(path.join(stateDir, sub))).toBe(true); + } } finally { fs.rmSync(stateDir, { recursive: true, force: true }); fs.rmSync(workspaceDir, { recursive: true, force: true }); diff --git a/tests/openai-orphan-tool-result-guard.test.ts b/tests/openai-orphan-tool-result-guard.test.ts new file mode 100644 index 000000000..b1e175a65 --- /dev/null +++ b/tests/openai-orphan-tool-result-guard.test.ts @@ -0,0 +1,156 @@ +import { describe, it, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { + dropOrphanFunctionCallOutputsFromPayload, + generateOpenAiOrphanToolResultExtension, + OPENAI_ORPHAN_TOOL_RESULT_STATE_SUBDIR, + resetOpenAiOrphanToolResultExtensionCache, + writeOpenAiOrphanToolResultExtension, +} from "../src/server/agent/openai-orphan-tool-result-extension.ts"; + +describe("dropOrphanFunctionCallOutputsFromPayload", () => { + it("keeps a matching function_call and function_call_output unchanged", () => { + const payload = { + model: "codex-test", + input: [ + { type: "message", role: "user", content: "hi" }, + { type: "function_call", call_id: "call-ok", name: "read", arguments: "{}" }, + { type: "function_call_output", call_id: "call-ok", output: "done" }, + ], + }; + + const result = dropOrphanFunctionCallOutputsFromPayload(payload); + + assert.equal(result.changed, false); + assert.equal(result.dropped, 0); + assert.equal(result.payload, payload); + }); + + it("drops a payload-leading orphan function_call_output", () => { + const payload = { + input: [ + { type: "function_call_output", call_id: "missing", output: "late" }, + { type: "message", role: "user", content: "continue" }, + ], + }; + + const result = dropOrphanFunctionCallOutputsFromPayload(payload); + + assert.equal(result.changed, true); + assert.equal(result.dropped, 1); + assert.deepEqual(result.payload, { + input: [{ type: "message", role: "user", content: "continue" }], + }); + }); + + it("preserves order and valid pairs while dropping only orphan outputs", () => { + const validCall = { type: "function_call", call_id: "valid", name: "bash", arguments: "{}" }; + const validOutput = { type: "function_call_output", call_id: "valid", output: "ok" }; + const laterCall = { type: "function_call", call_id: "later", name: "read", arguments: "{}" }; + const payload = { + metadata: { keep: true }, + input: [ + { type: "message", role: "user", content: "start" }, + { type: "function_call_output", call_id: "missing-a", output: "drop-a" }, + validCall, + validOutput, + { type: "function_call_output", call_id: "later", output: "drop-before-call" }, + laterCall, + { type: "function_call_output", call_id: "later", output: "keep-after-call" }, + { type: "message", role: "user", content: "end" }, + ], + }; + + const result = dropOrphanFunctionCallOutputsFromPayload(payload); + + assert.equal(result.changed, true); + assert.equal(result.dropped, 2); + assert.deepEqual(result.payload, { + metadata: { keep: true }, + input: [ + { type: "message", role: "user", content: "start" }, + validCall, + validOutput, + laterCall, + { type: "function_call_output", call_id: "later", output: "keep-after-call" }, + { type: "message", role: "user", content: "end" }, + ], + }); + }); + + it("no-ops for non-Responses and non-OpenAI-shaped payloads", () => { + const nonObject = "not a payload"; + const noInput = { contents: [{ role: "user", parts: [{ text: "hi" }] }] }; + const nonArrayInput = { input: { type: "message" } }; + const noOutputs = { input: [{ type: "function_call", call_id: "call-1" }] }; + + for (const payload of [nonObject, noInput, nonArrayInput, noOutputs]) { + const result = dropOrphanFunctionCallOutputsFromPayload(payload); + assert.equal(result.changed, false); + assert.equal(result.dropped, 0); + assert.equal(result.payload, payload); + } + }); + + it("counts dropped diagnostics without exposing raw output content", () => { + const secretOutput = "SECRET_TOOL_OUTPUT_SHOULD_NOT_BE_LOGGED"; + const payload = { + input: [ + { type: "function_call_output", call_id: "missing-a", output: secretOutput }, + { type: "function_call_output", output: "missing call id" }, + ], + }; + + const result = dropOrphanFunctionCallOutputsFromPayload(payload); + + assert.equal(result.dropped, 2); + assert.equal(result.changed, true); + assert.equal(JSON.stringify({ dropped: result.dropped }), "{\"dropped\":2}"); + assert.ok(!JSON.stringify({ dropped: result.dropped }).includes(secretOutput)); + }); +}); + +describe("generateOpenAiOrphanToolResultExtension", () => { + const previousBobbitDir = process.env.BOBBIT_DIR; + const tempDirs: string[] = []; + + afterEach(() => { + resetOpenAiOrphanToolResultExtensionCache(); + if (previousBobbitDir === undefined) delete process.env.BOBBIT_DIR; + else process.env.BOBBIT_DIR = previousBobbitDir; + for (const dir of tempDirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); + }); + + it("registers the before_provider_request hook and logs only a bounded count", () => { + const source = generateOpenAiOrphanToolResultExtension(); + + assert.ok(source.includes('pi.on("before_provider_request"')); + assert.ok(source.includes("event && event.payload")); + assert.ok(source.includes("[bobbit-openai-orphan-guard] Dropped ")); + assert.ok(!source.includes("output:"), "generated log path must not include raw tool output"); + }); + + it("writes the guard under a dedicated read-only sandbox mount subdir, not writable tool-guard", () => { + const bobbitDir = fs.mkdtempSync(path.join(os.tmpdir(), "openai-orphan-guard-")); + tempDirs.push(bobbitDir); + process.env.BOBBIT_DIR = bobbitDir; + resetOpenAiOrphanToolResultExtensionCache(); + + const extensionPath = writeOpenAiOrphanToolResultExtension(); + + assert.ok(extensionPath, "expected extension path"); + assert.ok(fs.existsSync(extensionPath), "expected extension file to be written"); + assert.ok( + extensionPath!.includes(path.join("state", OPENAI_ORPHAN_TOOL_RESULT_STATE_SUBDIR)), + `expected dedicated state subdir path, got ${extensionPath}`, + ); + assert.ok( + !extensionPath!.includes(path.join("state", "tool-guard")), + `OpenAI provider guard must not be written under writable tool-guard, got ${extensionPath}`, + ); + }); +}); diff --git a/tests/transcript-sanitizer.test.ts b/tests/transcript-sanitizer.test.ts index 92c2318ab..5d2b69a44 100644 --- a/tests/transcript-sanitizer.test.ts +++ b/tests/transcript-sanitizer.test.ts @@ -10,6 +10,8 @@ * (d) string-content blank user message → rewritten * (e) valid transcripts pass through unchanged + byte-identical (idempotent) * (f) assistant / non-user blank messages are NOT touched + * (g) valid tool-result history is left byte-identical while orphan results + * are dropped or filtered * * Run with: * npx tsx --test --test-force-exit tests/transcript-sanitizer.test.ts @@ -33,6 +35,52 @@ function msg(role: string, content: unknown, id = "x"): string { return JSON.stringify({ type: "message", id, ts: "2026-01-01T00-00-00-000Z", message: { role, content } }); } +function assistantToolCall(toolCallId: string, id = `assistant-${toolCallId}`, stopReason?: string): string { + return JSON.stringify({ + type: "message", + id, + ts: "2026-01-01T00-00-00-000Z", + message: { + role: "assistant", + content: [{ type: "toolCall", id: toolCallId, name: "bash", input: { command: "echo ok" } }], + ...(stopReason ? { stopReason } : {}), + }, + }); +} + +function assistantToolUse(toolCallId: string, id = `assistant-${toolCallId}`): string { + return JSON.stringify({ + type: "message", + id, + ts: "2026-01-01T00-00-00-000Z", + message: { + role: "assistant", + content: [{ type: "tool_use", id: toolCallId, name: "bash", input: { command: "echo ok" } }], + }, + }); +} + +function assistantPiToolCall(toolCallId: string, id = `assistant-${toolCallId}`): string { + return JSON.stringify({ + type: "message", + id, + ts: "2026-01-01T00-00-00-000Z", + message: { + role: "assistant", + content: [{ toolCallId, toolName: "bash", args: { command: "echo ok" } }], + }, + }); +} + +function toolResultRow(toolCallId: string, id = `result-${toolCallId}`): string { + return JSON.stringify({ + type: "message", + id, + ts: "2026-01-01T00-00-00-000Z", + message: { role: "toolResult", toolCallId, content: "ok done" }, + }); +} + const IMG = { type: "image", source: { data: "AAAA" } }; describe("sanitizeTranscriptContent", () => { @@ -95,27 +143,352 @@ describe("sanitizeTranscriptContent", () => { assert.equal(content, line); }); - it("(g) leaves a tool_result-only user message byte-identical (no text by design)", () => { - const line = msg("user", [{ type: "tool_result", toolCallId: "t1", content: "ok done" }]); - const { changed, content } = sanitizeTranscriptContent(line); - assert.equal(changed, false, "tool_result user message must NOT be rewritten"); - assert.equal(content, line, "tool_result user message must stay byte-identical"); + it("(g) leaves a valid toolCall + toolResult row pair byte-identical and idempotent", () => { + const file = [assistantToolCall("t1"), toolResultRow("t1")].join("\n"); + const first = sanitizeTranscriptContent(file); + assert.equal(first.changed, false, "valid message-level toolResult must stay unchanged"); + assert.equal(first.content, file, "valid tool-call history must stay byte-identical"); + assert.equal(first.droppedToolResultRows, 0); + assert.equal(first.filteredToolResultBlocks, 0); + + const second = sanitizeTranscriptContent(first.content); + assert.equal(second.changed, false, "valid pair sanitizer pass must remain a no-op"); + assert.equal(second.content, file); }); - it("(g') leaves a toolResult-variant user message byte-identical", () => { - const line = msg("user", [{ type: "toolResult", toolCallId: "t2", content: [{ type: "text", text: "" }] }]); - const { changed, content } = sanitizeTranscriptContent(line); - assert.equal(changed, false, "toolResult user message must NOT be rewritten"); - assert.equal(content, line, "toolResult user message must stay byte-identical"); + it("(g') leaves a valid tool_use + toolResult-block user message byte-identical", () => { + const result = msg("user", [{ type: "toolResult", toolCallId: "t2", content: [{ type: "text", text: "" }] }], "result-t2"); + const file = [assistantToolUse("t2"), result].join("\n"); + const { changed, content } = sanitizeTranscriptContent(file); + assert.equal(changed, false, "valid toolResult user message must NOT be rewritten"); + assert.equal(content, file, "valid toolResult user message must stay byte-identical"); }); - it("(g'') leaves a tool_result + blank-text user message byte-identical (tool result wins)", () => { - // Even if a stray empty text block coexists, the presence of a tool_result - // block means this is tool-call history and must not be touched. - const line = msg("user", [{ type: "text", text: "" }, { type: "tool_result", content: "x" }]); - const { changed, content } = sanitizeTranscriptContent(line); + it("(g' pi) leaves a valid pi {toolCallId,toolName} + toolResult row pair byte-identical", () => { + const file = [assistantPiToolCall("pi-tool"), toolResultRow("pi-tool")].join("\n"); + const first = sanitizeTranscriptContent(file); + assert.equal(first.changed, false, "pi-shaped assistant tool call must validate the matching toolResult row"); + assert.equal(first.content, file, "valid pi-shaped tool-call history must stay byte-identical"); + assert.equal(first.droppedToolResultRows, 0); + + const second = sanitizeTranscriptContent(first.content); + assert.equal(second.changed, false, "valid pi-shaped pair sanitizer pass must remain a no-op"); + assert.equal(second.content, file); + }); + + it("(g'') leaves a valid tool_result + blank-text user message byte-identical (tool result wins)", () => { + // Even if a stray empty text block coexists, the presence of a valid + // tool_result block means this is tool-call history and must not be touched. + const result = msg("user", [{ type: "text", text: "" }, { type: "tool_result", tool_use_id: "t3", content: "x" }], "result-t3"); + const file = [assistantToolCall("t3"), result].join("\n"); + const { changed, content } = sanitizeTranscriptContent(file); assert.equal(changed, false); - assert.equal(content, line); + assert.equal(content, file); + }); + + it("(g''' toolCall) recognizes a typed toolCall block carrying toolCallId instead of id", () => { + // Regression (PR #845): toolCallIdFromAssistantBlock() previously only + // accepted `id` for typed `toolCall`/`tool_use` blocks, so a block carrying + // `toolCallId` (or `tool_use_id`) instead of `id` was skipped and its + // matching toolResult wrongly dropped as orphaned. Match action-guard.ts. + const assistant = JSON.stringify({ + type: "message", + id: "assistant-tc-alt", + ts: "2026-01-01T00-00-00-000Z", + message: { + role: "assistant", + content: [{ type: "toolCall", toolCallId: "tc-alt", name: "bash", input: { command: "echo ok" } }], + }, + }); + const result = toolResultRow("tc-alt", "result-tc-alt"); + const file = [assistant, result].join("\n"); + const { changed, content, droppedToolResultRows } = sanitizeTranscriptContent(file); + assert.equal(changed, false, "typed toolCall with toolCallId must validate the matching toolResult"); + assert.equal(droppedToolResultRows, 0); + assert.equal(content, file, "valid typed-toolCall history must stay byte-identical"); + }); + + it("(g''' tool_use) recognizes a typed tool_use block carrying tool_use_id instead of id", () => { + const assistant = JSON.stringify({ + type: "message", + id: "assistant-tu-alt", + ts: "2026-01-01T00-00-00-000Z", + message: { + role: "assistant", + content: [{ type: "tool_use", tool_use_id: "tu-alt", name: "bash", input: { command: "echo ok" } }], + }, + }); + const result = toolResultRow("tu-alt", "result-tu-alt"); + const file = [assistant, result].join("\n"); + const { changed, content, droppedToolResultRows } = sanitizeTranscriptContent(file); + assert.equal(changed, false, "typed tool_use with tool_use_id must validate the matching toolResult"); + assert.equal(droppedToolResultRows, 0); + assert.equal(content, file); + }); + + it("drops a toolResult row whose only matching assistant row was aborted", () => { + const assistant = assistantToolCall("aborted-tool", "assistant-aborted", "aborted"); + const result = toolResultRow("aborted-tool", "late-result"); + const after = msg("user", "after", "after"); + const file = [assistant, result, after].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.rewritten, 0); + assert.equal(sanitized.droppedToolResultRows, 1); + assert.equal(sanitized.filteredToolResultBlocks, 0); + assert.equal(sanitized.content, [assistant, after].join("\n")); + + const second = sanitizeTranscriptContent(sanitized.content); + assert.equal(second.changed, false, "orphan repair must be idempotent"); + assert.equal(second.content, sanitized.content); + }); + + it("drops a toolResult row whose only matching assistant row errored", () => { + const assistant = assistantToolCall("errored-tool", "assistant-error", "error"); + const result = toolResultRow("errored-tool", "late-error-result"); + const file = [assistant, result].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.droppedToolResultRows, 1); + assert.equal(sanitized.content, assistant); + }); + + it("drops a toolResult row after a compaction boundary with no retained tool call", () => { + const user = msg("user", "retained prompt", "retained-user"); + const compaction = JSON.stringify({ type: "compaction", id: "compact-1" }); + const result = toolResultRow("pre-compaction-tool", "orphan-after-compaction"); + const file = [user, compaction, result].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.droppedToolResultRows, 1); + assert.equal(sanitized.content, [user, compaction].join("\n")); + }); + + it("drops a post-compaction toolResult whose matching assistant tool call was before the compaction marker", () => { + // Regression (PR #845): seenToolCallIds used to persist across the + // `compaction` marker, so a post-compaction toolResult could incorrectly + // match a pre-compaction assistant tool call that is no longer in the + // retained context — rehydrating an orphan function_call_output. + const assistant = assistantToolCall("pre-compact-call", "assistant-pre-compact"); + const compaction = JSON.stringify({ type: "compaction", id: "compact-1" }); + const result = toolResultRow("pre-compact-call", "late-result-after-compaction"); + const after = msg("user", "after", "after"); + const file = [assistant, compaction, result, after].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.droppedToolResultRows, 1); + assert.equal(sanitized.filteredToolResultBlocks, 0); + assert.equal(sanitized.content, [assistant, compaction, after].join("\n")); + + const second = sanitizeTranscriptContent(sanitized.content); + assert.equal(second.changed, false, "compaction-boundary orphan repair must be idempotent"); + assert.equal(second.content, sanitized.content); + }); + + it("keeps a toolCall before the marker when firstKeptEntryId names an earlier retained user row", () => { + // Pi's exact retained-range boundary can point to any retained entry, not + // just an assistant tool-call row. A later retained assistant tool call before + // the inline compaction marker is still valid for post-marker results. + const retainedUser = msg("user", "retained prompt", "retained-user"); + const assistant = assistantToolCall("kept-before-marker-call", "assistant-kept-before-marker"); + const compaction = JSON.stringify({ type: "compaction", id: "compact-1", firstKeptEntryId: "retained-user" }); + const result = toolResultRow("kept-before-marker-call", "result-after-marker"); + const file = [retainedUser, assistant, compaction, result].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, false, "valid retained-range pair must stay unchanged"); + assert.equal(sanitized.content, file, "valid retained-range pair must stay byte-identical"); + assert.equal(sanitized.droppedToolResultRows, 0); + }); + + it("keeps a valid toolCall + toolResult pair when both are after the compaction marker", () => { + // A post-compaction assistant tool call matched by a post-compaction + // toolResult is valid retained history and must stay byte-identical. + const preUser = msg("user", "retained prompt", "retained-user"); + const compaction = JSON.stringify({ type: "compaction", id: "compact-1" }); + const assistant = assistantToolCall("post-compact-call", "assistant-post-compact"); + const result = toolResultRow("post-compact-call", "post-compact-result"); + const file = [preUser, compaction, assistant, result].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, false, "valid post-compaction pair must stay unchanged"); + assert.equal(sanitized.content, file, "valid post-compaction pair must stay byte-identical"); + assert.equal(sanitized.droppedToolResultRows, 0); + }); + + it("keeps a post-compaction toolResult whose matching assistant call is in the retained range before the marker (resolvable firstKeptEntryId)", () => { + // Regression (PR #845 review): do NOT blindly clear seen ids at every + // compaction marker. The compaction entry carries a resolvable + // `firstKeptEntryId` pointing at a kept assistant tool-call entry that + // appears BEFORE the marker (the retained tail the marker trails). That + // call is still in the retained context, so a later toolResult matching it + // must be kept — not dropped as an orphan. + const keptAssistant = assistantToolCall("kept-range-call", "assistant-kept-range"); + const compaction = JSON.stringify({ + type: "compaction", + id: "compact-1", + firstKeptEntryId: "assistant-kept-range", + }); + const result = toolResultRow("kept-range-call", "late-result"); + const after = msg("user", "after", "after"); + const file = [keptAssistant, compaction, result, after].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, false, "retained-range call before marker must validate the later result"); + assert.equal(sanitized.droppedToolResultRows, 0); + assert.equal(sanitized.content, file, "valid retained-range pair must stay byte-identical"); + + const second = sanitizeTranscriptContent(sanitized.content); + assert.equal(second.changed, false, "retained-range repair must be idempotent"); + assert.equal(second.content, sanitized.content); + }); + + it("drops a toolResult whose matching assistant call was before firstKeptEntryId (summarized away)", () => { + // Two pre-marker assistant tool calls; the compaction entry's + // `firstKeptEntryId` points at the SECOND (kept) entry. A result matching + // the FIRST entry (strictly before the kept boundary) was summarized away + // and must be dropped, while a result matching the kept entry is retained. + const summarizedAssistant = assistantToolCall("summarized-call", "assistant-summarized"); + const keptAssistant = assistantToolCall("kept-call", "assistant-kept"); + const compaction = JSON.stringify({ + type: "compaction", + id: "compact-1", + firstKeptEntryId: "assistant-kept", + }); + const droppedResult = toolResultRow("summarized-call", "dropped-result"); + const keptResult = toolResultRow("kept-call", "kept-result"); + const after = msg("user", "after", "after"); + const file = [summarizedAssistant, keptAssistant, compaction, droppedResult, keptResult, after].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.droppedToolResultRows, 1, "summarized-away call result must be dropped"); + assert.equal(sanitized.content, [summarizedAssistant, keptAssistant, compaction, keptResult, after].join("\n")); + + const second = sanitizeTranscriptContent(sanitized.content); + assert.equal(second.changed, false, "firstKeptEntryId-boundary repair must be idempotent"); + assert.equal(second.content, sanitized.content); + }); + + it("falls back to clearing at the marker when firstKeptEntryId is absent (legacy compaction entry)", () => { + // No firstKeptEntryId → conservative fallback: clear everything at the + // marker, so a pre-compaction call cannot validate a post-compaction result. + const assistant = assistantToolCall("pre-call", "assistant-pre"); + const compaction = JSON.stringify({ type: "compaction", id: "compact-1" }); + const result = toolResultRow("pre-call", "late-result"); + const file = [assistant, compaction, result].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.droppedToolResultRows, 1); + assert.equal(sanitized.content, [assistant, compaction].join("\n")); + }); + + it("falls back to clearing at the marker when firstKeptEntryId is unresolvable", () => { + // firstKeptEntryId present but doesn't match any tracked retained tool + // call's entry id → unresolved → conservative fallback clear at marker. + const assistant = assistantToolCall("pre-call", "assistant-pre"); + const compaction = JSON.stringify({ + type: "compaction", + id: "compact-1", + firstKeptEntryId: "does-not-exist", + }); + const result = toolResultRow("pre-call", "late-result"); + const file = [assistant, compaction, result].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.droppedToolResultRows, 1); + assert.equal(sanitized.content, [assistant, compaction].join("\n")); + }); + + it("filters orphan user/content tool_result blocks while preserving valid blocks and other content", () => { + const assistant = assistantToolCall("valid-block"); + const userLine = msg("user", [ + { type: "text", text: "tool outputs:" }, + { type: "tool_result", tool_use_id: "valid-block", content: "keep me" }, + { type: "toolResult", toolCallId: "orphan-block", content: "drop me" }, + IMG, + ], "mixed-results"); + const file = [assistant, userLine].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.droppedToolResultRows, 0); + assert.equal(sanitized.filteredToolResultBlocks, 1); + + const lines = sanitized.content.split("\n"); + assert.equal(lines[0], assistant, "assistant tool call must stay byte-identical"); + const blocks = JSON.parse(lines[1]).message.content; + assert.deepEqual(blocks, [ + { type: "text", text: "tool outputs:" }, + { type: "tool_result", tool_use_id: "valid-block", content: "keep me" }, + IMG, + ]); + + const second = sanitizeTranscriptContent(sanitized.content); + assert.equal(second.changed, false, "filtered transcript must be idempotent"); + assert.equal(second.content, sanitized.content); + }); + + it("rewrites blank/image-only user content after filtering orphan tool_result blocks", () => { + const userLine = msg("user", [ + { type: "text", text: " " }, + { type: "tool_result", tool_use_id: "missing", content: "drop me" }, + IMG, + ], "orphan-plus-attachment"); + + const sanitized = sanitizeTranscriptContent(userLine); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.filteredToolResultBlocks, 1); + assert.equal(sanitized.rewritten, 1); + + const blocks = JSON.parse(sanitized.content).message.content; + assert.deepEqual(blocks, [ + { type: "text", text: "Attachments:" }, + IMG, + ]); + + const second = sanitizeTranscriptContent(sanitized.content); + assert.equal(second.changed, false, "filter + blank-user repair must be idempotent in one pass"); + assert.equal(second.content, sanitized.content); + }); + + it("rewrites image-only user content after filtering orphan toolResult blocks", () => { + const userLine = msg("user", [ + { type: "toolResult", toolCallId: "missing", content: "drop me" }, + IMG, + ], "orphan-plus-image"); + + const sanitized = sanitizeTranscriptContent(userLine); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.filteredToolResultBlocks, 1); + assert.equal(sanitized.rewritten, 1); + + const blocks = JSON.parse(sanitized.content).message.content; + assert.deepEqual(blocks, [ + { type: "text", text: "Attachments:" }, + IMG, + ]); + + const second = sanitizeTranscriptContent(sanitized.content); + assert.equal(second.changed, false, "image-only remainder must not need a second repair pass"); + assert.equal(second.content, sanitized.content); + }); + + it("drops a user/content tool_result-only row when every block is orphaned", () => { + const orphan = msg("user", [{ type: "tool_result", tool_use_id: "missing", content: "drop" }], "orphan-user-result"); + const next = msg("user", "next", "next"); + const file = [orphan, next].join("\n"); + + const sanitized = sanitizeTranscriptContent(file); + assert.equal(sanitized.changed, true); + assert.equal(sanitized.filteredToolResultBlocks, 1); + assert.equal(sanitized.content, next); }); it("preserves other lines and only rewrites poisoned ones in a multi-line file", () => {