diff --git a/internal/retro/artifact_parity_test.go b/internal/retro/artifact_parity_test.go new file mode 100644 index 00000000..a79607f2 --- /dev/null +++ b/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")) +} diff --git a/internal/retro/fake_uv_test.go b/internal/retro/fake_uv_test.go index a4fecac6..2bd1ea5f 100644 --- a/internal/retro/fake_uv_test.go +++ b/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 != "" { diff --git a/internal/retro/new.go b/internal/retro/new.go index 62338e2b..c2869ed0 100644 --- a/internal/retro/new.go +++ b/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 }