Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions app/src/components/orchestration/AgentChatPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ import debugFactory from 'debug';
import {
type KeyboardEvent,
type ReactNode,
type Ref,
useCallback,
useEffect,
useRef,
useState,
} from 'react';

import { useStickToBottom } from '../../hooks/useStickToBottom';
import { useT } from '../../lib/i18n/I18nContext';
import {
orchestrationClient,
Expand All @@ -47,6 +49,10 @@ import SessionTranscript from './SessionTranscript';

const debug = debugFactory('orchestration:agent-chat');

// Stable identity for an empty transcript so `useStickToBottom`'s layout effect
// doesn't re-run every render when the selected chat has no messages yet.
const EMPTY_MESSAGES: readonly unknown[] = [];

function sessionLabel(session: SessionSummary): string {
return session.label?.trim() || session.sessionId;
}
Expand All @@ -60,10 +66,12 @@ function sessionLabel(session: SessionSummary): string {
function ChatPageScaffold({
header,
footer,
scrollRef,
children,
}: {
header?: ReactNode;
footer?: ReactNode;
scrollRef?: Ref<HTMLDivElement>;
children: ReactNode;
}) {
const footerRef = useRef<HTMLDivElement | null>(null);
Expand Down Expand Up @@ -91,6 +99,8 @@ function ChatPageScaffold({
<div className="relative flex h-full flex-col overflow-hidden bg-surface/70 dark:bg-black/40">
{header}
<div
ref={scrollRef}
data-testid="orch-chat-scroll"
className="min-h-0 flex-1 overflow-y-auto"
style={footer ? { paddingBottom: footerHeight } : undefined}>
{children}
Expand Down Expand Up @@ -184,6 +194,11 @@ function AgentComposer({
function SessionChatView({ session }: { session: SessionSummary }) {
const { t } = useT();
const { state, messages, refresh } = useSessionTranscript(session.sessionId);
const { containerRef: scrollRef } = useStickToBottom(
messages,
session.sessionId,
session.sessionId
);
const [body, setBody] = useState('');
const [sending, setSending] = useState(false);
const [sendError, setSendError] = useState<string | null>(null);
Expand Down Expand Up @@ -232,6 +247,7 @@ function SessionChatView({ session }: { session: SessionSummary }) {

return (
<ChatPageScaffold
scrollRef={scrollRef}
header={
// Agent metadata, centered to the same width-capped column as the chat.
<div className="border-b border-line bg-surface/60 dark:bg-black/30">
Expand Down Expand Up @@ -343,6 +359,14 @@ export default function AgentChatPanel({
sendMessage,
} = useOrchestrationChats(t);
const contactSessions = useContactSessions();
// Keep the transcript pinned to the newest message (and disengage when the
// user scrolls up). Called before the `openSession` early return so hook order
// stays stable; `selectedId` as thread + reset key snaps fresh on tab switch.
const { containerRef: masterScrollRef } = useStickToBottom(
selected?.messages ?? EMPTY_MESSAGES,
selectedId,
selectedId
);
Comment thread
YellowSnnowmann marked this conversation as resolved.

const [composerBody, setComposerBody] = useState('');
const [sending, setSending] = useState(false);
Expand Down Expand Up @@ -456,6 +480,7 @@ export default function AgentChatPanel({
// switching chip in the footer. When subconscious is active, the steering
// directive + "Run review" ride alongside the chip (no header bar).
<ChatPageScaffold
scrollRef={masterScrollRef}
footer={
<div className="flex flex-col gap-2">
{showComposer ? (
Expand Down
72 changes: 72 additions & 0 deletions app/src/components/orchestration/__tests__/AgentChatPanel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,76 @@ describe('AgentChatPanel', () => {
render(<AgentChatPanel />);
expect(screen.getByText(/load failed/)).toBeInTheDocument();
});

// ── Autoscroll (regression: new master message snapped to the TOP) ─────────
const msg = (id: string) => ({
id,
from: 'you',
body: id,
timestamp: '2026-07-08T00:00:00Z',
encrypted: false,
});

// jsdom does no layout, so `scrollTop`/`scrollHeight`/`clientHeight` are inert.
// Back them with a stored value so the stick-to-bottom snap is observable.
const fakeMetrics = (el: HTMLElement, scrollHeight: number, clientHeight: number) => {
let top = 0;
Object.defineProperty(el, 'scrollHeight', { value: scrollHeight, configurable: true });
Object.defineProperty(el, 'clientHeight', { value: clientHeight, configurable: true });
Object.defineProperty(el, 'scrollTop', {
get: () => top,
set: v => {
top = v;
},
configurable: true,
});
};

it('pins the newest master message to the bottom on a new message (not the top)', () => {
chatsApi.current = {
...chatsApi.current,
selectedId: 'master',
selected: { id: 'master', title: 'Master', messages: [msg('m1')] },
};
const { rerender } = render(<AgentChatPanel />);
const scroll = screen.getByTestId('orch-chat-scroll') as HTMLDivElement;
fakeMetrics(scroll, 1000, 400);
scroll.scrollTop = 0; // as if reset to the top by the loading-spinner swap

chatsApi.current = {
...chatsApi.current,
selected: { id: 'master', title: 'Master', messages: [msg('m1'), msg('m2')] },
};
rerender(<AgentChatPanel />);

expect(scroll.scrollTop).toBe(1000); // snapped to the bottom, not left at 0
});

it('does not yank the master chat down when the user has scrolled up', () => {
chatsApi.current = {
...chatsApi.current,
selectedId: 'master',
selected: { id: 'master', title: 'Master', messages: [msg('m1')] },
};
const { rerender } = render(<AgentChatPanel />);
const scroll = screen.getByTestId('orch-chat-scroll') as HTMLDivElement;
fakeMetrics(scroll, 1000, 400);
scroll.scrollTop = 0; // 600px from the bottom, past the 80px threshold
fireEvent.scroll(scroll); // disengages stickiness

chatsApi.current = {
...chatsApi.current,
selected: { id: 'master', title: 'Master', messages: [msg('m1'), msg('m2')] },
};
rerender(<AgentChatPanel />);

expect(scroll.scrollTop).toBe(0); // left where the user parked it
});

it('renders with no selected chat (covers the empty-messages fallback)', () => {
chatsApi.current = { ...chatsApi.current, selected: undefined as never };
render(<AgentChatPanel />);
// Exercises `selected?.messages ?? EMPTY_MESSAGES`; the panel still renders.
expect(screen.getByTestId('orch-agent-tab-master')).toBeInTheDocument();
});
});
130 changes: 130 additions & 0 deletions src/openhuman/agent_orchestration/background_completions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ pub(crate) struct CompletedBackgroundAgent {
/// batch of threads is deleted.
const CANCELLED_TOMBSTONE_CAP: usize = 512;

/// Upper bound on the collected-task tombstone set. A completion records within
/// seconds of the parent collecting it inline, so only recently collected task
/// ids can still be racing a late record; older tombstones are evicted in
/// insertion order.
const COLLECTED_TOMBSTONE_CAP: usize = 512;

/// Shared state behind a single mutex so the cancellation check in
/// [`record_completion`] is atomic against the tombstone+sweep in
/// [`discard_for_thread`] — otherwise the cooperative-abort race could enqueue a
Expand All @@ -49,6 +55,15 @@ struct QueueState {
cancelled_threads: HashSet<String>,
/// Insertion order for `cancelled_threads`, used to bound the set.
cancelled_order: VecDeque<String>,
/// Task ids the parent already collected inline via `wait_subagent` and will
/// present in its own turn. A completion for a collected task is dropped by
/// [`record_completion`] (closing the wait/record ordering race) and any
/// already-queued entry is swept by [`mark_collected`], so background
/// delivery never re-answers a result the master already surfaced (the
/// duplicate-response bug).
collected_tasks: HashSet<String>,
/// Insertion order for `collected_tasks`, used to bound the set.
collected_order: VecDeque<String>,
}

impl QueueState {
Expand All @@ -63,6 +78,19 @@ impl QueueState {
}
}
}

/// Tombstone `task_id` so a completion that records after the parent
/// collected it inline is dropped rather than delivered again.
fn tombstone_collected(&mut self, task_id: &str) {
if self.collected_tasks.insert(task_id.to_string()) {
self.collected_order.push_back(task_id.to_string());
while self.collected_order.len() > COLLECTED_TOMBSTONE_CAP {
if let Some(evicted) = self.collected_order.pop_front() {
self.collected_tasks.remove(&evicted);
}
}
}
}
}

static QUEUE: OnceLock<Mutex<QueueState>> = OnceLock::new();
Expand Down Expand Up @@ -104,6 +132,18 @@ pub(crate) fn record_completion(
return;
}
}
// The parent already collected this result inline (`wait_subagent`) and
// presents it in its own turn, so a background-delivery turn for it would
// just re-answer the same thing. Drop it (closes the wait-before-record
// race; the record-before-wait order is handled by the sweep in
// `mark_collected`).
if state.collected_tasks.contains(&entry.task_id) {
log::debug!(
"[background_completions] dropping completion task_id={} already collected inline",
entry.task_id
);
return;
}
let pending = state.pending.entry(parent_session).or_default();
if pending.iter().any(|c| c.task_id == entry.task_id) {
return;
Expand Down Expand Up @@ -172,6 +212,33 @@ pub(crate) fn discard_for_thread(thread_id: &str) -> usize {
removed
}

/// Mark `task_id` as collected inline by the parent (via `wait_subagent`) so its
/// background completion is not independently delivered as a second, duplicate
/// answer. Tombstones the id — bounded — so a completion that records *after*
/// this call (the wait-before-record ordering) is dropped by
/// [`record_completion`], and sweeps any entry already queued for it across all
/// sessions (the record-before-wait ordering). Both orderings resolve
/// atomically under the single queue mutex. Returns whether a queued entry was
/// removed.
pub(crate) fn mark_collected(task_id: &str) -> bool {
let mut state = queue()
.lock()
.expect("background_completions queue poisoned");
state.tombstone_collected(task_id);
let mut removed = false;
for pending in state.pending.values_mut() {
let before = pending.len();
pending.retain(|c| c.task_id != task_id);
removed |= pending.len() != before;
}
// Drop now-empty session buckets so the map doesn't accumulate keys.
state.pending.retain(|_, v| !v.is_empty());
log::debug!(
"[background_completions] mark_collected task_id={task_id} removed_queued={removed}"
);
removed
}

/// Wipe every queued completion across all sessions. Called on a full thread
/// purge. Tombstones are left intact (the per-thread protection set by
/// [`discard_for_thread`]); the purge path tombstones each in-flight sub-agent's
Expand Down Expand Up @@ -401,4 +468,67 @@ mod tests {
assert!(!has_pending("sess-c2"));
assert_eq!(clear_all(), 0);
}

#[test]
fn mark_collected_sweeps_the_queued_entry() {
let _guard = test_guard();
let s = "sess-mc-sweep";
record_completion(s, "mc-sub-1", "researcher", "collected", None);
record_completion(s, "mc-sub-2", "researcher", "keep", None);

// The parent collected sub-1 inline, so it must not be delivered again;
// sub-2 (never waited on) survives for normal idle delivery.
assert!(mark_collected("mc-sub-1"), "swept the queued entry");
assert_eq!(pending_count(s), 1);
let drained = take_pending(s);
assert_eq!(drained[0].task_id, "mc-sub-2");
}

#[test]
fn record_after_mark_collected_is_dropped_by_tombstone() {
let _guard = test_guard();
// Collecting inline tombstones the task id...
assert!(
!mark_collected("mc-late"),
"nothing queued yet, so nothing swept"
);
// ...so a completion that records *after* (the wait-before-record order)
// is dropped rather than queued for a duplicate delivery turn.
record_completion("sess-mc-race", "mc-late", "researcher", "stale", None);
assert_eq!(
pending_count("sess-mc-race"),
0,
"a completion collected inline must not be re-delivered"
);
}

#[test]
fn mark_collected_is_task_scoped() {
let _guard = test_guard();
let s = "sess-mc-scope";
// Only the collected task is suppressed; an un-waited sibling still
// surfaces (the genuinely-later fire-and-forget feature is preserved).
mark_collected("mc-scope-1");
record_completion(s, "mc-scope-2", "researcher", "later", None);
assert_eq!(pending_count(s), 1);
assert!(has_pending(s));
take_pending(s);
}

#[test]
fn collected_tombstone_is_bounded() {
let _guard = test_guard();
for i in 0..(COLLECTED_TOMBSTONE_CAP + 50) {
mark_collected(&format!("mc-bound-{i}"));
}
let len = queue()
.lock()
.expect("queue poisoned")
.collected_tasks
.len();
assert!(
len <= COLLECTED_TOMBSTONE_CAP,
"collected tombstone must stay bounded, got {len}"
);
}
}
10 changes: 10 additions & 0 deletions src/openhuman/agent_orchestration/tools/wait_subagent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,16 @@ impl Tool for WaitSubagentTool {
resolved_task_id,
iterations
);
// The parent is collecting this result inline and will present
// it in this turn, so suppress the detached completion's separate
// background-delivery turn — otherwise the same result is
// re-answered as a duplicate. Only the Completed arm marks:
// AwaitingUser/Failed never record a completion, and a
// still-Running/TimedOut sub-agent has no terminal result yet, so
// a genuinely-later completion must still surface.
crate::openhuman::agent_orchestration::background_completions::mark_collected(
&resolved_task_id,
);
let status = wait_status_payload(
resume_ref.as_ref(),
&resolved_task_id,
Expand Down
1 change: 1 addition & 0 deletions src/openhuman/agent_registry/agents/orchestrator/prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Follow this sequence for every user message:
- Find the matching toolkit in the **Connected Integrations** section and call `delegate_to_integrations_agent` with that `toolkit`.
- **Do this even if remembered context could plausibly answer.** The user wants the live source of truth, not a stale summary.
- If the relevant toolkit is **not** in **Connected Integrations**, call `composio_connect { toolkit: "<slug>" }` **directly** to raise an **inline connect card** so the user can authorize in one click, then continue the task once it returns `connected: true`. Do **not** refuse based on the Connected Integrations list (that is only what is *already* connected, not what is *connectable*), do **not** make "go to Settings → Connections" your first move, and do **not** silently fall back to memory retrieval (see "Connecting external services" below).
- **Scope gate (required):** treat this as an external-service request ONLY if the ask actually operates on that service's own data or actions — its inbox/messages, files, calendar events, docs, tickets, etc. A service merely being *connected* is **not** a reason to touch it. General-knowledge answers, web/news lookups, headlines, date/time, and math must **not** spawn `delegate_to_integrations_agent` (or any email/inbox/calendar fetch) even when Gmail/Notion/etc. are connected — route those to a direct tool (Step 3) or the matching non-integration specialist (Step 4). When the request neither names nor clearly implies a specific service's own data or actions, do not reach into one — a clear implication ("check my inbox", "send an email") still counts as naming it and should be delivered; only a request that references no service at all (e.g. "today's date") stays off delegation.
3. **Can I solve this with direct tools?**
- Yes: use direct tools (`memory_recall`, `read_workspace_state`, `composio_list_connections`, task tools, etc.).
- **Memory is direct work.** Recalling a fact (`memory_recall`), storing a fact (`memory_store`), or saving a preference (`save_preference`) needs **no sub-agent** — call the direct tool and confirm the result. After a `memory_store` write, follow the memory protocol and call `update_memory_md` (targeting `MEMORY.md`) to keep the index in sync with the store; `save_preference` writes the preference store and needs no such reconcile. Reserve the `retrieve_memory` and `manage_profile_memory` **delegates** for genuinely deep work: multi-hop memory-tree walks, ingest/index into the long-term tree, reconciling overlapping notes, people-graph/alias management, or persona edits. Do **not** spawn a sub-agent for a single recall or a single "remember this".
Expand Down
Loading
Loading