Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions prompts/en/worker.md.j2
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ Automate a headless Chrome browser. Use this for web scraping, testing web inter
3. `snapshot` — Get the page's accessibility tree with element refs (e1, e2, e3...)
4. `act` — Interact with elements by ref: `click`, `type`, `press_key`, `hover`, `scroll_into_view`, `focus`
5. `screenshot` — Capture the page or a specific element
{%- if browser_persist_session %}
6. `close` — Detach from the browser when done (tabs and session are preserved for the next worker)
{%- else %}
6. `close` — Shut down the browser when done
{%- endif %}
Comment on lines +103 to +107
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Drive this instruction from the effective close policy, not browser_persist_session.

persist_session=true only changes the default. If config explicitly sets close_policy = "close_browser" or close_policy = "close_tabs", this text is now wrong and will tell the worker to preserve tabs/session when it will not. Please render against the resolved ClosePolicy and add a close_tabs branch as well.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@prompts/en/worker.md.j2` around lines 103 - 107, Update the templated
sentence to render from the resolved ClosePolicy rather than the
browser_persist_session flag: use the effective close policy variable (e.g.,
ClosePolicy or close_policy) that the runtime/template resolves and add explicit
branches for close_browser, close_tabs and close_session (or the equivalent
values) so the text reads "close — Shut down the browser when done" for
close_browser, "close — Detach from tabs but end the session when done" for
close_tabs, and "close — Detach from the browser when done (tabs and session are
preserved for the next worker)" for persistent sessions; replace the
browser_persist_session conditional with these close_policy branches in
prompts/en/worker.md.j2 and ensure the template uses the resolved policy
variable name used elsewhere in the codebase.


**Multi-tab support:** Use `open` to create new tabs, `tabs` to list them, `focus` to switch between them, `close_tab` to close one.

Expand Down
6 changes: 4 additions & 2 deletions src/agent/channel_dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ pub async fn spawn_worker_from_state(
None => Vec::new(),
};

let browser_config = (**rc.browser_config.load()).clone();
let worker_system_prompt = prompt_engine
.render_worker_prompt(
&rc.instance_dir.display().to_string(),
Expand All @@ -361,10 +362,10 @@ pub async fn spawn_worker_from_state(
sandbox_read_allowlist,
sandbox_write_allowlist,
&tool_secret_names,
browser_config.persist_session,
)
.map_err(|e| AgentError::Other(anyhow::anyhow!("{e}")))?;
let skills = rc.skills.load();
let browser_config = (**rc.browser_config.load()).clone();
let brave_search_key = (**rc.brave_search_key.load()).clone();

// Append skills listing to worker system prompt. Suggested skills are
Expand Down Expand Up @@ -887,6 +888,7 @@ pub async fn resume_idle_worker_into_state(
Some(store) => store.tool_secret_names(),
None => Vec::new(),
};
let browser_config = (**rc.browser_config.load()).clone();
let system_prompt = prompt_engine
.render_worker_prompt(
&rc.instance_dir.display().to_string(),
Expand All @@ -896,9 +898,9 @@ pub async fn resume_idle_worker_into_state(
sandbox_read_allowlist,
sandbox_write_allowlist,
&tool_secret_names,
browser_config.persist_session,
)
.map_err(|error| format!("failed to render worker prompt: {error}"))?;
let browser_config = (**rc.browser_config.load()).clone();
let brave_search_key = (**rc.brave_search_key.load()).clone();

let (worker, input_tx) = Worker::resume_interactive(
Expand Down
3 changes: 2 additions & 1 deletion src/agent/cortex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2398,6 +2398,7 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho
None => Vec::new(),
};

let browser_config = (**deps.runtime_config.browser_config.load()).clone();
let worker_system_prompt = prompt_engine
.render_worker_prompt(
&deps.runtime_config.instance_dir.display().to_string(),
Expand All @@ -2407,6 +2408,7 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho
sandbox_read_allowlist,
sandbox_write_allowlist,
&tool_secret_names,
browser_config.persist_session,
)
.map_err(|error| anyhow::anyhow!("failed to render worker prompt: {error}"))?;

Expand Down Expand Up @@ -2440,7 +2442,6 @@ async fn pickup_one_ready_task(deps: &AgentDeps, logger: &CortexLogger) -> anyho
tracing::warn!(%error, path = %logs_dir.display(), "failed to create logs directory");
}

let browser_config = (**deps.runtime_config.browser_config.load()).clone();
let brave_search_key = (**deps.runtime_config.brave_search_key.load()).clone();
let worker = Worker::new(
None,
Expand Down
30 changes: 26 additions & 4 deletions src/config/load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,21 @@ fn parse_close_policy(value: Option<&str>) -> Option<ClosePolicy> {
}
}

/// Resolve the effective close policy. When `persist_session` is enabled and no
/// explicit `close_policy` was provided, default to `Detach` so browser tabs and
/// cookies survive across workers.
fn resolve_close_policy(
explicit: Option<&str>,
persist_session: bool,
fallback: ClosePolicy,
) -> ClosePolicy {
parse_close_policy(explicit).unwrap_or(if persist_session {
ClosePolicy::Detach
} else {
fallback
})
}

impl CortexConfig {
fn resolve(overrides: TomlCortexConfig, defaults: CortexConfig) -> CortexConfig {
CortexConfig {
Expand Down Expand Up @@ -1423,8 +1438,11 @@ impl Config {
.map(PathBuf::from)
.or_else(|| base.screenshot_dir.clone()),
persist_session: b.persist_session.unwrap_or(base.persist_session),
close_policy: parse_close_policy(b.close_policy.as_deref())
.unwrap_or(base.close_policy),
close_policy: resolve_close_policy(
b.close_policy.as_deref(),
b.persist_session.unwrap_or(base.persist_session),
base.close_policy,
),
chrome_cache_dir: chrome_cache_dir.clone(),
}
})
Expand Down Expand Up @@ -1600,8 +1618,12 @@ impl Config {
persist_session: b
.persist_session
.unwrap_or(defaults.browser.persist_session),
close_policy: parse_close_policy(b.close_policy.as_deref())
.unwrap_or(defaults.browser.close_policy),
close_policy: resolve_close_policy(
b.close_policy.as_deref(),
b.persist_session
.unwrap_or(defaults.browser.persist_session),
defaults.browser.close_policy,
),
chrome_cache_dir: defaults.browser.chrome_cache_dir.clone(),
}),
channel: a.channel.and_then(|channel_config| {
Expand Down
2 changes: 2 additions & 0 deletions src/prompts/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ impl PromptEngine {
sandbox_read_allowlist: Vec<String>,
sandbox_write_allowlist: Vec<String>,
tool_secret_names: &[String],
browser_persist_session: bool,
) -> Result<String> {
self.render(
"worker",
Expand All @@ -264,6 +265,7 @@ impl PromptEngine {
sandbox_read_allowlist => sandbox_read_allowlist,
sandbox_write_allowlist => sandbox_write_allowlist,
tool_secret_names => tool_secret_names,
browser_persist_session => browser_persist_session,
},
)
}
Expand Down
72 changes: 55 additions & 17 deletions src/tools/browser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,14 @@ pub struct BrowserState {
element_refs: HashMap<String, ElementRef>,
/// Counter for generating element refs.
next_ref: usize,
/// Per-launch temp directory for Chrome's user data. Cleaned up on drop to
/// prevent stale singleton locks from blocking subsequent launches.
/// Chrome's user data directory. For ephemeral sessions this is a random
/// temp dir cleaned up on drop. For persistent sessions this is a stable
/// path under the instance dir that survives agent restarts.
user_data_dir: Option<PathBuf>,
/// When true, `user_data_dir` is a stable path that should NOT be deleted
/// on drop — it holds cookies, localStorage, and login sessions that must
/// survive across agent restarts.
persistent_profile: bool,
}

impl BrowserState {
Expand All @@ -176,6 +181,7 @@ impl BrowserState {
element_refs: HashMap::new(),
next_ref: 0,
user_data_dir: None,
persistent_profile: false,
}
}
}
Expand All @@ -184,6 +190,13 @@ impl Drop for BrowserState {
fn drop(&mut self) {
// Browser and handler task are dropped automatically —
// chromiumoxide's Child has kill_on_drop(true).

// Persistent profiles store cookies, localStorage, and login sessions
// that must survive across agent restarts — never delete them.
if self.persistent_profile {
return;
}

if let Some(dir) = self.user_data_dir.take() {
// Offload sync fs cleanup to a blocking thread so we don't stall
// the tokio worker that's dropping this state.
Expand All @@ -199,7 +212,12 @@ impl Drop for BrowserState {
});
} else {
// Dropped outside a tokio runtime (unlikely) — clean up inline.
let _ = std::fs::remove_dir_all(&dir);
if let Err(error) = std::fs::remove_dir_all(&dir) {
eprintln!(
"failed to clean up browser user data dir {}: {error}",
dir.display()
);
}
}
}
}
Expand All @@ -212,6 +230,7 @@ impl std::fmt::Debug for BrowserState {
.field("pages", &self.pages.len())
.field("active_target", &self.active_target)
.field("element_refs", &self.element_refs.len())
.field("persistent_profile", &self.persistent_profile)
.finish()
}
}
Expand Down Expand Up @@ -554,10 +573,16 @@ impl BrowserTool {
// 4. Auto-download via BrowserFetcher (cached in chrome_cache_dir)
let executable = resolve_chrome_executable(&self.config).await?;

// Use a unique temp dir per launch to avoid singleton lock collisions
// when multiple workers launch browsers or a previous session crashed.
let user_data_dir =
std::env::temp_dir().join(format!("spacebot-browser-{}", uuid::Uuid::new_v4()));
// Persistent sessions use a stable profile dir under chrome_cache_dir so
// cookies, localStorage, and login sessions survive across agent restarts.
// Ephemeral sessions use a random temp dir to avoid singleton lock collisions.
let (user_data_dir, persistent_profile) = if self.config.persist_session {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Persistent sessions now share a stable user-data-dir; if two workers call launch concurrently, Chrome can fail to start due to the profile lock even though another launch is in-flight/just succeeded. Consider serializing persistent launches (e.g., a launch_in_progress flag) or, on launch failure, re-checking state.browser and reconnecting before returning an error.

(self.config.chrome_cache_dir.join("profile"), true)
} else {
Comment on lines +576 to +581
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Scope the persistent profile path to the agent/site, not the whole instance.

Line 575 hard-codes {chrome_cache_dir}/profile. chrome_cache_dir is populated from {instance_dir}/chrome_cache during config loading, while the shared browser handle is per-agent. By default, that means two agents with persist_session = true can reuse the same on-disk profile, leaking cookies/login state across agents and contending on Chrome's profile lock. Please derive a stable path that is unique per agent/site, even if that means precomputing it during config load and passing it through BrowserConfig.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tools/browser.rs` around lines 571 - 576, The code currently uses a
hard-coded profile path by joining self.config.chrome_cache_dir with "profile",
which can cause different agents to share the same on-disk Chrome profile;
instead, add a per-agent/site stable profile path into the BrowserConfig during
config loading (e.g., config.persistent_profile_dir or config.agent_profile_dir)
and replace the join("profile") usage in the browser construction (the
user_data_dir/persistent_profile branch where self.config.persist_session is
checked) to use that precomputed per-agent path; ensure the code still sets
persistent_profile = true when persist_session is enabled and falls back to
ephemeral temp dirs when not.

let dir =
std::env::temp_dir().join(format!("spacebot-browser-{}", uuid::Uuid::new_v4()));
(dir, false)
};

let mut builder = ChromeConfig::builder()
.no_sandbox()
Expand Down Expand Up @@ -594,12 +619,21 @@ impl BrowserTool {
// the browser we just created and return success.
drop(browser);
handler_task.abort();
// Clean up the temp user data dir asynchronously so we don't block
// while holding the shared mutex.
let dir = user_data_dir;
tokio::spawn(async move {
let _ = tokio::fs::remove_dir_all(&dir).await;
});
// Clean up the temp user data dir — but only for ephemeral sessions.
// Persistent profiles use a shared stable path that the winner is
// actively using.
if !persistent_profile {
let dir = user_data_dir;
tokio::spawn(async move {
if let Err(error) = tokio::fs::remove_dir_all(&dir).await {
tracing::debug!(
path = %dir.display(),
%error,
"failed to clean up browser user data dir (concurrent launch race)"
);
}
});
}

if self.config.persist_session {
return self.reconnect_existing_tabs(&mut state).await;
Expand All @@ -610,6 +644,7 @@ impl BrowserTool {
state.browser = Some(browser);
state._handler_task = Some(handler_task);
state.user_data_dir = Some(user_data_dir);
state.persistent_profile = persistent_profile;

tracing::info!("browser launched");
Ok(BrowserOutput::success("Browser launched successfully"))
Expand Down Expand Up @@ -1196,16 +1231,17 @@ impl BrowserTool {
ClosePolicy::CloseBrowser => {
// Take everything out of state under the lock, then do the
// actual teardown outside it.
let (browser, handler_task, user_data_dir) = {
let (browser, handler_task, user_data_dir, persistent_profile) = {
let mut state = self.state.lock().await;
let browser = state.browser.take();
let handler_task = state._handler_task.take();
let user_data_dir = state.user_data_dir.take();
let persistent_profile = state.persistent_profile;
state.pages.clear();
state.active_target = None;
state.element_refs.clear();
state.next_ref = 0;
(browser, handler_task, user_data_dir)
(browser, handler_task, user_data_dir, persistent_profile)
};

if let Some(task) = handler_task {
Expand All @@ -1220,8 +1256,10 @@ impl BrowserTool {
return Err(BrowserError::new(message));
}

// Clean up the per-launch user data dir to free disk space.
if let Some(dir) = user_data_dir {
// Clean up the user data dir — but only for ephemeral sessions.
// Persistent profiles hold cookies and login sessions that must
// survive browser restarts.
if !persistent_profile && let Some(dir) = user_data_dir {
tokio::spawn(async move {
if let Err(error) = tokio::fs::remove_dir_all(&dir).await {
tracing::debug!(
Expand Down
8 changes: 4 additions & 4 deletions tests/context_dump.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ async fn dump_worker_context() {
let prompt_engine = rc.prompts.load();
let instance_dir = rc.instance_dir.to_string_lossy();
let workspace_dir = rc.workspace_dir.to_string_lossy();
let browser_config = (**rc.browser_config.load()).clone();
let worker_prompt = prompt_engine
.render_worker_prompt(
&instance_dir,
Expand All @@ -365,13 +366,11 @@ async fn dump_worker_context() {
Vec::new(),
Vec::new(),
&[],
browser_config.persist_session,
)
.expect("failed to render worker prompt");
print_section("WORKER SYSTEM PROMPT", &worker_prompt);
print_stats("System prompt", &worker_prompt);

// Build the actual worker tool server
let browser_config = (**rc.browser_config.load()).clone();
let brave_search_key = (**rc.brave_search_key.load()).clone();
let worker_id = uuid::Uuid::new_v4();

Expand Down Expand Up @@ -530,6 +529,7 @@ async fn dump_all_contexts() {
println!("--- TOTAL BRANCH: ~{} tokens ---", branch_total / 4);

// ── Worker ──
let browser_config = (**rc.browser_config.load()).clone();
let worker_prompt = prompt_engine
.render_worker_prompt(
&instance_dir,
Expand All @@ -539,9 +539,9 @@ async fn dump_all_contexts() {
Vec::new(),
Vec::new(),
&[],
browser_config.persist_session,
)
.expect("failed to render worker prompt");
let browser_config = (**rc.browser_config.load()).clone();
let brave_search_key = (**rc.brave_search_key.load()).clone();
let worker_tool_server = spacebot::tools::create_worker_tool_server(
deps.agent_id.clone(),
Expand Down