* docs(#457): rewrite ADR-457 to ground truth and accept build-at-publish The prior draft asserted a codebase state that never existed (13 tsc-generated files, src/ trees, a tests/cjs-ts-parity.test.cjs). Corrected to verified ground truth (84 bin/lib .cjs, 1 value-baked package-identity.cjs, no tsc pipeline), distinguished value-baking from transpilation so package-identity stops being miscited as precedent, made check-in-the-artifact vs build-at-publish the central decision, and flipped status to Accepted (build-at-publish). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * build(#537): pilot TS build-at-publish for bin/lib (semver-compare) First hand-written module collapsed to a TypeScript source of truth per ADR-457. src/semver-compare.cts compiles (tsc, strict, noEmitOnError) to a gitignored get-shit-done/bin/lib/semver-compare.cjs. build:lib is wired into build, pretest, pretest:coverage, and prepublishOnly so the artifact is built before test and shipped on publish. Type-aware ESLint on src/**/*.cts immediately caught the params were over-typed as `unknown` (no-base-to-string); narrowed to a honest VersionInput domain type. Behavior preserved: semver-compare.test.cjs (14) and bug-10 (4) pass against the generated output; runtime consumer changeset/cli.cjs unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): make build-at-publish robust across all CI paths (codex review) Adversarial review found the pilot's generated artifact would be missing on clean CI checkouts. `pretest`/`pretest:coverage` only fire for `npm test`, but CI runs `test:unit`/`test:integration`/`test:install` and `node run-tests.cjs` directly — none of which built the artifact, so any suite requiring semver-compare.cjs would hit module-not-found on a clean checkout, and install-smoke's `npm pack` could ship without it. - Add a `prepare` script (`npm run build:lib`). `npm ci` runs it automatically, so every CI test job and install-smoke's pack emit the artifact before use. This is the idiomatic npm mechanism for compiled-output-not-in-git and fixes both the test and pack paths in one place. - Add `src/` + `tsconfig.build.json` to ci-test-scope and the install-smoke / mutation path filters, so a source-only edit to a migrated module still triggers its tests and mutation coverage (prevents silent CI skips). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): map src/*.cts to built artifact in mutation changed-files detection Follow-up to the codex re-review. The prior commit added src/**/*.cts to the mutation workflow's path trigger but left its "compute changed core lib files" step diffing only get-shit-done/bin/lib/**/*.cjs — which are now gitignored and never appear in a diff. A source-only edit would trigger the workflow then early-exit ("no core lib files changed"), silently skipping mutation testing. Map each changed src/*.cts to its built get-shit-done/bin/lib/*.cjs path (the on-disk artifact Stryker mutates after prepare/build:lib), merge with the hand-written .cjs diff, and apply the test/excluded-module filters once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): use 'src/' pathspec in mutation diff (git glob doesn't match top-level) Codex review caught that `git diff -- 'src/**/*.cts'` returns empty for a top-level file like src/semver-compare.cts — git's default pathspec glob does not match `**` across zero directories (verified on git 2.50.1). The prior commit's src-detection therefore never fired, so source-only changes still skipped mutation. Switch to the dir-scoped pathspec 'src/' + a `.cts` grep (robust for flat and nested layouts), and broaden the workflow path trigger to 'src/**' to match install-smoke. Verified end-to-end: a change to src/semver-compare.cts now resolves to get-shit-done/bin/lib/semver-compare.cjs in the --mutate list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#537): add changeset fragment for build-at-publish pilot Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#537): replace prepare with prepack + build-if-missing; defer mutation wiring CI surfaced three real issues the local run and codex review missed: 1. lockfile-sync failed on every platform. Root cause: `npm ci --dry-run` (the repo's lockfile health check) RUNS the `prepare` script, but in dry-run the devDependencies aren't installed, so `tsc` is not found (exit 127) and the check reports a misleading "out of sync". `prepare` is the wrong hook for a build needing a devDep. Replace it with `prepack` (runs only on pack/publish, when node_modules exists) for the tarball path, and build the artifact inside scripts/run-tests.cjs (build-if-missing) for the test path — the universal chokepoint every CI test invocation funnels through, including the direct `node run-tests.cjs --files-from` step that bypasses npm lifecycle hooks. The guard is a no-op once built, so the run-tests harness test is unaffected. 2. The Stryker mutation gate ran only 1 test against semver-compare (~0% score, 71/71 mutants surviving) — a Stryker test-selection problem orthogonal to the build migration, and raising the score needs property tests (ADR-456). Revert the mutation.yml src wiring; mutation coverage for src-authored modules is a separate follow-up tracked in #537. (The deletion of the gitignored top-level .cjs does not match the workflow's `bin/lib/**/*.cjs` git pathspec, so the gate skips cleanly.) Verified: clean-room `npm ci --dry-run` exits 0; deleting the artifact then running a suite rebuilds it; run-tests harness 22/22 green; `npm pack` includes the built artifact via prepack. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
173 lines
9.2 KiB
Markdown
173 lines
9.2 KiB
Markdown
# ADR 457: Generation model for `bin/lib/*.cjs` type safety [Accepted]
|
||
|
||
- **Status:** Accepted
|
||
- **Date:** 2026-05-28 (rewritten and accepted 2026-05-31 after correcting fabricated context)
|
||
|
||
> **Accepted direction:** build-at-publish (model 2 below). Implementation is
|
||
> tracked in a separate migration issue and proceeds module-by-module. The sole
|
||
> prerequisite — the ESLint harness (ADR 452) — is already **accepted/merged**.
|
||
>
|
||
> **Provenance note.** An earlier draft of this ADR (and issue #457) was authored
|
||
> by an agent and asserted a codebase state that did not exist — "~13 files
|
||
> generated from `.ts` via `tsc`", `get-shit-done/src/` / `sdk/src/` source trees,
|
||
> and a `tests/cjs-ts-parity.test.cjs`. None of those existed. This rewrite
|
||
> grounds the decision in verified ground truth. Do not restore the earlier
|
||
> "natural completion of the 13 generated files" framing; it was fiction.
|
||
|
||
## Context
|
||
|
||
### What actually exists today (verified 2026-05-31)
|
||
|
||
- `get-shit-done/bin/lib/` holds **84** `.cjs` files. **Exactly one** carries a
|
||
`// @generated` header: `package-identity.cjs`.
|
||
- That one generated file is **not** `tsc` output. It is produced by
|
||
`scripts/generate-package-identity.cjs` — a plain Node script that reads
|
||
`package.json` and **bakes literal coordinate values** into a CJS module.
|
||
- There is **no** `get-shit-done/src/` or `sdk/src/` TypeScript tree. There is
|
||
**no** TS→CJS transpilation pipeline. There is **no**
|
||
`tests/cjs-ts-parity.test.cjs`. The only parity test is
|
||
`tests/issue-498-package-identity.test.cjs`, scoped to the one baked file: it
|
||
regenerates from `package.json` and asserts the committed output is not stale.
|
||
- `tsconfig.lint.json` exists with `allowJs` + `checkJs`, but it is **not**
|
||
wired into `eslint.config.mjs`. The `.cjs` config block (`eslint.config.mjs`
|
||
lines 61–92) sets only `sourceType`/`globals` — **no `parser`, no
|
||
`parserOptions.project`, no `projectService`, and no `@typescript-eslint`
|
||
rule** is enabled. A comment on line 60 nonetheless *claims* "Type-aware via
|
||
parserOptions.project=tsconfig.lint.json" — so the file is linted **without**
|
||
type information while the config advertises the opposite.
|
||
- `eslint.config.mjs` lists 12 files in `GENERATED_CJS_IGNORES` and treats them
|
||
as generated (never linted) — but those 12 are all **hand-written** (verified:
|
||
none carry an `@generated` header). This is a latent inconsistency: the lint
|
||
config already pretends a generation pipeline exists for them.
|
||
|
||
So the real situation is: **83 hand-written `.cjs`, 1 value-baked `.cjs`, and a
|
||
lint config that already advertises — in a comment and in a 12-file ignore list
|
||
— a type-aware generation pipeline that was never built.**
|
||
|
||
### Two different things are both called "generation"
|
||
|
||
This distinction is the crux of the decision, and the earlier draft erased it:
|
||
|
||
- **Value baking (exists, forced).** `package-identity.cjs` must be generated
|
||
because the *installed* tree ships a synthetic `{"type":"commonjs"}`
|
||
`package.json` with no `.name`, so a runtime `require('package.json').name`
|
||
is `undefined` (bug #378). The values literally cannot be read at runtime;
|
||
baking them at build time is the only option. **Deletion test:** remove the
|
||
generator and the complexity reappears across every consumer. It is a deep
|
||
seam and earns its keep.
|
||
|
||
- **Transpilation (proposed, optional).** Authoring `bin/lib` logic as TS and
|
||
emitting `.cjs` via `tsc`. **Deletion test:** remove it and *nothing*
|
||
reappears — a hand-written `.cjs` and a `tsc`-emitted `.cjs` are behaviorally
|
||
identical at runtime. The seam buys **no runtime leverage**. Its entire value
|
||
is **author-time and CI type checking**.
|
||
|
||
`package-identity` is therefore **not** precedent for the proposed transpilation
|
||
work. They are different techniques with different forcing functions.
|
||
|
||
### The problem actually worth solving
|
||
|
||
Type safety on the hand-written runtime surface is **second-class**: type errors
|
||
surface (if at all) as lint findings via the un-wired `tsconfig.lint.json`, not
|
||
as compile errors. Any contributor — human or agent — adding a `bin/lib` file
|
||
must decide hand-write vs generate, with no enforced answer. That inconsistency
|
||
is real and grows.
|
||
|
||
## The decision this ADR must make
|
||
|
||
Type-checking TS sources is the goal. The load-bearing question the earlier
|
||
draft skipped is: **do we check the generated `.cjs` into git, or treat it as a
|
||
build artifact?** Three models:
|
||
|
||
1. **Check in both `.ts` source and `.cjs` output.** Creates a permanent
|
||
"two copies must match" invariant, requiring parity tests, dual commits, and
|
||
a pre-commit/CI drift gate. This is the model the earlier draft assumed —
|
||
inherited from value-baking, where checking in output is *forced*. For
|
||
transpilation, nothing forces it, so this imports maximum friction for no
|
||
runtime gain.
|
||
|
||
2. **Build at publish (recommended).** `bin/lib/*.cjs` becomes a gitignored
|
||
build artifact emitted from a TS `src/` tree by `tsc`; npm publishes the
|
||
built output. **Feasible today:** `package.json` already ships `get-shit-done`
|
||
and `scripts` via its `files` array, and already runs a pre-publish build
|
||
step (`"prepublishOnly": "npm run build:hooks"`) — the `.cjs` emit hooks into
|
||
the same step, and `npm pack` includes on-disk artifacts regardless of
|
||
`.gitignore`. No drift invariant, no parity test, no dual commits — the
|
||
"Negative" consequences below mostly evaporate. Cost: contributors run a
|
||
build to exercise local changes, and CI must build before test.
|
||
|
||
3. **Build at install.** Rejected: fragile across Node versions and platforms
|
||
(CONTEXT.md notes Windows / Node 24 hazards) and slows every install.
|
||
|
||
## Decision [Accepted]
|
||
|
||
1. **Pursue type safety via a TS `src/` tree compiled with `tsc`**, adopting
|
||
**model 2 (build at publish)**: TS source is canonical, `.cjs` is a
|
||
gitignored artifact. This dissolves the drift-policing machinery rather than
|
||
building it.
|
||
2. **Keep value baking separate.** `package-identity.cjs` stays a checked-in
|
||
baked artifact under its existing generator and parity test; the install-tree
|
||
#378 constraint is unaffected by this ADR.
|
||
3. **Migrate incrementally, lowest-coupling module first**, behind one pilot PR
|
||
that stands up the `src/` tree + build wiring for a single module before any
|
||
bulk move.
|
||
4. **Reconcile the lint config to reality first.** The 12-entry
|
||
`GENERATED_CJS_IGNORES` list currently lies about hand-written files; it must
|
||
be corrected (those files linted as hand-written) before, not after, a
|
||
pipeline exists — otherwise the inconsistency masks the migration's progress.
|
||
5. **Wire type-aware linting** to the real `tsconfig.json` as modules become TS;
|
||
retire `tsconfig.lint.json` only when the last hand-written `.cjs` is gone.
|
||
|
||
## Consequences
|
||
|
||
### Positive
|
||
- Type-aware `typescript-eslint` rules apply to migrated runtime code.
|
||
- No checked-in generated `.cjs`, so **no** drift invariant and **no** parity
|
||
test to maintain for the transpiled surface (contrast: the rejected model 1).
|
||
- One enforced answer to "hand-write or generate?" for new `bin/lib` code.
|
||
|
||
### Negative
|
||
- A build step now sits between editing `src/*.ts` and running `bin/lib/*.cjs`.
|
||
Local dev and CI must build before exercising runtime behavior.
|
||
- Migration touches ~83 files; each may surface latent type errors to fix.
|
||
- Tooling that today reads `bin/lib/*.cjs` from a checkout (not an install) must
|
||
build first or read from `src/`.
|
||
|
||
### For testing
|
||
- Tests importing `bin/lib/*.cjs` keep working **only if** the build has run;
|
||
the test command must depend on the build. This is the main behavioral change
|
||
versus today, where the `.cjs` is always present in the tree.
|
||
|
||
## Rejected Alternatives
|
||
|
||
- **(a) Keep the split indefinitely** — leaves the type-aware gap and the
|
||
un-wired `tsconfig.lint.json` permanently. Rejected as a final state.
|
||
- **(b) Check in `.ts` + generated `.cjs` (model 1)** — imports a drift
|
||
invariant, parity tests, and dual commits for zero runtime benefit. Rejected
|
||
in favor of build-at-publish.
|
||
- **(c) Full ESM rewrite** — breaks `require()` consumers (no `"type":"module"`
|
||
today); semver-major. Out of scope.
|
||
- **(d) Wire `tsconfig.lint.json` into ESLint and stop there** — keeps the
|
||
stopgap permanent and never delivers compile-level (vs lint-level) type
|
||
errors. Rejected as the final state, but acceptable as an interim while the
|
||
pilot proves out.
|
||
|
||
## Open questions
|
||
|
||
- Does any consumer rely on `bin/lib/*.cjs` being present in a raw (un-built)
|
||
checkout? If so, build-at-publish needs a `prepare`-script bridge.
|
||
- `tsc` CJS interop details (`esModuleInterop`, `__importDefault` shims) for the
|
||
modules that re-`require` each other.
|
||
- Whether the pilot should be a leaf utility or one of the 12 mislabeled
|
||
`GENERATED_CJS_IGNORES` files (which already advertise themselves as generated).
|
||
|
||
## References
|
||
|
||
- ESLint harness prerequisite (**accepted**): `452-eslint-lint-harness.md`
|
||
- Test-rigor policies (**accepted**): `456-test-rigor-architecture.md`
|
||
- Single-runtime collapse (**accepted**): `0174-retire-gsd-sdk-package-boundary.md`
|
||
- Superseded shared-module seam: `3524-cjs-sdk-hard-seam.md`
|
||
- Tracking issue: [#457](https://github.com/open-gsd/gsd-core/issues/457)
|
||
- Value-baking precedent (distinct technique): `scripts/generate-package-identity.cjs`,
|
||
`tests/issue-498-package-identity.test.cjs`
|