fix: silent credential-save failures and session/tool hygiene gaps (#109622)

* fix(agents): surface auth persistence failures and harden session, model, and shell tool hygiene

* refactor(agents): keep shell invocation types module-local

* test(agents): derive shell config type locally
This commit is contained in:
Peter Steinberger
2026-07-17 00:02:45 -07:00
committed by GitHub
parent 0c989568b3
commit b84199644d
23 changed files with 585 additions and 87 deletions
@@ -62,6 +62,44 @@ describe("InMemorySessionStorage", () => {
});
});
it("returns a selected branch in root-to-leaf order", async () => {
const activeLeaf: SessionTreeEntry = {
type: "custom",
id: "active-leaf",
parentId: "child",
timestamp: "2026-01-01T00:00:02.000Z",
customType: "active",
};
const sideLeaf: SessionTreeEntry = {
type: "custom",
id: "side-leaf",
parentId: "root",
timestamp: "2026-01-01T00:00:03.000Z",
customType: "side",
};
const storage = new InMemorySessionStorage({
entries: [rootEntry, childEntry, activeLeaf, sideLeaf],
});
expect((await storage.getPathToRoot(activeLeaf.id)).map((entry) => entry.id)).toEqual([
"root",
"child",
"active-leaf",
]);
expect((await storage.getPathToRoot(sideLeaf.id)).map((entry) => entry.id)).toEqual([
"root",
"side-leaf",
]);
});
it("normalizes session names to one line", async () => {
const session = new Session(new InMemorySessionStorage());
await session.appendSessionName(" first\nsecond\r\nthird ");
expect(await session.getSessionName()).toBe("first second third");
});
it("traverses descendants of leaf markers through the selected target", async () => {
const leafEntry: SessionTreeEntry = {
type: "leaf",
@@ -259,7 +259,7 @@ export class Session<TMetadata extends SessionMetadata = SessionMetadata> {
id: await this.storage.createEntryId(),
parentId: await this.getAppendParentId(),
timestamp: new Date().toISOString(),
name: name.trim(),
name: name.replace(/[\r\n]+/g, " ").trim(),
} satisfies SessionInfoEntry);
}
@@ -252,7 +252,7 @@ export abstract class BaseSessionStorage<
}
seen.add(current.id);
if (current.type !== "leaf") {
path.unshift(current);
path.push(current);
}
// Leaf rows are control records. Descendants written by older appenders
// may point at the marker, but their visible ancestry starts at its target.
@@ -271,6 +271,7 @@ export abstract class BaseSessionStorage<
}
current = parent;
}
path.reverse();
return path;
}
@@ -25,6 +25,28 @@ afterEach(async () => {
});
describe("readTranscriptFileState", () => {
it("normalizes appended session names to one line", async () => {
const root = await makeRoot("openclaw-transcript-state-name-");
const sessionFile = path.join(root, "session.jsonl");
await fs.writeFile(
sessionFile,
`${JSON.stringify({
type: "session",
version: 3,
id: "session-1",
timestamp: "2026-07-16T00:00:00.000Z",
cwd: root,
})}\n`,
"utf-8",
);
const state = await readTranscriptFileState(sessionFile);
const entry = state.appendSessionInfo(" first\nsecond\r\nthird ");
expect(entry.name).toBe("first second third");
expect(state.getEntries().at(-1)).toMatchObject({ name: "first second third" });
});
it("skips malformed session entries without moving the active leaf", async () => {
// Bad rows are ignored for branch construction, but valid legacy orphan
// roots remain reachable so partial imports can still be replayed.
@@ -366,9 +366,10 @@ function readableSessionState(fileEntries: FileEntry[]): ReadableSessionState {
if (!entry) {
break;
}
pathLocal.unshift(entry);
pathLocal.push(entry);
id = entry.parentId;
}
pathLocal.reverse();
return pathLocal;
};
const firstReadableDescendantOnBranch = (
@@ -836,7 +837,7 @@ export class TranscriptFileState {
id: generateEntryId(this.byId),
parentId: this.appendParentId,
timestamp: new Date().toISOString(),
name: name.trim(),
name: name.replace(/[\r\n]+/g, " ").trim(),
});
}
+41
View File
@@ -4,6 +4,7 @@ import { tmpdir } from "node:os";
import { join } from "node:path";
import lockfile from "proper-lockfile";
import { afterEach, describe, expect, it, vi } from "vitest";
import type { AuthStorageBackend } from "./auth-storage.js";
// auth-storage.ts persists via the named import `writeFileSync` from node:fs,
// and replaceFileAtomicSync (in @openclaw/fs-safe) writes its temp file via the
@@ -127,4 +128,44 @@ describe("file auth storage", () => {
await expect(storage.withLockAsync(async () => ({ result: undefined }))).rejects.toBe(error);
});
it("throws without changing memory when malformed storage cannot be reloaded", () => {
tmpDir = fs.mkdtempSync(join(tmpdir(), "auth-malformed-"));
const authPath = join(tmpDir, "auth.json");
fs.writeFileSync(authPath, "{invalid-json", "utf-8");
const storage = AuthStorage.create(authPath);
expect(() => storage.set("openai", { type: "api_key", key: "test-token-placeholder" })).toThrow(
"Cannot update auth storage because it could not be loaded",
);
expect(storage.has("openai")).toBe(false);
expect(fs.readFileSync(authPath, "utf-8")).toBe("{invalid-json");
});
it("throws without changing memory when the durable write fails", () => {
const writeError = new Error("simulated durable write failure");
let persisted = "{}";
const backend: AuthStorageBackend = {
withLock: (fn) => {
const update = fn(persisted);
if (update.next !== undefined) {
throw writeError;
}
return update.result;
},
withLockAsync: async (fn) => {
const update = await fn(persisted);
if (update.next !== undefined) {
persisted = update.next;
}
return update.result;
},
};
const storage = AuthStorage.fromStorage(backend);
expect(() => storage.set("openai", { type: "api_key", key: "test-token-placeholder" })).toThrow(
/simulated durable write failure/,
);
expect(storage.has("openai")).toBe(false);
});
});
+30 -6
View File
@@ -34,6 +34,13 @@ export type AuthCredential = ApiKeyCredential | OAuthCredential;
export type AuthStorageData = Record<string, AuthCredential>;
class AuthStoragePersistenceError extends Error {
constructor(message: string, cause: unknown) {
super(message, { cause });
this.name = "AuthStoragePersistenceError";
}
}
export type AuthStatus = {
configured: boolean;
source?:
@@ -248,11 +255,20 @@ export class AuthStorage {
private persistProviderChange(provider: string, credential: AuthCredential | undefined): void {
if (this.loadError) {
return;
this.reload();
}
if (this.loadError) {
const error = new AuthStoragePersistenceError(
`Cannot update auth storage because it could not be loaded: ${this.loadError.message}`,
this.loadError,
);
this.recordError(error);
throw error;
}
try {
this.storage.withLock((current) => {
const persistedData = this.storage.withLock((current) => {
const currentData = this.parseStorageData(current);
const merged: AuthStorageData = { ...currentData };
if (credential) {
@@ -260,10 +276,20 @@ export class AuthStorage {
} else {
delete merged[provider];
}
return { result: undefined, next: JSON.stringify(merged, null, 2) };
return { result: merged, next: JSON.stringify(merged, null, 2) };
});
this.loadError = null;
this.data = persistedData;
} catch (error) {
this.recordError(error);
const persistenceError =
error instanceof AuthStoragePersistenceError
? error
: new AuthStoragePersistenceError(
`Failed to persist auth storage update for provider "${provider}": ${error instanceof Error ? error.message : String(error)}`,
error,
);
this.recordError(persistenceError);
throw persistenceError;
}
}
@@ -278,7 +304,6 @@ export class AuthStorage {
* Set credential for a provider.
*/
set(provider: string, credential: AuthCredential): void {
this.data[provider] = credential;
this.persistProviderChange(provider, credential);
}
@@ -286,7 +311,6 @@ export class AuthStorage {
* Remove credential for a provider.
*/
remove(provider: string): void {
delete this.data[provider];
this.persistProviderChange(provider, undefined);
}
+36 -4
View File
@@ -150,7 +150,7 @@ describe("ModelRegistry models.json auth", () => {
});
});
it("still rejects api-key custom models without apiKey", () => {
it("uses stored auth for custom models without an inline apiKey", async () => {
const modelsPath = writeModelsJson({
providers: {
custom: {
@@ -161,10 +161,42 @@ describe("ModelRegistry models.json auth", () => {
},
});
const registry = ModelRegistry.create(AuthStorage.inMemory(), modelsPath);
const registry = ModelRegistry.create(
AuthStorage.inMemory({
custom: { type: "api_key", key: "test-token-placeholder" },
}),
modelsPath,
);
const model = registry.find("custom", "example-model");
expect(registry.getError()).toContain('Provider custom: "apiKey" is required');
expect(registry.find("custom", "example-model")).toBeUndefined();
expect(registry.getError()).toBeUndefined();
expect(registry.getAvailable()).toEqual([model]);
await expect(registry.getApiKeyForProvider("custom")).resolves.toBe("test-token-placeholder");
});
it("uses stored auth for dynamically registered provider models", () => {
const authStorage = AuthStorage.inMemory({
custom: { type: "api_key", key: "test-token-placeholder" },
});
const registry = ModelRegistry.inMemory(authStorage);
registry.registerProvider("custom", {
baseUrl: "https://models.example/v1",
api: "openai-responses",
models: [
{
id: "example-model",
name: "Example Model",
reasoning: false,
input: ["text"],
cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
contextWindow: 128_000,
maxTokens: 16_384,
},
],
});
expect(registry.getAvailable().map((model) => model.id)).toEqual(["example-model"]);
});
it("loads provider models from generated plugin catalog shards", () => {
-16
View File
@@ -223,10 +223,6 @@ function formatValidationPath(error: TLocalizedValidationError): string {
return path || "root";
}
function allowsMissingProviderApiKey(auth: ProviderAuthMode | undefined): boolean {
return auth === "aws-sdk" || auth === "oauth";
}
/** Strip `//` line comments and trailing commas from JSON, leaving string literals untouched. */
function stripJsonComments(input: string): string {
return input
@@ -503,12 +499,6 @@ export class ModelRegistry {
`Provider ${providerName}: "baseUrl" is required when defining custom models.`,
);
}
if (!providerConfig.apiKey && !allowsMissingProviderApiKey(providerConfig.auth)) {
throw new Error(
`Provider ${providerName}: "apiKey" is required when defining custom models.`,
);
}
for (const modelDef of models) {
const hasModelApi = Boolean(modelDef.api);
@@ -841,12 +831,6 @@ export class ModelRegistry {
if (!config.baseUrl) {
throw new Error(`Provider ${providerName}: "baseUrl" is required when defining models.`);
}
if (!config.apiKey && !config.oauth && !allowsMissingProviderApiKey(config.auth)) {
throw new Error(
`Provider ${providerName}: "apiKey" or "oauth" is required when defining models.`,
);
}
for (const modelDef of config.models) {
const api = modelDef.api || config.api;
if (!api) {
+82 -4
View File
@@ -4,7 +4,12 @@ import { describe, expect, it } from "vitest";
import type { Model } from "../../llm/types.js";
import { DEFAULT_MODEL, DEFAULT_PROVIDER } from "../defaults.js";
import type { ModelRegistry } from "./model-registry.js";
import { findInitialModel, parseModelPattern, restoreModelFromSession } from "./model-resolver.js";
import {
findInitialModel,
parseModelPattern,
resolveCliModel,
restoreModelFromSession,
} from "./model-resolver.js";
function model(provider: string, id: string): Model {
return {
@@ -21,12 +26,13 @@ function model(provider: string, id: string): Model {
};
}
function registry(models: Model[]): ModelRegistry {
function registry(models: Model[], authenticatedModels: Model[] = models): ModelRegistry {
return {
find: (provider: string, modelId: string) =>
models.find((entry) => entry.provider === provider && entry.id === modelId),
getAvailable: () => models,
hasConfiguredAuth: (entry: Model) => models.includes(entry),
getAll: () => models,
getAvailable: () => authenticatedModels,
hasConfiguredAuth: (entry: Model) => authenticatedModels.includes(entry),
} as ModelRegistry;
}
@@ -56,6 +62,78 @@ describe("model resolver fallback selection", () => {
expect(result.model).toBe(firstAvailable);
});
it("ignores an unauthenticated saved default", async () => {
const savedDefault = model("saved-provider", "saved-model");
const available = model("available-provider", "available-model");
const result = await findInitialModel({
scopedModels: [],
isContinuing: false,
defaultProvider: savedDefault.provider,
defaultModelId: savedDefault.id,
modelRegistry: registry([savedDefault, available], [available]),
});
expect(result.model).toBe(available);
});
});
describe("custom model fallback", () => {
it.each([
{ suffix: "high", reasoning: true },
{ suffix: "off", reasoning: false },
] as const)("parses :$suffix and configures reasoning", ({ suffix, reasoning }) => {
const providerModel = model("custom-provider", "known-model");
const result = resolveCliModel({
cliModel: `custom-provider/new-model:${suffix}`,
modelRegistry: registry([providerModel]),
});
expect(result.error).toBeUndefined();
expect(result.model).toMatchObject({
provider: "custom-provider",
id: "new-model",
reasoning,
});
expect(result.thinkingLevel).toBe(suffix);
});
it("keeps an invalid suffix as part of the custom model id", () => {
const result = resolveCliModel({
cliProvider: "custom-provider",
cliModel: "new-model:specialized",
modelRegistry: registry([model("custom-provider", "known-model")]),
});
expect(result.model?.id).toBe("new-model:specialized");
expect(result.thinkingLevel).toBeUndefined();
});
it("preserves an explicit thinking level for a custom model", () => {
const result = resolveCliModel({
cliProvider: "custom-provider",
cliModel: "new-model",
cliThinking: "low",
modelRegistry: registry([model("custom-provider", "known-model")]),
});
expect(result.model).toMatchObject({ id: "new-model", reasoning: true });
expect(result.thinkingLevel).toBe("low");
});
it("uses the parsed thinking level during initial model selection", async () => {
const result = await findInitialModel({
cliProvider: "custom-provider",
cliModel: "new-model:high",
scopedModels: [],
isContinuing: false,
modelRegistry: registry([model("custom-provider", "known-model")]),
});
expect(result.model).toMatchObject({ id: "new-model", reasoning: true });
expect(result.thinkingLevel).toBe("high");
});
});
describe("parseModelPattern version sorting", () => {
+28 -9
View File
@@ -330,9 +330,10 @@ export interface ResolveCliModelResult {
export function resolveCliModel(options: {
cliProvider?: string;
cliModel?: string;
cliThinking?: ThinkingLevel;
modelRegistry: ModelRegistry;
}): ResolveCliModelResult {
const { cliProvider, cliModel, modelRegistry } = options;
const { cliProvider, cliModel, cliThinking, modelRegistry } = options;
if (!cliModel) {
return { model: undefined, warning: undefined, error: undefined };
@@ -443,14 +444,32 @@ export function resolveCliModel(options: {
}
if (provider) {
const fallbackModel = buildFallbackModel(provider, pattern, availableModels);
let fallbackPattern = pattern;
let fallbackThinking: ThinkingLevel | undefined;
if (!cliThinking) {
const lastColon = pattern.lastIndexOf(":");
if (lastColon !== -1) {
const suffix = pattern.slice(lastColon + 1);
if (isValidThinkingLevel(suffix)) {
fallbackPattern = pattern.slice(0, lastColon);
fallbackThinking = suffix;
}
}
}
const fallbackModel = buildFallbackModel(provider, fallbackPattern, availableModels);
if (fallbackModel) {
const requestedThinking = cliThinking ?? fallbackThinking;
const resolvedModel =
requestedThinking && requestedThinking !== "off"
? { ...fallbackModel, reasoning: true }
: fallbackModel;
const fallbackWarning = warning
? `${warning} Model "${pattern}" not found for provider "${provider}". Using custom model id.`
: `Model "${pattern}" not found for provider "${provider}". Using custom model id.`;
? `${warning} Model "${fallbackPattern}" not found for provider "${provider}". Using custom model id.`
: `Model "${fallbackPattern}" not found for provider "${provider}". Using custom model id.`;
return {
model: fallbackModel,
thinkingLevel: undefined,
model: resolvedModel,
thinkingLevel: requestedThinking,
warning: fallbackWarning,
error: undefined,
};
@@ -518,7 +537,7 @@ export async function findInitialModel(options: {
if (resolved.model) {
return {
model: resolved.model,
thinkingLevel: DEFAULT_THINKING_LEVEL,
thinkingLevel: resolved.thinkingLevel ?? DEFAULT_THINKING_LEVEL,
fallbackMessage: undefined,
};
}
@@ -537,10 +556,10 @@ export async function findInitialModel(options: {
};
}
// 3. Try saved default from settings
// 3. Try saved default from settings when its auth is configured
if (defaultProvider && defaultModelId) {
const found = modelRegistry.find(defaultProvider, defaultModelId);
if (found) {
if (found && modelRegistry.hasConfiguredAuth(found)) {
model = found;
if (defaultThinkingLevel) {
thinkingLevel = defaultThinkingLevel;
+45 -2
View File
@@ -1,14 +1,31 @@
// Package manager tests cover resource discovery boundaries for package,
// project, and npm-declared agent resources.
import { mkdtemp, mkdir, rm, symlink, writeFile } from "node:fs/promises";
import { mkdtemp, mkdir, realpath, rm, stat, symlink, writeFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { join, relative } from "node:path";
import { afterEach, describe, expect, it } from "vitest";
import { DefaultPackageManager } from "./package-manager.js";
import { SettingsManager } from "./settings-manager.js";
const tempDirs: string[] = [];
type PackageManagerInternals = {
parseSource(
source: string,
):
| { type: "npm"; spec: string; name: string; pinned: boolean }
| { type: "git"; host: string; path: string }
| { type: "local"; path: string };
getNpmInstallPath(
source: { type: "npm"; spec: string; name: string; pinned: boolean },
scope: "user" | "project" | "temporary",
): string;
getGitInstallPath(
source: { type: "git"; host: string; path: string },
scope: "user" | "project" | "temporary",
): string;
};
async function makeTempDir(prefix: string): Promise<string> {
const dir = await mkdtemp(join(tmpdir(), prefix));
tempDirs.push(dir);
@@ -249,4 +266,30 @@ describe("DefaultPackageManager", () => {
expect(resolved.prompts).toEqual([]);
expect(resolved.themes).toEqual([]);
});
it("keeps temporary package paths in a private per-agent directory", async () => {
const createdRoot = await makeTempDir("openclaw-package-manager-temp-");
const root = await realpath(createdRoot);
const agentDir = join(root, "agent");
const manager = new DefaultPackageManager({
cwd: root,
agentDir,
settingsManager: SettingsManager.inMemory({}),
}) as unknown as PackageManagerInternals;
const npmSource = manager.parseSource("npm:@openclaw/example");
const gitSource = manager.parseSource("https://github.com/openclaw/example.git");
if (npmSource.type !== "npm" || gitSource.type !== "git") {
throw new Error("Expected package sources");
}
const npmPath = manager.getNpmInstallPath(npmSource, "temporary");
const gitPath = manager.getGitInstallPath(gitSource, "temporary");
const tempRoot = join(agentDir, "tmp", "resources");
expect(relative(tempRoot, npmPath).startsWith("..")).toBe(false);
expect(relative(tempRoot, gitPath).startsWith("..")).toBe(false);
if (process.platform !== "win32") {
expect((await stat(tempRoot)).mode & 0o777).toBe(0o700);
}
});
});
+20 -3
View File
@@ -4,8 +4,17 @@
* Resolves extension, skill, prompt, and theme sources from npm, git, local paths, and project manifests.
*/
import { createHash } from "node:crypto";
import { existsSync, globSync, readdirSync, readFileSync, realpathSync, statSync } from "node:fs";
import { homedir, tmpdir } from "node:os";
import {
chmodSync,
existsSync,
globSync,
mkdirSync,
readdirSync,
readFileSync,
realpathSync,
statSync,
} from "node:fs";
import { homedir } from "node:os";
import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path";
import { minimatch } from "minimatch";
import { addIgnoreRules, toPosixPath, type IgnoreMatcher } from "../../shared/ignore-rules.js";
@@ -123,6 +132,14 @@ function getHomeDir(): string {
return process.env.HOME || homedir();
}
function getAgentResourceTempDir(agentDir: string): string {
const tempDir = join(agentDir, "tmp", "resources");
// Temporary packages can contain executable code, so other local users must not modify them.
mkdirSync(tempDir, { recursive: true, mode: 0o700 });
chmodSync(tempDir, 0o700);
return tempDir;
}
function isPattern(s: string): boolean {
return (
s.startsWith("!") ||
@@ -935,7 +952,7 @@ export class DefaultPackageManager implements PackageManager {
.update(`${prefix}-${suffix ?? ""}`)
.digest("hex")
.slice(0, 8);
return join(tmpdir(), "openclaw-resources", prefix, hash, suffix ?? "");
return join(getAgentResourceTempDir(this.agentDir), prefix, hash, suffix ?? "");
}
private getBaseDirForScope(scope: SourceScope): string {
+6 -4
View File
@@ -4,7 +4,7 @@
*/
import { execSync, spawnSync } from "node:child_process";
import { getBashShellConfig } from "../shell-utils.js";
import { buildShellCommandInvocation, getBashShellConfig } from "../shell-utils.js";
// Cache for shell command results (persists for process lifetime)
const commandResultCache = new Map<string, string | undefined>();
@@ -27,11 +27,13 @@ function executeWithConfiguredShell(command: string): {
value: string | undefined;
} {
try {
const { shell, args } = getBashShellConfig();
const result = spawnSync(shell, [...args, command], {
const invocation = buildShellCommandInvocation(command, getBashShellConfig());
const [shell, ...args] = invocation.argv;
const result = spawnSync(shell, args, {
encoding: "utf-8",
...(invocation.input === undefined ? {} : { input: invocation.input }),
timeout: 10000,
stdio: ["ignore", "pipe", "ignore"],
stdio: [invocation.stdin, "pipe", "ignore"],
shell: false,
windowsHide: true,
});
+2 -1
View File
@@ -132,9 +132,10 @@ export function buildSessionContext(
const path: SessionEntry[] = [];
let current: SessionEntry | undefined = leaf;
while (current) {
path.unshift(current);
path.push(current);
current = current.parentId ? byId.get(current.parentId) : undefined;
}
path.reverse();
return buildCoreSessionContext(path as CoreSessionTreeEntry[]) as SessionContext;
}
@@ -127,7 +127,7 @@ export class SessionManagerEntries extends SessionManagerPersistence {
id: generateSessionEntryId(this.byId),
parentId: this.appendParentId,
timestamp: new Date().toISOString(),
name: name.trim(),
name: name.replace(/[\r\n]+/g, " ").trim(),
};
this.appendEntry(entry);
return entry.id;
@@ -222,12 +222,13 @@ export class SessionManagerEntries extends SessionManagerPersistence {
const current = this.byId.get(currentId);
if (current) {
const normalizedCurrent = this.normalizeEntryParent(current);
path.unshift(normalizedCurrent);
path.push(normalizedCurrent);
currentId = normalizedCurrent.parentId;
} else {
currentId = this.opaqueParentsById.get(currentId) ?? null;
}
}
path.reverse();
return path;
}
@@ -19,6 +19,7 @@ import { prepareSessionManagerForRun } from "../embedded-agent-runner/session-ma
import { repairSessionFileIfNeeded } from "../session-file-repair.js";
import { loadSqliteMarkedSessionFile } from "./session-manager-file.js";
import {
buildSessionContext,
CURRENT_SESSION_VERSION,
findMostRecentSession,
loadEntriesFromFile,
@@ -26,6 +27,7 @@ import {
SessionManager,
type FileEntry,
type SessionEntry,
type SessionMessageEntry,
} from "./session-manager.js";
const tempPaths: string[] = [];
@@ -149,6 +151,66 @@ describe("SessionManager.open", () => {
);
});
it("preserves root-to-leaf ordering across session branches", () => {
const entries = [
{
type: "message",
id: "root",
parentId: null,
timestamp: "2026-07-16T00:00:00.000Z",
message: { role: "user", content: "root", timestamp: 1 },
},
{
type: "message",
id: "main-leaf",
parentId: "root",
timestamp: "2026-07-16T00:00:01.000Z",
message: { role: "user", content: "main", timestamp: 2 },
},
{
type: "message",
id: "side-middle",
parentId: "root",
timestamp: "2026-07-16T00:00:02.000Z",
message: { role: "user", content: "side middle", timestamp: 3 },
},
{
type: "message",
id: "side-leaf",
parentId: "side-middle",
timestamp: "2026-07-16T00:00:03.000Z",
message: { role: "user", content: "side leaf", timestamp: 4 },
},
] satisfies SessionMessageEntry[];
const manager = SessionManager.inMemory();
for (const entry of entries) {
manager.appendMessage(entry.message);
if (entry.id === "main-leaf") {
manager.branch(manager.getBranch().at(0)!.id);
}
}
expect(buildSessionContext(entries, "side-leaf").messages).toMatchObject([
{ content: "root" },
{ content: "side middle" },
{ content: "side leaf" },
]);
expect(
manager
.getBranch()
.filter((entry) => entry.type === "message")
.map((entry) => entry.message),
).toMatchObject([{ content: "root" }, { content: "side middle" }, { content: "side leaf" }]);
});
it("normalizes session names to one line", () => {
const manager = SessionManager.inMemory();
manager.appendSessionInfo(" first\nsecond\r\nthird ");
expect(manager.getSessionName()).toBe("first second third");
});
it("ignores opaque SQLite rows while resolving the session cwd", async () => {
const dir = await makeTempDir();
const storePath = path.join(dir, "sessions.json");
+33 -5
View File
@@ -2,7 +2,8 @@ import path from "node:path";
// Bash tool helper tests cover conversion from model-facing timeout seconds to
// timer-safe millisecond values.
import { MAX_TIMER_TIMEOUT_MS } from "@openclaw/normalization-core/number-coercion";
import { describe, expect, it } from "vitest";
import { describe, expect, it, vi } from "vitest";
import { buildShellCommandInvocation } from "../../shell-utils.js";
import type { BashOperations } from "./bash-operations.js";
import { createBashTool, createLocalBashOperations } from "./bash.js";
import { resolveBashTimeoutMs } from "./bash.test-support.js";
@@ -19,11 +20,38 @@ describe("bash tool timeout helpers", () => {
expect(resolveBashTimeoutMs(Number.MAX_SAFE_INTEGER)).toBe(MAX_TIMER_TIMEOUT_MS);
});
it("ignores absent, invalid, and non-positive timeout seconds", () => {
it("allows an absent timeout", () => {
expect(resolveBashTimeoutMs(undefined)).toBeUndefined();
expect(resolveBashTimeoutMs(Number.NaN)).toBeUndefined();
expect(resolveBashTimeoutMs(0)).toBeUndefined();
expect(resolveBashTimeoutMs(-1)).toBeUndefined();
});
it.each([Number.NaN, 0, -1])("rejects invalid timeout %s", (timeout) => {
expect(() => resolveBashTimeoutMs(timeout)).toThrow(
"Invalid timeout: must be a positive finite number of seconds",
);
});
it.each([Number.NaN, 0, -1])("rejects invalid timeout %s before execution", async (timeout) => {
const exec = vi.fn<BashOperations["exec"]>();
const tool = createBashTool(process.cwd(), { operations: { exec } });
await expect(
tool.execute("call-invalid-timeout", { command: "echo ok", timeout }),
).rejects.toThrow("Invalid timeout: must be a positive finite number of seconds");
expect(exec).not.toHaveBeenCalled();
});
it("omits the command argv and supplies stdin for legacy WSL bash", () => {
expect(
buildShellCommandInvocation("printf ready", {
shell: "C:\\Windows\\System32\\bash.exe",
args: ["-s"],
commandTransport: "stdin",
}),
).toEqual({
argv: ["C:\\Windows\\System32\\bash.exe", "-s"],
input: "printf ready",
stdin: "pipe",
});
});
});
+17 -6
View File
@@ -15,7 +15,12 @@ import { keyHint } from "../../modes/interactive/components/keybinding-hints.js"
import { truncateToVisualLines } from "../../modes/interactive/components/visual-truncate.js";
import { theme } from "../../modes/interactive/theme/theme.js";
import type { AgentTool } from "../../runtime/index.js";
import { getBashShellConfig, getShellEnv, killProcessTree } from "../../shell-utils.js";
import {
buildShellCommandInvocation,
getBashShellConfig,
getShellEnv,
killProcessTree,
} from "../../shell-utils.js";
import type { ToolDefinition, ToolRenderResultOptions } from "../extensions/types.js";
import type { BashOperations } from "./bash-operations.js";
import { OutputAccumulator } from "./output-accumulator.js";
@@ -29,12 +34,15 @@ const bashSchema = Type.Object({
timeout: Type.Optional(Type.Number({ description: "Optional timeout seconds; default none." })),
});
function resolveBashTimeoutMs(timeoutSeconds: unknown): number | undefined {
if (timeoutSeconds === undefined) {
return undefined;
}
if (
typeof timeoutSeconds !== "number" ||
!Number.isFinite(timeoutSeconds) ||
timeoutSeconds <= 0
) {
return undefined;
throw new Error("Invalid timeout: must be a positive finite number of seconds");
}
return resolveTimerTimeoutMs(timeoutSeconds * 1000, 1);
}
@@ -55,21 +63,23 @@ export function createLocalBashOperations(options?: { shellPath?: string }): Bas
return {
exec: (command, cwd, { onData, signal, timeout, env }) => {
return new Promise((resolve, reject) => {
const { shell, args } = getBashShellConfig(options?.shellPath);
const shellConfig = getBashShellConfig(options?.shellPath);
const invocation = buildShellCommandInvocation(command, shellConfig);
if (!existsSync(cwd)) {
reject(
new Error(`Working directory does not exist: ${cwd}\nCannot execute bash commands.`),
);
return;
}
const child = spawnCommand([shell, ...args, command], {
const child = spawnCommand(invocation.argv, {
baseEnv: {},
buffer: false,
cwd,
detached: process.platform !== "win32",
env: env ?? getShellEnv(),
...(invocation.input === undefined ? {} : { input: invocation.input }),
reject: false,
stdio: ["ignore", "pipe", "pipe"],
stdio: [invocation.stdin, "pipe", "pipe"],
});
const releaseOutput = releaseChildProcessOutputAfterExit(child);
let timedOut = false;
@@ -105,7 +115,7 @@ export function createLocalBashOperations(options?: { shellPath?: string }): Bas
if (result instanceof Error) {
throw result;
}
throw new Error(`Failed to launch shell: ${shell}`, { cause: result });
throw new Error(`Failed to launch shell: ${shellConfig.shell}`, { cause: result });
}
if (timeoutHandle) {
clearTimeout(timeoutHandle);
@@ -310,6 +320,7 @@ export function createBashToolDefinition(
) {
void toolCallId;
void ctx;
resolveBashTimeoutMs(timeout);
const resolvedCommand = commandPrefix ? `${commandPrefix}\n${command}` : command;
const spawnContext = resolveSpawnContext(resolvedCommand, cwd, spawnHook);
const output = new OutputAccumulator({ tempFilePrefix: "openclaw-bash" });
+9 -3
View File
@@ -222,15 +222,21 @@ describe("edit tool", () => {
expect(applyPatch(original, details.patch)).toBe(expected);
});
it("strips model-added metadata while retaining the strict edit schema", async () => {
it("accepts and strips model-added metadata while keeping required fields strict", async () => {
const filePath = await createTempFile("before\n");
const tool = createEditTool(tmpDir);
const prepared = tool.prepareArguments?.({
const raw = {
path: filePath,
reason: "model explanation",
edits: [{ oldText: "before", newText: "after", reason: "why" }],
});
};
const prepared = tool.prepareArguments?.(raw);
expect(Value.Check(tool.parameters, raw)).toBe(true);
expect(Value.Check(tool.parameters, { edits: raw.edits })).toBe(false);
expect(Value.Check(tool.parameters, { path: filePath, edits: [{ oldText: "before" }] })).toBe(
false,
);
expect(prepared).toEqual({
path: filePath,
edits: [{ oldText: "before", newText: "after" }],
+2 -2
View File
@@ -53,7 +53,7 @@ const replaceEditSchema = Type.Object(
description: "Replacement text.",
}),
},
{ additionalProperties: false },
{},
);
const editSchema = Type.Object(
@@ -66,7 +66,7 @@ const editSchema = Type.Object(
"Targeted replacements against original file; no overlap/nesting. Merge nearby changes.",
}),
},
{ additionalProperties: false },
{},
);
type LegacyEditToolInput = Record<string, unknown> & {
edits?: unknown;
+54 -2
View File
@@ -5,6 +5,7 @@ import path from "node:path";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { captureEnv } from "../test-utils/env.js";
import {
buildShellCommandInvocation,
detectRuntimeShell,
getBashShellConfig,
getShellEnv,
@@ -55,6 +56,8 @@ function createTempCommandDir(
return dir;
}
type ShellConfig = ReturnType<typeof getBashShellConfig>;
describe("getShellConfig", () => {
let envSnapshot: ReturnType<typeof captureEnv>;
const tempDirs: string[] = [];
@@ -138,7 +141,11 @@ describe("getShellConfig", () => {
const binDir = createTempCommandDir(tempDirs, [{ name: "zsh" }]);
const shellPath = path.join(binDir, "zsh");
expect(getShellConfig(shellPath)).toEqual({ shell: shellPath, args: ["-f", "-c"] });
expect(getShellConfig(shellPath)).toEqual({
shell: shellPath,
args: ["-f", "-c"],
commandTransport: "argv",
});
});
it("rejects a missing explicit custom shell path", () => {
@@ -211,7 +218,52 @@ describe("getBashShellConfig", () => {
process.env.ProgramFiles = programFiles;
process.env.PATH = "";
expect(getBashShellConfig()).toEqual({ shell: bashPath, args: ["-c"] });
expect(getBashShellConfig()).toEqual({
shell: bashPath,
args: ["-c"],
commandTransport: "argv",
});
});
it.each(["System32", "Sysnative"])(
"uses stdin transport for the legacy %s WSL launcher",
(systemDirectory) => {
const shellPath = `C:\\Windows\\${systemDirectory}\\bash.exe`;
vi.spyOn(fs, "existsSync").mockImplementation((candidate) => String(candidate) === shellPath);
expect(getBashShellConfig(shellPath)).toEqual({
shell: shellPath,
args: ["-s"],
commandTransport: "stdin",
});
},
);
it("builds a stdin invocation for the legacy WSL launcher", () => {
const config: ShellConfig = {
shell: "C:\\Windows\\System32\\bash.exe",
args: ["-s"],
commandTransport: "stdin",
};
expect(buildShellCommandInvocation("printf ready", config)).toEqual({
argv: [config.shell, "-s"],
input: "printf ready",
stdin: "pipe",
});
});
it("builds an argv invocation for regular shells", () => {
const config: ShellConfig = {
shell: "/bin/bash",
args: ["-c"],
commandTransport: "argv",
};
expect(buildShellCommandInvocation("printf ready", config)).toEqual({
argv: [config.shell, "-c", "printf ready"],
stdin: "ignore",
});
});
});
+49 -14
View File
@@ -13,9 +13,17 @@ import {
} from "../process/kill-tree.js";
import { getBinDir } from "./config.js";
interface ShellConfig {
type ShellConfig = {
shell: string;
args: string[];
} & ({ commandTransport: "argv" } | { commandTransport: "stdin" });
type ShellCommandInvocation =
| { argv: [string, ...string[]]; input?: undefined; stdin: "ignore" }
| { argv: [string, ...string[]]; input: string; stdin: "pipe" };
function createArgvShellConfig(shell: string, args: string[]): ShellConfig {
return { shell, args, commandTransport: "argv" };
}
function resolvePowerShellPath(): string {
@@ -93,12 +101,38 @@ function resolveWindowsBashPath(env: NodeJS.ProcessEnv = process.env): string |
return resolveShellFromPath("bash.exe", env) ?? resolveShellFromPath("bash", env);
}
function isLegacyWslBashPath(shellPath: string): boolean {
const normalized = shellPath.replace(/\//g, "\\").toLowerCase();
return /(?:^|\\)windows\\(?:system32|sysnative)\\bash\.exe$/.test(normalized);
}
function resolveBashCommandConfig(shell: string): ShellConfig {
if (isLegacyWslBashPath(shell)) {
return { shell, args: ["-s"], commandTransport: "stdin" };
}
return createArgvShellConfig(
shell,
process.platform === "win32" ? ["-c"] : getPosixShellArgs(shell),
);
}
export function buildShellCommandInvocation(
command: string,
config: ShellConfig,
): ShellCommandInvocation {
if (config.commandTransport === "stdin") {
// The legacy WSL launcher mangles command argv, so its -s mode must read the command from stdin.
return { argv: [config.shell, ...config.args], input: command, stdin: "pipe" };
}
return { argv: [config.shell, ...config.args, command], stdin: "ignore" };
}
export function getShellConfig(customShellPath?: string): ShellConfig {
if (customShellPath) {
if (!fs.existsSync(customShellPath)) {
throw new Error(`Custom shell path not found: ${customShellPath}`);
}
return { shell: customShellPath, args: getPosixShellArgs(customShellPath) };
return createArgvShellConfig(customShellPath, getPosixShellArgs(customShellPath));
}
if (process.platform === "win32") {
@@ -107,10 +141,11 @@ export function getShellConfig(customShellPath?: string): ShellConfig {
// directly to the console via WriteConsole API, bypassing stdout pipes.
// When Node.js spawns cmd.exe with piped stdio, these utilities produce no output.
// PowerShell properly captures and redirects their output to stdout.
return {
shell: resolvePowerShellPath(),
args: ["-NoProfile", "-NonInteractive", "-Command"],
};
return createArgvShellConfig(resolvePowerShellPath(), [
"-NoProfile",
"-NonInteractive",
"-Command",
]);
}
const rawEnvShell = process.env.SHELL?.trim();
@@ -120,20 +155,20 @@ export function getShellConfig(customShellPath?: string): ShellConfig {
if (shellName === "fish") {
const bash = resolveShellFromPath("bash");
if (bash) {
return { shell: bash, args: getPosixShellArgs(bash) };
return createArgvShellConfig(bash, getPosixShellArgs(bash));
}
const sh = resolveShellFromPath("sh");
if (sh) {
return { shell: sh, args: getPosixShellArgs(sh) };
return createArgvShellConfig(sh, getPosixShellArgs(sh));
}
}
if (envShell) {
return { shell: envShell, args: getPosixShellArgs(envShell) };
return createArgvShellConfig(envShell, getPosixShellArgs(envShell));
}
// Placeholder SHELL (or unset): prefer a resolved sh/bash on PATH so we do not
// re-invoke the placeholder and get a spurious exitCode=1.
const shell = resolveShellFromPath("sh") ?? resolveShellFromPath("bash") ?? "sh";
return { shell, args: getPosixShellArgs(shell) };
return createArgvShellConfig(shell, getPosixShellArgs(shell));
}
export function getBashShellConfig(customShellPath?: string): ShellConfig {
@@ -141,19 +176,19 @@ export function getBashShellConfig(customShellPath?: string): ShellConfig {
if (!fs.existsSync(customShellPath)) {
throw new Error(`Custom shell path not found: ${customShellPath}`);
}
return { shell: customShellPath, args: getPosixShellArgs(customShellPath) };
return resolveBashCommandConfig(customShellPath);
}
if (process.platform === "win32") {
const bash = resolveWindowsBashPath();
if (bash) {
return { shell: bash, args: ["-c"] };
return resolveBashCommandConfig(bash);
}
throw new Error("No bash shell found. Install Git for Windows or add bash.exe to PATH.");
}
if (fs.existsSync("/bin/bash")) {
return { shell: "/bin/bash", args: getPosixShellArgs("/bin/bash") };
return resolveBashCommandConfig("/bin/bash");
}
const shell =
@@ -161,7 +196,7 @@ export function getBashShellConfig(customShellPath?: string): ShellConfig {
resolveShellFromWhich("bash") ??
resolveShellFromPath("sh") ??
"sh";
return { shell, args: getPosixShellArgs(shell) };
return resolveBashCommandConfig(shell);
}
function resolveShellFromPath(