Enable global strict typechecking with real JSDoc types#2
Merged
Conversation
Annotate the core lib layer (credentials, control-fetch, command, config-state, output, dotenv, ns-pattern, stdin, token-store, common, bundle-modules, package-info) so it passes `tsc --strict` with real types and zero `any`/`@ts-` suppressions. Self-guarding validators take `unknown`; wire-parsed JSON stays `unknown` until narrowed. Introduces two shared typedefs the rest of the conversion references: - ControlResponse (control-fetch.js): the controlFetch/readControlResponse shape, with json() typed Promise<unknown>. - TokenStore (token-store.js): the on-disk store reader shape. Behavior unchanged; types only. Downstream commands/tests now see the new typedefs and are converted in following commits (global strict flips on at the end of the series). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Annotate the remaining non-test source — lib/wrangler/* + wrangler-pack + d1-files, the lib formatters (d1/r2/delete/workers/workflows-format, whoami), all commands/*, and bin/wdl.js — so the whole source tree passes `tsc --strict` with real types and zero `any`/`@ts-` suppressions. Highlights: - New wire-shape typedefs where formatters/commands read fixed fields (WranglerConfig, D1Database, R2Object/R2Bucket, WhoamiBody, SseEvent/ TailPayload, DoctorCheck, etc.); parsers that re-validate their input take `unknown` and narrow. - defineCommand's run spec uses method syntax with `values: Record<string, unknown>` (bivariant), so each command can type `values` from its own flags without an `any` escape in the framework contract. - EventEmitter surfaces (StdinLike, ControlClientRequest, ControlBodySource) use a generic `<A extends unknown[]>` listener instead of `any[]`. Behavior unchanged; types only. All 374 unit tests pass. Tests are converted and global strict flips on in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Annotate the unit + integration test suite (helpers.js and all cli-*.test.js, plus the hosted cli-live integration test) with real JSDoc types: typed mock deps/fakes, recorder arrays, response stubs (which fit the looser ControlResponseStatus/ControlJsonResponse shapes), and `catch (err)` narrowing. Flip tsconfig.json `strict` to true, so the whole tree — bin, commands, lib, tests — is now checked under `tsc --strict` and `npm run typecheck` (already run in CI) enforces it. Also widens WorkerSummary.activeVersion to `string | null` to match what the control plane actually sends for an undeployed worker. Whole repo: 0 strict errors, 0 `any`, 0 `@ts-` suppressions, eslint clean, 374/374 unit tests pass. Types only; no runtime behavior changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Two real gaps surfaced while typing the code (behavior fixes, not types): - parseServicesFromCfg only truthiness-checked `binding`/`service`. A non-string truthy `service` flowed unchanged into the deploy manifest, and a non-string `binding` (e.g. ["AB"]) would be String()-coerced past the BINDING_NAME_RE check. Now both must be non-empty strings, matching the d1/r2 parsers. - ensureControlContextFromConfigState returned `controlUrl.value` (possibly null) typed as string. Now fails closed with "No control URL configured" when the value is unresolved and no error was recorded. Adds unit coverage for both. All 375 unit tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Address findings from the post-conversion review (all low-severity): - d1-files.js: restore the empty-message fallback in readMigrationFiles' catch (`err instanceof Error && err.message ? err.message : String(err)`), matching the pattern used elsewhere, so an empty-message Error still prints a reason. - common.js / init.js: drop duplicate stacked JSDoc blocks left by the conversion (formatOption, readWdlCliPackage). - wrangler: consolidate the byte-identical `asRecord` helper (was duplicated in config.js and bindings.js) into its single owner lib/wrangler/utils.js. - tests: share the `ControlCall` typedef from helpers.js instead of redefining it in cli-config-doctor and cli-lifecycle. Behavior-preserving (the catch change restores prior behavior); 375 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Remove prose that only explained why a JSDoc cast/narrowing was written (deploy.js pack-options cast, command.js dep-merge readback, r2.js stream body narrow, common.js json-reader guard). These describe the type checker, not runtime behavior, and don't match the repo's domain-comment style. Comments stating real runtime invariants are kept. No code or type change; 375 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
CONTRIBUTING.md and AGENTS.md described `tsc --noEmit` but predated the `strict` flip. Note that typecheck now runs under `strict` and new code needs real JSDoc types (no implicit `any`; `unknown` + narrowing for runtime-validated values). Docs-only; English prose hard-wrapped at 80 per AGENTS.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Re-review of the branch found the asRecord dedup left one inline copy behind in wrangler-pack.js (the assets-config narrow); route it through the shared lib/wrangler/utils.js helper too, so the "non-null non-array object as record" logic now lives in exactly one place. Also restore a one-line rationale on deploy.js's packWranglerProject options cast — the earlier comment trim stripped it bare, unlike the kept one-liners on the r2/common casts. No behavior change; 375 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Address consistency/quality findings from the complete branch review (no correctness bugs were found): - Empty-message catch fallback: align the remaining `err instanceof Error ? err.message : String(err)` sites (config.js, wrangler-pack.js, d1.js x2) with the `&& err.message` form already used in d1-files.js, so an empty-message Error still yields a reason. - Injected-dep typing: deploy.js and r2.js read their command-specific deps (execFile / stdoutStream) via a named CommandContext intersection typedef and a single cast, matching doctor.js/tail.js, instead of casting context through `unknown` (which discarded the rest of the context's typing). - defineCommand: document why `run` uses method syntax (intentional bivariance). - Tests: type the shared mockDeps `calls` as ControlCall[] so cli-d1 no longer needs its RecordedCall/asCall re-narrowing shim; RecordedCall now aliases the shared helper type. Verified the "secret.js re-evaluates isNonEmptyString" finding is a false positive: isNonEmptyString is a `value is string` type predicate, so the inline calls are load-bearing for narrowing values.worker and cannot be replaced with the hasWorker boolean. Types/cleanup only; 375 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
The deploy pack-options cast carried a comment describing the type system (defaults-inferred narrower param types), not runtime behavior — the same category trimmed earlier. The `/** @type {Parameters<...>[0]} */` cast already reads as deliberate, so remove the note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
There was a problem hiding this comment.
Pull request overview
This pull request enables TypeScript strict checking across the CLI’s existing ESM JavaScript codebase by adding comprehensive, “real” JSDoc types (no .ts conversion / no emit), while also incorporating two small runtime hardening fixes discovered during typing work.
Changes:
- Turn on
tscstrict mode (tsconfig.json) and annotate the CLI implementation + tests with concrete JSDoc types/guards. - Introduce/propagate shared typed shapes for control-plane IO, Wrangler config parsing, command context, and test fakes.
- Apply and test two behavior fixes: stricter
[[services]]parsing and failing closed when the control URL is unresolved.
Reviewed changes
Copilot reviewed 55 out of 57 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Enables global strict typechecking. |
| tests/unit/helpers.js | Adds shared ControlCall typedef + typed test deps helpers. |
| tests/unit/cli-token.test.js | Tightens test typings (stdin fakes, caught error narrowing, controlFetch fakes). |
| tests/unit/cli-token-store.test.js | Adds JSDoc typing for temp-dir and permission tests. |
| tests/unit/cli-stdin.test.js | Types TTY/raw-mode fakes and stderr capture arrays. |
| tests/unit/cli-output.test.js | Types stdout capture and callback parameters. |
| tests/unit/cli-init.test.js | Types exit-capture wiring and console overrides for strict mode. |
| tests/unit/cli-deploy.test.js | Adds typed recorded-call shapes and narrows JSON parsing in assertions. |
| tests/unit/cli-d1.test.js | Adds typed request-body parsing helpers and recorded-call typing. |
| tests/unit/cli-credentials.test.js | Adds typed env/warnings collections and loader signatures. |
| tests/unit/cli-control-fetch.test.js | Types fake transport/streams for strict mode streaming tests. |
| tests/unit/cli-config-doctor.test.js | Expands strict typings + adds coverage for fail-closed control URL context. |
| tests/unit/cli-command.test.js | Types command-spec test scaffolding and runtime-invalid spec casts. |
| lib/wrangler/utils.js | Adds typed helpers (manifestMap, hasOwn, asRecord). |
| lib/wrangler/modules.js | Types module collection return shape. |
| lib/wrangler/config.js | Types Wrangler config shapes and improves error message extraction. |
| lib/wrangler/command.js | Adds typed exec failure shaping + narrows formatter inputs. |
| lib/wrangler/bindings.js | Adds typed binding parsers; hardens [[services]] string validation. |
| lib/wrangler/assets.js | Adds typed signatures to assets dir resolution and ignore matcher helpers. |
| lib/wrangler-pack.js | Adds manifest typedefs + tighter typing around Wrangler config/assets parsing. |
| lib/workflows-format.js | Adds typed workflow formatter input shapes. |
| lib/workers-format.js | Adds typed worker formatter input shapes. |
| lib/whoami.js | Adds whoami response typedefs + fail-closed config-state→control-context helper. |
| lib/token-store.js | Adds TokenStore typedef and uses hasErrorCode for fs error narrowing. |
| lib/stdin.js | Refines StdinLike typing and narrows callbacks in stdin readers. |
| lib/r2-format.js | Adds typed shapes for R2 list/head formatters. |
| lib/package-info.js | Adds return-type annotations for package.json helpers. |
| lib/output.js | Adds typed signatures for output helpers and warning formatting. |
| lib/ns-pattern.js | Adds predicate return types for identifier/ns validators. |
| lib/dotenv.js | Adds typed signatures for dotenv parsing/quoting helpers. |
| lib/delete-format.js | Adds typed delete-format body shapes; narrows numeric checks. |
| lib/d1-format.js | Adds typed shapes for D1 formatter inputs. |
| lib/d1-files.js | Hardens --file validation and adds typed migration-file filtering. |
| lib/credentials.js | Adds typed signatures for env resolution and uses hasErrorCode for ENOENT. |
| lib/control-fetch.js | Adds typed ControlResponse/init/transport surfaces and improves stream typing. |
| lib/config-state.js | Introduces ConfigEntry typedef and tightens config resolution typing. |
| lib/common.js | Adds option/typedef structure and introduces hasErrorCode guard. |
| lib/command.js | Tightens CommandContext typing and stabilizes fetch helper signatures. |
| lib/bundle-modules.js | Adds typed signatures for module wire encoding helpers. |
| CONTRIBUTING.md | Documents strict tsc expectations and “real JSDoc” requirement. |
| commands/workflows.js | Types workflow command flag values and narrows formatter inputs. |
| commands/workers.js | Types workers list response shape passed to formatter. |
| commands/whoami.js | Types whoami command flag values and output mode selection. |
| commands/token.js | Introduces typed TokenValues and token list row typedefs. |
| commands/tail.js | Adds typed SSE/tail payload shapes and strict typing across stream/render logic. |
| commands/secret.js | Adds typed secret response/warning shapes and narrows worker scope checks. |
| commands/r2.js | Adds typed R2Context, header handling, and streaming body typing. |
| commands/init.js | Adds typed CLI arg parsing and filesystem error narrowing. |
| commands/doctor.js | Adds typed doctor context/check shapes and strict typing for config/whoami checks. |
| commands/deploy.js | Adds deploy warning typedefs + typed pack options and control responses. |
| commands/delete.js | Types delete command flag values and formatter inputs. |
| commands/d1.js | Introduces D1Flags and tightens request/response shapes and param parsing. |
| commands/config.js | Types config command flag values + public config entry shape. |
| bin/wdl.js | Adds dispatcher module typedefs and tighter typing around loadEnv overrides. |
| AGENTS.md | Documents strict tsc + JSDoc typing expectations for contributors/agents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The `values` JSDoc for tail and doctor listed `control?: string`, but there is no `--control` flag — both commands use the `control` option preset, which provides `--control-url`, `--token`, and `--no-token-store`. Replace the phantom field with the real flag names so the strict JSDoc reflects the actual parsed shape. Per PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
collectRoutes only iterated `cfg.routes` when it was an array, so a mistakenly non-array `routes` (string/object) was silently dropped and the worker would deploy with no routes. Reject it with a clear error instead, matching the "map or loudly reject — silent drops are bugs" contract the other wrangler parsers follow. Export collectRoutes and add focused coverage. Per PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
`assets.directory` was read as `unknown` and passed to resolveAssetsDir on a truthiness check, so `directory: ""` was silently ignored and a non-string value hit path.resolve as a low-level TypeError. Validate it in resolveAssetsDir (the exported, tested seam): reject a missing/empty/non-string value with a clear `assets.directory must be a non-empty string` error, and gate the caller on "present" rather than truthy so an empty/invalid value fails loud instead of being dropped. Add focused coverage. Per PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
The caller comment restated what resolveAssetsDir already documents (it validates the value and checks the path on disk). Keep only the non-obvious bit: why the gate is `!== undefined` rather than truthy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
`if (!entry.binding || !entry.service)` treated an empty string as missing, so `binding: ""` got the generic "needs both" error instead of the specific non-empty-string validation just below. Switch to a nullish check so a present-but-empty/invalid value falls through to the clearer error; genuinely missing fields still report "needs both". Add coverage for the empty-string case. Per PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
secret.js called isNonEmptyString(values.worker) three times (for hasWorker and again when building secretPath/scopeLabel). The repeats were there to re-narrow values.worker to a string, since hasWorker is a plain boolean. Narrow once into a `worker` (string | null) const and derive hasWorker from it, so the predicate lives in one place and secretPath/scopeLabel use the narrowed value directly. No behavior change; 377 tests pass. Per PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
The nullish-check rationale and the string-type-check rationale had become two adjacent comment blocks on consecutive lines. Combine them into one. Comment-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
Empty `new Map()` / `new Set()` infer to `Map<any, any>` / `Set<any>` (unlike array literals, they don't evolve from later mutations), which undercut the no-`any` goal even though strict doesn't flag them. Give them explicit element types: config-state's `sources` (Map<string,string>), uniquePaths' `seen`/`out` (Set<string> / string[]), and the credentials-test `protectedKeys` fixtures (Set<string>). Containers already in a typed argument position keep their contextual type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 57 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/unit/cli-stdin.test.js:93
- The PR description says there are zero
anyuses, but this branch still has/** @type {any} */casts (e.g.confirmAction({ stdin: /** @type {any} */ ({ isTTY: false }), … })later in this file). Please replace these withunknown/structural types (or update the PR description if keeping them is intentional).
The "zero any" claim missed three `/** @type {any} */` casts in test files, all reading an off-type property on a probe value: the prototype-pollution checks in cli-deploy and cli-token-store now cast to `Record<string, unknown>`, and the partial stdin stub in cli-stdin casts through `unknown` to `StdinLike`. No more `any` anywhere in the tree. Per PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
packWranglerProject declares it returns `workerName: string` but only truthiness-checked `cfg.name`, then asserted it to string. The dry-run bundle runs against a sanitized temp name, so Wrangler never validates the original cfg.name — a non-string truthy value would be returned as a bogus string workerName. Check `typeof === "string"` and non-empty, which also lets the return drop its `/** @type {string} */` cast. Per PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
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
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
Flip the CLI to TypeScript
strictglobally (tsconfig.json→"strict": true) and annotate the whole tree —bin/,commands/,lib/, and the test suite — with real JSDoc types. No.tsconversion, no build step: it stays plain ESM JS checked bytsc --noEmit, matching the repo's existing approach.npm run typecheck(already run in CI on every PR) now enforces strict, so no workflow change is needed.What changed
ControlResponse(+ looserControlResponseStatus/ControlJsonResponsefor test fakes),TokenStore, plus a sharedhasErrorCodeguard.WranglerConfig,D1Database,R2Object,WhoamiBody,SseEvent,DoctorCheck, …); parsers that re-validate their input takeunknownand narrow.catch (err)narrowing); the sharedControlCallshape lives once intests/unit/helpers.js.strictflipsfalse→true; every other option/include/exclude is unchanged.No
anyescape hatchesThe conversion uses real types throughout — zero
any(includingRecord<string, any>and(...args: any[])) and zero@ts-ignore/@ts-expect-error/@ts-nocheck. Notable choices:unknownand narrow.<A extends unknown[]>listener instead ofany[].defineCommand'srunuses method syntax (bivariant) so each command types its ownvaluesshape without ananyin the framework contract.Behavior changes
Aside from these, the change is types-only. Typing the parsers surfaced several places where malformed wrangler config was silently dropped or produced a cryptic low-level error rather than the "map or loudly reject — silent drops are bugs" contract the rest of the parsers follow. Each is now a clear, fail-fast error, and each is covered by a new unit test:
parseServicesFromCfgnow requiresbindingandserviceto be non-empty strings, matching the d1/r2 parsers — previously a non-string truthyserviceflowed into the deploy manifest and a non-stringbinding(e.g.["AB"]) wasString()-coerced past the binding-name regex.ensureControlContextFromConfigStatefails closed with "No control URL configured" when the URL is unresolved, instead of returningnulltyped asstring.collectRoutesthrows whenroutesis present but not an array, instead of silently ignoring it and deploying a worker with no routes. (Added in review.)resolveAssetsDir/assets.directoryrejects a present-but-empty or non-stringdirectorywith a clear error, instead of silently skipping""or hittingpath.resolveas aTypeError. (Added in review.)All four are intentional hardenings that only reject input that was never valid; no previously-valid config is rejected.
Docs
CONTRIBUTING.mdandAGENTS.mdnow note thattscruns understrictand new code needs real JSDoc types.CHANGELOG.mdis intentionally untouched (this repo curates it at release time, not per-PR).Checks
npm run lint,npm run typecheck(strict), andnpm testall pass — 377/377 unit tests green, eslint clean. The diff is types-only apart from the four tested config-validation hardenings above.The branch went through two review passes (incremental, then a full 8-angle pass over the whole diff) plus the inline PR review; cleanups and fixes from all of them are folded in.
🤖 Generated with Claude Code