Skip to content

fix(cli): refuse to pin a Node runtime that cannot run the Host - #5427

Open
MasamiYui wants to merge 4 commits into
apache:mainfrom
MasamiYui:fix/managed-deployment-node-runtime-support
Open

MasamiYui wants to merge 4 commits into
apache:mainfrom
MasamiYui:fix/managed-deployment-node-runtime-support

Conversation

@MasamiYui

@MasamiYui MasamiYui commented Sep 17, 2026

Copy link
Copy Markdown
Member

Summary

A managed Runtime Host deployment pins one absolute nodePath for 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:zlib Zstandard 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.0 baseline accepts 23.0–23.7, which lack them, and that value is copied verbatim into the published maka-agent package (scripts/release-cli-package.mjs:686), so npm installs on those releases without a warning. CI covers only 22.19.0 and 24, so the gap is never exercised.

This change:

  • narrows the supported range to the one the code needs — >=22.19.0 <23.0.0 || >=23.8.0 — in the root manifest the published package inherits, in validateNodeVersion(), and in the documented prerequisites;
  • adds packages/cli/src/node-runtime-support.ts, which owns the range, the message, and a probe reporting what a Node binary says it is;
  • refuses an unusable runtime in 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.package and carries launch.nodePath through 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.ts covers 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.ts asserts 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.ts asserts runtime-host service update refuses a deployment pinned to an unusable runtime before staging or retirement.
  • Each guard fails its test when removed: without the boundary throw the lifecycle test passes an unsupported pin through; without the setup throw, setup returns 0 and 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 reported MANAGED_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.0 would 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 update should be able to re-point launch.nodePath explicitly felt like a separate design question and is noted in #5424 rather than answered here.

Legacy service path. The legacy RuntimeHostManagedServiceConfig path pins nodePath through prepareServiceConfig in runtime-host-service-manager.ts and 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:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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-by trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

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>
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 17, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@MasamiYui

Copy link
Copy Markdown
Member Author

Thanks — both findings hold up. Short version:

[P2] update path. Correct, and it invalidates a claim in my PR description. runtime-host-update-command.ts:787-798 spreads current into desired and replaces only launch.package, so launch.nodePath reaches replaceLifecycle without ever passing the setup-command helper. The repair path for already affected installations is therefore still broken, and "this also catches an already-broken carried-forward pin on update" is wrong. I will correct the description.

Instead of duplicating the check in the update command, I would put the single authoritative guard in replaceRuntimeHostLifecycle (runtime-host-lifecycle-transaction.ts:537). Every path that writes a deployment config converges there — 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. RuntimeHostLifecycleTransactionDeps already carries optional members, so this is one optional dep with a default and no changes at the construction sites. I would keep the cheap early checks so a package is not staged only to be rejected at the boundary.

[P2] probe failures. Agreed in principle — the stored nodePath is executed verbatim later, so ENOENT, EACCES, a non-zero exit, or malformed output are evidence the runtime cannot run the Host. I will preserve the failure kind and reject those. Two things I would like confirmed first:

  • The /opt/maka/node fixture predates this PR (fa25b3d01, runtime-host-setup.test.ts:237); it uses an absent path to show that an idempotent retry preserves the pin verbatim. Rejecting ENOENT requires changing it, so that becomes a deliberate behavior change rather than a silent break.
  • I would keep a probe timeout non-fatal. A loaded machine can time out spawning a healthy runtime, and hard-failing there turns a good pin into a flaky refusal. Execution and protocol failures reject; only the timeout stays unknown. Happy to make timeouts fatal too if you would rather.

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>
@MasamiYui

Copy link
Copy Markdown
Member Author

Pushed in 74e526a. Both findings addressed.

Update path. The authoritative check now lives in replaceRuntimeHostLifecycle, beside the existing decode and supervised preflight, so setup, update, peer management, and the managed lifecycle manager are all covered by one guard. RuntimeHostLifecycleTransactionDeps gained one optional member with a default, so no construction site changed. The update command also checks before prepareDeployment, which keeps a package from being staged and then rolled back through canDiscardRuntimeHostLifecycleDesiredArtifacts.

Probe failures. probeNodeRuntime now returns version / unusable / unknown instead of collapsing to undefined. An absent path, a non-executable file, a non-zero exit, and unintelligible output are all unusable and refused. Only a timed out spawn stays unknown; the timeout is 10s.

On the fixtures: both /opt/maka/node (runtime-host-setup.test.ts) and /source/node (runtime-host-selected-update.test.ts) used a runtime that never existed to show that a pin is carried forward verbatim. They now make the same point with a usable binary reached by a path of its own, so the verbatim assertions still hold.

New coverage: each probe outcome in node-runtime-support.test.ts; refusal before the provider is resolved or the operator converged, with the authority record left intact, in runtime-host-lifecycle-transaction.test.ts; refusal before staging or retirement on runtime-host service update in runtime-host-selected-update.test.ts; and an unknown probe letting the transaction proceed and fail on its own terms. Each guard fails its test when removed.

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.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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>
@MasamiYui

Copy link
Copy Markdown
Member Author

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 recovery_failed is also one of the two codes canDiscardRuntimeHostLifecycleDesiredArtifacts refuses to discard for. So proceeding on an unverified probe can leave the old owner retired, an unusable pin authoritative, and the staged artifacts retained: exactly the state this PR exists to prevent, reachable through a timeout. A spurious refusal is recoverable; a spurious commit here is not.

unknown is now refused, and classified apart from unusable. The probe learned nothing about the runtime, so the remedy is a retry rather than a reinstall and the message says so — it carries no version verdict. The boundary raises node_runtime_unverified (unsupported_node_runtime stays for a runtime actually shown to be unusable); setup and the update command mirror that distinction.

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 unverified; a binary slow only on its first spawn is accepted on the retry. The guard fails its test when removed. One fixture carried a deployment config with no nodePath at all, which previously slipped through as unverified — a decoded config always has one, so it now looks like a real record.

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 product-workhub stories, and after merging current main that step passed and two Clients share stable Session creation, CAS configuration, and catalog continuity failed instead, asserting ['committed', 'committed'] against ['committed', 'revision_conflict'], which reads as the two writes not overlapping. That test ran 8/8 locally here, and git diff origin/main...HEAD -- packages/runtime-host is empty. As a fork PR I cannot re-run the job; a committer re-run would settle it.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and major > 23 keeps 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:38 statically imports createZstdCompress / createZstdDecompress from node: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() and retireRuntimeHostLifecycleOwner (runtime-host-lifecycle-transaction.ts:573-587). uninstall and stop/retire never reach replaceRuntimeHostLifecycle (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: canDiscardRuntimeHostLifecycleDesiredArtifacts is true for the new codes, so the rollbacks at runtime-host-setup-command.ts:597 and runtime-host-update-command.ts:910 still 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 in runtime-host-selected-update.test.ts:442,464 were 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.node establishes "this binary runs and reports a version in range", not "the Host can load". Since the invariant is a node:zlib export, something like -p "typeof require('node:zlib').createZstdCompress" would detect the real condition and stop 23.8.0 from 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 published engines via scripts/release-cli-package.mjs:691), node-runtime-support.ts:31, and the release script's own re-implementation at scripts/release-cli-package.mjs:264-272. The test at node-runtime-support.test.ts:43 asserts the literal instead of reading the root manifest, so drift between the manifest and the constant stays invisible. Reading engines.node there 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), and ci.yml runs 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.0 narrowing 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 at runtime-host-update-command.ts:780 precedes prepareDeployment at :794) but not for setup: resolveRuntimeHostSetupPackage has already run (runtime-host-setup-command.ts:464, :821) and the supervised path has unpacked the package before :642 refuses 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_current early return (runtime-host-update-command.ts:764-773), so service update on 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 as realpath(process.execPath) still spawns a subprocess.
  • Question, not a defect: start / restart and 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cli): managed deployment pins a Node runtime without zstd support

3 participants