Conversation
Reviewer's GuideImplements a new SettingSync protocol for synchronizing all dot-prefixed configuration directories (e.g., .obsidian, .agents) alongside existing note/file sync flows, including protocol constants, engine wiring, local state tracking, and an echo-hash mechanism to avoid sync loops. Sequence diagram for incremental SettingSync and echo-hash handlingsequenceDiagram
actor User
participant SyncEngine
participant SettingSync
participant WSClient
participant Server
participant FS as FileSystem
User->>SyncEngine: sync_once()
activate SyncEngine
SyncEngine->>SettingSync: request_sync()
activate SettingSync
SettingSync->>SettingSync: _collect_local_settings()
SettingSync->>WSClient: send(WSMessage SettingSync)
deactivate SettingSync
WSClient-->>Server: SettingSync request
Server-->>WSClient: SettingSyncModify(path, content, mtime)
WSClient->>SettingSync: _on_sync_modify(msg)
activate SettingSync
SettingSync->>FS: write_text(path, content)
SettingSync->>FS: os.utime(path, mtime)
SettingSync->>SettingSync: _echo_hashes[path] = file_content_hash_binary(path)
SettingSync->>SettingSync: _received_modify++
SettingSync->>SettingSync: _check_all_received()
deactivate SettingSync
Server-->>WSClient: SettingSyncEnd(lastTime, needModifyCount, needDeleteCount)
WSClient->>SettingSync: _on_sync_end(msg)
activate SettingSync
SettingSync->>SettingSync: _expected_modify = needModifyCount
SettingSync->>SettingSync: _expected_delete = needDeleteCount
SettingSync->>SettingSync: _pending_last_time = lastTime
SettingSync->>SettingSync: _got_end = True
SettingSync->>SettingSync: _check_all_received()
SettingSync->>SyncEngine: state.last_setting_sync_time = lastTime
SettingSync->>SyncEngine: state.save()
SettingSync->>SyncEngine: is_sync_complete = True
deactivate SettingSync
SyncEngine-->>User: sync_once completed
deactivate SyncEngine
User->>SyncEngine: on_local_change(rel_path in dot directory)
activate SyncEngine
SyncEngine->>SettingSync: push_modify(rel_path)
activate SettingSync
SettingSync->>FS: read_text(rel_path)
SettingSync->>SettingSync: hash = file_content_hash_binary(rel_path)
SettingSync->>SettingSync: if _echo_hashes[rel_path] == hash then return
SettingSync->>WSClient: send(WSMessage SettingModify)
SettingSync->>SettingSync: _echo_hashes[rel_path] = hash
deactivate SettingSync
WSClient-->>Server: SettingModify
deactivate SyncEngine
Class diagram for SyncEngine and new SettingSync integrationclassDiagram
class SyncEngine {
- AppConfig config
- Path vault_path
- NoteSync note_sync
- FileSync file_sync
- FolderSync folder_sync
- SettingSync setting_sync
- set~str~ _ignored_files
- bool _watch_enabled
+ __init__(config: AppConfig)
+ run() async
+ sync_once() async
+ push() async
+ on_local_change(rel_path: str) async
+ on_local_delete(rel_path: str) async
+ on_local_rename(new_rel: str, old_rel: str) async
+ _register_handlers()
+ _is_note(rel_path: str) bool
+ _is_config(rel_path: str) bool
+ _should_sync_file(rel_path: str) bool
+ _initial_sync() async
+ _wait_note_sync(timeout: float) async
+ _wait_file_sync(timeout: float) async
+ _wait_setting_sync(timeout: float) async
+ _push_all_files() async
+ _push_all_settings() async
}
class SettingSync {
- SyncEngine engine
- AppConfig config
- Path vault_path
- bool _sync_complete
- int _expected_modify
- int _expected_delete
- int _received_modify
- int _received_delete
- bool _got_end
- int _pending_last_time
- dict~str, str~ _echo_hashes
+ __init__(engine: SyncEngine)
+ is_sync_complete bool
+ register_handlers()
+ request_sync() async
+ request_full_sync() async
+ push_modify(rel_path: str, force: bool) async
+ push_delete(rel_path: str) async
+ push_rename(new_rel: str, old_rel: str) async
+ _on_sync_modify(msg: WSMessage) async
+ _on_sync_delete(msg: WSMessage) async
+ _on_sync_rename(msg: WSMessage) async
+ _on_sync_mtime(msg: WSMessage) async
+ _on_sync_end(msg: WSMessage) async
+ _reset_counters()
+ _check_all_received()
+ _commit_last_time()
+ _try_remove_empty_parent(file_path: Path)
+ _collect_local_settings() list~dict~
}
class SyncState {
+ int last_note_sync_time
+ int last_file_sync_time
+ int last_setting_sync_time
- str _path
+ load(vault_dir: Path) SyncState
+ save()
}
class FileSync {
+ _collect_local_files() list~dict~
}
class NoteSync
class FolderSync
class WSClient {
+ on(action: str, handler)
+ send(msg: WSMessage) async
}
class WSMessage {
+ str action
+ dict data
+ __init__(action: str, data: dict)
}
SyncEngine *-- NoteSync
SyncEngine *-- FileSync
SyncEngine *-- FolderSync
SyncEngine *-- SettingSync
SyncEngine *-- SyncState
SyncEngine *-- WSClient
SettingSync --> SyncEngine
SettingSync --> WSClient
SettingSync --> WSMessage
FileSync --> SyncEngine
SyncState ..> Path
Flow diagram for file classification between notes, settings, and regular filesflowchart TD
A[Start]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The logic for identifying config paths (leading dot segment) is duplicated in
sync_engine._is_config,file_sync._collect_local_files, and_is_config_pathinsetting_sync; consider centralizing this into a single helper to avoid drift and keep behavior consistent. - The
_echo_hashescache inSettingSyncis unbounded and keyed by path; if a vault has many churned config files this could grow indefinitely—consider a simple eviction strategy (e.g., LRU with a max size) or periodically pruning entries for paths that no longer exist. - Both
_push_all_filesand_push_all_settingsperform separaterglob('*')passes over the vault; if vaults are large this may be expensive—consider consolidating into a single walk that routes each path to the appropriate sync channel.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for identifying config paths (leading dot segment) is duplicated in `sync_engine._is_config`, `file_sync._collect_local_files`, and `_is_config_path` in `setting_sync`; consider centralizing this into a single helper to avoid drift and keep behavior consistent.
- The `_echo_hashes` cache in `SettingSync` is unbounded and keyed by path; if a vault has many churned config files this could grow indefinitely—consider a simple eviction strategy (e.g., LRU with a max size) or periodically pruning entries for paths that no longer exist.
- Both `_push_all_files` and `_push_all_settings` perform separate `rglob('*')` passes over the vault; if vaults are large this may be expensive—consider consolidating into a single walk that routes each path to the appropriate sync channel.
## Individual Comments
### Comment 1
<location path="fns_cli/file_sync.py" line_range="503-506" />
<code_context>
if self.engine.is_excluded(rel) or rel.endswith(".md"):
continue
- if rel.startswith(".obsidian/") and not self.config.sync.sync_config:
+ first = rel.split("/")[0]
+ if first.startswith(".") and not self.config.sync.sync_config:
continue
- if not rel.startswith(".obsidian/") and not self.config.sync.sync_files:
+ if not first.startswith(".") and not self.config.sync.sync_files:
continue
try:
</code_context>
<issue_to_address>
**issue (bug_risk):** Config files are still included in FileSync when `sync_config` is enabled, causing overlap with SettingSync.
With these conditions, any path whose first segment starts with `.` is always included in `FileSync` when `sync_config` is true, even though the same paths are now also processed by `SettingSync` (`_is_config` / `_collect_local_settings`). That means config files are synced by both mechanisms.
If config/settings are meant to be owned by `SettingSync`, `FileSync._collect_local_files` should instead always exclude dot-prefixed directories (or at least when setting sync is enabled), rather than tying them to `sync_config`, to prevent duplicate and inconsistent syncing.
</issue_to_address>
### Comment 2
<location path="fns_cli/sync_engine.py" line_range="286-288" />
<code_context>
+ break
+ await asyncio.sleep(0.5)
+
async def _push_all_files(self) -> None:
"""Upload every non-note, non-excluded file in the vault."""
for fp in self.vault_path.rglob("*"):
</code_context>
<issue_to_address>
**issue (bug_risk):** Full push still sends config files through FileSync in addition to SettingSync.
Because `_push_all_files` only filters with `is_excluded` and `_is_note`, config files in dot-prefixed directories still go through `file_sync.push_upload`. With `push()` now calling both `_push_all_files()` (`sync_files`) and `_push_all_settings()` (`sync_config`), those config files will be sent twice when both options are enabled. If config should be handled only by the settings protocol, `_push_all_files` should also skip `_is_config(rel)` to avoid double processing.
</issue_to_address>
### Comment 3
<location path="fns_cli/setting_sync.py" line_range="41-49" />
<code_context>
+ return msg_data if isinstance(msg_data, dict) else {}
+
+
+def _is_config_path(rel: str) -> bool:
+ """Check whether a relative path belongs to config/settings scope.
+
+ This matches the Obsidian plugin behaviour: anything inside a dot-prefixed
+ directory (e.g. .obsidian, .agents) is treated as a setting file.
+ Standard exclusions (.git, .trash) are handled by is_excluded() upstream.
+ """
+ first = rel.split("/")[0]
+ return first.startswith(".")
+
+
</code_context>
<issue_to_address>
**suggestion:** Config path detection logic is duplicated and may drift from `SyncEngine._is_config`.
There are now two separate implementations of this rule: `SyncEngine._is_config` and `_is_config_path`. If one is updated (e.g., changing which dot-prefixed directories count as config) and the other isn’t, they may diverge and cause inconsistencies between what the engine sends to `SettingSync` and what `_collect_local_settings` sees. Please centralize this logic (e.g., via a shared helper or by delegating to the engine) so config path classification is defined in a single place.
Suggested implementation:
```python
def _extract_inner(msg_data: dict) -> dict:
"""Server wraps payloads as {code, status, message, data: {actual fields}}."""
if isinstance(msg_data, dict) and "data" in msg_data:
inner = msg_data["data"]
if isinstance(inner, dict):
return inner
return msg_data if isinstance(msg_data, dict) else {}
def _is_config_path(rel: str) -> bool:
"""Check whether a relative path belongs to config/settings scope.
Delegates to SyncEngine._is_config to keep config path classification in one place.
This ensures that what the engine considers "config" matches what
_collect_local_settings() sees.
"""
# Local import to avoid potential circular imports at module load time.
from .sync_engine import SyncEngine
return SyncEngine._is_config(rel)
```
To fully centralize config path detection and avoid drift:
1. Update `SyncEngine._is_config` (in `fns_cli/sync_engine.py`) to be a `@staticmethod` or `@classmethod` that accepts `(rel: str)` so it can be called as `SyncEngine._is_config(rel)` without an instance.
2. Ensure the logic for classifying config paths lives only in `SyncEngine._is_config` and remove any duplicated rules from there if they were previously inlined in multiple places.
3. If `SyncEngine` already imports `setting_sync`, you may need to move the shared helper into a third module (e.g., `fns_cli/config_paths.py`) to avoid circular imports, and then have both `SyncEngine._is_config` and `_is_config_path` delegate to that shared function instead of importing `SyncEngine` here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+503
to
+506
| first = rel.split("/")[0] | ||
| if first.startswith(".") and not self.config.sync.sync_config: | ||
| continue | ||
| if not rel.startswith(".obsidian/") and not self.config.sync.sync_files: | ||
| if not first.startswith(".") and not self.config.sync.sync_files: |
There was a problem hiding this comment.
issue (bug_risk): Config files are still included in FileSync when sync_config is enabled, causing overlap with SettingSync.
With these conditions, any path whose first segment starts with . is always included in FileSync when sync_config is true, even though the same paths are now also processed by SettingSync (_is_config / _collect_local_settings). That means config files are synced by both mechanisms.
If config/settings are meant to be owned by SettingSync, FileSync._collect_local_files should instead always exclude dot-prefixed directories (or at least when setting sync is enabled), rather than tying them to sync_config, to prevent duplicate and inconsistent syncing.
Comment on lines
286
to
288
| async def _push_all_files(self) -> None: | ||
| """Upload every non-note, non-excluded file in the vault.""" | ||
| for fp in self.vault_path.rglob("*"): |
There was a problem hiding this comment.
issue (bug_risk): Full push still sends config files through FileSync in addition to SettingSync.
Because _push_all_files only filters with is_excluded and _is_note, config files in dot-prefixed directories still go through file_sync.push_upload. With push() now calling both _push_all_files() (sync_files) and _push_all_settings() (sync_config), those config files will be sent twice when both options are enabled. If config should be handled only by the settings protocol, _push_all_files should also skip _is_config(rel) to avoid double processing.
Comment on lines
+41
to
+49
| def _is_config_path(rel: str) -> bool: | ||
| """Check whether a relative path belongs to config/settings scope. | ||
|
|
||
| This matches the Obsidian plugin behaviour: anything inside a dot-prefixed | ||
| directory (e.g. .obsidian, .agents) is treated as a setting file. | ||
| Standard exclusions (.git, .trash) are handled by is_excluded() upstream. | ||
| """ | ||
| first = rel.split("/")[0] | ||
| return first.startswith(".") |
There was a problem hiding this comment.
suggestion: Config path detection logic is duplicated and may drift from SyncEngine._is_config.
There are now two separate implementations of this rule: SyncEngine._is_config and _is_config_path. If one is updated (e.g., changing which dot-prefixed directories count as config) and the other isn’t, they may diverge and cause inconsistencies between what the engine sends to SettingSync and what _collect_local_settings sees. Please centralize this logic (e.g., via a shared helper or by delegating to the engine) so config path classification is defined in a single place.
Suggested implementation:
def _extract_inner(msg_data: dict) -> dict:
"""Server wraps payloads as {code, status, message, data: {actual fields}}."""
if isinstance(msg_data, dict) and "data" in msg_data:
inner = msg_data["data"]
if isinstance(inner, dict):
return inner
return msg_data if isinstance(msg_data, dict) else {}
def _is_config_path(rel: str) -> bool:
"""Check whether a relative path belongs to config/settings scope.
Delegates to SyncEngine._is_config to keep config path classification in one place.
This ensures that what the engine considers "config" matches what
_collect_local_settings() sees.
"""
# Local import to avoid potential circular imports at module load time.
from .sync_engine import SyncEngine
return SyncEngine._is_config(rel)To fully centralize config path detection and avoid drift:
- Update
SyncEngine._is_config(infns_cli/sync_engine.py) to be a@staticmethodor@classmethodthat accepts(rel: str)so it can be called asSyncEngine._is_config(rel)without an instance. - Ensure the logic for classifying config paths lives only in
SyncEngine._is_configand remove any duplicated rules from there if they were previously inlined in multiple places. - If
SyncEnginealready importssetting_sync, you may need to move the shared helper into a third module (e.g.,fns_cli/config_paths.py) to avoid circular imports, and then have bothSyncEngine._is_configand_is_config_pathdelegate to that shared function instead of importingSyncEnginehere.
…n, .agents, etc.) Adds full support for synchronizing Obsidian config directories (e.g. .obsidian, .agents/skills) via the SettingSync WebSocket protocol. - protocol.py: add SettingSync action constants - state.py: persist last_setting_sync_time - setting_sync.py: new module handling SettingSyncModify/Delete/Rename/Mtime/End - sync_engine.py: wire setting_sync into pull/push/run and watcher callbacks - file_sync.py: exclude all dot-prefixed dirs from FileSync to avoid conflicts Resolves the issue where config files uploaded by the Obsidian plugin never reached the CLI because the CLI only implemented NoteSync/FileSync.
LLQWQ
force-pushed
the
feat/setting-sync
branch
from
c0d7537 to
193d3d4
Compare
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
2 participants
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.
Closes #12
Summary
This PR adds full SettingSync (config directory) support to FastNodeSync-CLI, enabling synchronization of dot-prefixed directories such as
.obsidianand.agents/skills.What changed
protocol.py— Added SettingSync action constants (SettingSync,SettingModify,SettingDelete,SettingSyncModify,SettingSyncDelete,SettingSyncRename,SettingSyncMtime,SettingSyncEnd).state.py— Addedlast_setting_sync_timefield and persisted it to.fns_state.json.setting_sync.py— New module implementing the complete SettingSync protocol:sync_engine.py— Wiredsetting_syncintorun,pull,push,sync_once, and local file watcher callbacks.file_sync.py— Broadened config exclusion from.obsidian/only to all dot-prefixed directories, preventing FileSync and SettingSync from fighting over the same files.Verification
fns-cli.service; logs showSettingSyncModifymessages for.agents/skills/...and.obsidian/plugins/.....agents/skills/directory with all expected skill files.Notes
config.yamlwas not included in this PR because it is local configuration.setting_sync.pyfollows the existing patterns established bynote_sync.pyandfile_sync.pyfor consistency.Summary by Sourcery
Add support for synchronizing configuration files in dot-prefixed directories via a dedicated SettingSync protocol alongside existing note, file, and folder sync.
New Features:
Enhancements: