PR 029 P1: Harden termination promise resolution - #33
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe process transport now protects internally created promises from mutated Promise intrinsics. Termination and cleanup paths await protected promises. New subprocess tests cover hardening failures, timeouts, settlement, cleanup, and intrinsic restoration. ChangesProcess transport promise hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR hardens termination promise handling without introducing a demonstrated merge-blocking correctness or availability issue. It is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant runTermination
participant terminate
participant PlatformTermination
participant awaitClose
runTermination->>terminate: await protected termination promise
terminate->>PlatformTermination: request platform termination
PlatformTermination-->>terminate: return TerminationScope
runTermination->>awaitClose: wait for process close
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
![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: 5aa85827c4
ℹ️ 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".
| } catch { | ||
| // Unreachable for a freshly created promise; see the doc comment. Returning | ||
| // the promise unchanged keeps this helper total. | ||
| } |
There was a problem hiding this comment.
When a promise is non-extensible, this catch silently returns it without the own constructor, so the settlement hardening is lost. This is reachable by enabling an async_hooks hook after module load and calling Object.preventExtensions(resource) for PROMISE resources; with the persistent Promise.prototype.constructor and then substitutions this patch targets, I reproduced an ordinary timeout remaining pending because waitForExit/terminate awaits assimilate the unprotected promise through the hostile then. The definition failure needs a fallback that preserves settlement rather than treating the promise as protected.
Useful? React with 👍 / 👎.
1 participant
Purpose
Stacked validation PR for:
AUDIT-PR029-TERMINATE-RETURNS-THENABLE-DEFEATS-SETTLEMENTThis PR is intentionally narrow and quarantined.
It targets protected parent PR #29:
repair/pr025-normalization-release-orderIt does NOT target
main.It does NOT directly target PR #25.
It does NOT directly target protected PR #10.
Finding
Classification:
CURRENT P1Exact affected parent HEAD:
36f8ae0659f51c0891b93f15a40ea2fab71edd73The process transport remained vulnerable to persistent mutation of
Promise.prototype.thenthrough implicit Promise assimilation performed by theJavaScript runtime.
Even though explicit settlement-critical
.then(...)sites had already beenhardened,
terminate()still returned a Promise from an async function.Resolving the async function's own Promise capability with that Promise can
perform thenable assimilation and consult a mutable inherited
then.A persistent hostile replacement could therefore leave:
The stronger
Promise.prototype.constructor + thenmutation could also pushinternal awaits off their native fast path and into hostile thenable
assimilation.
The defect was independently reproduced on exact protected parent HEAD.
It was also verified to exist in the inherited lineage and was not introduced
by PR #29.
Repair
The bounded repair:
terminate()andreturns only the primitive
TerminationScope;protectPromiseResolution(...)for selected internally-createdPromises that this module later awaits after hostile code may have run;
constructorequal to capturedNativePromise;awaitfrom falling back into hostile thenable assimilation throughmutable
Promise.prototype.constructor;Symbol.specieson that route;Changed files exactly:
src/adapters/process-transport.tstests/adapters/process-transport.test.tsExact quarantine identity
Protected parent PR #29 HEAD:
36f8ae0659f51c0891b93f15a40ea2fab71edd73Repair commit:
5aa85827c41931f67053190c79bfa628a034f680Validated patch SHA-256:
B473C5BB0169204193466BD2E6D9D38C9D19091B64A9861D89F6EB0EDFA1C7F7Patch bytes:
37187Changed files exactly:
src/adapters/process-transport.tstests/adapters/process-transport.test.tsThe committed patch was mechanically verified byte-for-byte identical to the
candidate that passed fresh independent validation.
Independent validation
Fresh independent validation:
PASSThe validator independently:
terminate()return-await mechanism is load-bearing;protectPromiseResolution(...)is load-bearing;definePropertyfailure;SPAWN_FAILEDlaundering;AgentExchangelaundering;Validation evidence
Focused adversarial tests:
4 passedComplete process transport:
154 passed, 9 skippedAdapters suite:
463 passed, 9 skippedFull suite:
1189 passed, 9 skippedTypecheck:
PASSLint:
PASSBuild:
PASSgit diff --check:PASSValidation host:
Windows.
The 9 skipped tests are POSIX-gated tests.
No Windows result is claimed as proof of POSIX runtime behavior.
Public-return observation
Validation separately examined the public Promise returned by
invokeAgentProcess(...)under globally poisoned external Promise semantics.The transport itself still fulfills or rejects that Promise exactly once.
A caller's own ordinary
awaitmay fail to observe an already-settled Promiseafter the caller/runtime has globally poisoned Promise semantics.
That condition was independently classified:
NOT REPRODUCIBLEas an AgentBridge defect.
This PR does not broaden its API surface to address arbitrary external caller
Promise poisoning.
Out-of-scope CURRENT finding
This PR does NOT repair:
AUDIT-PR029-DISCARDED-RUNTERMINATION-REJECTIONClassification:
CURRENT P2That finding remains separately quarantined and must receive its own bounded
repair track.
Protected invariants
This repair preserves:
SPAWN_FAILEDlaundering;AgentExchangelaundering;shell:false;Quarantine rule
This DRAFT PR is evidence/proposal only.
Do not merge it because implementation, independent validation, CI, CodeRabbit,
or Codex reports success.
Required before upward integration into PR #29:
Summary by CodeRabbit