agent: MCP skills follow-ups, and the serve suite back in CI

.github/workflows/ci.yml

@@ -64,12 +64,13 @@ jobs:
     # groups are stable, reproducible locally with the same `-E` expression, and a red shard names
     # the area outright.
     #
-    # agent-serve is deliberately not in this matrix. On this PR every harness for
-    # `binary(~serve)` compiled in ~2–4m then froze the runner: the test step stayed
-    # `in_progress` past job/step timeouts, logs never uploaded, and `needs.test`
-    # became `cancelled` so Check went red. The same 32 binaries pass under
-    # sequential `cargo test` locally in ~4 minutes. Leaving them in the required
-    # fan-in only reproduces the 30:00 death.
+    # agent-serve was left out of this matrix from #39 on, after its harnesses froze the runner (the
+    # test step stuck `in_progress` past every timeout). Out of CI they rotted unseen: by 2026-10 48
+    # of them failed or hung on main, almost all because the session-title request `serve` began
+    # making after a first run took a reply from the in-order mock model's script, shifting every
+    # later turn by one. With that fixed in the harness, and the runner-freezing causes since
+    # addressed (systemd-scoped test step, nextest terminate-after and global-timeout below), they
+    # run again — as their own shard, so a hang there names itself instead of hiding another's.
     name: test (${{ matrix.name }})
     runs-on: ubuntu-latest
     timeout-minutes: 25
@@ -103,6 +104,9 @@ jobs:
           - name: agent-rest
             filter: package(beyond-ai-agent) and kind(test) and not binary(~serve) and not binary(exec_endpoint_live)
             extra: ""
+          - name: agent-serve
+            filter: package(beyond-ai-agent) and kind(test) and binary(~serve)
+            extra: ""
           - name: core-providers
             filter: package(beyond-ai-agent-core) or package(beyond-ai-providers)
             extra: ""

crates/agent/src/bin/mcp_skills_fixture_server.rs

@@ -41,6 +41,8 @@
 //!   reached.
 //! - `MCP_SKILLS_FIXTURE_TTL_MS` (default `0`) and `MCP_SKILLS_FIXTURE_CACHE_SCOPE` (default
 //!   `public`): the caching hints on `skills/list` / `skills/get`.
+//! - `MCP_SKILLS_FIXTURE_PRIVATE_FLAG=<path>`: while that file exists, the scope is `private`, so a
+//!   test can turn a listing private after connect.
 //!
 //! Also served: `skill://git-workflow/nested-helper/SKILL.md`, a skill nested inside `git-workflow`
 //! (whose manifest lists its file, as completeness requires); and two skills over the spec's
@@ -139,8 +141,14 @@ fn cache_hints() -> (u64, String) {
         .ok()
         .and_then(|v| v.parse().ok())
         .unwrap_or(0);
-    let scope =
-        std::env::var("MCP_SKILLS_FIXTURE_CACHE_SCOPE").unwrap_or_else(|_| "public".to_string());
+    let private_now = std::env::var("MCP_SKILLS_FIXTURE_PRIVATE_FLAG")
+        .is_ok_and(|p| std::path::Path::new(&p).exists());
+    let scope = match private_now {
+        true => "private".to_string(),
+        false => {
+            std::env::var("MCP_SKILLS_FIXTURE_CACHE_SCOPE").unwrap_or_else(|_| "public".to_string())
+        }
+    };
     (ttl, scope)
 }
 

crates/agent/src/lib.rs

@@ -33,6 +33,8 @@ pub mod service;
 pub mod session_store;
 pub mod settings;
 pub mod skills;
+#[cfg(test)]
+pub(crate) mod test_support;
 pub mod timing;
 pub mod tools;
 #[cfg(test)]

crates/agent/src/resources.rs

@@ -1832,7 +1832,7 @@ mod tests {
 
     #[tokio::test]
     async fn load_context_files_via_walks_home_then_every_ancestor_nearest_last() {
-        let tmp = tempfile::tempdir().unwrap();
+        let tmp = crate::test_support::isolated_tempdir();
         let home = tmp.path().join("home");
         let repo = tmp.path().join("box/repo");
         let ws = repo.join("app");
@@ -1853,7 +1853,7 @@ mod tests {
 
     #[tokio::test]
     async fn load_context_files_via_treats_an_empty_file_as_absent() {
-        let tmp = tempfile::tempdir().unwrap();
+        let tmp = crate::test_support::isolated_tempdir();
         let ws = tmp.path().join("ws");
         fs::create_dir_all(&ws).unwrap();
         fs::write(ws.join("AGENTS.md"), "   \n").unwrap();

crates/agent/src/serve.rs

@@ -148,7 +148,9 @@
 //!     active tip and advance the tip to it → `data: {id}` (the new entry's id); `data` defaults to `{}`
 //!     when omitted, `kind` identifies the shape of `data` to whatever produced it (this crate never
 //!     interprets either) — `SessionStore::append_custom`, fully built and tested but previously
-//!     unreachable from any RPC command; same persistence-required error as `set_label`
+//!     unreachable from any RPC command; same persistence-required error as `set_label`. A kind the
+//!     agent writes itself (`session_store::HOST_CUSTOM_KINDS`) is refused, so a client cannot forge
+//!     one
 //!   - `{type:"export_html", output_path?}` render the active session's transcript as a single
 //!     self-contained HTML file → `data: {path}`. `output_path` defaults to a timestamped
 //!     `session-<unix-seconds>.html` in the current directory; parent directories are created as
@@ -2380,7 +2382,7 @@ pub(crate) async fn scan_session_dirs(
             .flat_map(|dir| crate::session_store::scan_session_dir_in(dir, &layout))
             .collect();
         let mut metas = crate::session_store::scan_listings_in(paths, &layout, &on_progress);
-        metas.sort_by(|a, b| b.updated_at.cmp(&a.updated_at));
+        metas.sort_by(crate::session_store::by_recency);
         metas
     })
     .await
@@ -5065,7 +5067,7 @@ pub(crate) async fn serve_session(
                                             // must be answerable mid-run. `accepted:false` tells the losing
                                             // client in a multi-attach race that its answer arrived too late.
                                             "approve" => {
-                                                let _ = out_tx.send(handle_approve(cid, &c, &pending_approvals));
+                                                let _ = out_tx.send(handle_approve(cid, &c, &pending_approvals, approval.is_some()));
                                             }
                                             "elicit" => {
                                                 let _ = out_tx.send(handle_elicit(cid, &c, Some(&pending_elicitations)));
@@ -6533,6 +6535,18 @@ pub(crate) async fn serve_session(
                 )),
             },
             "append_custom" => match cmd.get("kind").and_then(Value::as_str) {
+                // The host's own journal kinds: an entry of one is trusted when the session is
+                // replayed (a task to resume, a result to deliver), so a client must not be able to
+                // write one.
+                Some(kind) if crate::session_store::is_host_custom_kind(kind) => emit!(response(
+                    id,
+                    "append_custom",
+                    false,
+                    None,
+                    Some(&format!(
+                        "custom entry kind `{kind}` is reserved for entries the agent writes itself"
+                    ))
+                )),
                 Some(kind) => {
                     let data = cmd.get("data").cloned().unwrap_or_else(|| json!({}));
                     match persistence.append_custom(kind, data) {
@@ -6576,7 +6590,12 @@ pub(crate) async fn serve_session(
             // Reachable while idle only for a stale/duplicate answer (`accepted:false`): a real question
             // can only be outstanding while a run is in flight, which is the busy arm above.
             "approve" => {
-                emit!(handle_approve(id, &cmd, &pending_approvals));
+                emit!(handle_approve(
+                    id,
+                    &cmd,
+                    &pending_approvals,
+                    approval.is_some()
+                ));
             }
             "get_todos" => {
                 // Straight from the session while idle — no mirror needed, and no chance of one going
@@ -10253,7 +10272,17 @@ fn approval_resolved_frame(
 /// `accepted` is `false` when the `request_id` names no outstanding question — it was already answered
 /// by another attached client, it timed out, or the run was aborted. That is not an error: it is the
 /// answer a client races and loses.
-fn handle_approve(id: Option<String>, cmd: &Value, pending: &PendingApprovals) -> OutFrame {
+///
+/// `tool_gate` is whether this session asks about tool calls (`--approve`). Without one, the only
+/// questions it can raise are MCP skills' — so an answer that matches none of them is refused with a
+/// pointer to `--approve` rather than acknowledged, since a client sending it is most likely expecting
+/// tool approvals this session will never ask for.
+fn handle_approve(
+    id: Option<String>,
+    cmd: &Value,
+    pending: &PendingApprovals,
+    tool_gate: bool,
+) -> OutFrame {
     let Some(request_id) = cmd.get("request_id").and_then(Value::as_str) else {
         return response(id, "approve", false, None, Some("missing `request_id`"));
     };
@@ -10262,6 +10291,18 @@ fn handle_approve(id: Option<String>, cmd: &Value, pending: &PendingApprovals) -
         Err(e) => return response(id, "approve", false, None, Some(&e)),
     };
     let accepted = resolve_approval(pending, request_id, decision);
+    if !accepted && !tool_gate {
+        return response(
+            id,
+            "approve",
+            false,
+            None,
+            Some(
+                "no such question: this session asks only about MCP skills; tool approvals need \
+                 `--approve`",
+            ),
+        );
+    }
     response(
         id,
         "approve",
@@ -12900,7 +12941,7 @@ mod tests {
     async fn git_branch_reports_none_outside_a_git_repository() {
         // Task #25 (pi-parity fix): a lookup failure (no repo here at all) must report `None`, never
         // an error surfaced to the RPC caller.
-        let dir = tempfile::tempdir().unwrap();
+        let dir = crate::test_support::isolated_tempdir();
         assert_eq!(git_branch(dir.path()).await, None);
     }
 

crates/agent/src/session_store.rs

@@ -339,13 +339,40 @@ pub fn search_sessions(sessions: Vec<SessionMeta>, query: Option<&str>) -> Vec<S
         })
         .collect();
     ranked.sort_by(|(a_meta, a_rank), (b_meta, b_rank)| {
-        a_rank
-            .cmp(b_rank)
-            .then_with(|| b_meta.updated_at.cmp(&a_meta.updated_at))
+        a_rank.cmp(b_rank).then_with(|| by_recency(a_meta, b_meta))
     });
     ranked.into_iter().map(|(m, _)| m).collect()
 }
 
+/// Custom-entry kinds the agent writes itself, and acts on when a session is replayed: an MCP task to
+/// resume and a resumed task's result (`crate::mcp_resume`), and `mcp_skill_approval`, which is no
+/// longer written (an approval read back from disk is one a model with file-write tools could have
+/// planted, so approvals are never persisted) but stays reserved so no client can make an older
+/// reader trust one. A client's `append_custom` of any of these is refused. A new host-written kind
+/// belongs here.
+pub const HOST_CUSTOM_KINDS: &[&str] = &[
+    crate::mcp_resume::TASK_ENTRY_KIND,
+    crate::mcp_resume::RESULT_ENTRY_KIND,
+    "mcp_skill_approval",
+];
+
+/// Whether `kind` is one of [`HOST_CUSTOM_KINDS`].
+pub fn is_host_custom_kind(kind: &str) -> bool {
+    HOST_CUSTOM_KINDS.contains(&kind)
+}
+
+/// Most recently active first; ties (same second — `updated_at` has one-second resolution, so a
+/// burst of sessions shares one) broken by id, descending, so a listing has one order however the
+/// directory happened to be scanned. Without the tie-break, `offset` paging over sessions written in
+/// the same second could serve one session twice and skip another. Descending, because a generated
+/// id ([`new_id`]) leads with its creation time in fixed-width hex nanoseconds: within the second the
+/// newest still comes first. (A client-chosen id has no such meaning, but still one stable order.)
+pub(crate) fn by_recency(a: &SessionMeta, b: &SessionMeta) -> std::cmp::Ordering {
+    b.updated_at
+        .cmp(&a.updated_at)
+        .then_with(|| b.id.cmp(&a.id))
+}
+
 /// One persisted line. Internally tagged on `type`. A `Message` entry flattens the wrapped
 /// `agent_core::Message` (`{role, content}`) alongside its tree fields, so the wire shape is
 /// `{"type":"message", id, parent_id, role, content}` — `agent_core::Message` itself carries no tree
@@ -2877,7 +2904,7 @@ impl SessionRepo {
             );
         }
         let mut metas = scan_listings(paths, &on_progress);
-        metas.sort_by(|a, b| b.updated_at.cmp(&a.updated_at));
+        metas.sort_by(by_recency);
         Ok(metas)
     }
 
@@ -2933,7 +2960,7 @@ impl SessionRepo {
             }
         }
         let mut metas = scan_listings_in(paths, self.layout(), &on_progress);
-        metas.sort_by(|a, b| b.updated_at.cmp(&a.updated_at));
+        metas.sort_by(by_recency);
         Ok(metas)
     }
 
@@ -8831,6 +8858,32 @@ mod tests {
         m
     }
 
+    /// Sessions active in the same second (one-second `updated_at`) still have one order, and it is
+    /// newest-first: a generated id leads with its creation time, so the later one sorts first. With
+    /// no tie-break (or an ascending one) `offset` paging over them could repeat or skip a session.
+    #[test]
+    fn same_second_sessions_list_newest_first_in_one_stable_order() {
+        let ids: Vec<String> = (0..4).map(|_| new_id()).collect();
+        let mut metas: Vec<SessionMeta> = ids
+            .iter()
+            .map(|id| meta_for_search(id, None, "x", 100))
+            .collect();
+        metas.push(meta_for_search("older", None, "x", 99));
+        // Oldest first in, whatever the scan order was.
+        metas.sort_by(by_recency);
+        let got: Vec<&str> = metas.iter().map(|m| m.id.as_str()).collect();
+        let mut want: Vec<&str> = ids.iter().rev().map(String::as_str).collect();
+        want.push("older");
+        assert_eq!(got, want);
+        // The same order from any starting order.
+        metas.reverse();
+        metas.sort_by(by_recency);
+        assert_eq!(
+            metas.iter().map(|m| m.id.as_str()).collect::<Vec<_>>(),
+            want
+        );
+    }
+
     #[test]
     fn search_sessions_with_no_query_returns_the_input_unchanged_and_in_order() {
         let sessions = vec![

crates/agent/src/skills.rs

@@ -1513,7 +1513,7 @@ mod tests {
 
     #[test]
     fn find_git_repo_root_returns_none_when_no_git_repo_encloses_it() {
-        let tmp = tempfile::tempdir().unwrap();
+        let tmp = crate::test_support::isolated_tempdir();
         let deep = tmp.path().join("a/b/c");
         fs::create_dir_all(&deep).unwrap();
         assert_eq!(find_git_repo_root(&deep), None);
@@ -1541,7 +1541,7 @@ mod tests {
     #[test]
     fn collect_ancestor_agents_skill_dirs_walks_to_the_filesystem_root_when_no_git_repo_encloses_it()
      {
-        let tmp = tempfile::tempdir().unwrap();
+        let tmp = crate::test_support::isolated_tempdir();
         let start = tmp.path().join("a/b");
         fs::create_dir_all(&start).unwrap();
         let dirs = collect_ancestor_agents_skill_dirs(&start);

crates/agent/src/test_support.rs

@@ -0,0 +1,67 @@
+//! Shared helpers for this crate's unit tests.
+
+use std::path::Path;
+
+/// What makes a directory "inside a project" to the code under test: an enclosing git repo, a
+/// context file, or a project skills root anywhere above it.
+const PROJECT_MARKERS: [&str; 4] = [".git", "CLAUDE.md", "AGENTS.md", ".agents"];
+
+/// Whether `dir` or any ancestor carries a [project marker](PROJECT_MARKERS).
+fn inside_a_project(dir: &Path) -> bool {
+    dir.ancestors()
+        .any(|a| PROJECT_MARKERS.iter().any(|m| a.join(m).exists()))
+}
+
+/// A temp directory that no ancestor walk can escape into a real project.
+///
+/// Tests of "no enclosing repo" / "no context file above" behaviour walk to the filesystem root, so
+/// a plain `tempfile::tempdir()` makes them depend on where `TMPDIR` points: under a git checkout or
+/// a `CLAUDE.md` (a worktree's own `target/tmp`, say) they find the surrounding project and fail.
+/// This picks the first candidate root with no project above it — `TMPDIR` itself when it is clean.
+///
+/// # Panics
+/// When every candidate root sits inside a project, naming them, rather than letting the test
+/// pass or fail for a reason that has nothing to do with it.
+pub(crate) fn isolated_tempdir() -> tempfile::TempDir {
+    let candidates = [
+        std::env::temp_dir(),
+        "/tmp".into(),
+        "/var/tmp".into(),
+        "/dev/shm".into(),
+    ];
+    for root in &candidates {
+        let Ok(root) = root.canonicalize() else {
+            continue;
+        };
+        if inside_a_project(&root) {
+            continue;
+        }
+        if let Ok(dir) = tempfile::tempdir_in(&root) {
+            return dir;
+        }
+    }
+    panic!(
+        "no temp root outside a project (git repo, CLAUDE.md, AGENTS.md or .agents above it) among \
+         {candidates:?}"
+    );
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn an_isolated_tempdir_has_no_project_above_it() {
+        let dir = isolated_tempdir();
+        assert!(!inside_a_project(dir.path()), "{}", dir.path().display());
+    }
+
+    #[test]
+    fn a_marker_anywhere_above_counts() {
+        let dir = isolated_tempdir();
+        let deep = dir.path().join("a/b");
+        std::fs::create_dir_all(&deep).unwrap();
+        std::fs::write(dir.path().join("CLAUDE.md"), "x").unwrap();
+        assert!(inside_a_project(&deep));
+    }
+}