Add Claude Code channel plugin prototype#721
Open
futurepaul wants to merge 11 commits intosledtools:masterfrom
Open
Add Claude Code channel plugin prototype#721futurepaul wants to merge 11 commits intosledtools:masterfrom
futurepaul wants to merge 11 commits intosledtools:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new pikachat-claude Claude Code channel plugin: manifests and docs, config resolution, daemon install/launch/client and protocol, channel runtime with access control (pairing/allowlist), message-format helpers, MCP tool wiring, tests (unit and e2e), and minor .gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as External User
participant Daemon as Pikachat Daemon
participant Channel as PikachatClaudeChannel
participant Access as Access Control
participant MCP as MCP Server/Claude
User->>Daemon: send message to bot (DM/group)
Daemon->>Channel: message_received (content, sender, attachments)
activate Channel
Channel->>Channel: ignore if from bot
Channel->>Channel: determine chat_type (direct vs group)
Channel->>Access: evaluateDmAccess()/evaluateGroupAccess()
activate Access
Access-->>Channel: allowed / pairing / blocked / requireMention
deactivate Access
alt pairing required
Channel->>Daemon: sendMessage("Pairing code: <code>")
else allowed (and mention satisfied)
Channel->>Channel: augment message (attachments/meta), detect mention
Channel->>MCP: onNotification(ChannelNotification)
end
deactivate Channel
sequenceDiagram
participant Admin as Admin/API Caller
participant Channel as PikachatClaudeChannel
participant Access as Access Control
participant Daemon as Pikachat Daemon
Admin->>Channel: approvePairing(code)
activate Channel
Channel->>Access: approvePairing(state, code)
activate Access
Access-->>Channel: updated AccessState (sender added)
deactivate Access
Channel->>Channel: saveAccessState(accessFile)
Channel->>Daemon: (subsequent DM processed)
deactivate Channel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
📝 Coding Plan
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
futurepaul
force-pushed
the
feat/claude-channel-plugin
branch
from
7d38903 to
be53a3e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Trust Claude Code's own permission model for file access control rather than restricting outbound attachments to the project root. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines
+371
to
+375
| if (directAccess.decision === "pairing" && directAccess.pairingCode) { | ||
| await this.#daemon!.sendMessage( | ||
| chatId, | ||
| `Pairing code: ${directAccess.pairingCode}\nApprove it from Claude with the approve_pairing tool.`, | ||
| ); |
There was a problem hiding this comment.
🟡 Potential null dereference of this.#daemon after async gap in #handleInboundMessage
At channel-runtime.ts:372, this.#daemon!.sendMessage(...) uses a non-null assertion, but this.#daemon can become null between the initial guard at channel-runtime.ts:299 and this line. The intervening await calls at channel-runtime.ts:340 (#resolveChatType) and channel-runtime.ts:344 (#withAccessState) yield the event loop, during which stop() can be called — setting this.#daemon = null at channel-runtime.ts:170. When execution resumes, the ! assertion fails at runtime with a TypeError, preventing the pairing code from being sent. The error is caught by the upstream handler in daemon-client.ts:130-133, so it doesn't crash the process, but silently drops the pairing code reply.
Suggested change
| if (directAccess.decision === "pairing" && directAccess.pairingCode) { | |
| await this.#daemon!.sendMessage( | |
| chatId, | |
| `Pairing code: ${directAccess.pairingCode}\nApprove it from Claude with the approve_pairing tool.`, | |
| ); | |
| if (directAccess.decision === "pairing" && directAccess.pairingCode && this.#daemon) { | |
| await this.#daemon.sendMessage( | |
| chatId, | |
| `Pairing code: ${directAccess.pairingCode}\nApprove it from Claude with the approve_pairing tool.`, | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
pikachat-claude/src/daemon-client.ts (2)
193-205: Consider clearing the timeout when ready wins the race.The timeout timer continues running even after
#readyPromiseresolves. Whileunref()prevents process-blocking and the extrarejectcall is harmless on an already-settled promise, explicitly clearing the timer would avoid unnecessary resource allocation.♻️ Suggested improvement
async waitForReady(timeoutMs = 10_000): Promise<PikachatDaemonOutMsg & { type: "ready" }> { if (this.#closed) { throw new Error("daemon already closed"); } + let timeoutId: NodeJS.Timeout | undefined; const timeoutPromise = new Promise<never>((_resolve, reject) => { - const timer = setTimeout(() => reject(new Error("timeout waiting for daemon ready")), timeoutMs); - (timer as any).unref?.(); + timeoutId = setTimeout(() => reject(new Error("timeout waiting for daemon ready")), timeoutMs); + (timeoutId as any).unref?.(); }); const exitPromise = once(this.#proc, "exit").then(() => { throw new Error("daemon exited before ready"); }); - return await Promise.race([this.#readyPromise, timeoutPromise, exitPromise]); + try { + return await Promise.race([this.#readyPromise, timeoutPromise, exitPromise]); + } finally { + if (timeoutId) clearTimeout(timeoutId); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pikachat-claude/src/daemon-client.ts` around lines 193 - 205, In waitForReady, the timeout timer created inside timeoutPromise isn't cleared when this.#readyPromise or exitPromise wins the race; store the timer in an outer-scoped variable (e.g., let timer: NodeJS.Timeout | undefined), create the timeout with timer = setTimeout(...), and ensure you clear it (clearTimeout(timer)) when any winning promise resolves/rejects — e.g., attach a finally or a .then/.catch on Promise.race([this.#readyPromise, timeoutPromise, exitPromise]) or clear the timer in both the `#readyPromise` and exitPromise handlers so the timer is cleaned up; keep the unref() call if desired.
230-252: Consider stronger typing instead ofas anyassertions.Multiple methods use
as anywhen constructing command objects (lines 231, 235, 243, 251). SincePikachatDaemonInCmdis a discriminated union with all variants defined, you could leverage TypeScript's type narrowing instead.♻️ Example for one method
async publishKeypackage(relays: string[]): Promise<void> { - await this.request({ cmd: "publish_keypackage", relays } as any); + await this.request({ cmd: "publish_keypackage", relays }); }The
request()signature already acceptsOmit<PikachatDaemonInCmd, "request_id">, so explicitas anyshouldn't be needed if the object literal matches a union variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pikachat-claude/src/daemon-client.ts` around lines 230 - 252, The command objects in publishKeypackage, setRelays, acceptWelcome, and listMembers are using `as any`; instead construct them to match the discriminated union PikachatDaemonInCmd (or use the exact Omit<PikachatDaemonInCmd, "request_id"> type) so no `as any` is needed. Update publishKeypackage to call this.request({ cmd: "publish_keypackage", relays }) with the relays property typed to match the union variant; update setRelays similarly for { cmd: "set_relays", relays }; update acceptWelcome to use { cmd: "accept_welcome", wrapper_event_id: wrapperEventId } matching the wrapper_event_id field; and update listMembers to use { cmd: "list_members", nostr_group_id: nostrGroupId } matching that variant. Ensure the request() call signature (which accepts Omit<PikachatDaemonInCmd, "request_id">) is satisfied so the `as any` casts can be removed.pikachat-claude/src/daemon-install.ts (1)
28-31: Avoid hardcoding the plugin version here.
getPackageVersion()always returns"0.1.0". That now has to stay in sync with the MCP server version inpikachat-claude/src/server.ts, Lines 14-16, and with the package manifest on every release, or daemon compatibility checks drift silently. Pull this from shared package metadata or inject it at build time instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pikachat-claude/src/daemon-install.ts` around lines 28 - 31, getPackageVersion currently returns a hardcoded "0.1.0" (cached in pluginVersionCache); change it to read the canonical package version instead (e.g., import or require the package manifest's version or use a build-time injected VERSION constant) so the daemon's version stays in sync with the MCP server and package.json; update getPackageVersion and its use sites (pluginVersionCache, getPackageVersion) to fetch and cache the real version value rather than a literal string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pikachat-claude/src/access.ts`:
- Around line 154-166: The insertion into state.pendingPairings uses a freshly
generated code (via randomBytes(3).toString("hex")) without checking for
existing keys, so rare collisions can overwrite an earlier PendingPairing;
change the logic in the function that builds the PendingPairing (the block using
randomBytes, code, and pendingPairings) to loop: generate a code, check if
state.pendingPairings[code] is undefined, and only break/assign when you have a
unique code (also consider increasing the entropy from randomBytes(3) to e.g.
randomBytes(4) or more); then create the PendingPairing and write it to
pendingPairings[code] as before.
- Around line 47-93: loadAccessState currently catches all exceptions and
returns defaultAccessState, which wipes real config on parse/I/O failures;
change the catch to only return defaultAccessState when the error is a
missing-file ENOENT. In the catch block for loadAccessState, inspect the caught
error (e.g., as NodeJS.ErrnoException) and if err?.code === "ENOENT" return
defaultAccessState(); otherwise rethrow the error so parse or other I/O errors
propagate to the caller (preserving JSON.parse failures and transient read
errors).
In `@pikachat-claude/src/config.ts`:
- Around line 69-84: The path-like overrides (stateDir, daemonCmd,
daemonAcpExec, daemonAcpCwd) must be normalized the same way as channelHome:
wrap their trimmed env values with expandTilde(...) and then path.resolve(...)
(and only do this when the env var is present/non-empty) so "~" is expanded and
you return an absolute path; for daemonCmd make sure you pick the existing
trimmed value from PIKACHAT_DAEMON_CMD or PIKACHAT_SIDECAR_CMD before
expanding/resolving, and apply the same pattern when assigning stateDir,
daemonAcpExec, and daemonAcpCwd in the configuration object in config.ts.
In `@pikachat-claude/src/daemon-install.ts`:
- Around line 204-207: The downloadToFile flow currently uses a fixed tmpPath
(`${destination}.tmp`) which allows concurrent installs to clobber each other;
change it to generate a unique temp filename (e.g., include process.pid,
Date.now(), or a crypto-random suffix) when creating `tmpPath` in the same
function where `buffer = Buffer.from(await response.arrayBuffer())`, write to
that unique `tmpPath` via `writeFile`, then `rename` it to `destination`; also
ensure any thrown errors trigger cleanup (unlink) of the unique temp file before
rethrowing to avoid orphaned temp files.
In `@pikachat-claude/src/message-format.ts`:
- Line 1: The MediaLike type is used in the exported function augmentMessageText
but isn't exported, preventing external callers from typing media arrays; export
the type by adding the export keyword to the MediaLike declaration (i.e., make
MediaLike an exported type) so consumers can import MediaLike when using
augmentMessageText.
---
Nitpick comments:
In `@pikachat-claude/src/daemon-client.ts`:
- Around line 193-205: In waitForReady, the timeout timer created inside
timeoutPromise isn't cleared when this.#readyPromise or exitPromise wins the
race; store the timer in an outer-scoped variable (e.g., let timer:
NodeJS.Timeout | undefined), create the timeout with timer = setTimeout(...),
and ensure you clear it (clearTimeout(timer)) when any winning promise
resolves/rejects — e.g., attach a finally or a .then/.catch on
Promise.race([this.#readyPromise, timeoutPromise, exitPromise]) or clear the
timer in both the `#readyPromise` and exitPromise handlers so the timer is cleaned
up; keep the unref() call if desired.
- Around line 230-252: The command objects in publishKeypackage, setRelays,
acceptWelcome, and listMembers are using `as any`; instead construct them to
match the discriminated union PikachatDaemonInCmd (or use the exact
Omit<PikachatDaemonInCmd, "request_id"> type) so no `as any` is needed. Update
publishKeypackage to call this.request({ cmd: "publish_keypackage", relays })
with the relays property typed to match the union variant; update setRelays
similarly for { cmd: "set_relays", relays }; update acceptWelcome to use { cmd:
"accept_welcome", wrapper_event_id: wrapperEventId } matching the
wrapper_event_id field; and update listMembers to use { cmd: "list_members",
nostr_group_id: nostrGroupId } matching that variant. Ensure the request() call
signature (which accepts Omit<PikachatDaemonInCmd, "request_id">) is satisfied
so the `as any` casts can be removed.
In `@pikachat-claude/src/daemon-install.ts`:
- Around line 28-31: getPackageVersion currently returns a hardcoded "0.1.0"
(cached in pluginVersionCache); change it to read the canonical package version
instead (e.g., import or require the package manifest's version or use a
build-time injected VERSION constant) so the daemon's version stays in sync with
the MCP server and package.json; update getPackageVersion and its use sites
(pluginVersionCache, getPackageVersion) to fetch and cache the real version
value rather than a literal string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af8aa57e-9576-407f-9c9f-ef5a3c847136
📒 Files selected for processing (22)
.gitignoredocs/claude-channel-plugin-brief.mdpikachat-claude/.claude-plugin/plugin.jsonpikachat-claude/.mcp.jsonpikachat-claude/README.mdpikachat-claude/package.jsonpikachat-claude/src/access.test.tspikachat-claude/src/access.tspikachat-claude/src/channel-runtime.test.tspikachat-claude/src/channel-runtime.tspikachat-claude/src/config.test.tspikachat-claude/src/config.tspikachat-claude/src/daemon-client.test.tspikachat-claude/src/daemon-client.tspikachat-claude/src/daemon-install.tspikachat-claude/src/daemon-launch.tspikachat-claude/src/daemon-protocol.tspikachat-claude/src/local-relay-e2e.test.tspikachat-claude/src/message-format.test.tspikachat-claude/src/message-format.tspikachat-claude/src/server.tspikachat-claude/tsconfig.json
✅ Files skipped from review due to trivial changes (10)
- .gitignore
- pikachat-claude/.mcp.json
- pikachat-claude/.claude-plugin/plugin.json
- pikachat-claude/tsconfig.json
- pikachat-claude/package.json
- pikachat-claude/src/access.test.ts
- docs/claude-channel-plugin-brief.md
- pikachat-claude/README.md
- pikachat-claude/src/message-format.test.ts
- pikachat-claude/src/daemon-protocol.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- pikachat-claude/src/config.test.ts
- pikachat-claude/src/local-relay-e2e.test.ts
- pikachat-claude/src/channel-runtime.test.ts
- pikachat-claude/src/daemon-launch.ts
- pikachat-claude/src/channel-runtime.ts
Comment on lines
+47
to
+93
| export async function loadAccessState(filePath: string): Promise<AccessState> { | ||
| try { | ||
| const raw = await readFile(filePath, "utf8"); | ||
| const parsed = JSON.parse(raw) as Partial<AccessState>; | ||
| return { | ||
| dmPolicy: | ||
| parsed.dmPolicy === "allowlist" || parsed.dmPolicy === "disabled" | ||
| ? parsed.dmPolicy | ||
| : "pairing", | ||
| allowFrom: Array.isArray(parsed.allowFrom) | ||
| ? parsed.allowFrom.map((entry) => normalizeSenderId(String(entry))).filter(Boolean) | ||
| : [], | ||
| groups: Object.fromEntries( | ||
| Object.entries(parsed.groups ?? {}).map(([groupId, rawGroup]) => { | ||
| const group = rawGroup as Partial<GroupAccess> | undefined; | ||
| return [ | ||
| normalizeGroupId(groupId), | ||
| { | ||
| requireMention: group?.requireMention !== false, | ||
| allowFrom: Array.isArray(group?.allowFrom) | ||
| ? group!.allowFrom.map((entry) => normalizeSenderId(String(entry))).filter(Boolean) | ||
| : [], | ||
| }, | ||
| ]; | ||
| }), | ||
| ), | ||
| mentionPatterns: Array.isArray(parsed.mentionPatterns) | ||
| ? parsed.mentionPatterns.map((entry) => String(entry)).filter(Boolean) | ||
| : [], | ||
| pendingPairings: Object.fromEntries( | ||
| Object.entries(parsed.pendingPairings ?? {}).map(([code, pending]) => { | ||
| const entry = pending as Partial<PendingPairing> | undefined; | ||
| return [ | ||
| code.trim().toLowerCase(), | ||
| { | ||
| code: code.trim().toLowerCase(), | ||
| senderId: normalizeSenderId(String(entry?.senderId ?? "")), | ||
| chatId: normalizeGroupId(String(entry?.chatId ?? "")), | ||
| createdAt: Number(entry?.createdAt ?? 0), | ||
| }, | ||
| ]; | ||
| }), | ||
| ), | ||
| }; | ||
| } catch { | ||
| return defaultAccessState(); | ||
| } |
There was a problem hiding this comment.
Don't treat parse or I/O failures as "no access state".
loadAccessState() falls back to defaultAccessState() for every exception, not just first-run ENOENT. If access.json is truncated or temporarily unreadable, PikachatClaudeChannel.#withAccessState() in pikachat-claude/src/channel-runtime.ts, Lines 433-445, will load the default state and persist it on the next mutation, silently wiping the real allowlist/group config. Only missing-file should default.
🛠️ Suggested fix
export async function loadAccessState(filePath: string): Promise<AccessState> {
try {
const raw = await readFile(filePath, "utf8");
const parsed = JSON.parse(raw) as Partial<AccessState>;
return {
@@
),
};
- } catch {
- return defaultAccessState();
+ } catch (err) {
+ if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+ return defaultAccessState();
+ }
+ throw err;
}
}📝 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
| export async function loadAccessState(filePath: string): Promise<AccessState> { | |
| try { | |
| const raw = await readFile(filePath, "utf8"); | |
| const parsed = JSON.parse(raw) as Partial<AccessState>; | |
| return { | |
| dmPolicy: | |
| parsed.dmPolicy === "allowlist" || parsed.dmPolicy === "disabled" | |
| ? parsed.dmPolicy | |
| : "pairing", | |
| allowFrom: Array.isArray(parsed.allowFrom) | |
| ? parsed.allowFrom.map((entry) => normalizeSenderId(String(entry))).filter(Boolean) | |
| : [], | |
| groups: Object.fromEntries( | |
| Object.entries(parsed.groups ?? {}).map(([groupId, rawGroup]) => { | |
| const group = rawGroup as Partial<GroupAccess> | undefined; | |
| return [ | |
| normalizeGroupId(groupId), | |
| { | |
| requireMention: group?.requireMention !== false, | |
| allowFrom: Array.isArray(group?.allowFrom) | |
| ? group!.allowFrom.map((entry) => normalizeSenderId(String(entry))).filter(Boolean) | |
| : [], | |
| }, | |
| ]; | |
| }), | |
| ), | |
| mentionPatterns: Array.isArray(parsed.mentionPatterns) | |
| ? parsed.mentionPatterns.map((entry) => String(entry)).filter(Boolean) | |
| : [], | |
| pendingPairings: Object.fromEntries( | |
| Object.entries(parsed.pendingPairings ?? {}).map(([code, pending]) => { | |
| const entry = pending as Partial<PendingPairing> | undefined; | |
| return [ | |
| code.trim().toLowerCase(), | |
| { | |
| code: code.trim().toLowerCase(), | |
| senderId: normalizeSenderId(String(entry?.senderId ?? "")), | |
| chatId: normalizeGroupId(String(entry?.chatId ?? "")), | |
| createdAt: Number(entry?.createdAt ?? 0), | |
| }, | |
| ]; | |
| }), | |
| ), | |
| }; | |
| } catch { | |
| return defaultAccessState(); | |
| } | |
| export async function loadAccessState(filePath: string): Promise<AccessState> { | |
| try { | |
| const raw = await readFile(filePath, "utf8"); | |
| const parsed = JSON.parse(raw) as Partial<AccessState>; | |
| return { | |
| dmPolicy: | |
| parsed.dmPolicy === "allowlist" || parsed.dmPolicy === "disabled" | |
| ? parsed.dmPolicy | |
| : "pairing", | |
| allowFrom: Array.isArray(parsed.allowFrom) | |
| ? parsed.allowFrom.map((entry) => normalizeSenderId(String(entry))).filter(Boolean) | |
| : [], | |
| groups: Object.fromEntries( | |
| Object.entries(parsed.groups ?? {}).map(([groupId, rawGroup]) => { | |
| const group = rawGroup as Partial<GroupAccess> | undefined; | |
| return [ | |
| normalizeGroupId(groupId), | |
| { | |
| requireMention: group?.requireMention !== false, | |
| allowFrom: Array.isArray(group?.allowFrom) | |
| ? group!.allowFrom.map((entry) => normalizeSenderId(String(entry))).filter(Boolean) | |
| : [], | |
| }, | |
| ]; | |
| }), | |
| ), | |
| mentionPatterns: Array.isArray(parsed.mentionPatterns) | |
| ? parsed.mentionPatterns.map((entry) => String(entry)).filter(Boolean) | |
| : [], | |
| pendingPairings: Object.fromEntries( | |
| Object.entries(parsed.pendingPairings ?? {}).map(([code, pending]) => { | |
| const entry = pending as Partial<PendingPairing> | undefined; | |
| return [ | |
| code.trim().toLowerCase(), | |
| { | |
| code: code.trim().toLowerCase(), | |
| senderId: normalizeSenderId(String(entry?.senderId ?? "")), | |
| chatId: normalizeGroupId(String(entry?.chatId ?? "")), | |
| createdAt: Number(entry?.createdAt ?? 0), | |
| }, | |
| ]; | |
| }), | |
| ), | |
| }; | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === "ENOENT") { | |
| return defaultAccessState(); | |
| } | |
| throw err; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pikachat-claude/src/access.ts` around lines 47 - 93, loadAccessState
currently catches all exceptions and returns defaultAccessState, which wipes
real config on parse/I/O failures; change the catch to only return
defaultAccessState when the error is a missing-file ENOENT. In the catch block
for loadAccessState, inspect the caught error (e.g., as NodeJS.ErrnoException)
and if err?.code === "ENOENT" return defaultAccessState(); otherwise rethrow the
error so parse or other I/O errors propagate to the caller (preserving
JSON.parse failures and transient read errors).
Comment on lines
+154
to
+166
| const code = randomBytes(3).toString("hex"); | ||
| const pairing: PendingPairing = { | ||
| code, | ||
| senderId: normalizedSenderId, | ||
| chatId: normalizedChatId, | ||
| createdAt: now, | ||
| }; | ||
| return { | ||
| state: { | ||
| ...state, | ||
| pendingPairings: { | ||
| ...state.pendingPairings, | ||
| [code]: pairing, |
There was a problem hiding this comment.
Retry on pairing-code collisions before writing the new entry.
The new code is inserted straight into pendingPairings[code]. A rare randomBytes(3) collision overwrites the previous pending request, so approving the older code can authorize the wrong sender. Loop until the generated code is unused, and consider a slightly longer token.
🛠️ Suggested fix
- const code = randomBytes(3).toString("hex");
+ let code: string;
+ do {
+ code = randomBytes(4).toString("hex");
+ } while (state.pendingPairings[code]);
const pairing: PendingPairing = {
code,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pikachat-claude/src/access.ts` around lines 154 - 166, The insertion into
state.pendingPairings uses a freshly generated code (via
randomBytes(3).toString("hex")) without checking for existing keys, so rare
collisions can overwrite an earlier PendingPairing; change the logic in the
function that builds the PendingPairing (the block using randomBytes, code, and
pendingPairings) to loop: generate a code, check if state.pendingPairings[code]
is undefined, and only break/assign when you have a unique code (also consider
increasing the entropy from randomBytes(3) to e.g. randomBytes(4) or more); then
create the PendingPairing and write it to pendingPairings[code] as before.
Comment on lines
+69
to
+84
| const channelHome = path.resolve( | ||
| expandTilde(env.PIKACHAT_CLAUDE_HOME?.trim() || defaultClaudeChannelHome()), | ||
| ); | ||
| const configuredRelays = parseStringArray(env.PIKACHAT_RELAYS); | ||
| return { | ||
| relays: configuredRelays.length > 0 ? configuredRelays : defaultPikachatRelays(), | ||
| stateDir: env.PIKACHAT_STATE_DIR?.trim() || undefined, | ||
| daemonCmd: env.PIKACHAT_DAEMON_CMD?.trim() || env.PIKACHAT_SIDECAR_CMD?.trim() || undefined, | ||
| daemonArgs: (() => { | ||
| const values = parseStringArray(env.PIKACHAT_DAEMON_ARGS?.trim() || env.PIKACHAT_SIDECAR_ARGS?.trim()); | ||
| return values.length > 0 ? values : undefined; | ||
| })(), | ||
| daemonVersion: env.PIKACHAT_DAEMON_VERSION?.trim() || env.PIKACHAT_SIDECAR_VERSION?.trim() || undefined, | ||
| daemonBackend: env.PIKACHAT_DAEMON_BACKEND === "acp" ? "acp" : "native", | ||
| daemonAcpExec: env.PIKACHAT_DAEMON_ACP_EXEC?.trim() || undefined, | ||
| daemonAcpCwd: env.PIKACHAT_DAEMON_ACP_CWD?.trim() || undefined, |
There was a problem hiding this comment.
Expand ~ for the other path-like env vars too.
PIKACHAT_CLAUDE_HOME goes through expandTilde(), but stateDir, daemonCmd, daemonAcpExec, and daemonAcpCwd are left verbatim. resolveExistingCommand() in pikachat-claude/src/daemon-install.ts, Lines 66-73, later does path.resolve(trimmed) on daemonCmd, so ~/bin/pikachat becomes a literal path and won't resolve. Please normalize the other path overrides the same way.
🛠️ Suggested fix
export function resolvePikachatClaudeConfig(env: NodeJS.ProcessEnv = process.env): PikachatClaudeConfig {
+ const stateDir = env.PIKACHAT_STATE_DIR?.trim();
+ const daemonCmd = env.PIKACHAT_DAEMON_CMD?.trim() || env.PIKACHAT_SIDECAR_CMD?.trim();
+ const daemonAcpExec = env.PIKACHAT_DAEMON_ACP_EXEC?.trim();
+ const daemonAcpCwd = env.PIKACHAT_DAEMON_ACP_CWD?.trim();
const channelHome = path.resolve(
expandTilde(env.PIKACHAT_CLAUDE_HOME?.trim() || defaultClaudeChannelHome()),
);
const configuredRelays = parseStringArray(env.PIKACHAT_RELAYS);
return {
relays: configuredRelays.length > 0 ? configuredRelays : defaultPikachatRelays(),
- stateDir: env.PIKACHAT_STATE_DIR?.trim() || undefined,
- daemonCmd: env.PIKACHAT_DAEMON_CMD?.trim() || env.PIKACHAT_SIDECAR_CMD?.trim() || undefined,
+ stateDir: stateDir ? expandTilde(stateDir) : undefined,
+ daemonCmd: daemonCmd ? expandTilde(daemonCmd) : undefined,
daemonArgs: (() => {
const values = parseStringArray(env.PIKACHAT_DAEMON_ARGS?.trim() || env.PIKACHAT_SIDECAR_ARGS?.trim());
return values.length > 0 ? values : undefined;
})(),
daemonVersion: env.PIKACHAT_DAEMON_VERSION?.trim() || env.PIKACHAT_SIDECAR_VERSION?.trim() || undefined,
daemonBackend: env.PIKACHAT_DAEMON_BACKEND === "acp" ? "acp" : "native",
- daemonAcpExec: env.PIKACHAT_DAEMON_ACP_EXEC?.trim() || undefined,
- daemonAcpCwd: env.PIKACHAT_DAEMON_ACP_CWD?.trim() || undefined,
+ daemonAcpExec: daemonAcpExec ? expandTilde(daemonAcpExec) : undefined,
+ daemonAcpCwd: daemonAcpCwd ? expandTilde(daemonAcpCwd) : undefined,
autoAcceptWelcomes: parseBoolean(env.PIKACHAT_AUTO_ACCEPT_WELCOMES, true),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pikachat-claude/src/config.ts` around lines 69 - 84, The path-like overrides
(stateDir, daemonCmd, daemonAcpExec, daemonAcpCwd) must be normalized the same
way as channelHome: wrap their trimmed env values with expandTilde(...) and then
path.resolve(...) (and only do this when the env var is present/non-empty) so
"~" is expanded and you return an absolute path; for daemonCmd make sure you
pick the existing trimmed value from PIKACHAT_DAEMON_CMD or PIKACHAT_SIDECAR_CMD
before expanding/resolving, and apply the same pattern when assigning stateDir,
daemonAcpExec, and daemonAcpCwd in the configuration object in config.ts.
Comment on lines
+135
to
+139
| const response = await fetch(releasesListApiUrl(params.repo, page), { headers }); | ||
| if (!response.ok) { | ||
| const body = await response.text().catch(() => ""); | ||
| throw new Error(`release list lookup failed ${response.status}: ${body.slice(0, 200)}`); | ||
| } |
There was a problem hiding this comment.
Bound the GitHub calls with a timeout.
These requests are all on the startup/install path and currently rely on unbounded fetch. A stalled connection will hang channel startup indefinitely. Please add an abort/timeout to the release lookups and asset download, and surface timeout failures explicitly.
Also applies to: 157-160, 199-203
Comment on lines
+204
to
+207
| const tmpPath = `${destination}.tmp`; | ||
| const buffer = Buffer.from(await response.arrayBuffer()); | ||
| await writeFile(tmpPath, buffer); | ||
| await rename(tmpPath, destination); |
There was a problem hiding this comment.
Use a unique temp path for concurrent installs.
downloadToFile() always writes to ${destination}.tmp. Two plugin processes installing the same version can clobber that file, so one rename removes the path while the other is still writing or renaming. Give each attempt its own temp name and clean it up on failure.
🛠️ Suggested fix
async function downloadToFile(url: string, destination: string): Promise<void> {
const response = await fetch(url, { headers: githubHeaders(), redirect: "follow" });
if (!response.ok || !response.body) {
throw new Error(`download failed ${response.status}: ${url}`);
}
- const tmpPath = `${destination}.tmp`;
- const buffer = Buffer.from(await response.arrayBuffer());
- await writeFile(tmpPath, buffer);
- await rename(tmpPath, destination);
+ const tmpPath = `${destination}.${process.pid}.${Date.now()}.${Math.random().toString(16).slice(2)}.tmp`;
+ try {
+ const buffer = Buffer.from(await response.arrayBuffer());
+ await writeFile(tmpPath, buffer);
+ await rename(tmpPath, destination);
+ } finally {
+ await rm(tmpPath, { force: true }).catch(() => {});
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pikachat-claude/src/daemon-install.ts` around lines 204 - 207, The
downloadToFile flow currently uses a fixed tmpPath (`${destination}.tmp`) which
allows concurrent installs to clobber each other; change it to generate a unique
temp filename (e.g., include process.pid, Date.now(), or a crypto-random suffix)
when creating `tmpPath` in the same function where `buffer = Buffer.from(await
response.arrayBuffer())`, write to that unique `tmpPath` via `writeFile`, then
`rename` it to `destination`; also ensure any thrown errors trigger cleanup
(unlink) of the unique temp file before rethrowing to avoid orphaned temp files.
| @@ -0,0 +1,63 @@ | |||
| type MediaLike = { | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MediaLike is imported/referenced elsewhere in the codebase
rg -n "MediaLike" --type=tsRepository: sledtools/pika
Length of output: 244
Export MediaLike type to support the public API of augmentMessageText.
The MediaLike type is used as a parameter in the exported augmentMessageText function but is not exported itself. External callers of this function cannot properly type their media arrays without access to the MediaLike type definition.
Add export to line 1:
export type MediaLike = {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pikachat-claude/src/message-format.ts` at line 1, The MediaLike type is used
in the exported function augmentMessageText but isn't exported, preventing
external callers from typing media arrays; export the type by adding the export
keyword to the MediaLike declaration (i.e., make MediaLike an exported type) so
consumers can import MediaLike when using augmentMessageText.
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
pikachat-claudepackage that exposes a Claude Code channel plugin backed bypikachat daemondocs/claude-channel-plugin-brief.mdTesting
Notes
pikachat-openclawSummary by CodeRabbit
New Features
Access
Documentation
Tests
Chores