Skip to content

refactor(core): move dotenv helpers to core#1372

Merged
Mateus Zitelli (MateusZitelli) merged 5 commits into
mainfrom
wiz-dotenv-core
Jun 16, 2026
Merged

refactor(core): move dotenv helpers to core#1372
Mateus Zitelli (MateusZitelli) merged 5 commits into
mainfrom
wiz-dotenv-core

Conversation

@MateusZitelli

@MateusZitelli Mateus Zitelli (MateusZitelli) commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Summary

This is the foundational refactor for the env-var work:

  • Move dotenv parse/serialize helpers from domains/flows to core
  • Update existing callers to import from ~/core/dotenv.js
  • Keep strict serializeDotenv() behavior: invalid env-var keys still throw
  • Add serializeDotenvSkippingInvalid() for callers that intentionally want to drop unrepresentable keys

Why

Both flows pull and local flow runtime deps need the same dotenv parser/serializer. Keeping this in core avoids cross-domain imports and lets later PRs build on a neutral pure utility.

Tests

  • bun run lint
  • bun run typecheck
  • bun run test src/core/dotenv.test.ts src/commands/flows/runDefaults.test.ts src/domains/flows/pull/envVars.test.ts

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2133c552-0f68-41ba-910e-17f00a2eda38

📥 Commits

Reviewing files that changed from the base of the PR and between d7181e6 and e229206.

📒 Files selected for processing (8)
  • .changeset/pull-skip-invalid-dotenv-keys.md
  • src/core/messages/flows.ts
  • src/domains/flows/pull/envVars.test.ts
  • src/domains/flows/pull/envVars.ts
  • src/domains/flows/pull/handler.test.ts
  • src/domains/flows/pull/handler.ts
  • src/domains/flows/pull/stage.test.ts
  • src/domains/flows/pull/stage.ts

Walkthrough

The PR adds graceful handling of invalid environment variable keys during flows pull. src/core/dotenv.ts gains an internal isDotenvKey predicate and a new exported serializeDotenvSkippingInvalid function that partitions variables into valid and invalid keys, serializes only the valid ones, and returns both the dotenv content string and a skippedKeys array. serializeDotenv is refactored to use isDotenvKey without behavioral change.

writeEnvFile in src/domains/flows/pull/envVars.ts switches from serializeDotenv to serializeDotenvSkippingInvalid, changes its return type to Promise<{ skippedKeys: readonly string[] }>, and only writes .env when the serialized content is non-empty. The skipped keys are then threaded upward through stageBundle (which gains a skippedEnvVarKeys field and adjusts envVarCount to exclude them) and handleFlowsPull (which emits a UI warning and includes skippedEnvVarKeys in JSON output). Dotenv imports are consolidated from ~/domains/flows/dotenv.js to ~/core/dotenv.js, and a new message formatter and changeset document the feature.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: moving dotenv helpers to core. It uses imperative mood, is concise (43 characters), follows conventional-commit format, and directly reflects the primary refactoring objective.
Description check ✅ Passed The pull request description includes all required sections: summary of changes, rationale (why), and concrete test commands. However, the description lacks the optional linked issues line and formal checklist structure from the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/domains/flows/pull/envVars.ts`:
- Line 6: The import statement is currently using the strict `serializeDotenv`
function which throws errors on non-POSIX keys and aborts .env file generation.
Replace this import with `serializeDotenvSkippingInvalid` instead, then locate
all usages of `serializeDotenv` in this file and switch them to use
`serializeDotenvSkippingInvalid` so that invalid keys are skipped rather than
causing failures. Make sure the returned `content` from the skip-invalid
serializer is properly written to the .env file.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8f75e6e4-0e2b-4d0c-8331-a639d20dc19b

📥 Commits

Reviewing files that changed from the base of the PR and between adfc74e and d7181e6.

📒 Files selected for processing (4)
  • src/commands/flows/loadEnvFile.ts
  • src/core/dotenv.test.ts
  • src/core/dotenv.ts
  • src/domains/flows/pull/envVars.ts

Comment thread src/domains/flows/pull/envVars.ts Outdated
fix(flows): skip invalid dotenv keys on pull
@MateusZitelli Mateus Zitelli (MateusZitelli) merged commit a7544cf into main Jun 16, 2026
2 checks passed
@MateusZitelli Mateus Zitelli (MateusZitelli) deleted the wiz-dotenv-core branch June 16, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants