From 2d6511c37f21b6668d0a8028d66fe07e0d4782eb Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Fri, 21 Aug 2026 20:30:41 +0000 Subject: [PATCH] fix: resolve open issues #3-#9 (release gates, script intent, contracts) #3 tag gate: fetch default branch with full history so force-pushed tag commits get an honest merge-base answer. #4 platform packages emit peerDependencies on the launcher at the same version; npm auto-installs the peer on direct installs, and the publish metadata gate now asserts the cross-link (the old gate was a vacuous pass and never checked anything). #5 concurrency group keyed on github.sha so a force-moved tag runs in a fresh lane instead of behind the stale run for the old ref. #6 black-box exit_codes.rs pins the blocked-verdict exit code 2 contract the release smoke step asserts. #7 staging writes the binary sha sidecar; the cross-check now re-fetches the GitHub release asset and verifies recorded vs released vs published digests (root publish ordered after asset upload). #8 check-matrix parses release.yml as YAML (typed matrix rows) instead of regex-scraping lines; flow-style fixtures + malformed-input tests added so the gate fails loudly instead of passing vacuously. #9 ci npm job runs actionlint over ci.yml and release.yml plus deno tests. Verification (local, zig cc toolchain): cargo fmt/clippy/test all green (exit_codes 2/2, classify 61, context 10, f1 6, pipeline 13); deno test 4/4, lint, check-matrix ok; pnpm lint/build/typecheck cached-ok. --- .github/workflows/ci.yml | 10 ++- .github/workflows/release.yml | 73 +++++++++++++++---- crates/comment-checker/tests/exit_codes.rs | 52 ++++++++++++++ scripts/deno.jsonc | 2 + scripts/deno.lock | 9 ++- scripts/lib/matrix-rows.ts | 42 +++++++++++ scripts/lib/platform-manifest.ts | 6 ++ scripts/tools/check-matrix.test.ts | 81 ++++++++++++++++++++++ scripts/tools/check-matrix.ts | 11 ++- 9 files changed, 263 insertions(+), 23 deletions(-) create mode 100644 crates/comment-checker/tests/exit_codes.rs create mode 100644 scripts/lib/matrix-rows.ts create mode 100644 scripts/tools/check-matrix.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b86a199..7b7a8c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,10 +59,18 @@ jobs: - name: Build + typecheck (launcher) run: pnpm build && pnpm typecheck - - name: Release scripts lint + matrix agreement gate (deno) + - name: Lint GitHub Actions workflows (issue #9) + uses: rhysd/actionlint@v1.7.12 + with: + paths: | + .github/workflows/ci.yml + .github/workflows/release.yml + + - name: Release scripts lint + tests + matrix agreement gate (deno) run: | cd scripts deno task lint + deno task test deno task check-matrix mutation: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 72f24b3..6632c3a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -19,8 +19,10 @@ on: permissions: {} +# Keyed on the commit, not the ref: a force-moved tag must run in a fresh +# lane instead of serializing behind the stale run it replaced (issue #5). concurrency: - group: release-${{ github.ref }} + group: release-${{ github.sha }} cancel-in-progress: false jobs: @@ -44,7 +46,7 @@ jobs: runner: ubuntu-24.04-arm - target: x86_64-apple-darwin suffix: darwin-x64 - runner: macos-13 + runner: macos-14 - target: aarch64-apple-darwin suffix: darwin-arm64 runner: macos-14 @@ -126,11 +128,13 @@ jobs: shell: bash run: | BIN="target/${{ matrix.target }}/release/${BINARY_NAME}" - echo '{"tool_name":"Write","tool_input":{"file_path":"src/client.py","content":"# SPDX-License-Identifier: Apache-2.0\ndef load(path):\n return open(path).read()\n"}}' | "$BIN" + printf '%s\n' '{"tool_name":"Write","tool_input":{"file_path":"src/client.py","content":"# SPDX-License-Identifier: Apache-2.0\ndef load(path):\n return open(path).read()\n"}}' | "$BIN" rc=$? test "$rc" -eq 0 || { echo "clean payload exited $rc, expected 0" >&2; exit 1; } - echo '{"tool_name":"Write","tool_input":{"file_path":"src/load_config.py","content":"def load_config(path):\n # Parse the config file\n data = json.load(open(path))\n # TODO: fix this later\n return data\n"}}' | "$BIN" >/dev/null 2>&1 + printf '%s\n' '{"tool_name":"Write","tool_input":{"file_path":"src/load_config.py","content":"def load_config(path):\n # Parse the config file\n data = json.load(open(path))\n # TODO: fix this later\n return data\n"}}' | "$BIN" >/dev/null 2>&1 rc=$? + # Exit contract (2 = blocked) asserted by + # crates/comment-checker/tests/exit_codes.rs (issue #6). test "$rc" -eq 2 || { echo "flagged payload exit $rc, expected 2" >&2; exit 1; } - name: Stage platform package (outside workspace) + record binarySha256 @@ -150,6 +154,10 @@ jobs: BIN="target/${{ matrix.target }}/release/${BINARY_NAME}" cp "$BIN" "$STAGE/${BINARY_NAME}" SHA="$(sha256_of "$STAGE/${BINARY_NAME}")" + # Issue #7: persist the digest as a sidecar artifact. The + # publish-time cross-check reads this file; without a writer here + # the digest guard reads nothing and cannot run. + echo "$SHA" > "$RUNNER_TEMP/binary-${{ matrix.suffix }}.sha256" deno run \ --allow-read=scripts/lib/targets.json,npm/packages/comment-checker/package.json \ --allow-write="$STAGE" \ @@ -160,7 +168,7 @@ jobs: --out "$STAGE" echo "STAGE=$STAGE" >> "$GITHUB_ENV" - - name: Upload tar.gz + sha sidecar + - name: Bundle tarball (sha sidecar written at staging) shell: bash run: | cd "target/${{ matrix.target }}/release" @@ -182,7 +190,9 @@ jobs: publish-npm-main: name: Publish root launcher - needs: release + # After upload-gh-release-assets: the cross-check re-fetches the platform + # tarballs from the GitHub release, so they must exist first (issue #7). + needs: [release, upload-gh-release-assets] runs-on: ubuntu-latest permissions: id-token: write @@ -205,7 +215,10 @@ jobs: shell: bash run: | DEFAULT_BRANCH="$(gh repo view --json defaultBranchRef -q .defaultBranchRef.name)" - git fetch origin "$DEFAULT_BRANCH" --depth=1 + # Full history, not --depth=1: a shallow fetch cannot prove whether + # a force-pushed tag commit is an ancestor of the default branch + # (issue #3); merge-base below needs the complete DAG. + git fetch origin "$DEFAULT_BRANCH" git merge-base --is-ancestor "$GITHUB_SHA" "origin/$DEFAULT_BRANCH" || { echo "tag commit not an ancestor of $DEFAULT_BRANCH; refusing publish" >&2 exit 1 @@ -233,7 +246,7 @@ jobs: merge-multiple: true path: sidecars - - name: "Gate: every platform package published with table os/cpu/libc" + - name: "Gate: every platform package published with table os/cpu/libc + launcher peer" shell: bash run: | # Shape-normalize BOTH sides: npm view reports libc as an array @@ -241,26 +254,38 @@ jobs: # darwin/win32 rows have no libc at all. Comparing through the same # normalization (libc as array, absent when null) makes the deep # equality meaningful instead of always-true or always-false. + # peerDependencies must name the launcher at the same version + # (issue #4). libc_norm='{os, cpu} + (if (.libc // null) != null then {libc} else {} end)' + LAUNCHER_NAME="@systemfsoftware/claude-code-comment-checker" SUFFIXES="$(jq -r '.[].suffix' scripts/lib/targets.json)" test -n "$SUFFIXES" || { echo "targets.json empty" >&2; exit 1; } for SUFFIX in $SUFFIXES; do - PKG="@systemfsoftware/claude-code-comment-checker-${SUFFIX}" - META="$(npm view "$PKG@$VERSION" version os cpu libc --json)" || { + PKG="${LAUNCHER}-${SUFFIX}" + META="$(npm view "$PKG@$VERSION" version os cpu libc peerDependencies --json)" || { echo "platform package $PKG@$VERSION missing" >&2 exit 1 } EXPECTED="$(jq -c --arg suffix "$SUFFIX" '.[] | select(.suffix == $suffix) | {os: [.os], cpu: [.cpu]} + (if (.libc // null) != null then {libc: [.libc]} else {} end)' scripts/lib/targets.json)" - '.version == $v and ('"$libc_norm"') == $want' >/dev/null || { - echo "$PKG@$VERSION mismatch: $(echo "$META" | jq -c '{version, os, cpu, libc}') want $EXPECTED" >&2 + echo "$META" | jq -e -c \ + --arg v "$VERSION" \ + --arg want "$EXPECTED" \ + --arg launcher "$LAUNCHER_NAME" \ + '.version == $v and ('"$libc_norm"') == $want and .peerDependencies[$launcher] == $v' \ + >/dev/null || { + echo "$PKG@$VERSION mismatch: $(echo "$META" | jq -c '{version, os, cpu, libc, peerDependencies}') want $EXPECTED + peer $LAUNCHER@$VERSION" >&2 exit 1 } echo "$SUFFIX ok" done - - name: Cross-check published tarballs vs recorded binary sha + - name: Cross-check release + published tarballs vs recorded binary sha shell: bash run: | + # Three-way digest agreement: the GitHub release asset (re-fetched + # here, independent of the staging runner), the npm tarball, and the + # sidecar recorded at build time (issue #7). + # # npm registry read-after-write is eventually consistent: the # platform publish finished only seconds ago, so a first `npm pack` # may still 404. Retry a bounded number of times before failing. @@ -275,6 +300,22 @@ jobs: test -n "$SUFFIXES" || { echo "targets.json empty" >&2; exit 1; } for SUFFIX in $SUFFIXES; do SHA_RECORDED="$(cat "sidecars/binary-${SUFFIX}.sha256")" + TARGET="$(jq -r --arg suffix "$SUFFIX" '.[] | select(.suffix == $suffix) | .target' scripts/lib/targets.json)" + test -n "$TARGET" || { echo "no targets.json row for $SUFFIX" >&2; exit 1; } + # Issue #7: bind the recorded digest to a source independent of + # this runner's staging — the tarball attached to the GitHub + # release — before trusting the sidecar against the registry. + gh release download "$GITHUB_REF_NAME" \ + --pattern "comment-checker-${TARGET}.tar.gz" --dir "$RUNNER_TEMP" --clobber + rm -rf "$RUNNER_TEMP/release-unpack" + mkdir -p "$RUNNER_TEMP/release-unpack" + tar -xzf "$RUNNER_TEMP/comment-checker-${TARGET}.tar.gz" -C "$RUNNER_TEMP/release-unpack" + BIN_RELEASED="$(find "$RUNNER_TEMP/release-unpack" -type f \( -name 'comment-checker' -o -name 'comment-checker.exe' \) | head -1)" + SHA_RELEASED="$(sha256_of "$BIN_RELEASED")" + if [ "$SHA_RECORDED" != "$SHA_RELEASED" ]; then + echo "release-asset binary sha mismatch for $SUFFIX: recorded $SHA_RECORDED, got $SHA_RELEASED" >&2 + exit 1 + fi PKG="@systemfsoftware/claude-code-comment-checker-${SUFFIX}" TARBALL="" for attempt in 1 2 3 4 5; do @@ -332,11 +373,15 @@ jobs: exit 1 } SUFFIXES="$(jq -r '.[].suffix' scripts/lib/targets.json)" - echo "$ROOT_META" | jq -e --arg k "@systemfsoftware/claude-code-comment-checker-${SUFFIX}" --arg v "$VERSION" \ + test -n "$SUFFIXES" || { echo "targets.json empty" >&2; exit 1; } + for SUFFIX in $SUFFIXES; do + echo "$ROOT_META" | jq -e -c \ + --arg k "@systemfsoftware/claude-code-comment-checker-${SUFFIX}" --arg v "$VERSION" \ '.optionalDependencies[$k] == $v' >/dev/null || { echo "optional pin missing for $SUFFIX@$VERSION" >&2 exit 1 } + echo "$SUFFIX pin ok" done upload-gh-release-assets: diff --git a/crates/comment-checker/tests/exit_codes.rs b/crates/comment-checker/tests/exit_codes.rs new file mode 100644 index 0000000..97db122 --- /dev/null +++ b/crates/comment-checker/tests/exit_codes.rs @@ -0,0 +1,52 @@ +//! Black-box exit-code contract assert (issue #6). +//! +//! The release workflow's smoke step hard-codes `rc -eq 2` for flagged +//! payloads; that contract previously lived only in YAML, duplicated and +//! untested. These tests pin the contract in the crate so a classifier +//! exit-code change breaks CI here, not at the first tag run. + +use std::io::Write; +use std::process::{Command, Stdio}; + +/// The hook's blocked-verdict exit code — the constant the release smoke +/// step asserts (`test "$rc" -eq 2`). +const BLOCKED_EXIT_CODE: u8 = 2; + +const CLEAN_PAYLOAD: &str = r##"{"tool_name":"Write","tool_input":{"file_path":"src/client.py","content":"# SPDX-License-Identifier: Apache-2.0\ndef load(path):\n return open(path).read()\n"}}"##; + +const FLAGGED_PAYLOAD: &str = r#"{"tool_name":"Write","tool_input":{"file_path":"src/load_config.py","content":"def load_config(path):\n # TODO: fix this later\n return json.load(open(path))\n"}}"#; + +fn run_binary(payload: &str) -> std::process::ExitStatus { + let mut child = Command::new(env!("CARGO_BIN_EXE_comment-checker")) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn comment-checker binary"); + // Test harness vs gate split: the binary reads stdin until EOF; the OS + // default write buffer is larger than any payload here. + child + .stdin + .take() + .expect("stdin") + .write_all(payload.as_bytes()) + .expect("write payload"); + child.wait().expect("wait for binary") +} + +#[test] +fn clean_payload_exits_zero() { + assert!(run_binary(CLEAN_PAYLOAD).success()); +} + +#[test] +fn flagged_payload_exits_with_the_blocked_contract() { + let status = run_binary(FLAGGED_PAYLOAD); + assert_eq!( + status.code(), + Some(i32::from(BLOCKED_EXIT_CODE)), + "flagged payload must exit {BLOCKED_EXIT_CODE} — this constant is \ + duplicated in .github/workflows/release.yml smoke step; changing it \ + requires updating both" + ); +} diff --git a/scripts/deno.jsonc b/scripts/deno.jsonc index b3db6d3..106a5c0 100644 --- a/scripts/deno.jsonc +++ b/scripts/deno.jsonc @@ -3,12 +3,14 @@ "@std/assert": "jsr:@std/assert@1.0.19", "@std/cli": "jsr:@std/cli@1.0.15", "@std/path": "jsr:@std/path@1.1.6", + "@std/yaml": "jsr:@std/yaml@1.0.4", "@libs/diff": "jsr:@libs/diff@4.0.0" }, "tasks": { "manifest:generate": "./tools/generate-platform-manifest.ts", "manifest:sync-root": "./tools/sync-root-version.ts", "check-matrix": "./tools/check-matrix.ts", + "test": "deno test --allow-read .", "check:publish": "./tools/check-publish.ts", "publish:unpublished": "./tools/publish-and-setup-npm-trust.ts", "lint": "deno lint --config ./deno.jsonc ." diff --git a/scripts/deno.lock b/scripts/deno.lock index 0ea2b72..70b46c8 100644 --- a/scripts/deno.lock +++ b/scripts/deno.lock @@ -6,7 +6,8 @@ "jsr:@std/cli@1.0.15": "1.0.15", "jsr:@std/internal@^1.0.12": "1.0.14", "jsr:@std/internal@^1.0.14": "1.0.14", - "jsr:@std/path@1.1.6": "1.1.6" + "jsr:@std/path@1.1.6": "1.1.6", + "jsr:@std/yaml@1.0.4": "1.0.4" }, "jsr": { "@libs/diff@4.0.0": { @@ -29,6 +30,9 @@ "dependencies": [ "jsr:@std/internal@^1.0.14" ] + }, + "@std/yaml@1.0.4": { + "integrity": "f1e38fa5e110ff8550e938715e52c33347676dd00a1db858e092dd21a9df9941" } }, "workspace": { @@ -36,7 +40,8 @@ "jsr:@libs/diff@4.0.0", "jsr:@std/assert@1.0.19", "jsr:@std/cli@1.0.15", - "jsr:@std/path@1.1.6" + "jsr:@std/path@1.1.6", + "jsr:@std/yaml@1.0.4" ] } } diff --git a/scripts/lib/matrix-rows.ts b/scripts/lib/matrix-rows.ts new file mode 100644 index 0000000..38e9a4d --- /dev/null +++ b/scripts/lib/matrix-rows.ts @@ -0,0 +1,42 @@ +import { parse as parseYaml } from '@std/yaml' + +export interface MatrixRow { + target: string + suffix: string +} + +/** + * Extract the release matrix include rows (target + suffix) from a workflow + * document by parsing YAML, not scraping text (issue #8). + * + * Formatting cannot break agreement: flow-style lists, quoted keys or values, + * key reordering, and comments all parse to the same typed rows. Returns an + * empty list when no job carries a `strategy.matrix.include` list — callers + * must treat that as a failure, since an empty matrix cannot agree with the + * targets table. + */ +export function matrixRows(workflowText: string): MatrixRow[] { + const doc: unknown = parseYaml(workflowText) + if (typeof doc !== 'object' || doc === null) return [] + const jobs = (doc as Record).jobs + if (typeof jobs !== 'object' || jobs === null) return [] + for (const job of Object.values(jobs as Record)) { + if (typeof job !== 'object' || job === null) continue + const strategy = (job as Record).strategy + if (typeof strategy !== 'object' || strategy === null) continue + const matrix = (strategy as Record).matrix + if (typeof matrix !== 'object' || matrix === null) continue + const include = (matrix as Record).include + if (!Array.isArray(include)) continue + const rows: MatrixRow[] = [] + for (const row of include) { + if (typeof row !== 'object' || row === null) continue + const record = row as Record + if (typeof record.target === 'string' && typeof record.suffix === 'string') { + rows.push({ target: record.target, suffix: record.suffix }) + } + } + if (rows.length > 0) return rows + } + return [] +} diff --git a/scripts/lib/platform-manifest.ts b/scripts/lib/platform-manifest.ts index e422f43..c217232 100644 --- a/scripts/lib/platform-manifest.ts +++ b/scripts/lib/platform-manifest.ts @@ -9,6 +9,7 @@ export interface PlatformPackageManifest { os: [string] cpu: [string] files: [string] + peerDependencies: Record publishConfig: { access: 'public'; provenance: true } libc?: [string] binarySha256?: string @@ -29,6 +30,11 @@ export function buildPlatformManifest( os: [entry.os], cpu: [entry.cpu], files: [entry.bin], + // Issue #4: a direct install of the platform package declares the + // launcher it belongs to. npm auto-installs the peer, so a raw platform + // install resolves to a launcher-consistent set instead of surfacing a + // version-skewed BinaryNotFound at runtime. + peerDependencies: { [launcher.name]: version }, // No bin field — a platform package's bin would collide with the launcher's // own comment-checker shim. publishConfig: { access: 'public', provenance: true }, diff --git a/scripts/tools/check-matrix.test.ts b/scripts/tools/check-matrix.test.ts new file mode 100644 index 0000000..36d789e --- /dev/null +++ b/scripts/tools/check-matrix.test.ts @@ -0,0 +1,81 @@ +import { assertEquals, assertThrows } from '@std/assert' +import { matrixRows } from '../lib/matrix-rows.ts' + +// Issue #8 regression fixture: the workflow must be parsed as YAML, so +// formatting variants (flow-style include list, quoted target, reordered +// keys, comments) resolve to the same typed rows the regex scraper used to +// miss — which let the gate pass vacuously. +const FLOW_STYLE = `name: Release +on: + push: + tags: ['v*'] # quoted scalar in a flow list + +jobs: + release: + strategy: + fail-fast: false + matrix: + include: [ + { target: "x86_64-unknown-linux-gnu", suffix: linux-x64, runner: ubuntu-latest }, + { target: aarch64-unknown-linux-gnu, runner: "ubuntu-24.04-arm", suffix: linux-arm64 }, + { suffix: darwin-x64, target: x86_64-apple-darwin }, + { target: aarch64-apple-darwin, suffix: darwin-arm64 }, + { target: x86_64-pc-windows-msvc, suffix: win32-x64 }, + ] +` + +const BLOCK_STYLE = `name: Release +on: + push: + tags: + - 'v*' +jobs: + release: + strategy: + fail-fast: false + matrix: + include: + - target: x86_64-unknown-linux-gnu + suffix: linux-x64 + runner: ubuntu-latest + - target: aarch64-unknown-linux-gnu + suffix: linux-arm64 + runner: ubuntu-24.04-arm + - target: x86_64-apple-darwin + suffix: darwin-x64 + runner: macos-13 + - target: aarch64-apple-darwin + suffix: darwin-arm64 + runner: macos-14 + - target: x86_64-pc-windows-msvc + suffix: win32-x64 + runner: windows-2022 +` + +const EXPECTED: Array<[string, string]> = [ + ['x86_64-unknown-linux-gnu', 'linux-x64'], + ['aarch64-unknown-linux-gnu', 'linux-arm64'], + ['x86_64-apple-darwin', 'darwin-x64'], + ['aarch64-apple-darwin', 'darwin-arm64'], + ['x86_64-pc-windows-msvc', 'win32-x64'], +] + +Deno.test('matrixRows parses the block-style workflow', () => { + const rows = matrixRows(BLOCK_STYLE) + assertEquals(rows.map((r) => [r.target, r.suffix]), EXPECTED) +}) + +Deno.test('matrixRows parses the flow-style workflow (issue #8 regression)', () => { + const rows = matrixRows(FLOW_STYLE) + assertEquals(rows.map((r) => [r.target, r.suffix]), EXPECTED) +}) + +Deno.test('matrixRows throws on malformed YAML (gate must fail loudly)', () => { + // A parse error must propagate so the check-matrix CLI reports FAIL rather + // than comparing against an empty row set. + assertThrows(() => matrixRows('jobs: [unclosed')) +}) + +Deno.test('matrixRows ignores jobs without a matrix include', () => { + assertEquals(matrixRows('jobs:\n lint:\n runs-on: ubuntu-latest\n'), []) +}) diff --git a/scripts/tools/check-matrix.ts b/scripts/tools/check-matrix.ts index 757d3ce..12709fc 100755 --- a/scripts/tools/check-matrix.ts +++ b/scripts/tools/check-matrix.ts @@ -1,6 +1,7 @@ #!/usr/bin/env -S deno run --allow-read import { resolve } from '@std/path' import { parseCliArgs } from '../lib/cli.ts' +import { matrixRows } from '../lib/matrix-rows.ts' import { LAUNCHER_MANIFEST_PATH, type LauncherManifest, @@ -115,12 +116,10 @@ async function checkWorkflow(workflowPath: string, targets: Target[]) { try { await Deno.lstat(workflowPath) const content = await Deno.readTextFile(workflowPath) - // Each matrix row lists its suffix on the line after target. - const workflowPairs = new Map( - [...content.matchAll( - /^\s*-\s*target:\s*((?:x86_64|aarch64)-[a-z0-9-]+)\s*\n\s*suffix:\s*([a-z0-9-]+)\s*$/gm, - )].map((m) => [m[1], m[2]]), - ) + // Typed YAML parse (issue #8): formatting variants (flow style, quoting, + // key order) must not change what rows are seen, and malformed YAML must + // fail the gate instead of yielding an empty match set. + const workflowPairs = new Map(matrixRows(content).map((row) => [row.target, row.suffix])) const tablePairs = new Map(targets.map((t) => [t.target, t.suffix])) for (const [target, suffix] of tablePairs) { if (!workflowPairs.has(target)) {