Skip to content

env: read varlock's own resolution sentinel instead of nub's wrap marker - #724

Open
colinhacks wants to merge 1 commit into
mainfrom
varlock-sentinel
Open

env: read varlock's own resolution sentinel instead of nub's wrap marker#724
colinhacks wants to merge 1 commit into
mainfrom
varlock-sentinel

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

A project that already wires Varlock into its own scripts got a second, Nub-inserted resolution in front of the one it asked for. That second resolution carries Nub's arguments rather than the script's.

Measured against Varlock 1.16.1, with the loader's bin instrumented to log every invocation:

script npm Nub before Nub after
varlock run --path ./config -- node … exit 0 exit 1, 2 resolutions exit 0, 1 resolution
varlock run --filter=PLAIN -- node … 0 exec() resolvers fired 1 fired, 2 resolutions 0 fired, 1 resolution
varlock run -- nub app.js 2 resolutions 1 resolution

The first row is the sharp one: Nub's inserted varlock run --path <project-root> resolved the root schema, failed validation, and killed the run before the script's own --path ./config invocation ever executed.

What changed

Two rules replace __NUB_ENV_OWNER_WRAPPED, which is deleted.

The loader already ran. __VARLOCK_ENV carries an absolute basePath and a typed sources entry per schema file, so Nub can ask whether this schema was resolved rather than whether the loader ran at all. Reading the loader's own surface instead of a marker Nub mints covers launchers Nub cannot observe — a Makefile, a CI wrapper, a standalone binary.

The test is containment against the schema directory Nub found, plus a scan of the blob's schema sources:

chain basePath Nub found result
varlock run -- nub app.js /repo /repo equal — stand down
varlock run --path ./config -- … /repo/config /repo below — stand down
workspace-root run, member ships its own schema /repo /repo/pkgs/web above — resolve
project A's run, Nub in project B A's dir B's dir unrelated — resolve
root schema @imports the member's /repo /repo/pkgs/web listed in sources — stand down

The third and fourth rows are what the 2026-08-02 decision record demanded of any such comparison, and they hold.

The loader is what Nub is about to launch. No blob exists at that moment, so this rule recognizes the program instead. It also replaces the marker as the recursion guard — structurally rather than by flag — which matters because the loader's bin is a #!/usr/bin/env node script whose interpreter re-enters Nub through the PATH shim.

On the earlier decision

The 2026-08-02 record rejected reading __VARLOCK_ENV on three grounds. Two do not reproduce on 1.16.1:

  • An encrypted blob is not opaque. With encryptInjectedEnv confirmed on, the envelope stayed plain JSON and basePath read fine.
  • A --path flag does move basePath.
  • A --filter does leave basePath unchanged with a subset injected. That is now honored deliberately: it is an explicit user flag, and the same principle already makes an explicit --env-file beat Nub's auto-discovery.

An unreadable blob degrades toward resolving, never toward an empty environment.

Verification

  • 14 unit and 20 integration tests pass, including 6 new unit tests and one end-to-end regression test.
  • Both rules were verified by disabling each and confirming the regression test goes red for the right reason.
  • The stub loader in the test harness now publishes the blob a real loader does, which is what makes the end-to-end resolution count meaningful.
  • Every row of the table above was reproduced against real Varlock, not the stub.

Not addressed

The per-Node-process cost is unchanged: three nested nub run calls still boot Varlock three times, because each leaf resolves independently and no sibling inherits the result. Reaching one resolution per chain needs the resolution to happen at the script launcher, which is a separate decision with a security-posture trade-off. There is also still no way to turn the hand-over off short of --node.

Reported by Varlock's author.

A project that already wires varlock into its own scripts got a second,
nub-inserted resolution in front of the one it asked for. That second
resolution carries nub's arguments rather than the script's, so it re-ran
`exec()` resolvers a `--filter` had excluded, and — where the script passed
its own `--path` — died on the root schema's validation before the script's
own invocation ever ran. Measured against varlock 1.16.1: `npm run` exited 0
on the same project, nub exited 1.

Two rules replace the `__NUB_ENV_OWNER_WRAPPED` marker, which is deleted:

- The loader already ran. `__VARLOCK_ENV` carries an absolute `basePath` and
  a typed `sources` entry per schema file, so nub can ask whether THIS schema
  was resolved rather than whether the loader ran at all. Containment against
  the schema directory keeps the case the marker existed for: a workspace
  root's resolution does not stand down a member that ships its own schema,
  and another project's says nothing about this one.
- The loader is what nub is about to launch. No blob exists at that moment,
  so this one recognizes the program instead. It also replaces the marker as
  the recursion guard, structurally rather than by flag.

Reading the loader's own surface covers launchers nub cannot observe — a
Makefile, a CI wrapper, a standalone binary. Two of the three objections the
2026-08-02 decision raised against it do not reproduce on 1.16.1: an
encrypted blob keeps a plain-JSON envelope, and `--path` does move
`basePath`. An outer `--filter` is now honored deliberately.

The stub loader in the tests now publishes the blob a real loader does, which
is what makes the end-to-end count meaningful. Both rules verified by
disabling each and watching the new regression test go red.
Copilot AI lite review requested due to automatic review settings August 12, 2026 16:07
@vercel

vercel Bot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Aug 12, 2026 4:08pm

Request Review

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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

Two things worth settling before merge: launches_loader matches on arguments as well as the program, which can hand a program an environment nothing resolved; and the "encryption keeps the envelope plain JSON" premise holds only for Varlock's default --inject mode.

Reviewed changes — the full diff for 2cdd8d0d, plus the surrounding env-owner call sites in cli.rs, spawn_node's loader command, and Varlock 1.16.1's published dist to check the parsed contract.

  • Stand down on Varlock's own blob. already_resolved_for replaces wrapped_for: containment of basePath against the schema directory nub found, plus a scan of the blob's type: "schema" sources for the @import case. Above / unrelated / unparseable all still wrap.
  • Recognize the loader as the program being launched. New EnvOwner::launches_loader and in_loader_package take over as the recursion guard, and run_file_in_dir filters both the check_schema_usable diagnostic and SpawnConfig::env_owner through it.
  • __NUB_ENV_OWNER_WRAPPED deleted. WRAPPED_ENV and wrapped_marker are gone along with both stamp sites (the file run and run_watch); the field is renamed already_resolved, and suppresses_env_files() stays ungated on the new filter.
  • Tests. Six unit tests over the new rules, the stub loader now publishes the blob a real loader does, and a #[cfg(unix)] end-to-end test asserting exactly one loader invocation for a script that runs Varlock itself.
  • Docs. The Varlock page states that hand-written invocations run as written; both wiki records carry the reversal and a measured-shape table.

Two premises checked independently and confirmed, so they need no further work: Varlock 1.16.1's getSerializedGraph really does emit basePath absolute and sources[].path as path.relative(basePath, fullPath) with the literal type: "schema", including a separate entry per @import; and the chain stays bounded when the blob is unreadable, because every nub-inserted wrap hands the loader an absolute Node path (spawn.rs:785, cli.rs:6313) so the loader's terminal spawn never re-enters the PATH shim.

ℹ️ Nitpicks

  • crates/nub-cli/src/cli.rs:5032-5036 still says what must flow from the run path is "the env-owner MARKERS and preload tokens". The marker is now deleted, so the term names nothing; inheritance of __VARLOCK_ENV is what carries the fact today.
  • wiki/research/varlock-integration.md (~line 220, inside the 2026-08-01 section) still reads "Nub should set and check its own neutral marker … treating __VARLOCK_ENV as one recognized signal among others rather than the contract." The changelog records the reversal, but that line is now the opposite of the decision and carries no superseded note.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

/// `--require` of something inside it — does not lose its wrap.
pub(crate) fn launches_loader(&self, args: &[String]) -> bool {
let cli = self.cli.as_deref().map(canonical);
args.iter().filter(|arg| !arg.starts_with('-')).any(|arg| {

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.

This scans every non-flag argument, not the program position, so a command whose arguments merely reference a path under node_modules/varlock/ also stands nub down — nub scripts/check-version.js node_modules/varlock/package.json is enough. On that path nub drops the wrap and the diagnostic, while suppresses_env_files() stays true because it is deliberately not gated on this filter, so the program runs with neither a resolution nor .env* and nothing is printed.

Technical details
# `launches_loader` matches arguments, not just the program

## Affected sites
- `crates/nub-cli/src/env_owner.rs:394-400``.any()` over all non-flag args; `in_loader_package` matches any `node_modules/varlock/` component, so a data argument counts.
- `crates/nub-cli/src/cli.rs:3905-3910` — a true result filters the owner to `None`, skipping `check_schema_usable` (both the explicit-`--env-file` conflict and `SchemaProblem`).
- `crates/nub-cli/src/cli.rs:3960-3963` — and drops the loader from `SpawnConfig::env_owner`.
- `crates/nub-cli/src/cli.rs:3719` + `3724-3731``runtime_child_env` gets the UNfiltered owner, `suppresses_env_files()` is `cli.is_some() || already_resolved`, so the `.env*` cascade is off. Net: the silent unresolved environment that `SchemaProblem` exists to refuse.
- `crates/nub-cli/src/env_owner.rs:812-820` — the negative assertions only cover an entry followed by a *flag* (`["/app/src/index.js", "--path"]`) and a `--require=` flag, so an entry followed by a non-flag argument inside the package is untested.

## Required outcome
- Only the program nub is about to launch decides this, not the arguments passed to it.
- A test where an ordinary entry point is followed by a non-flag argument under `node_modules/varlock/` and the wrap is retained.

## Suggested approach
- Narrow to the first non-flag argument. The real recursion channel always delivers the loader's bin in that position: a `#!/usr/bin/env node` shebang hands the interpreter exactly `node <script> <args…>`, so no node flags can precede it, and the `--enable-source-maps` prefix in the unit test is hypothetical rather than a shape nub receives.

/// ```
///
/// `basePath` and `sources` stay plain JSON even when the loader encrypts the
/// injected values, which covers the envelope's contents and not the envelope.

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.

This reads as unconditional, but the plain-JSON envelope is a property of the default --inject all mode, not of encryption. In buildInjectedBlobEnv, plain JSON is returned on the injectVars branch; with --inject blob and @encryptInjectedEnv the whole envelope becomes varlock:v1:<base64>, so nub falls through to wrapping and the --path / --filter clobbering this PR fixes still reproduces for that configuration.

Technical details
# The encrypted-envelope premise is scoped to `--inject all`

Verified against the published `varlock@1.16.1` dist (`https://unpkg.com/varlock@1.16.1/dist/chunk-OWQP2FL6.js`):

```js
function buildInjectedBlobEnv(opts) {
  if (!opts.injectBlob) return {};
  const json = JSON.stringify(opts.serializedGraph);
  if (opts.injectVars) return { __VARLOCK_ENV: json };
  if (opts.serializedGraph.settings?.encryptInjectedEnv) {
    const key = opts.ambientEnvKey ?? generateEncryptionKeyHex();
    return { __VARLOCK_ENV: encryptEnvBlobSync(json, key), _VARLOCK_ENV_KEY: key };
  }
  ...
```

`resolveInjectMode` in the same chunk sets `injectVars` for `all` and `vars` only, and defaults to `all` — which is why the 2026-08-12 measurement saw plain JSON. `--inject blob` is a documented flag.

## Affected sites
- `crates/nub-cli/src/env_owner.rs:78-81` — "stay plain JSON even when the loader encrypts the injected values" holds only for the default inject mode.
- `wiki/research/varlock-integration.md` — the `Encryption hides it | No` row and the reversal table's "The opaque form did not reproduce at all" are load-bearing for the decision this PR reverses, and are unqualified.

## Required outcome
- Both claims scoped to the inject mode they were measured under.
- `--inject blob` + `@encryptInjectedEnv` recorded as a residual case that still double-resolves, alongside the per-Node-process cost already listed.

## Open questions for the human
- The in-process `auto-load` integration builds `__VARLOCK_ENV` from the same JSON and appears to have no `injectVars` bypass, which would make an encrypted envelope reachable there too. Worth confirming before wording the residual, since that path also feeds every child by inheritance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants