Skip to content

fix(env): fixed zod error changing to preprocess and coercing includi…#531

Open
SandipGyawali wants to merge 2 commits into
OpenRouterTeam:mainfrom
SandipGyawali:main
Open

fix(env): fixed zod error changing to preprocess and coercing includi…#531
SandipGyawali wants to merge 2 commits into
OpenRouterTeam:mainfrom
SandipGyawali:main

Conversation

@SandipGyawali

@SandipGyawali SandipGyawali commented Jun 29, 2026

Copy link
Copy Markdown
  • changed the zod logic for OPENROUTER_DEBUG KEY
  • added test case for coerce behavior and preprocessor

perry-the-pr-reviewer[bot]

This comment was marked as outdated.

@perry-the-pr-reviewer perry-the-pr-reviewer Bot 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.

⚠️ APPROVE unavailable on this installation — the maintainer GitHub App does not have pull_requests: write on OpenRouterTeam. The verdict below is posted as COMMENT. Event-level approval (for branch-protection / review requirements) must be added out-of-band.

Perry's Review

Fixes OPENROUTER_DEBUG=false (and similar falsy strings) incorrectly enabling debug mode. Previously z.coerce.boolean("false") returns true, so the preprocess guard is the right fix. The new test coverage is thorough.

Verdict: ✅ LGTM

Details

Risk: 🟢 Low — debug-logging path only; no auth, payments, or hot paths; fully reversible

CI: no checks present

Findings (see inline comments for full context):

  • nit: resetEnv() in beforeEach is a no-op here — tests call envSchema.parse() directly (stateless), not env(), so clearing the memo has no effect. Harmless but slightly misleading.

Codex: skipped (small tier)

Research: skipped (small tier)

Security: no concerns — only affects debug-logging opt-in; no credential handling touched

Test coverage: 120-line test file covering falsy strings (false, 0, off, no, empty), case-insensitive variants, boolean pass-through, and undefined — good blast coverage.

Unresolved threads: 1 outdated Perry thread (prior import suggestion, resolved by this push)

Scope: first review (full)
Review: tier=small · model=claude-sonnet-latest · score=1.7

Comment thread src/lib/env.ts
OPENROUTER_APP_CATEGORIES: z.string().optional(),

OPENROUTER_DEBUG: z.coerce.boolean().optional(),
OPENROUTER_DEBUG: z.preprocess(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] src/lib/env.ts carries a Code generated by Speakeasy — DO NOT EDIT header — verify this change survives regeneration.

Details

Why: Speakeasy regenerates this file from the OpenAPI spec. With persistentEdits: enabled: "true" in .speakeasy/gen.yaml, Speakeasy will attempt to re-apply manual edits after regeneration, but this is not guaranteed — especially for logic changes inside schema fields. If a Speakeasy upgrade or spec bump triggers full file regeneration, the preprocess guard and tests may silently diverge from the schema.

Options:

  1. Run speakeasy run locally and confirm the preprocess block survives in the regenerated output.
  2. If persistentEdits is unreliable for this pattern, add a Speakeasy overlay (similar to .speakeasy/overlays/boolean-query-params.overlay.yaml) to make this the generated default.
Prompt for agents
In `src/lib/env.ts` at line 38, the manual `z.preprocess` block for `OPENROUTER_DEBUG` lives inside a Speakeasy-generated file (`DO NOT EDIT` header). Verify that `.speakeasy/gen.yaml`'s `persistentEdits: enabled: "true"` actually preserves this block by running `speakeasy run` locally and confirming the preprocess logic is still present in the regenerated output. If not preserved, either add a `.speakeasy/overlays/` overlay to codify this schema change, or document the manual steps required after each regeneration.

Reviewed at eed9ad8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant