From ace50757e40e46b51db982e0b86fa037bee95f4d Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Mon, 17 Aug 2026 01:55:14 +0100 Subject: [PATCH] fix: floor token budgets and surface truncation, add live progress Reasoning models burn thousands of completion tokens before the answer. Small per-test caps caused silent truncation and zero scores. The runner now floors max_tokens at the config default (12000). The client parses finish_reason. Reports mark truncated cells and the JSON format carries response text for debugging. A stderr progress reporter prints one line per completed request; --quiet disables it. --- .gitignore | 1 + README.md | 31 +++++- cmd/llmtest/main.go | 36 ++----- cmd/llmtest/progress.go | 63 ++++++++++++ cmd/llmtest/progress_test.go | 113 ++++++++++++++++++++ config.example.yaml | 8 +- internal/config/config.go | 29 ++++++ internal/config/config_test.go | 41 ++++++++ internal/eval/jsonarray.go | 22 ++-- internal/eval/jsonfield.go | 19 ++-- internal/eval/numeric.go | 135 ++++++++++++++++++------ internal/eval/numeric_test.go | 10 ++ internal/llm/llm.go | 8 ++ internal/llm/openai.go | 32 +++++- internal/llm/openai_test.go | 75 ++++++++++++-- internal/report/json.go | 6 ++ internal/report/markdown.go | 6 ++ internal/report/report.go | 36 +++++-- internal/report/report_test.go | 79 ++++++++++++++ internal/report/table.go | 5 + internal/runner/progress.go | 31 ++++++ internal/runner/runner.go | 59 +++++++++-- internal/runner/runner_test.go | 153 ++++++++++++++++++++++++++++ internal/tests/catalog_test.go | 77 +++++++------- internal/tests/golang.go | 15 +-- internal/tests/golang_test.go | 2 +- internal/tests/kubernetes.go | 62 +++++++---- internal/tests/kubernetes_test.go | 44 ++++++++ internal/tests/structlayout.go | 7 +- internal/tests/structlayout_test.go | 42 ++++++++ 30 files changed, 1063 insertions(+), 184 deletions(-) create mode 100644 cmd/llmtest/progress.go create mode 100644 cmd/llmtest/progress_test.go create mode 100644 internal/runner/progress.go diff --git a/.gitignore b/.gitignore index 21ba99d..dcba3ad 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,4 @@ # OS/editor cruft .DS_Store *.swp +.claude/ diff --git a/README.md b/README.md index b764149..702414a 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,25 @@ Both `list` and `run` accept: - `--concurrency` - override the config file's `concurrency`. - `--timeout` - override the config file's `request_timeout` (for example `60s`). -- `--verbose` - print one progress line per completed test to stderr. +- `--quiet` - suppress progress output on stderr. Progress is on by + default: a run can issue hundreds of requests at 30-50s each, and a + silent CLI reads as a hang. + +By default, `run` prints one line to stderr at the start ("running N tests +x M models = K requests, concurrency C") and one line per completed +request ("[done/total] model=... test=... score=... tokens=... (latency)", +with a `TRUNCATED` marker when a response was cut off by the token +budget). stdout carries only the table/markdown/json report, so `llmtest +run --format json > results.json` stays pipeable regardless of `--quiet`. + +If `config.yaml` omits `concurrency`, `request_timeout`, or +`max_tokens_default`, `internal/config` fills in `8`, `300s`, and `12000`. +The effective per-call token budget is `max(test.MaxTokens, +max_tokens_default)`: a test's own `MaxTokens` can only raise the budget +above the config default, never lower it below it. This matters for +reasoning models, which can spend thousands of completion tokens on +`reasoning_content` before writing an answer to `content`; a low budget +truncates the answer before the model reaches it. ### Output @@ -90,6 +108,17 @@ needs a local toolchain that is not installed (for example, a Python exec-evaluator when `python3` is missing). Skipped tests are excluded from every mean. +A cell carries a trailing `!` when the model's response was cut off by the +token budget (`finish_reason=length`) - for example `0.00 (6000tok)!`. A +truncated response's score is still whatever the evaluator computed on the +partial text, so a `!` cell needs a closer look: raise +`max_tokens_default` and re-run. `table` and `markdown` print a one-line +legend explaining `!` whenever at least one cell has it. `json` carries the +same signal per result as `"truncated": true`, plus `"finish_reason"` and +`"response_text"` (the exact normalized text that was scored) so a +truncated or unexpectedly low-scoring result can be debugged without +re-running the model. + ## Test catalog The catalog lives in `internal/tests`, one file per category. Every test diff --git a/cmd/llmtest/main.go b/cmd/llmtest/main.go index d309acb..ba10536 100644 --- a/cmd/llmtest/main.go +++ b/cmd/llmtest/main.go @@ -9,13 +9,11 @@ import ( "os" "sort" "strings" - "time" "github.com/lukaszraczylo/llm-testbench/internal/config" "github.com/lukaszraczylo/llm-testbench/internal/llm" "github.com/lukaszraczylo/llm-testbench/internal/report" "github.com/lukaszraczylo/llm-testbench/internal/runner" - "github.com/lukaszraczylo/llm-testbench/internal/testkit" "github.com/lukaszraczylo/llm-testbench/internal/tests" ) @@ -100,7 +98,7 @@ func runCommand(args []string) error { format := fs.String("format", "table", "output format: table|markdown|json") concurrency := fs.Int("concurrency", 0, "override config's concurrency (0 = use config)") timeout := fs.Duration("timeout", 0, "override config's request_timeout (0 = use config)") - verbose := fs.Bool("verbose", false, "print progress to stderr as tests complete") + quiet := fs.Bool("quiet", false, "suppress progress output on stderr") if err := fs.Parse(args); err != nil { return err } @@ -133,14 +131,20 @@ func runCommand(args []string) error { return fmt.Errorf("no tests matched category=%q subcategory=%q", shared.category, shared.subcategory) } + var reporter runner.ProgressReporter = runner.NoopProgressReporter{} + if !*quiet { + reporter = newStderrProgressReporter(os.Stderr) + } + r := runner.New(client, runner.Config{ Concurrency: conc, Temperature: requestTemperature, MaxTokensDefault: cfg.MaxTokensDefault, + Reporter: reporter, }) ctx := context.Background() - results := runWithProgress(ctx, r, models, selected, *verbose) + results := r.Run(ctx, models, selected) f, err := validateFormat(*format) if err != nil { @@ -163,30 +167,6 @@ func validateFormat(format string) (report.Format, error) { } } -// runWithProgress runs the given tests/models and, when verbose, prints one -// progress line per completed (model, test) pair to stderr. -func runWithProgress(ctx context.Context, r *runner.Runner, models []string, selected []testkit.Test, verbose bool) []runner.Result { - if !verbose { - return r.Run(ctx, models, selected) - } - - fmt.Fprintf(os.Stderr, "running %d tests x %d models = %d calls...\n", len(selected), len(models), len(selected)*len(models)) - start := time.Now() - results := r.Run(ctx, models, selected) - for _, res := range results { - status := fmt.Sprintf("%.2f", res.Score.Value) - switch { - case res.Err != nil: - status = "ERR: " + res.Err.Error() - case res.Score.Skipped: - status = "skip: " + res.Score.Detail - } - fmt.Fprintf(os.Stderr, " %-14s %-28s %s (%s)\n", res.Model, res.TestID, status, res.Latency) - } - fmt.Fprintf(os.Stderr, "done in %s\n", time.Since(start)) - return results -} - func splitCSV(s string) []string { parts := strings.Split(s, ",") out := make([]string, 0, len(parts)) diff --git a/cmd/llmtest/progress.go b/cmd/llmtest/progress.go new file mode 100644 index 0000000..2a980b5 --- /dev/null +++ b/cmd/llmtest/progress.go @@ -0,0 +1,63 @@ +package main + +import ( + "fmt" + "io" + "sync" + + "github.com/lukaszraczylo/llm-testbench/internal/runner" +) + +// stderrProgressReporter implements runner.ProgressReporter, writing one +// line per event to w (os.Stderr in production). Run's stdout stays +// pipeable (table/markdown/json only): all progress goes here instead. +type stderrProgressReporter struct { + w io.Writer + mu sync.Mutex +} + +// newStderrProgressReporter builds a stderrProgressReporter writing to w. +func newStderrProgressReporter(w io.Writer) *stderrProgressReporter { + return &stderrProgressReporter{w: w} +} + +// ReportStart implements runner.ProgressReporter. +func (r *stderrProgressReporter) ReportStart(totalTests, totalModels, concurrency int) { + r.mu.Lock() + defer r.mu.Unlock() + // A write failure on this side-channel progress stream (a closed pipe, + // a full/broken stderr) is not actionable and must not abort the run + // that produces the actual report on stdout. + _, _ = fmt.Fprintf(r.w, "running %d tests x %d models = %d requests, concurrency %d\n", + totalTests, totalModels, totalTests*totalModels, concurrency) +} + +// ReportDone implements runner.ProgressReporter. Safe for concurrent calls +// from up to Config.Concurrency goroutines at once: the mutex serializes +// each printed line so concurrent completions cannot interleave mid-line. +func (r *stderrProgressReporter) ReportDone(done, total int, result runner.Result) { + status := resultStatus(result) + truncated := "" + if result.Truncated() { + truncated = " TRUNCATED" + } + + r.mu.Lock() + defer r.mu.Unlock() + // See ReportStart: a write failure here is not actionable. + _, _ = fmt.Fprintf(r.w, "[%d/%d] model=%s test=%s score=%s tokens=%d (%s)%s\n", + done, total, result.Model, result.TestID, status, result.TotalTokens(), result.Latency, truncated) +} + +// resultStatus renders a Result's score for one progress line: the numeric +// score, or ERR/skip. +func resultStatus(result runner.Result) string { + switch { + case result.Err != nil: + return "ERR" + case result.Score.Skipped: + return "skip" + default: + return fmt.Sprintf("%.2f", result.Score.Value) + } +} diff --git a/cmd/llmtest/progress_test.go b/cmd/llmtest/progress_test.go new file mode 100644 index 0000000..5c5279a --- /dev/null +++ b/cmd/llmtest/progress_test.go @@ -0,0 +1,113 @@ +package main + +import ( + "bytes" + "errors" + "regexp" + "strings" + "sync" + "testing" + "time" + + "github.com/lukaszraczylo/llm-testbench/internal/eval" + "github.com/lukaszraczylo/llm-testbench/internal/runner" +) + +func TestStderrProgressReporter_ReportStart(t *testing.T) { + var buf bytes.Buffer + r := newStderrProgressReporter(&buf) + + r.ReportStart(113, 3, 8) + + got := buf.String() + want := "running 113 tests x 3 models = 339 requests, concurrency 8\n" + if got != want { + t.Errorf("ReportStart() output = %q, want %q", got, want) + } +} + +func TestStderrProgressReporter_ReportDone(t *testing.T) { + tests := []struct { + name string + wantSub []string + result runner.Result + }{ + { + name: "scored result", + result: runner.Result{Model: "m1", TestID: "t1", Score: eval.Score{Value: 0.85}, Latency: 2 * time.Second}, + wantSub: []string{"model=m1", "test=t1", "score=0.85", "(2s)"}, + }, + { + name: "error result", + result: runner.Result{Model: "m1", TestID: "t1", Err: errors.New("boom")}, + wantSub: []string{"score=ERR"}, + }, + { + name: "skipped result", + result: runner.Result{Model: "m1", TestID: "t1", Score: eval.Score{Skipped: true}}, + wantSub: []string{"score=skip"}, + }, + { + name: "truncated result carries the TRUNCATED marker", + result: runner.Result{Model: "m1", TestID: "t1", Score: eval.Score{Value: 0}, FinishReason: "length"}, + wantSub: []string{"score=0.00", "TRUNCATED"}, + }, + { + name: "non-truncated result has no marker", + result: runner.Result{Model: "m1", TestID: "t1", Score: eval.Score{Value: 1}, FinishReason: "stop"}, + wantSub: []string{"score=1.00"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + r := newStderrProgressReporter(&buf) + r.ReportDone(3, 10, tt.result) + + got := buf.String() + if !strings.HasPrefix(got, "[3/10] ") { + t.Errorf("ReportDone() output = %q, want prefix [3/10] ", got) + } + for _, want := range tt.wantSub { + if !strings.Contains(got, want) { + t.Errorf("ReportDone() output = %q, want substring %q", got, want) + } + } + if tt.name == "non-truncated result has no marker" && strings.Contains(got, "TRUNCATED") { + t.Errorf("ReportDone() output = %q, must not contain TRUNCATED", got) + } + }) + } +} + +// TestStderrProgressReporter_ReportDone_ConcurrencySafe writes many +// concurrent ReportDone calls (as Runner.Run's goroutines would) and +// verifies every line is well-formed and none interleaved mid-line - the +// reporter's own mutex must serialize writes, not just the counter. +func TestStderrProgressReporter_ReportDone_ConcurrencySafe(t *testing.T) { + var buf bytes.Buffer + r := newStderrProgressReporter(&buf) + + const n = 50 + var wg sync.WaitGroup + for i := 1; i <= n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + r.ReportDone(i, n, runner.Result{Model: "m", TestID: "t", Score: eval.Score{Value: 1}}) + }(i) + } + wg.Wait() + + lines := strings.Split(strings.TrimRight(buf.String(), "\n"), "\n") + if len(lines) != n { + t.Fatalf("got %d lines, want %d (a corrupted/interleaved write would produce a different count)", len(lines), n) + } + + linePattern := regexp.MustCompile(`^\[\d+/\d+\] model=m test=t score=1\.00 tokens=0 \(0s\)$`) + for _, line := range lines { + if !linePattern.MatchString(line) { + t.Errorf("malformed or interleaved line: %q", line) + } + } +} diff --git a/config.example.yaml b/config.example.yaml index 20f3157..0c043e8 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -4,6 +4,8 @@ models: - uni/deepseek-v4-flash-0731 - uni/qwen3.6-27b - uni/btl-4 -concurrency: 4 -request_timeout: 120s -max_tokens_default: 4000 +concurrency: 8 # default when omitted; matches internal/config's default +request_timeout: 300s # default when omitted +max_tokens_default: 12000 # default when omitted; a reasoning model can spend + # thousands of tokens on reasoning_content before + # writing an answer, so this must stay generous diff --git a/internal/config/config.go b/internal/config/config.go index 3ec1f22..f05b05f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,6 +14,18 @@ import ( //nolint:gosec // this is an env var *name*, not a credential value const apiKeyEnvVar = "LLMTB_API_KEY" // #nosec G101 -- this is an env var *name*, not a credential value +// Defaults applied when the corresponding YAML key is omitted (or explicit +// zero). max_tokens_default was raised from 4000 to 12000 after a live run +// showed reasoning models spend thousands of completion tokens on +// reasoning_content before writing an answer to content: a small +// per-test/default budget truncates the answer (finish_reason=length) +// before it is ever written, scoring a correct model as 0. +const ( + defaultConcurrency = 8 + defaultRequestTimeout = 300 * time.Second + defaultMaxTokens = 12000 +) + // Config is the top-level configuration loaded from config.yaml. type Config struct { Endpoint string `yaml:"endpoint"` @@ -73,6 +85,8 @@ func parse(data []byte) (Config, error) { cfg.RequestTimeout = d } + applyDefaults(&cfg) + if envKey := os.Getenv(apiKeyEnvVar); envKey != "" { cfg.APIKey = envKey } @@ -83,6 +97,21 @@ func parse(data []byte) (Config, error) { return cfg, nil } +// applyDefaults fills in Concurrency, RequestTimeout, and MaxTokensDefault +// when the YAML omitted them (or gave an explicit zero), so a minimal +// config.yaml still runs with sane, generous-enough values. +func applyDefaults(cfg *Config) { + if cfg.Concurrency <= 0 { + cfg.Concurrency = defaultConcurrency + } + if cfg.RequestTimeout <= 0 { + cfg.RequestTimeout = defaultRequestTimeout + } + if cfg.MaxTokensDefault <= 0 { + cfg.MaxTokensDefault = defaultMaxTokens + } +} + // Validate checks that required fields are present and sane. func (c Config) Validate() error { if c.Endpoint == "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 40d5c8a..2221843 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -75,6 +75,47 @@ max_tokens_default: 4000 } } +func TestParse_DefaultsAppliedWhenOmitted(t *testing.T) { + yaml := ` +endpoint: https://llm-gateway.example.com/v1 +models: + - uni/deepseek-v4-flash-0731 +` + cfg, err := parse([]byte(yaml)) + if err != nil { + t.Fatalf("parse() error = %v", err) + } + if cfg.Concurrency != defaultConcurrency { + t.Errorf("Concurrency = %d, want default %d", cfg.Concurrency, defaultConcurrency) + } + if cfg.RequestTimeout != defaultRequestTimeout { + t.Errorf("RequestTimeout = %v, want default %v", cfg.RequestTimeout, defaultRequestTimeout) + } + if cfg.MaxTokensDefault != defaultMaxTokens { + t.Errorf("MaxTokensDefault = %d, want default %d", cfg.MaxTokensDefault, defaultMaxTokens) + } +} + +func TestParse_DefaultsDoNotOverrideExplicitValues(t *testing.T) { + cfg, err := parse([]byte(validYAML)) + if err != nil { + t.Fatalf("parse() error = %v", err) + } + // validYAML sets concurrency=4, request_timeout=120s, + // max_tokens_default=4000, all below the new defaults; an explicit + // value, even a small one, must still win over the default floor + // applied by applyDefaults. + if cfg.Concurrency != 4 { + t.Errorf("Concurrency = %d, want the explicit 4, not the default", cfg.Concurrency) + } + if cfg.RequestTimeout != 120*time.Second { + t.Errorf("RequestTimeout = %v, want the explicit 120s, not the default", cfg.RequestTimeout) + } + if cfg.MaxTokensDefault != 4000 { + t.Errorf("MaxTokensDefault = %d, want the explicit 4000, not the default", cfg.MaxTokensDefault) + } +} + func TestParse_InvalidYAML(t *testing.T) { _, err := parse([]byte("not: [valid: yaml")) if err == nil { diff --git a/internal/eval/jsonarray.go b/internal/eval/jsonarray.go index e41d7c8..e7f5908 100644 --- a/internal/eval/jsonarray.go +++ b/internal/eval/jsonarray.go @@ -7,8 +7,10 @@ import ( "strings" ) -// decodeStringArray extracts the first JSON value from response and decodes -// it as a []string. Elements that are not strings cause an error. +// decodeStringArray extracts response's JSON value via ExtractJSON (the +// first ```json fenced block if present, otherwise the last balanced JSON +// value in the text) and decodes it as a []string. Elements that are not +// strings cause an error. func decodeStringArray(response string) ([]string, error) { raw, err := ExtractJSON(response) if err != nil { @@ -27,9 +29,10 @@ type jsonStringSetEval struct { want []string } -// JSONStringSet returns an Evaluator that parses the first JSON array in the -// response as a set of strings (case-insensitive, trimmed) and compares it -// to want, order-independent. Score is the Jaccard-style overlap +// JSONStringSet returns an Evaluator that parses response's JSON array (see +// decodeStringArray/ExtractJSON) as a set of strings (case-insensitive, +// trimmed) and compares it to want, order-independent. Score is the +// Jaccard-style overlap // |intersection| / max(len(want), len(got)), so both missing and extraneous // entries cost credit; full credit requires an exact set match. func JSONStringSet(want []string) Evaluator { @@ -81,10 +84,11 @@ type jsonStringArrayEqualsEval struct { want []string } -// JSONStringArrayEquals returns an Evaluator that parses the first JSON -// array in the response as []string and awards full credit only when it -// exactly equals want element-for-element, in order (case-insensitive, -// trimmed). Any mismatch, including reordering, scores zero. +// JSONStringArrayEquals returns an Evaluator that parses response's JSON +// array (see decodeStringArray/ExtractJSON) as []string and awards full +// credit only when it exactly equals want element-for-element, in order +// (case-insensitive, trimmed). Any mismatch, including reordering, scores +// zero. func JSONStringArrayEquals(want []string) Evaluator { return jsonStringArrayEqualsEval{want: want} } diff --git a/internal/eval/jsonfield.go b/internal/eval/jsonfield.go index 8ce15a6..4b36fb4 100644 --- a/internal/eval/jsonfield.go +++ b/internal/eval/jsonfield.go @@ -122,19 +122,22 @@ func asFloat64(v any) (float64, error) { } } -// jsonFieldEval extracts one field by path from the first JSON value in the -// response and compares it to want. +// jsonFieldEval extracts one field by path from the response's JSON value +// (see ExtractJSON: the first ```json fenced block if present, otherwise +// the last balanced JSON value in the response) and compares it to want. type jsonFieldEval[T comparable] struct { want T path string } -// JSONField returns an Evaluator that extracts the first JSON object/array -// in the response (stripping surrounding prose and code fences), navigates -// path (dot/bracket syntax, e.g. "bump" or "steps[0]"), and awards full -// credit when the field equals want. T is inferred from want; supported -// types are string, bool, float64, and int. String comparison is -// case-insensitive and trims whitespace. +// JSONField returns an Evaluator that extracts the response's JSON +// object/array via ExtractJSON (stripping surrounding prose and code +// fences; the first fenced ```json block wins if present, otherwise the +// last balanced JSON value in the text - see ExtractJSON), navigates path +// (dot/bracket syntax, e.g. "bump" or "steps[0]"), and awards full credit +// when the field equals want. T is inferred from want; supported types are +// string, bool, float64, and int. String comparison is case-insensitive +// and trims whitespace. func JSONField[T comparable](path string, want T) Evaluator { return jsonFieldEval[T]{path: path, want: want} } diff --git a/internal/eval/numeric.go b/internal/eval/numeric.go index cc43da6..71bbc4f 100644 --- a/internal/eval/numeric.go +++ b/internal/eval/numeric.go @@ -17,43 +17,115 @@ type Number interface { ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 } -// numberPattern matches signed integers and decimals, including a +// digitPattern matches unsigned integers and decimals, including a // leading-dot form with no integer part (".9896"). Longer alternatives are // listed first so the match is not truncated to just the integer part of -// "0.9896" or just the dot-less ".9896". -var numberPattern = regexp.MustCompile(`-?(?:\d+\.\d+|\.\d+|\d+)`) +// "0.9896" or just the dot-less ".9896". It deliberately excludes the sign: +// classifyMatch decides, per match, whether a leading '-' in the source +// text is a real minus sign or a compound-word hyphen ("x86-64"). +var digitPattern = regexp.MustCompile(`\d+\.\d+|\.\d+|\d+`) -// hyphenCompoundSuffixPattern matches a hyphen directly after a number -// joining it to a following word, e.g. the "-bit" in "64-bit" or the -// "-byte" in "8-byte". A number immediately followed by this is part of a -// unit/architecture descriptor, not a standalone answer value, and is -// skipped when picking "the" number out of a line - otherwise "sizeof(...) -// is 24 bytes on a 64-bit system" would extract the trailing 64 instead of -// the actual answer, 24. -var hyphenCompoundSuffixPattern = regexp.MustCompile(`^-\w`) +// isWordByte reports whether b is a letter, digit, or underscore - the +// standard regex \w character class - used to detect a digit run glued +// directly to an identifier ("LP64", the "86"/"64" in "x86_64"). +func isWordByte(b byte) bool { + return b == '_' || + (b >= 'a' && b <= 'z') || + (b >= 'A' && b <= 'Z') || + (b >= '0' && b <= '9') +} -// lastRelevantNumber returns the last number in text that is not part of a -// "-word" compound, scanning from the end. -func lastRelevantNumber(text string) (string, bool) { - matches := numberPattern.FindAllStringIndex(text, -1) +// numberCandidate is one digitPattern match, classified by classifyMatch. +type numberCandidate struct { + // literal is the text to parse: the matched digits, with a leading '-' + // prepended when classifyMatch determined it is a real sign. + literal string + // compoundHyphen marks a number joined by a hyphen to an adjacent word + // ("64-bit", "8-byte", or the "64" in "x86-64"): a unit/architecture + // descriptor, not a standalone answer value. Rejected in the first two + // extraction tiers, accepted only as ExtractLastNumber's last resort. + compoundHyphen bool +} + +// classifyMatch inspects the characters immediately surrounding the +// unsigned digit run text[start:end] and classifies it: +// +// - a digit run glued directly (no separating punctuation) to a +// letter/digit/underscore on either side is part of an identifier +// ("LP64", "x86_64") and is never a real candidate, at any tier; +// - a '-' immediately before the run is a real minus sign only when the +// character before THAT is not alphanumeric, so "x86-64" does not read +// as "x86" followed by the negative number -64; +// - a number joined by a hyphen to an adjacent word on either side +// ("64-bit", "8-byte", the "64" in "x86-64" once its bogus sign is +// stripped) is flagged compoundHyphen rather than rejected outright. +func classifyMatch(text string, start, end int) (numberCandidate, bool) { + literal := text[start:end] + + if start > 0 && text[start-1] == '-' { + signIsReal := start-2 < 0 || !isWordByte(text[start-2]) + if signIsReal { + literal = "-" + literal + start-- + } + } + + if start > 0 && isWordByte(text[start-1]) { + return numberCandidate{}, false // e.g. the "64" in "LP64" + } + if end < len(text) && isWordByte(text[end]) { + return numberCandidate{}, false // e.g. "24bytes" with no separator + } + + compound := start > 0 && text[start-1] == '-' && start-2 >= 0 && isWordByte(text[start-2]) + + if end < len(text) && text[end] == '-' && end+1 < len(text) && isWordByte(text[end+1]) { + compound = true // e.g. "64-bit", "24-byte" + } + + return numberCandidate{literal: literal, compoundHyphen: compound}, true +} + +// findLastNumber returns the last acceptable numberCandidate's literal in +// text, scanning from the end. allowCompound controls whether a +// compoundHyphen candidate ("24-byte") counts as acceptable. +func findLastNumber(text string, allowCompound bool) (string, bool) { + matches := digitPattern.FindAllStringIndex(text, -1) for i := len(matches) - 1; i >= 0; i-- { - start, end := matches[i][0], matches[i][1] - if hyphenCompoundSuffixPattern.MatchString(text[end:]) { + cand, ok := classifyMatch(text, matches[i][0], matches[i][1]) + if !ok { continue } - return text[start:end], true + if cand.compoundHyphen && !allowCompound { + continue + } + return cand.literal, true } return "", false } // ExtractLastNumber returns the model's answer number from s, converted to -// T. It first looks only at the last non-empty line (the common case: a -// final "24" or "0.9896" on its own line after any reasoning), and only -// falls back to the last number anywhere in s if that line has no usable -// number at all. Within whichever text it searches, it skips a number that -// is part of a "-word" compound (see hyphenCompoundSuffixPattern): -// "sizeof(struct Config) is 24 bytes on a 64-bit system" must extract 24, -// not the later, unrelated 64. It is the default extractor for Numeric. +// T, in three tiers: +// +// 1. The last non-empty line, accepting only a standalone number - not +// one glued to an identifier ("LP64", "x86_64") or joined by a hyphen +// to a unit/architecture word ("64-bit", "x86-64"). +// 2. If that line has no standalone number, the same standalone-only +// search over the whole response (the earlier "24 bytes on a 64-bit +// system" -> 24 case, when the qualifier ends up on the last line). +// 3. If still nothing, a hyphen-compound number is accepted as a last +// resort rather than erroring ("It is a 24-byte struct." -> 24). +// +// A number glued directly to an identifier is never accepted, at any +// tier: "LP64"/"x86_64" never contribute 64 as an answer. +// +// This is inherently a heuristic, not a parser: a response like "Total: 24 +// bytes (22 bytes of members plus 2 tail padding)" has three standalone, +// non-compound numbers, and ExtractLastNumber returns the last one (2), +// not the intended total (24). Prompts whose expected answer format risks +// this kind of trailing parenthetical breakdown should ask for the number +// alone, with no explanation, rather than rely on this heuristic to find +// it in prose. func ExtractLastNumber[T Number](s string) (T, error) { lines := strings.Split(s, "\n") for i := len(lines) - 1; i >= 0; i-- { @@ -61,20 +133,25 @@ func ExtractLastNumber[T Number](s string) (T, error) { if line == "" { continue } - if lit, ok := lastRelevantNumber(line); ok { + if lit, ok := findLastNumber(line, false); ok { return parseNumber[T](lit) } break } - if lit, ok := lastRelevantNumber(s); ok { + if lit, ok := findLastNumber(s, false); ok { return parseNumber[T](lit) } + + if lit, ok := findLastNumber(s, true); ok { + return parseNumber[T](lit) + } + return 0, fmt.Errorf("no number found in response") } -// parseNumber converts a numberPattern match (possibly ".9896", with no -// leading zero) to T. +// parseNumber converts a digitPattern-derived literal (possibly ".9896", +// with no leading zero, or signed) to T. func parseNumber[T Number](literal string) (T, error) { f, err := strconv.ParseFloat(literal, 64) if err != nil { diff --git a/internal/eval/numeric_test.go b/internal/eval/numeric_test.go index 727406a..c5afcb6 100644 --- a/internal/eval/numeric_test.go +++ b/internal/eval/numeric_test.go @@ -22,6 +22,16 @@ func TestExtractLastNumber(t *testing.T) { {"leading-dot decimal", "= .9896", 0.9896, false}, {"answer on last line, reasoning with numbers on earlier lines", "step 1 uses 8 items\nstep 2 uses 16 items\nfinal answer: 42", 42, false}, {"last line has only a unit-qualified number, falls back to whole text", "the computation used 100 iterations\nresult on a 64-bit system", 100, false}, + // 5b regressions: opus round 2's failing set. + {"platform qualifier with parenthetical acronym", "24 bytes on a 64-bit (LP64) platform.", 24, false}, + {"digits glued into an identifier via underscore", "sizeof = 24 bytes on x86_64", 24, false}, + {"digits glued into an identifier via hyphen (bogus sign)", "sizeof = 24 bytes on x86-64", 24, false}, + {"hyphen-compound number accepted as a last resort", "It is a 24-byte struct.", 24, false}, + // Documented limitation, not a regression: the heuristic has no + // notion of "the total" vs. "a breakdown component" and returns + // the last standalone number, which here is not the answer a + // human would pick. Prompts should ask for the number alone. + {"ambiguous parenthetical breakdown returns the wrong number by design", "Total: 24 bytes (22 bytes of members plus 2 tail padding)", 2, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/llm/llm.go b/internal/llm/llm.go index 9018ef1..75fcb58 100644 --- a/internal/llm/llm.go +++ b/internal/llm/llm.go @@ -24,11 +24,19 @@ type Request struct { // Response is the normalized result of a chat-completion call. type Response struct { Text string + FinishReason string PromptTokens int CompletionTokens int Latency time.Duration } +// FinishReasonLength is the OpenAI-compatible finish_reason value meaning +// the model hit its token budget (MaxTokens) before it finished: the +// response, and Text in particular, may be truncated or empty. A reasoning +// model that spends its whole budget on internal reasoning before writing +// anything to the answer channel reports this with Text == "". +const FinishReasonLength = "length" + // Client completes chat requests against a language model backend. // Implementations must be safe for concurrent use. type Client interface { diff --git a/internal/llm/openai.go b/internal/llm/openai.go index 5e529bc..35e65be 100644 --- a/internal/llm/openai.go +++ b/internal/llm/openai.go @@ -50,12 +50,36 @@ type chatMsg struct { Content string `json:"content"` } +// responseMsg is the assistant message inside one response choice. Content +// is a pointer, not a plain string: some OpenAI-compatible backends return +// "content": null when generation was cut off before any answer token was +// emitted (for example finish_reason=length on a reasoning model that +// spent its whole token budget on reasoning_content and never reached +// content). Text reports that case as "", explicitly, rather than relying +// on encoding/json's implicit null-into-string zero-value behavior. +// +// Deliberately not evaluated: reasoning_content. The catalog's evaluators +// score the answer a user would actually see, which is content only. +type responseMsg struct { + Content *string `json:"content"` + Role string `json:"role"` +} + +// Text returns m.Content, or "" if the backend sent "content": null. +func (m responseMsg) Text() string { + if m.Content == nil { + return "" + } + return *m.Content +} + type chatCompletionResponse struct { Error *struct { Message string `json:"message"` } `json:"error"` Choices []struct { - Message chatMsg `json:"message"` + Message responseMsg `json:"message"` + FinishReason string `json:"finish_reason"` } `json:"choices"` Usage struct { PromptTokens int `json:"prompt_tokens"` @@ -105,9 +129,10 @@ func (c *OpenAIClient) Complete(ctx context.Context, req Request) (Response, err } latency := time.Since(attemptStart) - text := "" + var text, finishReason string if len(resp.Choices) > 0 { - text = resp.Choices[0].Message.Content + text = resp.Choices[0].Message.Text() + finishReason = resp.Choices[0].FinishReason } if resp.Error != nil { lastErr = fmt.Errorf("llm: api error: %s", resp.Error.Message) @@ -115,6 +140,7 @@ func (c *OpenAIClient) Complete(ctx context.Context, req Request) (Response, err } return Response{ Text: text, + FinishReason: finishReason, PromptTokens: resp.Usage.PromptTokens, CompletionTokens: resp.Usage.CompletionTokens, Latency: latency, diff --git a/internal/llm/openai_test.go b/internal/llm/openai_test.go index 9bdb275..47a11a3 100644 --- a/internal/llm/openai_test.go +++ b/internal/llm/openai_test.go @@ -21,6 +21,23 @@ func newTestServer(t *testing.T, handler http.HandlerFunc) *httptest.Server { return srv } +// testChoice builds one response choice with a non-null content string, the +// common case. Its return type is exactly chatCompletionResponse.Choices's +// element type, so callers can append() it without repeating the anonymous +// struct type at each call site. +func testChoice(content, finishReason string) struct { + Message responseMsg `json:"message"` + FinishReason string `json:"finish_reason"` +} { + return struct { + Message responseMsg `json:"message"` + FinishReason string `json:"finish_reason"` + }{ + Message: responseMsg{Role: "assistant", Content: &content}, + FinishReason: finishReason, + } +} + func TestOpenAIClient_Complete_Success(t *testing.T) { srv := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/chat/completions" { @@ -34,9 +51,7 @@ func TestOpenAIClient_Complete_Success(t *testing.T) { t.Errorf("model = %q, want test-model", body.Model) } resp := chatCompletionResponse{} - resp.Choices = []struct { - Message chatMsg `json:"message"` - }{{Message: chatMsg{Role: "assistant", Content: "hello there"}}} + resp.Choices = append(resp.Choices, testChoice("hello there", "stop")) resp.Usage.PromptTokens = 12 resp.Usage.CompletionTokens = 3 _ = json.NewEncoder(w).Encode(resp) @@ -53,11 +68,61 @@ func TestOpenAIClient_Complete_Success(t *testing.T) { if resp.Text != "hello there" { t.Errorf("Text = %q, want %q", resp.Text, "hello there") } + if resp.FinishReason != "stop" { + t.Errorf("FinishReason = %q, want stop", resp.FinishReason) + } if resp.PromptTokens != 12 || resp.CompletionTokens != 3 { t.Errorf("tokens = %d/%d, want 12/3", resp.PromptTokens, resp.CompletionTokens) } } +// TestOpenAIClient_Complete_ParsesFinishReasonLength verifies +// choices[0].finish_reason lands in Response.FinishReason == "length", the +// signal that generation was cut off by the token budget (root cause of the +// live-run incident: a truncated reasoning-model response silently scored +// 0 with no visible signal that it was ever cut off). +func TestOpenAIClient_Complete_ParsesFinishReasonLength(t *testing.T) { + srv := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { + resp := chatCompletionResponse{} + resp.Choices = append(resp.Choices, testChoice("partial ans", FinishReasonLength)) + _ = json.NewEncoder(w).Encode(resp) + }) + + client := NewOpenAIClient(srv.URL, "", time.Second) + resp, err := client.Complete(context.Background(), Request{Model: "m", Messages: []Message{{Role: "user", Content: "x"}}}) + if err != nil { + t.Fatalf("Complete() error = %v", err) + } + if resp.FinishReason != FinishReasonLength { + t.Errorf("FinishReason = %q, want %q", resp.FinishReason, FinishReasonLength) + } +} + +// TestOpenAIClient_Complete_NullContentIsEmptyString verifies a backend +// sending "content": null (a reasoning model that spent its whole token +// budget on reasoning_content and never wrote to content) decodes to +// Response.Text == "", not a decode error or a panic. +func TestOpenAIClient_Complete_NullContentIsEmptyString(t *testing.T) { + srv := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"choices":[{"message":{"role":"assistant","content":null},"finish_reason":"length"}],"usage":{"prompt_tokens":50,"completion_tokens":6000}}`)) + }) + + client := NewOpenAIClient(srv.URL, "", time.Second) + resp, err := client.Complete(context.Background(), Request{Model: "m", Messages: []Message{{Role: "user", Content: "x"}}}) + if err != nil { + t.Fatalf("Complete() error = %v", err) + } + if resp.Text != "" { + t.Errorf("Text = %q, want empty string for null content", resp.Text) + } + if resp.FinishReason != FinishReasonLength { + t.Errorf("FinishReason = %q, want %q", resp.FinishReason, FinishReasonLength) + } + if resp.CompletionTokens != 6000 { + t.Errorf("CompletionTokens = %d, want 6000 (tokens were spent on reasoning_content, not content)", resp.CompletionTokens) + } +} + // TestOpenAIClient_Complete_SendsTemperatureZero verifies Request.Temperature // (PLAN.md pins this to 0 for deterministic scoring, wired from main.go's // requestTemperature constant) actually lands on the wire as a JSON @@ -107,9 +172,7 @@ func TestOpenAIClient_Complete_RetriesOn5xxThenSucceeds(t *testing.T) { return } resp := chatCompletionResponse{} - resp.Choices = []struct { - Message chatMsg `json:"message"` - }{{Message: chatMsg{Role: "assistant", Content: "ok"}}} + resp.Choices = append(resp.Choices, testChoice("ok", "stop")) _ = json.NewEncoder(w).Encode(resp) }) diff --git a/internal/report/json.go b/internal/report/json.go index 330b6d8..ba593da 100644 --- a/internal/report/json.go +++ b/internal/report/json.go @@ -17,12 +17,15 @@ type jsonResult struct { Subcategory string `json:"subcategory,omitempty"` Detail string `json:"detail,omitempty"` Error string `json:"error,omitempty"` + ResponseText string `json:"response_text"` + FinishReason string `json:"finish_reason,omitempty"` Score float64 `json:"score"` LatencyMS int64 `json:"latency_ms"` PromptTokens int `json:"prompt_tokens"` CompletionTokens int `json:"completion_tokens"` Tokens int `json:"tokens"` Skipped bool `json:"skipped"` + Truncated bool `json:"truncated"` } // renderJSON dumps raw results as a JSON array, joined with test metadata. @@ -42,7 +45,10 @@ func renderJSON(w io.Writer, tests []testkit.Test, results []runner.Result) erro Subcategory: t.Subcategory, Score: r.Score.Value, Detail: r.Score.Detail, + ResponseText: r.ResponseText, + FinishReason: r.FinishReason, Skipped: r.Score.Skipped, + Truncated: r.Truncated(), LatencyMS: r.Latency.Milliseconds(), PromptTokens: r.PromptTokens, CompletionTokens: r.CompletionTokens, diff --git a/internal/report/markdown.go b/internal/report/markdown.go index 4aa07b1..ad229f3 100644 --- a/internal/report/markdown.go +++ b/internal/report/markdown.go @@ -20,14 +20,20 @@ func renderMarkdown(w io.Writer, tests []testkit.Test, models []string, results ew.println() scoreHeader := append([]string{"Test", "Category", "Subcategory"}, models...) writeMarkdownHeader(ew, scoreHeader) + anyTruncated := false for _, t := range sortedTests(tests) { row := []string{t.ID, t.Category, t.Subcategory} for _, model := range models { r, ok := idx[t.ID][model] row = append(row, cellText(r, ok)) + anyTruncated = anyTruncated || r.Truncated() } writeMarkdownRow(ew, row) } + if anyTruncated { + ew.println() + ew.println(truncatedLegend) + } ew.println() ew.println("## Category rollup (mean score)") diff --git a/internal/report/report.go b/internal/report/report.go index 85ae4cb..876cae0 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -72,19 +72,37 @@ func sortedTests(tests []testkit.Test) []testkit.Test { return out } +// truncatedSuffix marks a cell whose underlying response was cut off by +// the token budget (finish_reason=length): the score may not reflect what +// the model would have answered with a larger budget. truncatedLegend is +// the one-line explanation printed once per table/markdown report. +const ( + truncatedSuffix = "!" + truncatedLegend = "! = response truncated by the token budget (finish_reason=length); increase max_tokens_default to avoid" +) + // cellText renders one (test, model) cell for the score table: "score -// (Ntok)" for a scored result, or ERR/skip/N/A otherwise. +// (Ntok)" for a scored result, or ERR/skip/N/A otherwise, with a +// truncatedSuffix appended when the response was cut off by the token +// budget. func cellText(r runner.Result, found bool) string { - switch { - case !found: + if !found { return "N/A" - case r.Err != nil: - return "ERR" - case r.Score.Skipped: - return "skip" - default: - return fmt.Sprintf("%.2f (%dtok)", r.Score.Value, r.TotalTokens()) } + + var text string + switch { + case r.Err != nil: + text = "ERR" + case r.Score.Skipped: + text = "skip" + default: + text = fmt.Sprintf("%.2f (%dtok)", r.Score.Value, r.TotalTokens()) + } + if r.Truncated() { + text += truncatedSuffix + } + return text } // categoryMean holds the per-model mean score for one category, computed diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 3cfffe7..f275046 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -116,6 +116,85 @@ func TestRenderJSON_DumpsRawResults(t *testing.T) { } } +func TestRenderTable_TruncatedCellsGetSuffixAndLegend(t *testing.T) { + tests := []testkit.Test{{ID: "t1", Category: "c", Subcategory: "s"}} + results := []runner.Result{ + {Model: "m1", TestID: "t1", Score: eval.Score{Value: 0, Detail: "TRUNCATED: got \"\""}, FinishReason: "length"}, + } + var buf bytes.Buffer + if err := Render(&buf, FormatTable, tests, []string{"m1"}, results); err != nil { + t.Fatalf("Render() error = %v", err) + } + out := buf.String() + if !strings.Contains(out, "0.00 (0tok)!") { + t.Errorf("table output missing truncated \"!\" suffix:\n%s", out) + } + if !strings.Contains(out, truncatedLegend) { + t.Errorf("table output missing the truncation legend:\n%s", out) + } +} + +func TestRenderTable_NoLegendWhenNothingTruncated(t *testing.T) { + var buf bytes.Buffer + if err := Render(&buf, FormatTable, sampleTests(), []string{"m1", "m2"}, sampleResults()); err != nil { + t.Fatalf("Render() error = %v", err) + } + out := buf.String() + if strings.Contains(out, truncatedLegend) { + t.Errorf("table output should not print the truncation legend when nothing truncated:\n%s", out) + } +} + +func TestRenderMarkdown_TruncatedCellsGetSuffixAndLegend(t *testing.T) { + tests := []testkit.Test{{ID: "t1", Category: "c", Subcategory: "s"}} + results := []runner.Result{ + {Model: "m1", TestID: "t1", Score: eval.Score{Value: 0}, FinishReason: "length"}, + } + var buf bytes.Buffer + if err := Render(&buf, FormatMarkdown, tests, []string{"m1"}, results); err != nil { + t.Fatalf("Render() error = %v", err) + } + out := buf.String() + if !strings.Contains(out, "0.00 (0tok)!") { + t.Errorf("markdown output missing truncated \"!\" suffix:\n%s", out) + } + if !strings.Contains(out, truncatedLegend) { + t.Errorf("markdown output missing the truncation legend:\n%s", out) + } +} + +func TestRenderJSON_IncludesResponseTextFinishReasonAndTruncated(t *testing.T) { + tests := []testkit.Test{{ID: "t1", Category: "c"}} + results := []runner.Result{ + {Model: "m1", TestID: "t1", Score: eval.Score{Value: 0, Detail: "TRUNCATED: got \"\""}, ResponseText: "partial answer", FinishReason: "length"}, + } + var buf bytes.Buffer + if err := Render(&buf, FormatJSON, tests, []string{"m1"}, results); err != nil { + t.Fatalf("Render() error = %v", err) + } + + var out []jsonResult + if err := json.Unmarshal(buf.Bytes(), &out); err != nil { + t.Fatalf("json.Unmarshal() error = %v; output:\n%s", err, buf.String()) + } + if len(out) != 1 { + t.Fatalf("len(out) = %d, want 1", len(out)) + } + got := out[0] + if got.ResponseText != "partial answer" { + t.Errorf("ResponseText = %q, want %q", got.ResponseText, "partial answer") + } + if got.FinishReason != "length" { + t.Errorf("FinishReason = %q, want length", got.FinishReason) + } + if !got.Truncated { + t.Error("Truncated = false, want true") + } + if !strings.HasPrefix(got.Detail, "TRUNCATED: ") { + t.Errorf("Detail = %q, want a TRUNCATED: prefix", got.Detail) + } +} + func TestRender_UnknownFormat(t *testing.T) { var buf bytes.Buffer err := Render(&buf, Format("bogus"), sampleTests(), []string{"m1"}, sampleResults()) diff --git a/internal/report/table.go b/internal/report/table.go index b20bad0..b7335a1 100644 --- a/internal/report/table.go +++ b/internal/report/table.go @@ -51,14 +51,19 @@ func writeScoreTable(ew *errWriter, tests []testkit.Test, models []string, idx r header = append(header, models...) ew.println(strings.Join(header, "\t")) + anyTruncated := false for _, t := range sortedTests(tests) { row := []string{t.ID, t.Category, t.Subcategory} for _, model := range models { r, ok := idx[t.ID][model] row = append(row, cellText(r, ok)) + anyTruncated = anyTruncated || r.Truncated() } ew.println(strings.Join(row, "\t")) } + if anyTruncated { + ew.println(truncatedLegend) + } } func writeCategoryTable(ew *errWriter, tests []testkit.Test, models []string, idx resultIndex) { diff --git a/internal/runner/progress.go b/internal/runner/progress.go new file mode 100644 index 0000000..2e442c6 --- /dev/null +++ b/internal/runner/progress.go @@ -0,0 +1,31 @@ +package runner + +// ProgressReporter receives progress events as Run executes, so a caller +// (typically the CLI) can show the operator something is happening across +// a run that may issue hundreds of requests at 30-50s each. Implementations +// must be safe for concurrent use: ReportDone is called from up to +// Config.Concurrency goroutines at once. +type ProgressReporter interface { + // ReportStart is called once, before any request is issued. + // totalTests and totalModels are the catalog and model-list sizes + // being fanned out (totalTests*totalModels is the total request + // count); concurrency is the bound Run is applying. + ReportStart(totalTests, totalModels, concurrency int) + + // ReportDone is called once per completed (model, test) call, after + // Run has already recorded result into its output slice. done is this + // call's 1-based completion index; total is the overall request + // count (totalTests*totalModels from ReportStart). + ReportDone(done, total int, result Result) +} + +// NoopProgressReporter discards every event. It is the default Runner uses +// when Config.Reporter is nil (unit tests, and any caller that does not +// want progress output, e.g. the CLI's --quiet flag). +type NoopProgressReporter struct{} + +// ReportStart implements ProgressReporter. +func (NoopProgressReporter) ReportStart(_, _, _ int) {} + +// ReportDone implements ProgressReporter. +func (NoopProgressReporter) ReportDone(_, _ int, _ Result) {} diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 47e71d1..1b1bf27 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -4,6 +4,7 @@ package runner import ( "context" + "sync/atomic" "time" "golang.org/x/sync/errgroup" @@ -13,17 +14,34 @@ import ( "github.com/lukaszraczylo/llm-testbench/internal/testkit" ) +// truncatedDetailPrefix is prepended to Score.Detail when the model's +// response was cut off by the token budget (llm.FinishReasonLength), so +// the reason a test scored the way it did is visible without cross- +// referencing FinishReason separately. +const truncatedDetailPrefix = "TRUNCATED: " + // Result is the outcome of running one test against one model. type Result struct { Err error Model string TestID string + ResponseText string + FinishReason string Score eval.Score Latency time.Duration PromptTokens int CompletionTokens int } +// Truncated reports whether the model's response was cut off by the token +// budget before it finished (finish_reason=length), rather than completing +// normally. A truncated response's Score is still whatever the evaluator +// computed on the partial text; Truncated is the signal that the score may +// not reflect what the model would have said with a larger budget. +func (r Result) Truncated() bool { + return r.FinishReason == llm.FinishReasonLength +} + // TotalTokens returns PromptTokens + CompletionTokens, the total token // usage billed for this (model, test) call. func (r Result) TotalTokens() int { @@ -32,12 +50,9 @@ func (r Result) TotalTokens() int { // Config controls how the Runner fans work out. type Config struct { - // Concurrency bounds the number of in-flight (model,test) calls. - Concurrency int - // Temperature is sent on every request; PLAN.md pins this to 0 for - // determinism across runs. - Temperature float64 - // MaxTokensDefault is used when a Test does not set its own MaxTokens. + Reporter ProgressReporter + Concurrency int + Temperature float64 MaxTokensDefault int } @@ -49,6 +64,9 @@ type Runner struct { // New builds a Runner that issues requests through client. func New(client llm.Client, cfg Config) *Runner { + if cfg.Reporter == nil { + cfg.Reporter = NoopProgressReporter{} + } return &Runner{client: client, cfg: cfg} } @@ -77,11 +95,21 @@ func (r *Runner) Run(ctx context.Context, models []string, tests []testkit.Test) } g.SetLimit(concurrency) + r.cfg.Reporter.ReportStart(len(tests), len(models), concurrency) + + var completed int32 // go.mod's go directive is 1.22+, so loop variables are already // per-iteration scoped; no manual i, j := i, j shadowing needed (N4). for i, j := range jobs { g.Go(func() error { - results[i] = r.runOne(gctx, j.model, j.test) + res := r.runOne(gctx, j.model, j.test) + results[i] = res + // atomic.AddInt32 makes the completion counter safe under + // Config.Concurrency-many concurrent goroutines; ReportDone + // itself must also be concurrency-safe per the ProgressReporter + // contract. + done := atomic.AddInt32(&completed, 1) + r.cfg.Reporter.ReportDone(int(done), len(jobs), res) return nil }) } @@ -95,10 +123,14 @@ func (r *Runner) Run(ctx context.Context, models []string, tests []testkit.Test) // runOne executes a single (model, test) combination. func (r *Runner) runOne(ctx context.Context, model string, test testkit.Test) Result { - maxTokens := test.MaxTokens - if maxTokens <= 0 { - maxTokens = r.cfg.MaxTokensDefault - } + // cfg.MaxTokensDefault is a floor, not a fallback used only when the + // test leaves MaxTokens unset: a per-test value only ever raises the + // budget, it never lowers it below the default. A reasoning model can + // burn thousands of tokens on reasoning_content before ever writing an + // answer to content, so a small per-test MaxTokens (sized for the + // answer alone) previously starved the model into finish_reason=length + // with an empty answer. + maxTokens := max(test.MaxTokens, r.cfg.MaxTokensDefault) messages := make([]llm.Message, 0, 2) if test.System != "" { @@ -120,11 +152,16 @@ func (r *Runner) runOne(ctx context.Context, model string, test testkit.Test) Re normalized := testkit.Normalize(resp.Text) score := test.Eval.Evaluate(ctx, normalized) + if resp.FinishReason == llm.FinishReasonLength { + score.Detail = truncatedDetailPrefix + score.Detail + } return Result{ Model: model, TestID: test.ID, Score: score, + ResponseText: normalized, + FinishReason: resp.FinishReason, Latency: resp.Latency, PromptTokens: resp.PromptTokens, CompletionTokens: resp.CompletionTokens, diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 2bef353..48ceef6 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -3,6 +3,8 @@ package runner import ( "context" "errors" + "strings" + "sync" "sync/atomic" "testing" "time" @@ -180,6 +182,74 @@ func TestRunner_Run_FallsBackToDefaultMaxTokens(t *testing.T) { } } +// TestRunner_Run_DefaultIsAFloorNotJustAFallback is the regression test for +// the systemic live-run bug: cfg.MaxTokensDefault must raise a too-small +// per-test MaxTokens, not just fill in for a zero one. +func TestRunner_Run_DefaultIsAFloorNotJustAFallback(t *testing.T) { + var gotMaxTokens int + client := &mockClient{ + respond: func(req llm.Request) (llm.Response, error) { + gotMaxTokens = req.MaxTokens + return llm.Response{Text: "OK"}, nil + }, + } + r := New(client, Config{Concurrency: 1, MaxTokensDefault: 6000}) + + r.Run(context.Background(), []string{"model-a"}, []testkit.Test{ + {ID: "t1", Category: "c", Prompt: "p", MaxTokens: 200, Eval: echoEval()}, + }) + + if gotMaxTokens != 6000 { + t.Errorf("MaxTokens = %d, want 6000 (config default is a floor; a smaller per-test 200 must not lower it)", gotMaxTokens) + } +} + +func TestRunner_Run_TracksFinishReasonAndResponseText(t *testing.T) { + client := &mockClient{ + respond: func(_ llm.Request) (llm.Response, error) { + return llm.Response{Text: " OK ", FinishReason: "stop"}, nil + }, + } + r := New(client, Config{Concurrency: 1, MaxTokensDefault: 100}) + + results := r.Run(context.Background(), []string{"model-a"}, []testkit.Test{ + {ID: "t1", Category: "c", Prompt: "p", Eval: echoEval()}, + }) + + if results[0].FinishReason != "stop" { + t.Errorf("FinishReason = %q, want stop", results[0].FinishReason) + } + if results[0].ResponseText != "OK" { + t.Errorf("ResponseText = %q, want the normalized \"OK\"", results[0].ResponseText) + } + if results[0].Truncated() { + t.Error("Truncated() = true, want false for finish_reason=stop") + } +} + +// TestRunner_Run_TruncatedPrefixesScoreDetail is the regression test for +// fix 4: a finish_reason=length response must be visibly flagged in +// Score.Detail, not just silently scored. +func TestRunner_Run_TruncatedPrefixesScoreDetail(t *testing.T) { + client := &mockClient{ + respond: func(_ llm.Request) (llm.Response, error) { + return llm.Response{Text: "partial", FinishReason: llm.FinishReasonLength}, nil + }, + } + r := New(client, Config{Concurrency: 1, MaxTokensDefault: 100}) + + results := r.Run(context.Background(), []string{"model-a"}, []testkit.Test{ + {ID: "t1", Category: "c", Prompt: "p", Eval: echoEval()}, + }) + + if !results[0].Truncated() { + t.Fatal("Truncated() = false, want true for finish_reason=length") + } + if !strings.HasPrefix(results[0].Score.Detail, "TRUNCATED: ") { + t.Errorf("Score.Detail = %q, want a TRUNCATED: prefix", results[0].Score.Detail) + } +} + func TestRunner_Run_IncludesSystemMessageWhenSet(t *testing.T) { var gotMessages []llm.Message client := &mockClient{ @@ -199,6 +269,89 @@ func TestRunner_Run_IncludesSystemMessageWhenSet(t *testing.T) { } } +// recordingReporter is a ProgressReporter that records every event under a +// mutex, so tests can assert on it after a concurrent Run. +type recordingReporter struct { + doneEvents []doneEvent + startCalls int + startTotalTests int + startTotalModels int + startConcurrency int + mu sync.Mutex +} + +type doneEvent struct { + testID string + done int + total int +} + +func (r *recordingReporter) ReportStart(totalTests, totalModels, concurrency int) { + r.mu.Lock() + defer r.mu.Unlock() + r.startCalls++ + r.startTotalTests = totalTests + r.startTotalModels = totalModels + r.startConcurrency = concurrency +} + +func (r *recordingReporter) ReportDone(done, total int, result Result) { + r.mu.Lock() + defer r.mu.Unlock() + r.doneEvents = append(r.doneEvents, doneEvent{done: done, total: total, testID: result.TestID}) +} + +func TestRunner_Run_ReportsProgress(t *testing.T) { + client := &mockClient{} + reporter := &recordingReporter{} + r := New(client, Config{Concurrency: 3, MaxTokensDefault: 100, Reporter: reporter}) + + tests := []testkit.Test{ + {ID: "t1", Category: "c", Prompt: "p", Eval: echoEval()}, + {ID: "t2", Category: "c", Prompt: "p", Eval: echoEval()}, + } + models := []string{"model-a", "model-b", "model-c"} + + r.Run(context.Background(), models, tests) + + if reporter.startCalls != 1 { + t.Errorf("ReportStart called %d times, want 1", reporter.startCalls) + } + if reporter.startTotalTests != len(tests) || reporter.startTotalModels != len(models) || reporter.startConcurrency != 3 { + t.Errorf("ReportStart(totalTests=%d, totalModels=%d, concurrency=%d), want (%d, %d, 3)", + reporter.startTotalTests, reporter.startTotalModels, reporter.startConcurrency, len(tests), len(models)) + } + + total := len(tests) * len(models) + if len(reporter.doneEvents) != total { + t.Fatalf("ReportDone called %d times, want %d", len(reporter.doneEvents), total) + } + + seenDone := make(map[int]bool, total) + for _, ev := range reporter.doneEvents { + if ev.total != total { + t.Errorf("ReportDone total = %d, want %d", ev.total, total) + } + if ev.done < 1 || ev.done > total { + t.Errorf("ReportDone done = %d, want in [1, %d]", ev.done, total) + } + if seenDone[ev.done] { + t.Errorf("ReportDone done=%d reported more than once: the completion counter is not concurrency-safe", ev.done) + } + seenDone[ev.done] = true + } +} + +func TestRunner_Run_NilReporterDefaultsToNoop(t *testing.T) { + client := &mockClient{} + r := New(client, Config{Concurrency: 2, MaxTokensDefault: 100}) + + // Must not panic when Config.Reporter is left nil. + r.Run(context.Background(), []string{"model-a"}, []testkit.Test{ + {ID: "t1", Category: "c", Prompt: "p", Eval: echoEval()}, + }) +} + func TestRunner_Run_TracksLatencyAndTokens(t *testing.T) { client := &mockClient{ respond: func(_ llm.Request) (llm.Response, error) { diff --git a/internal/tests/catalog_test.go b/internal/tests/catalog_test.go index 5eef4fd..bbe2e2c 100644 --- a/internal/tests/catalog_test.go +++ b/internal/tests/catalog_test.go @@ -2,60 +2,53 @@ package tests import "testing" -// wantCatalog is the full 17-test catalog per PLAN.md, id -> {category, -// subcategory}. TestAll_MatchesCatalog checks the registry against this -// list exactly, so an accidental removal, duplication, or miscategorized -// test fails loudly. -var wantCatalog = map[string][2]string{ - "go-struct-align": {"programming", "golang"}, - "go-worker-pool": {"programming", "golang"}, - "go-semver-classify": {"programming", "golang"}, - "py-log-triage": {"programming", "python"}, - "py-cosine": {"programming", "python"}, - "ts-debounce-composable": {"programming", "typescript"}, - "c-struct-size": {"programming", "c"}, - "macos-timeout-portability": {"operations", "macos"}, - "macos-launchd-cron": {"operations", "macos"}, - "linux-pct-exec": {"operations", "linux"}, - "linux-systemd-oneshot": {"operations", "linux"}, - "k8s-crashloop-gitops": {"operations", "kubernetes"}, - "web-robots-ai-crawlers": {"research", "web"}, - "paper-hnsw-params": {"research", "whitepapers"}, - "code-trace-go": {"research", "codebase"}, - "agent-tool-routing": {"agents", "tool-routing"}, - "agent-plan-ordering": {"agents", "planning"}, -} - -func TestAll_MatchesCatalog(t *testing.T) { +// TestAll_Invariants checks structural invariants across the whole +// catalog, rather than an exact id/category/count map (fix 6): four other +// authors are adding roughly 113 more tests to internal/tests in parallel +// worktrees, so a fixed catalog size or an exhaustive id list would break +// on every integration merge for reasons unrelated to a real bug. An +// exact-catalog check belongs at integration time, once the merge is +// final, not in a file every parallel author's branch also compiles. +func TestAll_Invariants(t *testing.T) { r := All() + all := r.All() - if r.Len() != len(wantCatalog) { - t.Fatalf("registry has %d tests, want %d", r.Len(), len(wantCatalog)) + if len(all) == 0 { + t.Fatal("catalog is empty") } - for id, catSub := range wantCatalog { - tc, ok := r.Get(id) - if !ok { - t.Errorf("missing test %q", id) + seenIDs := make(map[string]bool, len(all)) + for _, tc := range all { + if tc.ID == "" { + t.Error("found a test with an empty ID") continue } - if tc.Category != catSub[0] { - t.Errorf("%s: Category = %q, want %q", id, tc.Category, catSub[0]) + if seenIDs[tc.ID] { + t.Errorf("duplicate test ID %q", tc.ID) } - if tc.Subcategory != catSub[1] { - t.Errorf("%s: Subcategory = %q, want %q", id, tc.Subcategory, catSub[1]) + seenIDs[tc.ID] = true + + if tc.Category == "" { + t.Errorf("%s: Category is empty", tc.ID) + } + if tc.Subcategory == "" { + t.Errorf("%s: Subcategory is empty", tc.ID) } if tc.Prompt == "" { - t.Errorf("%s: Prompt is empty", id) - } - if tc.Eval == nil { - t.Errorf("%s: Eval is nil", id) + t.Errorf("%s: Prompt is empty", tc.ID) } if tc.Description == "" { - t.Errorf("%s: Description is empty", id) + t.Errorf("%s: Description is empty", tc.ID) } - if tc.MaxTokens <= 0 { - t.Errorf("%s: MaxTokens = %d, want > 0", id, tc.MaxTokens) + if tc.Eval == nil { + t.Errorf("%s: Eval is nil", tc.ID) + } + // MaxTokens == 0 is valid: runner.Runner floors it with + // cfg.MaxTokensDefault (max(test.MaxTokens, cfg.MaxTokensDefault)), + // so a test need not set its own MaxTokens at all. Only a + // negative value is a bug. + if tc.MaxTokens < 0 { + t.Errorf("%s: MaxTokens = %d, want >= 0", tc.ID, tc.MaxTokens) } } } diff --git a/internal/tests/golang.go b/internal/tests/golang.go index e22c3d4..2d96c91 100644 --- a/internal/tests/golang.go +++ b/internal/tests/golang.go @@ -86,8 +86,11 @@ Respond with the corrected struct definition only.` // response's Pool[T, R any] implementation (defined in solution.go, same // package) with a deliberately slow worker function and checks (a) output // order matches input order and (b) the pool actually parallelizes work, -// via both an observed-concurrency counter and a wall-clock bound far -// below the fully-sequential time. +// via an observed-concurrency counter (maxConcurrent >= 2). There is +// deliberately no wall-clock bound (5e): a CI runner under load can make +// even genuinely concurrent work take longer than any fixed millisecond +// budget, and maxConcurrent >= 2 already proves real parallelism without +// that flake risk. const goWorkerPoolHarness = `package main import ( @@ -122,9 +125,7 @@ func main() { return x * 2 } - start := time.Now() out := Pool(in, workers, f) - elapsed := time.Since(start) if len(out) != n { fmt.Fprintf(os.Stderr, "len(out)=%d, want %d\n", len(out), n) @@ -140,12 +141,6 @@ func main() { fmt.Fprintf(os.Stderr, "maxConcurrent=%d, want >= 2: no real concurrency observed\n", maxConcurrent) os.Exit(1) } - // Sequential execution would take n*perItem = 200ms; with workers - // goroutines truly running in parallel it should land well under that. - if elapsed > 120*time.Millisecond { - fmt.Fprintf(os.Stderr, "elapsed=%s, want < 120ms: looks sequential\n", elapsed) - os.Exit(1) - } fmt.Println("PASS") } diff --git a/internal/tests/golang_test.go b/internal/tests/golang_test.go index 525e9a4..7842fa4 100644 --- a/internal/tests/golang_test.go +++ b/internal/tests/golang_test.go @@ -119,7 +119,7 @@ func TestGoWorkerPoolTest_Eval(t *testing.T) { want float64 }{ {"correct concurrent pool", correctPool, 1}, - {"sequential pool fails concurrency/timing check", sequentialPool, 0}, + {"sequential pool fails the maxConcurrent>=2 check", sequentialPool, 0}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/tests/kubernetes.go b/internal/tests/kubernetes.go index 327e4a9..b8c3e80 100644 --- a/internal/tests/kubernetes.go +++ b/internal/tests/kubernetes.go @@ -56,37 +56,59 @@ func mentionsLimitIncrease() eval.Evaluator { } // kubectlLiveMutationPattern matches any mention of "kubectl edit" or -// "kubectl patch", negated or not. +// "kubectl patch", negated or not - whether mid-sentence, at a bare line +// start, or as a bulleted list item. var kubectlLiveMutationPattern = regexp.MustCompile(`(?i)kubectl\s+(edit|patch)\b`) -// kubectlImperativeMutationPattern matches a bare/imperative "kubectl -// edit"/"kubectl patch" at the start of a line (optionally after -// whitespace or a bullet marker) - a command a reader would actually run, -// never acceptable regardless of negation cues elsewhere in the response. -var kubectlImperativeMutationPattern = regexp.MustCompile(`(?im)^\s*(?:[-*]\s*)?kubectl\s+(edit|patch)\b`) - // negationCuePattern matches a word that turns a mention of the forbidden // command into a warning against running it, rather than an instruction to // run it (e.g. "do not run kubectl patch..."). -var negationCuePattern = regexp.MustCompile(`(?i)\b(don'?t|do not|never|avoid|instead of|not)\b`) +var negationCuePattern = regexp.MustCompile(`(?i)\b(don'?t|do not|never|avoid|instead of|not|cannot|can'?t|rather than|without|no need)\b`) // negationWindow is how many characters before a "kubectl edit|patch" -// mention (never crossing a newline) are searched for a negation cue. +// mention are searched for a negation cue. const negationWindow = 60 +// negationWindowStart returns the earliest byte offset in response to +// search for a negation cue before a "kubectl edit|patch" match starting +// at start. The window is the current line, extended back to the start of +// the immediately preceding line when that line is non-empty: a +// hard-wrapped sentence whose negation cue landed on the line above (e.g. +// "...do not run\nkubectl edit...") would otherwise score 0 even though it +// is the correct, negated answer. Either way, the window never reaches +// more than negationWindow characters before start. +func negationWindowStart(response string, start int) int { + curLineStart := strings.LastIndexByte(response[:start], '\n') + 1 + + windowFloor := curLineStart + if curLineStart > 0 { + prevLineEnd := curLineStart - 1 // the newline separating the two lines + prevLineStart := strings.LastIndexByte(response[:prevLineEnd], '\n') + 1 + if strings.TrimSpace(response[prevLineStart:prevLineEnd]) != "" { + windowFloor = prevLineStart + } + } + + return max(start-negationWindow, windowFloor) +} + // noLiveKubectlMutation scores full credit unless the response instructs // running "kubectl edit"/"kubectl patch" against the live cluster. A // contrastive mention - warning the reader not to do this, in favor of the -// GitOps procedure - is fine and does not cost credit; an imperative or -// otherwise unnegated mention scores zero. This is deliberately not -// eval.NotContains, which would also zero out the best possible answer -// (one that correctly explains why NOT to run kubectl edit/patch). +// GitOps procedure - is fine and does not cost credit; an unnegated +// mention scores zero. This is deliberately not eval.NotContains, which +// would also zero out the best possible answer (one that correctly +// explains why NOT to run kubectl edit/patch). +// +// Every occurrence goes through the same negation-window check, including +// one at the start of a line (a bulleted "don't do this" list item, or a +// hard-wrapped sentence whose negation cue landed on the line above): a +// response is not penalized just because word-wrap or list formatting put +// the command text at a line start. A genuinely bare imperative still +// fails, because negationWindowStart never finds a negation cue in its +// (possibly line-extended) window. func noLiveKubectlMutation() eval.Evaluator { return eval.EvaluatorFunc(func(_ context.Context, response string) eval.Score { - if kubectlImperativeMutationPattern.MatchString(response) { - return eval.Score{Value: 0, Detail: "imperative kubectl edit/patch command present"} - } - matches := kubectlLiveMutationPattern.FindAllStringIndex(response, -1) if len(matches) == 0 { return eval.Score{Value: 1, Detail: "no mention of kubectl edit/patch"} @@ -94,11 +116,7 @@ func noLiveKubectlMutation() eval.Evaluator { for _, loc := range matches { start := loc[0] - lineStart := strings.LastIndexByte(response[:start], '\n') + 1 - windowStart := start - negationWindow - if windowStart < lineStart { - windowStart = lineStart - } + windowStart := negationWindowStart(response, start) if !negationCuePattern.MatchString(response[windowStart:start]) { return eval.Score{Value: 0, Detail: "unnegated mention of kubectl edit/patch"} } diff --git a/internal/tests/kubernetes_test.go b/internal/tests/kubernetes_test.go index 7182593..76eb900 100644 --- a/internal/tests/kubernetes_test.go +++ b/internal/tests/kubernetes_test.go @@ -59,6 +59,50 @@ func TestNoLiveKubectlMutation(t *testing.T) { response: "Never mind the manifest for now, but you could just casually run a quick kubectl patch to fix the limit right away.", want: 0, }, + { + // Opus round 2, 5a: extending the negation window to the + // previous line (and routing every occurrence, including a + // line-start one, through that same window) fixes this - it + // used to score 0 because "kubectl edit" landed at a hard-wrap + // line start and the old code hard-vetoed any line-start + // occurrence regardless of the negation on the line above. + name: "hard-wrapped negation: cue on the previous line", + response: "Since this is GitOps-managed, you must not\nkubectl edit the deployment directly - instead commit the fix to git.", + want: 1, + }, + { + // Single bullet, not a multi-item list: the window extends back + // exactly one line, so the negation cue must be on the line + // immediately above the occurrence, not several lines back. + name: "hard-wrapped negation: single bulleted don't-do-this item", + response: "Do not run this:\n- kubectl edit deployment inventory-worker\nInstead, edit the manifest in git and commit.", + want: 1, + }, + { + name: "new negation cue: cannot", + response: "You cannot safely run kubectl patch against a GitOps-managed cluster.", + want: 1, + }, + { + name: "new negation cue: rather than", + response: "Commit the fix to git rather than running kubectl edit on the live object.", + want: 1, + }, + { + name: "new negation cue: without", + response: "Fix this without running kubectl patch against the live cluster.", + want: 1, + }, + { + name: "new negation cue: no need", + response: "There is no need to run kubectl edit here; commit the change to git instead.", + want: 1, + }, + { + name: "blank line still breaks the extension: unrelated previous paragraph", + response: "Here is some unrelated context that never mentions the fix.\n\nkubectl patch deployment inventory-worker --patch '...'", + want: 0, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/tests/structlayout.go b/internal/tests/structlayout.go index 7bd0110..bed0ad5 100644 --- a/internal/tests/structlayout.go +++ b/internal/tests/structlayout.go @@ -56,8 +56,11 @@ func computeLP64StructSize(fieldTypes []string) (uintptr, error) { // structAlignFieldPattern matches one "Name Type" struct field declaration // line for the specific 5 fields goStructAlignTest's prompt gives (Active // bool, ID int64, Priority int32, Name string, Flag byte), in whatever -// order the response declares them. -var structAlignFieldPattern = regexp.MustCompile(`(?m)^\s*(Active|ID|Priority|Name|Flag)\s+(bool|int64|int32|string|byte)\s*$`) +// order the response declares them. An optional Go struct tag +// ("`json:\"name\"`") and/or an optional trailing "// comment" (e.g. "Name +// string // 16 bytes", a model explaining its layout reasoning inline) are +// both tolerated between the type and the end of the line (5c). +var structAlignFieldPattern = regexp.MustCompile("(?m)^\\s*(Active|ID|Priority|Name|Flag)\\s+(bool|int64|int32|string|byte)(?:\\s+`[^`]*`)?\\s*(?://.*)?$") // structAlignWantFields is the required field name -> type mapping; // goStructAlignEval rejects a response that renames, retypes, drops, or diff --git a/internal/tests/structlayout_test.go b/internal/tests/structlayout_test.go index 3532d10..186b6d3 100644 --- a/internal/tests/structlayout_test.go +++ b/internal/tests/structlayout_test.go @@ -95,6 +95,48 @@ func TestGoStructAlignEval_AllFourMinimalOrdersScoreFull(t *testing.T) { } } +// TestGoStructAlignEval_TrailingCommentsAndTags is the 5c regression: a +// field line may carry a trailing "// comment" explaining the model's +// reasoning, and/or a Go struct tag, without breaking field extraction. +func TestGoStructAlignEval_TrailingCommentsAndTags(t *testing.T) { + e := goStructAlignEval() + + tests := []struct { + name string + response string + want float64 + }{ + { + name: "trailing comment on every field", + response: "```go\ntype Task struct {\n\tName string // 16 bytes\n\tID int64 // 8 bytes\n\tPriority int32 // 4 bytes\n\tActive bool // 1 byte\n\tFlag byte // 1 byte\n}\n```", + want: 1, + }, + { + name: "struct tag with no comment", + response: "```go\ntype Task struct {\n\tName string `json:\"name\"`\n\tID int64 `json:\"id\"`\n\tPriority int32 `json:\"priority\"`\n\tActive bool `json:\"active\"`\n\tFlag byte `json:\"flag\"`\n}\n```", + want: 1, + }, + { + name: "struct tag followed by a comment", + response: "```go\ntype Task struct {\n\tName string `json:\"name\"` // 16 bytes\n\tID int64 `json:\"id\"`\n\tPriority int32 `json:\"priority\"`\n\tActive bool `json:\"active\"`\n\tFlag byte `json:\"flag\"`\n}\n```", + want: 1, + }, + { + name: "wasteful order with comments still scores 0", + response: "```go\ntype Task struct {\n\tActive bool // 1 byte\n\tID int64 // 8 bytes\n\tPriority int32 // 4 bytes\n\tName string // 16 bytes\n\tFlag byte // 1 byte\n}\n```", + want: 0, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := e.Evaluate(context.Background(), tt.response) + if got.Value != tt.want { + t.Errorf("Evaluate() = %v, want %v (detail: %s)", got.Value, tt.want, got.Detail) + } + }) + } +} + func TestGoStructAlignEval_RejectsBadResponses(t *testing.T) { e := goStructAlignEval()