fix(retro): restore artifact copying and report failure evidence

internal/retro/artifact_parity_test.go

@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Tony Burns
+
+package retro_test
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"io/fs"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+const artifactDir = ".claude/retros/20260102_session-claude-1/artifacts"
+
+func runArtifactEngine(t *testing.T, shell bool, skill string, args ...string) (int, string, string) {
+	t.Helper()
+	if !shell {
+		return run(t, skill, args...)
+	}
+	command := append([]string{filepath.Join(skill, "scripts/new-retro.sh")}, args...)
+	cmd := exec.CommandContext(t.Context(), "bash", command...)
+	var out, stderr bytes.Buffer
+	cmd.Stdout, cmd.Stderr = &out, &stderr
+	err := cmd.Run()
+	if err != nil {
+		exit, ok := errors.AsType[*exec.ExitError](err)
+		require.True(t, ok, "%v", err)
+		return exit.ExitCode(), out.String(), stderr.String()
+	}
+	return 0, out.String(), stderr.String()
+}
+
+func TestRetroDanglingExtra(t *testing.T) {
+	skill, transcript, _ := fixture(t)
+	t.Setenv("DEVTOOLS_HARNESS", "claude")
+	write(t, "extra.txt", "evidence")
+	write(t, "extra-dir/nested", "nested")
+	require.NoError(t, os.Symlink("..", "extra-dir/ancestor"))
+	require.NoError(t, os.Symlink("missing", "extra-dir/dangling"))
+	require.NoError(t, os.Symlink("extra.txt", "linked-file"))
+	require.NoError(t, os.Symlink("extra-dir", "linked-dir"))
+	require.NoError(t, os.Symlink("missing", "dangling"))
+	original := read(t, transcript)
+	var expectedOut, expectedErr string
+	for _, shell := range []bool{true, false} {
+		require.NoError(t, os.RemoveAll(".claude/retros"))
+		require.NoError(t, os.WriteFile(transcript, []byte(original), 0o600))
+		write(t, ".git/info/exclude", "")
+		code, out, stderr := runArtifactEngine(
+			t,
+			shell,
+			skill,
+			transcript,
+			"extra.txt",
+			"extra-dir",
+			"linked-file",
+			"linked-dir",
+			"missing",
+			"dangling",
+		)
+		require.Zero(t, code, stderr)
+		require.Contains(t, out, "4 canned reports, 4 extra")
+		require.Contains(t, stderr, "skipping missing artifact: missing")
+		require.Contains(t, stderr, "skipping missing artifact: dangling")
+		_, err := os.Lstat(filepath.Join(artifactDir, "dangling"))
+		require.True(t, os.IsNotExist(err))
+		require.NoFileExists(t, filepath.Join(artifactDir, "missing"))
+		require.Equal(t, "evidence", read(t, filepath.Join(artifactDir, "extra.txt")))
+		require.Equal(t, "nested", read(t, filepath.Join(artifactDir, "extra-dir/nested")))
+		for link, target := range map[string]string{"linked-file": "extra.txt", "linked-dir": "extra-dir", "extra-dir/ancestor": "..", "extra-dir/dangling": "missing"} {
+			got, linkErr := os.Readlink(filepath.Join(artifactDir, link))
+			require.NoError(t, linkErr)
+			require.Equal(t, target, got)
+		}
+		require.Equal(t, read(t, transcript), read(t, filepath.Join(artifactDir, "transcript.jsonl")))
+		for _, name := range []string{"index", "errors", "loops", "lint"} {
+			var expected bytes.Buffer
+			for i := range 40 {
+				fmt.Fprintln(&expected, i)
+			}
+			require.Equal(t, expected.String(), read(t, filepath.Join(artifactDir, name+".txt")))
+		}
+		if shell {
+			expectedOut, expectedErr = out, stderr
+		} else {
+			require.Equal(t, expectedOut, out)
+			require.Equal(t, expectedErr, stderr)
+		}
+		if shell {
+			require.NoError(t, os.Symlink(transcript, "session-claude-1.jsonl"))
+			transcript = "session-claude-1.jsonl"
+		}
+		require.NoError(t, os.WriteFile(transcript, []byte("changed"), 0o600))
+		require.NotEqual(t, read(t, transcript), read(t, filepath.Join(artifactDir, "transcript.jsonl")))
+	}
+}
+
+func TestRetroSelfContainingExtra(t *testing.T) {
+	for _, extra := range []string{".", "containing", "alias/containing"} {
+		t.Run(extra, func(t *testing.T) {
+			skill, transcript, _ := fixture(t)
+			t.Setenv("DEVTOOLS_HARNESS", "claude")
+			root, err := os.Getwd()
+			require.NoError(t, err)
+			if extra != "." {
+				git(t, "init", "--quiet", "--initial-branch=main", "containing")
+				require.NoError(t, os.Symlink(root, "alias"))
+				t.Chdir("containing")
+				extra = filepath.Join(root, extra)
+			}
+			code, out, stderr := run(t, skill, transcript, extra)
+			require.NotZero(t, code)
+			require.Empty(t, out)
+			require.Contains(t, stderr, "artifact cannot be copied into itself")
+			transcripts := 0
+			require.NoError(
+				t,
+				filepath.WalkDir(".claude/retros", func(path string, entry fs.DirEntry, err error) error {
+					require.NoError(t, err)
+					require.NotEqual(t, "RETRO.md", entry.Name(), path)
+					if entry.Name() == "transcript.jsonl" {
+						transcripts++
+					}
+					return nil
+				}),
+			)
+			require.Equal(t, 1, transcripts)
+		})
+	}
+}
+
+func TestRetroReportFailureEvidence(t *testing.T) {
+	names := []string{"index", "errors", "loops", "lint"}
+	for failed, name := range names {
+		for _, empty := range []bool{true, false} {
+			t.Run(name+map[bool]string{true: "/empty", false: "/partial"}[empty], func(t *testing.T) {
+				skill, transcript, _ := fixture(t)
+				t.Setenv("DEVTOOLS_HARNESS", "claude")
+				t.Setenv("FAIL_REPORT", name)
+				t.Setenv("FAIL_REPORT_EMPTY", map[bool]string{true: "1"}[empty])
+				for _, existing := range []bool{false, true} {
+					for _, shell := range []bool{true, false} {
+						checkReportFailure(t, shell, existing, empty, failed, skill, transcript)
+					}
+				}
+			})
+		}
+	}
+}
+
+func checkReportFailure(t *testing.T, shell, existing, empty bool, failed int, skill, transcript string) {
+	t.Helper()
+	names := []string{"index", "errors", "loops", "lint"}
+	require.NoError(t, os.RemoveAll(".claude/retros"))
+	if existing {
+		for _, report := range names {
+			write(t, filepath.Join(artifactDir, report+".txt"), "stale")
+		}
+	}
+	code, out, stderr := runArtifactEngine(t, shell, skill, transcript)
+	require.Equal(t, 9, code, stderr)
+	require.Empty(t, out)
+	for i, report := range names {
+		path := filepath.Join(artifactDir, report+".txt")
+		switch {
+		case i < failed:
+			var expected bytes.Buffer
+			for i := range 40 {
+				fmt.Fprintln(&expected, i)
+			}
+			require.Equal(t, expected.String(), read(t, path))
+		case i == failed:
+			want := "partial output\n"
+			if empty {
+				want = ""
+			}
+			require.Equal(t, want, read(t, path))
+		case existing:
+			require.Equal(t, "stale", read(t, path))
+		default:
+			require.NoFileExists(t, path)
+		}
+	}
+	require.NoFileExists(t, filepath.Join(filepath.Dir(artifactDir), "RETRO.md"))
+}

internal/retro/fake_uv_test.go

@@ -50,7 +50,9 @@ func fakeUV(args []string, stdout io.Writer) int {
 	}
 	report, transcript := args[3], args[6]
 	if report == os.Getenv("FAIL_REPORT") {
-		_, _ = fmt.Fprintln(stdout, "partial output")
+		if os.Getenv("FAIL_REPORT_EMPTY") != "1" {
+			_, _ = fmt.Fprintln(stdout, "partial output")
+		}
 		return 9
 	}
 	if override := os.Getenv("RETRO_FAKE_META"); report == "meta" && override != "" {

internal/retro/new.go

@@ -27,6 +27,7 @@ var (
 	errNoTemplate   = errors.New("new-retro: missing template")
 	errNoRepository = errors.New("new-retro: not inside a git repository")
 	errNoMetadata   = errors.New("new-retro: could not read session metadata")
+	errSelfCopy     = errors.New("new-retro: artifact cannot be copied into itself")
 	errBadFormat    = errors.New("new-retro: unsupported ANALYZE_SESSION_FORMAT")
 )
 
@@ -82,12 +83,13 @@ func create(ctx context.Context, skill string, args []string, stderr io.Writer)
 		return "", errNoRepository
 	}
 	commonDir := strings.TrimSpace(string(common))
-	metadata, err := report(ctx, skill, "meta", transcript, stderr)
+	var metadata bytes.Buffer
+	err = report(ctx, skill, "meta", transcript, &metadata, stderr)
 	if err != nil {
 		return "", err
 	}
 	values := make(map[string]string)
-	for line := range strings.SplitSeq(string(metadata), "\n") {
+	for line := range strings.SplitSeq(metadata.String(), "\n") {
 		key, value, _ := strings.Cut(line, "=")
 		values[key] = value
 	}
@@ -122,7 +124,7 @@ func create(ctx context.Context, skill string, args []string, stderr io.Writer)
 	), nil
 }
 
-func report(ctx context.Context, skill, name, transcript string, stderr io.Writer) ([]byte, error) {
+func report(ctx context.Context, skill, name, transcript string, stdout, stderr io.Writer) error {
 	format := "auto"
 	switch value := os.Getenv("ANALYZE_SESSION_FORMAT"); value {
 	case "", "auto":
@@ -131,16 +133,15 @@ func report(ctx context.Context, skill, name, transcript string, stderr io.Write
 	case codexHarness:
 		format = codexHarness
 	default:
-		return nil, fmt.Errorf("%w: %q", errBadFormat, value)
+		return fmt.Errorf("%w: %q", errBadFormat, value)
 	}
 	script := filepath.Join(skill, "scripts/session.py")
 	cmd := exec.CommandContext(ctx, "uv", "run", "--script", script, name, "--format", format, transcript)
-	cmd.Env, cmd.Stderr = environment(), stderr
-	data, err := cmd.Output()
-	if err != nil {
-		return data, fmt.Errorf("new-retro: run %s report: %w", name, err)
+	cmd.Env, cmd.Stdout, cmd.Stderr = environment(), stdout, stderr
+	if err := cmd.Run(); err != nil {
+		return fmt.Errorf("new-retro: run %s report: %w", name, err)
 	}
-	return data, nil
+	return nil
 }
 
 func environment() []string {
@@ -185,6 +186,9 @@ func copyArtifact(src, dst string, follow bool) error {
 		return copySymlink(src, dst)
 	}
 	if info.IsDir() {
+		if err = rejectSelfCopy(src, dst); err != nil {
+			return err
+		}
 		return copyDirectory(src, dst)
 	}
 	data, err := os.ReadFile(src)
@@ -211,6 +215,29 @@ func writeFile(path string, data []byte, perm os.FileMode) error {
 	return nil
 }
 
+func rejectSelfCopy(src, dst string) error {
+	source, err := filepath.Abs(src)
+	if err != nil {
+		return fmt.Errorf("new-retro: resolve artifact: %w", err)
+	}
+	source, err = filepath.EvalSymlinks(source)
+	if err != nil {
+		return fmt.Errorf("new-retro: resolve artifact: %w", err)
+	}
+	parent, err := filepath.EvalSymlinks(filepath.Dir(dst))
+	if err != nil {
+		return fmt.Errorf("new-retro: resolve destination: %w", err)
+	}
+	rel, err := filepath.Rel(source, filepath.Join(parent, filepath.Base(dst)))
+	if err != nil {
+		return fmt.Errorf("new-retro: compare artifact paths: %w", err)
+	}
+	if rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		return fmt.Errorf("%w: %s", errSelfCopy, src)
+	}
+	return nil
+}
+
 func copyDirectory(src, dst string) error {
 	if err := os.MkdirAll(dst, 0o700); err != nil {
 		return fmt.Errorf("new-retro: copy directory: %w", err)
@@ -296,17 +323,26 @@ func preserve(
 		return 0, err
 	}
 	for _, name := range []string{"index", "errors", "loops", "lint"} {
-		data, reportErr := report(ctx, skill, name, transcript, stderr)
+		file, err := os.OpenFile(
+			filepath.Join(artifacts, name+".txt"),
+			os.O_CREATE|os.O_WRONLY|os.O_TRUNC,
+			artifactPerm,
+		)
+		if err != nil {
+			return 0, fmt.Errorf("new-retro: write report: %w", err)
+		}
+		reportErr := report(ctx, skill, name, transcript, file, stderr)
+		closeErr := file.Close()
 		if reportErr != nil {
 			return 0, reportErr
 		}
-		if err := os.WriteFile(filepath.Join(artifacts, name+".txt"), data, 0o600); err != nil {
-			return 0, fmt.Errorf("new-retro: write report: %w", err)
+		if closeErr != nil {
+			return 0, fmt.Errorf("new-retro: close report: %w", closeErr)
 		}
 	}
 	copied := 0
 	for _, extra := range extras {
-		if _, statErr := os.Lstat(extra); statErr != nil {
+		if _, statErr := os.Stat(extra); statErr != nil {
 			_, _ = fmt.Fprintf(stderr, "new-retro: skipping missing artifact: %s\n", extra)
 			continue
 		}