From 51a638c88f391587cfe3465e90eb383d87f83d0d Mon Sep 17 00:00:00 2001 From: "tbhb.dev Agent" <334376894+tbhb-agent@users.noreply.github.com> Date: Mon, 2 Nov 2026 09:18:45 -0500 Subject: [PATCH] fix(retro): preserve artifact copying and report failure evidence Dangling extras must not count as copied evidence, and directories containing the destination must fail before copying their own output. Failed reports must keep exactly what the invocation emitted so users can inspect the output after an unsuccessful retrospective creation. Assisted-by: codex:gpt-6.1-sol Signed-off-by: Tony Burns --- internal/retro/artifact_parity_test.go | 191 +++++++++++++++++++++++++ internal/retro/fake_uv_test.go | 4 +- internal/retro/new.go | 62 ++++++-- 3 files changed, 243 insertions(+), 14 deletions(-) create mode 100644 internal/retro/artifact_parity_test.go 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 }