Repository navigation
cherry-pick: Honor install-script policy in custom component commands (v5.2) - #2575
Closed
github-actions[bot] wants to merge 3 commits into
Closed
github-actions[bot] wants to merge 3 commits into
github-actions[bot] wants to merge 3 commits into
Conversation
Apply npm's ignore-scripts configuration to custom component install commands when scripts are disallowed, and let install_node_modules pass its explicit policy through the shared argument builder without changing its historical default. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
kriszyp
marked this pull request as draft
October 1, 2026 23:54
Keep v5.2's automatic install paths and legacy npm arguments while applying #2572's custom-command and deprecated-operation lifecycle script policy. Consume converted policy values locally, adapt the copied tests to release semantics, and keep the design record in the release branch's root document. Co-Authored-By: GPT-5 Codex <noreply@openai.com> Dispatch-Task: cherry-resolve-kriszyp_harper_2572-910d0d15
Remove unused workspace and production-install fixtures from the copied argument tests, and state the current operation contract in the design notes. Co-Authored-By: GPT-5 Codex <noreply@openai.com> Dispatch-Task: cherry-resolve-kriszyp_harper_2572-910d0d15
This was referenced Oct 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves the committed conflict markers from #2572's v5.2 backport. Custom component commands now honor the lifecycle-script policy, and the deprecated operation uses its validated policy while preserving v5.2's install arguments and scripts-enabled operation default.
For the human reviewer
Framing-Verdict: better-alternative-existswas resolved by keeping the narrower release-compatible implementation: retain v5.2 automatic installs and legacydryRun, consume Joi conversions locally for the new policy, and drop copied tests for unrelated main behavior. Porting main's production-work predicates, audit flags, shared argument builder or dry-run changes would expand this backport beyond Honor install-script policy in custom component commands #2572.install_allow_scripts: trueor root-configinstall.allowInstallScripts: trueis set; cold installs relying on native-module or generation hooks need that opt-in. The warning goes to server logs. The v5.2.x release note should state: “install_commandruns with npm lifecycle scripts disabled unlessinstall_allow_scripts: true(operations API) orinstall.allowInstallScripts: true(root config) is set.” Arbitrary custom commands can override npm configuration, and package managers that ignore it are outside this best-effort enforcement.deploy_componentvalidator accepts string booleans but forwards the raw request.install_allow_scripts: "false"is truthy at the custom-command boundary; use JSON booleanfalse. The same mapping is present in source commit7f0d5a08c; correcting it across deployment routes needs a separate upstream fix. The deprecated operation in this PR does consume converted string false values.v5.2(78e52844548477f97441ec6c9fb56a2a1b723ec5) into this branch as a new merge commit, then rerunning both workflows. A read-only merge preview is clean and leaves exactly this five-file backport relative to v5.2. No merge has been performed; dispatch is awaiting the owner's scope decision.Product and architecture tour
Install policy on the release line
How is the approved lifecycle policy applied without changing v5.2's installation conventions?
In
components/Application.ts, the custom command keeps its arguments and passes a dedicated flag intononInteractiveSpawn; child-environment construction removes case variants and setsnpm_config_ignore_scripts=truewhen scripts are disallowed. Explicit opt-in retains inherited host restrictions. The existing automatic and declared package-manager paths stay as they were.In
utility/npmUtilities.ts, the private Joi validator returns converted values with the existing validation options.install_node_modulesdefaults to scripts enabled, accepts either policy spelling, rejects both together and adds--ignore-scriptsfor false. The operation retainsinstall --force --omit=dev --json, its preparation lock and legacydryRunbehavior. Root DESIGN.md records the policy and command-enforcement boundary in the release branch's existing documentation structure.Verification
npm run buildpasses. The unresolved snapshot failed withTS1185: Merge conflict marker encountered; no markers remain in tracked files.npx mocha unitTests/components/applicationInstall.test.js unitTests/utility/npmUtilities.test.js: 12 passing. Component tests retain real npm undefined/false/true lifecycle-marker checks, warnings, exact custom argv and release-compatible automatic arguments. Deprecated-operation tests cover defaults, both spellings, string false conversion, operation metadata, invalid-policy rejection and legacy dryRun. Fixture registry audit is disabled only in the test environment, and environment/config state is restored.1af3d430363eand pass with this resolution; that baseline builds successfully.npm run format:check, changed-file oxlint andgit diff --checkpass. Fullnpm run lintreports 13 existing warnings in files identical to fetchedorigin/v5.2; no changed-file warning.deploy_componentassertion.49f266b592a94fddebf78ce11a57f9c1c767ece3. Unit Test: the new install-policy suites passed; the sole failing resource test is HNSW greedy routing (unitTests/resources/vectorIndex.test.js:1009). Integration Tests: 20 jobs passed and 7 failed. CRL/OCSP shards 3/4 fail on Node, uWS and Windows (401 expected, 404 received); uWS shard 6 fails during Harper startup ineviction-index-orphan-removal-paths.test.tsbefore its assertions. These source/test areas are identical to the pre-backport base.v5.2at78e52844548477f97441ec6c9fb56a2a1b723ec5has successful unit and integration runs. It contains the matching TLS fix and later native/startup changes; merging it is a proposal to align the baseline, not a claim that the remaining failures are proven fixed.Refs #2572
Complexity: medium
Generated by GPT-5 Codex.
Review-Coverage: authored=codex; ran=cursor-composer,gemini,claude; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ 49f266b
Human-Review-Need: 3 @ 49f266b