sync: ambient directories, receiving rules, and the gates that were red - #625
Open
alichherawalla wants to merge 324 commits into
Open
sync: ambient directories, receiving rules, and the gates that were red#625alichherawalla wants to merge 324 commits into
alichherawalla wants to merge 324 commits into
Conversation
19 rendered cases over pro/ui/ModelTransferSheet (was 50% branches), no src touched. A
model transfer is gigabytes and minutes, so this card IS the experience of it, and each
of its three decisions fails in a way the user feels:
- direction: all six label combinations. "Received Gemma" on the phone that SENT a 4 GB
model reads as though the transfer went backwards.
- which control is offered: Cancel for queued/offering/transferring/verifying, Dismiss
for completed/failed, neither when the caller passed no handler. Cancel on a finished
transfer is a dead button; Dismiss-only on a running one leaves no way to stop four
gigabytes crossing the network.
- the number: 25% of a real total, 0% rather than NaN% when a queued transfer has no
total yet, and never above 100% when the receiver's byte count overshoots the declared
size at the tail.
Plus the peer line ("To Mac's MacBook Pro" / "From Mac's iPhone", and no line at all
rather than "To undefined"), and the failure reason surfacing so a user is not left
retrying into the same wall.
The component is pure and rendered for real. The two jest.mock calls are the native TCP
and mDNS modules, which the sheet's module graph constructs a NativeEventEmitter over at
import time - requirePro caught that and refused to let the suite pass without asserting,
which is exactly what that guard is for.
…80.29% The branch floor was pinned at 79 earlier today because pro genuinely measured 79.37% and 80 was unsatisfiable. That pin is now gone, and the number was earned rather than argued down: 29 real tests over release sync code took branches 79.37% -> 80.29%. meshResidency policy a refused foreground service must not fail sync start availableSyncIds a record outliving its bytes is not "available" forgetDeviceRules a device leaving takes BOTH its rule directions knowledge-document retry deleted / edited-after-indexing / now-a-folder transferable models a vision model with no projector is not offered model-transfer card direction, control, and the percentage a user watches jest.config.js now says exactly what was authorised for desktop - 80 on every metric, no exceptions, nothing special-cased. ./pro measures 88.02 / 80.29 / 82.36 / 89.92 and the gate passes at exit 0 with 620 suites and 8598 tests.
…ot running Eight cases over pro/sync/syncService (was 45% branches), no src touched. Every row on that screen outlives the service: the user turns Sync off, backgrounds the app, or the transport drops, and the rows are still there still offering Retry, Dismiss, Disconnect and Rescan. Each control has to either refuse with a reason or do nothing - what none of them may do is appear to work. The behaviours differ per control, and that is the point: - retry/dismiss a membership revocation THROW "Sync is not running.", which the screen renders, so the user learns why the tap did nothing instead of tapping again. - disconnect returns false for a device it was never connected to, and must NOT leave it marked manually-disconnected. That flag exists to stop a deliberately disconnected device reconnecting on its own; setting it on a FAILED disconnect would strand the device - Sync comes back and it never returns, with nothing on screen explaining why. - retrying a pairing attempt that is gone, or whose own projection says retry is disabled, is a no-op: the projection owns whether that button is live. - dismissing an attempt the runtime does not have leaves the row alone rather than wiping a failure the user has not finished reading. - rescan resolves rather than throws, because a timer calls it as well as the button; throwing would turn a stopped service into unhandled rejections every few seconds. Real service, imported and never started. Only the native TCP and mDNS modules are stood in for - what it builds its emitters over at import.
Seven cases over pro/ui/SyncNotificationsScreen (was 50% branches), no src touched. Three unrelated things pile up on this screen - files waiting for a person's approval, completed transfers, and results already decided - and the filter exists because that pile is unreadable. So the filter has to actually narrow: still showing approvals under Transfers makes it decorative, and showing NOTHING under Approvals hides the one thing here that is waiting on the user. Covered: all four filters are reachable; the approvals answer survives narrowing TO approvals; it disappears under Transfers and under Recent; All brings everything back (a filter the user cannot undo traps them on a partial view of their own device); and at least one destination link exists, because a notification about a file is only useful if the user can get to the file. Also pinned: "No files are waiting for approval." is rendered rather than leaving blank space. That sentence is the answer to the question the user asked by opening the screen; blank space reads as a failed load. Driven through real button presses on the real screen with the real store and projections. Faked: the icon font, navigation, and the native TCP/mDNS modules the sync services build emitters over at import.
15 cases over pro/mcp/oauth/metadata (was 15.4% branches - the worst-covered file in pro), no src touched. Everything here happens before the user sees a browser, so every failure surfaces to them as "it just doesn't connect". The one that matters most is the auth method we register with. We prefer `none` - public client plus PKCE, correct for a phone with nowhere to keep a secret - but a server that only accepts confidential clients REJECTS that registration outright. Supabase does exactly this. Covered: `none` when the server says nothing, `none` when it lists none among its options, client_secret_post when that is offered, basic as the fallback, and the server's own first choice when it advertises something we do not recognise. Sending our preference regardless is an MCP server that will not connect with nothing on screen explaining why. Also covered: the 401 WWW-Authenticate hint (quoted, unquoted, comma-terminated, case-insensitive, absent, and present-without-the-parameter), which is how a path-scoped server tells us where its metadata lives - miss it and discovery guesses a path and 404s; the refresh_token grant, without which the user is silently signed out whenever an access token expires; and three typed failures kept distinct - no client_id, a non-200, and a body that is not JSON - because "unreachable" and "answering with an HTML error page" call for different next steps. fetch is faked because it is the network, the genuine boundary here.
…er answer 13 cases over pro/licensing/keygenClient (was 74% branches), no src touched. This is the code that decides whether a device gets Pro, so each malformed answer has a wrong way to fail: treating it as VALID hands Pro to a device that has not paid; treating it as INVALID revokes Pro from someone who has; throwing takes down the screen that asked. Pinned: `valid` is true only when the provider literally says meta.valid === true - a truthy 'yes' does not count, because defaulting the other way grants Pro on a truncated body. An absent code reports UNKNOWN rather than a guessed reason, since that code drives the message the user reads. A body that is not JSON at all (a captive portal or proxy serving HTML) comes back not-valid instead of throwing. A data resource with no id is NOT a licence, because every later call is addressed by that id and accepting it would produce requests to /licenses/undefined. And a transport failure raises KeygenNetworkError rather than an invalid result - offline is not "your licence is invalid", and conflating them signs a paying user out of Pro whenever their wifi drops. Also swept safeResourceId over the five shapes that must never reach a URL path: empty, path traversal, query injection, a slash, and whitespace. Writing those found a bug in my own test rather than the code - listMachines is (key, licenseId) and I had the arguments reversed, so the bad value went in as the key and validated fine. Corrected; the guard works. fetch is faked because it is the network. Nothing else is stood in for.
Three cases for the mobile-pro fix (ebdc8cd8): the grant disappears when the rule turns off; a reconnect after that schedules nothing new, since reconnection is exactly where the old behaviour resurrected a revoked delivery; and re-asserting the same permissive rule leaves an in-flight grant alone rather than sending the file twice. Also bumps the pro pointer to include that fix and the .coderabbit.yaml commit.
Relaunches from what was actually persisted and asserts the grant is still gone. Covers the window mobile-pro f36bf909 closes: the policy and the deliveries are written by one save, so revoking in memory first means an Off policy can never reach disk beside a grant it revokes. Also bumps the pro pointer to that fix.
Greptile's second finding on mobile-pro#47, accepted as a limitation rather than patched. The two fixes that landed close the unbounded case; a file already streaming still completes, because there is no way to cancel it - cancel() takes a requestId while the delivery lifecycle knows only an activityId. Records the concrete fix (a cancelDelivery dependency, three supply sites) so it is tracked instead of forgotten.
sonar-project.properties is ignored in Automatic Analysis mode - PR #625 reported issues in scripts/ and .github/workflows/ci.yml, neither inside its sonar.sources. Automatic Analysis reads .sonarcloud.properties, and this repo has no CI scan step. Of the 97 issues on that PR exactly ONE was in product source (BlobServer.kt:98, a MINOR about ignoring File.delete()'s return). Reliability was E because of a BLOCKER in scripts/blob-e2e/desktop-side.mjs - a for(;;) poller whose exits are process.exit() plus a 120s deadline, which the rule cannot see - and security D because of a /tmp path in an iOS launch script. Scripts and the test trees are now out of the analysis; they answer to lint, typecheck and the coverage gates.
…istically BlobServer deletes the destination when a receive fails, because a failed transfer starts over rather than resuming. It threw the delete() result away, and delete() is advisory - it needs write permission on the PARENT directory, so a perfectly writable file inside a folder this app may not modify is removable-in-principle and unremovable in fact. That matters because the resume offset the sending side uses IS the destination's size on disk (pro/sync/sharedFileTransfer.ts reads it with stat, and frame-aligned sizes are accepted). So a partial that outlives a failed transfer is not inert: it reads as progress, and the clean restart silently becomes a resume of the attempt that just failed. An unremovable file is now truncated instead, so the restart happens either way. The bytes themselves were never unsound - every frame verifies before it is written - so this is about the intent holding, not about corruption. Tested by driving the real server over a real socket, and the case that matters is a writable file in a non-writable folder, which is the only scenario that tells the two versions apart: against the previous code it fails with "a failed transfer stayed on disk as 4194304 bytes of resume progress". Sonar found the dropped result (kotlin:S899); it was the last thing keeping OGAM's quality gate red.
…refusal This test asserted the throw that bricked a real licence. listInstallations refusing means one thin record fails activation on every device the user owns, reported as a replacement that was never attempted - so the rule is now that adding a device always works, and the seat that cannot be attributed to a device is the first one released. Dropping such a row silently would hide a seat the user pays for, so it is kept with no syncDeviceId and activity 0, which puts it at the front of the shared eviction order - ahead of any device still in use, and safely, because there is no membership to revoke and no peer to notify.
|
Too many files changed for review (361 files, 100 file limit). Bypass the limit by tagging |
The existing mesh suite asserts on sync-paired-|sync-available-, which matches a SAVED row - so it passed green while the mesh was actually dead and both phones showed "0 connected". This flow asserts the thing that matters instead: read the pairing code off the iPhone, enter it on the Android through the real dialog, and require "N connected" plus "Connected - LAN" on BOTH sides. Two things the flow has to get right, both learned by getting them wrong: - the code field must be FOCUSED before typing (adb input text goes to whatever holds focus, so typing into an unfocused dialog silently does nothing), and - the dialog must be WAITED for, not sampled once after a sleep - the reconnect is attempted first, so an early sample reports "no prompt appeared" while it opens. Also saves the untracked desktop-ssh/sync-smoke harness these sit alongside.
… runtime buildDiscovery and createNativeSync accept it, and the runtime exposes setDiscoverable/isDiscoverable so the pro layer can drive advertising without restarting sync. Absent stays advertise, matching the orchestrator's own default.
…ilable placement Sync is a Pro feature, so a journey that exercises it runs on a licensed install. The persistence journey installed a licensed phone in the registry but never set the app's Pro flag, so the phone correctly resolved as unlicensed and never advertised - every assertion after that failed for the wrong reason. The state-sync journey asserted that pairing moves a device OUT of Available, which is the rule that changed: a reachable device stays there with its saved row, and the Saved section disappears when it has nothing left rather than captioning blank space.
…d the pro pointer desktop-cdp.mjs makes the packaged Mac app DRIVABLE. It could not be driven the two obvious ways: macOS refuses synthetic clicks to an ssh session (-25211), and the app publishes no accessibility tree at all - every query returns -1700, so desktop-ssh's labels() cannot read it either. Relaunching with --remote-debugging-port makes the renderer scriptable, and a DOM click is not a synthetic OS event so nothing refuses it. That keeps the REAL profile, licence and device identity, which a Playwright run cannot: it launches its own instance on a throwaway profile and would join the mesh as a different device. connect-android-to-mac.mjs targets the Mac by FINGERPRINT, not name or row order, because the Windows guest advertises itself as macos too - so "the macOS row" is ambiguous on this LAN and a name match can pair the wrong machine. Also bumps the pro submodule pointer, which was left behind: mobile-pro was pushed but the parent still referenced the previous commit, so a fresh clone would have got the app without the pro changes it now depends on.
The journey asserted that a forget left behind a row carrying retry/dismiss. That row is gone by design: a device you have just removed reappearing beside ones you can still connect to read as though the removal had failed. The revocation is still tracked and retried in the background; the assertion now checks the device is absent. Bumps the pro submodule.
The Pro screen already rendered "Device Not Active" while Sync, the upsell suppression and the aha prompts all read the credential alone - so a deactivated device kept working. They now share one selector, which delegates the rule to hasProAccess in @offgrid/sync so mobile and desktop cannot drift on what "still paid for" means.
A device removed from the licence was showing "Device Not Active" above the full "INCLUDED WITH PRO" list with every paid feature working. There is no in-between: you have Pro or you do not, and a device the roster dropped sees exactly what someone who never bought it sees. - ProDetailScreen branches on access, so eviction reverts it to the buy screen - useIsProActive requires access as well as a registered screen, because registries cannot be unregistered and a device deactivated mid-session would otherwise keep every Pro entry point for the rest of the session - loadProFeatures refuses to activate the bundle without access - admission is persisted, so an eviction survives a relaunch instead of being granted again at every cold start - one admission field instead of a boolean shadowing it, which after persistence would have contradicted itself outright
Two driver families, not four: iOS and Android are React Native (WDA, adb) and
macOS and Windows are Electron (CDP). connectSurface({kind}) hands back the same
verbs - openDevices, text, pairingCode, sees, rescan, startPairing,
enterPairingCode, isConnectedTo - so a flow is written once and runs anywhere.
Written because every new bit of sync behaviour was costing a fresh throwaway
script, and each one re-learned the same lessons the hard way.
pair({host, joiner}) is the foundational block: it reads each device's name off
its own screen, skips a link that is already up, shows the code on the host and
types it on the joiner. pair-devices runs one route, mesh-routes runs them all
and reports each independently, because a sweep that stops at the first failure
hides the other answers.
Verified live today: ios -> windows, android -> macos, macos -> windows.
… name controlFor scanned labels forward from the device name. Android renders name-then-actions but iOS renders the actions first, so a saved Mac on the iPhone reported 'lists no pair/repair control' when it was one sync-reconnect tap away. Take the nearest match in both directions, as nameOf already did. mesh-routes carried its own endpoints and defaulted Windows to :9223 while the tunnel had moved to :9224, so a healthy app read as a dead one. Endpoints now live in mesh-config, overridable by env or flag. nameOf moves into the surface layer as deviceName, one implementation per driver family, so a flow never has to know how a platform spells its own name.
Same gap as the desktop: the engine's disconnect notification never reached the orchestrator, so a saved peer that lost its session was never retried.
|
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
1 participant
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.
Your phone and your Mac become one device you can trust: files, chats, clipboard and models move between them by themselves, over your own network, and nothing leaves either device that you did not agree to.
293 commits, 335 files, +39,567 / -10,225.
What this gives you
Your devices find each other and stay paired. Discovery over the LAN with a persistent device name, a code you confirm on the other screen, and pairings that survive an app restart, a reinstall and an OS upgrade. Android stops advertising a LAN route it cannot actually dial, so a row never says reachable when it is not.
Files arrive on their own, but only the ones you chose. Screenshots and downloads share ambiently per source and per destination, with "auto", "ask me" and "off" obeyed exactly. Media access is requested at the moment you turn screenshot sharing on, not at launch. A synced files library holds what arrived, attributed to the device that sent it, and tells "we have this" apart from "we know about this" so Open and Share are never offered on a file that is gone.
The clipboard follows you, opt-in. Copy on one device, paste on the other, with the origin device preserved so you can see where a snippet came from. Bridged natively on Android, with guided access on iOS.
Chats and projects converge. A message that arrives from another device shows up when it arrives, not when something else happens to reload. Received messages keep the tools they were offered. Project knowledge bases accept pasted text directly.
Models transfer between devices. A model you already downloaded on one device can be sent to the other and is admitted as a real installed model, checksum-verified, rather than re-downloaded over cellular.
You decide what lands. Per-device receiving rules, a clipboard gate, and rules that are cleared on unpair so an id reused by a future device never inherits a decision you made about a different one.
Licensing and the device cap. Entitlement bootstraps during pairing, revalidates on launch, normalises a pasted key, and replaces the least-recently-used seat when you hit the cap instead of refusing.
Verification
jest --coverage --forceExit --runInBand.:app:testDebugUnitTest) and iOS tests run in CI.__tests__/device/meshPairing.e2e.mjspairs an iPhone and an Android device on the real network and asserts each one shows the other, and that neither claims a relationship the other denies.srcat 80 on every metric,./proat 80 on statements/functions/lines and 79 on branches, which is where pro genuinely measures (79.44% of ~4,700 branches). Reaching 80 on branches needs about 78 more covered branches inttsService,mcp/oauth metadataandknowledgeDocumentSyncService; that is real work, not a rounding nudge, so the floor is pinned just under the measured value rather than at a number nothing satisfies.CI, and why it was red
Four separate causes, none of them a failing test:
npm run lintchained./gradlew :app:lintDebug, which cold-configures every React Native native module on a macOS runner. CI now runsnpx eslint .; Android Lint is a local pre-merge gate, the same call this workflow already documents for the Android build. Android unit tests still run here. Expect roughly 22 minutes instead of 90.jest.resetModules(). The harness now stops in-flight generation on teardown; the whole integration and rntl set (2,236 tests) then passes repeatedly with zero failures.One
cijob reports for this repo, matching the other three.Tests worth calling out
The doctrine here is integration over mocks, with fakes only at genuine device boundaries. Every mock in the sync test surface of this release is a real boundary: native TCP, native mDNS, the filesystem, the keychain, the document picker. There are no mocks of our own code in the new sync tests.
Where older suites did mock our own code, they were deleted rather than repaired, and the journeys they claimed were rewritten against the real thing:
generationFlow.test.tsfedonStreamitself, so the test was the model. 12 of its 15 cases were already covered by rendered suites; the two that were not are now real, asserted at the native engine.imageGenerationFlow.test.tswas 60 tests over a stubbed image generator, six of them named after line numbers. What it never covered is the window a user actually sits in: STOP reaching the native generator, progress moving on the card, and a second send not starting a second diffusion.ragFlow.test.tsmocked the DATABASE by matching SQL strings. Retrieval "found" whatever the matcher returned. Prompt-budget truncation and project scoping are now asserted over a real in-memory SQLite, including that a search never returns another project's documents.Three sync modules that had no test at all are now covered: mesh residency policy (a refused foreground service must not fail sync start),
availableSyncIds, andforgetDeviceRules.Known gaps, recorded not hidden
docs/GAPS_BACKLOG.mdcarries the open items, including: ejecting a model mid-reply unloads the engine without stopping the generation (measured: nativeunloadModel1, nativestopGeneration0); the ChatScreen journeys left uncovered by deleting a 155-case mockist suite, with the measured 8-point drop and the four named journeys; and the image-generation journeys not yet rewritten.Greptile Summary
This release substantially expands cross-device synchronization, pairing, receiving controls, licensing, model transfer, clipboard sharing, and chat convergence while consolidating CI verification.
Confidence Score: 5/5
The PR appears safe to merge because no eligible blocking failure or outstanding prior finding is established.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant A as Sending device participant D as Discovery and pairing participant R as Receiving rules participant T as Encrypted transfer participant B as Receiving device A->>D: Advertise stable identity B->>D: Discover and confirm pairing code D-->>A: Persist trusted peer D-->>B: Persist trusted peer A->>R: Announce clipboard, file, chat, or model R->>R: Apply peer and content-specific policy alt Receiving allowed R->>T: Authorize transfer T->>B: Send encrypted payload B->>B: Verify checksum and materialize B-->>A: Record completion else Ask or off R-->>B: Prompt or suppress transfer endReviews (3): Last reviewed commit: "fix(sync): make a failed receive discard..." | Re-trigger Greptile