From c9a73726be6e6edd4e5d0c9c44abdf07d550ecd0 Mon Sep 17 00:00:00 2001 From: Wes Date: Sun, 26 Jul 2026 09:16:36 -0700 Subject: [PATCH 1/4] fix(mobile): validate invite relay destinations (#2986) ## Summary - require invite relay destinations to be secure public origins in production - reject non-public and ambiguous IP literals before confirmation and again before the claim request - disable redirects for invite claims so a validated relay cannot redirect the request elsewhere - preserve explicit debug-only localhost support ## Validation - pre-commit `dart format` and `flutter analyze` - pre-push full mobile test suite: 666 passed, 1 skipped - independent source reviews from Princess Donut and Mongo found no remaining blockers ## Scope and residual risk This fixes the mobile invite trust boundary without changing NIP-98 or NIP-42. Hostnames are not resolved and pinned by this patch, so DNS rebinding remains a networking-layer residual risk requiring connect-time resolution/pinning. Signed-off-by: Wes Co-authored-by: Carl --- .../invites/invite_join_provider.dart | 33 ++-- mobile/lib/shared/deeplink/deep_link.dart | 22 ++- mobile/lib/shared/relay/relay_validation.dart | 164 ++++++++++++++++++ .../invites/invite_join_provider_test.dart | 37 +++- .../test/shared/deeplink/deep_link_test.dart | 29 ++++ .../shared/relay/relay_validation_test.dart | 161 +++++++++++++++++ 6 files changed, 429 insertions(+), 17 deletions(-) create mode 100644 mobile/lib/shared/relay/relay_validation.dart create mode 100644 mobile/test/shared/relay/relay_validation_test.dart diff --git a/mobile/lib/features/invites/invite_join_provider.dart b/mobile/lib/features/invites/invite_join_provider.dart index 0cd58d47a7..0e5547aca5 100644 --- a/mobile/lib/features/invites/invite_join_provider.dart +++ b/mobile/lib/features/invites/invite_join_provider.dart @@ -7,6 +7,7 @@ import 'package:nostr/nostr.dart' as nostr; import '../../shared/auth/auth.dart'; import '../../shared/deeplink/deep_link.dart'; import '../../shared/relay/relay_session.dart'; +import '../../shared/relay/relay_validation.dart'; final inviteJoinHttpClientProvider = Provider((ref) { final client = http.Client(); @@ -68,6 +69,7 @@ class InviteJoinNotifier extends Notifier { InviteJoinState build() => const InviteJoinState(); Future prepare(InviteDeepLink invite) async { + validateInviteRelayUri(Uri.parse(invite.relayUrl)); final communities = await ref.read(communityListProvider.future); final existing = _existingCommunity(communities, invite.relayUrl); if (existing != null) { @@ -121,22 +123,25 @@ class InviteJoinNotifier extends Notifier { if (invite.policyReceipt != null) 'policy_receipt': invite.policyReceipt, }); + final relayUri = Uri.parse(invite.relayUrl); + validateInviteRelayUri(relayUri); final url = _claimUrlFromRelay(invite.relayUrl); - final response = await ref + final request = http.Request('POST', Uri.parse(url)) + ..followRedirects = false + ..headers.addAll({ + 'Authorization': buildNip98AuthHeader( + method: 'POST', + url: url, + bodyBytes: utf8.encode(body), + nsec: keys.nsec, + ), + 'Content-Type': 'application/json', + }) + ..body = body; + final streamedResponse = await ref .read(inviteJoinHttpClientProvider) - .post( - Uri.parse(url), - headers: { - 'Authorization': buildNip98AuthHeader( - method: 'POST', - url: url, - bodyBytes: utf8.encode(body), - nsec: keys.nsec, - ), - 'Content-Type': 'application/json', - }, - body: body, - ); + .send(request); + final response = await http.Response.fromStream(streamedResponse); final decoded = jsonDecode(response.body.isEmpty ? '{}' : response.body); if (response.statusCode < 200 || response.statusCode >= 300) { final message = decoded is Map && decoded['error'] is String diff --git a/mobile/lib/shared/deeplink/deep_link.dart b/mobile/lib/shared/deeplink/deep_link.dart index b2e57b8b00..a208048dbd 100644 --- a/mobile/lib/shared/deeplink/deep_link.dart +++ b/mobile/lib/shared/deeplink/deep_link.dart @@ -7,6 +7,8 @@ /// half-formed target. library; +import '../relay/relay_validation.dart'; + /// A parsed deep link supported by the app. sealed class BuzzDeepLink { const BuzzDeepLink(); @@ -107,8 +109,9 @@ MessageDeepLink? parseMessageDeepLink(Uri uri) { /// /// Accepted forms: /// - `https:///invite/` -> `wss://` + code -/// - `http:///invite/` -> `ws://` + code -/// - `buzz://join?relay=&code=` -> relay + code +/// - `http://localhost/invite/` -> `ws://localhost` + code in debug builds +/// - `buzz://join?relay=&code=` -> relay + code +/// - `buzz://join?relay=&code=` -> local relay in debug /// /// Rejects credentials, fragments, missing params, nested relay credentials, and /// non-invite paths so scanners do not accidentally treat arbitrary URLs as @@ -131,6 +134,11 @@ InviteDeepLink? parseInviteDeepLink(Uri uri) { relayUri.hasFragment) { return null; } + try { + validateInviteRelayUri(relayUri); + } on FormatException { + return null; + } final normalizedRelay = Uri( scheme: relayUri.scheme, host: relayUri.host, @@ -155,6 +163,16 @@ InviteDeepLink? parseInviteDeepLink(Uri uri) { return null; } final relayScheme = uri.scheme == 'https' ? 'wss' : 'ws'; + final relayUri = Uri( + scheme: relayScheme, + host: uri.host, + port: uri.hasPort ? uri.port : null, + ); + try { + validateInviteRelayUri(relayUri); + } on FormatException { + return null; + } final relay = Uri( scheme: relayScheme, host: uri.host, diff --git a/mobile/lib/shared/relay/relay_validation.dart b/mobile/lib/shared/relay/relay_validation.dart new file mode 100644 index 0000000000..eccbadf974 --- /dev/null +++ b/mobile/lib/shared/relay/relay_validation.dart @@ -0,0 +1,164 @@ +import 'dart:io'; + +import 'package:flutter/foundation.dart'; + +/// Validates an untrusted relay origin before the mobile app connects to it. +/// +/// Production relay URLs must use TLS and may not contain local or otherwise +/// non-public IP literals. Debug builds retain plaintext localhost support for +/// local development, but other non-public destinations remain blocked. +void validateInviteRelayUri( + Uri uri, { + bool allowInsecureLocalhost = kDebugMode, +}) { + if (uri.host.isEmpty || + uri.userInfo.isNotEmpty || + uri.hasFragment || + (uri.path.isNotEmpty && uri.path != '/') || + uri.hasQuery) { + throw const FormatException('Relay URL must be an origin'); + } + + if (uri.scheme != 'ws' && uri.scheme != 'wss') { + throw FormatException('Invalid relay URL scheme: ${uri.scheme}'); + } + + final host = uri.host.toLowerCase(); + final isLocalhost = host == 'localhost' || host.endsWith('.localhost'); + if (uri.scheme != 'wss' && !(allowInsecureLocalhost && isLocalhost)) { + throw const FormatException('Relay URL must use WSS'); + } + if (isLocalhost) { + if (!allowInsecureLocalhost) { + throw const FormatException('Relay URL cannot target localhost'); + } + return; + } + + final address = InternetAddress.tryParse(host); + if (_looksLikeNumericAddress(host)) { + // Only canonical dotted-decimal IPv4 is accepted. Ambiguous legacy forms + // such as a single integer, hexadecimal, shortened components, or leading + // zeroes have varied interpretation across URI/network stacks. + final ipv4Parts = host.split('.'); + final hasLeadingZero = ipv4Parts.any( + (part) => part.length > 1 && part.startsWith('0'), + ); + if (address == null || + address.type != InternetAddressType.IPv4 || + address.address != host || + ipv4Parts.length != 4 || + hasLeadingZero) { + throw const FormatException('Relay URL contains an invalid IP address'); + } + } + if (address != null) { + if (!_isPublicAddress(address)) { + throw const FormatException( + 'Relay URL cannot target non-public network addresses', + ); + } + return; + } + + // DNS hostnames are allowed here. Preventing a hostname from resolving or + // rebinding to a non-public address requires connect-time resolution and + // address pinning in the networking layer. +} + +bool _looksLikeNumericAddress(String host) { + if (RegExp(r'^\d+$').hasMatch(host) || + RegExp(r'^0x[0-9a-f]+$', caseSensitive: false).hasMatch(host)) { + return true; + } + final parts = host.split('.'); + return parts.length > 1 && + parts.every( + (part) => + RegExp(r'^\d+$').hasMatch(part) || + RegExp(r'^0x[0-9a-f]+$', caseSensitive: false).hasMatch(part), + ); +} + +bool _isPublicAddress(InternetAddress address) { + final bytes = address.rawAddress; + if (address.type == InternetAddressType.IPv4) { + return !_isNonPublicIpv4(bytes); + } + if (address.type != InternetAddressType.IPv6 || bytes.length != 16) { + return false; + } + + // IPv4-compatible and IPv4-mapped IPv6 addresses inherit the embedded + // IPv4 address's classification. + final firstTenZero = bytes.take(10).every((byte) => byte == 0); + if (firstTenZero && + ((bytes[10] == 0 && bytes[11] == 0) || + (bytes[10] == 0xff && bytes[11] == 0xff))) { + return !_isNonPublicIpv4(bytes.sublist(12)); + } + + // Accept only globally reachable IPv6 literals. Global unicast space is + // 2000::/3, with special-purpose exclusions and narrow globally reachable + // exceptions tracked by IANA (reconciled 2026-07-26): + // https://www.iana.org/assignments/iana-ipv6-special-registry/ + if (!_hasPrefix(bytes, [0x20], 3)) return false; + + // 2001::/23 is reserved for IETF protocol assignments. Only these entries + // are currently classified by IANA as globally reachable. + if (_hasPrefix(bytes, [0x20, 0x01, 0x00], 23) && + !_isGloballyReachableIetfAssignment(bytes)) { + return false; + } + + // Documentation and transition prefixes are not public relay destinations. + if (_hasPrefix(bytes, [0x20, 0x01, 0x0d, 0xb8], 32)) return false; + if (_hasPrefix(bytes, [0x20, 0x02], 16)) return false; + if (_hasPrefix(bytes, [0x3f, 0xff, 0x00], 20)) return false; + return true; +} + +bool _isGloballyReachableIetfAssignment(List bytes) { + final isGlobalAnycast = + _hasPrefix(bytes, [0x20, 0x01, 0x00, 0x01, 0, 0, 0, 0], 64) && + bytes.sublist(8, 15).every((byte) => byte == 0) && + bytes[15] >= 1 && + bytes[15] <= 3; + return isGlobalAnycast || + _hasPrefix(bytes, [0x20, 0x01, 0x00, 0x03], 32) || + _hasPrefix(bytes, [0x20, 0x01, 0x00, 0x04, 0x01, 0x12], 48) || + _hasPrefix(bytes, [0x20, 0x01, 0x00, 0x20], 28) || + _hasPrefix(bytes, [0x20, 0x01, 0x00, 0x30], 28); +} + +bool _hasPrefix(List address, List prefix, int prefixLength) { + final wholeBytes = prefixLength ~/ 8; + for (var i = 0; i < wholeBytes; i++) { + if (address[i] != prefix[i]) return false; + } + final remainingBits = prefixLength % 8; + if (remainingBits == 0) return true; + final mask = (0xff << (8 - remainingBits)) & 0xff; + return (address[wholeBytes] & mask) == (prefix[wholeBytes] & mask); +} + +bool _isNonPublicIpv4(List bytes) { + if (bytes.length != 4) return true; + final a = bytes[0]; + final b = bytes[1]; + final c = bytes[2]; + + return a == 0 || + a == 10 || + a == 127 || + (a == 100 && b >= 64 && b <= 127) || + (a == 169 && b == 254) || + (a == 172 && b >= 16 && b <= 31) || + (a == 192 && b == 0 && c == 0) || + (a == 192 && b == 0 && c == 2) || + (a == 192 && b == 168) || + (a == 198 && (b == 18 || b == 19)) || + (a == 198 && b == 51 && c == 100) || + (a == 203 && b == 0 && c == 113) || + a >= 224; +} diff --git a/mobile/test/features/invites/invite_join_provider_test.dart b/mobile/test/features/invites/invite_join_provider_test.dart index 29158f7910..e02134bb5f 100644 --- a/mobile/test/features/invites/invite_join_provider_test.dart +++ b/mobile/test/features/invites/invite_join_provider_test.dart @@ -5,6 +5,7 @@ import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:http/http.dart' as http; import 'package:http/testing.dart' as http_testing; import 'package:nostr/nostr.dart' as nostr; +import 'package:pointycastle/digests/sha256.dart'; import 'package:buzz/features/invites/invite_join_provider.dart'; import 'package:buzz/shared/auth/auth.dart'; @@ -128,7 +129,26 @@ void main() { 'https://relay.example.com/api/invites/claim', ); expect(capturedRequest!.body, jsonEncode({'code': 'code'})); - expect(capturedRequest!.headers['Authorization'], startsWith('Nostr ')); + final authHeader = capturedRequest!.headers['Authorization']; + expect(authHeader, startsWith('Nostr ')); + expect(capturedRequest!.followRedirects, isFalse); + final encoded = authHeader!.substring('Nostr '.length); + final authEvent = + jsonDecode( + utf8.decode(base64Url.decode(base64Url.normalize(encoded))), + ) + as Map; + final tags = (authEvent['tags'] as List) + .map((tag) => (tag as List).cast()) + .toList(); + final payloadHash = SHA256Digest() + .process(capturedRequest!.bodyBytes) + .map((byte) => byte.toRadixString(16).padLeft(2, '0')) + .join(); + expect(authEvent['kind'], 27235); + expect(tags, contains(equals(['u', capturedRequest!.url.toString()]))); + expect(tags, contains(equals(['method', 'POST']))); + expect(tags, contains(equals(['payload', payloadHash]))); expect(auth.authenticatedCommunities, hasLength(1)); expect( auth.authenticatedCommunities.single.relayUrl, @@ -139,6 +159,21 @@ void main() { }, ); + test('prepare rejects an unsafe relay before showing confirmation', () async { + final container = ProviderContainer(); + addTearDown(container.dispose); + + await expectLater( + container + .read(inviteJoinProvider.notifier) + .prepare( + const InviteDeepLink(relayUrl: 'wss://127.0.0.1', code: 'code'), + ), + throwsFormatException, + ); + expect(container.read(inviteJoinProvider).status, InviteJoinStatus.idle); + }); + test('join_policy_required requires a fresh link and cannot retry', () async { final keys = nostr.Keys.generate(); var attempts = 0; diff --git a/mobile/test/shared/deeplink/deep_link_test.dart b/mobile/test/shared/deeplink/deep_link_test.dart index a60056c3bc..64e15f0513 100644 --- a/mobile/test/shared/deeplink/deep_link_test.dart +++ b/mobile/test/shared/deeplink/deep_link_test.dart @@ -104,6 +104,23 @@ void _inviteTests() { ); }); + test('normalizes trailing slash in buzz join handoff', () { + final link = parseInviteDeepLink( + Uri.parse( + 'buzz://join?relay=wss%3A%2F%2Frelay.example.com%2F&code=abc123', + ), + ); + expect(link?.relayUrl, 'wss://relay.example.com'); + }); + + test('rejects plaintext public buzz join handoff', () { + final relay = Uri.encodeQueryComponent('ws://relay.example.com'); + expect( + parseInviteDeepLink(Uri.parse('buzz://join?relay=$relay&code=abc')), + isNull, + ); + }); + test('preserves policy receipt in buzz join handoff', () { final link = parseInviteDeepLink( Uri.parse( @@ -177,6 +194,18 @@ void _inviteTests() { ); }); + test('rejects non-public invite relay destinations', () { + for (final url in [ + 'https://127.0.0.1/invite/abc', + 'https://169.254.169.254/invite/abc', + 'https://192.168.1.1/invite/abc', + 'https://[::1]/invite/abc', + 'https://[::ffff:127.0.0.1]/invite/abc', + ]) { + expect(parseInviteDeepLink(Uri.parse(url)), isNull, reason: url); + } + }); + test('rejects buzz join with dangerous relay schemes', () { // The `relay=` param is an allowlist — only `ws` / `wss` are safe to // hand to a Nostr relay session. Anything else must be dropped by the diff --git a/mobile/test/shared/relay/relay_validation_test.dart b/mobile/test/shared/relay/relay_validation_test.dart new file mode 100644 index 0000000000..e564767ea4 --- /dev/null +++ b/mobile/test/shared/relay/relay_validation_test.dart @@ -0,0 +1,161 @@ +import 'package:flutter_test/flutter_test.dart'; + +import 'package:buzz/shared/relay/relay_validation.dart'; + +void main() { + group('validateInviteRelayUri', () { + test('accepts public secure relay origins', () { + for (final url in [ + 'wss://relay.example.com', + 'wss://relay.example.com/', + 'wss://relay.example.com:8443', + 'wss://8.8.8.8', + 'wss://[2001:4860:4860::8888]', + 'wss://[2001:1::1]', + 'wss://[2001:1::2]', + 'wss://[2001:1::3]', + 'wss://[2001:3::1]', + 'wss://[2001:4:112::1]', + 'wss://[2001:20::1]', + 'wss://[2001:30::1]', + 'wss://[2001:200::1]', + 'wss://[2620:4f:8000::1]', + 'wss://[3fff:1000::1]', + 'wss://[2606:4700:4700::1111]', + ]) { + expect( + () => validateInviteRelayUri( + Uri.parse(url), + allowInsecureLocalhost: false, + ), + returnsNormally, + reason: url, + ); + } + }); + + test('allows plaintext localhost only when explicitly enabled', () { + for (final url in ['ws://localhost:3000', 'ws://relay.localhost:3000']) { + expect( + () => validateInviteRelayUri( + Uri.parse(url), + allowInsecureLocalhost: true, + ), + returnsNormally, + reason: url, + ); + expect( + () => validateInviteRelayUri( + Uri.parse(url), + allowInsecureLocalhost: false, + ), + throwsFormatException, + reason: url, + ); + } + }); + + test('rejects plaintext public relays', () { + expect( + () => validateInviteRelayUri( + Uri.parse('ws://relay.example.com'), + allowInsecureLocalhost: false, + ), + throwsFormatException, + ); + }); + + test('rejects non-public IPv4 literals', () { + for (final host in [ + '0.0.0.0', + '10.0.0.1', + '100.64.0.1', + '127.0.0.1', + '169.254.169.254', + '172.16.0.1', + '192.168.0.1', + '198.18.0.1', + '224.0.0.1', + ]) { + expect( + () => validateInviteRelayUri( + Uri.parse('wss://$host'), + allowInsecureLocalhost: false, + ), + throwsFormatException, + reason: host, + ); + } + }); + + test('rejects non-public IPv6 literals and mapped IPv4', () { + for (final host in [ + '[::]', + '[::1]', + '[::ffff:127.0.0.1]', + '[::ffff:169.254.169.254]', + '[::ffff:a9fe:a9fe]', + '[64:ff9b::a9fe:a9fe]', + '[100::1]', + '[100:0:0:1::1]', + '[2001::1]', + '[2001:1::4]', + '[2001:2::1]', + '[2001:4:111::1]', + '[2001:10::1]', + '[2001:40::1]', + '[2001:db8::1]', + '[2002:a9fe:a9fe::1]', + '[3fff::1]', + '[3fff:fff::1]', + '[5f00::1]', + '[fc00::1]', + '[fd00::1]', + '[fe80::1]', + '[fec0::1]', + '[ff02::1]', + ]) { + expect( + () => validateInviteRelayUri( + Uri.parse('wss://$host'), + allowInsecureLocalhost: false, + ), + throwsFormatException, + reason: host, + ); + } + }); + + test('rejects legacy numeric IPv4 spellings', () { + for (final host in ['2130706433', '0x7f000001', '0177.0.0.1', '127.1']) { + expect( + () => validateInviteRelayUri( + Uri.parse('wss://$host'), + allowInsecureLocalhost: false, + ), + throwsFormatException, + reason: host, + ); + } + }); + + test('rejects non-origin URLs', () { + for (final url in [ + 'https://relay.example.com', + 'wss://user@relay.example.com', + 'wss://relay.example.com/path', + 'wss://relay.example.com?query=x', + 'wss://relay.example.com#fragment', + ]) { + expect( + () => validateInviteRelayUri( + Uri.parse(url), + allowInsecureLocalhost: false, + ), + throwsFormatException, + reason: url, + ); + } + }); + }); +} From 166c6655e8bca87d83ad60c087fb70a32a026baf Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 26 Jul 2026 09:28:30 -0700 Subject: [PATCH 2/4] fix(desktop): surface install failures hidden by curl-pipe exit codes (#2892) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ubuntu Doctor reports `Install failed at verify: The installer finished, but Buzz still could not use claude-code (observed: CLI missing)` while the `cli` step shows success. The `cli` step is lying. ## The masking Every CLI install command is a pipe — `curl -fsSL https://claude.ai/install.sh | bash` (`managed_agents/discovery.rs:109`), `… | sh` for Codex (`:141`), `… | CONFIGURE=false bash` for Goose (`:75`). `install_shell_command` ran them through `bash -l -c` with no `pipefail`, so the pipeline's exit status was the **right-hand** side's. A `curl` that fails — or that isn't on the child's PATH at all — feeds `bash` an empty stdin, and `bash` with nothing to run exits 0: ``` $ /bin/bash -l -c 'curl -fsSL https://nonexistent.invalid/x.sh | bash'; echo $? curl: (6) Could not resolve host: nonexistent.invalid 0 $ PATH=/tmp/empty /bin/bash -l -c 'curl -fsSL https://claude.ai/install.sh | bash'; echo $? bash: line 1: curl: command not found 0 ``` `run_install_command` records exit 0 as `success: true`, the adapter step then installs fine (it uses Buzz's own bundled Node, no system PATH needed), and `post_install_verification` correctly reports the CLI is absent. The user is handed a `verify` riddle instead of curl's error, which is why diagnosing this required three rounds of guessing. Install commands now run under `set -o pipefail`, so the left-hand side's failure is the step's failure and `InstallStepResult.stderr` carries the vendor's own message. `SHELLOPTS` is not exported by either shell, so the piped-to vendor script still runs with its default options. The Windows PowerShell install path (`install_powershell_command`) bypasses this shell and is untouched. ## The PATH collapse it was hiding `install_shell_command` composes the child's PATH and calls `cmd.env("PATH", …)`, which **replaces** rather than extends. `should_use_inherited` was `is_windows && !had_shell_path && has_local_context`, so on Unix the inherited process PATH was never appended. When `login_shell_path()` returns `None` — a login shell that exits non-zero or prints nothing, which a GUI-launched process can easily hit via `~/.profile` — the child's entire PATH becomes Buzz's two managed Node dirs. There is no `curl`, `sh`, `sha256sum`, or `tar` in either, so every curl-pipe install fails, and before this PR it failed invisibly. The `is_windows` requirement is dropped: the inherited PATH is the floor whenever no login-shell PATH was obtained, on every OS. Both existing suppressions are kept — a login-shell PATH present still suppresses it (no doubling), and no home/exe context still suppresses it (never manufacture a PATH from ambient state alone). Inherited entries stay **last**, so managed dirs keep precedence. The other caller, `build_augmented_path` (`runtime/path.rs:148`, feeding agent spawns and CLI probes), reads correctly under the new rule for the same reason: it only gains the inherited PATH in the case where it would otherwise hand a child a PATH with no native entries. When a login-shell PATH exists — the normal case on macOS and Linux — its output is unchanged, which `unix_shell_path_suppresses_inherited_fallback` pins. ## Scope This fixes the reporting defect and the PATH floor. The specific environment failure on the affected Ubuntu box is still being diagnosed and is deliberately not addressed here; the point of this change is that the next attempt produces the real error instead of a `verify` riddle. One interaction worth noting: `install_failure_is_retryable` retries any failure that carries an exit code, so a pipefail-surfaced curl failure now gets 3 attempts with backoff — correct for transient network blips, and harmless for hard failures. `desktop/scripts/check-file-sizes.mjs` ratchets the `agent_discovery.rs` ceiling 1836 → 1895 for the added tests. --------- Signed-off-by: Will Pfleger Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 --- desktop/scripts/check-file-sizes.mjs | 10 +- .../src-tauri/src/commands/agent_discovery.rs | 206 +++++++++++++++++- .../src/managed_agents/runtime/path.rs | 176 +++++++++------ 3 files changed, 319 insertions(+), 73 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 65928a29db..e593a8aa4e 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -492,7 +492,15 @@ const overrides = new Map([ // fix for the Goose Windows installer (PR #2680 interaction with #2750). // +10: pass an explicit PATH through Codex adapter install planning so unit // tests avoid the process-global login-shell PATH cache. - ["src-tauri/src/commands/agent_discovery.rs", 1836], + // +59: run install commands under `pipefail` so a failing `curl` in a + // `curl … | bash` install fails the `cli` step instead of being masked by + // `bash`'s exit 0, plus tests for the arg shape and the real pipeline status. + // +81: install_shell_args re-exports the composed PATH inside the command + // body so login startup files can't clear or reorder it, plus an isolated + // hostile-profile regression the pure composition tests structurally miss. + // +42: gate that re-export off Windows, where join_paths is `;`-separated and + // bash would collapse it into one entry, plus a platform-shape test. + ["src-tauri/src/commands/agent_discovery.rs", 2022], // draft-persistence predicate: submit-time `loadDraft` check + inline comment // + deps-array entry in submitMessage closes the never-persisted-boundary // defect (Thufir Pass-3 finding). Load-bearing correctness fix; queued to diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index e3d9b22189..d97e26001d 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -547,6 +547,62 @@ fn persist_last_error_on_install( save_managed_agents(app, &records) } +/// Build the `-l -c` argument list for the install shell. +/// +/// The body runs under `pipefail`: every CLI install command is a `curl … | +/// bash` / `| sh` pipe, and without it the pipeline's status is the right-hand +/// side's — `bash`/`sh` fed an empty stdin exits 0 — so a `curl` that fails (or +/// isn't on PATH at all) was recorded as a successful `cli` step, leaving the +/// user an unactionable `verify` error instead of curl's own stderr. Every +/// install shell supports it; the Windows PowerShell path bypasses this shell. +/// `SHELLOPTS` is not exported, so the piped-to vendor script keeps its own +/// defaults. +/// +/// Off Windows, `composed_path` is passed as a positional and re-exported +/// *inside* the body, because `-l` sources the user's login startup files after +/// the process environment is installed: a profile assigning PATH overwrites +/// `cmd.env("PATH", …)` before the vendor command runs. `export PATH=` empties +/// it outright; macOS `/etc/zprofile` runs `path_helper`, which reorders it and +/// costs Buzz's managed Node/npm dirs their precedence. A positional rather than +/// an interpolated body keeps entries containing spaces or quotes intact. +/// +/// The prelude is omitted where it would do harm: +/// - `composed_path` is `None` — `export PATH="$1"` with `$1` unset sets an +/// *empty* PATH, worse than the ambient one. +/// - `is_windows` — `join_paths` uses the platform separator, so the positional +/// would be `;`-joined while bash splits PATH on `:`, collapsing every entry +/// into one nonsense path; and Windows is where the inherited fallback always +/// fires (`login_shell_path()` is unconditionally `None` there), so this would +/// be the steady state. `cmd.env("PATH", …)` already delivers the native form +/// Git Bash translates on entry, and Windows has no login startup files doing +/// the clobbering this prelude defends against. +/// +/// `is_windows` is a parameter rather than a `#[cfg]` so the Windows shape stays +/// asserted on Unix CI — the same reason `should_skip_claude_executable` takes +/// one. Extracted from `install_shell_command` for that testability, not because +/// it has more than one caller. +fn install_shell_args( + command: &str, + composed_path: Option<&std::ffi::OsStr>, + is_windows: bool, +) -> Vec { + let Some(path) = composed_path.filter(|_| !is_windows) else { + return vec![ + "-l".into(), + "-c".into(), + format!("set -o pipefail; {command}").into(), + ]; + }; + vec![ + "-l".into(), + "-c".into(), + format!("export PATH=\"$1\"; set -o pipefail; {command}").into(), + // `$0` is the shell-name slot, so the PATH must be the second positional. + "buzz-install".into(), + path.to_os_string(), + ] +} + /// Build a login-shell `Command` for `command` with hermit env vars stripped, /// Buzz-managed npm locations set, and the user's PATH set. This is the /// single source of truth for @@ -561,7 +617,6 @@ fn install_shell_command(command: &str) -> Result let shell: std::path::PathBuf = resolve_install_shell()?; let mut cmd = std::process::Command::new(&shell); - cmd.args(["-l", "-c", command]); // Strip hermit vars and set managed npm paths (see apply_npm_env). apply_npm_env(&mut cmd); @@ -569,10 +624,18 @@ fn install_shell_command(command: &str) -> Result // Compose the PATH for the install shell using the same kernel as the // runtime/probe path so the two can never drift. managed entries first // (Node/npm bins keep precedence); login-shell entries next; inherited - // process PATH appended last on Windows when no login-shell PATH exists - // (login_shell_path() always returns None on Windows — Git Bash paths are - // POSIX-shaped and poison native children; cmd.env("PATH", …) replaces - // rather than extends, so without inherited the install shell loses npm). + // process PATH appended last when no login-shell PATH exists — the case + // where the composed PATH would otherwise be Buzz's managed Node dirs + // alone, with no `curl`/`sh`/`tar` for the vendor install pipes + // (cmd.env("PATH", …) replaces rather than extends). On Windows that case + // is the steady state: login_shell_path() always returns None there + // because Git Bash paths are POSIX-shaped and poison native children. + // + // The composed PATH is set twice on purpose off Windows: `cmd.env` so the + // login startup files themselves run with a usable PATH, and the `$1` + // export in `install_shell_args` so their own PATH assignments cannot undo + // it. Neither is redundant — see `install_shell_args`, which also explains + // why the export is suppressed on Windows. let login_path = crate::managed_agents::login_shell_path(); let had_login = login_path.is_some(); let managed: Vec = [ @@ -589,14 +652,20 @@ fn install_shell_command(command: &str) -> Result let inherited: Vec = std::env::var_os("PATH") .map(|p| std::env::split_paths(&p).collect()) .unwrap_or_default(); - let use_inherited = crate::managed_agents::should_use_inherited(had_login, true, cfg!(windows)); + let use_inherited = crate::managed_agents::should_use_inherited(had_login, true); let path_parts = crate::managed_agents::compose_path_entries(managed, login, inherited, use_inherited); - if !path_parts.is_empty() { - if let Ok(path) = std::env::join_paths(path_parts) { - cmd.env("PATH", path); - } + let composed_path = (!path_parts.is_empty()) + .then(|| std::env::join_paths(path_parts).ok()) + .flatten(); + if let Some(path) = composed_path.as_deref() { + cmd.env("PATH", path); } + cmd.args(install_shell_args( + command, + composed_path.as_deref(), + cfg!(windows), + )); // Detach from the controlling terminal so install scripts that read from // /dev/tty (e.g. Codex's "Start Codex now? [y/N]") fall back to stdin @@ -1408,6 +1477,123 @@ mod tests { assert!(result.is_ok(), "install_shell_command must succeed on Unix"); } + // ── pipefail: install pipes must not mask a failing left-hand side ──────── + + /// The command handed to the install shell must run under `set -o pipefail;` + /// with the vendor command preserved verbatim, so `curl … | bash` fails when + /// `curl` does. Platform-agnostic: only the PATH prelude differs by OS, and + /// `test_install_shell_args_shape_per_platform` pins that. + #[test] + fn test_install_shell_command_enables_pipefail() { + let cmd = super::install_shell_command("curl -fsSL https://example.test/i.sh | bash") + .expect("install shell must resolve on a test host"); + let args: Vec = cmd + .get_args() + .map(|a| a.to_string_lossy().into_owned()) + .collect(); + let body = &args[2]; + assert!( + body.contains("set -o pipefail; "), + "the install body must set pipefail; got: {body}" + ); + assert!( + body.ends_with("curl -fsSL https://example.test/i.sh | bash"), + "the vendor command must be preserved verbatim; got: {body}" + ); + } + + /// The PATH prelude is emitted only where it helps, and the exact argument + /// vector is the contract: a stray trailing positional with no `$1` reader, + /// or an export whose `$1` the shell cannot split, both corrupt PATH. + /// Windows is excluded because `join_paths` is `;`-separated there while bash + /// splits PATH on `:` — and it is the platform where the inherited fallback + /// always fires. See `install_shell_args` for the full reasoning. + #[test] + fn test_install_shell_args_shape_per_platform() { + let composed = std::ffi::OsString::from("/buzz/node/bin:/usr/bin"); + let windows_composed = std::ffi::OsString::from(r"C:\buzz\node;C:\Windows\system32"); + let bare = ["-l", "-c", "set -o pipefail; echo hi"].map(std::ffi::OsString::from); + + assert_eq!( + super::install_shell_args("echo hi", Some(&composed), false), + [ + "-l", + "-c", + "export PATH=\"$1\"; set -o pipefail; echo hi", + "buzz-install", + "/buzz/node/bin:/usr/bin", + ] + .map(std::ffi::OsString::from), + "Unix must re-export the composed PATH after login init" + ); + assert_eq!( + super::install_shell_args("echo hi", Some(&windows_composed), true), + bare, + "Windows must not re-export a `;`-joined PATH inside bash" + ); + assert_eq!( + super::install_shell_args("echo hi", None, false), + bare, + "no composed PATH must yield the bare pipefail body and no positionals" + ); + } + + /// Regression for the login-startup-file overwrite: `cmd.env("PATH", …)` is + /// installed *before* `-l` sources the user's profile, so a profile that + /// assigns PATH silently discards the composed one. Uses `/bin/bash` + /// explicitly — the planted profile is bash-specific, so resolving the host + /// shell (which prefers zsh) would make this vacuous. + #[cfg(unix)] + #[test] + fn test_composed_path_survives_a_profile_that_clears_it() { + let home = tempfile::tempdir().expect("temp HOME"); + std::fs::write(home.path().join(".bash_profile"), "export PATH=\n") + .expect("plant a hostile login profile"); + let composed = std::ffi::OsString::from("/buzz/sentinel/bin:/usr/bin:/bin"); + + // `echo` is a shell builtin, so the child needs no PATH to report one. + let out = std::process::Command::new("/bin/bash") + .args(super::install_shell_args( + "echo \"$PATH\"", + Some(&composed), + false, + )) + .env("HOME", home.path()) + .env("PATH", &composed) + .stdin(std::process::Stdio::null()) + .output() + .expect("bash must spawn"); + + let path = String::from_utf8_lossy(&out.stdout); + assert!( + path.contains("/buzz/sentinel/bin"), + "the composed PATH must survive login init; got: {path:?}" + ); + } + + /// End-to-end on the real resolved install shell (no network): a pipeline + /// whose left-hand side fails must exit non-zero, while a fully successful + /// pipeline must still succeed. Without `pipefail` the status is the + /// right-hand side's and the left-hand failure is invisible. + #[cfg(unix)] + #[test] + fn test_install_shell_pipeline_status_follows_left_side() { + for (command, expect_success) in [("false | true", false), ("echo ok | cat", true)] { + let status = super::install_shell_command(command) + .expect("Unix must always resolve an install shell") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .expect("install shell must spawn"); + assert_eq!( + status.success(), + expect_success, + "`{command}` must report success={expect_success}; got {status:?}" + ); + } + } + // ── Phase A: Windows install shell selection ─────────────────────────────── /// On Windows (CI runner has Git pre-installed), resolve_install_shell succeeds. diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index cf6950f577..efec0c903e 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -30,30 +30,25 @@ pub(crate) fn should_skip_claude_executable(path: &std::path::Path, is_windows: /// Decide whether the inherited process PATH should be appended to the /// composed PATH. /// -/// On Windows, `login_shell_path()` always returns `None` because Git Bash -/// returns POSIX colon-delimited paths that poison native children. -/// `Command::env("PATH", …)` replaces rather than extends, so without the -/// inherited PATH every child loses node/npm/git. -/// -/// This pure function takes an explicit `is_windows` flag so it can be -/// unit-tested cross-host (macOS CI can pass `true` to exercise the Windows -/// policy without needing the `cfg!(windows)` target). +/// `Command::env("PATH", …)` replaces rather than extends, so a child whose +/// composed PATH carries no native entries loses every system binary. On +/// Windows that is the steady state — `login_shell_path()` always returns +/// `None` because Git Bash returns POSIX colon-delimited paths that poison +/// native children. On Unix it is the failure mode: a login shell that exits +/// non-zero or prints nothing also yields `None`, and the child is then left +/// with only Buzz's managed Node dirs — no `curl`, `sh`, or `tar`, which +/// silently breaks every `curl … | bash` install. The inherited PATH is the +/// floor under both cases, appended last so managed dirs keep precedence. /// /// Rules: -/// - Only append when `is_windows` — on Unix the login-shell PATH always covers -/// the needed runtimes. /// - Suppress when `had_shell_path` is `true` — if a login-shell PATH was /// supplied it already carries the user's native entries; appending the /// process PATH would double them. /// - Suppress when `has_local_context` is `false` — callers that pass no home /// or exe-parent context must not receive a PATH manufactured from ambient /// process state alone. -pub(crate) fn should_use_inherited( - had_shell_path: bool, - has_local_context: bool, - is_windows: bool, -) -> bool { - is_windows && !had_shell_path && has_local_context +pub(crate) fn should_use_inherited(had_shell_path: bool, has_local_context: bool) -> bool { + !had_shell_path && has_local_context } /// Pure PATH composition kernel shared by the install shell and the runtime/probe paths. @@ -91,10 +86,14 @@ pub(crate) fn compose_path_entries( /// 4. `nvm_bin` — nvm's default Node.js bin dir (if the user uses nvm) /// 5. exe parent dir — DMG sidecars under `Contents/MacOS/` /// 6. user's login-shell `PATH` — runtimes like node/python from other managers -/// 7. Windows only: the current process `PATH` (appended when no login-shell -/// PATH exists, because callers use `Command::env("PATH", …)` which -/// *replaces* the child's PATH — without this, the child loses node/npm/git -/// and every npm `.cmd` shim fails with `'node' is not recognized`) +/// 7. the current process `PATH` — appended on every platform when no +/// login-shell PATH exists, because callers use `Command::env("PATH", …)` +/// which *replaces* the child's PATH. This is the steady state on Windows, +/// where `login_shell_path()` always returns `None` and without it the +/// child loses node/npm/git and every npm `.cmd` shim fails with +/// `'node' is not recognized`; on Unix it is the login-shell-probe failure +/// fallback, which keeps `curl`/`sh`/`tar` reachable. See +/// [`should_use_inherited`] for the suppression rules. /// /// `shell_path` is the raw colon-delimited string from a login shell, so it is /// split into individual entries before joining. Pushing it as a single segment @@ -145,7 +144,7 @@ pub(in crate::managed_agents) fn build_augmented_path( let inherited: Vec = std::env::var_os("PATH") .map(|p| std::env::split_paths(&p).collect()) .unwrap_or_default(); - let use_inherited = should_use_inherited(had_shell_path, has_local_context, cfg!(windows)); + let use_inherited = should_use_inherited(had_shell_path, has_local_context); let parts = compose_path_entries(managed, login, inherited, use_inherited); if parts.is_empty() { @@ -223,29 +222,86 @@ mod tests { #[cfg(unix)] #[test] fn nvm_bin_none_does_not_add_segment() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + // With no shell_path the inherited process PATH is appended last, so + // pin it to a sentinel to keep the assertion deterministic. + std::env::set_var("PATH", "/sentinel/inherited"); + let result = build_augmented_path( Some(PathBuf::from("/home/user")), Some(PathBuf::from("/usr/local/bin")), None, None, ); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + let result = result.expect("path"); assert!(result.starts_with("/home/user/.local/bin:"), "{result}"); - assert!(result.ends_with(":/usr/local/bin"), "{result}"); + assert!(!result.contains(".nvm"), "no nvm segment: {result}"); + assert!( + result.contains(":/usr/local/bin:"), + "exe parent must precede the inherited PATH: {result}" + ); + assert!( + result.ends_with(":/sentinel/inherited"), + "inherited PATH must be appended last when no shell_path: {result}" + ); } - /// On Unix, supplying a `shell_path` must NOT trigger the Windows process-PATH - /// fallback — the output must be byte-identical to what it was before this - /// fix. + /// On Unix with no login-shell PATH, `build_augmented_path` must fall back to + /// the inherited process PATH — otherwise the child gets only Buzz-managed + /// dirs and loses every system binary (`curl`, `sh`, `tar`). #[cfg(unix)] #[test] - fn unix_shell_path_output_unchanged_by_windows_fallback_logic() { + fn unix_appends_process_path_when_no_shell_path() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + std::env::set_var("PATH", "/usr/bin:/bin"); + + let result = build_augmented_path(Some(PathBuf::from("/home/user")), None, None, None); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + + let result = result.expect("path must not be None with a home dir"); + assert!( + result.starts_with("/home/user/.local/bin:"), + "home/.local/bin must be first: {result}" + ); + assert!( + result.ends_with(":/usr/bin:/bin"), + "process PATH must be last: {result}" + ); + } + + /// On Unix, supplying a `shell_path` must NOT also append the inherited + /// process PATH — the login-shell PATH already carries the native entries. + #[cfg(unix)] + #[test] + fn unix_shell_path_suppresses_inherited_fallback() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + std::env::set_var("PATH", "/should/not/appear"); + let result = build_augmented_path( Some(PathBuf::from("/home/user")), None, Some("/usr/local/bin:/usr/bin:/bin".to_string()), None, ); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + let result = result.expect("path"); assert!( result.ends_with(":/usr/local/bin:/usr/bin:/bin"), @@ -334,8 +390,8 @@ mod tests { // ── Pure policy and composition tests — run on every host ──────────────────── // // These test `should_use_inherited` and `compose_path_entries` with explicit -// inputs, so they run on macOS/Linux CI and validate the Windows policy -// behavior without touching process state or requiring a Windows target. +// inputs, so they run on macOS/Linux CI and validate the cross-platform +// fallback policy without touching process state or requiring a Windows target. #[cfg(test)] mod compose_tests { use super::{compose_path_entries, is_batch_shim, should_use_inherited}; @@ -347,43 +403,37 @@ mod compose_tests { // ── should_use_inherited policy matrix ──────────────────────────────────── - /// Windows + no shell path + has local context → must use inherited. + /// No shell path + has local context → must use inherited, on every OS. + /// This is the steady state on Windows (login_shell_path() is always None) + /// and the failure mode on Unix (login shell exited non-zero or printed + /// nothing); in both the child would otherwise get no native PATH entries. #[test] - fn policy_windows_no_shell_with_context_uses_inherited() { + fn policy_no_shell_with_context_uses_inherited() { assert!( - should_use_inherited(false, true, true), - "Windows, no shell path, has context → must append inherited" + should_use_inherited(false, true), + "no shell path, has context → must append inherited" ); } - /// Windows + shell path present → must NOT use inherited (login path covers it). + /// Shell path present → must NOT use inherited (login path covers it). #[test] - fn policy_windows_shell_path_present_suppresses_inherited() { + fn policy_shell_path_present_suppresses_inherited() { assert!( - !should_use_inherited(true, true, true), - "Windows, shell path present → must not append inherited" + !should_use_inherited(true, true), + "shell path present → must not append inherited" ); } - /// Windows + no local context → must NOT use inherited (no ambient state). + /// No local context → must NOT use inherited (no ambient state). #[test] - fn policy_windows_no_local_context_suppresses_inherited() { + fn policy_no_local_context_suppresses_inherited() { assert!( - !should_use_inherited(false, false, true), - "Windows, no local context → must not append inherited" - ); - } - - /// Non-Windows → never use inherited, regardless of other flags. - #[test] - fn policy_non_windows_never_uses_inherited() { - assert!( - !should_use_inherited(false, true, false), - "non-Windows must never append inherited PATH" + !should_use_inherited(false, false), + "no local context → must not append inherited" ); assert!( - !should_use_inherited(false, false, false), - "non-Windows + no context must never append inherited PATH" + !should_use_inherited(true, false), + "no local context → must not append inherited even with a shell path" ); } @@ -498,23 +548,25 @@ mod compose_tests { // compute the same `should_use_inherited` decision for equivalent inputs. // Tests the policy function directly to confirm the wrappers can't drift. - /// Exhaustive truth-table for `should_use_inherited` — all four input - /// combinations that affect real callers. Confirms the policy is correct - /// before either wrapper binds to it. + /// Exhaustive truth-table for `should_use_inherited` — every input + /// combination. Confirms the policy is correct before either wrapper binds + /// to it. The rule is OS-independent: the inherited PATH is the floor + /// whenever no login-shell PATH was obtained, because the alternative is a + /// child with no native binaries at all. #[test] fn should_use_inherited_policy_truth_table() { - // (had_shell, has_context, is_windows) → expected + // (had_shell, has_context) → expected let cases = [ - (false, true, true, true), // Windows, no shell, context → USE - (true, true, true, false), // Windows, shell present → NO - (false, false, true, false), // Windows, no context → NO - (false, true, false, false), // non-Windows → NO + (false, true, true), // no shell PATH, context → USE (the floor) + (true, true, false), // shell PATH present → NO (already covered) + (false, false, false), // no context → NO (no ambient-only PATH) + (true, false, false), // no context → NO, shell PATH irrelevant ]; - for (had_shell, has_ctx, is_win, expected) in cases { - let result = should_use_inherited(had_shell, has_ctx, is_win); + for (had_shell, has_ctx, expected) in cases { + let result = should_use_inherited(had_shell, has_ctx); assert_eq!( result, expected, - "policy mismatch: had_shell={had_shell} has_ctx={has_ctx} is_win={is_win}" + "policy mismatch: had_shell={had_shell} has_ctx={has_ctx}" ); } } From 8e67cf399d0291bcdbc69cd0402983ca030f05bb Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 26 Jul 2026 09:30:13 -0700 Subject: [PATCH 3/4] chore(desktop): delete dead persona catalog UI cluster (#2886) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Deletes 6 orphaned files in `desktop/src/features/agents/ui/` (556 lines) that formed a closed cluster with zero imports from the reachable module graph — orphaned by the 1B dialog consolidation (`PersonaCatalogSurface`, `PersonaCatalogSection`, `PersonaCatalogDetailsSheet`, `PersonaCatalogSelectionBadge`, `PersonaIdentity`, `PersonaLibraryEntryPoints`). - Removes a stale `check-file-sizes.mjs` override entry for the no-longer-existent `PersonaDialog.tsx`. Verified dead two ways: import-graph reachability walk from all entry points puts all six outside the reachable set, and `tsc --noEmit` passes clean after deletion. Their `data-testid`s have zero references outside the cluster. `PersonaCatalogDialog.tsx` is alive (`AgentsView` imports it) and stays. Independent of #1968 — pure dead code removal, no behavioral change. --- desktop/scripts/check-file-sizes.mjs | 14 -- .../agents/ui/PersonaCatalogDetailsSheet.tsx | 163 ----------------- .../agents/ui/PersonaCatalogSection.tsx | 166 ------------------ .../ui/PersonaCatalogSelectionBadge.tsx | 29 --- .../agents/ui/PersonaCatalogSurface.tsx | 79 --------- .../features/agents/ui/PersonaIdentity.tsx | 62 ------- .../agents/ui/PersonaLibraryEntryPoints.tsx | 57 ------ 7 files changed, 570 deletions(-) delete mode 100644 desktop/src/features/agents/ui/PersonaCatalogDetailsSheet.tsx delete mode 100644 desktop/src/features/agents/ui/PersonaCatalogSection.tsx delete mode 100644 desktop/src/features/agents/ui/PersonaCatalogSelectionBadge.tsx delete mode 100644 desktop/src/features/agents/ui/PersonaCatalogSurface.tsx delete mode 100644 desktop/src/features/agents/ui/PersonaIdentity.tsx delete mode 100644 desktop/src/features/agents/ui/PersonaLibraryEntryPoints.tsx diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index e593a8aa4e..b74e05ae7b 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -219,20 +219,6 @@ const overrides = new Map([ // (+3) — the new team-id spawn link and the runtime-layered instructions // field. ["src/shared/api/types.ts", 1047], - // readiness-gate: PersonaDialog.tsx threads computeLocalModeGate + - // requiredCredentialEnvKeys + RequiredFieldLabel so the "New agent" dialog - // shows required markers and credential amber rows (parity with - // CreateAgentDialog). +23 lines of gate wiring. Queued to split. - // config-bridge-aware requirements: useRuntimeFileConfigQuery wiring adds - // ~16 lines. Queued to split. - // baked-env-required-badge: useBakedBuildEnvKeysQuery + bakedEnvKeys wiring - // + correct exclusion-semantics for requiredEnvKeys adds ~14 lines. - // +2 lines: filter managed provider key from requiredEnvKeys (suppress dead-input locked row). - // global-agent-config parity: wire useGlobalAgentConfig into PersonaDialog - // (Gap A: global-aware computeLocalModeGate + drop bare requiredCredentialEnvKeys; - // Gap B: hasAutoOpenedAdvancedRef auto-expand effect) + effective-provider - // save gate + Inherit/Select-a-provider label. Queued to split. - ["src/features/agents/ui/PersonaDialog.tsx", 1080], // harness-persona-sync feature growth, queued to split in the resolver-unify // refactor followup. discovery.rs is dominated by the new test module // (the effective_agent_command / divergent / create-time override matrix); diff --git a/desktop/src/features/agents/ui/PersonaCatalogDetailsSheet.tsx b/desktop/src/features/agents/ui/PersonaCatalogDetailsSheet.tsx deleted file mode 100644 index 6aab3096cb..0000000000 --- a/desktop/src/features/agents/ui/PersonaCatalogDetailsSheet.tsx +++ /dev/null @@ -1,163 +0,0 @@ -import { isCatalogPersonaSelected } from "@/features/agents/lib/catalog"; -import { ProfileAvatar } from "@/features/profile/ui/ProfileAvatar"; -import type { AgentPersona } from "@/shared/api/types"; -import { cn } from "@/shared/lib/cn"; -import { promptPreview } from "@/shared/lib/promptPreview"; -import { - Sheet, - SheetContent, - SheetDescription, - SheetHeader, - SheetTitle, -} from "@/shared/ui/sheet"; - -import { PersonaCatalogSelectionBadge } from "./PersonaCatalogSelectionBadge"; -import { - getPersonaCatalogDetailSelectionCopy, - getPersonaCatalogSelectionAriaLabel, -} from "./personaLibraryCopy"; - -type PersonaCatalogDetailsSheetProps = { - feedbackErrorMessage: string | null; - feedbackNoticeMessage: string | null; - isPending: boolean; - onOpenChange: (open: boolean) => void; - onTogglePersona: (persona: AgentPersona) => void; - open: boolean; - persona: AgentPersona | null; -}; - -export function PersonaCatalogDetailsSheet({ - feedbackErrorMessage, - feedbackNoticeMessage, - isPending, - onOpenChange, - onTogglePersona, - open, - persona, -}: PersonaCatalogDetailsSheetProps) { - const preview = persona ? promptPreview(persona.systemPrompt) : ""; - const isSelected = persona ? isCatalogPersonaSelected(persona) : false; - const selectionCopy = getPersonaCatalogDetailSelectionCopy(isSelected); - - return ( - - - {persona ? ( - <> - -
- -
-
- - {persona.displayName} - - -
- - {preview || "No summary available."} - -
-
- -
- - - {feedbackNoticeMessage ? ( -

- {feedbackNoticeMessage} -

- ) : null} - - {feedbackErrorMessage ? ( -

- {feedbackErrorMessage} -

- ) : null} - -
-
-

- Type -

-

Built-in agent

-
-
-

- Preferred model -

-

- {persona.model ?? "Use app default"} -

-
-
-

- Preferred runtime -

-

- {persona.runtime ?? "Use app default"} -

-
-
- -
-

- System prompt -

- 
-                  {persona.systemPrompt}
-
-
-
- - ) : null} - - - ); -} diff --git a/desktop/src/features/agents/ui/PersonaCatalogSection.tsx b/desktop/src/features/agents/ui/PersonaCatalogSection.tsx deleted file mode 100644 index 25cc9e4751..0000000000 --- a/desktop/src/features/agents/ui/PersonaCatalogSection.tsx +++ /dev/null @@ -1,166 +0,0 @@ -import { isCatalogPersonaSelected } from "@/features/agents/lib/catalog"; -import type { AgentPersona } from "@/shared/api/types"; -import { promptPreview } from "@/shared/lib/promptPreview"; -import { cn } from "@/shared/lib/cn"; -import { Button } from "@/shared/ui/button"; -import { Card } from "@/shared/ui/card"; -import { Skeleton } from "@/shared/ui/skeleton"; -import { PersonaCatalogSelectionBadge } from "./PersonaCatalogSelectionBadge"; -import { PersonaIdentity } from "./PersonaIdentity"; -import { - getPersonaCatalogSelectionActionCopy, - getPersonaCatalogSelectionAriaLabel, - personaCatalogCopy, -} from "./personaLibraryCopy"; - -type PersonaCatalogSectionProps = { - emptyDescription?: string; - emptyTitle?: string; - error: Error | null; - isLoading: boolean; - isPending: boolean; - onTogglePersona: (persona: AgentPersona) => void; - onViewDetails: (persona: AgentPersona) => void; - personas: AgentPersona[]; - showHeader?: boolean; -}; - -export function PersonaCatalogSection({ - emptyDescription = personaCatalogCopy.emptyCatalogDescription, - emptyTitle = personaCatalogCopy.emptyCatalogTitle, - error, - isLoading, - isPending, - onTogglePersona, - onViewDetails, - personas, - showHeader = true, -}: PersonaCatalogSectionProps) { - return ( -
- {showHeader ? ( -
-

- {personaCatalogCopy.title} -

-

- {personaCatalogCopy.description} -

-
- ) : null} - - {isLoading ? ( -
- {["first", "second", "third", "fourth"].map((key) => ( - -
- -
- - -
-
- - ))} -
- ) : null} - - {!isLoading && personas.length > 0 ? ( -
- {personas.map((persona) => { - const preview = promptPreview(persona.systemPrompt); - const isSelected = isCatalogPersonaSelected(persona); - - return ( -
- - - - {getPersonaCatalogSelectionActionCopy(isSelected)} - -
-
- - ); - })} - - ) : null} - - {!isLoading && personas.length === 0 ? ( -
-

{emptyTitle}

-

- {emptyDescription} -

-
- ) : null} - - {error ? ( -

- {error.message} -

- ) : null} -
- ); -} diff --git a/desktop/src/features/agents/ui/PersonaCatalogSelectionBadge.tsx b/desktop/src/features/agents/ui/PersonaCatalogSelectionBadge.tsx deleted file mode 100644 index 75837d0f94..0000000000 --- a/desktop/src/features/agents/ui/PersonaCatalogSelectionBadge.tsx +++ /dev/null @@ -1,29 +0,0 @@ -import { Check } from "lucide-react"; - -import { cn } from "@/shared/lib/cn"; - -import { personaCatalogCopy } from "./personaLibraryCopy"; - -type PersonaCatalogSelectionBadgeProps = { - isActive: boolean; -}; - -export function PersonaCatalogSelectionBadge({ - isActive, -}: PersonaCatalogSelectionBadgeProps) { - return ( - - {isActive ? : null} - {isActive - ? personaCatalogCopy.selectedState - : personaCatalogCopy.availableState} - - ); -} diff --git a/desktop/src/features/agents/ui/PersonaCatalogSurface.tsx b/desktop/src/features/agents/ui/PersonaCatalogSurface.tsx deleted file mode 100644 index 159025f7b9..0000000000 --- a/desktop/src/features/agents/ui/PersonaCatalogSurface.tsx +++ /dev/null @@ -1,79 +0,0 @@ -import * as React from "react"; - -import { isCatalogPersonaSelected } from "@/features/agents/lib/catalog"; -import type { AgentPersona } from "@/shared/api/types"; -import { useFeedbackToasts } from "@/shared/hooks/useToastEffect"; - -import { PersonaCatalogDetailsSheet } from "./PersonaCatalogDetailsSheet"; -import { PersonaCatalogSection } from "./PersonaCatalogSection"; - -type PersonaCatalogSurfaceProps = { - error: Error | null; - feedbackErrorMessage: string | null; - feedbackNoticeMessage: string | null; - isLoading: boolean; - isPending: boolean; - onClearFeedback: () => void; - onSelectPersona: (persona: AgentPersona, active: boolean) => void; - personas: AgentPersona[]; - showHeader?: boolean; -}; - -export function PersonaCatalogSurface({ - error, - feedbackErrorMessage, - feedbackNoticeMessage, - isLoading, - isPending, - onClearFeedback, - onSelectPersona, - personas, - showHeader = true, -}: PersonaCatalogSurfaceProps) { - const [detailPersonaId, setDetailPersonaId] = React.useState( - null, - ); - const detailPersona = React.useMemo( - () => - detailPersonaId - ? (personas.find((persona) => persona.id === detailPersonaId) ?? null) - : null, - [detailPersonaId, personas], - ); - const handleTogglePersona = (persona: AgentPersona) => { - onSelectPersona(persona, !isCatalogPersonaSelected(persona)); - }; - - useFeedbackToasts(feedbackNoticeMessage, feedbackErrorMessage); - - return ( - <> - { - onClearFeedback(); - setDetailPersonaId(persona.id); - }} - personas={personas} - showHeader={showHeader} - /> - - { - if (!open) { - setDetailPersonaId(null); - } - }} - onTogglePersona={handleTogglePersona} - open={detailPersonaId !== null} - persona={detailPersona} - /> - - ); -} diff --git a/desktop/src/features/agents/ui/PersonaIdentity.tsx b/desktop/src/features/agents/ui/PersonaIdentity.tsx deleted file mode 100644 index eb2c5c0253..0000000000 --- a/desktop/src/features/agents/ui/PersonaIdentity.tsx +++ /dev/null @@ -1,62 +0,0 @@ -import { Info } from "lucide-react"; - -import { ProfileAvatar } from "@/features/profile/ui/ProfileAvatar"; -import type { AgentPersona } from "@/shared/api/types"; -import { cn } from "@/shared/lib/cn"; -import { promptPreview } from "@/shared/lib/promptPreview"; -import { Badge } from "@/shared/ui/badge"; -import { Tooltip, TooltipContent, TooltipTrigger } from "@/shared/ui/tooltip"; - -type PersonaIdentityProps = { - className?: string; - persona: AgentPersona; - showBuiltInBadge?: boolean; - showPromptTooltip?: boolean; -}; - -export function PersonaIdentity({ - className, - persona, - showBuiltInBadge = persona.isBuiltIn, - showPromptTooltip = true, -}: PersonaIdentityProps) { - const preview = promptPreview(persona.systemPrompt); - - return ( -
-
- -
-
-

- {persona.displayName} -

- {showBuiltInBadge ? ( - Built-in - ) : null} - {showPromptTooltip && preview ? ( - - - - - -

{preview}

- - - ) : null} -
-
-
-
- ); -} diff --git a/desktop/src/features/agents/ui/PersonaLibraryEntryPoints.tsx b/desktop/src/features/agents/ui/PersonaLibraryEntryPoints.tsx deleted file mode 100644 index 113d38b898..0000000000 --- a/desktop/src/features/agents/ui/PersonaLibraryEntryPoints.tsx +++ /dev/null @@ -1,57 +0,0 @@ -import { Upload } from "lucide-react"; - -import { Button } from "@/shared/ui/button"; - -import { CreateNewButton } from "./CreateNewButton"; -import { personaLibraryCopy } from "./personaLibraryCopy"; - -type PersonaLibraryEntryPointsProps = { - canChooseCatalog?: boolean; - isPending: boolean; - layout: "header" | "empty"; - onCreate: () => void; - onChooseCatalog?: () => void; - onImport?: () => void; -}; - -export function PersonaLibraryEntryPoints({ - canChooseCatalog = false, - isPending, - layout, - onCreate, - onChooseCatalog, - onImport, -}: PersonaLibraryEntryPointsProps) { - const isHeader = layout === "header"; - const chooseVariant = isHeader ? "outline" : "default"; - const createVariant = isHeader ? "default" : "outline"; - - return ( - <> - {canChooseCatalog && onChooseCatalog ? ( - - ) : null} - - {!isHeader && onImport ? ( - - ) : null} - - ); -} From 8c0e8cb1656b04ad269bce3c2deeda2a943ae78a Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 26 Jul 2026 09:30:33 -0700 Subject: [PATCH 4/4] fix(desktop): make agent definition authoritative for model/provider/prompt (#1968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Introduces a single effective-config resolver that makes agent definitions authoritative for model, provider, system prompt, and relay-mesh routing on linked instances. Stale materialized record bytes are never consulted at spawn, deploy, readiness, hash, card summary, or mesh preflight. **Resolution semantics (field-specific):** - **model/provider (linked):** definition → global. `None` = inherit global default. Record value never consulted. - **model/provider (definition-less):** instance → global. - **system_prompt (linked):** strictly from definition; blank = no prompt. No global tier for prompt. - **system_prompt (definition-less):** from the instance. - **relay-mesh preflight (both):** driven by the same `resolve_effective_config` resolution spawn's mesh env consults. For a linked instance the record's own `provider`/`model`/`relay_mesh` bytes are never consulted; for a definition-less instance with no provider of its own, the legacy marker and env preset are the last fallback (see below). - **Orphaned (linked record, definition missing):** spawn, deploy, and mesh preflight blocked with actionable user-facing error. **Changes:** - New `effective_config` resolver module with `ConfigSource` metadata (`definition`, `global`, `instance_legacy`) - All consumers routed through the single resolver: local spawn, deploy, readiness, spawn hash, card summary, relay-mesh preflight (interactive start and restore-on-launch) - `apply_persona_snapshot` no longer preserves stale record model/provider when definition is blank - Backend `update_managed_agent` blocks model/provider/prompt writes for linked records - Frontend omits model/provider/systemPrompt submission for linked instances; system prompt override hidden - `model_source` field added to `ManagedAgentSummary` so card labels distinguish inherited (`Default model (X)`) from explicit - Spawn hash now digests resolved model/provider (not raw record fields) so global default changes trip the restart badge even for runtimes without `model_env_var` - Orphan spawn/deploy/mesh-preflight blocked with jargon-free error ("This agent's configuration is missing — it may still be syncing or was deleted on another device") - `EffectiveAgentConfig::relay_mesh_model_id()` and `resolve_effective_relay_mesh_model_id()` added; both mesh preflights (`start_local_agent_with_preflight`, `restore_managed_agents_on_launch`) call this instead of the deleted `relay_mesh_config`/`relay_mesh_model_id` record-byte sniffs - Legacy relay-mesh records keep their mesh routing. Two shipped generations predate `provider: "relay-mesh"` and are never rewritten on load: the typed `relay_mesh` marker (added when `ManagedAgentRecord` had no `provider` field), and before it the mesh preset written directly into `env_vars`. `resolve_definition_less` falls back to the marker, then to the env preset, so these records still resolve to mesh instead of silently misrouting to an unrelated provider while their stale env bytes reach the child. The fallback is skipped when the record carries an explicit `provider` — that states current intent, including a switch away from mesh — and `resolve_linked` has no legacy fallback at all - The env discriminator accepts both spellings of the two sentinels renamed in the Jun-11 window without a record migration: the provider env key (`BUZZ_AGENT_PROVIDER`, previously `SPROUT_AGENT_PROVIDER`) and the api-key value (`buzz-mesh-local`, previously `sprout-mesh-local`). Each is independent, since a record can straddle the window; the current provider-key spelling wins when both are present - Dead `persona_field_with_record_fallback` and wrapper `persona_snapshot_with_agent_config_fallback` deleted; callers use `persona_snapshot` directly - New resolver/deploy/hash/write-guard/mesh-preflight tests, including switch-away and global-inheritance regressions for both mesh preflight call sites, per-class legacy-mesh resolution (typed marker and env preset, in every rename-window spelling combination), and the paired assertions that a linked instance's legacy mesh bytes stay inert --------- Signed-off-by: Will Pfleger Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 --- desktop/scripts/check-file-sizes.mjs | 38 +- .../src-tauri/src/commands/agent_config.rs | 98 +- .../src-tauri/src/commands/agent_models.rs | 73 +- .../src/commands/agent_models_tests.rs | 175 +++- desktop/src-tauri/src/commands/agents.rs | 47 +- .../src-tauri/src/commands/agents_deploy.rs | 79 +- .../src-tauri/src/commands/agents_tests.rs | 87 +- desktop/src-tauri/src/commands/mesh_llm.rs | 14 +- .../managed_agents/effective_config/mod.rs | 318 +++++++ .../managed_agents/effective_config/tests.rs | 872 ++++++++++++++++++ .../src-tauri/src/managed_agents/env_vars.rs | 8 +- .../src/managed_agents/global_config/mod.rs | 69 +- .../src/managed_agents/global_config/tests.rs | 56 +- desktop/src-tauri/src/managed_agents/mod.rs | 1 + .../src/managed_agents/persona_events.rs | 115 +-- .../managed_agents/persona_events/tests.rs | 302 +++--- .../src-tauri/src/managed_agents/readiness.rs | 15 +- .../src/managed_agents/relay_mesh.rs | 229 ----- .../src-tauri/src/managed_agents/restore.rs | 28 +- .../src-tauri/src/managed_agents/runtime.rs | 142 ++- .../src/managed_agents/runtime/metadata.rs | 28 +- .../src/managed_agents/runtime/tests.rs | 74 +- .../src/managed_agents/spawn_hash.rs | 42 +- .../src/managed_agents/spawn_hash/tests.rs | 129 ++- desktop/src-tauri/src/managed_agents/types.rs | 45 +- .../agents/lib/agentCardModelLabel.test.mjs | 58 ++ .../agents/lib/agentCardModelLabel.ts | 44 + .../agents/ui/AgentInstanceEditDialog.tsx | 36 +- .../agents/ui/EditAgentAdvancedFields.tsx | 47 +- .../features/agents/ui/ManagedAgentRow.tsx | 14 +- .../agents/ui/UnifiedAgentsSection.tsx | 47 +- .../features/agents/ui/personaRuntimeModel.ts | 43 +- desktop/src/shared/api/tauri.ts | 3 + desktop/src/shared/api/types.ts | 1 + 34 files changed, 2548 insertions(+), 829 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/effective_config/mod.rs create mode 100644 desktop/src-tauri/src/managed_agents/effective_config/tests.rs create mode 100644 desktop/src/features/agents/lib/agentCardModelLabel.test.mjs create mode 100644 desktop/src/features/agents/lib/agentCardModelLabel.ts diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index b74e05ae7b..9649e9bbfb 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -80,7 +80,15 @@ const overrides = new Map([ // ratcheting 1443 -> 1295. Queued to split further in the A2 fold. // global-agent-config: resolve_deploy_model_provider + visibility exports // add ~40 lines on top of the 1A.1 ratchet. Queued to split. - ["src-tauri/src/commands/agents.rs", 1340], + // +22 (1340 -> 1360): agent-config-resolver — start_local_agent_with_preflight + // uses resolve_effective_relay_mesh_model_id at both preflight call sites; + // preview_prospective_persona_snapshot helper extracted; orphan guard threaded + // through restore path. Load-bearing feature changes; queued to split. + // +9 (1360 -> 1369): start_local_agent_pairs_with_preflight — added + // personas/global load + resolve_effective_relay_mesh_model_id call to + // replace stale record-byte preflight. Same resolver pattern as + // start_local_agent_with_preflight. Load-bearing; queued to split. + ["src-tauri/src/commands/agents.rs", 1369], // agent-lifecycle-fixes: cascade-delete in delete_persona restructured into // 3-phase (stage/stop/commit) + commit_cascade_agents injectable helper for // retry-safety. Load-bearing reviewer-required change; queued to split. @@ -168,7 +176,9 @@ const overrides = new Map([ // Windows PATH-correctness fix: 3 #[cfg(windows)] test functions covering // .cmd shim rejection, .bat shim rejection, and .exe acceptance for // configure_runtime_cli (fix #2397). Test-only growth; queued to split. - ["src-tauri/src/managed_agents/runtime/tests.rs", 1041], + // +7 (1041 -> 1048): rebase onto main — this PR's resolver tests land on top + // of main's #2397 Windows shim tests. Test-only; queued to split. + ["src-tauri/src/managed_agents/runtime/tests.rs", 1055], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing @@ -203,7 +213,8 @@ const overrides = new Map([ // mapper. This is the existing API boundary; split remains queued. // team-instructions-first-class: createManagedAgent Tauri bridge threads the // new teamId input through to the backend (+1 line). - ["src/shared/api/tauri.ts", 1305], + // +2 for model_source field in RawManagedAgent + fromRawManagedAgent mapping. + ["src/shared/api/tauri.ts", 1307], // doctor-npm-eacces-preflight: hint field added to InstallStepResult (+1 line). // codex-acp-package-swap: "adapter_outdated" variant added to AcpAvailabilityStatus (+1 line). // doctor-install-reliability: AuthStatus tagged union + nodeRequired/authStatus/ @@ -438,7 +449,15 @@ const overrides = new Map([ // (if let Some(provider_update) = input.provider { record.provider = provider_update; }). // +8: harness_override thread-through in update_managed_agent so a deliberate // Custom pin routes to update_time_agent_command_override (comment + call). - ["src-tauri/src/commands/agent_models.rs", 1079], + // +22 (1079 -> 1101): Finding 2 — model discovery now resolves through + // resolve_effective_model_provider instead of raw record bytes + // (saved_agent_model_discovery_config takes personas/global and the + // get_agent_models call site loads global config), plus + // apply_model_provider_prompt_update's linked-instance write-guard + // extraction and its regression tests. + // +4 (1101 -> 1105): rebase onto agents-everywhere — agents.rs function + // signatures updated for ManagedAgentRuntimeKey-keyed runtimes map. + ["src-tauri/src/commands/agent_models.rs", 1113], // global-agent-config: get_agent_config_surface / write_agent_config_field / // put_agent_session_config commands + GlobalAgentConfig serde types. New file // in this PR; queued to split with the command module refactor. @@ -454,7 +473,12 @@ const overrides = new Map([ // relay from the harness-attached payload relayUrl (with effective-relay // fallback for older harnesses) instead of a required arg the frontend // wrapper never passed, which silently broke the session-config cache. - ["src-tauri/src/commands/agent_config.rs", 1050], + // +60 (1050 -> 1110): agent-config-resolver — resolve_config_surface now + // clears a linked instance's own system_prompt/model/provider before + // computing had_* so stale materialized snapshot bytes can never be tagged + // BuzzExplicit and shadow the definition/global fallthrough; the dead + // persona-model re-tag branch replaced; two new regression tests added. + ["src-tauri/src/commands/agent_config.rs", 1110], // codex-install-auto-restart review-fixes: should_restart_after_install // takes pid_alive:bool (pure predicate, no OS-dependent call); 3 racy // cache tests replaced with 6 pure availability_drift predicate tests; @@ -532,7 +556,9 @@ const overrides = new Map([ // for Databricks v1 gate; prospectiveRuntimeId default fallback for builtins. // PR-B moves default/API-key derivation into shared hooks; the explicit // hidden-key projection keeps the top-level secret out of Advanced rows. - ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1195], + // +6 (1195 -> 1201): rebase onto main — this PR's model-source label wiring + // lands on top of main's dialog growth. Queued to split. + ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1201], // AgentDefinitionDialog grew past 1000 with the following load-bearing fixes: // isRuntimeAutoSeededRef tracking for edit-mode seeding (Fizz shows models); // runtimeSupportsLlmProviderSelection guard on discovery provider (codex fix); diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index f6094ed529..461fed5dbd 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -39,6 +39,13 @@ pub struct RuntimeFileConfigSubset { /// Resolve the config surface with persona and global default values applied. /// +/// Linked instances are definition-authoritative: the record's own +/// system_prompt/model/provider are cleared before applying, so a stale +/// materialized snapshot can never shadow the persona's current values or a +/// blank-definition fallthrough to global defaults (mirrors +/// `effective_config::resolve_linked`). Definition-less instances keep their +/// own explicit values. +/// /// The pipeline: resolve the linked persona's prompt/model/provider, inject /// each into the record only where the record lacks its own value, let /// `read_config_surface` tag those injected fields `BuzzExplicit`, then re-tag @@ -59,6 +66,22 @@ fn resolve_config_surface( session_cache: Option<&SessionConfigCache>, global: &GlobalAgentConfig, ) -> RuntimeConfigSurface { + // Linked instances are definition-authoritative (mirrors + // `effective_config::resolve_linked`): the record's own + // system_prompt/model/provider fields are, at best, a stale materialized + // snapshot from the last `apply_persona_snapshot` — never a legitimate + // live override, since `update_managed_agent` blocks writing these three + // fields for linked instances. Clear them before computing `had_*` below + // so a stale byte can never masquerade as BuzzExplicit and suppress + // definition/global injection. Env var overrides (set via the advanced + // env-vars editor) are untouched — those remain a legitimate + // per-instance override regardless of link status. + if record.persona_id.is_some() { + record.system_prompt = None; + record.model = None; + record.provider = None; + } + let had_prompt = record.system_prompt.is_some() || record.env_vars.contains_key("BUZZ_ACP_SYSTEM_PROMPT"); let had_model = record.model.is_some(); @@ -172,22 +195,6 @@ fn resolve_config_surface( retag_global_default(&mut surface.normalized.provider); } - // Re-tag persona-snapshotted model from BuzzExplicit to PersonaDefault. - // Persona-created agents have record.model set at create time from the - // persona snapshot — had_model is true, but the model came from the persona, - // not an explicit user choice. Re-tag when the record model matches the - // persona model and no live override is active. Only applies when a persona - // is actually linked — non-persona agents with an explicit model keep BuzzExplicit. - if had_model && !model_overridden && record.persona_id.is_some() { - if let (Some(ref record_model), Some(ref persona_model_val)) = - (&record.model, &persona_model) - { - if record_model == persona_model_val { - retag_persona_default(&mut surface.normalized.model); - } - } - } - surface } @@ -728,11 +735,64 @@ mod tests { } } - /// A model the user set explicitly in Buzz must never be re-tagged to - /// `PersonaDefault`, even when the linked persona also has a model. + /// Definition-authoritative: a stale materialized `record.model` on a + /// linked instance must never outrank (or even be consulted against) the + /// linked persona's model. `update_managed_agent` already blocks writing + /// model/provider/prompt for linked instances, so a non-`None` value here + /// can only be leftover snapshot bytes from before a persona edit — the + /// panel must report the persona's current model, tagged `PersonaDefault`, + /// not the stale byte as `BuzzExplicit`. #[test] - fn explicit_record_model_outranks_persona_and_keeps_buzz_explicit_origin() { + fn linked_stale_record_model_never_outranks_persona_model() { let mut record = agent_record(); + record.model = Some("stale-explicit-model".to_string()); + let personas = vec![persona_with_model("persona-model")]; + + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + None, + &Default::default(), + ); + + let model = surface.normalized.model.as_ref().expect("model resolved"); + assert_eq!(model.value.as_deref(), Some("persona-model")); + assert_eq!(model.origin, ConfigOrigin::PersonaDefault); + } + + /// Definition-authoritative, blank-definition case: a linked instance + /// whose persona has no model of its own must fall through to the global + /// default, tagged `GlobalDefault` — mirroring + /// `effective_config::resolve_linked`'s `None => global` arm. A stale + /// materialized record model must not shadow this fallthrough either. + #[test] + fn linked_blank_definition_model_falls_through_to_global_default() { + let mut record = agent_record(); + record.model = Some("stale-explicit-model".to_string()); + let mut persona = persona_with_model("unused"); + persona.model = None; + let personas = vec![persona]; + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + ..Default::default() + }; + + let surface = + resolve_config_surface(record, &personas, Some(goose_runtime()), None, &global); + + let model = surface.normalized.model.as_ref().expect("model resolved"); + assert_eq!(model.value.as_deref(), Some("global-model")); + assert_eq!(model.origin, ConfigOrigin::GlobalDefault); + } + + /// A definition-less (no `persona_id`) instance's own explicit model IS + /// authoritative — the stale-record clearing above is scoped to linked + /// instances only. + #[test] + fn definition_less_explicit_record_model_keeps_buzz_explicit_origin() { + let mut record = agent_record(); + record.persona_id = None; record.model = Some("explicit-model".to_string()); let personas = vec![persona_with_model("persona-model")]; diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 112f297fa7..2e423ed6dd 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -87,9 +87,13 @@ pub async fn get_agent_models( // ModelPicker can persist a selected model but not rewrite the saved // provider/env snapshot, and runtime spawn reads that same snapshot. - // Discover models against the record snapshot so an out-of-date persona - // cannot offer models for a provider this agent will not launch with. - let discovery = saved_agent_model_discovery_config(record, &effective_command); + // Discover models against the resolver's effective model/provider — + // definition-authoritative for linked instances — so discovery can + // never query a stale provider this agent will not actually launch + // with. + let global = crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); + let discovery = + saved_agent_model_discovery_config(record, &effective_command, &personas, &global); ( resolved, @@ -161,27 +165,38 @@ struct SavedAgentModelDiscoveryConfig { env: BTreeMap, } +/// Resolve the model/provider discovery config from the same authoritative +/// source spawn uses (`resolve_effective_model_provider`) — linked instances +/// read their definition, never a stale materialized `record.model`/ +/// `record.provider`, so model discovery cannot query a provider this agent +/// will not actually launch with. Definition-less instances keep their own +/// record values, matching spawn's `resolve_definition_less` arm. fn saved_agent_model_discovery_config( record: &crate::managed_agents::ManagedAgentRecord, agent_command: &str, + personas: &[crate::managed_agents::AgentDefinition], + global: &crate::managed_agents::GlobalAgentConfig, ) -> SavedAgentModelDiscoveryConfig { let runtime_meta = known_acp_runtime(agent_command); + let (effective_model, effective_provider) = + crate::managed_agents::resolve_effective_model_provider(record, personas, global); + let mut derived_env = BTreeMap::new(); if let Some(meta) = runtime_meta { for (key, value) in crate::managed_agents::runtime_metadata_env_vars( meta.model_env_var, meta.provider_env_var, meta.provider_locked, - record.model.as_deref(), - record.provider.as_deref(), + effective_model.as_deref(), + effective_provider.as_deref(), ) { derived_env.insert(key.to_string(), value.to_string()); } } SavedAgentModelDiscoveryConfig { - model: record.model.clone(), - provider: record.provider.clone(), + model: effective_model, + provider: effective_provider, provider_env_var: runtime_meta.and_then(|meta| meta.provider_env_var), env: crate::managed_agents::merged_user_env(&derived_env, &record.env_vars), } @@ -775,6 +790,35 @@ async fn discover_databricks_models( })) } +/// Apply an `UpdateManagedAgentRequest`'s model/provider/system_prompt patch +/// to `record`, enforcing the linked-instance write guard: a definition-linked +/// record's model/provider/prompt are definition-authoritative (see +/// `effective_config::resolve_linked`), so writes to these three fields are +/// silently dropped for a linked instance rather than persisting a byte the +/// resolver will never read. Definition-less instances accept the patch +/// as-is. Extracted so the guard is exercised by both `update_managed_agent` +/// and its regression tests — a test that reimplements this check instead of +/// calling it can go green after the real guard is deleted. +fn apply_model_provider_prompt_update( + record: &mut crate::managed_agents::ManagedAgentRecord, + model: Option>, + provider: Option>, + system_prompt: Option>, +) { + if record.persona_id.is_some() { + return; + } + if let Some(model_update) = model { + record.model = model_update; + } + if let Some(provider_update) = provider { + record.provider = provider_update; + } + if let Some(prompt_update) = system_prompt { + record.system_prompt = prompt_update; + } +} + /// Update mutable fields on an existing managed agent record. /// /// Does NOT auto-restart the agent. Runtime config changes (system prompt, @@ -814,15 +858,12 @@ pub async fn update_managed_agent( name_changed = true; } } - if let Some(model_update) = input.model { - record.model = model_update; - } - if let Some(provider_update) = input.provider { - record.provider = provider_update; - } - if let Some(prompt_update) = input.system_prompt { - record.system_prompt = prompt_update; - } + apply_model_provider_prompt_update( + record, + input.model, + input.provider, + input.system_prompt, + ); if let Some(parallelism) = input.parallelism { record.parallelism = parallelism; } diff --git a/desktop/src-tauri/src/commands/agent_models_tests.rs b/desktop/src-tauri/src/commands/agent_models_tests.rs index 0d6449e781..d962545bdc 100644 --- a/desktop/src-tauri/src/commands/agent_models_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_tests.rs @@ -164,7 +164,7 @@ fn redaction_env_records_value_used_for_request() { } #[test] -fn saved_agent_model_discovery_uses_record_snapshot() { +fn saved_agent_model_discovery_uses_record_snapshot_for_definition_less_agent() { let record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( r#"{ "pubkey": "abcd1234", @@ -193,7 +193,12 @@ fn saved_agent_model_discovery_uses_record_snapshot() { ) .expect("sample managed agent record"); - let config = saved_agent_model_discovery_config(&record, "goose"); + let config = saved_agent_model_discovery_config( + &record, + "goose", + &[], + &crate::managed_agents::GlobalAgentConfig::default(), + ); assert_eq!(config.model.as_deref(), Some("record-model")); assert_eq!(config.provider.as_deref(), Some("databricks")); @@ -341,6 +346,74 @@ fn effective_discovery_provider_reads_the_runtimes_own_env_var() { ); } +/// Definition-authoritative: a linked agent's stale materialized +/// `record.model`/`record.provider` must never drive model discovery — the +/// linked definition's current model/provider wins, mirroring spawn's +/// `resolve_effective_model_provider`. +#[test] +fn saved_agent_model_discovery_ignores_stale_record_for_linked_agent() { + let record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "persona_id": "persona-1", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "model": "stale-record-model", + "provider": "stale-record-provider", + "env_vars": {}, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("sample managed agent record"); + + let persona = crate::managed_agents::AgentDefinition { + id: "persona-1".to_string(), + display_name: "Persona".to_string(), + avatar_url: None, + system_prompt: "You are a persona.".to_string(), + runtime: None, + model: Some("persona-model".to_string()), + provider: Some("anthropic".to_string()), + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + respond_to: None, + respond_to_allowlist: Vec::new(), + parallelism: None, + created_at: "".to_string(), + updated_at: "".to_string(), + }; + + let config = saved_agent_model_discovery_config( + &record, + "goose", + &[persona], + &crate::managed_agents::GlobalAgentConfig::default(), + ); + + assert_eq!(config.model.as_deref(), Some("persona-model")); + assert_eq!(config.provider.as_deref(), Some("anthropic")); + assert_eq!( + config.env.get("GOOSE_MODEL").map(String::as_str), + Some("persona-model") + ); +} + // --------------------------------------------------------------------------- // Databricks provider detection // --------------------------------------------------------------------------- @@ -381,6 +454,104 @@ fn update_request_turn_timeout_parses_for_wire_compat() { assert_eq!(req.turn_timeout_seconds, Some(9999)); } +// --------------------------------------------------------------------------- +// Linked-instance write guard (model/provider/prompt) +// --------------------------------------------------------------------------- + +#[test] +fn linked_instance_ignores_model_provider_prompt_writes() { + let mut record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "linked1", + "name": "linked-agent", + "persona_id": "p1", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "model": null, + "provider": null, + "env_vars": {}, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("linked agent record"); + + let is_linked = record.persona_id.is_some(); + assert!(is_linked, "test setup: record must be linked"); + + crate::commands::agent_models::apply_model_provider_prompt_update( + &mut record, + Some(Some("explicit-model".to_string())), + Some(Some("explicit-prov".to_string())), + Some(Some("explicit-prompt".to_string())), + ); + + assert!( + record.model.is_none(), + "linked record model must not be updated" + ); + assert!( + record.provider.is_none(), + "linked record provider must not be updated" + ); + assert!( + record.system_prompt.is_none(), + "linked record system_prompt must not be updated" + ); +} + +#[test] +fn definition_less_instance_accepts_model_provider_prompt_writes() { + let mut record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "standalone1", + "name": "standalone-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "model": null, + "provider": null, + "env_vars": {}, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("standalone agent record"); + + let is_linked = record.persona_id.is_some(); + assert!(!is_linked, "test setup: record must not be linked"); + + crate::commands::agent_models::apply_model_provider_prompt_update( + &mut record, + Some(Some("new-model".to_string())), + Some(Some("new-prov".to_string())), + Some(Some("new-prompt".to_string())), + ); + + assert_eq!(record.model.as_deref(), Some("new-model")); + assert_eq!(record.provider.as_deref(), Some("new-prov")); + assert_eq!(record.system_prompt.as_deref(), Some("new-prompt")); +} + #[test] fn is_databricks_provider_matches_both_variants() { assert!(is_databricks_provider(Some("databricks"))); diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index c65900b5de..61e4034b9f 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -293,16 +293,16 @@ fn resolve_created_avatar_url( #[cfg(feature = "mesh-llm")] async fn ensure_relay_mesh_for_record( app: &AppHandle, - record: &ManagedAgentRecord, + model_id: Option<&str>, allow_fresh_create_start: bool, ) -> Result<(), String> { - crate::commands::ensure_relay_mesh_for_record(app, record, allow_fresh_create_start).await + crate::commands::ensure_relay_mesh_for_record(app, model_id, allow_fresh_create_start).await } #[cfg(not(feature = "mesh-llm"))] async fn ensure_relay_mesh_for_record( _app: &AppHandle, - _record: &ManagedAgentRecord, + _model_id: Option<&str>, _allow_fresh_create_start: bool, ) -> Result<(), String> { Ok(()) @@ -327,7 +327,16 @@ pub(super) async fn start_local_agent_pairs_with_preflight( if record_snapshot.backend != BackendKind::Local { return Err(format!("agent {pubkey} is not a local agent")); } - ensure_relay_mesh_for_record(app, &record_snapshot, false).await?; + let personas_for_preflight = load_personas(app).unwrap_or_default(); + let global_for_preflight = + crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + let mesh_model_id = + crate::managed_agents::effective_config::resolve_effective_relay_mesh_model_id( + &record_snapshot, + &personas_for_preflight, + &global_for_preflight, + ); + ensure_relay_mesh_for_record(app, mesh_model_id.as_deref(), false).await?; { let _store_guard = state @@ -407,7 +416,22 @@ pub(super) async fn start_local_agent_with_preflight( return Err(format!("agent {pubkey} is not a local agent")); } - ensure_relay_mesh_for_record(app, &record_snapshot, allow_fresh_create_start).await?; + // Preflight against the same resolution spawn uses — `resolve_effective_config` + // (definition → global fallback). A linked instance's own `provider`/`model`/ + // `relay_mesh` bytes never contribute: this reads the CURRENT definition + // directly, so a definition edit that flips `provider` to/from relay-mesh + // between saves is reflected here without needing a prospective re-snapshot; + // for a global-inherited blank definition, it also folds in the global + // default, which record-byte sniffing could never see. + let personas = load_personas(app).unwrap_or_default(); + let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + let mesh_model_id = + crate::managed_agents::effective_config::resolve_effective_relay_mesh_model_id( + &record_snapshot, + &personas, + &global, + ); + ensure_relay_mesh_for_record(app, mesh_model_id.as_deref(), allow_fresh_create_start).await?; let _store_guard = state .managed_agents_store_lock @@ -431,9 +455,16 @@ pub(super) async fn start_local_agent_with_preflight( // at the end — avoids a second disk read for the same file in the same call. let personas = load_personas(app).unwrap_or_default(); if let Some(persona_id) = record.persona_id.clone() { - if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { - crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); - record.updated_at = crate::util::now_iso(); + match personas.iter().find(|p| p.id == persona_id) { + Some(persona) => { + crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); + record.updated_at = crate::util::now_iso(); + } + None => { + return Err( + crate::managed_agents::effective_config::ORPHANED_INSTANCE_ERROR.to_string(), + ); + } } } start_managed_agent_process(app, record, &mut runtimes, Some(owner_hex))?; diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index be132225ea..af785711d5 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -4,42 +4,36 @@ use tauri::AppHandle; +#[cfg(test)] +use crate::managed_agents::AgentDefinition; use crate::{ app_state::AppState, - managed_agents::{load_personas, AgentDefinition, ManagedAgentRecord}, + managed_agents::{load_personas, ManagedAgentRecord}, relay::relay_ws_url_with_override, }; /// Resolve the deploy-specific structured model/provider for a managed agent. /// -/// Deploy uses **live-persona-first** precedence so remote agents receive -/// current config after a persona update, without requiring delete+recreate. -/// Unlike local spawn (which re-snapshots the persona onto `record` at the -/// start of every spawn), provider start does not re-snapshot — so the -/// record may hold a stale snapshot while the linked persona has moved on. +/// Delegates to the single effective-config resolver which enforces +/// definition-authoritative semantics for linked instances: +/// - **Linked:** definition → global. Stale record bytes are never consulted. +/// - **Definition-less:** instance → global. +/// - **Orphaned:** returns `(None, None)` — spawn is blocked elsewhere. /// -/// Precedence: live-persona → record (snapshot fallback) → global. -/// Symmetric for both model and provider. +/// Both local spawn and deploy now use the same resolver, so they can never +/// disagree on what model/provider an agent runs with. /// /// Exported `pub(crate)` for unit testing. -pub(crate) fn resolve_deploy_model_provider<'a>( - record: &'a ManagedAgentRecord, - personas: &'a [AgentDefinition], - global: &'a crate::managed_agents::GlobalAgentConfig, -) -> (Option<&'a str>, Option<&'a str>) { - let live_persona = record - .persona_id - .as_deref() - .and_then(|pid| personas.iter().find(|p| p.id == pid)); - let model = live_persona - .and_then(|p| p.model.as_deref()) - .or(record.model.as_deref()) - .or(global.model.as_deref()); - let provider = live_persona - .and_then(|p| p.provider.as_deref()) - .or(record.provider.as_deref()) - .or(global.provider.as_deref()); - (model, provider) +#[cfg(test)] +pub(crate) fn resolve_deploy_model_provider( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], + global: &crate::managed_agents::GlobalAgentConfig, +) -> (Option, Option) { + crate::managed_agents::effective_config::resolve_effective_model_provider_pair( + record, personas, global, + ) + .unwrap_or((None, None)) } /// Build the standard agent JSON payload for provider deploy calls. @@ -84,15 +78,16 @@ pub(super) fn build_deploy_payload( let merged_env = crate::managed_agents::merged_user_env(&global_persona_merged, &record.env_vars); - // Resolve the deploy-specific structured provider/model. Uses the deploy - // resolver with live-persona → record → global precedence. let personas = load_personas(app).unwrap_or_default(); - let (effective_model, effective_provider) = - resolve_deploy_model_provider(record, &personas, &global_config); - let (effective_model, effective_provider) = ( - effective_model.map(str::to_string), - effective_provider.map(str::to_string), - ); + let cfg = crate::managed_agents::effective_config::resolve_effective_config( + record, + &personas, + &global_config, + ) + .require_resolved()?; + let effective_model = cfg.model.value; + let effective_provider = cfg.provider.value; + let effective_prompt = cfg.system_prompt.value; Ok(deploy_payload_json( record, @@ -102,6 +97,7 @@ pub(super) fn build_deploy_payload( ), effective_model, effective_provider, + effective_prompt, merged_env, )) } @@ -114,36 +110,25 @@ pub(super) fn deploy_payload_json( relay_url: String, effective_model: Option, effective_provider: Option, + effective_prompt: Option, merged_env: std::collections::BTreeMap, ) -> serde_json::Value { serde_json::json!({ "name": &record.name, - // Resolve the per-agent pin against the active workspace relay here: - // this payload crosses the host boundary to a remote provider harness - // that has no notion of the desktop's workspace, so the blank→workspace - // fallback (otherwise applied at read-time in `effective_agent_relay_url`) - // must be materialized into a concrete URL before serializing. "relay_url": relay_url, "private_key_nsec": &record.private_key_nsec, "auth_tag": &record.auth_tag, "agent_command": &record.agent_command, "agent_args": &record.agent_args, - "system_prompt": &record.system_prompt, + "system_prompt": effective_prompt, "model": effective_model, - // Structured provider from the persona record. Providers that don't - // yet read this field will fall back to env_vars or their own default - // — no protocol break. "provider": effective_provider, "turn_timeout_seconds": record.turn_timeout_seconds, "idle_timeout_seconds": record.idle_timeout_seconds, "max_turn_duration_seconds": record.max_turn_duration_seconds, "parallelism": record.parallelism, - // Inbound author gate. Providers that don't yet read these fall back - // to the harness default (`owner-only`) — no protocol break. "respond_to": record.respond_to, "respond_to_allowlist": &record.respond_to_allowlist, - // Merged persona + agent env vars. Providers that don't read this - // field will simply ignore it — no protocol break. "env_vars": merged_env, }) } diff --git a/desktop/src-tauri/src/commands/agents_tests.rs b/desktop/src-tauri/src/commands/agents_tests.rs index 1b2e0ed179..e32fc1cfe4 100644 --- a/desktop/src-tauri/src/commands/agents_tests.rs +++ b/desktop/src-tauri/src/commands/agents_tests.rs @@ -126,57 +126,56 @@ fn build_agent_archive_request_attaches_owner_auth_and_retired_reason() { })); } -/// Deploy-path regression for Fix 1 of Thufir pass-2: a persona-linked -/// provider agent with a stale record snapshot must use the live persona -/// model/provider in the deploy payload, not the stale record values. -/// -/// Scenario: agent was created with persona at model="old-model"/provider="old-prov". -/// The persona was subsequently updated to "new-model"/"new-prov" but the record -/// was NOT re-snapshotted (provider start skips re-snapshot; local spawn does it). -/// The deploy resolver must use the current persona values. -/// -/// Fails against `resolve_effective_model_provider` (record-first precedence), -/// which would return "old-model"/"old-prov" from the stale record. +/// Deploy resolver uses definition model/provider, ignoring stale record. #[test] -fn deploy_resolver_uses_live_persona_over_stale_record_snapshot() { - // Record holds the stale snapshot (created when persona had old values). +fn deploy_resolver_uses_definition_over_stale_record() { let record = bare_agent_record(Some("p1"), Some("old-model"), Some("old-prov")); - // Live persona has been updated since the record was snapshotted. let personas = vec![persona_record("p1", Some("new-model"), Some("new-prov"))]; let global = crate::managed_agents::GlobalAgentConfig::default(); let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); assert_eq!( - model, + model.as_deref(), Some("new-model"), - "deploy must use live persona model, not stale record snapshot" + "deploy must use definition model, not stale record snapshot" ); assert_eq!( - provider, + provider.as_deref(), Some("new-prov"), - "deploy must use live persona provider, not stale record snapshot" + "deploy must use definition provider, not stale record snapshot" ); } -/// Deploy resolver falls back to record when persona has no model/provider -/// (persona without structured model — fallback to record snapshot). +/// When a linked definition has blank model/provider (inherit), the deploy +/// resolver must fall through to global — stale record bytes are inert. #[test] -fn deploy_resolver_falls_back_to_record_when_persona_has_none() { - let record = bare_agent_record(Some("p1"), Some("record-model"), Some("record-prov")); - // Persona exists but has no model/provider. +fn deploy_resolver_inherits_global_when_definition_blank() { + let record = bare_agent_record(Some("p1"), Some("stale-model"), Some("stale-prov")); let personas = vec![persona_record("p1", None, None)]; - let global = crate::managed_agents::GlobalAgentConfig::default(); + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-prov".to_string()), + ..Default::default() + }; let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); - assert_eq!(model, Some("record-model")); - assert_eq!(provider, Some("record-prov")); + assert_eq!( + model.as_deref(), + Some("global-model"), + "definition blank → global; stale record ignored" + ); + assert_eq!( + provider.as_deref(), + Some("global-prov"), + "definition blank → global; stale record ignored" + ); } -/// Deploy resolver falls back to global when both persona and record have none. +/// Deploy resolver falls back to global when both definition and record have none. #[test] -fn deploy_resolver_falls_back_to_global_when_persona_and_record_have_none() { +fn deploy_resolver_falls_back_to_global_when_definition_and_record_have_none() { let record = bare_agent_record(Some("p1"), None, None); let personas = vec![persona_record("p1", None, None)]; let global = crate::managed_agents::GlobalAgentConfig { @@ -187,8 +186,35 @@ fn deploy_resolver_falls_back_to_global_when_persona_and_record_have_none() { let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); - assert_eq!(model, Some("global-model")); - assert_eq!(provider, Some("global-prov")); + assert_eq!(model.as_deref(), Some("global-model")); + assert_eq!(provider.as_deref(), Some("global-prov")); +} + +/// Orphan: linked record with missing definition → the pure model/provider +/// pair resolver returns `(None, None)`. This is NOT the deploy refusal +/// boundary — `build_deploy_payload` refuses an orphan outright via +/// `.require_resolved()?` before this pair is ever computed. This test pins +/// the resolver's own orphan behavior, which readiness/hash also depend on. +#[test] +fn deploy_resolver_returns_none_for_orphaned_instance() { + let record = bare_agent_record(Some("missing-def"), Some("stale-model"), Some("stale-prov")); + let personas: Vec = vec![]; + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-prov".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); + + assert!( + model.is_none(), + "orphaned instance must not resolve to any model" + ); + assert!( + provider.is_none(), + "orphaned instance must not resolve to any provider" + ); } #[test] @@ -404,6 +430,7 @@ fn deploy_payload_carries_the_full_behavioral_quad() { "wss://relay.example".to_string(), Some("gpt-x".to_string()), Some("openai".to_string()), + None, std::collections::BTreeMap::new(), ); diff --git a/desktop/src-tauri/src/commands/mesh_llm.rs b/desktop/src-tauri/src/commands/mesh_llm.rs index adcdeb7ec1..87af9fa08f 100644 --- a/desktop/src-tauri/src/commands/mesh_llm.rs +++ b/desktop/src-tauri/src/commands/mesh_llm.rs @@ -481,11 +481,11 @@ fn pick_serve_target_for_model( /// Non relay-mesh records are a no-op. pub(crate) async fn ensure_relay_mesh_for_record( app: &AppHandle, - record: &crate::managed_agents::ManagedAgentRecord, + model_id: Option<&str>, _allow_fresh_create_start: bool, ) -> Result<(), String> { let state = app.state::(); - let Some(model_id) = crate::managed_agents::relay_mesh_model_id(record) else { + let Some(model_id) = model_id else { return Ok(()); }; // A local serve/client runtime already owns the OpenAI ingress and its @@ -505,7 +505,7 @@ pub(crate) async fn ensure_relay_mesh_for_record( .await { mesh_llm::MeshRuntimeRecovery::Live => { - return wait_for_mesh_inference(&model_id).await; + return wait_for_mesh_inference(model_id).await; } mesh_llm::MeshRuntimeRecovery::Evicted | mesh_llm::MeshRuntimeRecovery::Absent => {} mesh_llm::MeshRuntimeRecovery::Debouncing => { @@ -521,7 +521,7 @@ pub(crate) async fn ensure_relay_mesh_for_record( ); } mesh_llm::MeshRuntimeRecovery::Replaced => { - return wait_for_mesh_inference(&model_id).await; + return wait_for_mesh_inference(model_id).await; } mesh_llm::MeshRuntimeRecovery::RestartRequired => { app.request_restart(); @@ -532,7 +532,7 @@ pub(crate) async fn ensure_relay_mesh_for_record( } } } - let target = match resolve_mesh_bootstrap_target(&state, &model_id).await { + let target = match resolve_mesh_bootstrap_target(&state, model_id).await { Ok(Some(target)) => target, Ok(None) => { return Err( @@ -556,8 +556,8 @@ pub(crate) async fn ensure_relay_mesh_for_record( // (the router resolves per-request), so it only cold-starts a Client when // there is genuinely no runtime. Falling back to Client if a serve node // crashed under local pressure is a desirable fail-safe, not a regression. - ensure_client_node_for_model(&state, &model_id, Some(target.endpoint_addr)).await?; - wait_for_mesh_inference(&model_id).await + ensure_client_node_for_model(&state, model_id, Some(target.endpoint_addr)).await?; + wait_for_mesh_inference(model_id).await } #[tauri::command] diff --git a/desktop/src-tauri/src/managed_agents/effective_config/mod.rs b/desktop/src-tauri/src/managed_agents/effective_config/mod.rs new file mode 100644 index 0000000000..e079c76a1d --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/effective_config/mod.rs @@ -0,0 +1,318 @@ +use std::collections::BTreeMap; + +use serde::Serialize; + +use super::global_config::GlobalAgentConfig; +use super::relay_mesh::{ + RELAY_MESH_API_BASE_URL, RELAY_MESH_API_KEY_PLACEHOLDER, RELAY_MESH_AUTO_MODEL_ID, + RELAY_MESH_PROVIDER_ID, +}; +use super::types::{AgentDefinition, ManagedAgentRecord}; + +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum ConfigSource { + Definition, + Global, + InstanceLegacy, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedField { + pub value: Option, + pub source: ConfigSource, +} + +#[derive(Debug, Clone)] +pub struct EffectiveAgentConfig { + pub model: ResolvedField, + pub provider: ResolvedField, + pub system_prompt: ResolvedField, +} + +impl EffectiveAgentConfig { + /// The relay-mesh model id this config resolves to, or `None` when the + /// effective provider isn't relay-mesh. + /// + /// This is the single authoritative mesh decision for this config. Both + /// the mesh preflight (interactive start, restore-on-launch) AND spawn's + /// `apply_relay_mesh_env` block MUST derive their mesh gate from this + /// method — never from a separate provider comparison — so the two paths + /// are guaranteed to agree even when the stored provider string has leading + /// or trailing whitespace. The provider is trimmed before matching; + /// a blank effective model falls back to "auto", mirroring + /// `apply_relay_mesh_env`'s own rule. + pub fn relay_mesh_model_id(&self) -> Option { + if self.provider.value.as_deref().map(str::trim) != Some(RELAY_MESH_PROVIDER_ID) { + return None; + } + Some( + self.model + .value + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + .unwrap_or(RELAY_MESH_AUTO_MODEL_ID) + .to_string(), + ) + } +} + +#[derive(Debug, Clone)] +pub enum EffectiveConfigResult { + Resolved(EffectiveAgentConfig), + OrphanedInstance { + record_pubkey: String, + missing_persona_id: String, + }, +} + +fn non_blank(v: Option<&str>) -> Option<&str> { + v.filter(|s| !s.trim().is_empty()) +} + +fn resolve_linked( + definition: &AgentDefinition, + global: &GlobalAgentConfig, +) -> EffectiveAgentConfig { + let model = match non_blank(definition.model.as_deref()) { + Some(m) => ResolvedField { + value: Some(m.to_owned()), + source: ConfigSource::Definition, + }, + None => ResolvedField { + value: global.model.clone(), + source: ConfigSource::Global, + }, + }; + + let provider = match non_blank(definition.provider.as_deref()) { + Some(p) => ResolvedField { + value: Some(p.to_owned()), + source: ConfigSource::Definition, + }, + None => ResolvedField { + value: global.provider.clone(), + source: ConfigSource::Global, + }, + }; + + let system_prompt = ResolvedField { + value: non_blank(Some(definition.system_prompt.as_str())).map(str::to_owned), + source: ConfigSource::Definition, + }; + + EffectiveAgentConfig { + model, + provider, + system_prompt, + } +} + +/// The API key value the relay-mesh preset wrote before the Jun-11 rename +/// window (#960, `8f580f308`) changed `RELAY_MESH_API_KEY_PLACEHOLDER` from +/// `"sprout-mesh-local"` to its current value. The old string is persisted as a +/// *value* in `env_vars` on records created before that, and no migration ever +/// rewrote it. +const LEGACY_MESH_API_KEY_PLACEHOLDER: &str = "sprout-mesh-local"; + +/// The provider key the preset wrote before #971 (`8c8312932`) renamed it to +/// `BUZZ_AGENT_PROVIDER`. That commit changed source literals only — persisted +/// `env_vars` keys were never migrated. +const LEGACY_MESH_PROVIDER_ENV_KEY: &str = "SPROUT_AGENT_PROVIDER"; + +/// The legacy env discriminator: recognizes the relay-mesh preset purely from +/// the env vars a pre-typed-field record carries, returning its served model id. +/// +/// All three sentinels must match — the local base URL alone is not enough, +/// since a user may point their own OpenAI-compatible provider at the same +/// port. The placeholder API key is what makes this Buzz's own preset. +/// +/// Two of those sentinels were renamed in the same Jun-11 window, in separate +/// commits, with neither migrating persisted records: the provider env *key* +/// (#971) and the API key *value* (#960). Each is therefore accepted under +/// either spelling, independently — a record straddling the window carries one +/// old and one new. In both cases the current spelling is authoritative when +/// present, so a record that has since been rewritten with a non-mesh value is +/// not resurrected by the stale leftover beside it. +/// +/// `OPENAI_COMPAT_BASE_URL` and `OPENAI_COMPAT_MODEL` were never renamed. +/// Nothing beyond these two either/ors is loosened: every sentinel dropped +/// widens the false-positive surface for a user's own openai-compatible agent. +fn mesh_preset_env_model_id(env_vars: &BTreeMap) -> Option { + let base_url = env_vars.get("OPENAI_COMPAT_BASE_URL")?.trim(); + if base_url.trim_end_matches('/') != RELAY_MESH_API_BASE_URL { + return None; + } + let provider = env_vars + .get("BUZZ_AGENT_PROVIDER") + .or_else(|| env_vars.get(LEGACY_MESH_PROVIDER_ENV_KEY))? + .trim(); + if provider != "openai" { + return None; + } + let api_key = env_vars.get("OPENAI_COMPAT_API_KEY")?.trim(); + if api_key != RELAY_MESH_API_KEY_PLACEHOLDER && api_key != LEGACY_MESH_API_KEY_PLACEHOLDER { + return None; + } + non_blank(env_vars.get("OPENAI_COMPAT_MODEL").map(String::as_str)).map(str::to_owned) +} + +/// The mesh model id a definition-less record carries in legacy form, or `None` +/// when it is not a legacy mesh record. +/// +/// Two shipped record generations predate `provider: "relay-mesh"` and are +/// never rewritten on load, so they are still on disk: +/// +/// - the typed `relay_mesh` marker, added before the record had a `provider` +/// field at all; +/// - before that, the mesh preset written straight into `env_vars`. +/// +/// Consulted only by [`resolve_definition_less`]: a record with no definition +/// has nothing authoritative to be overridden by, so its own bytes are the +/// highest-precedence signal it has. A linked instance never reaches here. +fn legacy_record_mesh_model_id(record: &ManagedAgentRecord) -> Option { + match &record.relay_mesh { + // The marker itself is the mesh signal; a blank `model_ref` still means + // mesh, resolved to the auto model exactly as `apply_relay_mesh_env` + // and `relay_mesh_model_id` treat a blank model. + Some(config) => Some( + non_blank(Some(config.model_ref.as_str())) + .unwrap_or(RELAY_MESH_AUTO_MODEL_ID) + .to_owned(), + ), + None => mesh_preset_env_model_id(&record.env_vars), + } +} + +fn resolve_definition_less( + record: &ManagedAgentRecord, + global: &GlobalAgentConfig, +) -> EffectiveAgentConfig { + let model = match non_blank(record.model.as_deref()) { + Some(m) => ResolvedField { + value: Some(m.to_owned()), + source: ConfigSource::InstanceLegacy, + }, + None => ResolvedField { + value: global.model.clone(), + source: ConfigSource::Global, + }, + }; + + let provider = match non_blank(record.provider.as_deref()) { + Some(p) => ResolvedField { + value: Some(p.to_owned()), + source: ConfigSource::InstanceLegacy, + }, + None => ResolvedField { + value: global.provider.clone(), + source: ConfigSource::Global, + }, + }; + + let system_prompt = ResolvedField { + value: non_blank(record.system_prompt.as_deref()).map(str::to_owned), + source: ConfigSource::InstanceLegacy, + }; + + let mut config = EffectiveAgentConfig { + model, + provider, + system_prompt, + }; + + // Legacy mesh compatibility. A record with an explicit `provider` has + // already stated its intent — including switching AWAY from mesh, which + // leaves the old marker and env bytes behind — so its legacy bytes are + // never consulted. Only a record that never carried a provider at all + // falls back, and both fields move together so the single mesh gate + // (`relay_mesh_model_id`) and the spawned model agree. + if non_blank(record.provider.as_deref()).is_none() { + if let Some(model_ref) = legacy_record_mesh_model_id(record) { + config.provider = ResolvedField { + value: Some(RELAY_MESH_PROVIDER_ID.to_owned()), + source: ConfigSource::InstanceLegacy, + }; + config.model = ResolvedField { + value: Some(model_ref), + source: ConfigSource::InstanceLegacy, + }; + } + } + + config +} + +pub fn resolve_effective_config( + record: &ManagedAgentRecord, + definitions: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> EffectiveConfigResult { + match &record.persona_id { + Some(pid) => match definitions.iter().find(|d| d.id == *pid) { + Some(def) => EffectiveConfigResult::Resolved(resolve_linked(def, global)), + None => EffectiveConfigResult::OrphanedInstance { + record_pubkey: record.pubkey.clone(), + missing_persona_id: pid.clone(), + }, + }, + None => EffectiveConfigResult::Resolved(resolve_definition_less(record, global)), + } +} + +pub fn resolve_effective_model_provider_pair( + record: &ManagedAgentRecord, + definitions: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> Option<(Option, Option)> { + match resolve_effective_config(record, definitions, global) { + EffectiveConfigResult::Resolved(cfg) => Some((cfg.model.value, cfg.provider.value)), + EffectiveConfigResult::OrphanedInstance { .. } => None, + } +} + +/// The relay-mesh preflight decision for `record`, resolved the same way +/// spawn resolves its mesh env: through `resolve_effective_config` (which +/// folds in the definition → global fallback). A linked instance's own +/// `provider`/`model`/`relay_mesh` bytes never contribute; a definition-less +/// legacy record may fall back to them via `legacy_record_mesh_model_id`, +/// which is confined to `resolve_definition_less`. +/// +/// `None` covers both "not a mesh agent" and "orphaned instance" — an orphan +/// never spawns (see `require_resolved`), so it never needs a mesh preflight +/// either; the caller's own orphan handling downstream is unaffected, this +/// just avoids tripping mesh bootstrap for a start that will be refused. +pub fn resolve_effective_relay_mesh_model_id( + record: &ManagedAgentRecord, + definitions: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> Option { + match resolve_effective_config(record, definitions, global) { + EffectiveConfigResult::Resolved(cfg) => cfg.relay_mesh_model_id(), + EffectiveConfigResult::OrphanedInstance { .. } => None, + } +} + +/// The single user-facing message for a linked instance whose definition no +/// longer exists. Shared by every path that must refuse to act on an orphan: +/// the spawn boundary (`spawn_agent_child`), the interactive start command, +/// and provider deploy. +pub const ORPHANED_INSTANCE_ERROR: &str = + "This agent's configuration is missing — it may still be \ + syncing or was deleted on another device."; + +impl EffectiveConfigResult { + /// Unwrap into the resolved config, or the shared orphan-refusal error. + pub fn require_resolved(self) -> Result { + match self { + EffectiveConfigResult::Resolved(cfg) => Ok(cfg), + EffectiveConfigResult::OrphanedInstance { .. } => { + Err(ORPHANED_INSTANCE_ERROR.to_string()) + } + } + } +} + +#[cfg(test)] +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/effective_config/tests.rs b/desktop/src-tauri/src/managed_agents/effective_config/tests.rs new file mode 100644 index 0000000000..81c2611d5c --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/effective_config/tests.rs @@ -0,0 +1,872 @@ +use super::*; +use std::collections::BTreeMap; + +fn definition( + id: &str, + model: Option<&str>, + provider: Option<&str>, + prompt: &str, +) -> AgentDefinition { + AgentDefinition { + id: id.to_string(), + display_name: "Test Definition".to_string(), + avatar_url: None, + system_prompt: prompt.to_string(), + runtime: None, + model: model.map(str::to_string), + provider: provider.map(str::to_string), + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + respond_to: None, + respond_to_allowlist: vec![], + parallelism: None, + created_at: "".to_string(), + updated_at: "".to_string(), + } +} + +fn record( + persona_id: Option<&str>, + model: Option<&str>, + provider: Option<&str>, + prompt: Option<&str>, +) -> ManagedAgentRecord { + use crate::managed_agents::{BackendKind, RespondTo}; + ManagedAgentRecord { + pubkey: "agent-pk".to_string(), + name: "Agent".to_string(), + persona_id: persona_id.map(str::to_string), + private_key_nsec: "".to_string(), + auth_tag: None, + relay_url: "ws://localhost:3000".to_string(), + avatar_url: None, + acp_command: "buzz-acp".to_string(), + agent_command: "goose".to_string(), + agent_command_override: None, + agent_args: vec![], + mcp_command: "".to_string(), + turn_timeout_seconds: 300, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: prompt.map(str::to_string), + model: model.map(str::to_string), + provider: provider.map(str::to_string), + persona_source_version: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + runtime_pid: None, + backend: BackendKind::Local, + backend_agent_id: None, + provider_binary_path: None, + team_id: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "".to_string(), + updated_at: "".to_string(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + relay_mesh: None, + auto_restart_on_config_change: false, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_parallelism: None, + } +} + +fn global(model: Option<&str>, provider: Option<&str>) -> GlobalAgentConfig { + GlobalAgentConfig { + model: model.map(str::to_string), + provider: provider.map(str::to_string), + ..Default::default() + } +} + +// ── Linked instance: definition → global, record ignored ── + +#[test] +fn linked_definition_model_wins_over_stale_record() { + let rec = record( + Some("d1"), + Some("stale-model"), + Some("stale-prov"), + Some("stale prompt"), + ); + let defs = vec![definition( + "d1", + Some("def-model"), + Some("def-prov"), + "def prompt", + )]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("def-model")); + assert_eq!(cfg.model.source, ConfigSource::Definition); + assert_eq!(cfg.provider.value.as_deref(), Some("def-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Definition); + assert_eq!(cfg.system_prompt.value.as_deref(), Some("def prompt")); + assert_eq!(cfg.system_prompt.source, ConfigSource::Definition); +} + +#[test] +fn linked_inherit_global_when_definition_blank() { + let rec = record( + Some("d1"), + Some("stale-model"), + Some("stale-prov"), + Some("stale prompt"), + ); + let defs = vec![definition("d1", None, None, "")]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("global-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); + assert_eq!(cfg.system_prompt.value, None); + assert_eq!(cfg.system_prompt.source, ConfigSource::Definition); +} + +#[test] +fn linked_stale_record_model_is_inert() { + let rec = record(Some("d1"), Some("stale-model"), Some("stale-prov"), None); + let defs = vec![definition("d1", None, None, "")]; + let g = global(None, None); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value, None); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value, None); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +#[test] +fn linked_definition_model_set_provider_inherits() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("def-model"), None, "prompt")]; + let g = global(None, Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("def-model")); + assert_eq!(cfg.model.source, ConfigSource::Definition); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +#[test] +fn linked_blank_prompt_means_no_prompt() { + let rec = record(Some("d1"), None, None, Some("stale prompt on record")); + let defs = vec![definition("d1", None, None, "")]; + let g = global(None, None); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.system_prompt.value, None); + assert_eq!(cfg.system_prompt.source, ConfigSource::Definition); +} + +#[test] +fn linked_whitespace_only_definition_model_inherits_global() { + let rec = record(Some("d1"), Some("stale"), None, None); + let defs = vec![definition("d1", Some(" "), Some(" \t"), "")]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("global-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +// ── Orphaned instance ── + +#[test] +fn orphaned_linked_instance_returns_error() { + let rec = record(Some("missing-def"), None, None, None); + let defs = vec![]; + let g = global(Some("global-model"), None); + + let result = resolve_effective_config(&rec, &defs, &g); + match result { + EffectiveConfigResult::OrphanedInstance { + record_pubkey, + missing_persona_id, + } => { + assert_eq!(record_pubkey, "agent-pk"); + assert_eq!(missing_persona_id, "missing-def"); + } + other => panic!("expected OrphanedInstance, got {:?}", other), + } +} + +// ── Definition-less instance: instance → global ── + +#[test] +fn definition_less_uses_own_fields() { + let rec = record(None, Some("my-model"), Some("my-prov"), Some("my prompt")); + let defs = vec![]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("my-model")); + assert_eq!(cfg.model.source, ConfigSource::InstanceLegacy); + assert_eq!(cfg.provider.value.as_deref(), Some("my-prov")); + assert_eq!(cfg.provider.source, ConfigSource::InstanceLegacy); + assert_eq!(cfg.system_prompt.value.as_deref(), Some("my prompt")); + assert_eq!(cfg.system_prompt.source, ConfigSource::InstanceLegacy); +} + +#[test] +fn definition_less_falls_back_to_global() { + let rec = record(None, None, None, None); + let defs = vec![]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("global-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +#[test] +fn definition_less_blank_record_fields_fall_through() { + let rec = record(None, Some(" "), Some(""), Some(" ")); + let defs = vec![]; + let g = global(Some("g-model"), None); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("g-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value, None); + assert_eq!(cfg.provider.source, ConfigSource::Global); + assert_eq!(cfg.system_prompt.value, None); +} + +// ── Convenience helper ── + +#[test] +fn model_provider_pair_returns_none_for_orphan() { + let rec = record(Some("missing"), None, None, None); + assert_eq!( + resolve_effective_model_provider_pair(&rec, &[], &global(None, None)), + None + ); +} + +#[test] +fn model_provider_pair_returns_resolved_values() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), Some("p"), "")]; + let g = global(None, None); + + let pair = resolve_effective_model_provider_pair(&rec, &defs, &g); + assert_eq!(pair, Some((Some("m".to_string()), Some("p".to_string())))); +} + +// ── require_resolved: the shared orphan-refusal contract used by +// build_deploy_payload and spawn_agent_child ── + +#[test] +fn require_resolved_returns_shared_error_for_orphan() { + let rec = record(Some("missing"), None, None, None); + let error = resolve_effective_config(&rec, &[], &global(None, None)) + .require_resolved() + .expect_err("orphan must not resolve"); + assert_eq!(error, ORPHANED_INSTANCE_ERROR); +} + +#[test] +fn require_resolved_returns_config_for_resolved() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), Some("p"), "prompt")]; + let cfg = resolve_effective_config(&rec, &defs, &global(None, None)) + .require_resolved() + .expect("linked instance with a live definition must resolve"); + assert_eq!(cfg.model.value.as_deref(), Some("m")); +} + +#[test] +fn require_resolved_refuses_orphan_only() { + let orphan = record(Some("missing"), None, None, None); + assert_eq!( + resolve_effective_config(&orphan, &[], &global(None, None)) + .require_resolved() + .unwrap_err(), + ORPHANED_INSTANCE_ERROR, + ); + + let linked = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), None, "")]; + assert!( + resolve_effective_config(&linked, &defs, &global(None, None)) + .require_resolved() + .is_ok() + ); + + // Definition-less instances are never orphaned regardless of how bare + // their own fields are — orphan status only applies to a dangling link. + let bare = record(None, None, None, None); + assert!(resolve_effective_config(&bare, &[], &global(None, None)) + .require_resolved() + .is_ok()); +} + +// ── Morgan's exact regression sequence ── + +#[test] +fn morgans_sequence_inherit_explicit_inherit() { + let g = global(Some("claude-opus-4-6"), Some("anthropic")); + + // Step 1: fresh agent with inherited model → resolves global + let rec_step1 = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, None, "agent prompt")]; + let cfg1 = match resolve_effective_config(&rec_step1, &defs, &g) { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("step 1: {:?}", other), + }; + assert_eq!(cfg1.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!(cfg1.model.source, ConfigSource::Global); + + // Step 2: set explicit model on definition + let defs_explicit = vec![definition( + "d1", + Some("goose-gpt-5-6-sol"), + Some("databricks"), + "agent prompt", + )]; + let cfg2 = match resolve_effective_config(&rec_step1, &defs_explicit, &g) { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("step 2: {:?}", other), + }; + assert_eq!(cfg2.model.value.as_deref(), Some("goose-gpt-5-6-sol")); + assert_eq!(cfg2.model.source, ConfigSource::Definition); + + // Step 3: switch back to inherit — even with stale record bytes + let rec_stale = record( + Some("d1"), + Some("goose-gpt-5-6-sol"), + Some("databricks"), + None, + ); + let defs_inherit = vec![definition("d1", None, None, "agent prompt")]; + let cfg3 = match resolve_effective_config(&rec_stale, &defs_inherit, &g) { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("step 3: {:?}", other), + }; + assert_eq!(cfg3.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!(cfg3.model.source, ConfigSource::Global); + assert_eq!(cfg3.provider.value.as_deref(), Some("anthropic")); + assert_eq!(cfg3.provider.source, ConfigSource::Global); +} + +// ── relay-mesh preflight resolution (Wes review 2 on #1968) ── +// +// Both regressions below reproduce the exact defects: the preflight must +// key off `resolve_effective_config`'s resolution — the same one spawn's +// mesh env consults — not the record's own `provider`/`model` bytes. + +#[test] +fn relay_mesh_model_id_none_for_non_mesh_config() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), Some("anthropic"), "")]; + let g = global(None, None); + + assert_eq!(resolve_effective_relay_mesh_model_id(&rec, &defs, &g), None); +} + +#[test] +fn relay_mesh_model_id_defaults_to_auto_when_model_blank() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, Some(RELAY_MESH_PROVIDER_ID), "")]; + let g = global(None, None); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g).as_deref(), + Some(RELAY_MESH_AUTO_MODEL_ID) + ); +} + +/// Switch-away regression (Wes finding 1): a linked definition that used to +/// be relay-mesh but was edited to another provider must NOT trigger the mesh +/// preflight — even though the record's own stale bytes still say +/// `provider: relay-mesh`. The old `relay_mesh_config(record)` sniff read +/// those stale record bytes directly and returned Some; the resolver-driven +/// decision must read the definition's CURRENT provider and return None. +#[test] +fn switch_away_from_relay_mesh_clears_preflight_despite_stale_record_bytes() { + let rec = record(Some("d1"), Some("auto"), Some(RELAY_MESH_PROVIDER_ID), None); + let defs = vec![definition( + "d1", + Some("claude-opus-4-6"), + Some("anthropic"), + "", + )]; + let g = global(None, None); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g), + None, + "definition switched away from relay-mesh — no mesh preflight should fire" + ); +} + +/// Global-inheritance regression (Wes finding 2): a blank linked definition +/// falling through to a relay-mesh GLOBAL default must trigger the mesh +/// preflight, even though the record carries `provider: None` and no legacy +/// env — all three of the old `relay_mesh_config` branches would miss here. +#[test] +fn global_relay_mesh_default_triggers_preflight_for_blank_definition() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, None, "")]; + let g = global(Some("Qwen3"), Some(RELAY_MESH_PROVIDER_ID)); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g).as_deref(), + Some("Qwen3"), + "global relay-mesh default must trigger the mesh preflight for a blank definition" + ); +} + +/// Symmetric case: a definition-less (legacy) instance inheriting the global +/// relay-mesh default must also trigger the preflight. +#[test] +fn global_relay_mesh_default_triggers_preflight_for_definition_less_instance() { + let rec = record(None, None, None, None); + let defs = vec![]; + let g = global(None, Some(RELAY_MESH_PROVIDER_ID)); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g).as_deref(), + Some(RELAY_MESH_AUTO_MODEL_ID) + ); +} + +/// Orphaned instance: no mesh preflight regardless of stale record bytes — +/// the caller's own orphan refusal is what matters, not a mesh bootstrap for +/// a start that will never happen. +#[test] +fn orphaned_instance_never_triggers_mesh_preflight() { + let rec = record( + Some("missing-def"), + Some("auto"), + Some(RELAY_MESH_PROVIDER_ID), + None, + ); + let g = global(None, None); + + assert_eq!(resolve_effective_relay_mesh_model_id(&rec, &[], &g), None); +} + +// ── Whitespace-provider parity (trim-mismatch regression, T1) ── +// +// The old spawn gate was `effective_provider.as_deref() == Some(RELAY_MESH_PROVIDER_ID)` +// — an exact compare — while `relay_mesh_model_id()` trims before matching. +// A stored provider of " relay-mesh " would make preflight fire while spawn +// skipped the mesh-env block. After the fix spawn derives its gate from +// `relay_mesh_model_id()` too, so the trim semantics are identical. +// +// These tests verify `relay_mesh_model_id()` returns Some for padded providers +// on every resolution path, confirming the helper that both callers now use +// handles whitespace correctly. + +/// Definition provider with leading/trailing whitespace — resolver preserves +/// the raw string, but `relay_mesh_model_id()` trims before matching, so the +/// mesh preflight fires correctly. +#[test] +fn whitespace_provider_in_definition_triggers_mesh_decision() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("qwen3"), Some(" relay-mesh "), "")]; + let g = global(None, None); + + let mesh_id = resolve_effective_relay_mesh_model_id(&rec, &defs, &g); + assert_eq!( + mesh_id.as_deref(), + Some("qwen3"), + "padded definition provider must be treated as relay-mesh by the shared helper" + ); +} + +/// Global provider with leading/trailing whitespace — same assertion for the +/// global-inherit path (blank definition falls through to padded global). +#[test] +fn whitespace_provider_in_global_triggers_mesh_decision() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, None, "")]; + let g = global(Some("qwen3"), Some("\trelay-mesh\t")); + + let mesh_id = resolve_effective_relay_mesh_model_id(&rec, &defs, &g); + assert_eq!( + mesh_id.as_deref(), + Some("qwen3"), + "padded global provider must be treated as relay-mesh by the shared helper" + ); +} + +// ── Legacy relay-mesh record compatibility (Wes review 3 on #1968) ── +// +// Two record generations shipped before `provider: "relay-mesh"` existed and +// are never rewritten on load, so they are still on disk: +// +// Class A (#798) — the four mesh preset env vars in `env_vars`, no typed +// field, no `provider` (the record had no `provider` field yet). +// Class B (#879) — the typed `relay_mesh` marker, still no `provider` field. +// +// Without the fallback both classes resolve to a non-mesh provider while their +// own stale env bytes still reach the child: a silent misroute, not a clean +// failure. The fallback lives in `resolve_definition_less` only — a linked +// instance's definition stays authoritative. + +/// Class A: env-only legacy record resolves mesh via the four-env preset. +#[test] +fn legacy_record_falls_back_to_env_sniff() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1/".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), + ), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "env-only legacy mesh record must still resolve to its served model" + ); +} + +/// Class A, oldest shipped record: both renamed sentinels in their original +/// spelling. The preset first wrote `SPROUT_AGENT_PROVIDER` (#798) and the +/// api-key value `sprout-mesh-local`; #971 and #960 renamed each in the same +/// Jun-11 window without migrating persisted `env_vars`, so this exact shape is +/// still on disk and must resolve mesh. +#[test] +fn legacy_record_falls_back_to_env_sniff_with_pre_rename_sentinels() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("SPROUT_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + "sprout-mesh-local".to_string(), + ), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "the oldest shipped mesh record carries both pre-rename sentinels and was never migrated" + ); +} + +/// Class A straddling the rename window: old provider key with the new api-key +/// value. Proves the two either/ors are independent rather than one either/or on +/// a bundled old/new pair. +#[test] +fn legacy_record_falls_back_to_env_sniff_with_mixed_rename_sentinels() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("SPROUT_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), + ), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "old provider key with new api key must resolve — the sentinels renamed independently" + ); +} + +/// The mirrored straddle: new provider key with the old api-key value. +#[test] +fn legacy_record_falls_back_to_env_sniff_with_new_provider_and_old_api_key() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + "sprout-mesh-local".to_string(), + ), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "new provider key with old api key must resolve too" + ); +} + +/// Both spellings present and disagreeing: the renamed key is authoritative, +/// so a record whose current provider env is non-mesh is not resurrected as +/// mesh by the stale `SPROUT_` leftover. +#[test] +fn legacy_env_sniff_prefers_renamed_provider_key_over_stale_old_key() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("BUZZ_AGENT_PROVIDER".to_string(), "anthropic".to_string()), + ("SPROUT_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), + ), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)), + None, + "the renamed key states current intent — a stale SPROUT_ key must not override it" + ); +} + +/// Class A negative: a user's own OpenAI-compatible provider on the same local +/// port is not Buzz's preset — the placeholder API key is the discriminator. +#[test] +fn legacy_env_sniff_ignores_user_openai_on_same_local_port() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ("OPENAI_COMPAT_API_KEY".to_string(), "real-key".to_string()), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)), + None, + "a real user key on the mesh port must not be mistaken for Buzz's preset" + ); +} + +/// Class B: typed marker with no `provider` resolves mesh, and its `model_ref` +/// wins over an unrelated global model so preflight and spawn agree. +#[test] +fn legacy_record_falls_back_to_typed_marker_without_provider() { + let mut rec = record(None, None, None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }); + + let cfg = match resolve_effective_config(&rec, &[], &global(Some("gpt-5"), Some("openai"))) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => { + panic!("definition-less is never orphaned") + } + }; + + assert_eq!(cfg.provider.value.as_deref(), Some(RELAY_MESH_PROVIDER_ID)); + assert_eq!( + cfg.relay_mesh_model_id().as_deref(), + Some("Qwen3"), + "typed marker must resolve mesh even with no provider on the record" + ); +} + +/// Class B with a blank `model_ref`: the marker is still the mesh signal, and +/// the model resolves to auto — same rule `apply_relay_mesh_env` applies. +#[test] +fn legacy_typed_marker_with_blank_model_resolves_auto() { + let mut rec = record(None, None, None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: " ".to_string(), + }); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some(RELAY_MESH_AUTO_MODEL_ID) + ); +} + +/// A definition-less record that was switched AWAY from mesh keeps its stale +/// marker and env bytes on disk. Its explicit `provider` states current intent, +/// so the legacy fallback must not resurrect mesh. +#[test] +fn definition_less_explicit_provider_wins_over_stale_legacy_mesh_bytes() { + let mut rec = record(None, Some("claude-opus-4-6"), Some("anthropic"), None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }); + rec.env_vars = BTreeMap::from([ + ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), + ), + ]); + + let cfg = match resolve_effective_config(&rec, &[], &global(None, None)) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => { + panic!("definition-less is never orphaned") + } + }; + + assert_eq!(cfg.provider.value.as_deref(), Some("anthropic")); + assert_eq!(cfg.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!( + cfg.relay_mesh_model_id(), + None, + "a switched-away legacy record must not be dragged back onto mesh" + ); +} + +/// The test that proves the fallback did not reintroduce the bug this PR +/// deletes: a LINKED record carrying both legacy mesh signals resolves purely +/// from its definition and never trips mesh. +#[test] +fn linked_record_ignores_legacy_mesh_marker_and_env() { + let mut rec = record(Some("d1"), Some("auto"), None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }); + rec.env_vars = BTreeMap::from([ + ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "http://127.0.0.1:9337/v1".to_string(), + ), + ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), + ), + ]); + let defs = vec![definition( + "d1", + Some("claude-opus-4-6"), + Some("anthropic"), + "", + )]; + + let cfg = match resolve_effective_config(&rec, &defs, &global(None, None)) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => panic!("definition exists"), + }; + + assert_eq!(cfg.provider.value.as_deref(), Some("anthropic")); + assert_eq!(cfg.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!( + cfg.relay_mesh_model_id(), + None, + "legacy record bytes must never influence a linked instance" + ); +} + +/// A linked instance whose definition inherits a blank provider from a blank +/// global must NOT pick up the record's legacy mesh bytes either — the linked +/// path has no legacy fallback at all, by construction. +#[test] +fn linked_record_with_legacy_bytes_inherits_global_not_mesh() { + let mut rec = record(Some("d1"), None, None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }); + let defs = vec![definition("d1", None, None, "")]; + + let cfg = match resolve_effective_config(&rec, &defs, &global(Some("gpt-5"), Some("openai"))) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => panic!("definition exists"), + }; + + assert_eq!(cfg.provider.value.as_deref(), Some("openai")); + assert_eq!(cfg.model.value.as_deref(), Some("gpt-5")); + assert_eq!(cfg.relay_mesh_model_id(), None); +} diff --git a/desktop/src-tauri/src/managed_agents/env_vars.rs b/desktop/src-tauri/src/managed_agents/env_vars.rs index 6318a5c838..c652dae427 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars.rs @@ -285,8 +285,12 @@ pub(crate) fn merged_user_env( /// Look up the live env map of `persona_id` within an already-loaded persona /// slice. Returns an empty map for standalone agents (`None`) and for links -/// to personas that no longer exist (an orphaned agent spawns from its own -/// overrides alone — same fallback the prompt/model resolution uses). +/// to personas that no longer exist. The latter is an orphaned instance, +/// which `spawn_agent_child` refuses before this is ever read at spawn time +/// (see `effective_config::resolve_effective_config`'s `OrphanedInstance` +/// arm via `require_resolved`) — an empty map here only matters for +/// non-spawn callers like readiness/hash that must still resolve +/// a slice for a record whose orphan status hasn't been checked yet. pub(crate) fn live_persona_env( personas: &[super::types::AgentDefinition], persona_id: Option<&str>, diff --git a/desktop/src-tauri/src/managed_agents/global_config/mod.rs b/desktop/src-tauri/src/managed_agents/global_config/mod.rs index 57993fc040..162f447981 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/mod.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/mod.rs @@ -1,13 +1,14 @@ //! Global agent configuration defaults. //! //! A single `global-agent-config.json` record that applies to ALL managed -//! agents. Per-agent config always wins; global provides the lowest -//! user-settable layer below persona. +//! agents. For a linked instance, the definition wins over global; for a +//! definition-less (legacy) instance, the record's own fields win over +//! global. Global is always the lowest user-settable layer. //! //! # Precedence (low → high) //! //! ```text -//! baked build env < GLOBAL < persona < per-agent < Buzz-identity +//! baked build env < GLOBAL < definition (linked) / instance (legacy) < Buzz-identity //! ``` //! //! # Semantics @@ -38,8 +39,10 @@ use crate::managed_agents::types::{AgentDefinition, ManagedAgentRecord}; /// so the config vocabulary is consistent across all three tiers. /// /// `env_vars` is the lowest user-settable env layer — global < persona < agent. -/// `provider` / `model` are fallback defaults: effective provider/model = -/// `agent → persona → global → None`. +/// `provider` / `model` are fallback defaults, resolved via +/// `effective_config::resolve_effective_config`: for a linked instance, +/// definition → global (the record's own `provider`/`model` bytes are never +/// consulted); for a definition-less instance, instance → global. #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] pub struct GlobalAgentConfig { /// Global env vars injected into ALL agents unconditionally. @@ -204,44 +207,28 @@ pub fn save_global_agent_config(app: &AppHandle, config: &GlobalAgentConfig) -> atomic_write_json_restricted(&path, &payload) } -/// Resolve the effective model and provider for an agent using the -/// precedence chain: `agent record → linked persona → global defaults → None`. +/// Resolve the effective model and provider for an agent. /// -/// This is the single source of truth used by readiness evaluation, spawn, -/// and deploy-payload construction. All three paths must use this function so -/// they agree on what model/provider the agent will actually run with. +/// Delegates to `effective_config::resolve_effective_config` which enforces +/// definition-authoritative semantics for linked instances: +/// - **Linked:** definition → global. Record bytes are never consulted. +/// - **Definition-less:** instance → global. +/// - **Orphaned:** returns `(None, None)`. This function is a display/ +/// readiness/hash convenience, not the spawn gate — an orphan must never +/// reach a real spawn regardless of what this returns, because +/// `spawn_agent_child` refuses orphans itself via +/// `effective_config::resolve_effective_config(...).require_resolved()` +/// before resolving model/provider/prompt for the process env. /// -/// # Arguments -/// * `record` — the `ManagedAgentRecord` (may have `None` for model/provider) -/// * `personas` — all current persona records (looked up by `record.persona_id`) -/// * `global` — global agent config defaults -/// -/// # Returns -/// `(effective_model, effective_provider)` — both `Option<&str>`. -pub(crate) fn resolve_effective_model_provider<'a>( - record: &'a ManagedAgentRecord, - personas: &'a [AgentDefinition], - global: &'a GlobalAgentConfig, -) -> (Option<&'a str>, Option<&'a str>) { - let (persona_model, persona_provider) = record - .persona_id - .as_deref() - .and_then(|pid| personas.iter().find(|p| p.id == pid)) - .map(|p| (p.model.as_deref(), p.provider.as_deref())) - .unwrap_or((None, None)); - - let effective_model = record - .model - .as_deref() - .or(persona_model) - .or(global.model.as_deref()); - let effective_provider = record - .provider - .as_deref() - .or(persona_provider) - .or(global.provider.as_deref()); - - (effective_model, effective_provider) +/// Returns owned `String`s because the effective value may come from a +/// tier that outlives none of the input borrows (e.g. global config). +pub(crate) fn resolve_effective_model_provider( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> (Option, Option) { + super::effective_config::resolve_effective_model_provider_pair(record, personas, global) + .unwrap_or((None, None)) } #[cfg(test)] diff --git a/desktop/src-tauri/src/managed_agents/global_config/tests.rs b/desktop/src-tauri/src/managed_agents/global_config/tests.rs index 6eb4338034..33b93d8a52 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/tests.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/tests.rs @@ -376,15 +376,14 @@ fn persona(id: &str, model: Option<&str>, provider: Option<&str>) -> AgentDefini } } -/// Tier 1 — agent record wins: record has explicit model/provider; they must -/// outrank both the linked persona and the global defaults. Fails against any -/// implementation that prefers global or persona over the record. +/// Linked instance: definition (persona) wins — stale record bytes are +/// ignored. This is the core model-inheritance fix. #[test] -fn resolve_agent_record_wins_over_persona_and_global() { +fn resolve_definition_wins_over_stale_record_for_linked_instance() { let mut record = bare_record(); record.persona_id = Some("p1".to_string()); - record.model = Some("record-model".to_string()); - record.provider = Some("record-provider".to_string()); + record.model = Some("stale-record-model".to_string()); + record.provider = Some("stale-record-provider".to_string()); let personas = vec![persona( "p1", Some("persona-model"), @@ -398,11 +397,15 @@ fn resolve_agent_record_wins_over_persona_and_global() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); - assert_eq!(model, Some("record-model"), "record model must win"); assert_eq!( - provider, - Some("record-provider"), - "record provider must win" + model.as_deref(), + Some("persona-model"), + "definition model must win over stale record" + ); + assert_eq!( + provider.as_deref(), + Some("persona-provider"), + "definition provider must win over stale record" ); } @@ -428,12 +431,12 @@ fn resolve_persona_fallback_when_record_has_none() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); assert_eq!( - model, + model.as_deref(), Some("persona-model"), "persona model must be used when record has none" ); assert_eq!( - provider, + provider.as_deref(), Some("persona-provider"), "persona provider must be used when record has none" ); @@ -458,12 +461,12 @@ fn resolve_global_fallback_when_record_and_persona_have_none() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); assert_eq!( - model, + model.as_deref(), Some("global-model"), "global model must be used when record and persona have none" ); assert_eq!( - provider, + provider.as_deref(), Some("global-provider"), "global provider must be used when record and persona have none" ); @@ -519,8 +522,8 @@ fn resolve_global_fallback_when_no_persona_linked() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); - assert_eq!(model, Some("global-model")); - assert_eq!(provider, Some("global-provider")); + assert_eq!(model.as_deref(), Some("global-model")); + assert_eq!(provider.as_deref(), Some("global-provider")); } /// All-None: no source provides model/provider → both must be None. @@ -543,18 +546,15 @@ fn resolve_all_none_when_no_source_provides_values() { ); } -/// Partial tier — record has model but not provider; persona has provider but -/// not model; global has both. Each field resolves independently through the -/// three-tier chain. +/// Each field resolves independently: definition model=None → global fills +/// model; definition has provider → definition wins for provider. Stale +/// record bytes are ignored for linked instances. #[test] fn resolve_each_field_resolves_independently_through_tiers() { let mut record = bare_record(); record.persona_id = Some("p1".to_string()); - record.model = Some("record-model".to_string()); - // record.provider = None → falls through to persona + record.model = Some("stale-record-model".to_string()); let personas = vec![persona("p1", None, Some("persona-provider"))]; - // persona.model = None → global fills model if record also had none, but - // record has model here so global is not needed for model. let global = GlobalAgentConfig { model: Some("global-model".to_string()), provider: Some("global-provider".to_string()), @@ -563,11 +563,15 @@ fn resolve_each_field_resolves_independently_through_tiers() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); - assert_eq!(model, Some("record-model"), "record wins for model"); assert_eq!( - provider, + model.as_deref(), + Some("global-model"), + "definition model=None → global fills model; stale record ignored" + ); + assert_eq!( + provider.as_deref(), Some("persona-provider"), - "persona wins for provider when record has none" + "definition provider wins" ); } diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 66f3d882b9..d213142352 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -8,6 +8,7 @@ pub(crate) use agent_env::{ mod backend; pub(crate) mod config_bridge; mod discovery; +pub(crate) mod effective_config; mod env_vars; pub(crate) mod git_bash; pub(crate) mod global_config; diff --git a/desktop/src-tauri/src/managed_agents/persona_events.rs b/desktop/src-tauri/src/managed_agents/persona_events.rs index 2033003bc8..5b62615a8c 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events.rs @@ -359,10 +359,14 @@ pub fn persona_event_content(record: &AgentDefinition) -> PersonaEventContent { } /// A persona's spawn-relevant config, pinned onto a `ManagedAgentRecord` at -/// create time. After the snapshot, spawn and deploy read these fields off the -/// record and never the live persona, so an agent stays pinned to the config -/// it was created with — restart reuses the snapshot, delete+respawn rewrites -/// it. +/// create time for display/backward-compat purposes. For a linked instance, +/// spawn and deploy do NOT read these snapshotted fields — they resolve +/// model/provider/prompt live from the definition on every spawn (see +/// `effective_config::resolve_effective_config`), so a definition edit +/// propagates on the next restart without delete+respawn. The snapshot still +/// matters for `runtime` (materialized per B5, no live-read path yet) and for +/// `persona_source_version`, the drift basis the Agents menu compares against +/// the definition's current content hash. pub struct PersonaSnapshot { pub system_prompt: Option, pub model: Option, @@ -377,24 +381,6 @@ pub struct PersonaSnapshot { pub source_version: String, } -/// Apply persona-wins-when-set precedence for a single optional string field. -/// -/// Returns the persona's value when it is non-`None` and non-whitespace-only; -/// otherwise falls back to the record's value with the same blank filter applied. -/// Returns `None` only when both are blank — a genuinely unconfigured field stays -/// unconfigured. -/// -/// This is the single source of truth for the precedence rule used by -/// `persona_snapshot_with_agent_config_fallback`, `build_deploy_payload`, and -/// `resolve_effective_prompt_model_provider` so the three paths cannot drift. -pub fn persona_field_with_record_fallback( - persona_value: Option<&str>, - record_value: Option<&str>, -) -> Option { - let non_blank = |v: Option<&str>| v.filter(|s| !s.trim().is_empty()).map(str::to_owned); - non_blank(persona_value).or_else(|| non_blank(record_value)) -} - /// Build the pinned snapshot for an agent created from `persona`. /// /// The persona's `system_prompt` is always present, so it is wrapped in @@ -411,51 +397,14 @@ pub fn persona_snapshot(persona: &AgentDefinition) -> PersonaSnapshot { } } -/// Build the pinned snapshot for an **existing** agent record being re-snapshotted -/// from its linked persona (on spawn or app-launch restore). +/// Re-pin `record` to `persona`: build a snapshot via [`persona_snapshot`] +/// and mirror it onto the record — the definition quad +/// (`system_prompt`/`model`/`provider`/`runtime`), the env-override +/// self-heal, and the `persona_source_version` drift basis. /// -/// Precedence rule: when the persona sets `model` or `provider` (non-`None`, non-empty), -/// the persona wins — this is the expected inheritance. When the persona leaves -/// these fields blank (`None` or empty string), the agent record's own values are -/// preserved instead. This prevents a persona with no configured model/provider from -/// clobbering a value the user already set on the agent, which would trap the agent -/// in a permanent "needs configuration" loop that users cannot escape. -/// -/// `source_version` is always updated to the current persona content hash so the -/// drift badge clears correctly even when model/provider are not touched. -/// -/// Env vars are not part of the snapshot: `record.env_vars` (agent overrides) -/// is left untouched and the live persona env is merged underneath at read time. -/// -/// The two fields (`model`, `provider`) are independent: a persona that sets only -/// `model` wins on `model` while the agent's `provider` is preserved, and vice versa. -pub fn persona_snapshot_with_agent_config_fallback( - persona: &AgentDefinition, - current_agent_model: Option<&str>, - current_agent_provider: Option<&str>, -) -> PersonaSnapshot { - // Delegate system_prompt and source_version to persona_snapshot so future - // PersonaSnapshot field additions stay automatically consistent. - let base = persona_snapshot(persona); - - // Apply the shared precedence rule: persona wins when non-blank, else - // the agent record's value is preserved so a configured agent stays configured. - let model = persona_field_with_record_fallback(base.model.as_deref(), current_agent_model); - let provider = - persona_field_with_record_fallback(base.provider.as_deref(), current_agent_provider); - - PersonaSnapshot { - model, - provider, - ..base - } -} - -/// Re-pin `record` to `persona`: build the snapshot (via -/// [`persona_snapshot_with_agent_config_fallback`], so blank persona -/// `model`/`provider` preserve the record's own values) and mirror it onto the -/// record — the definition quad (`system_prompt`/`model`/`provider`/`runtime`), -/// the env-override self-heal, and the `persona_source_version` drift basis. +/// Definition-authoritative: blank definition model/provider produce `None` +/// on the record. The effective-config resolver falls through to the global +/// default at read time; stale materialized record bytes are never preserved. /// /// This is the single apply used by every snapshot-apply site: the spawn /// re-pin (`start_local_agent_with_preflight`), the launch backfill and @@ -467,11 +416,7 @@ pub fn persona_snapshot_with_agent_config_fallback( /// caller's concern, and `spawn_config_hash` (which applies this to a clone) /// must stay pure. pub fn apply_persona_snapshot(record: &mut ManagedAgentRecord, persona: &AgentDefinition) { - let snapshot = persona_snapshot_with_agent_config_fallback( - persona, - record.model.as_deref(), // fallback: record.model - record.provider.as_deref(), // fallback: record.provider - ); + let snapshot = persona_snapshot(persona); if let Some(prompt) = snapshot.system_prompt { record.system_prompt = Some(prompt); } @@ -507,5 +452,33 @@ pub fn apply_persona_snapshot(record: &mut ManagedAgentRecord, persona: &AgentDe .retain(|k, v| persona.env_vars.get(k) != Some(v)); record.persona_source_version = Some(snapshot.source_version); } + +/// Preview what `record` would look like immediately after the start/restore +/// paths re-pin it to its linked persona, without mutating `record` itself. +/// +/// Every decision made ahead of the real re-pin — the relay-mesh preflight in +/// `start_local_agent_with_preflight`, the restart-badge hash in +/// `spawn_config_hash` — needs to reason about spawn-time state, not +/// pre-snapshot bytes, so a persona edit that flips a field (e.g. `provider` +/// to/from relay-mesh) between saves is reflected in the decision instead of +/// the stale value the real [`apply_persona_snapshot`] is about to overwrite +/// anyway. Idempotent: applying it to an already-current record is a no-op, +/// so the spawn-time stamp and later recomputes agree when nothing changed. +/// +/// Orphaned records (persona deleted) pass through unchanged: the caller's +/// own orphan handling — refusing to spawn, hashing as `(None, None, None)` +/// — runs on the real record downstream, not on this preview. +pub fn preview_prospective_persona_snapshot( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], +) -> ManagedAgentRecord { + let mut preview = record.clone(); + if let Some(persona_id) = preview.persona_id.clone() { + if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { + apply_persona_snapshot(&mut preview, persona); + } + } + preview +} #[cfg(test)] mod tests; diff --git a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs index d62cc5b4bb..27d3b0ce06 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs @@ -1,4 +1,141 @@ use super::*; +use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; + +/// A linked instance record with no persona-derived fields set yet — the +/// state right after creation, before any snapshot apply. +fn sample_record() -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: "p".repeat(64), + name: "agent".into(), + persona_id: Some("test-persona".into()), + private_key_nsec: "nsec1fake".into(), + auth_tag: None, + relay_url: "ws://localhost:3000".into(), + avatar_url: None, + acp_command: "buzz-acp".into(), + agent_command: "goose".into(), + agent_command_override: None, + agent_args: vec![], + mcp_command: String::new(), + turn_timeout_seconds: 320, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + provider: None, + persona_source_version: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: true, + runtime_pid: None, + backend: BackendKind::Local, + backend_agent_id: None, + provider_binary_path: None, + team_id: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "now".into(), + updated_at: "now".into(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + definition_respond_to: None, + definition_respond_to_allowlist: Vec::new(), + definition_parallelism: None, + relay_mesh: None, + } +} + +// ── preview_prospective_persona_snapshot (Finding 4: relay-mesh preflight +// ordering) ─────────────────────────────────────────────────────────────── + +/// Regression for the relay-mesh preflight-ordering defect: the preflight +/// used to check `ensure_relay_mesh_for_record` against the record's stale +/// pre-snapshot bytes, so a persona edit that flips `provider` to +/// `relay-mesh` between saves would not trigger the mesh preflight on the +/// very next start — only on the restart after that, once the real +/// `apply_persona_snapshot` had already landed. The preview must reflect the +/// same prospective re-snapshot the real spawn path applies. +#[test] +fn preview_reflects_persona_edit_flipping_provider_into_relay_mesh() { + let record = sample_record(); // provider: None (not relay-mesh) + let mut persona = sample_persona(); + persona.id = "test-persona".into(); + persona.provider = Some("relay-mesh".into()); + persona.model = Some("auto".into()); + + let preview = preview_prospective_persona_snapshot(&record, &[persona]); + + assert_eq!( + preview.provider.as_deref(), + Some("relay-mesh"), + "preview must carry the persona's current provider, not the record's stale None" + ); + assert_eq!(preview.model.as_deref(), Some("auto")); +} + +/// Preview reflects a persona edit flipping AWAY from relay-mesh too — +/// symmetric with the into-mesh case above. +#[test] +fn preview_reflects_persona_edit_flipping_provider_out_of_relay_mesh() { + let mut record = sample_record(); + record.provider = Some("relay-mesh".into()); + record.model = Some("auto".into()); + let mut persona = sample_persona(); + persona.id = "test-persona".into(); + persona.provider = Some("anthropic".into()); + persona.model = Some("claude-opus-4".into()); + + let preview = preview_prospective_persona_snapshot(&record, &[persona]); + + assert_eq!(preview.provider.as_deref(), Some("anthropic")); + assert_eq!(preview.model.as_deref(), Some("claude-opus-4")); +} + +/// The preview must not mutate the record passed in — callers (the mesh +/// preflight) need the original bytes intact for the real snapshot apply +/// that follows. +#[test] +fn preview_does_not_mutate_input_record() { + let record = sample_record(); + let original = record.clone(); + let mut persona = sample_persona(); + persona.id = "test-persona".into(); + persona.provider = Some("relay-mesh".into()); + + let _ = preview_prospective_persona_snapshot(&record, &[persona]); + + assert_eq!(record.provider, original.provider); + assert_eq!(record.model, original.model); +} + +/// Orphaned instance (persona deleted): the preview must pass the record +/// through unchanged so downstream orphan handling (refuse to spawn) sees +/// the same bytes it always would. +#[test] +fn preview_passes_through_unchanged_when_persona_missing() { + let mut record = sample_record(); + record.provider = Some("anthropic".into()); + record.persona_id = Some("deleted-persona".into()); + + let preview = preview_prospective_persona_snapshot(&record, &[]); + + assert_eq!(preview.provider.as_deref(), Some("anthropic")); + assert_eq!(preview.persona_id.as_deref(), Some("deleted-persona")); +} fn sample_persona() -> AgentDefinition { AgentDefinition { @@ -437,43 +574,6 @@ fn persona_content_hash_changes_on_edit() { ); } -// ── persona_field_with_record_fallback ──────────────────────────────────── - -#[test] -fn field_fallback_persona_present_wins() { - assert_eq!( - persona_field_with_record_fallback(Some("persona-model"), Some("record-model")), - Some("persona-model".to_owned()), - ); -} - -#[test] -fn field_fallback_persona_blank_uses_record() { - assert_eq!( - persona_field_with_record_fallback(None, Some("record-model")), - Some("record-model".to_owned()), - ); - assert_eq!( - persona_field_with_record_fallback(Some(" "), Some("record-model")), - Some("record-model".to_owned()), - ); -} - -#[test] -fn field_fallback_both_blank_is_none() { - assert_eq!(persona_field_with_record_fallback(None, None), None); - assert_eq!(persona_field_with_record_fallback(Some(""), Some("")), None); -} - -#[test] -fn field_fallback_record_blank_is_none() { - assert_eq!( - persona_field_with_record_fallback(None, Some(" ")), - None, - "whitespace-only record value must also be treated as blank" - ); -} - // ── PersonaSnapshot.runtime ─────────────────────────────────────────────── /// (b) The snapshot carries the persona's runtime VERBATIM — including None, @@ -484,7 +584,7 @@ fn field_fallback_record_blank_is_none() { #[test] fn snapshot_runtime_verbatim_from_persona() { let persona = sample_persona(); // runtime = Some("goose") - let snap = persona_snapshot_with_agent_config_fallback(&persona, Some("gpt-4"), Some("openai")); + let snap = persona_snapshot(&persona); assert_eq!( snap.runtime.as_deref(), Some("goose"), @@ -493,15 +593,14 @@ fn snapshot_runtime_verbatim_from_persona() { let mut no_runtime = sample_persona(); no_runtime.runtime = None; - let snap = - persona_snapshot_with_agent_config_fallback(&no_runtime, Some("gpt-4"), Some("openai")); + let snap = persona_snapshot(&no_runtime); assert_eq!( snap.runtime, None, "persona runtime None must produce None snapshot (clears stale materialized value)" ); } -// ── persona_snapshot_with_agent_config_fallback ──────────────────────────── +// ── persona_snapshot (definition-authoritative) ────────────────────────── /// Helper: a persona with no model/provider configured. fn blank_model_persona() -> AgentDefinition { @@ -512,25 +611,22 @@ fn blank_model_persona() -> AgentDefinition { } } -/// (a) Persona leaves model/provider blank, agent record has values → -/// record values preserved AND source_version still updated to current hash. +/// Definition-authoritative: blank definition model/provider → snapshot is +/// None. The effective-config resolver handles global fallback at read time. #[test] -fn fallback_preserves_record_values_when_persona_blank() { +fn blank_definition_clears_model_provider() { let persona = blank_model_persona(); let expected_version = persona_content_hash(&persona_event_content(&persona)); - let snapshot = - persona_snapshot_with_agent_config_fallback(&persona, Some("gpt-4o"), Some("openai")); + let snapshot = persona_snapshot(&persona); - assert_eq!( - snapshot.model.as_deref(), - Some("gpt-4o"), - "blank persona model must fall back to agent record value" + assert!( + snapshot.model.is_none(), + "blank definition model must produce None" ); - assert_eq!( - snapshot.provider.as_deref(), - Some("openai"), - "blank persona provider must fall back to agent record value" + assert!( + snapshot.provider.is_none(), + "blank definition provider must produce None" ); assert_eq!( snapshot.source_version, expected_version, @@ -538,124 +634,102 @@ fn fallback_preserves_record_values_when_persona_blank() { ); } -/// (b) Persona has model/provider set → persona wins over agent record. +/// Persona has model/provider set → snapshot carries them verbatim. #[test] -fn fallback_persona_wins_when_set() { +fn snapshot_carries_persona_model_provider() { let persona = sample_persona(); // has model=Some("claude-opus-4"), provider=Some("anthropic") - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("gpt-4o"), // agent had a different model - Some("openai"), // agent had a different provider - ); + let snapshot = persona_snapshot(&persona); assert_eq!( snapshot.model.as_deref(), Some("claude-opus-4"), - "persona model must win when persona has a value" + "persona model must be carried verbatim" ); assert_eq!( snapshot.provider.as_deref(), Some("anthropic"), - "persona provider must win when persona has a value" + "persona provider must be carried verbatim" ); } -/// (c) Both blank → snapshot keeps None; a genuinely unconfigured agent +/// Both blank → snapshot keeps None; a genuinely unconfigured agent /// stays unconfigured (no fabricated values). #[test] -fn fallback_both_blank_stays_none() { +fn both_blank_stays_none() { let persona = blank_model_persona(); - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, None, // agent also has no model - None, // agent also has no provider - ); + let snapshot = persona_snapshot(&persona); assert!( snapshot.model.is_none(), - "neither persona nor agent has model — snapshot must be None" + "blank persona model → snapshot None" ); assert!( snapshot.provider.is_none(), - "neither persona nor agent has provider — snapshot must be None" + "blank persona provider → snapshot None" ); } -/// Whitespace-only values on the persona are treated as blank; agent -/// fallback applies. +/// Whitespace-only values on the definition are preserved verbatim in the +/// snapshot. The effective-config resolver treats whitespace as blank. #[test] -fn fallback_treats_whitespace_only_persona_value_as_blank() { +fn whitespace_only_definition_preserved_verbatim() { let mut persona = sample_persona(); persona.model = Some(" ".to_string()); persona.provider = Some("\t".to_string()); - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("claude-opus-4"), - Some("anthropic"), - ); + let snapshot = persona_snapshot(&persona); assert_eq!( snapshot.model.as_deref(), - Some("claude-opus-4"), - "whitespace-only persona model must be treated as blank" + Some(" "), + "whitespace-only model is preserved verbatim in snapshot" ); assert_eq!( snapshot.provider.as_deref(), - Some("anthropic"), - "whitespace-only persona provider must be treated as blank" + Some("\t"), + "whitespace-only provider is preserved verbatim in snapshot" ); } -/// Cross-field independence: persona sets model but not provider → model -/// comes from persona, provider falls back to the record. This is the -/// practically common case (model-only personas). +/// Cross-field independence: definition sets model but not provider → model +/// comes from definition, provider is None (global fallback at read time). #[test] -fn fallback_persona_model_set_provider_blank_uses_record_provider() { - let mut persona = sample_persona(); // model=Some("claude-opus-4"), provider=Some("anthropic") - persona.provider = None; // blank provider on persona +fn definition_model_set_provider_blank_produces_none_provider() { + let mut persona = sample_persona(); + persona.provider = None; - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("gpt-4o"), // record model (should be overridden by persona) - Some("openai"), // record provider (should be preserved) - ); + let snapshot = persona_snapshot(&persona); assert_eq!( snapshot.model.as_deref(), Some("claude-opus-4"), - "persona model must win when persona has a value" + "definition model must be used" ); - assert_eq!( - snapshot.provider.as_deref(), - Some("openai"), - "record provider must be used when persona provider is blank" + assert!( + snapshot.provider.is_none(), + "definition provider=None → snapshot None" ); } -/// Inverse: persona sets provider but not model → provider comes from -/// persona, model falls back to the record. +/// Definition provider set, model blank → model=None (global fallback at +/// read time), provider from definition. #[test] -fn fallback_persona_provider_set_model_blank_uses_record_model() { - let mut persona = sample_persona(); // model=Some("claude-opus-4"), provider=Some("anthropic") - persona.model = None; // blank model on persona +fn definition_provider_set_model_blank_produces_none_model() { + let mut persona = sample_persona(); + persona.model = None; - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("gpt-4o"), // record model (should be preserved) - Some("openai"), // record provider (should be overridden by persona) - ); + let snapshot = persona_snapshot(&persona); - assert_eq!( - snapshot.model.as_deref(), - Some("gpt-4o"), - "record model must be used when persona model is blank" + assert!( + snapshot.model.is_none(), + "definition model=None → snapshot None" ); assert_eq!( snapshot.provider.as_deref(), Some("anthropic"), - "persona provider must win when persona has a value" + "definition provider must be used" ); } diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index c04dfa88a4..fb45cc9a4e 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -98,11 +98,6 @@ pub(crate) fn resolve_effective_agent_env( // Layer 1: baked build defaults (floor — internal builds only; OSS = empty). let mut env = baked_build_env(); - // Layer 2: runtime metadata env vars (model / provider keys derived from - // the record's structured fields, with global as fallback). - // - // Uses the shared resolver to guarantee readiness and spawn agree on the - // effective model/provider: agent → persona → global → None. let (effective_model, effective_provider) = super::global_config::resolve_effective_model_provider(record, personas, global); @@ -111,8 +106,8 @@ pub(crate) fn resolve_effective_agent_env( rt.model_env_var, rt.provider_env_var, rt.provider_locked, - effective_model, - effective_provider, + effective_model.as_deref(), + effective_provider.as_deref(), ) { env.insert(key.to_string(), value.to_string()); } @@ -138,7 +133,11 @@ pub(crate) fn resolve_effective_agent_env( // Buzz shared compute is a native Buzz provider. Translate it to buzz-agent's // OpenAI-compatible transport only in the effective runtime environment. #[cfg(feature = "mesh-llm")] - super::apply_relay_mesh_env(&mut env, effective_provider, effective_model); + super::apply_relay_mesh_env( + &mut env, + effective_provider.as_deref(), + effective_model.as_deref(), + ); EffectiveAgentEnv { env, diff --git a/desktop/src-tauri/src/managed_agents/relay_mesh.rs b/desktop/src-tauri/src/managed_agents/relay_mesh.rs index a47122df80..934155e881 100644 --- a/desktop/src-tauri/src/managed_agents/relay_mesh.rs +++ b/desktop/src-tauri/src/managed_agents/relay_mesh.rs @@ -1,6 +1,3 @@ -#[cfg(feature = "mesh-llm")] -use super::{ManagedAgentRecord, RelayMeshConfig}; - pub const RELAY_MESH_API_BASE_URL: &str = "http://127.0.0.1:9337/v1"; pub const RELAY_MESH_API_KEY_PLACEHOLDER: &str = "buzz-mesh-local"; pub const RELAY_MESH_PROVIDER_ID: &str = "relay-mesh"; @@ -46,146 +43,11 @@ pub fn apply_relay_mesh_env( env.insert("BUZZ_AGENT_THINKING_EFFORT".to_string(), "none".to_string()); } -/// Resolve a record's relay-mesh config, typed field first. -/// -/// Source of truth is the typed `record.relay_mesh` field. For records saved -/// before that field existed, fall back to detecting the relay-mesh preset -/// from `env_vars` (the legacy discriminator). New records carry the typed -/// field and need no env-var sniffing at all. -#[cfg(feature = "mesh-llm")] -pub fn relay_mesh_config(record: &ManagedAgentRecord) -> Option { - if record.provider.as_deref() == Some(RELAY_MESH_PROVIDER_ID) { - let model_ref = record - .model - .as_deref() - .map(str::trim) - .filter(|value| !value.is_empty()) - .unwrap_or(RELAY_MESH_AUTO_MODEL_ID) - .to_string(); - return Some(RelayMeshConfig { model_ref }); - } - if let Some(config) = &record.relay_mesh { - return Some(config.clone()); - } - relay_mesh_model_id_from_env(record).map(|model_ref| RelayMeshConfig { model_ref }) -} - -/// Returns the relay-mesh model id for agents whose provider env points at the -/// local mesh client endpoint created by Buzz's relay-mesh preset. -/// -/// Prefer [`relay_mesh_config`]; this remains as a convenience for call sites -/// that only need the model id. -#[cfg(feature = "mesh-llm")] -pub fn relay_mesh_model_id(record: &ManagedAgentRecord) -> Option { - relay_mesh_config(record).map(|config| config.model_ref) -} - -/// Legacy env-var discriminator: detects the relay-mesh preset purely from the -/// four preset env vars. Used as a fallback for records saved before the typed -/// `relay_mesh` field existed. -#[cfg(feature = "mesh-llm")] -fn relay_mesh_model_id_from_env(record: &ManagedAgentRecord) -> Option { - let base_url = record.env_vars.get("OPENAI_COMPAT_BASE_URL")?.trim(); - if base_url.trim_end_matches('/') != RELAY_MESH_API_BASE_URL { - return None; - } - let provider = record.env_vars.get("BUZZ_AGENT_PROVIDER")?.trim(); - if provider != "openai" { - return None; - } - let api_key = record.env_vars.get("OPENAI_COMPAT_API_KEY")?.trim(); - if api_key != RELAY_MESH_API_KEY_PLACEHOLDER { - return None; - } - record - .env_vars - .get("OPENAI_COMPAT_MODEL") - .map(|value| value.trim()) - .filter(|value| !value.is_empty()) - .map(str::to_string) -} - #[cfg(all(test, feature = "mesh-llm"))] mod tests { use std::collections::BTreeMap; use super::*; - use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; - - fn fixture() -> ManagedAgentRecord { - ManagedAgentRecord { - pubkey: "p".into(), - name: "n".into(), - persona_id: None, - private_key_nsec: "nsec1fake".into(), - auth_tag: Some("tag".into()), - relay_url: "ws://localhost:3000".into(), - avatar_url: None, - acp_command: "buzz-acp".into(), - agent_command: "goose".into(), - agent_command_override: None, - agent_args: vec![], - mcp_command: String::new(), - turn_timeout_seconds: 320, - idle_timeout_seconds: None, - max_turn_duration_seconds: None, - parallelism: 1, - system_prompt: None, - model: None, - env_vars: BTreeMap::new(), - start_on_app_launch: false, - auto_restart_on_config_change: true, - runtime_pid: None, - backend: BackendKind::Local, - backend_agent_id: None, - provider_binary_path: None, - team_id: None, - persona_team_dir: None, - persona_name_in_team: None, - persona_source_version: None, - provider: None, - created_at: "now".into(), - updated_at: "now".into(), - last_started_at: None, - last_stopped_at: None, - last_exit_code: None, - last_error: None, - last_error_code: None, - respond_to: RespondTo::OwnerOnly, - respond_to_allowlist: vec![], - display_name: None, - slug: None, - runtime: None, - name_pool: Vec::new(), - is_builtin: false, - is_active: true, - source_team: None, - source_team_persona_slug: None, - definition_respond_to: None, - definition_respond_to_allowlist: Vec::new(), - definition_parallelism: None, - relay_mesh: None, - } - } - - #[test] - fn relay_mesh_model_id_detects_mesh_preset_env() { - let mut rec = fixture(); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1/".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), - ( - "OPENAI_COMPAT_API_KEY".to_string(), - RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), - ), - ]); - - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("Qwen3")); - } #[test] fn native_provider_uses_context_safe_non_reasoning_budget() { @@ -205,95 +67,4 @@ mod tests { Some("none") ); } - - #[test] - fn relay_mesh_model_id_ignores_non_mesh_openai_env() { - let mut rec = fixture(); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "https://api.openai.com/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "gpt-5".to_string()), - ]); - - assert_eq!(relay_mesh_model_id(&rec), None); - } - - #[test] - fn relay_mesh_model_id_ignores_user_openai_on_same_local_port() { - let mut rec = fixture(); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), - ("OPENAI_COMPAT_API_KEY".to_string(), "real-key".to_string()), - ]); - - assert_eq!(relay_mesh_model_id(&rec), None); - } - - #[test] - fn native_provider_fields_are_authoritative() { - // The whole point: a typed record needs no env-var sniffing to be - // recognized as a relay-mesh agent. - let mut rec = fixture(); - rec.provider = Some(RELAY_MESH_PROVIDER_ID.to_string()); - rec.model = Some("Qwen3".to_string()); - assert!(rec.env_vars.is_empty()); - assert_eq!( - relay_mesh_config(&rec), - Some(RelayMeshConfig { - model_ref: "Qwen3".to_string() - }) - ); - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("Qwen3")); - } - - #[test] - fn native_provider_fields_win_over_legacy_config() { - let mut rec = fixture(); - rec.provider = Some(RELAY_MESH_PROVIDER_ID.to_string()); - rec.model = Some("native-model".to_string()); - rec.relay_mesh = Some(RelayMeshConfig { - model_ref: "typed-model".to_string(), - }); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "env-model".to_string()), - ( - "OPENAI_COMPAT_API_KEY".to_string(), - RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), - ), - ]); - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("native-model")); - } - - #[test] - fn legacy_record_falls_back_to_env_sniff() { - // Records saved before the typed field still resolve via env vars. - let mut rec = fixture(); - rec.relay_mesh = None; - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), - ( - "OPENAI_COMPAT_API_KEY".to_string(), - RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), - ), - ]); - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("Qwen3")); - } } diff --git a/desktop/src-tauri/src/managed_agents/restore.rs b/desktop/src-tauri/src/managed_agents/restore.rs index fab481e12b..1910620159 100644 --- a/desktop/src-tauri/src/managed_agents/restore.rs +++ b/desktop/src-tauri/src/managed_agents/restore.rs @@ -1,5 +1,3 @@ -#[cfg(feature = "mesh-llm")] -use super::relay_mesh_model_id; use super::{ find_managed_agent_mut, kill_stale_tracked_processes, load_managed_agents, load_personas, save_managed_agents, spawn_agent_child, sync_managed_agent_processes, BackendKind, @@ -36,10 +34,9 @@ type AgentSpawnResult = (String, SpawnOutcome); /// `model`/`provider` were clobbered by the old unconditional snapshot code before /// this fix — are skipped here; they self-heal on the next manual start via the /// start-path re-snapshot in `start_local_agent_with_preflight`. -/// If the linked persona is gone, we log loudly and leave the snapshot empty — -/// the record's own `system_prompt`/`model` (possibly empty for persona-created -/// agents) is then all the config that remains, which is the same fallback an -/// orphaned agent already gets. +/// If the linked persona is gone, we log loudly and leave the record untouched — +/// it stays orphaned and `spawn_agent_child` refuses to start it (see +/// `effective_config::resolve_effective_config`'s `OrphanedInstance` arm). pub fn backfill_persona_snapshots(app: &tauri::AppHandle) -> Result<(), String> { let state = app.state::(); let _store_guard = state @@ -66,7 +63,7 @@ pub fn backfill_persona_snapshots(app: &tauri::AppHandle) -> Result<(), String> } let Some(persona) = personas.iter().find(|p| p.id == persona_id) else { eprintln!( - "buzz-desktop: persona-snapshot backfill: agent {} links persona {persona_id} which no longer exists; leaving snapshot empty — it will spawn from its record fields", + "buzz-desktop: persona-snapshot backfill: agent {} links persona {persona_id} which no longer exists; leaving it orphaned — spawn will refuse it", record.pubkey ); continue; @@ -206,6 +203,9 @@ pub async fn restore_managed_agents_on_launch( continue; }; let Some(persona) = personas_for_snapshot.iter().find(|p| p.id == persona_id) else { + // Orphaned: no current persona to re-snapshot from. Leave the + // record as-is — `spawn_agent_child` (Phase B below) refuses to + // spawn it and Phase C persists the refusal to `last_error`. continue; }; super::persona_events::apply_persona_snapshot(record, persona); @@ -240,16 +240,26 @@ pub async fn restore_managed_agents_on_launch( #[cfg(feature = "mesh-llm")] let agents_to_start = { + // Preflight against the same resolution spawn uses — `resolve_effective_config` + // (definition → global fallback). A linked instance's own `provider`/`model`/ + // `relay_mesh` bytes never contribute. See `start_local_agent_with_preflight` + // in `commands/agents.rs` for the identical rationale on the interactive path. + let personas = load_personas(app).unwrap_or_default(); + let global = super::load_global_agent_config(app).unwrap_or_default(); let mut mesh_preflight_failures = std::collections::HashSet::new(); for record in &agents_to_start { - if relay_mesh_model_id(record).is_none() { + let mesh_model_id = super::effective_config::resolve_effective_relay_mesh_model_id( + record, &personas, &global, + ); + if mesh_model_id.is_none() { continue; } // Auto-start after relaunch: re-resolve a live bootstrap target and // dial it. Skip (with an actionable error) only when no live target // serves this model right now. if let Err(error) = - crate::commands::ensure_relay_mesh_for_record(app, record, false).await + crate::commands::ensure_relay_mesh_for_record(app, mesh_model_id.as_deref(), false) + .await { persist_restore_error(app, &state, &record.pubkey, error)?; mesh_preflight_failures.insert(record.pubkey.clone()); diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index b9c7ec65ea..79ee8df1ac 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -189,12 +189,37 @@ pub fn build_managed_agent_summary( } }; - // Display contract: show the pinned record snapshot — what the agent - // actually runs — not the live persona. The drift flags below signal when - // the snapshot has fallen behind an edited persona; showing live values - // next to an "out of date" badge would contradict it. let (persona_out_of_date, persona_orphaned) = persona_drift_state(record, personas); + let global_for_summary = + crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + let effective_cfg = crate::managed_agents::effective_config::resolve_effective_config( + record, + personas, + &global_for_summary, + ); + let (effective_model, effective_provider, effective_prompt, model_source) = match effective_cfg + { + crate::managed_agents::effective_config::EffectiveConfigResult::Resolved(cfg) => { + let source = cfg.model.source.clone(); + ( + cfg.model.value, + cfg.provider.value, + cfg.system_prompt.value, + Some(source), + ) + } + crate::managed_agents::effective_config::EffectiveConfigResult::OrphanedInstance { + record_pubkey, + missing_persona_id, + } => { + eprintln!( + "orphaned agent instance: pubkey={record_pubkey}, missing_persona_id={missing_persona_id}" + ); + (None, None, None, None) + } + }; + // Restart badge: the running process stamped its effective spawn config // at launch; recompute from current disk state and flag drift. Only the // tracked live pair for THIS workspace can drift — stopped agents spawn @@ -227,7 +252,13 @@ pub fn build_managed_agent_summary( runtime.adapter_availability.as_ref(), super::adapter_availability_cached(), ); - hash_drift || availability_drift + // An orphan can never be restarted successfully — + // `spawn_agent_child` refuses it before any process side effect — + // so `needs_restart` must never fire for one regardless of hash or + // availability drift. Surfacing "Restart required" here would offer + // an action guaranteed to fail; the UI shows `persona_orphaned` + // instead (see `ManagedAgentSummary::persona_orphaned`). + restart_eligible(persona_orphaned, hash_drift, availability_drift) }); // Resolve the effective harness the same way, then derive args/mcp from it, @@ -254,10 +285,11 @@ pub fn build_managed_agent_summary( idle_timeout_seconds: record.idle_timeout_seconds, max_turn_duration_seconds: record.max_turn_duration_seconds, parallelism: record.parallelism, - system_prompt: record.system_prompt.clone(), + system_prompt: effective_prompt, avatar_url: record.avatar_url.clone(), - model: record.model.clone(), - provider: record.provider.clone(), + model: effective_model, + model_source, + provider: effective_provider, persona_out_of_date, persona_orphaned, needs_restart, @@ -281,6 +313,19 @@ pub fn build_managed_agent_summary( }) } +/// Pure predicate: should the "Restart required" badge fire? +/// +/// An orphaned linked instance (its persona/definition no longer exists) +/// can never be restarted successfully — `spawn_agent_child` refuses to +/// spawn it before any process side effect. Surfacing "Restart required" +/// for one would offer an action guaranteed to fail, so this always +/// returns `false` for an orphan regardless of drift. Extracted for unit +/// testing without `AppHandle`/global state, following the +/// `availability_drift` pattern in `discovery.rs`. +fn restart_eligible(persona_orphaned: bool, hash_drift: bool, availability_drift: bool) -> bool { + !persona_orphaned && (hash_drift || availability_drift) +} + pub fn find_managed_agent_mut<'a>( records: &'a mut [ManagedAgentRecord], pubkey: &str, @@ -389,6 +434,33 @@ pub fn spawn_agent_child( return Err(error); } let runtime_key = ManagedAgentRuntimeKey::new(record.pubkey.clone(), relay_url)?; + // Resolve the effective harness (agent command) from the linked persona, so + // persona harness edits propagate on the next spawn; an explicit per-agent + // override wins. `agent_args` and `mcp_command` are pure derivations of the + // command, so we recompute them from the effective value rather than the + // frozen record snapshot. Mirrors the model resolution below. + let personas = super::load_personas(app).unwrap_or_default(); + let teams = super::load_teams(app).unwrap_or_default(); + // Load global config once; used for runtime_metadata_env_vars (model/provider fallback) + // and for the env-var merge at spawn time. + let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + + // Resolve model/provider/prompt ONCE, here, at the shared spawn boundary — + // the single source both the env writes below and `spawn_config_hash` + // read from. Previously prompt was read from the record's own (possibly + // stale, Phase-A-snapshot) bytes while model/provider were resolved live + // from `personas`; a definition edit landing between a caller's snapshot + // apply and this spawn could hand a fresh model/provider to a stale + // prompt. This also folds in orphan refusal via `require_resolved`: every + // caller (interactive start, launch restore, `start_managed_agent_process`) + // inherits it — no caller can bypass this by reaching `spawn_agent_child` + // directly. Checked before any side effect (log marker, log file, process + // spawn) so a refused spawn leaves no trace. + let effective_cfg = crate::managed_agents::effective_config::resolve_effective_config( + record, &personas, &global, + ) + .require_resolved()?; + let log_path = super::managed_agent_runtime_log_path(app, &runtime_key)?; append_log_marker( &log_path, @@ -404,16 +476,6 @@ pub fn spawn_agent_child( let stderr = stdout .try_clone() .map_err(|error| format!("failed to clone log handle: {error}"))?; - // Resolve the effective harness (agent command) from the linked persona, so - // persona harness edits propagate on the next spawn; an explicit per-agent - // override wins. `agent_args` and `mcp_command` are pure derivations of the - // command, so we recompute them from the effective value rather than the - // frozen record snapshot. Mirrors the model resolution below. - let personas = super::load_personas(app).unwrap_or_default(); - let teams = super::load_teams(app).unwrap_or_default(); - // Load global config once; used for runtime_metadata_env_vars (model/provider fallback) - // and for the env-var merge at spawn time. - let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let effective_command = super::record_agent_command(record, &personas); let agent_args = normalize_agent_args(&effective_command, record.agent_args.clone()); let resolved_acp_command = resolve_command(&record.acp_command) @@ -628,37 +690,40 @@ pub fn spawn_agent_child( command.env_remove("BUZZ_ACP_TEAM_INSTRUCTIONS"); } - // System prompt via the shared spawn-effective filter — the SAME function the - // config hash digests, so env write and badge cannot disagree (see - // `effective_spawn_prompt` for the Some("")/None collapse). Model and provider - // use the shared - // resolver: agent → persona → global → None, so a global-default-only agent - // spawns with the correct provider/model env. - let effective_prompt = super::spawn_hash::effective_spawn_prompt(record); - let (effective_model, effective_provider) = - crate::managed_agents::resolve_effective_model_provider(record, &personas, &global); + // Prompt, model, and provider all come from the single `effective_cfg` + // resolved at the top of this function — the SAME resolve `spawn_config_hash` + // performs below, so env write and restart badge cannot disagree. Linked + // instances never consult the record's own model/provider/prompt bytes; + // definition-less instances fall back to their own fields, then global. + // + // Derive the mesh decision BEFORE moving fields out — `relay_mesh_model_id` + // is the single authoritative gate; the mesh-llm block below MUST use it + // rather than re-deriving from `effective_provider` to keep preflight and + // spawn semantics in lock-step (see `EffectiveAgentConfig::relay_mesh_model_id`). + #[cfg(feature = "mesh-llm")] + let mesh_model_id = effective_cfg.relay_mesh_model_id(); + let effective_prompt = effective_cfg.system_prompt.value; + let effective_model = effective_cfg.model.value; + let effective_provider = effective_cfg.provider.value; if let Some(prompt) = &effective_prompt { command.env("BUZZ_ACP_SYSTEM_PROMPT", prompt); } else { command.env_remove("BUZZ_ACP_SYSTEM_PROMPT"); } - if let Some(model) = effective_model { + if let Some(model) = effective_model.as_deref() { command.env("BUZZ_ACP_MODEL", model); } else { command.env_remove("BUZZ_ACP_MODEL"); } - // Baked-in provider defaults for internal builds (buzz-releases sets - // BUZZ_BUILD_BUZZ_AGENT_* at compile time; OSS builds bake nothing). - // Written FIRST so that record/persona metadata env vars below override them. build_buzz_agent_provider_defaults(&mut command); if let Some(meta) = runtime_meta { for (key, value) in runtime_metadata_env_vars( meta.model_env_var, meta.provider_env_var, meta.provider_locked, - effective_model, - effective_provider, + effective_model.as_deref(), + effective_provider.as_deref(), ) { command.env(key, value); } @@ -749,10 +814,17 @@ pub fn spawn_agent_child( // Buzz shared compute is stored as a native provider; derive the OpenAI-compatible // transport at spawn time and scrub any unrelated ambient OpenAI key. + // Gate on `mesh_model_id` (derived from `effective_cfg.relay_mesh_model_id()` + // above) — not on `effective_provider` directly — so the mesh gate here + // uses the same trim semantics as the preflight callers. #[cfg(feature = "mesh-llm")] - if effective_provider == Some(super::RELAY_MESH_PROVIDER_ID) { + if let Some(ref mesh_model_id) = mesh_model_id { let mut mesh_env = std::collections::BTreeMap::new(); - super::apply_relay_mesh_env(&mut mesh_env, effective_provider, effective_model); + super::apply_relay_mesh_env( + &mut mesh_env, + Some(super::RELAY_MESH_PROVIDER_ID), + Some(mesh_model_id.as_str()), + ); command.env_remove("OPENAI_API_KEY"); for (key, value) in mesh_env { command.env(key, value); diff --git a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs index 56d2e68353..ac9e3a0a3f 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs @@ -24,18 +24,11 @@ pub(crate) fn runtime_metadata_env_vars<'a>( vars } -/// Resolve the effective (prompt, model, provider) triple for a persona-linked agent. -/// -/// Given a persona_id, finds the persona in the list and returns its system_prompt, -/// model, and provider as the authoritative values. When the persona leaves `model` -/// or `provider` blank (None or whitespace-only), falls back to the record's own -/// field using the same precedence rule as `persona_snapshot_with_agent_config_fallback` -/// so the display surface matches spawn behavior. Falls back to the record's own -/// prompt/model/provider when no persona is linked or found. +/// Resolve effective prompt/model/provider using definition-authoritative +/// semantics for linked instances. /// /// Used by `agent_config.rs` to inject persona defaults into the config surface -/// before running the reader, so BuzzExplicit-tagged fields can be re-tagged to -/// PersonaDefault for fields the record did not independently set. +/// before running the reader. pub(crate) fn resolve_effective_prompt_model_provider( persona_id: Option<&str>, personas: &[crate::managed_agents::types::AgentDefinition], @@ -43,13 +36,16 @@ pub(crate) fn resolve_effective_prompt_model_provider( record_model: Option, record_provider: Option, ) -> (Option, Option, Option) { - let fallback = crate::managed_agents::persona_events::persona_field_with_record_fallback; match persona_id.and_then(|pid| personas.iter().find(|p| p.id == pid)) { - Some(p) => ( - Some(p.system_prompt.clone()), - fallback(p.model.as_deref(), record_model.as_deref()), // fallback: record.model - fallback(p.provider.as_deref(), record_provider.as_deref()), // fallback: record.provider - ), + Some(p) => { + fn non_blank(v: Option<&str>) -> Option { + v.filter(|s| !s.trim().is_empty()).map(str::to_owned) + } + let prompt = non_blank(Some(&p.system_prompt)); + let model = non_blank(p.model.as_deref()); + let provider = non_blank(p.provider.as_deref()); + (prompt, model, provider) + } None => (record_prompt, record_model, record_provider), } } diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index 019bfe61d3..55fa8efba7 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -315,20 +315,16 @@ fn persona_with_provider( // that the old create-time env baking silently blocked. use crate::managed_agents::env_vars::{live_persona_env, merged_user_env}; -use crate::managed_agents::persona_events::persona_snapshot; use std::collections::BTreeMap; /// Apply a persona snapshot onto a record, mirroring `create_managed_agent`: -/// snapshotted prompt/model/provider/source_version are pinned, with the -/// system_prompt unwrapped (the persona always carries one). `env_vars` is -/// deliberately NOT touched — it stays agent overrides only. +/// links the record to `persona.id`, then delegates the actual snapshot +/// (prompt/model/provider/runtime/source_version) to the real production +/// `apply_persona_snapshot` — so a change to that function's behavior is +/// exercised by these tests instead of silently diverging from it. fn pin_persona(record: &mut ManagedAgentRecord, persona: &crate::managed_agents::AgentDefinition) { - let snapshot = persona_snapshot(persona); record.persona_id = Some(persona.id.clone()); - record.system_prompt = snapshot.system_prompt; - record.model = snapshot.model; - record.provider = snapshot.provider; - record.persona_source_version = Some(snapshot.source_version); + crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); } fn persona_v( @@ -428,19 +424,31 @@ fn agent_env_overrides_win_over_persona_env_at_spawn() { } #[test] -fn orphaned_agent_spawns_from_its_own_overrides() { - // Persona deleted: the live merge degrades to the record's own overrides. +fn orphaned_agent_refused_at_spawn_boundary() { + // Persona deleted: `spawn_agent_child` must refuse before any process + // side effect, not silently degrade to the record's stale overrides. + // `require_resolved` on the shared resolver is the pure predicate + // `spawn_agent_child` checks first — this pins the contract without + // needing a real `AppHandle`. let persona = persona_v("p", "prompt", &[("ANTHROPIC_API_KEY", "persona-key")]); let mut record = fixture(RespondTo::Anyone, vec![], Some("tag".into())); record.env_vars = BTreeMap::from([("EXTRA".to_string(), "agent-value".to_string())]); pin_persona(&mut record, &persona); - let env = spawn_user_env(&record, &[]); - assert_eq!(env.get("EXTRA").map(String::as_str), Some("agent-value")); + // The persona is absent from the live catalog — same shape restore/start + // see when a persona was deleted on another device. + let no_personas: &[crate::managed_agents::AgentDefinition] = &[]; + let error = crate::managed_agents::effective_config::resolve_effective_config( + &record, + no_personas, + &Default::default(), + ) + .require_resolved() + .unwrap_err(); assert_eq!( - env.get("ANTHROPIC_API_KEY"), - None, - "a deleted persona's env must not linger" + error, + crate::managed_agents::effective_config::ORPHANED_INSTANCE_ERROR.to_string(), + "an orphaned linked instance must be refused, not spawned from its own overrides" ); } @@ -1010,3 +1018,37 @@ fn invalid_pubkey_resolves_no_pair_key() { // the summary must fall back to the stopped/legacy-pid path, not panic. assert!(super::resolve_workspace_pair_key("not-a-key", "", "wss://one.example").is_none()); } + +// ── restart_eligible tests ────────────────────────────────────────────── + +#[test] +fn restart_eligible_true_when_non_orphan_has_hash_drift() { + assert!(super::restart_eligible(false, true, false)); +} + +#[test] +fn restart_eligible_true_when_non_orphan_has_availability_drift() { + assert!(super::restart_eligible(false, false, true)); +} + +#[test] +fn restart_eligible_false_when_orphan_has_hash_drift() { + // An orphan can never be restarted successfully — spawn refuses it — + // so hash drift alone must not surface "Restart required". + assert!(!super::restart_eligible(true, true, false)); +} + +#[test] +fn restart_eligible_false_when_orphan_has_availability_drift() { + assert!(!super::restart_eligible(true, false, true)); +} + +#[test] +fn restart_eligible_false_when_orphan_has_no_drift() { + assert!(!super::restart_eligible(true, false, false)); +} + +#[test] +fn restart_eligible_false_when_non_orphan_has_no_drift() { + assert!(!super::restart_eligible(false, false, false)); +} diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index 4d80549288..8cf0014e36 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -28,24 +28,14 @@ use std::hash::{DefaultHasher, Hash, Hasher}; use super::{ + effective_config::{resolve_effective_config, EffectiveConfigResult}, known_acp_runtime, normalize_agent_args, - persona_events::apply_persona_snapshot, + persona_events::preview_prospective_persona_snapshot, resolve_effective_agent_env, types::{AgentDefinition, ManagedAgentRecord, TeamRecord}, GlobalAgentConfig, }; -/// The prompt a spawn would actually deliver: `Some("")` collapses to `None` -/// because an empty `BUZZ_ACP_SYSTEM_PROMPT` is no prompt. -/// -/// The single source of truth for the spawn env write AND the config hash. -pub(crate) fn effective_spawn_prompt(record: &ManagedAgentRecord) -> Option { - record - .system_prompt - .clone() - .filter(|prompt| !prompt.is_empty()) -} - /// Resolve the current instructions for this instance's deployment-time team binding. /// A deleted team deliberately degrades to no team section. pub(crate) fn effective_team_instructions( @@ -78,12 +68,7 @@ pub(crate) fn spawn_config_hash( // when nothing changed. The persona env itself reaches the hash through // `resolve_effective_agent_env` below; `persona_source_version` is set on // the clone but is not a hash input. - let mut record = record.clone(); - if let Some(persona_id) = record.persona_id.clone() { - if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { - apply_persona_snapshot(&mut record, persona); - } - } + let record = preview_prospective_persona_snapshot(record, personas); let record = &record; let effective_command = crate::managed_agents::record_agent_command(record, personas); @@ -109,11 +94,24 @@ pub(crate) fn spawn_config_hash( // resolved: every record spawns on the workspace relay (legacy pins // ignored), so a workspace relay change must trip the badge. crate::relay::effective_agent_relay_url(&record.relay_url, workspace_relay).hash(&mut hasher); - // Prompt and runtime-layered team instructions use the same resolver as spawn. - effective_spawn_prompt(record).hash(&mut hasher); + // Team instructions use the same resolver as spawn. effective_team_instructions(record, teams).hash(&mut hasher); - record.model.hash(&mut hasher); - record.provider.hash(&mut hasher); + // Prompt, model, and provider all come from ONE `resolve_effective_config` + // call — the SAME resolve `spawn_agent_child` performs for the env write, + // so env write and this badge cannot disagree. An orphaned link (missing + // definition) hashes as if all three were absent: `spawn_agent_child` + // refuses to spawn an orphan regardless, so this is a display-only + // convenience, not the spawn gate. + let (resolved_prompt, resolved_model, resolved_provider) = + match resolve_effective_config(record, personas, global) { + EffectiveConfigResult::Resolved(cfg) => { + (cfg.system_prompt.value, cfg.model.value, cfg.provider.value) + } + EffectiveConfigResult::OrphanedInstance { .. } => (None, None, None), + }; + resolved_prompt.hash(&mut hasher); + resolved_model.hash(&mut hasher); + resolved_provider.hash(&mut hasher); record.auth_tag.hash(&mut hasher); record.respond_to.as_str().hash(&mut hasher); // The allowlist is hashed as the env receives it: spawn sets diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs index 6438b3b37d..44ad7f4bbc 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -441,18 +441,123 @@ fn missing_definition_leaves_materialized_runtime_in_hash() { ); } +// ── Global default trips hash for linked inherited agents ───────────────── + #[test] -fn effective_spawn_prompt_matches_hash_semantics() { - // The env write and the hash share effective_spawn_prompt — this row - // pins the helper's own semantics so a refactor of either caller cannot - // silently diverge from the contract. - let mut r = record(); - r.system_prompt = Some(String::new()); - assert_eq!( - effective_spawn_prompt(&r), - None, - "empty collapses to absent" +fn global_model_change_trips_hash_for_linked_inherited_agent() { + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.model = Some("stale-record-model".into()); + + let personas = vec![persona("p1", Some("goose"), "prompt")]; + + let global_a = GlobalAgentConfig { + model: Some("model-a".to_string()), + provider: Some("prov-a".to_string()), + ..Default::default() + }; + let global_b = GlobalAgentConfig { + model: Some("model-b".to_string()), + provider: Some("prov-b".to_string()), + ..Default::default() + }; + + let hash_a = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_a); + let hash_b = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_b); + + assert_ne!( + hash_a, hash_b, + "changing the global default must trip the hash for a linked inherited agent" + ); +} + +#[test] +fn global_model_change_trips_hash_without_model_env_var() { + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.agent_command = "some-harness-without-model-env".into(); + + let personas = vec![{ + let mut p = persona("p1", None, "prompt"); + p.model = None; + p.provider = None; + p + }]; + + let global_a = GlobalAgentConfig { + model: Some("model-a".to_string()), + ..Default::default() + }; + let global_b = GlobalAgentConfig { + model: Some("model-b".to_string()), + ..Default::default() + }; + + let hash_a = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_a); + let hash_b = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_b); + + assert_ne!( + hash_a, hash_b, + "global model change must trip hash even without a model_env_var runtime" + ); +} + +#[test] +fn linked_instance_stale_prompt_bytes_are_inert_at_hash_time() { + // Regression for the split-resolve defect: prompt used to be read from + // the record's own (possibly Phase-A-snapshot-stale) bytes while + // model/provider were resolved live from the definition. A definition + // edit landing between a caller's snapshot apply and spawn could hand a + // fresh model/provider to a stale prompt, and the hash (which already + // resolved model/provider live) would silently agree with a spawn that + // wrote the stale prompt. Now both come from one `resolve_effective_config` + // call, so a record whose own `system_prompt` bytes disagree with the + // live definition must hash exactly as if the record carried the + // definition's prompt verbatim — the record's prompt bytes are inert for + // a linked instance. + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.system_prompt = Some("stale prompt on record".into()); + + let mut matching_bytes = rec.clone(); + matching_bytes.system_prompt = Some("live prompt".into()); + + let personas = [persona("p1", Some("goose"), "live prompt")]; + + assert_eq!( + spawn_config_hash( + &rec, + &personas, + &[], + "wss://ws.example", + &Default::default() + ), + spawn_config_hash( + &matching_bytes, + &personas, + &[], + "wss://ws.example", + &Default::default() + ), + "record's own system_prompt bytes must not affect the hash of a linked instance" + ); +} + +#[test] +fn linked_instance_prompt_model_provider_resolve_from_one_call() { + // The prompt for a linked instance must track the definition, exactly + // like model/provider — a definition prompt edit trips the hash even + // though the record's own (stale) system_prompt bytes are unchanged. + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.system_prompt = Some("stale".into()); + + let before = [persona("p1", Some("goose"), "old definition prompt")]; + let after = [persona("p1", Some("goose"), "new definition prompt")]; + + assert_ne!( + spawn_config_hash(&rec, &before, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&rec, &after, &[], "wss://ws.example", &Default::default()), + "linked instance prompt must resolve from the live definition, not stale record bytes" ); - r.system_prompt = Some("real".into()); - assert_eq!(effective_spawn_prompt(&r).as_deref(), Some("real")); } diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index af4908911b..cce2cd4039 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -259,13 +259,19 @@ pub struct ManagedAgentRecord { /// Desired LLM model ID. Matches AgentModelInfo.id from discovery. /// The harness re-discovers the correct ACP switching metadata at session /// creation by matching this ID against the fresh session/new response. + /// For a linked instance this is a legacy/display snapshot only — spawn + /// and deploy resolve the effective model from the definition, never + /// from this field (see `effective_config::resolve_effective_config`). + /// For a definition-less instance this field is authoritative. #[serde(default)] pub model: Option, - /// LLM inference provider snapshotted from the persona at create time - /// (e.g. 'databricks', 'anthropic'). Spawn and deploy read this, never the - /// live persona — so the agent stays pinned to the provider it was created - /// with across restarts. `#[serde(default)]` so pre-existing records - /// deserialize as `None` and get backfilled on first load. + /// LLM inference provider. For a linked instance this is a legacy/display + /// snapshot only — spawn and deploy resolve the effective provider from + /// the definition, never from this field (see + /// `effective_config::resolve_effective_config`). For a definition-less + /// instance this field is authoritative. `#[serde(default)]` so + /// pre-existing records deserialize as `None` and get backfilled on + /// first load. #[serde(default)] pub provider: Option, /// Content hash of the persona at the time this agent was created — the @@ -390,12 +396,15 @@ pub struct ManagedAgentRecord { /// inference through Buzz's relay-mesh local endpoint; the `model_ref` is /// the served model id to route to. `None` is a normal agent. /// - /// This is the source of truth for "is this a mesh agent + which model" — - /// replacing the old practice of sniffing it back out of `env_vars` - /// (`relay_mesh_config`). Spawn-time env vars are *derived from* this, not - /// the other way around. `#[serde(default)]` so pre-existing saved records - /// deserialize as `None` and are resolved via the env-var fallback until - /// they are rewritten with this field. + /// Not the source of truth. `provider == "relay-mesh"` is, resolved through + /// `effective_config::resolve_effective_config`; spawn-time env vars are + /// derived from that resolution. This field is retained solely as a + /// backward-compatibility signal for records written before the record had + /// a `provider` field, and is consulted only for definition-less records + /// that carry no provider — after which the env-var preset is the last + /// fallback. A linked instance's marker is never read: its definition is + /// authoritative. `#[serde(default)]` so records predating the field + /// deserialize as `None`. #[serde(default, skip_serializing_if = "Option::is_none")] pub relay_mesh: Option, } @@ -478,7 +487,11 @@ pub struct ManagedAgentSummary { pub system_prompt: Option, pub avatar_url: Option, pub model: Option, - /// LLM inference provider, from the agent's pinned record snapshot. + #[serde(skip_serializing_if = "Option::is_none")] + pub model_source: Option, + /// LLM inference provider, resolved the same way as `model`/`model_source` + /// (definition → global for linked instances; instance → global for + /// definition-less instances). `None` for an orphaned instance. pub provider: Option, /// `true` when the linked persona has been edited since this agent was /// created — the running agent uses the older pinned snapshot. The UI @@ -487,9 +500,11 @@ pub struct ManagedAgentSummary { /// persona is gone, so there is nothing newer to drift toward). pub persona_out_of_date: bool, /// `true` when the agent was created from a persona that no longer exists. - /// Distinct from out-of-date: there is no current persona to respawn into, - /// so the UI should not prompt a respawn — the pinned snapshot is all the - /// config that remains. + /// Distinct from out-of-date: there is no current persona to respawn into. + /// An orphaned agent also cannot be (re)started — `spawn_agent_child` + /// refuses it (see `effective_config::resolve_effective_config`'s + /// `OrphanedInstance` arm via `require_resolved`) — so the UI + /// should surface that it's stuck, not merely stale. pub persona_orphaned: bool, /// `true` when the running process was spawned with a config that no /// longer matches what a spawn would use today — a plain restart would diff --git a/desktop/src/features/agents/lib/agentCardModelLabel.test.mjs b/desktop/src/features/agents/lib/agentCardModelLabel.test.mjs new file mode 100644 index 0000000000..696a055d20 --- /dev/null +++ b/desktop/src/features/agents/lib/agentCardModelLabel.test.mjs @@ -0,0 +1,58 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { resolveAgentCardModelLabel } from "./agentCardModelLabel.ts"; + +test("resolveAgentCardModelLabel — unspawned definition with explicit model renders the model, not inherited", () => { + const label = resolveAgentCardModelLabel({ + agent: undefined, + personaModel: "gpt-5", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "gpt-5"); +}); + +test("resolveAgentCardModelLabel — unspawned definition with no model renders the default", () => { + const label = resolveAgentCardModelLabel({ + agent: undefined, + personaModel: null, + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); + +test("resolveAgentCardModelLabel — linked instance inheriting the global default ignores stale persona.model", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: "global", model: "stale-model" }, + personaModel: "gpt-5", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); + +test("resolveAgentCardModelLabel — linked instance with no modelSource (legacy/unset) is treated as inherited", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: null, model: "stale-model" }, + personaModel: "gpt-5", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); + +test("resolveAgentCardModelLabel — linked instance with an explicit resolved model renders that model", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: "definition", model: "gpt-5" }, + personaModel: "should-not-be-used", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "gpt-5"); +}); + +test("resolveAgentCardModelLabel — non-inherited agent with a blank resolved model falls back to the default", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: "instance_legacy", model: " " }, + personaModel: null, + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); diff --git a/desktop/src/features/agents/lib/agentCardModelLabel.ts b/desktop/src/features/agents/lib/agentCardModelLabel.ts new file mode 100644 index 0000000000..4ec06f10c5 --- /dev/null +++ b/desktop/src/features/agents/lib/agentCardModelLabel.ts @@ -0,0 +1,44 @@ +import { formatAgentModelLabel } from "./formatAgentModelLabel"; +import type { ManagedAgent } from "@/shared/api/types"; + +/** + * Model label for the Agents-page card, shared by the definition-grouped + * card (`AgentPersonaCard`) and the standalone/legacy card + * (`StandaloneAgentCard`). + * + * A materialized `agent` is authoritative once it exists: its `modelSource` + * says whether the *effective* config (from `resolve_effective_config` on + * the backend) came from the global default or from an explicit + * definition/instance value, so the card never has to re-derive that from + * raw model bytes. Absent or `"global"` renders the default-model label; + * anything else renders the agent's own resolved model. + * + * With NO materialized instance yet (a definition that has never been + * started), there is no `modelSource` to read — the definition itself is + * authoritative, so an explicit `personaModel` must render directly rather + * than falling through to "inherited" for lack of an instance. + */ +export function resolveAgentCardModelLabel(input: { + agent: Pick | undefined; + personaModel: string | null | undefined; + defaultModel: string; +}): string { + if (input.agent) { + const isInherited = + !input.agent.modelSource || input.agent.modelSource === "global"; + if (isInherited) { + return formatDefaultModelLabel(input.defaultModel); + } + return input.agent.model?.trim() + ? formatAgentModelLabel(input.agent.model) + : formatDefaultModelLabel(input.defaultModel); + } + return input.personaModel?.trim() + ? formatAgentModelLabel(input.personaModel) + : formatDefaultModelLabel(input.defaultModel); +} + +export function formatDefaultModelLabel(defaultModel: string) { + const model = defaultModel.trim(); + return model ? `Default model (${model})` : "Default model"; +} diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 86a70c3db6..601d57f95d 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -655,30 +655,36 @@ export function AgentInstanceEditDialog({ parsedParallelism > 0 && parsedParallelism !== agent.parallelism ? parsedParallelism : undefined, - // Use tri-state: send null to clear, value to set, omit if unchanged. + // Linked instances defer model/provider/systemPrompt to the definition. systemPrompt: - (systemPrompt.trim() || null) !== agent.systemPrompt - ? systemPrompt.trim() || null - : undefined, + linkedPersona != null + ? undefined + : (systemPrompt.trim() || null) !== agent.systemPrompt + ? systemPrompt.trim() || null + : undefined, model: - normalizedModel !== (agent.model ?? null) - ? normalizedModel - : undefined, + linkedPersona != null + ? undefined + : normalizedModel !== (agent.model ?? null) + ? normalizedModel + : undefined, // Tri-state provider persistence keyed on providerRuntimeCapability: // "capable" → persist: value if changed, omit if unchanged. // "locked" → clear: send null if provider was set, else omit. // "unknown" → omit always (never send null for a transient state). // llmProviderFieldVisible is for UX visibility only; not used here. provider: - providerRuntimeCapability === "capable" - ? normalizedSubmitProvider !== (agent.provider ?? null) - ? normalizedSubmitProvider - : undefined - : providerRuntimeCapability === "locked" - ? (agent.provider ?? null) !== null - ? null + linkedPersona != null + ? undefined + : providerRuntimeCapability === "capable" + ? normalizedSubmitProvider !== (agent.provider ?? null) + ? normalizedSubmitProvider : undefined - : undefined, // "unknown" → omit always + : providerRuntimeCapability === "locked" + ? (agent.provider ?? null) !== null + ? null + : undefined + : undefined, // "unknown" → omit always envVars: envVarsEqual(submitEnvVars, agent.envVars) ? undefined : submitEnvVars, diff --git a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx index 35de0d983d..8412792a2c 100644 --- a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx +++ b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx @@ -211,29 +211,32 @@ export function EditAgentAdvancedFields({ - {/* System prompt override */} -
- -
-