Repository navigation
feat(grammar): universal highlighting distribution — sublime, KDE, highlight.js, tree-sitter - #3996
Merged
Merged
Conversation
…ghlight.js, tree-sitter Extends the grammar mirror pipeline (#3986) so every syntax-highlighting registry on the internet is fed from this monorepo, with mirrors as write-only build artifacts: - pkg-grammar: generated baml.sublime-syntax (bat/syntect/Sublime, validated against real syntect via bat on all fixtures), hand-authored KDE KSyntaxHighlighting definition (Kate/Pandoc, validates against the official XSD), canonical showcase__golden_sample.baml fixture (mirrored as samples/baml.sample), SUPPORT.md registry matrix, frozen-path contract in the mirror README, DISTRIBUTION.md rollout playbook. - pkg-grammar-hljs (new): highlight.js language definition with Jinja-in-prompt highlighting, tested against every pkg-grammar fixture; mirrors to BoundaryML/baml-highlightjs → npm @boundaryml/baml-highlightjs. - pkg-grammar-treesitter (new): modern tree-sitter grammar for the full expression language (no external scanner; 80/80 fixtures parse with zero ERROR/MISSING, 63 corpus tests, highlight + jinja injection queries); mirrors to BoundaryML/baml-treesitter for nvim/Zed/Helix. - sync-grammar-mirror: fans out to all three mirrors and tags npm-published versions (v*) so bat/Package Control/Linguist can pin. - ci: grammar-tests now runs all three packages as a conformance gate — a language change that a port doesn't handle fails the PR. Bootstrap steps for the two new mirror repos (deploy keys, first npm publish) are documented in typescript2/pkg-grammar/DISTRIBUTION.md. Co-Authored-By: Claude Fable 5 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis change adds Highlight.js and Tree-sitter grammars, generates Sublime Text and KDE syntax definitions, expands grammar fixtures and validation, and updates CI and mirror workflows for testing, assembly, publishing, and synchronization. ChangesGrammar distribution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GrammarSource
participant CI
participant MirrorWorkflow
participant GrammarMirror
participant NpmRegistry
GrammarSource->>CI: Detect grammar package changes
CI->>CI: Run grammar validation suites
MirrorWorkflow->>GrammarMirror: Assemble and push mirror artifacts
MirrorWorkflow->>NpmRegistry: Verify or publish Highlight.js package
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/sync-grammar-mirror.yml (1)
56-170: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd explicit
permissions:to these jobs.Neither
sync-textmatenorsync-highlightjsdeclares apermissions:block, so they run with the default (potentially broad)GITHUB_TOKENscope. Both jobs only need to checkout the repo and runpnpmscripts (third-party dependency code executes here); the actual mirror pushes use a dedicated SSH deploy key, notGITHUB_TOKEN. Scope the token down, e.g.permissions: contents: readat the workflow or job level (the same likely applies tosync-treesitter, not flagged in the given hints but structurally identical).permissions: contents: readAlso applies to: 172-269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sync-grammar-mirror.yml around lines 56 - 170, Add an explicit permissions block with contents: read to the sync-textmate and sync-highlightjs jobs, and apply the same restriction to the structurally identical sync-treesitter job if present. Keep the existing SSH deploy-key behavior unchanged; these jobs should not receive broader GITHUB_TOKEN permissions.Source: Linters/SAST tools
🧹 Nitpick comments (3)
typescript2/pkg-grammar-hljs/mirror/publish.yml (1)
33-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the Node.js version instead of
latest.Trusted publishing requires npm CLI >=11.5.1 and Node >=22.14.0 Trusted publishing requires npm CLI version 11.5.1 or later and Node version 22.14.0 or higher. The npm-version half is explicitly checked below (Lines 38-46), but the Node-version half relies implicitly on
latestalways exceeding that floor. Pinning to a specific major version keeps this publish workflow reproducible and avoids silent breakage from future Node releases, while still satisfying the floor.♻️ Proposed fix
- uses: actions/setup-node@v4 with: - node-version: latest + node-version: "24" registry-url: https://registry.npmjs.org🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/pkg-grammar-hljs/mirror/publish.yml` around lines 33 - 36, Update the node-version setting in the setup-node action within the publish workflow to a pinned Node.js version at or above 22.14.0, rather than latest. Keep the existing registry-url and npm version validation unchanged.typescript2/pkg-grammar/scripts/emit-sublime.ts (1)
235-237: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
includebranch skips the exhaustive key-combination check used elsewhere.
match,begin, andpatternsbranches all explicitly reject unexpected key combinations (Lines 240-242, 262-264, 304-306), butincludereturns immediately without checking for strayname/match/patterns/captureson the same rule object. If such a construct is ever emitted upstream, it would be silently dropped rather than failing loudly — and the drift test wouldn't catch it, since it just re-runs this same emitter.♻️ Proposed fix
if (rule.include !== undefined) { + const extraKeys = Object.keys(rule).filter((key) => key !== "include"); + if (extraKeys.length > 0) { + fail(where, `\`include\` rule combined with [${extraKeys.join(", ")}] is unhandled`); + } return [{ kind: "include", target: resolveInclude(rule.include, where) }]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/pkg-grammar/scripts/emit-sublime.ts` around lines 235 - 237, Update the include branch in the rule-emission logic to perform the same exhaustive key-combination validation as the match, begin, and patterns branches before returning the include token. Reject any accompanying name, match, patterns, captures, or other unsupported keys instead of silently ignoring them, while preserving valid include-only rules.typescript2/pkg-grammar/tests/kde-syntax.test.ts (1)
272-294: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDynamic-context check only follows the
contextattribute, missinglineEndContext/lineEmptyContext/fallthroughContext.Line 282 resolves
el.attributes.context ?? ""only. The same file already trackslineEndContext,lineEmptyContext, andfallthroughContextas valid ways to transition into a context (see thecontextKeysarray at line 185). A context reachable only via one of those three attributes, but containing adynamic="true"RegExpr, would silently pass this check even withoutdynamic="true"on itself.♻️ Suggested fix
for (const contextsBlock of find(language, "contexts")) { walk(contextsBlock, (el) => { if (el.name === "context" || el.name === "contexts") return; - // A rule whose String captures a group and whose target context uses - // %N substitution must land in a dynamic="true" context. - const target = referencedContextName(el.attributes.context ?? ""); - if (target === null) return; - const targetContext = contextsByName.get(target); - if (!targetContext) return; - const usesSubstitution = find(targetContext, "RegExpr").some( - (rule) => rule.attributes.dynamic === "true", - ); - if (usesSubstitution && targetContext.attributes.dynamic !== "true") { - throw new Error(`context "${target}" has dynamic rules but is not dynamic="true"`); - } + // A rule whose String captures a group and whose target context uses + // %N substitution must land in a dynamic="true" context. + for (const key of ["context", "lineEndContext", "lineEmptyContext", "fallthroughContext"]) { + const target = referencedContextName(el.attributes[key] ?? ""); + if (target === null) continue; + const targetContext = contextsByName.get(target); + if (!targetContext) continue; + const usesSubstitution = find(targetContext, "RegExpr").some( + (rule) => rule.attributes.dynamic === "true", + ); + if (usesSubstitution && targetContext.attributes.dynamic !== "true") { + throw new Error(`context "${target}" has dynamic rules but is not dynamic="true"`); + } + } }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/pkg-grammar/tests/kde-syntax.test.ts` around lines 272 - 294, Update the dynamic-context check in the test so it resolves every transition attribute listed by contextKeys, including context, lineEndContext, lineEmptyContext, and fallthroughContext, rather than only el.attributes.context. Apply the existing referenced-context and dynamic-rule validation to each resolved target while preserving the current behavior for absent or unknown targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sync-grammar-mirror.yml:
- Around line 142-147: Update the publish push commands in both mirror jobs,
including sync-highlightjs, to push the main branch and v${next} tag atomically
in a single git push --atomic operation. Replace the separate branch and tag
pushes while preserving the existing tag creation and version references.
In `@typescript2/pkg-grammar-hljs/mirror/README.md`:
- Line 48: Update the README script reference to use highlight.js’s prebuilt
browser-global bundle instead of the npm/CommonJS entrypoint, ensuring it
provides the global hljs expected by dist/baml.js.
In `@typescript2/pkg-grammar-hljs/scripts/assemble-mirror.mjs`:
- Line 15: Validate the resolved output path before the rmSync cleanup in
assemble-mirror.mjs. Reject paths equal to or containing the working directory,
repository root, package root, or any ancestor of those roots, including --out .
and parent directories; only perform deletion after this guard passes.
In `@typescript2/pkg-grammar-hljs/src/baml.js`:
- Around line 35-44: Update IDENT and KEYWORDS.$pattern in the BAML grammar to
allow lexer-supported $-joined identifier segments, matching the identifier form
used by the tree-sitter grammar. Preserve existing optional leading-$ behavior
while permitting internal $ characters so names such as Foo$bar remain a single
token and do not receive partial keyword or title scopes.
- Around line 216-224: Update the SUBST grammar mode so interpolation braces are
nested recursively and the mode closes only at the matching outer brace,
preserving nested block and constructor-expression highlighting. Add a BAML
snapshot test in the existing baml.test.ts coverage for an interpolation
containing nested braces.
In `@typescript2/pkg-grammar-treesitter/grammar.js`:
- Around line 11-14: The raw-string grammar currently supports only four hash
delimiter levels; update the raw-string token definitions around the per-level
delimiter declarations to support arbitrary hash counts as required by the
language spec. If the Tree-sitter grammar cannot express unbounded delimiters,
explicitly narrow the documented supported range to four hashes instead.
In `@typescript2/pkg-grammar-treesitter/mirror/README.md`:
- Around line 45-48: Pin the baml grammar source in the [[grammar]]
configuration by replacing the mutable rev = "main" value with a specific
immutable commit revision from baml-treesitter. Keep the existing repository URL
and grammar name unchanged.
In `@typescript2/pkg-grammar-treesitter/scripts/assemble-mirror.mjs`:
- Around line 47-49: Validate the resolved output path in the assemble script
before the rmSync call, rejecting any path that is the repository directory, an
ancestor of it, or contained within it. Ensure the overlap check uses normalized
absolute paths and aborts before deletion, while allowing output paths outside
the repository tree.
In `@typescript2/pkg-grammar/DISTRIBUTION.md`:
- Around line 9-16: Update the fenced diagram in the distribution documentation
to specify the text language by changing its opening fence to use text, while
preserving the diagram content unchanged.
In
`@typescript2/pkg-grammar/tests/snapshots/showcase__golden_sample.baml.scope.txt`:
- Around line 352-360: Update the grammar scope expectations for the args
constructor block so the quoted value following text is tokenized as
string.quoted.double.baml, including the embedded comma, rather than raw
constructor-body text with punctuation.separator.comma.baml. Adjust the relevant
grammar rule or snapshot generation around the args constructor expression while
preserving the existing scopes for the hints array and constructor delimiters.
---
Outside diff comments:
In @.github/workflows/sync-grammar-mirror.yml:
- Around line 56-170: Add an explicit permissions block with contents: read to
the sync-textmate and sync-highlightjs jobs, and apply the same restriction to
the structurally identical sync-treesitter job if present. Keep the existing SSH
deploy-key behavior unchanged; these jobs should not receive broader
GITHUB_TOKEN permissions.
---
Nitpick comments:
In `@typescript2/pkg-grammar-hljs/mirror/publish.yml`:
- Around line 33-36: Update the node-version setting in the setup-node action
within the publish workflow to a pinned Node.js version at or above 22.14.0,
rather than latest. Keep the existing registry-url and npm version validation
unchanged.
In `@typescript2/pkg-grammar/scripts/emit-sublime.ts`:
- Around line 235-237: Update the include branch in the rule-emission logic to
perform the same exhaustive key-combination validation as the match, begin, and
patterns branches before returning the include token. Reject any accompanying
name, match, patterns, captures, or other unsupported keys instead of silently
ignoring them, while preserving valid include-only rules.
In `@typescript2/pkg-grammar/tests/kde-syntax.test.ts`:
- Around line 272-294: Update the dynamic-context check in the test so it
resolves every transition attribute listed by contextKeys, including context,
lineEndContext, lineEmptyContext, and fallthroughContext, rather than only
el.attributes.context. Apply the existing referenced-context and dynamic-rule
validation to each resolved target while preserving the current behavior for
absent or unknown targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a803bc40-59db-4af0-b32b-9c13d025ef54
⛔ Files ignored due to path filters (1)
typescript2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.github/workflows/ci.yaml.github/workflows/sync-grammar-mirror.ymltypescript2/pkg-grammar-hljs/README.mdtypescript2/pkg-grammar-hljs/mirror/README.mdtypescript2/pkg-grammar-hljs/mirror/package.jsontypescript2/pkg-grammar-hljs/mirror/publish.ymltypescript2/pkg-grammar-hljs/package.jsontypescript2/pkg-grammar-hljs/scripts/assemble-mirror.mjstypescript2/pkg-grammar-hljs/src/baml.jstypescript2/pkg-grammar-hljs/tests/baml.test.tstypescript2/pkg-grammar-hljs/tests/snapshots/baml_tests__const_let_else_defer.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/baml_tests__retry_policy_valid_retry.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/generators.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/lsp_syntax_functions__jinja_control_prompt.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/ns_sentiment__sentiment.baml.htmltypescript2/pkg-grammar-hljs/vitest.config.tstypescript2/pkg-grammar-treesitter/.gitignoretypescript2/pkg-grammar-treesitter/README.mdtypescript2/pkg-grammar-treesitter/grammar.jstypescript2/pkg-grammar-treesitter/mirror/README.mdtypescript2/pkg-grammar-treesitter/package.jsontypescript2/pkg-grammar-treesitter/queries/highlights.scmtypescript2/pkg-grammar-treesitter/queries/injections.scmtypescript2/pkg-grammar-treesitter/scripts/assemble-mirror.mjstypescript2/pkg-grammar-treesitter/test/corpus/attributes.txttypescript2/pkg-grammar-treesitter/test/corpus/control_flow.txttypescript2/pkg-grammar-treesitter/test/corpus/declarations.txttypescript2/pkg-grammar-treesitter/test/corpus/expressions.txttypescript2/pkg-grammar-treesitter/test/corpus/patterns.txttypescript2/pkg-grammar-treesitter/test/corpus/strings.txttypescript2/pkg-grammar-treesitter/test/corpus/types.txttypescript2/pkg-grammar-treesitter/tree-sitter.jsontypescript2/pkg-grammar/DISTRIBUTION.mdtypescript2/pkg-grammar/README.mdtypescript2/pkg-grammar/baml.sublime-syntaxtypescript2/pkg-grammar/mirror/README.mdtypescript2/pkg-grammar/mirror/SUPPORT.mdtypescript2/pkg-grammar/package.jsontypescript2/pkg-grammar/scripts/assemble-mirror.mjstypescript2/pkg-grammar/scripts/emit-sublime.tstypescript2/pkg-grammar/syntaxes/baml.xmltypescript2/pkg-grammar/tests/fixtures/showcase__golden_sample.bamltypescript2/pkg-grammar/tests/kde-syntax.test.tstypescript2/pkg-grammar/tests/snapshots/showcase__golden_sample.baml.scope.txttypescript2/pkg-grammar/tests/sublime-syntax.test.tstypescript2/pnpm-workspace.yaml
- src/baml.ts: new constructor-field-shorthand rule so colon-less fields
(test args `text "..."`, `functions [F]`) scope name + value correctly —
pre-existing gap CodeRabbit caught via the golden-sample snapshot.
- sync-grammar-mirror: push main and the version tag atomically so a
half-failed push can never strand an untagged release.
- assemble-mirror scripts (all three): refuse --out paths overlapping the
repo checkout before the recursive rmSync.
- pkg-grammar-hljs: identifiers use the lexer's full $-joined Word shape
(Foo$bar, $stream are single tokens); backtick ${...} interpolation now
tracks nested braces; README points at the real browser CDN bundle;
+2 regression tests.
- pkg-grammar-treesitter: raw strings extended to 8 hash levels via
MAX_RAW_STRING_HASHES, matching the TextMate MAX_DELIMITER = 8 bound;
+1 corpus test; mirror README tells Helix users to pin a commit.
- DISTRIBUTION.md: language on the fenced diagram (MD040).
Co-Authored-By: Claude Fable 5 <[email protected]>
Contributor
Author
|
@coderabbitai resolve |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@typescript2/app-vscode-ext/syntaxes/baml.tmLanguage.json`:
- Around line 1700-1714: Update the constructor-field shorthand grammar around
"constructor-field-shorthand" so its begin boundary also recognizes a shorthand
field immediately following a same-line block comment, while preserving existing
start, brace, and comma boundaries. Add a regression fixture covering `/* note
*/ text "Jane"`, then update the canonical generator and regenerate the syntax
mirror.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3596083a-4d67-43f3-be29-3937ce873410
📒 Files selected for processing (18)
.github/workflows/sync-grammar-mirror.ymltypescript2/app-vscode-ext/syntaxes/baml.tmLanguage.jsontypescript2/pkg-grammar-hljs/mirror/README.mdtypescript2/pkg-grammar-hljs/scripts/assemble-mirror.mjstypescript2/pkg-grammar-hljs/src/baml.jstypescript2/pkg-grammar-hljs/tests/baml.test.tstypescript2/pkg-grammar-treesitter/README.mdtypescript2/pkg-grammar-treesitter/grammar.jstypescript2/pkg-grammar-treesitter/mirror/README.mdtypescript2/pkg-grammar-treesitter/scripts/assemble-mirror.mjstypescript2/pkg-grammar-treesitter/test/corpus/strings.txttypescript2/pkg-grammar/DISTRIBUTION.mdtypescript2/pkg-grammar/baml.sublime-syntaxtypescript2/pkg-grammar/baml.tmLanguage.jsontypescript2/pkg-grammar/scripts/assemble-mirror.mjstypescript2/pkg-grammar/src/baml.tstypescript2/pkg-grammar/tests/snapshots/lsp_semantic__test_block.baml.scope.txttypescript2/pkg-grammar/tests/snapshots/showcase__golden_sample.baml.scope.txt
🚧 Files skipped from review as they are similar to previous changes (12)
- typescript2/pkg-grammar-treesitter/mirror/README.md
- typescript2/pkg-grammar/baml.sublime-syntax
- typescript2/pkg-grammar/DISTRIBUTION.md
- typescript2/pkg-grammar-treesitter/README.md
- .github/workflows/sync-grammar-mirror.yml
- typescript2/pkg-grammar-hljs/mirror/README.md
- typescript2/pkg-grammar-hljs/scripts/assemble-mirror.mjs
- typescript2/pkg-grammar-hljs/src/baml.js
- typescript2/pkg-grammar-treesitter/scripts/assemble-mirror.mjs
- typescript2/pkg-grammar-treesitter/grammar.js
- typescript2/pkg-grammar-treesitter/test/corpus/strings.txt
- typescript2/pkg-grammar/tests/snapshots/showcase__golden_sample.baml.scope.txt
Contributor
✅ Action performedComments resolved and changes approved. |
Was written as an internal rollout playbook; now it reads as a contributor-facing doc: how to use the grammars in your own project, where highlighting fixes go, which integrations are live vs. looking for an owner, with maintainer wiring notes at the end. Co-Authored-By: Claude Fable 5 <[email protected]>
hellovai
enabled auto-merge
July 13, 2026 00:28
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@typescript2/pkg-grammar/DISTRIBUTION.md`:
- Around line 52-55: Update the npm consumer row in DISTRIBUTION.md to
distinguish the established `@boundaryml/baml-grammar` package from the newly
seeded `@boundaryml/baml-highlightjs` package: mark highlightjs as pending initial
manual publish and OTP/trusted-publisher setup rather than “✅ live” and
auto-published. Keep the documented live status for baml-grammar and ensure the
table agrees with the maintainer note about first publication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ac4971cd-80bf-4dba-b32b-20f04a8f6145
📒 Files selected for processing (1)
typescript2/pkg-grammar/DISTRIBUTION.md
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
typescript2/pkg-grammar/tests/fixtures/showcase__golden_sample.baml (1)
1-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider exercising the new
constructor-field-shorthandconstruct here.This is the shared conformance fixture for all four highlighter ports, but no constructor-instantiation expression appears in it — so the new
constructor-field-shorthandrule (baml.tmLanguage.jsonlines 1700-1717) currently isn't validated by any of the Sublime/KDE/hljs/tree-sitter conformance tests that consume this file. Note the value must start with an unambiguous literal (string/number/bool/null/env/array/backtick) for the shorthand rule to trigger — a bare identifier won't match its lookahead.Proposed addition
test GoldenExtract { functions [ExtractResume] args { text "Jane Doe, 12 years of Rust" hints ["skills", "email"] } } + +class Point { + x int + y int +} + +function MakePoint() -> Point { + Point { x: 1, y 2 } +}Worth confirming first whether constructor-literal syntax is already covered by another fixture under
tests/fixtures/*.bamlnot in this review batch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/pkg-grammar/tests/fixtures/showcase__golden_sample.baml` around lines 1 - 97, Add a constructor-instantiation expression to the shared GoldenExtract fixture to exercise the new constructor-field-shorthand rule, using a value that begins with an unambiguous supported literal such as a string, number, boolean, null, env, array, or backtick expression. Keep the example valid BAML syntax and avoid a bare identifier as the shorthand value; first reuse an existing fixture if constructor-literal coverage already exists there..github/workflows/sync-grammar-mirror.yml (1)
56-171: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd least-privilege
permissions:to the mirror-sync jobs.Neither
sync-textmatenorsync-highlightjsdeclares apermissions:block, so the job'sGITHUB_TOKENgets the repo/org default (potentially broad). Since these jobs push to mirrors via a deploy-key SSH secret (notGITHUB_TOKEN) and only needactions/checkoutfor read, an explicit minimal block (e.g.contents: read) would tighten this. Same applies tosync-treesitter(Lines 272-326).🔒 Example fix
sync-textmate: name: Sync to BoundaryML/textMate-baml runs-on: ubuntu-latest + permissions: + contents: read steps:Also applies to: 173-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sync-grammar-mirror.yml around lines 56 - 171, Declare an explicit least-privilege permissions block for the sync-textmate, sync-highlightjs, and sync-treesitter jobs, granting only contents: read. Keep the existing deploy-key SSH flow for mirror pushes unchanged and apply the same permissions configuration consistently to all three jobs.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@typescript2/pkg-grammar-hljs/scripts/assemble-mirror.mjs`:
- Around line 62-76: Harden the source validation in assemble-mirror.mjs before
constructing the IIFE: require the actual default export declaration to be a
function named baml, and ensure the transformation targets that declaration
rather than an arbitrary first matching substring. Keep the generated wrapper’s
registerLanguage('baml', baml) binding valid and fail with the existing error
path when the expected declaration is absent.
In `@typescript2/pkg-grammar/scripts/assemble-mirror.mjs`:
- Line 19: Canonicalize the --out path before the recursive deletion overlap
check in the assembly script. Use realpathSync on the nearest existing ancestor,
then rebuild the canonical output path and perform the existing checkout-overlap
validation against it, preserving normal handling for paths that do not yet
exist.
In `@typescript2/pkg-grammar/syntaxes/baml.xml`:
- Around line 44-60: The singular `implement` keyword is inconsistent across the
lexer, BAML grammar, tree-sitter grammar, and highlights. Choose the intended
keyword spelling, then update the `declarations` list, `grammar.js`,
`queries/highlights.scm`, and the lexer/BAML grammar mirrors so all
representations consistently recognize or omit `implement` alongside
`implements`.
---
Nitpick comments:
In @.github/workflows/sync-grammar-mirror.yml:
- Around line 56-171: Declare an explicit least-privilege permissions block for
the sync-textmate, sync-highlightjs, and sync-treesitter jobs, granting only
contents: read. Keep the existing deploy-key SSH flow for mirror pushes
unchanged and apply the same permissions configuration consistently to all three
jobs.
In `@typescript2/pkg-grammar/tests/fixtures/showcase__golden_sample.baml`:
- Around line 1-97: Add a constructor-instantiation expression to the shared
GoldenExtract fixture to exercise the new constructor-field-shorthand rule,
using a value that begins with an unambiguous supported literal such as a
string, number, boolean, null, env, array, or backtick expression. Keep the
example valid BAML syntax and avoid a bare identifier as the shorthand value;
first reuse an existing fixture if constructor-literal coverage already exists
there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 28bd6b79-cee3-47c9-b3b7-e69c663e2fbe
⛔ Files ignored due to path filters (1)
typescript2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (50)
.github/workflows/ci.yaml.github/workflows/sync-grammar-mirror.ymltypescript2/app-vscode-ext/syntaxes/baml.tmLanguage.jsontypescript2/pkg-grammar-hljs/README.mdtypescript2/pkg-grammar-hljs/mirror/README.mdtypescript2/pkg-grammar-hljs/mirror/package.jsontypescript2/pkg-grammar-hljs/mirror/publish.ymltypescript2/pkg-grammar-hljs/package.jsontypescript2/pkg-grammar-hljs/scripts/assemble-mirror.mjstypescript2/pkg-grammar-hljs/src/baml.jstypescript2/pkg-grammar-hljs/tests/baml.test.tstypescript2/pkg-grammar-hljs/tests/snapshots/baml_tests__const_let_else_defer.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/baml_tests__retry_policy_valid_retry.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/generators.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/lsp_syntax_functions__jinja_control_prompt.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/ns_sentiment__sentiment.baml.htmltypescript2/pkg-grammar-hljs/vitest.config.tstypescript2/pkg-grammar-treesitter/.gitignoretypescript2/pkg-grammar-treesitter/README.mdtypescript2/pkg-grammar-treesitter/grammar.jstypescript2/pkg-grammar-treesitter/mirror/README.mdtypescript2/pkg-grammar-treesitter/package.jsontypescript2/pkg-grammar-treesitter/queries/highlights.scmtypescript2/pkg-grammar-treesitter/queries/injections.scmtypescript2/pkg-grammar-treesitter/scripts/assemble-mirror.mjstypescript2/pkg-grammar-treesitter/test/corpus/attributes.txttypescript2/pkg-grammar-treesitter/test/corpus/control_flow.txttypescript2/pkg-grammar-treesitter/test/corpus/declarations.txttypescript2/pkg-grammar-treesitter/test/corpus/expressions.txttypescript2/pkg-grammar-treesitter/test/corpus/patterns.txttypescript2/pkg-grammar-treesitter/test/corpus/strings.txttypescript2/pkg-grammar-treesitter/test/corpus/types.txttypescript2/pkg-grammar-treesitter/tree-sitter.jsontypescript2/pkg-grammar/DISTRIBUTION.mdtypescript2/pkg-grammar/README.mdtypescript2/pkg-grammar/baml.sublime-syntaxtypescript2/pkg-grammar/baml.tmLanguage.jsontypescript2/pkg-grammar/mirror/README.mdtypescript2/pkg-grammar/mirror/SUPPORT.mdtypescript2/pkg-grammar/package.jsontypescript2/pkg-grammar/scripts/assemble-mirror.mjstypescript2/pkg-grammar/scripts/emit-sublime.tstypescript2/pkg-grammar/src/baml.tstypescript2/pkg-grammar/syntaxes/baml.xmltypescript2/pkg-grammar/tests/fixtures/showcase__golden_sample.bamltypescript2/pkg-grammar/tests/kde-syntax.test.tstypescript2/pkg-grammar/tests/snapshots/lsp_semantic__test_block.baml.scope.txttypescript2/pkg-grammar/tests/snapshots/showcase__golden_sample.baml.scope.txttypescript2/pkg-grammar/tests/sublime-syntax.test.tstypescript2/pnpm-workspace.yaml
- src/baml.ts: shorthand constructor fields now also match after a same-line block comment (/* note */ text "..."); regression case added to the golden sample. - pkg-grammar-treesitter: accept the singular `implement` keyword interchangeably with `implements` in both block and for-declaration positions, matching the compiler parser; highlight query + corpus test. - assemble-mirror scripts: canonicalize paths through symlinks (realpath of nearest existing ancestor) before the overlap check. - pkg-grammar-hljs assemble: dist wrapper guard now requires the exact `export default function baml(` shape it rewrites. Co-Authored-By: Claude Fable 5 <[email protected]>
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.github/workflows/sync-grammar-mirror.yml (1)
56-171: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit
permissions:blocks to the sync jobs.Static analysis flags both
sync-textmateandsync-highlightjs(and, by the same reasoning,sync-treesitter) for relying on defaultGITHUB_TOKENpermissions. Since these jobs authenticate to the mirrors via dedicated deploy-key secrets rather thanGITHUB_TOKEN, scoping down topermissions: contents: read(or similar minimal grant) at the job or workflow level would tighten the security posture without affecting functionality.Also applies to: 173-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sync-grammar-mirror.yml around lines 56 - 171, Add explicit minimal GitHub Actions permissions for the sync workflow, setting contents: read at the workflow or job level for sync-textmate, sync-highlightjs, and sync-treesitter. Preserve the existing deploy-key authentication and workflow behavior while ensuring these jobs do not rely on broader default GITHUB_TOKEN permissions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@typescript2/pkg-grammar-hljs/src/baml.js`:
- Around line 20-21: Extend the raw-string grammar registration in baml.js from
its current 1–3 hash levels to support 4–8 as well, preserving string
highlighting and embedded Jinja markup for every supported delimiter level.
- Around line 339-350: Update the INTERPOLATION_CONTAINS grammar list to include
RAW_STRINGS before QUOTED_STRING, ensuring raw strings inside interpolated
expressions consume internal braces and only the matching closing brace ends the
interpolation.
In `@typescript2/pkg-grammar-treesitter/grammar.js`:
- Around line 44-47: Update IDENT_REGEX to use the same optional-prefix
identifier expression as the Highlight.js mirror, allowing leading-$ identifiers
to contain hyphens and additional $-joined segments while still rejecting a
trailing $. Preserve the existing lexer-compatible handling for non-$-prefixed
words.
In `@typescript2/pkg-grammar-treesitter/package.json`:
- Line 9: Update the parse-fixtures script’s grep pattern to match only
tree-sitter’s actual ERROR or MISSING node markers, rather than those words
anywhere in the parse output. Preserve the existing failure counting, reporting,
and exit behavior.
In `@typescript2/pkg-grammar/baml.tmLanguage.json`:
- Around line 1643-1645: Update the grammar rule referenced by the
constructor-field-shorthand include so its begin pattern accepts all valid
$._expression starts, including identifiers, calls, namespace/type paths, nested
objects or lists, and lambdas, rather than only literal starts. Preserve the
existing property scope and constructor-field shorthand matching behavior.
In `@typescript2/pkg-grammar/tests/kde-syntax.test.ts`:
- Around line 272-294: Update the dynamic-context test in “marks contexts
entered by dynamic rules as dynamic” to iterate every context from
contextsByName directly, rather than only rules’ context= targets. For each
context, detect dynamic RegExpr rules and assert that the same context has
dynamic="true", covering contexts reached through lineEndContext,
lineEmptyContext, fallthroughContext, or not explicitly referenced.
---
Nitpick comments:
In @.github/workflows/sync-grammar-mirror.yml:
- Around line 56-171: Add explicit minimal GitHub Actions permissions for the
sync workflow, setting contents: read at the workflow or job level for
sync-textmate, sync-highlightjs, and sync-treesitter. Preserve the existing
deploy-key authentication and workflow behavior while ensuring these jobs do not
rely on broader default GITHUB_TOKEN permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 964ddb64-fb8e-438e-b53e-5863f3ebd403
⛔ Files ignored due to path filters (1)
typescript2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (50)
.github/workflows/ci.yaml.github/workflows/sync-grammar-mirror.ymltypescript2/app-vscode-ext/syntaxes/baml.tmLanguage.jsontypescript2/pkg-grammar-hljs/README.mdtypescript2/pkg-grammar-hljs/mirror/README.mdtypescript2/pkg-grammar-hljs/mirror/package.jsontypescript2/pkg-grammar-hljs/mirror/publish.ymltypescript2/pkg-grammar-hljs/package.jsontypescript2/pkg-grammar-hljs/scripts/assemble-mirror.mjstypescript2/pkg-grammar-hljs/src/baml.jstypescript2/pkg-grammar-hljs/tests/baml.test.tstypescript2/pkg-grammar-hljs/tests/snapshots/baml_tests__const_let_else_defer.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/baml_tests__retry_policy_valid_retry.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/generators.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/lsp_syntax_functions__jinja_control_prompt.baml.htmltypescript2/pkg-grammar-hljs/tests/snapshots/ns_sentiment__sentiment.baml.htmltypescript2/pkg-grammar-hljs/vitest.config.tstypescript2/pkg-grammar-treesitter/.gitignoretypescript2/pkg-grammar-treesitter/README.mdtypescript2/pkg-grammar-treesitter/grammar.jstypescript2/pkg-grammar-treesitter/mirror/README.mdtypescript2/pkg-grammar-treesitter/package.jsontypescript2/pkg-grammar-treesitter/queries/highlights.scmtypescript2/pkg-grammar-treesitter/queries/injections.scmtypescript2/pkg-grammar-treesitter/scripts/assemble-mirror.mjstypescript2/pkg-grammar-treesitter/test/corpus/attributes.txttypescript2/pkg-grammar-treesitter/test/corpus/control_flow.txttypescript2/pkg-grammar-treesitter/test/corpus/declarations.txttypescript2/pkg-grammar-treesitter/test/corpus/expressions.txttypescript2/pkg-grammar-treesitter/test/corpus/patterns.txttypescript2/pkg-grammar-treesitter/test/corpus/strings.txttypescript2/pkg-grammar-treesitter/test/corpus/types.txttypescript2/pkg-grammar-treesitter/tree-sitter.jsontypescript2/pkg-grammar/DISTRIBUTION.mdtypescript2/pkg-grammar/README.mdtypescript2/pkg-grammar/baml.sublime-syntaxtypescript2/pkg-grammar/baml.tmLanguage.jsontypescript2/pkg-grammar/mirror/README.mdtypescript2/pkg-grammar/mirror/SUPPORT.mdtypescript2/pkg-grammar/package.jsontypescript2/pkg-grammar/scripts/assemble-mirror.mjstypescript2/pkg-grammar/scripts/emit-sublime.tstypescript2/pkg-grammar/src/baml.tstypescript2/pkg-grammar/syntaxes/baml.xmltypescript2/pkg-grammar/tests/fixtures/showcase__golden_sample.bamltypescript2/pkg-grammar/tests/kde-syntax.test.tstypescript2/pkg-grammar/tests/snapshots/lsp_semantic__test_block.baml.scope.txttypescript2/pkg-grammar/tests/snapshots/showcase__golden_sample.baml.scope.txttypescript2/pkg-grammar/tests/sublime-syntax.test.tstypescript2/pnpm-workspace.yaml
Comment on lines
+20
to
+21
| - raw strings #"..."# (1-3 hash levels) with Jinja {{ ... }} / {% ... %} / | ||
| {# ... #} template markup inside |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby context
git ls-files typescript2/pkg-grammar-hljs/src/baml.js
wc -l typescript2/pkg-grammar-hljs/src/baml.js
cat -n typescript2/pkg-grammar-hljs/src/baml.js | sed -n '1,260p'
# Look for raw-string handling in related grammar/highlighter files
rg -n "raw strings|rawString|MAX_RAW_STRING_HASHES|hash levels|Jinja" typescript2 -S
# Inspect the matching source locations if they exist
fd -a "baml.js|baml.*" typescript2Repository: BoundaryML/baml
Length of output: 31760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the other mirrors that define the supported raw-string delimiter count
cat -n typescript2/pkg-grammar/src/baml.ts | sed -n '360,410p'
cat -n typescript2/pkg-grammar-hljs/tests/baml.test.ts | sed -n '1,120p'
cat -n typescript2/pkg-grammar-treesitter/grammar.js | sed -n '60,90p'
cat -n typescript2/pkg-grammar-treesitter/grammar.js | sed -n '1170,1188p'
cat -n typescript2/pkg-grammar-treesitter/README.md | sed -n '65,85p'Repository: BoundaryML/baml
Length of output: 11189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the hljs mirror has any tests covering 4+ hash raw strings
rg -n "#+\".*\"#+" typescript2/pkg-grammar-hljs/tests typescript2/pkg-grammar/tests -S
rg -n "MAX_RAW_STRING_HASHES|raw strings: nested hash levels|five hash delimiter" typescript2/pkg-grammar-hljs typescript2/pkg-grammar -SRepository: BoundaryML/baml
Length of output: 3278
Mirror raw strings through 8 hash levels. baml.js only registers #"..."# to ###"..."###, so valid 4–8 hash prompt strings miss string and embedded Jinja highlighting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typescript2/pkg-grammar-hljs/src/baml.js` around lines 20 - 21, Extend the
raw-string grammar registration in baml.js from its current 1–3 hash levels to
support 4–8 as well, preserving string highlighting and embedded Jinja markup
for every supported delimiter level.
Comment on lines
+339
to
+350
| // Shared contents of an interpolated expression. The list includes | ||
| // NESTED_BRACES, and NESTED_BRACES reuses the list, so arbitrarily deep | ||
| // `{ ... }` nesting inside `${ ... }` is tracked and only the balancing `}` | ||
| // closes the interpolation. | ||
| const INTERPOLATION_CONTAINS = [ | ||
| hljs.C_LINE_COMMENT_MODE, | ||
| hljs.C_BLOCK_COMMENT_MODE, | ||
| QUOTED_STRING, | ||
| NUMBER, | ||
| ENV_VAR, | ||
| NESTED_BRACES | ||
| ]; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first.
ast-grep outline typescript2/pkg-grammar-hljs/src/baml.js --view expanded
# Read the relevant section around the reported lines.
sed -n '280,390p' typescript2/pkg-grammar-hljs/src/baml.js
# Find raw-string and interpolation-related definitions/usages.
rg -n "RAW_STRINGS|QUOTED_STRING|SUBST|INTERPOLATION_CONTAINS|NESTED_BRACES|raw" typescript2/pkg-grammar-hljs/src/baml.jsRepository: BoundaryML/baml
Length of output: 3713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,260p' typescript2/pkg-grammar-hljs/src/baml.jsRepository: BoundaryML/baml
Length of output: 2732
Recognize raw strings inside interpolation. INTERPOLATION_CONTAINS should include RAW_STRINGS before QUOTED_STRING; otherwise ${#"a " } b"#} can be parsed as a plain quoted string and close the interpolation at the internal }.
Proposed fix
const INTERPOLATION_CONTAINS = [
hljs.C_LINE_COMMENT_MODE,
hljs.C_BLOCK_COMMENT_MODE,
+ ...RAW_STRINGS,
QUOTED_STRING,
NUMBER,
ENV_VAR,
NESTED_BRACES
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Shared contents of an interpolated expression. The list includes | |
| // NESTED_BRACES, and NESTED_BRACES reuses the list, so arbitrarily deep | |
| // `{ ... }` nesting inside `${ ... }` is tracked and only the balancing `}` | |
| // closes the interpolation. | |
| const INTERPOLATION_CONTAINS = [ | |
| hljs.C_LINE_COMMENT_MODE, | |
| hljs.C_BLOCK_COMMENT_MODE, | |
| QUOTED_STRING, | |
| NUMBER, | |
| ENV_VAR, | |
| NESTED_BRACES | |
| ]; | |
| // Shared contents of an interpolated expression. The list includes | |
| // NESTED_BRACES, and NESTED_BRACES reuses the list, so arbitrarily deep | |
| // `{ ... }` nesting inside `${ ... }` is tracked and only the balancing `}` | |
| // closes the interpolation. | |
| const INTERPOLATION_CONTAINS = [ | |
| hljs.C_LINE_COMMENT_MODE, | |
| hljs.C_BLOCK_COMMENT_MODE, | |
| ...RAW_STRINGS, | |
| QUOTED_STRING, | |
| NUMBER, | |
| ENV_VAR, | |
| NESTED_BRACES | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typescript2/pkg-grammar-hljs/src/baml.js` around lines 339 - 350, Update the
INTERPOLATION_CONTAINS grammar list to include RAW_STRINGS before QUOTED_STRING,
ensuring raw strings inside interpolated expressions consume internal braces and
only the matching closing brace ends the interpolation.
Comment on lines
+44
to
+47
| // Matches the lexer's Word token: hyphens allowed inside, `$`-joined segments, | ||
| // or a `$`-prefixed word ($watch). A trailing `$` is rejected (see tokens.rs). | ||
| const IDENT_REGEX = | ||
| /(\$[a-zA-Z_][a-zA-Z0-9_]*)|([a-zA-Z_][a-zA-Z0-9_-]*(\$[a-zA-Z_][a-zA-Z0-9_-]*)*)/; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Accept the full lexer form for $-prefixed identifiers.
The special leading-$ alternative rejects hyphens and additional $ segments, so valid identifiers such as $foo-bar or $foo$bar are split. Use the same optional-prefix expression as the Highlight.js mirror.
Proposed fix
const IDENT_REGEX =
- /(\$[a-zA-Z_][a-zA-Z0-9_]*)|([a-zA-Z_][a-zA-Z0-9_-]*(\$[a-zA-Z_][a-zA-Z0-9_-]*)*)/;
+ /\$?[a-zA-Z_][a-zA-Z0-9_-]*(?:\$[a-zA-Z_][a-zA-Z0-9_-]*)*/;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Matches the lexer's Word token: hyphens allowed inside, `$`-joined segments, | |
| // or a `$`-prefixed word ($watch). A trailing `$` is rejected (see tokens.rs). | |
| const IDENT_REGEX = | |
| /(\$[a-zA-Z_][a-zA-Z0-9_]*)|([a-zA-Z_][a-zA-Z0-9_-]*(\$[a-zA-Z_][a-zA-Z0-9_-]*)*)/; | |
| // Matches the lexer's Word token: hyphens allowed inside, `$`-joined segments, | |
| // or a `$`-prefixed word ($watch). A trailing `$` is rejected (see tokens.rs). | |
| const IDENT_REGEX = | |
| /\$?[a-zA-Z_][a-zA-Z0-9_-]*(?:\$[a-zA-Z_][a-zA-Z0-9_-]*)*/; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typescript2/pkg-grammar-treesitter/grammar.js` around lines 44 - 47, Update
IDENT_REGEX to use the same optional-prefix identifier expression as the
Highlight.js mirror, allowing leading-$ identifiers to contain hyphens and
additional $-joined segments while still rejecting a trailing $. Preserve the
existing lexer-compatible handling for non-$-prefixed words.
| "scripts": { | ||
| "generate": "tree-sitter generate", | ||
| "test": "tree-sitter generate && tree-sitter test && pnpm run parse-fixtures", | ||
| "parse-fixtures": "sh -c 'fail=0; for f in ../pkg-grammar/tests/fixtures/*.baml; do n=$(tree-sitter parse \"$f\" | grep -cE \"ERROR|MISSING\"); if [ \"$n\" != 0 ]; then fail=1; echo \"FAIL($n) $f\"; fi; done; [ $fail = 0 ] && echo \"all fixtures parse clean\"; exit $fail' -", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
grep pattern in parse-fixtures may produce false positives from string content.
grep -cE "ERROR|MISSING" matches any line containing those words anywhere in tree-sitter's S-expression output. If a BAML fixture contains "ERROR" or "MISSING" as string content, identifiers, or comments, the parse output will include them and the script will report a false failure. Anchor the pattern to tree-sitter's actual node markers instead.
🛡️ Proposed fix
- "parse-fixtures": "sh -c 'fail=0; for f in ../pkg-grammar/tests/fixtures/*.baml; do n=$(tree-sitter parse \"$f\" | grep -cE \"ERROR|MISSING\"); if [ \"$n\" != 0 ]; then fail=1; echo \"FAIL($n) $f\"; fi; done; [ $fail = 0 ] && echo \"all fixtures parse clean\"; exit $fail' -",
+ "parse-fixtures": "sh -c 'fail=0; for f in ../pkg-grammar/tests/fixtures/*.baml; do n=$(tree-sitter parse \"$f\" | grep -cE \"\\(ERROR\\)|\\(MISSING\"); if [ \"$n\" != 0 ]; then fail=1; echo \"FAIL($n) $f\"; fi; done; [ $fail = 0 ] && echo \"all fixtures parse clean\"; exit $fail' -",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typescript2/pkg-grammar-treesitter/package.json` at line 9, Update the
parse-fixtures script’s grep pattern to match only tree-sitter’s actual ERROR or
MISSING node markers, rather than those words anywhere in the parse output.
Preserve the existing failure counting, reporting, and exit behavior.
Comment on lines
+1643
to
+1645
| { | ||
| "include": "#constructor-field-shorthand" | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether constructor-field shorthand supports non-literal values
# (identifiers, nested constructors, calls) in the compiler/tree-sitter grammar.
rg -n -i 'shorthand' -g '!node_modules'
fd grammar.js typescript2/pkg-grammar-treesitter --exec cat -n {} \; | rg -n -B3 -A15 'constructor_field'Repository: BoundaryML/baml
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== matching files ==\n'
fd -a 'grammar.js|*.tmLanguage.json|*.ts|*.js|*.baml' . 2>/dev/null | sed -n '1,200p'
printf '\n== text search ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'constructor-field-shorthand|constructor-field|`#expression`|shorthand' .
printf '\n== tree-sitter grammar candidates ==\n'
fd -a 'grammar.js' . 2>/dev/null | sed -n '1,200p'Repository: BoundaryML/baml
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== tracked files with relevant names ==\n'
git ls-files | rg '(^|/)(grammar\.js|.*tmLanguage\.json|.*\.(baml|ts|js|json))$' | sed -n '1,200p'
printf '\n== textual matches ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'constructor-field-shorthand|constructor-field|`#expression`|constructor_field|shorthand' .
printf '\n== possible corpus/tests ==\n'
git ls-files | rg 'corpus|test|spec|fixture|examples|playground|samples' | sed -n '1,200p'Repository: BoundaryML/baml
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== grammar.js excerpt ==\n'
sed -n '850,920p' typescript2/pkg-grammar-treesitter/grammar.js
printf '\n== parser.rs excerpt ==\n'
sed -n '5340,5405p' baml_language/crates/baml_compiler_parser/src/parser.rs
printf '\n== corpus examples ==\n'
sed -n '180,240p' typescript2/pkg-grammar-treesitter/test/corpus/expressions.txtRepository: BoundaryML/baml
Length of output: 6240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob 'typescript2/pkg-grammar-treesitter/test/corpus/*.txt' \
-A4 -B4 'constructor_field_shorthand|constructor_field|object|constructor' \
typescript2/pkg-grammar-treesitter/test/corpus
printf '\n== likely shorthand examples in BAML tests ==\n'
rg -n --hidden --glob 'baml_language/crates/baml_lsp2_actions_tests/test_files/**/*.baml' \
'\{[^{}]*[A-Za-z_][A-Za-z0-9_]*[[:space:]]*\}' \
baml_language/crates/baml_lsp2_actions_tests/test_filesRepository: BoundaryML/baml
Length of output: 32647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '185,245p' typescript2/pkg-grammar-treesitter/test/corpus/expressions.txtRepository: BoundaryML/baml
Length of output: 1605
Widen constructor-field-shorthand to match real expression starts.
constructor_field_shorthand accepts a full $._expression after the field name, but this begin rule only matches literal starts. Shorthand fields whose value starts with an identifier, call, namespace/type path, nested object/list, or lambda will miss variable.other.property.baml.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typescript2/pkg-grammar/baml.tmLanguage.json` around lines 1643 - 1645,
Update the grammar rule referenced by the constructor-field-shorthand include so
its begin pattern accepts all valid $._expression starts, including identifiers,
calls, namespace/type paths, nested objects or lists, and lambdas, rather than
only literal starts. Preserve the existing property scope and constructor-field
shorthand matching behavior.
Comment on lines
+272
to
+294
| it("marks contexts entered by dynamic rules as dynamic", () => { | ||
| const contextsByName = new Map( | ||
| find(language, "context").map((el) => [el.attributes.name, el]), | ||
| ); | ||
|
|
||
| for (const contextsBlock of find(language, "contexts")) { | ||
| walk(contextsBlock, (el) => { | ||
| if (el.name === "context" || el.name === "contexts") return; | ||
| // A rule whose String captures a group and whose target context uses | ||
| // %N substitution must land in a dynamic="true" context. | ||
| const target = referencedContextName(el.attributes.context ?? ""); | ||
| if (target === null) return; | ||
| const targetContext = contextsByName.get(target); | ||
| if (!targetContext) return; | ||
| const usesSubstitution = find(targetContext, "RegExpr").some( | ||
| (rule) => rule.attributes.dynamic === "true", | ||
| ); | ||
| if (usesSubstitution && targetContext.attributes.dynamic !== "true") { | ||
| throw new Error(`context "${target}" has dynamic rules but is not dynamic="true"`); | ||
| } | ||
| }); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dynamic-context check has incomplete coverage.
This only validates contexts that are the context= target of some other rule. Contexts reached only via lineEndContext/lineEmptyContext/fallthroughContext, or never explicitly referenced by a context= attribute at all, are skipped — so a context with an unmarked dynamic RegExpr rule could slip through undetected, defeating the test's stated purpose.
Iterate contexts directly instead:
♻️ Proposed fix
- it("marks contexts entered by dynamic rules as dynamic", () => {
- const contextsByName = new Map(
- find(language, "context").map((el) => [el.attributes.name, el]),
- );
-
- for (const contextsBlock of find(language, "contexts")) {
- walk(contextsBlock, (el) => {
- if (el.name === "context" || el.name === "contexts") return;
- // A rule whose String captures a group and whose target context uses
- // %N substitution must land in a dynamic="true" context.
- const target = referencedContextName(el.attributes.context ?? "");
- if (target === null) return;
- const targetContext = contextsByName.get(target);
- if (!targetContext) return;
- const usesSubstitution = find(targetContext, "RegExpr").some(
- (rule) => rule.attributes.dynamic === "true",
- );
- if (usesSubstitution && targetContext.attributes.dynamic !== "true") {
- throw new Error(`context "${target}" has dynamic rules but is not dynamic="true"`);
- }
- });
- }
- });
+ it("marks contexts entered by dynamic rules as dynamic", () => {
+ for (const context of find(language, "context")) {
+ const usesSubstitution = find(context, "RegExpr").some(
+ (rule) => rule.attributes.dynamic === "true",
+ );
+ expect(
+ !usesSubstitution || context.attributes.dynamic === "true",
+ `context "${context.attributes.name}" has dynamic rules but is not dynamic="true"`,
+ ).toBe(true);
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("marks contexts entered by dynamic rules as dynamic", () => { | |
| const contextsByName = new Map( | |
| find(language, "context").map((el) => [el.attributes.name, el]), | |
| ); | |
| for (const contextsBlock of find(language, "contexts")) { | |
| walk(contextsBlock, (el) => { | |
| if (el.name === "context" || el.name === "contexts") return; | |
| // A rule whose String captures a group and whose target context uses | |
| // %N substitution must land in a dynamic="true" context. | |
| const target = referencedContextName(el.attributes.context ?? ""); | |
| if (target === null) return; | |
| const targetContext = contextsByName.get(target); | |
| if (!targetContext) return; | |
| const usesSubstitution = find(targetContext, "RegExpr").some( | |
| (rule) => rule.attributes.dynamic === "true", | |
| ); | |
| if (usesSubstitution && targetContext.attributes.dynamic !== "true") { | |
| throw new Error(`context "${target}" has dynamic rules but is not dynamic="true"`); | |
| } | |
| }); | |
| } | |
| }); | |
| it("marks contexts entered by dynamic rules as dynamic", () => { | |
| for (const context of find(language, "context")) { | |
| const usesSubstitution = find(context, "RegExpr").some( | |
| (rule) => rule.attributes.dynamic === "true", | |
| ); | |
| expect( | |
| !usesSubstitution || context.attributes.dynamic === "true", | |
| `context "${context.attributes.name}" has dynamic rules but is not dynamic="true"`, | |
| ).toBe(true); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@typescript2/pkg-grammar/tests/kde-syntax.test.ts` around lines 272 - 294,
Update the dynamic-context test in “marks contexts entered by dynamic rules as
dynamic” to iterate every context from contextsByName directly, rather than only
rules’ context= targets. For each context, detect dynamic RegExpr rules and
assert that the same context has dynamic="true", covering contexts reached
through lineEndContext, lineEmptyContext, fallthroughContext, or not explicitly
referenced.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the grammar mirror pipeline (#3986) so every syntax-highlighting registry on the internet is fed from this monorepo, with mirrors as write-only build artifacts. Follows the plan in
typescript2/pkg-grammar/DISTRIBUTION.md(included).What's here
pkg-grammar(extended)baml.sublime-syntaxgenerated from the TextMate grammar byscripts/emit-sublime.ts(wired intobuild, drift-guard test). Validated against real syntect:batrenders every fixture with zero errors.syntaxes/baml.xml— hand-authored KDE KSyntaxHighlighting definition (Kate, Pandoc via skylighting). Validates against the official KDElanguage.xsd. Dynamic rules support arbitrary raw-string#depth.tests/fixtures/showcase__golden_sample.baml— canonical showcase sample, mirrored assamples/baml.samplefor registry submissions (Shiki etc.).SUPPORT.mdregistry matrix, frozen-path contract + read-only footer in the mirror README,DISTRIBUTION.mdrollout playbook.pkg-grammar-hljs(new) → mirrors to BoundaryML/baml-highlightjs, npm@boundaryml/baml-highlightjs{{ }}/{% %}highlighting inside prompt bodies, raw/backtick/byte strings, full keyword taxonomy from the real lexer.pkg-grammarfixture (82 tests); browser/CDN dist build generated at mirror-assembly time.pkg-grammar-treesitter(new) → mirrors to BoundaryML/baml-treesitter (nvim-treesitter / Zed / Helix)let/constdestructuring, backtick${}bodies). No external scanner.BoundaryML/tree-sitter-baml(15 months stale) is untouched; the Zed pin migrates later.Automation
sync-grammar-mirror.ymlfans out to all three mirrors; npm-published mirrors getv*tags (bat/Package Control/Linguist pinning).grammar-testsCI runs all three packages as a conformance gate: every port validates against the shared fixture corpus, so a language change a port doesn't handle fails the PR.Bootstrap status (already done via gh)
BoundaryML/baml-highlightjs+BoundaryML/baml-treesittercreated and seeded with assembled content (hljs taggedv0.1.0)BAML_HIGHLIGHTJS_DEPLOY_KEY/BAML_TREESITTER_DEPLOY_KEYsecrets set on this reponpm publishof@boundaryml/baml-highlightjs(v0.1.0 on npm) + trusted publisher configured🤖 Generated with Claude Code
Summary by CodeRabbit
:) in syntax highlighting.