Skip to content

fix(varlock): record missing dataType as a ResolutionError - #1069

Closed
hassaans wants to merge 1 commit into
dmno-dev:mainfrom
hassaans:fix/unprocessed-item-datatype-assertion
Closed

fix(varlock): record missing dataType as a ResolutionError#1069
hassaans wants to merge 1 commit into
dmno-dev:mainfrom
hassaans:fix/unprocessed-item-datatype-assertion

Conversation

@hassaans

@hassaans hassaans commented Sep 5, 2026

Copy link
Copy Markdown

Summary

ConfigItem.resolve throws Error: expected dataType to be set when finishLoad() returns early (any invalid source, or a plugin loadingError) and the item is then resolved because its key is also in process.env / overrideValues.

EnvGraph.resolveEnvValues() does not await resolveItem() and uses a dead _reject, so that throw becomes an unhandledRejection while the promise the caller awaits hangs forever.

This is the crash hq-cli users hit as Sentry 7687037996 (and the same surface as #629 / #720 / #758). No released version removes the assertion.

Change

Record a ResolutionError on the item and return, matching every other resolve failure. resolveEnvValues() can then settle; callers see item.errors instead of a floated assertion.

Test

packages/varlock/src/env-graph/test/unprocessed-item-resolve.test.ts — valid .env.schema + malformed .env.local + override. Without the fix, resolveEnvValues() hangs and the assertion is unhandled. With it, the promise settles and the item carries a ResolutionError.

Does not change successful resolution.

Related: https://github.com/indigoai-us/hq-cli (consumer workaround while this lands).

ConfigItem.resolve threw `expected dataType to be set` when finishLoad()
bailed early (invalid source or plugin load failure) and the item was
then resolved via a process.env override. resolveEnvValues() does not
await resolveItem() and uses a dead _reject, so that throw became an
unhandledRejection while the returned promise hung forever.

Record a ResolutionError on the item instead so callers can fail
cleanly. Reproduces the crash hq-cli saw as Sentry 7687037996.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The new regression test fails repository lint because its Promise executor returns a timer handle.

Reviewed changes Reviewed the missing data type resolution fallback and its regression coverage.

  • Resolution fallback: ConfigItem.resolve() now records a ResolutionError when loading ended before an item received a data type, allowing resolveEnvValues() to settle instead of hanging on an unhandled assertion.
  • Regression coverage: The new test reproduces an invalid sibling source plus override and verifies settled resolution and item error reporting.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using azure/gpt-5.6-sol𝕏

);
const result = await Promise.race([
settled,
new Promise<'STILL_PENDING'>((r) => setTimeout(() => r('STILL_PENDING'), 200)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changed-file ESLint fails here because this expression-bodied Promise executor returns the Timeout from setTimeout, violating no-promise-executor-return; this will make lint CI fail. Wrap the executor body in braces and call setTimeout without returning it.

Suggested change
new Promise<'STILL_PENDING'>((r) => setTimeout(() => r('STILL_PENDING'), 200)),
new Promise<'STILL_PENDING'>((r) => {
setTimeout(() => r('STILL_PENDING'), 200);
}),

@theoephraim

Copy link
Copy Markdown
Member

Thanks for this. Will get it sorted asap.

@theoephraim

Copy link
Copy Markdown
Member

Thanks for the report and the repro, that was a good catch and the diagnosis is exactly right.

We went a different direction on the fix though, in #1072.

expected dataType to be set is an internal invariant guard rather than a user-facing error. It's only reachable because we resolved a graph we already knew had failed to load: finishLoad() bails before typing any items when a source fails to parse or a plugin fails to install. Turning it into a ResolutionError would make the crash survivable but leave the user staring at a message about an item's type, when the real problem is a parse error in a file we could point at with a line number.

So #1072 catches it a level earlier instead:

  • checkForSchemaErrors() now runs before resolution in the places that were missing it (the exported load(), plus encrypt --file, keychain import, and scan). Most CLI commands already did this, which is why the crash never showed up through varlock load or varlock run. You get the parse error with file and line instead of a crash.
  • resolveEnvValues() refuses to run at all on a graph whose load bailed, so a call site that forgets fails immediately with something actionable.
  • The other half of your diagnosis, the dead _reject, is fixed. Item resolution is fire-and-forget, so any throw became an unhandledRejection and left the returned promise permanently pending. That was a real bug on its own and is the reason this presented as a hang rather than an error.
  • The assertion itself stays a plain Error.

Two questions, if you don't mind:

How are you using varlock in hq-cli? I'd like to make sure the fix actually covers your path. The crash reaches ConfigItem.resolve() the same way regardless, but knowing whether you're calling the exported load(), going through internal.loadVarlockEnvGraph() yourself, or something else tells us where to put the guard rails. #1072 also adds checkForSchemaErrors to the internal export, since that surface previously handed you loadVarlockEnvGraph and checkForConfigErrors but not the check that has to run between them.

Heads up that load() may be deprecated. It isn't documented anywhere and predates the current integration story. If that's what you're on, varlock run or import 'varlock/auto-load' are the supported paths, and we'd want to make sure whatever you need from load() is covered before removing it.

Closing this in favor of #1072, but the fix is yours in substance. Thanks again.

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