From 877eeaf4c36a508c3c5109b0ec15dd0ae9526e0b Mon Sep 17 00:00:00 2001 From: NGHINAI <97311544+NGHINAI@users.noreply.github.com> Date: Wed, 10 Jun 2026 00:01:58 -0300 Subject: [PATCH] Surface truncation instead of silently compacting a partial log window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All input paths cap collection at 20k lines and kill the child when the cap is hit, keeping the oldest lines and dropping the newest — but nothing in the output said so. An agent (or user) debugging an incident saw a confident compaction summary computed over a slice that often excludes the incident itself, since recent lines are the ones that matter and file reads put them at the end. Closes the visibility half of #7: - exec.Collect now reports whether the line cap cut the stream short (Streamed.HitLimit), and readLines/readNonEmptyLines detect a dropped non-empty line past their caps. - CLI wrap prints a stderr warning (even with --quiet — coverage is correctness, not chrome) for both the line cap and the 90s collection timeout, before the compaction starts. - MCP tools append a "[codag] note: ..." to the returned text for both cases, telling the agent to narrow the window (--since/--tail) and rerun. The keep-newest ring-buffer alternative from #7 is left as a follow-up since it changes the latency profile; this makes the current behavior honest first. Co-Authored-By: Claude Fable 5 --- cmd/io.go | 12 ++++--- cmd/io_test.go | 60 +++++++++++++++++++++++++++++++++++ cmd/wrap.go | 33 +++++++++++++++---- internal/exec/collect_test.go | 38 ++++++++++++++++++++++ internal/exec/stream.go | 26 +++++++++------ internal/mcp/dispatch.go | 34 +++++++++++++++++--- internal/mcp/lines.go | 12 ++++--- internal/mcp/tools_test.go | 58 ++++++++++++++++++++++++++++++++- 8 files changed, 241 insertions(+), 32 deletions(-) create mode 100644 cmd/io_test.go create mode 100644 internal/exec/collect_test.go diff --git a/cmd/io.go b/cmd/io.go index 6eb48d1..070e627 100644 --- a/cmd/io.go +++ b/cmd/io.go @@ -25,21 +25,23 @@ func resolveAPIClient(cmd *cobra.Command) *api.Client { const maxLineLen = 256 * 1024 const maxInputLines = 20_000 -func readLines(r io.Reader) ([]string, error) { +// readLines reads up to maxInputLines non-empty lines. truncated is true +// when at least one more non-empty line existed past the cap, so the +// caller can warn that the compact view is missing the newest data. +func readLines(r io.Reader) (out []string, truncated bool, err error) { scanner := bufio.NewScanner(r) scanner.Buffer(make([]byte, maxLineLen), maxLineLen) - var out []string for scanner.Scan() { line := strings.TrimRight(scanner.Text(), "\r\n") if line == "" { continue } - out = append(out, line) if len(out) >= maxInputLines { - break + return out, true, scanner.Err() } + out = append(out, line) } - return out, scanner.Err() + return out, false, scanner.Err() } // linesToRecords turns plain text lines into LineRecord with sequential ids. diff --git a/cmd/io_test.go b/cmd/io_test.go new file mode 100644 index 0000000..d9a5c14 --- /dev/null +++ b/cmd/io_test.go @@ -0,0 +1,60 @@ +package cmd + +import ( + "bytes" + "strings" + "testing" +) + +func TestReadLinesReportsTruncation(t *testing.T) { + var b strings.Builder + for i := 0; i < maxInputLines+1; i++ { + b.WriteString("x\n") + } + lines, truncated, err := readLines(strings.NewReader(b.String())) + if err != nil { + t.Fatalf("readLines: %v", err) + } + if len(lines) != maxInputLines { + t.Fatalf("got %d lines, want cap of %d", len(lines), maxInputLines) + } + if !truncated { + t.Fatal("truncated = false, want true when input exceeds the cap") + } +} + +func TestReadLinesNoTruncationAtExactCap(t *testing.T) { + var b strings.Builder + for i := 0; i < maxInputLines; i++ { + b.WriteString("x\n") + } + // Trailing blank lines after the cap are not data — not a truncation. + b.WriteString("\n\n") + lines, truncated, err := readLines(strings.NewReader(b.String())) + if err != nil { + t.Fatalf("readLines: %v", err) + } + if len(lines) != maxInputLines { + t.Fatalf("got %d lines, want %d", len(lines), maxInputLines) + } + if truncated { + t.Fatal("truncated = true, want false when input fits exactly at the cap") + } +} + +func TestWarnIncompleteCoverage(t *testing.T) { + var buf bytes.Buffer + warnIncomplete(&buf, true, true) + out := buf.String() + for _, want := range []string{"20000", "oldest", "1m30s"} { + if !strings.Contains(out, want) { + t.Fatalf("warning %q missing %q", out, want) + } + } + + buf.Reset() + warnIncomplete(&buf, false, false) + if buf.Len() != 0 { + t.Fatalf("expected no output for complete input, got %q", buf.String()) + } +} diff --git a/cmd/wrap.go b/cmd/wrap.go index 7f21c8b..cdd496a 100644 --- a/cmd/wrap.go +++ b/cmd/wrap.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" osexec "os/exec" "strings" @@ -93,10 +94,11 @@ func runWrap(cmd *cobra.Command, args []string) error { } var lines []string + var truncated, timedOut bool var err error // child-command exit error; nil for the stdin path if len(args) > 0 { var fatalErr error - lines, err, fatalErr = readChildOutput(args) + lines, truncated, timedOut, err, fatalErr = readChildOutput(args) if fatalErr != nil { return fatalErr // child produced no output AND failed to start } @@ -110,7 +112,7 @@ func runWrap(cmd *cobra.Command, args []string) error { " e.g. `codag wrap -- vercel logs --since=1h`\n" + " `cat /var/log/app.log | codag wrap`") } - lines, err = readLines(os.Stdin) + lines, truncated, err = readLines(os.Stdin) if err != nil { return fmt.Errorf("read stdin: %w", err) } @@ -119,6 +121,10 @@ func runWrap(cmd *cobra.Command, args []string) error { } err = nil } + // Warn before the (possibly slow) compaction, and on stderr even with + // --quiet: a partial compact presented as full coverage is a + // correctness problem, not chrome. + warnIncomplete(os.Stderr, truncated, timedOut) level, _ := cmd.Flags().GetString("level") service, _ := cmd.Flags().GetString("service") @@ -233,18 +239,31 @@ func runWrap(cmd *cobra.Command, args []string) error { return nil } -func readChildOutput(args []string) (lines []string, runErr error, fatalErr error) { - lines, runErr = childexec.Collect(context.Background(), args[0], args[1:], wrapMaxLines, wrapTimeout) +func readChildOutput(args []string) (lines []string, truncated, timedOut bool, runErr error, fatalErr error) { + lines, truncated, runErr = childexec.Collect(context.Background(), args[0], args[1:], wrapMaxLines, wrapTimeout) if len(lines) == 0 { if runErr != nil { - return nil, nil, fmt.Errorf("child command failed and produced no output: %w", runErr) + return nil, false, false, nil, fmt.Errorf("child command failed and produced no output: %w", runErr) } - return nil, nil, fmt.Errorf("child command produced no log lines") + return nil, false, false, nil, fmt.Errorf("child command produced no log lines") } if errors.Is(runErr, context.DeadlineExceeded) { runErr = nil + timedOut = true + } + return lines, truncated, timedOut, runErr, nil +} + +// warnIncomplete tells the user when the input was cut short — by the +// line cap (oldest lines kept, newest dropped) or the collection timeout. +// Stderr only, so stdout stays clean for pipes and agents. +func warnIncomplete(w io.Writer, truncated, timedOut bool) { + if truncated { + fmt.Fprintf(w, "[codag] warning: input truncated at %d lines — the oldest %d were kept and the newest dropped. Narrow the log window (e.g. --since/--tail) for full coverage.\n", wrapMaxLines, wrapMaxLines) + } + if timedOut { + fmt.Fprintf(w, "[codag] warning: log command stopped at the %s collection limit; output may be incomplete.\n", wrapTimeout) } - return lines, runErr, nil } func jsonOut(v any) error { diff --git a/internal/exec/collect_test.go b/internal/exec/collect_test.go new file mode 100644 index 0000000..ee4c6de --- /dev/null +++ b/internal/exec/collect_test.go @@ -0,0 +1,38 @@ +package exec_test + +import ( + "context" + "testing" + + "github.com/codag-megalith/codag-cli/internal/exec" +) + +// TestCollect_ReportsTruncation verifies Collect tells the caller when the +// line cap cut the stream short, so callers can surface a truncation +// notice instead of presenting a partial compact as full coverage. +func TestCollect_ReportsTruncation(t *testing.T) { + lines, truncated, err := exec.Collect(context.Background(), "yes", nil, 50, 0) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if !truncated { + t.Fatal("truncated = false, want true when the cap kills the child") + } + if len(lines) < 50 { + t.Fatalf("got %d lines, want at least the 50-line cap", len(lines)) + } +} + +func TestCollect_NoTruncationUnderCap(t *testing.T) { + lines, truncated, err := exec.Collect(context.Background(), "printf", + []string{`a\nb\n`}, 50, 0) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if truncated { + t.Fatal("truncated = true, want false when output fits under the cap") + } + if len(lines) != 2 { + t.Fatalf("got %d lines, want 2", len(lines)) + } +} diff --git a/internal/exec/stream.go b/internal/exec/stream.go index 6ea78c9..254d089 100644 --- a/internal/exec/stream.go +++ b/internal/exec/stream.go @@ -29,15 +29,23 @@ const maxLineLen = 256 * 1024 // On context cancel or maxLines reached, the child is killed via the // context. Always drain the channel — leaking a reader leaks a goroutine. type Streamed struct { - Lines <-chan string - wait func() error + Lines <-chan string + wait func() error + hitLimit *atomic.Bool } func (s *Streamed) Wait() error { return s.wait() } +// HitLimit reports whether the stream stopped because maxLines was +// reached — the child was killed mid-output and the newest lines were +// dropped. Callers should surface this so a partial collection is not +// presented as full coverage. +func (s *Streamed) HitLimit() bool { return s.hitLimit.Load() } + // Collect streams a child command into memory with practical incident-sized // bounds. maxLines = 0 means unlimited; timeout = 0 means no extra timeout. -func Collect(ctx context.Context, cmd string, args []string, maxLines int, timeout time.Duration) ([]string, error) { +// truncated is true when the maxLines cap cut the stream short. +func Collect(ctx context.Context, cmd string, args []string, maxLines int, timeout time.Duration) (lines []string, truncated bool, err error) { if ctx == nil { ctx = context.Background() } @@ -48,13 +56,12 @@ func Collect(ctx context.Context, cmd string, args []string, maxLines int, timeo } s, err := Stream(ctx, cmd, args, maxLines) if err != nil { - return nil, err + return nil, false, err } - var lines []string for line := range s.Lines { lines = append(lines, line) } - return lines, s.Wait() + return lines, s.HitLimit(), s.Wait() } // Stream starts the child and begins emitting lines on the returned @@ -95,7 +102,7 @@ func stream(ctx context.Context, cmd string, args []string, maxLines int, emitSt var wg sync.WaitGroup var emitted int var emittedMu sync.Mutex - var hitLimit atomic.Bool + hitLimit := &atomic.Bool{} scanErrs := make(chan error, 2) var stderrMu sync.Mutex var stderrTail []string @@ -182,7 +189,8 @@ func stream(ctx context.Context, cmd string, args []string, maxLines int, emitSt }() return &Streamed{ - Lines: out, - wait: func() error { return <-waitErr }, + Lines: out, + wait: func() error { return <-waitErr }, + hitLimit: hitLimit, }, nil } diff --git a/internal/mcp/dispatch.go b/internal/mcp/dispatch.go index 0e3824f..8ed6b00 100644 --- a/internal/mcp/dispatch.go +++ b/internal/mcp/dispatch.go @@ -89,7 +89,7 @@ func runChildAndCompact(ctx context.Context, cmd string, args []string, level, s ctx = context.Background() } - lines, runErr := childexec.Collect(ctx, cmd, args, mcpMaxLines, mcpTimeout) + lines, truncated, runErr := childexec.Collect(ctx, cmd, args, mcpMaxLines, mcpTimeout) if len(lines) == 0 { if runErr != nil { return "", fmt.Errorf("child failed and produced no output: %w", runErr) @@ -99,7 +99,8 @@ func runChildAndCompact(ctx context.Context, cmd string, args []string, level, s if errors.Is(runErr, context.Canceled) { return "", fmt.Errorf("cancelled: %w", runErr) } - if errors.Is(runErr, context.DeadlineExceeded) { + timedOut := errors.Is(runErr, context.DeadlineExceeded) + if timedOut { runErr = nil } @@ -118,14 +119,33 @@ func runChildAndCompact(ctx context.Context, cmd string, args []string, level, s if err != nil { return "", wrapAPIError(err) } - return resp.Text, nil + text := resp.Text + if truncated { + text += "\n\n" + truncationNote(mcpMaxLines) + } + if timedOut { + text += "\n\n" + timeoutNote(mcpTimeout) + } + return text, nil +} + +// truncationNote and timeoutNote are appended to the agent-visible tool +// output so a partial compact is never mistaken for full log coverage. +// Collection keeps the oldest lines and drops the newest — for incident +// work the agent usually wants the opposite, so tell it how to narrow. +func truncationNote(maxLines int) string { + return fmt.Sprintf("[codag] note: input truncated at %d lines — the oldest %d were compacted and the newest dropped. Narrow the log window (e.g. --since, --tail, --limit) and rerun for full coverage.", maxLines, maxLines) +} + +func timeoutNote(limit time.Duration) string { + return fmt.Sprintf("[codag] note: the log command was stopped at the %s collection limit; output may be incomplete. Narrow the log window (e.g. --since, --tail, --limit) and rerun for full coverage.", limit) } func compactRawText(raw string, level, service, incidentType string) (string, error) { if raw == "" { return "", fmt.Errorf("empty input") } - lines, err := readNonEmptyLines(strings.NewReader(raw), mcpMaxLines) + lines, truncated, err := readNonEmptyLines(strings.NewReader(raw), mcpMaxLines) if err != nil { return "", fmt.Errorf("read input: %w", err) } @@ -141,7 +161,11 @@ func compactRawText(raw string, level, service, incidentType string) (string, er if err != nil { return "", wrapAPIError(err) } - return resp.Text, nil + text := resp.Text + if truncated { + text += "\n\n" + truncationNote(mcpMaxLines) + } + return text, nil } func deterministicCompact(req api.CapsuleRequest) (*api.CompactResponse, error) { diff --git a/internal/mcp/lines.go b/internal/mcp/lines.go index 5ab2227..73ead1d 100644 --- a/internal/mcp/lines.go +++ b/internal/mcp/lines.go @@ -11,21 +11,23 @@ import ( const maxLineLen = 256 * 1024 -func readNonEmptyLines(r io.Reader, maxLines int) ([]string, error) { +// readNonEmptyLines reads up to maxLines non-empty lines. truncated is +// true when at least one more non-empty line existed past the cap, so +// callers can tell the agent that the compact view is missing data. +func readNonEmptyLines(r io.Reader, maxLines int) (out []string, truncated bool, err error) { scanner := bufio.NewScanner(r) scanner.Buffer(make([]byte, maxLineLen), maxLineLen) - var out []string for scanner.Scan() { line := strings.TrimRight(scanner.Text(), "\r\n") if line == "" { continue } - out = append(out, line) if maxLines > 0 && len(out) >= maxLines { - break + return out, true, scanner.Err() } + out = append(out, line) } - return out, scanner.Err() + return out, false, scanner.Err() } // linesToRecords mirrors cli/cmd/io.go:linesToRecords — converts plain diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 389dddc..aa7e62f 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -74,13 +74,31 @@ func TestValidateLogCommand_RejectsSensitiveLocalFiles(t *testing.T) { } func TestReadNonEmptyLinesCapsInput(t *testing.T) { - lines, err := readNonEmptyLines(strings.NewReader("a\n\nb\nc\n"), 2) + lines, truncated, err := readNonEmptyLines(strings.NewReader("a\n\nb\nc\n"), 2) if err != nil { t.Fatalf("readNonEmptyLines: %v", err) } if got := strings.Join(lines, ","); got != "a,b" { t.Fatalf("lines = %q, want a,b", got) } + if !truncated { + t.Fatal("truncated = false, want true — line c was dropped") + } +} + +func TestReadNonEmptyLinesNoTruncationAtExactCap(t *testing.T) { + // Blank lines after the cap are not data — reaching the cap with + // nothing real behind it must not report truncation. + lines, truncated, err := readNonEmptyLines(strings.NewReader("a\nb\n\n\n"), 2) + if err != nil { + t.Fatalf("readNonEmptyLines: %v", err) + } + if got := strings.Join(lines, ","); got != "a,b" { + t.Fatalf("lines = %q, want a,b", got) + } + if truncated { + t.Fatal("truncated = true, want false when nothing non-empty was dropped") + } } func TestWrapAPIErrorBillingRequiredGivesAgentNextSteps(t *testing.T) { @@ -180,3 +198,41 @@ func TestCompactRawTextUsesDeterministicFreeCompact(t *testing.T) { t.Fatalf("freeCalls = %d, want 1", freeCalls) } } + +// TestCompactRawTextNotesTruncation verifies the agent-visible output says +// when input was cut at the line cap, so the agent doesn't treat a partial +// compact as full log coverage. +func TestCompactRawTextNotesTruncation(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/v1/anonymous/token": + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": "cda_mcp-test", + "token_type": "Bearer", + }) + case "/v1/free/compact": + _ = json.NewEncoder(w).Encode(map[string]any{ + "text": "[x1] deterministic summary", + "stats": map[string]any{"elapsed_ms": 1}, + }) + default: + t.Fatalf("unexpected path %s", r.URL.Path) + } + })) + defer srv.Close() + t.Setenv("CODAG_SERVER", srv.URL) + + raw := strings.Repeat("error 42\n", mcpMaxLines+1) + got, err := compactRawText(raw, "info", "api", "") + if err != nil { + t.Fatalf("compactRawText: %v", err) + } + if !strings.HasPrefix(got, "[x1] deterministic summary") { + t.Fatalf("summary missing from output: %q", got) + } + if !strings.Contains(got, "truncated") || !strings.Contains(got, "oldest") { + t.Fatalf("truncation note missing from output: %q", got) + } +}