Conversation
Session Bundles statically import the node:zlib Zstandard bindings, which Node added in 22.15.0 and 23.8.0. The declared >=22.19.0 baseline accepts 23.0 through 23.7, which lack them, and it is copied verbatim into the published package, so npm installs on those releases without a warning. Setup then pinned whatever runtime it ran on without checking it, producing a managed deployment whose Host crashes on every start; because an existing pin is carried forward rather than reselected, every later update kept it. Narrow the supported range to the one the code actually needs, in the root manifest the published package inherits and in the release validator, and probe the runtime a deployment is about to pin so an unusable one is refused before it reaches the record. Missing or unrecognized version evidence is treated as unknown rather than unusable, so an unreadable pin never blocks a setup that would otherwise succeed. Generated-by: Claude Code (Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hqhq1025
left a comment
There was a problem hiding this comment.
Two current-head blockers remain in the managed Runtime Host admission boundary. The declared Node range itself matches actual Node 22.19, 23.7, and 23.8 zstd availability, and the local build/test gates pass, but existing deployments can still carry an unsupported runtime through the canonical update path and exact-binary probe failures still fail open.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| current: RuntimeHostManagedDeploymentConfig | undefined, | ||
| deps: Pick<RuntimeHostSetupDeps, 'probeNodeRuntimeVersion'>, | ||
| ): Promise<string> { | ||
| const nodePath = current?.launch.nodePath ?? process.execPath; |
There was a problem hiding this comment.
[P2] Apply this validation to the canonical update path as well
runtime-host service update never calls this helper. runtime-host-update-command.ts:787-798 copies current.launch.nodePath into desired and then calls replaceLifecycle. A production-path probe with a canonical deployment pinned to /opt/node-v23.7.0/bin/node returned exit code 0 with update.kind === "repaired", and the replacement received that same unsupported path. This is the repair path for already affected installations, so they remain unable to start after a reported successful repair. Please move or share the runtime validation at the update authority before staging/retirement.
| windowsHide: true, | ||
| }); | ||
| return stdout.trim() || undefined; | ||
| } catch { |
There was a problem hiding this comment.
[P2] Treat a failed exact-binary probe as unusable
This catch converts ENOENT, EACCES, timeout, nonzero exit, and malformed/empty output to undefined; unsupportedNodeRuntimeMessage(undefined, ...) then permits the path. The new tests explicitly preserve this behavior, and the setup regression retains a missing /opt/maka/node pin. Both supervised and on-demand launch later execute the exact stored nodePath, so these failures are evidence that the selected runtime cannot run the Host, not an absence of relevant evidence. Please preserve the probe failure kind and reject execution/protocol failures before retaining or writing the deployment config.
|
Thanks — both findings hold up. Short version: [P2] update path. Correct, and it invalidates a claim in my PR description. Instead of duplicating the check in the update command, I would put the single authoritative guard in [P2] probe failures. Agreed in principle — the stored
Will push both changes with tests covering the update path and each probe failure kind. |
An update replaces only the package, carrying `launch.nodePath` forward untouched, so validating it during setup left the repair path for already affected deployments unguarded: `runtime-host service update` reported a successful repair while handing the replacement the same runtime that cannot load the Host. Move the authoritative check to `replaceRuntimeHostLifecycle`, where setup, update, peer management, and the lifecycle manager all converge, beside the decode and supervised preflight that already gate admission. The update command checks first as well so a package is not staged only to be refused. Preserve the probe failure kind instead of collapsing it. A deployment launches its pinned binary directly, so a path that does not exist, cannot be executed, exits non-zero, or answers unintelligibly is evidence the runtime cannot run the Host. Only a probe that could not answer at all — a timed out spawn — leaves the runtime unjudged. Two fixtures pinned runtimes that never existed to show a pin is carried forward verbatim; they now use a usable binary reached by a path of its own. Generated-by: Claude Code (Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed in 74e526a. Both findings addressed. Update path. The authoritative check now lives in Probe failures. On the fixtures: both New coverage: each probe outcome in The PR description has been corrected — it previously claimed the setup-time check covered the update path, which was exactly what you caught. Full CLI suite on Node 24.21.0: 1056 passing, 0 failing, 3 skipped; typecheck, lint, format, ASF headers, locale hygiene all pass. |
…t-node-runtime-support
hqhq1025
left a comment
There was a problem hiding this comment.
The two blockers reported on the previous head are fixed: the canonical update path now checks the carried-forward runtime before staging, and missing, non-executable, nonzero, or malformed probes are rejected at the shared lifecycle boundary.
One current-head blocker remains. A probe timeout is still treated as permission to continue, so replacement can retire the current lifecycle and commit a successor whose pinned runtime was never verified. Local build, typecheck, CLI, release-policy, locale, lint, format, ASF-header, Biome, and merge-tree checks passed. The hosted test job is red in an unchanged Runtime Host concurrency test; the Runtime Host tree matches current main and the focused test passed 20/20 locally, so I did not attribute that failure to this PR, but the gate remains red.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| nodePath?: string, | ||
| ): string | undefined { | ||
| const where = nodePath ? ` (${nodePath})` : ''; | ||
| if (probe.kind === 'unknown') return undefined; |
There was a problem hiding this comment.
[P2] Stop replacement when the runtime probe times out
This branch converts the only inconclusive probe result into permission to continue. A production probe against a real Node binary with delayed startup returned unknown after about 10 seconds, and the lifecycle transaction accepts that result and can commit the desired on-demand authority. That is unsafe at this boundary: replacement retires the current owner before activation, and an on-demand activation failure intentionally retains the successor, so a stalled or otherwise unverified pinned runtime can become authoritative. Treat unknown as a retryable failure before any lifecycle mutation (or retry under a bounded policy), and assert that the existing authority remains unchanged.
Replacement retires the current owner and commits the successor before activation, and an on-demand update deliberately retains that successor when activation fails — a path whose error is also the one artifacts are never discarded for. Treating a timed out probe as permission to continue could therefore make an unverified pin authoritative with no way back, which is the state this guard exists to prevent. Refuse an unverified runtime too, and classify it apart from an unusable one: nothing was learned about the runtime, so the remedy is a retry rather than a reinstall, and the message says so. Retry a timed out probe once first, so a single slow spawn never decides a deployment. Generated-by: Claude Code (Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed in 788783c. You are right, and my earlier reasoning for keeping the timeout non-fatal was weighing the wrong two risks against each other. I traced the sequence you describe: retirement happens before activation, the successor is committed in between, and on activation failure an on-demand update takes the branch that deliberately keeps that successor authoritative — whose
A timed out probe is also retried once before the runtime is left unverified, so a single slow spawn does not decide a deployment. The per-attempt deadline is unchanged at 10s, and both are injectable so the tests exercise them in milliseconds. New coverage: an unverified probe is refused before the provider is resolved or the operator converged, with the authority record asserted unchanged; a permanently stalling binary yields a refusal classified Full CLI suite on Node 24.21.0: 1108 passing, 0 failing, 3 skipped; typecheck, lint, format, and ASF headers pass. On the red gate: thanks for looking into it rather than attributing it here. For what it is worth, the two runs failed in different places — the first in the Storybook smoke on two |
Astro-Han
left a comment
There was a problem hiding this comment.
Review
Good change, and the boundary you picked is the right one. I checked the detection logic, the three call sites, and the blast radius of the new refusal.
What I verified
isSupportedNodeRuntimeVersion(packages/cli/src/node-runtime-support.ts:64) agrees with the declared range where it matters: 22.19+ accepted, 22.18.9 / 23.0–23.7 rejected, 23.8+ accepted, andmajor > 23keeps future lines in — consistent with>=22.19.0 <23.0.0 || >=23.8.0.- The premise holds:
packages/storage/src/session-bundle-file-service.ts:38statically importscreateZstdCompress/createZstdDecompressfromnode:zlib, so a missing export is a link-time failure, not a runtime one — a version gate is a fair proxy for it. - The refusal is genuinely pre-destructive: it precedes
resolveProvider(...).supervisor.preflight()andretireRuntimeHostLifecycleOwner(runtime-host-lifecycle-transaction.ts:573-587).uninstallandstop/retirenever reachreplaceRuntimeHostLifecycle(runtime-host-managed-lifecycle-manager.ts:238,:172), so a broken record can still be removed — the only remedy isn't locked out by the guard. - A refused transaction or setup leaves no staged artifacts:
canDiscardRuntimeHostLifecycleDesiredArtifactsis true for the new codes, so the rollbacks atruntime-host-setup-command.ts:597andruntime-host-update-command.ts:910still run. - The pre-existing fixtures that carry a fake
launch.nodePath(/usr/bin/node,/secret/node,/node) sit on paths that never reach the transaction, which is consistent with the suite passing; the two you re-pointed inruntime-host-selected-update.test.ts:442,464were the only ones that did.
The remediation in the message cannot be followed for a carried-forward pin
Not a nit — this is the exact population the PR is about. node-runtime-support.ts:103 ends with "install Node.js <range> and retry". That is actionable for a fresh install, where the pin is process.execPath. It is not actionable for the #5424 case: both writers carry the pin forward (runtime-host-setup-command.ts:620 and runtime-host-update-command.ts:781 both read current.launch.nodePath), and neither setup nor update exposes a way to change it. A user who installs 23.8 and retries gets the identical refusal, because the pin still names the old binary. Since uninstall + reinstall is the only remedy, naming it (or pointing at #5424) would keep the message from sending people down a path that cannot work.
Suggestions and nits
- The probe proves less than the claim.
node -p process.versions.nodeestablishes "this binary runs and reports a version in range", not "the Host can load". Since the invariant is anode:zlibexport, something like-p "typeof require('node:zlib').createZstdCompress"would detect the real condition and stop23.8.0from being a magic number that has to be re-derived by hand. Trade-off: it would also refuse a future Node that renames the export, so keep the range check alongside it. Your call — the version gate is defensible as-is. - Three copies of one policy, and the test that could tie them together hardcodes the literal.
package.json:7(which becomes the publishedenginesviascripts/release-cli-package.mjs:691),node-runtime-support.ts:31, and the release script's own re-implementation atscripts/release-cli-package.mjs:264-272. The test atnode-runtime-support.test.ts:43asserts the literal instead of reading the root manifest, so drift between the manifest and the constant stays invisible. Readingengines.nodethere would be a cheap invariant. - The new range's second branch is exercised nowhere in CI. The smoke matrix runs 22.19.0 and 24 only (
.github/workflows/cli-package-validation.yml:261-263), andci.ymlruns 24 (:195), so "23.8 has the bindings" is still only asserted by unit tests written against the same constant. Either add a 23.8 leg or take the>=24.0.0narrowing you offered under "Review focus" — with 23.x absent from CI entirely, the simpler range is also the safer one. - Two places where the PR body and the code disagree. (a) "Only a probe that could not answer at all … leaves the runtime unjudged; hard-failing there would turn a healthy pin into a flaky refusal" reads as pass-through, but the code throws
node_runtime_unverified(node-runtime-support.ts:88-92,runtime-host-lifecycle-transaction.ts:577-582) and the lifecycle test asserts the rejection. The rationale in the code comment is good; the body should say the same. (b) "so neither resolves nor stages a package only to be refused at the boundary" holds for update (the check atruntime-host-update-command.ts:780precedesprepareDeploymentat:794) but not for setup:resolveRuntimeHostSetupPackagehas already run (runtime-host-setup-command.ts:464,:821) and the supervised path has unpacked the package before:642refuses it. Rollback covers the leftovers, so it is only a wasted download — moving the setup check above package resolution would make the claim true. - nit: "it exited with code 1" (
node-runtime-support.ts:168) gives the user nothing to act on; naming what was executed (<node> -p process.versions.node) would make it reproducible. - nit: the update guard sits after the
already_currentearly return (runtime-host-update-command.ts:764-773), soservice updateon a broken pin that is already at the target version still exits 0. Nothing is written, so probably fine — flagging for completeness. - nit: on the happy path the runtime is probed twice (the setup/update check, then the transaction at
runtime-host-lifecycle-transaction.ts:573), and the self short-circuit is a string comparison (node-runtime-support.ts:118), so a pin recorded asrealpath(process.execPath)still spawns a subprocess. - Question, not a defect:
start/restartand on-demand activation bypass the transaction (runtime-host-managed-lifecycle-manager.ts:130-172), so a record already pinned to an unusable runtime still crash-loops with the #5424 symptom instead of the new code. That is consistent with a writers-only boundary — just confirming it is deliberate.
Nothing here blocks the change for me: the guard sits at the right place, refuses only on evidence, keeps the unverified case non-authoritative, and the tests fail when it is removed.
Summary
A managed Runtime Host deployment pins one absolute
nodePathfor the life of the record, and nothing checked that the pinned runtime could actually run the Host. Installing on Node 23.0–23.7 therefore produced a deployment that crashes on every start, reported as an unrelated diagnostic, with no supported way to correct the record.Session Bundles statically import the
node:zlibZstandard bindings (packages/storage/src/session-bundle-file-service.ts:38), which Node added in 22.15.0 and 23.8.0. The declared>=22.19.0baseline accepts 23.0–23.7, which lack them, and that value is copied verbatim into the publishedmaka-agentpackage (scripts/release-cli-package.mjs:686), sonpminstalls on those releases without a warning. CI covers only 22.19.0 and 24, so the gap is never exercised.This change:
>=22.19.0 <23.0.0 || >=23.8.0— in the root manifest the published package inherits, invalidateNodeVersion(), and in the documented prerequisites;packages/cli/src/node-runtime-support.ts, which owns the range, the message, and a probe reporting what a Node binary says it is;replaceRuntimeHostLifecycle, the boundary where every writer of a deployment record converges.Where the guard lives
An existing pin is carried forward rather than reselected, so the check must judge the binary rather than this process.
replaceRuntimeHostLifecycle(runtime-host-lifecycle-transaction.ts) is the single point all five writers reach — setup (both drafts), update, peer management, and the managed lifecycle manager — and it already decodes the desired config and runs the supervised preflight, so the runtime check sits beside the existing admission validation. Setup and the update command check first as well, so neither resolves nor stages a package only to be refused at the boundary.This matters most on the repair path: an update replaces only
launch.packageand carrieslaunch.nodePaththrough untouched, so a deployment pinned to an unusable runtime previously reported a successful repair while handing the replacement the same runtime.Probing short-circuits when the path resolves to
process.execPath, which is every fresh install, so no subprocess is launched on the common path.What counts as unusable
The pinned binary is launched verbatim later, so a path that does not exist, cannot be executed, exits non-zero, or answers unintelligibly is evidence the runtime cannot run the Host, and is refused. Only a probe that could not answer at all — a timed out spawn on a loaded machine — leaves the runtime unjudged; hard-failing there would turn a healthy pin into a flaky refusal.
Two fixtures pinned runtimes that never existed in order to show that a pin is carried forward verbatim (
runtime-host-setup.test.ts,runtime-host-selected-update.test.ts). They predate this PR; they now make the same point with a usable binary reached by a path of its own.Fixes #5424
Verification
Run on Node 24.21.0 / npm 11.19.0:
npm --workspace maka-agent run test:dist— 1056 passing, 0 failing, 3 skipped.npm --workspace maka-agent run typecheck,npm run lint,npm run format:check,npm run check:asf-headers,npm run check:locale-hygiene— all pass.node-runtime-support.test.tscovers the range boundaries (22.14/22.18.9/23.0/23.7 rejected; 22.19/23.8/24 accepted), and each probe outcome: absent path, non-executable file, non-zero exit, unintelligible output, a usable binary at its own path, and the self short-circuit.runtime-host-lifecycle-transaction.test.tsasserts an update carrying an unsupported or unexecutable pin is refused before the provider is resolved or the operator converged, that the authority record is left intact, and that a probe which could not answer lets the transaction proceed and fail on its own terms.runtime-host-selected-update.test.tsassertsruntime-host service updaterefuses a deployment pinned to an unusable runtime before staging or retirement.0and writes the record.Reproduced the original defect on the affected machine before the change — a supervised LaunchAgent deployment pinned to Node v23.7.0, crash-looping with
SyntaxError: The requested module 'node:zlib' does not provide an export named 'createZstdCompress'every ~2s while Desktop reportedMANAGED_ROOT_REQUIRES_OPERATOR. Details and logs are in #5424.Review focus
Range policy. Node 23.x is an odd line that reached end of life in June 2025 and is not exercised in CI, so
>=22.19.0 <23.0.0 || >=24.0.0would be simpler to state and strictly safer, at the cost of dropping 23.8–23.11, which do work. I chose the technically minimal variant to keep this a pure bug fix; switching is a one-line change plus the prerequisite wording.Re-pointing an existing pin. Refusing an unusable carried-forward pin turns a silent failure into a clear error, but the only remedy remains uninstall + reinstall. Whether
updateshould be able to re-pointlaunch.nodePathexplicitly felt like a separate design question and is noted in #5424 rather than answered here.Legacy service path. The legacy
RuntimeHostManagedServiceConfigpath pinsnodePaththroughprepareServiceConfiginruntime-host-service-manager.tsand does not pass through the lifecycle boundary. It is untouched here; happy to extend if that path is still expected to be reachable.The misreporting half of the original symptom — a crash-looping managed Host surfacing as
MANAGED_ROOT_REQUIRES_OPERATOR, with the service's stderr never reaching any surface — is filed separately as #5425 and is untouched by this PR.AI use
Select exactly one:
Tool(s) and scope: Claude Code (Opus 5) — root-cause investigation, the implementation, and the tests. Reviewed and submitted by me; commits carry a
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?