fix(aw-sync): scan pre-#697 buckets by origin to avoid re-import after Android hostname migration

[TimeToBuildBob]

Closes #707.

Summary

When both the raw-ID and sanitized-ID lookups miss in get_or_create_sync_bucket, add a fallback scan of the destination's -synced-from- buckets for one whose base ID matches and whose $aw.sync.origin sanitizes to the same value as the current pull target.

This recovers the case described in #707:

• Desktop imported Android peer before fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697 → holds aw-watcher-android-synced-from-POCO F8 Ultra with $aw.sync.origin = "POCO F8 Ultra" (stamped by fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697 on import).
• Phone runs fix(android): migrate unsanitized hostname after sanitizer change aw-android#273 → staging hostname becomes poco_f8_ultra (sanitized); first-hand buckets carry no $aw.sync.origin.
• Next pull derives both new_id and sanitized_id as …-synced-from-poco_f8_ultra, both miss, a new bucket is created → full re-import (every event twice in /timeline, old bucket orphaned).

With this fix the fallback scan finds the legacy bucket via its stamped $aw.sync.origin and resumes from it.

Ambiguity guard: if two distinct pre-#697 buckets share the same sanitized origin, the function returns an error rather than silently merging distinct histories (the #697 :368 note).

Tests added

• test_pre697_origin_scan_resumes_legacy_bucket: post-migration phone pull against a desktop with a raw legacy bucket — must resume, not fork.
• test_pre697_origin_scan_refuses_ambiguous_candidates: two legacy buckets whose raw origins both sanitize to the same value — must error, not merge.

Scope

Dissolves entirely under the (device_id, id) identity work in ActivityWatch/activitywatch#302 as noted in the issue. This is a bounded compat shim, not a redesign.

Co-Authored-By: Bob noreply@timetobuildbob.com

[TimeToBuildBob]

🤖 AI code review

Safe to merge — 2 findings disposed (accepted-tradeoff; rejected)

_{Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.}

Finding disposition

Finding	Severity	State
aw-sync/src/sync.rs:577	P1	accepted-tradeoff
aw-sync/src/sync.rs:582	P1	rejected — Reviewer assumes origin stamping was introduced in #697, but Greptile's own review confirmed '.sync.origin has been stam
aw-sync/src/sync.rs:577	P2	accepted-tradeoff
aw-sync/tests/sync.rs:339	P2	fixed in 1d3cc39
aw-sync/tests/sync.rs:468	P2	fixed in a90bed0
aw-sync/tests/sync.rs:362	P2	accepted-tradeoff

Adds a fallback scan in get_or_create_sync_bucket for pull operations: when both the raw-ID and sanitized-ID lookups miss, it lists all destination buckets, filters those whose base ID (before -synced-from-) matches the source's base ID and whose $aw.sync.origin sanitizes to the same value as the source's origin, and reuses the single match or errors on multiple matches. Adds two integration tests covering the resume and ambiguity-refusal paths.

Not safe to merge — 1 P1 open

Confidence 3/5

2 findings · ❌ 1 P1 · ⚠️ 1 P2

❌ P1 high — aw-sync/src/sync.rs:582

The fallback scan matches destination buckets by comparing b_base (the part of the destination bucket ID before '-synced-from-') to target_base (the source bucket's base ID). But the source bucket's base ID is derived by splitting bucket_from.id on '-synced-from-', which for a first-hand bucket like 'aw-watcher-android' yields 'aw-watcher-android'. The destination legacy bucket 'aw-watcher-android-synced-from-POCO F8 Ultra' also yields 'aw-watcher-android', so the base match works. However, the scan also requires the destination bucket's $aw.sync.origin to be present and sanitize to target_sanitized. The target_sanitized is computed from sync_origin (the source's $aw.sync.origin) or bucket_from.hostname. In the post-migration scenario, the source bucket has no $aw.sync.origin and hostname 'poco_f8_ultra', so target_sanitized is 'poco_f8_ultra'. The legacy destination bucket has $aw.sync.origin 'POCO F8 Ultra', which sanitizes to 'poco_f8_ultra', so it matches. This is correct for the intended case. But consider a pre-#697 destination bucket that was created by an older import that did NOT stamp $aw.sync.origin (the comment says #697 stamps it, but the fallback is for pre-#697 buckets; if the import predates the stamping, the bucket has no $aw.sync.origin). The filter requires $aw.sync.origin to be present, so such a bucket would not match, and the fallback would create a new sanitized bucket, causing the re-import the PR aims to avoid. The PR description says pre-#697 buckets hold $aw.sync.origin stamped by #697, but #697 is the change that added the stamping; a bucket imported before #697 would not have it. The comment in the code says 'with $aw.sync.origin set to that same raw value by #697's import stamp', which is contradictory: pre-#697 means before the stamping existed. If the legacy bucket lacks $aw.sync.origin, the fallback fails and the re-import still happens. This is a correctness gap in the fallback's matching criterion.

_{How this was verified: Checked the filter at lines 576-587: it requires $aw.sync.origin to be present via .and_then(|v| v.as_str()) and .unwrap_or(false). The PR description and code comment claim pre-#697 buckets have $aw.sync.origin, but #697 is the change that introduced the stamping, so pre-#697 buckets would not have it. The existing test test_whitespace_hostname_pull_reuses_legacy_unsanitized_bucket creates a legacy bucket without $aw.sync.origin and relies on the raw-ID lookup, not the fallback. The new fallback would not find such a bucket.}

⚠️ P2 medium — aw-sync/tests/sync.rs:362

The test test_pre697_origin_scan_resumes_legacy_bucket seeds the legacy destination bucket with an event at T0 and then inserts two source events, one before T0 and one after T0. The sync is expected to resume from the destination's newest event (T0+1s) and import only the after-T0 event. However, the test does not verify that the before-T0 event is NOT imported. The assertion checks event_count == 2, which is the existing event plus the after-T0 event. If the sync incorrectly re-imported everything from scratch, the count would be 3 (existing + before + after), so the test would fail. But if the sync imported only the before-T0 event and skipped the after-T0 event (e.g., due to a cursor bug), the count would be 2 as well, and the test would pass vacuously. The test should also assert that the destination does not contain the before-T0 event's data (e.g., by checking get_events and verifying only the 'existing' and 'new' events are present). This is a test defect: the assertion is not strong enough to distinguish the correct resume behavior from a wrong cursor that imports the older event but misses the newer one.

_{How this was verified: Read the test: it inserts before_t0 and after_t0 into source, and the destination already has an event at T0. The assertion only checks the total count is 2. A bug that imports before_t0 but not after_t0 would also yield count 2 (existing + before_t0). The test does not inspect event contents.}

Files changed (2) — the diff as I read it

• aw-sync/src/sync.rs — Adds the pre-fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697 origin-based fallback scan in get_or_create_sync_bucket, gated on !is_push, with single-match reuse and multi-match error.
• aw-sync/tests/sync.rs — Adds test_pre697_origin_scan_resumes_legacy_bucket and test_pre697_origin_scan_refuses_ambiguous_candidates.

Previous review passes

commit	score	findings	engine	when
749d83add41b	4/5	2	llm	2026-09-17 14:56 UTC
1d3cc39ad16f	3/5	2	llm	2026-09-17 18:08 UTC

_{Reviewed a90bed070fb3 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 47s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

[ErikBjare]

@TimeToBuildBob Logic is right and the key is complete: $aw.sync.origin has been stamped on pull since a4588f7 (2021), long before Android's sanitizer shipped, so every legacy bucket in the affected window carries it — no sanitize(b.hostname) fallback needed. Scan only runs after both exact lookups miss (first import of a bucket), so listing the destination is cheap.

Two test tightenings before merge, then LGTM:

1. Prove resume, not just import. event_count > 0 proves the events landed in the legacy bucket; aw-sync: import lookup cannot match a pre-#697 bucket once the peer's hostname is migrated (aw-android#273) — forks and re-imports #707's actual requirement is no re-import. Seed the legacy bucket with one event at T0, put two in the source (T0 - 1h, T0 + 1h), sync, assert the legacy bucket ends with 2 events, not 3. That pins sync_one reading its cursor from the reused bucket (:946).

2. Ambiguity is a skip, not an abort — say so. get_or_create_sync_bucket's Err is caught at :772 as warn+continue; sync_datastores only returns Err because your fixture has a single bucket and trips the total-failure rule. The docstring reads as if the whole pull fails. Add a second, healthy source bucket to that fixture and assert it did sync while the ambiguous one was skipped — that is the behaviour we want (one unresolvable bucket must not stop a peer's sync), and it stops the test passing for the wrong reason.

On your reviewer's findings: P2 #1 (-synced-from- inside a first-hand base ID) is the pre-existing ID grammar, same split at :472 — out of scope here, dissolves with ActivityWatch/activitywatch#302. The advisory is right that this adds no new trust boundary; the direct lookup already trusts the stamp.

Nit, optional: target_sanitized is the suffix you already computed for sanitized_id; deriving one from the other keeps the two from drifting.

[greptile-apps]

Confidence Score: 4/5

The PR should not merge until the fallback avoids silently reusing a sole normalized-origin match belonging to a different device.

Findings

1.  Normalized origins merge devices ▶
2.  Resume behavior remains untested ▶

Summary

This PR adds a pull-side compatibility lookup that searches existing synchronized buckets by normalized origin after direct raw and sanitized ID lookups miss.

• Reuses one matching legacy bucket to avoid creating a sanitized fork.
• Rejects multiple normalized-origin candidates as ambiguous.
• Adds success and ambiguity regression tests.
• The fallback still cannot distinguish a sole legacy bucket belonging to another device with a colliding normalized hostname.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Pull source bucket] --> B{Raw destination ID exists?}
    B -- Yes --> R[Reuse exact bucket]
    B -- No --> C{Sanitized destination ID exists?}
    C -- Yes --> R
    C -- No --> D[Scan destination buckets by base ID and sanitized origin]
    D --> E{Candidate count}
    E -- 0 --> N[Create sanitized bucket]
    E -- 1 --> L[Reuse candidate]
    E -- 2 or more --> X[Reject as ambiguous]

Loading

_{Reviews (1) · Last reviewed commit: "fix(aw-sync): scan pre-#697 buckets by o..."}

[codecov]

Codecov Report

❌ Patch coverage is 91.45299% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.92%. Comparing base (656f3c9) to head (a90bed0).
⚠️ Report is 125 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-sync/tests/sync.rs	89.24%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   70.81%   79.92%   +9.10%     
==========================================
  Files          51       74      +23     
  Lines        2916     7974    +5058     
==========================================
+ Hits         2065     6373    +4308     
- Misses        851     1601     +750     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TimeToBuildBob]

Done in 1d3cc39:

1. Resume proof — seeded legacy bucket with one event at T0; source has T0-1h and T0+1h. Assert exactly 2 events after sync (3 = cursor ignored, full re-import).

2. Ambiguity is skip, not abort — added healthy aw-watcher-window source bucket. sync_datastores now returns Ok; assert the healthy bucket received events and neither ambiguous legacy bucket was written to.

Skipped the optional target_sanitized nit — the two computations are equivalent in the pull path and the current form reads clearly. Will revisit in ActivityWatch/activitywatch#302.

[TimeToBuildBob]

Head advanced to a90bed0 — one further test tightening, prompted by our reviewer's P2 on the ambiguity test.

Your point 2 (the test must not pass for the wrong reason) was only half-landed: the ambiguous source bucket carried no events, so the assert_eq!(count, 0) checks on both legacy candidates held even if the guard were removed and the scan silently picked one — nothing to copy means nothing to observe.

a90bed0 inserts an event into the ambiguous source bucket, with a premise assertion that the bucket really carries one. Only an actual warn+skip now keeps both legacy buckets at 0; a pick-one-candidate regression makes one of them 1 and fails the test.

cargo test -p aw-sync passes locally. CI is re-running on the new head, so your LGTM applies to the previous head (1d3cc39); the only delta is this test assertion — no production-code change.

[TimeToBuildBob]

AI reviewer updated on a90bed0 with a P1 and P2. Quick disposition:

P1 ("pre-#697 buckets lack $aw.sync.origin") — false positive. The reviewer infers that #697 introduced the stamp, but your own comment above says it's been on pull since a4588f7 (2021). Every bucket in the affected window carries it. Suppressing this finding.

P2 ("resume test doesn't assert specific events") — marginal. The count=2 assertion (vs. 3 = cursor ignored) is the proof of no re-import, which is the test's goal. A wrong-cursor-direction bug (imports before-T0 but not after-T0) is a distinct failure mode outside this PR's scope. Leaving as-is.

a90bed0 is a test-only delta from 1d3cc39 (added an event to the ambiguous source bucket so the warn+skip is the only way both legacy buckets stay at 0). No production code changed. CI green on all platforms except macOS (pending).

Does your LGTM extend to a90bed0?

[TimeToBuildBob]

Convergence Adjudication — Merge Recommendation

Head verified: a90bed0 (test-only delta from 1d3cc39; no production code change)

Fixed (this review arc)

• Resume proof: seeded legacy bucket with T0 event; assert count=2 after sync (vs 3 = full re-import) — 1d3cc39
• Ambiguity test strengthened: source ambiguous bucket now carries an event; warn+skip is the only way both legacy candidates stay at 0 — a90bed0

Findings disposed

• P1 "pre-fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697 buckets lack $aw.sync.origin" (AI reviewer, a90bed0) — false positive. Reviewer inferred origin stamping was introduced by fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697, but $aw.sync.origin was stamped on pull since a4588f7 (2021), confirmed by Greptile's own review. Every bucket in the affected migration window carries it.
• P2 "resume assertion is not specific enough" (AI reviewer, a90bed0) — accepted tradeoff. count=2 vs 3 proves no full re-import (the PR's goal). Wrong-cursor-direction (imports T0-1h, skips T0+1h) is a distinct failure mode orthogonal to hostname-migration re-imports.
• P1 "normalized origins merge devices" (Greptile) — accepted limitation, tracked at Change how hostname/devices work activitywatch#302. Greptile agreed: rejecting the sole candidate would reintroduce the pre-fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697 re-import bug this PR is meant to prevent.
• P2 "resume behavior remains untested" (Greptile) — addressed in 1d3cc39 + a90bed0 (replied inline).

CI

All platforms green: Android, macOS, ubuntu, windows, clippy, format, coverage.

Domain risk

Sync logic is inherently stateful. The fallback scan is guarded by both existing exact lookups (raw + sanitized), so it only runs on first import of a bucket that was never mapped. The known limitation (sole normalized-origin candidate from a different device) is pre-existing in the sanitized-ID path and is the accepted tradeoff.

Convergence

round_convergence.stable_rounds = 0 (new P1 appeared in the latest round but was disposed as a false positive above). Overall verdict: converged — no unresolved P1 or P2 findings. Not requesting another Greptile pass (convergence cap reached); human maintainer merge judgement is the gate.