Skip to content

feat: add controlled diagnostics download - #127

Merged
psimaker merged 3 commits into
mainfrom
agent/app-controlled-download
Jul 15, 2026
Merged

feat: add controlled diagnostics download#127
psimaker merged 3 commits into
mainfrom
agent/app-controlled-download

Conversation

@psimaker

@psimaker psimaker commented Jul 15, 2026

Copy link
Copy Markdown
Owner

What & why

Implement the owner-authorized M6 controlled-download leg of Decision 024 against the published, unchanged helper 2.0.2 runtime. Download evidence derives only inside the same explicit operation after its exact accepted upload: one signed type-6 response authorization over the pinned endpoint, then a fresh ItemFinished apply of the exact expected response path — newer than the post-upload cursor and wall-clock baselines inside an unchanged engine generation — followed by complete type-7 chain validation. Causal roundtrip remains unset.

The signed owner-device suite was not executed — owner-approved physical-device waiver (2026-07-15). It is replaced by fresh exact-head simulator and isolated Syncthing substitute evidence listed under Testing; no real-device, hardware-keychain, real-APNs, background-wake, or TestFlight-hardware behavior is claimed.

Evidence boundary

  • Upload: unchanged from the M5 boundary; only the exact active type-5 helper attestation over the pinned channel can set it.
  • Download: set only by the same active operation after upload acceptance, one signed response authorization, a fresh post-baseline local apply of the exact response path, and full signature/binding/digest/nonce/payload/TTL validation. A response predating the baseline, an engine restart, or any validation failure can never set it; invalid bytes at the exact path terminate as conflict; every terminal outcome after upload preserves the upload field as a partial result.
  • Roundtrip: unset; no derivation exists in this milestone.
  • Cleanup: evidence-orthogonal; no app cleanup workflow is added here.
  • Helper wire surface: byte-identical helper 2.0.2; no helper, Relay v1, APNs, or StoreKit change.
  • Decision 024 remains unchanged at blob f41f597d3ceca73da102e5e447382dfae07d2e08.

Component(s)

  • go (bridge / Syncthing) — untouched
  • ios (app / widget)
  • notify (contract-isolation guard + isolated E2E test only; no runtime or wire change)
  • docs / CI

Testing

  • Production Swift response protocol bound to the cross-language diagnostics-response-m6.json golden vectors, including full-chain validation and per-byte tamper rejection
  • M6 controlled-download runtime suite: stale pre-baseline, tampered exact-path artifact, engine-generation change, cancellation during the download leg, and restart non-resumption never set evidence; the exact fresh chain sets upload then download
  • M5 foreground upload runtime suite re-run with the download leg, including partial-result preservation and rate limiting
  • TestDiagnosticsDownloadThroughTwoEphemeralSyncthingInstances in the isolated no-network Linux container: exact request/attestation propagate app→helper through real Syncthing, the real helper response foundation creates the one signed response artifact, the exact bytes propagate helper→app and validate through the full D024 chain, helper restart replays idempotently
  • TestDiagnosticsUploadThroughTwoEphemeralSyncthingInstances re-run in the same isolated container
  • cd notify && go test ./... -count=1 on macOS, plus go vet and gofmt
  • Complete iOS plan: 431 tests / 438 parameterized runs passed, zero failed/skipped
  • Release-configuration iOS Simulator build
  • Design-token lint, strings parity (883 keys), sync-proof privacy lint
  • Signed owner-device focused suite: not executed — owner-approved physical-device waiver; substitute evidence is the fresh simulator plan, focused suites, Release-configuration simulator build, and isolated two-instance Syncthing E2E above

Compatibility and rollback

Existing-user upgrade, app launch, Settings inspection, Relay/APNs activity, and ordinary/background sync create no key, pairing, trust, namespace, peer, share, artifact, rescan, or configuration change. Old or downgraded helpers yield capability unavailable without fallback. App/helper rollback preserves credentials, namespace authorization, opaque copies, backups, versions, conflicts, history, tombstones, mappings, and user data; the request, attestation, and response artifacts are synchronized opaque files whose retained copies never regain validity. Forward recovery starts with a fresh capability and never resumes an old proof.

Security and privacy

The download leg is foreground-only and bound to the same explicit operation, tuple, and TTL as its upload. It uses the pinned TLS-1.3/SPKI endpoint with a fixed path, one signed byte-exact authorization, local event observation only (no helper polling), rate/concurrency limits shared with the upload leg, and terminal late-response rejection. It performs no discovery, trust adoption, namespace creation, Relay call, APNs call, StoreKit call, logging, telemetry, crash annotation, durable proof storage, or global success derivation.

psimaker added 2 commits July 15, 2026 10:56
Add the M6 controlled-download leg to the explicit D024 operation:
after the exact accepted upload the app captures a fresh event-cursor,
wall-clock, and engine-generation baseline, sends one signed type-6
response authorization over the pinned endpoint, and sets download
observed only after a fresh ItemFinished apply of the exact expected
response path plus complete type-7 chain validation.

Bind product code to the cross-language M6 golden vectors and cover
stale, tampered, generation-changed, cancelled, and restarted downloads
plus partial-result preservation. Prove the response transport with a
second two-instance Syncthing E2E driving the real helper foundation.
Record the M6 evidence boundary: separate upload and download fields,
partial semantics after an accepted upload, retained opaque response
copies, unchanged helper 2.0.2 wire surface, and the owner-approved
physical-device waiver with simulator plus isolated Syncthing
substitute evidence. Roundtrip remains unset and VaultSync 2.0 NO-GO.
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds a signed diagnostics response protocol, controlled download evidence tracking, updated foreground-check UI and localization, runtime boundary tests, a Syncthing download E2E test, and CI coverage for both upload and response transfer.

Changes

Response protocol and acceptance

Layer / File(s) Summary
Response protocol and acceptance contract
ios/VaultSync/Services/..., docs/..., PRIVACY.md
Adds signed authorization and response-artifact validation, fresh event filtering, response paths, and M6 evidence semantics.
Foreground upload and download orchestration
ios/VaultSync/Services/DiagnosticsPairingController.swift
Tracks upload/download evidence, polls event snapshots, validates responses, and preserves counters across cancellation and failure states.
Runtime and failure-boundary validation
ios/VaultSyncTests/Diagnostics*RuntimeTests.swift
Covers golden vectors, tampering, stale events, conflicts, generation changes, cancellation, restart, and successful response handling.
Controlled-check UI and localized states
ios/VaultSync/Views/ControlledDiagnosticsView.swift, ios/VaultSync/*lproj/Localizable.strings
Updates controls and localized status messages for upload, pending response, download observed, and partial outcomes.
Cross-platform transfer verification
.github/workflows/ci.yml, notify/*, ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
Adds Syncthing response-transfer coverage, expands CI test selection, and allows the new response protocol carrier.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Poem

Signed replies cross the stream,
Upload wakes a download dream.
Fresh events pass the guarded gate,
Tampered bytes meet their fate.
Syncthing hums from side to side—
Roundtrip waits, but proofs abide.

🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Private Note Leakage ✅ Passed PASS: the new network post sends only signed CBOR authorization bytes; event JSON is read locally, and no logging/analytics/crash-report paths were added.
Bounded Ios Background Work ✅ Passed PASS: The new async upload/download work is finite, cancellation-aware, cancels on scene exit, and surfaces only generic errors; no BGTask APIs were added.
Bridge Contract Compatibility ✅ Passed No bridge contract break: Swift-only additions kept existing bridge wrappers intact, JSON event keys match Go’s Event tags, and contract tests now include DiagnosticsResponseProtocol.swift.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses conventional-commit style and accurately summarizes the new controlled diagnostics download flow.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/app-controlled-download

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@psimaker
psimaker marked this pull request as ready for review July 15, 2026 09:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
ios/VaultSync/Services/DiagnosticsPairingController.swift (1)

732-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Misleading "retransmit until accepted" comment — the retry can never actually happen.

transport.post(path: DiagnosticsResponseProtocol.path, ..., responseBody: false) only returns without throwing on HTTP 202, which immediately sets authorizationAccepted = true. Any other outcome (4xx/429/network error) throws, and since this call sits inside the same un-caught try chain as the rest of runControlledDownload, that throw aborts the whole controlled-download attempt (propagating to runForegroundUpload's outer catch). So the loop can never re-enter this block with authorizationAccepted == false on a later iteration — it either succeeds once or the whole operation dies on the first attempt. Unlike the upload leg's poll (where both 200 and 202 are non-throwing, so continue genuinely retries "not ready yet"), there's no non-throwing "pending" outcome here for the guard to protect against.

This matches what docs/m6-controlled-download-readiness.md documents ("the authorization is sent once"), so it's not a behavior bug — but the comment and the if !authorizationAccepted { … } wrapping imply resilience against a single transient failure that doesn't exist, which could mislead a future maintainer relying on it.

✏️ Suggested clarification
-            if !authorizationAccepted {
-                // Retransmit the byte-identical signed authorization until the
-                // idempotent helper accepts it; a 202 carries no body and is
-                // transport diagnostics only, never download evidence.
-                try consumeUploadRequest(recordID: recordID)
+            if !authorizationAccepted {
+                // Sent at most once: any transport failure here aborts the
+                // whole controlled-download attempt rather than retrying on a
+                // later poll (202 is the only non-throwing outcome, and it
+                // immediately marks acceptance). Matches the documented
+                // "sent once" behavior in m6-controlled-download-readiness.md.
+                try consumeUploadRequest(recordID: recordID)
🤖 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 `@ios/VaultSync/Services/DiagnosticsPairingController.swift` around lines 732 -
748, Clarify the comment above the authorization POST to state that the
canonical authorization is sent once and that a successful HTTP 202 marks it
accepted; remove the misleading “retransmit until accepted” wording. Keep the
existing `if !authorizationAccepted` flow and error propagation unchanged, since
no retry behavior is requested.
🤖 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 `@ios/VaultSync/Views/ControlledDiagnosticsView.swift`:
- Around line 538-545: Update the events closure to fetch and store the result
of SyncBridgeService.getEventsSince(Int(sinceID)) before reading
SyncBridgeService.eventStreamGeneration(), then pass the fetched events and
newly read generation to DiagnosticsResponseProtocol.eventSnapshot. Preserve the
existing sinceID conversion and snapshot structure.

---

Nitpick comments:
In `@ios/VaultSync/Services/DiagnosticsPairingController.swift`:
- Around line 732-748: Clarify the comment above the authorization POST to state
that the canonical authorization is sent once and that a successful HTTP 202
marks it accepted; remove the misleading “retransmit until accepted” wording.
Keep the existing `if !authorizationAccepted` flow and error propagation
unchanged, since no retry behavior is requested.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 65201fa3-a46e-4cb3-a5ad-178ceefde089

📥 Commits

Reviewing files that changed from the base of the PR and between e7334d3 and 3b8d827.

📒 Files selected for processing (18)
  • .github/workflows/ci.yml
  • PRIVACY.md
  • docs/architecture.md
  • docs/m6-controlled-download-readiness.md
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/de.lproj/Localizable.strings
  • ios/VaultSync/en.lproj/Localizable.strings
  • ios/VaultSync/es.lproj/Localizable.strings
  • ios/VaultSync/zh-Hans.lproj/Localizable.strings
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift
  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • notify/diagnostics_contract_model_test.go
  • notify/diagnostics_download_syncthing_e2e_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{md,sh,go,swift}

📄 CodeRabbit inference engine (README.md)

Keep Relay-side request observation and wake-ups received on the iPhone as separate diagnostics evidence; one must not be treated as proof of the other.

Files:

  • docs/m6-controlled-download-readiness.md
  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • notify/diagnostics_contract_model_test.go
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • notify/diagnostics_download_syncthing_e2e_test.go
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • PRIVACY.md
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • docs/architecture.md
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift
docs/**/*.md

📄 CodeRabbit inference engine (docs/troubleshooting.md)

docs/**/*.md: Troubleshooting documentation should map each symptom to a fix, and users should retry from the app after applying each fix.
Document the supported vaultsync-notify installation topologies separately: Docker, systemd, launchd, Docker Compose, and Windows PowerShell.
Treat HTTP 429 from the relay trigger endpoint as a successful reachability result, not a doctor failure.
Treat inactive subscriptions and peer-state warnings as non-failing diagnostic conditions; --healthcheck must omit peer-state checks so offline peers do not make the container unhealthy.
Explain that vaultsync-notify reads the Syncthing API key from config.xml; permission or wrong-file errors should be diagnosed through the process user, SYNCTHING_CONFIG, and any SYNCTHING_API_KEY override rather than requesting a pasted key.
When troubleshooting relay connectivity, verify internet access, RELAY_URL, egress rules, the relay health endpoint, and the app's relay diagnostics; a successful health check proves reachability, while an updated Last Trigger Received proves delivery.
APNs background wake-ups require a valid APNs token and provisioned device, but do not require notification permission; retry APNs registration and provisioning before testing a trigger.
VaultSync should not move, recreate, or delete vault folders automatically; recovery from moved, replaced, or deleted folders requires the user's manual decision and may involve removing and re-accepting the share.
Security-scoped bookmark failures should be resolved by reconnecting and reselecting the Obsidian folder, then rescanning; removing a vault only stops syncing on that iPhone and must not affect other devices.
Foreground iPhone-to-server syncing is reliable only while VaultSync is open; iOS background execution is system-controlled and not guaranteed.
When a required Syncthing device is disconnected, verify that it is online, connectivity exists through LAN/VPN/relay, and its device ID is unchanged before re...

Files:

  • docs/m6-controlled-download-readiness.md
  • docs/architecture.md
**/*

⚙️ CodeRabbit configuration file

**/*: VaultSync syncs private Obsidian notes through Syncthing. Treat data loss,
privacy leaks, security regressions, and broken sync behavior as high priority.
Do not nitpick formatting unless it affects maintainability, correctness, or public API clarity.
Flag any accidental logging, telemetry, crash reporting, or network transfer of note contents,
vault paths, filenames with private context, API keys, APNs tokens, relay keys, or security-scoped bookmark data.

Files:

  • docs/m6-controlled-download-readiness.md
  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • notify/diagnostics_contract_model_test.go
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • notify/diagnostics_download_syncthing_e2e_test.go
  • ios/VaultSync/de.lproj/Localizable.strings
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • PRIVACY.md
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • ios/VaultSync/en.lproj/Localizable.strings
  • docs/architecture.md
  • ios/VaultSync/zh-Hans.lproj/Localizable.strings
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
  • ios/VaultSync/es.lproj/Localizable.strings
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift
**/*.md

⚙️ CodeRabbit configuration file

**/*.md: Review public documentation for technical accuracy, privacy/security claims, App Store-facing wording,
setup correctness, and consistency with the free app plus optional Cloud Relay subscription model.

Files:

  • docs/m6-controlled-download-readiness.md
  • PRIVACY.md
  • docs/architecture.md
**/*.swift

📄 CodeRabbit inference engine (Custom checks)

For Swift background execution changes, pass if work is bounded, cancellation-aware, handles expiration callbacks, and records errors without leaking private vault data. Fail only when background work can continue unbounded, miss cleanup, or violate iOS background execution constraints.

Files:

  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift
ios/**/*.swift

📄 CodeRabbit inference engine (README.md)

ios/**/*.swift: Develop the iOS/iPadOS app using Swift 6 and SwiftUI, targeting iOS/iPadOS 18 or later.
Use VoiceOver and Dynamic Type throughout the iOS/iPadOS app.
Support localization in English, German, Spanish, and Simplified Chinese.

Files:

  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift

⚙️ CodeRabbit configuration file

ios/**/*.swift: Focus on Swift 6 strict concurrency, Sendable/MainActor correctness, Task cancellation,
retain cycles, memory pressure, SwiftUI observation state, StoreKit/APNs flows, and iOS background execution limits.
Pay special attention to BGAppRefreshTask and BGContinuedProcessingTask behavior, expiration handling,
bounded work, and cleanup when the app is suspended or terminated.

Files:

  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift
ios/**/*.{swift,plist}

📄 CodeRabbit inference engine (README.md)

ios/**/*.{swift,plist}: Use BGAppRefreshTask and BGContinuedProcessingTask when available for background processing, while allowing iOS to decide whether and when the app runs.
Implement APNs silent push handling for optional Cloud Relay wake-ups.

Files:

  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift
notify/**/*.{sh,go}

📄 CodeRabbit inference engine (README.md)

The optional notify sidecar must support server-side wake-up requests and must not receive notes, file or folder names, or vault structure.

Files:

  • notify/diagnostics_contract_model_test.go
  • notify/diagnostics_download_syncthing_e2e_test.go
notify/**/*.go

⚙️ CodeRabbit configuration file

notify/**/*.go: Review goroutine lifecycle, context cancellation, HTTP timeouts, signal handling, debounce behavior,
Syncthing REST API polling, relay API calls, error classification, and API-key handling.
Flag leaked request bodies, note metadata, Syncthing API keys, relay keys, or APNs-related secrets.

Files:

  • notify/diagnostics_contract_model_test.go
  • notify/diagnostics_download_syncthing_e2e_test.go
.github/workflows/**/*.yml

⚙️ CodeRabbit configuration file

.github/workflows/**/*.yml: Review CI for correct Go test tags, Xcode/iOS simulator assumptions, secret scoping,
dependency integrity, permissions, Docker publishing safety, and unnecessary privilege escalation.

Files:

  • .github/workflows/ci.yml
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: psimaker/vaultsync

Timestamp: 2026-07-15T09:11:23.730Z
Learning: VaultSync 2.0 remains NO-GO until the required helper publication, production rollout, rollback, real-device/PR gate, later download milestone, and causal roundtrip milestone are complete.
Learnt from: CR
Repo: psimaker/vaultsync

Timestamp: 2026-07-15T09:11:34.772Z
Learning: All Xcode results and derived data must remain outside the repository under `/tmp`.
Learnt from: CR
Repo: psimaker/vaultsync

Timestamp: 2026-07-15T09:11:34.772Z
Learning: Run the complete iOS plan, Release simulator build, design-token lint, string-key parity check, and sync-proof privacy lint before accepting the milestone.
Learnt from: CR
Repo: psimaker/vaultsync

Timestamp: 2026-07-15T09:11:34.772Z
Learning: Do not claim hardware keychain behavior, real APNs delivery, real background waking, TestFlight hardware installation, or real-device evidence when only simulator and isolated local Syncthing evidence exists.
Learnt from: CR
Repo: psimaker/vaultsync

Timestamp: 2026-07-15T09:11:34.772Z
Learning: Derive causal roundtrip only from this operation's upload and download legs after the PR and its review/CI gates complete.
📚 Learning: 2026-06-10T18:47:10.724Z
Learnt from: psimaker
Repo: psimaker/vaultsync PR: 38
File: ios/VaultSync/Views/ContentView.swift:605-611
Timestamp: 2026-06-10T18:47:10.724Z
Learning: In the SwiftUI codebase under ios/VaultSync, do not flag missing localization for SwiftUI string literals used as Text("…") or DisclosureGroup("…") titles/labels. In SwiftUI, these string literals are treated as LocalizedStringKey and resolve via the app’s Localizable.strings automatically—so they only need attention if the corresponding key is actually missing. Only require an explicit localization helper (e.g., L10n.tr(…)) when the string is not being passed through SwiftUI’s LocalizedStringKey path (e.g., plain String values provided to non-SwiftUI APIs).

Applied to files:

  • ios/VaultSync/Services/DiagnosticsPinnedTransport.swift
  • ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift
  • ios/VaultSync/Services/DiagnosticsResponseProtocol.swift
  • ios/VaultSync/Views/ControlledDiagnosticsView.swift
  • ios/VaultSync/Services/DiagnosticsPairingController.swift
📚 Learning: 2026-07-12T23:03:04.680Z
Learnt from: psimaker
Repo: psimaker/vaultsync PR: 107
File: ios/VaultSyncTests/DiagnosticsContractTests.swift:39-46
Timestamp: 2026-07-12T23:03:04.680Z
Learning: In iOS Swift tests that use CryptoKit’s `Curve25519.Signing.PrivateKey.signature(for:)` (Ed25519), don’t assert that a generated signature’s bytes exactly match deterministic “golden”/fixture signatures. CryptoKit signatures may be randomized (different but valid for the same key+message). Instead, verify correctness by calling `isValidSignature` (or equivalent) against (1) the golden bytes and (2) the freshly generated signature, and avoid byte-for-byte equality assertions between CryptoKit output and reference vectors.

Applied to files:

  • ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift
  • ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift
  • ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift
🔇 Additional comments (24)
.github/workflows/ci.yml (1)

120-120: LGTM!

Also applies to: 151-151, 165-165

ios/VaultSyncTests/DiagnosticsUploadM5Tests.swift (1)

349-355: LGTM!

Also applies to: 366-366

notify/diagnostics_contract_model_test.go (1)

296-296: LGTM!

Also applies to: 336-338

notify/diagnostics_download_syncthing_e2e_test.go (1)

1-121: LGTM!

ios/VaultSync/Views/ControlledDiagnosticsView.swift (1)

86-94: LGTM!

Also applies to: 103-103, 339-344, 548-568, 589-589, 598-598

ios/VaultSync/de.lproj/Localizable.strings (1)

917-930: LGTM!

ios/VaultSync/en.lproj/Localizable.strings (1)

917-930: LGTM!

ios/VaultSync/es.lproj/Localizable.strings (1)

917-930: LGTM!

ios/VaultSync/zh-Hans.lproj/Localizable.strings (1)

917-930: LGTM!

ios/VaultSync/Services/DiagnosticsCapabilityNamespaceProtocol.swift (1)

676-687: LGTM!

ios/VaultSync/Services/DiagnosticsResponseProtocol.swift (3)

1-84: LGTM!

makeAuthorization, validateResponseArtifact, and decode correctly chain digests across request → authorization → response, bound expiry to the tightest of the three, and select the signer (app vs. helper key) per message type. Field-label sets line up with expectedLabels and the golden-vector test.

Also applies to: 86-118, 120-147


169-192: LGTM!

freshResponseApply's combined event-id/path/action/error/wall-clock gate matches the documented download-evidence contract, and sign/validateAuthorizationChain/validateFields/validateClock/commonFieldsEqual are consistent with the equivalent patterns in DiagnosticsNamespaceProtocol/DiagnosticsUploadProtocol.

Also applies to: 194-306


149-167: 🩺 Stability & Availability

No issue: getEventsSince always returns JSON

SyncBridgeService.getEventsSince(lastID:) returns "[]" when idle, stopped, or on marshal failure, so an ordinary “no events yet” poll won’t be misread as a bridge failure.

			> Likely an incorrect or invalid review comment.
PRIVACY.md (1)

136-142: LGTM!

The evidence-boundary wording (fresh baselines, one signed authorization, conflict-on-invalid-file, roundtrip-remains-unset, simulator/isolated-Syncthing-only download evidence) matches the implementation and test assertions in this cohort, and correctly avoids overclaiming physical-device evidence.

Also applies to: 262-283

ios/VaultSyncTests/DiagnosticsControlledDownloadRuntimeTests.swift (2)

9-95: LGTM!

Exhaustive byte-tamper loop plus body/digest equality (not raw signature-byte equality) correctly follows the CryptoKit Ed25519 randomized-signature guidance from prior review learnings.

Source: Learnings


97-437: LGTM!

Stale/tampered/generation-changed/cancelled/restarted scenarios each correctly preserve uploadObserved while keeping downloadObserved false, matching the documented partial-result contract.

ios/VaultSyncTests/DiagnosticsForegroundUploadRuntimeTests.swift (4)

126-238: LGTM!

The M6 happy-path wiring (response-path derivation, respond closure writing the artifact + matching event, .downloadObserved/poll-count assertions) is internally consistent with DiagnosticsResponseProtocol's field/label contract.


273-274: LGTM!

Mechanical wiring of the new events: parameter across the late/restart/raced/rate-limited/timeout/rejected scenarios, plus widening waitForTerminalUpload's non-terminal set to include .uploadObserved, is consistent and doesn't change any prior assertions' meaning.

Also applies to: 319-320, 364-365, 392-431, 441-442, 481-482, 541-542, 632-634


694-699: LGTM!

LockedDownloadEventBox, iso8601WithNanoseconds, and makeHelperResponseArtifact are consistent with existing Locked*/test-fixture conventions in this file, and the artifact fields match DiagnosticsResponseProtocol's expectedLabels[.responseArtifact].

Also applies to: 717-800


827-827: LGTM!

ForegroundUploadTransport's new respond/authorizations state and @Sendable closure typing are correctly actor-isolated for Swift 6 strict concurrency.

Also applies to: 845-978

ios/VaultSync/Services/DiagnosticsPinnedTransport.swift (1)

47-47: LGTM!

docs/architecture.md (1)

50-61: LGTM!

Also applies to: 90-104, 124-134, 228-236

docs/m6-controlled-download-readiness.md (1)

1-89: LGTM!

ios/VaultSync/Services/DiagnosticsPairingController.swift (1)

27-52: LGTM!

Also applies to: 76-78, 392-434, 597-649, 651-731, 749-814, 906-936

Comment thread ios/VaultSync/Views/ControlledDiagnosticsView.swift
An engine restart between the two bridge reads would tag new-engine
events with the pre-restart generation and falsely pass the download
continuity check. Reading events first makes the generation a valid
witness: any restart before or during the event fetch fails the
caller's boundary check instead.
@psimaker
psimaker merged commit 60b446a into main Jul 15, 2026
31 checks passed
@psimaker
psimaker deleted the agent/app-controlled-download branch July 15, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant