Skip to content

fix(validation): disable implicit pnpm installs#670

Merged
hxy91819 merged 1 commit into
mainfrom
feature/disable-pnpm-validation-auto-install
Jul 18, 2026
Merged

fix(validation): disable implicit pnpm installs#670
hxy91819 merged 1 commit into
mainfrom
feature/disable-pnpm-validation-auto-install

Conversation

@hxy91819

@hxy91819 hxy91819 commented Jul 18, 2026

Copy link
Copy Markdown
Member

Summary

  • disable pnpm's verifyDepsBeforeRun behavior for validation commands
  • keep the existing lifecycle-hook suppression and workspace filter fail-closed behavior
  • update focused command-boundary assertions

Root cause

Dependency setup and validation intentionally run with separate disposable homes and pnpm stores. With pnpm 11.2.2, verifyDepsBeforeRun treats that profile/store change as a stale install and can run an implicit pnpm install before pnpm check:changed.

That hidden install rewrites runtime metadata such as node_modules/.modules.yaml and .pnpm-workspace-state-v1.json. ClawSweeper then correctly rejects validation because the checkout identity changed; on a full OpenClaw checkout, pnpm may also purge/reinstall the dependency tree or attempt registry access.

Dependency installation is already a separate reviewed phase. Validation should execute only the requested script, so the production command now passes --config.verify-deps-before-run=false alongside --config.enable-pre-post-scripts=false.

Validation

  • OpenClaw-shaped container E2E: happy path passed with different setup/validation homes and stores
  • OpenClaw-shaped historical CI regression scenario passed
  • node --test --test-name-pattern='package validation execution suppresses lifecycle hooks and implicit installs' test/repair/target-validation.test.ts
  • pnpm --config.verify-deps-before-run=false run build
  • pnpm --config.verify-deps-before-run=false run build:repair
  • pnpm --config.verify-deps-before-run=false run lint
  • focused oxfmt --check
  • isolated autoreview: no findings, correctness confidence 0.97

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Jul 18, 2026

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 8:29 AM ET / 12:29 UTC.

Summary
Adds pnpm's verify-deps-before-run=false to repair validation commands and updates focused command-boundary assertions so validation cannot trigger an implicit install.

Reproducibility: no. current-main run was performed in this review; the command boundary and the reported separate-store setup make the failure mode source-reproducible, while the PR body reports an OpenClaw-shaped E2E reproduction.

Review metrics: 1 noteworthy metric.

  • Patch scope: 2 files affected; 13 additions, 7 deletions. The change is confined to the pnpm validation command boundary and its focused regression assertions.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] Disabling pnpm's implicit install intentionally turns stale or missing dependency state into a validation failure, so the existing invariant that every validation entry point runs reviewed dependency setup first must remain true.

Maintainer options:

  1. Approve the intentional fail-closed validation behavior (recommended)
    Accept validation failing on stale dependencies rather than allowing pnpm to mutate the checkout, relying on the existing reviewed setup phase.
  2. Pause if validation must self-repair dependencies
    Do not merge this change if repair validation is intended to install or refresh dependencies during execution rather than fail after setup is incomplete.

Next step before merge

  • [P2] A repository MEMBER authored this PR, so policy requires a maintainer to review and approve the intentional validation-availability tradeoff rather than routing it to automated repair or closure.

Security
Cleared: The focused command change reduces unintended install, lifecycle, and registry-execution exposure during validation and introduces no new dependency, secret, permission, or execution source.

Review details

Best possible solution:

Keep dependency installation as the separate reviewed phase and land the non-mutating validation command only after a maintainer confirms the setup-before-validation invariant and required checks for the rebased head.

Do we have a high-confidence way to reproduce the issue?

No current-main run was performed in this review; the command boundary and the reported separate-store setup make the failure mode source-reproducible, while the PR body reports an OpenClaw-shaped E2E reproduction.

Is this the best way to solve the issue?

Yes. Keeping dependency setup separate and making validation execute only the requested script is the narrowest maintainable fix for checkout mutation during validation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 12cfc6670cef.

Label changes

Label justifications:

  • P2: This is a bounded repair to validation reliability with limited blast radius across repair execution paths.
  • merge-risk: 🚨 availability: Validation now fails instead of implicitly installing dependencies when setup state is stale or incomplete.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This is a MEMBER-authored PR, so the external-contributor real-behavior-proof gate does not apply; the PR body nevertheless reports container E2E and focused validation evidence.
Evidence reviewed

What I checked:

  • Production command change: The PR adds --config.verify-deps-before-run=false only to the pnpm branch of validationCommandForExecution, alongside the existing lifecycle-hook suppression, preserving the command's focused validation role. (src/repair/validation-command-utils.ts:712, bc3496b8ed62)
  • Focused regression coverage: The PR updates command-array and invocation-log assertions for pnpm validation paths, including filtered and target execution cases, to require the new non-mutating pnpm option. (test/repair/target-validation.test.ts:805, bc3496b8ed62)
  • pnpm behavior supports the root-cause analysis: pnpm 11 changed verifyDepsBeforeRun to default to install; disabling it is consistent with preventing an automatic dependency installation from occurring before a validation script.
  • Repository policy applies: The full target AGENTS.md states that maintainer-authored items, including GitHub MEMBER submissions, are non-closeable and require explicit maintainer handling. (AGENTS.md:1, 12cfc6670cef)
  • Related PR is independent: The merged related PR fix(apply): preserve runtime yields across coverage proof #671 describes a separate runtime-budget/coverage-proof fix and explicitly says it was discovered while validating this PR without incorporating this PR's validation change. (12cfc6670cef)

Likely related people:

  • hxy91819: Authored this focused repair-validation change and a related repair-lane PR that was merged into the current main base, providing the strongest available ownership signal for this active path. (role: recent repair-lane contributor; confidence: medium; commits: bc3496b8ed62, 12cfc6670cef; files: src/repair/validation-command-utils.ts, test/repair/target-validation.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-18T11:14:13.516Z sha 29c8c00 :: needs maintainer review before merge. :: none

@hxy91819 hxy91819 mentioned this pull request Jul 18, 2026
Merged
@hxy91819
hxy91819 force-pushed the feature/disable-pnpm-validation-auto-install branch from 29c8c00 to bc3496b Compare July 18, 2026 12:26
@hxy91819

hxy91819 commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

Rebased current head: bc3496b8ed620afe27b2ca3d42d68b892c7e667c. The production-call-site invariant has been checked: all four validation entry points invoke prepareTargetToolchain() before validation, so --config.verify-deps-before-run=false removes mutation from validation without skipping dependency setup.

@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@hxy91819
hxy91819 merged commit 32d4e4a into main Jul 18, 2026
11 checks passed
@hxy91819
hxy91819 deleted the feature/disable-pnpm-validation-auto-install branch July 18, 2026 12:37
This was referenced Jul 18, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@hxy91819