diff --git a/Sources/SwiftLM/Server.swift b/Sources/SwiftLM/Server.swift index 9aeef3c..e5cd29a 100644 --- a/Sources/SwiftLM/Server.swift +++ b/Sources/SwiftLM/Server.swift @@ -2405,7 +2405,12 @@ func handleChatCompletion( dflashTargetModel: (any DFlashTargetModel)? = nil, mtpAssistant: (any DualModelMTP)? = nil ) async throws -> Response { - let chatReq = try JSONDecoder().decode(ChatCompletionRequest.self, from: bodyData) + let chatReq: ChatCompletionRequest + do { + chatReq = try JSONDecoder().decode(ChatCompletionRequest.self, from: bodyData) + } catch let error as DecodingError { + return invalidRequestResponse(error) + } let isStream = chatReq.stream ?? false let jsonMode = chatReq.responseFormat?.type == "json_object" let emitPrefillProgress = prefillProgressEnabled(in: request) @@ -2417,7 +2422,7 @@ func handleChatCompletion( let topK = chatReq.topK ?? config.topK ?? 50 let minP = chatReq.minP.map(Float.init) ?? config.minP ?? 0.0 let repeatPenalty = chatReq.repetitionPenalty.map(Float.init) ?? config.repeatPenalty - let stopSequences = (chatReq.stop ?? []) + ["", "<|im_end|>", "<|eot_id|>", "", "<|tool_response|>"] + let stopSequences = (chatReq.stop?.values ?? []) + ["", "<|im_end|>", "<|eot_id|>", "", "<|tool_response|>"] let includeUsage = chatReq.streamOptions?.includeUsage ?? false // Log extra sampling params if provided (accepted for API compat, not all are used) @@ -3577,7 +3582,12 @@ func handleTextCompletion( semaphore: AsyncSemaphore, stats: ServerStats ) async throws -> Response { - let compReq = try JSONDecoder().decode(TextCompletionRequest.self, from: bodyData) + let compReq: TextCompletionRequest + do { + compReq = try JSONDecoder().decode(TextCompletionRequest.self, from: bodyData) + } catch let error as DecodingError { + return invalidRequestResponse(error) + } let isStream = compReq.stream ?? false let emitPrefillProgress = prefillProgressEnabled(in: request) @@ -3587,7 +3597,7 @@ func handleTextCompletion( let topK = compReq.topK ?? config.topK ?? 50 let minP = compReq.minP.map(Float.init) ?? config.minP ?? 0.0 let repeatPenalty = compReq.repetitionPenalty.map(Float.init) ?? config.repeatPenalty - let stopSequences = compReq.stop ?? [] + let stopSequences = compReq.stop?.values ?? [] var generateParams = GenerateParameters( maxTokens: tokenLimit, @@ -4142,6 +4152,51 @@ func rejectLongPrompt(promptTokens: Int, limit: Int?, slot: GenerationSlot, stat return Response(status: .badRequest, headers: jsonHeaders(), body: .init(byteBuffer: ByteBuffer(string: body))) } +/// The 400 for a request body that does not decode. OpenAI answers these with +/// `invalid_request_error` and names the offending `param`; the OpenAI SDKs treat a 400 +/// as final but retry a 5xx, so answering a decode failure with 500 `server_error` cost +/// every malformed request three attempts and told the client the server was at fault. +func invalidRequestResponse(_ error: DecodingError) -> Response { + Response( + status: .badRequest, + headers: jsonHeaders(), + body: .init(byteBuffer: ByteBuffer(string: invalidRequestJSON(error))) + ) +} + +func invalidRequestJSON(_ error: DecodingError) -> String { + func dotted(_ path: [CodingKey]) -> String { + path.map { $0.intValue.map(String.init) ?? $0.stringValue }.joined(separator: ".") + } + let message: String + let path: [CodingKey] + switch error { + case .keyNotFound(let key, let context): + path = context.codingPath + [key] + message = "Missing required parameter: '\(dotted(path))'." + case .typeMismatch(_, let context): + path = context.codingPath + message = "Invalid type for '\(dotted(path))': \(context.debugDescription)" + case .valueNotFound(_, let context): + path = context.codingPath + message = "Missing value for '\(dotted(path))'." + case .dataCorrupted(let context): + path = context.codingPath + message = context.debugDescription + @unknown default: + path = [] + message = String(describing: error) + } + var payload: [String: Any] = ["message": message, "type": "invalid_request_error"] + if !path.isEmpty { payload["param"] = dotted(path) } + guard let data = try? JSONSerialization.data(withJSONObject: ["error": payload]), + let json = String(data: data, encoding: .utf8) + else { + return "{\"error\":{\"message\":\"invalid request\",\"type\":\"invalid_request_error\"}}" + } + return json +} + /// Build an OpenAI-style `{"error":{...}}` body for a server error. The message /// is JSON-encoded, so quotes, backslashes and newlines in it stay valid JSON. func errorJSON(_ error: Error) -> String { @@ -4495,7 +4550,7 @@ struct ChatCompletionRequest: Decodable { let frequencyPenalty: Double? let presencePenalty: Double? let tools: [ToolDef]? - let stop: [String]? + let stop: StopSequences? let seed: Int? let streamOptions: StreamOptions? let responseFormat: ResponseFormat? @@ -4526,6 +4581,21 @@ struct ChatCompletionRequest: Decodable { } } +/// OpenAI's `stop` is "string / array / null": a single sequence may be sent bare, and +/// several clients do. Decoding only the array form answered those requests with a 500. +struct StopSequences: Decodable { + let values: [String] + + init(from decoder: Swift.Decoder) throws { + let container = try decoder.singleValueContainer() + if let single = try? container.decode(String.self) { + values = [single] + } else { + values = try container.decode([String].self) + } + } +} + struct TextCompletionRequest: Decodable { let model: String? let prompt: String @@ -4536,7 +4606,7 @@ struct TextCompletionRequest: Decodable { let topK: Int? let minP: Double? let repetitionPenalty: Double? - let stop: [String]? + let stop: StopSequences? let seed: Int? enum CodingKeys: String, CodingKey { diff --git a/tests/SwiftLMTests/RequestValidationTests.swift b/tests/SwiftLMTests/RequestValidationTests.swift new file mode 100644 index 0000000..26acf11 --- /dev/null +++ b/tests/SwiftLMTests/RequestValidationTests.swift @@ -0,0 +1,104 @@ +import XCTest +import Foundation +@testable import SwiftLM + +// MARK: - Request shapes the OpenAI API allows that the server used to reject +// +// `stop` is documented as "string / array / null". Both request types decoded only the +// array form, so `"stop": "\n"` — what several SDKs and llama.cpp-style clients send — +// failed to decode. And a decode failure was answered with a 500 `server_error`, which +// the OpenAI SDKs retry (they treat 5xx as transient), instead of the 400 +// `invalid_request_error` that tells a client its request is the problem. +final class RequestValidationTests: XCTestCase { + + private func decodeChat(_ json: String) throws -> ChatCompletionRequest { + try JSONDecoder().decode(ChatCompletionRequest.self, from: try XCTUnwrap(json.data(using: .utf8))) + } + + private func decodeText(_ json: String) throws -> TextCompletionRequest { + try JSONDecoder().decode(TextCompletionRequest.self, from: try XCTUnwrap(json.data(using: .utf8))) + } + + private let messages = #"[{"role":"user","content":"hi"}]"# + + // MARK: stop: string | [string] | null + + func testChatStopAcceptsAString() throws { + let req = try decodeChat(#"{"messages":\#(messages),"stop":"\n"}"#) + XCTAssertEqual(req.stop?.values, ["\n"]) + } + + func testChatStopStillAcceptsAnArray() throws { + let req = try decodeChat(#"{"messages":\#(messages),"stop":["a","b"]}"#) + XCTAssertEqual(req.stop?.values, ["a", "b"]) + } + + func testChatStopNullAndAbsentAreNil() throws { + XCTAssertNil(try decodeChat(#"{"messages":\#(messages),"stop":null}"#).stop) + XCTAssertNil(try decodeChat(#"{"messages":\#(messages)}"#).stop) + } + + func testTextCompletionStopAcceptsAString() throws { + let req = try decodeText(#"{"prompt":"x","stop":"<|end|>"}"#) + XCTAssertEqual(req.stop?.values, ["<|end|>"]) + } + + func testTextCompletionStopStillAcceptsAnArray() throws { + let req = try decodeText(#"{"prompt":"x","stop":["<|end|>","---"]}"#) + XCTAssertEqual(req.stop?.values, ["<|end|>", "---"]) + } + + /// Anything else is still a decode error — a number is not a stop sequence. + func testStopRejectsOtherTypes() { + XCTAssertThrowsError(try decodeChat(#"{"messages":\#(messages),"stop":42}"#)) { error in + XCTAssertTrue(error is DecodingError) + } + } + + // MARK: Decode failures become 400 invalid_request_error + + private func errorObject(_ json: String) throws -> [String: Any] { + let data = try XCTUnwrap(json.data(using: .utf8)) + let top = try XCTUnwrap(JSONSerialization.jsonObject(with: data) as? [String: Any]) + return try XCTUnwrap(top["error"] as? [String: Any]) + } + + func testTypeMismatchNamesTheParameter() throws { + let error = try captureDecodingError { try decodeChat(#"{"messages":\#(messages),"stop":42}"#) } + let payload = try errorObject(invalidRequestJSON(error)) + XCTAssertEqual(payload["type"] as? String, "invalid_request_error") + XCTAssertEqual(payload["param"] as? String, "stop") + XCTAssertTrue((payload["message"] as? String ?? "").contains("stop")) + } + + func testMissingKeyNamesTheParameter() throws { + let error = try captureDecodingError { try decodeChat(#"{"model":"x"}"#) } + let payload = try errorObject(invalidRequestJSON(error)) + XCTAssertEqual(payload["type"] as? String, "invalid_request_error") + XCTAssertEqual(payload["param"] as? String, "messages") + } + + func testNestedKeyPathIsDotted() throws { + let error = try captureDecodingError { try decodeChat(#"{"messages":[{"content":"hi"}]}"#) } + let payload = try errorObject(invalidRequestJSON(error)) + XCTAssertEqual(payload["param"] as? String, "messages.0.role") + } + + func testMalformedJSONHasNoParameter() throws { + let error = try captureDecodingError { try decodeChat("{not json") } + let payload = try errorObject(invalidRequestJSON(error)) + XCTAssertEqual(payload["type"] as? String, "invalid_request_error") + XCTAssertNil(payload["param"]) + XCTAssertFalse((payload["message"] as? String ?? "").isEmpty) + } + + private func captureDecodingError(_ body: () throws -> Any) throws -> DecodingError { + do { + _ = try body() + } catch let error as DecodingError { + return error + } + XCTFail("expected a DecodingError") + throw XCTSkip("no DecodingError thrown") + } +} diff --git a/tests/test-contract.sh b/tests/test-contract.sh index a550dcd..35dd6d9 100755 --- a/tests/test-contract.sh +++ b/tests/test-contract.sh @@ -267,6 +267,30 @@ else fail "negative seed: HTTP $NEG_CODE (000 = connection dropped), server alive: $(curl -sf "$URL/health" >/dev/null 2>&1 && echo yes || echo no)" fi +# ── 10. Request shapes the OpenAI API allows ────────────────────────────────── +# `stop` may be a bare string; both request types decoded only the array form, and the +# decode failure surfaced as a 500 `server_error`, which the OpenAI SDKs retry. +log "Test 10: stop as a plain string" +BODY=$(chat '{"model":"x","messages":[{"role":"user","content":"Count: one two three four five"}],"max_tokens":80,"temperature":0,"stop":"three"}' || true) +CONTENT=$(echo "$BODY" | python3 -c 'import json,sys; print(json.load(sys.stdin)["choices"][0]["message"].get("content") or "")' 2>/dev/null || echo "__no_body__") +if [ "$CONTENT" = "__no_body__" ]; then + fail "stop as a string was rejected: $(echo "$BODY" | head -c 120)" +elif echo "$CONTENT" | grep -q "three"; then + fail "stop string 'three' leaked into content: $(echo "$CONTENT" | head -c 80)" +else + pass "a bare stop string is accepted and honoured" +fi + +log "Test 10b: a request that does not decode is a 400 invalid_request_error" +CODE=$(curl -s -o /tmp/SwiftLM-contract-400.json -w '%{http_code}' "$URL/v1/chat/completions" \ + -H 'Content-Type: application/json' -d '{"model":"x","messages":[{"role":"user","content":"hi"}],"stop":42}') +ERR_TYPE=$(python3 -c 'import json; e=json.load(open("/tmp/SwiftLM-contract-400.json"))["error"]; print(e.get("type"), e.get("param"))' 2>/dev/null || echo "unparseable") +if [ "$CODE" = "400" ] && [ "$ERR_TYPE" = "invalid_request_error stop" ]; then + pass "400 invalid_request_error naming param 'stop'" +else + fail "got HTTP $CODE with error '$ERR_TYPE' (expected 400 invalid_request_error stop)" +fi + log "═══════════════════════════════════════" log "Results: $PASS passed, $FAIL failed, $SKIP skipped, $TOTAL total" log "═══════════════════════════════════════"