Skip to content

refactor: unified error handling with severity levels - #708

Merged
theoephraim merged 21 commits into
mainfrom
refactor/unified-error-handling
May 22, 2026
Merged

theoephraim merged 21 commits into
mainfrom
refactor/unified-error-handling

Conversation

@theoephraim

Copy link
Copy Markdown
Member

Summary

  • Add ErrorSeverity type ('warning' | 'error' | 'fatal') to VarlockError, replacing the binary isWarning boolean
  • Add LoadingError class for data source loading failures (replaces plain Error)
  • Unify error storage: single _errors: VarlockError[] array on DataSource, DecoratorInstance, and Resolver (replaces separate _loadingError, _schemaErrors, _executionError fields)
  • Root decorator execution errors now use ResolutionError with severity: 'fatal' instead of being wrapped as SchemaError
  • Replace FatalSchemaError with CliExitError({ silent: true }) — reuses existing CLI error class
  • Simplify test helpers: merge earlyError/loadingError into single expectError flag with optional error type assertion

CLI output improvements

  • Group warnings and errors per file under a single header
  • Show full clickable file path below the header
  • Add "Resolved config:" header to separate schema output from item list
  • Fix item icon preferring first non-warning error
  • Fix warning-only decorators (@todo, @see) crashing during execution phase
  • Errors shown in red with ❌ emoji, warnings with ⚠️
  • Remove separate "Invalid items" / "Items with warnings" sections — show all problem items together (errors first)

Non-breaking

  • isWarning constructor option still works (deprecated, maps to severity: 'warning')
  • toJSON() now includes severity field (additive)
  • No changes needed for plugin authors or integrations

@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch 2 times, most recently from 4589d86 to 981a005 Compare May 21, 2026 00:23
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch 11 times, most recently from 1ca2a00 to 74ea224 Compare May 21, 2026 05:25
@theoephraim
theoephraim requested a review from philmillman May 21, 2026 06:52

@philmillman philmillman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good to me once the bun audit stuff is merged, and the framework tests pass.

should the vscode stuff be in this PR or a follow up?

gracefulExit() is async and doesn't stop execution, so the CLI would
continue to print item output after encountering fatal errors. Replace
with a new FatalSchemaError that is thrown and caught by the CLI error
handler, properly halting execution.
…I output

- Add ErrorSeverity type ('warning' | 'error' | 'fatal') to VarlockError
- Add LoadingError class for data source loading failures (replaces plain Error)
- Unify error storage: single `_errors` array on DataSource, DecoratorInstance, and Resolver
- Root decorator execution errors use ResolutionError with severity 'fatal'
- Registry conflicts (duplicate decorators, resolvers, etc) use SchemaError
- Replace FatalSchemaError with CliExitError({ silent: true })
- Improve CLI error output: group warnings/errors per file, show full clickable paths
- Fix item icon preferring first non-warning error over warning
- Fix warning-only decorators crashing during execution phase
- Add "Resolved config:" header to separate schema output from item list
Parser: handle decorator-like comments gracefully instead of dying —
@todo, @see, etc. now parse as decorators with warnings/invalid names
instead of being silently treated as plain comments or causing parse errors.

NextJS: improve next-env-compat handling.
… dev mode

Pipe stderr so the user sees varlock's pretty error output, pass through
partial JSON so the vite plugin can set up file watchers for auto-reload,
and fall back to empty vars so the dev server can still start.
- Vite plugin: set __VARLOCK_ENV even on failure so varlock/env can
  initialize with partial data instead of throwing "not initialized"
- ENV proxy: distinguish between "config has errors" and "not set up"
  with actionable error messages
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch from a435c11 to b1bf9db Compare May 21, 2026 17:24
@github-actions

github-actions Bot commented May 21, 2026 •

Copy link
Copy Markdown
Contributor

bumpy-frog

The changes in this PR will be included in the next version bump.

minor Minor releases

  • varlock 1.2.0 → 1.3.0

patch Patch releases

  • @env-spec/parser 0.3.1 → 0.3.2
  • @varlock/astro-integration 1.0.2 → 1.0.3 (cascade)
  • @varlock/cloudflare-integration 1.1.2 → 1.1.3
  • @varlock/nextjs-integration 1.1.0 → 1.1.1
  • @varlock/vite-integration 1.1.1 → 1.1.2

Bump files in this PR

Click here if you want to add another bump file to this PR


This comment is maintained by bumpy.

Test schema errors (unknown root decorator) and validation errors
(invalid decorator name, missing required value) to verify:
- Non-zero exit codes on errors
- Error messages appear in output
- File paths shown in error output
- json-full format still outputs JSON with error details
- "Configuration is currently invalid" banner for validation errors
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch from b1bf9db to 2f68983 Compare May 21, 2026 17:26
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch from 3f1c97f to 691f0e2 Compare May 21, 2026 20:23
Instead of silently returning undefined for all keys when config is
invalid, the ENV proxy now throws a clear error message telling the
user to fix their config. The configHasErrors flag is set during
initVarlockEnv based on the serialized graph's errors field (which
only includes real errors, not warnings).
…verride leak

- ENV proxy: warn (not throw) when config has errors — logs on every access
  so it's visible in server output without crashing the app
- Next.js: filter updateInitialEnv to prevent varlock-injected process.env
  keys from being treated as overrides on reload
- Next.js: reset process.env on config error so stale values don't persist
- Vite: fix config hook reload logic, dynamic error page middleware
- Add Next.js framework test for invalid config in dev mode
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch from 765ea93 to f98cb4b Compare May 21, 2026 21:53
Instead of exiting on initial load failure, parse the partial JSON
output to get source file paths, set up file watchers, and start
wrangler with an empty env file. When the user fixes the config and
saves, the watcher triggers a reload and restarts wrangler with the
resolved values.
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch from f98cb4b to 1243bc2 Compare May 21, 2026 22:00
Comment-like decorators (@see, @todo) and invalid names (@sensitive,)
should only get their own warning/error, not an additional "cannot be
used twice" error. Item decorators skip the dupe check if process()
already recorded errors; root decorators skip it for invalid names
or names not in the registry.
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch from e35da96 to aac62f3 Compare May 21, 2026 22:47
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch 11 times, most recently from ff169d1 to 1b7b437 Compare May 22, 2026 17:50
- command-runner: resolve (not reject) on timeout, so output assertions
  still run when the process is killed
- cloudflare: use timeout-based approach since varlock-wrangler dev stays
  alive with invalid config
- nextjs: use unique port per version to avoid EADDRINUSE in parallel CI
@theoephraim
theoephraim force-pushed the refactor/unified-error-handling branch from 1b7b437 to d57cda4 Compare May 22, 2026 17:59
- Use --port 0 (OS-assigned) for all cloudflare dev server tests to
  eliminate hardcoded port collisions
- Kill entire process group (detached + negative PID) in command-runner
  so wrangler/workerd children don't survive and block the close event
- Track current URL across dev server restarts so requests go to the
  right port after a restart
@theoephraim
theoephraim merged commit a1e6a2a into main May 22, 2026
29 of 30 checks passed
theoephraim added a commit that referenced this pull request May 22, 2026
…ss both change

Previously, harness file changes triggered an early exit that ran all
integrations in quick mode, even when an integration's own package also
changed. This caused the nextjs v15 dev server test to be skipped in
PR #708 where both the harness and @varlock/nextjs-integration changed.
theoephraim added a commit that referenced this pull request May 22, 2026
* fix: include plugin loading errors in DataSource.errors getter

The unified error handling refactor (#708) moved error storage to
_errors arrays but the DataSource.errors getter didn't include plugin
loading errors (stored on Plugin objects, not in _errors). This caused
checkForSchemaErrors to miss plugin load failures, so legacy plugins
showed a generic validation error instead of the migration message.

* chore: add bump file for plugin errors getter fix

* fix: isolate framework test fixtures from root bunfig.toml

Test fixtures inherit the repo root's bunfig.toml which sets
minimumReleaseAge=86400. This blocks installs of recently published
packages (e.g. vite@8.0.14), causing bun to hang and the CI job to
fail on timeout. Write a local bunfig.toml in each fixture to zero
out the age gate while keeping the security scanner active.

* fix: use single quotes in stderr smoke test to fix Windows shell escaping

On Windows with shell=true, the double quotes inside the node -e argument
get consumed by cmd.exe, causing a syntax error. Single quotes work in
both JS and cmd.exe.

* fix: preserve initial env vars in nextjs dev error path

When loadEnvConfig fails in dev mode (e.g., invalid schema), return
combinedEnv with initialEnv instead of empty object. Next.js v15's
turbopack exits cleanly when combinedEnv is empty (missing NODE_ENV
etc.), preventing the dev server from staying alive for hot-reload.

* chore: add bump file for nextjs dev error env fix

* fix: run full framework test suite when integration package and harness both change

Previously, harness file changes triggered an early exit that ran all
integrations in quick mode, even when an integration's own package also
changed. This caused the nextjs v15 dev server test to be skipped in
PR #708 where both the harness and @varlock/nextjs-integration changed.

* fix: set NODE_ENV=development for dev server framework tests

Vitest sets NODE_ENV=test which gets inherited by spawned dev servers.
Next.js v15's turbopack exits cleanly (code 0) when NODE_ENV=test
combined with invalid config. Override to development since dev server
tests should run in development mode regardless of the test runner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants