From 95b6cb6f451876eb81e8269e3f224db36d5fc566 Mon Sep 17 00:00:00 2001 From: Aditya Garud <153842990+yashranaway@users.noreply.github.com> Date: Wed, 5 Aug 2026 19:51:13 +0000 Subject: [PATCH] fix: replace force-unwraps in host command paths Closes #13. visual compare read its two artifact names with request.parameters[...]! .stringValue! on both hosts. That is safe only while CommandRequest.validate() has run first, and these hosts are long-lived: a trap kills the browser, every session, and any active recording, so the failure mode is out of proportion to the mistake. Both sites now guard and answer MISSING_PARAMETER. Also replaces a nil-check-then-force-unwrap in Diagnostics with a plain if-let. Same behaviour, no trap to reason about. The validator does require both parameters, so these guards are defence in depth rather than a live bug fix. ProtocolTests now asserts that requirement directly, so relaxing the validator fails a test instead of quietly making the guards load-bearing. --- apps/headless/LinuxHost/main.swift | 8 ++++++-- .../Sources/HeadlessProtocol/Diagnostics.swift | 2 +- .../HeadlessProtocolTests/ProtocolTests.swift | 17 +++++++++++++++++ apps/headless/main.swift | 8 ++++++-- docs/roadmap/improvements-backlog.md | 7 +++++-- 5 files changed, 35 insertions(+), 7 deletions(-) diff --git a/apps/headless/LinuxHost/main.swift b/apps/headless/LinuxHost/main.swift index fa822eb..38a4f00 100644 --- a/apps/headless/LinuxHost/main.swift +++ b/apps/headless/LinuxHost/main.swift @@ -309,8 +309,12 @@ final class LinuxBrowserHost: @unchecked Sendable { case .networkMockSet: result = try session.setNetworkMock(parameters: request.parameters) case .networkMockClear: result = try session.clearNetworkMocks() case .visualCompare: - let before = request.parameters["before"]!.stringValue! - let after = request.parameters["after"]!.stringValue! + guard let before = request.parameters["before"]?.stringValue else { + return failure(request, "MISSING_PARAMETER", "Before artifact name is required.") + } + guard let after = request.parameters["after"]?.stringValue else { + return failure(request, "MISSING_PARAMETER", "After artifact name is required.") + } _ = try artifacts.read(name: before, expectedExtension: "png", maximumBytes: 100 * 1_024 * 1_024) _ = try artifacts.read(name: after, expectedExtension: "png", maximumBytes: 100 * 1_024 * 1_024) let difference = try artifacts.reserve(requestedName: request.parameters["output"]?.stringValue, diff --git a/apps/headless/Sources/HeadlessProtocol/Diagnostics.swift b/apps/headless/Sources/HeadlessProtocol/Diagnostics.swift index da1815c..b3f132c 100644 --- a/apps/headless/Sources/HeadlessProtocol/Diagnostics.swift +++ b/apps/headless/Sources/HeadlessProtocol/Diagnostics.swift @@ -152,7 +152,7 @@ public final class QADiagnosticStore: @unchecked Sendable { issue["suggestion"] = .string(suggestion) if let url { issue["url"] = .string(url) } if let status { issue["status"] = .number(status) } - if object["method"] != nil { issue["method"] = object["method"]! } + if let method = object["method"] { issue["method"] = method } if url != nil { issue["local"] = .bool(local) } return .object(issue) } diff --git a/apps/headless/Tests/HeadlessProtocolTests/ProtocolTests.swift b/apps/headless/Tests/HeadlessProtocolTests/ProtocolTests.swift index 2aa2e73..9fdc563 100644 --- a/apps/headless/Tests/HeadlessProtocolTests/ProtocolTests.swift +++ b/apps/headless/Tests/HeadlessProtocolTests/ProtocolTests.swift @@ -420,6 +420,23 @@ struct ProtocolTests { static func cliP2CommandsAndBoundaries() throws { let visual = try CLIParser().parse(["visual", "compare", "before.png", "after.png", "--output", "diff.png"]) try expect(visual.request?.command == .visualCompare, "visual compare should parse") + // Both hosts read these two names to locate the artifacts to diff. They + // guard the lookup and answer MISSING_PARAMETER, but the validator is + // what keeps a malformed request from reaching that path at all — if it + // ever stopped requiring them, the guards would be the only thing + // standing between a crafted request and a broken comparison. + for missing in ["before", "after"] { + try expectThrows("visual compare should require \(missing)") { + var parameters: [String: JSONValue] = [ + "before": .string("one.png"), "after": .string("two.png"), + ] + parameters.removeValue(forKey: missing) + try CommandRequest( + id: "visual-compare-missing-\(missing)", command: .visualCompare, + parameters: parameters + ).validate() + } + } try expect(visual.request?.parameters["before"] == .string("before.png"), "visual input should remain an artifact name") let flow = try CLIParser().parse(["flow", "run", "happy-path.json"]) try expect(flow.request?.command == .flowRun, "flow run should parse") diff --git a/apps/headless/main.swift b/apps/headless/main.swift index ba1e013..50a98d4 100644 --- a/apps/headless/main.swift +++ b/apps/headless/main.swift @@ -1172,8 +1172,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate { case .animationList: result = try controller.agentAnimations() case .visualCompare: - let before = request.parameters["before"]!.stringValue! - let after = request.parameters["after"]!.stringValue! + guard let before = request.parameters["before"]?.stringValue else { + return failure(request, code: "MISSING_PARAMETER", message: "Before artifact name is required.") + } + guard let after = request.parameters["after"]?.stringValue else { + return failure(request, code: "MISSING_PARAMETER", message: "After artifact name is required.") + } guard let artifacts else { throw ArtifactError.invalidRoot } _ = try artifacts.read(name: before, expectedExtension: "png", maximumBytes: 100 * 1_024 * 1_024) _ = try artifacts.read(name: after, expectedExtension: "png", maximumBytes: 100 * 1_024 * 1_024) diff --git a/docs/roadmap/improvements-backlog.md b/docs/roadmap/improvements-backlog.md index d25d665..fd9f8de 100644 --- a/docs/roadmap/improvements-backlog.md +++ b/docs/roadmap/improvements-backlog.md @@ -44,8 +44,11 @@ clean restart. `request.parameters["before"]!.stringValue!` on both hosts (`LinuxHost/main.swift:239-240`, `main.swift:1175-1176`). Safe only while `validate()` runs first; any future path that skips validation crashes the -host and kills every session. Replace with guarded extraction returning -`MISSING_PARAMETER`. +host and kills every session. ~~Replace with guarded extraction returning +`MISSING_PARAMETER`.~~ **Done** on both hosts, plus the nil-check-then-force- +unwrap in `HP/Diagnostics.swift:155`. The validator makes these parameters +required, so the guards are defence in depth; `ProtocolTests` now asserts that +requirement so the guards can never become the only thing holding the path up. **A3. Oversized responses break the 1 MiB frame.** ([#14](https://github.com/LockInTime/headless/issues/14)) `qa report` can hold 500 events × ~4 KiB ≈ 2 MB; `artifact.list` is unbounded. `encodeLine` throws