Repository navigation
feat(toolchain): toolchain-owned agent-skill warnings, codeload installs, and baml-old_skills archive - #4001
Merged
Merged
Conversation
…helpers New baml_release::skills module shared by the wrapper and baml-cli: skill repo URL builders (env-overridable for mirrors/tests), [skills] provenance state in state.toml written via read-modify-write so it never clobbers the wrapper-owned [channels.*] sections, a latest-commit freshness cache, and a GitHub API fetch for the skill repo head commit. Co-authored-by: Cursor <[email protected]>
…rovenance baml agent install now defaults to the current head of BoundaryML/baml-skill: it resolves the main commit SHA first, downloads the tarball pinned to that exact commit, and records [skills] provenance in state.toml (plus the freshness cache) after a successful install. The --latest flag is removed since it is the standard behavior; --from remains for custom sources and the toolchain-release snapshot survives only behind BAML_AGENT_SKILLS_RELEASE_VERSION. Includes an e2e test that runs the real binary against a stubbed GitHub server and asserts the recorded provenance. Co-authored-by: Cursor <[email protected]>
…ated The wrapper now surfaces staleness on normal commands: before exec'ing baml-cli it compares the recorded channel version against the cached channel manifest and the [skills] provenance against the cached latest skill commit, printing a styled bold-yellow warning: line on stderr when either is behind. Caches auto-refresh over the network at most once per 24h window (5s timeout, silent on failure, throttled by a .last-check marker so offline machines do not retry every command), activating the previously dead [update] auto_check config flag with default-on/opt-out semantics. toolchain status gains a skill section and toolchain use/update do a best-effort skill check afterward. The wrapper's State struct now round-trips foreign state.toml sections so it cannot drop the CLI-written [skills] entry. Covered by unit tests plus an e2e suite that runs the real wrapper binary against a temp BAML_HOME. Co-authored-by: Cursor <[email protected]>
Projects without any baml-* agent skills now get a warning on every command (No baml skill is installed, set it up with baml agent install), instead of staying silent. Projects with skills keep the upgrade warning when provenance is missing or behind the cached latest commit; the project-skills check is now the primary gate, so global provenance no longer triggers upgrade warnings in skill-less projects. Decision logic extracted into a pure skill_warning_message helper with unit tests, plus updated e2e coverage. Co-authored-by: Cursor <[email protected]>
CI runs clippy with -D warnings, which rejected the numeric casts in the skills date helper (cast_possible_wrap/truncation/sign_loss), a nonminimal_bool in file_older_than, and format_push_string in the e2e test. Replace the casts with try_from with in-range fallbacks and simplify the boolean with is_none_or. Co-authored-by: Cursor <[email protected]>
…se pipeline
agent install now pulls the skill repo head directly from GitHub, so the
versioned baml-agent-skills-{version}.tar.gz release asset has no
consumers. Remove the build-agent-skills job, the packaging script, and
the BAML_AGENT_SKILLS_RELEASE_VERSION escape hatch that pointed at it.
Co-authored-by: Cursor <[email protected]>
- agent install --from now clears recorded provenance instead of leaving a stale commit that misreports custom content as current - write_skills_state treats only a missing state.toml as empty; corrupt or unreadable files error instead of being silently replaced - passive cache refreshes share one 5s wall-clock budget instead of up to 5s each - skill detection requires a baml-* directory containing SKILL.md, so leftover empty directories don't suppress the install prompt Co-authored-by: Cursor <[email protected]>
…uto-check The passive path inlined its own fetch-then-persist sequence; route it through the same helper the explicit toolchain commands use so there is exactly one fetch-and-cache path and one compare-and-warn path, with trigger policy (TTL-gated vs forced) as the only difference. Co-authored-by: Cursor <[email protected]>
…ad of before it Warnings from existing caches still print immediately (no network). When a refresh is due (at most once per TTL window), it now runs on a background thread while baml-cli executes; the wrapper spawns and waits instead of exec'ing on that rare path, joins the refresh with whatever remains of the 5s budget after the command exits, and prints only warnings the fresh caches newly justify (never duplicating one already shown). The common no-refresh case keeps the zero-overhead exec path. Co-authored-by: Cursor <[email protected]>
…Hub API baml agent install resolved the skill repo head via an unauthenticated GET to api.github.com, whose 60-requests/hour per-IP rate limit made installs hard-fail with HTTP 403 on shared-IP environments (CI, agent sandboxes) — with no fallback and no offline path. The REST call only existed to learn the head SHA before downloading the tarball from codeload.github.com, which is not API-rate-limited and needs no credentials. Drop it: download the main-branch tarball directly, and recover the exact commit for provenance from the pax global header (comment=<sha>) that git archive embeds in every GitHub tarball. Archives without the header (custom mirrors) install fine and just skip provenance, matching --from semantics. The download error now points at the --from escape hatch. The wrapper's passive freshness check still uses the REST API, but it is throttled to once per day and silent on failure, so a rate limit there can no longer block anything. Co-Authored-By: Claude Fable 5 <[email protected]>
… dir Without --dir, baml agent install picked its root via an unbounded ancestor walk that accepted any directory owning a bare baml_src/. A stray ~/baml_src made installs run from ~/some/project land the skills in $HOME instead of the project. Detection is now scoped to where a project can plausibly be: inside a git repo the walk never leaves the repo (nearest baml.toml or baml_src owner, else the repo root, where agent harnesses discover .claude/skills); outside one it stops before the home directory and falls back to the current directory. The decision lives in a pure detect_install_root_in so the boundaries are unit-testable without env or cwd mutation. Co-Authored-By: Claude Fable 5 <[email protected]>
Co-Authored-By: Claude Fable 5 <[email protected]>
Two review findings on the bounded walk: a git toplevel rooted at (or containing) the home directory — dotfiles worktrees, home-as-git-repo — slipped past the ancestor filter and reopened the stray-~/baml_src hole, and with no determinable home directory the non-git walk was unbounded to the filesystem root. Reject home-containing toplevels, and without a home boundary don't walk above cwd at all: installing at cwd is always safe. Also satisfies clippy's unnecessary_to_owned in the test archive helper. Co-Authored-By: Claude Fable 5 <[email protected]>
Move the skill warning decision (missing vs outdated), the cwd-to-home project-skills walk, the TTL/marker refresh throttle, and the [update] auto_check config gate into baml_release::skills so the toolchain binary can own the passive skill check. Co-Authored-By: Claude Fable 5 <[email protected]>
…ommands Print the passive skill warning before the core authoring commands (run, generate, pack) from the local caches, and refresh the latest-commit cache in the background at most once per TTL window. Living in the toolchain means the warning ships with every nightly instead of waiting on a wrapper release; the whitelist keeps the nag out of machine-facing and utility invocations. The exit-code e2e harness now isolates BAML_HOME (with auto-check off) so CLI tests neither read the developer's real ~/.baml nor touch the network, and the clean-stderr run test installs skills in its fixture. Co-Authored-By: Claude Fable 5 <[email protected]>
The toolchain binary now owns the passive skill warning and the latest-commit cache refresh, so remove the wrapper's copy: the pass-through warning, the post-toolchain-change nudge, the skill half of the lazy refresh, and the skill section of toolchain status. None of it had shipped (added in #3970 without a wrapper version bump), so the published wrapper's behavior is unchanged and no wrapper release is needed for the warning to reach users. Co-Authored-By: Claude Fable 5 <[email protected]>
baml init sets up the project quietly, the first authoring command (generate) warns that no skill is installed, a plain baml agent install resolves the project root from the cwd and installs .claude/skills and .agents/skills from the codeload-shaped tarball stub (which 404s anything but the archive endpoint, so a regression to the GitHub REST API fails the test), and the next authoring command is quiet. Co-Authored-By: Claude Fable 5 <[email protected]>
…nism # Conflicts: # baml_language/crates/baml_cli/tests/agent_install_e2e.rs
Several doc comments still described the wrapper as the component that refreshes the latest-commit cache and reports stale skills; both jobs moved to the toolchain's passive authoring-command check, and the missing-skill prompt no longer fires on every command. Co-Authored-By: Claude Fable 5 <[email protected]>
A brand-new user's first command is baml init, so it should carry the install nudge; the first-run journey test asserts it again. Co-Authored-By: Claude Fable 5 <[email protected]>
When agent install replaces an existing skill, move the previous version into baml-old_skills/<name>/ inside the same skills directory (one slot per skill; each install keeps the immediately-previous copy) instead of deleting it. The archive doubles as the failure-recovery source, replacing the hidden temp backup. Archived copies nest one level deeper than the <skills>/<name>/SKILL.md layout, so neither agent harnesses nor the missing-skill detection treat them as installed. Co-Authored-By: Claude Fable 5 <[email protected]>
|
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe wrapper now refreshes only channel manifests. Passive agent-skill freshness checks run in selected ChangesSkill freshness and CLI checks
Wrapper manifest freshness
Skill installation archives
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant RuntimeCli
participant SkillCheck
participant SkillCache
Developer->>RuntimeCli: run an authoring command
RuntimeCli->>SkillCheck: start passive check
SkillCheck->>SkillCache: read skill state and cached commit
SkillCheck-->>Developer: print missing or outdated warning
SkillCheck->>SkillCache: refresh latest commit when due
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_cli/tests/agent_install_e2e.rs (1)
180-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
generatesucceeds before checking stderr.Unlike steps 1 and 2, this step doesn't check
output.status.success(). Ageneratefailure unrelated to skill freshness (e.g., a broken generator config) would still pass this assertion as long as stderr doesn't mention "baml skill", weakening the "next command is quiet" check this step is meant to validate.✅ Proposed fix
let output = run(&["generate"]); + assert!( + output.status.success(), + "generate failed: {}", + String::from_utf8_lossy(&output.stderr) + ); let stderr = String::from_utf8_lossy(&output.stderr); assert!(!stderr.contains("baml skill"), "{stderr}");🤖 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/tests/agent_install_e2e.rs` around lines 180 - 185, Update the Step 3 assertion around run(["generate"]) to first assert output.status.success(), preserving the existing stderr diagnostic, then perform the “baml skill” absence check. This ensures the quietness assertion only passes when generate completes successfully.
🤖 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/agent_command.rs`:
- Around line 647-677: Reject any skill whose name equals OLD_SKILLS_DIR before
installation or replacement begins, returning a clear error instead of
processing it. Apply the validation at the install entry point so upstream
skills named “baml-old_skills” cannot reach replace_skill_dir or conflict with
the internal archive path.
---
Nitpick comments:
In `@baml_language/crates/baml_cli/tests/agent_install_e2e.rs`:
- Around line 180-185: Update the Step 3 assertion around run(["generate"]) to
first assert output.status.success(), preserving the existing stderr diagnostic,
then perform the “baml skill” absence check. This ensures the quietness
assertion only passes when generate completes successfully.
🪄 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: 1f3823a8-69b6-4cd0-a8ed-33f2b0b98743
📒 Files selected for processing (10)
baml_language/crates/baml/src/main.rsbaml_language/crates/baml/tests/freshness_e2e.rsbaml_language/crates/baml_cli/src/agent_command.rsbaml_language/crates/baml_cli/src/commands.rsbaml_language/crates/baml_cli/src/lib.rsbaml_language/crates/baml_cli/src/skill_check.rsbaml_language/crates/baml_cli/tests/agent_install_e2e.rsbaml_language/crates/baml_cli/tests/exit_code_e2e.rsbaml_language/crates/baml_cli/tests/skill_warning_e2e.rsbaml_language/crates/baml_release/src/skills.rs
Binary size checks passed✅ 7 passed
Generated by |
An upstream skill named old_skills (legacy layout) or baml-old_skills (direct layout) would collide with the archive directory inside the skills dir: replacing it would rename skills/baml-old_skills into its own archive slot. Reject the reserved name during normalization, before anything touches disk. Addresses CodeRabbit review on #4001. Co-Authored-By: Claude Fable 5 <[email protected]>
…ness check A generate failure unrelated to skill freshness could previously slip past step 3 as long as stderr didn't mention skills. The fixture now gets a [generator.py] section after init so generate genuinely succeeds, and the step asserts success before checking quietness. Addresses CodeRabbit nitpick on #4001. Co-Authored-By: Claude Fable 5 <[email protected]>
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
Moves the agent-skill freshness system wholly into the toolchain binary and rounds out the install flow: the "install/upgrade your skill" warning now ships with every nightly (no wrapper release needed), fires only on the authoring commands, and
baml agent installarchives whatever it replaces.New features
Toolchain-owned skill warning (
skill_checkinbaml_cli): beforeinit,run,generate, andpack— and only those — the CLIchecks the project for installed
baml-*skills and prints one of:warning: No baml skill is installed, set it up with baml agent install.(no skills found between the cwd and$HOME)warning: Your baml skill is outdated, use baml agent install to upgrade it.(installed provenance is behind the skill repo's latestcommit)
The check never blocks on the network: warnings are decided from local caches, and a background refresh of the latest-commit cache runs at
most once per 24h (throttled by a marker even when offline, disabled via
[update] auto_check = falsein~/.baml/config.toml). All othercommands —
check,describe,fmt,lsp,agent, etc. — never nag.Replaced skills are archived, not deleted: when
baml agent installoverwrites an existing skill, the previous version moves tobaml-old_skills/<name>/inside the same skills directory (one slot per skill — each install keeps the immediately-previous copy). The archivedoubles as the failure-recovery source. Archived copies nest one level deeper than the
skills/<name>/SKILL.mdlayout, so agent harnesses don'tdiscover them (verified against a live Claude Code session), the missing-skill detection ignores them, and
--from <dir>re-ingestion filtersthem out.
Wrapper cleanup: all skill-warning machinery is removed from the
bamlwrapper (pass-through warning, post-toolchain use/updatenudge,skill half of the lazy refresh, skill section of
toolchain status). None of it had ever shipped — it was added in feat(toolchain): outdated warnings for toolchain + agent skills, install skills from repo head #3970 without a wrapperversion bump — so published wrapper behavior is unchanged and the wrapper stays at 0.2.0. The shared logic (warning decision, project-skills
walk, TTL/marker throttle, auto-check config gate) now lives in
baml_release::skills.Expected behavior (first-run journey)
Starting from an empty directory:
codeload.github.com— never the GitHub REST API (no credentials, nounauthenticated rate limit) — resolves the project root from the cwd (
baml.toml/baml_srcowner, bounded by the git repo /$HOME), andrecords the tarball's source commit as provenance in
~/.baml/state.toml..claude/skills/baml-old_skills/baml-core/(and the.agentstwin) while thenew one replaces
baml-core/.run/generate/pack/initare silent while the installed commit matches the repo head; once the skill repo advances (checkedin the background ≤ once/day), they warn
Your baml skill is outdated…until the nextbaml agent install.--from <url|tarball|dir>) record no provenance, so they keep nudging toward an official install by design.Tests
skill_warning_e2e.rs(11 tests: whitelist on/off, missing vs outdated vs current, background refresh persistence, offlinethrottling, auto-check opt-out, archive-not-installed) and the first-run journey in
agent_install_e2e.rs(init warns → plain install from acodeload stub that 404s any API path → next command quiet).
baml_release.exit_code_e2e.rsnow isolatesBAML_HOMEper test so CLI tests never read the developer's real~/.bamlor touch the network.Summary by CodeRabbit
baml-old_skills/archive slot (per skill name) with rollback on failure.update.auto_checkto describe wrapper manifest refresh only.