Skip to content

feat(cli): state each local engine's reach in the routing recipe - #155

Open
thecodedrift wants to merge 4 commits into
mainfrom
fix/engine-capabilities
Open

feat(cli): state each local engine's reach in the routing recipe#155
thecodedrift wants to merge 4 commits into
mainfrom
fix/engine-capabilities

Conversation

@thecodedrift

@thecodedrift thecodedrift commented Aug 24, 2026

Copy link
Copy Markdown
Member

Stack (root → tip):

An agent reading route.txt could not tell whether a language was buildable locally or had to escalate to runtime, which needs a login. The reporter's case: two GitHub Actions YAML rules were routed to runtime because nothing said whether ast-grep handles YAML.

It does. The pinned @ast-grep/cli@0.41.0 lists 26 languages including Yaml, and a real pr-workflow-no-branches-filter rule (language: Yaml) passes verify and test. Both Actions rules belong in create-sg-rule with no login.

Refs #151

Derived, not transcribed

There was no source of truth for either engine's reach. $defs.Language in the vendored schema is a bare string with no enum, verify never validates language, and detect --json reports the repository's languages in a different vocabulary. The binaries were the only authority, and route.txt said nothing.

Transcribing two lists into a .txt would drift on a version bump with nothing to catch it, and stale prose is worse than the current silence — silence at least makes the agent ask. So: pinned constants in src/rules/capabilities.ts, substituted into recipes as %(KEY)s, with vendor-contract tests asserting the constants still match the binaries. That is the pattern ast-grep-vendor-contract.test.ts already exists for — exact-pinned binary plus a test that fires on the bump.

capabilities.ts is deliberately import-free: prompts/recipes.ts imports it, and the assert-prompts-graph vite plugin fails the build if that chunk reaches a host capability.

Rejected alternative: extending detect --json with an engines block. cli-detect's spec frames detect as an offline repo scan, engine reach is knowable at build time, and it would cost a third subprocess turn in a recipe already spending two.

Vale's reach was measured, not read

Vale self-reports no format list, so every tier here comes from probing the shipped 3.17.1 binary — and the measurement corrected the starting assumptions in five places:

  • .css is comment-tier, not plaintext. The first probe said otherwise because it fed CSS a // comment, which is not one.
  • .sass is comment-tier; .scss is not.
  • Extension matching is case-sensitive and not closed over aliases.R is comment-aware, .PY/.JS/.TS are not; .cc/.cxx/.pyw/.pm are, .hh/.hxx are not.
  • Markup tier is wider than assumed (.htm, .xhtml).
  • --no-exit does not suppress the converter tier's E100 exit 2.

Each tier is pinned by its discriminating property, because on ordinary prose all three readable tiers look identical: markup by a construct only a parser skips, comment-tier by the negative (a bare non-comment line must yield zero), converter-tier by non-zero exit plus E100 plus the converter's name. A test also asserts the fixture map covers the markup constant exactly, so an unprobed entry cannot be added to a constant.

create-vale-rule.txt was teaching the crash

Its worked example globbed [*.{md,mdx}]. .mdx needs mdx2vast, which this CLI does not ship, and one such file fails the entire Vale pass — so any rule following that example silently disabled every other Vale rule. Now [*.{md,markdown}], with the point stated plainly: a matcher like that is not a wider [*.md], it is a broken one. (The guard that makes this non-fatal is in the PR above this one.)

A correction, included deliberately

The last commit walks back a claim this branch originally shipped. Both the recipe and the constants said a plausible C++ or yaml "passes verify and fails inside the binary." Measured at 0.41.0, C++ and cpp are accepted aliases that resolve to Cpp, so an off-list spelling is not reliably an error.

The two failures that are real, neither caught locally: a name ast-grep does not recognize (C# for CSharp) aborts config parsing, taking every other rule's report down with it; and a valid name for the wrong parser (TypeScript over a .tsx tree) reports nothing and reads as a clean codebase. The advice to copy from the list is unchanged; the reason for it is now the accurate one. The vendor-contract assertions were already right — they pin the canonical list, not what the binary rejects — but their comment said otherwise.

Also

route.txt now states that a language on neither list does not route to runtime by default: create-legacy-rule comes first, since the repo's own linter may already speak it. And a new note separates reach (a property of the pinned version) from availability (a property of the host), which the existing "availability you can assert" bullet was conflating while giving the agent no way to assert it.

OpenSpec delta on cli-rule-routing — engine reach becomes a fourth thing the recipe is specified to state, with the normative core being that it is derived from the pinned versions rather than transcribed. Not archived; this is not the tip of the stack.

thecodedrift and others added 2 commits August 23, 2026 21:24
`route` chose an engine from the shape of the evidence alone and said
nothing about language reach, so a rule over a GitHub Actions workflow
was escalated to `create-runtime-rule` — which needs a login — because
nothing stated that ast-grep parses YAML. It does; `Yaml` is one of the
26 languages the pinned `@ast-grep/cli@0.41.0` lists in `sg run -h`.

Nothing in the repo could have answered that. The vendored ast-grep
schema types `$defs.Language` as a bare string with no enum, `verify`
never validates a rule's `language`, and `detect --json` reports the
repository's own languages in a different vocabulary. Vale self-reports
nothing at all, so its reach was measured by probing the shipped 3.17.1
binary tier by tier.

A new `src/rules/capabilities.ts` carries both engines' reach as pure
data pinned to the version it came from, and `buildVariables` renders it
into `route.txt` (topic v2) and `create-vale-rule.txt` (topic v3) as
`%(...)s` substitutions rather than prose typed into a `.txt`. Two
vendor-contract blocks pin the constants to the binaries, so an engine
bump that changes what a binary parses fails a test instead of leaving a
confident wrong sentence in front of an agent.

Also names Vale's converter-dependent formats and what they cost: Vale
supports reStructuredText, AsciiDoc, XML, DITA and MDX by shelling out
to a converter this CLI does not ship, and one such file exits 2 with an
`E100` and abandons the whole run. `create-vale-rule` had been offering
`[*.{md,mdx}]` as its example of widening a matcher.

Refs #151

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
The recipe and the constants both claimed a plausible `C++` or `yaml`
passes `verify` and fails inside the binary. Measured against the pinned
0.41.0: `C++` and `cpp` are accepted aliases that resolve to Cpp, so an
off-list spelling is not reliably an error at all.

The two failures that are real, and neither is caught locally: a name
ast-grep does not recognize (`C#` for `CSharp`) aborts config parsing, so
every other rule goes unreported alongside it; and a valid name for the
wrong parser (`TypeScript` over a `.tsx` tree) reports nothing and reads
as a clean codebase. The advice to copy from the list is unchanged — the
reason for it is now the accurate one.

The assertions in the vendor contract were already correct: they pin the
canonical list, not what the binary rejects. Their comment said
otherwise and now says what they actually cover.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
Copilot AI lite review requested due to automatic review settings August 24, 2026 05:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Aug 24, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 13m 57s —— View job


Review complete

Review mode: incremental — read 1 prior review thread(s) before reviewing. .prior-review.json recorded reviewThreads: [], one review (Copilot's "unable to review — quota limit" message, not actual feedback), and one comment (the @claude /review trigger itself). There's nothing prior to classify as addressed/still-open, so this is effectively the first substantive review of this PR.

  • Read .prior-review.json and classify prior findings (none applicable)
  • Gather PR diff and context
  • Line-by-line scan of capabilities.ts, recipes.ts, route.txt, create-vale-rule.txt, and all four touched test files
  • Cross-file checks: pinned versions vs. package.json, assert-prompts-graph allow-list, buildVariables call sites, sprintf-js substitution safety
  • Removed-behavior audit (the [*.{md,mdx}][*.{md,markdown}] swap, topic version bumps, the "C++/cpp aliasing" correction commit)
  • Conventions check against root CLAUDE.md and .conventions/STYLEGUIDE-CODE.md
  • Posted findings

Summary

This is a well-scoped, carefully justified change. The core design — pinning engine reach as pure-data constants in src/rules/capabilities.ts, rendering them into route.txt/create-vale-rule.txt via %(KEY)s substitution, and backing each constant with a vendor-contract test that spawns the real binary — holds up. I verified several of the load-bearing claims directly rather than taking them on faith:

  • AST_GREP_VERSION (0.41.0) and VALE_VERSION (3.17.1) match what's pinned in packages/cli/package.json.
  • .markdown (the new worked example in create-vale-rule.txt) is genuinely in VALE_MARKUP_EXTENSIONS, so the replacement for the .mdx-trap example is accurate, not just less bad.
  • capabilities.ts has zero imports, so it can't trip assert-prompts-graph's host-capability check in vite.config.ts.
  • buildVariables() only runs once per CLI invocation (agent.ts, onboard.ts), so joining the ~26/~35-entry lists on every render is not a real perf concern.
  • No stray unescaped % was introduced into the recipe text that would confuse sprintf-js's %(KEY)s parsing.
  • No barrel-import violation: src/rules/ has no index.ts, so recipes.ts importing directly from ../rules/capabilities is correct per .conventions/STYLEGUIDE-CODE.md.

Two small things worth fixing, posted inline:

  1. packages/cli/test/ast-grep-vendor-contract.test.ts:633-634 — a comment points to a `the language field` test suite for the C#-aborts-parsing and wrong-parser-silent-failure cases. That suite doesn't exist anywhere in the repo (verified by grep) — the PR body itself says both failures are "neither caught locally." The pointer is dangling and could mislead a future reader into thinking those cases are covered when they aren't.
  2. packages/cli/src/rules/capabilities.ts:212-213VALE_CONVERTER_DEPENDENT_EXTENSIONS is exported but unused anywhere else in src/ or test/. Minor dead-code nit; flagging in case it's meant for fix(cli): require both fixture buckets for an ast-grep rule #156/fix(cli): stop one AsciiDoc file from disabling every Vale rule #157 further up the stack.

Neither is a functional bug — the recipe text, the substitution mechanism, and the two vendor-contract test suites (ast-grep set-equality, Vale tier-by-probe) all check out. Nice attention to detail in the Vale probing (measuring rather than assuming .css/.sass/.scss tiers, the case-sensitivity notes, the E100/--no-exit blast-radius framing) and in keeping create-vale-rule.txt's worked example from being the same trap it used to be.

Comment thread packages/cli/test/ast-grep-vendor-contract.test.ts
Comment thread packages/cli/src/rules/capabilities.ts
thecodedrift and others added 2 commits August 23, 2026 23:11
…pendent prose

The vendor contract asserted that Vale's `E100` text contains the converter
name, deriving the expected token from our own prose with
`converter.replace(/^an /, "").split(" ")[0]`. For `.xml` that yielded
"XSLT", which matched locally and failed in CI — the only failure across
all three stacked PRs.

The cause is that `.xml` fails two different ways. Vale reports
`xsltproc not found` where the program is absent and
`no XSLT transform provided` where it is present, and the split is by
platform: macOS ships /usr/bin/xsltproc, the Linux runner image does not.
So the assertion was reading host state through a vendor string while
presenting itself as a vendor contract.

It now asserts the checker tag — `[lintXML]`, `[lintAdoc]`, `[lintRST]`,
`[lintDITA]`, `[lintMDX]` — which is identical on both hosts, from a new
VALE_CONVERTER_CHECKERS map held to set-equality with the extension list.
The actionability claim the probe used to make moved to its own case,
asserted against our own data where no binary is involved.

Corrects the `.xml` entry while here. It is the one format whose converter
is not sufficient on its own: an XSLT transform is document-specific, so
there is no default to ship and installing xsltproc does not make `.xml`
lintable, where installing asciidoctor genuinely fixes `.adoc`. It now
names the stylesheet as well as the program, and that reaches the routing
recipe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
…ims them

A comment here pointed at a `the language field` suite that did not exist on
this branch — it was added one PR up the stack, so a maintainer following
the pointer found nothing and might have assumed the C#-abort and
wrong-parser behaviours were covered. Reported in review, and correct.

Moved down rather than the comment softened. `route.txt` makes both claims
on this branch, and the house rule is that an engine claim in a recipe is
quoted from a pinned binary rather than described, so the claim and its pin
belong in the same change.

Adds a third case while here, pinning the fact this branch's own history
got wrong: `C++` and `cpp` are ACCEPTED aliases resolving to Cpp, so an
off-list spelling is not reliably an error. Without it the suite reads as
"off-list spellings fail", which is the claim that had to be corrected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
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.

2 participants