diff --git a/internal/helpers/sheet_revision.go b/internal/helpers/sheet_revision.go index 703f4f43..434aec83 100644 --- a/internal/helpers/sheet_revision.go +++ b/internal/helpers/sheet_revision.go @@ -367,7 +367,11 @@ func decodeSheetRevisionResult(tool, raw string) (any, error) { } data, err := decodeSheetSingleJSON(raw) if err != nil { - return nil, apperrors.NewInternal(fmt.Sprintf("解析 %s 返回失败: %v", tool, err)) + return nil, invalidSheetRevisionResponse(tool, + fmt.Sprintf("MCP sheet read tool returned invalid JSON: %v", err), + "invalid_tool_response", + false, + ) } object, ok := data.(map[string]any) if !ok { @@ -395,7 +399,12 @@ func decodeSheetRevisionResult(tool, raw string) (any, error) { } if tool == sheetChangesetGetRemoteTool { if err := normalizeSheetChangesetTransport(object); err != nil { - return nil, apperrors.NewInternal(fmt.Sprintf("解析 %s 返回失败: %v%s", tool, err, sheetResultLogIDSuffix(object))) + return nil, invalidSheetRevisionResponse(tool, + fmt.Sprintf("MCP sheet read tool returned invalid changeset data: %v%s", + err, sheetResultLogIDSuffix(object)), + "invalid_tool_response", + false, + ) } } if err := validateSheetPublishedResult(tool, object); err != nil { @@ -493,7 +502,12 @@ func validateSheetPublishedResult(tool string, object map[string]any) error { default: return fmt.Errorf("未知工具 %q", tool) } + return validateSheetPublishedResultWithSchema(tool, object, rawSchema, cache) +} +func validateSheetPublishedResultWithSchema(tool string, object map[string]any, + rawSchema json.RawMessage, cache *sheetPublishedResultSchemaCache, +) error { cache.once.Do(func() { cache.err = json.Unmarshal(rawSchema, &cache.schema) }) if cache.err != nil { return fmt.Errorf("读取已发布 Result Schema 失败: %v", cache.err) diff --git a/internal/helpers/sheet_revision_test.go b/internal/helpers/sheet_revision_test.go index 56d18807..93d36127 100644 --- a/internal/helpers/sheet_revision_test.go +++ b/internal/helpers/sheet_revision_test.go @@ -260,11 +260,37 @@ func TestDecodeSheetChangesetTransportRejectsInvalidJSONStrings(t *testing.T) { } } -func TestDecodeSheetChangesetTransportPreservesLogIDInParseError(t *testing.T) { - const response = `{"success":true,"logId":"trace-malformed-changeset","changesetsJson":"not-json"}` - _, err := decodeSheetRevisionResult(sheetChangesetGetRemoteTool, response) - if err == nil || !strings.Contains(err.Error(), "logId=trace-malformed-changeset") { - t.Fatalf("parse error = %v, want backend logId", err) +func TestDecodeSheetRevisionProtocolFailuresHaveMCPResponseMetadata(t *testing.T) { + tests := []struct { + name string + tool string + response string + wantLogID string + }{ + { + name: "malformed outer json", tool: sheetRevisionGetRemoteTool, + response: `{"success":true`, + }, + { + name: "malformed changesets json", tool: sheetChangesetGetRemoteTool, + response: `{"success":true,"logId":"trace-malformed-changeset","changesetsJson":"not-json"}`, + wantLogID: "trace-malformed-changeset", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, err := decodeSheetRevisionResult(test.tool, test.response) + var apiErr *apperrors.Error + if !errors.As(err, &apiErr) || apiErr.Origin != "mcp" || + apiErr.FailureStage != "response_validation" || apiErr.Reason != "invalid_tool_response" || + apiErr.Operation != "sheet/"+test.tool || !apiErr.RetryableSet || apiErr.Retryable { + t.Fatalf("protocol failure error = %#v", err) + } + if test.wantLogID != "" && !strings.Contains(err.Error(), "logId="+test.wantLogID) { + t.Fatalf("protocol failure error = %v, want backend logId", err) + } + }) } } @@ -358,6 +384,11 @@ func TestSheetRevisionCommandsRejectPublishedResultContractDrift(t *testing.T) { response: `{"success":true,"logId":" ","revision":1}`, build: newSheetRevisionGetCmd, args: []string{"--node", "node-1"}, }, + { + name: "revision negative integer", tool: sheetRevisionGetRemoteTool, + response: `{"success":true,"logId":"trace-contract","revision":-1}`, + build: newSheetRevisionGetCmd, args: []string{"--node", "node-1"}, + }, { name: "changeset missing summary", tool: sheetChangesetGetRemoteTool, response: encodeSheetChangesetTransport(t, `{`+validChangesetPrefix+validChangesets+`}`), @@ -383,6 +414,31 @@ func TestSheetRevisionCommandsRejectPublishedResultContractDrift(t *testing.T) { response: encodeSheetChangesetTransport(t, `{`+validChangesetPrefix+`"summary":{"changeCount":0},`+validChangesets+`}`), build: newSheetChangesetGetCmd, args: []string{"--node", "node-1", "--start-revision", "0"}, }, + { + name: "changeset negative latest revision", tool: sheetChangesetGetRemoteTool, + response: encodeSheetChangesetTransport(t, `{"success":true,"logId":"trace-contract","schemaVersion":2,"changeSemantics":"FORWARD_ONLY","latestRevision":-1,"startRevision":0,"endRevision":0,`+validSummary+`,`+validChangesets+`}`), + build: newSheetChangesetGetCmd, args: []string{"--node", "node-1", "--start-revision", "0"}, + }, + { + name: "changeset negative start revision", tool: sheetChangesetGetRemoteTool, + response: encodeSheetChangesetTransport(t, `{"success":true,"logId":"trace-contract","schemaVersion":2,"changeSemantics":"FORWARD_ONLY","latestRevision":1,"startRevision":-1,"endRevision":0,`+validSummary+`,`+validChangesets+`}`), + build: newSheetChangesetGetCmd, args: []string{"--node", "node-1", "--start-revision", "0"}, + }, + { + name: "changeset negative end revision", tool: sheetChangesetGetRemoteTool, + response: encodeSheetChangesetTransport(t, `{"success":true,"logId":"trace-contract","schemaVersion":2,"changeSemantics":"FORWARD_ONLY","latestRevision":1,"startRevision":0,"endRevision":-1,`+validSummary+`,`+validChangesets+`}`), + build: newSheetChangesetGetCmd, args: []string{"--node", "node-1", "--start-revision", "0"}, + }, + { + name: "changeset start after end", tool: sheetChangesetGetRemoteTool, + response: encodeSheetChangesetTransport(t, `{"success":true,"logId":"trace-contract","schemaVersion":2,"changeSemantics":"FORWARD_ONLY","latestRevision":2,"startRevision":2,"endRevision":1,`+validSummary+`,`+validChangesets+`}`), + build: newSheetChangesetGetCmd, args: []string{"--node", "node-1", "--start-revision", "0"}, + }, + { + name: "changeset end after latest", tool: sheetChangesetGetRemoteTool, + response: encodeSheetChangesetTransport(t, `{"success":true,"logId":"trace-contract","schemaVersion":2,"changeSemantics":"FORWARD_ONLY","latestRevision":1,"startRevision":0,"endRevision":2,`+validSummary+`,`+validChangesets+`}`), + build: newSheetChangesetGetCmd, args: []string{"--node", "node-1", "--start-revision", "0"}, + }, { name: "changeset item missing required field", tool: sheetChangesetGetRemoteTool, response: encodeSheetChangesetTransport(t, `{`+validChangesetPrefix+validSummary+`,"changesets":[{"revision":1,"isSelfEdit":true,"eventType":"EDIT","detailsStatus":"COMPLETE","changes":[]}]}`), @@ -409,6 +465,22 @@ func TestSheetRevisionCommandsRejectPublishedResultContractDrift(t *testing.T) { } } +func TestValidateSheetPublishedResultCoversDefensiveFailures(t *testing.T) { + if err := validateSheetPublishedResult("unknown-tool", map[string]any{}); err == nil { + t.Fatal("unknown tool unexpectedly accepted") + } + + cache := &sheetPublishedResultSchemaCache{} + if err := validateSheetPublishedResultWithSchema(sheetRevisionGetRemoteTool, map[string]any{}, + json.RawMessage(`{`), cache); err == nil { + t.Fatal("invalid published schema unexpectedly accepted") + } + + if _, err := sheetNonNegativeInteger(int64(1)); err == nil { + t.Fatal("non-json number unexpectedly accepted") + } +} + func TestDecodeSheetRevisionEmptyResponseHasStructuredRetryableError(t *testing.T) { _, err := decodeSheetRevisionResult(sheetRevisionGetRemoteTool, " \n ") var apiErr *apperrors.Error