Repository navigation
More stdlib matrix fixes - #4361
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ 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):
|
Contributor
📝 WalkthroughWalkthroughThe changes revise symbol identity, CLI search, stdlib-matrix alignment and extraction, workflow credentials, and report loading. They add impl-qualified symbol resolution, stronger finding validation, safer extraction, deterministic search, and report format and URL guards. ChangesSymbol identity
CLI search
Stdlib matrix alignment
Report loading validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/stdlib-matrix/baml_src/toolbox.baml (1)
364-369: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the cached skill before returning it.
The fallback returns cached content without calling
looks_like_skill. A cache written by an older version, or otherwise corrupted, can still be HTML or unrelated text. The pipeline then sends that content to every alignment session.Read the cache, validate it, and throw the existing missing-skill error when it is invalid. Add a test for an invalid cached body.
🤖 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 `@tools/stdlib-matrix/baml_src/toolbox.baml` around lines 364 - 369, Update the cached fallback in the skill-loading flow to read the cache, pass its contents through looks_like_skill, and return it only when validation succeeds; otherwise throw the existing missing-skill error. Add a test covering an invalid cached body and confirming the error path.
🧹 Nitpick comments (4)
baml_language/crates/baml_surface/src/ids.rs (2)
317-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider rejecting non-method kinds in the impl-owned form.
The match accepts
T:(int as baml.ops.Add<bigint>)andV:(int as baml.ops.Add<bigint>)as valid ids withmember: None. No producer emits that form:of_symbolreturnsNoneforSymbol::Impl, andexport_impluses theX:{pkg}.impl[...]form instead.resolvealso returnsNonefor an impl owner without a member. The parser therefore accepts ids that can never be generated and never resolve.Restrict the impl-owned form to
IdKind::Methodso parsing and generation agree.♻️ Proposed tightening
- let member = match (kind, tail) { - (IdKind::Method, _) if !tail.is_empty() => { - let name = tail.strip_prefix('.').ok_or_else(invalid)?; - if name.is_empty() || name.contains('.') { - return Err(invalid()); - } - Some(name.to_string()) - } - (_, "") => None, - _ => return Err(invalid()), - }; - if kind.is_member() && member.is_none() { - return Err(invalid()); - } + // An impl block owns methods and nothing else: no fields, no + // variants, no associated types, and no item-kind id of its own — + // the export layer writes blocks in the `X:` form. + if kind != IdKind::Method { + return Err(invalid()); + } + let name = tail.strip_prefix('.').ok_or_else(invalid)?; + if name.is_empty() || name.contains('.') { + return Err(invalid()); + } + let member = Some(name.to_string());🤖 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 `@baml_language/crates/baml_surface/src/ids.rs` around lines 317 - 330, Update the match constructing member in the ID parser to reject impl-owned forms for non-method IdKind values, allowing only IdKind::Method with a valid member name while preserving the existing no-member handling for other owner forms. Ensure parsed IDs remain consistent with of_symbol, export_impl, and resolve behavior.
475-477: 🚀 Performance & Scalability | 🔵 TrivialImpl resolution renders every project impl per lookup.
The scan calls
impl_ownerfor each project impl and compares rendered strings. Each call runsimpl_dataand canonical type rendering. A caller that resolves many ids in a loop pays O(ids × impls) rendering work. If impl-owned resolution becomes a hot path, add a salsa-tracked map from renderedOwner::ImpltoImpland look up by key.🤖 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 `@baml_language/crates/baml_surface/src/ids.rs` around lines 475 - 477, Optimize impl resolution in the lookup containing Self::impl_owner by adding a Salsa-tracked map from rendered Owner::Impl keys to their Impl handles, then resolve self.owner through that map instead of scanning project_impls and rendering every owner per lookup. Preserve the existing unresolved-result behavior when no matching key exists.baml_language/crates/baml_surface/src/ids_tests.rs (1)
101-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative parse cases for the impl-owned form.
The two cases cover successful parsing only.
parse_impl_ownedadds several rejection paths that no test exercises: an unbalanced(, a missingas, an empty for-type or interface, a member containing., and a member kind other thanMethod. Add a table-driven test so a future change tomatching_parenorsplit_ascannot silently widen the accepted set.🧪 Proposed test
/// The rejection paths, so a looser parser cannot pass unnoticed. #[test] fn malformed_impl_owned_ids_are_rejected() { for id in [ "M:(int as baml.ops.Add<bigint>", // unbalanced "M:(int baml.ops.Add<bigint>).add", // no ` as ` "M:( as baml.ops.Add<bigint>).add", // empty for-type "M:(int as ).add", // empty interface "M:(int as baml.ops.Add<bigint>).a.b", // dotted member "M:(int as baml.ops.Add<bigint>)", // member kind, no member "F:(int as baml.ops.Add<bigint>).x", // a block owns no fields ] { assert!( SymbolId::from_str(id).is_err(), "{id} must not parse" ); } }🤖 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 `@baml_language/crates/baml_surface/src/ids_tests.rs` around lines 101 - 123, Add a table-driven test alongside impl_owned_ids_parse_around_nested_projections named malformed_impl_owned_ids_are_rejected, covering unbalanced parentheses, missing ` as `, empty for-type or interface, dotted members, missing member, and a non-Method kind such as `F`. Assert SymbolId::from_str returns an error for every case.baml_language/crates/baml_surface/src/export.rs (1)
532-534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or rewrite the stale doc comment on
export_impl.Lines 532-533 describe an interface rendering, not this function, and the block ends with an empty
///line. The text is a leftover from a removed helper. Replace it with a description ofexport_impl.♻️ Proposed doc fix
-/// The interface an impl names, with its arguments: `baml.ops.Add<bigint>`. -/// +/// Export one impl block, keyed by its canonical owner rendering: +/// `X:{pkg}.impl[{interface}<args> for {for_ty}]`. fn export_impl(db: &dyn Db, imp: Impl<'_>) -> Option<ImplExport> {🤖 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 `@baml_language/crates/baml_surface/src/export.rs` around lines 532 - 534, Replace the stale doc comment immediately above export_impl with documentation describing what export_impl does, and remove the empty documentation line. Do not retain the unrelated interface-rendering text.
🤖 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 `@baml_language/crates/baml_cli/src/describe_search.rs`:
- Around line 39-60: Update query handling around query_tokens so relevance
filtering does not eliminate exact-name searches for short tokens or STOP_WORDS
such as get, set, and id. Preserve the filtered tokens for scoring, but retain
the original normalized query tokens separately and use them for exact
symbol-name matching in search; add regression tests covering these exact names.
- Line 17: Update the final module documentation sentence in describe_search.rs
to clearly state that concept-based lookup requires a semantic index, which this
search does not provide.
- Around line 275-279: Update the candidate path prefix in the logic around
names.push to use the resolver’s root prefix so user-package symbols produce
resolvable paths. Add a unit test covering search for a user symbol and
verifying that the returned path resolves successfully.
In `@baml_language/crates/baml_surface/src/ids.rs`:
- Around line 346-354: Update the doc comment for `of_symbol` to remove the
stale claim that free-impl methods return `None`; document that every
impl-contributed method, including free-impl methods, receives an impl-qualified
ID via `impl_member_id`, while preserving the existing behavior description for
other symbols.
- Around line 473-486: Filter the methods used by the Owner::Impl resolution
branch before matching member_name so inherited interface default methods from
imp.all_methods(db) are excluded. Reuse the existing ownership/contributing-impl
distinction, ensuring only methods belonging directly to the matched
implementation resolve through the impl path and generation remains the inverse
of resolution.
In `@tools/stdlib-matrix/baml_src/align.baml`:
- Around line 556-588: Update finding_fault to reject pairing verdicts whose
finding.baml list is empty before the existing validation and compilation
checks. Require match or divergent findings to name at least one counterpart,
return a clear validation error, and preserve the current handling for
non-pairing findings.
In `@tools/stdlib-matrix/baml_src/revision.baml`:
- Around line 309-320: The carrying check in the judgement loop must recursively
validate every reachable BAML reference, not only each direct reference. Replace
the direct is_fresh call around judgement.baml and references.get with a
cycle-safe traversal that tracks visited IDs and sets carries false when any
reachable symbol is missing or stale; preserve successful results for
already-visited cycles.
---
Outside diff comments:
In `@tools/stdlib-matrix/baml_src/toolbox.baml`:
- Around line 364-369: Update the cached fallback in the skill-loading flow to
read the cache, pass its contents through looks_like_skill, and return it only
when validation succeeds; otherwise throw the existing missing-skill error. Add
a test covering an invalid cached body and confirming the error path.
---
Nitpick comments:
In `@baml_language/crates/baml_surface/src/export.rs`:
- Around line 532-534: Replace the stale doc comment immediately above
export_impl with documentation describing what export_impl does, and remove the
empty documentation line. Do not retain the unrelated interface-rendering text.
In `@baml_language/crates/baml_surface/src/ids_tests.rs`:
- Around line 101-123: Add a table-driven test alongside
impl_owned_ids_parse_around_nested_projections named
malformed_impl_owned_ids_are_rejected, covering unbalanced parentheses, missing
` as `, empty for-type or interface, dotted members, missing member, and a
non-Method kind such as `F`. Assert SymbolId::from_str returns an error for
every case.
In `@baml_language/crates/baml_surface/src/ids.rs`:
- Around line 317-330: Update the match constructing member in the ID parser to
reject impl-owned forms for non-method IdKind values, allowing only
IdKind::Method with a valid member name while preserving the existing no-member
handling for other owner forms. Ensure parsed IDs remain consistent with
of_symbol, export_impl, and resolve behavior.
- Around line 475-477: Optimize impl resolution in the lookup containing
Self::impl_owner by adding a Salsa-tracked map from rendered Owner::Impl keys to
their Impl handles, then resolve self.owner through that map instead of scanning
project_impls and rendering every owner per lookup. Preserve the existing
unresolved-result behavior when no matching key exists.
🪄 Autofix
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 Plus
Run ID: 94d7158d-9e00-4e0f-b081-1874efa672af
⛔ Files ignored due to path filters (1)
baml_language/crates/baml_cli/src/snapshots/baml_cli__help_command__tests__describe_detailed_help.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
.github/workflows/stdlib-matrix.ymlbaml_language/crates/baml_cli/src/describe_command.rsbaml_language/crates/baml_cli/src/describe_search.rsbaml_language/crates/baml_surface/src/export.rsbaml_language/crates/baml_surface/src/ids.rsbaml_language/crates/baml_surface/src/ids_tests.rstools/stdlib-matrix/README.mdtools/stdlib-matrix/baml_src/align.bamltools/stdlib-matrix/baml_src/entry.bamltools/stdlib-matrix/baml_src/models.bamltools/stdlib-matrix/baml_src/ratchet.bamltools/stdlib-matrix/baml_src/render.bamltools/stdlib-matrix/baml_src/report.bamltools/stdlib-matrix/baml_src/revision.bamltools/stdlib-matrix/baml_src/symbols.bamltools/stdlib-matrix/baml_src/tests.bamltools/stdlib-matrix/baml_src/toolbox.bamltools/stdlib-matrix/baml_src/ts_symbols.bamltools/stdlib-matrix/extractors/ts-surface.mjstools/stdlib-matrix/runtypescript2/app-stdlib-matrix/src/components/matrix-app.ts
💤 Files with no reviewable changes (1)
- tools/stdlib-matrix/baml_src/models.baml
Binary size checks passed✅ 7 passed
Generated by |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_surface/src/ids_tests.rs (1)
66-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComplete the inherited-default ID checks.
an_inherited_default_is_reachable_through_the_block_and_declared_oncedoes not assert string round-trips. Itsgreetcheck only testsis_some(). Assert the exact method and re-derived ID forgreet, and round-trip both IDs.🤖 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 `@baml_language/crates/baml_surface/src/ids_tests.rs` around lines 66 - 114, Update an_inherited_default_is_reachable_through_the_block_and_declared_once to round-trip both emitted IDs through SymbolId::from_str and compare their canonical strings. Strengthen the greet assertion to verify the resolved method is exactly greet and that SymbolId::of_symbol re-derives the same ID, matching the existing checks for the other inherited method.
🧹 Nitpick comments (1)
baml_language/crates/baml_cli/src/describe_search.rs (1)
530-536: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest the complete user-package path contract.
This test validates
addressable_packageonly. It does not verify thatcandidates()emitsroot.paths or thatdescribecan resolve a returned user-symbol path. Add a Rust unit test that constructs a user-package export, searches it, and resolves the returned path.As per coding guidelines:
**/*.rs: Prefer writing Rust unit tests over integration tests where possible.🤖 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 `@baml_language/crates/baml_cli/src/describe_search.rs` around lines 530 - 536, Extend the Rust unit test near `a_user_symbol_is_printed_as_a_path_describe_accepts` to construct a user-package export, invoke the search flow’s `candidates()`, and pass the returned user-symbol path through `describe` resolution. Assert that candidates emit the `root.`-prefixed path and that resolving it succeeds, covering the complete user-package path contract rather than only `addressable_package`.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@baml_language/crates/baml_surface/src/ids_tests.rs`:
- Around line 66-114: Update
an_inherited_default_is_reachable_through_the_block_and_declared_once to
round-trip both emitted IDs through SymbolId::from_str and compare their
canonical strings. Strengthen the greet assertion to verify the resolved method
is exactly greet and that SymbolId::of_symbol re-derives the same ID, matching
the existing checks for the other inherited method.
---
Nitpick comments:
In `@baml_language/crates/baml_cli/src/describe_search.rs`:
- Around line 530-536: Extend the Rust unit test near
`a_user_symbol_is_printed_as_a_path_describe_accepts` to construct a
user-package export, invoke the search flow’s `candidates()`, and pass the
returned user-symbol path through `describe` resolution. Assert that candidates
emit the `root.`-prefixed path and that resolving it succeeds, covering the
complete user-package path contract rather than only `addressable_package`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fcbae0e1-a651-4bc8-b385-267b9b4ac1d0
📒 Files selected for processing (9)
baml_language/crates/baml_cli/src/describe_search.rsbaml_language/crates/baml_surface/src/export.rsbaml_language/crates/baml_surface/src/export_tests.rsbaml_language/crates/baml_surface/src/ids.rsbaml_language/crates/baml_surface/src/ids_tests.rstools/stdlib-matrix/baml_src/align.bamltools/stdlib-matrix/baml_src/references.bamltools/stdlib-matrix/baml_src/revision.bamltools/stdlib-matrix/baml_src/tests.baml
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/stdlib-matrix/baml_src/revision.baml
- tools/stdlib-matrix/baml_src/tests.baml
- baml_language/crates/baml_surface/src/ids.rs
- tools/stdlib-matrix/baml_src/align.baml
2kai2kai2
enabled auto-merge
August 11, 2026 22:41
2kai2kai2
temporarily deployed
to
boundary-tools-prod
August 11, 2026 22:41 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation