-
Notifications
You must be signed in to change notification settings - Fork 713
fix(backup+admission): drop redundant api.resolvePath on already-absolute paths (Issue #682, supersedes PR #695) #743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jlin53882
wants to merge
1
commit into
CortexReach:master
Choose a base branch
from
jlin53882:james/handle-pr695-backup-682
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+154
−6
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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}`); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new absolute-path guard only checks
rawPath.startsWith("/"), which misses Windows absolute forms such asC:\\logs\\rejections.jsonland\\\\server\\share\\rejections.jsonl. In those cases this branch still callsapi.resolvePath(rawPath)on an already-absolute path; under the same strict OpenClaw behavior this patch is addressing, that can returnundefined, and the writer will fail at runtime when it reachesdirname(filePath)/appendFile(...), silently dropping rejection-audit writes on Windows setups. Use a platform-aware absolute check (for examplepath.isAbsolute) before deciding to callapi.resolvePath.Useful? React with 👍 / 👎.