diff --git a/cmd/agentop/apiclient/client.go b/cmd/agentop/apiclient/client.go index 06fff63fa..5b8a54e2b 100644 --- a/cmd/agentop/apiclient/client.go +++ b/cmd/agentop/apiclient/client.go @@ -249,14 +249,15 @@ type PipelineView struct { // PipelinePlugin describes one plugin's position, direction, and // capabilities. Mirrors the server's pipelinePluginView exactly. type PipelinePlugin struct { - Name string `json:"name"` - Direction string `json:"direction"` - Position int `json:"position"` - ReadsBody bool `json:"readsBody"` - Requires []string `json:"requires,omitempty"` - RequiresAny []string `json:"requiresAny,omitempty"` - Description string `json:"description,omitempty"` - Config json.RawMessage `json:"config,omitempty"` + Name string `json:"name"` + Direction string `json:"direction"` + Position int `json:"position"` + ReadsBody bool `json:"readsBody"` + Requires []string `json:"requires,omitempty"` + RequiresAny []string `json:"requiresAny,omitempty"` + RequiresLater []string `json:"requiresLater,omitempty"` + Description string `json:"description,omitempty"` + Config json.RawMessage `json:"config,omitempty"` // OnError is the plugin's on_error policy, "" for the default (enforce): the proxy sends // it only when it is not. A plugin under observe runs, but its rejections, body writes and // redirects are dropped — header writes still apply — so in those respects its Config says @@ -294,13 +295,14 @@ type PluginCatalog struct { // Describes a registered plugin's static type-level metadata; the // catalog includes plugins not currently in the active pipeline. type PluginCatalogEntry struct { - Name string `json:"name"` - Direction string `json:"direction,omitempty"` - ReadsBody bool `json:"readsBody,omitempty"` - Requires []string `json:"requires,omitempty"` - RequiresAny []string `json:"requiresAny,omitempty"` - Description string `json:"description,omitempty"` - Fields []PluginFieldEntry `json:"fields,omitempty"` + Name string `json:"name"` + Direction string `json:"direction,omitempty"` + ReadsBody bool `json:"readsBody,omitempty"` + Requires []string `json:"requires,omitempty"` + RequiresAny []string `json:"requiresAny,omitempty"` + RequiresLater []string `json:"requiresLater,omitempty"` + Description string `json:"description,omitempty"` + Fields []PluginFieldEntry `json:"fields,omitempty"` } // PluginFieldEntry mirrors sessionapi.FieldSchemaEntry — per-field diff --git a/cmd/agentop/apiclient/sse.go b/cmd/agentop/apiclient/sse.go index 2a11c64f1..97203ecb7 100644 --- a/cmd/agentop/apiclient/sse.go +++ b/cmd/agentop/apiclient/sse.go @@ -72,13 +72,21 @@ func (c *Client) Stream(ctx context.Context, sessionFilter string) <-chan Stream func (c *Client) streamLoop(ctx context.Context, filter string, out chan<- StreamEvent) { defer close(out) + // attempt is consecutive FAILED connects, not a lifetime counter: it + // resets once a connection reaches "open", so five disconnects spread + // over a day (laptop sleep, proxy restart) each wait 1s, not the capped + // 30s — and the banner's attempt number describes the current outage + // rather than the process's age (#1568). attempt := 0 for { - err := c.streamOnce(ctx, filter, out) + opened, err := c.streamOnce(ctx, filter, out) if ctx.Err() != nil { // Caller cancelled — terminal state is the channel close on defer. return } + if opened { + attempt = 0 + } attempt++ wait := backoffSchedule(attempt) sendStatus(out, StreamStatus{Phase: "reconnecting", Attempt: attempt, Wait: wait, Err: err}) @@ -92,15 +100,17 @@ func (c *Client) streamLoop(ctx context.Context, filter string, out chan<- Strea // streamOnce opens a single SSE connection and pumps events to out until // either the server closes, ctx is cancelled, or a transport error occurs. -// Returns nil only if the server closed cleanly. -func (c *Client) streamOnce(ctx context.Context, filter string, out chan<- StreamEvent) error { +// opened reports whether the connection got as far as the "open" status — +// the caller resets its backoff on it. Returns a nil error only on a clean +// server close. +func (c *Client) streamOnce(ctx context.Context, filter string, out chan<- StreamEvent) (opened bool, err error) { u := c.endpoint + "/v1/events" if filter != "" { u += "?session=" + filter } req, err := http.NewRequestWithContext(ctx, "GET", u, nil) if err != nil { - return err + return false, err } req.Header.Set("Accept", "text/event-stream") @@ -108,31 +118,47 @@ func (c *Client) streamOnce(ctx context.Context, filter string, out chan<- Strea // pool survives across reconnects. resp, err := c.httpStream.Do(req) if err != nil { - return err + return false, err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return fmt.Errorf("stream: unexpected status %d", resp.StatusCode) + return false, fmt.Errorf("stream: unexpected status %d", resp.StatusCode) } sendStatus(out, StreamStatus{Phase: "open"}) - return parseSSE(ctx, resp.Body, out) + return true, parseSSE(ctx, resp.Body, out) } +// maxSSELine bounds how much of one SSE line parseSSE keeps. NOT a wire +// limit: the server has no cap on event size (full inference bodies, +// against a proxy that accepts 32 MiB request bodies), so no fixed bound +// covers every legitimate event. A line longer than this marks its frame +// as oversized, and the frame is skipped — the connection survives. +const maxSSELine = 32 * 1024 * 1024 + // parseSSE reads SSE frames line-by-line. A frame is a run of lines ending // in a blank line. Lines starting with ':' are comments (our heartbeats). // We only care about frames with a `data:` line containing a JSON-encoded // SessionEvent; other line types are tolerated but ignored. func parseSSE(ctx context.Context, r io.Reader, out chan<- StreamEvent) error { - scanner := bufio.NewScanner(r) - // Some events (e.g. full LLM completion bodies) can be many KB. Raise the - // line buffer well above the default 64 KB to match the server's cap. - scanner.Buffer(make([]byte, 0, 16*1024), 4*1024*1024) + lines := newLineScan(r, maxSSELine) var dataBuf strings.Builder + // oversized marks the frame in progress as unparseable — one of its + // lines was truncated at the cap. The frame is dropped at its blank + // line and the stream keeps reading; before this, a bufio.Scanner + // ErrTooLong killed the connection, and every event published during + // the reconnect gap was lost (#1568). + oversized := false flush := func() { defer dataBuf.Reset() + bad := oversized + oversized = false + if bad { + slog.Debug("apiclient: skipping oversized SSE frame", "bytes", dataBuf.Len()) + return + } if dataBuf.Len() == 0 { return } @@ -156,14 +182,26 @@ func parseSSE(ctx context.Context, r io.Reader, out chan<- StreamEvent) error { } } - for scanner.Scan() { - line := scanner.Text() + for { + line, truncated, err := lines.next() + if err != nil { + // Drain trailing frame if the stream ended mid-record (rare; + // usual case is a clean "\n\n" before EOF). + flush() + if errors.Is(err, io.EOF) { + return io.EOF + } + return err + } switch { case line == "": flush() case strings.HasPrefix(line, ":"): // Comment / heartbeat — ignore. case strings.HasPrefix(line, "data:"): + if truncated { + oversized = true + } val := strings.TrimPrefix(line, "data:") val = strings.TrimPrefix(val, " ") if dataBuf.Len() > 0 { @@ -177,13 +215,62 @@ func parseSSE(ctx context.Context, r io.Reader, out chan<- StreamEvent) error { return ctx.Err() } } - // Drain trailing frame if the stream ended mid-record (rare; usual case - // is a clean "\n\n" before EOF). - flush() - if err := scanner.Err(); err != nil && !errors.Is(err, io.EOF) { - return err +} + +// lineScan is bufio.Scanner's line loop with one difference that matters +// here: a line longer than max does not fail the reader. The line comes +// back truncated (at most max bytes, truncated=true) and the reader is +// positioned at the next line, so one huge frame costs one frame instead +// of the connection. +type lineScan struct { + br *bufio.Reader + max int +} + +func newLineScan(r io.Reader, max int) *lineScan { + return &lineScan{br: bufio.NewReaderSize(r, 16*1024), max: max} +} + +// next returns the next line without its terminator. truncated reports +// that the line exceeded the cap and was cut. A terminal error is returned +// with whatever remained buffered; io.EOF means clean end of stream. +func (s *lineScan) next() (line string, truncated bool, err error) { + var buf []byte + for { + frag, rerr := s.br.ReadSlice('\n') + switch rerr { + case nil: + frag = frag[:len(frag)-1] // drop the '\n' + case bufio.ErrBufferFull: + // Line longer than the read buffer: keep accumulating if within + // the cap, otherwise discard the overflow and remember it. + if len(buf)+len(frag) > s.max { + if len(buf) < s.max { + buf = append(buf, frag[:s.max-len(buf)]...) + } + truncated = true + } else { + buf = append(buf, frag...) + } + continue + case io.EOF: + if len(frag) == 0 && len(buf) == 0 { + return "", false, io.EOF + } + // Final line without a trailing newline. + default: + return string(buf), truncated, rerr + } + if len(buf)+len(frag) > s.max { + if len(buf) < s.max { + buf = append(buf, frag[:s.max-len(buf)]...) + } + truncated = true + } else { + buf = append(buf, frag...) + } + return string(buf), truncated, nil } - return io.EOF } func sendStatus(out chan<- StreamEvent, s StreamStatus) { diff --git a/cmd/agentop/apiclient/sse_test.go b/cmd/agentop/apiclient/sse_test.go index 4d00a15db..c5385b26b 100644 --- a/cmd/agentop/apiclient/sse_test.go +++ b/cmd/agentop/apiclient/sse_test.go @@ -3,8 +3,10 @@ package apiclient import ( "context" "fmt" + "io" "net/http" "net/http/httptest" + "strings" "sync" "sync/atomic" "testing" @@ -220,3 +222,116 @@ func TestBackoffSchedule_CapsAt30s(t *testing.T) { } } } + +// Backoff counts consecutive FAILED connects, not a lifetime of disconnects +// (#1568): once a connection reaches "open", the counter resets, so five +// disconnects spread over a day each wait 1s instead of the capped 30s. +func TestStream_BackoffResetsAfterOpen(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(200) + w.(http.Flusher).Flush() + // Every connection opens, then closes — each is a successful open + // followed by a disconnect. + <-time.After(50 * time.Millisecond) + })) + defer ts.Close() + + c := New(ts.URL) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + ch := c.Stream(ctx, "") + + reconnects := 0 + deadline := time.After(5 * time.Second) + for reconnects < 3 { + select { + case msg, ok := <-ch: + if !ok { + t.Fatalf("channel closed after %d reconnects", reconnects) + } + if msg.Status.Phase == "reconnecting" { + reconnects++ + if msg.Status.Attempt != 1 { + t.Fatalf("reconnect #%d carried attempt %d, want 1 — backoff did not reset after a successful open", + reconnects, msg.Status.Attempt) + } + } + case <-deadline: + t.Fatalf("timeout after %d reconnects", reconnects) + } + } +} + +// A frame larger than the reader's cap must cost one frame, not the +// connection (#1568): the old bufio.Scanner died with ErrTooLong past 4 MiB +// — a size real events reach, since the server marshals full inference +// bodies — and everything published during the reconnect gap was lost. +// Here the oversized frame is 5 MiB (over the old limit, under the new cap) +// and the events on either side of it must both arrive. +func TestStream_SkipsOversizedFrame(t *testing.T) { + valid := "event: session-event\ndata: {\"sessionId\":\"s1\",\"at\":\"2026-01-01T00:00:00Z\",\"direction\":\"inbound\",\"phase\":\"request\"}\n\n" + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + flusher := w.(http.Flusher) + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(200) + fmt.Fprint(w, valid) + flusher.Flush() + fmt.Fprintf(w, "data: %s\n\n", strings.Repeat("x", 5*1024*1024)) + flusher.Flush() + fmt.Fprint(w, valid) + flusher.Flush() + <-r.Context().Done() + })) + defer ts.Close() + + c := New(ts.URL) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + ch := c.Stream(ctx, "") + + var gotEvents int + deadline := time.After(3 * time.Second) + for gotEvents < 2 { + select { + case msg, ok := <-ch: + if !ok { + t.Fatalf("channel closed after %d events — the oversized frame killed the connection", gotEvents) + } + if msg.Event != nil { + gotEvents++ + } + case <-deadline: + t.Fatalf("timeout after %d events; the oversized frame killed the connection", gotEvents) + } + } +} + +// lineScan's contract, at a cap small enough to exercise directly: a line +// within the cap passes through untouched, a longer one comes back +// truncated with the reader positioned at the NEXT line. +func TestLineScan_TruncatesInsteadOfFailing(t *testing.T) { + ls := newLineScan(strings.NewReader("short\n"+strings.Repeat("y", 64)+"\nalso short\n"), 16) + + line, trunc, err := ls.next() + if err != nil || trunc || line != "short" { + t.Fatalf("first line = %q trunc=%v err=%v, want \"short\" untouched", line, trunc, err) + } + line, trunc, err = ls.next() + if err != nil { + t.Fatalf("oversized line errored: %v — the reader must survive it", err) + } + if !trunc { + t.Fatal("64-byte line at a 16-byte cap must report truncated") + } + if len(line) != 16 { + t.Fatalf("truncated line kept %d bytes, want the 16-byte cap", len(line)) + } + line, trunc, err = ls.next() + if err != nil || trunc || line != "also short" { + t.Fatalf("line after the oversized one = %q trunc=%v err=%v, want \"also short\" — reader position was lost", line, trunc, err) + } + if _, _, err = ls.next(); err != io.EOF { + t.Fatalf("exhausted reader returned %v, want io.EOF", err) + } +} diff --git a/cmd/agentop/edit/validate.go b/cmd/agentop/edit/validate.go index 7c6105f88..72a380460 100644 --- a/cmd/agentop/edit/validate.go +++ b/cmd/agentop/edit/validate.go @@ -72,7 +72,64 @@ func ValidatePipeline(subtree []byte, catalog []apiclient.PluginCatalogEntry) [] return errs } -// validateChain runs the Requires/RequiresAny/unknown-name checks for one direction. +// CheckOrder reports every dependency-ordering violation for the plugin +// named name sitting at 1-based position pos, against positions (plugin +// name → first 1-based position in the same chain). It is agentop's one +// copy of the framework's validateRelationships ordering rules — the +// pre-apply validator below and the TUI's DEPS indicator both call it, so +// the two can no longer drift apart the way they did over RequiresLater +// (#1568). Each returned string is operator-facing. +// +// Rules, mirroring core/plugins/registry.go: +// - Requires: every named plugin at a strictly-lower position. +// - RequiresLater: every named plugin at a strictly-higher position +// (the response pass walks the chain in reverse). +// - RequiresAny: at least one named plugin at a lower position, and +// every named plugin that IS present at a lower position. +func CheckOrder(name string, pos int, positions map[string]int, requires, requiresAny, requiresLater []string) []string { + var problems []string + for _, req := range requires { + rp, present := positions[req] + switch { + case !present: + problems = append(problems, fmt.Sprintf("Requires %q, but it is not in this chain", req)) + case rp >= pos: + problems = append(problems, fmt.Sprintf("Requires %q upstream, but it is at position %d (must be < %d)", + req, rp, pos)) + } + } + for _, req := range requiresLater { + rp, present := positions[req] + switch { + case !present: + problems = append(problems, fmt.Sprintf("RequiresLater %q, but it is not in this chain", req)) + case rp <= pos: + problems = append(problems, fmt.Sprintf("RequiresLater %q downstream (it reads what %q publishes while folding response frames, and the response pass runs in reverse), but it is at position %d (must be > %d)", + req, req, rp, pos)) + } + } + if len(requiresAny) > 0 { + anyUpstream := false + for _, opt := range requiresAny { + rp, present := positions[opt] + if !present { + continue + } + if rp < pos { + anyUpstream = true + } else { + problems = append(problems, fmt.Sprintf("RequiresAny lists %q which is at position %d (must be < %d)", + opt, rp, pos)) + } + } + if !anyUpstream { + problems = append(problems, fmt.Sprintf("RequiresAny %v: none present upstream in this chain", requiresAny)) + } + } + return problems +} + +// validateChain runs the Requires/RequiresAny/RequiresLater/unknown-name checks for one direction. func validateChain(direction string, chain pipelineChain, byName map[string]apiclient.PluginCatalogEntry) []ValidationError { var errs []ValidationError // Track positions of each name for ordering checks. Using lowest @@ -102,61 +159,16 @@ func validateChain(direction string, chain pipelineChain, byName map[string]apic continue } - // Requires: every name MUST appear at strictly-lower position. - for _, req := range entry.Requires { - rp, present := positions[req] - if !present { - errs = append(errs, ValidationError{ - Direction: direction, - PluginName: p.Name, - Position: pos, - Message: fmt.Sprintf("Requires %q, but it is not in the %s chain", - req, direction), - }) - continue - } - if rp >= pos { - errs = append(errs, ValidationError{ - Direction: direction, - PluginName: p.Name, - Position: pos, - Message: fmt.Sprintf("Requires %q upstream, but it is at position %d (must be < %d)", - req, rp, pos), - }) - } - } - - // RequiresAny: at least one of the listed names must appear at - // lower position. Each named one that IS present must be earlier. - if len(entry.RequiresAny) > 0 { - anyOK := false - for _, opt := range entry.RequiresAny { - rp, present := positions[opt] - if !present { - continue - } - if rp < pos { - anyOK = true - } - if rp >= pos { - errs = append(errs, ValidationError{ - Direction: direction, - PluginName: p.Name, - Position: pos, - Message: fmt.Sprintf("RequiresAny lists %q which is at position %d (must be < %d)", - opt, rp, pos), - }) - } - } - if !anyOK { - errs = append(errs, ValidationError{ - Direction: direction, - PluginName: p.Name, - Position: pos, - Message: fmt.Sprintf("RequiresAny %v: none present upstream in %s chain", - entry.RequiresAny, direction), - }) - } + // Requires / RequiresAny / RequiresLater ordering — CheckOrder, the + // one copy of the framework's rules shared with the TUI's DEPS + // indicator. Messages name the chain direction here only. + for _, problem := range CheckOrder(p.Name, pos, positions, entry.Requires, entry.RequiresAny, entry.RequiresLater) { + errs = append(errs, ValidationError{ + Direction: direction, + PluginName: p.Name, + Position: pos, + Message: fmt.Sprintf("%s (%s chain)", problem, direction), + }) } } diff --git a/cmd/agentop/edit/validate_test.go b/cmd/agentop/edit/validate_test.go index f120408c0..152306c77 100644 --- a/cmd/agentop/edit/validate_test.go +++ b/cmd/agentop/edit/validate_test.go @@ -106,3 +106,62 @@ func TestValidatePipeline_NilCatalogSkips(t *testing.T) { t.Fatalf("nil catalog should disable validation, got %+v", errs) } } + +// The #1568 case: litellm-budget-track declares RequiresLater inference-parser, +// and neither this validator nor the TUI's DEPS indicator knew the rule — an +// edit the proxy would refuse passed agentop's pre-apply check as clean. +func validateRequiresLaterCatalog() []apiclient.PluginCatalogEntry { + return []apiclient.PluginCatalogEntry{ + {Name: "inference-parser", Description: "Parser"}, + {Name: "litellm-budget-track", Description: "Budget", + RequiresLater: []string{"inference-parser"}}, + } +} + +func TestValidatePipeline_RequiresLaterSatisfied(t *testing.T) { + yaml := `pipeline: + outbound: + plugins: + - name: litellm-budget-track + - name: inference-parser +` + errs := ValidatePipeline([]byte(yaml), validateRequiresLaterCatalog()) + if len(errs) != 0 { + t.Fatalf("satisfied RequiresLater produced errors: %+v", errs) + } +} + +func TestValidatePipeline_RequiresLaterMissing(t *testing.T) { + yaml := `pipeline: + outbound: + plugins: + - name: litellm-budget-track +` + errs := ValidatePipeline([]byte(yaml), validateRequiresLaterCatalog()) + if len(errs) != 1 { + t.Fatalf("expected exactly the RequiresLater error, got %+v", errs) + } + if !strings.Contains(errs[0].Message, "RequiresLater \"inference-parser\"") { + t.Fatalf("error should name the missing dependency; got %+v", errs) + } +} + +func TestValidatePipeline_RequiresLaterMisordered(t *testing.T) { + // The exact operator mistake the issue describes: inference-parser + // moved ABOVE budget-track, so the parser it reads while folding + // response frames now runs before it. The framework refuses this + // reload; agentop must refuse it at edit time too. + yaml := `pipeline: + outbound: + plugins: + - name: inference-parser + - name: litellm-budget-track +` + errs := ValidatePipeline([]byte(yaml), validateRequiresLaterCatalog()) + if len(errs) != 1 { + t.Fatalf("expected exactly the RequiresLater misorder error, got %+v", errs) + } + if !strings.Contains(errs[0].Message, "must be > 2") { + t.Fatalf("error should say the dependency must sit later; got %+v", errs) + } +} diff --git a/cmd/agentop/tui/app.go b/cmd/agentop/tui/app.go index 70b742c35..96f49af31 100644 --- a/cmd/agentop/tui/app.go +++ b/cmd/agentop/tui/app.go @@ -1795,7 +1795,26 @@ func (m *model) update(msg tea.Msg) (tea.Model, tea.Cmd) { // Learned from the echo, not assumed: see model.serverProjects. m.serverProjects = msg.projected // Only update if we're still focused on this session. - m.events[msg.id] = msg.events + // + // MERGE, not wholesale replace: events streamed while the snapshot + // request was in flight have a higher seq than the snapshot's tail + // (the server answers everything it held at request time), and a + // plain assignment dropped them until the next manual refresh (#1568). + // The comparison runs in the session's OWN numbering only — + // mergeLinked interleaves linked-bucket events, numbered in their own + // bucket's space, and a linked call can postdate the own tail, so the + // last element by time is not the highest own seq. A snapshot whose + // own events carry no seq at all (a proxy that predates paging) lands + // whole. + events := msg.events + if last := lastOwnSeq(events, msg.id); last > 0 { + for _, e := range m.events[msg.id] { + if eventSession(msg.id, &e) == msg.id && e.Seq > last { + events = append(events, e) + } + } + } + m.events[msg.id] = events // A wholesale replacement the length check cannot see, so the gauge is re-folded // over what just landed. NOT dropped: msg.events is projected (view=summary // strips the manifest and the message count), so dropping the figure here blanked @@ -2385,6 +2404,16 @@ func (m *model) watchingSession(id string) bool { return m.selectedSess == id || m.timeline.session == id } +// watchingSessionID is the id form of watchingSession: the session whose +// events are being held open, "" when none is. The opened session wins over +// the timeline's, matching which one a snapshot refresh is for. +func (m *model) watchingSessionID() string { + if m.selectedSess != "" { + return m.selectedSess + } + return m.timeline.session +} + // flushSessionsTable re-sorts the session list by recency and rebuilds its table when a streamed // event has changed it since the last rebuild. Called from the 1s tick; see sessionsStale. func (m *model) flushSessionsTable() { @@ -2645,10 +2674,10 @@ func (m *model) paneView() string { // so it cannot perturb the pane it sits above. rows := []string{header} // EXACTLY WHAT layout() RESERVED, blank where there is nothing to say. Both reservations are - // height-gated and must stay that way — layout() runs only from the WindowSizeMsg handler, so - // a content-gated reservation would go stale the moment a poll landed, and stale in the - // direction that overflows. So the render fills them rather than the reservation tracking the - // render. + // height-gated and must stay that way — layout() runs on resizes, drawer toggles and pane + // transitions, none of them on a poll's schedule, so a content-gated reservation would go + // stale the moment a poll landed, and stale in the direction that overflows. So the render + // fills them rather than the reservation tracking the render. // // The case that made this necessary: the renderer can return FEWER lines than the reservation, // which left the strip's row and the drawer's five unfilled and the footer six rows above the diff --git a/cmd/agentop/tui/deps.go b/cmd/agentop/tui/deps.go index 5083078c2..2e8b2a904 100644 --- a/cmd/agentop/tui/deps.go +++ b/cmd/agentop/tui/deps.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/rossoctl/cortex/cmd/agentop/apiclient" + "github.com/rossoctl/cortex/cmd/agentop/edit" ) // depCheck describes whether one declared dependency is satisfied by @@ -33,88 +34,78 @@ func sameDirectionChain(p *apiclient.PipelinePlugin, view *apiclient.PipelineVie return nil } -// requiresStatus returns one depCheck per entry in p.Requires. -// Satisfied iff the named plugin is present at a strictly-lower -// position. (RequiresAny semantics differ; see requiresAnyStatus.) -func requiresStatus(p *apiclient.PipelinePlugin, chain []apiclient.PipelinePlugin) []depCheck { - out := make([]depCheck, 0, len(p.Requires)) - for _, name := range p.Requires { - c := depCheck{Name: name} - for _, q := range chain { - if q.Name == name && q.Position < p.Position { - c.Satisfied = true - c.UpstreamPosition = q.Position - break - } +// depPositions maps each plugin name to its first 1-based position in +// chain — the same earliest-occurrence-wins rule the framework's +// validateRelationships uses. +func depPositions(chain []apiclient.PipelinePlugin) map[string]int { + positions := make(map[string]int, len(chain)) + for _, q := range chain { + if _, seen := positions[q.Name]; !seen { + positions[q.Name] = q.Position } - out = append(out, c) } - return out + return positions } -// requiresAnyOK matches the framework's validateRelationships rule -// exactly: at least one named plugin must be present upstream, AND -// every name that IS present must be earlier (a downstream presence -// is a misorder). Earlier this returned true on at-least-one-upstream -// without checking the misorder condition, which let the Pipeline -// pane's DEPS column show ✓ for chains the framework would reject. -func requiresAnyOK(p *apiclient.PipelinePlugin, chain []apiclient.PipelinePlugin) bool { - if len(p.RequiresAny) == 0 { - return true - } - anyUpstream := false - for _, name := range p.RequiresAny { +// positionStatus returns one depCheck per entry in names. Satisfied iff +// the named plugin is present strictly before (or, for RequiresLater, +// strictly after) p. One helper for all three declarations — Requires, +// RequiresAny and RequiresLater render per-row through it; the verdict +// itself is edit.CheckOrder's, not a reimplementation. +func positionStatus(p *apiclient.PipelinePlugin, chain []apiclient.PipelinePlugin, names []string, higher bool) []depCheck { + out := make([]depCheck, 0, len(names)) + for _, name := range names { + c := depCheck{Name: name} for _, q := range chain { if q.Name != name { continue } - if q.Position < p.Position { - anyUpstream = true - } else if q.Position > p.Position { - // Present-but-downstream violates the ordering rule. - return false + if (!higher && q.Position < p.Position) || (higher && q.Position > p.Position) { + c.Satisfied = true + c.UpstreamPosition = q.Position } break } + out = append(out, c) } - return anyUpstream + return out +} + +// requiresStatus returns one depCheck per entry in p.Requires. +func requiresStatus(p *apiclient.PipelinePlugin, chain []apiclient.PipelinePlugin) []depCheck { + return positionStatus(p, chain, p.Requires, false) } // requiresAnyStatus returns one depCheck per entry in p.RequiresAny. // Satisfied means "present at lower position." Used by the detail // pane to show which alternatives currently satisfy the OR-group. func requiresAnyStatus(p *apiclient.PipelinePlugin, chain []apiclient.PipelinePlugin) []depCheck { - out := make([]depCheck, 0, len(p.RequiresAny)) - for _, name := range p.RequiresAny { - c := depCheck{Name: name} - for _, q := range chain { - if q.Name == name && q.Position < p.Position { - c.Satisfied = true - c.UpstreamPosition = q.Position - break - } - } - out = append(out, c) - } - return out + return positionStatus(p, chain, p.RequiresAny, false) } -// pluginDepsAllSatisfied returns true iff Requires and RequiresAny are -// all OK for p in chain. Drives the Pipeline pane's per-row ✓/✗ indicator. +// requiresLaterStatus returns one depCheck per entry in p.RequiresLater. +// Satisfied means "present at higher position" — the response pass walks +// the chain in reverse, so a RequiresLater dependency sits downstream. +func requiresLaterStatus(p *apiclient.PipelinePlugin, chain []apiclient.PipelinePlugin) []depCheck { + return positionStatus(p, chain, p.RequiresLater, true) +} + +// pluginDepsAllSatisfied returns true iff Requires, RequiresAny and +// RequiresLater are all OK for p in chain. Drives the Pipeline pane's +// per-row ✓/✗ indicator. The verdict is edit.CheckOrder's — the same +// copy of the framework's validateRelationships rules the pre-apply +// validator runs, so the indicator cannot show ✓ for a chain the proxy +// would refuse to reload (#1568: it did, for RequiresLater). func pluginDepsAllSatisfied(p *apiclient.PipelinePlugin, chain []apiclient.PipelinePlugin) bool { - for _, c := range requiresStatus(p, chain) { - if !c.Satisfied { - return false - } - } - return requiresAnyOK(p, chain) + return len(edit.CheckOrder(p.Name, p.Position, depPositions(chain), + p.Requires, p.RequiresAny, p.RequiresLater)) == 0 } // pluginHasAnyDeps reports whether p declares any dependency that the // indicator can render. Plugins without any declarations get a blank // indicator (no false-positive ✓). func pluginHasAnyDeps(p *apiclient.PipelinePlugin) bool { - return len(p.Requires) > 0 || len(p.RequiresAny) > 0 + return len(p.Requires) > 0 || len(p.RequiresAny) > 0 || len(p.RequiresLater) > 0 } // formatDepCheck returns a one-line description of a dependency check. @@ -133,6 +124,9 @@ func formatDepCheck(c depCheck, withStatus bool) string { return styleOK.Render("✓ ") + c.Name + styleHint.Render(" — absent (soft)") } + // Not "NOT in this chain": a ✗ also covers the misorder case, where the + // name IS present but on the wrong side of p (Requires wants it earlier, + // RequiresLater later). return styleError.Render("✗ ") + c.Name + - styleHint.Render(" — NOT in this chain") + styleHint.Render(" — not satisfied in this chain") } diff --git a/cmd/agentop/tui/deps_test.go b/cmd/agentop/tui/deps_test.go index af66d48fa..267bad191 100644 --- a/cmd/agentop/tui/deps_test.go +++ b/cmd/agentop/tui/deps_test.go @@ -113,3 +113,44 @@ func TestPluginDepsAllSatisfied_RequiresAnyMisorderOnAlternate(t *testing.T) { t.Fatal("RequiresAny should NOT be satisfied when an alternative is downstream") } } + +// The #1568 case in the DEPS indicator: a RequiresLater misorder used to +// render ✓ (and still pass the pre-apply check), while the proxy refused +// the reload. The verdict now comes from edit.CheckOrder, the same copy +// of the framework's rules the pre-apply validator runs. +func TestPluginDepsAllSatisfied_RequiresLaterMet(t *testing.T) { + chain := []apiclient.PipelinePlugin{ + {Name: "litellm-budget-track", Direction: "outbound", Position: 1, + RequiresLater: []string{"inference-parser"}}, + {Name: "inference-parser", Direction: "outbound", Position: 2}, + } + if !pluginDepsAllSatisfied(&chain[0], chain) { + t.Fatal("RequiresLater should be satisfied when the target is downstream") + } + if !pluginHasAnyDeps(&chain[0]) { + t.Fatal("RequiresLater alone should mark the plugin as having deps") + } +} + +func TestPluginDepsAllSatisfied_RequiresLaterMisordered(t *testing.T) { + // The #1568 case: the parser moved above the plugin that reads its + // response-phase output. + chain := []apiclient.PipelinePlugin{ + {Name: "inference-parser", Direction: "outbound", Position: 1}, + {Name: "litellm-budget-track", Direction: "outbound", Position: 2, + RequiresLater: []string{"inference-parser"}}, + } + if pluginDepsAllSatisfied(&chain[1], chain) { + t.Fatal("RequiresLater should NOT be satisfied when the target is upstream") + } +} + +func TestPluginDepsAllSatisfied_RequiresLaterMissing(t *testing.T) { + chain := []apiclient.PipelinePlugin{ + {Name: "litellm-budget-track", Direction: "outbound", Position: 1, + RequiresLater: []string{"inference-parser"}}, + } + if pluginDepsAllSatisfied(&chain[0], chain) { + t.Fatal("RequiresLater should NOT be satisfied when the target is absent") + } +} diff --git a/cmd/agentop/tui/keys.go b/cmd/agentop/tui/keys.go index b812fc3f6..3897ef2a9 100644 --- a/cmd/agentop/tui/keys.go +++ b/cmd/agentop/tui/keys.go @@ -472,6 +472,19 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { case "p": m.paused = !m.paused + if m.paused { + return nil + } + // Resuming: everything streamed while paused was dropped, so refetch + // the watched session's tail rather than let the timeline quietly + // stop at the pause point (#1568). snapshotLoadedMsg merges by seq, + // so anything the refetch still misses can arrive off the stream + // without a duplicate. NOT while paged back: a snapshot ends the + // paging window, so this refetch would jump an operator reading an + // old page to the tail — [t] does that on purpose, [p] must not. + if id := m.watchingSessionID(); id != "" && !m.pagedBack(id) { + return m.snapshotCmd(id) + } return nil case "s": @@ -653,10 +666,13 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { // // The honest consequence: cached-only sessions are never released // while agentop runs, so the cache grows by one entry per restart the - // user visited a session across. Measured, that is ~165 bytes per - // event and 1000 events per session, so ~161 KB per session and a - // few MB for a long debugging afternoon — worth it, given the - // alternative is deleting the only copy of what someone is reading. + // user visited a session across. There is no per-session event cap + // anymore, so the cost of one entry is whatever its interned + // snapshot cost — a 23.5MB document decoded to 2.31MB retained on + // the benchmark in apiclient/snapshot.go. A few visited sessions is + // megabytes; a long debugging afternoon across many restarts can be + // more — worth it, given the alternative is deleting the only copy + // of what someone is reading. if id != m.selectedSess { for cached := range m.events { if cached != id && live[cached] { @@ -1385,14 +1401,15 @@ func (m *model) layout() { bodyH -= dividerLines // And one for the spend strip, which View() draws directly under the title. // - // BY HEIGHT ALONE, deliberately blind to the pane: layout() is called from exactly one - // place, the WindowSizeMsg handler, and no pane transition re-runs it. A reservation - // that read m.pane would go stale the moment the user moved between a picker and a data - // pane, leaving bodyHeight wrong until the next resize — which on a pane the strip DOES - // draw means a body one row too tall and a footer pushed off the bottom. The cost is - // that the two picker panes render one row shorter than they need, which is invisible - // next to that. See spendStripReservesRow and spendStripVisible for the same asymmetry - // stated from the other side. + // BY HEIGHT ALONE, deliberately blind to the pane: there is no setPane choke point to + // hook (the pane is assigned in twenty-odd places, and layout() runs on resizes, drawer + // toggles and pane transitions alike), so a reservation that read m.pane would go stale + // the moment the user moved between a picker and a data pane, leaving bodyHeight wrong + // until the next layout() — which on a pane the strip DOES draw means a body one row too + // tall and a footer pushed off the bottom. The cost is that the two picker panes render + // one row shorter than they need, which is invisible next to that. See + // spendStripReservesRow and spendStripVisible for the same asymmetry stated from the + // other side. if m.spendStripReservesRow() { // spendBandLines, not one row: the band stacks labels over values. paneView fills // exactly this many, and the two must move together — see paneView. diff --git a/cmd/agentop/tui/linked.go b/cmd/agentop/tui/linked.go index 61febcedf..2edcffb0b 100644 --- a/cmd/agentop/tui/linked.go +++ b/cmd/agentop/tui/linked.go @@ -147,6 +147,21 @@ func eventSession(sessionID string, e *pipeline.SessionEvent) string { return sessionID } +// lastOwnSeq is the highest seq among events that sessionID recorded itself, 0 +// when none carries one. The snapshot merge compares the session's own numbering +// only: mergeLinked interleaves linked-bucket events, numbered in their own +// bucket's space, so the last event by time is not necessarily the highest own +// seq — a linked call can postdate the session's own tail. +func lastOwnSeq(events []pipeline.SessionEvent, sessionID string) uint64 { + var last uint64 + for i := range events { + if e := &events[i]; eventSession(sessionID, e) == sessionID && e.Seq > last { + last = e.Seq + } + } + return last +} + // ownOldest is the oldest of held that sessionID recorded itself, nil when none is: paging // counts from it, because a linked bucket's events are numbered in their own bucket. func ownOldest(held []pipeline.SessionEvent, sessionID string) *pipeline.SessionEvent { diff --git a/cmd/agentop/tui/plugin_detail_pane.go b/cmd/agentop/tui/plugin_detail_pane.go index 20f000069..2e9bde48a 100644 --- a/cmd/agentop/tui/plugin_detail_pane.go +++ b/cmd/agentop/tui/plugin_detail_pane.go @@ -72,6 +72,16 @@ func (m *model) showPluginDetail(p *apiclient.PipelinePlugin, resetScroll bool) b.WriteString("\n") } } + if len(p.RequiresLater) > 0 { + fmt.Fprintln(&b) + b.WriteString(styleMuted.Render("Requires later:")) + b.WriteString("\n") + for _, c := range requiresLaterStatus(p, chain) { + b.WriteString(" ") + b.WriteString(formatDepCheck(c, chain != nil)) + b.WriteString("\n") + } + } // Metrics section, for plugins implementing pipeline.MetricsProvider. // Same always-newline treatment as Config below: the header is drawn // whether or not there are rows, so navigating between a plugin that diff --git a/cmd/agentop/tui/plugin_detail_pane_test.go b/cmd/agentop/tui/plugin_detail_pane_test.go index f3b3e441c..40638a8cb 100644 --- a/cmd/agentop/tui/plugin_detail_pane_test.go +++ b/cmd/agentop/tui/plugin_detail_pane_test.go @@ -128,7 +128,7 @@ func TestShowPluginDetailRendersUnmetRequires(t *testing.T) { if !strings.Contains(view, "missing-dep") { t.Fatalf("Requires should name missing-dep:\n%s", view) } - if !strings.Contains(view, "NOT in this chain") { + if !strings.Contains(view, "not satisfied in this chain") { t.Fatalf("Requires should call out the missing dep:\n%s", view) } } diff --git a/cmd/agentop/tui/snapshot_merge_test.go b/cmd/agentop/tui/snapshot_merge_test.go new file mode 100644 index 000000000..8dcc7b20b --- /dev/null +++ b/cmd/agentop/tui/snapshot_merge_test.go @@ -0,0 +1,184 @@ +package tui + +import ( + "testing" + "time" + + "github.com/charmbracelet/bubbletea" + + "github.com/rossoctl/cortex/core/pipeline" +) + +// #1568: a snapshot that lands while events are streaming used to REPLACE +// the held list wholesale, silently dropping everything that arrived after +// the request was issued. The merge keeps held events with a higher seq. +func TestSnapshot_MergesStreamedEventsAfterItsTail(t *testing.T) { + const id = "merge" + m := newRetentionModel(t, id, 0) + + at := time.Now() + streamed := make([]pipeline.SessionEvent, 0, 10) + for i := 1; i <= 10; i++ { + streamed = append(streamed, pipeline.SessionEvent{ + At: at.Add(time.Duration(i) * time.Millisecond), SessionID: id, Seq: uint64(i), + Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, Host: "h", + }) + } + m.events[id] = streamed + + // The snapshot answers everything the server held at request time: the + // first eight. Events 9 and 10 streamed in while it was in flight. + snapshot := make([]pipeline.SessionEvent, 0, 8) + for i := 1; i <= 8; i++ { + snapshot = append(snapshot, pipeline.SessionEvent{ + At: at.Add(time.Duration(i) * time.Millisecond), SessionID: id, Seq: uint64(i), + Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, Host: "h", + }) + } + m.Update(snapshotLoadedMsg{id: id, events: snapshot, serverOldest: 1}) + + got := m.events[id] + if len(got) != 10 { + t.Fatalf("snapshot of 8 merged with 10 held events kept %d, want 10 — events streamed during the fetch were dropped", len(got)) + } + if got[len(got)-1].Seq != 10 { + t.Fatalf("last event seq = %d, want 10", got[len(got)-1].Seq) + } +} + +// A snapshot from a proxy that predates paging carries no seqs; unordered +// events cannot merge, so the snapshot lands whole, as before. +func TestSnapshot_ReplacesWhenSeqsAbsent(t *testing.T) { + const id = "noseq" + m := newRetentionModel(t, id, 0) + + at := time.Now() + held := make([]pipeline.SessionEvent, 3) + snap := make([]pipeline.SessionEvent, 2) + for i := range held { + held[i] = pipeline.SessionEvent{At: at, SessionID: id, Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, Host: "h"} + } + for i := range snap { + snap[i] = pipeline.SessionEvent{At: at, SessionID: id, Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, Host: "h"} + } + m.events[id] = held + m.Update(snapshotLoadedMsg{id: id, events: snap}) + + if got := len(m.events[id]); got != 2 { + t.Fatalf("seq-less snapshot stored %d events, want the snapshot's 2", got) + } +} + +// #1568: pausing dropped every streamed event, and resuming did not +// refetch — the timeline just stopped. Resuming now issues a snapshot for +// the watched session (which merges by seq, so nothing doubles). +func TestPause_ResumeRefetchesWatchedSession(t *testing.T) { + const id = "resume" + m := newRetentionModel(t, id, 0) + + if cmd := m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("p")}); cmd != nil { + t.Fatal("pausing must not fetch anything") + } + if !m.paused { + t.Fatal("p did not pause") + } + cmd := m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("p")}) + if m.paused { + t.Fatal("second p did not resume") + } + if cmd == nil { + t.Fatal("resuming must refetch the watched session's tail — events streamed while paused were dropped") + } +} + +// With nothing watched, resume has nothing to refetch and returns no cmd. +func TestPause_ResumeWithoutWatchedSession(t *testing.T) { + m := newRetentionModel(t, "unused", 0) + m.selectedSess = "" + m.timeline.session = "" + + m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("p")}) + if cmd := m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("p")}); cmd != nil { + t.Fatal("resume with no watched session must not fetch") + } +} + +func ownEv(id string, seq int, at time.Time) pipeline.SessionEvent { + return pipeline.SessionEvent{ + At: at, SessionID: id, Seq: uint64(seq), + Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, Host: "h", + } +} + +// A snapshot's list is TIME-sorted, and mergeLinked interleaves linked-bucket +// events into it — events numbered in their OWN bucket's seq space. When a +// linked call is newer by time than the session's own tail, the last element +// is a bucket event, and keying the merge on its seq compares the session's +// own numbering against a bucket's: held events the snapshot already holds +// get appended a second time. +func TestSnapshot_MergeComparesOnlyOwnSessionSeqs(t *testing.T) { + const id = "own" + const bucket = "pending:claude@123.4" + m := newRetentionModel(t, id, 0) + + at := time.Now() + // The snapshot: the session's own first eight events, then a linked call + // from the bucket that postdates the session's own tail — the shape + // mergeLinked produces for it. + snapshot := make([]pipeline.SessionEvent, 0, 9) + for i := 1; i <= 8; i++ { + snapshot = append(snapshot, ownEv(id, i, at.Add(time.Duration(i)*time.Millisecond))) + } + snapshot = append(snapshot, pipeline.SessionEvent{ + At: at.Add(9 * time.Millisecond), SessionID: bucket, Seq: 2, + Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, Host: "h", + }) + // Held: the session's own events 6..10, streamed while the snapshot was + // in flight — 6..8 are in the snapshot, 9 and 10 are not. + held := make([]pipeline.SessionEvent, 0, 5) + for i := 6; i <= 10; i++ { + held = append(held, ownEv(id, i, at.Add(time.Duration(i)*time.Millisecond))) + } + m.events[id] = held + + m.Update(snapshotLoadedMsg{id: id, events: snapshot, serverOldest: 1}) + + got := m.events[id] + seen := map[uint64]bool{} + for _, e := range got { + if e.SessionID != id { + continue + } + if seen[e.Seq] { + t.Fatalf("own event seq %d appears twice after the merge — own seqs were compared against a linked bucket's", e.Seq) + } + seen[e.Seq] = true + } + if len(got) != 11 { + t.Fatalf("merged list holds %d events, want 11 (8 own + 1 linked + streamed 9 and 10)", len(got)) + } + if !seen[10] { + t.Fatal("streamed event 10 was dropped by the merge") + } +} + +// Resuming the stream must not refetch while paged back: a snapshot IS the +// tail, so it ends the paging window and jumps the timeline there — [t] does +// that on purpose, an un-pause must not destroy the page the operator paused +// to read. +func TestPause_ResumeWhilePagedBackKeepsTheWindow(t *testing.T) { + const id = "pagedback" + m := newRetentionModel(t, id, 0) + m.paging = map[string]*pagingState{id: {pageSizes: []int{10}, fetched: 1}} + + m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("p")}) + if !m.paused { + t.Fatal("p did not pause") + } + if cmd := m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("p")}); cmd != nil { + t.Fatal("resume while paged back must not fetch — the snapshot would end the paging window and jump to the tail") + } + if m.paging[id] == nil { + t.Fatal("the paging window did not survive the resume") + } +} diff --git a/cmd/agentop/tui/spend_band.go b/cmd/agentop/tui/spend_band.go index acbefac16..bd7c079f2 100644 --- a/cmd/agentop/tui/spend_band.go +++ b/cmd/agentop/tui/spend_band.go @@ -211,8 +211,9 @@ func spendBandCells(s spendSummary, width int) []bandCell { // renderSpendBand is the band as PLAIN TEXT: one line, no escape sequences. // -// This is what the width arithmetic and every assertion run over, which is why it is the form the -// renderer publishes. renderSpendBandStyled is the same line dressed for the screen. +// This is what the width arithmetic and every assertion run over — a TEST-facing form, +// not a production one: paneView calls only renderSpendBandStyled, which is this same +// line dressed for the screen. // // Returns a slice rather than a string so it keeps paneView's shape — spendBandLines rows in, the // same number out — and so growing the band back to two rows would not change the signature again. diff --git a/cmd/agentop/tui/spend_drawer.go b/cmd/agentop/tui/spend_drawer.go index f67c5c54f..de4d0a86a 100644 --- a/cmd/agentop/tui/spend_drawer.go +++ b/cmd/agentop/tui/spend_drawer.go @@ -101,7 +101,7 @@ func spendDrawerLinesFor(width int) int { return left + 2 // the header and the hint line } -// spendDrawerAxes are the breakdown axes `g` cycles through. +// spendDrawerAxes are the breakdown axes `a` cycles through. // // NO GroupNone in the cycle, unlike the Usage pane's grouping. This drawer's only content // IS the breakdown, so "no breakdown" is an empty drawer — a state the user can already @@ -310,11 +310,11 @@ func spendDrawerHostPane(pane paneID) (bool, string) { // back. // // THE FLAG AND THE HEIGHT, deliberately blind to the pane — the same asymmetry -// spendStripReservesRow has, for the same reason: layout() is called from the WindowSizeMsg -// handler, and no pane transition re-runs it. There is no setPane choke point to hook (the pane -// is assigned in twenty-odd places), so a pane-aware reservation would go stale on the next -// transition — and stale in the dangerous direction, because a drawer that DRAWS against an -// unreserved body overflows the terminal. +// spendStripReservesRow has, for the same reason: layout() runs on resizes, drawer +// toggles and pane transitions alike, and there is no setPane choke point to hook (the +// pane is assigned in twenty-odd places), so a pane-aware reservation would go stale on +// the next transition — and stale in the dangerous direction, because a drawer that DRAWS +// against an unreserved body overflows the terminal. // // The cost is larger than the strip's: while the drawer is open, a pane that cannot host it // renders its reservation shorter than it could. That is a visible loss where the strip's was diff --git a/cmd/agentop/tui/spend_strip.go b/cmd/agentop/tui/spend_strip.go index 53b314380..338d164b2 100644 --- a/cmd/agentop/tui/spend_strip.go +++ b/cmd/agentop/tui/spend_strip.go @@ -560,11 +560,13 @@ func (m *model) spendStripVisible() bool { // spendStripReservesRow reports whether layout() must hold a row back for the // strip. Height ONLY — deliberately blind to the pane. // -// layout() is called from exactly one place, the WindowSizeMsg handler; no pane -// transition re-runs it. So a reservation that read m.pane would go stale the -// moment the user moved between a picker and a data pane, and bodyHeight would -// stay wrong until the next terminal resize — which for a pane the strip DOES -// draw on means a body one row too tall and a footer pushed off the bottom. +// There is no setPane choke point to hook: the pane is assigned in twenty-odd +// places, and layout() runs on resizes, drawer toggles and pane transitions +// alike — none of them a guaranteed stop on the way between two panes. So a +// reservation that read m.pane would go stale the moment the user moved +// between a picker and a data pane, and bodyHeight would stay wrong until the +// next layout() — which for a pane the strip DOES draw on means a body one row +// too tall and a footer pushed off the bottom. // // Being blind to the pane costs the two picker panes one row they could have // used. That is the accepted trade: a picker one row short is invisible, an diff --git a/core/sessionapi/catalog_adapter.go b/core/sessionapi/catalog_adapter.go index ef03ceb5d..6bbebb51b 100644 --- a/core/sessionapi/catalog_adapter.go +++ b/core/sessionapi/catalog_adapter.go @@ -25,12 +25,13 @@ func PluginsCatalog() []CatalogEntry { for i, e := range src { n := e.Capabilities.Normalize() out[i] = CatalogEntry{ - Name: e.Name, - ReadsBody: n.ReadsResponseBody, - Requires: n.Requires, - RequiresAny: n.RequiresAny, - Description: n.Description, - Fields: convertFieldSchemas(e.Fields), + Name: e.Name, + ReadsBody: n.ReadsResponseBody, + Requires: n.Requires, + RequiresAny: n.RequiresAny, + RequiresLater: n.RequiresLater, + Description: n.Description, + Fields: convertFieldSchemas(e.Fields), } } return out diff --git a/core/sessionapi/server.go b/core/sessionapi/server.go index 88a9341b2..604aa998a 100644 --- a/core/sessionapi/server.go +++ b/core/sessionapi/server.go @@ -88,13 +88,18 @@ type Server struct { // that documents bodyAccess as deprecated, so there's no compat cost to // emit the right name from day one. type CatalogEntry struct { - Name string `json:"name"` - Direction string `json:"direction,omitempty"` - ReadsBody bool `json:"readsBody,omitempty"` - Requires []string `json:"requires,omitempty"` - RequiresAny []string `json:"requiresAny,omitempty"` - Description string `json:"description,omitempty"` - Fields []FieldSchemaEntry `json:"fields,omitempty"` + Name string `json:"name"` + Direction string `json:"direction,omitempty"` + ReadsBody bool `json:"readsBody,omitempty"` + Requires []string `json:"requires,omitempty"` + RequiresAny []string `json:"requiresAny,omitempty"` + // RequiresLater is the mirror of Requires for the response pass, which + // walks the chain in reverse: the named plugin must sit at a HIGHER + // position. Carried on the wire so agentop's pre-apply check and DEPS + // indicator can enforce the same rule the framework does. + RequiresLater []string `json:"requiresLater,omitempty"` + Description string `json:"description,omitempty"` + Fields []FieldSchemaEntry `json:"fields,omitempty"` } // FieldSchemaEntry is the wire shape for one config field's schema @@ -287,14 +292,15 @@ func (s *Server) handleHealthz(w http.ResponseWriter, _ *http.Request) { // compute the "deps satisfied" indicator on the Pipeline pane without // needing a separate /v1/plugins call. type pipelinePluginView struct { - Name string `json:"name"` - Direction string `json:"direction"` - Position int `json:"position"` // 1-based order within its direction - ReadsBody bool `json:"readsBody"` - Requires []string `json:"requires,omitempty"` - RequiresAny []string `json:"requiresAny,omitempty"` - Description string `json:"description,omitempty"` - Config json.RawMessage `json:"config,omitempty"` + Name string `json:"name"` + Direction string `json:"direction"` + Position int `json:"position"` // 1-based order within its direction + ReadsBody bool `json:"readsBody"` + Requires []string `json:"requires,omitempty"` + RequiresAny []string `json:"requiresAny,omitempty"` + RequiresLater []string `json:"requiresLater,omitempty"` + Description string `json:"description,omitempty"` + Config json.RawMessage `json:"config,omitempty"` // OnError is the plugin's on_error policy, present only when it is not the default // (enforce), so in practice only "observe": an off plugin is not built into the // pipeline, and is not listed here at all. A reader needs it because a plugin under @@ -363,13 +369,14 @@ func describePipeline(h *pipeline.Holder, direction string) []pipelinePluginView for i, pl := range plugins { caps := pl.Capabilities().Normalize() view := pipelinePluginView{ - Name: pl.Name(), - Direction: direction, - Position: i + 1, - ReadsBody: caps.ReadsResponseBody, - Requires: caps.Requires, - RequiresAny: caps.RequiresAny, - Description: caps.Description, + Name: pl.Name(), + Direction: direction, + Position: i + 1, + ReadsBody: caps.ReadsResponseBody, + Requires: caps.Requires, + RequiresAny: caps.RequiresAny, + RequiresLater: caps.RequiresLater, + Description: caps.Description, } if policy := p.PolicyAt(i); policy != pipeline.ErrorPolicyEnforce { view.OnError = policy