Address review feedback from #52 and #57

.env.example

@@ -60,13 +60,6 @@ TINYCOMPUTER_LAB_SELF_EMAIL=
 # TINYCOMPUTER_BROWSER_ENDPOINT=http://127.0.0.1:9222
 # The travel fixture's address for browser_fixture.
 # TINYCOMPUTER_FIXTURE_URL=http://127.0.0.1:8000
-# The attested module the bus runners (scripts/lab, task_live, task_fixture)
-# load; scripts/build-module builds it and prints this path.
-# TINYCOMPUTER_MODULE=target/lab/libtinycomputer.dylib
-# Where scripts/build-module installs it instead of <target>/lab: the loader
-# refuses a module under a directory another user can write, so the Docker
-# runners use $HOME/.local/lib/tinycomputer.
-# TINYCOMPUTER_MODULE_DIR=
 # The planner model task_live asks for.
 # TINYCOMPUTER_PLANNER_MODEL=
 # The reasoning model task_live rescues a failed step with (default

crates/tinycomputer-browser/src/error/error_tests.rs

@@ -149,6 +149,8 @@ fn a_stale_ref_envelope_matches_the_desktop_recovery() {
             .suggestion
             .is_some_and(|s| s.contains("BrowserSnapshot"))
     );
+    // agent-browser could not resolve the ref, so nothing reached the page:
+    // retrying with a fresh ref cannot repeat an effect.
     assert_eq!(envelope.disposition.retry, RetryDisposition::Safe);
 }
 
@@ -158,6 +160,10 @@ fn a_timeout_may_have_reached_the_page() {
     assert_eq!(envelope.code, "TIMEOUT");
     assert_eq!(envelope.disposition.delivery, DeliveryDisposition::Unknown);
     assert!(envelope.suggestion.is_none());
+    // A click may already have landed: inspect before repeating it.
+    let hint = envelope.recovery.expect("a timeout has a way out");
+    assert_eq!(hint.strategy, "inspect_state_then_retry_original");
+    assert!(hint.requires_fresh_snapshot);
 }
 
 #[test]
@@ -173,12 +179,36 @@ fn a_refused_navigation_says_not_to_retry() {
             .suggestion
             .is_some_and(|s| s.starts_with("do not retry"))
     );
+    // The domain filter refuses before any navigation reaches the page.
     assert_eq!(
         envelope.disposition.delivery,
         DeliveryDisposition::NotDelivered
     );
 }
 
+#[test]
+fn only_failures_decided_before_the_page_claim_nothing_was_delivered() {
+    let before_the_page = |error: &Error| {
+        matches!(
+            error,
+            Error::NoSuchSession { .. }
+                | Error::NoSuchOutput { .. }
+                | Error::StaleRef { .. }
+                | Error::BlockedByPolicy { .. }
+        )
+    };
+    for error in every_variant() {
+        let expected = before_the_page(&error);
+        let name = error.to_string();
+        let delivery = error.envelope().disposition.delivery;
+        assert_eq!(
+            delivery == DeliveryDisposition::NotDelivered,
+            expected,
+            "{name}"
+        );
+    }
+}
+
 #[test]
 fn a_lost_connection_suggests_a_new_session() {
     let envelope = Error::connection_lost("closed").envelope();

crates/tinycomputer-browser/src/error/mod.rs

@@ -160,9 +160,12 @@ impl Error {
     /// The code is [`errors::code`] of the wire name — the desktop's spelling
     /// where the meaning is shared — the recovery hint is
     /// [`errors::recovery`]'s, and the full wire name rides in
-    /// `details.name` for a host that matches on it. A failure refused before
-    /// the browser was asked anything is marked not delivered, so a caller
-    /// knows retrying it cannot repeat an effect.
+    /// `details.name` for a host that matches on it. Only a failure decided
+    /// before anything reaches the page is marked not delivered, so a caller
+    /// knows retrying it cannot repeat an effect: an unknown session or
+    /// output (local lookups), an unresolvable ref, or a refused origin
+    /// (rejected inside agent-browser before any input is sent). Every other
+    /// failure's delivery stays unknown.
     ///
     /// # Examples
     ///
@@ -218,17 +221,21 @@ impl Error {
         }
     }
 
-    /// Whether this failure was decided before any command reached the
-    /// browser.
+    /// Whether this failure is always decided before anything reaches the
+    /// page, so repeating the call cannot repeat an effect: a session or an
+    /// output looked up locally and not found, a ref agent-browser could not
+    /// resolve, and a destination its domain filter refused — both of those
+    /// are rejected inside agent-browser before any input is sent to the page.
+    ///
+    /// Invalid input and a limit can also come back after work was done — a
+    /// capture that turned out too large — so their delivery stays unknown.
     fn refused_before_delivery(&self) -> bool {
         matches!(
             self,
-            Self::InvalidInput { .. }
-                | Self::NoSuchSession { .. }
+            Self::NoSuchSession { .. }
+                | Self::NoSuchOutput { .. }
                 | Self::StaleRef { .. }
                 | Self::BlockedByPolicy { .. }
-                | Self::NoSuchOutput { .. }
-                | Self::LimitExceeded { .. }
         )
     }
 

crates/tinycomputer-browser/src/reply/mod.rs

@@ -82,7 +82,12 @@ pub(crate) fn classify(message: &str) -> Error {
     {
         return Error::invalid_input(message);
     }
-    if lower.starts_with("evaluation error") || lower.contains("dialog is blocking the page") {
+    // A dialog in front of the page passes once someone answers it, so the
+    // command is not wrong — the page is not ready for it yet.
+    if lower.contains("dialog is blocking the page") {
+        return Error::not_actionable(message);
+    }
+    if lower.starts_with("evaluation error") {
         return Error::page(message);
     }
     Error::failed(message)

crates/tinycomputer-browser/src/reply/reply_tests.rs

@@ -91,7 +91,7 @@ fn engine_messages_map_to_what_the_caller_should_do() {
         }),
         (
             "A JavaScript confirm dialog is blocking the page: \"Leave?\"",
-            |e| matches!(e, Error::PageError { .. }),
+            |e| matches!(e, Error::NotActionable { .. }),
         ),
         ("something unexpected", |e| {
             matches!(e, Error::ModuleFailed { .. })

crates/tinycomputer-browser/src/sessions/mod.rs

@@ -6,7 +6,7 @@
 
 use std::collections::HashMap;
 use std::path::PathBuf;
-use std::sync::atomic::{AtomicU64, Ordering};
+use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 
 use serde_json::{Value, json};
@@ -32,6 +32,20 @@ pub struct Browser {
     outputs: Mutex<OutputStore>,
     scratch: PathBuf,
     counter: AtomicU64,
+    /// Sessions being launched: they count against [`MAX_SESSIONS`] from
+    /// the moment the limit is checked, so two concurrent opens cannot both
+    /// see room for the last slot.
+    opening: AtomicUsize,
+}
+
+/// One reserved launch slot, given back when the launch ends — inserted,
+/// failed, or dropped mid-launch by a cancelled caller.
+struct Reservation<'a>(&'a AtomicUsize);
+
+impl Drop for Reservation<'_> {
+    fn drop(&mut self) {
+        self.0.fetch_sub(1, Ordering::SeqCst);
+    }
 }
 
 struct Session {
@@ -102,6 +116,7 @@ impl Browser {
             outputs: Mutex::new(OutputStore::default()),
             scratch,
             counter: AtomicU64::new(0),
+            opening: AtomicUsize::new(0),
         }
     }
 
@@ -112,11 +127,18 @@ impl Browser {
     /// [`Error::LimitExceeded`] when [`MAX_SESSIONS`] are open, and whatever
     /// the engine reports when the browser cannot be launched or reached.
     pub async fn open_session(&self, options: SessionOptions) -> Result<SessionInfo> {
-        if self.lock_sessions()?.len() >= MAX_SESSIONS {
-            return Err(Error::LimitExceeded {
-                message: format!("at most {MAX_SESSIONS} browser sessions may be open"),
-            });
-        }
+        // Checked and reserved under the table's lock, so the check and the
+        // claim are one step for every concurrent caller.
+        let reservation = {
+            let sessions = self.lock_sessions()?;
+            if sessions.len() + self.opening.load(Ordering::SeqCst) >= MAX_SESSIONS {
+                return Err(Error::LimitExceeded {
+                    message: format!("at most {MAX_SESSIONS} browser sessions may be open"),
+                });
+            }
+            self.opening.fetch_add(1, Ordering::SeqCst);
+            Reservation(&self.opening)
+        };
         let id = SessionId::new(format!("s-{}", self.next()));
         let mut session = Session {
             engine: self.launcher.open(id.as_str()),
@@ -140,8 +162,12 @@ impl Browser {
         session.info.url = page.url;
         session.info.title = page.title;
         let info = session.info.clone();
-        self.lock_sessions()?
-            .insert(id, Arc::new(tokio::sync::Mutex::new(session)));
+        // The slot moves from the reservation to the table in one step under
+        // the lock, so no concurrent check ever counts this launch twice.
+        let mut sessions = self.lock_sessions()?;
+        sessions.insert(id, Arc::new(tokio::sync::Mutex::new(session)));
+        drop(reservation);
+        drop(sessions);
         Ok(info)
     }
 

crates/tinycomputer-browser/src/sessions/sessions_tests/lifecycle_tests.rs

@@ -81,3 +81,64 @@ fn the_default_scratch_space_is_private_to_the_process() {
     let browser = Browser::new(Arc::new(Fake::new()));
     assert!(format!("{browser:?}").contains(&std::process::id().to_string()));
 }
+
+/// An engine that yields before every reply, so concurrent launches
+/// interleave the way real ones do.
+#[derive(Debug)]
+struct Slow;
+
+impl crate::engine::Engine for Slow {
+    fn execute(&mut self, command: serde_json::Value) -> crate::engine::Reply<'_> {
+        Box::pin(async move {
+            tokio::task::yield_now().await;
+            crate::fake::default_reply(&command)
+        })
+    }
+}
+
+impl crate::engine::Launcher for Slow {
+    fn open(&self, _session: &str) -> Box<dyn crate::engine::Engine> {
+        Box::new(Slow)
+    }
+}
+
+#[tokio::test]
+async fn concurrent_opens_never_exceed_the_cap() {
+    let browser = Arc::new(Browser::with_scratch(Arc::new(Slow), scratch("race")));
+    let mut opens = tokio::task::JoinSet::new();
+    for _ in 0..MAX_SESSIONS + 4 {
+        let browser = browser.clone();
+        opens.spawn(async move {
+            browser
+                .open_session(SessionOptions {
+                    endpoint: Some("ws://127.0.0.1:9222".to_owned()),
+                    ..SessionOptions::default()
+                })
+                .await
+        });
+    }
+    let mut refused = 0;
+    while let Some(opened) = opens.join_next().await {
+        if matches!(opened.unwrap(), Err(Error::LimitExceeded { .. })) {
+            refused += 1;
+        }
+    }
+    assert_eq!(refused, 4);
+    assert_eq!(browser.list_sessions().await.unwrap().len(), MAX_SESSIONS);
+}
+
+#[tokio::test]
+async fn a_failed_launch_gives_its_slot_back() {
+    let fake =
+        Fake::scripted(|command| (command["action"] == "launch").then(|| failure("Chrome exited")));
+    let browser = Browser::with_scratch(Arc::new(fake), scratch("slot"));
+    // Every launch fails; none may be refused for want of a slot, which is
+    // what leaked reservations would cause after MAX_SESSIONS attempts.
+    for _ in 0..MAX_SESSIONS + 2 {
+        let error = browser
+            .open_session(SessionOptions::default())
+            .await
+            .unwrap_err();
+        assert!(!matches!(error, Error::LimitExceeded { .. }), "{error:?}");
+    }
+}

crates/tinycomputer-bus/src/browser/errors/errors_tests.rs

@@ -129,3 +129,29 @@ fn a_refused_navigation_offers_no_way_out() {
     }
     assert!(recovery(NO_SUCH_SESSION).is_some_and(|hint| !hint.retryable));
 }
+
+#[test]
+fn a_timeout_says_inspect_before_retrying() {
+    let hint = recovery(TIMEOUT).expect("a timeout has a way out");
+    assert_eq!(hint.strategy, "inspect_state_then_retry_original");
+    assert!(hint.retryable && hint.requires_fresh_snapshot);
+}
+
+#[test]
+fn a_page_error_says_change_the_request_not_repeat_it() {
+    let hint = recovery(PAGE_ERROR).expect("a page error has a way out");
+    assert_eq!(hint.strategy, "inspect_state_then_revise_request");
+    assert!(!hint.retryable && hint.requires_fresh_snapshot);
+}
+
+#[test]
+fn every_recoverable_name_has_a_recovery_hint() {
+    for name in NAMES {
+        if is_agent_recoverable(name) {
+            assert!(
+                recovery(name).is_some(),
+                "{name} is recoverable without a hint"
+            );
+        }
+    }
+}

crates/tinycomputer-bus/src/browser/errors/mod.rs

@@ -157,6 +157,13 @@ pub fn code(name: &str) -> &'static str {
 /// (`refresh_snapshot_then_retry_original`), so an agent recovering from a
 /// stale ref does not care which surface it was on.
 ///
+/// A timeout, which may have come after the action reached the page, is
+/// `inspect_state_then_retry_original`, never a blind retry: a click or a
+/// form submission may already have happened, and repeating it could
+/// duplicate the effect. A page error — a thrown script, a rejected command —
+/// is `inspect_state_then_revise_request` and not retryable: the same request
+/// fails the same way again.
+///
 /// # Examples
 ///
 /// ```
@@ -176,8 +183,10 @@ pub fn recovery(name: &str) -> Option<crate::RecoveryHint> {
     match name {
         STALE_REF => Some(hint("refresh_snapshot_then_retry_original", true, true)),
         NO_SUCH_ELEMENT => Some(hint("refresh_snapshot_then_choose_again", true, true)),
-        NOT_ACTIONABLE => Some(hint("inspect_state_then_retry_original", true, true)),
-        TIMEOUT => Some(hint("retry_original", true, false)),
+        NOT_ACTIONABLE | TIMEOUT => Some(hint("inspect_state_then_retry_original", true, true)),
+        // A thrown script or a rejected command fails the same way again:
+        // look at the page and change the request rather than repeat it.
+        PAGE_ERROR => Some(hint("inspect_state_then_revise_request", false, true)),
         INVALID_INPUT => Some(hint("fix_request_then_retry", false, false)),
         NO_SUCH_SESSION => Some(hint("open_session_then_retry_original", false, false)),
         NO_SUCH_OUTPUT => Some(hint("capture_again_then_read", false, false)),

crates/tinycomputer-bus/src/browser/names/mod.rs

@@ -92,7 +92,8 @@ pub mod methods {
     pub const SCREENSHOT: &str = "BrowserScreenshot";
 
     /// Reads one chunk of a held output: a screenshot this interface took,
-    /// or one a task view or report names.
+    /// or one a task's `TaskReport.artifacts` names (task views never carry
+    /// one).
     ///
     /// Takes a [`crate::browser::ReadOutputRequest`]; its `data` is an
     /// [`crate::browser::OutputChunk`].

crates/tinycomputer-bus/src/browser/names/names_tests.rs

@@ -10,8 +10,10 @@ use super::{INTERFACE, METHODS, OBJECT_PATH, methods};
 
 #[test]
 fn browser_members_share_the_module_interface() {
-    assert_eq!(INTERFACE, crate::names::INTERFACE);
-    assert_eq!(OBJECT_PATH, crate::names::OBJECT_PATH);
+    // Literals, not the aliases they are defined from: a change to the
+    // published identity must be made here on purpose.
+    assert_eq!(INTERFACE, "ai.tinyhumans.tinycomputer.Desktop");
+    assert_eq!(OBJECT_PATH, "/ai/tinyhumans/tinycomputer/Desktop");
 }
 
 #[test]