From 44e5e0bb803366a3a590e45ffb57e2a9dfe7c9c4 Mon Sep 17 00:00:00 2001 From: cognitive <152830360+METAeuPHORIC@users.noreply.github.com> Date: Wed, 13 May 2026 08:17:15 +0000 Subject: [PATCH] fix(coding-agent/tui): rendered queued /skill: as compact pending chip --- .../src/modes/controllers/input-controller.ts | 13 + .../coding-agent/src/session/agent-session.ts | 102 ++++- packages/coding-agent/src/session/messages.ts | 45 ++ .../src/session/session-manager.ts | 6 +- .../test/input-controller-skill-queue.test.ts | 431 ++++++++++++++++++ .../session-manager-internal-details.test.ts | 137 ++++++ 6 files changed, 716 insertions(+), 18 deletions(-) create mode 100644 packages/coding-agent/test/input-controller-skill-queue.test.ts create mode 100644 packages/coding-agent/test/session-manager-internal-details.test.ts diff --git a/packages/coding-agent/src/modes/controllers/input-controller.ts b/packages/coding-agent/src/modes/controllers/input-controller.ts index b3885662d..86c51040f 100644 --- a/packages/coding-agent/src/modes/controllers/input-controller.ts +++ b/packages/coding-agent/src/modes/controllers/input-controller.ts @@ -443,6 +443,15 @@ export class InputController { args: args || undefined, lineCount: body ? body.split("\n").length : 0, }; + // When the agent is streaming, register the compact slash-form text as + // the pending-display twin BEFORE dispatching the CustomMessage. The + // returned tag is embedded in details so AgentSession.#handleAgentEvent + // can remove the matching display entry when the agent consumes this + // message (mirrors the user-message dequeue path). + if (this.ctx.session.isStreaming) { + const tag = this.ctx.session.enqueueCustomMessageDisplay(text, streamingBehavior); + details.__pendingDisplayTag = tag; + } await this.ctx.session.promptCustomMessage( { customType: SKILL_PROMPT_MESSAGE_TYPE, @@ -453,6 +462,10 @@ export class InputController { }, { streamingBehavior }, ); + if (this.ctx.session.isStreaming) { + this.ctx.updatePendingMessagesDisplay(); + this.ctx.ui.requestRender(); + } } catch (err) { this.ctx.showError(`Failed to load skill: ${err instanceof Error ? err.message : String(err)}`); } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 8aec02120..06b02b8ed 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -174,6 +174,7 @@ import { convertToLlm, type FileMentionMessage, type PythonExecutionMessage, + readPendingDisplayTag, } from "./messages"; import { formatSessionDumpText } from "./session-dump-format"; import type { @@ -594,6 +595,13 @@ function extractPermissionLocations(args: unknown, cwd: string): { path: string; // AgentSession Class // ============================================================================ +/** Internal record stored in the steering/followUp display queues. The optional + * `tag` is set only by `enqueueCustomMessageDisplay` (used for skill-prompt + * custom messages queued during streaming) and is matched by the custom-role + * `message_start` dequeue branch; user-message pushes leave it undefined and + * rely on the existing text-equality match. */ +type QueuedDisplayEntry = { text: string; tag?: string }; + export class AgentSession { readonly agent: Agent; readonly sessionManager: SessionManager; @@ -612,10 +620,15 @@ export class AgentSession { #unsubscribeAgent?: () => void; #eventListeners: AgentSessionEventListener[] = []; - /** Tracks pending steering messages for UI display. Removed when delivered. */ - #steeringMessages: string[] = []; - /** Tracks pending follow-up messages for UI display. Removed when delivered. */ - #followUpMessages: string[] = []; + /** Tracks pending steering messages for UI display. Removed when delivered. + * Entry shape: `{ text }` for plain-text steers (user-message dequeue + * matches by `.text`); `{ text, tag }` for queued custom messages (skill + * invocations dispatched while streaming) — the custom-role dequeue + * matches by `.tag` so duplicate-args queued skills cannot collide. */ + #steeringMessages: QueuedDisplayEntry[] = []; + /** Tracks pending follow-up messages for UI display. Removed when delivered. + * See `#steeringMessages` for entry shape. */ + #followUpMessages: QueuedDisplayEntry[] = []; /** Messages queued to be included with the next user prompt as context ("asides"). */ #pendingNextTurnMessages: CustomMessage[] = []; #scheduledHiddenNextTurnGeneration: number | undefined = undefined; @@ -729,6 +742,11 @@ export class AgentSession { #ttsrRetryToken = 0; #ttsrResumePromise: Promise | undefined = undefined; #ttsrResumeResolve: (() => void) | undefined = undefined; + + /** Monotonic counter for `enqueueCustomMessageDisplay` tag generation; + * combined with `Date.now()` so tags stay unique even across rapid + * same-tick enqueues. */ + #customDisplayTagCounter = 0; #postPromptTasks = new Set>(); #postPromptTasksPromise: Promise | undefined = undefined; #postPromptTasksResolve: (() => void) | undefined = undefined; @@ -950,6 +968,28 @@ export class AgentSession { return this.#ttsrAbortPending; } + /** Register a compact display string for a custom message that the caller is + * about to dispatch via `promptCustomMessage` / `sendCustomMessage`. + * Returns a stable tag the caller MUST embed in + * `CustomMessage.details.__pendingDisplayTag` so the agent-side + * `message_start` handler can remove the matching display entry when the + * queued message is consumed. + * + * Does NOT push to the agent's steering/followUp queue — that happens + * separately inside `sendCustomMessage`. */ + enqueueCustomMessageDisplay(text: string, mode: "steer" | "followUp"): string { + const tag = `omp-cmd-${Date.now()}-${++this.#customDisplayTagCounter}`; + const displayText = text.trim(); + if (!displayText) return tag; + const entry: QueuedDisplayEntry = { text: displayText, tag }; + if (mode === "steer") { + this.#steeringMessages.push(entry); + } else { + this.#followUpMessages.push(entry); + } + return tag; + } + getAsyncJobSnapshot(options?: { recentLimit?: number }): AsyncJobSnapshot | null { const manager = AsyncJobManager.instance(); if (!manager) return null; @@ -1038,13 +1078,13 @@ export class AgentSession { if (event.type === "message_start" && event.message.role === "user") { const messageText = this.#getUserMessageText(event.message); if (messageText) { - // Check steering queue first - const steeringIndex = this.#steeringMessages.indexOf(messageText); + // Check steering queue first (match by .text on tagged records) + const steeringIndex = this.#steeringMessages.findIndex(e => e.text === messageText); if (steeringIndex !== -1) { this.#steeringMessages.splice(steeringIndex, 1); } else { // Check follow-up queue - const followUpIndex = this.#followUpMessages.indexOf(messageText); + const followUpIndex = this.#followUpMessages.findIndex(e => e.text === messageText); if (followUpIndex !== -1) { this.#followUpMessages.splice(followUpIndex, 1); } @@ -1052,6 +1092,26 @@ export class AgentSession { } } + // Tag-based dequeue for custom messages (skills queued via promptCustomMessage). + // The InputController attached a stable tag via CustomMessage.details when it + // registered the display chip; pull it back here to remove the matching entry + // from the pending bar atomically with the agent's queue consumption. Match by + // tag (not text) — two queued skills with identical args cannot collide. + if (event.type === "message_start" && event.message.role === "custom") { + const tag = readPendingDisplayTag(event.message.details); + if (tag) { + const steerIdx = this.#steeringMessages.findIndex(e => e.tag === tag); + if (steerIdx !== -1) { + this.#steeringMessages.splice(steerIdx, 1); + } else { + const followUpIdx = this.#followUpMessages.findIndex(e => e.tag === tag); + if (followUpIdx !== -1) { + this.#followUpMessages.splice(followUpIdx, 1); + } + } + } + } + // Deobfuscate assistant message content for display emission — the LLM echoes back // obfuscated placeholders, but listeners (TUI, extensions, exporters) must see real // values. The original event.message stays obfuscated so the persistence path below @@ -3719,7 +3779,7 @@ export class AgentSession { */ async #queueSteer(text: string, images?: ImageContent[]): Promise { const displayText = text || (images && images.length > 0 ? "[Image]" : ""); - this.#steeringMessages.push(displayText); + this.#steeringMessages.push({ text: displayText }); const content: (TextContent | ImageContent)[] = [{ type: "text", text }]; if (images && images.length > 0) { content.push(...images); @@ -3737,7 +3797,7 @@ export class AgentSession { */ async #queueFollowUp(text: string, images?: ImageContent[]): Promise { const displayText = text || (images && images.length > 0 ? "[Image]" : ""); - this.#followUpMessages.push(displayText); + this.#followUpMessages.push({ text: displayText }); const content: (TextContent | ImageContent)[] = [{ type: "text", text }]; if (images && images.length > 0) { content.push(...images); @@ -3973,8 +4033,8 @@ export class AgentSession { * Useful for restoring to editor when user aborts. */ clearQueue(): { steering: string[]; followUp: string[] } { - const steering = [...this.#steeringMessages]; - const followUp = [...this.#followUpMessages]; + const steering = this.#steeringMessages.map(e => e.text); + const followUp = this.#followUpMessages.map(e => e.text); this.#steeringMessages = []; this.#followUpMessages = []; this.agent.clearAllQueues(); @@ -3986,27 +4046,35 @@ export class AgentSession { return this.#steeringMessages.length + this.#followUpMessages.length + this.#pendingNextTurnMessages.length; } - /** Get pending messages (read-only) */ + /** Get pending messages (read-only). Returns the public text-only view; + * internal `{text, tag?}` records are mapped to `.text` so callers + * (`updatePendingMessagesDisplay`, `restoreQueuedMessagesToEditor`) see + * the unchanged historical shape. */ getQueuedMessages(): { steering: readonly string[]; followUp: readonly string[] } { - return { steering: this.#steeringMessages, followUp: this.#followUpMessages }; + return { + steering: this.#steeringMessages.map(e => e.text), + followUp: this.#followUpMessages.map(e => e.text), + }; } /** * Pop the last queued message (steering first, then follow-up). * Used by dequeue keybinding to restore messages to editor one at a time. + * Returns the popped entry's `.text`; the tag (if any) dies with the + * record — no orphan state can outlive the queue entry. */ popLastQueuedMessage(): string | undefined { // Pop from steering first (LIFO) if (this.#steeringMessages.length > 0) { - const message = this.#steeringMessages.pop(); + const entry = this.#steeringMessages.pop(); this.agent.popLastSteer(); - return message; + return entry?.text; } // Then from follow-up if (this.#followUpMessages.length > 0) { - const message = this.#followUpMessages.pop(); + const entry = this.#followUpMessages.pop(); this.agent.popLastFollowUp(); - return message; + return entry?.text; } return undefined; } diff --git a/packages/coding-agent/src/session/messages.ts b/packages/coding-agent/src/session/messages.ts index 061ef5d42..3c67c2e06 100644 --- a/packages/coding-agent/src/session/messages.ts +++ b/packages/coding-agent/src/session/messages.ts @@ -30,6 +30,51 @@ export interface SkillPromptDetails { path: string; args?: string; lineCount: number; + /** Internal: tag used by AgentSession to remove the pending-display chip + * from `#steeringMessages` / `#followUpMessages` when the agent consumes + * this message. Not surfaced to renderers; the `__` prefix signals + * "private". Optional — non-streaming skill prompts never set it. Stripped + * from persisted `details` by `SessionManager.appendCustomMessageEntry` + * via the `INTERNAL_DETAILS_FIELDS` allowlist below. */ + __pendingDisplayTag?: string; +} + +/** Extract the optional `__pendingDisplayTag` field from a CustomMessage's + * `details` blob. Safe over `unknown`; returns undefined when the field is + * absent or non-string. */ +export function readPendingDisplayTag(details: unknown): string | undefined { + if (typeof details !== "object" || details === null) return undefined; + const candidate = (details as { __pendingDisplayTag?: unknown }).__pendingDisplayTag; + return typeof candidate === "string" ? candidate : undefined; +} + +/** Explicit allowlist of `details` field names that are AgentSession-internal + * transient bookkeeping and MUST be removed before SessionManager persists + * the CustomMessageEntry to disk. Scoped intentionally narrow: only fields + * declared here are stripped. Adding a new entry is a deliberate, reviewed + * change — unrelated future payload fields are never silently dropped. */ +export const INTERNAL_DETAILS_FIELDS = ["__pendingDisplayTag"] as const; + +/** Return a `details` copy with every key in `INTERNAL_DETAILS_FIELDS` + * removed. Returns the input unchanged when there is nothing to strip + * (null/non-object, or no listed fields present) so callers don't pay a + * clone cost on the common path. */ +export function stripInternalDetailsFields(details: T | undefined): T | undefined { + if (details == null || typeof details !== "object") return details; + const obj = details as Record; + let hit = false; + for (const key of INTERNAL_DETAILS_FIELDS) { + if (key in obj) { + hit = true; + break; + } + } + if (!hit) return details; + const cleaned: Record = { ...obj }; + for (const key of INTERNAL_DETAILS_FIELDS) { + delete cleaned[key]; + } + return cleaned as T; } function getPrunedToolResultContent(message: ToolResultMessage): (TextContent | ImageContent)[] { diff --git a/packages/coding-agent/src/session/session-manager.ts b/packages/coding-agent/src/session/session-manager.ts index 56555016c..f19e2c474 100644 --- a/packages/coding-agent/src/session/session-manager.ts +++ b/packages/coding-agent/src/session/session-manager.ts @@ -47,6 +47,7 @@ import { type HookMessage, type PythonExecutionMessage, sanitizeRehydratedOpenAIResponsesAssistantMessage, + stripInternalDetailsFields, } from "./messages"; import type { SessionStorage, SessionStorageWriter } from "./session-storage"; import { FileSessionStorage, MemorySessionStorage } from "./session-storage"; @@ -2544,7 +2545,10 @@ export class SessionManager { customType, content, display, - details, + // Drop AgentSession-internal transient fields (allowlist in + // `INTERNAL_DETAILS_FIELDS`) before disk persistence. Single + // chokepoint covers every CustomMessage write path. + details: stripInternalDetailsFields(details), attribution, id: generateId(this.#byId), parentId: this.#leafId, diff --git a/packages/coding-agent/test/input-controller-skill-queue.test.ts b/packages/coding-agent/test/input-controller-skill-queue.test.ts new file mode 100644 index 000000000..9334ac311 --- /dev/null +++ b/packages/coding-agent/test/input-controller-skill-queue.test.ts @@ -0,0 +1,431 @@ +/** + * Phase 6 — E layer. + * + * Tests the skill-queue + custom-role dequeue contract that ties together: + * - InputController.#invokeSkillCommand (tag generation when streaming); + * - AgentSession.enqueueCustomMessageDisplay + #handleAgentEvent's + * custom-role `message_start` dequeue; + * - UiHelpers.updatePendingMessagesDisplay (compact slash-form rendering); + * - InputController.restoreQueuedMessagesToEditor (recovery of the slash-form + * into the editor). + * + * Tests split into: + * - E1-E3: InputController-side tag generation, stubbed session; + * - E4-E7: Real AgentSession driving synthetic `message_start` events + * for the tag-based custom-role dequeue; + * - E8: real UiHelpers render against a queued-display entry; + * - E9: real InputController.restoreQueuedMessagesToEditor. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import * as fs from "node:fs"; +import * as path from "node:path"; +import { Agent } from "@oh-my-pi/pi-agent-core"; +import { getBundledModel } from "@oh-my-pi/pi-ai/models"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { InputController } from "@oh-my-pi/pi-coding-agent/modes/controllers/input-controller"; +import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types"; +import { UiHelpers } from "@oh-my-pi/pi-coding-agent/modes/utils/ui-helpers"; +import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { + SKILL_PROMPT_MESSAGE_TYPE, + type SkillPromptDetails, +} from "@oh-my-pi/pi-coding-agent/session/messages"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { getThemeByName, setThemeInstance } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import { Container } from "@oh-my-pi/pi-tui"; +import { TempDir } from "@oh-my-pi/pi-utils"; + +// ============================================================================ +// Shared helpers +// ============================================================================ + +function writeSkillFile(dir: string, skillName: string, body: string): string { + const skillPath = path.join(dir, `${skillName}.md`); + fs.writeFileSync(skillPath, `---\nname: ${skillName}\n---\n${body}\n`); + return skillPath; +} + +// ============================================================================ +// E1-E3: InputController tag generation with a stubbed session. +// ============================================================================ + +type StubEditor = { + setText: (text: string) => void; + getText: () => string; + addToHistory: ReturnType; + onSubmit?: (text: string) => Promise; +}; + +function createStubInputControllerContext(opts: { + skillCommands: Map; + isStreaming: boolean; +}) { + let editorText = ""; + const editor: StubEditor = { + setText(text) { + editorText = text; + }, + getText() { + return editorText; + }, + addToHistory: vi.fn(), + }; + const enqueueCustomMessageDisplay = vi.fn((_text: string, _mode: "steer" | "followUp") => "sk-test-0"); + // Annotate parameters so `mock.calls[N]` is typed as a tuple (not `[]`) — + // avoids TS2352/TS2493 when casting `.calls[0]` to a destructured shape. + const promptCustomMessage = vi.fn( + async (_message: { details?: SkillPromptDetails }, _options?: unknown) => {}, + ); + const updatePendingMessagesDisplay = vi.fn(); + const requestRender = vi.fn(); + const showError = vi.fn(); + + const ctx = { + editor, + ui: { requestRender }, + skillCommands: opts.skillCommands, + session: { + isStreaming: opts.isStreaming, + isCompacting: false, + isBashRunning: false, + isEvalRunning: false, + extensionRunner: undefined, + enqueueCustomMessageDisplay, + promptCustomMessage, + }, + showError, + updatePendingMessagesDisplay, + // Defaults that InputController touches on submit but don't matter here. + isBashMode: false, + isPythonMode: false, + pendingImages: [], + isBackgrounded: false, + loopModeEnabled: false, + compactionQueuedMessages: [], + locallySubmittedUserSignatures: new Set(), + withLocalSubmission: async (_text: string, fn: () => unknown) => fn(), + } as unknown as InteractiveModeContext; + + return { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage }; +} + +describe("InputController #invokeSkillCommand (E1-E3)", () => { + let tempDir: TempDir; + let skillCommands: Map; + + beforeEach(() => { + tempDir = TempDir.createSync("@pi-skill-queue-stub-"); + const skillPath = writeSkillFile(tempDir.path(), "test-skill", "Do the thing."); + skillCommands = new Map([["skill:test-skill", skillPath]]); + }); + + afterEach(() => { + tempDir.removeSync(); + vi.restoreAllMocks(); + }); + + it("E1: streaming + steer -> enqueueCustomMessageDisplay called and details.__pendingDisplayTag set", async () => { + const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = + createStubInputControllerContext({ skillCommands, isStreaming: true }); + + const controller = new InputController(ctx); + controller.setupEditorSubmitHandler(); + editor.setText("/skill:test-skill arg1 arg2"); + await editor.onSubmit?.("/skill:test-skill arg1 arg2"); + + expect(enqueueCustomMessageDisplay).toHaveBeenCalledTimes(1); + expect(enqueueCustomMessageDisplay).toHaveBeenCalledWith("/skill:test-skill arg1 arg2", "steer"); + + expect(promptCustomMessage).toHaveBeenCalledTimes(1); + const firstCall = promptCustomMessage.mock.calls[0]; + expect(firstCall).toBeDefined(); + // biome-ignore lint/style/noNonNullAssertion: just asserted non-undefined + const messageArg = firstCall![0] as { details: SkillPromptDetails }; + expect(messageArg.details.__pendingDisplayTag).toBe("sk-test-0"); + }); + + it("E2: streaming + followUp -> enqueueCustomMessageDisplay called with mode 'followUp', tag embedded", async () => { + const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = + createStubInputControllerContext({ skillCommands, isStreaming: true }); + + const controller = new InputController(ctx); + editor.setText("/skill:test-skill arg1 arg2"); + // `handleFollowUp` is the Ctrl+Enter dispatcher; it routes through the same + // `#invokeSkillCommand` helper with mode "followUp". + await controller.handleFollowUp(); + + expect(enqueueCustomMessageDisplay).toHaveBeenCalledWith("/skill:test-skill arg1 arg2", "followUp"); + + const firstCall = promptCustomMessage.mock.calls[0]; + expect(firstCall).toBeDefined(); + // biome-ignore lint/style/noNonNullAssertion: just asserted non-undefined + const messageArg = firstCall![0] as { details: SkillPromptDetails }; + expect(messageArg.details.__pendingDisplayTag).toBe("sk-test-0"); + }); + + it("E3: not streaming -> enqueueCustomMessageDisplay NOT called and tag absent", async () => { + const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = + createStubInputControllerContext({ skillCommands, isStreaming: false }); + + const controller = new InputController(ctx); + controller.setupEditorSubmitHandler(); + editor.setText("/skill:test-skill arg1 arg2"); + await editor.onSubmit?.("/skill:test-skill arg1 arg2"); + + expect(enqueueCustomMessageDisplay).not.toHaveBeenCalled(); + const firstCall = promptCustomMessage.mock.calls[0]; + expect(firstCall).toBeDefined(); + // biome-ignore lint/style/noNonNullAssertion: just asserted non-undefined + const messageArg = firstCall![0] as { details: SkillPromptDetails }; + expect(messageArg.details.__pendingDisplayTag).toBeUndefined(); + }); +}); + +// ============================================================================ +// E4-E7: Real AgentSession driving synthetic `message_start` events. +// ============================================================================ + +interface SessionFixture { + tempDir: TempDir; + authStorage: AuthStorage; + session: AgentSession; +} + +async function createRealSession(): Promise { + const tempDir = TempDir.createSync("@pi-skill-queue-real-"); + const authStorage = await AuthStorage.create(path.join(tempDir.path(), "testauth.db")); + authStorage.setRuntimeApiKey("anthropic", "test-key"); + const modelRegistry = new ModelRegistry(authStorage); + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) throw new Error("Expected built-in anthropic model to exist"); + + const agent = new Agent({ + initialState: { + model, + systemPrompt: ["Test"], + tools: [], + messages: [], + }, + }); + + const session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings: Settings.isolated(), + modelRegistry, + }); + + return { tempDir, authStorage, session }; +} + +/** Emit a `message_start` for a custom message whose `details` carries the supplied tag. */ +function emitCustomMessageStart(session: AgentSession, content: string, tag?: string): void { + const details: { __pendingDisplayTag?: string } | undefined = + tag === undefined ? undefined : { __pendingDisplayTag: tag }; + session.agent.emitExternalEvent({ + type: "message_start", + message: { + role: "custom", + customType: SKILL_PROMPT_MESSAGE_TYPE, + content, + display: true, + details, + timestamp: Date.now(), + }, + }); +} + +describe("AgentSession custom-role tag dequeue (E4-E7)", () => { + let fixture: SessionFixture | undefined; + + afterEach(async () => { + if (fixture) { + await fixture.session.dispose(); + fixture.authStorage.close(); + fixture.tempDir.removeSync(); + fixture = undefined; + } + vi.restoreAllMocks(); + }); + + it("E4: message_start with role=custom + matching tag removes the tagged display entry", async () => { + fixture = await createRealSession(); + const { session } = fixture; + const tag = session.enqueueCustomMessageDisplay("/skill:foo bar", "steer"); + expect(tag).not.toBe(""); + expect(session.getQueuedMessages().steering).toEqual(["/skill:foo bar"]); + + emitCustomMessageStart(session, "irrelevant content", tag); + await Promise.resolve(); + await Promise.resolve(); + + expect(session.getQueuedMessages().steering).toEqual([]); + // And internal queue counters reflect the empty steer/followUp arrays. The + // pending-next-turn store stays at zero too because this test never queued one. + expect(session.queuedMessageCount).toBe(0); + }); + + it("E5: message_start with role=custom but no tag is a no-op", async () => { + fixture = await createRealSession(); + const { session } = fixture; + session.enqueueCustomMessageDisplay("/skill:foo bar", "steer"); + const beforeCount = session.queuedMessageCount; + expect(beforeCount).toBe(1); + + emitCustomMessageStart(session, "irrelevant content"); // no tag + await Promise.resolve(); + await Promise.resolve(); + + expect(session.getQueuedMessages().steering).toEqual(["/skill:foo bar"]); + expect(session.queuedMessageCount).toBe(beforeCount); + }); + + it("E6: two queued skills with identical args text are dequeued independently by tag", async () => { + fixture = await createRealSession(); + const { session } = fixture; + const tag1 = session.enqueueCustomMessageDisplay("/skill:foo bar", "steer"); + const tag2 = session.enqueueCustomMessageDisplay("/skill:foo bar", "steer"); + expect(tag1).not.toBe(tag2); + expect(session.getQueuedMessages().steering).toEqual(["/skill:foo bar", "/skill:foo bar"]); + + // Consume the SECOND-enqueued tag. After dequeue, the SURVIVING entry must be + // the one that was added FIRST — proves the dequeue keys off `tag`, not off + // `indexOf(text)` (which would always have removed the first match). + emitCustomMessageStart(session, "any", tag2); + await Promise.resolve(); + await Promise.resolve(); + + expect(session.getQueuedMessages().steering).toEqual(["/skill:foo bar"]); + + // Now dequeue the first; nothing left. + emitCustomMessageStart(session, "any", tag1); + await Promise.resolve(); + await Promise.resolve(); + + expect(session.getQueuedMessages().steering).toEqual([]); + }); + + it("E7: popLastQueuedMessage on a tagged entry leaves no orphan tag state", async () => { + fixture = await createRealSession(); + const { session } = fixture; + const firstTag = session.enqueueCustomMessageDisplay("/skill:foo bar", "steer"); + const popped = session.popLastQueuedMessage(); + expect(popped).toBe("/skill:foo bar"); + expect(session.getQueuedMessages().steering).toEqual([]); + + // Push a NEW tagged entry with the same text. Emitting `message_start` for the + // FIRST (popped) tag must be a no-op — the dequeue cannot reach into the new + // entry because the popped tag died with its record. + const secondTag = session.enqueueCustomMessageDisplay("/skill:foo bar", "steer"); + expect(secondTag).not.toBe(firstTag); + + emitCustomMessageStart(session, "any", firstTag); + await Promise.resolve(); + await Promise.resolve(); + expect(session.getQueuedMessages().steering).toEqual(["/skill:foo bar"]); + + // Sanity: the second tag still works. + emitCustomMessageStart(session, "any", secondTag); + await Promise.resolve(); + await Promise.resolve(); + expect(session.getQueuedMessages().steering).toEqual([]); + }); +}); + +// ============================================================================ +// E8-E9: Real UiHelpers / InputController against a session populated through +// enqueueCustomMessageDisplay. +// ============================================================================ + +function createStubInteractiveModeContextForUiHelpers(session: AgentSession) { + let editorText = ""; + const editor: StubEditor = { + setText(text) { + editorText = text; + }, + getText() { + return editorText; + }, + addToHistory: vi.fn(), + }; + const pendingMessagesContainer = new Container(); + const requestRender = vi.fn(); + const updatePendingMessagesDisplay = vi.fn(); + + const ctx = { + editor, + ui: { requestRender }, + pendingMessagesContainer, + session, + compactionQueuedMessages: [], + keybindings: { + getDisplayString: (_action: string) => "Alt+Up", + }, + updatePendingMessagesDisplay, + locallySubmittedUserSignatures: new Set(), + } as unknown as InteractiveModeContext; + + return { ctx, editor, pendingMessagesContainer }; +} + +describe("UiHelpers / InputController against the queued-display layer (E8-E9)", () => { + let fixture: SessionFixture | undefined; + + beforeEach(async () => { + // E8 invokes the real `theme.fg(...)` codepath inside + // updatePendingMessagesDisplay; without an initialized theme module the + // global `theme` variable is undefined. Installs `dark` per-test — + // matches the established suite convention used by other test files + // (bash-execution-clamp.test.ts, bash-execution-sixel.test.ts) where + // `dark` is the agreed default for every test that needs a theme. + // No `afterEach` restore is required by that convention; the theme + // module exposes no reset API, and `dark` is the suite-wide assumed + // post-state. + const themeInstance = await getThemeByName("dark"); + expect(themeInstance).toBeDefined(); + setThemeInstance(themeInstance!); + }); + + afterEach(async () => { + if (fixture) { + await fixture.session.dispose(); + fixture.authStorage.close(); + fixture.tempDir.removeSync(); + fixture = undefined; + } + vi.restoreAllMocks(); + }); + + it("E8: updatePendingMessagesDisplay renders the compact slash form for queued skills", async () => { + fixture = await createRealSession(); + const { session } = fixture; + session.enqueueCustomMessageDisplay("/skill:test-skill arg1 arg2", "steer"); + + const { ctx, pendingMessagesContainer } = createStubInteractiveModeContextForUiHelpers(session); + const uiHelpers = new UiHelpers(ctx); + uiHelpers.updatePendingMessagesDisplay(); + + // Render the container at a generous width and assert the compact slash-form + // chip appears verbatim. Matches the user-facing "Steer: /skill:..." format. + const rendered = pendingMessagesContainer.render(120).join("\n"); + expect(rendered).toMatch(/Steer: \/skill:test-skill arg1 arg2/); + }); + + it("E9: restoreQueuedMessagesToEditor recovers the compact slash form into the editor and clears the queue", async () => { + fixture = await createRealSession(); + const { session } = fixture; + session.enqueueCustomMessageDisplay("/skill:test-skill arg1 arg2", "steer"); + + const { ctx, editor } = createStubInteractiveModeContextForUiHelpers(session); + const controller = new InputController(ctx); + const count = controller.restoreQueuedMessagesToEditor(); + expect(count).toBe(1); + expect(editor.getText()).toBe("/skill:test-skill arg1 arg2"); + // Queue cleared on both arrays. + const { steering, followUp } = session.getQueuedMessages(); + expect(steering).toEqual([]); + expect(followUp).toEqual([]); + }); +}); diff --git a/packages/coding-agent/test/session-manager-internal-details.test.ts b/packages/coding-agent/test/session-manager-internal-details.test.ts new file mode 100644 index 000000000..28323c6a6 --- /dev/null +++ b/packages/coding-agent/test/session-manager-internal-details.test.ts @@ -0,0 +1,137 @@ +/** + * Phase 6 — F layer. + * + * Direct unit tests on: + * - `SessionManager.appendCustomMessageEntry` — the single chokepoint that + * routes `details` through `stripInternalDetailsFields` before persistence; + * - `stripInternalDetailsFields` itself — the helper that enforces the + * `INTERNAL_DETAILS_FIELDS` allowlist. + * + * The contract under test is the explicit-allowlist regression guard: only the + * fields named in `INTERNAL_DETAILS_FIELDS` are removed; anything else (even + * `__`-prefixed fields not in the allowlist) is preserved verbatim. + */ +import { describe, expect, it } from "bun:test"; +import { + type SkillPromptDetails, + stripInternalDetailsFields, +} from "@oh-my-pi/pi-coding-agent/session/messages"; +import { type CustomMessageEntry, SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; + +const SKILL_TYPE = "skill-prompt"; + +function readPersistedCustomMessageEntry( + session: SessionManager, + id: string, +): CustomMessageEntry { + const branch = session.getBranch(); + const entry = branch.find(e => e.id === id); + if (!entry || entry.type !== "custom_message") { + throw new Error(`Expected custom_message entry with id ${id}, got ${entry?.type ?? "none"}`); + } + return entry as CustomMessageEntry; +} + +describe("SessionManager.appendCustomMessageEntry (allowlist strip + persistence contract)", () => { + it("F1: strips __pendingDisplayTag from persisted details while preserving all other SkillPromptDetails fields", () => { + const session = SessionManager.inMemory(); + const id = session.appendCustomMessageEntry( + SKILL_TYPE, + "skill body", + true, + { + name: "foo", + path: "/s.md", + args: "bar", + lineCount: 10, + __pendingDisplayTag: "omp-cmd-1-0", + }, + "user", + ); + + const entry = readPersistedCustomMessageEntry(session, id); + expect(entry.details).toEqual({ + name: "foo", + path: "/s.md", + args: "bar", + lineCount: 10, + }); + // Explicit absence assertion — defends against `toEqual` semantics drift + // where an `undefined`-valued key would still satisfy deep equality. + expect(Object.hasOwn(entry.details!, "__pendingDisplayTag")).toBe(false); + }); + + it("F2: persists details deep-equal to the input when no allowlisted field is present", () => { + const session = SessionManager.inMemory(); + const input: SkillPromptDetails = { + name: "foo", + path: "/s.md", + args: "bar", + lineCount: 10, + }; + const id = session.appendCustomMessageEntry(SKILL_TYPE, "skill body", true, input, "user"); + const entry = readPersistedCustomMessageEntry(session, id); + // Deep equality on shape only — the contract intentionally does NOT couple + // to whether the helper clones or short-circuits internally. Future + // refactors (defensive cloning, JSON round-trip) cannot break this test. + expect(entry.details).toEqual(input); + }); + + it("F3: does NOT strip __-prefixed fields that are not in INTERNAL_DETAILS_FIELDS (explicit-allowlist guard)", () => { + // Regression guard against an over-broad strip — only allowlisted keys go. + // Future internal fields that haven't been added to the allowlist must be + // preserved verbatim until that change ships intentionally. + const session = SessionManager.inMemory(); + const id = session.appendCustomMessageEntry>( + SKILL_TYPE, + "skill body", + true, + { + name: "foo", + path: "/s.md", + args: "bar", + lineCount: 10, + __future_field: "preserve-me", + }, + "user", + ); + const entry = readPersistedCustomMessageEntry>(session, id); + expect(entry.details).toEqual({ + name: "foo", + path: "/s.md", + args: "bar", + lineCount: 10, + __future_field: "preserve-me", + }); + }); + + it("F4: stripInternalDetailsFields treats undefined / null / non-object details as identity", () => { + expect(stripInternalDetailsFields(undefined)).toBeUndefined(); + // `null as never` here only because the public signature is `T | undefined`, + // but the runtime contract has to tolerate `null` defensively. + expect(stripInternalDetailsFields(null as unknown as undefined)).toBeNull(); + expect(stripInternalDetailsFields("string" as unknown as undefined)).toBe( + "string" as unknown as undefined, + ); + }); + + it("F5: stripInternalDetailsFields preserves the input shape verbatim when no allowlisted field is present", () => { + // Shape-preservation contract: the helper returns a value deep-equal to + // the input when no allowlisted key is present. The plan's original + // `Object.is` identity claim was deliberately weakened here to a + // shape-preservation assertion so a future defensive-clone refactor + // (e.g. structured-clone-on-read) cannot break this test without a real + // behavioral regression. Identity / allocation strategy is an internal + // implementation detail of the helper, not a public contract. + const input = { name: "foo", lineCount: 1 }; + const result = stripInternalDetailsFields(input); + expect(result).toEqual(input); + // Every input key survives — no allowlisted field touched, so no key + // dropped. Iterating the input's keys defends against a regression that + // silently drops one even when the shape happens to match deep-equality + // (e.g. via an extra `undefined` member). + for (const key of Object.keys(input)) { + expect(Object.hasOwn(result as object, key)).toBe(true); + } + }); +});