fix(add-skill): resolve script path and install scope in docs - #608
Conversation
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
5 similar comments
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
There was a problem hiding this comment.
🟡 Changes recommended
Real vendor manifests conflict with existing symlink-only tests, and the PR includes runtime changes beyond its documented docs-only scope.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates the add-skill documentation and generated catalog, while also adding vendor-manifest synchronization and GitHub automation profile behavior.
Changes:
- Clarifies script execution path and workspace-local installation.
- Supports real vendor manifest directories for Codex/Claude.
- Resolves
AUTOMATION_MODELprofiles ingithub-repo-monitor.
File summaries
| File | Description |
|---|---|
skills/add-skill/SKILL.md |
Documents the corrected install workflow. |
skills/index.js |
Regenerated skill catalog content. |
scripts/sync_extensions.py |
Adds real vendor manifest support. |
skills/iterate/.codex-plugin/plugin.json |
Adds Codex manifest copy. |
skills/iterate/.claude-plugin/plugin.json |
Adds Claude manifest copy. |
tests/test_sync_extensions.py |
Tests vendor manifest synchronization. |
skills/github-repo-monitor/scripts/main.py |
Adds LLM profile resolution. |
tests/test_github_repo_monitor.py |
Tests profile selection and fallback. |
Review details
Files not reviewed (1)
- skills/index.js: Generated file
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| elif link.is_dir(): | ||
| if _vendor_manifest_matches(directory, vendor): | ||
| continue | ||
| problems.append(f"stale manifest copy: {link.relative_to(REPO_ROOT)}") |
| def _fetch_profile_llm(agent_url: str, api_key: str, profile: str) -> dict | None: | ||
| """Fetch one LLM profile's config, or None when it no longer exists. | ||
|
|
||
| Uses X-Expose-Secrets: plaintext so the config carries a real api_key, | ||
| mirroring _fetch_settings. Only 404 falls back; other errors raise. | ||
| """ | ||
| req = urllib.request.Request( | ||
| f"{agent_url}/api/profiles/{quote(profile, safe='')}", |
Co-authored-by: openhands <openhands@all-hands.dev>
83eb7bd to
1356049
Compare
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review: fix(add-skill): resolve script path and install scope in docs
Docs-only change to skills/add-skill/SKILL.md (plus the regenerated skills/index.js). I fetched the exact head 1356049c and verified the changed behavior against the workspace and the repository guidance in AGENTS.md.
What the change does, and why it is correct
- Replaces the never-defined placeholder
python3 <this-skill-path>/scripts/fetch_skill.pywithpython3 scripts/fetch_skill.py, with the instruction to run it from the directory containingSKILL.md. This matches the documented usage inskills/add-skill/README.md(From the add-skill skill directory: python3 scripts/fetch_skill.py ...) and the convention used by other skills that ship scripts. The placeholder is now absent repo-wide (grep -rn 'this-skill-path'returns nothing), andfetch_skill.pystill lives atskills/add-skill/scripts/fetch_skill.py, so the relative path resolves as described. - The new install-scope note (
<workspace>/.agents/skills/<skill-name>/, not global) matches the implementation:fetch_skill.pysetsdest_dir = Path(workspace_path) / '.agents' / 'skills' / skill_nameand copies there viashutil.copytree. It also matches the existing example in the same file (ls /path/to/workspace/.agents/skills/codereview/SKILL.md) andREADME.md. The wording is accurate and consistent with the repository's plain-hyphen punctuation convention. - No side effects outside the reported bug: the call signature, argument order (
<github-url> <workspace-path>), and the--forcebehavior are untouched, and the only other changed file is the generated catalog entry whose embeddedcontentstring mirrorsSKILL.mdexactly.
Verification performed
- Regenerated the catalog with
node scripts/build-skills-catalog.mjs;git diffafterwards is empty, so the committedskills/index.jsmatches what the generator produces (content-only change, as claimed). - Ran the focused suite:
tests/test_interface_manifest.py,tests/test_add_skill_installs_to_agents_dir.py,tests/test_skills_catalog.py,tests/test_skills_have_readme.py-> 69 passed. python scripts/sync_extensions.py --checkexits 0 (only a pre-existing non-blocking coverage warning forplugins/issue-duplicate-checker, unrelated to this PR).- Reproduced the documented end-to-end flow from
skills/add-skill:python3 scripts/fetch_skill.py "OpenHands/extensions/skills/deno" <tmp-workspace>succeeded and installed to<tmp-workspace>/.agents/skills/deno/withSKILL.mdpresent, confirming the workspace-local scope stated in the new note. - GitHub Actions for head
1356049c:Validate PR description,check-pr-artifacts, andpr-titleall pass.
No material bugs, security concerns, or design flaws found. The UnicodeEncodeError on non-UTF-8 Windows consoles noted in the PR description is a pre-existing, cosmetic, locale-specific issue in fetch_skill.py and is correctly out of scope for this docs fix.
✅ APPROVED
HUMAN:
I ran the manifest tests, verified the documented
fetch_skill.pyflow installs the skill successfully fromskills/add-skill, and confirmednpm run build:skillsonly regenerates the expected content.Why
skills/add-skill/SKILL.mdtold the agent to runpython3 <this-skill-path>/scripts/fetch_skill.py ..., but that placeholder is never defined anywhere, so the agent cannot resolve the script location and flails (#413). The same page also never states the install scope, which caused the global-vs-workspace confusion in the thread.Summary
python3 scripts/fetch_skill.py ..., matching the README and repo convention), instead of the unresolvable placeholder.<workspace>/.agents/skills/<skill-name>/).skills/index.jsvianpm run build:skills(content-only change).Issue Number
Fixes #413
How to Test
python -m pytest tests/test_interface_manifest.py -q→ 16 passed (same on base).skills/add-skill,python scripts/fetch_skill.py "OpenHands/extensions/skills/deno" <tmp-workspace>installs the skill's SKILL.md successfully.npm run build:skillsregeneratesskills/index.js; diff is content-only.Notes: on a non-UTF-8 Windows console,
fetch_skill.py's final success print raisesUnicodeEncodeErrorafter a successful install (cosmetic, locale-specific, out of scope here; happy to follow up separately).Type
Bug fix