diff --git a/pkg/workflow/safe_output_helpers_test.go b/pkg/workflow/safe_output_helpers_test.go index 962436f33e..c547c63608 100644 --- a/pkg/workflow/safe_output_helpers_test.go +++ b/pkg/workflow/safe_output_helpers_test.go @@ -594,6 +594,9 @@ func TestEnginesUseSameHelperLogic(t *testing.T) { // TestBuildAgentOutputDownloadSteps verifies the agent output download steps // include directory creation to handle cases where artifact doesn't exist, // and that GH_AW_AGENT_OUTPUT is only set when the artifact download succeeds. +// The Gemini engine's GetPreBundleSteps moves /tmp/gemini-client-error-*.json +// into /tmp/gh-aw/ before upload, so the artifact LCA is always /tmp/gh-aw/ +// and the hardcoded path is reliable. func TestBuildAgentOutputDownloadSteps(t *testing.T) { steps := buildAgentOutputDownloadSteps("") stepsStr := strings.Join(steps, "") @@ -609,8 +612,9 @@ func TestBuildAgentOutputDownloadSteps(t *testing.T) { "- name: Setup agent output environment variable", "if: steps.download-agent-output.outcome == 'success'", "mkdir -p /tmp/gh-aw/", - "find \"/tmp/gh-aw/\" -type f -print", - "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json", + `find "/tmp/gh-aw/" -type f -print`, + // Hardcoded path is correct because GetPreBundleSteps ensures LCA is /tmp/gh-aw/ + `echo "GH_AW_AGENT_OUTPUT=/tmp/gh-aw/agent_output.json" >> "$GITHUB_ENV"`, } for _, expected := range expectedComponents { @@ -619,9 +623,15 @@ func TestBuildAgentOutputDownloadSteps(t *testing.T) { } } + // Verify no dynamic find-based lookup is used (regression guard: the Gemini engine + // moves files to /tmp/gh-aw/ via GetPreBundleSteps so the hardcoded path is always valid) + if strings.Contains(stepsStr, "FOUND_FILE=$(find") { + t.Error("Step must not use dynamic find resolution; hardcoded path should be used instead") + } + // Verify mkdir comes before find to ensure directory exists mkdirIdx := strings.Index(stepsStr, "mkdir -p /tmp/gh-aw/") - findIdx := strings.Index(stepsStr, "find \"/tmp/gh-aw/\"") + findIdx := strings.Index(stepsStr, `find "/tmp/gh-aw/"`) if mkdirIdx == -1 { t.Fatal("mkdir command not found in steps") diff --git a/pkg/workflow/step_order_validation.go b/pkg/workflow/step_order_validation.go index 80bd8722ed..d1f2d9d776 100644 --- a/pkg/workflow/step_order_validation.go +++ b/pkg/workflow/step_order_validation.go @@ -178,8 +178,10 @@ func (t *StepOrderTracker) findUnscannablePaths(artifactUploads []StepRecord) [] // isPathScannedBySecretRedaction checks if a path would be scanned by the secret redaction step // or is otherwise safe to upload (known engine-controlled diagnostic paths). func isPathScannedBySecretRedaction(path string) bool { - // Paths must be under /tmp/gh-aw/ or ${RUNNER_TEMP}/gh-aw/ to be scanned - // Accept both literal paths and environment variable references + // Paths must be under /tmp/gh-aw/ or ${RUNNER_TEMP}/gh-aw/ to be scanned. + // Accept both literal paths and environment variable references. + // Engines that produce output outside /tmp/gh-aw/ must move their files into /tmp/gh-aw/ + // via GetPreBundleSteps before the unified artifact upload (see gemini_engine.go). if !strings.HasPrefix(path, "/tmp/gh-aw/") && !strings.HasPrefix(path, "${RUNNER_TEMP}/gh-aw/") { // Check if it's an environment variable that might resolve to /tmp/gh-aw/ or ${RUNNER_TEMP}/gh-aw/ // For now, we'll allow ${{ env.* }} patterns through as we can't resolve them at compile time @@ -189,20 +191,6 @@ func isPathScannedBySecretRedaction(path string) bool { return true } - // Allow wildcard paths under /tmp/ with a known-safe extension. - // These are engine-declared diagnostic output files that have not yet been relocated. - // NOTE: The Gemini engine now moves its error reports into /tmp/gh-aw/ via - // GetPreBundleSteps before the upload, so they are covered by the /tmp/gh-aw/ branch - // above. This exception is kept for any future engine that may produce wildcard - // diagnostic files directly under /tmp/. - if strings.HasPrefix(path, "/tmp/") && strings.Contains(path, "*") { - ext := filepath.Ext(path) - safeExtensions := []string{".txt", ".json", ".log", ".jsonl"} - if slices.Contains(safeExtensions, ext) { - return true - } - } - return false } diff --git a/pkg/workflow/step_order_validation_test.go b/pkg/workflow/step_order_validation_test.go index 760fbd886d..cbf977bca3 100644 --- a/pkg/workflow/step_order_validation_test.go +++ b/pkg/workflow/step_order_validation_test.go @@ -166,6 +166,18 @@ func TestIsPathScannedBySecretRedaction_UnscannableFiles(t *testing.T) { path: "/tmp/gh-aw/data.bin", expected: false, }, + { + // Wildcard paths outside /tmp/gh-aw/ are rejected - engines must move files + // into /tmp/gh-aw/ via GetPreBundleSteps (e.g. gemini_engine.go) + name: "Wildcard JSON under /tmp/ (not /tmp/gh-aw/)", + path: "/tmp/gemini-client-error-*.json", + expected: false, + }, + { + name: "Wildcard log under /tmp/ (not /tmp/gh-aw/)", + path: "/tmp/some-engine-*.log", + expected: false, + }, } for _, tt := range tests {