Prepare OpenClaw contextEngine review packet#7
Open
dodge1218 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe PR integrates security hardening across the ContextClaw core engine and OpenClaw plugin, introduces comprehensive regression tests for path safety and DoS resilience, and documents the deterministic context-trimming approach as a control-plane guardrail with auditable request ledgers and dogfood evidence. ChangesSecurity Hardening and Evidence
Poem
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
dogfood-runs/2026-05-12-proceed-loop/summarize.mjs (1)
5-5: 💤 Low valueOptional: Windows path handling in
import.meta.url.On Windows,
new URL(import.meta.url).pathnamereturns a path like/C:/path/...with a leading slash, which can cause issues. Consider usingfileURLToPathfromnode:urlfor cross-platform compatibility:-import path from 'node:path'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; -const root = path.dirname(new URL(import.meta.url).pathname); +const root = path.dirname(fileURLToPath(import.meta.url));This is a dogfood script so impact is minimal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dogfood-runs/2026-05-12-proceed-loop/summarize.mjs` at line 5, The calculation of root uses new URL(import.meta.url).pathname which on Windows yields a leading slash (e.g. /C:/...) and can produce incorrect paths; change the implementation that sets root (where you call path.dirname(new URL(import.meta.url).pathname)) to convert import.meta.url to a file path using fileURLToPath from 'node:url' (i.e., call fileURLToPath(import.meta.url) and then pass that to path.dirname) so the root value is correct cross-platform.dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.before (1)
1-4: 💤 Low valueMinor: Absolute paths expose local username in checksum file.
The checksum file contains hardcoded absolute paths with username (
/home/yin/...). For dogfood evidence this is acceptable, but consider using relative paths in future artifacts to avoid leaking local directory structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.before` around lines 1 - 4, The SHA256SUMS.before file contains absolute home paths revealing the local username; update the checksum generation to emit relative paths (or strip the /home/yin/... prefix) so entries like "efficiency-before.json", "ledger-before.jsonl", "start.txt", and "stats-before.json" are recorded without the full home directory; locate where SHA256SUMS.before is produced and change the routine that writes path strings to use relative paths (or path.basename) before writing checksums.README.md (1)
121-123: ⚡ Quick winPrefer
npm ciovernpm installin setup instructions.For reproducible plugin setup (and alignment with CI behavior), use
npm ciin this block when a lockfile is present.Suggested doc patch
cd ~/.openclaw/workspace/contextclaw/plugin -npm install +npm ci🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 121 - 123, Update the README.md setup snippet to prefer reproducible installs by replacing the "npm install" command with "npm ci" (i.e., locate the block containing the shell commands that include "cd ~/.openclaw/workspace/contextclaw/plugin" and change the "npm install" line to "npm ci"), and add a brief note that this should be used when a lockfile (package-lock.json or npm-shrinkwrap.json) is present to align with CI behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/DOCTRINE_AGENT_LOOP_ECONOMICS.md`:
- Line 100: Replace the inline token "runIds" in the documentation with the
human-readable phrase "run IDs" for consistency and readability; search for the
exact string "runIds" (e.g., the occurrence in the sentence "The gateway
produced 26 distinct runIds across a 7-day window.") and update it to "run IDs"
while preserving the surrounding sentence and punctuation.
- Around line 10-14: Replace the four absolute local paths listed in
DOCTRINE_AGENT_LOOP_ECONOMICS.md with repo-relative or redacted references:
change
`/home/yin/.openclaw/workspace/memory/permanent/contextclaw-v2-vision.md`,
`/home/yin/.openclaw/workspace/memory/permanent/contextclaw-seatbelt-doctrine.md`,
`/home/yin/.openclaw/workspace/memory/permanent/contextclaw-spend-ledger-doctrine.md`,
and `/home/yin/.openclaw/workspace/outputs/openclaw-recovery-todo-20260418.md`
to either repository-relative paths (e.g.,
./memory/permanent/contextclaw-v2-vision.md) or a sanitized placeholder (e.g.,
<REPO_ROOT>/memory/permanent/contextclaw-v2-vision.md or <REDACTED_HOME>/... )
ensuring no local usernames or absolute /home/... paths remain.
In `@docs/NEXT_TICKETS_OPENCLAW_PUSH.md`:
- Line 47: Replace the current "session key(s)" requirement with a directive to
publish only hashed or redacted session identifiers: specify the hashing
algorithm (e.g., SHA-256) and salt/pepper policy, include an example of the
redaction format, and update the guidance to state that raw session keys must
never be included in evidence packets; ensure the document text that mentions
"session key(s)" is updated to reference "hashed/redacted session identifier(s)"
and to document the hashing method and verification process.
In `@dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.after`:
- Line 1: The checksum manifest
dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.after contains a
self-referential entry with the SHA-256 of an empty file (e3b0c442...), which is
invalid for validating the manifest; fix this by removing the self-entry line
from SHA256SUMS.after (or alternatively compute and store the manifest's hash in
a separate file rather than including it inside the manifest itself) so the
manifest only lists checksums for other files.
In `@plugin/index.js`:
- Around line 156-168: Replace the local _sanitizeIdSegment implementation with
the shared sanitizer from core: import and call sanitizeSegment (from the core
export) instead of duplicating logic; ensure you preserve the existing behavior
by passing the input and maxLen (default 64) through to sanitizeSegment and keep
the same fallback for null/undefined or empty results (returning 'unknown') so
callers of _sanitizeIdSegment continue to behave the same while using the
centralized sanitizeSegment implementation.
---
Nitpick comments:
In `@dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.before`:
- Around line 1-4: The SHA256SUMS.before file contains absolute home paths
revealing the local username; update the checksum generation to emit relative
paths (or strip the /home/yin/... prefix) so entries like
"efficiency-before.json", "ledger-before.jsonl", "start.txt", and
"stats-before.json" are recorded without the full home directory; locate where
SHA256SUMS.before is produced and change the routine that writes path strings to
use relative paths (or path.basename) before writing checksums.
In `@dogfood-runs/2026-05-12-proceed-loop/summarize.mjs`:
- Line 5: The calculation of root uses new URL(import.meta.url).pathname which
on Windows yields a leading slash (e.g. /C:/...) and can produce incorrect
paths; change the implementation that sets root (where you call path.dirname(new
URL(import.meta.url).pathname)) to convert import.meta.url to a file path using
fileURLToPath from 'node:url' (i.e., call fileURLToPath(import.meta.url) and
then pass that to path.dirname) so the root value is correct cross-platform.
In `@README.md`:
- Around line 121-123: Update the README.md setup snippet to prefer reproducible
installs by replacing the "npm install" command with "npm ci" (i.e., locate the
block containing the shell commands that include "cd
~/.openclaw/workspace/contextclaw/plugin" and change the "npm install" line to
"npm ci"), and add a brief note that this should be used when a lockfile
(package-lock.json or npm-shrinkwrap.json) is present to align with CI behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04f2f9cf-3e5e-4487-b7f5-ac8aa57475b9
⛔ Files ignored due to path filters (1)
plugin/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
.github/workflows/ci.ymlREADME.mddocs/DOCTRINE_AGENT_LOOP_ECONOMICS.mddocs/FOUNDER_VISION_CONTEXTCLAW.mddocs/MAINTAINER_NOTE_DRAFT.mddocs/MAINTAINER_TRIAGE_2026-05-12.mddocs/MEASUREMENT.mddocs/NEXT_TICKETS_OPENCLAW_PUSH.mddocs/openclaw-dogfood-2026-05-12.mddogfood-runs/2026-05-12-proceed-loop/README.mddogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.afterdogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.beforedogfood-runs/2026-05-12-proceed-loop/efficiency-after.jsondogfood-runs/2026-05-12-proceed-loop/efficiency-before.jsondogfood-runs/2026-05-12-proceed-loop/end.txtdogfood-runs/2026-05-12-proceed-loop/ledger-after.jsonldogfood-runs/2026-05-12-proceed-loop/ledger-before.jsonldogfood-runs/2026-05-12-proceed-loop/start.txtdogfood-runs/2026-05-12-proceed-loop/stats-after.jsondogfood-runs/2026-05-12-proceed-loop/stats-before.jsondogfood-runs/2026-05-12-proceed-loop/summarize.mjsdogfood-runs/2026-05-12-proceed-loop/summary.mdpackage.jsonpackages/core/src/__tests__/security.test.tspackages/core/src/memory.tspackages/core/src/path-safety.tspackages/core/src/watcher.tsplugin/__tests__/security.test.jsplugin/classifier.jsplugin/index.jsplugin/package.jsonplugin/policy.js
Comment on lines
+156
to
+168
| // Whitelist filename charset to defeat path traversal in user-influenced | ||
| // segments (sessionId may originate from outside the engine). | ||
| function _sanitizeIdSegment(input, maxLen = 64) { | ||
| if (input === null || input === undefined) return 'unknown'; | ||
| let s = String(input); | ||
| s = s.replace(/[/\\]/g, '_'); | ||
| s = s.replace(/[^A-Za-z0-9._-]/g, '_'); | ||
| s = s.replace(/^\.+/, '_'); | ||
| if (!s) return 'unknown'; | ||
| if (s.length > maxLen) s = s.slice(0, maxLen); | ||
| return s; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check core path-safety.ts for similar sanitization patterns
rg -nP --type=ts -C5 'sanitizeSegment|sanitizePointer' packages/core/src/path-safety.tsRepository: dodge1218/contextclaw
Length of output: 1736
Extract sanitization to core utility.
The _sanitizeIdSegment implementation duplicates sanitizeSegment from packages/core/src/path-safety.ts (which is already exported for exactly this purpose). Import and use the core function instead to avoid maintenance drift and ensure security updates propagate across the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/index.js` around lines 156 - 168, Replace the local _sanitizeIdSegment
implementation with the shared sanitizer from core: import and call
sanitizeSegment (from the core export) instead of duplicating logic; ensure you
preserve the existing behavior by passing the input and maxLen (default 64)
through to sanitizeSegment and keep the same fallback for null/undefined or
empty results (returning 'unknown') so callers of _sanitizeIdSegment continue to
behave the same while using the centralized sanitizeSegment implementation.
Owner
Author
|
Addressed the valid CodeRabbit items in fead969: removed local absolute paths from docs/checksums, removed the checksum-manifest self-entry, switched README setup to npm ci, fixed run IDs wording, and made the dogfood summarizer use fileURLToPath. I did not import the core TypeScript sanitizer into plugin/index.js because the plugin is a standalone JS package with its own package boundary; keeping the local sanitizer avoids coupling the plugin runtime to unbuilt TS core sources. |
dodge1218
force-pushed
the
contextclaw-openclaw-review-clean-2026-05-13
branch
from
4ed7fa4 to
8431722
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugin/classifier.js (1)
159-159: 💤 Low valueUnbounded
[ \t]*inconsistent with stated ReDoS hardening goal.The comment at lines 70-73 states "bounded quantifiers on every variable-length segment," but this regex starts with unbounded
[ \t]*. While the 5000-char input guard limits practical risk, bounding this quantifier would maintain consistency with the hardening approach.♻️ Proposed fix
- const configLines = lines.filter(l => /^[ \t]*"?[\w][\w.-]{0,128}"?[ \t]{0,4}[:=]/.test(l)).length; + const configLines = lines.filter(l => /^[ \t]{0,64}"?[\w][\w.-]{0,128}"?[ \t]{0,4}[:=]/.test(l)).length;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/classifier.js` at line 159, The regex used to count config-like lines in the configLines calculation uses an unbounded leading whitespace quantifier `[ \t]*`, which contradicts the stated ReDoS hardening of using bounded quantifiers; update the pattern in the configLines computation to replace the unbounded `[ \t]*` with a bounded form (e.g., `[ \t]{0,XXX}`) that reasonably limits leading whitespace while preserving intent, leaving the rest of the regex and the variable name configLines unchanged so the line-detection logic still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/MAINTAINER_TRIAGE_2026-05-12.md`:
- Line 5: Update the one-line verdict so it’s clearly historical: change the
existing line containing "Verdict: hold." to include a datestamped context
(e.g., "Status at 2026-05-13 — Verdict: hold (historical)") or add a top-note
above the file header that states "This verdict reflects status at 2026-05-13"
so maintainers don’t treat the line as a current instruction; modify the exact
string "Verdict: hold." in the document to reflect the date/context.
In `@dogfood-runs/2026-05-12-proceed-loop/README.md`:
- Around line 26-34: The README's step 6 claims to "Capture raw before/after
files" but the provided commands only take a single snapshot and write end.txt;
update the protocol to either (A) add explicit "before" capture commands that
copy ~/.openclaw/.contextclaw-stats.json,
~/.openclaw/.contextclaw-efficiency.json and
~/.openclaw/contextclaw/ledger.jsonl into /tmp/contextclaw-dogfood-private
before the run (so there are both before and after snapshots), or (B) reword the
step to "after-only capture" and clarify that the snippet records the post-run
snapshot and end.txt; reference the files .contextclaw-stats.json,
.contextclaw-efficiency.json, contextclaw/ledger.jsonl and the target directory
/tmp/contextclaw-dogfood-private when making the change.
---
Nitpick comments:
In `@plugin/classifier.js`:
- Line 159: The regex used to count config-like lines in the configLines
calculation uses an unbounded leading whitespace quantifier `[ \t]*`, which
contradicts the stated ReDoS hardening of using bounded quantifiers; update the
pattern in the configLines computation to replace the unbounded `[ \t]*` with a
bounded form (e.g., `[ \t]{0,XXX}`) that reasonably limits leading whitespace
while preserving intent, leaving the rest of the regex and the variable name
configLines unchanged so the line-detection logic still works.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4081e494-8451-4229-9041-e3f83190405c
⛔ Files ignored due to path filters (1)
plugin/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.github/workflows/ci.ymlREADME.mddocs/DOCTRINE_AGENT_LOOP_ECONOMICS.mddocs/FOUNDER_VISION_CONTEXTCLAW.mddocs/MAINTAINER_NOTE_DRAFT.mddocs/MAINTAINER_TRIAGE_2026-05-12.mddocs/MEASUREMENT.mddocs/NEXT_TICKETS_OPENCLAW_PUSH.mddocs/openclaw-dogfood-2026-05-12.mddogfood-runs/2026-05-12-proceed-loop/README.mddogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.txtdogfood-runs/2026-05-12-proceed-loop/dogfood-delta-summary.jsondogfood-runs/2026-05-12-proceed-loop/end.txtdogfood-runs/2026-05-12-proceed-loop/ledger-delta-redacted.jsonldogfood-runs/2026-05-12-proceed-loop/start.txtdogfood-runs/2026-05-12-proceed-loop/summarize.mjsdogfood-runs/2026-05-12-proceed-loop/summary.mdpackage.jsonpackages/core/src/__tests__/security.test.tspackages/core/src/memory.tspackages/core/src/path-safety.tspackages/core/src/watcher.tsplugin/__tests__/security.test.jsplugin/classifier.jsplugin/index.jsplugin/package.jsonplugin/policy.js
✅ Files skipped from review due to trivial changes (10)
- dogfood-runs/2026-05-12-proceed-loop/end.txt
- dogfood-runs/2026-05-12-proceed-loop/start.txt
- dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.txt
- package.json
- dogfood-runs/2026-05-12-proceed-loop/dogfood-delta-summary.json
- dogfood-runs/2026-05-12-proceed-loop/summary.md
- docs/MAINTAINER_NOTE_DRAFT.md
- dogfood-runs/2026-05-12-proceed-loop/ledger-delta-redacted.jsonl
- .github/workflows/ci.yml
- dogfood-runs/2026-05-12-proceed-loop/summarize.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/memory.ts
- plugin/package.json
- plugin/policy.js
- plugin/tests/security.test.js
- packages/core/src/watcher.ts
- packages/core/src/tests/security.test.ts
- plugin/index.js
|
|
||
| Date: 2026-05-12 | ||
| Updated: 2026-05-13 | ||
| Verdict: hold. Do not submit or push for maintainer review yet. |
There was a problem hiding this comment.
Clarify that this “hold” verdict is historical context.
Line 5 reads as current instruction, but this PR is already open. Please mark this as “status at 2026-05-13” (or add a top-note) to avoid process confusion for maintainers reading the packet now.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/MAINTAINER_TRIAGE_2026-05-12.md` at line 5, Update the one-line verdict
so it’s clearly historical: change the existing line containing "Verdict: hold."
to include a datestamped context (e.g., "Status at 2026-05-13 — Verdict: hold
(historical)") or add a top-note above the file header that states "This verdict
reflects status at 2026-05-13" so maintainers don’t treat the line as a current
instruction; modify the exact string "Verdict: hold." in the document to reflect
the date/context.
Comment on lines
+26
to
+34
| 6. Capture raw before/after files in a private scratch directory outside the repo. | ||
|
|
||
| ```bash | ||
| mkdir -p /tmp/contextclaw-dogfood-private | ||
| cp ~/.openclaw/.contextclaw-stats.json /tmp/contextclaw-dogfood-private/ | ||
| cp ~/.openclaw/.contextclaw-efficiency.json /tmp/contextclaw-dogfood-private/ | ||
| cp ~/.openclaw/contextclaw/ledger.jsonl /tmp/contextclaw-dogfood-private/ | ||
| date -Is > /tmp/contextclaw-dogfood-private/end.txt | ||
| ``` |
There was a problem hiding this comment.
Align protocol wording with the capture commands.
Line 26 says “before/after,” but the snippet only captures one snapshot plus end.txt. Add explicit “before” capture steps (or reword to “after-only capture”) so the run protocol is reproducible as written.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dogfood-runs/2026-05-12-proceed-loop/README.md` around lines 26 - 34, The
README's step 6 claims to "Capture raw before/after files" but the provided
commands only take a single snapshot and write end.txt; update the protocol to
either (A) add explicit "before" capture commands that copy
~/.openclaw/.contextclaw-stats.json, ~/.openclaw/.contextclaw-efficiency.json
and ~/.openclaw/contextclaw/ledger.jsonl into /tmp/contextclaw-dogfood-private
before the run (so there are both before and after snapshots), or (B) reword the
step to "after-only capture" and clarify that the snippet records the post-run
snapshot and end.txt; reference the files .contextclaw-stats.json,
.contextclaw-efficiency.json, contextclaw/ledger.jsonl and the target directory
/tmp/contextclaw-dogfood-private when making the change.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Maintainer Note Draft
Subject: Request for feedback on an OpenClaw
contextEngineguardrail pluginHi OpenClaw maintainers,
I built a small OpenClaw-first context/spend guardrail plugin and would value feedback on the plugin shape before trying to make a larger claim.
The narrow ask:
What ContextClaw does today:
What I am not claiming:
Current evidence:
proceedworkflow after enabling ContextClaw as thecontextEngineand restarting the gateway.Those are estimate/receipt metrics, not provider-billed measurements.
Pointers:
README.mddocs/openclaw-dogfood-2026-05-12.mddocs/MEASUREMENT.mddocs/MAINTAINER_TRIAGE_2026-05-12.mdThe question I would most like answered is whether this belongs as a
contextEngineplugin, or whether OpenClaw should expose a different preflight/context assembly surface for this class of guardrail.Summary by CodeRabbit
New Features
Documentation
Tests
Chores