fix: gate cortex memory injection to real user turns#22
Conversation
📝 WalkthroughWalkthroughAdds a "GBrain Shadow v1" observe-only logging feature that captures recall and capture operations without altering existing authorization or injection behavior. Introduces turn-gating classification APIs, GBrain shadow event creation, and per-owner JSONL log persistence. Includes configuration schema updates, test suites for the new features, and a build test script. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MemorySystem
participant TurnGate as Turn Classifier
participant Retriever
participant ShadowLog as Shadow Logger
Caller->>MemorySystem: Recall request (prompt, context)
MemorySystem->>TurnGate: classifyTurnForMemory(prompt, messages, ctx)
alt Synthetic/System Turn Detected
TurnGate-->>MemorySystem: {allow: false, reason: "..."}
MemorySystem->>ShadowLog: createGBrainShadowEvent(skipped)
ShadowLog->>ShadowLog: Write JSONL to dist/logs/
else Real User Turn
TurnGate-->>MemorySystem: {allow: true, reason: "real-user-turn"}
MemorySystem->>Retriever: Attempt authoritative retrieval
alt Items Found
Retriever-->>MemorySystem: Return items
MemorySystem->>ShadowLog: createGBrainShadowEvent(simulated, hits)
else No Items
Retriever-->>MemorySystem: Empty result
MemorySystem->>ShadowLog: createGBrainShadowEvent(skipped, no-items)
end
ShadowLog->>ShadowLog: Write JSONL to dist/logs/
end
MemorySystem-->>Caller: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: The Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR gates Cortex memory recall/capture so it only runs on “real user” turns (skipping synthetic/system control turns), and adds an observe-only “GBrain shadow” scaffold with logging plus regression tests and updated plugin artifacts.
Changes:
- Add shared turn-classification/gating used by both recall injection and capture paths.
- Add GBrain shadow observe-only event/logging scaffolding and new config schema fields.
- Add regression tests, update docs/plugin schema, and redeploy built
dist/artifacts.
Reviewed changes
Copilot reviewed 6 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Implements turn classification/gating, synthetic-turn filtering, and GBrain shadow logging/search scaffolding. |
| src/tests/turn-gating.test.ts | Adds regression tests for synthetic vs real user turn gating and message extraction filtering. |
| src/tests/gbrain-shadow.test.ts | Adds tests for GBrain shadow config parsing and event/log-path helpers. |
| package.json | Adds a consolidated npm test script running build + compiled tests. |
| openclaw.plugin.json | Extends plugin config schema with GBrain shadow options. |
| README.md | Documents new GBrain shadow options and feature overview. |
| dist/index.js | Rebuilt plugin artifact reflecting new gating + GBrain shadow logic and exports. |
| dist/index.d.ts / dist/index.d.ts.map | Updated type declarations and sourcemap for new exports/types. |
| dist/tests/turn-gating.* | Compiled test artifacts for the new turn-gating tests. |
| dist/tests/gbrain-shadow.* | Compiled test artifacts for the new GBrain shadow tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wouldInjectCount: 3, | ||
| }); | ||
| assert.equal(event.phase, "recall"); | ||
| assert.equal(event.mode, "observe"); | ||
| assert.equal(event.provider.model, "MiniMax-M2.7"); | ||
| assert.equal(event.wouldInjectCount, 3); |
There was a problem hiding this comment.
createGBrainShadowEvent doesn’t accept/emit wouldInjectCount, but this test passes wouldInjectCount and asserts event.wouldInjectCount. This will fail TypeScript compilation due to excess-property checks (and the runtime assertion will also fail because the returned event object never includes that field). Decide whether wouldInjectCount should be part of the event schema and update GBrainShadowEvent/createGBrainShadowEvent accordingly, or remove it from the test.
| wouldInjectCount: 3, | |
| }); | |
| assert.equal(event.phase, "recall"); | |
| assert.equal(event.mode, "observe"); | |
| assert.equal(event.provider.model, "MiniMax-M2.7"); | |
| assert.equal(event.wouldInjectCount, 3); | |
| }); | |
| assert.equal(event.phase, "recall"); | |
| assert.equal(event.mode, "observe"); | |
| assert.equal(event.provider.model, "MiniMax-M2.7"); |
| function runGBrainShadowSearch(query: string, limit = 3): { attempted: boolean; ok: boolean; query?: string; hits?: Array<{ slug: string; preview: string }>; summary?: string } { | ||
| const q = clampText(query.trim(), 240); | ||
| if (!q) return { attempted: false, ok: false, summary: "Empty query." }; | ||
| try { | ||
| const res = spawnSync("bun", ["run", "src/cli.ts", "search", q, "--limit", String(limit)], { | ||
| cwd: "/Users/lume/gbrain", | ||
| encoding: "utf8", | ||
| timeout: 15000, | ||
| maxBuffer: 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
runGBrainShadowSearch is hard-coded to run bun with cwd: "/Users/lume/gbrain". This will break in CI/production (nonexistent path, bun not installed) and also makes the plugin behavior machine-specific. Make the command/cwd configurable (or derive relative to the plugin), and ensure the shadow path gracefully disables itself when prerequisites aren’t available.
| function runGBrainShadowSearch(query: string, limit = 3): { attempted: boolean; ok: boolean; query?: string; hits?: Array<{ slug: string; preview: string }>; summary?: string } { | |
| const q = clampText(query.trim(), 240); | |
| if (!q) return { attempted: false, ok: false, summary: "Empty query." }; | |
| try { | |
| const res = spawnSync("bun", ["run", "src/cli.ts", "search", q, "--limit", String(limit)], { | |
| cwd: "/Users/lume/gbrain", | |
| encoding: "utf8", | |
| timeout: 15000, | |
| maxBuffer: 1024 * 1024, | |
| }); | |
| function resolveGBrainShadowCommand(): { cmd: string; cwd: string; script: string } | null { | |
| const cmd = (process.env.GBRAIN_SHADOW_CMD || "bun").trim(); | |
| const script = (process.env.GBRAIN_SHADOW_SCRIPT || "src/cli.ts").trim(); | |
| const configuredCwd = process.env.GBRAIN_SHADOW_CWD?.trim(); | |
| if (!cmd || !script) return null; | |
| const candidateCwds = [ | |
| configuredCwd, | |
| process.cwd(), | |
| ].filter((value): value is string => Boolean(value)); | |
| for (const cwd of candidateCwds) { | |
| if (!existsSync(cwd)) continue; | |
| if (!existsSync(join(cwd, script))) continue; | |
| return { cmd, cwd, script }; | |
| } | |
| return null; | |
| } | |
| function runGBrainShadowSearch(query: string, limit = 3): { attempted: boolean; ok: boolean; query?: string; hits?: Array<{ slug: string; preview: string }>; summary?: string } { | |
| const q = clampText(query.trim(), 240); | |
| if (!q) return { attempted: false, ok: false, summary: "Empty query." }; | |
| const resolved = resolveGBrainShadowCommand(); | |
| if (!resolved) { | |
| return { | |
| attempted: false, | |
| ok: false, | |
| query: q, | |
| summary: "GBrain shadow search is disabled: command prerequisites are not configured or available.", | |
| }; | |
| } | |
| try { | |
| const commandCheck = spawnSync(resolved.cmd, ["--version"], { | |
| cwd: resolved.cwd, | |
| encoding: "utf8", | |
| timeout: 5000, | |
| maxBuffer: 64 * 1024, | |
| }); | |
| if (commandCheck.error || commandCheck.status !== 0) { | |
| return { | |
| attempted: false, | |
| ok: false, | |
| query: q, | |
| summary: "GBrain shadow search is disabled: configured command is unavailable.", | |
| }; | |
| } | |
| const res = spawnSync(resolved.cmd, ["run", resolved.script, "search", q, "--limit", String(limit)], { | |
| cwd: resolved.cwd, | |
| encoding: "utf8", | |
| timeout: 15000, | |
| maxBuffer: 1024 * 1024, | |
| }); | |
| if (res.error) { | |
| return { attempted: true, ok: false, query: q, summary: res.error.message.slice(0, 240) }; | |
| } |
| const gbrainRetrieval = runGBrainShadowSearch(messages.map((m) => m.content).join(" "), 3); | ||
| maybeLogGBrainShadowEvent(cfg, createGBrainShadowEvent({ | ||
| phase: "capture", | ||
| sessionId: ctx.sessionKey, | ||
| turnId: ctx.sessionKey, | ||
| providerBaseUrl: cfg.gbrainShadowProviderBaseUrl, | ||
| model: cfg.gbrainShadowModel, | ||
| conversation: messages, | ||
| maxPromptChars: cfg.gbrainShadowMaxPromptChars, | ||
| status: "simulated", | ||
| note: "Observe-only capture seam. Logged real GBrain docs retrieval metadata without affecting authoritative storage.", | ||
| divergenceFromAuthoritative: "GBrain shadow search runs for comparison only. Cortex remains the authoritative storage path.", | ||
| gbrainRetrieval, | ||
| })); |
There was a problem hiding this comment.
runGBrainShadowSearch uses spawnSync inside before_agent_start and agent_end. Even with a 15s timeout, synchronous child process execution can block the Node event loop and increase turn latency or stall the gateway when enabled. Prefer an async/non-blocking implementation (e.g., spawn + streaming with a short timeout) or queue the work off the hot path so shadow logging can’t impact agent responsiveness.
| const gbrainRetrieval = runGBrainShadowSearch(messages.map((m) => m.content).join(" "), 3); | |
| maybeLogGBrainShadowEvent(cfg, createGBrainShadowEvent({ | |
| phase: "capture", | |
| sessionId: ctx.sessionKey, | |
| turnId: ctx.sessionKey, | |
| providerBaseUrl: cfg.gbrainShadowProviderBaseUrl, | |
| model: cfg.gbrainShadowModel, | |
| conversation: messages, | |
| maxPromptChars: cfg.gbrainShadowMaxPromptChars, | |
| status: "simulated", | |
| note: "Observe-only capture seam. Logged real GBrain docs retrieval metadata without affecting authoritative storage.", | |
| divergenceFromAuthoritative: "GBrain shadow search runs for comparison only. Cortex remains the authoritative storage path.", | |
| gbrainRetrieval, | |
| })); | |
| const shadowQuery = messages.map((m) => m.content).join(" "); | |
| setTimeout(() => { | |
| try { | |
| const gbrainRetrieval = runGBrainShadowSearch(shadowQuery, 3); | |
| maybeLogGBrainShadowEvent(cfg, createGBrainShadowEvent({ | |
| phase: "capture", | |
| sessionId: ctx.sessionKey, | |
| turnId: ctx.sessionKey, | |
| providerBaseUrl: cfg.gbrainShadowProviderBaseUrl, | |
| model: cfg.gbrainShadowModel, | |
| conversation: messages, | |
| maxPromptChars: cfg.gbrainShadowMaxPromptChars, | |
| status: "simulated", | |
| note: "Observe-only capture seam. Logged real GBrain docs retrieval metadata without affecting authoritative storage.", | |
| divergenceFromAuthoritative: "GBrain shadow search runs for comparison only. Cortex remains the authoritative storage path.", | |
| gbrainRetrieval, | |
| })); | |
| } catch (err) { | |
| api.logger.warn(`cortex: gbrain shadow capture logging failed: ${String(err)}`); | |
| } | |
| }, 0); |
| if (maxEntries > 0) { | ||
| try { | ||
| const lines = readFileSync(logPath, "utf8").split("\n").filter(Boolean); | ||
| if (lines.length > maxEntries) { | ||
| writeFileSync(logPath, `${lines.slice(-maxEntries).join("\n")}\n`, "utf8"); | ||
| } |
There was a problem hiding this comment.
Shadow log retention trimming reads and rewrites the entire JSONL file synchronously on every event (readFileSync/writeFileSync). With frequent turns this can become a noticeable blocking cost and wear on disk. Consider trimming less frequently (e.g., probabilistic or size-based), doing it asynchronously, or keeping a separate index/rotation strategy so the hot path remains append-only.
| | `gbrainShadowLogEnabled` | `boolean` | `true` | Persist structured timestamped shadow logs under `dist/logs/` | | ||
| | `gbrainShadowProviderBaseUrl` | `string` | `https://api.minimax.io/v1` | Base URL for MiniMax full-model observe-only path | | ||
| | `gbrainShadowModel` | `string` | `MiniMax-M2.7` | Model name used in GBrain shadow log metadata | |
There was a problem hiding this comment.
README defaults/paths for GBrain shadow don’t match the implementation: the code defaults gbrainShadowProviderBaseUrl to https://api.minimax.io/anthropic and gbrainShadowModel to MiniMax-M2.7-HighSpeed, and writes logs under shadow-logs/ next to the plugin file, not dist/logs/. Please update the README to reflect the actual defaults and log location (or change the code to match the documented behavior).
| | `gbrainShadowLogEnabled` | `boolean` | `true` | Persist structured timestamped shadow logs under `dist/logs/` | | |
| | `gbrainShadowProviderBaseUrl` | `string` | `https://api.minimax.io/v1` | Base URL for MiniMax full-model observe-only path | | |
| | `gbrainShadowModel` | `string` | `MiniMax-M2.7` | Model name used in GBrain shadow log metadata | | |
| | `gbrainShadowLogEnabled` | `boolean` | `true` | Persist structured timestamped shadow logs under `shadow-logs/` next to the plugin file | | |
| | `gbrainShadowProviderBaseUrl` | `string` | `https://api.minimax.io/anthropic` | Base URL for MiniMax full-model observe-only path | | |
| | `gbrainShadowModel` | `string` | `MiniMax-M2.7-HighSpeed` | Model name used in GBrain shadow log metadata | |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca41a8e961
ℹ️ 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".
| if (!q) return { attempted: false, ok: false, summary: "Empty query." }; | ||
| try { | ||
| const res = spawnSync("bun", ["run", "src/cli.ts", "search", q, "--limit", String(limit)], { | ||
| cwd: "/Users/lume/gbrain", |
There was a problem hiding this comment.
The shadow retrieval command is pinned to cwd: "/Users/lume/gbrain", which only exists on the original developer machine. In any other environment where gbrainShadowEnabled is turned on, spawnSync will fail to start from that directory and the new shadow retrieval path will consistently return failed results, making the feature effectively unusable outside that host.
Useful? React with 👍 / 👎.
| if (cfg.gbrainShadowEnabled && cfg.gbrainShadowRecall) { | ||
| const gbrainRetrieval = runGBrainShadowSearch(event.prompt ?? "", 3); | ||
| maybeLogGBrainShadowEvent(cfg, createGBrainShadowEvent({ |
There was a problem hiding this comment.
This recall hook executes runGBrainShadowSearch inline, and that helper uses spawnSync with a 15-second timeout. When gbrainShadowEnabled and gbrainShadowRecall are enabled, each before_agent_start can block the hot path for up to the timeout even though the shadow path is documented as observe-only, adding user-visible latency and reducing throughput.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const res = spawnSync("bun", ["run", "src/cli.ts", "search", q, "--limit", String(limit)], { | ||
| cwd: "/Users/lume/gbrain", |
There was a problem hiding this comment.
🔴 Hardcoded developer-local path makes GBrain shadow search non-functional on any deployment
runGBrainShadowSearch uses a hardcoded cwd: "/Users/lume/gbrain" (src/index.ts:1532) which is a specific developer's local machine path. When gbrainShadowEnabled is turned on in any deployed or other developer environment, every shadow search will fail with ENOENT (caught by try/catch, so no crash). The entire GBrain shadow retrieval feature is non-functional outside this one machine. The hardcoded path also assumes bun is installed as the runtime.
Prompt for agents
The runGBrainShadowSearch function at src/index.ts:1531-1532 has a hardcoded path cwd: /Users/lume/gbrain which is a developer's local machine path. This needs to be made configurable, likely via a new config option (e.g. gbrainShadowSearchCwd or gbrainShadowGBrainPath) added to EvaMemoryConfig with a sensible default or empty-string-means-disabled pattern. The function should also check whether the path exists before attempting spawnSync, and return a graceful 'not configured' result if missing. The bun runtime dependency should also be documented or made configurable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const res = spawnSync("bun", ["run", "src/cli.ts", "search", q, "--limit", String(limit)], { | ||
| cwd: "/Users/lume/gbrain", | ||
| encoding: "utf8", | ||
| timeout: 15000, | ||
| maxBuffer: 1024 * 1024, | ||
| }); |
There was a problem hiding this comment.
🔴 spawnSync blocks the Node.js event loop for up to 15 seconds on recall and capture paths
runGBrainShadowSearch uses spawnSync with a 15-second timeout (src/index.ts:1531-1536), which is a synchronous blocking call. This function is called from the before_agent_start async hook (src/index.ts:2183) and agent_end hook (src/index.ts:2275). While these are inside async functions, spawnSync blocks the entire Node.js event loop thread—no other I/O, timers, or request handlers can execute until the subprocess completes. This directly violates the stated design principle: "Non-blocking capture: agent_end fires and forgets, never blocks gateway". On machines where /Users/lume/gbrain exists and bun is slow to search, this could freeze the gateway for up to 15 seconds per turn.
Prompt for agents
The runGBrainShadowSearch function at src/index.ts:1527-1552 uses spawnSync which blocks the Node.js event loop. This should be converted to use the async spawn (child_process.spawn or child_process.execFile) and the function should be made async. The callers at src/index.ts:2183 (before_agent_start hook) and src/index.ts:2275 (agent_end hook) are already in async contexts so they can await the result. Alternatively, since this is an observe-only shadow feature, the search could be fire-and-forget with spawn (not spawnSync) and results collected asynchronously. The 15-second timeout is also excessive for an observe-only feature - consider reducing it to 2-3 seconds to match the retrieval timeout pattern used elsewhere in this codebase.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Skip synthetic control/system wrapper turns even when they entered as user text | ||
| if (isSyntheticPromptText(text)) continue; |
There was a problem hiding this comment.
🟡 extractMessages applies isSyntheticPromptText filter to assistant messages despite comment saying 'user text'
At src/index.ts:1591-1592, the isSyntheticPromptText(text) check is applied to all messages regardless of role, but the comment says "even when they entered as user text", indicating intent to filter only user messages. Some patterns in isSyntheticPromptText are not start-anchored (e.g., /Read HEARTBEAT\.md if it exists/im at line 934), so they could match inside normal assistant responses that discuss boot checks or heartbeat monitoring. An assistant message like "I checked and will read HEARTBEAT.md if it exists" would be silently dropped from memory capture, causing incomplete conversation storage.
| // Skip synthetic control/system wrapper turns even when they entered as user text | |
| if (isSyntheticPromptText(text)) continue; | |
| // Skip synthetic control/system wrapper turns even when they entered as user text | |
| if (role === "user" && isSyntheticPromptText(text)) continue; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const turnDecision = classifyTurnForMemory(event.prompt, event.messages, { | ||
| ...(ctx as HookLaneContext), | ||
| trigger: (ctx as { trigger?: string }).trigger, | ||
| }); | ||
| const doRetrieve = turnDecision.allow && event.prompt && isMemoryRelevant(event.prompt); |
There was a problem hiding this comment.
📝 Info: Redundant classifyTurnForMemory call in before_agent_start hook
In the before_agent_start hook, shouldSkipMemoryInjection is called at src/index.ts:2088 which internally calls classifyTurnForMemory. If it returns skip: false, we continue. Then at src/index.ts:2099, classifyTurnForMemory is called again with the same arguments. Since the first call already verified allow: true (otherwise we'd have returned), the second call's turnDecision.allow at line 2103 is guaranteed to be true, making it a no-op in the doRetrieve expression. This is harmless but wastes CPU doing the same pattern matching and context resolution twice per turn.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (maxEntries > 0) { | ||
| try { | ||
| const lines = readFileSync(logPath, "utf8").split("\n").filter(Boolean); | ||
| if (lines.length > maxEntries) { | ||
| writeFileSync(logPath, `${lines.slice(-maxEntries).join("\n")}\n`, "utf8"); | ||
| } |
There was a problem hiding this comment.
📝 Info: appendGBrainShadowLog reads-then-writes for log trimming without file locking
The appendGBrainShadowLog function at src/index.ts:1451-1472 appends a log line, then reads the entire file, checks line count against maxEntries, and rewrites if needed. If two concurrent turns trigger shadow logging for the same ownerId, both could read the file simultaneously and overwrite each other's trim results. Since shadow logging is observe-only and wrapped in try/catch, this is best-effort and data loss in logs is acceptable per design. Not a bug, but worth noting that concurrent captures could cause slightly more entries than maxEntries to accumulate.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function hasSyntheticUserTurn(messages: unknown[] | undefined): boolean { | ||
| if (!Array.isArray(messages) || messages.length === 0) return false; | ||
| for (let index = messages.length - 1; index >= 0; index -= 1) { | ||
| const message = messages[index]; | ||
| if (!message || typeof message !== "object") continue; | ||
| const role = (message as Record<string, unknown>).role; | ||
| if (role !== "user") continue; | ||
| const text = extractMessageText(message); | ||
| return isSyntheticPromptText(text); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
📝 Info: hasSyntheticUserTurn only checks the last user message in the array
The hasSyntheticUserTurn function at src/index.ts:957-968 iterates messages in reverse but returns on the first user message found (return isSyntheticPromptText(text) at line 965). This means it only checks whether the last user message in the array is synthetic. If the last user message is real but an earlier user message was synthetic, the function returns false (allow). This appears intentional — it gates on the most recent user turn — but it means a conversation with mixed synthetic and real user messages will be allowed through as long as the last user message is genuine.
Was this helpful? React with 👍 or 👎 to provide feedback.
| gbrainShadowProviderBaseUrl: "https://api.minimax.io/anthropic", | ||
| gbrainShadowModel: "MiniMax-M2.7-HighSpeed", |
There was a problem hiding this comment.
🚩 Default gbrainShadowProviderBaseUrl and gbrainShadowModel differ between config defaults and README docs
The config defaults in parseConfig at src/index.ts:159-160 set gbrainShadowProviderBaseUrl to "https://api.minimax.io/anthropic" and gbrainShadowModel to "MiniMax-M2.7-HighSpeed", while the README at lines 77-78 documents these as "https://api.minimax.io/v1" and "MiniMax-M2.7" respectively. Users reading the README will expect different defaults than what the code actually uses. The JSON schema description in openclaw.plugin.json:75-81 also omits the actual default values, making it ambiguous.
Was this helpful? React with 👍 or 👎 to provide feedback.
| wouldInjectCount: 3, | ||
| }); | ||
| assert.equal(event.phase, "recall"); | ||
| assert.equal(event.mode, "observe"); | ||
| assert.equal(event.provider.model, "MiniMax-M2.7"); | ||
| assert.equal(event.wouldInjectCount, 3); |
There was a problem hiding this comment.
🚩 Test file passes wouldInjectCount to createGBrainShadowEvent which is not in the function params or return type
In src/__tests__/gbrain-shadow.test.ts:26, wouldInjectCount: 3 is passed to createGBrainShadowEvent, but this property is not in the function's params type. At line 31, event.wouldInjectCount is asserted to equal 3, but createGBrainShadowEvent never copies this property to the returned event. With noEmitOnError: false in tsconfig.json, the JS is emitted despite the type error, but the runtime assertion assert.equal(undefined, 3) would fail. This means the test suite would fail when run. Not reporting as a bug per the rules (type-checker would catch the excess property), but the runtime test failure is worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/gbrain-shadow.test.ts`:
- Around line 26-31: The test fails because the GBrainShadowEvent objects
created by createGBrainShadowEvent do not include the wouldInjectCount property
but the test asserts it; update the GBrainShadowEvent interface to include
wouldInjectCount: number and modify createGBrainShadowEvent to accept a
wouldInjectCount parameter (and set it on the returned event object), then
update any call sites (e.g., the test invocation passing wouldInjectCount: 3) so
the created event has event.wouldInjectCount === 3.
In `@src/index.ts`:
- Around line 1460-1468: The current retention trim using
readFileSync(...).split(...) and writeFileSync(logPath, ...) can race with
concurrent writers; update the trimming logic in the block guarded by maxEntries
to perform an atomic replace: read the file via readFileSync(logPath, "utf8"),
compute the trimmed content, write the trimmed content to a temp file (unique
name) and then atomically rename it over logPath (e.g., using
fs.writeFileSync(tempPath, trimmed, "utf8") then fs.renameSync(tempPath,
logPath)); keep the try-catch around this work so failures remain best-effort
and reference the same symbols (maxEntries, logPath, readFileSync,
writeFileSync) when implementing the temp+rename strategy.
- Around line 1527-1552: The runGBrainShadowSearch function uses a hardcoded cwd
("/Users/lume/gbrain") which breaks portability; change it to read a
configurable path (e.g., process.env.GBRAIN_SHADOW_PATH or a passed-in config
gbrainShadowSearchPath) and resolve that path before calling spawnSync, verify
it exists with fs.existsSync and return a clear attempted/ok=false summary if
missing, and also handle the case where "bun" is not available by catching
ENOENT from spawnSync and returning a graceful summary (or fall back to a no-op
result); update the spawnSync call to use the resolved path for cwd and keep
existing timeouts/encoding logic so failures are reported cleanly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a042ab8-cb93-4e71-a604-f6f32bce7deb
⛔ Files ignored due to path filters (12)
dist/__tests__/gbrain-shadow.test.d.tsis excluded by!**/dist/**dist/__tests__/gbrain-shadow.test.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/__tests__/gbrain-shadow.test.jsis excluded by!**/dist/**dist/__tests__/gbrain-shadow.test.js.mapis excluded by!**/dist/**,!**/*.mapdist/__tests__/turn-gating.test.d.tsis excluded by!**/dist/**dist/__tests__/turn-gating.test.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/__tests__/turn-gating.test.jsis excluded by!**/dist/**dist/__tests__/turn-gating.test.js.mapis excluded by!**/dist/**,!**/*.mapdist/index.d.tsis excluded by!**/dist/**dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (6)
README.mdopenclaw.plugin.jsonpackage.jsonsrc/__tests__/gbrain-shadow.test.tssrc/__tests__/turn-gating.test.tssrc/index.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
🪛 LanguageTool
README.md
[typographical] ~137-~137: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...ain layer would have seen or contributed. Logs are timestamped, capped, and inclu...
(WRB_QUESTION_MARK)
🔇 Additional comments (8)
package.json (1)
9-9: Test script looks functional.The chained test execution via
&&ensures build runs first and failures halt the sequence. For larger test suites, consider using Node's built-in test runner (node --test) or a framework, but this approach is acceptable for the current scope.README.md (1)
73-78: Documentation additions look accurate.Config options align with schema and implementation defaults. The static analysis hint about "When" is a false positive—the sentence at line 137 is conditional, not a question.
openclaw.plugin.json (1)
51-86: Config schema additions are well-structured.Properties have appropriate types and clear descriptions. The
gbrainShadowApiKeyfield is correctly marked as optional for future use.src/__tests__/turn-gating.test.ts (1)
1-65: Test coverage is solid for turn-gating logic.
- Covers prompt-based synthetic detection (PLAN_DECISION, QUESTION_ANSWER, heartbeat)
- Covers messages-based synthetic detection
- Validates
extractMessagesfiltering behavior- Tests
isMemoryRelevantheuristicThe tests align with the implementation patterns in
src/index.ts.src/index.ts (4)
49-57: Config interface additions are well-designed.
gbrainShadowEnableddefaults tofalse(safe opt-in)- Sub-features like
gbrainShadowCaptureandgbrainShadowRecalldefault totruebut only activate when the master switch is on- Parsing at lines 193-201 correctly uses opt-in (
=== true) for enabled and opt-out (!== false) for sub-features
929-993: Turn classification logic is comprehensive.
SYNTHETIC_TURN_PATTERNScovers the key control turn markershasSyntheticUserTurncorrectly iterates backward to find the most recent user messageclassifyTurnForMemoryreturns structured{allow, reason}for debugging and logging- Priority order is sensible: heartbeat → runKind → trigger → synthetic checks
1591-1592: Synthetic turn filtering inextractMessagesis well-placed.The filter runs after text extraction and memory context stripping, ensuring synthetic control messages (PLAN_DECISION, session bootstrap, etc.) are excluded from capture payloads.
2182-2197: GBrain shadow logging integration is safely guarded.
- Double-checked by
cfg.gbrainShadowEnabled && cfg.gbrainShadowRecallmaybeLogGBrainShadowEventhas try-catch to prevent failures from blocking authoritative paths- Both "items found" and "no items" branches are logged for full observability
| wouldInjectCount: 3, | ||
| }); | ||
| assert.equal(event.phase, "recall"); | ||
| assert.equal(event.mode, "observe"); | ||
| assert.equal(event.provider.model, "MiniMax-M2.7"); | ||
| assert.equal(event.wouldInjectCount, 3); |
There was a problem hiding this comment.
Test references nonexistent wouldInjectCount field—will fail at runtime.
The GBrainShadowEvent interface and createGBrainShadowEvent function do not define wouldInjectCount:
- Line 26 passes it, but it's ignored (not in function params)
- Line 31 asserts
event.wouldInjectCount === 3, but the property isundefined
Either:
- Add
wouldInjectCountto the interface and function, or - Remove these lines from the test
🐛 Option 2: Remove broken assertion
const event = createGBrainShadowEvent({
phase: "recall",
sessionId: "sess-1",
providerBaseUrl: "https://api.minimax.io/v1",
model: "MiniMax-M2.7",
prompt: "This prompt is intentionally longer than the clamp",
maxPromptChars: 16,
status: "simulated",
note: "observe only",
- wouldInjectCount: 3,
});
assert.equal(event.phase, "recall");
assert.equal(event.mode, "observe");
assert.equal(event.provider.model, "MiniMax-M2.7");
- assert.equal(event.wouldInjectCount, 3);
assert.match(event.promptPreview ?? "", /^This prompt is/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wouldInjectCount: 3, | |
| }); | |
| assert.equal(event.phase, "recall"); | |
| assert.equal(event.mode, "observe"); | |
| assert.equal(event.provider.model, "MiniMax-M2.7"); | |
| assert.equal(event.wouldInjectCount, 3); | |
| }); | |
| assert.equal(event.phase, "recall"); | |
| assert.equal(event.mode, "observe"); | |
| assert.equal(event.provider.model, "MiniMax-M2.7"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/gbrain-shadow.test.ts` around lines 26 - 31, The test fails
because the GBrainShadowEvent objects created by createGBrainShadowEvent do not
include the wouldInjectCount property but the test asserts it; update the
GBrainShadowEvent interface to include wouldInjectCount: number and modify
createGBrainShadowEvent to accept a wouldInjectCount parameter (and set it on
the returned event object), then update any call sites (e.g., the test
invocation passing wouldInjectCount: 3) so the created event has
event.wouldInjectCount === 3.
| if (maxEntries > 0) { | ||
| try { | ||
| const lines = readFileSync(logPath, "utf8").split("\n").filter(Boolean); | ||
| if (lines.length > maxEntries) { | ||
| writeFileSync(logPath, `${lines.slice(-maxEntries).join("\n")}\n`, "utf8"); | ||
| } | ||
| } catch { | ||
| // best-effort retention trim only | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Log retention trimming is acceptable for current scale.
With maxEntries=200 default, file sizes stay bounded. The read-filter-rewrite approach has a potential race condition under concurrent writes, but:
- Logs are per-owner (reduces contention)
- This is observe-only scaffolding (non-critical)
- Wrapped in try-catch for resilience
For higher-scale usage, consider rotating logs or using an append-only approach with periodic cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 1460 - 1468, The current retention trim using
readFileSync(...).split(...) and writeFileSync(logPath, ...) can race with
concurrent writers; update the trimming logic in the block guarded by maxEntries
to perform an atomic replace: read the file via readFileSync(logPath, "utf8"),
compute the trimmed content, write the trimmed content to a temp file (unique
name) and then atomically rename it over logPath (e.g., using
fs.writeFileSync(tempPath, trimmed, "utf8") then fs.renameSync(tempPath,
logPath)); keep the try-catch around this work so failures remain best-effort
and reference the same symbols (maxEntries, logPath, readFileSync,
writeFileSync) when implementing the temp+rename strategy.
| function runGBrainShadowSearch(query: string, limit = 3): { attempted: boolean; ok: boolean; query?: string; hits?: Array<{ slug: string; preview: string }>; summary?: string } { | ||
| const q = clampText(query.trim(), 240); | ||
| if (!q) return { attempted: false, ok: false, summary: "Empty query." }; | ||
| try { | ||
| const res = spawnSync("bun", ["run", "src/cli.ts", "search", q, "--limit", String(limit)], { | ||
| cwd: "/Users/lume/gbrain", | ||
| encoding: "utf8", | ||
| timeout: 15000, | ||
| maxBuffer: 1024 * 1024, | ||
| }); | ||
| if (res.status !== 0) { | ||
| return { attempted: true, ok: false, query: q, summary: (res.stderr || res.stdout || `exit ${res.status}`).trim().slice(0, 240) }; | ||
| } | ||
| const lines = (res.stdout || "").split("\n").map((x) => x.trim()).filter(Boolean).slice(0, limit); | ||
| const hits = lines.map((line) => { | ||
| const match = line.match(/^\[[^\]]+\]\s+([^\s]+)\s+--\s+(.*)$/); | ||
| return { | ||
| slug: match?.[1] || line.slice(0, 80), | ||
| preview: clampText(match?.[2] || line, 160), | ||
| }; | ||
| }); | ||
| return { attempted: true, ok: true, query: q, hits, summary: hits.length ? `Found ${hits.length} doc hits.` : "No doc hits." }; | ||
| } catch (err) { | ||
| return { attempted: true, ok: false, query: q, summary: err instanceof Error ? err.message.slice(0, 240) : String(err).slice(0, 240) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoded path /Users/lume/gbrain breaks portability.
Line 1532 uses a developer-specific absolute path:
cwd: "/Users/lume/gbrain",This will fail on any machine except the original developer's. Additionally, bun may not be installed.
Recommendations:
- Use a config option (e.g.,
gbrainShadowSearchPath) or environment variable - Gracefully degrade if path doesn't exist or bun isn't available
🔧 Proposed fix: Make path configurable with graceful fallback
+function getGBrainSearchCwd(): string | null {
+ const fromEnv = process.env.GBRAIN_PATH;
+ if (fromEnv && existsSync(fromEnv)) return fromEnv;
+ return null;
+}
+
function runGBrainShadowSearch(query: string, limit = 3): { attempted: boolean; ok: boolean; query?: string; hits?: Array<{ slug: string; preview: string }>; summary?: string } {
const q = clampText(query.trim(), 240);
if (!q) return { attempted: false, ok: false, summary: "Empty query." };
+ const cwd = getGBrainSearchCwd();
+ if (!cwd) return { attempted: false, ok: false, summary: "GBrain path not configured." };
try {
const res = spawnSync("bun", ["run", "src/cli.ts", "search", q, "--limit", String(limit)], {
- cwd: "/Users/lume/gbrain",
+ cwd,
encoding: "utf8",
timeout: 15000,
maxBuffer: 1024 * 1024,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 1527 - 1552, The runGBrainShadowSearch function
uses a hardcoded cwd ("/Users/lume/gbrain") which breaks portability; change it
to read a configurable path (e.g., process.env.GBRAIN_SHADOW_PATH or a passed-in
config gbrainShadowSearchPath) and resolve that path before calling spawnSync,
verify it exists with fs.existsSync and return a clear attempted/ok=false
summary if missing, and also handle the case where "bun" is not available by
catching ENOENT from spawnSync and returning a graceful summary (or fall back to
a no-op result); update the spawnSync call to use the resolved path for cwd and
keep existing timeouts/encoding logic so failures are reported cleanly.
Summary
Verification
dist/index.jsnow matches the repo build hashMEMORY_PREAMBLE is not definedfailures after restartSummary by CodeRabbit
Release Notes
New Features
Documentation
Tests