fix(config): fail loud on unresolved env vars - #9045
Conversation
for more information, see https://pre-commit.ci
Rename the release note to the required bugfix category and update the npmrc test, which asserted the literal passthrough that jaseci-labs#8773 removes.
…env-interpolation
MusabMahmoodh
left a comment
MusabMahmoodh
left a comment
There was a problem hiding this comment.
Right fix, and deleting the catch rather than special-casing :? is the right shape: the swallow was making ${VAR} mean two different things depending on whether anyone had thought about it. The two new tests drive real toml through the real loader and cover the overlay path, which is where I would have expected the gap.
One thing before merge and one thing that is not yours, both below. jac-check red here is the runtime-cache flake, not your diff: the job dies in "Warm the runtime cache" on rmtree: could not remove .../rt/f4ca7e3e.../openai/types/conversations/__pycache__, before it reads a single file. A re-run clears it. Linus hit the same thing on #8175 yesterday, so I will file it separately rather than have people keep re-running.
…tion' into fix/8773-fail-loud-env-interpolation
|
Filed the |
MalithaPrabhashana
left a comment
MalithaPrabhashana
left a comment
There was a problem hiding this comment.
We filed #8773, so here is a validation pass from our side. Ran this on macOS
against pr-9045 (2eb371b) with main at 3ad7d91b5 as the baseline, both in
dev mode off the checkout so the config code under test is really yours.
The reported bug is fixed
Same project from the issue, jac scale deploy --dry-run --show-yaml, neither
variable exported.
main today, exit 0, and the literal ships into the manifests:
kind: Secret
metadata:
name: interp-repro-secrets
namespace: ${K8S_NAMESPACE:?set K8S_NAMESPACE}
stringData:
SOME_API_KEY: ${A_VAR_NOBODY_SET}(namespace: ${K8S_NAMESPACE:?set K8S_NAMESPACE} appears in 5 places.)
This branch, same command, exit 2:
✖ Error: jac.toml is invalid: scale.kubernetes.namespace: Environment variable K8S_NAMESPACE is required: set K8S_NAMESPACE
With both variables set it is exit 0 again and the real values land
(namespace: prod-ns, SOME_API_KEY: real-key-123), so the happy path is
untouched.
Also checked and fine: ${VAR:-default} unchanged, list elements are located
(project.authors[0]), deep tables get the full path
(scale.kubernetes.resources.limits), run / check / fmt / build /
clean / config show all give the same clean exit 2, and jac --help and
--version still work with a broken config.
Suites on this machine:
| suite | main | this branch |
|---|---|---|
tests/project/ |
320 passed, 3 failed | 321 passed, 3 failed |
tests/cli/ |
342 passed, 6 failed, 1 skipped | 342 passed, 6 failed, 1 skipped |
tests/client/test_cli.jac |
28 passed | 28 passed |
The 3 test_template.jac failures are already red on main, not from this PR.
I also reverted only the except hunk on your branch and re-ran
tests/project/test_config.jac: 129 passed with the fix, 128 passed + 1 failed
without it, so the new test really does pin the behaviour. The branch already
contains current main, nothing to merge.
Four things I ran into.
1. The profile overlay path still ends in a traceback
Put the value in jac.prod.toml instead of jac.toml and activate the profile:
$ JAC_PROFILE=prod jac run src/main.jac # BT_UNSET not exported
⚠ Failed to apply profile: scale.kubernetes.namespace: Environment variable BT_UNSET is required: you must set BT_UNSET for prod
✖ Error: Error executing 'run': scale.kubernetes.namespace: Environment variable BT_UNSET is required: you must set BT_UNSET for prod
Traceback (most recent call last):
...
File ".../jaclang/project/impl/plugin_config.impl.jac", line 109, in get_jac_config
config.apply_profile_overlay(profile or None);
...
ValueError: scale.kubernetes.namespace: Environment variable BT_UNSET is required: you must set BT_UNSET for prod
exit 1
Two spots miss it. _apply_profile (cli/impl/dispatch.impl.jac:129) catches
ValueError and only warns, and PluginConfigBase.get_jac_config
(project/impl/plugin_config.impl.jac:109) has no handler at all. The base file
is fine because _discover_project_config catches it and exits 2 cleanly.
Still better than main, which is exit 0 and RAN OK for the same run. But it
does not look like the rest of the feature. Worth noting your new overlay test
calls merge_from_toml_file directly, so it never goes through either handler
and stays green either way. This is the only one I would want looked at before
merge.
2. Only one missing variable is reported per run
The dict comprehension raises on the first bad key, so you fix them one at a
time. On jacBuilder's real jac.toml that is 47 runs before the config loads:
run 1 -> ECR_REGISTRY
run 2 -> K8S_APP_NAME
run 3 -> K8S_NAMESPACE
...
run 47 -> SHARED_DATA_PVC
Not a blocker, but collecting them and printing all at once would save a lot of
time on big configs. It also buries other errors: our file additionally has a
[scale.microservices] was removed error, and that message only appears after
all 47 variables are set.
3. Inline [profiles.*] is strict, jac.prod.toml is lazy
[profiles.prod.scale.kubernetes]
namespace = "${BT_UNSET:?prod only}"in jac.toml fails every command even when the prod profile is never used. The
same content in jac.prod.toml is only read when the profile is active, so it
does not fail. That asymmetry is probably fine, but it is worth a line in the
docs, because moving deploy-only variables into an overlay file is the easy
answer to the "you now have to export everything" note in your description.
4. The jac-client doc line is not quite right
It now suggests ${NODE_AUTH_TOKEN:-} to keep a registry entry optional. That
writes an empty token, not a pass-through. Measured through create_npmrc:
${NODE_AUTH_TOKEN} -> config rejected
${NODE_AUTH_TOKEN:-} -> //npm.pkg.github.com/:_authToken=
${NODE_AUTH_TOKEN:-${NODE_AUTH_TOKEN}} -> //npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}
Only the third keeps the old behaviour where npm resolves the token itself at
install time. An empty token is not optional, npm just gets a 401. Either
mention the third form, or say plainly that the leave-it-to-npm pattern is gone
and the variable has to be exported at build time.
Not covered
No real cluster, --dry-run only. macOS only. I did not test the LSP or
jac serve paths, though get_config() is unguarded in a lot of call sites so
1 may show up in more places than the profile one.
None of this changes the verdict for me. The fix does what #8773 asked for and I
am happy with it from our side.
…env-interpolation
…tion' into fix/8773-fail-loud-env-interpolation
|
Thanks for the validation pass, this was thorough. I reproduced all four on Linux and fixed 1, 3 and 4. Details below. 1. Profile overlay traceback — fixedReproduced exactly, including the double report and exit 1. Root cause is the one you identified: Rather than add a handler to def _apply_overlay_or_exit(config: any, profile: str | None) {
import from jaclang.project.tomlio { MalformedJacTomlError }
try {
config.apply_profile_overlay(profile);
} except MalformedJacTomlError as exc {
_exit_on_malformed_jac_toml(exc);
} except ValueError as exc {
_exit_on_invalid_jac_toml(exc);
}
}
No warning line, no traceback, identical to the base-file path. Verified for all three activation routes: One thing that fell out of writing it: You were right that the existing overlay test bypasses the handler. Added 4. The npmrc doc line — fixed, you are rightMy line was wrong. Measured through An empty token is a 401, not optional. The doc now names the third form as the way to keep npm resolving it at install time, and says plainly that 3. Inline
|
Greptile SummaryThis PR makes unresolved environment-variable references in
Confidence Score: 5/5The PR appears safe to merge; the previously reported discovery and nested-placeholder issues are resolved, and no new actionable failures remain. The current code consistently propagates unresolved interpolation errors with setting paths and converts them into clean CLI exits across initial discovery, profile overlays, and cross-project targets. Both previous threads are resolved, and the replacement integration test retains coverage of the relevant fatal paths.
|
| Filename | Overview |
|---|---|
| jac/jaclang/project/impl/config.impl.jac | Propagates unresolved-variable failures while adding dictionary and list paths to diagnostics. |
| jac/jaclang/cli/config_errors.jac | Centralizes malformed and invalid configuration reporting with a clean status-2 exit. |
| jac/jaclang/cli/commands/impl/execution.impl.jac | Makes invalid cross-project configuration discovery and profile overlays fatal while preserving warnings for lookup and filesystem failures. |
| jac/jaclang/cli/impl/dispatch.impl.jac | Reuses centralized configuration-error handling during initial discovery and profile application. |
| jac/tests/cli/test_run_project_scope.jac | Covers fatal base and overlay interpolation failures through the cross-project run path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CLI reads jac.toml] --> B[Recursively interpolate values]
B --> C{Variable resolves?}
C -->|Yes or default provided| D[Load configuration]
C -->|No| E[Attach setting path]
E --> F[CLI configuration-error handler]
F --> G[Print invalid jac.toml error]
G --> H[Exit with status 2]
D --> I{Active profile?}
I -->|Yes| J[Apply profile overlay]
J --> B
I -->|No| K[Continue command]
Reviews (4): Last reviewed commit: "test: fold config-load exit coverage int..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Both valid. Reproduced and fixed. Root cause is the same in both places I have now touched: "an unloadable def exit_on_config_error(exc: Exception) {
if isinstance(exc, MalformedJacTomlError) {
console.error(f"jac.toml is malformed: {exc.original}", hint=f"File: {exc.file_path}");
} else {
console.error(f"jac.toml is invalid: {exc}");
}
sys.`exit(2);
}
The two file-target handlers keep warning on Confirmed before: After: One nuance: Covered by a second case in Nested token fallbackYou are right, and it is worse than the report: the breakage is in the working case. With the variable set, That was my suggestion two commits ago and it was wrong, so I removed it rather than replace it with another workaround. Adding escape syntax to the interpolation parser is a real feature and does not belong in a bug fix, so the doc now states the situation plainly: the leave-it-to-npm pattern is gone, export Checks
|
|
Follow-up on P1: the main claim was fixed in 46b4680, but the second sentence of your comment ("active-profile overlays on this path can instead expose the same error without the clean invalid-config exit") was still true, and I had not covered it. Fixed in 5d9c261.
if config is not None {
if active_profile {
config.apply_profile_overlay(active_profile); // unguarded
}
add_venv_to_path(config);
}So a target project whose base file is fine but whose Grepping for the pattern found three unguarded overlay applications in def apply_overlay_or_exit(config: any, profile: str | None) {
try {
config.apply_profile_overlay(profile);
} except ValueError as exc {
exit_on_config_error(exc);
}
}The Same command now: Re-checked the whole P1 surface on the pushed commit: relative and absolute file targets,
|
4 participants
Summary
Fixes #8773.
jac.tomllets you write${VAR}in a value, and Jac swaps in an environment variable when it reads the file. Both${VAR}and${VAR:?message}are documented as errors when the variable is not set, and the function that does the swapping raised one correctly. But the function that walks the config caught that error and returned the raw text instead, so both forms did nothing at all. The literal${K8S_NAMESPACE:?set K8S_NAMESPACE}became the configured value and the command carried on.That is how a Kubernetes namespace ended up named after a shell expression while
--dry-runreported no errors.What changes
The walker no longer throws the error away. It also keeps track of where it is in the file, so the message names the setting rather than only the variable.
${VAR}jac.toml is invalid: <setting>: Environment variable VAR is not set${VAR:?message}jac.toml is invalid: <setting>: Environment variable VAR is required: message${VAR:-default}The reported case now stops instead of deploying to a literal namespace:
Array items are located too, for example
project.authors[0]: Environment variable JAC_MISSING_AUTHOR is not set. Every layer behaves the same way: the basejac.toml, profile overlays, andjac.local.toml.What this means for existing projects
This is the real cost of the change, so it is worth reading before the code.
The swap happens when the config is read, not when a value is used, and every command reads
jac.toml. A project with a deploy-only variable can therefore no longer runjac checkorjac fmtwithout exporting it, even though neither command touches that value.That is the correct behaviour, since
${VAR}is documented as required and honouring it in only some commands is what caused this bug in the first place. But people will hit it, so both ways out are now written down in the release note and the config reference:${VAR:-default}for values that only some commands need.The config reference also now explains a related trap: an inline
[profiles.<name>]table lives insidejac.toml, so its variables are resolved on every command even when that profile is never activated. A separatejac.<profile>.tomlfile is only read when the profile is active, which makes it the better home for deploy-only values.One config in this repo was affected.
[client.npm.auth]used_authToken = "${NODE_AUTH_TOKEN}"and relied on the placeholder reaching the generated.npmrcunexpanded, so npm could resolve it later at install time. That is no longer possible, and there is no syntax that restores it:${VAR:-}writes an empty token and npm answers 401, and a nested${VAR:-${VAR}}corrupts the value when the variable is set, because the pattern stops at the inner brace. Thejac-clientreference now says plainly that the variable has to be exported before the command that readsjac.toml, and that a genuinely optional registry should leave the table out rather than give it a blank value.How it is implemented
The fix itself is in
_interpolate_recursive. It stops catching the error, and carries a path string down as it walks so the failure can namescale.kubernetes.namespaceinstead of justK8S_NAMESPACE. The path lives in the walker rather than ininterpolate_env_vars, which stays a plain string function that knows nothing aboutjac.toml.Making the error fatal everywhere. Once the error could escape, it turned out several places caught it and carried on, so the run either continued on the wrong config or ended in a traceback: profile overlays applied during dispatch, and file targets that belong to a different project than the current directory. "an unloadable
jac.tomlstops the command" was being written out separately at each of those places, so it is now one small module,jaclang/cli/config_errors.jac, and every CLI discovery and overlay call site uses it. Two details it now gets right in one place instead of four:tomllib.TOMLDecodeErroris a subclass ofValueError, so a plainexcept ValueErrorwould report a malformed file as an invalid one. Malformed is checked first.ValueErroris fatal.OSErrorandKeyErrorduring discovery still warn and carry on, so a permissions problem does not stop a command.The
jaclang/project/call sites keep raising rather than exiting, which is the right behaviour for a library.Checked by hand
run,check,fmt,build,clean,config showandtestall exit 2 with no traceback on an unloadable config, and print the same message.--helpand--versionstill work when the config is broken. Malformed files still report as malformed, not invalid. Profile activation was checked throughJAC_PROFILE,--profileand[environment] default_profile, and file targets through relative paths, absolute paths,--no-takeover, and a target project whose base file is fine but whose overlay is not.One gap left alone:
jac check <file-in-another-project>reports the same error through its own per-file reporting (Error checking '...', exit 1) rather than these handlers. It is clear and has no traceback, socheck's reporting contract was not changed.Tests
mainjac/tests/project/jac/tests/cli/jac/tests/client/test_cli.jacTwo tests added, both table-driven, both extending files that already own the subject.
tests/project/test_config.jaccovers the config-loading path, which had none before:${VAR}in the base file,${VAR:?message}in the base file, and${VAR:?message}in an overlay, each asserting the setting name appears in the message.That test calls
merge_from_toml_filedirectly, so it never reaches the CLI handlers. Those are covered by a test intests/cli/test_run_project_scope.jac, whose subject is alreadyjac run <file>config discovery and which supplies the fixtures. It drives the realrun()in-process over an unresolvable base file, an unresolvable profile overlay, and a resolvable default. Reverting the discovery guards fails the first case and reverting the overlay guards errors on the second, so it pins both.tests/client/test_cli.jachad an assertion that locked in the old behaviour, with a comment saying so. It now setsNODE_AUTH_TOKENand checks the token is actually resolved.Known limitation
Only the first unresolvable variable is reported per run, so a config with many of them is fixed one at a time. Collecting them all needs an accumulator threaded through the walk, which brings back the catch-and-return-the-raw-value shape this PR exists to remove. Better done separately, with its own review.