PR 010 Flattened: Commander-Claude transport boundary - #108
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix: harden process lifecycle event dispatch
argv entries, the stdin payload, and environment names and values are all promised to reach the child exactly as supplied, and all four cross the same UTF-8 boundary on the way there. A JavaScript string may hold an unpaired UTF-16 surrogate, which UTF-8 cannot represent, so Node substituted U+FFFD and the child received a value the transport had already validated as something else. An ill-formed environment name arrived as a different name entirely. readInvocation now refuses such a request before anything is spawned: containsLoneSurrogate reports an isolated high or low surrogate, or a high surrogate not immediately followed by a low one, reading code units through the intrinsic captured at module load. argv and stdin report the new ARGUMENT_LONE_SURROGATE and STDIN_LONE_SURROGATE reasons; the environment reuses the existing ENVIRONMENT_ENTRY_INVALID. Valid surrogate pairs are well-formed, so every supplementary-plane character still passes through unchanged. Nothing is normalized, replaced, or rewritten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
runTermination cleared `terminating` as soon as terminate() returned, before the bounded close wait and the final settlement. The guard therefore covered only the kill, so a stronger terminal cause arriving mid-flight started a second lifecycle: it overwrote an already-reported TerminationScope, armed a second close-wait timer that displaced its predecessor's release hook, and left that timer running after the exchange had settled. The reset is gone. Once a lifecycle begins, `terminating` stays true for the rest of the invocation, so the guard now spans the kill, the bounded close wait, and settlement alike. Terminal-cause promotion is unaffected, because claim() decides that independently of this function; what a stronger cause can no longer do is start a second lifecycle over the first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Under Node's permission model `normalizeSpawnArguments` calls `copyPermissionModelFlagsToEnv(env, 'NODE_OPTIONS', args)`, which appends every permission flag from `process.execArgv` to `env.NODE_OPTIONS`. Unlike `copyProcessEnvToEnv` it consults no own-property guard, so the blockers that stop the copied runtime variables cannot stop this write. Against the frozen environment snapshot the assignment throws, and `invokeAgentProcess` reports the resulting TypeError as SPAWN_FAILED — a structurally valid invocation failing for a reason unrelated to the specification, the executable, or the caller. `NODE_OPTIONS` is now the one entry defined as an accessor with a discarding setter. `Object.freeze` only clears `configurable` on an accessor, so the snapshot stays frozen while the write becomes a no-op, and it is absorbed however Node arrives at it rather than only in the shape Node uses today. The child environment is unchanged in both directions: exactly the caller's value when one was supplied, and nothing at all when none was, because the synthetic entry stays out of the `for...in` walk that builds it. The parent's NODE_OPTIONS is never read and its permission flags reach neither the record nor the child. Regression coverage runs the transport inside a real interpreter launched with `--permission --allow-child-process`, distinguishing an ordinary invocation from one where Node propagates the flags, and each probe first proves independently that the interpreter really does write NODE_OPTIONS. In-process invariants replicate Node's assignment against the validated snapshot and pin that the NODE_V8_COVERAGE and z/OS blockers are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…proof PR 013 repair: make no-spawn marker proof conclusive
PR 010 repair: reject lone surrogates in process paths
PR 010 repair: handle Node permission propagation boundary
…-warning PR 010 repair: tolerate Node type stripping warning
…n-qualification PR 022 P3: Qualify process transport exchange outcomes
…claim PR 022 P3: Clarify hardening cleanup contract
…ack-signal PR 026 P2: Use nonignorable fallback child signal
…ck-child-leak PR 025 Finding B: Bound fallback child termination
…handling PR 025 Finding A: Handle probe child spawn errors
…ructor PR 029 P2: Capture stable Error intrinsic
…fication PR 029 P3: Bypass mutable Error Symbol.hasInstance
PR 029 P2: Capture settlement Promise then
A promise is not private between the allocation that makes it and the next statement: an ordinary `async_hooks` init hook receives each newly allocated promise as its own resource and can seal it there, so the own `constructor` this module installs to keep an `await` on its fast path may throw before it can land. Combined with a persistently replaced `Promise.prototype.constructor` and `Promise.prototype.then`, every internal `await` in the termination chain is then pushed into thenable assimilation and left with no continuation, so an ordinary timeout — and the mandatory hardening rejection — never settle. Internal promises are now allocated from an `InternalPromise` whose prototype is created and frozen at module load with `constructor` fixed to the captured intrinsic, so the recognition test is answered one link before `Promise.prototype` and needs no own property on the instance. The platform termination steps report through that capability instead of through the promise the runtime makes for an `async` function, which cannot be protected at all. `protectPromiseResolution` stays as a secondary layer whose failure is now survivable rather than silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dary Flattens the PR 010 transport stack into one main-target candidate. The #25 -> #29 -> #33 -> #34 settlement family is already contained in this branch's base; this merge adds the PR 022 contract clarification so the whole line lands as a single PR. Merged content verified green at tree fd3ddab327cf708c99c0db714885fe1e5a12607b against origin/main 891154e. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MRxUE54XK17eWaYPeGYsVx
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.githubusercontent.com/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: f6ccc69cc9
ℹ️ 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".
|
Merge pull request #108 from LogicDuke/flatten/pr010-transport-boundary PR 010 Flattened: Commander-Claude transport boundary Adds the Commander-Claude process transport boundary as a single main-target Absorbs #10, #22, #25, #29, #33, #34 — a six-deep nested chain flattened into Carries:
BINDING CONDITION: T21 and T22 must be repaired before any caller is wired to Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com |
|
POST-MERGE BINDING CONDITION — PR #108 PR #108 was merged into main at 46929a7. The merged tree is the verified tree fd3ddab327cf708c99c0db714885fe1e5a12607b. This PR carries four known findings:
BINDING CONDITION: T21 and T22 MUST be repaired before any caller is wired to invokeAgentProcess. src/adapters/** is currently unimported and unexported, which is the only reason T21 is unreachable. Wiring a caller to invokeAgentProcess removes that property and makes T21 reachable. Therefore, no future PR may introduce a production caller, export path, routing path, invocation path, adapter wiring, or runtime path to invokeAgentProcess until T21 and T22 are repaired or explicitly re-adjudicated under Commander authority. This comment records the condition missing from the merge commit message. It does not resolve any thread, edit code, edit the deferred register, delete branches, or authorize any feature wiring. |
1 participant
Flattens the PR 010 Commander-Claude transport stack into a single main-target
candidate. No repair, no redesign: this PR carries content that was already
reviewed and verified, consolidated so a 6-deep nested chain becomes one PR.
Prior evidence
PASS_PR10_VIABLEPASS_PR10_MERGED_TREE_VERIFY_GREENPASS_PR10_TRIAGE_COMPLETEPASS_PR10_ADJUDICATION_COMPLETEVerified tree
fd3ddab327cf708c99c0db714885fe1e5a12607bProven reproducible:
merge-tree(#34, #22)->merge-tree(main, result)==fd3ddab327cf708c99c0db714885fe1e5a12607b, againstorigin/main@891154ea608e7e19915dc82112f13b5d7b2ffbba.Verify result on that exact tree (Node v24.12.0):
npm ciPASS,typecheckPASS,
lintPASS,buildPASS,testPASS — 2826 passed, 45 skipped,0 failed. Baseline
mainalone: 2361 passed, 36 skipped, 0 failed. The deltais exactly the two new adapter test files (+465 passed, +9 POSIX-gated skips on
Windows); no pre-existing test changed status.
Changed files — exactly six, all additive
docs/architecture/010-commander-claude-bridge.mdsrc/adapters/agent-transport.tssrc/adapters/process-transport.tstests/adapters/process-transport.test.tstests/adapters/transport-fixtures.tstests/adapters/transport-invariants.test.tsZero modifications, zero deletions, zero conflicts with
main. No rebase wasperformed or required.
Source composition
62ea4a1899731c9b36f8ae065aa858272e5889052fb0ab77Thread triage summary
23 unresolved threads adjudicated against tree
fd3ddab3(not against stale PR heads):Carried CURRENT findings
T21 — P1 — async_hooks prototype reparenting
PRRT_kwDOTzqfcs6anD4N(Codex, PR #34) ·src/adapters/process-transport.tsDisposition: CARRY_INTO_FLATTENED_PR
Reproduced on Node 24.12.0: an
async_hooksinithook that applies bothsetPrototypeOf(resource, Promise.prototype)andpreventExtensions(resource)to an
InternalPromise, combined with a mutatedPromise.prototype, defeatsboth defense layers and leaves the exchange hanging. Reparent-only and
seal-only each remain defended; only the combination succeeds.
No production exposure:
src/adapters/**has no importer anywhere in this treeoutside its own slice, and is not exported from
src/index.ts.Repairing this means designing a mechanism that survives instance reparenting —
its own PR, its own adversarial probe. Not folded into a structural flatten.
T22 must be repaired together with T21.
T4 — P2 — POSIX process group signal after leader exit
PRRT_kwDOTzqfcs6Yoa5H(Codex, PR #10) ·src/adapters/process-transport.tsDisposition: CARRY_INTO_FLATTENED_PR
F3 family breaker ratified: PID-reuse safety (T7) outranks descendant reach
(T4). On POSIX the process-group ID is the leader's PID, so signalling
-pidafter the leader exits draws from the same reused-number space T7 wasaccepted to close. The two are not jointly satisfiable by their proposed
remedies. Any T4 repair requires WF2/WF3 design first (a reuse-proof handle:
pidfd, cgroup, or Job Object).
The contract is already honest here:
TERMINATION_SCOPErecords what wasrequested, never that it completed, and the module documents that absolute
process-tree termination is not claimed.
T12 — Minor — local stdin end on hardening-failure path
PRRT_kwDOTzqfcs6Zh5YJ(CodeRabbit, PR #10) ·src/adapters/process-transport.tsDisposition: CARRY_INTO_FLATTENED_PR
Non-blocking. The behaviour is documented as deliberate ("The local stdin end
is left as it is"); the concern is bounded host-liveness retention after a
rejection that has already been delivered. Reachable only through a child
handle hostile enough to defeat dispatch hardening.
T22 — Minor — probe counter attribution
PRRT_kwDOTzqfcs6anGQF(CodeRabbit, PR #34) ·tests/adapters/process-transport.test.tsDisposition: CARRY_INTO_FLATTENED_PR
Test-only:
protectionFailurescan be incremented by the probe's ownprotectLikeRepair, so the assertion can pass without the transport everfailing a protection. This is the assertion that would witness a failure of the
mechanism T21 concerns, so it should be repaired with T21.
Explicit non-actions
NON-BLOCKING / CARRY_OBLIGATION)
mainstays humanAbsorption plan
After this PR is open and CI-green, separate human authority will be sought to
close as absorbed: #10, #22, #25, #29, #33, #34.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MRxUE54XK17eWaYPeGYsVx