[PR #4] [CLOSED] feat(logging): add credential-free os.Logger error logging at SDK catch sites #9

New issue

Closed

opened 2026-10-26 22:33:51 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:33:51 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/4
Author: @mattmillerai
Created: 6/19/2026
Status: ❌ Closed

Base: matt/be-1606-comfy-ios-backgrounded-fast-job-recovers-but-the-output ← Head: matt/be-1608-comfyswiftsdk-add-minimal-error-logging-at-the

📝 Commits (1)

d765c2a feat(logging): add credential-free os.Logger error logging at SDK catch sites

📊 Changes

5 files changed (+521 additions, -15 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+12 -4)
➕ Sources/ComfySwiftSDK/Internal/SDKLog.swift (+214 -0)
📝 Sources/ComfySwiftSDK/Internal/Transport.swift (+14 -8)
📝 Sources/ComfySwiftSDK/Internal/WebSocketSession.swift (+11 -3)
➕ Tests/ComfySwiftSDKTests/LoggingPrivacyTests.swift (+270 -0)

📄 Description

ELI5

The SDK previously logged nothing, so when a job failed on-device the only visible clue was the final ComfyError thrown to the caller — with no indication of which internal step failed or what the transport saw. This PR adds .error-level Console logs at the failure points (output-build catch, WebSocket read-loop error, execution_error frame, polling give-up, output-assembly failure) so the next bug is self-diagnosing. Logged fields are strictly classification-only — ComfyError case name, POSIX code for socket-drop paths, and jobId — never the API key, OAuth token, Authorization header value, request body, response body, or any raw Error/URLRequest object.

What

Sources/ComfySwiftSDK/Internal/SDKLog.swift (new): thin os.Logger facade under subsystem org.comfy.ComfySwiftSDK, categories transport / websocket / polling. Includes an injectable SDKLogSink override (SDKLog.overrideSink(_:) / resetSink()) for test capture without live OSLog.
Transport.swift: translate(_:) logs the matched POSIX code on socket-drop paths and the error type name on the .unknown fallback.
WebSocketSession.swift: runReadLoop logs the execution_error frame (job id only), the output-build catch after retry exhaustion (case name + job id), and the transient→polling-handoff vs .failed decision.
PollingFallback.swift: poll-loop logs non-transient give-up, output-assembly failure, and empty-output-exhaustion (all case name + job id).
Tests/ComfySwiftSDKTests/LoggingPrivacyTests.swift (new): NFR-S2 enforcement suite. Installs SDKLog.overrideSink, drives each failure path, and asserts that known credential sentinels (sk-SENTINEL-API-KEY-NFR-S2-TEST, bearer-SENTINEL-OAUTH-TOKEN-NFR-S2-TEST) never appear in any captured message. Includes a "poisoned error userInfo" test that embeds the sentinel in an underlying error's NSLocalizedDescriptionKey and verifies it does not leak.

Why

On-device failures in the retry/recovery paths added by #3 (backgrounded job recovery, polling fallback, output-fetch transient retry) were completely silent in Console. When a user reports a blank result or a failed job, there is currently no SDK-level breadcrumb to distinguish "ECONNRESET on the first output download, retried and succeeded" from "ECONNRESET exhausted all retries". These logs close that gap.

Testing

swift build — clean (no errors, pre-existing warnings only)
swift test — 132 tests, 0 failures, 4 skipped (live-network gates, unchanged)
New LoggingPrivacyTests suite: 10 tests covering all logging call sites and the NFR-S2 sentinel assertion

Stacked on #3 — retargets to main when it merges.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-swift-sdk/pull/4 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 6/19/2026 **Status:** ❌ Closed **Base:** `matt/be-1606-comfy-ios-backgrounded-fast-job-recovers-but-the-output` ← **Head:** `matt/be-1608-comfyswiftsdk-add-minimal-error-logging-at-the` --- ### 📝 Commits (1) - [`d765c2a`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/d765c2a81131747c696b4dd2503e7bd407d7850c) feat(logging): add credential-free os.Logger error logging at SDK catch sites ### 📊 Changes **5 files changed** (+521 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+12 -4) ➕ `Sources/ComfySwiftSDK/Internal/SDKLog.swift` (+214 -0) 📝 `Sources/ComfySwiftSDK/Internal/Transport.swift` (+14 -8) 📝 `Sources/ComfySwiftSDK/Internal/WebSocketSession.swift` (+11 -3) ➕ `Tests/ComfySwiftSDKTests/LoggingPrivacyTests.swift` (+270 -0) </details> ### 📄 Description ## ELI5 The SDK previously logged nothing, so when a job failed on-device the only visible clue was the final `ComfyError` thrown to the caller — with no indication of *which internal step* failed or what the transport saw. This PR adds `.error`-level Console logs at the failure points (output-build catch, WebSocket read-loop error, `execution_error` frame, polling give-up, output-assembly failure) so the next bug is self-diagnosing. Logged fields are strictly classification-only — `ComfyError` case name, POSIX code for socket-drop paths, and `jobId` — never the API key, OAuth token, `Authorization` header value, request body, response body, or any raw `Error`/`URLRequest` object. ## What - **`Sources/ComfySwiftSDK/Internal/SDKLog.swift`** (new): thin `os.Logger` facade under subsystem `org.comfy.ComfySwiftSDK`, categories `transport` / `websocket` / `polling`. Includes an injectable `SDKLogSink` override (`SDKLog.overrideSink(_:)` / `resetSink()`) for test capture without live OSLog. - **`Transport.swift`**: `translate(_:)` logs the matched POSIX code on socket-drop paths and the error type name on the `.unknown` fallback. - **`WebSocketSession.swift`**: `runReadLoop` logs the `execution_error` frame (job id only), the output-build catch after retry exhaustion (case name + job id), and the transient→polling-handoff vs `.failed` decision. - **`PollingFallback.swift`**: poll-loop logs non-transient give-up, output-assembly failure, and empty-output-exhaustion (all case name + job id). - **`Tests/ComfySwiftSDKTests/LoggingPrivacyTests.swift`** (new): NFR-S2 enforcement suite. Installs `SDKLog.overrideSink`, drives each failure path, and asserts that known credential sentinels (`sk-SENTINEL-API-KEY-NFR-S2-TEST`, `bearer-SENTINEL-OAUTH-TOKEN-NFR-S2-TEST`) never appear in any captured message. Includes a "poisoned error userInfo" test that embeds the sentinel in an underlying error's `NSLocalizedDescriptionKey` and verifies it does not leak. ## Why On-device failures in the retry/recovery paths added by #3 (backgrounded job recovery, polling fallback, output-fetch transient retry) were completely silent in Console. When a user reports a blank result or a failed job, there is currently no SDK-level breadcrumb to distinguish "ECONNRESET on the first output download, retried and succeeded" from "ECONNRESET exhausted all retries". These logs close that gap. ## Testing - `swift build` — clean (no errors, pre-existing warnings only) - `swift test` — 132 tests, 0 failures, 4 skipped (live-network gates, unchanged) - New `LoggingPrivacyTests` suite: 10 tests covering all logging call sites and the NFR-S2 sentinel assertion Stacked on #3 — retargets to main when it merges. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:33:51 +00:00