Skip to content

Windows local archive deployments avoid unnecessary npm work - #2374

Merged
kriszyp merged 11 commits into
mainfrom
fix/windows-component-suite-timeout
Aug 28, 2026
Merged

Windows local archive deployments avoid unnecessary npm work#2374
kriszyp merged 11 commits into
mainfrom
fix/windows-component-suite-timeout

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 28, 2026

Copy link
Copy Markdown
Member

Windows drive-letter archive paths were mistaken for qualified package identifiers, so deploy_component synchronously ran npm packaging—and, for zero-work manifests, npm installation—before it could return response headers. This change classifies absolute archives before protocol detection, skips automatic installs with no production work, and makes remaining automatic npm installs production-only without raising client timeouts or changing shard budgets. Refs #2273.

For the human reviewer

  1. Keep the remediation narrow. Historical instance logs prove the requests reached Harper and blocked inside component preparation, but the old logs cannot distinguish a child that never exited, inherited stdio that never closed, or the deadline-free Windows process-tree confirmation loop. Planning review twice preferred hardening that supervisor; after a 40-deployment Windows diagnostic run found no supervisor failure, the owner explicitly chose the lower-risk archive/install fix plus permanent stage markers. Accepting this PR leaves supervisor semantics unchanged and keeps Windows CI: deploy_component (restart:true) hangs after npm pack — risk-query integration suite cancelled at 319s #2273 open for staged recurrence evidence; rejecting this choice means broadening the PR into process-liveness and rollback behavior.

  2. Automatic npm installs become production-only. The owner approved --omit=dev --no-audit --no-fund; runtime dependencies must live in dependencies, and install_command is the compatibility escape hatch for builds that need development tooling. This can break components that previously relied on runtime imports or allowed lifecycle scripts from devDependencies; reversing the flags is mechanically small but rollout-sensitive.

  3. A root-manifest predicate suppresses zero-work npm. Empty/default npm manifests do not start a child, while an explicitly selected non-npm manager still runs so it can inspect manager-specific workspace configuration. Always invoking npm would preserve maximum discovery but retains the proven unnecessary synchronous work; changing this predicate later is localized, but its twin restart-metadata predicate must remain aligned.

  4. Windows directory semantics remain provenance-sensitive. Bare absolute directories keep their historical npm-pack copy behavior so Harper rollback cannot operate through a live link into the operator's source tree; explicit file: and relative directories retain their historical symlink behavior. Linking every local directory would simplify routing but changes rollback isolation; the raw-identifier flag makes this choice locally reversible.

  5. The end-to-end oracle uses complete debug logs. The affected tests require positive direct-extraction and install-branch markers, reject throttled logs, and then assert the unnecessary spawn is absent. Test-only process instrumentation would decouple the proof from diagnostics but add a production observation seam; this log-based choice is isolated to integration tests and easy to replace.

Verification

  • npm run build — passed.
  • npm run lint:required — passed.
  • env -u GIT_CONFIG_GLOBAL -u GIT_PAGER npx mocha "unitTests/components/**/*.test.js" — 1541 passing, 1 pending.
  • env -u GIT_CONFIG_GLOBAL -u GIT_PAGER npm run test:integration -- integrationTests/components/risk-query.test.ts integrationTests/components/early-hints.test.ts — 21 passing, 0 failed/cancelled; direct archive, exact install branch, no-spawn, and source-archive preservation assertions passed.
  • Exact-head Windows Node 24 verification — PR run, repeat 1, and repeat 2: all six Windows shards passed each time; risk-query (shard 2) and early-hints (shard 4) passed in all three runs with zero failures or cancellations.
  • Isolated origin/main fails-on-base check — failed the Windows path-classification and dev-only runtime-metadata assertions as expected.
  • Final pre-push review — Claude + Gemini with Harper-domain adjudication; converged with no surviving findings at d22f6d1473a9.

— GPT-5 Codex

🤖 Generated with Codex

Complexity: medium

Review-Coverage: authored=codex; ran=gemini,claude; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=4 @ d22f6d1

Human-Review-Need: 3 (decisions: production-only-auto-install, zero-work-install-skip, windows-directory-materialization, log-oracle-routing-proof) @ d22f6d1

kriszyp and others added 11 commits August 27, 2026 08:56
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Classify Windows archive paths before package protocol detection, skip automatic installs with no production work, and make npm's automatic path production-only. Keep custom install commands and lifecycle opt-ins intact while adding durable child-process stage logs.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Document the shared runtime-metadata predicate, the explicit install-command escape hatch, and the Windows archive path rule so later component changes preserve the deployment semantics.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Classify absolute paths without synchronous filesystem probes, defer file type handling to async extraction, and preserve copy-based Windows directory deployment. Keep explicit non-npm workspace installs conservative while zero-work npm manifests skip child creation.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Retain the raw identifier provenance needed to pack only bare absolute Windows directories while leaving explicit file and relative directories on their established symlink path. Tighten the integration log oracle and reuse the component spawn logger.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Share the bare-absolute-path predicate between identifier derivation and Windows directory materialization, and assert that direct deployment leaves the source archive intact.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces automatic, production-only npm component installation, skipping the package-manager step when no production dependencies, workspaces, or install lifecycles are declared. It also adds support for Windows absolute paths and direct local archive extraction without repacking, accompanied by comprehensive unit and integration tests. Feedback is provided regarding the INSTALL_LIFECYCLE_SCRIPTS set, which incorrectly includes 'dependencies'—a top-level package.json field rather than a script lifecycle name.

Comment thread components/Application.ts
Comment on lines +499 to +508
const INSTALL_LIFECYCLE_SCRIPTS = new Set([
'preinstall',
'install',
'postinstall',
'prepublish',
'preprepare',
'prepare',
'postprepare',
'dependencies',
]);

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.

medium

The INSTALL_LIFECYCLE_SCRIPTS set contains 'dependencies', which is a top-level field in package.json rather than a script lifecycle name. Removing 'dependencies' fixes this correctness issue.

const INSTALL_LIFECYCLE_SCRIPTS = new Set([
	"preinstall",
	"install",
	"postinstall",
	"prepublish",
	"preprepare",
	"prepare",
	"postprepare"
]);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

dependencies is intentionally present here as a key under package.json#scripts, not the top-level dependency map. npm documents it as a special lifecycle script that runs after commands change node_modules (https://docs.npmjs.com/cli/using-npm/scripts/#dependencies). Keeping it ensures an application whose only allowed install-time work is scripts.dependencies does not incorrectly take the zero-work skip branch.

@kriszyp
kriszyp marked this pull request as ready for review August 28, 2026 13:25
@kriszyp
kriszyp merged commit ad9854b into main Aug 28, 2026
138 of 139 checks passed
@kriszyp
kriszyp deleted the fix/windows-component-suite-timeout branch August 28, 2026 13:25
Comment thread components/Application.ts
Comment on lines +499 to +508
const INSTALL_LIFECYCLE_SCRIPTS = new Set([
'preinstall',
'install',
'postinstall',
'prepublish',
'preprepare',
'prepare',
'postprepare',
'dependencies',
]);

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.

1. Missing per-name test coverage for INSTALL_LIFECYCLE_SCRIPTS iteration

File: components/Application.ts:499-508 (used at :567, gating the skip-install decision at :1584)

What: This new 8-entry Set feeds packageHasAllowedInstallLifecycleWork's .some() loop, which decides whether installApplication takes the zero-work skip path even when allowInstallScripts is true. unitTests/components/applicationInstall.test.js exercises only one entry (prepare, "runs an allowed install lifecycle..." at line ~130) — the other seven (preinstall, install, postinstall, prepublish, preprepare, postprepare, dependencies) have no direct coverage.

Why it matters: A future typo or edit to any of the untested seven names would silently make packageHasAllowedInstallLifecycleWork stop recognizing that lifecycle script, causing installApplication to skip the install even though a legitimate script needs to run — a regression no current test would catch.

Suggested fix: Parametrize the existing "runs an allowed install lifecycle" test (or add a sibling test) over each name in INSTALL_LIFECYCLE_SCRIPTS, asserting the install is not skipped when only that one script is present with allowInstallScripts: true.

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

1 blocker posted inline: new INSTALL_LIFECYCLE_SCRIPTS iteration in components/Application.ts (8 lifecycle-script names) has direct test coverage for only 1 entry. Prior gemini/kriszyp thread on dependencies in that same Set is not re-raised — kriszyp's (MEMBER) explanation that it's a real npm lifecycle script is correct.

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.

2 participants