From 97627a9f24abb2f96a51da9c4587bf11cd832563 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:14:56 +0000 Subject: [PATCH] fix(review): treat npm project's stray pnpm-lock.yaml as npm, not fail-closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The central coverage-evidence job materializes base JavaScript locks with scripts/ci/materialize_base_javascript_packages.py. It hard-fails (exit 1) for ANY base pnpm-lock.yaml whose sibling package.json lacks an exact `packageManager: pnpm@X.Y.Z`. That aborts coverage evidence — before any test runs — for npm-managed repositories that merely carry a vestigial second pnpm-lock.yaml alongside their canonical package-lock.json (observed live on pg-erd-cloud#635 and scopeweave PRs). This is a self-perpetuating pipeline defect: a base-fix PR that removes the stray lockfile cannot pass its own coverage-evidence, because the materializer reads the still-unfixed BASE tree, so the class can only be unblocked by an admin bypass-merge. Fix: when a base pnpm-lock.yaml directory has a sibling package-lock.json and no exact pnpm packageManager pin, skip pnpm materialization (the downstream npm `npm ci` path, selected from the head worktree's package-lock.json, owns the install) instead of raising. A genuine pnpm-only project — a pnpm-lock.yaml with NO sibling package-lock.json — still must pin an exact pnpm packageManager, so the supply-chain guard is preserved unchanged; nothing is weakened. Repos in the two-lockfile state can now pass coverage-evidence (paired with removing the stray head lock), which self-heals the bootstrap. Verified locally: tests/test_materialize_base_javascript_packages.py 16 passed, including the new test_skips_npm_project_with_vestigial_pnpm_lock covering the skip branch; the existing no-sibling case (test_rejects_mutable_or_non_pnpm_ package_manager) keeps covering the fail-closed branch (100% retained). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SEAGtwNR96cia2djq7XFCo --- .../materialize_base_javascript_packages.py | 17 ++++++++++- ...st_materialize_base_javascript_packages.py | 28 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/scripts/ci/materialize_base_javascript_packages.py b/scripts/ci/materialize_base_javascript_packages.py index 7defba36..2e394ddb 100644 --- a/scripts/ci/materialize_base_javascript_packages.py +++ b/scripts/ci/materialize_base_javascript_packages.py @@ -1,5 +1,12 @@ #!/usr/bin/env python3 -"""Materialize pnpm locks from a validated pull-request base commit.""" +"""Materialize pnpm locks from a validated pull-request base commit. + +A ``pnpm-lock.yaml`` whose sibling ``package.json`` does not pin an exact pnpm +``packageManager`` is treated as a genuine pnpm project only when no sibling +``package-lock.json`` exists; otherwise it is a vestigial second lockfile in an +npm-managed project and is skipped so the downstream npm install path handles it +instead of failing coverage evidence. +""" from __future__ import annotations @@ -97,6 +104,14 @@ def base_pnpm_projects( if not isinstance(package_manager, str) or not PNPM_SPEC_RE.fullmatch( package_manager ): + if str(project_root / "package-lock.json") in regular_paths: + # A sibling package-lock.json means npm owns this project and + # the pnpm-lock.yaml is a vestigial second lockfile. Skip pnpm + # materialization so the downstream npm (package-lock.json) + # install path handles it, instead of failing the whole + # coverage-evidence job. A genuine pnpm-only project (no sibling + # package-lock.json) still must pin an exact pnpm packageManager. + continue raise ValueError( f"trusted base package manifest {package_path} must declare an exact pnpm packageManager version" ) diff --git a/tests/test_materialize_base_javascript_packages.py b/tests/test_materialize_base_javascript_packages.py index 2534f906..039673a6 100644 --- a/tests/test_materialize_base_javascript_packages.py +++ b/tests/test_materialize_base_javascript_packages.py @@ -208,6 +208,34 @@ def fake_git(_repo_root: Path, _command: str, object_spec: str) -> bytes: materializer.base_pnpm_projects(tmp_path, "a" * 40) +def test_skips_npm_project_with_vestigial_pnpm_lock( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """An npm project's stray pnpm-lock.yaml is skipped, not fail-closed. + + A base tree with a ``pnpm-lock.yaml`` plus a sibling ``package-lock.json`` + and no exact pnpm ``packageManager`` is npm-managed, so pnpm materialization + is skipped (the downstream npm install path owns it) rather than failing the + whole coverage-evidence job. + """ + regular_paths = { + "frontend/package.json", + "frontend/pnpm-lock.yaml", + "frontend/package-lock.json", + } + monkeypatch.setattr( + materializer, "_regular_base_paths", lambda *_args: regular_paths + ) + + def fake_git(_repo_root: Path, _command: str, object_spec: str) -> bytes: + if object_spec.endswith(":frontend/package.json"): + return b"{}" + raise AssertionError(f"unexpected git object: {object_spec}") + + monkeypatch.setattr(materializer, "_git", fake_git) + assert materializer.base_pnpm_projects(tmp_path, "a" * 40) == [] + + def test_rejects_mutable_or_non_pnpm_package_manager(tmp_path: Path) -> None: """Only an exact pnpm runner specification may populate the trusted store.""" repo, base_sha = fixture_repo(tmp_path)