Skip to content

fix(runner): honor Node build manager during Job admission - #1244

Merged
swjnxyf merged 2 commits into
yyjeqhc:mainfrom
ITSGJY:fix/962-node-build-admission
Oct 21, 2026
Merged

swjnxyf merged 2 commits into
yyjeqhc:mainfrom
ITSGJY:fix/962-node-build-admission

Conversation

@ITSGJY

@ITSGJY ITSGJY commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix Runner Job admission for Node project_build so a build planned from a Project's declared npm, pnpm, yarn, or bun manager is admitted under the same canonical process/digest contract as the typed Build Plan.

  • Reuse ProjectBuildPlan::is_valid() at the Runner Job admission boundary, aligning it with Core wire validation and the Runner-owned planner.
  • Preserve exact command shape, invocation digest, current manifest/lockfile provenance, queued-source revalidation, and the existing Job/Runner authority model.
  • Add a deterministic no-child regression for all four managers with both explicit adapter=node and adapter=auto, including typed wire decode, Job queue admission, and a manager substitution with a recomputed digest rejected by the source fence.

The previous admission validator rebuilt the canonical command with an npm-defaulting helper. A valid pnpm/yarn/bun plan thus passed Runner planning and Core validation but was rejected before Job enqueue. The Runner's current Project recipe remains authoritative for manager selection.

Verification

  • RED (original main@7b35aafb): The new regression fails on pnpm/Node: Runner rejected its own canonical Node build plan.
  • GREEN (commit ed5afe29): The identical regression passes, including all eight manager/adapter combinations.
  • Focused Runner project_build suite: 25 passed, 0 failed, 1 ignored (real Node/npm fixture).
  • Ran the existing ignored real Node/npm build fixture separately: 1 passed.
  • cargo fmt --all -- --check and git diff --check passed.
  • Independent read-only Codex review: no P1/P2 findings.
  • Fork GitHub Actions CI on exact commit ed5afe29eaa2c86e11aaa3b906cb6d3b0959a2f2: Success — 13 jobs passed, 0 failed, 5 correctly skipped by path-based selection (run 39567710526). Linux Runner/Server/workspace, Windows Runner, macOS Core, Contracts, MCP conformance, and the CI test gate passed.

Scope

Only the Runner admission check and its adjacent regression tests change (2 files). No new model-facing tool, schema/capability, executable-selection rule, process fallback, or persistence/retry contract. This follows the existing project-operation lifecycle tracked in #962.

Copilot AI balanced review requested due to automatic review settings October 21, 2026 09:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

ITSGJY commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

CI follow-up for the Windows Runner lane on upstream run 39577268778:

  • The Runner test binary reported 1,025 passed, 4 failed, 55 ignored. All four failures were pre-existing project_format_tests cases returning format_timeout (rather than their expected formatting result/error); the new Node project_build admission regression passed.
  • On the same exact commit ed5afe29, fork run 39567710526 passed the Windows Runner lane and all four formatting cases on the same windows-2025-vs2026 runner image. The upstream Windows lane also took over 60 seconds in an unrelated Ruff real-process test that completed promptly on the fork, consistent with transient child-process slowness.
  • This PR changes only Node Build Job admission and its regression fixture; it does not change project_format or the Windows workflow.

Could a maintainer rerun just the failed test-windows-runner job against the unchanged head SHA? The contributor account lacks upstream Actions rerun permission. I have left the reviewed patch unchanged so that a rerun can verify the exact candidate.

@swjnxyf swjnxyf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed PR #1244 in the MSI main checkout at 6fa4637b9e95130b2816f92afd733a30ec135a55.

The admission change correctly reuses ProjectBuildPlan::is_valid() rather than the old npm-defaulting canonical helper. This accepts Runner-selected npm/pnpm/yarn/bun Node build plans without allowing caller argv or manager substitution: the exact process/invocation digest must match, and the existing project_build::fence still replans the current manifest/lockfile at Job admission/queue execution. No new execution or effect authority is introduced.

Independent Windows checks:

  • Deterministic eight-way (4 managers x Node/Auto) Node Build Job admission regression: passed twice.
  • cargo test -p webcodex-core --lib project_build --offline: 12 passed.
  • Wider webcodex-runner project_build group: 20 passed, 1 ignored, with 1 unrelated python_artifact_observation_uses_existing_metadata_and_bounds 5-second timeout under load; the exact test passed unchanged on isolated rerun in 2.14 seconds. This existing Python observation path is untouched by the PR.
  • cargo fmt --all -- --check and git diff --check: passed.
  • Exact-head upstream CI run 39580760135: 18/18 jobs successful, including Windows Runner.

No blocking findings or corrective commits needed. Approved for squash merge.

@swjnxyf
swjnxyf merged commit 705329a into yyjeqhc:main Oct 21, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants