From ca4914ca5614edf48c8b08506f3660583feeb346 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 08:45:04 +0000 Subject: [PATCH 01/22] fix(session): harden runtime control paths --- internal/session/attach.go | 16 ++-- internal/session/client.go | 13 ++- internal/session/host.go | 8 +- internal/session/liveness_test.go | 151 ++++++++++++++++++++++++++++++ internal/session/proto.go | 38 +++++++- internal/session/proto_test.go | 86 +++++++++++++++++ internal/session/session.go | 78 +++++++++++++-- 7 files changed, 367 insertions(+), 23 deletions(-) create mode 100644 internal/session/proto_test.go diff --git a/internal/session/attach.go b/internal/session/attach.go index 91819bd..d3da306 100644 --- a/internal/session/attach.go +++ b/internal/session/attach.go @@ -90,6 +90,9 @@ func runAttachWithOptions(dir, name string, stdin *os.File, stdout *os.File, opt if err := ValidateName(name); err != nil { return err } + if err := VerifyDir(dir); err != nil { + return err + } conn, err := net.Dial("unix", SocketPath(dir, name)) if err != nil { return fmt.Errorf("session %s is not running: %w", name, err) @@ -111,6 +114,7 @@ func runAttachWithOptions(dir, name string, stdin *os.File, stdout *os.File, opt if !resp.OK { return fmt.Errorf("attach %s: %s", name, resp.Err) } + frames := newFrameWriter(conn) var ttyState *term.State if term.IsTerminal(stdin.Fd()) { @@ -151,7 +155,7 @@ func runAttachWithOptions(dir, name string, stdin *os.File, stdout *os.File, opt go func() { for range winch { if w, h, err := term.GetSize(stdout.Fd()); err == nil { - _ = writeFrame(conn, frameResize, resizePayload(w, h)) + _ = frames.WriteFrame(frameResize, resizePayload(w, h)) } } }() @@ -160,7 +164,7 @@ func runAttachWithOptions(dir, name string, stdin *os.File, stdout *os.File, opt // cannot be interrupted; when the session ends the process exits anyway. detached := make(chan struct{}) go func() { - pumpStdin(stdin, conn, backDetachEnabled()) + pumpStdin(stdin, frames, backDetachEnabled()) close(detached) }() @@ -176,12 +180,12 @@ func runAttachWithOptions(dir, name string, stdin *os.File, stdout *os.File, opt var note string select { case <-detached: - _ = writeFrame(conn, frameDetach, nil) + _ = frames.WriteFrame(frameDetach, nil) note = "detached" case <-done: note = "session ended" case <-quit: - _ = writeFrame(conn, frameDetach, nil) + _ = frames.WriteFrame(frameDetach, nil) note = "detached" } // Stop the host→terminal pump and drain it before restoring the screen: @@ -273,7 +277,7 @@ const maxStrLen = 4096 // pumpStdin forwards terminal input to the host, filtering the detach chord, // Ctrl+Z, and (when enabled) the left-arrow quick detach. It returns when the // user detaches or stdin/conn fails. -func pumpStdin(stdin io.Reader, conn net.Conn, backDetach bool) { +func pumpStdin(stdin io.Reader, frames *frameWriter, backDetach bool) { f := &stdinFilter{backDetach: backDetach} buf := make([]byte, 4096) for { @@ -281,7 +285,7 @@ func pumpStdin(stdin io.Reader, conn net.Conn, backDetach bool) { if n > 0 { out, detach := f.filter(buf[:n]) if len(out) > 0 { - if werr := writeFrame(conn, frameStdin, out); werr != nil { + if werr := frames.WriteFrame(frameStdin, out); werr != nil { return } } diff --git a/internal/session/client.go b/internal/session/client.go index ef76d95..d96b4c9 100644 --- a/internal/session/client.go +++ b/internal/session/client.go @@ -153,6 +153,12 @@ func sortedKeys(m map[string]string) []string { // files — no subprocess, no socket round-trips. Leftovers from a crashed host // are swept once both the host and its agent are gone. func (c *Client) List(_ context.Context) ([]Info, error) { + if err := VerifyDir(c.Dir); err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + return nil, err + } entries, err := os.ReadDir(c.Dir) if err != nil { if errors.Is(err, os.ErrNotExist) { @@ -179,7 +185,7 @@ func (c *Client) List(_ context.Context) ([]Info, error) { out = append(out, infoFromState(st)) continue } - removeSessionFiles(c.Dir, name) + _ = removeSessionFiles(c.Dir, name) continue } out = append(out, infoFromState(st)) @@ -261,7 +267,7 @@ func (c *Client) Kill(ctx context.Context, name string) error { deadline := time.Now().Add(callTimeout) for time.Now().Before(deadline) { if !st.childAlive() && !st.hostAlive() { - removeSessionFiles(c.Dir, name) + _ = removeSessionFiles(c.Dir, name) return nil } select { @@ -318,6 +324,9 @@ func (c *Client) roundTrip(ctx context.Context, name string, req request) (respo return response{}, err } name = strings.TrimPrefix(name, "=") + if err := VerifyDir(c.Dir); err != nil { + return response{}, err + } ctx, cancel := context.WithTimeout(ctx, callTimeout) defer cancel() var d net.Dialer diff --git a/internal/session/host.go b/internal/session/host.go index 00384e1..4bc2481 100644 --- a/internal/session/host.go +++ b/internal/session/host.go @@ -131,7 +131,9 @@ func runHost(dir, name, cwd, label string, envs, command []string, ready *os.Fil return fmt.Errorf("session %s already exists (host pid %d)", name, st.HostPID) } // Stale leftovers from a crashed host: safe to clear, the pid is gone. - removeSessionFiles(dir, name) + if err := removeSessionFiles(dir, name); err != nil { + return fmt.Errorf("remove stale session files: %w", err) + } ln, err := net.Listen("unix", SocketPath(dir, name)) if err != nil { @@ -519,7 +521,9 @@ func (h *host) shutdown(exitCode int) { cl.drop() } h.mu.Unlock() - removeSessionFiles(h.dir, h.name) + if err := removeSessionFiles(h.dir, h.name); err != nil { + log.Warn("remove session files failed", "session", h.name, "error", err) + } } func (h *host) markClosed(exitCode int) { diff --git a/internal/session/liveness_test.go b/internal/session/liveness_test.go index 516c7d3..353ffd1 100644 --- a/internal/session/liveness_test.go +++ b/internal/session/liveness_test.go @@ -1,6 +1,8 @@ package session import ( + "context" + "errors" "os" "os/exec" "path/filepath" @@ -10,6 +12,155 @@ import ( "testing" ) +func TestVerifyDirRejectsUnsafeRuntimeDirectories(t *testing.T) { + parent := t.TempDir() + + missing := filepath.Join(parent, "missing") + if err := VerifyDir(missing); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("VerifyDir missing error = %v, want os.ErrNotExist", err) + } + + permissive := filepath.Join(parent, "permissive") + if err := os.Mkdir(permissive, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(permissive, 0o755); err != nil { + t.Fatal(err) + } + if err := VerifyDir(permissive); err == nil { + t.Fatal("VerifyDir must reject a group/world-accessible runtime directory") + } + info, err := os.Stat(permissive) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o755 { + t.Fatalf("VerifyDir mutated mode to %o, want read-only verification", got) + } + + target := filepath.Join(parent, "target") + if err := os.Mkdir(target, 0o700); err != nil { + t.Fatal(err) + } + link := filepath.Join(parent, "link") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + if err := VerifyDir(link); err == nil { + t.Fatal("VerifyDir must reject a symlink runtime directory") + } +} + +func TestListMissingDirIsEmptyButUnsafeDirFailsClosed(t *testing.T) { + c := &Client{Dir: filepath.Join(t.TempDir(), "missing")} + infos, err := c.List(context.Background()) + if err != nil || len(infos) != 0 { + t.Fatalf("List missing dir = (%v, %v), want empty success", infos, err) + } + + if err := os.Mkdir(c.Dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(c.Dir, 0o755); err != nil { + t.Fatal(err) + } + if _, err := c.List(context.Background()); err == nil { + t.Fatal("List must reject an unsafe existing runtime directory") + } + info, err := os.Stat(c.Dir) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o755 { + t.Fatalf("List repaired unsafe mode to %o instead of failing closed", got) + } +} + +func TestStateReadRejectsMismatchedNameAndNonRegularFile(t *testing.T) { + dir := filepath.Join(t.TempDir(), "runtime") + if err := EnsureDir(dir); err != nil { + t.Fatal(err) + } + requested := "uam-fake-aabbccdd" + mismatch := []byte(`{"name":"uam-fake-deadbeef","host_pid":1}`) + if err := os.WriteFile(statePath(dir, requested), mismatch, 0o600); err != nil { + t.Fatal(err) + } + if _, err := readState(dir, requested); err == nil { + t.Fatal("readState must reject a state whose embedded name differs from its filename") + } + + if err := os.Remove(statePath(dir, requested)); err != nil { + t.Fatal(err) + } + target := filepath.Join(t.TempDir(), "state.json") + if err := os.WriteFile(target, []byte(`{"name":"`+requested+`"}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, statePath(dir, requested)); err != nil { + t.Fatal(err) + } + if _, err := readState(dir, requested); err == nil { + t.Fatal("readState must reject symlink state files") + } +} + +func TestControlPathsRejectUnsafeRuntimeDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "runtime") + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(dir, 0o755); err != nil { + t.Fatal(err) + } + c := &Client{Dir: dir} + if _, err := c.Capture(context.Background(), "uam-fake-aabbccdd", 1); err == nil || !strings.Contains(err.Error(), "unsafe mode") { + t.Fatalf("Capture error = %v, want unsafe directory rejection before dialing", err) + } + if c.HasSession(context.Background(), "uam-fake-aabbccdd") { + t.Fatal("HasSession must not trust state in an unsafe runtime directory") + } + stdin, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + defer func() { _ = stdin.Close() }() + stdout, err := os.CreateTemp(t.TempDir(), "attach-output") + if err != nil { + t.Fatal(err) + } + defer func() { _ = stdout.Close() }() + if err := runAttach(dir, "uam-fake-aabbccdd", stdin, stdout); err == nil || !strings.Contains(err.Error(), "unsafe mode") { + t.Fatalf("runAttach error = %v, want unsafe directory rejection before dialing", err) + } +} + +func TestRemoveSessionFilesRejectsUnsafeRuntimeDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "runtime") + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatal(err) + } + name := "uam-fake-aabbccdd" + state := statePath(dir, name) + socket := SocketPath(dir, name) + for _, path := range []string{state, socket} { + if err := os.WriteFile(path, []byte("keep"), 0o600); err != nil { + t.Fatal(err) + } + } + if err := os.Chmod(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := removeSessionFiles(dir, name); err == nil { + t.Fatal("removeSessionFiles must reject an unsafe runtime directory") + } + for _, path := range []string{state, socket} { + if _, err := os.Stat(path); err != nil { + t.Fatalf("unsafe cleanup removed %s: %v", path, err) + } + } +} + func TestDefaultDirResolution(t *testing.T) { t.Setenv("UAM_SESSION_DIR", "/custom/dir") if got := DefaultDir(); got != "/custom/dir" { diff --git a/internal/session/proto.go b/internal/session/proto.go index 7ca3639..080a7c7 100644 --- a/internal/session/proto.go +++ b/internal/session/proto.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "sync" ) // Wire protocol between the client (uam TUI/CLI) and a session host. @@ -72,14 +73,45 @@ func readJSONLine(r *bufio.Reader, v any) error { func writeFrame(w io.Writer, kind byte, payload []byte) error { hdr := [5]byte{kind} binary.BigEndian.PutUint32(hdr[1:], uint32(len(payload))) // #nosec G115 -- payload length is bounded by callers - if _, err := w.Write(hdr[:]); err != nil { + if err := writeAll(w, hdr[:]); err != nil { return err } if len(payload) == 0 { return nil } - _, err := w.Write(payload) - return err + return writeAll(w, payload) +} + +func writeAll(w io.Writer, data []byte) error { + for len(data) > 0 { + n, err := w.Write(data) + if n > 0 { + data = data[n:] + } + if err != nil { + return err + } + if n == 0 { + return io.ErrShortWrite + } + } + return nil +} + +// frameWriter serializes complete attach frames. A frame is two physical +// writes (header then payload), so protecting individual net.Conn.Write calls +// is insufficient when stdin, resize, and detach goroutines share a socket. +type frameWriter struct { + mu sync.Mutex + w io.Writer +} + +func newFrameWriter(w io.Writer) *frameWriter { return &frameWriter{w: w} } + +func (w *frameWriter) WriteFrame(kind byte, payload []byte) error { + w.mu.Lock() + defer w.mu.Unlock() + return writeFrame(w.w, kind, payload) } func readFrame(r io.Reader) (byte, []byte, error) { diff --git a/internal/session/proto_test.go b/internal/session/proto_test.go new file mode 100644 index 0000000..87f9ac8 --- /dev/null +++ b/internal/session/proto_test.go @@ -0,0 +1,86 @@ +package session + +import ( + "bytes" + "errors" + "io" + "sync" + "testing" +) + +type shortWriter struct { + mu sync.Mutex + buf bytes.Buffer + max int +} + +func (w *shortWriter) Write(p []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + if len(p) > w.max { + p = p[:w.max] + } + return w.buf.Write(p) +} + +func TestWriteFrameHandlesShortWrites(t *testing.T) { + w := &shortWriter{max: 2} + if err := writeFrame(w, frameStdin, []byte("payload")); err != nil { + t.Fatalf("writeFrame: %v", err) + } + kind, payload, err := readFrame(bytes.NewReader(w.buf.Bytes())) + if err != nil { + t.Fatalf("readFrame: %v", err) + } + if kind != frameStdin || string(payload) != "payload" { + t.Fatalf("frame = (%d, %q), want stdin payload", kind, payload) + } +} + +type zeroWriter struct{} + +func (zeroWriter) Write([]byte) (int, error) { return 0, nil } + +func TestWriteFrameRejectsNoProgressWriter(t *testing.T) { + if err := writeFrame(zeroWriter{}, frameDetach, nil); !errors.Is(err, io.ErrShortWrite) { + t.Fatalf("writeFrame error = %v, want io.ErrShortWrite", err) + } +} + +func TestFrameWriterSerializesConcurrentFrames(t *testing.T) { + underlying := &shortWriter{max: 1} + w := newFrameWriter(underlying) + const frames = 100 + + var wg sync.WaitGroup + for i := 0; i < frames; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + payload := bytes.Repeat([]byte{byte(i)}, 32) + if err := w.WriteFrame(frameStdin, payload); err != nil { + t.Errorf("WriteFrame(%d): %v", i, err) + } + }(i) + } + wg.Wait() + + r := bytes.NewReader(underlying.buf.Bytes()) + for i := 0; i < frames; i++ { + kind, payload, err := readFrame(r) + if err != nil { + t.Fatalf("read frame %d: %v", i, err) + } + if kind != frameStdin || len(payload) != 32 { + t.Fatalf("frame %d = kind %d len %d", i, kind, len(payload)) + } + for _, b := range payload[1:] { + if b != payload[0] { + t.Fatalf("frame %d contains interleaved payload: %v", i, payload) + } + } + } + if _, _, err := readFrame(r); !errors.Is(err, io.EOF) { + t.Fatalf("trailing read error = %v, want EOF", err) + } +} diff --git a/internal/session/session.go b/internal/session/session.go index 117d9cf..c4f08c4 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -65,6 +65,37 @@ func EnsureDir(dir string) error { if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("create session dir %s: %w", dir, err) } + if err := verifyDirIdentity(dir); err != nil { + return err + } + // MkdirAll is a no-op on an existing directory. Creation paths retain the + // historical repair behavior for a directory owned by this user; read and + // control paths use VerifyDir and fail closed instead. + if err := os.Chmod(dir, 0o700); err != nil { // #nosec G302 -- directory needs the execute bit; owner-only. + return fmt.Errorf("restrict session dir %s: %w", dir, err) + } + return VerifyDir(dir) +} + +// VerifyDir validates an existing runtime directory without changing it. +// The directory is the local authorization boundary around session sockets +// and state, so every read/control path must call this before trusting files +// beneath it. +func VerifyDir(dir string) error { + if err := verifyDirIdentity(dir); err != nil { + return err + } + info, err := os.Lstat(dir) + if err != nil { + return fmt.Errorf("stat session dir %s: %w", dir, err) + } + if info.Mode().Perm() != 0o700 { + return fmt.Errorf("session dir %s has unsafe mode %04o; want 0700", dir, info.Mode().Perm()) + } + return nil +} + +func verifyDirIdentity(dir string) error { info, err := os.Lstat(dir) if err != nil { return fmt.Errorf("stat session dir %s: %w", dir, err) @@ -75,12 +106,6 @@ func EnsureDir(dir string) error { if st, ok := info.Sys().(*syscall.Stat_t); ok && int(st.Uid) != os.Getuid() { return fmt.Errorf("session dir %s is owned by uid %d, not the current user", dir, st.Uid) } - // MkdirAll is a no-op on an existing directory; re-assert the mode so a - // pre-existing world-readable dir cannot silently expose sockets. 0700 is - // required (not 0600): the owner must traverse the directory. - if err := os.Chmod(dir, 0o700); err != nil { // #nosec G302 -- directory needs the execute bit; owner-only. - return fmt.Errorf("restrict session dir %s: %w", dir, err) - } return nil } @@ -129,6 +154,12 @@ func statePath(dir, name string) string { return filepath.Join(dir, name+".json" func SocketPath(dir, name string) string { return filepath.Join(dir, name+".sock") } func writeState(dir string, st State) error { + if err := VerifyDir(dir); err != nil { + return err + } + if err := ValidateName(st.Name); err != nil { + return err + } data, err := json.Marshal(st) if err != nil { return err @@ -142,7 +173,21 @@ func writeState(dir string, st State) error { } func readState(dir, name string) (State, error) { - data, err := os.ReadFile(statePath(dir, name)) + if err := ValidateName(name); err != nil { + return State{}, err + } + if err := VerifyDir(dir); err != nil { + return State{}, err + } + path := statePath(dir, name) + info, err := os.Lstat(path) + if err != nil { + return State{}, err + } + if !info.Mode().IsRegular() { + return State{}, fmt.Errorf("session state %s is not a regular file", name) + } + data, err := os.ReadFile(path) if err != nil { return State{}, err } @@ -150,12 +195,25 @@ func readState(dir, name string) (State, error) { if err := json.Unmarshal(data, &st); err != nil { return State{}, fmt.Errorf("parse session state %s: %w", name, err) } + if st.Name != name { + return State{}, fmt.Errorf("session state name %q does not match file name %q", st.Name, name) + } return st, nil } -func removeSessionFiles(dir, name string) { - _ = os.Remove(statePath(dir, name)) - _ = os.Remove(SocketPath(dir, name)) +func removeSessionFiles(dir, name string) error { + if err := ValidateName(name); err != nil { + return err + } + if err := VerifyDir(dir); err != nil { + return err + } + for _, path := range []string{statePath(dir, name), SocketPath(dir, name)} { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return err + } + } + return nil } // ProcAlive reports whether pid is a live process (signal-0 probe). It is the From f9baa7f3d0326bf84a11a38c1ecfe98b94c1d7e8 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 08:51:24 +0000 Subject: [PATCH 02/22] fix: harden terminal display boundaries --- internal/adapter/agent.go | 3 +- internal/adapter/agent_test.go | 21 ++++ internal/app/app.go | 21 ++-- internal/app/render_cluster_test.go | 23 ++++ internal/app/service.go | 9 +- internal/app/service_test.go | 13 +- internal/displaytext/sanitize.go | 105 ++++++++++++++++ internal/displaytext/sanitize_test.go | 56 +++++++++ internal/vterm/vterm.go | 170 +++++++++++++++++++------- internal/vterm/vterm_test.go | 32 +++++ 10 files changed, 393 insertions(+), 60 deletions(-) create mode 100644 internal/displaytext/sanitize.go create mode 100644 internal/displaytext/sanitize_test.go diff --git a/internal/adapter/agent.go b/internal/adapter/agent.go index 6de0a4f..4611f8b 100644 --- a/internal/adapter/agent.go +++ b/internal/adapter/agent.go @@ -13,6 +13,7 @@ import ( "sync" "time" + "github.com/RandomCodeSpace/unified-agent-manager/internal/displaytext" "github.com/RandomCodeSpace/unified-agent-manager/internal/log" "github.com/RandomCodeSpace/unified-agent-manager/internal/session" ) @@ -225,7 +226,7 @@ func (a *Agent) startSession(ctx context.Context, req ResumeRequest, activity st if displayName == "" { displayName = displayNameFromDir(cwd) } - if err := a.Backend.SetSessionLabel(ctx, sessionName, displayName+" · "+a.Name()); err != nil { + if err := a.Backend.SetSessionLabel(ctx, sessionName, displaytext.Sanitize(displayName+" · "+a.Name())); err != nil { log.Debug("set session label failed", "session", sessionName, "error", err) } shouldSendPrompt := strings.TrimSpace(req.Prompt) != "" && (activity != "resumed" || !a.SkipPromptOnResume) diff --git a/internal/adapter/agent_test.go b/internal/adapter/agent_test.go index 6b8ddae..97e2481 100644 --- a/internal/adapter/agent_test.go +++ b/internal/adapter/agent_test.go @@ -99,6 +99,27 @@ func TestDispatchSetsSessionLabel(t *testing.T) { } } +func TestDispatchSanitizesLabelButPreservesPrompt(t *testing.T) { + ag, be := newLifecycleAgent(t) + name := "bug\x1b]52;c;YQ==\x07fix\nnow" + prompt := "keep \x1b[31mraw\x1b[0m\nexactly" + sess, err := ag.Dispatch(context.Background(), DispatchRequest{Name: name, Prompt: prompt, Cwd: "/tmp", Mode: "yolo"}) + if err != nil { + t.Fatalf("Dispatch: %v", err) + } + if sess.DisplayName != name || sess.Prompt != prompt { + t.Fatalf("persisted metadata changed: name=%q prompt=%q", sess.DisplayName, sess.Prompt) + } + labels := be.CallsOf("label") + if len(labels) != 1 || labels[0].Label != "bugfix now · fake" { + t.Fatalf("label calls = %+v", labels) + } + sends := be.CallsOf("send") + if len(sends) != 1 || sends[0].Text != prompt { + t.Fatalf("prompt delivery changed: %+v", sends) + } +} + // F52 — a per-session capture failure during the PR scrape is non-fatal: the // session stays in the List result with PR nil. func TestListKeepsSessionWhenCaptureFails(t *testing.T) { diff --git a/internal/app/app.go b/internal/app/app.go index f907f6c..da0f8c3 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -14,6 +14,7 @@ import ( "github.com/RandomCodeSpace/unified-agent-manager/internal/adapter" "github.com/RandomCodeSpace/unified-agent-manager/internal/agents" + "github.com/RandomCodeSpace/unified-agent-manager/internal/displaytext" "github.com/RandomCodeSpace/unified-agent-manager/internal/log" "github.com/RandomCodeSpace/unified-agent-manager/internal/session" "github.com/RandomCodeSpace/unified-agent-manager/internal/store" @@ -1271,7 +1272,7 @@ func (m Model) renderDetails() string { if _, _, showTask := m.tableWidths(); !showTask { b.WriteString(" " + taskStyle.Render(truncate(promptText(sess), max(8, m.contentWidth()-6))) + "\n") } - b.WriteString(" " + hintStyle.Render("agent: "+firstNonEmpty(sess.AgentType, "?")) + "\n") + b.WriteString(" " + hintStyle.Render("agent: "+displaytext.Sanitize(firstNonEmpty(sess.AgentType, "?"))) + "\n") if !sess.CreatedAt.IsZero() { b.WriteString(" " + hintStyle.Render("created: "+sess.CreatedAt.Format("Jan 02 15:04")) + "\n") } @@ -1395,26 +1396,26 @@ func (m Model) renderPrompt() string { var b strings.Builder b.WriteString("\n") if m.renaming { - b.WriteString(bar() + " " + hintStyle.Render("rename") + " " + titleStyle.Render(m.input) + brandStyle.Render("▏") + "\n") + b.WriteString(bar() + " " + hintStyle.Render("rename") + " " + titleStyle.Render(displaytext.Sanitize(m.input)) + brandStyle.Render("▏") + "\n") } else if m.peekOpen { // The command line doubles as a reply composer while peek is open: label // it so the sub-mode is discoverable (Enter sends, Esc closes) (F36). field := hintStyle.Render("type a reply…") if m.input != "" { - field = titleStyle.Render(m.input) + field = titleStyle.Render(displaytext.Sanitize(m.input)) } hints := hintStyle.Render("Enter send · Esc close") b.WriteString(bar() + " " + hintStyle.Render("reply") + " " + brandStyle.Render("›") + " " + field + brandStyle.Render("▏") + " " + hints + "\n") } else { field := hintStyle.Render("type a command…") if m.input != "" { - field = titleStyle.Render(m.input) + field = titleStyle.Render(displaytext.Sanitize(m.input)) } hints := hintStyle.Render(m.defaultAgent + " · ? help · e new · Esc quit") b.WriteString(bar() + " " + brandStyle.Render("›") + " " + field + brandStyle.Render("▏") + " " + hints + "\n") } if m.message != "" { - b.WriteString(" " + hintStyle.Render(m.message) + "\n") + b.WriteString(" " + hintStyle.Render(displaytext.Sanitize(m.message)) + "\n") } return b.String() } @@ -1481,9 +1482,9 @@ func absCwd(cwd string) string { return "?" } if abs, err := filepath.Abs(cwd); err == nil { - return abs + return displaytext.Sanitize(abs) } - return cwd + return displaytext.Sanitize(cwd) } func (m Model) renderHelp() string { @@ -1505,7 +1506,7 @@ func (m Model) renderHelp() string { func (m Model) renderConfirm() string { sess, _ := m.sessionByID(m.confirmStopID) - name := firstNonEmpty(sess.DisplayName, sess.ID, "session") + name := displaytext.Sanitize(firstNonEmpty(sess.DisplayName, sess.ID, "session")) return "\n " + sectionStyle.Render("Stop session") + "\n " + hintStyle.Render("Stop and remove ") + titleStyle.Render(name) + hintStyle.Render("?") + " " + brandStyle.Render("y") + hintStyle.Render(" / restart ") + brandStyle.Render("r") + hintStyle.Render(" / ") + titleStyle.Render("N") + "\n" @@ -1524,7 +1525,7 @@ func (m Model) renderWizard() string { } var b strings.Builder b.WriteString("\n " + sectionStyle.Render("NEW SESSION") + " " + hintStyle.Render(fmt.Sprintf("step %d of 4", step+1)) + "\n") - b.WriteString(" " + titleStyle.Render(steps[step]) + brandStyle.Render("▏") + "\n") // #nosec G602 -- step is clamped to [0, len(steps)) just above. + b.WriteString(" " + titleStyle.Render(displaytext.Sanitize(steps[step])) + brandStyle.Render("▏") + "\n") // #nosec G602 -- step is clamped to [0, len(steps)) just above. switch step { case 2: // Warn when the chosen working directory is not inside a git repo: there @@ -1620,7 +1621,7 @@ func prStatusStyle(s adapter.PRStatus) lipgloss.Style { // occupy rather than their byte length. When clipping happens an ellipsis is // appended and the result still fits within n columns (F28). func truncate(s string, n int) string { - s = strings.ReplaceAll(s, "\n", " ") + s = displaytext.Sanitize(s) if n <= 0 { return "" } diff --git a/internal/app/render_cluster_test.go b/internal/app/render_cluster_test.go index 57e3884..ef24bd0 100644 --- a/internal/app/render_cluster_test.go +++ b/internal/app/render_cluster_test.go @@ -87,6 +87,29 @@ func TestTruncateNeverEmitsMojibake(t *testing.T) { } } +func TestRenderedMetadataCannotInjectTerminalControls(t *testing.T) { + sess := adapter.Session{ + ID: "1", + AgentType: "fake\x1b[2J", + DisplayName: "safe\x1b]52;c;YQ==\x07name", + Prompt: "fix\nthis\x1b[31m now", + Cwd: "/tmp/evil\x1b[Hrepo", + ProcAlive: adapter.Alive, + } + m := Model{width: 100, sessions: []adapter.Session{sess}, selected: 0, confirmStopID: "1"} + out := m.renderDetails() + renderRow(sess, false, 30, 40, true) + m.renderConfirm() + for _, unsafe := range []string{"\x1b[2J", "\x1b]52", "\x1b[31m", "\x1b[H"} { + if strings.Contains(out, unsafe) { + t.Fatalf("rendered control sequence %q: %q", unsafe, out) + } + } + for _, want := range []string{"safename", "fix this now", "/tmp/evilrepo"} { + if !strings.Contains(out, want) { + t.Fatalf("sanitized text missing %q: %q", want, out) + } + } +} + // F28 — the task column must stay aligned even when a row's name contains wide // characters: byte-length padding would push the task cell out of alignment. func TestRenderRowAlignsTaskColumnForMultibyte(t *testing.T) { diff --git a/internal/app/service.go b/internal/app/service.go index c143cfb..de3e367 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -12,6 +12,7 @@ import ( "time" "github.com/RandomCodeSpace/unified-agent-manager/internal/adapter" + "github.com/RandomCodeSpace/unified-agent-manager/internal/displaytext" "github.com/RandomCodeSpace/unified-agent-manager/internal/log" "github.com/RandomCodeSpace/unified-agent-manager/internal/pr" "github.com/RandomCodeSpace/unified-agent-manager/internal/store" @@ -623,7 +624,13 @@ func (s *Service) PrintList(ctx context.Context, asJSON bool) error { return json.NewEncoder(os.Stdout).Encode(sessions) } for _, sess := range sessions { - fmt.Printf("%s\t%s\t%s\t%s\t%s\n", sess.ID, sess.AgentType, sess.State, sess.DisplayName, sess.Cwd) + fmt.Printf("%s\t%s\t%s\t%s\t%s\n", + displaytext.Sanitize(sess.ID), + displaytext.Sanitize(sess.AgentType), + displaytext.Sanitize(string(sess.State)), + displaytext.Sanitize(sess.DisplayName), + displaytext.Sanitize(sess.Cwd), + ) } return nil } diff --git a/internal/app/service_test.go b/internal/app/service_test.go index 6321421..9a9e1ad 100644 --- a/internal/app/service_test.go +++ b/internal/app/service_test.go @@ -189,7 +189,12 @@ func assertWorkflowAdapterActions(t *testing.T, svc *Service, fake *svcFakeAdapt func TestServicePrintListAndErrors(t *testing.T) { dir := t.TempDir() st, _ := store.Open(filepath.Join(dir, "sessions.json")) - svc := NewService(st, adapter.NewRegistry([]adapter.AgentAdapter{&svcFakeAdapter{name: "fake", available: true}})) + unsafeName := "safe\x1b]52;c;YQ==\x07name" + fake := &svcFakeAdapter{name: "fake", available: true, sessions: []adapter.Session{{ + ID: "live0001", AgentType: "fake", DisplayName: unsafeName, Cwd: "/tmp/evil\x1b[2Jrepo", + SessionName: "uam-fake-live0001", State: adapter.Active, ProcAlive: adapter.Alive, CreatedAt: time.Now(), + }}} + svc := NewService(st, adapter.NewRegistry([]adapter.AgentAdapter{fake})) if _, err := svc.DispatchNamed(context.Background(), "missing", "", "x", "", ""); err == nil { t.Fatal("want missing agent error") } @@ -201,7 +206,7 @@ func TestServicePrintListAndErrors(t *testing.T) { t.Fatal(err) } }) - if !strings.Contains(out, "[") { + if !strings.Contains(out, "[") || !strings.Contains(out, `\u001b]52`) { t.Fatalf("json out=%q", out) } out = captureStdout(t, func() { @@ -209,7 +214,9 @@ func TestServicePrintListAndErrors(t *testing.T) { t.Fatal(err) } }) - _ = out + if strings.Contains(out, "\x1b") || !strings.Contains(out, "safename") || !strings.Contains(out, "/tmp/evilrepo") { + t.Fatalf("plain text output was not sanitized: %q", out) + } } func TestServicePersistsPromptAndReportsDeadTmuxRecord(t *testing.T) { diff --git a/internal/displaytext/sanitize.go b/internal/displaytext/sanitize.go new file mode 100644 index 0000000..78116c8 --- /dev/null +++ b/internal/displaytext/sanitize.go @@ -0,0 +1,105 @@ +// Package displaytext prepares untrusted metadata for display in a terminal. +package displaytext + +import ( + "strings" + "unicode/utf8" +) + +type parseState uint8 + +const ( + ground parseState = iota + escape + csi + controlString + controlStringEscape +) + +// Sanitize returns valid UTF-8 terminal display text. It removes ANSI CSI, +// OSC, DCS, SOS, PM, and APC control sequences; drops unsafe control runes; +// and turns horizontal tabs and line breaks into spaces. +func Sanitize(s string) string { + var b strings.Builder + b.Grow(len(s)) + state := ground + for i := 0; i < len(s); { + // Accept the legacy single-byte C1 representation as well as valid + // UTF-8 C1 runes. DecodeRuneInString otherwise treats it as invalid. + if s[i] >= 0x80 && s[i] <= 0x9f { + state = step(state, rune(s[i]), &b) + i++ + continue + } + r, size := utf8.DecodeRuneInString(s[i:]) + if r == utf8.RuneError && size == 1 { + i++ // discard invalid bytes; output is always valid UTF-8 + continue + } + state = step(state, r, &b) + i += size + } + return b.String() +} + +func step(state parseState, r rune, b *strings.Builder) parseState { + switch state { + case escape: + switch r { + case '[': + return csi + case ']', 'P', 'X', '^', '_': + return controlString + case 0x1b: + return escape + default: + return appendGround(r, b) + } + case csi: + if r == 0x1b { + return escape + } + if r == 0x9c || r >= 0x40 && r <= 0x7e { + return ground + } + return csi + case controlString: + switch r { + case 0x07, 0x9c: + return ground + case 0x1b: + return controlStringEscape + default: + return controlString + } + case controlStringEscape: + if r == '\\' || r == 0x9c || r == 0x07 { + return ground + } + if r == 0x1b { + return controlStringEscape + } + return controlString + default: + return appendGround(r, b) + } +} + +func appendGround(r rune, b *strings.Builder) parseState { + switch r { + case 0x1b: + return escape + case 0x9b: + return csi + case 0x90, 0x98, 0x9d, 0x9e, 0x9f: + return controlString + case '\t', '\n', '\r': + b.WriteByte(' ') + return ground + } + if r < 0x20 || r == 0x7f || r >= 0x80 && r <= 0x9f { + return ground + } + b.WriteRune(r) + return ground +} diff --git a/internal/displaytext/sanitize_test.go b/internal/displaytext/sanitize_test.go new file mode 100644 index 0000000..13db12d --- /dev/null +++ b/internal/displaytext/sanitize_test.go @@ -0,0 +1,56 @@ +package displaytext + +import ( + "strings" + "testing" + "unicode/utf8" +) + +func TestSanitizeRemovesTerminalControls(t *testing.T) { + in := "safe\tname\npath\r" + + "\x1b[31mred\x1b[0m" + + "\x1b]52;c;Y2xpcGJvYXJk\x07" + + "\x1bPignored\x1b\\" + + "\u009b2Jhidden\u009c" + + "\x00\x08\x7f世界" + want := "safe name path redhidden世界" + if got := Sanitize(in); got != want { + t.Fatalf("Sanitize() = %q, want %q", got, want) + } +} + +func TestSanitizeIsUTF8SafeAndIdempotent(t *testing.T) { + in := string([]byte{'o', 'k', 0xff, 0xfe}) + " 🚀\x1b[999999999999999999999Cdone" + got := Sanitize(in) + if !utf8.ValidString(got) { + t.Fatalf("Sanitize() emitted invalid UTF-8: %q", got) + } + if twice := Sanitize(got); twice != got { + t.Fatalf("Sanitize() is not idempotent: once %q, twice %q", got, twice) + } +} + +func FuzzSanitize(f *testing.F) { + for _, seed := range []string{ + "plain text", + "café 世界 🚀", + "\x1b[31mred\x1b[0m", + "\x1b]52;c;YQ==\x07visible", + "\x1bPpayload\x1b\\visible", + string([]byte{0xff, 'x', 0x9b, '2', 'J'}), + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, in string) { + got := Sanitize(in) + if !utf8.ValidString(got) { + t.Fatalf("invalid UTF-8: %q", got) + } + if strings.ContainsAny(got, "\x1b\x07\x00\x7f") { + t.Fatalf("unsafe control survived: %q", got) + } + if twice := Sanitize(got); twice != got { + t.Fatalf("not idempotent: %q != %q", twice, got) + } + }) +} diff --git a/internal/vterm/vterm.go b/internal/vterm/vterm.go index c973055..d660332 100644 --- a/internal/vterm/vterm.go +++ b/internal/vterm/vterm.go @@ -262,22 +262,25 @@ func (t *Terminal) dispatchCSI(params string, final byte) { } return def } + boundedArg := func(i, def, limit int) int { + return min(arg(i, def), limit) + } s := t.active() switch final { case 'A': - s.moveY(-arg(0, 1)) + s.moveY(-boundedArg(0, 1, t.rows)) case 'B', 'e': - s.moveY(arg(0, 1)) + s.moveY(boundedArg(0, 1, t.rows)) case 'C', 'a': - s.moveX(arg(0, 1)) + s.moveX(boundedArg(0, 1, t.cols)) case 'D': - s.moveX(-arg(0, 1)) + s.moveX(-boundedArg(0, 1, t.cols)) case 'E': s.x = 0 - s.moveY(arg(0, 1)) + s.moveY(boundedArg(0, 1, t.rows)) case 'F': s.x = 0 - s.moveY(-arg(0, 1)) + s.moveY(-boundedArg(0, 1, t.rows)) case 'G', '`': s.x = clamp(arg(0, 1)-1, 0, t.cols-1) s.pendingWrap = false @@ -293,23 +296,19 @@ func (t *Terminal) dispatchCSI(params string, final byte) { case 'K': s.eraseLine(argDefault(n, 0, 0), t.cols, t.bceFill()) case 'L': - s.insertLines(arg(0, 1), t.bceFill()) + s.insertLines(boundedArg(0, 1, s.bottom-s.y+1), t.bceFill()) case 'M': s.deleteLines(arg(0, 1), t.bceFill()) case '@': - s.insertChars(arg(0, 1), t.cols, t.bceFill()) + s.insertChars(boundedArg(0, 1, t.cols-s.x), t.cols, t.bceFill()) case 'P': s.deleteChars(arg(0, 1), t.cols, t.bceFill()) case 'X': - s.eraseChars(arg(0, 1), t.cols, t.bceFill()) + s.eraseChars(boundedArg(0, 1, t.cols-s.x), t.cols, t.bceFill()) case 'S': - for i := 0; i < arg(0, 1); i++ { - t.scrollUp() - } + t.scrollUpN(boundedArg(0, 1, s.bottom-s.top+1)) case 'T': - for i := 0; i < arg(0, 1); i++ { - t.scrollDownRegion() - } + t.scrollDownRegionN(boundedArg(0, 1, s.bottom-s.top+1)) case 'r': top := clamp(arg(0, 1)-1, 0, t.rows-1) bot := clamp(arg(1, t.rows)-1, 0, t.rows-1) @@ -366,16 +365,25 @@ func csiParams(s string) []int { for _, p := range parts { v := 0 for _, c := range p { - if c < '0' || c > '9' || v > 1<<20 { + if c < '0' || c > '9' { break } - v = v*10 + int(c-'0') + digit := int(c - '0') + if v > (maxCSIParam-digit)/10 { + v = maxCSIParam + continue + } + v = v*10 + digit } out = append(out, v) } return out } +// Keep parsed arithmetic comfortably away from integer overflow. Grid-affecting +// operations are clamped further to their row/column dimensions at dispatch. +const maxCSIParam = int(^uint(0)>>1) / 2 + // replayModes are the input- and cursor-affecting DEC private modes Redraw // re-emits, in a deterministic order. The agent's attach client tears all of // these down on detach (mouse + bracketed paste explicitly, DECCKM via @@ -466,28 +474,71 @@ func (t *Terminal) lineFeed(wrapped bool) { // scrollUp shifts the scroll region up one row. On the main screen with a // full-height region the departing top row enters scrollback history. func (t *Terminal) scrollUp() { + t.scrollUpN(1) +} + +func (t *Terminal) scrollUpN(n int) { s := t.active() - if !t.onAlt && s.top == 0 && s.bottom == t.rows-1 && t.maxHistory > 0 { - t.pushHistory(bufLine{text: rowText(s.cells[0]), wrapped: s.wrapped[0]}) - } top, bot := s.top, s.bottom - rec := s.cells[top] - copy(s.cells[top:bot], s.cells[top+1:bot+1]) - copy(s.wrapped[top:bot], s.wrapped[top+1:bot+1]) - s.cells[bot] = rec - clearRow(s.cells[bot], t.bceFill()) - s.wrapped[bot] = false + n = clamp(n, 0, bot-top+1) + if n == 0 { + return + } + if n == 1 { + if !t.onAlt && top == 0 && bot == t.rows-1 && t.maxHistory > 0 { + t.pushHistory(bufLine{text: rowText(s.cells[top]), wrapped: s.wrapped[top]}) + } + recycled := s.cells[top] + copy(s.cells[top:bot], s.cells[top+1:bot+1]) + copy(s.wrapped[top:bot], s.wrapped[top+1:bot+1]) + s.cells[bot] = recycled + clearRow(s.cells[bot], t.bceFill()) + s.wrapped[bot] = false + return + } + if !t.onAlt && top == 0 && bot == t.rows-1 && t.maxHistory > 0 { + for i := top; i < top+n; i++ { + t.pushHistory(bufLine{text: rowText(s.cells[i]), wrapped: s.wrapped[i]}) + } + } + recycled := append([][]cell(nil), s.cells[top:top+n]...) + copy(s.cells[top:bot-n+1], s.cells[top+n:bot+1]) + copy(s.cells[bot-n+1:bot+1], recycled) + copy(s.wrapped[top:bot-n+1], s.wrapped[top+n:bot+1]) + for i := bot - n + 1; i <= bot; i++ { + clearRow(s.cells[i], t.bceFill()) + s.wrapped[i] = false + } } func (t *Terminal) scrollDownRegion() { + t.scrollDownRegionN(1) +} + +func (t *Terminal) scrollDownRegionN(n int) { s := t.active() top, bot := s.top, s.bottom - rec := s.cells[bot] - copy(s.cells[top+1:bot+1], s.cells[top:bot]) - copy(s.wrapped[top+1:bot+1], s.wrapped[top:bot]) - s.cells[top] = rec - clearRow(s.cells[top], t.bceFill()) - s.wrapped[top] = false + n = clamp(n, 0, bot-top+1) + if n == 0 { + return + } + if n == 1 { + recycled := s.cells[bot] + copy(s.cells[top+1:bot+1], s.cells[top:bot]) + copy(s.wrapped[top+1:bot+1], s.wrapped[top:bot]) + s.cells[top] = recycled + clearRow(s.cells[top], t.bceFill()) + s.wrapped[top] = false + return + } + recycled := append([][]cell(nil), s.cells[bot-n+1:bot+1]...) + copy(s.cells[top+n:bot+1], s.cells[top:bot-n+1]) + copy(s.cells[top:top+n], recycled) + copy(s.wrapped[top+n:bot+1], s.wrapped[top:bot-n+1]) + for i := top; i < top+n; i++ { + clearRow(s.cells[i], t.bceFill()) + s.wrapped[i] = false + } } func (t *Terminal) reverseIndex() { @@ -731,13 +782,26 @@ func (s *screen) insertLines(n int, fill cell) { if s.y < s.top || s.y > s.bottom { return } - for i := 0; i < n; i++ { - rec := s.cells[s.bottom] + n = clamp(n, 0, s.bottom-s.y+1) + if n == 0 { + return + } + if n == 1 { + recycled := s.cells[s.bottom] copy(s.cells[s.y+1:s.bottom+1], s.cells[s.y:s.bottom]) copy(s.wrapped[s.y+1:s.bottom+1], s.wrapped[s.y:s.bottom]) - s.cells[s.y] = rec + s.cells[s.y] = recycled clearRow(s.cells[s.y], fill) s.wrapped[s.y] = false + return + } + recycled := append([][]cell(nil), s.cells[s.bottom-n+1:s.bottom+1]...) + copy(s.cells[s.y+n:s.bottom+1], s.cells[s.y:s.bottom-n+1]) + copy(s.cells[s.y:s.y+n], recycled) + copy(s.wrapped[s.y+n:s.bottom+1], s.wrapped[s.y:s.bottom-n+1]) + for i := s.y; i < s.y+n; i++ { + clearRow(s.cells[i], fill) + s.wrapped[i] = false } } @@ -745,35 +809,51 @@ func (s *screen) deleteLines(n int, fill cell) { if s.y < s.top || s.y > s.bottom { return } - for i := 0; i < n; i++ { - rec := s.cells[s.y] + n = clamp(n, 0, s.bottom-s.y+1) + if n == 0 { + return + } + if n == 1 { + recycled := s.cells[s.y] copy(s.cells[s.y:s.bottom], s.cells[s.y+1:s.bottom+1]) copy(s.wrapped[s.y:s.bottom], s.wrapped[s.y+1:s.bottom+1]) - s.cells[s.bottom] = rec + s.cells[s.bottom] = recycled clearRow(s.cells[s.bottom], fill) s.wrapped[s.bottom] = false + return + } + recycled := append([][]cell(nil), s.cells[s.y:s.y+n]...) + copy(s.cells[s.y:s.bottom-n+1], s.cells[s.y+n:s.bottom+1]) + copy(s.cells[s.bottom-n+1:s.bottom+1], recycled) + copy(s.wrapped[s.y:s.bottom-n+1], s.wrapped[s.y+n:s.bottom+1]) + for i := s.bottom - n + 1; i <= s.bottom; i++ { + clearRow(s.cells[i], fill) + s.wrapped[i] = false } } func (s *screen) insertChars(n, cols int, fill cell) { row := s.cells[s.y] - for i := 0; i < n; i++ { - copy(row[s.x+1:cols], row[s.x:cols-1]) - row[s.x] = fill + n = clamp(n, 0, cols-s.x) + copy(row[s.x+n:cols], row[s.x:cols-n]) + for i := s.x; i < s.x+n; i++ { + row[i] = fill } } func (s *screen) deleteChars(n, cols int, fill cell) { row := s.cells[s.y] - for i := 0; i < n; i++ { - copy(row[s.x:cols-1], row[s.x+1:cols]) - row[cols-1] = fill + n = clamp(n, 0, cols-s.x) + copy(row[s.x:cols-n], row[s.x+n:cols]) + for i := cols - n; i < cols; i++ { + row[i] = fill } } func (s *screen) eraseChars(n, cols int, fill cell) { row := s.cells[s.y] - for x := s.x; x < s.x+n && x < cols; x++ { + n = clamp(n, 0, cols-s.x) + for x := s.x; x < s.x+n; x++ { row[x] = fill } } diff --git a/internal/vterm/vterm_test.go b/internal/vterm/vterm_test.go index 546b5fe..16257d8 100644 --- a/internal/vterm/vterm_test.go +++ b/internal/vterm/vterm_test.go @@ -340,6 +340,38 @@ func TestMalformedCSIRecovers(t *testing.T) { } } +func TestHugeCSICountsAreClampedToTheGrid(t *testing.T) { + const huge = "999999999999999999999" + for _, tc := range []struct { + name, setup, hugeOp, boundedOp string + }{ + {"IL", "one\r\ntwo\r\nthree\r\nfour\x1b[1;1H", "\x1b[" + huge + "L", "\x1b[4L"}, + {"DL", "one\r\ntwo\r\nthree\r\nfour\x1b[1;1H", "\x1b[" + huge + "M", "\x1b[4M"}, + {"ICH", "abcdefgh\x1b[1;1H", "\x1b[" + huge + "@", "\x1b[8@"}, + {"DCH", "abcdefgh\x1b[1;1H", "\x1b[" + huge + "P", "\x1b[8P"}, + {"ECH", "abcdefgh\x1b[1;1H", "\x1b[" + huge + "X", "\x1b[8X"}, + {"SU", "one\r\ntwo\r\nthree\r\nfour", "\x1b[" + huge + "S", "\x1b[4S"}, + {"SD", "one\r\ntwo\r\nthree\r\nfour", "\x1b[" + huge + "T", "\x1b[4T"}, + } { + t.Run(tc.name, func(t *testing.T) { + hugeTerm, boundedTerm := New(8, 4, 0), New(8, 4, 0) + feed(t, hugeTerm, tc.setup+tc.hugeOp) + feed(t, boundedTerm, tc.setup+tc.boundedOp) + if got, want := string(hugeTerm.Redraw()), string(boundedTerm.Redraw()); got != want { + t.Fatalf("huge count differs from grid-sized count:\n got %q\nwant %q", got, want) + } + }) + } +} + +func BenchmarkHostileCSICounts(b *testing.B) { + seq := []byte("\x1b[999999999999999999999S\x1b[999999999999999999999L\x1b[999999999999999999999P") + for b.Loop() { + term := New(120, 40, 200) + _, _ = term.Write(seq) + } +} + func TestScrollRegionReverseWrapAndKeypadIgnored(t *testing.T) { term := New(20, 4, 0) // Keypad mode escapes and charset designators are consumed silently. From 4c1ecef76fa7e4230e9b2a9fda483d942d830632 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 08:47:28 +0000 Subject: [PATCH 03/22] fix(lifecycle): close persistence races --- internal/app/loadsessions_test.go | 64 ++++++++++++++++++++++++++ internal/app/service.go | 75 +++++++++++++++++++++++++++---- internal/session/host.go | 50 ++++++++++++++++----- internal/session/session_test.go | 43 ++++++++++++++++++ internal/store/store.go | 32 +++++++++++-- internal/store/store_test.go | 48 ++++++++++++++++++++ 6 files changed, 291 insertions(+), 21 deletions(-) diff --git a/internal/app/loadsessions_test.go b/internal/app/loadsessions_test.go index 890c190..6754bda 100644 --- a/internal/app/loadsessions_test.go +++ b/internal/app/loadsessions_test.go @@ -2,6 +2,8 @@ package app import ( "context" + "errors" + "os" "path/filepath" "sync" "testing" @@ -75,6 +77,68 @@ func TestLoadSessionsRefreshDoesNotClobberConcurrentPin(t *testing.T) { } } +func TestRefreshDoesNotClobberConcurrentMutationOnSameSession(t *testing.T) { + now := time.Now() + live := adapter.Session{ID: "aaaa1111", AgentType: "fake", DisplayName: "before", Cwd: "/tmp", SessionName: "uam-fake-aaaa1111", State: adapter.Active, ProcAlive: adapter.Alive, CreatedAt: now} + svc, st, _ := newLoadService(t, []adapter.Session{live}) + key := store.Key("fake", live.ID) + if err := st.Update(func(cfg *store.Config) error { + cfg.Sessions[key] = store.SessionRecord{ID: live.ID, Agent: "fake", Name: "before", Workdir: "/tmp", SessionName: live.SessionName, Status: store.StatusActive, LastSeenAt: now.Add(-2 * lastSeenRefresh)} + return nil + }); err != nil { + t.Fatal(err) + } + + stale, err := st.Load() + if err != nil { + t.Fatal(err) + } + liveMap := map[string]adapter.Session{key: live} + svc.mergeStoredSessions(liveMap, stale, now) + updates := svc.refreshSessionRecords(context.Background(), liveMap, &stale) + if err := svc.Rename(context.Background(), live.ID, "after"); err != nil { + t.Fatal(err) + } + if err := svc.TogglePin(context.Background(), live.ID); err != nil { + t.Fatal(err) + } + if err := svc.persistRefresh(updates); err != nil { + t.Fatal(err) + } + + cfg, err := st.Load() + if err != nil { + t.Fatal(err) + } + rec := cfg.Sessions[key] + if rec.Name != "after" || !rec.Pinned { + t.Fatalf("refresh clobbered same-session mutation: %+v", rec) + } + if rec.LastSeenAt.Before(now) { + t.Fatalf("refresh-owned field was not persisted: %+v", rec) + } +} + +func TestPersistRefreshReturnsReadOnlyStoreError(t *testing.T) { + path := filepath.Join(t.TempDir(), "sessions.json") + data := []byte(`{"schema_version":999,"default_agent":"opencode","sessions":{},"ui":{}}`) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + st, err := store.Open(path) + if err != nil { + t.Fatal(err) + } + svc := NewService(st, nil) + status := store.StatusActive + err = svc.persistRefresh(map[string]refreshPatch{ + "fake:aaaa1111": {status: &status}, + }) + if !errors.Is(err, store.ErrReadOnly) { + t.Fatalf("persistRefresh error = %v, want ErrReadOnly", err) + } +} + // F01 (-race) — concurrent LoadSessions refreshes and TogglePin calls must not // race and must not lose the final pin state. func TestLoadSessionsConcurrentWithTogglePin_Race(t *testing.T) { diff --git a/internal/app/service.go b/internal/app/service.go index de3e367..11736fe 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -64,7 +64,9 @@ func (s *Service) LoadSessions(ctx context.Context) ([]adapter.Session, store.Co // writes only those keys, so a concurrent TogglePin/Rename on a different // session is never clobbered by a stale whole-config Save (F01). updates := s.refreshSessionRecords(ctx, live, &cfg) - s.persistRefresh(updates) + if err := s.persistRefresh(updates); err != nil { + log.Warn("persist refreshed session metadata failed", "records", len(updates), "error", err) + } out := sessionsFromMap(live) SortSessions(out) return out, cfg, nil @@ -105,12 +107,36 @@ func (s *Service) PruneStartup(ctx context.Context) error { // atomic read-modify-write. It re-reads the on-disk config inside the lock and // overwrites only the supplied keys, leaving every other record (and any // concurrent mutation to it) intact. -func (s *Service) persistRefresh(updates map[string]store.SessionRecord) { +type refreshPatch struct { + create *store.SessionRecord + status *store.Status + lastSeenAt *time.Time + pr *store.PRRecord + clearPR bool +} + +func (s *Service) persistRefresh(updates map[string]refreshPatch) error { if len(updates) == 0 || s.Store == nil { - return + return nil } - _ = s.Store.Update(func(cfg *store.Config) error { - for key, rec := range updates { + return s.Store.Update(func(cfg *store.Config) error { + for key, patch := range updates { + rec, exists := cfg.Sessions[key] + if (!exists || rec.ID == "") && patch.create != nil { + rec = *patch.create + } + if patch.status != nil { + rec.Status = *patch.status + } + if patch.lastSeenAt != nil { + rec.LastSeenAt = *patch.lastSeenAt + } + if patch.clearPR { + rec.PR = nil + } else if patch.pr != nil { + pr := *patch.pr + rec.PR = &pr + } cfg.Sessions[key] = rec } return nil @@ -200,8 +226,8 @@ func deadSessionFromRecord(rec store.SessionRecord, now time.Time) adapter.Sessi // also updates the in-memory cfg and live maps so the returned snapshot is // consistent. pr.Check runs here, OUTSIDE any store lock; persistence happens // later via persistRefresh's atomic re-read. -func (s *Service) refreshSessionRecords(ctx context.Context, live map[string]adapter.Session, cfg *store.Config) map[string]store.SessionRecord { - updates := map[string]store.SessionRecord{} +func (s *Service) refreshSessionRecords(ctx context.Context, live map[string]adapter.Session, cfg *store.Config) map[string]refreshPatch { + updates := map[string]refreshPatch{} now := time.Now() // Acquire the PR-check guard for the duration of this refresh's network pass. // If another refresh already holds it, skip the gh calls (the persisted PR @@ -213,6 +239,7 @@ func (s *Service) refreshSessionRecords(ctx context.Context, live map[string]ada defer s.prInFlight.Store(false) } for key, sess := range live { + before, existed := cfg.Sessions[key] rec, changed, keep := reconcileRefreshRecord(cfg, key, sess, now) if !keep { continue @@ -222,12 +249,44 @@ func (s *Service) refreshSessionRecords(ctx context.Context, live map[string]ada changed = true } if changed { - updates[key] = rec + updates[key] = makeRefreshPatch(before, rec, existed && before.ID != "") } } return updates } +func makeRefreshPatch(before, after store.SessionRecord, existed bool) refreshPatch { + patch := refreshPatch{} + if !existed { + created := after + patch.create = &created + } + if !existed || before.Status != after.Status { + status := after.Status + patch.status = &status + } + if !existed || !before.LastSeenAt.Equal(after.LastSeenAt) { + lastSeen := after.LastSeenAt + patch.lastSeenAt = &lastSeen + } + if !prRecordEqual(before.PR, after.PR) { + if after.PR == nil { + patch.clearPR = true + } else { + pr := *after.PR + patch.pr = &pr + } + } + return patch +} + +func prRecordEqual(a, b *store.PRRecord) bool { + if a == nil || b == nil { + return a == nil && b == nil + } + return *a == *b +} + func reconcileRefreshRecord(cfg *store.Config, key string, sess adapter.Session, now time.Time) (store.SessionRecord, bool, bool) { rec, ok := cfg.Sessions[key] changed := false diff --git a/internal/session/host.go b/internal/session/host.go index 4bc2481..93dd274 100644 --- a/internal/session/host.go +++ b/internal/session/host.go @@ -11,6 +11,7 @@ import ( "os/signal" "strings" "sync" + "sync/atomic" "syscall" "time" @@ -43,6 +44,12 @@ const killGrace = 1500 * time.Millisecond // disconnected rather than allowed to stall the session. const attachBufFrames = 512 +const ( + markClosedRetryWindow = 2 * time.Second + markClosedRetryBase = 25 * time.Millisecond + markClosedRetryMax = 400 * time.Millisecond +) + // RunHost is the entry point of the detached per-session host process // (`uam __host`). It starts the agent command under a PTY, mirrors all output // into a terminal emulator (for peek/replay), serves the control socket, and @@ -100,8 +107,9 @@ type host struct { // escalation stops there. cleaned is closed after shutdown has also // removed the runtime files, so a Kill reply means the session is fully // gone — a List immediately after must not see leftovers. - exited chan struct{} - cleaned chan struct{} + exited chan struct{} + cleaned chan struct{} + stopping atomic.Bool } type attachClient struct { @@ -273,6 +281,7 @@ func (h *host) signalLoop() { signal.Notify(ch, syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP) <-ch // Forward termination to the agent; the normal exit path then runs. + h.stopping.Store(true) h.terminateChild() } @@ -315,6 +324,7 @@ func (h *host) handleConn(conn net.Conn) { _ = writeJSONLine(conn, response{OK: true}) _ = conn.Close() case opKill: + h.stopping.Store(true) h.terminateChild() select { case <-h.cleaned: @@ -514,7 +524,6 @@ func (h *host) signalChild(sig syscall.Signal) { // closed (the native replacement for the tmux session-closed hook), tell any // attached clients, and remove the runtime files. func (h *host) shutdown(exitCode int) { - h.markClosed(exitCode) h.mu.Lock() for cl := range h.clients { delete(h.clients, cl) @@ -524,15 +533,36 @@ func (h *host) shutdown(exitCode int) { if err := removeSessionFiles(h.dir, h.name); err != nil { log.Warn("remove session files failed", "session", h.name, "error", err) } + h.markClosed(exitCode) } func (h *host) markClosed(exitCode int) { - st, err := store.Open(store.DefaultPath()) - if err != nil { - log.Warn("open store to mark session closed failed", "session", h.name, "error", err) - return - } - if err := st.MarkSessionClosed(h.name, exitCode); err != nil { - log.Warn("mark session closed failed", "session", h.name, "error", err) + deadline := time.Now().Add(markClosedRetryWindow) + delay := markClosedRetryBase + var lastErr error + for { + st, err := store.Open(store.DefaultPath()) + if err == nil { + var matched bool + matched, err = st.TryMarkSessionClosed(h.name, exitCode) + if err == nil && matched { + return + } + } + if h.stopping.Load() && err == nil { + return + } + lastErr = err + remaining := time.Until(deadline) + if remaining <= 0 { + if lastErr != nil { + log.Warn("mark session closed failed after retry", "session", h.name, "error", lastErr) + } else { + log.Warn("mark session closed record not found before retry deadline", "session", h.name) + } + return + } + time.Sleep(min(delay, remaining)) + delay = min(delay*2, markClosedRetryMax) } } diff --git a/internal/session/session_test.go b/internal/session/session_test.go index cb8d6a4..e7251bb 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -179,6 +179,49 @@ func TestAgentExitMarksStoreRecordClosed(t *testing.T) { }) } +func TestAgentExitBeforeRecordPersistenceEventuallyMarksRecordClosed(t *testing.T) { + c := newTestClient(t) + shortDir, err := os.MkdirTemp("", "uam-exit-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(shortDir) }) + c.Dir = shortDir + ctx := context.Background() + name := "uam-fake-feedface" + + st, err := store.Open(store.DefaultPath()) + if err != nil { + t.Fatal(err) + } + if err := c.CreateSession(ctx, name, t.TempDir(), nil, []string{"/bin/sh", "-c", "exit 3"}); err != nil { + t.Fatalf("CreateSession: %v", err) + } + + // Waiting for runtime cleanup proves the host observed the exit before the + // durable record existed. The host must retain responsibility for recording + // the exit long enough for dispatch persistence to catch up. + waitFor(t, "runtime files removed before persistence", func() bool { + _, err := os.Stat(SocketPath(c.Dir, name)) + return os.IsNotExist(err) + }) + if err := st.Update(func(cfg *store.Config) error { + cfg.PutSession("fake:feedface", store.SessionRecord{ID: "feedface", Agent: "fake", Name: "n", SessionName: name, Status: store.StatusActive}) + return nil + }); err != nil { + t.Fatal(err) + } + + waitFor(t, "late record marked closed", func() bool { + cfg, err := st.Load() + if err != nil { + return false + } + rec := cfg.Sessions["fake:feedface"] + return rec.Status == store.StatusClosedByUser && rec.LastExitCode != nil && *rec.LastExitCode == 3 + }) +} + func TestCreateSessionReportsStartupFailure(t *testing.T) { c := newTestClient(t) err := c.CreateSession(context.Background(), "uam-fake-11112222", t.TempDir(), nil, []string{"/nonexistent/agent-binary"}) diff --git a/internal/store/store.go b/internal/store/store.go index 02be9a9..2b143d0 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -574,7 +574,22 @@ func (s *Store) saveNoLock(cfg Config) error { _ = os.Remove(tmp) return err } - return os.Rename(tmp, s.path) + if err := os.Rename(tmp, s.path); err != nil { + return err + } + return syncDir(filepath.Dir(s.path)) +} + +func syncDir(dir string) error { + f, err := os.Open(dir) + if err != nil { + return fmt.Errorf("open store directory for sync: %w", err) + } + defer func() { _ = f.Close() }() + if err := f.Sync(); err != nil && !errors.Is(err, syscall.EINVAL) && !errors.Is(err, syscall.ENOTSUP) { + return fmt.Errorf("sync store directory: %w", err) + } + return nil } func (s *Store) lock() (func(), error) { @@ -620,10 +635,19 @@ func (s *Store) moveAside() error { return os.Rename(s.path, s.backupPath()) } // session-closed hook driving `uam notify-closed`. Idempotent and a no-op // when no record matches (e.g. uam already deleted it via `uam rm`). func (s *Store) MarkSessionClosed(sessionName string, exitCode int) error { + _, err := s.TryMarkSessionClosed(sessionName, exitCode) + return err +} + +// TryMarkSessionClosed is MarkSessionClosed with an explicit match result. The +// host uses the result to distinguish an intentionally idempotent no-op from +// the narrow launch race where the durable record has not been written yet. +func (s *Store) TryMarkSessionClosed(sessionName string, exitCode int) (bool, error) { if sessionName == "" { - return nil + return false, nil } - return s.Update(func(cfg *Config) error { + matched := false + err := s.Update(func(cfg *Config) error { for key, rec := range cfg.Sessions { if rec.SessionName != sessionName { continue @@ -632,10 +656,12 @@ func (s *Store) MarkSessionClosed(sessionName string, exitCode int) error { code := exitCode rec.LastExitCode = &code cfg.Sessions[key] = rec + matched = true return nil } return nil }) + return matched, err } func PruneOld(cfg *Config, maxAge time.Duration, exists func(string) bool) { diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 8ce87b6..026a420 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -68,6 +68,54 @@ func TestSaveLoadRoundTrip(t *testing.T) { } } +func TestTryMarkSessionClosedReportsWhetherRecordMatched(t *testing.T) { + dir := t.TempDir() + s, err := Open(filepath.Join(dir, "sessions.json")) + if err != nil { + t.Fatal(err) + } + if err := s.Update(func(cfg *Config) error { + cfg.Sessions["fake:deadbeef"] = SessionRecord{ID: "deadbeef", Agent: "fake", SessionName: "uam-fake-deadbeef", Status: StatusActive} + return nil + }); err != nil { + t.Fatal(err) + } + + matched, err := s.TryMarkSessionClosed("uam-fake-missing", 7) + if err != nil { + t.Fatalf("missing record: %v", err) + } + if matched { + t.Fatal("missing record unexpectedly matched") + } + + matched, err = s.TryMarkSessionClosed("uam-fake-deadbeef", 7) + if err != nil { + t.Fatalf("matching record: %v", err) + } + if !matched { + t.Fatal("existing record was not reported as matched") + } + cfg, err := s.Load() + if err != nil { + t.Fatal(err) + } + rec := cfg.Sessions["fake:deadbeef"] + if rec.Status != StatusClosedByUser || rec.LastExitCode == nil || *rec.LastExitCode != 7 { + t.Fatalf("record not closed: %+v", rec) + } +} + +func TestSyncDirAcceptsStoreDirectoryAndRejectsMissingDirectory(t *testing.T) { + dir := t.TempDir() + if err := syncDir(dir); err != nil { + t.Fatalf("syncDir(temp): %v", err) + } + if err := syncDir(filepath.Join(dir, "missing")); err == nil { + t.Fatal("syncDir(missing) succeeded") + } +} + func TestSessionRecordCommandAliasJSONOmitEmpty(t *testing.T) { withoutAlias, err := json.Marshal(SessionRecord{ID: "id", Agent: "fake"}) if err != nil { From eec7df9a66e664d3fbd1b2407f2074ed8053b744 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 08:53:56 +0000 Subject: [PATCH 04/22] perf(discovery): decouple PR enrichment --- internal/adapter/agent.go | 7 ++ internal/adapter/registry.go | 57 +++++++++- internal/adapter/registry_test.go | 30 ++++- internal/app/app.go | 28 ++++- internal/app/pr_refresh_test.go | 145 ++++++++++++++--------- internal/app/service.go | 183 ++++++++++++++++++++++-------- internal/cli/cli.go | 2 +- 7 files changed, 343 insertions(+), 109 deletions(-) diff --git a/internal/adapter/agent.go b/internal/adapter/agent.go index 4611f8b..5dc5a74 100644 --- a/internal/adapter/agent.go +++ b/internal/adapter/agent.go @@ -278,6 +278,13 @@ func (a *Agent) List(ctx context.Context) ([]Session, error) { if err != nil { return nil, fmt.Errorf("list %s sessions: %w", a.Name(), err) } + return a.ListFromSnapshot(ctx, infos) +} + +// ListFromSnapshot filters a shared backend snapshot for this provider. The +// registry uses it to scan the runtime directory once per refresh while custom +// adapters that only implement List retain their existing behavior. +func (a *Agent) ListFromSnapshot(ctx context.Context, infos []session.Info) ([]Session, error) { var out []Session prefix := "uam-" + a.Name() + "-" seen := make(map[string]struct{}, len(infos)) diff --git a/internal/adapter/registry.go b/internal/adapter/registry.go index dbf2751..e6c26bd 100644 --- a/internal/adapter/registry.go +++ b/internal/adapter/registry.go @@ -1,14 +1,31 @@ package adapter -import "sort" +import ( + "errors" + "fmt" + "sort" + + "github.com/RandomCodeSpace/unified-agent-manager/internal/session" +) type Registry struct { adapters map[string]AgentAdapter reasons map[string]string + backend Backend } func NewRegistry(adapters []AgentAdapter) *Registry { - r := &Registry{adapters: map[string]AgentAdapter{}, reasons: map[string]string{}} + return newRegistry(nil, adapters) +} + +// NewRegistryWithBackend records the backend shared by production adapters so +// ListAll can enumerate it once and fan the immutable snapshot out by provider. +func NewRegistryWithBackend(backend Backend, adapters []AgentAdapter) *Registry { + return newRegistry(backend, adapters) +} + +func newRegistry(backend Backend, adapters []AgentAdapter) *Registry { + r := &Registry{adapters: map[string]AgentAdapter{}, reasons: map[string]string{}, backend: backend} for _, a := range adapters { ok, reason := a.Available() if ok { @@ -20,6 +37,42 @@ func NewRegistry(adapters []AgentAdapter) *Registry { return r } +type snapshotListingAdapter interface { + ListFromSnapshot(ctx Context, infos []session.Info) ([]Session, error) +} + +// ListAll returns every enabled provider's sessions. Production adapters share +// one backend snapshot; custom adapters without snapshot support fall back to +// their ordinary List implementation. Partial results survive adapter errors. +func (r *Registry) ListAll(ctx Context) ([]Session, error) { + enabled := r.Enabled() + var infos []session.Info + if r.backend != nil { + var err error + infos, err = r.backend.List(ctx) + if err != nil { + return nil, fmt.Errorf("list shared session backend: %w", err) + } + } + var out []Session + var joined error + for _, a := range enabled { + var sessions []Session + var err error + if lister, ok := a.(snapshotListingAdapter); ok && r.backend != nil { + sessions, err = lister.ListFromSnapshot(ctx, infos) + } else { + sessions, err = a.List(ctx) + } + if err != nil { + joined = errors.Join(joined, fmt.Errorf("list %s sessions: %w", a.Name(), err)) + continue + } + out = append(out, sessions...) + } + return out, joined +} + func (r *Registry) Get(name string) (AgentAdapter, bool) { a, ok := r.adapters[name]; return a, ok } func (r *Registry) Enabled() []AgentAdapter { diff --git a/internal/adapter/registry_test.go b/internal/adapter/registry_test.go index 2071b50..c4dd1e7 100644 --- a/internal/adapter/registry_test.go +++ b/internal/adapter/registry_test.go @@ -1,6 +1,13 @@ package adapter -import "testing" +import ( + "context" + "testing" + "time" + + "github.com/RandomCodeSpace/unified-agent-manager/internal/adapter/adaptertest" + "github.com/RandomCodeSpace/unified-agent-manager/internal/session" +) func TestRegistryDefaultAndDisabledReasons(t *testing.T) { r := NewRegistry([]AgentAdapter{fakeAdapter{name: "b", available: true}, fakeAdapter{name: "a", available: true}, fakeAdapter{name: "x", available: false}}) @@ -29,6 +36,27 @@ func TestRegistrySkipsUnavailableAdapters(t *testing.T) { } } +func TestRegistryListAllUsesSingleBackendSnapshot(t *testing.T) { + backend := &adaptertest.Backend{Sessions: []session.Info{ + {Name: "uam-a-aaaa1111", CreatedUnix: time.Now().Unix(), Alive: true}, + {Name: "uam-b-bbbb2222", CreatedUnix: time.Now().Unix(), Alive: true}, + }} + a := NewAgent("a", "A", []CommandCandidate{{Display: "sh", Args: []string{"/bin/sh"}}}, nil, backend) + b := NewAgent("b", "B", []CommandCandidate{{Display: "sh", Args: []string{"/bin/sh"}}}, nil, backend) + r := NewRegistryWithBackend(backend, []AgentAdapter{a, b}) + + sessions, err := r.ListAll(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(sessions) != 2 { + t.Fatalf("sessions = %d, want 2: %+v", len(sessions), sessions) + } + if got := len(backend.CallsOf("list")); got != 1 { + t.Fatalf("backend List calls = %d, want 1", got) + } +} + type fakeAdapter struct { name string available bool diff --git a/internal/app/app.go b/internal/app/app.go index da0f8c3..e78ac26 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -111,6 +111,8 @@ type attachSpecMsg struct { } type attachFinishedMsg struct{ err error } type refreshMsg time.Time +type prRefreshMsg time.Time +type prRefreshedMsg struct{ err error } // peekTickMsg is the peek-focus poll tick. When the peek panel is open it // re-captures the focused session's pane (rate-limited per id) so the panel @@ -143,7 +145,7 @@ func New() Model { // Build the registry from the single shared adapter list so the TUI and the // CLI service can never diverge (the old hand-rolled list here omitted // hermes — F14). - reg := adapter.NewRegistry(agents.Default(client)) + reg := adapter.NewRegistryWithBackend(client, agents.Default(client)) return NewWithDeps(st, reg) } @@ -180,13 +182,17 @@ func NewWizard(st *store.Store, reg *adapter.Registry) Model { } func (m Model) Init() tea.Cmd { - return tea.Batch(m.loadSessionsCmd(), refreshTick(), peekTick()) + return tea.Batch(m.loadSessionsCmd(), refreshTick(), peekTick(), prRefreshTick(100*time.Millisecond)) } func refreshTick() tea.Cmd { return tea.Tick(2*time.Second, func(t time.Time) tea.Msg { return refreshMsg(t) }) } +func prRefreshTick(after time.Duration) tea.Cmd { + return tea.Tick(after, func(t time.Time) tea.Msg { return prRefreshMsg(t) }) +} + func peekTick() tea.Cmd { return tea.Tick(peekTickInterval, func(t time.Time) tea.Msg { return peekTickMsg(t) }) } @@ -212,6 +218,12 @@ func (m Model) loadSessionsCmd() tea.Cmd { } } +func (m Model) refreshPRCmd() tea.Cmd { + return func() tea.Msg { + return prRefreshedMsg{err: m.service.RefreshPRStatuses(context.Background())} + } +} + func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { case tea.WindowSizeMsg: @@ -224,6 +236,18 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return next, tea.Batch(next.loadSessionsCmd(), refreshTick()) } return next, refreshTick() + case prRefreshMsg: + return m, tea.Batch(m.refreshPRCmd(), prRefreshTick(prRefreshAge)) + case prRefreshedMsg: + if msg.err != nil { + log.Warn("refresh pull-request statuses failed", "error", msg.err) + return m, nil + } + if m.loading { + return m, nil + } + m.loading = true + return m, m.loadSessionsCmd() case peekTickMsg: // Re-arm the peek ticker unconditionally; only re-capture the focused // session when the panel is open and the per-id rate limit allows it diff --git a/internal/app/pr_refresh_test.go b/internal/app/pr_refresh_test.go index e84cd79..ecaf4a9 100644 --- a/internal/app/pr_refresh_test.go +++ b/internal/app/pr_refresh_test.go @@ -2,42 +2,32 @@ package app import ( "context" - "os" - "path/filepath" + "errors" + "fmt" "sync" + "sync/atomic" "testing" "time" "github.com/RandomCodeSpace/unified-agent-manager/internal/adapter" + "github.com/RandomCodeSpace/unified-agent-manager/internal/pr" "github.com/RandomCodeSpace/unified-agent-manager/internal/store" ) -// writeFakeGH installs a fake `gh` binary at UAM_GH_BIN/PATH whose script body -// is given, returning nothing. Used to make pr.Check behavior deterministic. -func writeFakeGH(t *testing.T, body string) { - t.Helper() - dir := t.TempDir() - gh := filepath.Join(dir, "gh") - if err := os.WriteFile(gh, []byte(body), 0o755); err != nil { - t.Fatal(err) - } - t.Setenv("UAM_GH_BIN", gh) - t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) -} - -// F02 — when pr.Check times out (hung gh), updatePRRecord must still stamp -// LastChecked so the 60s guard arms and the next tick does NOT re-launch gh. -func TestUpdatePRRecordWritesLastCheckedOnTimeout(t *testing.T) { - // gh sleeps long enough to blow the per-check timeout. - writeFakeGH(t, "#!/bin/sh\nsleep 30\n") - +func TestLoadSessionsNeverRunsPRChecker(t *testing.T) { live := adapter.Session{ID: "aaaa1111", AgentType: "fake", DisplayName: "A", Cwd: "/tmp", SessionName: "uam-fake-aaaa1111", State: adapter.Active, ProcAlive: adapter.Alive, CreatedAt: time.Now(), PR: &adapter.PRRef{URL: "https://github.com/o/r/pull/1", Number: 1, Status: adapter.PROpen}} svc, st, _ := newLoadService(t, []adapter.Session{live}) - - before := time.Now() + var calls atomic.Int32 + svc.checkPR = func(context.Context, string) (pr.Status, error) { + calls.Add(1) + return pr.Open, nil + } if _, _, err := svc.LoadSessions(context.Background()); err != nil { t.Fatalf("LoadSessions: %v", err) } + if calls.Load() != 0 { + t.Fatalf("LoadSessions invoked PR checker %d times", calls.Load()) + } cfg, err := st.Load() if err != nil { t.Fatalf("Load: %v", err) @@ -46,48 +36,91 @@ func TestUpdatePRRecordWritesLastCheckedOnTimeout(t *testing.T) { if !ok || rec.PR == nil { t.Fatalf("PR record not persisted: %+v", cfg.Sessions) } - if !rec.PR.LastChecked.After(before.Add(-time.Second)) { - t.Fatalf("LastChecked not stamped on the timeout path: %v", rec.PR.LastChecked) + if !rec.PR.LastChecked.IsZero() { + t.Fatalf("discovery should not claim a network check: %v", rec.PR.LastChecked) } } -// F02 — concurrent LoadSessions must not stack overlapping pr.Check subprocesses. -// The fake gh uses an atomic mkdir lock to detect overlap: if a second gh runs -// while the first is still in flight, the lock fails and writes a sentinel. -func TestRefreshDoesNotStackConcurrentLoads(t *testing.T) { - dir := t.TempDir() - lockDir := filepath.Join(dir, "lock") - overlap := filepath.Join(dir, "overlap") - gh := filepath.Join(dir, "gh") - // mkdir is atomic: only one process can hold the lock at a time. If a second - // gh runs concurrently it can't create the dir, so it records overlap. - body := "#!/bin/sh\n" + - "if ! mkdir '" + lockDir + "' 2>/dev/null; then touch '" + overlap + "'; fi\n" + - "sleep 0.2\n" + - "rmdir '" + lockDir + "' 2>/dev/null\n" + - "echo '{\"state\":\"OPEN\",\"isDraft\":false,\"mergedAt\":null}'\n" - if err := os.WriteFile(gh, []byte(body), 0o755); err != nil { +func TestRefreshPRStatusesUsesBoundedConcurrencyAndPersistsResults(t *testing.T) { + sessions := make([]adapter.Session, 10) + for i := range sessions { + id := fmt.Sprintf("%08x", i+1) + sessions[i] = adapter.Session{ID: id, AgentType: "fake", DisplayName: id, Cwd: "/tmp", SessionName: "uam-fake-" + id, State: adapter.Active, ProcAlive: adapter.Alive, CreatedAt: time.Now(), PR: &adapter.PRRef{URL: fmt.Sprintf("https://github.com/o/r/pull/%d", i+1), Number: i + 1, Status: adapter.PROpen}} + } + svc, st, _ := newLoadService(t, sessions) + var active atomic.Int32 + var maximum atomic.Int32 + svc.checkPR = func(ctx context.Context, _ string) (pr.Status, error) { + n := active.Add(1) + defer active.Add(-1) + for { + old := maximum.Load() + if n <= old || maximum.CompareAndSwap(old, n) { + break + } + } + select { + case <-ctx.Done(): + return pr.None, ctx.Err() + case <-time.After(20 * time.Millisecond): + return pr.Merged, nil + } + } + if err := svc.RefreshPRStatuses(context.Background()); err != nil { + t.Fatal(err) + } + if got := maximum.Load(); got < 2 || got > prRefreshWorkers { + t.Fatalf("max concurrency = %d, want 2..%d", got, prRefreshWorkers) + } + cfg, err := st.Load() + if err != nil { t.Fatal(err) } - t.Setenv("UAM_GH_BIN", gh) - t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + for key, rec := range cfg.Sessions { + if rec.PR == nil || rec.PR.LastStatus != string(adapter.PRMerged) || rec.PR.LastChecked.IsZero() { + t.Fatalf("record %s not refreshed: %+v", key, rec) + } + } +} +func TestRefreshPRStatusesDoesNotOverlapPasses(t *testing.T) { live := adapter.Session{ID: "bbbb2222", AgentType: "fake", DisplayName: "B", Cwd: "/tmp", SessionName: "uam-fake-bbbb2222", State: adapter.Active, ProcAlive: adapter.Alive, CreatedAt: time.Now(), PR: &adapter.PRRef{URL: "https://github.com/o/r/pull/2", Number: 2, Status: adapter.PROpen}} svc, _, _ := newLoadService(t, []adapter.Session{live}) - - var wg sync.WaitGroup - for i := 0; i < 8; i++ { - wg.Add(1) - go func() { - defer wg.Done() - if _, _, err := svc.LoadSessions(context.Background()); err != nil { - t.Errorf("LoadSessions: %v", err) - } - }() + entered := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + svc.checkPR = func(context.Context, string) (pr.Status, error) { + once.Do(func() { close(entered) }) + <-release + return pr.Open, nil } - wg.Wait() + done := make(chan error, 1) + go func() { done <- svc.RefreshPRStatuses(context.Background()) }() + <-entered + if err := svc.RefreshPRStatuses(context.Background()); err != nil { + t.Fatalf("overlapping pass should skip cleanly: %v", err) + } + close(release) + if err := <-done; err != nil { + t.Fatal(err) + } +} - if _, err := os.Stat(overlap); err == nil { - t.Fatal("overlapping pr.Check subprocesses ran concurrently — in-flight guard missing") +func TestRefreshPRStatusesPreservesLastKnownStatusOnCheckerError(t *testing.T) { + live := adapter.Session{ID: "cccc3333", AgentType: "fake", DisplayName: "C", Cwd: "/tmp", SessionName: "uam-fake-cccc3333", State: adapter.Active, ProcAlive: adapter.Alive, CreatedAt: time.Now(), PR: &adapter.PRRef{URL: "https://github.com/o/r/pull/3", Number: 3, Status: adapter.PROpen}} + svc, st, _ := newLoadService(t, []adapter.Session{live}) + svc.checkPR = func(context.Context, string) (pr.Status, error) { + return pr.None, errors.New("checker unavailable") + } + if err := svc.RefreshPRStatuses(context.Background()); err != nil { + t.Fatal(err) + } + cfg, err := st.Load() + if err != nil { + t.Fatal(err) + } + rec := cfg.Sessions[store.Key("fake", live.ID)] + if rec.PR == nil || rec.PR.LastStatus != string(adapter.PROpen) || rec.PR.LastChecked.IsZero() { + t.Fatalf("last known PR state was not preserved: %+v", rec.PR) } } diff --git a/internal/app/service.go b/internal/app/service.go index 11736fe..2d130cc 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -8,6 +8,7 @@ import ( "os" "sort" "strings" + "sync" "sync/atomic" "time" @@ -26,6 +27,7 @@ type Service struct { // that fails to acquire it skips the network check and keeps the persisted // PR record; the holder clears it on completion, success or error (F02). prInFlight atomic.Bool + checkPR func(context.Context, string) (pr.Status, error) } const agentUnavailableFormat = "agent %q unavailable" @@ -43,13 +45,15 @@ const lastSeenRefresh = time.Minute // long (F20 Stage 2). const pruneMaxAge = 30 * 24 * time.Hour -// prCheckTimeout bounds a single `gh pr view` call so a hung gh cannot wedge the -// 2s refresh. It is comfortably under the 60s PR re-check interval, so a -// timed-out check just defers to the next tick (F02). -const prCheckTimeout = 4 * time.Second +const ( + prCheckTimeout = 4 * time.Second + prRefreshTimeout = 5 * time.Second + prRefreshAge = 60 * time.Second + prRefreshWorkers = 4 +) func NewService(st *store.Store, reg *adapter.Registry) *Service { - return &Service{Store: st, Registry: reg} + return &Service{Store: st, Registry: reg, checkPR: pr.Check} } func (s *Service) LoadSessions(ctx context.Context) ([]adapter.Session, store.Config, error) { @@ -59,10 +63,8 @@ func (s *Service) LoadSessions(ctx context.Context) ([]adapter.Session, store.Co } live := s.liveSessions(ctx) s.mergeStoredSessions(live, cfg, time.Now()) - // refreshSessionRecords runs pr.Check OUTSIDE any store lock and returns the - // records this refresh owns. persistRefresh re-reads inside the flock and - // writes only those keys, so a concurrent TogglePin/Rename on a different - // session is never clobbered by a stale whole-config Save (F01). + // Discovery and reconciliation are local-only. Network PR enrichment runs on + // the independent RefreshPRStatuses cadence and never delays this path. updates := s.refreshSessionRecords(ctx, live, &cfg) if err := s.persistRefresh(updates); err != nil { log.Warn("persist refreshed session metadata failed", "records", len(updates), "error", err) @@ -156,17 +158,14 @@ func (s *Service) liveSessions(ctx context.Context) map[string]adapter.Session { if s.Registry == nil { return live } - for _, a := range s.Registry.Enabled() { - sessions, err := a.List(ctx) - if err != nil { - // One adapter's List failure must not blank the dashboard for the - // others, but it shouldn't vanish silently either (F12). - log.Warn("listing sessions for adapter failed", "agent", a.Name(), "error", err) - continue - } - for _, sess := range sessions { - live[store.Key(sess.AgentType, sess.ID)] = sess - } + sessions, err := s.Registry.ListAll(ctx) + if err != nil { + // Partial custom-adapter results are retained; production's shared backend + // failure yields an empty snapshot and one actionable warning. + log.Warn("listing managed sessions failed", "error", err) + } + for _, sess := range sessions { + live[store.Key(sess.AgentType, sess.ID)] = sess } return live } @@ -224,27 +223,18 @@ func deadSessionFromRecord(rec store.SessionRecord, now time.Time) adapter.Sessi // refreshSessionRecords reconciles live sessions against the loaded config and // returns the records this refresh wants to persist (keyed by store key). It // also updates the in-memory cfg and live maps so the returned snapshot is -// consistent. pr.Check runs here, OUTSIDE any store lock; persistence happens -// later via persistRefresh's atomic re-read. -func (s *Service) refreshSessionRecords(ctx context.Context, live map[string]adapter.Session, cfg *store.Config) map[string]refreshPatch { +// consistent. It only records locally discovered PR metadata; status checks run +// independently in RefreshPRStatuses. +func (s *Service) refreshSessionRecords(_ context.Context, live map[string]adapter.Session, cfg *store.Config) map[string]refreshPatch { updates := map[string]refreshPatch{} now := time.Now() - // Acquire the PR-check guard for the duration of this refresh's network pass. - // If another refresh already holds it, skip the gh calls (the persisted PR - // record is kept) so stacked ticks never launch overlapping gh subprocesses. - // Released unconditionally on completion so a single pass never wedges the - // guard (F02). - doPR := s.prInFlight.CompareAndSwap(false, true) - if doPR { - defer s.prInFlight.Store(false) - } for key, sess := range live { before, existed := cfg.Sessions[key] rec, changed, keep := reconcileRefreshRecord(cfg, key, sess, now) if !keep { continue } - if doPR && updatePRRecord(ctx, key, &sess, &rec, live, now) { + if syncDiscoveredPRRecord(sess, &rec) { cfg.Sessions[key] = rec changed = true } @@ -319,24 +309,123 @@ func reconcileRefreshRecord(cfg *store.Config, key string, sess adapter.Session, return rec, changed, true } -func updatePRRecord(ctx context.Context, key string, sess *adapter.Session, rec *store.SessionRecord, live map[string]adapter.Session, now time.Time) bool { - if sess.PR == nil || (rec.PR != nil && rec.PR.URL == sess.PR.URL && time.Since(rec.PR.LastChecked) <= 60*time.Second) { +func syncDiscoveredPRRecord(sess adapter.Session, rec *store.SessionRecord) bool { + if sess.PR == nil || (rec.PR != nil && rec.PR.URL == sess.PR.URL) { return false } - status := string(sess.PR.Status) - // Bound the gh call so a hung process cannot wedge the refresh. On timeout - // (or any error) the status is left at the last-known value and LastChecked is - // stamped below, arming the 60s guard so the next tick does not immediately - // re-launch gh (F02). - checkCtx, cancel := context.WithTimeout(ctx, prCheckTimeout) + rec.PR = &store.PRRecord{URL: sess.PR.URL, Number: sess.PR.Number, LastStatus: string(sess.PR.Status)} + return true +} + +type prRefreshJob struct { + key string + ref adapter.PRRef +} + +type prRefreshResult struct { + key string + record store.PRRecord +} + +// RefreshPRStatuses enriches stale discovered PRs outside the session-discovery +// path. Only PR-owned fields are persisted, and overlapping passes coalesce. +func (s *Service) RefreshPRStatuses(ctx context.Context) error { + if s.Store == nil || !s.prInFlight.CompareAndSwap(false, true) { + return nil + } + defer s.prInFlight.Store(false) + ctx, cancel := context.WithTimeout(ctx, prRefreshTimeout) defer cancel() - if checked, err := pr.Check(checkCtx, sess.PR.URL); err == nil && checked != pr.None { - status = string(checked) - sess.PR.Status = adapter.PRStatus(status) - live[key] = *sess + sessions, cfg, err := s.LoadSessions(ctx) + if err != nil { + return err + } + now := time.Now() + jobs := make([]prRefreshJob, 0, len(sessions)) + for _, sess := range sessions { + if sess.PR == nil { + continue + } + rec := cfg.Sessions[store.Key(sess.AgentType, sess.ID)] + if rec.PR != nil && rec.PR.URL == sess.PR.URL && now.Sub(rec.PR.LastChecked) < prRefreshAge { + continue + } + jobs = append(jobs, prRefreshJob{key: store.Key(sess.AgentType, sess.ID), ref: *sess.PR}) + } + if len(jobs) == 0 { + return nil + } + checker := s.checkPR + if checker == nil { + checker = pr.Check + } + jobCh := make(chan prRefreshJob) + resultCh := make(chan prRefreshResult, len(jobs)) + workers := min(prRefreshWorkers, len(jobs)) + var wg sync.WaitGroup + for range workers { + wg.Add(1) + go func() { + defer wg.Done() + for job := range jobCh { + status := job.ref.Status + checkCtx, checkCancel := context.WithTimeout(ctx, prCheckTimeout) + checked, checkErr := checker(checkCtx, job.ref.URL) + checkCancel() + if checkErr == nil && checked != pr.None { + status = adapterStatus(checked) + } + resultCh <- prRefreshResult{key: job.key, record: store.PRRecord{URL: job.ref.URL, Number: job.ref.Number, LastStatus: string(status), LastChecked: time.Now()}} + } + }() + } + go func() { + defer close(jobCh) + for _, job := range jobs { + select { + case jobCh <- job: + case <-ctx.Done(): + return + } + } + }() + wg.Wait() + close(resultCh) + results := make([]prRefreshResult, 0, len(jobs)) + for result := range resultCh { + results = append(results, result) + } + updateErr := s.Store.Update(func(cfg *store.Config) error { + for _, result := range results { + rec, ok := cfg.Sessions[result.key] + if !ok || rec.PR == nil || rec.PR.URL != result.record.URL { + continue + } + record := result.record + rec.PR = &record + cfg.Sessions[result.key] = rec + } + return nil + }) + if updateErr != nil { + return updateErr + } + return ctx.Err() +} + +func adapterStatus(status pr.Status) adapter.PRStatus { + switch status { + case pr.Open: + return adapter.PROpen + case pr.Merged: + return adapter.PRMerged + case pr.Closed: + return adapter.PRClosed + case pr.Draft: + return adapter.PRDraft + default: + return adapter.PRNone } - rec.PR = &store.PRRecord{URL: sess.PR.URL, Number: sess.PR.Number, LastStatus: status, LastChecked: now} - return true } func sessionsFromMap(live map[string]adapter.Session) []adapter.Session { diff --git a/internal/cli/cli.go b/internal/cli/cli.go index a4b48fb..07012a2 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -242,7 +242,7 @@ func NewService(st *store.Store) *app.Service { }) // Build the registry from the single shared adapter list so the CLI service // and the TUI can never diverge on which providers exist (F14). - reg := adapter.NewRegistry(agents.Default(client)) + reg := adapter.NewRegistryWithBackend(client, agents.Default(client)) return app.NewService(st, reg) } From 995ccee21070ff240dd86de8809912cc0eb5e193 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:26:25 +0000 Subject: [PATCH 05/22] test(discovery): characterize PR refresh paths --- internal/app/app_more_test.go | 47 +++++++++++++++++++++++++++++++++ internal/app/pr_refresh_test.go | 44 ++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/internal/app/app_more_test.go b/internal/app/app_more_test.go index e0b0314..ac630f7 100644 --- a/internal/app/app_more_test.go +++ b/internal/app/app_more_test.go @@ -141,6 +141,53 @@ func TestModelUpdateMessages(t *testing.T) { } } +func TestPRRefreshCommandsAndMessages(t *testing.T) { + m := NewWithDeps(nil, nil) + msg, ok := m.refreshPRCmd()().(prRefreshedMsg) + if !ok || msg.err != nil { + t.Fatalf("refreshPRCmd message = %#v", msg) + } + + model, cmd := m.Update(prRefreshedMsg{}) + m = model.(Model) + if !m.loading || cmd == nil { + t.Fatalf("successful PR refresh did not schedule cached reload: loading=%v cmd=%v", m.loading, cmd) + } + model, cmd = m.Update(prRefreshedMsg{}) + if cmd != nil || !model.(Model).loading { + t.Fatal("PR refresh overlapped an in-flight cached reload") + } + + m.loading = false + model, cmd = m.Update(prRefreshedMsg{err: errors.New("refresh failed")}) + if cmd != nil || model.(Model).loading { + t.Fatal("failed PR refresh should keep cached sessions without scheduling a reload") + } +} + +func TestRenderPeekAndLongestCommonPrefix(t *testing.T) { + m := NewWithDeps(nil, nil) + m.peekText = "one\ntwo\nthree\nfour\nfive\nsix" + m.height = 9 + peek := m.renderPeek() + if !strings.Contains(peek, "PEEK") || strings.Contains(peek, "one") || !strings.Contains(peek, "six") { + t.Fatalf("renderPeek = %q", peek) + } + for _, tc := range []struct { + items []string + want string + }{ + {items: nil, want: ""}, + {items: []string{"alpha"}, want: "alpha"}, + {items: []string{"alpha", "alpine", "alps"}, want: "alp"}, + {items: []string{"one", "two"}, want: ""}, + } { + if got := longestCommonPrefix(tc.items); got != tc.want { + t.Fatalf("longestCommonPrefix(%q) = %q, want %q", tc.items, got, tc.want) + } + } +} + func TestDispatchedMessageAttachesNewSession(t *testing.T) { fake := &svcFakeAdapter{name: "fake", available: true} m := NewWithDeps(nil, adapter.NewRegistry([]adapter.AgentAdapter{fake})) diff --git a/internal/app/pr_refresh_test.go b/internal/app/pr_refresh_test.go index ecaf4a9..5cb8408 100644 --- a/internal/app/pr_refresh_test.go +++ b/internal/app/pr_refresh_test.go @@ -124,3 +124,47 @@ func TestRefreshPRStatusesPreservesLastKnownStatusOnCheckerError(t *testing.T) { t.Fatalf("last known PR state was not preserved: %+v", rec.PR) } } + +func TestRefreshPRStatusesSkipsFreshRecords(t *testing.T) { + live := adapter.Session{ID: "dddd4444", AgentType: "fake", DisplayName: "D", Cwd: "/tmp", SessionName: "uam-fake-dddd4444", State: adapter.Active, ProcAlive: adapter.Alive, CreatedAt: time.Now(), PR: &adapter.PRRef{URL: "https://github.com/o/r/pull/4", Number: 4, Status: adapter.PROpen}} + svc, st, _ := newLoadService(t, []adapter.Session{live}) + if _, _, err := svc.LoadSessions(context.Background()); err != nil { + t.Fatal(err) + } + if err := st.Update(func(cfg *store.Config) error { + rec := cfg.Sessions[store.Key("fake", live.ID)] + rec.PR.LastChecked = time.Now() + cfg.Sessions[store.Key("fake", live.ID)] = rec + return nil + }); err != nil { + t.Fatal(err) + } + var calls atomic.Int32 + svc.checkPR = func(context.Context, string) (pr.Status, error) { + calls.Add(1) + return pr.Closed, nil + } + if err := svc.RefreshPRStatuses(context.Background()); err != nil { + t.Fatal(err) + } + if calls.Load() != 0 { + t.Fatalf("fresh PR record triggered %d checks", calls.Load()) + } +} + +func TestAdapterStatusMapping(t *testing.T) { + for _, tc := range []struct { + in pr.Status + want adapter.PRStatus + }{ + {pr.Open, adapter.PROpen}, + {pr.Merged, adapter.PRMerged}, + {pr.Closed, adapter.PRClosed}, + {pr.Draft, adapter.PRDraft}, + {pr.None, adapter.PRNone}, + } { + if got := adapterStatus(tc.in); got != tc.want { + t.Fatalf("adapterStatus(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} From 1beb57017797d0d284acc740671ca5cb61b0ad5c Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 08:48:20 +0000 Subject: [PATCH 06/22] fix: harden CLI startup and logging --- .claude/worktrees/plan-review-edits | 1 - .gitignore | 1 + internal/cli/cli.go | 63 ++++++++++++- internal/cli/cli_test.go | 82 ++++++++++++++++ internal/log/log.go | 139 +++++++++++++++++++++++++++- internal/log/log_test.go | 112 ++++++++++++++++++++++ 6 files changed, 390 insertions(+), 8 deletions(-) delete mode 160000 .claude/worktrees/plan-review-edits diff --git a/.claude/worktrees/plan-review-edits b/.claude/worktrees/plan-review-edits deleted file mode 160000 index 98a3861..0000000 --- a/.claude/worktrees/plan-review-edits +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 98a3861bdde9e53022ba1b1808b4d20ec084ab96 diff --git a/.gitignore b/.gitignore index 5882b61..8d40a06 100644 --- a/.gitignore +++ b/.gitignore @@ -6,5 +6,6 @@ coverage.* .DS_Store .idea/ .vscode/ +/.claude/worktrees/ /CLAUDE.md /PLAN.md diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 07012a2..1df5d85 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -26,16 +26,29 @@ import ( func Main() { flag.Usage = Usage flag.Parse() + args := flag.Args() + + // Help and version are deliberately independent from both the cache logger + // and the persistent store. They must remain usable when either location is + // unavailable (for example during installation diagnostics). + if handled, err := runBeforeLogger(args); handled { + if err != nil && !errors.Is(err, context.Canceled) { + fmt.Fprintf(os.Stderr, "uam: %v\n", err) + os.Exit(1) + } + return + } closer, err := log.Init() if err != nil { + log.UseStderr(os.Stderr) fmt.Fprintf(os.Stderr, "uam: failed to initialize logger: %v\n", err) - os.Exit(1) + } else { + defer func() { _ = closer.Close() }() } - defer func() { _ = closer.Close() }() ctx := context.Background() - if err := Run(ctx, flag.Args()); err != nil && !errors.Is(err, context.Canceled) { + if err := Run(ctx, args); err != nil && !errors.Is(err, context.Canceled) { log.Error("run exited with error", "err", err) fmt.Fprintf(os.Stderr, "uam: %v\n", err) os.Exit(1) @@ -69,6 +82,9 @@ func Run(ctx context.Context, args []string) error { // RunWithTUI executes the CLI with an injectable TUI runner for tests. func RunWithTUI(ctx context.Context, args []string, runTUI func(context.Context, tea.Model) error) error { + if handled, err := runWithoutStore(ctx, args); handled { + return err + } st, err := store.Open(store.DefaultPath()) if err != nil { return err @@ -86,6 +102,47 @@ func RunWithTUI(ctx context.Context, args []string, runTUI func(context.Context, return runCommand(ctx, svc, args, runTUI) } +// runBeforeLogger handles commands that have no cache, store, or session +// dependency. Main invokes it before log.Init; RunWithTUI reaches the same +// behavior through runWithoutStore when called directly by tests/embedders. +func runBeforeLogger(args []string) (bool, error) { + if len(args) == 0 { + return false, nil + } + switch args[0] { + case "-h", "--help", "help": + Usage() + return true, nil + case "version", "--version": + fmt.Println(version.String()) + return true, nil + default: + return false, nil + } +} + +// runWithoutStore handles commands whose behavior does not depend on +// sessions.json. Keeping these ahead of store.Open makes recovery commands +// available even when the config directory is damaged or unwritable. +func runWithoutStore(ctx context.Context, args []string) (bool, error) { + if handled, err := runBeforeLogger(args); handled { + return true, err + } + if len(args) == 0 { + return false, nil + } + switch args[0] { + case "kill-all": + return true, runKillAll(ctx, session.NewClient().KillAll) + case "__host": + return true, session.RunHost(args[1:]) + case "__attach": + return true, session.RunAttach(args[1:]) + default: + return false, nil + } +} + func runCommand(ctx context.Context, svc *app.Service, args []string, runTUI func(context.Context, tea.Model) error) error { switch args[0] { case "-h", "--help", "help": diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index c12addb..bee254c 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -4,8 +4,10 @@ import ( "bytes" "context" "errors" + "flag" "io" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -147,6 +149,86 @@ func TestRunWithTUIHelpVersionAndDefault(t *testing.T) { } } +func TestRunWithTUIStateFreeCommandsDoNotOpenStore(t *testing.T) { + blocked := filepath.Join(t.TempDir(), "not-a-directory") + if err := os.WriteFile(blocked, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("UAM_CONFIG_DIR", blocked) + t.Setenv("UAM_SESSION_DIR", t.TempDir()) + + for _, args := range [][]string{{"help"}, {"version"}, {"kill-all"}} { + if err := RunWithTUI(context.Background(), args, noopRunTUI); err != nil { + t.Fatalf("RunWithTUI(%q) opened the store: %v", args, err) + } + } + if err := RunWithTUI(context.Background(), []string{"__host", "--name", "bad name", "--", "/bin/true"}, noopRunTUI); err == nil || !strings.Contains(err.Error(), "invalid session name") { + t.Fatalf("__host must be routed before the store and return its own validation error, got %v", err) + } + if err := RunWithTUI(context.Background(), []string{"__attach"}, noopRunTUI); err == nil || !strings.Contains(err.Error(), "attach requires a session name") { + t.Fatalf("__attach must be routed before the store and return its own validation error, got %v", err) + } +} + +func TestMainStatelessCommandsSkipLoggerAndStore(t *testing.T) { + blocked := filepath.Join(t.TempDir(), "not-a-directory") + if err := os.WriteFile(blocked, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + for _, command := range []string{"help", "version"} { + cmd := cliMainSubprocess(t, command, blocked, blocked) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("uam %s: %v\n%s", command, err, output) + } + if strings.Contains(string(output), "failed to initialize logger") { + t.Fatalf("uam %s initialized the logger: %s", command, output) + } + } +} + +func TestMainFallsBackToStderrWhenFileLoggerFails(t *testing.T) { + blocked := filepath.Join(t.TempDir(), "not-a-directory") + if err := os.WriteFile(blocked, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + cmd := cliMainSubprocess(t, "kill-all", blocked, filepath.Join(t.TempDir(), "config")) + cmd.Env = append(cmd.Env, "UAM_SESSION_DIR="+t.TempDir()) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("uam kill-all must continue with stderr logging: %v\n%s", err, output) + } + text := string(output) + if strings.Count(text, "failed to initialize logger") != 1 { + t.Fatalf("logger fallback warning count = %d, want 1: %s", strings.Count(text, "failed to initialize logger"), text) + } + if !strings.Contains(text, "all uam sessions stopped") { + t.Fatalf("kill-all did not run after logger fallback: %s", text) + } +} + +func TestCLIMainHelperProcess(t *testing.T) { + command := os.Getenv("UAM_TEST_MAIN_COMMAND") + if command == "" { + return + } + flag.CommandLine = flag.NewFlagSet("uam", flag.ContinueOnError) + os.Args = []string{"uam", command} + Main() + os.Exit(0) +} + +func cliMainSubprocess(t *testing.T, command, cacheDir, configDir string) *exec.Cmd { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run=^TestCLIMainHelperProcess$") + cmd.Env = append(os.Environ(), + "UAM_TEST_MAIN_COMMAND="+command, + "UAM_CACHE_DIR="+cacheDir, + "UAM_CONFIG_DIR="+configDir, + ) + return cmd +} + func TestRunCommandAttachLastAndNew(t *testing.T) { svc, fake := newCLITestService(t) id := dispatchAndCaptureID(t, svc, []string{"fake", "attachable"}) diff --git a/internal/log/log.go b/internal/log/log.go index fce104e..633ef5a 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -6,26 +6,157 @@ import ( "log/slog" "os" "path/filepath" + "sync" +) + +const ( + maxLogSize = int64(5 << 20) + maxBackups = 3 ) var current *slog.Logger = slog.New(slog.NewTextHandler(io.Discard, nil)) +type rotatingFile struct { + mu sync.Mutex + path string + file *os.File + size int64 +} + func Init() (io.Closer, error) { dir := cacheDir() if err := os.MkdirAll(dir, 0o700); err != nil { return nil, err } - f, err := os.OpenFile(filepath.Join(dir, "uam.log"), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) // #nosec G304 -- UAM intentionally writes its own cache log path. + path := filepath.Join(dir, "uam.log") + if err := rotateIfNeeded(path); err != nil { + return nil, err + } + f, err := openLogFile(path) + if err != nil { + return nil, err + } + if err := enforcePrivateLogModes(path); err != nil { + _ = f.Close() + return nil, err + } + info, err := f.Stat() if err != nil { + _ = f.Close() return nil, err } + w := &rotatingFile{path: path, file: f, size: info.Size()} + current = newLogger(w) + current.Info("logger initialized", "path", f.Name()) + return w, nil +} + +func openLogFile(path string) (*os.File, error) { + return os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) // #nosec G304 -- UAM intentionally writes its own cache log path. +} + +func (w *rotatingFile) Write(p []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + if w.file == nil { + return 0, os.ErrClosed + } + if w.size > 0 && w.size+int64(len(p)) > maxLogSize { + if err := w.file.Close(); err != nil { + return 0, err + } + w.file = nil + if err := rotateLogs(w.path); err != nil { + return 0, err + } + f, err := openLogFile(w.path) + if err != nil { + return 0, err + } + w.file = f + w.size = 0 + if err := enforcePrivateLogModes(w.path); err != nil { + _ = w.file.Close() + w.file = nil + return 0, err + } + } + n, err := w.file.Write(p) + w.size += int64(n) + return n, err +} + +func (w *rotatingFile) Close() error { + w.mu.Lock() + defer w.mu.Unlock() + if w.file == nil { + return nil + } + err := w.file.Close() + w.file = nil + return err +} + +// UseStderr installs the non-file fallback used when Init cannot create or +// open the cache log. The caller remains responsible for printing the single +// initialization warning that explains why the fallback was selected. +func UseStderr(w io.Writer) { + if w == nil { + w = os.Stderr + } + current = newLogger(w) +} + +func newLogger(w io.Writer) *slog.Logger { level := slog.LevelInfo if os.Getenv("UAM_DEBUG") != "" { level = slog.LevelDebug } - current = slog.New(slog.NewTextHandler(f, &slog.HandlerOptions{Level: level})) - current.Info("logger initialized", "path", f.Name()) - return f, nil + return slog.New(slog.NewTextHandler(w, &slog.HandlerOptions{Level: level})) +} + +func rotateIfNeeded(path string) error { + info, err := os.Stat(path) + if os.IsNotExist(err) { + return enforcePrivateLogModes(path) + } + if err != nil { + return err + } + if info.Size() < maxLogSize { + return enforcePrivateLogModes(path) + } + return rotateLogs(path) +} + +func rotateLogs(path string) error { + if err := os.Remove(path + ".3"); err != nil && !os.IsNotExist(err) { + return err + } + for i := maxBackups - 1; i >= 1; i-- { + oldPath := path + "." + string(rune('0'+i)) + newPath := path + "." + string(rune('1'+i)) + if err := os.Rename(oldPath, newPath); err != nil && !os.IsNotExist(err) { + return err + } + } + if err := os.Rename(path, path+".1"); err != nil { + return err + } + return enforcePrivateLogModes(path) +} + +func enforcePrivateLogModes(path string) error { + for i := 0; i <= maxBackups; i++ { + candidate := path + if i > 0 { + candidate += "." + string(rune('0'+i)) + } + if err := os.Chmod(candidate, 0o600); err != nil && !os.IsNotExist(err) { + return err + } + } + return nil } func L() *slog.Logger { return current } diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 7daaaaa..9af8b5f 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -3,6 +3,7 @@ package log import ( "os" "path/filepath" + "strings" "testing" ) @@ -39,6 +40,117 @@ func TestInitError(t *testing.T) { } } +func TestInitEnforcesPrivateLogPermissions(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "uam.log") + if err := os.WriteFile(path, []byte("old log\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("UAM_CACHE_DIR", dir) + c, err := Init() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := c.Close(); err != nil { + t.Errorf("close logger: %v", err) + } + }) + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("uam.log mode = %o, want 600", got) + } +} + +func TestInitRotatesOversizedLogAndRetainsThreePrivateBackups(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "uam.log") + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatal(err) + } + if err := os.Truncate(path, maxLogSize); err != nil { + t.Fatal(err) + } + for i, body := range []string{"backup-one", "backup-two", "backup-three"} { + if err := os.WriteFile(path+"."+string(rune('1'+i)), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + t.Setenv("UAM_CACHE_DIR", dir) + c, err := Init() + if err != nil { + t.Fatal(err) + } + if err := c.Close(); err != nil { + t.Fatal(err) + } + + wantBodies := map[string]string{ + path + ".2": "backup-one", + path + ".3": "backup-two", + } + for _, candidate := range []string{path, path + ".1", path + ".2", path + ".3"} { + info, err := os.Stat(candidate) + if err != nil { + t.Fatalf("stat %s: %v", candidate, err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("%s mode = %o, want 600", candidate, got) + } + if want, ok := wantBodies[candidate]; ok { + got, err := os.ReadFile(candidate) + if err != nil { + t.Fatal(err) + } + if string(got) != want { + t.Fatalf("%s = %q, want %q", candidate, got, want) + } + } + } + rotated, err := os.Stat(path + ".1") + if err != nil { + t.Fatal(err) + } + if rotated.Size() != maxLogSize { + t.Fatalf("rotated size = %d, want %d", rotated.Size(), maxLogSize) + } +} + +func TestLoggerRotatesWhenActiveLogCrossesSizeLimit(t *testing.T) { + dir := t.TempDir() + t.Setenv("UAM_CACHE_DIR", dir) + c, err := Init() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := c.Close(); err != nil { + t.Errorf("close logger: %v", err) + } + }) + + Info("fill", "data", strings.Repeat("x", int(maxLogSize-1024))) + Info("after rotation", "marker", "new-log", "padding", strings.Repeat("y", 2048)) + + rotated, err := os.ReadFile(filepath.Join(dir, "uam.log.1")) + if err != nil { + t.Fatalf("read rotated log: %v", err) + } + if !strings.Contains(string(rotated), "fill") { + t.Fatal("rotated log does not contain the pre-limit entry") + } + current, err := os.ReadFile(filepath.Join(dir, "uam.log")) + if err != nil { + t.Fatalf("read current log: %v", err) + } + if !strings.Contains(string(current), "new-log") { + t.Fatal("current log does not contain the post-rotation entry") + } +} + func TestCacheDirFallbacks(t *testing.T) { dir := t.TempDir() t.Setenv("UAM_CACHE_DIR", "") From 83c9a31df7fdbcb10eea320e7feb59fdecfff1bc Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:00:14 +0000 Subject: [PATCH 07/22] fix(session): verify portable process identity --- internal/app/app.go | 24 -------------- internal/app/app_cmd_test.go | 5 +-- internal/app/app_more_test.go | 5 +-- internal/app/loadsessions_test.go | 4 ++- internal/app/render_cluster_test.go | 10 +++--- internal/log/log.go | 7 ---- internal/session/client.go | 15 ++++++--- internal/session/liveness_test.go | 35 ++++++++++++++++++++ internal/session/proc_start_darwin.go | 22 +++++++++++++ internal/session/proc_start_linux.go | 38 ++++++++++++++++++++++ internal/session/proc_start_other.go | 7 ++++ internal/session/session.go | 47 +++++++++------------------ internal/store/store.go | 8 +++-- 13 files changed, 142 insertions(+), 85 deletions(-) create mode 100644 internal/session/proc_start_darwin.go create mode 100644 internal/session/proc_start_linux.go create mode 100644 internal/session/proc_start_other.go diff --git a/internal/app/app.go b/internal/app/app.go index e78ac26..8bdea79 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -175,12 +175,6 @@ func (m Model) validateDefaultAgent(candidate string) string { } return candidate } -func NewWizard(st *store.Store, reg *adapter.Registry) Model { - m := NewWithDeps(st, reg) - m.wizard = true - return m -} - func (m Model) Init() tea.Cmd { return tea.Batch(m.loadSessionsCmd(), refreshTick(), peekTick(), prRefreshTick(100*time.Millisecond)) } @@ -1063,9 +1057,6 @@ func (m Model) resumeSelectedCmd() tea.Cmd { return m.loadSessionsCmd()() } } -func (m Model) stopSelectedCmd(remove bool) tea.Cmd { - return m.stopTargetCmd("", remove) -} // persistDefaultAgent persists the default-agent choice. On failure it surfaces // the error in the status line instead of swallowing it; on success it returns a @@ -1594,21 +1585,6 @@ func sessionGlyph(s adapter.Session) (string, lipgloss.Style) { } } -func stateGlyph(s adapter.State) (string, lipgloss.Style) { - switch s { - case adapter.Active: - return "⟳", liveGlyphStyle - case adapter.Failed: - return "✕", failGlyphStyle - default: - return "•", hintStyle - } -} - -func stateLabel(s adapter.State) string { - return strings.ToLower(string(s)) -} - // prStatusDot returns a distinct glyph per PR status (not color-only) so the PR // state survives a monochrome terminal or a screen scrape: open=hollow circle, // merged=filled circle, draft=half circle, closed=cross (F26). diff --git a/internal/app/app_cmd_test.go b/internal/app/app_cmd_test.go index 6a8f080..2aa67fb 100644 --- a/internal/app/app_cmd_test.go +++ b/internal/app/app_cmd_test.go @@ -80,9 +80,6 @@ func TestModelCommandFactories(t *testing.T) { if New().defaultAgent == "" { t.Fatal("New default empty") } - if !NewWizard(st, m.service.Registry).wizard { - t.Fatal("NewWizard not wizard") - } if m.Init() == nil { t.Fatal("Init returned nil") } @@ -98,7 +95,7 @@ func TestModelCommandFactories(t *testing.T) { if msg := m.pinSelectedCmd()(); msg.(sessionsLoadedMsg).err != nil { t.Fatalf("pin msg=%+v", msg) } - if msg := m.stopSelectedCmd(false)(); msg.(sessionsLoadedMsg).err != nil { + if msg := m.stopTargetCmd("", false)(); msg.(sessionsLoadedMsg).err != nil { t.Fatalf("stop msg=%+v", msg) } if msg := m.persistOrderCmd()(); msg.(sessionsLoadedMsg).err != nil { diff --git a/internal/app/app_more_test.go b/internal/app/app_more_test.go index ac630f7..f71dd74 100644 --- a/internal/app/app_more_test.go +++ b/internal/app/app_more_test.go @@ -114,7 +114,7 @@ func assertDispatchParsing(t *testing.T, m Model) { func assertViewHelpers(t *testing.T) { t.Helper() - if stateLabel(adapter.Active) != "active" || prStatusDot(adapter.PRMerged) == " " { + if prStatusDot(adapter.PRMerged) == " " { t.Fatal("status helpers bad") } if truncate("abcdef", 4) != "abc…" || trimLines("a\nb\nc", 2) != "b\nc" { @@ -431,9 +431,6 @@ func TestInputWindowAndStateBranches(t *testing.T) { if start < 0 || end < start || end > len(m.sessions) { t.Fatalf("bad window %d:%d", start, end) } - if stateLabel(adapter.Active) != "active" || stateLabel(adapter.Failed) != "failed" { - t.Fatal("state labels not covered") - } } func TestRenameEditingBranches(t *testing.T) { diff --git a/internal/app/loadsessions_test.go b/internal/app/loadsessions_test.go index 6754bda..bcd35e0 100644 --- a/internal/app/loadsessions_test.go +++ b/internal/app/loadsessions_test.go @@ -63,7 +63,9 @@ func TestLoadSessionsRefreshDoesNotClobberConcurrentPin(t *testing.T) { live := map[string]adapter.Session{store.Key("fake", liveA.ID): liveA} svc.mergeStoredSessions(live, cfgStale, time.Now()) updates := svc.refreshSessionRecords(context.Background(), live, &cfgStale) - svc.persistRefresh(updates) + if err := svc.persistRefresh(updates); err != nil { + t.Fatal(err) + } cfg, err := st.Load() if err != nil { diff --git a/internal/app/render_cluster_test.go b/internal/app/render_cluster_test.go index ef24bd0..75dbcac 100644 --- a/internal/app/render_cluster_test.go +++ b/internal/app/render_cluster_test.go @@ -178,9 +178,8 @@ func TestStateGlyphDistinguishesResumableFromFailed(t *testing.T) { if live == resumable { t.Fatalf("a live session and a resumable dead session must use distinct glyphs (both %q)", live) } - failGlyph, _ := stateGlyph(adapter.Failed) - if resumable == failGlyph { - t.Fatalf("a reboot-survivor resumable session must not use the red Failed glyph %q", failGlyph) + if resumable == "✕" { + t.Fatal("a reboot-survivor resumable session must not use the failure glyph") } _ = closed } @@ -192,9 +191,8 @@ func TestRenderRowResumableSessionNotMarkedFailed(t *testing.T) { if strings.Contains(strings.ToLower(row), "failed") { t.Fatalf("resumable row should not show the literal \"failed\": %q", row) } - failGlyph, _ := stateGlyph(adapter.Failed) - if strings.Contains(row, failGlyph) { - t.Fatalf("resumable row should not show the red Failed glyph %q: %q", failGlyph, row) + if strings.Contains(row, "✕") { + t.Fatalf("resumable row should not show the failure glyph: %q", row) } } diff --git a/internal/log/log.go b/internal/log/log.go index 633ef5a..f9571d6 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -1,7 +1,6 @@ package log import ( - "fmt" "io" "log/slog" "os" @@ -188,9 +187,3 @@ func cacheDir() string { } return filepath.Join(".uam", "cache") } - -// Fatal prints to stderr and exits with code 1. -func Fatal(format string, args ...any) { - fmt.Fprintf(os.Stderr, "uam: "+format+"\n", args...) - os.Exit(1) -} diff --git a/internal/session/client.go b/internal/session/client.go index d96b4c9..8b381cb 100644 --- a/internal/session/client.go +++ b/internal/session/client.go @@ -254,11 +254,18 @@ func (c *Client) Kill(ctx context.Context, name string) error { } // State exists but the socket path failed: the host is wedged, crashed, // or mid-shutdown. Escalate directly and wait for both processes to go. - // The start-time-verified probes matter most here: a recycled PID must - // never be signalled as if it were the session. - if st.hostAlive() { + // A live PID is not sufficient authority to signal: fallback control paths + // require a nonzero matching start identity so a recycled PID can never be + // signalled as if it were the session. + if ProcAlive(st.HostPID) { + if !procIdentityMatches(st.HostPID, st.HostStart) { + return fmt.Errorf("kill session %s: cannot verify process identity for host pid %d", name, st.HostPID) + } _ = syscall.Kill(st.HostPID, syscall.SIGTERM) - } else if st.childAlive() { + } else if ProcAlive(st.ChildPID) { + if !procIdentityMatches(st.ChildPID, st.ChildStart) { + return fmt.Errorf("kill session %s: cannot verify process identity for child pid %d", name, st.ChildPID) + } // Orphaned agent (host crashed): signal its process group directly. if err := syscall.Kill(-st.ChildPID, syscall.SIGTERM); err != nil { _ = syscall.Kill(st.ChildPID, syscall.SIGTERM) diff --git a/internal/session/liveness_test.go b/internal/session/liveness_test.go index 353ffd1..b311480 100644 --- a/internal/session/liveness_test.go +++ b/internal/session/liveness_test.go @@ -237,6 +237,41 @@ func TestProcAliveWithStartDetectsPIDReuse(t *testing.T) { } } +func TestProcIdentityMatchesRequiresRecordedAndCurrentIdentity(t *testing.T) { + pid := os.Getpid() + real := procStartTime(pid) + if real == 0 { + t.Skip("process start identity unavailable on this platform") + } + if !procIdentityMatches(pid, real) { + t.Fatal("matching nonzero process identity must be accepted") + } + if procIdentityMatches(pid, 0) { + t.Fatal("zero recorded identity must fail closed for signaling") + } + if procIdentityMatches(pid, real+1) { + t.Fatal("mismatched process identity must fail closed for signaling") + } +} + +func TestKillRefusesPIDFallbackWithoutVerifiedIdentity(t *testing.T) { + c := newTestClient(t) + if err := EnsureDir(c.Dir); err != nil { + t.Fatal(err) + } + name := "uam-fake-1d1d1d1d" + if err := writeState(c.Dir, State{Name: name, HostPID: os.Getpid(), HostStart: 0}); err != nil { + t.Fatal(err) + } + err := c.Kill(t.Context(), name) + if err == nil || !strings.Contains(err.Error(), "cannot verify process identity") { + t.Fatalf("Kill error = %v, want explicit process-identity safety error", err) + } + if !ProcAlive(os.Getpid()) { + t.Fatal("Kill signaled the test process despite missing identity") + } +} + // A stale state file whose PIDs were recycled by other processes (alive, but // with different start times) must be swept by List, not reported live. func TestListSweepsRecycledPIDState(t *testing.T) { diff --git a/internal/session/proc_start_darwin.go b/internal/session/proc_start_darwin.go new file mode 100644 index 0000000..2f2f1cd --- /dev/null +++ b/internal/session/proc_start_darwin.go @@ -0,0 +1,22 @@ +//go:build darwin + +package session + +import "golang.org/x/sys/unix" + +// procStartTime returns the process start timestamp in microseconds since the +// Unix epoch, or 0 when Darwin cannot provide a stable identity. +func procStartTime(pid int) int64 { + if pid <= 0 { + return 0 + } + info, err := unix.SysctlKinfoProc("kern.proc.pid", pid) + if err != nil || info == nil { + return 0 + } + started := info.Proc.P_starttime + if started.Sec <= 0 { + return 0 + } + return started.Sec*1_000_000 + int64(started.Usec) +} diff --git a/internal/session/proc_start_linux.go b/internal/session/proc_start_linux.go new file mode 100644 index 0000000..d23568b --- /dev/null +++ b/internal/session/proc_start_linux.go @@ -0,0 +1,38 @@ +//go:build linux + +package session + +import ( + "fmt" + "os" + "strconv" + "strings" +) + +// procStartTime returns /proc//stat field 22 (clock ticks since boot), +// or 0 when the process identity cannot be read. +func procStartTime(pid int) int64 { + if pid <= 0 { + return 0 + } + data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) + if err != nil { + return 0 + } + // comm (field 2) is parenthesized and may itself contain spaces or ')', + // so split after the LAST ')'. starttime is overall field 22, i.e. index + // 19 of the fields that follow comm. + rest := string(data) + if i := strings.LastIndexByte(rest, ')'); i >= 0 { + rest = rest[i+1:] + } + fields := strings.Fields(rest) + if len(fields) < 20 { + return 0 + } + v, err := strconv.ParseInt(fields[19], 10, 64) + if err != nil { + return 0 + } + return v +} diff --git a/internal/session/proc_start_other.go b/internal/session/proc_start_other.go new file mode 100644 index 0000000..9907f7c --- /dev/null +++ b/internal/session/proc_start_other.go @@ -0,0 +1,7 @@ +//go:build !linux && !darwin + +package session + +// procStartTime reports that stable process identity is unavailable. Display +// liveness remains permissive, while PID-based fallback signaling fails closed. +func procStartTime(int) int64 { return 0 } diff --git a/internal/session/session.go b/internal/session/session.go index c4f08c4..7cd53b8 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -15,7 +15,6 @@ import ( "path/filepath" "regexp" "strconv" - "strings" "syscall" ) @@ -115,9 +114,9 @@ func verifyDirIdentity(dir string) error { type State struct { Name string `json:"name"` HostPID int `json:"host_pid"` - // HostStart / ChildStart are the processes' kernel start times - // (/proc//stat field 22, in clock ticks since boot; 0 where - // unavailable). They disambiguate a recycled PID from the original + // HostStart / ChildStart are platform-specific stable process identities + // derived from kernel start times (0 where unavailable). They disambiguate + // a recycled PID from the original // process, so a stale state file can never make uam treat — or worse, // signal — an unrelated process as a session. HostStart int64 `json:"host_start,omitempty"` @@ -226,9 +225,10 @@ func ProcAlive(pid int) bool { } // procAliveWithStart is ProcAlive hardened against PID reuse: when a start -// time was recorded AND the live process's start time is readable, they must -// match. Where /proc is unavailable (e.g. macOS) either side reads as 0 and -// the check degrades to the plain signal-0 probe. +// identity was recorded AND the live process's identity is readable, they +// must match. This intentionally remains permissive for display compatibility: +// missing identity degrades to the plain signal-0 probe. Signaling paths use +// procIdentityMatches instead. func procAliveWithStart(pid int, start int64) bool { if !ProcAlive(pid) { return false @@ -240,32 +240,15 @@ func procAliveWithStart(pid int, start int64) bool { return current == 0 || current == start } -// procStartTime returns the kernel start time of pid (clock ticks since boot, -// /proc//stat field 22), or 0 when unavailable. -func procStartTime(pid int) int64 { - if pid <= 0 { - return 0 - } - data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) - if err != nil { - return 0 - } - // comm (field 2) is parenthesized and may itself contain spaces or ')', - // so split after the LAST ')'. starttime is overall field 22, i.e. index - // 19 of the fields that follow comm. - rest := string(data) - if i := strings.LastIndexByte(rest, ')'); i >= 0 { - rest = rest[i+1:] - } - fields := strings.Fields(rest) - if len(fields) < 20 { - return 0 - } - v, err := strconv.ParseInt(fields[19], 10, 64) - if err != nil { - return 0 +// procIdentityMatches is the fail-closed process identity check used before +// PID-based fallback signaling. Both recorded and live identities must be +// available and equal; liveness alone is never authorization to signal. +func procIdentityMatches(pid int, recorded int64) bool { + if pid <= 0 || recorded == 0 || !ProcAlive(pid) { + return false } - return v + current := procStartTime(pid) + return current != 0 && current == recorded } // procCwd returns the live working directory of pid via /proc (Linux). On diff --git a/internal/store/store.go b/internal/store/store.go index 2b143d0..732d825 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -147,9 +147,11 @@ func (c *Config) UnmarshalJSON(data []byte) error { } type UISettings struct { - GroupByDir bool `json:"group_by_dir"` - Sort string `json:"sort"` - PeekWidth int `json:"peek_width"` + GroupByDir bool `json:"group_by_dir"` + // Sort and PeekWidth are retained schema-v3 compatibility fields. They are + // normalized and round-tripped even when the TUI exposes no direct control. + Sort string `json:"sort"` + PeekWidth int `json:"peek_width"` } type SessionRecord struct { From 8650e168d0f50e58c8cbc52f2fb0e8f5e83e4f58 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:41:18 +0000 Subject: [PATCH 08/22] test(cli): create owner-only runtime fixtures --- internal/cli/cli_test.go | 15 ++++++++++++--- internal/cli/killall_test.go | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index bee254c..8f712cf 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -155,7 +155,7 @@ func TestRunWithTUIStateFreeCommandsDoNotOpenStore(t *testing.T) { t.Fatal(err) } t.Setenv("UAM_CONFIG_DIR", blocked) - t.Setenv("UAM_SESSION_DIR", t.TempDir()) + t.Setenv("UAM_SESSION_DIR", secureSessionDir(t)) for _, args := range [][]string{{"help"}, {"version"}, {"kill-all"}} { if err := RunWithTUI(context.Background(), args, noopRunTUI); err != nil { @@ -193,7 +193,7 @@ func TestMainFallsBackToStderrWhenFileLoggerFails(t *testing.T) { t.Fatal(err) } cmd := cliMainSubprocess(t, "kill-all", blocked, filepath.Join(t.TempDir(), "config")) - cmd.Env = append(cmd.Env, "UAM_SESSION_DIR="+t.TempDir()) + cmd.Env = append(cmd.Env, "UAM_SESSION_DIR="+secureSessionDir(t)) output, err := cmd.CombinedOutput() if err != nil { t.Fatalf("uam kill-all must continue with stderr logging: %v\n%s", err, output) @@ -378,6 +378,15 @@ func writeCLIExecutable(t *testing.T, path string) { } } +func secureSessionDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.Chmod(dir, 0o700); err != nil { + t.Fatal(err) + } + return dir +} + func must(t *testing.T, err error) { t.Helper() if err != nil { @@ -397,7 +406,7 @@ func firstNonEmpty(values ...string) string { // The internal __host/__attach subcommands are routed through runCommand; // invalid input must surface as errors rather than silently doing nothing. func TestRunCommandInternalSubcommands(t *testing.T) { - t.Setenv("UAM_SESSION_DIR", t.TempDir()) + t.Setenv("UAM_SESSION_DIR", secureSessionDir(t)) svc, _ := newCLITestService(t) noTUI := func(context.Context, tea.Model) error { return nil } if err := runCommand(context.Background(), svc, []string{"__host", "--name", "bad name", "--", "/bin/true"}, noTUI); err == nil { diff --git a/internal/cli/killall_test.go b/internal/cli/killall_test.go index b5bb76a..0ea7025 100644 --- a/internal/cli/killall_test.go +++ b/internal/cli/killall_test.go @@ -37,7 +37,7 @@ func TestRunKillAllPropagatesError(t *testing.T) { // real sessions: KillAll over zero sessions is an idempotent success. func TestRunCommandKillAllDispatches(t *testing.T) { svc, _ := newCLITestService(t) - t.Setenv("UAM_SESSION_DIR", t.TempDir()) + t.Setenv("UAM_SESSION_DIR", secureSessionDir(t)) out := captureCLIStdout(t, func() { if err := runCommand(context.Background(), svc, []string{"kill-all"}, func(context.Context, tea.Model) error { return nil }); err != nil { From 0f62d90a13771276a75263e033a17cf2b121b131 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 10:02:49 +0000 Subject: [PATCH 09/22] test: bound in-process host readiness --- internal/session/host_inprocess_test.go | 32 +++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/internal/session/host_inprocess_test.go b/internal/session/host_inprocess_test.go index 1a39ecb..3d824d4 100644 --- a/internal/session/host_inprocess_test.go +++ b/internal/session/host_inprocess_test.go @@ -3,6 +3,7 @@ package session import ( "bufio" "context" + "errors" "net" "os" "strings" @@ -20,14 +21,37 @@ func startInProcessHost(t *testing.T, c *Client, name, command string) chan erro if err != nil { t.Fatal(err) } + cwd := t.TempDir() done := make(chan error, 1) go func() { - done <- runHost(c.Dir, name, t.TempDir(), "label0", []string{"UAM_T=1"}, []string{"/bin/sh", "-c", command}, w) + defer func() { _ = w.Close() }() + done <- runHost(c.Dir, name, cwd, "label0", []string{"UAM_T=1"}, []string{"/bin/sh", "-c", command}, w) }() - line, err := bufio.NewReader(r).ReadString('\n') + type readyResult struct { + line string + err error + } + ready := make(chan readyResult, 1) + go func() { + line, err := bufio.NewReader(r).ReadString('\n') + ready <- readyResult{line: line, err: err} + }() + var result readyResult + select { + case result = <-ready: + case <-time.After(10 * time.Second): + _ = r.Close() + t.Fatal("timed out waiting for host readiness") + } _ = r.Close() - if err != nil || strings.TrimSpace(line) != "ok" { - t.Fatalf("host not ready: %q %v", line, err) + if result.err != nil || strings.TrimSpace(result.line) != "ok" { + var hostErr error + select { + case hostErr = <-done: + case <-time.After(time.Second): + hostErr = errors.New("host did not return after closing readiness pipe") + } + t.Fatalf("host not ready: %q (read: %v, host: %v)", result.line, result.err, hostErr) } return done } From a41c5283ae64b3b596c66821b26a3e11a58efd01 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 10:04:37 +0000 Subject: [PATCH 10/22] test: use short Darwin session paths --- internal/session/session_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/session/session_test.go b/internal/session/session_test.go index e7251bb..5201d90 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -30,7 +30,15 @@ func TestMain(m *testing.M) { func newTestClient(t *testing.T) *Client { t.Helper() - dir := filepath.Join(t.TempDir(), "run") + dir, err := os.MkdirTemp("", "uam-test-") + if err != nil { + t.Fatal(err) + } + if err := os.Chmod(dir, 0o700); err != nil { + _ = os.RemoveAll(dir) + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) t.Setenv("UAM_CONFIG_DIR", filepath.Join(t.TempDir(), "cfg")) exe, err := os.Executable() if err != nil { From c352dfe5b8f79b60d71b9e8146d7ed627d94e09c Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:00:40 +0000 Subject: [PATCH 11/22] build: use Go 1.25.12 toolchain --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index aba81bd..dc63bdd 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/RandomCodeSpace/unified-agent-manager go 1.25.0 -toolchain go1.25.11 +toolchain go1.25.12 require ( github.com/charmbracelet/bubbletea v1.3.10 From 8c98d102e0bab51aaedc5eab98175550d84f4012 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:01:17 +0000 Subject: [PATCH 12/22] chore(deps): upgrade x/term to 0.2.2 --- go.mod | 4 ++-- go.sum | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index dc63bdd..c218439 100644 --- a/go.mod +++ b/go.mod @@ -7,9 +7,10 @@ toolchain go1.25.12 require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 - github.com/charmbracelet/x/term v0.2.1 + github.com/charmbracelet/x/term v0.2.2 github.com/creack/pty v1.1.24 github.com/mattn/go-runewidth v0.0.16 + golang.org/x/sys v0.36.0 ) require ( @@ -26,6 +27,5 @@ require ( github.com/muesli/termenv v0.16.0 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect - golang.org/x/sys v0.36.0 // indirect golang.org/x/text v0.3.8 // indirect ) diff --git a/go.sum b/go.sum index c6551b4..58b52be 100644 --- a/go.sum +++ b/go.sum @@ -10,8 +10,8 @@ github.com/charmbracelet/x/ansi v0.10.1 h1:rL3Koar5XvX0pHGfovN03f5cxLbCF2YvLeyz7 github.com/charmbracelet/x/ansi v0.10.1/go.mod h1:3RQDQ6lDnROptfpWuUVIUG64bD2g2BgntdxH0Ya5TeE= github.com/charmbracelet/x/cellbuf v0.0.13-0.20250311204145-2c3ea96c31dd h1:vy0GVL4jeHEwG5YOXDmi86oYw2yuYUGqz6a8sLwg0X8= github.com/charmbracelet/x/cellbuf v0.0.13-0.20250311204145-2c3ea96c31dd/go.mod h1:xe0nKWGd3eJgtqZRaN9RjMtK7xUYchjzPr7q6kcvCCs= -github.com/charmbracelet/x/term v0.2.1 h1:AQeHeLZ1OqSXhrAWpYUtZyX1T3zVxfpZuEQMIQaGIAQ= -github.com/charmbracelet/x/term v0.2.1/go.mod h1:oQ4enTYFV7QN4m0i9mzHrViD7TQKvNEEkHUMCmsxdUg= +github.com/charmbracelet/x/term v0.2.2 h1:xVRT/S2ZcKdhhOuSP4t5cLi5o+JxklsoEObBSgfgZRk= +github.com/charmbracelet/x/term v0.2.2/go.mod h1:kF8CY5RddLWrsgVwpw4kAa6TESp6EB5y3uxGLeCqzAI= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f h1:Y/CXytFA4m6baUTXGLOoWe4PQhGxaX0KpnayAqC48p4= From e3f2aaa5295113ce07cd8708ef5977bb7f8d6bd7 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:01:52 +0000 Subject: [PATCH 13/22] chore(deps): upgrade go-runewidth to 0.0.24 --- go.mod | 3 ++- go.sum | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index c218439..4d29612 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/charmbracelet/lipgloss v1.1.0 github.com/charmbracelet/x/term v0.2.2 github.com/creack/pty v1.1.24 - github.com/mattn/go-runewidth v0.0.16 + github.com/mattn/go-runewidth v0.0.24 golang.org/x/sys v0.36.0 ) @@ -18,6 +18,7 @@ require ( github.com/charmbracelet/colorprofile v0.2.3-0.20250311203215-f60798e515dc // indirect github.com/charmbracelet/x/ansi v0.10.1 // indirect github.com/charmbracelet/x/cellbuf v0.0.13-0.20250311204145-2c3ea96c31dd // indirect + github.com/clipperhouse/uax29/v2 v2.2.0 // indirect github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect github.com/lucasb-eyer/go-colorful v1.2.0 // indirect github.com/mattn/go-isatty v0.0.20 // indirect diff --git a/go.sum b/go.sum index 58b52be..b1e0044 100644 --- a/go.sum +++ b/go.sum @@ -12,6 +12,8 @@ github.com/charmbracelet/x/cellbuf v0.0.13-0.20250311204145-2c3ea96c31dd h1:vy0G github.com/charmbracelet/x/cellbuf v0.0.13-0.20250311204145-2c3ea96c31dd/go.mod h1:xe0nKWGd3eJgtqZRaN9RjMtK7xUYchjzPr7q6kcvCCs= github.com/charmbracelet/x/term v0.2.2 h1:xVRT/S2ZcKdhhOuSP4t5cLi5o+JxklsoEObBSgfgZRk= github.com/charmbracelet/x/term v0.2.2/go.mod h1:kF8CY5RddLWrsgVwpw4kAa6TESp6EB5y3uxGLeCqzAI= +github.com/clipperhouse/uax29/v2 v2.2.0 h1:ChwIKnQN3kcZteTXMgb1wztSgaU+ZemkgWdohwgs8tY= +github.com/clipperhouse/uax29/v2 v2.2.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f h1:Y/CXytFA4m6baUTXGLOoWe4PQhGxaX0KpnayAqC48p4= @@ -22,15 +24,14 @@ github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWE github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-localereader v0.0.1 h1:ygSAOl7ZXTx4RdPYinUpg6W99U8jWvWi9Ye2JC/oIi4= github.com/mattn/go-localereader v0.0.1/go.mod h1:8fBrzywKY7BI3czFoHkuzRoWE9C+EiG4R1k4Cjx5p88= -github.com/mattn/go-runewidth v0.0.16 h1:E5ScNMtiwvlvB5paMFdw9p4kSQzbXFikJ5SQO6TULQc= -github.com/mattn/go-runewidth v0.0.16/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= +github.com/mattn/go-runewidth v0.0.24 h1:cpokDiIn0MGnhdHwuWnJBITySJ20QyNGnY2kR/ay2DU= +github.com/mattn/go-runewidth v0.0.24/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 h1:ZK8zHtRHOkbHy6Mmr5D264iyp3TiX5OmNcI5cIARiQI= github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6/go.mod h1:CJlz5H+gyd6CUWT45Oy4q24RdLyn7Md9Vj2/ldJBSIo= github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELUXHmA= github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo= github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc= github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= -github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= From d38383fb4e7d184ac5d2d2ae06b8c0a0f4b2e65e Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:08:53 +0000 Subject: [PATCH 14/22] test: add fuzz and release smoke coverage --- .github/workflows/fuzz.yml | 40 ++++++++++++++ .github/workflows/release-smoke.yml | 75 ++++++++++++++++++++++++++ internal/adapter/registry_test.go | 20 +++++++ internal/app/pr_refresh_test.go | 42 +++++++++++++++ internal/app/render_cluster_test.go | 21 ++++++++ internal/session/attach_filter_test.go | 13 +++++ internal/session/liveness_test.go | 29 ++++++++++ internal/session/proto_test.go | 25 +++++++++ internal/vterm/vterm_test.go | 30 +++++++++++ 9 files changed, 295 insertions(+) create mode 100644 .github/workflows/fuzz.yml create mode 100644 .github/workflows/release-smoke.yml diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml new file mode 100644 index 0000000..e1565b1 --- /dev/null +++ b/.github/workflows/fuzz.yml @@ -0,0 +1,40 @@ +name: Scheduled fuzzing + +on: + schedule: + - cron: "41 4 * * 2" + workflow_dispatch: + +permissions: + contents: read + +jobs: + fuzz: + name: fuzz-${{ matrix.target }} + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - package: ./internal/session + target: FuzzFrameDecoding + - package: ./internal/vterm + target: FuzzTerminalInput + - package: ./internal/displaytext + target: FuzzSanitize + - package: ./internal/session + target: FuzzAttachFiltering + - package: ./internal/session + target: FuzzStateDecoding + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Set up Go + uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 + with: + go-version-file: go.mod + cache: true + + - name: Fuzz for 30 seconds + run: go test "${{ matrix.package }}" -run=^$ -fuzz="^${{ matrix.target }}$" -fuzztime=30s diff --git a/.github/workflows/release-smoke.yml b/.github/workflows/release-smoke.yml new file mode 100644 index 0000000..6af1e4a --- /dev/null +++ b/.github/workflows/release-smoke.yml @@ -0,0 +1,75 @@ +name: Release smoke + +on: + push: + branches: [main] + pull_request: + workflow_dispatch: + +permissions: + contents: read + +jobs: + snapshot: + name: GoReleaser snapshot + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 + with: + go-version-file: go.mod + cache: true + + - name: Validate GoReleaser configuration + uses: goreleaser/goreleaser-action@f06c13b6b1a9625abc9e6e439d9c05a8f2190e94 # v7.2.3 + with: + distribution: goreleaser + version: v2.17.0 + args: check + + - name: Build snapshot archives + uses: goreleaser/goreleaser-action@f06c13b6b1a9625abc9e6e439d9c05a8f2190e94 # v7.2.3 + with: + distribution: goreleaser + version: v2.17.0 + args: release --snapshot --clean --skip=sign,sbom + + - name: Verify archive names and contents + shell: bash + run: | + mapfile -t archives < <(find dist -maxdepth 1 -type f -name 'uam_*.tar.gz' -print | sort) + test "${#archives[@]}" -eq 4 + for archive in "${archives[@]}"; do + basename "${archive}" | grep -Eq '^uam_.+_(linux|darwin)_(amd64|arm64)\.tar\.gz$' + tar -tzf "${archive}" | grep -Eq '(^|/)uam$' + done + linux_archive="$(find dist -maxdepth 1 -type f -name 'uam_*_linux_amd64.tar.gz' -print -quit)" + test -n "${linux_archive}" + mkdir -p /tmp/uam-release-smoke + tar -xzf "${linux_archive}" -C /tmp/uam-release-smoke + binary="$(find /tmp/uam-release-smoke -type f -name uam -print -quit)" + test -n "${binary}" + "${binary}" version | grep -E '.+' + + darwin-native: + name: Darwin-native version smoke + runs-on: macos-14 + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Set up Go + uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 + with: + go-version-file: go.mod + cache: true + + - name: Build and run version command + run: | + go build -o "${RUNNER_TEMP}/uam" ./cmd/uam + "${RUNNER_TEMP}/uam" version | grep -E '.+' diff --git a/internal/adapter/registry_test.go b/internal/adapter/registry_test.go index c4dd1e7..10fad55 100644 --- a/internal/adapter/registry_test.go +++ b/internal/adapter/registry_test.go @@ -2,6 +2,7 @@ package adapter import ( "context" + "fmt" "testing" "time" @@ -57,6 +58,25 @@ func TestRegistryListAllUsesSingleBackendSnapshot(t *testing.T) { } } +func BenchmarkRegistryListAllSharedSnapshot(b *testing.B) { + infos := make([]session.Info, 300) + for i := range infos { + infos[i] = session.Info{Name: fmt.Sprintf("uam-a-%08x", i+1), CreatedUnix: time.Now().Unix(), Alive: true} + } + backend := &adaptertest.Backend{Sessions: infos} + adapters := make([]AgentAdapter, 0, 6) + for _, name := range []string{"a", "b", "c", "d", "e", "f"} { + adapters = append(adapters, NewAgent(name, name, []CommandCandidate{{Display: "sh", Args: []string{"/bin/sh"}}}, nil, backend)) + } + registry := NewRegistryWithBackend(backend, adapters) + b.ResetTimer() + for b.Loop() { + if _, err := registry.ListAll(context.Background()); err != nil { + b.Fatal(err) + } + } +} + type fakeAdapter struct { name string available bool diff --git a/internal/app/pr_refresh_test.go b/internal/app/pr_refresh_test.go index 5cb8408..2a48e21 100644 --- a/internal/app/pr_refresh_test.go +++ b/internal/app/pr_refresh_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "path/filepath" "sync" "sync/atomic" "testing" @@ -168,3 +169,44 @@ func TestAdapterStatusMapping(t *testing.T) { } } } + +func BenchmarkRefreshPRStatuses(b *testing.B) { + sessions := make([]adapter.Session, 20) + for i := range sessions { + id := fmt.Sprintf("%08x", i+1) + sessions[i] = adapter.Session{ + ID: id, AgentType: "fake", DisplayName: id, Cwd: "/tmp", + SessionName: "uam-fake-" + id, State: adapter.Active, ProcAlive: adapter.Alive, + CreatedAt: time.Now(), PR: &adapter.PRRef{URL: fmt.Sprintf("https://github.com/o/r/pull/%d", i+1), Number: i + 1, Status: adapter.PROpen}, + } + } + st, err := store.Open(filepath.Join(b.TempDir(), "sessions.json")) + if err != nil { + b.Fatal(err) + } + fake := &svcFakeAdapter{name: "fake", available: true, sessions: sessions} + svc := NewService(st, adapter.NewRegistry([]adapter.AgentAdapter{fake})) + svc.checkPR = func(context.Context, string) (pr.Status, error) { return pr.Open, nil } + if _, _, err := svc.LoadSessions(context.Background()); err != nil { + b.Fatal(err) + } + b.ResetTimer() + for b.Loop() { + b.StopTimer() + if err := st.Update(func(cfg *store.Config) error { + for key, rec := range cfg.Sessions { + if rec.PR != nil { + rec.PR.LastChecked = time.Time{} + cfg.Sessions[key] = rec + } + } + return nil + }); err != nil { + b.Fatal(err) + } + b.StartTimer() + if err := svc.RefreshPRStatuses(context.Background()); err != nil { + b.Fatal(err) + } + } +} diff --git a/internal/app/render_cluster_test.go b/internal/app/render_cluster_test.go index 75dbcac..0a96f82 100644 --- a/internal/app/render_cluster_test.go +++ b/internal/app/render_cluster_test.go @@ -75,6 +75,27 @@ func TestTruncateIsWidthAware(t *testing.T) { } } +func TestTruncateUnicodeGolden(t *testing.T) { + for _, tc := range []struct { + name string + in string + cols int + want string + }{ + {name: "emoji", in: "🚀🚀🚀", cols: 5, want: "🚀🚀…"}, + {name: "combining", in: "e\u0301clair", cols: 3, want: "e\u0301c…"}, + {name: "cjk", in: "你好世界", cols: 5, want: "你好…"}, + {name: "narrow", in: "anything", cols: 1, want: "…"}, + {name: "pasted-multibyte-name", in: "部署 café 🚀", cols: 10, want: "部署 café…"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := truncate(tc.in, tc.cols); got != tc.want { + t.Fatalf("truncate(%q, %d) = %q, want %q", tc.in, tc.cols, got, tc.want) + } + }) + } +} + // F28 — truncate must not chop a multibyte rune mid-sequence (the old byte-slice // path produced mojibake on the boundary). func TestTruncateNeverEmitsMojibake(t *testing.T) { diff --git a/internal/session/attach_filter_test.go b/internal/session/attach_filter_test.go index ecbae72..d1c7a3f 100644 --- a/internal/session/attach_filter_test.go +++ b/internal/session/attach_filter_test.go @@ -21,6 +21,19 @@ func runFilter(t *testing.T, f *stdinFilter, chunks ...string) (string, bool) { return out.String(), false } +func FuzzAttachFiltering(f *testing.F) { + for _, seed := range []string{"plain text", "世界🚀", "\x02d", "\x1b[D", "\x1b]11;rgb:ffff/ffff/ffff\x1b\\"} { + f.Add(seed, true) + } + f.Fuzz(func(t *testing.T, input string, backDetach bool) { + filter := &stdinFilter{backDetach: backDetach} + out, _ := filter.filter([]byte(input)) + if len(out) > 2*len(input)+2 { + t.Fatalf("filter expanded %d input bytes to %d bytes", len(input), len(out)) + } + }) +} + func TestLeftArrowDetachesWhenNothingTyped(t *testing.T) { f := &stdinFilter{backDetach: true} out, detach := runFilter(t, f, "\x1b[D") diff --git a/internal/session/liveness_test.go b/internal/session/liveness_test.go index b311480..48475aa 100644 --- a/internal/session/liveness_test.go +++ b/internal/session/liveness_test.go @@ -2,6 +2,7 @@ package session import ( "context" + "encoding/json" "errors" "os" "os/exec" @@ -12,6 +13,34 @@ import ( "testing" ) +func FuzzStateDecoding(f *testing.F) { + for _, seed := range [][]byte{ + []byte(`{"name":"uam-fake-aabbccdd","host_pid":1,"host_start":2}`), + []byte(`{"name":"../escape","host_pid":1}`), + []byte(`null`), + []byte(`{"unknown":{"nested":true}}`), + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, data []byte) { + var state State + if err := json.Unmarshal(data, &state); err != nil { + return + } + encoded, err := json.Marshal(state) + if err != nil { + t.Fatal(err) + } + var decoded State + if err := json.Unmarshal(encoded, &decoded); err != nil { + t.Fatal(err) + } + if decoded.Name != state.Name || decoded.HostPID != state.HostPID || decoded.HostStart != state.HostStart { + t.Fatalf("state round trip changed identity: before=%+v after=%+v", state, decoded) + } + }) +} + func TestVerifyDirRejectsUnsafeRuntimeDirectories(t *testing.T) { parent := t.TempDir() diff --git a/internal/session/proto_test.go b/internal/session/proto_test.go index 87f9ac8..24f9882 100644 --- a/internal/session/proto_test.go +++ b/internal/session/proto_test.go @@ -84,3 +84,28 @@ func TestFrameWriterSerializesConcurrentFrames(t *testing.T) { t.Fatalf("trailing read error = %v, want EOF", err) } } + +func FuzzFrameDecoding(f *testing.F) { + for _, seed := range [][]byte{ + {frameDetach, 0, 0, 0, 0}, + {frameStdin, 0, 0, 0, 3, 'a', 'b', 'c'}, + {frameResize, 0, 0, 0, 4, 0, 80, 0, 24}, + {frameStdin, 0xff, 0xff, 0xff, 0xff}, + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, data []byte) { + kind, payload, err := readFrame(bytes.NewReader(data)) + if err != nil { + return + } + var roundTrip bytes.Buffer + if err := writeFrame(&roundTrip, kind, payload); err != nil { + t.Fatal(err) + } + gotKind, gotPayload, err := readFrame(&roundTrip) + if err != nil || gotKind != kind || !bytes.Equal(gotPayload, payload) { + t.Fatalf("frame round trip = (%d, %x, %v), want (%d, %x)", gotKind, gotPayload, err, kind, payload) + } + }) +} diff --git a/internal/vterm/vterm_test.go b/internal/vterm/vterm_test.go index 16257d8..9824783 100644 --- a/internal/vterm/vterm_test.go +++ b/internal/vterm/vterm_test.go @@ -372,6 +372,36 @@ func BenchmarkHostileCSICounts(b *testing.B) { } } +func BenchmarkOrdinaryTerminalStream(b *testing.B) { + stream := []byte("\x1b[32mready\x1b[0m> go test ./...\r\nok package 0.123s\r\n") + term := New(120, 40, 500) + b.SetBytes(int64(len(stream))) + b.ResetTimer() + for b.Loop() { + _, _ = term.Write(stream) + } +} + +func FuzzTerminalInput(f *testing.F) { + for _, seed := range [][]byte{ + []byte("plain\r\ntext"), + []byte("café 世界 🚀"), + []byte("\x1b[999999999999999999999S"), + []byte("\x1b]0;title\x07visible"), + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, input []byte) { + term := New(80, 24, 100) + if _, err := term.Write(input); err != nil { + t.Fatal(err) + } + if got := term.Capture(100); len(got) > 80*(24+100)*4 { + t.Fatalf("capture exceeded screen/history bound: %d bytes", len(got)) + } + }) +} + func TestScrollRegionReverseWrapAndKeypadIgnored(t *testing.T) { term := New(20, 4, 0) // Keypad mode escapes and charset designators are consumed silently. From 5b938d1e1237dcc96190143687a1da17437ceef9 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:54:59 +0000 Subject: [PATCH 15/22] ci: bound Darwin lifecycle smoke tests --- .github/workflows/release-smoke.yml | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/.github/workflows/release-smoke.yml b/.github/workflows/release-smoke.yml index 6af1e4a..4590d20 100644 --- a/.github/workflows/release-smoke.yml +++ b/.github/workflows/release-smoke.yml @@ -9,6 +9,10 @@ on: permissions: contents: read +concurrency: + group: release-smoke-${{ github.event.pull_request.head.ref || github.ref_name }} + cancel-in-progress: true + jobs: snapshot: name: GoReleaser snapshot @@ -57,8 +61,9 @@ jobs: "${binary}" version | grep -E '.+' darwin-native: - name: Darwin-native version smoke + name: Darwin-native lifecycle and version smoke runs-on: macos-14 + timeout-minutes: 8 steps: - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -69,6 +74,12 @@ jobs: go-version-file: go.mod cache: true + - name: Run native session lifecycle tests + run: go test -race -count=1 -timeout=4m ./internal/session + + - name: Run stop, restart, and resume tests + run: go test -race -count=1 -timeout=2m ./internal/app + - name: Build and run version command run: | go build -o "${RUNNER_TEMP}/uam" ./cmd/uam From a0187b3cb5dfc2fe238bdf10592b83fc4190099f Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:14:19 +0000 Subject: [PATCH 16/22] ci: enforce least-privilege release gates --- .github/workflows/ci.yml | 69 ++++++++++++++++++++++++++-------- .github/workflows/release.yml | 57 ++++++++++++++++++++++++---- .github/workflows/security.yml | 46 ++++++++--------------- .github/workflows/sonar.yml | 21 +++-------- README.md | 10 +++-- 5 files changed, 131 insertions(+), 72 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0473cc4..559a197 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,44 +3,83 @@ name: CI on: push: pull_request: + workflow_call: + workflow_dispatch: permissions: contents: read jobs: - build-test-lint: - name: Build, test, and lint + build: + name: build runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Go - uses: actions/setup-go@v5 + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 with: go-version-file: go.mod cache: true + - name: Verify modules + run: go mod verify + - name: Build run: go build ./... - - name: Vet - run: go vet ./... + test-race: + name: test-race + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + + - name: Set up Go + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + with: + go-version-file: go.mod + cache: true + + - name: Test with race detector and coverage + run: go test -race -count=1 -covermode=atomic -coverpkg=./... -coverprofile=coverage.out ./... + + - name: Enforce coverage floor + shell: bash + run: | + coverage="$(go tool cover -func=coverage.out | awk '$1 == "total:" {gsub("%", "", $3); print $3}')" + awk -v coverage="${coverage}" 'BEGIN { if (coverage + 0 < 88.1) { printf "coverage %.1f%% is below 88.1%%\n", coverage; exit 1 } }' + + lint: + name: lint + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + + - name: Set up Go + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + with: + go-version-file: go.mod + cache: true - name: Check gofmt + shell: bash run: | unformatted="$(gofmt -l . | grep -v '^vendor/' || true)" - if [ -n "$unformatted" ]; then - echo "The following files are not gofmt-clean:" - echo "$unformatted" - exit 1 - fi + test -z "${unformatted}" || { printf 'The following files are not gofmt-clean:\n%s\n' "${unformatted}"; exit 1; } + + - name: Vet + run: go vet ./... - - name: Test - run: go test -race -cover ./... + - name: Staticcheck + run: | + go install honnef.co/go/tools/cmd/staticcheck@2025.1.1 + staticcheck ./... - - name: Lint - uses: golangci/golangci-lint-action@9fae48acfc02a90574d7c304a1758ef9895495fa # v7 + - name: GolangCI-Lint + uses: golangci/golangci-lint-action@9fae48acfc02a90574d7c304a1758ef9895495fa # v7.0.1 with: version: v2.5.0 install-mode: goinstall diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c0d2cc6..bb8a8dc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -6,22 +6,63 @@ on: - "v*" permissions: - contents: write - id-token: write - packages: read + contents: read jobs: - goreleaser: - name: GoReleaser + quality: + name: Quality verification + uses: ./.github/workflows/ci.yml + + security: + name: Security verification + uses: ./.github/workflows/security.yml + + ancestry: + name: Verify tag is on main + runs-on: ubuntu-latest + steps: + - name: Checkout tagged commit + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + fetch-depth: 0 + + - name: Verify tagged commit is an ancestor of main + run: | + git fetch --no-tags origin main + git merge-base --is-ancestor "${GITHUB_SHA}" origin/main + + release-config: + name: Validate release configuration + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + fetch-depth: 0 + + - name: Validate GoReleaser configuration + uses: goreleaser/goreleaser-action@e435ccd777264be153ace6237001ef4d979d3a7a # v6.4.0 + with: + version: "~> v2" + args: check + + publish: + name: Publish release + needs: [quality, security, ancestry, release-config] runs-on: ubuntu-latest + environment: release + permissions: + contents: write + id-token: write + packages: read steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: fetch-depth: 0 - name: Set up Go - uses: actions/setup-go@v5 + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 with: go-version-file: go.mod cache: true @@ -33,7 +74,7 @@ jobs: uses: sigstore/cosign-installer@dc72c7d5c4d10cd6bcb8cf6e3fd625a9e5e537da # v3.7.0 - name: Run GoReleaser - uses: goreleaser/goreleaser-action@e435ccd777264be153ace6237001ef4d979d3a7a # v6 + uses: goreleaser/goreleaser-action@e435ccd777264be153ace6237001ef4d979d3a7a # v6.4.0 with: version: "~> v2" args: release --clean diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 8d43616..3af1054 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -4,72 +4,58 @@ on: push: branches: [main] pull_request: - branches: [main] schedule: - cron: "23 3 * * 1" + workflow_call: workflow_dispatch: permissions: contents: read - actions: read - security-events: write - pull-requests: read jobs: govulncheck: - name: Go vulnerability check + name: govulncheck runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Go - uses: actions/setup-go@v5 + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 with: go-version-file: go.mod cache: true - - name: Install govulncheck - run: go install golang.org/x/vuln/cmd/govulncheck@v1.3.0 - - name: Run govulncheck - run: govulncheck ./... + run: | + go install golang.org/x/vuln/cmd/govulncheck@v1.3.0 + govulncheck ./... gosec: - name: Gosec static analysis + name: gosec runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Go - uses: actions/setup-go@v5 + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 with: go-version-file: go.mod cache: true - - name: Install gosec - run: go install github.com/securego/gosec/v2/cmd/gosec@v2.26.1 - - name: Run gosec - run: gosec -exclude-dir=.claude -exclude-dir=vendor -fmt=sarif -out=gosec.sarif ./... - - - name: Upload gosec SARIF - if: always() - uses: github/codeql-action/upload-sarif@v3 - with: - sarif_file: gosec.sarif + run: | + go install github.com/securego/gosec/v2/cmd/gosec@v2.26.1 + gosec -exclude-dir=.claude -exclude-dir=vendor ./... dependency-review: - name: Dependency review + name: dependency-review if: github.event_name == 'pull_request' runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: read steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Review dependency changes - uses: actions/dependency-review-action@v4 + uses: actions/dependency-review-action@3c4e3dcb1aa7874d2c16be7d79418e9b7efd6261 # v4.8.2 diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index 2505e4b..5b39293 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -3,9 +3,8 @@ name: SonarCloud on: push: branches: [main] - pull_request: - branches: [main] - workflow_dispatch: + schedule: + - cron: "17 5 * * 3" permissions: contents: read @@ -15,16 +14,14 @@ jobs: sonarcloud: name: SonarCloud scan runs-on: ubuntu-latest - env: - SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: fetch-depth: 0 - name: Set up Go - uses: actions/setup-go@v5 + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 with: go-version-file: go.mod cache: true @@ -34,15 +31,7 @@ jobs: - name: Run SonarCloud scan if: env.SONAR_TOKEN != '' - uses: SonarSource/sonarqube-scan-action@v6 + uses: SonarSource/sonarqube-scan-action@fd88b7d7ccbaefd23d8f36f73b59db7a3d246602 # v6.0.0 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} - - - name: Explain skipped SonarCloud scan - if: env.SONAR_TOKEN == '' - run: | - echo "SONAR_TOKEN is not configured, so the SonarCloud scan was skipped." - echo "Add a repository secret named SONAR_TOKEN to enable this check." - env: - SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} diff --git a/README.md b/README.md index 5b8e8af..02750eb 100644 --- a/README.md +++ b/README.md @@ -189,9 +189,13 @@ run `loginctl enable-linger`. ## Safety model -`uam` can launch providers in their full-access or auto-approve mode when the -provider supports it. Use `uam dispatch --safe ...` when you want the provider's -default approval behavior instead. +`uam` launches providers in their full-access or auto-approve ("yolo") mode by +default when the provider supports it. In that mode, treat the repository, +prompt, provider configuration, and any instructions the agent reads as trusted: +the provider may execute commands and change files without pausing for approval. +Use `uam dispatch --safe ...` when you want the provider's default approval +behavior instead. Safe mode changes provider arguments; it is not an operating- +system sandbox and does not reduce the permissions of the `uam` process itself. `uam` does not make git checkpoints, stash changes, or modify your repository on its own. It starts and manages agent sessions; the provider remains responsible From 3f2f08d11b3d889443b17ce473cbe86586839a60 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:21:47 +0000 Subject: [PATCH 17/22] test: restore coverage and remove duplicate routing --- internal/cli/cli.go | 15 --------------- internal/cli/cli_test.go | 10 ++++------ internal/cli/killall_test.go | 9 +++------ internal/log/log_test.go | 7 +++++++ internal/session/host.go | 9 ++------- internal/session/session_test.go | 8 ++++++++ internal/store/store_test.go | 3 +++ 7 files changed, 27 insertions(+), 34 deletions(-) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 1df5d85..58c0c1d 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -145,14 +145,8 @@ func runWithoutStore(ctx context.Context, args []string) (bool, error) { func runCommand(ctx context.Context, svc *app.Service, args []string, runTUI func(context.Context, tea.Model) error) error { switch args[0] { - case "-h", "--help", "help": - Usage() - return nil case "new": return runNew(ctx, svc, runTUI) - case "version", "--version": - fmt.Println(version.String()) - return nil case "dispatch": return RunDispatch(ctx, svc, args[1:]) case "ls", "list": @@ -165,15 +159,6 @@ func runCommand(ctx context.Context, svc *app.Service, args []string, runTUI fun return runRestart(ctx, svc, args[1:]) case "notify-closed": return runNotifyClosed(svc, args[1:]) - case "kill-all": - return runKillAll(ctx, session.NewClient().KillAll) - case "__host": - // Internal: the detached per-session host process (see - // internal/session). Spawned by CreateSession, never typed by hand. - return session.RunHost(args[1:]) - case "__attach": - // Internal: the interactive attach client run by AttachSpec argv. - return session.RunAttach(args[1:]) case "attach": id, err := requireArg(args[1:], "attach requires ") if err != nil { diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 8f712cf..6bffe19 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -403,16 +403,14 @@ func firstNonEmpty(values ...string) string { return "" } -// The internal __host/__attach subcommands are routed through runCommand; +// The internal __host/__attach subcommands are routed before store access; // invalid input must surface as errors rather than silently doing nothing. -func TestRunCommandInternalSubcommands(t *testing.T) { +func TestRunWithoutStoreInternalSubcommands(t *testing.T) { t.Setenv("UAM_SESSION_DIR", secureSessionDir(t)) - svc, _ := newCLITestService(t) - noTUI := func(context.Context, tea.Model) error { return nil } - if err := runCommand(context.Background(), svc, []string{"__host", "--name", "bad name", "--", "/bin/true"}, noTUI); err == nil { + if handled, err := runWithoutStore(context.Background(), []string{"__host", "--name", "bad name", "--", "/bin/true"}); !handled || err == nil { t.Fatal("__host with an invalid name must fail") } - if err := runCommand(context.Background(), svc, []string{"__attach"}, noTUI); err == nil { + if handled, err := runWithoutStore(context.Background(), []string{"__attach"}); !handled || err == nil { t.Fatal("__attach without a session must fail") } } diff --git a/internal/cli/killall_test.go b/internal/cli/killall_test.go index 0ea7025..d28c512 100644 --- a/internal/cli/killall_test.go +++ b/internal/cli/killall_test.go @@ -4,8 +4,6 @@ import ( "context" "errors" "testing" - - tea "github.com/charmbracelet/bubbletea" ) // F24 — `uam kill-all` must invoke the session teardown exactly once. @@ -32,15 +30,14 @@ func TestRunKillAllPropagatesError(t *testing.T) { } } -// F24 — `kill-all` must be routed by runCommand to the default teardown path. +// F24 — `kill-all` must be routed before store access to the default teardown path. // An empty session runtime dir (via UAM_SESSION_DIR) keeps the test off any // real sessions: KillAll over zero sessions is an idempotent success. -func TestRunCommandKillAllDispatches(t *testing.T) { - svc, _ := newCLITestService(t) +func TestRunWithoutStoreKillAllDispatches(t *testing.T) { t.Setenv("UAM_SESSION_DIR", secureSessionDir(t)) out := captureCLIStdout(t, func() { - if err := runCommand(context.Background(), svc, []string{"kill-all"}, func(context.Context, tea.Model) error { return nil }); err != nil { + if handled, err := runWithoutStore(context.Background(), []string{"kill-all"}); !handled || err != nil { t.Fatalf("kill-all dispatch: %v", err) } }) diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 9af8b5f..f644743 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -158,4 +158,11 @@ func TestCacheDirFallbacks(t *testing.T) { if got := cacheDir(); got != filepath.Join(dir, "uam") { t.Fatalf("%s", got) } + t.Setenv("XDG_CACHE_HOME", "") + t.Setenv("HOME", dir) + if got := cacheDir(); got != filepath.Join(dir, ".cache", "uam") { + t.Fatalf("home cache dir = %s", got) + } + UseStderr(nil) + Debug("stderr fallback debug path") } diff --git a/internal/session/host.go b/internal/session/host.go index 93dd274..971ac78 100644 --- a/internal/session/host.go +++ b/internal/session/host.go @@ -17,6 +17,7 @@ import ( "github.com/creack/pty" + "github.com/RandomCodeSpace/unified-agent-manager/internal/displaytext" "github.com/RandomCodeSpace/unified-agent-manager/internal/log" "github.com/RandomCodeSpace/unified-agent-manager/internal/store" "github.com/RandomCodeSpace/unified-agent-manager/internal/vterm" @@ -369,13 +370,7 @@ func (h *host) setLabel(label string) { // titleSequence sets the terminal title via OSC 0 — the native stand-in for // tmux's set-titles-string showing the user-facing session label. func titleSequence(label string) string { - clean := strings.Map(func(r rune) rune { - if r < 0x20 || r == 0x7f { - return -1 - } - return r - }, label) - return "\x1b]0;" + clean + "\x07" + return "\x1b]0;" + displaytext.Sanitize(label) + "\x07" } func validSize(cols, rows int) bool { diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 5201d90..d16dcbf 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -309,6 +309,14 @@ func TestSetSessionLabelPersistsToState(t *testing.T) { }) } +func TestTitleSequenceSanitizesTerminalControls(t *testing.T) { + got := titleSequence("safe\u009d0;forged\a red\nnow") + want := "\x1b]0;safe red now\x07" + if got != want { + t.Fatalf("titleSequence = %q, want %q", got, want) + } +} + func TestListSweepsStaleStateFiles(t *testing.T) { c := newTestClient(t) if err := EnsureDir(c.Dir); err != nil { diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 026a420..55be9a1 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -104,6 +104,9 @@ func TestTryMarkSessionClosedReportsWhetherRecordMatched(t *testing.T) { if rec.Status != StatusClosedByUser || rec.LastExitCode == nil || *rec.LastExitCode != 7 { t.Fatalf("record not closed: %+v", rec) } + if err := s.MarkSessionClosed("uam-fake-deadbeef", 8); err != nil { + t.Fatalf("idempotent MarkSessionClosed: %v", err) + } } func TestSyncDirAcceptsStoreDirectoryAndRejectsMissingDirectory(t *testing.T) { From 071050e5c439a57abd1cf05dc6ea6f0bf65cbecd Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:23:34 +0000 Subject: [PATCH 18/22] fix(pr): enforce refresh pass deadlines --- internal/app/pr_refresh_test.go | 22 ++++++++++++++++++++++ internal/app/service.go | 24 +++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/internal/app/pr_refresh_test.go b/internal/app/pr_refresh_test.go index 2a48e21..2601f36 100644 --- a/internal/app/pr_refresh_test.go +++ b/internal/app/pr_refresh_test.go @@ -170,6 +170,28 @@ func TestAdapterStatusMapping(t *testing.T) { } } +func TestCheckPRWithContextReturnsWhenCheckerIgnoresCancellation(t *testing.T) { + release := make(chan struct{}) + checkerDone := make(chan struct{}) + checker := func(context.Context, string) (pr.Status, error) { + defer close(checkerDone) + <-release + return pr.Merged, nil + } + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + defer cancel() + started := time.Now() + status, err := checkPRWithContext(ctx, checker, "https://github.com/o/r/pull/5") + if !errors.Is(err, context.DeadlineExceeded) || status != pr.None { + t.Fatalf("checkPRWithContext = (%q, %v), want deadline", status, err) + } + if elapsed := time.Since(started); elapsed > 250*time.Millisecond { + t.Fatalf("cancellation-ignoring checker blocked for %v", elapsed) + } + close(release) + <-checkerDone +} + func BenchmarkRefreshPRStatuses(b *testing.B) { sessions := make([]adapter.Session, 20) for i := range sessions { diff --git a/internal/app/service.go b/internal/app/service.go index 2d130cc..0869ffb 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -327,6 +327,28 @@ type prRefreshResult struct { record store.PRRecord } +type prCheckResult struct { + status pr.Status + err error +} + +// checkPRWithContext enforces the caller's deadline even when a custom checker +// fails to observe context cancellation. The result channel is buffered so a +// late checker can finish without retaining the refresh worker. +func checkPRWithContext(ctx context.Context, checker func(context.Context, string) (pr.Status, error), url string) (pr.Status, error) { + resultCh := make(chan prCheckResult, 1) + go func() { + status, err := checker(ctx, url) + resultCh <- prCheckResult{status: status, err: err} + }() + select { + case result := <-resultCh: + return result.status, result.err + case <-ctx.Done(): + return pr.None, ctx.Err() + } +} + // RefreshPRStatuses enriches stale discovered PRs outside the session-discovery // path. Only PR-owned fields are persisted, and overlapping passes coalesce. func (s *Service) RefreshPRStatuses(ctx context.Context) error { @@ -370,7 +392,7 @@ func (s *Service) RefreshPRStatuses(ctx context.Context) error { for job := range jobCh { status := job.ref.Status checkCtx, checkCancel := context.WithTimeout(ctx, prCheckTimeout) - checked, checkErr := checker(checkCtx, job.ref.URL) + checked, checkErr := checkPRWithContext(checkCtx, checker, job.ref.URL) checkCancel() if checkErr == nil && checked != pr.None { status = adapterStatus(checked) From ae1f883b1533dda08e61aa1cdc0015bd2462afc8 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:32:37 +0000 Subject: [PATCH 19/22] refactor: simplify hardened control paths --- internal/adapter/agent.go | 25 +++++--- internal/app/service.go | 102 +++++++++++++++++++------------ internal/displaytext/sanitize.go | 44 +++++++------ internal/session/client.go | 80 ++++++++++++++---------- internal/session/host.go | 14 +++-- 5 files changed, 160 insertions(+), 105 deletions(-) diff --git a/internal/adapter/agent.go b/internal/adapter/agent.go index 5dc5a74..76036f4 100644 --- a/internal/adapter/agent.go +++ b/internal/adapter/agent.go @@ -219,16 +219,7 @@ func (a *Agent) startSession(ctx context.Context, req ResumeRequest, activity st if err := a.Backend.CreateSession(ctx, sessionName, cwd, env, cmd); err != nil { return Session{}, fmt.Errorf("create session %s: %w", sessionName, err) } - // Surface the user-facing name in attached terminals' titles (the - // canonical uam-- stays the machine-parseable session name). - // Cosmetic and best-effort — a failure never affects the session. - displayName := req.Name - if displayName == "" { - displayName = displayNameFromDir(cwd) - } - if err := a.Backend.SetSessionLabel(ctx, sessionName, displaytext.Sanitize(displayName+" · "+a.Name())); err != nil { - log.Debug("set session label failed", "session", sessionName, "error", err) - } + displayName := a.setSessionDisplayLabel(ctx, sessionName, req.Name, cwd) shouldSendPrompt := strings.TrimSpace(req.Prompt) != "" && (activity != "resumed" || !a.SkipPromptOnResume) if shouldSendPrompt { if err := a.Backend.SendLine(ctx, sessionName, req.Prompt); err != nil { @@ -254,6 +245,20 @@ func (a *Agent) startSession(ctx context.Context, req ResumeRequest, activity st return Session{ID: req.ID, AgentType: a.Name(), CommandAlias: req.CommandAlias, DisplayName: displayName, Prompt: req.Prompt, Cwd: cwd, SessionName: sessionName, ProviderSessionID: providerID, State: Active, ProcAlive: Alive, CreatedAt: created, LastChange: now}, nil } +// setSessionDisplayLabel surfaces the user-facing name in attached terminal +// titles while keeping the canonical session name machine-parseable. It is +// cosmetic and best-effort: failure never changes session startup. +func (a *Agent) setSessionDisplayLabel(ctx context.Context, sessionName, requestedName, cwd string) string { + displayName := requestedName + if displayName == "" { + displayName = displayNameFromDir(cwd) + } + if err := a.Backend.SetSessionLabel(ctx, sessionName, displaytext.Sanitize(displayName+" · "+a.Name())); err != nil { + log.Debug("set session label failed", "session", sessionName, "error", err) + } + return displayName +} + func resolveSessionCwd(cwd string) (string, error) { if cwd == "" { var err error diff --git a/internal/app/service.go b/internal/app/service.go index 0869ffb..3b66326 100644 --- a/internal/app/service.go +++ b/internal/app/service.go @@ -123,28 +123,32 @@ func (s *Service) persistRefresh(updates map[string]refreshPatch) error { } return s.Store.Update(func(cfg *store.Config) error { for key, patch := range updates { - rec, exists := cfg.Sessions[key] - if (!exists || rec.ID == "") && patch.create != nil { - rec = *patch.create - } - if patch.status != nil { - rec.Status = *patch.status - } - if patch.lastSeenAt != nil { - rec.LastSeenAt = *patch.lastSeenAt - } - if patch.clearPR { - rec.PR = nil - } else if patch.pr != nil { - pr := *patch.pr - rec.PR = &pr - } - cfg.Sessions[key] = rec + applyRefreshPatch(cfg, key, patch) } return nil }) } +func applyRefreshPatch(cfg *store.Config, key string, patch refreshPatch) { + rec, exists := cfg.Sessions[key] + if (!exists || rec.ID == "") && patch.create != nil { + rec = *patch.create + } + if patch.status != nil { + rec.Status = *patch.status + } + if patch.lastSeenAt != nil { + rec.LastSeenAt = *patch.lastSeenAt + } + if patch.clearPR { + rec.PR = nil + } else if patch.pr != nil { + pr := *patch.pr + rec.PR = &pr + } + cfg.Sessions[key] = rec +} + func (s *Service) loadConfig() (store.Config, error) { cfg := store.DefaultConfig() if s.Store == nil { @@ -362,25 +366,41 @@ func (s *Service) RefreshPRStatuses(ctx context.Context) error { if err != nil { return err } - now := time.Now() + jobs := stalePRRefreshJobs(sessions, cfg, time.Now()) + if len(jobs) == 0 { + return nil + } + results := runPRRefreshJobs(ctx, jobs, firstPRChecker(s.checkPR)) + if err := s.persistPRRefreshResults(results); err != nil { + return err + } + return ctx.Err() +} + +func stalePRRefreshJobs(sessions []adapter.Session, cfg store.Config, now time.Time) []prRefreshJob { jobs := make([]prRefreshJob, 0, len(sessions)) for _, sess := range sessions { if sess.PR == nil { continue } - rec := cfg.Sessions[store.Key(sess.AgentType, sess.ID)] + key := store.Key(sess.AgentType, sess.ID) + rec := cfg.Sessions[key] if rec.PR != nil && rec.PR.URL == sess.PR.URL && now.Sub(rec.PR.LastChecked) < prRefreshAge { continue } - jobs = append(jobs, prRefreshJob{key: store.Key(sess.AgentType, sess.ID), ref: *sess.PR}) + jobs = append(jobs, prRefreshJob{key: key, ref: *sess.PR}) } - if len(jobs) == 0 { - return nil - } - checker := s.checkPR - if checker == nil { - checker = pr.Check + return jobs +} + +func firstPRChecker(checker func(context.Context, string) (pr.Status, error)) func(context.Context, string) (pr.Status, error) { + if checker != nil { + return checker } + return pr.Check +} + +func runPRRefreshJobs(ctx context.Context, jobs []prRefreshJob, checker func(context.Context, string) (pr.Status, error)) []prRefreshResult { jobCh := make(chan prRefreshJob) resultCh := make(chan prRefreshResult, len(jobs)) workers := min(prRefreshWorkers, len(jobs)) @@ -390,14 +410,7 @@ func (s *Service) RefreshPRStatuses(ctx context.Context) error { go func() { defer wg.Done() for job := range jobCh { - status := job.ref.Status - checkCtx, checkCancel := context.WithTimeout(ctx, prCheckTimeout) - checked, checkErr := checkPRWithContext(checkCtx, checker, job.ref.URL) - checkCancel() - if checkErr == nil && checked != pr.None { - status = adapterStatus(checked) - } - resultCh <- prRefreshResult{key: job.key, record: store.PRRecord{URL: job.ref.URL, Number: job.ref.Number, LastStatus: string(status), LastChecked: time.Now()}} + resultCh <- runPRRefreshJob(ctx, job, checker) } }() } @@ -417,7 +430,22 @@ func (s *Service) RefreshPRStatuses(ctx context.Context) error { for result := range resultCh { results = append(results, result) } - updateErr := s.Store.Update(func(cfg *store.Config) error { + return results +} + +func runPRRefreshJob(ctx context.Context, job prRefreshJob, checker func(context.Context, string) (pr.Status, error)) prRefreshResult { + status := job.ref.Status + checkCtx, checkCancel := context.WithTimeout(ctx, prCheckTimeout) + checked, checkErr := checkPRWithContext(checkCtx, checker, job.ref.URL) + checkCancel() + if checkErr == nil && checked != pr.None { + status = adapterStatus(checked) + } + return prRefreshResult{key: job.key, record: store.PRRecord{URL: job.ref.URL, Number: job.ref.Number, LastStatus: string(status), LastChecked: time.Now()}} +} + +func (s *Service) persistPRRefreshResults(results []prRefreshResult) error { + return s.Store.Update(func(cfg *store.Config) error { for _, result := range results { rec, ok := cfg.Sessions[result.key] if !ok || rec.PR == nil || rec.PR.URL != result.record.URL { @@ -429,10 +457,6 @@ func (s *Service) RefreshPRStatuses(ctx context.Context) error { } return nil }) - if updateErr != nil { - return updateErr - } - return ctx.Err() } func adapterStatus(status pr.Status) adapter.PRStatus { diff --git a/internal/displaytext/sanitize.go b/internal/displaytext/sanitize.go index 78116c8..76a600f 100644 --- a/internal/displaytext/sanitize.go +++ b/internal/displaytext/sanitize.go @@ -45,16 +45,7 @@ func Sanitize(s string) string { func step(state parseState, r rune, b *strings.Builder) parseState { switch state { case escape: - switch r { - case '[': - return csi - case ']', 'P', 'X', '^', '_': - return controlString - case 0x1b: - return escape - default: - return appendGround(r, b) - } + return stepEscape(r, b) case csi: if r == 0x1b { return escape @@ -64,14 +55,7 @@ func step(state parseState, r rune, b *strings.Builder) parseState { } return csi case controlString: - switch r { - case 0x07, 0x9c: - return ground - case 0x1b: - return controlStringEscape - default: - return controlString - } + return stepControlString(r) case controlStringEscape: if r == '\\' || r == 0x9c || r == 0x07 { return ground @@ -85,6 +69,30 @@ func step(state parseState, r rune, b *strings.Builder) parseState { } } +func stepEscape(r rune, b *strings.Builder) parseState { + switch r { + case '[': + return csi + case ']', 'P', 'X', '^', '_': + return controlString + case 0x1b: + return escape + default: + return appendGround(r, b) + } +} + +func stepControlString(r rune) parseState { + switch r { + case 0x07, 0x9c: + return ground + case 0x1b: + return controlStringEscape + default: + return controlString + } +} + func appendGround(r rune, b *strings.Builder) parseState { switch r { case 0x1b: diff --git a/internal/session/client.go b/internal/session/client.go index 8b381cb..dd7e242 100644 --- a/internal/session/client.go +++ b/internal/session/client.go @@ -168,32 +168,36 @@ func (c *Client) List(_ context.Context) ([]Info, error) { } var out []Info for _, e := range entries { - name, isState := strings.CutSuffix(e.Name(), ".json") - if !isState || ValidateName(name) != nil { - continue + if info, keep := c.infoFromStateEntry(e); keep { + out = append(out, info) } - st, err := readState(c.Dir, name) - if err != nil { - log.Warn("skipping unreadable session state", "file", e.Name(), "error", err) - continue - } - if !st.hostAlive() { - if st.childAlive() { - // Host crashed but the agent is still winding down (it gets - // SIGHUP when the PTY master closed). Keep it visible and - // leave the files for the next sweep. - out = append(out, infoFromState(st)) - continue - } - _ = removeSessionFiles(c.Dir, name) - continue - } - out = append(out, infoFromState(st)) } sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) return out, nil } +func (c *Client) infoFromStateEntry(entry os.DirEntry) (Info, bool) { + name, isState := strings.CutSuffix(entry.Name(), ".json") + if !isState || ValidateName(name) != nil { + return Info{}, false + } + st, err := readState(c.Dir, name) + if err != nil { + log.Warn("skipping unreadable session state", "file", entry.Name(), "error", err) + return Info{}, false + } + if st.hostAlive() { + return infoFromState(st), true + } + if st.childAlive() { + // Host crashed but the agent is still winding down. Keep it visible + // and leave the runtime files for the next sweep. + return infoFromState(st), true + } + _ = removeSessionFiles(c.Dir, name) + return Info{}, false +} + func infoFromState(st State) Info { cwd := procCwd(st.ChildPID) if cwd == "" { @@ -257,19 +261,8 @@ func (c *Client) Kill(ctx context.Context, name string) error { // A live PID is not sufficient authority to signal: fallback control paths // require a nonzero matching start identity so a recycled PID can never be // signalled as if it were the session. - if ProcAlive(st.HostPID) { - if !procIdentityMatches(st.HostPID, st.HostStart) { - return fmt.Errorf("kill session %s: cannot verify process identity for host pid %d", name, st.HostPID) - } - _ = syscall.Kill(st.HostPID, syscall.SIGTERM) - } else if ProcAlive(st.ChildPID) { - if !procIdentityMatches(st.ChildPID, st.ChildStart) { - return fmt.Errorf("kill session %s: cannot verify process identity for child pid %d", name, st.ChildPID) - } - // Orphaned agent (host crashed): signal its process group directly. - if err := syscall.Kill(-st.ChildPID, syscall.SIGTERM); err != nil { - _ = syscall.Kill(st.ChildPID, syscall.SIGTERM) - } + if err := signalVerifiedFallback(name, st); err != nil { + return err } deadline := time.Now().Add(callTimeout) for time.Now().Before(deadline) { @@ -286,6 +279,27 @@ func (c *Client) Kill(ctx context.Context, name string) error { return fmt.Errorf("kill session %s: still running", name) } +func signalVerifiedFallback(name string, st State) error { + if ProcAlive(st.HostPID) { + if !procIdentityMatches(st.HostPID, st.HostStart) { + return fmt.Errorf("kill session %s: cannot verify process identity for host pid %d", name, st.HostPID) + } + _ = syscall.Kill(st.HostPID, syscall.SIGTERM) + return nil + } + if !ProcAlive(st.ChildPID) { + return nil + } + if !procIdentityMatches(st.ChildPID, st.ChildStart) { + return fmt.Errorf("kill session %s: cannot verify process identity for child pid %d", name, st.ChildPID) + } + // Orphaned agent (host crashed): signal its process group directly. + if err := syscall.Kill(-st.ChildPID, syscall.SIGTERM); err != nil { + _ = syscall.Kill(st.ChildPID, syscall.SIGTERM) + } + return nil +} + // KillAll terminates every managed session. It replaces `tmux kill-server` // and is idempotent: an empty (or missing) runtime directory is success. func (c *Client) KillAll(ctx context.Context) error { diff --git a/internal/session/host.go b/internal/session/host.go index 971ac78..52ca71c 100644 --- a/internal/session/host.go +++ b/internal/session/host.go @@ -550,14 +550,18 @@ func (h *host) markClosed(exitCode int) { lastErr = err remaining := time.Until(deadline) if remaining <= 0 { - if lastErr != nil { - log.Warn("mark session closed failed after retry", "session", h.name, "error", lastErr) - } else { - log.Warn("mark session closed record not found before retry deadline", "session", h.name) - } + h.logMarkClosedFailure(lastErr) return } time.Sleep(min(delay, remaining)) delay = min(delay*2, markClosedRetryMax) } } + +func (h *host) logMarkClosedFailure(err error) { + if err != nil { + log.Warn("mark session closed failed after retry", "session", h.name, "error", err) + return + } + log.Warn("mark session closed record not found before retry deadline", "session", h.name) +} From 5b94e78a1f416054066e08a3de86a3b6160e210d Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 09:35:03 +0000 Subject: [PATCH 20/22] chore(security): document validated filesystem boundaries --- internal/session/session.go | 4 +++- internal/store/store.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/session/session.go b/internal/session/session.go index 7cd53b8..543d4c4 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -186,7 +186,9 @@ func readState(dir, name string) (State, error) { if !info.Mode().IsRegular() { return State{}, fmt.Errorf("session state %s is not a regular file", name) } - data, err := os.ReadFile(path) + // name has passed the strict canonical allow-list and dir has passed the + // owner-only identity check above, so path cannot escape the runtime dir. + data, err := os.ReadFile(path) // #nosec G304 -- validated name within a verified owner-only directory. if err != nil { return State{}, err } diff --git a/internal/store/store.go b/internal/store/store.go index 732d825..0d39ba0 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -583,7 +583,9 @@ func (s *Store) saveNoLock(cfg Config) error { } func syncDir(dir string) error { - f, err := os.Open(dir) + // dir is the containing directory of Store.path; opening that configured + // path is the intended durability operation, not user-controlled inclusion. + f, err := os.Open(dir) // #nosec G304 -- Store intentionally fsyncs its configured parent directory. if err != nil { return fmt.Errorf("open store directory for sync: %w", err) } From 5810dea66cc3abbbf5a08d67d9cf8c8d20e386d5 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 10:06:08 +0000 Subject: [PATCH 21/22] docs: define Windows support via WSL2 --- README.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/README.md b/README.md index 02750eb..384e0cb 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,19 @@ there is nothing else to install. Providers are capability-probed at runtime. If a CLI is missing, `uam` hides it from the dispatch UI instead of failing the whole app. +## Supported platforms + +- Linux, including Ubuntu, on amd64 and arm64 +- macOS on Intel and Apple silicon +- Windows 10/11 through WSL2 with an Ubuntu distribution + +On Windows, install and run `uam` and the provider CLIs inside the same WSL2 +distribution. Sessions and paths then live inside that Linux environment. +Native Windows processes are not supported yet: the session host depends on a +Unix PTY, Unix process groups, and owner-only Unix runtime directories. A native +port requires a ConPTY and Job Object backend and will not be advertised until +its full create/list/attach/stop/restart lifecycle passes on Windows. + ## Install Install the `uam` binary directly: From 35af55fabf77402ebe0fa7e0609223d9bba2d596 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sun, 12 Jul 2026 11:35:01 +0000 Subject: [PATCH 22/22] chore: ignore graphify output artifacts --- .gitignore | 2 ++ README.md | 13 +++---------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/.gitignore b/.gitignore index 8d40a06..6b039f7 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,5 @@ coverage.* /.claude/worktrees/ /CLAUDE.md /PLAN.md +graphify_out/ +graphify-out/ diff --git a/README.md b/README.md index 384e0cb..aa06c84 100644 --- a/README.md +++ b/README.md @@ -48,16 +48,9 @@ from the dispatch UI instead of failing the whole app. ## Supported platforms -- Linux, including Ubuntu, on amd64 and arm64 -- macOS on Intel and Apple silicon -- Windows 10/11 through WSL2 with an Ubuntu distribution - -On Windows, install and run `uam` and the provider CLIs inside the same WSL2 -distribution. Sessions and paths then live inside that Linux environment. -Native Windows processes are not supported yet: the session host depends on a -Unix PTY, Unix process groups, and owner-only Unix runtime directories. A native -port requires a ConPTY and Job Object backend and will not be advertised until -its full create/list/attach/stop/restart lifecycle passes on Windows. +- Linux (Ubuntu), on amd64 and arm64 +- macOS, on Intel and Apple silicon +- Native Windows is not supported. ## Install