Skip to content

[codex] Fix dynamic handler test env cleanup#5

Closed
coe0718 wants to merge 2 commits intoghostwright:mainfrom
coe0718:codex/fix-dynamic-handlers-lint
Closed

[codex] Fix dynamic handler test env cleanup#5
coe0718 wants to merge 2 commits intoghostwright:mainfrom
coe0718:codex/fix-dynamic-handlers-lint

Conversation

@coe0718
Copy link
Copy Markdown
Contributor

@coe0718 coe0718 commented Mar 31, 2026

Summary

Fixes the pre-existing Biome lint failure in dynamic-handlers.test.ts without changing the test semantics.

What Changed

  • replaces delete process.env.* cleanup with a small restoreEnvVar() helper
  • uses Reflect.deleteProperty() so env vars are actually removed instead of being set to the string "undefined"
  • keeps the original env value when one existed before the test
  • applies Biome import ordering in the file

Why

Biome flags the delete operator in this test file, but its suggested replacement (process.env.KEY = undefined) is behaviorally unsafe for Node/Bun because env vars become the string "undefined" instead of being removed.

This keeps the lint fix small and behavior-preserving.

Validation

  • bash -lc 'export PATH="$HOME/.bun/bin:$PATH" && /home/coemedia/.bun/bin/bun test src/mcp/__tests__/dynamic-handlers.test.ts'
  • bun run lint
  • ./node_modules/.bin/biome check src/mcp/__tests__/dynamic-handlers.test.ts

@mcheemaa
Copy link
Copy Markdown
Member

Hey @coe0718 - thanks for catching this! Really appreciate you taking the time to put together a fix.

We ended up fixing the same lint issue in v0.18.0 (commit 2bca825, merged via #6), but looking at both approaches more carefully, your Reflect.deleteProperty() pattern is actually more correct than our process.env.X = undefined. Setting undefined on process.env can coerce to the string "undefined" instead of actually removing the key. Your approach properly removes it.

We're going to close this PR since the changes overlap and would conflict, but we'll adopt the Reflect.deleteProperty pattern in a follow-up. Good catch on the semantics.

Your memory PRs (#2, #3, #4) are excellent work - we're reviewing those now. Thanks again!

@mcheemaa mcheemaa closed this Mar 31, 2026
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.

2 participants