diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 843bc8bc0..069029d08 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `AppendOnlyContextManager.syncMessages` clearing the entire log on any in-place rewrite of an already-synced message. Per-turn tool-output pruning, image stripping, or any `transformContext` re-render that touched a single message used to drop every prior turn out of the append-only log and re-send the conversation from scratch, forcing local backends (llama.cpp / Ollama / LM Studio) to re-prefill tens of thousands of tokens every few turns. `syncMessages` now finds the longest byte-stable prefix between the previously-synced messages and the new ones, truncates the log to that prefix, and only re-appends the diverged tail — so the provider's KV cache stays warm up to the divergence point. ([#3406](https://github.com/can1357/oh-my-pi/issues/3406)) + ## [16.1.17] - 2026-06-24 ### Fixed diff --git a/packages/agent/src/append-only-context.ts b/packages/agent/src/append-only-context.ts index 898489b9c..3e5498272 100644 --- a/packages/agent/src/append-only-context.ts +++ b/packages/agent/src/append-only-context.ts @@ -132,6 +132,15 @@ export class AppendOnlyLog { return this.#entries; } + /** Drop entries past index `count`, keeping the first `count` byte-stable. + * Used by {@link AppendOnlyContextManager.syncMessages} to preserve the + * already-on-the-wire prefix when a later message diverges. */ + truncate(count: number): void { + if (count < 0) count = 0; + if (count >= this.#entries.length) return; + this.#entries.length = count; + } + clear(): void { this.#entries = []; } @@ -162,8 +171,14 @@ export class AppendOnlyContextManager { readonly log = new AppendOnlyLog(); /** How many normalized messages were synced into the log as of the last sync. */ #lastSyncCount = 0; - /** Rolling digest of synced message content — detects in-place rewrites. */ - #syncedDigest = 0; + /** + * Per-message digests of the synced log. Lets a deep or tail rewrite + * (per-turn pruning, image strip, transformContext re-render) preserve + * the byte-stable prefix instead of re-sending the entire conversation + * — keeps the provider's prompt-cache hit rate up to the divergence + * point on every subsequent turn. + */ + #messageDigests: number[] = []; build(context: AgentContext, options: BuildOptions): Context { this.prefix.build(context, options); @@ -174,33 +189,51 @@ export class AppendOnlyContextManager { /** * Sync normalized (provider-level) messages into the append-only log. * - * Detects both compaction (shorter array) and in-place rewrites - * (same length, changed content via a rolling digest). + * Three cases: + * + * 1. **Append**: same prefix, new tail → push the new entries. + * 2. **Compaction**: shorter array → clear the log and replay. + * 3. **In-place rewrite** (per-turn pruning, transformContext re-render, + * image strip, etc.): find the longest byte-stable prefix between + * the previously-synced messages and the new ones, drop the log + * down to that prefix, then append the diverged tail. Earlier + * revisions cleared the whole log on any digest change, which on + * llama.cpp / local backends forced a full ~40k-token re-prefill + * every turn that an extension, prune pass, or steering re-wrap + * rewrote a single message (#3406). Preserving the stable prefix + * lets the provider's KV cache stay warm up to the divergence + * point — the model only re-prefills from the changed message on. */ syncMessages(normalizedMessages: any[]): void { - // Detect in-place rewrites of already-synced messages. - if ( - this.#lastSyncCount > 0 && - this.#lastSyncCount <= normalizedMessages.length && - this.#computeDigest(normalizedMessages.slice(0, this.#lastSyncCount)) !== this.#syncedDigest - ) { - this.log.clear(); - this.#lastSyncCount = 0; - } - - // Compaction — array shrunk. + // Compaction (array shrunk) — every previously-synced message is gone, + // so the log can't carry any byte-stable bytes forward. if (normalizedMessages.length < this.#lastSyncCount) { this.log.clear(); this.#lastSyncCount = 0; + this.#messageDigests = []; } - const newMsgs = normalizedMessages.slice(this.#lastSyncCount); - for (const msg of newMsgs) { + // In-place rewrite: trim the log down to the longest byte-stable prefix + // that both the previous sync and the new messages share. Bound it by + // the current log length because `log.clear()` is public; direct clears + // (advisor reset) can leave the sync cursor ahead of the physical log. + // Anything past that point will be re-appended below with the new bytes. + if (this.#lastSyncCount > 0) { + const stableCount = Math.min(this.#longestStablePrefix(normalizedMessages), this.log.length); + if (stableCount < this.#lastSyncCount) { + this.log.truncate(stableCount); + this.#lastSyncCount = stableCount; + this.#messageDigests.length = stableCount; + } + } + + // Append the diverged tail (or the full delta on a normal turn). + for (let i = this.#lastSyncCount; i < normalizedMessages.length; i++) { + const msg = normalizedMessages[i]; this.log.append(msg); + this.#messageDigests.push(this.#messageDigest(msg)); } - this.#lastSyncCount = normalizedMessages.length; - this.#syncedDigest = this.#computeDigest(normalizedMessages); } /** Reset prefix + log for a model/provider switch while mode stays active. */ @@ -208,14 +241,14 @@ export class AppendOnlyContextManager { this.prefix.invalidate(); this.log.clear(); this.#lastSyncCount = 0; - this.#syncedDigest = 0; + this.#messageDigests = []; } /** Reset the sync cursor AND clear the log. */ resetSyncCursor(): void { this.log.clear(); this.#lastSyncCount = 0; - this.#syncedDigest = 0; + this.#messageDigests = []; } appendMessage(message: any): void { @@ -234,34 +267,45 @@ export class AppendOnlyContextManager { this.prefix.invalidate(); this.log.clear(); this.#lastSyncCount = 0; - this.#syncedDigest = 0; + this.#messageDigests = []; this.prefix.build(context, options); } - /** - * Deterministic digest over every field the provider may serialize — role, - * content, tool calls (both `toolCalls` and OpenAI-wire `tool_calls`), - * `tool_call_id`, `name`, `id`. Hashed with the same FNV-style rolling - * accumulator so in-place rewrites of *any* of these fields are visible. - */ - #computeDigest(messages: readonly unknown[]): number { - let hash = 0; - for (let i = 0; i < messages.length; i++) { - const msg = messages[i]; - if (!msg || typeof msg !== "object") continue; - const m = msg as Record; - const payload = JSON.stringify({ - r: m.role ?? null, - c: m.content ?? null, - tc: m.toolCalls ?? m.tool_calls ?? null, - tcid: m.tool_call_id ?? null, - n: m.name ?? null, - id: m.id ?? null, - }); - for (let j = 0; j < payload.length; j++) { - hash = ((hash << 5) - hash + payload.charCodeAt(j)) | 0; + /** Index of the first message whose serialized bytes differ from the + * previously-synced log; equals `min(lastSyncCount, normalizedMessages.length)` + * when nothing diverged. */ + #longestStablePrefix(normalizedMessages: readonly unknown[]): number { + const bound = Math.min(this.#lastSyncCount, normalizedMessages.length); + for (let i = 0; i < bound; i++) { + if (this.#messageDigest(normalizedMessages[i]) !== this.#messageDigests[i]) { + return i; } } + return bound; + } + + /** Deterministic digest over every field the provider may serialize — role, + * content, provider-native replay payloads, tool calls (both `toolCalls` and + * OpenAI-wire `tool_calls`), tool-result ids/names/error flags (both internal + * camelCase and wire snake_case), and assistant `id` — so an in-place rewrite + * of *any* of these fields is visible to {@link #longestStablePrefix}. */ + #messageDigest(msg: unknown): number { + if (!msg || typeof msg !== "object") return 0; + const m = msg as Record; + const payload = JSON.stringify({ + r: m.role ?? null, + c: m.content ?? null, + pp: m.providerPayload ?? null, + tc: m.toolCalls ?? m.tool_calls ?? null, + tcid: m.toolCallId ?? m.tool_call_id ?? null, + tn: m.toolName ?? m.name ?? null, + err: m.isError ?? null, + id: m.id ?? null, + }); + let hash = 0; + for (let j = 0; j < payload.length; j++) { + hash = ((hash << 5) - hash + payload.charCodeAt(j)) | 0; + } return hash >>> 0; } } diff --git a/packages/agent/test/append-only-context.test.ts b/packages/agent/test/append-only-context.test.ts index 585752af2..5424ceb7c 100644 --- a/packages/agent/test/append-only-context.test.ts +++ b/packages/agent/test/append-only-context.test.ts @@ -511,36 +511,191 @@ describe("message sync", () => { expect(result.messages[0]!.content).toBe("fresh"); }); - it("detects in-place rewrite of already-synced messages", () => { + it("preserves the byte-stable prefix when a deep message is rewritten (#3406)", () => { const mgr = new AppendOnlyContextManager(); mgr.build(makeContext(), BUILD_OPTS); - // Sync two messages - mgr.syncMessages([ - { role: "user", content: "q1" }, - { role: "assistant", content: "original long result" }, - ]); + const original0 = { role: "user", content: "q1" } as any; + const original1 = { role: "assistant", content: "original long result" } as any; + mgr.syncMessages([original0, original1]); expect(mgr.log.length).toBe(2); - // Same length, but second message content changed (simulates tool-output pruning) + // Same length, but the second message's content changed (simulates per-turn + // tool-output pruning / transformContext re-render). mgr.syncMessages([ { role: "user", content: "q1" }, { role: "assistant", content: "[pruned]" }, - ]); - // Log should have been reset and re-synced with the new content + ] as any); expect(mgr.log.length).toBe(2); - const msgs = mgr.build(makeContext(), BUILD_OPTS).messages; - expect(msgs[1]!.content).toBe("[pruned]"); + + const entries = mgr.log.entries(); + // The first message MUST keep its on-the-wire identity — that's what + // stops llama.cpp from re-prefilling the entire prior context. + expect(entries[0]).toBe(original0); + // The diverged tail is re-synced with the new bytes. + expect((entries[1] as { content: unknown }).content).toBe("[pruned]"); }); - it("detects in-place rewrite via digest mismatch", () => { + it("detects tool-result metadata-only rewrites before preserving a later prefix (#3406)", () => { + const mgr = new AppendOnlyContextManager(); + mgr.build(makeContext(), BUILD_OPTS); + + const original0 = { role: "user", content: "q1" } as any; + const original1 = { + role: "toolResult", + content: [{ type: "text", text: "same output" }], + toolCallId: "old-call", + toolName: "read", + isError: false, + } as any; + const original2 = { role: "assistant", content: "a1" } as any; + mgr.syncMessages([original0, original1, original2]); + + mgr.syncMessages([ + { role: "user", content: "q1" }, + { + role: "toolResult", + content: [{ type: "text", text: "same output" }], + toolCallId: "new-call", + toolName: "write", + isError: true, + }, + { role: "assistant", content: "a1-pruned" }, + ] as any); + + const entries = mgr.log.entries(); + expect(entries).toHaveLength(3); + expect(entries[0]).toBe(original0); + expect((entries[1] as { toolCallId: unknown }).toolCallId).toBe("new-call"); + expect((entries[1] as { toolName: unknown }).toolName).toBe("write"); + expect((entries[1] as { isError: unknown }).isError).toBe(true); + expect((entries[2] as { content: unknown }).content).toBe("a1-pruned"); + }); + + it("detects providerPayload-only rewrites before preserving a later prefix (#3406)", () => { + const mgr = new AppendOnlyContextManager(); + mgr.build(makeContext(), BUILD_OPTS); + + const original0 = { role: "user", content: "q1" } as any; + const original1 = { + role: "assistant", + content: [{ type: "text", text: "same visible output" }], + id: "assistant-1", + providerPayload: { + type: "openaiResponsesHistory", + provider: "openai", + items: [{ type: "message", role: "assistant", content: [{ type: "output_text", text: "old native" }] }], + }, + } as any; + const original2 = { role: "user", content: "q2" } as any; + mgr.syncMessages([original0, original1, original2]); + + mgr.syncMessages([ + { role: "user", content: "q1" }, + { + role: "assistant", + content: [{ type: "text", text: "same visible output" }], + id: "assistant-1", + providerPayload: { + type: "openaiResponsesHistory", + provider: "openai", + items: [{ type: "message", role: "assistant", content: [{ type: "output_text", text: "new native" }] }], + }, + }, + { role: "user", content: "q2-rewritten" }, + ] as any); + + const entries = mgr.log.entries(); + expect(entries).toHaveLength(3); + expect(entries[0]).toBe(original0); + expect( + (entries[1] as { providerPayload?: { items?: Array<{ content?: Array<{ text?: string }> }> } }).providerPayload + ?.items?.[0]?.content?.[0]?.text, + ).toBe("new native"); + expect((entries[2] as { content: unknown }).content).toBe("q2-rewritten"); + }); + + it("does not reuse a stable prefix longer than the current log after direct log clear (#3406)", () => { + const mgr = new AppendOnlyContextManager(); + mgr.build(makeContext(), BUILD_OPTS); + + mgr.syncMessages([ + { role: "user", content: "q1" }, + { role: "assistant", content: "a1" }, + ] as any); + expect(mgr.log.length).toBe(2); + + // Public log clear used by advisor reset: it intentionally empties the + // provider-bound message log but does not touch the private sync cursor. + mgr.log.clear(); + expect(mgr.log.length).toBe(0); + + mgr.syncMessages([ + { role: "user", content: "q1" }, + { role: "assistant", content: "a1-rewritten" }, + ] as any); + + const entries = mgr.log.entries(); + expect(entries).toHaveLength(2); + expect((entries[0] as { content: unknown }).content).toBe("q1"); + expect((entries[1] as { content: unknown }).content).toBe("a1-rewritten"); + }); + + it("preserves the prefix when the tail is rewritten (#3406)", () => { + const mgr = new AppendOnlyContextManager(); + mgr.build(makeContext(), BUILD_OPTS); + + const original0 = { role: "user", content: "q1" } as any; + const original1 = { role: "assistant", content: "a1" } as any; + const original2 = { role: "user", content: "q2" } as any; + mgr.syncMessages([original0, original1, original2]); + + // Tail-only rewrite (e.g. per-turn pruning of the most recent tool result): + // the first two messages MUST stay byte-stable; only the tail re-syncs. + mgr.syncMessages([ + { role: "user", content: "q1" }, + { role: "assistant", content: "a1" }, + { role: "user", content: "q2-rewritten" }, + ] as any); + + const entries = mgr.log.entries(); + expect(entries).toHaveLength(3); + expect(entries[0]).toBe(original0); + expect(entries[1]).toBe(original1); + expect((entries[2] as { content: unknown }).content).toBe("q2-rewritten"); + }); + + it("appended new messages keep the prefix stable even when the prior tail also diverged (#3406)", () => { + const mgr = new AppendOnlyContextManager(); + mgr.build(makeContext(), BUILD_OPTS); + + const original0 = { role: "user", content: "q1" } as any; + const original1 = { role: "assistant", content: "a1" } as any; + mgr.syncMessages([original0, original1]); + + // Re-sync with: (a) message #1 rewritten in place; (b) a brand-new tail + // appended. The prefix [original0] MUST stay byte-stable. + mgr.syncMessages([ + { role: "user", content: "q1" }, + { role: "assistant", content: "a1-pruned" }, + { role: "user", content: "q2" }, + ] as any); + + const entries = mgr.log.entries(); + expect(entries).toHaveLength(3); + expect(entries[0]).toBe(original0); + expect((entries[1] as { content: unknown }).content).toBe("a1-pruned"); + expect((entries[2] as { content: unknown }).content).toBe("q2"); + }); + + it("rewriting the first message still re-syncs from scratch", () => { const mgr = new AppendOnlyContextManager(); mgr.build(makeContext(), BUILD_OPTS); mgr.syncMessages([{ role: "user", content: "hello" }]); expect(mgr.log.length).toBe(1); - // Content changed but length same + // No byte-stable prefix — the only message diverged. mgr.syncMessages([{ role: "user", content: "world" }]); const msgs = mgr.build(makeContext(), BUILD_OPTS).messages; diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 17926f4bb..a1165903b 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -2,6 +2,23 @@ ## [Unreleased] +### Added + +- Added `listOAuthAccounts` for retrieving a read-only list of stored OAuth account identities +- Added `getOAuthAccessAt` to resolve an OAuth token exclusively for a specific account position + +### Changed + +- Refactored OAuth token persistence and disable logic to use stable credential IDs instead of positional indices to prevent race conditions during concurrent updates +- Updated OAuth failure classification to treat 403 status codes, rate limits, and network errors as transient, preventing unnecessary credential invalidation + +### Fixed + +- Fixed Codex Responses Lite staying enabled for image prompts, which caused GPT/Codex image turns to be rejected as `Invalid value: 'input_image'`; image-bearing Codex requests now fall back to the full Responses transport. ([#3421](https://github.com/can1357/oh-my-pi/issues/3421)) +- Fixed the auth-broker background refresher disabling OAuth credentials unconditionally (`disableCredentialById`) on a definitive refresh failure, so a credential another process or a fresh login rotated mid-refresh could be torn down even though the stored row already held a valid token. The definitive-failure teardown now happens inside `AuthStorage.refreshCredentialById` via the same compare-and-set the in-stream and usage-probe paths use — it disables only when the persisted row still matches the credential the refresh actually attempted, and reloads on a CAS loss; the refresher now only logs. +- Fixed OAuth refresh persisting the rotated token by a positional index captured before the refresh `await`. A concurrent disable could reorder or shrink a provider's credential array while the refresh was in flight, landing the new token on the wrong row (or silently dropping it) and leaving accounts with a stale refresh token that failed — and was then disabled — on the next cycle. Refresh persistence, selection-index resync, and CAS-disable now address the row by id across `forceRefreshCredentialById`, candidate preflight, and in-stream selection (`#replaceCredentialById` / `#disableCredentialByIdIfMatches`). +- Fixed `isDefinitiveOAuthFailure` treating a bare HTTP 403 (and generic `unauthorized` / access-token-expired wording) as a definitive credential failure, which permanently disabled healthy OAuth accounts on WAF, egress rate-limit, permission, and account-verification responses. Bare 403, rate limits (429), gateway/5xx, and more network errors (`ECONNRESET`, `ETIMEDOUT`, `EAI_AGAIN`, …) are now classified transient; only explicit dead-grant errors (`invalid_grant`, `invalid_token`, `unauthorized_client`, revoked, `refresh token … expired`) or a bare 401 tear the credential down. + ## [16.1.17] - 2026-06-24 ### Added @@ -4090,4 +4107,4 @@ _Dedicated to Peter's shoulder ([@steipete](https://twitter.com/steipete))_ ## [0.9.4] - 2025-11-26 -Initial release with multi-provider LLM support. +Initial release with multi-provider LLM support. \ No newline at end of file diff --git a/packages/ai/src/auth-broker/refresher.ts b/packages/ai/src/auth-broker/refresher.ts index 551f0a405..5bb4c0e25 100644 --- a/packages/ai/src/auth-broker/refresher.ts +++ b/packages/ai/src/auth-broker/refresher.ts @@ -5,9 +5,10 @@ * any whose `expires - Date.now() < refreshSkewMs`. Refresh single-flight * lives in {@link AuthStorage} so manual and background refreshes share the * same upstream attempt. - * Definitively-failed credentials (invalid_grant / 401 not from network blip) - * are disabled via {@link AuthStorage.disableCredentialById} so the next - * snapshot pull surfaces a clean delete on the client. + * Definitively-failed credentials (invalid_grant / bare 401, not a network + * blip) are torn down inside {@link AuthStorage.refreshCredentialById} via a + * compare-and-set disable — only when no peer/login rotated the row first — so + * the next snapshot pull surfaces a clean delete on the client. */ import { logger } from "@oh-my-pi/pi-utils"; import { type AuthStorage, isDefinitiveOAuthFailure } from "../auth-storage"; @@ -104,11 +105,10 @@ export class AuthBrokerRefresher { } catch (error) { const errorMsg = String(error); if (isDefinitiveOAuthFailure(errorMsg)) { - logger.warn("auth-broker refresh failed definitively; disabling credential", { - id, - error: errorMsg, - }); - this.#storage.disableCredentialById(id, `auth-broker refresh failed: ${errorMsg}`); + // AuthStorage.refreshCredentialById already CAS-disabled the row + // (unless a peer/login rotated it first, in which case the live + // credential is intentionally kept). Nothing to do here but record it. + logger.warn("auth-broker refresh failed definitively", { id, error: errorMsg }); } else { logger.debug("auth-broker refresh failed (transient)", { id, error: errorMsg }); } diff --git a/packages/ai/src/auth-storage.ts b/packages/ai/src/auth-storage.ts index 7c7f903da..a5711ee34 100644 --- a/packages/ai/src/auth-storage.ts +++ b/packages/ai/src/auth-storage.ts @@ -568,9 +568,17 @@ const MAX_PENDING_DISABLED_EVENTS = 32; * while streaming requests correctly tear the row down. */ const OAUTH_DEFINITIVE_FAILURE_REGEX = - /invalid_grant|invalid_token|revoked|unauthorized|expired.*refresh|refresh.*expired/i; -const OAUTH_TRANSIENT_FAILURE_REGEX = /timeout|network|fetch failed|ECONNREFUSED/i; -const OAUTH_HTTP_AUTH_REGEX = /\b(401|403)\b/; + /invalid_grant|invalid_token|unauthorized_client|\brevoked\b|refresh[\s_]?token.*expired/i; +// Transient: network blips, rate limits, gateway/5xx, and infra denials +// (WAF / egress 403, permission / account-verification) — block-and-retry, +// never tear the credential down for these. +const OAUTH_TRANSIENT_FAILURE_REGEX = + /timeout|network|fetch failed|ECONN(?:REFUSED|RESET)|ETIMEDOUT|EAI_AGAIN|socket hang up|\b(?:408|425|429|5\d{2})\b|rate.?limit|too many requests|temporar|unavailable|forbidden|permission_denied|cloudflare|captcha/i; +// A bare 401 from an OAuth token endpoint means the stored grant/client is +// dead. 403 is deliberately excluded: it is overwhelmingly WAF / egress +// rate-limit / permission / account-verification — none of which mean the +// refresh token itself is invalid. +const OAUTH_HTTP_AUTH_REGEX = /\b401\b/; export function isDefinitiveOAuthFailure(errorMsg: string): boolean { if (OAUTH_DEFINITIVE_FAILURE_REGEX.test(errorMsg)) return true; @@ -673,6 +681,20 @@ export interface OAuthAccountIdentity { } export type OAuthAccessResolution = ({ ok: true } & OAuthAccess) | ({ ok: false } & OAuthAccessFailure); + +/** + * Read-only identity of one stored OAuth account, in stable storage order. + * Returned by {@link AuthStorage.listOAuthAccounts}; `position` (0-based) is the + * selector accepted by {@link AuthStorage.getOAuthAccessAt}. + */ +export interface OAuthAccountSummary { + position: number; + credentialId: number; + accountId?: string; + email?: string; + projectId?: string; + enterpriseUrl?: string; +} export interface InvalidateCredentialMatchingOptions { signal?: AbortSignal; sessionId?: string; @@ -887,6 +909,7 @@ class AuthStorageUsageCache implements UsageCache { type StoredCredential = { id: number; credential: AuthCredential }; type OAuthSelection = { credential: OAuthCredential; index: number }; +type StoredOAuthSelection = { credentialId: number; credential: OAuthCredential; index: number }; type OAuthCandidate = { selection: OAuthSelection; @@ -1519,6 +1542,42 @@ export class AuthStorage { return true; } + /** + * Persist a refreshed credential addressed by id, not a positional index. + * A concurrent disable can reorder/shrink the provider's row array while an + * async refresh is in flight, so a pre-await index is unsafe; resolving the + * row by id at write time lands the rotated token on the correct row. Returns + * the row's current index, or -1 when it was disabled/removed mid-refresh. + */ + #replaceCredentialById(provider: string, id: number, credential: AuthCredential): number { + const entries = this.#getStoredCredentials(provider); + const index = entries.findIndex(entry => entry.id === id); + if (index === -1) return -1; + this.#store.updateAuthCredential(id, credential); + const updated = [...entries]; + updated[index] = { id, credential }; + this.#setStoredCredentials(provider, updated); + return index; + } + + /** + * CAS-disable the row with `id`, but only if its persisted credential still + * matches `expected` — i.e. no peer/login rotated it while we refreshed. + * Addresses the row by id (re-resolved here, then matched on `data` in the + * store) so a concurrent reorder can't tear down the wrong credential. + */ + #disableCredentialByIdIfMatches( + provider: string, + id: number, + expected: AuthCredential, + disabledCause: string, + ): boolean { + const entries = this.#getStoredCredentials(provider); + const index = entries.findIndex(entry => entry.id === id); + if (index === -1) return false; + return this.#tryDisableCredentialAtIfMatches(provider, index, expected, disabledCause); + } + #emitCredentialDisabled(event: CredentialDisabledEvent): void { if (this.#credentialDisabledListeners.size === 0) { // No subscribers — buffer for later replay. Cap the backlog so a process that runs @@ -3278,7 +3337,12 @@ export class AuthStorage { type: "oauth", }; candidate.selection.credential = updated; - this.#replaceCredentialAt(provider, candidate.selection.index, updated); + if (credentialId !== undefined) { + const idx = this.#replaceCredentialById(provider, credentialId, updated); + if (idx !== -1) candidate.selection.index = idx; + } else { + this.#replaceCredentialAt(provider, candidate.selection.index, updated); + } } catch (error) { // Recovery for definitive failures (incl. peer rotation) lives in // #tryOAuthCredential; log instead of swallowing silently — a bare @@ -3460,6 +3524,8 @@ export class AuthStorage { strategy?: CredentialRankingStrategy; rankingContext?: CredentialRankingContext; blockScope?: string; + /** When false, a definitive failure of THIS credential returns undefined instead of falling back to the ranked/round-robin selector (target-only resolution). */ + allowFallback?: boolean; }, ): Promise { const { @@ -3471,6 +3537,7 @@ export class AuthStorage { strategy, rankingContext, blockScope, + allowFallback = true, } = usageOptions; if (!allowBlocked && this.#isCredentialBlocked(providerKey, selection.index, blockScope)) { return undefined; @@ -3479,6 +3546,11 @@ export class AuthStorage { if (!(await this.#prepareOAuthCredentialForRequest(provider, selection, options))) { return undefined; } + // Capture the row id once, immediately after #prepareOAuthCredentialForRequest + // resynced selection.index from the store. A concurrent disable during the + // usage/refresh awaits below can shift positional indices, so every later + // refresh / persist / CAS-disable addresses the row by this stable id. + const credentialId = this.#getStoredCredentials(provider)[selection.index]?.id; const requiresProModel = requiresOpenAICodexProModel(provider, options?.modelId); const applyProFilter = enforceProRequirement ?? requiresProModel; @@ -3521,7 +3593,7 @@ export class AuthStorage { const refreshedCredentials = await this.#refreshOAuthCredential( provider, selection.credential, - this.#getStoredCredentials(provider)[selection.index]?.id, + credentialId, options?.signal, ); const apiKey = customProvider.getApiKey @@ -3537,7 +3609,7 @@ export class AuthStorage { const refreshedCredentials = await this.#refreshOAuthCredential( provider, selection.credential, - this.#getStoredCredentials(provider)[selection.index]?.id, + credentialId, options?.signal, ); const oauthCreds: Record = { @@ -3557,7 +3629,12 @@ export class AuthStorage { enterpriseUrl: result.newCredentials.enterpriseUrl ?? selection.credential.enterpriseUrl, apiEndpoint: result.newCredentials.apiEndpoint ?? selection.credential.apiEndpoint, }; - this.#replaceCredentialAt(provider, selection.index, updated); + if (credentialId !== undefined) { + const idx = this.#replaceCredentialById(provider, credentialId, updated); + if (idx !== -1) selection.index = idx; + } else { + this.#replaceCredentialAt(provider, selection.index, updated); + } if ((checkUsage && !allowBlocked) || requiresProModel) { const sameAccount = selection.credential.accountId === updated.accountId; if (!usageChecked || !sameAccount) { @@ -3607,7 +3684,6 @@ export class AuthStorage { // refresh token has changed, the peer rotation succeeded and we should pick // up the new credential instead of soft-deleting the row that the peer just // updated. - const credentialId = this.#getStoredCredentials(provider)[selection.index]?.id; if (credentialId !== undefined) { const latestRow = this.#store.listAuthCredentials(provider).find(row => row.id === credentialId); const latestCredential = latestRow?.credential; @@ -3618,29 +3694,37 @@ export class AuthStorage { credentialId, }); await this.reload(); - return this.#resolveOAuthSelection(provider, sessionId, options); + if (allowFallback) return this.#resolveOAuthSelection(provider, sessionId, options); } } // Permanently disable invalid credentials with an explicit cause for inspection/debugging. // Use a CAS-style disable conditioned on the row still containing the stale credential // we tried to refresh, so a peer rotation that lands between the pre-check above and // this disable doesn't soft-delete the freshly-rotated row. - const disabled = this.#tryDisableCredentialAtIfMatches( - provider, - selection.index, - selection.credential, - `oauth refresh failed: ${errorMsg}`, - ); + const disabled = + credentialId !== undefined + ? this.#disableCredentialByIdIfMatches( + provider, + credentialId, + selection.credential, + `oauth refresh failed: ${errorMsg}`, + ) + : this.#tryDisableCredentialAtIfMatches( + provider, + selection.index, + selection.credential, + `oauth refresh failed: ${errorMsg}`, + ); if (!disabled) { logger.debug("OAuth refresh disable lost CAS; reloading after peer rotation", { provider, index: selection.index, }); await this.reload(); - return this.#resolveOAuthSelection(provider, sessionId, options); + if (allowFallback) return this.#resolveOAuthSelection(provider, sessionId, options); } if (this.#getCredentialsForProvider(provider).some(credential => credential.type === "oauth")) { - return this.#resolveOAuthSelection(provider, sessionId, options); + if (allowFallback) return this.#resolveOAuthSelection(provider, sessionId, options); } } else { // Block temporarily for transient failures (5 minutes) @@ -3784,6 +3868,83 @@ export class AuthStorage { }; } + /** Stored OAuth credentials for `provider` in stable order, paired with their full-list index and row id. */ + #getStoredOAuthSelections(provider: string): StoredOAuthSelection[] { + return this.#getStoredCredentials(provider) + .map((entry, index) => ({ credentialId: entry.id, credential: entry.credential, index })) + .filter((entry): entry is StoredOAuthSelection => entry.credential.type === "oauth"); + } + + /** Refresh one stored OAuth selection and shape it as an {@link OAuthAccessResolution}. */ + async #resolveStoredOAuthAccess( + provider: string, + selection: StoredOAuthSelection, + providerKey: string, + options: AuthApiKeyOptions | undefined, + ): Promise { + try { + const resolved = await this.#tryOAuthCredential( + provider, + { credential: selection.credential, index: selection.index }, + providerKey, + undefined, + options, + { checkUsage: false, allowBlocked: true, allowFallback: false }, + ); + if (!resolved) { + return { + ok: false, + credentialId: selection.credentialId, + accountId: selection.credential.accountId, + email: selection.credential.email, + projectId: selection.credential.projectId, + enterpriseUrl: selection.credential.enterpriseUrl, + error: "OAuth access unavailable", + }; + } + const { credential } = resolved; + return { + ok: true, + credentialId: selection.credentialId, + accessToken: credential.access, + accountId: credential.accountId, + email: credential.email, + projectId: credential.projectId, + enterpriseUrl: credential.enterpriseUrl, + }; + } catch (error) { + return { + ok: false, + credentialId: selection.credentialId, + accountId: selection.credential.accountId, + email: selection.credential.email, + projectId: selection.credential.projectId, + enterpriseUrl: selection.credential.enterpriseUrl, + error: error instanceof Error ? error.message : String(error), + }; + } + } + + /** + * Read-only list of stored OAuth accounts for `provider` in stable storage + * order, WITHOUT refreshing any token. The array position (0-based) is the + * selector accepted by {@link AuthStorage.getOAuthAccessAt}; a "pick the Nth + * account" UI should render `position + 1`. + */ + listOAuthAccounts(provider: string): OAuthAccountSummary[] { + if (this.#runtimeOverrides.has(provider) || this.#configOverrides.has(provider)) { + return []; + } + return this.#getStoredOAuthSelections(provider).map((selection, position) => ({ + position, + credentialId: selection.credentialId, + accountId: selection.credential.accountId, + email: selection.credential.email, + projectId: selection.credential.projectId, + enterpriseUrl: selection.credential.enterpriseUrl, + })); + } + /** * Resolve every stored OAuth credential for `provider` independently. * @@ -3797,62 +3958,38 @@ export class AuthStorage { return []; } const providerKey = this.#getProviderTypeKey(provider, "oauth"); - const selections = this.#getStoredCredentials(provider) - .map((entry, index) => ({ credentialId: entry.id, credential: entry.credential, index })) - .filter( - (entry): entry is { credentialId: number; credential: OAuthCredential; index: number } => - entry.credential.type === "oauth", - ); return Promise.all( - selections.map(async (selection): Promise => { - try { - const resolved = await this.#tryOAuthCredential( - provider, - { credential: selection.credential, index: selection.index }, - providerKey, - undefined, - options, - { - checkUsage: false, - allowBlocked: true, - }, - ); - if (!resolved) { - return { - ok: false, - credentialId: selection.credentialId, - accountId: selection.credential.accountId, - email: selection.credential.email, - projectId: selection.credential.projectId, - enterpriseUrl: selection.credential.enterpriseUrl, - error: "OAuth access unavailable", - }; - } - const { credential } = resolved; - return { - ok: true, - credentialId: selection.credentialId, - accessToken: credential.access, - accountId: credential.accountId, - email: credential.email, - projectId: credential.projectId, - enterpriseUrl: credential.enterpriseUrl, - }; - } catch (error) { - return { - ok: false, - credentialId: selection.credentialId, - accountId: selection.credential.accountId, - email: selection.credential.email, - projectId: selection.credential.projectId, - enterpriseUrl: selection.credential.enterpriseUrl, - error: error instanceof Error ? error.message : String(error), - }; - } - }), + this.#getStoredOAuthSelections(provider).map(selection => + this.#resolveStoredOAuthAccess(provider, selection, providerKey, options), + ), ); } + /** + * Resolve a single stored OAuth credential by its account position (0-based, + * matching {@link AuthStorage.listOAuthAccounts}). Refreshes ONLY that + * credential ({@link #resolveStoredOAuthAccess} runs with `allowFallback: + * false`), so — unlike {@link AuthStorage.getOAuthAccesses} — a definitive + * failure of the targeted account surfaces as a failed resolution rather than + * silently rotating or rate-tripping a sibling. + * + * Returns `undefined` when `position` is out of range or runtime/config + * overrides have replaced OAuth with an explicit API key. + */ + async getOAuthAccessAt( + provider: string, + position: number, + options?: AuthApiKeyOptions, + ): Promise { + if (this.#runtimeOverrides.has(provider) || this.#configOverrides.has(provider)) { + return undefined; + } + const selection = this.#getStoredOAuthSelections(provider)[position]; + if (!selection) return undefined; + const providerKey = this.#getProviderTypeKey(provider, "oauth"); + return this.#resolveStoredOAuthAccess(provider, selection, providerKey, options); + } + /** * List saved rate-limit resets for every stored OAuth account of `provider` * (Codex), fetched LIVE from the dedicated `rate-limit-reset-credits` route. @@ -4229,22 +4366,55 @@ export class AuthStorage { if (target.credential.type !== "oauth") { throw new Error(`Credential ${id} is not OAuth (provider=${provider}, type=${target.credential.type})`); } + // The exact credential we are about to refresh — captured before the + // await so a definitive failure can CAS-disable the row against the + // value we actually attempted (NOT the expires:0 clone below). + const attempted = target.credential; // Pass a clone with expires=0 so the cached not-yet-expired short-circuit // in #refreshOAuthCredential doesn't suppress the requested refresh. - const stale: OAuthCredential = { ...target.credential, expires: 0 }; - const refreshed = await this.#refreshOAuthCredential(provider as Provider, stale, id, signal); + const stale: OAuthCredential = { ...attempted, expires: 0 }; + let refreshed: OAuthCredentials; + try { + refreshed = await this.#refreshOAuthCredential(provider as Provider, stale, id, signal); + } catch (error) { + // A definitively-dead grant tears the row down here, where the + // attempted credential is known. CAS on the persisted credential so a + // peer/login rotation in flight leaves the freshly-rotated row intact. + if (isDefinitiveOAuthFailure(String(error))) { + // CAS-loss (false) means a peer/login rotated the row mid-refresh, so + // our #data copy is stale — reload so the next caller serves the + // freshly-rotated credential rather than the dead token we attempted. + if ( + !this.#disableCredentialByIdIfMatches( + provider, + id, + attempted, + `oauth refresh failed: ${String(error)}`, + ) + ) { + await this.reload(); + } + } + throw error; + } const updated: OAuthCredential = { type: "oauth", access: refreshed.access, refresh: refreshed.refresh, expires: refreshed.expires, - accountId: refreshed.accountId ?? target.credential.accountId, - email: refreshed.email ?? target.credential.email, - projectId: refreshed.projectId ?? target.credential.projectId, - enterpriseUrl: refreshed.enterpriseUrl ?? target.credential.enterpriseUrl, - apiEndpoint: refreshed.apiEndpoint ?? target.credential.apiEndpoint, + accountId: refreshed.accountId ?? attempted.accountId, + email: refreshed.email ?? attempted.email, + projectId: refreshed.projectId ?? attempted.projectId, + enterpriseUrl: refreshed.enterpriseUrl ?? attempted.enterpriseUrl, + apiEndpoint: refreshed.apiEndpoint ?? attempted.apiEndpoint, }; - this.#replaceCredentialAt(provider, index, updated); + // Persist by id: the array may have been reordered/shrunk while the + // refresh was in flight, so the pre-await positional index is unsafe. A + // -1 means the row was disabled/removed mid-refresh — surface that as a + // miss rather than implying a live row the snapshot won't contain. + if (this.#replaceCredentialById(provider, id, updated) === -1) { + throw new Error(`No credential with id=${id}`); + } return { id, provider, diff --git a/packages/ai/src/providers/openai-codex-responses.ts b/packages/ai/src/providers/openai-codex-responses.ts index b7838fd18..4a7f911fb 100644 --- a/packages/ai/src/providers/openai-codex-responses.ts +++ b/packages/ai/src/providers/openai-codex-responses.ts @@ -61,6 +61,7 @@ import { type CodexRequestOptions, type InputItem, type RequestBody, + shouldUseCodexResponsesLite, transformRequestBody, } from "./openai-codex/request-transformer"; import { CodexApiError } from "./openai-codex/response-handler"; @@ -697,7 +698,7 @@ async function buildCodexRequestContext( }; const providerSessionState = getCodexProviderSessionState(options?.providerSessionState); - const responsesLite = options?.responsesLite === true; + const responsesLite = shouldUseCodexResponsesLite(transformedBody, options?.responsesLite); const sessionKey = getCodexWebSocketSessionKey(transportSessionId, model, accountId, baseUrl, responsesLite); const publicSessionKey = transportSessionId ? `${baseUrl}:${model.id}:${transportSessionId}` : undefined; if (sessionKey && publicSessionKey) { diff --git a/packages/ai/src/providers/openai-codex/request-transformer.ts b/packages/ai/src/providers/openai-codex/request-transformer.ts index 288f6f3ca..7e684e000 100644 --- a/packages/ai/src/providers/openai-codex/request-transformer.ts +++ b/packages/ai/src/providers/openai-codex/request-transformer.ts @@ -59,6 +59,26 @@ export interface RequestBody { [key: string]: unknown; } +function containsInputImage(value: unknown): boolean { + if (!value || typeof value !== "object") return false; + if ((value as { type?: unknown }).type === "input_image") return true; + if (Array.isArray(value)) { + for (const item of value) { + if (containsInputImage(item)) return true; + } + return false; + } + for (const item of Object.values(value)) { + if (containsInputImage(item)) return true; + } + return false; +} + +/** Returns whether a Codex request can use the text-only Responses Lite transport. */ +export function shouldUseCodexResponsesLite(body: RequestBody, requested: boolean | undefined): boolean { + return requested === true && !containsInputImage(body.input); +} + function getReasoningConfig(model: Model, options: CodexRequestOptions): ReasoningConfig { const config: ReasoningConfig = { effort: @@ -211,7 +231,8 @@ export async function transformRequestBody( body.input = [...developerMessages, ...body.input]; } - if (options.responsesLite) { + const responsesLite = shouldUseCodexResponsesLite(body, options.responsesLite); + if (responsesLite) { if (Array.isArray(body.input)) { stripImageDetails(body.input); } @@ -231,7 +252,7 @@ export async function transformRequestBody( // Responses Lite keeps reasoning replay server-side; codex-rs requests // `all_turns` there and otherwise omits context so the server default // (currently `current_turn`) applies. - const reasoningContext = options.reasoningContext ?? (options.responsesLite ? "all_turns" : undefined); + const reasoningContext = options.reasoningContext ?? (responsesLite ? "all_turns" : undefined); if (reasoningContext !== undefined) { body.reasoning.context = reasoningContext; } diff --git a/packages/ai/test/auth-broker-refresher.test.ts b/packages/ai/test/auth-broker-refresher.test.ts index 732b62b4b..0929f4087 100644 --- a/packages/ai/test/auth-broker-refresher.test.ts +++ b/packages/ai/test/auth-broker-refresher.test.ts @@ -148,4 +148,51 @@ describe("AuthBrokerRefresher", () => { expect(disableEvents).toHaveLength(0); expect(storage.exportSnapshot().credentials).toHaveLength(1); }); + + test("does not disable a credential a peer rotated during the refresh (CAS)", async () => { + const now = 1_700_000_000_000; + store!.saveOAuth("anthropic", { + access: "stale", + refresh: "stale-refresh", + expires: now + 60_000, + accountId: "a", + }); + storage = new AuthStorage(store!); + const disableEvents: string[] = []; + storage.onCredentialDisabled(event => { + disableEvents.push(event.disabledCause); + }); + await storage.reload(); + const id = store!.listAuthCredentials("anthropic")[0]!.id; + + // Our refresh fails with a dead-grant error, but a peer (another process / + // a fresh login) rotates the persisted row to a new token first. The + // CAS disable must see the row no longer holds the token we attempted and + // leave the freshly-rotated credential intact instead of clobbering it. + vi.spyOn(oauthUtils, "refreshOAuthToken").mockImplementation(async () => { + store!.updateAuthCredential(id, { + type: "oauth", + access: "fresh-from-peer", + refresh: "fresh-refresh-from-peer", + expires: now + 60 * 60_000, + accountId: "a", + }); + throw new Error('HTTP 400 invalid_grant {"error":"invalid_grant"}'); + }); + + const refresher = new AuthBrokerRefresher({ + storage, + refreshSkewMs: 5 * 60_000, + now: () => now, + }); + await refresher.tick(); + + expect(disableEvents).toHaveLength(0); + const rows = store!.listAuthCredentials("anthropic"); + expect(rows).toHaveLength(1); + expect(rows[0]?.credential.type).toBe("oauth"); + if (rows[0]?.credential.type === "oauth") { + expect(rows[0].credential.refresh).toBe("fresh-refresh-from-peer"); + } + }); }); diff --git a/packages/ai/test/auth-storage-oauth-account-select.test.ts b/packages/ai/test/auth-storage-oauth-account-select.test.ts new file mode 100644 index 000000000..cf7d50593 --- /dev/null +++ b/packages/ai/test/auth-storage-oauth-account-select.test.ts @@ -0,0 +1,118 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { type AuthCredentialStore, AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai/auth-storage"; +import * as oauthUtils from "@oh-my-pi/pi-ai/registry/oauth"; + +const PROVIDER = "unit-oauth-select"; + +function oauthCredential(suffix: string) { + return { + type: "oauth" as const, + access: `access-${suffix}`, + refresh: `refresh-${suffix}`, + expires: Date.now() + 60 * 60_000, + accountId: `acc-${suffix}`, + email: `${suffix}@example.com`, + }; +} + +describe("AuthStorage OAuth account selection", () => { + let tempDir = ""; + let store: AuthCredentialStore | null = null; + let authStorage: AuthStorage | null = null; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "pi-ai-oauth-select-")); + store = await SqliteAuthCredentialStore.open(path.join(tempDir, "agent.db")); + authStorage = new AuthStorage(store); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + store?.close(); + store = null; + authStorage = null; + if (tempDir) { + await fs.rm(tempDir, { recursive: true, force: true }); + tempDir = ""; + } + }); + + test("listOAuthAccounts reports stored order, positions, and identity without refreshing", async () => { + const storage = authStorage; + if (!storage) throw new Error("test setup failed"); + const refreshSpy = vi.spyOn(oauthUtils, "getOAuthApiKey"); + await storage.set(PROVIDER, [oauthCredential("a"), oauthCredential("b"), oauthCredential("c")]); + + const accounts = storage.listOAuthAccounts(PROVIDER); + + expect(accounts.map(a => a.position)).toEqual([0, 1, 2]); + expect(accounts.map(a => a.accountId)).toEqual(["acc-a", "acc-b", "acc-c"]); + expect(accounts.map(a => a.email)).toEqual(["a@example.com", "b@example.com", "c@example.com"]); + // Read-only: listing must not refresh any token. + expect(refreshSpy).not.toHaveBeenCalled(); + }); + + test("getOAuthAccessAt resolves the credential at the requested position and touches only that one", async () => { + const storage = authStorage; + if (!storage) throw new Error("test setup failed"); + const seen: string[] = []; + vi.spyOn(oauthUtils, "getOAuthApiKey").mockImplementation(async (provider, credentials) => { + const credential = credentials[provider]; + if (!credential) return null; + seen.push(credential.access); + return { newCredentials: credential, apiKey: credential.access }; + }); + await storage.set(PROVIDER, [oauthCredential("a"), oauthCredential("b"), oauthCredential("c")]); + + for (const [position, suffix] of [ + [0, "a"], + [1, "b"], + [2, "c"], + ] as const) { + seen.length = 0; + const result = await storage.getOAuthAccessAt(PROVIDER, position); + expect(result?.ok).toBe(true); + if (!result?.ok) throw new Error("expected ok resolution"); + expect(result.accountId).toBe(`acc-${suffix}`); + expect(result.accessToken).toBe(`access-${suffix}`); + // Only the targeted credential is resolved — no sibling is touched. + expect(seen).toEqual([`access-${suffix}`]); + } + }); + + test("getOAuthAccessAt returns undefined for an out-of-range position", async () => { + const storage = authStorage; + if (!storage) throw new Error("test setup failed"); + await storage.set(PROVIDER, [oauthCredential("a"), oauthCredential("b")]); + expect(await storage.getOAuthAccessAt(PROVIDER, 2)).toBeUndefined(); + expect(await storage.getOAuthAccessAt(PROVIDER, -1)).toBeUndefined(); + }); + + test("getOAuthAccessAt fails the requested account without touching siblings", async () => { + const storage = authStorage; + if (!storage) throw new Error("test setup failed"); + // The targeted account (acc-b) fails definitively; siblings would refresh fine. + const seen: string[] = []; + vi.spyOn(oauthUtils, "getOAuthApiKey").mockImplementation(async (provider, credentials) => { + const credential = credentials[provider]; + if (!credential) return null; + seen.push(credential.access); + if (credential.access === "access-b") throw new Error("invalid_grant"); + return { newCredentials: credential, apiKey: credential.access }; + }); + await storage.set(PROVIDER, [oauthCredential("a"), oauthCredential("b"), oauthCredential("c")]); + + const result = await storage.getOAuthAccessAt(PROVIDER, 1); + + expect(result?.ok).toBe(false); + if (!result || result.ok) throw new Error("expected failed resolution"); + // Reports the requested account, never a sibling's token. + expect(result.accountId).toBe("acc-b"); + expect("accessToken" in result).toBe(false); + // Target-only: no sibling credential was refreshed/rotated on the failure path. + expect(seen).toEqual(["access-b"]); + }); +}); diff --git a/packages/ai/test/auth-storage-oauth-refresh-race.test.ts b/packages/ai/test/auth-storage-oauth-refresh-race.test.ts index bc68bf60e..df96caf30 100644 --- a/packages/ai/test/auth-storage-oauth-refresh-race.test.ts +++ b/packages/ai/test/auth-storage-oauth-refresh-race.test.ts @@ -384,4 +384,51 @@ describe("AuthStorage OAuth refresh race", () => { const retryKey = await authStorage.getApiKey("unit-oauth-rotation", sessionId); expect(retryKey).toBe("access-b"); }); + + test("persists a refreshed token by id when a concurrent disable shifts indices", async () => { + if (!authStorage || !store) throw new Error("test setup failed"); + const now = Date.now(); + // Three distinct expired accounts → index order A, B, C by id ascending. + await authStorage.set("anthropic", [ + { type: "oauth", access: "a-acc", refresh: "a-ref", expires: now - 60_000, accountId: "acc-a", email: "a@x" }, + { type: "oauth", access: "b-acc", refresh: "b-ref", expires: now - 60_000, accountId: "acc-b", email: "b@x" }, + { type: "oauth", access: "c-acc", refresh: "c-ref", expires: now - 60_000, accountId: "acc-c", email: "c@x" }, + ]); + const seeded = store.listAuthCredentials("anthropic"); + expect(seeded).toHaveLength(3); + const idA = seeded[0]!.id; + const idB = seeded[1]!.id; + const idC = seeded[2]!.id; + + // While B refreshes, a definitive failure disables A — removing index 0 and + // shifting B from index 1 to index 0. A pre-await positional write would + // land B's rotated token on C; the id-addressed write must hit B and leave + // C untouched. + vi.spyOn(oauthUtils, "refreshOAuthToken").mockImplementation(async (_provider, credential) => { + if (credential.refresh === "b-ref") { + authStorage!.disableCredentialById(idA, "test: concurrent disable"); + return { + access: "b-fresh", + refresh: "b-fresh-ref", + expires: now + 60 * 60_000, + accountId: "acc-b", + email: "b@x", + }; + } + return { ...credential, expires: now + 60 * 60_000 }; + }); + + await withEnv(SUPPRESS_ANTHROPIC_ENV, async () => { + const refreshed = await authStorage!.forceRefreshCredentialById(idB); + expect(refreshed.id).toBe(idB); + }); + + const after = store.listAuthCredentials("anthropic"); + const bRow = after.find(row => row.id === idB); + const cRow = after.find(row => row.id === idC); + expect(bRow?.credential.type).toBe("oauth"); + if (bRow?.credential.type === "oauth") expect(bRow.credential.refresh).toBe("b-fresh-ref"); + expect(cRow?.credential.type).toBe("oauth"); + if (cRow?.credential.type === "oauth") expect(cRow.credential.refresh).toBe("c-ref"); + }); }); diff --git a/packages/ai/test/oauth-definitive-failure.test.ts b/packages/ai/test/oauth-definitive-failure.test.ts new file mode 100644 index 000000000..6e77164f9 --- /dev/null +++ b/packages/ai/test/oauth-definitive-failure.test.ts @@ -0,0 +1,67 @@ +/** + * Contract for {@link isDefinitiveOAuthFailure} — the shared classifier that + * decides whether an OAuth refresh error tears the credential down (re-login + * required) or is a transient blip to block-and-retry. A false positive here + * permanently disables a healthy account, so the 403 / rate-limit / 5xx cases + * below are load-bearing, not cosmetic. + */ +import { describe, expect, it } from "bun:test"; +import { isDefinitiveOAuthFailure } from "@oh-my-pi/pi-ai/auth-storage"; + +describe("isDefinitiveOAuthFailure", () => { + it("treats explicit dead-grant errors as definitive", () => { + for (const msg of [ + 'HTTP 400 invalid_grant {"error":"invalid_grant"}', + "invalid_token", + "OAuth refresh failed: refresh token revoked", + 'invalid_grant {"error_description":"Refresh token expired"}', + "unauthorized_client", + ]) { + expect(isDefinitiveOAuthFailure(msg)).toBe(true); + } + }); + + it("treats a bare 401 from the token endpoint as definitive", () => { + expect(isDefinitiveOAuthFailure("HTTP 401 Unauthorized")).toBe(true); + }); + + it("never treats a bare 403 as definitive (WAF / egress / permission, not a dead token)", () => { + // Regression: a shared broker egress IP that gets 403'd by the provider, + // or a google PERMISSION_DENIED / account-verification 403, must NOT + // permanently disable an otherwise-valid credential. + expect(isDefinitiveOAuthFailure("HTTP 403 Forbidden")).toBe(false); + expect(isDefinitiveOAuthFailure("403 PERMISSION_DENIED: account verification required")).toBe(false); + expect(isDefinitiveOAuthFailure("blocked by cloudflare (403)")).toBe(false); + }); + + it("treats rate-limit and server/gateway errors as transient", () => { + for (const msg of [ + "429 too many requests", + "HTTP 503 Service Unavailable", + "500 internal server error", + "rate limit exceeded", + ]) { + expect(isDefinitiveOAuthFailure(msg)).toBe(false); + } + }); + + it("treats network blips as transient (incl. ECONNRESET)", () => { + for (const msg of [ + "fetch failed: ECONNRESET", + "fetch failed: ECONNREFUSED", + "ETIMEDOUT", + "socket hang up", + "network error", + "OAuth token refresh timed out for provider: anthropic", + ]) { + expect(isDefinitiveOAuthFailure(msg)).toBe(false); + } + }); + + it("lets a transient signal override a bare 401 (rate-limited auth endpoint)", () => { + // A 401 wrapped in a rate-limit / 5xx context is the provider throttling, + // not a dead grant — block-and-retry instead of nuking the row. + expect(isDefinitiveOAuthFailure("401 unauthorized — 429 too many requests")).toBe(false); + expect(isDefinitiveOAuthFailure("502 bad gateway (was 401 upstream)")).toBe(false); + }); +}); diff --git a/packages/ai/test/openai-codex-responses-lite.test.ts b/packages/ai/test/openai-codex-responses-lite.test.ts index 72083ada6..e8f6864e6 100644 --- a/packages/ai/test/openai-codex-responses-lite.test.ts +++ b/packages/ai/test/openai-codex-responses-lite.test.ts @@ -115,7 +115,7 @@ describe("openai-codex reasoning.context", () => { }); describe("openai-codex Responses Lite input shaping", () => { - it("strips image detail from message content and tool outputs only under lite", async () => { + it("keeps full Responses image details when a requested lite body contains images", async () => { const model = createCodexModel("gpt-5.1-codex"); const makeInput = (): InputItem[] => [ { @@ -137,8 +137,8 @@ describe("openai-codex Responses Lite input shaping", () => { const lite = await transformRequestBody({ model: model.id, input: makeInput() }, model, { responsesLite: true }); const liteMessage = lite.input?.[0]?.content as Array>; const liteOutput = lite.input?.[2]?.output as Array>; - expect(liteMessage[1]).toEqual({ type: "input_image", image_url: "data:image/png;base64,AAAA" }); - expect(liteOutput[0]).toEqual({ type: "input_image", image_url: "data:image/png;base64,BBBB" }); + expect(liteMessage[1]).toEqual({ type: "input_image", detail: "auto", image_url: "data:image/png;base64,AAAA" }); + expect(liteOutput[0]).toEqual({ type: "input_image", detail: "high", image_url: "data:image/png;base64,BBBB" }); const plain = await transformRequestBody({ model: model.id, input: makeInput() }, model, {}); const plainMessage = plain.input?.[0]?.content as Array>; @@ -214,6 +214,57 @@ describe("openai-codex Responses Lite and client metadata wire format", () => { expect(captured?.headers.get("x-openai-internal-codex-responses-lite")).toBe("true"); expect(captured?.body.client_metadata).toEqual(clientMetadata); }); + it("falls back to full Responses when a lite request contains images", async () => { + const model = buildModel({ + id: "gpt-5.5", + name: "GPT-5.5", + api: "openai-codex-responses", + provider: "openai-codex", + baseUrl: "https://api.openai.com/v1", + reasoning: true, + input: ["text", "image"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 272_000, + maxTokens: 128_000, + }); + let captured: CapturedCodexRequest | undefined; + const fetchMock = createCodexFetchMock(createCodexSse(COMPLETED_CODEX_EVENTS), request => { + captured = request; + }); + + const result = await streamOpenAICodexResponses( + model, + { + messages: [ + { + role: "user", + timestamp: Date.now(), + content: [ + { type: "text", text: "read this image" }, + { type: "image", mimeType: "image/png", data: "AAAA" }, + ], + }, + ], + }, + { + apiKey: createCodexTestToken(), + fetch: fetchMock, + responsesLite: true, + }, + ).result(); + + expect(result.stopReason).toBe("stop"); + expect(captured?.headers.get("x-openai-internal-codex-responses-lite")).toBeNull(); + expect(captured?.body.input).toEqual([ + { + role: "user", + content: [ + { type: "input_text", text: "read this image" }, + { type: "input_image", detail: "auto", image_url: "data:image/png;base64,AAAA" }, + ], + }, + ]); + }); it("omits the lite header and client_metadata when not requested", async () => { const model = createCodexModel("gpt-5.1-codex"); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2cb19fe24..663381b33 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -11,6 +11,22 @@ - Fixed reversible secret placeholders sharing a case-folded hash base across ASCII case variants, which let a prompt-injected model synthesize a never-provider-visible sibling secret's keyed token by swapping the case hint (`#…:L#` → `#…:U#`) in a tool-call argument. Placeholder bases are now keyed on the exact secret value, so each casing variant gets an independent base and a synthesized sibling token deobfuscates to nothing on live provider/tool-call paths ([#2465](https://github.com/can1357/oh-my-pi/issues/2465)). - Fixed an auto-collected environment secret that is also declared as a plain `mode: "replace"` entry with the same content still forcing creation of the persisted `secret-placeholder.key`. Replace mappings run before obfuscate mappings, so the value is one-way replaced and the obfuscate entry never emits a reversible placeholder; the key-need check now ignores such replace-shadowed obfuscate entries, so an effectively replace-only secret set no longer requires (or writes) the key file and no longer fails startup when the agent config dir is unwritable ([#2465](https://github.com/can1357/oh-my-pi/issues/2465)). +### Added + +- Added --list flag to view stored OAuth accounts for a provider +- Added --account flag to select a specific OAuth account by index for token retrieval +- Added mouse support for navigation, selection, and toggling in the extensions dashboard + +### Changed + +- Enabled fullscreen alternate-screen mode for the extensions dashboard +- Improved tab navigation to mute empty enabled providers while keeping disabled ones selectable +- `/extensions` (the Extension Control Center) now opens as a fullscreen window on the terminal's alternate screen, matching `/settings`: it borrows the alt buffer for its lifetime (the transcript is untouched underneath) and uses the same modern chrome — a titled rounded frame, the shared `TabBar` for provider switching, and a divider/footer layout. The dashboard is now mouse-aware: the wheel scrolls the list (and the detail inspector, which gained its own scroll viewport), hovering highlights tabs and rows, and clicking selects a row or — when it is already selected — toggles it; clicking a provider tab switches to it. Empty enabled providers are unselectable while disabled providers stay selectable so their master switch can re-enable them. + +### Fixed + +- Fixed `omp install` of legacy pi extensions failing with `Cannot find module '/$bunfs/root/packages/coding-agent/src/extensibility/typebox.js'` on every released `omp--` binary. Commit `dc5c93462f` removed worker entrypoints from `scripts/ci-release-build-binaries.ts`; the inline comment then claimed the legacy-shim and package-barrel entrypoints (`typebox.ts`, `legacy-pi-{ai,coding-agent}-shim.ts`, `packages/{agent,natives,tui,utils}/...`) were "still" passed to `bun build --compile`, but they had never been re-added. The release binaries shipped without those files in bunfs, so `legacy-pi-compat.ts` redirected `typebox` imports to a bunfs path that didn't exist. The two build scripts (release CI + local dev) now share `scripts/binary-entrypoints.ts` so the lists cannot drift apart, and `__resolveTypeBoxShimPath` mirrors `__validateLegacyPiPackageRootOverrides` (#2168) by dropping the override when the shim file is missing so future regressions fall through to native `node_modules` resolution instead of emitting a dead bunfs URL ([#3414](https://github.com/can1357/oh-my-pi/issues/3414)). + ## [16.1.17] - 2026-06-24 ### Fixed @@ -12427,4 +12443,4 @@ Initial public release. ## [0.7.6] - 2025-11-13 -Previous releases did not maintain a changelog. +Previous releases did not maintain a changelog. \ No newline at end of file diff --git a/packages/coding-agent/scripts/build-binary.ts b/packages/coding-agent/scripts/build-binary.ts index b90498214..995bcf2b0 100644 --- a/packages/coding-agent/scripts/build-binary.ts +++ b/packages/coding-agent/scripts/build-binary.ts @@ -2,6 +2,7 @@ import { createRequire } from "node:module"; import * as path from "node:path"; +import { LEGACY_COMPAT_BUILD_ENTRYPOINTS } from "../../../scripts/binary-entrypoints"; const packageDir = path.join(import.meta.dir, ".."); const repoRoot = path.join(packageDir, "..", ".."); @@ -82,22 +83,7 @@ async function main(): Promise { "--root", ".", "./packages/coding-agent/src/cli.ts", - // Legacy pi-* extension compat entrypoints served by - // `legacy-pi-compat.ts`. These are reached via computed bunfs paths - // (which `--compile`'s static analyzer cannot trace), so each must be - // listed here to land in bunfs at - // `/$bunfs/root/packages//.js`. The coding-agent's own - // `./src/index.ts` is intentionally NOT listed: bun --compile silently - // breaks the CLI entry when the same package's barrel appears as an - // extra entrypoint (issue #1474), so legacy `pi-coding-agent` imports - // resolve through `legacy-pi-coding-agent-shim.ts` instead. - "./packages/agent/src/index.ts", - "./packages/natives/native/index.js", - "./packages/tui/src/index.ts", - "./packages/utils/src/index.ts", - "./packages/coding-agent/src/extensibility/typebox.ts", - "./packages/coding-agent/src/extensibility/legacy-pi-ai-shim.ts", - "./packages/coding-agent/src/extensibility/legacy-pi-coding-agent-shim.ts", + ...LEGACY_COMPAT_BUILD_ENTRYPOINTS, "--outfile", `packages/coding-agent/dist/${outName}`, ], diff --git a/packages/coding-agent/src/commands/token.ts b/packages/coding-agent/src/commands/token.ts index fb8162ad6..b2bf5eb8a 100644 --- a/packages/coding-agent/src/commands/token.ts +++ b/packages/coding-agent/src/commands/token.ts @@ -28,12 +28,23 @@ export default class Token extends Command { description: "Force refresh the OAuth token even if it has not expired", default: false, }), + account: Flags.integer({ + char: "a", + description: "Select the Nth OAuth account (1-based) in stored order instead of the round-robin default", + }), + list: Flags.boolean({ + char: "l", + description: "List the provider's OAuth accounts (index + identity) and exit", + default: false, + }), }; static examples = [ "# Get API key for Anthropic\n omp token anthropic", "# Get raw Copilot credential JSON\n omp token github-copilot --raw", "# Force refresh and get Gemini CLI token\n omp token google-gemini-cli --force-refresh", + "# List Anthropic OAuth accounts\n omp token anthropic --list", + "# Get the 2nd Anthropic OAuth account's token\n omp token anthropic --account 2", ]; async run(): Promise { @@ -43,6 +54,49 @@ export default class Token extends Command { const authStorage = await discoverAuthStorage(); try { + if (flags.list || flags.account !== undefined) { + const accounts = authStorage.listOAuthAccounts(provider); + if (accounts.length === 0) { + process.stderr.write(`${chalk.red(`No OAuth accounts found for provider "${providerName}".`)}\n`); + process.stderr.write("--account/--list select among OAuth accounts; this provider has none stored.\n"); + process.exitCode = 1; + return; + } + if (flags.list) { + for (const acct of accounts) { + const label = + acct.email ?? + acct.accountId ?? + acct.projectId ?? + acct.enterpriseUrl ?? + `credential #${acct.credentialId}`; + process.stdout.write(`${acct.position + 1}. ${label}\n`); + } + return; + } + const n = flags.account; + if (n === undefined || n < 1 || n > accounts.length) { + process.stderr.write( + `${chalk.red(`Invalid --account ${n ?? "(missing)"}.`)} Provider "${providerName}" has ${accounts.length} OAuth account(s) (1-${accounts.length}).\n`, + ); + process.exitCode = 1; + return; + } + const resolution = await authStorage.getOAuthAccessAt(provider, n - 1, { + forceRefresh: flags["force-refresh"], + }); + if (!resolution?.ok) { + const reason = resolution && !resolution.ok ? resolution.error : "no OAuth credential available"; + process.stderr.write( + `${chalk.red(`Could not get token for account ${n} of "${providerName}": ${reason}`)}\n`, + ); + process.exitCode = 1; + return; + } + process.stdout.write(`${resolution.accessToken}\n`); + return; + } + const modelRegistry = new ModelRegistry(authStorage); // Resolve the API key / token diff --git a/packages/coding-agent/src/extensibility/plugins/legacy-pi-compat.ts b/packages/coding-agent/src/extensibility/plugins/legacy-pi-compat.ts index bc69f2b43..98898ed09 100644 --- a/packages/coding-agent/src/extensibility/plugins/legacy-pi-compat.ts +++ b/packages/coding-agent/src/extensibility/plugins/legacy-pi-compat.ts @@ -189,9 +189,35 @@ function sourceShimPath(file: string): string { : path.resolve(import.meta.dir, "..", file); } -const TYPEBOX_SHIM_PATH = BUNFS_PACKAGE_ROOT - ? bunfsPath("coding-agent", "src", "extensibility", "typebox.js") - : sourceShimPath("typebox.ts"); +/** + * Resolve the path the TypeBox compatibility shim ships at — bunfs in compiled + * mode, on-disk source elsewhere — then drop it when the file is missing. + * + * Validation mirrors `__validateLegacyPiPackageRootOverrides` (#2168): if the + * computed candidate doesn't exist (e.g. a release build silently omitted the + * `--compile` entrypoint — issue #3414), `resolveTypeBoxSpecifier` returns + * `undefined` and `rewriteLegacyExtensionSource` leaves bare `typebox` / + * `@sinclair/typebox` specifiers alone, so Bun falls through to native + * resolution against the extension's own `node_modules`. + * + * Exported for tests; production callers use `TYPEBOX_SHIM_PATH`. + */ +export function __resolveTypeBoxShimPath( + bunfsRoot: string | null, + bundledSelfRoot: string | undefined, + metaDir: string, + pathExistsSync: (p: string) => boolean = fs.existsSync, + pathImpl: typeof path = path, +): string | null { + const candidate = bunfsRoot + ? __joinBunfsPath(bunfsRoot, ["coding-agent", "src", "extensibility", "typebox.js"], pathImpl) + : bundledSelfRoot + ? pathImpl.join(bundledSelfRoot, "src", "extensibility", "typebox.ts") + : pathImpl.resolve(metaDir, "..", "typebox.ts"); + return pathExistsSync(candidate) ? candidate : null; +} + +const TYPEBOX_SHIM_PATH = __resolveTypeBoxShimPath(BUNFS_PACKAGE_ROOT, BUNDLED_SELF_PACKAGE_ROOT, import.meta.dir); // Legacy extensions historically imported `Type` (and `Static`/`TSchema`) from // the package root of `@(scope)/pi-ai`. pi-ai 15.1.0 removed the runtime `Type` @@ -341,12 +367,17 @@ const TYPEBOX_IMPORT_SPECIFIER_REGEX = /((?:from\s+|import\s+|import\s*\(\s*)["' */ async function rewriteLegacyExtensionSource(source: string, importerPath: string): Promise { const withPi = rewriteLegacyPiImports(source); - const withTypeBox = withPi.replace( - TYPEBOX_IMPORT_SPECIFIER_REGEX, - (_match, prefix: string, _specifier: string, suffix: string) => { - return `${prefix}${toImportSpecifier(TYPEBOX_SHIM_PATH)}${suffix}`; - }, - ); + // When the TypeBox shim is missing (release build dropped the entrypoint — + // issue #3414), leave bare specifiers untouched so Bun resolves a real + // `typebox` / `@sinclair/typebox` install from the extension's own + // `node_modules`. `resolveTypeBoxSpecifier` mirrors the fall-through. + const withTypeBox = TYPEBOX_SHIM_PATH + ? withPi.replace( + TYPEBOX_IMPORT_SPECIFIER_REGEX, + (_match, prefix: string, _specifier: string, suffix: string) => + `${prefix}${toImportSpecifier(TYPEBOX_SHIM_PATH)}${suffix}`, + ) + : withPi; return rewriteExtensionPackageImports(withTypeBox, importerPath); } @@ -719,8 +750,8 @@ function resolveLegacyPiSpecifier(args: { path: string; importer: string }): { p } } -function resolveTypeBoxSpecifier(): { path: string } { - return { path: TYPEBOX_SHIM_PATH }; +function resolveTypeBoxSpecifier(): { path: string } | undefined { + return TYPEBOX_SHIM_PATH ? { path: TYPEBOX_SHIM_PATH } : undefined; } export function installLegacyPiSpecifierShim(): void { diff --git a/packages/coding-agent/src/modes/components/extensions/extension-dashboard.ts b/packages/coding-agent/src/modes/components/extensions/extension-dashboard.ts index 5dc84f278..bc998b528 100644 --- a/packages/coding-agent/src/modes/components/extensions/extension-dashboard.ts +++ b/packages/coding-agent/src/modes/components/extensions/extension-dashboard.ts @@ -1,32 +1,34 @@ /** - * ExtensionDashboard - Tabbed layout for the Extension Control Center. + * ExtensionDashboard - Fullscreen alternate-screen control center for extensions. * - * Layout: - * - Top: Horizontal tab bar for provider selection - * - Body: 2-column grid (inventory list | preview panel) + * Chrome mirrors the `/settings` overlay: a titled rounded box, a shared + * {@link TabBar} for provider selection, and a two-column body (inventory list | + * inspector). Both panes are mouse-aware — wheel scrolls, hover highlights, and + * clicks select/activate — routed from a single SGR-mouse handler. * * Navigation: - * - TAB/Shift+TAB: Cycle through provider tabs - * - Up/Down/j/k: Navigate list - * - Space: Toggle selected item (or master switch) - * - Esc: Close dashboard (clears search first if active) + * - Tab/Shift+Tab or ←/→: switch provider tab + * - Up/Down/j/k or wheel: move list selection + * - Space/Enter or click: toggle selected item (or provider master switch) + * - Wheel over the inspector: scroll the detail pane + * - Esc: clear search (if active) then close */ import { type Component, - Container, matchesKey, padding, - Spacer, - Text, + parseSgrMouse, + ScrollView, + type Tab, + TabBar, truncateToWidth, visibleWidth, - wrapTextWithAnsi, } from "@oh-my-pi/pi-tui"; import { Settings } from "../../../config/settings"; -import { DynamicBorder } from "../../../modes/components/dynamic-border"; +import { getTabBarTheme } from "../../../modes/shared"; import { theme } from "../../../modes/theme/theme"; import { matchesAppInterrupt } from "../../../modes/utils/keybinding-matchers"; -import { handleTabSwitchKey, padLinesToHeight } from "../selector-helpers"; +import { bottomBorder, divider, row, topBorder } from "../overlay-box"; import { ExtensionList } from "./extension-list"; import { InspectorPanel } from "./inspector-panel"; import { @@ -37,17 +39,41 @@ import { refreshState, toggleProvider, } from "./state-manager"; -import type { DashboardState } from "./types"; +import type { DashboardState, ProviderTab } from "./types"; -const EXT_FOOTER = " ↑/↓: navigate Space: toggle ←/→: provider Esc: close"; +const EXT_FOOTER = " ↑/↓: navigate · Space: toggle · ←/→: provider · Esc: close"; -export class ExtensionDashboard extends Container { +/** + * Map dashboard provider tabs to {@link TabBar} tabs. Empty *enabled* providers + * are muted — skipped by keyboard nav and unclickable; disabled providers stay + * selectable (with a leading disabled glyph) so their master switch can be + * re-enabled from the list. The "all" tab is never muted or marked. + */ +export function buildTabBarTabs(tabs: ProviderTab[]): Tab[] { + return tabs.map(tab => { + const isAll = tab.id === "all"; + const isEmptyEnabled = tab.count === 0 && tab.enabled && !isAll; + const isDisabled = !tab.enabled && !isAll; + let label = tab.label; + if (tab.count > 0) label += ` (${tab.count})`; + if (isDisabled) label = `${theme.status.disabled} ${label}`; + return { id: tab.id, label, short: tab.label, muted: isEmptyEnabled }; + }); +} + +export class ExtensionDashboard implements Component { #state!: DashboardState; #mainList!: ExtensionList; #inspector!: InspectorPanel; + #tabBar!: TabBar; + #body!: TwoColumnBody; #refreshToken = 0; - #builtRows = -1; - #builtCols = -1; + // Frame geometry from the last render, for SGR mouse hit-testing. The + // fullscreen overlay paints from screen row 0, so mouse rows map 1:1. + #tabRowStart = 0; + #tabRowCount = 0; + #bodyRowStart = 0; + #bodyRowCount = 0; onClose?: () => void; onRequestRender?: () => void; @@ -56,9 +82,7 @@ export class ExtensionDashboard extends Container { private readonly cwd: string, private readonly settings: Settings | null, private readonly terminalHeight: number, - ) { - super(); - } + ) {} static async create( cwd: string, @@ -75,37 +99,36 @@ export class ExtensionDashboard extends Container { const disabledIds = sm ? ((sm.get("disabledExtensions") as string[]) ?? []) : []; this.#state = await createInitialState(this.cwd, disabledIds); - // Calculate max visible items based on terminal height - // Reserve ~10 lines for header, tabs, help text, borders - const maxVisible = this.#maxVisibleItems(); - - // Create main list - always focused + const initialMaxVisible = Math.max(3, this.terminalHeight - 9); this.#mainList = new ExtensionList( this.#state.searchFiltered, { onSelectionChange: ext => { this.#state.selected = ext; this.#inspector.setExtension(ext); + // A fresh selection resets the inspector to the top. + this.#body.resetInspectorScroll(); }, - onToggle: (extensionId, enabled) => { - this.#handleExtensionToggle(extensionId, enabled); - }, - onMasterToggle: providerId => { - this.#handleProviderToggle(providerId); - }, + onToggle: (extensionId, enabled) => this.#handleExtensionToggle(extensionId, enabled), + onMasterToggle: providerId => this.#handleProviderToggle(providerId), masterSwitchProvider: this.#getActiveProviderId(), }, - maxVisible, + initialMaxVisible, ); this.#mainList.setFocused(true); - // Create inspector this.#inspector = new InspectorPanel(); if (this.#state.selected) { this.#inspector.setExtension(this.#state.selected); } - this.#buildLayout(); + this.#body = new TwoColumnBody(this.#mainList, this.#inspector, this.terminalHeight); + + this.#tabBar = new TabBar("", buildTabBarTabs(this.#state.tabs), getTabBarTheme()); + this.#tabBar.showHint = false; + this.#tabBar.onTabChange = tab => this.#selectProviderById(tab.id); + const activeId = this.#state.tabs[this.#state.activeTabIndex]?.id; + if (activeId) this.#tabBar.setActiveById(activeId); } #getActiveProviderId(): string | null { @@ -118,98 +141,117 @@ export class ExtensionDashboard extends Container { return process.stdout.rows || this.terminalHeight || 24; } - #uiWidth(): number { - return Math.max(20, process.stdout.columns || 80); + /** + * Fullscreen frame: titled top border, the tab row(s), a divider, the + * two-column body sized to fill the viewport, a divider, the footer hint, and + * the bottom border. Records row geometry for mouse hit-testing. + */ + render(width: number): readonly string[] { + const height = Math.max(14, this.#terminalRows()); + const innerWidth = Math.max(1, width - 4); + + const tabLines = this.#tabBar.render(innerWidth); + // Fixed chrome: top border + tab rows + divider + divider + footer + bottom border. + const fixedRows = 1 + tabLines.length + 1 + 1 + 1 + 1; + const contentRows = Math.max(5, height - fixedRows); + + this.#mainList.setMaxVisible(Math.max(3, contentRows - 2)); + this.#body.setMaxHeight(contentRows); + const bodyLines = this.#body.render(innerWidth); + + const out: string[] = []; + out.push(topBorder(width, "Extension Control Center")); + this.#tabRowStart = out.length; + this.#tabRowCount = tabLines.length; + for (const line of tabLines) out.push(row(line, width)); + out.push(divider(width)); + this.#bodyRowStart = out.length; + this.#bodyRowCount = contentRows; + for (let i = 0; i < contentRows; i++) out.push(row(bodyLines[i] ?? "", width)); + out.push(divider(width)); + out.push(row(theme.fg("dim", EXT_FOOTER), width)); + out.push(bottomBorder(width)); + return out; } - #footerLines(): number { - return Math.max(1, wrapTextWithAnsi(theme.fg("dim", EXT_FOOTER), this.#uiWidth()).length); + invalidate(): void { + this.#tabBar.invalidate(); + this.#mainList.invalidate(); + this.#inspector.invalidate(); } - /** Height budget for the two-column body, sized to the live terminal. */ - #computeBodyHeight(): number { - // Chrome: top border + title + tab bar + spacer (4), then spacer + footer + bottom border. - const chrome = 4 + 1 + this.#footerLines() + 1; - return Math.max(5, this.#terminalRows() - chrome); - } + /** + * Route an SGR mouse report against the last render's geometry. Wheel scrolls + * the pane under the pointer, motion drives hover highlights (tabs + rows), + * and a left click switches tabs or selects/activates a list row. + */ + #handleMouse(data: string): void { + const event = parseSgrMouse(data); + if (!event) return; - #maxVisibleItems(): number { - // List chrome inside the body: search line, blank line, scroll indicator. - return Math.max(3, this.#computeBodyHeight() - 3); - } + // row() insets content by two columns (border + space). + const innerCol = event.col - 2; + const tabLine = event.row - this.#tabRowStart; + const overTabs = tabLine >= 0 && tabLine < this.#tabRowCount; + const bodyLine = event.row - this.#bodyRowStart; + const overBody = bodyLine >= 0 && bodyLine < this.#bodyRowCount; + const leftWidth = this.#body.leftWidth; + const overList = overBody && innerCol < leftWidth; + const overInspector = overBody && innerCol >= leftWidth + 3; - override render(width: number): readonly string[] { - // Rebuild when terminal geometry changes so the full-screen overlay - // re-fits on resize. - if (this.#terminalRows() !== this.#builtRows || this.#uiWidth() !== this.#builtCols) { - this.#buildLayout(); - } - const lines = super.render(width); - // Pad to the full viewport so the dashboard covers the screen instead of - // letting the transcript peek through below it. - return padLinesToHeight(lines, this.#terminalRows()); - } - - #buildLayout(): void { - this.clear(); - - // Top border - this.addChild(new DynamicBorder()); - - // Title - this.addChild(new Text(theme.bold(theme.fg("accent", " Extension Control Center")), 0, 0)); - - // Tab bar - this.addChild(new Text(this.#renderTabBar(), 0, 0)); - this.addChild(new Spacer(1)); - - // 2-column body sized to fill the live terminal viewport. - const bodyMaxHeight = this.#computeBodyHeight(); - this.#mainList.setMaxVisible(this.#maxVisibleItems()); - this.addChild(new TwoColumnBody(this.#mainList, this.#inspector, bodyMaxHeight)); - - this.addChild(new Spacer(1)); - this.addChild(new Text(theme.fg("dim", EXT_FOOTER), 0, 0)); - - // Bottom border - this.addChild(new DynamicBorder()); - this.#builtRows = this.#terminalRows(); - this.#builtCols = this.#uiWidth(); - } - - #renderTabBar(): string { - const parts: string[] = [" "]; - - for (let i = 0; i < this.#state.tabs.length; i++) { - const tab = this.#state.tabs[i]; - const isActive = i === this.#state.activeTabIndex; - const isEmpty = tab.count === 0 && tab.id !== "all"; - const isDisabled = !tab.enabled && tab.id !== "all"; - - // Build label with count - let label = tab.label; - if (tab.count > 0) { - label += ` (${tab.count})`; - } - - const displayLabel = isDisabled ? `${theme.status.disabled} ${label}` : label; - - if (isActive) { - // Active tab: background highlight - parts.push(theme.bg("selectedBg", ` ${displayLabel} `)); - } else if (isDisabled) { - // Disabled provider: dim - parts.push(theme.fg("dim", ` ${displayLabel} `)); - } else if (isEmpty) { - // Empty enabled provider: very dim, unselectable - parts.push(theme.fg("dim", ` ${label} `)); - } else { - // Normal enabled provider - parts.push(theme.fg("muted", ` ${label} `)); + if (event.wheel !== null) { + if (overList) { + this.#mainList.handleWheel(event.wheel); + this.onRequestRender?.(); + } else if (overInspector) { + this.#body.scrollInspector(event.wheel); + this.onRequestRender?.(); } + return; } - return parts.join(""); + if (event.motion) { + const hoveredTab = overTabs ? this.#tabBar.tabAt(tabLine, innerCol) : undefined; + this.#tabBar.setHoverTab(hoveredTab && !hoveredTab.muted ? hoveredTab.id : null); + this.#mainList.setHoverIndex(overList ? this.#mainList.hitTest(bodyLine) : null); + this.onRequestRender?.(); + return; + } + + if (!event.leftClick) return; + + if (overTabs) { + const tab = this.#tabBar.tabAt(tabLine, innerCol); + if (tab) this.#tabBar.selectTab(tab.id); + return; + } + if (overList) { + this.#mainList.handleClick(bodyLine); + this.onRequestRender?.(); + } + } + + /** Switch to the provider tab with `id`, re-filtering the list around it. */ + #selectProviderById(id: string): void { + const index = this.#state.tabs.findIndex(t => t.id === id); + if (index < 0) return; + this.#state.activeTabIndex = index; + + const tab = this.#state.tabs[index]; + this.#state.tabFiltered = filterByProvider(this.#state.extensions, tab.id); + this.#state.searchFiltered = applyFilter(this.#state.tabFiltered, this.#state.searchQuery); + this.#state.listIndex = 0; + this.#state.scrollOffset = 0; + this.#state.selected = this.#state.searchFiltered[0] ?? null; + + this.#mainList.setExtensions(this.#state.searchFiltered); + this.#mainList.setMasterSwitchProvider(this.#getActiveProviderId()); + this.#mainList.resetSelection(); + if (this.#state.selected) { + this.#inspector.setExtension(this.#state.selected); + } + this.#body.resetInspectorScroll(); + this.onRequestRender?.(); } #handleProviderToggle(providerId: string): void { @@ -241,7 +283,7 @@ export class ExtensionDashboard extends Container { async #refreshFromState(): Promise { const refreshToken = ++this.#refreshToken; - // Remember current tab ID before refresh + // Remember the current tab so it survives the re-sort. const currentTabId = this.#state.tabs[this.#state.activeTabIndex]?.id; const sm = this.settings ?? Settings.instance; @@ -250,7 +292,7 @@ export class ExtensionDashboard extends Container { if (refreshToken !== this.#refreshToken) return; this.#state = nextState; - // Find the same tab in the new (re-sorted) list + // Re-anchor on the same tab id in the (re-sorted) list. if (currentTabId) { const newIndex = this.#state.tabs.findIndex(t => t.id === currentTabId); if (newIndex >= 0) { @@ -260,12 +302,11 @@ export class ExtensionDashboard extends Container { this.#mainList.setExtensions(this.#state.searchFiltered); this.#mainList.setMasterSwitchProvider(this.#getActiveProviderId()); - if (this.#state.selected) { this.#inspector.setExtension(this.#state.selected); } - this.#buildLayout(); + this.#tabBar.setTabs(buildTabBarTabs(this.#state.tabs), currentTabId); this.onRequestRender?.(); } @@ -275,45 +316,17 @@ export class ExtensionDashboard extends Container { if (this.#state.selected) { this.#inspector.setExtension(this.#state.selected); } - this.#buildLayout(); + this.#tabBar.setTabs(buildTabBarTabs(this.#state.tabs), this.#state.tabs[this.#state.activeTabIndex]?.id); this.onRequestRender?.(); } - #switchTab(direction: 1 | -1): void { - const numTabs = this.#state.tabs.length; - if (numTabs === 0) return; - - // Find next selectable tab (skip empty+enabled providers) - let nextIndex = this.#state.activeTabIndex; - for (let i = 0; i < numTabs; i++) { - nextIndex = (nextIndex + direction + numTabs) % numTabs; - const tab = this.#state.tabs[nextIndex]; - const isEmptyEnabled = tab.count === 0 && tab.enabled && tab.id !== "all"; - if (!isEmptyEnabled) break; - } - this.#state.activeTabIndex = nextIndex; - - // Re-filter for new tab - const tab = this.#state.tabs[this.#state.activeTabIndex]; - this.#state.tabFiltered = filterByProvider(this.#state.extensions, tab.id); - this.#state.searchFiltered = applyFilter(this.#state.tabFiltered, this.#state.searchQuery); - this.#state.listIndex = 0; - this.#state.scrollOffset = 0; - this.#state.selected = this.#state.searchFiltered[0] ?? null; - - // Update list - this.#mainList.setExtensions(this.#state.searchFiltered); - this.#mainList.setMasterSwitchProvider(this.#getActiveProviderId()); - this.#mainList.resetSelection(); - - if (this.#state.selected) { - this.#inspector.setExtension(this.#state.selected); - } - - this.#buildLayout(); - } - handleInput(data: string): void { + // SGR mouse reports (the fullscreen overlay enables tracking). + if (data.startsWith("\x1b[<")) { + this.#handleMouse(data); + return; + } + // Ctrl+C - close immediately if (matchesKey(data, "ctrl+c")) { this.onClose?.(); @@ -327,56 +340,99 @@ export class ExtensionDashboard extends Container { this.#state.searchFiltered = this.#state.tabFiltered; this.#mainList.setExtensions(this.#state.searchFiltered); this.#mainList.clearSearch(); - this.#buildLayout(); + this.onRequestRender?.(); return; } this.onClose?.(); return; } - // Tab/Shift+Tab or Left/Right: Cycle through tabs - if (handleTabSwitchKey(data, direction => this.#switchTab(direction))) { + // Tab/Shift+Tab or ←/→: switch provider tabs (fires onTabChange). + if (this.#tabBar.handleInput(data)) { return; } - // All other input goes to the list + // All other input goes to the list. this.#mainList.handleInput(data); - // Sync search query back to state + // Sync search query back to state. const query = this.#mainList.getSearchQuery(); if (query !== this.#state.searchQuery) { this.#state.searchQuery = query; this.#state.searchFiltered = applyFilter(this.#state.tabFiltered, query); } + this.onRequestRender?.(); } } /** - * Two-column body component for side-by-side rendering. + * Two-column body: inventory list on the left, inspector on the right, split by + * a vertical rule. The inspector is a {@link ScrollView} viewport so long detail + * panes scroll (wheel) with an auto scrollbar; the left list manages its own + * windowing. Records the left-column width so the host can hit-test panes. */ class TwoColumnBody implements Component { + #maxHeight: number; + #rightScroll = 0; + #rightTotal = 0; + #leftWidth = 0; + constructor( private readonly leftPane: ExtensionList, private readonly rightPane: InspectorPanel, - private readonly maxHeight: number, - ) {} + maxHeight: number, + ) { + this.#maxHeight = maxHeight; + } + + setMaxHeight(maxHeight: number): void { + this.#maxHeight = maxHeight; + } + + /** Content width of the left (list) column from the last render. */ + get leftWidth(): number { + return this.#leftWidth; + } + + resetInspectorScroll(): void { + this.#rightScroll = 0; + } + + /** Wheel notch over the inspector pane: scroll its content, clamped. */ + scrollInspector(delta: -1 | 1): void { + const max = Math.max(0, this.#rightTotal - this.#maxHeight); + this.#rightScroll = Math.max(0, Math.min(this.#rightScroll + delta, max)); + } render(width: number): readonly string[] { const leftWidth = Math.floor(width * 0.5); + this.#leftWidth = leftWidth; const rightWidth = Math.max(0, width - leftWidth - 3); + const numLines = this.#maxHeight; const leftLines = this.leftPane.render(leftWidth); const rightLines = this.rightPane.render(rightWidth); + this.#rightTotal = rightLines.length; + const maxScroll = Math.max(0, this.#rightTotal - numLines); + if (this.#rightScroll > maxScroll) this.#rightScroll = maxScroll; + + // `totalRows` omitted so the ScrollView windows `rightLines` by the scroll + // offset (rather than treating them as a pre-windowed slice) and pads short + // content to exactly `numLines`. + const rightView = new ScrollView(rightLines, { + height: numLines, + scrollbar: "auto", + theme: { track: t => theme.fg("muted", t), thumb: t => theme.fg("accent", t) }, + }); + rightView.setScrollOffset(this.#rightScroll); + const rightRendered = rightView.render(rightWidth); - // Fill the full body height so the dashboard reads as a full-screen view. - const numLines = this.maxHeight; const combined: string[] = []; const separator = theme.fg("dim", ` ${theme.boxRound.vertical} `); - for (let i = 0; i < numLines; i++) { const left = truncateToWidth(leftLines[i] ?? "", leftWidth); const leftPadded = left + padding(Math.max(0, leftWidth - visibleWidth(left))); - const right = truncateToWidth(rightLines[i] ?? "", rightWidth); + const right = rightRendered[i] ?? ""; combined.push(leftPadded + separator + right); } diff --git a/packages/coding-agent/src/modes/components/extensions/extension-list.ts b/packages/coding-agent/src/modes/components/extensions/extension-list.ts index b095a8ab5..cf6cf8d86 100644 --- a/packages/coding-agent/src/modes/components/extensions/extension-list.ts +++ b/packages/coding-agent/src/modes/components/extensions/extension-list.ts @@ -40,6 +40,9 @@ export class ExtensionList implements Component { #focused = false; #masterSwitchProvider: string | null = null; #maxVisible: number; + #hoveredIndex: number | null = null; + /** Item rows rendered in the last frame, for mouse hit-testing. */ + #visibleCount = 0; constructor( private extensions: Extension[], @@ -108,6 +111,7 @@ export class ExtensionList implements Component { render(width: number): readonly string[] { const lines: string[] = []; + this.#visibleCount = 0; // Search bar const searchPrefix = theme.fg("muted", "Search: "); @@ -136,15 +140,20 @@ export class ExtensionList implements Component { for (let i = startIdx; i < endIdx; i++) { const listItem = this.#listItems[i]; const isSelected = this.#focused && i === this.#selectedIndex; + const isHovered = this.#focused && i === this.#hoveredIndex && !isSelected; + let rowStr: string; if (listItem.type === "master") { - rows.push(this.#renderMasterSwitch(listItem, isSelected, rowWidth)); + rowStr = this.#renderMasterSwitch(listItem, isSelected, rowWidth); } else if (listItem.type === "kind-header") { - rows.push(this.#renderKindHeader(listItem, isSelected, rowWidth)); + rowStr = this.#renderKindHeader(listItem, isSelected, rowWidth); } else { - rows.push(this.#renderExtensionRow(listItem.item, isSelected, rowWidth, masterDisabled)); + rowStr = this.#renderExtensionRow(listItem.item, isSelected, rowWidth, masterDisabled); } + if (isHovered) rowStr = theme.bg("selectedBg", rowStr); + rows.push(rowStr); } + this.#visibleCount = rows.length; lines.push( ...renderScrollableList(rows, { @@ -387,6 +396,57 @@ export class ExtensionList implements Component { this.#scrollOffset = next.scrollOffset; } + /** Toggle the selected item, or flip the provider master switch when on it. */ + #activateSelected(): void { + const item = this.#listItems[this.#selectedIndex]; + if (item?.type === "master") { + this.callbacks.onMasterToggle?.(item.providerId); + } else if (item?.type === "extension") { + // Only allow toggling if the provider master switch is enabled. + const masterDisabled = this.#masterSwitchProvider !== null && !isProviderEnabled(this.#masterSwitchProvider); + if (!masterDisabled) { + const newEnabled = item.item.state === "disabled"; + this.callbacks.onToggle?.(item.item.id, newEnabled); + } + } + } + + /** Highlight the row under the pointer (null clears). */ + setHoverIndex(index: number | null): void { + this.#hoveredIndex = index; + } + + /** + * Map a 0-based line within this component's render to the absolute list-item + * index, or null when the line is the search banner, a padding row, or outside + * the visible window. The first two lines are the search banner and a blank + * separator; item rows follow, windowed at the current scroll offset. + */ + hitTest(line: number): number | null { + const rowLine = line - 2; + if (rowLine < 0 || rowLine >= this.#visibleCount) return null; + const index = this.#scrollOffset + rowLine; + return index < this.#listItems.length ? index : null; + } + + /** Wheel notch: move the selection (and the inspector) one row. */ + handleWheel(delta: -1 | 1): void { + if (delta < 0) this.#moveSelectionUp(); + else this.#moveSelectionDown(); + } + + /** Click: select the row under the pointer, or activate it when already selected. */ + handleClick(line: number): void { + const index = this.hitTest(line); + if (index === null) return; + if (index === this.#selectedIndex) { + this.#activateSelected(); + return; + } + this.#selectedIndex = index; + this.#notifySelectionChange(); + } + handleInput(data: string): void { // Navigation if (matchesSelectUp(data) || data === "k") { @@ -399,36 +459,9 @@ export class ExtensionList implements Component { return; } - // Space: Toggle selected item - if (data === " ") { - const item = this.#listItems[this.#selectedIndex]; - if (item?.type === "master") { - this.callbacks.onMasterToggle?.(item.providerId); - } else if (item?.type === "extension") { - // Only allow toggling if master is enabled - const masterDisabled = - this.#masterSwitchProvider !== null && !isProviderEnabled(this.#masterSwitchProvider); - if (!masterDisabled) { - const newEnabled = item.item.state === "disabled"; - this.callbacks.onToggle?.(item.item.id, newEnabled); - } - } - return; - } - - // Enter: Same as space - toggle selected item - if (matchesKey(data, "enter") || matchesKey(data, "return") || data === "\n") { - const item = this.#listItems[this.#selectedIndex]; - if (item?.type === "master") { - this.callbacks.onMasterToggle?.(item.providerId); - } else if (item?.type === "extension") { - const masterDisabled = - this.#masterSwitchProvider !== null && !isProviderEnabled(this.#masterSwitchProvider); - if (!masterDisabled) { - const newEnabled = item.item.state === "disabled"; - this.callbacks.onToggle?.(item.item.id, newEnabled); - } - } + // Space or Enter: activate the selected row (toggle item / master switch) + if (data === " " || matchesKey(data, "enter") || matchesKey(data, "return") || data === "\n") { + this.#activateSelected(); return; } diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index d462f4bf4..5ef346fe0 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -232,11 +232,15 @@ export class SelectorController { */ async showExtensionsDashboard(): Promise { const dashboard = await ExtensionDashboard.create(getProjectDir(), this.ctx.settings, this.ctx.ui.terminal.rows); + // Fullscreen dashboard on the alternate screen (the /settings idiom): the + // overlay borrows the terminal's alt buffer and enables mouse tracking for + // its lifetime, leaving the transcript untouched underneath. const overlay = this.ctx.ui.showOverlay(dashboard, { width: "100%", maxHeight: "100%", anchor: "top-left", margin: 0, + fullscreen: true, }); dashboard.onClose = () => { overlay.hide(); diff --git a/packages/coding-agent/test/agent-session-message-pipeline.test.ts b/packages/coding-agent/test/agent-session-message-pipeline.test.ts index 96ba0e79e..4c1d4db3a 100644 --- a/packages/coding-agent/test/agent-session-message-pipeline.test.ts +++ b/packages/coding-agent/test/agent-session-message-pipeline.test.ts @@ -14,12 +14,15 @@ import { } from "@oh-my-pi/pi-ai"; import { AssistantMessageEventStream } from "@oh-my-pi/pi-ai/utils/event-stream"; 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 * as memoryBackend from "@oh-my-pi/pi-coding-agent/memory-backend"; import type { MemoryBackend } from "@oh-my-pi/pi-coding-agent/memory-backend/types"; import { type MnemopiSessionState, setMnemopiSessionState } from "@oh-my-pi/pi-coding-agent/mnemopi/state"; +import { createAgentSession, type ExtensionFactory } from "@oh-my-pi/pi-coding-agent/sdk"; import { obfuscateProviderContext, SecretObfuscator } from "@oh-my-pi/pi-coding-agent/secrets"; import { AgentSession, type AgentSessionEvent } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; import { convertToLlm, wrapSteeringForModel } from "@oh-my-pi/pi-coding-agent/session/messages"; import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; import { TempDir } from "@oh-my-pi/pi-utils"; @@ -689,6 +692,94 @@ describe("AgentSession message pipeline", () => { expect(contexts[1]!.systemPrompt).toEqual(firstSystemPrompt); }); + it("preserves append-only prefixes in subagent sessions when context handlers rewrite prior turns", async () => { + using tempDir = TempDir.createSync("@pi-subagent-append-only-"); + const api = "test-subagent-append-only-cache"; + const contexts: Context[] = []; + registerCustomApi(api, (_model, context) => { + contexts.push(context); + const stream = new AssistantMessageEventStream(); + queueMicrotask(() => { + const message = createAssistantMessage(`ok-${contexts.length}`); + stream.push({ type: "text_delta", contentIndex: 0, delta: "ok", partial: message }); + stream.push({ type: "done", reason: "stop", message }); + }); + return stream; + }); + const model = buildModel({ + id: "local-subagent-model", + name: "Local Subagent Model", + api, + provider: "llama.cpp", + baseUrl: "http://127.0.0.1:8080/v1", + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 4096, + maxTokens: 1024, + } as ModelSpec) as Model; + const rewritePriorAssistant: ExtensionFactory = pi => { + pi.on("context", async event => { + const hasSecondTurn = event.messages.some(message => { + if (message.role !== "user") return false; + const content = message.content; + if (typeof content === "string") return content.includes("second"); + return content.some(part => part.type === "text" && part.text.includes("second")); + }); + if (!hasSecondTurn) return undefined; + return { + messages: event.messages.map(message => + message.role === "assistant" + ? { ...message, content: [{ type: "text" as const, text: "rewritten assistant" }] } + : message, + ), + }; + }); + }; + const authStorage = await AuthStorage.create(tempDir.join("auth.db")); + const modelRegistry = new ModelRegistry(authStorage, tempDir.join("models.yml")); + const { session } = await createAgentSession({ + cwd: tempDir.path(), + agentDir: tempDir.path(), + sessionManager: SessionManager.inMemory(tempDir.path()), + authStorage, + modelRegistry, + settings: Settings.isolated({ + "compaction.enabled": false, + "provider.appendOnlyContext": "auto", + }), + model, + disableExtensionDiscovery: true, + extensions: [rewritePriorAssistant], + skills: [], + contextFiles: [], + promptTemplates: [], + slashCommands: [], + enableMCP: false, + enableLsp: false, + skipPythonPreflight: true, + taskDepth: 1, + agentId: "SubAgent", + }); + try { + expect(session.agent.appendOnlyContext).toBeDefined(); + + await session.sendUserMessage("first"); + await session.sendUserMessage("second"); + + expect(contexts).toHaveLength(2); + expect(contexts[0]!.messages).toHaveLength(1); + expect(contexts[1]!.messages).toHaveLength(3); + expect(contexts[1]!.messages[0]).toBe(contexts[0]!.messages[0]); + expect((contexts[1]!.messages[1] as { content: unknown }).content).toEqual([ + { type: "text", text: "rewritten assistant" }, + ]); + } finally { + await session.dispose(); + authStorage.close(); + } + }); + it("clears promoted memory from the base prompt when switching sessions", async () => { using tempDir = TempDir.createSync("@pi-injected-memory-switch-"); const sessionManager = SessionManager.create(tempDir.path(), tempDir.join("sessions")); diff --git a/packages/coding-agent/test/extensibility/legacy-pi-compat-entrypoints.test.ts b/packages/coding-agent/test/extensibility/legacy-pi-compat-entrypoints.test.ts new file mode 100644 index 000000000..60a7b5d75 --- /dev/null +++ b/packages/coding-agent/test/extensibility/legacy-pi-compat-entrypoints.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from "bun:test"; +import * as path from "node:path"; +import { LEGACY_COMPAT_BUILD_ENTRYPOINTS } from "../../../../scripts/binary-entrypoints"; + +/** + * Regression for issue #3414. + * + * `legacy-pi-compat.ts` redirects bare `typebox` / `@sinclair/typebox` imports + * and the legacy `@(scope)/pi-*` package roots onto computed bunfs paths under + * `/$bunfs/root/packages/coding-agent/src/extensibility/` and + * `/$bunfs/root/packages//...`. Bun's `--compile` static analyzer + * never visits those literals, so each shim must be passed to `bun build` as + * an extra entrypoint or it is silently omitted from bunfs and plugin loads + * fail with `Cannot find module '/$bunfs/root/.../typebox.js'`. + * + * Commit `dc5c93462f` removed the worker entrypoints from + * `scripts/ci-release-build-binaries.ts` but its inline comment falsely + * claimed the legacy-shim entrypoints were "still" listed — they had never + * been re-added. Released `omp-darwin-arm64` and `omp-linux-x64` shipped + * without the shims, breaking every legacy pi plugin install. + * + * Contract pinned here: + * - Both build scripts (release CI + local dev) feed the SAME entrypoint + * list to `bun build --compile`, sourced from `binary-entrypoints.ts`. + * - That list covers every shim referenced by `legacy-pi-compat.ts`. + */ +describe("legacy pi-compat --compile entrypoints (issue #3414)", () => { + const repoRoot = path.resolve(import.meta.dir, "../../../.."); + const ciScriptPath = path.join(repoRoot, "scripts/ci-release-build-binaries.ts"); + const devScriptPath = path.join(repoRoot, "packages/coding-agent/scripts/build-binary.ts"); + const compatPath = path.join(repoRoot, "packages/coding-agent/src/extensibility/plugins/legacy-pi-compat.ts"); + + it("declares every shim file legacy-pi-compat redirects to as a --compile entrypoint", () => { + // The shared constant must list every bunfs file the resolver hands back + // to Bun. Drop one here and the corresponding plugin import breaks at + // runtime with `Cannot find module '/$bunfs/root/...'`. + expect(LEGACY_COMPAT_BUILD_ENTRYPOINTS).toEqual([ + "./packages/agent/src/index.ts", + "./packages/natives/native/index.js", + "./packages/tui/src/index.ts", + "./packages/utils/src/index.ts", + "./packages/coding-agent/src/extensibility/typebox.ts", + "./packages/coding-agent/src/extensibility/legacy-pi-ai-shim.ts", + "./packages/coding-agent/src/extensibility/legacy-pi-coding-agent-shim.ts", + ]); + }); + + it("release and dev build scripts both source entrypoints from binary-entrypoints.ts", async () => { + const releaseSource = await Bun.file(ciScriptPath).text(); + const devSource = await Bun.file(devScriptPath).text(); + + // Both scripts import the shared constant rather than inlining their + // own copy, so the two halves of the contract cannot drift apart. + expect(releaseSource).toContain("import { LEGACY_COMPAT_BUILD_ENTRYPOINTS } from"); + expect(releaseSource).toContain("binary-entrypoints"); + expect(releaseSource).toContain("...LEGACY_COMPAT_BUILD_ENTRYPOINTS"); + + expect(devSource).toContain("import { LEGACY_COMPAT_BUILD_ENTRYPOINTS } from"); + expect(devSource).toContain("binary-entrypoints"); + expect(devSource).toContain("...LEGACY_COMPAT_BUILD_ENTRYPOINTS"); + }); + + it("each entrypoint actually exists on disk under the repo root", async () => { + // `--compile` silently swallows a missing entrypoint when run without + // `--root .` mismatches, then drops the file from bunfs and ships a + // broken binary. Make every path a real file so the build fails loudly + // if the constant ever lists a stale path. + for (const relative of LEGACY_COMPAT_BUILD_ENTRYPOINTS) { + const abs = path.join(repoRoot, relative.replace(/^\.\//, "")); + expect(await Bun.file(abs).exists()).toBe(true); + } + }); + + it("legacy-pi-compat.ts only redirects to shims declared in the entrypoint list", async () => { + // Every `bunfsPath("", "", ..., ".js")` and + // `bunfsPath("coding-agent", "src", "extensibility", ".js")` in + // the compat module must have a corresponding `.ts` (or `.js`) entry + // in `LEGACY_COMPAT_BUILD_ENTRYPOINTS`. Otherwise a plugin import goes + // to a bunfs file that wasn't compiled in. + const compatSource = await Bun.file(compatPath).text(); + const bunfsPathPattern = /bunfsPath\(([^)]+)\)/g; + const segmentsList = [...compatSource.matchAll(bunfsPathPattern)] + .map(match => match[1]) + .filter((segment): segment is string => typeof segment === "string"); + expect(segmentsList.length).toBeGreaterThan(0); + + for (const segmentsExpr of segmentsList) { + const segments = [...segmentsExpr.matchAll(/"([^"]+)"/g)] + .map(match => match[1]) + .filter((seg): seg is string => typeof seg === "string"); + if (segments.length === 0) continue; + + // Convert the bunfs path tail (e.g. `tui/src/index.js`) to the + // matching repo-root build entrypoint (`./packages/tui/src/index.{ts,js}`). + const bunfsTail = segments.join("/"); + const matched = LEGACY_COMPAT_BUILD_ENTRYPOINTS.some(entry => { + const stripped = entry.replace(/^\.\/packages\//, "").replace(/\.(ts|js)$/, ""); + const bunfsExpected = bunfsTail.replace(/\.(js|ts)$/, ""); + return stripped === bunfsExpected; + }); + expect(matched, `bunfsPath(${segmentsExpr}) has no matching --compile entrypoint`).toBe(true); + } + }); +}); diff --git a/packages/coding-agent/test/extensibility/legacy-pi-typebox-shim-validation.test.ts b/packages/coding-agent/test/extensibility/legacy-pi-typebox-shim-validation.test.ts new file mode 100644 index 000000000..363d03bc6 --- /dev/null +++ b/packages/coding-agent/test/extensibility/legacy-pi-typebox-shim-validation.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "bun:test"; +import * as path from "node:path"; +import { __resolveTypeBoxShimPath } from "@oh-my-pi/pi-coding-agent/extensibility/plugins/legacy-pi-compat"; + +/** + * Defensive tier for issue #3414. + * + * Even with the entrypoint list pinned in `binary-entrypoints.ts`, a future + * Bun `--compile` regression (cf. #2168) can silently drop the typebox shim + * file from bunfs. Without runtime validation the resolver would emit a + * `file://` URL to a path that no longer exists and plugin loads fail with + * `Cannot find module '/$bunfs/root/.../typebox.js'`. + * + * `__resolveTypeBoxShimPath` mirrors the `__validateLegacyPiPackageRootOverrides` + * fallback already in place for the legacy package-root overrides: if the + * computed candidate is missing, return `null` so the rewriter leaves bare + * `typebox` / `@sinclair/typebox` specifiers alone and Bun resolves a real + * install from the extension's own `node_modules`. + */ +describe("legacy pi-compat typebox shim path validation (issue #3414)", () => { + const stubMetaDir = "/repo/packages/coding-agent/src/extensibility/plugins"; + + it("returns the bunfs candidate when --compile bundled the shim", () => { + const result = __resolveTypeBoxShimPath( + "/$bunfs/root/packages", + undefined, + "/$bunfs/root", + () => true, + path.posix, + ); + expect(result).toBe("/$bunfs/root/packages/coding-agent/src/extensibility/typebox.js"); + }); + + it("drops the bunfs candidate when --compile silently omitted the shim", () => { + const result = __resolveTypeBoxShimPath( + "/$bunfs/root/packages", + undefined, + "/$bunfs/root", + () => false, + path.posix, + ); + expect(result).toBeNull(); + }); + + it("preserves the double-slash bunfs prefix on cross-compiled darwin-arm64 (issue #3329)", () => { + // `BUNFS_PACKAGE_ROOT` on the cross-compiled release is `//root/packages`. + // The shim path must keep that exact double-slash mount prefix so Bun's + // bunfs lookup matches. + const result = __resolveTypeBoxShimPath( + "//root/packages", + undefined, + "//root/omp-darwin-arm64", + () => true, + path.posix, + ); + expect(result).toBe("//root/packages/coding-agent/src/extensibility/typebox.js"); + }); + + it("returns the npm prebuilt dist path when PI_BUNDLED is set", () => { + const bundledRoot = "/home/me/.bun/install/global/node_modules/@oh-my-pi/pi-coding-agent"; + const result = __resolveTypeBoxShimPath(null, bundledRoot, stubMetaDir, () => true, path.posix); + expect(result).toBe(`${bundledRoot}/src/extensibility/typebox.ts`); + }); + + it("returns the dev source path when neither bunfs nor bundled is set", () => { + const result = __resolveTypeBoxShimPath(null, undefined, stubMetaDir, () => true, path.posix); + expect(result).toBe("/repo/packages/coding-agent/src/extensibility/typebox.ts"); + }); + + it("returns null when the dev source path is missing too", () => { + const result = __resolveTypeBoxShimPath(null, undefined, stubMetaDir, () => false, path.posix); + expect(result).toBeNull(); + }); +}); diff --git a/packages/coding-agent/test/extension-list-mouse.test.ts b/packages/coding-agent/test/extension-list-mouse.test.ts new file mode 100644 index 000000000..81ba31b9c --- /dev/null +++ b/packages/coding-agent/test/extension-list-mouse.test.ts @@ -0,0 +1,129 @@ +import { beforeAll, describe, expect, test } from "bun:test"; +import { buildTabBarTabs } from "@oh-my-pi/pi-coding-agent/modes/components/extensions/extension-dashboard"; +import { ExtensionList } from "@oh-my-pi/pi-coding-agent/modes/components/extensions/extension-list"; +import type { Extension } from "@oh-my-pi/pi-coding-agent/modes/components/extensions/types"; +import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; + +beforeAll(async () => { + await initTheme(false); +}); + +function skill(name: string, state: Extension["state"] = "active"): Extension { + return { + id: `skill:${name}`, + kind: "skill", + name, + displayName: name, + path: `/tmp/skill-${name}`, + source: { provider: "native", providerName: "Native", level: "native" }, + state, + raw: {}, + }; +} + +/** + * ALL view layout (no master switch): the list renders a search banner, a blank + * separator, one kind header ("Skills"), then one row per extension. Rendered + * line indices therefore are: 0 search, 1 blank, 2 header, 3 alpha, 4 beta, + * 5 gamma. `hitTest` maps those lines to absolute list-item indices. + */ +function buildList(onToggle: (id: string, enabled: boolean) => void, onSelect: (ext: Extension | null) => void) { + const list = new ExtensionList([skill("alpha"), skill("beta"), skill("gamma")], { + masterSwitchProvider: null, + onSelectionChange: onSelect, + onToggle, + }); + list.setFocused(true); + list.render(40); // populate hit rows + visible-count window + return list; +} + +describe("ExtensionList mouse routing", () => { + test("hitTest skips the search banner and blank row, mapping item rows by offset", () => { + const list = buildList( + () => {}, + () => {}, + ); + expect(list.hitTest(0)).toBeNull(); // search banner + expect(list.hitTest(1)).toBeNull(); // blank separator + expect(list.hitTest(2)).toBe(0); // kind header + expect(list.hitTest(3)).toBe(1); // alpha + expect(list.hitTest(5)).toBe(3); // gamma + expect(list.hitTest(6)).toBeNull(); // past the last rendered row + }); + + test("clicking a row selects its extension; clicking the selected row toggles it", () => { + const toggles: Array<{ id: string; enabled: boolean }> = []; + let selected: Extension | null = null; + const list = buildList( + (id, enabled) => toggles.push({ id, enabled }), + ext => { + selected = ext; + }, + ); + + // First click on alpha's row (line 3) selects it — no toggle yet. + list.handleClick(3); + expect(list.getSelectedExtension()?.id).toBe("skill:alpha"); + expect(selected).not.toBeNull(); + expect(toggles).toHaveLength(0); + + // Clicking the already-selected row activates it; an active skill toggles off. + list.handleClick(3); + expect(toggles).toEqual([{ id: "skill:alpha", enabled: false }]); + }); + + test("clicking the search banner or a padding row is a no-op", () => { + const toggles: string[] = []; + const list = buildList( + id => toggles.push(id), + () => {}, + ); + list.handleClick(0); // search banner + list.handleClick(9); // below the last row + expect(list.getSelectedExtension()).toBeNull(); // selection still on the header + expect(toggles).toHaveLength(0); + }); + + test("wheel notches move the selection like j/k", () => { + const seen: Array = []; + const list = buildList( + () => {}, + ext => seen.push(ext?.id ?? null), + ); + list.handleClick(3); // select alpha (index 1) + list.handleWheel(1); // down -> beta + expect(list.getSelectedExtension()?.id).toBe("skill:beta"); + list.handleWheel(-1); // up -> alpha + expect(list.getSelectedExtension()?.id).toBe("skill:alpha"); + expect(seen).toEqual(["skill:alpha", "skill:beta", "skill:alpha"]); + }); +}); + +describe("buildProviderTabs", () => { + test("mutes empty enabled providers but keeps disabled providers selectable", () => { + const tabs = buildTabBarTabs([ + { id: "all", label: "ALL", enabled: true, count: 5 }, + { id: "skills", label: "Skills", enabled: true, count: 3 }, + { id: "empty", label: "Empty", enabled: true, count: 0 }, + { id: "off", label: "Off", enabled: false, count: 2 }, + ]); + + const all = tabs.find(t => t.id === "all"); + const skills = tabs.find(t => t.id === "skills"); + const empty = tabs.find(t => t.id === "empty"); + const off = tabs.find(t => t.id === "off"); + + // The "all" tab is never muted or marked; it still shows its total count. + expect(all?.muted).toBe(false); + expect(all?.label).toBe("ALL (5)"); + // A populated enabled provider shows its count and stays selectable. + expect(skills?.muted).toBe(false); + expect(skills?.label).toBe("Skills (3)"); + // Empty *enabled* provider is muted (unselectable). + expect(empty?.muted).toBe(true); + // Disabled provider stays selectable (not muted) so it can be re-enabled. + expect(off?.muted).toBe(false); + expect(off?.label).toContain("Off (2)"); + }); +}); diff --git a/scripts/binary-entrypoints.ts b/scripts/binary-entrypoints.ts new file mode 100644 index 000000000..81f652673 --- /dev/null +++ b/scripts/binary-entrypoints.ts @@ -0,0 +1,30 @@ +/** + * Extra `bun build --compile` entrypoints that must land in bunfs at + * `/$bunfs/root/packages//...`. + * + * `legacy-pi-compat.ts` redirects bare TypeBox imports and the legacy + * `@(scope)/pi-*` package roots onto computed bunfs paths — Bun's `--compile` + * static analyzer cannot trace those, so each shim and bundled package barrel + * must be passed as an explicit additional entrypoint or the file is silently + * omitted from bunfs and plugin loads fail with `Cannot find module + * '/$bunfs/root/...'` (issue #3414). + * + * The coding-agent's own `./src/index.ts` is intentionally NOT listed: bun + * --compile silently breaks the CLI entry when the same package's barrel + * appears as an extra entrypoint (issue #1474), so legacy `pi-coding-agent` + * imports resolve through `legacy-pi-coding-agent-shim.ts` instead. + * + * Repo-root-relative — both `scripts/ci-release-build-binaries.ts` (release) + * and `packages/coding-agent/scripts/build-binary.ts` (dev) invoke + * `bun build --compile --root .` so the paths land at the same bunfs + * locations the runtime computes. + */ +export const LEGACY_COMPAT_BUILD_ENTRYPOINTS: readonly string[] = [ + "./packages/agent/src/index.ts", + "./packages/natives/native/index.js", + "./packages/tui/src/index.ts", + "./packages/utils/src/index.ts", + "./packages/coding-agent/src/extensibility/typebox.ts", + "./packages/coding-agent/src/extensibility/legacy-pi-ai-shim.ts", + "./packages/coding-agent/src/extensibility/legacy-pi-coding-agent-shim.ts", +]; diff --git a/scripts/ci-release-build-binaries.ts b/scripts/ci-release-build-binaries.ts index 8a6f65e2a..8ec0b3a3d 100644 --- a/scripts/ci-release-build-binaries.ts +++ b/scripts/ci-release-build-binaries.ts @@ -2,6 +2,7 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; +import { LEGACY_COMPAT_BUILD_ENTRYPOINTS } from "./binary-entrypoints"; interface BinaryTarget { id: string; @@ -14,10 +15,14 @@ interface BinaryTarget { const repoRoot = path.join(import.meta.dir, ".."); const binariesDir = path.join(repoRoot, "packages", "coding-agent", "binaries"); const entrypoint = "./packages/coding-agent/src/cli.ts"; -// Legacy extension shims and package barrels in the build argv below are still -// explicit `--compile` entrypoints because they are reached via computed bunfs -// paths. Worker threads spawn `new Worker(Bun.main, { argv })` — they re-enter -// the binary's own entry module — so no separate worker modules are compiled. +// Legacy extension shims and package barrels reached via computed bunfs paths. +// Declared in `binary-entrypoints.ts` so the dev build script +// (`packages/coding-agent/scripts/build-binary.ts`) and this release script +// stay in sync — drifting them silently strips the shims from the release +// `omp-` assets and breaks `omp install` for legacy pi extensions +// (issue #3414). Worker threads spawn `new Worker(Bun.main, { argv })` — they +// re-enter the binary's own entry module — so no separate worker modules +// are compiled. const isDryRun = process.argv.includes("--dry-run"); const targets: BinaryTarget[] = [ { @@ -110,7 +115,7 @@ async function buildBinary(target: BinaryTarget): Promise { console.log(`Building ${target.outfile}...`); await embedNative(target); if (isDryRun) { - console.log(`DRY RUN bun build --compile --no-compile-autoload-bunfig --no-compile-autoload-dotenv --no-compile-autoload-tsconfig --no-compile-autoload-package-json --minify-identifiers --keep-names --define process.env.PI_COMPILED="true" --root . --target=${target.target} ${entrypoint} --outfile ${target.outfile}`); + console.log(`DRY RUN bun build --compile --no-compile-autoload-bunfig --no-compile-autoload-dotenv --no-compile-autoload-tsconfig --no-compile-autoload-package-json --minify-identifiers --keep-names --define process.env.PI_COMPILED="true" --root . --target=${target.target} ${entrypoint} ${LEGACY_COMPAT_BUILD_ENTRYPOINTS.join(" ")} --outfile ${target.outfile}`); return; } @@ -135,6 +140,7 @@ async function buildBinary(target: BinaryTarget): Promise { "--target", target.target, entrypoint, + ...LEGACY_COMPAT_BUILD_ENTRYPOINTS, "--outfile", target.outfile, ],