Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,16 @@ User annotates content, provides feedback
Send Annotations → feedback sent to agent session
```

### Tolerant annotate target resolution

The host slash commands invoke the CLI through a bash-substitution prefix (`` !`plannotator annotate $ARGUMENTS` ``), so `$ARGUMENTS` reaches argv unquoted and unparsed and trailing natural language used to be fatal (`/plannotator-annotate the aim doc` → `File not found: the`). Argument resolution is therefore tolerant, in `packages/shared/annotate-target.ts` (`resolveAnnotateTargetArg`) and shared by all three hosts — the Claude Code binary, OpenCode, and Pi — so the behavior and both error messages are identical everywhere. The bang prefix is deliberate (#872) and the skill templates are **not** the place to fix this.

The rule is conservative: every candidate token is resolved and the command proceeds only when **exactly one** resolves to a path, URL, or folder. Two resolving tokens are ambiguous and error naming both — never guess, never pick the first. Nothing resolving errors naming what was tried plus the accepted shapes, which is the message that tells a user their slash-command argument shape was the real problem. Flag-shaped tokens are never candidates. Hosts that receive the remainder pre-joined (OpenCode, Pi) try the un-split string first, so an unquoted path containing spaces still wins over its own tokens.

Two cases deliberately stay with the caller's existing, more specific error: a single unresolvable token (`File not found: typo.md`) and an argument that does exist but isn't annotatable (`File type not supported: .pdf`).

**Tolerance is bypassed for strict invocations** (`--require-approval` / `--result-file` — see `isStrictAnnotateInvocation` in `apps/hook/server/strict-annotate-result.ts`, the same predicate that picks the startup-failure exit code). Those own an exit-code contract: a typo must keep exiting 2, and quietly annotating a later argument because the first one was a typo would let a gate publish `approved` for a document the caller never named.

### Strict direct annotate results

Direct `plannotator annotate` invocations may add `--require-approval` and/or `--result-file <path>` only with `--gate --json`; both reject `--hook` and are not shared with OpenCode/Pi slash-command parsing. Legacy plaintext, JSON, hook, and exit behavior remains unchanged when neither strict option is present.
Expand Down
25 changes: 24 additions & 1 deletion apps/hook/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ import {
type GoalSetupStage,
} from "@plannotator/shared/goal-setup";
import { stripAtPrefix, resolveAtReference } from "@plannotator/shared/at-reference";
import {
annotateInputExists,
annotateTokenResolves,
resolveAnnotateTargetArg,
} from "@plannotator/shared/annotate-target";
import { htmlToMarkdown } from "@plannotator/shared/html-to-markdown";
import { urlToMarkdown, isConvertedSource } from "@plannotator/shared/url-to-markdown";
import { createWorktreePool, type WorktreePool, type PoolEntry } from "@plannotator/shared/worktree-pool";
Expand Down Expand Up @@ -161,6 +166,7 @@ import { completeAnnotateCommand } from "./annotate-command";
import {
annotateStartupFailureExitCode,
assertResultPathAvailable,
isStrictAnnotateInvocation,
resolveResultFilePath,
STRICT_GATE_ERROR_EXIT_CODE,
} from "./strict-annotate-result";
Expand Down Expand Up @@ -1000,7 +1006,7 @@ if (args[0] === "sessions") {
);
}

const rawFilePath = args[1];
let rawFilePath = args[1];
if (!rawFilePath) {
exitAnnotateStartupFailure("Usage: plannotator annotate <file.md | file.txt | file.html | https://... | folder/> [--markdown] [--no-jina] [--gate] [--json] [--hook] [--require-approval] [--result-file <path>]");
}
Expand All @@ -1013,6 +1019,23 @@ if (args[0] === "sessions") {
// Use PLANNOTATOR_CWD if set (original working directory before script cd'd)
const projectRoot = process.env.PLANNOTATOR_CWD || process.cwd();

// Tolerant target selection: the host slash commands substitute
// `$ARGUMENTS` unquoted, so trailing prose lands in argv as extra tokens.
// Strict invocations bypass this and keep their exit-code contract — see
// resolveAnnotateTargetArg.
const annotateTarget = resolveAnnotateTargetArg({
raw: rawFilePath,
tokens: args.slice(1),
strict: isStrictAnnotateInvocation({ requireApproval: requireApprovalFlag, resultFile }),
resolves: (token) => annotateTokenResolves(token, projectRoot),
inputExists: (input) => annotateInputExists(input, projectRoot),
});
if (annotateTarget.kind === "error") {
exitAnnotateStartupFailure(annotateTarget.message);
}
rawFilePath = annotateTarget.token;
filePath = stripAtPrefix(rawFilePath);

if (resultFile) {
try {
await assertResultPathAvailable(resultFile);
Expand Down
152 changes: 152 additions & 0 deletions apps/hook/server/strict-annotate-result.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,17 @@ import {
import { readFileSync } from "node:fs";
import { join } from "node:path";
import { tmpdir } from "node:os";
import {
annotateInputExists,
annotateTokenResolves,
resolveAnnotateTargetArg,
} from "@plannotator/shared/annotate-target";
import { parseStrictAnnotateOptions } from "./cli";
import {
annotateOutcomeExitCode,
annotateStartupFailureExitCode,
assertResultPathAvailable,
isStrictAnnotateInvocation,
resolveResultFilePath,
serializeStrictAnnotateResult,
STRICT_GATE_ERROR_EXIT_CODE,
Expand Down Expand Up @@ -178,6 +185,151 @@ describe("annotate startup failure exit codes", () => {
});
});

describe("tolerant annotate target resolution vs the strict exit-code contract", () => {
/**
* Replay exactly the wiring `index.ts` uses for the annotate branch:
* parse the strict options off argv, ask whether the invocation is strict,
* resolve the target, and — when nothing usable came back — pick the
* startup-failure exit code from those same strict flags.
*
* Nothing here spawns the binary: `index.ts` imports the bundled
* `../dist/*.html`, which CI's `bun test` never builds. So the contract is
* asserted over the real production units in the real order instead.
*/
function annotateInvocation(argv: string[], projectRoot: string) {
const parsed = parseStrictAnnotateOptions(argv);
const args = parsed.remainingArgs.filter(
(arg) => arg !== "--gate" && arg !== "--json",
);
const rawFilePath = args[1];
const decision = resolveAnnotateTargetArg({
raw: rawFilePath,
tokens: args.slice(1),
strict: isStrictAnnotateInvocation(parsed),
resolves: (token) => annotateTokenResolves(token, projectRoot),
inputExists: (input) => annotateInputExists(input, projectRoot),
});
return {
strict: isStrictAnnotateInvocation(parsed),
decision,
startupFailureExitCode: annotateStartupFailureExitCode(parsed),
};
}

async function projectWithOneDocument(): Promise<string> {
const directory = await makeTemporaryDirectory();
await writeFile(join(directory, "spec.md"), "# Spec\n", "utf8");
return directory;
}

test("a typo under --gate --json --require-approval still exits 2", async () => {
const projectRoot = await projectWithOneDocument();
const { strict, decision, startupFailureExitCode } = annotateInvocation(
["annotate", "typo.md", "--gate", "--json", "--require-approval"],
projectRoot,
);

expect(strict).toBe(true);
// Tolerance is bypassed: the target is what was typed, verbatim.
expect(decision).toEqual({ kind: "target", token: "typo.md" });
// …and it does not resolve, so the startup-failure path is the one taken.
expect(annotateTokenResolves("typo.md", projectRoot)).toBe(false);
expect(startupFailureExitCode).toBe(STRICT_GATE_ERROR_EXIT_CODE);
});

test("a typo under --gate --json --result-file still exits 2", async () => {
const projectRoot = await projectWithOneDocument();
const { strict, decision, startupFailureExitCode } = annotateInvocation(
[
"annotate",
"typo.md",
"--gate",
"--json",
"--result-file",
join(projectRoot, "result.json"),
],
projectRoot,
);

expect(strict).toBe(true);
expect(decision).toEqual({ kind: "target", token: "typo.md" });
expect(startupFailureExitCode).toBe(STRICT_GATE_ERROR_EXIT_CODE);
});

test("strict never annotates a different argument than the one it was given", async () => {
const projectRoot = await projectWithOneDocument();
// `spec.md` is real and would win under tolerant resolution. A gate that
// silently reviewed it would publish "approved" for a document the caller
// never named, so strict must keep failing on `typo.md`.
const { decision, startupFailureExitCode } = annotateInvocation(
["annotate", "typo.md", "spec.md", "--gate", "--json", "--require-approval"],
projectRoot,
);

expect(decision).toEqual({ kind: "target", token: "typo.md" });
expect(startupFailureExitCode).toBe(STRICT_GATE_ERROR_EXIT_CODE);
});

test("strict does not error on ambiguity either — it just uses argv[1]", async () => {
const projectRoot = await projectWithOneDocument();
await writeFile(join(projectRoot, "notes.md"), "# Notes\n", "utf8");
const { decision } = annotateInvocation(
["annotate", "spec.md", "notes.md", "--gate", "--json", "--require-approval"],
projectRoot,
);

expect(decision).toEqual({ kind: "target", token: "spec.md" });
});

test("the same argv without a strict flag gets the tolerant behavior", async () => {
const projectRoot = await projectWithOneDocument();

// Prose around a real file resolves to the file.
expect(
annotateInvocation(
["annotate", "the", "aim", "spec.md", "--gate", "--json"],
projectRoot,
),
).toMatchObject({
strict: false,
decision: { kind: "target", token: "spec.md" },
startupFailureExitCode: 1,
});

// Nothing usable errors with the shape hint instead of "File not found: the".
const noTarget = annotateInvocation(
["annotate", "the", "aim", "doc"],
projectRoot,
);
expect(noTarget.decision.kind).toBe("error");
if (noTarget.decision.kind !== "error") throw new Error("unreachable");
expect(noTarget.decision.message).toContain("Tried: the, aim, doc");
expect(noTarget.decision.message).toContain("path, URL, or folder");
});

test("index.ts gates tolerant resolution on the strict predicate", () => {
// The bypass has to be wired in the annotate startup block itself; a
// tolerant path that forgot `strict` would pass every unit test above
// and still break the gate contract in production.
const source = readFileSync(join(import.meta.dir, "index.ts"), "utf8");
const start = source.indexOf('} else if (args[0] === "annotate") {');
const end = source.indexOf(
'} else if (args[0] === "annotate-last" || args[0] === "last") {',
start,
);
expect(start).toBeGreaterThan(-1);
expect(end).toBeGreaterThan(start);

const annotateStartupBlock = source.slice(start, end);
const call = annotateStartupBlock.indexOf("resolveAnnotateTargetArg({");
expect(call).toBeGreaterThan(-1);
const callArgs = annotateStartupBlock.slice(call, call + 600);
expect(callArgs).toContain("strict: isStrictAnnotateInvocation({");
expect(callArgs).toContain("requireApproval: requireApprovalFlag");
expect(callArgs).toContain("resultFile");
});
});

describe("atomic annotate result publication", () => {
test("resolves relative result paths from the invocation directory", () => {
expect(
Expand Down
32 changes: 25 additions & 7 deletions apps/hook/server/strict-annotate-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,27 @@ export interface AnnotateOutcome {
*/
export const STRICT_GATE_ERROR_EXIT_CODE = 2;

export interface StrictAnnotateFlags {
requireApproval: boolean;
resultFile?: string;
}

/**
* True when the invocation carries a strict flag (`--require-approval` /
* `--result-file`, neither of which the CLI accepts without `--gate --json`).
*
* Strict invocations own the exit-code contract below, which is why tolerant
* annotate argument resolution is bypassed for them: quietly annotating a
* later argument because the first one was a typo would let a gate publish
* "approved" for a document the caller never named. A typo must keep exiting
* 2 here.
*/
export function isStrictAnnotateInvocation(
flags: StrictAnnotateFlags,
): boolean {
return flags.requireApproval || !!flags.resultFile;
}

/**
* Exit code for an annotate startup failure (missing path, unreachable URL,
* empty folder, ambiguous name, missing file, oversized file).
Expand All @@ -32,13 +53,10 @@ export const STRICT_GATE_ERROR_EXIT_CODE = 2;
* "the reviewer did not approve", so a startup failure must exit with the gate
* error code instead — otherwise automation reads a typo'd path as a rejection.
*/
export function annotateStartupFailureExitCode(strict: {
requireApproval: boolean;
resultFile?: string;
}): number {
return strict.requireApproval || strict.resultFile
? STRICT_GATE_ERROR_EXIT_CODE
: 1;
export function annotateStartupFailureExitCode(
strict: StrictAnnotateFlags,
): number {
return isStrictAnnotateInvocation(strict) ? STRICT_GATE_ERROR_EXIT_CODE : 1;
}

export function serializeStrictAnnotateResult(
Expand Down
30 changes: 28 additions & 2 deletions apps/opencode-plugin/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ import { resolveMarkdownFile, resolveUserPath, hasMarkdownFiles, ANNOTATABLE_DOC
import { FILE_BROWSER_EXCLUDED } from "@plannotator/shared/reference-common";
import { htmlToMarkdown } from "@plannotator/shared/html-to-markdown";
import { parseAnnotateArgs } from "@plannotator/shared/annotate-args";
import {
annotateInputExists,
annotateTokenResolves,
resolveAnnotateTargetArg,
} from "@plannotator/shared/annotate-target";
import { stripAtPrefix } from "@plannotator/shared/at-reference";
import { parseReviewArgs } from "@plannotator/shared/review-args";
import { urlToMarkdown, isConvertedSource } from "@plannotator/shared/url-to-markdown";
import { buildLocalWorkspaceReview, type WorkspaceDiffType } from "@plannotator/server/review-workspace";
Expand Down Expand Up @@ -218,7 +224,9 @@ export async function handleAnnotateCommand(
// --json is accepted silently (OpenCode writes to session, not stdout).
// parseAnnotateArgs strips leading @ on filePath (reference-mode convention).
// `rawFilePath` preserves it for the scoped-package markdown fallback.
const { filePath, rawFilePath, gate, renderMarkdown: renderMarkdownFlag, noJina } = parseAnnotateArgs(rawArgs);
const parsedAnnotateArgs = parseAnnotateArgs(rawArgs);
const { gate, renderMarkdown: renderMarkdownFlag, noJina } = parsedAnnotateArgs;
let { filePath, rawFilePath } = parsedAnnotateArgs;
// @ts-ignore - Event properties contain sessionID
const sessionId = event.properties?.sessionID;

Expand All @@ -227,6 +235,25 @@ export async function handleAnnotateCommand(
return;
}

// Tolerant target selection: the slash command hands us whatever the user
// typed, so trailing prose ("the aim doc") used to be fatal. The un-split
// string is tried first, so an unquoted path containing spaces still wins
// over its own tokens; only then do we sift per token.
const agentCwd = directory || process.cwd();
// No `strict` here — OpenCode writes decisions back into the session and
// owns no exit-code contract.
const annotateTarget = resolveAnnotateTargetArg({
raw: rawFilePath,
resolves: (token) => annotateTokenResolves(token, agentCwd),
inputExists: (input) => annotateInputExists(input, agentCwd),
});
if (annotateTarget.kind === "error") {
client.app.log({ level: "error", message: annotateTarget.message });
return;
}
rawFilePath = annotateTarget.token;
filePath = stripAtPrefix(rawFilePath);

let markdown: string;
let rawHtml: string | undefined;
let absolutePath: string;
Expand All @@ -235,7 +262,6 @@ export async function handleAnnotateCommand(
let isFolder = false;
let sourceInfo: string | undefined;
let sourceConverted = false;
const agentCwd = directory || process.cwd();

// --- URL annotation ---
const isUrl = /^https?:\/\//i.test(filePath);
Expand Down
Loading