Skip to content

feat: bridge QMD into Honcho memory_search/memory_get - #99

Open
unithejerk wants to merge 14 commits into
plastic-labs:mainfrom
unithejerk:feat/qmd-honcho-bridge
Open

unithejerk wants to merge 14 commits into
plastic-labs:mainfrom
unithejerk:feat/qmd-honcho-bridge

Conversation

@unithejerk

@unithejerk unithejerk commented May 20, 2026

Copy link
Copy Markdown

When memory.backend: 'qmd' is configured, the Honcho plugin's memory_search/memory_get tools only return Honcho session transcripts. This patch adds parallel QMD query execution so indexed local files (such as wiki docs) appear in results alongside session data.

Changes:

  • import execSync from child_process
  • isQmdConfigured(), qmdSearchMode(), qmdCommand() helpers reading from state.api.config.memory (openclaw.json)
  • qmdSearch shells out to qmd --json -n with 30s timeout, maps qmd's file/line fields to Honcho's path/startLine format
  • qmdGet shells out to qmd get for qmd:// URIs
  • search() runs QMD in parallel with Honcho, merges results, clamps session scores to 0.5 so file results sort above them
  • readFile() handles qmd:// paths before falling back to Honcho
  • status() reports provider: 'honcho+qmd' when QMD is active

This was developed with AI assistance and tested on live infrastructure.

Summary by CodeRabbit

  • New Features

    • Optional QMD-backed memory search alongside existing search; qmd:// content retrieval with optional allowlist; status now reflects combined provider/QMD availability.
  • Bug Fixes

    • CLI flag validation and safer workspace path resolution; more robust sender/peer ID generation and stricter timestamp handling.
  • Tests

    • Added deterministic QMD bridge tests covering detection, search merging/scoring, retrieval, prefix gating, and fallbacks.
  • Tools

    • Memory search tool no longer accepts minScore; ask tool returns a safe fallback when no answer.
  • Documentation

    • Expanded JSDoc across tools, hooks, config, helpers, and runtime.

Review Change Stack

When memory.backend: 'qmd' is configured, the Honcho plugin's
memory_search/memory_get tools only return Honcho session transcripts.
This patch adds parallel QMD query execution so indexed local files
(such as wiki docs) appear in results alongside session data.

Changes:
- import execSync from child_process
- isQmdConfigured(), qmdSearchMode(), qmdCommand() helpers reading
  from state.api.config.memory (openclaw.json)
- qmdSearch shells out to qmd <mode> <query> --json -n <limit>
  with 30s timeout, maps qmd's file/line fields to Honcho's
  path/startLine format
- qmdGet shells out to qmd get <path> for qmd:// URIs
- search() runs QMD in parallel with Honcho, merges results,
  clamps session scores to 0.5 so file results sort above them
- readFile() handles qmd:// paths before falling back to Honcho
- status() reports provider: 'honcho+qmd' when QMD is active
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e339cb4e-d850-451d-a069-e18aaf0dffa0

📥 Commits

Reviewing files that changed from the base of the PR and between 9de232a and 690f8b7.

📒 Files selected for processing (1)
  • runtime.ts

Walkthrough

Adds optional QMD CLI integration to the Honcho memory runtime: concurrent QMD searches merged ahead of Honcho results with clamped Honcho scores; readFile handles qmd:// URIs with optional prefix gating; status reports QMD availability when configured; tests cover config, search, readFile, and merging behavior.

Changes

QMD Memory Backend Integration

Layer / File(s) Summary
QMD CLI Infrastructure and Test Setup
runtime.ts, test/qmd-bridge.test.ts
Adds child_process import, Vitest test file with subprocess mocks, and a deterministic QMD_RESPONSE payload used across tests.
QMD Search Helpers and Invocation
runtime.ts, test/qmd-bridge.test.ts
Adds QMD configuration helpers, a timed execFile wrapper, qmdSearch/qmdGet, starts a concurrent QMD search alongside Honcho, and adjusts session key flow; tests assert subprocess args, binary selection, JSON→result mapping, and fallback behavior.
Result Merging, readFile, and Status
runtime.ts, test/qmd-bridge.test.ts
Merges QMD+Honcho results (QMD first; clamp Honcho scores ≤0.5), slices to limit; readFile() supports qmd:// URIs with optional allowed-prefix validation and returns { source: "qmd" }; status() advertises honcho+qmd and includes "qmd"/custom.qmd when available; tests validate status, readFile, and score clamping.

Docs, small behavioral changes, and tooling tweaks

Layer / File(s) Summary
CLI option validation and config docs
commands/cli.ts, config.ts
Adds JSDoc for manifest helpers, wraps fs.realpathSync to avoid throwing on resolution, validates honcho search top-k/max-distance flags, and documents env var resolution in config.
Message extraction and timestamp hardening
helpers.ts
Treats custom_message like user for sender resolution, tightens timestamp conversion to finite >0 values, and refreshes JSDoc around message/session helpers.
Peer-id fallback & participant cache
peers.ts, state.ts
Adds base64url fallback peer-id when sanitization yields empty, deterministic sender_<hash> minting, and replaces iterative participant peer checks with a cached Set + lazy rebuild; documents related helpers.
Tools and memory schema
tools/*.ts
Adds JSDoc for several tools, switches to api.logger.warn for one warning path, removes minScore from MemorySearch tool schema and input parsing, and hardens honcho_ask fallback text.
Hooks and plugin entry docs
hooks/*.ts, index.ts
Adds brief docs above hook registrations and the plugin entry; no runtime behavior changes.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant MemoryRuntime
  participant HonchoSession
  participant QMD_CLI
  Client->>MemoryRuntime: search(query, maxResults)
  MemoryRuntime->>HonchoSession: collectTranscriptMatches(query)
  MemoryRuntime->>QMD_CLI: execFile("qmd", ["search", "--mode", ...])
  QMD_CLI-->>MemoryRuntime: stdout JSON results
  HonchoSession-->>MemoryRuntime: honchoResults
  MemoryRuntime->>MemoryRuntime: clampHonchoScores(<=0.5), prepend QMD results, slice(limit)
  MemoryRuntime-->>Client: mergedResults
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibbled through the runtime's lane,
Two threads now hum a doubled refrain,
QMD brings lines, Honcho keeps the train,
Scores tucked softly, merged without pain,
A hop, a sniff, and memory's gain.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: integrating QMD into Honcho's memory_search and memory_get functionality.
Docstring Coverage ✅ Passed Docstring coverage is 90.63% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@runtime.ts`:
- Around line 167-179: The qmdGet function uses execSync with a concatenated
command which allows command injection via the path parameter; change qmdGet to
call execFileSync (or execFile) using qmdCommand() as the executable and pass
['get', path] (without shell-concatenation/JSON.stringify) as the args array,
keep the same options (encoding, timeout, stdio) and preserve the try/catch
behavior so it returns stdout or null; update any import/require if needed and
ensure this mirrors the fix applied elsewhere (e.g., the readFile/relPath use)
to avoid shell interpretation of the path.
- Around line 185-210: Remove the duplicate earlier declaration and redundant
init: delete the first "let requestedSessionKey = typeof opts.sessionKey ===
\"string\" && opts.sessionKey.length > 0 ? opts.sessionKey : activeSessionKey ??
null;" so only the later const requestedSessionKey remains, and remove the
earlier redundant await state.ensureInitialized() so state.ensureInitialized()
is called only once before resolving participantPeer (keep the single await near
where participantPeer is resolved); ensure all references use the remaining
requestedSessionKey and that state initialization still happens before
state.resolveSessionParticipantPeer/getParticipantPeer.
- Around line 143-165: The qmdSearch function currently builds a shell string
and calls execSync, which allows command injection via the query; replace the
execSync call with execFileSync and pass qmdCommand() as the executable plus an
arguments array containing qmdSearchMode(), JSON.stringify(query), "--json",
"-n", String(limit) (so the shell is not used), preserve encoding, timeout and
stdio options, then parse stdout as before and keep the same mapping logic and
error handling in qmdSearch.
🪄 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: CHILL

Plan: Pro

Run ID: 8d09ad0d-675f-421b-96f0-067d817b919e

📥 Commits

Reviewing files that changed from the base of the PR and between bb546c2 and 6b5d0c7.

📒 Files selected for processing (1)
  • runtime.ts

Comment thread runtime.ts Outdated
Comment thread runtime.ts
Comment thread runtime.ts Outdated
13 tests covering:
- isQmdConfigured detection (4 config scenarios)
- qmd search field mapping and command construction (4 scenarios)
- score clamping in merged results (2 scenarios)
- qmd:// path handling in readFile (2 scenarios)
- fallback behavior when QMD fails (1 scenario)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
runtime.ts (1)

185-188: ⚡ Quick win

Remove duplicated requestedSessionKey assignment.

The same expression is computed twice (Line 185 and Line 208). Keep a single const assignment to avoid drift and unnecessary mutable state.

Suggested cleanup
-        let requestedSessionKey =
-          typeof opts.sessionKey === "string" && opts.sessionKey.length > 0
-            ? opts.sessionKey
-            : activeSessionKey ?? null;
+        const requestedSessionKey =
+          typeof opts.sessionKey === "string" && opts.sessionKey.length > 0
+            ? opts.sessionKey
+            : activeSessionKey ?? null;
...
-        requestedSessionKey =          typeof opts.sessionKey === "string" && opts.sessionKey.length > 0
-            ? opts.sessionKey
-            : activeSessionKey ?? null;

Also applies to: 208-210

🤖 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 `@runtime.ts` around lines 185 - 188, The code computes requestedSessionKey
twice causing duplicate/mutable state; consolidate to a single const by removing
the later reassignment and using the initial const requestedSessionKey computed
from opts.sessionKey and activeSessionKey (i.e., keep the expression typeof
opts.sessionKey === "string" && opts.sessionKey.length > 0 ? opts.sessionKey :
activeSessionKey ?? null as a single const) and update any subsequent logic to
reference that const instead of reassigning requestedSessionKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@runtime.ts`:
- Around line 185-188: The code computes requestedSessionKey twice causing
duplicate/mutable state; consolidate to a single const by removing the later
reassignment and using the initial const requestedSessionKey computed from
opts.sessionKey and activeSessionKey (i.e., keep the expression typeof
opts.sessionKey === "string" && opts.sessionKey.length > 0 ? opts.sessionKey :
activeSessionKey ?? null as a single const) and update any subsequent logic to
reference that const instead of reassigning requestedSessionKey.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f66c0d03-0146-4d33-be67-bff505cb43a4

📥 Commits

Reviewing files that changed from the base of the PR and between 6b5d0c7 and 038492d.

📒 Files selected for processing (2)
  • runtime.ts
  • test/qmd-bridge.test.ts

…undant ensureInitialized

Addresses PR review feedback:
- qmdSearch: replace execSync shell string with execFileSync, passing
  [cmd, args...] array to prevent command injection via query parameter
- qmdGet: replace execSync shell string with execFileSync, passing
  ["get", path] array to prevent injection via path parameter
- search(): remove duplicate let requestedSessionKey declaration and
  redundant early ensureInitialized() call; single const declaration
  remains after the QMD promise block
- Keep unused execSync import removed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
runtime.ts (1)

276-286: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Error message is misleading when qmd get fails.

When qmdGet(relPath) returns null (CLI failure), the error thrown says "Unsupported Honcho memory path" — but the qmd:// path format is supported; the QMD CLI invocation simply failed. This could cause confusion during debugging.

🛠️ Proposed fix: distinguish QMD retrieval failure from unsupported path
         if (typeof relPath === "string" && relPath.startsWith("qmd://")) {
           const qmdText = qmdGet(relPath);
           if (qmdText !== null) {
             return {
               path: relPath,
               text: sliceLines(qmdText, params.from, params.lines),
               source: "qmd",
             };
           }
-          throw new Error(`Unsupported Honcho memory path: ${relPath}`);
+          throw new Error(`Failed to retrieve QMD content for path: ${relPath}`);
         }
🤖 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 `@runtime.ts` around lines 276 - 286, The error thrown when qmdGet(relPath)
returns null is misleading; instead of throwing "Unsupported Honcho memory path"
(which implies the qmd:// scheme isn't supported), change the code in the qmd://
handling branch to throw a clear retrieval failure that includes the relPath
(and any available error info) — e.g. "Failed to retrieve qmd content for
<relPath>" — so callers know the QMD CLI/lookup failed; update the throw in the
block that checks qmdGet(relPath) and keep the returned object using
sliceLines(qmdText, params.from, params.lines) unchanged.
🧹 Nitpick comments (1)
test/qmd-bridge.test.ts (1)

137-146: 💤 Low value

Unused variable state before memState.

Line 138 creates state but it's never used — memState is created immediately after and passed to the manager. This appears to be leftover code from an earlier iteration.

♻️ Proposed fix
   it("returns honcho+qmd when qmd block exists (even empty)", async () => {
-    const state = createState({ memory: createMemoryConfig({ command: undefined, searchMode: undefined }) });
-    // Empty qmd block — isQmdConfigured checks mem?.qmd truthiness
-    // But qmd is { includeDefaultMemory: true } which is truthy
-    // So for this test, set qmd to {}
     const memState = createState({ memory: { backend: "qmd", qmd: {} } });
     const { manager } = await getHonchoMemorySearchManager(memState, { agentId: "main" });
     const s = manager.status();
     expect(s.provider).toBe("honcho+qmd");
   });
🤖 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 `@test/qmd-bridge.test.ts` around lines 137 - 146, In the test "returns
honcho+qmd when qmd block exists (even empty)" there is an unused variable
`state` created via createState(...) that is never used; remove that leftover
`state` declaration and keep only `memState` (the one passed into
getHonchoMemorySearchManager), ensuring the test uses `memState` exclusively and
no unused local `state` remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@runtime.ts`:
- Line 146: The CLI argument for the qmd search is being built with
JSON.stringify(query), which causes extra quoting when using execFileSync;
update the call that builds args for execFileSync (where qmdCommand(),
qmdSearchMode(), JSON.stringify(query), "--json", "-n", String(limit) are used)
to pass the query value directly (or ensure it's a plain string) instead of
JSON.stringify; if query can be a non-string object, serialize it outside in a
way the qmd CLI expects (e.g., convert to the intended string form) before
passing it as an argument to execFileSync.

---

Outside diff comments:
In `@runtime.ts`:
- Around line 276-286: The error thrown when qmdGet(relPath) returns null is
misleading; instead of throwing "Unsupported Honcho memory path" (which implies
the qmd:// scheme isn't supported), change the code in the qmd:// handling
branch to throw a clear retrieval failure that includes the relPath (and any
available error info) — e.g. "Failed to retrieve qmd content for <relPath>" — so
callers know the QMD CLI/lookup failed; update the throw in the block that
checks qmdGet(relPath) and keep the returned object using sliceLines(qmdText,
params.from, params.lines) unchanged.

---

Nitpick comments:
In `@test/qmd-bridge.test.ts`:
- Around line 137-146: In the test "returns honcho+qmd when qmd block exists
(even empty)" there is an unused variable `state` created via createState(...)
that is never used; remove that leftover `state` declaration and keep only
`memState` (the one passed into getHonchoMemorySearchManager), ensuring the test
uses `memState` exclusively and no unused local `state` remains.
🪄 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: CHILL

Plan: Pro

Run ID: f53c4ef2-4957-42d7-b726-0a8172f203f4

📥 Commits

Reviewing files that changed from the base of the PR and between 038492d and 710e72c.

📒 Files selected for processing (2)
  • runtime.ts
  • test/qmd-bridge.test.ts

Comment thread runtime.ts Outdated
unithejerk and others added 3 commits May 20, 2026 00:20
execFileSync doesn't invoke a shell so JSON.stringify adds unwanted
literal quote characters to the query string. Pass query directly.
execFileSync doesn't invoke a shell so JSON.stringify adds
unwanted literal quote characters. Pass query directly.
@unithejerk

Copy link
Copy Markdown
Author

Warning

Rate limit exceeded

@r0c1n4nte[bot] has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minute and 30 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.
⌛ How to resolve this issue?

🚦 How do rate limits work?

ℹ️ Review info

Walkthrough

This PR extends the memory runtime to optionally integrate a QMD CLI backend for memory search and file retrieval. When configured, QMD search runs in parallel with Honcho transcript-based search, with results merged by QMD priority and Honcho results clamped and appended. File reads now recognize qmd:// paths and retrieve content via QMD CLI. Status reporting advertises QMD availability when configured.

Changes

QMD Memory Backend Integration
Layer / File(s) Summary
QMD CLI Infrastructure and Helpers
runtime.ts, test/qmd-bridge.test.ts Imports execSync; adds QMD configuration detection and two helpers to run qmd search (JSON → search shape) and qmd get (raw content), both returning null on failure; test scaffolding and mocked QMD payload added.
Parallel Search and Result Merging
runtime.ts, test/qmd-bridge.test.ts search() computes effective limits, starts a QMD search concurrently with Honcho collection, normalizes QMD results, clamps Honcho session scores, concatenates QMD + clamped Honcho, and slices to the requested limit; tests cover subprocess invocation, JSON mapping, fallback, and score clamping.
File Reading and Availability Reporting
runtime.ts, test/qmd-bridge.test.ts readFile() accepts qmd:// URIs and uses qmd get to fetch content (returns path/text/source: "qmd"); status() reports honcho+qmd and includes qmd in sources/custom when available; tests validate config detection and read failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I dug a tunnel through the code,
Two paths met where memories grow,
Honcho hums while QMD strode,
Results braided in a gentle flow,
Carrots and commits in a tidy row.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

✨ Finishing Touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Warning

Rate limit exceeded

@r0c1n4nte[bot] has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minute and 30 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.
⌛ How to resolve this issue?

🚦 How do rate limits work?

ℹ️ Review info

Walkthrough

This PR extends the memory runtime to optionally integrate a QMD CLI backend for memory search and file retrieval. When configured, QMD search runs in parallel with Honcho transcript-based search, with results merged by QMD priority and Honcho results clamped and appended. File reads now recognize qmd:// paths and retrieve content via QMD CLI. Status reporting advertises QMD availability when configured.

Changes

QMD Memory Backend Integration
Layer / File(s) Summary
QMD CLI Infrastructure and Helpers
runtime.ts, test/qmd-bridge.test.ts Imports execSync; adds QMD configuration detection and two helpers to run qmd search (JSON → search shape) and qmd get (raw content), both returning null on failure; test scaffolding and mocked QMD payload added.
Parallel Search and Result Merging
runtime.ts, test/qmd-bridge.test.ts search() computes effective limits, starts a QMD search concurrently with Honcho collection, normalizes QMD results, clamps Honcho session scores, concatenates QMD + clamped Honcho, and slices to the requested limit; tests cover subprocess invocation, JSON mapping, fallback, and score clamping.
File Reading and Availability Reporting
runtime.ts, test/qmd-bridge.test.ts readFile() accepts qmd:// URIs and uses qmd get to fetch content (returns path/text/source: "qmd"); status() reports honcho+qmd and includes qmd in sources/custom when available; tests validate config detection and read failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I dug a tunnel through the code,
Two paths met where memories grow,
Honcho hums while QMD strode,
Results braided in a gentle flow,
Carrots and commits in a tidy row.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

✨ Finishing Touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

We also added 13 unit tests for the QMD bridge in 038492d — covers config detection, field mapping, score clamping, qmd:// path handling, and fallback behavior. All tests passing alongside the existing 28 tests (41 total).

Resolved in 992c0d2 — added JSDoc docstrings to search(), readFile(), and status() methods.

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown

Review rate limit: 2/2 reviews remaining, available now.

@unithejerk

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
runtime.ts (1)

316-325: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Misleading error when QMD CLI retrieval fails for a valid qmd:// path.

When qmdGet(relPath) returns null (CLI timeout, non-zero exit, file not found, etc.) for a qmd:// URI that already passed the allowedPrefixes check, the error thrown is Unsupported Honcho memory path: .... The path is supported — the underlying retrieval failed. This conflates a config/path-format error with a runtime CLI failure and complicates debugging in production.

🐛 Proposed fix: differentiate the qmd:// runtime-failure error
           const qmdText = await qmdGet(relPath);
           if (qmdText !== null) {
             return {
               path: relPath,
               text: sliceLines(qmdText, params.from, params.lines),
               source: "qmd",
             };
           }
-          throw new Error(`Unsupported Honcho memory path: ${relPath}`);
+          throw new Error(`Failed to read QMD memory path: ${relPath}`);
         }

Note: test/qmd-bridge.test.ts line ~296 asserts /Unsupported Honcho memory path/ for the qmd-get-fails case; that regex will also need updating to match the new message.

🤖 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 `@runtime.ts` around lines 316 - 325, The current code throws "Unsupported
Honcho memory path: relPath" when qmdGet(relPath) returns null, which obscures
that the qmd:// path passed allowedPrefixes but the CLI retrieval failed; change
the thrown error inside the qmdGet null branch to a runtime-failure message
(e.g., "Failed to retrieve qmd:// path: <relPath> — qmd CLI returned no content"
and include any available error/exit info) so it distinguishes config/format
errors from CLI/runtime failures; update the test (test/qmd-bridge.test.ts) that
matches /Unsupported Honcho memory path/ to expect the new message or a more
general regex for "Failed to retrieve" instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@runtime.ts`:
- Around line 316-325: The current code throws "Unsupported Honcho memory path:
relPath" when qmdGet(relPath) returns null, which obscures that the qmd:// path
passed allowedPrefixes but the CLI retrieval failed; change the thrown error
inside the qmdGet null branch to a runtime-failure message (e.g., "Failed to
retrieve qmd:// path: <relPath> — qmd CLI returned no content" and include any
available error/exit info) so it distinguishes config/format errors from
CLI/runtime failures; update the test (test/qmd-bridge.test.ts) that matches
/Unsupported Honcho memory path/ to expect the new message or a more general
regex for "Failed to retrieve" instead.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c093cf02-8f9e-4a71-bc0a-b62bc377d621

📥 Commits

Reviewing files that changed from the base of the PR and between 992c0d2 and 5d4492a.

📒 Files selected for processing (2)
  • runtime.ts
  • test/qmd-bridge.test.ts

@r0c1n4nte
r0c1n4nte Bot force-pushed the feat/qmd-honcho-bridge branch from 5d4492a to 1633b16 Compare May 21, 2026 19:32
@r0c1n4nte
r0c1n4nte Bot force-pushed the feat/qmd-honcho-bridge branch from 1633b16 to c5b83d2 Compare May 21, 2026 19:41
@unithejerk

Copy link
Copy Markdown
Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
✅ Actions performed

Reviews resumed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/qmd-bridge.test.ts (1)

155-164: ⚡ Quick win

Drop the unused state setup in the "even empty" test.

state on line 156 is created but never used — only memState is consumed by the test. The dead line plus the inline rationale comments make the test harder to read.

🧹 Proposed cleanup
   it("returns honcho+qmd when qmd block exists (even empty)", async () => {
-    const state = createState({ memory: createMemoryConfig({ command: undefined, searchMode: undefined }) });
-    // Empty qmd block — isQmdConfigured checks mem?.qmd truthiness
-    // But qmd is { includeDefaultMemory: true } which is truthy
-    // So for this test, set qmd to {}
-    const memState = createState({ memory: { backend: "qmd", qmd: {} } });
-    const { manager } = await getHonchoMemorySearchManager(memState, { agentId: "main" });
+    // isQmdConfigured() only requires mem.qmd to be truthy; an empty object qualifies.
+    const state = createState({ memory: { backend: "qmd", qmd: {} } });
+    const { manager } = await getHonchoMemorySearchManager(state, { agentId: "main" });
     const s = manager.status();
     expect(s.provider).toBe("honcho+qmd");
   });
🤖 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 `@test/qmd-bridge.test.ts` around lines 155 - 164, Remove the dead setup in the
unit test "returns honcho+qmd when qmd block exists (even empty)": delete the
unused local variable state created via createState(...) and its inline
rationale comments, leaving only the memState = createState({ memory: { backend:
"qmd", qmd: {} } }) setup and the calls to getHonchoMemorySearchManager and
manager.status(); ensure createState, memState and getHonchoMemorySearchManager
remain unchanged so the test still asserts
expect(s.provider).toBe("honcho+qmd").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/qmd-bridge.test.ts`:
- Around line 155-164: Remove the dead setup in the unit test "returns
honcho+qmd when qmd block exists (even empty)": delete the unused local variable
state created via createState(...) and its inline rationale comments, leaving
only the memState = createState({ memory: { backend: "qmd", qmd: {} } }) setup
and the calls to getHonchoMemorySearchManager and manager.status(); ensure
createState, memState and getHonchoMemorySearchManager remain unchanged so the
test still asserts expect(s.provider).toBe("honcho+qmd").

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9b4966bb-7899-45ae-b198-2fb87ec1ba25

📥 Commits

Reviewing files that changed from the base of the PR and between 992c0d2 and c5b83d2.

📒 Files selected for processing (2)
  • runtime.ts
  • test/qmd-bridge.test.ts

r0c1n4nte Bot and others added 3 commits May 21, 2026 19:59
- Docstrings: bring coverage to 82.5% across all source files
  (config.ts, helpers.ts, index.ts, peers.ts, runtime.ts, state.ts,
  hooks/*, tools/*)
- Fix: cast config access in qmdAllowedPrefixes for TS type compat
- Fix: capture custom_message events (was data loss bug)
- Fix: validate timestamps with Number.isFinite() before new Date()
- Fix: guard against empty sanitized peer IDs
- Fix: optimize isParticipantPeerId() from O(n) to O(1) with Set cache
- Fix: skip wasted Honcho API call when search limit already filled
- Fix: handle broken symlinks in uniqueWorkspacePaths()
- Fix: validate NaN from parseInt/parseFloat in CLI search
- Fix: remove non-null assertion on agentPeer.chat() result
- Fix: replace console.warn with api.logger.warn for log routing
- Fix: remove unused minScore param from memory_search schema
- Fix: handle empty sanitized sender IDs in resolveParticipantPeerId()

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
commands/cli.ts (1)

492-492: ⚡ Quick win

Consider enforcing topK upper bound for consistency with tool schema.

The validation checks topK > 0 but allows arbitrarily large values. The tool schema in tools/search.ts enforces maximum: 100. For consistency, consider clamping topK to 100.

♻️ Proposed refinement
-const searchTopK = Number.isFinite(topK) && topK > 0 ? topK : 10;
+const searchTopK = Number.isFinite(topK) && topK > 0 ? Math.min(topK, 100) : 10;
🤖 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 `@commands/cli.ts` at line 492, The current calculation of searchTopK allows
arbitrarily large topK values (const searchTopK = Number.isFinite(topK) && topK
> 0 ? topK : 10), which conflicts with the tool schema maximum of 100; change
the logic to clamp the value to the schema bound by ensuring searchTopK =
Math.min(100, Number.isFinite(topK) && topK > 0 ? topK : 10) (or equivalent) so
values above 100 are reduced to 100; reference the variable searchTopK and the
input topK and keep the fallback of 10 when topK is invalid.
state.ts (1)

180-184: 💤 Low value

Fallback format differs from peers.ts — consider aligning.

This secondary fallback uses sender-${sha256...} (hyphen, 16 chars) while peers.ts uses sender_${base64url...} (underscore, 32 chars). If both fallbacks ever fire for the same input (e.g., due to a future regression), the same channelPeerId would map to different Honcho peer IDs. Consider aligning the format with peers.ts for consistency, or removing this guard if the peers.ts fallback is considered sufficient.

🤖 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 `@state.ts` around lines 180 - 184, The fallback peer-id generation in this
file uses `sender-${createHash("sha256")...slice(0,16)}` which differs from the
format used in peers.ts; update the fallback to match peers.ts (use the same
base64url-encoded SHA256 slice with an underscore prefix) so identical inputs
produce identical Honcho peer IDs. Concretely, in the branch after
`resolveParticipantPeerId(...)` update the `resolvedPeerId` assignment to
replicate peers.ts' logic (use the same base64url(sha256(channelPeerId))
encoding and slicing and the `sender_` prefix) instead of the current hyphenated
hex slice; keep the guard but ensure you reference the same encoding/util used
in peers.ts to maintain consistency between `resolveParticipantPeerId`, this
fallback, and peers.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@state.ts`:
- Around line 211-225: The participantPeerIdCache is built lazily by
rebuildParticipantPeerIdCache() but never cleared when the participantPeers map
changes, causing isParticipantPeerId() to return stale results; update all
places that mutate state.participantPeers (e.g., the code paths calling
participantPeers.set(...) and participantPeers.delete(...) or any add/remove
handlers) to invalidate the cache by setting participantPeerIdCache = null after
the mutation so rebuildParticipantPeerIdCache() will rebuild an up-to-date Set
on the next isParticipantPeerId(...) call.

---

Nitpick comments:
In `@commands/cli.ts`:
- Line 492: The current calculation of searchTopK allows arbitrarily large topK
values (const searchTopK = Number.isFinite(topK) && topK > 0 ? topK : 10), which
conflicts with the tool schema maximum of 100; change the logic to clamp the
value to the schema bound by ensuring searchTopK = Math.min(100,
Number.isFinite(topK) && topK > 0 ? topK : 10) (or equivalent) so values above
100 are reduced to 100; reference the variable searchTopK and the input topK and
keep the fallback of 10 when topK is invalid.

In `@state.ts`:
- Around line 180-184: The fallback peer-id generation in this file uses
`sender-${createHash("sha256")...slice(0,16)}` which differs from the format
used in peers.ts; update the fallback to match peers.ts (use the same
base64url-encoded SHA256 slice with an underscore prefix) so identical inputs
produce identical Honcho peer IDs. Concretely, in the branch after
`resolveParticipantPeerId(...)` update the `resolvedPeerId` assignment to
replicate peers.ts' logic (use the same base64url(sha256(channelPeerId))
encoding and slicing and the `sender_` prefix) instead of the current hyphenated
hex slice; keep the guard but ensure you reference the same encoding/util used
in peers.ts to maintain consistency between `resolveParticipantPeerId`, this
fallback, and peers.ts.
🪄 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: CHILL

Plan: Pro

Run ID: 65bb9c30-96f2-4add-a82b-6a1b0a9dd0df

📥 Commits

Reviewing files that changed from the base of the PR and between 49cf252 and fb06a47.

📒 Files selected for processing (16)
  • commands/cli.ts
  • config.ts
  • helpers.ts
  • hooks/capture.ts
  • hooks/context.ts
  • hooks/gateway.ts
  • index.ts
  • peers.ts
  • runtime.ts
  • state.ts
  • tools/ask.ts
  • tools/context.ts
  • tools/memory-passthrough.ts
  • tools/message-search.ts
  • tools/search.ts
  • tools/session.ts
💤 Files with no reviewable changes (1)
  • tools/memory-passthrough.ts
✅ Files skipped from review due to trivial changes (8)
  • index.ts
  • tools/message-search.ts
  • hooks/gateway.ts
  • tools/session.ts
  • hooks/capture.ts
  • hooks/context.ts
  • config.ts
  • tools/search.ts

Comment thread state.ts
r0c1n4nte Bot added 2 commits May 23, 2026 20:30
…mat, clamp searchTopK

- Invalidate participantPeerIdCache after every participantPeers.set() so
  isParticipantPeerId() does not return stale results
- Use base64url SHA-256 fallback in state.ts to match peers.ts format
- Clamp CLI search topK to max 100 per Honcho schema bound
The function required both backend === "qmd" AND a truthy mem.qmd
sub-object, but a minimal config like "memory": {"backend": "qmd"}
(without an explicit qmd block) broke the entire QMD bridge.
All downstream qmd helper functions use optional chaining and handle
undefined sub-objects fine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant