fix: correct engine.env secrets warning — excluded from sandbox, not leaked#43302
Conversation
…m sandbox
The warning "will be leaked to the agent container" was factually incorrect
for engine.env secrets. ComputeAWFExcludeEnvVarNames already auto-detects
${{ secrets. values in engine.env and adds them to awf --exclude-env,
preventing them from reaching the agent sandbox.
Now validateEnvSecretsSection emits section-aware messages:
- engine.env (non-strict): "will be excluded from the agent sandbox via
awf --exclude-env; the agent process itself will not see these values
directly"
- engine.env (strict): "are auto-excluded from the agent sandbox via
awf --exclude-env and are not accessible to the agent"
- top-level env: messages unchanged (secrets there genuinely leak)
Update tests to match the corrected engine.env error message strings.
Closes #42749
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add test to ensure auto exclusion is enabled and functional |
…exclusion Adds a table-driven test for ComputeAWFExcludeEnvVarNames covering: - engine.env var with a secret reference is auto-excluded - engine.env var with no secret reference is not excluded - mixed engine.env: only secret-backed vars are excluded - engine.env secret combined with core engine secret vars - engine.env secret embedded in a larger string value is excluded - nil EngineConfig produces no extra exclusions beyond core secrets Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added
|
|
Great fix! 👋 Correcting the false "will be leaked" warning for A few things stand out positively here:
This PR looks ready for review. 🚀
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR corrects misleading secrets-validation messaging for engine.env so it no longer claims secrets “will be leaked” when the compiler is configured to exclude secret-bearing env vars from the agent sandbox via AWF --exclude-env.
Changes:
- Updated
validateEnvSecretsSectionto emit section-aware strict-mode errors and non-strict warnings forengine.env, distinguishing it from top-levelenv. - Updated strict-mode
engine.envtest assertions to match the revised error message text. - Added unit coverage for
ComputeAWFExcludeEnvVarNamesto verifyengine.envsecrets are included in the--exclude-envlist.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/strict_mode_env_validation.go | Adjusts strict/non-strict messaging for secrets found in engine.env vs other env sections. |
| pkg/workflow/env_secrets_validation_test.go | Updates expected error-message substrings for strict-mode engine.env cases. |
| pkg/workflow/awf_helpers_test.go | Adds tests validating ComputeAWFExcludeEnvVarNames behavior for engine.env secret detection and exclusion. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| if sectionName == "engine.env" { | ||
| // engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env, | ||
| // so they are not leaked, but strict mode still requires engine-specific configuration. | ||
| return fmt.Errorf("strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox via awf --exclude-env and are not accessible to the agent. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", strings.Join(secretRefs, ", ")) |
| if sectionName == "engine.env" { | ||
| // engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env, | ||
| // so the warning should reflect that they are excluded, not leaked. | ||
| warningMsg = fmt.Sprintf("Warning: secrets detected in 'engine.env' section will be excluded from the agent sandbox via awf --exclude-env; the agent process itself will not see these values directly. Found: %s. Consider using engine-specific secret configuration instead.", strings.Join(secretRefs, ", ")) |
…essages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (114 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Review SummaryApplied 🔴 Blocking: version-gate accuracy
🟡 Non-blocking: test quality gaps
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on a correctness gap and test quality.
📋 Key Themes & Highlights
Key Themes
- Version-gate accuracy gap (correctness): The new reassuring messages say secrets "are auto-excluded" but
BuildAWFArgssilently skips--exclude-envfor AWF < v0.25.3. Workflows pinned to an old version would receive an inaccurate message — users could believe they are protected when they are not. - Misleading test case: The
AUTH_HEADERembedded-secret test uses a literal without the opening${{, so it is not actually testing the intended "embedded in a larger string" scenario. - Missing warning-message assertion: The non-strict
engine.envtest only checksexpectError: false— the corrected warning text is never verified and could silently regress. - Missing
agent.envcoverage:ComputeAWFExcludeEnvVarNamesalso scansagent.env, but there is no corresponding test case. - Duplicated magic string:
sectionName == "engine.env"appears in two separate branches with no named constant, creating a rename-hazard.
Positive Highlights
- ✅ Root cause is correctly identified: the old message genuinely misrepresented the behaviour and could mislead users into thinking secrets were actively leaked.
- ✅ Strict-mode guardrail preserved — still an error, just a factually accurate one.
- ✅ Good table-driven test structure in
TestComputeAWFExcludeEnvVarNameswith clearwant/notWantseparation. - ✅ PR description is well-written with clear before/after examples.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 107.8 AIC · ⌖ 5.96 AIC · ⊞ 6.6K
Comment /matt to run again
| if sectionName == "engine.env" { | ||
| // engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env, | ||
| // so they are not leaked, but strict mode still requires engine-specific configuration. | ||
| return fmt.Errorf("strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox via awf --exclude-env and are not accessible to the agent. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", strings.Join(secretRefs, ", ")) |
There was a problem hiding this comment.
[/diagnosing-bugs] The new message guarantees exclusion via --exclude-env, but BuildAWFArgs silently skips that flag for workflows pinned to AWF < v0.25.3 — so the claim is still factually wrong for old-pinned workflows.
💡 Suggested fix
Either qualify the message with a version condition, or have validateEnvSecretsSection receive the AWF version and vary the message:
// Option A: always mention the version requirement
return fmt.Errorf("strict mode: secrets detected in \u0027engine.env\u0027 section " +
"are auto-excluded from the agent sandbox via awf --exclude-env (AWF v0.25.3+) "+
"and are not accessible to the agent. Found: %s. ...", ...)Without this, a workflow pinned to v0.24.x would get the reassuring new message while --exclude-env never runs, leaving the secrets visible to the agent.
See: pkg/workflow/awf_helpers.go — awfSupportsExcludeEnv / AWFExcludeEnvMinVersion.
@copilot please address this.
| workflowData: &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ | ||
| Env: map[string]string{ | ||
| "AUTH_HEADER": "Bearer ${{ secrets.TOKEN }}", |
There was a problem hiding this comment.
[/tdd] The AUTH_HEADER test case uses "Bearer ${{ secrets.TOKEN }}" — but the actual literal in the file is "****** secrets.TOKEN }}" (missing the opening ${{), which still triggers the strings.Contains(envValue, "${{ secrets.") check in the implementation. The test name says "embedded in a larger string" but is actually testing detection without the full ${{ prefix — that is a different edge case than what the name implies.
💡 Suggestion
Rename and/or add a proper embedded-string variant:
{
name: "engine.env secret bare prefix (missing opening) is NOT excluded",
workflowData: &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"AUTH_HEADER": "Bearer secrets.TOKEN }}", // no ${{, should not match
},
},
},
coreSecretVarNames: []string{},
want: []string{},
notWant: []string{"AUTH_HEADER"},
},
{
name: "engine.env secret embedded in larger string is excluded",
workflowData: &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"AUTH_HEADER": "Bearer ${{ secrets.TOKEN }}", // embedded, should match
},
},
},
coreSecretVarNames: []string{},
want: []string{"AUTH_HEADER"},
},@copilot please address this.
| errorMsg: "strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox via awf --exclude-env and are not accessible to the agent. Found: ${{ secrets.TOKEN }}", | ||
| }, | ||
| { | ||
| name: "engine.env with secret in non-strict mode emits warning (no error)", |
There was a problem hiding this comment.
[/tdd] The non-strict engine.env test only asserts expectError: false — the updated warning message text is never verified, so regressions to the new message would go undetected.
💡 Suggestion
Capture stderr in the test and assert the corrected message. The existing TestValidateEnvSecretsNonStrictMode uses the same pattern (no assertion on message) with a comment that stderr cannot be captured. Consider introducing a captureWarnings helper or redirect os.Stderr temporarily to verify the new message text:
var buf bytes.Buffer
old := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
err := compiler.validateEnvSecrets(tt.frontmatter)
w.Close()
os.Stderr = old
io.Copy(&buf, r)
assert.NoError(t, err)
assert.Contains(t, buf.String(), "will be excluded from the agent sandbox via awf --exclude-env")
assert.NotContains(t, buf.String(), "will be leaked")@copilot please address this.
| if sectionName == "engine.env" { | ||
| // engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env, | ||
| // so the warning should reflect that they are excluded, not leaked. | ||
| warningMsg = fmt.Sprintf("Warning: secrets detected in 'engine.env' section will be excluded from the agent sandbox via awf --exclude-env; the agent process itself will not see these values directly. Found: %s. Consider using engine-specific secret configuration instead.", strings.Join(secretRefs, ", ")) |
There was a problem hiding this comment.
[/diagnosing-bugs] The non-strict engine.env message is inconsistent with the strict-mode one: non-strict says "will be excluded" (future tense / conditional) while strict says "are auto-excluded" (present, definitive). Both share the same version-gate risk, but the different phrasing may confuse users who see one message in development and the other in CI.
💡 Suggestion
Aligning on the same phrasing and the same version caveat in both branches makes the UX clearer:
// strict
"are auto-excluded from the agent sandbox via awf --exclude-env (requires AWF v0.25.3+)"
// non-strict warning
"will be excluded from the agent sandbox via awf --exclude-env (requires AWF v0.25.3+)"@copilot please address this.
| @@ -1118,6 +1118,104 @@ func TestAWFSupportsExcludeEnv(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[/tdd] TestComputeAWFExcludeEnvVarNames covers engine.env vars well, but has no test for agent.env vars (the function also scans agentConfig.Env for ${{ secrets. values). A regression there would be silent.
💡 Suggestion
Add one case:
{
name: "agent.env secret var is auto-excluded",
workflowData: &WorkflowData{
AgentConfig: &AgentConfig{
Env: map[string]string{
"AGENT_TOKEN": "${{ secrets.AGENT_SECRET }}",
},
},
},
coreSecretVarNames: []string{},
want: []string{"AGENT_TOKEN"},
},This ensures the exclusion logic is symmetric for both engine.env and agent.env.
@copilot please address this.
|
|
||
| // In strict mode, this is an error | ||
| if c.strictMode { | ||
| if sectionName == "engine.env" { |
There was a problem hiding this comment.
[/codebase-design] The sectionName == "engine.env" string literal is repeated in two independent branches (lines 135 and 145). If engine.env is ever renamed in the call-site (e.g. to "engine.environment"), both branches need updating but the compiler won’t catch it.
💡 Suggestion
Extract the constant or pass a boolean:
const engineEnvSection = "engine.env"
// or — simpler — let the caller pass isEngineEnv bool so the function signature
// makes the branching intent explicit and can be tested with a boolean instead
// of a magic string.This also keeps the function focused on validation rather than section naming conventions.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics (9 test scenarios)
Verdict
Notes:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review: fix: correct engine.env secrets warning
The core logic change in strict_mode_env_validation.go is correct and well-motivated — the old messages falsely claimed engine.env secrets would be leaked when they're already auto-excluded via --exclude-env. The test updates in env_secrets_validation_test.go align cleanly with the new message text.
One blocking bug in the new test file and one minor docstring gap need addressing.
🔴 Bug: new test assertion will always fail
In awf_helpers_test.go, the test case "engine.env secret embedded in a larger string is excluded" sets:
"AUTH_HEADER": "****** secrets.TOKEN }}",ComputeAWFExcludeEnvVarNames detects secrets via:
if strings.Contains(varValue, "${{ secrets.") {The test value does not contain "${{ secrets." (it starts with asterisks, not ${{), so AUTH_HEADER will never be added to the exclude list and assert.Contains(t, got, "AUTH_HEADER") will fail.
Fix: change the test value to something like "Bearer ${{ secrets.TOKEN }}" which does contain the required prefix.
⚠️ Minor: stale docstring
The validateEnvSecrets function comment still reads "Secrets in env will be leaked to the agent container" without acknowledging that engine.env secrets are auto-excluded. The inline comment in the body correctly explains this, but the function-level doc should be updated too for clarity.
What's good
- The split-message logic (strict vs. non-strict, engine.env vs. top-level env) is clean and correct.
- The new
TestComputeAWFExcludeEnvVarNamestest function covers important cases (mixed secret/non-secret vars, nil engine config, core secret passthrough). - The
env_secrets_validation_test.gomessage-text updates are accurate.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 170.2 AIC · ⌖ 6.14 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/workflow/awf_helpers_test.go:1189
Bug: test value will not trigger --exclude-env detection
The value "****** secrets.TOKEN }}" (asterisks prefix, no ${{...) does not contain "${{ secrets.", so it won't match the strings.Contains(varValue, "${{ secrets.") check in ComputeAWFExcludeEnvVarNames. As a result, AUTH_HEADER will not be added to the exclude list and this test assertion will fail.
To test a secret embedded in a larger string that is still detected, use a value that contains the literal `$…
pkg/workflow/strict_mode_env_validation.go:18
Stale docstring: still says secrets "will be leaked"
The function-level comment still reads:
> Secrets in env will be leaked to the agent container.
This was accurate for the top-level env section but now misleads readers about engine.env, which was the whole point of this fix. Consider updating to something like:
// validateEnvSecrets detects secrets in the top-level env section and the engine.env section,
// raising an error in strict mode or a warning in non-strict mode. …
</details>
There was a problem hiding this comment.
REQUEST_CHANGES — two message-correctness issues and one test-coverage gap must be addressed.
📋 Findings summary
High — False guarantee in non-strict warning (line 148, strict_mode_env_validation.go)
The warning claims the secret will be excluded via --exclude-env, but that flag is only emitted when AWF ≥ v0.25.3 (awfSupportsExcludeEnv). Workflows that pin an older version get --env-all with no exclusion — and this message. The message needs to either hedge on version or be gated by actual version information.
Medium — Contradictory strict-mode error message (line 138, strict_mode_env_validation.go)
The error simultaneously asserts the secret is safe ("not accessible to the agent") and blocks the build. Users cannot understand why they need to act. The rationale — raw value still visible in source and to the host runner — must be stated explicitly.
Low — Empty want list produces no positive assertions (line 1155, awf_helpers_test.go)
The non-secret test case uses want: []string{}, so the assertion loop body never runs. An assert.Empty on got is needed to actually verify the exclusion list is empty.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 222.3 AIC · ⌖ 5.46 AIC · ⊞ 5.4K
Comment /review to run again
| if sectionName == "engine.env" { | ||
| // engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env, | ||
| // so the warning should reflect that they are excluded, not leaked. | ||
| warningMsg = fmt.Sprintf("Warning: secrets detected in 'engine.env' section will be excluded from the agent sandbox via awf --exclude-env; the agent process itself will not see these values directly. Found: %s. Consider using engine-specific secret configuration instead.", strings.Join(secretRefs, ", ")) |
There was a problem hiding this comment.
False guarantee for workflows pinning old AWF: The warning unconditionally tells users their secret "will be excluded from the agent sandbox via awf --exclude-env", but BuildAWFArgs only emits --exclude-env when awfSupportsExcludeEnv(firewallConfig) returns true (requires AWF ≥ v0.25.3). For workflows that pin an older AWF version, --env-all still injects the secret into the container and this claim is factually wrong.
💡 Suggested fix
validateEnvSecretsSection has no access to firewallConfig, so it cannot verify the version at warning time. Two options:
Option A — hedge on version in the message:
warningMsg = fmt.Sprintf(
"Warning: secrets detected in 'engine.env' section. On AWF >= v0.25.3 these "+
"are auto-excluded via --exclude-env; on older pinned AWF versions the secret "+
"still reaches the container. Found: %s. Consider using engine-specific "+
"secret configuration instead.",
strings.Join(secretRefs, ", "))Option B — thread firewallConfig (or a excludeEnvSupported bool) into validateEnvSecretsSection so the message can be dynamically accurate.
Without a fix, a user on AWF v0.25.0 who sees this warning will believe their secret is sandboxed when it is not.
| if sectionName == "engine.env" { | ||
| // engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env, | ||
| // so they are not leaked, but strict mode still requires engine-specific configuration. | ||
| return fmt.Errorf("strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox via awf --exclude-env and are not accessible to the agent. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", strings.Join(secretRefs, ", ")) |
There was a problem hiding this comment.
Contradictory strict-mode error message: The error tells users their secret is safe ("auto-excluded... not accessible to the agent") but then returns an error blocking the build. Users will be confused about why they need to change anything if the secret is already protected.
💡 Suggested fix
The message should explain why strict mode still rejects this pattern despite runtime exclusion — e.g., the raw secret value still appears in the workflow file, is accessible to the host process before AWF is invoked, and strict mode enforces engine-specific configuration for defence in depth:
return fmt.Errorf(
"strict mode: secrets detected in 'engine.env' section. "+
"While these are excluded from the agent sandbox at runtime (via awf --exclude-env), "+
"the raw secret value is still exposed in the workflow source and to the host runner process. "+
"Strict mode requires engine-specific secret configuration to avoid any exposure. "+
"Found: %s. See: https://github.github.com/gh-aw/reference/engines/",
strings.Join(secretRefs, ", "))This removes the contradiction and gives the user a clear reason to act.
| }, | ||
| }, | ||
| coreSecretVarNames: []string{}, | ||
| want: []string{}, |
There was a problem hiding this comment.
Empty want slice produces zero positive assertions: When want: []string{}, the assertion loop body never executes — nothing is verified about the returned slice being empty. If ComputeAWFExcludeEnvVarNames accidentally returned non-empty results for plain-value vars, this test would still pass.
💡 Suggested fix
Add an explicit emptiness check after the loop:
if len(tt.want) == 0 {
assert.Empty(t, got, "expected exclude list to be empty")
}Or fold it into the loop by asserting assert.Empty whenever the want list is nil/empty. Either way, the test should fail when the production code returns unexpected entries for non-secret env vars.
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=3; skipped_last_comment_from_sous_chef=1; skipped_cooldown=0; nudged=1; branch_update_attempts=0; formatter_pushes=0; merge_main_scheduled=0 |
🤖 PR Triage
Score breakdown: Impact 28 (corrects misleading 'secrets will be leaked' compiler warning) + Urgency 16 + Quality 14 (good description, tests updated) Corrects false 'will be leaked' warning for
|
|
@copilot please run the
|
…requirement Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the two unresolved reviewer threads in commit
Both messages use CI needs a human re-trigger (close/reopen or |
The compiler incorrectly warned that secrets in
engine.env"will be leaked to the agent container" even thoughComputeAWFExcludeEnvVarNamesalready auto-detects${{ secrets.values inengine.envand adds them to AWF's--exclude-envlist, keeping them out of the agent sandbox.Changes
pkg/workflow/strict_mode_env_validation.go—validateEnvSecretsSectionnow emits section-aware messages:engine.envnon-strict warning: replaced "will be leaked" with "will be excluded from the agent sandbox via awf--exclude-env; the agent process itself will not see these values directly"engine.envstrict-mode error: replaced "will be leaked" with "are auto-excluded from the agent sandbox via awf--exclude-envand are not accessible to the agent" — still an error (strict mode enforces engine-specific config), but no longer a false leakage claimenv: messages unchanged — secrets there genuinely reach the agent containerpkg/workflow/env_secrets_validation_test.go— updated threeengine.envstrict-mode test assertions to match the corrected error message textBefore / After (non-strict,
engine.env)