Repository navigation
fix: privacy and security gaps from post-merge review (#44) - #47
Conversation
Profile cloud push runs only when the user enables backup after an in-app consent step. Autosave stays local-only by default. identityToCloudPayload sends sync fields plus public contact fields only. Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
ucguy4u
left a comment
There was a problem hiding this comment.
Changes requested and blocking
Head reviewed: 343e247 (re-fetched immediately before posting; draft, 7 commits, base main 6d9b087). This is the fix PR for my post-merge review 5742517935 on #44. Flagged for owner (human) review: it touches consent, personal data, secrets storage and webhook authentication. Not an approval.
CI on this exact SHA: run 41536138845 is completed/success with headSha=343e2476. The flutter, rust and worker jobs are all success, and the check-runs API shows the same three for the SHA. CI has no Android build, no tsc and no manifest check, so those are not covered by it.
How I verified. I read the whole diff. The app uses the Thermion native build, which can't compile on my box, so I ran the app suite in a copy with avatar_renderer stubbed (174/174 pass, same as CI) and ran the mutation passes there. I also drove real behaviour with throwaway probes: a local fake-Supabase HTTP server, a widget test of the Identity screen, and the worker's fetch() with a mocked upstream.
What holds up (verified in code and probes)
- B1 consent gate.
- There is no path that uploads before consent on this head.
push()re-checks the persisted opt-in itself (supabase_profile_cloud.dart:44), as does the autosave site (identity_edit_screen.dart:148). Probe: typing for 2 s, then Save, then Not-now each caused 0 pushes and 0 HTTP requests. Accept caused 1 push. pull()at launch is also gated (main.dart:53).- The allowlist is real. Over HTTP, the upload had keys
[id, displayName, username, privacy, website]. Private mobile, X, Instagram, email and bio were absent, and the public website was present.revenuecat_app_user_idis no longer sent.
- There is no path that uploads before consent on this head.
- B3. Nothing at launch reaches RevenueCat.
StoreScreen(isActive: _index == 2)(app.dart:82) means the mounted-but-hidden Store does nothing. The onlyPurchases.*callers are inpackages/store. The Share and Encounter screens only watch the provider, which returns{}until activated. - B4. No
GoogleFonts.*call remains inapp/lib. OnlyallowRuntimeFetching=falseatmain.dart:28. Fonts are bundled and the families resolve. - B5. The wire contract is
schemaVersion,remoteIdentityId,displayName,activeAvatarConfig, plus the optional lists. The shape is unchanged, so old receivers are fine. Empty lists validate (amiro_sharing_payload.dartvalidate), andEncounterComparisonandIdentityCardrender a minimal payload. - Migration. On success, both legacy plaintext files are deleted after the secure write (mutants M1 and M2 were killed). If the secure write throws, the legacy files are kept, so there is no data loss.
- Auto Backup.
allowBackup=false,fullBackupContent=falseanddataExtractionRulesare present. Both XML files are well-formed. The rules exclude sharedpref, database and file, which cover both the Isar files underfiles/and Keystore-wrapped prefs. They are redundant withallowBackup=false. Not build-verified. - Webhook. The HMAC payload is
t.plus the body, which matches RevenueCat's documented scheme. Skew is ±300 s. I confirmed valid=200 and stale, future, tampered, missing and malformed signatures=401 with 0 upstream calls. Authorization is required when configured./crashfails closed whenCRASH_PATH_TOKENis unset. The out-of-order nit still applies unchanged: the grant upsert stampsgranted_at=now()and ignores event time (index.tsgrant block).
BLOCKING
1. The constant-time compare introduces a fail-open on the webhook (security.ts:1-13 used at index.ts:106).
TextEncoder.encode(undefined)is empty, soconstantTimeEqual('', undefined)returns true.- With
WEBHOOK_PATH_TOKENunset (or empty),POST /webhook/returns 200 and performs the service-role upserts. I ran this against the head: 200 with 2 Supabase calls. The same request on main returns 404 with 0 calls. - Separately,
WEBHOOK_AUTHORIZATIONandWEBHOOK_SIGNING_SECRETare both optional. With neither set, the worker silently stays path-token-only (200 for a correct path), so "fails closed" is false for the new layers. RevenueCat's own guidance is to configure at least one. - Fix:
- Reject when
!env.WEBHOOK_PATH_TOKEN. - Return 500, with a log line, when neither Authorization nor the signing secret is configured.
- Add tests for both.
- Reject when
2. App startup now depends on the Keystore for every user, including those who never opt in (main.dart:45).
await profileSecrets.migrateFromLegacyPlaintext()runs unguarded beforerunApp. It calls_secure.read()first (profile_secrets_store.dart:25), even when no legacy file exists.- Probe: a throwing secure storage, with no legacy files, makes
migrateFromLegacyPlaintextthrow. Inmain()that means the app never starts. A failed write also throws, though the legacy files are kept. - Fix:
- Check that the legacy files exist before touching secure storage.
- Wrap the migration in try/catch, and on failure keep the files and continue with backup unavailable.
- Apply the same guard to the
recoveryCode()call ininitState.
3. Revoking consent does not remove or even mention the server copy (identity_edit_screen.dart:184-218).
- Turning the switch off only writes the flag. I confirmed 0 requests and no DELETE afterwards. There is also no delete grant or policy in
supabase/migrations. - The row (name, avatar, public fields, recovery code) stays on the server. The copy says "You can turn this off anytime" (
:196) and the toggle text says nothing about the existing copy. - Minimum fix: say plainly that the existing backup stays until deletion is requested, and give the deletion route. Preferred: delete on revoke (needs a migration). The owner decides which.
- Related: pre-fix server rows still hold private fields and the RevenueCat id. They are overwritten only if that user opts in, so the owner needs a purge or migration plan.
4. The tests do not protect the claims. I ran 26 app mutants and 11 worker mutants.
App mutants that SURVIVED (the suite stayed green):
- Both consent guards removed (C3), and each guard individually (C1, C2).
- The consent dialog skipped (C6), or Not-now treated as accept (C7).
- Opt-in default flipped on for a corrupt file (C5).
mobileuploaded unconditionally (A2), or the X, Instagram and website handles uploaded unconditionally (A4).- The Store tab always active at launch (R3, the B3 regression).
- Legacy files not deleted on later writes (M4, M5), and secrets written back to plaintext (M6).
- Deleting the legacy file before the secure write (M3).
- Cosmetics always sent (S1), and the progress default flipped on in JSON (S4).
Killed: C4, A1, A3, A5, R1, R2, R4, M1, M2, S2 and S3.
Why the survivors are not caught:
push is a no-op when backup is disabledis non-discriminating. With the guard removed it still returns false, because the network call to example.supabase.co fails and is caught.runtime font fetching is disabledsets the flag and then asserts it.- No test covers the autosave gate, the dialog, revocation, or the app.dart launch path.
Worker mutants that SURVIVED:
- Signature check skipped (W4).
- Timestamp window removed (W6), because the test passes a skew of 999999999.
fetch()ignoring the auth result (W8)./crashtoken check removed or fail-open (W9, W10).- A prefix-match signature compare (W11).
- The
constantTimeEquallength check removed (W2).
W1, W3, W5 and W7 were killed. The tests only call verifyWebhookRequest and never fetch().
Required before clear:
- Cloud-level test against a local HTTP server (as I did): zero requests while disabled, an allowlist assertion over every contact field, and no upload after revoke.
- Widget test of the autosave gate and the dialog.
- A launch test that mounts the app tabs and asserts no
setupPurchasescall. - Migration failure tests.
- Worker handler-level tests: unset token, neither secret set, stale signature, missing signature,
/crash.
Non-blocking
- iOS:
FlutterSecretStorageuses defaults, so Keychain items persist across uninstall and travel in backups and device migration (Android Auto Backup is off for the same data). SetIOSOptions(accessibility: first_unlock_this_device). Do this before any iOS release. claim()is not consent-gated (supabase_profile_cloud.dart:101). It creates an anonymous session and joins the row. It is user-initiated, but it should be stated in the policy, and it leaves backup off.- Share progress is understated until the Store has been opened this session.
ownedCosmeticsProvideris{}until activated (share_screen.dart:132), so a user with the progress toggle on shares wrong counts. Also,ensureLoaded()has an unhandled error path (state staysAsyncLoading). tsc --noEmitnow fails with TS5097 (index.ts:4,allowImportingTsExtensionsmissing). wrangler bundling is fine and CI doesn't type-check, but fix it.- The HMAC is computed over
request.text()re-encoded, not raw bytes. A BOM-prefixed body signed over its true bytes returns 401 (fail-closed, edge case). UsearrayBuffer(). - Worker hardening:
- The body is read before auth.
- 404 versus 401 reveals which layer failed.
- Authorization-only has no replay protection (this should be noted in the README).
- Only the last
v1is used.
- Fonts:
InstrumentSansandJetBrainsMonoare variable fonts (fvar) declared without weights. Check w600 rendering on a device.- OFL texts are not added to
third-party-licenses.md. - The
google_fontsdependency remains only for the config line. isConfigured(revenuecat_entitlement_store.dart:19) is unused.
- A receiver replaces
observedCosmeticsandobservedCollectionson update, so a sender who turns sharing off erases the prior look in the receiver's passport. Probably acceptable, but say so. pubspec.lockcarries incidental bumps of test packages.TODOS.mddrops the "no signature verification" note without describing the new auth.
Docs now stale, from the #44 Pass 2 lines on main 6d9b087 (the docs owner's pass must cover these)
- privacy-policy .md/.html:
- :5-10, :33 (backup is now opt-in, so "optional" becomes true, but say what is uploaded).
- :39 ("On first launch", still wrong).
- :44-45 (the code is in Keystore/Keychain, not "on your device" in general).
- :46-48 (RevenueCat id in the backup is gone).
- :60-62 and :72-74 (what the share carries; display name is always included).
- :89-91 and :103-104 (RevenueCat is now configured when the Store tab opens, not only on purchase).
- :112-114.
- :120-125 (Auto Backup is off; revocation leaves the server copy).
- :129-131.
- data-safety.md: :16-18, :22-25, :33-36, :57-58, :69-70, :84-88. The Supabase rows become optional.
- store-listing.md: :23-25, :29-38 (the toggles are now the cosmetics and progress switches; phone, handles and website still don't exist), :108-115.
- workers/revenuecat-webhook/README.md: :24, :28-42 (add the Authorization and signing secrets and
CRASH_PATH_TOKEN), :8-9. - TODOS.md:154 (the live Play form's "optional" RevenueCat answer).
Scope
The diff is in scope for B1, B3, B4 and B5, plus the owner-approved storage and webhook items. The CI worker job and docs/protocols/encounter-share-data.md are additive and fine, and the doc is accurate for the wire contract. No collision with the Pass 2 docs, since the PR doesn't touch docs/legal. It does leave them inaccurate, as listed above.
Verdict: Changes requested and blocking. Blockers: 1 (webhook fail-open), 2 (Keystore on the startup path), 3 (revocation), 4 (tests). The rest is sound. Nothing here is an approval, and merge and release decisions stay with the owner and PR Governor.
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Extract cloud backup consent and confirmCloudBackupEnable; add widget/unit tests for C6/C7 and C2 push gate. Add launch provider overrides, in-memory backup prefs for stable tests, amiroRootTabStoreScreenForIndex for R3, and lazy Store avatar/RC init when tab is active. W11 prefix-signature tests and expanded mutant runner patterns. Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
ucguy4u
left a comment
There was a problem hiding this comment.
Changes requested and blocking — architect re-review of 3d9a8b9f5dc2f991b104849ca9d12b5e0068a601 (live head, re-fetched before posting; draft, base 6d9b0878). This supersedes my BLOCKED review 5744469051 at 343e2476. COMMENT review only: not an approval. Flagged for owner (human) review: this is a privacy, deletion-semantics, secrets-handling and webhook-auth change, so a human must sign off whatever the verdict.
I exported this exact SHA and ran it myself. I did not trust the PR's own mutant script. Flutter tests ran in a scratch copy with the native-asset thermion renderer stubbed (the box has no C toolchain): baseline 198/198 pass. Worker tests: 19/19. tsc --noEmit on the worker: exit 0.
Verdict: two blockers. The first is the headline claim of this PR.
Blocking
B1. revokeBackup() can report success when nothing was deleted, so the UI says "deleted" while the server copy stays. app/lib/identity/supabase_profile_cloud.dart:107-118 treats any 2xx as success. PostgREST answers a DELETE that matches zero rows (including rows RLS hides) with 204. The new policy is correctly owner-only (supabase/migrations/20261102100000_profiles_delete_owner.sql:5-8, owner_id = auth.uid()), so these cases delete nothing but return success:
- a device that joined through a recovery code (it is a member, not the owner) and turned backup on;
- any session that was silently replaced.
_ensureSession(:268-271) swallows every refresh failure, including a transient 5xx, and signs in as a brand-new anonymous user, whose DELETE then matches nothing.
In both cases identity_edit_screen.dart:220-241 goes on to clear the opt-in and show no error, and the dialog copy (cloud_backup_consent_dialog.dart:98, identity_edit_screen.dart revoke dialog) promises the server copy is deleted. I reproduced it: a fake server answering 204 to DELETE gives revokeBackup() == true (probe P3). Fix: send Prefer: return=representation (or count=exact) and require at least one deleted row. If zero rows were deleted, re-select; if the row is still visible, return false and show an honest message (for example "only the device that created the backup can delete it"). Do not clear the opt-in in that case. Add a test for the 204-with-nothing-deleted case.
B2. The two safety invariants this PR claims are not protected by any test. Mutation results from the full suite on this SHA:
- M3 (delete the legacy plaintext file before the secure write) survives. The "continues when migration throws" test (
test/identity/profile_secrets_migration_test.dart:32-43) uses storage whosereadthrows first, so it never reacheswrite. Fix: storage with a workingreadand a throwingwrite, asserting the legacy file still exists. M5 (writeRecoveryCodeleaves the legacy file) also survives. - N1 (timeout removed) survives: nothing hangs the storage.
- N5 (opt-in cleared even when DELETE fails), N4 (non-2xx treated as success) and N6 (confirm dialog skipped) all survive. There is no widget test of the revoke path at all (confirm, fail, retry). The success path is only covered at the
SupabaseProfileCloudlevel.
The code is correct on the migration points I probed (below), so these are test gaps, not behaviour bugs. But the 5744469051 findings were exactly "claimed safety, no test", and these are the same class. Please add the tests, then re-run the mutants.
Verified fixed (against the code, not the PR text)
- Webhook fails closed. Empty or unset
WEBHOOK_PATH_TOKENgives 500 (src/index.ts:122-126). NeitherWEBHOOK_AUTHORIZATIONnorWEBHOOK_SIGNING_SECRETgives 500 (:128-132).constantTimeEqualrejects null/undefined and checks byte length (src/security.ts:1-22). The HMAC runs overrequest.arrayBuffer()bytes (index.ts:162,168;security.ts:70-75). The 500 body is a fixedWebhook not configured. Variable names go only toconsole.error, so nothing leaks to callers. Nit: the config check runs before the path-token check, so any POST to any path learns "misconfigured". It is harmless, but checking the token first would be tidier. - Migration cannot lose secrets.
profile_secrets_store.dart:34-49deletes the file only after a successfulwrite. I probed a storage whose write takes 600 ms with a 100 ms timeout (P1): at timeout the legacy file is still there and secure storage is empty. The un-cancelled migration finishes later and the file is gone with the value stored. So a timeout loses nothing, and a failed write leaves the file for the next launch. The "skip when no legacy files" guard works (:97). - Auto Backup rules, bundled fonts,
allowRuntimeFetching=false, lazy RevenueCat are unchanged and still correct. The Keychain accessibility fix is in (secret_storage.dart:114-118,first_unlock_this_device). TS5097 is fixed (tsconfig.jsonhasallowImportingTsExtensions;tscpasses). - RLS. The delete policy cannot touch other users' rows. It is the only DELETE policy and RLS is on.
profile_membersrows cascade (on delete cascade), and the cascade runs as table owner, so it works. - Offline DELETE. It returns false (
_ensureSessionor the request throws, caught at:119). The toggle stays on, the error text shows, and the user can retry. No stuck state.
Server-side leftovers after a successful revoke (docs must say so)
- The anonymous
auth.usersrow stays (no PII, but a persistent id). revenuecat_events(fullraw_event,app_user_id) andentitlement_grantsare never touched. They are keyed by the RevenueCat id, not the profile. That is the purchase audit, by design, and RevenueCat holds its own copy.- No encounter or discovery tables exist server-side (migrations: entitlements, crash_reports, profiles, profile_members only).
crash_reportshas no identity link. - An owner's revoke cascades to every joined device's access. The copy says "your profile copy" only.
Non-blocking
- Startup can still throw in one case.
main.dart:49-52callsprofileCloud.pull(), andpull()(supabase_profile_cloud.dart:88) callssecrets.readSessionJson()outside the try. It runs when the user is opted in but has no local identity, and it throws on a Keystore failure. It can also throw after a timed-out migration: the late migration andreadSessionJson's own migration race and the loser throwsPathNotFoundException(probe P2). Wrap it. - Local secrets survive revoke. The session and recovery code stay in Keystore, and the code stays on screen (
identity_edit_screen.dart:440-443) after the server copy is gone. Delete both on successful revoke and clear_recoveryCode. - Stale plaintext remains if secure storage already has a value (
profile_secrets_store.dart:34,43): the legacy file is not deleted in that branch. docs/legal/third-party-licenses.mdrow andapp/assets/fonts/OFL.txtare a pointer only. There is no OFL 1.1 text and no per-font copyright lines, which OFL requires to travel with the fonts. Nothing registers them withLicenseRegistry. Add the full text and copyrights.- Surviving mutants, minor. W7 (HMAC over decoded text instead of raw bytes) survives: add a BOM or invalid-UTF-8 body test. S4 (
fromJsondefault for collection progress) survives. R3b survives:AmiroRootTabsis never mounted by any test,lightweightTabsForTestandomitThermionOverlayForTesthave no caller, and R3 only calls the helper. N7 (DELETE with no identity filter) and N8 (catchErrorremoved) survive. N9 (empty-guard) is equivalent. N10 (main.dart) is not unit-testable. - CI cannot see the TS5097 or Android fixes. The worker job runs
npm testonly. There is notscstep and no manifest/XML check. Addtsc --noEmit. - The delete policy uses bare
auth.uid(); the later policies use(select auth.uid()). Make it consistent for the planner. - Scope.
scripts/run-review-mutants.mjs(review tooling, with hard-coded string patterns that will rot) and test names like "mutant C1" belong in a separate PR or none. The rest of the 37-file diff matches the task. It touches no #44 doc except one row inthird-party-licenses.md, so it does not collide with the Pass 2 docs.
Mutants (reran my original 37 on this SHA, full suite per mutant)
App, original 26: 23 killed, 3 survived.
- Killed: C1–C7 (C7 split into helper and dialog, both killed), A1–A5, R1, R2, R3, R4, M1, M2, M4, M6, S1, S2, S3.
- Survived: M3, M5, S4.
Worker, original 11: 10 killed, 1 survived.
- Killed: W1 (null guard), W2 (length check removed, killed with the PR's full-removal form; my weaker variant that checks only one direction survives, which is expected), W3 (path token), W4 (signature skipped), W5 (Authorization), W6 (skew), W8 (auth ignored), W9 (/crash), W10 (neither-secret 500), W11 (prefix signature), plus a new W12 (empty path token) killed.
- Survived: W7.
New mutants for this diff: 11, of which 2 killed and 9 survived.
- Killed: N2 (safety catch rethrows), N3 (revoke makes no DELETE).
- Survived: R3b, N1, N4, N5, N6, N7, N8, N9, N10.
Total: 36 killed of 48 run (the PR's own script reports 16 killed on its subset, but it only runs targeted tests, not the full suite).
CI (checked myself, not from the PR text)
Actions run 41552378596 on exactly 3d9a8b9f5dc2f991b104849ca9d12b5e0068a601, event pull_request: flutter, rust and worker all completed / success. No queued, skipped or cancelled checks. CI does not run tsc, Android builds or manifest checks (see item 6). Mergeable and CLEAN, but still a draft.
#44 doc lines that need updating once this merges
docs/legal/privacy-policy.md (and the .html twin; keep them identical):
:9-10and:36-42: backup is now opt-in, off by default, behind a consent dialog. "On first launch… may create an anonymous session and upload" is false.:46-48: the backup no longer stores the RevenueCat id. Remove the sentence.:44: the recovery code is stored in Keystore/Keychain, not just "on your device".:84-100: RevenueCat is configured lazily, when the Store tab is opened or a purchase/restore is started, not at launch. Add that.:116-119: editing a field only updates the cloud copy if backup is on.:122-125: add the in-app "turn off backup deletes the cloud copy" path, and what stays (the leftovers above). Keep the email route for purchase audit rows.- Add the encounter-share wording: optional cosmetics and collection progress, off by default.
- Fonts are now bundled with no Google Fonts fetch. Say nothing that implies a Google request.
docs/legal/data-safety.md: :22-23, :33-36 and :58 (all "optional Supabase backup" rows: still optional, now opt-in and consented); :57 and :70 (RevenueCat id only once the Store is used); :82-87 (the deletion answer moves from "Partially / email only" to in-app deletion for the profile, with purchase audit rows by request); :9-11.
docs/legal/store-listing.md:24: "optional cloud backup… recover after reinstalling" should say it is opt-in, and that Android system backup is disabled.
workers/revenuecat-webhook/README.md: :24 and the Secrets section from :30 (still lists only WEBHOOK_PATH_TOKEN). Document WEBHOOK_AUTHORIZATION, WEBHOOK_SIGNING_SECRET, CRASH_PATH_TOKEN, the fail-closed 500s, the 5-minute timestamp skew, and that replay within the window is not rejected beyond the event-id dedupe.
TODOS.md: :155 ("Worker has no request-signature verification" is now false), :146 and :154, and :59 ("No privacy toggle for the collection" is now false). README.md and packages/discovery/README.md need no change.
Not changed by me. I did not merge, push or mark the PR ready. Review run on this SHA only; any later push voids it.
Revoke returns RevokeBackupResult and uses Prefer return=representation so 204/empty bodies cannot count as success; clear Keystore credentials only after rows delete. Refresh failures only replace the session on 401/403. Add revoke UI, migration, zero-row, session, W7 and S4 tests; guard startup pull; stale legacy cleanup; full OFL.txt; worker tsc in CI. Remove in-repo mutant runner script. Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
Add AmiroRootTabs widget test (R3b), DELETE identity_id filter test (N7), recoveryCode catchError widget test (N8), and startup pull guard unit tests (N10). Keep StoreScreen out of the tab tree until the Store tab is selected. Document N9 as equivalent to supportsRestore early return. Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
ucguy4u
left a comment
There was a problem hiding this comment.
Changes requested and blocking — architect re-review of fbfe6e28ea55ebec02076c1c2fdc942e5835b37a (live head, re-fetched before posting; draft, mergeable/CLEAN). This supersedes my review 5745473115 at 3d9a8b9f. COMMENT review only: not an approval. Flagged for owner (human) review: privacy, deletion semantics, secrets and webhook auth, plus one product decision (O1 below) that only the owner can make.
I exported this exact SHA and ran it myself: stubbed-renderer baseline 216/216 pass (the box can't build the thermion native assets), worker 20/20, tsc --noEmit exit 0. Correction to my last review: I said "36 killed of 48 run". The right figures for that review were 37 killed of 50, 13 survived. This review's counts are below and checked.
Verdict: two blockers, both in the new revoke/session code. Most of what 5745473115 asked for is fixed and verified.
Blocking
B1. A genuinely dead refresh token is now treated as transient, so the session never recovers. supabase_profile_cloud.dart (_ensureSession, around :289-296) swaps to a new anonymous user only when refresh returns 401/403 (SessionRefreshFailure.isAuthFailure, :415-423). Supabase Auth answers a revoked, missing or already-used refresh token with HTTP 400 (refresh_token_not_found, refresh_token_already_used; see supabase/auth errorcode.go and the docs' error-codes page), not 401/403. I probed it with a fake server returning 400 refresh_token_not_found: push returns false, pull returns null and revokeBackup fails, each time. Refresh was retried 4 times with 0 sign-ins, so the app never recovers. Backup is silently dead and cannot be turned off. The revoke error text then misleadingly says "Check your connection" (userMessage is null). The PR fixed the 503 case and broke the real dead-token case. Fix: treat any 4xx except 408/429 as an auth failure, and let only network errors, 5xx, 408 and 429 propagate. Add a test using the real 400 body.
B2. The user cannot turn backup off when the server has nothing to delete, and a retry after a successful delete gets stuck. revokeBackup returns failed on any zero-row DELETE (:119-130), including a legitimately empty server. I probed it: with backup on and nothing uploaded, revokeBackup returns failed with "Only the device that created the backup can remove… or the copy was already removed". identity_edit_screen.dart:220-232 then keeps the switch on, so the user is stuck. Real paths into this:
- backup enabled before a profile exists, or before the first push succeeded (
_hasProfileTextfalse, or offline); - the row was already removed;
- the app was killed after the DELETE succeeded and
clearCloudBackupCredentialsran, but beforesetEnabled(false). On retry the session is gone, a new anonymous user is created, the DELETE hits zero rows, and it fails forever.
The operation is not idempotent. Fix: on zero rows, re-select. If no row is visible to this session, report success (nothing remains) and let the opt-in clear. If a row is visible (member case), return the honest failure. Add tests for the empty-server and retry-after-clear paths.
Owner decision O1: recovery-code member device can never turn backup off
I verified your claim: a member device's DELETE matches zero rows (the policy is correctly owner-only), so it fails honestly with a clear message and the credentials stay (:109-122). But as written it is a trap: the switch can never go off on that device, so it keeps pushing edits into the owner's row. If the owner deletes the row on device A, device B's next push finds it empty, INSERTs a fresh row owned by B and resurrects the server copy. Options for the owner to choose:
- (a) let a member "stop backing up on this device", which is local only with the server copy untouched and the copy saying so, plus a
profile_membersdelete-self policy so it can truly leave; - (b) make the owner's revoke cascade to the devices' opt-ins (not possible client-only);
- (c) accept the trap and say so in the privacy copy.
I recommend (a). The policy copy must match whichever is chosen. I do not block the PR on O1 once B2 is fixed, but I would not ship the revoke UI without the owner picking one.
Your specific questions
- 401/403 swap pointing data at a different account? No cross-account exposure: the local identity is untouched and the new anonymous user can't read the old row. But the opt-in and the saved recovery code stay on while the server row belongs to the old user. By the schema (
identity_idis the primary key) the next push then fails to INSERT and can't join as a member, so push returns false silently (reasoned from the schema, not probed). Revoke then fails zero-row, which is B2/O1 again. The swap should at least clear the opt-in or surface "backup needs re-linking". - Do propagating errors crash any caller? No. Every
_ensureSessioncall site is inside a try/catch (push :48,pull :91,revoke :108,claim :171). Startup goes throughtryRestoreIdentityFromCloud(launch/startup_cloud_restore.dart), which never throws;main.dart:50-55uses it. Nit:claimreturns null on a 503, and the UI says "That code did not match an Amiro" (identity_edit_screen.dart,_claim). That is misleading for a transient error. - Is clearing credentials safe for the join and recovery flows? Yes for the flows as designed. It runs only when this device's DELETE removed at least one row, so only the owner reaches it; a member fails before the clear. A deleted row's recovery code is dead server-side anyway. After a clear,
claim()signs in anonymously and joins, and re-enabling backup generates a new code (recoveryCode()is null, so_newCode()). The one hole is the retry path in B2. - Is N9 truly equivalent? Mine is (removing the
hasLegacyPlaintextFilesguard inrunProfileSecretsMigrationSafely):migrateFromLegacyPlaintextstarts with the same check, so the end state is identical (onlyexists()errors move inside the try). Amiro's "N9" test is a different thing, revoke with no cloud support, and it is real: removing thesupportsRestoreguard inrevokeBackupis killed by that test (my extra mutant X15). So the equivalence claim is right for my N9, but the reason given (supportsRestore) doesn't apply to it.
Verified fixed
- B1 from last time:
Prefer: return=representationis sent and success needs at least one deleted row (:128-134). Zero rows or a null id givesRevokeBackupResult.failed, the opt-in stays on, and the error shows (UI:222-232); a widget test covers failed DELETE and cancel. - Credentials cleared on success only. True in code, but see X7 below: no test proves it.
- Stale plaintext removed (
profile_secrets_store.dart:40-41,51-52). Startup pull guarded. Recovery code cleared on screen after revoke (identity_edit_screen.dart:236). Full OFL text and copyrights (app/assets/fonts/OFL.txt, 95 lines). Mutant script removed. The delete policy now uses(select auth.uid()). The webhook W7 test (non-UTF-8 bytes) is in. - CI now runs the TS fix:
npm ci,npm testandnpx --no-install tsc --noEmitall succeeded in the worker job. - Behaviour change to check:
app.dart:55-57now returnsSizedBoxfor the Store slot unless the Store tab is selected. Leaving the tab disposes the Store screen, so every return to it rebuilds it and reloads entitlements (and the scroll position is lost). That is stricter than the old lazy design and affects UX, so I'm noting it for the owner. Nit: X14 shows the matchingisActivegate instore_screen.dart:349is redundant.
Non-blocking
- Test gaps (surviving mutants on the new code):
- credentials cleared even on a failed DELETE (X7). Nothing asserts they survive a failure.
Preferheader removed (X3). The fake server returns a body regardless, so nothing proves the header is sent.- null id treated as success (X2).
pull()reading the session outside the try (X8). The startup helper masks it.- stale recovery-code legacy file kept (X11).
- recovery code left on screen after revoke (X12).
- M1/M2 survive. The new else-branch deletes the plaintext on the next migrate call, which masks a missing delete after the first write. Assert after a single call.
- R3b's "real AmiroRootTabs" test mounts the
lightweightTabsForTestlist, not the production tab list (app.dart:100-117). PuttingStoreScreen(isActive: true)inline in the production list survives.omitThermionOverlayForTestandlightweightTabsForTeststill exist only to serve tests. - N10 (
main.dartcalling the helper) isn't unit-testable. The helper is tested; this is fine as an accepted gap. - The W7 test covers
security.tsonly. Re-encoding the decoded text at theindex.tscall site isn't caught, and for valid UTF-8 JSON that differs only for a BOM. Low risk. - In-app font licence display (
LicenseRegistry) is still not registered. OFL doesn't require it; optional.
Mutants
I reran all 50 of my mutants from the last review (it was 50, not 49) on this exact SHA, running the full suite for each Flutter mutant. Patterns were adapted where the code moved. R3's original pattern no longer applies, so I ran the equivalent (X13, Store always mounted) in its place.
| Group | Run | Killed | Survived |
|---|---|---|---|
| App, original set (C1–C7 with C7a/C7b, A1–A5, R1–R4, M1–M6, S1–S4) | 27 | 25 | M1, M2 |
| New-diff set (R3b, N1–N10) | 11 | 8 | R3b, N9, N10 |
| Worker (W1–W12; W2 is the full length-check removal) | 12 | 12 | none |
| Total of the 50 | 50 | 45 | 5 |
- Of those 5: N9 is equivalent (above), N10 isn't unit-testable, and M1, M2 and R3b are real test gaps.
- Amiro's "49/49" does not reproduce: 45 killed, 5 survived. Their script ran a targeted subset and their count is off by one.
- Extras I added for the new code (15): 8 killed (X1 zero-row success, X4 and X5 swap policy, X6, X9, X10, X13, X15) and 7 survived (X2, X3, X7, X8, X11, X12, X14), listed above.
CI (checked myself)
Actions run 41558575861 on exactly fbfe6e28ea55ebec02076c1c2fdc942e5835b37a, event pull_request, completed / success. flutter (analyze and test), rust (test and clippy) and worker (npm ci, npm test, tsc --noEmit) all passed; no queued, skipped or cancelled checks. CI does not run Android builds or the manifest check.
#44 doc lines that still need updating once this merges
This PR changes no #44 doc, so every item from my last review still stands, with the line references unchanged. Highlights:
docs/legal/privacy-policy.md(and the.html, kept identical)::9-10,:36-42: backup is opt-in and off by default behind a consent dialog.:44: the recovery code is stored in Keystore/Keychain.:46-48: remove the RevenueCat id claim.:84-100: RevenueCat is configured lazily.:116-125: add in-app deletion by turning backup off, and what stays: the anonymous Auth row, and therevenuecat_eventsandentitlement_grantsaudit rows.- Add the member-device behaviour once O1 is decided.
- Encounter-share options (optional cosmetics and collection progress, off by default), and bundled fonts with no Google request.
docs/legal/data-safety.md::9-11,:22-23,:33-36,:57-58,:70,:82-87(the deletion answer).docs/legal/store-listing.md:24: opt-in backup; Android system backup disabled.workers/revenuecat-webhook/README.md::24,:30on. DocumentWEBHOOK_AUTHORIZATION,WEBHOOK_SIGNING_SECRET,CRASH_PATH_TOKEN, the fail-closed 500s, the 5-minute skew, and that a replay inside the window is only deduped by event id.TODOS.md::59,:146,:154,:155.README.mdandpackages/discovery/README.mdneed no change.
I did not merge, push or mark the PR ready. This review covers this SHA only; any later push voids it.
Session refresh: all 4xx except 408/429 trigger anonymous re-sign-in (Supabase 400 refresh_token_not_found). Revoke: zero-row DELETE re-selects; success when no profile visible; credentials cleared. Claim surfaces transient errors in UI. Add PA mutant-killing tests (X2/X3/X7, M1/M2, R3b production tabs, 400 refresh). Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
ucguy4u
left a comment
There was a problem hiding this comment.
Changes requested and blocking — architect re-review of 969ca4c09880cc6b44f780805e407691fa5b6a46 (live head, re-fetched before posting; draft, CLEAN). This supersedes my review 5746139880 at fbfe6e28. COMMENT review only: not an approval. Flagged for owner (human) review: privacy, deletion semantics and secrets handling. O1 (a recovery-code member device can't turn backup off) is still pending the owner and is not a blocker here.
I exported this exact SHA and ran it myself. Flutter ran in a scratch copy with the native thermion renderer stubbed (the box can't build it): baseline 226/226. Worker 20/20, tsc --noEmit exit 0. Mutants ran the full suite for each Flutter mutant.
Verdict: two blockers. The 400-as-auth-failure fix works and the idempotent revoke is the right idea, but the re-select path introduces a false "deleted", and the tests for it never reach the new branch.
Blocking
B1. After a session swap, revoke reports success while the user's real row and its data remain, and it wipes the only key to that row. supabase_profile_cloud.dart:107-139: if the DELETE removes zero rows, the code runs _selectPayloads(session). If nothing is visible it calls clearCloudBackupCredentials() and returns ok. But "nothing visible" is also what a brand-new anonymous user sees when the old row belongs to a different user. _ensureSession (:296-315) swaps to a new anonymous user on any 4xx refresh error, and _writeSession overwrites the old session. I probed it (Q1): the refresh returns 400, a new user is created, DELETE gets [], the re-select gets []. Result: revokeBackup returns success=true, the local session and the recovery code are deleted, and the old row is never touched. That is the exact case you asked about. Worse than before, because the credentials are now gone, so the row can't even be re-joined or deleted later. The same happens if the swap happened earlier, on a previous push, because the persisted session is already the new user's.
Fix: treat an invisible row as "nothing to delete" only when it provably isn't someone else's. Concretely, persist the user id that created the row (on the successful INSERT) and require session.userId == ownerId for the empty-select success. If the id differs, or a recovery code exists locally with no stored owner id, return failed with "reconnect with your recovery code" and keep the credentials. With no recovery code and no owner id (the already-cleared retry case) empty-select can safely succeed. Add the swap test.
B2. The tests for the idempotent-revoke fix don't execute the new branch, so they are tautological. The fake server's DELETE answers [{identity_id:'deleted'}] whenever deleteReturnsEmpty is false, even when no row exists (supabase_profile_cloud_http_test.dart, _handle, DELETE branch). So revokeBackup succeeds when backup on but server has no profile row (:94) and revoke succeeds on retry… (:250) both succeed through the "DELETE removed a row" path and never reach the re-select. Proof: Y2 (never succeed after zero rows) and Y3 (don't clear credentials on the re-select success) both survive the full suite. The deleteReturnsEmpty tests cover only the visible-row failure. Fix: have the fake DELETE return [] when the row doesn't exist, and assert (a) empty-select success clears credentials, and (b) the B1 swap case fails with credentials kept.
Your questions
- Re-select path wrongly clearing opt-in or credentials? Yes: B1. For an unswapped session it is correct, because RLS lets an owner or member always see their own row, so "empty" really means gone. Only a swapped user breaks that.
- Does the 4xx swap fire on 400s that aren't a dead token? Yes.
profile_cloud.dart:12-19treats every 4xx except 408/429 as auth failure. I probed a 400validation_failed(a malformed request) and it still swapped users and overwrote the session. Real dead-token 400s are now handled, which was the point. Orphaning only happens when refresh is rejected but anonymous sign-up then succeeds, so a bad-request client bug would orphan the row rather than loop. That is acceptable only if B1 is fixed (otherwise the orphan is then reported as deleted). Narrowing to the documented error codes (refresh_token_not_found,refresh_token_already_used,invalid_grant,session_not_found/session_expired) would be tighter. - Do claim errors propagate without crashing? No crash. A 5xx, 408 or 429 from the join call, or from the refresh inside
claim, rethrowsSessionRefreshFailure, and the UI catches that and shows the connection message (identity_edit_screen.dart:255-267). Gap: network failures (offline, timeouts) are still swallowed tonull, so offlineclaimshows "That code did not match an Amiro" (probe Q2). Non-blocking, but it is the common transient case. - CI: see below.
HMAC end to end (W7)
Exact raw bytes, confirmed in code and by probe. index.ts:162 reads request.arrayBuffer() into a Uint8Array, passes those same bytes through verifyWebhookRequest (:140, :168) into verifyRevenueCatWebhookSignature, which builds timestamp + '.' + bytes as a Uint8Array (security.ts:70-75) and hands it to hmacSha256Hex unchanged. The decoded text is used only afterwards for JSON.parse. I sent a BOM-prefixed body signed over the raw bytes through the real handler: 200. With the call site mutated to re-encode the decoded text it gives 401. So the production code is correct. What is real is a test gap: nothing at handler level catches that mutant (W7b); the unit test on security.ts kills W7. It fails closed (availability only, no security effect), so it is low severity.
Mutants
All run on this exact SHA with the full suite. Patterns were adapted where the code moved (R3's old pattern no longer applies, so X13 stands in; revoke patterns re-indented).
| Set | Run | Killed | Survived |
|---|---|---|---|
| My 50 (27 app + 11 new-diff + 12 worker) | 50 | 47 | 3: R3b, N9, N10 |
| Extras X1–X15 | 15 | 10 | 5: X2, X8, X11, X12, X14 |
| New for this head, Y1–Y9 | 9 | 5 | 4: Y2, Y3, Y5, Y9 |
| Total | 74 | 62 | 12 |
Which survivors are real:
- Real, matter: Y2 and Y3 (B2). R3b: the new "production AmiroRootTabs" test uses
find.byType(StoreScreen), which skips offstageIndexedStackchildren, and the other R3b test grepslib/app.dartsource text. PuttingStoreScreen(isActive: true)inline in the production list passes, so it is tautological. - Real, minor: Y9 (no UI test of the claim connection message), Y5 (429 boundary untested), X2 (null id path), X8 (
pull()guard, masked by the startup helper), X11 (legacy recovery-code file), X12 (recovery code left on screen). - Equivalent or not testable: N9 (my guard removal:
migrateFromLegacyPlaintextrepeats the same check), X14 (theisActivegate instore_screen.dartis redundant once the Store isn't mounted), N10 (main.dartwiring, not unit-testable).
Amiro's survivor list vs mine: C4, A1, A3–A5, R1, R4 and W10 are all killed under my patterns, so those are artifacts of their targeted patterns. N9 is equivalent. W7 is killed at the unit level and survives only at handler level (above). M1/M2, X3, X7, S4 and the others Amiro said they fixed now do die.
CI (checked myself)
Actions run 41562732667 on exactly 969ca4c09880cc6b44f780805e407691fa5b6a46, event pull_request, completed / success: flutter (analyze and test), rust (test and clippy) and worker (npm ci, npm test, npx tsc --noEmit) all succeeded, with no queued, skipped or cancelled checks. CI still has no Android or manifest check.
Non-blocking
- After any swap the stored recovery code is kept but never used to rejoin, so push silently fails (an INSERT on the existing
identity_idconflicts) and the opt-in stays on. Auto-rejoin throughjoin_profile, or at least clear the opt-in with a visible notice, would close it. Revoke then can't delete as a member (O1). - The one-sided
deletedCount > 0check treats any non-list body as zero rows. That is fine, but only because B1 isn't hit. - The Store tab is now disposed when you leave it (
app.dart:55-57); still an owner call on UX. - In-app font licence registration is optional.
#44 doc lines that still need updating
This PR changes no #44 doc, so the earlier list stands, with the same line references: privacy-policy.md (+ .html, identical) :9-10, :36-42, :44, :46-48, :84-100, :116-125 (add in-app deletion by turning backup off and what stays: the anonymous Auth row and the revenuecat_events / entitlement_grants audit rows; add the member-device behaviour once O1 is decided; encounter-share options; bundled fonts). data-safety.md :9-11, :22-23, :33-36, :57-58, :70, :82-87. store-listing.md:24. workers/revenuecat-webhook/README.md :24, :30 on (WEBHOOK_AUTHORIZATION, WEBHOOK_SIGNING_SECRET, CRASH_PATH_TOKEN, fail-closed 500s, 5-minute skew, replay only deduped by event id). TODOS.md :59, :146, :154, :155. README.md and packages/discovery/README.md are unchanged.
I did not merge, push or mark the PR ready. This review covers this SHA only; any later push voids it.
Persist cloud profile owner id on insert; only treat empty re-select as gone when the session matches that owner, with conservative legacy behavior. Fix DELETE harness to return empty when no row removed; narrow refresh auth swap to documented codes; surface connection errors on claim/revoke paths. Tests: B1 session swap, Y3/Y5/X2/X8/X11, R3b finder, Y9/X12 widgets, W7b BOM handler. Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
ucguy4u
left a comment
There was a problem hiding this comment.
Changes requested and blocking — architect re-review of 4e00853c97638034619c9901756fff9fbd0d4b55 (live head, re-fetched before posting; draft, CLEAN). This supersedes my review 5746661468 at 969ca4c0. COMMENT review only: not an approval. Flagged for owner (human) review: privacy, deletion semantics and secrets handling, plus the owner decision O2 below. O1 is still pending the owner and is not a blocker.
I exported this exact SHA and ran it myself. Flutter ran in a scratch copy with the native thermion renderer stubbed (the box can't build it): baseline 232/232. Worker 21/21, tsc --noEmit exit 0. Every Flutter mutant ran the full suite. I also probed the revoke/owner logic against a fake Supabase that models real per-user row visibility.
Verdict: one blocker, and it is test coverage, not behaviour. The production code for B1 is correct (probed below). But the test added to prove it never exercises the guard: deleting the guard entirely still passes all 232 tests.
Blocking
B1. The owner-id guard has no test that can fail. Mutants Z5 (session.userId == ownerId changed to true), Z6 (no-owner-id-plus-recovery-code case changed to "gone") and Z9 (the whole _canTreatEmptySelectAsGone check bypassed) all survive the full suite. The test B1: session swap after dead refresh cannot falsely revoke (supabase_profile_cloud_http_test.dart:~309) reaches its failure through the "row still visible" branch, because the fake's GET /profiles returns every row to every user (profileRows isn't scoped to the caller). So a swapped user still "sees" the old row and revoke fails for the wrong reason. The empty-select branch with a mismatched owner is never executed. This is the same class as B2 last round, on the most privacy-critical branch in the PR (a false "your data is deleted"). Fix: make the fake's GET (and DELETE) user-scoped (owner or member only), as in my probe, and add three tests: swapped owner (mismatched id) fails and keeps credentials; legacy install (no owner id, recovery code present) fails; no owner id and no recovery code succeeds. Add one member case (below).
Verified (code and probe, not the PR text)
Probe fake: rows have an owner and members; GET/DELETE are scoped to the caller's token.
- L1 legacy install (no owner id, code present), row exists, owner: revoke succeeds via the DELETE. A pre-owner-id install is not stranded in the normal case.
- L6 owner push then revoke: the owner id is stored after the first successful INSERT (
supabase_profile_cloud.dart:244-247,_insertProfile), andclearCloudBackupCredentialsdeletes it on a successful revoke. After revoke the owner id, the code and the session are all null. - Storage: the owner id is kept in the same
SecretStorage(Keystore/Keychain) as the session and recovery code (profile_secrets_store.dart:18,58-67), not in plain files. - Is the owner id ever set from a member device? No. It is written in exactly one place, after a successful
profilesINSERT withowner_id = session.userId._insertMemberandclaim()don't write it (probe L5: after a recovery-code join the owner id staysnull). A member's empty re-select therefore fails conservatively. The gap is only that no test pins it: mutants Z12 (member insert writes it) and Z13 (claim writes it) both survive. - Narrowed swap still recovers from a real dead token: yes. I sent the real GoTrue body
{"code":400,"error_code":"refresh_token_not_found","msg":"Invalid Refresh Token: Refresh Token Not Found"}: it swapped to a new anonymous user (signups=1),validation_faileddid not swap (L4: push false, signups 0). But the tests use a made-up body (error: invalid_grant,error_description: refresh_token_not_found), so Z2 (marker removed), Z3 (error_codefield not read, which is what real GoTrue sends) and Z4 (msgnot read) all survive. Add a test with the real body shape; today a regression in the parser wouldn't be caught. - CI below.
Your questions: stranded installs
Honest, conservative failures, no false success. The cases that cannot turn backup off:
- Legacy (no owner id, code present) and the server row is gone (for example removed after an emailed request, which the privacy policy offers): probe L2 returns
failedwith "Restore with your recovery code, then turn backup off again". But the claim then returns null (the row is gone), so it is a dead end. - Swapped owner (the original session is lost; the owner id is stored, mismatch): probe L3 — revoke fails, you claim the code and become a member, and revoke then fails again with "only the device that created the backup". The real owner can never delete their own row from the app. The recovery hint is wrong here too.
- Member (O1): same wall.
Decision: should-fix, plus an owner decision (O2). Not a blocker, because every case fails honestly and none reports deletion that didn't happen. Two parts:
- Should-fix, cheap: backfill the owner id.
owner_idis selectable (table-wide select grant in20261016181000, RLS limits it to the caller's rows), so on a successful push, or before revoke, runGET profiles?select=owner_id&identity_id=eq.<id>and write the owner id when it equals the session user. That rescues legacy installs that still hold their session. Also fix the copy: "Restore with your recovery code, then turn backup off again" promises a path that doesn't work. - Owner decision O2 (with O1): the swapped-owner, member and legacy-row-gone cases share one root cause, the server can't be told "I hold the recovery code, delete it". A
SECURITY DEFINERfunctiondelete_profile(p_code)that deletes the row matchingrecovery_code(the code is already the capability thatjoin_profileaccepts) would close all of them, and it needs no new client trust. The alternative is the local-only "stop backing up on this device" from O1(a). I recommend the RPC for deletion plus O1(a) for stopping pushes. The owner must choose, because it changes what the privacy policy can promise.
Also for the docs: users who installed before backup became opt-in have server copies from older builds but the toggle now shows off. They can only reach deletion by turning it on and then off, which is non-obvious and not documented.
Mutants
All on this exact SHA. Patterns were adapted where code moved (R3's old pattern no longer applies, so X13 stands in for it; Y8's old pattern no longer applies, so Y8b replaces it; the Y3, Y4–Y6 and X5 patterns were rewritten). I added 13 new mutants for the owner-id and swap code (Z1–Z13), plus Z14 (both offline mappings removed) and W7b (a BOM body through the handler).
| Set | Run | Killed | Survived |
|---|---|---|---|
| Original 50 (27 app, 11 new-diff, 12 worker) | 50 | 48 | N9, N10 |
| Extras X1–X15 | 15 | 11 | X2, X11, X12, X14 |
| Y1–Y9 (Y8 replaced by Y8b) | 9 | 9 | none |
| New Z1–Z13 | 13 | 3 (Z1, Z7, Z8) | Z2, Z3, Z4, Z5, Z6, Z9, Z10, Z11, Z12, Z13 |
| New Z14, W7b | 2 | 1 (W7b) | Z14 |
| Total | 89 | 72 | 17 |
Of the 17 survivors:
- Equivalent or not testable (5): N9 (the guard it removes is repeated inside the migration), N10 (
main.dartwiring), X14 (theisActivegate instore_screen.dartis redundant now), and Z10 and Z11 (each is redundant with the other: the offline mapping exists in bothclaimand_send). - Real, matter (9): Z5, Z6, Z9 (B1); Z2, Z3, Z4 (real error-body shape untested); Z12, Z13 (member/claim never sets the owner id); Z14 (offline claim message has no cloud-level test: removing both mappings passes the suite).
- Real, minor (3): X2 (null id), X11 (stale recovery-code legacy file), X12 (recovery code left on screen after revoke).
Claimed survivors that now die: Y2, Y3, Y5, Y9, X8, X7, X3, M1/M2, R3b (it is now killed with the skipOffstage: false test) and W7b. That part of Amiro's claim holds. R3 and W7b fixes are real.
CI (checked myself)
Actions run 41568905698 on exactly 4e00853c97638034619c9901756fff9fbd0d4b55, event pull_request, completed / success. flutter (analyze, test), rust (test, clippy) and worker (npm ci, npm test, npx --no-install tsc --noEmit) all succeeded. No queued, skipped or cancelled checks. No Android or manifest check exists.
Non-blocking
- An unrecognised 400 from refresh (a new GoTrue error code) is now permanent: no swap, and revoke fails with the generic "check your connection" text. That is the safer default, but it means the marker list must track Supabase.
- A refresh that succeeds but whose new session fails to persist (a crash between the two) leaves the old token to be reused, which becomes
refresh_token_already_usedand swaps users. This is rare, and it is now safe because of the owner guard. - Only
SocketExceptionandTimeoutExceptionmap to the connection message.HandshakeException,HttpExceptionand other network errors inclaimstill become "That code did not match an Amiro". - The Store tab is disposed when you leave it (
app.dart:55-57). It is still an owner UX call.
#44 doc follow-up status
This PR touches TODOS.md and docs/legal/third-party-licenses.md only. Done by this PR: TODOS.md:155 (the stale "no signature verification" bullet is replaced by a docs-owner note), the font licence row, and the full OFL file. Still needed (nothing else touched the legal docs):
docs/legal/privacy-policy.md(+.html, identical):9-10,:36-42,:44,:46-48,:84-100,:116-125. Add in-app deletion by turning backup off, what stays (the anonymous Auth row; therevenuecat_eventsandentitlement_grantsaudit rows), what the owner decides in O1/O2 (member and stranded cases), the pre-opt-in server copies, encounter-share options, and bundled fonts.data-safety.md:9-11,:22-23,:33-36,:57-58,:70,:82-87.store-listing.md:24.workers/revenuecat-webhook/README.md:24,:30on (WEBHOOK_AUTHORIZATION,WEBHOOK_SIGNING_SECRET,CRASH_PATH_TOKEN, the fail-closed 500s, the 5-minute skew, replays only deduped by event id).TODOS.md:59,:146,:154.README.mdandpackages/discovery/README.mdneed no change.
I did not merge, push or mark the PR ready. This review covers this SHA only; any later push voids it.
Principal Architect review 5746661468 — addressed at
4e00853Blockers
cloudProfileOwnerIdon successful profile INSERT; empty re-select clears credentials only whensession.userIdmatches stored owner; legacy owner unknown + recovery code present → fail with reconnect message (credentials retained). Test:B1: session swap after dead refresh cannot falsely revokeinsupabase_profile_cloud_http_test.dart.[]when no row removed so re-select path is exercised. Tests:Y3,X2, plus fixed harness behavior.Should-fix
SessionRefreshFailure.isAuthFailure);400 validation_failed refresh does not swap userstest.SessionRefreshFailure→Y9widget test.PA named survivors killed (evidence)
test('B1: session swap after dead refresh cannot falsely revoke')Y3: empty re-select clears credentials when owner session matchesY5: 429 refresh does not swap to a new anonymous useridentity_edit_claim_widget_test.dart— connection message, not code mismatchX2: revoke fails when a profile row is still visible after zero DELETEX8: pull returns null when session read throwsX11: successful revoke removes legacy recovery plaintext fileidentity_edit_revoke_widget_test.dart—recoveryCodekey still visible after failed revokeapp_launch_revenuecat_test.dart—find.byType(StoreScreen, skipOffstage: false)(source grep removed)worker_handler.test.mjs— BOM-prefixed body signed over raw bytesAccepted (no new tests): N9, X14, N10. O1: unchanged (owner).
CI
Green on
4e00853: run 41568905698 (flutter + rust + worker).