Skip to content

fix(annotate): recognize wrapped URLs in token probe and port #1185 coverage - #1187

Merged
backnotprop merged 1 commit into
mainfrom
fix/1185-ports
Aug 3, 2026
Merged

fix(annotate): recognize wrapped URLs in token probe and port #1185 coverage#1187
backnotprop merged 1 commit into
mainfrom
fix/1185-ports

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

Ports five small items from @technicalpickles' closed parallel implementation (#1185) into the tolerant annotate argument resolution that landed via #1183 (#1182). Full credit to @technicalpickles for catching the URL-probe bug and the coverage gaps; a comparison pass of the two implementations surfaced these.

1. Bug fix: wrapped URLs were not URL candidates

probeAnnotateToken tested the raw token against /^https?:\/\//i, but the resolution pipeline strips the @ reference marker and wrapping quotes before its own URL check (stripAtPrefix unwraps both). So the multi-token invocation

plannotator annotate @https://example.com/page and summarize it

probed to zero candidates and emitted the tier-3 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. Verified by unit tests (@-prefixed and quote-wrapped URLs, single and multi-token) and a manual smoke: the invocation above now reaches Fetching: https://example.com (tier 1), while annotate the aim doc still hands off and a strict typo still exits 2.

2. Test ports

Added to packages/shared/annotate-target.test.ts from their suite's coverage: absolute paths resolve as candidates; the wider plain-text set (.txt, .yaml) resolves, guarding ANNOTATABLE_DOC_REGEX breadth (which the probe's no-walk cheapness argument relies on); the scoped-package literal-@ fallback resolves against a real @scope/ directory (the strip half was covered, the fallback half was not); and the whole un-split string is preferred over its own tokens (Meeting Notes.md wins over a resolving Notes.md token), covering annotateInputNamesExistingTarget in the OpenCode/Pi pre-pass direction.

3. Defensive source scan

The strict bypass cannot be spawn-tested in its inverted form (tolerance applying in strict mode would require a live server to observe). The existing source-scan test in apps/hook/server/strict-annotate-result.test.ts now also asserts the annotate startup block gates tolerance on !strictAnnotate computed via isStrictAnnotateInvocation, so an inverted or bypassed gate fails the scan.

4. DRY: single strict predicate

The strict predicate (requireApproval || !!resultFile) was defined twice: in annotateStartupFailureExitCode and inline in index.ts for the tolerance bypass. Adopted their factoring: a StrictAnnotateFlags type plus isStrictAnnotateInvocation in strict-annotate-result.ts, used by both the exit-code helper and the bypass, so the two paths can never drift. Behavior is byte-identical; the existing subprocess tests in annotate-cli.test.ts pass unchanged.

5. Docs

The AGENTS.md tolerant-resolution section now cites #872 (commit aac5aacb, "restore /plannotator-* bash execution on Claude Code") for why the bang prefix is deliberate, and states that argument-shape issues belong in the CLI's resolution rather than the skill templates.

Verified

  • bun run typecheck green; full bun test green (2709 pass, 0 fail); bash apps/pi-extension/vendor.sh regenerates cleanly.
  • Manual smoke: annotate @https://example.com and summarize --no-jina resolves the URL (tier 1, fetch reached); annotate the aim doc still hands off with exit 0; annotate nope-not-here.md --gate --json --require-approval still exits 2 with File not found on stderr.

Refs #1185, #1182. Co-authored with Josh Nichols (@technicalpickles).

…overage

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>
@backnotprop

Copy link
Copy Markdown
Owner Author

Review (at 6de25caa)

Verdict: merge as-is. Full suite 2709 pass / 0 fail, typecheck clean, vendor parity confirmed (regenerated copy carries the probe fix and remains in the Pi pack).

The URL fix is a real user-facing repair, measured against base: annotate @https://.../page and summarize it went from a tier-3 handoff to fetching the URL, quoted URLs likewise, with the round-trip through the pipeline verified (the later re-strip is a no-op on the unwrapped value) and zero regressions across schemeless tokens, the @file strip half, the @scope/ literal fallback, and sole-token invocations.

The ported tests were classified by running the new file against pre-PR source: exactly the two wrapped-URL tests fail on main (regression tests), the other four pass there (coverage ports, correctly labelled). The Meeting Notes.md fixture rename holds up as platform-safe on both case-sensitive and case-insensitive filesystems.

The strict-predicate extraction is behavior-identical in every case including empty-string resultFile, and both mutations are caught: inverting the tolerance gate fails the new source-scan assertion plus five subprocess tests, and re-inlining the predicate fails the scan. The AGENTS.md #872 citation is faithful to commit aac5aacb.

One cosmetic note for a future pass: the source-scan toContain assertions run against the whole annotate block, so a failure dumps ~9KB into test output and the assertions are brittle to a rename of strictAnnotate; a scoped regex would keep failures readable. The two known low trade-offs from #1183 (unknown-flag short-circuit bypassing tier-2 ambiguity, OpenCode/Pi joined-string quoting hazard) are unchanged by this PR and remain on record.

Co-authored-by trailer verified well-formed and parsing in git.

@backnotprop
backnotprop merged commit 46f1e8d into main Aug 3, 2026
14 checks passed
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.

1 participant