Add a CI job for the TypeScript half, and correct a stale security comment - #133
Merged
Conversation
…mment The TS half had no automated gate at all. Six PRs of real application code — the pnpm workspace, @vsms/env, @vsms/ui, the gateway/tRPC/hooks spine, the composer, the messages list — landed with nothing checking that they built, linted, typechecked, or passed their own tests. The only Node step in this workflow installed mermaid-cli for docs. This repo has already paid for this exact mistake once: AGENTS.md records 14 live-Postgres suites that CI never ran, one of which hid a policy bug for a whole milestone. The new `js` job runs corepack-pinned pnpm, `biome ci`, and `turbo run typecheck build test`. It sets SKIP_ENV_VALIDATION=1 because `next build` sets NODE_ENV=production while *compiling*, which would otherwise trip @vsms/env's deploy-time rules (production requires DASHBOARD_AUTH and an https upstream) — deployment guarantees that CI can neither satisfy nor meaningfully check. Runtime boot still enforces them. Verified by reproducing the job locally from a wiped node_modules with the cache bypassed: `pnpm install --frozen-lockfile`, `biome ci .` clean over 82 files, and `turbo run typecheck build --force` reporting 7/7 tasks successful. That also answers a question the audit had to leave open — a clean checkout does build. Separately: `packages/api/src/context.ts`'s module doc still claimed a POST with no `Origin` header "is not treated as [cross-site] here". That was true before the header-absent case was closed, and the code has rejected it since; the doc was left behind. A stale comment that describes the unsafe behaviour as intended is worse than no comment — the next reader could "fix" the code to match it and silently reopen the hole. Corrected, with a note recording that the earlier wording was wrong so it does not drift back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
5 tasks
`typecheck` had no `dependsOn`, so turbo was free to schedule `admin:typecheck` (`tsc --noEmit`) before or alongside `admin:build` (`next build`). `admin/tsconfig.json` includes `.next/types/**/*.ts`, which only exists after a build, so the ordering mattered and was implicit. Honest scope: I could NOT reproduce a failure — 3 concurrent runs from a wiped `.next` and `.turbo` all passed, and `tsc` tolerates the glob matching nothing. This is defensive rather than a confirmed fix. It is worth doing anyway because the new `js` CI job runs both tasks together on a slower runner, which widens any such window, and because an ordering this load- bearing should be declared rather than left to the scheduler. `test` already declares the same dependency. Verified after the change: 7/7 tasks successful from clean with the cache bypassed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stephane-segning
added a commit
that referenced
this pull request
Aug 8, 2026
Owner directive, superseding the previous commit on this branch: auto- generated code must not go into version control. packages/sms-client is entirely written by `cratestack generate-typescript` from schema/schema.cstack, so it is now gitignored (packages/sms-client/*) with one deliberate exception — package.json stays tracked, because pnpm needs it on disk to resolve the workspace member and validate `pnpm install --frozen-lockfile` against the committed pnpm-lock.yaml, and that install has to succeed before generation is even possible to run. Its content is exactly what generation itself writes (confirmed: regenerating produces byte-identical bytes, since the generator's dependency list is schema-invariant boilerplate) — not a hand fork of it. packages/sms-client/GENERATING.md is the other tracked file: a short, hand-written note that exists in a fresh, ungenerated clone specifically to say "run `just client-gen` first," since the real generated README (which would otherwise carry that instruction) doesn't exist until generation has already happened once. Gate A (ci/assert-sms-client-matches-schema.mjs, "does the committed client match the schema") is deleted rather than kept: with nothing committed to diff against, it would assert nothing. Gate B (ci/assert-client-routes-match-server.mjs, "does every route the generated client calls exist on the pinned server's real route table") is the one that still matters — it catches the CLI-vs-library skew regardless of what's committed, since it compares live-generated output against a live-built sms-gateway binary. Re-verified it still fails correctly: pointing a route at a nonexistent path produced the same named failure as before, reverted cleanly afterward. justfile's client-check now depends on client-gen (`client-check: client-gen`) rather than assuming a committed client already exists, and only runs Gate B. Real bug found and fixed while wiring this up, not assumed: Turborepo's default task-hashing only considers git-tracked (or untracked-but-not- gitignored) files. Since packages/sms-client/src is now gitignored, `turbo run build` served a **stale cache hit** after mutating generated source content — confirmed live (mutate the gitignored file, rerun, watch it replay old logs instead of rebuilding). Fixed with an explicit `@vsms/sms-client#build` task override in turbo.json declaring `inputs: ["src/**", "package.json", "tsconfig.json"]`, which forces turbo to hash those globs regardless of .gitignore. Reverified with a clean three-step sequence (miss on first build, hit on an unchanged rebuild, miss again after mutating) — all three behaved correctly after the fix. CI (.github/workflows/ci.yml): added a `js` job — PR #133 (which adds an equivalent job) had not landed on main as of this branch, so there was nothing to attach to; a comment on the new job flags this for whoever merges both, so the eventual duplicate doesn't get missed. The job's order matches the one constraint that makes this work at all: `pnpm install --frozen-lockfile` runs first (succeeds because package.json is tracked), then Biome, then the published `cratestack` 0.7.8 CLI is installed via `cargo install cratestack-cli --version 0.7.8 --locked` (never a locally built binary — otherwise the gate only proves something on one machine) and used to generate the client, then Gate B, then `turbo run typecheck build` and `turbo run test`. Could not verify this job by actually running it in GitHub Actions from this environment; verified everything it does (install-then-generate ordering, Gate B, typecheck/build/test) by hand in a genuinely clean `git clone`, documented in the PR body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
stephane-segning
added a commit
that referenced
this pull request
Aug 8, 2026
Auto-generated code should not be in version control, so `packages/sms-client` is now produced by `just client-gen` rather than checked in. Two files stay tracked, each for a concrete reason: - `package.json`, because pnpm needs a manifest on disk to resolve the workspace member at all — `pnpm install --frozen-lockfile` has to succeed *before* generation can run. Its content is schema-invariant boilerplate and byte-identical to what the generator writes, so it is not a hand-forked copy that can drift. - `GENERATING.md`, hand-written, because in a fresh un-generated clone it is the only thing telling you to run `just client-gen` first. The generated README that would say so does not exist yet at that point. Gate A (`generate-typescript --check`) is deleted rather than kept: it existed to prove the *committed* client matched the schema, and with nothing committed there is no drift to detect. A check that asserts nothing is worse than no check. Gate B — `ci/assert-client-routes-match-server.mjs`, comparing every route the client calls against the pinned `sms-gateway routes` output — is now the load-bearing one and is verified to fail when a route is broken. CI generates before typechecking, installing the CLI from crates.io version-locked to the library pin. A client generated by a locally-built binary is reproducible on exactly one machine, which would defeat the point. Two real problems found while doing this, neither predictable from reading: - **Turborepo's default hashing ignores gitignored files.** Once the generated sources became untracked, `turbo run build` happily served a stale cache after they changed. Fixed with an explicit `inputs` override on `@vsms/sms-client#build`, reverified with a miss/hit/miss sequence. - **The TypeScript override was declared in two places.** T1 put it in `.pnpmrc`; this change needs it in `pnpm-workspace.yaml` because the generated `package.json` asks for `typescript@^7.0.2` and Next 15.5 predates TS 7. Both present produced `ERR_PNPM_LOCKFILE_CONFIG_MISMATCH` on a clean clone — invisible to any existing checkout with a warm `node_modules`. Consolidated on `pnpm-workspace.yaml`, pnpm 11's documented home for settings. Extends the existing `js` CI job rather than adding a second one, since #133 landed that job while this branch was open. Verified from a real `git clone` into a temp directory, not a wiped `node_modules`: `pnpm install --frozen-lockfile` succeeds, `just client-gen` generates, `just client-check` reports all 102 client calls matching 102 served routes, and `turbo run typecheck build` reports 8/8. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stephane-segning
added a commit
that referenced
this pull request
Aug 8, 2026
Auto-generated code should not be in version control, so `packages/sms-client` is now produced by `just client-gen` rather than checked in. Two files stay tracked, each for a concrete reason: - `package.json`, because pnpm needs a manifest on disk to resolve the workspace member at all — `pnpm install --frozen-lockfile` has to succeed *before* generation can run. Its content is schema-invariant boilerplate and byte-identical to what the generator writes, so it is not a hand-forked copy that can drift. - `GENERATING.md`, hand-written, because in a fresh un-generated clone it is the only thing telling you to run `just client-gen` first. The generated README that would say so does not exist yet at that point. Gate A (`generate-typescript --check`) is deleted rather than kept: it existed to prove the *committed* client matched the schema, and with nothing committed there is no drift to detect. A check that asserts nothing is worse than no check. Gate B — `ci/assert-client-routes-match-server.mjs`, comparing every route the client calls against the pinned `sms-gateway routes` output — is now the load-bearing one and is verified to fail when a route is broken. CI generates before typechecking, installing the CLI from crates.io version-locked to the library pin. A client generated by a locally-built binary is reproducible on exactly one machine, which would defeat the point. Two real problems found while doing this, neither predictable from reading: - **Turborepo's default hashing ignores gitignored files.** Once the generated sources became untracked, `turbo run build` happily served a stale cache after they changed. Fixed with an explicit `inputs` override on `@vsms/sms-client#build`, reverified with a miss/hit/miss sequence. - **The TypeScript override was declared in two places.** T1 put it in `.pnpmrc`; this change needs it in `pnpm-workspace.yaml` because the generated `package.json` asks for `typescript@^7.0.2` and Next 15.5 predates TS 7. Both present produced `ERR_PNPM_LOCKFILE_CONFIG_MISMATCH` on a clean clone — invisible to any existing checkout with a warm `node_modules`. Consolidated on `pnpm-workspace.yaml`, pnpm 11's documented home for settings. Extends the existing `js` CI job rather than adding a second one, since #133 landed that job while this branch was open. Verified from a real `git clone` into a temp directory, not a wiped `node_modules`: `pnpm install --frozen-lockfile` succeeds, `just client-gen` generates, `just client-check` reports all 102 client calls matching 102 served routes, and `turbo run typecheck build` reports 8/8. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stephane-segning
added a commit
that referenced
this pull request
Aug 8, 2026
…#135) Auto-generated code should not be in version control, so `packages/sms-client` is now produced by `just client-gen` rather than checked in. Two files stay tracked, each for a concrete reason: - `package.json`, because pnpm needs a manifest on disk to resolve the workspace member at all — `pnpm install --frozen-lockfile` has to succeed *before* generation can run. Its content is schema-invariant boilerplate and byte-identical to what the generator writes, so it is not a hand-forked copy that can drift. - `GENERATING.md`, hand-written, because in a fresh un-generated clone it is the only thing telling you to run `just client-gen` first. The generated README that would say so does not exist yet at that point. Gate A (`generate-typescript --check`) is deleted rather than kept: it existed to prove the *committed* client matched the schema, and with nothing committed there is no drift to detect. A check that asserts nothing is worse than no check. Gate B — `ci/assert-client-routes-match-server.mjs`, comparing every route the client calls against the pinned `sms-gateway routes` output — is now the load-bearing one and is verified to fail when a route is broken. CI generates before typechecking, installing the CLI from crates.io version-locked to the library pin. A client generated by a locally-built binary is reproducible on exactly one machine, which would defeat the point. Two real problems found while doing this, neither predictable from reading: - **Turborepo's default hashing ignores gitignored files.** Once the generated sources became untracked, `turbo run build` happily served a stale cache after they changed. Fixed with an explicit `inputs` override on `@vsms/sms-client#build`, reverified with a miss/hit/miss sequence. - **The TypeScript override was declared in two places.** T1 put it in `.pnpmrc`; this change needs it in `pnpm-workspace.yaml` because the generated `package.json` asks for `typescript@^7.0.2` and Next 15.5 predates TS 7. Both present produced `ERR_PNPM_LOCKFILE_CONFIG_MISMATCH` on a clean clone — invisible to any existing checkout with a warm `node_modules`. Consolidated on `pnpm-workspace.yaml`, pnpm 11's documented home for settings. Extends the existing `js` CI job rather than adding a second one, since #133 landed that job while this branch was open. Verified from a real `git clone` into a temp directory, not a wiped `node_modules`: `pnpm install --frozen-lockfile` succeeds, `just client-gen` generates, `just client-check` reports all 102 client calls matching 102 served routes, and `turbo run typecheck build` reports 8/8. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Adds a
jsCI job that lints, typechecks, builds and tests the TypeScript half, and corrects a stale module doc inpackages/api/src/context.tsthat described unsafe behaviour as intended.Intent
Found by a read-only audit ahead of a planned production release. The TypeScript half had no automated gate at all. Six PRs of real application code landed with nothing verifying they built:
@vsms/env+ Next scaffold@vsms/uiThe only Node step in
ci.ymlinstalled mermaid-cli for docs. This repo has already paid for this exact mistake:AGENTS.mdrecords 14 live-Postgres suites CI never ran, one of which hid a policy bug for an entire milestone.Scope
.github/workflows/ci.yml— newjsjob:corepack enable(pins pnpm frompackageManager),pnpm install --frozen-lockfile,pnpm biome ci .,pnpm turbo run typecheck build,pnpm turbo run test.SKIP_ENV_VALIDATION=1on that job, with the reasoning inline:next buildsetsNODE_ENV=productionwhile compiling, which trips@vsms/env's deploy-time rules (production requiresDASHBOARD_AUTH, requires anhttpsupstream). Those are deployment guarantees; CI has neither real auth config nor a TLS upstream, so enforcing them at build time protects nothing. Runtime boot still enforces them — unchanged.packages/api/src/context.ts— module doc corrected.The doc fix, and why it is not cosmetic
The doc claimed a POST with no
Originheader "is not treated as [cross-site] here". That was accurate before #129 closed the header-absent case; the code has rejected it since, and the doc was left behind.A stale comment asserting the unsafe behaviour is intended is worse than no comment: the next reader could "correct" the code to match it and silently reopen the hole that #129 fixed — where plain
curlwith no headers reachedcompose.send, which sends a real billed SMS. The replacement records that the earlier wording was wrong, so it does not drift back.Verification
Reproduced the CI job locally from a wiped
node_moduleswith the turbo cache bypassed:pnpm install --frozen-lockfile— cleanpnpm biome ci .— 82 files, no findingsSKIP_ENV_VALIDATION=1 pnpm turbo run typecheck build --force— 7/7 tasks successfulThat last run also settles a question the audit had to leave open: a clean checkout does build. Nothing had previously proven it.
Screenshots/Evidence
N/A — CI config and a comment.
Risk Assessment
Low, and net risk-reducing. Adds a gate where none existed. The one judgement call is
SKIP_ENV_VALIDATIONon the build job; the alternative is that no build can ever run in CI, and the runtime guarantee is untouched.AI Usage Declaration
Written by Claude Code (Claude Opus 5) after a read-only audit by a subagent identified the missing gate and the contradiction.
SKIP_ENV_VALIDATIONexemption does not weaken runtime enforcement.Reviewer Focus
Whether
SKIP_ENV_VALIDATION=1on the build job is the right trade, versus supplying a full dummy env in CI. I chose the exemption because a fabricatedhttpsupstream and fake cert paths would make the build pass for reasons unrelated to correctness.