Prevent raw observability log loss during artifact extraction

pkg/cli/codemod_engine_env_secrets_pure_test.go

@@ -173,7 +173,6 @@ func TestRemoveUnsafeEngineEnvKeys(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			gotLines, gotModified := removeUnsafeEngineEnvKeys(tt.lines, tt.unsafeKeys)

pkg/cli/codemod_messages_effective_tokens_suffix_to_ai_credits_suffix_pure_test.go

@@ -147,7 +147,6 @@ func TestMigrateMessagesEffectiveTokensSuffixToAICreditsSuffix(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			gotLines, gotModified := migrateMessagesEffectiveTokensSuffixToAICreditsSuffix(tt.lines)

pkg/cli/logs_artifact_set.go

@@ -301,8 +301,8 @@ func findMissingFilterEntries(filter []string, outputDir string) []string {
 }
 
 func markArtifactDownloaded(outputDir, artifactName string) error {
-	if artifactName == "" || filepath.Base(artifactName) != artifactName {
-		return fmt.Errorf("invalid artifact name %q", artifactName)
+	if err := validateArtifactName(artifactName); err != nil {
+		return err
 	}
 	markerDir := filepath.Join(outputDir, downloadedArtifactsMarkerDir)
 	if err := os.MkdirAll(markerDir, constants.DirPermPublic); err != nil {

pkg/cli/logs_artifact_set_test.go

@@ -453,9 +453,13 @@ func TestFindMissingFilterEntriesAllMarkerSatisfiesFiltered(t *testing.T) {
 }
 
 func TestMarkArtifactDownloadedRejectsInvalidNames(t *testing.T) {
-	err := markArtifactDownloaded(t.TempDir(), "../activation")
-	require.Error(t, err)
-	assert.Contains(t, err.Error(), "invalid artifact name")
+	for _, name := range []string{"../activation", `..\activation`, ".", ".."} {
+		t.Run(name, func(t *testing.T) {
+			err := markArtifactDownloaded(t.TempDir(), name)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "invalid artifact name")
+		})
+	}
 }
 
 // TestFindMissingFilterEntriesIncrementalScenario validates the key scenario used by

pkg/cli/logs_download.go

@@ -468,6 +468,20 @@ func ensureUsageAwInfoFallback(ctx context.Context, opts downloadArtifactsOption
 	if fileutil.FileExists(awInfoPath) {
 		return
 	}
+	if usageDir := findArtifactDir(opts.outputDir, constants.UsageArtifactName, ""); usageDir != "" {
+		usageAwInfoPath := filepath.Join(usageDir, "aw_info.json")
+		if fileutil.FileExists(usageAwInfoPath) {
+			data, err := os.ReadFile(usageAwInfoPath)
+			if err != nil {
+				logsDownloadLog.Printf("Failed to read usage aw_info.json: %v", err)
+			} else if err := os.WriteFile(awInfoPath, data, constants.FilePermPublic); err != nil {
+				logsDownloadLog.Printf("Failed to copy usage aw_info.json to run root: %v", err)
+			} else {
+				logsDownloadLog.Printf("Copied usage aw_info.json to run root")
+				return
+			}
+		}
+	}
 
 	logsDownloadLog.Printf("aw_info.json missing from usage artifact, downloading activation artifact as fallback")
 	if opts.verbose {

pkg/cli/logs_download_artifacts.go

@@ -130,7 +130,17 @@ func downloadArtifactsByName(ctx context.Context, opts downloadArtifactsOptions,
 	shouldLogProgress := IsRunningInCI() || opts.verbose
 
 	for _, name := range names {
-		args := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", opts.outputDir}
+		if err := validateArtifactName(name); err != nil {
+			return err
+		}
+		// Stage next to the output directory so promotion can use an atomic same-filesystem rename.
+		stagingDir, err := os.MkdirTemp(filepath.Dir(opts.outputDir), "."+filepath.Base(opts.outputDir)+"-"+name+"-")
+		if err != nil {
+			return fmt.Errorf("failed to create staging directory for artifact %q: %w", name, err)
+		}
+
+		artifactDir := filepath.Join(opts.outputDir, name)
+		args := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", stagingDir}
 		if repoFlag != "" {
 			args = append(args, "-R", repoFlag)
 		}
@@ -143,13 +153,22 @@ func downloadArtifactsByName(ctx context.Context, opts downloadArtifactsOptions,
 		cmd := workflow.ExecGHContext(ctx, args...)
 		cmdOutput, cmdErr := cmd.CombinedOutput()
 		if cmdErr != nil {
+			_ = os.RemoveAll(stagingDir)
 			logsDownloadLog.Printf("Failed to download artifact %q: %v (%s)", name, cmdErr, string(cmdOutput))
 			if opts.verbose {
 				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to download artifact %q: %v", name, cmdErr)))
 			}
 			// Non-fatal: continue downloading other artifacts
 		} else {
 			logsDownloadLog.Printf("Downloaded artifact %q", name)
+			if err := os.RemoveAll(artifactDir); err != nil {
+				_ = os.RemoveAll(stagingDir)
+				return fmt.Errorf("failed to remove existing artifact directory %q: %w", artifactDir, err)
+			}
+			if err := os.Rename(stagingDir, artifactDir); err != nil {
+				_ = os.RemoveAll(stagingDir)
+				return fmt.Errorf("failed to promote artifact %q from staging: %w", name, err)
+			}
 			if err := markArtifactDownloaded(opts.outputDir, name); err != nil {
 				return err
 			}
@@ -159,6 +178,13 @@ func downloadArtifactsByName(ctx context.Context, opts downloadArtifactsOptions,
 	return nil
 }
 
+func validateArtifactName(name string) error {
+	if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) || filepath.Base(name) != name {
+		return fmt.Errorf("invalid artifact name %q", name)
+	}
+	return nil
+}
+
 // criticalArtifactNames lists the artifact names that are essential for audit analysis.
 // When a bulk download fails partially (e.g., due to non-zip artifacts), these artifacts
 // are retried individually so that flattening and audit extraction have data to work with.
@@ -173,6 +199,10 @@ func retryCriticalArtifacts(ctx context.Context, opts downloadArtifactsOptions)
 	repoFlag := buildRepoFlag(opts.owner, opts.repo, opts.hostname)
 
 	for _, name := range criticalArtifactNames {
+		if err := validateArtifactName(name); err != nil {
+			logsDownloadLog.Printf("Skipping invalid critical artifact name: %v", err)
+			continue
+		}
 		// Skip artifacts not included in the active filter.
 		if !artifactMatchesFilter(name, opts.artifactFilter) {
 			logsDownloadLog.Printf("Skipping critical artifact %q (not in artifact filter)", name)
@@ -184,7 +214,14 @@ func retryCriticalArtifacts(ctx context.Context, opts downloadArtifactsOptions)
 			continue
 		}
 
-		retryArgs := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", opts.outputDir}
+		// Stage next to the output directory so promotion can use an atomic same-filesystem rename.
+		stagingDir, err := os.MkdirTemp(filepath.Dir(opts.outputDir), "."+filepath.Base(opts.outputDir)+"-"+name+"-")
+		if err != nil {
+			logsDownloadLog.Printf("Failed to create staging directory for critical artifact %q: %v", name, err)
+			continue
+		}
+
+		retryArgs := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", stagingDir}
 		if repoFlag != "" {
 			retryArgs = append(retryArgs, "-R", repoFlag)
 		}
@@ -197,12 +234,23 @@ func retryCriticalArtifacts(ctx context.Context, opts downloadArtifactsOptions)
 		retryCmd := workflow.ExecGHContext(ctx, retryArgs...)
 		retryOutput, retryErr := retryCmd.CombinedOutput()
 		if retryErr != nil {
+			_ = os.RemoveAll(stagingDir)
 			logsDownloadLog.Printf("Failed to download artifact %q individually: %v (%s)", name, retryErr, string(retryOutput))
 			if opts.verbose {
 				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not download artifact %q: %v", name, retryErr)))
 			}
 		} else {
 			logsDownloadLog.Printf("Successfully downloaded artifact %q individually", name)
+			if err := os.RemoveAll(artifactDir); err != nil {
+				_ = os.RemoveAll(stagingDir)
+				logsDownloadLog.Printf("Failed to remove existing critical artifact directory %q: %v", artifactDir, err)
+				continue
+			}
+			if err := os.Rename(stagingDir, artifactDir); err != nil {
+				_ = os.RemoveAll(stagingDir)
+				logsDownloadLog.Printf("Failed to promote critical artifact %q from staging: %v", name, err)
+				continue
+			}
 			// Marker write failures are non-fatal in the retry path: retryCriticalArtifacts
 			// is a best-effort recovery after a partial bulk download, so a missing marker
 			// only causes a redundant re-download on the next run (not data loss).

pkg/cli/logs_download_flatten.go

@@ -169,6 +169,14 @@ func flattenArtifactTree(sourceDir, artifactDir, outputDir, label string, verbos
 				return fmt.Errorf("failed to create parent directory for %s: %w", destPath, err)
 			}
 
+			if fileutil.FileExists(destPath) {
+				logsDownloadLog.Printf("Skipping duplicate flattened file %s from %s; destination already exists", relPath, label)
+				if verbose {
+					fmt.Fprintln(os.Stderr, console.FormatVerboseMessage("Skipped duplicate flattened file: "+relPath))
+				}
+				return nil
+			}
+
 			if err := os.Rename(path, destPath); err != nil {
 				return fmt.Errorf("failed to move file %s to %s: %w", path, destPath, err)
 			}

pkg/cli/logs_download_test.go

@@ -328,7 +328,8 @@ func TestDownloadArtifactsByName_LogsArtifactNamesInCI(t *testing.T) {
 		os.Stderr = originalStderr
 	})
 
-	err = downloadArtifactsByName(context.Background(), downloadArtifactsOptions{runID: 12345, outputDir: t.TempDir()}, []string{"usage"})
+	outputDir := t.TempDir()
+	err = downloadArtifactsByName(context.Background(), downloadArtifactsOptions{runID: 12345, outputDir: outputDir}, []string{"usage"})
 	require.NoError(t, err)
 
 	require.NoError(t, writer.Close())
@@ -339,6 +340,92 @@ func TestDownloadArtifactsByName_LogsArtifactNamesInCI(t *testing.T) {
 	argsLog, err := os.ReadFile(argsLogPath)
 	require.NoError(t, err)
 	assert.Contains(t, string(argsLog), "run download 12345 --name usage")
+	assert.Contains(t, string(argsLog), "--dir ")
+	assert.NotContains(t, string(argsLog), "--dir "+filepath.Join(outputDir, "usage"))
+}
+
+func TestDownloadRunArtifacts_IsolatesAndFlattensOverlappingArtifacts(t *testing.T) {
+	fakeBinDir := testutil.TempDir(t, "fake-gh-*")
+	fakeGH := filepath.Join(fakeBinDir, "gh")
+	fakeGHScript := `#!/bin/sh
+if [ "$1" = "api" ]; then
+  printf '%s\n' "usage"
+  printf '%s\n' "agent"
+  exit 0
+fi
+name=""
+dir=""
+while [ $# -gt 0 ]; do
+  case "$1" in
+    --name) name="$2"; shift 2 ;;
+    --dir) dir="$2"; shift 2 ;;
+    *) shift ;;
+  esac
+done
+mkdir -p "$dir"
+if [ -e "$dir/shared.json" ]; then
+  exit 1
+fi
+printf '%s' "$name" > "$dir/shared.json"
+if [ "$name" = "agent" ]; then
+  mkdir -p "$dir/mcp-logs" "$dir/sandbox/firewall/logs"
+  printf '%s' '{}' > "$dir/mcp-logs/rpc-messages.jsonl"
+  printf '%s' 'request' > "$dir/sandbox/firewall/logs/access.log"
+fi
+`
+	require.NoError(t, os.WriteFile(fakeGH, []byte(fakeGHScript), 0o755))
+	t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	outputDir := t.TempDir()
+	err := downloadRunArtifacts(
+		context.Background(),
+		downloadArtifactsOptions{runID: 12345, outputDir: outputDir, owner: "github", repo: "gh-aw", artifactFilter: []string{"usage", "agent"}},
+	)
+	// This is the regression gate: both isolated downloads and final flattening
+	// must tolerate overlapping shared.json files without aborting.
+	require.NoError(t, err)
+
+	assert.FileExists(t, filepath.Join(outputDir, "shared.json"))
+	assert.FileExists(t, filepath.Join(outputDir, "mcp-logs", "rpc-messages.jsonl"))
+	assert.FileExists(t, filepath.Join(outputDir, "sandbox", "firewall", "logs", "access.log"))
+	assert.NoDirExists(t, filepath.Join(outputDir, "agent"))
+}
+
+func TestDownloadArtifactsByName_DoesNotCacheFailedStagingDirectory(t *testing.T) {
+	fakeBinDir := testutil.TempDir(t, "fake-gh-*")
+	fakeGH := filepath.Join(fakeBinDir, "gh")
+	statePath := filepath.Join(fakeBinDir, "state")
+	fakeGHScript := `#!/bin/sh
+dir=""
+while [ $# -gt 0 ]; do
+  case "$1" in
+    --dir) dir="$2"; shift 2 ;;
+    *) shift ;;
+  esac
+done
+mkdir -p "$dir"
+if [ ! -e "` + statePath + `" ]; then
+  : > "` + statePath + `"
+  printf '%s' 'partial' > "$dir/partial.txt"
+  exit 1
+fi
+printf '%s' 'complete' > "$dir/complete.txt"
+`
+	require.NoError(t, os.WriteFile(fakeGH, []byte(fakeGHScript), 0o755))
+	t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	outputDir := t.TempDir()
+	err := downloadArtifactsByName(context.Background(), downloadArtifactsOptions{runID: 12345, outputDir: outputDir}, []string{"usage"})
+	// downloadArtifactsByName deliberately logs command failures and continues;
+	// the regression check is that failed staging content is removed and does not
+	// satisfy the cache.
+	require.NoError(t, err)
+	assert.NoDirExists(t, filepath.Join(outputDir, "usage"))
+	assert.Equal(t, []string{"usage"}, findMissingFilterEntries([]string{"usage"}, outputDir))
+
+	err = downloadArtifactsByName(context.Background(), downloadArtifactsOptions{runID: 12345, outputDir: outputDir}, []string{"usage"})
+	require.NoError(t, err)
+	assert.FileExists(t, filepath.Join(outputDir, "usage", "complete.txt"))
 }
 
 func TestDownloadRunArtifacts_CachedUsageFallbackToActivation(t *testing.T) {
@@ -373,9 +460,9 @@ func TestDownloadRunArtifacts_CachedUsageFallbackToActivation(t *testing.T) {
 		"    fi\n" +
 		"    shift\n" +
 		"  done\n" +
-		"  mkdir -p \"$dir/$name\"\n" +
-		"  printf '%s' '{\"engine_id\":\"claude\"}' > \"$dir/$name/aw_info.json\"\n" +
-		"  : > \"$dir/.fallback-download-$name\"\n" +
+		"  mkdir -p \"$dir\"\n" +
+		"  printf '%s' '{\"engine_id\":\"claude\"}' > \"$dir/aw_info.json\"\n" +
+		"  : > \"" + tmpDir + "/.fallback-download-$name\"\n" +
 		"  exit 0\n" +
 		"fi\n" +
 		"exit 1\n"
@@ -400,6 +487,50 @@ func TestDownloadRunArtifacts_CachedUsageFallbackToActivation(t *testing.T) {
 	assert.Contains(t, string(argsLog), "run download 12345 --name abc123-activation")
 }
 
+func TestDownloadRunArtifacts_UsesAwInfoFromUsageArtifact(t *testing.T) {
+	tmpDir := testutil.TempDir(t, "usage-aw-info-*")
+
+	fakeBinDir := testutil.TempDir(t, "fake-gh-*")
+	fakeGH := filepath.Join(fakeBinDir, "gh")
+	argsLogPath := filepath.Join(fakeBinDir, "gh-args.log")
+	fakeGHScript := "#!/bin/sh\n" +
+		"printf '%s\\n' \"$*\" >> \"" + argsLogPath + "\"\n" +
+		"if [ \"$1\" = \"api\" ]; then\n" +
+		"  printf '%s\\n' \"usage\"\n" +
+		"  exit 0\n" +
+		"fi\n" +
+		"if [ \"$1\" = \"run\" ] && [ \"$2\" = \"download\" ]; then\n" +
+		"  name=\"\"\n" +
+		"  dir=\"\"\n" +
+		"  while [ $# -gt 0 ]; do\n" +
+		"    if [ \"$1\" = \"--name\" ]; then name=\"$2\"; shift 2; continue; fi\n" +
+		"    if [ \"$1\" = \"--dir\" ]; then dir=\"$2\"; shift 2; continue; fi\n" +
+		"    shift\n" +
+		"  done\n" +
+		"  mkdir -p \"$dir\"\n" +
+		"  if [ \"$name\" = \"usage\" ]; then\n" +
+		"    printf '%s' '{\"engine_id\":\"claude\"}' > \"$dir/aw_info.json\"\n" +
+		"    printf '%s' '{\"tokens\":1}' > \"$dir/usage.jsonl\"\n" +
+		"    exit 0\n" +
+		"  fi\n" +
+		"fi\n" +
+		"exit 1\n"
+	require.NoError(t, os.WriteFile(fakeGH, []byte(fakeGHScript), 0o755))
+	t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	err := downloadRunArtifacts(context.Background(), downloadArtifactsOptions{runID: 12345, outputDir: tmpDir, verbose: false, owner: "github", repo: "gh-aw", artifactFilter: []string{"usage"}})
+	require.NoError(t, err)
+
+	awInfo, err := os.ReadFile(filepath.Join(tmpDir, "aw_info.json"))
+	require.NoError(t, err)
+	assert.JSONEq(t, `{"engine_id":"claude"}`, string(awInfo))
+
+	argsLog, err := os.ReadFile(argsLogPath)
+	require.NoError(t, err)
+	assert.Contains(t, string(argsLog), "run download 12345 --name usage")
+	assert.NotContains(t, string(argsLog), "--name activation")
+}
+
 func TestDownloadRunArtifactsFallbackWhenListFails(t *testing.T) {
 	tmpDir := testutil.TempDir(t, "test-download-fallback-*")
 
@@ -429,8 +560,8 @@ func TestDownloadRunArtifactsFallbackWhenListFails(t *testing.T) {
 		"    shift\n" +
 		"  done\n" +
 		"  if [ \"$name\" = \"usage\" ]; then\n" +
-		"    mkdir -p \"$dir/$name\"\n" +
-		"    printf '%s' '{\"tokens\":1}' > \"$dir/$name/usage.jsonl\"\n" +
+		"    mkdir -p \"$dir\"\n" +
+		"    printf '%s' '{\"tokens\":1}' > \"$dir/usage.jsonl\"\n" +
 		"  fi\n" +
 		"  exit 0\n" +
 		"fi\n" +