test: DO NOT MERGE — verify CI catches a type-invalid test fixture - #37
Closed
kewang wants to merge 4 commits into
Closed
test: DO NOT MERGE — verify CI catches a type-invalid test fixture#37kewang wants to merge 4 commits into
kewang wants to merge 4 commits into
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
Removes the required 'vcs' field from a WorktreeInfo fixture in packages/core/src/aggregate.test.ts. Reproduces the exact hole from issue #16: npm test passes 271 assertions, npm run type-check exits 2. Throwaway branch. Verifies task 9.4 of add-ci-and-npm-publish-automation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgfFLCcVjPxZJmMspbY4Bu
Collaborator
Author
|
Verified. |
kewang
added a commit
that referenced
this pull request
Aug 4, 2026
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.
Throwaway PR verifying task 9.4 of #36. Close without merging.
Removes the required
vcsfield from aWorktreeInfofixture inpackages/core/src/aggregate.test.ts— the exact defect issue #16 documents.Locally:
npm testpasses all 271 assertions,npm run type-checkexits 2. Before #36 nothing in CI ran either check, and even after adding one, atype-checkthat skipped test files would still go green here.Expected: Node gates fails at the Type check step. If it passes, the hole is not actually closed.
Refs #16