diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-07-11 09:37:31 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-07-11 09:37:31 +0000 |
| commit | 027536520409757ba38cd4c78e07b2ca0797a2d7 (patch) | |
| tree | dc0671430ede3e0adf80d6640ed62c7d7c979264 /internal/executor/classifier_test.go | |
| parent | 7978760316319d22670cd6369c15b68c649761bc (diff) | |
fix(executor): validate the model classifier's output against a known model list
The Gemini-based Classifier trusted its LLM output verbatim. Twice in
production it echoed part of its own prompt's JSON schema literally --
'model-name' (the prompt's own placeholder, also fixed here to be less
echo-prone) and separately 'choose-the-best-model' -- instead of
substituting a real model identifier. Both were syntactically valid JSON
strings that passed straight through to the claude CLI's --model flag,
which rejected them and failed the task outright. validateClassification
now rejects anything outside the known model list, which Classify's
existing error handling already treats as 'classification failed' and
falls back to no explicit model override -- the same documented fallback
path, now also covering 'succeeded but returned garbage.'
Diffstat (limited to 'internal/executor/classifier_test.go')
| -rw-r--r-- | internal/executor/classifier_test.go | 42 |
1 files changed, 42 insertions, 0 deletions
diff --git a/internal/executor/classifier_test.go b/internal/executor/classifier_test.go index 84fffcf..f0bb32f 100644 --- a/internal/executor/classifier_test.go +++ b/internal/executor/classifier_test.go @@ -112,6 +112,48 @@ func TestClassifier_LLMTakesPrecedence_OverGemini(t *testing.T) { } } +// TestClassifier_Classify_RejectsEchoedPlaceholder proves the 2026-07-11 fix: +// a gemini CLI response whose "model" field is the prompt's own literal +// placeholder text (or any other string outside the known model list) must +// be rejected as an error, not trusted as a real model identifier -- this is +// exactly what happened in production ("model-name" and separately +// "choose-the-best-model" both slipped through unvalidated and were passed +// straight to the claude CLI's --model flag, which rejected them outright). +func TestClassifier_Classify_RejectsEchoedPlaceholder(t *testing.T) { + mockBinary := filepathJoin(t.TempDir(), "mock-gemini") + mockContent := `#!/bin/sh +echo '{"response": "{\"agent_type\": \"claude\", \"model\": \"model-name\", \"reason\": \"echoed the placeholder\"}"}' +` + if err := os.WriteFile(mockBinary, []byte(mockContent), 0755); err != nil { + t.Fatal(err) + } + + c := &Classifier{GeminiBinaryPath: mockBinary} + _, err := c.Classify(context.Background(), "Test Task", "Test Instructions", SystemStatus{}, "claude") + if err == nil { + t.Fatal("expected an error for an unrecognized model, got nil") + } + if !strings.Contains(err.Error(), "model-name") { + t.Errorf("expected error to name the bad model value, got: %v", err) + } +} + +// TestClassifier_ClassifyViaLLM_RejectsUnrecognizedModel mirrors the above +// for the local-LLM path, which is equally capable of echoing garbage. +func TestClassifier_ClassifyViaLLM_RejectsUnrecognizedModel(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintln(w, `{"model":"x","choices":[{"message":{"content":"{\"agent_type\":\"claude\",\"model\":\"choose-the-best-model\",\"reason\":\"r\"}"},"finish_reason":"stop"}],"usage":{}}`) + })) + defer srv.Close() + + c := &Classifier{LLM: &llm.Client{Endpoint: srv.URL + "/v1", Model: "x"}} + _, err := c.Classify(context.Background(), "n", "i", SystemStatus{}, "claude") + if err == nil { + t.Fatal("expected an error for an unrecognized model, got nil") + } +} + func filepathJoin(elems ...string) string { var path string for i, e := range elems { |
