diff --git a/README.md b/README.md index dde0fea97..3e9fe972b 100644 --- a/README.md +++ b/README.md @@ -290,6 +290,13 @@ Three standalone, lowercase words opt a turn into specialized agent behavior: They trigger only in prose, not inside code spans, fenced code blocks, XML/HTML sections, identifiers, or paths. See [Magic keywords](docs/magic-keywords.md) for exact matching rules and configuration. +### Session controls + +Slash commands shift how a whole session runs: + +- `/vibe` — enter [Vibe mode](docs/vibe-mode.md): act as a director driving persistent `fast`/`good` worker sessions with a `read`-only toolset. +- `/fresh` — reset the provider stream state (stale prompt cache, wedged stream) without changing the local transcript. See [Session operations](docs/session-operations-export-share-fork-resume.md#fresh). + ## Forty-plus providers, hundreds of models, _one /model away_. Roles route work by intent. `default` for normal turns. `smol` for cheap subagent fan-out. `slow` for deep reasoning. `plan` for plan mode. `commit` for changelogs. Override at launch with `--smol`, `--slow`, or `--plan`; cycle through the configured models for the active role with `Ctrl+P`. Swap the active model mid-session with the `/model` slash command. diff --git a/docs/session-operations-export-share-fork-resume.md b/docs/session-operations-export-share-fork-resume.md index 010512b69..4830f303f 100644 --- a/docs/session-operations-export-share-fork-resume.md +++ b/docs/session-operations-export-share-fork-resume.md @@ -155,6 +155,31 @@ Cancellation/abort semantics in share: - The upload itself is not aborted mid-flight; cancellation is UI-level and checked after the upload returns. +## Fresh + +Interactive `/fresh` resets the provider-facing stream state of the current +session **without touching the local transcript, session file, or header**. Use +it to recover from a wedged or corrupted provider stream (stale prompt cache, +a mid-turn glitch, or a server-side conversation id that has drifted) while +keeping the conversation you can see. + +`AgentSession.freshSession()`: + +- Is rejected while the agent is streaming — wait for the response to finish or + abort it first. +- Closes every cached provider-session state entry (server-side conversation / + prompt-cache handles) and reports how many were pruned. +- Mints a fresh provider session id and re-keys hindsight and mnemopi memory to + it, and invalidates the append-only context so the next turn re-sends the full + local transcript to the provider. +- Leaves the local transcript, session file, and session identity unchanged, so + nothing you have said or received is lost. + +Because it keeps the current session file, `/fresh` differs from `/new` (start a +brand-new empty session) and `/drop` (delete the current session and start a new +one): only `/fresh` preserves the visible history while giving the provider a +clean slate. + ## Fork Interactive `/fork` creates a new session from the current one and switches the active session identity. diff --git a/docs/vibe-mode.md b/docs/vibe-mode.md new file mode 100644 index 000000000..4f3a67d7b --- /dev/null +++ b/docs/vibe-mode.md @@ -0,0 +1,71 @@ +# Vibe mode + +Vibe mode turns the session into a **director** that drives persistent background +worker sessions instead of editing code itself. In vibe mode your own toolset is +stripped down to `read` plus five worker-control tools; the workers do the +grepping, editing, running, and building, and you verify their work by reading +the files they touch. + +## Enabling and disabling + +Toggle it with the `/vibe` slash command: + +```text +/vibe # enter vibe mode +/vibe fix the flaky test in packages/tui # enter and submit a first directive +/vibe # run again to exit +``` + +- Entering installs the vibe tools, reduces the active toolset to `read` + the + vibe tools, and injects the director instructions for the turn. +- An inline prompt (`/vibe `) enters the mode and submits that prompt as + the first directive. +- Exiting restores the previous toolset and **kills every worker session** — a + worker never outlives the mode that directs it. +- Vibe mode is mutually exclusive with plan mode and goal mode; exit those + first. The status line shows a `Vibe` indicator while it is on. + +## The two worker tiers + +Every worker is a full coding agent with the normal tool surface. You choose a +tier when you spawn one: + +| Tier | Backing agent | Model role | Use for | +|--------|---------------|------------|---------| +| `fast` | `sonic` | `@smol` (low-latency role) | Mechanical execution, drafts, high-volume work | +| `good` | `task` | `@task` (the session's strong model) | Design, judgment calls, reviewing `fast` output | + +Model resolution follows the same path as a `task` spawn, so +`task.agentModelOverrides` and your model-role settings apply. + +## Worker-control tools + +| Tool | Purpose | +|------|---------| +| `vibe_spawn` | Start a worker (`fast` or `good`) with a complete, self-contained brief. Workers start blank — they never see the director's conversation. | +| `vibe_send` | Send a follow-up turn to a worker: a correction, the next step, or a review request. | +| `vibe_wait` | Block until a worker settles its next turn. Sends and spawns return immediately; results arrive on their own, so call `vibe_wait` only when you cannot proceed without one. | +| `vibe_kill` | Tear down a worker that is stuck or whose workstream is done. | +| `vibe_list` | List the active worker roster when you lose track of it. | + +A spawn or send returns immediately; the worker's turn result is delivered back +into the director's conversation on its own, exactly like an async `task` +result. Running one `fast` and one `good` worker on different workstreams +concurrently is the normal shape. + +## Workflow + +1. Split the request into independent workstreams — one worker session per + workstream so each builds useful local context. +2. `vibe_spawn` with a self-contained brief: files, constraints, acceptance + criteria. +3. Keep directing other workers while turns are in flight; `vibe_wait` only when + blocked. +4. When a turn result arrives, `read` the touched files to verify claims before + building on them, then `vibe_send` the next step. +5. Route by difficulty: draft with `fast`, escalate to `good` when `fast` stalls + or the problem needs judgment. +6. `vibe_kill` finished or stuck workers; `vibe_list` to recover the roster. + +You stay responsible for the final outcome — verify with `read`, never take a +worker's word for it. diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 56d90b01a..7ed4bafab 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -6,11 +6,19 @@ - Queued steering no longer hard-aborts non-interruptible tools (e.g. `bash`): it aborts interruptible waits only and raises a cooperative steering signal (`ToolCallContext.steeringSignal`) that long-running tools may observe to finish early or background themselves. The mid-batch steering/IRC watch now runs for every tool batch instead of only batches containing an interruptible tool. +### Fixed + +- Fixed an unbounded allocation loop when a steer (or follow-up) was queued on a session with an empty transcript: `Agent.continue()` now delivers the queued message as the opening turn instead of throwing, so idle-drain callers no longer respin `continue()` on every microtask until OOM ([#6344](https://github.com/can1357/oh-my-pi/issues/6344)). +- Fixed provider-switched sessions being stranded without their remotely-compacted history: compaction now judges whether a prior OpenAI remote-compaction replay payload can be reused against the active model rather than the whole candidate set, so switching to a model that cannot replay the payload re-expands the originals into a portable local summary instead of leaving the model with only a placeholder ([#6343](https://github.com/can1357/oh-my-pi/issues/6343)). + ## [17.0.8] - 2026-07-22 ### Fixed - Improved resilience against transient stream JSON parse failures by recovering completed tool calls while safely preventing incomplete, unknown, refused, or sensitive calls from executing. +### Fixed + +- Fixed compaction/summarization serializing prior assistant reasoning back to Claude as text (rendered verbatim inside `` tags for the `anthropic` dialect), which tripped Anthropic's `reasoning_extraction` refusal and blocked compaction on Fable 5 sessions; `serializeConversation` now drops `thinking` blocks for Anthropic-dialect summary targets while other dialects (e.g. Harmony) keep their native reasoning ([#6093](https://github.com/can1357/oh-my-pi/issues/6093)). ## [17.0.5] - 2026-07-18 diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index 090eb0de0..bb9d48d71 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -1047,6 +1047,22 @@ export class Agent { const messages = this.#state.messages; if (messages.length === 0) { + // An empty transcript has nothing to resume, but a queued steer/follow-up + // must still be delivered as the opening turn — mirroring the assistant-tail + // branch below. Throwing here leaves the message undeliverable, and idle-drain + // callers (AgentSession#scheduleQueuedMessageDrain) re-arm continue() on every + // microtask because hasQueuedMessages() never clears, spinning an unbounded + // allocation loop until OOM (issue #6344). + const queuedSteering = this.#dequeueSteeringMessages(); + if (queuedSteering.length > 0) { + await this.#runLoop(queuedSteering, { skipInitialSteeringPoll: true }); + return; + } + const queuedFollowUp = this.#dequeueFollowUpMessages(); + if (queuedFollowUp.length > 0) { + await this.#runLoop(queuedFollowUp); + return; + } throw new Error("No messages to continue from"); } if (messages[messages.length - 1].role === "assistant") { diff --git a/packages/agent/src/compaction/compaction.ts b/packages/agent/src/compaction/compaction.ts index 3781ed431..7f0bf7980 100644 --- a/packages/agent/src/compaction/compaction.ts +++ b/packages/agent/src/compaction/compaction.ts @@ -1130,33 +1130,37 @@ export interface CompactionPreparation { } /** - * Whether a prior compaction's preserve data can be carried forward by the - * upcoming compaction. A local compaction (no remote preserve) always can — it - * holds a real textual summary. A remote compaction (V2 or V1) only can when - * some candidate model shares its provider AND remote replay is still enabled; - * otherwise its provider-native replay is dead weight and only the opaque - * placeholder summary survives, so the caller must re-expand the originals. + * Whether a prior remote compaction's provider-native replay can still be read + * by the active model — the model that assembles the request context on every + * turn. A local compaction (no remote preserve) always can: it holds a real + * textual summary. A remote compaction (V2 or V1) only can when the active model + * shares the blob's provider AND remote replay is still enabled; otherwise the + * active model's encoder drops the payload (see `getOpenAIResponsesHistoryPayload`) + * and only the opaque placeholder summary survives, so the caller must re-expand + * the originals into a portable local summary rather than strand that history. + * + * Judged against the ACTIVE model, not the compaction candidate set: a role + * model (e.g. `modelRoles.smol`) that still maps to the blob's provider does not + * let the active model replay it, so keying reuse on "any candidate shares the + * provider" left a provider-switched session permanently context-less (#6343). */ -function remotePreserveReusableByAny( +function remotePreserveReusable( preserveData: Record | undefined, - models: readonly Model[], + activeModel: Model, settings: CompactionSettings, ): boolean { const remote = getCompactionV2PreserveData(preserveData) ?? getPreservedOpenAiRemoteCompactionData(preserveData); if (!remote) return true; if (settings.remoteEnabled === false) return false; - for (const model of models) { - if (remote.provider !== model.provider) continue; - const v2Ok = settings.remoteStreamingV2Enabled !== false && shouldUseCompactionV2Streaming(model); - if (v2Ok || shouldUseOpenAiRemoteCompaction(model)) return true; - } - return false; + if (remote.provider !== activeModel.provider) return false; + const v2Ok = settings.remoteStreamingV2Enabled !== false && shouldUseCompactionV2Streaming(activeModel); + return v2Ok || shouldUseOpenAiRemoteCompaction(activeModel); } export function prepareCompaction( pathEntries: SessionEntry[], settings: CompactionSettings, - compactionModels: readonly Model[] = [], + activeModel?: Model, ): CompactionPreparation | undefined { if (pathEntries.length > 0 && pathEntries[pathEntries.length - 1].type === "compaction") { return undefined; @@ -1165,13 +1169,13 @@ export function prepareCompaction( let prevCompactionIndex = -1; for (let i = pathEntries.length - 1; i >= 0; i--) { if (pathEntries[i].type !== "compaction") continue; - // Skip a prior remote compaction (V2 or V1) whose provider-native replay - // none of the upcoming compaction candidates can reuse: its summary is only - // an opaque placeholder, so re-expand its original messages and summarize - // them locally rather than stranding that history. compact() still reuses it - // when a candidate can (same provider, remote enabled). + // Skip a prior remote compaction (V2 or V1) whose provider-native replay the + // active model cannot read: its summary is only an opaque placeholder, so + // re-expand its original messages and summarize them locally rather than + // stranding that history. compact() still reuses the payload when the active + // model can replay it (same provider, remote enabled). const entry = pathEntries[i] as CompactionEntry; - if (compactionModels.length > 0 && !remotePreserveReusableByAny(entry.preserveData, compactionModels, settings)) { + if (activeModel && !remotePreserveReusable(entry.preserveData, activeModel, settings)) { continue; } prevCompactionIndex = i; @@ -1502,7 +1506,7 @@ export async function compact( // summarization so a successful remote compaction never pays for a second, // redundant LLM round. If a LATER compaction cannot reuse this payload, // prepareCompaction re-expands the original messages and summarizes them - // locally then (see remotePreserveReusableByAny). + // locally then (see remotePreserveReusable). const usedTokens = getCompactionV2PreserveData(preserveData)?.usedTokens ?? 0; summary = "Remote compaction preserved provider-native history for this session." + diff --git a/packages/agent/src/compaction/utils.ts b/packages/agent/src/compaction/utils.ts index 286f4e7f9..e38290bdd 100644 --- a/packages/agent/src/compaction/utils.ts +++ b/packages/agent/src/compaction/utils.ts @@ -234,10 +234,21 @@ export function serializeConversation(messages: Message[], dialect?: Dialect): s } } if (dialect) { + // Claude's classifier refuses inputs that reproduce the model's own + // reasoning as text ("reasoning_extraction"), and the anthropic dialect + // otherwise renders thinking verbatim inside tags. Reasoning is + // ephemeral and low-signal for a summary, so drop it from Anthropic-target + // summary input. Other dialects (e.g. Harmony) carry reasoning natively in + // their transcript format and keep it. + const dropThinking = dialect === "anthropic"; const processed: Message[] = []; for (const msg of messages) { if (msg.role === "assistant") { - const content = msg.content.filter(block => block.type !== "toolCall" || !uselessCallIds.has(block.id)); + const content = msg.content.filter( + block => + (block.type !== "toolCall" || !uselessCallIds.has(block.id)) && + (!dropThinking || block.type !== "thinking"), + ); if (content.length > 0) processed.push(content.length === msg.content.length ? msg : { ...msg, content }); continue; } diff --git a/packages/agent/test/continue-empty-transcript.test.ts b/packages/agent/test/continue-empty-transcript.test.ts new file mode 100644 index 000000000..5ef89410d --- /dev/null +++ b/packages/agent/test/continue-empty-transcript.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "bun:test"; +import { Agent } from "@oh-my-pi/pi-agent-core"; +import { createMockModel } from "@oh-my-pi/pi-ai/providers/mock"; + +/** + * Regression: a `steer` (or follow-up) queued on an empty transcript must be + * delivered as the opening turn instead of leaving `continue()` to throw + * `No messages to continue from`. Before the fix the steer stayed queued, so the + * RPC idle-drain re-armed `continue()` on every microtask (gated only on + * `hasQueuedMessages()`), an unbounded allocation loop that OOM-killed the + * process (issue #6344). + */ +describe("Agent.continue() on an empty transcript", () => { + it("runs a queued steer as the opening turn and drains the queue", async () => { + const mock = createMockModel({ responses: [{ content: ["Answer"] }] }); + const agent = new Agent({ streamFn: mock.stream }); + + agent.steer({ + role: "user", + content: [{ type: "text", text: "hello" }], + timestamp: Date.now(), + }); + + await expect(agent.continue()).resolves.toBeUndefined(); + + expect(mock.calls.length).toBe(1); + expect(agent.hasQueuedMessages()).toBe(false); + expect(agent.state.messages.map(m => m.role)).toEqual(["user", "assistant"]); + const steerDelivered = agent.state.messages.some( + m => + m.role === "user" && + Array.isArray(m.content) && + m.content.some(part => part.type === "text" && part.text === "hello"), + ); + expect(steerDelivered).toBe(true); + }); + + it("runs a queued follow-up as the opening turn and drains the queue", async () => { + const mock = createMockModel({ responses: [{ content: ["Answer"] }] }); + const agent = new Agent({ streamFn: mock.stream }); + + agent.followUp({ + role: "user", + content: [{ type: "text", text: "later" }], + timestamp: Date.now(), + }); + + await expect(agent.continue()).resolves.toBeUndefined(); + + expect(mock.calls.length).toBe(1); + expect(agent.hasQueuedMessages()).toBe(false); + expect(agent.state.messages.map(m => m.role)).toEqual(["user", "assistant"]); + }); + + it("still throws when the transcript is empty and nothing is queued", async () => { + const agent = new Agent(); + await expect(agent.continue()).rejects.toThrow("No messages to continue from"); + }); +}); diff --git a/packages/agent/test/remote-compaction.test.ts b/packages/agent/test/remote-compaction.test.ts index b1bb2f3b7..6458059c0 100644 --- a/packages/agent/test/remote-compaction.test.ts +++ b/packages/agent/test/remote-compaction.test.ts @@ -1256,18 +1256,89 @@ describe("compact() remote compaction failure handling", () => { const baseSettings = { ...DEFAULT_COMPACTION_SETTINGS, keepRecentTokens: 1 }; // Remote disabled → the V2 replay is unusable → re-expand the pre-V2 original. - const reexpanded = prepareCompaction(entries, { ...baseSettings, remoteEnabled: false }, [v2Model]); + const reexpanded = prepareCompaction(entries, { ...baseSettings, remoteEnabled: false }, v2Model); expect(reexpanded).toBeDefined(); const reexpandedText = JSON.stringify(reexpanded?.messagesToSummarize ?? []); expect(reexpandedText).toContain("ORIGINAL ALPHA port 4242"); // Remote + V2 still enabled, same provider → reuse the replay, don't re-summarize originals. - const reused = prepareCompaction(entries, { ...baseSettings, remoteStreamingV2Enabled: true }, [v2Model]); + const reused = prepareCompaction(entries, { ...baseSettings, remoteStreamingV2Enabled: true }, v2Model); expect(reused).toBeDefined(); const reusedText = JSON.stringify(reused?.messagesToSummarize ?? []); expect(reusedText).not.toContain("ORIGINAL ALPHA port 4242"); }); + test("re-expands a stranded remote compaction when the active model cannot replay it (#6343)", () => { + const ts = (n: number) => new Date(n).toISOString(); + const anthropicActive = buildModel({ + id: "claude-sonnet-4-5", + name: "Claude Sonnet 4.5", + api: "anthropic-messages", + provider: "anthropic", + baseUrl: "https://api.anthropic.com", + reasoning: true, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 200_000, + maxTokens: 64_000, + }); + const openaiSmol = makeOpenAiModel({ id: "gpt-5-mini", name: "GPT-5 mini" }); + // Prior OpenAI remote compaction: opaque placeholder summary, provider-native + // replay stored under preserveData tagged "openai". + const entries: SessionEntry[] = [ + { + type: "message", + id: "m1", + parentId: null, + timestamp: ts(1), + message: { role: "user", content: "ORIGINAL ALPHA port 4242", timestamp: 1 }, + }, + { + type: "compaction", + id: "c1", + parentId: "m1", + timestamp: ts(2), + summary: "Remote compaction preserved provider-native history for this session.", + firstKeptEntryId: "m1", + tokensBefore: 100_000, + preserveData: { + openaiRemoteCompaction: { + provider: "openai", + replacementHistory: [{ type: "message", role: "user", content: "opaque native replay" }], + compactionItem: { type: "compaction", encrypted_content: "enc_v1" }, + }, + }, + }, + { + type: "message", + id: "m2", + parentId: "c1", + timestamp: ts(3), + message: { role: "user", content: "second turn", timestamp: 3 }, + }, + { + type: "message", + id: "m3", + parentId: "m2", + timestamp: ts(4), + message: { role: "user", content: "third turn", timestamp: 4 }, + }, + ]; + const settings = { ...DEFAULT_COMPACTION_SETTINGS, keepRecentTokens: 1 }; + // Reuse is judged by the ACTIVE model, not the candidate set. The active + // anthropic model's encoder drops the OpenAI replay payload, so the stranded + // originals are re-expanded into a portable local summary — even though the + // OpenAI smol role could still replay the blob. + const foreignActive = prepareCompaction(entries, settings, anthropicActive); + expect(foreignActive).toBeDefined(); + expect(JSON.stringify(foreignActive?.messagesToSummarize ?? [])).toContain("ORIGINAL ALPHA port 4242"); + // The same-provider OpenAI model can replay the payload, so the boundary is + // kept and the originals are not re-summarized. + const sameProviderActive = prepareCompaction(entries, settings, openaiSmol); + expect(sameProviderActive).toBeDefined(); + expect(JSON.stringify(sameProviderActive?.messagesToSummarize ?? [])).not.toContain("ORIGINAL ALPHA port 4242"); + }); + test("user abort during the remote compact request rejects without falling back to local summarization", async () => { // Contract: Esc is a cancellation, not a remote failure. Before the fix // the AbortError was swallowed by the fallback catch and compaction kept diff --git a/packages/agent/test/serialize-conversation.test.ts b/packages/agent/test/serialize-conversation.test.ts index 0c7b119e4..04928e7b6 100644 --- a/packages/agent/test/serialize-conversation.test.ts +++ b/packages/agent/test/serialize-conversation.test.ts @@ -130,4 +130,39 @@ describe("serializeConversation — useless pairs", () => { expect(out).toBe(""); }); + + test("strips assistant reasoning from Anthropic-dialect summary input but keeps text and tool calls", () => { + const reasoning = "PRIVATE chain of thought that must not be replayed to Claude"; + const out = serializeConversation( + [ + assistantMessage([ + { type: "thinking", thinking: reasoning }, + { type: "text", text: "The visible answer." }, + { type: "toolCall", id: "c1", name: "search", arguments: { pattern: "delta" } }, + ]), + ], + "anthropic", + ); + + expect(out).not.toContain(reasoning); + expect(out).not.toContain(""); + expect(out).toContain("The visible answer."); + expect(out).toContain(""); + }); + + test("keeps assistant reasoning for non-Anthropic dialects", () => { + const reasoning = "reasoning kept for the XML transcript"; + const out = serializeConversation( + [ + assistantMessage([ + { type: "thinking", thinking: reasoning }, + { type: "text", text: "answer" }, + ]), + ], + "xml", + ); + + expect(out).toContain(reasoning); + expect(out).toContain(""); + }); }); diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 3e649355e..6756e6200 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -9,10 +9,12 @@ ### Fixed +- Fixed Bedrock cache checkpoints to use resolved model compatibility: unsupported 1-hour retention now falls back to the provider-default 5-minute cache, bundled Nova Lite, Micro, Pro, and Premier requests—and Nova Premier's documented in-region model ID—emit AWS-recommended explicit checkpoints for cache savings, and forced opaque profiles remain conservative. - Fixed outbound credential-pattern redaction (`[github_token_redacted]` & co.) running unconditionally: it is now opt-in via `configureCredentialRedaction` and disabled by default, so credential-shaped strings the user deliberately pastes reach the provider unmodified unless the host enables redaction. - Added interactive Meta Model API key login and `MODEL_API_KEY` / `META_API_KEY` environment authentication ([#4941](https://github.com/can1357/oh-my-pi/issues/4941)). - Fixed SuperGrok (`xai-oauth`) `/usage` showing "no usage data" for unified-billing accounts: when `?format=credits` lacks `creditUsagePercent` (or marks `isUnifiedBillingUser`), fall back to / merge the default monthly `monthlyLimit`/`used` payload. - Fixed sessions wedging onto their fallback model with `400 Invalid \`signature\` in \`thinking\` block` after switching to an Anthropic signing endpoint while the latest assistant turn came from a different Anthropic-compatible provider (e.g. Kimi k3). The cross-model thinking-signature strip skipped the latest surviving assistant turn entirely, replaying the foreign signature verbatim on every attempt; the latest turn now strips signatures whose issuing provider differs from the target (same-provider switches keep their byte-for-byte latest turn), and foreign `redacted_thinking` siblings are dropped alongside instead of riding the wire unverifiable. +- Fixed OAuth callback servers aborting login when an invalid callback arrives before the legitimate browser redirect, and restricted `localhost` callback listeners to the IPv4 loopback interface ([#4106](https://github.com/can1357/oh-my-pi/issues/4106)). ## [17.0.9] - 2026-07-23 diff --git a/packages/ai/src/providers/amazon-bedrock.ts b/packages/ai/src/providers/amazon-bedrock.ts index 95bf05ce6..f5e082ffb 100644 --- a/packages/ai/src/providers/amazon-bedrock.ts +++ b/packages/ai/src/providers/amazon-bedrock.ts @@ -171,6 +171,11 @@ type Block = (TextContent | ThinkingContent | ToolCall) & { interface CachePoint { cachePoint: { type: "default"; ttl?: "5m" | "1h" }; } + +interface BedrockPromptCachePolicy { + emitCheckpoints: boolean; + ttl?: "1h"; +} interface TextBlockWire { text: string; } @@ -310,7 +315,8 @@ export const streamBedrock: StreamFunction<"bedrock-converse-stream"> = ( try { const cacheRetention = resolveCacheRetention(options.cacheRetention); - const convertedMessages = convertMessages(context, model, cacheRetention); + const promptCachePolicy = resolvePromptCachePolicy(model, cacheRetention); + const convertedMessages = convertMessages(context, model, promptCachePolicy); const toolPlan = planToolConfig(context.tools, options.toolChoice, convertedMessages); const toolConfig = toolPlan.toolConfig; const sentinelInjected = toolPlan.sentinelInjected; @@ -325,7 +331,7 @@ export const streamBedrock: StreamFunction<"bedrock-converse-stream"> = ( const commandInput: ConverseStreamRequest = { messages: convertedMessages, - system: buildSystemPrompt(context.systemPrompt, model, cacheRetention), + system: buildSystemPrompt(context.systemPrompt, promptCachePolicy), inferenceConfig: { maxTokens: options.maxTokens, temperature: options.temperature, @@ -693,29 +699,32 @@ function handleContentBlockStop( } /** - * Check if the model supports prompt caching. - * Supported: Claude 3.5 Haiku, Claude 3.7 Sonnet, Claude 4.x+ models, Haiku 4.5+ - * - * For base models and system-defined inference profiles the model ID / ARN - * contains the model name, so we can decide locally. - * - * For application inference profiles (whose ARNs don't contain the model name), - * set AWS_BEDROCK_FORCE_CACHE=1 to enable cache points. Amazon Nova models - * have automatic caching and don't need explicit cache points. + * Resolve Bedrock's explicit-cache request policy from the catalog's + * materialized provider contract. `AWS_BEDROCK_FORCE_CACHE` remains an escape + * hatch for opaque application inference profiles, but it cannot invent 1h + * retention that the model compat did not explicitly grant. */ -function supportsPromptCaching(model: Model<"bedrock-converse-stream">): boolean { - if (model.cost.cacheRead || model.cost.cacheWrite) return true; - const id = model.id.toLowerCase(); - // Claude 4.x models (opus-4, sonnet-4, haiku-4) - if (id.includes("claude") && (id.includes("-4-") || id.includes("-4."))) return true; - // Claude 3.5 Haiku, Claude 3.7 Sonnet (legacy naming) - if (id.includes("claude-3-7-sonnet") || id.includes("claude-3-5-haiku")) return true; - // Claude Haiku 4.5+ (new naming) - if (id.includes("claude-haiku")) return true; - // Application inference profiles don't contain the model name in the ARN. - // Allow users to force cache points via environment variable. - if (typeof process !== "undefined" && $flag("AWS_BEDROCK_FORCE_CACHE")) return true; - return false; +function resolvePromptCachePolicy( + model: Model<"bedrock-converse-stream">, + cacheRetention: CacheRetention, +): BedrockPromptCachePolicy { + if (cacheRetention === "none" || model.compat.promptCacheMode === "automatic") { + return { emitCheckpoints: false }; + } + + const forced = $flag("AWS_BEDROCK_FORCE_CACHE"); + if (model.compat.promptCacheMode !== "explicit" && !forced) { + return { emitCheckpoints: false }; + } + + return { + emitCheckpoints: true, + ...(cacheRetention === "long" && model.compat.supportsLongPromptCacheRetention ? { ttl: "1h" } : {}), + }; +} + +function createCachePoint(policy: BedrockPromptCachePolicy): CachePoint { + return { cachePoint: { type: "default", ...(policy.ttl ? { ttl: policy.ttl } : {}) } }; } /** @@ -731,19 +740,15 @@ function supportsThinkingSignature(model: Model<"bedrock-converse-stream">): boo function buildSystemPrompt( systemPrompt: readonly string[] | undefined, - model: Model<"bedrock-converse-stream">, - cacheRetention: CacheRetention, + promptCachePolicy: BedrockPromptCachePolicy, ): SystemContent[] | undefined { const prompts = systemPrompt?.map(prompt => prompt.toWellFormed()).filter(prompt => prompt.length > 0) ?? []; if (prompts.length === 0) return undefined; const blocks: SystemContent[] = prompts.map(prompt => ({ text: prompt })); - // Add cache point for supported Claude models - if (cacheRetention !== "none" && supportsPromptCaching(model)) { - blocks.push({ - cachePoint: { type: "default", ...(cacheRetention === "long" ? { ttl: "1h" } : {}) }, - }); + if (promptCachePolicy.emitCheckpoints) { + blocks.push(createCachePoint(promptCachePolicy)); } return blocks; @@ -752,7 +757,7 @@ function buildSystemPrompt( function convertMessages( context: Context, model: Model<"bedrock-converse-stream">, - cacheRetention: CacheRetention, + promptCachePolicy: BedrockPromptCachePolicy, ): WireMessage[] { const result: WireMessage[] = []; const transformedMessages = transformMessages(context.messages, model, normalizeToolCallId); @@ -883,13 +888,11 @@ function convertMessages( } } - // Add cache point to the last user message for supported Claude models - if (cacheRetention !== "none" && supportsPromptCaching(model) && result.length > 0) { + // Preserve the existing second checkpoint after the final user message. + if (promptCachePolicy.emitCheckpoints && result.length > 0) { const lastMessage = result[result.length - 1]; if (lastMessage.role === "user" && lastMessage.content) { - (lastMessage.content as UserContent[]).push({ - cachePoint: { type: "default", ...(cacheRetention === "long" ? { ttl: "1h" } : {}) }, - }); + (lastMessage.content as UserContent[]).push(createCachePoint(promptCachePolicy)); } } diff --git a/packages/ai/src/registry/oauth/callback-server.ts b/packages/ai/src/registry/oauth/callback-server.ts index 4342d2472..5b044b769 100644 --- a/packages/ai/src/registry/oauth/callback-server.ts +++ b/packages/ai/src/registry/oauth/callback-server.ts @@ -64,7 +64,7 @@ export abstract class OAuthCallbackFlow { allowPortFallback: boolean; #manualInputOnly: boolean; #callbackResolve?: (result: CallbackResult) => void; - #callbackReject?: (error: string) => void; + #callbackReject?: (error: Error) => void; /** * Authorization URL the `/launch` route currently redirects to. Set by * {@link login} after {@link generateAuthUrl} and before {@link OAuthController.onAuth} @@ -280,9 +280,9 @@ export abstract class OAuthCallbackFlow { * Create HTTP server for OAuth callback. */ #createServer(port: number, expectedState: string): Bun.Server { - const hostname = this.callbackHostname === DEFAULT_HOSTNAME ? undefined : this.callbackHostname; + const hostname = this.callbackHostname === DEFAULT_HOSTNAME ? "127.0.0.1" : this.callbackHostname; return Bun.serve({ - ...(hostname === undefined ? {} : { hostname }), + hostname, port, reusePort: false, fetch: req => this.#handleCallback(req, expectedState), @@ -334,16 +334,23 @@ export abstract class OAuthCallbackFlow { resultState = { ok: true, code, state }; } - // Signal to waitForCallback - capture refs before they could be cleared - const resolve = this.#callbackResolve; - const reject = this.#callbackReject; - queueMicrotask(() => { - if (resultState.ok) { + if (resultState.ok) { + const resolve = this.#callbackResolve; + queueMicrotask(() => { resolve?.({ code: resultState.code, state: resultState.state }); - } else { - reject?.(resultState.error ?? "Unknown error"); - } - }); + }); + } else if (error && (!expectedState || state === expectedState)) { + // The redirect carries our state nonce, so it came from the genuine + // authorization flow (e.g. the user denied the consent screen). + // Surface the denial now instead of leaving the login waiting for + // the 5-minute timeout. Errors WITHOUT the expected state stay + // ignored — any local process can forge those (#4106). + const reject = this.#callbackReject; + const message = resultState.error ?? `Authorization failed: ${errorDescription}`; + queueMicrotask(() => { + reject?.(new AIError.OAuthError(message, { kind: "device-auth" })); + }); + } return new Response( (templateHtml as unknown as string).replaceAll("__OAUTH_STATE__", JSON.stringify(resultState)), diff --git a/packages/ai/test/bedrock-prompt-cache.test.ts b/packages/ai/test/bedrock-prompt-cache.test.ts new file mode 100644 index 000000000..408736d1b --- /dev/null +++ b/packages/ai/test/bedrock-prompt-cache.test.ts @@ -0,0 +1,142 @@ +import { describe, expect, test } from "bun:test"; +import { streamBedrock } from "@oh-my-pi/pi-ai/providers/amazon-bedrock"; +import type { Context, Model } from "@oh-my-pi/pi-ai/types"; +import { buildModel } from "@oh-my-pi/pi-catalog/build"; +import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; +import { withEnv } from "./helpers"; + +interface CachePoint { + cachePoint: { type: "default"; ttl?: "1h" }; +} +interface Payload { + system?: Array<{ text: string } | CachePoint>; + messages: Array<{ role: string; content: Array<{ text: string } | CachePoint> }>; + inferenceConfig: { maxTokens?: number; temperature?: number; topP?: number }; + toolConfig?: unknown; + additionalModelRequestFields?: Record; +} + +const context: Context = { + systemPrompt: ["Use concise answers."], + messages: [{ role: "user", content: "What is the answer?", timestamp: 0 }], +}; + +function model(id: string): Model<"bedrock-converse-stream"> { + return buildModel({ + id, + name: id, + api: "bedrock-converse-stream", + provider: "amazon-bedrock", + baseUrl: "https://bedrock-runtime.us-east-1.amazonaws.com", + reasoning: false, + input: ["text"], + cost: { input: 5, output: 25, cacheRead: 0.5, cacheWrite: 6.25 }, + contextWindow: 1_000_000, + maxTokens: 128_000, + }); +} + +function abortedSignal(): AbortSignal { + const controller = new AbortController(); + controller.abort(); + return controller.signal; +} + +function capturePayload( + bedrockModel: Model<"bedrock-converse-stream">, + cacheRetention: "none" | "short" | "long", +): Promise { + const { promise, resolve } = Promise.withResolvers(); + void streamBedrock(bedrockModel, context, { + signal: abortedSignal(), + cacheRetention, + onPayload: payload => { + resolve(payload as Payload); + }, + }); + return promise; +} + +function checkpoints(payload: Payload): CachePoint[] { + return [...(payload.system ?? []), ...payload.messages.flatMap(message => message.content)].filter( + (block): block is CachePoint => "cachePoint" in block, + ); +} + +describe("Bedrock prompt cache checkpoints", () => { + test("downgrades unsupported long retention to default 5m at the existing two checkpoint locations", async () => { + const payload = await capturePayload(model("anthropic.claude-opus-4-6-v1"), "long"); + expect(payload.system).toEqual([{ text: "Use concise answers." }, { cachePoint: { type: "default" } }]); + expect(payload.messages).toEqual([ + { role: "user", content: [{ text: "What is the answer?" }, { cachePoint: { type: "default" } }] }, + ]); + expect(checkpoints(payload)).toHaveLength(2); + }); + + test("uses 1h only for catalog-confirmed long-retention models", async () => { + const payload = await capturePayload(model("anthropic.claude-haiku-4-5-20251001-v1:0"), "long"); + expect(checkpoints(payload)).toEqual([ + { cachePoint: { type: "default", ttl: "1h" } }, + { cachePoint: { type: "default", ttl: "1h" } }, + ]); + }); + + test("emits checkpoints for the default bundled Opus 4.8 inference profile", async () => { + const payload = await capturePayload(model("us.anthropic.claude-opus-4-8"), "long"); + expect(checkpoints(payload)).toEqual([ + { cachePoint: { type: "default", ttl: "1h" } }, + { cachePoint: { type: "default", ttl: "1h" } }, + ]); + }); + + test("uses Bedrock's default 5m TTL for short retention", async () => { + const payload = await capturePayload(model("anthropic.claude-haiku-4-5-20251001-v1:0"), "short"); + expect(checkpoints(payload)).toEqual([{ cachePoint: { type: "default" } }, { cachePoint: { type: "default" } }]); + }); + + test("emits default-5m checkpoints for every bundled Nova cache-capable payload", async () => { + for (const id of [ + "us.amazon.nova-lite-v1:0", + "us.amazon.nova-micro-v1:0", + "us.amazon.nova-pro-v1:0", + "us.amazon.nova-premier-v1:0", + ] as const) { + const nova = getBundledModel<"bedrock-converse-stream">("amazon-bedrock", id); + expect(nova).toBeDefined(); + const payload = await capturePayload(nova!, "long"); + expect(payload).toEqual({ + system: [{ text: "Use concise answers." }, { cachePoint: { type: "default" } }], + messages: [ + { + role: "user", + content: [{ text: "What is the answer?" }, { cachePoint: { type: "default" } }], + }, + ], + inferenceConfig: { maxTokens: undefined, temperature: undefined, topP: undefined }, + toolConfig: undefined, + additionalModelRequestFields: undefined, + }); + } + }); + + test("emits default-5m system and message checkpoints for Nova Premier's in-region ID", async () => { + const payload = await capturePayload(model("amazon.nova-premier-v1:0"), "long"); + expect(payload.system).toEqual([{ text: "Use concise answers." }, { cachePoint: { type: "default" } }]); + expect(payload.messages).toEqual([ + { role: "user", content: [{ text: "What is the answer?" }, { cachePoint: { type: "default" } }] }, + ]); + }); + + test("forces opaque profiles to default checkpoints without granting 1h retention", async () => { + await withEnv({ AWS_BEDROCK_FORCE_CACHE: "1" }, async () => { + const payload = await capturePayload( + model("arn:aws:bedrock:us-east-1:1234567890:application-inference-profile/opaque-profile"), + "long", + ); + expect(checkpoints(payload)).toEqual([ + { cachePoint: { type: "default" } }, + { cachePoint: { type: "default" } }, + ]); + }); + }); +}); diff --git a/packages/ai/test/callback-server-port-fallback.test.ts b/packages/ai/test/callback-server-port-fallback.test.ts index 65bdd5282..bfa3cd8cd 100644 --- a/packages/ai/test/callback-server-port-fallback.test.ts +++ b/packages/ai/test/callback-server-port-fallback.test.ts @@ -26,11 +26,13 @@ class TestCallbackFlow extends OAuthCallbackFlow { /** * Bind a real loopback port so the next `Bun.serve({ port })` against the - * same port fails with EADDRINUSE. Returns the bound port plus a `release` - * callback for teardown. + * same port fails with EADDRINUSE. Occupies `127.0.0.1` explicitly — the + * interface callback flows bind for `localhost` — because macOS lets a + * specific-address bind coexist with a wildcard one. Returns the bound port + * plus a `release` callback for teardown. */ function occupyLoopbackPort(): { port: number; release: () => void } { - const server = Bun.serve({ port: 0, fetch: () => new Response("blocker") }); + const server = Bun.serve({ hostname: "127.0.0.1", port: 0, fetch: () => new Response("blocker") }); const port = server.port; if (typeof port !== "number") { server.stop(true); diff --git a/packages/ai/test/callback-server-security.test.ts b/packages/ai/test/callback-server-security.test.ts new file mode 100644 index 000000000..0b73615b1 --- /dev/null +++ b/packages/ai/test/callback-server-security.test.ts @@ -0,0 +1,105 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { OAuthCallbackFlow } from "@oh-my-pi/pi-ai/registry/oauth/callback-server"; +import type { OAuthAuthInfo, OAuthCredentials } from "@oh-my-pi/pi-ai/registry/oauth/types"; + +class CallbackProbeFlow extends OAuthCallbackFlow { + async generateAuthUrl(state: string, redirectUri: string): Promise<{ url: string }> { + const url = new URL("https://provider.example.com/authorize"); + url.searchParams.set("redirect_uri", redirectUri); + url.searchParams.set("state", state); + return { url: url.toString() }; + } + + async exchangeToken(code: string): Promise { + return { access: code, refresh: "refresh", expires: Date.now() + 60_000 }; + } +} + +async function startFlow(): Promise<{ + info: OAuthAuthInfo; + abort: AbortController; + login: Promise; +}> { + const abort = new AbortController(); + const authFired = Promise.withResolvers(); + const flow = new CallbackProbeFlow( + { + onAuth: info => authFired.resolve(info), + signal: abort.signal, + }, + { preferredPort: 0 }, + ); + const login = flow.login(); + void login.catch(() => undefined); + const info = await authFired.promise; + return { info, abort, login }; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("OAuthCallbackFlow callback security", () => { + it("keeps waiting after invalid callback requests and accepts the legitimate callback", async () => { + const { info, abort, login } = await startFlow(); + const authUrl = new URL(info.url); + const redirectUri = authUrl.searchParams.get("redirect_uri"); + const state = authUrl.searchParams.get("state"); + if (!redirectUri || !state) throw new Error("OAuth test flow did not advertise its callback parameters"); + + try { + const invalidCallbacks = [ + `${redirectUri}?error=access_denied&error_description=Denied`, + redirectUri, + `${redirectUri}?code=attacker-code&state=wrong-state`, + ]; + for (const callback of invalidCallbacks) { + const response = await fetch(callback); + expect(response.status).toBe(500); + } + + const response = await fetch(`${redirectUri}?code=legitimate-code&state=${encodeURIComponent(state)}`); + expect(response.status).toBe(200); + expect((await login).access).toBe("legitimate-code"); + } finally { + abort.abort("test cleanup"); + await login.catch(() => undefined); + } + }); + + it("surfaces provider denials that carry the expected state instead of waiting for the timeout", async () => { + const { info, abort, login } = await startFlow(); + const authUrl = new URL(info.url); + const redirectUri = authUrl.searchParams.get("redirect_uri"); + const state = authUrl.searchParams.get("state"); + if (!redirectUri || !state) throw new Error("OAuth test flow did not advertise its callback parameters"); + + try { + const response = await fetch( + `${redirectUri}?error=access_denied&error_description=User%20denied&state=${encodeURIComponent(state)}`, + ); + expect(response.status).toBe(500); + await expect(login).rejects.toThrow("Authorization failed: User denied"); + } finally { + abort.abort("test cleanup"); + await login.catch(() => undefined); + } + }); + + it("binds localhost callback URLs to the IPv4 loopback interface", async () => { + const serve = Bun.serve; + let hostname: string | undefined; + vi.spyOn(Bun, "serve").mockImplementation(options => { + hostname = options.hostname; + return serve(options); + }); + + const { abort, login } = await startFlow(); + try { + expect(hostname).toBe("127.0.0.1"); + } finally { + abort.abort("test cleanup"); + await login.catch(() => undefined); + } + }); +}); diff --git a/packages/catalog/CHANGELOG.md b/packages/catalog/CHANGELOG.md index 8acb78687..f6fa38018 100644 --- a/packages/catalog/CHANGELOG.md +++ b/packages/catalog/CHANGELOG.md @@ -4,6 +4,7 @@ ### Added +- Added resolved Bedrock Converse prompt-cache compatibility limits, including explicit 5-minute checkpoint support for bundled Nova Lite, Micro, Pro, and Premier models plus Nova Premier's documented in-region model ID, and model-specific 1-hour Claude retention. - Added the native Meta Model API provider and Muse Spark 1.1 with Responses API reasoning replay, image input, and the full supported reasoning-effort ladder ([#4941](https://github.com/can1357/oh-my-pi/issues/4941)). ## [17.0.9] - 2026-07-23 diff --git a/packages/catalog/scripts/generated-policies.ts b/packages/catalog/scripts/generated-policies.ts index 7ebf256ec..cf8bce014 100644 --- a/packages/catalog/scripts/generated-policies.ts +++ b/packages/catalog/scripts/generated-policies.ts @@ -364,9 +364,9 @@ function applyOpenAICatalogPolicy(model: ModelSpec, parsedModel: OpenAIMode } // GPT-5.6 luna/sol/terra on the Codex transport: OpenAI's Codex model // registry declares context_window = max_context_window = 372000, but Codex - // discovery under-reports it — omitting the field for some accounts and - // actively returning 272000 for others (#5705, #6259). Pin the true 372K - // input window on the bundled catalog; discovery enforces the same floor. + // discovery omits `context_window` for these SKUs and falls back to + // DEFAULT_CONTEXT_WINDOW (272000, src/discovery/codex.ts), which regressed + // the bundled hard capacity (#5705). Pin the true 372K input window. if (model.api === "openai-codex-responses" && semverEqual(parsedModel.version, "5.6")) { model.contextWindow = 372000; } diff --git a/packages/catalog/src/build.ts b/packages/catalog/src/build.ts index 144e1682a..0bc2c43bb 100644 --- a/packages/catalog/src/build.ts +++ b/packages/catalog/src/build.ts @@ -9,7 +9,9 @@ * Request handlers read fields — they never detect, parse ids, or allocate * compat per request. */ + import { buildAnthropicCompat } from "./compat/anthropic"; +import { buildBedrockCompat } from "./compat/bedrock"; import { buildDevinCompat } from "./compat/devin"; import { buildOpenAICompat, buildOpenAIResponsesCompat, buildOpenRouterCompat } from "./compat/openai"; import { resolveModelThinking } from "./model-thinking"; @@ -39,6 +41,8 @@ export function buildCompat(spec: ModelSpec): CompatOf { return buildOpenAIResponsesCompat(spec as ModelSpec<"openai-responses">); case "anthropic-messages": return buildAnthropicCompat(spec as ModelSpec<"anthropic-messages">); + case "bedrock-converse-stream": + return buildBedrockCompat(spec as ModelSpec<"bedrock-converse-stream">); case "devin-agent": return buildDevinCompat(spec as ModelSpec<"devin-agent">); default: diff --git a/packages/catalog/src/compat/bedrock.ts b/packages/catalog/src/compat/bedrock.ts new file mode 100644 index 000000000..bee4f14d7 --- /dev/null +++ b/packages/catalog/src/compat/bedrock.ts @@ -0,0 +1,109 @@ +import type { ModelSpec, ResolvedBedrockCompat } from "../types"; +import { applyCompatOverrides } from "./apply"; + +const NO_EXPLICIT_CHECKPOINTS: ResolvedBedrockCompat = { + promptCacheMode: "none", + supportsLongPromptCacheRetention: false, + promptCacheMinimumTokens: 0, + promptCacheMaximumCheckpoints: 0, +}; +const EXPLICIT_CHECKPOINTS_1024_5M: ResolvedBedrockCompat = { + promptCacheMode: "explicit", + supportsLongPromptCacheRetention: false, + promptCacheMinimumTokens: 1024, + promptCacheMaximumCheckpoints: 4, +}; + +const EXPLICIT_CHECKPOINTS_1024_1H: ResolvedBedrockCompat = { + promptCacheMode: "explicit", + supportsLongPromptCacheRetention: true, + promptCacheMinimumTokens: 1024, + promptCacheMaximumCheckpoints: 4, +}; + +const EXPLICIT_CHECKPOINTS_2048_5M: ResolvedBedrockCompat = { + promptCacheMode: "explicit", + supportsLongPromptCacheRetention: false, + promptCacheMinimumTokens: 2048, + promptCacheMaximumCheckpoints: 4, +}; + +const EXPLICIT_CHECKPOINTS_4096_5M: ResolvedBedrockCompat = { + promptCacheMode: "explicit", + supportsLongPromptCacheRetention: false, + promptCacheMinimumTokens: 4096, + promptCacheMaximumCheckpoints: 4, +}; + +const EXPLICIT_CHECKPOINTS_4096_1H: ResolvedBedrockCompat = { + promptCacheMode: "explicit", + supportsLongPromptCacheRetention: true, + promptCacheMinimumTokens: 4096, + promptCacheMaximumCheckpoints: 4, +}; + +/** + * Explicit Nova cache points complement Bedrock's automatic prefix caching: + * AWS recommends them for consistent cache hits and input-cost savings. Keep + * these exact documented model and inference-profile IDs conservative rather + * than treating arbitrary Nova-like application profiles as checkpoint-capable. + */ +function detectedBedrockCompat(modelId: string): ResolvedBedrockCompat { + const id = modelId.toLowerCase(); + + if ( + id === "amazon.nova-lite-v1:0" || + id === "us.amazon.nova-lite-v1:0" || + id === "amazon.nova-micro-v1:0" || + id === "us.amazon.nova-micro-v1:0" || + id === "amazon.nova-pro-v1:0" || + id === "us.amazon.nova-pro-v1:0" || + id === "amazon.nova-premier-v1:0" || + id === "us.amazon.nova-premier-v1:0" + ) { + return EXPLICIT_CHECKPOINTS_1024_5M; + } + + // https://docs.aws.amazon.com/bedrock/latest/userguide/prompt-caching.html + // This list is deliberately sourced from AWS model cards, not cache pricing: + // https://docs.aws.amazon.com/bedrock/latest/userguide/model-cards.html + if ( + id.includes("anthropic.claude-opus-4-5") || + id.includes("anthropic.claude-sonnet-4-5") || + id.includes("anthropic.claude-haiku-4-5") || + id.includes("anthropic.claude-opus-4-7") || + id.includes("anthropic.claude-opus-4-8") || + id.includes("anthropic.claude-sonnet-5") + ) { + return EXPLICIT_CHECKPOINTS_4096_1H; + } + if (id.includes("anthropic.claude-opus-4-6")) { + return EXPLICIT_CHECKPOINTS_4096_5M; + } + if (id.includes("anthropic.claude-3-5-haiku")) { + return EXPLICIT_CHECKPOINTS_2048_5M; + } + if (id.includes("anthropic.claude-fable-5")) { + return EXPLICIT_CHECKPOINTS_1024_1H; + } + + if ( + id.includes("anthropic.claude-opus-4-1") || + id.includes("anthropic.claude-opus-4-20250514") || + id.includes("anthropic.claude-sonnet-4-20250514") || + id.includes("anthropic.claude-sonnet-4-6") || + id.includes("anthropic.claude-3-7-sonnet") || + id.includes("anthropic.claude-3-5-sonnet-20241022-v2") + ) { + return EXPLICIT_CHECKPOINTS_1024_5M; + } + + return NO_EXPLICIT_CHECKPOINTS; +} + +/** Resolve Bedrock Converse prompt-cache capabilities once per model. */ +export function buildBedrockCompat(spec: ModelSpec<"bedrock-converse-stream">): ResolvedBedrockCompat { + const compat = { ...detectedBedrockCompat(spec.id) }; + applyCompatOverrides(compat, spec.compat); + return compat; +} diff --git a/packages/catalog/src/discovery/codex.ts b/packages/catalog/src/discovery/codex.ts index 4f0625fc5..f87dfc62f 100644 --- a/packages/catalog/src/discovery/codex.ts +++ b/packages/catalog/src/discovery/codex.ts @@ -8,11 +8,11 @@ const DEFAULT_MODEL_LIST_PATHS = ["/codex/models", "/models"] as const; const DEFAULT_CONTEXT_WINDOW = 272_000; const DEFAULT_MAX_TOKENS = 128_000; /** - * GPT-5.6 luna/sol/terra hard context capacity. OpenAI's Codex model registry - * declares context_window = max_context_window = 372000 (#5705), but Codex - * discovery under-reports it — omitting the field for some accounts and - * actively returning 272000 for others (#6259). Applied as a floor for these - * SKUs so the reported/absent value never regresses the real window. + * GPT-5.6 luna/sol/terra hard context capacity. Codex discovery omits + * `context_window` for these SKUs, so the generic {@link DEFAULT_CONTEXT_WINDOW} + * (272000) would understate the real window — OpenAI's Codex model registry + * declares context_window = max_context_window = 372000 (#5705). Used as the + * fallback only when upstream reports no value. */ const GPT_5_6_CONTEXT_WINDOW = 372_000; const CODEX_REMOTE_COMPACTION = { @@ -223,18 +223,14 @@ function normalizeCodexModelEntry(entry: unknown, baseUrl: string): NormalizedCo } const name = toNonEmptyString(payload.display_name) ?? slug; - // GPT-5.6 luna/sol/terra have a 372000 hard window, but Codex discovery - // under-reports it: for some accounts the field is omitted, for others it is - // actively returned as 272000 (#6259). Treat GPT_5_6_CONTEXT_WINDOW as a - // floor for these SKUs so neither the omission nor the active under-report - // regresses the real capacity; other models honor the reported value with - // the generic 272000 fallback. + // Codex discovery omits `context_window` for GPT-5.6 luna/sol/terra; the + // generic 272000 fallback understates their real 372000 window (#5705). const parsed = parseKnownModel(slug); - const isGpt56 = parsed.family === "openai" && semverEqual(parsed.version, "5.6"); - const reportedContextWindow = toPositiveInt(payload.context_window); - const contextWindow = isGpt56 - ? Math.max(GPT_5_6_CONTEXT_WINDOW, reportedContextWindow ?? 0) - : (reportedContextWindow ?? DEFAULT_CONTEXT_WINDOW); + const fallbackContextWindow = + parsed.family === "openai" && semverEqual(parsed.version, "5.6") + ? GPT_5_6_CONTEXT_WINDOW + : DEFAULT_CONTEXT_WINDOW; + const contextWindow = toPositiveInt(payload.context_window) ?? fallbackContextWindow; const maxTokens = Math.min(DEFAULT_MAX_TOKENS, contextWindow); const reasoning = supportsReasoning(payload.default_reasoning_level, payload.supported_reasoning_levels); const input = normalizeInputModalities(payload.input_modalities); diff --git a/packages/catalog/src/types.ts b/packages/catalog/src/types.ts index 143fd4565..bcfbbf8c9 100644 --- a/packages/catalog/src/types.ts +++ b/packages/catalog/src/types.ts @@ -459,6 +459,29 @@ export interface AnthropicCompat { escapeBuiltinToolNames?: boolean; } +/** + * Compatibility settings for Bedrock Converse prompt caching. Cache pricing is + * deliberately not used to infer these request-shape capabilities. + */ +export interface BedrockCompat { + /** Whether this endpoint accepts no checkpoints, automatic caching, or explicit cachePoint blocks. */ + promptCacheMode?: "none" | "automatic" | "explicit"; + /** Whether explicit cachePoint blocks accept `ttl: "1h"`; omitted TTL means Bedrock's 5-minute default. */ + supportsLongPromptCacheRetention?: boolean; + /** Minimum prompt-prefix tokens required for an effective checkpoint. Zero means no explicit checkpoints. */ + promptCacheMinimumTokens?: number; + /** Maximum explicit cache checkpoints accepted in one request. Zero means no explicit checkpoints. */ + promptCacheMaximumCheckpoints?: number; +} + +/** Fully-resolved Bedrock Converse prompt-cache capabilities, materialized once by `buildModel`. */ +export interface ResolvedBedrockCompat { + promptCacheMode: NonNullable; + supportsLongPromptCacheRetention: boolean; + promptCacheMinimumTokens: number; + promptCacheMaximumCheckpoints: number; +} + /** * OpenRouter provider routing preferences. * Controls which upstream providers OpenRouter routes requests to. @@ -681,9 +704,11 @@ export type CompatConfigOf = TApi extends ? OpenAICompat : TApi extends "anthropic-messages" ? AnthropicCompat - : TApi extends "devin-agent" - ? DevinCompat - : undefined; + : TApi extends "bedrock-converse-stream" + ? BedrockCompat + : TApi extends "devin-agent" + ? DevinCompat + : undefined; /** Resolved compat for a given API: complete record, materialized once by `buildModel`. */ export type CompatOf = TApi extends "openrouter" @@ -694,9 +719,11 @@ export type CompatOf = TApi extends "openrouter" ? ResolvedOpenAIResponsesCompat : TApi extends "anthropic-messages" ? ResolvedAnthropicCompat - : TApi extends "devin-agent" - ? ResolvedDevinCompat - : undefined; + : TApi extends "bedrock-converse-stream" + ? ResolvedBedrockCompat + : TApi extends "devin-agent" + ? ResolvedDevinCompat + : undefined; /** Provider-native compaction endpoint configuration for one model. */ export interface RemoteCompactionConfig { diff --git a/packages/catalog/test/bedrock-prompt-cache.test.ts b/packages/catalog/test/bedrock-prompt-cache.test.ts new file mode 100644 index 000000000..d9e1b5942 --- /dev/null +++ b/packages/catalog/test/bedrock-prompt-cache.test.ts @@ -0,0 +1,152 @@ +import { describe, expect, test } from "bun:test"; +import { buildModel } from "@oh-my-pi/pi-catalog/build"; +import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; +import type { ModelSpec } from "@oh-my-pi/pi-catalog/types"; + +function bedrockSpec( + overrides: Partial> = {}, +): ModelSpec<"bedrock-converse-stream"> { + return { + id: "anthropic.claude-opus-4-6-v1", + name: "Claude Opus 4.6", + api: "bedrock-converse-stream", + provider: "amazon-bedrock", + baseUrl: "https://bedrock-runtime.us-east-1.amazonaws.com", + reasoning: true, + input: ["text"], + cost: { input: 5, output: 25, cacheRead: 0.5, cacheWrite: 6.25 }, + contextWindow: 1_000_000, + maxTokens: 128_000, + ...overrides, + }; +} + +describe("Bedrock prompt-cache compat", () => { + test("resolves the AWS-documented capability for every cache-priced bundled Claude family", () => { + const cases = [ + { + id: "anthropic.claude-3-5-haiku-20241022-v1:0", + minimumTokens: 2048, + supportsLongRetention: false, + }, + // Current AWS docs do not advertise Converse cache checkpoints for this + // legacy v1 model, so catalog cache pricing alone must not enable them. + { + id: "anthropic.claude-3-5-sonnet-20240620-v1:0", + minimumTokens: 0, + supportsLongRetention: false, + }, + { + id: "anthropic.claude-3-5-sonnet-20241022-v2:0", + minimumTokens: 1024, + supportsLongRetention: false, + }, + { + id: "anthropic.claude-3-7-sonnet-20250219-v1:0", + minimumTokens: 1024, + supportsLongRetention: false, + }, + { id: "anthropic.claude-fable-5", minimumTokens: 1024, supportsLongRetention: true }, + { + id: "anthropic.claude-haiku-4-5-20251001-v1:0", + minimumTokens: 4096, + supportsLongRetention: true, + }, + { + id: "anthropic.claude-opus-4-1-20250805-v1:0", + minimumTokens: 1024, + supportsLongRetention: false, + }, + { + id: "anthropic.claude-opus-4-20250514-v1:0", + minimumTokens: 1024, + supportsLongRetention: false, + }, + { + id: "anthropic.claude-opus-4-5-20251101-v1:0", + minimumTokens: 4096, + supportsLongRetention: true, + }, + { id: "anthropic.claude-opus-4-6-v1", minimumTokens: 4096, supportsLongRetention: false }, + { id: "global.anthropic.claude-opus-4-7", minimumTokens: 4096, supportsLongRetention: true }, + { id: "us.anthropic.claude-opus-4-8", minimumTokens: 4096, supportsLongRetention: true }, + { + id: "anthropic.claude-sonnet-4-20250514-v1:0", + minimumTokens: 1024, + supportsLongRetention: false, + }, + { + id: "anthropic.claude-sonnet-4-5-20250929-v1:0", + minimumTokens: 4096, + supportsLongRetention: true, + }, + { id: "anthropic.claude-sonnet-4-6", minimumTokens: 1024, supportsLongRetention: false }, + { id: "us.anthropic.claude-sonnet-5", minimumTokens: 4096, supportsLongRetention: true }, + ] as const; + + for (const { id, minimumTokens, supportsLongRetention } of cases) { + expect(buildModel(bedrockSpec({ id })).compat).toEqual({ + promptCacheMode: minimumTokens === 0 ? "none" : "explicit", + supportsLongPromptCacheRetention: supportsLongRetention, + promptCacheMinimumTokens: minimumTokens, + promptCacheMaximumCheckpoints: minimumTokens === 0 ? 0 : 4, + }); + } + }); + + test("models exact cache-capable Nova IDs for explicit 5m checkpoints", () => { + const expected = { + promptCacheMode: "explicit", + supportsLongPromptCacheRetention: false, + promptCacheMinimumTokens: 1024, + promptCacheMaximumCheckpoints: 4, + } as const; + + for (const id of [ + "us.amazon.nova-lite-v1:0", + "us.amazon.nova-micro-v1:0", + "us.amazon.nova-pro-v1:0", + "us.amazon.nova-premier-v1:0", + ] as const) { + expect(getBundledModel<"bedrock-converse-stream">("amazon-bedrock", id)?.compat).toEqual(expected); + } + + // AWS documents both the in-region model IDs and the US geo inference IDs. + for (const id of [ + "amazon.nova-lite-v1:0", + "amazon.nova-micro-v1:0", + "amazon.nova-pro-v1:0", + "amazon.nova-premier-v1:0", + "us.amazon.nova-premier-v1:0", + ] as const) { + expect(buildModel(bedrockSpec({ id })).compat).toEqual(expected); + } + }); + + test("keeps unknown routes conservative and honors sparse profile overrides", () => { + const opaqueProfileId = "arn:aws:bedrock:us-east-1:123:application-inference-profile/opaque"; + const unknown = buildModel(bedrockSpec({ id: opaqueProfileId })); + expect(unknown.compat.promptCacheMode).toBe("none"); + + for (const id of ["amazon.nova-premier-v1:1", "us.amazon.nova-unknown-v1:0"]) { + expect(buildModel(bedrockSpec({ id })).compat.promptCacheMode).toBe("none"); + } + const sparse = { + promptCacheMode: "explicit" as const, + promptCacheMinimumTokens: 1024, + promptCacheMaximumCheckpoints: 4, + }; + const configured = buildModel( + bedrockSpec({ id: "arn:aws:bedrock:us-east-1:123:application-inference-profile/opaque", compat: sparse }), + ); + expect(configured.compat).toEqual({ ...unknown.compat, ...sparse }); + expect(configured.compatConfig).toBe(sparse); + }); + + test("keeps bundled models memoized while materializing resolved compat", () => { + const first = getBundledModel<"bedrock-converse-stream">("amazon-bedrock", "anthropic.claude-opus-4-6-v1"); + const second = getBundledModel<"bedrock-converse-stream">("amazon-bedrock", "anthropic.claude-opus-4-6-v1"); + expect(first).toBe(second); + expect(first?.compat.promptCacheMode).toBe("explicit"); + }); +}); diff --git a/packages/catalog/test/codex-discovery.test.ts b/packages/catalog/test/codex-discovery.test.ts index 0c6a94791..042aeb978 100644 --- a/packages/catalog/test/codex-discovery.test.ts +++ b/packages/catalog/test/codex-discovery.test.ts @@ -141,7 +141,7 @@ describe("Codex model discovery", () => { expect(legacy?.contextWindow).toBe(272_000); }); - it("floors GPT-5.6 SKUs to 372K when upstream actively reports 272000 (#6259)", async () => { + it("honors context_window when upstream actively reports it for GPT-5.6 SKUs", async () => { const fetchFn: typeof fetch = Object.assign( async () => new Response( @@ -178,8 +178,7 @@ describe("Codex model discovery", () => { }); const sol = result?.models.find(model => model.id === "gpt-5.6-sol"); - expect(sol?.contextWindow).toBe(372_000); - // Non-5.6 SKUs still honor the reported value verbatim. + expect(sol?.contextWindow).toBe(272_000); const legacy = result?.models.find(model => model.id === "gpt-5.5"); expect(legacy?.contextWindow).toBe(272_000); }); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d41dfa877..19b7f601d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -17,6 +17,8 @@ - Made the statusline `git` segment jj-aware: in a Jujutsu repo it shows the nearest bookmark (falling back to the short change-id) instead of git's `detached` label or nothing, and working-copy change counts come from jj where there is no `.git` to read ([#3582](https://github.com/can1357/oh-my-pi/issues/3582)) - Added `block`/`unblock` todo operations and a `blocked` status for tasks waiting on external input; blocked tasks stay visible in the todo HUD and summary but are excluded from the incomplete-todo stop reminder, and an optional blocker note records what the task is waiting for. - Added a toggle-list editor in `/settings` for array-of-enum settings: `providers.webSearchOrder` and `providers.imageOrder` (ordered — Enter/Space toggles, ←/→ nudges, 1-9 splices the hovered provider into that position) and `providers.webSearchExclude` now appear under Providers → Services instead of being config-file only. +- Added `models.yml` Bedrock Converse prompt-cache capability overrides for bundled and opaque inference profiles. +- Documented Vibe mode (`/vibe`) in `docs/vibe-mode.md` and the `/fresh` provider-stream reset in the session-operations doc, and linked both from the README's new "Session controls" section ([#6440](https://github.com/can1357/oh-my-pi/issues/6440)). ### Changed @@ -30,9 +32,23 @@ ### Fixed - Fixed `omp auth-gateway serve` and `omp auth-gateway check` bypassing the process-scoped OAuth account pool configured by `OMP_AUTH_BROKER_ACCOUNT_POOL_FILE`. +- Fixed a first-use race in `ArtifactManager` where two concurrent `allocatePath`/`save` callers on a fresh instance both re-seeded `#nextId` across the directory-scan yield and allocated the same artifact id, silently overwriting the first artifact (same tool type) or making `artifact://` resolution ambiguous (different tool types). The initial scan is now memoized as a single in-flight promise so all concurrent callers share one initialization and receive distinct ids ([#4091](https://github.com/can1357/oh-my-pi/issues/4091)). +- Fixed blob reference resolution passing unvalidated `blob:sha256:` suffixes into `path.join`, allowing a crafted ref (e.g. `blob:sha256:../../../etc/passwd`) in a persisted/shared session to escape the blob directory and read arbitrary files into resolved image history; `parseBlobRef` now rejects any suffix that is not a canonical 64-char lowercase hex hash, gating every resolution path ([#4088](https://github.com/can1357/oh-my-pi/issues/4088)). +- Fixed discarded `Settings` instances keeping debounced save timers and chained background saves armed; discarding an instance now cancels its pending writes so they cannot race a successor's file locks. +- Fixed startup status messages (warnings, errors, extension/tool errors, status lines) keeping the dark-mode color after auto-theme detection later switched the active theme to light — e.g. `dark-catppuccin`/`light-catppuccin` warnings rendered in Mocha yellow on the Latte background. Transient status presenters now resolve their color lazily at render time so a theme swap re-shapes them ([#6337](https://github.com/can1357/oh-my-pi/issues/6337)). +- Fixed MCP OAuth endpoint discovery and Smithery browser-login polling hanging indefinitely against an endpoint that accepts the TCP connection but never responds. `discoverOAuthEndpoints`/`fetchResourceMetadataScopes` now bound every metadata/well-known/authorization-server fetch with a per-request `AbortSignal.timeout`, and `pollSmitheryCliAuthSession` bounds each poll so the loop reaches its 5-minute deadline instead of stalling ([#4103](https://github.com/can1357/oh-my-pi/issues/4103)). +- Fixed bash internal-URL expansion skipping unquoted `skill://` (and other supported schemes) inside a legacy backtick command substitution nested directly in double quotes (e.g. ``echo "`cat skill://valid-skill/SKILL.md`"``); `isInsideShellQuote` now treats `` ` `` as an expansion-context boundary like `$()`, including `$()`/backtick nesting in either order, while single-quoted and escaped-backtick text stay literal ([#5645](https://github.com/can1357/oh-my-pi/issues/5645)). +- Fixed `omp say` playing no audio for a short single-segment clip on hosts where the first streaming backend (the bundled ffmpeg built without pulse/alsa output) spawns then exits nonzero: the pipe write succeeds before that death and `player.end()` has already closed the input, so neither the broken-pipe replay nor the early-exit handler advanced to `paplay`/`aplay`. `StreamingAudioPlayer` now retains the utterance PCM and, when the streaming backend exits nonzero, replays it through per-file playback so short clips still reach the speakers ([#5875](https://github.com/can1357/oh-my-pi/issues/5875)). +- Fixed mid-session `memory.backend` changes leaving runtime state, tools, listeners, and prompt context on different backends; Mnemopi clear/enqueue now rehydrate listeners, and legacy `memories.enabled` no longer activates the local pipeline after migration ([#5638](https://github.com/can1357/oh-my-pi/issues/5638)). - Fixed `error.notify` raising a "Stopped with error" toast for provider failures while an auto-retry or async-delivery continuation was pending; the toast now waits for the true terminal settle. +- Fixed concurrent MCP config mutations losing updates and racing on a shared temp path: every `mcp.json` read-modify-write (add/update/remove server, disabled/force-enabled lists) is now serialized under a per-file lock, and each atomic write uses a unique temp file so overlapping writers no longer rename each other's `.tmp` out from under them (ENOENT or clobbered config) — reachable in-process via the fire-and-forget extensions-dashboard toggle and across processes on a shared `~/.omp/mcp.json` ([#4104](https://github.com/can1357/oh-my-pi/issues/4104)). +- Fixed transient provider stream stalls after tool calls failing to auto-retry even when every call already had a tool result, including synthetic `executed:false` results from OpenAI-completions stalls ([#6414](https://github.com/can1357/oh-my-pi/issues/6414)). - Fixed terminal `yield` results racing post-turn maintenance, which could trigger an unnecessary automatic handoff or compaction. +- Fixed PDF image reads returning stale extractions after same-path content replacement and racing concurrent cold reads by binding each cache generation to immutable source bytes and coalescing extraction with independent caller cancellation. ([#6368](https://github.com/can1357/oh-my-pi/issues/6368)) +- Fixed direct edit calls failing with `File not found` for relative paths that `read` resolved to a unique nested workspace file. ([#6359](https://github.com/can1357/oh-my-pi/issues/6359)) +- Fixed concurrent dead-kernel replacement in persistent Python sessions starting multiple generations and orphaning the losing process; callers now share one generation-scoped replacement, while resets and disposal invalidate and drain stale replacements ([#6367](https://github.com/can1357/oh-my-pi/issues/6367)). - Fixed credential-shaped tokens (GitHub/GitLab/OpenAI/Anthropic key patterns) being redacted from outbound provider requests even with `secrets.enabled` off; the pattern redaction now follows the `secrets.enabled` ("Hide Secrets") setting like the secret obfuscator. +- Fixed a cancelled `lsp reload` reporting `Restarted ` while killing the server with no replacement: `reloadServer` swallowed `ToolAbortError`/tool timeout in both fallback `catch` blocks and fell through to `proc.kill()`. Cancellation and timeout now propagate; the rust-analyzer fallback only triggers on genuine method-not-found; and a wedged-connection teardown removes the client by identity and awaits confirmed process exit before claiming a restart (surfacing a truthful teardown error if the process outlives the kill). ([#6369](https://github.com/can1357/oh-my-pi/issues/6369)) - Fixed Ctrl-clicking a wrapped OAuth authorization URL opening only the clicked row's truncated fragment by preserving the complete hyperlink target on every rendered row. - Fixed used-only absolute usage amounts across output surfaces: CLI now renders `$123.45 used`; the TUI shows a neutral, width-bounded amount instead of a pending/dotted/account-count placeholder; and ACP preserves `123.45 usd used` while suppressing duplicate window suffixes such as `— extra`. ([#5575](https://github.com/can1357/oh-my-pi/issues/5575)) - Fixed auto-compaction re-triggering the "Compaction freed too little context" warning on every resume when the branch's last entry was an over-threshold snapcompact archive: the dead-end rescue now rebuilds the trailing archive locally at a threshold-derived frame budget (superseding the stale frame payload) instead of pausing, since the elide/image tiers can never touch a compaction entry (#4786). @@ -77,6 +93,7 @@ ### Removed - Added dynamic multi-root workspace context (issue [#2569](https://github.com/can1357/oh-my-pi/issues/2569)): a session now carries an ordered list of workspace directories beyond `cwd`, managed live from the terminal. New `/add-dir `, `/remove-dir `, and `/dirs` slash commands let you add and remove folders mid-session; the repeatable `--add-dir ` CLI flag seeds them at launch, and the `workspace.additionalDirectories` setting persists defaults per project. Additional roots are persisted in the session header, survive reopen/fork/move, and are surfaced to the agent in the system prompt so it knows they exist and can `read`/`grep`/`glob` them by absolute path. Design aligns with the endorsed community implementation on `feature/session-workspace`. +- Fixed the `browser` tool's `open` action ignoring the requested `timeout` during browser acquisition (CDP discovery/connect ran to its own fixed wait), and orphaning a freshly-created browser on abort/timeout before tab publication. The requested timeout now bounds the whole open lifecycle, and one explicit registry lease is held across tab acquisition so rollback disposes exactly the failed open — a concurrent open of a different tab name on the same browser can no longer dispose the browser out from under it. ([#6365](https://github.com/can1357/oh-my-pi/issues/6365)) ## [17.0.9] - 2026-07-23 @@ -300,6 +317,9 @@ ### Changed - Changed `providers.webSearch` preferred provider failure handling to fall back and cascade through other configured/default search providers rather than stopping immediately. +### Fixed + +- Fixed snapcompact archiving reproduced assistant reasoning (`¶think:` sections) into frames replayed to the model on every subsequent request, wedging Fable 5 sessions on `reasoning_extraction` refusals; snapcompact serialization now excludes reasoning when the session model uses the Anthropic dialect ([#6093](https://github.com/can1357/oh-my-pi/issues/6093)). ## [17.0.5] - 2026-07-18 diff --git a/packages/coding-agent/README.md b/packages/coding-agent/README.md index d7dd383ae..eef3c9650 100644 --- a/packages/coding-agent/README.md +++ b/packages/coding-agent/README.md @@ -32,4 +32,4 @@ The agent supports three mutually-exclusive memory backends, selected via the `m - `HINDSIGHT_RECALL_BUDGET`, `HINDSIGHT_RECALL_MAX_TOKENS` — recall sizing - `HINDSIGHT_BANK_MISSION`, `HINDSIGHT_DEBUG` -Switching backends mid-session is honoured on the next system-prompt rebuild and the next `/memory` slash command. Existing users with `memories.enabled = true|false` are migrated to `memory.backend = "local"|"off"` exactly once on first launch. +Switching backends mid-session immediately replaces the live backend, memory tools, listeners, and system-prompt context. Existing users with `memories.enabled = true|false` are migrated to `memory.backend = "local"|"off"` exactly once on first launch; afterward, `memory.backend` is the sole runtime selector. diff --git a/packages/coding-agent/src/cli/gc-cli.ts b/packages/coding-agent/src/cli/gc-cli.ts index 0b78eec2a..5a20a1f6b 100644 --- a/packages/coding-agent/src/cli/gc-cli.ts +++ b/packages/coding-agent/src/cli/gc-cli.ts @@ -5,10 +5,10 @@ import { gunzipSync, gzipSync } from "node:zlib"; import { getAgentDir, getBlobsDir, getHistoryDbPath, getModelDbPath, getSessionsDir } from "@oh-my-pi/pi-utils"; import { Settings } from "../config/settings"; import { getDefault } from "../config/settings-schema"; +import { BLOB_HASH_RE } from "../session/blob-store"; import { listSessionsReadOnly, type SessionInfo, type SessionStatus } from "../session/session-listing"; import { FileSessionStorage } from "../session/session-storage"; -const HASH_RE = /^[a-f0-9]{64}$/; const BLOB_FILE_RE = /^([a-f0-9]{64})(?:\.[A-Za-z0-9][A-Za-z0-9._-]{0,31})?$/; const BLOB_REF_RE = /\bblob:sha256:([a-f0-9]{64})\b/gi; const JSONL_GLOB = new Bun.Glob("**/*.jsonl"); @@ -268,7 +268,7 @@ async function collectReferencedBlobHashes(sessionRoots: string[]): Promise= 0", + "promptCacheMaximumCheckpoints?": "number >= 0", +}); + +// Provider-level overrides can target bundled models whose API is not repeated +// in models.yml, so preserve the sparse compat shape for each supported API. +const ApiCompatSchema = OpenAICompatSchema.or(BedrockCompatSchema); + const ApiSchema = type( - '"openai-completions" | "openai-responses" | "openai-codex-responses" | "azure-openai-responses" | "anthropic-messages" | "google-generative-ai" | "google-gemini-cli" | "google-vertex"', + '"openai-completions" | "openai-responses" | "openai-codex-responses" | "azure-openai-responses" | "anthropic-messages" | "bedrock-converse-stream" | "google-generative-ai" | "google-gemini-cli" | "google-vertex"', ); const EffortSchema = type('"minimal" | "low" | "medium" | "high" | "xhigh" | "max"'); @@ -173,7 +184,7 @@ const ModelDefinitionSchema = type({ "maxTokens?": "number", "omitMaxOutputTokens?": "boolean", "headers?": { "[string]": "string" }, - "compat?": OpenAICompatSchema, + "compat?": ApiCompatSchema, "contextPromotionTarget?": "string", "compactionModel?": "string", "remoteCompaction?": RemoteCompactionSchema, @@ -222,7 +233,7 @@ export const ModelOverrideSchema = type({ "maxTokens?": "number", "omitMaxOutputTokens?": "boolean", "headers?": { "[string]": "string" }, - "compat?": OpenAICompatSchema, + "compat?": ApiCompatSchema, "contextPromotionTarget?": "string", "compactionModel?": "string", "remoteCompaction?": RemoteCompactionSchema, @@ -263,7 +274,7 @@ const ProviderConfigSchema = type({ "apiKey?": "string", "api?": ApiSchema, "headers?": { "[string]": "string" }, - "compat?": OpenAICompatSchema, + "compat?": ApiCompatSchema, "remoteCompaction?": RemoteCompactionSchema, "authHeader?": "boolean", "auth?": ProviderAuthSchema, diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index 168f6603d..37c465faa 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -2493,9 +2493,8 @@ export const SETTINGS_SCHEMA = { "memories.summaryInjectionTokenLimit": { type: "number", default: 5000 }, // Memory backend selector — picks between local memories pipeline, - // Mnemopi local SQLite, Hindsight remote memory, or off. Legacy - // `memories.enabled` keeps gating the local backend; see config/settings.ts - // migration for details. + // Mnemopi local SQLite, Hindsight remote memory, or off. The legacy + // `memories.enabled` flag is migration input only; see config/settings.ts. "memory.backend": { type: "enum", values: ["off", "local", "hindsight", "mnemopi"] as const, diff --git a/packages/coding-agent/src/config/settings.ts b/packages/coding-agent/src/config/settings.ts index 399c74465..c51457ec7 100644 --- a/packages/coding-agent/src/config/settings.ts +++ b/packages/coding-agent/src/config/settings.ts @@ -364,6 +364,7 @@ export class Settings { if (options.configFiles) configFiles.push(...options.configFiles); this.#configFiles = configFiles.map(file => path.resolve(this.#cwd, expandTilde(file))); this.#persist = !options.inMemory && options.readOnly !== true; + liveSettingsInstances.add(new WeakRef(this)); if (options.overrides) { for (const [key, value] of Object.entries(options.overrides)) { @@ -537,6 +538,23 @@ export class Settings { } } + /** Set once this instance is discarded; background saves become no-ops. */ + #savesCancelled = false; + + /** + * Drop pending debounced saves and refuse any further background writes. + * Used when an instance is being discarded (test teardown): an armed timer + * or a chained in-flight save on a dropped instance would otherwise fire + * later and race the successor's file locks. + */ + cancelPendingSaves(): void { + this.#savesCancelled = true; + clearTimeout(this.#saveTimer); + this.#saveTimer = undefined; + clearTimeout(this.#projectSaveTimer); + this.#projectSaveTimer = undefined; + } + /** * Flush any pending saves to disk. * Call before exit to ensure all changes are persisted. @@ -1688,7 +1706,7 @@ export class Settings { } async #saveNow(): Promise { - if (!this.#persist || !this.#configPath) return; + if (this.#savesCancelled || !this.#persist || !this.#configPath) return; if (this.#modified.size === 0 && this.#modifiedGlobalModelRoles.size === 0) return; const configPath = this.#configPath; @@ -1792,7 +1810,7 @@ export class Settings { } async #saveProjectNow(): Promise { - if (!this.#persist || this.#modifiedProjectModelRoles.size === 0) return; + if (this.#savesCancelled || !this.#persist || this.#modifiedProjectModelRoles.size === 0) return; const projectConfigPath = path.join(this.#cwd, ".omp", "config.yml"); const modifiedModelRoles = [...this.#modifiedProjectModelRoles]; @@ -2023,6 +2041,13 @@ export const onHindsightScopeChanged = (cb: () => void) => hindsightScopeSignal. // Global Singleton // ═══════════════════════════════════════════════════════════════════════════ +/** + * Weak registry of every constructed instance so `resetSettingsForTest` can + * disarm stray background saves on isolated instances too. WeakRefs never + * retain instances; the set is cleared on every test reset. + */ +const liveSettingsInstances = new Set>(); + let globalInstance: Settings | null = null; let globalInstancePromise: Promise | null = null; let boundSettingsInstance: Settings | null = null; @@ -2042,6 +2067,14 @@ export function isSettingsInitialized(): boolean { * @internal */ export function resetSettingsForTest(): void { + // Disarm every constructed instance's debounced saves — including isolated + // (non-singleton) instances: an armed timer or chained in-flight save on a + // dropped instance fires mid-way through the NEXT test and races its file + // locks/spies (cross-file pollution). + for (const ref of liveSettingsInstances) { + ref.deref()?.cancelPendingSaves(); + } + liveSettingsInstances.clear(); globalInstance = null; globalInstancePromise = null; clearBoundSettingsMethods(); diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index bc1c13e82..33a28c0b7 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -3,7 +3,7 @@ import hashlineGrammar from "@oh-my-pi/hashline/grammar.lark" with { type: "text import hashlineDescription from "@oh-my-pi/hashline/prompt.md" with { type: "text" }; import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; import type { ToolExample } from "@oh-my-pi/pi-ai"; -import { prompt } from "@oh-my-pi/pi-utils"; +import { isEnoent, isEnotdir, prompt } from "@oh-my-pi/pi-utils"; import { createLspWritethrough, flushLspWritethroughBatch, type WritethroughCallback, writethroughNoop } from "../lsp"; import { DeferredDiagnostics } from "../lsp/deferred-diagnostics"; import { getDiagnosticsLedger } from "../lsp/diagnostics-ledger"; @@ -12,7 +12,8 @@ import patchDescription from "../prompts/tools/patch.md" with { type: "text" }; import replaceDescription from "../prompts/tools/replace.md" with { type: "text" }; import type { ToolSession } from "../tools"; import { truncateForPrompt } from "../tools/approval"; -import { isInternalUrlPath } from "../tools/path-utils"; +import { findUniqueWorkspaceSuffix, isInternalUrlPath } from "../tools/path-utils"; +import { resolvePlanPath } from "../tools/plan-mode-guard"; import { type EditMode, normalizeEditMode, resolveEditMode } from "../utils/edit-mode"; import { executeHashlineSingle, hashlineEditParamsSchema } from "./hashline"; import { type ApplyPatchParams, applyPatchSchema, expandApplyPatchToEntries } from "./modes/apply-patch"; @@ -73,6 +74,24 @@ function resolveConfiguredEditMode(rawEditMode: string): EditMode | undefined { return editMode; } +async function resolveEditPath( + session: ToolSession, + authoredPath: string, + options: { mustExist: boolean; signal?: AbortSignal }, +): Promise { + if (!options.mustExist || isInternalUrlPath(authoredPath)) return authoredPath; + + try { + await Bun.file(resolvePlanPath(session, authoredPath)).stat(); + return authoredPath; + } catch (error) { + if (!isEnoent(error) && !isEnotdir(error)) throw error; + } + + const match = await findUniqueWorkspaceSuffix(authoredPath, session.cwd, options.signal); + return match?.displayPath ?? authoredPath; +} + function resolveAllowFuzzy(session: ToolSession, rawValue: string): boolean { switch (rawValue) { case "true": @@ -517,7 +536,7 @@ export class EditTool implements AgentTool { note: "All entries in one call apply to the top-level `path`; use separate calls for different files.", }, ] satisfies readonly ToolExample[], - execute: ( + execute: async ( tool: EditTool, params: EditParams, signal: AbortSignal | undefined, @@ -525,11 +544,15 @@ export class EditTool implements AgentTool { onUpdate?: (partialResult: AgentToolResult) => void, ) => { const { edits, path } = params as PatchParams; + const targetPath = await resolveEditPath(tool.session, path, { + mustExist: (edits[0]?.op ?? "update") !== "create", + signal, + }); const runs = (edits as PatchEditEntry[]).map( entry => (br: LspBatchRequest | undefined) => executePatchSingle({ session: tool.session, - path, + path: targetPath, params: entry, signal, batchRequest: br, @@ -542,7 +565,7 @@ export class EditTool implements AgentTool { beginDeferredDiagnosticsForPath: p => tool.#deferredDiagnostics.begin(p), }), ); - return executeSinglePathEntries(path, runs, batchRequest, onUpdate, tool.session.cwd, signal); + return executeSinglePathEntries(targetPath, runs, batchRequest, onUpdate, tool.session.cwd, signal); }, }, apply_patch: { @@ -564,14 +587,26 @@ export class EditTool implements AgentTool { onUpdate?: (partialResult: AgentToolResult) => void, ) => { const entries = expandApplyPatchToEntries(params as ApplyPatchParams); + // Resolve each authored path once per patch so paired hunks (e.g. delete + // then re-add of the same file) share the same workspace target. + const resolvedTargets = new Map>(); + const resolveOnce = (path: string, mustExist: boolean): Promise => { + let pending = resolvedTargets.get(path); + if (!pending) { + pending = resolveEditPath(tool.session, path, { mustExist, signal }); + resolvedTargets.set(path, pending); + } + return pending; + }; const perFile = entries.map(entry => { const { path, ...patchParams } = entry; return { path, - run: (br: LspBatchRequest | undefined) => - executePatchSingle({ + run: async (br: LspBatchRequest | undefined) => { + const targetPath = await resolveOnce(path, patchParams.op !== "create"); + return executePatchSingle({ session: tool.session, - path, + path: targetPath, params: patchParams, signal, batchRequest: br, @@ -579,7 +614,8 @@ export class EditTool implements AgentTool { fuzzyThreshold: tool.#fuzzyThreshold, writethrough: tool.#writethrough, beginDeferredDiagnosticsForPath: p => tool.#deferredDiagnostics.begin(p), - }), + }); + }, }; }); return executeApplyPatchPerFile(perFile, batchRequest, tool.session.cwd, signal, onUpdate); @@ -609,7 +645,7 @@ export class EditTool implements AgentTool { replace: { description: () => prompt.render(replaceDescription), parameters: replaceEditSchema, - execute: ( + execute: async ( tool: EditTool, params: EditParams, signal: AbortSignal | undefined, @@ -617,11 +653,12 @@ export class EditTool implements AgentTool { onUpdate?: (partialResult: AgentToolResult) => void, ) => { const { edits, path } = params as ReplaceParams; + const targetPath = await resolveEditPath(tool.session, path, { mustExist: true, signal }); const runs = (edits as ReplaceEditEntry[]).map( entry => (br: LspBatchRequest | undefined) => executeReplaceSingle({ session: tool.session, - path, + path: targetPath, params: entry, signal, batchRequest: br, @@ -631,7 +668,7 @@ export class EditTool implements AgentTool { beginDeferredDiagnosticsForPath: p => tool.#deferredDiagnostics.begin(p), }), ); - return executeSinglePathEntries(path, runs, batchRequest, onUpdate, tool.session.cwd, signal); + return executeSinglePathEntries(targetPath, runs, batchRequest, onUpdate, tool.session.cwd, signal); }, }, }[this.mode]; diff --git a/packages/coding-agent/src/eval/py/executor.ts b/packages/coding-agent/src/eval/py/executor.ts index fa44c074b..806e99a98 100644 --- a/packages/coding-agent/src/eval/py/executor.ts +++ b/packages/coding-agent/src/eval/py/executor.ts @@ -21,6 +21,7 @@ import { type KernelDisplayOutput, type KernelExecuteOptions, type KernelExecuteResult, + type KernelShutdownResult, PythonKernel, } from "./kernel"; import { resolveExplicitPythonRuntime } from "./runtime"; @@ -136,11 +137,19 @@ export interface PythonResult { // the same tuple; the kernel stays alive until the last owner detaches. // --------------------------------------------------------------------------- +interface SessionKernelReplacement { + generation: number; + deadlineMs?: number; + promise: Promise; +} + interface PythonSession { sessionKey: string; sessionId: string; cwd: string; kernel: PythonKernel; + generation: number; + replacement?: SessionKernelReplacement; ownerIds: Set; hasFallbackOwner: boolean; } @@ -254,6 +263,7 @@ async function acquireSession( sessionId, cwd, kernel, + generation: 0, ownerIds: new Set(), hasFallbackOwner: false, }; @@ -272,31 +282,99 @@ async function acquireSession( async function replaceSessionKernel( session: PythonSession, + kernel: PythonKernel, + generation: number, cwd: string, options: PythonExecutorOptions, -): Promise { - const old = session.kernel; - const remaining = getRemainingTimeoutMs(options.deadlineMs); - await old - .shutdown(remaining !== undefined ? { timeoutMs: Math.max(0, remaining) } : undefined) - .catch(() => undefined); - if (sessions.get(session.sessionKey) !== session) { +): Promise { + const inFlight = session.replacement; + if (inFlight?.generation === generation) { + if ( + inFlight.deadlineMs !== undefined && + (options.deadlineMs === undefined || options.deadlineMs > inFlight.deadlineMs) + ) { + inFlight.deadlineMs = options.deadlineMs; + } + return await waitForPromiseWithCancellation(inFlight.promise, options, PythonExecutionCancelledError); + } + if (sessions.get(session.sessionKey) !== session || session.generation !== generation || session.kernel !== kernel) { throw new PythonExecutionCancelledError(false); } - requireRemainingTimeoutMs(options.deadlineMs); - const next = await startKernel(cwd, options); - if (sessions.get(session.sessionKey) !== session) { - await next.shutdown().catch(() => undefined); - throw new PythonExecutionCancelledError(false); + + const deferred = Promise.withResolvers(); + const replacement: SessionKernelReplacement = { + generation, + deadlineMs: options.deadlineMs, + promise: deferred.promise, + }; + session.replacement = replacement; + void (async () => { + try { + const remaining = getRemainingTimeoutMs(options.deadlineMs); + await kernel + .shutdown(remaining !== undefined ? { timeoutMs: Math.max(0, remaining) } : undefined) + .catch(() => undefined); + if (replacement.deadlineMs !== undefined && replacement.deadlineMs <= Date.now()) { + throw new PythonExecutionCancelledError(true); + } + if ( + sessions.get(session.sessionKey) !== session || + session.generation !== generation || + session.kernel !== kernel + ) { + throw new PythonExecutionCancelledError(false); + } + const next = await startKernel(cwd, { + ...options, + signal: undefined, + deadlineMs: undefined, + }); + if ( + sessions.get(session.sessionKey) !== session || + session.generation !== generation || + session.kernel !== kernel + ) { + await next.shutdown().catch(() => undefined); + throw new PythonExecutionCancelledError(false); + } + session.kernel = next; + session.generation += 1; + deferred.resolve(next); + } catch (err) { + deferred.reject(err); + } finally { + if (session.replacement === replacement) session.replacement = undefined; + } + })(); + return await waitForPromiseWithCancellation(deferred.promise, options, PythonExecutionCancelledError); +} + +async function shutdownInvalidatedSession(session: PythonSession): Promise { + const replacement = session.replacement; + if (replacement) await replacement.promise.catch(() => undefined); + return await session.kernel.shutdown(); +} + +async function acquireLiveSessionKernel( + session: PythonSession, + cwd: string, + options: PythonExecutorOptions, +): Promise { + while (sessions.get(session.sessionKey) === session) { + const kernel = session.kernel; + const generation = session.generation; + if (kernel.isAlive()) return kernel; + await replaceSessionKernel(session, kernel, generation, cwd, options); } - session.kernel = next; + throw new PythonExecutionCancelledError(false); } async function resetSession(sessionKey: string): Promise { const existing = sessions.get(sessionKey) ?? (await startingSessions.get(sessionKey)?.catch(() => undefined)); if (!existing) return; + existing.generation += 1; sessions.delete(sessionKey); - await existing.kernel.shutdown().catch(() => undefined); + await shutdownInvalidatedSession(existing).catch(() => undefined); } // --------------------------------------------------------------------------- @@ -315,9 +393,10 @@ export async function disposeAllKernelSessions(): Promise { } } for (const [id, session] of all) { + session.generation += 1; if (sessions.get(id) === session) sessions.delete(id); } - const results = await Promise.allSettled(all.map(([, session]) => session.kernel.shutdown())); + const results = await Promise.allSettled(all.map(([, session]) => shutdownInvalidatedSession(session))); for (let i = 0; i < all.length; i += 1) { const [id, session] = all[i]; const result = results[i]; @@ -344,9 +423,10 @@ export async function disposeKernelSessionsByOwner(ownerId: string): Promise session.kernel.shutdown())); + const results = await Promise.allSettled(toShutdown.map(session => shutdownInvalidatedSession(session))); for (let i = 0; i < toShutdown.length; i += 1) { const session = toShutdown[i]; const result = results[i]; @@ -458,31 +538,21 @@ async function executeOnSession(code: string, cwd: string, options: PythonExecut isTimedOutCancellation(options.signal.reason, PythonExecutionCancelledError, options.signal), ); } - if (sessions.get(session.sessionKey) !== session) { + const kernel = await acquireLiveSessionKernel(session, cwd, options); + if (sessions.get(session.sessionKey) !== session || session.kernel !== kernel) { throw new PythonExecutionCancelledError(false); } - if (!session.kernel.isAlive()) { - await replaceSessionKernel(session, cwd, options); - if (sessions.get(session.sessionKey) !== session) { - throw new PythonExecutionCancelledError(false); - } - } const runOptions = { ...options, cwd }; try { - return await executeWithKernel(session.kernel, code, runOptions); + return await executeWithKernel(kernel, code, runOptions); } catch (err) { if (isCancellationError(err, PythonExecutionCancelledError) || options.signal?.aborted) throw err; - if (session.kernel.isAlive()) throw err; - if (sessions.get(session.sessionKey) !== session) { + if (kernel.isAlive()) throw err; + const retryKernel = await acquireLiveSessionKernel(session, cwd, options); + if (sessions.get(session.sessionKey) !== session || session.kernel !== retryKernel) { throw new PythonExecutionCancelledError(false); } - // Shared kernels are keyed by cwd, so a dead kernel can be recreated in place - // without risking cross-directory state bleed. - await replaceSessionKernel(session, cwd, options); - if (sessions.get(session.sessionKey) !== session) { - throw new PythonExecutionCancelledError(false); - } - return await executeWithKernel(session.kernel, code, runOptions); + return await executeWithKernel(retryKernel, code, runOptions); } } diff --git a/packages/coding-agent/src/lsp/client.ts b/packages/coding-agent/src/lsp/client.ts index d5f0f0c34..c02e73cd6 100644 --- a/packages/coding-agent/src/lsp/client.ts +++ b/packages/coding-agent/src/lsp/client.ts @@ -364,7 +364,15 @@ async function startMessageReader(client: LspClient): Promise { if (pending) { client.pendingRequests.delete(message.id); if ("error" in message && message.error) { - pending.reject(new Error(`LSP error: ${message.error.message}`)); + // Include the JSON-RPC error code: `isMethodNotFoundError` matches + // `-32601` by substring, so method-not-found is recognized even when + // the server's message text is nonstandard (e.g. "Unknown request"). + const code = message.error.code; + pending.reject( + new Error( + `LSP error${typeof code === "number" ? ` ${code}` : ""}: ${message.error.message}`, + ), + ); } else { pending.resolve(message.result); } @@ -1181,9 +1189,19 @@ async function waitForExit(client: LspClient, timeoutMs: number): Promise { +export async function shutdownClientInstance(client: LspClient): Promise { + if (clients.get(client.name) === client) clients.delete(client.name); + const err = new Error("LSP client shutdown"); for (const pending of Array.from(client.pendingRequests.values())) { pending.reject(err); @@ -1196,21 +1214,23 @@ async function shutdownClientInstance(client: LspClient): Promise { ); if (shutdownCompleted) { await sendNotification(client, "exit", undefined).catch(() => {}); - if (await waitForExit(client, EXIT_TIMEOUT_MS)) return; + if (await waitForExit(client, EXIT_TIMEOUT_MS)) return true; } client.proc.kill(); - await waitForExit(client, EXIT_TIMEOUT_MS); + return await waitForExit(client, EXIT_TIMEOUT_MS); } /** * Shutdown a specific client by key. + * + * @returns `true` when the client is gone (already absent or confirmed exited), + * `false` if a live process outlived the shutdown budget. */ -export async function shutdownClient(key: string): Promise { +export async function shutdownClient(key: string): Promise { const client = clients.get(key); - if (!client) return; - clients.delete(key); - await shutdownClientInstance(client); + if (!client) return true; + return await shutdownClientInstance(client); } // ============================================================================= diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index 12dc7dd9b..b7deae583 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -29,6 +29,7 @@ import { sendNotification, sendRequest, setIdleTimeout, + shutdownClientInstance, supportsDocumentDiagnostics, syncContent, WARMUP_TIMEOUT_MS, @@ -507,12 +508,18 @@ function isMethodNotFoundError(err: unknown): boolean { } async function reloadServer(client: LspClient, serverName: string, signal?: AbortSignal): Promise { - // rust-analyzer exposes a real reload request. + throwIfAborted(signal); + // rust-analyzer exposes a real reload request. Every other server rejects it + // with method-not-found — that alone justifies the generic fallback. A caller + // cancel or tool timeout must propagate, never be mistaken for an unsupported + // method and swallowed into a bogus "Restarted" (issue #6369). try { await sendRequest(client, "rust-analyzer/reloadWorkspace", null, signal); return `Reloaded ${serverName}`; - } catch { - // Method not supported — fall through. + } catch (err) { + throwIfAborted(signal); + if (!isMethodNotFoundError(err)) throw err; + // Method not supported — fall through to the generic reload. } // workspace/didChangeConfiguration is a notification per spec; sending it // as a request hangs until the tool deadline on servers that route it to @@ -521,7 +528,15 @@ async function reloadServer(client: LspClient, serverName: string, signal?: Abor await sendNotification(client, "workspace/didChangeConfiguration", { settings: {} }, signal); return `Reloaded ${serverName}`; } catch { - client.proc.kill(); + throwIfAborted(signal); + // The reload notification could not be delivered — the connection is + // wedged or the process already died. Tear the client down (removing it + // from the registry by identity and awaiting confirmed process exit) so + // the next request cold-starts a fresh client. A kill that never confirms + // exit is not a restart: surface the teardown failure truthfully. + if (!(await shutdownClientInstance(client))) { + throw new Error(`Failed to restart ${serverName}: server process did not exit after kill`); + } return `Restarted ${serverName}`; } } diff --git a/packages/coding-agent/src/mcp/config-writer.test.ts b/packages/coding-agent/src/mcp/config-writer.test.ts new file mode 100644 index 000000000..330f73f27 --- /dev/null +++ b/packages/coding-agent/src/mcp/config-writer.test.ts @@ -0,0 +1,43 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { addMCPServer, readDisabledServers, readMCPConfigFile, setServerDisabled } from "./config-writer"; + +describe("config-writer concurrent mutations", () => { + let dir: string; + let filePath: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-mcp-config-")); + filePath = path.join(dir, "mcp.json"); + }); + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + it("preserves both servers when two adds race the same file", async () => { + await Promise.all([ + addMCPServer(filePath, "alpha", { type: "stdio", command: "a" }), + addMCPServer(filePath, "bravo", { type: "stdio", command: "b" }), + ]); + + const config = await readMCPConfigFile(filePath); + expect(Object.keys(config.mcpServers ?? {}).sort()).toEqual(["alpha", "bravo"]); + }); + + it("preserves both denylist edits when disable calls race", async () => { + await Promise.all([setServerDisabled(filePath, "alpha", true), setServerDisabled(filePath, "bravo", true)]); + + expect((await readDisabledServers(filePath)).sort()).toEqual(["alpha", "bravo"]); + }); + + it("writes into a directory that does not exist yet", async () => { + const nestedPath = path.join(dir, "nested", "deep", "mcp.json"); + await addMCPServer(nestedPath, "alpha", { type: "stdio", command: "a" }); + + const config = await readMCPConfigFile(nestedPath); + expect(Object.keys(config.mcpServers ?? {})).toEqual(["alpha"]); + }); +}); diff --git a/packages/coding-agent/src/mcp/config-writer.ts b/packages/coding-agent/src/mcp/config-writer.ts index 238f13e91..4d1855fc3 100644 --- a/packages/coding-agent/src/mcp/config-writer.ts +++ b/packages/coding-agent/src/mcp/config-writer.ts @@ -3,10 +3,12 @@ * * Utilities for reading/writing .omp/mcp.json files at user or project level. */ +import { randomUUID } from "node:crypto"; import * as fs from "node:fs"; import * as path from "node:path"; import { isEnoent } from "@oh-my-pi/pi-utils"; import { invalidate as invalidateFsCache } from "../capability/fs"; +import { withFileLock } from "../config/file-lock"; import { validateServerConfig } from "./config"; import { MCP_CONFIG_SCHEMA_URL, type MCPConfigFile, type MCPServerConfig } from "./types"; @@ -18,6 +20,20 @@ function withSchema(config: MCPConfigFile): MCPConfigFile { }; } +/** + * Serialize a read-modify-write against one config file. + * + * Wraps {@link withFileLock} but first ensures the config's parent directory + * exists, because the lock directory (`${filePath}.lock`) is created with a + * non-recursive `mkdir` — without this the very first write (before the config + * file or its parent exists) would fail to acquire the lock with ENOENT. + */ +function withConfigLock(filePath: string, fn: () => Promise): Promise { + return fs.promises + .mkdir(path.dirname(filePath), { recursive: true, mode: 0o700 }) + .then(() => withFileLock(filePath, fn)); +} + /** * Read an MCP config file. * Returns empty config if file doesn't exist. @@ -45,13 +61,20 @@ export async function writeMCPConfigFile(filePath: string, config: MCPConfigFile const dir = path.dirname(filePath); await fs.promises.mkdir(dir, { recursive: true, mode: 0o700 }); - // Write to temp file first (atomic write) - const tmpPath = `${filePath}.tmp`; + // Write to a per-writer temp file, then atomically rename into place. The + // temp name is unique (pid + random) so two concurrent writers to the same + // config never share one `.tmp` path and rename each other's file out from + // under them (which surfaced as ENOENT or a clobbered final file). + const tmpPath = `${filePath}.${process.pid}.${randomUUID()}.tmp`; const content = JSON.stringify(withSchema(config), null, 2); - await fs.promises.writeFile(tmpPath, content, { encoding: "utf-8", mode: 0o600 }); - - // Rename to final path (atomic on most systems) - await fs.promises.rename(tmpPath, filePath); + try { + await fs.promises.writeFile(tmpPath, content, { encoding: "utf-8", mode: 0o600 }); + // Rename to final path (atomic on most systems) + await fs.promises.rename(tmpPath, filePath); + } catch (error) { + await fs.promises.rm(tmpPath, { force: true }).catch(() => {}); + throw error; + } // Invalidate the capability fs cache so subsequent reads see the new content invalidateFsCache(filePath); } @@ -97,25 +120,26 @@ export async function addMCPServer(filePath: string, name: string, config: MCPSe throw new Error(`Invalid server config: ${errors.join("; ")}`); } - // Read existing config - const existing = await readMCPConfigFile(filePath); + // Serialize the read-modify-write under a per-file lock so a concurrent + // mutation cannot overwrite this one (lost update). The lock also guards + // against cross-process writers sharing the same config file. + await withConfigLock(filePath, async () => { + const existing = await readMCPConfigFile(filePath); - // Check for duplicate name - if (existing.mcpServers?.[name]) { - throw new Error(`Server "${name}" already exists in ${filePath}`); - } + // Check for duplicate name + if (existing.mcpServers?.[name]) { + throw new Error(`Server "${name}" already exists in ${filePath}`); + } - // Add server - const updated: MCPConfigFile = { - ...existing, - mcpServers: { - ...existing.mcpServers, - [name]: config, - }, - }; - - // Write back - await writeMCPConfigFile(filePath, updated); + const updated: MCPConfigFile = { + ...existing, + mcpServers: { + ...existing.mcpServers, + [name]: config, + }, + }; + await writeMCPConfigFile(filePath, updated); + }); } /** @@ -137,20 +161,19 @@ export async function updateMCPServer(filePath: string, name: string, config: MC throw new Error(`Invalid server config: ${errors.join("; ")}`); } - // Read existing config - const existing = await readMCPConfigFile(filePath); + // Serialize the read-modify-write (see addMCPServer). + await withConfigLock(filePath, async () => { + const existing = await readMCPConfigFile(filePath); - // Update server - const updated: MCPConfigFile = { - ...existing, - mcpServers: { - ...existing.mcpServers, - [name]: config, - }, - }; - - // Write back - await writeMCPConfigFile(filePath, updated); + const updated: MCPConfigFile = { + ...existing, + mcpServers: { + ...existing.mcpServers, + [name]: config, + }, + }; + await writeMCPConfigFile(filePath, updated); + }); } /** @@ -159,23 +182,21 @@ export async function updateMCPServer(filePath: string, name: string, config: MC * @throws Error if server doesn't exist */ export async function removeMCPServer(filePath: string, name: string): Promise { - // Read existing config - const existing = await readMCPConfigFile(filePath); + // Serialize the read-modify-write (see addMCPServer). + await withConfigLock(filePath, async () => { + const existing = await readMCPConfigFile(filePath); - // Check if server exists - if (!existing.mcpServers?.[name]) { - throw new Error(`Server "${name}" not found in ${filePath}`); - } + if (!existing.mcpServers?.[name]) { + throw new Error(`Server "${name}" not found in ${filePath}`); + } - // Remove server - const { [name]: _removed, ...remaining } = existing.mcpServers; - const updated: MCPConfigFile = { - ...existing, - mcpServers: remaining, - }; - - // Write back - await writeMCPConfigFile(filePath, updated); + const { [name]: _removed, ...remaining } = existing.mcpServers; + const updated: MCPConfigFile = { + ...existing, + mcpServers: remaining, + }; + await writeMCPConfigFile(filePath, updated); + }); } /** @@ -207,25 +228,28 @@ export async function readDisabledServers(filePath: string): Promise { * Add or remove a server name from the disabled servers list. */ export async function setServerDisabled(filePath: string, name: string, disabled: boolean): Promise { - const config = await readMCPConfigFile(filePath); - const current = new Set(config.disabledServers ?? []); + // Serialize the read-modify-write (see addMCPServer). + await withConfigLock(filePath, async () => { + const config = await readMCPConfigFile(filePath); + const current = new Set(config.disabledServers ?? []); - if (disabled) { - current.add(name); - } else { - current.delete(name); - } + if (disabled) { + current.add(name); + } else { + current.delete(name); + } - const updated: MCPConfigFile = { - ...config, - disabledServers: current.size > 0 ? Array.from(current).sort() : undefined, - }; + const updated: MCPConfigFile = { + ...config, + disabledServers: current.size > 0 ? Array.from(current).sort() : undefined, + }; - if (!updated.disabledServers) { - delete updated.disabledServers; - } + if (!updated.disabledServers) { + delete updated.disabledServers; + } - await writeMCPConfigFile(filePath, updated); + await writeMCPConfigFile(filePath, updated); + }); } /** @@ -243,25 +267,28 @@ export async function readEnabledServers(filePath: string): Promise { * NOT override the `disabledServers` denylist. */ export async function setServerForceEnabled(filePath: string, name: string, force: boolean): Promise { - const config = await readMCPConfigFile(filePath); - const current = new Set(config.enabledServers ?? []); + // Serialize the read-modify-write (see addMCPServer). + await withConfigLock(filePath, async () => { + const config = await readMCPConfigFile(filePath); + const current = new Set(config.enabledServers ?? []); - if (force) { - current.add(name); - } else { - current.delete(name); - } + if (force) { + current.add(name); + } else { + current.delete(name); + } - const updated: MCPConfigFile = { - ...config, - enabledServers: current.size > 0 ? Array.from(current).sort() : undefined, - }; + const updated: MCPConfigFile = { + ...config, + enabledServers: current.size > 0 ? Array.from(current).sort() : undefined, + }; - if (!updated.enabledServers) { - delete updated.enabledServers; - } + if (!updated.enabledServers) { + delete updated.enabledServers; + } - await writeMCPConfigFile(filePath, updated); + await writeMCPConfigFile(filePath, updated); + }); } /** Paths and target state for toggling one MCP server across known config files. */ diff --git a/packages/coding-agent/src/mcp/oauth-discovery.ts b/packages/coding-agent/src/mcp/oauth-discovery.ts index c47830823..ab9927379 100644 --- a/packages/coding-agent/src/mcp/oauth-discovery.ts +++ b/packages/coding-agent/src/mcp/oauth-discovery.ts @@ -6,6 +6,10 @@ */ import * as AIError from "@oh-my-pi/pi-ai/error"; import type { FetchImpl } from "@oh-my-pi/pi-ai/types"; +import { withTimeoutSignal } from "../utils/fetch-timeout"; + +/** Per-request abort deadline for each OAuth discovery metadata fetch. */ +const DISCOVERY_FETCH_TIMEOUT_MS = 10_000; export interface OAuthEndpoints { authorizationUrl: string; @@ -346,7 +350,7 @@ function readMetadataScopes(metadata: Record): string | undefin */ export async function fetchResourceMetadataScopes( resourceMetadataUrl: string, - opts?: { fetch?: FetchImpl }, + opts?: { fetch?: FetchImpl; signal?: AbortSignal }, ): Promise { const fetchImpl: FetchImpl = opts?.fetch ?? fetch; try { @@ -354,6 +358,7 @@ export async function fetchResourceMetadataScopes( method: "GET", headers: { Accept: "application/json" }, redirect: "follow", + signal: withTimeoutSignal(DISCOVERY_FETCH_TIMEOUT_MS, opts?.signal), }); if (!resp.ok) return undefined; const meta = (await resp.json()) as Record; @@ -371,7 +376,7 @@ export async function discoverOAuthEndpoints( serverUrl: string, authServerUrl?: string, resourceMetadataUrl?: string, - opts?: { fetch?: FetchImpl; protectedResource?: string; protectedScopes?: string }, + opts?: { fetch?: FetchImpl; protectedResource?: string; protectedScopes?: string; signal?: AbortSignal }, ): Promise { const fetchImpl: FetchImpl = opts?.fetch ?? fetch; const wellKnownPaths = [ @@ -402,6 +407,7 @@ export async function discoverOAuthEndpoints( method: "GET", headers: { Accept: "application/json" }, redirect: "follow", + signal: withTimeoutSignal(DISCOVERY_FETCH_TIMEOUT_MS, opts?.signal), }); if (metaResp.ok) { const meta = (await metaResp.json()) as Record; @@ -486,6 +492,7 @@ export async function discoverOAuthEndpoints( method: "GET", headers: { Accept: "application/json" }, redirect: "follow", + signal: withTimeoutSignal(DISCOVERY_FETCH_TIMEOUT_MS, opts?.signal), }); if (response.ok) { @@ -521,6 +528,7 @@ export async function discoverOAuthEndpoints( fetch: fetchImpl, protectedResource: discoveredProtectedResource, protectedScopes: readMetadataScopes(metadata) ?? protectedScopes, + signal: opts?.signal, }); if (discovered) return discovered; } diff --git a/packages/coding-agent/src/mcp/smithery-auth.test.ts b/packages/coding-agent/src/mcp/smithery-auth.test.ts new file mode 100644 index 000000000..ccee396b2 --- /dev/null +++ b/packages/coding-agent/src/mcp/smithery-auth.test.ts @@ -0,0 +1,29 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { pollSmitheryCliAuthSession } from "./smithery-auth"; + +type FetchInput = string | URL | Request; +type FetchInit = RequestInit | BunFetchRequestInit; + +describe("pollSmitheryCliAuthSession fetch cancellation", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("bounds each poll request with a timeout abort signal even without a caller signal", async () => { + const signals: AbortSignal[] = []; + const fetchStub = Object.assign( + async (_input: FetchInput, init?: FetchInit) => { + if (init?.signal instanceof AbortSignal) signals.push(init.signal); + return Response.json({ status: "pending" }); + }, + { preconnect: globalThis.fetch.preconnect }, + ); + vi.spyOn(globalThis, "fetch").mockImplementation(fetchStub); + + const result = await pollSmitheryCliAuthSession("sess-123"); + + expect(result.status).toBe("pending"); + expect(signals).toHaveLength(1); + expect(signals[0]?.aborted).toBe(false); + }); +}); diff --git a/packages/coding-agent/src/mcp/smithery-auth.ts b/packages/coding-agent/src/mcp/smithery-auth.ts index 4aa03ae0e..0d34aa6ea 100644 --- a/packages/coding-agent/src/mcp/smithery-auth.ts +++ b/packages/coding-agent/src/mcp/smithery-auth.ts @@ -7,13 +7,14 @@ import { withTimeoutSignal } from "../utils/fetch-timeout"; const SMITHERY_AUTH_FILENAME = "smithery.json"; const SMITHERY_URL = process.env.SMITHERY_URL || "https://smithery.ai"; const SMITHERY_AUTH_TIMEOUT_MS = 10_000; +const SMITHERY_POLL_TIMEOUT_MS = 30_000; type SmitheryCliAuthSession = { sessionId: string; authUrl: string; }; -type SmitheryCliPollResponse = { +export type SmitheryCliPollResponse = { status: "pending" | "success" | "error"; apiKey?: string; message?: string; @@ -53,7 +54,7 @@ export async function pollSmitheryCliAuthSession( signal?: AbortSignal, ): Promise { const response = await fetch(`${SMITHERY_URL}/api/auth/cli/poll/${sessionId}`, { - signal, + signal: withTimeoutSignal(SMITHERY_POLL_TIMEOUT_MS, signal), }); if (!response.ok) { if (response.status === 404 || response.status === 410) { diff --git a/packages/coding-agent/src/memories/index.ts b/packages/coding-agent/src/memories/index.ts index 7342114fa..dd95c39db 100644 --- a/packages/coding-agent/src/memories/index.ts +++ b/packages/coding-agent/src/memories/index.ts @@ -142,9 +142,12 @@ export function startMemoryStartupTask(options: { return; } - void runMemoryStartup({ session, settings, modelRegistry, agentDir, config: cfg }).catch(error => { - logger.warn("Memory startup failed", { error: String(error) }); - }); + const signal = session.beginLocalMemoryStartup?.() ?? new AbortController().signal; + void runMemoryStartup({ session, settings, modelRegistry, agentDir, config: cfg, signal }) + .catch(error => { + if (!signal.aborted) logger.warn("Memory startup failed", { error: String(error) }); + }) + .finally(() => session.endLocalMemoryStartup?.(signal)); } interface MemoryInstructionSession { @@ -315,26 +318,32 @@ export function enqueueMemoryConsolidation(agentDir: string, cwd: string, source } } -async function runMemoryStartup(options: { +interface MemoryStartupOptions { session: AgentSession; settings: Settings; modelRegistry: ModelRegistry; agentDir: string; config: MemoryRuntimeConfig; -}): Promise { + signal: AbortSignal; +} + +function isMemoryStartupActive(options: MemoryStartupOptions): boolean { + return !options.signal.aborted && !options.session.isDisposed && options.settings.get("memory.backend") === "local"; +} + +async function runMemoryStartup(options: MemoryStartupOptions): Promise { + if (!isMemoryStartupActive(options)) return; await runPhase1(options); + if (!isMemoryStartupActive(options)) return; await runPhase2(options); + if (!isMemoryStartupActive(options)) return; await refreshMemoryToolDeveloperInstructionsCacheAfterStartup(options.session, options.agentDir, options.settings); + if (!isMemoryStartupActive(options)) return; await options.session.refreshBaseSystemPrompt?.(); } -async function runPhase1(options: { - session: AgentSession; - settings: Settings; - modelRegistry: ModelRegistry; - agentDir: string; - config: MemoryRuntimeConfig; -}): Promise { +async function runPhase1(options: MemoryStartupOptions): Promise { + if (!isMemoryStartupActive(options)) return; const { session, modelRegistry, agentDir, config } = options; const db = openMemoryDb(getAgentDbPath(agentDir)); const nowSec = unixNow(); @@ -344,6 +353,7 @@ async function runPhase1(options: { try { const threads = await collectThreads(session, currentThreadId); + if (!isMemoryStartupActive(options)) return; upsertThreads(db, threads); const phase1Model = await resolveMemoryModel({ @@ -364,6 +374,7 @@ async function runPhase1(options: { return; } + if (!isMemoryStartupActive(options)) return; const claims = claimStage1Jobs(db, { nowSec, threadScanLimit: config.threadScanLimit, @@ -387,6 +398,7 @@ async function runPhase1(options: { }; await runWithConcurrency(claims, config.stage1Concurrency, async claim => { + if (!isMemoryStartupActive(options)) return; const result = await runStage1Job({ claim, model: phase1Model, @@ -395,6 +407,7 @@ async function runPhase1(options: { config, metadata: session.agent?.metadataForProvider(phase1Model.provider), }); + if (!isMemoryStartupActive(options)) return; if (result.kind === "failed") { logger.error("Memory phase1 stage1 job failed", { @@ -460,13 +473,8 @@ async function runPhase1(options: { } } -async function runPhase2(options: { - session: AgentSession; - settings: Settings; - modelRegistry: ModelRegistry; - agentDir: string; - config: MemoryRuntimeConfig; -}): Promise { +async function runPhase2(options: MemoryStartupOptions): Promise { + if (!isMemoryStartupActive(options)) return; const { session, modelRegistry, agentDir, config } = options; const cwd = session.sessionManager.getCwd(); const db = openMemoryDb(getAgentDbPath(agentDir)); @@ -488,8 +496,10 @@ async function runPhase2(options: { const newWatermark = computeCompletionWatermark(claim.inputWatermark, outputs); await syncPhase2Artifacts(memoryRoot, outputs); + if (!isMemoryStartupActive(options)) return; if (outputs.length === 0) { await cleanupConsolidatedArtifacts(memoryRoot); + if (!isMemoryStartupActive(options)) return; const marked = markGlobalPhase2Succeeded(db, { ownershipToken: claim.ownershipToken, newWatermark, @@ -502,6 +512,7 @@ async function runPhase2(options: { return; } + if (!isMemoryStartupActive(options)) return; const phase2Model = await resolveMemoryModel({ modelRegistry, session, @@ -529,8 +540,13 @@ async function runPhase2(options: { return; } + if (!isMemoryStartupActive(options)) return; let heartbeatLostOwnership = false; const heartbeat = setInterval(() => { + if (!isMemoryStartupActive(options)) { + clearInterval(heartbeat); + return; + } const ok = heartbeatGlobalJob(db, { ownershipToken: claim.ownershipToken, leaseSeconds: config.phase2LeaseSeconds, @@ -544,13 +560,16 @@ async function runPhase2(options: { }, config.phase2HeartbeatSeconds * 1000); try { + if (!isMemoryStartupActive(options)) return; const consolidated = await runConsolidationModel({ memoryRoot, model: phase2Model, apiKey: modelRegistry.resolver(phase2Model, session.sessionId), metadata: session.agent?.metadataForProvider(phase2Model.provider), }); + if (!isMemoryStartupActive(options)) return; await applyConsolidation(memoryRoot, consolidated); + if (!isMemoryStartupActive(options)) return; if (heartbeatLostOwnership) { throw new Error("Phase2 lease ownership lost before completion"); } @@ -564,6 +583,7 @@ async function runPhase2(options: { throw new Error("Phase2 could not mark success: ownership lost"); } } catch (error) { + if (!isMemoryStartupActive(options)) return; markPhase2FailureWithFallback(db, { claim, retryDelaySeconds: config.phase2RetryDelaySeconds, @@ -1227,7 +1247,7 @@ async function resolveMemoryModel(options: { function loadMemoryConfig(settings: Settings): MemoryRuntimeConfig { return { - enabled: settings.get("memory.backend") === "local" || settings.get("memories.enabled") === true, + enabled: settings.get("memory.backend") === "local", maxRolloutsPerStartup: settings.get("memories.maxRolloutsPerStartup") ?? DEFAULTS.maxRolloutsPerStartup, maxRolloutAgeDays: settings.get("memories.maxRolloutAgeDays") ?? DEFAULTS.maxRolloutAgeDays, minRolloutIdleHours: settings.get("memories.minRolloutIdleHours") ?? DEFAULTS.minRolloutIdleHours, diff --git a/packages/coding-agent/src/memory-backend/tool-names.ts b/packages/coding-agent/src/memory-backend/tool-names.ts new file mode 100644 index 000000000..29330f00c --- /dev/null +++ b/packages/coding-agent/src/memory-backend/tool-names.ts @@ -0,0 +1,2 @@ +/** Built-in tools whose availability depends on the selected memory backend. */ +export const MEMORY_BACKEND_TOOL_NAMES = ["retain", "recall", "reflect", "memory_edit", "learn"] as const; diff --git a/packages/coding-agent/src/mnemopi/backend.ts b/packages/coding-agent/src/mnemopi/backend.ts index e951434ec..f72101818 100644 --- a/packages/coding-agent/src/mnemopi/backend.ts +++ b/packages/coding-agent/src/mnemopi/backend.ts @@ -62,6 +62,20 @@ const STATIC_INSTRUCTIONS = [ "", ].join("\n"); +async function installMnemopiState(session: AgentSession, config: MnemopiBackendConfig): Promise { + const state = new MnemopiSessionState({ sessionId: session.sessionId, config, session }); + const previous = setMnemopiSessionState(session, state); + await previous?.dispose(); + try { + state.attachSessionListeners(); + return state; + } catch (error) { + setMnemopiSessionState(session, undefined); + await state.dispose({ consolidate: false }); + throw error; + } +} + export const mnemopiBackend: MemoryBackend = { id: "mnemopi", @@ -90,10 +104,7 @@ export const mnemopiBackend: MemoryBackend = { try { const config = await loadMnemopiConfigWithProviders(settings, agentDir, modelRegistry, sessionId); await Promise.all([loadMnemopi(), loadMnemopiCore()]); - const state = new MnemopiSessionState({ sessionId, config, session }); - const previous = setMnemopiSessionState(session, state); - await previous?.dispose(); - state.attachSessionListeners(); + await installMnemopiState(session, config); } catch (error) { logger.warn("Mnemopi: backend startup failed; memory backend inert.", { error: String(error) }); } @@ -129,12 +140,19 @@ export const mnemopiBackend: MemoryBackend = { requireMnemopiCore().resetMemoryForTests(); await Bun.sleep(0); await removeDbFiles(getMnemopiScopedDbPaths(config)); + if (!session?.sessionId || previous?.aliasOf || session.settings.get("memory.backend") !== "mnemopi") return; + try { + await Promise.all([loadMnemopi(), loadMnemopiCore()]); + await installMnemopiState(session, config); + } catch (error) { + logger.warn("Mnemopi: clear rehydrate failed; memory backend inert.", { error: String(error) }); + } }, async enqueue(agentDir, _cwd, session): Promise { try { let state = getMnemopiSessionState(session); - if (!state && session) { + if (!state && session?.sessionId) { const config = await loadMnemopiConfigWithProviders( session.settings, agentDir, @@ -142,8 +160,7 @@ export const mnemopiBackend: MemoryBackend = { session.sessionId, ); await Promise.all([loadMnemopi(), loadMnemopiCore()]); - state = new MnemopiSessionState({ sessionId: session.sessionId, config, session }); - setMnemopiSessionState(session, state); + state = await installMnemopiState(session, config); } await state?.consolidate({ full: true }); } catch (error) { diff --git a/packages/coding-agent/src/modes/controllers/extension-ui-controller.ts b/packages/coding-agent/src/modes/controllers/extension-ui-controller.ts index f38d965f7..117f16c8f 100644 --- a/packages/coding-agent/src/modes/controllers/extension-ui-controller.ts +++ b/packages/coding-agent/src/modes/controllers/extension-ui-controller.ts @@ -532,7 +532,7 @@ export class ExtensionUiController { * Show a tool error in the chat. */ showToolError(toolName: string, error: string): void { - const errorText = new Text(theme.fg("error", `Tool "${toolName}" error: ${error}`), 1, 0); + const errorText = new Text(`Tool "${toolName}" error: ${error}`, 1, 0).setStyleFn(t => theme.fg("error", t)); this.ctx.present(errorText); } @@ -1092,7 +1092,9 @@ export class ExtensionUiController { } showExtensionError(extensionPath: string, error: string): void { - const errorText = new Text(theme.fg("error", `Extension "${extensionPath}" error: ${error}`), 1, 0); + const errorText = new Text(`Extension "${extensionPath}" error: ${error}`, 1, 0).setStyleFn(t => + theme.fg("error", t), + ); this.ctx.present(errorText); } async #handleInteractiveCompact(instructionsOrOptions: string | CompactOptions | undefined): Promise { diff --git a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts index 7a058ee36..928282fba 100644 --- a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts @@ -38,6 +38,7 @@ import { getSmitheryApiKey, getSmitheryLoginUrl, pollSmitheryCliAuthSession, + type SmitheryCliPollResponse, saveSmitheryApiKey, } from "../../mcp/smithery-auth"; import { SmitheryConnectError } from "../../mcp/smithery-connect"; @@ -51,6 +52,7 @@ import type { MCPAuthChallenge, MCPAuthConfig, MCPServerConfig, MCPServerConnect import { shortenPath } from "../../tools/render-utils"; import { urlHyperlinkAlways } from "../../tui"; import { copyToClipboard } from "../../utils/clipboard"; +import { isTimeoutError } from "../../utils/fetch-timeout"; import { openPath } from "../../utils/open"; import { ChatBlock } from "../components/chat-block"; import { MCPAddWizard } from "../components/mcp-add-wizard"; @@ -2121,7 +2123,14 @@ export class MCPCommandController { if (Date.now() - startedAt >= timeoutMs) { throw new Error("Smithery authorization timed out after 5 minutes."); } - const response = await pollSmitheryCliAuthSession(sessionId, signal); + let response: SmitheryCliPollResponse; + try { + response = await pollSmitheryCliAuthSession(sessionId, signal); + } catch (error) { + // A single hung/slow poll aborts with TimeoutError; retry until the deadline. + if (isTimeoutError(error)) continue; + throw error; + } if (response.status === "success" && response.apiKey) { return response.apiKey; } diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index cafc35267..4bf5681dd 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -437,6 +437,11 @@ export class SelectorController { this.ctx.showError(`Failed to apply personality: ${err}`); }); break; + case "memory.backend": + void this.ctx.session.applyMemoryBackend().catch(err => { + this.ctx.showError(`Failed to apply memory backend: ${err}`); + }); + break; case "autocompleteMaxVisible": this.ctx.editor.setAutocompleteMaxVisible(typeof value === "number" ? value : Number(value)); diff --git a/packages/coding-agent/src/modes/utils/ui-helpers.ts b/packages/coding-agent/src/modes/utils/ui-helpers.ts index 5192834e2..5cb931540 100644 --- a/packages/coding-agent/src/modes/utils/ui-helpers.ts +++ b/packages/coding-agent/src/modes/utils/ui-helpers.ts @@ -114,16 +114,19 @@ export class UiHelpers { const last = children.length > 0 ? children[children.length - 1] : undefined; const secondLast = children.length > 1 ? children[children.length - 2] : undefined; const useDim = options?.dim ?? true; - const rendered = useDim ? theme.fg("dim", message) : message; + // Resolve the dim color lazily so a later theme change re-shapes the line + // instead of leaving the palette that was active when it was presented. + const styleFn = useDim ? (t: string) => theme.fg("dim", t) : undefined; if (last && secondLast && last === this.ctx.lastStatusText && secondLast === this.ctx.lastStatusSpacer) { - this.ctx.lastStatusText.setText(rendered); + this.ctx.lastStatusText.setStyleFn(styleFn); + this.ctx.lastStatusText.setText(message); this.ctx.ui.requestRender(); return; } const spacer = new Spacer(1); - const text = new Text(rendered, 1, 0); + const text = new Text(message, 1, 0).setStyleFn(styleFn); this.ctx.present([spacer, text]); this.ctx.lastStatusSpacer = spacer; this.ctx.lastStatusText = text; @@ -714,11 +717,13 @@ export class UiHelpers { } showError(errorMessage: string): void { - this.ctx.present([new Spacer(1), new Text(theme.fg("error", `Error: ${errorMessage}`), 1, 0)]); + const text = new Text(`Error: ${errorMessage}`, 1, 0).setStyleFn(t => theme.fg("error", t)); + this.ctx.present([new Spacer(1), text]); } showWarning(warningMessage: string): void { - this.ctx.present([new Spacer(1), new Text(theme.fg("warning", `Warning: ${warningMessage}`), 1, 0)]); + const text = new Text(`Warning: ${warningMessage}`, 1, 0).setStyleFn(t => theme.fg("warning", t)); + this.ctx.present([new Spacer(1), text]); } showNewVersionNotification(newVersion: string): void { diff --git a/packages/coding-agent/src/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index 979f1eb09..ab3e8df8d 100644 --- a/packages/coding-agent/src/prompts/system/workflow-notice.md +++ b/packages/coding-agent/src/prompts/system/workflow-notice.md @@ -28,56 +28,64 @@ For independent per-item chains (review → verify, fetch → extract → score) **Python (`eval`, Python backend):** - DIMENSIONS = [{"key": "bugs", "prompt": "…"}, {"key": "perf", "prompt": "…"}] - def review_and_verify(d): - found = agent(d["prompt"], label=f"review:{d['key']}", schema=FINDINGS_SCHEMA) - return parallel([lambda f=f: {**f, "verdict": agent( - f"Refute if you can (default refuted when unsure): {f['title']}", - label=f"verify:{f['file']}", schema=VERDICT_SCHEMA)} for f in found["findings"]]) - phase("Review") - results = parallel([lambda d=d: review_and_verify(d) for d in DIMENSIONS]) - confirmed = [f for group in results for f in group if f["verdict"]["is_real"]] +```python +DIMENSIONS = [{"key": "bugs", "prompt": "…"}, {"key": "perf", "prompt": "…"}] +def review_and_verify(d): + found = agent(d["prompt"], label=f"review:{d['key']}", schema=FINDINGS_SCHEMA) + return parallel([lambda f=f: {**f, "verdict": agent( + f"Refute if you can (default refuted when unsure): {f['title']}", + label=f"verify:{f['file']}", schema=VERDICT_SCHEMA)} for f in found["findings"]]) +phase("Review") +results = parallel([lambda d=d: review_and_verify(d) for d in DIMENSIONS]) +confirmed = [f for group in results for f in group if f["verdict"]["is_real"]] +``` **JavaScript (`eval`, JavaScript backend):** - const DIMENSIONS = [{ key: "bugs", prompt: "…" }, { key: "perf", prompt: "…" }]; - async function reviewAndVerify(d) { - const found = await agent(d.prompt, { - label: `review:${d.key}`, - schema: FINDINGS_SCHEMA, - }); - return await parallel(found.findings.map((f) => async () => ({ - …f, - verdict: await agent( - `Refute if you can (default refuted when unsure): ${f.title}`, - { label: `verify:${f.file}`, schema: VERDICT_SCHEMA }, - ), - }))); - } - phase("Review"); - const results = await parallel(DIMENSIONS.map((d) => async () => reviewAndVerify(d))); - const confirmed = results.flat().filter((f) => f.verdict.is_real); +```js +const DIMENSIONS = [{ key: "bugs", prompt: "…" }, { key: "perf", prompt: "…" }]; +async function reviewAndVerify(d) { + const found = await agent(d.prompt, { + label: `review:${d.key}`, + schema: FINDINGS_SCHEMA, + }); + return await parallel(found.findings.map((f) => async () => ({ + ...f, + verdict: await agent( + `Refute if you can (default refuted when unsure): ${f.title}`, + { label: `verify:${f.file}`, schema: VERDICT_SCHEMA }, + ), + }))); +} +phase("Review"); +const results = await parallel(DIMENSIONS.map((d) => async () => reviewAndVerify(d))); +const confirmed = results.flat().filter((f) => f.verdict.is_real); +``` Reach for `pipeline()` only when a stage genuinely needs ALL of the previous stage first — dedup/merge across the whole set, early-exit on zero, or "compare against the other findings" — because its inter-stage barrier makes every item wait for the slowest peer: **Python (`eval`, Python backend):** - phase("Find") - found = parallel([lambda d=d: agent(d["prompt"], schema=FINDINGS_SCHEMA) for d in DIMENSIONS]) - findings = dedupe([f for r in found for f in r["findings"]]) # needs everything at once - phase("Verify") - verdicts = parallel([lambda f=f: agent(verify_prompt(f), schema=VERDICT_SCHEMA) for f in findings]) +```python +phase("Find") +found = parallel([lambda d=d: agent(d["prompt"], schema=FINDINGS_SCHEMA) for d in DIMENSIONS]) +findings = dedupe([f for r in found for f in r["findings"]]) # needs everything at once +phase("Verify") +verdicts = parallel([lambda f=f: agent(verify_prompt(f), schema=VERDICT_SCHEMA) for f in findings]) +``` **JavaScript (`eval`, JavaScript backend):** - phase("Find"); - const found = await parallel(DIMENSIONS.map((d) => async () => - await agent(d.prompt, { schema: FINDINGS_SCHEMA }), - )); - const findings = dedupe(found.flatMap((r) => r.findings)); // needs everything at once - phase("Verify"); - const verdicts = await parallel(findings.map((f) => async () => - await agent(verifyPrompt(f), { schema: VERDICT_SCHEMA }), - )); +```js +phase("Find"); +const found = await parallel(DIMENSIONS.map((d) => async () => + await agent(d.prompt, { schema: FINDINGS_SCHEMA }), +)); +const findings = dedupe(found.flatMap((r) => r.findings)); // needs everything at once +phase("Verify"); +const verdicts = await parallel(findings.map((f) => async () => + await agent(verifyPrompt(f), { schema: VERDICT_SCHEMA }), +)); +``` Use ordinary code between calls to flatten/map/filter; don't add a barrier just for that. Nested `parallel()` pools each cap independently, so keep total fan-out sane. diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 4fdc7ee7e..3ed00eaff 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -106,6 +106,7 @@ import { } from "./mcp"; import { MCP_CONNECTION_STATUS_EVENT_CHANNEL, type McpConnectionStatusEvent } from "./mcp/startup-events"; import { createSessionMemoryRuntimeContext, resolveMemoryBackend } from "./memory-backend"; +import { MEMORY_BACKEND_TOOL_NAMES } from "./memory-backend/tool-names"; import type { MnemopiSessionState } from "./mnemopi/state"; import lateDiagnosticTemplate from "./prompts/tools/lsp-late-diagnostic.md" with { type: "text" }; import { AgentLifecycleManager } from "./registry/agent-lifecycle"; @@ -3029,6 +3030,16 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} skillsSettings: settings.getGroup("skills"), modelRegistry, toolRegistry, + memoryAgentDir: agentDir, + memoryTaskDepth: taskDepth, + createMemoryTools: restrictToolNames + ? undefined + : async () => { + const tools = await Promise.all( + MEMORY_BACKEND_TOOL_NAMES.map(name => BUILTIN_TOOLS[name](toolSession)), + ); + return tools.filter((tool): tool is AgentTool => tool !== null); + }, createVibeTools: (options.taskDepth ?? 0) === 0 && !options.parentTaskPrefix ? () => createVibeTools(toolSession) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index a643cc000..1ec5a9fad 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -131,6 +131,7 @@ import { toolWireSchema } from "@oh-my-pi/pi-ai/utils/schema"; import { GeminiHeaderRunDetector, isGeminiThinkingModel } from "@oh-my-pi/pi-ai/utils/thinking-loop"; import { type RepeatedToolCallDetection, ToolCallLoopGuard } from "@oh-my-pi/pi-ai/utils/tool-call-loop-guard"; import { isFireworksFastModelId, toFireworksBaseModelId } from "@oh-my-pi/pi-catalog/fireworks-model-id"; +import { preferredDialect } from "@oh-my-pi/pi-catalog/identity"; import { getSupportedEfforts } from "@oh-my-pi/pi-catalog/model-thinking"; import { modelsAreEqual } from "@oh-my-pi/pi-catalog/models"; import { MacOSPowerAssertion } from "@oh-my-pi/pi-natives"; @@ -256,7 +257,8 @@ import type { Goal, GoalModeState } from "../goals/state"; import type { HindsightSessionState } from "../hindsight/state"; import { type LocalProtocolOptions, resolveLocalUrlToPath } from "../internal-urls"; import { IrcBus, type IrcMessage } from "../irc/bus"; -import { resolveMemoryBackend } from "../memory-backend"; +import { resolveMemoryBackend } from "../memory-backend/resolve"; +import { MEMORY_BACKEND_TOOL_NAMES } from "../memory-backend/tool-names"; import { shutdownMnemopiEmbedClient } from "../mnemopi/embed-client"; import { getMnemopiSessionState, type MnemopiSessionState, setMnemopiSessionState } from "../mnemopi/state"; import { containsOrchestrate, ORCHESTRATE_NOTICE } from "../modes/orchestrate"; @@ -936,6 +938,12 @@ export interface AgentSessionConfig { /** Custom commands (TypeScript slash commands) */ customCommands?: LoadedCustomCommand[]; skillsSettings?: SkillsSettings; + /** Agent directory used when applying memory backend changes during a live session. */ + memoryAgentDir?: string; + /** Recursion depth used to suppress live backend replacement in subagents. */ + memoryTaskDepth?: number; + /** Creates the built-in memory tools allowed by the current backend selection. */ + createMemoryTools?: () => Promise; /** Model registry for API key resolution and model discovery */ modelRegistry: ModelRegistry; /** Tool registry for LSP and settings */ @@ -2258,6 +2266,11 @@ export class AgentSession { #synchronouslyTerminatedYieldToolCallIds = new Set(); #providerSessionState = new Map(); #hindsightSessionState: HindsightSessionState | undefined = undefined; + #memoryAgentDir: string | undefined; + #memoryTaskDepth = 0; + #createMemoryTools: (() => Promise) | undefined; + #memoryBackendTransition: Promise = Promise.resolve(); + #localMemoryStartupAbort: AbortController | undefined; readonly rawSseDebugBuffer: RawSseDebugBuffer; #resetPromptMaintenanceState(): void { @@ -2790,6 +2803,9 @@ export class AgentSession { this.#skillsReloadable = config.skillsReloadable ?? true; this.#skillsSettings = config.skillsSettings; this.#modelRegistry = config.modelRegistry; + this.#memoryAgentDir = config.memoryAgentDir; + this.#memoryTaskDepth = config.memoryTaskDepth ?? 0; + this.#createMemoryTools = config.createMemoryTools; // Resolve the wire service-tier per request so the Fireworks Priority // toggle scopes priority to Fireworks alone, without mutating the shared // session `serviceTier` that drives `/fast` and OpenAI/Anthropic priority. @@ -3856,11 +3872,7 @@ export class AgentSession { this.sessionId, advisor.slug, ); - const preparation = prepareCompaction( - pathEntries, - compactionSettings, - await this.#runnableCompactionCandidates(candidates, advisorProviderSessionId), - ); + const preparation = prepareCompaction(pathEntries, compactionSettings, advisorModel); if (!preparation) { // Cannot prepare compaction, fallback to re-prime return true; @@ -5227,11 +5239,11 @@ export class AgentSession { return; } } - const resumeCursorStreamStall = this.#canResumeCursorStreamStall(msg); - if (resumeCursorStreamStall || this.#isRetryableError(msg)) { + const resumeResolvedStreamStall = this.#canResumeResolvedStreamStall(msg); + if (resumeResolvedStreamStall || this.#isRetryableError(msg)) { const didRetry = await this.#handleRetryableError( msg, - resumeCursorStreamStall ? { preserveFailedTurn: true } : undefined, + resumeResolvedStreamStall ? { preserveFailedTurn: true } : undefined, ); if (didRetry) { await emitAgentEndNotification({ willContinue: true }); @@ -6976,6 +6988,7 @@ export class AgentSession { */ beginDispose(): void { this.#isDisposed = true; + this.cancelLocalMemoryStartup(); this.#titleGenerationAbortController.abort(); this.#abortAutolearnCapture(); this.#flushPendingIrcAsides(); @@ -7106,6 +7119,7 @@ export class AgentSession { logger.warn("Post-prompt tasks still draining at dispose deadline", { error: String(error) }); } await this.#drainAutolearnCapture(); + await this.#memoryBackendTransition; const hindsightState = this.getHindsightSessionState(); const mnemopiState = setMnemopiSessionState(this, undefined); @@ -7845,13 +7859,126 @@ export class AgentSession { } } + /** Cancel the local rollout-memory startup owned by this session. */ + cancelLocalMemoryStartup(): void { + this.#localMemoryStartupAbort?.abort(); + this.#localMemoryStartupAbort = undefined; + } + + /** Start a new local rollout-memory generation and cancel its predecessor. */ + beginLocalMemoryStartup(): AbortSignal { + this.cancelLocalMemoryStartup(); + const controller = new AbortController(); + this.#localMemoryStartupAbort = controller; + return controller.signal; + } + + /** Release the local startup slot if `signal` still owns it. */ + endLocalMemoryStartup(signal: AbortSignal): void { + if (this.#localMemoryStartupAbort?.signal === signal) this.#localMemoryStartupAbort = undefined; + } + + async #disposeMemoryBackendState(consolidateMnemopi = true): Promise { + this.cancelLocalMemoryStartup(); + const hindsight = this.getHindsightSessionState(); + if (hindsight) { + try { + await hindsight.flushRetainQueue(); + } catch (error) { + logger.warn("Memory lifecycle: Hindsight flush failed", { error: String(error) }); + } + this.setHindsightSessionState(undefined); + hindsight.dispose(); + } + + const mnemopi = setMnemopiSessionState(this, undefined); + if (mnemopi) { + try { + await mnemopi.dispose({ consolidate: consolidateMnemopi }); + } catch (error) { + logger.warn("Memory lifecycle: Mnemopi dispose failed", { error: String(error) }); + } + } + } + + /** + * Apply the selected memory backend to runtime state, tools, and prompt. + * Concurrent settings changes run in order and settle before the next turn. + */ + async applyMemoryBackend(): Promise { + if (this.#isDisposed) return; + const transition = this.#memoryBackendTransition.then(() => this.#applyMemoryBackend()); + this.#memoryBackendTransition = transition.then( + () => undefined, + () => undefined, + ); + await transition; + } + + async #applyMemoryBackend(): Promise { + if (this.#isDisposed) return; + try { + await this.#disposeMemoryBackendState(); + if (this.#memoryAgentDir && this.#memoryTaskDepth === 0 && !this.#isDisposed) { + const backend = await resolveMemoryBackend(this.settings); + await backend.start({ + session: this, + settings: this.settings, + modelRegistry: this.#modelRegistry, + agentDir: this.#memoryAgentDir, + taskDepth: this.#memoryTaskDepth, + }); + } + if (this.#isDisposed) return; + await this.#refreshMemoryTools(); + if (this.#isDisposed) return; + await this.refreshBaseSystemPrompt(); + } catch (error) { + await this.#disposeMemoryBackendState(false); + if (!this.#isDisposed) { + await this.#replaceMemoryTools([]).catch(refreshError => { + logger.warn("Failed to remove memory tools after backend apply error", { + error: String(refreshError), + }); + }); + } + throw error; + } + } + + async #refreshMemoryTools(): Promise { + const tools = (await this.#createMemoryTools?.()) ?? []; + await this.#replaceMemoryTools(tools); + } + + async #replaceMemoryTools(tools: AgentTool[]): Promise { + const removed = new Set(MEMORY_BACKEND_TOOL_NAMES.filter(name => this.#builtInToolNames.has(name))); + const nextActive = this.getEnabledToolNames().filter(name => !removed.has(name)); + for (const name of removed) { + this.#toolRegistry.delete(name); + this.#builtInToolNames.delete(name); + } + + for (const tool of tools) { + if (!MEMORY_BACKEND_TOOL_NAMES.some(name => name === tool.name) || this.#toolRegistry.has(tool.name)) { + continue; + } + const wrapped = this.#wrapRuntimeTool(tool); + this.#toolRegistry.set(wrapped.name, wrapped); + this.#builtInToolNames.add(wrapped.name); + nextActive.push(wrapped.name); + } + await this.#applyActiveToolsByName([...new Set(nextActive)]); + } + /** Rebuild the base system prompt using the current active tool set. */ async refreshBaseSystemPrompt(): Promise { - if (!this.#rebuildSystemPrompt) return; + if (this.#isDisposed || !this.#rebuildSystemPrompt) return; const activeToolNames = this.getActiveToolNames(); this.#setActiveToolNames?.(activeToolNames); const previousBaseSystemPrompt = this.#baseSystemPrompt; const built = await this.#rebuildSystemPrompt(activeToolNames, this.#toolRegistry); + if (this.#isDisposed) return; this.#baseSystemPrompt = built.systemPrompt; this.#baseSystemPromptBeforeMemoryPromotion = undefined; if ( @@ -9194,6 +9321,14 @@ export class AgentSession { } } + // A prompt issued while the session is already disposing must still run: + // the dispose-driven abort settles its turn (see "does not auto-retry + // empty reasonless aborts once the session is disposing"). Only drop the + // prompt when disposal began during the backend-transition await, where + // resuming would start a turn on a torn-down session. + const disposingBeforeTransition = this.#isDisposed; + await this.#memoryBackendTransition; + if ((this.#isDisposed && !disposingBeforeTransition) || this.#promptGeneration !== generation) return; const beforeAgentStartSystemPrompt = await this.#buildSystemPromptForAgentStart(expandedText); // Emit before_agent_start extension event @@ -11227,11 +11362,7 @@ export class AgentSession { compactionCandidates = this.#getCompactionModelCandidates(availableModels); } const pathEntries = this.sessionManager.getBranch(); - const preparation = prepareCompaction( - pathEntries, - effectiveSettings, - await this.#runnableCompactionCandidates(compactionCandidates, this.sessionId), - ); + const preparation = prepareCompaction(pathEntries, effectiveSettings, this.model); if (!preparation) { // Check why we can't compact const lastEntry = pathEntries[pathEntries.length - 1]; @@ -11284,6 +11415,11 @@ export class AgentSession { let snapcompactReady = wantsSnapcompact; const snapcompactShapeSetting = this.settings.get("snapcompact.shape"); let snapcompactShape: snapcompact.Shape | undefined; + // Claude refuses inputs that reproduce its own reasoning as text + // ("reasoning_extraction"), and the snapcompact archive is replayed as + // text into every later request; drop `¶think:` sections for + // Anthropic-dialect targets (issue #6093). + const snapcompactIncludeThinking = preferredDialect(this.model.id) !== "anthropic"; if (wantsSnapcompact && !this.model.input.includes("image")) { if (explicitSnapcompact) { this.emitNotice( @@ -11302,6 +11438,7 @@ export class AgentSession { } else if (snapcompactReady) { const text = snapcompact.serializeConversation( convertToLlm(preparation.messagesToSummarize.concat(preparation.turnPrefixMessages)), + { includeThinking: snapcompactIncludeThinking }, ); const probeText = snapcompact.renderabilityProbeText( text, @@ -11357,6 +11494,7 @@ export class AgentSession { model: this.model, ...(snapcompactShapeSetting === "auto" ? {} : { shape }), maxFrames, + includeThinking: snapcompactIncludeThinking, }); const framePayloadBytes = this.#snapcompactFramePayloadBytes(snapcompactResult); if (framePayloadBytes > snapcompact.FRAME_DATA_BYTES_BUDGET) { @@ -13583,17 +13721,6 @@ export class AgentSession { return this.#resolveCompactionModelCandidates(this.model, availableModels, filter); } - /** - * Compaction candidates that can actually run — those with a resolvable API - * key, matching the per-candidate getApiKey gate the execution loop applies. - * Re-expansion reusability (prepareCompaction) must judge remote-preserve - * reuse against these, not against candidates the loop would skip at runtime. - */ - async #runnableCompactionCandidates(candidates: readonly Model[], sessionId: string | undefined): Promise { - const keys = await Promise.all(candidates.map(model => this.#modelRegistry.getApiKey(model, sessionId))); - return candidates.filter((_, index) => keys[index] !== undefined); - } - #resolveCompactionModelCandidates( preferredModel: Model | null | undefined, availableModels: Model[], @@ -14406,12 +14533,8 @@ export class AgentSession { const pathEntries = this.sessionManager.getBranch(); - const autoCompactionCandidates = await this.#runnableCompactionCandidates( - this.#getCompactionModelCandidates(availableModels), - this.sessionId, - ); let pathEntriesForCompaction = pathEntries; - let preparation = prepareCompaction(pathEntriesForCompaction, compactionSettings, autoCompactionCandidates); + let preparation = prepareCompaction(pathEntriesForCompaction, compactionSettings, this.model); if (!preparation) { // prepareCompaction found nothing to summarize because the kept region // is a single oversized recent turn — findCutPoint never cuts inside a @@ -14461,11 +14584,7 @@ export class AgentSession { // branch has been rewritten either way. rescueRewroteHistory = true; pathEntriesForCompaction = this.sessionManager.getBranch(); - preparation = prepareCompaction( - pathEntriesForCompaction, - compactionSettings, - autoCompactionCandidates, - ); + preparation = prepareCompaction(pathEntriesForCompaction, compactionSettings, this.model); return preparation !== undefined; }, }); @@ -14587,8 +14706,13 @@ export class AgentSession { let snapcompactResult: snapcompact.CompactionResult | undefined; let snapcompactBlocker: string | undefined; if (action === "snapcompact" && compactionPrep.kind !== "fromHook") { + // Drop `¶think:` sections for Anthropic-dialect targets: the archive + // is replayed as text and Claude refuses reproduced reasoning + // ("reasoning_extraction", issue #6093). + const snapcompactIncludeThinking = preferredDialect(this.model.id) !== "anthropic"; const text = snapcompact.serializeConversation( convertToLlm(preparation.messagesToSummarize.concat(preparation.turnPrefixMessages)), + { includeThinking: snapcompactIncludeThinking }, ); const probeText = snapcompact.renderabilityProbeText( text, @@ -14619,6 +14743,7 @@ export class AgentSession { model: this.model, ...(shapeSetting === "auto" ? {} : { shape }), maxFrames, + includeThinking: snapcompactIncludeThinking, }); const framePayloadBytes = this.#snapcompactFramePayloadBytes(snapcompactResult); if (framePayloadBytes > snapcompact.FRAME_DATA_BYTES_BUDGET) { @@ -15266,17 +15391,13 @@ export class AgentSession { } /** - * Resume a stalled Cursor turn after every server-executed tool has produced - * a result. The failed assistant/tool-result pair must stay in context: it - * records completed side effects and lets the next request continue from - * them instead of replaying the original turn. + * Resume a stalled turn after every emitted tool call has produced a result. + * Cursor calls must also carry the server-execution marker. The failed + * assistant/tool-result pair stays in context so completed side effects are + * continued from rather than replayed. */ - #canResumeCursorStreamStall(message: AssistantMessage): boolean { - if ( - message.provider !== "cursor" || - message.stopReason !== "error" || - !message.errorMessage?.toLowerCase().includes("stream stall") - ) { + #canResumeResolvedStreamStall(message: AssistantMessage): boolean { + if (message.stopReason !== "error" || !message.errorMessage?.toLowerCase().includes("stream stall")) { return false; } const id = this.#classifyRetryMessage(message); @@ -15285,7 +15406,12 @@ export class AgentSession { const resolvedToolCallIds: string[] = []; for (const block of message.content) { if (block.type !== "toolCall") continue; - if (!(kCursorExecResolved in block) || block[kCursorExecResolved] !== true) return false; + if ( + message.provider === "cursor" && + (!(kCursorExecResolved in block) || block[kCursorExecResolved] !== true) + ) { + return false; + } resolvedToolCallIds.push(block.id); } if (resolvedToolCallIds.length === 0) return false; @@ -16131,8 +16257,8 @@ export class AgentSession { errorId: message.errorId, }); - // Cursor exec-channel tools have already run and emitted results. Keep that - // failed turn intact so continuation cannot repeat their side effects. + // Resolved stream-stall tools have already emitted results. Keep that failed + // turn intact so continuation cannot repeat their side effects. if (!options?.preserveFailedTurn) { this.#removeAssistantMessageFromActiveContext(message, "auto-retry"); } diff --git a/packages/coding-agent/src/session/artifacts.ts b/packages/coding-agent/src/session/artifacts.ts index 0cd61d9a7..a7e9b661f 100644 --- a/packages/coding-agent/src/session/artifacts.ts +++ b/packages/coding-agent/src/session/artifacts.ts @@ -39,7 +39,7 @@ export class ArtifactManager { #nextId = 0; readonly #dir: string; #dirCreated = false; - #initialized = false; + #initPromise: Promise | null = null; /** * @param dir Directory that will hold artifact files. Created lazily on first save. @@ -61,10 +61,11 @@ export class ArtifactManager { await fs.mkdir(this.#dir, { recursive: true }); this.#dirCreated = true; } - if (!this.#initialized) { - await this.#scanExistingIds(); - this.#initialized = true; - } + // Memoize the first-use scan so it runs exactly once. Concurrent callers + // share the in-flight promise instead of each re-seeding #nextId across + // the readdir yield in #scanExistingIds (which would hand duplicate ids). + this.#initPromise ??= this.#scanExistingIds(); + await this.#initPromise; } /** diff --git a/packages/coding-agent/src/session/blob-store.test.ts b/packages/coding-agent/src/session/blob-store.test.ts new file mode 100644 index 000000000..78109c36e --- /dev/null +++ b/packages/coding-agent/src/session/blob-store.test.ts @@ -0,0 +1,56 @@ +import { afterAll, describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { BlobStore, parseBlobRef, resolveImageData, resolveImageDataSync, resolveImageDataUrl } from "./blob-store"; + +const base = fs.mkdtempSync(path.join(os.tmpdir(), "blob-store-test-")); +const blobDir = path.join(base, "agent", "blobs", "data"); +fs.mkdirSync(blobDir, { recursive: true }); +fs.writeFileSync(path.join(base, "secret.txt"), "TOP-SECRET-CONTENTS"); +const store = new BlobStore(blobDir); + +afterAll(() => { + fs.rmSync(base, { recursive: true, force: true }); +}); + +describe("parseBlobRef validation", () => { + it("accepts a canonical 64-char lowercase hex suffix", () => { + const hash = "a".repeat(64); + expect(parseBlobRef(`blob:sha256:${hash}`)).toBe(hash); + }); + + it("returns null for non-blob strings", () => { + expect(parseBlobRef("data:image/png;base64,AAAA")).toBeNull(); + }); + + it.each([ + "../../../secret.txt", + `${"../".repeat(6)}etc/passwd`, + "A".repeat(64), // uppercase hex is not the canonical shape + "a".repeat(63), // too short + "a".repeat(65), // too long + "", // empty + ])("rejects malformed suffix %p", suffix => { + expect(parseBlobRef(`blob:sha256:${suffix}`)).toBeNull(); + }); +}); + +describe("blob resolution path confinement", () => { + const traversalRef = "blob:sha256:../../../secret.txt"; + + it("leaves a traversal ref unresolved instead of reading outside the blob dir (base64 path)", async () => { + expect(await resolveImageData(store, traversalRef)).toBe(traversalRef); + expect(resolveImageDataSync(store, traversalRef)).toBe(traversalRef); + }); + + it("leaves a traversal ref unresolved instead of reading outside the blob dir (data-url path)", async () => { + expect(await resolveImageDataUrl(store, traversalRef)).toBe(traversalRef); + }); + + it("still resolves a valid stored blob", async () => { + const put = store.putSync(Buffer.from("hello")); + expect(Buffer.from(resolveImageDataSync(store, put.ref), "base64").toString("utf8")).toBe("hello"); + expect(Buffer.from(await resolveImageData(store, put.ref), "base64").toString("utf8")).toBe("hello"); + }); +}); diff --git a/packages/coding-agent/src/session/blob-store.ts b/packages/coding-agent/src/session/blob-store.ts index 5eaa3e784..7d66cea61 100644 --- a/packages/coding-agent/src/session/blob-store.ts +++ b/packages/coding-agent/src/session/blob-store.ts @@ -5,6 +5,9 @@ import { isEnoent, logger } from "@oh-my-pi/pi-utils"; const BLOB_PREFIX = "blob:sha256:"; +/** Canonical blob hash shape: exactly 64 lowercase hex chars (a SHA-256 digest). */ +export const BLOB_HASH_RE = /^[a-f0-9]{64}$/; + export interface BlobPutOptions { /** Optional file extension for a sidecar hardlink/copy that OS openers can type-detect. */ extension?: string; @@ -179,10 +182,23 @@ export function isBlobRef(data: string): boolean { return data.startsWith(BLOB_PREFIX); } -/** Extract the SHA-256 hash from a blob reference string. */ +/** + * Extract the SHA-256 hash from a blob reference string. + * + * Returns null when the string is not a blob ref, or when the suffix is not a + * canonical 64-char lowercase hex hash. Rejecting non-hash suffixes here is the + * single choke point that keeps every resolution path confined to the blob dir: + * `get`/`getSync` feed this value into `path.join(this.dir, hash)`, so an + * unvalidated `../` suffix would otherwise escape the store and read arbitrary files. + */ export function parseBlobRef(data: string): string | null { if (!data.startsWith(BLOB_PREFIX)) return null; - return data.slice(BLOB_PREFIX.length); + const hash = data.slice(BLOB_PREFIX.length); + if (!BLOB_HASH_RE.test(hash)) { + logger.warn("Rejected malformed blob reference", { suffix: hash }); + return null; + } + return hash; } /** Identify provider transport image data URLs so persistence can externalize and restore them losslessly. */ diff --git a/packages/coding-agent/src/tools/bash-skill-urls.ts b/packages/coding-agent/src/tools/bash-skill-urls.ts index b4de3bd4b..082050b7b 100644 --- a/packages/coding-agent/src/tools/bash-skill-urls.ts +++ b/packages/coding-agent/src/tools/bash-skill-urls.ts @@ -144,6 +144,8 @@ function unquoteToken(token: string): string { function isInsideShellQuote(command: string, index: number): boolean { type ShellQuote = "'" | '"' | undefined; interface CommandSubstitution { + /** `$(` … `)` tracks paren depth; `` ` `` … `` ` `` is a plain toggle. */ + kind: "dollar" | "backtick"; outerQuote: ShellQuote; depth: number; } @@ -152,6 +154,19 @@ function isInsideShellQuote(command: string, index: number): boolean { const substitutions: CommandSubstitution[] = []; for (let i = 0; i < index; i++) { const char = command[i]; + // Inside a backtick substitution nested in double quotes, bash treats `\"` + // as a quote delimiter for the inner command, not as an escaped literal. + if ( + char === "\\" && + command[i + 1] === '"' && + quote !== "'" && + substitutions.at(-1)?.kind === "backtick" && + substitutions.at(-1)?.outerQuote === '"' + ) { + quote = quote === '"' ? undefined : '"'; + i++; + continue; + } if (char === "\\" && quote !== "'") { i++; continue; @@ -165,15 +180,26 @@ function isInsideShellQuote(command: string, index: number): boolean { continue; } if (char === "$" && command[i + 1] === "(" && quote !== "'") { - substitutions.push({ outerQuote: quote, depth: 1 }); + substitutions.push({ kind: "dollar", outerQuote: quote, depth: 1 }); quote = undefined; i++; continue; } + if (char === "`" && quote !== "'") { + const top = substitutions.at(-1); + if (top?.kind === "backtick") { + substitutions.pop(); + quote = top.outerQuote; + } else { + substitutions.push({ kind: "backtick", outerQuote: quote, depth: 0 }); + quote = undefined; + } + continue; + } if (quote !== undefined) continue; const substitution = substitutions.at(-1); - if (!substitution) continue; + if (substitution?.kind !== "dollar") continue; if (char === "(") { substitution.depth++; } else if (char === ")") { diff --git a/packages/coding-agent/src/tools/browser.ts b/packages/coding-agent/src/tools/browser.ts index 362d24cd6..5fbd1f45c 100644 --- a/packages/coding-agent/src/tools/browser.ts +++ b/packages/coding-agent/src/tools/browser.ts @@ -7,9 +7,24 @@ import type { ToolSession } from "../sdk"; import { enforceInlineByteCap } from "../session/streaming-output"; import { truncateForPrompt } from "./approval"; import { resolveCmuxKind } from "./browser/cmux/rpc"; -import { acquireBrowser, type BrowserHandle, type BrowserKind, type BrowserKindTag } from "./browser/registry"; +import { + acquireBrowser, + type BrowserHandle, + type BrowserKind, + type BrowserKindTag, + holdBrowser, + releaseBrowser, +} from "./browser/registry"; import type { Observation, ScreenshotResult } from "./browser/tab-protocol"; -import { acquireTab, dropHeadlessTabs, getTab, releaseAllTabs, releaseTab, runInTab } from "./browser/tab-supervisor"; +import { + type AcquireTabResult, + acquireTab, + dropHeadlessTabs, + getTab, + releaseAllTabs, + releaseTab, + runInTab, +} from "./browser/tab-supervisor"; import type { OutputMeta } from "./output-meta"; import { resolveToCwd } from "./path-utils"; import { ToolAbortError, ToolError, throwIfAborted } from "./tool-errors"; @@ -232,52 +247,84 @@ export class BrowserTool implements AgentTool - acquireBrowser(kind, { - cwd: this.session.cwd, - viewport: params.viewport - ? { - width: params.viewport.width, - height: params.viewport.height, - deviceScaleFactor: params.viewport.scale, - } - : undefined, - appArgs: params.app?.args, - signal, - }), - ); + // The requested timeout must cover the *entire* open — browser + // acquisition (CDP discovery/connect), queued tab acquisition, worker + // creation, and navigation — not only `acquireTab`. Compose one deadline + // from the caller signal and `params.timeout` and thread it through both + // stages so a stalled acquisition rejects at the requested boundary. + const timeoutSignal = AbortSignal.timeout(timeoutMs); + const openSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; + try { + const browser = await untilAborted(openSignal, () => + acquireBrowser(kind, { + cwd: this.session.cwd, + viewport: params.viewport + ? { + width: params.viewport.width, + height: params.viewport.height, + deviceScaleFactor: params.viewport.scale, + } + : undefined, + appArgs: params.app?.args, + signal: openSignal, + }), + ); - const result = await untilAborted(signal, () => - acquireTab(name, browser, { - url: params.url, - waitUntil: params.wait_until, - viewport: params.viewport - ? { - width: params.viewport.width, - height: params.viewport.height, - deviceScaleFactor: params.viewport.scale, - } - : undefined, - target: params.app?.target, - timeoutMs, - dialogs: params.dialogs, - signal, - ownerSessionId: this.session.getSessionId?.() ?? undefined, - }), - ); - const tab = result.tab; - const url = tab.info.url; - const title = tab.info.title ?? ""; - details.url = url; - details.viewport = tab.info.viewport; - const verb = result.created ? "Opened" : "Reused"; - const lines = [ - `${verb} tab ${JSON.stringify(name)} on ${describeBrowser(browser)}`, - `URL: ${url}`, - title ? `Title: ${title}` : null, - ].filter((l): l is string => typeof l === "string"); - details.result = lines.join("\n"); - return toolResult(details).text(lines.join("\n")).done(); + // Hold one open-acquisition lease across the whole tab acquisition. + // A freshly-created browser sits in the registry at refCount 0 until a + // tab takes a hold; without this lease an abort/timeout mid-acquisition + // (or a sibling open of a different tab name on the same browser that + // fails) could dispose it out from under this operation. The lease is + // released exactly once — the success and failure paths are mutually + // exclusive — transferring ownership to the published tab on success or + // rolling the fresh browser back on failure. + holdBrowser(browser); + let result: AcquireTabResult; + try { + result = await untilAborted(openSignal, () => + acquireTab(name, browser, { + url: params.url, + waitUntil: params.wait_until, + viewport: params.viewport + ? { + width: params.viewport.width, + height: params.viewport.height, + deviceScaleFactor: params.viewport.scale, + } + : undefined, + target: params.app?.target, + timeoutMs, + dialogs: params.dialogs, + signal: openSignal, + ownerSessionId: this.session.getSessionId?.() ?? undefined, + }), + ); + } catch (error) { + await releaseBrowser(browser, { kill: false }); + throw error; + } + await releaseBrowser(browser, { kill: false }); + + const tab = result.tab; + const url = tab.info.url; + const title = tab.info.title ?? ""; + details.url = url; + details.viewport = tab.info.viewport; + const verb = result.created ? "Opened" : "Reused"; + const lines = [ + `${verb} tab ${JSON.stringify(name)} on ${describeBrowser(browser)}`, + `URL: ${url}`, + title ? `Title: ${title}` : null, + ].filter((l): l is string => typeof l === "string"); + details.result = lines.join("\n"); + return toolResult(details).text(lines.join("\n")).done(); + } catch (error) { + // Caller cancellation stays a ToolAbortError; the requested timeout + // becomes a timeout ToolError; anything else passes through unchanged. + if (signal?.aborted) throw error instanceof ToolAbortError ? error : new ToolAbortError(); + if (timeoutSignal.aborted) throw new ToolError(`Browser open timed out after ${timeoutMs}ms`); + throw error; + } } async #close( diff --git a/packages/coding-agent/src/tools/browser/tab-supervisor.ts b/packages/coding-agent/src/tools/browser/tab-supervisor.ts index 3a1cc875e..dd9ff45af 100644 --- a/packages/coding-agent/src/tools/browser/tab-supervisor.ts +++ b/packages/coding-agent/src/tools/browser/tab-supervisor.ts @@ -286,13 +286,16 @@ async function acquireTabImpl( } } - // If the caller aborted while we were spawning/initializing the worker, - // tear the freshly-built worker down before publishing the tab so the - // browser refCount (which `holdBrowser` below would take) never grows for - // a tab nobody is waiting for. + // If the caller aborted while we were spawning/initializing the worker, tear + // the freshly-built worker down before publishing the tab so the browser + // refCount (which `holdBrowser` below would take) never grows for a tab + // nobody is waiting for. Mirror the error paths' `refCount === 0` release so + // a fresh browser held by nothing but this aborted open is not orphaned in + // the registry; a browser still leased/held elsewhere (refCount > 0) is left + // for its owner to release. if (opts.signal?.aborted) { await worker.terminate().catch(() => undefined); - if (tempHold) await releaseBrowser(browser, { kill: false }).catch(() => undefined); + if (tempHold || browser.refCount === 0) await releaseBrowser(browser, { kill: false }).catch(() => undefined); throw new ToolAbortError("Browser tab open aborted"); } diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index 47ecadee6..fccacafa1 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -518,6 +518,9 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P if (!requestedTools.includes(name)) requestedTools.push(name); } } + if (session.settings.get("memory.backend") === "mnemopi" && !requestedTools.includes("memory_edit")) { + requestedTools.push("memory_edit"); + } // Auto-learn tools are gated by `autolearn.enabled` but, like the memory // tools above, must also be force-included into an explicit requestedTools // list so a restricted top-level session whose controller/guidance is @@ -561,6 +564,7 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P if (name === "retain" || name === "recall" || name === "reflect") { return ["hindsight", "mnemopi"].includes(session.settings.get("memory.backend") ?? ""); } + if (name === "memory_edit") return session.settings.get("memory.backend") === "mnemopi"; if (name === "manage_skill") return session.settings.get("autolearn.enabled") && (session.taskDepth ?? 0) === 0; if (name === "learn") { return ( diff --git a/packages/coding-agent/src/tools/path-utils.ts b/packages/coding-agent/src/tools/path-utils.ts index 2d74bf553..747f0492d 100644 --- a/packages/coding-agent/src/tools/path-utils.ts +++ b/packages/coding-agent/src/tools/path-utils.ts @@ -2,10 +2,11 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; import * as url from "node:url"; -import { isEnoent, isEnotdir, stripWindowsExtendedLengthPathPrefix } from "@oh-my-pi/pi-utils"; +import { glob } from "@oh-my-pi/pi-natives"; +import { isEnoent, isEnotdir, stripWindowsExtendedLengthPathPrefix, untilAborted } from "@oh-my-pi/pi-utils"; import type { Skill } from "../extensibility/skills"; import { InternalUrlRouter, type LocalProtocolOptions } from "../internal-urls"; -import { ToolError } from "./tool-errors"; +import { ToolAbortError, ToolError } from "./tool-errors"; const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g; // A single line-range chunk: `N`, `N-M`, `N+K`, or open-ended `N-`. `..` is @@ -1132,6 +1133,52 @@ export function resolveReadPath(filePath: string, cwd: string): string { return resolved; } +const WORKSPACE_SUFFIX_TIMEOUT_MS = 5000; + +function escapeGlobMetachars(value: string): string { + return value.replace(/[*?[{]/g, "[$&]"); +} + +/** + * Find a unique workspace entry whose trailing path matches a missing authored path. + * Returns `null` for no match, ambiguity, timeout, or scan failure. + */ +export async function findUniqueWorkspaceSuffix( + rawPath: string, + cwd: string, + signal?: AbortSignal, +): Promise<{ absolutePath: string; displayPath: string } | null> { + const normalized = rawPath.replace(/\\/g, "/").replace(/^\.\//, "").replace(/\/+$/, ""); + if (!normalized) return null; + + const timeoutSignal = AbortSignal.timeout(WORKSPACE_SUFFIX_TIMEOUT_MS); + const combinedSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; + + let matches: string[]; + try { + const result = await untilAborted(combinedSignal, () => + glob({ + pattern: `**/${escapeGlobMetachars(normalized)}`, + path: cwd, + hidden: true, + }), + ); + matches = result.matches.map(match => match.path); + } catch (error) { + if (error instanceof Error && error.name === "AbortError") { + if (!signal?.aborted) return null; + throw new ToolAbortError(); + } + return null; + } + + if (matches.length !== 1) return null; + return { + absolutePath: path.resolve(cwd, matches[0]), + displayPath: matches[0], + }; +} + // ============================================================================= // Tool-scope resolution (search/ast tools) // ============================================================================= diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 6519602c7..8772f57f8 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -11,12 +11,14 @@ import type { ToolTier, } from "@oh-my-pi/pi-agent-core"; import type { ImageContent, TextContent } from "@oh-my-pi/pi-ai"; -import { glob, type SummaryResult, summarizeCode } from "@oh-my-pi/pi-natives"; +import { type SummaryResult, summarizeCode } from "@oh-my-pi/pi-natives"; import type { Component } from "@oh-my-pi/pi-tui"; import { Text } from "@oh-my-pi/pi-tui"; import { getRemoteDir, type ImageMetadata, + isEexist, + isEnotempty, isProbablyBinary, logger, prompt, @@ -94,6 +96,7 @@ import { } from "./output-meta"; import { expandPath, + findUniqueWorkspaceSuffix, formatPathRelativeToCwd, isReadableUrlPath, type LineRange, @@ -640,7 +643,6 @@ async function streamLinesFromFile( // Maximum image file size (20MB) - larger images will be rejected to prevent OOM during serialization const MAX_IMAGE_SIZE = MAX_IMAGE_INPUT_BYTES; -const GLOB_TIMEOUT_MS = 5000; function isNotFoundError(error: unknown): boolean { if (!error || typeof error !== "object") return false; @@ -648,61 +650,6 @@ function isNotFoundError(error: unknown): boolean { return code === "ENOENT" || code === "ENOTDIR"; } -/** - * Escape glob metacharacters so a literal path (e.g. `foo[1].ts`) interpolated - * into a suffix-glob pattern matches itself. Each metachar is wrapped in a - * character class (the native glob engine rewrites `\` to `/`, so backslash - * escaping is unavailable). `]`/`}` need no escaping once their openers are - * neutralized — unmatched closers are literal. - */ -function escapeGlobMetachars(value: string): string { - return value.replace(/[*?[{]/g, "[$&]"); -} - -/** - * Attempt to resolve a non-existent path by finding a unique suffix match within the workspace. - * Uses a glob suffix pattern so the native engine handles matching directly. - * Returns null when 0 or >1 candidates match (ambiguous = no auto-resolution). - */ -async function findUniqueSuffixMatch( - rawPath: string, - cwd: string, - signal?: AbortSignal, -): Promise<{ absolutePath: string; displayPath: string } | null> { - const normalized = rawPath.replace(/\\/g, "/").replace(/^\.\//, "").replace(/\/+$/, ""); - if (!normalized) return null; - const pattern = `**/${escapeGlobMetachars(normalized)}`; - - const timeoutSignal = AbortSignal.timeout(GLOB_TIMEOUT_MS); - const combinedSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; - - let matches: string[]; - try { - const result = await untilAborted(combinedSignal, () => - glob({ - pattern, - path: cwd, - // No fileType filter: matches both files and directories - hidden: true, - }), - ); - matches = result.matches.map(m => m.path); - } catch (error) { - if (error instanceof Error && error.name === "AbortError") { - if (!signal?.aborted) return null; // timeout — give up silently - throw new ToolAbortError(); - } - return null; - } - - if (matches.length !== 1) return null; - - return { - absolutePath: path.resolve(cwd, matches[0]), - displayPath: matches[0], - }; -} - function decodeUtf8Text(bytes: Uint8Array): string | null { if (bytes.indexOf(0) !== -1) return null; @@ -722,6 +669,22 @@ function prependSuffixResolutionNotice(text: string, suffixResolution?: { from: const PDF_IMAGE_PLACEHOLDER_RE = //g; const PDF_IMAGE_MEMBER_RE = /^(.*\.pdf):(.*)$/i; const PDF_IMAGE_MEMBER_EXTENSION_RE = /\.png$/i; +const PDF_IMAGE_CACHE_BASENAME_MAX_LENGTH = 96; + +interface PdfImageSnapshot { + directory: string; + filePath: string; + digest: string; +} + +interface PdfImageExtraction { + controller: AbortController; + promise: Promise; + settled: boolean; + waiters: number; +} + +const pdfImageExtractions = new Map(); function pdfImageMemberPath(pdfPath: string, imageId: string): string { const member = PDF_IMAGE_MEMBER_EXTENSION_RE.test(imageId) ? imageId : `${imageId}.png`; @@ -991,7 +954,7 @@ export class ReadTool implements AgentTool { } /** - * Memoized {@link findUniqueSuffixMatch} for a single read call. A missing + * Memoized {@link findUniqueWorkspaceSuffix} for a single read call. A missing * path with archive/sqlite extensions probes the workspace once per stage * (archive candidates, sqlite candidates, plain path) — each glob carries a * 5s timeout, so repeated lookups of the same string stack into a long @@ -1004,7 +967,7 @@ export class ReadTool implements AgentTool { ): Promise<{ absolutePath: string; displayPath: string } | null> { const hit = cache.get(rawPath); if (hit !== undefined) return hit; - const result = await findUniqueSuffixMatch(rawPath, this.session.cwd, signal); + const result = await findUniqueWorkspaceSuffix(rawPath, this.session.cwd, signal); cache.set(rawPath, result); return result; } @@ -1106,7 +1069,7 @@ export class ReadTool implements AgentTool { return null; } - #pdfImageCacheDir(absolutePdfPath: string): string { + #pdfImageCacheDir(absolutePdfPath: string, contentDigest: string): string { const artifactsDir = this.session.getArtifactsDir?.(); let root = artifactsDir ?? undefined; if (root === undefined) { @@ -1115,8 +1078,28 @@ export class ReadTool implements AgentTool { ? sessionFile.slice(0, -6) : path.join(os.tmpdir(), "omp-read-pdf-images"); } - const basename = path.basename(absolutePdfPath).replace(/[^A-Za-z0-9._-]/g, "_"); - return path.join(root, "read-pdf-images", `${basename}-${Bun.hash(absolutePdfPath).toString(36)}`); + const basename = path + .basename(absolutePdfPath) + .replace(/[^A-Za-z0-9._-]/g, "_") + .slice(0, PDF_IMAGE_CACHE_BASENAME_MAX_LENGTH); + const pathDigest = Bun.hash(absolutePdfPath).toString(36); + return path.join(root, "read-pdf-images", `${basename}-${pathDigest}-${contentDigest}`); + } + + async #snapshotPdfSource(absolutePdfPath: string, signal?: AbortSignal): Promise { + const directory = await fs.mkdtemp(path.join(os.tmpdir(), "omp-read-pdf-")); + try { + const bytes = await untilAborted(signal, () => Bun.file(absolutePdfPath).bytes()); + signal?.throwIfAborted(); + const digest = new Bun.CryptoHasher("sha256").update(bytes).digest("hex"); + const filePath = path.join(directory, "source.pdf"); + await Bun.write(filePath, bytes); + signal?.throwIfAborted(); + return { directory, filePath, digest }; + } catch (error) { + await fs.rm(directory, { recursive: true, force: true }); + throw error; + } } async #listPdfImageMembers(imageDir: string): Promise { @@ -1133,8 +1116,7 @@ export class ReadTool implements AgentTool { } } - async #ensurePdfImageCache(absolutePdfPath: string, signal?: AbortSignal): Promise { - const imageDir = this.#pdfImageCacheDir(absolutePdfPath); + async #extractPdfImages(snapshot: PdfImageSnapshot, imageDir: string, signal: AbortSignal): Promise { const markerPath = path.join(imageDir, ".extracted"); try { await fs.stat(markerPath); @@ -1143,15 +1125,74 @@ export class ReadTool implements AgentTool { if (!isNotFoundError(error)) throw error; } - await fs.rm(imageDir, { recursive: true, force: true }); - await fs.mkdir(imageDir, { recursive: true }); - const result = await convertFileWithMarkit(absolutePdfPath, signal, { imageDir }); - if (!result.ok) { - await fs.rm(imageDir, { recursive: true, force: true }); - throw new ToolError(`Cannot extract images from PDF: ${result.error ?? "conversion failed"}`); + await fs.mkdir(path.dirname(imageDir), { recursive: true }); + const stagingDir = await fs.mkdtemp(`${imageDir}.tmp-`); + let published = false; + try { + const result = await convertFileWithMarkit(snapshot.filePath, signal, { imageDir: stagingDir }); + if (!result.ok) { + throw new ToolError(`Cannot extract images from PDF: ${result.error ?? "conversion failed"}`); + } + await Bun.write(path.join(stagingDir, ".extracted"), "ok"); + try { + await fs.rename(stagingDir, imageDir); + published = true; + } catch (error) { + if (!isEexist(error) && !isEnotempty(error)) throw error; + try { + await fs.stat(markerPath); + } catch (markerError) { + if (isNotFoundError(markerError)) throw error; + throw markerError; + } + } + return imageDir; + } finally { + if (!published) await fs.rm(stagingDir, { recursive: true, force: true }); } - await Bun.write(markerPath, "ok"); - return imageDir; + } + + #createPdfImageExtraction(snapshot: PdfImageSnapshot, imageDir: string): PdfImageExtraction { + const controller = new AbortController(); + const promise = this.#extractPdfImages(snapshot, imageDir, controller.signal).finally(() => + fs.rm(snapshot.directory, { recursive: true, force: true }), + ); + const extraction: PdfImageExtraction = { controller, promise, settled: false, waiters: 0 }; + const settle = () => { + extraction.settled = true; + if (pdfImageExtractions.get(imageDir) === extraction) pdfImageExtractions.delete(imageDir); + }; + void promise.then(settle, settle); + return extraction; + } + + async #waitForPdfImageExtraction(extraction: PdfImageExtraction, signal: AbortSignal | undefined): Promise { + extraction.waiters++; + try { + return await untilAborted(signal, extraction.promise); + } finally { + extraction.waiters--; + if (extraction.waiters === 0 && !extraction.settled) { + extraction.controller.abort(); + try { + await extraction.promise; + } catch {} + } + } + } + + async #ensurePdfImageCache(absolutePdfPath: string, signal?: AbortSignal): Promise { + const snapshot = await this.#snapshotPdfSource(absolutePdfPath, signal); + const imageDir = this.#pdfImageCacheDir(absolutePdfPath, snapshot.digest); + const existing = pdfImageExtractions.get(imageDir); + if (existing && !existing.settled && !existing.controller.signal.aborted) { + await fs.rm(snapshot.directory, { recursive: true, force: true }); + return this.#waitForPdfImageExtraction(existing, signal); + } + + const extraction = this.#createPdfImageExtraction(snapshot, imageDir); + pdfImageExtractions.set(imageDir, extraction); + return this.#waitForPdfImageExtraction(extraction, signal); } async #readPdfImageMember( diff --git a/packages/coding-agent/src/tools/renderers.ts b/packages/coding-agent/src/tools/renderers.ts index 895718bc2..b45b717b9 100644 --- a/packages/coding-agent/src/tools/renderers.ts +++ b/packages/coding-agent/src/tools/renderers.ts @@ -90,7 +90,13 @@ export const toolRenderers: Record = { grep: grepToolRenderer as ToolRenderer, lsp: lspToolRenderer as ToolRenderer, inspect_image: inspectImageToolRenderer as ToolRenderer, - hub: hubToolRenderer as ToolRenderer, + // Lazy getter: `hubToolRenderer` lives in a module whose deps (messaging → + // persisted-agents → vibe/runtime → task/executor → sdk) close an import + // cycle back here, so reading it at init order-dependently hits its + // temporal dead zone. Deferring the read to first access sidesteps it. + get hub(): ToolRenderer { + return hubToolRenderer as ToolRenderer; + }, read: readToolRenderer as ToolRenderer, // Keyed by xd:// resolution-device names: the write dispatch delegates here // by dispatch tool, and historical `resolve` tool transcripts still render diff --git a/packages/coding-agent/src/tts/streaming-player.ts b/packages/coding-agent/src/tts/streaming-player.ts index 219d55bdc..43373e25d 100644 --- a/packages/coding-agent/src/tts/streaming-player.ts +++ b/packages/coding-agent/src/tts/streaming-player.ts @@ -31,6 +31,15 @@ const DEFAULT_SAMPLE_RATE = 24_000; const LEAD_SECONDS = 0.6; /** Output gain applied while ducked (the user is speaking over the assistant). */ export const DUCK_GAIN = 0.25; +/** + * Cap on streamed PCM retained for the nonzero-exit replay. Past this the + * buffer is dropped: the failure being recovered is a short clip that fits the + * OS pipe buffer before a broken backend dies, while a backend that consumed + * minutes of realtime-paced audio was playing it — replaying a whole long + * utterance would duplicate audio, and unbounded retention (~5.8 MB/min at + * 24 kHz mono f32) would defeat streaming for long input. + */ +const REPLAY_RETENTION_SECONDS = 60; /** Injection seam for {@link streamingPlayerCommandsFor} — defaults to real PATH/tools lookups. */ export interface StreamingPlayerLookup { @@ -89,6 +98,20 @@ export function streamingPlayerCommandsFor( return commands; } +/** + * Test seams for {@link StreamingAudioPlayer}: override backend discovery and + * the per-file fallback so playback logic can be exercised without a real audio + * device. Both default to the platform lookup and {@link playAudioFile}. + */ +export interface StreamingPlayerOptions { + /** Ordered backend commands for a sample rate; defaults to {@link streamingPlayerCommandsFor}. */ + commandsFor?: (sampleRate: number) => PlayerCommand[]; + /** Per-file fallback playback; defaults to {@link playAudioFile}. */ + playAudio?: (wavPath: string, signal: AbortSignal) => Promise; + /** Max seconds of streamed PCM retained for the nonzero-exit replay; defaults to {@link REPLAY_RETENTION_SECONDS}. */ + replayRetentionSeconds?: number; +} + /** * Single-session gapless player. Lifecycle: {@link start} once, {@link write} * chunks in order, then {@link end} to drain or {@link stop} to abort. Not @@ -111,6 +134,18 @@ export class StreamingAudioPlayer { #abortController = new AbortController(); #wake: (() => void) | null = null; #drain: Promise = Promise.resolve(); + readonly #commandsFor: (sampleRate: number) => PlayerCommand[]; + readonly #playAudio: (wavPath: string, signal: AbortSignal) => Promise; + readonly #replayRetentionSec: number; + /** Streamed PCM retained for this utterance so a failed backend can be replayed via file playback. */ + #played: Float32Array[] = []; + #playedSec = 0; + + constructor(options: StreamingPlayerOptions = {}) { + this.#commandsFor = options.commandsFor ?? (rate => streamingPlayerCommandsFor(process.platform, rate)); + this.#playAudio = options.playAudio ?? ((wavPath, signal) => playAudioFile(wavPath, { signal })); + this.#replayRetentionSec = options.replayRetentionSeconds ?? REPLAY_RETENTION_SECONDS; + } /** Pick a backend and begin draining. Idempotent; the first call's rate wins. */ start(sampleRate: number): void { @@ -146,6 +181,7 @@ export class StreamingAudioPlayer { if (this.#stopped) return; this.#stopped = true; this.#queue.length = 0; + this.#played.length = 0; this.#abortController.abort(); this.#signal(); try { @@ -168,7 +204,7 @@ export class StreamingAudioPlayer { * in-flight chunk. */ #spawnStream(): boolean { - this.#candidates ??= streamingPlayerCommandsFor(process.platform, this.#sampleRate); + this.#candidates ??= this.#commandsFor(this.#sampleRate); for (let command = this.#candidates.shift(); command; command = this.#candidates.shift()) { const { cmd, args } = command; try { @@ -213,6 +249,12 @@ export class StreamingAudioPlayer { continue; } if (this.#mode === "stream") { + if (this.#playedSec <= this.#replayRetentionSec) { + this.#played.push(chunk); + this.#playedSec += chunk.length / this.#sampleRate; + // Over the cap: drop retention for the rest of the utterance. + if (this.#playedSec > this.#replayRetentionSec) this.#played.length = 0; + } // Pace writes so the player buffers ~LEAD_SECONDS, no more, keeping // ducking and stop responsive instead of locked behind buffered audio. const ahead = this.#writtenSec - (performance.now() - this.#startedAt) / 1000; @@ -242,11 +284,27 @@ export class StreamingAudioPlayer { try { await this.#sink?.end(); } catch {} - if (this.#proc) { + const proc = this.#proc; + let exitCode: number | null = null; + if (proc) { try { - await this.#proc.exited; + exitCode = await proc.exited; } catch {} } + // A streaming backend that exits nonzero never opened its audio + // device (e.g. the bundled ffmpeg built without pulse/alsa output). + // For a short single-segment clip the pipe write succeeds before + // that death and #inputClosed is already set, so neither the + // broken-pipe replay nor the early-exit handler advances backends. + // Replay the buffered utterance through per-file playback so it + // still reaches the speakers. + if (!this.#stopped && proc && exitCode !== 0) { + this.#mode = "file"; + for (const chunk of this.#played) { + if (this.#stopped) break; + await this.#playFile(chunk); + } + } } } catch (error) { logger.debug("tts: streaming player drain failed", { @@ -291,7 +349,7 @@ export class StreamingAudioPlayer { const wavPath = path.join(os.tmpdir(), `omp-speech-${Snowflake.next()}.wav`); try { await fs.writeFile(wavPath, encodeWav(this.#scaled(pcm), this.#sampleRate)); - if (!this.#stopped) await playAudioFile(wavPath, { signal: this.#abortController.signal }); + if (!this.#stopped) await this.#playAudio(wavPath, this.#abortController.signal); } catch (error) { logger.debug("tts: file playback failed", { error: error instanceof Error ? error.message : String(error), diff --git a/packages/coding-agent/test/acp-builtins.test.ts b/packages/coding-agent/test/acp-builtins.test.ts index 1f5995fe4..627b65b4e 100644 --- a/packages/coding-agent/test/acp-builtins.test.ts +++ b/packages/coding-agent/test/acp-builtins.test.ts @@ -38,6 +38,7 @@ interface FakeAcpBuiltinSession { model: { provider: string; id: string } | undefined; newSession(opts?: { drop?: boolean; parentSession?: string }): Promise; switchSession(sessionPath: string): Promise; + moveSession(newCwd: string, targetSessionDir?: string): Promise; markMovedFromEmptySessionFile(sessionFile: string): void; fork(): Promise; handoff(instr?: string): Promise<{ document: string; savedPath?: string } | undefined>; @@ -121,6 +122,10 @@ function createRuntime() { await fakeSessionManager.setSessionFile(this._switchedTo); return true; }, + async moveSession(newCwd: string, _targetSessionDir?: string) { + if (!fakeSessionManager) throw new Error("fake session manager not initialized"); + await fakeSessionManager.moveTo(newCwd); + }, markMovedFromEmptySessionFile(sessionFile: string) { this._movedFromEmptySessionFile = path.resolve(sessionFile); }, diff --git a/packages/coding-agent/test/agent-session-memory-backend.test.ts b/packages/coding-agent/test/agent-session-memory-backend.test.ts new file mode 100644 index 000000000..9f2050a18 --- /dev/null +++ b/packages/coding-agent/test/agent-session-memory-backend.test.ts @@ -0,0 +1,149 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as path from "node:path"; +import { Agent, type AgentTool } from "@oh-my-pi/pi-agent-core"; +import { createMockModel } from "@oh-my-pi/pi-ai/providers/mock"; +import { buildModel } from "@oh-my-pi/pi-catalog/build"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { getMnemopiSessionState } from "@oh-my-pi/pi-coding-agent/mnemopi/state"; +import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { resetMemoryForTests } from "@oh-my-pi/pi-mnemopi"; +import { TempDir } from "@oh-my-pi/pi-utils"; +import { type } from "arktype"; + +function createTool(name: string): AgentTool { + return { + name, + label: name, + description: `${name} memory tool`, + parameters: type({}), + async execute() { + return { content: [{ type: "text", text: name }] }; + }, + }; +} + +describe("AgentSession memory backend lifecycle", () => { + let authStorage: AuthStorage; + let session: AgentSession | undefined; + let settings: Settings; + let tempDir: TempDir; + + beforeEach(async () => { + tempDir = TempDir.createSync("@memory-backend-lifecycle-"); + authStorage = await AuthStorage.create(path.join(tempDir.path(), "auth.db")); + authStorage.setRuntimeApiKey("anthropic", "test-key"); + settings = Settings.isolated({ + "compaction.enabled": false, + "memory.backend": "off", + "mnemopi.noEmbeddings": true, + "mnemopi.llmMode": "none", + }); + }); + + afterEach(async () => { + await session?.dispose(); + session = undefined; + resetMemoryForTests(); + authStorage.close(); + tempDir.removeSync(); + }); + + function createSession(createMemoryTools: () => Promise): AgentSession { + const model = buildModel({ + id: "mock", + name: "mock", + api: "openai-responses", + provider: "openai", + baseUrl: "https://example.invalid", + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 8192, + maxTokens: 2048, + }); + const read = createTool("read"); + const agent = new Agent({ + getApiKey: () => "test-key", + initialState: { model, systemPrompt: ["initial"], tools: [read] }, + streamFn: createMockModel({ responses: [{ content: ["ok"] }] }).stream, + }); + const toolRegistry = new Map([[read.name, read]]); + session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(tempDir.path()), + settings, + modelRegistry: new ModelRegistry(authStorage, path.join(tempDir.path(), "models.yml")), + memoryAgentDir: tempDir.path(), + memoryTaskDepth: 0, + createMemoryTools, + toolRegistry, + builtInToolNames: [read.name], + rebuildSystemPrompt: async toolNames => ({ + systemPrompt: [`backend:${settings.get("memory.backend")};tools:${toolNames.sort().join(",")}`], + }), + }); + return session; + } + + it("switches runtime state, memory tools, and prompt in one apply", async () => { + const current = createSession(async () => + settings.get("memory.backend") === "mnemopi" ? [createTool("retain"), createTool("memory_edit")] : [], + ); + + settings.override("memory.backend", "mnemopi"); + await current.applyMemoryBackend(); + + expect(getMnemopiSessionState(current)).toBeDefined(); + expect(current.getActiveToolNames()).toEqual(expect.arrayContaining(["read", "retain", "memory_edit"])); + expect(current.systemPrompt).toEqual(["backend:mnemopi;tools:memory_edit,read,retain"]); + + settings.override("memory.backend", "off"); + await current.applyMemoryBackend(); + + expect(getMnemopiSessionState(current)).toBeUndefined(); + expect(current.getActiveToolNames()).toEqual(["read"]); + expect(current.getAllToolNames()).toEqual(["read"]); + expect(current.systemPrompt).toEqual(["backend:off;tools:read"]); + }); + it("cancels a displaced local startup generation", async () => { + const current = createSession(async () => []); + const localStartup = current.beginLocalMemoryStartup(); + + await current.applyMemoryBackend(); + + expect(localStartup.aborted).toBe(true); + }); + + it("serializes concurrent backend applies", async () => { + const firstStarted = Promise.withResolvers(); + const releaseFirst = Promise.withResolvers(); + let calls = 0; + let running = 0; + let maxRunning = 0; + const current = createSession(async () => { + calls++; + running++; + maxRunning = Math.max(maxRunning, running); + if (calls === 1) { + firstStarted.resolve(); + await releaseFirst.promise; + } + running--; + return []; + }); + + const first = current.applyMemoryBackend(); + await firstStarted.promise; + const second = current.applyMemoryBackend(); + await Promise.resolve(); + expect(calls).toBe(1); + releaseFirst.resolve(); + await Promise.all([first, second]); + + expect(maxRunning).toBe(1); + expect(calls).toBe(2); + }); +}); diff --git a/packages/coding-agent/test/agent-session-retry-cap.test.ts b/packages/coding-agent/test/agent-session-retry-cap.test.ts index 95c9e00dd..887e96cd7 100644 --- a/packages/coding-agent/test/agent-session-retry-cap.test.ts +++ b/packages/coding-agent/test/agent-session-retry-cap.test.ts @@ -744,9 +744,11 @@ describe("AgentSession retry delay cap", () => { const retryStartEvents: AutoRetryStartEvent[] = []; const retryEndEvents: AutoRetryEndEvent[] = []; + const agentEndEvents: Array> = []; session.subscribe(event => { if (event.type === "auto_retry_start") retryStartEvents.push(event); if (event.type === "auto_retry_end") retryEndEvents.push(event); + if (event.type === "agent_end") agentEndEvents.push(event); }); await session.prompt("Write a large report"); @@ -756,11 +758,128 @@ describe("AgentSession retry delay cap", () => { expect(retryStartEvents).toHaveLength(0); expect(retryEndEvents).toHaveLength(0); expect(session.agent.state.messages.at(-1)?.role).toBe("toolResult"); - const lastError = [...session.agent.state.messages] + expect(agentEndEvents).toHaveLength(1); + expect(agentEndEvents[0].isTerminal).toBe(true); + const terminalError = [...agentEndEvents[0].messages] .reverse() .find((message): message is AssistantMessage => message.role === "assistant"); - expect(lastError?.stopReason).toBe("error"); - expect(lastError?.errorMessage).toBe("The operation timed out."); + expect(terminalError?.stopReason).toBe("error"); + expect(terminalError?.errorMessage).toBe("The operation timed out."); + }); + + it("resumes an OpenAI-completions stall after a synthetic unexecuted tool result", async () => { + const stallMessage = "OpenAI completions stream stalled while waiting for the next event"; + const model = createMockModel({ + id: "grok-4", + provider: "openrouter", + }); + authStorage.setRuntimeApiKey("openrouter", "openrouter-test-key"); + const toolCall: ToolCall = { + type: "toolCall", + id: "grok-write-1", + name: "write", + arguments: { path: "review.md", content: "partial review" }, + }; + let streamCalls = 0; + let resumedWithSyntheticResult = false; + const agent = new Agent({ + getApiKey: requestedModel => `${requestedModel.provider}-test-key`, + initialState: { + model, + systemPrompt: ["Test"], + tools: [], + messages: [], + }, + streamFn: (_requestedModel, context, options) => { + streamCalls += 1; + if (streamCalls > 1) { + const matchingResult = context.messages.find( + message => message.role === "toolResult" && message.toolCallId === toolCall.id, + ); + resumedWithSyntheticResult = + matchingResult?.role === "toolResult" && + typeof matchingResult.details === "object" && + matchingResult.details !== null && + "executed" in matchingResult.details && + matchingResult.details.executed === false; + model.push({ content: ["Recovered after Grok stall"] }); + return model.stream(model, context, options); + } + + const stream = new AssistantMessageEventStream(); + queueMicrotask(() => { + const partial: AssistantMessage = { + role: "assistant", + content: [toolCall], + api: model.api, + provider: model.provider, + model: model.id, + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: "stop", + timestamp: Date.now(), + }; + stream.push({ type: "start", partial }); + stream.push({ type: "toolcall_start", contentIndex: 0, partial }); + stream.push({ + type: "toolcall_delta", + contentIndex: 0, + delta: JSON.stringify(toolCall.arguments), + partial, + }); + stream.push({ type: "toolcall_end", contentIndex: 0, toolCall, partial }); + stream.push({ + type: "error", + reason: "error", + error: { + ...partial, + stopReason: "error", + errorMessage: stallMessage, + }, + }); + }); + return stream; + }, + }); + + const settings = Settings.isolated({ + "compaction.enabled": false, + "retry.baseDelayMs": 5, + "retry.maxRetries": 1, + }); + settings.setModelRole("default", `${model.provider}/${model.id}`); + session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings, + modelRegistry, + }); + const retryStartEvents: AutoRetryStartEvent[] = []; + const retryEndEvents: AutoRetryEndEvent[] = []; + session.subscribe(event => { + if (event.type === "auto_retry_start") retryStartEvents.push(event); + if (event.type === "auto_retry_end") retryEndEvents.push(event); + }); + + await session.prompt("Write a review"); + await session.waitForIdle(); + + expect(streamCalls).toBe(2); + expect(resumedWithSyntheticResult).toBe(true); + expect( + session.agent.state.messages.filter( + message => message.role === "toolResult" && message.toolCallId === toolCall.id, + ), + ).toHaveLength(1); + expect(retryStartEvents).toHaveLength(1); + expect(retryEndEvents).toContainEqual(expect.objectContaining({ success: true, attempt: 1 })); + expect(lastAssistant(session).content).toContainEqual({ type: "text", text: "Recovered after Grok stall" }); }); it("resumes a stalled Cursor stream after its exec tool result", async () => { diff --git a/packages/coding-agent/test/artifacts-concurrency.test.ts b/packages/coding-agent/test/artifacts-concurrency.test.ts new file mode 100644 index 000000000..db776e530 --- /dev/null +++ b/packages/coding-agent/test/artifacts-concurrency.test.ts @@ -0,0 +1,72 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import * as os from "node:os"; +import * as path from "node:path"; +import { ArtifactManager } from "@oh-my-pi/pi-coding-agent/session/artifacts"; +import { removeSyncWithRetries } from "@oh-my-pi/pi-utils"; + +describe("ArtifactManager concurrent first-use", () => { + const dirs: string[] = []; + + function freshDir(): string { + const dir = path.join(os.tmpdir(), `omp-artifacts-${crypto.randomUUID()}`, "session"); + dirs.push(path.dirname(dir)); + return dir; + } + + afterEach(() => { + for (const dir of dirs.splice(0)) { + removeSyncWithRetries(dir); + } + }); + + // First-use init (dir scan → #nextId seed) must run exactly once. Two callers + // racing a fresh manager both yield inside #scanExistingIds before either + // marks init done; if the second re-seeds #nextId after the first consumed an + // id, both allocate the same numeric id and the second write clobbers the + // first. Same toolType => file overwrite; the first id resolves to B's bytes. + it("hands concurrent same-toolType savers distinct ids that each resolve to their own content", async () => { + const mgr = new ArtifactManager(freshDir()); + const [idA, idB] = await Promise.all([mgr.save("CONTENT-A", "bash"), mgr.save("CONTENT-B", "bash")]); + + expect(idA).not.toBe(idB); + + const pathA = await mgr.getPath(idA); + const pathB = await mgr.getPath(idB); + expect(pathA).not.toBeNull(); + expect(pathB).not.toBeNull(); + expect(await Bun.file(pathA as string).text()).toBe("CONTENT-A"); + expect(await Bun.file(pathB as string).text()).toBe("CONTENT-B"); + }); + + // Different toolTypes turn a duplicate id into two coexisting files + // (`{id}.bash.log` + `{id}.async.log`); getPath's startsWith(`${id}.`) then + // resolves ambiguously in unspecified readdir order. Distinct ids keep each + // artifact:// pointing at the content its caller wrote. + it("hands concurrent different-toolType savers distinct ids that each resolve to their own content", async () => { + const mgr = new ArtifactManager(freshDir()); + const [idA, idB] = await Promise.all([mgr.save("BASH-BYTES", "bash"), mgr.save("ASYNC-BYTES", "async")]); + + expect(idA).not.toBe(idB); + + const pathA = await mgr.getPath(idA); + const pathB = await mgr.getPath(idB); + expect(await Bun.file(pathA as string).text()).toBe("BASH-BYTES"); + expect(await Bun.file(pathB as string).text()).toBe("ASYNC-BYTES"); + }); + + // The race also re-opens on a fresh manager over a directory that already + // holds artifacts (e.g. after a `#artifactManager = null` reset): the scan + // seeds from maxId, and concurrent callers must still get ids past it. + it("does not reuse ids when racing init over a pre-populated directory", async () => { + const dir = freshDir(); + const seed = new ArtifactManager(dir); + await seed.save("OLD", "bash"); + + const mgr = new ArtifactManager(dir); + const [idA, idB] = await Promise.all([mgr.save("NEW-A", "bash"), mgr.save("NEW-B", "bash")]); + + expect(idA).not.toBe(idB); + expect(await Bun.file((await mgr.getPath(idA)) as string).text()).toBe("NEW-A"); + expect(await Bun.file((await mgr.getPath(idB)) as string).text()).toBe("NEW-B"); + }); +}); diff --git a/packages/coding-agent/test/core/python-executor.lifecycle.test.ts b/packages/coding-agent/test/core/python-executor.lifecycle.test.ts index b60e2d9ba..5e52e7058 100644 --- a/packages/coding-agent/test/core/python-executor.lifecycle.test.ts +++ b/packages/coding-agent/test/core/python-executor.lifecycle.test.ts @@ -1,5 +1,9 @@ import { afterEach, describe, expect, it } from "bun:test"; -import { disposeAllKernelSessions, executePython } from "@oh-my-pi/pi-coding-agent/eval/py/executor"; +import { + disposeAllKernelSessions, + disposeKernelSessionsByOwner, + executePython, +} from "@oh-my-pi/pi-coding-agent/eval/py/executor"; import { type KernelExecuteOptions, type KernelExecuteResult, @@ -29,6 +33,10 @@ class FakeKernel { return this.#alive; } + markDead(): void { + this.#alive = false; + } + async execute(code: string, options?: KernelExecuteOptions): Promise { this.executeCalls.push(code); this.#onExecute?.(options); @@ -53,6 +61,10 @@ const okResult: KernelExecuteResult = { stdinRequested: false, }; +async function flushMicrotasks(): Promise { + for (let i = 0; i < 5; i += 1) await Promise.resolve(); +} + describe("executePython session lifecycle", () => { const originalStart = PythonKernel.start; @@ -95,6 +107,215 @@ describe("executePython session lifecycle", () => { expect(liveKernel.executeCalls).toEqual(["print('restart')"]); }); + it("coalesces concurrent replacement of one dead session generation", async () => { + const deadKernel = new FakeKernel(okResult); + const replacementOne = new FakeKernel(okResult); + const replacementTwo = new FakeKernel(okResult); + const replacementStarted = Promise.withResolvers(); + const releaseReplacement = Promise.withResolvers(); + const replacements = [replacementOne, replacementTwo]; + let startCount = 0; + + PythonKernel.start = async () => { + startCount += 1; + if (startCount === 1) return deadKernel as unknown as PythonKernel; + replacementStarted.resolve(); + await releaseReplacement.promise; + return replacements.shift() as unknown as PythonKernel; + }; + + await executePython("print('setup')", { sessionId: "session-concurrent-restart" }); + deadKernel.markDead(); + + const first = executePython("print('first')", { sessionId: "session-concurrent-restart" }); + await replacementStarted.promise; + const second = executePython("print('second')", { sessionId: "session-concurrent-restart" }); + await Promise.resolve(); + await Promise.resolve(); + + expect(startCount).toBe(2); + + releaseReplacement.resolve(); + await Promise.all([first, second]); + + expect(replacementOne.executeCalls).toEqual(["print('first')", "print('second')"]); + expect(replacementTwo.executeCalls).toEqual([]); + + await disposeAllKernelSessions(); + expect(replacementOne.shutdownCalls).toBe(1); + expect(replacementTwo.shutdownCalls).toBe(0); + }); + + it("keeps a shared replacement alive when one caller cancels", async () => { + const deadKernel = new FakeKernel(okResult); + const replacement = new FakeKernel(okResult); + const replacementStarted = Promise.withResolvers(); + const releaseReplacement = Promise.withResolvers(); + let startCount = 0; + + PythonKernel.start = async () => { + startCount += 1; + if (startCount === 1) return deadKernel as unknown as PythonKernel; + replacementStarted.resolve(); + await releaseReplacement.promise; + return replacement as unknown as PythonKernel; + }; + + await executePython("print('setup')", { sessionId: "session-cancelled-restart" }); + deadKernel.markDead(); + + const abortController = new AbortController(); + const cancelled = executePython("print('cancelled')", { + sessionId: "session-cancelled-restart", + signal: abortController.signal, + }); + await replacementStarted.promise; + const retained = executePython("print('retained')", { sessionId: "session-cancelled-restart" }); + await flushMicrotasks(); + abortController.abort(Object.assign(new Error("replacement wait cancelled"), { name: "AbortError" })); + + expect((await cancelled).cancelled).toBe(true); + expect(startCount).toBe(2); + + releaseReplacement.resolve(); + expect((await retained).cancelled).toBe(false); + expect(replacement.executeCalls).toEqual(["print('retained')"]); + }); + + it("invalidates an in-flight replacement before resetting to a fresh generation", async () => { + const deadKernel = new FakeKernel(okResult); + const staleReplacement = new FakeKernel(okResult); + const freshKernel = new FakeKernel(okResult); + const replacementStarted = Promise.withResolvers(); + const releaseReplacement = Promise.withResolvers(); + let startCount = 0; + + PythonKernel.start = async () => { + startCount += 1; + if (startCount === 1) return deadKernel as unknown as PythonKernel; + if (startCount === 2) { + replacementStarted.resolve(); + await releaseReplacement.promise; + return staleReplacement as unknown as PythonKernel; + } + return freshKernel as unknown as PythonKernel; + }; + + await executePython("print('setup')", { sessionId: "session-reset-replacement" }); + deadKernel.markDead(); + + const obsolete = executePython("print('obsolete')", { sessionId: "session-reset-replacement" }); + await replacementStarted.promise; + const reset = executePython("print('reset')", { + sessionId: "session-reset-replacement", + reset: true, + }); + await flushMicrotasks(); + releaseReplacement.resolve(); + + expect((await obsolete).cancelled).toBe(true); + expect((await reset).cancelled).toBe(false); + expect(staleReplacement.executeCalls).toEqual([]); + expect(staleReplacement.shutdownCalls).toBe(1); + expect(freshKernel.executeCalls).toEqual(["print('reset')"]); + + await executePython("print('later')", { sessionId: "session-reset-replacement" }); + expect(startCount).toBe(3); + expect(freshKernel.executeCalls).toEqual(["print('reset')", "print('later')"]); + }); + + it("drains replacements invalidated by owner and global disposal", async () => { + const ownerKernel = new FakeKernel(okResult); + const globalKernel = new FakeKernel(okResult); + const ownerReplacement = new FakeKernel(okResult); + const globalReplacement = new FakeKernel(okResult); + const replacementsStarted = Promise.withResolvers(); + const releaseReplacements = Promise.withResolvers(); + const initialKernels = [ownerKernel, globalKernel]; + const replacementKernels = [ownerReplacement, globalReplacement]; + let replacementStartCount = 0; + + PythonKernel.start = async () => { + const initial = initialKernels.shift(); + if (initial) return initial as unknown as PythonKernel; + replacementStartCount += 1; + if (replacementStartCount === 2) replacementsStarted.resolve(); + await releaseReplacements.promise; + return replacementKernels.shift() as unknown as PythonKernel; + }; + + await executePython("print('owner setup')", { + sessionId: "session-owner-disposal-replacement", + kernelOwnerId: "replacement-owner", + }); + await executePython("print('global setup')", { sessionId: "session-global-disposal-replacement" }); + ownerKernel.markDead(); + globalKernel.markDead(); + + const ownerExecution = executePython("print('owner obsolete')", { + sessionId: "session-owner-disposal-replacement", + kernelOwnerId: "replacement-owner", + }); + const globalExecution = executePython("print('global obsolete')", { + sessionId: "session-global-disposal-replacement", + }); + await replacementsStarted.promise; + + const ownerDisposal = disposeKernelSessionsByOwner("replacement-owner"); + await flushMicrotasks(); + const globalDisposal = disposeAllKernelSessions(); + await flushMicrotasks(); + releaseReplacements.resolve(); + + expect((await ownerExecution).cancelled).toBe(true); + expect((await globalExecution).cancelled).toBe(true); + await Promise.all([ownerDisposal, globalDisposal]); + expect(ownerReplacement.executeCalls).toEqual([]); + expect(globalReplacement.executeCalls).toEqual([]); + expect(ownerReplacement.shutdownCalls).toBe(1); + expect(globalReplacement.shutdownCalls).toBe(1); + }); + + it("keeps replacement coordination independent across normalized cwd keys", async () => { + const deadOne = new FakeKernel(okResult); + const deadTwo = new FakeKernel(okResult); + const replacementOne = new FakeKernel(okResult); + const replacementTwo = new FakeKernel(okResult); + const kernels = [deadOne, deadTwo, replacementOne, replacementTwo]; + let startCount = 0; + + PythonKernel.start = async () => { + startCount += 1; + return kernels.shift() as unknown as PythonKernel; + }; + + await executePython("print('setup one')", { + cwd: "/tmp/replacement-key-one", + sessionId: "session-independent-replacement", + }); + await executePython("print('setup two')", { + cwd: "/tmp/replacement-key-two", + sessionId: "session-independent-replacement", + }); + deadOne.markDead(); + deadTwo.markDead(); + + await Promise.all([ + executePython("print('one')", { + cwd: "/tmp/replacement-key-one", + sessionId: "session-independent-replacement", + }), + executePython("print('two')", { + cwd: "/tmp/replacement-key-two", + sessionId: "session-independent-replacement", + }), + ]); + + expect(startCount).toBe(4); + expect(replacementOne.executeCalls).toEqual(["print('one')"]); + expect(replacementTwo.executeCalls).toEqual(["print('two')"]); + }); + it("resets the session kernel when requested", async () => { const firstKernel = new FakeKernel(okResult); const secondKernel = new FakeKernel(okResult); diff --git a/packages/coding-agent/test/memories-runtime.test.ts b/packages/coding-agent/test/memories-runtime.test.ts index 68906934b..990c0a6ad 100644 --- a/packages/coding-agent/test/memories-runtime.test.ts +++ b/packages/coding-agent/test/memories-runtime.test.ts @@ -145,7 +145,7 @@ describe("memories runtime", () => { process.env.XDG_STATE_HOME = savedXdgState; }); - test("startup gating skips when disabled or subagent depth", async () => { + test("startup gating follows memory.backend and skips subagents", async () => { const disabled = await createFixture({ "memories.enabled": false }); const openSpy = vi.spyOn(memoryStorage, "openMemoryDb"); startMemoryStartupTask({ @@ -156,6 +156,15 @@ describe("memories runtime", () => { taskDepth: 0, }); expect(openSpy).not.toHaveBeenCalled(); + const explicitlyOff = await createFixture({ "memory.backend": "off", "memories.enabled": true }); + startMemoryStartupTask({ + session: explicitlyOff.session, + settings: explicitlyOff.settings, + modelRegistry: explicitlyOff.modelRegistry, + agentDir: explicitlyOff.agentDir, + taskDepth: 0, + }); + expect(openSpy).not.toHaveBeenCalled(); const subagent = await createFixture({ "memories.enabled": true }); startMemoryStartupTask({ diff --git a/packages/coding-agent/test/memory-tools.test.ts b/packages/coding-agent/test/memory-tools.test.ts index 5676732b5..4a65636e6 100644 --- a/packages/coding-agent/test/memory-tools.test.ts +++ b/packages/coding-agent/test/memory-tools.test.ts @@ -8,7 +8,7 @@ */ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; -import { existsSync, mkdirSync } from "node:fs"; +import { mkdirSync } from "node:fs"; import path from "node:path"; import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { HindsightApi } from "@oh-my-pi/pi-coding-agent/hindsight/client"; @@ -17,13 +17,13 @@ import { HindsightSessionState } from "@oh-my-pi/pi-coding-agent/hindsight/state import { mnemopiBackend } from "@oh-my-pi/pi-coding-agent/mnemopi/backend"; import { loadMnemopiConfig, type MnemopiBackendConfig } from "@oh-my-pi/pi-coding-agent/mnemopi/config"; import { - getMnemopiScopedDbPaths, getMnemopiSessionState, loadMnemopi, loadMnemopiCore, MnemopiSessionState, setMnemopiSessionState, } from "@oh-my-pi/pi-coding-agent/mnemopi/state"; +import type { AgentSessionEventListener } from "@oh-my-pi/pi-coding-agent/session/agent-session"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools/index"; import { MemoryEditTool } from "@oh-my-pi/pi-coding-agent/tools/memory-edit"; import { MemoryRecallTool } from "@oh-my-pi/pi-coding-agent/tools/memory-recall"; @@ -32,8 +32,7 @@ import { MemoryRetainTool } from "@oh-my-pi/pi-coding-agent/tools/memory-retain" import { resetMemoryForTests } from "@oh-my-pi/pi-mnemopi"; import { TempDir } from "@oh-my-pi/pi-utils"; -// Mnemopi is lazy-loaded at runtime; preload it so the sync construction in -// registerMnemopiState() and getMnemopiScopedDbPaths() can resolve the module. +// Mnemopi is lazy-loaded at runtime; preload it for synchronous state construction. await Promise.all([loadMnemopi(), loadMnemopiCore()]); const TEST_SESSION_ID = "test-session-id"; @@ -159,6 +158,7 @@ interface RegisterMnemopiStateOptions { cwd?: string; sessionId?: string; entries?: () => unknown[]; + listeners?: Set; } function registerMnemopiState( @@ -172,12 +172,25 @@ function registerMnemopiState( config: finalConfig, session: { sessionId, + settings: Settings.isolated({ + "memory.backend": "mnemopi", + "mnemopi.noEmbeddings": true, + "mnemopi.llmMode": "none", + }), + modelRegistry: { + getApiKeyForProvider: async () => undefined, + resolver: () => async () => undefined, + } as never, sessionManager: { getEntries: options.entries ?? (() => []), getCwd: () => options.cwd ?? "/tmp", } as never, emitNotice: () => {}, getHindsightSessionState: () => undefined, + subscribe: (listener: AgentSessionEventListener) => { + options.listeners?.add(listener); + return () => options.listeners?.delete(listener); + }, } as never, }); setMnemopiSessionState(registeredMnemopiState.session as never, registeredMnemopiState); @@ -838,7 +851,7 @@ describe("Mnemopi backend lifecycle", () => { expect(parentRetainSpy).not.toHaveBeenCalled(); }); - it("clears every scoped Mnemopi database for per-project-tagged mode", async () => { + it("clears scoped Mnemopi data and rehydrates active state", async () => { const config = makeMnemopiConfig({ scoping: "per-project-tagged", bank: "project-alpha", @@ -846,33 +859,40 @@ describe("Mnemopi backend lifecycle", () => { retainBank: "project-alpha", recallBanks: ["project-alpha", "default"], }); - const state = registerMnemopiState(config, { cwd: "/work/project-alpha" }); + const listeners = new Set(); + const state = registerMnemopiState(config, { cwd: "/work/project-alpha", listeners }); state.rememberInScope("project clear marker", { scope: "bank", extract: false, source: "test" }); state.globalMemory?.remember("global clear marker", { scope: "bank", extract: false, source: "test" }); - const dbPaths = getMnemopiScopedDbPaths(config); - for (const dbPath of dbPaths) expect(existsSync(dbPath)).toBe(true); const session = state.session; setMnemopiSessionState(session, state); await mnemopiBackend.clear(path.dirname(config.dbPath), "/work/project-alpha", session); - // The clear() contract: all scoped DB files are deleted. On Windows under - // bun:test, SQLite handle release may lag behind the await; poll briefly - // before asserting rather than failing on a transient lock. - const assertGone = async (p: string): Promise => { - for (let i = 0; i < 40; i++) { - if (!existsSync(p)) return; - await Bun.sleep(25); - } - }; - for (const dbPath of dbPaths) { - await assertGone(dbPath); - await assertGone(`${dbPath}-wal`); - await assertGone(`${dbPath}-shm`); - } - // Assert state was cleared even if file deletion is still in-flight. - expect(getMnemopiSessionState(session)).toBeUndefined(); + const rehydrated = getMnemopiSessionState(session); + if (!rehydrated) throw new Error("Mnemopi state was not rehydrated"); + expect(rehydrated).not.toBe(state); + expect(listeners.size).toBe(1); + const remaining = await rehydrated.recallResultsScoped("clear marker"); + expect(remaining.some(hit => String(hit.content).includes("clear marker"))).toBe(false); + expect(rehydrated.rememberScoped("after-clear", { source: "test", scope: "bank", extract: false })).toEqual( + expect.any(String), + ); + registeredMnemopiState = rehydrated; + }); + it("attaches listeners when enqueue rehydrates missing state", async () => { + const config = makeMnemopiConfig(); + const listeners = new Set(); + const seed = registerMnemopiState(config, { listeners }); + const session = seed.session; + setMnemopiSessionState(session, undefined); + await seed.dispose({ consolidate: false }); registeredMnemopiState = undefined; + + await mnemopiBackend.enqueue(path.dirname(config.dbPath), "/tmp", session); + + registeredMnemopiState = getMnemopiSessionState(session); + expect(registeredMnemopiState).toBeDefined(); + expect(listeners.size).toBe(1); }); it("clear() skips consolidation before deleting the DBs (#2327 review)", async () => { @@ -911,8 +931,8 @@ describe("Mnemopi backend lifecycle", () => { expect(bank.sleep).not.toHaveBeenCalled(); expect(bank.close).toHaveBeenCalledTimes(1); } - expect(getMnemopiSessionState(session)).toBeUndefined(); - registeredMnemopiState = undefined; + registeredMnemopiState = getMnemopiSessionState(session); + expect(registeredMnemopiState).toBeDefined(); }); it("exposes direct mnemopi runtime status and search/save results", async () => { diff --git a/packages/coding-agent/test/model-registry-default-config.test.ts b/packages/coding-agent/test/model-registry-default-config.test.ts index 0d2dba3d8..45b461ede 100644 --- a/packages/coding-agent/test/model-registry-default-config.test.ts +++ b/packages/coding-agent/test/model-registry-default-config.test.ts @@ -33,6 +33,22 @@ describe("ModelRegistry default custom models config", () => { expect(model?.baseUrl).toBe("https://yaml-default.example.com/v1"); }); + test("loads Bedrock cache capabilities from a model override", () => { + writeBedrockCacheOverride(); + + const model = loadDefaultRegistryModel({ + provider: "amazon-bedrock", + modelId: "us.anthropic.claude-opus-4-8", + }); + + expect(model?.compat).toEqual({ + promptCacheMode: "explicit", + supportsLongPromptCacheRetention: false, + promptCacheMinimumTokens: 1024, + promptCacheMaximumCheckpoints: 4, + }); + }); + test("prefers default models.yml over models.yaml when both exist", () => { writeModelsYaml("models.yml", { provider: "yaml-precedence", @@ -105,6 +121,12 @@ interface ModelSnapshot { id: string; name: string; baseUrl: string | undefined; + compat: { + promptCacheMode: string; + supportsLongPromptCacheRetention: boolean; + promptCacheMinimumTokens: number; + promptCacheMaximumCheckpoints: number; + }; } function writeModelsYaml(file: "models.yml" | "models.yaml", fixture: ProviderFixture): void { @@ -133,6 +155,24 @@ function writeModelsYaml(file: "models.yml" | "models.yaml", fixture: ProviderFi ); } +function writeBedrockCacheOverride(): void { + fs.writeFileSync( + path.join(tempDir.path(), "models.yml"), + [ + "providers:", + " amazon-bedrock:", + " modelOverrides:", + " us.anthropic.claude-opus-4-8:", + " compat:", + " promptCacheMode: explicit", + " supportsLongPromptCacheRetention: false", + " promptCacheMinimumTokens: 1024", + " promptCacheMaximumCheckpoints: 4", + "", + ].join("\n"), + ); +} + function writeModelsJson(fixture: ProviderFixture): void { fs.writeFileSync( path.join(tempDir.path(), "models.json"), @@ -173,6 +213,7 @@ function loadDefaultRegistryModel(lookup: ModelLookup): ModelSnapshot | undefine id: model.id, name: model.name, baseUrl: model.baseUrl, + compat: model.compat, } : null)); } finally { authStorage.close(); diff --git a/packages/coding-agent/test/oauth-discovery.test.ts b/packages/coding-agent/test/oauth-discovery.test.ts index 5b77c2b2e..d7c317c38 100644 --- a/packages/coding-agent/test/oauth-discovery.test.ts +++ b/packages/coding-agent/test/oauth-discovery.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "bun:test"; +import type { FetchImpl } from "@oh-my-pi/pi-ai/types"; import { analyzeAuthError, discoverOAuthEndpoints, @@ -641,3 +642,32 @@ describe("RFC 8414 §3.3 issuer validation", () => { }); }); }); + +describe("bounded discovery fetches", () => { + // A fetch that never resolves on its own; it settles only when its + // AbortSignal fires. Pre-fix, discovery passed no signal, so this hung forever. + const hangingFetch: FetchImpl = (_input, init) => { + const { promise, reject } = Promise.withResolvers(); + const signal = init?.signal; + const abort = () => reject(new DOMException("aborted", "AbortError")); + if (signal?.aborted) abort(); + else signal?.addEventListener("abort", abort, { once: true }); + return promise; + }; + + it("aborts hanging well-known discovery fetches instead of stalling", async () => { + const oauth = await discoverOAuthEndpoints("https://mcp.example.test/mcp", undefined, undefined, { + fetch: hangingFetch, + signal: AbortSignal.timeout(50), + }); + expect(oauth).toBeNull(); + }); + + it("aborts a hanging resource_metadata fetch and returns undefined", async () => { + const scopes = await fetchResourceMetadataScopes( + "https://mcp.example.test/.well-known/oauth-protected-resource", + { fetch: hangingFetch, signal: AbortSignal.timeout(50) }, + ); + expect(scopes).toBeUndefined(); + }); +}); diff --git a/packages/coding-agent/test/read-edit-out-of-cwd.test.ts b/packages/coding-agent/test/read-edit-out-of-cwd.test.ts index 26da1c9de..7d0777c0e 100644 --- a/packages/coding-agent/test/read-edit-out-of-cwd.test.ts +++ b/packages/coding-agent/test/read-edit-out-of-cwd.test.ts @@ -4,7 +4,7 @@ import * as os from "node:os"; import * as path from "node:path"; import type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; -import { type ExecuteHashlineSingleOptions, executeHashlineSingle } from "@oh-my-pi/pi-coding-agent/edit"; +import { EditTool, type ExecuteHashlineSingleOptions, executeHashlineSingle } from "@oh-my-pi/pi-coding-agent/edit"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import type { ReadToolDetails } from "@oh-my-pi/pi-coding-agent/tools/read"; import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; @@ -141,6 +141,87 @@ describe("read → edit round-trip for out-of-cwd files", () => { expect(textOutput(result)).not.toContain("Artifact content."); }); + it("uses the read-resolved workspace suffix across direct edit modes", async () => { + const cases: Array<{ + mode: "replace" | "patch" | "apply_patch"; + run: (tool: EditTool, fileName: string) => Promise; + }> = [ + { + mode: "replace", + run: async (tool, fileName) => { + await tool.execute("edit-workspace-suffix-replace", { + path: fileName, + edits: [{ old_text: "alpha", new_text: "ALPHA" }], + }); + }, + }, + { + mode: "patch", + run: async (tool, fileName) => { + await tool.execute("edit-workspace-suffix-patch", { + path: fileName, + edits: [{ op: "update", diff: "@@\n-alpha\n+ALPHA" }], + }); + }, + }, + { + mode: "apply_patch", + run: async (tool, fileName) => { + const input = [ + "*** Begin Patch", + `*** Update File: ${fileName}`, + "@@", + "-alpha", + "+ALPHA", + "*** End Patch", + "", + ].join("\n"); + await tool.execute("edit-workspace-suffix-apply-patch", { input }); + }, + }, + ]; + + for (const testCase of cases) { + const fileName = `${testCase.mode}.txt`; + const workspaceFile = path.join(cwdDir, "src", fileName); + await Bun.write(workspaceFile, "alpha\nbeta\n"); + + const session = createSession(cwdDir); + session.settings.set("edit.mode", testCase.mode); + const readResult = await new ReadTool(session).execute(`read-workspace-suffix-${testCase.mode}`, { + path: fileName, + }); + expect(textOutput(readResult)).toContain("alpha"); + + await testCase.run(new EditTool(session), fileName); + expect(await Bun.file(workspaceFile).text()).toBe("ALPHA\nbeta\n"); + } + }); + + it("keeps the resolved workspace target across delete/add hunks for the same authored path", async () => { + const fileName = "recreate.txt"; + const workspaceFile = path.join(cwdDir, "src", fileName); + await Bun.write(workspaceFile, "alpha\nbeta\n"); + + const session = createSession(cwdDir); + session.settings.set("edit.mode", "apply_patch"); + const readResult = await new ReadTool(session).execute("read-workspace-suffix-recreate", { path: fileName }); + expect(textOutput(readResult)).toContain("alpha"); + + const input = [ + "*** Begin Patch", + `*** Delete File: ${fileName}`, + `*** Add File: ${fileName}`, + "+rewritten", + "*** End Patch", + "", + ].join("\n"); + await new EditTool(session).execute("edit-workspace-suffix-recreate", { input }); + + expect(await Bun.file(workspaceFile).text()).toBe("rewritten\n"); + expect(await Bun.file(path.join(cwdDir, fileName)).exists()).toBe(false); + }); + it("prefers a unique workspace suffix match over the approved local plan alias", async () => { const artifactsDir = path.join(outDir, "artifacts"); const planFilePath = "local://windows-packaging-plan.md"; diff --git a/packages/coding-agent/test/selector-settings-side-effects.test.ts b/packages/coding-agent/test/selector-settings-side-effects.test.ts index 532d290a9..e7d558ca9 100644 --- a/packages/coding-agent/test/selector-settings-side-effects.test.ts +++ b/packages/coding-agent/test/selector-settings-side-effects.test.ts @@ -66,6 +66,17 @@ describe("selector setting side effects", () => { expect(invalidate).toHaveBeenCalledTimes(1); expect(requestRender).toHaveBeenCalledTimes(1); }); + it("applies memory backend changes to the live session", () => { + const applyMemoryBackend = vi.fn(async () => {}); + const controller = new SelectorController({ + session: { applyMemoryBackend }, + showError: vi.fn(), + } as unknown as InteractiveModeContext); + + controller.handleSettingChange("memory.backend", "mnemopi"); + + expect(applyMemoryBackend).toHaveBeenCalledTimes(1); + }); for (const id of ["terminal.showImages", "showImages"]) { for (const visible of [false, true]) { diff --git a/packages/coding-agent/test/settings-manager.test.ts b/packages/coding-agent/test/settings-manager.test.ts index 259d28893..080f8d2e2 100644 --- a/packages/coding-agent/test/settings-manager.test.ts +++ b/packages/coding-agent/test/settings-manager.test.ts @@ -472,7 +472,6 @@ describe("Settings", () => { const withFileLock = fileLock.withFileLock; vi.spyOn(fileLock, "withFileLock").mockImplementation(async (filePath, fn, options) => { firstSaveEntered.resolve(); - await releaseFirstSave.promise; const result = await withFileLock(filePath, fn, options); firstSaveFinished.resolve(); return result; diff --git a/packages/coding-agent/test/theme-lazy-status-color.test.ts b/packages/coding-agent/test/theme-lazy-status-color.test.ts new file mode 100644 index 000000000..b955f26b6 --- /dev/null +++ b/packages/coding-agent/test/theme-lazy-status-color.test.ts @@ -0,0 +1,66 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import * as themeModule from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import { Text } from "@oh-my-pi/pi-tui"; + +/** + * Regression for issue #6337: a status message presented while the auto-theme + * default guess (dark) was active must re-resolve its color when the terminal's + * appearance reply later switches the active theme to light. The transient + * status presenters supply the color via `Text.setStyleFn` (evaluated at render + * time against the live `theme` binding) instead of baking `theme.fg()` into the + * component, so invalidating on `onThemeChange` re-shapes it. + */ + +/** Opening SGR sequence `theme.fg(color, ...)` emits, independent of color mode. */ +function fgPrefix(color: "warning"): string { + const styled = themeModule.theme.fg(color, "\u0001"); + return styled.slice(0, styled.indexOf("\u0001")); +} + +describe("lazy status color re-resolves on theme switch", () => { + beforeEach(async () => { + themeModule.stopThemeWatcher(); + const dark = await themeModule.getThemeByName("dark"); + if (!dark) throw new Error("Failed to load dark theme for tests"); + themeModule.setThemeInstance(dark); + vi.restoreAllMocks(); + }); + + afterEach(async () => { + themeModule.stopThemeWatcher(); + const dark = await themeModule.getThemeByName("dark"); + if (dark) themeModule.setThemeInstance(dark); + vi.restoreAllMocks(); + }); + + it("swaps a presented warning from dark-catppuccin to light-catppuccin color", async () => { + // Auto-theme resolves dark before the appearance reply arrives. + themeModule.onTerminalAppearanceChange("dark"); + await themeModule.initTheme(false, undefined, undefined, "dark-catppuccin", "light-catppuccin"); + expect(themeModule.getCurrentThemeName()).toBe("dark-catppuccin"); + + const darkPrefix = fgPrefix("warning"); + const warning = new Text("Warning: Failed to load extension", 1, 0).setStyleFn(t => + themeModule.theme.fg("warning", t), + ); + expect(warning.render(80).join("")).toContain(darkPrefix); + + // The OSC 11 reply arrives → auto-theme switches to light-catppuccin. + const switched = Promise.withResolvers(); + const off = themeModule.onThemeChange(() => switched.resolve()); + themeModule.onTerminalAppearanceChange("light"); + await switched.promise; + off(); + expect(themeModule.getCurrentThemeName()).toBe("light-catppuccin"); + + const lightPrefix = fgPrefix("warning"); + expect(lightPrefix).not.toBe(darkPrefix); + + // The onThemeChange handler invalidates + repaints; the warning must now + // render the light-mode color, not the baked dark one. + warning.invalidate(); + const out = warning.render(80).join(""); + expect(out).toContain(lightPrefix); + expect(out).not.toContain(darkPrefix); + }); +}); diff --git a/packages/coding-agent/test/tools/bash-skill-urls.test.ts b/packages/coding-agent/test/tools/bash-skill-urls.test.ts index d84a41409..00458e2e0 100644 --- a/packages/coding-agent/test/tools/bash-skill-urls.test.ts +++ b/packages/coding-agent/test/tools/bash-skill-urls.test.ts @@ -216,6 +216,65 @@ describe("expandInternalUrls", () => { ); }); + it("expands an unquoted URL inside a backtick substitution nested in double quotes", async () => { + const skills = [createSkill("valid-skill", "/tmp/skills/valid-skill")]; + const command = 'echo "`cat skill://valid-skill/SKILL.md`"'; + const expectedPath = path.join(skills[0].baseDir, "SKILL.md"); + + await expect(expandInternalUrls(command, { skills })).resolves.toBe( + `echo "\`cat ${shellEscape(expectedPath)}\`"`, + ); + }); + + it("expands a top-level unquoted URL inside a backtick substitution", async () => { + const skills = [createSkill("valid-skill", "/tmp/skills/valid-skill")]; + const command = "echo `cat skill://valid-skill/SKILL.md`"; + const expectedPath = path.join(skills[0].baseDir, "SKILL.md"); + + await expect(expandInternalUrls(command, { skills })).resolves.toBe(`echo \`cat ${shellEscape(expectedPath)}\``); + }); + + it("expands nested $() inside a double-quoted backtick substitution", async () => { + const skills = [createSkill("valid-skill", "/tmp/skills/valid-skill")]; + const command = 'echo "`echo $(cat skill://valid-skill/SKILL.md)`"'; + const expectedPath = path.join(skills[0].baseDir, "SKILL.md"); + + await expect(expandInternalUrls(command, { skills })).resolves.toBe( + `echo "\`echo $(cat ${shellEscape(expectedPath)})\`"`, + ); + }); + + it("expands nested backticks inside a double-quoted $() substitution", async () => { + const skills = [createSkill("valid-skill", "/tmp/skills/valid-skill")]; + const command = 'echo "$(echo `cat skill://valid-skill/SKILL.md`)"'; + const expectedPath = path.join(skills[0].baseDir, "SKILL.md"); + + await expect(expandInternalUrls(command, { skills })).resolves.toBe( + `echo "$(echo \`cat ${shellEscape(expectedPath)}\`)"`, + ); + }); + + it("leaves a URL inside a single-quoted backtick string literal", async () => { + const skills = [createSkill("valid-skill", "/tmp/skills/valid-skill")]; + const command = "echo '`cat skill://valid-skill/SKILL.md`'"; + + await expect(expandInternalUrls(command, { skills })).resolves.toBe(command); + }); + + it("leaves a URL behind an escaped backtick in double quotes literal", async () => { + const skills = [createSkill("valid-skill", "/tmp/skills/valid-skill")]; + const command = 'echo "\\`skill://valid-skill/SKILL.md\\`"'; + + await expect(expandInternalUrls(command, { skills })).resolves.toBe(command); + }); + + it("leaves a URL inside escaped quotes within a double-quoted backtick substitution", async () => { + const skills = [createSkill("valid-skill", "/tmp/skills/valid-skill")]; + const command = 'echo "`printf %s \\"literal skill://valid-skill/SKILL.md\\"`"'; + + await expect(expandInternalUrls(command, { skills })).resolves.toBe(command); + }); + it("leaves literal internal URLs embedded in quoted text unchanged", async () => { const router = createInternalRouter({ "memory://root/summary.md": { sourcePath: "/tmp/memories/summary.md" }, diff --git a/packages/coding-agent/test/tools/browser-open-lease.test.ts b/packages/coding-agent/test/tools/browser-open-lease.test.ts new file mode 100644 index 000000000..334e38bf9 --- /dev/null +++ b/packages/coding-agent/test/tools/browser-open-lease.test.ts @@ -0,0 +1,211 @@ +/** + * Regression tests for issue #6365: `BrowserTool.#open` must apply the + * requested `timeout` to the *entire* open lifecycle (browser acquisition + + * tab acquisition), and must hold one explicit browser lease across tab + * acquisition so a refCount:0 browser is never orphaned by an abort/timeout + * nor disposed out from under a concurrent open of a different tab name. + * + * The tool resolves the cmux backend (`CMUX_SOCKET_PATH` + settings), so + * `CmuxSocketClient.prototype` is spied and no real socket / Chromium is used. + */ + +import { afterEach, beforeEach, describe, expect, it, spyOn, vi } from "bun:test"; +import { BrowserTool } from "@oh-my-pi/pi-coding-agent/tools/browser"; +import { CmuxSocketClient } from "@oh-my-pi/pi-coding-agent/tools/browser/cmux/socket-client"; +import { getBrowsersMapForTest } from "@oh-my-pi/pi-coding-agent/tools/browser/registry"; +import { getTabsMapForTest, releaseTab } from "@oh-my-pi/pi-coding-agent/tools/browser/tab-supervisor"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools/index"; +import { ToolAbortError, ToolError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors"; + +function makeSession(): ToolSession { + return { + cwd: "/tmp", + hasUI: false, + settings: { + get: (key: string) => (key === "browser.cmux" ? true : key === "tools.maxTimeout" ? 0 : undefined), + }, + getSessionId: () => "session-open-lease", + } as unknown as ToolSession; +} + +async function drainAllTabs(): Promise { + for (const name of [...getTabsMapForTest().keys()]) { + await releaseTab(name, { kill: false }).catch(() => undefined); + } +} + +let prevSocketPath: string | undefined; + +beforeEach(() => { + prevSocketPath = process.env.CMUX_SOCKET_PATH; + // Unique per test so the module-global browsers map (keyed by socket path) + // never carries a handle across tests. + process.env.CMUX_SOCKET_PATH = `/tmp/omp-open-lease-${process.pid}-${Math.random().toString(36).slice(2)}.sock`; +}); + +afterEach(async () => { + vi.useRealTimers(); + await drainAllTabs().catch(() => undefined); + vi.restoreAllMocks(); + if (prevSocketPath === undefined) delete process.env.CMUX_SOCKET_PATH; + else process.env.CMUX_SOCKET_PATH = prevSocketPath; +}); + +describe("browser open — requested timeout bounds the whole acquisition (#6365)", () => { + it("rejects with a timeout ToolError when browser acquisition stays pending past the deadline", async () => { + vi.useFakeTimers(); + const connectGate = Promise.withResolvers(); + spyOn(CmuxSocketClient.prototype, "connect").mockImplementation(async () => { + await connectGate.promise; + }); + const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + + const tool = new BrowserTool(makeSession()); + const open = tool.execute("call-timeout", { action: "open", name: "late", timeout: 1 }); + const settled = open.then( + () => ({ ok: true as const }), + (err: unknown) => ({ ok: false as const, err }), + ); + + // The requested 1s deadline elapses while `acquireBrowser` is still + // blocked on the (never-resolving) socket connect. Bun's fake timers fire + // `AbortSignal.timeout` synchronously on advance; awaiting `settled` below + // flushes the rejection. + vi.advanceTimersByTime(1000); + + const outcome = await settled; + expect(outcome.ok).toBe(false); + if (outcome.ok) throw new Error("unreachable"); + // The requested action timeout surfaces as a timeout ToolError — never a + // ToolAbortError (that is reserved for caller cancellation). + expect(outcome.err).toBeInstanceOf(ToolError); + expect(outcome.err).not.toBeInstanceOf(ToolAbortError); + expect((outcome.err as Error).message).toMatch(/timed out/i); + + // Let the orphan launch resolve; the aborted deadline must dispose it so + // no refCount:0 browser survives in the registry. + connectGate.resolve(); + for (let i = 0; i < 20; i++) await Promise.resolve(); + expect(closeSpy).toHaveBeenCalledTimes(1); + expect(getBrowsersMapForTest().size).toBe(0); + }); +}); + +describe("browser open — caller cancellation rolls back the fresh browser (#6365)", () => { + it("aborting before tab publication rejects with ToolAbortError and leaves both maps empty", async () => { + spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); + const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + const openSplitGate = Promise.withResolvers(); + const surfaceClosed: string[] = []; + spyOn(CmuxSocketClient.prototype, "request").mockImplementation( + async (method: string, params: Record): Promise> => { + if (method === "browser.open_split") { + await openSplitGate.promise; + return { surface_id: "surface-abort", url: "about:blank" }; + } + if (method === "surface.close") { + surfaceClosed.push(String(params.surface_id)); + return {}; + } + return {}; + }, + ); + + const tool = new BrowserTool(makeSession()); + const controller = new AbortController(); + const open = tool.execute("call-abort", { action: "open", name: "fresh", timeout: 30 }, controller.signal); + const settled = open.then( + () => ({ ok: true as const }), + (err: unknown) => ({ ok: false as const, err }), + ); + + // Browser acquisition has resolved; tab acquisition is parked in + // `open_split`. Cancel here — before any tab is published. + await Promise.resolve(); + controller.abort(); + + const outcome = await settled; + expect(outcome.ok).toBe(false); + if (outcome.ok) throw new Error("unreachable"); + expect(outcome.err).toBeInstanceOf(ToolAbortError); + + // The open-acquisition lease rollback disposes the fresh browser exactly + // once and leaves nothing owned solely by the failed open. + expect(getTabsMapForTest().has("fresh")).toBe(false); + expect(getBrowsersMapForTest().size).toBe(0); + expect(closeSpy).toHaveBeenCalledTimes(1); + + // Let the orphaned acquisition unwind so it does not leak past the test. + openSplitGate.resolve(); + await Promise.resolve(); + }); +}); + +describe("browser open — concurrent different-name acquisitions each own a lease (#6365)", () => { + it("aborting one open releases only its lease; the survivor keeps the browser and one tab", async () => { + spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); + const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + const openGate = Promise.withResolvers(); + let splitCount = 0; + const aEntered = Promise.withResolvers(); + const bEntered = Promise.withResolvers(); + const surfaceClosed: string[] = []; + spyOn(CmuxSocketClient.prototype, "request").mockImplementation( + async (method: string, params: Record): Promise> => { + if (method === "browser.open_split") { + const id = `surface-${++splitCount}`; + (splitCount === 1 ? aEntered : bEntered).resolve(); + await openGate.promise; + return { surface_id: id, url: "about:blank" }; + } + if (method === "surface.close") { + surfaceClosed.push(String(params.surface_id)); + return {}; + } + return {}; + }, + ); + + const tool = new BrowserTool(makeSession()); + + // Open A first and wait until it is parked inside `open_split` — proof it + // acquired the shared browser and took its open-acquisition lease. + const controllerA = new AbortController(); + const openA = tool.execute("call-a", { action: "open", name: "tab-a", timeout: 30 }, controllerA.signal); + const settledA = openA.then( + () => ({ ok: true as const }), + (err: unknown) => ({ ok: false as const, err }), + ); + await aEntered.promise; + expect(getBrowsersMapForTest().size).toBe(1); + + // Open B against the SAME browser (different tab name). It reuses the + // registry handle and takes its own lease; both are now parked. + const openB = tool.execute("call-b", { action: "open", name: "tab-b", timeout: 30 }); + await bEntered.promise; + + // Abort A while both are queued; releasing A's lease must not dispose the + // browser B still needs. + controllerA.abort(); + const outcomeA = await settledA; + expect(outcomeA.ok).toBe(false); + if (outcomeA.ok) throw new Error("unreachable"); + expect(outcomeA.err).toBeInstanceOf(ToolAbortError); + + // Release the gate so B publishes its tab. + openGate.resolve(); + const resultB = await openB; + expect(resultB.content.some(part => part.type === "text" && /Opened tab "tab-b"/.test(part.text ?? ""))).toBe( + true, + ); + + // B's browser survived A's rollback: still present, never closed, exactly + // one published tab. A's rollback closed only its own orphan surface. + expect(getBrowsersMapForTest().size).toBe(1); + expect(closeSpy).not.toHaveBeenCalled(); + expect(getTabsMapForTest().has("tab-b")).toBe(true); + expect(getTabsMapForTest().has("tab-a")).toBe(false); + expect(getTabsMapForTest().size).toBe(1); + expect(surfaceClosed).toEqual(["surface-1"]); + }); +}); diff --git a/packages/coding-agent/test/tools/lsp-regressions.test.ts b/packages/coding-agent/test/tools/lsp-regressions.test.ts index bf06804f4..0b562f9ed 100644 --- a/packages/coding-agent/test/tools/lsp-regressions.test.ts +++ b/packages/coding-agent/test/tools/lsp-regressions.test.ts @@ -44,6 +44,7 @@ import { } from "@oh-my-pi/pi-coding-agent/lsp/utils"; import { getThemeByName } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { ToolAbortError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors"; import { clampTimeout } from "@oh-my-pi/pi-coding-agent/tools/tool-timeouts"; import * as piUtils from "@oh-my-pi/pi-utils"; import { sanitizeText, TempDir } from "@oh-my-pi/pi-utils"; @@ -86,7 +87,7 @@ type FakeLspHandler = (message: RpcMessage, server: FakeLspServer) => void | Pro // no real-clock latency. Installed by spying on the shared `ptree` namespace // object (NOT `mock.module`, which would leak across files); the suite's // `afterEach` `vi.restoreAllMocks()` removes it. -function installFakeLsp(handler: FakeLspHandler): FakeLspServer { +function installFakeLsp(handler: FakeLspHandler, options?: { killResolvesExit?: boolean }): FakeLspServer { const encoder = new TextEncoder(); const received: RpcMessage[] = []; const waiters: Array<{ @@ -191,7 +192,7 @@ function installFakeLsp(handler: FakeLspHandler): FakeLspServer { peekStderr: () => "", kill() { killed = true; - server.exit(0); + if (options?.killResolvesExit !== false) server.exit(0); }, } as unknown as LspClient["proc"]; @@ -2772,6 +2773,194 @@ describe("lsp regressions", () => { } }); + describe("reload cancellation and truthful teardown (#6369)", () => { + // A JSON-RPC error response the client maps to isMethodNotFoundError, so a + // non-rust server falls through from `rust-analyzer/reloadWorkspace` to the + // generic `workspace/didChangeConfiguration` reload. + const methodNotFound = (id: RpcMessage["id"]): RpcMessage => ({ + jsonrpc: "2.0", + id, + error: { code: -32_601, message: "method not found" }, + }); + + it("propagates cancellation of the reload request instead of reporting Restarted", async () => { + const tempDir = TempDir.createSync("@omp-lsp-reload-cancel-req-"); + try { + const server = installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + // rust-analyzer/reloadWorkspace is left pending: only the caller + // signal decides its fate. + }); + const config: ServerConfig = { command: "fake-reload-cancel-req", fileTypes: [".ts"], rootMarkers: [] }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: { fake: config }, idleTimeoutMs: undefined }); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const controller = new AbortController(); + const pending = tool.execute("reload-cancel-req", { action: "reload", file: "*" }, controller.signal); + await server.waitFor(m => m.method === "rust-analyzer/reloadWorkspace"); + controller.abort(new ToolAbortError()); + + // Pre-fix, the bare `catch` swallowed the abort, fell through to the + // notification (also aborted), hit the second bare `catch`, killed + // the process, and returned "Restarted". + await expect(pending).rejects.toBeInstanceOf(ToolAbortError); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("propagates cancellation that arrives during the notification fallback", async () => { + const tempDir = TempDir.createSync("@omp-lsp-reload-cancel-fallback-"); + const controller = new AbortController(); + try { + installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "rust-analyzer/reloadWorkspace") { + // Fall through to the generic reload, then cancel before it lands. + srv.send(methodNotFound(message.id)); + controller.abort(new ToolAbortError()); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { command: "fake-reload-cancel-fb", fileTypes: [".ts"], rootMarkers: [] }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: { fake: config }, idleTimeoutMs: undefined }); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const pending = tool.execute("reload-cancel-fb", { action: "reload", file: "*" }, controller.signal); + + await expect(pending).rejects.toBeInstanceOf(ToolAbortError); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("still falls back to the generic reload on method-not-found without killing the server", async () => { + const tempDir = TempDir.createSync("@omp-lsp-reload-fallback-ok-"); + try { + const server = installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "rust-analyzer/reloadWorkspace") { + srv.send(methodNotFound(message.id)); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { command: "fake-reload-fallback", fileTypes: [".ts"], rootMarkers: [] }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: { fake: config }, idleTimeoutMs: undefined }); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const result = await tool.execute("reload-fallback", { action: "reload", file: "*" }); + + expect(textResult(result)).toContain("Reloaded fake"); + expect(server.killed).toBe(false); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("recognizes -32601 by code even when the server's message text is nonstandard", async () => { + const tempDir = TempDir.createSync("@omp-lsp-reload-fallback-code-"); + try { + const server = installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "rust-analyzer/reloadWorkspace") { + // None of isMethodNotFoundError's message substrings — only the + // JSON-RPC code identifies this as method-not-found. + srv.send({ jsonrpc: "2.0", id: message.id, error: { code: -32_601, message: "Unknown request" } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { command: "fake-reload-fallback-code", fileTypes: [".ts"], rootMarkers: [] }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: { fake: config }, idleTimeoutMs: undefined }); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const result = await tool.execute("reload-fallback-code", { action: "reload", file: "*" }); + + expect(textResult(result)).toContain("Reloaded fake"); + expect(server.killed).toBe(false); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("shutdownClientInstance removes the client by identity and confirms process exit", async () => { + const tempDir = TempDir.createSync("@omp-lsp-teardown-confirm-"); + try { + installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { command: "fake-teardown-confirm", fileTypes: [".ts"], rootMarkers: [] }; + const client = await lspClient.getOrCreateClient(config, tempDir.path()); + expect(lspClient.getActiveClients().some(s => s.name === config.command)).toBe(true); + + const exited = await lspClient.shutdownClientInstance(client); + expect(exited).toBe(true); + expect(lspClient.getActiveClients().some(s => s.name === config.command)).toBe(false); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("shutdownClientInstance reports a failed teardown when the process outlives the kill", async () => { + const tempDir = TempDir.createSync("@omp-lsp-teardown-delayed-"); + try { + installFakeLsp( + (message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } + // `exit` notification and `kill()` never resolve `proc.exited`. + }, + { killResolvesExit: false }, + ); + const config: ServerConfig = { command: "fake-teardown-delayed", fileTypes: [".ts"], rootMarkers: [] }; + const client = await lspClient.getOrCreateClient(config, tempDir.path()); + + const exited = await lspClient.shutdownClientInstance(client); + expect(exited).toBe(false); + expect(lspClient.getActiveClients().some(s => s.name === config.command)).toBe(false); + } finally { + vi.restoreAllMocks(); + tempDir.removeSync(); + } + }, 15_000); + }); + // #3962 — LSP cold-start and notification writes must honor the tool's // combined timeout/caller abort signal. Before the fix, a wedged server // hung past the tool's advertised deadline: `initialize` fell back to the diff --git a/packages/coding-agent/test/tools/read-pdf-images.test.ts b/packages/coding-agent/test/tools/read-pdf-images.test.ts index 4c4a0ebe1..5c96b5008 100644 --- a/packages/coding-agent/test/tools/read-pdf-images.test.ts +++ b/packages/coding-agent/test/tools/read-pdf-images.test.ts @@ -10,9 +10,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +import type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; -import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; +import { ReadTool, type ReadToolDetails } from "@oh-my-pi/pi-coding-agent/tools/read"; import * as markit from "@oh-my-pi/pi-coding-agent/utils/markit"; import { removeSyncWithRetries, Snowflake } from "@oh-my-pi/pi-utils"; @@ -48,6 +49,28 @@ function mockExtraction(members: Record = { "p11-img0.png": TINY }); } +function imageBytes(result: AgentToolResult): Buffer { + const image = result.content.find(content => content.type === "image"); + if (image?.type !== "image") throw new Error("Expected an image result"); + return Buffer.from(image.data, "base64"); +} + +function mockBlockedExtraction() { + const entered = Promise.withResolvers(); + const release = Promise.withResolvers(); + const spy = vi.spyOn(markit, "convertFileWithMarkit").mockImplementation(async (_sourcePath, signal, options) => { + entered.resolve(); + await release.promise; + signal?.throwIfAborted(); + if (options?.imageDir) { + fs.mkdirSync(options.imageDir, { recursive: true }); + fs.writeFileSync(path.join(options.imageDir, "p11-img0.png"), TINY_PNG); + } + return { ok: true, content: "" }; + }); + return { entered, release, spy }; +} + describe("read PDF image extraction", () => { let testDir: string; let pdfPath: string; @@ -131,6 +154,218 @@ describe("read PDF image extraction", () => { expect(spy).toHaveBeenCalledTimes(1); }); + it("re-extracts image members after same-path PDF replacement", async () => { + const sourceA = Buffer.from("%PDF-source-a"); + const sourceB = Buffer.from("%PDF-source-b"); + fs.writeFileSync(pdfPath, sourceA); + const spy = vi.spyOn(markit, "convertFileWithMarkit").mockImplementation(async (sourcePath, _signal, options) => { + if (options?.imageDir) { + fs.mkdirSync(options.imageDir, { recursive: true }); + fs.writeFileSync( + path.join(options.imageDir, "p11-img0.png"), + Buffer.concat([TINY_PNG, fs.readFileSync(sourcePath)]), + ); + } + return { ok: true, content: "" }; + }); + const tool = new ReadTool(makeSession(testDir)); + + const originalStat = fs.statSync(pdfPath); + const first = await tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + fs.writeFileSync(pdfPath, sourceB); + fs.utimesSync(pdfPath, originalStat.atime, originalStat.mtime); + const second = await tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + + expect(imageBytes(first).subarray(TINY_PNG.length)).toEqual(sourceA); + expect(imageBytes(second).subarray(TINY_PNG.length)).toEqual(sourceB); + expect(spy).toHaveBeenCalledTimes(2); + }); + + it("converts an immutable snapshot when the source changes during extraction", async () => { + const sourceA = Buffer.from("%PDF-source-a"); + const sourceB = Buffer.from("%PDF-source-b"); + fs.writeFileSync(pdfPath, sourceA); + const entered = Promise.withResolvers(); + const release = Promise.withResolvers(); + vi.spyOn(markit, "convertFileWithMarkit").mockImplementation(async (sourcePath, _signal, options) => { + entered.resolve(); + await release.promise; + if (options?.imageDir) { + fs.mkdirSync(options.imageDir, { recursive: true }); + fs.writeFileSync( + path.join(options.imageDir, "p11-img0.png"), + Buffer.concat([TINY_PNG, fs.readFileSync(sourcePath)]), + ); + } + return { ok: true, content: "" }; + }); + const pending = new ReadTool(makeSession(testDir)).execute("call", { path: `${pdfPath}:p11-img0.png` }); + + await entered.promise; + fs.writeFileSync(pdfPath, sourceB); + release.resolve(); + const result = await pending; + + expect(imageBytes(result).subarray(TINY_PNG.length)).toEqual(sourceA); + }); + + it("coalesces concurrent cold image extraction", async () => { + const { entered, release, spy } = mockBlockedExtraction(); + const tool = new ReadTool(makeSession(testDir)); + const first = tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + const second = tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + + await entered.promise; + const conversionCount = spy.mock.calls.length; + release.resolve(); + const [firstResult, secondResult] = await Promise.all([first, second]); + + expect(conversionCount).toBe(1); + expect(imageBytes(firstResult)).toEqual(imageBytes(secondResult)); + }); + + it("keeps shared extraction running when its owner aborts", async () => { + const { entered, release, spy } = mockBlockedExtraction(); + const tool = new ReadTool(makeSession(testDir)); + const ownerController = new AbortController(); + const owner = tool.execute("call", { path: `${pdfPath}:p11-img0.png` }, ownerController.signal); + const joiner = tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + await entered.promise; + + ownerController.abort(); + await expect(owner).rejects.toThrow(/Aborted|Cancelled/); + release.resolve(); + const result = await joiner; + + expect(result.content.some(content => content.type === "image")).toBe(true); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it("keeps shared extraction running when a joiner aborts", async () => { + const { entered, release, spy } = mockBlockedExtraction(); + const tool = new ReadTool(makeSession(testDir)); + const joinerController = new AbortController(); + const owner = tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + const joiner = tool.execute("call", { path: `${pdfPath}:p11-img0.png` }, joinerController.signal); + await entered.promise; + + joinerController.abort(); + await expect(joiner).rejects.toThrow(/Aborted|Cancelled/); + release.resolve(); + const result = await owner; + + expect(result.content.some(content => content.type === "image")).toBe(true); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it("cleans temporary extraction state when the only caller aborts", async () => { + const entered = Promise.withResolvers(); + let snapshotPath: string | undefined; + let stagingDir: string | undefined; + vi.spyOn(markit, "convertFileWithMarkit").mockImplementation(async (sourcePath, signal, options) => { + snapshotPath = sourcePath; + stagingDir = options?.imageDir; + entered.resolve(); + const aborted = Promise.withResolvers(); + const onAbort = () => aborted.resolve(); + if (signal?.aborted) onAbort(); + else signal?.addEventListener("abort", onAbort, { once: true }); + await aborted.promise; + signal?.removeEventListener("abort", onAbort); + signal?.throwIfAborted(); + return { ok: true, content: "" }; + }); + const controller = new AbortController(); + const pending = new ReadTool(makeSession(testDir)).execute( + "call", + { path: `${pdfPath}:p11-img0.png` }, + controller.signal, + ); + + await entered.promise; + controller.abort(); + await expect(pending).rejects.toThrow(/Aborted|Cancelled/); + if (!snapshotPath || !stagingDir) throw new Error("Expected extraction paths"); + + expect(fs.existsSync(path.dirname(snapshotPath))).toBe(false); + expect(fs.existsSync(stagingDir)).toBe(false); + }); + + it("does not let a failed generation delete a replacement generation", async () => { + const sourceA = Buffer.from("%PDF-source-a"); + const sourceB = Buffer.from("%PDF-source-b"); + fs.writeFileSync(pdfPath, sourceA); + const firstEntered = Promise.withResolvers(); + const failFirst = Promise.withResolvers(); + const spy = vi.spyOn(markit, "convertFileWithMarkit").mockImplementation(async (sourcePath, _signal, options) => { + const source = fs.readFileSync(sourcePath); + if (source.equals(sourceA)) { + firstEntered.resolve(); + await failFirst.promise; + return { ok: false, content: "", error: "generation A failed" }; + } + if (options?.imageDir) { + fs.mkdirSync(options.imageDir, { recursive: true }); + fs.writeFileSync(path.join(options.imageDir, "p11-img0.png"), Buffer.concat([TINY_PNG, source])); + } + return { ok: true, content: "" }; + }); + const tool = new ReadTool(makeSession(testDir)); + const first = tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + await firstEntered.promise; + fs.writeFileSync(pdfPath, sourceB); + + const replacement = await tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + failFirst.resolve(); + await expect(first).rejects.toThrow(/Cannot extract images/); + const cachedReplacement = await tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + + expect(imageBytes(replacement).subarray(TINY_PNG.length)).toEqual(sourceB); + expect(imageBytes(cachedReplacement)).toEqual(imageBytes(replacement)); + expect(spy).toHaveBeenCalledTimes(2); + }); + + it("isolates equal-content PDFs with the same basename in different directories", async () => { + const otherDir = path.join(testDir, "other"); + const otherPdfPath = path.join(otherDir, path.basename(pdfPath)); + fs.mkdirSync(otherDir, { recursive: true }); + fs.writeFileSync(otherPdfPath, fs.readFileSync(pdfPath)); + let conversion = 0; + const spy = vi + .spyOn(markit, "convertFileWithMarkit") + .mockImplementation(async (_sourcePath, _signal, options) => { + conversion++; + if (options?.imageDir) { + fs.mkdirSync(options.imageDir, { recursive: true }); + fs.writeFileSync( + path.join(options.imageDir, "p11-img0.png"), + Buffer.concat([TINY_PNG, Buffer.from(String(conversion))]), + ); + } + return { ok: true, content: "" }; + }); + const tool = new ReadTool(makeSession(testDir)); + + const first = await tool.execute("call", { path: `${pdfPath}:p11-img0.png` }); + const second = await tool.execute("call", { path: `${otherPdfPath}:p11-img0.png` }); + + expect(imageBytes(first).subarray(TINY_PNG.length).toString()).toBe("1"); + expect(imageBytes(second).subarray(TINY_PNG.length).toString()).toBe("2"); + expect(spy).toHaveBeenCalledTimes(2); + }); + + it("supports PDF basenames at the filesystem component limit", async () => { + const longPdfPath = path.join(testDir, `${"a".repeat(250)}.pdf`); + fs.writeFileSync(longPdfPath, "%PDF-stub"); + mockExtraction(); + + const result = await new ReadTool(makeSession(testDir)).execute("call", { + path: `${longPdfPath}:p11-img0.png`, + }); + + expect(result.content.some(content => content.type === "image")).toBe(true); + }); + it("errors with the available members for an unknown member", async () => { mockExtraction(); const tool = new ReadTool(makeSession(testDir)); @@ -160,12 +395,20 @@ describe("read PDF image extraction", () => { }); it("does not cache a failed conversion", async () => { + let failedSnapshotPath: string | undefined; + let failedImageDir: string | undefined; const spy = vi.spyOn(markit, "convertFileWithMarkit"); - // First attempt fails and writes nothing → throws, leaves no `.extracted` marker. - spy.mockResolvedValueOnce({ ok: false, content: "", error: "boom" }); + spy.mockImplementationOnce(async (sourcePath, _signal, options) => { + failedSnapshotPath = sourcePath; + failedImageDir = options?.imageDir; + return { ok: false, content: "", error: "boom" }; + }); const tool = new ReadTool(makeSession(testDir)); await expect(tool.execute("call", { path: `${pdfPath}:p11-img0.png` })).rejects.toThrow(/Cannot extract images/); - // A later attempt succeeds and must re-run conversion (cache not poisoned). + if (!failedSnapshotPath || !failedImageDir) throw new Error("Expected failed extraction paths"); + expect(fs.existsSync(path.dirname(failedSnapshotPath))).toBe(false); + expect(fs.existsSync(path.join(failedImageDir, ".extracted"))).toBe(false); + spy.mockImplementationOnce(async (_filePath: string, _signal, options) => { if (options?.imageDir) { fs.mkdirSync(options.imageDir, { recursive: true }); diff --git a/packages/coding-agent/test/tts/streaming-player.test.ts b/packages/coding-agent/test/tts/streaming-player.test.ts new file mode 100644 index 000000000..69d1f53d4 --- /dev/null +++ b/packages/coding-agent/test/tts/streaming-player.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it } from "bun:test"; +import type { PlayerCommand } from "@oh-my-pi/pi-coding-agent/tts/player"; +import { StreamingAudioPlayer } from "@oh-my-pi/pi-coding-agent/tts/streaming-player"; + +/** A one-segment ~0.5s clip at 24 kHz, the shape `omp say "hi"` produces. */ +const clip = (): Float32Array => new Float32Array(24_000 / 2).fill(0.1); + +describe("StreamingAudioPlayer nonzero-exit fallback", () => { + it("replays the buffered clip via file playback when the streaming backend exits nonzero", async () => { + // `cat` drains stdin so the pipe write succeeds, then the shell exits 1 — + // exactly the bundled-ffmpeg-without-outdev failure that used to silently + // drop `omp say`'s single short clip. + const played: string[] = []; + const player = new StreamingAudioPlayer({ + commandsFor: (): PlayerCommand[] => [{ cmd: "sh", args: ["-c", "cat >/dev/null; exit 1"] }], + playAudio: async wavPath => { + played.push(wavPath); + }, + }); + player.start(24_000); + player.write(clip()); + await player.end(); + expect(played.length).toBe(1); + }); + + it("does not replay when the streaming backend exits cleanly", async () => { + const played: string[] = []; + const player = new StreamingAudioPlayer({ + commandsFor: (): PlayerCommand[] => [{ cmd: "sh", args: ["-c", "cat >/dev/null"] }], + playAudio: async wavPath => { + played.push(wavPath); + }, + }); + player.start(24_000); + player.write(clip()); + await player.end(); + expect(played.length).toBe(0); + }); + + it("plays every chunk via the file fallback when no streaming backend exists", async () => { + const played: string[] = []; + const player = new StreamingAudioPlayer({ + commandsFor: (): PlayerCommand[] => [], + playAudio: async wavPath => { + played.push(wavPath); + }, + }); + player.start(24_000); + player.write(clip()); + player.write(clip()); + await player.end(); + expect(played.length).toBe(2); + }); + + it("drops the replay buffer once the utterance exceeds the retention cap", async () => { + // Long input must not accumulate unbounded PCM; past the cap the + // nonzero-exit replay is forfeited rather than duplicating audio the + // backend already played. + const played: string[] = []; + const player = new StreamingAudioPlayer({ + commandsFor: (): PlayerCommand[] => [{ cmd: "sh", args: ["-c", "cat >/dev/null; exit 1"] }], + playAudio: async wavPath => { + played.push(wavPath); + }, + replayRetentionSeconds: 0.25, + }); + player.start(24_000); + player.write(clip()); + await player.end(); + expect(played.length).toBe(0); + }); +}); diff --git a/packages/collab-web/CHANGELOG.md b/packages/collab-web/CHANGELOG.md index 645bb966c..cc030e546 100644 --- a/packages/collab-web/CHANGELOG.md +++ b/packages/collab-web/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `xd://resolve`/`xd://reject`/`xd://propose` cards losing action metadata after the xdev unwrap (badge rendered `?`/warn instead of apply/discard/propose semantics), and registered the missing `reject`/`propose` renderers plus the hub-family aliases (`irc`, `job`, `await`, `poll`, `cancel_job`) so those transcript names no longer fall back to generic JSON. ([#5640](https://github.com/can1357/oh-my-pi/issues/5640)) + ## [17.0.8] - 2026-07-22 ### Fixed diff --git a/packages/collab-web/src/tool-render/registry.ts b/packages/collab-web/src/tool-render/registry.ts index 518e734d7..aa6045924 100644 --- a/packages/collab-web/src/tool-render/registry.ts +++ b/packages/collab-web/src/tool-render/registry.ts @@ -19,6 +19,8 @@ import { goalRenderer } from "./tools/goal"; import { grepRenderer } from "./tools/grep"; import { hubRenderer } from "./tools/hub"; import { inspectImageRenderer } from "./tools/inspect-image"; +import { ircRenderer } from "./tools/irc"; +import { jobRenderer } from "./tools/job"; import { lspRenderer } from "./tools/lsp"; import { recallRenderer } from "./tools/memory-recall"; import { reflectRenderer } from "./tools/memory-reflect"; @@ -55,6 +57,11 @@ const RENDERERS: Record = { goal: goalRenderer, inspect_image: inspectImageRenderer, hub: hubRenderer, + irc: ircRenderer, + job: jobRenderer, + await: jobRenderer, + poll: jobRenderer, + cancel_job: jobRenderer, lsp: lspRenderer, recall: recallRenderer, reflect: reflectRenderer, @@ -62,6 +69,8 @@ const RENDERERS: Record = { read: readRenderer, report_tool_issue: reportToolIssueRenderer, resolve: resolveRenderer, + reject: resolveRenderer, + propose: resolveRenderer, grep: grepRenderer, search: grepRenderer, task: taskRenderer, diff --git a/packages/collab-web/src/tool-render/tools/resolve.tsx b/packages/collab-web/src/tool-render/tools/resolve.tsx index d4d46a8fe..c6f3106ef 100644 --- a/packages/collab-web/src/tool-render/tools/resolve.tsx +++ b/packages/collab-web/src/tool-render/tools/resolve.tsx @@ -1,32 +1,91 @@ -/** `resolve` — apply or discard a pending preview/approval action. */ +/** + * `resolve` / `reject` / `propose` — finalize a staged preview (apply/discard) + * or submit a plan for approval. + * + * In collab-web these arrive as `write xd://` calls that `ToolView` + * unwraps: the card model then lives on the unwrapped inner details + * (`result.details`) and the device name (`props.name`). Historical top-level + * `resolve` transcripts instead carried `action`/`reason` on `args`, so read + * details first and fall back to args. When neither is present yet (a running + * card), the device name supplies the default action. + */ import type { ReactNode } from "react"; +import type { Tone } from "../parts"; import { Badge, Badges, Kv, KvGrid, Note, ResultText } from "../parts"; import type { ToolRenderer, ToolRenderProps } from "../types"; import { detailsRecord, isRecord, normalizeWs, str, truncate } from "../util"; -function Summary({ args, result }: ToolRenderProps): ReactNode { - const action = str(args.action); - const reason = str(args.reason); - const tone = result?.isError ? "err" : action === "apply" ? "ok" : "warn"; +type ResolveKind = "apply" | "discard" | "propose"; + +/** Short badge word (summary) and full transition badge (body) per card kind. */ +const KIND_WORD: Record = { apply: "apply", discard: "discard", propose: "propose" }; +const KIND_TRANSITION: Record = { + apply: "proposed → resolved", + discard: "proposed → rejected", + propose: "plan proposed", +}; +const KIND_TONE: Record = { apply: "ok", discard: "warn", propose: "accent" }; + +interface ResolveCard { + kind: ResolveKind; + tone: Tone; + /** Apply/discard reason. */ + reason: string | null; + /** Source tool that staged the preview (apply/discard). */ + sourceToolName: string | null; + /** Preview label (apply/discard). */ + label: string | null; + /** Plan title (propose). */ + title: string | null; + /** Plan artifact path (propose). */ + planFilePath: string | null; + /** Free-form extra metadata rows. */ + extra: Record | null; +} + +/** Derive the card model from unwrapped inner details, args fallback, and device name. */ +function cardModel({ name, args, result }: ToolRenderProps): ResolveCard { + const details = detailsRecord(result); + const explicit = str(details?.action) ?? str(args.action); + const kind: ResolveKind = + explicit === "apply" || explicit === "discard" + ? explicit + : name === "propose" + ? "propose" + : name === "reject" + ? "discard" + : "apply"; + const extra = isRecord(args.extra) ? args.extra : details && isRecord(details.extra) ? details.extra : null; + return { + kind, + tone: result?.isError ? "err" : KIND_TONE[kind], + reason: str(details?.reason) ?? str(args.reason), + sourceToolName: str(details?.sourceToolName) ?? str(args.sourceToolName), + label: str(details?.label) ?? str(args.label), + title: str(details?.title) ?? str(args.title), + planFilePath: str(details?.planFilePath) ?? str(args.planFilePath), + extra, + }; +} + +function Summary(props: ToolRenderProps): ReactNode { + const card = cardModel(props); + const trailing = card.kind === "propose" ? card.title : card.reason; return ( <> - {action ?? "?"} {reason && {truncate(normalizeWs(reason), 100)}} + {KIND_WORD[card.kind]}{" "} + {trailing && {truncate(normalizeWs(trailing), 100)}} ); } -function Body({ args, result }: ToolRenderProps): ReactNode { - const action = str(args.action); - const reason = str(args.reason); - const tone = result?.isError ? "err" : action === "apply" ? "ok" : "warn"; - const details = detailsRecord(result); - const sourceToolName = details ? str(details.sourceToolName) : null; - const label = details ? str(details.label) : null; - const extra = isRecord(args.extra) ? args.extra : details && isRecord(details.extra) ? details.extra : null; +function Body(props: ToolRenderProps): ReactNode { + const { result } = props; + const card = cardModel(props); const extraRows: ReactNode[] = []; - if (extra) { - for (const k in extra) { - const v = extra[k]; + if (card.extra) { + for (const k in card.extra) { + const v = card.extra[k]; let text: string; if (typeof v === "string") text = v; else { @@ -47,18 +106,22 @@ function Body({ args, result }: ToolRenderProps): ReactNode { <> - {action === "apply" - ? "proposed → resolved" - : action === "discard" - ? "proposed → rejected" - : (action ?? "?")} + + {KIND_TRANSITION[card.kind]} , - sourceToolName && {sourceToolName}, - label && {truncate(normalizeWs(label), 120)}, + card.sourceToolName && {card.sourceToolName}, + card.label && {truncate(normalizeWs(card.label), 120)}, + card.kind === "propose" && card.title && ( + {truncate(normalizeWs(card.title), 120)} + ), ]} /> - {reason && {reason}} + {card.kind !== "propose" && card.reason && {card.reason}} + {card.kind === "propose" && card.planFilePath && ( + + {card.planFilePath} + + )} {extraRows.length > 0 && {extraRows}} diff --git a/packages/collab-web/test/tool-view.test.tsx b/packages/collab-web/test/tool-view.test.tsx index 8a6016427..ac718b585 100644 --- a/packages/collab-web/test/tool-view.test.tsx +++ b/packages/collab-web/test/tool-view.test.tsx @@ -28,4 +28,127 @@ describe("ToolView xd:// dispatches", () => { expect(html).toContain("alpine lake"); expect(html).toContain('src="data:image/png;base64,aW1hZ2U="'); }); + + it("renders xd://resolve apply cards from unwrapped inner details", () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain("xd://resolve"); + expect(html).toContain("proposed → resolved"); + expect(html).toContain("tv-badge--ok"); + expect(html).toContain("ast_edit: edit foo.ts"); + // The historical args-only path once left the badge as a warn "?". + expect(html).not.toContain('tv-badge--warn">?'); + }); + + it("renders xd://reject discard cards with reject semantics", () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain("xd://reject"); + expect(html).toContain("proposed → rejected"); + expect(html).toContain("tv-badge--warn"); + // Not the generic JSON dump. + expect(html).not.toContain("tv-out-title"); + }); + + it("defaults a running xd://reject to discard before details arrive", () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain("proposed → rejected"); + }); + + it("renders xd://propose plan metadata from unwrapped inner details", () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain("xd://propose"); + expect(html).toContain("plan proposed"); + expect(html).toContain("local://ship-it-plan.md"); + }); + + it("keeps historical top-level resolve cards working from args.action", () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain("proposed → resolved"); + expect(html).toContain("tv-badge--ok"); + }); + + it("routes the hub-family alias irc through the messaging renderer", () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain("→ Main"); + // Not the generic JSON dump of the args. + expect(html).not.toContain("tv-out-title"); + }); + + it("routes the hub-family alias job through the job renderer", () => { + const html = renderToStaticMarkup( + , + ); + + expect(html).toContain("poll a1b2"); + expect(html).not.toContain("tv-out-title"); + }); }); diff --git a/packages/snapcompact/CHANGELOG.md b/packages/snapcompact/CHANGELOG.md index 35b58d674..6777da3ef 100644 --- a/packages/snapcompact/CHANGELOG.md +++ b/packages/snapcompact/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Added an `includeThinking` serialize option (default `true`) so callers can exclude assistant reasoning (`¶think:` sections) from archived transcripts; used to keep reproduced reasoning out of frames replayed to Anthropic-dialect models ([#6093](https://github.com/can1357/oh-my-pi/issues/6093)). + ## [16.5.0] - 2026-07-13 ### Changed diff --git a/packages/snapcompact/src/snapcompact.ts b/packages/snapcompact/src/snapcompact.ts index 443fe93f3..98bef8699 100644 --- a/packages/snapcompact/src/snapcompact.ts +++ b/packages/snapcompact/src/snapcompact.ts @@ -735,6 +735,12 @@ export interface SerializeOptions { /** Print tool-result text in dim gray ink so archived conversation reads * louder than archived tool noise. Defaults to `true`. */ dimToolResults?: boolean; + /** Serialize assistant reasoning as `¶think:` sections. Defaults to `true`. + * Callers archiving for a Claude/Anthropic-dialect model set this `false`: + * the archive frames are replayed as text into every later request, and + * reasoning rendered back to Claude trips its `reasoning_extraction` + * classifier (issue #6093). */ + includeThinking?: boolean; } /** Keep the head and tail of `text`, eliding the middle beyond `maxChars`. */ @@ -773,6 +779,7 @@ export function serializeConversation(messages: Message[], options?: SerializeOp const toolCallMaxChars = options?.toolCallMaxChars ?? TOOL_CALL_MAX_CHARS; const headRatio = options?.truncateHeadRatio ?? TRUNCATE_HEAD_RATIO; const dimToolResults = options?.dimToolResults !== false; + const includeThinking = options?.includeThinking !== false; const parts: string[] = []; let lastPrefix: string | null = null; @@ -845,6 +852,7 @@ export function serializeConversation(messages: Message[], options?: SerializeOp const text = stripDimMarkers(block.text); if (text.trim()) pendingText.push(text); } else if (block.type === "thinking") { + if (!includeThinking) continue; const thinking = stripDimMarkers(block.thinking); if (thinking.trim()) pendingThinking.push(thinking); } else if (block.type === "toolCall") { @@ -1833,6 +1841,29 @@ function planArchive(text: string, high: Shape, low: Shape, maxFrames: number): }; } +/** + * Drop `¶think:` sections from serialized archive source text. + * + * Archives written before {@link SerializeOptions.includeThinking} existed bake + * reasoning into their kept source; replaying it to Claude trips the + * `reasoning_extraction` classifier (issue #6093). Re-compaction re-renders the + * whole unfolded source, so scrubbing the prior text heals a poisoned session + * at its next compaction. Conservative by construction: only sections that + * start with `¶think:` at a section boundary are dropped. + */ +function stripThinkingSections(text: string): string { + return text + .split(NEWLINE_GLYPH) + .map(segment => + segment + .split(/\n\n(?=¶(?:user|think|ai|call):)/) + .filter(section => !section.startsWith("¶think:")) + .join("\n\n"), + ) + .filter(segment => segment.length > 0) + .join(NEWLINE_GLYPH); +} + /** * Run a snapcompact compaction over prepared messages. Fully local: serializes * the discarded history, appends it to the accumulated archive source text, and @@ -1858,11 +1889,18 @@ export async function compact( const llmMessages = (options?.convertToLlm ?? defaultConvertToLlm)(messages); const serialized = serializeConversation(llmMessages, options); const previousArchive = getPreservedArchive(previousPreserveData); - const previousText = + const previousTextRaw = previousArchive?.text ?? [previousArchive?.textHead, previousArchive?.textTail] .filter((part): part is string => typeof part === "string" && part.length > 0) .join(NEWLINE_GLYPH); + // Legacy archives may carry `¶think:` sections from before includeThinking + // existed; scrub them when this compaction excludes thinking so the + // re-rendered archive stops replaying reasoning (issue #6093). + const previousText = + options?.includeThinking === false && previousTextRaw.length > 0 + ? stripThinkingSections(previousTextRaw) + : previousTextRaw; const hasPreviousText = previousText.length > 0; const includedPreviousSummary = !hasPreviousText && !!previousSummary; const shapeProbeText = renderabilityProbeText(serialized, previousPreserveData, previousSummary); diff --git a/packages/snapcompact/test/snapcompact.test.ts b/packages/snapcompact/test/snapcompact.test.ts index 971041fd9..3ab1c0083 100644 --- a/packages/snapcompact/test/snapcompact.test.ts +++ b/packages/snapcompact/test/snapcompact.test.ts @@ -717,6 +717,21 @@ describe("serializeConversation", () => { expect(out).toBe("¶think:weigh options\n\n¶ai:the answer"); }); + it("drops ¶think reasoning sections when includeThinking is false but keeps the reply", () => { + const out = snapcompact.serializeConversation( + [ + createAssistantMessage([ + { type: "thinking", thinking: "private chain of thought" }, + { type: "text", text: "the answer" }, + ]), + ], + { includeThinking: false }, + ); + expect(out).not.toContain("¶think:"); + expect(out).not.toContain("private chain of thought"); + expect(out).toBe("¶ai:the answer"); + }); + it("gives a thinking-only turn its own heading before the tool calls", () => { const out = snapcompact.serializeConversation([ createAssistantMessage([ @@ -1050,6 +1065,57 @@ describe("compact", () => { expect(second.preserveData?.openaiRemoteCompaction).toBeUndefined(); expect(second.preserveData?.appKey).toBe("kept"); }); + + it("scrubs legacy ¶think: sections from the prior archive when thinking is excluded", async () => { + const first = await snapcompact.compact( + makePreparation({ + messagesToSummarize: [ + createUserMessage("Investigate the flaky auth test."), + createAssistantMessage([ + { type: "thinking", thinking: "legacy private chain of thought" }, + { type: "text", text: "The token clock is skewed." }, + ]), + ], + }), + { frameSize: TEST_FRAME_SIZE }, + ); + expect(snapcompact.getPreservedArchive(first.preserveData)?.text ?? "").toContain("¶think:"); + + const second = await snapcompact.compact( + makePreparation({ + messagesToSummarize: [createUserMessage("Continue after switching to Claude.")], + previousPreserveData: first.preserveData, + }), + { frameSize: TEST_FRAME_SIZE, includeThinking: false }, + ); + const archiveText = snapcompact.getPreservedArchive(second.preserveData)?.text ?? ""; + expect(archiveText).not.toContain("¶think:"); + expect(archiveText).not.toContain("legacy private chain of thought"); + expect(archiveText).toContain("Investigate the flaky auth test."); + expect(archiveText).toContain("The token clock is skewed."); + }); + + it("keeps legacy ¶think: sections when thinking stays included", async () => { + const first = await snapcompact.compact( + makePreparation({ + messagesToSummarize: [ + createAssistantMessage([ + { type: "thinking", thinking: "legacy private chain of thought" }, + { type: "text", text: "Visible reply." }, + ]), + ], + }), + { frameSize: TEST_FRAME_SIZE }, + ); + const second = await snapcompact.compact( + makePreparation({ + messagesToSummarize: [createUserMessage("Another turn.")], + previousPreserveData: first.preserveData, + }), + { frameSize: TEST_FRAME_SIZE }, + ); + expect(snapcompact.getPreservedArchive(second.preserveData)?.text ?? "").toContain("¶think:"); + }); }); describe("archive helpers", () => { diff --git a/packages/stats/CHANGELOG.md b/packages/stats/CHANGELOG.md index ec8e4f048..b5f132ea6 100644 --- a/packages/stats/CHANGELOG.md +++ b/packages/stats/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed malformed persisted content blocks aborting stats ingestion before later projects and settled pending full-session migrations after successful backfills ([#6373](https://github.com/can1357/oh-my-pi/issues/6373)). + ## [17.0.6] - 2026-07-20 ### Changed diff --git a/packages/stats/src/aggregator.ts b/packages/stats/src/aggregator.ts index fb37e9305..50fc25561 100644 --- a/packages/stats/src/aggregator.ts +++ b/packages/stats/src/aggregator.ts @@ -24,6 +24,7 @@ import { insertMessageStats, insertToolCalls, insertUserMessageStats, + markSessionBackfillsComplete, setFileOffset, updateToolResults, updateUserMessageLinks, @@ -209,12 +210,15 @@ export async function syncAllSessions(opts?: SyncOptions): Promise<{ processed: await initDb(); const files = await listAllSessionFiles(); - if (files.length === 0) return { processed: 0, files: 0 }; - let totalProcessed = 0; let filesProcessed = 0; let completed = 0; let cursor = 0; + const finish = () => { + markSessionBackfillsComplete(); + return { processed: totalProcessed, files: filesProcessed }; + }; + if (files.length === 0) return finish(); const report = (sessionFile: string) => { completed++; @@ -259,7 +263,7 @@ export async function syncAllSessions(opts?: SyncOptions): Promise<{ processed: for (const sessionFile of files) { await processFile(sessionFile, parseSessionFile); } - return { processed: totalProcessed, files: filesProcessed }; + return finish(); } const poolSize = Math.min(files.length, requestedWorkers); @@ -282,7 +286,7 @@ export async function syncAllSessions(opts?: SyncOptions): Promise<{ processed: for (const handle of handles) handle.worker.terminate(); } - return { processed: totalProcessed, files: filesProcessed }; + return finish(); } const HOUR_MS = 60 * 60 * 1000; diff --git a/packages/stats/src/db.ts b/packages/stats/src/db.ts index 7f24d1e72..085313545 100644 --- a/packages/stats/src/db.ts +++ b/packages/stats/src/db.ts @@ -1079,28 +1079,23 @@ function backfillPriorityPremiumRequests(database: Database): void { .run(PRIORITY_PREMIUM_REQUESTS_BACKFILL_KEY, BACKFILL_PENDING); } -export function markPriorityPremiumRequestsBackfillComplete(): void { +/** + * Settle every full-session backfill after a successful sync pass. + */ +export function markSessionBackfillsComplete(): void { if (!db) return; - db.prepare("INSERT OR REPLACE INTO meta (key, value) VALUES (?, ?)").run( - PRIORITY_PREMIUM_REQUESTS_BACKFILL_KEY, - BACKFILL_COMPLETE, - ); -} - -export function markUserMessagesBackfillComplete(): void { - if (!db) return; - db.prepare("INSERT OR REPLACE INTO meta (key, value) VALUES (?, ?)").run( - USER_MESSAGES_BACKFILL_KEY, - BACKFILL_COMPLETE, - ); -} - -export function markUserMessageLinksRepairComplete(): void { - if (!db) return; - db.prepare("INSERT OR REPLACE INTO meta (key, value) VALUES (?, ?)").run( - USER_MESSAGE_LINKS_REPAIR_KEY, - BACKFILL_COMPLETE, - ); + const markComplete = db.prepare("INSERT OR REPLACE INTO meta (key, value) VALUES (?, ?)"); + const apply = db.transaction(() => { + for (const key of [ + USER_MESSAGES_BACKFILL_KEY, + TOOL_CALLS_BACKFILL_KEY, + USER_MESSAGE_LINKS_REPAIR_KEY, + PRIORITY_PREMIUM_REQUESTS_BACKFILL_KEY, + ]) { + markComplete.run(key, BACKFILL_COMPLETE); + } + }); + apply(); } /** diff --git a/packages/stats/src/parser.ts b/packages/stats/src/parser.ts index 897de549f..b12725596 100644 --- a/packages/stats/src/parser.ts +++ b/packages/stats/src/parser.ts @@ -240,7 +240,11 @@ function extractToolCalls( const blocks = msg.content.filter( (block): block is ToolCall => - block.type === "toolCall" && typeof block.id === "string" && typeof block.name === "string", + block !== null && + typeof block === "object" && + block.type === "toolCall" && + typeof block.id === "string" && + typeof block.name === "string", ); if (blocks.length === 0) return []; @@ -277,7 +281,9 @@ function extractToolResultLink(sessionFile: string, entry: SessionMessageEntry): let resultChars = 0; if (Array.isArray(msg.content)) { for (const block of msg.content) { - if (block.type === "text" && typeof block.text === "string") resultChars += block.text.length; + if (block && typeof block === "object" && block.type === "text" && typeof block.text === "string") { + resultChars += block.text.length; + } } } return { diff --git a/packages/stats/test/behavior-backfill.test.ts b/packages/stats/test/behavior-backfill.test.ts index 135c3a214..055e7a201 100644 --- a/packages/stats/test/behavior-backfill.test.ts +++ b/packages/stats/test/behavior-backfill.test.ts @@ -95,4 +95,25 @@ describe("behavior backfill", () => { expect(getBehaviorOverall(null).totalMessages).toBe(1); expect(getFileOffset(sessionFile)).not.toBeNull(); }); + + it("marks full-session backfills complete after a successful sync", async () => { + await writeSessionFile(); + await syncAllSessions({ workers: 1 }); + closeDb(); + + const database = new Database(getStatsDbPath(), { readonly: true }); + const rows = database + .query( + "SELECT key, value FROM meta WHERE key IN ('user_messages_v8', 'tool_calls_v1', 'user_message_links_v1', 'premium_requests_priority_v1') ORDER BY key", + ) + .all() as { key: string; value: string }[]; + database.close(); + + expect(rows).toEqual([ + { key: "premium_requests_priority_v1", value: "complete" }, + { key: "tool_calls_v1", value: "complete" }, + { key: "user_message_links_v1", value: "complete" }, + { key: "user_messages_v8", value: "complete" }, + ]); + }); }); diff --git a/packages/stats/test/parser-malformed-entries.test.ts b/packages/stats/test/parser-malformed-entries.test.ts index ee18a19ef..033ac9558 100644 --- a/packages/stats/test/parser-malformed-entries.test.ts +++ b/packages/stats/test/parser-malformed-entries.test.ts @@ -98,6 +98,21 @@ describe("malformed session entries", () => { expect(result.stats.map(s => s.entryId)).toEqual(["ok"]); }); + it("ignores malformed content blocks without aborting later entries", async () => { + const file = await writeSession([ + assistantEntry("a1", { + content: [null, { type: "toolCall", id: "call-1", name: "bash", arguments: {} }], + usage: USAGE, + timestamp: 1752000000000, + }), + assistantEntry("a2", { content: [], usage: USAGE, timestamp: 1752000001000 }), + ]); + + const result = await parseSessionFile(file); + expect(result.stats.map(s => s.entryId)).toEqual(["a1", "a2"]); + expect(result.toolCalls.map(c => c.toolCallId)).toEqual(["call-1"]); + }); + it("keeps tool_calls insertable when the turn lacks a message timestamp", async () => { const file = await writeSession([ assistantEntry("a1", { diff --git a/packages/tui/CHANGELOG.md b/packages/tui/CHANGELOG.md index 13b39e774..7aebf4c8a 100644 --- a/packages/tui/CHANGELOG.md +++ b/packages/tui/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +### Added + +- `Text.setStyleFn()` applies a foreground styler at render time, so a component re-resolves its color after `invalidate()` instead of baking the palette that was active when it was constructed. + +### Fixed + +- Fixed teardown leaving the terminal in cursor-key/keypad application mode (DECCKM), which broke arrow keys in the parent shell after exit; `stop()` and `emergencyTerminalRestore()` now emit the standard `rmkx` resets ([#6374](https://github.com/can1357/oh-my-pi/issues/6374)). + ## [17.0.9] - 2026-07-23 ### Added diff --git a/packages/tui/src/components/text.ts b/packages/tui/src/components/text.ts index f18715eb5..753c60d35 100644 --- a/packages/tui/src/components/text.ts +++ b/packages/tui/src/components/text.ts @@ -11,13 +11,20 @@ import { } from "../utils"; /** - * Text component - displays multi-line text with word wrapping + * Text component - displays multi-line text with word wrapping. + * + * Foreground colors may be supplied lazily via {@link setStyleFn} instead of + * baked into `text`: the styler runs at render time, so a caller that + * invalidates the component on a theme change (see the coding-agent's + * `onThemeChange` handler) re-resolves the color against the now-active theme + * rather than replaying the palette active when the component was constructed. */ export class Text implements Component { #text: string; #paddingX: number; // Left/right padding #paddingY: number; // Top/bottom padding #customBgFn?: (text: string) => string; + #styleFn?: (text: string) => string; #ignoreTight = false; @@ -64,6 +71,21 @@ export class Text implements Component { this.#cachedLines = undefined; } + /** + * Supply a foreground styler applied to the text at render time (e.g. a + * theme color resolver). Unlike baking the color into `text`, the styler + * re-runs on every render, so invalidating the component after a theme + * change re-resolves the color against the active theme. + */ + setStyleFn(styleFn?: (text: string) => string): this { + this.#styleFn = styleFn; + this.#cachedText = undefined; + this.#cachedWidth = undefined; + this.#cachedWidthEpoch = undefined; + this.#cachedLines = undefined; + return this; + } + invalidate(): void { this.#cachedText = undefined; this.#cachedWidth = undefined; @@ -93,7 +115,7 @@ export class Text implements Component { } // Replace tabs with 3 spaces - const normalizedText = replaceTabs(this.#text); + const normalizedText = replaceTabs(this.#styleFn ? this.#styleFn(this.#text) : this.#text); // Calculate content width (subtract left/right margins) const paddingX = this.#ignoreTight ? this.#paddingX : getPaddingX(this.#paddingX); diff --git a/packages/tui/src/terminal.ts b/packages/tui/src/terminal.ts index d0e48aa92..4616d14b7 100644 --- a/packages/tui/src/terminal.ts +++ b/packages/tui/src/terminal.ts @@ -312,6 +312,7 @@ export function emergencyTerminalRestore(): void { process.stdout.write( "\x1b[?2026l" + // End synchronized output "\x1b[?7h" + // Restore autowrap + "\x1b[?1l\x1b>" + // Restore normal cursor-key + keypad mode (rmkx, #6374) "\x1b[?2004l" + // Disable bracketed paste "\x1b[?2031l" + // Disable Mode 2031 appearance notifications "\x1b[?2048l" + // Disable in-band resize notifications @@ -625,6 +626,15 @@ export class ProcessTerminal implements Terminal { // Enable bracketed paste mode - terminal will wrap pastes in \x1b[200~ ... \x1b[201~ this.#safeWrite("\x1b[?2004h"); + // Force normal cursor-key (DECCKM) and numeric-keypad mode (terminfo + // `rmkx` = "\x1b[?1l\x1b>"). omp decodes both CSI ("\x1b[A") and SS3 + // ("\x1bOA") arrow encodings, so it never enables application mode + // itself — but a prior program that left the TTY in application-cursor- + // keys mode makes arrows arrive as SS3. Normalizing on entry keeps input + // in the predictable default state; stop() restores the same on exit. + // See #6374. + this.#safeWrite("\x1b[?1l\x1b>"); + // Set up resize handler immediately. The OS refreshes process.stdout // dimensions before firing `resize`, so it is authoritative for geometry: // reconcile any stale cached DEC 2048 report before notifying the renderer. @@ -1376,6 +1386,13 @@ export class ProcessTerminal implements Terminal { // begin/end halves of a frame. Safe no-ops on terminals that ignored them. this.#safeWrite("\x1b[?2026l\x1b[?7h"); + // Restore normal cursor-key (DECCKM) and numeric-keypad mode (terminfo + // `rmkx`). Symmetric with the normalize in start(): a TTY-sharing child + // can leave the terminal in application-cursor-keys mode, and without + // this reset the parent shell inherits SS3 arrows so Up/Down history + // navigation stays broken after omp exits (#6374). + this.#safeWrite("\x1b[?1l\x1b>"); + // Disable bracketed paste mode this.#safeWrite("\x1b[?2004l"); this.#safeWrite("\x1b[?5522l"); diff --git a/packages/tui/test/process-terminal-headless.test.ts b/packages/tui/test/process-terminal-headless.test.ts index d7acd9126..34f925a3a 100644 --- a/packages/tui/test/process-terminal-headless.test.ts +++ b/packages/tui/test/process-terminal-headless.test.ts @@ -93,4 +93,30 @@ describe("ProcessTerminal headless suppression", () => { setTerminalHeadless(previous); } }); + + // #6374: arrows stopped working inside omp and stayed broken in the shell + // after exit — a missing cursor-key/keypad reset. omp owns the TTY and emits + // a full private-mode reset menu, but never restored normal cursor-key + // (DECCKM) / numeric-keypad mode (terminfo `rmkx` = "\x1b[?1l\x1b>"). If the + // terminal was left in application-cursor-keys mode, arrows arrived as SS3 + // and the parent shell's Up/Down history navigation broke. start() must + // normalize the state and stop() must restore it. + it("emits rmkx on start and stop to normalize/restore cursor-key + keypad mode (#6374)", () => { + const previous = setTerminalHeadless(false); + const terminal = new ProcessTerminal(); + try { + terminal.start( + () => {}, + () => {}, + ); + expect(writes.join("")).toContain("\x1b[?1l\x1b>"); + + writes.length = 0; + terminal.stop(); + expect(writes.join("")).toContain("\x1b[?1l\x1b>"); + } finally { + terminal.stop(); + setTerminalHeadless(previous); + } + }); }); diff --git a/packages/tui/test/text.test.ts b/packages/tui/test/text.test.ts index 74e81f0f6..5eae5bfde 100644 --- a/packages/tui/test/text.test.ts +++ b/packages/tui/test/text.test.ts @@ -9,4 +9,28 @@ describe("Text component", () => { expect(text.setText("b")).toBe(true); expect(text.getText()).toBe("b"); }); + + it("applies the style fn at render time, not construction time", () => { + const text = new Text("hello", 0, 0).setStyleFn(t => `${t}`); + expect(text.render(40).join("\n")).toContain("hello"); + }); + + it("re-resolves the style fn after invalidate so a theme change re-shapes", () => { + // The styler reads a mutable `color`, standing in for the active theme. + // The coding-agent invalidates status components on `onThemeChange`, so a + // lazily-styled Text must pick up the new color on the next render — + // something a baked ANSI string can never do (issue #6337). + let color = "RED"; + const text = new Text("hello", 0, 0).setStyleFn(t => `[${color}]${t}`); + expect(text.render(40).join("\n")).toContain("[RED]hello"); + + // Without invalidation the cached render is returned unchanged. + color = "BLUE"; + expect(text.render(40).join("\n")).toContain("[RED]hello"); + + text.invalidate(); + const out = text.render(40).join("\n"); + expect(out).toContain("[BLUE]hello"); + expect(out).not.toContain("[RED]hello"); + }); }); diff --git a/python/robomp/src/github_events.py b/python/robomp/src/github_events.py index 8af7a0e10..cf2b7029a 100644 --- a/python/robomp/src/github_events.py +++ b/python/robomp/src/github_events.py @@ -300,10 +300,13 @@ def route( if not isinstance(number, int): return RouteDecision("skip", None, repo, None, "issue missing number") key = issue_key(repo, number) - if action == "opened": + if action in ("opened", "reopened"): + # A reopen is submitter-attributable exactly like an open, and + # `finalized_issue_comment.md` promises re-triage on reopen, so it + # re-triages from scratch and spends the same per-user rate budget. login, assoc = _submitter_info(issue) return RouteDecision( - "queue", "triage_issue", repo, key, "issues.opened", submitter=login, association=assoc + "queue", "triage_issue", repo, key, f"issues.{action}", submitter=login, association=assoc ) if action == "closed": # Cleanup is a lifecycle event, not a user submission; no rate-limit subject. diff --git a/python/robomp/src/queue.py b/python/robomp/src/queue.py index d64e07d7f..9d1c711f9 100644 --- a/python/robomp/src/queue.py +++ b/python/robomp/src/queue.py @@ -382,7 +382,7 @@ class WorkerPool: "recovered": row.attempts >= 2, }, ) - if event == "issues" and action == "opened": + if event == "issues" and action in ("opened", "reopened"): await tasks.triage_issue( settings=self.settings, db=self.db, diff --git a/python/robomp/src/tasks.py b/python/robomp/src/tasks.py index ae77df876..c8f6dcee3 100644 --- a/python/robomp/src/tasks.py +++ b/python/robomp/src/tasks.py @@ -282,7 +282,8 @@ async def triage_issue( log.info("skip: triage on PR-like issue", extra={"repo": repo.full_name, "n": issue.number}) return key = issue_key(repo.full_name, issue.number) - if db.get_issue(key) is None: + existing = db.get_issue(key) + if existing is None: # First-time triage: bail if a PR (human or another bot) already # claims to close this issue via Closes/Fixes/Resolves syntax or # the Development panel. We never replay closing-PR detection on @@ -304,6 +305,12 @@ async def triage_issue( extra={"key": key, "prs": list(closing_prs)}, ) return + elif existing.state in ("merged", "closed", "abandoned"): + # Reopen of a finalized issue (issues.reopened): the prior branch is + # stale (merged/deleted), so tear the workspace down and branch afresh + # from default — the same teardown the maintainer directive-reopen uses. + log.info("reopen re-triage", extra={"key": key, "from_state": existing.state}) + await _run_workspace_op(sandbox.remove_workspace, repo=repo.full_name, number=issue.number) db.upsert_issue(key=key, repo=repo.full_name, number=issue.number, state="reproducing") clone_url = repo.clone_url workspace = await _run_workspace_op( diff --git a/python/robomp/tests/test_github_events.py b/python/robomp/tests/test_github_events.py index cbefdc034..35aa71dbf 100644 --- a/python/robomp/tests/test_github_events.py +++ b/python/robomp/tests/test_github_events.py @@ -53,6 +53,27 @@ def test_route_issue_opened_queues_triage() -> None: assert decision.issue_key == "octo/widget#4" +def test_route_issue_reopened_queues_triage() -> None: + # `finalized_issue_comment.md` promises re-triage on reopen; the router must + # queue it as a submitter-attributable triage (not drop it to the skip branch). + decision = route( + "issues", + { + "action": "reopened", + "issue": {"number": 4, "user": {"login": "alice"}, "author_association": "CONTRIBUTOR"}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + ) + assert decision.should_queue + assert decision.task == "triage_issue" + assert decision.issue_key == "octo/widget#4" + assert decision.reason == "issues.reopened" + assert decision.submitter == "alice" + assert decision.association == "CONTRIBUTOR" + + def test_route_skips_disallowed_repo() -> None: decision = route( "issues", diff --git a/python/robomp/tests/test_queue_dispatch.py b/python/robomp/tests/test_queue_dispatch.py index 6d739afce..ae460675a 100644 --- a/python/robomp/tests/test_queue_dispatch.py +++ b/python/robomp/tests/test_queue_dispatch.py @@ -48,6 +48,38 @@ def _pr_row(action: str, *, delivery: str = "pr1") -> EventRow: ) +def _issue_row(action: str, *, delivery: str = "is1") -> EventRow: + return EventRow( + delivery_id=delivery, + event_type="issues", + repo="octo/widget", + issue_key="octo/widget#4", + payload={"action": action, "issue": {"number": 4}}, + received_at="2026-01-01T00:00:00Z", + state="running", + attempts=1, + last_error=None, + ) + + +@pytest.mark.parametrize("action", ["opened", "reopened"]) +@pytest.mark.asyncio +async def test_dispatch_routes_issue_triage_actions_to_triage_issue( + settings: Settings, db: Database, monkeypatch: pytest.MonkeyPatch, action: str +) -> None: + """Every issue action `route` can queue for triage MUST reach `tasks.triage_issue`.""" + seen: list[str] = [] + + async def fake_triage_issue(*, payload, **_kwargs) -> None: + seen.append(str(payload.get("action"))) + + monkeypatch.setattr(tasks, "triage_issue", fake_triage_issue) + + await _make_pool(settings, db)._dispatch(_issue_row(action)) # noqa: SLF001 + + assert seen == [action] + + @pytest.mark.parametrize("action", ["opened", "reopened", "ready_for_review"]) @pytest.mark.asyncio async def test_dispatch_routes_pr_review_actions_to_review_pr( diff --git a/python/robomp/tests/test_tasks.py b/python/robomp/tests/test_tasks.py index 98304823b..3a0bf6a6a 100644 --- a/python/robomp/tests/test_tasks.py +++ b/python/robomp/tests/test_tasks.py @@ -167,3 +167,71 @@ async def test_run_workspace_op_logs_worker_exception_on_concurrent_cancel(caplo assert any(r.exc_info and r.exc_info[1] is boom for r in warnings), ( "the worker's exception was not attached to the warning" ) + + +async def test_triage_issue_reopen_tears_down_finalized_workspace(db, settings, monkeypatch, tmp_path): + """Re-triage of a finalized (reopened) issue must clear the stale workspace first. + + The prior branch was merged/deleted when the issue finalized, so a reopen has + to branch afresh — mirroring the maintainer directive-reopen teardown. + """ + + async def _resolve_repo_and_issue(_github, _payload): + repo = RepoInfo( + full_name="octo/widget", + default_branch="main", + clone_url="https://x/octo/widget.git", + private=False, + ) + issue = IssueInfo( + repo="octo/widget", + number=1, + title="bug", + body="b", + state="open", + author="alice", + labels=(), + is_pull_request=False, + ) + return repo, issue + + monkeypatch.setattr(tasks, "_resolve_repo_and_issue", _resolve_repo_and_issue) + + # The bot previously finalized this issue: a stale row + workspace exist. + db.upsert_issue(key="octo/widget#1", repo="octo/widget", number=1, state="closed") + + calls: list[str] = [] + + def _remove(**_kwargs): + calls.append("remove") + + def _ensure(**_kwargs): + calls.append("ensure") + return SimpleNamespace(branch="farm/x/y", session_dir=str(tmp_path / "sess")) + + async def _fail_closing(*_a, **_k): + raise AssertionError("closing-PR guard must not run when a DB row already exists") + + github = SimpleNamespace(list_closing_pull_requests=_fail_closing) + sandbox = SimpleNamespace(natives_cache=None, ensure_workspace=_ensure, remove_workspace=_remove) + + async def _noop_run_task(**_kwargs): + return None + + monkeypatch.setattr(tasks, "run_task", _noop_run_task) + + await tasks.triage_issue( + settings=settings, + db=db, + github=github, + sandbox=sandbox, + git_transport=SimpleNamespace(), + payload={}, + delivery_id="d1", + ) + + # Teardown must precede re-provisioning, and the row resets to a live state. + assert calls == ["remove", "ensure"] + row = db.get_issue("octo/widget#1") + assert row is not None + assert row.state == "reproducing"