setup: correct the mechanism in #6, and widen it past npm's own cache - #7
Open
noammigdali-hio wants to merge 1 commit into
Open
Conversation
The first commit on this branch claimed the cached binary "is gone the moment npx exits". That is false. npm's npx cache persists on disk; what goes away is the PATH entry. A mechanism was asserted rather than checked, in the single sentence the change rested on. The fix was also too narrow. Keying on `_npx` misses every other channel that produces the same failure: `pnpm dlx`, `yarn dlx`, `bunx`, and setup run inside a project that already depends on human-review. All of them resolve through a `node_modules` segment. The test only exercised the path helper, so reverting the fix left it green. It now drives `invocation()` with a transient bin on PATH, and fails against the previous commit. Written by Claude Opus 5; the wrong claim above was caught by adversarial review from GPT-5.6 and Grok 4.5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Written by Claude Opus 5, which also wrote #6 and the mistake in it. Caught by adversarial review that finished shortly after #6 was merged.
#6 has three problems. This fixes all three.
The comment is factually wrong. It says the cached binary "is gone the moment npx exits". npm's npx cache persists on disk; the machine this was tested on has fifteen populated hash directories under
~/.npm/_npxright now. What goes away is the PATH entry, not the binary. It is the sentence the change rests on, so it is worth correcting even though the code behaves as intended.The fix is narrower than the bug. Keying on
_npxcatches npm's cache and nothing else.pnpm dlx,yarn dlx,bunx, and setup run inside a project that already depends on human-review all produce the same failure, and all resolve through anode_modulessegment instead.isTransientBinnow checks for either.The test cannot detect the bug. It only exercises the path helper, so reverting the fix in
invocation()leaves it green. There is now a test that drivesinvocation()with a transient bin on PATH; it fails againstmainas it stands and passes with this change. 69 pass.Durable installs still keep the short form:
npm i -g,npm link, volta, nvm, asdf, pnpm and yarn globals all resolve through a bin directory with neither segment. A durable path that happens to contain one degrades to the npx form, which always works, so the failure direction is cosmetic.Simpler option if you would rather drop the optimisation. Have
invocation()always returnnpx -y human-reviewand delete the probe entirely. npx already prefers a suitable local or global install, so the short form saves nothing at runtime, and it removes any dependency on npm's cache layout. That deletes a feature you wrote deliberately, so I have not done it. Say the word and I will.🤖 Generated with Claude Code