test(swift-sdk): make resolver sign tests hermetic to CI keychain state - #4386
test(swift-sdk): make resolver sign tests hermetic to CI keychain state#4386QuantumExplorer wants to merge 2 commits into
Conversation
The two IdentityResolverSignIntegrationTests cases were the only tests in the `swift test` phase that wrote to the real macOS keychain, and they fail whenever a self-hosted runner's keychain session is locked or permission-denied (keychainError -60008 on mac-runner-2, -61 on mac-runner-1) — even with the dedicated CI keychain run_tests.sh provisions. KeychainSigner now accepts an injectable WalletStorage (defaulting to the real one, matching the existing "overridable for tests" pattern on MnemonicResolver and ManagedPlatformWallet), and the tests use an in-memory subclass so the derive-sign-destroy path under test never touches keychain state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughKeychainSigner now accepts injectable wallet storage and passes it to MnemonicResolver. Resolver integration tests use a thread-safe in-memory storage implementation instead of the default storage. ChangesWallet storage injection
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to The signing path can reject a valid resolver-backed key before signing because its preflight check does not use the injected storage. This is a bounded correctness issue that should be fixed before merging. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 1d86a94) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Sources/SwiftDashSDK/FFI/KeychainSigner.swift`:
- Around line 197-204: Retain the initializer’s injected WalletStorage as a
KeychainSigner property, and update both hasMnemonic checks in
canSign(publicKey:keyType:) to use that property instead of creating new
WalletStorage instances. Add a canSign assertion to the successful
resolver-backed integration test.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d4ceb0f6-3bdc-4ece-bffa-f5dbb57d535e
📒 Files selected for processing (2)
packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityResolverSignIntegrationTests.swift
The canSign hasMnemonic preflights constructed fresh WalletStorage() instances, so with an injected storage the preflight and the sign path answered from different stores — a resolver-signable key could preflight as unsignable. Retain the injected storage and consult it in both checks; the in-memory test storage overrides mnemonicAvailability and the successful-sign test now asserts canSign first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head diff correctly injects one shared WalletStorage into KeychainSigner, its MnemonicResolver, and both canSign preflight branches while preserving production behavior through the default argument. The resolver integration tests now use synchronized in-memory storage and explicitly cover successful canSign; no in-scope issues remain.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
2 participants
Issue being fixed or feature implemented
The "Swift SDK build + tests (warnings as errors)" job fails intermittently on the self-hosted mac runners with
keychainErrorfromWalletStorage—-60008(errAuthorizationInternal) on mac-runner-2 and now-61(errSecWrPerm) on mac-runner-1 (e.g. this run on an unrelated PR). The twoIdentityResolverSignIntegrationTestscases were the only tests in theswift testphase writing to the real macOS keychain, so they fail whenever a runner's keychain session is locked or permission-denied — even with the dedicated CI keychainrun_tests.shprovisions.What was done?
KeychainSigner.initnow accepts an injectableWalletStorage(defaulting to the real one), forwarded to itsMnemonicResolver— the same "overridable for tests" pattern already used byMnemonicResolverandManagedPlatformWallet.IdentityResolverSignIntegrationTestsuses an in-memoryWalletStoragesubclass, so the derive-sign-destroy resolver path under test never touches keychain state.How Has This Been Tested?
swift test --filter IdentityResolverSignIntegrationTests— both tests pass, with the resolver trampoline confirmed firing through the injected storage.swift test: 349 tests, 0 failures.Breaking Changes
None — the new parameter is defaulted; existing call sites are unchanged.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests