Skip to content

Merge v4-beta to v4 (2026-05-29)#173

Closed
PaulNewling wants to merge 2 commits into
v4from
merge-2026-05-29
Closed

Merge v4-beta to v4 (2026-05-29)#173
PaulNewling wants to merge 2 commits into
v4from
merge-2026-05-29

Conversation

@PaulNewling
Copy link
Copy Markdown
Collaborator

@PaulNewling PaulNewling commented May 29, 2026

Merge v4-beta into v4

Greptile Summary

This PR scopes the catalog-bump coverage check in check-coverage.sh to only runtime dependencies (dependencies and peerDependencies), dropping devDependencies and optionalDependencies from the jq filter. The motivation is to stop false positives on PRs that touch a catalog entry used only as build tooling (e.g. block-tools, tengo-builder) — previously any package listing such a catalog entry under devDependencies was incorrectly required to bump its version.

  • check-coverage.sh: jq filter narrowed from all four dep sections to [.dependencies?, .peerDependencies?]; inline comment documents the intent.
  • action.yaml: description updated to reflect the runtime-only scoping.
  • test/coverage.bats + pkg-dev fixture: new test case verifies a devDependency-only catalog consumer is not flagged while a runtime consumer (pkg-c) still is.

Confidence Score: 4/5

Safe to merge with awareness of the optionalDependencies exclusion, which differs from the stated intent of only skipping build-only tooling.

The devDependency exclusion is well-motivated, tested, and documented. The one concern is that optionalDependencies — which are runtime-facing and published in the package manifest — were dropped from the jq filter alongside devDependencies without a test or explicit justification. This could silently skip a required bump for any package that pulls a catalog entry via optionalDependencies. Everything else in the diff looks correct and is well-tested.

The jq filter in check-coverage.sh around line 203 warrants a second look regarding optionalDependencies.

Important Files Changed

Filename Overview
actions/changeset/check-coverage/check-coverage.sh Core logic change: jq dep-section filter narrowed to runtime-only; optionalDependencies silently excluded without a dedicated test or matching documentation in action.yaml
actions/changeset/check-coverage/action.yaml Description updated to mention runtime-only scoping, but does not call out the optionalDependencies exclusion alongside devDependencies
actions/changeset/check-coverage/test/coverage.bats New test case correctly validates that devDependency-only catalog consumers are not flagged while runtime consumers still are
actions/changeset/check-coverage/test/fixtures/minimal-workspace/packages/pkg-dev/package.json New non-private fixture package that lists is-string only as a devDependency, correctly scoped to exercise the new exclusion logic
actions/changeset/check-coverage/test/fixtures/minimal-workspace/packages/pkg-dev/index.js Minimal stub module for the pkg-dev fixture; no issues

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pnpm-workspace.yaml changed?] -- No --> Z[Exit 0 / only direct-edit check]
    A -- Yes --> B[Extract changed catalog keys via yq]
    B --> C{For each workspace package not private}
    C --> D[Read package.json]
    D --> E{Dep section check: dependencies + peerDependencies only}
    E -- catalog: match found --> F[require_pkg bump]
    E -- no match --> G[Skip package]
    F --> H{Package in changeset bumped set?}
    H -- Yes --> I[Covered]
    H -- No --> J[Add to missing list]
    J --> K[Exit 1 + error message]
    I --> K2[Exit 0]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
actions/changeset/check-coverage/check-coverage.sh:198-203
`optionalDependencies` are quietly dropped alongside `devDependencies`, but their semantics differ: optional deps are declared in the published package manifest and installed at runtime (when the platform supports them). The PR motivation and inline comment specifically call out build-only tooling under `devDependencies`; there is no matching justification for `optionalDependencies`, no test fixture covering that case, and the `action.yaml` description only mentions "devDependencies / build-only tooling are not flagged". A catalog bump for a dep used in `optionalDependencies` will now silently skip the consumer, which is the same false-negative the script is supposed to catch.

```suggestion
        # Runtime sections only. devDependencies are excluded on purpose —
        # bumping build-only tooling consumed via `catalog:` (block-tools,
        # tengo-builder, …) doesn't alter the published runtime, so requiring
        # a release for it is a false positive.
        # optionalDependencies ARE runtime-facing (published in the manifest,
        # installed on supported platforms), so they are included here.
        if jq -e --arg k "${key}" '
            [.dependencies?, .peerDependencies?, .optionalDependencies?]
```

Reviews (1): Last reviewed commit: "ci(changeset/check-coverage): ignore dev..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Section 2b required a changeset for every workspace package consuming a
bumped catalog key, scanning all dependency sections. That flagged
packages whose only "change" was a build-tool catalog bump in a
devDependency (e.g. @platforma-sdk/block-tools in a block model,
@platforma-sdk/tengo-builder in a workflow) even though the package's own
files were untouched and `pnpm changeset` does not flag them.

Restrict the catalog-consumer scan to runtime sections (dependencies,
peerDependencies). A runtime catalog dep bump still requires a release;
build-only tooling no longer produces false positives.

Adds a bats case + pkg-dev fixture covering the devDependency path.
Comment on lines +198 to +203
# Runtime sections only. devDependencies/optionalDependencies are
# excluded on purpose — bumping build-only tooling consumed via
# `catalog:` (block-tools, tengo-builder, …) doesn't alter the
# published runtime, so requiring a release for it is a false positive.
if jq -e --arg k "${key}" '
[.dependencies?, .devDependencies?, .peerDependencies?, .optionalDependencies?]
[.dependencies?, .peerDependencies?]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 optionalDependencies are quietly dropped alongside devDependencies, but their semantics differ: optional deps are declared in the published package manifest and installed at runtime (when the platform supports them). The PR motivation and inline comment specifically call out build-only tooling under devDependencies; there is no matching justification for optionalDependencies, no test fixture covering that case, and the action.yaml description only mentions "devDependencies / build-only tooling are not flagged". A catalog bump for a dep used in optionalDependencies will now silently skip the consumer, which is the same false-negative the script is supposed to catch.

Suggested change
# Runtime sections only. devDependencies/optionalDependencies are
# excluded on purpose — bumping build-only tooling consumed via
# `catalog:` (block-tools, tengo-builder, …) doesn't alter the
# published runtime, so requiring a release for it is a false positive.
if jq -e --arg k "${key}" '
[.dependencies?, .devDependencies?, .peerDependencies?, .optionalDependencies?]
[.dependencies?, .peerDependencies?]
# Runtime sections only. devDependencies are excluded on purpose —
# bumping build-only tooling consumed via `catalog:` (block-tools,
# tengo-builder, …) doesn't alter the published runtime, so requiring
# a release for it is a false positive.
# optionalDependencies ARE runtime-facing (published in the manifest,
# installed on supported platforms), so they are included here.
if jq -e --arg k "${key}" '
[.dependencies?, .peerDependencies?, .optionalDependencies?]
Prompt To Fix With AI
This is a comment left during a code review.
Path: actions/changeset/check-coverage/check-coverage.sh
Line: 198-203

Comment:
`optionalDependencies` are quietly dropped alongside `devDependencies`, but their semantics differ: optional deps are declared in the published package manifest and installed at runtime (when the platform supports them). The PR motivation and inline comment specifically call out build-only tooling under `devDependencies`; there is no matching justification for `optionalDependencies`, no test fixture covering that case, and the `action.yaml` description only mentions "devDependencies / build-only tooling are not flagged". A catalog bump for a dep used in `optionalDependencies` will now silently skip the consumer, which is the same false-negative the script is supposed to catch.

```suggestion
        # Runtime sections only. devDependencies are excluded on purpose —
        # bumping build-only tooling consumed via `catalog:` (block-tools,
        # tengo-builder, …) doesn't alter the published runtime, so requiring
        # a release for it is a false positive.
        # optionalDependencies ARE runtime-facing (published in the manifest,
        # installed on supported platforms), so they are included here.
        if jq -e --arg k "${key}" '
            [.dependencies?, .peerDependencies?, .optionalDependencies?]
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@PaulNewling PaulNewling deleted the merge-2026-05-29 branch May 29, 2026 17:15
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