Turn automatic sync on once a sync target is configured - #61
Merged
Conversation
Configuring a target is the statement of intent to keep it current, so automatic sync now resolves to on from that point instead of waiting for a separate `sync auto on` that most people never found. A stated preference always wins. `auto on` and `auto off` record that the user chose, and adding more targets never overrides that choice, so turning it off once keeps it off. The policy lives in one function, `automatic_sync_enabled`, read by the background gate, the `sync auto` display, and `sync target add`. Those three previously each read `auto.enabled` directly and would have drifted into reporting different answers for the same config. Nothing is written to reach the answer, so adding a target cannot overwrite a preference. `explicitly_set` is a plain bool rather than a nullable `enabled` tri-state because `load_sync_config` validates straight into the model: a null there would raise on any older CLI that still types the field as bool, breaking every command after a downgrade. A config written before the field existed carries no preference and so follows the default. Automatic sync shipped opt-in, so `enabled: false` in such a config almost always means untouched rather than declined. User-facing wording moves from "automatic pull" to "automatic sync". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The module docstring still described automatic pull as something the user opts into with `skills sync auto on`, which contradicts the gate below it: having a sync target is what turns automatic sync on, and an explicit preference is what overrides that. Point the docstring at automatic_sync_enabled as the single resolved answer.
The --json output of skills sync target add carried automatic_sync_enabled only when it created a new target, not when it appended --only entries to an existing one, so the same command returned two payload shapes. Add the field to the append branch so machine readers see it on every target add.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Configuring a sync target is the statement of intent to keep it current, so automatic sync now resolves to on from that point instead of waiting for a separate
sync auto onthat most people never found. Getting to a synced state drops from three commands to two.Respecting an explicit choice
A stated preference always wins.
auto onandauto offrecord that the user chose, and adding more targets never overrides that choice, so turning it off once keeps it off.This is structural rather than a guard:
target addno longer writes to the auto config at all, so it cannot overwrite a preference.One place answers the question
The policy lives in
automatic_sync_enabled, read by the background gate, thesync autodisplay, andsync target add. Those three previously each readauto.enableddirectly and would have drifted into reporting different answers for the same config.Why two bools rather than a nullable tri-state
enabled: bool | Nonewould express this in one field, butload_sync_configvalidates straight into the model, so anullthere would raise on any older CLI that still types the field asbool, breaking every command after a downgrade. An unknown extra key is ignored instead, so an older CLI reads a config written by this one and falls back toenabled.Pre-existing configs
A config written before the field existed carries no preference and follows the default. Automatic sync shipped opt-in, so
enabled: falsein such a config almost always means untouched rather than declined, and treating those as opted-out would strand the majority to protect a near-empty set.Verification
pre-commit run --all-filesandpytestboth pass; 1053 tests. Also exercised the real binary end to end: fresh add turns it on,auto off, then two further target adds confirming it stays off and reports why, thenauto onrestores it.Companion changes
User-facing wording moves from "automatic pull" to "automatic sync". The matching docs update is in
goodeye-docs, and the section rename moves an anchor that a published changelog entry links to, fixed in the server repo.🤖 Generated with Claude Code