fix(gha): Fix pnpm upgrade workflow

[jsteinich]

Description

Attempting to fix failures seen in pnpm upgrade job: https://github.com/open-constructs/cdk-terrain/actions/runs/29232337176/job/86759126716

Checklist

• I have updated the PR title to match CDKTN's style guide
• I have run the linter on my code locally
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation if applicable
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works if applicable
• New and existing unit tests pass locally with my changes

[sakul-learning]

The direction is right, but the current diff cannot work with the ncu version installed by this workflow, and there is a second package-matrix failure that --packageManager pnpm alone does not address.

1. Pin a pnpm-capable ncu release

.github/workflows/pnpm-upgrade.yml still installs this in all three upgrade jobs (currently lines 43, 168, and 235):

npm -g install npm-check-updates@^9.0.0

The latest v9 release, 9.2.4, accepts only npm and yarn for --packageManager; it exits with Error: Invalid package manager: pnpm. Please pin the current reviewed release instead in all three places:

npm -g install npm-check-updates@22.2.9

I validated 22.2.9 against this PR's toolchain and workspace:

• Node 22.22.3
• npm 10.9.8
• ncu 22.2.9
• a non-mutating root invocation with the proposed flags returned {} successfully
• a non-mutating invocation using repeated --workspace cdktn --workspace @cdktn/cli-core --no-root resolved the two expected workspace manifests successfully

Those Node/npm versions satisfy ncu 22.2.9's documented engines (^20.19 || ^22.12 || >=24, npm >=10). Current ncu can auto-detect pnpm from pnpm-lock.yaml, so the explicit flag is not strictly required; retaining it is still reasonable in CI because it makes the intended package manager deterministic.

2. Make the v9 → v22 default changes explicit

Current ncu includes the root packageManager field in its default dependency sections and includes deprecated releases by default. Without an explicit --dep, the broad root update can also rewrite the integrity-pinned:

"packageManager": "pnpm@11.5.2+sha512..."

That would couple pnpm toolchain upgrades to the ordinary dependency-upgrade job. To preserve the older policy and keep the package-manager pin separately controlled, add these options to every ncu invocation:

--packageManager pnpm
--dep prod,dev,optional,peer
--no-deprecated

In particular, --dep prod,dev,optional,peer deliberately omits packageManager.

3. Use ncu's workspace support instead of dispatching manifest edits through repeated pnpm exec nx exec

The failed package-matrix jobs show this sequence:

1. the first ncu pass edits workspace manifests;
2. the next outer pnpm exec nx exec starts under CI=true;
3. pnpm checks the now-dirty workspace against pnpm-lock.yaml and exits with ERR_PNPM_OUTDATED_LOCKFILE before the final non-frozen install can run.

Adding --packageManager pnpm does not fix that sequencing issue. ncu 22.2.9 will not fix repeated outer pnpm exec calls automatically either. However, current ncu supports pnpm workspaces directly, so this PR can remove pnpm/Nx from the manifest-edit dispatch path and perform one final lockfile reconciliation afterward. This is consistent with the repository's broader move away from Yarn Classic/Lerna toward pnpm/Nx, while using each tool for the job it owns.

All required changes are in .github/workflows/pnpm-upgrade.yml:

upgradeRoot

Keep the three direct root ncu calls, but use ncu 22.2.9 and append the explicit compatibility options above. No workspace option is needed here.

upgradePackage

Replace the three commands shaped like:

pnpm exec nx exec -p '${{ join(matrix.pr.packages, ',') }}' -- ncu ...

with direct ncu workspace calls. ncu requires one --workspace option per workspace. A safe way to derive that from the existing static matrix without interpolating expressions directly into shell code is:

- name: Run "ncu -u"
  shell: bash
  env:
    NCU_WORKSPACES: ${{ join(matrix.pr.packages, ' ') }}
  run: |-
    read -ra workspaces <<< "$NCU_WORKSPACES"
    workspace_args=()
    for workspace in "${workspaces[@]}"; do
      workspace_args+=(--workspace "$workspace")
    done

    ncu --upgrade "${workspace_args[@]}" --no-root \
      --filter=@types/fs-extra --target=minor \
      --packageManager pnpm --dep prod,dev,optional,peer --no-deprecated

    ncu --upgrade "${workspace_args[@]}" --no-root \
      --filter=typescript --target=patch \
      --packageManager pnpm --dep prod,dev,optional,peer --no-deprecated

    ncu --upgrade "${workspace_args[@]}" --no-root \
      --reject='@types/node,@types/fs-extra,constructs,typescript,graphology-types,jsii,jsii-pacmak,jsii-rosetta,jsii-docgen,codemaker,${{ steps.list-packages.outputs.list }}' \
      --target=minor \
      --packageManager pnpm --dep prod,dev,optional,peer --no-deprecated

--no-root is important here because ncu v17+ includes the root manifest by default in workspace mode; each matrix entry should touch only its selected workspace packages.

Keep the existing final reconciliation:

pnpm install --prefer-offline --no-frozen-lockfile

Then validate the result with a clean frozen install. Direct ncu workspace runs only edit manifests; they do not regenerate pnpm-lock.yaml, so the final pnpm step remains required.

upgradeJSII

Replace:

pnpm exec nx exec -- ncu --upgrade --filter='jsii,jsii-pacmak,jsii-rosetta,jsii-docgen,codemaker,constructs' --target=minor ...

with ncu's all-workspaces mode, explicitly preserving the current root-inclusive behavior:

ncu --upgrade --workspaces --root \
  --filter='jsii,jsii-pacmak,jsii-rosetta,jsii-docgen,codemaker,constructs' \
  --target=minor \
  --packageManager pnpm --dep prod,dev,optional,peer --no-deprecated

Keep its final pnpm install --prefer-offline --no-frozen-lockfile as well.

The focused validation for this change should reproduce the upgradePackage sequence with CI=true, confirm all selected manifests are processed without ERR_PNPM_OUTDATED_LOCKFILE, run the final non-frozen install, and then confirm a clean pnpm install --frozen-lockfile succeeds.

Separate follow-up: reviewer-assignment authentication

The linked JSII job actually completed ncu, refreshed the pnpm lockfile, pushed the branch, and created PR #327; it was marked failed afterward because team-reviewers: cdktn-maintainers could not be requested with the existing token. That authentication issue is separate from this PR.

A follow-up should reuse the existing CDKTN maintainers GitHub App flow from .github/workflows/release-please.yml:

uses: actions/create-github-app-token@...
with:
  app-id: ${{ secrets.CDKTN_MAINTAINERS_APP_ID }}
  private-key: ${{ secrets.CDKTN_MAINTAINERS_APP_PRIVATE_KEY }}

That App token is already used so bot-created PRs trigger normal CI. Once the requested organization/team read permissions are available, it should also be evaluated for the dependency PR jobs instead of retaining a separate legacy PAT. This should remain a separate auth-focused change rather than being mixed into the ncu/workspace repair.

References:

• ncu 22.2.9 options and engines: https://github.com/raineorshine/npm-check-updates/blob/v22.2.9/README.md
• ncu workspace usage: https://thedocs.io/npm-check-updates/usage/workspaces/
• pnpm update/lockfile behavior: https://pnpm.io/cli/update

[so0k]

Based on analysis from GPT5.6 Sol - I'm going to agree we may need to bump npm-check-updates version to achieve this?

[sakul-learning]

Follow-up review — bf3a0be

The ncu/workspace repair now addresses the original functional blockers:

• ncu is upgraded to the pnpm-capable v22 line;
• every invocation explicitly uses --dep prod,dev,optional,peer --no-deprecated, so the root packageManager declaration is not upgraded with ordinary dependencies;
• upgradePackage uses repeated native ncu workspace arguments instead of launching another pnpm exec after manifests become dirty;
• the final pnpm install --no-frozen-lockfile remains after all manifest edits.

I still see the following blockers before merge.

1. New App tokens inherit every installation permission

The new actions/create-github-app-token calls at .github/workflows/pnpm-upgrade.yml:98, :225, and :303 specify no permission-* inputs. By default, each minted token inherits all permissions granted to the App installation. Zizmor reports all three as new high-severity / high-confidence github-app findings.

Please either keep the reviewer-auth migration in the previously suggested separate PR, or explicitly narrow every token to what these dependency PR jobs need:

with:
  app-id: ${{ secrets.CDKTN_MAINTAINERS_APP_ID }}
  private-key: ${{ secrets.CDKTN_MAINTAINERS_APP_PRIVATE_KEY }}
  permission-contents: write
  permission-pull-requests: write
  permission-members: read

• contents: write is needed to create/update the automation branch.
• pull-requests: write covers creating/updating the PR, labels, and review requests.
• members: read limits organization access to the team lookup already being requested for cdktn-maintainers.

The App installation must grant those permissions; these inputs narrow the resulting token and cannot elevate it. prRoot should also declare an explicit job-level permissions: { contents: read }, matching the other jobs, so its ambient GITHUB_TOKEN is not left at repository defaults.

2. ncu is still range-pinned rather than fully pinned

All three install steps currently use:

npm -g install npm-check-updates@^22.2.9

The prior recommendation was an exact reviewed tool pin:

npm -g install npm-check-updates@22.2.9

The caret allows a future 22.x release to change scheduled CI behavior without a repository change or review. This workflow already pins actions/container content and the repository integrity-pins pnpm, so ncu should be deterministic as well.

Scope / artifact-value

The App-token migration is separable from the ncu/workspace fix and expands this PR into credential/identity behavior. split: the smallest reviewable path is to merge the ncu repair independently and move the three App-token/committer changes to an auth-focused PR. If retained here, the least-privilege fixes above are required.

No new tests were added. The configured cdktn tests are green, but they do not exercise this scheduled workflow; a safe workflow_dispatch run remains the meaningful end-to-end validation for ncu workspace selection, lockfile reconciliation, App permissions, and team-reviewer assignment.

Zizmor baseline comparison

• Changed workflow: .github/workflows/pnpm-upgrade.yml
• Baseline findings: 121
• Head findings in the changed workflow: 12
• New/touched findings: 3
• Result: ❌ ATTENTION REQUIRED — the three new unrestricted GitHub App-token findings above are PR-specific blockers.

Verdict: REQUEST_CHANGES

[so0k]

oops not sure why The bot posted that 2nd comment - I didn't approve it to comment 😬

[sakul-learning]

Follow-up review — 4dff66d

The previous blockers are resolved:

• all three jobs now pin the reviewed tool exactly as npm-check-updates@22.2.9;
• all three GitHub App tokens are narrowed to contents: write, pull-requests: write, and members: read;
• prRoot now limits its ambient GITHUB_TOKEN to contents: read;
• the ncu native-workspace selection, explicit dependency/deprecation policy, and final non-frozen lockfile refresh remain intact.

The configured checks pass:

• pnpm exec jest packages/cdktn/test/validations.test.ts --runInBand: 31/31 tests and 9/9 snapshots
• pnpm exec nx build cdktn: success, 0 errors / 0 warnings (+30 silenced)
• pnpm exec nx test cdktn --runInBand: 457/457 tests and 288/288 snapshots

These code-level checks do not execute the scheduled workflow; a safe workflow_dispatch run remains the best end-to-end confirmation of the App installation permissions and team-reviewer assignment, but I do not see a merge blocker in the current diff.

Zizmor baseline comparison

• Changed workflow: .github/workflows/pnpm-upgrade.yml
• Baseline findings: 121
• Head findings in the changed workflow: 7
• New/touched findings: 0
• Result: ✅ PASS — the three prior unrestricted GitHub App-token findings are gone, with no new or touched workflow-security findings.

Ponytail artifact-value pass: the changed workflow logic is now cohesive and each added permission/configuration line serves the active dependency-update path. No new tests, generated files, dependencies, docs, or delivered SpecLedger artifacts were added.

Verdict: APPROVE