feat(ci): add quality gates and automate the npm package release line - #36
Merged
Conversation
Nothing ran tests, type checks, or lint automatically: all three existing workflows publish or deploy on a tag, none run on pull_request (issue #16). Add .github/workflows/ci.yml on pull_request + push:[master] with three jobs — the Node gates (test, type check, lint, build, build:demo under NODE_ENV=production), the Kotlin suite, and a smoke test of the composite action. The type check previously covered only @spekjs/web, and core/ui excluded their own tests from checking while the suites run through tsx, which strips types without checking them. That pair of holes was hiding two live TS2741s in core's aggregate.test.ts while npm test reported every test passing. Test files are now checked through a separate tsconfig.test.json per package rather than by deleting the exclude — those configs also drive the build, which emits into the published dist, so deleting it would ship compiled tests to consumers. scripts/build-demo.ts and scripts/generate-badges.ts were named by no tsconfig at all, despite being what the composite action and the demo page execute. They now have one. The root tsconfig.json — project references listing 2 of 4 packages, invoked by nothing — is removed. Lint is real for the first time: packages/web declared lint and format scripts while neither eslint nor prettier was installed and no config existed. Prettier is deliberately not added; it would rewrite nearly every file for no defect caught. The smoke job pins spek-version to the commit under test — the action checks out spekhq/spek at that ref and builds from that copy, so the default of "master" would test master's action implementation and report green on a change that breaks it. It asserts on file contents, not on outputs being set: an output is populated whether or not the build produced anything, which is how the ui-dist break shipped. Fixes found by the newly-enabled gates: - core/aggregate.test.ts: missing vcs on two WorktreeInfo fixtures - vscode/panel.ts: getHtml read index.webview.html on every render and discarded it - vscode/extension.ts: two require() calls in a file already using ESM imports - web/useOpenSpec.ts: an unused ref and an unused type import - web/useScrollspy.ts: effect now keyed on the joined ids, no longer references the unstable array (behavior identical — slugs cannot contain the separator) - web/SearchDialog.tsx: the filtered lists are memoized, so the flatResults memo they feed actually memoizes instead of recomputing every render Refs #16 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgfFLCcVjPxZJmMspbY4Bu
…ease tags @spekjs/core and @spekjs/ui were published entirely by hand — bump, npm publish, then a chore(npm) commit — and neither version line had a single git tag, so "what changed between core 1.3.0 and 1.4.0" could not be answered from the repository. Add .github/workflows/npm-publish.yml: on push to master it compares each package's declared version against the registry and publishes the ones that differ, then creates a core-vX.Y.Z / ui-vX.Y.Z tag. An already-published version is a quiet skip, not a failure — nearly every push to master changes no version, and a workflow that errors there would make red the normal state of master. Authentication is npm Trusted Publishing (OIDC), so no npm token is stored in this repository, and provenance is attached automatically. Three constraints are load- bearing and documented in the file: the workflow's filename is registered with npm and matched exactly (a rename fails only at authentication, naming no cause), the publish steps cannot be factored into a reusable workflow (npm validates the calling workflow's name), and npm must be upgraded first because the Node in .nvmrc bundles one older than trusted publishing requires. The gates run inside the publish job rather than chaining on ci.yml via workflow_run: that trigger evaluates the workflow file from the default branch rather than the triggering commit, and getting its coupling wrong publishes on a red build, which cannot be undone. Version detection checks membership of the full version list rather than `npm view <pkg>@<version>`, whose E404 exit conflates "not published" with network, auth and registry failures — and a failure read as "not published" is a failure read as "publish it". Backfill the twelve tags for every version already on the registry, each verified against the version its commit actually declares. Three of them predate the chore(npm) message convention, which is why the convention cannot be the anchor. The release skill gains the step CI cannot do: deciding the increment. It reads the archived changes' Impact rather than commit prefixes, which get this wrong about half the time here — core 1.3.0 and 1.4.0 were both `fix:` commits that warranted a minor. Refs #16 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgfFLCcVjPxZJmMspbY4Bu
The first run of this workflow failed with "TS2307: Cannot find module '@spekjs/ui'" plus a spray of implicit-any errors in the files importing it. @spekjs/web type-checks against ui's dist/, not its sources, and `npm ci` does not produce that dist: ui's build is deliberately on prepublishOnly, because a prepare hook would run before npm creates the workspace symlinks and take the whole install down. The job built core and stopped there. Invisible locally — a dist from an earlier build is always lying around — and this is the same trap that shipped the composite action broken, which the github-action capability already carries a requirement for. design.md stated the constraint correctly; the spec requirement and both workflows only implemented half of it. Reproduced by deleting both dists: 50 type errors before, 0 after. Widen the requirement from "build core" to "build every workspace package resolved through its dist", with a scenario for the next package that acquires one, and fix the same omission in npm-publish.yml. Refs #16 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgfFLCcVjPxZJmMspbY4Bu
…po PR spek-version pinned to github.sha resolves and the smoke job passes (PR #36). The fork case is still unobserved and stays an open question, with the push:master fallback recorded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgfFLCcVjPxZJmMspbY4Bu
All three jobs green on #36. #37 confirmed the closed hole: Test passed 267 assertions, Type check then failed with the expected TS2741. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgfFLCcVjPxZJmMspbY4Bu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #16.
Why
Nothing ran tests, type checks, or lint automatically — all three existing workflows publish or deploy on a tag, none run on
pull_request. Every gate was a command a human remembered to type.That was hiding a live defect:
packages/core/src/aggregate.test.tsfailed to type-check with twoTS2741s whilenpm testreported 60/60 passing. Two holes lined up — a roottype-checkcovering only@spekjs/web, and package tsconfigs excluding their own tests while the suites run throughtsx, which strips types without checking them.Separately,
@spekjs/core/@spekjs/uiwere published entirely by hand, and neither version line had a single git tag.What's here
Gates —
ci.ymlonpull_request+push:[master]: Node gates (test, type check, lint, build,build:demounderNODE_ENV=production), the Kotlin suite on JDK 17, and a smoke test of the composite action.Type check now covers core / ui / web / vscode and
scripts/—build-demo.tsandgenerate-badges.tswere named by no tsconfig at all, despite being what the action and the demo page execute. Test files are included via a per-packagetsconfig.test.jsonrather than by deleting theexclude: those configs also drive the build, which emits into the publisheddist, so deleting it would ship compiled tests to consumers.Lint is real for the first time.
packages/webdeclaredlintandformatscripts while neither eslint nor prettier was installed and no config existed anywhere. Prettier is deliberately not added — it would rewrite nearly every file for no defect caught.Publishing —
npm-publish.ymlpublishes each package when its declared version differs from the registry, via Trusted Publishing (OIDC, no stored token, provenance automatic), then tagscore-vX.Y.Z/ui-vX.Y.Z. An already-published version is a quiet skip, not a failure. Twelve backfill tags cover every version already on the registry.The version decision stays in the
releaseskill, read from the archived changes' Impact — never from commit prefixes, which get it wrong about half the time here (core 1.3.0 and 1.4.0 were bothfix:commits that warranted a minor).Bugs the new gates caught
vscode/panel.ts—getHtml()readindex.webview.htmlon every render and discarded itweb/SearchDialog.tsx— theflatResultsmemo recomputed every render, its deps being freshly-built arraysweb/useOpenSpec.ts— an unused ref and an unused type importweb/useScrollspy.ts— effect now keyed on the joined ids instead of the unstable array (behavior identical; slugs cannot contain the separator)vscode/extension.ts— tworequire()calls in a file already using ESM importsNotes for review
spek-version: ${{ github.sha }}. The action checks outspekhq/spekat that ref and builds from that copy, so the defaultmasterwould test master's implementation and go green on a PR that breaks the action. Whether this resolves for a fork PR is the one open question — if it doesn't, the fallback is restricting the job topush: master.npm-publish.yml's filename is registered with npm and matched exactly. Renaming it leaves a workflow that runs, resolves the version difference, and fails only at authentication, naming no cause.ci.ymlviaworkflow_run— that trigger evaluates the workflow file from the default branch rather than the triggering commit, and getting its coupling wrong publishes on a red build, which cannot be undone.🤖 Generated with Claude Code
https://claude.ai/code/session_01PgfFLCcVjPxZJmMspbY4Bu