fix: validate agentDir is a directory and use dynamic label in checkDir logs

Copilot · pelikhan · web-flow · commit 7c6b5c646159 · 2026-04-30T18:37:02.000Z
- Guard against findArtifactDir returning a file path: check os.Stat IsDir() before probing for audit files inside the agent artifact directory - Derive log labels from filepath.Base(agentDir) so debug output reflects the actual on-disk name (agent-artifacts, abc123-agent, etc.) instead of the hardcoded "agent/..." string - Add test: file named "agent" does not panic or falsely match Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5534ec16-9018-4e51-8a41-c13438eecaf6 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
diff --git a/pkg/cli/firewall_policy.go b/pkg/cli/firewall_policy.go
@@ -443,15 +443,21 @@ func detectFirewallAuditArtifacts(runDir string) (manifestPath, auditJSONLPath s
 	// e.g., when the audit command is run on a directory populated via `gh run download`).
 	// Handles "agent", "agent-artifacts", and workflow_call prefixed names (e.g. "hash-agent").
 	if agentDir := findArtifactDir(runDir, "agent", "agent-artifacts"); agentDir != "" {
-		// New artifact structure (actions/upload-artifact v4+, /tmp/gh-aw/ prefix stripped):
-		//   agent/sandbox/firewall/audit/
-		if !checkDir(filepath.Join(agentDir, "sandbox", "firewall", "audit"), "agent/sandbox/firewall/audit") {
-			// Old artifact structure (/tmp/gh-aw/ prefix preserved inside the artifact):
-			//   agent/tmp/gh-aw/sandbox/firewall/audit/
-			checkDir(filepath.Join(agentDir, "tmp", "gh-aw", "sandbox", "firewall", "audit"), "agent/tmp/gh-aw/sandbox/firewall/audit")
-		}
-		if manifestPath != "" && auditJSONLPath != "" {
-			return
+		// Guard: findArtifactDir checks existence but not type; skip if it resolved to a file.
+		if info, err := os.Stat(agentDir); err != nil || !info.IsDir() {
+			firewallPolicyLog.Printf("Skipping agent artifact path (not a directory): %s", agentDir)
+		} else {
+			agentBase := filepath.Base(agentDir)
+			// New artifact structure (actions/upload-artifact v4+, /tmp/gh-aw/ prefix stripped):
+			//   <agentDir>/sandbox/firewall/audit/
+			if !checkDir(filepath.Join(agentDir, "sandbox", "firewall", "audit"), agentBase+"/sandbox/firewall/audit") {
+				// Old artifact structure (/tmp/gh-aw/ prefix preserved inside the artifact):
+				//   <agentDir>/tmp/gh-aw/sandbox/firewall/audit/
+				checkDir(filepath.Join(agentDir, "tmp", "gh-aw", "sandbox", "firewall", "audit"), agentBase+"/tmp/gh-aw/sandbox/firewall/audit")
+			}
+			if manifestPath != "" && auditJSONLPath != "" {
+				return
+			}
 		}
 	}
 
diff --git a/pkg/cli/firewall_policy_test.go b/pkg/cli/firewall_policy_test.go
@@ -655,6 +655,17 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) {
 		assert.Empty(t, foundManifest, "Should not find manifest")
 		assert.Empty(t, foundAudit, "Should not find audit JSONL")
 	})
+
+	t.Run("file named 'agent' does not panic or falsely match", func(t *testing.T) {
+		// If a plain file happens to be named "agent" in the run directory (e.g., a flattened
+		// single-file artifact from a different upload), the lookup must skip it gracefully.
+		dir := t.TempDir()
+		require.NoError(t, os.WriteFile(filepath.Join(dir, "agent"), []byte("not a directory"), 0644))
+
+		foundManifest, foundAudit := detectFirewallAuditArtifacts(dir)
+		assert.Empty(t, foundManifest, "Should not find manifest when 'agent' is a file")
+		assert.Empty(t, foundAudit, "Should not find audit JSONL when 'agent' is a file")
+	})
 }
 
 func TestAnalyzeFirewallPolicy(t *testing.T) {
