Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/claude-review-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,8 @@ The review covers one immutable comparison pair: the merge base commit and the h

## Dependency delta

- **Supported files:** `package.json`, `package-lock.json` (lockfile v2 and v3), `yarn.lock` (v1 and berry), `pnpm-lock.yaml` (v6 and later), `pyproject.toml`, `poetry.lock`, `uv.lock`, `requirements*.txt`. The parser for each file family has fixtures for each supported version.
- **Supported versions:** `package-lock.json` lockfile v2 and v3; `yarn.lock` v1 and berry metadata versions 4 to 8; `pnpm-lock.yaml` 6.0, 6.1 and 9.0; `poetry.lock` 1.1, 2.0 and 2.1; `uv.lock` version 1. Any other version is unknown. Pip `-r` and `-c` includes are not followed, and an unknown pip option is a parse failure. `npm-shrinkwrap.json` is not parsed; it is an unsupported dependency file.
- **Supported files:** `package.json`, `package-lock.json` (lockfile v2 and v3), `yarn.lock` (v1 and berry), `pnpm-lock.yaml` (v6 and later, one YAML document or the pnpm 12 pair of env and main documents, K-16), `pyproject.toml`, `poetry.lock`, `uv.lock`, `requirements*.txt`. The parser for each file family has fixtures for each supported version.
- **Supported versions:** `package-lock.json` lockfile v2 and v3; `yarn.lock` v1 and berry metadata versions 4 to 8; `pnpm-lock.yaml` 6.0, 6.1 and 9.0, in each document of a two-document file; `poetry.lock` 1.1, 2.0 and 2.1; `uv.lock` version 1. Any other version is unknown. Pip `-r` and `-c` includes are not followed, and an unknown pip option is a parse failure. `npm-shrinkwrap.json` is not parsed; it is an unsupported dependency file.
- **Delta:** parse each changed supported file at the merge base and at the head (both from blobs by SHA) and compare. For each added or changed package: name, old and new version, resolved source (normalized), registry publish time of the new version, `hasInstallScript` (npm lockfile v2 and v3), and for npm direct deps the install lifecycle scripts from the registry manifest (`preinstall`, `install`, `postinstall`, `prepare`, `preprepare`, `postprepare`, `prepublish`).
- **Alternatives:** a `pyproject.toml` dependency can list alternatives, split by markers: a list under `[tool.uv.sources]`, or a list of Poetry constraints. The code reads every alternative, whatever its marker, as its own package with its own version and source, so each one gets the source check and the 72-hour rule. A uv alternative with no known source key, or an empty Poetry list, is a parse failure.
- **Registry lookups:** only fixed routes built from validated names: `https://registry.npmjs.org/<encoded-name>` and `https://pypi.org/pypi/<name>/json`. No redirects to other hosts, a response byte cap, and a timeout. Never fetch a URL from the PR, including a lockfile `resolved` field. The code refuses every redirect. Caps: 16 MiB for one dependency file, 32 MiB for an npm response (the full packument, for per-version publish times), 16 MiB for a PyPI response. Each lookup has a 20-second timeout and at most 2 retries, for 429, 5xx and network errors only.
Expand Down Expand Up @@ -985,3 +985,4 @@ P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line,
- P3-5. Summary comment redesigned for fast human judgment (owner and coordinator, 2026-10-27). The comment now opens with a result heading and one colored callout, then a findings table, a checks strip and a provenance line, with the waiver and the full record collapsed ("Summary comment" under "Publishing (code)"). The owner chose the Fable design with these rulings: every fail uses the red `[!CAUTION]` callout and the red heading, waivable or not (a waivable fail on a fake check was misread as a mild warning under `[!WARNING]`, so `[!WARNING]` is not used for a fail); the waiver line stays in the collapsed Waiver block; model summaries and token counts stay in the collapsed record; duplicate findings merge only at the same file, side, line and gate effect; a medium finding in a blocking category reads "Does not block"; the visible line shows the 7-character head SHA and the record shows the full SHAs; emoji are allowed, each next to its word. Machine markers, the override line format, the size limit and the overflow path are unchanged.
- P3-6. Account X results and Sonnet release (owner and coordinator, 2026-10-28). Account X is `irvingpop2`, a separate GitHub account outside the org that the owner made as a test identity; it became an outside collaborator when it got repo access. Steps ran from an isolated container. V8 b pass (PR 38): the central run started on a first-time-contributor fork PR with no approval click, and marking the draft ready started no new run. V8 a pass (PR 39): from a first-time fork, GitHub holds the fake `pull_request` job (`action_required`), so it cannot start before the central run; after a maintainer approved only that run it passed, and the box stayed BLOCKED and followed the central run; the fake-first case stays covered by PR 36. V23 pass (PR 40): a same-path `claude-review.yml` in the fork PR did not replace the central run, which failed on the bidi hard block; the fork run stayed `action_required` and never ran, so the running case is covered by PR 35. V6 f as a Write user: CONFIRMED residual (PR 41); deleting the newer run reopened the box on an older pass, and it stays in the residuals the owner must accept before org-wide enforcement. A Write user's plain merge of a failed PR was refused; this does not test the bypass, and the `--admin` attempt as Write was not run. V25 pass for Maintain and Admin, Write bypass NOT RUN: with the TR bypass list, a Maintain user (PR 42) and an Admin user (PR 43) merged a failed PR; GitHub records rule suite result `bypass` with the user ID as actor and no role ID, and the org rule-suites endpoint returns 403 on this plan, so use the repo endpoint; a direct push to main by Maintain is refused (GH013). The TR bypass list is a test-only setting and goes back to empty after phase 3 (owner's choice pending). Process: the owner ran the bypass merges and the direct push by hand, because the automation's safety classifier blocks bypass merges; the evidence comes from the GitHub API. Sonnet release: release evals 13 of 13 at M3 ([run 40592861233](https://github.com/OperationCode/.github/actions/runs/40592861233)); the live run [40593729924](https://github.com/OperationCode/claude-review-test/actions/runs/40593729924) passed with `claude-sonnet-5-5`. Console read-back: the owner read back R1 at 590a387 and R2 at 1cefe42 on 2026-10-27, before the M3 edit; R1 was then moved to M3; the service account and the workspace were not read back. V26 a is done for `job_workflow_ref`. NOT RUN, stays open: the Write `--admin` bypass attempt and V26 c deny at M2 before the R2 edit. Web UI "Re-run all jobs": pass (run 40628870689, PR 43, 2026-10-28): the click made attempt 2 of the same run ID, which re-read live state and updated the one summary comment; the merge box was not observed because the PR was already merged.
- P3-7. Release M4, the comment redesign (owner and coordinator, 2026-10-28). M4 is `9e2795b97b22869b9e5c7a777e71a2afa8366eeb` on main (code from PR 12 `a410ddd`, docs PR 13). There is no separate candidate branch: the merge SHA was validated directly. Unit tests: `test.yml` at M4, [run 40710532524](https://github.com/OperationCode/.github/actions/runs/40710532524), success. Release evals: `eval.yml` at M4, [run 40713029193](https://github.com/OperationCode/.github/actions/runs/40713029193), 13 of 13 matched, success; the coordinator approved the run under the owner's delegation through phase 4. Console: the owner edited R1 `claude-review-prod-r1` (`fdrl_01N2ZktTiVKqUvvXwywBFfys`) so that `job_workflow_ref` is `OperationCode/.github/.github/workflows/claude-review.yml@9e2795b97b22869b9e5c7a777e71a2afa8366eeb`. The owner reported "rule updated" and shared a screenshot of the rules list; the rule detail value was not read back. TR (org ruleset 25835887) moved from M3 `2c63f49b461feac74c2c70c4f86ca90040df6a0a` to M4; read back: `repository_id` 1432342572, path `.github/workflows/claude-review.yml`, `sha` `9e2795b`, no `ref`, bypass list empty, enforcement active. Live checks in claude-review-test: PR 28 reopened, [run 40713506554](https://github.com/OperationCode/claude-review-test/actions/runs/40713506554) at M4, success, all four jobs `ok`, summary comment in the new layout (green PASS heading, TIP callout, checks strip). PR 40 reopened, [run 40713856249](https://github.com/OperationCode/claude-review-test/actions/runs/40713856249), failure, red FAIL heading, CAUTION callout, the hard block row (bidi) first, merged duplicate rows with "Found by" listing both passes. Both PRs were closed again. Console rules in the owner's screenshot: `claude-review-p3-r2`, `claude-review-prod-r1`, `eval-federation-rule`, `claude-reviewer-test`; the coordinator advised archiving `claude-review-p3-r2`. Deviation 1: no new federation rule for this release; R1 was edited in place, because the merged code names R1 (same as the phase 3 release, where R1 moved from S1 to M3). Cost: M3 can no longer get a token, so a rollback to M3 needs another Console edit. Deviation 2: the denied exchange at M4 before the Console edit (release step 5a) was NOT RUN, because the owner made the edit before the check. M4 is the current production SHA and the current pin.
- K-16. pnpm 12 lockfiles with two YAML documents (ruling on front-end PR 2027, 2026-10-29). pnpm 12 writes `pnpm-lock.yaml` as two YAML documents: first the env lockfile (one importer `.` with `configDependencies` and `packageManagerDependencies`, and the `packages` and `snapshots` of pnpm itself), then the main lockfile. The parser read one document only, so every front-end PR that changed the lockfile got the false hard block "incomplete dependency data" (does not parse), at the head and at the merge base. The parser now reads every document with the same safe loader that refuses duplicate keys. The loader expands YAML merge keys before that check, so a merged key that repeats another key in the same mapping is a parse failure too, in every YAML lockfile. It accepts one document, as before, or exactly two in that order. Each document must have a supported `lockfileVersion`. The first may hold only `lockfileVersion`, `importers`, `packages` and `snapshots`, and its only importer is `.` with only the two env keys. An env `packages` entry with its own `name` or `version` must match its key, and each env dependency must be a package that the parser emits from that document, so no env dependency can skip a check under another identity. The second may hold only the top-level keys of pnpm 12's main lockfile: `lockfileVersion`, `settings`, `catalogs`, `overrides`, `packageExtensionsChecksum`, `pnpmfileChecksum`, `ignoredOptionalDependencies`, `patchedDependencies`, `importers`, `packages`, `snapshots` and `time`. A one-document file keeps the old check, which ignores other keys. Packages of both documents go through every dependency check; pnpm and its platform binaries are dependencies too, and an env dependency counts as direct. Any other document count, an empty document, another env or main shape, or a bare document after a `...` end marker with no `---` is still the hard block. A malformed two-document merge-base file counts as empty, as in K-10. The size gate is unchanged.
84 changes: 78 additions & 6 deletions review/src/oc_review/deps.py
Original file line number Diff line number Diff line change
Expand Up @@ -762,13 +762,17 @@ def _parse_yarn_berry(text: str) -> _Result:


class _UniqueKeyLoader(_SafeLoader): # type: ignore[misc, valid-type]
"""Safe loader that refuses a duplicate mapping key (parser differential)."""
"""Safe loader that refuses a duplicate mapping key (parser differential).

Merge keys are expanded first, so a merged key that repeats another key
is refused too: no parser has to choose which value wins.
"""

def construct_mapping(self, node: Any, deep: bool = False) -> dict[Any, Any]:
if isinstance(node, yaml.MappingNode):
self.flatten_mapping(node)
seen: set[Any] = set()
for key_node, _value in node.value:
if key_node.tag == "tag:yaml.org,2002:merge":
continue
key = self.construct_object(key_node, deep=True)
if key in seen:
raise DepsError(f"duplicate YAML key {str(key)[:60]!r}")
Expand Down Expand Up @@ -801,11 +805,79 @@ def _pnpm_source(resolution: dict[str, Any], key: str, name: str, version: str)
return Source(kind="unknown", normalized="")


# pnpm 12 writes the env lockfile (the package manager and config
# dependencies) as a first YAML document before the main lockfile.
_PNPM_ENV_KEYS = frozenset({"lockfileVersion", "importers", "packages", "snapshots"})
_PNPM_ENV_IMPORTER_KEYS = frozenset({"configDependencies", "packageManagerDependencies"})
# The top-level fields of pnpm 12's main lockfile (`Lockfile` in its
# lockfile crate). The single-document form keeps the old, open check.
_PNPM_MAIN_KEYS = frozenset({
"lockfileVersion", "settings", "catalogs", "overrides", "packageExtensionsChecksum", "pnpmfileChecksum",
"ignoredOptionalDependencies", "patchedDependencies", "importers", "packages", "snapshots", "time",
})


def _parse_pnpm_lock(text: str) -> _Result:
doc = _as_dict(yaml.load(text, Loader=_UniqueKeyLoader), "pnpm-lock.yaml")
"""One document, or two: the env lockfile, then the main lockfile.

Any other document count, an empty document or a document of an
unexpected shape is a parse failure. Packages of both documents are
returned, so the env document's entries (pnpm itself) get every check.
"""
docs: list[Any] = []
for doc in yaml.load_all(text, Loader=_UniqueKeyLoader):
docs.append(doc)
if len(docs) > 2:
raise DepsError("pnpm-lock.yaml has more than two YAML documents")
if len(docs) == 1:
return _parse_pnpm_doc(_as_dict(docs[0], "pnpm-lock.yaml"), "pnpm-lock.yaml")
if len(docs) != 2:
raise DepsError("pnpm-lock.yaml has no YAML document")
env = _as_dict(docs[0], "pnpm-lock.yaml env document")
root = _check_pnpm_env_doc(env)
_env_version, env_packages, _ = _parse_pnpm_doc(env, "pnpm-lock.yaml env document")
main = _as_dict(docs[1], "pnpm-lock.yaml main document")
if set(main) - _PNPM_MAIN_KEYS:
raise DepsError(f"pnpm-lock.yaml main document has an unexpected shape (keys {sorted(map(str, main))})")
version, packages, _ = _parse_pnpm_doc(main, "pnpm-lock.yaml")
# Every env dependency must be a package the walker emits, so none
# escapes a check under another identity.
emitted = {(p.name, p.version) for p in env_packages}
for section in _PNPM_ENV_IMPORTER_KEYS:
for name, dep in _as_dict({} if root.get(section) is None else root[section], section).items():
dep_version = _as_str(_as_dict(dep, f"{section}.{name}").get("version"), f"{section}.{name} version")
if (name, dep_version.split("(", 1)[0]) not in emitted:
raise DepsError(f"pnpm-lock.yaml env dependency {name}@{dep_version} has no packages entry")
return version, env_packages + packages, []


def _check_pnpm_env_doc(env: dict[str, Any]) -> dict[str, Any]:
"""The env document holds one importer `.` with only env dependencies,
and its package keys name the identity: an entry's own `name` or
`version` must not differ from its key. Returns the importer `.`.
"""
extra = set(env) - _PNPM_ENV_KEYS
if extra or "lockfileVersion" not in env or "importers" not in env:
raise DepsError(f"pnpm-lock.yaml env document has an unexpected shape (keys {sorted(map(str, env))})")
importers = _as_dict(env["importers"], "env importers")
if list(importers) != ["."]:
raise DepsError("pnpm-lock.yaml env document must have exactly the importer '.'")
root = _as_dict(importers["."], "env importer '.'")
if set(root) - _PNPM_ENV_IMPORTER_KEYS:
raise DepsError("pnpm-lock.yaml env importer '.' has an unexpected key")
entries = _as_dict({} if env.get("packages") is None else env["packages"], "env packages")
for key, entry in entries.items():
name, version = _pnpm_split_key(_as_str(key, "env package key"))
entry = _as_dict(entry, f"env packages[{key!r}]")
if entry.get("name", name) != name or str(entry.get("version", version)) != version:
raise DepsError(f"pnpm-lock.yaml env package {str(key)[:60]!r} names another package")
return root


def _parse_pnpm_doc(doc: dict[str, Any], what: str) -> _Result:
version = str(doc.get("lockfileVersion"))
if version not in SUPPORTED_PNPM_LOCK_VERSIONS:
raise UnsupportedVersionError(f"pnpm-lock.yaml lockfileVersion {version!r} is not supported")
raise UnsupportedVersionError(f"{what} lockfileVersion {version!r} is not supported")
entries = _as_dict({} if doc.get("packages") is None else doc["packages"], "packages")
snapshots = _as_dict({} if doc.get("snapshots") is None else doc["snapshots"], "snapshots")
build_names: set[tuple[str, str]] = set()
Expand Down Expand Up @@ -846,7 +918,7 @@ def _pnpm_direct_names(doc: dict[str, Any]) -> set[str]:
sections.extend(_as_dict(v, "importer") for v in _as_dict(importers, "importers").values())
names: set[str] = set()
for section in sections:
for dep_key in ("dependencies", "devDependencies", "optionalDependencies"):
for dep_key in ("dependencies", "devDependencies", "optionalDependencies", *_PNPM_ENV_IMPORTER_KEYS):
deps = section.get(dep_key)
if deps is not None:
names.update(_as_dict(deps, dep_key))
Expand Down
87 changes: 87 additions & 0 deletions review/tests/unit/fixtures/deps/pnpm-lock.v9-env.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
---
lockfileVersion: '9.0'

importers:

.:
configDependencies: {}
packageManagerDependencies:
pnpm:
specifier: 12.15.0
version: 12.15.0

packages:

'@pnpm/exe.darwin-arm64@12.15.0':
resolution: {integrity: sha512-N0ZykiDWCvqSBJvt71j7cSoBR993FqvYZFtmpCtqvW3pguiI+NAgraLPA5OTwu5mV+/d8hYpVcPktJA3xFDLtA==}
cpu: [arm64]
os: [darwin]

'@pnpm/exe.linux-x64@12.15.0':
resolution: {integrity: sha512-yc+QlZR+9+nr29J4fRzhMoapgxTBDndq/+peyoq1XUufnQFnfbXUMFudDIKzHi545linGqmOmXfn0NoufNgcOg==}
cpu: [x64]
os: [linux]
libc: [glibc]

pnpm@12.15.0:
resolution: {integrity: sha512-s5UdbW/0qfE9x89WzSjSuOs328ycaOUuHhGRhz6p+CoBcoerULqtxczElPA2xnwOR+sTYIwvo29HcRth6hKOYQ==}
engines: {node: '>=18'}
hasBin: true

snapshots:

'@pnpm/exe.darwin-arm64@12.15.0':
optional: true

'@pnpm/exe.linux-x64@12.15.0':
optional: true

pnpm@12.15.0:
optionalDependencies:
'@pnpm/exe.darwin-arm64': 12.15.0
'@pnpm/exe.linux-x64': 12.15.0

---
lockfileVersion: '9.0'

settings:
autoInstallPeers: true
excludeLinksFromLockfile: false

overrides:
postcss: ^8.5.18

importers:

.:
dependencies:
'@next/bundle-analyzer':
specifier: ^16.4.3
version: 16.4.3
devDependencies:
esbuild:
specifier: ^0.28.2
version: 0.28.2

packages:

'@next/bundle-analyzer@16.4.3':
resolution: {integrity: sha512-FGcLziT2rqvGZTxZUjnCzwjCANnigN9vrbCm+1hQtzXbbtc1bXBTRmkXohVqyB/ReF5bOjpM2bNJQhz0PfLk9Q==}

esbuild@0.28.2:
resolution: {integrity: sha512-HKVLS8dvII+xoKW9kmqxbRKrnWEXfJJr/FZhhJmiqIB0e053QNYFqOBouTMO/k5sID4MvCiUCvv8b9M4h32wIA==}
engines: {node: '>=18'}
hasBin: true

ms@2.1.3:
resolution: {integrity: sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==}

snapshots:

'@next/bundle-analyzer@16.4.3':
dependencies:
ms: 2.1.3

esbuild@0.28.2: {}

ms@2.1.3: {}
Loading
Loading