Skip to content

feat(aw-sync): daemon pull is opt-in via aw-sync config.toml - #716

Merged
ErikBjare merged 5 commits into
ActivityWatch:masterfrom
TimeToBuildBob:feat/aw-sync-pull-opt-in-config
Sep 18, 2026
Merged

ErikBjare merged 5 commits into
ActivityWatch:masterfrom
TimeToBuildBob:feat/aw-sync-pull-opt-in-config

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

Implements #714 for v0.14.0b8.

What

  • aw-sync/src/dirs.rs: adds dirs::get_config_dir()'s first real consumer — SyncConfig { pull: bool }, loaded from {config_dir}/config.toml via load_or_create_sync_config(). Missing file is created with the commented default:
    # aw-sync config
    pull = false   # default; set true to import peers from the sync folder every pass
  • dirs::effective_daemon_mode(cli_mode: Option<SyncMode>, pull: bool) -> SyncMode: an explicit --mode always wins; otherwise pull = falsePush, pull = trueBoth.
  • main.rs: Daemon.mode changes from a defaulted SyncMode to Option<SyncMode> so "not given" is distinguishable from an explicit choice. The daemon loads the config once at startup, logs the effective mode + config path, then runs with it.
  • aw-sync sync (the one-shot command) is untouched — still defaults to pull+push, as required.
  • status.rs: aw-sync status now prints the effective daemon mode and where the config was read from.

Why no separate #687 warning suppression was needed

sync_run() already only captures pull-discovery warnings (the "zero peers … layout problem" diagnostics) when mode == Pull || mode == Both. Since a push-only daemon pass now runs with SyncMode::Push, that whole block is skipped naturally — no extra gating required.

Tests

  • effective_daemon_mode_no_cli_follows_config_pull_flag: no --mode, pull=falsePush; pull=trueBoth.
  • effective_daemon_mode_explicit_cli_always_wins: --mode pull overrides pull=false (and any config).
  • load_or_create_sync_config_writes_commented_default_when_missing / ..._respects_existing_pull_true: config round-trip.

All 53 existing aw-sync lib/bin tests still pass; cargo fmt/cargo clippy -p aw-sync clean.

Manually verified end-to-end with XDG_CONFIG_HOME pointed at a scratch dir:

$ aw-sync --testing status
...
daemon mode: push (pull=false, config: .../activitywatch-testing/aw-sync/config.toml)

and with pull = true written into that file, daemon mode: both (pull=true, ...).

cc @ErikBjare

…yWatch#714)

Erik's b8 decision: 0.14 is push-only everywhere unless the user opts
in. Add the first proper config for aw-sync's config dir
(dirs::get_config_dir(), previously a TODO stub):

  # aw-sync config
  pull = false   # default; set true to import peers every pass

Daemon subcommand: --mode changes from a defaulted SyncMode to
Option<SyncMode> so "not given" is distinguishable from an explicit
choice. Effective mode is push-only when the config's pull=false,
both when pull=true, and an explicit --mode always overrides the
config (dirs::effective_daemon_mode). The one-shot `aw-sync sync`
command is untouched -- it keeps its own pull+push default.

The file is created with the commented default on first daemon start
(or `status`), so users find the switch. `aw-sync status` now prints
the effective mode and where the config was read from.

No separate suppression was needed for the ActivityWatch#687 "zero peers" warning:
sync_run() already only captures pull-discovery warnings when mode is
Pull/Both, so a push-only daemon pass naturally produces none.

Git-Session-Id: 2f40c8f4-6fa5-5c1d-9313-e9e188acb41c
@ErikBjare

Copy link
Copy Markdown
Member

@TimeToBuildBob Matches the #714 requirement point for point: one setting in aw-sync's own (profile-aware) config dir, default off ⇒ Push, pull = trueBoth, explicit --mode wins, one-shot sync untouched, file created with the commented default, mode + path logged at daemon start. The "no separate #687 suppression needed" reasoning holds — sync_run only captures pull-discovery warnings in Pull/Both. 53/53 + 9 + 20 pass locally. LGTM for b8 with two small asks, both in status.rs:

  1. status is the read-only doctor; it should read the config, not load_or_create it. Print daemon mode: push (pull=false, config: <path> — not present, default) when the file is missing instead of writing it.
  2. When pull = false, the doctor still lists every peer with ! peer X has not been imported locally as if something were wrong. Say it once at the top — pull is off in <config>; peers below are visible but not imported by the daemon (set pull = true, or run aw-sync sync) — and drop the per-peer "not imported" warnings in that state. That is the first screen Erik and every b8 tester will read.

Nit: the test temp-dir helper is the pid + nanos pattern that flaked #710; the distinct labels save it here, but tempfile::tempdir() is already a dependency.

@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The explicit-mode startup failure is fixed, but the repository’s unified-configuration requirement must be satisfied before merging; the remaining status issues are non-blocking diagnostic and side-effect defects.

Findings

  1. P1 Explicit mode requires config
  2. P2 Daemon-only configuration violates requirement
  3. P2 Status ignores CLI overrides
  4. P2 Read-only status creates directory

Summary

This PR makes recurring daemon pulls opt-in through profile-specific aw-sync/config.toml, preserves explicit CLI mode precedence, and adds effective-mode diagnostics to aw-sync status.

  • Defaults an unspecified daemon mode to push-only unless pull = true.
  • Avoids accessing configuration when an explicit daemon mode is supplied.
  • Adds read-only configuration parsing and mode-aware status warnings.
  • Leaves one-shot sync behavior as pull-and-push.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    D[Daemon invocation] --> M{Explicit --mode?}
    M -->|Yes| C[Use CLI mode]
    M -->|No| F[Read or create config.toml]
    F --> P{pull enabled?}
    P -->|No| Push[Push mode]
    P -->|Yes| Both[Both mode]
    S[Status invocation] --> R[Read config without creating file]
    R --> Report[Display config-derived daemon mode and warnings]
Loading

Reviews (2) · Last reviewed commit: "fix(aw-sync): explicit --mode daemon sta..."

Comment thread aw-sync/src/main.rs Outdated
@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 35.18519% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.03%. Comparing base (656f3c9) to head (c3c123b).
⚠️ Report is 130 commits behind head on master.

Files with missing lines Patch % Lines
aw-sync/src/status.rs 0.00% 22 Missing ⚠️
aw-sync/src/main.rs 0.00% 10 Missing ⚠️
aw-sync/src/dirs.rs 86.36% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #716      +/-   ##
==========================================
+ Coverage   70.81%   80.03%   +9.21%     
==========================================
  Files          51       74      +23     
  Lines        2916     8209    +5293     
==========================================
+ Hits         2065     6570    +4505     
- Misses        851     1639     +788     

☔ 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.

…eer pull warnings

Two fixes from ActivityWatch#716 review (Erik, 2026-09-18):

1. `status` is a read-only doctor. Swap `load_or_create_sync_config` for the
   new `read_sync_config` helper which returns `None` when the file is absent
   instead of writing the default. When absent, status now prints:
     daemon mode: push (pull=false, config: <path> — not present, default)

2. When `pull = false`, listing every peer with "! has not been imported
   locally" is misleading — the daemon is doing the right thing. Replace the
   per-peer warnings with one top-level note:
     pull is off in <config>; peers below are visible but not imported by
     the daemon (set pull = true, or run `aw-sync sync`)

Also adds two tests for `read_sync_config` (absent → None, no file created;
present + pull=true → Some(config)).

Git-Session-Id: d398
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Both asks addressed in 99d5c45:

  1. status reads config, never creates it — added read_sync_config returning None when absent. Output when missing: daemon mode: push (pull=false, config: — not present, default)

  2. pull=false: one top-level note, no per-peer churn — collect_warnings now takes pull: bool; when false it emits a single note: "pull is off in ; peers below are visible but not imported by the daemon (set pull = true, or run aw-sync sync)" and skips the per-peer 'has not been imported locally' warnings.

Also added two tests for read_sync_config (absent -> None + file not created; present + pull=true -> Some(config)).

…fig.toml

An explicit --mode is documented to always win over config.toml, but the
daemon still unconditionally loaded/created the config file first. A
malformed or unwritable config.toml would then abort startup before
daemon() ran, even with an explicit CLI override.

Skip config loading entirely when --mode is given explicitly.

Git-Session-Id: 212461ef-20e7-561e-b99b-a8f641b73b9a
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

Comment thread aw-sync/src/dirs.rs Outdated
Comment thread aw-sync/src/status.rs Outdated
Comment on lines +94 to +105
let (pull, config_label) =
match crate::dirs::get_config_dir().and_then(|dir| crate::dirs::read_sync_config(&dir)) {
Ok((Some(cfg), path)) => {
let effective_mode = crate::dirs::effective_daemon_mode(None, cfg.pull);
out.push_str(&format!(
"daemon mode: {} (pull={}, config: {})\n",
effective_mode.as_str(),
cfg.pull,
path.display()
));
(cfg.pull, path.display().to_string())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Status ignores CLI overrides

When the daemon starts with an explicit --mode, status still derives its “daemon mode” and peer-warning policy only from config.toml. For example, --mode both with pull = false is reported as push-only and suppresses peer import warnings relevant to the daemon's actual behavior, making the diagnostic output misleading.

Knowledge Base Used:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f995a3. status genuinely can't see a --mode override given to a separate, already-running daemon process — there's no IPC between the two invocations. What was misleading is fixed instead: the "daemon mode: ..." line is now labeled — config-derived; see 'Last pass' below for the mode actually used, which wins if --mode was passed explicitly, and the "Last pass" section (already printed just below it) reports the real SyncReport.mode from the last completed pass, which does reflect any CLI override.

Comment thread aw-sync/src/status.rs Outdated
Comment on lines +94 to +95
let (pull, config_label) =
match crate::dirs::get_config_dir().and_then(|dir| crate::dirs::read_sync_config(&dir)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Read-only status creates directory

The new read-only configuration path still calls get_config_dir(), which unconditionally creates the directory before read_sync_config runs. As a result, aw-sync status mutates a fresh writable configuration root even though the read-only reader was introduced to prevent the doctor command from creating configuration state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5f995a3. Added dirs::config_dir_path() — path construction only, no fs::create_dir_all — and switched status to call it instead of get_config_dir() (which the daemon still uses, since it's about to write config.toml there anyway). Verified manually with a scratch XDG_CONFIG_HOME: aw-sync status no longer creates the aw-sync config dir when it's absent.

… mode is a guess

- config_dir_path() constructs the aw-sync config path without creating it;
  get_config_dir() (used by the daemon, which is about to write config.toml
  there) now delegates to it. status previously called get_config_dir()
  before read_sync_config(), so the read-only doctor command silently
  created the config directory as a side effect.
- status's 'daemon mode:' line is derived from config.toml alone and cannot
  see an explicit --mode passed to a running daemon; label it as such and
  point at the 'Last pass' section, which reports the mode actually used.

Git-Session-Id: a7e90efc-d5b6-5d9b-a826-55b00e53d16c
Comment thread aw-sync/src/dirs.rs
@ErikBjare

Copy link
Copy Markdown
Member

@TimeToBuildBob See #716 (comment)

Erik: 'Properties should be in some default namespace, not in
top-level imo.' Follows aw-server's own convention of nesting a
distinguishable sub-concern in its own [section] (cf. [auth] in
aw-server/src/config.rs) rather than dumping it at the config's
top level. pull now lives at [daemon].pull; SyncConfig wraps a new
DaemonConfig. Updated the commented default, all call sites, and
the dirs.rs unit tests.

Git-Session-Id: bcd5de1a-146f-5d49-a776-ae61c28c2602
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