Duplicate flattening logic across artifact handlers in pkg/cli/logs_download.go #21168

Overview

The latest commit (b049e6dcb91e6b00b8a251b6c74e52806e38e1cd) includes a high-churn update in pkg/cli/logs_download.go that now contains three near-identical flattening flows for different artifact roots.

This is significant structural duplication (well above 10 lines and appearing in 3 locations), and it increases the risk of inconsistent behavior when flattening logic is changed.

Critical Information

• Pattern: Repeated directory-walk + move + cleanup workflow
• Severity: Medium
• Occurrences: 3
• Locations:
  • pkg/cli/logs_download.go (flattenUnifiedArtifact, lines 155-246)
  • pkg/cli/logs_download.go (flattenActivationArtifact, lines 252-326)
  • pkg/cli/logs_download.go (flattenAgentOutputsArtifact, lines 330-405)

err := filepath.Walk(sourceDir, func(path string, info os.FileInfo, err error) error {
    if err != nil {
        return err
    }
    if path == sourceDir {
        return nil
    }

    relPath, err := filepath.Rel(sourceDir, path)
    if err != nil {
        return fmt.Errorf("failed to get relative path for %s: %w", path, err)
    }

    destPath := filepath.Join(outputDir, relPath)
    if info.IsDir() {
        if err := os.MkdirAll(destPath, 0750); err != nil {
            return fmt.Errorf("failed to create directory %s: %w", destPath, err)
        }
    } else {
        if err := os.MkdirAll(filepath.Dir(destPath), 0750); err != nil {
            return fmt.Errorf("failed to create parent directory for %s: %w", destPath, err)
        }
        if err := os.Rename(path, destPath); err != nil {
            return fmt.Errorf("failed to move file %s to %s: %w", path, destPath, err)
        }
    }
    return nil
})

if err != nil {
    return fmt.Errorf("failed to flatten ...: %w", err)
}

if err := os.RemoveAll(sourceDir); err != nil {
    // warn only
}

Impact Analysis

• Maintainability: Any behavioral fix (permissions, logging, symlink policy, error wrapping, path handling) must be repeated in 3 places.
• Bug Risk: High chance of divergent fixes over time across artifact types.
• Code Bloat: ~150+ lines of repeated traversal/move/cleanup code.

Refactoring Recommendations

1. Extract shared flattener helper

• Suggested function: flattenArtifactTree(sourceDir, outputDir, verbose, options) in pkg/cli/logs_download.go (or a nearby helper file).
• Keep pre-processing differences in callers (e.g., old/new unified artifact source resolution).
• Estimated effort: 1-2 hours.

2. Parameterize post-flatten messages and error prefixes

• Pass artifact label ("unified", "activation", "agent_outputs") to keep current user-facing text while reusing logic.

3. Add focused tests for helper behavior

• Cases: nested dirs, file move errors, cleanup failures (warn-only), permissions creation, relative path handling.

Implementation Checklist

• Extract common flatten helper
• Migrate all 3 artifact-specific functions to helper
• Preserve existing logging and error semantics
• Add/adjust tests for each artifact variant
• Verify no behavior regression in logs and audit flows

Analysis Metadata

• Analyzed Files: 581 changed non-test Go files in latest commit scope; focused semantic review on high-churn pkg/cli paths
• Detection Method: Serena semantic analysis + structural pattern matching
• Commit: b049e6dcb91e6b00b8a251b6c74e52806e38e1cd
• Workflow Run: §23127574643
• Analysis Date: 2026-03-16 UTC

References:

• §23127574643

Generated by Duplicate Code Detector · ◷

network:
  allowed:
    - defaults
    - "ab.chatgpt.com"