mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
Raúl
9a450a69e7
* feat(annotate): add strict atomic result output * feat(annotate): exit 2 for strict-gate usage and publication errors Adopt the grep convention for the strict annotate gate's exit codes: 0 = approved, 1 = negative human outcome (annotated/dismissed under --require-approval), 2 = the gate itself was misconfigured or could not start/deliver a decision. Previously all usage/startup/validation failures shared exit 1 with "reviewer did not approve", so callers could not tell a denied review from a broken gate. - parseStrictAnnotateOptions failures (bad flag combos, strict flags outside annotate --gate --json) now exit 2 - --result-file preflight failures (missing parent, pre-existing or dangling-symlink destination) now exit 2 - post-decision publication failures (destination raced into existence, hard links unavailable, stdout write failure) now exit 2: they deliver no decision record at all, so the code's own fail-closed handling presents them as environment errors, never as a reviewer outcome -- and never approval, since only 0 means approved - decision outcomes keep 0/1 exactly as before; signal deaths keep 128+n - document the contract in AGENTS.md and the annotate-gates guide Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk * feat(annotate): preserve notes on structured approval * test(pi): use exact annotate outcome import * fix(annotate): exit 2 for strict-gate startup failures The six startup-failure sites in the annotate path (missing path, unreachable URL, empty folder, ambiguous name, missing/unsupported file, oversized file) run after flag parsing and exited 1. Under --require-approval / --result-file, 1 is the "reviewer requested changes" signal, so a typo'd path made automation misclassify a configuration error as a legitimate rejection. Route those sites through exitAnnotateStartupFailure(), which picks its code from the already-parsed strict options via the new pure helper annotateStartupFailureExitCode(). Non-strict invocations still exit 1 with byte-identical stderr; strict invocations exit STRICT_GATE_ERROR_EXIT_CODE (2). Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * fix(annotate): emit the strict decision on stdout before publishing it writeResultFile ran before the decision JSON reached stdout. On a filesystem without hard links (exFAT, FAT32, most SMB/NFS, some container bind mounts) publication fails deterministically, the catch exited 2 with nothing written anywhere — and the reviewer's autosaved draft had already been deleted by the feedback flow, so their completed decision was lost. Emit the stdout record first, then publish the result file. Exit semantics are unchanged: a publication failure still exits 2, but the decision has reached stdout by then. Only a stdout write failure now leaves no record at all. Correct the docs and comments that claimed exit 2 delivers no decision record: it means the result *file* was not published. Also document the two publication caveats: the 0600 mode is a no-op on Windows, and the atomic link/rename is not followed by a parent-directory fsync, so publication is atomic but not crash-durable. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * fix(annotate): parse linked docs with the render-side frontmatter rule on export buildCompleteAnnotateFeedback re-parsed each linked document with parseMarkdownToBlocks(entry.markdown) — no options, so frontmatter stripping defaulted on. The render side parses with { frontmatter: shouldStripFrontmatter(path) }. For plain-text linked docs (.yaml/.json/.toml/…) a leading `---` is real content, not frontmatter: a multi-document YAML opens with it. Stripping it on the export side shifted every block id, so ordinary Send Feedback and deny emitted wrong `(line N)` labels — or dropped them entirely when the annotation's block no longer existed. Pass the same shouldStripFrontmatter(filepath) option at the export call site so both sides agree. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * fix(annotate): carry the message scope through approve-with-notes /api/feedback forwards selectedMessageId and feedbackScope; /api/approve dropped them. Pi resolves the anchor message from those fields, so notes delivered on the approve path anchored to the last message instead of the one the reviewer picked in a multi-message annotate-last session — while Send Feedback in the same session anchored correctly. Forward both fields on the approve path in the Bun and Pi servers, and have the client build the approval body with the same scope resolution Send Feedback uses (extracted as getFeedbackMessageScope so the two can no longer drift). Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * docs(annotate): tell agents an approval may carry notes The skill and slash-command files still described `"decision": "approved"` as "acknowledge and stop", with no mention of the feedback field the gate can now attach — so an agent reading them would silently drop the reviewer's approval notes. Update the Claude core/claude skills, the Copilot commands, the Gemini annotate command, and the annotate command reference so the approved branch names the optional feedback field and says what to do with it: carry it into subsequent work, do not treat it as a change request. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * docs(annotate): document the real approvedWithNotes default The default annotate.approvedWithNotes template is `{{contextBlock}}{{feedback}}`, not `{{context}}` on its own line, and {{contextBlock}} was missing from the variable table entirely. Show the actual default, add {{contextBlock}} to the variable table, and explain why the default prefers it: it collapses to nothing for message annotations instead of leaving a stray blank line. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com>
291 lines
9.9 KiB
TypeScript
291 lines
9.9 KiB
TypeScript
import { afterEach, describe, expect, mock, test } from "bun:test";
|
|
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs";
|
|
import { tmpdir } from "os";
|
|
import path from "path";
|
|
import { handleAnnotateCommand, handleAnnotateLastCommand } from "./commands";
|
|
import { OpenCodePromptDeliveryError } from "./prompt-delivery-error";
|
|
|
|
// Inject the annotate-server stub through CommandDeps rather than
|
|
// `mock.module`. Bun's module mocks are process-global and cannot be unset,
|
|
// so a `mock.module("@plannotator/server/annotate", ...)` here would leak the
|
|
// stub into every other suite (it previously broke packages/server tests that
|
|
// boot the real annotate server). Dependency injection keeps it local.
|
|
const startAnnotateServerMock = mock(async (_options: any) => ({
|
|
port: 0,
|
|
url: "http://localhost",
|
|
isRemote: false,
|
|
waitForDecision: async () => ({ feedback: "", annotations: [] }),
|
|
stop: () => {},
|
|
}));
|
|
|
|
const tempDirs: string[] = [];
|
|
|
|
function makeTempDir(): string {
|
|
const dir = mkdtempSync(path.join(tmpdir(), "plannotator-opencode-commands-"));
|
|
tempDirs.push(dir);
|
|
return dir;
|
|
}
|
|
|
|
function makeDeps() {
|
|
return {
|
|
client: {
|
|
app: {
|
|
log: mock((_entry: unknown) => {}),
|
|
},
|
|
session: {
|
|
prompt: mock(async (_input: unknown) => {}),
|
|
messages: mock(async (_input: unknown) => ({ data: [] })),
|
|
},
|
|
},
|
|
htmlContent: "<html></html>",
|
|
reviewHtmlContent: "<html></html>",
|
|
getSharingEnabled: async () => true,
|
|
getShareBaseUrl: () => "https://share.example.test",
|
|
getPasteApiUrl: () => "https://paste.example.test",
|
|
directory: undefined as string | undefined,
|
|
startAnnotateServer: startAnnotateServerMock,
|
|
};
|
|
}
|
|
|
|
afterEach(() => {
|
|
startAnnotateServerMock.mockClear();
|
|
for (const dir of tempDirs.splice(0)) {
|
|
rmSync(dir, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
describe("handleAnnotateCommand", () => {
|
|
test("advertises approval notes only when an OpenCode session is available", async () => {
|
|
const projectRoot = makeTempDir();
|
|
const filePath = path.join(projectRoot, "plan.md");
|
|
writeFileSync(filePath, "# Plan\n");
|
|
|
|
const withSession = makeDeps();
|
|
withSession.directory = projectRoot;
|
|
await handleAnnotateCommand(
|
|
{ properties: { arguments: "plan.md --gate", sessionID: "session-123" } },
|
|
withSession,
|
|
);
|
|
expect(startAnnotateServerMock.mock.calls[0]?.[0].approvalNotesSupported).toBe(true);
|
|
|
|
startAnnotateServerMock.mockClear();
|
|
const withoutSession = makeDeps();
|
|
withoutSession.directory = projectRoot;
|
|
await handleAnnotateCommand(
|
|
{ properties: { arguments: "plan.md --gate" } },
|
|
withoutSession,
|
|
);
|
|
expect(startAnnotateServerMock.mock.calls[0]?.[0].approvalNotesSupported).toBe(false);
|
|
});
|
|
|
|
test("injects approved feedback as non-blocking notes with file context", async () => {
|
|
const projectRoot = makeTempDir();
|
|
const filePath = path.join(projectRoot, "plan.md");
|
|
writeFileSync(filePath, "# Plan\n");
|
|
const deps: any = makeDeps();
|
|
deps.directory = projectRoot;
|
|
deps.startAnnotateServer = mock(async (options: any) => ({
|
|
port: 0,
|
|
url: "http://localhost",
|
|
isRemote: false,
|
|
options,
|
|
waitForDecision: async () => ({
|
|
approved: true,
|
|
feedback: "Keep the retry bounded.",
|
|
annotations: [{ id: "a1" }],
|
|
}),
|
|
stop: () => {},
|
|
}));
|
|
|
|
await handleAnnotateCommand(
|
|
{ properties: { arguments: "plan.md --gate", sessionID: "session-123" } },
|
|
deps,
|
|
);
|
|
|
|
expect(deps.client.session.prompt).toHaveBeenCalledTimes(1);
|
|
const prompt = deps.client.session.prompt.mock.calls[0]?.[0].body.parts[0].text;
|
|
expect(prompt).toContain("artifact is approved");
|
|
expect(prompt).toContain("non-blocking guidance");
|
|
expect(prompt).toContain(`File: ${filePath}`);
|
|
expect(prompt).toContain("Keep the retry bounded.");
|
|
expect(prompt).not.toContain("Please address");
|
|
});
|
|
|
|
test("logs and rejects when approved file notes cannot be injected", async () => {
|
|
const projectRoot = makeTempDir();
|
|
const filePath = path.join(projectRoot, "plan.md");
|
|
writeFileSync(filePath, "# Plan\n");
|
|
const deps: any = makeDeps();
|
|
deps.directory = projectRoot;
|
|
deps.client.session.prompt = mock(async () => {
|
|
throw new Error("session busy");
|
|
});
|
|
deps.startAnnotateServer = mock(async () => ({
|
|
port: 0,
|
|
url: "http://localhost",
|
|
isRemote: false,
|
|
waitForDecision: async () => ({
|
|
approved: true,
|
|
feedback: "Keep the retry bounded.",
|
|
annotations: [{ id: "a1" }],
|
|
}),
|
|
stop: () => {},
|
|
}));
|
|
|
|
try {
|
|
await handleAnnotateCommand(
|
|
{ properties: { arguments: "plan.md --gate", sessionID: "session-123" } },
|
|
deps,
|
|
);
|
|
throw new Error("Expected prompt delivery to fail");
|
|
} catch (error) {
|
|
expect(error).toBeInstanceOf(OpenCodePromptDeliveryError);
|
|
expect(error).toHaveProperty(
|
|
"message",
|
|
"Could not deliver approved annotation notes to the OpenCode session.",
|
|
);
|
|
}
|
|
expect(deps.client.app.log).toHaveBeenCalledWith({
|
|
level: "error",
|
|
message: expect.stringContaining("Could not deliver approved annotation notes"),
|
|
});
|
|
});
|
|
|
|
test("strips wrapping quotes from HTML paths and forwards pasteApiUrl", async () => {
|
|
const projectRoot = makeTempDir();
|
|
const docsDir = path.join(projectRoot, "docs");
|
|
mkdirSync(docsDir, { recursive: true });
|
|
const htmlPath = path.join(docsDir, "Design Spec.html");
|
|
writeFileSync(htmlPath, "<h1>Design Spec</h1><p>Body</p>");
|
|
|
|
const deps = makeDeps();
|
|
deps.directory = projectRoot;
|
|
|
|
await handleAnnotateCommand(
|
|
{ properties: { arguments: "\"docs/Design Spec.html\"" } },
|
|
deps,
|
|
);
|
|
|
|
expect(startAnnotateServerMock).toHaveBeenCalledTimes(1);
|
|
const options = startAnnotateServerMock.mock.calls[0]?.[0];
|
|
expect(options.filePath).toBe(htmlPath);
|
|
expect(options.mode).toBe("annotate");
|
|
expect(options.pasteApiUrl).toBe("https://paste.example.test");
|
|
expect(options.shareBaseUrl).toBe("https://share.example.test");
|
|
expect(options.markdown).toBe("");
|
|
expect(options.rawHtml).toContain("<h1>Design Spec</h1>");
|
|
expect(options.renderHtml).toBe(true);
|
|
expect(options.convertHtml).toBe(false);
|
|
expect(options.sourceConverted).toBe(false);
|
|
});
|
|
|
|
test("--markdown converts HTML paths via Turndown", async () => {
|
|
const projectRoot = makeTempDir();
|
|
const docsDir = path.join(projectRoot, "docs");
|
|
mkdirSync(docsDir, { recursive: true });
|
|
const htmlPath = path.join(docsDir, "Design Spec.html");
|
|
writeFileSync(htmlPath, "<h1>Design Spec</h1><p>Body</p>");
|
|
|
|
const deps = makeDeps();
|
|
deps.directory = projectRoot;
|
|
|
|
await handleAnnotateCommand(
|
|
{ properties: { arguments: "\"docs/Design Spec.html\" --markdown" } },
|
|
deps,
|
|
);
|
|
|
|
expect(startAnnotateServerMock).toHaveBeenCalledTimes(1);
|
|
const options = startAnnotateServerMock.mock.calls[0]?.[0];
|
|
expect(options.filePath).toBe(htmlPath);
|
|
expect(options.markdown).toContain("# Design Spec");
|
|
expect(options.rawHtml).toBeUndefined();
|
|
expect(options.renderHtml).toBe(false);
|
|
expect(options.convertHtml).toBe(true);
|
|
expect(options.sourceConverted).toBe(true);
|
|
});
|
|
|
|
test("supports quoted folder paths and opens annotate-folder mode", async () => {
|
|
const projectRoot = makeTempDir();
|
|
const folderPath = path.join(projectRoot, "docs", "Specs Folder");
|
|
mkdirSync(folderPath, { recursive: true });
|
|
writeFileSync(path.join(folderPath, "plan.md"), "# Plan\n");
|
|
|
|
const deps = makeDeps();
|
|
deps.directory = projectRoot;
|
|
|
|
await handleAnnotateCommand(
|
|
{ properties: { arguments: "\"docs/Specs Folder\"" } },
|
|
deps,
|
|
);
|
|
|
|
expect(startAnnotateServerMock).toHaveBeenCalledTimes(1);
|
|
const options = startAnnotateServerMock.mock.calls[0]?.[0];
|
|
expect(options.filePath).toBe(folderPath);
|
|
expect(options.folderPath).toBe(folderPath);
|
|
expect(options.mode).toBe("annotate-folder");
|
|
expect(options.pasteApiUrl).toBe("https://paste.example.test");
|
|
expect(options.markdown).toBe("");
|
|
});
|
|
});
|
|
|
|
describe("handleAnnotateLastCommand", () => {
|
|
test("returns approved feedback and advertises support for an active session", async () => {
|
|
const deps: any = makeDeps();
|
|
deps.client.session.messages = mock(async (_input: unknown) => ({
|
|
data: [
|
|
{
|
|
info: { role: "assistant" },
|
|
parts: [{ type: "text", text: "Latest assistant message" }],
|
|
},
|
|
],
|
|
}));
|
|
deps.startAnnotateServer = mock(async (options: any) => ({
|
|
port: 0,
|
|
url: "http://localhost",
|
|
isRemote: false,
|
|
options,
|
|
waitForDecision: async () => ({
|
|
approved: true,
|
|
feedback: "Retain this caveat.",
|
|
annotations: [{ id: "a1" }],
|
|
}),
|
|
stop: () => {},
|
|
}));
|
|
|
|
const outcome = await handleAnnotateLastCommand(
|
|
{ properties: { sessionID: "session-123", arguments: "--gate" } },
|
|
deps,
|
|
);
|
|
|
|
expect(deps.startAnnotateServer.mock.calls[0]?.[0].approvalNotesSupported).toBe(true);
|
|
expect(outcome).toEqual({
|
|
approved: true,
|
|
feedback: "Retain this caveat.",
|
|
});
|
|
});
|
|
|
|
test("forwards pasteApiUrl for annotate-last sessions", async () => {
|
|
const deps = makeDeps();
|
|
deps.client.session.messages = mock(async (_input: unknown) => ({
|
|
data: [
|
|
{
|
|
info: { role: "assistant" },
|
|
parts: [{ type: "text", text: "Latest assistant message" }],
|
|
},
|
|
],
|
|
}));
|
|
|
|
await handleAnnotateLastCommand(
|
|
{ properties: { sessionID: "session-123" } },
|
|
deps,
|
|
);
|
|
|
|
expect(startAnnotateServerMock).toHaveBeenCalledTimes(1);
|
|
const options = startAnnotateServerMock.mock.calls[0]?.[0];
|
|
expect(options.mode).toBe("annotate-last");
|
|
expect(options.filePath).toBe("last-message");
|
|
expect(options.pasteApiUrl).toBe("https://paste.example.test");
|
|
expect(options.markdown).toBe("Latest assistant message");
|
|
});
|
|
});
|