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 | |
| 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.'
| -rw-r--r-- | internal/executor/classifier.go | 45 | ||||
| -rw-r--r-- | internal/executor/classifier_test.go | 42 |
2 files changed, 86 insertions, 1 deletions
diff --git a/internal/executor/classifier.go b/internal/executor/classifier.go index 049dc4f..05ed021 100644 --- a/internal/executor/classifier.go +++ b/internal/executor/classifier.go @@ -59,11 +59,48 @@ Instructions: %s Respond with ONLY a JSON object: { "agent_type": "%s", - "model": "model-name", + "model": "<one of the exact model identifiers listed above, verbatim>", "reason": "brief reason" } ` +// validModels are the exact model identifiers classificationPrompt lists as +// valid choices. Classify's and classifyViaLLM's output is validated +// against this set before being trusted -- see validateClassification's +// doc comment for why this exists. +var validModels = map[string]bool{ + "claude-sonnet-4-6": true, + "claude-opus-4-6": true, + "claude-haiku-4-5-20251001": true, + "gemini-2.5-flash-lite": true, + "gemini-2.5-flash": true, + "gemini-2.5-pro": true, +} + +// validateClassification rejects a Classification whose Model isn't one of +// the exact identifiers classificationPrompt actually lists. Added +// 2026-07-11 after two real production failures where the underlying LLM +// echoed part of the prompt's own JSON schema literally -- "model-name" +// (this prompt's own placeholder text at the time, since fixed to be less +// echo-prone) and, separately, "choose-the-best-model" -- instead of +// substituting a real identifier. Both were syntactically valid JSON +// strings, so nothing before this caught them; the claude CLI then rejected +// the literal string as an invalid --model argument and the task failed +// outright, with no automatic recovery until internal/scheduler's +// retryWithoutLadder was added for the retry side of this same incident. +// Rejecting here makes Classify return an error, which callers already +// treat as "classification failed" and fall back to dispatching with no +// explicit model override (see internal/executor/executor.go's Pool.execute, +// which only sets t.Agent.Model when err == nil) -- exactly the documented +// "falls back to the default model if Gemini fails" behavior, now also +// covering "Gemini succeeded but returned garbage." +func validateClassification(cls *Classification) error { + if !validModels[cls.Model] { + return fmt.Errorf("classifier returned an unrecognized model %q -- not one of the known model identifiers, likely echoed the prompt's own example text instead of substituting a real one", cls.Model) + } + return nil +} + func (c *Classifier) Classify(ctx context.Context, taskName, instructions string, _ SystemStatus, agentType string) (*Classification, error) { prompt := fmt.Sprintf(classificationPrompt, agentType, taskName, instructions, agentType, @@ -131,6 +168,9 @@ func (c *Classifier) Classify(ctx context.Context, taskName, instructions string if err := json.Unmarshal([]byte(cleanOut), &cls); err != nil { return nil, fmt.Errorf("failed to parse classification JSON: %w\nOriginal Output: %s\nCleaned Output: %s", err, string(out), cleanOut) } + if err := validateClassification(&cls); err != nil { + return nil, err + } return &cls, nil } @@ -154,5 +194,8 @@ func (c *Classifier) classifyViaLLM(ctx context.Context, prompt, agentType strin if cls.AgentType == "" { cls.AgentType = agentType } + if err := validateClassification(&cls); err != nil { + return nil, err + } return &cls, nil } 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 { |
