From f45c732703625d0cedc57ba062dd13fa82cb5f4d Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 9 Aug 2026 14:37:03 +0200 Subject: [PATCH 1/5] =?UTF-8?q?feat:=20add=20dist-diff=20=E2=80=94=20artif?= =?UTF-8?q?act-level=20diff=20reports=20for=20PRs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PR's source diff doesn't show its effect on the built artifacts: an 8-line config change can add a wizard cliEntry, a marketplace plugin, and a mirror entry, while a naive diff of two builds reports 221 changed files of which 216 are noise (zip entry mtimes, manifest buildTimestamp). dist-diff compares two normalized dist/ trees and reports the true artifact delta per consumer surface, as a sticky PR comment (posted by a minimal privileged workflow_run job so fork PRs get identical treatment) plus a full report with content hunks in the step summary. A twice-build self-check guards the normalization on pipeline-touching PRs and main. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GDjuSAs927XvcyziFJk865 --- .github/workflows/dist-diff-comment.yml | 85 ++++ .github/workflows/dist-diff.yml | 112 +++++ package.json | 3 + pnpm-lock.yaml | 31 ++ scripts/diff-dist.js | 90 ++++ scripts/lib/dist-diff.js | 542 ++++++++++++++++++++++++ scripts/lib/tests/dist-diff.test.js | 331 +++++++++++++++ 7 files changed, 1194 insertions(+) create mode 100644 .github/workflows/dist-diff-comment.yml create mode 100644 .github/workflows/dist-diff.yml create mode 100644 scripts/diff-dist.js create mode 100644 scripts/lib/dist-diff.js create mode 100644 scripts/lib/tests/dist-diff.test.js diff --git a/.github/workflows/dist-diff-comment.yml b/.github/workflows/dist-diff-comment.yml new file mode 100644 index 00000000..c119740c --- /dev/null +++ b/.github/workflows/dist-diff-comment.yml @@ -0,0 +1,85 @@ +name: dist-diff comment + +# Posts the dist-diff report as a single sticky PR comment, edited in place on +# every push. Split from dist-diff.yml (workflow_run) so the report job stays +# unprivileged and fork PRs get the same comment as internal ones. +# +# This is the only privileged part of dist-diff. It treats the artifact as +# data: the PR number is verified against the run's head SHA before posting, +# and the report body is only ever posted as comment text. + +on: + workflow_run: + workflows: [dist-diff] + types: [completed] + +permissions: + pull-requests: write + +jobs: + comment: + # No conclusion gate: a failed self-check still uploads a (warning-stamped) + # report, and the PR should get its comment either way. Runs where the + # report step itself died leave no artifact — the download step's outcome + # check below skips the comment quietly in that case. + if: github.event.workflow_run.event == 'pull_request' + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Download the report artifact + id: download + continue-on-error: true + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: dist-diff-report + path: dist-diff-report + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Upsert the sticky comment + if: steps.download.outcome == 'success' + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const fs = require('fs'); + const marker = ''; + + const prNumber = parseInt(fs.readFileSync('dist-diff-report/pr-number', 'utf8').trim(), 10); + if (!Number.isInteger(prNumber)) throw new Error('invalid PR number in artifact'); + + // The artifact comes from an unprivileged run of untrusted code: + // only post to the PR whose head produced this exact run. + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + if (pr.head.sha !== context.payload.workflow_run.head_sha) { + throw new Error('PR number in artifact does not match the run head SHA'); + } + + let body = `${marker}\n${fs.readFileSync('dist-diff-report/comment.md', 'utf8')}`; + if (body.length > 65000) body = `${body.slice(0, 65000)}\n…truncated — see the workflow summary.`; + + const comments = await github.paginate(github.rest.issues.listComments, { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + per_page: 100, + }); + const existing = comments.find(c => c.body?.startsWith(marker)); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body, + }); + } diff --git a/.github/workflows/dist-diff.yml b/.github/workflows/dist-diff.yml new file mode 100644 index 00000000..9697af4d --- /dev/null +++ b/.github/workflows/dist-diff.yml @@ -0,0 +1,112 @@ +name: dist-diff + +# Shows reviewers what a PR changes in the BUILT artifacts (dist/) — the +# skill menu the wizard reads, the marketplace plugins, the MCP manifest — +# which a source diff of configs/markdown can't show. Runs unprivileged; +# the sticky PR comment is posted by dist-diff-comment.yml (workflow_run), +# so fork PRs get the same report as internal ones. +# +# Normalization guarantee: dist-diff reports are only trustworthy if two +# builds of the same ref produce an empty normalized diff. That self-check +# runs on every push to main (catches environment drift) and on PRs that +# touch the build pipeline or dependencies (the only changes that can +# introduce new nondeterminism) — content PRs can never go red here. +# A failed self-check still renders and uploads the report (stamped with a +# warning) before failing the job, so the PR never silently loses its comment. + +on: + pull_request: + branches: [main] + push: + branches: [main] + +permissions: + contents: read + +jobs: + dist-diff: + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - name: Checkout repository + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + with: + fetch-depth: 0 + + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 + with: + node-version: lts/* + + - uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4.3.0 + with: + version: latest + + - name: Install dependencies + run: pnpm install + + - name: Scope the run + id: scope + run: | + if [ "${{ github.event_name }}" = "push" ]; then + echo "self-check=true" >> "$GITHUB_OUTPUT" + else + MERGE_BASE=$(git merge-base "origin/${{ github.base_ref }}" HEAD) + echo "merge-base=$MERGE_BASE" >> "$GITHUB_OUTPUT" + if git diff --name-only "$MERGE_BASE"...HEAD | grep -qE '^(scripts/|package\.json$|pnpm-lock\.yaml$|\.github/workflows/dist-diff)'; then + echo "self-check=true" >> "$GITHUB_OUTPUT" + else + echo "self-check=false" >> "$GITHUB_OUTPUT" + fi + fi + + - name: Build head + # Also primes .docs-cache, which every later build in this job shares — + # so upstream doc changes between builds cannot leak into the diff. + run: pnpm build + + - name: Self-check — two builds of the same ref must normalize identically + id: selfcheck + if: steps.scope.outputs.self-check == 'true' + # Deferred failure: the report steps below still run, then the last + # step turns a failed self-check into a red job. + continue-on-error: true + run: | + mv dist dist-head-first + pnpm build + pnpm --silent diff --exit-code dist-head-first dist + rm -rf dist-head-first + + - name: Build merge-base and render the report + if: github.event_name == 'pull_request' + run: | + git worktree add /tmp/base "${{ steps.scope.outputs.merge-base }}" + ln -s "$PWD/node_modules" /tmp/base/node_modules + mkdir -p .docs-cache + ln -s "$PWD/.docs-cache" /tmp/base/.docs-cache + (cd /tmp/base && pnpm build) + + RUN_URL="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" + WARN="" + if [ "${{ steps.selfcheck.outcome }}" = "failure" ]; then + WARN="> ⚠️ **Self-check failed on this run** — two builds of the same ref differ after normalization, so this report may contain noise. See the job log." + fi + + mkdir -p dist-diff-report + { [ -n "$WARN" ] && printf '%s\n\n' "$WARN"; \ + pnpm --silent diff /tmp/base/dist dist --format comment --summary-url "$RUN_URL"; } > dist-diff-report/comment.md + { [ -n "$WARN" ] && printf '%s\n\n' "$WARN"; \ + pnpm --silent diff /tmp/base/dist dist --format full; } >> "$GITHUB_STEP_SUMMARY" + echo "${{ github.event.pull_request.number }}" > dist-diff-report/pr-number + + - name: Upload report for the comment workflow + if: github.event_name == 'pull_request' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: dist-diff-report + path: dist-diff-report/ + + - name: Fail the job on a failed self-check + if: steps.selfcheck.outcome == 'failure' + run: | + echo "::error::dist-diff self-check failed: the build is nondeterministic. Add a normalization rule in scripts/lib/dist-diff.js for the file(s) named above." + exit 1 diff --git a/package.json b/package.json index 6fe272a0..7503aaf3 100644 --- a/package.json +++ b/package.json @@ -7,6 +7,7 @@ "scripts": { "build": "node scripts/build.js", "dev": "node scripts/dev-server.js", + "diff": "node scripts/diff-dist.js", "visual-dags": "node scripts/visual-dags.js", "test:plugins": "vitest run scripts/plugins/tests", "test:plugins:watch": "vitest scripts/plugins/tests", @@ -19,6 +20,8 @@ "devDependencies": { "archiver": "^7.0.1", "chokidar": "^4.0.3", + "diff": "9.0.0", + "fflate": "0.8.3", "vitest": "^2.0.0" }, "engines": { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7bb3d4d5..0785b136 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -27,6 +27,12 @@ importers: chokidar: specifier: ^4.0.3 version: 4.0.3 + diff: + specifier: 9.0.0 + version: 9.0.0 + fflate: + specifier: 0.8.3 + version: 0.8.3 vitest: specifier: ^2.0.0 version: 2.1.9 @@ -233,66 +239,79 @@ packages: resolution: {integrity: sha512-E8jKK87uOvLrrLN28jnAAAChNq5LeCd2mGgZF+fGF5D507WlG/Noct3lP/QzQ6MrqJ5BCKNwI9ipADB6jyiq2A==} cpu: [arm] os: [linux] + libc: [glibc] '@rollup/rollup-linux-arm-musleabihf@4.56.0': resolution: {integrity: sha512-jQosa5FMYF5Z6prEpTCCmzCXz6eKr/tCBssSmQGEeozA9tkRUty/5Vx06ibaOP9RCrW1Pvb8yp3gvZhHwTDsJw==} cpu: [arm] os: [linux] + libc: [musl] '@rollup/rollup-linux-arm64-gnu@4.56.0': resolution: {integrity: sha512-uQVoKkrC1KGEV6udrdVahASIsaF8h7iLG0U0W+Xn14ucFwi6uS539PsAr24IEF9/FoDtzMeeJXJIBo5RkbNWvQ==} cpu: [arm64] os: [linux] + libc: [glibc] '@rollup/rollup-linux-arm64-musl@4.56.0': resolution: {integrity: sha512-vLZ1yJKLxhQLFKTs42RwTwa6zkGln+bnXc8ueFGMYmBTLfNu58sl5/eXyxRa2RarTkJbXl8TKPgfS6V5ijNqEA==} cpu: [arm64] os: [linux] + libc: [musl] '@rollup/rollup-linux-loong64-gnu@4.56.0': resolution: {integrity: sha512-FWfHOCub564kSE3xJQLLIC/hbKqHSVxy8vY75/YHHzWvbJL7aYJkdgwD/xGfUlL5UV2SB7otapLrcCj2xnF1dg==} cpu: [loong64] os: [linux] + libc: [glibc] '@rollup/rollup-linux-loong64-musl@4.56.0': resolution: {integrity: sha512-z1EkujxIh7nbrKL1lmIpqFTc/sr0u8Uk0zK/qIEFldbt6EDKWFk/pxFq3gYj4Bjn3aa9eEhYRlL3H8ZbPT1xvA==} cpu: [loong64] os: [linux] + libc: [musl] '@rollup/rollup-linux-ppc64-gnu@4.56.0': resolution: {integrity: sha512-iNFTluqgdoQC7AIE8Q34R3AuPrJGJirj5wMUErxj22deOcY7XwZRaqYmB6ZKFHoVGqRcRd0mqO+845jAibKCkw==} cpu: [ppc64] os: [linux] + libc: [glibc] '@rollup/rollup-linux-ppc64-musl@4.56.0': resolution: {integrity: sha512-MtMeFVlD2LIKjp2sE2xM2slq3Zxf9zwVuw0jemsxvh1QOpHSsSzfNOTH9uYW9i1MXFxUSMmLpeVeUzoNOKBaWg==} cpu: [ppc64] os: [linux] + libc: [musl] '@rollup/rollup-linux-riscv64-gnu@4.56.0': resolution: {integrity: sha512-in+v6wiHdzzVhYKXIk5U74dEZHdKN9KH0Q4ANHOTvyXPG41bajYRsy7a8TPKbYPl34hU7PP7hMVHRvv/5aCSew==} cpu: [riscv64] os: [linux] + libc: [glibc] '@rollup/rollup-linux-riscv64-musl@4.56.0': resolution: {integrity: sha512-yni2raKHB8m9NQpI9fPVwN754mn6dHQSbDTwxdr9SE0ks38DTjLMMBjrwvB5+mXrX+C0npX0CVeCUcvvvD8CNQ==} cpu: [riscv64] os: [linux] + libc: [musl] '@rollup/rollup-linux-s390x-gnu@4.56.0': resolution: {integrity: sha512-zhLLJx9nQPu7wezbxt2ut+CI4YlXi68ndEve16tPc/iwoylWS9B3FxpLS2PkmfYgDQtosah07Mj9E0khc3Y+vQ==} cpu: [s390x] os: [linux] + libc: [glibc] '@rollup/rollup-linux-x64-gnu@4.56.0': resolution: {integrity: sha512-MVC6UDp16ZSH7x4rtuJPAEoE1RwS8N4oK9DLHy3FTEdFoUTCFVzMfJl/BVJ330C+hx8FfprA5Wqx4FhZXkj2Kw==} cpu: [x64] os: [linux] + libc: [glibc] '@rollup/rollup-linux-x64-musl@4.56.0': resolution: {integrity: sha512-ZhGH1eA4Qv0lxaV00azCIS1ChedK0V32952Md3FtnxSqZTBTd6tgil4nZT5cU8B+SIw3PFYkvyR4FKo2oyZIHA==} cpu: [x64] os: [linux] + libc: [musl] '@rollup/rollup-openbsd-x64@4.56.0': resolution: {integrity: sha512-O16XcmyDeFI9879pEcmtWvD/2nyxR9mF7Gs44lf1vGGx8Vg2DRNx11aVXBEqOQhWb92WN4z7fW/q4+2NYzCbBA==} @@ -491,6 +510,10 @@ packages: resolution: {integrity: sha512-h5k/5U50IJJFpzfL6nO9jaaumfjO/f2NjK/oYB2Djzm4p9L+3T9qWpZqZ2hAbLPuuYq9wrU08WQyBTL5GbPk5Q==} engines: {node: '>=6'} + diff@9.0.0: + resolution: {integrity: sha512-svtcdpS8CgJyqAjEQIXdb3OjhFVVYjzGAPO8WGCmRbrml64SPw/jJD4GoE98aR7r25A0XcgrK3F02yw9R/vhQw==} + engines: {node: '>=0.3.1'} + eastasianwidth@0.2.0: resolution: {integrity: sha512-I88TYZWc9XiYHRQ4/3c5rjjfgkjhLyW2luGIheGERbNQ6OY7yTybanSpDXZa8y7VUP9YmDcYa+eyq4ca7iLqWA==} @@ -541,6 +564,9 @@ packages: fast-sha256@1.3.0: resolution: {integrity: sha512-n11RGP/lrWEFI/bWdygLxhI+pVeo1ZYIVwvvPkW7azl/rOy+F3HYRZ2K5zeE9mmkhQppyv9sQFx0JM9UabnpPQ==} + fflate@0.8.3: + resolution: {integrity: sha512-tbZNuJrLwGUp3zshBtdy4W+ORxZuIh8a5ilyIEQDC5rY1f3U20JMry0Ll3WBzU58EZKsEuJFXhb5gwv8CsPvgA==} + foreground-child@3.3.1: resolution: {integrity: sha512-gIXjKqtFuWEgzFRJA9WCQeSJLZDjgJUOMCMzxtvFq/37KojM1BFGufqsCy0r4qSQmYLsZYMeyRqzIWOMup03sw==} engines: {node: '>=14'} @@ -552,6 +578,7 @@ packages: glob@10.5.0: resolution: {integrity: sha512-DfXN8DfhJ7NH3Oe7cFmu3NCu1wKbkReJ8TorzSAFbSKrlNaQSKfIzqYqVY8zlbs2NLBbWpRiU52GX2PbaBVNkg==} + deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me hasBin: true graceful-fs@4.2.11: @@ -1212,6 +1239,8 @@ snapshots: deep-eql@5.0.2: {} + diff@9.0.0: {} + eastasianwidth@0.2.0: {} emoji-regex@8.0.0: {} @@ -1272,6 +1301,8 @@ snapshots: fast-sha256@1.3.0: {} + fflate@0.8.3: {} + foreground-child@3.3.1: dependencies: cross-spawn: 7.0.6 diff --git a/scripts/diff-dist.js b/scripts/diff-dist.js new file mode 100644 index 00000000..b72f9716 --- /dev/null +++ b/scripts/diff-dist.js @@ -0,0 +1,90 @@ +#!/usr/bin/env node +/** + * dist-diff CLI — show what a change does to the built artifacts in dist/. + * + * Usage: + * node scripts/diff-dist.js [options] + * node scripts/diff-dist.js --against [options] + * + * Options: + * --format comment|full comment: sticky-comment report (default); full: complete listing + * --summary-url URL append a "Full report" link to the comment format + * --exit-code exit 1 when the normalized diff is non-empty (like `git diff --exit-code`); + * CI uses this to assert two builds of the same ref are identical + * + * --against builds the given ref in a temporary worktree and rebuilds the + * local dist/, both sharing the current .docs-cache — so upstream doc drift + * cannot appear in the diff (same guarantee the CI job gets by running both + * builds in one job). + */ +import { execFileSync } from 'child_process'; +import { existsSync, mkdtempSync, rmSync, symlinkSync } from 'fs'; +import { join, resolve } from 'path'; +import { tmpdir } from 'os'; +import { diffDistTrees, renderComment, renderFull } from './lib/dist-diff.js'; + +const options = { positional: [] }; +const argv = process.argv.slice(2); +for (let i = 0; i < argv.length; i++) { + switch (argv[i]) { + case '--against': options.against = argv[++i]; break; + case '--format': options.format = argv[++i]; break; + case '--summary-url': options.summaryUrl = argv[++i]; break; + case '--exit-code': options.exitCode = true; break; + default: options.positional.push(argv[i]); + } +} + +const repoRoot = resolve(import.meta.dirname, '..'); + +function buildRefDist(ref) { + const worktree = mkdtempSync(join(tmpdir(), 'dist-diff-base-')); + execFileSync('git', ['worktree', 'add', '--detach', worktree, ref], { cwd: repoRoot, stdio: 'inherit' }); + try { + symlinkSync(join(repoRoot, 'node_modules'), join(worktree, 'node_modules')); + // Share the docs cache so both sides see identical upstream doc bytes. + if (existsSync(join(repoRoot, '.docs-cache'))) { + symlinkSync(join(repoRoot, '.docs-cache'), join(worktree, '.docs-cache')); + } + execFileSync('node', ['scripts/build.js'], { cwd: worktree, stdio: 'inherit' }); + return { worktree, dist: join(worktree, 'dist') }; + } catch (err) { + rmSync(worktree, { recursive: true, force: true }); + execFileSync('git', ['worktree', 'prune'], { cwd: repoRoot }); + throw err; + } +} + +let beforeDir, afterDir, cleanup; +if (options.against) { + console.error(`Building ${options.against} in a temporary worktree...`); + const base = buildRefDist(options.against); + beforeDir = base.dist; + afterDir = join(repoRoot, 'dist'); + cleanup = () => { + rmSync(base.worktree, { recursive: true, force: true }); + execFileSync('git', ['worktree', 'prune'], { cwd: repoRoot }); + }; + // Always rebuild — a dist/ left over from another ref would silently + // poison the comparison. The base build above primed .docs-cache. + console.error('Rebuilding local dist/...'); + execFileSync('node', ['scripts/build.js'], { cwd: repoRoot, stdio: 'inherit' }); +} else { + [beforeDir, afterDir] = options.positional; + if (!beforeDir || !afterDir) { + console.error('Usage: diff-dist.js | --against [--format comment|full] [--summary-url URL] [--exit-code]'); + process.exit(2); + } +} + +try { + const model = await diffDistTrees(beforeDir, afterDir); + if (options.format === 'full') { + console.log(renderFull(model)); + } else { + console.log(renderComment(model, { fullReportUrl: options.summaryUrl })); + } + if (options.exitCode && model.changes.length) process.exitCode = 1; +} finally { + cleanup?.(); +} diff --git a/scripts/lib/dist-diff.js b/scripts/lib/dist-diff.js new file mode 100644 index 00000000..e3cc80d2 --- /dev/null +++ b/scripts/lib/dist-diff.js @@ -0,0 +1,542 @@ +/** + * dist-diff — normalized differ over two built dist/ trees. + * + * Design invariants (see the dist-diff PR/issue for the full rationale): + * - Totality: every file under both trees is compared. Unknown file types + * degrade to byte comparison — a future artifact type can at worst add a + * noisy line to the report, never be silently skipped. + * - Normalization is a small set of NAMED rules, nothing broader: + * ZIPs are compared by entry contents (the archive format embeds mtimes, + * which differ on every build), and JSON files are compared by parsed + * value with known build-stamp fields removed. + * - Determinism guard: `npm run diff -- --self-check A B` asserts two builds + * of the same ref produce an empty normalized diff, so any new + * nondeterminism in the build fails loudly instead of eroding the report. + */ +import { readdirSync, readFileSync } from 'fs'; +import { join } from 'path'; +import { createHash } from 'crypto'; +import { unzipSync } from 'fflate'; +import { structuredPatch } from 'diff'; + +/** + * Read a ZIP buffer into { entryName: contentBuffer }. fflate is the same + * library the PostHog MCP server uses to unzip these archives. Entry mtimes + * and attributes are dropped here on purpose: they are exactly the noise this + * tool normalizes away. + */ +export function readZipEntries(buf) { + const entries = {}; + for (const [name, data] of Object.entries(unzipSync(buf))) { + if (!name.endsWith('/')) entries[name] = Buffer.from(data.buffer, data.byteOffset, data.byteLength); + } + return entries; +} + +/** + * True when two JSON buffers are equal after removing build-stamp fields. + * buildTimestamp (build-phases.js writeManifestAndMenu) is the build's only + * per-run stamp today; add fields here if another one appears. + */ +function jsonEqualIgnoringStamps(a, b) { + let pa, pb; + try { + pa = JSON.parse(a.toString('utf8')); + pb = JSON.parse(b.toString('utf8')); + } catch { + return false; // unparseable JSON falls back to the byte comparison verdict + } + if (pa && typeof pa === 'object') delete pa.buildTimestamp; + if (pb && typeof pb === 'object') delete pb.buildTimestamp; + return JSON.stringify(pa) === JSON.stringify(pb); +} + +/** + * True when two buffers are equal after normalization for their file type. + * Applies recursively inside zips, so aggregate archives (zips of zips plus a + * stamped manifest) normalize the same way the tree does. + */ +function normalizedEqual(name, a, b) { + if (a.equals(b)) return true; + if (name.endsWith('.json')) return jsonEqualIgnoringStamps(a, b); + if (name.endsWith('.zip')) return zipInnerChanges(a, b).length === 0; + return false; +} + +/** + * Fingerprint of a zip's inner delta: same changed entry names with the same + * before/after bytes → same hash. Lets the report group fan-out (one shared + * doc rebuilt into dozens of variant zips) into a single line. + */ +// Length-framed so contents can't collide with each other or with absence. +function hashFrame(hash, buf) { + hash.update(String(buf ? buf.length : -1)).update('\0').update(buf ?? '').update('\0'); +} + +function zipDeltaHash(a, b, innerChanges) { + const ea = readZipEntries(a); + const eb = readZipEntries(b); + const hash = createHash('sha256'); + for (const name of innerChanges) { + hash.update(name).update('\0'); + hashFrame(hash, ea[name]); + hashFrame(hash, eb[name]); + } + return hash.digest('hex'); +} + +/** Fingerprint of a plain file's before→after delta, for fan-out grouping. */ +function fileDeltaHash(a, b) { + const hash = createHash('sha256'); + hashFrame(hash, a); + hashFrame(hash, b); + return hash.digest('hex'); +} + +/** Inner entry names that differ (normalized) between two zip buffers. */ +function zipInnerChanges(a, b) { + const ea = readZipEntries(a); + const eb = readZipEntries(b); + const names = [...new Set([...Object.keys(ea), ...Object.keys(eb)])].sort(); + return names.filter(n => !ea[n] || !eb[n] || !normalizedEqual(n, ea[n], eb[n])); +} + +/** Recursively list files under dir as tree-relative paths. */ +function listFiles(dir, prefix = '') { + const out = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const rel = prefix ? `${prefix}/${entry.name}` : entry.name; + if (entry.isDirectory()) out.push(...listFiles(join(dir, entry.name), rel)); + else out.push(rel); + } + return out; +} + +/** + * Diff two dist trees. Returns { changes: [{ path, kind }] } where kind is + * 'added' | 'removed' | 'changed'. + */ +export async function diffDistTrees(beforeDir, afterDir) { + const beforeFiles = new Set(listFiles(beforeDir)); + const afterFiles = new Set(listFiles(afterDir)); + const changes = []; + + for (const path of [...new Set([...beforeFiles, ...afterFiles])].sort()) { + if (!beforeFiles.has(path)) { + changes.push({ path, kind: 'added' }); + } else if (!afterFiles.has(path)) { + changes.push({ path, kind: 'removed' }); + } else { + const a = readFileSync(join(beforeDir, path)); + const b = readFileSync(join(afterDir, path)); + if (normalizedEqual(path, a, b)) continue; + if (path.endsWith('.zip')) { + const innerChanges = zipInnerChanges(a, b); + changes.push({ path, kind: 'changed', innerChanges, deltaHash: zipDeltaHash(a, b, innerChanges) }); + } else { + changes.push({ path, kind: 'changed', deltaHash: fileDeltaHash(a, b) }); + } + } + } + return { beforeDir, afterDir, changes }; +} + +/** Review surfaces, ordered by user impact. classify() returns the first match. */ +const SURFACES = [ + { key: 'wizard', title: 'Wizard surface', match: p => p === 'skills/skill-menu.json' }, + { + key: 'skill-content', + title: 'Skill content', + match: p => p.startsWith('skills/') && p !== 'skills/manifest.json', + }, + { key: 'marketplace', title: 'Marketplace', match: p => p.startsWith('marketplace/') }, + { key: 'agents', title: 'Agents', match: p => p.startsWith('agents/') }, + { + key: 'mcp', + title: 'MCP manifest', + match: p => p === 'skills/manifest.json' || p === 'skills-mcp-resources.zip', + }, + { key: 'mirror', title: 'Skills-repo mirror', match: p => p === 'push-manifest.json' }, + { key: 'other', title: 'Other artifacts', match: () => true }, +]; + +function classify(path) { + return SURFACES.find(s => s.match(path)).key; +} + +function bucketBySurface(model) { + const bySurface = new Map(SURFACES.map(s => [s.key, []])); + for (const change of model.changes) bySurface.get(classify(change.path)).push(change); + return bySurface; +} + +// Report bodies are ```diff fenced blocks: a leading '+' renders green, +// '-' renders red, and any other first character (we use '~' and ' ') gray. +const STATUS = { added: '+', removed: '-', changed: '~' }; +const MAX_INLINE_INNER = 5; + +/** Path prefix implied by a surface's title, stripped inside its block. */ +const SURFACE_STRIP = { 'skill-content': 'skills/', marketplace: 'marketplace/', agents: 'agents/' }; + +function stripSurfacePrefix(surfaceKey, path) { + const prefix = SURFACE_STRIP[surfaceKey]; + return prefix && path.startsWith(prefix) ? path.slice(prefix.length) : path; +} + +/** + * Render changes as an indented directory tree (single-child directory chains + * collapsed, `tree`-style). A directory whose descendants all share one status + * carries that status char itself, so a freshly added subtree is solid green. + */ +function treeLines(items, { capInner = true } = {}) { + const root = { dirs: new Map(), files: [] }; + for (const { rel, change } of items) { + const segs = rel.split('/'); + let node = root; + for (const seg of segs.slice(0, -1)) { + if (!node.dirs.has(seg)) node.dirs.set(seg, { dirs: new Map(), files: [] }); + node = node.dirs.get(seg); + } + node.files.push({ name: segs.at(-1), change }); + } + const kindsOf = node => [ + ...node.files.map(f => f.change.kind), + ...[...node.dirs.values()].flatMap(kindsOf), + ]; + const lines = []; + const emit = (node, depth) => { + for (const [name, start] of [...node.dirs].sort(([a], [b]) => a.localeCompare(b))) { + let label = name; + let child = start; + while (child.files.length === 0 && child.dirs.size === 1) { + const [[next, sub]] = child.dirs; + label = `${label}/${next}`; + child = sub; + } + const kinds = new Set(kindsOf(child)); + const status = kinds.size === 1 ? STATUS[[...kinds][0]] : ' '; + // A directory holding exactly one file collapses onto one line. + if (child.dirs.size === 0 && child.files.length === 1) { + const f = child.files[0]; + const inner = f.change.innerChanges?.length + ? ` — ${capInner ? capList(f.change.innerChanges) : f.change.innerChanges.join(', ')}` + : ''; + lines.push(`${STATUS[f.change.kind]} ${' '.repeat(depth)}${label}/${f.name}${inner}`); + continue; + } + lines.push(`${status} ${' '.repeat(depth)}${label}/`); + emit(child, depth + 1); + } + for (const f of [...node.files].sort((a, b) => a.name.localeCompare(b.name))) { + const inner = f.change.innerChanges?.length + ? ` — ${capInner ? capList(f.change.innerChanges) : f.change.innerChanges.join(', ')}` + : ''; + lines.push(`${STATUS[f.change.kind]} ${' '.repeat(depth)}${f.name}${inner}`); + } + }; + emit(root, 0); + return lines; +} + +/** Added/removed cliEntries between the two skill menus. */ +function cliEntryDelta(model) { + const read = dir => { + try { + return JSON.parse(readFileSync(join(dir, 'skills/skill-menu.json'), 'utf8')).cliEntries ?? []; + } catch { + return []; + } + }; + const key = e => [e.skillId, e.parentCommand, e.command].join('|'); + const beforeEntries = new Map(read(model.beforeDir).map(e => [key(e), e])); + const afterEntries = new Map(read(model.afterDir).map(e => [key(e), e])); + return { + added: [...afterEntries.values()].filter(e => !beforeEntries.has(key(e))), + removed: [...beforeEntries.values()].filter(e => !afterEntries.has(key(e))), + modified: [...afterEntries.values()].filter(e => { + const prev = beforeEntries.get(key(e)); + return prev && JSON.stringify(prev) !== JSON.stringify(e); + }), + }; +} + +function describeEntry(e) { + const cmd = e.command ? `, command: ${e.parentCommand ? `${e.parentCommand} ` : ''}${e.command}` : ''; + return `${e.skillId} (role: ${e.role}${cmd})`; +} + +/** Diff-block lines for the wizard surface: semantic cliEntry delta. */ +function wizardBlock(model) { + const { added, removed, modified } = cliEntryDelta(model); + const block = [ + ...added.map(e => `+ cliEntry ${describeEntry(e)}`), + ...removed.map(e => `- cliEntry ${describeEntry(e)}`), + ...modified.map(e => `~ cliEntry ${e.skillId} updated`), + ]; + return block.length ? block : ['~ skill-menu.json changed (no cliEntry changes)']; +} + +/** + * Render the sticky-comment report: surfaces ordered by user impact, explicit + * "unchanged" assertions, hard line budget. + */ +export function renderComment(model, { fullReportUrl } = {}) { + const bySurface = bucketBySurface(model); + const lines = [`## dist-diff — ${model.changes.length} artifact change(s)`]; + for (const surface of SURFACES) { + const changes = bySurface.get(surface.key); + if (surface.key === 'other' && changes.length === 0) continue; + // Blank line between blocks — without it, GFM folds a line that + // follows a list item into that item (lazy continuation). + lines.push(''); + if (changes.length === 0) { + lines.push(`**${surface.title}** ✓ unchanged`); + continue; + } + lines.push(`**${surface.title}**`); + const { block, after } = surface.key === 'wizard' + ? { block: wizardBlock(model), after: [] } + : surfaceBlock(changes, surface.key); + lines.push('```diff', ...block, '```', ...after); + } + + if (fullReportUrl) lines.push('', `[Full report](${fullReportUrl})`); + + const BUDGET = 40; + if (lines.length > BUDGET) { + const link = fullReportUrl ? lines[lines.length - 1] : null; + const hidden = lines.length - (BUDGET - 1); + lines.length = BUDGET - (link ? 2 : 1); + // Never truncate inside an open ```diff fence — close it first. + if (lines.filter(l => l.startsWith('```')).length % 2 === 1) { + lines[lines.length - 1] = '```'; + } + lines.push(`…${hidden} more line(s) — see the full report in the workflow summary.`); + if (link) lines.push(link); + } + return lines.join('\n'); +} + +// Content-hunk limits for the full report: the step summary allows ~1MB, but +// a diff nobody can scan is worse than a truncated one. +const MAX_HUNK_LINES = 120; +const MAX_LINE_CHARS = 300; +const MAX_DIFF_BYTES = 300 * 1024; + +function isText(buf) { + return !buf.subarray(0, 8192).includes(0); +} + +function hunkLines(oldStr, newStr) { + const patch = structuredPatch('a', 'b', oldStr, newStr, '', '', { context: 3 }); + const lines = []; + for (const h of patch.hunks) { + lines.push(`@@ -${h.oldStart},${h.oldLines} +${h.newStart},${h.newLines} @@`); + lines.push(...h.lines); + } + return lines.map(l => (l.length > MAX_LINE_CHARS ? `${l.slice(0, MAX_LINE_CHARS)}…` : l)); +} + +function capHunks(lines) { + if (lines.length <= MAX_HUNK_LINES) return lines; + return [...lines.slice(0, MAX_HUNK_LINES), `… ${lines.length - MAX_HUNK_LINES} more diff line(s) omitted`]; +} + +/** Unified-diff lines for one changed artifact; zips diff per inner entry. */ +function changeHunks(model, change) { + const a = readFileSync(join(model.beforeDir, change.path)); + const b = readFileSync(join(model.afterDir, change.path)); + if (a.length > MAX_DIFF_BYTES || b.length > MAX_DIFF_BYTES) { + return [`(file too large to diff: ${Math.round(Math.max(a.length, b.length) / 1024)} KB)`]; + } + if (change.path.endsWith('.zip')) { + const ea = readZipEntries(a); + const eb = readZipEntries(b); + const lines = []; + for (const name of change.innerChanges ?? []) { + lines.push(`# ${name}`); + if (name.endsWith('.zip')) { + lines.push('(nested archive — its contents are diffed as individual skill zips)'); + continue; + } + const ia = ea[name] ?? Buffer.alloc(0); + const ib = eb[name] ?? Buffer.alloc(0); + if (!isText(ia) || !isText(ib)) { + lines.push('(binary entry)'); + continue; + } + lines.push(...hunkLines(ia.toString('utf8'), ib.toString('utf8'))); + } + return capHunks(lines); + } + if (!isText(a) || !isText(b)) return ['(binary file)']; + return capHunks(hunkLines(a.toString('utf8'), b.toString('utf8'))); +} + +/** + * Content sections for a surface's changed artifacts, one per unique delta: + * an identical-delta fan-out shows its hunk once, titled with the member count. + */ +function contentSections(model, changes) { + const groups = new Map(); + for (const change of changes) { + if (change.kind !== 'changed') continue; + const key = change.deltaHash + ? (change.innerChanges ? `zip:${change.innerChanges.join('|')}#${change.deltaHash}` : `file:#${change.deltaHash}`) + : `single:${change.path}`; + if (!groups.has(key)) groups.set(key, []); + groups.get(key).push(change); + } + return [...groups.values()].map(members => ({ + title: members.length > 1 + ? `${members.length} files — identical delta (${commonPathSuffix(members.map(m => m.path)) || members[0].path})` + : members[0].path, + hunks: changeHunks(model, members[0]), + })); +} + +/** Render the full report: every change listed; content hunks size-budgeted. */ +export function renderFull(model) { + const bySurface = bucketBySurface(model); + const lines = [`# dist-diff full report — ${model.changes.length} artifact change(s)`, '']; + let hunkBudget = 4000; + for (const surface of SURFACES) { + const changes = bySurface.get(surface.key); + if (surface.key === 'other' && changes.length === 0) continue; + lines.push(`## ${surface.title}`, ''); + if (changes.length === 0) { + lines.push('✓ unchanged', ''); + continue; + } + if (surface.key === 'wizard') { + lines.push('```diff', ...wizardBlock(model), '```', ''); + } + // Complete tree of every change — nothing grouped, nothing capped. + lines.push('```diff'); + lines.push(...treeLines( + changes.map(c => ({ rel: stripSurfacePrefix(surface.key, c.path), change: c })), + { capInner: false }, + )); + lines.push('```', ''); + // Content-level hunks, one per unique delta, under a report-wide + // budget — the step summary rejects anything over 1MB. + const sections = contentSections(model, changes); + let omitted = 0; + for (const { title, hunks } of sections) { + if (hunkBudget <= 0) { omitted++; continue; } + hunkBudget -= hunks.length; + lines.push(`
${title}`, '', '```diff', ...hunks, '```', '', '
', ''); + } + if (omitted) { + lines.push(`_${omitted} more content diff(s) omitted for size — run \`npm run diff\` locally for the complete set._`, ''); + } + } + return lines.join('\n'); +} + +/** + * One surface's diff-block content plus its
member lists (which + * are HTML and must live outside the code fence). Fan-out collapses in three + * ways: + * - changed files/zips with an IDENTICAL delta → one "identical delta" line + * - ≥4 changed items touching the same file name / inner-file set with + * differing contents (a skill rebuilt across variants) → one line + * - ≥4 files added/removed in the same directory (a new variant) → one line + * Everything below the thresholds is rendered as a directory tree. + */ +const GROUP_MIN = 4; + +function surfaceBlock(changes, surfaceKey) { + const block = []; + const after = []; + const singles = []; + const exactGroups = new Map(); + const dirBuckets = new Map(); + for (const change of changes) { + if (change.kind === 'changed' && change.deltaHash) { + const key = change.innerChanges + ? `zip:${change.innerChanges.join('|')}#${change.deltaHash}` + : `file:#${change.deltaHash}`; + if (!exactGroups.has(key)) exactGroups.set(key, []); + exactGroups.get(key).push(change); + } else { + const dir = change.path.slice(0, change.path.lastIndexOf('/') + 1); + const key = `${change.kind}:${dir}`; + if (!dirBuckets.has(key)) dirBuckets.set(key, []); + dirBuckets.get(key).push(change); + } + } + + // A new variant lands as one directory tree; fold each bucket into the + // shallowest same-kind bucket whose directory contains it. + const dirKeys = [...dirBuckets.keys()].sort((a, b) => a.length - b.length); + for (const key of dirKeys) { + if (!dirBuckets.has(key)) continue; + const ancestor = dirKeys.find(k => k !== key && dirBuckets.has(k) && key.startsWith(k)); + if (ancestor) { + dirBuckets.get(ancestor).push(...dirBuckets.get(key)); + dirBuckets.delete(key); + } + } + for (const [key, members] of dirBuckets) { + if (members.length >= GROUP_MIN) { + const dir = stripSurfacePrefix(surfaceKey, key.slice(key.indexOf(':') + 1)); + block.push(`${STATUS[members[0].kind]} ${dir} — ${members.length} files ${members[0].kind}`); + after.push(`
${dir} (${members.length} files)${members.map(m => m.path).join(', ')}
`); + } else { + singles.push(...members); + } + } + + // Merge exact-delta groups that share a signature (same inner-file set for + // zips, same file name for plain files) — a rebuild fans out over dozens + // of variants whose contents differ per variant. + const signatureBuckets = new Map(); + for (const members of exactGroups.values()) { + const first = members[0]; + const sig = first.innerChanges ? `zip:${first.innerChanges.join('|')}` : `file:${first.path.split('/').pop()}`; + if (!signatureBuckets.has(sig)) signatureBuckets.set(sig, []); + signatureBuckets.get(sig).push(members); + } + for (const groups of signatureBuckets.values()) { + const total = groups.reduce((n, g) => n + g.length, 0); + const first = groups[0][0]; + let label = null; + if (groups.length === 1 && total > 1) { + label = first.innerChanges + ? `${total} zips changed — identical delta in ${capList(first.innerChanges)}` + : `${total} files changed — identical delta (${commonPathSuffix(groups[0].map(m => m.path)) || 'no common name'})`; + } else if (groups.length > 1 && total >= GROUP_MIN) { + label = first.innerChanges + ? `${total} zips changed — same files touched (${capList(first.innerChanges)}), contents differ per archive` + : `${total} files changed — same file name (${first.path.split('/').pop()}), contents differ`; + } + if (label) { + block.push(`~ ${label}`); + after.push(`
${first.innerChanges ? 'archives' : 'files'} (${total})${groups.flat().map(m => m.path).join(', ')}
`); + } else { + singles.push(...groups.flat()); + } + } + + block.push(...treeLines(singles.map(c => ({ rel: stripSurfacePrefix(surfaceKey, c.path), change: c })))); + return { block, after }; +} + +/** Comma-joined list, capped at MAX_INLINE_INNER with a "+N more" tail. */ +function capList(names) { + if (names.length <= MAX_INLINE_INNER) return names.join(', '); + return [...names.slice(0, MAX_INLINE_INNER), `…+${names.length - MAX_INLINE_INNER} more`].join(', '); +} + +/** Longest common trailing path segments, e.g. shared "references/4-conclude.md". */ +function commonPathSuffix(paths) { + const parts = paths.map(p => p.split('/')); + const suffix = []; + for (let i = 1; ; i++) { + const seg = parts[0][parts[0].length - i]; + if (seg === undefined || !parts.every(p => p[p.length - i] === seg)) break; + suffix.unshift(seg); + } + return suffix.join('/'); +} diff --git a/scripts/lib/tests/dist-diff.test.js b/scripts/lib/tests/dist-diff.test.js new file mode 100644 index 00000000..358da02b --- /dev/null +++ b/scripts/lib/tests/dist-diff.test.js @@ -0,0 +1,331 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdirSync, writeFileSync, mkdtempSync, rmSync } from 'fs'; +import { join, dirname } from 'path'; +import { tmpdir } from 'os'; +import archiver from 'archiver'; + +import { diffDistTrees, renderComment, renderFull } from '../dist-diff.js'; + +/** Write a file tree: { 'a/b.txt': 'content' } */ +function writeTree(baseDir, files) { + for (const [rel, content] of Object.entries(files)) { + const full = join(baseDir, rel); + mkdirSync(dirname(full), { recursive: true }); + writeFileSync(full, content); + } +} + +/** Zip entries { name: content } into a buffer via archiver (the build's writer). */ +function zipBuffer(entries, { date } = {}) { + return new Promise((resolve, reject) => { + const chunks = []; + const archive = archiver('zip', { zlib: { level: 9 } }); + archive.on('data', c => chunks.push(c)); + archive.on('end', () => resolve(Buffer.concat(chunks))); + archive.on('error', reject); + for (const [name, content] of Object.entries(entries)) { + archive.append(content, { name, date: date ?? new Date('2020-01-01T00:00:00Z') }); + } + archive.finalize(); + }); +} + +let before, after; + +beforeEach(() => { + before = mkdtempSync(join(tmpdir(), 'distdiff-before-')); + after = mkdtempSync(join(tmpdir(), 'distdiff-after-')); +}); + +afterEach(() => { + rmSync(before, { recursive: true, force: true }); + rmSync(after, { recursive: true, force: true }); +}); + +describe('diffDistTrees', () => { + it('reports no changes for identical trees', async () => { + const files = { + 'skills/skill-menu.json': '{"cliEntries":[]}', + 'skills/web-analytics.zip': 'placeholder', + }; + writeTree(before, files); + writeTree(after, files); + + const model = await diffDistTrees(before, after); + expect(model.changes).toEqual([]); + }); + + it('filters zip archives whose bytes differ only by entry mtimes', async () => { + const entries = { 'SKILL.md': '# Web analytics', 'references/setup.md': 'steps' }; + writeTree(before, { + 'skills/web-analytics.zip': await zipBuffer(entries, { date: new Date('2020-01-01T00:00:00Z') }), + }); + writeTree(after, { + 'skills/web-analytics.zip': await zipBuffer(entries, { date: new Date('2026-08-09T12:00:00Z') }), + }); + + const model = await diffDistTrees(before, after); + expect(model.changes).toEqual([]); + }); + + it('reports a zip whose entry content changed, naming the inner files', async () => { + writeTree(before, { + 'skills/web-analytics.zip': await zipBuffer({ 'SKILL.md': 'v1', 'references/setup.md': 'same' }), + }); + writeTree(after, { + 'skills/web-analytics.zip': await zipBuffer({ 'SKILL.md': 'v2', 'references/setup.md': 'same' }), + }); + + const model = await diffDistTrees(before, after); + expect(model.changes).toEqual([ + { path: 'skills/web-analytics.zip', kind: 'changed', innerChanges: ['SKILL.md'], deltaHash: expect.any(String) }, + ]); + }); + + it('filters a manifest whose only difference is buildTimestamp, but reports real manifest changes', async () => { + const resource = { id: 'web-analytics', uri: 'posthog://skills/web/web-analytics' }; + writeTree(before, { + 'skills/manifest.json': JSON.stringify({ buildTimestamp: '2026-08-01T00:00:00Z', resources: [resource] }), + }); + writeTree(after, { + 'skills/manifest.json': JSON.stringify({ buildTimestamp: '2026-08-09T00:00:00Z', resources: [resource] }), + }); + expect((await diffDistTrees(before, after)).changes).toEqual([]); + + writeTree(after, { + 'skills/manifest.json': JSON.stringify({ + buildTimestamp: '2026-08-09T00:00:00Z', + resources: [resource, { id: 'new-skill', uri: 'posthog://skills/new/new-skill' }], + }), + }); + expect((await diffDistTrees(before, after)).changes).toEqual([ + { path: 'skills/manifest.json', kind: 'changed', deltaHash: expect.any(String) }, + ]); + }); + + it('reports unknown future artifact types instead of skipping them (totality)', async () => { + writeTree(before, { 'desktop/blob.bin': 'v1' }); + writeTree(after, { 'desktop/blob.bin': 'v2', 'desktop/extra.dat': 'new' }); + + expect((await diffDistTrees(before, after)).changes).toEqual([ + { path: 'desktop/blob.bin', kind: 'changed', deltaHash: expect.any(String) }, + { path: 'desktop/extra.dat', kind: 'added' }, + ]); + }); + + it('normalizes recursively inside aggregate zips (inner manifest stamps, inner zip mtimes)', async () => { + const innerSkill = { 'SKILL.md': 'stable' }; + writeTree(before, { + 'skills-mcp-resources.zip': await zipBuffer({ + 'manifest.json': JSON.stringify({ buildTimestamp: '2026-08-01T00:00:00Z', resources: [] }), + 'web-analytics.zip': await zipBuffer(innerSkill, { date: new Date('2020-01-01T00:00:00Z') }), + }), + }); + writeTree(after, { + 'skills-mcp-resources.zip': await zipBuffer({ + 'manifest.json': JSON.stringify({ buildTimestamp: '2026-08-09T00:00:00Z', resources: [] }), + 'web-analytics.zip': await zipBuffer(innerSkill, { date: new Date('2026-08-09T12:00:00Z') }), + }), + }); + + expect((await diffDistTrees(before, after)).changes).toEqual([]); + }); +}); + +describe('renderComment', () => { + it('renders a cliEntry addition under the wizard surface, with unchanged assertions for the rest', async () => { + const menu = entries => JSON.stringify({ cliEntries: entries }); + const base = [{ skillId: 'audit-events', role: 'command', parentCommand: 'audit', command: 'events' }]; + writeTree(before, { 'skills/skill-menu.json': menu(base) }); + writeTree(after, { + 'skills/skill-menu.json': menu([ + ...base, + { skillId: 'creating-product-tours', role: 'skill' }, + ]), + }); + + const model = await diffDistTrees(before, after); + const comment = renderComment(model); + + expect(comment).toContain('creating-product-tours'); + expect(comment).toMatch(/\+.*creating-product-tours.*skill/); + expect(comment).toMatch(/Skill content.*✓/i); + expect(comment).toMatch(/Marketplace.*✓/i); + expect(comment.split('\n').length).toBeLessThanOrEqual(40); + }); + + it('groups zips sharing an identical inner delta and keeps the comment within budget', async () => { + const beforeFiles = {}; + const afterFiles = {}; + // 45 variants all pick up the same shared-doc change... + for (let i = 0; i < 45; i++) { + const id = `ai-observability-variant-${i}`; + beforeFiles[`skills/${id}.zip`] = await zipBuffer({ 'SKILL.md': `# ${id}`, 'references/auth.md': 'old auth' }); + afterFiles[`skills/${id}.zip`] = await zipBuffer({ 'SKILL.md': `# ${id}`, 'references/auth.md': 'new auth' }); + } + // ...and one unrelated zip changes differently. + beforeFiles['skills/web-analytics.zip'] = await zipBuffer({ 'SKILL.md': 'wa v1' }); + afterFiles['skills/web-analytics.zip'] = await zipBuffer({ 'SKILL.md': 'wa v2' }); + writeTree(before, beforeFiles); + writeTree(after, afterFiles); + + const comment = renderComment(await diffDistTrees(before, after)); + + expect(comment).toMatch(/45 zips.*identical.*references\/auth\.md/i); + expect(comment).toContain('web-analytics.zip'); + expect(comment.split('\n').length).toBeLessThanOrEqual(40); + + const full = renderFull(await diffDistTrees(before, after)); + for (let i = 0; i < 45; i++) expect(full).toContain(`ai-observability-variant-${i}.zip`); + }); + + it('does not group a zip entry changed to the literal "absent" with a removed entry', async () => { + writeTree(before, { + 'skills/a.zip': await zipBuffer({ 'SKILL.md': 'keep', 'doc.md': 'old' }), + 'skills/b.zip': await zipBuffer({ 'SKILL.md': 'keep', 'doc.md': 'old' }), + }); + writeTree(after, { + 'skills/a.zip': await zipBuffer({ 'SKILL.md': 'keep', 'doc.md': 'absent' }), // content change + 'skills/b.zip': await zipBuffer({ 'SKILL.md': 'keep' }), // entry removed + }); + + const model = await diffDistTrees(before, after); + const [a, b] = model.changes; + expect(a.deltaHash).not.toEqual(b.deltaHash); + expect(renderComment(model)).not.toMatch(/2 zips.*identical/i); + }); + + it('groups many zips touching the same inner files with differing contents (skill rebuild)', async () => { + const beforeFiles = {}, afterFiles = {}; + for (const id of ['anthropic', 'openai', 'mistral', 'groq', 'cohere']) { + beforeFiles[`skills/ai-observability-${id}.zip`] = await zipBuffer({ 'SKILL.md': `old ${id}`, 'references/setup.md': `old setup ${id}` }); + afterFiles[`skills/ai-observability-${id}.zip`] = await zipBuffer({ 'SKILL.md': `new ${id}`, 'references/setup.md': `new setup ${id}` }); + } + // Two unrelated zips with the same inner-file signature must stay itemized (below threshold). + for (const id of ['quack', 'omnibus']) { + beforeFiles[`skills/${id}.zip`] = await zipBuffer({ 'SKILL.md': `old ${id}` }); + afterFiles[`skills/${id}.zip`] = await zipBuffer({ 'SKILL.md': `new ${id}` }); + } + writeTree(before, beforeFiles); + writeTree(after, afterFiles); + + const comment = renderComment(await diffDistTrees(before, after)); + + expect(comment).toMatch(/5 zips changed — same files touched.*SKILL\.md, references\/setup\.md/i); + expect(comment).toMatch(/^~ quack\.zip/m); + expect(comment).toMatch(/^~ omnibus\.zip/m); + }); + + it('collapses the new-variant shape: added files group by directory, small identical-delta pairs merge by file name', async () => { + const beforeFiles = {}, afterFiles = {}; + // 5 files added in one new plugin directory... + for (const f of ['SKILL.md', 'references/cli.md', 'references/go.md', 'references/upload.md', 'references/COMMANDMENTS.md']) { + afterFiles[`marketplace/plugins/posthog-uploads/skills/go/${f}`] = `new ${f}`; + } + // ...and 5 variants whose SKILL.md changed identically in their two plugin copies (5 pairs, distinct per variant). + for (const id of ['android', 'ios', 'react', 'node', 'python']) { + for (const loc of ['posthog-all/skills', 'posthog-uploads/skills']) { + beforeFiles[`marketplace/plugins/${loc}/${id}/SKILL.md`] = `old ${id}`; + afterFiles[`marketplace/plugins/${loc}/${id}/SKILL.md`] = `new ${id}`; + } + } + writeTree(before, beforeFiles); + writeTree(after, { ...afterFiles }); + + const comment = renderComment(await diffDistTrees(before, after)); + + expect(comment).toMatch(/^\+ plugins\/posthog-uploads\/skills\/go\/ — 5 files added/im); + expect(comment).toMatch(/10 files changed — same file name \(SKILL\.md\)/i); + expect(comment.split('\n').length).toBeLessThanOrEqual(22); + }); + + it('groups plain files sharing an identical content delta (marketplace fan-out)', async () => { + const beforeFiles = {}, afterFiles = {}; + for (const id of ['android', 'angular', 'django']) { + beforeFiles[`marketplace/plugins/posthog-all/skills/integration-${id}/references/4-conclude.md`] = 'old conclusion'; + afterFiles[`marketplace/plugins/posthog-all/skills/integration-${id}/references/4-conclude.md`] = 'new conclusion'; + } + beforeFiles['marketplace/plugins/posthog-all/skills/quack/SKILL.md'] = 'unrelated old'; + afterFiles['marketplace/plugins/posthog-all/skills/quack/SKILL.md'] = 'unrelated new'; + writeTree(before, beforeFiles); + writeTree(after, afterFiles); + + const comment = renderComment(await diffDistTrees(before, after)); + + expect(comment).toMatch(/3 files changed — identical delta.*4-conclude\.md/i); + expect(comment).toMatch(/^~ plugins\/posthog-all\/skills\/quack\/SKILL\.md/m); // single-file dir collapses + // Members live only inside the collapsed
, never as diff-block lines. + expect(comment).not.toMatch(/^[-+~ ] .*integration-angular/m); + }); + + it('reports a modified cliEntry (same identity, changed fields)', async () => { + const menu = entries => JSON.stringify({ cliEntries: entries }); + writeTree(before, { + 'skills/skill-menu.json': menu([{ skillId: 'audit-events', role: 'command', parentCommand: 'audit', command: 'events', default: false }]), + }); + writeTree(after, { + 'skills/skill-menu.json': menu([{ skillId: 'audit-events', role: 'command', parentCommand: 'audit', command: 'events', default: true }]), + }); + + const comment = renderComment(await diffDistTrees(before, after)); + expect(comment).toMatch(/^~ cliEntry audit-events updated/m); + }); + + it('full report shows content-level hunks for changed files, once per identical-delta group', async () => { + const beforeFiles = {}, afterFiles = {}; + for (const id of ['android', 'angular', 'django']) { + beforeFiles[`marketplace/plugins/posthog-all/skills/integration-${id}/references/4-conclude.md`] = 'old conclusion\nshared line'; + afterFiles[`marketplace/plugins/posthog-all/skills/integration-${id}/references/4-conclude.md`] = 'new conclusion\nshared line'; + } + beforeFiles['skills/web-analytics.zip'] = await zipBuffer({ 'SKILL.md': 'zip v1' }); + afterFiles['skills/web-analytics.zip'] = await zipBuffer({ 'SKILL.md': 'zip v2' }); + writeTree(before, beforeFiles); + writeTree(after, afterFiles); + + const full = renderFull(await diffDistTrees(before, after)); + + expect(full).toContain('-old conclusion'); + expect(full).toContain('+new conclusion'); + // Identical delta across 3 files → the hunk is shown exactly once. + expect(full.match(/-old conclusion/g)).toHaveLength(1); + // Zip inner content is diffed too. + expect(full).toContain('-zip v1'); + expect(full).toContain('+zip v2'); + }); + + it('golden: the PR #330 shape — menu/marketplace/mirror changes surface, stamp and mtime noise does not', async () => { + const menu = entries => JSON.stringify({ cliEntries: entries }); + const baseEntries = [{ skillId: 'audit-events', role: 'command', parentCommand: 'audit', command: 'events' }]; + const zipContent = { 'SKILL.md': '# stable skill' }; + + writeTree(before, { + 'skills/skill-menu.json': menu(baseEntries), + 'skills/manifest.json': JSON.stringify({ buildTimestamp: '2026-08-01T00:00:00Z', resources: [] }), + 'push-manifest.json': JSON.stringify({ plugins: [] }), + 'marketplace/.claude-plugin/marketplace.json': JSON.stringify({ plugins: [] }), + 'skills/web-analytics.zip': await zipBuffer(zipContent, { date: new Date('2020-01-01T00:00:00Z') }), + }); + writeTree(after, { + 'skills/skill-menu.json': menu([...baseEntries, { skillId: 'creating-product-tours', role: 'skill' }]), + 'skills/manifest.json': JSON.stringify({ buildTimestamp: '2026-08-09T00:00:00Z', resources: [] }), + 'push-manifest.json': JSON.stringify({ plugins: [{ name: 'posthog-product-tours' }] }), + 'marketplace/.claude-plugin/marketplace.json': JSON.stringify({ plugins: [{ name: 'posthog-product-tours' }] }), + 'marketplace/plugins/posthog-product-tours/SKILL.md': '# product tours', + 'skills/web-analytics.zip': await zipBuffer(zipContent, { date: new Date('2026-08-09T12:00:00Z') }), + }); + + const comment = renderComment(await diffDistTrees(before, after)); + + expect(comment).toMatch(/^\+ cliEntry creating-product-tours \(role: skill\)/m); + expect(comment).toMatch(/^~ \.claude-plugin\/marketplace\.json/m); // marketplace index, gray + expect(comment).toMatch(/^\+ plugins\/posthog-product-tours\/SKILL\.md/m); // new plugin file, green, chain-collapsed + expect(comment).toContain('push-manifest.json'); + expect(comment).toMatch(/Skill content.*✓ unchanged/i); // mtime-only zip filtered + expect(comment).toMatch(/MCP manifest.*✓ unchanged/i); // buildTimestamp-only filtered + + const linked = renderComment(await diffDistTrees(before, after), { + fullReportUrl: 'https://github.com/PostHog/context-mill/actions/runs/123', + }); + expect(linked).toContain('[Full report](https://github.com/PostHog/context-mill/actions/runs/123)'); + }); +}); From 5048b87773ca825e2389010ce6e6676f2b20e9d4 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 9 Aug 2026 15:13:26 +0200 Subject: [PATCH 2/5] fix: address code-review findings in dist-diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Root-level files no longer fold into a nameless directory bucket (the empty-dir key was a string-prefix of every same-kind key) - Budget truncation now trims inside diff blocks, so surface headers and the "✓ unchanged" assertions always survive; removes the old tail-cut that could clobber a line and leave an open fence - Content-hunk size gate moved to decompressed inner-entry size — a tiny archive holding a huge entry no longer reaches the differ unchecked - Full report diffs added/removed files against the empty side - Corrupt-zip failures now name the offending file (still fail loudly: a zip our own build wrote but can't read back is a broken build) - Dedupe delta-key and inner-suffix construction; fix stale --self-check reference in the module header Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GDjuSAs927XvcyziFJk865 --- scripts/lib/dist-diff.js | 130 +++++++++++++++++----------- scripts/lib/tests/dist-diff.test.js | 54 ++++++++++++ 2 files changed, 135 insertions(+), 49 deletions(-) diff --git a/scripts/lib/dist-diff.js b/scripts/lib/dist-diff.js index e3cc80d2..57d97d77 100644 --- a/scripts/lib/dist-diff.js +++ b/scripts/lib/dist-diff.js @@ -9,9 +9,10 @@ * ZIPs are compared by entry contents (the archive format embeds mtimes, * which differ on every build), and JSON files are compared by parsed * value with known build-stamp fields removed. - * - Determinism guard: `npm run diff -- --self-check A B` asserts two builds - * of the same ref produce an empty normalized diff, so any new - * nondeterminism in the build fails loudly instead of eroding the report. + * - Determinism guard: `npm run diff` with `--exit-code` (git-diff + * convention) lets CI assert two builds of the same ref produce an empty + * normalized diff, so any new nondeterminism in the build fails loudly + * instead of eroding the report. */ import { readdirSync, readFileSync } from 'fs'; import { join } from 'path'; @@ -129,7 +130,13 @@ export async function diffDistTrees(beforeDir, afterDir) { } else { const a = readFileSync(join(beforeDir, path)); const b = readFileSync(join(afterDir, path)); - if (normalizedEqual(path, a, b)) continue; + try { + if (normalizedEqual(path, a, b)) continue; + } catch (err) { + // A file our own build just wrote but can't be read back (e.g. + // a truncated zip) is a broken build — fail loudly, naming it. + throw new Error(`dist-diff failed reading ${path}: ${err.message}`, { cause: err }); + } if (path.endsWith('.zip')) { const innerChanges = zipInnerChanges(a, b); changes.push({ path, kind: 'changed', innerChanges, deltaHash: zipDeltaHash(a, b, innerChanges) }); @@ -183,6 +190,19 @@ function stripSurfacePrefix(surfaceKey, path) { return prefix && path.startsWith(prefix) ? path.slice(prefix.length) : path; } +/** Grouping key for a changed artifact: same key ⇔ identical delta. */ +function deltaKey(change) { + return change.innerChanges + ? `zip:${change.innerChanges.join('|')}#${change.deltaHash}` + : `file:#${change.deltaHash}`; +} + +/** " — inner1, inner2" suffix for a zip change, optionally capped. */ +function innerSuffix(change, capInner) { + if (!change.innerChanges?.length) return ''; + return ` — ${capInner ? capList(change.innerChanges) : change.innerChanges.join(', ')}`; +} + /** * Render changes as an indented directory tree (single-child directory chains * collapsed, `tree`-style). A directory whose descendants all share one status @@ -218,20 +238,14 @@ function treeLines(items, { capInner = true } = {}) { // A directory holding exactly one file collapses onto one line. if (child.dirs.size === 0 && child.files.length === 1) { const f = child.files[0]; - const inner = f.change.innerChanges?.length - ? ` — ${capInner ? capList(f.change.innerChanges) : f.change.innerChanges.join(', ')}` - : ''; - lines.push(`${STATUS[f.change.kind]} ${' '.repeat(depth)}${label}/${f.name}${inner}`); + lines.push(`${STATUS[f.change.kind]} ${' '.repeat(depth)}${label}/${f.name}${innerSuffix(f.change, capInner)}`); continue; } lines.push(`${status} ${' '.repeat(depth)}${label}/`); emit(child, depth + 1); } for (const f of [...node.files].sort((a, b) => a.name.localeCompare(b.name))) { - const inner = f.change.innerChanges?.length - ? ` — ${capInner ? capList(f.change.innerChanges) : f.change.innerChanges.join(', ')}` - : ''; - lines.push(`${STATUS[f.change.kind]} ${' '.repeat(depth)}${f.name}${inner}`); + lines.push(`${STATUS[f.change.kind]} ${' '.repeat(depth)}${f.name}${innerSuffix(f.change, capInner)}`); } }; emit(root, 0); @@ -282,38 +296,50 @@ function wizardBlock(model) { */ export function renderComment(model, { fullReportUrl } = {}) { const bySurface = bucketBySurface(model); - const lines = [`## dist-diff — ${model.changes.length} artifact change(s)`]; + + // Build per-surface segments first, so the budget can trim inside diff + // blocks while the surface headers and "✓ unchanged" assertions — the + // report's core guarantee — always survive truncation. + const segments = []; for (const surface of SURFACES) { const changes = bySurface.get(surface.key); if (surface.key === 'other' && changes.length === 0) continue; - // Blank line between blocks — without it, GFM folds a line that - // follows a list item into that item (lazy continuation). - lines.push(''); if (changes.length === 0) { - lines.push(`**${surface.title}** ✓ unchanged`); + segments.push({ head: [`**${surface.title}** ✓ unchanged`], block: null, after: [] }); continue; } - lines.push(`**${surface.title}**`); const { block, after } = surface.key === 'wizard' ? { block: wizardBlock(model), after: [] } : surfaceBlock(changes, surface.key); - lines.push('```diff', ...block, '```', ...after); + segments.push({ head: [`**${surface.title}**`], block, after }); } - if (fullReportUrl) lines.push('', `[Full report](${fullReportUrl})`); - const BUDGET = 40; - if (lines.length > BUDGET) { - const link = fullReportUrl ? lines[lines.length - 1] : null; - const hidden = lines.length - (BUDGET - 1); - lines.length = BUDGET - (link ? 2 : 1); - // Never truncate inside an open ```diff fence — close it first. - if (lines.filter(l => l.startsWith('```')).length % 2 === 1) { - lines[lines.length - 1] = '```'; + const fixedCost = 1 + (fullReportUrl ? 2 : 0) + + segments.reduce((n, s) => n + 1 + s.head.length + (s.block ? 2 : 0) + s.after.length, 0); + let overflow = segments.reduce((n, s) => n + (s.block?.length ?? 0), 0) - (BUDGET - fixedCost); + if (overflow > 0) { + // Trim the largest blocks first; each trimmed block keeps its first + // lines plus a gray pointer at the full report. + for (const s of [...segments].sort((a, b) => (b.block?.length ?? 0) - (a.block?.length ?? 0))) { + if (overflow <= 0) break; + const len = s.block?.length ?? 0; + if (len < 4) continue; + const cut = Math.min(overflow + 1, len - 2); + s.block.length = len - cut; + s.block.push(`~ …${cut} more line(s) — see the full report in the workflow summary`); + overflow -= cut - 1; } - lines.push(`…${hidden} more line(s) — see the full report in the workflow summary.`); - if (link) lines.push(link); } + + const lines = [`## dist-diff — ${model.changes.length} artifact change(s)`]; + for (const s of segments) { + // Blank line between blocks — without it, GFM folds a line that + // follows a fenced block or list into the preceding element. + lines.push('', ...s.head); + if (s.block) lines.push('```diff', ...s.block, '```', ...s.after); + } + if (fullReportUrl) lines.push('', `[Full report](${fullReportUrl})`); return lines.join('\n'); } @@ -342,18 +368,18 @@ function capHunks(lines) { return [...lines.slice(0, MAX_HUNK_LINES), `… ${lines.length - MAX_HUNK_LINES} more diff line(s) omitted`]; } -/** Unified-diff lines for one changed artifact; zips diff per inner entry. */ +/** Unified-diff lines for one artifact (added/removed diff against empty). */ function changeHunks(model, change) { - const a = readFileSync(join(model.beforeDir, change.path)); - const b = readFileSync(join(model.afterDir, change.path)); - if (a.length > MAX_DIFF_BYTES || b.length > MAX_DIFF_BYTES) { - return [`(file too large to diff: ${Math.round(Math.max(a.length, b.length) / 1024)} KB)`]; - } + const tooLarge = size => `(too large to diff: ${Math.round(size / 1024)} KB)`; + const a = change.kind === 'added' ? Buffer.alloc(0) : readFileSync(join(model.beforeDir, change.path)); + const b = change.kind === 'removed' ? Buffer.alloc(0) : readFileSync(join(model.afterDir, change.path)); if (change.path.endsWith('.zip')) { - const ea = readZipEntries(a); - const eb = readZipEntries(b); + const ea = a.length ? readZipEntries(a) : {}; + const eb = b.length ? readZipEntries(b) : {}; + const names = change.innerChanges + ?? [...new Set([...Object.keys(ea), ...Object.keys(eb)])].sort(); const lines = []; - for (const name of change.innerChanges ?? []) { + for (const name of names) { lines.push(`# ${name}`); if (name.endsWith('.zip')) { lines.push('(nested archive — its contents are diffed as individual skill zips)'); @@ -361,6 +387,11 @@ function changeHunks(model, change) { } const ia = ea[name] ?? Buffer.alloc(0); const ib = eb[name] ?? Buffer.alloc(0); + // Gate on DECOMPRESSED size — a tiny archive can hold a huge entry. + if (ia.length > MAX_DIFF_BYTES || ib.length > MAX_DIFF_BYTES) { + lines.push(tooLarge(Math.max(ia.length, ib.length))); + continue; + } if (!isText(ia) || !isText(ib)) { lines.push('(binary entry)'); continue; @@ -369,6 +400,9 @@ function changeHunks(model, change) { } return capHunks(lines); } + if (a.length > MAX_DIFF_BYTES || b.length > MAX_DIFF_BYTES) { + return [tooLarge(Math.max(a.length, b.length))]; + } if (!isText(a) || !isText(b)) return ['(binary file)']; return capHunks(hunkLines(a.toString('utf8'), b.toString('utf8'))); } @@ -380,10 +414,7 @@ function changeHunks(model, change) { function contentSections(model, changes) { const groups = new Map(); for (const change of changes) { - if (change.kind !== 'changed') continue; - const key = change.deltaHash - ? (change.innerChanges ? `zip:${change.innerChanges.join('|')}#${change.deltaHash}` : `file:#${change.deltaHash}`) - : `single:${change.path}`; + const key = change.kind === 'changed' && change.deltaHash ? deltaKey(change) : `single:${change.path}`; if (!groups.has(key)) groups.set(key, []); groups.get(key).push(change); } @@ -454,9 +485,7 @@ function surfaceBlock(changes, surfaceKey) { const dirBuckets = new Map(); for (const change of changes) { if (change.kind === 'changed' && change.deltaHash) { - const key = change.innerChanges - ? `zip:${change.innerChanges.join('|')}#${change.deltaHash}` - : `file:#${change.deltaHash}`; + const key = deltaKey(change); if (!exactGroups.has(key)) exactGroups.set(key, []); exactGroups.get(key).push(change); } else { @@ -468,18 +497,21 @@ function surfaceBlock(changes, surfaceKey) { } // A new variant lands as one directory tree; fold each bucket into the - // shallowest same-kind bucket whose directory contains it. + // shallowest same-kind bucket whose directory contains it. The root + // bucket (empty dir, key "kind:") is a string-prefix of every same-kind + // key and has no meaningful label — it never folds or groups. + const isRootKey = k => k.endsWith(':'); const dirKeys = [...dirBuckets.keys()].sort((a, b) => a.length - b.length); for (const key of dirKeys) { if (!dirBuckets.has(key)) continue; - const ancestor = dirKeys.find(k => k !== key && dirBuckets.has(k) && key.startsWith(k)); + const ancestor = dirKeys.find(k => k !== key && !isRootKey(k) && dirBuckets.has(k) && key.startsWith(k)); if (ancestor) { dirBuckets.get(ancestor).push(...dirBuckets.get(key)); dirBuckets.delete(key); } } for (const [key, members] of dirBuckets) { - if (members.length >= GROUP_MIN) { + if (members.length >= GROUP_MIN && !isRootKey(key)) { const dir = stripSurfacePrefix(surfaceKey, key.slice(key.indexOf(':') + 1)); block.push(`${STATUS[members[0].kind]} ${dir} — ${members.length} files ${members[0].kind}`); after.push(`
${dir} (${members.length} files)${members.map(m => m.path).join(', ')}
`); diff --git a/scripts/lib/tests/dist-diff.test.js b/scripts/lib/tests/dist-diff.test.js index 358da02b..aab3c3a6 100644 --- a/scripts/lib/tests/dist-diff.test.js +++ b/scripts/lib/tests/dist-diff.test.js @@ -293,6 +293,60 @@ describe('renderComment', () => { expect(full).toContain('+zip v2'); }); + it('does not fold root-level files into directory buckets (empty-dir key is a prefix of every key)', async () => { + const afterFiles = {}; + for (let i = 0; i < 3; i++) afterFiles[`root-${i}.bin`] = `r${i}`; + for (let i = 0; i < 4; i++) afterFiles[`deep/nested/f${i}.bin`] = `n${i}`; + writeTree(before, { 'keep.txt': 'x' }); + writeTree(after, { 'keep.txt': 'x', ...afterFiles }); + + const comment = renderComment(await diffDistTrees(before, after)); + + expect(comment).toMatch(/deep\/nested\/ — 4 files added/); + expect(comment).not.toMatch(/^\+ {2}— \d+ files/m); // no nameless bucket + expect(comment).toMatch(/^\+ root-0\.bin/m); // root files itemized + }); + + it('keeps every surface assertion visible when the budget forces truncation', async () => { + const beforeFiles = {}, afterFiles = {}; + // Distinct inner-file names per zip, so no grouping tier can collapse them. + for (let i = 0; i < 60; i++) { + beforeFiles[`skills/skill-${i}.zip`] = await zipBuffer({ [`ref-${i}.md`]: `old ${i}` }); + afterFiles[`skills/skill-${i}.zip`] = await zipBuffer({ [`ref-${i}.md`]: `new ${i}` }); + } + writeTree(before, beforeFiles); + writeTree(after, afterFiles); + + const comment = renderComment(await diffDistTrees(before, after), { fullReportUrl: 'https://x' }); + const lines = comment.split('\n'); + + expect(lines.length).toBeLessThanOrEqual(40); + // Truncation must spend block content, never the surface assertions. + for (const title of ['Wizard surface', 'Marketplace', 'Agents', 'MCP manifest', 'Skills-repo mirror']) { + expect(comment).toContain(`**${title}** ✓ unchanged`); + } + expect(comment).toMatch(/more .*full report/i); + expect(lines.filter(l => l.startsWith('```')).length % 2).toBe(0); // fences balanced + }); + + it('caps content hunks by decompressed inner-entry size, not archive size', async () => { + const big = 'line\n'.repeat(80_000); // ~400KB decompressed, tiny compressed + writeTree(before, { 'skills/a.zip': await zipBuffer({ 'huge.md': big }) }); + writeTree(after, { 'skills/a.zip': await zipBuffer({ 'huge.md': `${big}tail\n` }) }); + + const full = renderFull(await diffDistTrees(before, after)); + expect(full).toMatch(/too large to diff/i); + }); + + it('full report shows content for added and removed files', async () => { + writeTree(before, { 'skills/old-note.md': 'goodbye content' }); + writeTree(after, { 'skills/new-note.md': 'hello content' }); + + const full = renderFull(await diffDistTrees(before, after)); + expect(full).toContain('+hello content'); + expect(full).toContain('-goodbye content'); + }); + it('golden: the PR #330 shape — menu/marketplace/mirror changes surface, stamp and mtime noise does not', async () => { const menu = entries => JSON.stringify({ cliEntries: entries }); const baseEntries = [{ skillId: 'audit-events', role: 'command', parentCommand: 'audit', command: 'events' }]; From 743e346ad569e3921d97016750a7ccbb846d6c83 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 9 Aug 2026 15:25:00 +0200 Subject: [PATCH 3/5] feat: name full-report groups by the changed file, hunk inline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An identical-delta fan-out is one content change that happens to land in many archives — so the full report now leads with what changed (the shared inner file) and the hunk itself, and collapses the member list into
. Replaces the per-surface tree + hunks-in-details layout where 68 zip lines repeated the same inner path. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GDjuSAs927XvcyziFJk865 --- scripts/lib/dist-diff.js | 53 +++++++++++++++++------------ scripts/lib/tests/dist-diff.test.js | 20 +++++++++++ 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/scripts/lib/dist-diff.js b/scripts/lib/dist-diff.js index 57d97d77..6d90dce6 100644 --- a/scripts/lib/dist-diff.js +++ b/scripts/lib/dist-diff.js @@ -408,22 +408,39 @@ function changeHunks(model, change) { } /** - * Content sections for a surface's changed artifacts, one per unique delta: - * an identical-delta fan-out shows its hunk once, titled with the member count. + * Sections for the full report, one per unique delta. An identical-delta + * fan-out is titled by WHAT changed (the shared inner file), shows its hunk + * once, and collapses the member list — the zip names are the noise, the + * content change is the signal. */ -function contentSections(model, changes) { +function fullSections(model, changes) { const groups = new Map(); for (const change of changes) { const key = change.kind === 'changed' && change.deltaHash ? deltaKey(change) : `single:${change.path}`; if (!groups.has(key)) groups.set(key, []); groups.get(key).push(change); } - return [...groups.values()].map(members => ({ - title: members.length > 1 - ? `${members.length} files — identical delta (${commonPathSuffix(members.map(m => m.path)) || members[0].path})` - : members[0].path, - hunks: changeHunks(model, members[0]), - })); + return [...groups.values()].map(members => { + const rep = members[0]; + const isZip = rep.path.endsWith('.zip'); + let title; + if (members.length > 1) { + const what = rep.innerChanges + ? capList(rep.innerChanges.map(n => `\`${n}\``)) + : `\`${commonPathSuffix(members.map(m => m.path)) || rep.path}\``; + title = `${what} — changed identically in ${members.length} ${isZip ? 'zips' : 'files'}`; + } else { + const kindNote = rep.kind === 'changed' ? '' : ` (${rep.kind})`; + title = `\`${rep.path}\`${kindNote}`; + } + return { + title, + hunks: changeHunks(model, rep), + members: members.length > 1 + ? `
${members.length} ${isZip ? 'archives' : 'files'}${members.map(m => m.path).join(', ')}
` + : null, + }; + }); } /** Render the full report: every change listed; content hunks size-budgeted. */ @@ -442,21 +459,15 @@ export function renderFull(model) { if (surface.key === 'wizard') { lines.push('```diff', ...wizardBlock(model), '```', ''); } - // Complete tree of every change — nothing grouped, nothing capped. - lines.push('```diff'); - lines.push(...treeLines( - changes.map(c => ({ rel: stripSurfacePrefix(surface.key, c.path), change: c })), - { capInner: false }, - )); - lines.push('```', ''); - // Content-level hunks, one per unique delta, under a report-wide - // budget — the step summary rejects anything over 1MB. - const sections = contentSections(model, changes); + // One section per unique delta: hunk inline (the signal), member list + // collapsed (the noise). Report-wide hunk budget — the step summary + // rejects anything over 1MB. let omitted = 0; - for (const { title, hunks } of sections) { + for (const { title, hunks, members } of fullSections(model, changes)) { if (hunkBudget <= 0) { omitted++; continue; } hunkBudget -= hunks.length; - lines.push(`
${title}`, '', '```diff', ...hunks, '```', '', '
', ''); + lines.push(`### ${title}`, '', '```diff', ...hunks, '```', ''); + if (members) lines.push(members, ''); } if (omitted) { lines.push(`_${omitted} more content diff(s) omitted for size — run \`npm run diff\` locally for the complete set._`, ''); diff --git a/scripts/lib/tests/dist-diff.test.js b/scripts/lib/tests/dist-diff.test.js index aab3c3a6..1443a1a7 100644 --- a/scripts/lib/tests/dist-diff.test.js +++ b/scripts/lib/tests/dist-diff.test.js @@ -271,6 +271,26 @@ describe('renderComment', () => { expect(comment).toMatch(/^~ cliEntry audit-events updated/m); }); + it('full report names identical-delta groups by the changed file, hunk inline, members collapsed', async () => { + const beforeFiles = {}, afterFiles = {}; + for (const id of ['anthropic', 'openai', 'mistral', 'groq', 'cohere']) { + beforeFiles[`skills/ai-observability-${id}.zip`] = await zipBuffer({ 'SKILL.md': `keep ${id}`, 'references/setup.md': 'old setup' }); + afterFiles[`skills/ai-observability-${id}.zip`] = await zipBuffer({ 'SKILL.md': `keep ${id}`, 'references/setup.md': 'new setup' }); + } + writeTree(before, beforeFiles); + writeTree(after, afterFiles); + + const full = renderFull(await diffDistTrees(before, after)); + + // The group is titled by WHAT changed, not by 68 repeating zip names... + expect(full).toMatch(/`references\/setup\.md` — changed identically in 5 zips/); + // ...the hunk appears once, inline (not inside
)... + expect(full.match(/-old setup/g)).toHaveLength(1); + expect(full.indexOf('-old setup')).toBeLessThan(full.indexOf('
')); + // ...and the member list is what's collapsed. + expect(full).toMatch(/
5 archives<\/summary>skills\/ai-observability-anthropic\.zip/); + }); + it('full report shows content-level hunks for changed files, once per identical-delta group', async () => { const beforeFiles = {}, afterFiles = {}; for (const id of ['android', 'angular', 'django']) { From 8e6c32117b5f56d6526cc79bcd66f4e4c74f1dca Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 9 Aug 2026 15:33:03 +0200 Subject: [PATCH 4/5] feat: verify the aggregate bundle against its constituents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit skills-mcp-resources.zip is derived (manifest + every skill zip), so its diff repeats what the report already shows. Cross-check each inner change against the reported constituent changes: consistent collapses to one verified summary line; an unexplained inner change — a broken bundling step — goes loud with a warning in both reports. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GDjuSAs927XvcyziFJk865 --- scripts/lib/dist-diff.js | 48 +++++++++++++++++++++++++++-- scripts/lib/tests/dist-diff.test.js | 41 ++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/scripts/lib/dist-diff.js b/scripts/lib/dist-diff.js index 6d90dce6..e469e9db 100644 --- a/scripts/lib/dist-diff.js +++ b/scripts/lib/dist-diff.js @@ -190,6 +190,25 @@ function stripSurfacePrefix(surfaceKey, path) { return prefix && path.startsWith(prefix) ? path.slice(prefix.length) : path; } +/** + * skills-mcp-resources.zip is DERIVED: it bundles manifest.json plus every + * skill zip/bundle, so its diff can never carry new information — unless the + * bundling step broke. Verify each inner change maps to a reported + * constituent change (inner `x.zip` ↔ `skills/x.zip`); consistent means the + * one-line summary is a checked claim, not an assumption. + */ +const AGGREGATE_PATH = 'skills-mcp-resources.zip'; + +function aggregateConsistency(model, change) { + const changedPaths = new Set(model.changes.map(c => c.path)); + const inner = change.innerChanges ?? []; + const unexplained = inner.filter(name => !changedPaths.has(`skills/${name}`)); + const zips = inner.filter(name => name.endsWith('.zip')).length; + const summary = [zips && `${zips} zip(s)`, inner.length - zips && `${inner.length - zips} other file(s)`] + .filter(Boolean).join(', '); + return { consistent: unexplained.length === 0, unexplained, summary }; +} + /** Grouping key for a changed artifact: same key ⇔ identical delta. */ function deltaKey(change) { return change.innerChanges @@ -310,7 +329,7 @@ export function renderComment(model, { fullReportUrl } = {}) { } const { block, after } = surface.key === 'wizard' ? { block: wizardBlock(model), after: [] } - : surfaceBlock(changes, surface.key); + : surfaceBlock(model, changes, surface.key); segments.push({ head: [`**${surface.title}**`], block, after }); } @@ -422,6 +441,21 @@ function fullSections(model, changes) { } return [...groups.values()].map(members => { const rep = members[0]; + if (rep.path === AGGREGATE_PATH && rep.kind === 'changed') { + const { consistent, unexplained, summary } = aggregateConsistency(model, rep); + if (consistent) { + return { + title: `\`${AGGREGATE_PATH}\` — aggregate`, + hunks: [`(consistent with its constituents — ${summary}, diffed in the sections above)`], + members: null, + }; + } + return { + title: `⚠️ \`${AGGREGATE_PATH}\` — aggregate diverges from its constituents`, + hunks: [`unexplained inner change(s): ${unexplained.join(', ')}`, ...changeHunks(model, rep)], + members: null, + }; + } const isZip = rep.path.endsWith('.zip'); let title; if (members.length > 1) { @@ -488,13 +522,23 @@ export function renderFull(model) { */ const GROUP_MIN = 4; -function surfaceBlock(changes, surfaceKey) { +function surfaceBlock(model, changes, surfaceKey) { const block = []; const after = []; const singles = []; const exactGroups = new Map(); const dirBuckets = new Map(); for (const change of changes) { + if (change.path === AGGREGATE_PATH && change.kind === 'changed') { + const { consistent, unexplained, summary } = aggregateConsistency(model, change); + if (consistent) { + block.push(`~ ${AGGREGATE_PATH} — aggregate, consistent with its constituents (${summary}, reported above)`); + } else { + block.push(`~ ⚠️ ${AGGREGATE_PATH} — ${unexplained.length} inner change(s) no constituent explains: ${capList(unexplained)}`); + singles.push(change); + } + continue; + } if (change.kind === 'changed' && change.deltaHash) { const key = deltaKey(change); if (!exactGroups.has(key)) exactGroups.set(key, []); diff --git a/scripts/lib/tests/dist-diff.test.js b/scripts/lib/tests/dist-diff.test.js index 1443a1a7..00a5d068 100644 --- a/scripts/lib/tests/dist-diff.test.js +++ b/scripts/lib/tests/dist-diff.test.js @@ -367,6 +367,47 @@ describe('renderComment', () => { expect(full).toContain('-goodbye content'); }); + it('summarizes the aggregate bundle as one verified line when its changes match its constituents', async () => { + const oldSkill = { 'SKILL.md': 'v1' }; + const newSkill = { 'SKILL.md': 'v2' }; + writeTree(before, { + 'skills/web-analytics.zip': await zipBuffer(oldSkill), + 'skills-mcp-resources.zip': await zipBuffer({ + 'manifest.json': JSON.stringify({ resources: [] }), + 'web-analytics.zip': await zipBuffer(oldSkill), + }), + }); + writeTree(after, { + 'skills/web-analytics.zip': await zipBuffer(newSkill), + 'skills-mcp-resources.zip': await zipBuffer({ + 'manifest.json': JSON.stringify({ resources: [] }), + 'web-analytics.zip': await zipBuffer(newSkill), + }), + }); + + const model = await diffDistTrees(before, after); + const comment = renderComment(model); + const full = renderFull(model); + + expect(comment).toMatch(/skills-mcp-resources\.zip — aggregate, consistent with/i); + expect(full).toMatch(/consistent with its constituents/i); + // The constituent's hunk appears once (its own section), not again for the aggregate. + expect(full.match(/-v1/g)).toHaveLength(1); + }); + + it('goes loud when the aggregate contains a change no constituent explains', async () => { + writeTree(before, { + 'skills-mcp-resources.zip': await zipBuffer({ 'phantom.zip': await zipBuffer({ 'SKILL.md': 'v1' }) }), + }); + writeTree(after, { + 'skills-mcp-resources.zip': await zipBuffer({ 'phantom.zip': await zipBuffer({ 'SKILL.md': 'v2' }) }), + }); + + const model = await diffDistTrees(before, after); + expect(renderComment(model)).toMatch(/⚠️.*phantom\.zip/); + expect(renderFull(model)).toMatch(/⚠️.*diverges/i); + }); + it('golden: the PR #330 shape — menu/marketplace/mirror changes surface, stamp and mtime noise does not', async () => { const menu = entries => JSON.stringify({ cliEntries: entries }); const baseEntries = [{ skillId: 'audit-events', role: 'command', parentCommand: 'audit', command: 'events' }]; From 47e9ca961b6016af9314aff9c04a0e5aced4d9e9 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 9 Aug 2026 15:37:38 +0200 Subject: [PATCH 5/5] fix: render the aggregate consistency notice as plaintext, not a diff block Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GDjuSAs927XvcyziFJk865 --- scripts/lib/dist-diff.js | 8 ++++++-- scripts/lib/tests/dist-diff.test.js | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/scripts/lib/dist-diff.js b/scripts/lib/dist-diff.js index e469e9db..39f2f7e5 100644 --- a/scripts/lib/dist-diff.js +++ b/scripts/lib/dist-diff.js @@ -446,7 +446,7 @@ function fullSections(model, changes) { if (consistent) { return { title: `\`${AGGREGATE_PATH}\` — aggregate`, - hunks: [`(consistent with its constituents — ${summary}, diffed in the sections above)`], + note: `✓ consistent with its constituents — ${summary}, diffed in the sections above`, members: null, }; } @@ -497,7 +497,11 @@ export function renderFull(model) { // collapsed (the noise). Report-wide hunk budget — the step summary // rejects anything over 1MB. let omitted = 0; - for (const { title, hunks, members } of fullSections(model, changes)) { + for (const { title, hunks, note, members } of fullSections(model, changes)) { + if (note) { + lines.push(`### ${title}`, '', note, ''); + continue; + } if (hunkBudget <= 0) { omitted++; continue; } hunkBudget -= hunks.length; lines.push(`### ${title}`, '', '```diff', ...hunks, '```', ''); diff --git a/scripts/lib/tests/dist-diff.test.js b/scripts/lib/tests/dist-diff.test.js index 00a5d068..83b5ddf4 100644 --- a/scripts/lib/tests/dist-diff.test.js +++ b/scripts/lib/tests/dist-diff.test.js @@ -390,7 +390,8 @@ describe('renderComment', () => { const full = renderFull(model); expect(comment).toMatch(/skills-mcp-resources\.zip — aggregate, consistent with/i); - expect(full).toMatch(/consistent with its constituents/i); + expect(full).toMatch(/^✓ consistent with its constituents/m); // plaintext line, not a code block + expect(full).not.toMatch(/```diff\n\(consistent/); // The constituent's hunk appears once (its own section), not again for the aggregate. expect(full.match(/-v1/g)).toHaveLength(1); });