test(e2e): backfill coverage for dispatch gates, probe outcomes, and settings failure paths

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 <p>, 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();
+  });
+});

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();
+  });
+});

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.<driver>.command`;
+        // it does not touch `diff.<driver>.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");
         }

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=",
 ];

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()
+    );
+}