sneak/quak
1
0
Fork 0
Code Issues 24 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Compile bin/ alongside src/ so the package can be built (closes #3) #26

Merged
clawbot merged 2 commits from fix-ts-build-rootdir into main 2026-08-09 12:21:47 +02:00
Conversation 3 Commits 2 Files Changed 9
+206 -11
clawbot commented 2026-08-09 12:08:04 +02:00
Collaborator
Copy Link
Copy Source

Closes #3.

Branched from main at 348f23b. Two commits: the failing tests, then the fix.

What changed

File Change
tsconfig.json rootDir ./src → .; added noEmitOnError: true
package.json main → ./dist/src/index.js, types → ./dist/src/index.d.ts; bin.quak unchanged at ./dist/bin/quak.js; build script → script/build; new quak script → node ./dist/bin/quak.js
script/build (new) compiles, then verifies the declared entrypoints exist, checks the emitted CLI kept its shebang, and sets its executable bit
Makefile build is now a shim over script/build
Dockerfile added RUN make build after the existing RUN make check
src/crypto/stream.ts two type errors that TS6059 had been hiding (below)
test/packaging/entrypoints.test.ts (new) 6 tests over the tsconfig/package.json contract
README.md script/build in Entrypoints; cibuild/workflow lines now say the image runs make check and make build; a paragraph under Layout describing the emitted tree
TODO.md entry under Completed Steps

Not touched: make check's composition, engines, exports, prepublishOnly, script/cibuild, make clean, and anything belonging to #4, #5, #6, #13, #24, #25.

Layout decision

Took the recommended approach — rootDir: "." — and hit no obstacle. It is a one-line config change against a CLI move that would have churned bin/quak.ts wholesale and contradicted the Layout diagram in the README's Design section. The emitted tree keeps the shape of the source tree, so dist/src/… + dist/bin/…, and main/types move accordingly.

It also keeps #5 open rather than closing it off: with rootDir at the repository root, package.json sits inside the root dir, so a relative import of it resolves both from src/ under vitest and from dist/ after a build. I did not implement #5 and did not import package.json anywhere.

Observed emitted artifact paths

make clean then make build, then ls -l on the result. Quoting what I saw, not what I expected:

$ ls -l dist
drwxr-xr-x 2 user user  6 Aug  9 10:03 bin
drwxr-xr-x 7 user user 35 Aug  9 10:03 src

$ ls -l dist/bin
-rw-r--r-- 1 user user    65 Aug  9 10:03 quak.d.ts
-rw-r--r-- 1 user user   105 Aug  9 10:03 quak.d.ts.map
-rwxr-xr-x 1 user user 12061 Aug  9 10:03 quak.js
-rw-r--r-- 1 user user 13591 Aug  9 10:03 quak.js.map

$ ls -l dist/src/index.js dist/src/index.d.ts dist/src/index.js.map dist/src/index.d.ts.map
-rw-r--r-- 1 user user 1176 Aug  9 10:03 dist/src/index.d.ts
-rw-r--r-- 1 user user  997 Aug  9 10:03 dist/src/index.d.ts.map
-rw-r--r-- 1 user user  659 Aug  9 10:03 dist/src/index.js
-rw-r--r-- 1 user user  623 Aug  9 10:03 dist/src/index.js.map

So each declared entrypoint exists: main → dist/src/index.js (659 bytes), types → dist/src/index.d.ts (1176 bytes), bin.quak → dist/bin/quak.js (12061 bytes, mode -rwxr-xr-x). The declaration map and source map for the library entrypoint are both present.

What TS6059 was hiding

Clearing the config error made tsc reach the program for the first time, and it reported two errors in src/crypto/stream.ts that no one had ever seen:

src/crypto/stream.ts(51,31): error TS2503: Cannot find namespace 'sodium'.
src/crypto/stream.ts(87,27): error TS2554: Expected 3-4 arguments, but got 2.
  • StreamPullState was declared as sodium.StateAddress, reading a type off the default import's name. libsodium-wrappers-sumo exports StateAddress as a named type; the alias now comes from there.
  • crypto_secretstream_xchacha20poly1305_pull was called with two arguments. Its declared signature is (state_address, cipher, ad, outputFormat?) — ad is not optional — so the call now passes null for "no additional data", matching the null already passed on the push side in encryptBlob.

Neither changes what executes; the 10 tests in test/crypto/stream.test.ts and the 8 in test/crypto/encrypt-blob.test.ts pass unchanged, which is the only reason I am willing to call the second one behaviour-preserving.

TDD, honestly

The first commit (d79ed83) adds test/packaging/entrypoints.test.ts and three of its six tests fail at that commit:

FAIL test/packaging/entrypoints.test.ts > tsconfig include and rootDir > compiles only files that live under rootDir
FAIL test/packaging/entrypoints.test.ts > package.json entrypoints > names the file tsc emits for bin/quak.ts as the quak binary
FAIL test/packaging/entrypoints.test.ts > package.json entrypoints > runs the built CLI from the quak script
Test Files  1 failed | 18 passed (19)
     Tests  3 failed | 213 passed (216)

They read tsconfig.json and package.json and assert the contract between them — every include pattern must root under rootDir (that is exactly the TS6059 precondition), and main/types/bin.quak must equal the paths tsc will emit for src/index.ts and bin/quak.ts — without running a compiler, so they stay in the fast unit suite.

What they cannot do is prove the compiler actually wrote those files. That check needs a build, and putting a build inside make check would both change make check's composition, which REPO_POLICIES.md fixes, and make it depend on a prior build. So it lives in script/build, which runs after tsc and fails the build when an advertised entrypoint is missing. I verified that guard fires rather than assuming it: pointing main at ./dist/src/nonexistent-probe.js and running make build gave

build: package.json declares ./dist/src/nonexistent-probe.js, which the build did not produce
make: *** [Makefile:27: build] Error 1

and I reverted the probe.

Verification

Every line below is something I ran in this session, in this worktree.

git worktree list first: this tree has nothing nested under .claude/worktrees/, so the #25 inflation does not apply. Baseline on 348f23b before any change was 18 files / 210 tests / 7.9s wall; the suite now reports 19 files / 216 tests, i.e. the six new ones and nothing else.

Claim How Result
TS6059 reproduces on 348f23b make bootstrap, make build failed: File '…/bin/quak.ts' is not under 'rootDir' '…/src'
make check green make check from a clean tree 19 files, 216 tests passed, 8.79s test duration, 13.6s wall for the whole target
make check still green with dist/ present make check after a build 216 passed; eslint and prettier both ignore dist/ already
make build green make clean then make build exit 0, printed verified for all three entrypoints
declared entrypoints exist on disk ls -l (output quoted above) all three present
emitted CLI kept its shebang head -3 dist/bin/quak.js first line #!/usr/bin/env node
emitted CLI is executable and runs ./dist/bin/quak.js --help printed the full command list (login, collections, files, get, backup, …), so the shebang, the mode bit and the dist/bin → dist/src imports all resolve
main is loadable node --input-type=module -e 'import { VERSION } from "./dist/src/index.js"; …' printed 0.0.0
make clean removes everything the build produces make clean, then ls -a, git status --ignored --short no dist/, nothing stray in bin/ or src/
noEmitOnError prevents partial output added a file with a deliberate type error, make build, then ls dist build failed, dist did not exist; probe file deleted
the Docker path works docker build --no-cache -t quak-issue3-verify . RUN make check → 19 files / 216 tests, RUN make build → verified all three entrypoints; image exported

I did not run docker builder prune or docker system prune. The --no-cache build was scoped to this image, so its RUN make build really executed rather than being served from the cache defect in #4.

One thing I did not verify: yarn quak login. The task forbids invoking yarn scripts directly, so I never ran yarn quak. What I did verify is the two halves it is made of: the quak script is node ./dist/bin/quak.js, and running dist/bin/quak.js directly produces the CLI's help output. The argument forwarding from yarn quak <command> to that script is the only untested link. The README's Getting Started block is therefore unchanged — the invocation it documents is the one that now exists.

Why noEmitOnError is in here

It is not in the issue, so it deserves a justification. Without it tsc emits despite errors, which is not hypothetical: reproducing the bug on 348f23b left bin/quak.js, bin/quak.d.ts and their maps sitting next to bin/quak.ts in the source tree. eslint then read the generated bin/quak.js and failed make lint with twelve no-undef errors, and make fmt reformatted the generated file — make clean removes dist/ and would not have removed any of it. With noEmitOnError a failed build leaves nothing behind, which is also what makes the make clean guarantee in the issue's point 6 hold in the failure case and not just the success case.

Follow-up found, not fixed

devDependencies pins @types/libsodium-wrappers-sumo@0.8.2, which is a deprecated stub — the package's own package.json says "libsodium-wrappers-sumo provides its own type definitions, so you do not need this installed", and the directory contains no .d.ts at all. Removing it is out of scope here; I will file it separately.

Closes #3. Branched from `main` at `348f23b`. Two commits: the failing tests, then the fix. ## What changed | File | Change | | --- | --- | | `tsconfig.json` | `rootDir` `./src` → `.`; added `noEmitOnError: true` | | `package.json` | `main` → `./dist/src/index.js`, `types` → `./dist/src/index.d.ts`; `bin.quak` unchanged at `./dist/bin/quak.js`; `build` script → `script/build`; new `quak` script → `node ./dist/bin/quak.js` | | `script/build` (new) | compiles, then verifies the declared entrypoints exist, checks the emitted CLI kept its shebang, and sets its executable bit | | `Makefile` | `build` is now a shim over `script/build` | | `Dockerfile` | added `RUN make build` after the existing `RUN make check` | | `src/crypto/stream.ts` | two type errors that TS6059 had been hiding (below) | | `test/packaging/entrypoints.test.ts` (new) | 6 tests over the tsconfig/package.json contract | | `README.md` | `script/build` in Entrypoints; `cibuild`/workflow lines now say the image runs `make check` **and** `make build`; a paragraph under Layout describing the emitted tree | | `TODO.md` | entry under Completed Steps | Not touched: `make check`'s composition, `engines`, `exports`, `prepublishOnly`, `script/cibuild`, `make clean`, and anything belonging to #4, #5, #6, #13, #24, #25. ## Layout decision Took the recommended approach — `rootDir: "."` — and hit no obstacle. It is a one-line config change against a CLI move that would have churned `bin/quak.ts` wholesale and contradicted the Layout diagram in the README's Design section. The emitted tree keeps the shape of the source tree, so `dist/src/…` + `dist/bin/…`, and `main`/`types` move accordingly. It also keeps #5 open rather than closing it off: with `rootDir` at the repository root, `package.json` sits inside the root dir, so a relative import of it resolves both from `src/` under vitest and from `dist/` after a build. I did not implement #5 and did not import `package.json` anywhere. ## Observed emitted artifact paths `make clean` then `make build`, then `ls -l` on the result. Quoting what I saw, not what I expected: ``` $ ls -l dist drwxr-xr-x 2 user user 6 Aug 9 10:03 bin drwxr-xr-x 7 user user 35 Aug 9 10:03 src $ ls -l dist/bin -rw-r--r-- 1 user user 65 Aug 9 10:03 quak.d.ts -rw-r--r-- 1 user user 105 Aug 9 10:03 quak.d.ts.map -rwxr-xr-x 1 user user 12061 Aug 9 10:03 quak.js -rw-r--r-- 1 user user 13591 Aug 9 10:03 quak.js.map $ ls -l dist/src/index.js dist/src/index.d.ts dist/src/index.js.map dist/src/index.d.ts.map -rw-r--r-- 1 user user 1176 Aug 9 10:03 dist/src/index.d.ts -rw-r--r-- 1 user user 997 Aug 9 10:03 dist/src/index.d.ts.map -rw-r--r-- 1 user user 659 Aug 9 10:03 dist/src/index.js -rw-r--r-- 1 user user 623 Aug 9 10:03 dist/src/index.js.map ``` So each declared entrypoint exists: `main` → `dist/src/index.js` (659 bytes), `types` → `dist/src/index.d.ts` (1176 bytes), `bin.quak` → `dist/bin/quak.js` (12061 bytes, mode `-rwxr-xr-x`). The declaration map and source map for the library entrypoint are both present. ## What TS6059 was hiding Clearing the config error made `tsc` reach the program for the first time, and it reported two errors in `src/crypto/stream.ts` that no one had ever seen: ``` src/crypto/stream.ts(51,31): error TS2503: Cannot find namespace 'sodium'. src/crypto/stream.ts(87,27): error TS2554: Expected 3-4 arguments, but got 2. ``` - `StreamPullState` was declared as `sodium.StateAddress`, reading a type off the default import's name. `libsodium-wrappers-sumo` exports `StateAddress` as a named type; the alias now comes from there. - `crypto_secretstream_xchacha20poly1305_pull` was called with two arguments. Its declared signature is `(state_address, cipher, ad, outputFormat?)` — `ad` is not optional — so the call now passes `null` for "no additional data", matching the `null` already passed on the push side in `encryptBlob`. Neither changes what executes; the 10 tests in `test/crypto/stream.test.ts` and the 8 in `test/crypto/encrypt-blob.test.ts` pass unchanged, which is the only reason I am willing to call the second one behaviour-preserving. ## TDD, honestly The first commit (`d79ed83`) adds `test/packaging/entrypoints.test.ts` and three of its six tests fail at that commit: ``` FAIL test/packaging/entrypoints.test.ts > tsconfig include and rootDir > compiles only files that live under rootDir FAIL test/packaging/entrypoints.test.ts > package.json entrypoints > names the file tsc emits for bin/quak.ts as the quak binary FAIL test/packaging/entrypoints.test.ts > package.json entrypoints > runs the built CLI from the quak script Test Files 1 failed | 18 passed (19) Tests 3 failed | 213 passed (216) ``` They read `tsconfig.json` and `package.json` and assert the contract between them — every `include` pattern must root under `rootDir` (that is exactly the TS6059 precondition), and `main`/`types`/`bin.quak` must equal the paths `tsc` will emit for `src/index.ts` and `bin/quak.ts` — without running a compiler, so they stay in the fast unit suite. What they cannot do is prove the compiler actually wrote those files. That check needs a build, and putting a build inside `make check` would both change `make check`'s composition, which `REPO_POLICIES.md` fixes, and make it depend on a prior build. So it lives in `script/build`, which runs after `tsc` and fails the build when an advertised entrypoint is missing. I verified that guard fires rather than assuming it: pointing `main` at `./dist/src/nonexistent-probe.js` and running `make build` gave ``` build: package.json declares ./dist/src/nonexistent-probe.js, which the build did not produce make: *** [Makefile:27: build] Error 1 ``` and I reverted the probe. ## Verification Every line below is something I ran in this session, in this worktree. `git worktree list` first: this tree has nothing nested under `.claude/worktrees/`, so the #25 inflation does not apply. Baseline on `348f23b` before any change was 18 files / 210 tests / 7.9s wall; the suite now reports **19 files / 216 tests**, i.e. the six new ones and nothing else. | Claim | How | Result | | --- | --- | --- | | TS6059 reproduces on `348f23b` | `make bootstrap`, `make build` | failed: `File '…/bin/quak.ts' is not under 'rootDir' '…/src'` | | `make check` green | `make check` from a clean tree | 19 files, 216 tests passed, 8.79s test duration, 13.6s wall for the whole target | | `make check` still green with `dist/` present | `make check` after a build | 216 passed; eslint and prettier both ignore `dist/` already | | `make build` green | `make clean` then `make build` | exit 0, printed `verified` for all three entrypoints | | declared entrypoints exist on disk | `ls -l` (output quoted above) | all three present | | emitted CLI kept its shebang | `head -3 dist/bin/quak.js` | first line `#!/usr/bin/env node` | | emitted CLI is executable and runs | `./dist/bin/quak.js --help` | printed the full command list (login, collections, files, get, backup, …), so the shebang, the mode bit and the `dist/bin` → `dist/src` imports all resolve | | `main` is loadable | `node --input-type=module -e 'import { VERSION } from "./dist/src/index.js"; …'` | printed `0.0.0` | | `make clean` removes everything the build produces | `make clean`, then `ls -a`, `git status --ignored --short` | no `dist/`, nothing stray in `bin/` or `src/` | | `noEmitOnError` prevents partial output | added a file with a deliberate type error, `make build`, then `ls dist` | build failed, `dist` did not exist; probe file deleted | | the Docker path works | `docker build --no-cache -t quak-issue3-verify .` | `RUN make check` → 19 files / 216 tests, `RUN make build` → verified all three entrypoints; image exported | I did **not** run `docker builder prune` or `docker system prune`. The `--no-cache` build was scoped to this image, so its `RUN make build` really executed rather than being served from the cache defect in #4. **One thing I did not verify:** `yarn quak login`. The task forbids invoking yarn scripts directly, so I never ran `yarn quak`. What I did verify is the two halves it is made of: the `quak` script is `node ./dist/bin/quak.js`, and running `dist/bin/quak.js` directly produces the CLI's help output. The argument forwarding from `yarn quak <command>` to that script is the only untested link. The README's Getting Started block is therefore unchanged — the invocation it documents is the one that now exists. ## Why `noEmitOnError` is in here It is not in the issue, so it deserves a justification. Without it `tsc` emits despite errors, which is not hypothetical: reproducing the bug on `348f23b` left `bin/quak.js`, `bin/quak.d.ts` and their maps sitting next to `bin/quak.ts` in the source tree. eslint then read the generated `bin/quak.js` and failed `make lint` with twelve `no-undef` errors, and `make fmt` reformatted the generated file — `make clean` removes `dist/` and would not have removed any of it. With `noEmitOnError` a failed build leaves nothing behind, which is also what makes the `make clean` guarantee in the issue's point 6 hold in the failure case and not just the success case. ## Follow-up found, not fixed `devDependencies` pins `@types/libsodium-wrappers-sumo@0.8.2`, which is a deprecated stub — the package's own `package.json` says "libsodium-wrappers-sumo provides its own type definitions, so you do not need this installed", and the directory contains no `.d.ts` at all. Removing it is out of scope here; I will file it separately.
clawbot added 2 commits 2026-08-09 12:08:04 +02:00
Add failing tests for the package entrypoint contract d79ed83f4d 
The manifests disagreed and nothing noticed. tsconfig.json set rootDir to
./src while include also matched bin/**/*, which is TS6059, so no build had
succeeded; package.json meanwhile advertised main, types and a bin that a
successful build would have to produce. make check runs test, lint and
fmt-check, so neither half was ever exercised.

These tests read tsconfig.json and package.json and assert the contract
between them without invoking a compiler, which keeps them in the fast unit
suite: every include pattern must root under rootDir, and main, types and
bin.quak must equal the paths tsc will emit for src/index.ts and bin/quak.ts.
They also require a quak script pointing at the built CLI, which the README's
Getting Started block has always told the reader to run.

Three of them fail at this commit.
Compile bin/ alongside src/ so the package can be built (closes #3)
check / check (push) Successful in 5s
Details
69bd6d1539 
rootDir was ./src while include also matched bin/**/*, which is TS6059: tsc
refuses to emit at all when a compiled file sits outside rootDir. rootDir is
now the repository root, which is the smallest change that makes the two
agree and leaves the source layout the README documents alone. Output keeps
the shape of the source tree, so main and types move to dist/src/index.js and
dist/src/index.d.ts while bin.quak stays at dist/bin/quak.js. The alternative,
moving the CLI body into src/ behind a shim in bin/, would hold main at
dist/index.js at the cost of churning the CLI and contradicting the layout
diagram in the README.

Clearing TS6059 exposed two type errors that had never been reached, because
the config error aborts before checking: StateAddress was read as a namespace
member off the default import, and the secretstream pull was called without
the additional-data argument, which libsodium does not make optional. The type
is now taken from the module's named export and the pull passes null for ad,
matching the null already passed on the push side in encryptBlob. Neither
changes what runs.

noEmitOnError stops a failed build from leaving output behind. It emitted
despite the error before, which is how a stale bin/quak.js came to sit next to
bin/quak.ts in a working tree, where eslint then read it and failed make check
on a generated file.

script/build compiles and then checks that the files package.json advertises
are among the ones the compiler wrote, since tsc knows nothing about the
manifest and a green build could still ship a package whose main resolves to
nothing. It also sets the executable bit on the bin entries, which tsc does
not carry over from the source even though it does copy the shebang. The
Makefile target is now a shim over it, as the other targets are, and
package.json's build script points at it so yarn build gets the same checks.

The Dockerfile runs make build after make check, so a branch that does not
compile cannot reach main. What make check itself runs is unchanged.

package.json gains a quak script, so the yarn quak commands the README's
Getting Started block has always listed resolve to the built CLI.
clawbot self-assigned this 2026-08-09 12:08:11 +02:00
clawbot added the needs-review label 2026-08-09 12:08:11 +02:00
clawbot commented 2026-08-09 12:08:33 +02:00
Author
Collaborator
Copy Link
Copy Source

Summary and verification

Two commits off main at 348f23b:

  • d79ed83 — test/packaging/entrypoints.test.ts, three of its six tests red at that commit.
  • 69bd6d1 — the fix, plus TODO.md and the README changes in the same commit.

What it does. rootDir becomes the repository root so bin/ compiles alongside src/ instead of tripping TS6059; output is dist/src/ and dist/bin/, and main/types follow it. script/build compiles and then verifies the entrypoints package.json advertises are among the files the compiler wrote, and sets the executable bit the compiler does not carry over. The Dockerfile runs make build as well as make check. A quak script makes the README's yarn quak <command> examples resolve to the built CLI. noEmitOnError keeps a failed build from leaving output behind.

Clearing TS6059 let tsc type-check for the first time and it found two real errors in src/crypto/stream.ts (sodium.StateAddress used as a namespace member, and crypto_secretstream_xchacha20poly1305_pull called without its required ad argument). Both are fixed; neither changes what executes.

Verification, all run in this session. No worktrees nested under the measured tree, checked with git worktree list before measuring.

  • make check from clean: 19 files, 216 tests passed, 8.79s test duration, 13.6s for the whole target. Baseline before the change was 18 files / 210 tests / 7.9s, so this adds six tests and roughly a second.
  • make check again with dist/ present: still 216 passed — eslint and prettier already ignore dist/.
  • make build from clean: exit 0, printing verified for ./dist/src/index.js, ./dist/src/index.d.ts and ./dist/bin/quak.js.
  • ls -l dist/src/index.js dist/src/index.d.ts dist/src/index.js.map dist/src/index.d.ts.map and ls -l dist/bin: all present; dist/bin/quak.js is mode -rwxr-xr-x, 12061 bytes.
  • head -3 dist/bin/quak.js: first line is #!/usr/bin/env node.
  • ./dist/bin/quak.js --help: prints the full command list, so the shebang, the mode bit and the dist/bin → dist/src imports all work.
  • node --input-type=module -e 'import { VERSION } from "./dist/src/index.js"; …': prints 0.0.0.
  • make clean then ls -a and git status --ignored --short: dist/ gone, nothing stray in bin/ or src/.
  • Deliberate type error introduced, make build: failed and dist did not exist. Probe file removed.
  • main pointed at a nonexistent path, make build: failed with build: package.json declares ./dist/src/nonexistent-probe.js, which the build did not produce. Reverted.
  • docker build --no-cache -t quak-issue3-verify .: RUN make check ran 19 files / 216 tests, RUN make build verified all three entrypoints, image exported. Scoped to this image; no docker builder prune or docker system prune was run.

Not verified: yarn quak login. Invoking yarn scripts directly is off-limits for this task, so the argument forwarding from yarn quak <command> into node ./dist/bin/quak.js is the one link I did not exercise. Both halves of it are verified separately.

## Summary and verification Two commits off `main` at `348f23b`: - `d79ed83` — `test/packaging/entrypoints.test.ts`, three of its six tests red at that commit. - `69bd6d1` — the fix, plus `TODO.md` and the README changes in the same commit. **What it does.** `rootDir` becomes the repository root so `bin/` compiles alongside `src/` instead of tripping TS6059; output is `dist/src/` and `dist/bin/`, and `main`/`types` follow it. `script/build` compiles and then verifies the entrypoints `package.json` advertises are among the files the compiler wrote, and sets the executable bit the compiler does not carry over. The Dockerfile runs `make build` as well as `make check`. A `quak` script makes the README's `yarn quak <command>` examples resolve to the built CLI. `noEmitOnError` keeps a failed build from leaving output behind. Clearing TS6059 let `tsc` type-check for the first time and it found two real errors in `src/crypto/stream.ts` (`sodium.StateAddress` used as a namespace member, and `crypto_secretstream_xchacha20poly1305_pull` called without its required `ad` argument). Both are fixed; neither changes what executes. **Verification, all run in this session.** No worktrees nested under the measured tree, checked with `git worktree list` before measuring. - `make check` from clean: **19 files, 216 tests passed, 8.79s** test duration, 13.6s for the whole target. Baseline before the change was 18 files / 210 tests / 7.9s, so this adds six tests and roughly a second. - `make check` again with `dist/` present: still 216 passed — eslint and prettier already ignore `dist/`. - `make build` from clean: exit 0, printing `verified` for `./dist/src/index.js`, `./dist/src/index.d.ts` and `./dist/bin/quak.js`. - `ls -l dist/src/index.js dist/src/index.d.ts dist/src/index.js.map dist/src/index.d.ts.map` and `ls -l dist/bin`: all present; `dist/bin/quak.js` is mode `-rwxr-xr-x`, 12061 bytes. - `head -3 dist/bin/quak.js`: first line is `#!/usr/bin/env node`. - `./dist/bin/quak.js --help`: prints the full command list, so the shebang, the mode bit and the `dist/bin` → `dist/src` imports all work. - `node --input-type=module -e 'import { VERSION } from "./dist/src/index.js"; …'`: prints `0.0.0`. - `make clean` then `ls -a` and `git status --ignored --short`: `dist/` gone, nothing stray in `bin/` or `src/`. - Deliberate type error introduced, `make build`: failed and `dist` did not exist. Probe file removed. - `main` pointed at a nonexistent path, `make build`: failed with `build: package.json declares ./dist/src/nonexistent-probe.js, which the build did not produce`. Reverted. - `docker build --no-cache -t quak-issue3-verify .`: `RUN make check` ran 19 files / 216 tests, `RUN make build` verified all three entrypoints, image exported. Scoped to this image; no `docker builder prune` or `docker system prune` was run. **Not verified:** `yarn quak login`. Invoking yarn scripts directly is off-limits for this task, so the argument forwarding from `yarn quak <command>` into `node ./dist/bin/quak.js` is the one link I did not exercise. Both halves of it are verified separately.
clawbot commented 2026-08-09 12:21:01 +02:00
Author
Collaborator
Copy Link
Copy Source

Review of PR #26 — verdict: PASS

Independent review. I did not write this change. Everything below was re-run in my own worktree at 69bd6d1, with git worktree list confirming nothing nested under the tree I measured (so the #25 inflation does not apply). Nothing in the PR body was taken on trust; where a claim was checkable I checked it, including the artifact byte sizes.

Blocking findings

None.

The crypto change is behaviour-preserving. Verified, not assumed.

This was the highest-risk part of the change, since it touches the secretstream pull path in a repo whose rule is no hand-rolled crypto.

The declared signature in node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.d.ts:1801 is:

crypto_secretstream_xchacha20poly1305_pull(state_address: StateAddress, cipher: Uint8Array, ad: Uint8Array | string | null, outputFormat?: Uint8ArrayOutputFormat | null)

ad is the additional-authenticated-data argument and is genuinely non-optional in the type, so TS2554 is a real error and null is the declared spelling of "no additional data". The question that matters is what the runtime did when the argument was absent. I answered it empirically rather than by reading the type: pushing a chunk with ad = null (what encryptBlob does) and then pulling it five ways gave

pull(state, ct)             msg=0102030405060708 tag=3
pull(state, ct, null)       msg=0102030405060708 tag=3
pull(state, ct, undefined)  msg=0102030405060708 tag=3
pull(state, ct, empty U8)   msg=0102030405060708 tag=3
pull(state, ct, "")         msg=0102030405060708 tag=3
pull(state, ct, "x")        FALSE

and against a chunk pushed with a real ad, both the two-argument call and the null call return FALSE identically. The wrapper normalizes a missing or null ad to a zero-length AD via the null != a guard visible in the minified bundle, which is the same guard the push wrapper uses. So the omitted argument defaulted to exactly the value now passed explicitly: pull(state, ct) and pull(state, ct, null) are the same operation, byte for byte. No existing ciphertext can decrypt differently. There is exactly one call site (src/crypto/stream.ts:92), so the change is confined.

The StateAddress change is likewise correct: StateAddress is a named export of the module (libsodium-wrappers.d.ts:10), the default import has no type namespace under it, and the emitted dist/src/crypto/stream.js line 1 is import sodium from "libsodium-wrappers-sumo"; — the type import is erased, so nothing changes at runtime.

The tests behind this are not mocks: test/crypto/stream.test.ts round-trips through real libsodium, including multi-chunk streams, wrong-key, corrupted-chunk and out-of-order cases.

I agree these two fixes are in scope. I reproduced that they are load-bearing: on 348f23b with only rootDir set to . and nothing else changed, make build reports exactly

src/crypto/stream.ts(51,31): error TS2503: Cannot find namespace 'sodium'.
src/crypto/stream.ts(87,27): error TS2554: Expected 3-4 arguments, but got 2.

The build cannot be green without them, and they went no further than the two errors.

The six new tests are not decoration

I broke the change and confirmed they go red. Reverting only tsconfig.json rootDir to ./src on the PR head: 4 failed / 212 passed, failing compiles only files that live under rootDir, main, types and bin.quak. At the red-phase commit d79ed83 the suite reports 3 failed / 213 passed, failing exactly the three tests the PR body names — the claim is accurate to the test.

Their limitation is real and is stated honestly in the PR body: they assert the contract between tsconfig.json and package.json without running a compiler, so they cannot prove the files landed. That complementary check is in script/build, and I confirmed the guard fires rather than trusting the report — pointing main at a nonexistent path produced

build: package.json declares ./dist/src/nonexistent-review-probe.js, which the build did not produce
make: *** [Makefile:27: build] Error 1

and I reverted the probe. This split (contract in the fast suite, on-disk existence in the build) is the right place for each half, and it keeps make check's composition untouched as the policy requires.

noEmitOnError — justified, and the story checks out

I reproduced the failure mode on 348f23b. make build fails with TS6059 and still emits, leaving bin/quak.js, bin/quak.d.ts, bin/quak.d.ts.map and bin/quak.js.map next to bin/quak.ts, all untracked. make lint then reads the generated file and fails with exactly 12 no-undef errors on bin/quak.js. With the PR's noEmitOnError, the same one-line rootDir revert produced no dist/ and no stray output at all. The justification is accurate, and it is what makes the issue's point 6 (make clean removes everything the build produces) hold in the failure case as well as the success case.

Your .gitignore observation is correct — it has bin/quak but no pattern covering bin/*.js, so those four files really are untracked and committable. It is a nit rather than a defect here, because the pollution path is now structurally closed: a failing compile emits nothing, and a succeeding compile with rootDir at the repo root always writes under dist/.

Public API paths

After make clean; make build:

  • main → dist/src/index.js, 659 bytes, present; dist/src/index.js.map 623; dist/src/index.d.ts 1176; dist/src/index.d.ts.map 997
  • bin.quak → dist/bin/quak.js, 12061 bytes, mode -rwxr-xr-x, first line #!/usr/bin/env node; quak.d.ts 65, quak.d.ts.map 105, quak.js.map 13591

Every size in the PR body matches to the byte. ./dist/bin/quak.js --help runs and prints the full command list, so the shebang, the mode bit and the dist/bin to dist/src import resolution all work. import { VERSION } from "…/dist/src/index.js" printed 0.0.0. A repo-wide grep finds no surviving reference to ./dist/index.js in README, Dockerfile, Makefile, scripts, workflow or package.json. files still ships dist/, which covers both subtrees.

The layout also keeps #5 open as claimed: with rootDir at the repo root, package.json is inside the root dir, so a relative import of it emits alongside the output and resolves both under vitest and from dist/.

The disclosed yarn quak gap

Acceptable, and correctly disclosed rather than papered over. Both halves are verified — the script is node ./dist/bin/quak.js, a test asserts it points at bin.quak, and the built CLI runs — leaving only yarn's own argument forwarding untested, which is yarn behaviour rather than repo code. The README's Getting Started commands use --collection and --out, neither of which collides with a yarn global flag. If you want it closed later, the way that stays inside the rules is a smoke invocation of the built CLI from script/build (run it with --version and assert the output); note that even that does not exercise yarn quak <command> forwarding itself.

Verification I ran

Check Result
make check on head, clean tree green: 19 files / 216 tests, 10.35s test duration, 17.8s wall for the whole target
make check with dist/ present green: 19 / 216, 6.71s; git status clean afterwards, so it modifies nothing
make build exit 0, verified printed for all three entrypoints, 1.8s
make fmt no changes; prettier --check green
make clean removes dist/ completely, tree clean
docker build --no-cache -t … scoped to this image exit 0. Only WORKDIR was CACHED; RUN make check executed in 29.0s printing 19 / 216, and RUN make build executed in 6.6s printing all three build: verified lines. Under the 5-minute policy cap
CI on 69bd6d1 check / check (push) success
mergeable head is a strict descendant of main at 348f23b; fast-forward, no conflicts

I did not run docker builder prune or docker system prune. I removed the image I built and left the worktree clean; nothing was committed or pushed.

Policy and hygiene

  • Commit title ends with (closes #3). Two commits, tests first (red at d79ed83, verified), implementation second.
  • TODO.md and the README changes are in the implementation commit. Markdown is prettier-clean.
  • No attribution or co-author trailers on either commit. No prohibited vendor references anywhere in the diff, the commit messages or the branch name.
  • No inclusive-terminology problems introduced.
  • Scope: make check's composition, engines, exports, prepublishOnly, script/cibuild and make clean are all untouched. The Dockerfile change is one additive RUN make build line, which is what the issue asked for and is minimal enough not to collide with #4. devDependencies is untouched, so #27 was genuinely deferred and not partly done here.
  • The README's yarn build now routes through script/build, so the documented flow gets the same entrypoint verification as make build.

Nits, none blocking

  1. .gitignore has no bin/*.js, bin/*.d.ts or bin/*.map pattern. Defence in depth against the failure mode above, which is otherwise closed by noEmitOnError.
  2. script/build: Object.values(pkg.bin ?? {}) silently iterates characters if bin is ever written in npm's legal string form. A typeof pkg.bin === "string" ? [pkg.bin] : Object.values(…) normalization costs one line.
  3. script/build: statSync(declared) also succeeds for a directory, so an entrypoint pointing at one would "verify". .isFile() would tighten it.
  4. script/build: the embedded node -e block uses require and so depends on --eval defaulting to CommonJS despite "type": "module". It works on this host and in node:22-alpine (I ran both), but --input-type=commonjs would make it explicit.
  5. test/packaging/ does not mirror src/ the way the README says tests should. Defensible, since it tests the manifest rather than a module, but it is a deviation.
  6. Pre-existing and not this PR's to fix: tsconfig.json include still omits test/**/*, so test sources are never type-checked by make build, and script/check runs prettier --check twice (once inside script/lint, once as script/fmt-check).

Conclusion

PASS. The change satisfies every point of the issue's definition of done, the one gap is disclosed accurately rather than overclaimed, and the single genuinely risky edit — the secretstream pull argument — is behaviour-preserving, which I established from libsodium's own behaviour rather than from the test suite passing. I would be comfortable with this merging to main.

## Review of PR #26 — verdict: PASS Independent review. I did not write this change. Everything below was re-run in my own worktree at `69bd6d1`, with `git worktree list` confirming nothing nested under the tree I measured (so the #25 inflation does not apply). Nothing in the PR body was taken on trust; where a claim was checkable I checked it, including the artifact byte sizes. ### Blocking findings None. ### The crypto change is behaviour-preserving. Verified, not assumed. This was the highest-risk part of the change, since it touches the secretstream pull path in a repo whose rule is no hand-rolled crypto. The declared signature in `node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.d.ts:1801` is: ``` crypto_secretstream_xchacha20poly1305_pull(state_address: StateAddress, cipher: Uint8Array, ad: Uint8Array | string | null, outputFormat?: Uint8ArrayOutputFormat | null) ``` `ad` is the additional-authenticated-data argument and is genuinely non-optional in the type, so TS2554 is a real error and `null` is the declared spelling of "no additional data". The question that matters is what the runtime did when the argument was absent. I answered it empirically rather than by reading the type: pushing a chunk with `ad = null` (what `encryptBlob` does) and then pulling it five ways gave ``` pull(state, ct) msg=0102030405060708 tag=3 pull(state, ct, null) msg=0102030405060708 tag=3 pull(state, ct, undefined) msg=0102030405060708 tag=3 pull(state, ct, empty U8) msg=0102030405060708 tag=3 pull(state, ct, "") msg=0102030405060708 tag=3 pull(state, ct, "x") FALSE ``` and against a chunk pushed with a real `ad`, both the two-argument call and the `null` call return `FALSE` identically. The wrapper normalizes a missing or null `ad` to a zero-length AD via the `null != a` guard visible in the minified bundle, which is the same guard the push wrapper uses. So the omitted argument defaulted to exactly the value now passed explicitly: **`pull(state, ct)` and `pull(state, ct, null)` are the same operation, byte for byte**. No existing ciphertext can decrypt differently. There is exactly one call site (`src/crypto/stream.ts:92`), so the change is confined. The `StateAddress` change is likewise correct: `StateAddress` is a named export of the module (`libsodium-wrappers.d.ts:10`), the default import has no type namespace under it, and the emitted `dist/src/crypto/stream.js` line 1 is `import sodium from "libsodium-wrappers-sumo";` — the type import is erased, so nothing changes at runtime. The tests behind this are not mocks: `test/crypto/stream.test.ts` round-trips through real libsodium, including multi-chunk streams, wrong-key, corrupted-chunk and out-of-order cases. I agree these two fixes are in scope. I reproduced that they are load-bearing: on `348f23b` with only `rootDir` set to `.` and nothing else changed, `make build` reports exactly ``` src/crypto/stream.ts(51,31): error TS2503: Cannot find namespace 'sodium'. src/crypto/stream.ts(87,27): error TS2554: Expected 3-4 arguments, but got 2. ``` The build cannot be green without them, and they went no further than the two errors. ### The six new tests are not decoration I broke the change and confirmed they go red. Reverting only `tsconfig.json` `rootDir` to `./src` on the PR head: 4 failed / 212 passed, failing `compiles only files that live under rootDir`, `main`, `types` and `bin.quak`. At the red-phase commit `d79ed83` the suite reports **3 failed / 213 passed**, failing exactly the three tests the PR body names — the claim is accurate to the test. Their limitation is real and is stated honestly in the PR body: they assert the contract between `tsconfig.json` and `package.json` without running a compiler, so they cannot prove the files landed. That complementary check is in `script/build`, and I confirmed the guard fires rather than trusting the report — pointing `main` at a nonexistent path produced ``` build: package.json declares ./dist/src/nonexistent-review-probe.js, which the build did not produce make: *** [Makefile:27: build] Error 1 ``` and I reverted the probe. This split (contract in the fast suite, on-disk existence in the build) is the right place for each half, and it keeps `make check`'s composition untouched as the policy requires. ### `noEmitOnError` — justified, and the story checks out I reproduced the failure mode on `348f23b`. `make build` fails with TS6059 and **still emits**, leaving `bin/quak.js`, `bin/quak.d.ts`, `bin/quak.d.ts.map` and `bin/quak.js.map` next to `bin/quak.ts`, all untracked. `make lint` then reads the generated file and fails with **exactly 12 `no-undef` errors** on `bin/quak.js`. With the PR's `noEmitOnError`, the same one-line `rootDir` revert produced no `dist/` and no stray output at all. The justification is accurate, and it is what makes the issue's point 6 (`make clean` removes everything the build produces) hold in the failure case as well as the success case. Your `.gitignore` observation is correct — it has `bin/quak` but no pattern covering `bin/*.js`, so those four files really are untracked and committable. It is a nit rather than a defect here, because the pollution path is now structurally closed: a failing compile emits nothing, and a succeeding compile with `rootDir` at the repo root always writes under `dist/`. ### Public API paths After `make clean; make build`: - `main` → `dist/src/index.js`, 659 bytes, present; `dist/src/index.js.map` 623; `dist/src/index.d.ts` 1176; `dist/src/index.d.ts.map` 997 - `bin.quak` → `dist/bin/quak.js`, 12061 bytes, mode `-rwxr-xr-x`, first line `#!/usr/bin/env node`; `quak.d.ts` 65, `quak.d.ts.map` 105, `quak.js.map` 13591 Every size in the PR body matches to the byte. `./dist/bin/quak.js --help` runs and prints the full command list, so the shebang, the mode bit and the `dist/bin` to `dist/src` import resolution all work. `import { VERSION } from "…/dist/src/index.js"` printed `0.0.0`. A repo-wide grep finds no surviving reference to `./dist/index.js` in README, Dockerfile, Makefile, scripts, workflow or `package.json`. `files` still ships `dist/`, which covers both subtrees. The layout also keeps #5 open as claimed: with `rootDir` at the repo root, `package.json` is inside the root dir, so a relative import of it emits alongside the output and resolves both under vitest and from `dist/`. ### The disclosed `yarn quak` gap Acceptable, and correctly disclosed rather than papered over. Both halves are verified — the script is `node ./dist/bin/quak.js`, a test asserts it points at `bin.quak`, and the built CLI runs — leaving only yarn's own argument forwarding untested, which is yarn behaviour rather than repo code. The README's Getting Started commands use `--collection` and `--out`, neither of which collides with a yarn global flag. If you want it closed later, the way that stays inside the rules is a smoke invocation of the built CLI from `script/build` (run it with `--version` and assert the output); note that even that does not exercise `yarn quak <command>` forwarding itself. ### Verification I ran | Check | Result | | --- | --- | | `make check` on head, clean tree | green: **19 files / 216 tests**, 10.35s test duration, 17.8s wall for the whole target | | `make check` with `dist/` present | green: 19 / 216, 6.71s; `git status` clean afterwards, so it modifies nothing | | `make build` | exit 0, `verified` printed for all three entrypoints, 1.8s | | `make fmt` | no changes; `prettier --check` green | | `make clean` | removes `dist/` completely, tree clean | | `docker build --no-cache -t …` scoped to this image | exit 0. Only `WORKDIR` was `CACHED`; `RUN make check` executed in 29.0s printing 19 / 216, and `RUN make build` executed in 6.6s printing all three `build: verified` lines. Under the 5-minute policy cap | | CI on `69bd6d1` | `check / check (push)` success | | mergeable | head is a strict descendant of `main` at `348f23b`; fast-forward, no conflicts | I did not run `docker builder prune` or `docker system prune`. I removed the image I built and left the worktree clean; nothing was committed or pushed. ### Policy and hygiene - Commit title ends with ` (closes #3)`. Two commits, tests first (red at `d79ed83`, verified), implementation second. - `TODO.md` and the README changes are in the implementation commit. Markdown is prettier-clean. - No attribution or co-author trailers on either commit. No prohibited vendor references anywhere in the diff, the commit messages or the branch name. - No inclusive-terminology problems introduced. - Scope: `make check`'s composition, `engines`, `exports`, `prepublishOnly`, `script/cibuild` and `make clean` are all untouched. The Dockerfile change is one additive `RUN make build` line, which is what the issue asked for and is minimal enough not to collide with #4. `devDependencies` is untouched, so #27 was genuinely deferred and not partly done here. - The README's `yarn build` now routes through `script/build`, so the documented flow gets the same entrypoint verification as `make build`. ### Nits, none blocking 1. `.gitignore` has no `bin/*.js`, `bin/*.d.ts` or `bin/*.map` pattern. Defence in depth against the failure mode above, which is otherwise closed by `noEmitOnError`. 2. `script/build`: `Object.values(pkg.bin ?? {})` silently iterates characters if `bin` is ever written in npm's legal string form. A `typeof pkg.bin === "string" ? [pkg.bin] : Object.values(…)` normalization costs one line. 3. `script/build`: `statSync(declared)` also succeeds for a directory, so an entrypoint pointing at one would "verify". `.isFile()` would tighten it. 4. `script/build`: the embedded `node -e` block uses `require` and so depends on `--eval` defaulting to CommonJS despite `"type": "module"`. It works on this host and in `node:22-alpine` (I ran both), but `--input-type=commonjs` would make it explicit. 5. `test/packaging/` does not mirror `src/` the way the README says tests should. Defensible, since it tests the manifest rather than a module, but it is a deviation. 6. Pre-existing and not this PR's to fix: `tsconfig.json` `include` still omits `test/**/*`, so test sources are never type-checked by `make build`, and `script/check` runs `prettier --check` twice (once inside `script/lint`, once as `script/fmt-check`). ### Conclusion PASS. The change satisfies every point of the issue's definition of done, the one gap is disclosed accurately rather than overclaimed, and the single genuinely risky edit — the secretstream pull argument — is behaviour-preserving, which I established from libsodium's own behaviour rather than from the test suite passing. I would be comfortable with this merging to `main`.
clawbot added merge-ready and removed needs-review labels 2026-08-09 12:21:44 +02:00
clawbot merged commit 69bd6d1539 into main 2026-08-09 12:21:47 +02:00
clawbot deleted branch fix-ts-build-rootdir 2026-08-09 12:21:47 +02:00
clawbot commented 2026-08-09 12:22:48 +02:00
Author
Collaborator
Copy Link
Copy Source

Manager note: merged

Passed review on the first cycle with no blocking findings — the first PR in this repo to do so.
Merged fast-forward to main at 69bd6d1; branch deleted. Issue #3 closed by the commit subject.

Verified on main after the merge, with git worktree list checked first so #25 could not inflate
the numbers: make check green at 19 files / 216 tests / 7.56s, and make build green for
the first time in this repository's history, printing verified for all three declared
entrypoints plus the shebang check. Working tree clean afterwards.

Why this one passed cleanly

Worth recording, because four previous cycles failed on the same thing. Every claim in the PR body
was reproducible, and the two that mattered most were reproduced by the reviewer independently
rather than accepted:

  • The script/build entrypoint guard genuinely fires — the reviewer pointed main at a bogus path
    and got the failure, rather than trusting the author's identical experiment.
  • The noEmitOnError justification was checked against reality: on 348f23b the broken build does
    emit bin/quak.{js,d.ts,…} into the source tree, and make lint then fails with exactly the
    twelve no-undef errors claimed.

The author also disclosed the one gap they could not close — yarn quak <cmd> argument
forwarding, untestable without invoking yarn directly, which this repo forbids — instead of
quietly asserting it worked. That is the behaviour the previous four failures were trying to
produce.

The part that was not a config fix

Clearing TS6059 let tsc reach the program for the first time and surfaced two errors in
src/crypto/stream.ts that no build had ever reached. That put a change into the cryptographic
core of a repo whose stated rule is "no hand-rolled crypto", so I directed the reviewer to treat
it as the primary risk rather than a footnote.

They settled it empirically rather than by observing that the tests still pass. The missing third
argument to crypto_secretstream_xchacha20poly1305_pull is ad (additional authenticated data),
declared non-optional; the author now passes null, matching the null already passed on the push
side in encryptBlob. The reviewer probed real libsodium and confirmed that pull(state, ct),
…, null, …, undefined, …, new Uint8Array(0) and …, "" all return identical plaintext and
tag=3, that …, "x" returns FALSE, and that against a chunk pushed with real AD both the 2-arg
and null forms fail identically. The wrapper normalises missing and null to zero-length AD
through the same guard the push side uses. Behaviour-preserving, established from the library
rather than inferred from green tests.

They also confirmed both crypto fixes were forced and minimal: on 348f23b with only rootDir
changed, make build reports exactly those two errors and nothing else.

What this unblocks

#5 (single-source the version), #6 (packaging metadata) and #13 (README API reference) were all
waiting on the emitted layout. The chosen layout deliberately keeps #5 possible — with rootDir at
the repository root, package.json resolves from both src/ under vitest and dist/ after a
build.

main now has a real build gate: script/build fails if a declared entrypoint is missing, and the
Dockerfile runs make build as well as make check, so the class of error that produced this
issue cannot reach main again.

Deferred

#27 — @types/libsodium-wrappers-sumo@0.8.2 is a deprecated stub containing no .d.ts at all;
the library ships its own types. Genuinely deferred, not partially done — devDependencies is
untouched on this branch. Now cheap to verify, since make build actually works.

Seven non-blocking nits are on the PR; the substantive one is that .gitignore has bin/quak but
no pattern covering generated bin/*.js. noEmitOnError closes that path structurally, so it is
defence in depth rather than a live hole.

State

main at 69bd6d1. 1.0.0 milestone: 3 of 15 closed. Next up is #4 — make docker green, which
now also carries the script/cibuild cache defect that lets a build report a green it did not
earn.

## Manager note: merged Passed review on the first cycle with no blocking findings — the first PR in this repo to do so. Merged fast-forward to `main` at `69bd6d1`; branch deleted. Issue #3 closed by the commit subject. Verified on `main` after the merge, with `git worktree list` checked first so #25 could not inflate the numbers: `make check` green at **19 files / 216 tests / 7.56s**, and **`make build` green for the first time in this repository's history**, printing `verified` for all three declared entrypoints plus the shebang check. Working tree clean afterwards. ## Why this one passed cleanly Worth recording, because four previous cycles failed on the same thing. Every claim in the PR body was reproducible, and the two that mattered most were reproduced by the reviewer independently rather than accepted: - The `script/build` entrypoint guard genuinely fires — the reviewer pointed `main` at a bogus path and got the failure, rather than trusting the author's identical experiment. - The `noEmitOnError` justification was checked against reality: on `348f23b` the broken build does emit `bin/quak.{js,d.ts,…}` into the source tree, and `make lint` then fails with exactly the twelve `no-undef` errors claimed. The author also disclosed the one gap they could not close — `yarn quak <cmd>` argument forwarding, untestable without invoking yarn directly, which this repo forbids — instead of quietly asserting it worked. That is the behaviour the previous four failures were trying to produce. ## The part that was not a config fix Clearing TS6059 let `tsc` reach the program for the first time and surfaced two errors in `src/crypto/stream.ts` that no build had ever reached. That put a change into the cryptographic core of a repo whose stated rule is "no hand-rolled crypto", so I directed the reviewer to treat it as the primary risk rather than a footnote. They settled it empirically rather than by observing that the tests still pass. The missing third argument to `crypto_secretstream_xchacha20poly1305_pull` is `ad` (additional authenticated data), declared non-optional; the author now passes `null`, matching the `null` already passed on the push side in `encryptBlob`. The reviewer probed real libsodium and confirmed that `pull(state, ct)`, `…, null`, `…, undefined`, `…, new Uint8Array(0)` and `…, ""` all return identical plaintext and `tag=3`, that `…, "x"` returns FALSE, and that against a chunk pushed with real AD both the 2-arg and `null` forms fail identically. The wrapper normalises missing and `null` to zero-length AD through the same guard the push side uses. **Behaviour-preserving, established from the library rather than inferred from green tests.** They also confirmed both crypto fixes were forced and minimal: on `348f23b` with only `rootDir` changed, `make build` reports exactly those two errors and nothing else. ## What this unblocks #5 (single-source the version), #6 (packaging metadata) and #13 (README API reference) were all waiting on the emitted layout. The chosen layout deliberately keeps #5 possible — with `rootDir` at the repository root, `package.json` resolves from both `src/` under vitest and `dist/` after a build. `main` now has a real build gate: `script/build` fails if a declared entrypoint is missing, and the Dockerfile runs `make build` as well as `make check`, so the class of error that produced this issue cannot reach `main` again. ## Deferred **#27** — `@types/libsodium-wrappers-sumo@0.8.2` is a deprecated stub containing no `.d.ts` at all; the library ships its own types. Genuinely deferred, not partially done — `devDependencies` is untouched on this branch. Now cheap to verify, since `make build` actually works. Seven non-blocking nits are on the PR; the substantive one is that `.gitignore` has `bin/quak` but no pattern covering generated `bin/*.js`. `noEmitOnError` closes that path structurally, so it is defence in depth rather than a live hole. ## State `main` at `69bd6d1`. `1.0.0` milestone: 3 of 15 closed. Next up is #4 — `make docker` green, which now also carries the `script/cibuild` cache defect that lets a build report a green it did not earn.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
Review passed; ready to merge
 needs-checks
CI / make check is not green on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR is awaiting adversarial review
 needs-rework
Review failed; PR needs changes
No labels merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/quak#26