From 48b69403357d716e3fb7ee295e534e1f67fc1cc5 Mon Sep 17 00:00:00 2001 From: James Date: Mon, 4 May 2026 23:52:43 +0800 Subject: [PATCH] fix(backup+admission): drop redundant api.resolvePath on already-absolute paths (Issue #682) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: OpenClaw 2026.4.x strict plugin API — calling api.resolvePath() on an already-absolute path that points outside the workspace root returns undefined. Two call sites were affected: 1. runBackup() — plugin's own 60s-after-start and every-24h internal timer backup crashed with: TypeError [ERR_INVALID_ARG_TYPE]: The path argument must be of type string or an instance of Buffer or URL. Received undefined. 2. createAdmissionRejectionAuditWriter() — same pattern, same root cause. Fix for both: - runBackup(): resolvedDbPath is already absolute; join() directly without re-wrapping in api.resolvePath(). - admission writer: apply same guard — if rawPath starts with '/' (already absolute) use as-is, otherwise call api.resolvePath() for relative paths. Also adds test/admission-rejection-audit-path.test.mjs covering the three resolveRejectedAuditFilePath() path-construction scenarios: (a) default — always returns already-absolute path (no re-resolve needed) (b) explicit relative path — returned as-is for caller to resolve (c) explicit absolute path — returned as-is, must NOT be re-resolved Supersedes PR #695 (closed due to inactivity after maintainer review). Maintainer feedback from PR #695: - direction confirmed correct - admission audit path same bug flagged as follow-up suggestion - regression test requested --- index.ts | 43 ++++++- test/admission-rejection-audit-path.test.mjs | 117 +++++++++++++++++++ 2 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 test/admission-rejection-audit-path.test.mjs diff --git a/index.ts b/index.ts index 5a9b5fe9..8017b95a 100644 --- a/index.ts +++ b/index.ts @@ -1672,9 +1672,16 @@ function createAdmissionRejectionAuditWriter( return null; } - const filePath = api.resolvePath( - resolveRejectedAuditFilePath(resolvedDbPath, config.admissionControl), - ); + // resolveRejectedAuditFilePath returns: + // - an already-absolute derived path (join of resolvedDbPath + "..") when no + // explicit config is set; these are safe to use directly without re-wrapping + // in api.resolvePath (which returns undefined for already-absolute paths in + // OpenClaw 2026.4.x strict mode). + // - the user-provided explicit path as-is (trimmed). If that path is relative, + // the caller is responsible for resolving it via api.resolvePath. + // Absolute explicit paths pass through unchanged and must NOT be re-resolved. + const rawPath = resolveRejectedAuditFilePath(resolvedDbPath, config.admissionControl); + const filePath = rawPath.startsWith("/") ? rawPath : api.resolvePath(rawPath); return async (entry: AdmissionRejectionAuditEntry) => { try { @@ -4007,9 +4014,33 @@ const memoryLanceDBProPlugin = { async function runBackup() { try { - const backupDir = api.resolvePath( - join(resolvedDbPath, "..", "backups"), - ); + // ── Defensive: guard against undefined resolvedDbPath ───────────────── + // api.resolvePath() may return undefined when config.dbPath is an + // empty string or an unhandled edge-case, rather than throwing. + // This guards against: + // TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be + // of type string or an instance of Buffer or URL. Received undefined + // (reported: backup failed at join(resolvedDbPath, "..", "backups")) + if (!resolvedDbPath || typeof resolvedDbPath !== "string") { + api.logger.warn( + `memory-lancedb-pro: backup skipped — resolvedDbPath is "${String(resolvedDbPath)}"`, + ); + return; + } + + // resolvedDbPath is already absolute (produced by api.resolvePath at + // plugin init); wrapping it again triggered a path-argument `undefined` + // in OpenClaw 2026.4.x's stricter plugin API. Join directly. + const backupDir = join(resolvedDbPath, "..", "backups"); + + // ── Secondary guard: ensure join() also returned a valid string ────── + if (!backupDir || typeof backupDir !== "string") { + api.logger.warn( + `memory-lancedb-pro: backup skipped — backupDir resolved to "${String(backupDir)}"`, + ); + return; + } + await mkdir(backupDir, { recursive: true }); const allMemories = await store.list(undefined, undefined, 10000, 0); diff --git a/test/admission-rejection-audit-path.test.mjs b/test/admission-rejection-audit-path.test.mjs new file mode 100644 index 00000000..c95e6fc7 --- /dev/null +++ b/test/admission-rejection-audit-path.test.mjs @@ -0,0 +1,117 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import jitiFactory from "jiti"; +import { join } from "path"; + +const jiti = jitiFactory(import.meta.url, { interopDefault: true }); + +const { resolveRejectedAuditFilePath } = jiti("../src/admission-control.ts"); + +// ============================================================================ +// resolveRejectedAuditFilePath — path construction regression tests +// +// Issue #682 (PR #695): OpenClaw 2026.4.x strict plugin API causes +// api.resolvePath(already-absolute-path) → undefined +// +// This function is the shared path-construction layer used by both: +// - runBackup() in index.ts (fixed by PR #695) +// - admission rejection audit writer in index.ts (fixed alongside PR #695) +// +// When no explicit rejectedAuditFilePath is configured, the default derived +// path is already absolute (join of resolvedDbPath + ".."). The caller must +// NOT wrap it again in api.resolvePath. +// +// When an explicit path IS configured, the caller is responsible for resolving +// it based on whether it is relative or absolute. +// ============================================================================ + +describe("resolveRejectedAuditFilePath", () => { + const ABSOLUTE_DB_PATH = "/home/user/.openclaw/memory/lancedb-pro"; + + // -------------------------------------------------------------------------- + // Default path — always derived from dbPath, always absolute + // -------------------------------------------------------------------------- + it("returns an already-absolute path when no explicit config is set", () => { + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, null); + assert.ok(result.startsWith("/"), `Expected absolute path, got: ${result}`); + }); + + it("derived path contains admission-audit segment", () => { + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, null); + assert.ok( + result.includes("admission-audit"), + `Expected "admission-audit" in path, got: ${result}`, + ); + }); + + it("derived path ends with rejections.jsonl", () => { + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, null); + assert.ok( + result.endsWith("rejections.jsonl"), + `Expected "rejections.jsonl" suffix, got: ${result}`, + ); + }); + + it("derived path is based on dbPath .. parent", () => { + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, null); + const expectedParent = join(ABSOLUTE_DB_PATH, ".."); + assert.ok( + result.startsWith(expectedParent), + `Expected path to start with "${expectedParent}", got: ${result}`, + ); + }); + + // -------------------------------------------------------------------------- + // Explicit relative path — returned as-is for caller to resolve + // -------------------------------------------------------------------------- + it("returns explicit relative path as-is (caller must resolve)", () => { + const config = { rejectedAuditFilePath: "data/audit/rejections.jsonl" }; + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, config); + assert.strictEqual(result, "data/audit/rejections.jsonl"); + }); + + it("trims whitespace from explicit relative path", () => { + const config = { rejectedAuditFilePath: " data/audit/rejections.jsonl " }; + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, config); + assert.strictEqual(result, "data/audit/rejections.jsonl"); + }); + + // -------------------------------------------------------------------------- + // Explicit absolute path — returned as-is, must NOT be re-resolved + // -------------------------------------------------------------------------- + it("returns explicit absolute path as-is", () => { + const config = { rejectedAuditFilePath: "/var/log/memory/rejections.jsonl" }; + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, config); + assert.strictEqual(result, "/var/log/memory/rejections.jsonl"); + }); + + it("explicit absolute path starts with / and must not be re-resolved", () => { + const config = { rejectedAuditFilePath: "/custom/path/rejections.jsonl" }; + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, config); + assert.ok(result.startsWith("/"), `Expected absolute, got: ${result}`); + }); + + // -------------------------------------------------------------------------- + // Empty / whitespace-only explicit path — falls back to default + // -------------------------------------------------------------------------- + it("treats empty string explicit path as unset (uses default)", () => { + const config = { rejectedAuditFilePath: "" }; + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, config); + assert.ok(result.endsWith("rejections.jsonl"), `Expected default, got: ${result}`); + }); + + it("treats whitespace-only explicit path as unset (uses default)", () => { + const config = { rejectedAuditFilePath: " " }; + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, config); + assert.ok(result.endsWith("rejections.jsonl"), `Expected default, got: ${result}`); + }); + + // -------------------------------------------------------------------------- + // Config object with no rejectedAuditFilePath key — uses default + // -------------------------------------------------------------------------- + it("uses default when config has no rejectedAuditFilePath key", () => { + const config = {}; + const result = resolveRejectedAuditFilePath(ABSOLUTE_DB_PATH, config); + assert.ok(result.endsWith("rejections.jsonl"), `Expected default, got: ${result}`); + }); +});