From 8aa6ed6d57a974f907ea76785478d33a7f26cbb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maple=EF=BC=81?= Date: Wed, 2 Sep 2026 17:02:10 +0800 Subject: [PATCH] refactor: apply review findings from the ADR-007/008 PRs Give the compacted-harness-injection group a single owner, UserMessage.IsCompactedHarnessInjection(), and make stats call it instead of repeating the flag list the renderer already enumerates. Cover ComputeStats turn counting with tests that fail when the increment is removed. Rename parseLineWithToolNames to parseLineWithToolCalls to match what it threads. Scan excerpt lines without splitting the whole text and strip ANSI only when an escape byte is present. Drop the unused FirstLine helper and a regexp that matched a plain literal; pin CommandMarker precedence in the turn policy and mixed teammate tag variants in one body. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TcvWErnKnuADJsomnZfwFr --- internal/analyzer/stats.go | 7 +-- internal/analyzer/stats_test.go | 78 +++++++++++++++++++++++++ internal/claudecodec/reader.go | 10 ++-- internal/claudecodec/reader_test.go | 2 +- internal/session/event.go | 62 ++++++++++++++------ internal/session/event_test.go | 90 ++++++++--------------------- internal/session/turn_test.go | 4 ++ 7 files changed, 155 insertions(+), 98 deletions(-) diff --git a/internal/analyzer/stats.go b/internal/analyzer/stats.go index e1187f4..31c2f2e 100644 --- a/internal/analyzer/stats.go +++ b/internal/analyzer/stats.go @@ -114,12 +114,7 @@ func ComputeStats(events []session.Event) StatsResult { // Harness injections that are compacted rather than dropped; // their KEPT (compacted) size is measured below via the render // pass. Only the raw side is recorded here. - if event.User.IsSkillInjection || event.User.IsTeammateMessage || - event.User.IsCommandInjection || event.User.IsTaskNotification || - event.User.IsCompactionSummary || event.User.IsStopHookGoal || - event.User.IsAgentsStopped || event.User.IsInterrupted || - event.User.IsCoordinatorMessage || event.User.IsContinuePrompt || - event.User.IsForkBoilerplate || event.User.IsNoVisibleOutputNudge { + if event.User.IsCompactedHarnessInjection() { rawParts = append(rawParts, event.User.Text) continue } diff --git a/internal/analyzer/stats_test.go b/internal/analyzer/stats_test.go index e985328..6c0f080 100644 --- a/internal/analyzer/stats_test.go +++ b/internal/analyzer/stats_test.go @@ -277,6 +277,84 @@ func buildNestedCCSessionEvents() []session.Event { return events } +// TestComputeStats_UserTurnCount verifies UserTurnCount tallies exactly the +// message kinds session.UserMessage.CountsAsTurn() says start a unit of agent +// work (ADR-008 decision 5), parameterized over the kinds that count and the +// kinds that don't so a mutation deleting the userTurnCount++ call (which the +// pre-existing suite never asserted on) fails every case at once. +func TestComputeStats_UserTurnCount(t *testing.T) { + tests := map[string]struct { + user session.UserMessage + want int + }{ + "a plain typed message counts": { + user: session.UserMessage{Text: "把這個修好"}, + want: 1, + }, + "a teammate message counts": { + user: session.UserMessage{Text: "…", IsTeammateMessage: true}, + want: 1, + }, + "a task notification counts": { + user: session.UserMessage{Text: "…", IsTaskNotification: true}, + want: 1, + }, + "a compaction summary counts": { + user: session.UserMessage{Text: "…", IsCompactionSummary: true}, + want: 1, + }, + "an interruption sentinel does not count": { + user: session.UserMessage{Text: "…", IsInterrupted: true}, + want: 0, + }, + "a stop hook goal notice does not count": { + user: session.UserMessage{Text: "…", IsStopHookGoal: true}, + want: 0, + }, + "a skill injection does not count": { + user: session.UserMessage{Text: "…", IsSkillInjection: true}, + want: 0, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + events := []session.Event{{Kind: session.EventUserMessage, User: &tc.user}} + result := ComputeStats(events) + if got := result.UserTurnCount; got != tc.want { + t.Fatalf("UserTurnCount = %d, want %d", got, tc.want) + } + }) + } +} + +// TestComputeStats_HarnessFlaggedMessage_ContributesToRawSizeNotRawUserText +// guards the accounting split ADR-008 depends on: a harness injection that is +// compacted (not dropped) contributes its full raw text to RawChars (every +// byte the transcript contains), but its KEPT category — measured below via +// the render pass, per stats.go's own comment — is the compact form's size, +// never a second, drift-prone tally of the raw text itself. +func TestComputeStats_HarnessFlaggedMessage_ContributesToRawSizeNotRawUserText(t *testing.T) { + const rawText = "The coordinator sent a message while you were working:\nplease also update the README" + events := []session.Event{ + { + Kind: session.EventUserMessage, + User: &session.UserMessage{Text: rawText, IsCoordinatorMessage: true}, + }, + } + + result := ComputeStats(events) + + assertContains(t, "RawText", result.RawText, rawText) + // The compact render form ("[coordinator]\n") never quotes the + // harness opening line the raw text carries, so its absence from + // FilteredText proves user_text was populated from the render pass's + // compact output, not from a raw-text tally. + assertNotContains(t, "FilteredText", result.FilteredText, "The coordinator sent a message while you were working") + wantUserTextChars := len([]rune(session.CompactCoordinatorMessage(rawText))) + assertCategory(t, result, "user_text", wantUserTextChars) +} + func assertCategory(t *testing.T, result StatsResult, key string, want int) { t.Helper() if got := result.Categories[key]; got != want { diff --git a/internal/claudecodec/reader.go b/internal/claudecodec/reader.go index ea1a759..ccbfa73 100644 --- a/internal/claudecodec/reader.go +++ b/internal/claudecodec/reader.go @@ -61,7 +61,7 @@ func ReadFile(path string, handle func(session.Event) error) error { defer f.Close() // toolCalls accumulates tool_use_id -> tool call info across the - // sequential read, scoped to this file. See parseLineWithToolNames for why + // sequential read, scoped to this file. See parseLineWithToolCalls for why // this state can't live inside the stateless public ParseLine. toolCalls := map[string]toolCallInfo{} reader := bufio.NewReader(f) @@ -79,7 +79,7 @@ func ReadFile(path string, handle func(session.Event) error) error { } continue } - event, ok, parseErr := parseLineWithToolNames(line, toolCalls) + event, ok, parseErr := parseLineWithToolCalls(line, toolCalls) if parseErr != nil { return parseErr } @@ -105,7 +105,7 @@ func ReadAll(path string) ([]session.Event, error) { } func ParseLine(line []byte) (session.Event, bool, error) { - return parseLineWithToolNames(line, nil) + return parseLineWithToolCalls(line, nil) } // toolCallInfo is the per-tool_use state ReadFile threads across the @@ -119,7 +119,7 @@ type toolCallInfo struct { SkillArgs string } -// parseLineWithToolNames is ParseLine's implementation, extended with the +// parseLineWithToolCalls is ParseLine's implementation, extended with the // tool_use_id -> tool call info state ReadFile accumulates across a // sequential read. Real transcripts carry no commandName/agentType field on // Bash/Edit/Write/Read toolUseResults — the tool name only exists on the @@ -130,7 +130,7 @@ type toolCallInfo struct { // ParseLine keeps its public, stateless single-line contract by passing // toolCalls=nil, under which both fall back to their text-based paths only, // same as before this fix. -func parseLineWithToolNames(line []byte, toolCalls map[string]toolCallInfo) (session.Event, bool, error) { +func parseLineWithToolCalls(line []byte, toolCalls map[string]toolCallInfo) (session.Event, bool, error) { var raw rawEntry if err := json.Unmarshal(line, &raw); err != nil { return session.Event{}, false, fmt.Errorf("parse transcript line: %w", err) diff --git a/internal/claudecodec/reader_test.go b/internal/claudecodec/reader_test.go index 97f0bc5..2f9b99d 100644 --- a/internal/claudecodec/reader_test.go +++ b/internal/claudecodec/reader_test.go @@ -405,7 +405,7 @@ func TestParseLine_UnknownEntryWithoutMessageIsSkipped(t *testing.T) { // --- Malformed JSONL robustness --- // // These tests pin the reader's actual contract for malformed input, per the -// project's own decision (see reader.go's parseLineWithToolNames): a line +// project's own decision (see reader.go's parseLineWithToolCalls): a line // that fails json.Unmarshal returns an explicit "parse transcript line" // error and aborts the read, rather than panicking or silently dropping the // line to produce a truncated-but-successful result. Read/ReadAll callers diff --git a/internal/session/event.go b/internal/session/event.go index b9d107f..13f9850 100644 --- a/internal/session/event.go +++ b/internal/session/event.go @@ -178,6 +178,26 @@ type UserMessage struct { // "arrives ... within the running turn," so it does not start a new one — // same reasoning as the injections that arrive alongside the turn that // triggered them. +// IsCompactedHarnessInjection reports whether this message is a harness +// injection that is rendered in compact form rather than dropped or shown +// under the user role. This is the single enumeration of that set: stats.go +// (raw-side accounting) and render.go's per-flag dispatch (each flag needs +// its own compact form, so the dispatch itself is not collapsed into this +// method) both derive from it, keeping the set from drifting between the two +// call sites the way it did before ADR-008. +// +// IsSystemReminder/IsContextUsage are dropped outright, not compacted, and +// IsMidTurnUserMessage is human-typed and rendered under the user role, so +// none of the three belongs in this set. +func (u UserMessage) IsCompactedHarnessInjection() bool { + return u.IsSkillInjection || u.IsTeammateMessage || + u.IsCommandInjection || u.IsTaskNotification || + u.IsCompactionSummary || u.IsStopHookGoal || + u.IsAgentsStopped || u.IsInterrupted || + u.IsCoordinatorMessage || u.IsContinuePrompt || + u.IsForkBoilerplate || u.IsNoVisibleOutputNudge +} + func (u UserMessage) CountsAsTurn() bool { if u.CommandMarker != "" { return false @@ -394,17 +414,15 @@ var catLineNumberPrefix = regexp.MustCompile(`^\s*\d+\t`) // when picking an excerpt in favor of the actual error line beneath it. var bareExitCodeLine = regexp.MustCompile(`^Exit code \d+\s*$`) -// hookErrorBoilerplate matches PreToolUse/PostToolUse hook rejection preamble -// lines ("... hook error"), which name the hook stage rather than the actual -// failure — the reader wants what's beneath it, not the wrapper. -var hookErrorBoilerplate = regexp.MustCompile(`hook error`) - // isNoiseExcerptLine reports whether line is one of the known-noise shapes // that should be skipped when picking a failure excerpt (ADR-003 decision 2). +// "hook error" matches PreToolUse/PostToolUse hook rejection preamble lines, +// which name the hook stage rather than the actual failure — the reader wants +// what's beneath it, not the wrapper. func isNoiseExcerptLine(line string) bool { return catLineNumberPrefix.MatchString(line) || bareExitCodeLine.MatchString(line) || - hookErrorBoilerplate.MatchString(line) + strings.Contains(line, "hook error") } // progressLine matches a line where a tool announces what it is about to do @@ -466,19 +484,30 @@ func firstMeaningfulSuccessLine(text string, maxRunes int) string { // nothing about, with terminal escape sequences removed. Falling back to the // first non-empty line keeps an excerpt when every line looks like noise, // rather than reporting a bare status for a call that did produce output. +// Lines are visited without materializing the full strings.Split slice, and +// StripANSI only runs on a line that actually contains an escape byte — +// tool output is rarely ANSI-colored, so most lines skip the regexp entirely. func firstMeaningfulLine(text string, maxRunes int, isNoise func(string) bool) string { var firstNonEmpty string - for _, raw := range strings.Split(text, "\n") { - line := strings.TrimSpace(StripANSI(raw)) - if line == "" { - continue + remaining := text + for { + raw, rest, hasMore := strings.Cut(remaining, "\n") + if strings.IndexByte(raw, '\x1b') >= 0 { + raw = StripANSI(raw) } - if firstNonEmpty == "" { - firstNonEmpty = line + line := strings.TrimSpace(raw) + if line != "" { + if firstNonEmpty == "" { + firstNonEmpty = line + } + if !isNoise(line) { + return Truncate(line, maxRunes) + } } - if !isNoise(line) { - return Truncate(line, maxRunes) + if !hasMore { + break } + remaining = rest } return Truncate(firstNonEmpty, maxRunes) } @@ -487,11 +516,6 @@ type NoiseEvent struct { Text string } -func FirstLine(s string, maxRunes int) string { - line := strings.SplitN(strings.TrimSpace(s), "\n", 2)[0] - return Truncate(line, maxRunes) -} - func Truncate(s string, maxRunes int) string { // Byte length >= rune count, so a string within maxRunes bytes is // guaranteed within maxRunes runes — a fast early return that avoids diff --git a/internal/session/event_test.go b/internal/session/event_test.go index c8c1571..3526415 100644 --- a/internal/session/event_test.go +++ b/internal/session/event_test.go @@ -180,73 +180,6 @@ func TestTruncate_GivenZeroRuneLimit_ThenReturnsEmptyString(t *testing.T) { } } -func TestFirstLine(t *testing.T) { - tests := []struct { - name string - s string - maxRunes int - want string - }{ - { - // Multi-line input: only the first line survives, the rest is dropped. - name: "given multiline then keeps only first line", - s: "first line\nsecond line\nthird", - maxRunes: 80, - want: "first line", - }, - { - // First line itself exceeds the budget: it is truncated to maxRunes. - name: "given long first line then truncates first line to budget", - s: "abcdefghij\nsecond", - maxRunes: 4, - want: "abcd", - }, - { - // Leading/trailing whitespace is trimmed before the first line is taken. - name: "given surrounding whitespace then trims before splitting", - s: " \n hello\nworld ", - maxRunes: 80, - want: "hello", - }, - { - name: "given empty string then returns empty", - s: "", - maxRunes: 80, - want: "", - }, - { - name: "given all whitespace then returns empty", - s: " \n\t \n ", - maxRunes: 80, - want: "", - }, - { - // CJK first line cut mid-string: must land on a rune boundary via - // the shared Truncate helper, never a half-character. - name: "given CJK first line over budget then cuts on rune boundary", - s: "甲乙丙丁\nsecond", - maxRunes: 2, - want: "甲乙", - }, - { - // Zero rune limit: the first line exists but the budget allows no - // runes at all, so the result must be "" rather than panicking. - name: "given zero rune limit then returns empty", - s: "hello\nworld", - maxRunes: 0, - want: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := FirstLine(tt.s, tt.maxRunes); got != tt.want { - t.Fatalf("FirstLine(%q, %d) = %q, want %q", tt.s, tt.maxRunes, got, tt.want) - } - }) - } -} - func TestShortID(t *testing.T) { tests := []struct { name string @@ -687,6 +620,29 @@ func TestCompactTeammateMessage_GivenAgentMessageWithNoAttributes_ThenOmitsTheID } } +// A single message can carry both tag variants when a teammate reply is +// relayed alongside a coordinator note, since nextTeammateTagMatch picks +// whichever variant occurs earliest rather than assuming one variant per +// message. Both blocks must compact and preserve document order. +func TestCompactTeammateMessage_GivenBothTagVariantsInOneBody_ThenCompactsBothInOrder(t *testing.T) { + input := ` +First block. + + + +Second block. +` + + got, ok := CompactTeammateMessage(input) + if !ok { + t.Fatal("CompactTeammateMessage returned false") + } + want := "[teammate: reviewer-1]\nFirst block.\n\n[teammate: worker-2]\nSecond block." + if got != want { + t.Fatalf("got %q, want %q", got, want) + } +} + // --- CompactCommandInjection tests --- func TestCompactCommandInjection_GivenCommandXML_ThenReturnsOneLine(t *testing.T) { diff --git a/internal/session/turn_test.go b/internal/session/turn_test.go index 5083a12..316dee5 100644 --- a/internal/session/turn_test.go +++ b/internal/session/turn_test.go @@ -84,6 +84,10 @@ func TestCountsAsTurn_GivenMessageKind_WhenCounted_ThenFollowsWorkUnitPolicy(t * message: UserMessage{Text: "…", IsMidTurnUserMessage: true, MidTurnUserText: "改一下這裡"}, want: false, }, + "a command marker wins over a teammate flag set on the same message": { + message: UserMessage{Text: "…", CommandMarker: "[/goal]", IsTeammateMessage: true}, + want: false, + }, } for name, tc := range tests {