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.
This commit is contained in:
can1357
@@ -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
|
||||
|
||||
|
||||
@@ -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:
|
||||
// <project>/.git/ ← shared common dir (basename ".git")
|
||||
// <project>/.git/worktrees/<name>/ ← this worktree's gitdir
|
||||
// <worktreeRoot>/.git ← file: `gitdir: <…/worktrees/<name>>`
|
||||
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();
|
||||
});
|
||||
});
|
||||
@@ -39,6 +39,7 @@ function createCtx(usage: Partial<SegmentContext["usageStats"]>): SegmentContext
|
||||
subagentCount: 0,
|
||||
activeMs: 0,
|
||||
activeRepo: null,
|
||||
worktree: null,
|
||||
git: {
|
||||
branch: null,
|
||||
status: null,
|
||||
|
||||
@@ -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<ReadToolDetails>): 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");
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
Reference in New Issue
Block a user