Skip to content

test(ast-grep): make the ast-grep 0.45.3 upgrade land green - #337

Open
thecodedrift wants to merge 3 commits into
vendor/ast-grep/upgradefrom
vendor/ast-grep/upgrade-tests
Open

thecodedrift wants to merge 3 commits into
vendor/ast-grep/upgradefrom
vendor/ast-grep/upgrade-tests

Conversation

@thecodedrift

@thecodedrift thecodedrift commented Sep 15, 2026

Copy link
Copy Markdown
Member

Stack (root → tip):

#334 moves the @ast-grep/cli* pins to 0.45.3 and touches nothing else. This is what makes that bump correct.

What the pin bump alone left broken

  • AST_GREP_VERSION still read 0.45.2, failing engine-version-consistency, the vendor contract's version pin, and the reconciliation marker, which records this constant as the engine the rules are valid against. Bumped, and the marker test's deliberate literal (it exists so an upgrade cannot refresh it silently) refreshed by hand.
  • src/generated/ast-grep-rule-schema.json is fetched from the tagged upstream, so it was version-bound. Regenerated for 0.45.3; the only change is the Severity enum's order (off now first, since --min-severity compares severities as an ordered type). Nothing of ours reads the order.
  • The severity-vocabulary pin asserted ast-grep's error text in the old order. The vocabulary is unchanged; the pin records the reorder and why.

Full CLI suite after the fixes: 88 files, 1444 tests, all passing. No 0.45.3 regression found: every existing language-alias, kind, $$$, Markdown and binding pin held unchanged under tree-sitter 0.27.

New vendor-contract pins

ast-grep-vendor-contract.test.ts gains an inline ast-grep-ignore comments block. 0.45.3 (ast-grep/ast-grep#2909) stopped treating any comment that contains ast-grep-ignore as a directive; it must now be the comment's first alphabetic text. Nothing of ours writes those comments, but check scans whatever code a project has, so it reaches a user as findings appearing under prose or hints vanishing.

Measured against both binaries (0.45.2 swapped into the platform package path), each case records its side of the bump:

case 0.45.2 0.45.3
// ast-grep-ignore / // ast-grep-ignore: no-eval above a match suppresses it pass pass
a prose comment mentioning the directive above a match no longer suppresses it fail pass
a prose mention with nothing to suppress no longer yields an unused-suppression hint fail pass
anchor is the first alphabetic char: /* */, extra spaces, and // 1. still count; // NOTE ast-grep-ignore is prose fail pass
a genuinely unused directive reaches --json=stream as ruleId: unused-suppression, severity: hint, note: null pass pass

The two baseline cases were also mutated at the fixture level (directive removed; scope changed) and fail as expected.

Not pinned, deliberately: --min-severity. check has no severity filter and runAstGrepScan passes no such flag, so there is no path by which it reaches a user. Outline changes are not exposed by the CLI.

Changeset

.changeset/ast-grep-0-45-3.md is grown in place (still patch) with the two user-observable directions. No update.md ledger entry: nothing installed under .taskless/ migrates, and the ledger exists only for that.

One aside worth knowing: the repo's own taskless check flagged a wrapped comment line in the new test whose text began with the token, exactly the anchor being pinned. Reflowed.

Stacked on #334 — merges down into vendor/ast-grep/upgrade.

… line

The pin bump alone left four things behind:

- AST_GREP_VERSION still said 0.45.2, so engine-version-consistency and
  the vendor contract's version pin both failed, and the reconciliation
  marker recorded the wrong engine.
- The vendored rule schema is fetched from the tagged upstream, so it is
  regenerated. The only change is the Severity enum's order: `off` now
  sorts first, because `--min-severity` compares severities as an
  ordered type. No consumer of ours reads the order.
- The severity-vocabulary pin asserted the old order in ast-grep's error
  text. The vocabulary is unchanged; the pin records the reorder.
- The reconciliation marker test's deliberate literal, which exists so an
  upgrade cannot refresh it silently, is refreshed by hand.
ast-grep 0.45.3 (ast-grep/ast-grep#2909) stopped treating any comment
that CONTAINS `ast-grep-ignore` as a directive; it now has to be the
comment's first alphabetic text. Nothing of ours writes these, but
`check` scans whatever code a project has, so the change reaches a user
as a finding that appears under a prose comment, or an
`unused-suppression` hint that vanishes, with nothing saying why.

Measured against both binaries, swapped in place: the prose-mention,
unused-hint and first-alphabetic cases all fail on 0.45.2 and pass on
0.45.3, and the two baseline cases (the directive works; a genuinely
unused one is reported as a hint on the stream) fail when their fixture
is broken.

Not pinned, deliberately: `--min-severity`. `check` has no severity
filter and `runAstGrepScan` passes no such flag, so there is no path by
which it reaches a user. tree-sitter 0.27 moved nothing the existing
language-alias, kind and pattern pins can see.
@thecodedrift
thecodedrift force-pushed the vendor/ast-grep/upgrade-tests branch from bd2a640 to cb7c5e4 Compare September 15, 2026 23:57
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@thecodedrift
thecodedrift added this pull request to stack #338 September 15, 2026 23:58
@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 2m 25s —— View job


Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no review threads, review summaries, or prior top-level comments (only the @claude /review trigger itself), so this is the first review of this PR.

Todo list

  • Read .prior-review.json
  • Fetch PR diff via gh pr diff
  • Review changed files for correctness, security, performance, test adequacy, clarity
  • Post inline comments for concrete issues (none found — see below)
  • Post final summary

Scope reviewed: the 3-commit diff on vendor/ast-grep/upgrade-tests vs base vendor/ast-grep/upgradeAST_GREP_VERSION bump, regenerated ast-grep-rule-schema.json, the severity-vocabulary pin reorder, the reconciliation-marker literal, the new inline ast-grep-ignore comments test block, and the changeset.

Findings: none. I did not find any correctness, security, performance, or clarity issues. Specifics I checked and confirmed:

  • AST_GREP_VERSION is the single source of truth (packages/cli/src/rules/capabilities.ts:42) and is consumed consistently everywhere (reconcile-marker.ts, verify.ts, recipes.ts, doc templates via %(AST_GREP_VERSION)s) — no stray hardcoded 0.45.2 left behind anywhere in src/ or test/.
  • The Severity enum reorder in ast-grep-rule-schema.json (off now first) is read by verify.ts/ast-grep-rule.ts only for its const values via schema validation, never by array position — confirmed nothing else in the codebase depends on that ordering (format.ts's own ["error","warning","info","hint"] list is an unrelated, independently-defined type).
  • The reordered error-text pin in ast-grep-vendor-contract.test.ts:107-109 matches the schema reorder rationale (--min-severity needs off to sort lowest).
  • The new inline ast-grep-ignore comments describe block: walked through each case (// see ast-grep-ignore: … for how to suppress, // This comment mentions ast-grep-ignore as prose, /* ast-grep-ignore */, // ast-grep-ignore, // 1. ast-grep-ignore, // NOTE ast-grep-ignore) against the "first alphabetic character" rule described in the PR body, and each expected suppress/no-suppress outcome is consistent with that rule.
  • The self-referential comment-wrapping concern the PR body calls out (own check flagging a wrapped line that starts with the token) — verified no comment line in the new test block starts with ast-grep-ignore after the reflow; it's clean.
  • .changeset/ast-grep-0-45-3.md stays patch, correct per this repo's pre-1.0 semver convention (packages/cli/package.json is 0.11.1), grown in place rather than duplicated, consistent with the stacked-PR changeset placement rules.
  • Version-pin consistency: packages/cli/package.json already pins @ast-grep/cli* to 0.45.3 (from the stacked base PR chore(ast-grep): upgrade to ast-grep 0.45.3 #334), matching the constants/tests updated here.

One thing I could not verify myself (not a finding, just a disclosure): the exact JSON-stream ordering asserted in the "reports a genuinely unused directive" test (no-eval error before both unused-suppression hints) is pinned as a literal sequence, consistent with this file's existing convention of pinning literal ast-grep output — I did not run the binary to confirm it, per review instructions not to run tests locally; the PR body states this was measured against both binaries directly.

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