fix: refresh outdated default Syncthing binary - #272
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new version-refresh path can be skipped when version parsing fails and the version-read helper can throw during startup, both of which undermine reliability of the intended update behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates SyncTrayzor’s startup configuration logic to keep the default (non-custom) Syncthing executable in sync with the bundled version, preventing issues when an older installed binary encounters newer config formats.
Changes:
- Extend
IFilesystemProvider.Copyto support overwriting and addGetFileVersionfor executable version comparisons. - During configuration initialization, detect when the bundled Syncthing binary is newer than the default installed copy and overwrite the installed copy (while leaving user-custom paths untouched).
File summaries
| File | Description |
|---|---|
| src/SyncTrayzor/Services/FilesystemProvider.cs | Adds overwrite-capable copy and executable file-version retrieval to support binary refresh logic. |
| src/SyncTrayzor/Services/Config/ConfigurationProvider.cs | Adds version comparison to refresh the default Syncthing binary from the bundled one when newer. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public Version GetFileVersion(string path) | ||
| { | ||
| var fileVersion = FileVersionInfo.GetVersionInfo(path).FileVersion?.Trim(); | ||
| return Version.TryParse(fileVersion, out var version) ? version : null; | ||
| } |
| { | ||
| var installedVersion = filesystem.GetFileVersion(expandedSyncthingPath); | ||
| var bundledVersion = filesystem.GetFileVersion(paths.SyncthingBackupPath); | ||
| if (installedVersion != null && bundledVersion != null && bundledVersion > installedVersion) |
|
This is an anti-feature in my opinion. First of all, it forcefully upgrades users who are running with --no-upgrade set. This may break their setup for no reason at all. The (clearly AI-written) comment claims that this "Prevents an older installed binary from failing to read configuration written by a newer bundled version" - but I fail to see how it does that? If the user is running an old syncthing version, then how would that write newer configurations? I get the idea of updating new installs on first run, but we already ship that. This adds nothing on top of that, except for breaking existing setups for no reason. |
Summary
Testing
dotnet test src -c Release --no-restore