mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-14 01:50:39 +09:00
Address greptile review feedback
This commit is contained in:
parent
81b96c6021
commit
b5e177df7e
4 changed files with 208 additions and 14 deletions
|
|
@ -586,6 +586,87 @@ describe("worktree helpers", () => {
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("restores the current worktree config and instance data if reseed fails", async () => {
|
||||||
|
const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "paperclip-worktree-reseed-rollback-"));
|
||||||
|
const repoRoot = path.join(tempRoot, "repo");
|
||||||
|
const sourceRoot = path.join(tempRoot, "source");
|
||||||
|
const homeDir = path.join(tempRoot, ".paperclip-worktrees");
|
||||||
|
const currentInstanceId = "rollback-worktree";
|
||||||
|
const currentPaths = resolveWorktreeLocalPaths({
|
||||||
|
cwd: repoRoot,
|
||||||
|
homeDir,
|
||||||
|
instanceId: currentInstanceId,
|
||||||
|
});
|
||||||
|
const sourcePaths = resolveWorktreeLocalPaths({
|
||||||
|
cwd: sourceRoot,
|
||||||
|
homeDir: path.join(tempRoot, ".paperclip-source"),
|
||||||
|
instanceId: "default",
|
||||||
|
});
|
||||||
|
const originalCwd = process.cwd();
|
||||||
|
const originalPaperclipConfig = process.env.PAPERCLIP_CONFIG;
|
||||||
|
|
||||||
|
try {
|
||||||
|
fs.mkdirSync(path.dirname(currentPaths.configPath), { recursive: true });
|
||||||
|
fs.mkdirSync(path.dirname(sourcePaths.configPath), { recursive: true });
|
||||||
|
fs.mkdirSync(currentPaths.instanceRoot, { recursive: true });
|
||||||
|
fs.mkdirSync(path.dirname(sourcePaths.secretsKeyFilePath), { recursive: true });
|
||||||
|
fs.mkdirSync(repoRoot, { recursive: true });
|
||||||
|
fs.mkdirSync(sourceRoot, { recursive: true });
|
||||||
|
|
||||||
|
const currentConfig = buildWorktreeConfig({
|
||||||
|
sourceConfig: buildSourceConfig(),
|
||||||
|
paths: currentPaths,
|
||||||
|
serverPort: 3114,
|
||||||
|
databasePort: 54341,
|
||||||
|
});
|
||||||
|
const sourceConfig = {
|
||||||
|
...buildSourceConfig(),
|
||||||
|
database: {
|
||||||
|
mode: "postgres",
|
||||||
|
connectionString: "",
|
||||||
|
},
|
||||||
|
secrets: {
|
||||||
|
provider: "local_encrypted",
|
||||||
|
strictMode: false,
|
||||||
|
localEncrypted: {
|
||||||
|
keyFilePath: sourcePaths.secretsKeyFilePath,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
} as PaperclipConfig;
|
||||||
|
|
||||||
|
fs.writeFileSync(currentPaths.configPath, JSON.stringify(currentConfig, null, 2), "utf8");
|
||||||
|
fs.writeFileSync(currentPaths.envPath, `PAPERCLIP_HOME=${homeDir}\nPAPERCLIP_INSTANCE_ID=${currentInstanceId}\n`, "utf8");
|
||||||
|
fs.writeFileSync(path.join(currentPaths.instanceRoot, "marker.txt"), "keep me", "utf8");
|
||||||
|
fs.writeFileSync(sourcePaths.configPath, JSON.stringify(sourceConfig, null, 2), "utf8");
|
||||||
|
fs.writeFileSync(sourcePaths.secretsKeyFilePath, "source-secret", "utf8");
|
||||||
|
|
||||||
|
delete process.env.PAPERCLIP_CONFIG;
|
||||||
|
process.chdir(repoRoot);
|
||||||
|
|
||||||
|
await expect(worktreeReseedCommand({
|
||||||
|
fromConfig: sourcePaths.configPath,
|
||||||
|
yes: true,
|
||||||
|
})).rejects.toThrow("Source instance uses postgres mode but has no connection string");
|
||||||
|
|
||||||
|
const restoredConfig = JSON.parse(fs.readFileSync(currentPaths.configPath, "utf8"));
|
||||||
|
const restoredEnv = fs.readFileSync(currentPaths.envPath, "utf8");
|
||||||
|
const restoredMarker = fs.readFileSync(path.join(currentPaths.instanceRoot, "marker.txt"), "utf8");
|
||||||
|
|
||||||
|
expect(restoredConfig.server.port).toBe(3114);
|
||||||
|
expect(restoredConfig.database.embeddedPostgresPort).toBe(54341);
|
||||||
|
expect(restoredEnv).toContain(`PAPERCLIP_INSTANCE_ID=${currentInstanceId}`);
|
||||||
|
expect(restoredMarker).toBe("keep me");
|
||||||
|
} finally {
|
||||||
|
process.chdir(originalCwd);
|
||||||
|
if (originalPaperclipConfig === undefined) {
|
||||||
|
delete process.env.PAPERCLIP_CONFIG;
|
||||||
|
} else {
|
||||||
|
process.env.PAPERCLIP_CONFIG = originalPaperclipConfig;
|
||||||
|
}
|
||||||
|
fs.rmSync(tempRoot, { recursive: true, force: true });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
it("rebinds same-repo workspace paths onto the current worktree root", () => {
|
it("rebinds same-repo workspace paths onto the current worktree root", () => {
|
||||||
expect(
|
expect(
|
||||||
rebindWorkspaceCwd({
|
rebindWorkspaceCwd({
|
||||||
|
|
|
||||||
|
|
@ -108,6 +108,12 @@ type WorktreeReseedOptions = {
|
||||||
seed?: boolean;
|
seed?: boolean;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
type WorktreeReseedBackup = {
|
||||||
|
tempRoot: string;
|
||||||
|
repoConfigDirBackup: string | null;
|
||||||
|
instanceRootBackup: string | null;
|
||||||
|
};
|
||||||
|
|
||||||
type WorktreeEnvOptions = {
|
type WorktreeEnvOptions = {
|
||||||
config?: string;
|
config?: string;
|
||||||
json?: boolean;
|
json?: boolean;
|
||||||
|
|
@ -1109,6 +1115,48 @@ function resolveCurrentWorktreeReseedState(opts: { home?: string } = {}) {
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async function snapshotDirectory(sourcePath: string, targetPath: string): Promise<string | null> {
|
||||||
|
if (!existsSync(sourcePath)) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
await fsPromises.cp(sourcePath, targetPath, { recursive: true });
|
||||||
|
return targetPath;
|
||||||
|
}
|
||||||
|
|
||||||
|
async function snapshotWorktreeReseedState(target: {
|
||||||
|
repoConfigDir: string;
|
||||||
|
instanceRoot: string;
|
||||||
|
}): Promise<WorktreeReseedBackup> {
|
||||||
|
const tempRoot = await fsPromises.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-reseed-backup-"));
|
||||||
|
return {
|
||||||
|
tempRoot,
|
||||||
|
repoConfigDirBackup: await snapshotDirectory(
|
||||||
|
target.repoConfigDir,
|
||||||
|
path.resolve(tempRoot, "repo-config"),
|
||||||
|
),
|
||||||
|
instanceRootBackup: await snapshotDirectory(
|
||||||
|
target.instanceRoot,
|
||||||
|
path.resolve(tempRoot, "instance-root"),
|
||||||
|
),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
async function restoreDirectoryBackup(backupPath: string | null, targetPath: string): Promise<void> {
|
||||||
|
rmSync(targetPath, { recursive: true, force: true });
|
||||||
|
if (!backupPath) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
await fsPromises.cp(backupPath, targetPath, { recursive: true });
|
||||||
|
}
|
||||||
|
|
||||||
|
async function restoreWorktreeReseedState(
|
||||||
|
backup: WorktreeReseedBackup,
|
||||||
|
target: { repoConfigDir: string; instanceRoot: string },
|
||||||
|
): Promise<void> {
|
||||||
|
await restoreDirectoryBackup(backup.repoConfigDirBackup, target.repoConfigDir);
|
||||||
|
await restoreDirectoryBackup(backup.instanceRootBackup, target.instanceRoot);
|
||||||
|
}
|
||||||
|
|
||||||
export async function worktreeReseedCommand(opts: WorktreeReseedOptions): Promise<void> {
|
export async function worktreeReseedCommand(opts: WorktreeReseedOptions): Promise<void> {
|
||||||
printPaperclipCliBanner();
|
printPaperclipCliBanner();
|
||||||
p.intro(pc.bgCyan(pc.black(" paperclipai worktree reseed ")));
|
p.intro(pc.bgCyan(pc.black(" paperclipai worktree reseed ")));
|
||||||
|
|
@ -1143,6 +1191,14 @@ export async function worktreeReseedCommand(opts: WorktreeReseedOptions): Promis
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const targetPaths = resolveWorktreeLocalPaths({
|
||||||
|
cwd: process.cwd(),
|
||||||
|
homeDir: target.homeDir,
|
||||||
|
instanceId: target.instanceId,
|
||||||
|
});
|
||||||
|
const backup = await snapshotWorktreeReseedState(targetPaths);
|
||||||
|
|
||||||
|
try {
|
||||||
await runWorktreeInit({
|
await runWorktreeInit({
|
||||||
name: target.worktreeName,
|
name: target.worktreeName,
|
||||||
color: target.worktreeColor,
|
color: target.worktreeColor,
|
||||||
|
|
@ -1158,6 +1214,12 @@ export async function worktreeReseedCommand(opts: WorktreeReseedOptions): Promis
|
||||||
seedMode,
|
seedMode,
|
||||||
force: true,
|
force: true,
|
||||||
});
|
});
|
||||||
|
} catch (error) {
|
||||||
|
await restoreWorktreeReseedState(backup, targetPaths);
|
||||||
|
throw error;
|
||||||
|
} finally {
|
||||||
|
rmSync(backup.tempRoot, { recursive: true, force: true });
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function worktreeMakeCommand(nameArg: string, opts: WorktreeMakeOptions): Promise<void> {
|
export async function worktreeMakeCommand(nameArg: string, opts: WorktreeMakeOptions): Promise<void> {
|
||||||
|
|
|
||||||
|
|
@ -170,6 +170,29 @@ describe("buildAssistantPartsFromTranscript", () => {
|
||||||
]);
|
]);
|
||||||
expect(result.notices).toEqual([]);
|
expect(result.notices).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("preserves diff transcript output as a fenced diff block", () => {
|
||||||
|
const result = buildAssistantPartsFromTranscript([
|
||||||
|
{ kind: "assistant", ts: "2026-04-06T12:00:00.000Z", text: "Applied the patch." },
|
||||||
|
{ kind: "diff", ts: "2026-04-06T12:00:01.000Z", changeType: "file_header", text: "ui/src/lib/issue-chat-messages.ts" },
|
||||||
|
{ kind: "diff", ts: "2026-04-06T12:00:02.000Z", changeType: "add", text: "+function formatDiffBlock(lines: string[]) {" },
|
||||||
|
{ kind: "diff", ts: "2026-04-06T12:00:03.000Z", changeType: "add", text: "+ return ````diff`;" },
|
||||||
|
]);
|
||||||
|
|
||||||
|
expect(result.parts).toMatchObject([
|
||||||
|
{ type: "text", text: "Applied the patch." },
|
||||||
|
{
|
||||||
|
type: "text",
|
||||||
|
text: [
|
||||||
|
"```diff",
|
||||||
|
"ui/src/lib/issue-chat-messages.ts",
|
||||||
|
"+function formatDiffBlock(lines: string[]) {",
|
||||||
|
"+ return ````diff`;",
|
||||||
|
"```",
|
||||||
|
].join("\n"),
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("buildIssueChatMessages", () => {
|
describe("buildIssueChatMessages", () => {
|
||||||
|
|
|
||||||
|
|
@ -151,6 +151,10 @@ function mergePartText(
|
||||||
: `${previous.text}\n${next.text}`;
|
: `${previous.text}\n${next.text}`;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function formatDiffBlock(lines: string[]) {
|
||||||
|
return `\`\`\`diff\n${lines.join("\n")}\n\`\`\``;
|
||||||
|
}
|
||||||
|
|
||||||
function createAssistantMetadata(custom: Record<string, unknown>) {
|
function createAssistantMetadata(custom: Record<string, unknown>) {
|
||||||
return {
|
return {
|
||||||
unstable_state: null,
|
unstable_state: null,
|
||||||
|
|
@ -297,6 +301,7 @@ function computeSegmentTimings(entries: readonly IssueChatTranscriptEntry[]): Se
|
||||||
entry.kind === "thinking" ||
|
entry.kind === "thinking" ||
|
||||||
entry.kind === "tool_call" ||
|
entry.kind === "tool_call" ||
|
||||||
entry.kind === "tool_result" ||
|
entry.kind === "tool_result" ||
|
||||||
|
entry.kind === "diff" ||
|
||||||
(entry.kind === "result" && ((entry.isError && !!entry.errors?.length) || !!entry.text));
|
(entry.kind === "result" && ((entry.isError && !!entry.errors?.length) || !!entry.text));
|
||||||
const isText = entry.kind === "assistant" && !!entry.text;
|
const isText = entry.kind === "assistant" && !!entry.text;
|
||||||
|
|
||||||
|
|
@ -434,8 +439,29 @@ export function buildAssistantPartsFromTranscript(entries: readonly IssueChatTra
|
||||||
const toolParts = new Map<string, ToolCallMessagePart<JsonObject, unknown>>();
|
const toolParts = new Map<string, ToolCallMessagePart<JsonObject, unknown>>();
|
||||||
const toolIndices = new Map<string, number>();
|
const toolIndices = new Map<string, number>();
|
||||||
const notices: string[] = [];
|
const notices: string[] = [];
|
||||||
|
let pendingDiffLines: string[] = [];
|
||||||
|
let pendingDiffParentId: string | undefined;
|
||||||
|
|
||||||
|
const flushPendingDiff = () => {
|
||||||
|
if (pendingDiffLines.length === 0) return;
|
||||||
|
orderedParts.push({
|
||||||
|
type: "text",
|
||||||
|
text: formatDiffBlock(pendingDiffLines),
|
||||||
|
parentId: pendingDiffParentId,
|
||||||
|
});
|
||||||
|
pendingDiffLines = [];
|
||||||
|
pendingDiffParentId = undefined;
|
||||||
|
};
|
||||||
|
|
||||||
for (const [index, entry] of entries.entries()) {
|
for (const [index, entry] of entries.entries()) {
|
||||||
|
if (entry.kind === "diff") {
|
||||||
|
pendingDiffParentId ??= `diff-group:${index}`;
|
||||||
|
pendingDiffLines.push(entry.text ?? "");
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
flushPendingDiff();
|
||||||
|
|
||||||
if (entry.kind === "assistant" && entry.text) {
|
if (entry.kind === "assistant" && entry.text) {
|
||||||
orderedParts.push({ type: "text", text: entry.text });
|
orderedParts.push({ type: "text", text: entry.text });
|
||||||
continue;
|
continue;
|
||||||
|
|
@ -510,6 +536,8 @@ export function buildAssistantPartsFromTranscript(entries: readonly IssueChatTra
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
flushPendingDiff();
|
||||||
|
|
||||||
const mergedParts: Array<TextMessagePart | ReasoningMessagePart | ToolCallMessagePart<JsonObject, unknown>> = [];
|
const mergedParts: Array<TextMessagePart | ReasoningMessagePart | ToolCallMessagePart<JsonObject, unknown>> = [];
|
||||||
for (const part of orderedParts) {
|
for (const part of orderedParts) {
|
||||||
if (part.type === "tool-call") {
|
if (part.type === "tool-call") {
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue