Fail closed on malformed lockfile patch metadata

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Merges are gated by the policy job, which includes the lockfile consistency guard — the required check standing between master and ERR_PNPM_LOCKFILE_CONFIG_MISMATCH
• That failure mode is not hypothetical: per BLO-24150 it broke master three times in ~10 hours and drained the merge queue to zero by ejection
• fix(ci): verify patch content hash, not just path, in lockfile-overrides guard (BLO-24150) #1338 added patch-content verification to close the last hole, but ran it only under if (repoRoot && lockedEntry.hash) — so an absent, empty, or unparsed hash skipped verification, skipped the missing-file check nested inside it, pushed nothing, and reported green
• A falsy hash is reachable from parsing, not just hand-editing: the parser populated hash only from a line containing the literal ": ", so any change in how pnpm serialises it would silently disable content verification fleet-wide. The thing that would catch the regression is the thing that regressed
• The same shape sits one layer up — both parsers break on an unrecognised line and return a partial map, and an entry never parsed can never mismatch
• This pull request makes every unverifiable shape fail closed, and reports "could not parse" distinctly from "drift detected"
• The benefit is that a green run means "everything matched" rather than "everything I managed to parse matched"

Linked Issues or Issue Description

Refs: BLO-27241

Related PRs (both merged, neither duplicates this work):

• fix(ci): verify patch content hash, not just path, in lockfile-overrides guard (BLO-24150) #1338 — added the patch-content verification whose gating condition this PR fixes.
• ci(lockfile): add hourly master lockfile-overrides drift monitor (BLO-24150) #1340 — the hourly drift monitor that consumes this same script, so the guard's silent-pass mode was the monitor's silent-pass mode too. Its post-hoc review independently raised the partial-parse half of this fix.

No open PR overlaps: searched repo:Blockcast/paperclip lockfile (157 results reviewed) and lockfile consistency guard in:title. The only other open policy-guard PR, #1183, covers workflow block scalars — unrelated.

What Changed

• A falsy hash is now a mismatch. Content verification ran only under if (repoRoot && lockedEntry.hash); an absent, empty, or unparsed hash skipped the block, pushed nothing, and fell through to the next key.
• Patch-file existence is checked independently of the hash. The ENOENT check lived inside that same block, so a falsy hash also disabled detection of a patch file that does not exist at all.
• Unrecognised shapes throw instead of truncating. A break out of a block returns a partial Map, structurally indistinguishable from a complete parse.
• main() reports a parse failure distinctly from drift, because the fix differs: the parser needs to learn the shape pnpm now emits, rather than someone re-running pnpm install --lockfile-only.
• Six new regression cases: no hash key, hash: "", missing patch file with no hash, a hash: line not matching the ": " shape, a 3-space-indented overrides block, and a package key with no trailing :.

Verification

• node --test scripts/check-lockfile-overrides-consistency.test.mjs → 13/13 pass, run locally.
• Negative control (the issue's hard requirement): the same test file against master's script fails exactly the 6 new cases, with 7/7 pre-existing passing. A test that passed before the fix would not be exercising the gap — this issue exists because a guard reported green on a case it never evaluated.
• node ./scripts/check-lockfile-overrides-consistency.mjs → green (exit 0) against this repo's real package.json/pnpm-lock.yaml, so the stricter parser does not false-positive on the shape pnpm emits today.
• node ./scripts/check-commit-author-attribution.mjs --base origin/master --head HEAD → clean.

Risks

The tradeoff is deliberate and worth stating plainly: the guard now throws on lockfile shapes it does not recognise, and it runs on every PR plus hourly on master. If pnpm changes its serialisation, this turns fleet-wide red instead of silently green. That is the intended direction for a guard — a control that cannot verify must not report success — and the failure message names the file to update. Valid pnpm shapes are unchanged; the repo's own lockfile parses green.

No UI surface. No migration. No documentation outside the script's own comments, which are updated in place.

Model Used

Claude Opus 4.5 (claude-opus-4-5), extended thinking, with tool use / code execution — tests and the negative control were executed in a real checkout, not reasoned about.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — the script's own comments; no external docs describe this guard
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

---

Note on the force-push. The prior revision of this branch was rebuilt, for two reasons:

1. Its commits carried the shared allyblockcast[bot] App identity (the MCP write path stamps it), failing policy on the commit-attribution gate — that, not the guard tests, was the actual CI failure. The guard's own tests passed 14/14 inside that same red job, and verify/build/typecheck/general_tests were skipped as a knock-on rather than independently broken.
2. It reflowed both files onto single lines and deleted the surrounding comments — including the provenance note documenting that computePnpmPatchHash reproduces @pnpm/crypto.base32-hash and was verified against this repo's own committed hashes, and the BLO-24169 reproduction context on the tests. On a guard whose entire failure mode is subtlety, that context is load-bearing, and BLO-27241 asks for an additive change rather than a rewrite. The rebuilt branch is 229 insertions / 18 deletions where the prior one was 95 / 179; the seven pre-existing tests keep their original names and assertions.

[allyblockcast]

🔗 Paperclip issue: BLO-27241

[allyblockcast]

🔗 Paperclip issue: BLO-27241

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

End-to-end verification against the repo's real lockfile

Unit fixtures prove the branch logic; this drives the actual CLI against a copy of this repo's real package.json, pnpm-lock.yaml, and patches/, comparing master's script to this branch's on the same tampered tree. Each case is the attack the guard exists to stop.

A — patch content edited, hash: line dropped (the issue's headline case):

MASTER: pnpm-lock.yaml overrides/patchedDependencies match package.json.   exit=0
FIXED:  pnpm-lock.yaml's patchedDependencies["postgres@3.4.9"] records no non-empty hash,
        so the content of "patches/postgres@3.4.9.patch" cannot be verified             exit=1

B — declared patch file deleted from disk, entry has no hash (proves the ENOENT check is no longer gated behind hash):

MASTER: exit=0, silent
FIXED:  - ... records no non-empty hash ...
        - pnpm.patchedDependencies["postgres@3.4.9"] points to "patches/postgres@3.4.9.patch",
          which does not exist in the repository                                        exit=1

C — hash: reshaped to hash:<value> with the patch also tampered. This is the fleet-wide scenario: a pnpm serialisation change silently disabling content verification everywhere while the guard stays green.

MASTER: pnpm-lock.yaml overrides/patchedDependencies match package.json.   exit=0
FIXED:  Could not fully parse pnpm-lock.yaml, so consistency could not be verified:
          - Malformed patchedDependencies: block at pnpm-lock.yaml line 56 has no `: `
            field/value separator. Refusing to report consistency from a partially-parsed lockfile.

        This is a guard failure, not a lockfile drift. If pnpm changed how it serialises
        these blocks, update scripts/check-lockfile-overrides-consistency.mjs ...        exit=1

Note C reports as a guard failure with a line number, not as drift — the two have different fixes, and telling a reader to run pnpm install --lockfile-only when the real problem is an unparsed shape would send them in the wrong direction.

Untampered, the same script exits 0 on this repo's real tree, so the stricter parser does not false-positive on what pnpm emits today.