Repository navigation
Conversation
Co-authored-by: Dhilan Shah <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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)
📝 WalkthroughWalkthroughRefactors ChangesAgent skill installer: --check mode and provenance reporting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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):
|
|
No description provided. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 673-674: After the target_statuses call computes per-skill
statuses, filter the skills array to exclude any skills marked as Unchanged
before passing them to the install_skills_to function call. Update the
install_skills_to function signature to accept a reference to a slice of skill
references (&[&Skill]) instead of the current parameter type to accommodate the
filtered skills list. This ensures unchanged skills are not rewritten or have
their directories replaced.
- Around line 715-716: The code at the point where existing is assigned uses
fs::read_to_string(...).ok() which converts all IO errors to None, treating
permission errors and invalid UTF-8 the same as missing files. This prevents
reporting actual read failures to the user. Modify the code to explicitly handle
the Result from fs::read_to_string on the path variable, converting only
NotFound errors to None for the existing variable, while allowing other IO
errors to propagate so they can be properly reported instead of silently
treating them as a missing file scenario.
🪄 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: b8879079-3ee8-4d9f-ba90-304367b6ea7a
📒 Files selected for processing (1)
baml_language/crates/baml_cli/src/agent_command.rs
Co-authored-by: Dhilan Shah <[email protected]>
…agent install summary conflict Co-authored-by: Dhilan Shah <[email protected]>
Co-authored-by: Dhilan Shah <[email protected]>
Issue Reference
baml agent install --latestnot reporting the installed ref/commit or per-skill status.The error
baml agent installexited 0 and printed a static, uninformative summary. Two runs against the same source produced byte-for-byte identical output, with no indication of whether each skill was created, updated, or already up to date, and no indication of which upstream ref/commit was installed:To verify
--latestactually refreshed anything, a user had to capture MD5 checksums before/after and query the GitHub API by hand. There was also no--checkdry-run.Root cause
In
baml_language/crates/baml_cli/src/agent_command.rs:install_skills_tooverwrote eachbaml-*skill directory atomically but never compared old vs. new content, so it had no created/updated/unchanged signal to surface.load_skillsreturned a bareVec<Skill>and discarded all provenance — the branch/tag/URL it fetched from and the resolved commit were never captured.print_successemitted a fixed string built only from the install root, independent of what actually happened on disk.The fix
All changes are confined to
agent_command.rs:load_skillsnow returns aSkillSource { reference, commit, skills }. For--latestit best-effort resolves themaincommit SHA viaGET api.github.com/repos/BoundaryML/baml-skill/commits/mainand downloads that exact commit so the reported SHA matches the installed bytes (falling back tomainif the API is unavailable).--fromand the versioned-release path populate a descriptivereference.target_statusesreads each existingSKILL.mdand classifies it ascreated/updated/unchanged (already latest)immediately before writing. The summary now prints one status line per skill under each target (Claude Code (.claude/skills),Codex / OpenCode (.agents/skills)) plus theSource:line with the commit when known.--checkdry-run. A new--checkflag computes and prints the same statuses (would create/would update/up to date) without writing anything, and never creates a missing--dir.Example after the fix:
Testing
cargo test -p baml_cli agent_command(18 passed), covering status classification, disk-state-aware statuses,--checknot writing, install-then-reinstall (created→unchanged), and the rendered report (with/without commit).--check/would-update → updated transitions, and that--checkdoes not create a missing--dir.cargo fmt --all,cargo clippy --all-targets -- -D warnings(clean across the workspace), andcargo test -p baml_tests -p baml_lsp2_actions_tests -p baml_cliall pass. No snapshots changed. The only failing tests in a fullcargo test --workspaceare thesdk_test_python_pydantic2integration tests, which requireuv(not installed in this environment) and are unrelated to this change.Additional Notes
--checkintentionally exits 0 (it prints the verdict) to keep the change minimal and avoid repurposing existing error exit codes.CODERABBIT_API_KEYwas not present in this environment and the CLI could not authenticate non-interactively, so the automated self-review could not be run. The diff was self-reviewed manually instead.PR Checklist
Summary by CodeRabbit
Release Notes
New Features
--checkflag to run a dry-run that reports what would be created or updated without writing changes.Improvements
--checkmode.--check, the install path is resolved without creating missing directories.Tests