Skip to content

fix(backup+admission): drop redundant api.resolvePath on already-absolute paths (Issue #682, supersedes PR #695)#743

Open
jlin53882 wants to merge 1 commit intoCortexReach:masterfrom
jlin53882:james/handle-pr695-backup-682
Open

fix(backup+admission): drop redundant api.resolvePath on already-absolute paths (Issue #682, supersedes PR #695)#743
jlin53882 wants to merge 1 commit intoCortexReach:masterfrom
jlin53882:james/handle-pr695-backup-682

Conversation

@jlin53882
Copy link
Copy Markdown
Contributor

@jlin53882 jlin53882 commented May 4, 2026

Summary

Fixes Issue #682 — two code paths crashed on OpenClaw 2026.4.x with:

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type
string or an instance of Buffer or URL. Received undefined.

This PR supersedes PR #695 (closed 2026-05-04 by maintainer due to inactivity after review).


Root Cause

OpenClaw 2026.4.x tightened the plugin API: calling api.resolvePath() on an already-absolute path that points outside the agent workspace root now returns undefined.

Two call sites had this pattern:

1. runBackup() — plugin internal timer backup

resolvedDbPath is produced by api.resolvePath(...) at plugin init and is always absolute. Inside runBackup():

// BEFORE (crashes on 2026.4.x)
const backupDir = api.resolvePath(
  join(resolvedDbPath, "..", "backups"),
);
// api.resolvePath(absolute-path) → undefined → mkdir crashes

Fix: resolvedDbPath is already absolute — join directly without re-wrapping.

Defensive guard: Additionally guards against resolvedDbPath being undefined (edge case where api.resolvePath() returns undefined for empty-string input rather than throwing), preventing the TypeError at the join() call site.

2. createAdmissionRejectionAuditWriter() — admission audit sidecar

Same redundant wrapper pattern:

// BEFORE (same crash)
const filePath = api.resolvePath(
  resolveRejectedAuditFilePath(resolvedDbPath, config.admissionControl),
);

resolveRejectedAuditFilePath() returns:

Scenario Return value Re-resolve needed?
No explicit config join(dbPath, "..", "admission-audit", "rejections.jsonl") — already absolute NO
Explicit relative path "data/audit/rejections.jsonl" — caller must resolve YES
Explicit absolute path "/var/log/memory/rejections.jsonl" — must NOT be re-resolved NO

Fix: Guard with startsWith("/") — only resolve relative paths.


Changes

index.tsrunBackup() (~line 4015)

  async function runBackup() {
    try {
+     // ── 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.
+     if (!resolvedDbPath || typeof resolvedDbPath !== "string") {
+       api.logger.warn(
+         `memory-lancedb-pro: backup skipped — resolvedDbPath is "${String(resolvedDbPath)}"`,
+       );
+       return;
+     }
+
      // resolvedDbPath is already absolute; wrapping it again triggered
      // api.resolvePath(absolute-path) → undefined in OpenClaw 2026.4.x.
      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 });

index.tscreateAdmissionRejectionAuditWriter() (~line 1675)

- const filePath = api.resolvePath(
-   resolveRejectedAuditFilePath(resolvedDbPath, config.admissionControl),
- );
+ const rawPath = resolveRejectedAuditFilePath(resolvedDbPath, config.admissionControl);
+ const filePath = rawPath.startsWith("/") ? rawPath : api.resolvePath(rawPath);

test/admission-rejection-audit-path.test.mjs — new regression test (11 cases)

Covers all three resolveRejectedAuditFilePath() scenarios:

  • Default derived path is always absolute (no re-resolve needed)
  • Explicit relative path returned as-is for caller to resolve
  • Explicit absolute path returned as-is and must NOT be re-resolved

Relationship to PR #695

PR #695 (closed 2026-05-04, commit 82b0b6b) was opened by an external contributor and closed due to inactivity after maintainer review. Maintainer feedback:

"The direction is good and the one-line backup path change looks like the right root-cause fix."

"One related follow-up: there appears to be the same redundant resolve pattern in the admission rejection audit path. It may not need to be in this PR if admission-control is out of scope, but please either fix it here or split a follow-up so the same class of path bug does not remain."

This PR:


Verification

# After applying patch, restart gateway:
# 2026-04-24T23:29:45.785+09:00 [plugins] memory-lancedb-pro: backup completed

# Regression tests:
node --test test/admission-rejection-audit-path.test.mjs
# → 11/11 pass

Test Plan

  • Apply patch, restart gateway on OpenClaw 2026.4.x
  • Confirm backup completed log appears within ~60s (no TypeError)
  • Run full npm test — no regression
  • If admission control configured with explicit path, verify audit writes correctly

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 136cff7247

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread index.ts
// 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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Detect absolute audit paths with path.isAbsolute

The new absolute-path guard only checks rawPath.startsWith("/"), which misses Windows absolute forms such as C:\\logs\\rejections.jsonl and \\\\server\\share\\rejections.jsonl. In those cases this branch still calls api.resolvePath(rawPath) on an already-absolute path; under the same strict OpenClaw behavior this patch is addressing, that can return undefined, and the writer will fail at runtime when it reaches dirname(filePath)/appendFile(...), silently dropping rejection-audit writes on Windows setups. Use a platform-aware absolute check (for example path.isAbsolute) before deciding to call api.resolvePath.

Useful? React with 👍 / 👎.

@jlin53882 jlin53882 changed the title fix(backup+admission): drop redundant api.resolvePath on already-absolute paths (Issue #682) fix(backup+admission): drop redundant api.resolvePath on already-absolute paths (Issue #682, supersedes PR #695) May 4, 2026
…lute paths (Issue CortexReach#682)

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 CortexReach#695 (closed due to inactivity after maintainer review).
Maintainer feedback from PR CortexReach#695:
  - direction confirmed correct
  - admission audit path same bug flagged as follow-up suggestion
  - regression test requested
@jlin53882 jlin53882 force-pushed the james/handle-pr695-backup-682 branch from 136cff7 to 48b6940 Compare May 6, 2026 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant