advisory check for published tarball drift [agent-managed] - #7019
advisory check for published tarball drift [agent-managed]#7019pip-the-concierge-via-chinmina[bot] wants to merge 7 commits into
Conversation
…tions
- Add new check-changeset workflow that runs on PRs to detect when package
source files change without an accompanying changeset. Non-package changes
(like renovate config, docs, CI files) no longer require empty changesets.
- Split the changeset.yaml notify-slack job into two targeted notifications:
- notify-slack-success: fires only when packages are actually published
- notify-slack-failure: fires only when the version job fails (real publish
failure), not when there are simply no changesets to publish
- Add check:changeset script to package.json for local changeset status checks
- Add .devbox/ to prettierignore and eslint ignores to prevent environment
artifacts from failing lint checks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Chris Chan <chris.chan@cultureamp.com>
…-managed] Replace the path-based check-changeset.yml (which missed cross-package propagation like design-token changes affecting components' built CSS) with a tarball-fingerprint check that compares pnpm-packed output against the published npm registry version. Changes: - Add scripts/check-package-output.ts: per-package sha256 manifest fingerprinting against the published tarball on npm - Add .github/workflows/check-package-output.yml: advisory PR check (not a merge gate) that reports drift without a changeset - Delete .github/workflows/check-changeset.yml: superseded by the tarball check which catches all cases including cross-package deps - Fix notify-slack-failure: add always() so the job actually runs when the version job fails (GitHub Actions skips dependents by default) - Improve failure Slack notification: use Block Kit with commit SHA, actor, subject line, and a direct link to the failed run No hard changeset gate remains — by design. A package change without a changeset will merge and simply not publish. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Chris Chan <chris.chan@cultureamp.com>
🛎️ Concierge
|
|
There was a problem hiding this comment.
Overview
This PR replaces a path-based changeset check with an advisory tarball-fingerprint check that actually builds packages and compares their output to what's published on npm — a significant improvement in accuracy. It also fixes the notify-slack-failure job to actually fire on failure and upgrades the Slack payload to Block Kit. The architectural approach is sound and well-structured.
Issues
Two confirmed issues: (1) the curl command interpolates an unsanitized npm registry URL into a shell string, which is vulnerable to $(...) command injection; (2) the Slack Block Kit payload interpolates the commit subject directly into JSON without escaping, which will produce malformed JSON for commits containing quotes or backslashes.
For reviewers
Change groups
- Advisory tarball drift check (new workflow + script):
.github/workflows/check-package-output.yml,scripts/check-package-output.ts,package.json - Slack failure notification fix:
.github/workflows/changeset.yaml(lines 54-116)
Reading order
.github/workflows/check-package-output.yml— 26-line workflow; shows the trigger, build prerequisite, and script invocationscripts/check-package-output.ts— core logic: pack local tarball, download published tarball, fingerprint both, cross-reference changeset status, report drift.github/workflows/changeset.yaml— independent change:always()condition onnotify-slack-failurejob and new Block Kit payload
Key questions
- The script shells out to
curlwith an unescapedtarballUrlfromnpm view(line 198) — is there any concern about URL injection from a compromised registry response? - Exit code 1 (drift detected) will surface as a red check on PRs even though it is advisory — is that the intended signal, or should it use
continue-on-error/ neutral conclusion to avoid confusion with true failures?
7 Low-signal findings hidden
click to expand
scripts/check-package-output.ts: Claimedpath.joinbug with absolute pnpm output — rejected (pnpm outputs filename only, not absolute path)scripts/check-package-output.ts: UnhandledexecSyncfor tar extraction — rejected (tied to false path.join premise; surrounding try/catch catches thrown errors).github/workflows/check-package-output.yml: Advisory vs blocking mismatch — rejected (non-required checks that exit 1 are standard advisory pattern in GitHub).github/workflows/changeset.yaml:${{ }}wrapper on jobif:claimed invalid — rejected (this is valid GitHub Actions syntax).github/workflows/check-package-output.yml: AI Testing Harness requirement — rejected (PR is currently going through that process).github/workflows/check-package-output.yml: Missingpermissionskey — rejected (consistent with all other CI workflows in this repo).github/workflows/check-package-output.yml: GitHub Actionscontainon tech radar — rejected (repo already has 14 GH Actions workflows; this replaces an existing one)
generated using the pr-review workflow in 32 turns
give feedback with reactions 👍 👎
- Kept the changeset cross-reference so PRs that already have one stay green Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
- Trigger inverted to push/branches-ignore so the guard works without a PR context Co-Authored-By: Claude <noreply@anthropic.com>
- npm view inherits pnpm's env config and buries the result in warnings - A 404 is now distinguishable from a real registry failure Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Objective
Advisory check: does the tarball we would publish differ from the one on npm, for a package with no changeset? Replaces the path-based changeset gate, which could not see changes that originate outside a package but still alter its output — a design-token value is inlined into
@kaizen/components' built CSS. Pass/fail per package only; a human decides whether it warrants a release.Not a merge gate — must not be added to required checks. No hard changeset gate remains, by design: a missing changeset means no publish, not a blocked merge. Runs on feature-branch pushes, the inverse of
changeset.yaml.Folds in #7018 (now closed): the publish-failure Slack detail and the
.devbox/lint ignores come with it.Reviewer guide
pnpm-lock.yaml(addstsx),.prettierignore+eslint.config.js(ignore devbox artefacts).scripts/check-package-output.ts— fingerprint and skip semantics.check-package-output.yml—branches-ignorecarries the whole guard, sincehead_refis empty on push events.Decisions
pnpm packmatches publish output byte for byte, and the resolvedworkspace:version is part of the signal.Verifications
@kaizen/componentsalone, exit 1; with a changeset present it reports~, exit 0.Requested by: @ckychris