diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b8d24221..43b25c02 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,7 +35,7 @@ jobs: - name: build and test run: | - go test -race -timeout=100s -covermode=atomic -coverprofile=$GITHUB_WORKSPACE/profile.cov_tmp ./... + go test -race -timeout=180s -covermode=atomic -coverprofile=$GITHUB_WORKSPACE/profile.cov_tmp ./... grep -v -E "_mock.go|/mocks/" $GITHUB_WORKSPACE/profile.cov_tmp > $GITHUB_WORKSPACE/profile.cov - name: golangci-lint diff --git a/Makefile b/Makefile index eb31b17f..db809f4e 100644 --- a/Makefile +++ b/Makefile @@ -26,7 +26,7 @@ fmt: ~/.claude/format.sh race: - go test -race -timeout=100s ./... + go test -race -timeout=180s ./... version: @echo "branch: $(BRANCH), hash: $(HASH), timestamp: $(TIMESTAMP)" diff --git a/app/config.go b/app/config.go index 0be1e47d..fc702625 100644 --- a/app/config.go +++ b/app/config.go @@ -209,8 +209,7 @@ func loadConfigFile(iniParser *flags.IniParser, configPath string) { if err == nil || errors.Is(err, os.ErrNotExist) { return } - var pathErr *os.PathError - if errors.As(err, &pathErr) { + if _, ok := errors.AsType[*os.PathError](err); ok { return // file access error (permission denied, etc.) } fmt.Fprintf(os.Stderr, "warning: config %s: %v\n", configPath, err) diff --git a/app/diff/diff.go b/app/diff/diff.go index 92e286de..ea96c003 100644 --- a/app/diff/diff.go +++ b/app/diff/diff.go @@ -734,8 +734,7 @@ func runVCSEnv(workDir string, env []string, binary string, args ...string) (str cmd.Env = env out, err := cmd.Output() if err != nil { - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { return "", fmt.Errorf("%s %s: %s", binary, strings.Join(args, " "), string(exitErr.Stderr)) } return "", fmt.Errorf("%s %s: %w", binary, strings.Join(args, " "), err) diff --git a/app/diff/directory.go b/app/diff/directory.go index eb664b8d..78a0fac3 100644 --- a/app/diff/directory.go +++ b/app/diff/directory.go @@ -60,8 +60,7 @@ func NewJjDirectoryReader(workDir string) *DirectoryReader { func (dr *DirectoryReader) ChangedFiles(_ string, _ bool) ([]FileEntry, error) { out, err := dr.listFiles() if err != nil { - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { stderr := strings.TrimSpace(string(exitErr.Stderr)) if stderr != "" { return nil, fmt.Errorf("%s: %s", dr.listSource, stderr) diff --git a/app/diff/fallback.go b/app/diff/fallback.go index 2754c174..43132ce2 100644 --- a/app/diff/fallback.go +++ b/app/diff/fallback.go @@ -254,8 +254,7 @@ func readFileAsContext(path string) ([]DiffLine, error) { lines, err := readReaderAsContext(f) if err != nil { - var ctxErr readerContextError - if errors.As(err, &ctxErr) { + if ctxErr, ok := errors.AsType[readerContextError](err); ok { return nil, fmt.Errorf("%s file %s: %w", ctxErr.op, path, ctxErr.err) } return nil, fmt.Errorf("read file %s: %w", path, err) diff --git a/app/main.go b/app/main.go index 79813381..8cc2ff65 100644 --- a/app/main.go +++ b/app/main.go @@ -9,6 +9,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" + "github.com/charmbracelet/x/term" "github.com/jessevdk/go-flags" "github.com/muesli/termenv" @@ -123,6 +124,22 @@ func run(opts options) (int, error) { ) programOptions := []tea.ProgramOption{tea.WithAltScreen(), tea.WithoutSignalHandler()} + tuiOut, err := (tuiOutput{ + stdout: os.Stdout, + isTerminal: term.IsTerminal, + openTTY: func() (*os.File, error) { + // stdin mode and Bubble Tea's default input fallback open a separate + // read handle; each side owns and closes its handle independently. + return os.OpenFile("/dev/tty", os.O_WRONLY, 0) + }, + }).open() + if err != nil { + return 0, err + } + if tuiOut != os.Stdout { + defer func() { _ = tuiOut.Close() }() + } + programOptions = append(programOptions, tea.WithOutput(tuiOut)) if !opts.NoMouse { programOptions = append(programOptions, tea.WithMouseCellMotion()) } @@ -292,6 +309,25 @@ func run(opts options) (int, error) { return 0, nil } +type tuiOutput struct { + stdout *os.File + isTerminal func(uintptr) bool + openTTY func() (*os.File, error) +} + +// open keeps Bubble Tea's display traffic out of redirected stdout, which is +// reserved for the final annotation stream. Terminal stdout is returned as-is. +func (r tuiOutput) open() (*os.File, error) { + if r.isTerminal(r.stdout.Fd()) { + return r.stdout, nil + } + tty, err := r.openTTY() + if err != nil { + return nil, fmt.Errorf("revdiff requires an interactive terminal for the TUI: %w", err) + } + return tty, nil +} + type finalizeReq struct { opts options annotations string diff --git a/app/main_test.go b/app/main_test.go index 47a3d8ab..a2f8a2a0 100644 --- a/app/main_test.go +++ b/app/main_test.go @@ -3,9 +3,14 @@ package main import ( "bytes" "errors" + "fmt" + "io" "os" + "os/exec" "path/filepath" + "runtime" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -15,6 +20,22 @@ type errWriter struct{} func (errWriter) Write([]byte) (int, error) { return 0, errors.New("write failed") } +type markerWriter struct { + bytes.Buffer + marker string + ready chan struct{} + found bool +} + +func (w *markerWriter) Write(p []byte) (int, error) { + n, _ := w.Buffer.Write(p) + if !w.found && bytes.Contains(w.Bytes(), []byte(w.marker)) { + w.found = true + close(w.ready) + } + return n, nil +} + func TestAnnotationExitCode(t *testing.T) { tests := []struct { name string @@ -181,3 +202,281 @@ func TestWriteAnnotationOutput(t *testing.T) { assert.Empty(t, buf.String()) }) } + +func TestTUIOutput_Open(t *testing.T) { + stdout, err := os.CreateTemp(t.TempDir(), "stdout-*") + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, stdout.Close()) }) + + tty, err := os.CreateTemp(t.TempDir(), "tty-*") + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, tty.Close()) }) + + tests := []struct { + name string + terminal bool + openErr error + wantOutput *os.File + wantOpened bool + wantErr bool + }{ + {name: "terminal stdout remains TUI output", terminal: true, wantOutput: stdout}, + {name: "redirected stdout uses tty", terminal: false, wantOutput: tty, wantOpened: true}, + {name: "redirected stdout requires tty", terminal: false, openErr: errors.New("no tty"), wantOpened: true, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opened := false + req := tuiOutput{ + stdout: stdout, + isTerminal: func(uintptr) bool { return tt.terminal }, + openTTY: func() (*os.File, error) { + opened = true + return tty, tt.openErr + }, + } + + got, openErr := req.open() + if tt.wantErr { + require.Error(t, openErr) + require.ErrorContains(t, openErr, "revdiff requires an interactive terminal") + } else { + require.NoError(t, openErr) + } + assert.Same(t, tt.wantOutput, got) + assert.Equal(t, tt.wantOpened, opened) + }) + } +} + +func TestRun_RedirectedStdoutUsesTTY(t *testing.T) { + if os.Getenv("REVDIFF_TUI_TEST_HELPER") == "1" { + opts, err := parseArgs([]string{ + "--config=" + os.Getenv("REVDIFF_TUI_TEST_CONFIG"), + "--annotations=" + os.Getenv("REVDIFF_TUI_TEST_ANNOTATIONS"), + "--history-dir=" + os.Getenv("REVDIFF_TUI_TEST_HISTORY"), + "--no-colors", + "--no-mouse", + }) + if err != nil { + _, _ = fmt.Fprintf(os.Stderr, "parse helper args: %v\n", err) + os.Exit(1) + } + code, runErr := run(opts) + if runErr != nil { + _, _ = fmt.Fprintf(os.Stderr, "run helper: %v\n", runErr) + os.Exit(1) + } + os.Exit(code) + } + + if runtime.GOOS != "darwin" && runtime.GOOS != "linux" { + t.Skip("PTY integration is supported on darwin and linux") + } + scriptBin, err := exec.LookPath("script") + if err != nil { + t.Skip("script utility is unavailable") + } + mkfifoBin, err := exec.LookPath("mkfifo") + if err != nil { + t.Skip("mkfifo utility is unavailable") + } + probePath := filepath.Join(t.TempDir(), "probe-tty.sh") + probe := "#!/bin/sh\n( : > /dev/tty ) 2>/dev/null || exit 77\n" + require.NoError(t, os.WriteFile(probePath, []byte(probe), 0o600)) + require.NoError(t, os.Chmod(probePath, 0o700)) //nolint:gosec // executable test fixture + probeArgs := []string{"-q", "/dev/null", probePath} + if runtime.GOOS == "linux" { + probeArgs = []string{"-q", "-e", "-c", probePath, "/dev/null"} + } + probeCmd := exec.Command(scriptBin, probeArgs...) //nolint:gosec // fixed utility with test-controlled arguments + probeCmd.Stdout, probeCmd.Stderr = io.Discard, io.Discard + if probeErr := probeCmd.Run(); probeErr != nil { + if probeCmd.ProcessState != nil && probeCmd.ProcessState.ExitCode() == 77 { + t.Skip("sandbox denies access to the test PTY") + } + require.NoError(t, probeErr) + } + + repo := t.TempDir() + runMainTestGit(t, repo, "init", "-q") + runMainTestGit(t, repo, "config", "user.email", "test@example.com") + runMainTestGit(t, repo, "config", "user.name", "test") + require.NoError(t, os.WriteFile(filepath.Join(repo, "a.txt"), []byte("one\ntwo\n"), 0o600)) + runMainTestGit(t, repo, "add", "a.txt") + runMainTestGit(t, repo, "commit", "-qm", "initial") + require.NoError(t, os.WriteFile(filepath.Join(repo, "a.txt"), []byte("one\nchanged\n"), 0o600)) + + const annotations = "## a.txt:2 (+)\npipe output stays clean\n" + notesPath := filepath.Join(t.TempDir(), "annotations.md") + require.NoError(t, os.WriteFile(notesPath, []byte(annotations), 0o600)) + capturePath := filepath.Join(t.TempDir(), "stdout.txt") + wrapperPath := filepath.Join(t.TempDir(), "run-revdiff.sh") + wrapper := "#!/bin/sh\n\"$REVDIFF_TEST_BINARY\" -test.run '^TestRun_RedirectedStdoutUsesTTY$' | cat > \"$REVDIFF_TEST_CAPTURE\"\n" + require.NoError(t, os.WriteFile(wrapperPath, []byte(wrapper), 0o600)) + require.NoError(t, os.Chmod(wrapperPath, 0o700)) //nolint:gosec // executable test fixture + transcriptPath := filepath.Join(t.TempDir(), "terminal.fifo") + require.NoError(t, exec.Command(mkfifoBin, transcriptPath).Run()) //nolint:gosec // fixed utility with test-controlled path + + args := []string{"-q", "-F", transcriptPath, wrapperPath} + if runtime.GOOS == "linux" { + args = []string{"-q", "-e", "-f", "-c", wrapperPath, transcriptPath} + } + cmd := exec.Command(scriptBin, args...) //nolint:gosec // fixed utility with test-controlled arguments + cmd.Dir = repo + cmd.Env = mergeEnv(map[string]string{ + "REVDIFF_TUI_TEST_HELPER": "1", + "REVDIFF_TUI_TEST_CONFIG": filepath.Join(t.TempDir(), "missing-config"), + "REVDIFF_TUI_TEST_ANNOTATIONS": notesPath, + "REVDIFF_TUI_TEST_HISTORY": filepath.Join(t.TempDir(), "history"), + "REVDIFF_TEST_BINARY": os.Args[0], + "REVDIFF_TEST_CAPTURE": capturePath, + }) + stdin, err := cmd.StdinPipe() + require.NoError(t, err) + terminal := &markerWriter{marker: "\x1b[?1049h", ready: make(chan struct{})} + var stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = io.Discard, &stderr + require.NoError(t, cmd.Start()) + waitDone := make(chan error, 1) + go func() { waitDone <- cmd.Wait() }() + proc := ptyTestProcess{cmd: cmd, waitDone: waitDone, stderr: &stderr} + transcript := proc.openFIFO(t, transcriptPath) + readDone := make(chan error, 1) + go func() { + // hide bytes.Buffer.ReadFrom so io.Copy calls markerWriter.Write and + // closes ready when the terminal marker arrives. + _, copyErr := io.Copy(struct{ io.Writer }{terminal}, transcript) + readDone <- copyErr + }() + + select { + case <-terminal.ready: + case waitErr := <-waitDone: + readErr := <-readDone + _ = transcript.Close() + t.Fatalf("script exited before TUI output\nstdout: %q\nstderr: %s\nwait: %v\nread: %v", + terminal.String(), stderr.String(), waitErr, readErr) + case <-time.After(5 * time.Second): + _, _ = io.WriteString(stdin, "q") + _ = stdin.Close() + waitErr := <-waitDone + readErr := <-readDone + _ = transcript.Close() + t.Fatalf("TUI never rendered on the terminal\nstdout: %q\nstderr: %s\nwait: %v\nread: %v", + terminal.String(), stderr.String(), waitErr, readErr) + } + _, err = io.WriteString(stdin, "q") + require.NoError(t, err) + require.NoError(t, stdin.Close()) + waitErr := <-waitDone + readErr := <-readDone + require.NoError(t, transcript.Close()) + require.NoError(t, waitErr, "terminal: %q\nstderr: %s", terminal.String(), stderr.String()) + require.NoError(t, readErr) + + captured, err := os.ReadFile(capturePath) //nolint:gosec // test-owned path + require.NoError(t, err) + assert.Equal(t, annotations, string(captured)) + assert.NotContains(t, string(captured), "\x1b[") +} + +func TestHoldMainTestFIFOWriter_UnblocksLateReader(t *testing.T) { + if runtime.GOOS != "darwin" && runtime.GOOS != "linux" { + t.Skip("FIFO test is supported on darwin and linux") + } + mkfifoBin, err := exec.LookPath("mkfifo") + if err != nil { + t.Skip("mkfifo utility is unavailable") + } + path := filepath.Join(t.TempDir(), "late-reader.fifo") + require.NoError(t, exec.Command(mkfifoBin, path).Run()) //nolint:gosec // fixed utility with test-controlled path + + writer, err := holdMainTestFIFOWriter(path) + require.NoError(t, err) + readerDone := make(chan *os.File, 1) + go func() { + reader, _ := os.Open(path) //nolint:gosec // test-owned FIFO + readerDone <- reader + }() + + select { + case reader := <-readerDone: + require.NotNil(t, reader) + require.NoError(t, reader.Close()) + case <-time.After(time.Second): + t.Fatal("late FIFO reader stayed blocked after writer helper returned") + } + require.NoError(t, writer.Close()) +} + +func runMainTestGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) //nolint:gosec // test-controlled arguments + cmd.Dir = dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "%s", out) +} + +type ptyTestProcess struct { + cmd *exec.Cmd + waitDone chan error + stderr *bytes.Buffer +} + +func (p ptyTestProcess) openFIFO(t *testing.T, path string) *os.File { + t.Helper() + type openResult struct { + file *os.File + err error + } + openDone := make(chan openResult, 1) + go func() { + file, err := os.Open(path) //nolint:gosec // test-owned FIFO + openDone <- openResult{file: file, err: err} + }() + + select { + case result := <-openDone: + if result.err != nil { + _ = p.cmd.Process.Kill() + waitErr := <-p.waitDone + t.Fatalf("open terminal transcript: %v; wait: %v; stderr: %s", result.err, waitErr, p.stderr.String()) + } + return result.file + case waitErr := <-p.waitDone: + writer, writerErr := holdMainTestFIFOWriter(path) + if writerErr != nil { + t.Fatalf("open FIFO writer after script exit: %v; wait: %v; stderr: %s", writerErr, waitErr, p.stderr.String()) + } + result := <-openDone + _ = writer.Close() + if result.file != nil { + _ = result.file.Close() + } + t.Fatalf("script exited before opening the terminal transcript: %v; stderr: %s", waitErr, p.stderr.String()) + case <-time.After(5 * time.Second): + _ = p.cmd.Process.Kill() + waitErr := <-p.waitDone + writer, writerErr := holdMainTestFIFOWriter(path) + if writerErr != nil { + t.Fatalf("open FIFO writer after timeout: %v; wait: %v; stderr: %s", writerErr, waitErr, p.stderr.String()) + } + result := <-openDone + _ = writer.Close() + if result.file != nil { + _ = result.file.Close() + } + t.Fatalf("timed out opening terminal transcript; wait: %v; stderr: %s", waitErr, p.stderr.String()) + } + return nil +} + +func holdMainTestFIFOWriter(path string) (*os.File, error) { + file, err := os.OpenFile(path, os.O_RDWR, 0) //nolint:gosec // test-owned FIFO + if err != nil { + return nil, fmt.Errorf("open FIFO read-write: %w", err) + } + return file, nil +} diff --git a/app/plugin_exit_code_test.go b/app/plugin_exit_code_test.go index 576c7dc4..853af38c 100644 --- a/app/plugin_exit_code_test.go +++ b/app/plugin_exit_code_test.go @@ -1250,8 +1250,7 @@ func commandExitCode(err error) int { if err == nil { return 0 } - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { return exitErr.ExitCode() } return 1 diff --git a/docs/backlog/post-flush-clipboard-route-undocumented.md b/docs/backlog/post-flush-clipboard-route-undocumented.md new file mode 100644 index 00000000..33a3596d --- /dev/null +++ b/docs/backlog/post-flush-clipboard-route-undocumented.md @@ -0,0 +1,22 @@ +--- +worth: later +where: README.md:781 +added: 2026-08-28 +--- +# post-flush command clipboard route is undocumented + +Setting `post-flush-command = pbcopy` in the config file and exporting `REVDIFF_OUTPUT` in the shell +profile makes every `O` flush copy the annotations to the clipboard in every session, with no per-launch +flags. This is the direct answer to a common ask, but it appears nowhere in the documentation. + +`Output` has `no-ini:"true"` in app/config.go, so the config file cannot provide the output path required +by `O`. Users must either export `REVDIFF_OUTPUT` once or pass `--output` on each launch. Whether +`--output` should become config-settable is a separate product decision. + +README.md:781 teaches only the hand-rolled OSC 52 shell-script recipe. `pbcopy` appears in +site/docs.html only inside the Zed section, where it is attached to quit-time output rather than to `O`. + +Add the same four-line explanation to README.md, site/docs.html, +.claude-plugin/skills/revdiff/references/config.md, and +.claude-plugin/skills/revdiff/references/usage.md. Surfaced while investigating issue #336, which is +otherwise being answered rather than implemented. diff --git a/docs/backlog/race-timeout-budget-too-tight.md b/docs/backlog/race-timeout-budget-too-tight.md deleted file mode 100644 index eef10386..00000000 --- a/docs/backlog/race-timeout-budget-too-tight.md +++ /dev/null @@ -1,25 +0,0 @@ ---- -worth: later -where: Makefile:29 -added: 2026-08-19 ---- -# make race timeout leaves too little headroom for the launcher matrix - -`make race` runs with `-timeout=100s` and the `app` package alone takes 68s of that on an idle M1 Ultra, -almost all of it `TestShellLaunchersPreserveAnnotationExitCode` shelling out through the launcher-backend -matrix (59s for that test by itself). Under any concurrent load the package crosses 100s and the run fails -with a panic naming that test, which reads as a test defect rather than as the budget being spent. - -Surfaced while reviewing PR #321 with six parallel review agents running: the same test timed out in the -review worktree, passed in 192s standalone under that load, and was green at 68s once the machine was idle. -Nothing about the PR was involved. CI is green and so is an idle local run, which is why this is `later` -rather than `yes`. - -Re-measured during the PR #327 review (2026-08-19), and the margin has shrunk: the `app` package now runs -81.5s and 87.5s on master and 79.9s on that PR's branch, against 68s before, with -`TestShellLaunchersPreserveAnnotationExitCode` alone at ~77s. Headroom against the 100s budget is down from -roughly 32s to roughly 13s, so an idle run is no longer comfortably clear of it. Still `later`, but the next -addition to the launcher matrix is what turns this into `yes`. - -Fix is a choice, not a one-liner: raise the timeout, or split the launcher matrix into its own target with -its own budget so the ordinary race run stays fast and the slow matrix is allowed to be slow. diff --git a/go.mod b/go.mod index 83a2e3a3..f8152f83 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 github.com/charmbracelet/x/ansi v0.11.8 + github.com/charmbracelet/x/term v0.2.2 github.com/dlclark/regexp2/v2 v2.7.1 github.com/jessevdk/go-flags v1.6.1 github.com/mattn/go-runewidth v0.0.28 @@ -20,7 +21,6 @@ require ( github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/charmbracelet/colorprofile v0.4.3 // indirect github.com/charmbracelet/x/cellbuf v0.0.15 // indirect - github.com/charmbracelet/x/term v0.2.2 // indirect github.com/clipperhouse/displaywidth v0.11.0 // indirect github.com/clipperhouse/uax29/v2 v2.7.0 // indirect github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect