import VAR silently loses its cached value across builds when imports are declared out of name order

[typeless]

Summary

The cached-env-var table is sorted by StringId handle and binary-searched by name — the same defect as the exported-variable bug filed alongside this one, but this one reaches command identity.

// src/cli/context.cpp:677 — default < on pair<StringId, StringId> compares the key by HANDLE
std::sort(result.cached_env_vars.begin(), result.cached_env_vars.end());

// src/graph/builder.cpp:1305-1311 — searched with a NAME comparator
[](auto const& p, std::string_view k) { return str(p.first) < k; }

include/pup/graph/builder.hpp:38 documents the contract as "sorted by key" — a comment-enforced invariant, which is the tell.

Handle order here is the order the $/KEY=VALUE entries appear in the loaded index (src/cli/context.cpp:663-675), which tracks the order the import directives were parsed.

Reproduced

Two projects, identical except for the order of three import lines. Rule is : |> echo "A=$(AVAR) M=$(MVAR) Z=$(ZVAR)" > %o |> out.txt. Build 1 with the variables set, build 2 with them unset — the documented cache-fallback path (src/graph/builder.cpp:1301-1304, "fall back to the cached value from the previous build so plain import VAR stays stable across shell sessions that forget to re-export the variable"):

imports A, M, Z:   build1 (env set) -> A=aaa M=mmm Z=zzz
                   build2 (unset)   -> A=aaa M=mmm Z=zzz     fallback works

imports Z, M, A:   build1 (env set) -> A=aaa M=mmm Z=zzz
                   build2 (unset)   -> A= M= Z=              fallback silently fails

Severity

Worse than the exported-variable case. The missed lookup falls through to an empty value, which changes the rendered command text, which feeds compute_command_identity (src/graph/dag.cpp:1040) — so this reaches the cross-build join key and the persisted index, not just one child process. The command re-runs and writes wrong content, with no diagnostic.

Fix

Sort with the key the lookup uses:

std::ranges::sort(result.cached_env_vars, {}, [](auto const& p) { return global_pool().get(p.first); });

Structurally: the vector and its lookup should share one projection defined once, or the type should be name-keyed, so a future producer cannot re-establish handle order. The ///< ... (sorted by key) comment at builder.hpp:38 is exactly the conventional invariant that the deleted-relational-operator change (filed separately) would turn into a compile error.

Provenance

Found by adversarial multi-agent review of PR #176, whose body wrongly cleared src/cli/context.cpp's sorts as "order-agnostic by construction". Pre-existing on main — not introduced by any open PR. Reproduced independently before filing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA

[typeless]

The general enforcement point for this class is #185 (delete StringId's relational operators) — this bug is one of the four instances it would catch at compile time.