Prepare OpenClaw contextEngine review packet#6
Conversation
📝 WalkthroughWalkthroughAdds MissionLedger and CLI, path-safety/DoS guards, a plugin request ledger with budget/premium gates and llm_output receipts, autocompaction policy and classifier security fixes, extensive PRDs/docs, dogfood evidence, a Claude Code watcher, prototypes, and verification scripts. ChangesCore control-plane scaffold (TypeScript)
Plugin: request ledger, budget gates, receipts, policy, and tests
Evidence, Prototypes, and Scripts
Docs, Standards, Templates, and CI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Superseding this PR with a cleaner branch based directly on origin/master. This branch was created from a local master that was ahead of GitHub master, so the public diff is larger than intended. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/MAINTAINER_PROPOSAL.md (1)
64-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEvidence epoch should be explicitly marked as historical.
Given the packet’s primary May 12 evidence framing, this April 15 block should be clearly labeled “historical baseline” (or moved to an appendix) to prevent mixed-signal interpretation.
🤖 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_PROPOSAL.md` around lines 64 - 75, Label the April 15, 2026 efficiency ledger block in MAINTAINER_PROPOSAL.md as historical by adding an explicit heading or badge such as "Historical baseline (April 15, 2026)" immediately above the paragraph that begins "The current efficiency ledger records production dogfood data..." or move that entire block into an appendix section; ensure the May 12 evidence framing remains the primary, time-stamped section and update any internal references to point to the new "historical baseline" label so readers cannot conflate the two.packages/core/src/watcher.ts (1)
141-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDeadline check granularity — a single oversized JSONL line can block the event loop past
parseTimeoutMs.The deadline is only inspected between lines yielded from
streamJsonl. Sincereadline.createInterface()has no per-line size cap andJSON.parse(line)is synchronous, a hostile or corrupt session file with one extremely long line will block the event loop during parsing — the timeout fires only after parsing finishes, defeating the DoS intent.Options to consider:
- Enforce a per-line byte cap in
streamJsonl(e.g., reject lines > a few MB) to prevent a single line from dominating the event loop.- Or guard via
setTimeout+ anAbortControllerpassed into the stream so the underlyingfsread is torn down when the deadline is exceeded.Not strictly a blocker given session files are normally well-formed, but worth addressing since the surrounding code frames this as a DoS guard.
🤖 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 `@packages/core/src/watcher.ts` around lines 141 - 152, The deadline check is too coarse — a single huge JSONL line can block parsing past parseTimeoutMs; update streamJsonl to enforce a per-line byte cap (e.g., MAX_LINE_BYTES ~ a few MB) and immediately reject/throw if a line exceeds that cap so large lines are never passed to the synchronous JSON.parse in _entryToTurn, and keep the existing deadline logic in the caller (where parseTimeoutMs / DEFAULT_PARSE_TIMEOUT_MS and the loop over streamJsonl(filePath) live); alternatively add an AbortController tied to a setTimeout deadline that cancels the underlying fs read in streamJsonl, but prefer the per-line cap as the simpler fix.
🟠 Major comments (21)
dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.before-1-4 (1)
1-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse repo-relative paths in checksum manifests.
Lines 1–4 expose local absolute paths (
/home/yin/...), which is avoidable metadata leakage and makes portability/check verification harder across environments.🤖 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 checksum manifest SHA256SUMS.before currently records absolute local paths (e.g., entries containing “/home/yin/...”) which leaks environment-specific metadata; update the manifest entries in SHA256SUMS.before to use repo-relative paths (for example replace the absolute paths for efficiency-before.json, ledger-before.jsonl, start.txt, stats-before.json with their relative paths under dogfood-runs/2026-05-12-proceed-loop/) so checksums remain portable and verifiable across environments.dogfood-runs/2026-05-12-proceed-loop/README.md-29-30 (1)
29-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExclude the checksum manifest from its own hash set.
Line 29 can include
SHA256SUMS.afterin*after*, which makes the manifest self-referential and breaks evidence integrity. Hash only explicit artifact files and then write the manifest.Proposed fix
-sha256sum dogfood-runs/2026-05-12-proceed-loop/*after* > dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.after +sha256sum \ + dogfood-runs/2026-05-12-proceed-loop/stats-after.json \ + dogfood-runs/2026-05-12-proceed-loop/efficiency-after.json \ + dogfood-runs/2026-05-12-proceed-loop/ledger-after.jsonl \ + > dogfood-runs/2026-05-12-proceed-loop/SHA256SUMS.after🤖 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 29 - 30, The checksum command currently includes the manifest itself (SHA256SUMS.after) via the *after* glob, creating a self-referential hash; change the step that runs sha256sum over the dogfood-runs/*after* set so it excludes SHA256SUMS.after (e.g., by filtering the glob results, using find with ! -name 'SHA256SUMS.after', or looping and skipping that filename) and then write the manifest only after computing hashes for the other artifact files.dogfood-runs/2026-05-12-proceed-loop/summarize.mjs-5-5 (1)
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
fileURLToPath(import.meta.url)for filesystem-safe path resolution.Line 5 uses
.pathnameto get a filesystem path from a file URL. On Windows, this produces an invalid path with a leading slash (e.g.,/C:/Users/...), and percent-encoded characters remain unencoded. Use Node'sfileURLToPath()helper instead.Proposed fix
import fs from 'node:fs'; 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));🤖 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 current extraction of a filesystem path uses path.dirname(new URL(import.meta.url).pathname) which can yield invalid/percent-encoded paths on Windows; replace this with Node's fileURLToPath utility: call fileURLToPath(import.meta.url) and pass that into path.dirname (i.e., compute root using path.dirname(fileURLToPath(import.meta.url))). Import or reference fileURLToPath from 'url' alongside existing path usage so root is filesystem-safe.dogfood-runs/2026-05-12-proceed-loop/summary.md-9-18 (1)
9-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix inconsistent truncation counts in the same evidence summary.
Truncated itemsis769in the delta table, but749in the new-entry bullets. This makes the packet internally inconsistent for maintainer review; please recompute from source artifacts and align both values.🤖 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/summary.md` around lines 9 - 18, The "Truncated items" count is inconsistent between the delta table value (769) and the "New Ledger Entries" bullets (749); recompute the truncation count from the source artifacts and update both occurrences so they match, ensuring the delta table row labeled "Truncated items" and the bullet line "Truncated items: 749" reflect the same recomputed value and any dependent totals (e.g., "Estimated input tokens" / "Compression chars saved") are adjusted if affected.outputs/pick-back-up-contextclaw-email-2026-04-29.md-26-30 (1)
26-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace user-specific absolute path with repo-relative instructions.
The hard-coded
/home/yin/...path is non-portable and leaks local environment details. Use repo-relative steps (e.g., “from repository root, run …”) so others can execute safely.🤖 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 `@outputs/pick-back-up-contextclaw-email-2026-04-29.md` around lines 26 - 30, The README snippet uses a hard-coded absolute path (/home/yin/.openclaw/workspace/contextclaw) which is non-portable; change the instructions to be repo-relative by removing the absolute cd and instead state “from the repository root, run” followed by the commands shown (npm run ledger, npm run build, npx vitest run packages/core/src/__tests__/mission-ledger.test.ts) or use a relative cd like cd . or cd contextclaw if the repo root folder name is relevant—ensure the text no longer contains any user-specific absolute paths.notes/claude-code-article-next-steps.md-9-23 (1)
9-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the
$553claim to match current evidence artifacts.This note hard-codes a public-facing savings number that appears out of sync with the attached 2026-05-12 dogfood packet metrics. Please replace with a source-backed figure (or explicitly label it as historical/outdated) before outreach/article use.
🤖 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 `@notes/claude-code-article-next-steps.md` around lines 9 - 23, The article hook line that reads “I saved $553 by making my AI agent show me what context it was wasting.” is hard-coded and out of sync with current metrics; update that claim in the document so it either (a) uses a source-backed figure pulled from the Claude Code receipts/metrics (the savings value present in the claude-code savings ledger and the 2026-05-12 dogfood packet) or (b) is explicitly labeled as historical/outdated (e.g., “historical: $553”) until the read-only sidecar watcher (per EQUIP_PLAN) has run on ≥3 sessions and populated the claude-code-savings-ledger.jsonl and weekly summary; ensure the new wording references the ledger/summary for verification and do not leave an unsupported hard number in the Article hook.packages/core/src/cli.ts-3-6 (1)
3-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate the ledger directory before appending receipt entries.
appendFileSyncfails withENOENTwhen~/.openclaw/contextclaw/doesn’t exist yet.Proposed fix
-import { appendFileSync, existsSync, readFileSync } from 'node:fs'; +import { appendFileSync, existsSync, mkdirSync, readFileSync } from 'node:fs'; import { homedir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; @@ function appendRequestLedgerEntry(path: string, entry: RequestLedgerEntry & Record<string, unknown>) { + mkdirSync(dirname(path), { recursive: true }); appendFileSync(path, `${JSON.stringify(entry)}\n`); }Also applies to: 235-237
🤖 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 `@packages/core/src/cli.ts` around lines 3 - 6, Before calling appendFileSync to write receipts, ensure the ledger directory exists by creating it if missing; locate the code that constructs the ledger path using homedir()/join and the calls to appendFileSync (referenced as appendFileSync and the ledger path variables) and insert a check that creates the parent directory (using fs.mkdirSync or fs.promises.mkdir with { recursive: true }) when it does not exist so appendFileSync no longer throws ENOENT; apply the same change around the other appendFileSync usages mentioned (lines ~235-237).package.json-11-13 (1)
11-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
inspectscript with the new build output path.Line 11 builds from
packages/core/tsconfig.json, but Line 13 still runsnode dist/inspector/server.js. This likely breaksnpm run inspectbecause artifacts are now underpackages/core/dist.Proposed fix
- "inspect": "node dist/inspector/server.js", + "inspect": "node packages/core/dist/inspector/server.js",🤖 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 `@package.json` around lines 11 - 13, The "inspect" npm script currently points to dist/inspector/server.js but after changing the build target the compiled artifacts live under packages/core/dist; update the "inspect" script in package.json (the "inspect" entry) to run node packages/core/dist/inspector/server.js (or otherwise reference the new compiled path), ensuring it aligns with the "build" output from the "build" script that uses packages/core/tsconfig.json.packages/core/src/mission-ledger.ts-192-195 (1)
192-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent cross-mission artifact injection in
planPass.When
artifactIdsis an explicit list, artifacts from other missions are accepted. That breaks mission isolation.Proposed fix
const artifacts = input.artifactIds === 'all' ? [...this.artifacts.values()].filter((artifact) => artifact.missionId === input.missionId) - : input.artifactIds.map((id) => this.mustArtifact(id)); + : input.artifactIds.map((id) => { + const artifact = this.mustArtifact(id); + if (artifact.missionId !== input.missionId) { + throw new Error(`Artifact ${id} does not belong to mission ${input.missionId}`); + } + return artifact; + });🤖 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 `@packages/core/src/mission-ledger.ts` around lines 192 - 195, In planPass, when input.artifactIds is an explicit list, validate each resolved artifact from this.mustArtifact(id) belongs to the same mission as input.missionId (compare artifact.missionId) and reject the request (throw an error) if any artifact is from a different mission to prevent cross-mission injection; alternatively, filter out mismatched artifacts and surface an error describing the offending artifact IDs so mission isolation is preserved.packages/core/src/mission-ledger.ts-153-167 (1)
153-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate mission IDs in
createMission.Currently, creating the same mission ID twice silently overwrites the existing mission state.
Proposed fix
createMission(input: { id: string; objective: string; budget: number; acceptanceCriteria?: string; sticker?: string; premiumUnitBudget?: number; unitCostBasis?: UnitCostBasis }): Mission { + if (this.missions.has(input.id)) { + throw new Error(`Mission already exists: ${input.id}`); + } const mission: Mission = {🤖 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 `@packages/core/src/mission-ledger.ts` around lines 153 - 167, createMission currently overwrites existing missions with the same id; update createMission to first check this.missions.has(input.id) and reject duplicates by throwing an Error (or returning a failure) instead of proceeding, so the existing Mission in the missions Map is preserved; ensure the duplicate-check happens before calling this.missions.set(...) and reference the createMission method, the missions Map and the Mission object in your change.packages/core/src/cli.ts-168-175 (1)
168-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake request-ledger parsing resilient to malformed JSONL rows.
A single malformed line currently throws in
JSON.parseand breaks all ledger read commands.Proposed fix
function readRequestLedger(path = requestLedgerPath()): RequestLedgerEntry[] { if (!existsSync(path)) return []; - return readFileSync(path, 'utf8') + return readFileSync(path, 'utf8') .trim() .split('\n') .filter(Boolean) - .map((line) => JSON.parse(line) as RequestLedgerEntry); + .flatMap((line, idx) => { + try { + return [JSON.parse(line) as RequestLedgerEntry]; + } catch { + console.warn(`Skipping malformed ledger row ${idx + 1}`); + return []; + } + }); }🤖 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 `@packages/core/src/cli.ts` around lines 168 - 175, The readRequestLedger function currently throws if any JSONL line is malformed; update readRequestLedger to parse each line defensively by iterating lines and wrapping JSON.parse in a try/catch (or using JSON.parse inside a safe helper) so malformed rows are skipped (or optionally logged) while valid RequestLedgerEntry objects are collected and returned; keep the existsSync early-return behavior and ensure the function still returns RequestLedgerEntry[] of only successfully parsed entries.packages/core/src/cli.ts-147-162 (1)
147-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle invalid
--sincevalues without crashing.Line 161 can throw on invalid date input (
toISOString()onInvalid Date), which terminatesledger-summary/ledger-session.Proposed fix
function parseSinceFlag(): string | undefined { @@ - return new Date(since).toISOString(); + const parsed = new Date(since); + if (Number.isNaN(parsed.getTime())) { + console.error(`Invalid --since value: ${since}`); + process.exit(1); + } + return parsed.toISOString(); }🤖 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 `@packages/core/src/cli.ts` around lines 147 - 162, The parseSinceFlag function can call toISOString() on an invalid Date and crash; update parseSinceFlag (which uses parseStringFlag and handles --today) to validate Dates before calling toISOString(): after creating the Date (both for the parsed relative ms branch and the fallback new Date(since) branch) check isNaN(date.getTime()) and if invalid return undefined (or a safe fallback) instead of calling toISOString(); ensure all return paths that previously returned strings now only return validated ISO strings or undefined.plugin/autocompaction-policy.js-137-156 (1)
137-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
turnin labels so age-based policy works.
planCompactionActions()depends onlabel.turn, butlabelContextItem()doesn’t carry it forward. As a result,ageis typically undefined and bulky items are prematurely summarized. Includeturn(and/or compute age from timestamps) in the label payload.Also applies to: 225-244
🤖 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/autocompaction-policy.js` around lines 137 - 156, The label object built in labelContextItem() is missing the originating turn, causing planCompactionActions() to see undefined age; add a turn property (e.g., turn: item?.turn || state.turn || inferred.turn) to the label payload so age-based policies work, and mirror the same change where labels are constructed in the other block (lines ~225-244) so bulky items retain their turn metadata (or compute age from timestamps if preferred).plugin/index.js-594-595 (1)
594-595:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove sync config file reads from
assemble()hot path.Fallback model resolution currently triggers synchronous disk I/O in request assembly flow. Cache the resolved primary model (or refresh on config reload) instead of reading/parsing
openclaw.jsonper call.🤖 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 594 - 595, The assemble() hot path is performing synchronous disk I/O via resolveConfiguredPrimaryModel() when this.config.activeModel is unset; change this to use a cached primary model value instead of calling resolveConfiguredPrimaryModel() on each assemble() call—store the resolved model in a module or instance-level cache (e.g., this._cachedPrimaryModel) and return that in assemble() (use this.config.activeModel || this._cachedPrimaryModel); add logic to refresh/clear the cache when configuration reload occurs (where config is updated) so the cached value is recomputed then, and ensure resolveConfiguredPrimaryModel() is only called during init/config-reload paths, not inside assemble().plugin/index.js-439-440 (1)
439-440:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCap and evict latest-estimate maps to avoid memory growth.
_latestEstimateByRunand_latestEstimateBySessionare append-only. In long sessions this becomes unbounded. Add eviction (size cap/TTL) and delete entries after successful receipt correlation.Also applies to: 619-620
plugin/ledger.js-318-320 (1)
318-320:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid synchronous disk writes on the request path.
appendFileSyncruns for each ledger event and can block the event loop during assemble/receipt flow. Move to buffered async writes (ordered queue) to keep request latency stable.🤖 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/ledger.js` around lines 318 - 320, The append method currently performs synchronous disk I/O (mkdirSync and appendFileSync) which blocks the event loop; change Appendix to perform non-blocking, ordered async writes: replace mkdirSync/appendFileSync usage in append(entry) with an async enqueue mechanism that (1) ensures the directory exists using an async call (fs.promises.mkdir) and (2) writes entries via an internal FIFO write queue/task runner that calls fs.promises.appendFile in order for this.path; implement a small in-process buffer/queue (e.g., an array and a running promise loop) tied to the append method to serialize writes and handle errors without blocking the request path.plugin/index.js-815-818 (1)
815-818:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubagent spend is being double-counted.
This accumulator already includes estimated subagent spend during
assemble, then adds actual receipt cost again here. That inflates status totals. Keep estimated and actual in separate counters, or replace estimate with actual for matched receipts.🤖 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 815 - 818, The code is double-counting subagent spend by adding actual receipt cost into stats.totalEstimatedSubagentSpendUsd (which was set during assemble); instead, stop adding actualCostUsd to totalEstimatedSubagentSpendUsd and instead increment a separate counter like stats.totalActualSubagentSpendUsd when receipt.sessionKind === 'subagent'. Update the block that checks receipt?.actualCostUsd to: (1) leave stats.totalEstimatedSubagentSpendUsd unchanged, and (2) add (stats.totalActualSubagentSpendUsd = (stats.totalActualSubagentSpendUsd || 0) + receipt.actualCostUsd). Also review the assemble logic that populates estimated subagent spend to ensure it still writes only estimated values and not actuals.plugin/ledger.js-170-171 (1)
170-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound in-memory prompt tracking to prevent unbounded growth.
RequestLedger.stateonly grows and is never pruned. Long-running sessions will accumulate prompt IDs indefinitely and can degrade memory over time. Add a size cap/TTL and evict oldest entries.Also applies to: 190-194
🤖 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/ledger.js` around lines 170 - 171, RequestLedger.state is an unbounded Map causing memory growth; modify RequestLedger (constructor and the methods that add entries—e.g., the functions around lines 190-194 that insert into state) to enforce a max size and/or TTL: store timestamps with values or maintain insertion order in the Map, and on each insert check if size > MAX_ENTRIES (or remove entries older than TTL) and evict oldest entries using Map.keys().next().value (or delete by timestamp) so the Map never grows unbounded. Make MAX_ENTRIES and TTL configurable constants and ensure eviction logic runs in the same methods that currently call this.state.set(...).plugin/policy.js-22-23 (1)
22-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden marker verification to reject malformed marker bodies.
verifyTruncationMarkercurrently accepts any bracketedContextClaw-looking token and then slices payload even when the expected(Run cc_rehydratesuffix is missing (Line 66 path). That makes malformed markers enter HMAC validation instead of being rejected up front.🔧 Proposed fix
-const MARKER_REGEX = /\[ContextClaw:([0-9a-f]{8,16})[ \t][^\]]{1,256}\]/; +const MARKER_REGEX = /\[ContextClaw:([0-9a-f]{8}) ([^\]]{1,256}) \(Run cc_rehydrate\("([0-9a-f]{8})"\) to read full\)\]/; export function verifyTruncationMarker(text) { if (typeof text !== 'string') return false; const m = text.match(MARKER_REGEX); if (!m) return false; - const tag = m[1].slice(0, 8); - // Extract the message payload between the tag and ` (Run cc_rehydrate` - const inner = m[0].slice(`[ContextClaw:${tag} `.length, m[0].lastIndexOf(' (Run cc_rehydrate')); - if (!inner) return false; + const tag = m[1]; + const inner = m[2]; + const rehydrateTag = m[3]; + if (!inner || rehydrateTag !== tag) return false; const expected = createHmac('sha256', SESSION_KEY) .update(inner) .digest('hex') .slice(0, 8); return tag === expected; }Also applies to: 60-67
🤖 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/policy.js` around lines 22 - 23, The MARKER_REGEX and verifyTruncationMarker are too permissive and allow bracketed ContextClaw-like tokens without the required " (Run cc_rehydrate" suffix, causing malformed markers to reach HMAC validation; update the logic in verifyTruncationMarker (and tighten MARKER_REGEX) so it requires the full expected suffix before slicing payload—explicitly check for the " (Run cc_rehydrate" substring immediately after the marker body and return a rejection/error if it is missing, ensuring only well-formed markers proceed to HMAC verification (refer to MARKER_REGEX and verifyTruncationMarker).scripts/verify-friendly-ledger.sh-3-7 (1)
3-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a per-run temp file instead of a fixed
/tmppath.Line 3 writes ledger output to a predictable filename, which risks cross-run collisions and data exposure on shared hosts. Use
mktempand cleanup on exit.🔧 Proposed hardening
npm run ledger > /tmp/contextclaw-friendly-ledger.out -grep 'No model was called' /tmp/contextclaw-friendly-ledger.out -grep 'Allowed pass:' /tmp/contextclaw-friendly-ledger.out -grep 'Blocked pass:' /tmp/contextclaw-friendly-ledger.out -cat /tmp/contextclaw-friendly-ledger.out +OUT_FILE="$(mktemp /tmp/contextclaw-friendly-ledger.XXXXXX.out)" +trap 'rm -f "$OUT_FILE"' EXIT +npm run ledger > "$OUT_FILE" +grep 'No model was called' "$OUT_FILE" +grep 'Allowed pass:' "$OUT_FILE" +grep 'Blocked pass:' "$OUT_FILE" +cat "$OUT_FILE"🤖 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 `@scripts/verify-friendly-ledger.sh` around lines 3 - 7, Replace the fixed /tmp/contextclaw-friendly-ledger.out with a per-run temporary file created via mktemp in the scripts/verify-friendly-ledger.sh script, store its path in a variable (e.g., LEDGER_OUT), update all uses of that fixed path (the npm run ledger redirection and the three grep and cat commands) to reference the variable, and add a trap to remove the temp file on exit or error to ensure cleanup.scripts/verify-premium-units.sh-7-7 (1)
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClarify grep failure behavior.
When
grepfinds no matches, it returns exit code 1, which will cause the script to fail due toset -e. If this is intentional verification (ensuringestimatedPremiumUnitsexists in the output), consider making it explicit. If grep failure is acceptable, handle it explicitly to avoid confusing script failures.💡 Proposed fixes for different intentions
If you want to verify the field exists and fail if missing:
-grep 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out | head +grep 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out | head || { + echo "ERROR: estimatedPremiumUnits not found in output" >&2 + exit 1 +}If grep failure is acceptable (field might not exist):
-grep 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out | head +grep 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out | head || trueIf you want to validate the value exists and exit with clear success:
-grep 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out | head +if grep -q 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out; then + echo "✓ estimatedPremiumUnits found in output:" + grep 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out | head +else + echo "✗ estimatedPremiumUnits not found in output" >&2 + exit 1 +fi🤖 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 `@scripts/verify-premium-units.sh` at line 7, The grep invocation "grep 'estimatedPremiumUnits' /tmp/contextclaw-premium-units.out | head" can return exit code 1 when no match and cause the script to fail under set -e; decide the intended behavior and make it explicit: if the script should fail when the field is missing, change the check to run grep in quiet mode and explicitly test its exit status and print a clear error and exit (referencing the grep command line and the file /tmp/contextclaw-premium-units.out in scripts/verify-premium-units.sh); if missing matches are acceptable, suppress grep's non-zero exit by handling its return (e.g., allow the pipeline to succeed or add an explicit fallback/notice) so that the script does not abort unexpectedly.
🟡 Minor comments (7)
.github/pull_request_template.md-16-19 (1)
16-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit core test checkbox for core changes.
Line 16-19 omits a direct core test command, so core-touching PRs can be “verified” without running the core suite locally.
Suggested update
## Verification - [ ] `npm run test:plugin` +- [ ] `cd packages/core && npx vitest run` if core/package code changed - [ ] `npm run build` if core/package code changed - [ ] Demo/manual check if UI or docs changed🤖 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 @.github/pull_request_template.md around lines 16 - 19, Add an explicit core test checkbox under the "## Verification" section so PRs that touch core must run the core test suite; update the checklist that currently contains `npm run test:plugin` and `npm run build` to include a new line `- [ ] npm run test:core` (or the project's canonical core test command) ensuring the `## Verification` block forces core-suite verification for core changes.WORK-IN-PROGRESS.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate timestamp appears stale for this review packet.
Please refresh the
Updatedvalue to the actual document update date so triage context remains trustworthy.🤖 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 `@WORK-IN-PROGRESS.md` at line 3, The document's "Updated" timestamp is stale; replace the current "Updated: 2026-04-30 22:42 ET" line with the actual date/time of your latest edit (e.g., "Updated: 2026-05-13 HH:MM TZ") so the `Updated` field accurately reflects the document's current modification time and preserves triage trustworthiness.WORK-IN-PROGRESS.md-49-56 (1)
49-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCLI command shape is inconsistent with the control-plane PRD.
This section uses
cc ledger-tailstyle, while the PRD documentscc ledger tailstyle. Pick one canonical syntax across docs to avoid operator confusion.🤖 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 `@WORK-IN-PROGRESS.md` around lines 49 - 56, The CLI examples use the hyphenated form (e.g., `cc ledger-tail`, `cc ledger-summary`, `cc ledger-session`) which conflicts with the PRD's space-separated subcommand style; update these examples to the canonical form `cc ledger <subcommand>` throughout WORK-IN-PROGRESS.md (e.g., `cc ledger tail`, `cc ledger summary`, `cc ledger summary --today`, `cc ledger session <sessionKey>`, `cc ledger subagents <parentSessionKey>`, `cc ledger explain <entryId>`, `cc ledger receipt <entryId> --tokens-in N --tokens-out N --source manual`) and ensure any other occurrences of the hyphenated forms in the file are changed to the same space-separated pattern so docs are consistent with the control-plane PRD.README.md-164-168 (1)
164-168:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid hard-coded test counts in the main README.
These fixed counts will drift and can become misleading; prefer dated snapshots or a link to reproducible CI/local command output.
Suggested doc tweak
-Current local result from this checkout: - -- core tests: 60 passed -- plugin tests: 78 passed -- whitespace diff check: clean +Local verification (run in this checkout): + +- `npm test` +- `npm run test:plugin` +- `git diff --check` + +For current pass/fail counts, use CI or command output at review time.🤖 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 164 - 168, Replace the hard-coded test counts under the "Current local result from this checkout:" section with a non-drifting alternative: either remove the bullet list and add a dated snapshot note (e.g., "As of YYYY-MM-DD, local run produced ...") or replace it with a link/instruction to reproduce the results (CI badge or command to run tests) so the README no longer contains fixed counts; update the surrounding text in the README.md to reference the snapshot date or the CI/build results instead of the literal numbers.scripts/verify-mission-ledger-review-formats.sh-4-4 (1)
4-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winActually validate JSON format instead of only previewing output.
Line 4 truncates output for display but does not verify it’s valid JSON, so format regressions can slip through.
🧪 Suggested verification step
-node packages/core/dist/cli.js mission-review --load /tmp/contextclaw-ledger-demo.json --format json | sed -n '1,30p' +OUT_JSON="$(mktemp /tmp/contextclaw-mission-review.XXXXXX.json)" +trap 'rm -f "$OUT_JSON"' EXIT +node packages/core/dist/cli.js mission-review --load /tmp/contextclaw-ledger-demo.json --format json >"$OUT_JSON" +node -e "const fs=require('node:fs'); JSON.parse(fs.readFileSync(process.argv[1],'utf8'))" "$OUT_JSON" +sed -n '1,30p' "$OUT_JSON"🤖 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 `@scripts/verify-mission-ledger-review-formats.sh` at line 4, The script currently runs the mission-review command and pipes it to "sed -n '1,30p'" which only previews output; replace the sed preview with a real JSON validation step: run the same command and pipe its stdout into a JSON validator (e.g., "jq -e ." or "python -m json.tool" or "node -e 'JSON.parse(require(\"fs\").readFileSync(0,\"utf8\"))'") and ensure the command exits non‑zero on invalid JSON so the CI detects regressions; update the invocation of "node packages/core/dist/cli.js mission-review --load /tmp/contextclaw-ledger-demo.json --format json" accordingly and remove the sed preview.scripts/verify-mission-ledger-persistence.sh-10-10 (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert non-empty persisted ledger sections.
Line 10 only prints counts; it doesn’t fail when persistence silently produces empty arrays.
✅ Suggested assertion
-node -e "const s=require('/tmp/contextclaw-ledger-demo.json'); console.log(s.missions.length, s.artifacts.length, s.passes.length)" +node -e "const s=require('/tmp/contextclaw-ledger-demo.json'); if(!s.missions.length||!s.artifacts.length||!s.passes.length) process.exit(1); console.log(s.missions.length, s.artifacts.length, s.passes.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 `@scripts/verify-mission-ledger-persistence.sh` at line 10, The script currently only prints counts from /tmp/contextclaw-ledger-demo.json (the node -e line reading s.missions.length, s.artifacts.length, s.passes.length) but doesn't fail if any of those arrays are empty; update that inline Node check to assert each length > 0 and if any is zero print a clear error (mention which of missions, artifacts, passes is empty) and exit with non-zero status (process.exit(1)), otherwise exit 0 or print success—modify the node -e invocation to perform these checks and proper exit codes.scripts/verify-premium-units.sh-5-5 (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBuild log is created but never used.
The output is redirected to
/tmp/cc_build.log, but the script never examines or references this file. If the log isn't needed for verification, remove the redirection; if it is needed, add a step to check it or print relevant sections.🧹 Proposed fix to remove unused redirection
-npm run build >/tmp/cc_build.log +npm run buildAlternatively, if you want to suppress build output but keep error visibility:
-npm run build >/tmp/cc_build.log +npm run build >/dev/null🤖 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 `@scripts/verify-premium-units.sh` at line 5, The build output is being redirected to /tmp/cc_build.log by the line "npm run build >/tmp/cc_build.log" in scripts/verify-premium-units.sh but that file is never used; either remove the redirection so the build runs as "npm run build" (so CI sees failures and logs), or keep the redirection but add a follow-up check that inspects the log and fails/prints relevant sections (e.g., test for non-zero exit and on failure cat /tmp/cc_build.log or tail it) so build errors are surfaced; update the script around the "npm run build >/tmp/cc_build.log" invocation accordingly.
🧹 Nitpick comments (9)
docs/MVP_REVIEW_FEED_DEMO.md (1)
1-31: 💤 Low valueConsider distinguishing the two demo snapshots with unique headings.
Both sections use identical heading text, which may confuse readers. While the context (different pass IDs and decisions) differentiates them, explicit labels like "Demo Snapshot 1: Blocked" and "Demo Snapshot 2: Allowed" would improve clarity.
📝 Suggested heading improvement
-## ContextClaw MVP before security research +## ContextClaw MVP before security research — Snapshot 1: Blocked Mission: `mis_contextclaw_mvp` | Sticker: `CC-MVP` | State: **waiting_approval** Pass: `pass_8db44d4241ff` | Role: `planner` | Model: `local/free` | Decision: **blocked**-## ContextClaw MVP before security research +## ContextClaw MVP before security research — Snapshot 2: Allowed Mission: `mis_contextclaw_mvp` | Sticker: `CC-MVP` | State: **waiting_approval** Pass: `pass_86b9a647303b` | Role: `planner` | Model: `local/free` | Decision: **allowed**🤖 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/MVP_REVIEW_FEED_DEMO.md` around lines 1 - 31, The two identical section headings "ContextClaw MVP before security research" should be made distinct to avoid confusion: rename the first to something like "Demo Snapshot — Blocked (pass_8db44d4241ff, Decision: blocked)" and the second to "Demo Snapshot — Allowed (pass_86b9a647303b, Decision: allowed)"; update any repeated subheadings or the top-line title in those blocks (including the "Next action" label if needed) so readers can immediately tell which snapshot relates to the blocked vs allowed pass without scanning pass IDs.docs/DOCTRINE_AGENT_LOOP_ECONOMICS.md (1)
10-13: ⚡ Quick winConsider replacing local filesystem paths in maintainer documentation.
The absolute local paths (e.g.,
/home/yin/.openclaw/workspace/memory/permanent/...) are exposed in documentation intended for external OpenClaw maintainers. This reads as internal lineage notes rather than polished documentation for an external audience.📝 Suggested revision
Replace with relative or conceptual references:
-This is not a new doctrine. It consolidates the existing ContextClaw scope-creep / future-work notes into one maintainer-readable frame: - -- `/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` -- `/home/yin/.openclaw/workspace/outputs/openclaw-recovery-todo-20260418.md` +This is not a new doctrine. It consolidates existing ContextClaw scope-creep and future-work notes from prior internal memory artifacts into one maintainer-readable frame.🤖 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/DOCTRINE_AGENT_LOOP_ECONOMICS.md` around lines 10 - 13, The document contains absolute local paths like `contextclaw-v2-vision.md`, `contextclaw-seatbelt-doctrine.md`, `contextclaw-spend-ledger-doctrine.md`, and `openclaw-recovery-todo-20260418.md`; replace these absolute filesystem references with relative links, repository-relative references, or human-friendly conceptual names (e.g., "permanent memory: contextclaw-v2-vision" or "./memory/permanent/contextclaw-v2-vision.md" or a short descriptive phrase) so the doc is suitable for external maintainers; update any inline lists or links to use the new format and confirm they resolve correctly in the published docs.packages/core/src/watcher.ts (1)
128-139: 💤 Low valueRedundant try/catch around
statSync— collapses to a no-op.Both branches of the
catchunconditionallythrow err, so theif ((err as NodeJS.ErrnoException)?.code)guard is dead and the wrapper accomplishes nothing beyond what would happen ifstatSyncwere called directly. The comment on Line 136 ("statSync errors … bubble up") is already true without the wrapper. Recommend simplifying so the intent is obvious and a future reader doesn't assume special handling is happening.♻️ Proposed simplification
- const limit = this.config.maxSessionBytes ?? DEFAULT_MAX_SESSION_BYTES; - const allowLarge = this.config.allowLargeSession === true; - try { - const st = statSync(filePath); - if (!allowLarge && st.size > limit) { - throw new Error( - `ContextClaw: refused session parse — size ${st.size} exceeds limit ${limit} (allowLargeSession=false)`, - ); - } - } catch (err) { - // statSync errors (ENOENT, EACCES) bubble up so callers see them. - if ((err as NodeJS.ErrnoException)?.code) throw err; - throw err; - } + const limit = this.config.maxSessionBytes ?? DEFAULT_MAX_SESSION_BYTES; + const allowLarge = this.config.allowLargeSession === true; + // statSync errors (ENOENT, EACCES) intentionally bubble up to the caller. + const st = statSync(filePath); + if (!allowLarge && st.size > limit) { + throw new Error( + `ContextClaw: refused session parse — size ${st.size} exceeds limit ${limit} (allowLargeSession=false)`, + ); + }🤖 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 `@packages/core/src/watcher.ts` around lines 128 - 139, The current try/catch around statSync in watcher.ts is redundant because both catch branches rethrow the error; remove the try/catch and call statSync(filePath) directly, then perform the size check using the returned st (same variable name) and throw the existing Error when !allowLarge && st.size > limit; keep the original error semantics so statSync errors (ENOENT, EACCES) naturally bubble up, preserving symbols: statSync, filePath, allowLarge, limit, and the thrown Error message.claude-code/contextclaw_claude_watcher.py (1)
129-134: 💤 Low valueConsider SHA-256 for fingerprinting consistency.
While SHA-1 is adequate for non-cryptographic deduplication fingerprinting within a session, using SHA-256 (already used elsewhere in the codebase for
short_hash) would provide consistency and avoid static analysis warnings.♻️ Optional refactor for consistency
def fingerprint(s: str) -> str: """Stable fingerprint used to detect repeated stack/lock dumps within a session.""" # Normalize whitespace and drop digits to merge near-identical traces. norm = re.sub(r"\s+", " ", s) norm = re.sub(r"\d+", "0", norm) - return hashlib.sha1(norm[:4000].encode("utf-8", errors="ignore")).hexdigest()[:16] + return hashlib.sha256(norm[:4000].encode("utf-8", errors="ignore")).hexdigest()[:16]🤖 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 `@claude-code/contextclaw_claude_watcher.py` around lines 129 - 134, The fingerprint function uses SHA-1; replace it with SHA-256 to match the project's short_hash usage and avoid warnings: in fingerprint(s: str) update the hashing call from hashlib.sha1(...) to hashlib.sha256(...), keep the same input normalization (norm[:4000].encode(...)) and still truncate the hex digest to the same length ([:16]) so callers remain unchanged.prototypes/contextclaw_mvp.py (2)
174-224: ⚖️ Poor tradeoffOptional: Consider standard formatting for CLI setup.
The CLI argument parser setup uses extensive semicolon-separated statements (lines 209-223). While functional, standard Python style recommends separate statements for better readability.
Given this is a prototype and the compact format aids at-a-glance comprehension of the command structure, reformatting is optional.
🤖 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 `@prototypes/contextclaw_mvp.py` around lines 174 - 224, The CLI setup uses dense semicolon-separated statements (e.g., the argparse block that creates p, sub, and repeated s=sub.add_parser(...) calls like for 'mission','artifact','pass', etc., and the pause/continue/kill loop), which hurts readability; refactor by expanding those semicolon-chained lines into one statement per line: create p = argparse.ArgumentParser(), sub = p.add_subparsers(...), then for each command call s = sub.add_parser(...), configure s.add_argument(...) on separate lines and call s.set_defaults(...) on its own line (do the same for the pause/continue/kill loop), keeping the same argument names and defaults so behavior (functions like cmd_mission, cmd_artifact, cmd_plan_pass, cmd_status, cmd_inspect, cmd_why, cmd_review_feed, cmd_dogfood, cmd_approve and the state-setters) remains unchanged.
22-26: ⚡ Quick winSQL f-string pattern is vulnerable to injection.
The
has_colandadd_colfunctions use f-strings to build SQL queries with thetableparameter. While currently only called with hardcoded table names frominit(), this pattern is inherently unsafe and could lead to SQL injection if the code evolves.🛡️ Recommended fix using allowlist validation
+ALLOWED_TABLES = {'missions', 'artifacts', 'passes', 'approvals'} + def has_col(c, table, col): + if table not in ALLOWED_TABLES: + raise ValueError(f"Invalid table name: {table}") return any(r['name'] == col for r in c.execute(f'pragma table_info({table})')) def add_col(c, table, col, ddl): + if table not in ALLOWED_TABLES: + raise ValueError(f"Invalid table name: {table}") if not has_col(c, table, col): c.execute(f'alter table {table} add column {ddl}')🤖 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 `@prototypes/contextclaw_mvp.py` around lines 22 - 26, The SQL f-strings in has_col and add_col are vulnerable because they interpolate table/col names directly; replace that by validating table and column identifiers before interpolating: in has_col and add_col (and calls from init) enforce either an explicit allowlist of allowed table names/columns or a strict identifier regex (e.g. /^[A-Za-z_][A-Za-z0-9_]*$/) and raise on mismatch, then only after validation use the (now-safe) formatted SQL for pragma table_info(...) and alter table ... add column ...; keep parameterized queries for any user data (not identifiers).scripts/verify-mission-ledger-approval.sh (1)
5-5: ⚡ Quick winCapture both stdout and stderr for build logs.
Line 5 currently captures stdout only; build errors often surface on stderr and won’t be preserved in
/tmp/cc_build.log.🛠️ Suggested change
-npm run build >/tmp/cc_build.log +npm run build >/tmp/cc_build.log 2>&1🤖 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 `@scripts/verify-mission-ledger-approval.sh` at line 5, The build command currently redirects only stdout ("npm run build >/tmp/cc_build.log"), so stderr is lost; change the redirection to capture both stdout and stderr (e.g., redirect stderr to stdout using "2>&1" or use a combined redirect like "&>") so that the full build log (both stdout and stderr) is written to /tmp/cc_build.log—update the line invoking npm run build accordingly in scripts/verify-mission-ledger-approval.sh.scripts/verify-premium-units.sh (2)
5-7: ⚡ Quick winConsider cleaning up temporary files.
The script creates files in
/tmpthat are not cleaned up. While/tmpis periodically cleared by the system, adding explicit cleanup improves hygiene, especially for verification scripts run repeatedly during development.🧼 Proposed cleanup pattern
Add a trap at the beginning of the script:
#!/usr/bin/env bash set -euo pipefail + +# Cleanup temp files on exit +trap 'rm -f /tmp/cc_build.log /tmp/contextclaw-premium-units.out' EXIT + npx tsc -p packages/core/tsconfig.json --noEmit🤖 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 `@scripts/verify-premium-units.sh` around lines 5 - 7, The script leaves /tmp/cc_build.log and /tmp/contextclaw-premium-units.out behind; add an exit cleanup using a trap that removes those temp files (the ones produced by running "npm run build >/tmp/cc_build.log" and "node packages/core/dist/cli.js mission-demo > /tmp/contextclaw-premium-units.out") so they are deleted on EXIT or ERR; implement the trap near the top of the script and ensure it runs even on interrupts so both temp files are unconditionally removed after the grep/verification step.
1-7: ⚡ Quick winAdd documentation explaining verification purpose and success criteria.
As a verification script in an evidence packet, it should document what it's verifying and what constitutes success. This helps maintainers understand the intent and expected behavior when reviewing the submission.
📝 Proposed documentation
#!/usr/bin/env bash +# +# Verify premium units functionality +# +# This script validates that: +# 1. TypeScript code compiles without errors +# 2. Mission ledger tests pass +# 3. The CLI mission-demo command runs successfully +# 4. The output contains estimatedPremiumUnits fields +# +# Success: Script exits 0 and prints estimatedPremiumUnits entries +# Failure: Script exits non-zero if any step fails or field is missing +# set -euo pipefail🤖 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 `@scripts/verify-premium-units.sh` around lines 1 - 7, The verification script scripts/verify-premium-units.sh currently runs build and test commands but lacks any documentation describing what it verifies and what counts as success; add a brief header comment at the top of the script (above the shebang or immediately after) that states the purpose (e.g., "verify estimatedPremiumUnits output for mission-demo"), the steps performed (mention commands like npx tsc, vitest test mission-ledger.test.ts, npm run build, node packages/core/dist/cli.js mission-demo and grep for estimatedPremiumUnits) and explicit success criteria (e.g., exit code 0, presence of the 'estimatedPremiumUnits' line in /tmp/contextclaw-premium-units.out and expected value/format or non-empty output). Include any notes about artifacts produced (/tmp/cc_build.log, /tmp/contextclaw-premium-units.out) and intended usage so reviewers can understand and reproduce the verification.
|
Closing in favor of the clean replacement PR #7, which is based directly on origin/master and has the intended review diff. |
1 participant
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
Security Improvements