Pin the BXL authoring skill's claims against the engine - #5819
Open
habdelra wants to merge 7 commits into
Open
Conversation
Card authors get no guard rails from the engine: it tolerates missing values, catches Excel error sentinels, and compiles a readable spreadsheet dialect into jq, so several ways of getting an expression wrong produce a plausible number instead of an error. The skill ships through the boxel-cli plugin, next to the glossary whose bxl entries name it. A BXL suite pins each claim twice — the snippet still appears in the skill, and it still behaves as described — and asserts the repo paths the skill cites resolve. The bxl CI filter now covers the skill directory so an edit there runs that suite. Also corrects two syntax-modes examples that handed an iterating path straight to SUM, and points its import line at the platform module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a70971b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An independent review found three claims that were wrong and two rules stated more broadly than they hold: - ROUND absorbs a null operand but not an Excel sentinel: a sentinel propagates out and blanks the whole expression. - Only sentinels blank a field. A misspelled function name or a structural op on the wrong shape throws a non-sentinel error and the instance indexes as an error, so a formula typo is a broken card. - Of the self-enumeration ops, only value-reading ones re-enter. keys reads field names and never re-enters; unique over an object throws. - The collect rule applies to the argument that iterates. A comma list is collected by the compiler, an array-valued field is already one value, and wrapping a scalar parameter blanks the field. - Both tags preserve the raw source, so the requirement is a tag, not the jq tag specifically. A PascalCase label inside an interpolation is read as a function call and throws on first read. The checklist restates those narrower rules, #NAME? leaves the sentinel list (nothing raises it), and modulemeta leaves the refusal list (it does not parse, so it has no derive diagnostic). The guard grew to match. It now evaluates every allowed form with an expected value instead of only constructing it — construction proved nothing, since a name the registry has never heard of constructs too, and NPV and isEmail live in lazy chunks that were never loaded. List membership, the WRONG/RIGHT pairing, the tag and blank-input reason columns, and the doc filenames are pinned, and the header states what the guard cannot reach rather than implying it covers the prose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new bxl-authoring skill for card authors and pins its behavioral claims against the BXL engine to prevent documentation drift.
Changes:
- Add
bxl-authoringskill content for BXL authoring/guard rails in cards. - Add a drift-guard test suite that asserts the skill’s examples still exist and still behave as described.
- Update BXL docs + CI path filters so skill edits run the guarding suite.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bxl/tests/boxel/authoring-skill-claims.ts | New drift-guard suite that pins skill snippets and validates described engine behavior. |
| packages/bxl/tests/boxel/README.md | Document the new drift-guard suite in the Boxel BXL tests index. |
| packages/bxl/docs/syntax-modes.md | Fix aggregate examples and document the “collect before aggregate” rule + import guidance. |
| packages/boxel-cli/plugin/skills/bxl-authoring/SKILL.md | New authoring skill content describing traps, profiles, and review checklist. |
| .github/workflows/ci.yaml | Ensure CI runs BXL suite when the skill file changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The two lists are read as their backticked spans and compared as sets against the names the cases cover, so an entry added without a case, dropped, or moved between the lists fails here. Exact tokens rather than substrings: a short name like `env` cannot be satisfied by a longer word containing it, or by appearing inside a neighbouring code span. That brings @env, $old, stderr, halt and bare `=` under evaluation alongside the rest, and each allowed form now evaluates once per case rather than twice, so a throw and a wrong value are told apart by where the failure lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A card-authoring skill has to reach both agent harnesses, and the monorepo's plugin tree reaches only one. cardstack/boxel-skills is the single source: build:skills copies skills/<name>/SKILL.md from a pinned tag into the boxel-cli plugin, and the same repo syncs to the skills realm, where a Skill card makes the file loadable in the in-app AI assistant. The nine hand-authored skills in plugin/skills/ are CLI-command docs and none carry the boxel.kind marker. What stays here is the half the monorepo owns: the suite that pins the skill's claims against the engine, the syntax-modes.md corrections, and the change filter. The suite reads the copy build:skills brings in, so bumping BOXEL_SKILLS_VERSION to a tag carrying the skill is what turns it green — verified locally by staging the upstream file at the copy's path, 31/31. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
habdelra
changed the title
habdelra
marked this pull request as draft
August 19, 2026 15:48
burieberry
reviewed
Aug 19, 2026
burieberry
left a comment
burieberry
left a comment
Contributor
There was a problem hiding this comment.
[Claude Code 🤖] Automated review — 3 findings, each verified against the branch. Details inline.
The package's front-page "Using BXL inside Boxel" section taught the
relative-bundle import as the only form and an uncollected
SUM("Line Item".Amount) — which, with no schema, is the form that throws
"Cannot index string with string" on the first read. It now names the
platform module and collects the iterating argument, matching the
syntax-modes guidance.
The bxl filter gains the host paths the drift guard asserts exist.
bxl-test runs unconditionally on main, so a host-only rename of a cited
suite would otherwise merge green and redden main on a commit whose own
CI never ran the failing suite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
habdelra
marked this pull request as ready for review
August 19, 2026 16:03
Comparing the skill's refusal list against this suite's own cases is a closed loop: a ban neither enumerated is invisible to both. That is how isAfter and isBefore stayed missing from a page that blesses the validator helpers they sit among — the derive profile's volatile set is six calls, not the four the skill named, because VOLATILE_VALIDATION_FUNCTIONS folds those two in. The new case walks BXL_DERIVE_DENIED_CALLS and requires every denied call to be either named in the skill's list or waived here with the category the skill describes it by, and it checks each waiver against categoryForBxlFunction so a waiver cannot outlive its reason. A ban the engine gains now fails until someone decides which it is — verified by injecting one. isAfter, isBefore and isDate gain cases of their own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lukemelia
approved these changes
Aug 19, 2026
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.
The BXL engine gives card authors no guard rails: it tolerates missing values, catches spreadsheet error values rather than throwing, and compiles a readable spreadsheet dialect down to jq, so several of the ways to get an expression wrong land a plausible value in the field instead of an error. The authoring skill for that surface belongs in
cardstack/boxel-skills, which is the single source that reaches both agent harnesses — the boxel-cli plugin viabuild:skills, and the in-app AI assistant via the skills-realm sync. This PR is the half the monorepo owns: proof that what the skill teaches is what the engine does.The drift guard
packages/bxl/tests/boxel/authoring-skill-claims.ts— 31 cases against the copybuild:skillsbrings in. Each asserts both halves of a claim: the snippet still appears in the skill, and the behavior it describes still holds against the engine. Whitespace is normalized before matching, so rewrapping prose or realigning a table is not a failure — only changing what an example says is.Substring matching alone is too weak for that, so the pinning is shaped against the ways a skill actually drifts:
envcan't be satisfied by a longer word containing it.*.mdfilenames as well.NPVandisEmailwould be checked against a narrower library than a card gets.Two things the guard cannot reach, stated in its header rather than implied away: prose, and anything needing a live card runtime. Query-backed inverse staleness and the
{ id }clip across a cycle are pinned by the host integration suites the skill names, and the path case checks those pointers resolve; the stale-paint interaction with Glimmer's render flush is not pinned anywhere — it follows from runloop ordering, and what is pinned is thememoize: falsemitigation the skill gives.The
bxlCI change filter now covers the skill's directory, so once the copy lands, editing it runs this suite.Engine-doc corrections
Two
packages/bxl/docs/syntax-modes.mdexamples handed an iterating path straight toSUM—SUM(Patients[].Billing.RoomCharge)callsSUMonce per patient and yields one output per patient rather than a total — and one used a quoted multi-word label, which throws without a schema. Both corrected, with the collect rule and its two boundaries (comma lists collect themselves; scalar parameters must not be wrapped, orROUND([1.234], 2)blanks the field) added to the cross-cutting behaviors list. The doc's import line now names the platform module@cardstack/bxl, keeping the relative-bundle form for realms that carry their own.Test plan
pnpm testinpackages/bxl: 69/69 suites with the skill copy staged at its build output path, including the new one at 31/31.pnpm lintinpackages/bxl: eslint and types clean.reducebetween the lists, swapping the WRONG/RIGHT aggregate examples, renaming a cited doc, falsifying a reason cell in either table, adding an unpinned entry to a list, and listing an error value the engine never raises each fail their case with a pointer to the line to update.