Skip to content

Fix dependency Version Check false positives#154

Open
gtbuchanan wants to merge 1 commit into
mainfrom
fix/version-check-transitive-deps
Open

Fix dependency Version Check false positives#154
gtbuchanan wants to merge 1 commit into
mainfrom
fix/version-check-transitive-deps

Conversation

@gtbuchanan

@gtbuchanan gtbuchanan commented Jun 8, 2026

Copy link
Copy Markdown
Owner

Problem

The dependency Version Check checks every dependency GitHub's dep-graph compare marks added, which produced two classes of false failure (both seen on #152, a one-line prettier-plugin-multiline-arrays bump that failed on five unrelated deps):

  • Transitive churn. The compare collapses npm/pnpm/yarn to the lockfile, so a single-package update surfaces all of that package's transitive deps as added. Their versions are dictated by our direct deps, not chosen by us.
  • Release-age quarantine. "Latest" meant the absolute latest, so a dep failed the moment upstream cut a release — even though our policy (Renovate's minimumReleaseAge) deliberately holds adoption for 3 days.

Fix

  • Only check deps the PR actually declares in a changed non-lockfile manifest; lockfile-only (transitive) entries are skipped.
  • "Latest" now means the newest release that has cleared the quarantine window, and a pin is stale only when it's behind that — being at or ahead is fine. New minimum-release-age-days input (default 3, set 0 to disable) mirrors Renovate's minimumReleaseAge.

Verified locally against #152's base/head SHAs: all transitive deps are skipped and prettier-plugin-multiline-arrays@4.1.9 validates against its eligible latest → exit 0. actionlint and eslint pass.

Known gap

A version duplicated into a new manifest with no corresponding removal (e.g. terraform module deps repeated across modules) still reads as new, since the diff alone can't see the pre-existing copy. Documented in AGENTS.md.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The dependency-review workflow is enhanced to parameterize the staleness quarantine window via a new minimum-release-age-days input (default 3 days), filter checked dependencies to only manifest-declared newly-added entries, resolve latest versions within the cutoff date across GitHub actions and package ecosystems, and evaluate staleness using wildcard-constrained and exact pin semantics.

Changes

Dependency Version Quarantine with Minimum Release Age

Layer / File(s) Summary
Workflow input and environment setup
.github/workflows/dependency-review.yml
New workflow input minimum-release-age-days (type number, default 3) is declared and passed to the version-check job as MIN_RELEASE_AGE_DAYS environment variable.
Quarantine cutoff and dependency filtering
.github/workflows/dependency-review.yml
Cutoff timestamp is computed from the quarantine window. Dependency-graph processing gathers added entries while excluding lockfiles, then the staleness check loop is restricted to dependencies whose names appear in manifest-anchored added lines (skipping transitive dependencies).
Latest version resolution across ecosystems
.github/workflows/dependency-review.yml
Latest-version resolution branches are reworked: for actions, newest stable (non-draft/prerelease) GitHub releases with published_at on/before cutoff are selected; for package ecosystems (npm, pip, maven, nuget, rubygems, go, cargo), newest non-deprecated versions with publishedAt on/before cutoff are selected from deps.dev.
Staleness comparison with wildcard pin support
.github/workflows/dependency-review.yml
Stale/OK comparison is replaced with new logic that computes normalized version forms and conditionally applies wildcard prefix-matching (when pinned version contains *) or semver ordering (exact pins); success/error output is updated to reflect "eligible latest" semantics.
Documentation update
AGENTS.md
Version Check description is refined to specify that checks apply only to newly-added dependencies in non-lockfile manifests, document the minimum-release-age-days quarantine behavior, describe a known false-positive gap when version duplication occurs without matching removal, and explain version resolution via dep-graph compare API and deps.dev/GitHub Releases.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: fixing false positives in the dependency Version Check workflow, which aligns with the primary objective of reducing false failures in version checking.
Description check ✅ Passed The description comprehensively explains the problem being solved, the implemented fix, verification steps, and known limitations—all directly related to the changeset in the workflow and documentation files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@gtbuchanan gtbuchanan force-pushed the fix/version-check-transitive-deps branch from 9d8c187 to 6085a9b Compare June 9, 2026 20:27
The Version Check iterated every dep GitHub's dep-graph compare marked
"added", which produced two classes of false failure:

- Transitive churn: the compare collapses npm/pnpm/yarn to the lockfile,
  so a single-package bump surfaces all of that package's transitive
  deps as "added". Those versions aren't ours to control. Now only deps
  the PR actually declares in a changed non-lockfile manifest are
  checked; lockfile-only (transitive) entries are skipped.

- Release-age quarantine: "latest" meant the absolute latest, so any dep
  failed the moment upstream cut a release, even though our policy
  (Renovate's minimumReleaseAge) holds adoption for 3 days. "Latest" now
  means the newest release past the quarantine window, and a pin is
  stale only when it's behind that -- being at or ahead is fine. Adds a
  minimum-release-age-days input (default 3, set 0 to disable).

Together these let legitimate dependency PRs pass while still catching
genuinely outdated new pins. Documented the behavior and the known
cross-manifest duplicate gap in AGENTS.md.
@gtbuchanan gtbuchanan force-pushed the fix/version-check-transitive-deps branch from 6085a9b to 3c28faa Compare June 9, 2026 21:05
@gtbuchanan gtbuchanan marked this pull request as ready for review June 9, 2026 21:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/dependency-review.yml:
- Around line 107-115: The gh API call used to compute the latest release (the
command that sets the variable `latest`) only requests a single page
(`per_page=100`) and can miss older releases; update that invocation to use
`--paginate` so all release pages are retrieved before filtering and computing
`latest`, ensuring the `latest` variable is computed across all releases rather
than just the first page.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 505bb31d-5842-4315-8083-2071907d4bf6

📥 Commits

Reviewing files that changed from the base of the PR and between e1ab5ed and 3c28faa.

📒 Files selected for processing (2)
  • .github/workflows/dependency-review.yml
  • AGENTS.md
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • gtbuchanan/tooling (manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
.github/workflows/*.yml

📄 CodeRabbit inference engine (AGENTS.md)

GitHub Actions workflow files must reference composite actions by full path (e.g., gtbuchanan/tooling/.github/actions/<name>@main``), not relative paths like ./, to ensure resolution against the correct repository workspace

Node.js, pnpm, hk, or pkl requiring jobs must prepend the mise-setup composite action as their first step to install exact tool versions from mise.toml and mise.lock

Leaf job names in GitHub Actions branch protection (e.g., Build, E2E Test, Pre-Commit Run) must stay descriptive and unique; only pipeline job names are for Checks-tab grouping

Files:

  • .github/workflows/dependency-review.yml
.github/workflows/!(pr|release).yml

📄 CodeRabbit inference engine (AGENTS.md)

In CI/CD workflows, reusable workflows must be workflow_call-only (no direct triggers) to ensure inputs context always populates; triggers belong in pipeline jobs (pr.yml, release.yml)

Files:

  • .github/workflows/dependency-review.yml
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: gtbuchanan/tooling

Timestamp: 2026-06-09T21:10:02.580Z
Learning: Every PR requires a changeset in `.changeset/<name>.md` with YAML frontmatter listing affected packages and bump types; use empty frontmatter for PRs that don't affect published packages
Learnt from: CR
Repo: gtbuchanan/tooling

Timestamp: 2026-06-09T21:10:02.580Z
Learning: Source tests belong in `test/` directories; E2E tests belong in `e2e/` directories. Coverage is tracked via source config for unit/integration tests but not for artifact E2E tests
Learnt from: CR
Repo: gtbuchanan/tooling

Timestamp: 2026-06-09T21:10:02.580Z
Learning: Turbo tasks in this monorepo expect full cache misses on any source change in `packages/*` workspaces due to Turbo 2.x's `hashOfInternalDependencies` behavior mixing all reachable packages
Learnt from: CR
Repo: gtbuchanan/tooling

Timestamp: 2026-06-09T21:10:02.580Z
Learning: On Termux/Android, run `pnpm build`, `pnpm test:slow`, and `pnpm test:e2e` with `--concurrency=1` to avoid OOM due to memory constraints
📚 Learning: 2026-06-04T18:38:29.949Z
Learnt from: gtbuchanan
Repo: gtbuchanan/tooling PR: 141
File: .github/workflows/changeset-check.yml:15-15
Timestamp: 2026-06-04T18:38:29.949Z
Learning: In this repository (gtbuchanan/tooling), GitHub Actions used in workflows are intentionally referenced by moving tags/branches (e.g., `actions/checkoutv*`, `actions/upload-artifactv*`, `actions/cachev*`, `actions/create-github-app-tokenv*`, and other third-party actions like `rharkor/caching-for-turbov*`), and should never be pinned to a commit SHA. There is no repo-wide SHA-pinning policy; therefore, do not flag these as “unpinned” or suggest SHA-pinning (zizmor’s default `unpinned-uses` rule does not apply here). For in-repo composite actions referenced by reusable workflows, the deliberate documented convention is to use `main` (e.g., `gtbuchanan/tooling/.github/actions/mise-setupmain`), matching how consumers reference the reusable workflows themselves (`.../.github/workflows/<name>main`). Do not change `main` references to SHAs to avoid hash bumps and the self-reference paradox.

Applied to files:

  • .github/workflows/dependency-review.yml
🔇 Additional comments (2)
AGENTS.md (1)

237-254: LGTM!

.github/workflows/dependency-review.yml (1)

28-45: LGTM!

Also applies to: 59-85, 123-177, 214-224

Comment on lines +107 to +115
# Newest stable release whose publish date clears the window.
latest=$(
gh api "repos/${repo}/releases?per_page=100" \
--jq "[ .[]
| select(.draft == false and .prerelease == false)
| select((.published_at | fromdateiso8601) <= ${cutoff})
| .tag_name ] | .[]" 2>/dev/null \
| sed 's/^v//' | grep -E '^[0-9]+(\.[0-9]+)*$' | sort -V | tail -n1
) || latest=""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

For the GitHub REST API releases endpoint (GET /repos/{owner}/{repo}/releases), what is the maximum per_page, and does gh api --paginate fetch all pages?

💡 Result:

For the GitHub REST API releases endpoint (GET /repos/{owner}/{repo}/releases), the maximum value for the per_page parameter is 100 [1][2][3]. Yes, when using the GitHub CLI command gh api --paginate, it automatically fetches and prints all pages of results until no further pages remain [4][5][6]. When used with a REST API request, the GitHub CLI automatically sets per_page to 100 to optimize the number of requests made [6].

Citations:


Paginate the GitHub releases lookup before computing latest.

GitHub’s GET /repos/{owner}/{repo}/releases caps per_page at 100; without gh api --paginate, this only inspects the first page and can make latest resolve empty, silently skipping the quarantine window. Add --paginate to the gh api call so all release pages are considered.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/dependency-review.yml around lines 107 - 115, The gh API
call used to compute the latest release (the command that sets the variable
`latest`) only requests a single page (`per_page=100`) and can miss older
releases; update that invocation to use `--paginate` so all release pages are
retrieved before filtering and computing `latest`, ensuring the `latest`
variable is computed across all releases rather than just the first page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant