Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a contained server update bug fix: native dependencies are approved and verified before a runtime is published or handed off, while failed validation leaves the existing service untouched. The affected production logic is localized and supported by tests for both successful and failed native-module loading. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe pinned runtime installation now approves required native builds and verifies that ChangesPinned runtime validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The runtime activation flow now validates required native dependencies before replacing or reusing a server runtime, with no current merge-blocking risk identified. Sequence Diagram(s)sequenceDiagram
participant BootOrUpdate
participant ensurePinnedRuntimeInstalled
participant validatePinnedRuntime
participant HostNode
participant node-pty
BootOrUpdate->>ensurePinnedRuntimeInstalled: provide host execPath
ensurePinnedRuntimeInstalled->>validatePinnedRuntime: validate staged or cached runtime
validatePinnedRuntime->>HostNode: require node-pty with CommonJS check
HostNode->>node-pty: load native module
node-pty-->>HostNode: success or load error
validatePinnedRuntime-->>ensurePinnedRuntimeInstalled: validation result
ensurePinnedRuntimeInstalled-->>BootOrUpdate: prepared runtime or error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
49f6449 to
de5b70d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note 🤖 GPT-6 on behalf of Oliver Closing as superseded by #11510, which is already in both This PR adds npm build approvals and a Node-based |
Note
🤖 GPT-6 on behalf of Oliver
ELI5
Check that an update's terminal dependency works before switching to it.
Problem
npm 12 can exit successfully while skipping unapproved native builds. T3 then marks the runtime complete, but the new server fails to start because
node-ptyis missing its native binary.Fix
Write staging-local build approvals for
node-ptyandmsgpackr-extract. Load the candidate'snode-ptywith the service's Node executable before publishing or reusing the runtime. This covers local service installation and remote updates, preserving the running service on failure.Validation: 54 focused tests, targeted lint, and server typecheck pass. Isolated npm 12 fixtures confirmed approved scripts run while unrelated scripts stay blocked; npm 11 accepts the manifest. Independent review found no actionable issues.
UI Changes
Before: no visual changes.
After: no visual changes.
Made with GPT-6 in Codex.
Note
Probe
node-ptywith host Node executable before activating pinned runtime updatesnode-ptyvia the host NodeexecPathbefore the completion sentinel is written or the staged directory is published; npm installs also receive a project manifest granting build approval fornode-ptyandmsgpackr-extract.bootService.tsandselfUpdate.tspass the host executable path intoensurePinnedRuntimeInstalledso the probe runs before server preflight and launcher handoff.bootService.test.ts,pinnedRuntime.test.ts, andselfUpdate.test.tscover the probe success and failure paths, including verification that a failed probe leaves the running service untouched.node-ptyload probe now produce aPinnedRuntimeInstallErrorand leave no final installation, whereas previously a runtime with missing or unusable native binaries could be accepted.Macroscope summarized 49f6449.
Summary by CodeRabbit