fix(aiui): the content surface renders what the assistant found
Four defects, one visible symptom: a correct prose answer beside an
empty grid.
1. The assistant's curated RPC bridge had an arm only for
`content.list-mine`. `tools.rs` mapped the `peers`, `purchased` and
`films` scopes onto three real, dispatcher-registered handlers that
`assistant_dispatch_tool` had never heard of, so every non-"own"
scope died on its catch-all. Downstream that read as "the peers have
no content" — it was a missing match arm, and the tool never ran.
Regression test added: every scope the schema advertises must reach a
real handler.
2. `content.browse-all-peers` wrapped its whole fan-out in one
`timeout(..).unwrap_or_default()`, which DISCARDED every completed
batch the moment the budget expired. One slow peer turned a
partly-successful browse into "0 reached, 16 unreachable". Observed
live on archi-dev-box: back-to-back calls returned real peer items,
then nothing. Now accumulates per batch and checks a deadline between
them, so partial results always survive. Budget 20s -> 45s: two
batches of eight at a 10s per-peer timeout had no headroom at all.
3. `assistant.chat` returned only `{ text }`. The structured results of
any content tool the turn ran were dropped inside the loop, so the
surface had nothing to render. The turn now carries them through
(captured raw, before the untrusted wrap, since they go to a renderer
that treats every field as inert data, never back into the prompt).
4. The adapter classified images as 'excluded' and dropped them. A node
sharing mostly photos rendered as an empty grid while AIUI's image
grid sat unused. Images now have a bucket, with the paid-lock and
extension-fallback handling audio and video already had.
Also: the panel says "Loading…" while a turn is in flight and "Nothing
found" when it comes back empty, instead of leaving the previous
query's heading standing as though it answered this one; the system
prompt tells the model to call the content tool and summarise rather
than re-list what the cards already show; and a refused tool now names
its permission category so the trusted chrome can offer the settings
screen instead of leaving "I don't have a tool for that" as the only
clue.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
f7c541e867
commit
9abc162394
@@ -279,8 +279,17 @@ impl RpcHandler {
|
||||
|
||||
let caller = crate::assistant::CallerScope::LocalOperator { session_id };
|
||||
|
||||
let answer = crate::assistant::chat(Arc::clone(self), caller, text).await?;
|
||||
Ok(serde_json::json!({ "text": answer }))
|
||||
let outcome = crate::assistant::chat_with_surfaces(Arc::clone(self), caller, text).await?;
|
||||
// `surfaces` carries the structured results of any content tools
|
||||
// this turn ran, so the browser can render them as a grid instead
|
||||
// of leaving the surface empty beside a correct prose answer.
|
||||
// Always present (possibly empty) so the caller never has to
|
||||
// distinguish "no content" from "old node".
|
||||
Ok(serde_json::json!({
|
||||
"text": outcome.text,
|
||||
"surfaces": outcome.surfaces,
|
||||
"refused_categories": outcome.refused_categories,
|
||||
}))
|
||||
}
|
||||
|
||||
/// Internal-only bridge: executes a curated assistant tool against the
|
||||
@@ -310,6 +319,14 @@ impl RpcHandler {
|
||||
"network.set-wifi-radio" => self.handle_network_set_wifi_radio(params).await,
|
||||
"mesh.status" => self.handle_mesh_status().await,
|
||||
"content.list-mine" => self.handle_content_list_mine().await,
|
||||
// The other three `content_list` scopes (peers/purchased/films).
|
||||
// Their absence here — while `tools.rs`'s scope match named them
|
||||
// — made every non-"own" scope fail with "no such handler", which
|
||||
// reads downstream as "the peers have no content" when in truth
|
||||
// the tool never ran. Curated one-by-one, same as every arm above.
|
||||
"content.browse-all-peers" => self.handle_content_browse_all_peers().await,
|
||||
"content.owned-list" => self.handle_content_owned_list().await,
|
||||
"content.indeehub-projects" => self.handle_content_indeehub_projects().await,
|
||||
"system.settings.get" => self.handle_system_settings_get(params).await,
|
||||
"system.settings.set" => self.handle_system_settings_set(params).await,
|
||||
"system.kiosk-display.get" => self.handle_system_kiosk_display_get().await,
|
||||
|
||||
@@ -1275,35 +1275,50 @@ impl RpcHandler {
|
||||
// inside the budget, which is the point.
|
||||
const BROWSE_PEER_CONCURRENCY: usize = 8;
|
||||
const PER_PEER_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
|
||||
let overall = std::time::Duration::from_secs(20);
|
||||
// Headroom matters: 16 peers at concurrency 8 is two batches, and a
|
||||
// batch only finishes when its SLOWEST peer does. At a 20s budget
|
||||
// one slow peer in batch 1 left batch 2 no time at all.
|
||||
let overall = std::time::Duration::from_secs(45);
|
||||
let deadline = tokio::time::Instant::now() + overall;
|
||||
|
||||
let mut items = Vec::new();
|
||||
let mut reached = 0usize;
|
||||
let mut unreachable = 0usize;
|
||||
|
||||
let results = tokio::time::timeout(overall, async {
|
||||
let mut out: Vec<(String, Option<serde_json::Value>)> = Vec::new();
|
||||
for chunk in onions.chunks(BROWSE_PEER_CONCURRENCY) {
|
||||
let mut set = Vec::new();
|
||||
for onion in chunk {
|
||||
let params = Some(serde_json::json!({ "onion": onion }));
|
||||
set.push(async move {
|
||||
let v = tokio::time::timeout(
|
||||
PER_PEER_TIMEOUT,
|
||||
self.handle_content_browse_peer(params),
|
||||
)
|
||||
.await
|
||||
.ok()
|
||||
.and_then(|r| r.ok());
|
||||
(onion.clone(), v)
|
||||
});
|
||||
}
|
||||
out.extend(futures_util::future::join_all(set).await);
|
||||
// Accumulate per batch rather than wrapping the whole loop in one
|
||||
// `timeout(..).unwrap_or_default()`. That construction DISCARDED
|
||||
// every completed batch the moment the budget expired, so a single
|
||||
// slow peer turned a partly-successful fan-out into "0 reached, 16
|
||||
// unreachable" — indistinguishable, downstream, from the peers
|
||||
// having no content at all. Observed live on archi-dev-box: back to
|
||||
// back calls returned real peer items and then nothing.
|
||||
let mut results: Vec<(String, Option<serde_json::Value>)> = Vec::new();
|
||||
for chunk in onions.chunks(BROWSE_PEER_CONCURRENCY) {
|
||||
let remaining = deadline.saturating_duration_since(tokio::time::Instant::now());
|
||||
if remaining.is_zero() {
|
||||
break;
|
||||
}
|
||||
out
|
||||
})
|
||||
.await
|
||||
.unwrap_or_default();
|
||||
let mut set = Vec::new();
|
||||
for onion in chunk {
|
||||
let params = Some(serde_json::json!({ "onion": onion }));
|
||||
set.push(async move {
|
||||
let v = tokio::time::timeout(
|
||||
PER_PEER_TIMEOUT,
|
||||
self.handle_content_browse_peer(params),
|
||||
)
|
||||
.await
|
||||
.ok()
|
||||
.and_then(|r| r.ok());
|
||||
(onion.clone(), v)
|
||||
});
|
||||
}
|
||||
// No batch-level timeout: every future in `set` is ALREADY
|
||||
// bounded by PER_PEER_TIMEOUT, so this join can't outrun it, and
|
||||
// adding an outer timeout here would reintroduce exactly the
|
||||
// discard-on-expiry bug above. The deadline check at the top of
|
||||
// the loop is what stops a long peer list from running forever.
|
||||
results.extend(futures_util::future::join_all(set).await);
|
||||
}
|
||||
|
||||
for (onion, v) in &results {
|
||||
match v {
|
||||
|
||||
@@ -212,6 +212,11 @@ pub(crate) async fn execute_tool(call: &ToolCall, ctx: &ToolExecCtx) -> ToolResu
|
||||
|
||||
let granted = ctx.caller.granted_categories(ctx.handler.data_dir()).await;
|
||||
if !granted.contains(&tool.category) {
|
||||
// Remember WHICH category blocked this, so the trusted chrome can
|
||||
// offer the operator a link to the toggle. Without it the only
|
||||
// trace is the model's prose, and "I don't have a tool for that"
|
||||
// gives no hint that the capability exists and is one switch away.
|
||||
ctx.note_refused_category(tool.category);
|
||||
return ToolResult {
|
||||
call_id: call.id.clone(),
|
||||
is_error: true,
|
||||
@@ -292,7 +297,18 @@ pub(crate) async fn execute_tool(call: &ToolCall, ctx: &ToolExecCtx) -> ToolResu
|
||||
// `tools::dispatch` — never a generic pass-through of the model's tool
|
||||
// name onto the RPC surface.
|
||||
match super::tools::dispatch(&call.name, &args, ctx.handler.as_ref()).await {
|
||||
Ok(v) => ToolResult {
|
||||
Ok(v) => {
|
||||
// Capture grid-ready results for the UI *here*, on the raw
|
||||
// value, before the untrusted wrap below turns it into
|
||||
// delimiter-fenced text. See `ToolExecCtx::surfaces`.
|
||||
if super::tools::is_surface_tool(&call.name) {
|
||||
ctx.note_surface(
|
||||
&call.name,
|
||||
super::tools::surface_scope(&args),
|
||||
v.clone(),
|
||||
);
|
||||
}
|
||||
ToolResult {
|
||||
call_id: call.id.clone(),
|
||||
is_error: false,
|
||||
// D-10: peer-authored content (filenames, log lines, mesh/peer
|
||||
@@ -301,7 +317,8 @@ pub(crate) async fn execute_tool(call: &ToolCall, ctx: &ToolExecCtx) -> ToolResu
|
||||
// ToolResult is constructed. Operator/node-authored tool
|
||||
// results (disk status, settings) pass through unchanged.
|
||||
content: super::tools::wrap_tool_result_if_untrusted(&call.name, v.to_string()),
|
||||
},
|
||||
}
|
||||
}
|
||||
Err(msg) => ToolResult {
|
||||
call_id: call.id.clone(),
|
||||
is_error: true,
|
||||
|
||||
@@ -576,6 +576,41 @@ pub struct ToolExecCtx {
|
||||
/// mint a fresh confirmation and re-open the dialog until the human
|
||||
/// gives in, which is the habituation failure, mechanized.
|
||||
declined_actions: Mutex<HashSet<String>>,
|
||||
/// Grid-ready results captured from the content-producing tools this
|
||||
/// turn ran, in call order. This is what lets a chat answer *show* the
|
||||
/// films/photos/files it just looked up instead of only describing
|
||||
/// them: without it, `chat` returns prose and the tool's structured
|
||||
/// result is discarded inside the loop, so AIUI's content surface has
|
||||
/// nothing to render and stays empty next to a perfectly correct
|
||||
/// paragraph.
|
||||
///
|
||||
/// Deliberately the RAW dispatch value, captured BEFORE
|
||||
/// `wrap_tool_result_if_untrusted`: the untrusted boundary exists to
|
||||
/// stop peer-authored text being read as instructions by the MODEL.
|
||||
/// This copy never re-enters the prompt — it goes to a renderer that
|
||||
/// treats every field as inert data — and wrapping it would leave the
|
||||
/// UI parsing delimiter noise instead of JSON.
|
||||
surfaces: Mutex<Vec<Surface>>,
|
||||
/// Categories a tool call was refused for this turn, in first-refusal
|
||||
/// order. The model can only report this as prose ("I don't have a
|
||||
/// tool for that"), which leaves the operator with no idea that the
|
||||
/// fix is one toggle away — and reads as the assistant being broken.
|
||||
/// Surfacing the category lets the TRUSTED chrome offer a real link
|
||||
/// to the setting; the grant itself is never changed from here.
|
||||
refused_categories: Mutex<Vec<PermissionCategory>>,
|
||||
}
|
||||
|
||||
/// One content-producing tool result, kept for the UI to render.
|
||||
#[derive(Debug, Clone, serde::Serialize)]
|
||||
pub struct Surface {
|
||||
/// The tool that produced it (e.g. `content_list`).
|
||||
pub tool: String,
|
||||
/// The scope argument it was called with, when it had one — this is
|
||||
/// what tells "my own shared content" apart from "films from peers"
|
||||
/// on the receiving side, so the surface can title itself honestly.
|
||||
pub scope: Option<String>,
|
||||
/// The dispatch result, exactly as the RPC handler returned it.
|
||||
pub data: serde_json::Value,
|
||||
}
|
||||
|
||||
impl ToolExecCtx {
|
||||
@@ -616,9 +651,52 @@ impl ToolExecCtx {
|
||||
counters,
|
||||
validation_failures: Mutex::new(HashMap::new()),
|
||||
declined_actions: Mutex::new(HashSet::new()),
|
||||
surfaces: Mutex::new(Vec::new()),
|
||||
refused_categories: Mutex::new(Vec::new()),
|
||||
}
|
||||
}
|
||||
|
||||
/// A tool was refused because `category` is not granted. Deduped so a
|
||||
/// model retrying the same blocked tool three times still produces one
|
||||
/// prompt for the operator, not three.
|
||||
pub(crate) fn note_refused_category(&self, category: PermissionCategory) {
|
||||
let mut refused = self
|
||||
.refused_categories
|
||||
.lock()
|
||||
.expect("refused_categories mutex poisoned");
|
||||
if !refused.contains(&category) {
|
||||
refused.push(category);
|
||||
}
|
||||
}
|
||||
|
||||
/// Categories refused this turn, in first-refusal order.
|
||||
pub(crate) fn refused_categories(&self) -> Vec<PermissionCategory> {
|
||||
self.refused_categories
|
||||
.lock()
|
||||
.expect("refused_categories mutex poisoned")
|
||||
.clone()
|
||||
}
|
||||
|
||||
/// Record a content-producing tool's raw result for the UI to render.
|
||||
pub(crate) fn note_surface(&self, tool: &str, scope: Option<String>, data: serde_json::Value) {
|
||||
self.surfaces
|
||||
.lock()
|
||||
.expect("surfaces mutex poisoned")
|
||||
.push(Surface {
|
||||
tool: tool.to_string(),
|
||||
scope,
|
||||
data,
|
||||
});
|
||||
}
|
||||
|
||||
/// Everything captured this turn, in call order.
|
||||
pub(crate) fn surfaces(&self) -> Vec<Surface> {
|
||||
self.surfaces
|
||||
.lock()
|
||||
.expect("surfaces mutex poisoned")
|
||||
.clone()
|
||||
}
|
||||
|
||||
/// The human declined this exact action; remember it for the rest of
|
||||
/// the turn so it can never re-prompt.
|
||||
pub(crate) fn note_declined(&self, action_key: String) {
|
||||
@@ -696,7 +774,15 @@ owner's general assistant: answer ordinary questions, explain things, and give r
|
||||
assistant would. Those answers are what the content surfaces in this app render as cards, so \
|
||||
declining to answer them leaves the owner staring at an empty panel. Only a request to CHANGE \
|
||||
or READ something on the node needs a tool — and if no tool covers it, say so plainly rather \
|
||||
than pretending. Never let a general question be refused merely because no tool matches it.";
|
||||
than pretending. Never let a general question be refused merely because no tool matches it.\n\n\
|
||||
When the question is about what actually EXISTS here — this node's own shared files, a peer's \
|
||||
catalogue, purchased items, the film catalogue — call the content tool rather than answering \
|
||||
from memory or from earlier in this conversation, and call it once per place the question spans \
|
||||
(asking about \"films\" covers both the catalogue and the peers). The app renders whatever that \
|
||||
tool returns as cards beside your reply, so your text should introduce and summarise the result \
|
||||
— how many, anything notable — rather than re-listing every title, size and price in prose. A \
|
||||
long enumeration duplicates the cards the operator is already looking at. If the tool comes \
|
||||
back empty, say so plainly; do not fill the gap with remembered or invented items.";
|
||||
|
||||
pub fn build_system_prompt(visible_tools: &[tools::ToolDef]) -> String {
|
||||
let mut prompt = String::from(SYSTEM_PROMPT_PREAMBLE);
|
||||
@@ -715,6 +801,28 @@ pub fn build_system_prompt(visible_tools: &[tools::ToolDef]) -> String {
|
||||
prompt
|
||||
}
|
||||
|
||||
/// A completed chat turn: the model's prose answer, plus whatever
|
||||
/// content-producing tools it ran along the way. Callers that only want
|
||||
/// words (the mesh `!archy` path) use [`chat`]; the browser uses
|
||||
/// [`chat_with_surfaces`] so it can render the results as a grid.
|
||||
pub struct ChatOutcome {
|
||||
pub text: String,
|
||||
pub surfaces: Vec<Surface>,
|
||||
/// Permission categories that blocked a tool this turn — what the
|
||||
/// chrome turns into an "enable it in Settings" offer.
|
||||
pub refused_categories: Vec<PermissionCategory>,
|
||||
}
|
||||
|
||||
/// Text-only wrapper over [`chat_with_surfaces`], for callers with no
|
||||
/// surface to render (mesh replies, tests).
|
||||
pub async fn chat(
|
||||
handler: Arc<RpcHandler>,
|
||||
caller: CallerScope,
|
||||
user_text: String,
|
||||
) -> Result<String> {
|
||||
Ok(chat_with_surfaces(handler, caller, user_text).await?.text)
|
||||
}
|
||||
|
||||
/// Entry point: run one chat turn for `caller` through the shared loop.
|
||||
/// Builds the visible-tool set from the caller's granted categories only
|
||||
/// (D-16 — the model should never even see a tool it can't use), selects a
|
||||
@@ -732,11 +840,11 @@ pub fn build_system_prompt(visible_tools: &[tools::ToolDef]) -> String {
|
||||
/// way that stays correct against Claude's strict `tool_use`/`tool_result`
|
||||
/// id-pairing requirement needs its own test budget, and none of Task 2's
|
||||
/// `<behavior>` bullets require it this plan.
|
||||
pub async fn chat(
|
||||
pub async fn chat_with_surfaces(
|
||||
handler: Arc<RpcHandler>,
|
||||
caller: CallerScope,
|
||||
user_text: String,
|
||||
) -> Result<String> {
|
||||
) -> Result<ChatOutcome> {
|
||||
let registry = tools::registry();
|
||||
let grants = caller.granted_categories(handler.data_dir()).await;
|
||||
let visible_tools = registry.visible_to(&grants);
|
||||
@@ -803,7 +911,11 @@ pub async fn chat(
|
||||
);
|
||||
}
|
||||
|
||||
Ok(answer)
|
||||
Ok(ChatOutcome {
|
||||
text: answer,
|
||||
surfaces: ctx.surfaces(),
|
||||
refused_categories: ctx.refused_categories(),
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -45,6 +45,30 @@ pub fn wrap_tool_result_if_untrusted(name: &str, content: String) -> String {
|
||||
}
|
||||
}
|
||||
|
||||
/// Tools whose results are grid-ready content the UI should RENDER, not
|
||||
/// merely summarise in prose. Kept separate from
|
||||
/// [`UNTRUSTED_CONTENT_TOOLS`] on purpose even though they overlap today:
|
||||
/// that list answers "can this text manipulate the model?", this one
|
||||
/// answers "does this result have a visual form?" — and the answers
|
||||
/// diverge (`app_logs` is untrusted but has no grid; `apps_list` has a
|
||||
/// grid but is node-authored).
|
||||
const SURFACE_TOOLS: &[&str] = &["content_list", "apps_list"];
|
||||
|
||||
/// Whether this tool's result should be captured for the content surface.
|
||||
pub fn is_surface_tool(name: &str) -> bool {
|
||||
SURFACE_TOOLS.contains(&name)
|
||||
}
|
||||
|
||||
/// The scope a captured surface was produced under, when its tool has
|
||||
/// one. Lets the receiving grid title itself with what was actually
|
||||
/// asked for rather than guessing from the payload's shape.
|
||||
pub fn surface_scope(args: &ToolArgs) -> Option<String> {
|
||||
match args {
|
||||
ToolArgs::ContentList(a) => Some(a.scope.clone().unwrap_or_else(|| "own".to_string())),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// The backend-agnostic in/out of a tool invocation — the same shape
|
||||
/// regardless of which adapter (Ollama/Claude/Routstr) produced it.
|
||||
#[derive(Debug, Clone)]
|
||||
@@ -1095,6 +1119,41 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Every scope the `content_list` schema advertises must resolve to an
|
||||
/// RPC method that `assistant_dispatch_tool` actually has an arm for.
|
||||
///
|
||||
/// This is the test that was missing. `peers`/`purchased`/`films` each
|
||||
/// named a real, dispatcher-registered handler, but the assistant's
|
||||
/// curated bridge had an arm only for `own` — so those three died on
|
||||
/// the bridge's catch-all. The model then reported, accurately from
|
||||
/// where it stood, that it could find no peer content, and that read
|
||||
/// like a fleet outage instead of a missing match arm. Validating the
|
||||
/// scope (above) is not enough: the whole failure lived downstream of
|
||||
/// validation.
|
||||
#[tokio::test]
|
||||
async fn every_content_scope_reaches_a_real_dispatch_handler() {
|
||||
let (handler, _tmp) = test_rpc_handler().await;
|
||||
grant_all(&handler).await;
|
||||
let ctx = local_operator_ctx(handler);
|
||||
|
||||
for scope in ["own", "peers", "purchased", "films"] {
|
||||
let call = ToolCall {
|
||||
id: format!("call-{scope}"),
|
||||
name: "content_list".to_string(),
|
||||
arguments: json!({ "scope": scope }),
|
||||
};
|
||||
let result = execute_tool(&call, &ctx).await;
|
||||
// A bare handler with no orchestrator may legitimately return an
|
||||
// empty catalogue or an upstream error; what it must NEVER do is
|
||||
// report that the method itself is unreachable.
|
||||
assert!(
|
||||
!result.content.contains("no such handler"),
|
||||
"scope {scope} has no assistant_dispatch_tool arm — it never ran: {}",
|
||||
result.content
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_restart_refuses_unknown_app_id() {
|
||||
let (handler, _tmp) = test_rpc_handler().await;
|
||||
|
||||
Reference in New Issue
Block a user