[PR #6235] [MERGED] fix(ci): run the lockfile-pinned Skills CLI in package verification #7216

New issue

Closed

opened 2026-11-01 21:46:14 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:14 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6235
Author: @kaoru0822-kitauji
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/ci-pin-repository-skill-installer

📝 Commits (2)

aa4652a fix(ci): run the lockfile-pinned Skills CLI in package verification
68cf54f fix(ci): lock the Skills CLI outside the root dependency tree

📊 Changes

3 files changed (+147 additions, -8 deletions)

View changed files

📝 scripts/verify-repository-skill.mjs (+27 -8)
➕ scripts/verify-tools/skills/package-lock.json (+112 -0)
➕ scripts/verify-tools/skills/package.json (+8 -0)

📄 Description

What broke: The required package-e2e lane intermittently timed out in the repository skill stage, on main (ce5bc8cb) and on PRs. npm exec --yes --package skills@1.7.2 resolved the CLI and its unlocked dependencies from the npm registry on every run; the stage took between 5 s and 118 s against a 120 s limit.
Fix: Give the verifier its own lockfile for skills@1.7.2 (scripts/verify-tools/skills/). Install it with npm ci --prefer-offline in the verification workspace and run its bin with Node. No registry metadata is resolved, and every transitive dependency is locked.
Risk: Low. Verifier-only change. The root dependency tree and the supported Node range are unchanged. Skills' default repository discovery is exercised exactly as before.

Summary

Fixes #6234.

The repository skill check is meant to verify Skills' default root discovery against a clean repository snapshot. Its time was dominated by registry resolution, not by the check itself, so a slow registry made a required lane fail.

Problem and expected behavior

Main CI run 39709973007 (ce5bc8cb), plus PR runs 39710915987 and 39711962559: repository skill: start at 02:21:30, then SIGTERM at the 120 s timeout with empty stdout and stderr.
Passing runs in the same window took 4 702 ms, 27 101 ms and 117 695 ms for this stage.
Locally, skills add against a full git archive snapshot takes ~2 s with a warm npm cache and ~7 s with a cold one, so the variance comes from registry resolution.

Expected: this stage needs no network and has a deterministic, locked toolchain.

Change and scope

scripts/verify-tools/skills/package.json and package-lock.json (new): a private manifest that pins skills 1.7.2, plus its lockfile (8 packages, all with resolved and integrity).
scripts/verify-repository-skill.mjs: copies that manifest and lockfile into the verification workspace, runs npm ci --prefer-offline --ignore-scripts there, then runs node <bin> add … with the same arguments, environment isolation and timeout as before.
Why not a root devDependency: the first revision pinned it in the root package.json. That made npm ci fail under engine-strict on Node 22.19.x, which is still supported, because skills 1.6+ requires >=22.20.0 (review finding). The separate lockfile keeps that constraint inside the verifier, as it was before.
Not included: no change to what is verified, and no change to other verifiers.

Contribution scope: not applicable (CI tooling fix).

Contract and boundary impact

Semantic owner and earliest changed stage: verifyRepositorySkill (package verification only).
Contribution scope fit and linked maintainer decision: not applicable.
CLI and MCP/tool-catalog contract: none.
Provider, bridge, target-format, or platform compatibility: none. The bin is launched through process.execPath, which also works on Windows.
Evidence, artifact, provenance, or reconstruction contract: none.
Generated metadata, package, or installation impact: none. The root dependencies and the published package are unchanged.
Read existing files or captured data.

Evidence and regression coverage

Tests added or updated: none. The verifier itself is the check, and it runs in the package-e2e lane.
Base reproduction or other evidence: the CI timings above. Locally the stage now completes in 2 s with a warm npm cache and 6 s with a cold one, returning {skillsVersion: "1.7.2", files: 7}. With --loglevel http, the locked npm ci makes only the 8 tarball requests on a cold cache (plus npm's own version check) and 0 requests on a warm one. It makes no packument (metadata) requests.
Remaining proof gaps: the full npm run verify:package was not run locally. The CI package-e2e lane on this PR is the end-to-end proof.

Validation performed

node -e 'import("./scripts/verify-repository-skill.mjs").then(m => m.verifyRepositorySkill({root, workspace}))': passed (2 s warm, 6 s with an empty npm_config_cache). The 7 authored files were installed identically.
npm ci --loglevel http in the tool directory: tarball requests only, as described above.
Root npm ci: passed. The root package.json and package-lock.json are unchanged from main.
npm run check:fast: passed.
npm run format:check: passed.
npx knip: reports only two "unresolved imports" (#web-bundle-analysis-worker, #javascript-analysis-worker), in files this change does not touch.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable.
Package or release metadata impact: none. Dev dependency only.
Security, privacy, process, or containment review: replaces an unlocked registry resolution with a lockfile that has integrity hashes. --ignore-scripts prevents install scripts from running.

Author readiness

Human owner: @kaoru0822-kitauji

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/6235 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/ci-pin-repository-skill-installer` --- ### 📝 Commits (2) - [`aa4652a`](https://github.com/morluto/rea/commit/aa4652ac79eff7af433d64c3254884edf74b133a) fix(ci): run the lockfile-pinned Skills CLI in package verification - [`68cf54f`](https://github.com/morluto/rea/commit/68cf54fada0ec6fa326cb450062f9e4b47805252) fix(ci): lock the Skills CLI outside the root dependency tree ### 📊 Changes **3 files changed** (+147 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `scripts/verify-repository-skill.mjs` (+27 -8) ➕ `scripts/verify-tools/skills/package-lock.json` (+112 -0) ➕ `scripts/verify-tools/skills/package.json` (+8 -0) </details> ### 📄 Description **What broke:** The required `package-e2e` lane intermittently timed out in the `repository skill` stage, on main (`ce5bc8cb`) and on PRs. `npm exec --yes --package skills@1.7.2` resolved the CLI and its unlocked dependencies from the npm registry on every run; the stage took between 5 s and 118 s against a 120 s limit. **Fix:** Give the verifier its own lockfile for `skills@1.7.2` (`scripts/verify-tools/skills/`). Install it with `npm ci --prefer-offline` in the verification workspace and run its bin with Node. No registry metadata is resolved, and every transitive dependency is locked. **Risk:** Low. Verifier-only change. The root dependency tree and the supported Node range are unchanged. Skills' default repository discovery is exercised exactly as before. ## Summary Fixes #6234. The repository skill check is meant to verify Skills' default root discovery against a clean repository snapshot. Its time was dominated by registry resolution, not by the check itself, so a slow registry made a required lane fail. ## Problem and expected behavior - Main CI run 39709973007 (`ce5bc8cb`), plus PR runs 39710915987 and 39711962559: `repository skill: start` at 02:21:30, then SIGTERM at the 120 s timeout with empty stdout and stderr. - Passing runs in the same window took 4 702 ms, 27 101 ms and 117 695 ms for this stage. - Locally, `skills add` against a full `git archive` snapshot takes ~2 s with a warm npm cache and ~7 s with a cold one, so the variance comes from registry resolution. Expected: this stage needs no network and has a deterministic, locked toolchain. ## Change and scope - `scripts/verify-tools/skills/package.json` and `package-lock.json` (new): a private manifest that pins `skills` 1.7.2, plus its lockfile (8 packages, all with `resolved` and `integrity`). - `scripts/verify-repository-skill.mjs`: copies that manifest and lockfile into the verification workspace, runs `npm ci --prefer-offline --ignore-scripts` there, then runs `node <bin> add …` with the same arguments, environment isolation and timeout as before. - Why not a root devDependency: the first revision pinned it in the root `package.json`. That made `npm ci` fail under `engine-strict` on Node 22.19.x, which is still supported, because `skills` 1.6+ requires `>=22.20.0` (review finding). The separate lockfile keeps that constraint inside the verifier, as it was before. - Not included: no change to what is verified, and no change to other verifiers. Contribution scope: not applicable (CI tooling fix). ## Contract and boundary impact - Semantic owner and earliest changed stage: `verifyRepositorySkill` (package verification only). - Contribution scope fit and linked maintainer decision: not applicable. - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none. The bin is launched through `process.execPath`, which also works on Windows. - Evidence, artifact, provenance, or reconstruction contract: none. - Generated metadata, package, or installation impact: none. The root dependencies and the published package are unchanged. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: none. The verifier itself is the check, and it runs in the `package-e2e` lane. - Base reproduction or other evidence: the CI timings above. Locally the stage now completes in 2 s with a warm npm cache and 6 s with a cold one, returning `{skillsVersion: "1.7.2", files: 7}`. With `--loglevel http`, the locked `npm ci` makes only the 8 tarball requests on a cold cache (plus npm's own version check) and 0 requests on a warm one. It makes no packument (metadata) requests. - Remaining proof gaps: the full `npm run verify:package` was not run locally. The CI `package-e2e` lane on this PR is the end-to-end proof. ## Validation performed - `node -e 'import("./scripts/verify-repository-skill.mjs").then(m => m.verifyRepositorySkill({root, workspace}))'`: passed (2 s warm, 6 s with an empty `npm_config_cache`). The 7 authored files were installed identically. - `npm ci --loglevel http` in the tool directory: tarball requests only, as described above. - Root `npm ci`: passed. The root `package.json` and `package-lock.json` are unchanged from main. - `npm run check:fast`: passed. - `npm run format:check`: passed. - `npx knip`: reports only two "unresolved imports" (`#web-bundle-analysis-worker`, `#javascript-analysis-worker`), in files this change does not touch. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. - Package or release metadata impact: none. Dev dependency only. - Security, privacy, process, or containment review: replaces an unlocked registry resolution with a lockfile that has integrity hashes. `--ignore-scripts` prevents install scripts from running. ## Author readiness Human owner: @kaoru0822-kitauji - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:46:14 +00:00