Repository navigation
Conversation
ffc91a7 to
a01c77a
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Tick revision attribution and permission-only drift repair do not yet satisfy the documented behavior.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Adds opt-in periodic reconciliation while avoiding unnecessary rewrites of unchanged managed files.
Changes:
- Adds serialized reconciliation ticks and current-configuration tracking.
- Skips writes when content and permissions already match.
- Adds interval validation, documentation, and extensive tests.
Three issues were identified: stale tick revisions, incomplete permission repair, and missing documentation of the 30-day limit.
| File | Description |
|---|---|
README.md |
Documents periodic reconciliation. |
crates/agent/src/daemon.rs |
Initializes reconciliation state and scheduling. |
crates/agent/src/lib.rs |
Registers the tick module. |
crates/agent/src/provider/copilot/reconcile.rs |
Adapts Copilot reconciliation. |
crates/agent/src/provider/json_merge.rs |
Supports unchanged merged-file detection. |
crates/agent/src/provider/vscode/integration_tests.rs |
Tests VS Code reconciliation behavior. |
crates/agent/src/provider/vscode/reconcile.rs |
Adapts VS Code reconciliation. |
crates/agent/src/provider/vscode/settings.rs |
Adapts settings reconciliation. |
crates/agent/src/reconcile/mod.rs |
Serializes applies and tracks outcomes. |
crates/agent/src/reconcile/plan.rs |
Skips identical writes and checks modes. |
crates/agent/src/remote.rs |
Tracks current configuration and tick reports. |
crates/agent/src/tick.rs |
Implements periodic reconciliation. |
crates/core/src/config.rs |
Defines and validates the interval. |
schema/daemon-config.json |
Exposes the setting in JSON Schema. |
schema/daemon-config.md |
Documents the setting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if let Some(contents) = &change.contents | ||
| && already_in_place(&change.path, contents, change.permissions, &observed) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
Fixed in 5c3e9b1: Codex, Grok, OpenCode, Claude Code and Claude Desktop now plan an update for identical bytes at a looser mode, and one predicate (reconcile::grants_beyond) decides "looser than planned" for all of them, json_merge and the plan's skip check. Tests: identical_bytes_with_looser_mode_is_an_update_at_the_planned_mode and identical_bytes_within_mode_is_unchanged per provider.
| let Some((previous, report, result)) = reconciler.apply_read_under_lock(|| { | ||
| latest | ||
| .borrow() | ||
| .as_ref() | ||
| .map(|current| Arc::clone(¤t.config)) | ||
| }) else { |
There was a problem hiding this comment.
Fixed in 5c3e9b1: run_tick reads revision and configuration together under the apply lock and publishes that revision; a tick that applies nothing publishes nothing. Test: a_tick_reports_the_revision_the_apply_returns.
| |`daemon.openCode`|object|OpenCode paths.| | ||
| |`daemon.openCode.config`|string|Configuration file.| | ||
| |`daemon.openCode.plugin`|string|Credential plugin path.| | ||
| |`daemon.reconcileInterval`|string|Opt-in interval between periodic re-applies of the current<br>configuration, which repair drift (a managed file deleted or edited by<br>hand, a fixed conflict, a loosened mode) without rewriting anything<br>unchanged. Unset means no periodic re-apply; a controller-managed<br>device still re-applies the controller's configuration on every<br>reconnect. Must be greater than zero; read at startup only (restart the<br>daemon after changing it).| |
There was a problem hiding this comment.
Fixed in 5c3e9b1: the doc comment and the generated schema say "at most 30 days".
Part of agentdesktop-dev#86. Two programs that point Copilot at the local LLM proxy, user mode only: - `programs.copilot` writes the Copilot CLI's BYOK provider registry (`~/.copilot/providers.json`, or `$COPILOT_HOME` / `COPILOT_PROVIDERS_CONFIG`): providers `agentdesktop` (OpenAI-shaped) and `agentdesktop-anthropic` on the proxy's `/copilot-cli` route, with the pairing header; the user's own providers are kept. Discovery reports the CLI and its version from the npm manifest. - `programs.vscode` with `copilotChat: ownModels` (default) writes `chatLanguageModels.json` for Copilot Chat's built-in Custom Endpoint provider on `/vscode-copilot`; the user's entries are kept. - `copilotChat: githubModels` keeps GitHub's own models and the user's Copilot seat: the daemon sets `github.copilot.advanced.debug.overrideCapiUrl` in the user `settings.json` to the proxy's `/vscode-copilot-capi/<pairing>` route (pairing in the path, since VS Code cannot add a header), forwards to `llmGateway.proxyUrl` with the client's Copilot token moved aside, tunnels the Responses WebSocket for agent mode, forwards `_ping` without a credential, and keeps the override out of Settings Sync. `settings.json` is edited in place (JSONC): only the managed keys change, comments, key order and formatting stay, and removal restores the file byte for byte. Managed files are written 0600 and carry a sidecar that records what the daemon added, so removal takes out exactly that. A file the daemon cannot parse is a conflict and left alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…recision - Copilot CLI: removing our providers by their pairing header (no sidecar) no longer adds "models": null to a providers.json that has no models key. - README: the CAPI WebSocket tunnel closes after 4 to 5 minutes with the controller's default gatewayJwt.lifetime, also mid-turn; VS Code's reconnect behaviour is not verified. - README: the settings.json removal exceptions are listed in full (multi-line empty object, comment-only file). - README: the providers.json sidecar holds the user's own apiKey values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a01c77a to
fb0bb2b
Compare
…idecar mode repaired Review on agentdesktop-dev#135. - `.settings.json.agentdesktop` that is not JSON or not a known settings state now fails the apply, as the providers.json and chatLanguageModels.json sidecars already do: like a conflict, every managed file on the device stays untouched, also while `programs.vscode` is not configured (removal reads the sidecar too), and the daemon does not start. Before, the sidecar was ignored and rebuilt from the sidecar-less rules, so a user's own `overrideCapiUrl` was lost on removal. The error names the sidecar path and, for a syntax error, the line and column only, never a value. A sidecar without `version` (the earlier whole-document form) is still upgraded; a leading byte-order mark is accepted. - An identical sidecar whose mode is looser than 0600 is written again at 0600, as settings.json is; settings.json and its sidecar use the same check (a 0700 settings.json is now also tightened). The rewrite of the sidecar alone is not shown in the plan summary, since settings.json is unchanged. - README: the unreadable-sidecar behaviour and the sidecar's mode. Behaviour change: the test `an_unreadable_sidecar_warns_and_apply_proceeds` is replaced. Tests: 9 new, 2 of them unix-only. The file-mode tests in this module (old and new) assume umask 022. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each apply reports every managed program's outcome with a detail: applied,
unchanged, removed, conflict, inactive (the program uses the gateway but the
local proxy is not running), blocked (not applied because another program
conflicted) or failed. The daemon logs one line per program that was not
applied and sends the list with its ConfigStatus (additive proto fields
`programs`, `programs_reported`).
The controller stores the latest list per device (migration 0003,
`device_program_status`), returns it in `GET /api/v1/devices/{id}`, and the
device page shows a "Managed programs" card. Upgrade the controller first
(migration 0003 is one-way).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`daemon.reconcileInterval` (local startup setting, off when unset; zero or more than 30 days is rejected) re-applies the current configuration, which repairs drift between pushes: a deleted or hand-edited managed file, a fixed conflict, a loosened file mode. - The current configuration is the startup configuration, replaced by each pushed configuration that parses, and cleared on logout (also between controller sessions and while waiting to retry). - Applies are serialized: a push, the startup apply and a tick never overlap. - A tick reports to the controller only when its outcome differs from what was last sent on the stream; a pushed configuration whose apply failed is saved once a tick applies it. Behaviour change for every program: an apply no longer rewrites a file that already holds the planned bytes with no mode bit beyond the planned mode, so unchanged files and sidecars are no longer rewritten on each apply. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ired for every managed file Review on agentdesktop-dev#137. - Reconcile tick: the revision in a tick's status is the one read together with the configuration under the apply lock, not the one the tick woke up with. Before, a push between the two published the new revision's outcome under the old revision; the connection loop dropped it and delayed saving the pushed configuration by one interval. A tick that applies nothing (after a logout) publishes nothing. - Mode repair: Codex, Grok, OpenCode, Claude Code and Claude Desktop now treat a managed file with identical bytes but a mode looser than planned as an update and rewrite it at the planned mode (before, they returned `unchanged` before planning a write, so the plan's mode check never saw them). For Claude Code and Claude Desktop such a file is rewritten even without the owner marker, as the unchanged path already claimed it. One predicate, `reconcile::grants_beyond`, decides "looser than planned" for the plan's skip check, json_merge and the VS Code settings file and sidecar; Codex, Grok, OpenCode and Claude Code name their file mode once. Only the permission bits are compared (setuid/setgid/sticky are not); a managed path that is a symlink is replaced by a regular file on such a repair, as on any rewrite. - Schema: `daemon.reconcileInterval` states the 30-day maximum. Tests: 16 new (2 tick, 4 plan, 10 provider; the 10 provider tests and 2 of the plan tests are unix-only); the five existing tick tests take the closure's new return type. The file-mode tests assume umask 022, as the existing ones do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fb0bb2b to
5c3e9b1
Compare
…Unavailable (#146) * feat: report per-program configuration status to the controller Each apply reports every managed program's outcome with a detail: applied, unchanged, removed, conflict, inactive (the program uses the gateway but the local proxy is not running), blocked (not applied because another program conflicted) or failed. The daemon logs one line per program that was not applied and sends the list with its ConfigStatus (additive proto fields `programs`, `programs_reported`). The controller stores the latest list per device (migration 0003, `device_program_status`), returns it in `GET /api/v1/devices/{id}`, and the device page shows a "Managed programs" card. Upgrade the controller first (migration 0003 is one-way). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(agent): opt-in reconcile tick; skip identical writes `daemon.reconcileInterval` (local startup setting, off when unset; zero or more than 30 days is rejected) re-applies the current configuration, which repairs drift between pushes: a deleted or hand-edited managed file, a fixed conflict, a loosened file mode. - The current configuration is the startup configuration, replaced by each pushed configuration that parses, and cleared on logout (also between controller sessions and while waiting to retry). - Applies are serialized: a push, the startup apply and a tick never overlap. - A tick reports to the controller only when its outcome differs from what was last sent on the stream; a pushed configuration whose apply failed is saved once a tick applies it. Behaviour change for every program: an apply no longer rewrites a file that already holds the planned bytes with no mode bit beyond the planned mode, so unchanged files and sidecars are no longer rewritten on each apply. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(agent): tick reports the revision it applied; loosened modes repaired for every managed file Review on #137. - Reconcile tick: the revision in a tick's status is the one read together with the configuration under the apply lock, not the one the tick woke up with. Before, a push between the two published the new revision's outcome under the old revision; the connection loop dropped it and delayed saving the pushed configuration by one interval. A tick that applies nothing (after a logout) publishes nothing. - Mode repair: Codex, Grok, OpenCode, Claude Code and Claude Desktop now treat a managed file with identical bytes but a mode looser than planned as an update and rewrite it at the planned mode (before, they returned `unchanged` before planning a write, so the plan's mode check never saw them). For Claude Code and Claude Desktop such a file is rewritten even without the owner marker, as the unchanged path already claimed it. One predicate, `reconcile::grants_beyond`, decides "looser than planned" for the plan's skip check, json_merge and the VS Code settings file and sidecar; Codex, Grok, OpenCode and Claude Code name their file mode once. Only the permission bits are compared (setuid/setgid/sticky are not); a managed path that is a symlink is replaced by a regular file on such a repair, as on any rewrite. - Schema: `daemon.reconcileInterval` states the 30-day maximum. Tests: 16 new (2 tick, 4 plan, 10 provider; the 10 provider tests and 2 of the plan tests are unix-only); the five existing tick tests take the closure's new return type. The file-mode tests assume umask 022, as the existing ones do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: llmGateway.whenProxyUnavailable and a GitHub Copilot example Proxy-unavailable policy - While the daemon runs without its local LLM proxy, the programs behind it used to remove their managed entries; for VS Code on GitHub's models that removes the CAPI override, so Copilot Chat talks to GitHub directly, past the gateway. - `llmGateway.whenProxyUnavailable` now decides for `programs.copilot` and `programs.vscode`: `failClosed` (default) leaves the managed files as they are, so the tools stay pointed at the loopback port and fail; `failOpen` removes them until the proxy is back. The program is `inactive` with a reason that names the policy. - Program removal, a mode switch, `useLlmGateway: false` and a missing `proxyUrl` remove as before. The controller's configuration view offers the choice ("Fail open without the local proxy") when a Copilot program is selected. Older daemons and controllers reject the key (`deny_unknown_fields`); the default needs no key. Example - `examples/copilot/`: Dex, agentgateway, a daemon config and a step-by-step guide for the Copilot CLI, VS Code on own models and VS Code on GitHub's models, with what to check at each step (drift, conflicts, removal, the proxy gone), plus the settings the GitHub organisation owner configures (`github-side.md`). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(agent): failClosed says whether anything is in place; failOpen names its policy and cleans up without a sidecar Review on #138. - failClosed without the proxy: the program reports "no managed entries are in place, so the tool keeps its own settings" when nothing of the daemon's is in the file (a first apply without the proxy, or entries removed by hand), and keeps "the managed entries stay and the tool fails" when they are. In place = a managed provider or chat-model entry with a pairing header, or a VS Code override pointing at an agentdesktop loopback route; the file is read for this only under failClosed and never written. A file that cannot be read is logged and counts as not in place; one that does not parse counts as not in place (the merge reports it once the proxy is back). Before, the status claimed the tool fails while VS Code on GitHub's models went to GitHub directly. - failOpen: all three planners report "the managed entries are removed until the proxy is back (whenProxyUnavailable: failOpen)" instead of the generic proxy-absent reason, which is removed. - Cleanup without a sidecar while the daemon has no pairing (proxy off, failOpen or program removed): Copilot CLI providers and VS Code chat-model entries under the managed names that carry any pairing header are removed; user entries stay; with a known pairing only that value counts, as before. Before, such entries were left pointing at the unavailable port. - README (when failClosed has nothing to keep, the status row, the any-pairing rule), schema (`llmGateway.whenProxyUnavailable`), the controller's toggle text and a Storybook fixture. Tests: 13 new (Copilot CLI 5, VS Code chat models 5, VS Code settings 3; 2 of them guard existing behaviour). The two "keeps its mode" tests assume umask 022, as the existing file-mode tests do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>


Part of #86 (series overview: #86 overview). Stacked on #136; review the last two commits (the second addresses the review below).
Why
Between configuration pushes, a deleted or hand-edited managed file, or a loosened file mode, stays wrong until the next push or restart.
Change
daemon.reconcileIntervalis a local startup setting, off when unset. It re-applies the current configuration. Zero or more than 30 days is rejected.Risk and compatibility
How to review
tick.rsholds the tick.remote.rsholds the current configuration and tick reporting; it keeps the session supervisor from fix: retry transient OIDC refresh failures instead of disabling the controller #129.reconcile/mod.rsholds the apply lock.reconcile/plan.rsholds the identical-write skip.Review fixes
The last commit (5c3e9b1) addresses the Copilot review:
reconcile::grants_beyond, decides it everywhere.Tests
CI green. Unit tests cover the dedup, lock ordering, logout and skipped writes. A Docker test deletes a managed file, checks that the tick restores it, and checks that later ticks write nothing. Lab-tested on a Linux VM with a 20 s interval:
chmod 664goes back to 600;Follow-ups
🤖 Generated with Claude Code