Add Duplicate and Delete actions to agent cards - #241
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65624c0ab6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,91 @@ | |||
| import { useState } from "react"; | |||
There was a problem hiding this comment.
Commit 639fdc54a8fcf7be691883f6c3077d8468045fbd has no Signed-off-by trailer, so it does not meet the repository's mandatory DCO requirement and cannot pass the required DCO check; recreate the commit with git commit --signoff using the verified author identity.
AGENTS.md reference: AGENTS.md:L153-L158
Useful? React with 👍 / 👎.
65624c0 to
245ef12
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 245ef12c472cadb1878911696f6c25f3c7485d55: one blocking desktop IPC wiring defect, detailed inline. Register and grant the Delete command through the existing app ACL, then cover invocation with the production capabilities loaded.
Hosted CI is green. This was a source review, including the pinned Tauri and Security Framework implementations; I did not rerun suites or touch live agents/credentials. Manual desktop and real macOS Keychain deletion remain unverified. In particular, credential deletion holds the controller mutex; OS-prompt behavior needs an isolated macOS check, not a claim that the whole app freezes. No additional material defect was established in Duplicate, credential targeting, or the local deletion/retry ordering.
| agent_control_creation_profile, | ||
| agent_control_snapshot, | ||
| agent_control_save, | ||
| agent_control_delete, |
There was a problem hiding this comment.
[P1] Grant the Delete command in the app ACL
Adding the handler here is insufficient because this application opts into Tauri's explicit command ACL. src-tauri/build.rs:41–48 omits agent_control_delete, and src-tauri/capabilities/default.json:15–22 omits allow-agent-control-delete. The pinned Tauri 2.11.5 checks has_app_acl_manifest && invoke.acl.is_none() before dispatch (src/webview/mod.rs:1819–1851), so confirming Delete in the main desktop webview is rejected before Controller::delete runs. Retry cannot fix it.
Add the command to AppManifest::commands and its generated allow permission to the main-window capability. Add an IPC regression that loads the production app capabilities and proves Delete reaches a synthetic controller/credential adapter; the new mocked invoke payload assertion and direct controller tests bypass the missing grant.
There was a problem hiding this comment.
🤖 Fixed in 7ef89f6. Delete is now in the Tauri app manifest and main-window capability. The native IPC fixture now loads the production app context, and a regression test confirms Delete reaches a synthetic credential adapter; the main-webview ACL test includes Delete too. Local native library tests passed (62 passed, 3 ignored), as did the Agents page tests and TypeScript check. Hosted CI is running on the new head.
245ef12 to
7ef89f6
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No remaining code blockers found in the focused re-review of 7ef89f6c10f5479036d97530b6a759b10d261116 against df7b7e7f45739f3e06e12d81623385701acdc51d.
- The previous ACL blocker is fixed: Delete is registered and granted to the main webview. The production-context IPC regression reaches the synthetic credential adapter; the permission matrix rejects guest/remote calls. Duplicate, deletion/retry ordering, and the rebased stop/supervisor boundary yielded no new material defect.
- Hosted CI passes, including both ACL regressions and the relevant native/UI tests. Its merge commit
80c5972chas the same tree as this PR head. No local suites were rerun. - Remaining validation gap: CI ran on Linux; Windows was skipped. I have no verified macOS production-build or attended desktop/real-Keychain deletion evidence. Those remain acceptance checks, not an additional established code defect.
This is a review comment, not an approval. The prior changes request is obsolete because its sole blocker is fixed.
Carl, an automated reviewer, commenting via Wes’s GitHub account. Withdrawing my obsolete ACL changes request: 7ef89f6 registers/grants Delete and hosted CI passes the production-context regression. No approval is issued; remaining macOS acceptance limits are recorded in the re-review.
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
7ef89f6 to
b2ded6d
Compare
Why
The new Agents page only offers Edit. People need a quick way to make a new agent from existing settings and to remove an agent from this desktop.
What
How
The actions use the existing agent control and native store. Delete checks the saved revision and keeps a disabled card if credential removal fails, so the person can retry.
Risk
Delete is destructive on this device. It does not archive the relay identity or remove past messages; the dialog says so. The live macOS Keychain path has not been exercised against a real agent.
Testing
No manual desktop test yet. The focused UI and native fixture checks passed locally after rebasing onto main; the live app flow remains for the feedback round.
Bigger picture
Relay archival parity with old Buzz needs a separate protocol write path and a product decision.
Generated with Codex