diff --git a/src/lib/adapters/openshell/provider-profile.test.ts b/src/lib/adapters/openshell/provider-profile.test.ts index e5658de01c1..d064b5fbf15 100644 --- a/src/lib/adapters/openshell/provider-profile.test.ts +++ b/src/lib/adapters/openshell/provider-profile.test.ts @@ -2,9 +2,12 @@ // SPDX-License-Identifier: Apache-2.0 import path from "node:path"; + +import YAML from "yaml"; import { describe, expect, it, vi } from "vitest"; import { + compareExportedProfileToCheckedIn, endpointlessProviderProfilePath, ensureEndpointlessProviderProfile, type EndpointlessProviderProfileRunner, @@ -212,4 +215,234 @@ describe("OpenShell endpointless provider profiles", () => { expect(ensureProfile(runOpenshell)).toEqual({ ok: false, reason: "export-failed" }); }); + + it("reuses an exact profile whose import-race diagnostic is wrapped across a box-drawing line (#10371)", () => { + // Same failure shape #10159 fixed for the "not found" match: OpenShell + // can wrap "already exists" across a box-drawing continuation depending + // on terminal width/TTY-ness, which a plain substring test would miss. + const runOpenshell = vi + .fn() + .mockReturnValueOnce({ status: 1, stderr: "provider profile not found" }) + .mockReturnValueOnce({ status: 1, stderr: "provider profile already\n │ exists" }) + .mockReturnValueOnce({ status: 0, stdout: EXPECTED_PROFILE }); + + expect(ensureProfile(runOpenshell)).toEqual({ ok: true }); + expect(runOpenshell).toHaveBeenCalledTimes(3); + }); +}); + +describe("compareExportedProfileToCheckedIn", () => { + // A refreshing profile as this repository checks it in: the kebab-case + // strategy spelling, and material entries that omit `secret`/`required` + // where the default is false. + const CHECKED_IN = { + id: "google-chat-bridge", + credentials: [ + { + name: "access_token", + env_vars: ["GOOGLE_CHAT_ACCESS_TOKEN"], + required: true, + auth_style: "bearer", + header_name: "Authorization", + query_param: "", + refresh: { + strategy: "google-service-account-jwt", + scopes: ["https://www.googleapis.com/auth/chat.bot"], + material: [ + { name: "client_email", description: "JWT issuer", required: true }, + { + name: "private_key", + description: "signs the assertion", + required: true, + secret: true, + }, + { name: "scope", description: "scope to mint for" }, + ], + }, + }, + ], + endpoints: [{ host: "chat.googleapis.com", port: 443, protocol: "rest", access: "read-write" }], + binaries: ["/usr/bin/node"], + inference_capable: false, + }; + + // What `openshell provider profile export -o json` returns for that same + // profile on the pinned OpenShell v0.0.106: the stored profile is + // re-serialized, so `serialize_refresh_strategy` writes the snake_case wire + // spelling and CredentialRefreshMaterialProfile's `required`/`secret` carry + // no `skip_serializing_if`, so both appear on every material entry. + const PINNED_EXPORT = { + ...CHECKED_IN, + credentials: [ + { + ...CHECKED_IN.credentials[0], + refresh: { + ...CHECKED_IN.credentials[0].refresh, + strategy: "google_service_account_jwt", + material: [ + { name: "client_email", description: "JWT issuer", required: true, secret: false }, + { + name: "private_key", + description: "signs the assertion", + required: true, + secret: true, + }, + { name: "scope", description: "scope to mint for", required: false, secret: false }, + ], + }, + }, + ], + }; + + const compare = (exported: unknown, readCheckedIn: () => string) => + compareExportedProfileToCheckedIn( + JSON.stringify(exported), + readCheckedIn, + "google-chat-bridge", + ); + + const readCheckedIn = () => YAML.stringify(CHECKED_IN); + + it("accepts OpenShell's own re-serialization of the checked-in profile (#10371)", () => { + // The gateway holds exactly what this checkout imported. Comparing the + // export byte-for-byte against the YAML would report a byte-valid, + // unmodified profile as drift and route the operator to delete it. + expect(compare(PINNED_EXPORT, readCheckedIn)).toBe("match"); + }); + + it("ignores refresh material help text outside the credential boundary", () => { + const updatedHelp = { + ...PINNED_EXPORT, + credentials: [ + { + ...PINNED_EXPORT.credentials[0], + refresh: { + ...PINNED_EXPORT.credentials[0].refresh, + material: PINNED_EXPORT.credentials[0].refresh.material.map((entry) => ({ + ...entry, + description: `updated ${entry.name} help`, + })), + }, + }, + ], + }; + + expect(compare(updatedHelp, readCheckedIn)).toBe("match"); + }); + + it("does not accept checked-in refresh YAML as an exported profile", () => { + // The pinned exporter always emits both material flags. Missing flags mean + // the output did not complete the expected OpenShell serialization. + expect(compare(CHECKED_IN, readCheckedIn)).toBe("indeterminate"); + }); + + it("reports a registered profile with different endpoint authority as drift", () => { + const drifted = { ...PINNED_EXPORT, endpoints: [{ host: "attacker.example", port: 443 }] }; + expect(compare(drifted, readCheckedIn)).toBe("mismatch"); + }); + + it("keeps the pinned exporter's multi-entry endpoint order in the boundary", () => { + // OpenShell v0.0.106 stores and serializes profile sequences as Vec values, + // so declaration order survives import/export and remains part of the + // exact checked-in contract. + const endpoints = [ + { host: "chat.googleapis.com", port: 443 }, + { host: "pubsub.googleapis.com", port: 443 }, + ]; + const readTwoEndpoints = () => YAML.stringify({ ...CHECKED_IN, endpoints }); + + expect(compare({ ...PINNED_EXPORT, endpoints }, readTwoEndpoints)).toBe("match"); + expect( + compare({ ...PINNED_EXPORT, endpoints: [...endpoints].reverse() }, readTwoEndpoints), + ).toBe("mismatch"); + }); + + it("reports a genuinely different refresh strategy as drift, not a spelling difference", () => { + const drifted = { + ...PINNED_EXPORT, + credentials: [ + { + ...PINNED_EXPORT.credentials[0], + refresh: { ...PINNED_EXPORT.credentials[0].refresh, strategy: "oauth2_refresh_token" }, + }, + ], + }; + expect(compare(drifted, readCheckedIn)).toBe("mismatch"); + }); + + it("reports a material entry promoted to secret as drift", () => { + const drifted = { + ...PINNED_EXPORT, + credentials: [ + { + ...PINNED_EXPORT.credentials[0], + refresh: { + ...PINNED_EXPORT.credentials[0].refresh, + material: [ + { name: "client_email", description: "JWT issuer", required: true, secret: true }, + ], + }, + }, + ], + }; + expect(compare(drifted, readCheckedIn)).toBe("mismatch"); + }); + + it.each(["required", "secret"])( + "reports a null exported refresh material %s flag as indeterminate", + (field) => { + const malformed = { + ...PINNED_EXPORT, + credentials: [ + { + ...PINNED_EXPORT.credentials[0], + refresh: { + ...PINNED_EXPORT.credentials[0].refresh, + material: [ + { + ...PINNED_EXPORT.credentials[0].refresh.material[0], + [field]: null, + }, + ], + }, + }, + ], + }; + expect(compare(malformed, readCheckedIn)).toBe("indeterminate"); + }, + ); + + it("reports an unreadable checked-in profile as indeterminate, not drift (#10371)", () => { + const throwing = () => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }); + }; + expect(compare(PINNED_EXPORT, throwing)).toBe("indeterminate"); + }); + + it("reports an unparseable checked-in profile as indeterminate, not drift (#10371)", () => { + expect(compare(PINNED_EXPORT, () => "id: [unterminated")).toBe("indeterminate"); + }); + + it("reports a checked-in file that is not a provider profile as indeterminate", () => { + expect(compare(PINNED_EXPORT, () => YAML.stringify({ id: "google-chat-bridge" }))).toBe( + "indeterminate", + ); + }); + + it("reports an export that is not JSON as indeterminate, not drift (#10371)", () => { + // A truncated or diagnostic-laden export is a read that never completed. + // Treating it as drift would tell the operator to delete a profile whose + // contents were never seen. + expect( + compareExportedProfileToCheckedIn("gateway unavailable", readCheckedIn, "google-chat-bridge"), + ).toBe("indeterminate"); + }); + + it("reports a valid JSON export that is not a provider profile as indeterminate", () => { + expect(compare({}, readCheckedIn)).toBe("indeterminate"); + }); + + it("reports an export of a different profile id as drift", () => { + expect(compare({ ...PINNED_EXPORT, id: "brave" }, readCheckedIn)).toBe("mismatch"); + }); }); diff --git a/src/lib/adapters/openshell/provider-profile.ts b/src/lib/adapters/openshell/provider-profile.ts index a891b4480be..42f068a2f8d 100644 --- a/src/lib/adapters/openshell/provider-profile.ts +++ b/src/lib/adapters/openshell/provider-profile.ts @@ -2,6 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 import path from "node:path"; +import { isDeepStrictEqual } from "node:util"; + +import YAML from "yaml"; + import { REPOSITORY_ROOT } from "../../core/repository-root"; import { OPENSHELL_OPERATION_TIMEOUT_MS } from "./provider-command"; @@ -25,6 +29,17 @@ function outputText(value: unknown): string { return typeof value === "string" ? value : ""; } +/** Join captured OpenShell diagnostics without exposing them to the terminal. */ +export function openshellResultDiagnostic(result: { + readonly error?: Error; + readonly stderr?: unknown; + readonly stdout?: unknown; +}): string { + return [outputText(result.stderr), outputText(result.stdout), result.error?.message ?? ""] + .filter(Boolean) + .join(" "); +} + function commandOutput(result: { readonly output?: unknown; readonly stdout?: unknown; @@ -44,12 +59,27 @@ function commandStdout(result: { readonly output?: unknown; readonly stdout?: un return Array.isArray(result.output) ? outputText(result.output[1]) : outputText(result.output); } -function isMissingProviderProfile(output: string, profileId: string): boolean { - const normalized = output +/** + * Strip ANSI escapes, carriage returns, and OpenShell's box-drawing line + * continuations so a diagnostic can be pattern-matched regardless of the + * terminal width or TTY-ness that produced them (#10159). + */ +export function normalizeOpenshellDiagnostic(output: string): string { + return output .replace(/\u001b\[[0-?]*[ -/]*[@-~]/gu, "") .replace(/\r/gu, "") .replace(/\n\s*│\s*/gu, " ") .trim(); +} + +/** + * Whether `output` (an export probe's failure diagnostic) means the profile + * genuinely doesn't exist yet, as opposed to the probe itself failing for + * some other reason (gateway unavailable, auth, timeout, malformed + * response). Only a genuine "not found" makes it safe to proceed to import. + */ +export function isMissingProviderProfile(output: string, profileId: string): boolean { + const normalized = normalizeOpenshellDiagnostic(output); const escapedProfileId = profileId.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const missingMessage = new RegExp( `^(?:(?:custom )?provider )?profile(?: ['\"]${escapedProfileId}['\"])? not found[.!]?$`, @@ -63,6 +93,180 @@ function isMissingProviderProfile(output: string, profileId: string): boolean { return structuredStatus.test(normalized) && missingMessage.test(message); } +/** + * Project an OpenShell provider-profile document down to the fields that + * define its authorization boundary (credentials, endpoints, binaries, + * inference capability), or null if the document doesn't have the expected + * shape. Callers compare this projection between an exported profile and its + * checked-in YAML rather than trusting a matching profile ID alone, since a + * host-global profile store can hold a profile some other process imported + * under the same ID with a different boundary. + */ +export function credentialBoundary(doc: Record): Record | null { + if ( + typeof doc.id !== "string" || + !Array.isArray(doc.credentials) || + !Array.isArray(doc.endpoints) || + !Array.isArray(doc.binaries) || + typeof doc.inference_capable !== "boolean" + ) { + return null; + } + const credentials = doc.credentials.map((entry) => { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) return null; + const credential = entry as Record; + return { + name: credential.name, + env_vars: credential.env_vars, + required: credential.required, + auth_style: credential.auth_style, + header_name: credential.header_name, + query_param: credential.query_param, + refresh: credential.refresh ?? null, + }; + }); + if (credentials.some((entry) => entry === null)) return null; + return { + id: doc.id, + credentials, + endpoints: doc.endpoints, + binaries: doc.binaries, + inference_capable: doc.inference_capable, + }; +} + +/** + * Project one credential's `refresh` block into the representation OpenShell + * exports it in. + * + * `provider profile export` re-serializes the stored profile rather than + * echoing the YAML that was imported, so two fields do not survive the round + * trip byte-identically on the OpenShell release this blueprint pins + * (v0.0.106): + * + * - `strategy` is read through `provider_refresh_strategy_from_yaml`, which + * lowercases and maps `-` to `_`, and written back through + * `provider_refresh_strategy_to_yaml`, which only ever emits the snake_case + * wire spelling. A profile checked in as `google-service-account-jwt` + * therefore exports as `google_service_account_jwt`. + * - `material[].required` and `material[].secret` carry no + * `skip_serializing_if`, so both are emitted on every entry even where the + * checked-in YAML omits them and relies on the `false` default. + */ +function canonicalizeRefresh(refresh: unknown): unknown { + if (refresh === null || typeof refresh !== "object" || Array.isArray(refresh)) return refresh; + const block = refresh as Record; + const canonical: Record = { ...block }; + if (typeof block.strategy === "string") { + canonical.strategy = block.strategy.trim().toLowerCase().replaceAll("-", "_"); + } + if (Array.isArray(block.material)) { + canonical.material = block.material.map((entry) => { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) return entry; + const material = entry as Record; + const { description: _description, ...credentialMaterial } = material; + return { + ...credentialMaterial, + required: material.required ?? false, + secret: material.secret ?? false, + }; + }); + } + return canonical; +} + +/** + * Project a credential boundary into OpenShell's export representation so a + * checked-in profile and the export of that same profile compare equal. Applied + * to both sides of the comparison; it is a no-op on an already-exported + * boundary. See {@link canonicalizeRefresh} for what OpenShell normalizes. + */ +export function canonicalizeCredentialBoundary( + boundary: Record, +): Record { + const credentials = boundary.credentials as Record[]; + return { + ...boundary, + credentials: credentials.map((credential) => ({ + ...credential, + refresh: canonicalizeRefresh(credential.refresh), + })), + }; +} + +/** + * Whether an exported profile is the checked-in profile this codebase ships + * for `expectedId`. + * + * `indeterminate` means the comparison never completed: the export was not + * JSON, or the checked-in YAML could not be read, parsed, or projected to a + * credential boundary. An unfinished read is not evidence of drift, so callers + * must route it to "could not verify" rather than to guidance that deletes the + * registered profile. + */ +export type CheckedInBoundaryComparison = "match" | "mismatch" | "indeterminate"; +export type RegisteredProfileComparison = CheckedInBoundaryComparison | "absent"; + +function hasValidRefreshMaterialFlags( + boundary: Record, + requireExplicitFlags: boolean, +): boolean { + const credentials = boundary.credentials as Record[]; + return credentials.every((credential) => { + const refresh = credential.refresh; + if (refresh === null || refresh === undefined) return true; + if (typeof refresh !== "object" || Array.isArray(refresh)) return false; + const material = (refresh as Record).material; + if (material === undefined) return true; + if (!Array.isArray(material)) return false; + return material.every((entry) => { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) return false; + const item = entry as Record; + return ["required", "secret"].every((field) => { + const value = item[field]; + return requireExplicitFlags + ? typeof value === "boolean" + : value === undefined || typeof value === "boolean"; + }); + }); + }); +} + +export function compareExportedProfileToCheckedIn( + exportedJson: string, + readCheckedInYaml: () => string, + expectedId: string, +): CheckedInBoundaryComparison { + let expected: Record | null; + try { + expected = credentialBoundary(YAML.parse(readCheckedInYaml()) as Record); + } catch { + return "indeterminate"; + } + if (expected === null || expected.id !== expectedId) return "indeterminate"; + + let actual: Record | null; + try { + actual = credentialBoundary(JSON.parse(exportedJson) as Record); + } catch { + return "indeterminate"; + } + if (actual === null) return "indeterminate"; + if ( + !hasValidRefreshMaterialFlags(actual, true) || + !hasValidRefreshMaterialFlags(expected, false) + ) { + return "indeterminate"; + } + + return isDeepStrictEqual( + canonicalizeCredentialBoundary(actual), + canonicalizeCredentialBoundary(expected), + ) + ? "match" + : "mismatch"; +} + function profileHasExpectedCredentialBoundary( output: string, expected: { readonly id: string; readonly inferenceCapable: boolean }, @@ -147,7 +351,7 @@ export function ensureEndpointlessProviderProfile(input: { if (imported.status === 0) return { ok: true }; const importOutput = commandOutput(imported); - if (!/already exists/iu.test(importOutput)) { + if (!/already exists/iu.test(normalizeOpenshellDiagnostic(importOutput))) { return { ok: false, reason: "import-failed" }; } diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index d5ac5be6ac3..7193edf2a22 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -3337,7 +3337,7 @@ async function runOnboard(opts: OnboardOptions = {}): Promise { finalization: { stagedLegacyKeys, migratedLegacyKeys, - webSearchEnabled: (config) => braveProviderProfile.shouldEnableBraveWebSearch(config), + webSearchEnabled: (config) => braveProviderProfile.shouldEnableWebSearch(config), webSearchProvider: (config) => webSearchProviderForConfig(config), }, finalizationDeps: { diff --git a/src/lib/onboard/brave-provider-profile.test.ts b/src/lib/onboard/brave-provider-profile.test.ts index 90556de649b..b693f079254 100644 --- a/src/lib/onboard/brave-provider-profile.test.ts +++ b/src/lib/onboard/brave-provider-profile.test.ts @@ -1,19 +1,115 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import YAML from "yaml"; import { describe, expect, it, vi } from "vitest"; +import { OPENSHELL_OPERATION_TIMEOUT_MS } from "../adapters/openshell/provider-command"; import { BRAVE_PROVIDER_PROFILE_ID, - braveProviderProfilePath, - ensureBraveProviderProfile, ensureWebSearchProviderProfiles, HERMES_TAVILY_PROVIDER_PROFILE_ID, - shouldEnableBraveWebSearch, + shouldEnableWebSearch, TAVILY_PROVIDER_PROFILE_ID, webSearchProviderProfilePath, } from "./brave-provider-profile"; +type RunResult = { status: number; stderr: string; stdout: string }; + +const PROFILE_ABSENT: RunResult = { + status: 1, + stderr: "custom provider profile not found", + stdout: "", +}; +const IMPORTED: RunResult = { status: 0, stderr: "", stdout: "" }; + +/** A minimal, internally-consistent credential boundary for a given profile id. */ +function boundary(id: string, overrides: Record = {}) { + return { + id, + credentials: [ + { + name: "api_key", + env_vars: [`${id.toUpperCase()}_API_KEY`], + required: true, + auth_style: "header", + header_name: "x-api-key", + query_param: "", + }, + ], + endpoints: [{ host: `api.${id}.example`, port: 443, protocol: "rest", access: "read-write" }], + binaries: ["/usr/bin/node"], + inference_capable: false, + ...overrides, + }; +} + +/** + * The checked-in YAML `readFileSync` should return for each provider id, by + * default — every "already registered" fixture below claims a matching + * export unless a test explicitly builds a drifted one, so this is what + * every id-aware probe response is compared against. + */ +function makeReadFileSync(perProvider: Record = {}) { + return vi.fn((file: string) => { + const id = WEB_SEARCH_IDS.find((candidate) => file.endsWith(`${candidate}.yaml`)); + return YAML.stringify(perProvider[id ?? ""] ?? boundary(id ?? "unknown")); + }); +} + +const WEB_SEARCH_IDS = [ + BRAVE_PROVIDER_PROFILE_ID, + TAVILY_PROVIDER_PROFILE_ID, + HERMES_TAVILY_PROVIDER_PROFILE_ID, +] as const; + +/** + * Answer the existence probe per the exact profile id the call names — + * catches a probe that hardcodes one provider id instead of using the + * loop variable, which a call-shape-only mock cannot (#10371). Registered + * ids answer with a matching boundary by default so the new drift check + * doesn't fail every pre-existing "already registered" test; pass an + * override to test the mismatch path itself. + */ +function makeRunOpenshell( + registeredIds: readonly string[], + importResult: RunResult, + registeredBoundaryOverrides: Record> = {}, +) { + return vi.fn((args: string[]) => { + const exportIndex = args.indexOf("export"); + const probedId = exportIndex === -1 ? null : args[exportIndex + 1]; + return probedId === null + ? importResult + : registeredIds.includes(probedId) + ? { + status: 0, + stderr: "", + stdout: JSON.stringify(boundary(probedId, registeredBoundaryOverrides[probedId] ?? {})), + } + : PROFILE_ABSENT; + }); +} + +function importCalls( + runOpenshell: ReturnType, +): Array<{ args: string[]; options: Record }> { + return runOpenshell.mock.calls + .map(([args, options]) => ({ + args: args as string[], + options: options as Record, + })) + .filter(({ args }) => args.includes("import")); +} + +function importCallArgs(runOpenshell: ReturnType): string[][] { + return importCalls(runOpenshell).map(({ args }) => args); +} + +function loggedText(deps: Parameters[1]): string { + return (deps.log as ReturnType).mock.calls.flat().join("\n"); +} + function makeDeps(runOpenshell: ReturnType, overrides: Record = {}) { return { root: "/repo", @@ -23,20 +119,24 @@ function makeDeps(runOpenshell: ReturnType, overrides: Record { throw new Error(`exit:${code ?? 0}`); }), + readFileSync: makeReadFileSync(), ...overrides, - } as Parameters[1]; + } as Parameters[1]; } -describe("ensureBraveProviderProfile", () => { +describe("ensureWebSearchProviderProfiles", () => { it("does nothing when no token def is brave-typed", () => { const runOpenshell = vi.fn(); - ensureBraveProviderProfile([{ providerType: "generic", token: "tok" }], makeDeps(runOpenshell)); + ensureWebSearchProviderProfiles( + [{ providerType: "generic", token: "tok" }], + makeDeps(runOpenshell), + ); expect(runOpenshell).not.toHaveBeenCalled(); }); it("does nothing when the brave token def has no token", () => { const runOpenshell = vi.fn(); - ensureBraveProviderProfile( + ensureWebSearchProviderProfiles( [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: null }], makeDeps(runOpenshell), ); @@ -44,19 +144,25 @@ describe("ensureBraveProviderProfile", () => { }); it("imports the Brave profile from the blueprint path on first run", () => { - const runOpenshell = vi.fn(() => ({ status: 0, stderr: "", stdout: "" })); - ensureBraveProviderProfile( + const runOpenshell = makeRunOpenshell([], IMPORTED); + ensureWebSearchProviderProfiles( [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], makeDeps(runOpenshell), ); expect(runOpenshell).toHaveBeenCalledWith( - ["provider", "profile", "import", "--file", braveProviderProfilePath("/repo")], + [ + "provider", + "profile", + "import", + "--file", + webSearchProviderProfilePath("/repo", BRAVE_PROVIDER_PROFILE_ID), + ], expect.objectContaining({ ignoreError: true }), ); }); it("imports Tavily and Brave profiles when both have tokens", () => { - const runOpenshell = vi.fn(() => ({ status: 0, stderr: "", stdout: "" })); + const runOpenshell = makeRunOpenshell([], IMPORTED); ensureWebSearchProviderProfiles( [ { providerType: TAVILY_PROVIDER_PROFILE_ID, token: "tvly-test" }, @@ -64,20 +170,20 @@ describe("ensureBraveProviderProfile", () => { ], makeDeps(runOpenshell), ); - expect(runOpenshell).toHaveBeenNthCalledWith( - 1, + expect(importCallArgs(runOpenshell)).toEqual([ ["provider", "profile", "import", "--file", webSearchProviderProfilePath("/repo", "tavily")], - expect.objectContaining({ ignoreError: true }), - ); - expect(runOpenshell).toHaveBeenNthCalledWith( - 2, - ["provider", "profile", "import", "--file", braveProviderProfilePath("/repo")], - expect.objectContaining({ ignoreError: true }), - ); + [ + "provider", + "profile", + "import", + "--file", + webSearchProviderProfilePath("/repo", BRAVE_PROVIDER_PROFILE_ID), + ], + ]); }); it("uses a versioned Hermes profile instead of accepting a stale Tavily profile", () => { - const runOpenshell = vi.fn(() => ({ status: 0, stderr: "", stdout: "" })); + const runOpenshell = makeRunOpenshell([], IMPORTED); ensureWebSearchProviderProfiles( [{ providerType: HERMES_TAVILY_PROVIDER_PROFILE_ID, token: "tvly-test" }], @@ -96,15 +202,178 @@ describe("ensureBraveProviderProfile", () => { ); }); - it("treats an existing-profile diagnostic as success on re-onboard", () => { - const runOpenshell = vi.fn(() => ({ + it("skips the import when the host already registered the Brave profile (#10371)", () => { + const runOpenshell = makeRunOpenshell([BRAVE_PROVIDER_PROFILE_ID], IMPORTED); + const deps = makeDeps(runOpenshell); + + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ); + + expect(importCallArgs(runOpenshell)).toEqual([]); + expect(deps.log).not.toHaveBeenCalled(); + expect(deps.exit).not.toHaveBeenCalled(); + }); + + it("rejects an already-registered profile whose boundary doesn't match the checked-in one (#10371)", () => { + // A profile ID match alone is not proof it's the same profile this + // checkout ships — it could be a stale import from an older version, + // or an unrelated host-global registration that happens to share the + // name. Skipping on ID alone would silently trust its unverified + // endpoints, credentials, and binaries. + const runOpenshell = makeRunOpenshell([BRAVE_PROVIDER_PROFILE_ID], IMPORTED, { + [BRAVE_PROVIDER_PROFILE_ID]: { endpoints: [{ host: "attacker.example", port: 443 }] }, + }); + const deps = makeDeps(runOpenshell); + + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(importCallArgs(runOpenshell)).toEqual([]); + expect(deps.exit).toHaveBeenCalledWith(1); + }); + + it("skips the import for every web-search profile the host already registered (#10371)", () => { + const runOpenshell = makeRunOpenshell( + [TAVILY_PROVIDER_PROFILE_ID, BRAVE_PROVIDER_PROFILE_ID, HERMES_TAVILY_PROVIDER_PROFILE_ID], + IMPORTED, + ); + const deps = makeDeps(runOpenshell); + + ensureWebSearchProviderProfiles( + [ + { providerType: TAVILY_PROVIDER_PROFILE_ID, token: "tvly-test" }, + { providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }, + { providerType: HERMES_TAVILY_PROVIDER_PROFILE_ID, token: "tvly-test" }, + ], + deps, + ); + + expect(importCallArgs(runOpenshell)).toEqual([]); + expect(deps.log).not.toHaveBeenCalled(); + expect(deps.exit).not.toHaveBeenCalled(); + }); + + it("probes each provider by its own id, not a copy-pasted one (#10371)", () => { + // Only Brave is registered. A probe that hardcoded one provider's id + // instead of using the loop variable would wrongly report Tavily and + // the Hermes Tavily variant as already registered too, and skip both. + const runOpenshell = makeRunOpenshell([BRAVE_PROVIDER_PROFILE_ID], IMPORTED); + + ensureWebSearchProviderProfiles( + [ + { providerType: TAVILY_PROVIDER_PROFILE_ID, token: "tvly-test" }, + { providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }, + { providerType: HERMES_TAVILY_PROVIDER_PROFILE_ID, token: "tvly-test" }, + ], + makeDeps(runOpenshell), + ); + + expect(importCallArgs(runOpenshell)).toEqual([ + ["provider", "profile", "import", "--file", webSearchProviderProfilePath("/repo", "tavily")], + [ + "provider", + "profile", + "import", + "--file", + webSearchProviderProfilePath("/repo", HERMES_TAVILY_PROVIDER_PROFILE_ID), + ], + ]); + }); + + it("probes for an existing profile with output suppressed so a rebuild stays quiet (#10371)", () => { + const runOpenshell = makeRunOpenshell([BRAVE_PROVIDER_PROFILE_ID], IMPORTED); + + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + makeDeps(runOpenshell), + ); + + expect(runOpenshell).toHaveBeenCalledWith( + ["provider", "profile", "export", BRAVE_PROVIDER_PROFILE_ID, "--output", "json"], + expect.objectContaining({ ignoreError: true, suppressOutput: true }), + ); + }); + + it("suppresses the import's own output too, not just the probe's (#10371)", () => { + const runOpenshell = makeRunOpenshell([], IMPORTED); + + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + makeDeps(runOpenshell), + ); + + expect(importCalls(runOpenshell)).toEqual([ + { + args: [ + "provider", + "profile", + "import", + "--file", + webSearchProviderProfilePath("/repo", BRAVE_PROVIDER_PROFILE_ID), + ], + options: expect.objectContaining({ suppressOutput: true }), + }, + ]); + }); + + /** + * A race has three calls in sequence: the initial probe (not found), the + * import (collides with a concurrent winner), and the post-race re-export + * that must now find the concurrent winner's matching profile — simulated + * with a call-order counter rather than `makeRunOpenshell`'s id-keyed + * lookup, since the same id genuinely answers differently before and + * after the race. + */ + function makeRaceRunOpenshell(importResult: RunResult) { + let exportCalls = 0; + return vi.fn((args: string[]) => { + const isExport = args.includes("export"); + exportCalls += isExport ? 1 : 0; + return !isExport + ? importResult + : exportCalls === 1 + ? PROFILE_ABSENT + : { status: 0, stderr: "", stdout: JSON.stringify(boundary(BRAVE_PROVIDER_PROFILE_ID)) }; + }); + } + + it("treats an existing-profile diagnostic as success when an import loses a race", () => { + const runOpenshell = makeRaceRunOpenshell({ status: 1, stderr: "custom provider profile 'brave' already exists", stdout: "", - })); + }); + const deps = makeDeps(runOpenshell); + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).not.toThrow(); + expect(importCallArgs(runOpenshell)).toHaveLength(1); + expect(deps.exit).not.toHaveBeenCalled(); + }); + + it("tolerates the existing-profile diagnostic across a wrapped, box-drawn terminal line (#10371)", () => { + // OpenShell can wrap styled output across a box-drawing continuation + // (│) depending on terminal width/TTY-ness (#10159's failure shape). + // The plain, unnormalized substring test that guarded this same race + // before #10371 would miss "already exists" split across a line break + // like this and fall through to a hard failure instead of tolerating + // the race. + const runOpenshell = makeRaceRunOpenshell({ + status: 1, + stderr: "custom provider profile 'brave' already\n │ exists", + stdout: "", + }); const deps = makeDeps(runOpenshell); expect(() => - ensureBraveProviderProfile( + ensureWebSearchProviderProfiles( [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], deps, ), @@ -116,27 +385,191 @@ describe("ensureBraveProviderProfile", () => { ); }); + it("rejects a race winner whose profile doesn't match the checked-in boundary (#10371)", () => { + let exportCalls = 0; + const runOpenshell = vi.fn((args: string[]) => { + const isExport = args.includes("export"); + exportCalls += isExport ? 1 : 0; + return !isExport + ? { status: 1, stderr: "custom provider profile 'brave' already exists", stdout: "" } + : exportCalls === 1 + ? PROFILE_ABSENT + : { + status: 0, + stderr: "", + stdout: JSON.stringify( + boundary(BRAVE_PROVIDER_PROFILE_ID, { inference_capable: true }), + ), + }; + }); + const deps = makeDeps(runOpenshell); + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(deps.exit).toHaveBeenCalledWith(1); + }); + it("exits with the OpenShell status when import fails for a non-idempotent reason", () => { - const runOpenshell = vi.fn(() => ({ + const runOpenshell = makeRunOpenshell([], { status: 2, stderr: "schema validation error: missing endpoints", stdout: "", - })); + }); const deps = makeDeps(runOpenshell); expect(() => - ensureBraveProviderProfile( + ensureWebSearchProviderProfiles( [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], deps, ), ).toThrow(/exit:2/); expect(deps.exit).toHaveBeenCalledWith(2); }); + + it("stops without importing when the probe fails for a reason other than a missing profile (#10371)", () => { + // An unreachable gateway or an unauthorized account both return a + // nonzero export status, exactly like a genuinely missing profile does + // — but proceeding to import in either case would attempt a state- + // changing operation in response to a read that never actually told us + // whether the profile exists. + const runOpenshell = vi.fn((args: string[]) => + args.includes("export") + ? { status: 1, stderr: "gateway unreachable: connection refused", stdout: "" } + : IMPORTED, + ); + const deps = makeDeps(runOpenshell); + + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(importCallArgs(runOpenshell)).toEqual([]); + expect(deps.exit).toHaveBeenCalledWith(1); + }); + + it("reports a redacted cause without importing when the probe times out (#10371)", () => { + const runOpenshell = vi.fn((args: string[]) => + args.includes("export") + ? { + status: null, + stderr: "", + stdout: "", + error: new Error("spawnSync openshell ETIMEDOUT secret-value"), + } + : IMPORTED, + ); + const deps = makeDeps(runOpenshell, { + redact: (text: string) => text.replaceAll("secret-value", "[REDACTED]"), + }); + + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(importCallArgs(runOpenshell)).toEqual([]); + expect(loggedText(deps)).toContain("ETIMEDOUT"); + expect(loggedText(deps)).toContain("[REDACTED]"); + expect(loggedText(deps)).not.toContain("secret-value"); + }); + + it("stops without importing when the post-race re-export fails (#10371)", () => { + let exportCalls = 0; + const runOpenshell = vi.fn((args: string[]) => { + const isExport = args.includes("export"); + exportCalls += isExport ? 1 : 0; + return !isExport + ? { status: 1, stderr: "custom provider profile 'brave' already exists", stdout: "" } + : exportCalls === 1 + ? PROFILE_ABSENT + : { status: 1, stderr: "gateway unreachable: connection refused", stdout: "" }; + }); + const deps = makeDeps(runOpenshell); + + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(deps.exit).toHaveBeenCalledWith(1); + }); + + it("passes the bounded OpenShell operation timeout to the probe and the import (#10371)", () => { + const runOpenshell = makeRunOpenshell([], IMPORTED); + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + makeDeps(runOpenshell), + ); + const calls = runOpenshell.mock.calls as unknown as Array<[string[], { timeout?: number }]>; + const timeouts = calls.map(([, options]) => options.timeout); + expect(timeouts).toEqual(calls.map(() => OPENSHELL_OPERATION_TIMEOUT_MS)); + }); + + it("does not report an unreadable checked-in profile as drift (#10371)", () => { + // Reading our own YAML can fail for reasons that say nothing about the + // registered profile. Reporting that as drift sends the operator to + // delete a profile whose contents were never compared. + const runOpenshell = makeRunOpenshell([BRAVE_PROVIDER_PROFILE_ID], IMPORTED); + const deps = makeDeps(runOpenshell, { + readFileSync: vi.fn(() => { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }); + }), + }); + + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(loggedText(deps)).not.toContain("delete"); + expect(importCallArgs(runOpenshell)).toEqual([]); + }); + + it("does not report an export that is not JSON as drift (#10371)", () => { + const runOpenshell = vi.fn((args: string[]) => + args.includes("export") + ? { status: 0, stderr: "", stdout: "profile export interrupted" } + : IMPORTED, + ); + const deps = makeDeps(runOpenshell); + + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(loggedText(deps)).not.toContain("delete"); + }); + + it("does not report valid JSON with no provider boundary as drift (#10371)", () => { + const runOpenshell = vi.fn((args: string[]) => + args.includes("export") ? { status: 0, stderr: "", stdout: "{}" } : IMPORTED, + ); + const deps = makeDeps(runOpenshell); + + expect(() => + ensureWebSearchProviderProfiles( + [{ providerType: BRAVE_PROVIDER_PROFILE_ID, token: "brv-test" }], + deps, + ), + ).toThrow(/exit:1/); + expect(loggedText(deps)).not.toContain("delete"); + expect(importCallArgs(runOpenshell)).toEqual([]); + }); }); -describe("shouldEnableBraveWebSearch", () => { +describe("shouldEnableWebSearch", () => { it("returns false for null/undefined web search config", () => { - expect(shouldEnableBraveWebSearch(null)).toBe(false); - expect(shouldEnableBraveWebSearch(undefined)).toBe(false); + expect(shouldEnableWebSearch(null)).toBe(false); + expect(shouldEnableWebSearch(undefined)).toBe(false); }); it("returns false when fetchEnabled is missing or falsy", () => { @@ -144,12 +577,12 @@ describe("shouldEnableBraveWebSearch", () => { // tripped `if (webSearchConfig)` in createSandbox and pushed a Brave // provider/token plus the BRAVE_API_KEY abort even though the runtime // gate downstream is `fetchEnabled`. - expect(shouldEnableBraveWebSearch({})).toBe(false); - expect(shouldEnableBraveWebSearch({ fetchEnabled: false })).toBe(false); - expect(shouldEnableBraveWebSearch({ fetchEnabled: null })).toBe(false); + expect(shouldEnableWebSearch({})).toBe(false); + expect(shouldEnableWebSearch({ fetchEnabled: false })).toBe(false); + expect(shouldEnableWebSearch({ fetchEnabled: null })).toBe(false); }); it("returns true only when fetchEnabled is explicitly true", () => { - expect(shouldEnableBraveWebSearch({ fetchEnabled: true })).toBe(true); + expect(shouldEnableWebSearch({ fetchEnabled: true })).toBe(true); }); }); diff --git a/src/lib/onboard/brave-provider-profile.ts b/src/lib/onboard/brave-provider-profile.ts index 7365f7eea5a..b6781acf4d8 100644 --- a/src/lib/onboard/brave-provider-profile.ts +++ b/src/lib/onboard/brave-provider-profile.ts @@ -1,8 +1,17 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import fs from "node:fs"; import path from "node:path"; +import { OPENSHELL_OPERATION_TIMEOUT_MS } from "../adapters/openshell/provider-command"; +import type { CheckedInBoundaryComparison } from "../adapters/openshell/provider-profile"; +import { + compareExportedProfileToCheckedIn, + isMissingProviderProfile, + normalizeOpenshellDiagnostic, + openshellResultDiagnostic, +} from "../adapters/openshell/provider-profile"; import { compactText } from "../core/url-utils"; import { isWebSearchEnabled } from "../inference/web-search"; @@ -20,38 +29,39 @@ export const WEB_SEARCH_PROVIDER_PROFILE_IDS = [ export type WebSearchProviderProfileId = (typeof WEB_SEARCH_PROVIDER_PROFILE_IDS)[number]; /** - * Single source of truth for "the user opted in to Brave Search at runtime." + * Single source of truth for "the user opted in to web search at runtime." * Returning true on a config whose `fetchEnabled` is false would cause - * `createSandbox` to push a Brave provider/token and trip the BRAVE_API_KEY- - * required abort even when the feature is off, while the downstream + * `createSandbox` to push a web-search provider/token and trip the required + * abort even when the feature is off, while the downstream * finalization/verifier paths already gate on `fetchEnabled`. Keep every gate * routed through this helper so they stay aligned. */ -export function shouldEnableBraveWebSearch( - webSearchConfig: { fetchEnabled?: boolean | null } | null | undefined, -): boolean { - return shouldEnableWebSearch(webSearchConfig); -} - export function shouldEnableWebSearch( webSearchConfig: { fetchEnabled?: boolean | null } | null | undefined, ): boolean { return isWebSearchEnabled(webSearchConfig as { fetchEnabled: boolean } | null | undefined); } -export type BraveProviderProfileDeps = { +type RunOpenshellResult = { + status: number | null; + stderr?: string | Buffer | null; + stdout?: string | Buffer | null; + error?: Error; +}; + +export type WebSearchProviderProfileDeps = { root: string; runOpenshell: ( args: string[], - // The runner accepts a wider options shape; we only set ignoreError + - // stdio here, so erase the type at the boundary to keep this module - // free of the runner.ts internals. + // The runner accepts a wider options shape. Keep this module free of the + // runner.ts internals by erasing that shape at the injected boundary. // eslint-disable-next-line @typescript-eslint/no-explicit-any opts: any, - ) => { status: number | null; stderr?: string | Buffer | null; stdout?: string | Buffer | null }; + ) => RunOpenshellResult; redact: (input: string) => string; log?: (message?: string) => void; exit?: (code?: number) => never; + readFileSync?: (file: string) => string; }; type TokenDefShape = { providerType?: string; token: string | null }; @@ -63,8 +73,30 @@ function bufferOrStringToText(value: string | Buffer | null | undefined): string return ""; } -export function braveProviderProfilePath(root: string): string { - return webSearchProviderProfilePath(root, "brave"); +/** + * Whether an exported profile is the one this codebase ships for `provider`, + * comparing its credential boundary (endpoints, binaries, credential rewrite + * rules, inference_capable). A profile ID match alone is not proof of this: + * OpenShell custom profiles are immutable after import, but that says nothing + * about what content some other process (an older NemoClaw version, a + * different tool, an unrelated host-global registration) imported under the + * same ID before this run ever probed it. Skipping re-import on ID match alone + * would silently trust that unverified boundary (#10371). + * + * Compares through OpenShell's export representation, and distinguishes a read + * that never completed from confirmed drift. + */ +function compareWebSearchProfileToCheckedIn( + root: string, + provider: WebSearchProviderProfileId, + exportedJson: string, + readFileSync: (file: string) => string, +): CheckedInBoundaryComparison { + return compareExportedProfileToCheckedIn( + exportedJson, + () => readFileSync(webSearchProviderProfilePath(root, provider)), + provider, + ); } export function webSearchProviderProfilePath( @@ -74,24 +106,10 @@ export function webSearchProviderProfilePath( return path.join(root, "nemoclaw-blueprint", "provider-profiles", `${provider}.yaml`); } -/** - * Register the Brave Search provider profile with OpenShell so providers - * created with `--type brave` drive the L7 proxy's X-Subscription-Token - * rewrite. Skipped unless at least one token definition is Brave-typed and - * has a usable token. Idempotent: tolerates OpenShell reporting that the - * custom profile is already registered. - */ -export function ensureBraveProviderProfile( - tokenDefs: readonly TokenDefShape[], - deps: BraveProviderProfileDeps, -): void { - ensureWebSearchProviderProfiles(tokenDefs, deps); -} - /** Register every selected web-search provider profile before token upsert. */ export function ensureWebSearchProviderProfiles( tokenDefs: readonly TokenDefShape[], - deps: BraveProviderProfileDeps, + deps: WebSearchProviderProfileDeps, ): void { const neededProviders = new Set(); for (const { providerType, token } of tokenDefs) { @@ -107,8 +125,104 @@ export function ensureWebSearchProviderProfiles( const errorLog = deps.log ?? console.error; const exit = deps.exit ?? ((code?: number) => process.exit(code)); + const readFileSync = deps.readFileSync ?? ((file: string) => fs.readFileSync(file, "utf-8")); + + const rejectDriftedProfile = (provider: WebSearchProviderProfileId): never => { + errorLog( + `\n ✗ The '${provider}' OpenShell provider profile already registered in the selected ` + + "OpenShell gateway does not match the profile this NemoClaw checkout ships.", + ); + errorLog( + " Its endpoints, binaries, credential rules, or inference capability differ from " + + `${webSearchProviderProfilePath(deps.root, provider)}.`, + ); + errorLog( + " Find the selected gateway's name with 'openshell gateway info', then remove the " + + `conflicting profile from that gateway (openshell provider profile -g ` + + `delete ${provider}) and re-run onboarding. Other sandboxes that use the same gateway may ` + + "share this profile — confirm the effect before removing it.", + ); + return exit(1); + }; + + const rejectUnverifiableProfile = (provider: WebSearchProviderProfileId): never => { + errorLog( + `\n ✗ Could not verify the '${provider}' OpenShell provider profile already registered in ` + + "the selected OpenShell gateway against the profile this NemoClaw checkout ships.", + ); + errorLog( + " The gateway's export was not readable as JSON, or " + + `${webSearchProviderProfilePath(deps.root, provider)} could not be read as a provider ` + + "profile. An unfinished check is not proof the registered profile drifted, so it was " + + "left in place. Resolve the read failure and re-run onboarding.", + ); + return exit(1); + }; + + const rejectProbeFailure = ( + provider: WebSearchProviderProfileId, + operation: string, + rawDiagnostic: string, + ): never => { + const diagnostic = compactText(deps.redact(rawDiagnostic)); + errorLog( + `\n ✗ Could not check whether the ${provider} web-search provider profile is already ` + + `registered (${operation} failed).`, + ); + if (diagnostic) errorLog(` ${diagnostic.slice(0, 500)}`); + errorLog( + " Confirm the OpenShell gateway is reachable and this account is authorized, then re-run onboarding.", + ); + return exit(1); + }; + + const exportProfile = (provider: WebSearchProviderProfileId) => + deps.runOpenshell(["provider", "profile", "export", provider, "--output", "json"], { + ignoreError: true, + suppressOutput: true, + stdio: ["ignore", "pipe", "pipe"], + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }); for (const provider of neededProviders) { + // These profiles live in one host-global OpenShell custom-profile store, so + // every rebuild and every additional sandbox re-imports what the first + // onboard already registered. Probe first and skip the re-import — the same + // idiom ensureMessagingBridgeProfiles() uses for the same reason — so + // OpenShell's "already exists" diagnostic stops reaching the terminal on + // routine rebuilds. A fresh host answers the probe with a harmless + // "not found" that suppressOutput hides. + const alreadyRegistered = exportProfile(provider); + if (alreadyRegistered.status === 0) { + // A profile ID match is not proof this is the checked-in profile: immutability + // after import says nothing about what content was imported under + // this ID before this run ever probed it. Verify the boundary before + // trusting it (#10371). + const comparison = compareWebSearchProfileToCheckedIn( + deps.root, + provider, + bufferOrStringToText(alreadyRegistered.stdout), + readFileSync, + ); + if (comparison === "indeterminate") rejectUnverifiableProfile(provider); + if (comparison === "mismatch") rejectDriftedProfile(provider); + continue; + } + + // A nonzero probe status alone is not proof the profile is missing — the + // gateway could be unreachable, this account unauthorized, or the probe + // could have timed out or spawned incorrectly. Only a recognized + // "not found" diagnostic makes it safe to proceed to import; anything + // else must stop here rather than attempt a state-changing import in + // response to a read that never actually completed (#10371). + const probeDiagnostic = openshellResultDiagnostic(alreadyRegistered); + if ( + !Number.isInteger(alreadyRegistered.status) || + !isMissingProviderProfile(probeDiagnostic, provider) + ) { + rejectProbeFailure(provider, "provider profile export", probeDiagnostic); + } + const result = deps.runOpenshell( [ "provider", @@ -117,21 +231,52 @@ export function ensureWebSearchProviderProfiles( "--file", webSearchProviderProfilePath(deps.root, provider), ], - { ignoreError: true, stdio: ["ignore", "pipe", "pipe"], suppressOutput: true }, + { + ignoreError: true, + suppressOutput: true, + stdio: ["ignore", "pipe", "pipe"], + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }, ); if (result.status === 0) continue; - // OpenShell reports re-imports of an already-registered custom profile as - // a non-zero exit. Tolerate that so re-onboard / recreate keeps working. - const rawDiagnostic = `${bufferOrStringToText(result.stderr)} ${bufferOrStringToText(result.stdout)}`; - if (/already exists/i.test(rawDiagnostic)) continue; + // Reconcile a lost race: the probe saw no profile but a concurrent import + // registered it first. OpenShell reports that re-import as a non-zero exit, + // so tolerate it and keep re-onboard / recreate working. Normalize wrapped + // diagnostics so race recovery recognizes `already exists` regardless of + // terminal width or TTY output (#10159, #10371). + const rawDiagnostic = openshellResultDiagnostic(result); + if (/already exists/iu.test(normalizeOpenshellDiagnostic(rawDiagnostic))) { + // The race winner might not be our own import: re-export and verify + // the boundary before treating the race as resolved, exactly as the + // initial probe above does. + const raced = exportProfile(provider); + if (raced.status !== 0) { + rejectProbeFailure( + provider, + "post-race provider profile export", + openshellResultDiagnostic(raced), + ); + } + const racedComparison = compareWebSearchProfileToCheckedIn( + deps.root, + provider, + bufferOrStringToText(raced.stdout), + readFileSync, + ); + if (racedComparison === "indeterminate") rejectUnverifiableProfile(provider); + if (racedComparison === "mismatch") rejectDriftedProfile(provider); + continue; + } const diagnostic = compactText(deps.redact(rawDiagnostic)); errorLog( `\n ✗ Failed to register the ${provider} web-search provider profile with OpenShell.`, ); if (diagnostic) errorLog(` ${diagnostic.slice(0, 500)}`); - errorLog(" Update OpenShell with scripts/install-openshell.sh and re-run onboarding."); + errorLog( + " Fix the error above. If OpenShell requires an update, rerun the NemoClaw installer. Then rerun onboarding.", + ); exit(result.status || 1); } } diff --git a/src/lib/onboard/command.test.ts b/src/lib/onboard/command.test.ts index e43e394cc23..8b51201dc93 100644 --- a/src/lib/onboard/command.test.ts +++ b/src/lib/onboard/command.test.ts @@ -13,6 +13,7 @@ import { servingProfileProvenance } from "../inference/serving/profile-provenanc import { NEMOCLAW_VLLM_GPU_DEVICE_ENV } from "../inference/vllm-models"; import { resolveOnboardOptions, runOnboardCommand, servingProfileProviderKey } from "./command"; import type { OnboardFlags } from "./command-support"; +import { UnverifiableStaticProviderProfileError } from "./credential-provider-registration"; import { PortableInferenceDescriptorError } from "./experimental/portable-inference-descriptor"; import { invalidGatewayManagementDeclarationError } from "./gateway-management"; import { GatewayAuthorityError } from "./gateway-teardown-authority"; @@ -1238,6 +1239,27 @@ describe("onboard command options", () => { expect(output).not.toContain(" at "); }); + it("prints a clean CLI error when a static provider profile cannot be verified", async () => { + const errors: string[] = []; + const onboardError = new UnverifiableStaticProviderProfileError("selected-gateway"); + expect(onboardError.message).toContain("No provider profile was changed"); + await expect( + runOnboardCommand({ + flags: {}, + env: {}, + runOnboard: async () => { + throw onboardError; + }, + error: (message = "") => errors.push(message), + exit: exitWithCode, + }), + ).rejects.toThrow("exit:1"); + + const output = errors.join("\n"); + expect(output).toContain("OpenShell gateway 'selected-gateway'"); + expect(output).not.toContain(" at "); + }); + it("redacts credentials in a gateway declaration diagnostic (#9035)", async () => { const errors: string[] = []; await expect( diff --git a/src/lib/onboard/command.ts b/src/lib/onboard/command.ts index 90af6b148bc..bed33ff2ed8 100644 --- a/src/lib/onboard/command.ts +++ b/src/lib/onboard/command.ts @@ -547,6 +547,12 @@ function handleOnboardCommandError(error: unknown, deps: RunOnboardCommandDeps): if (error instanceof PortableInferenceDescriptorError) { return reportOnboardCommandError(deps, ` ${error.message}`); } + if ( + error instanceof Error && + (error as Error & { code?: unknown }).code === "NEMOCLAW_UNVERIFIABLE_STATIC_PROVIDER_PROFILE" + ) { + return reportOnboardCommandError(deps, ` ${error.message}`); + } // Gateway-authority refusals are reported, never rethrown. Recreation is not // selected in one place: `--recreate-sandbox` sets the flag, but `runOnboard` // independently honours NEMOCLAW_RECREATE_SANDBOX and reaches the same diff --git a/src/lib/onboard/credential-provider-registration.test.ts b/src/lib/onboard/credential-provider-registration.test.ts index 2095e40c984..71639ecf889 100644 --- a/src/lib/onboard/credential-provider-registration.test.ts +++ b/src/lib/onboard/credential-provider-registration.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it, vi } from "vitest"; +import { OPENSHELL_OPERATION_TIMEOUT_MS } from "../adapters/openshell/provider-command"; import type { SandboxMessagingPlan } from "../messaging/manifest"; import type { Session } from "../state/onboard-session"; import { requiredMessagingProviderBindings } from "./checkpoint-replay"; @@ -207,7 +208,10 @@ describe("credential provider registration", () => { "--output", "json", ], - expect.objectContaining({ suppressOutput: true }), + expect.objectContaining({ + suppressOutput: true, + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }), ); expect( runOpenshell.mock.calls @@ -217,6 +221,53 @@ describe("credential provider registration", () => { }, ); + it("reports an unreadable static profile separately from confirmed drift", () => { + const session = { stagedCredentialProviders: [] } as unknown as Session; + const runOpenshell = vi.fn((args: string[]) => + args.includes("profile") && args.includes("export") + ? { status: 1, stdout: "", stderr: "gateway unavailable" } + : providerMetadata("alpha-discord-bridge", "discord-hermes-static-v1", "DISCORD_BOT_TOKEN"), + ); + const deps = registrationDeps(runOpenshell, session); + deps.root = process.cwd(); + const registration = createCredentialProviderRegistration(deps); + + expect(() => + registration.providerMatchesGatewayCredential( + "alpha-discord-bridge", + "discord-hermes-static-v1", + "DISCORD_BOT_TOKEN", + ), + ).toThrow( + "static provider profile for OpenShell gateway 'test-gateway' against its checked-in " + + "credential boundary", + ); + }); + + it("treats a missing static profile as unavailable instead of unreadable", () => { + const session = { stagedCredentialProviders: [] } as unknown as Session; + const runOpenshell = vi.fn((args: string[]) => + args.includes("profile") && args.includes("export") + ? { + status: 1, + stdout: "", + stderr: "custom provider profile 'discord-hermes-static-v1' not found", + } + : providerMetadata("alpha-discord-bridge", "discord-hermes-static-v1", "DISCORD_BOT_TOKEN"), + ); + const deps = registrationDeps(runOpenshell, session); + deps.root = process.cwd(); + const registration = createCredentialProviderRegistration(deps); + + expect( + registration.providerMatchesGatewayCredential( + "alpha-discord-bridge", + "discord-hermes-static-v1", + "DISCORD_BOT_TOKEN", + ), + ).toBe(false); + }); + it("uses one selected gateway for static profile and provider identity", () => { const session = { stagedCredentialProviders: [] } as unknown as Session; const commandResults = new Map([ @@ -380,6 +431,14 @@ describe("credential provider registration", () => { "provider get -g test-gateway alpha-discord-bridge", providerMetadata("alpha-discord-bridge", "generic", "DISCORD_BOT_TOKEN"), ], + // Not registered yet, so ensureWebSearchProviderProfiles (#10371) takes + // the import path below instead of its already-registered boundary + // check — this test is about provider/receipt registration, not + // profile-content validation. + [ + "provider profile -g test-gateway export brave --output json", + { status: 1, stdout: "", stderr: "custom provider profile not found" }, + ], ]); const defaultResult = { status: 0, stdout: "", stderr: "" }; const runOpenshell = vi.fn( @@ -452,7 +511,7 @@ describe("credential provider registration", () => { it("creates a missing messaging provider and records its receipt (#6743)", async () => { const session = { stagedCredentialProviders: [] } as unknown as Session; - const missing = { status: 1, stdout: "", stderr: "not found" }; + const missing = { status: 1, stdout: "", stderr: "custom provider profile not found" }; const success = { status: 0, stdout: "", stderr: "" }; const runOpenshell = vi.fn((args: string[]) => args[0] === "provider" && args[1] === "get" ? missing : success, @@ -496,7 +555,7 @@ describe("credential provider registration", () => { it("registers one static Hermes Discord provider from the checkpoint binding", async () => { const session = { stagedCredentialProviders: [] } as unknown as Session; - const missing = { status: 1, stdout: "", stderr: "not found" }; + const missing = { status: 1, stdout: "", stderr: "custom provider profile not found" }; const success = { status: 0, stdout: "", stderr: "" }; const runOpenshell = vi.fn((args: string[]) => (args[0] === "provider" && args.includes("profile") && args.includes("export")) || @@ -630,7 +689,7 @@ describe("credential provider registration", () => { const session = { stagedCredentialProviders: ["alpha-slack-bridge", "alpha-slack-app"], } as unknown as Session; - const missing = { status: 1, stdout: "", stderr: "not found" }; + const missing = { status: 1, stdout: "", stderr: "custom provider profile not found" }; const success = { status: 0, stdout: "", stderr: "" }; const responses = new Map([ [ @@ -720,7 +779,7 @@ describe("credential provider registration", () => { const session = { stagedCredentialProviders: ["alpha-slack-bridge", "alpha-slack-app"], } as unknown as Session; - const missing = { status: 1, stdout: "", stderr: "not found" }; + const missing = { status: 1, stdout: "", stderr: "custom provider profile not found" }; const success = { status: 0, stdout: "", stderr: "" }; const responses = new Map([ ["provider get -g test-gateway alpha-slack-bridge", missing], diff --git a/src/lib/onboard/credential-provider-registration.ts b/src/lib/onboard/credential-provider-registration.ts index 363d36ea112..6fe210a0dd1 100644 --- a/src/lib/onboard/credential-provider-registration.ts +++ b/src/lib/onboard/credential-provider-registration.ts @@ -125,6 +125,21 @@ export interface CredentialProviderRegistrationDeps { persistMigratedLegacyKeys(): void; } +export class UnverifiableStaticProviderProfileError extends Error { + readonly code = "NEMOCLAW_UNVERIFIABLE_STATIC_PROVIDER_PROFILE"; + + constructor(gatewayName: string) { + super( + "The gateway export or checked-in profile was unavailable or invalid. NemoClaw could not " + + `verify the static provider profile for OpenShell gateway '${gatewayName}' against its ` + + "checked-in credential boundary. No provider profile was changed. Confirm the gateway is " + + "reachable, this account is authorized, and the checked-in profile is readable and valid. " + + "Then rerun onboarding.", + ); + this.name = "UnverifiableStaticProviderProfileError"; + } +} + function recordMigratedLegacyMessagingCredentials( tokenDefs: readonly MessagingTokenDef[], registeredProviderNames: readonly string[], @@ -279,7 +294,10 @@ export function createCredentialProviderRegistration(deps: CredentialProviderReg binding.type, { root: deps.root, runOpenshell }, ); - if (staticProfileMatches === false) return false; + if (staticProfileMatches === "indeterminate") { + throw new UnverifiableStaticProviderProfileError(deps.getGatewayName()); + } + if (staticProfileMatches === "absent" || staticProfileMatches === "mismatch") return false; return gatewayProviderMetadata.matchesGatewayCredentialFamilyProviderBinding( providers.readGatewayProviderMetadata(binding.name, runOpenshell, deps.getGatewayName()), { diff --git a/src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts b/src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts index 916220cb203..e5e374fed69 100644 --- a/src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts +++ b/src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts @@ -111,8 +111,28 @@ function fakeGatewayRunOpenshell() { return OK_RESULT; }; + const PROFILE_NOT_FOUND: StubbedRunOpenshellResult = { + status: 1, + stdout: "", + stderr: "custom provider profile not found", + }; + // Export probes only the built-in messaging-bridge profile this fixture + // already registers (id "nemoclaw-mcp-v1"); any other profile id + // (brave/tavily/tavily-hermes-v1) is a fresh host that has never imported + // it, matching this fixture's original always-import intent for + // web-search provider registration (#10371). Look up "export" by index, + // not a fixed position, since gateway scoping inserts "-g " + // between "profile" and "export". + const handleProfile = (args: string[]): StubbedRunOpenshellResult => { + const exportIndex = args.indexOf("export"); + const probedId = exportIndex === -1 ? null : args[exportIndex + 1]; + return probedId !== null && probedId !== "nemoclaw-mcp-v1" + ? PROFILE_NOT_FOUND + : EXACT_MESSAGING_PROFILE; + }; + const handlersByAction: Record StubbedRunOpenshellResult> = { - profile: () => EXACT_MESSAGING_PROFILE, + profile: handleProfile, get: handleGet, create: handleCreate, update: () => OK_RESULT, @@ -1126,6 +1146,39 @@ describe("sandbox crash-recovery replay (#5961, #6228)", () => { expect(calls.error.mock.calls.flat().join("\n")).toContain("my-assistant-brave-search"); }); + it("stops checkpoint recovery when a registered provider cannot be verified", async () => { + const session = sessionWithCheckpoint( + crashedCheckpoint({ + bindings: { + credentialEnvs: [], + registeredProviders: [ + { + name: "my-assistant-discord-bridge", + type: "discord-hermes-static-v1", + credentialEnv: "DISCORD_BOT_TOKEN", + }, + ], + }, + }), + ); + const { deps, calls } = createDeps({ + getSandboxReuseState: () => "missing", + providerMatchesGatewayCredential: () => { + throw new Error("registered provider profile could not be verified"); + }, + }); + + await expect( + handleSandboxState({ + ...baseOptions(deps, session), + resume: true, + sandboxName: "my-assistant", + }), + ).rejects.toThrow("registered provider profile could not be verified"); + + expect(calls.createSandbox).not.toHaveBeenCalled(); + }); + it("creates the missing sandbox when its exact registered provider binding remains live (#7022)", async () => { const providerMatchesGatewayCredential = vi.fn(() => true); const session = sessionWithCheckpoint( diff --git a/src/lib/onboard/messaging-bridge-provider.test.ts b/src/lib/onboard/messaging-bridge-provider.test.ts index 7ad51053a1c..86038125e35 100644 --- a/src/lib/onboard/messaging-bridge-provider.test.ts +++ b/src/lib/onboard/messaging-bridge-provider.test.ts @@ -4,6 +4,8 @@ import fs from "node:fs"; import { describe, expect, it, vi } from "vitest"; import YAML from "yaml"; +import { OPENSHELL_OPERATION_TIMEOUT_MS } from "../adapters/openshell/provider-command"; +import { createBuiltInChannelManifestRegistry } from "../messaging/channels"; import { bridgeProviderNamesForChannel, bridgeSecretEnvsForChannel, @@ -60,6 +62,27 @@ const GC_HERMES_PROFILE: MessagingBridgeProfile = { profileId: "google-chat-bridge-hermes", }; +// Synthetic exported-profile JSON matching GC_PROFILE's checked-in boundary — +// unlike DISCORD_PROFILE_DOC, a refreshing profile legitimately grants +// endpoint/binary authority, so this is not the empty-endpoints/binaries +// static-profile shape. +const GC_PROFILE_DOC = { + id: GC_PROFILE.profileId, + credentials: [ + { + name: "access_token", + env_vars: [GC_PROFILE.credentialKey], + required: true, + auth_style: "header", + header_name: "Authorization", + query_param: "", + }, + ], + endpoints: [{ host: "chat.googleapis.com", port: 443, protocol: "rest", access: "read-write" }], + binaries: ["/usr/bin/node"], + inference_capable: false, +}; + const DISCORD_PROFILE: MessagingBridgeProfile = { channelId: "discord", agent: "hermes", @@ -93,6 +116,23 @@ const DISCORD_PROFILE_DOC = { inference_capable: false, }; +const DISCORD_MANIFEST = createBuiltInChannelManifestRegistry() + .list() + .find((manifest) => manifest.id === DISCORD_PROFILE.channelId)!; +const SYNTHETIC_DISCORD_MANIFEST = { + ...DISCORD_MANIFEST, + supportedAgents: [DISCORD_PROFILE.agent], +}; + +function discoverSyntheticDiscordProfile(doc: Record) { + return listMessagingBridgeProfiles({ + root: "/repo", + manifests: [SYNTHETIC_DISCORD_MANIFEST], + existsSync: () => true, + readFileSync: () => YAML.stringify(doc), + }); +} + const STATIC_DEF = { name: "sbx-discord-bridge", providerType: DISCORD_PROFILE.profileId, @@ -434,9 +474,9 @@ describe("configureMessagingBridgeRefreshes", () => { expect(result.ok).toBe(false); // Six probes at a minute each cross the five-minute deadline well before // the fifty-attempt cap. - expect( - runOpenshell.mock.calls.filter((call) => call[0][2] === "status").length, - ).toBeLessThan(10); + expect(runOpenshell.mock.calls.filter((call) => call[0][2] === "status").length).toBeLessThan( + 10, + ); }); it("bounds each status probe with a command timeout", () => { @@ -490,7 +530,9 @@ describe("ensureMessagingBridgeProfiles", () => { it("imports the profile from its co-located path when not yet registered", () => { const runOpenshell = vi.fn((args: string[], _opts: unknown) => - args.includes("export") ? { status: 1 } : { status: 0 }, + args.includes("export") + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 0 }, ); const exit = vi.fn(() => undefined as never); ensureMessagingBridgeProfiles([BRIDGE_DEF], { ...baseDeps(), runOpenshell, exit }); @@ -503,9 +545,17 @@ describe("ensureMessagingBridgeProfiles", () => { it("skips the import when the profile is already registered", () => { // A fresh onboard registers bridge providers twice; the second pass must not // re-import and trigger OpenShell's "already exists / import failed" output. - const runOpenshell = vi.fn((_args: string[], _opts: unknown) => ({ status: 0 })); + const runOpenshell = vi.fn((_args: string[], _opts: unknown) => ({ + status: 0, + stdout: JSON.stringify(GC_PROFILE_DOC), + })); const exit = vi.fn(() => undefined as never); - ensureMessagingBridgeProfiles([BRIDGE_DEF], { ...baseDeps(), runOpenshell, exit }); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + }); expect(runOpenshell.mock.calls.some((call) => call[0].includes("import"))).toBe(false); const exportCall = runOpenshell.mock.calls.find((call) => call[0].includes("export")); expect(exportCall?.[0]).toEqual([ @@ -583,7 +633,7 @@ describe("ensureMessagingBridgeProfiles", () => { it("rejects a mismatched static profile that wins an import race", () => { const runOpenshell = vi .fn() - .mockReturnValueOnce({ status: 1 }) + .mockReturnValueOnce({ status: 1, stderr: "custom provider profile not found" }) .mockReturnValueOnce({ status: 1, stderr: "profile already exists" }) .mockReturnValueOnce({ status: 0, @@ -604,18 +654,334 @@ describe("ensureMessagingBridgeProfiles", () => { }); it("tolerates an already-registered profile without exiting", () => { - const runOpenshell = vi.fn(() => ({ status: 1, stderr: "profile already exists" })); + // First export: not yet registered. Import: lost the race. Post-race + // export: the winning profile matches the checked-in boundary. + let exportCalls = 0; + const runOpenshell = vi.fn((args: string[]) => + !args.includes("export") + ? { status: 1, stderr: "profile already exists" } + : (exportCalls += 1) === 1 + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 0, stdout: JSON.stringify(GC_PROFILE_DOC) }, + ); const exit = vi.fn(() => undefined as never); - ensureMessagingBridgeProfiles([BRIDGE_DEF], { ...baseDeps(), runOpenshell, exit }); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + }); expect(exit).not.toHaveBeenCalled(); }); it("exits when profile import fails for another reason", () => { - const runOpenshell = vi.fn(() => ({ status: 1, stderr: "connection refused" })); + const runOpenshell = vi.fn((args: string[]) => + args.includes("export") + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 1, stderr: "connection refused" }, + ); const exit = vi.fn(() => undefined as never); ensureMessagingBridgeProfiles([BRIDGE_DEF], { ...baseDeps(), runOpenshell, exit }); expect(exit).toHaveBeenCalled(); }); + + it("exits without importing when the profile probe fails for a reason other than missing", () => { + const runOpenshell = vi.fn((_args: string[]) => ({ status: 1, stderr: "gateway unreachable" })); + const exit = vi.fn(() => undefined as never); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { ...baseDeps(), runOpenshell, exit }); + expect(exit).toHaveBeenCalledWith(1); + expect(runOpenshell.mock.calls.some((call) => call[0].includes("import"))).toBe(false); + }); + + it("reports and redacts a timeout without importing the profile", () => { + const runOpenshell = vi.fn((_args: string[]) => ({ + status: null, + stderr: "", + stdout: "", + error: new Error("spawnSync openshell ETIMEDOUT secret-value"), + })); + const exit = vi.fn(() => undefined as never); + const log = vi.fn(); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + redact: (text) => text.replaceAll("secret-value", "[REDACTED]"), + runOpenshell, + exit, + log, + }); + expect(exit).toHaveBeenCalledWith(1); + expect(runOpenshell.mock.calls.some((call) => call[0].includes("import"))).toBe(false); + const logged = log.mock.calls.flat().join("\n"); + expect(logged).toContain("ETIMEDOUT"); + expect(logged).toContain("[REDACTED]"); + expect(logged).not.toContain("secret-value"); + }); + + it("tolerates the existing-profile diagnostic across a wrapped, box-drawn terminal line (#10371)", () => { + // Same failure shape as the web-search race check (#10159/#10371): a + // plain, unnormalized substring test would miss "already exists" split + // across a box-drawing continuation and fall through to a hard failure + // instead of tolerating the race. + let exportCalls = 0; + const runOpenshell = vi.fn((args: string[]) => + !args.includes("export") + ? { status: 1, stderr: "custom provider profile 'google-chat-bridge' already\n │ exists" } + : (exportCalls += 1) === 1 + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 0, stdout: JSON.stringify(GC_PROFILE_DOC) }, + ); + const exit = vi.fn(() => undefined as never); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + }); + expect(exit).not.toHaveBeenCalled(); + }); + + it("passes the bounded OpenShell operation timeout to the probe and the import", () => { + const runOpenshell = vi.fn((args: string[]) => + args.includes("export") + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 0 }, + ); + const exit = vi.fn(() => undefined as never); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { ...baseDeps(), runOpenshell, exit }); + const calls = runOpenshell.mock.calls as unknown as Array<[string[], { timeout?: number }]>; + const timeouts = calls.map(([, options]) => options.timeout); + expect(timeouts).toEqual(calls.map(() => OPENSHELL_OPERATION_TIMEOUT_MS)); + }); + + it("suppresses import output when a concurrent importer creates the profile", () => { + // If a concurrent onboard imports the profile after this probe reports it + // missing, OpenShell's "already exists" import diagnostic must stay + // suppressed — the code re-exports and recovers, so that diagnostic + // would misleadingly read as a failure even though onboarding succeeds. + let exportCalls = 0; + const runOpenshell = vi.fn((args: string[], _options: { suppressOutput?: boolean }) => + !args.includes("export") + ? { status: 1, stderr: "profile already exists" } + : (exportCalls += 1) === 1 + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 0, stdout: JSON.stringify(GC_PROFILE_DOC) }, + ); + const exit = vi.fn(() => undefined as never); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + }); + expect(exit).not.toHaveBeenCalled(); + const importCall = runOpenshell.mock.calls.find((call) => call[0].includes("import")); + expect(importCall?.[1]?.suppressOutput).toBe(true); + }); + + it("rejects an existing refreshing profile whose credential boundary drifted from the checked-in YAML", () => { + const drifted = { ...GC_PROFILE_DOC, endpoints: [{ host: "evil.example", port: 443 }] }; + const runOpenshell = vi.fn((_args: string[]) => ({ + status: 0, + stdout: JSON.stringify(drifted), + })); + const exit = vi.fn(() => undefined as never); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + }); + expect(exit).toHaveBeenCalledWith(1); + expect(runOpenshell.mock.calls.some((call) => call[0].includes("import"))).toBe(false); + }); + + it("rejects a race-winning refreshing profile whose credential boundary drifted from the checked-in YAML", () => { + const drifted = { ...GC_PROFILE_DOC, binaries: ["/usr/bin/curl"] }; + let exportCalls = 0; + const runOpenshell = vi.fn((args: string[]) => + !args.includes("export") + ? { status: 1, stderr: "profile already exists" } + : (exportCalls += 1) === 1 + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 0, stdout: JSON.stringify(drifted) }, + ); + const exit = vi.fn(() => undefined as never); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + }); + expect(exit).toHaveBeenCalledWith(1); + }); + + it("reports the real cause, not a fabricated conflict, when the post-race re-export itself fails", () => { + // A failed post-race export means the profile content was never read — + // it is not proof of a conflict, and must not tell the operator to + // delete a profile that may be fine. + let exportCalls = 0; + const runOpenshell = vi.fn((args: string[]) => + !args.includes("export") + ? { status: 1, stderr: "profile already exists" } + : (exportCalls += 1) === 1 + ? { status: 1, stderr: "custom provider profile not found" } + : { status: 1, stderr: "gateway unreachable" }, + ); + const exit = vi.fn(() => undefined as never); + const log = vi.fn(); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { ...baseDeps(), log, runOpenshell, exit }); + expect(exit).toHaveBeenCalledWith(1); + const logged = log.mock.calls.flat().join("\n"); + expect(logged).toContain("gateway unreachable"); + expect(logged).not.toContain("delete"); + }); + + it("accepts OpenShell's pinned export representation of a refreshing profile (#10371)", () => { + const checkedInYaml = YAML.stringify({ + id: GC_PROFILE.profileId, + credentials: [ + { + name: "access_token", + env_vars: [GC_PROFILE.credentialKey], + required: true, + auth_style: "bearer", + header_name: "Authorization", + query_param: "", + refresh: { + strategy: "google-service-account-jwt", + scopes: ["https://www.googleapis.com/auth/chat.bot"], + material: [ + { name: "client_email", required: true }, + { name: "private_key", required: true, secret: true }, + { name: "scope" }, + ], + }, + }, + ], + endpoints: [ + { + host: "chat.googleapis.com", + port: 443, + protocol: "rest", + access: "read-write", + }, + ], + binaries: ["/usr/bin/node"], + inference_capable: false, + }); + const pinnedExport = { + id: GC_PROFILE.profileId, + credentials: [ + { + name: "access_token", + env_vars: [GC_PROFILE.credentialKey], + required: true, + auth_style: "bearer", + header_name: "Authorization", + query_param: "", + refresh: { + strategy: "google_service_account_jwt", + scopes: ["https://www.googleapis.com/auth/chat.bot"], + material: [ + { name: "client_email", required: true, secret: false }, + { name: "private_key", required: true, secret: true }, + { name: "scope", required: false, secret: false }, + ], + }, + }, + ], + endpoints: [ + { + host: "chat.googleapis.com", + port: 443, + protocol: "rest", + access: "read-write", + }, + ], + binaries: ["/usr/bin/node"], + inference_capable: false, + }; + + const runOpenshell = vi.fn((_args: string[]) => ({ + status: 0, + stdout: JSON.stringify(pinnedExport), + })); + const exit = vi.fn(() => undefined as never); + const log = vi.fn(); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + profiles: [GC_PROFILE], + readFileSync: () => checkedInYaml, + runOpenshell, + exit, + log, + }); + + expect(exit).not.toHaveBeenCalled(); + expect(log).not.toHaveBeenCalled(); + expect(runOpenshell.mock.calls.some((call) => call[0].includes("import"))).toBe(false); + }); + + it("does not report an unreadable checked-in profile as drift (#10371)", () => { + // Reading our own YAML can fail for reasons that say nothing about the + // registered profile. Reporting that as a conflict sends the operator to + // delete a profile whose contents were never compared. + const runOpenshell = vi.fn((_args: string[]) => ({ + status: 0, + stdout: JSON.stringify(GC_PROFILE_DOC), + })); + const exit = vi.fn(() => undefined as never); + const log = vi.fn(); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }); + }, + runOpenshell, + exit, + log, + }); + + expect(exit).toHaveBeenCalledWith(1); + expect(log.mock.calls.flat().join("\n")).not.toContain("delete"); + expect(runOpenshell.mock.calls.some((call) => call[0].includes("import"))).toBe(false); + }); + + it("does not report an export that is not JSON as drift (#10371)", () => { + const runOpenshell = vi.fn((_args: string[]) => ({ + status: 0, + stdout: "profile export interrupted", + })); + const exit = vi.fn(() => undefined as never); + const log = vi.fn(); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + log, + }); + + expect(exit).toHaveBeenCalledWith(1); + expect(log.mock.calls.flat().join("\n")).not.toContain("delete"); + }); + + it("does not report valid JSON with no provider boundary as drift (#10371)", () => { + const runOpenshell = vi.fn((_args: string[]) => ({ status: 0, stdout: "{}" })); + const exit = vi.fn(() => undefined as never); + const log = vi.fn(); + ensureMessagingBridgeProfiles([BRIDGE_DEF], { + ...baseDeps(), + readFileSync: () => YAML.stringify(GC_PROFILE_DOC), + runOpenshell, + exit, + log, + }); + + expect(exit).toHaveBeenCalledWith(1); + expect(log.mock.calls.flat().join("\n")).not.toContain("delete"); + expect(runOpenshell.mock.calls.some((call) => call[0].includes("import"))).toBe(false); + }); }); describe("matchesRegisteredStaticMessagingProfile", () => { @@ -632,20 +998,68 @@ describe("matchesRegisteredStaticMessagingProfile", () => { readFileSync: () => YAML.stringify(DISCORD_PROFILE_DOC), runOpenshell, }), - ).toBe(true); + ).toBe("match"); expect(runOpenshell).toHaveBeenCalledWith( ["provider", "profile", "export", DISCORD_PROFILE.profileId, "--output", "json"], - expect.objectContaining({ suppressOutput: true }), + expect.objectContaining({ + suppressOutput: true, + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }), ); }); - it("rejects a registered static profile with endpoint authority", () => { + it.each([ + ["endpoint authority", { endpoints: [{ host: "gateway.discord.gg", port: 443 }] }], + ["binary authority", { binaries: ["/usr/bin/curl"] }], + ["inference capability", { inference_capable: true }], + ])("rejects a registered static profile with changed %s", (_description, override) => { const runOpenshell = vi.fn(() => ({ status: 0, stdout: JSON.stringify({ ...DISCORD_PROFILE_DOC, - endpoints: [{ host: "gateway.discord.gg", port: 443 }], + ...override, + }), + })); + + expect( + matchesRegisteredStaticMessagingProfile(DISCORD_PROFILE.profileId, { + root: "/repo", + profiles: [DISCORD_PROFILE], + readFileSync: () => YAML.stringify(DISCORD_PROFILE_DOC), + runOpenshell, + }), + ).toBe("mismatch"); + }); + + it.each([ + ["a failed export", { status: 1, stderr: "gateway unavailable", stdout: "" }], + [ + "a non-completing export with a missing diagnostic", + { + status: null, + stderr: `custom provider profile '${DISCORD_PROFILE.profileId}' not found`, + stdout: "", + }, + ], + ["malformed export output", { status: 0, stderr: "", stdout: "not-json" }], + ])("reports %s as indeterminate", (_condition, result) => { + const runOpenshell = vi.fn(() => result); + + expect( + matchesRegisteredStaticMessagingProfile(DISCORD_PROFILE.profileId, { + root: "/repo", + profiles: [DISCORD_PROFILE], + readFileSync: () => YAML.stringify(DISCORD_PROFILE_DOC), + runOpenshell, }), + ).toBe("indeterminate"); + }); + + it("reports a recognized missing static profile as absent", () => { + const runOpenshell = vi.fn(() => ({ + status: 1, + stderr: `custom provider profile '${DISCORD_PROFILE.profileId}' not found`, + stdout: "", })); expect( @@ -655,7 +1069,33 @@ describe("matchesRegisteredStaticMessagingProfile", () => { readFileSync: () => YAML.stringify(DISCORD_PROFILE_DOC), runOpenshell, }), - ).toBe(false); + ).toBe("absent"); + }); + + it.each([ + [ + "invalid checked-in boundary", + () => YAML.stringify({ ...DISCORD_PROFILE_DOC, endpoints: [{ host: "unsafe.example" }] }), + ], + [ + "unreadable checked-in profile", + () => { + throw new Error("EACCES"); + }, + ], + ])("fails closed when discovery finds an %s", (_condition, readFileSync) => { + const runOpenshell = vi.fn(); + + expect( + matchesRegisteredStaticMessagingProfile(DISCORD_PROFILE.profileId, { + root: "/repo", + manifests: [SYNTHETIC_DISCORD_MANIFEST], + existsSync: () => true, + readFileSync, + runOpenshell, + }), + ).toBe("indeterminate"); + expect(runOpenshell).not.toHaveBeenCalled(); }); it("does not apply the static-profile check to other provider types", () => { @@ -672,6 +1112,23 @@ describe("matchesRegisteredStaticMessagingProfile", () => { }); }); +describe("listMessagingBridgeProfiles (synthetic static profile)", () => { + it("discovers an endpointless, binaryless, non-inference static profile", () => { + expect(discoverSyntheticDiscordProfile(DISCORD_PROFILE_DOC)).toEqual([DISCORD_PROFILE]); + }); + + it.each([ + ["missing endpoints", { endpoints: undefined }], + ["endpoint authority", { endpoints: [{ host: "gateway.discord.gg", port: 443 }] }], + ["missing binaries", { binaries: undefined }], + ["binary authority", { binaries: ["/usr/bin/curl"] }], + ["missing inference capability", { inference_capable: undefined }], + ["inference capability", { inference_capable: true }], + ])("rejects %s", (_description, override) => { + expect(discoverSyntheticDiscordProfile({ ...DISCORD_PROFILE_DOC, ...override })).toEqual([]); + }); +}); + describe("listMessagingBridgeProfiles (real registry + co-located YAML)", () => { it("discovers the Google Chat bridge and keeps the credential key in lockstep", () => { const profiles = listMessagingBridgeProfiles(); diff --git a/src/lib/onboard/messaging-bridge-provider.ts b/src/lib/onboard/messaging-bridge-provider.ts index 73b2f7adf54..e5e89fbebc1 100644 --- a/src/lib/onboard/messaging-bridge-provider.ts +++ b/src/lib/onboard/messaging-bridge-provider.ts @@ -17,9 +17,19 @@ import fs from "node:fs"; import path from "node:path"; -import { isDeepStrictEqual } from "node:util"; import YAML from "yaml"; +import { OPENSHELL_OPERATION_TIMEOUT_MS } from "../adapters/openshell/provider-command"; +import type { + CheckedInBoundaryComparison, + RegisteredProfileComparison, +} from "../adapters/openshell/provider-profile"; +import { + compareExportedProfileToCheckedIn, + isMissingProviderProfile, + normalizeOpenshellDiagnostic, + openshellResultDiagnostic, +} from "../adapters/openshell/provider-profile"; import { compactText } from "../core/url-utils"; import { createBuiltInChannelManifestRegistry } from "../messaging/channels"; import type { @@ -40,14 +50,20 @@ const PROVIDER_PROFILE_FILE_BY_AGENT: Readonly> hermes: "hermes.yaml", }; +type RunOpenshellResult = { + status: number | null; + stderr?: string | Buffer | null; + stdout?: string | Buffer | null; + error?: Error; +}; + type RunOpenshell = ( args: string[], - // The runner accepts a wider options shape; we only set ignoreError + stdio - // here, so erase the type at the boundary to keep this module free of the - // runner.ts internals. + // The runner accepts a wider options shape. Keep this module free of the + // runner.ts internals by erasing that shape at the injected boundary. // eslint-disable-next-line @typescript-eslint/no-explicit-any opts: any, -) => { status: number | null; stderr?: string | Buffer | null; stdout?: string | Buffer | null }; +) => RunOpenshellResult; type TokenDefShape = { name: string; providerType?: string; token: string | null }; @@ -119,6 +135,8 @@ export interface MatchRegisteredStaticMessagingProfileDeps { readonly root: string; readonly runOpenshell: RunOpenshell; readonly profiles?: readonly MessagingBridgeProfile[]; + readonly manifests?: readonly ChannelManifest[]; + readonly existsSync?: (file: string) => boolean; readonly readFileSync?: (file: string) => string; } @@ -145,80 +163,107 @@ function bufferOrStringToText(value: string | Buffer | null | undefined): string return ""; } -function credentialBoundary(doc: Record): Record | null { - if ( - typeof doc.id !== "string" || - !Array.isArray(doc.credentials) || - !Array.isArray(doc.endpoints) || - !Array.isArray(doc.binaries) || - typeof doc.inference_capable !== "boolean" - ) { - return null; - } - const credentials = doc.credentials.map((entry) => { - if (!entry || typeof entry !== "object" || Array.isArray(entry)) return null; - const credential = entry as Record; - return { - name: credential.name, - env_vars: credential.env_vars, - required: credential.required, - auth_style: credential.auth_style, - header_name: credential.header_name, - query_param: credential.query_param, - refresh: credential.refresh ?? null, - }; - }); - if (credentials.some((entry) => entry === null)) return null; - return { - id: doc.id, - credentials, - endpoints: doc.endpoints, - binaries: doc.binaries, - inference_capable: doc.inference_capable, - }; -} - -function staticProfileMatchesCheckedInBoundary( +/** + * Whether an exported profile is the checked-in YAML this codebase ships for + * it. The checked-in boundary itself defines what's allowed for both static + * and refreshing profiles; a profile ID match alone is not proof this is that + * checked-in profile (#10371). + * + * Compares through OpenShell's export representation, since a refresh block + * does not round-trip byte-identically, and distinguishes a read that never + * completed from confirmed drift. + */ +function compareProfileToCheckedIn( profile: MessagingBridgeProfile, exported: string, readFileSync: (file: string) => string, -): boolean { - try { - const actual = JSON.parse(exported) as Record; - const expected = YAML.parse(readFileSync(profile.profilePath)) as Record; - const actualBoundary = credentialBoundary(actual); - const expectedBoundary = credentialBoundary(expected); - return ( - actualBoundary !== null && - expectedBoundary !== null && - expectedBoundary.id === profile.profileId && - Array.isArray(expectedBoundary.endpoints) && - expectedBoundary.endpoints.length === 0 && - Array.isArray(expectedBoundary.binaries) && - expectedBoundary.binaries.length === 0 && - expectedBoundary.inference_capable === false && - isDeepStrictEqual(actualBoundary, expectedBoundary) - ); - } catch { - return false; +): CheckedInBoundaryComparison { + return compareExportedProfileToCheckedIn( + exported, + () => readFileSync(profile.profilePath), + profile.profileId, + ); +} + +type MessagingBridgeProfileCandidate = Pick< + MessagingBridgeProfile, + "agent" | "channelId" | "profilePath" | "sourceSecretEnv" +>; + +function messagingBridgeProfileCandidates(input: { + root: string; + manifests: readonly ChannelManifest[]; + existsSync: (file: string) => boolean; +}): MessagingBridgeProfileCandidate[] { + const candidates: MessagingBridgeProfileCandidate[] = []; + for (const manifest of input.manifests) { + const sourceSecretEnv = primarySecretEnv(manifest); + if (!sourceSecretEnv) continue; + for (const agent of manifest.supportedAgents) { + const profilePath = channelProviderProfilePath(input.root, manifest.id, agent); + if (!profilePath || !input.existsSync(profilePath)) continue; + candidates.push({ agent, channelId: manifest.id, profilePath, sourceSecretEnv }); + } } + return candidates; +} + +function discoverStaticProfileForInspection( + providerType: string, + deps: MatchRegisteredStaticMessagingProfileDeps, +): MessagingBridgeProfile | "indeterminate" | null { + const readFileSync = deps.readFileSync ?? ((file: string) => fs.readFileSync(file, "utf-8")); + const candidates = messagingBridgeProfileCandidates({ + root: deps.root, + manifests: deps.manifests ?? createBuiltInChannelManifestRegistry().list(), + existsSync: deps.existsSync ?? ((file) => fs.existsSync(file)), + }); + for (const candidate of candidates) { + let content: string; + let declaredId: unknown; + try { + content = readFileSync(candidate.profilePath); + declaredId = (YAML.parse(content) as Record | null)?.id; + } catch { + return "indeterminate"; + } + if (declaredId !== providerType) continue; + const parsed = parseProfileYaml(content); + if (!parsed) return "indeterminate"; + if (parsed.strategy !== null) return null; + return { ...candidate, ...parsed }; + } + return null; } /** Compare a registered static profile with its checked-in credential boundary. */ export function matchesRegisteredStaticMessagingProfile( providerType: string, deps: MatchRegisteredStaticMessagingProfileDeps, -): boolean | null { - const profile = (deps.profiles ?? listMessagingBridgeProfiles({ root: deps.root })).find( - (candidate) => candidate.profileId === providerType && candidate.strategy === null, - ); +): RegisteredProfileComparison | null { + const profile = deps.profiles + ? (deps.profiles.find( + (candidate) => candidate.profileId === providerType && candidate.strategy === null, + ) ?? null) + : discoverStaticProfileForInspection(providerType, deps); + if (profile === "indeterminate") return "indeterminate"; if (!profile) return null; const exported = deps.runOpenshell( ["provider", "profile", "export", profile.profileId, "--output", "json"], - { ignoreError: true, suppressOutput: true, stdio: ["ignore", "pipe", "pipe"] }, + { + ignoreError: true, + suppressOutput: true, + stdio: ["ignore", "pipe", "pipe"], + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }, ); - if (exported.status !== 0) return false; - return staticProfileMatchesCheckedInBoundary( + if (exported.status !== 0) { + return Number.isInteger(exported.status) && + isMissingProviderProfile(openshellResultDiagnostic(exported), profile.profileId) + ? "absent" + : "indeterminate"; + } + return compareProfileToCheckedIn( profile, bufferOrStringToText(exported.stdout), deps.readFileSync ?? ((file: string) => fs.readFileSync(file, "utf-8")), @@ -272,8 +317,14 @@ function parseProfileYaml( const refresh = credential.refresh as Record | undefined; const strategy = typeof refresh?.strategy === "string" && refresh.strategy ? refresh.strategy : null; - if (strategy === null && (!Array.isArray(doc?.endpoints) || doc.endpoints.length !== 0)) { - return null; + if (strategy === null) { + const isStaticCredentialBoundary = + Array.isArray(doc?.endpoints) && + doc.endpoints.length === 0 && + Array.isArray(doc?.binaries) && + doc.binaries.length === 0 && + doc.inference_capable === false; + if (!isStaticCredentialBoundary) return null; } const scopes = Array.isArray(refresh?.scopes) ? refresh.scopes.filter((s): s is string => typeof s === "string") @@ -304,16 +355,11 @@ export function listMessagingBridgeProfiles( const manifests = deps.manifests ?? createBuiltInChannelManifestRegistry().list(); const profiles: MessagingBridgeProfile[] = []; - for (const manifest of manifests) { - const sourceSecretEnv = primarySecretEnv(manifest); - if (!sourceSecretEnv) continue; - for (const agent of manifest.supportedAgents) { - const profilePath = channelProviderProfilePath(root, manifest.id, agent); - if (!profilePath || !existsSync(profilePath)) continue; - const parsed = parseProfileYaml(readFileSync(profilePath)); - if (!parsed) continue; - profiles.push({ channelId: manifest.id, agent, profilePath, sourceSecretEnv, ...parsed }); - } + const candidates = messagingBridgeProfileCandidates({ root, manifests, existsSync }); + for (const candidate of candidates) { + const parsed = parseProfileYaml(readFileSync(candidate.profilePath)); + if (!parsed) continue; + profiles.push({ ...candidate, ...parsed }); } return profiles; } @@ -464,84 +510,156 @@ export function ensureMessagingBridgeProfiles( const exit = deps.exit ?? ((code?: number) => process.exit(code)); const readFileSync = deps.readFileSync ?? ((file: string) => fs.readFileSync(file, "utf-8")); - const rejectMismatchedStaticProfile = (profile: MessagingBridgeProfile): void => { + const rejectMismatchedProfile = (profile: MessagingBridgeProfile): void => { + errorLog( + `\n ✗ OpenShell provider profile '${profile.profileId}' already registered in the selected ` + + `OpenShell gateway does not match NemoClaw's checked-in ${profile.channelId} credential contract.`, + ); errorLog( - `\n ✗ OpenShell provider profile '${profile.profileId}' does not match NemoClaw's endpointless ${profile.channelId} credential contract.`, + " Find the selected gateway's name with 'openshell gateway info', then remove the " + + `conflicting profile from that gateway (openshell provider profile -g ` + + `delete ${profile.profileId}) and re-run onboarding. Other sandboxes that use the same ` + + "gateway may share this profile — confirm the effect before removing it.", + ); + exit(1); + }; + + const rejectUnverifiableProfile = (profile: MessagingBridgeProfile): void => { + errorLog( + `\n ✗ Could not verify the OpenShell provider profile '${profile.profileId}' already ` + + `registered in the selected OpenShell gateway against NemoClaw's checked-in ` + + `${profile.channelId} credential contract.`, + ); + errorLog( + ` The gateway's export was not readable as JSON, or ${profile.profilePath} could not be ` + + "read as a provider profile. An unfinished check is not proof the registered profile " + + "drifted, so it was left in place. Resolve the read failure and re-run onboarding.", + ); + exit(1); + }; + + const rejectProbeFailure = ( + profile: MessagingBridgeProfile, + operation: string, + rawDiagnostic: string, + ): void => { + const diagnostic = compactText(deps.redact(rawDiagnostic)); + errorLog( + `\n ✗ Could not check whether the ${profile.channelId} provider profile is already ` + + `registered (${operation} failed).`, + ); + if (diagnostic) errorLog(` ${diagnostic.slice(0, 500)}`); + errorLog( + " Confirm the OpenShell gateway is reachable and this account is authorized, then re-run onboarding.", ); - errorLog(" Remove the conflicting profile and re-run onboarding."); exit(1); }; for (const profile of active) { // Onboard registers each bridge provider twice: once up front so an // interrupted run can resume, then again during create-plan materialization. - // Probe first and skip the re-import so the second pass never hits OpenShell's - // "already exists" error. A fresh gateway answers the probe with a harmless - // "not found" that suppressOutput hides — only the exit status says whether - // the profile already exists. + // Probe before import so the second registration reuses the host-global + // profile. Only a recognized missing-profile result permits an import. const alreadyRegistered = deps.runOpenshell( ["provider", "profile", "export", profile.profileId, "--output", "json"], - { ignoreError: true, suppressOutput: true, stdio: ["ignore", "pipe", "pipe"] }, + { + ignoreError: true, + suppressOutput: true, + stdio: ["ignore", "pipe", "pipe"], + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }, ); if (alreadyRegistered.status === 0) { - if ( - profile.strategy === null && - !staticProfileMatchesCheckedInBoundary( - profile, - bufferOrStringToText(alreadyRegistered.stdout), - readFileSync, - ) - ) { - rejectMismatchedStaticProfile(profile); + const comparison = compareProfileToCheckedIn( + profile, + bufferOrStringToText(alreadyRegistered.stdout), + readFileSync, + ); + if (comparison === "indeterminate") { + rejectUnverifiableProfile(profile); + return; + } + if (comparison === "mismatch") { + rejectMismatchedProfile(profile); return; } continue; } - // Probe failed for something other than "not found" (gateway down, auth, …): - // surface it instead of masking a real problem. - const probeDiagnostic = `${bufferOrStringToText(alreadyRegistered.stderr)} ${bufferOrStringToText( - alreadyRegistered.stdout, - )}`; - if (probeDiagnostic.trim() && !/not found/i.test(probeDiagnostic)) { - errorLog(`\n ⚠ Unexpected error probing the ${profile.channelId} provider profile:`); - const probeText = compactText(deps.redact(probeDiagnostic)); - if (probeText) errorLog(` ${probeText.slice(0, 500)}`); + // A nonzero probe status alone is not proof the profile is missing — the + // gateway could be unreachable, this account unauthorized, or the probe + // could have timed out or spawned incorrectly. Only a recognized "not + // found" diagnostic makes it safe to proceed to import; anything else + // must stop here rather than attempt a state-changing import in + // response to a read that never actually completed. + const probeDiagnostic = openshellResultDiagnostic(alreadyRegistered); + if ( + !Number.isInteger(alreadyRegistered.status) || + !isMissingProviderProfile(probeDiagnostic, profile.profileId) + ) { + rejectProbeFailure(profile, "provider profile export", probeDiagnostic); + return; } const result = deps.runOpenshell( ["provider", "profile", "import", "--file", profile.profilePath], - { ignoreError: true, stdio: ["ignore", "pipe", "pipe"] }, + { + ignoreError: true, + suppressOutput: true, + stdio: ["ignore", "pipe", "pipe"], + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }, ); if (result.status === 0) continue; // Reconcile a lost race: the probe saw no profile but a concurrent import made it. - const rawDiagnostic = `${bufferOrStringToText(result.stderr)} ${bufferOrStringToText(result.stdout)}`; - if (/already exists/i.test(rawDiagnostic)) { - if (profile.strategy !== null) continue; + // Normalize the diagnostic because OpenShell can wrap `already exists` + // across a box-drawing continuation (#10159, #10371). + const rawDiagnostic = openshellResultDiagnostic(result); + if (/already exists/iu.test(normalizeOpenshellDiagnostic(rawDiagnostic))) { const racedProfile = deps.runOpenshell( ["provider", "profile", "export", profile.profileId, "--output", "json"], - { ignoreError: true, suppressOutput: true, stdio: ["ignore", "pipe", "pipe"] }, + { + ignoreError: true, + suppressOutput: true, + stdio: ["ignore", "pipe", "pipe"], + timeout: OPENSHELL_OPERATION_TIMEOUT_MS, + }, ); - if ( - racedProfile.status !== 0 || - !staticProfileMatchesCheckedInBoundary( + // A nonzero post-race export status means the export itself failed + // (gateway unreachable, unauthorized, timed out) — the profile content + // was never read, so this is not proof of a conflict. Report the real + // cause instead of telling the operator to delete a profile that may + // be fine. + if (racedProfile.status !== 0) { + rejectProbeFailure( profile, - bufferOrStringToText(racedProfile.stdout), - readFileSync, - ) - ) { - rejectMismatchedStaticProfile(profile); + "post-race provider profile export", + openshellResultDiagnostic(racedProfile), + ); + return; + } + const racedComparison = compareProfileToCheckedIn( + profile, + bufferOrStringToText(racedProfile.stdout), + readFileSync, + ); + if (racedComparison === "indeterminate") { + rejectUnverifiableProfile(profile); + return; + } + if (racedComparison === "mismatch") { + rejectMismatchedProfile(profile); return; } continue; } const diagnostic = compactText(deps.redact(rawDiagnostic)); + errorLog(`\n ✗ Failed to register the ${profile.channelId} provider profile with OpenShell.`); + if (diagnostic) errorLog(` ${diagnostic.slice(0, 500)}`); errorLog( - `\n ✗ Failed to register the ${profile.channelId} provider profile with OpenShell.`, + " Fix the error above. If OpenShell requires an update, rerun the NemoClaw installer. Then rerun onboarding.", ); - if (diagnostic) errorLog(` ${diagnostic.slice(0, 500)}`); - errorLog(" Update OpenShell with scripts/install-openshell.sh and re-run onboarding."); exit(result.status || 1); return; } diff --git a/src/lib/onboard/providers.test.ts b/src/lib/onboard/providers.test.ts index afa68083488..385e34d3b46 100644 --- a/src/lib/onboard/providers.test.ts +++ b/src/lib/onboard/providers.test.ts @@ -677,9 +677,17 @@ describe("onboard provider helpers", () => { }, ], (command) => { - commands.push(command.join(" ")); - if (command.includes("get")) return { status: 1, stdout: "", stderr: "" }; - return { status: 0, stdout: "", stderr: "" }; + const commandText = command.join(" "); + commands.push(commandText); + return ( + new Map([ + [ + "provider profile export brave --output json", + { status: 1, stdout: "", stderr: "custom provider profile not found" }, + ], + ["provider get alpha-brave-search", { status: 1, stdout: "", stderr: "" }], + ]).get(commandText) ?? { status: 0, stdout: "", stderr: "" } + ); }, ); @@ -914,8 +922,16 @@ describe("onboard provider helpers", () => { }, ], (command) => { - commands.push(command.join(" ")); - return { status: 0, stdout: "", stderr: "" }; + const commandText = command.join(" "); + commands.push(commandText); + return ( + new Map([ + [ + "provider profile export brave --output json", + { status: 1, stdout: "", stderr: "custom provider profile not found" }, + ], + ]).get(commandText) ?? { status: 0, stdout: "", stderr: "" } + ); }, ); @@ -923,6 +939,7 @@ describe("onboard provider helpers", () => { // still attached to a live sandbox, so reuse paths must use `update`. expect(providers).toEqual(["alpha-brave-search"]); expect(commands).toEqual([ + "provider profile export brave --output json", expect.stringContaining("nemoclaw-blueprint/provider-profiles/brave.yaml"), "provider get alpha-brave-search", "provider update alpha-brave-search --credential BRAVE_API_KEY", @@ -1234,14 +1251,23 @@ describe("onboard provider helpers", () => { }, ], (command) => { - commands.push(command.join(" ")); - return { status: 0, stdout: "", stderr: "" }; + const commandText = command.join(" "); + commands.push(commandText); + return ( + new Map([ + [ + "provider profile export brave --output json", + { status: 1, stdout: "", stderr: "custom provider profile not found" }, + ], + ]).get(commandText) ?? { status: 0, stdout: "", stderr: "" } + ); }, { replaceExisting: true }, ); expect(providers).toEqual(["alpha-brave-search"]); expect(commands).toEqual([ + "provider profile export brave --output json", expect.stringContaining("nemoclaw-blueprint/provider-profiles/brave.yaml"), "provider get alpha-brave-search", "provider delete alpha-brave-search", diff --git a/test/channels/channels-add-bridge-lifecycle.test.ts b/test/channels/channels-add-bridge-lifecycle.test.ts index afc62686aaa..1d7a35b8454 100644 --- a/test/channels/channels-add-bridge-lifecycle.test.ts +++ b/test/channels/channels-add-bridge-lifecycle.test.ts @@ -188,12 +188,19 @@ beforeEach(() => { runOpenshellSpy = vi.spyOn(runtime, "runOpenshell").mockImplementation((args) => { const command = withoutGateway(args); const providerMissing = command[0] === "provider" && command[1] === "get"; + const profileMissing = + command[0] === "provider" && command[1] === "profile" && command.includes("export"); + const missing = providerMissing || profileMissing; return { pid: 0, output: [null, "", ""], stdout: isRefreshStatus(args) ? refreshStatusTable(command) : "", - stderr: providerMissing ? `provider '${args[args.length - 1]}' not found` : "", - status: providerMissing ? 1 : 0, + stderr: profileMissing + ? "custom provider profile not found" + : providerMissing + ? `provider '${args[args.length - 1]}' not found` + : "", + status: missing ? 1 : 0, signal: null, }; });