fix: capture channel delta messages#103
Open
Seekerzero wants to merge 1 commit into
Open
Conversation
WalkthroughThe PR updates message handling and Honcho session management across the plugin. It introduces helper functions to unwrap OpenClaw-wrapped messages and extract metadata, updates message capture to handle delta batches with refined index tracking, improves turn-boundary detection in the context hook, refactors session retrieval to remove metadata overrides, and expands local host detection for self-hosted Honcho setups. ChangesMessage Capture & Session Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hooks/context.ts (1)
6-19: ⚡ Quick winReuse
getMessageRolehere to avoid wrapper-parsing drift.This reimplements message unwrapping/role detection that now exists in
helpers.ts. Using the shared helper keeps turn-boundary logic aligned with capture/extraction paths.Refactor sketch
-import { buildSessionKey, extractSenderId, isSubagentSession } from "../helpers.js"; +import { buildSessionKey, extractSenderId, getMessageRole, isSubagentSession } from "../helpers.js"; function resolveTurnStartIndex(messages: unknown[]): number { if (!Array.isArray(messages) || messages.length === 0) return 0; const lastIndex = messages.length - 1; - const rawLast = messages[lastIndex]; - const last = - rawLast && typeof rawLast === "object" && - "message" in rawLast && typeof (rawLast as { message?: unknown }).message === "object" - ? (rawLast as { message: unknown }).message - : rawLast; - if (last && typeof last === "object" && (last as { role?: unknown }).role === "user") { - return lastIndex; - } - return messages.length; + return getMessageRole(messages[lastIndex]) === "user" ? lastIndex : messages.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 `@hooks/context.ts` around lines 6 - 19, resolveTurnStartIndex reimplements message unwrapping and role detection; replace that logic by importing and using the shared helper getMessageRole (from helpers.ts) instead of manually unwrapping rawLast. Update resolveTurnStartIndex to call getMessageRole(rawLast) and compare the returned role to "user" to decide the turn start, keep the same return values (lastIndex or messages.length), and ensure proper typing/imports so compilation/type-narrowing still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helpers.ts`:
- Around line 385-387: The code currently treats any typeof timestamp ===
"number" as valid, which lets NaN/Infinity produce invalid Date objects; update
the guard around getMessageTimestamp(msg) so you only construct a Date when the
numeric timestamp is finite (e.g. use Number.isFinite(timestamp) or a similar
check) before creating ts and passing { createdAt: ts } into peer.message,
ensuring invalid numeric timestamps are rejected/handled instead of turned into
bad Date values.
In `@state.ts`:
- Around line 33-35: The octet parsing in the normalizedHost check currently
uses Number(part) which accepts non-decimal forms like "1e1" or "0x0a"; update
the logic around the octets variable so each part is first validated against a
decimal-digit regex (e.g., /^\d+$/) before numeric conversion, then parse with
parseInt(part, 10) and enforce the existing length/range checks (octets.length
=== 4 and each 0<=value<=255). Modify the mapping/validation that produces
octets in the function that uses normalizedHost to reject any non-digit octet
and only accept strictly decimal values.
In `@test/state.test.ts`:
- Around line 14-17: The test currently checks that public Honcho URLs are not
classified as self-hosted but misses non-decimal dotted host tokens; update the
test that uses isLocalHonchoBaseUrl to add two assertions ensuring that
"http://1e1.0.0.1" and "http://0x0a.0.0.1" both return false
(expect(...).toBe(false)), preventing numeric-coercion regressions in
isLocalHonchoBaseUrl.
---
Nitpick comments:
In `@hooks/context.ts`:
- Around line 6-19: resolveTurnStartIndex reimplements message unwrapping and
role detection; replace that logic by importing and using the shared helper
getMessageRole (from helpers.ts) instead of manually unwrapping rawLast. Update
resolveTurnStartIndex to call getMessageRole(rawLast) and compare the returned
role to "user" to decide the turn start, keep the same return values (lastIndex
or messages.length), and ensure proper typing/imports so
compilation/type-narrowing still works.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a191d3e5-2a07-41bb-8b80-0a48fc0f0295
📒 Files selected for processing (9)
config.tshelpers.tshooks/capture.tshooks/context.tsruntime.tsstate.tstest/capture.test.tstest/helpers.test.tstest/state.test.ts
Comment on lines
+385
to
387
| const timestamp = getMessageTimestamp(msg); | ||
| const ts = typeof timestamp === "number" ? new Date(timestamp) : undefined; | ||
| result.push(peer.message(content, ts ? { createdAt: ts } : undefined)); |
There was a problem hiding this comment.
Guard against invalid numeric timestamps before creating Date.
typeof timestamp === "number" also passes NaN/Infinity, which can produce invalid createdAt values downstream.
Suggested fix
- const ts = typeof timestamp === "number" ? new Date(timestamp) : undefined;
+ const ts =
+ typeof timestamp === "number" && Number.isFinite(timestamp)
+ ? new Date(timestamp)
+ : undefined;📝 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.
Suggested change
| const timestamp = getMessageTimestamp(msg); | |
| const ts = typeof timestamp === "number" ? new Date(timestamp) : undefined; | |
| result.push(peer.message(content, ts ? { createdAt: ts } : undefined)); | |
| const timestamp = getMessageTimestamp(msg); | |
| const ts = | |
| typeof timestamp === "number" && Number.isFinite(timestamp) | |
| ? new Date(timestamp) | |
| : undefined; | |
| result.push(peer.message(content, ts ? { createdAt: ts } : undefined)); |
🤖 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 `@helpers.ts` around lines 385 - 387, The code currently treats any typeof
timestamp === "number" as valid, which lets NaN/Infinity produce invalid Date
objects; update the guard around getMessageTimestamp(msg) so you only construct
a Date when the numeric timestamp is finite (e.g. use Number.isFinite(timestamp)
or a similar check) before creating ts and passing { createdAt: ts } into
peer.message, ensuring invalid numeric timestamps are rejected/handled instead
of turned into bad Date values.
Comment on lines
+33
to
+35
| const octets = normalizedHost.split(".").map((part) => Number(part)); | ||
| if (octets.length !== 4 || octets.some((part) => !Number.isInteger(part) || part < 0 || part > 255)) { | ||
| return false; |
There was a problem hiding this comment.
Require strict decimal IPv4 octets before numeric conversion.
On Line 33, using Number(part) allows non-decimal forms (e.g., 1e1, 0x0a) to pass validation, which can incorrectly classify non-IPv4 hostnames as local/private. Validate octets as digits first, then parse.
Proposed fix
- const octets = normalizedHost.split(".").map((part) => Number(part));
- if (octets.length !== 4 || octets.some((part) => !Number.isInteger(part) || part < 0 || part > 255)) {
+ const parts = normalizedHost.split(".");
+ if (parts.length !== 4 || parts.some((part) => !/^\d{1,3}$/.test(part))) {
+ return false;
+ }
+ const octets = parts.map((part) => Number.parseInt(part, 10));
+ if (octets.some((part) => part < 0 || part > 255)) {
return 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 `@state.ts` around lines 33 - 35, The octet parsing in the normalizedHost check
currently uses Number(part) which accepts non-decimal forms like "1e1" or
"0x0a"; update the logic around the octets variable so each part is first
validated against a decimal-digit regex (e.g., /^\d+$/) before numeric
conversion, then parse with parseInt(part, 10) and enforce the existing
length/range checks (octets.length === 4 and each 0<=value<=255). Modify the
mapping/validation that produces octets in the function that uses normalizedHost
to reject any non-digit octet and only accept strictly decimal values.
Comment on lines
+14
to
+17
| it("does not classify public Honcho URLs as self-hosted", () => { | ||
| expect(isLocalHonchoBaseUrl("https://api.honcho.dev")).toBe(false); | ||
| expect(isLocalHonchoBaseUrl("https://8.8.8.8")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add regression cases for non-decimal dotted host tokens.
Please add assertions that values like http://1e1.0.0.1 and http://0x0a.0.0.1 are false so local-host detection doesn’t regress through numeric coercion.
🤖 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/state.test.ts` around lines 14 - 17, The test currently checks that
public Honcho URLs are not classified as self-hosted but misses non-decimal
dotted host tokens; update the test that uses isLocalHonchoBaseUrl to add two
assertions ensuring that "http://1e1.0.0.1" and "http://0x0a.0.0.1" both return
false (expect(...).toBe(false)), preventing numeric-coercion regressions in
isLocalHonchoBaseUrl.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agent_endbatches as already sliced whenbefore_prompt_buildrecorded a full-transcript cursormessagetool calls while filtering channel delivery placeholders100.64.0.0/10self-host URLs as local Honcho deploymentsTesting
./node_modules/.bin/vitest --run./node_modules/.bin/tsc --noEmitSummary by CodeRabbit
Bug Fixes
Chores