diff --git a/crates/patchwork-relay/src/api.rs b/crates/patchwork-relay/src/api.rs index f132b7e..d19b5a1 100644 --- a/crates/patchwork-relay/src/api.rs +++ b/crates/patchwork-relay/src/api.rs @@ -1705,11 +1705,11 @@ async fn open_ask( } None => None, }; - let task_id = input + let reference = input .task_id .clone() .or_else(|| run.as_ref().and_then(|run| run.task_id.clone())); - let task = match &task_id { + let task = match &reference { Some(id) => Some( state .store @@ -1718,6 +1718,10 @@ async fn open_ask( ), None => None, }; + // Callers name a task however they read it — "PW-102" as often as its id — + // so the ask hangs off what the reference resolved to. Stored raw, the ask + // belongs to no task: no card, no status, and nothing to answer it from. + let task_id = task.as_ref().map(|task| task.id.clone()); if let Some(task) = &task { if task.status.is_terminal() { return Err(ApiError::conflict("that task is already closed")); @@ -5115,12 +5119,15 @@ mod tests { None, ) .unwrap(); + // Named by its key, the way a person or an agent reads it back. The + // ask must hang off the task that resolved to, or it belongs to no + // task at all: no card, no status, and nothing to answer it from. let response = router(state.clone()) .oneshot(ask_as( &answer_run_token, format!( r#"{{"kind":"review","task_id":"{}","text":"Is this the answer you needed?","summary":["It indexes task titles and outcomes"]}}"#, - auto_reviewed.id + auto_reviewed.key ), )) .await diff --git a/crates/patchwork-relay/src/orchestrator.rs b/crates/patchwork-relay/src/orchestrator.rs index 632b2d4..7673d96 100644 --- a/crates/patchwork-relay/src/orchestrator.rs +++ b/crates/patchwork-relay/src/orchestrator.rs @@ -698,6 +698,40 @@ async fn trigger_agents( } } + // Naming an agent on a closed task is asking for more work on it, so the + // task reopens itself rather than answering a person with an error and a + // chore. Only a person can do this; an agent still cannot reopen its own. + // An agent that is muted here would not run, so naming it must not reopen + // the task either: the gate is the same one the run loop uses below. + if author.kind == MemberKind::Human && channel.kind == ChannelKind::Task { + let addressed = reply_agent.is_some() + || members.iter().any(|member| { + member.kind == MemberKind::Agent + && message.mentions.contains(&member.id) + && participation_in(member, &channel.id) != Participation::Off + }); + let closed = channel + .task_id + .as_deref() + .and_then(|id| state.store.task(id).ok().flatten()) + .filter(|task| task.status.is_terminal()); + if let (true, Some(task)) = (addressed, closed) { + // Boxed: reopening posts a system note, which comes back through + // here for a message no agent can be triggered by. + Box::pin(update_task( + state, + &message.author_id, + false, + &task.id, + patchwork_core::wire::UpdateTask { + status: Some(TaskStatus::Planned), + ..Default::default() + }, + )) + .await?; + } + } + // A task is already addressed: its owner is the recipient. let task_owner = if author.kind == MemberKind::Human && message.mentions.is_empty() @@ -736,11 +770,7 @@ async fn trigger_agents( if channel.kind == ChannelKind::Dm && !in_dm { continue; } - let participation = profile - .channel_participation - .get(&channel.id) - .copied() - .unwrap_or(profile.default_participation); + let participation = participation_in(agent, &channel.id); let replied_to_agent = reply_agent.as_deref() == Some(agent.id.as_str()); // Agent-authored messages only ever wake an explicitly mentioned agent, @@ -870,6 +900,17 @@ async fn trigger_agents( Ok(()) } +fn participation_in(member: &Member, channel_id: &str) -> Participation { + match member.agent.as_ref() { + Some(profile) => profile + .channel_participation + .get(channel_id) + .copied() + .unwrap_or(profile.default_participation), + None => Participation::default(), + } +} + fn display_name_of(members: &[Member], id: &str) -> String { members .iter() @@ -4693,6 +4734,115 @@ mod tests { let _ = std::fs::remove_file(path); } + #[tokio::test] + async fn naming_an_agent_on_a_closed_task_reopens_it_instead_of_refusing() { + let path = std::env::temp_dir().join(format!("patchwork-reopen-{}.sqlite", new_id())); + let store = Store::open(&path).unwrap(); + store.create_workspace("workspace", "Test").unwrap(); + let human = member("human", "vince", MemberKind::Human); + let mut agent = member("agent", "claude", MemberKind::Agent); + agent.agent.as_mut().unwrap().default_participation = Participation::Mention; + store.insert_member(&human).unwrap(); + store.insert_member(&agent).unwrap(); + let members = [human, agent]; + let state = std::sync::Arc::new(crate::state::AppState::new( + store.clone(), + path.with_extension("files"), + "http://workspace".into(), + "relay".into(), + )); + let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel(); + state + .hosts + .write() + .await + .insert("relay".into(), crate::state::HostConn { tx }); + let task = create_task_with_result( + &state, + "human", + patchwork_core::wire::CreateTask { + title: "Ship the thing".into(), + outcome: "It is shipped".into(), + owner_id: Some("agent".into()), + status: Some(TaskStatus::Planned), + start: false, + ..Default::default() + }, + ) + .await + .unwrap() + .task; + let task = update_task( + &state, + "human", + false, + &task.id, + patchwork_core::wire::UpdateTask { + status: Some(TaskStatus::Done), + ..Default::default() + }, + ) + .await + .unwrap(); + assert_eq!(task.status, TaskStatus::Done); + let channel = store.channel(&task.discussion_channel_id).unwrap().unwrap(); + let mut nudge = message_at("nudge", "human", 9, None); + nudge.channel_id = channel.id.clone(); + nudge.task_id = Some(task.id.clone()); + nudge.body = "@claude this is still broken".into(); + nudge.mentions = vec!["agent".into()]; + store.insert_message(&nudge).unwrap(); + + trigger_agents(&state, &nudge, &channel, &members) + .await + .unwrap(); + + assert!( + !store.task(&task.id).unwrap().unwrap().status.is_terminal(), + "a person writing to a closed task reopens it" + ); + let RelayToHost::StartRun { spec } = rx.recv().await.unwrap() else { + panic!("expected the named agent to start on the reopened task"); + }; + assert_eq!(spec.agent_id, "agent"); + + // Muted here, so naming it starts nothing — and must not reopen either. + let task = update_task( + &state, + "human", + false, + &task.id, + patchwork_core::wire::UpdateTask { + status: Some(TaskStatus::Done), + ..Default::default() + }, + ) + .await + .unwrap(); + assert_eq!(task.status, TaskStatus::Done); + let mut members = members.to_vec(); + members[1].agent.as_mut().unwrap().default_participation = Participation::Off; + let mut muted = message_at("muted", "human", 10, None); + muted.channel_id = channel.id.clone(); + muted.task_id = Some(task.id.clone()); + muted.body = "@claude again".into(); + muted.mentions = vec!["agent".into()]; + store.insert_message(&muted).unwrap(); + + trigger_agents(&state, &muted, &channel, &members) + .await + .unwrap(); + + assert!( + store.task(&task.id).unwrap().unwrap().status.is_terminal(), + "naming a muted agent must not reopen a task nothing will work on" + ); + + drop(state); + drop(store); + let _ = std::fs::remove_file(path); + } + #[tokio::test] async fn a_mention_does_not_summon_an_outside_agent_into_a_dm() { let path = std::env::temp_dir().join(format!("patchwork-dm-{}.sqlite", new_id())); diff --git a/crates/patchwork-relay/src/store.rs b/crates/patchwork-relay/src/store.rs index 8bb349c..b294d4a 100644 --- a/crates/patchwork-relay/src/store.rs +++ b/crates/patchwork-relay/src/store.rs @@ -3073,12 +3073,21 @@ impl Store { Ok(()) } + /// What actually reached this person. An ask that is no longer open needs + /// nobody, whatever row it left behind: it can close by being answered, by + /// its run ending, by being superseded, or by the task closing under it, + /// and one of those paths will always forget to clear the notification. So + /// the row is only ever shown while the ask behind it is still open, which + /// also retires the ones a migration brought over from the old model. pub fn inbox(&self, member_id: &str, include_read: bool) -> Result> { let conn = self.conn()?; let sql = if include_read { - "SELECT * FROM inbox WHERE member_id = ?1 ORDER BY id DESC LIMIT 200" + "SELECT * FROM inbox WHERE member_id = ?1 AND (kind != 'ask' OR message_id IN + (SELECT message_id FROM asks WHERE status = 'open')) ORDER BY id DESC LIMIT 200" } else { - "SELECT * FROM inbox WHERE member_id = ?1 AND read_at IS NULL ORDER BY id DESC LIMIT 200" + "SELECT * FROM inbox WHERE member_id = ?1 AND read_at IS NULL AND (kind != 'ask' OR + message_id IN (SELECT message_id FROM asks WHERE status = 'open')) + ORDER BY id DESC LIMIT 200" }; let mut stmt = conn.prepare(sql)?; let rows = stmt.query_map(params![member_id], |r| Self::inbox_from_row(r))?; @@ -5637,6 +5646,52 @@ mod tests { let _ = std::fs::remove_file(path); } + #[test] + fn an_inbox_only_carries_an_ask_while_the_ask_is_open() { + let (store, path) = store(); + let item = |id: &str, message_id: Option<&str>| InboxItem { + id: id.into(), + member_id: "human".into(), + kind: InboxKind::Ask, + title: "Ready for review".into(), + preview: String::new(), + actor_id: None, + channel_id: Some("channel".into()), + message_id: message_id.map(str::to_string), + task_id: Some("task".into()), + run_id: Some("run".into()), + automation_id: None, + created_at: 1, + read_at: None, + }; + let mut open = ask("open", Some("run"), Some("task")); + open.message_id = Some("asking".into()); + let mut closed = ask("closed", Some("other-run"), Some("other-task")); + closed.message_id = Some("answered".into()); + store.commit_ask(&open, None, &[], false).unwrap(); + store.commit_ask(&closed, None, &[], false).unwrap(); + store + .answer_ask("closed", &["looks good".into()], "", "human") + .unwrap(); + store.insert_inbox(&item("live", Some("asking"))).unwrap(); + store.insert_inbox(&item("stale", Some("answered"))).unwrap(); + // What a migration leaves behind: an ask item with no ask at all. + store.insert_inbox(&item("legacy", None)).unwrap(); + + for include_read in [false, true] { + let ids = store + .inbox("human", include_read) + .unwrap() + .into_iter() + .map(|item| item.id) + .collect::>(); + assert_eq!(ids, vec!["live".to_string()]); + } + + drop(store); + let _ = std::fs::remove_file(path); + } + #[test] fn asks_resolve_independently_without_reading_other_run_items() { let (store, path) = store(); diff --git a/mobile/src/app/(app)/(tabs)/_layout.tsx b/mobile/src/app/(app)/(tabs)/_layout.tsx index 3fc0ed7..8a08b22 100644 --- a/mobile/src/app/(app)/(tabs)/_layout.tsx +++ b/mobile/src/app/(app)/(tabs)/_layout.tsx @@ -1,6 +1,10 @@ import { NativeTabs } from "expo-router/unstable-native-tabs"; +import type { SFSymbol } from "sf-symbols-typescript"; + import { unreadInboxCount } from "@client/inbox"; +import { workspaceSymbol } from "@/lib/paired"; +import { usePairedSession } from "@/lib/session"; import { useWorkspace } from "@/lib/store"; import { useTheme } from "@/lib/theme"; @@ -8,6 +12,13 @@ export default function TabLayout() { const theme = useTheme(); const bootstrap = useWorkspace().bootstrap; const unread = unreadInboxCount(bootstrap?.inbox ?? []); + const { session } = usePairedSession(); + // Which workspace is on screen rides on the More tab, the way a settings tab + // carries the current account, instead of taking a header row on every tab. + const workspace = workspaceSymbol(session && bootstrap ? { ...session, name: bootstrap.workspace.name } : session) as { + default: SFSymbol; + selected: SFSymbol; + }; return ( Chats + + + More + Search diff --git a/mobile/src/app/(app)/(tabs)/channels/index.tsx b/mobile/src/app/(app)/(tabs)/channels/index.tsx index e2d1f96..381c91d 100644 --- a/mobile/src/app/(app)/(tabs)/channels/index.tsx +++ b/mobile/src/app/(app)/(tabs)/channels/index.tsx @@ -7,7 +7,7 @@ import type { Channel, Id } from "@client/types"; import { Conversation } from "@/components/Message"; import { Avatar, Button, ChoiceField, Empty, ErrorNotice, Glass, Icon, Measured, Sheet, TextField } from "@/components/ui"; import { relative } from "@/lib/format"; -import { useLayout } from "@/lib/layout"; +import { autoTopInset, useLayout } from "@/lib/layout"; import { useWorkspace, useWorkspaceStore } from "@/lib/store"; import { useTheme } from "@/lib/theme"; @@ -114,7 +114,7 @@ export default function ChannelsScreen() { ) : ( {inlineTitle ? ( - + Chats {actions} diff --git a/mobile/src/app/(app)/(tabs)/home/index.tsx b/mobile/src/app/(app)/(tabs)/home/index.tsx index 41d6465..66498d7 100644 --- a/mobile/src/app/(app)/(tabs)/home/index.tsx +++ b/mobile/src/app/(app)/(tabs)/home/index.tsx @@ -6,10 +6,9 @@ import { groupInbox, unreadInboxCount } from "@client/inbox"; import type { InboxGroup } from "@client/inbox"; import type { Ask, InboxItem, InboxKind } from "@client/types"; import { AskCard } from "@/components/AskCard"; -import { WorkspaceMark } from "@/components/WorkspaceSwitcher"; import { Avatar, Button, Empty, Icon, Measured, Screen } from "@/components/ui"; import { relative } from "@/lib/format"; -import { usePairedSession } from "@/lib/session"; +import { autoTopInset } from "@/lib/layout"; import { useWorkspace, useWorkspaceStore } from "@/lib/store"; import { useTheme } from "@/lib/theme"; @@ -26,7 +25,6 @@ export default function HomeScreen() { const workspace = useWorkspace(); const store = useWorkspaceStore(); const insets = useSafeAreaInsets(); - const { session } = usePairedSession(); const data = workspace.bootstrap; const groups = groupInbox(data?.inbox ?? []); const unread = unreadInboxCount(data?.inbox ?? []); @@ -50,10 +48,13 @@ export default function HomeScreen() { false, ).catch(() => undefined); } + // What arrived was said somewhere, so it opens where it was said. A run's + // activity log is a drill-down from there, never the destination for + // something addressed to a person. if (item.task_id) return router.push({ pathname: "/tasks/[taskId]", params: { taskId: item.task_id } }); - if (item.run_id) return router.push({ pathname: "/(app)/runs/[runId]", params: { runId: item.run_id } }); if (item.channel_id) return router.push({ pathname: "/channels/[channelId]", params: { channelId: item.channel_id } }); if (item.automation_id) return router.push({ pathname: "/(app)/automations/[automationId]", params: { automationId: item.automation_id } }); + if (item.run_id) return router.push({ pathname: "/(app)/runs/[runId]", params: { runId: item.run_id } }); }; return ( @@ -71,20 +72,11 @@ export default function HomeScreen() { )} ListHeaderComponent={ - + Home {unread ? (