Skip to content

Commit 52d26c1

Browse files
LutzLangeclaude
andcommitted
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>
1 parent 96da0bf commit 52d26c1

13 files changed

Lines changed: 479 additions & 43 deletions

File tree

‎README.md‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,11 @@ Avoid repeating `/v1` in both URLs.
140140
programs that use the proxy (`programs.copilot`, `programs.vscode`). With
141141
`failClosed`, the default, the daemon leaves their managed entries as they
142142
are: the Copilot CLI and VS Code stay pointed at the loopback port, and those
143-
requests fail instead of reaching GitHub past the gateway. If the address could
143+
requests fail instead of reaching GitHub past the gateway. This needs entries
144+
to be in place: on a first apply without the proxy, or after the entries were
145+
removed by hand, nothing points at the loopback port, so VS Code on GitHub's
146+
models talks to GitHub directly and the Copilot CLI and VS Code on own models
147+
have no gateway models; the status says so. If the address could
144148
not be bound because another process holds it, that process receives those
145149
requests, with VS Code's Copilot tokens and the pairing value: use the default
146150
on devices with a single user, as for the proxy in general. With
@@ -348,7 +352,10 @@ startup stops the daemon from starting, as for the other programs). If the
348352
daemon's sidecar next to the file is gone, entries under the managed names
349353
that carry a pairing header are replaced on the next apply, and those that
350354
carry this daemon's own pairing value are removed when the program goes
351-
away; entries with another pairing value are left alone. The file is written owner-only. The
355+
away; entries with another pairing value are left alone. While the daemon runs
356+
without the proxy it has no pairing of its own, so any pairing value counts
357+
(only an agentdesktop daemon writes that header). The file is written
358+
owner-only. The
352359
daemon's sidecar next to it (`.providers.json.agentdesktop`, owner-only) keeps
353360
a copy of the file as last written, including the user's own providers and
354361
their `apiKey` values, until the next apply. Removing
@@ -432,7 +439,8 @@ resolved (and stops the daemon at startup, as for the other programs); an
432439
empty file is filled. If the daemon's sidecar next to the file is gone, an
433440
`agentdesktop` entry carrying a pairing header is replaced on the next apply,
434441
and one carrying this daemon's own pairing value is removed when the program
435-
goes away. Pass-through
442+
goes away (while the daemon runs without the proxy, one carrying any pairing
443+
value). Pass-through
436444
model keys are written as given, so a misspelt key reaches VS Code unchanged.
437445
The daemon's sidecar next to the file (`.chatLanguageModels.json.agentdesktop`,
438446
owner-only) keeps a copy of the file as last written, including the user's
@@ -672,7 +680,7 @@ device-wide state. The device page in the controller lists them under
672680
| `unchanged` | The program is configured and nothing needed to change. |
673681
| `removed` | The program is no longer configured and its managed content was removed. |
674682
| `conflict` | A managed file holds configuration the daemon will not overwrite; the detail names the file. |
675-
| `inactive` | The program uses the LLM gateway but the local LLM proxy is not running (see `llmProxy.error` in daemon-info): its entries were left pointing at the loopback port (`whenProxyUnavailable: failClosed`, the default) or removed (`failOpen`). |
683+
| `inactive` | The program uses the LLM gateway but the local LLM proxy is not running (see `llmProxy.error` in daemon-info): its entries were left pointing at the loopback port (`whenProxyUnavailable: failClosed`, the default; the detail says when no entries were in place, so the tool keeps its own settings) or removed (`failOpen`). |
676684
| `blocked` | The program had changes, but none were written because another program conflicted or failed; the detail names that program. |
677685
| `failed` | Planning or writing this program failed; the detail carries the error. |
678686

‎crates/agent/src/provider/copilot/reconcile.rs‎

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -108,18 +108,18 @@ pub(super) fn plan(
108108
return Ok(());
109109
};
110110
let Some((listen, pairing)) = proxy else {
111-
if crate::reconcile::fail_closed_without_proxy(gateway, plan) {
111+
if crate::reconcile::fail_closed_without_proxy(gateway, plan, || in_place(path, plan)) {
112112
tracing::debug!(
113113
path = %path.display(),
114-
"programs.copilot is configured but the local LLM proxy is not available; whenProxyUnavailable: failClosed, so the managed Copilot CLI providers are left as they are"
114+
"programs.copilot is configured but the local LLM proxy is not available; whenProxyUnavailable: failClosed, so the Copilot CLI providers file is left as it is"
115115
);
116116
return Ok(());
117117
}
118118
tracing::debug!(
119119
path = %path.display(),
120120
"programs.copilot is configured but the local LLM proxy is not available, so the Copilot CLI is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); whenProxyUnavailable: failOpen, removing the managed Copilot CLI providers"
121121
);
122-
plan.inactive(crate::reconcile::PROXY_ABSENT_REASON);
122+
plan.inactive(crate::reconcile::FAIL_OPEN_REASON);
123123
remove(path, &state_path, None, plan)?;
124124
return Ok(());
125125
};
@@ -164,13 +164,40 @@ fn remove(
164164
plan: &ReconcilePlan,
165165
) -> anyhow::Result<()> {
166166
if !json_merge::plan_remove_with(path, state_path, DESCRIPTION, DISPLAY_NAME, options(), plan)?
167-
&& let Some(pairing) = pairing
168167
{
169168
plan_remove_orphaned(path, pairing, plan)?;
170169
}
171170
Ok(())
172171
}
173172

173+
/// Whether a provider under a managed name carries a pairing header: with
174+
/// `Some(value)` that value only; with `None` (the daemon runs without the
175+
/// proxy and has no pairing of its own) any value, since only an
176+
/// agentdesktop daemon writes that header.
177+
fn paired(provider: &Value, pairing: Option<&str>) -> bool {
178+
let header = &provider["headers"][crate::llm_proxy::PAIRING_HEADER];
179+
let name = provider["name"].as_str();
180+
name.is_some_and(|name| name == PROVIDER_OPENAI || name == PROVIDER_ANTHROPIC)
181+
&& match pairing {
182+
Some(pairing) => header == pairing,
183+
None => !header.is_null(),
184+
}
185+
}
186+
187+
/// Whether the file holds a provider entry an agentdesktop daemon wrote (a
188+
/// managed name with a pairing header). A missing file is not in place; a
189+
/// file that cannot be read or parsed is logged and counts as not in place.
190+
fn in_place(path: &Path, plan: &ReconcilePlan) -> bool {
191+
match read_object(path, plan) {
192+
Ok(Some(current)) => providers(&current).any(|provider| paired(provider, None)),
193+
Ok(None) => false,
194+
Err(error) => {
195+
warn!(error = %format!("{error:#}"), "reading the Copilot CLI providers file");
196+
false
197+
}
198+
}
199+
}
200+
174201
/// Before agentdesktop has written the file (no sidecar), an entry under a
175202
/// managed key is the user's: a provider with one of the managed names that
176203
/// carries no pairing header, or a model under a managed provider name whose
@@ -228,12 +255,17 @@ fn foreign_managed_entry(
228255
}
229256

230257
/// Removal without a sidecar: entries under the managed names that carry
231-
/// this daemon's own pairing value were written for this daemon (before its
232-
/// sidecar was deleted), so they and the models under them are taken out by
233-
/// key; the file keeps its mode and is never deleted here. Entries with
234-
/// another pairing value, or none, are left alone, and a file that cannot be
235-
/// read is skipped with a warning rather than failing the whole apply.
236-
fn plan_remove_orphaned(path: &Path, pairing: &str, plan: &ReconcilePlan) -> anyhow::Result<()> {
258+
/// this daemon's pairing value (any pairing value while the daemon runs
259+
/// without the proxy and has none of its own) were written by an agentdesktop
260+
/// daemon, so they and the models under them are taken out by key; the file
261+
/// keeps its mode and is never deleted here. Entries with another pairing
262+
/// value, or none, are left alone, and a file that cannot be read is skipped
263+
/// with a warning rather than failing the whole apply.
264+
fn plan_remove_orphaned(
265+
path: &Path,
266+
pairing: Option<&str>,
267+
plan: &ReconcilePlan,
268+
) -> anyhow::Result<()> {
237269
let current = match read_object(path, plan) {
238270
Ok(Some(current)) => current,
239271
Ok(None) => return Ok(()),
@@ -243,9 +275,8 @@ fn plan_remove_orphaned(path: &Path, pairing: &str, plan: &ReconcilePlan) -> any
243275
}
244276
};
245277
let orphaned: Vec<String> = providers(&current)
246-
.filter(|provider| provider["headers"][crate::llm_proxy::PAIRING_HEADER] == pairing)
278+
.filter(|provider| paired(provider, pairing))
247279
.filter_map(|provider| provider["name"].as_str().map(str::to_owned))
248-
.filter(|name| name == PROVIDER_OPENAI || name == PROVIDER_ANTHROPIC)
249280
.collect();
250281
if orphaned.is_empty() {
251282
return Ok(());

‎crates/agent/src/provider/copilot/tests.rs‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1129,3 +1129,143 @@ fn config_rejects_a_non_object_entry_and_names_the_entry() {
11291129
"{error:#}"
11301130
);
11311131
}
1132+
1133+
// --- failClosed in place, failOpen reason, cleanup without a sidecar --------
1134+
1135+
fn fail_open_gateway() -> LlmGatewayConfig {
1136+
let mut gateway = sample_gateway();
1137+
gateway.when_proxy_unavailable = ProxyUnavailable::FailOpen;
1138+
gateway
1139+
}
1140+
1141+
#[test]
1142+
fn fail_closed_first_apply_reports_not_in_place() {
1143+
let dir = tempfile::tempdir().unwrap();
1144+
let path = dir.path().join("providers.json");
1145+
let config = sample_config();
1146+
let gateway = sample_gateway();
1147+
1148+
let changes = ReconcilePlan::default();
1149+
plan(&path, None, Some((&config, Some(&gateway))), &changes).unwrap();
1150+
assert_eq!(
1151+
changes.inactive_reason().as_deref(),
1152+
Some(crate::reconcile::FAIL_CLOSED_NOT_IN_PLACE_REASON)
1153+
);
1154+
changes.apply().unwrap();
1155+
assert!(
1156+
!path.exists(),
1157+
"nothing is written when nothing is in place"
1158+
);
1159+
assert!(!super::super::json_merge::state_path(&path).exists());
1160+
}
1161+
1162+
#[test]
1163+
fn fail_open_reports_the_policy() {
1164+
let dir = tempfile::tempdir().unwrap();
1165+
let path = dir.path().join("providers.json");
1166+
let config = sample_config();
1167+
let gateway = fail_open_gateway();
1168+
apply_managed_with(&path, &config, &gateway);
1169+
1170+
let changes = ReconcilePlan::default();
1171+
plan(&path, None, Some((&config, Some(&gateway))), &changes).unwrap();
1172+
assert_eq!(
1173+
changes.inactive_reason().as_deref(),
1174+
Some(crate::reconcile::FAIL_OPEN_REASON)
1175+
);
1176+
}
1177+
1178+
fn apply_managed_with(path: &std::path::Path, config: &CopilotConfig, gateway: &LlmGatewayConfig) {
1179+
let changes = ReconcilePlan::default();
1180+
plan(
1181+
path,
1182+
Some((listen_addr(), PAIRING)),
1183+
Some((config, Some(gateway))),
1184+
&changes,
1185+
)
1186+
.unwrap();
1187+
assert!(!changes.has_conflicts(), "{}", changes.render());
1188+
changes.apply().unwrap();
1189+
}
1190+
1191+
/// A user-owned file with our entries applied, the sidecar gone and the mode
1192+
/// loosened to 0640.
1193+
fn paired_entries_without_a_sidecar(path: &std::path::Path, gateway: &LlmGatewayConfig) {
1194+
write_user_owned_document(path);
1195+
apply_managed_with(path, &sample_config(), gateway);
1196+
fs::remove_file(super::super::json_merge::state_path(path)).unwrap();
1197+
#[cfg(unix)]
1198+
{
1199+
use std::os::unix::fs::PermissionsExt;
1200+
fs::set_permissions(path, fs::Permissions::from_mode(0o640)).unwrap();
1201+
}
1202+
assert_eq!(our_provider_count(&read(path)), 2);
1203+
}
1204+
1205+
fn assert_paired_entries_gone_user_entries_kept(path: &std::path::Path) {
1206+
let after = read(path);
1207+
assert_eq!(our_provider_count(&after), 0, "{after}");
1208+
assert!(
1209+
after["models"]
1210+
.as_array()
1211+
.unwrap()
1212+
.iter()
1213+
.all(|m| m["provider"] == "my-own-openai"),
1214+
"{after}"
1215+
);
1216+
assert_user_entries_present(&after);
1217+
#[cfg(unix)]
1218+
{
1219+
use std::os::unix::fs::PermissionsExt;
1220+
assert_eq!(
1221+
fs::metadata(path).unwrap().permissions().mode() & 0o777,
1222+
0o640,
1223+
"the file keeps its mode"
1224+
);
1225+
}
1226+
}
1227+
1228+
#[test]
1229+
fn fail_open_without_sidecar_removes_paired_entries_and_keeps_user_entries() {
1230+
let dir = tempfile::tempdir().unwrap();
1231+
let path = dir.path().join("providers.json");
1232+
let config = sample_config();
1233+
let gateway = fail_open_gateway();
1234+
paired_entries_without_a_sidecar(&path, &gateway);
1235+
1236+
// No proxy, so no pairing is known.
1237+
let changes = ReconcilePlan::default();
1238+
plan(&path, None, Some((&config, Some(&gateway))), &changes).unwrap();
1239+
changes.apply().unwrap();
1240+
assert_paired_entries_gone_user_entries_kept(&path);
1241+
}
1242+
1243+
#[test]
1244+
fn removal_without_proxy_and_sidecar_removes_paired_entries() {
1245+
let dir = tempfile::tempdir().unwrap();
1246+
let path = dir.path().join("providers.json");
1247+
paired_entries_without_a_sidecar(&path, &sample_gateway());
1248+
1249+
let changes = ReconcilePlan::default();
1250+
plan(&path, None, None, &changes).unwrap();
1251+
changes.apply().unwrap();
1252+
assert_paired_entries_gone_user_entries_kept(&path);
1253+
}
1254+
1255+
#[test]
1256+
fn known_pairing_keeps_entries_with_another_pairing() {
1257+
let dir = tempfile::tempdir().unwrap();
1258+
let path = dir.path().join("providers.json");
1259+
paired_entries_without_a_sidecar(&path, &sample_gateway());
1260+
1261+
let changes = ReconcilePlan::default();
1262+
plan(
1263+
&path,
1264+
Some((listen_addr(), "PAIRING-OTHER")),
1265+
None,
1266+
&changes,
1267+
)
1268+
.unwrap();
1269+
changes.apply().unwrap();
1270+
assert_eq!(our_provider_count(&read(&path)), 2);
1271+
}

‎crates/agent/src/provider/vscode/reconcile.rs‎

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -103,18 +103,18 @@ pub(super) fn plan(
103103
return Ok(());
104104
};
105105
let Some((listen, pairing)) = proxy else {
106-
if crate::reconcile::fail_closed_without_proxy(gateway, plan) {
106+
if crate::reconcile::fail_closed_without_proxy(gateway, plan, || in_place(path, plan)) {
107107
tracing::debug!(
108108
path = %path.display(),
109-
"programs.vscode is configured but the local LLM proxy is not available; whenProxyUnavailable: failClosed, so the managed chat language models are left as they are"
109+
"programs.vscode is configured but the local LLM proxy is not available; whenProxyUnavailable: failClosed, so the chat language models file is left as it is"
110110
);
111111
return Ok(());
112112
}
113113
tracing::debug!(
114114
path = %path.display(),
115115
"programs.vscode is configured but the local LLM proxy is not available, so VS Code is not pointed at the gateway; the reason is llmProxy.error in daemon-info (or daemon.llmProxy.listen is unset); whenProxyUnavailable: failOpen, removing the managed chat language models"
116116
);
117-
plan.inactive(crate::reconcile::PROXY_ABSENT_REASON);
117+
plan.inactive(crate::reconcile::FAIL_OPEN_REASON);
118118
remove(path, &state_path, None, plan)?;
119119
return Ok(());
120120
};
@@ -149,7 +149,7 @@ pub(super) fn plan(
149149
}
150150

151151
/// Managed entries are removed through the sidecar when there is one, and by
152-
/// this daemon's pairing value when there is none.
152+
/// pairing value when there is none (any value when `pairing` is `None`).
153153
fn remove(
154154
path: &Path,
155155
state_path: &Path,
@@ -163,13 +163,29 @@ fn remove(
163163
VsCode::DISPLAY_NAME,
164164
options(),
165165
plan,
166-
)? && let Some(pairing) = pairing
167-
{
166+
)? {
168167
plan_remove_orphaned(path, pairing, plan)?;
169168
}
170169
Ok(())
171170
}
172171

172+
/// Whether the file holds the `agentdesktop` vendor entry an agentdesktop
173+
/// daemon wrote (a model with a pairing header). A missing file is not in
174+
/// place; a file that cannot be read or parsed is logged and counts as not
175+
/// in place.
176+
fn in_place(path: &Path, plan: &ReconcilePlan) -> bool {
177+
match read_array(path, plan) {
178+
Ok(Some(current)) => current.iter().any(|vendor| {
179+
vendor["name"] == VsCodeConfig::VENDOR_NAME && carries_pairing(vendor, None)
180+
}),
181+
Ok(None) => false,
182+
Err(error) => {
183+
warn!(error = %format!("{error:#}"), "reading the VS Code chat language models file");
184+
false
185+
}
186+
}
187+
}
188+
173189
/// Whether a vendor entry carries the pairing header on one of its models,
174190
/// which only an agentdesktop daemon writes. With `Some(value)`, only that
175191
/// pairing value counts.
@@ -212,10 +228,15 @@ fn foreign_managed_entry(
212228
}
213229

214230
/// Removal without a sidecar: the vendor entry named `agentdesktop` whose
215-
/// models carry this daemon's own pairing value was written for this daemon,
216-
/// so it is taken out; the file keeps its mode and is never deleted here. A
217-
/// file that cannot be read is skipped with a warning.
218-
fn plan_remove_orphaned(path: &Path, pairing: &str, plan: &ReconcilePlan) -> anyhow::Result<()> {
231+
/// models carry this daemon's pairing value (any pairing value while the
232+
/// daemon runs without the proxy and has none of its own) was written by an
233+
/// agentdesktop daemon, so it is taken out; the file keeps its mode and is
234+
/// never deleted here. A file that cannot be read is skipped with a warning.
235+
fn plan_remove_orphaned(
236+
path: &Path,
237+
pairing: Option<&str>,
238+
plan: &ReconcilePlan,
239+
) -> anyhow::Result<()> {
219240
let current = match read_array(path, plan) {
220241
Ok(Some(current)) => current,
221242
Ok(None) => return Ok(()),
@@ -227,7 +248,7 @@ fn plan_remove_orphaned(path: &Path, pairing: &str, plan: &ReconcilePlan) -> any
227248
let remaining: Vec<Value> = current
228249
.iter()
229250
.filter(|vendor| {
230-
!(vendor["name"] == VsCodeConfig::VENDOR_NAME && carries_pairing(vendor, Some(pairing)))
251+
!(vendor["name"] == VsCodeConfig::VENDOR_NAME && carries_pairing(vendor, pairing))
231252
})
232253
.cloned()
233254
.collect();

0 commit comments

Comments
 (0)