feat(artifacts): persist thread_id + ai_list_artifacts thread filter (#3226) (#3274)

This commit is contained in:
YOMXXX
2026-06-04 00:52:03 -04:00
committed by GitHub
parent dd726fda14
commit 6a91dcd481
7 changed files with 297 additions and 14 deletions
+13 -3
View File
@@ -12,20 +12,30 @@ const MAX_LIMIT: usize = 200;
/// List artifacts in the workspace with pagination.
///
/// When `thread_id` is `Some(_)` the listing is filtered to artifacts whose
/// persisted `meta.json` was produced in that chat thread (#3226) — the
/// pagination `total` reflects the filtered set, not the workspace total,
/// so the UI's "showing N of M" tally is meaningful per-thread. Legacy
/// artifacts written before `ArtifactMeta.thread_id` existed have
/// `thread_id = None` and are excluded from thread-scoped listings.
///
/// Returns `{ "artifacts": [...], "total": N, "offset": M, "limit": L }`.
pub async fn ai_list_artifacts(
config: &Config,
offset: Option<usize>,
limit: Option<usize>,
thread_id: Option<&str>,
) -> Result<RpcOutcome<Value>, String> {
let offset = offset.unwrap_or(0);
let limit = limit.unwrap_or(DEFAULT_LIMIT).min(MAX_LIMIT);
log::debug!(
"[artifacts] ai_list_artifacts: workspace={:?} offset={offset} limit={limit}",
config.workspace_dir
"[artifacts] ai_list_artifacts: workspace={:?} offset={offset} limit={limit} thread_id={:?}",
config.workspace_dir,
thread_id,
);
let (artifacts, total) = store::list_artifacts(&config.workspace_dir, offset, limit).await?;
let (artifacts, total) =
store::list_artifacts(&config.workspace_dir, offset, limit, thread_id).await?;
log::debug!(
"[artifacts] ai_list_artifacts: returning {} of {total} total",
+128 -1
View File
@@ -17,7 +17,7 @@ fn test_config(tmp: &TempDir) -> Config {
async fn list_empty() {
let tmp = TempDir::new().unwrap();
let config = test_config(&tmp);
let outcome = ai_list_artifacts(&config, None, None).await.unwrap();
let outcome = ai_list_artifacts(&config, None, None, None).await.unwrap();
let value = outcome.into_cli_compatible_json().unwrap();
assert_eq!(value["total"], 0);
assert_eq!(value["artifacts"].as_array().unwrap().len(), 0);
@@ -25,6 +25,133 @@ async fn list_empty() {
assert_eq!(value["limit"], DEFAULT_LIMIT as u64);
}
/// #3226: when no `thread_id` filter is supplied the listing surfaces
/// artifacts from every thread (and the legacy ones without a thread).
#[tokio::test]
async fn list_without_thread_filter_returns_all_threads() {
use crate::openhuman::artifacts::store::save_artifact_meta;
use crate::openhuman::artifacts::types::{ArtifactKind, ArtifactMeta, ArtifactStatus};
let tmp = TempDir::new().unwrap();
let config = test_config(&tmp);
for (id, tid) in [
("a1", Some("thread-a".to_string())),
("b1", Some("thread-b".to_string())),
("legacy", None),
] {
save_artifact_meta(
tmp.path(),
&ArtifactMeta {
id: id.to_string(),
kind: ArtifactKind::Document,
title: id.to_string(),
path: format!("{id}/x.txt"),
size_bytes: 0,
status: ArtifactStatus::Ready,
created_at: chrono::Utc::now(),
error: None,
thread_id: tid,
},
)
.await
.unwrap();
}
let outcome = ai_list_artifacts(&config, None, None, None).await.unwrap();
let value = outcome.into_cli_compatible_json().unwrap();
assert_eq!(value["total"], 3, "unfiltered list returns every artifact");
}
/// #3226: `thread_id = Some(_)` returns ONLY artifacts whose persisted
/// meta matches that thread. Legacy entries (thread_id absent) are
/// deliberately excluded — they have no addressable owning thread.
#[tokio::test]
async fn list_with_thread_filter_returns_only_matching_thread() {
use crate::openhuman::artifacts::store::save_artifact_meta;
use crate::openhuman::artifacts::types::{ArtifactKind, ArtifactMeta, ArtifactStatus};
let tmp = TempDir::new().unwrap();
let config = test_config(&tmp);
for (id, tid) in [
("a1", Some("thread-a".to_string())),
("a2", Some("thread-a".to_string())),
("b1", Some("thread-b".to_string())),
("legacy", None),
] {
save_artifact_meta(
tmp.path(),
&ArtifactMeta {
id: id.to_string(),
kind: ArtifactKind::Document,
title: id.to_string(),
path: format!("{id}/x.txt"),
size_bytes: 0,
status: ArtifactStatus::Ready,
created_at: chrono::Utc::now(),
error: None,
thread_id: tid,
},
)
.await
.unwrap();
}
let outcome = ai_list_artifacts(&config, None, None, Some("thread-a"))
.await
.unwrap();
let value = outcome.into_cli_compatible_json().unwrap();
assert_eq!(value["total"], 2, "only two artifacts belong to thread-a");
let ids: Vec<&str> = value["artifacts"]
.as_array()
.unwrap()
.iter()
.map(|a| a["id"].as_str().unwrap())
.collect();
assert!(ids.contains(&"a1"));
assert!(ids.contains(&"a2"));
assert!(
!ids.contains(&"b1"),
"thread-b leaked into thread-a listing"
);
assert!(
!ids.contains(&"legacy"),
"legacy artifact (thread_id=None) must NOT match a specific thread filter"
);
}
/// #3226: when the filtered thread has no artifacts, `total` is 0
/// (per-thread, not per-workspace) so the UI's "showing N of M" line
/// stays meaningful.
#[tokio::test]
async fn list_with_thread_filter_unknown_thread_returns_zero() {
use crate::openhuman::artifacts::store::save_artifact_meta;
use crate::openhuman::artifacts::types::{ArtifactKind, ArtifactMeta, ArtifactStatus};
let tmp = TempDir::new().unwrap();
let config = test_config(&tmp);
save_artifact_meta(
tmp.path(),
&ArtifactMeta {
id: "only".to_string(),
kind: ArtifactKind::Document,
title: "only".to_string(),
path: "only/x.txt".to_string(),
size_bytes: 0,
status: ArtifactStatus::Ready,
created_at: chrono::Utc::now(),
error: None,
thread_id: Some("thread-a".to_string()),
},
)
.await
.unwrap();
let outcome = ai_list_artifacts(&config, None, None, Some("thread-missing"))
.await
.unwrap();
let value = outcome.into_cli_compatible_json().unwrap();
assert_eq!(value["total"], 0);
assert_eq!(value["artifacts"].as_array().unwrap().len(), 0);
}
// ── ai_get_artifact ────────────────────────────────────────────────────────
#[tokio::test]
+40 -1
View File
@@ -51,6 +51,15 @@ pub fn schemas(function: &str) -> ControllerSchema {
"Maximum number of artifacts to return; defaults to 50, capped at 200.",
required: false,
},
FieldSchema {
name: "thread_id",
ty: TypeSchema::Option(Box::new(TypeSchema::String)),
comment:
"When set, return only artifacts whose meta.json was written for this \
chat thread (#3226). Lets ChatFilesPanel repopulate from disk on a \
fresh redux slice / new device. Absent (or null) returns all artifacts.",
required: false,
},
],
outputs: vec![
FieldSchema {
@@ -202,7 +211,16 @@ fn handle_list_artifacts(params: Map<String, Value>) -> ControllerFuture {
let limit = read_optional_u64(&params, "limit")?
.map(|raw| usize::try_from(raw).map_err(|_| "limit is too large for usize".to_string()))
.transpose()?;
to_json(crate::openhuman::artifacts::ops::ai_list_artifacts(&config, offset, limit).await?)
let thread_id = read_optional_string(&params, "thread_id")?;
to_json(
crate::openhuman::artifacts::ops::ai_list_artifacts(
&config,
offset,
limit,
thread_id.as_deref(),
)
.await?,
)
})
}
@@ -235,6 +253,27 @@ fn read_required<T: DeserializeOwned>(params: &Map<String, Value>, key: &str) ->
serde_json::from_value(value).map_err(|e| format!("invalid '{key}': {e}"))
}
fn read_optional_string(params: &Map<String, Value>, key: &str) -> Result<Option<String>, String> {
match params.get(key) {
None => Ok(None),
Some(Value::Null) => Ok(None),
Some(Value::String(s)) => {
let trimmed = s.trim();
// Treat whitespace-only input as absent so the handler doesn't
// try to match an artifact against an unfilterable empty string.
if trimmed.is_empty() {
Ok(None)
} else {
Ok(Some(trimmed.to_string()))
}
}
Some(other) => Err(format!(
"invalid '{key}': expected string, got {}",
type_name(other)
)),
}
}
fn read_optional_u64(params: &Map<String, Value>, key: &str) -> Result<Option<u64>, String> {
match params.get(key) {
None => Ok(None),
+29 -4
View File
@@ -110,15 +110,24 @@ pub(crate) async fn save_artifact_meta(
/// List artifacts in the workspace, sorted by `created_at` descending.
///
/// Corrupt or unreadable `meta.json` files are skipped with a `warn!` log.
/// When `thread_id` is `Some(_)` the listing is filtered to entries whose
/// persisted `meta.thread_id` matches verbatim (#3226); legacy meta.json
/// files without a `thread_id` field are excluded from the filtered set
/// because they have no addressable owning thread. The returned `total`
/// reflects the filtered count so the caller's pagination is per-thread,
/// not workspace-global.
///
/// Returns `(page, total)` where `page` is the requested slice and `total` is
/// the count before pagination.
/// the count before pagination (but after filtering).
pub(crate) async fn list_artifacts(
workspace_dir: &Path,
offset: usize,
limit: usize,
thread_id: Option<&str>,
) -> Result<(Vec<ArtifactMeta>, usize), String> {
log::debug!(
"[artifacts] list_artifacts: offset={offset} limit={limit} workspace={:?}",
"[artifacts] list_artifacts: offset={offset} limit={limit} thread_id={:?} workspace={:?}",
thread_id,
workspace_dir
);
let root = artifacts_root(workspace_dir).await?;
@@ -185,6 +194,13 @@ pub(crate) async fn list_artifacts(
// Sort descending by created_at (newest first)
all.sort_by(|a, b| b.created_at.cmp(&a.created_at));
// Apply thread filter BEFORE pagination so `total` reflects the
// per-thread count the UI surfaces, and so a small page doesn't get
// silently emptied by filtering after the slice (#3226).
if let Some(tid) = thread_id {
all.retain(|m| m.thread_id.as_deref() == Some(tid));
}
let total = all.len();
let page = all.into_iter().skip(offset).take(limit).collect::<Vec<_>>();
@@ -336,6 +352,13 @@ pub async fn create_artifact(
})?;
let absolute_path = artifact_dir.join(&filename);
// Capture the originating chat thread (if any) at create-time so the
// panel can repopulate from disk after a redux-persist purge — see
// #3226. `finalize_artifact` / `fail_artifact` already read the same
// task-local for event publication; persisting it here means the
// routing target survives a process restart.
let (thread_id, _) = current_chat_context();
let meta = ArtifactMeta {
id: id.clone(),
kind,
@@ -345,13 +368,15 @@ pub async fn create_artifact(
status: ArtifactStatus::Pending,
created_at: chrono::Utc::now(),
error: None,
thread_id,
};
save_artifact_meta(workspace_dir, &meta).await?;
log::debug!(
"[artifacts] create_artifact: id={id} kind={} path={:?}",
"[artifacts] create_artifact: id={id} kind={} path={:?} thread_id={:?}",
meta.kind.as_str(),
absolute_path
absolute_path,
meta.thread_id,
);
// Surface the "Generating…" card the moment the row is reserved so
+5 -4
View File
@@ -14,6 +14,7 @@ fn make_meta(id: &str, title: &str, created_at: chrono::DateTime<Utc>) -> Artifa
status: ArtifactStatus::Ready,
created_at,
error: None,
thread_id: None,
}
}
@@ -53,7 +54,7 @@ async fn list_returns_saved_items_sorted_by_created_at() {
.await
.unwrap();
let (items, total) = list_artifacts(tmp.path(), 0, 100).await.unwrap();
let (items, total) = list_artifacts(tmp.path(), 0, 100, None).await.unwrap();
assert_eq!(total, 3);
assert_eq!(items.len(), 3);
// Newest first
@@ -65,7 +66,7 @@ async fn list_returns_saved_items_sorted_by_created_at() {
#[tokio::test]
async fn list_empty_workspace() {
let tmp = TempDir::new().unwrap();
let (items, total) = list_artifacts(tmp.path(), 0, 50).await.unwrap();
let (items, total) = list_artifacts(tmp.path(), 0, 50, None).await.unwrap();
assert_eq!(total, 0);
assert!(items.is_empty());
}
@@ -83,7 +84,7 @@ async fn list_pagination() {
.unwrap();
}
let (items, total) = list_artifacts(tmp.path(), 1, 2).await.unwrap();
let (items, total) = list_artifacts(tmp.path(), 1, 2, None).await.unwrap();
assert_eq!(total, 5);
assert_eq!(items.len(), 2);
}
@@ -162,7 +163,7 @@ async fn list_skips_corrupt_meta() {
std::fs::create_dir_all(&corrupt_dir).unwrap();
std::fs::write(corrupt_dir.join("meta.json"), b"this is not json").unwrap();
let (items, total) = list_artifacts(tmp.path(), 0, 100).await.unwrap();
let (items, total) = list_artifacts(tmp.path(), 0, 100, None).await.unwrap();
// Only the valid one should be returned
assert_eq!(total, 1);
assert_eq!(items[0].id, "good-id");
+5 -1
View File
@@ -82,7 +82,11 @@ impl Tool for ArtifactListTool {
log::debug!("[tool][artifacts] list invoked");
let offset = read_opt_usize(&args, "offset");
let limit = read_opt_usize(&args, "limit");
let outcome = ops::ai_list_artifacts(&self.config, offset, limit)
// The agent-facing tool surface lists everything in the workspace;
// the per-chat filter is an RPC-only knob used by the React panel
// (#3226). Keep the tool path unchanged so existing agent flows
// (orchestrator artifact reasoning) still see the full set.
let outcome = ops::ai_list_artifacts(&self.config, offset, limit, None)
.await
.map_err(|e| anyhow::anyhow!("artifact_list: {e}"))?;
Ok(ToolResult::success(serde_json::to_string(&outcome.value)?))
+77
View File
@@ -99,6 +99,16 @@ pub struct ArtifactMeta {
/// stderr from a separate log.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub error: Option<String>,
/// Chat thread that produced the artifact, captured from
/// [`crate::openhuman::approval::APPROVAL_CHAT_CONTEXT`] at create-time
/// (#3226). `None` for CLI / cron / sub-agent paths and for legacy
/// `meta.json` files written before this field existed — same convention
/// as the `thread_id` carried on the producer events
/// (`DomainEvent::ArtifactReady` / `Failed`). Used by
/// `ai_list_artifacts(thread_id = …)` to rebuild `ChatFilesPanel` from
/// disk after a redux-persist purge / fresh-device boot.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub thread_id: Option<String>,
}
#[cfg(test)]
@@ -214,6 +224,7 @@ mod tests {
status: ArtifactStatus::Ready,
created_at: Utc.with_ymd_and_hms(2025, 6, 1, 12, 0, 0).unwrap(),
error: None,
thread_id: Some("thread-42".to_string()),
};
let json = serde_json::to_value(&meta).unwrap();
assert_eq!(json["id"], "abc-123");
@@ -237,6 +248,7 @@ mod tests {
status: ArtifactStatus::Pending,
created_at: Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap(),
error: None,
thread_id: None,
};
let v = serde_json::to_value(&meta).unwrap();
// Verify all expected fields are present
@@ -256,4 +268,69 @@ mod tests {
let result: Result<ArtifactMeta, _> = serde_json::from_value(incomplete);
assert!(result.is_err());
}
/// #3226: legacy meta.json files written before `thread_id` existed
/// must still deserialise — `#[serde(default)]` keeps them readable
/// and they show as `None`, so `ai_list_artifacts(thread_id=…)` skips
/// them rather than crashing the listing.
#[test]
fn artifact_meta_legacy_meta_without_thread_id_deserialises() {
let legacy = json!({
"id": "legacy-1",
"kind": "presentation",
"title": "old",
"path": "legacy-1/old.pptx",
"size_bytes": 0,
"status": "ready",
"created_at": "2025-06-01T12:00:00Z",
});
let meta: ArtifactMeta =
serde_json::from_value(legacy).expect("legacy meta.json must still deserialise");
assert!(meta.thread_id.is_none());
assert_eq!(meta.id, "legacy-1");
}
/// #3226: round-tripping a meta with `thread_id = None` must NOT emit
/// the field — `skip_serializing_if = "Option::is_none"` keeps freshly
/// written legacy-ish meta.json compatible with the pre-#3226 shape.
#[test]
fn artifact_meta_thread_id_none_is_skipped_in_serialised_form() {
let meta = ArtifactMeta {
id: "x".to_string(),
kind: ArtifactKind::Other,
title: "t".to_string(),
path: "x/t.txt".to_string(),
size_bytes: 0,
status: ArtifactStatus::Pending,
created_at: Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap(),
error: None,
thread_id: None,
};
let v = serde_json::to_value(&meta).unwrap();
assert!(
v.get("thread_id").is_none(),
"None thread_id must be skipped, got {v}"
);
}
/// #3226: when a chat context produced the artifact, the persisted
/// shape carries `thread_id` verbatim so the panel can filter on it.
#[test]
fn artifact_meta_thread_id_some_round_trips() {
let meta = ArtifactMeta {
id: "x".to_string(),
kind: ArtifactKind::Other,
title: "t".to_string(),
path: "x/t.txt".to_string(),
size_bytes: 0,
status: ArtifactStatus::Pending,
created_at: Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap(),
error: None,
thread_id: Some("thread-42".to_string()),
};
let v = serde_json::to_value(&meta).unwrap();
assert_eq!(v["thread_id"], "thread-42");
let back: ArtifactMeta = serde_json::from_value(v).unwrap();
assert_eq!(back.thread_id.as_deref(), Some("thread-42"));
}
}