fix(ci): verify patch content hash, not just path, in lockfile-overrides guard (BLO-24150) - #1338
Merged
Merged
Conversation
…des guard Addresses Ally's post-merge review on #1273: the guard compared patchedDependencies path but never hash, so editing an existing patch file's content (path/package.json unchanged) landed a stale hash that only surfaced when pnpm's own --frozen-lockfile config-mismatch check caught it on master. Reproduces pnpm 9.x's own patch-hash algorithm (MD5 + RFC4648 base32, verified against this repo's committed hashes) so the check stays dependency-free.
Author
|
🔗 Paperclip issue: BLO-24150 |
1 similar comment
Author
|
🔗 Paperclip issue: BLO-24150 |
13 tasks
Author
|
@ally post-hoc review requested on this already-merged PR (BLO-26654 AC #2b). This merged to Review focus on the merged diff:
This is a change to correctness machinery (a CI guard), which is the class most likely to fail silently — the thing that would catch a regression is the thing being changed. A finding here is still actionable: we can land a follow-up fix. |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b855674
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/check-lockfile-overrides-consistency.mjs:187— patch content validation is skipped when the lockfile entry has an absent or emptyhash, so a matching path can still produce no mismatch without reading the patch file.- Treat a missing/empty hash as a lockfile mismatch (and fail closed) before or alongside reading the patch. Add a regression test with a present patch file and a
patchedDependenciesentry missinghash, asserting a non-empty mismatch. The current test only covers a non-empty stale hash atscripts/check-lockfile-overrides-consistency.test.mjs:119.
- Treat a missing/empty hash as a lockfile mismatch (and fail closed) before or alongside reading the patch. Add a regression test with a present patch file and a
Suggestions (1)
scripts/check-lockfile-overrides-consistency.test.mjs:142— add an explicit missing-file test withrepoRoot; this documents the intended fail-closed behavior for the requested unreadable/missing-file case, rather than relying on the CLI's uncaught error path for non-ENOENTfailures.
Strengths
- The implementation's MD5/RFC4648 base32 calculation matches the committed hashes for all three repository patches.
- The content-swap regression test uses a real temporary patch file and passes
repoRoot, exercising the newly added read-and-hash path. - Missing files are reported as mismatches, while other read failures terminate the CLI nonzero instead of silently passing.
Recommended Action
- Address the Important issue before treating this correctness guard as complete.
- Add the missing-file regression test and retain the existing content-swap coverage.
13 tasks
allyblockcast Bot
pushed a commit
that referenced
this pull request
Aug 15, 2026
…O-27241) The patch-content verification added by #1338 ran only under `if (repoRoot && lockedEntry.hash)`. An absent, empty, or unparsed hash skipped the whole block, pushed nothing to `mismatches`, and fell through to the next key — so the guard standing between master and ERR_PNPM_LOCKFILE_CONFIG_MISMATCH reported green on precisely the edit class #1338 was written to catch. The missing-patch-file check lived inside the same block, so a falsy hash also disabled detection of a patch file that does not exist at all. A falsy hash was reachable from parsing, not just hand-editing: `parsePatchedDependenciesBlock` populated `hash` only from a line containing the literal `": "`, and `continue`d past anything else. Any change in how pnpm serialises that line would have disabled content verification fleet-wide while the guard still reported green — the thing that would catch the regression being the thing that regressed. The same shape sat one layer up, raised on #1340's post-hoc review: both parsers `break` out of a block on an unrecognised line and return a *partial* Map, which is structurally indistinguishable from a complete parse. An entry that was never parsed can never mismatch, so a truncated or oddly-indented block compared green. Green meant "everything I managed to parse matched", not "everything matched". So: a missing hash is now itself a mismatch; patch-file existence is checked independently of the hash; and every unrecognised shape throws rather than truncating, surfaced by `main()` as a distinct guard failure ("could not parse") rather than a drift report, since the fix is different — the parser needs to learn the shape pnpm now emits. Each of the six new tests fails against the pre-fix script and passes after; the seven existing cases, including the #1338 content-swap coverage, are untouched and pass against both. Co-Authored-By: Claude <noreply@anthropic.com>
13 tasks
allyblockcast Bot
pushed a commit
that referenced
this pull request
Aug 15, 2026
…O-27241) The patch-content verification added by #1338 ran only under `if (repoRoot && lockedEntry.hash)`. An absent, empty, or unparsed hash skipped the whole block, pushed nothing to `mismatches`, and fell through to the next key — so the guard standing between master and ERR_PNPM_LOCKFILE_CONFIG_MISMATCH reported green on precisely the edit class #1338 was written to catch. The missing-patch-file check lived inside the same block, so a falsy hash also disabled detection of a patch file that does not exist at all. A falsy hash was reachable from parsing, not just hand-editing: `parsePatchedDependenciesBlock` populated `hash` only from a line containing the literal `": "`, and `continue`d past anything else. Any change in how pnpm serialises that line would have disabled content verification fleet-wide while the guard still reported green — the thing that would catch the regression being the thing that regressed. The same shape sat one layer up, raised on #1340's post-hoc review: both parsers `break` out of a block on an unrecognised line and return a *partial* Map, which is structurally indistinguishable from a complete parse. An entry that was never parsed can never mismatch, so a truncated or oddly-indented block compared green. Green meant "everything I managed to parse matched", not "everything matched". So: a missing hash is now itself a mismatch; patch-file existence is checked independently of the hash; and every unrecognised shape throws rather than truncating, surfaced by `main()` as a distinct guard failure ("could not parse") rather than a drift report, since the fix is different — the parser needs to learn the shape pnpm now emits. Each of the six new tests fails against the pre-fix script and passes after; the seven existing cases, including the #1338 content-swap coverage, are untouched and pass against both. Co-Authored-By: Claude <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.
Thinking Path
Linked Issues or Issue Description
scripts/check-lockfile-overrides-consistency.mjs:168, "Patch-only changes are still allowed to leave the lockfile stale")d36a72e) by the time this review landed, so this ships as a follow-up rather than a commit onto that branchWhat Changed
scripts/check-lockfile-overrides-consistency.mjs: addedcomputePnpmPatchHash(), a from-scratch reproduction of@pnpm/crypto.base32-hash'screateBase32HashFromFile(the function pnpm 9.x actually uses forpatchedDependencies[*].hash) — normalize CRLF→LF, MD5, RFC4648 base32 encode, strip padding, lowercase.findLockfileOverrideMismatches()now accepts an optional{ repoRoot }and, when a patch's declared path matches the lockfile's recorded path, reads the real patch file and compares its computed hash against the lockfile's recorded hash — flagging both a stale hash and a path that points at a nonexistent file.main()now passes{ repoRoot }so the CLI entrypoint gets full hash verification; fixture-based unit tests that omitrepoRootkeep their prior path-only behavior unaffected.repoRootisn't supplied (protects the pre-existing fixture tests, which use placeholder hashes).Verification
node --test scripts/check-lockfile-overrides-consistency.test.mjs— 7/7 passing (5 pre-existing + 2 new).patches/brace-expansion@5.0.9.patch, left everything else untouched, and confirmed the guard now flags it:computePnpmPatchHash()against pnpm's actual published algorithm: fetched@pnpm/crypto.base32-hash@3.0.1(the package pnpm 9.15.4, this repo's pinnedpackageManager, depends on for this) from the npm registry, read its compiled source, and confirmed my reimplementation produces byte-identical output against all three patches already committed in this repo'spatches/directory and their existing lockfile hashes.node scripts/check-lockfile-overrides-consistency.mjspasses clean against current master.Risks
require/importbeyond Node built-ins), no CI wiring changes needed —.github/workflows/pr.ymlalready calls this script unconditionally from fix(ci): fail pre-merge when pnpm overrides drift from the lockfile (BLO-24169) #1273.computePnpmPatchHash()is ever wrong for some patch-file edge case (e.g. an unusual encoding), it would produce a false-positive failure rather than a false negative — annoying but safe, and the "repo's own files are consistent" test would need to already be failing for that to reach master undetected.Model Used
Claude, Sonnet 5 (
claude-sonnet-5[1m]), extended context window, agentic tool use (file edits, shell, GitHub CLI, GitHub code search against pnpm's own source and npm registry tarballs to verify the hash algorithm) — Paperclip PlatformSREEngineer agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template