diff options
| -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 { |