Skip to content

ci: test the shipped package, lint workflows, rescan on acceptance, verify publishes - #211

Merged
askalf merged 4 commits into
masterfrom
ci/test-shipped-package-clean
Sep 25, 2026
Merged

askalf merged 4 commits into
masterfrom
ci/test-shipped-package-clean

Conversation

@askalf

@askalf askalf commented Sep 25, 2026

Copy link
Copy Markdown
Owner

What this changes

This is the CI half of #208, which was closed before it landed. It's rebased onto master after the 0.10.4 release (#209) and has no release bump of its own.

I reviewed every workflow. All recent runs were green, and everything passes actionlint and shellcheck, so the gaps were in what CI never checked:

Workflow Change Why
ci.yml New package job: installs the npm pack tarball into an empty project (ubuntu + windows, node 20), then runs the installed CLI, truecopy-mcp and the library The test suite imports ./src straight from the checkout, so a broken published package was invisible
ci.yml Node 24 added to the matrix Active LTS, and what publish.yml already uses
ci.yml New actionlint job, with the binary pinned by version and sha256 Lints every workflow, and every run: script through shellcheck
marketplace-watch.yml Also runs on master pushes that touch acceptances or detection; runs are serialized #207 merged 40 minutes after the run that flagged its skill, so the badge stayed red until a manual dispatch. Every run also force-pushes watch, so two runs must never overlap.
publish.yml After npm publish: install that version back from the registry, check --version, check it flags the poisoned demo, run npm audit signatures A green publish only means npm accepted the upload
truecopy-gate.yml verify-attestation: 'true' The self-gate now checks the Sigstore provenance of the tarball it installs
codeql.yml Adds the actions language CodeQL also scans the workflows themselves
docker.yml Also runs when truecopy.lock or demo/clean-mcp.json change Both are copied into the image, and its smoke test verifies them
test/release-hygiene.test.mjs Checks that the CHANGELOG section, lockfile version and documented pins all match package.json Catches a version bump that leaves any of them behind

Bug fix: truecopy --version printed the usage text and exited 2, even though both issue templates ask reporters for its output. The just-published 0.10.4 still does this. --version, -v and version now print the version. Regression test added, and the CHANGELOG entry is under [Unreleased].

Why

CI tested the source tree but not the package users install. It also didn't confirm a publish was actually usable, and it left the watch badge red after a triage PR merged.

How it was verified

  • npm test: 274 tests, 273 pass, 0 fail, 1 skipped (Node 22). Before the new tests, the suite also passed on Node 24.21.0 locally.
  • actionlint 1.7.12 with shellcheck 0.11.0: clean on all workflows
  • I ran the package job's two scripts locally, taken straight from ci.yml. The installed CLI reports its version, the clean demo passes, the poisoned demo exits 1, verify passes, truecopy-mcp lists truecopy-verify, truecopy-status, and the library has 33 exports.
  • npm audit signatures against the published @askalf/truecopy@0.10.3: 1 verified registry signature, 1 verified attestation
  • The v0.10.3 release tarball has a Sigstore attestation, so the gate's verify-attestation has something to verify
  • Detection change? N/A
  • Acceptance change? N/A

Replaces #210 with the same change as one commit, without model attribution; em dashes in the added text are rewritten.

…erify publishes

Same change as #210, as one commit.
@github-actions github-actions Bot added documentation Improvements or additions to documentation github_actions Pull requests that update GitHub Actions code cli Command-line surface watch Marketplace watch: scanner harness and acceptance ledger tests Test suite and CI size/L 200-799 hand-written lines labels Sep 25, 2026

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: No blocking issues. The one code change (--version) is correct and has a test that fails without it. The workflow additions match the PR body. CI is green at 0460014 on all 18 checks, including the new packed tarball (ubuntu and windows) and actionlint jobs.

Findings

1. Low / info: npm audit signatures does not fail when the provenance attestation is missing

.github/workflows/publish.yml (new step, last line)

+      # `npm audit signatures` check the registry signature and the provenance
+      # attestation OIDC publishing is supposed to attach.
...
+          npm audit signatures

npm audit signatures verifies any attestations that are present. It exits non-zero on invalid or missing registry signatures, but not on a package that simply has no provenance attestation. So if trusted publishing stopped attaching provenance (for example, a publish that fell back to token auth), this step would still pass. The comment says it checks the attestation OIDC publishing "is supposed to attach", which is stronger than what the step enforces. This isn't blocking: the step does what the PR body says ("run npm audit signatures"). It's a hardening option if the goal is to catch lost provenance.

Suggested fix (optional):

out="$(npm audit signatures 2>&1)"; printf '%s\n' "$out"
printf '%s' "$out" | grep -Eq '[0-9]+ packages? ha(s|ve) (a )?verified attestations?' \
  || { echo "::error::@askalf/truecopy@${v} has no verified provenance attestation"; exit 1; }

2. Info: stale comment next to the Node 24 change

.github/workflows/publish.yml:51 (unchanged context in a touched file) still says # ci.yml still tests the supported runtime floor (20, 22). After this PR, ci.yml tests [20, 22, 24]. It's a one-word fix if you touch the file again.

Boundaries I rebuilt from the diff

Predicate / guard Input Behaviour at head Pinned by
src/cli.mjs:503 cmd === '--version' || '-v' || 'version' each of the three prints PKG_VERSION, exit 0 test/cli-robustness.test.mjs new test, per flag: status === 0 and stdout.trim() === pkgVersion. Without the fix each flag falls to !table[cmd] → usage, exit 2, so both assertions fail for every variant (not vacuous).
same cmd undefined / -h / --help / unknown unchanged path at :506 existing behaviour
same -v colliding with an existing flag none: no -v anywhere else in src/ (grep); cmd is pre[0] only, so truecopy scan -v is unaffected n/a
same PKG_VERSION null (unreadable package.json) prints unknown, exit 0 untested. It needs a broken install, and the fallback matches :464's handling
release-hygiene CHANGELOG regex requires - YYYY-MM-DD ## [Unreleased] excluded (no date), so an Unreleased-only entry cannot satisfy it inherent
release-hygiene pin tests regex matches nothing found.length > 0 assertion fails, so a renamed or reworded doc cannot pass vacuously itself
publish verify: registry poll loop version never appears in 12×10s loop falls through, then npm install fails the step n/a (CI-only)
publish verify gated on steps.gate.outputs.skip != 'true' already-published re-dispatch skipped, same as publish consistent with existing steps
marketplace-watch concurrency cancel-in-progress: false 3+ triggers in flight GitHub keeps one running and one pending. A newer arrival replaces the pending run, so the run that survives scans the newest master. Correct for this workflow. n/a

Claims checked against the diff

  • "truecopy --version printed usage and exited 2": confirmed from the base dispatch at src/cli.mjs:506 (!table[cmd] → usage(); exit 2). Both issue templates do ask for its output (bug.yml:43, false-positive.yml:51).
  • Docker paths: Dockerfile:51-52 COPY truecopy.lock / COPY demo/clean-mcp.json, so adding both to docker.yml paths is right.
  • marketplace-watch push paths: support/marketplace-watch.mjs imports ../src/index.mjs and ./evidence.mjs, and both are covered (src/**, support/evidence.mjs). support/watch-accepted.json is the acceptance input. No github.event_name conditionals exist in the workflow, so a push run takes the same path as a scheduled one.
  • verify-attestation: 'true': this is a real input on the pinned truecopy-action@3983d25 (gh attestation verify --owner askalf, tarball path). The verify pinned skills log at this head shows Verifying Sigstore attestation for askalf-truecopy-0.10.3.tgz… and the job passed.
  • The package job drives the installed bins from $RUNNER_TEMP/consumer/node_modules/.bin, not ./src. The windows log shows packed: askalf-truecopy-0.10.4.tgz and truecopy-mcp tools: truecopy-verify, truecopy-status. With files: ["src"] and no npm ci at the repo root, an import that reaches outside src/ would fail there, as intended.
  • actionlint download is pinned by version and sha256, and is checked with sha256sum -c before extraction.

What's good

The packed-tarball job closes a real blind spot (the suite imports ./src directly), and it proves itself here by exercising the new --version path from the installed bin. The release-hygiene tests guard against vacuous passes. The publish verification reuses the poisoned-demo check, so a published build that silently stopped detecting would go red.

Not read: auto-release.yml (unchanged), and the full marketplace-watch.yml job body beyond the trigger and concurrency lines. I did not run the test suite locally; CI is the test signal.

SECOND READ: READY

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: request changes — public added code contains patch-narration comments that read as generated. rule:reads-as-generated

Blocking — generated-writing tell

.github/workflows/ci.yml:48-51

# The test suite imports ./src straight from the checkout, so it cannot see a
# broken PUBLISHED package: a file left out of \files`, an import reaching # outside src/, a bin that no longer resolves. This installs the tarball # `npm pack` produces (the bytes npm publish would ship) into an empty project`

This comment narrates the patch rationale and history rather than documenting an enduring workflow contract. The first-party public-repo tell pass treats such patch narration as blocking. Keep only an operational description, or remove it if the job name and steps are already clear.

  package:
    name: packed tarball (${{ matrix.os }})

Blocking — generated-writing tell

src/cli.mjs:502

// The issue templates ask reporters for \truecopy --version`, so it must print the version, not usage.`

This comment explains why the newly added branch is correct and describes the prior behavior, rather than what the code does. It is a public generated-writing tell under the same rule. Delete it; the three explicit command comparisons make the behavior self-evident.

if (cmd === '--version' || cmd === '-v' || cmd === 'version') { out(PKG_VERSION || 'unknown'); process.exit(0); }

What's good: I reviewed the 215 added / 2 removed lines, including package-install, publish, workflow-trigger, CLI, and regression-test paths. Required CI is green at 0460014d2ce06899a4af2e94115be20bb0b23db7, including the new packed-tarball jobs on Ubuntu and Windows, actionlint, CodeQL Actions analysis, and Node 24 matrix jobs. I did not run the local suite, per review policy.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Verdict: changes requested - the newly public test comment narrates the release-patch rationale. rule:reads-as-generated

Findings

Blocking - generated-writing tell - test/release-hygiene.test.mjs:1-4

// A release is cut by bumping package.json alone (auto-release.yml), so nothing
// else would notice a bump that forgot its CHANGELOG section (the GitHub release
// silently falls back to "Release vX") or left the documented pins on the old
// version (users copy those lines verbatim).

This is patch narration explaining why the new test was added and what failure it is intended to prevent, rather than documentation of code behavior. It is public repository prose and remains a generated-writing tell under the first-party gate. A maintainer reading the file only needs the test names and assertions; the release history and justification belong in the PR or commit description.

import { test } from 'node:test';

I reviewed the live-head diff, including the packed-tarball and publish verification workflows, CLI version handling, and the new release-hygiene assertions. All required CI checks are green at 8ffc9bfb24458527fea954d725b3c7bc23d0f175; I did not run the test suite locally.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Verdict: changes requested: public added comments narrate the patch rationale/history rather than document behavior. rule:reads-as-generated

  • Blocking — .github/workflows/ci.yml:36

    # 20 is the engines floor; 24 is the active LTS and what publish.yml runs.

    This is patch rationale for the newly expanded matrix, not documentation of behavior that a maintainer needs to operate the workflow. In this first-party strict review, public prose that reads as generated/patched is blocking; it also makes the matrix needlessly carry release-policy narration in code. Remove the narration and leave the self-explanatory matrix entry.

          node: [20, 22, 24]
    
  • Blocking — .github/workflows/marketplace-watch.yml:18-20

    # Re-scan as soon as an acceptance or a detector change lands. Without this, a
    # merged triage PR left the badge and WATCH.md red until the next daily run
    # (2026-09-24: #207 merged 40 minutes after the run that flagged its skill).

    These lines explain the history and justification of the new trigger rather than the workflow's behavior; the dated PR narration is particularly a generated-patch tell in public configuration. Keep the trigger and, if a comment is needed, state only the enduring behavior.

    push:
      branches: [master]
      paths:
    

Required CI is green at b1abe8d12c999546840e20d84877d1547213735f. I reviewed the live diff, including the package/publish workflow paths, CLI version dispatch, and the added hygiene tests; no separate correctness defect was identified in that pass.

@github-actions github-actions Bot added size/M 50-199 hand-written lines and removed size/L 200-799 hand-written lines labels Sep 25, 2026

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: no blocking issues at f242672. The --version fix is correct and its test fails without it. Every claim in the PR body matches the diff. I have two low-severity notes; neither blocks.

Note on head: the ticket pins 8ffc9bf, but the live head is f242672 (ci: comments state what the workflows do, not why they changed), so this read covers f242672. The commits after 0460014 only remove comments (net +2 / -21 across six files) and change no behaviour.

Findings

1. Low: stale comment, since the matrix now includes 24

.github/workflows/publish.yml:51 (unchanged line, but this PR makes it wrong)

          # ci.yml still tests the supported runtime floor (20, 22).

ci.yml:34 now reads node: [20, 22, 24]. The next reader who compares this comment with the matrix will wonder which one is right.

Suggested fix:

          # ci.yml tests the supported runtimes (20, 22, 24).

2. Low: when the registry poll times out, the loop falls through silently

.github/workflows/publish.yml:97-101

          for i in $(seq 1 12); do
            if npm view "@askalf/truecopy@${v}" version >/dev/null 2>&1; then break; fi
            echo "waiting for @askalf/truecopy@${v} to appear on the registry (${i}/12)…"
            sleep 10
          done

Scenario: the packument takes more than 120s to show the new version. The loop ends without an error, and npm install "@askalf/truecopy@${v}" fails with ETARGET. The job still fails, which is the right outcome. But the log reads like a broken package when the actual cause is registry lag, and the step runs after npm publish, so someone will be looking at this log under pressure.

Suggested fix:

          for i in $(seq 1 12); do
            if npm view "@askalf/truecopy@${v}" version >/dev/null 2>&1; then break; fi
            [ "$i" -lt 12 ] || { echo "::error::@askalf/truecopy@${v} not on the registry after 120s"; exit 1; }
            echo "waiting for @askalf/truecopy@${v} to appear on the registry (${i}/12)…"
            sleep 10
          done

Boundaries rebuilt from the diff

Row Input Fixed behaviour Pinned by
src/cli.mjs:502 cmd === '--version' --version prints PKG_VERSION, exit 0 test/cli-robustness.test.mjs:29-33 (loop variant --version)
same, -v -v same same test, variant -v
same, version version same same test, variant version
other path no args / -h / unknown (-V, VERSION) falls through to the existing usage branch at :505 (exit 0 / 0 / 2), unchanged existing behaviour, not touched
-v shadowing -v used as another flag? grep finds no other -v handling in src/, so nothing is shadowed n/a
`PKG_VERSION 'unknown'` package.json unreadable
release-hygiene found.length > 0 a pin regex that matches nothing fails with the file and regex named test/release-hygiene.test.mjs:29
release-hygiene version mismatch any pin/lock/CHANGELOG behind package.json fails, naming the file and both versions :17-18, :30, :12
MCP handshake in package job server answers no tools/list process.exit(1) the CI job itself (green on ubuntu and windows)
poisoned demo exit code status != 1 job fails ci.yml package job and publish.yml:110-111

Can the new tests fail? Yes. Without the cli.mjs:502 line, all three variants hit usage() and exit 2, so assert.equal(r.status, 0, flag) fails for each. The stdout === pkgVersion assertion reads the same package.json the CLI reads, but the status assertion already makes the test non-vacuous. The release-hygiene tests read the real files and fail on any drift.

What I checked

  • Every PR-body claim against the diff: Node 24 in the matrix; the package job on ubuntu+windows at node 20; actionlint pinned by version and sha256, with sha256sum -c gating the download; marketplace-watch push paths plus a concurrency group with cancel-in-progress: false; the publish verify step; verify-attestation: 'true'; CodeQL actions; docker paths. The Dockerfile really does COPY truecopy.lock and demo/clean-mcp.json (Dockerfile:51-52), so those new path filters are justified.
  • Gate attestation actually runs: the verify pinned skills log at this head shows Verifying Sigstore attestation for askalf-truecopy-0.10.3.tgz… then truecopy 0.10.3 installed from the signed release tarball. The workflow's permissions: read-all supplies attestations: read.
  • Security surface: the new marketplace-watch trigger is push to master only, so no PR code runs in the write-token job. The publish verify step installs only the version this same job just published, and runs with no extra permissions beyond the job's existing id-token: write.
  • CI at f242672: all 18 checks pass, including test on 3 OSes × Node 20/22/24, both packed tarball jobs, actionlint, and analyze (actions).
  • Not read: the full support/marketplace-*.mjs sources (only the history.jsonl handling, to confirm extra push-triggered runs only add per-run lines, which matches the file's stated per-run semantics). I did not run the suite locally.

What's good: the package job closes a real gap, because the suite imports ./src and never exercised the packed files: ["src"] artifact. The Windows cygpath / bare-tarball-name handling shows it was actually run on that platform.

SECOND READ: READY

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: approve — no blocking issues found in the live-head re-review.

I reviewed the full PR diff and the live-head follow-up diff from b1abe8d12c999546840e20d84877d1547213735f to f242672a3ad912bec57385691897e2a445449c13. The follow-up removes the patch-narration comments without changing workflow behavior. I also checked the version-command branches and their regression coverage, package/publish workflow paths, and all current CI checks. Required CI and the added actionlint, packed-tarball (Ubuntu/Windows), CodeQL, test-matrix, smoke, and gate checks are green at f242672a3ad912bec57385691897e2a445449c13.

What's good: the package job exercises the installed tarball rather than checkout sources, and the version aliases are covered for status and output.

@askalf
askalf merged commit 6995e6f into master Sep 25, 2026
18 checks passed
@askalf
askalf deleted the ci/test-shipped-package-clean branch September 25, 2026 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Command-line surface documentation Improvements or additions to documentation github_actions Pull requests that update GitHub Actions code size/M 50-199 hand-written lines tests Test suite and CI watch Marketplace watch: scanner harness and acceptance ledger

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants