feat(swift-sdk): iOS simluator writes logs to disk#3785
Open
ZocoLini wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements multi-platform file-based logging across Rust FFI and Swift SDK. It captures build-time Git metadata, adds a Rust FFI logging entrypoint that initializes per-component log files and writes build information, integrates file logging into Swift with timestamped session directories for iOS Simulator, and provides a Bash script utility to extract logs from Simulator app containers. ChangesMulti-Platform File Logging Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZocoLini
force-pushed
the
feat/swift-sdk-logs
branch
from
1f9cbbd to
14b641b
Compare
ZocoLini
changed the title
Collaborator
|
✅ Review complete (commit 33b7e99) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift (1)
32-38: 💤 Low valueEmpty
ifbody reads awkwardly. The success branch is intentionally empty; the control flow is clearer when expressed as a single fallback check.♻️ Optional cleanup
- // First subscriber install wins, so try files before - // falling back to stdout-only. - if let sessionRoot = launchLogPaths(), - SDK.enableFileLogging(level: .info, sessionRoot: sessionRoot) { - } else { - SDK.enableLogging(level: .info) - } + // First subscriber install wins, so try files before + // falling back to stdout-only. + let fileLoggingInstalled = launchLogPaths() + .map { SDK.enableFileLogging(level: .info, sessionRoot: $0) } ?? false + if !fileLoggingInstalled { + SDK.enableLogging(level: .info) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift` around lines 32 - 38, The empty if-body is confusing; replace the current two-branch structure by attempting the file-logging call and only calling SDK.enableLogging(level:.info) if that attempt fails — i.e., call launchLogPaths() and then SDK.enableFileLogging(level:.info, sessionRoot: sessionRoot) and, when that returns false (or when launchLogPaths() is nil), call SDK.enableLogging(level:.info); update the logic around launchLogPaths(), SDK.enableFileLogging(level: .info, sessionRoot: sessionRoot) and SDK.enableLogging(level: .info) so there is no empty if block and the fallback is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/logging.rs`:
- Around line 94-109: The function currently returns false if
fs::write(path.join("build_info.txt"), build_info_string()) fails after
successfully calling tracing_subscriber::registry()...try_init(), which
misreports installation state; move the fs::write call to run before invoking
tracing_subscriber::registry().with(...).try_init() (or alternatively, keep it
after but do not make its error dictate the function's boolean return) so that
try_init() success determines the returned true/false; reference the
fs::write(..., build_info_string()), the tracing_subscriber::...try_init() call
and the path variable when applying the change and ensure any write error is
logged but does not flip the installed boolean.
- Around line 56-57: In enable_file_logging() the function currently calls
try_init() to install the global subscriber and only afterwards attempts
fs::write(...) for build_info.txt, returning false if that write fails which
incorrectly signals logging is disabled; either move the build_info.txt write to
before the tracing_subscriber::fmt::layer().with_writer(...)/try_init() sequence
(so write errors prevent installing the subscriber) or keep the current install
order but do not treat a build_info.txt write failure as disabling
logging—instead log the write failure (or ignore it) and still return true to
indicate the global subscriber was successfully installed; reference
enable_file_logging, tracing_subscriber::fmt::layer().with_writer and the
build_info.txt fs::write call when making the change.
In `@packages/rs-sdk-ffi/src/lib.rs`:
- Around line 119-121: The doc comment above build_env_filter is stale—update it
to accurately state which functions use it (e.g., mention
dash_sdk_enable_logging) and remove the reference to a non-existent
enable_logging_to_file; locate the build_env_filter function and its surrounding
comment and change the text to say it builds the per-crate EnvFilter used by
dash_sdk_enable_logging (and that RUST_LOG still overrides programmatic
directives) so the documentation matches the actual symbols in this crate.
In `@packages/swift-sdk/extract_simulator_logs.sh`:
- Line 169: The conditional uses the non-portable -a inside [ ]: replace the
combined test [ -z "$SESSION" -a "${`#sessions`[@]}" -eq 1 ] with two separate
tests joined by && and grouped, e.g. use: if [ "$SESSION" = "latest" ] || { [ -z
"$SESSION" ] && [ "${`#sessions`[@]}" -eq 1 ]; } so that SESSION and sessions
array checks are POSIX-compliant; update the conditional in the same if
statement that references the SESSION variable and the sessions array.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift`:
- Around line 32-38: The empty if-body is confusing; replace the current
two-branch structure by attempting the file-logging call and only calling
SDK.enableLogging(level:.info) if that attempt fails — i.e., call
launchLogPaths() and then SDK.enableFileLogging(level:.info, sessionRoot:
sessionRoot) and, when that returns false (or when launchLogPaths() is nil),
call SDK.enableLogging(level:.info); update the logic around launchLogPaths(),
SDK.enableFileLogging(level: .info, sessionRoot: sessionRoot) and
SDK.enableLogging(level: .info) so there is no empty if block and the fallback
is explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2fd3834-9f54-425b-b82f-4ec7cf22d377
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/build.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/logging.rspackages/rs-sdk-ffi/src/lib.rspackages/swift-sdk/.gitignorepackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/extract_simulator_logs.sh
ZocoLini
force-pushed
the
feat/swift-sdk-logs
branch
from
14b641b to
479795e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/swift-sdk/extract_simulator_logs.sh (1)
36-47: ⚡ Quick winMissing option values fail with a raw
unbound variableerror.Under
set -u, invoking an option without its value (e.g../extract_simulator_logs.sh --out) dereferences$2and aborts with$2: unbound variableinstead of a clear message. Guarding the argument count gives a friendlier failure.♻️ Proposed guard
while [ "$#" -gt 0 ]; do case "$1" in - --bundle-id) BUNDLE_ID="$2"; shift 2 ;; - --out) OUT_DIR="$2"; shift 2 ;; - --device) DEVICE="$2"; shift 2 ;; - --session) SESSION="$2"; shift 2 ;; + --bundle-id|--out|--device|--session) + if [ "$#" -lt 2 ]; then echo "Missing value for $1" >&2; exit 2; fi ;;& + --bundle-id) BUNDLE_ID="$2"; shift 2 ;; + --out) OUT_DIR="$2"; shift 2 ;; + --device) DEVICE="$2"; shift 2 ;; + --session) SESSION="$2"; shift 2 ;;Note:
;;&requires Bash 4+. macOS ships Bash 3.2 (as the comment at lines 153-155 notes), so prefer an explicit per-option check instead if macOS compatibility is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/extract_simulator_logs.sh` around lines 36 - 47, The option parsing currently dereferences $2 under set -u causing an unbound variable error when a flag is passed without a value; update the case arms for --bundle-id, --out, --device, and --session to explicitly check that a value exists before using $2 (e.g. test that "$#" -ge 2) and emit a clear error like "Missing value for --out" (or the specific flag) and exit with non-zero if not present, then perform the assignment to BUNDLE_ID/OUT_DIR/DEVICE/SESSION and shift 2; keep the case structure and avoid Bash-4-only constructs so macOS compatibility is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/extract_simulator_logs.sh`:
- Around line 11-15: The header comment in extract_simulator_logs.sh lists
component directories with hyphens (e.g., "dash-sdk", "platform-wallet",
"dash-spv", "key-wallet") but the Rust writer creates directories with
underscores (dash_sdk, platform_wallet, dash_spv, key_wallet, grpc); update that
comment to use the underscore names so it matches actual output (look for the
comment block that currently shows "// dash-sdk/run.log" etc., and
replace hyphenated names with dash_sdk, platform_wallet, dash_spv, key_wallet,
grpc).
---
Nitpick comments:
In `@packages/swift-sdk/extract_simulator_logs.sh`:
- Around line 36-47: The option parsing currently dereferences $2 under set -u
causing an unbound variable error when a flag is passed without a value; update
the case arms for --bundle-id, --out, --device, and --session to explicitly
check that a value exists before using $2 (e.g. test that "$#" -ge 2) and emit a
clear error like "Missing value for --out" (or the specific flag) and exit with
non-zero if not present, then perform the assignment to
BUNDLE_ID/OUT_DIR/DEVICE/SESSION and shift 2; keep the case structure and avoid
Bash-4-only constructs so macOS compatibility is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13e347a3-96c8-42e0-8bcc-f67c2fffaea7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/build.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/logging.rspackages/swift-sdk/.gitignorepackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/extract_simulator_logs.sh
✅ Files skipped from review due to trivial changes (1)
- packages/swift-sdk/.gitignore
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rs-platform-wallet-ffi/Cargo.toml
- packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/rs-platform-wallet-ffi/build.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift
- packages/rs-platform-wallet-ffi/src/logging.rs
thepastaclaw
suggested changes
Jun 2, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds an optional per-bucket file-logging FFI plus a log extraction script. Verified two notable in-scope issues: (1) the Swift entry point unconditionally enables on-device file logging despite the PR scoping the feature to the simulator, with attendant privacy/retention implications; (2) the Rust FFI's boolean contract is violated when build_info.txt write fails after the global subscriber has already been installed, which causes the Swift caller to silently take a now-noop fallback. Several smaller issues exist in the build-info plumbing and extraction-script documentation.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift:32-38: File logging is enabled on physical iOS devices, not just the simulator
`LoggingPreferences.configure()` calls `SDK.enableFileLogging` unconditionally, and `launchLogPaths()` only checks that a Library directory exists — there is no `#if targetEnvironment(simulator)` guard. As a result, physical iOS devices (and the macOS build) will also start writing per-launch session directories at `<sandbox>/Library/Logs/SwiftDashSDK/<timestamp>/` containing `info`-level logs from `dash_sdk`, `platform_wallet`, `dash_spv`, `key_wallet`, and `grpc`. Two concrete problems on devices: (a) no rotation or cap, so disk usage grows monotonically across the app's lifetime, and (b) `Library/Logs` is included in iTunes/iCloud backups by default, meaning logs containing identity IDs, balances, endpoints, and asset-lock traces can leave the device. The PR title ("iOS simluator writes logs to disk") and the accompanying `extract_simulator_logs.sh` make it clear the feature is intended for simulator debugging. Either gate the file-logging path with `#if targetEnvironment(simulator)` or document and accept the on-device behavior with an explicit retention/backup policy.
In `packages/rs-platform-wallet-ffi/src/logging.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/logging.rs:94-109: FFI returns `false` after the global tracing subscriber is already installed
`tracing_subscriber::registry()...try_init()` at lines 94-102 installs the global subscriber for the lifetime of the process — it cannot be replaced afterwards. The subsequent `fs::write(path.join("build_info.txt"), ...)` at line 107 returning `Err` causes the function to return `false`, but the file subscriber is already active. The docstring (lines 14-18) tells Swift that `false` means "no subscriber was installed by this call", so `SDK.enableFileLogging` propagates `false` to `LoggingPreferences.configure()`, which then falls through to `SDK.enableLogging(level: .info)` — that path calls `dash_sdk_enable_logging`, which silently no-ops because the global is taken. Net result: the documented fallback never runs, the caller has no signal that file logging is in fact active, and a callsite that branches on the return value to pick alternate routing will be misled. Either return `true` once `try_init()` succeeds and downgrade the `build_info.txt` write failure to a `tracing::warn!`, or write `build_info.txt` *before* `try_init()` so the contract holds.
In `packages/rs-platform-wallet-ffi/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/build.rs:13-26: Build-info `git-dirty` is monorepo-wide and the rerun hooks are broken in worktrees
Two issues in the same plumbing that make the metadata baked into `build_info.txt` unreliable:
1. `run_git(&["status", "--porcelain"])` (line 16) has no pathspec and no `--untracked-files=no`, so it scans the entire workspace. Any untracked or modified file in any crate of the monorepo marks this crate's build as dirty. In practice `git-dirty: yes` will be the common case whenever a developer has anything in flight, even when `rs-platform-wallet-ffi/` itself is clean. Scoping the status check to the files this crate's binary actually depends on (e.g. `packages/rs-platform-wallet-ffi`, `packages/rs-platform-wallet`, `packages/key-wallet`) would make the flag meaningful.
2. `cargo:rerun-if-changed={workspace_root}/.git/HEAD` / `/.git/index` (lines 24-25) assumes `.git` is a directory. This repo is commonly checked out as a Git worktree (in fact this verifier is running from `/Users/claw/.openclaw/workspace/worktrees/review-platform-3785`), where `.git` is a text file pointing at the real Git dir and neither `.git/HEAD` nor `.git/index` exists at that path. Cargo therefore never invalidates the build script when the commit or index changes, leaving stale `PLATFORM_WALLET_GIT_COMMIT` / `PLATFORM_WALLET_GIT_DIRTY` baked into the binary across rebuilds. Resolve the actual paths with `git rev-parse --git-path HEAD` / `--git-path index` and emit those instead.
479795e to
c677109
Compare
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "06b958767e21da883e67416eee1f8d5e8f1411d304a9fe73e0c9d1ad2ab25a5c"
)Xcode manual integration:
|
thepastaclaw
reviewed
Jun 2, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-verified prior review at head 479795e. All five prior findings remain valid: the latest delta only refactored dash_sdk_enable_logging in rs-sdk-ffi/src/lib.rs and did not touch any of the previously flagged code. One additional convergent finding was introduced by the refactor: the new in-process EnvFilter string dropped dash_spv and key_wallet directives that the sibling broad_env_filter (and the prior build_env_filter helper) still carry, causing SPV and key-wallet traces to fall through to EnvFilter's default warn level on the stdout-only path.
🟡 3 suggestion(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/lib.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/lib.rs:112-117: Refactor of `dash_sdk_enable_logging` dropped `dash_spv` and `key_wallet` directives
The 14b641bb..479795ed delta inlined the prior `build_env_filter` helper into `dash_sdk_enable_logging` (a legitimate fix that avoids the `env::set_var("RUST_LOG", ...)` data-race documented in the surrounding comment). However, the new `filter_string` no longer lists `dash_spv={log_level},key_wallet={log_level}` — both of which were present in the previous implementation and are still present in the sibling `broad_env_filter` at `packages/rs-platform-wallet-ffi/src/logging.rs:127-138`. Consequences:
- On the stdout fallback path (when `enable_file_logging` returns false, when `targetEnvironment(simulator)` is gated off after the blocking finding is applied, on macOS host runs, in unit-test harnesses, and for any FFI consumer that only calls `dash_sdk_enable_logging`), SPV and key-wallet traces will silently fall through to EnvFilter's default `warn` level instead of honoring the requested level.
- The two filter sets now disagree about which crates' logs the user gets, which is confusing for downstream consumers and likely unintentional given the inlined comment only justifies adding `platform_wallet`/`platform_wallet_ffi`, not removing the other two.
Restore the dropped directives so the stdout filter matches `broad_env_filter`.
In `packages/rs-platform-wallet-ffi/src/logging.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/logging.rs:94-113: `platform_wallet_enable_file_logging` returns `false` after the global subscriber is already installed
`tracing_subscriber::registry()...try_init()` at line 101 installs the process-wide subscriber for the lifetime of the program — once it succeeds, no subsequent `try_init` call (including the stdout fallback `SDK.enableLogging` from Swift) can register another subscriber. If the subsequent `fs::write(path.join("build_info.txt"), ...)` at line 107 fails (read-only volume, sandbox quirk, transient FS error), the function still returns `false`. The Swift caller then takes the `else` branch in `LoggingPreferences.configure()` and calls `SDK.enableLogging(level: .info)`, which is now a silent no-op because the registry subscriber is already global. Result: file logging is in fact active, but the caller has been told it isn't — confusing both for diagnostics and for future attempts to attach extra layers.
Either write `build_info.txt` *before* `try_init` (so a write failure is observable and prevents installation), or treat the write as best-effort: log a warning and still return `true`.
In `packages/rs-platform-wallet-ffi/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/build.rs:13-26: Git-dirty detection is monorepo-wide and rerun hints reference paths that don't exist in worktrees
Two related problems whose outputs are baked into `build_info.txt` and shipped with every session:
1. `run_git(&["status", "--porcelain"])` (line 16) runs with no pathspec and no `--untracked-files=no`, so any unrelated edit anywhere in the ~44-crate monorepo — including untracked files outside this crate — flips `PLATFORM_WALLET_GIT_DIRTY` to `1`. Every iOS log session then carries the `WARNING: this build had uncommitted changes` blurb even when `packages/rs-platform-wallet-ffi/` and its deps are clean. Scope the check to this crate (and its workspace dependencies if needed): `git status --porcelain --untracked-files=no -- <crate-dir>`.
2. `cargo:rerun-if-changed={workspace_root}/.git/HEAD` and `.git/index` (lines 24-25) assume a non-worktree checkout. In a git worktree (this very review is running in one: `/Users/claw/.openclaw/workspace/worktrees/review-platform-3785`), the worktree root's `.git` is a *file* pointing into `<main>/.git/worktrees/<name>/`, so `HEAD`/`index` live there, not at the workspace root. The hints point to non-existent paths and Cargo will not invalidate the build info when HEAD advances or branches switch inside the worktree — leading to stale `GIT_COMMIT` values in the shipped binary. Use `git rev-parse --git-path HEAD` / `--git-path index` (or `--git-dir`) to resolve the real paths.
8280549 to
33b7e99
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/get_logs.sh`:
- Around line 174-190: The session-prefix matching in get_logs.sh currently
accepts exact matches or prefixes but always returns matched[0] even when
multiple sessions match; update the logic in the pick_session/SESSION handling
block to check "${`#matched`[@]}" and if it is greater than 1 print an error like
"Ambiguous session prefix '$SESSION' matches multiple sessions" to stderr and
exit 1, only printing "${matched[0]}" (and returning) when the match count is
exactly 1; reference the variables and arrays SESSION, sessions, matched and the
existing printf '%s\n' "${matched[0]}" location to implement this check.
- Around line 36-46: The arg-parsing loop reads $2 for
--bundle-id/--out/--device/--session without checking presence and the
pick_session helper silently picks matched[0] when multiple sessions match;
update the while/case block to validate arity before assigning (ensure "$#" -ge
2 and that $2 is not another option like starts with '-') and emit a
usage/argument error if the value is missing, then shift 2 as before; update
pick_session so it detects multiple matches (inspect the matched array/variable)
and returns an "ambiguous session" error (and non-zero exit) instead of silently
using matched[0], only accepting a single exact or uniquely matching prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67d6ef36-eea3-4fe9-b2d6-5e5a93292785
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/build.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/logging.rspackages/swift-sdk/.gitignorepackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/get_logs.sh
✅ Files skipped from review due to trivial changes (1)
- packages/swift-sdk/.gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-platform-wallet-ffi/Cargo.toml
- packages/rs-platform-wallet-ffi/build.rs
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/SDKLogger.swift
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/rs-platform-wallet-ffi/src/logging.rs
ZocoLini
force-pushed
the
feat/swift-sdk-logs
branch
from
33b7e99 to
b7ef95b
Compare
thepastaclaw
reviewed
Jun 2, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: file-logging filter l_sdk and the stdout fallback in rs-sdk-ffi both still omit components, build.rs still reports monorepo-wide dirtiness, the new get_logs.sh header still documents hyphenated bucket names that Rust never creates, and a failed enable_file_logging still leaves a partial session directory behind. Fixed in this delta: the simulator-only gate in Swift, and writing build_info.txt before try_init. One new latest-delta-adjacent finding: the per-file dash_sdk bucket filter is missing the rs_sdk directive that the broad filter and the new Swift API contract both imply.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/lib.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/lib.rs:112-117: Stdout fallback filter omits dash_spv and key_wallet
`dash_sdk_enable_logging` is the fallback path used on every non-simulator build and whenever `platform_wallet_enable_file_logging` returns false on the simulator (see SDKLogger.swift:33-41). Its filter string lists dash_sdk/rs_sdk/rs_sdk_ffi/platform_wallet/platform_wallet_ffi/dapi_grpc/h2/tower/hyper/tonic but not dash_spv or key_wallet, while the sibling file-logging path (`broad_env_filter` in platform-wallet-ffi/src/logging.rs:127-134) does include them. Result: SPV-sync and key-wallet traces silently fall through to EnvFilter's default (`warn`) on every device build, regressing observability that the file-logging path is careful to preserve.
In `packages/rs-platform-wallet-ffi/src/logging.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/logging.rs:56-61: dash_sdk bucket file filter omits rs_sdk crate
The `l_sdk` layer routes records to `dash_sdk/run.log` and its EnvFilter only enables `dash_sdk={log_level},rs_sdk_ffi={log_level}`. The shared `broad_env_filter` in the same file (line 129) explicitly treats `rs_sdk` as part of the SDK surface, and so does the stdout fallback in rs-sdk-ffi. Anyone collecting `dash_sdk/run.log` via `get_logs.sh` for a simulator triage will be missing every `rs_sdk` event even though file logging reported success.
In `packages/rs-platform-wallet-ffi/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/build.rs:16-20: Git-dirty flag reflects monorepo-wide status, not the wallet crate
`run_git(&["status", "--porcelain"])` runs with no pathspec and default untracked handling, so any unrelated edit anywhere in the worktree flips `PLATFORM_WALLET_GIT_DIRTY` to 1. Simulator log bundles produced by this PR will then claim the wallet build was dirty even when only an unrelated package was touched, making `build_info.txt` actively misleading for the debugging workflow this PR is adding.
thepastaclaw
reviewed
Jun 2, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried forward review of PR #3785 at 33b7e99. The two highest-impact prior issues are resolved: file logging is now correctly gated to the iOS Simulator (prior-1), and build_info.txt is written before try_init so the boolean return value no longer misreports state after the subscriber is installed (prior-3). Four prior findings still reproduce on the current head and are surfaced here; one of them (the stdout fallback filter omitting dash_spv/key_wallet) is now strictly worse because the new simulator gate routes all device/macOS callers exclusively through the narrower filter. No new in-scope issues introduced by the delta beyond these carried-forward items.
🟡 2 suggestion(s) | 💬 2 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/lib.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/lib.rs:112-117: Stdout-fallback EnvFilter omits dash_spv and key_wallet, diverging from file-logging path
`dash_sdk_enable_logging`'s inlined filter string lists dash_sdk, rs_sdk, rs_sdk_ffi, platform_wallet, platform_wallet_ffi, dapi_grpc, h2, tower, hyper, and tonic — but not dash_spv or key_wallet. The companion file-logging path (`broad_env_filter` in `packages/rs-platform-wallet-ffi/src/logging.rs:127-138`, plus the per-bucket layers for dash_spv and key_wallet at lines 70-82) does include both. After the prior-1 fix introduced `#if targetEnvironment(simulator)` in `SDKLogger.swift:33-41`, every non-simulator Swift caller — physical iOS devices and the fallback path when file logging fails — now routes exclusively through `dash_sdk_enable_logging`. The result is that SPV and key-wallet info-level traces silently fall back to EnvFilter's `warn` default on devices, while the same call on the simulator captures them. The two FFI entry points should expose the same logical coverage; the prior refactor that inlined this filter clearly intended to.
In `packages/rs-platform-wallet-ffi/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/build.rs:16-21: Dirty-state detection is monorepo-wide and includes untracked files
`run_git(&["status", "--porcelain"])` has no pathspec and no `--untracked-files=no`, so any edit anywhere in the ~44-crate monorepo — including stray untracked files such as the `.review/` artifacts review tooling creates in this very worktree — flips `PLATFORM_WALLET_GIT_DIRTY` to `1`. The `build_info.txt` emitted into each simulator session then claims the wallet binary is dirty (and appends the 'commit hash above does NOT fully describe the source state' warning) even when nothing in the wallet crate or its dependencies actually changed. The rerun hints were correctly tightened to use `git rev-parse --git-path` in this delta; the dirty probe should be scoped to match. At minimum exclude untracked files; ideally restrict to paths the wallet crate actually depends on.
QuantumExplorer
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes two things:
the logs structure is as follows:
. ├── build_info.txt ├── dash_sdk │ └── run.log ├── dash_spv │ └── run.log ├── grpc │ └── run.log ├── key_wallet │ └── run.log └── platform_wallet └── run.logbuild_info contains the state of the repo at compile time and each folder the logs written by those crates
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit