fix(agents): wait for native host operations - #331
Conversation
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
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: a443a48808
ℹ️ 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".
| pub(crate) struct AgentHost( | ||
| Arc<Mutex<Result<Host, String>>>, | ||
| Arc<AtomicBool>, | ||
| Arc<tokio::sync::Mutex<()>>, | ||
| ); |
There was a problem hiding this comment.
The reviewed commit contains no Signed-off-by trailer, so it violates the repository's mandatory DCO policy and cannot pass the hosted DCO Check. Ensure the actual author supplies a valid sign-off before this commit is submitted.
AGENTS.md reference: AGENTS.md:L153-L160
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review — via Wes’s account
No actionable defects found in the seven-file change at head a443a4880880851653c43cc21b51293b61b7b061, against base/merge-base 85d6bf82c54d1c8d930d58444597a1fe31cc8975.
Reviewed FIFO admission and worker-owned guard lifetime, Start/Stop and shutdown fencing, import/create credential phase boundaries, profile revision/publication guards, and the converted model-discovery and Goose-setup callers. The change stays within the contention fix rather than adding retries or changing launch policy. The added source tests use explicit gates for the ordering they assert; I did not execute them.
Validation limits: source-only review of hash-verified pinned files, with no dirty checkout inputs, builds, tests, app launches, or live credential/network workflows. One exact-head GitHub check snapshot showed the automatic Linux/browser lanes, CI required, DCO, Semgrep and zizmor successful; Windows validation was skipped. Native GUI, live Goose/Keychain behavior, staged-runtime credential/model integration, installed-Pi behavior and human acceptance remain unverified. Waiting behind slow host operations remains the documented tradeoff; this review does not establish a latency bound.
This is a non-blocking COMMENT review, not an approval or merge authorization.
Native commands now wait for host admission instead of rejecting with "Another native agent operation is in progress", so the frontend no longer retries that string for snapshot reads or log challenge/read. The initializing retry is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review — follow-up
Published through Wes’s account. No new actionable findings in the five-file delta since the previously reviewed a443a4880880851653c43cc21b51293b61b7b061 (prior review). This is not a fresh review of unrelated, unchanged code.
- Snapshot, log-challenge and log-read commands all use native waiting admission (
src-tauri/src/agents.rs:504–549), consistent with removing the obsolete frontend busy-error retries. - The bounded initializing retry, read coalescing, stale-result/disposal guards and no-automatic-write-replay behavior remain in place (
src/features/agents/control.ts:271–379). - Log access still obtains one target-bound nonce, authorizes it once and submits one read; native expiry and one-use consumption remain unchanged. The retained tests and polling/lifecycle callers were inspected as source, including native admission-order/cancellation coverage.
Pinned head: aa1467088fcbe3effa724b53ed4f48cf5970a22a
Pinned base: 85d6bf82c54d1c8d930d58444597a1fe31cc8975
Limits: Source-only review of hash-verified API blobs; no dirty checkout inputs, PR code/test execution, installs, app launches or live credential workflows. CI was not checked in this follow-up. Author-reported test/IPC results were not independently reproduced. Native GUI/Goose/Keychain behavior, staged credential/model integration, installed-Pi behavior, Windows and human acceptance remain unverified. Slow native operations can still delay the queue; this change adds no admission deadline. This COMMENT is not approval or merge authorization.
…ad-on-send * origin/main: (58 commits) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) fix(status): reopen a Today status as Today near 16:00 (#275) test: use current navigation for GIF send roundtrip (#309) Fix composer focus when selecting channels and DMs (#307) fix: retire mention searches after chips and refuted prose (#303) ... # Conflicts: # src/features/messages/MessageComposer.test.tsx # src/features/messages/MessageComposer.tsx
* origin/main: (45 commits) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentCard.tsx # src/bundled/agents/AgentsPage.tsx
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
* origin/main: (36 commits) Delay message timestamp tooltips by 500 ms (#321) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentEditor.tsx # src/bundled/profiles/ProfileAgentIdentity.test.tsx
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Why
A native status poll can make Create or Start fail with “Another native agent operation is in progress.” The rejected operation also leaves the UI showing unconfirmed status.
What
Make native agent commands wait for host access instead of rejecting ordinary contention. Keep this fix separate from create-and-start UI changes and snapshot performance work.
The frontend no longer retries “Another native agent operation is in progress” for snapshot reads or log challenge/read, because native no longer returns it. The initializing retry is unchanged.
How
Use FIFO admission before running each controller phase on a blocking worker. This keeps a queued Start preparation ahead of a later recovery Stop. Workers retain admission if their caller disappears. Credential and network waits release admission, and queued work rechecks shutdown before changing state. Failed commands are never replayed automatically.
Risk
This changes scheduling for all native agent commands. Slow existing lock holders can still delay commands; admission has no new deadline. Existing launch tickets, revision checks and profile publication guards remain in place.
Testing
Independent agent review found no blockers. Native GUI, live Goose/Keychain behavior and human acceptance remain pending; this PR stays draft.
After rebuilding the desktop from this branch, create a Goose agent, then Start and Stop it while Agents remains open. Polling should cause a brief wait instead of a busy error. Confirm Stop still cancels a pending credential-based launch.
Agent exercise at
aa146708: a temporary (uncommitted) Rust scenario drove Create prepare, Create commit, Start and Stop through real Tauri IPC with in-memory credentials and the synthetic runtime, while a second thread polledagent_control_snapshotcontinuously. Onmain(85d6bf8), 5/5 rounds failed with “Another native agent operation is in progress” and 2/3 polls were rejected. On this branch, 25/25 rounds succeeded, every Start snapshot showed the agent enabled, and 0/135 polls were rejected. The synthetic supervisor never confirms, so each Start waited out the existing 10s supervisor timeout while queued polls waited. It was not committed because it is timing-dependent and slow; the deterministic admission tests cover the same ordering.Also ran:
cargo test --lib agents(22 passed),cargo fmt --check,cargo clippy --all-targets, andvitest run src/features/agents src/bundled/agents(22 files, 244 tests). Frontend tests removed: the two log-retry-on-busy cases (behavior removed); the no-retry case is kept. The busy variants of the transient-read cases now use the initializing error.The staged-runtime credential and model integration checks, installed-Pi check, and Windows validation are deferred. Browser cases added/removed: 0.
Generated with Codex
🤖 Generated with Claude Code