Also exclude config-level PassUnsafeEnv from remote digest

The previous change only stripped per-target pass_unsafe_env from the cache-key
command. Values declared via the global [Build] PassUnsafeEnv config keyword
enter the build environment through config.GetBuildEnv() and were still left in
the canonical command, so changing them still caused remote cache misses.

Strip both config-level and target-level PassUnsafeEnv (keeping anything also
listed in PassEnv), and treat a non-empty config-level list as enough to enable
the cache-key split. This matches the local cache, which excludes both from its
hash.

Author: Andrzej J Skalski

src/remote/action.go

@@ -132,7 +132,7 @@ func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory,
 	cmd, err := core.ReplaceSequences(state, target, cmd)
 	env := c.stampedBuildEnvironment(state, target, inputRoot, stamp, isTest || isRun)
 	if canonical {
-		c.stripUnsafeEnv(target, env)
+		c.stripUnsafeEnv(state, target, env)
 	}
 	return &pb.Command{
 		Platform:             c.targetPlatformProperties(target),
@@ -142,29 +142,47 @@ func (c *Client) buildCommand(target *core.BuildTarget, inputRoot *pb.Directory,
 	}, err
 }
 
-// excludesUnsafeEnv reports whether PassUnsafeEnv values should be kept out of the action digest for this target.
-func (c *Client) excludesUnsafeEnv(target *core.BuildTarget) bool {
-	return c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest && target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0
+// excludesUnsafeEnv reports whether PassUnsafeEnv values should be kept out of the action digest for this
+// target. This considers both the global [Build] PassUnsafeEnv config keyword and the target's own
+// pass_unsafe_env attribute, mirroring how the local cache excludes both from its hash.
+func (c *Client) excludesUnsafeEnv(state *core.BuildState, target *core.BuildTarget) bool {
+	if !c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest {
+		return false
+	}
+	if len(state.Config.Build.PassUnsafeEnv) > 0 {
+		return true
+	}
+	return target.PassUnsafeEnv != nil && len(*target.PassUnsafeEnv) > 0
 }
 
 // stripUnsafeEnv removes the values of PassUnsafeEnv variables from the given environment so that they do
-// not contribute to the action digest. Variables that are also listed in PassEnv are left intact, since
-// those values are intentionally part of the cache key.
-func (c *Client) stripUnsafeEnv(target *core.BuildTarget, env core.BuildEnv) {
-	if !c.excludesUnsafeEnv(target) {
+// not contribute to the action digest. Both the global [Build] PassUnsafeEnv config keyword and the
+// target's pass_unsafe_env attribute are considered. Variables that are also listed in PassEnv (config or
+// target level) are left intact, since those values are intentionally part of the cache key.
+func (c *Client) stripUnsafeEnv(state *core.BuildState, target *core.BuildTarget, env core.BuildEnv) {
+	if !c.excludesUnsafeEnv(state, target) {
 		return
 	}
 	safe := map[string]bool{}
+	for _, e := range state.Config.Build.PassEnv {
+		safe[e] = true
+	}
 	if target.PassEnv != nil {
 		for _, e := range *target.PassEnv {
 			safe[e] = true
 		}
 	}
-	for _, e := range *target.PassUnsafeEnv {
-		if !safe[e] {
-			delete(env, e)
+	strip := func(vars []string) {
+		for _, e := range vars {
+			if !safe[e] {
+				delete(env, e)
+			}
 		}
 	}
+	strip(state.Config.Build.PassUnsafeEnv)
+	if target.PassUnsafeEnv != nil {
+		strip(*target.PassUnsafeEnv)
+	}
 }
 
 // stampedBuildEnvironment returns a build environment, optionally with a stamp if stamp is true.
@@ -194,7 +212,7 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg
 	cmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state))
 	env := core.TestEnvironment(state, target, ".", run)
 	if canonical {
-		c.stripUnsafeEnv(target, env)
+		c.stripUnsafeEnv(state, target, env)
 	}
 	return &pb.Command{
 		Platform: &pb.Platform{

src/remote/impl_test.go

@@ -34,13 +34,23 @@ func newClient() *Client {
 }
 
 func newClientInstance(name string) *Client {
+	return newClientInstanceWith(name, nil)
+}
+
+// newClientInstanceWith builds a client, optionally applying configure to the configuration before the
+// build state (and the client's async initialisation) is created. This matters for config values that are
+// captured once, e.g. the build environment derived from PassUnsafeEnv.
+func newClientInstanceWith(name string, configure func(*core.Configuration)) *Client {
 	config := core.DefaultConfiguration()
 	config.Build.Path = []string{"/usr/local/bin", "/usr/bin", "/bin"}
 	config.Build.HashFunction = "sha256"
 	config.Remote.NumExecutors = 1
 	config.Remote.Instance = name
 	config.Remote.Secure = false
 	config.Remote.Platform = []string{"OSFamily=linux"}
+	if configure != nil {
+		configure(config)
+	}
 	state := core.NewBuildState(config)
 	state.Config.Remote.URL = "127.0.0.1:9987"
 	state.Config.Remote.AssetURL = state.Config.Remote.URL

src/remote/remote.go

@@ -417,7 +417,7 @@ func (c *Client) build(target *core.BuildTarget) (*core.BuildMetadata, *pb.Actio
 	// store & look up results under a "cache-key" action digest that omits those inputs, while still
 	// executing the real action that includes them. This implements the rule whereby e.g. the SCM
 	// revision or an unsafe env var changing doesn't force a rebuild.
-	useCacheKeyDigest := target.Stamp || c.excludesUnsafeEnv(target)
+	useCacheKeyDigest := target.Stamp || c.excludesUnsafeEnv(c.state.ForTarget(target), target)
 	var cacheKeyDigest *pb.Digest
 	if useCacheKeyDigest {
 		command, digest, err := c.buildAction(target, false, false, true, 0)
@@ -614,7 +614,7 @@ func (c *Client) Test(target *core.BuildTarget, run int) (metadata *core.BuildMe
 	}
 	// As in build(), if PassUnsafeEnv values are excluded from the digest we look results up under a
 	// cache-key digest that omits them, while executing the real action that includes them.
-	useCacheKeyDigest := c.excludesUnsafeEnv(target)
+	useCacheKeyDigest := c.excludesUnsafeEnv(c.state.ForTarget(target), target)
 	var ar *pb.ActionResult
 	if useCacheKeyDigest {
 		cacheKeyCommand, cacheKeyDigest, cErr := c.buildAction(target, true, false, true, run)

src/remote/remote_test.go

@@ -267,6 +267,65 @@ func TestPassUnsafeEnvExcludedFromDigest(t *testing.T) {
 	assert.True(t, envContainsValue(real1, "first"))
 }
 
+// TestStripUnsafeEnvConfigLevel checks the stripping logic removes values from the global [Build]
+// PassUnsafeEnv config keyword while retaining PassEnv values (which are intentionally part of the cache key).
+func TestStripUnsafeEnvConfigLevel(t *testing.T) {
+	c := newClientInstance("strip")
+	c.state.Config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true
+	c.state.Config.Build.PassUnsafeEnv = []string{"CFG_UNSAFE"}
+	c.state.Config.Build.PassEnv = []string{"CFG_SAFE"}
+
+	target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "x"})
+	unsafe := []string{"TGT_UNSAFE"}
+	target.PassUnsafeEnv = &unsafe
+
+	env := core.BuildEnv{
+		"CFG_UNSAFE": "secret",
+		"CFG_SAFE":   "keep",
+		"TGT_UNSAFE": "alsosecret",
+		"OTHER":      "v",
+	}
+	c.stripUnsafeEnv(c.state.ForTarget(target), target, env)
+
+	_, cfgUnsafePresent := env["CFG_UNSAFE"]
+	_, tgtUnsafePresent := env["TGT_UNSAFE"]
+	assert.False(t, cfgUnsafePresent, "config-level PassUnsafeEnv should be stripped")
+	assert.False(t, tgtUnsafePresent, "target-level PassUnsafeEnv should be stripped")
+	assert.Equal(t, "keep", env["CFG_SAFE"], "PassEnv must be retained")
+	assert.Equal(t, "v", env["OTHER"], "unrelated env must be retained")
+}
+
+// TestConfigPassUnsafeEnvExcludedFromDigest checks that values declared via the global [Build]
+// PassUnsafeEnv config keyword (not just the per-target attribute) are excluded from the cache-key digest.
+// Because config-level values are captured once per config object, this uses a separate client per value
+// and sets the config before the client (and its async init) is created.
+func TestConfigPassUnsafeEnvExcludedFromDigest(t *testing.T) {
+	canonicalAndReal := func(value string) (string, string) {
+		t.Setenv("MY_CFG_UNSAFE", value)
+		c := newClientInstanceWith(fmt.Sprintf("cfg-unsafe-%d", time.Now().UnixNano()), func(config *core.Configuration) {
+			config.Remote.ExcludePassUnsafeEnvVarsFromDigest = true
+			config.Build.PassUnsafeEnv = []string{"MY_CFG_UNSAFE"}
+		})
+		target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "cfgunsafe"})
+		target.AddOutput("out.txt")
+		target.Command = "echo hello > $OUT"
+		target.BuildTimeout = time.Minute
+		canon, canonDigest, err := c.buildAction(target, false, false, true, 0)
+		require.NoError(t, err)
+		require.False(t, envContainsValue(canon, value), "config-level PassUnsafeEnv value must not be in the canonical command")
+		real, realDigest, err := c.buildAction(target, false, true, false, 0)
+		require.NoError(t, err)
+		require.True(t, envContainsValue(real, value), "executed action should include the config-level value")
+		return canonDigest.Hash, realDigest.Hash
+	}
+
+	canonFirst, realFirst := canonicalAndReal("first")
+	canonSecond, realSecond := canonicalAndReal("second")
+
+	assert.Equal(t, canonFirst, canonSecond, "config-level PassUnsafeEnv value must not affect the cache-key digest")
+	assert.NotEqual(t, realFirst, realSecond, "the executed action should still include the config-level value")
+}
+
 // TestPassUnsafeEnvRemoteCacheHitAcrossValues is an end-to-end test against the in-process testServer
 // that proves changing a PassUnsafeEnv value does not re-execute the action: the first build executes
 // and backfills the cache-key digest, and a second build with a *different* value (and a fresh client,