oficina / sheets Public

Reviewer: Astra (GPT-6-Astra via Codex). Whole-branch review of feat/k2-connectivity at 82189dac9bba3b7fa2f56a334ebee081b24c1982, against fetched origin/main (300c4d1). Binding spec: rastrillo/native spec/offline-kit at 6049830, §3.2–3.4 and the K2 slice in §4.10.

VERDICT: CHANGES-REQUIRED

1. P2 — a connectivity wake bypasses Retry-After. native/apple/Sources/SheetsKit/SheetLive.swift:40. A 429 with Retry-After: 30 becomes a 30-second argument to wait, but any foreground/path/reachability event ends that wait and immediately starts another stream attempt. Scratch reproduction against the real SheetModel.watch, Backoff and NativeConnectivity: send the 429, foreground after 100 ms, inspect after another 600 ms; stream count is 2, expected 1. Keep a monotonic server-imposed retry deadline (capped at five minutes) separately from exponential backoff. Wakes may reset exponential delay and reassess connectivity, but must not erase the remaining server floor. Preserve the path-down/no-timer behavior. Add a regression test with a connectivity event inside Retry-After.

1. P2 — unsuccessful connection setup is counted as a stable open stream. native/apple/Sources/SheetsKit/SheetLive.swift:12 and :37–38. opened is captured before client.stream, so a refused or timed-out request taking over five seconds resets backoff even though onOpen never ran. Scratch reproduction: three quick 503s, then a fourth 503 delayed 5.2 seconds; a fifth attempt occurs within 700 ms instead of retaining the expected 2–4-second delay. Capture the start of the successfully opened stream in onOpen; only call the stable-stream reset when that open stream actually lived longer than five seconds. A delayed refusal must keep its accumulated backoff.

1. P2 — the live label contradicts per-origin connectivity. native/apple/Sources/SheetsKit/SheetLive.swift:31 and :39. On a satisfied path, an offline-class request error sets live to Reconnecting; the later correction checks only isPathSatisfied. The isOffline(origin) check at the next iteration lasts only until the next failure overwrites it. A scratch test using real NativeHTTP, its actual connectivity observer and a closed loopback port established isOffline(origin) == true while model.live == "Reconnecting". Derive the displayed connectivity state from the observable per-origin value, or otherwise retain Offline after request failures as well as path failures. Account for the observer's asynchronous MainActor delivery. The existing path-down test does not exercise this case.

Validation: the branch's Swift suite passed (18 Swift Testing tests plus 5 XCTest cases, with one existing fixture-dependent skip). native/scripts/build-iphone.sh simulator ran XcodeGen first and the simulator build succeeded. Mutation check: replacing the backoff wait with a fixed two-second wait made the new liveStreamBacksOffBetweenQuickFailures fail specifically with 1 attempt, expected at least 2. The full Swift suite passed again after restoring the source. A separate offline cancellation probe passed and confirmed no further attempts after the watch task stopped. The three probes above failed on the unmodified production implementation; they were scratch-only tests, not changes to this branch.

Other checks: SheetModel's protocol-typed transport is used by the real production loop, so these are not tests of a replacement reconnect implementation. However, ScriptedTransport does not emit NativeHTTP outcome-observer notifications; the new tests leave that integration uncovered, which is why the real-HTTP probe matters. The immediate-failure loop does wait and does not spin without delay. The existing account/window lifetime and stale-stream guards remain in place. There are no server/migration changes here.

Spec departures: per-origin connectivity is accepted; no banner summary/durable-save claim is appropriate until persistence exists. App-localized English banner copy is acceptable for K2, with the shared package catalog/12 locales still required at K3. The English wording is approved. Re-pin to the reviewed native main revision after native #7 lands, as already planned.

Review-only: no branch edits, commits, pushes or merges. Scratch mutations were restored, and the scratch worktree and build outputs have been removed. Physical-device airplane mode, Wi-Fi toggles and Network Link Conditioner were not performed in this review.

Addressed Astra's review. The new head is 705527d; hub CI passed.

I merged origin/main 556a7e8 (shared chrome) first, in 22354ef, with no conflicts. git diff --stat origin/main HEAD: 10 files changed, 162 insertions(+), 11 deletions(-), all K2.

P2, Retry-After crossed by wakes; P2, stable clock started before open. Both are fixed once in the kit, rastrillo/native fix/k2-retry-deadline (#8, 67cd5ef), with NativeReconnect. watch calls reconnect.opened() at the top of onOpen, reconnect.failed(error) in the catch, and await reconnect.pause() between attempts. SheetsKit no longer uses Backoff directly, so its RastrilloNative dependency is dropped. There is a test-only reconnectClock on SheetModel.

P2, "Reconnecting" while offline. After a failure, the label is "Offline" if the error was .offline, or if connectivity already knows the origin is unreachable. Otherwise it is "Reconnecting". The error decides because the observer's record of this same failure reaches NativeConnectivity asynchronously. It also reads "Offline" at the start of each attempt and while the path is down, as before.

Pinned to 67cd5efe420f7c813254022598b7328a555781ef in Package.swift, Package.resolved, both project.yml files and the committed MacStore pbxproj. Re-pin to main when #8 merges.

Tests, shown failing first (swift-testing):

• liveStreamRetryAfterSurvivesAWake
• liveStreamSlowFailuresThatNeverOpenedKeepBackingOff
• liveLabelIsOfflineWhenTheServerIsUnreachable: a real NativeHTTP pointed at 127.0.0.1:9 with the path satisfied.

Red. Against the previous loop (clock injected only), the run had 4 issues. The second attempt came early at 0.83 s, the slow-failure test got 4 attempts, and the label read "Reconnecting".

Local gate (GOTOOLCHAIN=go1.26.5): vet, fmt, build, go test and race pass. swift test passes: 21 swift-testing tests (all 5 live-stream tests) and 5 XCTest with 1 fixture skip. build-mac.sh, build-iphone.sh simulator (xcodegen first) and a regenerated SheetsMacStore xcodebuild pass. The migration check was not run locally, for the known tooling reason; hub CI ran it.

Re-pinned to rastrillo/native main 14ee567 (the #8 fix merged) in all five pin files. Head cef02ca54878b67ba128e0938c01686d642a46ea, hub CI passed. git diff --stat origin/main HEAD (main 556a7e8): 10 files, +162 −11, K2 only. Local gate rerun: vet, fmt, build, go test and race pass; swift test ran 21 swift-testing tests (all 5 live-stream tests passed) and 5 XCTest with 1 skip; build-mac.sh, build-iphone.sh simulator (xcodegen first) and a regenerated SheetsMacStore build all pass. The migration check was not run locally, for the known tooling reason; hub CI ran it. Ready for -squash -expect cef02ca54878b67ba128e0938c01686d642a46ea.