diff --git a/app/test/playwright/specs/settings-profiles-crud.spec.ts b/app/test/playwright/specs/settings-profiles-crud.spec.ts index 4f764ed138..fdbccf3247 100644 --- a/app/test/playwright/specs/settings-profiles-crud.spec.ts +++ b/app/test/playwright/specs/settings-profiles-crud.spec.ts @@ -208,3 +208,135 @@ test.describe('Agent profiles — activate and delete', () => { await expect(page.getByLabel('ID', { exact: true })).toHaveCount(0); }); }); + +/** + * Failure paths (#5944 / #5900). + * + * Every test above is a happy path, which is how `[object Object]` shipped: + * `dispatch(thunk).unwrap()` rejects with Redux Toolkit's `SerializedError` — a + * plain object, never an `Error` — so the old `err instanceof Error` guard took + * its `String(err)` branch and stringified an object. The defect was even pinned + * as *expected* in a jsdom test before #5944 inverted it. + * + * These drive the real panels against a real core with only the one failing + * method stubbed, so the whole chain the fix lives on runs: + * `core RPC error → CoreRpcError → thunk rejection → SerializedError → + * errorMessage() → rendered text`. `coreRpcClient.ts:855-859` puts the JSON-RPC + * `error.message` on `CoreRpcError` verbatim, so the string asserted here is the + * one the backend sent. + * + * Each asserts the message IS shown *and* that `[object Object]` is NOT — the + * first fails on a regression, the second names the specific defect. + */ +test.describe('Agent profiles — a failing action shows the reason', () => { + test.describe.configure({ mode: 'serial' }); + const NAME = 'W6 Failure Profile'; + const ID = 'w6-failure-profile'; + + /** + * Fail exactly one RPC method with a JSON-RPC error, passing everything else + * through to the real core. + * + * The message is deliberately bland: `classifyRpcError` re-routes anything + * that looks like an auth, timeout or not-found failure, and a reclassified + * error would take a different path through the UI than the one under test. + */ + const failMethod = async (page: Page, method: string, message: string) => { + await page.route('**/rpc', async (route, request) => { + const body = JSON.parse(request.postData() || '{}'); + if (body.method === method) { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + jsonrpc: '2.0', + id: body.id, + error: { code: -32000, message }, + }), + }); + return; + } + await route.continue(); + }); + }; + + test.afterEach(async () => { + await deleteFromCore(ID); + }); + + test('a failing save shows the backend reason, not [object Object]', async ({ page }) => { + const REASON = 'the core declined to store this profile'; + await deleteFromCore(ID); + await bootAuthenticatedPage(page, 'pw-w6-profile-fail', '/settings/profiles'); + await openProfiles(page); + await failMethod(page, 'openhuman.profiles_upsert', REASON); + + await page.getByRole('button', { name: 'New profile' }).click(); + await expect(page.getByLabel('Name', { exact: true })).toBeVisible({ timeout: 30_000 }); + await page.getByLabel('Name', { exact: true }).click(); + await page.keyboard.type(NAME); + await page.getByRole('button', { name: 'Create' }).click(); + + const alert = page.getByRole('alert'); + await expect(alert).toContainText(REASON, { timeout: 30_000 }); + await expect(alert).not.toContainText('[object Object]'); + + // The editor must stay open on failure — navigating back to the list would + // discard what the user typed on an error they can act on. + await expect(page.getByLabel('Name', { exact: true })).toHaveValue(NAME); + }); + + test('a failing Set as active shows the backend reason, not [object Object]', async ({ + page, + }) => { + const REASON = 'the core declined to switch profile'; + await deleteFromCore(ID); + await callCoreRpc('openhuman.profiles_upsert', { + profile: { + id: ID, + name: NAME, + description: 'Seeded over RPC so the list starts from a known state.', + agentId: 'orchestrator', + builtIn: false, + includeAgentConversations: true, + }, + }).catch(() => {}); + await bootAuthenticatedPage(page, 'pw-w6-profile-fail-select', '/settings/profiles'); + await openProfiles(page); + await failMethod(page, 'openhuman.profiles_select', REASON); + + await row(page, NAME).getByText('Set as active').click(); + + // ProfilesPanel renders `actionError` as a plain styled

, not an Alert, + // so this is located by its text rather than by role. + await expect(page.getByText(REASON)).toBeVisible({ timeout: 30_000 }); + await expect(page.getByText('[object Object]')).toHaveCount(0); + }); + + test('a failing delete shows the backend reason, not [object Object]', async ({ page }) => { + const REASON = 'the core declined to remove this profile'; + await deleteFromCore(ID); + await callCoreRpc('openhuman.profiles_upsert', { + profile: { + id: ID, + name: NAME, + description: 'Seeded over RPC so the list starts from a known state.', + agentId: 'orchestrator', + builtIn: false, + includeAgentConversations: true, + }, + }).catch(() => {}); + await bootAuthenticatedPage(page, 'pw-w6-profile-fail-delete', '/settings/profiles'); + await openProfiles(page); + await failMethod(page, 'openhuman.profiles_delete', REASON); + + page.once('dialog', d => void d.accept()); + await row(page, NAME).getByText('Delete').click(); + + await expect(page.getByText(REASON)).toBeVisible({ timeout: 30_000 }); + await expect(page.getByText('[object Object]')).toHaveCount(0); + + // The delete failed, so the profile must still be there. + await expect(row(page, NAME)).toBeVisible(); + }); +}); diff --git a/app/test/playwright/specs/token-usage-load-failure.spec.ts b/app/test/playwright/specs/token-usage-load-failure.spec.ts new file mode 100644 index 0000000000..e73eda9ff3 --- /dev/null +++ b/app/test/playwright/specs/token-usage-load-failure.spec.ts @@ -0,0 +1,111 @@ +import { expect, type Page, test } from '@playwright/test'; + +import { + bootAuthenticatedPage, + dismissWalkthroughIfPresent, + waitForAppReady, +} from '../helpers/core-rpc'; + +/** + * Token & Cost panel — what the compression switches do when their settings + * never arrive (#5925). + * + * Two separate properties, and before #5925 the panel got both wrong: + * + * 1. **A settings failure must disable the switches.** `settings` is `null`, so + * every switch fell back to `checked={settings?.x ?? false}` and rendered + * unchecked but *live*. Toggling one sent a `tokenjuice_settings_update` + * built on nothing, silently writing `false` over whatever the user had. + * 2. **A savings failure must NOT disable them.** The old loader awaited both + * calls in one `Promise.all`, so a failure in the display-only savings + * figure threw away the settings too and took the whole configuration + * surface down with it. The two are now loaded independently. + * + * No spec in any lane reached this panel before. The jsdom suite added with + * #5925 covers the same ground with a mocked transport; this drives the real + * panel in a browser with only the failing RPC stubbed. + */ + +const USAGE_TAB = '/#/connections?tab=usage#tokens'; + +/** English labels — the browser lane has i18n loaded, so `t()` has resolved. */ +const COMPRESSION_SWITCH = 'Enable compression'; +const SEARCH_SWITCH = 'Search results'; + +/** + * Fail exactly one RPC method, passing every other call through to the real + * core, so only the property under test is perturbed. + */ +async function failMethod(page: Page, method: string, message: string) { + await page.route('**/rpc', async (route, request) => { + const body = JSON.parse(request.postData() || '{}'); + if (body.method === method) { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + jsonrpc: '2.0', + id: body.id, + error: { code: -32000, message }, + }), + }); + return; + } + await route.continue(); + }); +} + +async function openUsageTab(page: Page) { + await page.goto(USAGE_TAB); + await waitForAppReady(page); + await dismissWalkthroughIfPresent(page); +} + +test.describe('Token & Cost — settings that fail to load', () => { + test('a settings failure disables every compression switch', async ({ page }) => { + await bootAuthenticatedPage(page, 'pw-w6-tokenusage-settings-fail', '/connections?tab=usage'); + await failMethod( + page, + 'openhuman.tokenjuice_settings_get', + 'the core could not read compression settings' + ); + await openUsageTab(page); + + const compression = page.getByRole('switch', { name: COMPRESSION_SWITCH }); + await expect(compression).toBeVisible({ timeout: 30_000 }); + + // The assertion #5925 added. Without `disabled={settings === null}` the + // switch renders unchecked and fully interactive, and clicking it patches + // the backend from a settings object that was never loaded. + await expect(compression).toBeDisabled(); + await expect(page.getByRole('switch', { name: SEARCH_SWITCH })).toBeDisabled(); + + // Every switch on the panel, not just the two named above: a partial fix + // that missed one would leave exactly the hazard this closes. + const switches = page.getByRole('switch'); + const count = await switches.count(); + expect(count).toBeGreaterThan(0); + for (let i = 0; i < count; i += 1) { + await expect(switches.nth(i)).toBeDisabled(); + } + }); + + test('a savings failure leaves the compression switches usable', async ({ page }) => { + await bootAuthenticatedPage(page, 'pw-w6-tokenusage-savings-fail', '/connections?tab=usage'); + await failMethod( + page, + 'openhuman.tokenjuice_savings_stats', + 'the core could not read savings statistics' + ); + await openUsageTab(page); + + const compression = page.getByRole('switch', { name: COMPRESSION_SWITCH }); + await expect(compression).toBeVisible({ timeout: 30_000 }); + + // The half the old `Promise.all` broke: settings loaded fine, so the + // configuration controls must stay interactive even though the savings + // figure beside them could not be fetched. + await expect(compression).toBeEnabled(); + await expect(page.getByRole('switch', { name: SEARCH_SWITCH })).toBeEnabled(); + }); +}); diff --git a/src/openhuman/tools/impl/filesystem/git_operations.rs b/src/openhuman/tools/impl/filesystem/git_operations.rs index 0186ce495e..1fbcd7dd5c 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations.rs @@ -174,7 +174,20 @@ impl GitOperationsTool { // Validate files argument against injection patterns self.sanitize_git_args(files)?; - let mut git_args = vec!["diff", "--unified=3"]; + // `--no-ext-diff` is where external-diff suppression has to live: it is + // a diff-command flag, and the `-c diff.external=` that used to stand in + // for it made git exec the empty string instead of disabling anything. + // + // `--no-textconv` is a SEPARATE mechanism and needs its own flag. + // `--no-ext-diff` covers `diff.external` and `diff..command`; + // it does not touch `diff..textconv`, which a `.gitattributes` + // line (`*.bin diff=evil`) can select and which git then EXECUTES to + // render a binary file as text. Verified against a scratch repo: with + // `--no-ext-diff` alone the textconv script ran; adding + // `--no-textconv` it did not. `hardened_git`'s `-c` list cannot close + // this — driver names are arbitrary, so there is no finite set of keys + // to neutralise, and the suppression has to be a command flag. + let mut git_args = vec!["diff", "--no-ext-diff", "--no-textconv", "--unified=3"]; if cached { git_args.push("--cached"); } diff --git a/src/openhuman/tools/impl/filesystem/git_operations_config.rs b/src/openhuman/tools/impl/filesystem/git_operations_config.rs index d4143a1b9a..20f11429f1 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations_config.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations_config.rs @@ -125,7 +125,14 @@ pub(super) const NEUTRALISED_CONFIG: &[&str] = &[ "core.sshCommand=", "core.pager=cat", "core.editor=false", - "diff.external=", + // NOT `diff.external=`. An empty value does not disable an external diff — + // git tries to *execute* the empty string and the whole command dies with + // `error: cannot run : No such file or directory` / `fatal: external diff + // died`, so every `diff` operation failed rather than being hardened. + // Suppression belongs on the command instead: `git diff --no-ext-diff`, + // which ignores `diff.external` however the repository set it. Verified + // both ways against a repo with `diff.external=/bin/false`: plain `diff` + // dies, `--no-ext-diff` prints the patch. "sequence.editor=false", "uploadpack.packObjectsHook=", ]; diff --git a/src/openhuman/tools/impl/filesystem/git_operations_config_tests.rs b/src/openhuman/tools/impl/filesystem/git_operations_config_tests.rs index 3cdcc4e8d6..ddb30306ff 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations_config_tests.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations_config_tests.rs @@ -463,3 +463,81 @@ fn a_subsection_is_elided_so_one_entry_covers_every_remote() { // A key with no dot at all is returned unchanged rather than panicking. assert_eq!(normalise_config_key("bare"), "bare"); } + +// ── External diff suppression ───────────────────────────────────────────── + +/// An ordinary repository's `diff` must produce its patch. +/// +/// That reads like it could not possibly regress, and it did. Suppression of +/// an external diff was attempted with `-c diff.external=` in +/// `NEUTRALISED_CONFIG`, and an empty value does not disable one — git tries +/// to *execute* the empty string, so **every** diff died with +/// `error: cannot run : No such file or directory` / +/// `fatal: external diff died`, on every repository, hostile or not. The +/// hardening removed the operation instead of hardening it. `--no-ext-diff` +/// on the diff command is the real suppression. +/// +/// This lives in the lib suite deliberately. The integration test that caught +/// it sits in `raw_coverage_all`, and a change to `git_operations.rs` maps to +/// the `openhuman::tools` libtest filter — which never selects that target. So +/// the lane the change picks did not run the test covering the change, and the +/// breakage sat until an unrelated PR happened to select the other filter. +/// A test here runs whenever this file is touched. +/// +/// A repository that *sets* `diff.external` is a separate matter and is +/// already refused before reaching the invocation: the key is on neither +/// allowlist, so `first_disallowed_repo_config_key` rejects the repository +/// outright. `--no-ext-diff` is the second layer, covering the gap between +/// that inspection and the command. +#[tokio::test] +async fn an_ordinary_repository_still_produces_a_diff() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + + // A committer identity has to be set in the repository itself. `hermetic` + // closes the global and system config, which is the point of it — so on a + // CI container with no identity of its own `git commit` fails with + // "Author identity unknown". Both keys are on `ALLOWED_REPO_CONFIG`, so + // setting them does not trip the repository-config refusal. + set_config(tmp.path(), "user.email", "test@example.invalid"); + set_config(tmp.path(), "user.name", "Test"); + + let tracked = tmp.path().join("tracked.txt"); + std::fs::write(&tracked, "first\n").unwrap(); + // `hermetic` closes the ambient git config the way the fixtures do — + // without it a developer's global `commit.gpgsign` or hooks path can fail + // this commit for reasons unrelated to the test. + // Slices, not arrays: `["add", "tracked.txt"]` and `["commit", "-m", "one"]` + // are `[&str; 2]` and `[&str; 3]`, which are different types and cannot + // share an array literal. + for args in [&["add", "tracked.txt"][..], &["commit", "-m", "one"][..]] { + let ok = hermetic( + std::process::Command::new("git") + .args(args) + .current_dir(tmp.path()), + ) + .status() + .unwrap() + .success(); + assert!(ok, "failed to run `git {}` in the test workspace", args[0]); + } + std::fs::write(&tracked, "first\nsecond\n").unwrap(); + + let tool = test_tool(tmp.path()); + let result = tool + .execute(json!({"operation": "diff", "files": "tracked.txt"})) + .await + .expect("a diff on a plain repository must not error out"); + + assert!(!result.is_error, "got: {}", result.output()); + assert!( + result.output().contains("second"), + "the added line must appear in the patch: {}", + result.output() + ); + assert!( + !result.output().contains("external diff died"), + "git must not be handed an empty `diff.external` to execute: {}", + result.output() + ); +} diff --git a/tests/json_rpc_e2e.rs b/tests/json_rpc_e2e.rs index 61f97d1884..93a6ece86b 100644 --- a/tests/json_rpc_e2e.rs +++ b/tests/json_rpc_e2e.rs @@ -14099,3 +14099,68 @@ async fn memory_flavour_agent_tool_e2e_5172() { .expect_err("an unrecognized flavour slug must be rejected"); assert!(unknown.to_string().contains("Unknown flavour")); } + +/// The `memory_diff` RPC surface is gone, and the `memory` domain is not (#5839). +/// +/// #5839 deleted the `memory-git` feature, `src/openhuman/memory/diff/`, the +/// `memory_diff` tool and `tests/memory_artifacts_e2e.rs`. What it left behind +/// is a unit test over `all_controller_schemas()` +/// (`src/core/all_tests.rs::memory_diff_controllers_are_gone_and_memory_survives`), +/// which reads the registry as a data structure. Nothing dispatched a removed +/// method through the live router, so nothing proved the wire surface actually +/// went with it — a re-registration behind a different namespace, or a stale +/// alias, would satisfy that unit test and still answer on `/rpc`. +/// +/// This asks the real HTTP router. Both halves matter and are deliberately in +/// one test: removing the git ledger must not take the memory domain with it, +/// so a change that deleted too much fails here rather than passing quietly. +#[tokio::test] +async fn json_rpc_memory_diff_surface_is_gone_and_memory_still_answers() { + let _env_lock = json_rpc_e2e_env_lock(); + let tmp = tempdir().expect("tempdir"); + let home = tmp.path(); + let openhuman_home = home.join(".openhuman"); + + let _home_guard = EnvVarGuard::set_to_path("HOME", home); + let _workspace_guard = EnvVarGuard::unset("OPENHUMAN_WORKSPACE"); + let _backend_url_guard = EnvVarGuard::unset("BACKEND_URL"); + let _vite_backend_guard = EnvVarGuard::unset("VITE_BACKEND_URL"); + + write_min_config(&openhuman_home, "http://127.0.0.1:9"); + + let (rpc_addr, rpc_join) = serve_on_ephemeral(build_core_http_router(false)).await; + let rpc_base = format!("http://{rpc_addr}"); + + // Every function the deleted `memory_diff` namespace used to register. + for (id, method) in [ + "openhuman.memory_diff_take_snapshot", + "openhuman.memory_diff_create_checkpoint", + "openhuman.memory_diff_diff_since_checkpoint", + "openhuman.memory_diff_diff_since_last", + "openhuman.memory_diff_diff_since_read", + "openhuman.memory_diff_mark_read", + ] + .into_iter() + .enumerate() + { + let response = post_json_rpc(&rpc_base, 5_839_000 + id as i64, method, json!({})).await; + assert_unknown_method(&response, method); + } + + // The other half: the memory domain survived the removal. `memory_init` is + // dispatched here purely to prove the namespace still answers — any + // response but unknown-method is a pass, because what is under test is + // registration, not this call's own outcome. + let survivor = post_json_rpc(&rpc_base, 5_839_100, "openhuman.memory_init", json!({})).await; + let unknown = survivor + .get("error") + .and_then(|error| error.get("message")) + .and_then(Value::as_str) + .is_some_and(|message| message.contains("unknown method")); + assert!( + !unknown, + "removing the git ledger must not deregister the memory domain: {survivor}" + ); + + rpc_join.abort(); +} diff --git a/tests/mcp_registry_e2e.rs b/tests/mcp_registry_e2e.rs index 913a0544c6..7a379ba982 100644 --- a/tests/mcp_registry_e2e.rs +++ b/tests/mcp_registry_e2e.rs @@ -755,3 +755,98 @@ async fn per_config_lookups_see_a_per_config_connection() { assert!(connections::disconnect_for_config(&cfg_a, &server.server_id).await); assert!(!connections::is_connected_for_config(&cfg_a, &server.server_id).await); } + +// ── Probe outcomes (#5772) ──────────────────────────────────────────────────── + +/// `probe_alive` returns a four-variant `ProbeOutcome` instead of a bool, and +/// `probe_alive_reflects_transport_liveness` above only ever asks +/// `.is_alive()` — which is `false` for `Missing`, `Broken` and `TimedOut` +/// alike, so nothing pinned which one comes back. +/// +/// This pins the two that are reachable in a hermetic test: an entry that was +/// never connected, and one that has been disconnected, must both report +/// `Missing` — "there was nothing to probe" — rather than a transport failure +/// nobody observed. That distinction is what the old bool could not express and +/// is the half of #5636 this harness can actually demonstrate. +#[tokio::test] +async fn probe_alive_distinguishes_a_missing_entry_from_a_timed_out_one() { + let (_tmp, cfg) = fresh_workspace_config(); + let h = host(&cfg); + let server = make_installed_server(); + h.dynamic() + .store() + .insert_server(&server) + .expect("insert installed server"); + + // Installed but never connected: there is no live entry, so there is + // nothing to probe. + assert_eq!( + h.dynamic() + .connections() + .probe_alive(&server.server_id, std::time::Duration::from_secs(8)) + .await, + tinymcp::ProbeOutcome::Missing, + "a server that was never connected has no entry to probe" + ); + + h.dynamic() + .connect(&server.server_id) + .await + .expect("connect") + .tools; + + // Connected and answering: a real round trip inside a real window. + match h + .dynamic() + .connections() + .probe_alive(&server.server_id, std::time::Duration::from_secs(8)) + .await + { + tinymcp::ProbeOutcome::Alive { .. } => {} + other => panic!("a live stub must probe Alive, got {other:?}"), + } + + // NOT asserted here: `TimedOut`. It is the variant this enum most exists + // for, and it is unreachable against this stub — a probe with a + // `Duration::ZERO` window still came back `Alive { elapsed: 42.708µs }`, + // because `tokio::time::timeout` polls the inner future before it checks + // the deadline and `list_tools` on an established connection answers inside + // that first poll. Producing a real timeout needs a server that stalls on + // `tools/list`, which `test-mcp-stub` cannot be asked to do. Asserting it + // by contriving the window would have pinned nothing; see + // ~/tinyhuman/bugs/W6-test-findings.md. + + // Disconnecting removes the entry, so the outcome goes back to Missing + // rather than to a transport error. + h.dynamic() + .connections() + .disconnect(&server.server_id) + .await; + assert_eq!( + h.dynamic() + .connections() + .probe_alive(&server.server_id, std::time::Duration::from_secs(8)) + .await, + tinymcp::ProbeOutcome::Missing, + "a disconnected server has no entry, so nothing was observed to fail" + ); +} + +/// The supervisor's default probe window is 8s. +/// +/// tinymcp#5 widened it to 30s and paired that with `MissedTickBehavior::Delay` +/// in `Supervisor::run` — which this host never calls: `mcp/registry/mod.rs` +/// builds its own interval and calls `Supervisor::tick` directly, once per open +/// workspace. openhuman would have inherited the wider window with none of the +/// protection, so `b44b958d` put the default back. Nothing pinned it, and the +/// value is only reachable through `SupervisorConfig::default()` — exactly what +/// `supervise_once` above uses. +#[test] +fn supervisor_default_probe_window_stays_eight_seconds() { + assert_eq!( + tinymcp::SupervisorConfig::default().probe_timeout, + std::time::Duration::from_secs(8), + "widening this default without a missed-tick policy in the host's own loop is the \ + regression b44b958d reverted" + ); +} diff --git a/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs b/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs index 48c2d14105..e0a0dcb937 100644 --- a/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs +++ b/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs @@ -1,5 +1,11 @@ use anyhow::Result; use async_trait::async_trait; +use openhuman_core::openhuman::agent::context::prompt::{ + render_ambient_environment, render_subagent_system_prompt, render_tools, render_user_files, + ConnectedIntegration, CuratedMemoryPromptSnapshot, LearnedContextData, NamespaceSummary, + PersonalityRosterEntry, PromptContext, PromptTool, SubagentRenderOptions, SystemPromptBuilder, + ToolCallFormat, UserIdentity, +}; use openhuman_core::openhuman::agent::dispatcher::NativeToolDispatcher; use openhuman_core::openhuman::agent::harness::definition::AgentTier; use openhuman_core::openhuman::agent::harness::session::Agent; @@ -8,16 +14,10 @@ use openhuman_core::openhuman::agent::harness::{ ParentExecutionContext, PromptSource, SandboxMode, SubagentRunOptions, ToolScope, }; use openhuman_core::openhuman::config::AgentConfig; -use openhuman_core::openhuman::agent::context::prompt::{ - render_ambient_environment, render_subagent_system_prompt, render_tools, render_user_files, - ConnectedIntegration, CuratedMemoryPromptSnapshot, LearnedContextData, NamespaceSummary, - PersonalityRosterEntry, PromptContext, PromptTool, SubagentRenderOptions, SystemPromptBuilder, - ToolCallFormat, UserIdentity, -}; +use openhuman_core::openhuman::inference::tokenjuice::AgentTokenjuiceCompression; use openhuman_core::openhuman::memory::{ Memory, MemoryCategory, MemoryEntry, NamespaceSummary as MemoryNamespaceSummary, RecallOpts, }; -use openhuman_core::openhuman::inference::tokenjuice::AgentTokenjuiceCompression; use openhuman_core::openhuman::tools::{PermissionLevel, Tool, ToolContent, ToolResult}; use parking_lot::Mutex; use serde_json::json; @@ -248,7 +248,7 @@ fn tool_response(id: &str, name: &str, arguments: serde_json::Value) -> ModelRes raw: None, resolved_model: None, continue_turn: None, - served_from_cache: false, + served_from_cache: false, } } @@ -363,9 +363,8 @@ fn parent_context(workspace: PathBuf, provider: Arc) -> ParentExe ] .into_iter() .collect(), - turn_model_source: openhuman_core::openhuman::agent::tinyagents::TurnModelSource::from_model( - provider, - ), + turn_model_source: + openhuman_core::openhuman::agent::tinyagents::TurnModelSource::from_model(provider), all_tools: Arc::new(tools), all_tool_specs: Arc::new(specs), visible_tool_names: std::collections::HashSet::new(), @@ -705,3 +704,192 @@ fn subagent_prompt_renderer_handles_formats_caps_and_stale_tool_indices() -> Res assert!(native_prompt.contains("native tool-calling output")); Ok(()) } + +// ── Turn dispatch guard (#5810) ──────────────────────────────────────────────── +// +// `run_subagent` consults `turn_dispatch_guard::check()` as its first statement +// and refuses two ways: a graceful pause already requested at the model-call +// cap, and less wall-clock remaining than this turn's slowest completed child. +// +// Both cases below install a REAL guard around the call — the gate is a no-op +// outside a turn scope, so a test that skips `with_dispatch_guard` exercises +// nothing. Each asserts on the refusal AND on the provider request count: the +// refusal is meant to cost nothing, so a gate that let the dispatch reach the +// model before erroring would still be a defect. Each also drives an ALLOWED +// dispatch through the same guard first, so a gate that refused unconditionally +// could not pass either test. + +#[tokio::test] +async fn dispatch_is_refused_once_the_turn_has_requested_a_cap_pause() -> Result<()> { + let tmp = TempDir::new()?; + let provider = ScriptedModel::new(vec![text_response("first child answer")]); + let provider_handle = provider.clone(); + let parent = parent_context(tmp.path().to_path_buf(), provider); + + let outcome = with_parent_context(parent, async { + // No ceiling, so the budget gate can never fire here and the only thing + // under test is the pause. + openhuman_core::openhuman::agent::harness::turn_dispatch_guard::with_dispatch_guard( + None, + async { + // Control: inside the guard, with nothing recorded, a dispatch + // must still go through. Without this a gate that refused every + // call would satisfy the assertion below. + let allowed = run_subagent( + &definition(None), + "before the cap", + SubagentRunOptions::default(), + ) + .await; + + let state = + openhuman_core::openhuman::agent::harness::turn_dispatch_guard::current() + .expect("the guard is installed for this turn"); + state.record_pause_requested(15, 15); + + let refused = run_subagent( + &definition(None), + "after the cap", + SubagentRunOptions { + task_id: Some("post-pause-dispatch".to_string()), + ..SubagentRunOptions::default() + }, + ) + .await; + + (allowed, refused) + }, + ) + .await + }) + .await; + + let (allowed, refused) = outcome; + assert_eq!( + allowed + .expect("a dispatch before the pause must be allowed") + .output, + "first child answer", + "the guard must not refuse before a pause is recorded" + ); + + match refused { + Err(openhuman_core::openhuman::agent::harness::SubagentRunError::PauseRequested { + completed_model_calls, + cap, + }) => { + assert_eq!(completed_model_calls, 15); + assert_eq!(cap, 15); + } + other => panic!( + "a dispatch after the cap pause must be refused with PauseRequested, got: {other:?}" + ), + } + + // The refusal is pre-dispatch: only the first (allowed) child may have + // reached the provider. A second request means the gate ran too late to + // stop the work it exists to stop. + assert_eq!( + provider_handle.requests().len(), + 1, + "the refused dispatch must not reach the provider at all" + ); + Ok(()) +} + +#[tokio::test] +async fn dispatch_is_refused_when_less_budget_remains_than_the_slowest_child() -> Result<()> { + let tmp = TempDir::new()?; + let provider = ScriptedModel::new(vec![text_response("fast child answer")]); + let provider_handle = provider.clone(); + let parent = parent_context(tmp.path().to_path_buf(), provider); + + let outcome = with_parent_context(parent, async { + // A generous ceiling, so `remaining` stays far above the sample the + // control records and only the deliberate one below can trip the gate. + openhuman_core::openhuman::agent::harness::turn_dispatch_guard::with_dispatch_guard( + Some(std::time::Duration::from_secs(3600)), + async { + // Control: a budget of an hour against a one-millisecond + // observed maximum must still allow a dispatch. + openhuman_core::openhuman::agent::harness::turn_dispatch_guard::record_subagent_elapsed( + std::time::Duration::from_millis(1), + ); + let allowed = run_subagent( + &definition(None), + "while budget remains", + SubagentRunOptions::default(), + ) + .await; + + // Now fold in a child that took far longer than the whole + // ceiling. `remaining` is at most an hour; the observed maximum + // is a hundred, so the refusal is a fact rather than a race. + openhuman_core::openhuman::agent::harness::turn_dispatch_guard::record_subagent_elapsed( + std::time::Duration::from_secs(360_000), + ); + let refused = run_subagent( + &definition(None), + "after the budget is gone", + SubagentRunOptions { + task_id: Some("over-budget-dispatch".to_string()), + ..SubagentRunOptions::default() + }, + ) + .await; + + (allowed, refused) + }, + ) + .await + }) + .await; + + let (allowed, refused) = outcome; + assert_eq!( + allowed + .expect("a dispatch with budget to spare must be allowed") + .output, + "fast child answer", + "the guard must not refuse while the remaining budget exceeds the observed maximum" + ); + + match refused { + Err( + openhuman_core::openhuman::agent::harness::SubagentRunError::DispatchBudgetExhausted { + remaining_ms, + observed_max_ms, + observed_samples, + }, + ) => { + assert_eq!( + observed_max_ms, 360_000_000, + "the refusal must quote the turn's own measured maximum" + ); + assert!( + remaining_ms < observed_max_ms, + "refused with {remaining_ms} ms remaining against a {observed_max_ms} ms maximum" + ); + // Three, not the two recorded by hand: the ALLOWED dispatch above + // completed, and `run_subagent` folds a real child's wall-clock + // into the estimator on its own success path. That the runner + // measures its own children is the whole mechanism gate 2 rests + // on, so counting it here is the assertion, not an off-by-one. + assert_eq!( + observed_samples, 3, + "the two hand-recorded samples plus the real completed dispatch" + ); + } + other => panic!( + "a dispatch with less budget than the slowest child must be refused with \ + DispatchBudgetExhausted, got: {other:?}" + ), + } + + assert_eq!( + provider_handle.requests().len(), + 1, + "the refused dispatch must not reach the provider at all" + ); + Ok(()) +} diff --git a/tests/raw_coverage/tools_network_channels_raw_coverage_e2e.rs b/tests/raw_coverage/tools_network_channels_raw_coverage_e2e.rs index e40e2fcecc..fd5b3f352b 100644 --- a/tests/raw_coverage/tools_network_channels_raw_coverage_e2e.rs +++ b/tests/raw_coverage/tools_network_channels_raw_coverage_e2e.rs @@ -375,10 +375,34 @@ async fn web_channel_public_paths_cover_validation_cancel_schema_and_event_bus() assert_eq!(event.message.as_deref(), Some("payload")); } +/// Run `git` in `repo` with the developer's own git configuration closed out. +/// +/// The fixture below performs a real `commit`, and without this it inherits +/// whatever the machine running it happens to configure. A global +/// `commit.gpgsign = true` — which every maintainer who signs commits has, and +/// which this repository's own contributing guide asks for — makes that commit +/// try to sign, and it fails with `error: gpg failed to sign the data` for +/// reasons that have nothing to do with the code under test. CI has no global +/// git config, so the test is green there and red only on the laptops of the +/// people most likely to be running it. +/// +/// `GIT_CONFIG_GLOBAL` must name a readable-but-empty path rather than be +/// unset: unsetting it lets git fall back to `~/.gitconfig`, which is the thing +/// being closed. This mirrors `NULL_CONFIG_PATH` and `suppress_ambient_git_config` +/// in `tools/impl/filesystem/git_operations_config.rs`, and the unit suite's +/// own `hermetic()` helper. +/// +/// The committer identity is unaffected: it is set repository-locally at the +/// top of the fixture, so closing the global config does not strand the commit +/// the way it would if the identity were ambient too. fn run_git(repo: &std::path::Path, args: &[&str]) { + // `/dev/null` is not a path on Windows; `NUL` is. + let null_config = if cfg!(windows) { "NUL" } else { "/dev/null" }; let output = Command::new("git") .args(args) .current_dir(repo) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_GLOBAL", null_config) .output() .expect("spawn git"); assert!(