chore: bump js-yaml to ^5.4.1

[wrn14897]

Bumps js-yaml from ^4.2.0 to ^5.3.0 (latest). v5 keeps the CJS load/dump API used by scripts/update-chart-versions.js; verified the script still produces byte-identical Chart.yaml output after the upgrade.

---

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7a617d9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

<!-- deep-review -->

Deep Review

🔴 P0/P1 — must fix

• scripts/update-chart-versions.js:2 — The release script loads the bumped dependency with CommonJS require("js-yaml") from a package with no "type": "module", while the lockfile entry in this diff renames the package bin from bin/js-yaml.js to bin/js-yaml.mjs and the v5.0.0 changelog embedded in base commit d370fb5 records a TypeScript rewrite that "reorganized the public API around flat named exports" with a dedicated migrate_v4_to_v5.md guide, so require() may throw ERR_REQUIRE_ESM or yield a namespace object whose .load is undefined. Because .github/workflows/release.yml:42 (version: yarn run version) is the only place this script ever runs and no PR-time workflow installs root dependencies, the first real execution against v5 happens post-merge inside changesets/action on main, where the failure blocks all chart releases rather than failing this PR.
  • Fix: Run yarn install && node scripts/update-chart-versions.js on Node 20 and confirm a zero exit before merging, and if require() fails, move to ^4.3.0 instead — it carries the maxTotalMergeKeys security backport that this repo's current 4.2.0 lacks, without the v5 API rewrite.
  • _{correctness, testing, maintainability, project-standards, reliability, security}

🟡 P2 — recommended

• scripts/update-chart-versions.js:12 — yaml.dump(chart) is called with no options, so the entire published charts/clickstack/Chart.yaml is re-serialized from whatever emitter defaults the installed major ships, and v5.1.0's changelog marks quoteStyle as [breaking] while v5.0.0 reworked scalar resolution and added seven new dump options. Release commit b42bbac confirms the v4 round-trip currently rewrites the file with only the version: line changing, meaning the folded description: >- at charts/clickstack/Chart.yaml:3-5 and the literal artifacthub.io/links: | block at lines 12-16 are v4-emitter-normalized and would silently reformat in the release commit under any v5 emitter delta.
  • Fix: Pin the emitter by passing explicit options such as yaml.dump(chart, { lineWidth: 80, noRefs: true }), or follow the precedent already set in .github/workflows/update-app-version.yml:26 and replace the version: line with a targeted edit instead of a full parse-and-re-serialize.
  • _{correctness, maintainability, reliability}
• .github/workflows/release.yml:42 — Neither chart-test.yml nor helm-test.yaml installs root Node dependencies or invokes update-chart-versions, so this dependency bump merges with zero automated evidence that its single consumer still runs, which is precisely why both findings above can only surface during a live release.
  • Fix: Add a PR-time job that runs the script against a scratch copy of Chart.yaml and asserts both a zero exit and that git diff reports no change beyond the version: field.
  • _{testing, correctness, maintainability, project-standards, reliability, security}

🔵 P3 nitpicks (1)

• package.json:11 — "version": "changeset version && npm run update-chart-versions" sequences two mutations non-atomically, so a throw in the second step leaves package.json bumped and charts/clickstack/Chart.yaml:18 stale — the exact skew the script exists to prevent; CI fails safe here because changesets/action aborts without opening a PR, but a maintainer running yarn version locally can commit the half-applied state.
  • Fix: Add a post-version assertion that package.json version equals the chart's version.

---

Reviewers (7): correctness, testing, maintainability, project-standards, reliability, security, learnings-researcher

Testing gaps:

• No CI job executes scripts/update-chart-versions.js before merge; the only invocation path is changesets/action on main after merge.
• No golden-file or idempotency assertion that a yaml.load + yaml.dump round-trip leaves charts/clickstack/Chart.yaml unchanged apart from the version: field.
• No smoke check that require("js-yaml").load and .dump are callable functions under the release runtime.
• docs/solutions/ does not exist, and scripts/update-chart-versions.js has never been modified since creation, so there is no prior incident record for this upgrade path.

Cleared during review: No secrets in the diff; the lockfile entry is self-consistent with a plain npm: resolution, an unchanged transitive set (argparse ^2.0.1), and a well-formed checksum. yaml.load uses DEFAULT_SCHEMA on repo-owned input only, so there is no parsing-safety regression. The caret range is inert because Yarn 4 enforces immutable installs in CI and the lockfile pins 5.3.0, and omitting a changeset matches this repo's precedent for dependency bumps.

[wrn14897]

Re: P0 — require("js-yaml") under v5

Verified the concern doesn't materialize, and closed the coverage gap:

• js-yaml v5 is a dual CJS/ESM build: exports.require → dist/js-yaml.cjs.js. Only the bin entrypoint moved to .mjs, which this repo never invokes. CJS require() returns an object with load/dump functions.
• Ran on Node 20.19.3 (matching release.yml): yarn install && node scripts/update-chart-versions.js → exit 0, and the Chart.yaml round-trip output is byte-identical to the committed file.
• The real gap was that this script only ever executed post-merge in release.yml. Added a version-script-smoke job to the PR-time helm-test.yaml workflow (373f7ce) that installs root deps on Node 20, runs the script, and asserts the round-tripped Chart.yaml parses with the version from package.json — so future dependency bumps fail on the PR instead of blocking releases on main.

No downgrade to ^4.3.0 needed; v5.3.0 includes the same maxTotalMergeKeys hardening.

[github-actions]

Deep Review

✅ No critical issues found. The lockfile change is internally consistent and actually repairs a real desync — at db925bb package.json declared js-yaml ^4.2.0 while yarn.lock carried no matching resolution and the helm-charts@workspace:. entry listed no js-yaml at all, so a Yarn 4 immutable CI install would have failed (or require("js-yaml") would have hoisted to the transitive 3.14.1). The ESM concern raised in earlier rounds does not reproduce: v5 ships a dual build and both consumers use only load/dump. The findings below are about the new guard being narrower than its comment claims, and about drift between what was verified and what is pinned.

🟡 P2 — recommended

• .github/workflows/helm-test.yaml:42 — The version assertion is true by construction: update-chart-versions.js writes chart.version from ./package.json, and the check re-reads the same ./package.json, so it passes even if the script stopped writing the version entirely.
  • Fix: Write a sentinel version into package.json before invoking the script, assert the sentinel landed in Chart.yaml, then git checkout -- package.json charts/*/Chart.yaml.
  • _{correctness, testing, maintainability, previous-comments}
• .github/workflows/helm-test.yaml:41 — The round-trip check parses only version, so a yaml.dump emitter change could reformat the folded description: >-, the annotations."artifacthub.io/links" literal block that ArtifactHub re-parses, or the ~0.146.0 dependency constraint, and still pass; the in-repo changelog at d370fb5 records quoteStyle/forceQuotes as an explicit [breaking] dump change shipped in the v5.1.0 minor.
  • Fix: Deep-compare the regenerated document against git show HEAD:charts/clickstack/Chart.yaml with only version normalized, or run git diff --exit-code -- charts/clickstack/Chart.yaml before the restore.
  • _{correctness, testing, reliability, security, previous-comments}
• .github/workflows/helm-test.yaml:52 — The extract-release-notes.js step cannot fail: the script console.warns and process.exit(0) when no ## <version> section matches, and the following rm -f succeeds whether or not the file was written.
  • Fix: Insert test -s charts/clickstack/RELEASE_NOTES.md || exit 1 between the script invocation and the rm -f.
  • _{correctness, testing, security}
• .github/workflows/helm-test.yaml:48 — git checkout -- charts/*/Chart.yaml restores the pristine file before extract-release-notes.js runs, inverting the release ordering where yarn run version rewrites Chart.yaml via yaml.dump first and extract-release-notes.js parses that dumped output afterwards.
  • Fix: Move the git checkout -- restore after the release-notes step so the dump→load handoff between the two scripts is exercised.
• package.json:16 — The range is pinned at ^5.4.1 while the round-trip verification recorded on this PR was performed against 5.3.0, and the only in-repo js-yaml changelog stops at 5.2.0, so neither the shipped version nor any future 5.x minor the caret admits has a verified emitter baseline.
  • Fix: Re-run the round-trip byte-diff against 5.4.1 and narrow the range to ~5.4.1, or pass explicit dump options in scripts/update-chart-versions.js so formatting stops depending on emitter defaults.
  • _{previous-comments, correctness, security}
• .github/workflows/helm-test.yaml:12 — This workflow previously ran only helm/apt commands on pull_request; the new job installs npm dependencies (Yarn Berry runs package build scripts by default — .yarnrc.yml sets only nodeLinker) and executes PR-authored scripts/*.js, yet helm-test.yaml declares no permissions: block and actions/checkout@v3 defaults to persist-credentials: true.
  • Fix: Add permissions: { contents: read } to the job and set persist-credentials: false on the checkout step, since the only git operation needed is git checkout -- <path>.
  • _security
• .github/workflows/helm-test.yaml:6 — The workflow's push trigger is filtered to paths: 'charts/**', so a dependency-or-script-only merge never re-runs this gate on main, while chart-test.yml triggers on push to main with no paths filter and is separately listed in release.yml's workflow_run — meaning it can green-light a release on its own.
  • Fix: Add package.json, yarn.lock, scripts/**, and .github/workflows/helm-test.yaml to the push paths filter.
  • _{correctness, maintainability}
• .github/workflows/release.yml:7 — Pre-existing, but this PR extends the workflow that feeds it: the workflow_run branches: [main] filter matches the head branch of the triggering run, so a fork PR opened from a branch literally named main produces a successful Helm Chart Tests run that satisfies both the branch filter and conclusion == 'success', firing a job that holds contents: write and pull-requests: write.
  • Fix: Extend the guard to github.event.workflow_run.event == 'push' && github.event.workflow_run.head_repository.full_name == github.repository.
  • _security

🔵 P3 nitpicks (6)

• .github/workflows/helm-test.yaml:37 — The ten-line assertion lives as a node -e string literal inside a YAML block scalar, so it cannot be linted, unit-tested, or run locally without copy-pasting it out.
  • Fix: Move it to scripts/assert-chart-version.js and invoke that from the workflow.
• AGENTS.md:153 — The CI Workflows table still describes helm-test.yaml as "Unit tests + example validation", which no longer covers the version-script-smoke job.
  • Fix: Extend the Purpose column to mention the release-script smoke test.
  • _{maintainability, project-standards}
• .github/workflows/helm-test.yaml:16 — The job has no timeout-minutes, so a hung corepack enable or yarn falls back to the 360-minute default while sitting on the release gate.
  • Fix: Add timeout-minutes: 10.
• .github/workflows/helm-test.yaml:30 — run: yarn relies on Yarn Berry's CI=true auto-detection for lockfile enforcement rather than stating it.
  • Fix: Use yarn install --immutable.
• .github/workflows/helm-test.yaml:18 — The checkout/setup-node/corepack/yarn block duplicates release.yml's setup and has already drifted: this job uses actions/checkout@v3 while release.yml still uses @v2.
  • Fix: Extract the shared setup into a composite action referenced by both workflows.
• .github/workflows/helm-test.yaml:41 — yaml.load is called with no options, and maxAliases defaults to -1 (unbounded) in the v5 line, so a fork PR could put an alias bomb in Chart.yaml to burn a runner.
  • Fix: Pass { maxAliases: 100 } to yaml.load here and in both scripts.

---

Reviewers (7): correctness, testing, maintainability, project-standards, reliability, security, previous-comments.

Testing gaps:

• No assertion covers the fields most exposed to a v5 emitter change — folded description, the artifacthub.io/links literal block, and dependencies[0].version: ~0.146.0.
• Nothing runs helm lint / helm show chart against the dumped Chart.yaml before it is restored, so "parses as YAML" is tested but "still a valid chart" is not.
• The two scripts are exercised in isolation rather than chained the way release.yml chains them.
• No test covers extract-release-notes.js's no-match branch or its regex escaping of the version string.

[wrn14897]

Re: P0/P1 — v5 ESM rewrite vs CommonJS require() in the release scripts

Verified directly against the installed js-yaml@5.3.0 — no ESM breakage:

• The package's exports map declares an explicit require condition → dist/js-yaml.cjs.js (import gets dist/js-yaml.mjs). Only the bin entrypoint moved to .mjs, and nothing in this repo invokes the CLI.
• require.resolve("js-yaml") → node_modules/js-yaml/dist/js-yaml.cjs.js; typeof load / typeof dump are both function, and a parse/dump round-trip succeeds — on Node 20 (release.yml's version).
• Both consumers use only load/dump: scripts/update-chart-versions.js and scripts/extract-release-notes.js. Ran both on Node 20.19.3 with exit 0.

Coverage: update-chart-versions.js was already smoke-tested at PR time by the version-script-smoke job; a228ba5 extends that job to also run extract-release-notes.js, so both release-only scripts now execute on every PR. No ESM conversion or hold on 4.x needed.