Finish incomplete imported-agent setup with Use here - #226
Conversation
a132537 to
f5f6acd
Compare
129a378 to
cf5bae6
Compare
f5f6acd to
5b860bc
Compare
cf5bae6 to
c47f7ab
Compare
5b860bc to
4208fd5
Compare
c47f7ab to
b1e77fe
Compare
46f77c1 to
f5a68de
Compare
745aaf9 to
cd4172f
Compare
f5a68de to
0912c62
Compare
cd4172f to
a7151f5
Compare
1ee2ac3 to
c89f21c
Compare
a7151f5 to
0d2b922
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: two blockers in the setup delivery contract, detailed inline. Merge criteria: authorize the new IPC command through the production capability and preserve stopped/manual-start state when completing an incomplete import, with regression coverage for both.
Reviewed head c89f21c18b963b764027f43d58046b913c5f08c6 against base 0d2b9223c128880cb8aedd8c7cecf6e41578513d. Existing CI is green. Validation: complete diff and cross-layer source review; isolated controller reproduction of all three retained-startup-intent cases. No broad CI rerun or live desktop/Keychain/ACP acceptance.
Non-blocking UX follow-up: cross-community setup leaves the source card’s Use here prompt unchanged while adding a configured sibling. Directing focus/status to the completed destination would make success clearer.
| agent_control_create_commit, | ||
| agent_control_creation_profile, | ||
| agent_control_snapshot, | ||
| agent_control_use_here, |
There was a problem hiding this comment.
[P1] Grant the Use here command through the production app ACL
Registering this handler does not make it callable. agent_control_use_here is absent from src-tauri/build.rs’s AppManifest::commands and src-tauri/capabilities/default.json. With this app’s ACL manifest, pinned Tauri 2.11.5 rejects a command without a resolved grant before dispatching the handler (webview/mod.rs:1794–1852). Consequently Use here obtains the signature, then fails at native invocation instead of completing setup.
Add the command manifest entry and main-webview permission, and include it in browser_permissions_tests.rs’s production-context ACL test (including guest/remote rejection). The new test mocking invoke cannot catch this failure.
There was a problem hiding this comment.
Larry (agent), replying via Logan's account.
Fixed in 76d00bc. agent_control_use_here is now in src-tauri/build.rs AppManifest::commands, and allow-agent-control-use-here is in capabilities/default.json. browser_permissions_tests.rs checks it in the production ACL test, including guest and remote rejection. I also found the same gap for the Clone commands later in the stack (agent_control_clone_settings, agent_control_local_clone_settings) and fixed it there too.
| if !target.configured() { | ||
| target.extra.insert("configured".into(), Value::Bool(true)); | ||
| target.revision = target | ||
| .revision | ||
| .checked_add(1) |
There was a problem hiding this comment.
[P2] Clear inherited automatic-start intent when completing setup
For a retained incomplete import with startOnAppLaunch: true, this transition changes configured but preserves the startup preference. The new-target branch also clones that preference, despite clearing enabled. Controller::launch_ids() therefore admits the completed target on the next app launch, and native restore starts it without the separate Start promised by this PR. In-place setup additionally preserves legacy enabled: true, which becomes the startup preference when the explicit field is absent.
An isolated run against this head reproduced all three cases: same destination with explicit startup true, different destination with explicit startup true, and same destination with legacy enabled true. Each had zero launch candidates before Use here and one afterward. No actual credentials/processes were used.
When transitioning an incomplete target to configured (or creating its destination record), save enabled = false and start_on_app_launch = Some(false) and test post-setup/reopen restore eligibility. Keep already-configured idempotent retries from resetting a legitimately running agent.
There was a problem hiding this comment.
Larry (agent), replying via Logan's account.
Fixed in 76d00bc. When Use here moves a target from incomplete to configured, it now saves enabled = false and start_on_app_launch = Some(false). This covers the in-place record and the new destination record. A retry on an already-configured target changes nothing. The regression test use_here_clears_retained_startup_intent_until_an_explicit_start covers all three cases you reproduced: same destination with startup true, a new destination with startup true, and same destination with legacy enabled: true. It checks that restore has no launch candidates after setup.
Related fix: main added Delete (#241). A new-destination Use here setup shares the source credential reference, so deleting either card removed the shared Keychain entry. Delete now keeps the credential while another setup of the same identity still uses it. Test: delete_keeps_a_key_shared_by_another_setup_of_the_same_identity.
baxen
left a comment
There was a problem hiding this comment.
LGTM, checked the rust side in more detail than the TS
0d2b922 to
657ea06
Compare
c89f21c to
76d00bc
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
No actionable findings in this follow-up. The two prior findings are addressed in the reviewed source:
- Use here IPC permission:
agent_control_use_hereis now included in the generated application-command manifest and the capability restricted to the main webview. The existing permission regression also includes this command for local-main admission and guest/remote rejection (src-tauri/build.rs,src-tauri/capabilities/default.json,src-tauri/src/browser_permissions_tests.rs). - Stopped setup / startup intent: both incomplete-target paths clear
enabledandstart_on_app_launch. The added regression source covers same- and other-community setup, legacy enabled fallback, persistence across reopen, and preserving an already-configured setup’s deliberate startup preference on retry (crates/agent-controller/src/store.rs:186-210,crates/agent-controller/src/runtime/tests.rs:1504-1565).
I also inspected the shared-key deletion change against the serialized native caller: deleting one setup retains custody while another saved setup uses the same credential reference and public key; final deletion still removes custody. The frontend/native caller chain, destination-scoped unmount guard, incomplete-import launch/selection exclusions, and Stop recovery were inspected for integration regressions. The earlier optional cross-community card/focus UX observation is not a new blocker.
Pinned scope: head 76d00bcc0a2fef7c815d636f91e02258e1cc02d9; base/merge-base 657ea06871cf38c6fbdf393085cb541fec413459 (stacked on #225). This is a bounded follow-up to the feedback on c89f21c18b963b764027f43d58046b913c5f08c6; actual branch changes were distinguished from incoming base changes. It does not clear findings on ancestor PRs.
Validation limits: source-only review of immutable Git objects, with extracted source bytes/blob IDs/SHA256 verified and no dirty source inputs. Regression tests were read, not executed. No builds, app launches, credential operations, or live setup/delete workflows were run; CI was not assessed. Native/runtime behavior and human acceptance remain unverified. This COMMENT is not an approval or merge authorization; the earlier submitted review was not dismissed.
…t starting Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
657ea06 to
cc4b36f
Compare
76d00bc to
3e749fd
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
No new actionable findings in this follow-up. Direct tree comparison with the last reviewed revision, 76d00bcc0a2fef7c815d636f91e02258e1cc02d9, shows changes in only four files; the previously reviewed Use here implementation and its two blocker fixes remain unchanged.
- The import preview now excludes a public key already managed in any community. The caller supplies the complete native agent snapshot, and native import rejects duplicate keys both during preparation and commit (
src/bundled/agents/AgentImport.tsx:70–78,AgentControlPanel.tsx:181–190,crates/agent-controller/src/import.rs:153–165,252–259). The added component regression covers a different-community record, case normalization, and repeated loading. - Inventory definition aliases now require an exact local definition ID. A reference to an omitted, slug-colliding definition remains unmatched instead of being grouped under another profile (
src/features/agents/inventory.ts:14–22,43–52). I inspected the regression and the definition/avatar/grouping consumers. - The production Use here command manifest/main-webview capability and the stopped-setup fix remain present. Both incomplete-target paths clear
enabledandstart_on_app_launch; already-configured retries preserve their deliberate preferences. These source files and the earlier regression coverage are byte-identical to the last reviewed revision.
Pinned scope: head 3e749fdb8a8e4ec60f9246091017f8850a427423; base cc4b36f735fcfd7ce44111a4115456d66addbfc7 (stacked on #225). This is a bounded follow-up, including the incoming-base changes; it does not clear findings on ancestor PRs or reopen the earlier optional card/focus observation.
Validation limits: source-only review of immutable GitHub blobs, verified against pinned trees; no dirty working-tree inputs. Regression tests were read, not run. No builds, app launches, credential operations, or live setup/import/delete workflows were executed. CI was not assessed; native/runtime behavior and human acceptance remain unverified. This COMMENT is not an approval or merge authorization, and no earlier review was dismissed.
Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
🤖
Summary
Details