Skip to content

fix(aw-sync): skip dot-directories when walking peer databases - #702

Merged
ErikBjare merged 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/aw-sync-skip-dot-dirs
Sep 17, 2026
Merged

ErikBjare merged 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/aw-sync-skip-dot-dirs

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

Fixes item 3 of #689. Independent of the parked daemon-layout switch in #685.

Problem

list_remote_dbs (3-level pull_all walker) and find_remotes (2-level walker) recurse into every directory at the first (and, for 3-level, second) level with no name filter. That includes .git, .stfolder, and .stversions.

No .db files live there in the common case, so this was not a correctness bug — but both walkers now propagate I/O errors rather than swallowing them. An unreadable entry under a metadata dir aborts the whole pass. Syncthing Trash Can versioning can also place a .db at {sync}/.stversions/*.db, shallow enough for the 2-level walker to treat it as a peer.

Fix

Skip .-prefixed directories in:

  • list_remote_dbs (host and device levels)
  • find_remotes (first level)
  • scan_sync_dir (so aw-sync status does not walk .git either)

Not in this PR: leftover 2-level staging db cleanup, stale -synced-from- prune, or the test.db rename (items 1, 2, 4 of #689). Those wait on the layout we are not switching to in v0.14.0.

Tests

  • list_remote_dbs_skips_dot_directories
  • find_remotes_skips_dot_directories
  • scan_sync_dir_skips_dot_directories

Does not touch the sync_run region rewritten in #698, so it is independent of the #700#678#697 merge order.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Review guide (auto-posted)

Structured warm-up for reviewers — what changed, what to run, where to look.

Key files

  • aw-sync/src/util.rs (+88/-4, Δ92)

Suggested verification

  • Run the repo's usual CI-equivalent checks locally

Known risks / watch points

  • No automatic high-risk tags; use file list + diff for judgment.

Suggested review focus

  • Confirm behavior matches the PR description acceptance criteria.
  • Skim the largest diffs first (listed above).
  • If CI is green, spot-check the highest-risk paths called out here.

Generated by scripts/github/pr-warmup-review-guide.py for #702.

@ErikBjare

Copy link
Copy Markdown
Member

One is_dot_dir guard applied to list_remote_dbs, find_remotes and scan_sync_dir, with a test per walker — exactly #689 item 3. No legitimate peer directory starts with ., and this stops a pass walking .git / .stfolder / .stversions on every cycle. Merge on green; independent of the #700#678#697 order.

@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the targeted metadata-directory filtering is consistently applied to the intended walkers and covered by regression tests.

Summary

This PR updates ActivityWatch sync-folder discovery to ignore dot-prefixed metadata directories.

  • Skips dot-directories at the relevant levels of the three-level and two-level remote database walkers.
  • Applies the same root-level filtering to sync status scans.
  • Adds focused regression coverage for .git, .stfolder, and .stversions.

Reviews (1) · Last reviewed commit: "fix(aw-sync): skip dot-directories when ..."

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.41%. Comparing base (656f3c9) to head (db6c568).
⚠️ Report is 123 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
+ Coverage   70.81%   79.41%   +8.59%     
==========================================
  Files          51       74      +23     
  Lines        2916     7825    +4909     
==========================================
+ Hits         2065     6214    +4149     
- Misses        851     1611     +760     

☔ 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

Copy link
Copy Markdown
Contributor Author

CI-green and mergeable — waiting only on a maintainer click.

This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted.

@TimeToBuildBob

TimeToBuildBob commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

This PR adds a helper is_dot_dir and applies it to the directory-walking loops in list_remote_dbs, find_remotes, and scan_sync_dir so that dot-prefixed directories (e.g. .git, .stfolder, .stversions) are skipped. It also adds three tests covering the new skip behavior in each walker.

Safe to merge — no P0/P1 findings

Confidence 5/5

No thread-worthy findings. Advisory notes follow; they are retained without opening review threads.

1 advisory finding (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 mediumaw-sync/src/util.rs

This is a fix(...) PR but no test files are included in the diff. Erik's feedback: 'where is the repro & fixes they are supposed to catch' (gptme#3441), 'that measurement should come with a regression test' (gptme#3446). Add a test that would have caught this bug. (Advisory: Erik merged all such PRs but consistently requested tests.)

Add a test file that reproduces the bug before the fix and passes after it.

How this was verified: static preflight: fix-commit + touched-files scan (rule 7)

Files changed (1) — the diff as I read it
  • aw-sync/src/util.rs — Adds is_dot_dir helper and uses it in three walkers; adds three tests for dot-directory skipping.
Previous review passes
commit score findings engine when
2ecfb10f16cf 4/5 2 llm 2026-09-16 10:12 UTC
d7cc1eaa2e77 4/5 2 llm 2026-09-17 09:41 UTC

Reviewed db6c5680599d · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 9s · about this reviewer

Maintainer commands

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

Comment thread aw-sync/src/util.rs
@ErikBjare

Copy link
Copy Markdown
Member

@TimeToBuildBob CONFLICTING again — #699 and #678 both landed this morning (master is 3d3b9dd). Rebase onto current master; nothing else has moved and nothing else will before these three go in, so this is the last rebase. Order after rebase: #697#703 (same sync_run region) → #702 any time.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto 3d3b9dd (head 2b58d4a) — master should not move again before this lands. The conflict was confined to the test module in aw-sync/src/util.rs (both #699's and #678's new tests landed in the same spot); no logic changed, both test sets kept. CI is running on the new head.

@ErikBjare

Copy link
Copy Markdown
Member

@TimeToBuildBob Re-verified on 2b58d4a: is_dot_dir guards all three walkers (list_remote_dbs :403/:412, find_remotes :532, scan_sync_dir :718), one test each. Content is done.

But the format check failedcargo fmt --check is red on this head. Run cargo fmt and push; no other change needed. Master moved once more (#697 landed) but this PR is still MERGEABLE against it, so no further rebase — just the fmt push.

@ErikBjare

Copy link
Copy Markdown
Member

Correction: the master move was #701 (README) landing, not #697#697 is still open. Doesn't change the ask: this PR is MERGEABLE against current master, so a cargo fmt push is all it needs.

`.git`, `.stfolder`, and `.stversions` are not host folders. The 2-level
and 3-level walkers used to descend into them; because they propagate I/O
errors, an unreadable entry under a metadata dir aborted the whole pass.
ActivityWatch#689 item 3.

Git-Session-Id: 6a1c8cc0-cfa7-5a7e-a4ee-d4eea39f9844
@ErikBjare

Copy link
Copy Markdown
Member

72d099c: one-line rustfmt fix, format now green, content unchanged from the verified 2b58d4a. Merge on green — after #697, any time relative to #703.

@TimeToBuildBob
TimeToBuildBob force-pushed the fix/aw-sync-skip-dot-dirs branch from 72d099c to d7cc1ea Compare September 17, 2026 09:12
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Two follow-up pushes since the rebase comment: an intermediate head had a stray brace in the test module (caught by local cargo check, fixed in 72d099c), then a fast-forward over the #701 docs merge (docs-only, no conflict). Final head is d7cc1ea; cargo check -p aw-sync --all-targets and the 19 util lib tests pass locally. CI is running on it.

Comment thread aw-sync/src/util.rs
Comment thread aw-sync/src/util.rs
to_str() returns None for non-UTF8 filenames, so a dot-directory with
invalid UTF-8 bytes after the leading '.' was not recognized as a
dot-dir and could still be walked — the exact failure mode this fix
addresses (ActivityWatch#689). to_string_lossy()
preserves the valid-ASCII leading byte regardless of what follows.

Flagged by AI review on ActivityWatch#702.

Git-Session-Id: 065f
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

AI review threads addressed: fixed the real one (non-UTF8 dot-dir names not recognized by is_dot_dir — now uses to_string_lossy()) in db6c568, and resolved the self-refuting test-defect finding as FP (the reviewer's own text concludes "I don't see a test defect here"). Content otherwise unchanged from the head Erik re-verified. Waiting on CI for db6c568 before merging per the standing "merge on green" authorization.

@ErikBjare

Copy link
Copy Markdown
Member

db6c568 verified: to_str()to_string_lossy() in is_dot_dir is a real fix — a non-UTF8 dot-directory name previously came back None and was not skipped; the leading . survives lossy conversion. Four lines, walkers and tests otherwise unchanged from the verified d7cc1ea. Still MERGEABLE against current master; merge on green.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

CI green on db6c568 (all 7 checks pass), both AI-review threads resolved, mergeStateStatus: CLEAN. Ready for merge — I don't have write access to this repo, so this needs your click.

@ErikBjare
ErikBjare merged commit 94bfc54 into ActivityWatch:master Sep 17, 2026
7 checks passed
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Merge verified live — 94bfc544 is on master and the is_dot_dir guards are present in all three walkers (list_remote_dbs :403/:412, find_remotes :532, scan_sync_dir :718), with the to_string_lossy fix from db6c568 retained. Nothing further needed on this PR; #689's remaining items (1–2) still wait for the v0.14.0 cut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants