diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 84b5760079..955f9cf1ad 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -27,7 +27,8 @@ apps/backend/ │ │ ├── registry/ # Agent type registry and defaults │ │ ├── settings/ # Agent settings │ │ ├── mcpconfig/ # MCP server configuration -│ │ └── remoteauth/ # Remote auth catalog and method IDs for remote executors/UI +│ │ ├── remoteauth/ # Remote auth catalog and method IDs for remote executors/UI +│ │ └── planinjection/ # Bounds a task plan document before session-handover/dynamic-continuation injection │ ├── auth/ # Opt-in auth, per-user scoping, middleware, API, store │ ├── agentctl/ │ │ └── server/ # agentctl HTTP server @@ -282,8 +283,7 @@ Prefer stable error codes for new output so the frontend translates it. See `doc ## Code-quality limits Enforced by `apps/backend/.golangci.yml` (errors on new code only): -- Functions: ≤80 lines, ≤50 statements · Cyclomatic complexity: ≤15 · Cognitive complexity: ≤30 -- Nesting depth: ≤5 · Naked returns only in functions ≤30 lines · No duplicated blocks (≥150 tokens) · Repeated strings → constants (≥3 occurrences) · Revive's 800-effective-line file limit also applies to test files; put new tests in a new file instead of appending to an already-large test file. +- Functions: ≤80 lines, ≤50 statements · Cyclomatic complexity: ≤15 · Cognitive complexity: ≤30 · Nesting depth: ≤5 · Naked returns only in functions ≤30 lines · No duplicated blocks (≥150 tokens) · Repeated strings → constants (≥3 occurrences) · Revive's 800-effective-line file limit also applies to test files; put new tests in a new file instead of appending to an already-large test file. When a PR fixup touches backend code, run the CI-style changed-file linter locally from `apps/backend` with the PR base SHA before pushing, because CI enforces changed-file complexity thresholds: diff --git a/apps/backend/internal/agent/planinjection/reduce.go b/apps/backend/internal/agent/planinjection/reduce.go new file mode 100644 index 0000000000..fb93f876db --- /dev/null +++ b/apps/backend/internal/agent/planinjection/reduce.go @@ -0,0 +1,267 @@ +// Package planinjection bounds a task plan document before it is injected +// into a launching agent session's prompt. It is the single place budgeting, +// section selection, and truncation are implemented; callers must not carry +// their own copy of any of the three. +package planinjection + +import ( + "bytes" + "fmt" + "strings" + + "github.com/kandev/kandev/internal/sysprompt" +) + +const ( + // HandoverBudget bounds the plan text injected at session handover. + HandoverBudget = 12000 + // DynamicBudget bounds the plan text injected into a dynamic + // continuation. It must stay at or below the dynamic runtime's + // continuationFieldLimit; internal/agent/runtime/dynamic asserts that + // relationship, since the limit is unexported there. + DynamicBudget = 4000 +) + +const ( + omissionNoticeTemplate = "[Kandev: plan reduced to fit the injection budget; %d of %d sections omitted%s. Call get_task_plan_kandev for the full plan.]" + shortenedClause = ", and the retained section was shortened" + cutMarker = "[Kandev: section truncated here]" +) + +var ( + tagStartBytes = []byte(sysprompt.TagStart) + tagEndBytes = []byte(sysprompt.TagEnd) +) + +// ContainTags removes every occurrence of the and +// literals from text. A single pass, or two independent +// per-literal loops, can each be evaded by an input where removing one +// literal constructs the other, so every byte is appended to an output +// buffer one at a time and, after each append, the buffer's tail is checked +// against both literals and popped immediately on a match — equivalent to +// rescanning for both literals after every removal until neither remains, +// but in one linear pass instead of a multi-pass rescan, which is +// quadratic on adversarially nested input. +func ContainTags(text string) string { + buf := make([]byte, 0, len(text)) + for i := 0; i < len(text); i++ { + buf = append(buf, text[i]) + switch { + case bytes.HasSuffix(buf, tagEndBytes): + buf = buf[:len(buf)-len(tagEndBytes)] + case bytes.HasSuffix(buf, tagStartBytes): + buf = buf[:len(buf)-len(tagStartBytes)] + } + } + return string(buf) +} + +// Reduce bounds document to at most budget bytes. Under budget it returns +// document unchanged. Over budget it drops whole sections from the middle, +// keeping a contiguous run from each end (tail first), falling back to +// shortening the document's first section only when no whole section can be +// retained. It returns the reduced text, whether anything was dropped, and +// the number of whole sections not represented in the output at all. +func Reduce(document string, budget int) (output string, reduced bool, omittedSections int) { + if strings.TrimSpace(document) == "" { + return "", false, 0 + } + if budget <= 0 { + // Handled before any reservation is subtracted from budget: for a + // budget near the minimum int, that subtraction underflows and + // wraps positive. + return "", true, len(splitSections(document)) + } + if len(document) <= budget { + return document, false, 0 + } + + sections := splitSections(document) + total := len(sections) + noticeReservation := len(renderNotice(total, total, true)) + 1 + markerReservation := len(cutMarker) + 1 + + headRetain, tailRetain := selectSections(sections, budget-noticeReservation) + if len(headRetain) > 0 || len(tailRetain) > 0 { + return assembleWholeSections(sections, headRetain, tailRetain, total) + } + + return reduceFirstSection(sections[0], total, budget-noticeReservation-markerReservation) +} + +// selectSections grows a head run from the start and a tail run from the +// end, considering candidates alternately with the tail run first. A +// candidate is retained when it fits avail minus the sections already +// retained; when it does not, that run closes permanently. A closed run +// forfeits all its remaining turns to the other. Selection stops when both +// runs are closed or no unconsidered section remains between them. A +// non-positive avail retains nothing; the caller never reaches this today +// (Reduce guards budget <= 0 before the reservation arithmetic that could +// drive avail negative), but the invariant is made explicit so the function +// stays safe if reused. +func selectSections(sections []string, avail int) (headRetain, tailRetain []int) { + if avail <= 0 { + return nil, nil + } + return selectSectionsRun(sections, avail) +} + +func selectSectionsRun(sections []string, avail int) (headRetain, tailRetain []int) { + lo, hi := 0, len(sections)-1 + headOpen, tailOpen := true, true + nextIsTail := true + used := 0 + + for (headOpen || tailOpen) && lo <= hi { + actTail := tailOpen && (nextIsTail || !headOpen) + if headOpen && tailOpen { + nextIsTail = !nextIsTail + } + + idx := hi + if !actTail { + idx = lo + } + size := len(sections[idx]) + fits := used+size <= avail + switch { + case fits && actTail: + used += size + tailRetain = append(tailRetain, idx) + case fits: + used += size + headRetain = append(headRetain, idx) + case actTail: + tailOpen = false + default: + headOpen = false + } + + if actTail { + hi-- + } else { + lo++ + } + } + + return headRetain, tailRetain +} + +// assembleWholeSections concatenates the retained sections in original +// document order with no inserted separator, then appends the omission +// notice, preceded by a single newline only when the assembled text does +// not already end with one. +func assembleWholeSections(sections []string, headRetain, tailRetain []int, total int) (string, bool, int) { + retained := append(append([]int{}, headRetain...), reverseInts(tailRetain)...) + var sb strings.Builder + for _, idx := range retained { + sb.WriteString(sections[idx]) + } + result := sb.String() + omitted := total - len(retained) + result = appendWithSeparator(result, renderNotice(omitted, total, false)) + return result, true, omitted +} + +// reduceFirstSection keeps leading whole lines of the document's first +// section while they fit avail, cuts at the resulting line boundary, and +// marks the cut. When not even one complete line fits, it returns no plan +// text at all rather than a bare marker or notice. +func reduceFirstSection(firstSection string, total, avail int) (string, bool, int) { + lines := splitLines(firstSection) + var sb strings.Builder + used := 0 + kept := 0 + for _, line := range lines { + size := len(line) + if used+size > avail { + break + } + sb.WriteString(line) + used += size + kept++ + } + if kept == 0 { + return "", true, total + } + + result := sb.String() + result = appendWithSeparator(result, cutMarker) + result = appendWithSeparator(result, renderNotice(total-1, total, true)) + return result, true, total - 1 +} + +// appendWithSeparator appends next to base, preceded by a single newline +// only when base does not already end with one. +func appendWithSeparator(base, next string) string { + if base != "" && !strings.HasSuffix(base, "\n") { + base += "\n" + } + return base + next +} + +func renderNotice(omitted, total int, shortened bool) string { + clause := "" + if shortened { + clause = shortenedClause + } + return fmt.Sprintf(omissionNoticeTemplate, omitted, total, clause) +} + +// splitSections splits document at lines starting with "## ". Content +// before the first such line, when non-empty, is the preamble section. A +// document whose first line already begins with "## " has no preamble. A +// document with no such line is one section. +func splitSections(document string) []string { + var headings []int + for i := range document { + if i != 0 && document[i-1] != '\n' { + continue + } + if strings.HasPrefix(document[i:], "## ") { + headings = append(headings, i) + } + } + if len(headings) == 0 { + return []string{document} + } + + var sections []string + if headings[0] > 0 { + sections = append(sections, document[:headings[0]]) + } + for i, start := range headings { + end := len(document) + if i+1 < len(headings) { + end = headings[i+1] + } + sections = append(sections, document[start:end]) + } + return sections +} + +// splitLines splits s into complete lines, each running up to and including +// its trailing "\n", except a final line with no terminator, which runs to +// s's end. +func splitLines(s string) []string { + var lines []string + start := 0 + for i := 0; i < len(s); i++ { + if s[i] == '\n' { + lines = append(lines, s[start:i+1]) + start = i + 1 + } + } + if start < len(s) { + lines = append(lines, s[start:]) + } + return lines +} + +func reverseInts(s []int) []int { + out := make([]int, len(s)) + for i, v := range s { + out[len(s)-1-i] = v + } + return out +} diff --git a/apps/backend/internal/agent/planinjection/reduce_test.go b/apps/backend/internal/agent/planinjection/reduce_test.go new file mode 100644 index 0000000000..d18a8e2458 --- /dev/null +++ b/apps/backend/internal/agent/planinjection/reduce_test.go @@ -0,0 +1,618 @@ +package planinjection + +import ( + "fmt" + "math" + "strings" + "testing" + "time" + "unicode/utf8" + + "github.com/kandev/kandev/internal/sysprompt" +) + +// wantNotice mirrors the fixed template from the requirement's Terminology +// section independently of renderNotice, so tests assert against the frozen +// contract text rather than against whatever the implementation produces. +func wantNotice(omitted, total int, shortened bool) string { + clause := "" + if shortened { + clause = ", and the retained section was shortened" + } + return fmt.Sprintf("[Kandev: plan reduced to fit the injection budget; %d of %d sections omitted%s. Call get_task_plan_kandev for the full plan.]", omitted, total, clause) +} + +const wantCutMarker = "[Kandev: section truncated here]" + +// buildDoc returns a document with exactly n sections, each headed +// "## Section i" and carrying a body of bodyLen 'x' bytes. The first line +// begins with "## ", so the document has no preamble. +func buildDoc(n, bodyLen int) string { + var sb strings.Builder + for i := 1; i <= n; i++ { + fmt.Fprintf(&sb, "## Section %d\n", i) + sb.WriteString(strings.Repeat("x", bodyLen)) + sb.WriteString("\n") + } + return sb.String() +} + +func TestReduceReturnsUnchangedUnderBudget(t *testing.T) { + doc := buildDoc(3, 50) + out, reduced, omitted := Reduce(doc, len(doc)+10) + if out != doc { + t.Fatalf("output = %q, want the document unchanged", out) + } + if reduced { + t.Fatal("reduced = true for a document under budget") + } + if omitted != 0 { + t.Fatalf("omitted = %d, want 0", omitted) + } +} + +func TestReduceReturnsUnchangedAtExactBudget(t *testing.T) { + doc := buildDoc(2, 20) + out, reduced, _ := Reduce(doc, len(doc)) + if out != doc || reduced { + t.Fatalf("equality with the budget must be treated as within budget; out=%q reduced=%v", out, reduced) + } +} + +func TestReduceReturnsNothingForEmptyDocument(t *testing.T) { + out, reduced, omitted := Reduce("", 100) + if out != "" || reduced || omitted != 0 { + t.Fatalf("Reduce(\"\", 100) = (%q, %v, %d), want (\"\", false, 0)", out, reduced, omitted) + } +} + +func TestReduceReturnsNothingForWhitespaceOnlyDocument(t *testing.T) { + out, reduced, omitted := Reduce(" \n\t \n", 100) + if out != "" || reduced || omitted != 0 { + t.Fatalf("Reduce(whitespace, 100) = (%q, %v, %d), want (\"\", false, 0)", out, reduced, omitted) + } +} + +func TestReduceRetainsTailWhenFirstSectionIsOversized(t *testing.T) { + // The 71%-of-plans trap: an oversized first section must not force the + // intra-section fallback when a later section fits under AC-001.6. + doc := "## Old context\n" + strings.Repeat("a", 5000) + "\n" + + "## Working notes\n" + strings.Repeat("b", 5000) + "\n" + + "## Current state\nPR #123 opened.\n" + + out, reduced, omitted := Reduce(doc, 200) + + if !reduced { + t.Fatal("reduced = false, want true") + } + if strings.Contains(out, wantCutMarker) { + t.Fatalf("output reached the intra-section fallback; got %q", out) + } + if strings.Contains(out, "Old context") || strings.Contains(out, "Working notes") { + t.Fatalf("output retains a dropped section: %q", out) + } + if !strings.Contains(out, "Current state") || !strings.Contains(out, "PR #123 opened.") { + t.Fatalf("output is missing the retained tail section: %q", out) + } + if want := wantNotice(omitted, 3, false); !strings.HasSuffix(out, want) { + t.Fatalf("output does not end with the expected notice %q; got %q", want, out) + } + if len(out) > 200 { + t.Fatalf("len(out) = %d, exceeds budget 200", len(out)) + } +} + +func TestReduceFallsBackToFirstSectionWhenNothingWholeFits(t *testing.T) { + doc := "## Old context\n" + strings.Repeat("a", 5000) + "\n" + + "## Working notes\n" + strings.Repeat("b", 5000) + "\n" + + "## Current state\n" + strings.Repeat("c", 5000) + "\n" + + out, reduced, omitted := Reduce(doc, 300) + + if !reduced { + t.Fatal("reduced = false, want true") + } + if !strings.Contains(out, wantCutMarker) { + t.Fatalf("output did not reach the intra-section fallback; got %q", out) + } + if strings.Contains(out, "Working notes") || strings.Contains(out, "Current state") { + t.Fatalf("output retains a section beyond the shortened first one: %q", out) + } + if omitted != 2 { + t.Fatalf("omitted = %d, want {total}-1 = 2", omitted) + } + if want := wantNotice(2, 3, true); !strings.HasSuffix(out, want) { + t.Fatalf("output does not end with the expected notice %q; got %q", want, out) + } + if len(out) > 300 { + t.Fatalf("len(out) = %d, exceeds budget 300", len(out)) + } +} + +func TestReduceOmittedCountOnIntraSectionPathIsNotZero(t *testing.T) { + doc := buildDoc(4, 5000) + out, reduced, omitted := Reduce(doc, 300) + if !reduced { + t.Fatal("reduced = false, want true") + } + if omitted != 3 { + t.Fatalf("omitted = %d, want {total}-1 = 3 (not 0)", omitted) + } + if !strings.Contains(out, wantCutMarker) { + t.Fatal("expected the intra-section fallback to have run") + } +} + +func TestReduceOmittedCountOnSingleSectionFallbackIsZero(t *testing.T) { + doc := "## Only section\n" + strings.Repeat("x", 5000) + "\n" + out, reduced, omitted := Reduce(doc, 300) + if !reduced { + t.Fatal("reduced = false, want true") + } + if omitted != 0 { + t.Fatalf("omitted = %d, want 0 for a single-section document", omitted) + } + if want := wantNotice(0, 1, true); !strings.HasSuffix(out, want) { + t.Fatalf("output does not end with the expected notice %q; got %q", want, out) + } +} + +// buildTightFirstSectionDoc returns a document with n sections: the first +// built from many 1-byte lines so the greedy fill in reduceFirstSection can +// exhaust the available budget almost exactly, and n-1 trailing sections +// each too large to be retained whole, forcing AC-001.7's fallback. The +// near-zero slack this leaves is what lets a reservation-arithmetic defect +// be observed at all; buildDoc's two-line sections leave tens of bytes of +// unspent budget, which a one-byte or few-byte reservation error hides in. +func buildTightFirstSectionDoc(n int) string { + var sb strings.Builder + sb.WriteString("## First\n") + sb.WriteString(strings.Repeat("\n", 5000)) + for i := 2; i <= n; i++ { + fmt.Fprintf(&sb, "## Section %d\n", i) + sb.WriteString(strings.Repeat("x", 5000)) + sb.WriteString("\n") + } + return sb.String() +} + +func TestReduceNeverExceedsBudgetAtNarrowSectionCounts(t *testing.T) { + // N=10 and N=100 hide the missing-separator defect on the intra-section + // path; N=2, 3, 11, 101 do not, because the reserved and rendered notice + // widths coincide only at powers of ten. + for _, n := range []int{2, 3, 11, 101} { + n := n + t.Run(fmt.Sprintf("n=%d", n), func(t *testing.T) { + doc := buildTightFirstSectionDoc(n) + out, reduced, omitted := Reduce(doc, 300) + if !reduced { + t.Fatal("reduced = false, want true") + } + if !strings.Contains(out, wantCutMarker) { + t.Fatal("expected the intra-section fallback to have run") + } + if len(out) > 300 { + t.Fatalf("len(out) = %d, exceeds budget 300 at n=%d", len(out), n) + } + if omitted != n-1 { + t.Fatalf("omitted = %d, want %d", omitted, n-1) + } + }) + } +} + +func TestReduceNeverExceedsBudgetAtWideSectionCount(t *testing.T) { + // A three-digit section count exposes a fixed-width notice reservation; + // the tight first section leaves no slack for the wider rendered count + // to hide in. + doc := buildTightFirstSectionDoc(150) + out, reduced, omitted := Reduce(doc, 400) + if !reduced { + t.Fatal("reduced = false, want true") + } + if !strings.Contains(out, wantCutMarker) { + t.Fatal("expected the intra-section fallback to have run") + } + if len(out) > 400 { + t.Fatalf("len(out) = %d, exceeds budget 400", len(out)) + } + if omitted != 149 { + t.Fatalf("omitted = %d, want 149", omitted) + } +} + +func TestReduceIsDeterministic(t *testing.T) { + doc := buildDoc(20, 300) + out1, reduced1, omitted1 := Reduce(doc, 1000) + out2, reduced2, omitted2 := Reduce(doc, 1000) + if out1 != out2 || reduced1 != reduced2 || omitted1 != omitted2 { + t.Fatalf("Reduce is not deterministic: (%q,%v,%d) vs (%q,%v,%d)", out1, reduced1, omitted1, out2, reduced2, omitted2) + } +} + +func TestReduceIsIdempotentOnReducedOutput(t *testing.T) { + doc := buildDoc(20, 300) + out, reduced, _ := Reduce(doc, 1000) + if !reduced { + t.Fatal("fixture did not actually reduce; strengthen it") + } + out2, reduced2, omitted2 := Reduce(out, 1000) + if out2 != out { + t.Fatalf("re-reducing the output changed it: %q -> %q", out, out2) + } + if reduced2 { + t.Fatal("re-reducing an already-bounded output reported reduced=true") + } + if omitted2 != 0 { + t.Fatalf("re-reducing an already-bounded output reported omitted=%d, want 0", omitted2) + } +} + +func TestReduceTreatsNoHeadingLineDocumentAsOneSection(t *testing.T) { + doc := strings.Repeat("no headings here at all\n", 400) + out, reduced, omitted := Reduce(doc, 300) + if !reduced { + t.Fatal("reduced = false, want true") + } + if omitted != 0 { + t.Fatalf("omitted = %d, want 0 for a single-section document", omitted) + } + if !strings.Contains(out, wantCutMarker) { + t.Fatal("expected the intra-section fallback for a headingless document") + } +} + +func TestReduceRetainsPreambleAsFirstSection(t *testing.T) { + doc := strings.Repeat("intro ", 20) + "\n## Heading\n" + strings.Repeat("body ", 20) + "\n" + sections := splitSections(doc) + if len(sections) != 2 { + t.Fatalf("splitSections returned %d sections, want 2 (preamble + heading)", len(sections)) + } + if strings.HasPrefix(sections[0], "## ") { + t.Fatalf("section[0] should be the preamble, got %q", sections[0]) + } +} + +func TestSplitSectionsGivesNoZeroLengthPreambleWhenDocStartsWithHeading(t *testing.T) { + doc := "## Heading\nbody\n" + sections := splitSections(doc) + if len(sections) != 1 { + t.Fatalf("splitSections returned %d sections, want 1 (no preamble)", len(sections)) + } + if sections[0] != doc { + t.Fatalf("sections[0] = %q, want the whole document", sections[0]) + } +} + +// TestReduceOutputIsValidUTF8ForMultibyteInput exercises reduceFirstSection's +// line-boundary cut, not just the empty-output shadow path: the multibyte +// content is spread across many short lines, so at this budget the greedy +// fill retains several whole lines and stops mid-body, forcing the output to +// actually contain multibyte runes. A single 9000-byte line (the previous +// fixture) never fits even one line after reservations, so Reduce returns +// "" under AC-001.13 and utf8.ValidString("") trivially passes regardless of +// whether the cut logic is rune-safe. +func TestReduceOutputIsValidUTF8ForMultibyteInput(t *testing.T) { + rune3byte := "中" // a 3-byte UTF-8 rune + var body strings.Builder + for i := 0; i < 2000; i++ { + body.WriteString(strings.Repeat(rune3byte, 10)) + body.WriteString("\n") + } + doc := "## Section\n" + body.String() + + "## Oversized\n" + strings.Repeat("x", 90000) + "\n" + + out, reduced, _ := Reduce(doc, 400) + if !reduced { + t.Fatal("reduced = false, want true") + } + if out == "" { + t.Fatal("fixture did not exercise the line-boundary cut; strengthen it") + } + if !strings.Contains(out, rune3byte) { + t.Fatal("fixture did not retain any multibyte content; strengthen it") + } + if !utf8.ValidString(out) { + t.Fatalf("output is not valid UTF-8: %q", out) + } +} + +func TestReduceReturnsNothingForNonPositiveBudget(t *testing.T) { + doc := buildDoc(3, 100) + for _, budget := range []int{0, -5, math.MinInt, math.MinInt + 50} { + out, reduced, omitted := Reduce(doc, budget) + if out != "" { + t.Fatalf("budget=%d: out = %q, want \"\"", budget, out) + } + if !reduced { + t.Fatalf("budget=%d: reduced = false, want true (content was dropped)", budget) + } + if omitted != 3 { + t.Fatalf("budget=%d: omitted = %d, want 3 (total, nothing represented)", budget, omitted) + } + } +} + +func TestReduceReturnsNothingWhenBudgetBelowNoticeReservation(t *testing.T) { + doc := buildDoc(3, 100) + reservation := len(renderNotice(3, 3, true)) + 1 + out, reduced, omitted := Reduce(doc, reservation-1) + if out != "" || !reduced || omitted != 3 { + t.Fatalf("Reduce below the notice reservation = (%q, %v, %d), want (\"\", true, 3)", out, reduced, omitted) + } +} + +func TestReduceReturnsNothingWhenBudgetHoldsNoticeButNotMarker(t *testing.T) { + doc := buildDoc(3, 100) + noticeReservation := len(renderNotice(3, 3, true)) + 1 + markerReservation := len(cutMarker) + 1 + budget := noticeReservation + markerReservation - 1 + out, reduced, omitted := Reduce(doc, budget) + if out != "" || !reduced || omitted != 3 { + t.Fatalf("Reduce holding notice but not notice+marker = (%q, %v, %d), want (\"\", true, 3)", out, reduced, omitted) + } +} + +func TestReduceReturnsNothingWhenBudgetHoldsBothReservationsButNotOneLine(t *testing.T) { + // A single line far larger than what's left after both reservations. + doc := "## Section\n" + strings.Repeat("x", 5000) + "\n" + noticeReservation := len(renderNotice(1, 1, true)) + 1 + markerReservation := len(cutMarker) + 1 + budget := noticeReservation + markerReservation + 5 + out, reduced, omitted := Reduce(doc, budget) + if out != "" || !reduced || omitted != 1 { + t.Fatalf("Reduce with no complete line fitting = (%q, %v, %d), want (\"\", true, 1)", out, reduced, omitted) + } +} + +func TestReduceOutputNeverEndsWithTrailingNewline(t *testing.T) { + doc := buildDoc(20, 300) + out, reduced, _ := Reduce(doc, 1000) + if !reduced { + t.Fatal("fixture did not actually reduce; strengthen it") + } + if strings.HasSuffix(out, "\n") { + t.Fatalf("output ends with a trailing newline: %q", out) + } +} + +func TestReduceAssemblyInsertsNoSeparatorBetweenRetainedSections(t *testing.T) { + // A small first section and oversized later ones force the tail run + // closed immediately, so every remaining turn goes to the head run and + // the first section is what gets retained. + doc := "## Small\nfits easily\n" + + "## Big one\n" + strings.Repeat("a", 5000) + "\n" + + "## Big two\n" + strings.Repeat("b", 5000) + "\n" + sections := splitSections(doc) + out, reduced, _ := Reduce(doc, 300) + if !reduced { + t.Fatal("fixture did not actually reduce; strengthen it") + } + if !strings.HasPrefix(out, sections[0]) { + t.Fatalf("output does not start with the retained head section verbatim: %q", out) + } +} + +// TestReduceSelectionPrefersTailWhenOnlyOneRunCanFit is the AC-001.6 +// tail-first discriminator: a document shaped so exactly one of the head or +// tail section can be retained, never both. A head-first implementation +// would retain the opposite section here while still passing every other +// committed assertion (staying under budget, emitting a well-formed notice, +// being deterministic and idempotent) — verified by hand: flipping this +// package's tail-first tie-break to head-first left the rest of this file +// green. +func TestReduceSelectionPrefersTailWhenOnlyOneRunCanFit(t *testing.T) { + head := "## Framing\n" + strings.Repeat("h", 10) + "\n" + mid := "## Working notes\n" + strings.Repeat("m", 4000) + "\n" + tail := "## Current state\n" + strings.Repeat("t", 40) + "\n" + doc := head + mid + tail + + total := 3 + noticeReservation := len(renderNotice(total, total, true)) + 1 + // Sized to hold exactly one of the two small sections plus the notice, + // never both: the tail-first tie-break is what decides which. + budget := noticeReservation + len(tail) + + out, reduced, omitted := Reduce(doc, budget) + if !reduced { + t.Fatal("reduced = false, want true") + } + if !strings.HasPrefix(out, tail) { + t.Fatalf("output does not retain the tail section first, contrary to AC-001.6's tail-first rule: %q", out) + } + if strings.Contains(out, "Framing") { + t.Fatalf("output retains the head section instead of the tail: %q", out) + } + if omitted != 2 { + t.Fatalf("omitted = %d, want 2 (head and the oversized middle)", omitted) + } + if len(out) > budget { + t.Fatalf("len(out) = %d, exceeds budget %d", len(out), budget) + } +} + +// TestReduceAssemblyConcatenatesMultiSectionHeadAndTailInDocumentOrder covers +// what TestReduceAssemblyInsertsNoSeparatorBetweenRetainedSections's name +// promises but its single-retained-section fixture cannot show: two +// sections in the head run and two in the tail run, both non-empty, with an +// oversized section dropped from the middle. AC-001.6 requires the retained +// sections to be concatenated in original document order with no inserted +// separator. +func TestReduceAssemblyConcatenatesMultiSectionHeadAndTailInDocumentOrder(t *testing.T) { + s0 := "## H1\nh1\n" + s1 := "## H2\nh2\n" + mid := "## Mid\n" + strings.Repeat("m", 5000) + "\n" + s3 := "## T1\nt1\n" + s4 := "## T2\nt2\n" + doc := s0 + s1 + mid + s3 + s4 + + total := 5 + noticeReservation := len(renderNotice(total, total, true)) + 1 + // Exactly enough for the four small sections plus the notice; the + // oversized middle cannot fit alongside them. + budget := noticeReservation + len(s0) + len(s1) + len(s3) + len(s4) + + out, reduced, omitted := Reduce(doc, budget) + if !reduced { + t.Fatal("reduced = false, want true") + } + if omitted != 1 { + t.Fatalf("omitted = %d, want 1 (only the oversized middle section)", omitted) + } + want := s0 + s1 + s3 + s4 + if !strings.HasPrefix(out, want) { + t.Fatalf("retained sections are not concatenated in document order with no separator: got %q, want prefix %q", out, want) + } + if strings.Contains(out, "Mid") { + t.Fatalf("output retains the oversized middle section: %q", out) + } + if len(out) > budget { + t.Fatalf("len(out) = %d, exceeds budget %d", len(out), budget) + } +} + +// TestReduceSelectionClosesRunInsteadOfSkippingNonFittingSection is the +// AC-001.5/AC-001.6 contiguity discriminator: a document shaped [small, +// oversized, small, oversized] where a fitting section sits *behind* a +// non-fitting one within the same run. AC-001.6 requires that run to close +// permanently the moment a candidate does not fit, never skip past it to try +// a smaller one behind it — skipping would retain the second small section +// too, producing a "patchwork" whose hole a reader cannot locate. +// TestReduceAssemblyConcatenatesMultiSectionHeadAndTailInDocumentOrder can't +// catch a skip-instead-of-close regression: its dropped section sits at the +// head/tail boundary, where skipping and closing produce identical output. +func TestReduceSelectionClosesRunInsteadOfSkippingNonFittingSection(t *testing.T) { + s0 := "## Small1\nfits\n" + s1 := "## Big1\n" + strings.Repeat("a", 5000) + "\n" + s2 := "## Small2\nalso\n" + s3 := "## Big2\n" + strings.Repeat("b", 5000) + "\n" + doc := s0 + s1 + s2 + s3 + + out, reduced, omitted := Reduce(doc, 250) + if !reduced { + t.Fatal("reduced = false, want true") + } + if !strings.HasPrefix(out, s0) { + t.Fatalf("output does not retain the head section: %q", out) + } + if strings.Contains(out, "Small2") { + t.Fatalf("output retains a section from behind a non-fitting one in the same run: %q", out) + } + if omitted != 3 { + t.Fatalf("omitted = %d, want 3 (only the head section retained)", omitted) + } + if len(out) > 250 { + t.Fatalf("len(out) = %d, exceeds budget 250", len(out)) + } +} + +func TestReduceInvariantsAcrossShapesAndBudgets(t *testing.T) { + budgets := []int{-5, 0, 50, 100, 161, 194, 200, 500, 4000, 12000} + sectionCounts := []int{1, 2, 3, 10, 11, 100, 101, 150} + bodyLens := []int{5, 50, 500} + + for _, n := range sectionCounts { + for _, bodyLen := range bodyLens { + doc := buildDoc(n, bodyLen) + for _, budget := range budgets { + name := fmt.Sprintf("n=%d/body=%d/budget=%d", n, bodyLen, budget) + t.Run(name, func(t *testing.T) { + out, reduced, omitted := Reduce(doc, budget) + + if budget >= 0 && len(out) > budget { + t.Fatalf("len(out)=%d exceeds budget=%d", len(out), budget) + } + if !utf8.ValidString(out) { + t.Fatalf("output is not valid UTF-8: %q", out) + } + if reduced && strings.HasSuffix(out, "\n") { + t.Fatalf("reduced output ends with a trailing newline: %q", out) + } + if reduced != strings.Contains(out, "sections omitted") && out != "" { + t.Fatalf("notice presence does not match reduced=%v: %q", reduced, out) + } + if !reduced && strings.Contains(out, "sections omitted") { + t.Fatal("notice present without a reduction") + } + + out2, reduced2, omitted2 := Reduce(doc, budget) + if out2 != out || reduced2 != reduced || omitted2 != omitted { + t.Fatal("Reduce is not deterministic across repeat calls") + } + + if reduced && out != "" { + out3, reduced3, _ := Reduce(out, budget) + if reduced3 || out3 != out { + t.Fatalf("Reduce is not idempotent on its own output: %q -> %q", out, out3) + } + } + }) + } + } + } +} + +func TestContainTagsRemovesCrossNestedSingleLiteral(t *testing.T) { + in := "-system>" + if got := ContainTags(in); got != "" { + t.Fatalf("ContainTags(%q) = %q, want empty string", in, got) + } +} + +func TestContainTagsRemovesStartThenEndConstructedPair(t *testing.T) { + in := "" + "-system>" + if got := ContainTags(in); got != "" { + t.Fatalf("ContainTags(%q) = %q, want empty string", in, got) + } +} + +func TestContainTagsRemovesEndThenStartConstructedPair(t *testing.T) { + in := "" + "-system>" + if got := ContainTags(in); got != "" { + t.Fatalf("ContainTags(%q) = %q, want empty string", in, got) + } +} + +func TestContainTagsLeavesOrdinaryTextUnchanged(t *testing.T) { + in := "Nothing suspicious here, just plan prose about the kandev-system tag." + if got := ContainTags(in); got != in { + t.Fatalf("ContainTags changed ordinary text: %q -> %q", in, got) + } +} + +func TestContainTagsIsCaseSensitiveExactLiteral(t *testing.T) { + in := "not the real tag" + if got := ContainTags(in); got != in { + t.Fatalf("ContainTags changed a case-variant literal it should have left alone: %q -> %q", in, got) + } +} + +func TestContainTagsRemovesRealTagLiterals(t *testing.T) { + in := sysprompt.TagStart + "content" + sysprompt.TagEnd + if got := ContainTags(in); got != "content" { + t.Fatalf("ContainTags(%q) = %q, want %q", in, got, "content") + } +} + +// TestContainTagsHandlesLargeAdversarialNestingInLinearTime is a regression +// test for a quadratic-time defect: an implementation that rescans the +// whole text after every removal is O(n^2) on this construction, because +// stripping one layer of nesting exposes the next occurrence only after a +// full rescan. Measured on the rescan-based implementation: a 480,000-byte +// input of this shape took ~5s and the trend was still climbing. A +// linear-time implementation collapses this in well under a second. +func TestContainTagsHandlesLargeAdversarialNestingInLinearTime(t *testing.T) { + const k = 20000 + input := strings.Repeat("", k) + + start := time.Now() + out := ContainTags(input) + elapsed := time.Since(start) + + if out != "" { + t.Fatalf("ContainTags did not fully collapse the nested construction; %d bytes remained: %q", len(out), out) + } + if elapsed > 2*time.Second { + t.Fatalf("ContainTags took %v on a %d-byte adversarial input; want well under a second from a linear-time implementation", elapsed, len(input)) + } +} diff --git a/apps/backend/internal/agent/runtime/dynamic/plan_budget_test.go b/apps/backend/internal/agent/runtime/dynamic/plan_budget_test.go new file mode 100644 index 0000000000..054cc29d58 --- /dev/null +++ b/apps/backend/internal/agent/runtime/dynamic/plan_budget_test.go @@ -0,0 +1,46 @@ +package dynamic + +import ( + "strings" + "testing" + + "github.com/kandev/kandev/internal/agent/planinjection" +) + +// TestDynamicPlanBudgetDoesNotExceedContinuationFieldLimit is the AC-002.4 +// assertion: continuationFieldLimit is unexported, so this relationship can +// only be checked from inside this package. A later increase to +// planinjection.DynamicBudget above continuationFieldLimit would silently +// restore bounded()'s head-only truncation and cut the reducer's own notice +// off the end. +func TestDynamicPlanBudgetDoesNotExceedContinuationFieldLimit(t *testing.T) { + if planinjection.DynamicBudget > continuationFieldLimit { + t.Fatalf("planinjection.DynamicBudget = %d, want <= continuationFieldLimit = %d", + planinjection.DynamicBudget, continuationFieldLimit) + } +} + +// TestBoundedIsNoOpOnReducedPlanOutput asserts the second condition AC-002.4 +// requires: because bounded() unconditionally TrimSpaces before its length +// check, the reducer's output must carry no leading or trailing whitespace +// whenever its input carries none, or the call would silently re-trim +// already-bounded plan text. +func TestBoundedIsNoOpOnReducedPlanOutput(t *testing.T) { + var doc strings.Builder + for i := 1; i <= 300; i++ { + doc.WriteString("## Section ") + doc.WriteString(strings.Repeat("x", 20)) + doc.WriteString("\n") + doc.WriteString(strings.Repeat("y", 60)) + doc.WriteString("\n") + } + + out, reduced, _ := planinjection.Reduce(doc.String(), planinjection.DynamicBudget) + if !reduced { + t.Fatal("fixture did not actually reduce the plan; strengthen it") + } + + if got := bounded(out); got != out { + t.Fatalf("bounded(reducerOutput) = %q, want it unchanged (%q)", got, out) + } +} diff --git a/apps/backend/internal/orchestrator/dynamic_launch.go b/apps/backend/internal/orchestrator/dynamic_launch.go index 1f856cb4be..27ea630a57 100644 --- a/apps/backend/internal/orchestrator/dynamic_launch.go +++ b/apps/backend/internal/orchestrator/dynamic_launch.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" + "github.com/kandev/kandev/internal/agent/planinjection" agentruntime "github.com/kandev/kandev/internal/agent/runtime" dynamicruntime "github.com/kandev/kandev/internal/agent/runtime/dynamic" "github.com/kandev/kandev/internal/agent/runtime/routingerr" @@ -372,8 +373,24 @@ func (s *Service) addDynamicPlan(ctx context.Context, taskID string, input *dyna } return fmt.Errorf("load plan for dynamic continuation: %w", err) } - if plan != nil { - input.PlanSummary = strings.TrimSpace(plan.Title + "\n" + plan.Content) + if plan == nil { + return nil + } + + composed := strings.TrimSpace(plan.Title + "\n" + plan.Content) + // ContainTags is not called here: PlanSummary lands in the plain-text + // continuation package (ContinuationPrompt), not in a + // block. If that ever changes, add ContainTags(composed) before Reduce. + reducedPlan, reduced, omitted := planinjection.Reduce(composed, planinjection.DynamicBudget) + input.PlanSummary = reducedPlan + if reduced { + s.logger.Info("reducing dynamic continuation plan", + zap.String("site", "dynamic_continuation"), + zap.String("task_id", taskID), + zap.Int("plan_input_bytes", len(composed)), + zap.Int("plan_output_bytes", len(reducedPlan)), + zap.Int("plan_sections_omitted", omitted), + ) } return nil } diff --git a/apps/backend/internal/orchestrator/dynamic_launch_plan_injection_test.go b/apps/backend/internal/orchestrator/dynamic_launch_plan_injection_test.go new file mode 100644 index 0000000000..39e206aa20 --- /dev/null +++ b/apps/backend/internal/orchestrator/dynamic_launch_plan_injection_test.go @@ -0,0 +1,272 @@ +package orchestrator + +import ( + "context" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/agent/planinjection" + dynamicruntime "github.com/kandev/kandev/internal/agent/runtime/dynamic" + commonlogger "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/models" +) + +// newTestServiceWithObservedLogger builds a minimal Service — just the repo +// and an in-memory log sink — sufficient for addDynamicPlan, which only +// touches those two fields. Used to assert on the AC-002.6 log fields. +func newTestServiceWithObservedLogger(t *testing.T, repo sessionExecutorStore) (*Service, *observer.ObservedLogs) { + t.Helper() + core, logs := observer.New(zapcore.InfoLevel) + log, err := commonlogger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + return &Service{repo: repo, logger: log}, logs +} + +// fieldValue returns the field with the given key from a logged entry's +// context, and whether it was present at all — a present field can still +// carry a zero value, which the absent-vs-zero AC-002.6 tests need to tell +// apart. +func fieldValue(fields []zapcore.Field, key string) (zapcore.Field, bool) { + for _, f := range fields { + if f.Key == key { + return f, true + } + } + return zapcore.Field{}, false +} + +// TestAddDynamicPlanBoundsOverBudgetPlan is the AC-002.2 anti-bypass test for +// the dynamic continuation site: it exercises addDynamicPlan itself, not the +// reducer, so a site that stopped calling the reducer would fail this test +// even though the reducer's own tests still pass. +func TestAddDynamicPlanBoundsOverBudgetPlan(t *testing.T) { + ctx := context.Background() + const taskID = "task-dynamic-plan-over-budget" + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, taskID, "session-dynamic-plan-over-budget", models.TaskSessionStateRunning) + + var body strings.Builder + for i := 1; i <= 400; i++ { + body.WriteString("## Section ") + body.WriteString(strings.Repeat("x", 40)) + body.WriteString("\n") + body.WriteString(strings.Repeat("y", 80)) + body.WriteString("\n") + } + if err := repo.CreateTaskPlan(ctx, &models.TaskPlan{ + ID: "plan-dynamic-over-budget", TaskID: taskID, Title: "Big plan", Content: body.String(), + }); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + + taskRepo := newMockTaskRepo() + svc := createTestServiceWithScheduler(repo, newMockStepGetter(), taskRepo, &mockAgentManager{}) + + var input dynamicruntime.ContinuationInput + if err := svc.addDynamicPlan(ctx, taskID, &input); err != nil { + t.Fatalf("addDynamicPlan: %v", err) + } + + if !strings.Contains(input.PlanSummary, "sections omitted") || !strings.Contains(input.PlanSummary, "get_task_plan_kandev") { + t.Fatalf("PlanSummary does not carry the omission notice: %q", input.PlanSummary) + } + if len(input.PlanSummary) > 4000 { + t.Fatalf("PlanSummary len = %d, exceeds the dynamic budget", len(input.PlanSummary)) + } +} + +func TestAddDynamicPlanIsByteIdenticalUnderBudget(t *testing.T) { + ctx := context.Background() + const taskID = "task-dynamic-plan-under-budget" + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, taskID, "session-dynamic-plan-under-budget", models.TaskSessionStateRunning) + + if err := repo.CreateTaskPlan(ctx, &models.TaskPlan{ + ID: "plan-dynamic-under-budget", TaskID: taskID, Title: "Small plan", Content: "## Notes\nJust a few notes.\n", + }); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + + taskRepo := newMockTaskRepo() + svc := createTestServiceWithScheduler(repo, newMockStepGetter(), taskRepo, &mockAgentManager{}) + + var input dynamicruntime.ContinuationInput + if err := svc.addDynamicPlan(ctx, taskID, &input); err != nil { + t.Fatalf("addDynamicPlan: %v", err) + } + + want := "Small plan\n## Notes\nJust a few notes." + if input.PlanSummary != want { + t.Fatalf("PlanSummary = %q, want %q (byte-identical to today's composition)", input.PlanSummary, want) + } +} + +// TestAddDynamicPlanInjectsBareTitleWhenContentIsWhitespaceOnly is the +// AC-001.12 named exception for the dynamic site: a plan with a title but +// whitespace-only content composes to a non-empty document (the bare +// title), unlike the handover site where the composed document is content +// alone and this same input injects nothing. +func TestAddDynamicPlanInjectsBareTitleWhenContentIsWhitespaceOnly(t *testing.T) { + ctx := context.Background() + const taskID = "task-dynamic-plan-bare-title" + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, taskID, "session-dynamic-plan-bare-title", models.TaskSessionStateRunning) + + if err := repo.CreateTaskPlan(ctx, &models.TaskPlan{ + ID: "plan-dynamic-bare-title", TaskID: taskID, Title: "Some title", Content: " \n\t \n", + }); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + + taskRepo := newMockTaskRepo() + svc := createTestServiceWithScheduler(repo, newMockStepGetter(), taskRepo, &mockAgentManager{}) + + var input dynamicruntime.ContinuationInput + if err := svc.addDynamicPlan(ctx, taskID, &input); err != nil { + t.Fatalf("addDynamicPlan: %v", err) + } + + if input.PlanSummary != "Some title" { + t.Fatalf("PlanSummary = %q, want %q (bare title)", input.PlanSummary, "Some title") + } +} + +// TestAddDynamicPlanLogsReductionFields is the AC-002.6 assertion for the +// dynamic site: on a reducing plan, the "reducing dynamic continuation plan" +// record carries exactly the five documented keys, "site" set to the literal +// "dynamic_continuation", and byte/section counts matching what the reducer +// actually returned. +func TestAddDynamicPlanLogsReductionFields(t *testing.T) { + ctx := context.Background() + const taskID = "task-dynamic-plan-log-reduced" + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, taskID, "session-dynamic-plan-log-reduced", models.TaskSessionStateRunning) + + var body strings.Builder + for i := 1; i <= 400; i++ { + body.WriteString("## Section ") + body.WriteString(strings.Repeat("x", 40)) + body.WriteString("\n") + body.WriteString(strings.Repeat("y", 80)) + body.WriteString("\n") + } + title := "Big plan" + content := body.String() + if err := repo.CreateTaskPlan(ctx, &models.TaskPlan{ + ID: "plan-dynamic-log-reduced", TaskID: taskID, Title: title, Content: content, + }); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + + composed := strings.TrimSpace(title + "\n" + content) + wantOutput, wantReduced, wantOmitted := planinjection.Reduce(composed, planinjection.DynamicBudget) + if !wantReduced { + t.Fatal("fixture did not actually reduce; strengthen it") + } + + svc, logs := newTestServiceWithObservedLogger(t, repo) + var input dynamicruntime.ContinuationInput + if err := svc.addDynamicPlan(ctx, taskID, &input); err != nil { + t.Fatalf("addDynamicPlan: %v", err) + } + + entries := logs.FilterMessage("reducing dynamic continuation plan").All() + if len(entries) != 1 { + t.Fatalf("log entries = %d, want 1; all=%v", len(entries), logs.All()) + } + fields := entries[0].Context + + if f, ok := fieldValue(fields, "site"); !ok || f.String != "dynamic_continuation" { + t.Fatalf(`"site" field = %+v (present=%v), want "dynamic_continuation"`, f, ok) + } + if f, ok := fieldValue(fields, "task_id"); !ok || f.String != taskID { + t.Fatalf(`"task_id" field = %+v (present=%v), want %q`, f, ok, taskID) + } + if f, ok := fieldValue(fields, "plan_input_bytes"); !ok || f.Integer != int64(len(composed)) { + t.Fatalf(`"plan_input_bytes" field = %+v (present=%v), want %d`, f, ok, len(composed)) + } + if f, ok := fieldValue(fields, "plan_output_bytes"); !ok || f.Integer != int64(len(wantOutput)) { + t.Fatalf(`"plan_output_bytes" field = %+v (present=%v), want %d`, f, ok, len(wantOutput)) + } + if f, ok := fieldValue(fields, "plan_sections_omitted"); !ok || f.Integer != int64(wantOmitted) { + t.Fatalf(`"plan_sections_omitted" field = %+v (present=%v), want %d`, f, ok, wantOmitted) + } +} + +// TestAddDynamicPlanLogsZeroOutputWhenNothingFits is the AC-001.13 shape of +// AC-002.6: when the reducer cannot emit any plan content at all, that IS a +// reduction and must be logged, with plan_output_bytes at zero and +// plan_sections_omitted equal to the document's total section count. +func TestAddDynamicPlanLogsZeroOutputWhenNothingFits(t *testing.T) { + ctx := context.Background() + const taskID = "task-dynamic-plan-log-nothing-fits" + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, taskID, "session-dynamic-plan-log-nothing-fits", models.TaskSessionStateRunning) + + // A title alone, with no content: composing trims the sole "\n" this + // site inserts between title and content, leaving one line — the title + // itself — far larger than DynamicBudget, so not even one complete line + // fits after reservations. (An empty title would default to "Plan" at + // the repository, which fits trivially and defeats the fixture.) + title := strings.Repeat("T", planinjection.DynamicBudget*2) + if err := repo.CreateTaskPlan(ctx, &models.TaskPlan{ + ID: "plan-dynamic-log-nothing-fits", TaskID: taskID, Title: title, Content: "", + }); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + + svc, logs := newTestServiceWithObservedLogger(t, repo) + var input dynamicruntime.ContinuationInput + if err := svc.addDynamicPlan(ctx, taskID, &input); err != nil { + t.Fatalf("addDynamicPlan: %v", err) + } + if input.PlanSummary != "" { + t.Fatalf("PlanSummary = %q, want empty when nothing fits", input.PlanSummary) + } + + entries := logs.FilterMessage("reducing dynamic continuation plan").All() + if len(entries) != 1 { + t.Fatalf("log entries = %d, want 1; all=%v", len(entries), logs.All()) + } + fields := entries[0].Context + + if f, ok := fieldValue(fields, "plan_output_bytes"); !ok || f.Integer != 0 { + t.Fatalf(`"plan_output_bytes" field = %+v (present=%v), want 0`, f, ok) + } + if f, ok := fieldValue(fields, "plan_sections_omitted"); !ok || f.Integer != 1 { + t.Fatalf(`"plan_sections_omitted" field = %+v (present=%v), want 1 (single section, nothing represented)`, f, ok) + } +} + +// TestAddDynamicPlanEmitsNoRecordWhenNotReduced is the negative half of +// AC-002.6 for the dynamic site: unlike the handover site, which always logs +// its own "injecting session handover context" record, the dynamic site +// emits no log at all when nothing was reduced. +func TestAddDynamicPlanEmitsNoRecordWhenNotReduced(t *testing.T) { + ctx := context.Background() + const taskID = "task-dynamic-plan-log-not-reduced" + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, taskID, "session-dynamic-plan-log-not-reduced", models.TaskSessionStateRunning) + + if err := repo.CreateTaskPlan(ctx, &models.TaskPlan{ + ID: "plan-dynamic-log-not-reduced", TaskID: taskID, Title: "Small plan", Content: "## Notes\nJust a few notes.\n", + }); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + + svc, logs := newTestServiceWithObservedLogger(t, repo) + var input dynamicruntime.ContinuationInput + if err := svc.addDynamicPlan(ctx, taskID, &input); err != nil { + t.Fatalf("addDynamicPlan: %v", err) + } + + if got := logs.Len(); got != 0 { + t.Fatalf("log entries = %d, want 0 (dynamic site emits no record when not reduced); all=%v", got, logs.All()) + } +} diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 794c1cb277..95bc1ca07c 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -12,6 +12,7 @@ import ( "time" "github.com/google/uuid" + "github.com/kandev/kandev/internal/agent/planinjection" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/agent/runtime/routingerr" "github.com/kandev/kandev/internal/common/constants" @@ -2232,17 +2233,33 @@ func (e *Executor) injectHandoverIfNeeded(ctx context.Context, taskID, currentSe return prompt } - // Build the plan section if a plan exists. + // Build the plan section if a plan exists. The composed document is the + // content verbatim (this site does not trim it); containment runs before + // the reducer so the reducer's guarantees hold over the contained text. var planSection string - plan, err := e.repo.GetTaskPlan(ctx, taskID) - if err == nil && plan != nil && plan.Content != "" { - planSection = fmt.Sprintf("\nThe task has an implementation plan:\n\n%s\n", plan.Content) - } - - e.logger.Info("injecting session handover context", + logFields := []zap.Field{ zap.String("task_id", taskID), zap.String("session_id", currentSessionID), - zap.Int("previous_sessions", previousCount)) + zap.Int("previous_sessions", previousCount), + } + plan, err := e.repo.GetTaskPlan(ctx, taskID) + if err == nil && plan != nil { + composed := planinjection.ContainTags(plan.Content) + reducedPlan, reduced, omitted := planinjection.Reduce(composed, planinjection.HandoverBudget) + if reducedPlan != "" { + planSection = fmt.Sprintf("\nThe task has an implementation plan:\n\n%s\n", reducedPlan) + } + if reduced { + logFields = append(logFields, + zap.String("site", "handover"), + zap.Int("plan_input_bytes", len(composed)), + zap.Int("plan_output_bytes", len(reducedPlan)), + zap.Int("plan_sections_omitted", omitted), + ) + } + } + + e.logger.Info("injecting session handover context", logFields...) return sysprompt.InjectSessionHandover(previousCount, planSection, prompt) } diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index 0535bb5e58..e11d1cc867 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -273,6 +273,7 @@ type mockRepository struct { executorsRunning map[string]*models.ExecutorRunning taskEnvironments map[string]*models.TaskEnvironment taskEnvironmentRepos map[string][]*models.TaskEnvironmentRepo // env_id → rows + plans map[string]*models.TaskPlan // task_id → plan // Optional hook to inject behavior into GetTaskSession (e.g. simulate a // transient DB error); if nil, the default map lookup is used. @@ -374,6 +375,7 @@ func newMockRepository() *mockRepository { executorsRunning: make(map[string]*models.ExecutorRunning), taskEnvironments: make(map[string]*models.TaskEnvironment), taskEnvironmentRepos: make(map[string][]*models.TaskEnvironmentRepo), + plans: make(map[string]*models.TaskPlan), updateTaskStateIfNotArchivedCh: make(chan struct{}, 8), } } @@ -1270,12 +1272,29 @@ func (m *mockRepository) UpdateTaskEnvironmentRepo(_ context.Context, repo *mode } // Task Plan operations -func (m *mockRepository) CreateTaskPlan(ctx context.Context, plan *models.TaskPlan) error { return nil } -func (m *mockRepository) GetTaskPlan(ctx context.Context, taskID string) (*models.TaskPlan, error) { - return nil, nil +func (m *mockRepository) CreateTaskPlan(_ context.Context, plan *models.TaskPlan) error { + m.mu.Lock() + defer m.mu.Unlock() + m.plans[plan.TaskID] = plan + return nil +} +func (m *mockRepository) GetTaskPlan(ctx context.Context, taskID string) (*models.TaskPlan, error) { + m.mu.Lock() + defer m.mu.Unlock() + return m.plans[taskID], nil +} +func (m *mockRepository) UpdateTaskPlan(_ context.Context, plan *models.TaskPlan) error { + m.mu.Lock() + defer m.mu.Unlock() + m.plans[plan.TaskID] = plan + return nil +} +func (m *mockRepository) DeleteTaskPlan(_ context.Context, taskID string) error { + m.mu.Lock() + defer m.mu.Unlock() + delete(m.plans, taskID) + return nil } -func (m *mockRepository) UpdateTaskPlan(ctx context.Context, plan *models.TaskPlan) error { return nil } -func (m *mockRepository) DeleteTaskPlan(ctx context.Context, taskID string) error { return nil } // Session File Review operations func (m *mockRepository) UpsertSessionFileReview(ctx context.Context, review *models.SessionFileReview) error { diff --git a/apps/backend/internal/orchestrator/executor/executor_plan_injection_test.go b/apps/backend/internal/orchestrator/executor/executor_plan_injection_test.go new file mode 100644 index 0000000000..966df88ae7 --- /dev/null +++ b/apps/backend/internal/orchestrator/executor/executor_plan_injection_test.go @@ -0,0 +1,317 @@ +package executor + +import ( + "context" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/agent/planinjection" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/sysprompt" + "github.com/kandev/kandev/internal/task/models" +) + +// newTestExecutorWithObservedLogger is newTestExecutor plus an in-memory log +// sink, for asserting on the AC-002.6 log fields injectHandoverIfNeeded emits. +func newTestExecutorWithObservedLogger(t *testing.T, repo *mockRepository) (*Executor, *observer.ObservedLogs) { + t.Helper() + core, logs := observer.New(zapcore.InfoLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + exec := NewExecutor(&mockAgentManager{}, repo, log, ExecutorConfig{ + ShellPrefs: &mockShellPrefs{}, + }) + exec.SetCapabilities(&mockCapabilities{}) + return exec, logs +} + +// fieldValue returns the field with the given key from a logged entry's +// context, and whether it was present at all — a present field can still +// carry a zero value, which the absent-vs-zero AC-002.6 tests need to tell +// apart. +func fieldValue(fields []zapcore.Field, key string) (zapcore.Field, bool) { + for _, f := range fields { + if f.Key == key { + return f, true + } + } + return zapcore.Field{}, false +} + +// TestInjectHandoverIfNeededBoundsOverBudgetPlan is the AC-002.2 anti-bypass +// test for the handover site: it exercises injectHandoverIfNeeded itself, +// not the reducer, so a site that stopped calling the reducer would fail +// this test even though the reducer's own tests still pass. +func TestInjectHandoverIfNeededBoundsOverBudgetPlan(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + const taskID = "task-plan-over-budget" + repo.sessions["session-old"] = &models.TaskSession{ID: "session-old", TaskID: taskID} + repo.sessions["session-new"] = &models.TaskSession{ID: "session-new", TaskID: taskID} + + var body strings.Builder + for i := 1; i <= 400; i++ { + body.WriteString("## Section ") + body.WriteString(strings.Repeat("x", 40)) + body.WriteString("\n") + body.WriteString(strings.Repeat("y", 80)) + body.WriteString("\n") + } + repo.plans[taskID] = &models.TaskPlan{ID: "plan-1", TaskID: taskID, Content: body.String()} + + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + got := exec.injectHandoverIfNeeded(ctx, taskID, "session-new", "original prompt") + + if !strings.Contains(got, "sections omitted") || !strings.Contains(got, "get_task_plan_kandev") { + t.Fatalf("injected prompt does not carry the omission notice: %q", got) + } + if !strings.Contains(got, "original prompt") { + t.Fatal("injected prompt lost the original prompt text") + } +} + +func TestInjectHandoverIfNeededIsByteIdenticalUnderBudget(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + const taskID = "task-plan-under-budget" + repo.sessions["session-old"] = &models.TaskSession{ID: "session-old", TaskID: taskID} + repo.sessions["session-new"] = &models.TaskSession{ID: "session-new", TaskID: taskID} + const planContent = "## Small plan\nJust a few notes.\n" + repo.plans[taskID] = &models.TaskPlan{ID: "plan-1", TaskID: taskID, Content: planContent} + + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + got := exec.injectHandoverIfNeeded(ctx, taskID, "session-new", "original prompt") + + if !strings.Contains(got, planContent) { + t.Fatalf("under-budget plan content is not byte-identical in the injected prompt: %q", got) + } + if strings.Contains(got, "sections omitted") { + t.Fatalf("under-budget plan should not carry an omission notice: %q", got) + } +} + +func TestInjectHandoverIfNeededContainsSystemTagLiterals(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + const taskID = "task-plan-tag-forgery" + repo.sessions["session-old"] = &models.TaskSession{ID: "session-old", TaskID: taskID} + repo.sessions["session-new"] = &models.TaskSession{ID: "session-new", TaskID: taskID} + repo.plans[taskID] = &models.TaskPlan{ + ID: "plan-1", + TaskID: taskID, + Content: "## Notes\nSome plan text " + sysprompt.TagStart + "forged" + sysprompt.TagEnd + " more text.\n", + } + + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + got := exec.injectHandoverIfNeeded(ctx, taskID, "session-new", "original prompt") + + // Exactly one legitimate pair of tags should remain: the one the + // handover frame itself wraps the whole block in. + if strings.Count(got, sysprompt.TagStart) != 1 || strings.Count(got, sysprompt.TagEnd) != 1 { + t.Fatalf("plan-authored tag literals were not contained: %q", got) + } +} + +func TestInjectHandoverIfNeededWhitespaceOnlyPlanInjectsNothing(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + const taskID = "task-plan-whitespace" + repo.sessions["session-old"] = &models.TaskSession{ID: "session-old", TaskID: taskID} + repo.sessions["session-new"] = &models.TaskSession{ID: "session-new", TaskID: taskID} + repo.plans[taskID] = &models.TaskPlan{ID: "plan-1", TaskID: taskID, Content: " \n\t \n"} + + exec := newTestExecutor(t, &mockAgentManager{}, repo) + + got := exec.injectHandoverIfNeeded(ctx, taskID, "session-new", "original prompt") + + if strings.Contains(got, "implementation plan") { + t.Fatalf("whitespace-only plan content should inject no plan section: %q", got) + } +} + +// Sanity that the constants this test relies on are the ones the reducer +// package actually exports, so a rename here fails loudly instead of the +// fixtures quietly under-running the real budget. +func TestHandoverBudgetConstantIsPositive(t *testing.T) { + if planinjection.HandoverBudget <= 0 { + t.Fatalf("planinjection.HandoverBudget = %d, want > 0", planinjection.HandoverBudget) + } +} + +// TestInjectHandoverIfNeededLogsReductionFields is the AC-002.6 assertion for +// the handover site: on a reducing plan, the "injecting session handover +// context" record carries exactly the five documented keys, "site" set to +// the literal "handover", and byte/section counts matching what the reducer +// actually returned. +func TestInjectHandoverIfNeededLogsReductionFields(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + const taskID = "task-plan-log-reduced" + repo.sessions["session-old"] = &models.TaskSession{ID: "session-old", TaskID: taskID} + repo.sessions["session-new"] = &models.TaskSession{ID: "session-new", TaskID: taskID} + + var body strings.Builder + for i := 1; i <= 400; i++ { + body.WriteString("## Section ") + body.WriteString(strings.Repeat("x", 40)) + body.WriteString("\n") + body.WriteString(strings.Repeat("y", 80)) + body.WriteString("\n") + } + content := body.String() + repo.plans[taskID] = &models.TaskPlan{ID: "plan-1", TaskID: taskID, Content: content} + + wantOutput, wantReduced, wantOmitted := planinjection.Reduce(planinjection.ContainTags(content), planinjection.HandoverBudget) + if !wantReduced { + t.Fatal("fixture did not actually reduce; strengthen it") + } + + exec, logs := newTestExecutorWithObservedLogger(t, repo) + exec.injectHandoverIfNeeded(ctx, taskID, "session-new", "original prompt") + + entries := logs.FilterMessage("injecting session handover context").All() + if len(entries) != 1 { + t.Fatalf("log entries = %d, want 1; all=%v", len(entries), logs.All()) + } + fields := entries[0].Context + + if f, ok := fieldValue(fields, "site"); !ok || f.String != "handover" { + t.Fatalf(`"site" field = %+v (present=%v), want "handover"`, f, ok) + } + if f, ok := fieldValue(fields, "task_id"); !ok || f.String != taskID { + t.Fatalf(`"task_id" field = %+v (present=%v), want %q`, f, ok, taskID) + } + if f, ok := fieldValue(fields, "plan_input_bytes"); !ok || f.Integer != int64(len(content)) { + t.Fatalf(`"plan_input_bytes" field = %+v (present=%v), want %d`, f, ok, len(content)) + } + if f, ok := fieldValue(fields, "plan_output_bytes"); !ok || f.Integer != int64(len(wantOutput)) { + t.Fatalf(`"plan_output_bytes" field = %+v (present=%v), want %d`, f, ok, len(wantOutput)) + } + if f, ok := fieldValue(fields, "plan_sections_omitted"); !ok || f.Integer != int64(wantOmitted) { + t.Fatalf(`"plan_sections_omitted" field = %+v (present=%v), want %d`, f, ok, wantOmitted) + } +} + +// TestInjectHandoverIfNeededLogsZeroOutputWhenNothingFits is the AC-001.13 +// shape of AC-002.6: when the reducer cannot emit any plan content at all, +// that IS a reduction and must be logged, with plan_output_bytes at zero and +// plan_sections_omitted equal to the document's total section count. +func TestInjectHandoverIfNeededLogsZeroOutputWhenNothingFits(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + const taskID = "task-plan-log-nothing-fits" + repo.sessions["session-old"] = &models.TaskSession{ID: "session-old", TaskID: taskID} + repo.sessions["session-new"] = &models.TaskSession{ID: "session-new", TaskID: taskID} + + // No heading and no internal newline: the whole content is a single line + // far larger than HandoverBudget, so not even one complete line fits + // after reservations. + content := strings.Repeat("x", planinjection.HandoverBudget*2) + repo.plans[taskID] = &models.TaskPlan{ID: "plan-1", TaskID: taskID, Content: content} + + exec, logs := newTestExecutorWithObservedLogger(t, repo) + got := exec.injectHandoverIfNeeded(ctx, taskID, "session-new", "original prompt") + + if strings.Contains(got, "implementation plan") { + t.Fatalf("expected no plan section injected when nothing fits: %q", got) + } + + entries := logs.FilterMessage("injecting session handover context").All() + if len(entries) != 1 { + t.Fatalf("log entries = %d, want 1; all=%v", len(entries), logs.All()) + } + fields := entries[0].Context + + if f, ok := fieldValue(fields, "plan_output_bytes"); !ok || f.Integer != 0 { + t.Fatalf(`"plan_output_bytes" field = %+v (present=%v), want 0`, f, ok) + } + if f, ok := fieldValue(fields, "plan_sections_omitted"); !ok || f.Integer != 1 { + t.Fatalf(`"plan_sections_omitted" field = %+v (present=%v), want 1 (single section, nothing represented)`, f, ok) + } +} + +// TestInjectHandoverIfNeededLogsNoReductionFieldsWhenPlanFits is the negative +// half of AC-002.6: when nothing was reduced, the four reduction fields and +// "site" must be ABSENT, not present with a zero value, while the record's +// existing fields (task_id, session_id, previous_sessions) still fire. +func TestInjectHandoverIfNeededLogsNoReductionFieldsWhenPlanFits(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + const taskID = "task-plan-log-fits" + repo.sessions["session-old"] = &models.TaskSession{ID: "session-old", TaskID: taskID} + repo.sessions["session-new"] = &models.TaskSession{ID: "session-new", TaskID: taskID} + repo.plans[taskID] = &models.TaskPlan{ID: "plan-1", TaskID: taskID, Content: "## Small plan\nJust a few notes.\n"} + + exec, logs := newTestExecutorWithObservedLogger(t, repo) + exec.injectHandoverIfNeeded(ctx, taskID, "session-new", "original prompt") + + entries := logs.FilterMessage("injecting session handover context").All() + if len(entries) != 1 { + t.Fatalf("log entries = %d, want 1; all=%v", len(entries), logs.All()) + } + fields := entries[0].Context + + if f, ok := fieldValue(fields, "task_id"); !ok || f.String != taskID { + t.Fatalf(`"task_id" field = %+v (present=%v), want %q`, f, ok, taskID) + } + if _, ok := fieldValue(fields, "session_id"); !ok { + t.Fatal(`"session_id" field missing, want present (an existing field, unaffected by this criterion)`) + } + if _, ok := fieldValue(fields, "previous_sessions"); !ok { + t.Fatal(`"previous_sessions" field missing, want present (an existing field, unaffected by this criterion)`) + } + for _, key := range []string{"site", "plan_input_bytes", "plan_output_bytes", "plan_sections_omitted"} { + if _, ok := fieldValue(fields, key); ok { + t.Fatalf("%q field present on a non-reducing plan, want absent (not zero)", key) + } + } +} + +func TestMockRepositoryTaskPlanWritesRoundTrip(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + plan := &models.TaskPlan{TaskID: "task-plan-write-round-trip", Title: "Plan", Content: "initial"} + + if err := repo.CreateTaskPlan(ctx, plan); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + got, err := repo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan after create: %v", err) + } + if got == nil || got.TaskID != plan.TaskID || got.Title != plan.Title || got.Content != plan.Content { + t.Fatalf("GetTaskPlan after create = %#v, want the created plan", got) + } + + plan.Content = "updated" + if err := repo.UpdateTaskPlan(ctx, plan); err != nil { + t.Fatalf("UpdateTaskPlan: %v", err) + } + got, err = repo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan after update: %v", err) + } + if got == nil || got.TaskID != plan.TaskID || got.Title != plan.Title || got.Content != "updated" { + t.Fatalf("GetTaskPlan after update = %#v, want updated plan", got) + } + + if err := repo.DeleteTaskPlan(ctx, plan.TaskID); err != nil { + t.Fatalf("DeleteTaskPlan: %v", err) + } + got, err = repo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan after delete: %v", err) + } + if got != nil { + t.Fatalf("GetTaskPlan after delete = %#v, want nil", got) + } +} diff --git a/docs/specs/agents/README.md b/docs/specs/agents/README.md index a29344a699..7e0bd2392a 100644 --- a/docs/specs/agents/README.md +++ b/docs/specs/agents/README.md @@ -60,6 +60,7 @@ surface shared by task and Office consumers. - [Spawn Session Effective Agent Profile](requirements/spawn-session-effective-profile.md) - [Subagent context persistence](requirements/subagent-context-persistence.md) - [Profile-backed Utility Agents](requirements/utility-agent-profiles.md) +- [Plan injection budget](requirements/plan-injection-budget-01.md) ### System design @@ -83,6 +84,8 @@ surface shared by task and Office consumers. - [Subagent context persistence System Design Part 7](system-design/subagent-context-persistence-07.md) - [Subagent context persistence System Design Part 8](system-design/subagent-context-persistence-08.md) - [Profile-backed Utility Agents](system-design/utility-agent-profiles.md) +- [Plan injection budget System Design Part 1](system-design/plan-injection-budget-01.md) +- [Plan injection budget System Design Part 2](system-design/plan-injection-budget-02.md) ## Migration record diff --git a/docs/specs/agents/requirements/plan-injection-budget-01.md b/docs/specs/agents/requirements/plan-injection-budget-01.md new file mode 100644 index 0000000000..17d58e1ddf --- /dev/null +++ b/docs/specs/agents/requirements/plan-injection-budget-01.md @@ -0,0 +1,214 @@ +--- +status: draft +system: agents +created: 2026-09-01 +owners: + - nova28 +--- + +# Plan injection budget Requirements + +## Overview + +Kandev pastes a task's implementation plan into the prompt of a launching agent session +at two independent sites. Neither bounds the plan against a stated budget, and both +reduce it — when they reduce it at all — in a way that discards the newest part of the +running record without telling the agent anything was lost. This capability introduces +one shared, deterministic plan-bounding policy, applies it at both sites, and requires that any +reduction be declared in the injected text. + +The agent system owns this contract: both consumers are agent-runtime session-launch +surfaces, and [dynamic agent routing](../system-design/dynamic-agent-routing-01.md) +already requires "a bounded continuation package" without defining the bound. The task +and workflow system keeps the plan artifact, its write API, and its revision model; +this capability only reads. + +Observable behavior is defined here. The paired [system design](../system-design/plan-injection-budget-01.md) +records the components, control flow, implementation details, and rationale. Its +measurement and prior-art notes are in [system design Part 2](../system-design/plan-injection-budget-02.md). +The two requirements in this file cover the one plan-injection outcome. + +## Terminology + +- **Plan document / composed document:** the text prepared for injection by each + surface. Handover keeps the current content composition. Dynamic continuation + combines the title and content with its current whitespace handling. The budget + applies to this plan text, not to the surrounding prompt frame or prefix. +- **Section:** a run of the plan document beginning at a line starting with `## ` and + continuing to the line before the next such line, or to the end. Content before the + first such line is the **preamble section**, and ONLY when there is such content: a + document whose first line already begins with `## ` has NO preamble section and SHALL + NOT be given a zero-length one, which would inflate `{total}` by one. Every other + section begins at a `## ` line, so the preamble is the only section that could ever be + empty. A document with no `## ` line is one. +- **Budget:** the maximum size in bytes of the reducer's output, including any + omission notice, cut marker, and separator bytes. +- **Line:** a run of a section up to AND INCLUDING the next `\n`, or — for a final line + carrying no terminator — up to the section's end. Both are COMPLETE lines, and a + line's length counts its terminator when it has one. Cutting at a **line boundary** + therefore leaves text ending with `\n` or at the section's end; the latter is normal, + not a corner, the dynamic site's composed document being trimmed. +- **Omission notice:** the fixed template appended when, and only when, the reducer + dropped content, carrying two independent facts: + + ```text + [Kandev: plan reduced to fit the injection budget; {omitted} of {total} sections omitted{shortened}. Call get_task_plan_kandev for the full plan.] + ``` + + `{omitted}` and `{total}` are decimal integers. `{shortened}` is the literal + `, and the retained section was shortened` when the intra-section path of AC-001.7 + ran, and empty otherwise. +- **Inline cut marker:** the fixed literal `[Kandev: section truncated here]`, emitted + only on the intra-section path. +- **Containment:** the transformation of AC-001.14. Not part of the reducer. + +## Measurement + +The design's Appendix A records the read-only corpus measurement that informed the +budgets and section policy. + +## Requirements + +### REQ-AGENTS-PLAN-INJECTION-BUDGET-001: Bounded, deterministic plan injection + +**Intent:** Bound what a launching session pays for plan context, keep the parts that +describe both the plan's framing and its current state, and never let the agent believe +a reduced plan is the whole plan. + +**User story:** As an agent starting a new session on a task that already has a plan, I +want a bounded excerpt that tells me when it is an excerpt, so that I neither exhaust my +context on a document I can fetch nor act on a silently truncated record as complete. + +#### Acceptance criteria + +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.1:** WHEN a plan document is at most the + budget in bytes, THEN the reducer SHALL return it byte-identical and SHALL NOT append + an omission notice. Equality with the budget is within budget. Byte-identity is + relative to the document the reducer RECEIVES, which at the handover site is the + composed document after AC-001.14 containment. AC-002.3 is the complete register of + the respects in which injected bytes may differ from today. + PRECEDENCE: this is the GENERAL case and it yields to the two special ones, which are + evaluated first. AC-001.12 governs an empty or whitespace-only composed document — it + injects nothing, even though it is trivially within budget, so byte-identity does NOT + apply to it — and AC-001.13 governs a budget too small to emit anything at all. Where + either applies it wins; everywhere else this criterion holds. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.2:** WHEN a plan document exceeds the budget, + THEN the reducer's output SHALL be at most the budget in bytes, including the + omission notice, inline cut marker, and any separator bytes. This bound SHALL hold + for every input. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.3:** WHEN the reducer drops any content, THEN + the output SHALL end with the omission notice of [Terminology](#terminology). Its two + facts are reported independently. `{omitted}` SHALL be the number of whole sections + not represented in the output AT ALL and `{total}` the document's section count, a + section present but shortened by AC-001.7 NOT counting as omitted. Zero is therefore + correct ONLY when the document has one section: when AC-001.7's path runs on a + document of `{total}` sections it retains a shortened first section and drops the + rest, so `{omitted}` SHALL be `{total} - 1`. The notice is tied to content having been + dropped, never to a non-zero section count. This criterion yields to AC-001.13: when + the reducer can emit no plan content at all it SHALL return nothing, the notice + included, even though content was dropped. That is the ONLY case in which dropped + content is not followed by a notice. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.4:** WHEN the reducer drops no content, THEN + its output SHALL NOT contain an omission notice. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.5:** WHEN the reducer drops content, THEN it + SHALL retain a contiguous run of whole sections from the start of the document and a + contiguous run from the end, and SHALL drop only whole sections from the middle, + except as permitted by AC-001.7. Either run MAY be empty; retaining only a tail run is + expected when the first section does not fit. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.6:** WHEN the reducer selects which sections + to retain, THEN it SHALL retain at most one contiguous run from the start and one + contiguous run from the end of the document. The tail run SHALL be considered first, + followed by alternating consideration of the two runs. A run SHALL close when its + next section does not fit. Retained sections SHALL remain in document order, and the + same input and budget SHALL always select the same sections. + The output SHALL preserve the selected section text without rewriting it and SHALL + not add separators between retained sections or end with a trailing newline. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.7:** WHEN the selection of AC-001.6 retains + no whole section, THEN it SHALL retain leading complete lines from the first + section, add the inline cut marker, and end with the omission notice. It SHALL cut + only at line boundaries. If no complete line fits with the generated text, it SHALL + return no plan text. It SHALL NOT shorten a section when any whole section fits. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.8:** The reducer's output SHALL be valid + UTF-8 for every input that is valid UTF-8. A cut SHALL NOT split a rune. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.9:** WHEN the reducer is called twice with + the same plan document and budget, THEN it SHALL return byte-identical output, + depending only on those two inputs — not on the workflow step, session, task, + agent, clock, map iteration order, or any heading's text. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.10:** WHEN the reducer is applied to its own + output under the same budget, THEN the result SHALL be byte-identical to that + output. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.11:** WHEN the plan document contains no + `## ` line, THEN it SHALL be treated as one section; WHEN such a document also exceeds + the budget it is governed by AC-001.7, one that fits returning unchanged under + AC-001.1. WHEN + it contains content before its first `## ` line, THEN that preamble SHALL be the + first section, eligible for retention on the same terms as any other. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.12:** WHEN the plan is absent, or the site's + composed document is empty or only whitespace, THEN no plan text and no omission + notice SHALL be injected, and the launch SHALL proceed unchanged. Emptiness SHALL + be judged after the surface applies containment, where containment applies. A title + with whitespace-only content remains a title-only document on dynamic continuation; + handover treats whitespace-only content as empty. These are the two named exceptions + in AC-002.3. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.13:** WHEN the reducer cannot emit any plan + content within the budget, THEN it SHALL return no plan text at all. It SHALL NOT + return a notice or marker without plan text, and it SHALL NOT exceed the budget. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.14:** The injected plan text SHALL NOT be able + to terminate or forge the enclosing `` block. At handover, all + occurrences of the exact `` and `` literals SHALL be + removed, including literals exposed by earlier removals. Matching SHALL be + case-sensitive and exact-literal; no other text SHALL change. Containment SHALL + happen before reduction and only at handover. Dynamic continuation does not contain + these literals because it does not place plan text in that block. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.15:** The reducer SHALL hold no mutable shared + state that can change another call's output. Concurrent calls with the same input and + budget SHALL produce the same result. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-001.16:** WHEN a plan is mutated between two + session launches, THEN each launch SHALL reflect the plan snapshot used for that + launch. A later mutation SHALL NOT change an already prepared injection. + +### REQ-AGENTS-PLAN-INJECTION-BUDGET-002: Consistent plan injection across surfaces + +**Intent:** Keep the bounded-plan contract consistent wherever Kandev injects plan +context, so one surface does not silently lose the other surface's protections. + +**User story:** As an engineer changing plan context, I want both injection surfaces +to follow the same contract, so their behavior does not drift. + +#### Acceptance criteria + +- **AC-AGENTS-PLAN-INJECTION-BUDGET-002.1:** The handover and dynamic continuation + surfaces SHALL apply the same section retention, truncation, notice, and UTF-8 + rules from REQ-001. Handover SHALL also apply the containment rule in AC-001.14; + dynamic continuation SHALL not apply containment. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-002.2:** An over-budget plan at either shipped + surface SHALL produce output within that surface's budget. When plan text can fit, + the output SHALL declare omitted content with the notice in the shared contract. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-002.3:** Each surface SHALL keep its current plan + composition. Under budget, injected plan bytes SHALL remain unchanged except for + these two handover cases: exact system-tag literals are removed, and whitespace-only + content produces no plan section. Dynamic continuation SHALL still inject a title + when its content is whitespace-only. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-002.4:** The dynamic plan excerpt SHALL fit within + the continuation field's current size limit without a second truncation changing + the reducer's output. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-002.5:** The handover budget SHALL be 12,000 bytes + and the dynamic continuation budget SHALL be 4,000 bytes. Each budget SHALL apply + to the plan document only, before the surrounding prompt frame or prefix. +- **AC-AGENTS-PLAN-INJECTION-BUDGET-002.6:** WHEN plan content is reduced, the backend + SHALL write an `Info` log with `site`, `task_id`, `plan_input_bytes`, + `plan_output_bytes`, and `plan_sections_omitted`. The site value SHALL be + `handover` or `dynamic_continuation`. The log SHALL contain no plan text. WHEN no + reduction occurs, reduction fields SHALL be absent. + +## Out of scope + +- Selecting sections by relevance to the incoming workflow step. +- The plan write API, revision model, and workflow prompt. +- Containing case-variant or whitespace-variant tag forgeries. +- Bounding continuation fields other than the plan summary. +- Changing either surface's existing behavior when a plan read fails. +- Making the budgets configurable. +- Token counting or model-specific calibration. +- Defining behavior for input that is not valid UTF-8. +- Summarizing or rewriting plan content. diff --git a/docs/specs/agents/system-design/plan-injection-budget-01.md b/docs/specs/agents/system-design/plan-injection-budget-01.md new file mode 100644 index 0000000000..31fa8da300 --- /dev/null +++ b/docs/specs/agents/system-design/plan-injection-budget-01.md @@ -0,0 +1,436 @@ +--- +status: draft +system: agents +requirements: + - REQ-AGENTS-PLAN-INJECTION-BUDGET-001 + - REQ-AGENTS-PLAN-INJECTION-BUDGET-002 +--- + +# Plan injection budget System Design Part 1 + +## Purpose and boundaries + +The agent system owns the context a launching agent session receives. Two of its +surfaces inject a task's plan, and this design places a single reducer between +the plan and both of them. + +Appendices A (measurement), B (prior art) and C (per-criterion notes) are in +[Part 2](plan-injection-budget-02.md); every bare "Appendix" reference below means one +of those. + +Contracts this design uses but does not own: + +- **The plan artifact** (`task_plans`, one row per task, whole-document replace) + belongs to the task and workflow system. This design reads `Title` and + `Content` through the existing `GetTaskPlan` and writes nothing. +- **`get_task_plan_kandev`** belongs to the MCP handler surface. This design + only names it in the omission notice. +- **The `` wrapper and prompt templates** belong to + `internal/sysprompt`. This design adds no template and changes no prompt. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-AGENTS-PLAN-INJECTION-BUDGET-001` | [Components and responsibilities](#components-and-responsibilities), [Control flow](#control-flow), [The reduction algorithm](#the-reduction-algorithm) | +| `REQ-AGENTS-PLAN-INJECTION-BUDGET-002` | [Call sites](#call-sites), [Preventing drift](#preventing-drift) | + +## Components and responsibilities + +| Component | Responsibility | +| --- | --- | +| **Plan reducer** (new, backend-only) | Split a composed plan document into sections, drop from the middle until it fits, append the omission notice when it dropped anything. Pure function; no I/O, no clock, no shared state. | +| `injectHandoverIfNeeded` (`internal/orchestrator/executor/executor_execute.go`) | Compose the handover plan document from `plan.Content` VERBATIM (this site does not trim), apply containment, call the reducer with the handover budget, then wrap the reducer's output in the site's EXISTING frame — `"\nThe task has an implementation plan:\n\n%s\n"` — and pass that framed string as `planSection`, as it does today. The frame is outside the budget (AC-002.5). When the reducer returns nothing, `planSection` stays empty and no frame is emitted (AC-001.12). | +| `addDynamicPlan` (`internal/orchestrator/dynamic_launch.go`) | Compose `strings.TrimSpace(plan.Title + "\n" + plan.Content)`, call the reducer with the dynamic budget, assign to `ContinuationInput.PlanSummary`. | +| `bounded()` (`internal/agent/runtime/dynamic/conductor.go`) | Unchanged. Continues to bound every continuation field. For `PlanSummary` it is a no-op *given* two conditions the design must maintain — budget ≤ `continuationFieldLimit`, and no reducer-introduced surrounding whitespace — both asserted by the AC-002.4 test. | +| **Tag containment** (new, alongside the reducer) | Remove `` and `` literals until stable (AC-001.14). Called by the handover site only, before the reducer. | + +No frontend component implements this outcome. There is no user-visible surface; +the only observable artifacts are the agent's injected prompt and the backend +log. + +### Placement + +The reducer is a new package under `internal/agent/` — it is agent-runtime +behaviour, and both consumers already depend on that subtree. It must not live +in `internal/sysprompt` (the dynamic site does not use system prompts) nor in +`internal/agent/runtime/dynamic` (the handover site must not depend on the +dynamic routing runtime to reduce a plan). + +## Data and contracts + +The reducer takes a composed document and a budget in bytes and returns the +reduced text plus exactly two facts: whether it reduced at all, and the number of +whole sections not represented in the output — AC-001.3's count, on every path. +Both are needed and neither follows from the other: a single-section document cut +by AC-001.7 reports a count of zero while having dropped content, so the boolean, +not the count, is what AC-002.6 keys its log on. Input and output sizes are NOT +returned — a caller measures those from the string it passed and the string it got +back — and a caller must never re-derive the count by parsing the notice. It +exposes no other surface: no configuration struct, no options, no interface, no +registry. + +Two named constants carry the budgets. `continuationFieldLimit` is unexported in +`internal/agent/runtime/dynamic`, so the dynamic constant cannot be *defined* as +it from the reducer package; AC-002.4 therefore asserts the relationship from a +test inside that package, which is the only place both names are visible. + +The reducer exposes one companion function, the containment of AC-001.14. It is +separate because it is not budgeting: it is applied by the handover site alone, +before the reducer runs, and the dynamic site never calls it. + +The omission notice is a fixed English sentence carrying two independent facts — +a count of whole sections omitted out of the document's total, and a clause +present only when the retained section was shortened. It names +`get_task_plan_kandev`. It is backend prompt text for an agent, not product copy, +so it is not localized and does not enter the i18n catalogs. + +The intra-section marker of AC-001.7 is a distinct fixed string, so a reader can +tell "whole sections were dropped from the middle" from "this one section was +cut short". + +## Control flow + +Both paths are synchronous and local to an already-running launch. + +```text +launch → site reads plan (GetTaskPlan) → site composes plan document + → reducer(document, site budget) → reduced text + stats + → site wraps the reduced text in its own frame/prefix (outside the budget) + → site injects → site logs stats when anything was dropped +``` + +Handover: `Execute` → `injectHandoverIfNeeded` → reducer → +`sysprompt.InjectSessionHandover(previousCount, planSection, prompt)` → the +plan text is placed inside the `` block by the existing template. + +Dynamic: `buildDynamicContinuation` → `addDynamicPlan` → reducer → +`ContinuationInput.PlanSummary` → `BuildBoundedContinuation` → `ContinuationPrompt` +→ downstream launch. + +`bounded()` is left unmodified and becomes a no-op for this field only under two +conditions, both of which AC-002.4 makes explicit rather than assuming. Its length +check cannot fire, because the dynamic budget is at most `continuationFieldLimit`. +Its unconditional `strings.TrimSpace` cannot change the value either, because +AC-001.6's assembly rule forbids the reducer from emitting leading or trailing +whitespace that its input did not have. Without that second condition the call +would not be a no-op at all; it would silently re-trim the reducer's output. + +### Call sites + +Each site keeps its own composition step. This is deliberate and is what makes +AC-002.3 hold: because the reducer returns its input unchanged when it fits, an +under-budget plan produces byte-identical output to today at both sites, without +either site having to adopt the other's title handling. + +The handover site inserts one extra step between composing and reducing — +containment (AC-001.14) — and the dynamic site does not. That asymmetry is one +of two documented exceptions to byte-identity, and is why AC-002.3 carries an +explicit carve-out rather than an unqualified guarantee. It is placed before the +reducer so that every guarantee the reducer makes is stated over the text that is +actually injected, and so that emptiness (AC-001.12) is judged after it. + +### Preventing drift + +REQ-002 exists because the defect being fixed is duplicated, divergent handling of +one artifact. Three things keep it from returning. + +The reducer is the only place budgeting, section selection, or truncation is +implemented; neither call site carries any of the three (AC-002.1). Containment is +the one function that lives beside it rather than inside it, and it is shared code +called from one site, not inline logic at that site — so there is still exactly one +implementation of each behaviour. + +The anti-bypass test (AC-002.2) exercises each site's injection path with an +over-budget plan and asserts the notice appears in what that site injects. It is +deliberately not a test of the reducer: asserting the reducer's behaviour proves +nothing about whether a site calls it, and a site that stopped calling it would +still pass a reducer-only suite. + +The reducer holds no mutable shared state and reads no database (AC-001.15), so it +is safe under the concurrent launches the orchestrator already performs, and its +output for a given task cannot be perturbed by another task reducing at the same +moment. Each caller passes the snapshot it already read; the reducer neither +re-reads nor caches (AC-001.16). The symmetric guarantee is AC-001.4: when nothing +was dropped, no notice is emitted, so a plan that fits is never decorated with a +claim that something is missing. + +### The reduction algorithm + +Given a document and a budget: + +The implementation reserves space for generated text before it selects source +sections. The notice reservation uses the widest notice for the document's section +count and one possible preceding newline. The marker reservation uses the marker +length and one possible preceding newline. These are upper bounds, so the final +assembly remains within the budget even when a conditional newline is not needed. + +1. If the document is empty or only whitespace, return nothing at all — no text, + no notice, no stats (AC-001.12). +2. If `len(document) <= budget`, return it unchanged with a zero drop count + (AC-001.1). This is the only path most callers take for a small plan, and it + allocates nothing. +3. Split into sections at lines beginning `## `, keeping any preamble as the + first section (AC-001.11). Section order is fixed here and never changed + (AC-001.6). +4. Compute the notice reservation once, now that the section count is known: the + notice rendered at its widest for this document — both numbers set to the total + section count, the `shortened` clause present — PLUS ONE BYTE for the conditional + `\n` step 6 may place before it. Both parts are upper bounds, which is what makes + AC-001.2 hold for every input without a second pass: the notice finally rendered can + only be shorter and the separator can only be absent, so the finished output can only + come in under the bound, never over it. Two ways to get this wrong, both producing an + over-budget output the algorithm cannot recover from, there being no eviction or + restart step: reserving a fixed-width count and then formatting a wider one, and + reserving the notice WITHOUT its separator byte. Appendix C works both, and Testing + notes says which fixture catches which. +5. Grow a tail run and a head run, considering candidates alternately with the + tail run first. Retain a candidate that fits the budget remaining after the + reservation and the sections already retained; when one does not fit, close + that run permanently rather than skipping past it, so both runs stay contiguous + (AC-001.5, AC-001.6). Stop when both runs are closed or the runs meet — which + AC-001.6 defines as no unconsidered section remaining between them, and which is the + ONLY thing that word means here. Closing rather than skipping is deliberate: skipping + a section that does not fit and taking a smaller one behind it would use the budget + slightly better and produce a patchwork whose holes the reader cannot locate. + One trace exposes a sloppy loop and must hold. TWO sections whose last does not fit: + the tail closes, and the head, still open, considers and retains S1. An implementation + that stops because ONE run closed retains nothing, falls through to step 7, and + shortens a section that never needed shortening. + There is deliberately NO trace in which every section is retained, and none should be + added. Step 5 runs only when the document exceeds the budget, so retaining every + section would require `len(document) <= budget - reservation`; the output would then be + the whole over-budget document carrying no notice, violating AC-001.2 and AC-001.4. The + same arithmetic covers the last section still unconsidered when the runs meet — it is + retainable only if every section is, so it never is. AC-001.6 still REQUIRES that it be + considered, consideration being what makes the traversal fully determined and keeps it + correct under a future budget or reservation change, but no trace can show a conforming + loop retaining it. +6. If step 5 retained at least one whole section, emit them in original document order + and append the notice, reporting the number of whole sections omitted out of the + total (AC-001.3), then return. Assembly is exact (AC-001.6): sections are + concatenated with no inserted separator: every section EXCEPT the document's last + ends at a `## ` line and so already carries its trailing newline. The last section + is the one that may not — the dynamic site's `TrimSpace` guarantees it does not, the + handover site passes content verbatim — but it cannot strand a join, because it can + only land at the END of the output, which the next rule covers. Tail-first retains + it FIRST on every reduction, so that is the common case, not an edge one. The notice + is preceded by a single `\n` only when the text so far does not already end in one: + the byte step 4 reserved. The output carries no trailing newline; that rule is not + cosmetic, it is the condition that keeps `bounded()` a no-op. +7. If step 5 retained nothing, fall back to the single intra-section path: take leading + whole lines of the **first** section while they fit the budget less BOTH the notice + reservation AND the marker reservation — each carrying its own conditional separator + byte — then append the marker, then the notice, and return (AC-001.7). A line is what + Terminology says it is, its terminator included. The retained prefix is always + strictly shorter than the section: step 7 runs only because step 5 could not retain + that section whole against a LARGER remaining budget, so its final line is never + among the lines taken and the retained text always ends with `\n`. The separator + before the marker is therefore never spent on this path; the one before the notice + always is, the marker not ending in a newline. Both are reserved regardless — see + Appendix C for why an upper bound is what AC-001.2 needs. + The notice reports both facts independently. `{omitted}` is the number of whole + sections NOT REPRESENTED AT ALL, which on this path is `{total} - 1`: the first + section is present but shortened, and every other section was dropped whole. It is + zero ONLY when the document had one section to begin with. Reporting a blanket zero + here would tell the agent nothing was omitted while `{total} - 1` sections are gone, + which is the failure REQ-001's Intent exists to prevent. The `shortened` clause + carries the separate fact that the retained section was cut. +8. If not even one complete line fits after those two reservations — separators + included — return no plan text at all (AC-001.13): never a notice with nothing + attached, never a marker with nothing attached, never an over-budget output. With the + shipped constants this band is unreachable; it is defined so the function is total. + +Determinism (AC-001.9) follows from the algorithm reading only the document and +the budget: no map iteration, no randomness, no clock, no heading text +interpretation. Headings are located, never read. + +Idempotency (AC-001.10) needs care and is the one place a naive implementation +will fail the criteria. Feeding the reducer's output back in presents a document +whose last section is the notice. The implementation must return such an input +unchanged when it fits, which step 2 already does, since the output is by +construction within budget. The test for AC-001.10 must therefore use an input +that actually reduced, not merely a short one. + +Rune safety (AC-001.8) follows from cutting only at line boundaries and section +boundaries, both of which fall on rune boundaries in valid UTF-8. No code path +slices a string at an arbitrary byte offset. This is the specific defect in +today's `bounded()`, which does `value[:continuationFieldLimit]`. + +### Why the fallback reduces the first section + +When nothing whole fits, the opening is the only position guaranteed to carry the +document's framing; leading lines of a later section would arrive detached from +their heading. The trigger is *nothing whole fits*, never *some section is +oversized*; Testing notes works that trap and the fixture that catches it. + +### Why head and tail rather than head alone + +A plan is appended to over a task's life, so its tail holds current state — the +latest verdict, open findings, the PR number — its head holds framing, and its +middle is where superseded working notes accumulate. Keeping both ends costs the +same budget as keeping one and loses the least useful part. Today's bound keeps +the head alone (Appendix A). The tail-first tie-break in AC-001.6 follows: when +only one more section fits, current state is worth more than more framing. + +## Failure and recovery + +- **Plan read fails.** Each site keeps its existing policy unchanged (an + explicit exclusion in the requirement). The reducer is not reached. +- **Plan missing, empty, or whitespace.** The reducer returns nothing when the + document it receives is empty or whitespace-only, and the launch proceeds + (AC-001.12). Because each site composes its own document, one input splits them: + a plan with a set title and whitespace content composes to a non-empty document + at the dynamic site, which still injects the bare title as today, and to a + whitespace-only one at the handover site, which injects no plan section. The + latter is a deliberate change — today's `plan.Content != ""` guard passes on + whitespace and frames it — and is the second of AC-002.3's named exceptions. +- **Budget too small for the notice, or non-positive.** Return no plan text + rather than an over-budget output or a bare notice (AC-001.13). This cannot + arise from the shipped constants; it is defined so the function is total. +- **Malformed markdown.** There is no failure mode. Unbalanced fences, headings + inside code fences, and `##` at a line start within a fenced block are all + treated as section boundaries. This is a deliberate simplification: a + false-positive boundary can only change where a cut lands, never whether the + output is valid or within budget. Tracking fence state would add a parser to + buy nothing the criteria require. +- **The reducer cannot fail.** It returns no error. A launch is never blocked, + degraded, or retried because of plan reduction. + +## Persistence + +None. This design stores nothing, caches nothing (AC-001.16), and runs no +migration. It reads `task_plans` through the existing `GetTaskPlan` and holds +the result only for the duration of one launch. + +## Security + +Plan content is authored by agents and is untrusted as prompt input. It is +injected inside the `` block on the handover path, so text that +closes or forges that block would let plan content escape the system context and +present itself as trusted framing. + +`sysprompt.StripTags` removes only ``; one stored plan already +contains a `` start tag and none contains an end tag. AC-001.14 +requires containment of both directions, and requires ONE combined replace-until-stable +loop rather than two independent per-literal ones. A single pass is not enough: one pass +over `-system>` reconstitutes a live start tag. Two per-literal +loops of the shape `StripTags` uses are not enough either, and that is the sharper trap — +`StripTags` loops over one literal because it has only one to remove, whereas containment +has two that can build each other. Given `` + `-system>`, a +start-only pass finds no start literal and changes nothing, and the end-only pass removes +the end literal and leaves a live start tag its own stop condition never rechecks. +Reversing the two loops does not help — it only moves the hole, as the mirror input +`` + `-system>` shows, leaving a live end tag by the same +argument. Every pass must therefore rescan for BOTH literals, stopping only when neither +is found. + +Containment is **not** a no-op on the measured corpus: it changes the injected +bytes of exactly the one plan that carries the start tag. That is a deliberate +carve-out, decided rather than assumed away. AC-002.3 records it as one of two +exceptions to byte-identity under budget, and AC-001.1 states byte-identity +relative to the document the reducer receives, which on the handover path is the +contained document. + +The carve-out is scoped to the handover site only. The dynamic site does not wrap +plan text in a `` block — nothing in `internal/agent/runtime/dynamic` +references the tag — so containing there would change injected bytes for no +security gain. Matching is case-sensitive and exact-literal, so `` +and `< kandev-system >` are not removed; that limit is stated in the requirement's +Out of scope (Part 2), because broadening the match would rewrite legitimate plan prose +that discusses the tag. + +The omission notice is generated, never derived from plan content, so a plan +cannot forge or suppress it. Sizes and counts are the only plan-derived values +that reach the log; no plan text is logged. + +## Observability + +AC-002.6 requires a log on reduction, carrying the site, task, input size, output +size, and sections omitted. The two sites are not symmetric here, and AC-002.6 is +therefore written per site rather than as one sentence about existing behaviour. +The handover site already emits an `Info` record at injection ("injecting session +handover context") and extends it with these fields. The dynamic site emits no log +at injection at all — `addDynamicPlan` contains no logging — so it gains a new +`Info` record. Treating both as "already logging" would have left the dynamic +site's reductions invisible. + +No expvar counter is added. The existing `routing_*` and `workflow_*` families +answer different questions, and there is no requester for a metric here; the log +is sufficient to confirm the bound is engaging and to see how often plans exceed +it. + +## Testing notes + +The criteria that will be got wrong without deliberate tests: + +- **AC-001.1 / AC-002.3** — byte-identical output under budget, asserted per + site with each site's own composition, not just on the reducer. +- **AC-001.10** — idempotency asserted on a document that actually reduced. +- **AC-002.4** — the dynamic budget's relationship to `continuationFieldLimit` + asserted directly, so raising one without the other fails the build. +- **AC-002.2** — the anti-bypass test. Asserting the reducer's own behaviour + does not show that a site calls it; the test must exercise each site's + injection path with an over-budget plan and assert the notice appears. +- **AC-001.14's containment loop** — THREE fixtures, and all three are needed. The + single-literal `-system>` catches an implementation that does not + repeat at all. The other two catch the likelier defect, two independent per-literal + loops, and ONE OF THEM IS NOT ENOUGH because each ordering fails on a different input: + `` + `-system>` survives a start-then-end pair as a live + `` but is cleared by end-then-start, and the mirror + `` + `-system>` survives end-then-start as a live + `` but is cleared by start-then-end. A suite carrying only one of the + pair therefore passes a broken implementation half the time, depending on which order + the builder happened to write. The combined loop returns the empty string for all + three; assert that, not merely that no literal remains. +- **AC-001.13** — a budget smaller than the notice, which the shipped constants + never produce and a hand-written test must construct. +- **AC-001.2 needs BOTH a narrow and a wide section count**, because the two ways + to exceed the budget are caught at opposite ends of the range and a suite that + tests only one ships the other green. Assert `len(output) <= budget` on both. + - *Wide count.* The notice's length varies with the number it carries, so an + implementation that reserves room for a fixed-width count and then formats a + wider one goes over. A document with enough sections to force a three-digit + count catches this; a two-section document does not. + - *Narrow count, on the intra-section path.* A reservation that omits the + separator byte of AC-001.6 goes over by exactly one byte, but only when the + reserved notice is no wider than the rendered one. On this path the reserved + notice carries `{total}` where the rendered one carries `{total} - 1`, so the + slack is one byte when the section count is a power of ten and zero for every + other count. The defect is therefore invisible at N=10 and N=100 and visible at + N=2, N=3, N=11, N=101. Do not leave this to the wide fixture above: N=100 is the + roundest way to force three digits and is exactly a count that hides it. Require + a small-N case explicitly. Appendix C works the arithmetic. The fixture needs no + special construction beyond a small section count and nothing whole fitting: the + separator this catches is the one before the NOTICE, and step 7 always spends it, + because the marker does not end in a newline. Do not try to build a case whose + retained text ends without a `\n` — step 7 shows that cannot happen on this path, + and an implementation that produced it would be cutting mid-line. +- **AC-001.7** — the trigger is that *nothing whole fits*, not that some section + is oversized, and the two must be tested apart. The positive case is a document + where no whole section fits, so the intra-section path runs. The negative case is + the trap: a document whose first section is far larger than the budget but whose + last section is small must retain that last section under step 5 and must **not** + reach the fallback. A suite that only asserts "oversized section ⇒ marker appears" + passes against an implementation that wrongly reduces the first section whenever + it is oversized, which would discard current state on 71% of stored plans. +- **AC-001.13** on the intra-section path — a budget that holds the notice but not + the notice and marker together, and one that holds both but not one complete + line. Both must return nothing rather than a bare notice or marker. +- **AC-001.3's omitted count on the intra-section path.** Nothing else in this + list reaches it: every other notice assertion runs on the whole-section path, + where the count is the number of dropped sections and the obvious + implementation is right. On the fallback the obvious implementation — report + zero, because no section was dropped *whole* — is wrong. Give the test a + multi-section document where no whole section fits, and assert the notice + carries `{total} - 1`, not zero. A single-section document is the degenerate + case where zero IS correct, so it cannot stand in for this. + +Real plan shapes are available from the measurement in the requirement; fixtures +should include a plan with no `## ` line at all, one with a preamble, and one +whose single largest section exceeds the dynamic budget. + +## Related decisions + +No ADR is required. This design adds one internal pure function and changes no +architecture, public contract, persistence boundary, or security boundary. diff --git a/docs/specs/agents/system-design/plan-injection-budget-02.md b/docs/specs/agents/system-design/plan-injection-budget-02.md new file mode 100644 index 0000000000..759e5ce449 --- /dev/null +++ b/docs/specs/agents/system-design/plan-injection-budget-02.md @@ -0,0 +1,150 @@ +--- +status: draft +system: agents +requirements: + - REQ-AGENTS-PLAN-INJECTION-BUDGET-001 + - REQ-AGENTS-PLAN-INJECTION-BUDGET-002 +--- + +# Plan injection budget System Design Part 2 + +Appendices to [Part 1](plan-injection-budget-01.md), which carries the design itself. +Part 1 is normative-adjacent prose about how the reducer works; this part is the +evidence behind it — the measurement the criteria cite, the prior art the shape was +taken from, and the per-criterion arithmetic that would otherwise crowd the +requirement. + +## Appendix A: measurement + +From the live Kandev SQLite database at `~/.kandev/data/kandev.db`, read-only, +2026-09-01. The requirement carries the rows the criteria cite; this is the full +table and what each row decides. + +| Quantity | Value | What it decides | +| --- | --- | --- | +| Plans stored (`task_plans`, `UNIQUE(task_id)`) | 178 | Corpus size; one plan per task | +| Plan content: min / median / max characters | 1,209 / 17,448 / 122,215 | The handover budget's ceiling | +| Plans exceeding 4,000 bytes composed | 160 of 178 (90%) | The dynamic bound engages on almost every plan | +| Plans with one section larger than 4,000 characters | 127 of 178 (71%) | Whole-section granularity cannot always fit, so AC-001.7 must exist | +| Share of plan surviving today's 4,000-byte cut | median 18.2%, min 3.3% | How much is lost today, and from which end | +| Plans containing multibyte runes, among those cut | 160 of 160 (100%) | Byte slicing is not rune-safe, so AC-001.8 is not theoretical | +| `## ` heading occurrences across the corpus | 2,348 | Heading text is uncontrolled, so it is never interpreted | +| Distinct workflow step names | 39 | Step names are installation-defined, so they cannot key selection | +| Plans containing a `` literal | 1 | Containment is not a no-op; hence the AC-002.3 carve-out | + +Three consequences drive the criteria. Today's dynamic bound keeps a median 18.2% +of the plan and all of it from the head, while a plan's newest material is at its +tail — hence head *and* tail. 71% of plans hold a section larger than the dynamic +budget, so a whole-section-only reducer would frequently return nothing — hence the +intra-section fallback. And every cut plan contains multibyte runes, so a byte +slice would corrupt output on real data, not only in principle. + +The handover site applies no bound at all, re-injecting on the order of 16M +characters (roughly 4M tokens) across 421 eligible launches. + +The heading row counts occurrences, not distinct strings; a re-measurement on +2026-09-02 (181 plans, the corpus having grown by three) gives 2,320 occurrences +against 1,846 distinct, so headings do repeat across plans. The conclusion is +unchanged either way — roughly ten distinct headings per plan is uncontrolled +vocabulary, and AC-001.9 locates headings as cut boundaries without reading them. +Every other row in this table is the 2026-09-01 snapshot the criteria cite and is +not re-baselined here. + +## Appendix B: prior art, in full + +**Receipt (our own prior reasoning).** The QMD `wiki` collection (443 documents), +searched semantically — the vector index was present and current, so this was not +the silent keyword fallback. Two queries: context-injection budgeting and omission +signalling; preloading versus fetching on demand. + +The governing hit is `concepts/artifact-write-api.md` (`lifecycle: draft`, +2026-09-01), which measures the same production database and reaches the same +live-content total. Two of its positions bind this design. **"Control flow parsing +the artifact" is an anti-pattern** — "a text-derived state machine does not merely +cost tokens; it returns wrong answers silently" — which is why headings here are +located as cut boundaries and never read. **"Heuristic truncation guard as a +stand-in" is an anti-pattern**, because it "cannot tell deliberately smaller from +read stale"; this design does not guess whether a plan is complete, it states +in-band when it dropped something. + +**Receipt (what others shipped).** `saas-kb` `search_fsm_docs`, `category: +"ai_sdlc"`, two queries covering session-handover injection budgets and +context-window compaction. Best relevance across both was 0.016 — the flat, low +distribution that corpus produces when nothing matches, so this is a genuine +negative result rather than a failed search. No platform in that slice documents +bounding a **durable artifact** re-injected into a new session; they document +*conversation* compaction, which is a different problem and is not adopted: a +conversation is consumed once and may be summarised lossily, whereas a plan is a +durable record the agent can re-fetch in full. + +One primitive transfers. OpenHands' `maybe_truncate()` "keeps the head and tail of +the content to preserve context at both ends" and inserts a caller-supplied +`truncate_notice` at the cut. That shape is adopted — head and tail, plus an +in-band notice — minus its saved-file reference, since `get_task_plan_kandev` +already returns the full plan and no file needs writing. + +## Appendix C: notes on individual criteria + +Rationale that would otherwise crowd the requirement: + +- **AC-001.12, on reachability.** The whitespace-content row is reachable, not + hypothetical: `content` defaults to the empty string and no constraint prevents + it. Judging emptiness on the `content` column rather than on the composed + document would require a call site to carry its own plan logic, which AC-002.1 + forbids; the site asymmetry it produces instead is stated under Failure and + recovery. +- **AC-001.13, on the shipped constants.** The four bands the criterion covers — a + non-positive budget, one below the notice reservation, one holding the notice but + not notice-plus-marker, and one holding both but not a single line — are all + unreachable at 12,000 and 4,000 bytes. The criterion exists so the function is + total and so a future budget change cannot produce an undefined case. +- **AC-002.5, on the handover value.** 12,000 bytes caps that site's worst observed + injection of 122,215 characters at roughly a tenth, while leaving 22% of stored + plans untouched. The dynamic value is unchanged at 4,000, so this capability + loosens no bound that exists today. +- **AC-002.5, on what the budget measures.** Each budget bounds the reducer's + output — the plan document alone — not the text a site wraps around it. The + handover frame (`"\nThe task has an implementation plan:\n\n%s\n"`) and the + `` wrapper lie outside the 12,000; the dynamic `"Plan summary: "` + prefix lies outside the 4,000. Tests must assert against the reducer's output, + not the wrapped prompt, or they will measure the frame and fail for the wrong + reason. +- **AC-001.2, the reservation arithmetic, worked.** With the pinned literals the + notice is 161 bytes when both counts are single-digit and the `{shortened}` + clause is present, and the marker is 32 bytes. Take the dynamic budget of 4,000 + and a three-section document on which nothing whole fits, so the intra-section + path runs. + + Reserving the notice and marker *without* their separators leaves + `4000 - 161 - 32 = 3807` bytes for lines. Fill them and assemble. The retained + text ends at a line boundary strictly inside the section — step 7 is only reached + because the section does not fit whole, so the last retained line carries its + terminator — and therefore ends in `\n`, so no separator precedes the marker. The + marker does not end in `\n`, so one precedes the notice. The output is + `3807 + 32 + 1 + 161 = 4001`: one byte over, on a path the criterion says can + never exceed the budget. + + Reserving with the separators leaves `4000 - 162 - 33 = 3805`, and the same + assembly yields `3805 + 32 + 1 + 161 = 3999`. Under the bound. + + The marker's reserved byte is not spent here, and on the only path that emits a + marker it never is. It is reserved because AC-001.6 states both separators + conditionally, and a reservation that is an upper bound can leave the output + under budget but can never let it past — which is the property AC-001.2 needs and + the reason neither reservation is computed from what assembly will actually + do. + + What makes this survive a careless test is that the over-budget form has no + slack to absorb it. The reserved notice sets both counts to `{total}`, the + rendered one carries `{total} - 1`, and those two decimal renderings are the same + width unless `{total}` is a power of ten — so the reservation is exact, not + generous, for almost every document. The `{shortened}` clause gives no slack + either: it is reserved as present and, on this path, is present. On the + whole-section path the same clause is reserved but never rendered, which is why + that path is 39 bytes clear and does not expose the defect at all. +- **AC-002.4, on test placement.** The assertion lives in package + `internal/agent/runtime/dynamic` because `continuationFieldLimit` is unexported + and only visible there. It imports the reducer package for the exported budget, + which is cycle-free: `internal/agent/**` does not import `internal/orchestrator`. + Neither exporting the constant nor modifying `bounded()` is required, so that + package's production code is untouched.