ci(deps): guard pnpm-workspace floor-pin upper bounds (LAB-1513) - #101
ci(deps): guard pnpm-workspace floor-pin upper bounds (LAB-1513)#10127Bslash6 wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
WalkthroughThe change adds pre-install CI validation for bounded dependency floor-pins. It also adds Renovate approval rules and documents the required override format, review process, and security exceptions. ChangesDependency floor-pin governance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Renovate
participant WorkspaceOverrides
participant CISecurityJob
participant DependencyInstallation
Renovate->>WorkspaceOverrides: Propose override update
CISecurityJob->>WorkspaceOverrides: Read and parse overrides
CISecurityJob->>CISecurityJob: Compare overrides with expected floor-pins
CISecurityJob->>DependencyInstallation: Allow installation after validation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
An `overrides` upper bound in pnpm-workspace.yaml is a decision (LAB-1020),
and four are load-bearing — widening `uuid` past `<12` alone breaks Node
<22.12, which `engines` allows and CI on 22-latest would never catch.
Renovate proposes exactly that: a 43.288.0 dry-run against this tree has
cross-major widenings queued on all four bounded pins right now
(protobufjs <8->9, brace-expansion@2 <3->6, js-yaml <5->6, uuid <12->15).
Until now the only defence was prose asking reviewers to look harder.
Two layers, because neither is sufficient alone.
renovate.json gates cross-major updates to these entries behind
dependency-dashboard approval. Matching on depType
(`pnpm-workspace.overrides`) rather than matchFileNames scopes it to the
override entries themselves, so a pnpm catalog added to the same file later
is not silently swept in, and same-major floor moves still flow freely.
That is prevention: no branch is ever created.
But it covers routine majors only. A GitHub vulnerability-alert update
carries `force: {...vulnerabilityAlerts}`, applied last by mergeChildConfig,
which resets `dependencyDashboardApproval` to false — and, with this repo's
preset setting `vulnerabilityAlerts.enabled`, `enabled` back to true —
regardless of repo config; `force` is globalOnly so it cannot be countered
from renovate.json at all. Verified by executing Renovate's own
applyPackageRules with the alert-rule shape vulnerability.ts builds. PR #94
came through that path, so the config rule alone would not have stopped the
PR that prompted this work.
So the CI step is what actually closes it. It declares the bounded pins and
their upper-bound majors and requires the file to match that set exactly.
An earlier revision derived the rule instead (`<N` == floor-major + 1); the
panel showed that fails open per entry — reformat one pin and it drops out
of the checked set while the others keep CI green. A set comparison fails
closed on anything it cannot parse, which also closes bound deletion and a
floor moved into a new major. Runs ahead of `pnpm install` so no install
script can rewrite the file between check and merge.
A cross-major security fix that is genuinely required stays possible and
discoverable: CI goes red naming the pin, and the bound comes off in a
hand-written commit that says why.
5018611 to
4404750
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This comment has been minimized.
This comment has been minimized.
…1513) The `found=$(...)` pipeline runs under `set -o pipefail`. Its `grep -v` stage exits 1 whenever it selects no lines — which is precisely the "file shape moved" case the guard exists to catch (overrides header renamed, block entirely comments, file unreadable). Under pipefail plus `set -e` that aborted the step at the grep stage, before the set comparison and its `::error::` remediation text could run, so the guard failed closed but silently: a bare exit-1 with nothing telling the reader which pin moved or what to do about it. `|| true` on the substitution, mirroring the sibling "Verify brace-expansion advisory-ignore invariant" step, which already solves this and documents why. It cannot mask real drift: a broken pipeline yields empty or partial output, which never equals `expected`, so the explicit branch still fails — now with its explanation intact. Chosen over folding the comment filter into the awk program, which would drop the grep stage but leave a missing/unreadable pnpm-workspace.yaml aborting bare — the same contract violation by another door. Mutation matrix re-run against the step body extracted from the YAML, 15/15: the four shape-moved cases now emit the diagnostic (3 did not before), and every drift case still fails while every legitimate edit still passes. Reported by Kody, PR #101 review 2026-08-05.
efd0d5d
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@kody start-review |
|
@coderabbitai review |
|
Summary
This PR adds a CI-enforced guard to prevent bounded floor-pins in
pnpm-workspace.yamlfrom being silently widened across major versions, closing a gap where vulnerability-alert-driven dependency updates could bypass existing review controls.Background
The repository floor-pins vulnerable transitive dependencies in
pnpm-workspace.yamloverrides, bounded to a specific major (e.g.'>=2.1.3 <3'). Renovate takes ownership of these entries and can propose widening the upper bound across majors. PR #94 demonstrated this by wideningbrace-expansion@2from<3to<6. Critically, GitHub vulnerability-alert updates carry aforceflag that overrides repo-level dependency-dashboard approval, so cross-major widening could get through without human sign-off.Changes
CI workflow (
.github/workflows/ci.yml)pnpm installso no install script can rewrite the file before the check reads it.brace-expansion@2,js-yaml,protobufjs,uuid).overridesblock and matches only plain>=floor <boundvalues. Any unrecognized shape (a||union, a missing bound, reindented YAML) fails the set comparison — the check fails closed rather than silently skipping what it cannot parse.Documentation (
CONTRIBUTING.md)renovate.jsondependency-dashboard approval (covers routine majors only) plus the CI floor-pin check (holds against vulnerability-alert PRs and hand-written diffs).forceoverride on vulnerability alerts) and how a genuinely necessary cross-major security fix stays possible — CI goes red as a discovery channel, not a wall.pnpm-workspace.yamloverridesto note that the bounds are now enforced by CI, not merely documented, and describes the process for intentionally crossing a bound.Impact
Crossing a floor-pin upper bound now requires a hand-written commit that removes the bound and updates the expected list in the CI step, ensuring every cross-major dependency change reflects a deliberate human decision.
Summary
This PR adds a CI enforcement layer to prevent floor-pinned dependency overrides in
pnpm-workspace.yamlfrom being widened across major versions without explicit human review.What Changed
New CI guard (
.github/workflows/ci.yml)pnpm installso no install script can rewrite the file before verification runs.brace-expansion@2,js-yaml,protobufjs,uuid) with their upper-bound majors, then parses theoverridesblock ofpnpm-workspace.yamland fails the job unless the declared set matches exactly.>=floor <boundshape. Any unrecognized shape (a||union, a missing bound, a moved floor, or reindented YAML) fails closed rather than being silently skipped.Documentation updates (
CONTRIBUTING.md,pnpm-workspace.yaml)renovate.json's dashboard-approval gate (covers routine majors) and the new CI check (covers vulnerability-alert PRs and hand-written diffs).forceconfig that overrides repo settings and bypasses the approval gate — the path that PR chore(deps): update dependency brace-expansion@2 to v5 [security] - autoclosed #94 used to widenbrace-expansion@2from<3to<6.Why
The existing Renovate approval gate does not cover vulnerability-alert-driven updates, which can silently widen an upper bound across a major version (as PR #94 demonstrated). This adds an enforcement layer that treats each upper bound as a deliberate constraint, ensuring any change to a bounded floor-pin requires an explicit human decision.
Description
This PR fixes a potential silent failure in the CI guard that validates bounded floor-pin overrides in
pnpm-workspace.yaml.Problem
The
foundvariable is populated by a shell pipeline that includes agrep -vcommand. When the overrides block shape changes and no matching lines are found,grep -vexits with status code 1. Underpipefail, this non-zero exit would abort the step immediately with no explanatory message — precisely in the "shape-moved" scenario this guard is meant to detect.Change
Added
|| trueto the end of the pipeline (consistent with the sibling step referenced in the comments). This ensures that an empty result flows through to the explicit comparison branch, so the guard can produce its intended, self-explaining failure message rather than silently aborting.Impact
expectedvalue, so the comparison still fails as intended.|| trueis safe and necessary here.Summary by CodeRabbit
Security
Documentation
Chores