From ff6fd3df084cc64fdb957ec853ded760db8ffa15 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 28 Jun 2026 22:47:31 +0200 Subject: [PATCH] test(coding-agent): strengthened coding agent reliability and pathing - Collapsed git worktree path in status line to project name with icon. - Fixed out-of-workspace file edits by including the full path in headers. - Fixed structured output schema violations by correctly handling payload nesting in terminal yields. - Added test suites for git worktree logic, out-of-cwd reading, and subagent output serialization. --- packages/coding-agent/CHANGELOG.md | 6 + .../test/git-linked-worktree.test.ts | 65 +++++++++++ .../coding-agent/test/issue-953-repro.test.ts | 1 + .../test/read-edit-out-of-cwd.test.ts | 106 ++++++++++++++++++ .../test/status-line-model.test.ts | 1 + .../test/status-line-overflow.test.ts | 1 + .../test/status-line-path.test.ts | 58 ++++++++++ .../test/status-line-time-spent.test.ts | 1 + .../test/task/executor-warnings.test.ts | 43 ++++++- 9 files changed, 280 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/git-linked-worktree.test.ts create mode 100644 packages/coding-agent/test/read-edit-out-of-cwd.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b65d05cca..16d3c3fbf 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,9 +2,15 @@ ## [Unreleased] +### Changed + +- Status line now collapses a linked git worktree path to the project name with a worktree icon, leaving the git segment to show the branch once instead of repeating it in the path. + ### Fixed - Fixed recoverable context-overflow compaction keeping the failed assistant error turn in visible session history after scheduling the retry. ([#3747](https://github.com/can1357/oh-my-pi/issues/3747)) +- Fixed editing a file read from outside the workspace (e.g. `~/.claude/settings.json`) failing with "File not found": the read snapshot header now carries the full out-of-workspace path so the edit resolves it directly instead of against the working directory. +- Fixed subagents spawned with an output schema (`agent(..., schema=...)`, `task` with structured output) failing with `schema_violation: missing required fields` since the typed-yield rework: a `type: "result"` finalize carrying the full object was assembled as a section named `result`, nesting the payload one level deep. String-typed yields are now treated as terminal finalizers (their data is the complete result), only array-typed yields form accumulating sections, and a data-less finalize keeps accumulated sections instead of collapsing to the last assistant turn. ## [16.2.4] - 2026-06-28 diff --git a/packages/coding-agent/test/git-linked-worktree.test.ts b/packages/coding-agent/test/git-linked-worktree.test.ts new file mode 100644 index 000000000..a9cf271c1 --- /dev/null +++ b/packages/coding-agent/test/git-linked-worktree.test.ts @@ -0,0 +1,65 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; + +import { repo } from "@oh-my-pi/pi-coding-agent/utils/git"; + +// Builds the on-disk shape of a linked git worktree without invoking git: +// /.git/ ← shared common dir (basename ".git") +// /.git/worktrees// ← this worktree's gitdir +// /.git ← file: `gitdir: <…/worktrees/>` +function linkWorktree(project: string, worktreeRoot: string): void { + const commonDir = path.join(project, ".git"); + const gitDir = path.join(commonDir, "worktrees", path.basename(worktreeRoot)); + fs.mkdirSync(gitDir, { recursive: true }); + fs.mkdirSync(worktreeRoot, { recursive: true }); + fs.writeFileSync(path.join(commonDir, "HEAD"), "ref: refs/heads/main\n", "utf8"); + fs.writeFileSync(path.join(gitDir, "HEAD"), "ref: refs/heads/feature\n", "utf8"); + fs.writeFileSync(path.join(gitDir, "commondir"), `${path.relative(gitDir, commonDir)}\n`, "utf8"); + fs.writeFileSync(path.join(worktreeRoot, ".git"), `gitdir: ${path.relative(worktreeRoot, gitDir)}\n`, "utf8"); +} + +describe("git repo.linkedWorktreeSync", () => { + let tempRoot: string; + + beforeEach(() => { + tempRoot = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), "omp-linked-worktree-"))); + }); + + afterEach(() => { + fs.rmSync(tempRoot, { recursive: true, force: true, maxRetries: 5, retryDelay: 50 }); + }); + + it("names the worktree root and the shared primary checkout", () => { + const project = path.join(tempRoot, "pi"); + const worktreeRoot = path.join(tempRoot, ".tree", "pi", "xx"); + linkWorktree(project, worktreeRoot); + + expect(repo.linkedWorktreeSync(worktreeRoot)).toEqual({ root: worktreeRoot, primaryRoot: project }); + }); + + it("resolves from a subdirectory of the worktree to the worktree root", () => { + const project = path.join(tempRoot, "pi"); + const worktreeRoot = path.join(tempRoot, ".tree", "pi", "xx"); + linkWorktree(project, worktreeRoot); + const sub = path.join(worktreeRoot, "packages", "foo"); + fs.mkdirSync(sub, { recursive: true }); + + expect(repo.linkedWorktreeSync(sub)).toEqual({ root: worktreeRoot, primaryRoot: project }); + }); + + it("returns null for the primary checkout", () => { + const project = path.join(tempRoot, "pi"); + linkWorktree(project, path.join(tempRoot, ".tree", "pi", "xx")); + + expect(repo.linkedWorktreeSync(project)).toBeNull(); + }); + + it("returns null outside any repository", () => { + const bare = path.join(tempRoot, "loose"); + fs.mkdirSync(bare, { recursive: true }); + + expect(repo.linkedWorktreeSync(bare)).toBeNull(); + }); +}); diff --git a/packages/coding-agent/test/issue-953-repro.test.ts b/packages/coding-agent/test/issue-953-repro.test.ts index 3a84682ca..91371eb71 100644 --- a/packages/coding-agent/test/issue-953-repro.test.ts +++ b/packages/coding-agent/test/issue-953-repro.test.ts @@ -39,6 +39,7 @@ function createCtx(usage: Partial): SegmentContext subagentCount: 0, activeMs: 0, activeRepo: null, + worktree: null, git: { branch: null, status: null, diff --git a/packages/coding-agent/test/read-edit-out-of-cwd.test.ts b/packages/coding-agent/test/read-edit-out-of-cwd.test.ts new file mode 100644 index 000000000..64c41848d --- /dev/null +++ b/packages/coding-agent/test/read-edit-out-of-cwd.test.ts @@ -0,0 +1,106 @@ +import { afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; +import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { type ExecuteHashlineSingleOptions, executeHashlineSingle } from "@oh-my-pi/pi-coding-agent/edit"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import type { ReadToolDetails } from "@oh-my-pi/pi-coding-agent/tools/read"; +import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; +import { removeWithRetries } from "@oh-my-pi/pi-utils"; + +function textOutput(result: AgentToolResult): string { + return result.content + .filter(c => c.type === "text") + .map(c => c.text) + .join("\n"); +} + +beforeAll(async () => { + // The edit path's auto-generated-file guard reads the global Settings proxy. + resetSettingsForTest(); + await Settings.init({ inMemory: true, cwd: process.cwd() }); +}); + +function createSession(cwd: string): ToolSession { + const settings = Settings.isolated(); + settings.set("read.summarize.enabled", false); + return { + cwd, + hasUI: false, + getSessionFile: () => path.join(cwd, "session.jsonl"), + getSessionSpawns: () => "*", + getArtifactsDir: () => path.join(cwd, "artifacts"), + allocateOutputArtifact: async () => ({ id: "artifact-1", path: path.join(cwd, "artifact-1.log") }), + settings, + } as unknown as ToolSession; +} + +function editOptions(session: ToolSession, input: string): ExecuteHashlineSingleOptions { + return { + session, + input, + writethrough: async (targetPath, content) => { + await Bun.write(targetPath, content); + return undefined; + }, + beginDeferredDiagnosticsForPath: () => ({ + onDeferredDiagnostics: () => {}, + signal: new AbortController().signal, + finalize: () => {}, + }), + }; +} + +// Regression: reading a file *outside* the session cwd (e.g. `~/.claude/settings.json`) +// and then editing it anchored on the emitted hashline header. The header used to +// collapse to the bare filename for every read; out-of-tree the edit tool's +// snapshot-tag recovery refuses to rebind a bare name (allowTagPathRecovery), so the +// path resolved against cwd, missed, and failed with "File not found". The header now +// carries the full out-of-cwd path so the edit resolves directly. +describe("read → edit round-trip for out-of-cwd files", () => { + let cwdDir: string; + let outDir: string; + + beforeEach(async () => { + cwdDir = await fs.mkdtemp(path.join(os.tmpdir(), "read-edit-cwd-")); + outDir = await fs.mkdtemp(path.join(os.tmpdir(), "read-edit-out-")); + }); + + afterEach(async () => { + await removeWithRetries(cwdDir); + await removeWithRetries(outDir); + }); + + it("anchors the out-of-cwd path in the header so a follow-up edit lands", async () => { + const outFile = path.join(outDir, "settings.json"); + await fs.writeFile(outFile, "alpha\nbeta\n"); + + const session = createSession(cwdDir); + const header = textOutput(await new ReadTool(session).execute("read-out", { path: outFile })).split("\n")[0]; + + // The header must carry the directory, not just `settings.json`, or the + // edit below would resolve the bare name against cwdDir and miss. + expect(header).toMatch(/^\[.+settings\.json#[0-9A-F]{4}\]$/); + expect(header).toContain(path.basename(outDir)); + + const result = await executeHashlineSingle(editOptions(session, `${header}\nSWAP 1.=1:\n+ALPHA\n`)); + const resultText = result.content.map(part => (part.type === "text" ? part.text : "")).join("\n"); + + expect(resultText).not.toContain("File not found"); + expect(await Bun.file(outFile).text()).toBe("ALPHA\nbeta\n"); + }); + + it("still collapses an in-cwd read header to the bare filename", async () => { + const inFile = path.join(cwdDir, "src", "settings.json"); + await fs.mkdir(path.dirname(inFile), { recursive: true }); + await fs.writeFile(inFile, "alpha\nbeta\n"); + + const session = createSession(cwdDir); + const header = textOutput(await new ReadTool(session).execute("read-in", { path: inFile })).split("\n")[0]; + + expect(header).toMatch(/^\[settings\.json#[0-9A-F]{4}\]$/); + expect(header).not.toContain("src"); + }); +}); diff --git a/packages/coding-agent/test/status-line-model.test.ts b/packages/coding-agent/test/status-line-model.test.ts index afbda9c9d..03ba9ce85 100644 --- a/packages/coding-agent/test/status-line-model.test.ts +++ b/packages/coding-agent/test/status-line-model.test.ts @@ -40,6 +40,7 @@ function createModelContext(advisorActive: boolean): SegmentContext { subagentCount: 0, activeMs: 0, activeRepo: null, + worktree: null, git: { branch: null, status: null, pr: null }, usage: null, }; diff --git a/packages/coding-agent/test/status-line-overflow.test.ts b/packages/coding-agent/test/status-line-overflow.test.ts index e551be5de..16f543c2e 100644 --- a/packages/coding-agent/test/status-line-overflow.test.ts +++ b/packages/coding-agent/test/status-line-overflow.test.ts @@ -63,6 +63,7 @@ function createCtx(overrides?: { pathMaxLength?: number; branch?: string | null subagentCount: 0, activeMs: 0, activeRepo: null, + worktree: null, git: { branch: overrides?.branch ?? null, status: null, diff --git a/packages/coding-agent/test/status-line-path.test.ts b/packages/coding-agent/test/status-line-path.test.ts index e1c37fc8a..3c8a6b431 100644 --- a/packages/coding-agent/test/status-line-path.test.ts +++ b/packages/coding-agent/test/status-line-path.test.ts @@ -49,6 +49,7 @@ function createPathContext(): SegmentContext { subagentCount: 0, activeMs: 0, activeRepo: null, + worktree: null, git: { branch: null, status: null, @@ -233,3 +234,60 @@ describe("status line path segment", () => { } }); }); + +describe("status line path segment in a linked worktree", () => { + function worktreeContext( + worktree: { projectName: string; worktreeName: string } | null, + branch: string | null, + ): SegmentContext { + const ctx = createPathContext(); + ctx.worktree = worktree; + ctx.git = { branch, status: null, pr: null }; + return ctx; + } + + it("collapses to the project name and drops the worktree dir when it equals the branch", () => { + const rendered = renderSegment("path", worktreeContext({ projectName: "pi", worktreeName: "xx" }, "xx")); + const content = Bun.stripANSI(rendered.content); + expect(rendered.visible).toBe(true); + expect(content).toBe(`${theme.icon.worktree} pi`); + // The base prefix, the worktree dir, and the folder icon are all gone. + expect(content).not.toContain(".tree"); + expect(content).not.toContain("/xx"); + expect(content).not.toContain(theme.icon.folder); + }); + + it("keeps the worktree dir when it diverges from the branch", () => { + const rendered = renderSegment("path", worktreeContext({ projectName: "pi", worktreeName: "wt-icon" }, "icon")); + expect(Bun.stripANSI(rendered.content)).toBe(`${theme.icon.worktree} pi/wt-icon`); + }); + + it("keeps the worktree dir when no branch is shown", () => { + const rendered = renderSegment("path", worktreeContext({ projectName: "pi", worktreeName: "xx" }, null)); + expect(Bun.stripANSI(rendered.content)).toBe(`${theme.icon.worktree} pi/xx`); + }); + + it("falls back to the on-disk path when stripWorkPrefix is disabled", () => { + const scratchDir = fs.mkdtempSync(path.join(os.tmpdir(), "omp-status-line-wt-noprefix-")); + try { + setProjectDir(scratchDir); + const ctx = worktreeContext({ projectName: "pi", worktreeName: "xx" }, "xx"); + ctx.options.path = { ...ctx.options.path, stripWorkPrefix: false }; + const content = Bun.stripANSI(renderSegment("path", ctx).content); + expect(content).not.toContain(theme.icon.worktree); + expect(content).toContain(theme.icon.folder); + } finally { + setProjectDir(originalProjectDir); + removeSyncWithRetries(scratchDir); + } + }); + + it("clamps a long worktree label to maxLength so overflow shrink works", () => { + const ctx = worktreeContext({ projectName: "very-long-project-name", worktreeName: "feature" }, "other"); + ctx.options.path = { ...ctx.options.path, maxLength: 10 }; + const label = Bun.stripANSI(renderSegment("path", ctx).content).slice(theme.icon.worktree.length + 1); + expect(label.length).toBeLessThanOrEqual(10); + expect(label.startsWith("…")).toBe(true); + expect(label.endsWith("feature")).toBe(true); + }); +}); diff --git a/packages/coding-agent/test/status-line-time-spent.test.ts b/packages/coding-agent/test/status-line-time-spent.test.ts index 45800dcfb..19def6cea 100644 --- a/packages/coding-agent/test/status-line-time-spent.test.ts +++ b/packages/coding-agent/test/status-line-time-spent.test.ts @@ -59,6 +59,7 @@ function createCtx(activeMs: number): SegmentContext { subagentCount: 0, activeMs, activeRepo: null, + worktree: null, git: { branch: null, status: null, pr: null }, usage: null, }; diff --git a/packages/coding-agent/test/task/executor-warnings.test.ts b/packages/coding-agent/test/task/executor-warnings.test.ts index 7dfd1262a..046f6f10d 100644 --- a/packages/coding-agent/test/task/executor-warnings.test.ts +++ b/packages/coding-agent/test/task/executor-warnings.test.ts @@ -301,7 +301,10 @@ describe("subagent warning injection", () => { expect(result.rawOutput).toBe("final answer from the assistant"); }); - it("lets a terminal string-typed last-turn result override earlier incremental sections", () => { + it("keeps accumulated sections when a data-less terminal yield finalizes", () => { + // Previously a data-less `type: "final"` collapsed to the last assistant + // turn and dropped earlier incremental sections; the sections are the work + // product, so they must survive the finalize. const result = finalizeSubprocessOutput({ rawOutput: "", exitCode: 0, @@ -317,7 +320,43 @@ describe("subagent warning injection", () => { }); expect(result.exitCode).toBe(0); - expect(result.rawOutput).toBe("plain final answer"); + expect(JSON.parse(result.rawOutput)).toEqual({ summary: "first" }); + }); + + it("finalizes a string-typed terminal yield that carries data as the top-level result", () => { + // Regression: a `type: "result"` finalize with the full structured object + // was treated as a section labeled "result", nesting the payload one level + // deep so every required field read as missing (schema_violation). + const result = finalizeSubprocessOutput({ + rawOutput: "", + exitCode: 0, + stderr: "", + doneAborted: false, + signalAborted: false, + yieldItems: [ + { + status: "success", + type: "result", + data: { summary: "did it", filesChanged: ["a.ts"], notes: ["ok"] }, + }, + ], + outputSchema: { + properties: { + summary: { type: "string" }, + filesChanged: { elements: { type: "string" } }, + notes: { elements: { type: "string" } }, + }, + }, + lastAssistantText: "some prose that must not become the result", + }); + + expect(result.stderr).not.toContain("schema_violation"); + expect(result.exitCode).toBe(0); + expect(JSON.parse(result.rawOutput)).toEqual({ + summary: "did it", + filesChanged: ["a.ts"], + notes: ["ok"], + }); }); it("serializes untyped useLastTurn yield as raw text", () => {