fix(acp): isolate and tear down the /browser Chrome session#505
Open
nori-sessions[bot] wants to merge 3 commits into
Open
fix(acp): isolate and tear down the /browser Chrome session#505nori-sessions[bot] wants to merge 3 commits into
nori-sessions[bot] wants to merge 3 commits into
Conversation
The 'CDP endpoint: http://127.0.0.1:<port>' parser was reimplemented in mock-acp-agent and tui-pty-e2e (both as u16, against the house no-unsigned rule). Extract one i32 parser into a new non-gated nori-acp module, backend::browser_cdp::extract_cdp_port_from_prompt, and have both test crates depend on nori-acp and call it. A round-trip test guards the parser against compose_agent_prompt's output.
Two browser-lifecycle bugs from the PR review: - launch reused the user's default Chrome profile (no --user-data-dir), so launching while Chrome was already running handed off to that instance and never exposed CDP, hanging until the 15s timeout. Launch against a throwaway tempfile::TempDir profile instead. - BrowserSession lives in a process-lifetime static, which Rust never drops at exit, so the Drop SIGTERM and kill_on_drop never fired and Chrome was orphaned. Add browser_session::shutdown_active_session(), called from run_ratatui_app after the event loop returns, which drops the session (kill_on_drop kills Chrome, TempDir removes the profile). Remove the now-redundant unsafe Drop impl. A hard kill / terminal close that bypasses the exit path can still orphan Chrome (documented). tempfile promoted from dev- to cfg(unix) dependency for the profile dir.
3 tasks
CSRessel
added a commit
that referenced
this pull request
Jun 8, 2026
## Summary Goal-notice fixes for nori-client MCP goal resume, split out from the original combined cleanup PR (the browser-CDP fixes now live in #505 targeting main). - Surface the goal-unavailable notice for **all** in-play goals on resume, not just the first. - Stop recording the goal-unavailable notice to the transcript (it is a transient UI notice, not conversation history). - Document the goal-notice gating and non-recording-on-resume behavior. - Share a panic-safe `EnvGuard` across the goal resume tests. Targets `feat/nori-client-mcp-cleanup`. ## Test plan - [x] `cargo test -p nori-acp` (574 pass, with the feat-branch mock agent built) - [x] `cargo clippy -p nori-acp --tests` (clean) - [x] `cargo fmt --check`
Extend the /browser command with three Chrome profile tiers, secure by default with explicit power-user opt-ins: - Throwaway (default): fresh temp profile per launch, wiped on shutdown; shares no cookies/logins/settings with the user's real Chrome. - Persistent: a nori-owned <nori_home>/browser-profile, kept across launches so logins survive, isolated from real Chrome. - System: the user's real default Chrome profile, with all their logins. /browser now opens a 3-way picker pre-selected on the saved default; choosing a tier persists it (`[tui] browser_profile`) and launches with it. The System tier reuses the real profile, so an already-running Chrome silently hands the launch off and never exposes CDP; the launch detects that and returns a precise "fully quit Chrome" hint instead of a generic timeout. Profile resolution and directory lifetime live in the new backend/browser_profile.rs (throwaway owns a TempDir; persistent/system are kept on disk). Adds config serde, picker, persistence, and dir resolution tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/browserChrome launch in a throwawaytempfile::TempDirprofile (--user-data-dir) so sessions never share cookies/state, and tear the browser down deterministically:kill_on_drop(true)on the child plus an explicitshutdown_active_session()invoked from the TUI shutdown path before terminal restore.nori_acp::backend::browser_cdp::extract_cdp_port_from_prompt, replacing the duplicated parsers inmock-acp-agentandtui-pty-e2eso all three crates key off the same"CDP endpoint: http://127.0.0.1:"format.These are the browser-related fixes split out of #503 (which now targets only the nori-client MCP cleanup). No nori-client/MCP changes are included here.
Test plan
cargo test -p nori-acp(566 pass)cargo clippy -p nori-acp --testsand-p nori-tui --tests(clean)cargo build --bin noricargo test -p tui-pty-e2e(display-requiring browser test stays#[ignore])cargo fmt --check