[common-skills migration] Add optional best-effort warp-skills install step - #13247
[common-skills migration] Add optional best-effort warp-skills install step#13247vkodithala wants to merge 6 commits into
Conversation
Add script/install_warp_skills to install the internal warpdotdev/warp-skills skills pinned in warp-skills-lock.json. It is wired optionally and best-effort into script/bootstrap and script/run after the common-skills step, skips on --skip-warp-skills or WARP_SKIP_WARP_SKILLS_INSTALL=1, no-ops without a lock file, and never fails its caller (e.g. external contributors without access). The common-skills install flow is left untouched. Document the optional step and opt-out in AGENTS.md and CONTRIBUTING.md, noting it is not required for external contributors. Co-Authored-By: Oz <oz-agent@warp.dev>
Run the skills CLI inside a throwaway temp dir and copy only the resulting skill directories into the target (.agents/skills or ~/.agents/skills), instead of running --copy from the repo root. Running from the repo root made the CLI write/merge its own skills-lock.json into the committed common-skills lock at runtime. The repo's skills-lock.json now stays clean. Co-Authored-By: Oz <oz-agent@warp.dev>
vkodithala
changed the title
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds a best-effort script/install_warp_skills step, wires it into script/bootstrap and script/run, and documents the optional internal skill installation flow.
Concerns
- The installer reads
warp-skills-lock.jsononly to extract the first ref, then runsskills addagainst that mutable ref. The committedcomputedHashvalues are never enforced, so the new lock file does not actually pin the installed skill content. - The warp-skills target selection does not use the actual common-skills target when that target is chosen interactively or inferred from existing installs, so the two installers can write to different locations.
- Copy/setup failures inside the best-effort installer can still produce a successful "installed" message because the script intentionally omits
set -eand does not checkmkdir,rm, orcpresults.
Security
- The lock file's integrity hashes are bypassed during installation, which weakens the supply-chain guarantee described by the PR.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| tmp_dir="$(mktemp -d "${TMPDIR:-/tmp}/warp-skills.XXXXXX")" | ||
| trap 'rm -rf "${tmp_dir}"' EXIT | ||
|
|
||
| if ( cd "${tmp_dir}" && npx --yes "skills@${SKILLS_CLI_VERSION}" add "${source_spec}" \ |
There was a problem hiding this comment.
skills add and never verifies the committed warp-skills-lock.json computedHash values, so the lock does not actually pin content; restore from the lock or validate hashes before copying skills.
| fi | ||
|
|
||
| # Install into the same target as the common skills (project by default). | ||
| if [[ "${WARP_COMMON_SKILLS_INSTALL_TARGET:-}" = "global" ]]; then |
There was a problem hiding this comment.
WARP_COMMON_SKILLS_INSTALL_TARGET, but common-skills can choose global via the prompt or existing install state without setting that env var, so warp-skills can default to project-local and diverge from the common-skills target.
| [[ -d "${skill_dir}" ]] || continue | ||
| skill_name="$(basename "${skill_dir}")" | ||
| rm -rf "${target_dir:?}/${skill_name}" | ||
| cp -R "${skill_dir%/}" "${target_dir}/${skill_name}" |
There was a problem hiding this comment.
set -e, a failing mkdir, rm, or cp still falls through with installed=1 and reports success; check these operations and print the best-effort skip notice when copying fails.
Pin the consumer lock to main (canonical) to match warp-server. main is empty until warp-skills #1 merges, so the optional install step cleanly no-ops until then; the common-skills downstream lock-automation refreshes it afterward. Co-Authored-By: Oz <oz-agent@warp.dev>
Delete the vendored script/install_warp_skills and instead call the shared wrapper via ./script/resolve_common_skills install_warp_skills, resolved from warpdotdev/common-skills. bootstrap and run keep the best-effort || true guard and the --skip-warp-skills / WARP_SKIP_WARP_SKILLS_INSTALL gating. Update AGENTS.md and CONTRIBUTING.md accordingly. Co-Authored-By: Oz <oz-agent@warp.dev>
- Clarify docs (CONTRIBUTING.md, AGENTS.md): warp-skills is internal-only — external contributors lack access to warpdotdev/warp-skills, so it no-ops for them. - bootstrap/run: surface a warning if the warp-skills install unexpectedly fails, instead of silently swallowing it with || true (still non-blocking). - Note that the Windows (bootstrap.ps1) path does not wire up warp-skills yet. Co-Authored-By: Oz <oz-agent@warp.dev>
The warp-skills install guard in bootstrap/run only checked INSTALL_WARP_SKILLS (the --skip-warp-skills flag). Add a WARP_SKIP_WARP_SKILLS_INSTALL=1 check so the documented env opt-out short-circuits before invoking the resolver, matching the common-skills guard. Co-Authored-By: Oz <oz-agent@warp.dev>
2 participants
What
Wire up the optional, best-effort warp-skills install step in
bootstrap/runby calling the sharedinstall_warp_skillswrapper resolved fromwarpdotdev/common-skills, instead of vendoring a copy of the installer in this repo.Depends on warpdotdev/common-skills#37 (which adds
install_warp_skillsto the shared scripts).Changes
script/bootstrapandscript/runinstall the internal warp-skills (pinned inwarp-skills-lock.json) by invoking the sharedinstall_warp_skillswrapper through./script/resolve_common_skills. They land in the same target as the common skills (project unlessWARP_COMMON_SKILLS_INSTALL_TARGET=global).warpdotdev/warp-skills(external contributors) or when the lock is absent, and it can be skipped with--skip-warp-skills/WARP_SKIP_WARP_SKILLS_INSTALL=1. An unexpected failure logs a warning but never blocksbootstrap/run.AGENTS.mdandCONTRIBUTING.mddocument the step.Testing
Ran the exact command
bootstrap/runinvoke, via the full resolver chain (client shim → common resolver →install_warp_skills→install_common_skills), pointing at the common-skills branch:--skip-warp-skills,WARP_SKIP_WARP_SKILLS_INSTALL=1, and a missing lock all exit 0 with a clean notice; an inaccessible source fails fast (no credential prompt) and skips — never abortingbootstrap/run.… match warp-skills-lock.json), writes.git/warp/warp-skills-lock.hash, and is idempotent under--if-needed;remove_warp_skillscleans up.bash -nonscript/bootstrapandscript/run. Shell/docs-only change; no Rust touched, so./script/format/cargo clippywere not run.warp-skills-lock.jsoncurrently pinsref: main, but the warp-skills content is not onmainyet (it lives onoz/skills-migration, which currently carries onlyagent-attribution). Until the content is finalized onmainand the lock regenerated, this step best-effort no-ops (exit 0; does not blockbootstrap/run). Regenerate once warp-skillsmainis ready:update_common_skills_lock --source warpdotdev/warp-skills --lock-file warp-skills-lock.json.bootstrap.ps1path installs common skills but does not wire up warp-skills yet, so warp-skills is macOS/Linux only for now.Plan: https://staging.warp.dev/drive/notebook/R4ucMiSKpclPGJXzpoFu4t
This PR was generated with _Oz._