Skip to content

Tighten remaining wrangler parser contracts and field types#3

Merged
cnluzhang merged 8 commits into
mainfrom
claude/cli-security-hardening-tctybe
Jun 30, 2026
Merged

Tighten remaining wrangler parser contracts and field types#3
cnluzhang merged 8 commits into
mainfrom
claude/cli-security-hardening-tctybe

Conversation

@cnluzhang

@cnluzhang cnluzhang commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the strict-typecheck PR (#2). Closes the two parser-consistency
gaps and the validated-but-unknown field types that were called out in that
review but deferred to a focused PR.

Parser consistency — "map or loudly reject"

The d1/r2/services parsers reject malformed config loudly; the last two loose
spots now match:

  • kv_namespaces: replaced the inline cfg.kv_namespaces || [] loop with a
    dedicated, exported, unit-tested parseKvNamespacesFromCfg. It rejects a
    non-array list, a non-table entry, and a missing/empty/non-string binding or
    id. Previously the list was cast and iterated, so a non-array produced a
    confusing error and a non-string binding/id was String()-coerced past the
    binding-name regex.
  • assets: a present-but-non-table [assets] block is now rejected instead
    of being nulled out by asRecord() and silently skipping assets.

Field types — tighten unknown → string

Several parser results validate a field at runtime but typed it unknown
because the validators weren't TS type predicates. Annotate
isValidJsClassDeclarationName, isAdminAcceptableNs, and isReservedNs as
value is string predicates (JSDoc only — runtime unchanged, regex/logic stays
in sync with shared/ns-pattern.js), and capture the service entrypoint/ns
in the same block as their validation so the narrowing flows. These now type as
string:

  • ServiceBinding entrypoint / ns
  • Durable Object and Workflow className
  • export entrypoint

Checks

npm run lint, npm run typecheck (strict), npm test, and npm pack --dry-run all pass — 378/378 unit tests (new parseKvNamespacesFromCfg
coverage). Behavior change is limited to the two loud-reject hardenings above,
each of which only rejects input that was never valid.

🤖 Generated with Claude Code

Generated by Claude Code

claude added 2 commits June 30, 2026 01:26
@claude
Harden kv_namespaces and assets parser contracts
524c175 
Bring the last two loose wrangler parsers in line with the "map or loudly
reject" contract the d1/r2/services parsers follow:

- Add parseKvNamespacesFromCfg (exported, unit-tested) replacing the inline
  kv_namespaces loop. It rejects a non-array table list, a non-table entry, and
  a missing/empty/non-string `binding` or `id` — previously `cfg.kv_namespaces
  || []` was cast and iterated, so a non-array gave a confusing error and a
  non-string binding/id was String()-coerced past the binding-name regex.
- Reject a present-but-non-table [assets] block instead of letting asRecord()
  null it out and silently skip assets.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@claude
Tighten parser field types via type-predicate validators
a9becf4 
The wrangler parsers validated several fields at runtime but typed them
`unknown` because the validators weren't TS type predicates. Annotate
isValidJsClassDeclarationName, isAdminAcceptableNs, and isReservedNs as
`value is string` predicates (JSDoc only; runtime unchanged, regex/logic stays
in sync with shared/ns-pattern.js), and capture the service entrypoint/ns in
the same block as their validation so the narrowing flows. The parser results
now type these as `string` instead of `unknown`:
- ServiceBinding `entrypoint` / `ns`
- Durable Object and Workflow `className`
- export `entrypoint`

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@cnluzhang cnluzhang requested a review from Copilot June 30, 2026 01:32
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR is a follow-up tightening of the Wrangler config parsing/typing layer in the WDL CLI deploy pipeline, aligning remaining loose parser contracts with the repo’s “validate or loudly reject” approach while improving JSDoc-driven type narrowing under tsc --strict.

Changes:

  • Introduces and unit-tests parseKvNamespacesFromCfg, and uses it in packWranglerProject instead of ad-hoc iteration/casting.
  • Makes [assets] fail fast when present but not a table (instead of being silently ignored).
  • Converts several validators to JSDoc type predicates so validated fields narrow from unknown to string (e.g., service entrypoint/ns, DO/workflow className, export entrypoint).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/unit/cli-deploy.test.js Adds focused unit coverage for parseKvNamespacesFromCfg validation behavior.
lib/wrangler/bindings.js Adds KV namespace parser; tightens binding result types and uses predicate narrowing for validated string fields.
lib/wrangler-pack.js Switches KV parsing to the shared parser and rejects non-table [assets] blocks.
lib/ns-pattern.js Adds JSDoc type-predicate return annotations for validators to enable strict narrowing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/wrangler/bindings.js Outdated
Comment thread lib/wrangler/bindings.js
@claude
Trim kv id and tighten newClasses to Set<string>
e5a77d0 
Two PR-review follow-ups:
- parseKvNamespacesFromCfg stored the untrimmed `id`, so "abc " passed
  validation but forwarded trailing whitespace. Store `.trim()`ed binding/id
  like the d1/r2 parsers; add coverage.
- newClasses no longer needs Set<unknown> now that isValidJsClassDeclarationName
  is a `value is string` predicate and class_name narrows to string.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@cnluzhang cnluzhang requested a review from Copilot June 30, 2026 01:39
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread lib/wrangler-pack.js Outdated
@claude
Reject a non-table [assets] before bundling, with coverage
21a53d7 
The [assets]-must-be-a-table check ran during manifest assembly, after the
wrangler dry-run bundle — so a malformed config bundled first and was also
untestable via the standard "reject before bundling" pattern. Move it next to
the other pre-bundle cfg-shape checks (name/main/vars) so it fails fast, and add
a runDeployCommand test asserting it rejects before execFile is called.

Per PR review feedback.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@cnluzhang cnluzhang requested a review from Copilot June 30, 2026 01:45
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@claude
Tighten WorkerManifest workflow/service field types to string
fdbfb6f 
Now that parseWorkflowsFromCfg and parseServicesFromCfg return string-typed
className/service/entrypoint/ns, reflect that in the manifest types instead of
leaving them `unknown`: WorkerManifest.workflows[].className and the inline
service-binding entry (service, entrypoint, ns) are now `string`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@cnluzhang cnluzhang requested a review from Copilot June 30, 2026 02:35
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread tests/unit/cli-deploy.test.js Outdated
Comment thread lib/wrangler/bindings.js
@claude
Reject unknown keys in parseKvNamespacesFromCfg
91d627c 
Allowlist binding/id/preview_id and throw on any other [[kv_namespaces]] entry
key, matching the d1/r2 parsers — so a typo or unsupported field is loudly
rejected instead of silently ignored. preview_id (a `wrangler dev` concept WDL
doesn't use) is accepted but ignored. Add coverage for both.

Per PR review feedback.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@cnluzhang cnluzhang requested a review from Copilot June 30, 2026 02:41
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@claude
Allow Wrangler's remote key in kv_namespaces
840dc9a 
`remote` (force `wrangler dev` to use real Cloudflare KV) is a valid Wrangler v4
local-dev field with no meaning for WDL deploy. Add it to the kv allowlist
alongside preview_id so it's accepted but ignored (only binding/id are
forwarded to the manifest); add coverage.

Per PR review feedback.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@cnluzhang cnluzhang requested a review from Copilot June 30, 2026 02:59
Copilot AI reviewed Jun 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@claude
Drop redundant comment labels in the kv parser test
43666ce 
Three test-section labels just restated what the assert arguments already show
(trimmed values, missing/empty/non-string binding, missing/non-string id).
Remove them; the comments that convey non-obvious intent (grammar still
enforced, d1/r2 consistency, allow-but-ignore) stay.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W9ZkKKDBaExknoWiERCDen
@cnluzhang cnluzhang merged commit 347778b into main Jun 30, 2026
6 checks passed
@cnluzhang cnluzhang deleted the claude/cli-security-hardening-tctybe branch June 30, 2026 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cnluzhang @claude