diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2e11f5229..81d141c95 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,7 +1,6 @@ # Changelog ## [Unreleased] - ### Added - Added support for GitLab Duo authentication provider @@ -14,6 +13,9 @@ ### Changed +- Moved artifact management from `ToolSession.getArtifactManager()` to `SessionManager`, providing centralized artifact lifecycle management +- Replaced `getArtifactManager()` method with `allocateOutputArtifact()` in `ToolSession` for simpler artifact allocation without exposing manager internals +- Simplified artifact allocation in bash, fetch, python, and ssh tools by calling `session.allocateOutputArtifact()` directly instead of importing helper function - Changed default value of `todo.reminders` setting from false to true to enable todo reminders by default - Updated system prompt discipline guidelines with new guidance on code cleanup, root-cause fixes, and avoiding breadcrumb comments - Enhanced work procedure with architecture-first approach, idiomatic code standards, and ruthless dead code cleanup @@ -45,6 +47,7 @@ ### Removed +- Removed `allocateOutputArtifact()` helper function from `streaming-output` module (functionality moved to `SessionManager`) - Removed git context (branch, status, commit history) from system prompt — version control information is no longer injected into agent instructions - Removed `ToolUIKit` class from render-utils (replaced with standalone formatting functions) - Removed `normalizeUnicodeSpaces` and `expandPath` from discovery/helpers (moved to path-utils) @@ -59,6 +62,7 @@ ### Fixed +- Fixed Lobsters scraper to correctly parse API responses where user fields are strings instead of objects, resolving undefined user display in story listings - Fixed artifact manager caching to properly invalidate when session file changes, preventing stale artifact references - Fixed truncation notice formatting consistency across all tool renderers by centralizing logic - Fixed UTF-8 boundary handling in byte truncation to prevent invalid character sequences diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 6a6037a96..764223aba 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -13,7 +13,6 @@ import { loadPromptTemplates as loadPromptTemplatesInternal, type PromptTemplate import { Settings, type SkillsSettings } from "./config/settings"; import { CursorExecHandlers } from "./cursor"; import "./discovery"; -import { ArtifactManager } from "@oh-my-pi/pi-coding-agent/session/artifacts"; import { resolveConfigValue } from "./config/resolve-config-value"; import { initializeWithSettings } from "./discovery"; import { TtsrManager } from "./export/ttsr"; @@ -717,8 +716,6 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} const enableLsp = options.enableLsp ?? true; - let artifactManager: ArtifactManager | null = null; - let artifactManagerSessionFile: string | null = null; const toolSession: ToolSession = { cwd, hasUI: options.hasUI ?? false, @@ -746,20 +743,12 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} }, getPlanModeState: () => session.getPlanModeState(), getCompactContext: () => session.formatCompactContext(), - getArtifactManager: () => { - const sessionFile = sessionManager.getSessionFile(); - if (!sessionFile) { - artifactManager = null; - artifactManagerSessionFile = null; - return null; + allocateOutputArtifact: async toolType => { + try { + return await sessionManager.allocateArtifactPath(toolType); + } catch { + return {}; } - if (artifactManager && artifactManagerSessionFile === sessionFile) { - return artifactManager; - } - const manager = new ArtifactManager(sessionFile); - artifactManager = manager; - artifactManagerSessionFile = sessionFile; - return manager; }, settings, authStorage, @@ -768,10 +757,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} // Initialize internal URL router for internal protocols (agent://, artifact://, plan://, memory://, skill://, rule://) const internalRouter = new InternalUrlRouter(); - const getArtifactsDir = () => { - const sessionFile = sessionManager.getSessionFile(); - return sessionFile ? sessionFile.slice(0, -6) : null; // strip .jsonl - }; + const getArtifactsDir = () => sessionManager.getArtifactsDir(); internalRouter.register(new AgentProtocolHandler({ getArtifactsDir })); internalRouter.register(new ArtifactProtocolHandler({ getArtifactsDir })); internalRouter.register( diff --git a/packages/coding-agent/src/session/session-manager.ts b/packages/coding-agent/src/session/session-manager.ts index e32f4df9b..d3b880204 100644 --- a/packages/coding-agent/src/session/session-manager.ts +++ b/packages/coding-agent/src/session/session-manager.ts @@ -6,6 +6,7 @@ import type { ImageContent, Message, TextContent, Usage } from "@oh-my-pi/pi-ai" import { getTerminalId } from "@oh-my-pi/pi-tui"; import { isEnoent, logger, parseJsonlLenient, Snowflake, toError } from "@oh-my-pi/pi-utils"; import { getBlobsDir, getAgentDir as getDefaultAgentDir, getProjectDir } from "@oh-my-pi/pi-utils/dirs"; +import { ArtifactManager } from "./artifacts"; import { type BlobPutResult, BlobStore, externalizeImageData, isBlobRef, resolveImageData } from "./blob-store"; import { type BashExecutionMessage, @@ -216,6 +217,10 @@ export type ReadonlySessionManager = Pick< | "getSessionDir" | "getSessionId" | "getSessionFile" + | "getArtifactsDir" + | "allocateArtifactPath" + | "saveArtifact" + | "getArtifactPath" | "getLeafId" | "getLeafEntry" | "getEntry" @@ -1129,6 +1134,8 @@ export class SessionManager { #persistChain: Promise = Promise.resolve(); #persistError: Error | undefined; #persistErrorReported = false; + #artifactManager: ArtifactManager | null = null; + #artifactManagerSessionFile: string | null = null; readonly #blobStore: BlobStore; private constructor( @@ -1530,6 +1537,67 @@ export class SessionManager { return this.#sessionFile; } + /** + * Returns the session artifacts directory path (session file path without .jsonl). + * Returns null when the session is not persisted to a file. + */ + getArtifactsDir(): string | null { + const sessionFile = this.#sessionFile; + return sessionFile ? sessionFile.slice(0, -6) : null; + } + + /** + * Returns an artifact manager bound to the current session file. + * Recreates the manager when the active session file changes. + */ + #getOrCreateArtifactManager(): ArtifactManager | null { + const sessionFile = this.#sessionFile; + if (!sessionFile) { + this.#artifactManager = null; + this.#artifactManagerSessionFile = null; + return null; + } + + if (this.#artifactManager && this.#artifactManagerSessionFile === sessionFile) { + return this.#artifactManager; + } + + const manager = new ArtifactManager(sessionFile); + this.#artifactManager = manager; + this.#artifactManagerSessionFile = sessionFile; + return manager; + } + + /** + * Allocate a new artifact path and ID for the current session. + * Returns an empty object when the session is not persisted. + */ + async allocateArtifactPath(toolType: string): Promise<{ id?: string; path?: string }> { + const manager = this.#getOrCreateArtifactManager(); + if (!manager) return {}; + return manager.allocatePath(toolType); + } + + /** + * Save artifact content under the current session and return artifact ID. + * Returns undefined when the session is not persisted. + */ + async saveArtifact(content: string, toolType: string): Promise { + const manager = this.#getOrCreateArtifactManager(); + if (!manager) return undefined; + return manager.save(content, toolType); + } + + /** + * Resolve an artifact ID to an on-disk path for the current session. + * Returns null when missing or when the session is not persisted. + */ + async getArtifactPath(id: string): Promise { + const manager = this.#getOrCreateArtifactManager(); + if (!manager) return null; + return manager.getPath(id); + } + getSessionName(): string | undefined { return this.#sessionName; } diff --git a/packages/coding-agent/src/session/streaming-output.ts b/packages/coding-agent/src/session/streaming-output.ts index d12fb284d..a79ac624b 100644 --- a/packages/coding-agent/src/session/streaming-output.ts +++ b/packages/coding-agent/src/session/streaming-output.ts @@ -1,5 +1,4 @@ import { sanitizeText } from "@oh-my-pi/pi-natives"; -import type { ToolSession } from "../tools"; import { formatBytes } from "../tools/render-utils"; // ============================================================================= @@ -678,24 +677,6 @@ export class OutputSink { } } -// ============================================================================= -// Session helpers -// ============================================================================= - -const kEmpty = Object.freeze({} as { id?: string; path?: string }); - -/** Allocate a new artifact path and ID without writing content. */ -export async function allocateOutputArtifact(session: ToolSession, toolType: string) { - const manager = session.getArtifactManager?.(); - if (!manager) return kEmpty; - - try { - return await manager.allocatePath(toolType); - } catch { - return kEmpty; - } -} - // ============================================================================= // Truncation notice formatting // ============================================================================= diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 738d0926c..b408a8bbc 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -12,7 +12,7 @@ import type { RenderResultOptions } from "../extensibility/custom-tools/types"; import { truncateToVisualLines } from "../modes/components/visual-truncate"; import type { Theme } from "../modes/theme/theme"; import bashDescription from "../prompts/tools/bash.md" with { type: "text" }; -import { allocateOutputArtifact, DEFAULT_MAX_BYTES, TailBuffer } from "../session/streaming-output"; +import { DEFAULT_MAX_BYTES, TailBuffer } from "../session/streaming-output"; import { renderStatusLine } from "../tui"; import { CachedOutputBlock } from "../tui/output-block"; import type { ToolSession } from "."; @@ -118,7 +118,7 @@ export class BashTool implements AgentTool { // Set up artifacts environment and allocation const artifactsDir = this.session.getArtifactsDir?.(); const extraEnv = artifactsDir ? { ARTIFACTS: artifactsDir } : undefined; - const { path: artifactPath, id: artifactId } = await allocateOutputArtifact(this.session, "bash"); + const { path: artifactPath, id: artifactId } = (await this.session.allocateOutputArtifact?.("bash")) ?? {}; const usePty = this.session.settings.get("bash.virtualTerminal") === "on" && diff --git a/packages/coding-agent/src/tools/fetch.ts b/packages/coding-agent/src/tools/fetch.ts index 2c9955c27..1b10eca3d 100644 --- a/packages/coding-agent/src/tools/fetch.ts +++ b/packages/coding-agent/src/tools/fetch.ts @@ -10,7 +10,7 @@ import { renderPromptTemplate } from "../config/prompt-templates"; import type { RenderResultOptions } from "../extensibility/custom-tools/types"; import { type Theme, theme } from "../modes/theme/theme"; import fetchDescription from "../prompts/tools/fetch.md" with { type: "text" }; -import { allocateOutputArtifact, DEFAULT_MAX_BYTES, truncateHead } from "../session/streaming-output"; +import { DEFAULT_MAX_BYTES, truncateHead } from "../session/streaming-output"; import { renderStatusLine } from "../tui"; import { CachedOutputBlock } from "../tui/output-block"; import { ensureTool } from "../utils/tools-manager"; @@ -899,7 +899,7 @@ export class FetchTool implements AgentTool string | null; /** Get session ID */ getSessionId?: () => string | null; - /** Get artifact manager (allocated per ToolSession) */ - getArtifactManager?: () => ArtifactManager | null; /** Get artifacts directory for artifact:// URLs and $ARTIFACTS env var */ getArtifactsDir?: () => string | null; + /** Allocate a new artifact path and ID for session-scoped truncated output. */ + allocateOutputArtifact?: (toolType: string) => Promise<{ id?: string; path?: string }>; /** Get session spawns */ getSessionSpawns: () => string | null; /** Get resolved model string if explicitly set for this session */ diff --git a/packages/coding-agent/src/tools/python.ts b/packages/coding-agent/src/tools/python.ts index d5a2ad7e1..41d5700e9 100644 --- a/packages/coding-agent/src/tools/python.ts +++ b/packages/coding-agent/src/tools/python.ts @@ -13,13 +13,7 @@ import type { PreludeHelper, PythonStatusEvent } from "../ipy/kernel"; import { truncateToVisualLines } from "../modes/components/visual-truncate"; import type { Theme } from "../modes/theme/theme"; import pythonDescription from "../prompts/tools/python.md" with { type: "text" }; -import { - allocateOutputArtifact, - DEFAULT_MAX_BYTES, - OutputSink, - type OutputSummary, - TailBuffer, -} from "../session/streaming-output"; +import { DEFAULT_MAX_BYTES, OutputSink, type OutputSummary, TailBuffer } from "../session/streaming-output"; import { getTreeBranch, getTreeContinuePrefix, renderCodeCell } from "../tui"; import type { ToolSession } from "."; import { formatStyledTruncationWarning, type OutputMeta } from "./output-meta"; @@ -259,7 +253,7 @@ export class PythonTool implements AgentTool { const sessionFile = this.session.getSessionFile?.() ?? undefined; const artifactsDir = this.session.getArtifactsDir?.() ?? undefined; - const { path: artifactPath, id: artifactId } = await allocateOutputArtifact(this.session, "python"); + const { path: artifactPath, id: artifactId } = (await this.session.allocateOutputArtifact?.("python")) ?? {}; outputSink = new OutputSink({ artifactPath, artifactId, diff --git a/packages/coding-agent/src/tools/ssh.ts b/packages/coding-agent/src/tools/ssh.ts index 3545e96d5..34605bd7d 100644 --- a/packages/coding-agent/src/tools/ssh.ts +++ b/packages/coding-agent/src/tools/ssh.ts @@ -9,7 +9,7 @@ import { loadCapability } from "../discovery"; import type { RenderResultOptions } from "../extensibility/custom-tools/types"; import type { Theme } from "../modes/theme/theme"; import sshDescriptionBase from "../prompts/tools/ssh.md" with { type: "text" }; -import { allocateOutputArtifact, DEFAULT_MAX_BYTES, TailBuffer } from "../session/streaming-output"; +import { DEFAULT_MAX_BYTES, TailBuffer } from "../session/streaming-output"; import type { SSHHostInfo } from "../ssh/connection-manager"; import { ensureHostInfo, getHostInfoForHost } from "../ssh/connection-manager"; import { executeSSH } from "../ssh/ssh-executor"; @@ -158,7 +158,7 @@ export class SshTool implements AgentTool { const timeoutMs = timeoutSec * 1000; const tailBuffer = new TailBuffer(DEFAULT_MAX_BYTES); - const { path: artifactPath, id: artifactId } = await allocateOutputArtifact(this.session, "ssh"); + const { path: artifactPath, id: artifactId } = (await this.session.allocateOutputArtifact?.("ssh")) ?? {}; const result = await executeSSH(hostConfig, remoteCommand, { timeout: timeoutMs, diff --git a/packages/coding-agent/src/web/scrapers/lobsters.ts b/packages/coding-agent/src/web/scrapers/lobsters.ts index ac3bc0527..8449f1d2c 100644 --- a/packages/coding-agent/src/web/scrapers/lobsters.ts +++ b/packages/coding-agent/src/web/scrapers/lobsters.ts @@ -10,9 +10,7 @@ interface LobstersStory { title: string; url?: string; description?: string; - submitter_user: { - username: string; - }; + submitter_user: string; score: number; comment_count: number; created_at: string; @@ -22,9 +20,7 @@ interface LobstersStory { interface LobstersComment { short_id: string; comment: string; - commenting_user: { - username: string; - }; + commenting_user: string; score: number; created_at: string; indent_level: number; @@ -36,9 +32,7 @@ interface LobstersStoryResponse { title: string; url?: string; description?: string; - submitter_user: { - username: string; - }; + submitter_user: string; score: number; comment_count: number; created_at: string; @@ -59,7 +53,7 @@ function renderComments(comments: LobstersComment[], maxDepth = 5): string { if (comment.indent_level >= maxDepth) continue; const indent = " ".repeat(comment.indent_level); - md += `${indent}### ${comment.commenting_user.username} · ${comment.score} points\n\n`; + md += `${indent}### ${comment.commenting_user} · ${comment.score} points\n\n`; md += `${indent}${comment.comment.split("\n").join(`\n${indent}`)}\n\n`; if (comment.comments && comment.comments.length > 0) { @@ -94,7 +88,7 @@ export const handleLobsters: SpecialHandler = async (url: string, timeout: numbe if (!story) return null; md = `# ${story.title}\n\n`; - md += `**${story.submitter_user.username}** · ${story.score} points · ${story.comment_count} comments`; + md += `**${story.submitter_user}** · ${story.score} points · ${story.comment_count} comments`; if (story.tags.length > 0) { md += ` · [${story.tags.join(", ")}]`; } @@ -155,7 +149,7 @@ export const handleLobsters: SpecialHandler = async (url: string, timeout: numbe for (const story of listingStories) { md += `- **${story.title}** (${story.score} pts, ${story.comment_count} comments)\n`; - md += ` by ${story.submitter_user.username}`; + md += ` by ${story.submitter_user}`; if (story.tags.length > 0) { md += ` · [${story.tags.join(", ")}]`; } diff --git a/packages/coding-agent/test/streaming-output.test.ts b/packages/coding-agent/test/streaming-output.test.ts index 34f9b4d58..9eab0a4a1 100644 --- a/packages/coding-agent/test/streaming-output.test.ts +++ b/packages/coding-agent/test/streaming-output.test.ts @@ -2,9 +2,7 @@ import { afterEach, describe, expect, test } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; -import { ArtifactManager } from "../src/session/artifacts"; import { - allocateOutputArtifact, DEFAULT_MAX_BYTES, DEFAULT_MAX_COLUMN, DEFAULT_MAX_LINES, @@ -18,7 +16,6 @@ import { truncateTail, truncateTailBytes, } from "../src/session/streaming-output"; -import type { ToolSession } from "../src/tools"; const createdTempDirs: string[] = []; @@ -32,17 +29,6 @@ function byteLength(text: string): number { return Buffer.byteLength(text, "utf-8"); } -function createSession(overrides: Partial = {}): ToolSession { - return { - cwd: "/", - hasUI: false, - getSessionFile: () => null, - getSessionSpawns: () => null, - settings: {} as ToolSession["settings"], - ...overrides, - }; -} - afterEach(async () => { for (const dir of createdTempDirs.splice(0)) { await fs.rm(dir, { recursive: true, force: true }); @@ -288,25 +274,6 @@ describe("OutputSink", () => { }); }); -describe("allocateOutputArtifact", () => { - test("returns empty object when session has no artifact manager", async () => { - const session = createSession({ getArtifactManager: undefined }); - expect(await allocateOutputArtifact(session, "bash")).toEqual({}); - }); - - test("allocates artifact path via session artifact manager", async () => { - const dir = await createTempDir(); - const sessionFile = path.join(dir, "session.jsonl"); - await Bun.write(sessionFile, ""); - const manager = new ArtifactManager(sessionFile); - const session = createSession({ getArtifactManager: () => manager }); - - const result = await allocateOutputArtifact(session, "bash"); - expect(result.id).toBeDefined(); - expect(result.path).toContain(`${path.sep}${result.id}.bash.log`); - }); -}); - describe("truncation notice formatting", () => { test("formatTailTruncationNotice returns empty string for non-truncated results", () => { const truncation = truncateTail("a\nb", { maxLines: 10, maxBytes: 50 }); diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index 8da1d3022..deb121e64 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -4,7 +4,6 @@ import * as os from "node:os"; import * as path from "node:path"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { EditTool } from "@oh-my-pi/pi-coding-agent/patch"; -import { ArtifactManager } from "@oh-my-pi/pi-coding-agent/session/artifacts"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; import { BashTool } from "@oh-my-pi/pi-coding-agent/tools/bash"; import { FindTool } from "@oh-my-pi/pi-coding-agent/tools/find"; @@ -26,13 +25,12 @@ function getTextOutput(result: any): string { function createTestToolSession(cwd: string): ToolSession { const sessionFile = path.join(cwd, "session.jsonl"); - const artifactManager = new ArtifactManager(sessionFile); return { cwd, hasUI: false, getSessionFile: () => sessionFile, getSessionSpawns: () => "*", - getArtifactManager: () => artifactManager, + allocateOutputArtifact: async () => ({}), settings: Settings.isolated(), }; } diff --git a/packages/coding-agent/test/tools/web-scrapers/dev-platforms.test.ts b/packages/coding-agent/test/tools/web-scrapers/dev-platforms.test.ts index 47dd300e1..fc004737c 100644 --- a/packages/coding-agent/test/tools/web-scrapers/dev-platforms.test.ts +++ b/packages/coding-agent/test/tools/web-scrapers/dev-platforms.test.ts @@ -80,11 +80,12 @@ describe.skipIf(SKIP)("handleLobsters", () => { expect(result).toBeNull(); }); - it("returns null for front page due to handler bug", async () => { - // Note: handler has bug with "https://lobste.rs.json" URL construction - // Should use "/hottest.json" but currently constructs invalid URL + it("fetches front page", async () => { const result = await handleLobsters("https://lobste.rs/", 20000); - expect(result).toBeNull(); + expect(result).not.toBeNull(); + expect(result?.method).toBe("lobsters"); + expect(result?.content).toContain("Lobste.rs Front Page"); + expect(result?.content).not.toContain("by undefined"); }); it("fetches newest page", async () => {