From ffd7595c2d5f3e2058a8464dc13ad6d6ddd6e118 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 16:46:12 +0530 Subject: [PATCH 1/8] test(e2e): backfill coverage for dispatch gates, probe outcomes, and settings failure paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six merged PRs changed behaviour that no e2e test exercised. An audit of the three lanes found the changed symbols present in e2e files for three of them — `run_subagent` in nine, `handoff` in eighteen, both profile panels in a Playwright spec — with none of those tests driving the changed path. The repo's domain e2e gate counts literals, so all three read as covered. Rust: - #5810 `run_subagent` refuses a dispatch after a cap pause, and when less wall-clock remains than the turn's slowest completed child. Both cases install a real `turn_dispatch_guard` (the gate is a no-op outside a turn scope, so a test that skips it exercises nothing) and assert the provider was never reached — the refusal is meant to cost nothing. Each drives an allowed dispatch through the same guard first, so a gate that refused unconditionally could not pass. - #5772 `probe_alive` returns a four-variant `ProbeOutcome`; the existing test only asked `.is_alive()`, which is false for all three non-alive variants alike. Pins `Missing` for an entry that was never connected, `TimedOut` for a demonstrably healthy server probed with an unmeetable window, and that the session survives it. Also pins the 8s default probe window that b44b958d restored. - #5852 `wait_agents` prunes a terminal child, so a second wait misses. The one semantic change in an otherwise-deletion PR; it lived only in prose. - #5839 the `memory_diff` RPC surface answers unknown-method on the live router, and the memory domain still answers. The existing removal regression reads the registry as a data structure, so a re-registration behind a different namespace would satisfy it. Playwright: - #5944 a failing profile save, activate and delete each show the backend's reason and not `[object Object]`. Every existing test in that spec is a happy path, which is how the defect shipped and was then pinned as expected. - #5925 a settings-load failure disables every compression switch; a savings failure leaves them usable. The second is the half the old `Promise.all` broke. --- .../specs/settings-profiles-crud.spec.ts | 132 +++++++++ .../specs/token-usage-load-failure.spec.ts | 111 +++++++ tests/json_rpc_e2e.rs | 65 +++++ tests/mcp_registry_e2e.rs | 117 ++++++++ ...gent_harness_leftovers_raw_coverage_e2e.rs | 274 ++++++++++++++++++ 5 files changed, 699 insertions(+) create mode 100644 app/test/playwright/specs/token-usage-load-failure.spec.ts 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/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..20c28bb2bd 100644 --- a/tests/mcp_registry_e2e.rs +++ b/tests/mcp_registry_e2e.rs @@ -755,3 +755,120 @@ 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`, and the distinction the +/// enum exists to carry is `TimedOut` vs `Broken`: a slow server is not a failed +/// one, and collapsing the two is what made the supervisor tear down working +/// sessions and then report a drop that never happened (#5636). +/// +/// `probe_alive_reflects_transport_liveness` above only ever asks `.is_alive()`, +/// which is `false` for all three non-alive variants alike, so nothing pinned +/// which one comes back. This does. +#[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:?}"), + } + + // The same live, healthy connection probed with a window it cannot meet. + // `Duration::ZERO` is deterministic rather than racy: an async stdio round + // trip cannot complete on the first poll, and tokio's zero-length sleep is + // already elapsed, so the timeout arm is taken every time. + // + // This is the assertion that matters: the server is demonstrably fine — it + // answered a moment ago and answers again below — so anything other than + // `TimedOut` would be the supervisor asserting a drop it did not observe. + match h + .dynamic() + .connections() + .probe_alive(&server.server_id, std::time::Duration::ZERO) + .await + { + tinymcp::ProbeOutcome::TimedOut { after } => { + assert_eq!(after, std::time::Duration::ZERO, "the window is reported back"); + } + other => panic!( + "a healthy server probed with an unmeetable window must report TimedOut, not a \ + failure it did not observe; got {other:?}" + ), + } + + // Still alive: the impossible window did not disturb the session, which is + // the whole point of not treating a timeout as a break. + assert!( + h.dynamic() + .connections() + .probe_alive(&server.server_id, std::time::Duration::from_secs(8)) + .await + .is_alive(), + "a timed-out probe must leave the connection usable" + ); + + // 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..54a7c2608d 100644 --- a/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs +++ b/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs @@ -705,3 +705,277 @@ 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" + ); + assert_eq!( + observed_samples, 2, + "both completed children must have fed the estimator" + ); + } + 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(()) +} + +// ── Orchestration registry pruning (#5852) ──────────────────────────────────── + +/// `wait_agents` prunes a child once it observes a terminal status, so a child +/// can be waited on exactly once (#5852). +/// +/// This is the one *semantic* change in a PR that otherwise claimed to be a +/// pure deletion: `AgentOrchestrationSession` moved off its own process-local +/// `SessionState` onto the crate's `DetachedTaskRegistry`, whose `wait` prunes. +/// The PR records it as an accepted behaviour change and `ops.rs:20-23` states +/// it in the module docs, but nothing anywhere asserted it — so the contract +/// existed only in prose, and a future registry swap could quietly restore the +/// old "wait as often as you like" behaviour with every test still green. +/// +/// Both halves are in one test on purpose: the first wait must succeed and +/// report a terminal child (otherwise the second wait's failure proves nothing +/// beyond "spawn is broken"), and only then must the second wait miss. +#[tokio::test] +async fn waiting_twice_on_one_orchestration_child_misses_the_pruned_entry() -> Result<()> { + use openhuman_core::openhuman::agent::harness::definition::AgentDefinitionRegistry; + use openhuman_core::openhuman::agent::orchestration::{ + AgentOrchestrationSession, OrchestrationError, OrchestrationTaskStatus, SpawnAgentRequest, + WaitAgentOptions, + }; + + // Idempotent: other cases in this binary initialise it too. + let _ = AgentDefinitionRegistry::init_global_builtins(); + + let tmp = TempDir::new()?; + let provider = ScriptedModel::new(vec![text_response("child finished its work")]); + let parent = parent_context(tmp.path().to_path_buf(), provider); + let session = AgentOrchestrationSession::new("w6-prune-session"); + + let orchestration_id = with_parent_context(parent, async { + session + .spawn_agent(SpawnAgentRequest { + agent_id: "code_executor".to_string(), + prompt: "do one small thing".to_string(), + model: Some("round19-parent".to_string()), + ..Default::default() + }) + .await + .map(|spawned| spawned.orchestration_id) + }) + .await + .expect("spawning one child"); + + let first = session + .wait_agents(WaitAgentOptions { + orchestration_ids: vec![orchestration_id.clone()], + timeout_ms: Some(20_000), + }) + .await + .expect("the first wait resolves"); + + assert!(first.completed, "the first wait must reach a terminal status"); + assert_eq!(first.agents.len(), 1); + assert!( + first.agents[0].status.is_terminal(), + "expected a terminal child, got {:?}", + first.agents[0].status + ); + + // The pruning half. The entry is gone now that a terminal status has been + // observed, so the same id no longer resolves — `AgentNotFound`, not a + // second copy of the terminal snapshot. + match session + .wait_agents(WaitAgentOptions { + orchestration_ids: vec![orchestration_id.clone()], + timeout_ms: Some(1_000), + }) + .await + { + Err(OrchestrationError::AgentNotFound(missing)) => { + assert_eq!( + missing, orchestration_id, + "the error must name the child that was pruned" + ); + } + Ok(response) => panic!( + "a second wait must miss the pruned entry, but it resolved again with {:?}", + response + .agents + .iter() + .map(|agent| agent.status) + .collect::>() + ), + Err(other) => panic!("expected AgentNotFound from the pruned entry, got: {other:?}"), + } + + Ok(()) +} From a3836c78df3edfe1b9e851ba5fd2844de3c3a1f3 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 16:54:25 +0530 Subject: [PATCH 2/8] style: rustfmt the lines this branch added --- tests/mcp_registry_e2e.rs | 6 +++++- .../agent_harness_leftovers_raw_coverage_e2e.rs | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/mcp_registry_e2e.rs b/tests/mcp_registry_e2e.rs index 20c28bb2bd..c5f35467ad 100644 --- a/tests/mcp_registry_e2e.rs +++ b/tests/mcp_registry_e2e.rs @@ -819,7 +819,11 @@ async fn probe_alive_distinguishes_a_missing_entry_from_a_timed_out_one() { .await { tinymcp::ProbeOutcome::TimedOut { after } => { - assert_eq!(after, std::time::Duration::ZERO, "the window is reported back"); + assert_eq!( + after, + std::time::Duration::ZERO, + "the window is reported back" + ); } other => panic!( "a healthy server probed with an unmeetable window must report TimedOut, not a \ 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 54a7c2608d..229c14bc1d 100644 --- a/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs +++ b/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs @@ -767,7 +767,9 @@ async fn dispatch_is_refused_once_the_turn_has_requested_a_cap_pause() -> Result let (allowed, refused) = outcome; assert_eq!( - allowed.expect("a dispatch before the pause must be allowed").output, + allowed + .expect("a dispatch before the pause must be allowed") + .output, "first child answer", "the guard must not refuse before a pause is recorded" ); @@ -942,7 +944,10 @@ async fn waiting_twice_on_one_orchestration_child_misses_the_pruned_entry() -> R .await .expect("the first wait resolves"); - assert!(first.completed, "the first wait must reach a terminal status"); + assert!( + first.completed, + "the first wait must reach a terminal status" + ); assert_eq!(first.agents.len(), 1); assert!( first.agents[0].status.is_terminal(), From 3c5bd4c9f739e7b8b4b574ad8cc902d9deb53185 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 16:55:37 +0530 Subject: [PATCH 3/8] style: rustfmt agent_harness_leftovers, unblocking the Rust Quality gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file was already unformatted on `upstream/main` (verified by running rustfmt against the pristine blob), so `cargo fmt --check` was failing before this branch existed. That matters here rather than being someone else's problem: `rust-core-coverage` is declared `if: … needs['rust-quality'].result == 'success'` (ci-lite.yml:721), so the formatting failure skips the job that would actually run the tests this branch adds. Formatter output only — 10 insertions, 11 deletions, all import ordering and wrapping. Toolchain-pinned rustfmt 1.96.1, matching CI. --- ...gent_harness_leftovers_raw_coverage_e2e.rs | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) 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 229c14bc1d..2ba4965f15 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(), From 68d5f62865c7f5110702972a58b579bb7c6bfff4 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 18:10:53 +0530 Subject: [PATCH 4/8] test(e2e): correct two assertions and drop one the revert-check proved vacuous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert-checking found three things, and in two of them the code was right and my test was wrong. - **#5772 `TimedOut` is not reachable here.** A probe with a `Duration::ZERO` window still returned `Alive { elapsed: 42.708µs }`: `tokio::time::timeout` polls the inner future before it checks the deadline, and `list_tools` on an established stdio connection answers inside that first poll. Producing a real timeout needs a stub that stalls on a named method, which `test-mcp-stub` cannot be asked for. The assertion is dropped rather than contrived; the test keeps the `Missing` half, which is reachable and which the old `bool` API could equally not express. - **#5810 `observed_samples` is 3, not 2.** The allowed dispatch in the same test completes, and `run_subagent` folds a real child's wall-clock into the estimator on its success path. That the runner measures its own children is the mechanism gate 2 rests on, so counting it is the assertion. - **#5852's pruning test is removed as vacuous.** Deleting `self.remove(task_id)?` from `DetachedTaskRegistry::wait` did not make it fail — the run rebuilt (1m27s) and still passed, so the entry is pruned by some path other than the one `ops.rs:20-23` documents. It asserted something true without being able to distinguish the documented mechanism from whatever actually does the work, which is not a test that would catch the regression it was written for. Remaining five, each revert-checked: both #5810 dispatch refusals (gate removed -> both fail), #5772 `Missing` (Missing -> Broken -> fails naming the assertion), #5772's 8s window (8s -> 30s -> fails naming the message), and #5839's `memory_diff` removal. --- tests/mcp_registry_e2e.rs | 62 +++-------- ...gent_harness_leftovers_raw_coverage_e2e.rs | 104 ++---------------- 2 files changed, 25 insertions(+), 141 deletions(-) diff --git a/tests/mcp_registry_e2e.rs b/tests/mcp_registry_e2e.rs index c5f35467ad..7a379ba982 100644 --- a/tests/mcp_registry_e2e.rs +++ b/tests/mcp_registry_e2e.rs @@ -758,14 +758,16 @@ async fn per_config_lookups_see_a_per_config_connection() { // ── Probe outcomes (#5772) ──────────────────────────────────────────────────── -/// `probe_alive` returns a four-variant `ProbeOutcome`, and the distinction the -/// enum exists to carry is `TimedOut` vs `Broken`: a slow server is not a failed -/// one, and collapsing the two is what made the supervisor tear down working -/// sessions and then report a drop that never happened (#5636). +/// `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. /// -/// `probe_alive_reflects_transport_liveness` above only ever asks `.is_alive()`, -/// which is `false` for all three non-alive variants alike, so nothing pinned -/// which one comes back. This does. +/// 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(); @@ -804,43 +806,15 @@ async fn probe_alive_distinguishes_a_missing_entry_from_a_timed_out_one() { other => panic!("a live stub must probe Alive, got {other:?}"), } - // The same live, healthy connection probed with a window it cannot meet. - // `Duration::ZERO` is deterministic rather than racy: an async stdio round - // trip cannot complete on the first poll, and tokio's zero-length sleep is - // already elapsed, so the timeout arm is taken every time. - // - // This is the assertion that matters: the server is demonstrably fine — it - // answered a moment ago and answers again below — so anything other than - // `TimedOut` would be the supervisor asserting a drop it did not observe. - match h - .dynamic() - .connections() - .probe_alive(&server.server_id, std::time::Duration::ZERO) - .await - { - tinymcp::ProbeOutcome::TimedOut { after } => { - assert_eq!( - after, - std::time::Duration::ZERO, - "the window is reported back" - ); - } - other => panic!( - "a healthy server probed with an unmeetable window must report TimedOut, not a \ - failure it did not observe; got {other:?}" - ), - } - - // Still alive: the impossible window did not disturb the session, which is - // the whole point of not treating a timeout as a break. - assert!( - h.dynamic() - .connections() - .probe_alive(&server.server_id, std::time::Duration::from_secs(8)) - .await - .is_alive(), - "a timed-out probe must leave the connection usable" - ); + // 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. 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 2ba4965f15..e0a0dcb937 100644 --- a/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs +++ b/tests/raw_coverage/agent_harness_leftovers_raw_coverage_e2e.rs @@ -870,9 +870,14 @@ async fn dispatch_is_refused_when_less_budget_remains_than_the_slowest_child() - 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, 2, - "both completed children must have fed the estimator" + observed_samples, 3, + "the two hand-recorded samples plus the real completed dispatch" ); } other => panic!( @@ -888,98 +893,3 @@ async fn dispatch_is_refused_when_less_budget_remains_than_the_slowest_child() - ); Ok(()) } - -// ── Orchestration registry pruning (#5852) ──────────────────────────────────── - -/// `wait_agents` prunes a child once it observes a terminal status, so a child -/// can be waited on exactly once (#5852). -/// -/// This is the one *semantic* change in a PR that otherwise claimed to be a -/// pure deletion: `AgentOrchestrationSession` moved off its own process-local -/// `SessionState` onto the crate's `DetachedTaskRegistry`, whose `wait` prunes. -/// The PR records it as an accepted behaviour change and `ops.rs:20-23` states -/// it in the module docs, but nothing anywhere asserted it — so the contract -/// existed only in prose, and a future registry swap could quietly restore the -/// old "wait as often as you like" behaviour with every test still green. -/// -/// Both halves are in one test on purpose: the first wait must succeed and -/// report a terminal child (otherwise the second wait's failure proves nothing -/// beyond "spawn is broken"), and only then must the second wait miss. -#[tokio::test] -async fn waiting_twice_on_one_orchestration_child_misses_the_pruned_entry() -> Result<()> { - use openhuman_core::openhuman::agent::harness::definition::AgentDefinitionRegistry; - use openhuman_core::openhuman::agent::orchestration::{ - AgentOrchestrationSession, OrchestrationError, OrchestrationTaskStatus, SpawnAgentRequest, - WaitAgentOptions, - }; - - // Idempotent: other cases in this binary initialise it too. - let _ = AgentDefinitionRegistry::init_global_builtins(); - - let tmp = TempDir::new()?; - let provider = ScriptedModel::new(vec![text_response("child finished its work")]); - let parent = parent_context(tmp.path().to_path_buf(), provider); - let session = AgentOrchestrationSession::new("w6-prune-session"); - - let orchestration_id = with_parent_context(parent, async { - session - .spawn_agent(SpawnAgentRequest { - agent_id: "code_executor".to_string(), - prompt: "do one small thing".to_string(), - model: Some("round19-parent".to_string()), - ..Default::default() - }) - .await - .map(|spawned| spawned.orchestration_id) - }) - .await - .expect("spawning one child"); - - let first = session - .wait_agents(WaitAgentOptions { - orchestration_ids: vec![orchestration_id.clone()], - timeout_ms: Some(20_000), - }) - .await - .expect("the first wait resolves"); - - assert!( - first.completed, - "the first wait must reach a terminal status" - ); - assert_eq!(first.agents.len(), 1); - assert!( - first.agents[0].status.is_terminal(), - "expected a terminal child, got {:?}", - first.agents[0].status - ); - - // The pruning half. The entry is gone now that a terminal status has been - // observed, so the same id no longer resolves — `AgentNotFound`, not a - // second copy of the terminal snapshot. - match session - .wait_agents(WaitAgentOptions { - orchestration_ids: vec![orchestration_id.clone()], - timeout_ms: Some(1_000), - }) - .await - { - Err(OrchestrationError::AgentNotFound(missing)) => { - assert_eq!( - missing, orchestration_id, - "the error must name the child that was pruned" - ); - } - Ok(response) => panic!( - "a second wait must miss the pruned entry, but it resolved again with {:?}", - response - .agents - .iter() - .map(|agent| agent.status) - .collect::>() - ), - Err(other) => panic!("expected AgentNotFound from the pruned entry, got: {other:?}"), - } - - Ok(()) -} From 30ba798fe8bbfc6129056fdaf81f746ec98d55e0 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 19:44:01 +0530 Subject: [PATCH 5/8] fix(git_operations): suppress an external diff with --no-ext-diff, not an empty config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NEUTRALISED_CONFIG` carried `diff.external=`, and an empty value does not disable an external diff — git tries to *execute* the empty string, so every `diff` operation died before producing a patch: error: cannot run : No such file or directory fatal: external diff died, stopping at tracked.txt Reproduced against git directly rather than inferred: $ git -c diff.external= diff -- tracked.txt error: cannot run : No such file or directory # the CI failure verbatim $ git diff --no-ext-diff -- tracked.txt +second `--no-ext-diff` is the real suppression and is strictly stronger: verified against a repository with `diff.external=/bin/false`, plain `diff` dies and `--no-ext-diff` still prints the patch. So this hardens the operation where the old form removed it. `diff.external` is on neither allowlist, so a repository carrying it is refused before the invocation; this is the second layer, for the gap between that inspection and the command. The regression test goes in the **lib** suite deliberately. This broke every diff on every repository and still reached `main`, because a change to `git_operations.rs` maps to the `openhuman::tools` libtest filter while the test that caught it lives in `raw_coverage_all` — a target that filter never selects. `main`'s own run over this file (8e65c4008) therefore did not execute it. A test here runs whenever this file is touched. Revert-checked: with `diff.external=` restored and `--no-ext-diff` removed, the new test fails naming its own assertion and reproducing the exact error above; restored, 43 pass. The same entry is still present in `tools/impl/system/workspace_state.rs:235`. It is latent there — that module runs only `status` and `log --oneline`, neither of which generates a diff — so it is left alone here and recorded rather than swept into an unrelated change. --- .../tools/impl/filesystem/git_operations.rs | 5 +- .../impl/filesystem/git_operations_config.rs | 9 ++- .../filesystem/git_operations_config_tests.rs | 70 +++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/src/openhuman/tools/impl/filesystem/git_operations.rs b/src/openhuman/tools/impl/filesystem/git_operations.rs index 0186ce495e..c96d1dd7fa 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations.rs @@ -174,7 +174,10 @@ 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. + let mut git_args = vec!["diff", "--no-ext-diff", "--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..e52af52109 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,73 @@ 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()); + + 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() + ); +} From f4c3ab8038d7565bef297cffdb50bab48322f8e4 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 20:16:40 +0530 Subject: [PATCH 6/8] test(git_operations): give the diff regression test its own committer identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test failed in CI at the `git commit` fixture step and passed locally. The difference is the container, not the code: `hermetic` closes the global and system config — which is its whole purpose — and CI's git then has no identity to fall back on, so `git commit` refuses with "Author identity unknown". macOS git derives one from the system instead, which is why this could not be reproduced here (verified: the same commit under `GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null` succeeds locally). Set `user.email` / `user.name` in the repository itself, so the fixture does not depend on whether the host's git can invent an identity. Both keys are on `ALLOWED_REPO_CONFIG`, so this does not trip the repository-config refusal the surrounding tests exercise. --- .../tools/impl/filesystem/git_operations_config_tests.rs | 8 ++++++++ 1 file changed, 8 insertions(+) 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 e52af52109..ddb30306ff 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations_config_tests.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations_config_tests.rs @@ -494,6 +494,14 @@ 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 — From 9d4cc26d879f251477055d5a8d710b957ec73732 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 21:30:32 +0530 Subject: [PATCH 7/8] test(git_operations): make the e2e git fixture hermetic instead of reading the developer's config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run_git` in `tools_network_channels_raw_coverage_e2e.rs` spawned git with the machine's own configuration in scope, and the fixture performs a real `commit`. A global `commit.gpgsign = true` — which every maintainer who signs commits has — made that commit try to sign and fail: gpg: skipped "DEADBEEFDEADBEEF": Input/output error gpg: signing failed: Input/output error fatal: failed to write commit object CI has no global git config, so this was green there and red only on the laptops of the people most likely to run it. Reported by W2, who lost time to it. Close the system and global config the way the unit suite's `hermetic()` and the product's own `suppress_ambient_git_config` do. `GIT_CONFIG_GLOBAL` must name a readable-but-empty path rather than be unset, or git falls back to `~/.gitconfig` — the thing being closed. The committer identity is unaffected: the fixture already sets `user.email` / `user.name` repository-locally, so nothing is stranded by removing the ambient config. Revert-checked under a fabricated signing developer's `HOME` (global `commit.gpgsign = true`, unusable key): - without the fix: fails at the commit, `gpg: signing failed` - with the fix: no gpg complaint at all; the commit succeeds With the fix the test then reaches, and fails on, the `diff.external` defect fixed earlier in this same PR — which is why the two belong together: neither alone makes this test pass on a maintainer's machine. --- ...tools_network_channels_raw_coverage_e2e.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) 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!( From 34fe8e90d4a7a2f912c81681807c157cbb2547f2 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 2 Sep 2026 21:34:08 +0530 Subject: [PATCH 8/8] fix(git-tool): suppress textconv on git_diff, not just external diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit caught a gap in the `--no-ext-diff` hardening this PR added: `--no-ext-diff` covers `diff.external` and `diff..command`, but textconv is a separate mechanism with its own flag. A `.gitattributes` line selecting a driver whose `diff..textconv` is set makes git EXECUTE that command to render a binary file as text. Reproduced against a scratch repo before changing anything: with `--no-ext-diff` alone the textconv script ran; with `--no-textconv` added it did not. Scope, stated honestly: this is the same second layer `--no-ext-diff` is, not a live hole. `diff..textconv` is not on `ALLOWED_REPO_CONFIG`, which is an allowlist that fails closed, so a repository carrying the key is already refused before the command runs. The flag closes the window between that inspection and the invocation — the same rationale the existing `--no-ext-diff` comment gives for itself. `hardened_git`'s `-c` list cannot do this job: driver names are arbitrary, so there is no finite set of keys to neutralise. It has to be a command flag. --- .../tools/impl/filesystem/git_operations.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/openhuman/tools/impl/filesystem/git_operations.rs b/src/openhuman/tools/impl/filesystem/git_operations.rs index c96d1dd7fa..1fbcd7dd5c 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations.rs @@ -177,7 +177,17 @@ impl GitOperationsTool { // `--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. - let mut git_args = vec!["diff", "--no-ext-diff", "--unified=3"]; + // + // `--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"); }