fix(shell): the jail test asks the sandbox's verdict on the hub, not the whole connect's

crates/kernel/src/kernel.rs

@@ -60,6 +60,9 @@ struct Io {
     /// host-managed server stops when it reaches EOF, which also happens
     /// when this process dies, so a crashed shell leaves no kernel behind.
     lifeline: Option<tokio::process::ChildStdin>,
+    /// The task copying the child's stderr into the [`Tail`]; it ends at
+    /// the end of the pipe, after the kernel's last line.
+    stderr: Option<tokio::task::JoinHandle<()>>,
 }
 
 /// Keeps the last lines the kernel wrote to stderr, to say why it exited.
@@ -116,7 +119,7 @@ fn start(launch: &Launch, network: &Network, log: &LogSink, tail: &Tail) -> Resu
                 return Err("the kernel's stdin/stdout are not piped".into());
             };
             // The kernel logs to stderr; it never carries protocol frames.
-            if let Some(stderr) = child.stderr.take() {
+            let stderr = child.stderr.take().map(|stderr| {
                 let log = log.clone();
                 let tail = tail.clone();
                 tokio::spawn(async move {
@@ -126,14 +129,14 @@ fn start(launch: &Launch, network: &Network, log: &LogSink, tail: &Tail) -> Resu
                         (log)(&format!("octos: {line}"));
                         tail.push(line);
                     }
-                });
-            }
+                })
+            });
             let lines = BufReader::new(Box::pin(stdout) as Pin<Box<dyn AsyncRead + Send>>).lines();
             Ok(if shared {
                 // Frames go over the WebSocket once it is up (`prepare_network`).
-                Io { writer: Box::pin(tokio::io::sink()), lines, running: Running::Child(child), lifeline: Some(stdin) }
+                Io { writer: Box::pin(tokio::io::sink()), lines, running: Running::Child(child), lifeline: Some(stdin), stderr }
             } else {
-                Io { writer: Box::pin(stdin), lines, running: Running::Child(child), lifeline: None }
+                Io { writer: Box::pin(stdin), lines, running: Running::Child(child), lifeline: None, stderr }
             })
         }
         #[cfg(target_env = "ohos")]
@@ -153,6 +156,7 @@ fn start(launch: &Launch, network: &Network, log: &LogSink, tail: &Tail) -> Resu
                 lines: BufReader::new(Box::pin(reader) as Pin<Box<dyn AsyncRead + Send>>).lines(),
                 running: Running::Embedded(task),
                 lifeline: None,
+                stderr: None,
             })
         }
         #[cfg(not(target_env = "ohos"))]
@@ -266,7 +270,7 @@ async fn exited(running: &mut Running) -> String {
 async fn stop(io: Io) {
     // Closing stdin stops either kind of child: the stdio pipe drains owned
     // turns on EOF; a host-managed server drains its connections and exits.
-    let Io { writer, lines, mut running, lifeline } = io;
+    let Io { writer, lines, mut running, lifeline, .. } = io;
     let grace = if lifeline.is_some() { SHARED_GRACE } else { STDIO_GRACE };
     drop(writer);
     drop(lines);
@@ -401,16 +405,18 @@ pub(crate) async fn supervise(
                                 }
                             }
                         }
-                        Ok(None) => break CloseReason::Exited(with_tail("the kernel closed its output", &tail)),
+                        Ok(None) => break CloseReason::Exited("the kernel closed its output".into()),
                         Err(e) => break CloseReason::Exited(format!("reading from the kernel failed: {e}")),
                     },
-                    why = exited(&mut io.running) => break CloseReason::Exited(with_tail(&why, &tail)),
+                    why = exited(&mut io.running) => break CloseReason::Exited(why),
                 }
             }};
             (log)(&format!("octos-core: stopping kernel {generation}: {reason}"));
+            let stderr = io.stderr.take();
             stop(io).await;
             match reason {
-                CloseReason::Failed(why) => CloseReason::Failed(with_tail(&why, &tail)),
+                CloseReason::Failed(why) => CloseReason::Failed(last_words(&why, stderr, &tail).await),
+                CloseReason::Exited(why) => CloseReason::Exited(last_words(&why, stderr, &tail).await),
                 other => other,
             }
         }
@@ -464,6 +470,24 @@ fn tool_list_reply(text: &str) -> Option<String> {
     })
 }
 
+/// How long a stopped kernel's stderr may take to reach its end. The pipe
+/// closes with the kernel; only a process it started that inherited the
+/// pipe can hold it open longer.
+const LAST_WORDS_GRACE: Duration = Duration::from_secs(2);
+
+/// `why` with the kernel's last words (the [`Tail`]), once `stderr`, the
+/// task reading them, has reached the end of the pipe. That task runs
+/// beside the supervisor, so lines the kernel wrote just before its stdout
+/// closed or it exited can still be on their way to the tail: read at once,
+/// the reason said "the kernel closed its output: fake kernel up" without
+/// the "asked to exit" the kernel wrote last.
+async fn last_words(why: &str, stderr: Option<tokio::task::JoinHandle<()>>, tail: &Tail) -> String {
+    if let Some(reader) = stderr {
+        let _ = tokio::time::timeout(LAST_WORDS_GRACE, reader).await;
+    }
+    with_tail(why, tail)
+}
+
 fn with_tail(why: &str, tail: &Tail) -> String {
     let tail = tail.text();
     if tail.is_empty() {

crates/kernel/tests/core.rs

@@ -238,7 +238,9 @@ async fn a_kernel_that_dies_closes_its_connections_with_its_last_words() {
     call(&mut a, "1", "session/list", json!({})).await;
     a.send(json!({"jsonrpc": "2.0", "method": "test/exit", "params": {}}).to_string()).unwrap();
     match next(&mut a).await {
-        Err(CloseReason::Exited(why)) => assert!(why.contains("asked to exit") || why.contains("exit"), "{why}"),
+        // The line it wrote to stderr last, whichever end the core saw
+        // first: its output closing or its exit status.
+        Err(CloseReason::Exited(why)) => assert!(why.contains("asked to exit"), "{why}"),
         other => panic!("expected Exited, got {other:?}"),
     }
     for _ in 0..100 {

crates/shell/src/approvals/mod.rs

@@ -130,6 +130,31 @@ impl Approvals {
     pub fn consent_granted(&self, app: &str) -> bool {
         self.consent.granted(app, self.router.hooks().grants_all(app))
     }
+    /// The relay's entry point (octos#2567) on these approvals: a call
+    /// needs an approval.
+    pub fn approval_requested(&mut self, app: &str, tool: ToolSpec, args: serde_json::Value, caller: Caller, context: RequestContext) -> Route {
+        self.router.approval_requested(app, tool, args, caller, context)
+    }
+    /// The relay installs itself; decisions made before are handed over
+    /// first.
+    pub fn set_relay(&mut self, mut relay: Box<dyn ApprovalRelay>) {
+        for (id, decision, reason) in self.queue.take() {
+            relay.approval_decided(&id, decision, &reason);
+        }
+        *self.external.lock().unwrap_or_else(|e| e.into_inner()) = Some(relay);
+    }
+    /// The AI bus's held `confirm: host` call as a request like any other.
+    pub fn bus_requested(&mut self, held: &crate::ai_bus::HeldCall) -> Route {
+        let mut tool = ToolSpec::host(&held.tool);
+        tool.auto_approvable = held.auto_approvable;
+        if held.command {
+            tool = tool.command();
+        }
+        let args = serde_json::from_str(&held.args).unwrap_or_else(|_| serde_json::Value::String(held.args.clone()));
+        // The pane is the person's own conversation with the system agent.
+        let context = RequestContext { call_id: held.key.clone(), trigger: Trigger::Person, ..RequestContext::default() };
+        self.approval_requested(&held.app, tool, args, Caller::SystemAgent, context)
+    }
 }
 
 static STATE: Mutex<Option<Approvals>> = Mutex::new(None);
@@ -160,34 +185,23 @@ pub fn now() -> u64 {
 
 // ------------------------------------------------------------ the relay
 
-/// The relay's entry point (octos#2567): a call needs an approval.
+/// The relay's entry point (octos#2567): a call needs an approval
+/// ([`Approvals::approval_requested`] on the shell's approvals).
 pub fn approval_requested(app: &str, tool: ToolSpec, args: serde_json::Value, caller: Caller, context: RequestContext) -> Route {
-    with(|a| a.router.approval_requested(app, tool, args, caller, context)).unwrap_or_else(|| Route::Refused("approvals are not set up".into()))
+    with(|a| a.approval_requested(app, tool, args, caller, context)).unwrap_or_else(|| Route::Refused("approvals are not set up".into()))
 }
 
-/// The relay installs itself; decisions made before are handed over first.
-pub fn set_relay(mut relay: Box<dyn ApprovalRelay>) {
-    with(|a| {
-        for (id, decision, reason) in a.queue.take() {
-            relay.approval_decided(&id, decision, &reason);
-        }
-        *a.external.lock().unwrap_or_else(|e| e.into_inner()) = Some(relay);
-    });
+/// The relay installs itself; decisions made before are handed over first
+/// ([`Approvals::set_relay`] on the shell's approvals).
+pub fn set_relay(relay: Box<dyn ApprovalRelay>) {
+    with(|a| a.set_relay(relay));
 }
 
 /// The AI bus's `confirm: host` calls (`ai_bus::Route::Approval`): each is
 /// a request like any other; the shell drains the answers here and
 /// releases the held call (`AiBus::release`).
 pub fn bus_requested(held: &crate::ai_bus::HeldCall) -> Route {
-    let mut tool = ToolSpec::host(&held.tool);
-    tool.auto_approvable = held.auto_approvable;
-    if held.command {
-        tool = tool.command();
-    }
-    let args = serde_json::from_str(&held.args).unwrap_or_else(|_| serde_json::Value::String(held.args.clone()));
-    // The pane is the person's own conversation with the system agent.
-    let context = RequestContext { call_id: held.key.clone(), trigger: Trigger::Person, ..RequestContext::default() };
-    approval_requested(&held.app, tool, args, Caller::SystemAgent, context)
+    with(|a| a.bus_requested(held)).unwrap_or_else(|| Route::Refused("approvals are not set up".into()))
 }
 
 pub fn take_bus_decisions() -> Vec<(RequestId, Decision, String)> {

crates/shell/src/approvals/tests.rs

@@ -993,32 +993,31 @@ fn only_settings_and_the_sheet_make_a_person_gesture() {
 
 // ---------------------------------------------------------------- the relay seam
 
+/// On approvals of its own, never the shell's (`super::with`): those are
+/// the process's, and the tests that run app agents keep their consent
+/// and relay there while this runs.
 #[test]
 fn the_relay_gets_decisions_made_before_it_was_installed() {
-    super::init_memory();
-    let route = super::approval_requested("os.mail", ToolSpec::host("mail.archive").not_auto_approvable(), json!({"id": 1}), Caller::SystemAgent, ctx("g1", Trigger::Person));
-    assert!(matches!(route, Route::Sheet(_)));
-    let (sheet, id) = super::with(|a| {
+    let mut a = super::Approvals::memory();
+    let front = |a: &super::Approvals| {
         let s = a.router.front_sheet().unwrap();
         (s.id, s.lines[0].request.clone())
-    })
-    .unwrap();
-    super::with(|a| a.router.answer(sheet, &id, Answer::Deny, &ApprovalGesture::sheet_tap(), T0)).unwrap().unwrap();
+    };
+    let route = a.approval_requested("os.mail", ToolSpec::host("mail.archive").not_auto_approvable(), json!({"id": 1}), Caller::SystemAgent, ctx("g1", Trigger::Person));
+    assert!(matches!(route, Route::Sheet(_)));
+    let (sheet, id) = front(&a);
+    a.router.answer(sheet, &id, Answer::Deny, &ApprovalGesture::sheet_tap(), T0).unwrap();
     let relay = RecordingRelay::default();
-    super::set_relay(Box::new(relay.clone()));
+    a.set_relay(Box::new(relay.clone()));
     assert_eq!(relay.take(), vec![(RequestId("g1".into()), Decision::Deny, "denied on the sheet".into())]);
     // From now on, straight to it.
-    super::approval_requested("os.mail", ToolSpec::host("mail.archive").not_auto_approvable(), json!({"id": 2}), Caller::SystemAgent, ctx("g2", Trigger::Person));
-    let (sheet, id) = super::with(|a| {
-        let s = a.router.front_sheet().unwrap();
-        (s.id, s.lines[0].request.clone())
-    })
-    .unwrap();
-    super::with(|a| a.router.answer(sheet, &id, Answer::Once, &ApprovalGesture::sheet_tap(), T0)).unwrap().unwrap();
+    a.approval_requested("os.mail", ToolSpec::host("mail.archive").not_auto_approvable(), json!({"id": 2}), Caller::SystemAgent, ctx("g2", Trigger::Person));
+    let (sheet, id) = front(&a);
+    a.router.answer(sheet, &id, Answer::Once, &ApprovalGesture::sheet_tap(), T0).unwrap();
     assert_eq!(relay.take().len(), 1);
     // The AI bus's held `confirm: host` calls come back to the shell, not
     // to the relay: the Terminal's `run` asks the person, never a rule.
-    super::with(|a| a.router.create_rule(&ApprovalGesture::settings_tap(), RuleDraft::everything("terminal", 30), T0)).unwrap().unwrap();
+    a.router.create_rule(&ApprovalGesture::settings_tap(), RuleDraft::everything("terminal", 30), T0).unwrap();
     let held = crate::ai_bus::HeldCall {
         key: "bus:w4:c1".into(),
         app: "terminal".into(),
@@ -1027,19 +1026,18 @@ fn the_relay_gets_decisions_made_before_it_was_installed() {
         auto_approvable: false,
         command: true,
     };
-    assert!(matches!(super::bus_requested(&held), Route::Sheet(_)));
-    let (sheet, id, caller, args) = super::with(|a| {
+    assert!(matches!(a.bus_requested(&held), Route::Sheet(_)));
+    let (sheet, id, caller, args) = {
         let s = a.router.front_sheet().unwrap();
         (s.id, s.lines[0].request.clone(), s.lines[0].caller.clone(), s.lines[0].args.join(" "))
-    })
-    .unwrap();
+    };
     assert_eq!(id, RequestId("bus:w4:c1".into()));
     assert_eq!(caller, "The system agent");
     assert_eq!(args, "command (1 line):   1 \u{2502} ls", "a command, one numbered row per line");
-    super::with(|a| a.router.answer(sheet, &id, Answer::Once, &ApprovalGesture::sheet_tap(), T0)).unwrap().unwrap();
+    a.router.answer(sheet, &id, Answer::Once, &ApprovalGesture::sheet_tap(), T0).unwrap();
     assert!(relay.take().is_empty());
-    assert_eq!(super::take_bus_decisions(), vec![(id, Decision::ApproveOnce, "approved on the sheet".into())]);
-    assert!(!super::consent_granted("os.news"), "developer mode is off in tests: nothing is granted");
+    assert_eq!(a.bus.take(), vec![(id, Decision::ApproveOnce, "approved on the sheet".into())]);
+    assert!(!a.consent_granted("os.news"), "developer mode is off in tests: nothing is granted");
 }
 
 /// The module host's gate: the first ask shows the first-use sheet and

crates/shell/src/apps.rs

@@ -141,6 +141,32 @@ pub fn system_card_apps() -> Vec<crate::clients::AppDef> {
     Vec::new()
 }
 
+/// System apps a test registers for itself (the glance tile tests'
+/// probes). They join App Hub's registry, the one [`system_card_apps`]
+/// lists, which is process-wide and never forgets an app, so a test of the
+/// build's own catalog saw them or not depending on which tests had run
+/// before it in the same process. A test registers its app here, and the
+/// catalog tests leave these out.
+#[cfg(test)]
+pub(crate) mod test_system_apps {
+    use std::sync::Mutex;
+
+    static IDS: Mutex<Vec<&'static str>> = Mutex::new(Vec::new());
+
+    /// Register `app` for a test. It is noted first, so a catalog that
+    /// lists it was read after it was noted.
+    #[cfg(feature = "app-hub")]
+    pub(crate) fn register(app: octosense_appstore::system::SystemApp) {
+        IDS.lock().unwrap_or_else(|e| e.into_inner()).push(app.id);
+        octosense_appstore::system::register_system_app(app);
+    }
+
+    /// Whether a test registered the system app `id` (an `os.` id).
+    pub(crate) fn is_test_app(id: &str) -> bool {
+        IDS.lock().unwrap_or_else(|e| e.into_inner()).contains(&id)
+    }
+}
+
 /// The services contained apps call through `host.request` (ADR 0004)
 /// that are not the assistant's, registered once, before the first system
 /// app can open: `mail` keeps accounts and passwords for the Mail app;
@@ -658,12 +684,19 @@ mod tests {
             .collect()
     }
 
+    /// The build's own rows among `rows`: without the system apps other
+    /// tests registered for themselves ([`super::test_system_apps`]).
+    /// `rows` must be read before this looks, as an argument is.
+    fn build_rows(rows: Vec<crate::clients::AppDef>) -> Vec<crate::clients::AppDef> {
+        rows.into_iter().filter(|row| !card_manifest_id(row).is_some_and(super::test_system_apps::is_test_app)).collect()
+    }
+
     #[cfg(feature = "mobile-apps")]
     #[test]
     fn bundled_apps_open_without_catalog_files_or_child_processes() {
         use makepad_widgets::*;
         let _one_rinx = crate::module_host::RINX_INSTANCE_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner());
-        let catalog = bundled_catalog();
+        let catalog = build_rows(bundled_catalog());
         // The linked modules in link order (AppCard is opt-in, `app-appcard`,
         // not part of `mobile-apps`; `settings` is the phone product's), then
         // the system apps no native module of the same id replaces.
@@ -718,7 +751,7 @@ mod tests {
         let mut cx = Cx::new(Box::new(|_, _| {}));
         cx.with_vm(makepad_widgets::script_mod);
         let mut host = crate::module_host::ModuleHost::default();
-        for (index, app) in bundled_catalog().iter().enumerate() {
+        for (index, app) in build_rows(bundled_catalog()).iter().enumerate() {
             let module = registry.module(&app.id).unwrap();
             let client = index as u64 + 1;
             host.create(&mut cx, client, module, module_open(module, app).unwrap(), dvec2(400.0, 700.0)).unwrap();
@@ -768,7 +801,7 @@ mod tests {
     fn the_system_apps_ship_as_card_apps() {
         let registry = AppRegistry::default();
         let native = registry.linked_ids();
-        let ids: Vec<String> = system_card_apps().into_iter().map(|app| app.id).collect();
+        let ids: Vec<String> = build_rows(system_card_apps()).into_iter().map(|app| app.id).collect();
         let expected: Vec<&str> = system_app_ids().iter().copied().filter(|id| !native.contains(id)).collect();
         assert_eq!(ids, expected);
         for id in &ids {

crates/shell/src/glance_card.rs

@@ -839,7 +839,8 @@ mod tests {
     }
 
     /// A system app, registered from a pack made on the fly, whose manifest
-    /// grants `glance` and `storage` and nothing else.
+    /// grants `glance` and `storage` and nothing else. It is registered as
+    /// a test's, so the catalog tests running beside these leave it out.
     #[cfg(feature = "app-hub")]
     fn register_test_app(id: &'static str) {
         let dir = std::env::temp_dir().join(format!("glance-tile-app-{id}-{}", std::process::id()));
@@ -852,7 +853,7 @@ mod tests {
         );
         std::fs::write(dir.join("manifest.json"), manifest).unwrap();
         let pack = serde_json::to_string(&octosense_app_hub::pack::pack_dir(&dir).unwrap()).unwrap();
-        octosense_appstore::system::register_system_app(octosense_appstore::system::SystemApp { id, name: "Glance test", pack: Box::leak(pack.into_boxed_str()), assets: &[] });
+        crate::apps::test_system_apps::register(octosense_appstore::system::SystemApp { id, name: "Glance test", pack: Box::leak(pack.into_boxed_str()), assets: &[] });
     }
 
     /// A Cx with the widgets and the tile frame registered, for driving