ci: test the shipped package, lint workflows, rescan on acceptance, verify publishes - #211
Conversation
…erify publishes Same change as #210, as one commit.
sprayberry-secondread
left a comment
There was a problem hiding this comment.
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 --versionprinted usage and exited 2": confirmed from the base dispatch atsrc/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-52COPY truecopy.lock/COPY demo/clean-mcp.json, so adding both todocker.ymlpaths is right. - marketplace-watch push paths:
support/marketplace-watch.mjsimports../src/index.mjsand./evidence.mjs, and both are covered (src/**,support/evidence.mjs).support/watch-accepted.jsonis the acceptance input. Nogithub.event_nameconditionals exist in the workflow, so apushrun takes the same path as a scheduled one. verify-attestation: 'true': this is a real input on the pinnedtruecopy-action@3983d25(gh attestation verify --owner askalf, tarball path). Theverify pinned skillslog at this head showsVerifying 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 showspacked: askalf-truecopy-0.10.4.tgzandtruecopy-mcp tools: truecopy-verify, truecopy-status. Withfiles: ["src"]and nonpm ciat the repo root, an import that reaches outsidesrc/would fail there, as intended. - actionlint download is pinned by version and sha256, and is checked with
sha256sum -cbefore 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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
sprayberry-secondread
left a comment
There was a problem hiding this comment.
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
doneScenario: 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
doneBoundaries 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
packagejob on ubuntu+windows at node 20; actionlint pinned by version and sha256, withsha256sum -cgating the download; marketplace-watch push paths plus aconcurrencygroup withcancel-in-progress: false; the publish verify step;verify-attestation: 'true'; CodeQLactions; docker paths. The Dockerfile really doesCOPY truecopy.lockanddemo/clean-mcp.json(Dockerfile:51-52), so those new path filters are justified. - Gate attestation actually runs: the
verify pinned skillslog at this head showsVerifying Sigstore attestation for askalf-truecopy-0.10.3.tgz…thentruecopy 0.10.3 installed from the signed release tarball. The workflow'spermissions: read-allsuppliesattestations: read. - Security surface: the new marketplace-watch trigger is
pushtomasteronly, 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 existingid-token: write. - CI at
f242672: all 18 checks pass, includingteston 3 OSes × Node 20/22/24, bothpacked tarballjobs,actionlint, andanalyze (actions). - Not read: the full
support/marketplace-*.mjssources (only thehistory.jsonlhandling, 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
left a comment
There was a problem hiding this comment.
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.
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:
ci.ymlpackagejob: installs thenpm packtarball into an empty project (ubuntu + windows, node 20), then runs the installed CLI,truecopy-mcpand the library./srcstraight from the checkout, so a broken published package was invisibleci.ymlpublish.ymlalready usesci.ymlrun:script through shellcheckmarketplace-watch.ymlwatch, so two runs must never overlap.publish.ymlnpm publish: install that version back from the registry, check--version, check it flags the poisoned demo, runnpm audit signaturestruecopy-gate.ymlverify-attestation: 'true'codeql.ymlactionslanguagedocker.ymltruecopy.lockordemo/clean-mcp.jsonchangetest/release-hygiene.test.mjspackage.jsonBug fix:
truecopy --versionprinted 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,-vandversionnow 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.actionlint1.7.12 with shellcheck 0.11.0: clean on all workflowspackagejob's two scripts locally, taken straight fromci.yml. The installed CLI reports its version, the clean demo passes, the poisoned demo exits 1,verifypasses,truecopy-mcpliststruecopy-verify, truecopy-status, and the library has 33 exports.npm audit signaturesagainst the published@askalf/truecopy@0.10.3: 1 verified registry signature, 1 verified attestationverify-attestationhas something to verifyReplaces #210 with the same change as one commit, without model attribution; em dashes in the added text are rewritten.