Skip to content

fix(annotate): tolerate non-path tokens in annotate arguments - #1185

Closed
technicalpickles wants to merge 1 commit into
backnotprop:mainfrom
technicalpickles:tolerant-annotate-args
Closed

fix(annotate): tolerate non-path tokens in annotate arguments#1185
technicalpickles wants to merge 1 commit into
backnotprop:mainfrom
technicalpickles:tolerant-annotate-args

Conversation

@technicalpickles

Copy link
Copy Markdown

Closes #1182.

Takes the direction from your reply: tolerant resolution in the CLI's argument handling rather than the skill template, so every host gets it at once and the bang prefix stays exactly as #872 left it.

Summary

  • annotate resolves each whitespace-delimited token and proceeds only when exactly one names something annotatable, so trailing prose from a slash command stops being fatal.
  • Two or more resolving is an error listing both candidates, rather than guessing which was meant.
  • Nothing resolving reports what it tried and what the command accepts. That is the part that would have saved most of the retries behind /plannotator-annotate treats trailing natural language as a filename and fails #1182.
  • Strict surfaces keep their exit-code contract. --gate --json with --require-approval or --result-file still exits 2 on a typo and still takes its target from argv, so tolerant resolution never reaches them.

Resolution lives in packages/shared/annotate-target.ts so the Bun and Pi runtimes share one implementation, vendored through vendor.sh. All three hosts call it.

Behavior

$ plannotator annotate the aim doc
No annotate target found. Tried: the, aim, doc
plannotator annotate accepts a path, URL, or folder, e.g. docs/spec.md, ./docs/, or https://example.com/page.
(exit 1)

$ plannotator annotate README.md and give me the URL for it
(annotates README.md)

$ plannotator annotate README.md CONTRIBUTING.md
(reports an ambiguous target, lists both candidates, exits 1)
  README.md
  CONTRIBUTING.md
Pass exactly one path, URL, or folder.

The exact ambiguity header wording is pinned as a literal in packages/shared/annotate-target.test.ts.

How a token resolves

annotateTokenResolves() mirrors the branches already at the call sites, in order: https?:// prefix, then directory, then an existing .html/.htm, then resolveMarkdownFile() returning found or ambiguous. Ambiguous counts as resolved deliberately, so the existing "Ambiguous filename" error still fires with its in-project matches instead of being replaced by a vaguer hint. Everything routes through resolveAtReference, so @-references and the scoped-package literal fallback behave as before.

Cheap for prose tokens: resolveMarkdownFile rejects anything without an annotatable extension before walking the tree, so a word like the costs two failed stat calls. Flag-shaped tokens are never candidates, and identical tokens collapse so annotate spec.md spec.md is not an ambiguity.

Test plan

bun run typecheck is clean. The three behaviors above were exercised against the real entry point, not just unit tests.

packages/shared/annotate-target.test.ts covers resolution, and apps/hook/server/strict-annotate-result.test.ts pins the strict contract, including that a typo under --gate --json --require-approval and under --gate --json --result-file still exits 2, and that the same argv without a strict flag gets tolerant behavior. Error text is asserted as literal strings rather than against a shared constant, so rewording the message fails the test.

bun test has three failures in apps/pi-extension/server.test.ts that predate this branch and reproduce on a sibling branch off the same base: a global gpg.format=ssh config leaks into the test's temp HOME and git cannot write a commit object. That file imports only ./server.ts plus generated modules.

Note

This branch and #1184 both touch apps/hook/server/index.ts, in different regions. git merge-tree reports a clean merge, so they can land in either order.

The host slash commands invoke the CLI through a bash-substitution prefix
(!`plannotator annotate $ARGUMENTS`), so $ARGUMENTS reaches argv unquoted
and unparsed. Any trailing natural language was fatal:

  /plannotator-annotate and give me the URL for it  ->  File not found: and
  /plannotator-annotate the aim doc                 ->  File not found: the

The bang prefix is deliberate (backnotprop#872), so the fix belongs in argument
resolution rather than the skill templates: every host gets the forgiving
behavior at once.

resolveAnnotateTargetArg in packages/shared/annotate-target.ts is now the
single decision, shared by the Claude Code binary, OpenCode, and Pi, so the
behavior and both new error messages are identical everywhere. The rule is
conservative: resolve every candidate token and proceed only when exactly one
resolves to a path, URL, or folder. Two resolving tokens are ambiguous and
error naming both, never guessing and never picking the first. Nothing
resolving errors naming what was tried plus the accepted shapes, which is the
message that tells a user their argument shape was the real problem.
Flag-shaped tokens are never candidates. Hosts that receive the remainder
pre-joined try the un-split string first, so an unquoted path containing
spaces still wins over its own tokens.

Two cases deliberately keep the existing, more specific error: a single
unresolvable token (File not found: typo.md) and an argument that does exist
but is not annotatable (File type not supported: .pdf).

Tolerance is bypassed for strict invocations (--require-approval /
--result-file) via isStrictAnnotateInvocation, the same predicate that picks
the startup-failure exit code. Those own an exit-code contract: a typo keeps
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.

Closes backnotprop#1182
@backnotprop

Copy link
Copy Markdown
Owner

First: this collision is our fault, and I am sorry for the wasted work. We offered you the issue in the triage reply, then picked it up ourselves the same day without posting a claim on the thread before starting. You did everything right, and the implementation shows it: you landed on the same architecture we did independently, down to the shared packages/shared/annotate-target.ts module vendored to both runtimes, exactly-one-token resolution, ambiguity as an error naming candidates, and the strict-gate exit-code contract preserved. That is strong validation that the design direction was right.

The fix that merged this morning as #1183 covers the same ground with one addition worth explaining, because it changes the failure mode your PR would still have had. While building it we tested how Claude Code handles a !-prefixed skill command that exits non-zero: it aborts the prompt before the model runs at all. So an improved error message that exits 1, like the "No annotate target found. Tried: ..." path here, never reaches the agent on the exact host #1182 was filed against; the user sees the skill do nothing, which is the original invisible failure with better words. #1183 therefore prints an agent-addressed handoff on stdout and exits 0 in plain mode (the only combination that lands in the model's context), so the agent interprets the natural language and re-runs with the concrete path. Machine surfaces (--json, --hook, strict gates) keep their existing codes and clean streams.

I am closing this as superseded by #1183, with two genuine asks: if you spot anything in your implementation that handles a case ours misses, say so here or on a fresh issue and we will port it, and we will credit you alongside the #1182 report in the release notes for this fix, since you built a correct solution in parallel. Your #1184 remains very much alive and is the one we are excited about; the two review items there are small.

@backnotprop

Copy link
Copy Markdown
Owner

Promised follow-up: we did a full comparison of your implementation against what merged, and your branch caught real things. #1187 is now open porting five items from it, with you credited as co-author on the commit:

  1. Your URL probe was right and ours was wrong: we tested the raw token against the URL regex while our own pipeline strips @-prefixes and quotes first, so a multi-token invocation like annotate @https://example.com/page and summarize it fell through to the handoff instead of opening the URL. Your unwrap-first approach is now on main.
  2. Your test fixture covered four behaviors we document but never verified: absolute paths, the wider plain-text extension set, the scoped-package literal-@ fallback, and whole-string-beats-its-own-tokens. All ported.
  3. Your isStrictAnnotateInvocation factoring replaced our duplicated strict predicate, which was drift risk on a security-relevant check.
  4. Your source-scan guard for the strict-ambiguity leak and your AGENTS.md warning about fix(install): restore /plannotator-* bash execution on Claude Code + harden unattended installs #872 and the skill-template layer both landed too.

Where the designs diverged (flag tolerance, bare directories in multi-token mode), we kept the merged behavior deliberately: unknown flags disable tolerance so a typo like --no-jna cannot silently route a private URL through Jina, and bare directory tokens cannot hijack resolution in multi-token mode. Happy to explain either call if you are curious.

A shipped bug found, four coverage gaps closed, and a cleaner factoring adopted is a substantive contribution from a branch that never merged. It will read that way in the release notes as well.

backnotprop added a commit that referenced this pull request Aug 3, 2026
…overage (#1187)

Ports five small items from the closed parallel PR #1185 into the
tolerant annotate argument resolution that landed in #1183 (#1182):

- Bug fix: the token probe tested the raw token against the URL regex,
  but the pipeline strips the @ reference marker and wrapping quotes
  first, so a multi-token 'annotate @https://example.com/page and
  summarize it' probed to nothing and emitted the handoff instead of
  opening the URL. The probe now unwraps with stripAtPrefix before the
  regex and returns the unwrapped form (the pipeline re-strips
  harmlessly). Tests cover @-prefixed and quote-wrapped URLs as
  multi-token candidates.
- Test ports: absolute-path candidate, the wider plain-text set (.txt,
  .yaml) guarding ANNOTATABLE_DOC_REGEX breadth, the scoped-package
  literal-@ fallback against a real @scope/ directory, and the
  whole-un-split-string preference over its own tokens ('Meeting
  Notes.md' wins over a resolving 'Notes.md' token) covering
  annotateInputNamesExistingTarget.
- Defensive scan: the strict-mode source-scan test now asserts the
  annotate startup block gates tolerance on !strictAnnotate via
  isStrictAnnotateInvocation, since an inverted gate cannot be
  spawn-tested without starting a server.
- DRY: the strict predicate was defined twice (strict-annotate-result
  exit-code helper and the index.ts tolerance bypass). Extracted
  isStrictAnnotateInvocation with a StrictAnnotateFlags type; both
  sites use it so the exit-code path and the tolerance bypass can
  never drift. Behavior byte-identical; existing subprocess tests
  unchanged.
- Docs: the tolerant-resolution section now cites #872 (commit
  aac5aac) for why the bang prefix is deliberate and states that
  argument-shape issues belong in the CLI's resolution, not the skill
  templates.

Refs #1185, #1182

Co-authored-by: Josh Nichols <josh.nichols+agent@gusto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/plannotator-annotate treats trailing natural language as a filename and fails

2 participants