fix(varlock): report load errors before resolving values - #1072
Conversation
finishLoad() bails before processing config items when a source failed to load/parse or a plugin failed to install, which leaves every item untyped. Resolving that graph hit the `expected dataType to be set` invariant deep in ConfigItem.resolve() - and since resolveEnvValues() starts each item without awaiting it and never used its reject(), the throw escaped as an unhandledRejection while the caller's promise hung forever. The assertion is a real invariant, so it stays. Instead: - `varlock`'s exported load(), plus the encrypt/keychain/scan commands, now run checkForSchemaErrors() before resolving, like the other CLI commands already do. The user sees the parse error that actually caused the failure. - resolveEnvValues() guards on a new EnvGraph.configItemsProcessed flag, so a call site that forgets fails immediately with an actionable message. - resolveItem() rejections are routed to reject(), so any future unexpected throw settles the promise instead of hanging.
|
The changes in this PR will be included in the next version bump.
|
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The failed-load fix is sound, but the exported load() also gains an unrelated breaking behavior for empty configurations.
Reviewed changes in 4ce89c30: failed graph guards, promise rejection propagation, CLI and library caller ordering, and regression coverage.
- Failed-load guard: Tracks whether config items were processed and prevents resolution of a graph left half-built by a source or plugin load failure.
- Promise settlement: Routes unexpected item-resolution throws to the promise returned by
resolveEnvValues(). - Caller ordering: Checks schema errors before resolution in
encrypt --file,keychain import,scan, and package-rootload(). - Regression coverage: Exercises failed graph resolution and unexpected asynchronous resolution throws.
azure/gpt-5.6-sol ｜ 𝕏
| // graph half-built, and resolving it produces confusing downstream errors rather than the | ||
| // parse error that actually caused them (same order the CLI commands use) | ||
| checkForSchemaErrors(envGraph); | ||
| checkForNoEnvFiles(envGraph); |
There was a problem hiding this comment.
checkForNoEnvFiles() is unrelated to malformed-load handling and changes the exported programmatic load() from a successful empty initialization into CLI-formatted stderr output followed by a rejected CliExitError. Please remove this check, or explicitly make strict non-empty validation opt-in, since an empty graph already completes finishLoad() and safely resolves no keys.
There was a problem hiding this comment.
Leaving checkForNoEnvFiles() in. The exported load() is undocumented and has no callers anywhere (the load() hits in the docs are all the codegen'd Java/Rust/PHP Env.load(), unrelated), so we're treating it as internal and matching CLI behavior rather than preserving a silent empty-graph success nobody depends on.
The comment did point at something worth fixing though: the internal export offers loadVarlockEnvGraph and checkForConfigErrors but not checkForSchemaErrors, so a consumer following that surface writes exactly the sequence this PR fixes. Added it in e2e1d00.
@env-spec/parser
varlock
@varlock/native-helper-darwin
@varlock/native-helper-linux-arm64
@varlock/native-helper-linux-x64
@varlock/native-helper-win32-x64
@varlock/aws-sigv4-plugin
commit: |
The internal surface hands consumers loadVarlockEnvGraph and checkForConfigErrors but not checkForSchemaErrors, so following it leads straight to the sequence this PR fixes - resolving a graph that failed to load, with only a check that never reports source-level parse errors.
There was a problem hiding this comment.
ℹ️ No new issues in the incremental change. The existing review thread remains open.
Reviewed changes since the prior Pullfrog review: the internal API was extended to expose the required pre-resolution schema check.
- Internal validation API: Exported
checkForSchemaErrorsalongsideloadVarlockEnvGraphandcheckForConfigErrors, so low-level consumers can surface failed-load errors before callingresolveEnvValues().
azure/gpt-5.6-sol ｜ 𝕏
1 participant
Alternative to #1069, same crash, fixed a level up.
What happens
finishLoad()bails before processing config items when any source failed to load/parse or a plugin failed to install. That leaves every item untyped, so resolving the graph hitsexpected dataType to be setinConfigItem.resolve(). BecauseresolveEnvValues()starts each item without awaiting it and never used itsreject(), the throw escaped as anunhandledRejectionwhile the promise the caller awaited hung forever.That assertion is a real invariant guard, not a user-facing error, so it stays as a plain
Error. The bug is that we resolve a graph we already know failed to load.Reproduced through the public API on
main:Why callers hit it
Most CLI commands (
load,run,explain,reveal,proxy,plugin) already callcheckForSchemaErrors()before resolving, which is why the crash never showed up there. Four call sites didn't:varlock's exportedload()resolved first and only rancheckForConfigErrors()afterwards, which never reports source-level parse errorsencrypt --file,keychain import, andscanChange
checkForSchemaErrors()before resolving. The user gets the parse error that actually caused the failure, with file/line, instead of a crash.EnvGraph.configItemsProcessedrecords whetherfinishLoad()got as far as typing items;resolveEnvValues()throws immediately if it didn't, so a call site that forgets fails with an actionable message rather than somewhere deep in coercion.resolveItem()rejections are routed to the deferred'sreject(), so an unexpected throw settles the promise instead of hanging the caller.scan's ad-hocloadingErrorloop is replaced bycheckForSchemaErrors(), which covers the same conditionsfinishLoad()bails on and prints the standard output.Tests
packages/varlock/src/env-graph/test/failed-load-resolve.test.tscovers a failed load (rejects, no floating rejection) and an unexpected throw mid-resolution (rejects rather than hanging). Verified both fail without the fix.Closes #1069. Thanks @hassaans for the diagnosis and repro.