From 531753c635dac7b34790467b17139d7a18bb71c4 Mon Sep 17 00:00:00 2001 From: cyrus Date: Thu, 20 Aug 2026 16:30:03 +0530 Subject: [PATCH 01/14] workflows: paginate run history by finish time, cursor, bounded read GET .../workflows/runs now returns a WorkflowRunsPage envelope ({ runs, hasMore }) instead of a bare array, ordered newest-first by the run's actual settle time (seq/atMillis), with beforeSeq-based cursor pagination so a page can be fetched further back without an unbounded read. RunHistoryPanel gains a "Load older" affordance gated on hasMore. Every existing call site and test that read the runs endpoint as a bare array needed updating to the new envelope shape. Closes #1012 --- frontend/src/api/workflows.ts | 32 +- frontend/src/views/WorkflowsView.tsx | 56 +- .../src/views/workflows/RunHistoryPanel.tsx | 30 + .../e2e/workflow-selection-persists.spec.ts | 2 +- .../unit/workflow-deeplink-reread.test.ts | 2 +- .../test/unit/workflow-index-first.test.ts | 2 +- .../test/unit/workflow-index-paused.test.ts | 2 +- .../unit/workflow-live-node-state.test.ts | 2 +- .../test/unit/workflow-run-failure.test.ts | 2 +- frontend/test/unit/workflow-run-input.test.ts | 2 +- .../test/unit/workflow-toolbar-layout.test.ts | 2 +- src/server/ops/inference.rs | 2 +- src/server/ops/workflows.rs | 823 +++++++++++------- src/server/ops/write_test.rs | 2 +- 14 files changed, 636 insertions(+), 325 deletions(-) diff --git a/frontend/src/api/workflows.ts b/frontend/src/api/workflows.ts index 96073f100..739ea3aa8 100644 --- a/frontend/src/api/workflows.ts +++ b/frontend/src/api/workflows.ts @@ -792,23 +792,41 @@ export function cancelWorkflowRun( } /** - * The company's finished workflow runs, **newest first** (issue #228). + * One page of {@link listWorkflowRuns} (issue #1012). + * + * `hasMore` says whether an older page exists behind `beforeSeq` — the run + * history drawer's "Load older" affordance is gated on it, so a truncated + * history never silently reads as the whole thing. + */ +export interface WorkflowRunsPage { + runs: WorkflowRunOutcome[]; + hasMore: boolean; +} + +/** + * The company's finished workflow runs, **newest first** (issue #228) — now + * genuinely true of the *displayed* `seq`/`atMillis`, not just the order two + * runs started in (issue #1012). * * `workflow` narrows to one graph's runs; `limit` caps the page (the host - * defaults to a short recent list and clamps a large ask). A host predating this - * route answers 404 — callers should treat that as "no history yet" rather than - * an error, since the console still works without it. + * defaults to a short recent list and clamps a large ask). `beforeSeq` pages + * further back: pass the `seq` of the oldest run already held to fetch the + * page before it (issue #1012) — `hasMore` on the returned page says whether + * one exists. A host predating this route answers 404 — callers should treat + * that as "no history yet" rather than an error, since the console still works + * without it. */ export function listWorkflowRuns( client: OpenCompanyClient, company: string | null, - options?: { workflow?: string; limit?: number }, -): Promise { + options?: { workflow?: string; limit?: number; beforeSeq?: number }, +): Promise { const params = new URLSearchParams(); if (options?.workflow) params.set("workflow", options.workflow); if (options?.limit) params.set("limit", String(options.limit)); + if (options?.beforeSeq) params.set("before_seq", String(options.beforeSeq)); const query = params.toString(); - return client.get( + return client.get( `${client.scopeFor(company)}/workflows/runs${query ? `?${query}` : ""}`, ); } diff --git a/frontend/src/views/WorkflowsView.tsx b/frontend/src/views/WorkflowsView.tsx index 2f154e905..ea2616e0b 100644 --- a/frontend/src/views/WorkflowsView.tsx +++ b/frontend/src/views/WorkflowsView.tsx @@ -408,6 +408,15 @@ export function WorkflowsView({ // vanished when the drawer was dismissed and a scheduled run's never reached // the operator at all. const [runs, setRuns] = useState([]); + // Issue #1012: whether an older page of `runs` exists behind the oldest + // `seq` currently held — gates the drawer's "Load older" affordance. Reset + // to `false` whenever the effect below replaces `runs` wholesale (a fresh + // newest-page fetch has not yet learned this), and updated by both that + // effect and `loadOlderRuns` from each fetch's own `hasMore`. + const [runsHasMore, setRunsHasMore] = useState(false); + // A "Load older" fetch in flight, so the drawer can disable the control and + // avoid a second click racing the first for the same older page. + const [loadingOlderRuns, setLoadingOlderRuns] = useState(false); // Which workflow the rows in `runs` were fetched for. // // `graph` and `runs` are two independent requests off the same selection, so @@ -988,18 +997,25 @@ export function WorkflowsView({ // whether the host serves this route. if (!selectedId) { setRuns([]); + setRunsHasMore(false); setRunsFor(null); return; } let live = true; (async () => { try { - const rows = await listWorkflowRuns(client, company, { + const { runs: rows, hasMore } = await listWorkflowRuns(client, company, { workflow: selectedId, limit: 50, }); if (!live) return; setRuns(rows); + // Issue #1012: this effect always replaces the page wholesale (a + // company switch, a run event, an explicit refresh) — any older runs + // a "Load older" click had appended are gone with it, so `hasMore` + // starts back over from this fresh newest page's own answer rather + // than carrying forward whatever the appended state last said. + setRunsHasMore(hasMore); setRunsFor(selectedId); setHistorySupported(true); // Issue #371, the no-live-stream fallback. If the run we just POSTed is @@ -1022,6 +1038,7 @@ export function WorkflowsView({ // Degrade quietly: an older host simply has no history to show. console.debug("[WorkflowsView] run history unavailable", e); setRuns([]); + setRunsHasMore(false); // Still THIS workflow's answer — "the host has no history for it" — so // the pair agrees and the copilot may proceed, told via `runsKnown` // that nothing is known about runs rather than that there were none. @@ -1039,6 +1056,36 @@ export function WorkflowsView({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [client, company, selectedId, runsTick, runEventTick]); + // Issue #1012: "Load older", the run-history drawer's pagination affordance. + // APPENDS to `runs` rather than replacing it — unlike the effect above, + // which always starts over from the newest page. Paged off the `seq` of the + // oldest run currently held, matching the host's `?before_seq=` cursor + // semantics (issue #1012's ordering fix made `seq` the field every row's + // own display agrees with, which is what makes it a stable paging key). + const loadOlderRuns = useCallback(() => { + if (!selectedId || loadingOlderRuns) return; + const oldest = runs.at(-1)?.seq; + if (oldest === undefined) return; + setLoadingOlderRuns(true); + (async () => { + try { + const { runs: older, hasMore } = await listWorkflowRuns(client, company, { + workflow: selectedId, + limit: 50, + beforeSeq: oldest, + }); + setRuns((prev) => [...prev, ...older]); + setRunsHasMore(hasMore); + } catch (e) { + // Same quiet degradation as the newest-page fetch — leave what is + // already shown in place rather than losing it to a failed page. + console.debug("[WorkflowsView] loading older run history failed", e); + } finally { + setLoadingOlderRuns(false); + } + })(); + }, [client, company, selectedId, runs, loadingOlderRuns]); + // Issue #303: the run page the index's health readings are folded from. // // Fetched only while the index is on screen — every card reads from one @@ -1060,7 +1107,9 @@ export function WorkflowsView({ let live = true; (async () => { try { - const rows = await listWorkflowRuns(client, company, { limit: 200 }); + // `hasMore` is ignored here: the index only needs enough of the + // company-wide page to fold per-card health, not a pagination UI. + const { runs: rows } = await listWorkflowRuns(client, company, { limit: 200 }); if (!live) return; setIndexRuns(rows); setIndexRunsLoaded(true); @@ -2697,6 +2746,9 @@ export function WorkflowsView({ onFixWithCopilot={handleFixWithCopilot} fixingRunSeq={fixingRunSeq} fixReason={fixReason} + hasMore={runsHasMore} + onLoadOlder={loadOlderRuns} + loadingOlder={loadingOlderRuns} /> ) : null } diff --git a/frontend/src/views/workflows/RunHistoryPanel.tsx b/frontend/src/views/workflows/RunHistoryPanel.tsx index e15f6b11d..d011b0d76 100644 --- a/frontend/src/views/workflows/RunHistoryPanel.tsx +++ b/frontend/src/views/workflows/RunHistoryPanel.tsx @@ -168,6 +168,9 @@ export function RunHistoryPanel({ onFixWithCopilot, fixingRunSeq, fixReason, + hasMore, + onLoadOlder, + loadingOlder, }: { runs: WorkflowRunOutcome[]; /** @@ -192,6 +195,19 @@ export function RunHistoryPanel({ fixingRunSeq?: number | null; /** A run the copilot judged un-fixable, shown inline under that run's row. */ fixReason?: { seq: number; reason: string } | null; + /** + * Whether an older page of `runs` exists behind the oldest `seq` currently + * held (issue #1012) — the silent-truncation half of that issue. Omitted + * (or `false`) hides "Load older" entirely, which is also how a host + * predating the pagination fields degrades: no crash, just no affordance. + */ + hasMore?: boolean; + /** Fetch and append the next-older page. Absent hides "Load older" even if + * `hasMore` is true — a caller with nowhere to route the click should not + * offer it. */ + onLoadOlder?: () => void; + /** An older-page fetch is in flight, so "Load older" shows as busy. */ + loadingOlder?: boolean; }) { // Only one fix may be in flight at a time: `handleFixWithCopilot` sets a // single `prefilledDraft`/`editOpen` slot, so a second Fix started on a @@ -266,6 +282,20 @@ export function RunHistoryPanel({ fixReason={fixReason?.seq === run.seq ? fixReason.reason : null} /> ))} + {/* Issue #1012: the honest half of the page cap — a truncated + history says so, with a way to see more, rather than silently + ending at `limit` and reading as the whole story. */} + {hasMore && onLoadOlder && ( + + )} )} diff --git a/frontend/test/e2e/workflow-selection-persists.spec.ts b/frontend/test/e2e/workflow-selection-persists.spec.ts index 0e514df45..988c515a4 100644 --- a/frontend/test/e2e/workflow-selection-persists.spec.ts +++ b/frontend/test/e2e/workflow-selection-persists.spec.ts @@ -111,7 +111,7 @@ async function mockCompanySwitchApi(page: Page) { if (path === `/api/v1/companies/${company}/workflows`) return json(workflows[company]); if (path.endsWith("/workflows/tool-slugs")) return json({ slugs: [] }); if (path.endsWith("/workflows/wired-channels")) return json({ channels: [] }); - if (path.endsWith("/workflows/runs")) return json([]); + if (path.endsWith("/workflows/runs")) return json({ runs: [], hasMore: false }); const workflowId = path.match(/\/workflows\/([^/]+)$/)?.[1]; if (workflowId) { diff --git a/frontend/test/unit/workflow-deeplink-reread.test.ts b/frontend/test/unit/workflow-deeplink-reread.test.ts index 401197370..0a5a2c721 100644 --- a/frontend/test/unit/workflow-deeplink-reread.test.ts +++ b/frontend/test/unit/workflow-deeplink-reread.test.ts @@ -107,7 +107,7 @@ function makeClient( if (path.endsWith("/workflows")) return listFor(); if (path.includes("/workflows/tool-slugs")) return { slugs: [], unwired: [] }; if (path.includes("/workflows/wired-channels")) return { channels: [] }; - if (path.includes("/workflows/runs")) return []; + if (path.includes("/workflows/runs")) return { runs: [], hasMore: false }; const m = path.match(/\/workflows\/([^/?]+)$/); if (m) { const id = decodeURIComponent(m[1]); diff --git a/frontend/test/unit/workflow-index-first.test.ts b/frontend/test/unit/workflow-index-first.test.ts index 077ce5f78..1dbccf7d0 100644 --- a/frontend/test/unit/workflow-index-first.test.ts +++ b/frontend/test/unit/workflow-index-first.test.ts @@ -95,7 +95,7 @@ function makeClient(rows: WorkflowSummary[] = ROWS) { if (path.endsWith("/workflows")) return rows; if (path.includes("/workflows/tool-slugs")) return { slugs: [], unwired: [] }; if (path.includes("/workflows/wired-channels")) return { channels: [] }; - if (path.includes("/workflows/runs")) return []; + if (path.includes("/workflows/runs")) return { runs: [], hasMore: false }; const m = path.match(/\/workflows\/([^/?]+)$/); if (m) { const id = decodeURIComponent(m[1]); diff --git a/frontend/test/unit/workflow-index-paused.test.ts b/frontend/test/unit/workflow-index-paused.test.ts index ebf649a30..de4202c25 100644 --- a/frontend/test/unit/workflow-index-paused.test.ts +++ b/frontend/test/unit/workflow-index-paused.test.ts @@ -85,7 +85,7 @@ function makeClient(rows: WorkflowSummary[] = ROWS, created?: WorkflowGraph) { if (path.endsWith("/workflows")) return rows; if (path.includes("/workflows/tool-slugs")) return { slugs: [], unwired: [] }; if (path.includes("/workflows/wired-channels")) return { channels: [] }; - if (path.includes("/workflows/runs")) return []; + if (path.includes("/workflows/runs")) return { runs: [], hasMore: false }; const m = path.match(/\/workflows\/([^/?]+)$/); if (m) return graphFor(decodeURIComponent(m[1])); return null; diff --git a/frontend/test/unit/workflow-live-node-state.test.ts b/frontend/test/unit/workflow-live-node-state.test.ts index 4995742a7..237cb2e77 100644 --- a/frontend/test/unit/workflow-live-node-state.test.ts +++ b/frontend/test/unit/workflow-live-node-state.test.ts @@ -200,7 +200,7 @@ function fakeClient(): OpenCompanyClient { scopeFor: (company: string | null) => `/api/v1/${company ?? "company"}`, get: async (path: string) => { if (path.endsWith("/workflows")) return [{ id: GRAPH.id, name: GRAPH.name }]; - if (path.includes("/workflows/runs")) return [RUNNING_ROW]; + if (path.includes("/workflows/runs")) return { runs: [RUNNING_ROW], hasMore: false }; return GRAPH; }, post: async () => ({}), diff --git a/frontend/test/unit/workflow-run-failure.test.ts b/frontend/test/unit/workflow-run-failure.test.ts index 8b6d56e63..a0abee438 100644 --- a/frontend/test/unit/workflow-run-failure.test.ts +++ b/frontend/test/unit/workflow-run-failure.test.ts @@ -92,7 +92,7 @@ function fakeClient(post: () => Promise): OpenCompanyClient { scopeFor: (company: string | null) => `/api/v1/${company ?? "company"}`, get: async (path: string) => { if (path.endsWith("/workflows")) return [{ id: GRAPH.id, name: GRAPH.name }]; - if (path.includes("/workflows/runs")) return []; + if (path.includes("/workflows/runs")) return { runs: [], hasMore: false }; return GRAPH; }, post, diff --git a/frontend/test/unit/workflow-run-input.test.ts b/frontend/test/unit/workflow-run-input.test.ts index e18d574dd..8c6bf6ba2 100644 --- a/frontend/test/unit/workflow-run-input.test.ts +++ b/frontend/test/unit/workflow-run-input.test.ts @@ -94,7 +94,7 @@ function fakeClient(posts: Posted[]): OpenCompanyClient { scopeFor: (company: string | null) => `/api/v1/${company ?? "company"}`, get: async (path: string) => { if (path.endsWith("/workflows")) return [{ id: GRAPH.id, name: GRAPH.name }]; - if (path.includes("/workflows/runs")) return []; + if (path.includes("/workflows/runs")) return { runs: [], hasMore: false }; return GRAPH; }, post: async (path: string, body: unknown) => { diff --git a/frontend/test/unit/workflow-toolbar-layout.test.ts b/frontend/test/unit/workflow-toolbar-layout.test.ts index db30a056c..08630ccc0 100644 --- a/frontend/test/unit/workflow-toolbar-layout.test.ts +++ b/frontend/test/unit/workflow-toolbar-layout.test.ts @@ -101,7 +101,7 @@ function makeClient(rows: WorkflowSummary[] = ROWS) { if (path.endsWith("/workflows")) return rows; if (path.includes("/workflows/tool-slugs")) return { slugs: [], unwired: [] }; if (path.includes("/workflows/wired-channels")) return { channels: [] }; - if (path.includes("/workflows/runs")) return []; + if (path.includes("/workflows/runs")) return { runs: [], hasMore: false }; const m = path.match(/\/workflows\/([^/?]+)$/); if (m) return graphFor(decodeURIComponent(m[1])); return null; diff --git a/src/server/ops/inference.rs b/src/server/ops/inference.rs index c10bf594d..e95ad3ee8 100644 --- a/src/server/ops/inference.rs +++ b/src/server/ops/inference.rs @@ -1763,7 +1763,7 @@ base_url = "https://byo.example/v1" // The dead end left nothing behind — run history is still empty. let (status, runs, raw) = send(&state, "GET", "/api/v1/company/workflows/runs", None).await; assert_eq!(status, StatusCode::OK, "{raw}"); - assert_eq!(runs.as_array().map(Vec::len), Some(0), "{runs}"); + assert_eq!(runs["runs"].as_array().map(Vec::len), Some(0), "{runs}"); } /// Issue #514 (review): a config that cannot be *read* is not evidence to diff --git a/src/server/ops/workflows.rs b/src/server/ops/workflows.rs index 991f042d1..f2ea18e5c 100644 --- a/src/server/ops/workflows.rs +++ b/src/server/ops/workflows.rs @@ -90,7 +90,7 @@ use crate::company::{ }; use crate::error::OpenCompanyError; use crate::ports::types::{ - CompanyEvent, CompanyRecord, EventSeq, OverlayWorkflow, WorkflowNodeStatus, + CompanyEvent, CompanyRecord, EventSeq, OverlayWorkflow, StoredEvent, WorkflowNodeStatus, }; use crate::ports::workflow_verdict::{RunVerdictFacts, WorkflowRunVerdict}; use crate::runtime::cron::{CivilTime, CronExpr}; @@ -2369,7 +2369,15 @@ async fn workflow_wired_channels(company: ScopedCompany) -> Json, + /// Opaque pagination cursor (issue #1012): only runs whose displayed + /// `seq` is strictly less than this are considered. Absent reads the + /// newest page. The console walks backward through history by passing the + /// `seq` of the oldest run it already holds — the same `before_seq` shape + /// [`chat_history`](crate::server::chat_history)'s `?before=` and + /// `TaskDetailQuery`'s `?discussionBefore=` already use for the same + /// problem. + before_seq: Option, } /// One finished run as the console's history panel renders it (camelCase). @@ -2567,64 +2583,50 @@ impl From for WorkflowRunNode { } } -/// `GET …/workflows/runs?workflow=&limit=` — the company's finished workflow -/// runs, **newest first** (issue #228). +/// Folds a chronologically-ordered slice of journal rows into per-run outcomes +/// (issue #371's group-by-run fold). Extracted out of [`list_runs`] by issue +/// #1012 so the caller can run it repeatedly over a growing, backward-paged +/// buffer instead of once over an unbounded forward read — see the read loop +/// there. /// -/// This is the durable half of the issue: a manual run's delivery rows used to -/// live only in the console drawer until it was dismissed, and a scheduled run's -/// only on host stdout. Folding -/// [`CompanyEvent::WorkflowRunFinished`](crate::ports::types::CompanyEvent) out -/// of the journal makes both survive a console reload, which is the whole point. +/// Issue #371 turned this from a filter into a **group-by-run fold**: a run +/// now contributes up to N+2 rows (a start, one per node, a finish) instead of +/// one, and they have to come back as a single history entry. /// -/// The fold reads the company's whole event log -/// (`read_from(0, MAX)`) — the same thing -/// [`chat_history`](crate::server::chat_history) already does on every history -/// GET. Following that precedent keeps this route from inventing an index the -/// rest of the read plane doesn't have; if the journal scan ever becomes the -/// bottleneck it should be fixed for both surfaces at once, not just here. -async fn list_runs( - company: ScopedCompany, - Query(query): Query, -) -> Result>, ApiError> { - let limit = match query.limit { - Some(0) | None => DEFAULT_RUN_LIMIT, - Some(n) => n.min(MAX_RUN_LIMIT), - }; - - let stored = company - .runtime - .events() - .read_from(company.id(), EventSeq::new(0), usize::MAX) - .await - .map_err(ApiError)?; - - // Issue #371 turned this from a filter into a **group-by-run fold**: a run - // now contributes up to N+2 rows (a start, one per node, a finish) instead - // of one, and they have to come back as a single history entry. - // - // The invariant that keeps it simple: the journal is append-only and - // single-writer, so a run's rows are ordered `Started < Node… < Finished` — - // the runner drains and joins its progress collector before returning, which - // is what makes the last part true rather than a race. Rows of *different* - // runs may interleave (two workflows can run at once), so the grouping is - // keyed on run id rather than on adjacency. - // - // A pre-#371 finished row has no run id and no start, so it simply folds to - // itself — one row in, one entry out, exactly as before. +/// The invariant that keeps it simple: the journal is append-only and +/// single-writer, so a run's rows are ordered `Started < Node… < Finished` — +/// the runner drains and joins its progress collector before returning, which +/// is what makes the last part true rather than a race. Rows of *different* +/// runs may interleave (two workflows can run at once), so the grouping is +/// keyed on run id rather than on adjacency. **`rows` must be chronologically +/// ordered (ascending `seq`)** for this invariant to hold — a caller reading +/// backward via [`EventLog::read_before`](crate::ports::EventLog::read_before) +/// (which comes back newest-first) must reverse each page before it is folded +/// in. +/// +/// A pre-#371 finished row has no run id and no start, so it simply folds to +/// itself — one row in, one entry out, exactly as before. The same shape +/// results for a post-#371 row whose start fell outside `rows` — whether +/// because the caller's window does not reach that far back yet, or because a +/// retention pass pruned the `WorkflowRunStarted` row while keeping its +/// `WorkflowRunFinished` (the two are independently prunable; see +/// `CompanyEvent::retention_class`). Both are "legitimate" in the sense the +/// original comment on this fold already drew: nothing here tells them apart, +/// and nothing needs to — a caller paging backward for more history is exactly +/// how the first case resolves itself into the second, or into a real match. +/// +/// Returns the folded runs, in fold/push order (not sorted or truncated — the +/// caller does that), and the highest `seq` seen among EVERY row, matched or +/// not — the `read_through` high-water mark [`list_runs`]'s #1009 cross-check +/// resumes reading from. +fn fold_run_events(rows: Vec, wanted: Option<&str>) -> (Vec, u64) { let mut runs: Vec = Vec::new(); let mut index: std::collections::HashMap = std::collections::HashMap::new(); - // The `?workflow=` filter is applied per event rather than after the `limit` - // cut, so asking for one workflow returns that workflow's most recent N — - // not "whichever of the last N happen to match". - let wanted = query.workflow.as_deref(); let matches = |workflow_id: &str| wanted.is_none_or(|w| w == workflow_id); - // The high-water mark of the snapshot above, kept over EVERY row rather than - // only the matched ones. It is where the settle below resumes reading, which - // is what tells a run that died apart from one that merely finished while - // this request was folding. + // The high-water mark over EVERY row rather than only the matched ones. let mut read_through = 0u64; - for stored in stored { + for stored in rows { let seq = stored.seq.value(); let at_millis = stored.at_millis; read_through = read_through.max(seq); @@ -2815,216 +2817,338 @@ async fn list_runs( } } - // Issue #1009: cross-check the still-`running` rows against the live run set - // and settle the ones nobody is running. - // - // The fold above marks a start with no finish `running: true`, which is only - // ever settled by the boot sweep ([`sweep_interrupted_runs`]). Three ways a - // finish never lands — a task that panicked, an append that failed, a host - // that died — therefore all read as an eternal spinner *until the next host - // restart*, with a Stop button that cannot help and a 2s console poll that - // never stops. This closes the gap between restarts: any run the fold thinks - // is in flight whose id is **absent** from the supervisor's live set has no - // task behind it here and now, so it is journaled a synthetic finish (the - // same `INTERRUPTED_BY_RESTART` the boot sweep uses) and flipped in the - // in-memory row, so this very response is already self-consistent. - // - // Keyed strictly on `live()` membership. A run the current process is - // genuinely running is registered there and is left untouched — the watchdog - // (issue #1009, path A) is what guarantees a *panicking* run never reaches - // this predicate, because it journals its own finish before its guard drops. - // - // The one accepted false positive: a run that survived a live - // `rebuild_company` swap is registered on the *old* supervisor and so is - // absent from the successor's `live()`, so this could settle a run that is - // still walking its graph. Accepted because (i) the watchdog keeps panics out - // of this path entirely, (ii) that run's real finish lands later in journal - // order and wins the read's last-writer-wins display, and (iii) it is the - // same class the boot sweep already accepts — which is why that sweep gates - // on the handover being absent (see the runtime builder call site). It never - // corrupts the journal: that run's second, truthful finish lands *after* the - // synthetic one and so supersedes it. - // - // That last argument turns on ORDER, and it does not carry to a run which - // settles inside this request — there the truthful finish lands first and - // loses. See the window handled below; it is closed rather than accepted. - let live_ids: HashSet = company - .runtime - .run_supervisor() - .live() - .into_iter() - .map(|(run_id, _workflow_id)| run_id) - .collect(); - let mut dead: Vec = Vec::new(); - for (index, entry) in runs.iter().enumerate() { - if !entry.running { - continue; + (runs, read_through) +} + +/// `GET …/workflows/runs?workflow=&limit=&before_seq=` — the company's +/// finished workflow runs, **newest first** (issue #228), a page at a time +/// (issue #1012). +/// +/// This is the durable half of the issue: a manual run's delivery rows used to +/// live only in the console drawer until it was dismissed, and a scheduled run's +/// only on host stdout. Folding +/// [`CompanyEvent::WorkflowRunFinished`](crate::ports::types::CompanyEvent) out +/// of the journal makes both survive a console reload, which is the whole point. +/// +/// The fold walks the journal **backward**, in bounded +/// [`RUN_EVENT_PAGE`]-sized pages via +/// [`EventLog::read_before`](crate::ports::EventLog::read_before) — the same +/// pattern [`history_for_desk`](crate::server::chat_history::history_for_desk) +/// already uses for a desk transcript, which has the same +/// unbounded-forever-growing-journal problem and answers it the same way. +/// Before #1012 this read all of `read_from(0, MAX)` on every call (a company's +/// *entire* event history, not just its workflow runs — the same call +/// `chat_history` used to make too), which got slower as the journal grew and +/// never stopped growing; the backward-paged walk instead reads only as much +/// of the journal as it takes to answer this page's `limit`, plus one extra run +/// to know whether there is more (`hasMore`). +/// +/// A run is a *group* of events (`Started`, N × node events, `Finished`), not +/// one — so a page of raw events is not a page of runs. See the loop below and +/// [`fold_run_events`]'s doc for how a bracket split across a page boundary is +/// handled. +async fn list_runs( + company: ScopedCompany, + Query(query): Query, +) -> Result, ApiError> { + let limit = match query.limit { + Some(0) | None => DEFAULT_RUN_LIMIT, + Some(n) => n.min(MAX_RUN_LIMIT), + }; + // The `?workflow=` filter is applied per event rather than after the `limit` + // cut, so asking for one workflow returns that workflow's most recent N — + // not "whichever of the last N happen to match". + let wanted = query.workflow.as_deref(); + + // A run counts as ready to answer with once its `Started` row has been + // found (full data — `startedNodes`, `nodes`, `startedAtMillis` — is then + // known), or it has no run id at all (a pre-#371/gapped orphan finish, + // complete by definition — nothing more to wait for). A run whose + // `Finished`/node row has been seen but whose `Started` has not (yet) is + // still open: walking backward means its `Started` row, if it exists, is + // further back than what has been read so far. + let is_settled = + |run: &WorkflowRunOutcome| run.run_id.is_none() || run.started_at_millis.is_some(); + + let mut cursor = query.before_seq.map(EventSeq::new); + let mut buffer: Vec = Vec::new(); + let mut runs: Vec = Vec::new(); + let mut read_through = 0u64; + // Whether the walk reached the true beginning of the journal — the only + // condition under which an open (not-yet-settled) run can be trusted as + // permanently orphaned rather than merely not-yet-resolved. See the + // `retain` below. No placeholder initial value: every path out of the loop + // assigns it before breaking, so the compiler can already prove it is set + // by the time it is read after the loop. `mut` because the loop can + // reassign it once per page before the page that finally breaks out. + let mut exhausted; + loop { + let page = company + .runtime + .events() + .read_before(company.id(), cursor, RUN_EVENT_PAGE) + .await + .map_err(ApiError)?; + if page.is_empty() { + exhausted = true; + break; } - let Some(run_id) = entry.run_id.as_ref() else { - continue; - }; - if live_ids.contains(run_id) { - continue; + // A page shorter than asked-for proves there is nothing older left to + // read, without waiting for one more round trip that would only + // confirm it empty. + exhausted = page.len() < RUN_EVENT_PAGE; + // `read_before` returns newest-first; its own last element is this + // page's oldest row, and the correct cursor to resume strictly before. + cursor = page.last().map(|event| event.seq); + let mut chrono_page = page; + chrono_page.reverse(); + buffer.splice(0..0, chrono_page); + + let (folded, through) = fold_run_events(buffer.clone(), wanted); + // `read_through`'s meaning — the high-water mark of a "now" snapshot, + // which the #1009 cross-check below resumes reading from — is fixed + // by the FIRST page: `fold_run_events` computes it as the buffer's max + // `seq`, and the buffer only grows *older* on every later page, so + // this value cannot change after the first assignment. Reassigning it + // unconditionally is simplest and gives the identical answer. + read_through = through; + let settled = folded.iter().filter(|run| is_settled(run)).count(); + runs = folded; + if exhausted || settled > limit { + break; } - dead.push(index); + } + if !exhausted { + // Drop any run still open: walking further back might yet resolve it + // (find its `Started` row) or might not, and returning it now, in the + // fold's orphan placeholder shape, would risk showing a real run's + // history as gapped when the only reason it looks that way is that + // this page chose not to read far enough. Once the journal actually IS + // exhausted, every remaining open row is a genuine orphan — see + // `fold_run_events`'s doc — and is kept exactly as the fold shaped it. + runs.retain(is_settled); } - // ── The window between the snapshot and `live()` ──────────────────────── - // - // `live()` is consulted AFTER the journal snapshot was taken, and a run can - // settle in between: it appends its finish (too late for the snapshot) and - // then drops its guard (in time to be missing from `live()`). Such a run is - // indistinguishable, on the two facts above, from one that died — but it is - // the opposite, and settling it is worse than the hang this repairs. - // - // The ordering is what makes it worse rather than merely wrong. The rebuild - // false positive this block already accepts is self-correcting because the - // run's real finish lands *after* the synthetic one, and the fold settles an - // entry from the last finish it sees. Here the real finish lands *first*, so - // the synthetic one wins for good: a successful run reads - // `INTERRUPTED_BY_RESTART` permanently, and because the fold overwrites - // `deliveries` from whichever finish settles last, the record of what it - // sent is replaced by an empty list. - // - // So before writing anything, read the journal on from where the snapshot - // stopped and drop any candidate whose finish turns up there. That is - // exact rather than a heuristic: a run whose start was in the snapshot was - // registered before it (`begin` precedes both the spawn and the runner's - // `WorkflowRunStarted`), so a candidate missing from `live()` has already - // been deregistered — and a deregistered run journaled its finish first, if - // it was ever going to. Anything appended before that point is at a higher - // sequence than the whole snapshot, so this second read cannot miss it. - // - // Cheap where it matters: it runs only when there are candidates at all, - // which after the first settle is nothing, and it reads only the tail. - if !dead.is_empty() { - match company + // Issue #1012: this cross-check only makes sense against "now" — an + // older page (a `before_seq` cursor was given) is not the newest state, + // so a `running: true` row on it is out of scope here: either it was + // already resolved by an earlier newest-page read (whose synthetic + // finish will surface naturally once an older page's window reaches + // that seq), or it is a genuinely long-lived run outside what + // pagination is meant to answer. + if query.before_seq.is_none() { + // Issue #1009: cross-check the still-`running` rows against the live run set + // and settle the ones nobody is running. + // + // The fold above marks a start with no finish `running: true`, which is only + // ever settled by the boot sweep ([`sweep_interrupted_runs`]). Three ways a + // finish never lands — a task that panicked, an append that failed, a host + // that died — therefore all read as an eternal spinner *until the next host + // restart*, with a Stop button that cannot help and a 2s console poll that + // never stops. This closes the gap between restarts: any run the fold thinks + // is in flight whose id is **absent** from the supervisor's live set has no + // task behind it here and now, so it is journaled a synthetic finish (the + // same `INTERRUPTED_BY_RESTART` the boot sweep uses) and flipped in the + // in-memory row, so this very response is already self-consistent. + // + // Keyed strictly on `live()` membership. A run the current process is + // genuinely running is registered there and is left untouched — the watchdog + // (issue #1009, path A) is what guarantees a *panicking* run never reaches + // this predicate, because it journals its own finish before its guard drops. + // + // The one accepted false positive: a run that survived a live + // `rebuild_company` swap is registered on the *old* supervisor and so is + // absent from the successor's `live()`, so this could settle a run that is + // still walking its graph. Accepted because (i) the watchdog keeps panics out + // of this path entirely, (ii) that run's real finish lands later in journal + // order and wins the read's last-writer-wins display, and (iii) it is the + // same class the boot sweep already accepts — which is why that sweep gates + // on the handover being absent (see the runtime builder call site). It never + // corrupts the journal: that run's second, truthful finish lands *after* the + // synthetic one and so supersedes it. + // + // That last argument turns on ORDER, and it does not carry to a run which + // settles inside this request — there the truthful finish lands first and + // loses. See the window handled below; it is closed rather than accepted. + let live_ids: HashSet = company .runtime - .events() - .read_from( - company.id(), - EventSeq::new(read_through.saturating_add(1)), - usize::MAX, - ) - .await - { - Ok(tail) => { - let settled_since: HashSet = tail - .into_iter() - .filter_map(|stored| match stored.event { - CompanyEvent::WorkflowRunFinished { - run_id: Some(run_id), - .. - } => Some(run_id), - _ => None, - }) - .collect(); - dead.retain(|index| { - runs[*index] - .run_id - .as_ref() - .is_none_or(|run_id| !settled_since.contains(run_id)) - }); + .run_supervisor() + .live() + .into_iter() + .map(|(run_id, _workflow_id)| run_id) + .collect(); + let mut dead: Vec = Vec::new(); + for (index, entry) in runs.iter().enumerate() { + if !entry.running { + continue; } - Err(err) => { - // Unprovable, so nothing is settled. The row keeps reporting - // `running` and a later poll retries — strictly better than - // stamping "interrupted" on a run that may well be finishing. - tracing::warn!( - company = %company.id(), - %err, - "could not re-read the journal to confirm a workflow run is dead; \ - leaving it as running" - ); - dead.clear(); + let Some(run_id) = entry.run_id.as_ref() else { + continue; + }; + if live_ids.contains(run_id) { + continue; } + dead.push(index); } - } - for index in &dead { - let entry = &mut runs[*index]; - let Some(run_id) = entry.run_id.clone() else { - continue; - }; - // Durable half: append the finish so it survives this response, folds - // settled on the next `GET …/workflows/runs`, and stops the boot sweep - // from having to. Best-effort by construction — a failed append leaves - // the row as the in-memory flip below still makes it, and the next read - // simply retries. - crate::runtime::record_run_finished( - company.runtime.events(), - company.id(), - &entry.workflow_id, - entry.scheduled, - &run_id, - Err(crate::runtime::workflow_outcome::INTERRUPTED_BY_RESTART.into()), - ) - .await; - // In-memory half: flip the row this response returns, so the console does - // not have to wait for the next poll to stop the spinner. - entry.running = false; - entry.error = Some(crate::runtime::workflow_outcome::INTERRUPTED_BY_RESTART.to_string()); - } + // ── The window between the snapshot and `live()` ──────────────────────── + // + // `live()` is consulted AFTER the journal snapshot was taken, and a run can + // settle in between: it appends its finish (too late for the snapshot) and + // then drops its guard (in time to be missing from `live()`). Such a run is + // indistinguishable, on the two facts above, from one that died — but it is + // the opposite, and settling it is worse than the hang this repairs. + // + // The ordering is what makes it worse rather than merely wrong. The rebuild + // false positive this block already accepts is self-correcting because the + // run's real finish lands *after* the synthetic one, and the fold settles an + // entry from the last finish it sees. Here the real finish lands *first*, so + // the synthetic one wins for good: a successful run reads + // `INTERRUPTED_BY_RESTART` permanently, and because the fold overwrites + // `deliveries` from whichever finish settles last, the record of what it + // sent is replaced by an empty list. + // + // So before writing anything, read the journal on from where the snapshot + // stopped and drop any candidate whose finish turns up there. That is + // exact rather than a heuristic: a run whose start was in the snapshot was + // registered before it (`begin` precedes both the spawn and the runner's + // `WorkflowRunStarted`), so a candidate missing from `live()` has already + // been deregistered — and a deregistered run journaled its finish first, if + // it was ever going to. Anything appended before that point is at a higher + // sequence than the whole snapshot, so this second read cannot miss it. + // + // Cheap where it matters: it runs only when there are candidates at all, + // which after the first settle is nothing, and it reads only the tail. + if !dead.is_empty() { + match company + .runtime + .events() + .read_from( + company.id(), + EventSeq::new(read_through.saturating_add(1)), + usize::MAX, + ) + .await + { + Ok(tail) => { + let settled_since: HashSet = tail + .into_iter() + .filter_map(|stored| match stored.event { + CompanyEvent::WorkflowRunFinished { + run_id: Some(run_id), + .. + } => Some(run_id), + _ => None, + }) + .collect(); + dead.retain(|index| { + runs[*index] + .run_id + .as_ref() + .is_none_or(|run_id| !settled_since.contains(run_id)) + }); + } + Err(err) => { + // Unprovable, so nothing is settled. The row keeps reporting + // `running` and a later poll retries — strictly better than + // stamping "interrupted" on a run that may well be finishing. + tracing::warn!( + company = %company.id(), + %err, + "could not re-read the journal to confirm a workflow run is dead; \ + leaving it as running" + ); + dead.clear(); + } + } + } - // ── Serve the row the NEXT read will fold, identically ────────────────── - // - // The fold keys a settled entry on its **finish**, taking `seq` and - // `at_millis` from that row. So flipping `running` while leaving the - // start's values in place means this response and the one 2s later carry - // *different* `seq` for the same run — and the console keys its history - // rows on exactly that field (`RunHistoryPanel`: `key={run.seq}`, with - // `selectedRunSeq` / `fixingRunSeq` / `fixReason.seq` compared against it). - // The row remounts and any selection on it is dropped, in the one window - // where an operator is most likely to be looking: the 2s recovery poll runs - // precisely because someone is watching this run. - // - // So the appended rows are read back and their real `seq` / `at_millis` - // stamped on. Read back rather than returned from `record_run_finished`, - // which reports only whether the append happened — the values served are - // then the durable ones rather than a second construction of them. - if !dead.is_empty() { - match company - .runtime - .events() - .read_from( + for index in &dead { + let entry = &mut runs[*index]; + let Some(run_id) = entry.run_id.clone() else { + continue; + }; + // Durable half: append the finish so it survives this response, folds + // settled on the next `GET …/workflows/runs`, and stops the boot sweep + // from having to. Best-effort by construction — a failed append leaves + // the row as the in-memory flip below still makes it, and the next read + // simply retries. + crate::runtime::record_run_finished( + company.runtime.events(), company.id(), - EventSeq::new(read_through.saturating_add(1)), - usize::MAX, + &entry.workflow_id, + entry.scheduled, + &run_id, + Err(crate::runtime::workflow_outcome::INTERRUPTED_BY_RESTART.into()), ) - .await - { - Ok(appended) => { - let stamped: std::collections::HashMap = appended - .into_iter() - .filter_map(|stored| match stored.event { - CompanyEvent::WorkflowRunFinished { - run_id: Some(run_id), - .. - } => Some((run_id, (stored.seq.value(), stored.at_millis))), - _ => None, - }) - .collect(); - for index in &dead { - let entry = &mut runs[*index]; - let Some((seq, at_millis)) = - entry.run_id.as_ref().and_then(|id| stamped.get(id)) - else { - continue; - }; - entry.seq = *seq; - entry.at_millis = *at_millis; + .await; + // In-memory half: flip the row this response returns, so the console does + // not have to wait for the next poll to stop the spinner. + entry.running = false; + entry.error = + Some(crate::runtime::workflow_outcome::INTERRUPTED_BY_RESTART.to_string()); + } + + // ── Serve the row the NEXT read will fold, identically ────────────────── + // + // The fold keys a settled entry on its **finish**, taking `seq` and + // `at_millis` from that row. So flipping `running` while leaving the + // start's values in place means this response and the one 2s later carry + // *different* `seq` for the same run — and the console keys its history + // rows on exactly that field (`RunHistoryPanel`: `key={run.seq}`, with + // `selectedRunSeq` / `fixingRunSeq` / `fixReason.seq` compared against it). + // The row remounts and any selection on it is dropped, in the one window + // where an operator is most likely to be looking: the 2s recovery poll runs + // precisely because someone is watching this run. + // + // So the appended rows are read back and their real `seq` / `at_millis` + // stamped on. Read back rather than returned from `record_run_finished`, + // which reports only whether the append happened — the values served are + // then the durable ones rather than a second construction of them. + if !dead.is_empty() { + match company + .runtime + .events() + .read_from( + company.id(), + EventSeq::new(read_through.saturating_add(1)), + usize::MAX, + ) + .await + { + Ok(appended) => { + let stamped: std::collections::HashMap = appended + .into_iter() + .filter_map(|stored| match stored.event { + CompanyEvent::WorkflowRunFinished { + run_id: Some(run_id), + .. + } => Some((run_id, (stored.seq.value(), stored.at_millis))), + _ => None, + }) + .collect(); + for index in &dead { + let entry = &mut runs[*index]; + let Some((seq, at_millis)) = + entry.run_id.as_ref().and_then(|id| stamped.get(id)) + else { + continue; + }; + entry.seq = *seq; + entry.at_millis = *at_millis; + } + } + Err(err) => { + // The settle itself stands — it is already durable. Only the + // row's identity is left at the start's, which the next read + // corrects. + tracing::warn!( + company = %company.id(), + %err, + "settled a dead workflow run but could not read back its finish row; \ + this response carries the start's seq and time" + ); } - } - Err(err) => { - // The settle itself stands — it is already durable. Only the - // row's identity is left at the start's, which the next read - // corrects. - tracing::warn!( - company = %company.id(), - %err, - "settled a dead workflow run but could not read back its finish row; \ - this response carries the start's seq and time" - ); } } } @@ -3043,10 +3167,29 @@ async fn list_runs( run.verdict = run.derive_verdict(); } - // Newest first: a history panel leads with the run that just happened. The - // `limit` now cuts *runs* rather than journal rows, which is the number the - // caller was asking about all along. - runs.reverse(); + // Newest first: a history panel leads with the run that just happened. + // + // Issue #1012: sorted explicitly by `(at_millis, seq)` descending — the + // very pair every row *displays* — rather than `reverse()`d. `reverse()` + // only flips the fold's push order, which is the order runs *started* (an + // entry is pushed once, at its `WorkflowRunStarted` row, and only mutated + // in place — never re-pushed — when its `WorkflowRunFinished` row later + // overwrites `seq`/`at_millis` to the finish's own). Two runs that + // interleave (B starts after A, but finishes first) therefore used to come + // back in *start* order while every row read as if it were ordered by + // *finish* — the row for A would lead even though B's `seq`/`atMillis` say + // B is newer. Sorting on the same field the row displays makes the two + // agree by construction, for a run still in flight (which sorts on its own + // start) exactly as for one already settled. + // + // The `limit` now cuts *runs* rather than journal rows, which is the + // number the caller was asking about all along. + runs.sort_by_key(|r| std::cmp::Reverse((r.at_millis, r.seq))); + // Issue #1012: the backward-paged read above stopped once it had settled + // at least `limit + 1` runs (or ran out of journal), precisely so this + // count is known here — one more than fit on the page means there is a + // page after this one. + let has_more = runs.len() > limit; runs.truncate(limit); // Issue #1143. A blocked node's `approval_ids` is a receipt of what the run @@ -3087,7 +3230,22 @@ async fn list_runs( } } - Ok(Json(runs)) + Ok(Json(WorkflowRunsResponse { runs, has_more })) +} + +/// The `GET …/workflows/runs` response body (issue #1012). +/// +/// Wrapped rather than a bare array — as this route answered before — because +/// `hasMore` has nowhere else to ride: the console's history drawer cannot +/// otherwise tell "this is the whole history" from "this page was truncated at +/// `limit`", which is exactly the silent-truncation half of the issue. +#[derive(Debug, Serialize)] +#[serde(rename_all = "camelCase")] +struct WorkflowRunsResponse { + runs: Vec, + /// Whether a further, older page exists behind `?before_seq=`. + has_more: bool, } /// Relabels a run's node rows for the nodes it blocked on a human (issue #881). @@ -5322,7 +5480,7 @@ mod tests { .unwrap(); assert_eq!(response.status(), StatusCode::OK); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 2, "body: {body}"); // Newest first: the history panel leads with the run that just ran. @@ -5390,7 +5548,7 @@ mod tests { .unwrap(); assert_eq!(response.status(), StatusCode::OK); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 3, "body: {body}"); // The more serious fact first: a run that broke mid-graph AND did @@ -5449,7 +5607,7 @@ mod tests { .unwrap(); assert_eq!(response.status(), StatusCode::OK); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1, "body: {body}"); assert_eq!(rows[0]["running"], true, "{body}"); assert_eq!(rows[0]["verdict"], "running", "{body}"); @@ -5538,10 +5696,10 @@ mod tests { .unwrap(); let body = json_body(response).await; assert_eq!( - body[0]["error"], + body["runs"][0]["error"], "no inference source for agent node `worker`" ); - assert_eq!(body[0]["deliveries"].as_array().unwrap().len(), 0); + assert_eq!(body["runs"][0]["deliveries"].as_array().unwrap().len(), 0); } // ── Issue #371: the per-node progress fold ───────────────────────── @@ -5690,7 +5848,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1, "four journal rows fold to one run: {body}"); assert_eq!(rows[0]["runId"], "run-1"); @@ -5746,7 +5904,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1, "one run: {body}"); assert_eq!(rows[0]["running"], true, "still in flight: {body}"); assert_eq!(rows[0]["runId"], run, "{body}"); @@ -5794,7 +5952,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1, "only the asked-for workflow: {body}"); assert_eq!(rows[0]["runId"], "run-mine"); let started = rows[0]["startedNodes"].as_array().expect("startedNodes"); @@ -5828,7 +5986,7 @@ mod tests { .unwrap(); let body = json_body(response).await; assert!( - body[0].get("startedNodes").is_none(), + body["runs"][0].get("startedNodes").is_none(), "an empty trail is absent, not `[]`: {body}" ); } @@ -5868,11 +6026,13 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - assert!(body[0].get("running").is_none(), "settled: {body}"); - let started = body[0]["startedNodes"].as_array().expect("startedNodes"); + assert!(body["runs"][0].get("running").is_none(), "settled: {body}"); + let started = body["runs"][0]["startedNodes"] + .as_array() + .expect("startedNodes"); assert_eq!(started.len(), 2, "{body}"); assert_eq!(started[1], "draft"); - let nodes = body[0]["nodes"].as_array().expect("nodes"); + let nodes = body["runs"][0]["nodes"].as_array().expect("nodes"); assert_eq!(nodes.len(), 1, "`draft` never finished: {body}"); } @@ -5943,7 +6103,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1, "{body}"); assert!( rows[0].get("error").is_none(), @@ -6007,7 +6167,7 @@ mod tests { .unwrap(); let body = json_body(response).await; assert_eq!( - body[0]["blockedNodes"][0]["stranded"], 1, + body["runs"][0]["blockedNodes"][0]["stranded"], 1, "an approval id the journal no longer holds must read as stranded, \ or the drawer goes on linking to an empty queue: {body}" ); @@ -6092,7 +6252,7 @@ mod tests { .unwrap(); let body = json_body(response).await; assert!( - body[0]["blockedNodes"][0].get("stranded").is_none(), + body["runs"][0]["blockedNodes"][0].get("stranded").is_none(), "a parked approval is still decidable, so nothing may be marked \ stranded: {body}" ); @@ -6131,9 +6291,9 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - assert_eq!(body[0]["running"], true, "{body}"); - assert_eq!(body[0]["nodes"].as_array().unwrap().len(), 1); - assert!(body[0].get("error").is_none(), "{body}"); + assert_eq!(body["runs"][0]["running"], true, "{body}"); + assert_eq!(body["runs"][0]["nodes"].as_array().unwrap().len(), 1); + assert!(body["runs"][0].get("error").is_none(), "{body}"); } /// An event log that lets exactly one run settle **inside** `list_runs`' @@ -6318,7 +6478,7 @@ mod tests { // still the snapshot's, `running: true`. That is honest: the run WAS // in flight when the journal was sampled. What matters is that it is // not stamped dead. - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1, "{body}"); assert!( rows[0].get("error").is_none(), @@ -6335,9 +6495,12 @@ mod tests { .unwrap(), ) .await; - assert!(next[0].get("running").is_none(), "settled: {next}"); - assert!(next[0].get("error").is_none(), "a successful run: {next}"); - assert_eq!(next[0]["deliveries"][0]["status"], "sent", "{next}"); + assert!(next["runs"][0].get("running").is_none(), "settled: {next}"); + assert!( + next["runs"][0].get("error").is_none(), + "a successful run: {next}" + ); + assert_eq!(next["runs"][0]["deliveries"][0]["status"], "sent", "{next}"); } /// **A settled row keeps one identity across reads.** The response that @@ -6374,21 +6537,24 @@ mod tests { ) .await; - assert!(first[0].get("running").is_none(), "settled: {first}"); + assert!( + first["runs"][0].get("running").is_none(), + "settled: {first}" + ); assert_eq!( - first[0]["seq"], second[0]["seq"], + first["runs"][0]["seq"], second["runs"][0]["seq"], "the settling read and the one after it must agree on the row's \ identity: {first} then {second}" ); assert_eq!( - first[0]["atMillis"], second[0]["atMillis"], + first["runs"][0]["atMillis"], second["runs"][0]["atMillis"], "…and on when it settled: {first} then {second}" ); // Specifically the FINISH's row, which is what the next fold uses — // not the start's, which is the only other candidate. assert!( - first[0]["atMillis"].as_u64().unwrap() - >= first[0]["startedAtMillis"].as_u64().unwrap(), + first["runs"][0]["atMillis"].as_u64().unwrap() + >= first["runs"][0]["startedAtMillis"].as_u64().unwrap(), "the settle cannot predate the start: {first}" ); } @@ -6410,7 +6576,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1); assert_eq!(rows[0]["workflowId"], "digest"); assert!(rows[0].get("nodes").is_none(), "{body}"); @@ -6440,7 +6606,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 2, "{body}"); let by_id = |run: &str| { rows.iter() @@ -6454,6 +6620,44 @@ mod tests { assert_eq!(by_id("run-b")["nodes"].as_array().unwrap().len(), 1); } + /// Issue #1012. Two interleaved runs — `run-a` starts first but + /// `run-b` finishes last — must come back ordered by **finish**, the + /// field every row displays, not by the order they started. The old + /// `runs.reverse()` only flipped the fold's push order (start order), + /// so this exact fixture used to list `run-a` first despite `run-b` + /// carrying the newer `seq`/`atMillis`. + #[tokio::test] + async fn run_history_orders_by_finish_not_start() { + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let (state, _store, id) = hosted_state(&home).await; + + // Start order: a, then b. Finish order: a, then b — so b is both + // the last to start AND the last to finish, which alone would not + // distinguish "ordered by start" from "ordered by finish". Insert + // a THIRD run, `c`, that starts before `b` but finishes before `a` + // too, so the two orderings genuinely disagree on where it lands. + journal_start(&state, &id, "wf", "run-a", false).await; + journal_start(&state, &id, "wf", "run-c", false).await; + journal_finish(&state, &id, "wf", "run-c", false, None).await; + journal_start(&state, &id, "wf", "run-b", false).await; + journal_finish(&state, &id, "wf", "run-a", false, None).await; + journal_finish(&state, &id, "wf", "run-b", false, None).await; + + // Start order: a, c, b. Finish order: c, a, b. + // By-start reversal would read: b, c, a (wrong). + // By-finish descending must read: b, a, c. + let response = router(state) + .oneshot(request("GET", "/api/v1/company/workflows/runs", None)) + .await + .unwrap(); + let body = json_body(response).await; + let rows = body["runs"].as_array().expect("array"); + assert_eq!(rows.len(), 3, "{body}"); + let ids: Vec<&str> = rows.iter().map(|r| r["runId"].as_str().unwrap()).collect(); + assert_eq!(ids, vec!["run-b", "run-a", "run-c"], "{body}"); + } + /// `?limit=` now cuts **runs**, not journal rows — the number the caller /// was asking about all along. Without the group-aware cut, a limit of 2 /// over three 4-row runs would return fragments. @@ -6480,7 +6684,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 2, "{body}"); // Newest first, and each one whole. assert_eq!(rows[0]["runId"], "run-2"); @@ -6513,7 +6717,7 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - let rows = body.as_array().expect("array"); + let rows = body["runs"].as_array().expect("array"); assert_eq!(rows.len(), 1, "body: {body}"); assert_eq!(rows[0]["workflowId"], "digest"); } @@ -6545,14 +6749,14 @@ mod tests { // Explicit cap, taken from the newest end. let capped = page("/api/v1/company/workflows/runs?limit=3").await; - let rows = capped.as_array().expect("array"); + let rows = capped["runs"].as_array().expect("array"); assert_eq!(rows.len(), 3); assert_eq!(rows[0]["workflowId"], "wf-24", "newest first: {capped}"); // No `limit` → the default page, not the whole 25. let defaulted = page("/api/v1/company/workflows/runs").await; assert_eq!( - defaulted.as_array().unwrap().len(), + defaulted["runs"].as_array().unwrap().len(), DEFAULT_RUN_LIMIT, "{defaulted}" ); @@ -6560,11 +6764,15 @@ mod tests { // `limit=0` means "I didn't really mean zero" — an empty page is // never what a caller wants, so it falls back to the default. let zero = page("/api/v1/company/workflows/runs?limit=0").await; - assert_eq!(zero.as_array().unwrap().len(), DEFAULT_RUN_LIMIT, "{zero}"); + assert_eq!( + zero["runs"].as_array().unwrap().len(), + DEFAULT_RUN_LIMIT, + "{zero}" + ); // Above the ceiling clamps; with only 25 rows that is all of them. let huge = page("/api/v1/company/workflows/runs?limit=100000").await; - assert_eq!(huge.as_array().unwrap().len(), 25, "{huge}"); + assert_eq!(huge["runs"].as_array().unwrap().len(), 25, "{huge}"); } /// A company that has never run a workflow gets an empty list, not a @@ -6580,7 +6788,10 @@ mod tests { .await .unwrap(); assert_eq!(response.status(), StatusCode::OK); - assert_eq!(json_body(response).await.as_array().unwrap().len(), 0); + assert_eq!( + json_body(response).await["runs"].as_array().unwrap().len(), + 0 + ); } /// **Route-ordering pin.** `runs` is a syntactically valid `wid`, so @@ -6604,9 +6815,12 @@ mod tests { StatusCode::OK, "the static /workflows/runs must win over /workflows/{{wid}}" ); - // An array of outcomes, not a single graph object. + // A runs page — `{ runs: [...], hasMore }` — not a single graph object. let body = json_body(response).await; - assert!(body.is_array(), "graph read shadowed the history: {body}"); + assert!( + body["runs"].is_array(), + "graph read shadowed the history: {body}" + ); } // ------------------------------------------------------------------- @@ -6737,7 +6951,7 @@ mod tests { .unwrap(); assert_eq!(response.status(), StatusCode::OK); let body = json_body(response).await; - assert_eq!(body[0]["workflowId"], "digest"); + assert_eq!(body["runs"][0]["workflowId"], "digest"); } // ── Issue #259: edit + delete at the HTTP boundary ────────────────── @@ -7298,12 +7512,9 @@ mod tests { .await .unwrap(); assert_eq!(response.status(), StatusCode::OK); - let rows = json_body(response).await; - assert_eq!( - rows.as_array().unwrap().len(), - 1, - "past runs must outlive the workflow: {rows}" - ); + let body = json_body(response).await; + let rows = body["runs"].as_array().unwrap(); + assert_eq!(rows.len(), 1, "past runs must outlive the workflow: {body}"); assert_eq!(rows[0]["workflowId"], "greeter"); } @@ -7552,15 +7763,15 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - assert_eq!(body.as_array().unwrap().len(), 1); + assert_eq!(body["runs"].as_array().unwrap().len(), 1); // `running` is skip-serialized when false, so a settled row simply // omits it — assert it is not `true` rather than equal to `false`. assert_ne!( - body[0]["running"], true, + body["runs"][0]["running"], true, "an absent run is settled on the read, not left spinning: {body}" ); assert_eq!( - body[0]["error"], + body["runs"][0]["error"], crate::runtime::workflow_outcome::INTERRUPTED_BY_RESTART ); @@ -7602,9 +7813,9 @@ mod tests { .await .unwrap(); let body = json_body(response).await; - assert_eq!(body.as_array().unwrap().len(), 1); + assert_eq!(body["runs"].as_array().unwrap().len(), 1); assert_eq!( - body[0]["running"], true, + body["runs"][0]["running"], true, "a run the process is running must not be settled from under it" ); @@ -8182,7 +8393,7 @@ label = "ok" .await .unwrap(); let rows = json_body(response).await; - let row = &rows.as_array().expect("array")[0]; + let row = &rows["runs"].as_array().expect("array")[0]; assert_eq!(row["runId"], run_id.as_str(), "{rows}"); assert_eq!(row["cancelled"], true, "{rows}"); assert!( diff --git a/src/server/ops/write_test.rs b/src/server/ops/write_test.rs index e7a32ecbd..5810349fd 100644 --- a/src/server/ops/write_test.rs +++ b/src/server/ops/write_test.rs @@ -9114,7 +9114,7 @@ async fn the_run_history_carries_a_runs_board_rows() { let (status, runs) = send(&state, "GET", "/api/v1/company/workflows/runs", None).await; assert_eq!(status, StatusCode::OK); - let runs = runs.as_array().expect("an array of runs"); + let runs = runs["runs"].as_array().expect("an array of runs"); let settled = runs .iter() From 7afadec648e6a3e2dcc2511050b4dac9b5d6aa38 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 20 Aug 2026 16:46:55 +0530 Subject: [PATCH 02/14] feat: real local ACP engine + per-harness model override (#1245) Named harnesses (#993) let a teammate bind to an `acp` harness, but no engine existed to actually run that turn -- lanes::build unconditionally recorded every acp harness `unavailable`. This wires a real one for `transport = "local"`, plus a `model` field so a power user can pin a specific model on it, mirroring the pattern block/buzz already ships (a teammate carries harness + model as independent settings, and the host injects the model into the harness's own startup lever). - `AcpHarness.model: Option` (src/company/types.rs) + validation (manifest.rs): a plain string hint forwarded to the agent's own lever, not a credential, so it does not join `[harness.inference]`'s prohibition on acp harnesses. Rejected on `transport = "runner"` (no wire protocol for it yet) and when empty. - The `AcpAgent`/`AcpAgentFactory`/`AcpTurn`/`AcpUpdate` port moved from `harness::acp::run_turn` (behind the `openhuman` feature) to `src/ports/acp.rs`, ungated. The desktop shell -- the only implementation this crate does not itself provide -- does not enable `openhuman` on its `opencompany` dependency at all, so the port had to live somewhere it could actually see without pulling in the whole embedded-engine dependency tree. `harness::acp::run_turn` re-exports the types and keeps `AcpRunTurn`/`fold`, which do need `openhuman`'s `TurnStep`/`RunTurn`. - `lanes::build` resolves a real engine for `transport = "local"` when given a factory (`Option<&dyn AcpAgentFactory>`, `#[cfg(feature = "acp")]` with an uninhabited-type fallback for `openhuman`-without-`acp` builds); `transport = "runner"` still resolves `unavailable` (its own, larger piece of work). - `LocalAcpAgent`/`LocalAcpAgentFactory` (src-tauri/src/acp/local_agent.rs): spawns the harness's CLI via the existing `AcpClient`, demultiplexes ACP's single global `session/update` stream by session id (one subprocess serves every teammate on the harness), and injects the model via a per-CLI env var confirmed live against the real adapter -- `ANTHROPIC_MODEL` for claude, `GOOSE_MODEL` for goose. `codex` has no confirmed lever yet (validated but not injected, rather than guessed). V1 fails closed on ACP permission requests rather than routing them through the company's approval-policy gate -- a known, documented gap, not the intended end state. - `AppState::with_acp_agents` (src/app/types.rs) threads the factory to `desktop::register`, mirroring `with_rebuilder`'s exact pattern; wired for real in src-tauri/src/embedded.rs. - Found and fixed a real bug via live testing: discovery.rs's catalog still named the legacy `claude-code-acp` binary; the current package installs `claude-agent-acp`. Would have silently failed every spawn on a current install. Live-tested against a real, authenticated claude-agent-acp (not just the scripted fixture): a real prompt/response round trip, `session/new` advertising a model config option, `ANTHROPIC_MODEL` actually steering the reported current model, and the full `LocalAcpAgent` path through the `AcpAgent` trait -- see src-tauri/tests/acp_live_smoke.rs (`#[ignore]`d, costs real usage, never runs in CI). Co-Authored-By: Claude --- docs/spec/runtime/harnesses.md | 83 ++++++-- src-tauri/src/acp/client.rs | 7 + src-tauri/src/acp/discovery.rs | 18 +- src-tauri/src/acp/local_agent.rs | 320 ++++++++++++++++++++++++++++++ src-tauri/src/acp/mod.rs | 2 + src-tauri/src/embedded.rs | 7 +- src-tauri/tests/acp_client.rs | 1 + src-tauri/tests/acp_live_smoke.rs | 246 +++++++++++++++++++++++ src/app/types.rs | 20 ++ src/company/manifest.rs | 51 +++++ src/company/types.rs | 11 + src/desktop.rs | 9 + src/harness/acp/run_turn.rs | 69 +------ src/harness/built_in/brain.rs | 1 + src/harness/lanes.rs | 80 ++++++++ src/ports/acp.rs | 124 ++++++++++++ src/ports/mod.rs | 2 + src/runtime/builder.rs | 38 ++++ 18 files changed, 1006 insertions(+), 83 deletions(-) create mode 100644 src-tauri/src/acp/local_agent.rs create mode 100644 src-tauri/tests/acp_live_smoke.rs create mode 100644 src/ports/acp.rs diff --git a/docs/spec/runtime/harnesses.md b/docs/spec/runtime/harnesses.md index 46e7cfdde..4e5c4a718 100644 --- a/docs/spec/runtime/harnesses.md +++ b/docs/spec/runtime/harnesses.md @@ -64,6 +64,7 @@ kind = "acp" [harness.acp] transport = "local" agent = "claude" +model = "claude-opus-4-5" # optional — see "Model", below ``` `[harness.inference]` and `[harness.acp]` attach to the **most recently @@ -71,6 +72,28 @@ declared** `[[harness]]`. That is ordinary TOML array-of-tables sub-table syntax, but it is easy to misread as a company-level section, so it is worth reading twice. +### Model + +`[harness.acp].model` is a hint forwarded to the agent's own startup lever — +not a credential, so it does not join `[harness.inference]`'s prohibition on +`acp` harnesses (see [Validation](#validation)). Optional; a harness with none +runs whatever the agent's own config or CLI default resolves to. + +Whether it actually does anything depends on whether this build knows a +startup lever for that `agent` — confirmed live against the real adapters +(issue #1245), not guessed: + +| `agent` | lever | +|---|---| +| `claude` | `ANTHROPIC_MODEL` | +| `goose` | `GOOSE_MODEL` | +| `codex` | none known yet — `model` is accepted and validated, but not injected | + +`transport = "local"` only, for now: the `runner` wire protocol does not carry +`model`, so validation rejects it there rather than accepting and silently +dropping it — the same "my model setting does nothing" failure mode +[Validation](#validation) already guards against for `[harness.inference]`. + ### Binding an agent ```toml @@ -106,6 +129,8 @@ engine, which is never true. - `[harness.inference]` on an `acp` kind, or `[harness.acp]` on a `built_in` one - `transport = "local"` with no `agent`, or naming a `runner`; and the reverse for `transport = "runner"` +- an empty `model`, or one set on `transport = "runner"` (see + [Model](#model)) A section on the wrong kind is an **error, not an ignored key**. This is the same rule [agents.md](agents.md) applies to a bundle carrying both roster forms, @@ -127,16 +152,26 @@ transport = "runner" # reach one that dialed in runner = "stevens_laptop" ``` -**A remote runner is a transport, not a third kind.** -`src/runner/dispatch.rs::RunnerDispatch` already implements the same `AcpAgent` -port the local subprocess does, so the only thing that differs is how bytes -reach the agent. Modelling it as a third kind would add a resolution path that -resolves to the same place. +**A remote runner is a transport, not a third kind.** `transport = "local"` and +`transport = "runner"` resolve to the same `AcpAgent` port +(`crate::ports::acp::AcpAgent`); only how bytes reach the agent differs. +Modelling the runner as a third kind would add a resolution path that resolves +to the same place. The transports differ in where they live, which is why `AcpAgent` is a **port** rather than an ACP client in the host crate: a subprocess over stdio belongs to the desktop shell, a WebSocket to the runner lane. The same inversion the -storage ports use. +storage ports use — and, concretely, why the port itself lives at +`crate::ports::acp`, ungated, rather than under `crate::harness` (behind +`openhuman`): the desktop shell that supplies the `local` implementation does +not enable that feature. See that module's own docs for the full reasoning. + +`local` has a real implementation as of issue #1245 — `LocalAcpAgent` +(`src-tauri/src/acp/local_agent.rs`), wired through `AppState::with_acp_agents` +and `desktop::register`. `runner` does not yet: `src/runner/dispatch.rs` +declares `RunnerDispatch`, but it does not implement `AcpAgent`, and nothing +wires it into `lanes::build`. A `runner`-transport harness resolves +`unavailable` on every build today, `local` included. ### Readiness @@ -187,14 +222,19 @@ All three methods route. A method forwarding to a fixed engine would send ### A harness with no engine fails the turn -A harness can be declared, valid, and still have no engine. Today that is every -`acp` harness on a server build: the transports live in the desktop shell (a -stdio subprocess) and the runner lane (a socket), and neither is wired into the -server. Those turns fail, naming the harness and the fix. +A harness can be declared, valid, and still have no engine. That is every `acp` +harness on a server build (no transport is wired there at all), every +`runner`-transport harness on any build (its socket transport isn't wired +yet), and a `local`-transport harness on a desktop build that was not given an +`AcpAgentFactory` (`AppState::with_acp_agents` — every embedder but the +packaged desktop app). Those turns fail, naming the harness and the fix. They MUST NOT fall back to another harness's engine. That is the worst outcome available: the turn would succeed, on a model and a credential nobody chose, and -the only evidence would be a billing line. +the only evidence would be a billing line. This also covers the agent itself +failing to start (not installed, not signed in, or a spawn error) — that +surfaces as the same kind of failure, naming the harness and the reason, not a +silent fallback either. --- @@ -204,7 +244,12 @@ the only evidence would be a billing line. cognition seam *within* the built-in harness. - **Tools, policy, budgets, desks.** All company- or agent-scoped, and unchanged by which engine runs the turn. An ACP agent is still subject to the company's - approval policy. + approval policy — **not yet true for `local`'s own permission prompts** + (`session/request_permission`): `LocalAcpAgent` fails closed on every one it + was not explicitly configured to allow, rather than routing it through + `ApprovalRequestQueue`. Safe (a refusal is a visible, actionable failure; a + silent auto-approval would not be), but a known gap, not the intended + end state. - **Which model an agent's `tier` means.** A tier names a workload and is resolved against whatever provider its harness turns out to use, so an agent keeps its tier when it moves between harnesses. See @@ -216,12 +261,16 @@ the only evidence would be a billing line. | concern | where | |---|---| -| manifest types, kind/transport vocabularies | `src/company/types.rs` | +| manifest types, kind/transport/model vocabulary | `src/company/types.rs` | | validation, `effective_harnesses`, `harness_for` | `src/company/manifest.rs` | | per-agent dispatch | `src/harness/router.rs` | -| building the lanes at boot | `src/harness/lanes.rs` | +| building the lanes at boot, resolving `acp` engines | `src/harness/lanes.rs` | | the built-in engine | `src/harness/built_in/` | -| the ACP `RunTurn` and its port | `src/harness/acp/run_turn.rs` | -| local transport: discovery, spawn, codec | `src-tauri/src/acp/` | -| runner transport | `src/runner/dispatch.rs` | +| the `AcpAgent`/`AcpAgentFactory` ports (ungated) | `src/ports/acp.rs` | +| the ACP `RunTurn` (folds a port `AcpTurn` into `TurnStep`) | `src/harness/acp/run_turn.rs` | +| wiring an `AcpAgentFactory` onto a host | `AppState::with_acp_agents` (`src/app/types.rs`), consumed by `desktop::register` | +| local transport: discovery, spawn, codec | `src-tauri/src/acp/` (`client.rs`, `discovery.rs`, `confine.rs`) | +| the `local` `AcpAgentFactory` implementation | `src-tauri/src/acp/local_agent.rs` (`LocalAcpAgent`/`LocalAcpAgentFactory`) | +| the desktop's own wiring | `src-tauri/src/embedded.rs` | +| runner transport (declared, not yet an engine) | `src/runner/dispatch.rs` | | per-harness roster narrowing | `HarnessDeps::serves` | diff --git a/src-tauri/src/acp/client.rs b/src-tauri/src/acp/client.rs index 46ddf268b..21eab4731 100644 --- a/src-tauri/src/acp/client.rs +++ b/src-tauri/src/acp/client.rs @@ -160,15 +160,22 @@ pub type UpdateSink = Arc; impl AcpClient { /// Spawns `command` and starts reading it. + /// + /// `env` is added on top of this process's own inherited environment — + /// not a replacement for it — so a harness that also needs `PATH`, `HOME`, + /// etc. keeps them. Callers that need no extra vars (every one before + /// issue #1245) pass `&[]`. pub async fn spawn( command: &str, args: &[&str], cwd: &Path, + env: &[(&str, &str)], handler: Arc, updates: UpdateSink, ) -> Result { let mut child = Command::new(command) .args(args) + .envs(env.iter().copied()) .current_dir(cwd) .stdin(Stdio::piped()) .stdout(Stdio::piped()) diff --git a/src-tauri/src/acp/discovery.rs b/src-tauri/src/acp/discovery.rs index 098e8e91e..60961f3dc 100644 --- a/src-tauri/src/acp/discovery.rs +++ b/src-tauri/src/acp/discovery.rs @@ -75,7 +75,13 @@ pub const HARNESSES: &[Harness] = &[ Harness { id: "claude", label: "Claude Code", - command: "claude-code-acp", + // Confirmed live (issue #1245): `npm install -g + // @agentclientprotocol/claude-agent-acp` installs a binary named + // `claude-agent-acp`, not `claude-code-acp` (the package's former + // name, before it moved under the `@agentclientprotocol` scope). A + // stale `claude-code-acp` here silently fails every "not found" probe + // and every spawn on a current install. + command: "claude-agent-acp", args: &[], credential: Some(".claude/.credentials.json"), }, @@ -251,7 +257,7 @@ mod test { // THE distinction this module exists for. Both are "unavailable", and // the fixes are completely different — install it, versus sign in — so // collapsing them tells the operator to do the wrong thing. - let probe = Fake::new().with_installed("claude-code-acp"); + let probe = Fake::new().with_installed("claude-agent-acp"); assert_eq!(readiness_of(&probe, "claude"), Readiness::NotSignedIn); assert_ne!(readiness_of(&probe, "claude"), Readiness::NotInstalled); } @@ -259,7 +265,7 @@ mod test { #[test] fn a_signed_in_harness_is_ready() { let probe = Fake::new() - .with_installed("claude-code-acp") + .with_installed("claude-agent-acp") .with_file("/home/ada/.claude/.credentials.json"); assert_eq!(readiness_of(&probe, "claude"), Readiness::Ready); assert!(readiness_of(&probe, "claude").is_ready()); @@ -269,7 +275,7 @@ mod test { fn each_harness_is_probed_at_its_own_paths() { // One harness being signed in must not make another look signed in. let probe = Fake::new() - .with_installed("claude-code-acp") + .with_installed("claude-agent-acp") .with_installed("codex-acp") .with_file("/home/ada/.claude/.credentials.json"); assert_eq!(readiness_of(&probe, "claude"), Readiness::Ready); @@ -280,7 +286,9 @@ mod test { fn no_home_directory_reads_as_signed_out_rather_than_ready() { // The safe direction of the two wrong answers: claiming ready would // fail at first use, far from the cause. - let probe = Fake::new().with_installed("claude-code-acp").without_home(); + let probe = Fake::new() + .with_installed("claude-agent-acp") + .without_home(); assert_eq!(readiness_of(&probe, "claude"), Readiness::NotSignedIn); } diff --git a/src-tauri/src/acp/local_agent.rs b/src-tauri/src/acp/local_agent.rs new file mode 100644 index 000000000..120b2bf87 --- /dev/null +++ b/src-tauri/src/acp/local_agent.rs @@ -0,0 +1,320 @@ +//! `LocalAcpAgent`: the `transport = "local"` implementation of the host +//! crate's [`AcpAgent`] port (issue #1245) — a real coding CLI, spawned once +//! per declared local-acp harness and driven over stdio through the existing +//! [`AcpClient`]. +//! +//! ## One process, many sessions +//! +//! A harness can serve more than one teammate, but [`AcpClient::spawn`] opens +//! one subprocess with one global update sink — ACP's `session/update` +//! notifications are not routed per caller, only tagged with the `sessionId` +//! they belong to. So this buffers every notification by `sessionId` as it +//! arrives, and a `prompt` call drains only its own session's buffer after +//! `session/prompt` returns rather than reading whatever the sink last saw. +//! +//! ## Permission requests: fails closed (deliberate, and a known gap) +//! +//! `docs/spec/runtime/harnesses.md` says an ACP agent "is still subject to +//! the company's approval policy" — this does not yet route ACP permission +//! requests through that policy gate; it refuses every one it did not +//! explicitly configure to allow, via the same [`ConfinedFiles`] the fixture +//! tests already use. That is the safe direction to be wrong in: a refused +//! edit is a visible failure the operator can act on, where a silently +//! auto-approved one would not be. Wiring ACP's `session/request_permission` +//! into `ApprovalRequestQueue` is real follow-up work, not done here. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex as StdMutex}; + +use async_trait::async_trait; +use opencompany::Result; +use opencompany::error::OpenCompanyError; +use opencompany::ports::acp::{AcpAgent, AcpAgentFactory, AcpTurn, AcpUpdate}; +use opencompany::ports::types::CompanyId; +use serde_json::Value; +use tokio::sync::Mutex as AsyncMutex; + +use crate::acp::client::{AcpClient, ClientHandler, ConfinedFiles}; +use crate::acp::confine::Confinement; +use crate::acp::discovery::HARNESSES; + +/// Per-CLI startup model env var, confirmed live against the real adapter +/// (issue #1245's live smoke test) — not guessed. `None` means this build has +/// no known lever for that CLI: `model` is still accepted on the manifest, +/// but nothing is injected, rather than silently spawning a process that +/// ignores the setting. +fn model_env_var(agent: &str) -> Option<&'static str> { + match agent { + "claude" => Some("ANTHROPIC_MODEL"), + "goose" => Some("GOOSE_MODEL"), + // codex: no confirmed startup-model env var. Buzz (block/buzz), the + // one other project this design is modeled on, has none either. + _ => None, + } +} + +/// One spawned local-transport ACP harness, serving every teammate bound to +/// it. +pub struct LocalAcpAgent { + command: &'static str, + args: Vec, + env: Vec<(String, String)>, + /// The company's agent-workspace root (`HarnessDeps::workspace_root`). + /// Each session roots at `workspace_root///workspace`, + /// mirroring `harness::built_in::build::agent_workspace` exactly, so an + /// ACP-run teammate's files land in the same conventional place a + /// `built_in`-run one's would. + workspace_root: PathBuf, + client: AsyncMutex>>, + /// `session_key` (`"{company}::{agent_id}"`) → ACP `sessionId`. + sessions: AsyncMutex>, + /// `session/update` notifications, demultiplexed by ACP `sessionId` — + /// see the module docs for why this exists at all. + pending_updates: Arc>>>, +} + +impl LocalAcpAgent { + /// `agent` is one of `ACP_AGENTS` (the manifest already validated this). + /// `model`, when set, is forwarded via that agent's own startup lever + /// when this build knows one. + pub fn new(agent: &str, model: Option<&str>, workspace_root: PathBuf) -> Result { + let def = HARNESSES.iter().find(|h| h.id == agent).ok_or_else(|| { + OpenCompanyError::Config(format!("no local ACP harness definition for `{agent}`")) + })?; + + let mut env = Vec::new(); + if let (Some(model), Some(var)) = (model, model_env_var(agent)) { + env.push((var.to_string(), model.to_string())); + } + + Ok(Self { + command: def.command, + args: def.args.iter().map(|a| a.to_string()).collect(), + env, + workspace_root, + client: AsyncMutex::new(None), + sessions: AsyncMutex::new(HashMap::new()), + pending_updates: Arc::new(StdMutex::new(HashMap::new())), + }) + } + + /// The spawned client, spawning it on first call. + async fn client(&self) -> Result> { + let mut guard = self.client.lock().await; + if let Some(client) = guard.as_ref() { + return Ok(client.clone()); + } + + std::fs::create_dir_all(&self.workspace_root).map_err(|error| { + OpenCompanyError::Config(format!( + "could not create ACP workspace root {}: {error}", + self.workspace_root.display() + )) + })?; + let confinement = Confinement::new(&self.workspace_root) + .map_err(|error| OpenCompanyError::Config(format!("acp workspace: {error}")))?; + // V1 fails closed — see the module docs. + let handler: Arc = Arc::new(ConfinedFiles::new(confinement, None)); + + let pending = Arc::clone(&self.pending_updates); + let sink: crate::acp::client::UpdateSink = Arc::new(move |update: Value| { + let session_id = update["sessionId"].as_str().unwrap_or_default().to_string(); + pending + .lock() + .unwrap() + .entry(session_id) + .or_default() + .push(update); + }); + + let args: Vec<&str> = self.args.iter().map(String::as_str).collect(); + let env: Vec<(&str, &str)> = self + .env + .iter() + .map(|(k, v)| (k.as_str(), v.as_str())) + .collect(); + let client = AcpClient::spawn( + self.command, + &args, + &self.workspace_root, + &env, + handler, + sink, + ) + .await + .map_err(|error| { + OpenCompanyError::Config(format!("could not start `{}`: {error}", self.command)) + })?; + client + .initialize() + .await + .map_err(|error| OpenCompanyError::Config(format!("acp initialize: {error}")))?; + + let client = Arc::new(client); + *guard = Some(client.clone()); + Ok(client) + } + + /// The per-(company, agent) session directory, created if it does not + /// exist yet — mirrors `harness::built_in::build::agent_workspace`. + fn session_root(&self, company: &CompanyId, agent_id: &str) -> Result { + let dir = self + .workspace_root + .join(company.as_ref()) + .join(agent_id) + .join("workspace"); + std::fs::create_dir_all(&dir).map_err(|error| { + OpenCompanyError::Config(format!( + "could not create ACP session workspace {}: {error}", + dir.display() + )) + })?; + Ok(dir) + } + + /// This session's cached ACP `sessionId`, opening one if none exists yet. + async fn session_for( + &self, + client: &AcpClient, + session_key: &str, + root: &Path, + ) -> Result { + let mut sessions = self.sessions.lock().await; + if let Some(id) = sessions.get(session_key) { + return Ok(id.clone()); + } + let id = client + .new_session(root) + .await + .map_err(|error| OpenCompanyError::Config(format!("acp session/new: {error}")))?; + sessions.insert(session_key.to_string(), id.clone()); + Ok(id) + } + + /// `session_key` is `"{company}::{agent_id}"` — recovers `agent_id` by + /// stripping the company prefix, since `AcpAgent::prompt` does not carry + /// it separately. Agent ids are `snake_case` (manifest-validated) and + /// cannot themselves contain `::`, so this split is unambiguous. + fn agent_id_of<'a>(company: &CompanyId, session_key: &'a str) -> &'a str { + session_key + .strip_prefix(company.as_ref()) + .and_then(|rest| rest.strip_prefix("::")) + .unwrap_or(session_key) + } +} + +/// Translates one raw `session/update` notification into this crate's +/// [`AcpUpdate`], or `None` for a kind that is dropped rather than +/// approximated (`plan`, `available_commands_update`, …) — see +/// `harness::acp::run_turn`'s own module docs for the mapping table this +/// mirrors. +fn parse_update(raw: &Value) -> Option { + let update = raw.get("update")?; + match update.get("sessionUpdate")?.as_str()? { + "agent_message_chunk" => Some(AcpUpdate::MessageChunk( + update["content"]["text"].as_str()?.to_string(), + )), + "agent_thought_chunk" => Some(AcpUpdate::ThoughtChunk), + "tool_call" => Some(AcpUpdate::ToolCall { + id: update["toolCallId"].as_str()?.to_string(), + title: update["title"].as_str().unwrap_or_default().to_string(), + }), + "tool_call_update" => Some(AcpUpdate::ToolCallUpdate { + id: update["toolCallId"].as_str()?.to_string(), + status: update["status"].as_str().unwrap_or_default().to_string(), + result: update + .get("content") + .and_then(|c| c.as_array()) + .map(|blocks| { + blocks + .iter() + .filter_map(|b| b["text"].as_str()) + .collect::>() + .join("") + }), + }), + _ => None, + } +} + +#[async_trait] +impl AcpAgent for LocalAcpAgent { + async fn prompt( + &self, + company: &CompanyId, + session_key: &str, + message: &str, + ) -> Result { + let client = self.client().await?; + let agent_id = Self::agent_id_of(company, session_key); + let root = self.session_root(company, agent_id)?; + let session_id = self.session_for(&client, session_key, &root).await?; + + // Clear any stale buffer before the turn starts, so the drain below + // sees exactly this turn's updates and nothing left over from one + // that timed out or was cancelled without being read. + self.pending_updates.lock().unwrap().remove(&session_id); + + let stop_reason = client + .prompt(&session_id, message) + .await + .map_err(|error| OpenCompanyError::Config(format!("acp prompt: {error}")))?; + + let raw = self + .pending_updates + .lock() + .unwrap() + .remove(&session_id) + .unwrap_or_default(); + let updates = raw.iter().filter_map(parse_update).collect(); + Ok(AcpTurn { + updates, + stop_reason, + }) + } + + async fn cancel(&self, company: &CompanyId, session_key: &str) -> Result<()> { + let session_id = { + let sessions = self.sessions.lock().await; + sessions.get(session_key).cloned() + }; + let Some(session_id) = session_id else { + // No session ever opened for this (company, agent) — nothing to + // cancel, and asking a client that may not exist yet would spawn + // one just to tell it to stop. + return Ok(()); + }; + let client = { self.client.lock().await.clone() }; + let Some(client) = client else { + return Ok(()); + }; + let _ = company; // carried for symmetry with `prompt`; not needed here + client + .cancel(&session_id) + .await + .map_err(|error| OpenCompanyError::Config(format!("acp cancel: {error}"))) + } +} + +/// Builds a fresh [`LocalAcpAgent`] per call — no caching, matching +/// `harness::lanes::built_in_lane`'s own precedent of building a fresh pool +/// on every `RuntimeBuilder::build`. A rebuild is rare (a manifest or +/// inference-settings change), and the old agent's subprocess is killed on +/// drop (`AcpClient::kill_on_drop`), so nothing leaks. +pub struct LocalAcpAgentFactory; + +impl AcpAgentFactory for LocalAcpAgentFactory { + fn build( + &self, + agent: &str, + model: Option<&str>, + workspace_root: &Path, + ) -> Result> { + Ok(Arc::new(LocalAcpAgent::new( + agent, + model, + workspace_root.to_path_buf(), + )?)) + } +} diff --git a/src-tauri/src/acp/mod.rs b/src-tauri/src/acp/mod.rs index 036e0a05b..169958bb7 100644 --- a/src-tauri/src/acp/mod.rs +++ b/src-tauri/src/acp/mod.rs @@ -13,10 +13,12 @@ pub mod client; pub mod codec; pub mod confine; pub mod discovery; +pub mod local_agent; pub mod worktree; pub use client::{AcpClient, AcpError, ClientHandler, ConfinedFiles}; pub use codec::{Message, RequestId}; pub use confine::{ConfineError, Confinement}; pub use discovery::{Harness, HarnessStatus, Readiness, SystemProbe, survey}; +pub use local_agent::{LocalAcpAgent, LocalAcpAgentFactory}; pub use worktree::{Isolation, TaskWorkspace, WorktreeError}; diff --git a/src-tauri/src/embedded.rs b/src-tauri/src/embedded.rs index 72bb53bba..d75aa94b5 100644 --- a/src-tauri/src/embedded.rs +++ b/src-tauri/src/embedded.rs @@ -154,7 +154,12 @@ pub async fn start_with( admin_email: Some(opencompany::desktop::DESKTOP_OPERATOR_EMAIL.to_string()), ..AppConfig::default() }; - let state = AppState::new(config).with_home(instance.home().to_path_buf()); + let state = AppState::new(config) + .with_home(instance.home().to_path_buf()) + // Issue #1245: the desktop is the one place with an + // `AcpAgentFactory` implementation to give — a `local` acp harness + // only has an engine because this line exists. + .with_acp_agents(std::sync::Arc::new(crate::acp::LocalAcpAgentFactory)); // Read before `state` moves into `bind`. Minting here rather than on the // first `/spec` also means the console can be told who this host is without // waiting to contact it — which is the whole point, since the address it diff --git a/src-tauri/tests/acp_client.rs b/src-tauri/tests/acp_client.rs index 3224ca4ce..bcc95e72c 100644 --- a/src-tauri/tests/acp_client.rs +++ b/src-tauri/tests/acp_client.rs @@ -54,6 +54,7 @@ async fn connect(root: &Path, handler: Arc) -> (AcpClient, Up "python3", &[fixture().to_str().unwrap()], root, + &[], handler, updates.sink(), ) diff --git a/src-tauri/tests/acp_live_smoke.rs b/src-tauri/tests/acp_live_smoke.rs new file mode 100644 index 000000000..bc58a2075 --- /dev/null +++ b/src-tauri/tests/acp_live_smoke.rs @@ -0,0 +1,246 @@ +//! Live smoke test against a real, installed `claude-agent-acp` — not the +//! scripted fixture `acp_client.rs` drives. +//! +//! Requires `claude-agent-acp` on `PATH` +//! (`npm install -g @agentclientprotocol/claude-agent-acp`) and an +//! authenticated `claude` CLI (`claude auth status`). Costs real API / +//! subscription usage on every run, so this is `#[ignore]`d and never +//! selected by CI — run explicitly: +//! +//! ```text +//! cargo test -p opencompany-desktop --test acp_live_smoke -- --ignored --nocapture +//! ``` +//! +//! Exists to validate, against the real adapter rather than the fixture, the +//! two assumptions issue #1245's harness-level `model` field depends on: +//! that `session/new` actually advertises a model-category config option or +//! the unstable `models` block, and that an env var set on the spawned +//! process actually steers which model that option reports as current. + +use std::path::Path; +use std::sync::{Arc, Mutex}; + +use opencompany_desktop_lib::acp::client::{AcpClient, ClientHandler, ConfinedFiles}; +use opencompany_desktop_lib::acp::confine::Confinement; +use serde_json::Value; + +fn handler(root: &Path) -> Arc { + Arc::new(ConfinedFiles::new( + Confinement::new(root).unwrap(), + Some("yes".to_string()), + )) +} + +#[derive(Clone, Default)] +struct Updates(Arc>>); + +impl Updates { + fn sink(&self) -> Arc { + let inner = Arc::clone(&self.0); + Arc::new(move |value| inner.lock().unwrap().push(value)) + } + fn said(&self) -> String { + self.0 + .lock() + .unwrap() + .iter() + .filter(|u| u["update"]["sessionUpdate"] == "agent_message_chunk") + .filter_map(|u| u["update"]["content"]["text"].as_str()) + .collect::>() + .join("") + } +} + +#[tokio::test] +#[ignore = "spawns a real, authenticated claude-agent-acp and costs real usage"] +async fn a_real_claude_agent_acp_answers_a_prompt() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + let updates = Updates::default(); + + let client = AcpClient::spawn( + "claude-agent-acp", + &[], + &root, + &[], + handler(&root), + updates.sink(), + ) + .await + .expect( + "claude-agent-acp must be on PATH: npm install -g @agentclientprotocol/claude-agent-acp", + ); + client.initialize().await.expect("initialize"); + let session = client.new_session(&root).await.expect("session/new"); + + let stop_reason = client + .prompt( + &session, + "Reply with exactly the single word PONG and nothing else.", + ) + .await + .expect("prompt"); + assert_eq!(stop_reason, "end_turn", "updates were: {:?}", updates.0); + + let said = updates.said(); + assert!(said.contains("PONG"), "got: {said:?}"); +} + +/// Bypasses `new_session`'s narrow `sessionId`-only parsing to see the full +/// raw `session/new` response, so this can inspect `configOptions`/`models` +/// without a helper this crate doesn't have yet (that helper is #1245's job; +/// this test is what justifies building it at all). +#[tokio::test] +#[ignore = "spawns a real, authenticated claude-agent-acp and costs real usage"] +async fn session_new_advertises_a_model_config_option() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + let updates = Updates::default(); + + let client = AcpClient::spawn( + "claude-agent-acp", + &[], + &root, + &[], + handler(&root), + updates.sink(), + ) + .await + .expect("claude-agent-acp must be on PATH"); + client.initialize().await.expect("initialize"); + + let raw = client + .call( + "session/new", + serde_json::json!({ "cwd": root.display().to_string(), "mcpServers": [] }), + ) + .await + .expect("session/new"); + + let model_option = raw["configOptions"].as_array().and_then(|opts| { + opts.iter() + .find(|o| o.get("category").and_then(|c| c.as_str()) == Some("model")) + }); + + assert!( + model_option.is_some() || raw.get("models").is_some(), + "expected a `configOptions` entry with category \"model\" or an unstable \ + `models` block in session/new's response, got: {raw:#}" + ); +} + +/// The mechanism issue #1245's `LocalAcpAgent` will actually use: an env var +/// set on the spawned process, not a live ACP config-option switch. Runs the +/// adapter twice, under two different `ANTHROPIC_MODEL` values, and confirms +/// the reported "current" model differs — proof the env var is actually +/// consulted at startup, not silently ignored. +#[tokio::test] +#[ignore = "spawns a real, authenticated claude-agent-acp twice and costs real usage"] +async fn anthropic_model_env_var_steers_the_startup_model() { + async fn current_model_id(model_env: &str) -> Option { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + let updates = Updates::default(); + let client = AcpClient::spawn( + "claude-agent-acp", + &[], + &root, + &[("ANTHROPIC_MODEL", model_env)], + handler(&root), + updates.sink(), + ) + .await + .expect("claude-agent-acp must be on PATH"); + client.initialize().await.expect("initialize"); + let raw = client + .call( + "session/new", + serde_json::json!({ "cwd": root.display().to_string(), "mcpServers": [] }), + ) + .await + .expect("session/new"); + + // Stable path: the configOptions entry whose category is "model" names + // its current value's `currentValue` (not `configId`/`id`'s own spec + // name of "value" — claude-agent-acp's real wire shape, confirmed + // live). Unstable path: `models.currentModelId`. + raw["configOptions"] + .as_array() + .and_then(|opts| { + opts.iter() + .find(|o| o.get("category").and_then(|c| c.as_str()) == Some("model")) + }) + .and_then(|opt| opt.get("currentValue").and_then(|v| v.as_str())) + .map(str::to_string) + .or_else(|| raw["models"]["currentModelId"].as_str().map(str::to_string)) + } + + let haiku = current_model_id("claude-haiku-4-5").await; + let sonnet = current_model_id("claude-sonnet-4-5").await; + + assert!( + haiku.is_some() && sonnet.is_some(), + "session/new must report a current model id under ANTHROPIC_MODEL: \ + haiku={haiku:?} sonnet={sonnet:?}" + ); + assert_ne!( + haiku, sonnet, + "ANTHROPIC_MODEL must actually steer the reported current model, not be ignored" + ); +} + +/// The full `LocalAcpAgent` path, through the `AcpAgent` trait rather than +/// the raw `AcpClient` the tests above drive directly — the same seam +/// `harness::lanes::build` calls in production. Proves the whole chain: model +/// env-var injection, lazy session creation, and raw-JSON-to-`AcpUpdate` +/// parsing all work against the real adapter, not just each piece in +/// isolation. +#[tokio::test] +#[ignore = "spawns a real, authenticated claude-agent-acp and costs real usage"] +async fn local_acp_agent_answers_a_prompt_through_the_acp_agent_trait() { + use opencompany::ports::acp::AcpAgentFactory; + use opencompany::ports::types::CompanyId; + use opencompany_desktop_lib::acp::LocalAcpAgentFactory; + + let dir = tempfile::tempdir().unwrap(); + let workspace_root = dir.path().canonicalize().unwrap(); + + let agent = LocalAcpAgentFactory + .build("claude", None, &workspace_root) + .expect("claude-agent-acp must be on PATH"); + + let company = CompanyId::new("acme-live-smoke"); + let turn = agent + .prompt( + &company, + &format!("{}::researcher", company.as_ref()), + "Reply with exactly the single word PONG and nothing else.", + ) + .await + .expect("prompt"); + + assert_eq!( + turn.stop_reason, "end_turn", + "updates were: {:?}", + turn.updates + ); + let said: String = turn + .updates + .iter() + .filter_map(|u| match u { + opencompany::ports::acp::AcpUpdate::MessageChunk(text) => Some(text.as_str()), + _ => None, + }) + .collect(); + assert!(said.contains("PONG"), "got: {said:?}"); + + // The per-agent workspace directory was created, mirroring + // `harness::built_in::build::agent_workspace`'s layout. + assert!( + workspace_root + .join("acme-live-smoke") + .join("researcher") + .join("workspace") + .is_dir() + ); +} diff --git a/src/app/types.rs b/src/app/types.rs index f3c0e8bbb..c00aa1310 100644 --- a/src/app/types.rs +++ b/src/app/types.rs @@ -494,6 +494,14 @@ pub struct AppState { /// `restartRequired` and the console still says so, which is the honest /// answer when a rebuild is genuinely unavailable. rebuilder: Option>, + /// Builds the engine for a `transport = "local"` `acp` harness (issue + /// #1245). `None` — every test host, and any embedder that does not wire + /// one — leaves every such harness `unavailable`. Only the desktop shell + /// has an implementation to give this; it lives at + /// [`crate::ports::acp::AcpAgentFactory`], ungated, for the same reason + /// [`rebuilder`](Self::rebuilder) above is: the desktop supplies it, this + /// crate only defines the seam. + acp_agents: Option>, /// The boot-only builder inputs recorded per company at registration, so a /// rebuild configures the successor exactly as boot configured its /// predecessor. See [`BootInputs`](crate::runtime::BootInputs) for why @@ -543,6 +551,7 @@ impl AppState { #[cfg(feature = "mcp")] oauth_pending: Arc::new(std::sync::Mutex::new(HashMap::new())), rebuilder: None, + acp_agents: None, boot_inputs: Arc::new(RwLock::new(HashMap::new())), } } @@ -553,6 +562,17 @@ impl AppState { self } + /// Wires this host's local-transport ACP agent factory (issue #1245). + pub fn with_acp_agents(mut self, factory: Arc) -> Self { + self.acp_agents = Some(factory); + self + } + + /// This host's local-transport ACP agent factory, when one is wired. + pub fn acp_agents(&self) -> Option> { + self.acp_agents.clone() + } + /// This host's in-place runtime rebuilder, when one is wired. pub fn rebuilder(&self) -> Option> { self.rebuilder.clone() diff --git a/src/company/manifest.rs b/src/company/manifest.rs index c9e1e0f85..7588bb3db 100644 --- a/src/company/manifest.rs +++ b/src/company/manifest.rs @@ -817,10 +817,24 @@ impl CompanyManifest { A runner advertises the harnesses it can drive — this host does not choose one for it." )); } + if acp.model.is_some() { + problems.push(format!( + "`[[harness]]` `{id}` uses `transport = \"runner\"` but names a `model`. \ + Model overrides aren't supported for a runner yet — the runner wire \ + protocol doesn't carry them." + )); + } } _ => unreachable!("transport was checked against ACP_TRANSPORTS above"), } + if acp.model.as_deref().is_some_and(|m| m.trim().is_empty()) { + problems.push(format!( + "`[[harness]]` `{id}`'s `[harness.acp].model` is set but empty. Drop the key \ + to use the agent's own default, rather than naming an empty one." + )); + } + problems } @@ -2252,6 +2266,43 @@ provider = "openrouter" } } + /// Issue #1245: `model` is a hint forwarded to the agent's own startup + /// lever, not a credential — so unlike `[harness.inference]` it is + /// perfectly valid on a `local` acp harness. It is rejected on `runner` + /// (no wire protocol yet) and when set to an empty string (nothing to + /// forward, and silently accepting it invites "my model setting does + /// nothing"). + #[test] + fn model_is_valid_on_local_rejected_on_runner_and_must_not_be_empty() { + let cases: &[(&str, Option<&str>)] = &[ + ( + "transport = \"local\"\nagent = \"claude\"\nmodel = \"claude-opus-4-5\"\n", + None, + ), + ( + "transport = \"runner\"\nrunner = \"laptop\"\nmodel = \"claude-opus-4-5\"\n", + Some("but names a `model`"), + ), + ( + "transport = \"local\"\nagent = \"claude\"\nmodel = \" \"\n", + Some("is set but empty"), + ), + ]; + for (acp, expected) in cases { + let manifest = parse(&format!( + "{BASE}\n[[harness]]\nid = \"a\"\nkind = \"acp\"\ndefault = true\n\n[harness.acp]\n{acp}" + )); + let problems = harness_problems(&manifest); + match expected { + None => assert!(problems.is_empty(), "`{acp}` should be valid: {problems:?}"), + Some(msg) => assert!( + problems.iter().any(|p| p.contains(msg)), + "`{acp}` should report {msg:?}, got {problems:?}" + ), + } + } + } + #[test] fn an_acp_harness_with_no_acp_section_is_rejected() { let manifest = parse(&format!( diff --git a/src/company/types.rs b/src/company/types.rs index 1c83c9938..cb3dcc691 100644 --- a/src/company/types.rs +++ b/src/company/types.rs @@ -1170,6 +1170,17 @@ pub struct AcpHarness { /// Which registered runner holds this scope. `runner` transport only. #[serde(default)] pub runner: Option, + /// A model hint forwarded to the agent's own startup lever, when this + /// build knows one for `agent` (issue #1245). + /// + /// Not a credential — the ACP agent already holds its own, which is the + /// whole point of this harness kind — so this does not join + /// `[harness.inference]`'s prohibition on `acp` harnesses. `local` + /// transport only for now: the `runner` wire protocol does not carry it + /// yet, so validation rejects it there rather than accepting and silently + /// dropping it. + #[serde(default)] + pub model: Option, } /// `[inference]` — per-tenant Bring-Your-Own-Key inference routing (issue #56). diff --git a/src/desktop.rs b/src/desktop.rs index f55a8826a..620244bec 100644 --- a/src/desktop.rs +++ b/src/desktop.rs @@ -430,6 +430,15 @@ async fn register( if let Some(provenance) = provenance { builder = builder.with_template_provenance(provenance); } + // Issue #1245: `with_acp_agents` only exists under `acp` — an + // `openhuman`-only build (or one with no `AcpAgentFactory` wired on + // `state`, e.g. every non-desktop embedder) leaves the builder's default + // `None`, so a `local` acp harness resolves `unavailable` exactly as it + // already does. + #[cfg(feature = "acp")] + if let Some(factory) = state.acp_agents() { + builder = builder.with_acp_agents(factory); + } let runtime = builder.build().await?; // The same refusal `serve` applies at boot and provisioning: a `none`-mode // company on a routable bind is an unauthenticated admin console. The diff --git a/src/harness/acp/run_turn.rs b/src/harness/acp/run_turn.rs index 08a186a19..805293a31 100644 --- a/src/harness/acp/run_turn.rs +++ b/src/harness/acp/run_turn.rs @@ -20,10 +20,14 @@ //! ## Why a port rather than an ACP client in here //! //! The transport differs per caller — a subprocess over stdio for the desktop, -//! a WebSocket for a runner — and neither belongs in the host crate. So this -//! defines [`AcpAgent`] as a port and folds whatever it reports; the desktop -//! shell supplies the stdio implementation, and the runner lane will supply the -//! socket one. The same inversion the storage ports use. +//! a WebSocket for a runner — and neither belongs in the host crate. The port +//! itself ([`AcpAgent`], [`AcpAgentFactory`], `AcpTurn`, `AcpUpdate`) lives at +//! [`crate::ports::acp`], ungated, because the desktop shell that supplies the +//! stdio implementation deliberately does not enable the `openhuman` feature +//! this module lives behind — see that module's own docs for why. What +//! belongs here is [`AcpRunTurn`]: the adapter that folds whatever an +//! `AcpAgent` reports into this crate's own [`TurnStep`] shape, a genuine +//! `openhuman` dependency the port itself has none of. //! //! ## The mapping, and where it is lossy //! @@ -50,65 +54,10 @@ use async_trait::async_trait; use crate::Result; use crate::error::OpenCompanyError; use crate::harness::TurnOutcome; +pub use crate::ports::acp::{AcpAgent, AcpAgentFactory, AcpTurn, AcpUpdate}; use crate::ports::types::{CompanyId, TurnStep, TurnStepKind, TurnStepStatus}; use crate::runtime::delegation::RunTurn; -/// One `session/update` payload, already parsed into what this layer needs. -/// -/// A narrow enum rather than raw JSON, so the wire-format knowledge stays in -/// the transport and the folding below stays testable without one. -#[derive(Clone, Debug, PartialEq)] -pub enum AcpUpdate { - /// Assistant text. Concatenated, in arrival order, into the reply. - MessageChunk(String), - /// Reasoning. Coalesced into a single step — the console shows "Thinking", - /// never the content, matching what the OpenHuman path surfaces. - ThoughtChunk, - /// A tool call started. - ToolCall { id: String, title: String }, - /// A tool call progressed or finished. - ToolCallUpdate { - id: String, - /// ACP's `pending` / `in_progress` / `completed` / `failed`. - status: String, - /// A short summary of what came back, already bounded by the transport. - result: Option, - }, -} - -/// What an ACP agent reports for one turn. -#[derive(Clone, Debug, Default)] -pub struct AcpTurn { - pub updates: Vec, - /// ACP's `stopReason`. - pub stop_reason: String, -} - -/// An ACP agent this host can run a turn on. -/// -/// Implemented by the desktop (a subprocess over stdio) and, later, by the -/// runner lane (a socket). Deliberately says nothing about transport. -#[async_trait] -pub trait AcpAgent: Send + Sync { - /// Runs one turn and returns everything it produced. - /// - /// `session_key` is stable for a (company, agent) pair so the agent can - /// keep a conversation rather than starting fresh each turn. - async fn prompt( - &self, - company: &CompanyId, - session_key: &str, - message: &str, - ) -> Result; - - /// Asks the agent to stop the turn in flight. - /// - /// Advisory, and the caller must treat it that way: ACP's `session/cancel` - /// is a notification, and a harness inside a long tool call notices only - /// when that call returns. - async fn cancel(&self, company: &CompanyId, session_key: &str) -> Result<()>; -} - /// [`RunTurn`] over an [`AcpAgent`]. pub struct AcpRunTurn { agent: Arc, diff --git a/src/harness/built_in/brain.rs b/src/harness/built_in/brain.rs index 8a9e07d27..6ecd3c2d8 100644 --- a/src/harness/built_in/brain.rs +++ b/src/harness/built_in/brain.rs @@ -9245,6 +9245,7 @@ agent = "claude" &brain.deps, secrets, None, + None, ); assert!( diff --git a/src/harness/lanes.rs b/src/harness/lanes.rs index 9f561f361..e5f01dc40 100644 --- a/src/harness/lanes.rs +++ b/src/harness/lanes.rs @@ -35,6 +35,15 @@ //! the default the same way as every other harness, in this one place, is what //! closes that gap for good instead of leaving a second opinion for a future //! caller to reintroduce. +//! +//! ## `local` acp harnesses, when a factory is wired (issue #1245) +//! +//! `transport = "local"` now has a real engine wherever the caller supplies an +//! [`AcpAgentFactory`](crate::harness::acp::run_turn::AcpAgentFactory) — the +//! desktop shell, which owns the only implementation this crate does not +//! provide itself. A server build, or a desktop build asked to run a `runner` +//! harness (its socket transport is still unwired), passes `None`/leaves it +//! `unavailable` exactly as before. use std::collections::HashSet; use std::sync::Arc; @@ -48,6 +57,16 @@ use crate::ports::SecretStore; use crate::ports::types::{CompanyId, CompanyRecord}; use crate::runtime::delegation::RunTurn; +/// The type `build`'s `acp_agents` parameter takes. Real under `acp` +/// (`crate::harness::acp::run_turn` — the `AcpAgent`/`AcpRunTurn` types — only +/// exists there); an uninhabited placeholder otherwise, so callers built +/// under plain `openhuman` (no `acp`) still compile and simply can never pass +/// `Some`. +#[cfg(feature = "acp")] +pub type AcpFactory<'a> = &'a dyn crate::harness::acp::run_turn::AcpAgentFactory; +#[cfg(not(feature = "acp"))] +pub type AcpFactory<'a> = &'a std::convert::Infallible; + /// Why a declared harness of `kind` has no engine on this host — the one /// message both the default-harness path and the named-harness loop use, so /// they cannot drift into saying different things about the same gap. @@ -60,6 +79,55 @@ fn unavailable_reason(kind: &str) -> String { } } +/// Resolves one `kind = "acp"` harness to an engine, or records why it has +/// none. Shared by the default-harness resolution and the named-harness loop +/// so the two cannot describe the same gap differently. +#[cfg(feature = "acp")] +fn resolve_acp_engine( + harness: &Harness, + acp_agents: Option>, + workspace_root: &std::path::Path, +) -> std::result::Result, String> { + // Validation guarantees `acp` is `Some` and `transport` is one of + // `ACP_TRANSPORTS` on every harness that reaches here — this crate's own + // `CompanyManifest::validate`, not a caller-supplied invariant. + let acp = harness + .acp + .as_ref() + .ok_or_else(|| unavailable_reason("acp"))?; + + if acp.transport != "local" { + // `runner` (a remote socket dispatch) has no engine on any build yet — + // a materially different, larger piece of work than the local + // subprocess case, and out of scope here. + return Err( + "it uses `transport = \"runner\"` and this build has no runner transport wired yet" + .to_string(), + ); + } + + let factory = acp_agents.ok_or_else(|| unavailable_reason("acp"))?; + let agent_id = acp.agent.as_deref().unwrap_or_default(); + factory + .build(agent_id, acp.model.as_deref(), workspace_root) + .map(|agent| { + Arc::new(crate::harness::acp::run_turn::AcpRunTurn::new(agent)) as Arc + }) + .map_err(|error| format!("`{agent_id}` could not be started: {error}")) +} + +/// The `openhuman`-without-`acp` build: unconditionally unavailable, exactly +/// as every `acp` harness was before issue #1245 — `acp_agents` can only ever +/// be `None` here (its type is uninhabited), so there is nothing to build. +#[cfg(not(feature = "acp"))] +fn resolve_acp_engine( + _harness: &Harness, + _acp_agents: Option>, + _workspace_root: &std::path::Path, +) -> std::result::Result, String> { + Err(unavailable_reason("acp")) +} + /// The engines a company's declared harnesses resolve to on this host. pub struct Lanes { /// Agents the **default** harness serves, when the company declares more @@ -116,6 +184,7 @@ pub fn build( base: &HarnessDeps, secrets: Arc, env_default: Option, + acp_agents: Option>, ) -> Lanes { let declared = record.manifest.effective_harnesses(); let default_harness = record.manifest.default_harness(); @@ -131,6 +200,13 @@ pub fn build( "built_in" => { Some(Arc::new(HarnessRunTurn::new(pool, Arc::new(base.clone()))) as Arc) } + "acp" => match resolve_acp_engine(&default_harness, acp_agents, &base.workspace_root) { + Ok(engine) => Some(engine), + Err(reason) => { + unavailable.push((default_harness_id.clone(), reason)); + None + } + }, kind => { unavailable.push((default_harness_id.clone(), unavailable_reason(kind))); None @@ -150,6 +226,10 @@ pub fn build( &default_harness_id, ), )), + "acp" => match resolve_acp_engine(harness, acp_agents, &base.workspace_root) { + Ok(engine) => lanes.push((harness.id.clone(), engine)), + Err(reason) => unavailable.push((harness.id.clone(), reason)), + }, kind => unavailable.push((harness.id.clone(), unavailable_reason(kind))), } } diff --git a/src/ports/acp.rs b/src/ports/acp.rs new file mode 100644 index 000000000..358a92a09 --- /dev/null +++ b/src/ports/acp.rs @@ -0,0 +1,124 @@ +//! The [`AcpAgent`]/[`AcpAgentFactory`] ports: running a company's turn on an +//! external agent over the Agent Client Protocol, instead of the embedded +//! OpenHuman harness (issue #1245). +//! +//! ## Why these live here, not under `harness` +//! +//! Everything under `crate::harness` is gated behind the `openhuman` feature +//! — the embedded engine and its dependency tree. The desktop shell, which is +//! the whole reason ACP exists (an operator's own coding CLI, no credential +//! from us at all), deliberately does **not** enable that feature: pulling in +//! the entire vendored OpenHuman runtime just to reach two trait definitions +//! would be exactly backwards for "the operator's own subscription, nothing +//! to configure." +//! +//! So the port — what an ACP agent *is*, and what builds one — lives here, +//! ungated, alongside every other cross-cutting port +//! ([`SecretStore`](crate::ports::SecretStore), +//! [`ContextStore`](crate::ports::ContextStore), …). `crate::harness::acp` +//! (openhuman-gated) re-exports these and adds +//! [`AcpRunTurn`](crate::harness::acp::run_turn::AcpRunTurn) — the +//! `RunTurn` adapter that folds an [`AcpTurn`] into the embedded engine's own +//! [`TurnStep`](crate::ports::types::TurnStep) shape — because *that* piece +//! genuinely needs the embedded engine's types. +//! +//! ## What this unlocks +//! +//! - **A desktop company with no key.** The embedded host runs a turn on the +//! operator's own `claude-agent-acp`, against their existing subscription. +//! - **Reverse dispatch.** A cloud host hands a task to a runner on someone's +//! machine; the runner is an ACP agent as far as this is concerned. +//! - **Any other harness.** Codex, goose, and anything else that speaks ACP. + +use std::sync::Arc; + +use async_trait::async_trait; + +use crate::Result; +use crate::ports::types::CompanyId; + +/// One `session/update` payload, already parsed into what this layer needs. +/// +/// A narrow enum rather than raw JSON, so the wire-format knowledge stays in +/// the transport and the folding downstream stays testable without one. +#[derive(Clone, Debug, PartialEq)] +pub enum AcpUpdate { + /// Assistant text. Concatenated, in arrival order, into the reply. + MessageChunk(String), + /// Reasoning. Coalesced into a single step — the console shows "Thinking", + /// never the content, matching what the OpenHuman path surfaces. + ThoughtChunk, + /// A tool call started. + ToolCall { id: String, title: String }, + /// A tool call progressed or finished. + ToolCallUpdate { + id: String, + /// ACP's `pending` / `in_progress` / `completed` / `failed`. + status: String, + /// A short summary of what came back, already bounded by the transport. + result: Option, + }, +} + +/// What an ACP agent reports for one turn. +#[derive(Clone, Debug, Default)] +pub struct AcpTurn { + pub updates: Vec, + /// ACP's `stopReason`. + pub stop_reason: String, +} + +/// An ACP agent this host can run a turn on. +/// +/// Implemented by the desktop (a subprocess over stdio) and, later, by the +/// runner lane (a socket). Deliberately says nothing about transport. +#[async_trait] +pub trait AcpAgent: Send + Sync { + /// Runs one turn and returns everything it produced. + /// + /// `session_key` is stable for a (company, agent) pair so the agent can + /// keep a conversation rather than starting fresh each turn. + async fn prompt( + &self, + company: &CompanyId, + session_key: &str, + message: &str, + ) -> Result; + + /// Asks the agent to stop the turn in flight. + /// + /// Advisory, and the caller must treat it that way: ACP's `session/cancel` + /// is a notification, and a harness inside a long tool call notices only + /// when that call returns. + async fn cancel(&self, company: &CompanyId, session_key: &str) -> Result<()>; +} + +/// Builds an [`AcpAgent`] for one declared `transport = "local"` harness. +/// +/// A port, exactly like [`AcpAgent`] itself: only the desktop shell can +/// actually spawn a subprocess, so this crate defines the seam and the +/// desktop supplies the implementation. `lanes::build` receives this as +/// `Option<&dyn AcpAgentFactory>` — `None` on a server build, which is why a +/// `local` acp harness there still resolves to `unavailable` rather than a +/// broken or panicking build attempt. +/// +/// Synchronous and infallible-to-call-lazily on purpose: building the value +/// (a struct holding the command/args/env to spawn) does no I/O — the actual +/// subprocess spawns lazily, on the agent's first `prompt` — so `lanes::build` +/// itself never blocks on process startup or a harness that is slow to boot. +pub trait AcpAgentFactory: Send + Sync { + /// `agent` is one of `ACP_AGENTS` (the manifest already validated this). + /// `model`, when set, is forwarded to that agent's own startup lever + /// where this build knows one — see the implementation's own docs for + /// which agents that currently covers. `workspace_root` is the same root + /// the embedded engine roots a company's agent workspaces under + /// (`HarnessDeps::workspace_root`) — the factory has no other way to + /// learn it, since it is built once and shared across every company the + /// host runs, not constructed fresh per company. + fn build( + &self, + agent: &str, + model: Option<&str>, + workspace_root: &std::path::Path, + ) -> Result>; +} diff --git a/src/ports/mod.rs b/src/ports/mod.rs index e30a082b9..fbe6b9e9d 100644 --- a/src/ports/mod.rs +++ b/src/ports/mod.rs @@ -8,6 +8,7 @@ mod ids; +pub mod acp; pub mod approvals; pub mod artifacts; pub mod brain; @@ -40,6 +41,7 @@ pub mod workflow_runner; pub mod workflow_verdict; pub mod workspace; +pub use acp::{AcpAgent, AcpAgentFactory, AcpTurn, AcpUpdate}; pub use approvals::ApprovalGate; pub use artifacts::{ ArtifactAuthor, ArtifactDiff, ArtifactKind, ArtifactRecord, ArtifactStore, ArtifactVersion, diff --git a/src/runtime/builder.rs b/src/runtime/builder.rs index 22dcdfa6b..3a64bb877 100644 --- a/src/runtime/builder.rs +++ b/src/runtime/builder.rs @@ -472,6 +472,16 @@ pub struct RuntimeBuilder { /// consumed when a company **explicitly** grants the `search` namespace. #[cfg(feature = "openhuman")] search_backend: Option, + /// Issue #1245: builds the engine for a `transport = "local"` `acp` + /// harness. `None` — the default — leaves every such harness + /// `unavailable`, exactly as before this existed; only the desktop shell + /// (the only implementation this crate does not itself provide) sets it. + /// + /// Gated on `acp` specifically, not `openhuman` — `AcpAgentFactory` lives + /// behind the narrower feature (`acp = ["openhuman"]`), so an + /// `openhuman`-only build (no `acp`) does not have the type to name here. + #[cfg(feature = "acp")] + acp_agents: Option>, /// Issue #290: the live state of the runtime this build is *replacing*. /// /// Present only on a rebuild. It supplies the per-instance pieces a second @@ -551,6 +561,8 @@ impl RuntimeBuilder { media_backend: None, #[cfg(feature = "openhuman")] search_backend: None, + #[cfg(feature = "acp")] + acp_agents: None, handover: None, } } @@ -967,6 +979,21 @@ impl RuntimeBuilder { self } + /// Issue #1245: sets the factory that builds the engine for a + /// `transport = "local"` `acp` harness. Only the desktop shell has an + /// implementation to give this — a server build leaves it unset, so + /// `lanes::build` records every such harness `unavailable` instead of + /// having anything to spawn a subprocess with. Feature-gated on `acp` + /// specifically; see the field's own doc for why. + #[cfg(feature = "acp")] + pub fn with_acp_agents( + mut self, + factory: Arc, + ) -> Self { + self.acp_agents = Some(factory); + self + } + /// Issue #109: sets the MANAGED media-generation backend (platform /// credential + URL, resolved from the environment via /// [`media_backend_from_env`](crate::harness::provider::media_backend_from_env)). @@ -2651,12 +2678,23 @@ impl RuntimeBuilder { // `serves`) could build the whole roster on the // default provider regardless of which agents it // actually serves. + // + // `self.acp_agents` only exists under `acp` + // (narrower than this whole block's `openhuman` + // gate) — an `openhuman`-only build has nothing to + // pass, so every `local` acp harness resolves to + // `unavailable` there, same as before issue #1245. + #[cfg(feature = "acp")] + let acp_agents = self.acp_agents.as_deref(); + #[cfg(not(feature = "acp"))] + let acp_agents = None; let lanes = crate::harness::lanes::build( &record, pool.clone(), &deps, secrets.clone(), env_default, + acp_agents, ); if !lanes.lanes.is_empty() || !lanes.unavailable.is_empty() { tracing::info!( From 3ba465c76ab22a1147ab7d1175cb34e8ab106284 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 20 Aug 2026 14:02:57 +0530 Subject: [PATCH 03/14] fix(harness): baseline teammate steps aside for a company-authored tie (#1196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the #1106 park-and-ask for the specific tie #1196 reports: a global baseline teammate (globals/agents/) plausibly fits alongside a role the company staffed itself. The company has already expressed a preference by staffing that role, so the baseline candidate is dropped and the card dispatches instead of parking. A tie between two baseline teammates, or between two company teammates, is untouched — #1106's park-and-ask stands. Carries Agent::global into TeammateBrief so the planner's roster prompt also distinguishes a baseline teammate ("— from the shared baseline"). Co-Authored-By: Claude --- src/harness/built_in/planning.rs | 51 +++++++++++ src/harness/built_in/planning/test.rs | 120 ++++++++++++++++++++++++++ 2 files changed, 171 insertions(+) diff --git a/src/harness/built_in/planning.rs b/src/harness/built_in/planning.rs index bc4b7f9a6..8946ca3ba 100644 --- a/src/harness/built_in/planning.rs +++ b/src/harness/built_in/planning.rs @@ -352,6 +352,7 @@ pub async fn run_planning_pass(runtime: Arc, task_id: String) { let prerequisites = verify_prerequisites(&runtime, &evidence, &draft.prerequisites).await; let candidates = resolve_assignee_candidates(&evidence, &draft.assignee_candidates); + let candidates = prefer_company_over_baseline(&evidence, candidates); // Issue #1106. One surviving candidate is a proposal and behaves exactly as // it did before this change. Two or more is an open question, and a question // is not something to answer by taking the first element — so nothing is @@ -692,6 +693,10 @@ struct TeammateBrief { description: Option, /// Effective tool grants — namespace names only, never a credential. grants: Vec, + /// Whether this teammate came from the global baseline rather than the + /// company's own roster (mirrors [`crate::company::types::Agent::global`]). + /// An overlay teammate is never global — it always has an author. + global: bool, } /// Everything the host gathered before the model was asked anything. @@ -839,6 +844,7 @@ async fn gather_evidence( role: a.role.clone(), description: a.description.clone(), grants: crate::runtime::builder::agent_effective_grants(&allow, &a.tools), + global: a.global, }) .collect(); teammates.extend( @@ -851,6 +857,7 @@ async fn gather_evidence( role: overlay.role.clone(), description: overlay.description.clone(), grants: crate::runtime::builder::agent_effective_grants(&allow, &overlay.tools), + global: false, }), ); @@ -1346,6 +1353,9 @@ fn evidence_prompt(e: &Evidence) -> String { if let Some(description) = &t.description { out.push_str(&format!(" — {description}")); } + if t.global { + out.push_str(" — from the shared baseline"); + } out.push('\n'); } for (desk, members) in &e.desks { @@ -1862,6 +1872,47 @@ fn resolve_assignee_candidates( out } +/// Issue #1196. Drops baseline candidates from a tie that also names a +/// company-authored teammate. +/// +/// `resolve_assignee_candidates` only validates names — it stays that way. +/// This runs as a separate pass over its output because the tie it resolves +/// is not about which name is real, it is about provenance: every company +/// carries the same four baseline teammates ([`crate::globals`]), and when one +/// of them ties against a role the company chose to staff itself, the company +/// has already expressed the answer by staffing that role. A tie between two +/// baseline teammates, or between two company teammates, carries no such +/// signal and is left untouched — issue #1106's park-and-ask stands for both. +/// +/// A candidate id that does not resolve to a manifest agent (a desk, or an +/// overlay teammate — [`OverlayAgent`](crate::ports::types::OverlayAgent) has +/// no `global` field, so it can never be one) counts as company-side: only a +/// manifest agent explicitly marked `global` is baseline. +fn prefer_company_over_baseline( + evidence: &Evidence, + candidates: Vec, +) -> Vec { + let is_baseline = |id: &str| { + evidence + .record + .manifest + .agents + .iter() + .find(|a| a.id == id) + .is_some_and(|a| a.global) + }; + let has_company_side = candidates.iter().any(|c| !is_baseline(&c.id)); + let has_baseline = candidates.iter().any(|c| is_baseline(&c.id)); + if has_company_side && has_baseline { + candidates + .into_iter() + .filter(|c| !is_baseline(&c.id)) + .collect() + } else { + candidates + } +} + /// The note line a card parks with when the pass declined to choose. /// /// Rendered in the same shape as the blocked-on-prerequisites reason — a diff --git a/src/harness/built_in/planning/test.rs b/src/harness/built_in/planning/test.rs index 2af49ed57..e845e3f0b 100644 --- a/src/harness/built_in/planning/test.rs +++ b/src/harness/built_in/planning/test.rs @@ -191,6 +191,7 @@ fn evidence() -> Evidence { role: a.role.clone(), description: a.description.clone(), grants: crate::runtime::builder::agent_effective_grants(&allow, &a.tools), + global: a.global, }) .collect(); Evidence { @@ -1723,6 +1724,125 @@ async fn a_manifest_teammate_and_a_runtime_one_can_be_the_ambiguous_pair() { ); } +/// Issue #1196. A tie between a company-authored teammate and a baseline one +/// is not the tie #1106 exists for: the company already expressed a +/// preference by staffing its own `Writer` (`maya`), so the baseline `writer` +/// (`globals/agents/writer.toml`, merged into every company's roster) steps +/// aside and the card dispatches instead of parking. Mirrors issue #1196's own +/// worked example — a company `Writer` tying against the global `writer`. +#[tokio::test] +async fn a_company_teammate_beats_a_baseline_tie_and_dispatches_without_parking() { + let reply = r#"{"description":"do it","steps":[],"prerequisites":[],"risks":[], + "verification":"v","scope":"s","assigneeCandidates":[ + {"id":"maya","reason":"the company's own writer"}, + {"id":"writer","reason":"the shared baseline writer"}]}"#; + let (_home, runtime) = runtime_with(ScriptedModel::replying(reply)).await; + runtime + .tasks() + .upsert(runtime.id(), &card("t-29", "")) + .await + .unwrap(); + + run_planning_pass(Arc::clone(&runtime), "t-29".to_string()).await; + + let after = read(&runtime, "t-29").await; + assert_eq!( + after.column, COLUMN_IN_PROGRESS, + "the company's own pick dispatches rather than parking" + ); + assert_eq!(after.assignee, "maya"); + let plan = after.plan.expect("the brief is still written"); + assert_eq!( + plan.proposed_assignee.as_deref(), + Some("maya"), + "the baseline candidate is dropped, leaving one proposal" + ); + assert!( + plan.assignee_candidates.is_empty(), + "with one candidate left there is no ownership question to persist" + ); +} + +/// The baseline exists for a company that never staffed a role itself — so a +/// tie between two baseline teammates carries no company preference and must +/// keep parking exactly like #1106's original case. +#[tokio::test] +async fn two_baseline_teammates_still_park_with_both() { + let reply = r#"{"description":"do it","steps":[],"prerequisites":[],"risks":[], + "verification":"v","scope":"s","assigneeCandidates":[ + {"id":"writer","reason":"could turn this into copy"}, + {"id":"researcher","reason":"could dig up the source material first"}]}"#; + let (_home, runtime) = runtime_with(ScriptedModel::replying(reply)).await; + runtime + .tasks() + .upsert(runtime.id(), &card("t-30", "")) + .await + .unwrap(); + + run_planning_pass(Arc::clone(&runtime), "t-30".to_string()).await; + + let after = read(&runtime, "t-30").await; + assert_eq!( + after.column, COLUMN_TODO, + "neither baseline teammate outranks the other" + ); + assert_eq!(after.assignee, ""); + assert_eq!( + after + .plan + .expect("plan") + .assignee_candidates + .iter() + .map(|c| c.id.as_str()) + .collect::>(), + vec!["writer", "researcher"], + "both survive, the same as any other unresolved tie" + ); +} + +/// Direct unit coverage of the precedence filter, independent of the planning +/// pass and any one scripted model. +#[test] +fn prefer_company_over_baseline_drops_only_a_true_mixed_tie() { + let mut evidence = evidence(); + for agent in evidence.record.manifest.agents.iter_mut() { + if agent.id == "sam" { + agent.global = true; + } + } + let candidate = |id: &str| AssigneeCandidate { + id: id.to_string(), + reason: String::new(), + }; + + // A company teammate and a baseline one: the baseline is dropped. + let mixed = prefer_company_over_baseline(&evidence, vec![candidate("maya"), candidate("sam")]); + assert_eq!( + mixed.iter().map(|c| c.id.as_str()).collect::>(), + vec!["maya"] + ); + + // A teammate and a desk: neither resolves to a baseline agent, so both + // count as company-side and the tie is untouched — #1106's case, and the + // reason an unresolved id (a desk) must default to company-side rather + // than silently misfiring as baseline. + let teammate_and_desk = + prefer_company_over_baseline(&evidence, vec![candidate("maya"), candidate("studio")]); + assert_eq!( + teammate_and_desk.len(), + 2, + "no baseline teammate in the tie" + ); + + // A single baseline candidate, alone: nothing to prefer it over. + let solo_baseline = prefer_company_over_baseline(&evidence, vec![candidate("sam")]); + assert_eq!( + solo_baseline.len(), + 1, + "a lone baseline candidate is not a tie" + ); +} + /// Direct unit coverage of the resolver's caps and drops, so the rules hold /// independently of what any one scripted model happens to emit. #[test] From 6eba1fb669a0a9f6dcfc5366ba3544e8b9bdc6e3 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 20 Aug 2026 14:27:38 +0530 Subject: [PATCH 04/14] test(harness): assert the roster prompt marks a baseline teammate (#1196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit review on #1246: direct coverage that a global teammate's prompt line carries "— from the shared baseline" and a company-authored teammate's does not. Co-Authored-By: Claude --- src/harness/built_in/planning/test.rs | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/harness/built_in/planning/test.rs b/src/harness/built_in/planning/test.rs index e845e3f0b..cd49b9f7b 100644 --- a/src/harness/built_in/planning/test.rs +++ b/src/harness/built_in/planning/test.rs @@ -1800,6 +1800,41 @@ async fn two_baseline_teammates_still_park_with_both() { ); } +/// Issue #1196. The prompt marks a baseline teammate as such, so the model +/// has the provenance evidence directly — even on a pass where the host-side +/// precedence never has to act on it, as here: one company teammate proposed, +/// no tie in play. +#[tokio::test] +async fn the_prompt_marks_a_baseline_teammate_from_the_shared_baseline() { + let model = ScriptedModel::replying(CLEAN_PLAN); + let (_home, runtime) = runtime_with(Arc::clone(&model)).await; + runtime + .tasks() + .upsert(runtime.id(), &card("t-31", "")) + .await + .unwrap(); + + run_planning_pass(Arc::clone(&runtime), "t-31".to_string()).await; + + let prompt = model.last_prompt(); + let writer_line = prompt + .lines() + .find(|l| l.contains("`writer`")) + .unwrap_or_else(|| panic!("the merged baseline puts `writer` on the roster:\n{prompt}")); + assert!( + writer_line.contains("— from the shared baseline"), + "{writer_line}" + ); + let maya_line = prompt + .lines() + .find(|l| l.contains("`maya`")) + .unwrap_or_else(|| panic!("the company's own roster is still shown:\n{prompt}")); + assert!( + !maya_line.contains("— from the shared baseline"), + "a company-authored teammate is never mis-marked:\n{maya_line}" + ); +} + /// Direct unit coverage of the precedence filter, independent of the planning /// pass and any one scripted model. #[test] From 1412c39fa1a10d20ce47732797489b049c7955dc Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 20 Aug 2026 17:02:36 +0530 Subject: [PATCH 05/14] fix: give codex a real model lever via session/set_config_option (#1245) Verified live rather than left as a documented gap. Tried four candidate startup env vars against a real codex-acp (OPENAI_MODEL, CODEX_MODEL, MODEL, OPENAI_DEFAULT_MODEL) -- none moved the reported current model off its default. codex-acp does advertise a real configOptions entry with category "model" though, and session/set_config_option against it, right after session/new, does work -- confirmed live, and that it's per-session state (a second, independent session reverts to the adapter's default). LocalAcpAgent::session_for now tries that fallback whenever this build has no known startup env var for the agent (model_env_var returned None) and a model was requested -- not codex-specific in the code, so any future agent in the same position gets it for free. Guarded so it never fires redundantly when an env var already carried the model at spawn. New: a deterministic (non-live, runs in CI) test pinning the configId lookup against codex-acp's real captured response shape, plus a live test proving the full LocalAcpAgent path completes through the fallback without error against the real adapter. Co-Authored-By: Claude --- docs/spec/runtime/harnesses.md | 21 ++++-- src-tauri/src/acp/local_agent.rs | 110 ++++++++++++++++++++++++++++-- src-tauri/tests/acp_live_smoke.rs | 99 +++++++++++++++++++++++++++ 3 files changed, 216 insertions(+), 14 deletions(-) diff --git a/docs/spec/runtime/harnesses.md b/docs/spec/runtime/harnesses.md index 4e5c4a718..b1c4a6da1 100644 --- a/docs/spec/runtime/harnesses.md +++ b/docs/spec/runtime/harnesses.md @@ -74,20 +74,27 @@ reading twice. ### Model -`[harness.acp].model` is a hint forwarded to the agent's own startup lever — +`[harness.acp].model` is a hint forwarded to the agent's own model lever — not a credential, so it does not join `[harness.inference]`'s prohibition on `acp` harnesses (see [Validation](#validation)). Optional; a harness with none runs whatever the agent's own config or CLI default resolves to. -Whether it actually does anything depends on whether this build knows a -startup lever for that `agent` — confirmed live against the real adapters -(issue #1245), not guessed: +`LocalAcpAgent` reaches that lever one of two ways, confirmed live against the +real adapters (issue #1245), not guessed — whichever this build knows for that +`agent`: | `agent` | lever | |---|---| -| `claude` | `ANTHROPIC_MODEL` | -| `goose` | `GOOSE_MODEL` | -| `codex` | none known yet — `model` is accepted and validated, but not injected | +| `claude` | startup env var `ANTHROPIC_MODEL` | +| `goose` | startup env var `GOOSE_MODEL` | +| `codex` | no startup env var (`OPENAI_MODEL`, `CODEX_MODEL`, `MODEL` and `OPENAI_DEFAULT_MODEL` all tried, none had any effect) — instead, `session/set_config_option` right after `session/new`, using the `configOptions` entry `codex-acp` itself advertises with `category: "model"` | + +The `set_config_option` fallback is not codex-specific in the code — it fires +for any agent whose startup env var this build does not know, whenever the +fresh `session/new` response advertises a `category: "model"` option matching +the requested value. It is per-session state, confirmed live: a second, +independent session on the same subprocess starts back at the adapter's +default, not the previously-set model. `transport = "local"` only, for now: the `runner` wire protocol does not carry `model`, so validation rejects it there rather than accepting and silently diff --git a/src-tauri/src/acp/local_agent.rs b/src-tauri/src/acp/local_agent.rs index 120b2bf87..19760ae04 100644 --- a/src-tauri/src/acp/local_agent.rs +++ b/src-tauri/src/acp/local_agent.rs @@ -41,15 +41,14 @@ use crate::acp::discovery::HARNESSES; /// Per-CLI startup model env var, confirmed live against the real adapter /// (issue #1245's live smoke test) — not guessed. `None` means this build has -/// no known lever for that CLI: `model` is still accepted on the manifest, -/// but nothing is injected, rather than silently spawning a process that -/// ignores the setting. +/// no known startup env var for that CLI, and [`LocalAcpAgent::session_for`] +/// falls back to the ACP-native `session/set_config_option` path instead — +/// also confirmed live, for `codex-acp` specifically (its `configOptions` +/// model entry accepts a set; no env var candidate tried had any effect). fn model_env_var(agent: &str) -> Option<&'static str> { match agent { "claude" => Some("ANTHROPIC_MODEL"), "goose" => Some("GOOSE_MODEL"), - // codex: no confirmed startup-model env var. Buzz (block/buzz), the - // one other project this design is modeled on, has none either. _ => None, } } @@ -60,6 +59,12 @@ pub struct LocalAcpAgent { command: &'static str, args: Vec, env: Vec<(String, String)>, + /// The desired model, kept regardless of whether an env var already + /// carries it — [`Self::session_for`] falls back to + /// `session/set_config_option` when [`model_env_var`] returned `None` at + /// construction, so this is the only record of what was actually asked + /// for in that case. + model: Option, /// The company's agent-workspace root (`HarnessDeps::workspace_root`). /// Each session roots at `workspace_root///workspace`, /// mirroring `harness::built_in::build::agent_workspace` exactly, so an @@ -92,6 +97,7 @@ impl LocalAcpAgent { command: def.command, args: def.args.iter().map(|a| a.to_string()).collect(), env, + model: model.map(str::to_string), workspace_root, client: AsyncMutex::new(None), sessions: AsyncMutex::new(HashMap::new()), @@ -174,6 +180,15 @@ impl LocalAcpAgent { } /// This session's cached ACP `sessionId`, opening one if none exists yet. + /// + /// A fresh session is where model steering happens when no startup env + /// var carries it ([`model_env_var`] returned `None` for this agent): + /// `session/new`'s own response is inspected for a `configOptions` entry + /// with `category: "model"` whose options include the desired value, and + /// if found, `session/set_config_option` applies it before this session + /// is used for anything. Confirmed live to be per-session state (not + /// global), which is exactly the granularity wanted — a session opened + /// here is one (company, agent) pair for its whole life. async fn session_for( &self, client: &AcpClient, @@ -184,10 +199,52 @@ impl LocalAcpAgent { if let Some(id) = sessions.get(session_key) { return Ok(id.clone()); } - let id = client - .new_session(root) + + let raw = client + .call( + "session/new", + serde_json::json!({ "cwd": root.display().to_string(), "mcpServers": [] }), + ) .await .map_err(|error| OpenCompanyError::Config(format!("acp session/new: {error}")))?; + let id = raw["sessionId"] + .as_str() + .ok_or_else(|| { + OpenCompanyError::Config("acp session/new returned no sessionId".to_string()) + })? + .to_string(); + + // `self.env` carries the model only when `new()` found a known env + // var for this agent — non-empty means the spawn already handled it, + // so the fallback below must not also fire (redundant at best, and + // this session's model would otherwise be decided by whichever of + // the two APIs the adapter honors last). No matching `config_id` + // falls through the same way: either an env var already carried it + // at spawn, or this build has no lever for this agent at all (issue + // #1245's documented codex gap, before this fallback existed) — + // either way, silently doing nothing here is correct, not a missed + // error. + if self.env.is_empty() + && let Some(model) = &self.model + && let Some(config_id) = model_config_id(&raw, model) + { + client + .call( + "session/set_config_option", + serde_json::json!({ + "sessionId": id, + "configId": config_id, + "value": model, + }), + ) + .await + .map_err(|error| { + OpenCompanyError::Config(format!( + "acp session/set_config_option (model `{model}`): {error}" + )) + })?; + } + sessions.insert(session_key.to_string(), id.clone()); Ok(id) } @@ -204,6 +261,45 @@ impl LocalAcpAgent { } } +/// Finds the `configId` to set to reach `desired_model`, from a fresh +/// `session/new` response's `configOptions` — the entry whose `category` is +/// `"model"` and whose `options` include a `value` matching `desired_model`. +/// `None` when nothing matches: either this adapter advertises no such +/// option, or it does but not for this exact value. +/// +/// Accepts both `configId` (the ACP spec's own name) and `id` — confirmed +/// live that `codex-acp` emits `id`, matching the same quirk documented for +/// `claude-agent-acp` in `harness::acp::run_turn`. +/// +/// `pub` (not private) so `tests/acp_live_smoke.rs` can pin this parsing +/// against a captured real response without a live spawn — the one part of +/// the fallback that can be tested deterministically and in CI. +pub fn model_config_id(session_new_result: &Value, desired_model: &str) -> Option { + session_new_result["configOptions"] + .as_array()? + .iter() + .find_map(|opt| { + if opt.get("category").and_then(|c| c.as_str()) != Some("model") { + return None; + } + let matches = opt + .get("options") + .and_then(|o| o.as_array()) + .is_some_and(|options| { + options + .iter() + .any(|o| o.get("value").and_then(|v| v.as_str()) == Some(desired_model)) + }); + if !matches { + return None; + } + opt.get("configId") + .or_else(|| opt.get("id")) + .and_then(|v| v.as_str()) + .map(str::to_string) + }) +} + /// Translates one raw `session/update` notification into this crate's /// [`AcpUpdate`], or `None` for a kind that is dropped rather than /// approximated (`plan`, `available_commands_update`, …) — see diff --git a/src-tauri/tests/acp_live_smoke.rs b/src-tauri/tests/acp_live_smoke.rs index bc58a2075..a6154e817 100644 --- a/src-tauri/tests/acp_live_smoke.rs +++ b/src-tauri/tests/acp_live_smoke.rs @@ -244,3 +244,102 @@ async fn local_acp_agent_answers_a_prompt_through_the_acp_agent_trait() { .is_dir() ); } + +/// `codex-acp` has no startup-model env var — confirmed by trying +/// `OPENAI_MODEL`, `CODEX_MODEL`, `MODEL` and `OPENAI_DEFAULT_MODEL` against +/// the real adapter; none moved `currentValue` off its default +/// (`gpt-5.6-sol`). It does advertise a real `configOptions` model entry +/// though, so `LocalAcpAgent` falls back to `session/set_config_option`, +/// applied once per session right after `session/new` — this proves that +/// fallback actually works, through the `AcpAgent` trait rather than the raw +/// client. +#[tokio::test] +#[ignore = "spawns a real, authenticated codex-acp and costs real usage"] +async fn local_acp_agent_steers_codex_via_set_config_option_fallback() { + use opencompany::ports::acp::AcpAgentFactory; + use opencompany::ports::types::CompanyId; + use opencompany_desktop_lib::acp::LocalAcpAgentFactory; + + let dir = tempfile::tempdir().unwrap(); + let workspace_root = dir.path().canonicalize().unwrap(); + + let agent = LocalAcpAgentFactory + .build("codex", Some("gpt-5.5"), &workspace_root) + .expect("codex-acp must be on PATH"); + + let company = CompanyId::new("acme-codex-smoke"); + // `prompt` is what actually opens the session and runs the fallback + // (`session/set_config_option`) before the turn starts. Not asserting on + // which model answered — a model's own self-reported name is not a + // reliable oracle for the ACP-level `value` id it was switched to, + // especially for aliased/internal names like these. What this proves is + // that the fallback call itself is accepted by the real adapter with no + // error: `model_config_id_matches_the_real_codex_shape` (below) already + // proves the *parsing* deterministically, and the manual probe that + // designed this fallback directly confirmed `currentValue` changes in + // `session/set_config_option`'s own echoed response. + let turn = agent + .prompt( + &company, + &format!("{}::researcher", company.as_ref()), + "Reply with exactly the single word PONG and nothing else.", + ) + .await + .expect("prompt — including its session/set_config_option fallback call"); + + assert_eq!( + turn.stop_reason, "end_turn", + "updates were: {:?}", + turn.updates + ); +} + +/// Pins `model_config_id`'s parsing against the real shape `codex-acp` +/// returns from `session/new` (captured live while designing the +/// `session/set_config_option` fallback) — including its `"id"` key rather +/// than the ACP spec's own `"configId"`, and the other, non-model config +/// options a real response carries alongside it. No process spawned, so this +/// runs in CI unlike the rest of this file. +#[test] +fn model_config_id_matches_the_real_codex_shape() { + let raw: serde_json::Value = serde_json::from_str( + r#"{ + "sessionId": "sess-1", + "configOptions": [ + { + "id": "mode", + "category": "mode", + "currentValue": "agent", + "options": [{"value": "agent"}] + }, + { + "id": "model", + "category": "model", + "currentValue": "gpt-5.6-sol", + "options": [ + {"value": "gpt-5.6-sol"}, + {"value": "gpt-5.5"}, + {"value": "gpt-5.4"} + ] + }, + { + "id": "reasoning_effort", + "category": "thought_level", + "currentValue": "medium", + "options": [{"value": "medium"}] + } + ] + }"#, + ) + .unwrap(); + + assert_eq!( + opencompany_desktop_lib::acp::local_agent::model_config_id(&raw, "gpt-5.5"), + Some("model".to_string()) + ); + assert_eq!( + opencompany_desktop_lib::acp::local_agent::model_config_id(&raw, "not-a-real-model"), + None, + "must not match a value the adapter never advertised" + ); +} From 1bd4f21d3dc820a963244a4f2b1a4c71d82ee41c Mon Sep 17 00:00:00 2001 From: oxoxDev Date: Thu, 20 Aug 2026 16:35:05 +0530 Subject: [PATCH 06/14] fix(mcp): tell a Sign in button from a token field before the click (#1260) A server can require OAuth and still be undrivable from this console. Slack's MCP endpoint answers 401 with a proper resource-metadata challenge and advertises no `registration_endpoint`, so `begin` has no client to mint and refuses -- correctly, but only once the operator has pressed a button that could never work, and the refusal is the only place the real remedy appears. The probe could not say so because `oauth_required` covered both states. It answers one question, "did this server ask for OAuth", and the console reads it as the answer to a different one, "can we complete a sign-in". Those come apart exactly when dynamic client registration is missing. `select_auth_server` already decides this -- authorize + token + registration + authorization_code -- inside `begin`, at click time. `supports_console_oauth` reuses it rather than restating the rule, so the question the probe asks and the one `begin` enforces cannot drift. The refinement sits in `probe_server`, not in `classify_mcp_error`, which is pure and synchronous by design and holds only the resource-metadata URL, never the fetched authorization-server document. It runs on the `OauthRequired` arm alone, so a healthy server pays nothing: by then this server has already answered 401. Bounded at 5s, because a metadata endpoint that accepts a connection and then hangs would otherwise stall the probe. Every uncertain answer is `true`. Discovery is a live call that fails for reasons that say nothing about capability, and answering `false` on a timeout would replace a working Sign in button with "paste a token" on a server that signs in perfectly well -- worse than the behaviour being fixed. Unsure means leave it as it was. Both credential messages also stop naming Connections, which carries no MCP server row at all. An instruction that cannot be followed reads as a broken feature rather than a missing token. Gated on `mcp` with a no-op fallback: a build without it has no `oauth/start` route, so there is no button to withdraw. Part of #1260 --- src/company/mcp_oauth.rs | 45 +++++++++ src/harness/built_in/mcp_probe.rs | 148 +++++++++++++++++++++++++++++- 2 files changed, 191 insertions(+), 2 deletions(-) diff --git a/src/company/mcp_oauth.rs b/src/company/mcp_oauth.rs index e4e29dd6f..06e9feade 100644 --- a/src/company/mcp_oauth.rs +++ b/src/company/mcp_oauth.rs @@ -325,6 +325,51 @@ fn build_authorize_url( } /// Begin the browser-OAuth flow for `server_name` at `endpoint`: discover → +/// How long [`supports_console_oauth`] will wait on OAuth discovery. +/// +/// Short on purpose: this is a refinement of an answer the probe already has, +/// not the answer itself, so it must not dominate the probe's own budget. +const CONSOLE_OAUTH_DISCOVERY_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); + +/// Whether console OAuth can actually drive this server (issue #1260). +/// +/// [`begin`] answers this already, but only by attempting the flow and failing — +/// which is too late for the health probe, whose answer decides whether the +/// console offers a **Sign in** button or a credential field. A server can +/// require OAuth and still be undrivable from here: Slack's MCP endpoint +/// answers `401` with a proper resource-metadata challenge and advertises no +/// `registration_endpoint`, so there is no client for us to mint and no sign-in +/// we can complete. +/// +/// Reuses [`select_auth_server`] rather than re-deriving the rule, so the +/// question the probe asks and the one `begin` enforces cannot drift apart. +/// +/// **Every failure answers `true`.** Discovery is a live outbound call and can +/// fail for reasons that say nothing about the server's capabilities — a +/// timeout, a transient 5xx, a network blip. Answering `false` on one of those +/// would replace a working Sign in button with "paste a token" on a server that +/// supports sign-in perfectly well, which is a worse outcome than today's +/// behaviour. Unsure therefore means "leave it as it was". +pub(crate) async fn supports_console_oauth(endpoint: &str) -> bool { + // Bounded explicitly: this sits on the health-probe path, and a metadata + // endpoint that accepts a connection and then hangs would otherwise stall + // the probe for whatever the HTTP client's own default happens to be. + // Timing out answers `true` for the same reason every other failure does. + let Ok(discovered) = + tokio::time::timeout(CONSOLE_OAUTH_DISCOVERY_TIMEOUT, discover(endpoint)).await + else { + return true; + }; + match discovered { + // No authorization context at all: the server did not ask for OAuth on + // this probe. Nothing to downgrade — the caller's classification stands. + Ok(None) => true, + Ok(Some(ctx)) => select_auth_server(ctx).is_some(), + Err(_) => true, + } +} + +/// Discover the server's OAuth metadata, register a client via RFC 7591 /// dynamic client registration → PKCE, and return the live `/authorize` URL plus /// the [`PendingOAuth`] the caller parks (keyed by the returned `state`). /// diff --git a/src/harness/built_in/mcp_probe.rs b/src/harness/built_in/mcp_probe.rs index b47fd2855..5b1595125 100644 --- a/src/harness/built_in/mcp_probe.rs +++ b/src/harness/built_in/mcp_probe.rs @@ -62,6 +62,14 @@ enum FailureKind { ServerError, /// A tool call was rejected by the server (JSON-RPC error in call context). ToolCallRejected, + /// The server requires OAuth, but console OAuth cannot drive it: it + /// advertises no RFC 7591 dynamic client registration, so there is no + /// client to mint and no sign-in to complete (issue #1260). + /// + /// Distinct from [`FailureKind::OauthRequired`] because the operator's next + /// action is the opposite one — paste a static token, rather than press a + /// Sign in button that cannot succeed. + StaticTokenRequired, /// Anything not otherwise recognised. Unknown, } @@ -86,6 +94,7 @@ impl FailureKind { match self { FailureKind::CredentialRequired => "credential_required", FailureKind::OauthRequired => "oauth_required", + FailureKind::StaticTokenRequired => "static_token_required", FailureKind::TokenRejected => "token_rejected", FailureKind::Timeout => "timeout", FailureKind::Unreachable => "unreachable", @@ -103,6 +112,7 @@ impl FailureKind { match self { FailureKind::CredentialRequired | FailureKind::OauthRequired + | FailureKind::StaticTokenRequired | FailureKind::TokenRejected => McpStatus::NeedsConfig, _ => McpStatus::Error, } @@ -113,6 +123,7 @@ impl FailureKind { match self { FailureKind::CredentialRequired => Some("credential_required".to_string()), FailureKind::OauthRequired => Some("oauth_required".to_string()), + FailureKind::StaticTokenRequired => Some("static_token_required".to_string()), FailureKind::TokenRejected => Some("token_rejected".to_string()), _ => None, } @@ -299,10 +310,13 @@ fn looks_like_tls(err: &anyhow::Error) -> bool { pub fn operator_message(server: &str, class: &ProbeClass, err: &anyhow::Error) -> String { match class.kind { FailureKind::CredentialRequired => format!( - "MCP server '{server}' needs a credential. Add its API token (or query-parameter key) in Connections, then Test again." + "MCP server '{server}' needs a credential. Add its API token (or query-parameter key) in its Token field, then Test again." ), FailureKind::OauthRequired => format!( - "MCP server '{server}' needs OAuth sign-in — click Sign in on the server in Connections to authorize it." + "MCP server '{server}' needs OAuth sign-in — click Sign in on this server to authorize it." + ), + FailureKind::StaticTokenRequired => format!( + "MCP server '{server}' requires OAuth, but it doesn't offer the automatic client registration this console signs in with. Paste a static API token in its Token field, then Test again." ), FailureKind::TokenRejected => format!( "MCP server '{server}' rejected the credential — it's wrong or expired. Update it and Test again." @@ -360,6 +374,12 @@ pub async fn probe_server(decl: &McpServerDecl) -> McpHealth { } Err(err) => { let class = classify_mcp_error(&err, auth_configured, false); + // Issue #1260: "wants OAuth" and "we can sign in" are two different + // questions, and only the second decides whether the console should + // offer a Sign in button. Asked here rather than in + // `classify_mcp_error`, which is pure and synchronous by design — + // this needs a live discovery call. + let class = refine_oauth_capability(&decl.endpoint, class).await; let message = scrub(&operator_message(&decl.name, &class, &err), &secrets); McpHealth { status: class.status, @@ -372,6 +392,32 @@ pub async fn probe_server(decl: &McpServerDecl) -> McpHealth { } } +/// Downgrade an `oauth_required` verdict to `static_token_required` when console +/// OAuth provably cannot drive this server (issue #1260). +/// +/// Only ever consulted for [`FailureKind::OauthRequired`], so a healthy server +/// pays nothing: this runs on a server that already answered `401`. +#[cfg(feature = "mcp")] +async fn refine_oauth_capability(endpoint: &str, class: ProbeClass) -> ProbeClass { + if class.kind != FailureKind::OauthRequired + || crate::company::mcp_oauth::supports_console_oauth(endpoint).await + { + return class; + } + ProbeClass { + status: FailureKind::StaticTokenRequired.status(), + auth_hint: FailureKind::StaticTokenRequired.auth_hint(), + kind: FailureKind::StaticTokenRequired, + } +} + +/// Without the `mcp` feature there is no `oauth/start` route for the console to +/// call, so there is no Sign in button to withdraw and nothing to refine. +#[cfg(not(feature = "mcp"))] +async fn refine_oauth_capability(_endpoint: &str, class: ProbeClass) -> ProbeClass { + class +} + /// Scrub a message so it can be safely persisted, returned, or shown to an agent. /// /// Three passes, in order: @@ -691,6 +737,104 @@ mod tests { assert_eq!(class.status, McpStatus::NeedsConfig); } + /// Issue #1260: the two OAuth states are distinguishable on the wire, and + /// they carry the two different actions an operator has to take. + #[test] + fn the_two_oauth_states_do_not_share_a_hint() { + assert_eq!( + FailureKind::OauthRequired.auth_hint().as_deref(), + Some("oauth_required") + ); + assert_eq!( + FailureKind::StaticTokenRequired.auth_hint().as_deref(), + Some("static_token_required") + ); + assert_ne!( + FailureKind::OauthRequired.auth_hint(), + FailureKind::StaticTokenRequired.auth_hint(), + "the console renders a Sign in button off this code; collapsing the two \ + is what put an unusable button on a Slack row" + ); + // Both are a resting state the operator can fix, not a failure. + assert_eq!( + FailureKind::StaticTokenRequired.status(), + McpStatus::NeedsConfig + ); + assert_eq!( + FailureKind::StaticTokenRequired.code(), + "static_token_required" + ); + } + + /// The message names the field the operator can actually use. + /// + /// The previous text sent them to Connections, which carries no MCP server + /// row at all — a instruction that cannot be followed reads as a broken + /// feature rather than a missing token. + #[test] + fn the_credential_messages_name_a_field_that_exists() { + for kind in [ + FailureKind::StaticTokenRequired, + FailureKind::CredentialRequired, + ] { + let class = ProbeClass { + status: kind.status(), + auth_hint: kind.auth_hint(), + kind, + }; + let msg = operator_message("slack", &class, &anyhow_str("x")); + assert!( + msg.contains("Token field"), + "{kind:?} must name the Token field: {msg}" + ); + assert!( + !msg.contains("in Connections"), + "{kind:?} still points at Connections, which has no MCP row: {msg}" + ); + } + } + + /// A refinement only ever fires on the one kind it is about, and every + /// uncertain answer leaves the classification alone. + #[tokio::test] + async fn refinement_leaves_every_other_kind_untouched() { + for kind in [ + FailureKind::CredentialRequired, + FailureKind::TokenRejected, + FailureKind::Timeout, + FailureKind::Unreachable, + ] { + let class = ProbeClass { + status: kind.status(), + auth_hint: kind.auth_hint(), + kind, + }; + // An endpoint that cannot resolve: discovery fails, and a failed + // discovery must never rewrite a verdict. + let refined = refine_oauth_capability("https://127.0.0.1:1/mcp", class.clone()).await; + assert_eq!(refined, class, "{kind:?} was rewritten by the refinement"); + } + } + + /// Discovery that fails keeps `oauth_required` rather than downgrading. + /// + /// The safe direction is the one that does not regress a server whose sign-in + /// works: a timeout or a transient must not replace a working Sign in button + /// with "paste a token". + #[tokio::test] + async fn an_unreachable_discovery_keeps_sign_in() { + let class = ProbeClass { + status: FailureKind::OauthRequired.status(), + auth_hint: FailureKind::OauthRequired.auth_hint(), + kind: FailureKind::OauthRequired, + }; + let refined = refine_oauth_capability("https://127.0.0.1:1/mcp", class.clone()).await; + assert_eq!( + refined, class, + "an unreachable discovery must leave the verdict as it was" + ); + } + #[test] fn classify_typed_401_without_metadata_respects_credential_state() { let err = anyhow::Error::new(oh::mcp::http_client::McpUnauthorizedError { From 46c26b6e8f121e7ddbe9491b752bf75e5d7b58dd Mon Sep 17 00:00:00 2001 From: oxoxDev Date: Thu, 20 Aug 2026 16:35:20 +0530 Subject: [PATCH 07/14] feat(console): offer a token field where sign-in cannot work (#1260) Which control a row offers is now a function of the host's hint and nothing else, stated once in `credentialAffordance` rather than as two inline conditions. `sign_in` for `oauth_required`; `add_token` for the new `static_token_required` and for a plain `credential_required`, which wanted the same field and simply never had a button to withdraw. The inline field is the half that had to come with it. Following the old error was impossible: the only Token input on the page belongs to the *add* form, so an operator told to paste a token for an existing server would have created a second copy of it. The host has accepted a credential rotation on `PUT .../mcp/servers/{name}` all along -- `token`, `authKind`, `headerName`, `paramName` -- and the control was simply never built. Per-row, write-only, Enter to save and Escape to close. It re-tests on success rather than trusting the write, because the operator cannot know whether the token was the right one until the server answers, and a silent save would leave the amber badge sitting there with no way to tell a wrong token from an unsaved one. Verified against a live host: a junk token saves, re-probes, and stays `needs_config` rather than turning green, and the value appears nowhere in the server list response. A no-auth server grows no credential control at all -- the easy way to get this wrong is to sprout a token prompt on every row. Closes #1260 --- .../views/connections/McpServersSection.tsx | 139 +++++++++++++++++- .../unit/mcp-credential-affordance.test.ts | 44 ++++++ 2 files changed, 181 insertions(+), 2 deletions(-) create mode 100644 frontend/test/unit/mcp-credential-affordance.test.ts diff --git a/frontend/src/views/connections/McpServersSection.tsx b/frontend/src/views/connections/McpServersSection.tsx index 39229c209..0a7ba72fa 100644 --- a/frontend/src/views/connections/McpServersSection.tsx +++ b/frontend/src/views/connections/McpServersSection.tsx @@ -5,6 +5,7 @@ import { ChevronDown, ChevronRight, Info, + KeyRound, Loader2, LogIn, Plug, @@ -37,6 +38,32 @@ import { Skeleton } from "@/components/ui/skeleton"; import { Switch } from "@/components/ui/switch"; import { ProviderDetail } from "@/views/connections/ProviderDetail"; +/** + * What a server's health entitles its row to offer (issue #1260). + * + * A rule rather than two inline conditions, because the two OAuth states differ + * by exactly one thing an operator cannot see: whether the server advertises + * dynamic client registration. `oauth_required` means the server asked for + * OAuth; only the host knows whether this console can complete one, and it says + * so by sending `static_token_required` instead. Reading them as the same state + * is what put a Sign in button on a Slack row that could never sign in. + */ +export function credentialAffordance( + authHint: string | undefined, +): "sign_in" | "add_token" | "none" { + switch (authHint) { + case "oauth_required": + return "sign_in"; + case "static_token_required": + // A plain credential prompt wants the same field; it simply never had a + // sign-in button to withdraw. + case "credential_required": + return "add_token"; + default: + return "none"; + } +} + type McpLoad = "loading" | "ready" | "unavailable" | "error"; type ToolsState = | { kind: "idle" } @@ -96,6 +123,17 @@ export function McpServersSection({ client, company, canManage, chrome = "inline // A name rather than the row itself, so an open panel re-derives from // `servers` after a refresh instead of showing the row as it was when clicked. const [opened, setOpened] = useState(null); + /** + * The server whose inline credential field is open, and its draft value + * (issue #1260). + * + * Per-row rather than a shared field: the add form's Token creates a *new* + * server, so pointing an operator at it to fix an existing one would have + * them add a second copy. The host has accepted a credential rotation on + * `PUT …/mcp/servers/{name}` all along — this is the control that was missing. + */ + const [credentialFor, setCredentialFor] = useState(null); + const [credentialDraft, setCredentialDraft] = useState(""); // Set by the unmount cleanup below. A sign-in poll that is mid-`await` when // this component goes away has already removed its own timer entry, so the // cleanup has nothing left to cancel — it checks this instead of re-arming. @@ -210,7 +248,12 @@ export function McpServersSection({ client, company, canManage, chrome = "inline // Exception: an OAuth-required result is not an error to shout about — the // amber "needs config" badge carries a Sign in button, so a red alert here // would be redundant and misleading. - if (res.test && res.test.status !== "ok" && res.test.authHint !== "oauth_required") { + if ( + res.test && + res.test.status !== "ok" && + res.test.authHint !== "oauth_required" && + res.test.authHint !== "static_token_required" + ) { setAddError(res.test.message); } else if (res.warning) { setAddError(res.warning); @@ -253,6 +296,32 @@ export function McpServersSection({ client, company, canManage, chrome = "inline // Browser OAuth sign-in (issue #90): open the authorization URL in a new tab, // then poll the server's health until it flips to `ok` (the host stores the // token on its callback route) so the amber badge turns green on its own. + /** + * Rotate one server's credential from its own row (issue #1260). + * + * Re-tests on success rather than trusting the write: the whole point of the + * flow is that the operator does not know whether the token is the right one + * until the server answers, and a silent save would leave the amber badge + * sitting there with no way to tell "wrong token" from "not saved". + */ + async function saveCredential(server: McpServer) { + if (busy) return; + const token = credentialDraft.trim(); + if (!token) return; + setBusy(server.name); + try { + await updateMcpServer(client, company, server.name, { token, authKind: "bearer" }); + setCredentialFor(null); + setCredentialDraft(""); + await refresh(); + await test(server); + } catch (err) { + toast.error(err instanceof ApiError ? err.message : "Couldn't save the token."); + } finally { + setBusy(null); + } + } + async function signIn(server: McpServer) { // Guard both the shared `busy` flag and a per-server poll already in flight: // the poll outlives `busy`, so without the second check a repeat click would @@ -474,7 +543,32 @@ export function McpServersSection({ client, company, canManage, chrome = "inline onCheckedChange={(v) => void toggle(server, v)} aria-label={`Enable ${server.name}`} /> - {health?.authHint === "oauth_required" && canManage && ( + {/* Issue #1260: `oauth_required` means the server asked for + OAuth; it does NOT mean this console can complete one. A + server advertising no dynamic client registration — Slack's + MCP endpoint, for one — has no client for us to mint, so + Sign in cannot succeed and the host says so with a distinct + hint. Offering the button anyway spends a click to reach an + error naming something the operator cannot act on, which is + the same trade `hub_providers` already refuses to make for + the Google and GitHub buttons. */} + {credentialAffordance(health?.authHint) === "add_token" && + canManage && + credentialFor !== server.name && ( + + )} + {credentialAffordance(health?.authHint) === "sign_in" && canManage && ( + + + )} ); diff --git a/frontend/test/unit/mcp-credential-affordance.test.ts b/frontend/test/unit/mcp-credential-affordance.test.ts new file mode 100644 index 000000000..2f41b57e7 --- /dev/null +++ b/frontend/test/unit/mcp-credential-affordance.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from "vitest"; + +import { credentialAffordance } from "@/views/connections/McpServersSection"; + +/** + * Issue #1260. The MCP row offered a **Sign in** button on a server whose OAuth + * could never complete: Slack's MCP endpoint answers `401` with a proper + * resource-metadata challenge but advertises no RFC 7591 dynamic client + * registration, so there is no client to mint. `POST …/oauth/start` refused + * with a `400` naming the real remedy — paste a static token — which the + * operator could only read by pressing a button that could not work. + * + * The host now distinguishes the two states. These pin the console half: which + * control a row offers is a function of that hint and nothing else. + */ +describe("credentialAffordance", () => { + it("offers sign-in only when the host says sign-in can complete", () => { + expect(credentialAffordance("oauth_required")).toBe("sign_in"); + }); + + it("offers a token field when OAuth is required but undrivable", () => { + expect(credentialAffordance("static_token_required")).toBe("add_token"); + }); + + it("never offers sign-in for a server that cannot complete one", () => { + // The regression itself. Both codes carry `status: needs_config` and both + // read as "an auth problem"; only this distinction stops the unusable + // button coming back. + expect(credentialAffordance("static_token_required")).not.toBe("sign_in"); + }); + + it("routes a plain credential prompt to the same field", () => { + expect(credentialAffordance("credential_required")).toBe("add_token"); + }); + + it("offers nothing for a healthy server or an unknown code", () => { + // A server that probed `ok` carries no hint at all, and must not sprout a + // credential control; an unrecognised future code must not either, because + // guessing which control it wants is how the wrong one gets offered. + expect(credentialAffordance(undefined)).toBe("none"); + expect(credentialAffordance("token_rejected")).toBe("none"); + expect(credentialAffordance("some_code_added_later")).toBe("none"); + }); +}); From ab6bd1e57f62e2ac812fcc3ee5f7ea355057c838 Mon Sep 17 00:00:00 2001 From: cyrus Date: Thu, 20 Aug 2026 17:32:35 +0530 Subject: [PATCH 08/14] fix: send ledger status needsReason as camelCase on the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The console's "ask for a reason before closing" guard never fired on any declared ledger: the server sent StatusSpec::needs_reason as snake_case while the frontend's LedgerStatus.needsReason expected camelCase, so it always read undefined. StatusSpec itself must stay snake_case in both directions — it's also what a declared ledger round-trips through in every store backend, so an asymmetric rename there would silently drop the flag back to false on save-then-reload (verified this against closing_without_a_reason_is_a_400_that_says_so, which broke under that approach). Instead, add a wire-only LedgerStatusDto in src/server/ops/ledgers.rs with #[serde(rename_all = "camelCase")], and have LedgerSummary.statuses build through it. Closes #1266 --- src/ledger/spec.rs | 9 +++++++++ src/ledger/spec_test.rs | 22 ++++++++++++++++++++++ src/server/ops/ledgers.rs | 33 +++++++++++++++++++++++++++++++-- src/server/ops/ledgers_test.rs | 27 +++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/src/ledger/spec.rs b/src/ledger/spec.rs index 4c7b86caf..6c2ea1c25 100644 --- a/src/ledger/spec.rs +++ b/src/ledger/spec.rs @@ -136,6 +136,15 @@ pub struct StatusSpec { #[serde(default)] pub closed: bool, /// Whether closing into this status demands a `reason`. + /// + /// Plain snake_case on purpose (issue #1266's near-miss): this type is + /// also what a declared ledger round-trips through in every store + /// backend (`store/fs_ops.rs`, `sqlite.rs`, `mongodb.rs` all persist + /// `LedgerSpec` via this same `Serialize`/`Deserialize`), so a rename + /// here would make Serialize write one key and Deserialize expect + /// another — silently dropping `needs_reason` back to `false` on the + /// very next reload. The console-facing camelCase key belongs on a + /// wire-only DTO instead — see `server::ops::ledgers::LedgerStatusDto`. #[serde(default)] pub needs_reason: bool, } diff --git a/src/ledger/spec_test.rs b/src/ledger/spec_test.rs index 52a7f94ac..f687ce40a 100644 --- a/src/ledger/spec_test.rs +++ b/src/ledger/spec_test.rs @@ -34,6 +34,28 @@ fn a_minimal_declaration_parses() { assert!(spec.written_by.contains("record_entry")); } +/// **`StatusSpec` itself stays snake_case both ways** (issue #1266's +/// near-miss): it is also what a declared ledger round-trips through in +/// every store backend, so `needs_reason` in must equal `needs_reason` out +/// or a save-then-reload silently drops the flag back to `false`. The +/// console-facing camelCase key lives on a wire-only DTO instead +/// (`server::ops::ledgers::LedgerStatusDto`), covered by +/// `ledgers_test.rs`'s `a_status_that_needs_a_reason_carries_camel_case_on_the_wire`. +#[test] +fn status_spec_round_trips_needs_reason_through_its_own_serde_unchanged() { + let spec = parse(&minimal(), false).expect("declaration still parses with needs_reason"); + let status = spec.status("closed").expect("the closed status"); + assert!(status.needs_reason); + + let wire = serde_json::to_value(status).unwrap(); + assert_eq!(wire["needs_reason"], true); + let restored: StatusSpec = serde_json::from_value(wire).unwrap(); + assert!( + restored.needs_reason, + "a store round-trip through this type's own serde must not lose the flag" + ); +} + #[test] fn a_ledger_needs_exactly_one_id_field() { let mut document = minimal(); diff --git a/src/server/ops/ledgers.rs b/src/server/ops/ledgers.rs index 23e49965f..4ab8326c4 100644 --- a/src/server/ops/ledgers.rs +++ b/src/server/ops/ledgers.rs @@ -47,6 +47,35 @@ pub fn router() -> Router { )) } +/// One status, wire-shaped for the console (issue #1266). +/// +/// `crate::ledger::StatusSpec` stays plain snake_case in both directions, +/// because it is also what a declared ledger round-trips through in every +/// store backend — a rename there would make a save write one key and a +/// reload expect another, silently dropping `needs_reason` back to `false`. +/// This DTO exists so the API-output shape can be camelCase (matching every +/// other field `LedgerSummary` sends) without that type doing double duty. +#[derive(Debug, Serialize)] +#[serde(rename_all = "camelCase")] +struct LedgerStatusDto { + name: String, + #[serde(skip_serializing_if = "String::is_empty")] + label: String, + closed: bool, + needs_reason: bool, +} + +impl From<&crate::ledger::StatusSpec> for LedgerStatusDto { + fn from(status: &crate::ledger::StatusSpec) -> Self { + Self { + name: status.name.clone(), + label: status.label.clone(), + closed: status.closed, + needs_reason: status.needs_reason, + } + } +} + /// One ledger as the console lists it. #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] @@ -65,7 +94,7 @@ struct LedgerSummary { /// Whether the runtime ships it. A built-in cannot be retired. builtin: bool, fields: Vec, - statuses: Vec, + statuses: Vec, sections: Vec, #[serde(skip_serializing_if = "Vec::is_empty")] writers: Vec, @@ -211,7 +240,7 @@ async fn summary(ctx: &ledgers::Ledgers, spec: &LedgerSpec) -> Result Date: Thu, 20 Aug 2026 17:43:58 +0530 Subject: [PATCH 09/14] fix: update issue #1189 tests for the runs response envelope The #1189 stranded-approvals tests merged in from upstream still indexed the run history response as a bare array (body[0][...]), a leftover from before #1012 wrapped it in { runs, hasMore }. Point them at body["runs"][0] so they assert against the actual response shape. --- src/server/ops/workflows.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/server/ops/workflows.rs b/src/server/ops/workflows.rs index 992e53960..d95186ad2 100644 --- a/src/server/ops/workflows.rs +++ b/src/server/ops/workflows.rs @@ -6252,7 +6252,7 @@ mod tests { // blocked-node list said "cannot be continued" and the run's own // one-word reading said "go and decide it", on the same row. assert_eq!( - body[0]["verdict"], "stranded", + body["runs"][0]["verdict"], "stranded", "the verdict must be derived after the reconciliation, not before it: {body}" ); } @@ -6341,7 +6341,7 @@ mod tests { stranded: {body}" ); assert_eq!( - body[0]["verdict"], "blocked", + body["runs"][0]["verdict"], "blocked", "a run with a live card is still blocked, not stranded: {body}" ); } @@ -6391,11 +6391,11 @@ mod tests { .unwrap(); let body = json_body(response).await; assert_eq!( - body[0]["strandedApprovals"], 2, + body["runs"][0]["strandedApprovals"], 2, "both gates lost their card, and nothing else on this row says so: {body}" ); assert_eq!( - body[0]["verdict"], "stranded", + body["runs"][0]["verdict"], "stranded", "nothing in the queue is waiting on this run: {body}" ); } @@ -6475,11 +6475,11 @@ mod tests { .unwrap(); let body = json_body(response).await; assert!( - body[0].get("strandedApprovals").is_none(), + body["runs"][0].get("strandedApprovals").is_none(), "the gate's card is on the queue, so nothing may be marked stranded: {body}" ); assert_eq!( - body[0]["verdict"], "awaiting-approval", + body["runs"][0]["verdict"], "awaiting-approval", "a decidable gate is still awaiting a person: {body}" ); } @@ -6527,11 +6527,11 @@ mod tests { .unwrap(); let body = json_body(response).await; assert!( - body[0].get("strandedApprovals").is_none(), + body["runs"][0].get("strandedApprovals").is_none(), "a row with no run id cannot be joined, so it must report nothing: {body}" ); assert_eq!( - body[0]["verdict"], "awaiting-approval", + body["runs"][0]["verdict"], "awaiting-approval", "an unjoinable row keeps the reading it had before #1189: {body}" ); } From 6927810c33fdeb43bef82cb38a02206296bdba60 Mon Sep 17 00:00:00 2001 From: oxoxDev Date: Thu, 20 Aug 2026 17:54:01 +0530 Subject: [PATCH 10/14] fix(mcp): mark StaticTokenRequired dead-code-safe under non-mcp builds The `openhuman,tinycortex` CI lane builds without the `mcp` feature, so refine_oauth_capability's real body (the only place that constructs this variant) compiles out in favor of its no-op stub. Match the existing cfg_attr(not(feature = "..."), allow(dead_code)) idiom used elsewhere in this file's siblings (paypal.rs, rpc.rs, workflows.rs) rather than widening the feature gate. --- src/harness/built_in/mcp_probe.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/harness/built_in/mcp_probe.rs b/src/harness/built_in/mcp_probe.rs index 5b1595125..8dea7495a 100644 --- a/src/harness/built_in/mcp_probe.rs +++ b/src/harness/built_in/mcp_probe.rs @@ -69,6 +69,12 @@ enum FailureKind { /// Distinct from [`FailureKind::OauthRequired`] because the operator's next /// action is the opposite one — paste a static token, rather than press a /// Sign in button that cannot succeed. + /// + /// Only ever constructed by `refine_oauth_capability`, which is itself + /// gated on `feature = "mcp"` (no `mcp` feature means no `oauth/start` + /// route, so there is nothing to downgrade) — so this variant is + /// legitimately unconstructed in builds without that feature. + #[cfg_attr(not(feature = "mcp"), allow(dead_code))] StaticTokenRequired, /// Anything not otherwise recognised. Unknown, From b0e918629c456868ce41e031d563b69970191541 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 20 Aug 2026 18:20:06 +0530 Subject: [PATCH 11/14] fix(harness): a desk must not stand in for a company teammate (#1196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit review: prefer_company_over_baseline classified any non-baseline candidate id as company-side, so a desk (which resolves to neither) could count as "company" and wrongly trigger dropping a genuine baseline teammate from a tie the company never actually resolved. Replaced the boolean classifier with a three-way Provenance (Baseline / Company / neither) so a desk stays neutral on both sides — it neither triggers the drop nor is dropped by it. Co-Authored-By: Claude --- src/harness/built_in/planning.rs | 53 ++++++++++++++++++--------- src/harness/built_in/planning/test.rs | 21 +++++++++-- 2 files changed, 53 insertions(+), 21 deletions(-) diff --git a/src/harness/built_in/planning.rs b/src/harness/built_in/planning.rs index 8946ca3ba..e8640b5eb 100644 --- a/src/harness/built_in/planning.rs +++ b/src/harness/built_in/planning.rs @@ -1884,29 +1884,48 @@ fn resolve_assignee_candidates( /// baseline teammates, or between two company teammates, carries no such /// signal and is left untouched — issue #1106's park-and-ask stands for both. /// -/// A candidate id that does not resolve to a manifest agent (a desk, or an -/// overlay teammate — [`OverlayAgent`](crate::ports::types::OverlayAgent) has -/// no `global` field, so it can never be one) counts as company-side: only a -/// manifest agent explicitly marked `global` is baseline. +/// A candidate id resolves to exactly one of three provenances: a manifest +/// agent marked `global` is [`Baseline`](Provenance::Baseline); a manifest +/// agent that is not, or an overlay teammate — which +/// [`OverlayAgent`](crate::ports::types::OverlayAgent) can never be, having no +/// `global` field at all — is [`Company`](Provenance::Company); anything else +/// `resolve_assignee_candidates` could still have handed back (a desk) is +/// neither. A desk is not the company's own choice of *teammate*, so its mere +/// presence must not stand in for a real one: it neither triggers the drop nor +/// is dropped by it, on either side of the tie. +enum Provenance { + Baseline, + Company, +} + +fn provenance_of(evidence: &Evidence, id: &str) -> Option { + if let Some(agent) = evidence.record.manifest.agents.iter().find(|a| a.id == id) { + return Some(if agent.global { + Provenance::Baseline + } else { + Provenance::Company + }); + } + if evidence.record.overlay_agents.iter().any(|a| a.id == id) { + return Some(Provenance::Company); + } + None +} + fn prefer_company_over_baseline( evidence: &Evidence, candidates: Vec, ) -> Vec { - let is_baseline = |id: &str| { - evidence - .record - .manifest - .agents - .iter() - .find(|a| a.id == id) - .is_some_and(|a| a.global) - }; - let has_company_side = candidates.iter().any(|c| !is_baseline(&c.id)); - let has_baseline = candidates.iter().any(|c| is_baseline(&c.id)); - if has_company_side && has_baseline { + let has_company = candidates + .iter() + .any(|c| matches!(provenance_of(evidence, &c.id), Some(Provenance::Company))); + let has_baseline = candidates + .iter() + .any(|c| matches!(provenance_of(evidence, &c.id), Some(Provenance::Baseline))); + if has_company && has_baseline { candidates .into_iter() - .filter(|c| !is_baseline(&c.id)) + .filter(|c| !matches!(provenance_of(evidence, &c.id), Some(Provenance::Baseline))) .collect() } else { candidates diff --git a/src/harness/built_in/planning/test.rs b/src/harness/built_in/planning/test.rs index cd49b9f7b..4b422d6ec 100644 --- a/src/harness/built_in/planning/test.rs +++ b/src/harness/built_in/planning/test.rs @@ -1857,10 +1857,8 @@ fn prefer_company_over_baseline_drops_only_a_true_mixed_tie() { vec!["maya"] ); - // A teammate and a desk: neither resolves to a baseline agent, so both - // count as company-side and the tie is untouched — #1106's case, and the - // reason an unresolved id (a desk) must default to company-side rather - // than silently misfiring as baseline. + // A teammate and a desk: no baseline teammate in the tie (the desk isn't + // one), so nothing is dropped — #1106's case. let teammate_and_desk = prefer_company_over_baseline(&evidence, vec![candidate("maya"), candidate("studio")]); assert_eq!( @@ -1869,6 +1867,21 @@ fn prefer_company_over_baseline_drops_only_a_true_mixed_tie() { "no baseline teammate in the tie" ); + // A baseline teammate and a desk: a desk is not the company's own choice + // of *teammate*, so its presence must not stand in for one and silently + // knock the real baseline candidate out of a tie nobody actually resolved + // in the company's favour. + let baseline_and_desk = + prefer_company_over_baseline(&evidence, vec![candidate("sam"), candidate("studio")]); + assert_eq!( + baseline_and_desk + .iter() + .map(|c| c.id.as_str()) + .collect::>(), + vec!["sam", "studio"], + "a desk is neutral: it neither triggers the drop nor gets dropped by it" + ); + // A single baseline candidate, alone: nothing to prefer it over. let solo_baseline = prefer_company_over_baseline(&evidence, vec![candidate("sam")]); assert_eq!( From 450e594fe3fb13d251a61df9362db430bc9dc3e3 Mon Sep 17 00:00:00 2001 From: cyrus Date: Thu, 20 Aug 2026 18:21:49 +0530 Subject: [PATCH 12/14] fix: workflow-list-columns e2e mock serves the runs envelope shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The route stub for GET …/workflows/runs still fulfilled with a bare JSON array. Since #1012 wrapped that response as { runs, hasMore }, the console's fetch never found `data.runs` and the list never rendered, timing out both geometry specs in this file (CI: Console E2E and Console E2E (live brain)). --- frontend/test/e2e/workflow-list-columns.spec.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/test/e2e/workflow-list-columns.spec.ts b/frontend/test/e2e/workflow-list-columns.spec.ts index 8a4cd5288..ba38a5ff2 100644 --- a/frontend/test/e2e/workflow-list-columns.spec.ts +++ b/frontend/test/e2e/workflow-list-columns.spec.ts @@ -101,7 +101,11 @@ async function openList(page: Page) { (url) => isRunPage(url), async (route: Route) => { if (route.request().method() !== "GET") return route.fallback(); - await route.fulfill({ status: 200, contentType: "application/json", body: JSON.stringify(RUNS) }); + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ runs: RUNS, hasMore: false }), + }); }, ); await page.addInitScript(() => { From 32fb7cbe83118fdc97d45e6acdc5647d1e3c9ec5 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 20 Aug 2026 18:46:26 +0530 Subject: [PATCH 13/14] fix: auto-approve LocalAcpAgent permission requests, copied from buzz-agent (#1245) LocalAcpAgent previously failed closed on every session/request_permission call it wasn't explicitly configured to allow. Replace that with AutoApprovingFiles, mirroring buzz-agent's handle_permission_request: pick the allow_once-kind option the CLI offered, falling back to reject_once/ reject_always, never a hardcoded optionId. The CLI's own permission mode is the trust boundary, same as running it interactively. Co-Authored-By: Claude --- docs/spec/runtime/harnesses.md | 17 +++--- src-tauri/src/acp/client.rs | 88 ++++++++++++++++++++++++++++++++ src-tauri/src/acp/local_agent.rs | 26 +++++----- src-tauri/tests/acp_client.rs | 23 ++++++++- 4 files changed, 134 insertions(+), 20 deletions(-) diff --git a/docs/spec/runtime/harnesses.md b/docs/spec/runtime/harnesses.md index b1c4a6da1..e0f407755 100644 --- a/docs/spec/runtime/harnesses.md +++ b/docs/spec/runtime/harnesses.md @@ -250,13 +250,16 @@ silent fallback either. - **`[brain].mode`** (`hosted` | `sidecar`) is a separate axis. It selects the cognition seam *within* the built-in harness. - **Tools, policy, budgets, desks.** All company- or agent-scoped, and unchanged - by which engine runs the turn. An ACP agent is still subject to the company's - approval policy — **not yet true for `local`'s own permission prompts** - (`session/request_permission`): `LocalAcpAgent` fails closed on every one it - was not explicitly configured to allow, rather than routing it through - `ApprovalRequestQueue`. Safe (a refusal is a visible, actionable failure; a - silent auto-approval would not be), but a known gap, not the intended - end state. + by which engine runs the turn — **except `local`'s own permission prompts** + (`session/request_permission`), which are not routed through + `ApprovalRequestQueue` at all. `LocalAcpAgent` auto-approves whatever its CLI + still asks about, by option `kind` rather than a configured id, mirroring + `buzz-agent`'s own answer to the same protocol gap + (`crates/buzz-acp/src/acp.rs::handle_permission_request`): the CLI's own + permission mode is the trust boundary, the same as it is for a developer + running that CLI interactively themselves. This is a deliberate choice, not + a placeholder — an ACP-run teammate is not gated by the company's approval + policy the way a `built_in`-run one is. - **Which model an agent's `tier` means.** A tier names a workload and is resolved against whatever provider its harness turns out to use, so an agent keeps its tier when it moves between harnesses. See diff --git a/src-tauri/src/acp/client.rs b/src-tauri/src/acp/client.rs index 21eab4731..e0ee257fa 100644 --- a/src-tauri/src/acp/client.rs +++ b/src-tauri/src/acp/client.rs @@ -135,6 +135,58 @@ impl ClientHandler for ConfinedFiles { } } +/// Wraps another handler's file logic but answers every permission request +/// itself, picking by option `kind` rather than a caller-configured id. +/// +/// Ported from how `buzz-agent` handles the same protocol gap +/// (`crates/buzz-acp/src/acp.rs::handle_permission_request`): finds the +/// option whose `kind` is `allow_once`, falling back to `reject_once` / +/// `reject_always` if the agent offered no allow option at all. Never a +/// hardcoded `optionId` — adapters name their ids however they like, and only +/// `kind` is part of the ACP spec's stable vocabulary. +/// +/// This is `LocalAcpAgent`'s production handler: an ACP agent's own +/// permission-mode config option (the same `session/set_config_option` lever +/// already used for model steering) is meant to keep it from asking at all, +/// and this is the fallback for whatever still does — never a hang, never a +/// silent refusal that reads as the harness doing nothing. +pub struct AutoApprovingFiles { + inner: H, +} + +impl AutoApprovingFiles { + pub fn new(inner: H) -> Self { + Self { inner } + } +} + +#[async_trait::async_trait] +impl ClientHandler for AutoApprovingFiles { + async fn read_text_file(&self, path: &Path) -> Result { + self.inner.read_text_file(path).await + } + + async fn write_text_file(&self, path: &Path, content: &str) -> Result<(), String> { + self.inner.write_text_file(path, content).await + } + + async fn request_permission(&self, _tool_call: &Value, options: &Value) -> String { + let by_kind = |kind: &str| { + options.as_array().and_then(|list| { + list.iter() + .find(|o| o["kind"].as_str() == Some(kind)) + .and_then(|o| o["optionId"].as_str()) + }) + }; + by_kind("allow_once") + .or_else(|| by_kind("reject_once")) + .or_else(|| by_kind("reject_always")) + .map(str::to_string) + // Nothing offered at all: say so rather than inventing an id. + .unwrap_or_else(|| "reject".to_string()) + } +} + type Pending = Arc>>>>; /// One spawned harness. @@ -400,3 +452,39 @@ async fn serve(method: &str, params: &Value, handler: &dyn ClientHandler) -> Res other => Err(format!("unsupported client method: {other}")), } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::acp::confine::Confinement; + + fn auto_approving(root: &std::path::Path) -> AutoApprovingFiles { + AutoApprovingFiles::new(ConfinedFiles::new(Confinement::new(root).unwrap(), None)) + } + + #[tokio::test] + async fn falls_back_to_reject_once_when_the_agent_offers_no_allow_option() { + let dir = tempfile::tempdir().unwrap(); + let handler = auto_approving(dir.path()); + let options = json!([ + { "optionId": "n1", "name": "Reject", "kind": "reject_once" }, + { "optionId": "n2", "name": "Reject always", "kind": "reject_always" }, + ]); + + assert_eq!( + handler.request_permission(&Value::Null, &options).await, + "n1" + ); + } + + #[tokio::test] + async fn falls_back_to_the_literal_reject_when_the_agent_offers_nothing_to_pick() { + let dir = tempfile::tempdir().unwrap(); + let handler = auto_approving(dir.path()); + + assert_eq!( + handler.request_permission(&Value::Null, &json!([])).await, + "reject" + ); + } +} diff --git a/src-tauri/src/acp/local_agent.rs b/src-tauri/src/acp/local_agent.rs index 19760ae04..99599b3f6 100644 --- a/src-tauri/src/acp/local_agent.rs +++ b/src-tauri/src/acp/local_agent.rs @@ -12,16 +12,16 @@ //! arrives, and a `prompt` call drains only its own session's buffer after //! `session/prompt` returns rather than reading whatever the sink last saw. //! -//! ## Permission requests: fails closed (deliberate, and a known gap) +//! ## Permission requests: copied from `buzz-agent`, not bridged to the queue //! -//! `docs/spec/runtime/harnesses.md` says an ACP agent "is still subject to -//! the company's approval policy" — this does not yet route ACP permission -//! requests through that policy gate; it refuses every one it did not -//! explicitly configure to allow, via the same [`ConfinedFiles`] the fixture -//! tests already use. That is the safe direction to be wrong in: a refused -//! edit is a visible failure the operator can act on, where a silently -//! auto-approved one would not be. Wiring ACP's `session/request_permission` -//! into `ApprovalRequestQueue` is real follow-up work, not done here. +//! An earlier draft routed ACP `session/request_permission` calls through +//! `ApprovalRequestQueue` and, until that landed, refused every request by +//! default. `buzz-agent` (`crates/buzz-acp`) answers a much simpler question +//! instead — trust the CLI's own permission mode, and auto-approve whatever +//! it still asks about — and that is what this does too, via +//! [`AutoApprovingFiles`]. There is no human-approval queue in the loop here; +//! an ACP-run teammate's own CLI is the trust boundary, the same as it is for +//! a developer running that CLI interactively themselves. use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -35,7 +35,7 @@ use opencompany::ports::types::CompanyId; use serde_json::Value; use tokio::sync::Mutex as AsyncMutex; -use crate::acp::client::{AcpClient, ClientHandler, ConfinedFiles}; +use crate::acp::client::{AcpClient, AutoApprovingFiles, ClientHandler, ConfinedFiles}; use crate::acp::confine::Confinement; use crate::acp::discovery::HARNESSES; @@ -120,8 +120,10 @@ impl LocalAcpAgent { })?; let confinement = Confinement::new(&self.workspace_root) .map_err(|error| OpenCompanyError::Config(format!("acp workspace: {error}")))?; - // V1 fails closed — see the module docs. - let handler: Arc = Arc::new(ConfinedFiles::new(confinement, None)); + // Auto-approves permission requests by kind — see the module docs. + let handler: Arc = Arc::new(AutoApprovingFiles::new( + ConfinedFiles::new(confinement, None), + )); let pending = Arc::clone(&self.pending_updates); let sink: crate::acp::client::UpdateSink = Arc::new(move |update: Value| { diff --git a/src-tauri/tests/acp_client.rs b/src-tauri/tests/acp_client.rs index bcc95e72c..2fa93fd45 100644 --- a/src-tauri/tests/acp_client.rs +++ b/src-tauri/tests/acp_client.rs @@ -10,7 +10,9 @@ use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; -use opencompany_desktop_lib::acp::client::{AcpClient, AcpError, ClientHandler, ConfinedFiles}; +use opencompany_desktop_lib::acp::client::{ + AcpClient, AcpError, AutoApprovingFiles, ClientHandler, ConfinedFiles, +}; use opencompany_desktop_lib::acp::confine::Confinement; use serde_json::Value; @@ -230,6 +232,25 @@ async fn an_option_the_agent_never_offered_is_not_echoed_back() { assert_eq!(updates.said(), "chose:no"); } +#[tokio::test] +async fn auto_approving_files_picks_the_allow_once_option_unprompted() { + // `LocalAcpAgent`'s production handler (issue #1245) — the opposite of + // `permission_defaults_to_refusing_rather_than_allowing` above by design: + // no configured id at all, yet it still answers "yes", because it looks + // at `kind` rather than needing one told to it. + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap(); + let handler: Arc = Arc::new(AutoApprovingFiles::new(ConfinedFiles::new( + Confinement::new(&root).unwrap(), + None, + ))); + let (client, updates) = connect(&root, handler).await; + let session = client.new_session(&root).await.unwrap(); + + client.prompt(&session, "ask").await.unwrap(); + assert_eq!(updates.said(), "chose:yes"); +} + #[tokio::test] async fn a_harness_that_dies_mid_turn_fails_the_caller_instead_of_hanging() { // An ordinary event — a crash, an OOM kill — and the caller has to hear From f81ea96fd905ce1c42ca8f7baed4bc463902e98c Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 20 Aug 2026 19:02:52 +0530 Subject: [PATCH 14/14] feat: expose ACP harness readiness as a Tauri command (#1245) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not yet wired into generate_handler! or called by the frontend — this is the read-only probe (acp::discovery::survey) on its own IPC surface, ready for the UI work that consumes it. Co-Authored-By: Claude --- src-tauri/src/commands.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index d479f47b7..3b2bc4b30 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -398,6 +398,19 @@ pub async fn oc_forget_local_instance( local.forget(&id) } +/// Every coding harness this shell knows how to drive over ACP, and whether +/// each is actually usable right now. +/// +/// Takes no state and no connection id: unlike everything else in this file, +/// readiness is a property of *this machine*, not of a host it talks to. The +/// probe reads `PATH` and the credential files each harness keeps under the +/// user's home — see `acp::discovery`'s module docs for why that is checked by +/// file rather than by starting the harness. +#[tauri::command] +pub fn oc_acp_harnesses() -> Vec { + crate::acp::discovery::survey(&crate::acp::discovery::SystemProbe) +} + #[cfg(test)] mod test { use std::sync::Arc;