Add Browser CLI delivery skills and package install-skills assets#12
Merged
xhwSkhizein merged 7 commits intomainfrom Apr 14, 2026
Merged
Add Browser CLI delivery skills and package install-skills assets#12xhwSkhizein merged 7 commits intomainfrom
xhwSkhizein merged 7 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughIntroduces a modular three-skill delivery architecture for Browser CLI tasks (delivery, explore, converge) replacing a monolithic approach, adds packaged skill distribution via installed wheels using Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI: install-skills
participant Discover as discover_packaged_skills()
participant Resources as importlib.resources
participant Install as install_skills_from_paths()
participant FS as Filesystem
User->>CLI: invoke install-skills --target ~/my/path
CLI->>Discover: locate packaged skills
Discover->>Resources: query browser_cli.packaged_skills
Resources-->>Discover: skill assets (SKILL.md + paths)
Discover->>Discover: validate all required skills present
alt missing skills
Discover-->>CLI: raise InvalidInputError
CLI-->>User: error: incomplete packaged assets
else all found
Discover-->>CLI: list[PackagedSkill]
CLI->>Install: install_skills_from_paths(skills, target)
Install->>FS: create/resolve target directory
Install->>FS: stage each skill via resources.as_file()
Install->>FS: move staged to destination
Install-->>CLI: per-skill status (installed/updated)
CLI-->>User: success output with target path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
skills/browser-cli-delivery/SKILL.md (1)
1-61: Reduce drift risk between duplicated delivery skill docs.This file and
src/browser_cli/packaged_skills/browser-cli-delivery/SKILL.mdare effectively parallel sources. Consider generating/copied packaged content from one canonical source and enforcing exact-sync with a guard test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/browser-cli-delivery/SKILL.md` around lines 1 - 61, The two SKILL.md files for the browser-cli-delivery skill are duplicated and prone to drift; make the top-level browser-cli-delivery SKILL.md the canonical source, add a generation step (e.g., scripts/generate_packaged_skill_docs) that copies or renders that canonical SKILL.md into the packaged_skills browser-cli-delivery SKILL.md during build, and add a CI guard test named test_sync_packaged_skill_docs that compares the generated packaged SKILL.md to the committed packaged copy and fails the build if they differ.tests/unit/test_install_skills_command.py (1)
44-54: Add coverage for “skill dir exists butSKILL.mdis missing”.
discover_packaged_skills()has a dedicated incomplete-skill error path; adding that test would close a meaningful branch gap in this suite.Proposed test addition
+def test_discover_packaged_skills_fails_when_skill_doc_is_missing( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + source_root = tmp_path / "source" + # Create required dirs, but leave one incomplete (no SKILL.md). + _write_skill(source_root, "browser-cli-delivery") + _write_skill(source_root, "browser-cli-explore") + incomplete = source_root / "browser-cli-converge" + incomplete.mkdir(parents=True) + monkeypatch.setattr(install_skills_module, "_packaged_skills_root", lambda: source_root) + + with pytest.raises(InvalidInputError, match="browser-cli-converge"): + install_skills_module.discover_packaged_skills()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_install_skills_command.py` around lines 44 - 54, Add a new unit test in tests/unit/test_install_skills_command.py that exercises discover_packaged_skills()'s incomplete-skill error path by creating a skill directory (using the existing _write_skill helper or manually creating a folder) for a skill id (e.g., "browser-cli-incomplete") but intentionally omit the SKILL.md file, set install_skills_module._packaged_skills_root to that tmp path, then assert that install_skills_module.discover_packaged_skills() raises InvalidInputError and that the error message mentions the skill id and the missing "SKILL.md" so the test covers the "skill dir exists but SKILL.md is missing" branch.docs/superpowers/plans/2026-04-14-browser-cli-delivery-skills-implementation-plan.md (1)
563-564: Replace placeholder path with a concrete directory in the command.Line 563 uses
<legacy-skill-dir>, which makes the plan less executable as-is. Prefer an explicit path so the working log is copy/paste runnable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-14-browser-cli-delivery-skills-implementation-plan.md` around lines 563 - 564, Replace the placeholder token "<legacy-skill-dir>" in the git commands with a concrete directory path so the commands are copy/paste runnable (e.g., the actual legacy skill directory name used in the repo); update the two lines shown (the git rm -r <legacy-skill-dir> and subsequent git commit -m ...) to use that real path and ensure the directory name matches the repo structure and exists before running the git rm command.skills/browser-cli-explore/SKILL.md (1)
1-77: Avoid doc drift between repo and packaged skill copies.This file duplicates the packaged skill doc under
src/browser_cli/packaged_skills/browser-cli-explore/SKILL.md. Consider enforcing parity (or generating one from the other) so install-time behavior and repo guidance can’t diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/browser-cli-explore/SKILL.md` around lines 1 - 77, The SKILL.md in skills/browser-cli-explore is duplicated by src/browser_cli/packaged_skills/browser-cli-explore/SKILL.md causing doc drift; fix by choosing a single source of truth and implementing synchronization (e.g., generate the packaged SKILL.md from skills/browser-cli-explore/SKILL.md at build/install time or replace one copy with a symlink), add a CI parity check that compares these two files (skills/browser-cli-explore/SKILL.md and src/browser_cli/packaged_skills/browser-cli-explore/SKILL.md) and fail the build if they differ, and update any relevant packaging scripts to ensure the chosen approach is enforced.tests/unit/test_repo_skill_docs.py (1)
29-33: Add explicit guard-expectation assertion alongside AGENTS.md check.The architecture guard already includes
packaged_skillswith the correct empty dependency set (line 48 ofscripts/guards/architecture.py), andtest_guard_scripts.pyvalidates the overall architecture guard passes. However, to prevent future drift and explicitly document the contract, add an assertion in this test that verifiespackaged_skillsexists in the allowed dependencies with the expected configuration—keeping both the AGENTS.md pointer and the guard expectation locked in the same place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_repo_skill_docs.py` around lines 29 - 33, Update the test_agents_points_to_browser_cli_delivery_skill test to also assert the architecture guard expectation for packaged_skills: inside test_agents_points_to_browser_cli_delivery_skill (and/or a helper it calls) add an explicit assertion that the guard's allowed dependencies mapping contains a "packaged_skills" key with the expected configuration (an empty dependency set), mirroring the entry defined in scripts/guards/architecture.py so the test documents and locks that contract alongside the existing AGENTS.md pointer; reference the existing test_guard_scripts.py pattern for how guard expectations are accessed/validated when implementing the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@docs/superpowers/plans/2026-04-11-browser-cli-network-response-body-implementation-plan.md`:
- Around line 106-108: The Milestone 7 task currently uses the outdated phrase
"explore-delivery skill references"; update that task text in the plan
(Milestone 7) to use the new split skill names—replace "explore-delivery skill
references" with a phrase that names the three current stacks (e.g., "explore,
delivery, and converge skill references" or "delivery/explore/converge skill
references") so it consistently matches the new folders browser-cli-delivery,
browser-cli-explore, and browser-cli-converge.
In
`@docs/superpowers/plans/2026-04-14-browser-cli-install-skills-implementation-plan.md`:
- Around line 13-42: The plan omitted adding or updating AGENTS.md and the
repository guard expectations for the new top-level package and public CLI
surface; update AGENTS.md to document the new browser_cli package and the public
install-skills --target contract, and modify the guard rules (the repo guard
expectations that validate new top-level packages/CLI surfaces and runtime
contract changes) so they reflect the new packaged skills whitelist and target
behavior before marking the checklist items complete; ensure references to
src/browser_cli, the install_skills command, and the new packaged_skills assets
are included in AGENTS.md and that CI/guard configs (.github/workflows and
whatever guard rules enforce package/CLI surface checks) are updated to fail if
those docs/guards are missing.
In `@docs/superpowers/specs/2026-04-14-browser-cli-delivery-skills-design.md`:
- Line 5: The spec header currently hard-codes an absolute developer path after
the "Repo:" header; replace that absolute workspace path with a portable repo
identifier (e.g., change the header from Repo:
`/home/hongv/workspace/browser-cli` to Repo: `browser-cli`) so the document is
not machine-specific and remains portable across environments.
In `@src/browser_cli/commands/install_skills.py`:
- Around line 84-93: The current _install_one_skill uses
resources.as_file(source) which is not supported on Python 3.10 and removes
destination before staging; change it to use the importlib.resources Traversable
API (e.g., resources.files(source) / .joinpath and iterate over .iterdir()) to
copy the package tree in a Python-3.10-compatible way, and stage the copy to a
temporary sibling directory (create a temp dir next to destination, e.g.,
destination.with_name(destination.name + '.tmp-<unique>') and copy the
Traversable contents into that staged dir), then once the staged dir is fully
populated, atomically swap by removing the old destination and moving/renaming
the staged dir to destination (ensure errors during copy do not delete the
existing destination and clean up the temp sibling on failure); modify the
function _install_one_skill and operations around destination, source, and
staging to implement this flow.
In `@tests/unit/test_release_artifacts.py`:
- Around line 14-17: The test currently selects a wheel via lexicographic sort
of wheels = sorted((_repo_root() / "dist").glob("*.whl")), which can pick an
older file; change selection to sort by file modification time and pick the
newest file instead. Locate the wheels variable and wheel_path assignment in
test_release_artifacts.py and replace the filename-based sorted(...) usage with
a sort/key that uses Path.stat().st_mtime (or use max(...,
key=Path.stat().st_mtime) ) so wheel_path points to the most recently modified
wheel.
---
Nitpick comments:
In
`@docs/superpowers/plans/2026-04-14-browser-cli-delivery-skills-implementation-plan.md`:
- Around line 563-564: Replace the placeholder token "<legacy-skill-dir>" in the
git commands with a concrete directory path so the commands are copy/paste
runnable (e.g., the actual legacy skill directory name used in the repo); update
the two lines shown (the git rm -r <legacy-skill-dir> and subsequent git commit
-m ...) to use that real path and ensure the directory name matches the repo
structure and exists before running the git rm command.
In `@skills/browser-cli-delivery/SKILL.md`:
- Around line 1-61: The two SKILL.md files for the browser-cli-delivery skill
are duplicated and prone to drift; make the top-level browser-cli-delivery
SKILL.md the canonical source, add a generation step (e.g.,
scripts/generate_packaged_skill_docs) that copies or renders that canonical
SKILL.md into the packaged_skills browser-cli-delivery SKILL.md during build,
and add a CI guard test named test_sync_packaged_skill_docs that compares the
generated packaged SKILL.md to the committed packaged copy and fails the build
if they differ.
In `@skills/browser-cli-explore/SKILL.md`:
- Around line 1-77: The SKILL.md in skills/browser-cli-explore is duplicated by
src/browser_cli/packaged_skills/browser-cli-explore/SKILL.md causing doc drift;
fix by choosing a single source of truth and implementing synchronization (e.g.,
generate the packaged SKILL.md from skills/browser-cli-explore/SKILL.md at
build/install time or replace one copy with a symlink), add a CI parity check
that compares these two files (skills/browser-cli-explore/SKILL.md and
src/browser_cli/packaged_skills/browser-cli-explore/SKILL.md) and fail the build
if they differ, and update any relevant packaging scripts to ensure the chosen
approach is enforced.
In `@tests/unit/test_install_skills_command.py`:
- Around line 44-54: Add a new unit test in
tests/unit/test_install_skills_command.py that exercises
discover_packaged_skills()'s incomplete-skill error path by creating a skill
directory (using the existing _write_skill helper or manually creating a folder)
for a skill id (e.g., "browser-cli-incomplete") but intentionally omit the
SKILL.md file, set install_skills_module._packaged_skills_root to that tmp path,
then assert that install_skills_module.discover_packaged_skills() raises
InvalidInputError and that the error message mentions the skill id and the
missing "SKILL.md" so the test covers the "skill dir exists but SKILL.md is
missing" branch.
In `@tests/unit/test_repo_skill_docs.py`:
- Around line 29-33: Update the test_agents_points_to_browser_cli_delivery_skill
test to also assert the architecture guard expectation for packaged_skills:
inside test_agents_points_to_browser_cli_delivery_skill (and/or a helper it
calls) add an explicit assertion that the guard's allowed dependencies mapping
contains a "packaged_skills" key with the expected configuration (an empty
dependency set), mirroring the entry defined in scripts/guards/architecture.py
so the test documents and locks that contract alongside the existing AGENTS.md
pointer; reference the existing test_guard_scripts.py pattern for how guard
expectations are accessed/validated when implementing the assertion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f87aad3-9652-4ea4-a5c9-30096e4c7c57
📒 Files selected for processing (31)
.github/workflows/ci.yml.github/workflows/release.ymlAGENTS.mddocs/superpowers/plans/2026-04-11-browser-cli-network-response-body-implementation-plan.mddocs/superpowers/plans/2026-04-13-browser-cli-task-automation-implementation-plan.mddocs/superpowers/plans/2026-04-14-browser-cli-delivery-skills-implementation-plan.mddocs/superpowers/plans/2026-04-14-browser-cli-install-skills-implementation-plan.mddocs/superpowers/specs/2026-04-13-browser-cli-task-automation-design.mddocs/superpowers/specs/2026-04-14-browser-cli-delivery-skills-design.mddocs/superpowers/specs/2026-04-14-install-skills-design.mdpyproject.tomlscripts/guards/architecture.pyskills/browser-cli-converge/SKILL.mdskills/browser-cli-delivery/SKILL.mdskills/browser-cli-explore-delivery/SKILL.mdskills/browser-cli-explore-delivery/agents/openai.yamlskills/browser-cli-explore-delivery/references/preflight-and-runtime.mdskills/browser-cli-explore-delivery/references/task-input-design.mdskills/browser-cli-explore-delivery/references/task-modes.mdskills/browser-cli-explore/SKILL.mdsrc/browser_cli/cli/main.pysrc/browser_cli/commands/install_skills.pysrc/browser_cli/packaged_skills/__init__.pysrc/browser_cli/packaged_skills/browser-cli-converge/SKILL.mdsrc/browser_cli/packaged_skills/browser-cli-delivery/SKILL.mdsrc/browser_cli/packaged_skills/browser-cli-explore/SKILL.mdtests/unit/test_cli.pytests/unit/test_install_skills_command.pytests/unit/test_release_artifacts.pytests/unit/test_repo_metadata.pytests/unit/test_repo_skill_docs.py
💤 Files with no reviewable changes (5)
- skills/browser-cli-explore-delivery/agents/openai.yaml
- skills/browser-cli-explore-delivery/references/preflight-and-runtime.md
- skills/browser-cli-explore-delivery/references/task-input-design.md
- skills/browser-cli-explore-delivery/SKILL.md
- skills/browser-cli-explore-delivery/references/task-modes.md
Owner
Author
|
Addressed the still-valid review findings and pushed them to this PR in Updated items:
Validation on this branch:
|
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
1 participant
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
browser-cli install-skills--targetsupport plus wheel-level CI/release smoke checksValidation
./scripts/lint.sh./scripts/test.sh./scripts/guard.shbrowser-cli install-skills --dry-run --target <tmpdir>in a clean venvSummary by CodeRabbit
New Features
install-skillscommand now supports--targetargument to customize skill installation directory (defaults to~/.agents/skills).Documentation
browser-cli-delivery,browser-cli-explore, andbrowser-cli-converge, each with dedicated documentation.