Open
Conversation
|
|
github-actions
bot
added
the
size/L
|
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 Changes
Sequence Diagram(s)sequenceDiagram
participant Feishu as Feishu (Platform)
participant Subscriber as EventSubscriber
participant Handler as BotHandler/Router/SessionMgr
participant Claude as Claude CLI
participant Sender as MessageSender
Feishu->>Subscriber: WebSocket event (im.message.receive_v1)
Subscriber->>Handler: parse event, extract text
Handler->>Handler: lookup/create session (SessionManager)
Handler->>Claude: ProcessMessage (claude CLI)
Claude-->>Handler: JSON result (result, session_id)
Handler->>Handler: update session state
Handler->>Sender: send reply payload
Sender->>Feishu: create message (reply)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces a
Confidence Score: 3/5Not safe to merge — multiple P1 issues including a compile error, broken post-message parsing, incorrect subprocess working directory, and world-readable session files. Three confirmed P1 correctness/security bugs remain unresolved: session files are world-readable, the Claude subprocess working directory is never created or set, and Lark post-type message content is silently dropped due to wrong JSON traversal. Additionally subscribe.go references an undefined shortcuts/bot/handler.go (post parsing), shortcuts/bot/claude.go (workdir), shortcuts/bot/session.go (file permissions), shortcuts/bot/subscribe.go (compile error)
|
| Filename | Overview |
|---|---|
| shortcuts/bot/claude.go | Claude CLI wrapper with retry logic; workdir is never created and cmd.Dir is never set, so every invocation runs in the wrong directory with a non-existent --add-dir path |
| shortcuts/bot/handler.go | Message event parsing and Claude routing; post-type message content extraction always fails due to incorrect assumption about Lark's nested locale→content JSON structure |
| shortcuts/bot/session.go | TTL-based session persistence with atomic writes; directory and files use world-readable permissions (0755/0644) exposing session IDs to all OS users |
| shortcuts/bot/subscribe.go | WebSocket event subscription with graceful shutdown; references undefined atomic identifier (missing import, wrong field type) — compile error |
| shortcuts/bot/router.go | Command and pattern routing with whitelist support; /help handler still acquires a reentrant RLock — potential deadlock under write contention |
| shortcuts/bot/sender.go | Real Lark IM send/reply implementation wired up; passes empty WithTenantAccessToken("") which overrides SDK-managed auth on every API call |
| cmd/bot/start.go | Bot start command; properly creates BotStartOptions from flags, but --config value is captured yet LoadMultiAppConfig() is called without it |
| cmd/bot/bot.go | Top-level bot command wiring; sub-commands now correctly receive their own typed options structs |
Sequence Diagram
sequenceDiagram
participant U as Lark User
participant LWS as Lark WebSocket
participant ES as EventSubscriber
participant BH as BotHandler
participant CC as ClaudeClient
participant SM as SessionManager
participant MS as MessageSender
U->>LWS: Send message
LWS->>ES: im.message.receive_v1 event
ES->>BH: HandleMessage(event)
BH->>BH: parseMessageEvent()
BH->>SM: Get(chatID)
SM-->>BH: sessionData (or nil)
BH->>CC: ProcessMessage(content, sessionID)
CC->>CC: exec claude -p ... --output-format json
CC-->>BH: ClaudeResponse{Result, SessionID}
BH->>SM: Set(chatID, newSessionID)
BH-->>ES: response string
ES->>MS: SendMessage(chatID, response, messageID)
MS->>LWS: Im.V1.Message.Reply
LWS-->>U: Reply
Reviews (9): Last reviewed commit: "Update shortcuts/bot/subscribe.go" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
cmd/bot/start.go (1)
10-10: Consider usingvfs.*instead ofos.*for filesystem access.The coding guidelines specify using
vfs.*instead ofos.*for all filesystem access. While this file currently only usesos.Signal, future implementations involving config file reading should use the virtual filesystem abstraction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bot/start.go` at line 10, The file currently imports os (used for os.Signal) but the project guideline requires using the virtual filesystem API for file operations; keep os for signal handling (os.Signal) but switch any filesystem calls (e.g., os.Open, os.ReadFile, os.Stat, os.MkdirAll) to use the vfs package instead (e.g., vfs.Open, vfs.ReadFile, vfs.Stat, vfs.MkdirAll), add the vfs import, and update any future config/file access code to call the vfs functions rather than os.* while leaving os.Signal usage as-is.docs/CODEMAPS/architecture.md (1)
113-113: Tighten wording: use “CLI” instead of “CLI interface.”This removes redundancy and reads cleaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CODEMAPS/architecture.md` at line 113, Change the redundant phrase "CLI interface" to "CLI" in the responsibility line so it reads "Responsibility: CLI, command parsing, user interaction"; locate the exact string "Responsibility: CLI interface, command parsing, user interaction" in the architecture document and replace "CLI interface" with "CLI".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/bot/start.go`:
- Around line 80-82: The error returned for daemon mode in cmd/bot/start.go
currently uses errors.New("daemon 模式尚未实现"); update the RunE handler to return
output.Errorf (or output.ErrWithHint if a hint is appropriate) instead, e.g.,
replace the errors.New return in the daemon-mode branch with output.Errorf and
keep the same localized message (or add a short actionable hint via
output.ErrWithHint), ensuring the command's RunE follows the project's
cmd/**/*.go error-return convention.
- Line 57: The startup/shutdown banner and other progress messages are being
written to stdout via fmt.Fprintf(f.IOStreams.Out, ...) but per guidelines
progress/warnings must go to stderr; change those calls (e.g., the banner at
fmt.Fprintf(f.IOStreams.Out, "=== Claude Code Bot 启动中 ===\n") and the similar
prints around lines 74–76) to write to f.IOStreams.ErrOut (or use
fmt.Fprintf(f.IOStreams.ErrOut, ...)) so progress messages go to stderr while
keeping program output on stdout.
- Around line 28-43: Function signature and usage are wrong: newCmdBotStart
currently takes *BotOptions but accesses Ctx, Config, Daemon and calls
botStartRun which expect *BotStartOptions. Change newCmdBotStart to accept
*BotStartOptions (not *BotOptions), update the function parameter name if
needed, keep the flags binding to opts.Config and opts.Daemon and set opts.Ctx =
cmd.Context(), and call botStartRun(opts). Also update any callers that pass a
*BotOptions to instead construct or pass a *BotStartOptions so types align.
In `@cmd/bot/status.go`:
- Around line 22-34: newCmdBotStatus currently accepts *BotStatusOptions but is
called with *BotOptions (from bot.go), causing a type mismatch; update the API
so the types align by either changing newCmdBotStatus's parameter to accept
*BotOptions (and propagate needed fields to botStatusRun) or by constructing a
*BotStatusOptions from the provided *BotOptions before calling newCmdBotStatus;
locate the function newCmdBotStatus and the caller in bot.go and make the
minimal change to use the same option type (BotOptions -> newCmdBotStatus or
convert opts -> BotStatusOptions) and ensure botStatusRun still receives the
context/options it needs.
- Around line 37-56: In botStatusRun, remove or explicitly ignore the unused
variable ctx (currently set as ctx := opts.Ctx) by deleting it or replacing with
_ = opts.Ctx, and send the progress message to the error stream instead of
stdout by changing the fmt.Fprintf target from f.IOStreams.Out to
f.IOStreams.ErrOut so progress/warnings go to stderr while the JSON result
remains printed to f.IOStreams.Out via output.PrintJson; refer to the
botStatusRun function and the opts.Factory f / f.IOStreams.Out and
f.IOStreams.ErrOut symbols to locate where to apply these changes.
In `@cmd/bot/stop.go`:
- Around line 49-58: The command currently writes a progress banner to stdout
and emits a "not_implemented" JSON success envelope then returns nil; change
this so progress/hints go to stderr and the command fails with a structured JSON
error on stdout. Concretely: send the banner using f.IOStreams.Err (not
f.IOStreams.Out), remove the current success envelope, and instead create an
error payload (e.g., {"status":"error","error":"bot stop not
implemented","code":"not_implemented"}) and emit it via
output.PrintJson(f.IOStreams.Out, payload) then return a non-nil error (e.g.,
fmt.Errorf or the existing CLI error type) so the process exits non-zero;
reference f.IOStreams.Out, f.IOStreams.Err and output.PrintJson when making
these changes.
- Around line 40-42: Remove the unused ctx assignment that causes a "declared
but not used" compile error: delete the line "ctx := opts.Ctx" (or replace it
with an underscore if the context must be referenced) so only the used variable
"f := opts.Factory" remains; locate this in the function where opts is used and
adjust any subsequent references to use opts.Ctx directly if needed.
In `@cmd/bot/TEST.md`:
- Around line 47-50: Replace the hardcoded developer path and incorrect build
target in the test instructions: remove the absolute path
(/Users/rongchuanxie/...) and change the build invocation 'go build -o
/tmp/lark-cli ./cmd/root.go' to a portable command such as 'go build -o
/tmp/lark-cli .' (or 'go build -o ./bin/lark-cli .' if you prefer a
repo-relative output), updating the line that contains the exact string 'go
build -o /tmp/lark-cli ./cmd/root.go' in TEST.md.
In `@docs/bot-integration-plan.md`:
- Around line 48-60: The fenced code blocks in docs/bot-integration-plan.md (for
example the block beginning with "lark-cli bot start") lack language identifiers
which trips Markdownlint MD040; update each fence by appending an appropriate
language tag (e.g., ```bash for shell/CLI sequences like the "lark-cli bot
start" block, ```text for plain diagrams, ```go for Go snippets, ```yaml for
YAML snippets) so the blocks at the shown ranges (48-60 and also 64-78, 199-204,
217-219, 232-235, 299-308) and any other fenced blocks in the file all include
the correct language identifier after the opening ``` fence.
- Around line 360-365: The args slice currently unconditionally includes the
dangerous flag "--dangerously-skip-permissions" (see args variable and usage of
content and c.workDir) — make this flag opt-in by gating its inclusion behind an
explicit configuration (e.g., an environment variable like
DANGEROUSLY_SKIP_PERMISSIONS or a boolean field on the caller/config passed into
the method that builds args), so only when that opt-in is true you append
"--dangerously-skip-permissions"; also add a clear inline comment near the args
construction and update any public docs to warn about the security implications
and when it may be safely used.
In `@docs/CODEMAPS/backend.md`:
- Around line 108-113: The docs incorrectly claim RegisterShortcuts
auto-discovers shortcuts from subdirectories; update the documentation to
reflect the actual implementation: describe that RegisterShortcuts consumes the
allShortcuts collection, groups entries by service, and mounts grouped commands
onto the root command (referencing RegisterShortcuts and allShortcuts and the
grouping/mounting behavior) and remove the statement about directory-based
discovery.
- Around line 9-27: The fenced code block that starts with "lark-cli <command>
[subcommand] [flags]" is unlabeled and several tables in
docs/CODEMAPS/backend.md are missing surrounding blank lines, causing
markdownlint errors (MD040, MD058); fix by adding an appropriate language tag
(e.g., ```bash or ```text) to each unlabeled fenced block including the
"lark-cli..." block and the other occurrences referenced (around the sections
you flagged), and ensure every table is preceded and followed by a blank line so
tables are isolated from surrounding text (apply these edits to the other ranges
you listed: 84-98, 172-174, 192-208, 213-245, 305-310).
In `@README.bot.md`:
- Around line 124-138: The code block containing the ASCII architecture diagram
(starting with the line "飞书用户消息" and including "lark-cli event +subscribe
(WebSocket 长连接)") is missing a language specifier; update the opening
triple-backtick fence to include a plain-text specifier (e.g., ```text or
```plaintext) so the block is recognized as plain text in README.bot.md.
- Around line 80-89: The fenced chat example in README.bot.md is missing a
language specifier; update the triple-backtick block that contains the dialogue
lines ("你: 帮我写一个 Python 函数计算斐波那契数列" ... "/run tests") to use a plain-text
specifier (e.g., ```text) so static analysis recognizes it as plain text; ensure
every similar chat/example fenced block in the file uses `text` or `plaintext`
consistently.
---
Nitpick comments:
In `@cmd/bot/start.go`:
- Line 10: The file currently imports os (used for os.Signal) but the project
guideline requires using the virtual filesystem API for file operations; keep os
for signal handling (os.Signal) but switch any filesystem calls (e.g., os.Open,
os.ReadFile, os.Stat, os.MkdirAll) to use the vfs package instead (e.g.,
vfs.Open, vfs.ReadFile, vfs.Stat, vfs.MkdirAll), add the vfs import, and update
any future config/file access code to call the vfs functions rather than os.*
while leaving os.Signal usage as-is.
In `@docs/CODEMAPS/architecture.md`:
- Line 113: Change the redundant phrase "CLI interface" to "CLI" in the
responsibility line so it reads "Responsibility: CLI, command parsing, user
interaction"; locate the exact string "Responsibility: CLI interface, command
parsing, user interaction" in the architecture document and replace "CLI
interface" with "CLI".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6cc3fb5-a3c6-4e90-85c6-4e7db32233cc
📒 Files selected for processing (12)
.reports/codemap-diff.txtREADME.bot.mdcmd/bot/TEST.mdcmd/bot/bot.gocmd/bot/start.gocmd/bot/status.gocmd/bot/stop.gocmd/root.godocs/CODEMAPS/architecture.mddocs/CODEMAPS/backend.mddocs/CODEMAPS/dependencies.mddocs/bot-integration-plan.md
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
github-actions
bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
cmd/bot/start.go (1)
113-115:⚠️ Potential issue | 🟠 MajorUse
output.Errorfinstead oferrors.Newfor daemon mode error.Per coding guidelines,
RunEfunctions must returnoutput.Errorforoutput.ErrWithHint. The error message should also be structured and actionable.Proposed fix
// Daemon 模式：后台运行 // TODO: 实现 daemon 模式（fork 进程、PID 文件等） - return errors.New("daemon 模式尚未实现") + return output.Errorf(1, "not_implemented", "daemon mode is not yet implemented; run without --daemon flag")As per coding guidelines: "RunE functions in commands must return
output.Errorforoutput.ErrWithHint— never barefmt.Errorf"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bot/start.go` around lines 113 - 115, Replace the bare errors.New return in the daemon branch with the project's output error helper: in the RunE for the start command (the code returning errors.New("daemon 模式尚未实现")), return output.ErrWithHint or output.Errorf instead; for example use output.ErrWithHint("daemon 模式尚未实现", "implement daemon support (forking, PID file) or run without --daemon") or output.Errorf("daemon 模式尚未实现: %s", "implement forking and PID file"), so the error follows the RunE guideline and provides a structured, actionable hint.
🧹 Nitpick comments (4)
shortcuts/bot/handler.go (1)
80-84: Warning output usesfmt.Printf(stdout) instead of stderr.Per coding guidelines, warnings should go to stderr. However,
BotHandlerdoesn't have access toIOStreams. Consider either:
- Adding an optional logger/IOStreams to
BotHandlerConfig- Using
log.Printfwhich defaults to stderr- Returning a structured result that includes warnings
Proposed fix using log package
+import "log" + // Update session if _, err := h.sessionMgr.Set(msgEvent.ChatID, response.SessionID); err != nil { // Log error but don't fail the response - fmt.Printf("Warning: failed to save session: %v\n", err) + log.Printf("Warning: failed to save session: %v", err) }As per coding guidelines: "Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/handler.go` around lines 80 - 84, The warning currently prints to stdout via fmt.Printf in the BotHandler session save block; change it to write to stderr by replacing fmt.Printf with log.Printf (importing the log package) or, preferably, add an optional logger/IOStreams field to BotHandlerConfig and BotHandler and use that logger to emit the warning from the h.sessionMgr.Set error path (reference the h.sessionMgr.Set call and BotHandler/BotHandlerConfig types so you update the correct place), ensuring progress/JSON stays on stdout while warnings go to stderr.shortcuts/bot/router.go (2)
248-259: Consider usingsort.Sliceinstead of manual bubble sort.While the comment notes pattern count is typically small, using the standard library's
sort.Sliceis more idiomatic and equally efficient for small collections.Proposed fix
+import "sort" + // sortPatterns sorts patterns by priority (highest first) func (pr *PatternRouter) sortPatterns() { - // Simple bubble sort (pattern count is typically small) - n := len(pr.patterns) - for i := 0; i < n-1; i++ { - for j := 0; j < n-i-1; j++ { - if pr.patterns[j].Priority < pr.patterns[j+1].Priority { - pr.patterns[j], pr.patterns[j+1] = pr.patterns[j+1], pr.patterns[j] - } - } - } + sort.Slice(pr.patterns, func(i, j int) bool { + return pr.patterns[i].Priority > pr.patterns[j].Priority + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/router.go` around lines 248 - 259, Replace the manual bubble sort in PatternRouter.sortPatterns with the standard library sort.Slice to be more idiomatic: import sort, then call sort.Slice(pr.patterns, func(i, j int) bool { return pr.patterns[i].Priority > pr.patterns[j].Priority }) so patterns are sorted by Priority descending; ensure you reference the pr.patterns slice and the Priority field of the pattern struct in the comparator and remove the bubble sort loop.
173-176:/clearcommand is a stub that doesn't actually clear sessions.The
/clearcommand returns a success message but doesn't interact withSessionManagerto delete the session. This needs to be wired up to actually clear the session for the givenchatID.Would you like me to help implement the actual session clearing logic? This would require passing a
SessionManagerreference to the router or using a callback pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/router.go` around lines 173 - 176, The `/clear` handler registered via r.RegisterCommand currently only returns a message and must invoke the session manager to remove the session for chatID; modify the router to accept or have access to the SessionManager (e.g., add a sessionManager field on the router or pass it into the factory that sets up RegisterCommand) and update the handler to call the appropriate method (e.g., SessionManager.DeleteSession or SessionManager.ClearSession) with chatID, handle any returned error and return the success message only on success or an error otherwise; keep the handler signature (ctx, args, chatID) and ensure you reference the router's SessionManager inside the closure so the session is actually cleared.shortcuts/bot/session.go (1)
36-59: Consider usingvfs.*instead ofos.*for filesystem access.Per coding guidelines, use
vfs.*instead ofos.*for all filesystem access. This file usesos.UserHomeDir,os.MkdirAll,os.Stat,os.ReadFile,os.WriteFile,os.Rename,os.Remove, andos.ReadDirthroughout.If the
vfspackage provides equivalent functions, consider refactoring to use them for consistency and testability. This would also allow injecting a mock filesystem for unit testing.As per coding guidelines: "Use
vfs.*instead ofos.*for all filesystem access"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/session.go` around lines 36 - 59, The code in NewSessionManager (and related SessionManager file methods) uses os.* helpers (e.g., os.UserHomeDir, os.MkdirAll and other os.* calls noted in the review) instead of the project's vfs abstraction; refactor to use the vfs package equivalents by injecting or referencing a vfs filesystem into SessionManager (e.g., add a fs vfs.FS field or accept it via SessionManagerConfig), replace os.UserHomeDir/os.MkdirAll/etc. calls with vfs methods, and update constructors and any file operations (Stat/ReadFile/WriteFile/Rename/Remove/ReadDir) across SessionManager to use that vfs instance for testability and consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/bot/start.go`:
- Around line 57-59: Replace bare fmt.Errorf returns in the RunE handler with
the project output helper: for the ValidateClaudeCLI call (the err check around
bot.ValidateClaudeCLI(ctx)) and the other two error returns referenced (around
lines 68 and 88), change fmt.Errorf("...: %w", err) to output.Errorf("claude CLI
validation failed: %v", err) or output.ErrWithHint(...) if you want to add a
user hint; ensure you keep the original message text but use %v (or appropriate
formatting) instead of %w, and add the output package import if missing so the
RunE function returns output.Errorf/output.ErrWithHint rather than fmt.Errorf.
- Around line 97-98: The call to botHandler.GetStats(ctx) swallows errors and
may cause invalid JSON to be printed; change the code around GetStats to check
the returned error, and if non-nil handle it (e.g., log or print a clear error
to the user and exit with a non-zero status) instead of calling
output.PrintJson(io.Out, stats). Specifically, inspect the error from GetStats,
avoid calling output.PrintJson when err != nil, and use the existing
logging/output mechanism (or io.ErrOut) to surface the error so users aren’t
shown misleading JSON.
In `@shortcuts/bot/claude.go`:
- Around line 58-67: The backoff in the retry loop inside the code that iterates
"for attempt := 0; attempt < c.maxRetries; attempt++" is linear (waitTime :=
time.Duration(attempt) * time.Second) despite the "Exponential backoff" comment;
change waitTime calculation in that loop (the waitTime variable used before the
select) to use an exponential formula such as doubling per attempt (e.g.,
baseDelay * 2^attempt) and optionally clamp to a max delay to avoid unbounded
waits, updating the comment to match; keep the retry loop, ctx cancellation
handling, and time.After usage unchanged.
- Around line 142-148: The retry logic in ProcessMessage currently treats
"context deadline exceeded" as always retryable; change it to only retry when
the per-attempt timeout triggered the deadline, not when the parent context was
cancelled or timed out. Remove or special-case "context deadline exceeded" from
retryablePatterns and, where you handle errors after an attempt (in
ProcessMessage/attempt loop, the per-attempt context creation and error check
around the attempt), check the parent context with parentCtx.Err() (or
errors.Is(err, context.DeadlineExceeded) combined with parentCtx.Err()==nil)
before deciding to retry; if parentCtx.Err() != nil then abort and return the
error, otherwise allow retries for per-attempt deadline exceeded. Ensure you
reference the per-attempt context variable (the one created for each attempt)
and the parent ctx passed into ProcessMessage when implementing this check.
- Around line 12-13: Remove the unused import
"github.com/larksuite/cli/cmd/cmdutil" from shortcuts/bot/claude.go; locate the
import block in claude.go and delete that import entry (ensure any remaining
imports remain properly formatted) so the file no longer imports cmdutil when no
symbols from it are used.
In `@shortcuts/bot/handler.go`:
- Around line 17-19: The CreateTime field in the struct (fields MessageType and
CreateTime) is misaligned; fix the indentation so CreateTime lines up with
MessageType (same leading tabs/spaces) and keep its json tag
`json:"create_time"` unchanged so the struct formatting is consistent and Go
linting/formatting passes (update the struct definition containing MessageType
and CreateTime).
- Around line 183-196: The extractPostText function only flattens a single-level
[]interface{} but Lark posts are nested as {"post": {"zh_cn": {"content":
[[{...}]]}}}; update BotHandler.extractPostText to recursively walk the nested
structure: detect when an item is a map[string]interface{} containing language
keys (e.g., "zh_cn") or "content", handle nested []interface{} paragraphs and
segments, and when a segment is a map with a "text" string append it (preserving
newlines between paragraphs); ensure the function safely handles both the
existing single-level slices and the multi-level post.lang.content arrays by
recursing into slices and maps until all "text" values are collected.
In `@shortcuts/bot/router.go`:
- Around line 160-170: The /help handler deadlocks because Route holds
r.mu.RLock while invoking handlers and the help handler also acquires
r.mu.RLock; fix by changing Route so it only holds the lock to fetch the handler
(and any needed metadata) and then releases the lock before calling the handler:
in Route(), acquire r.mu.RLock(), look up the handler from r.commands into a
local variable, release the lock, then invoke the handler; keep the help handler
(registered in RegisterCommand "help") unchanged so it can safely acquire
r.mu.RLock when building the help text from r.commands.
- Around line 91-139: The Route method currently holds r.mu.RLock() for the
whole call, including during handler execution; change it to only hold the read
lock while you inspect state (resolve aliases, lookup handler and copy
r.defaultHandler), then release the read lock (call r.mu.RUnlock()) before
invoking any handler so handlers can call back into Router; specifically, in
Router.Route gather command, args, resolved target via r.aliases, and the
handler (from r.commands) and/or defaultHandler while under the read lock, then
unlock and call handler(ctx, args, chatID) or defaultHandler(ctx,
[]string{message}, chatID).
In `@shortcuts/bot/session.go`:
- Around line 62-91: Get deadlocks because List holds sm.mu.RLock and calls Get
which tries to RLock again; extract the core read logic into an unlocked helper
(e.g., add func (sm *SessionManager) getUnlocked(chatID string) (*SessionData,
error)) that performs sessionPath resolution, file Stat/Read, json.Unmarshal,
expiration check (calling sm.isExpired and sm.Delete as needed) but does NOT
acquire any locks, then change Get to acquire RLock/RUnlock and call
getUnlocked, and change List to call getUnlocked directly while holding its own
RLock; ensure names referenced: SessionManager, Get, List, getUnlocked,
isExpired, Delete, sessionPath.
- Around line 210-216: sessionPath currently only replaces '/' and '\' and can
still allow traversal via sequences like ".."; update SessionManager.sessionPath
to validate and sanitize the final filename using validate.SafeInputPath before
performing any file I/O: build the candidate filename (e.g., safeChatID +
".json"), call validate.SafeInputPath on that filename (or the joined path with
sm.baseDir as required by the validator), handle validation errors (return a
safe fallback or propagate the error appropriately), and then return
filepath.Join(sm.baseDir, validatedName) so all uses of sessionPath are
guaranteed safe.
- Around line 183-203: CleanupExpired currently holds sm.mu.Lock while calling
sm.List() and sm.Delete(), causing deadlocks because List uses RLock and Delete
uses Lock; fix by adding internal unlocked helpers listUnlocked and
deleteUnlocked that assume the caller holds the appropriate lock, then change
CleanupExpired to: obtain a read lock (or brief write lock) and call
listUnlocked to copy sessions, release the lock, iterate sessions and for each
expired acquire sm.mu.Lock, call deleteUnlocked(session.ChatID) and release the
lock (or batch deletes under one Lock if desired); reference the existing
methods isExpired, List, Delete and add listUnlocked and deleteUnlocked to avoid
calling exported methods while holding locks.
---
Duplicate comments:
In `@cmd/bot/start.go`:
- Around line 113-115: Replace the bare errors.New return in the daemon branch
with the project's output error helper: in the RunE for the start command (the
code returning errors.New("daemon 模式尚未实现")), return output.ErrWithHint or
output.Errorf instead; for example use output.ErrWithHint("daemon 模式尚未实现",
"implement daemon support (forking, PID file) or run without --daemon") or
output.Errorf("daemon 模式尚未实现: %s", "implement forking and PID file"), so the
error follows the RunE guideline and provides a structured, actionable hint.
---
Nitpick comments:
In `@shortcuts/bot/handler.go`:
- Around line 80-84: The warning currently prints to stdout via fmt.Printf in
the BotHandler session save block; change it to write to stderr by replacing
fmt.Printf with log.Printf (importing the log package) or, preferably, add an
optional logger/IOStreams field to BotHandlerConfig and BotHandler and use that
logger to emit the warning from the h.sessionMgr.Set error path (reference the
h.sessionMgr.Set call and BotHandler/BotHandlerConfig types so you update the
correct place), ensuring progress/JSON stays on stdout while warnings go to
stderr.
In `@shortcuts/bot/router.go`:
- Around line 248-259: Replace the manual bubble sort in
PatternRouter.sortPatterns with the standard library sort.Slice to be more
idiomatic: import sort, then call sort.Slice(pr.patterns, func(i, j int) bool {
return pr.patterns[i].Priority > pr.patterns[j].Priority }) so patterns are
sorted by Priority descending; ensure you reference the pr.patterns slice and
the Priority field of the pattern struct in the comparator and remove the bubble
sort loop.
- Around line 173-176: The `/clear` handler registered via r.RegisterCommand
currently only returns a message and must invoke the session manager to remove
the session for chatID; modify the router to accept or have access to the
SessionManager (e.g., add a sessionManager field on the router or pass it into
the factory that sets up RegisterCommand) and update the handler to call the
appropriate method (e.g., SessionManager.DeleteSession or
SessionManager.ClearSession) with chatID, handle any returned error and return
the success message only on success or an error otherwise; keep the handler
signature (ctx, args, chatID) and ensure you reference the router's
SessionManager inside the closure so the session is actually cleared.
In `@shortcuts/bot/session.go`:
- Around line 36-59: The code in NewSessionManager (and related SessionManager
file methods) uses os.* helpers (e.g., os.UserHomeDir, os.MkdirAll and other
os.* calls noted in the review) instead of the project's vfs abstraction;
refactor to use the vfs package equivalents by injecting or referencing a vfs
filesystem into SessionManager (e.g., add a fs vfs.FS field or accept it via
SessionManagerConfig), replace os.UserHomeDir/os.MkdirAll/etc. calls with vfs
methods, and update constructors and any file operations
(Stat/ReadFile/WriteFile/Rename/Remove/ReadDir) across SessionManager to use
that vfs instance for testability and consistency.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 856ed970-9f4a-4d16-baa3-2fe852afbe30
📒 Files selected for processing (5)
cmd/bot/start.goshortcuts/bot/claude.goshortcuts/bot/handler.goshortcuts/bot/router.goshortcuts/bot/session.go
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
cmd/bot/start.go (2)
64-66:⚠️ Potential issue | 🟠 MajorUse
output.Errorfinstead offmt.Errorffor RunE errors.Per coding guidelines for
cmd/**/*.go,RunEfunctions must returnoutput.Errorforoutput.ErrWithHint—never barefmt.Errorf. This applies to lines 65, 75, 95, 108, and 129.🐛 Suggested fix for line 65
if err := bot.ValidateClaudeCLI(ctx); err != nil { - return fmt.Errorf("claude CLI validation failed: %w", err) + return output.ErrWithHint(output.ExitRuntime, "claude_cli", fmt.Sprintf("validation failed: %v", err), "ensure 'claude' CLI is installed and in PATH") }Apply similar changes to the other
fmt.Errorfreturns at lines 75, 95, 108, and 129, using appropriate error codes and actionable hints.As per coding guidelines: "RunE functions in commands must return
output.Errorforoutput.ErrWithHint— never barefmt.Errorf"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bot/start.go` around lines 64 - 66, Replace bare fmt.Errorf returns in the RunE for the bot start command with output.Errorf or output.ErrWithHint: where you currently return fmt.Errorf("claude CLI validation failed: %w", err) after calling bot.ValidateClaudeCLI(ctx), change to output.Errorf with an appropriate error code and include err (e.g., output.Errorf(output.CodeInvalidInput, "claude CLI validation failed: %v", err)) or output.ErrWithHint when a user-actionable hint is needed; apply the same pattern to the other fmt.Errorf sites in this function (the returns after the calls/conditions around starting the bot, initializing clients, or config validation) so every RunE return uses output.Errorf/output.ErrWithHint, includes the wrapped error message, and supplies actionable hints where appropriate.
60-68:⚠️ Potential issue | 🟡 MinorProgress messages should go to stderr, not stdout.
Per coding guidelines, program output (JSON envelopes) goes to stdout while progress, warnings, and hints go to stderr. All the startup progress messages should use
io.ErrOut.♻️ Suggested fix pattern
- fmt.Fprintf(io.Out, "=== Claude Code Bot 启动中 ===\n") + fmt.Fprintf(io.ErrOut, "=== Claude Code Bot 启动中 ===\n") - fmt.Fprintf(io.Out, "验证 Claude Code CLI...\n") + fmt.Fprintf(io.ErrOut, "验证 Claude Code CLI...\n")Apply the same change to all progress messages (lines 60, 63, 67, 70, 77, 88, 97, 100, 123, 126, 132).
As per coding guidelines: "Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bot/start.go` around lines 60 - 68, The progress prints in the start routine are currently writing to stdout via fmt.Fprintf(io.Out, ...) but per guidelines must go to stderr; update all progress/warning/hint fmt.Fprintf calls in the start function (e.g., the calls printing "=== Claude Code Bot 启动中 ===", "验证 Claude Code CLI...", "✓ Claude Code CLI 已就绪" and the other startup messages) to use io.ErrOut instead of io.Out so program output stays on stdout and progress goes to stderr.
🧹 Nitpick comments (6)
shortcuts/bot/sender.go (2)
31-43: Unused helper functionbuildMessageContent.This function is defined but never called. Consider removing it until the actual Lark API integration is implemented, or add a TODO comment explaining its intended use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/sender.go` around lines 31 - 43, The helper function buildMessageContent on type MessageSender is currently unused; either remove the entire buildMessageContent method to eliminate dead code or keep it but add a clear TODO comment above the function explaining its intended purpose (building Lark message JSON) and when it will be used, so future Lark API integration won’t treat it as accidental dead code; update references to MessageSender only if you remove the function.
19-29: Progress/debug output should go to stderr, not stdout.The
[TODO]placeholder message is progress/diagnostic output. Per coding guidelines, it should be written to stderr rather than stdout (which is reserved for JSON envelopes/program output).♻️ Suggested fix
- fmt.Printf("[TODO] Send message to chat %s: %s\n", chatID, message) + fmt.Fprintf(os.Stderr, "[TODO] Send message to chat %s: %s\n", chatID, message)Add
"os"to the imports.Based on learnings: "Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/sender.go` around lines 19 - 29, The placeholder diagnostic print in MessageSender.SendMessage writes progress to stdout; change it to write to stderr by importing "os" and replacing fmt.Printf with fmt.Fprintln(os.Stderr, ...) (or equivalent) inside the SendMessage method so the "[TODO] Send message to chat ..." diagnostic goes to stderr while keeping the rest of the function unchanged.docs/CODEMAPS/backend.md (1)
300-313: Error handling pattern example usesfmt.Errorfbutcmd/**/*.gorequiresoutput.Errorf.The documented error handling pattern shows
fmt.Errorf, but per coding guidelines,cmd/**/*.gofiles must useoutput.Errorforoutput.ErrWithHint. Consider adding a note that command handlers have different requirements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CODEMAPS/backend.md` around lines 300 - 313, The docs example shows using fmt.Errorf for wrapping errors but command files under cmd/**/*.go must use output.Errorf or output.ErrWithHint; update the backend.md Error Handling Pattern to note this exception and either add an alternative example demonstrating output.Errorf/output.ErrWithHint for command handlers or a short note stating that fmt.Errorf is acceptable elsewhere but command handlers should use output.Errorf/output.ErrWithHint (reference the fmt.Errorf, output.Errorf, output.ErrWithHint symbols and the cmd/**/*.go/command handler context).docs/CODEMAPS/architecture.md (1)
17-76: Add language specifier to fenced code blocks.The ASCII diagram blocks should have a language tag (e.g.,
textorplaintext) to satisfy markdownlint MD040 and improve rendering consistency.♻️ Example fix for the first block
-``` +```text ┌─────────────────────────────────────────────────────────────┐ │ User/AI Agent │Apply similar changes to the other fenced blocks at lines 147, 169, and 209.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CODEMAPS/architecture.md` around lines 17 - 76, The fenced ASCII diagram blocks in docs/CODEMAPS/architecture.md (e.g., the block starting with the "User/AI Agent" diagram and the other ASCII diagram blocks in the file) are missing a language specifier; update each triple-backtick fence to include a plain-text language tag such as ```text or ```plaintext to satisfy markdownlint MD040 and ensure consistent rendering across the diagrams.cmd/bot/start.go (1)
48-49:--configand--daemonflags are defined but unused.The
--configflag is accepted butopts.Configis never used inbotStartRun. The--daemonflag is similarly unused. Consider either implementing these features or removing the flags to avoid user confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bot/start.go` around lines 48 - 49, The flags registered with cmd.Flags().StringVar(&config, "config", ...) and cmd.Flags().BoolVar(&daemon, "daemon", ...) are unused in botStartRun (opts.Config is never read and daemon is ignored); either remove these flag registrations or wire them into startup logic: read the parsed config string into opts.Config (or pass it to the function that loads configuration) and handle the daemon bool (e.g., set opts.Daemon, call the daemonize routine, or early-return with background spawn); update references to config/daemon or opts.Config/opts.Daemon so the variables are actually consumed by botStartRun and any config-loading/daemonizing helper functions.shortcuts/bot/subscribe.go (1)
155-177: Event body is parsed twice - once in handler, once in sendReply.The
event.BodyJSON is parsed increateEventHandler(line 110) and again insendReply(line 159). Consider extracting chat_id and message_id during the first parse and passing them through the call chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/subscribe.go` around lines 155 - 177, The sendReply function currently reparses event.Body to extract chat_id and message_id; instead, extract chat_id and message_id once in createEventHandler when event.Body is first unmarshaled and pass those values down to sendReply (or attach them to EventSubscriber/event context) so sendReply no longer calls json.Unmarshal on event.Body. Update createEventHandler to set chatID and messageID variables (or populate a small struct/fields on EventSubscriber) and change sendReply signature to accept chatID and messageID (or read from those fields) and then call s.sender.SendMessage(ctx, chatID, message, messageID), removing the duplicate parsing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/bot/start.go`:
- Around line 79-85: The WorkDir for claudeClient is hardcoded to "/tmp/..."
which breaks on Windows; change the WorkDir in the bot.NewClaudeClient call to
use a cross-platform temp path (e.g., filepath.Join(os.TempDir(),
"lark-claude-bot")) and update imports to include "os" and "path/filepath";
adjust the claudeClient initialization where bot.ClaudeClientConfig{ WorkDir:
... } is set so it builds the path at runtime instead of the hardcoded string.
In `@docs/CODEMAPS/backend.md`:
- Around line 192-206: The table row for the "Sender" component has a malformed
cell: the file path token `shortcuts/bot/sender.go is missing its closing
backtick; open docs/CODEMAPS/backend.md, locate the "Sender" row (the line
containing Sender and the file path), add the missing closing backtick so the
cell reads `shortcuts/bot/sender.go`, and confirm the table's pipe separators
remain correctly aligned.
In `@shortcuts/bot/subscribe.go`:
- Around line 188-193: The info method on EventSubscriber currently writes to
stdout via fmt.Println; change it to write to stderr (e.g., use
fmt.Fprintln(os.Stderr, msg)) so progress/info messages don't pollute program
stdout JSON envelopes, and add the os import if missing; keep the quiet check
and the EventSubscriber.info signature unchanged.
- Around line 79-84: The "Connected" message is printed before cli.Start(ctx)
actually succeeds; modify the logic so the success message is only shown after
startup completes: after launching the goroutine that sends the result to
startErrCh (where cli.Start(ctx) is invoked), read from startErrCh, check the
returned error, and call s.info("Connected. Waiting for events... (Ctrl+C to
stop)") only if the error is nil; otherwise log the error and exit/handle it.
Use the existing startErrCh, cli.Start(ctx), and s.info symbols to locate and
update the code.
- Around line 25-26: Replace the plain int eventCount field with sync/atomic's
atomic.Int64 (e.g., eventCount atomic.Int64) and update all accesses: in the
event handler increment using s.eventCount.Add(1), in the signal handler read
with s.eventCount.Load(), and in GetStats() return the loaded value
(s.eventCount.Load()) where "events_received" is produced; also add the
sync/atomic import and convert the loaded int64 as needed for any JSON or
formatting consumers.
---
Duplicate comments:
In `@cmd/bot/start.go`:
- Around line 64-66: Replace bare fmt.Errorf returns in the RunE for the bot
start command with output.Errorf or output.ErrWithHint: where you currently
return fmt.Errorf("claude CLI validation failed: %w", err) after calling
bot.ValidateClaudeCLI(ctx), change to output.Errorf with an appropriate error
code and include err (e.g., output.Errorf(output.CodeInvalidInput, "claude CLI
validation failed: %v", err)) or output.ErrWithHint when a user-actionable hint
is needed; apply the same pattern to the other fmt.Errorf sites in this function
(the returns after the calls/conditions around starting the bot, initializing
clients, or config validation) so every RunE return uses
output.Errorf/output.ErrWithHint, includes the wrapped error message, and
supplies actionable hints where appropriate.
- Around line 60-68: The progress prints in the start routine are currently
writing to stdout via fmt.Fprintf(io.Out, ...) but per guidelines must go to
stderr; update all progress/warning/hint fmt.Fprintf calls in the start function
(e.g., the calls printing "=== Claude Code Bot 启动中 ===", "验证 Claude Code
CLI...", "✓ Claude Code CLI 已就绪" and the other startup messages) to use
io.ErrOut instead of io.Out so program output stays on stdout and progress goes
to stderr.
---
Nitpick comments:
In `@cmd/bot/start.go`:
- Around line 48-49: The flags registered with cmd.Flags().StringVar(&config,
"config", ...) and cmd.Flags().BoolVar(&daemon, "daemon", ...) are unused in
botStartRun (opts.Config is never read and daemon is ignored); either remove
these flag registrations or wire them into startup logic: read the parsed config
string into opts.Config (or pass it to the function that loads configuration)
and handle the daemon bool (e.g., set opts.Daemon, call the daemonize routine,
or early-return with background spawn); update references to config/daemon or
opts.Config/opts.Daemon so the variables are actually consumed by botStartRun
and any config-loading/daemonizing helper functions.
In `@docs/CODEMAPS/architecture.md`:
- Around line 17-76: The fenced ASCII diagram blocks in
docs/CODEMAPS/architecture.md (e.g., the block starting with the "User/AI Agent"
diagram and the other ASCII diagram blocks in the file) are missing a language
specifier; update each triple-backtick fence to include a plain-text language
tag such as ```text or ```plaintext to satisfy markdownlint MD040 and ensure
consistent rendering across the diagrams.
In `@docs/CODEMAPS/backend.md`:
- Around line 300-313: The docs example shows using fmt.Errorf for wrapping
errors but command files under cmd/**/*.go must use output.Errorf or
output.ErrWithHint; update the backend.md Error Handling Pattern to note this
exception and either add an alternative example demonstrating
output.Errorf/output.ErrWithHint for command handlers or a short note stating
that fmt.Errorf is acceptable elsewhere but command handlers should use
output.Errorf/output.ErrWithHint (reference the fmt.Errorf, output.Errorf,
output.ErrWithHint symbols and the cmd/**/*.go/command handler context).
In `@shortcuts/bot/sender.go`:
- Around line 31-43: The helper function buildMessageContent on type
MessageSender is currently unused; either remove the entire buildMessageContent
method to eliminate dead code or keep it but add a clear TODO comment above the
function explaining its intended purpose (building Lark message JSON) and when
it will be used, so future Lark API integration won’t treat it as accidental
dead code; update references to MessageSender only if you remove the function.
- Around line 19-29: The placeholder diagnostic print in
MessageSender.SendMessage writes progress to stdout; change it to write to
stderr by importing "os" and replacing fmt.Printf with fmt.Fprintln(os.Stderr,
...) (or equivalent) inside the SendMessage method so the "[TODO] Send message
to chat ..." diagnostic goes to stderr while keeping the rest of the function
unchanged.
In `@shortcuts/bot/subscribe.go`:
- Around line 155-177: The sendReply function currently reparses event.Body to
extract chat_id and message_id; instead, extract chat_id and message_id once in
createEventHandler when event.Body is first unmarshaled and pass those values
down to sendReply (or attach them to EventSubscriber/event context) so sendReply
no longer calls json.Unmarshal on event.Body. Update createEventHandler to set
chatID and messageID variables (or populate a small struct/fields on
EventSubscriber) and change sendReply signature to accept chatID and messageID
(or read from those fields) and then call s.sender.SendMessage(ctx, chatID,
message, messageID), removing the duplicate parsing logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f730e6a-2332-47e5-90d2-6f436315f041
📒 Files selected for processing (10)
.gitignore.reports/codemap-diff.txtcmd/bot/start.gocmd/bot/status.gocmd/bot/stop.godocs/CODEMAPS/architecture.mddocs/CODEMAPS/backend.mdshortcuts/bot/claude.goshortcuts/bot/sender.goshortcuts/bot/subscribe.go
💤 Files with no reviewable changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/bot/status.go
- cmd/bot/stop.go
- shortcuts/bot/claude.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/bot/handler_test.go (1)
73-77: Avoid relying on nil-receiver behavior in these tests.
NewBotHandlerrejects a nilSessionManager, so both setups returnerrand a nilhandler. The subtests only work becauseextractTextContent/parseMessageEventcurrently avoid touching receiver state. Please construct a valid handler or instantiate&BotHandler{}directly for these helper-level tests so they stay aligned with the constructor contract.Also applies to: 188-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/bot/handler_test.go` around lines 73 - 77, Tests currently pass a nil SessionManager to NewBotHandler which violates its contract and results in err + nil handler; update the tests to either construct a valid handler via NewBotHandler with a real SessionManager mock/fixture or bypass the constructor by instantiating &BotHandler{} directly for helper-level tests that only exercise extractTextContent and parseMessageEvent; change both occurrences (around the shown block and the similar setup at lines ~188-192) to use a proper SessionManager mock or &BotHandler{} so the tests no longer rely on nil-receiver behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/bot/claude_test.go`:
- Around line 103-152: The test currently calls the test-only
ClaudeResponse.UnmarshalJSON stub (which always returns nil and doesn't populate
fields), so replace that call in TestClaudeResponse with the real JSON
unmarshal: use json.Unmarshal([]byte(tt.json), &resp) and ensure the test
imports "encoding/json"; this will exercise actual parsing for the
ClaudeResponse struct instead of the stubbed UnmarshalJSON method.
In `@shortcuts/bot/session_test.go`:
- Around line 171-203: The test creates both sessions before the sleep so both
exceed the 50ms TTL and will be expired; update the test to ensure "chat_active"
remains fresh by either (a) creating or updating "chat_active" after the 60ms
sleep (use SessionManager.Set("chat_active", ...) again) or (b) use a larger TTL
in the SessionManagerConfig for the active session test; ensure the code paths
referencing NewSessionManager, SessionManagerConfig (TTL), Set, CleanupExpired,
and Get reflect this change so CleanupExpired returns count == 1 and
Get("chat_active") is non-nil.
---
Nitpick comments:
In `@shortcuts/bot/handler_test.go`:
- Around line 73-77: Tests currently pass a nil SessionManager to NewBotHandler
which violates its contract and results in err + nil handler; update the tests
to either construct a valid handler via NewBotHandler with a real SessionManager
mock/fixture or bypass the constructor by instantiating &BotHandler{} directly
for helper-level tests that only exercise extractTextContent and
parseMessageEvent; change both occurrences (around the shown block and the
similar setup at lines ~188-192) to use a proper SessionManager mock or
&BotHandler{} so the tests no longer rely on nil-receiver behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8f8ba26-7b16-405f-932f-aa8cff145e0b
📒 Files selected for processing (4)
shortcuts/bot/claude_test.goshortcuts/bot/handler_test.goshortcuts/bot/router_test.goshortcuts/bot/session_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/bot/router_test.go
新增文档： - README.bot.md: Bot 功能说明和使用指南 - docs/bot-integration-plan.md: 完整的技术方案设计 主要内容包括： - 项目目标：飞书 Bot + Claude Code 集成 - 核心功能：会话管理、命令路由、文件操作 - 架构设计：复用 lark-cli 现有基础设施 - 开发计划：4 个 Phase，预计 4-6 天
新增功能： - cmd/bot/: 创建 Bot 子命令框架 - bot.go: Bot 命令入口，包含 start/status/stop 子命令 - start.go: 启动 Bot（基础框架，TODO: 完整实现） - status.go: 查看 Bot 状态（基础框架，TODO: 实现） - stop.go: 停止 Bot（基础框架，TODO: 实现） - cmd/root.go: 注册 bot 命令到命令树 实现进度： - ✅ 命令框架结构 - ✅ cobra 命令注册 - ⏳ 实际功能实现（Phase 1 进行中） 下一步： - 集成 event +subscribe - 实现 session 管理 - 实现 Claude Code 集成 - 实现命令路由 相关文档：README.bot.md, docs/bot-integration-plan.md
添加测试验证文档： - 静态代码验证清单（全部通过） - 动态功能测试指南（需要 Go 环境） - 编译和功能测试步骤 - 当前实现状态说明 静态验证结果： - ✅ 代码结构正确 - ✅ 导入和注册正确 - ✅ 命令模式一致 - ✅ 版权和注释完整 下一步：安装 Go 后进行编译测试
生成代码地图（Code Maps）： - docs/CODEMAPS/architecture.md - 整体架构设计 - docs/CODEMAPS/backend.md - 后端实现详解 - docs/CODEMAPS/dependencies.md - 依赖项清单 - .reports/codemap-diff.txt - 生成报告 项目分析： - 项目类型：Go CLI 工具（飞书/Lark） - 代码规模：520 个 Go 文件 - 命令数量：10 个内置命令 + 200+ shortcuts - 业务领域：12 个（日历、消息、文档、表格等） - AI Skills：20 个 Agent Skills 架构亮点： - 三层命令系统（Shortcuts → API Commands → Raw API） - 事件订阅（WebSocket 长连接） - AI Agent 友好设计 - 新增：Bot 功能框架（feature/claude-code-bot） Token优化： - 使用 ASCII 图表 - 函数签名替代完整实现 - 结构化列表便于扫描 - 每个文档 < 1000 tokens 生成时间：2026-04-10 文件扫描：520 个 Go 文件
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Implement 4 core modules for Phase 2 of bot development: - claude.go: Claude Code CLI integration with retry logic - session.go: Session persistence with TTL and cleanup - handler.go: Message event processing and routing - router.go: Command routing with whitelist and pattern matching Updated start.go to initialize all modules and validate Claude CLI. Status: Core modules complete, event subscription integration pending.
Implement event subscription integration for Phase 1: - subscribe.go: WebSocket event subscriber with graceful shutdown - Connects to Lark event stream via WebSocket - Handles im.message.receive_v1 events - Routes events to bot handler for processing - sender.go: Message sender for replying to Lark - Placeholder for im +messages-send integration - Builds message content JSON - Creates Lark API request objects - start.go: Updated to initialize and start event subscriber - Removed placeholder code - Now blocks on event subscription - Graceful shutdown on Ctrl+C Status: Event subscription complete, reply sending needs im integration.
Updated 3 codemap documents to reflect Bot integration status: - architecture.md: Added bot module to system diagram, updated file counts - backend.md: Detailed Bot implementation with complete data flow - codemap-diff.txt: Full analysis report showing 549 files (+29) Key updates: - Bot integration: ~1,200 lines across 6 core modules - Architecture: Complete Feishu → Claude → Feishu flow - Progress: ~80% complete, reply sending pending Also removed docs/ from .gitignore to track documentation.
Fixed multiple compilation issues: 1. Import path corrections: - cmd/bot/start.go: cmd/cmdutil → internal/cmdutil - shortcuts/bot/claude.go: removed unused cmdutil import - shortcuts/bot/subscribe.go: added dispatcher import, fixed larkws alias 2. Type fixes: - cmd/bot/start.go: use core.LoadMultiAppConfig() instead of non-existent InitializedRuntime - shortcuts/bot/subscribe.go: SecretInput type support (use .Plain field) - shortcuts/bot/sender.go: commented out TODO im.CreateMessageReq usage 3. Unused variable cleanup: - cmd/bot/status.go: removed unused ctx variable - cmd/bot/stop.go: removed unused ctx variable 4. Command flow: - cmd/bot/start.go: create BotStartOptions in RunE, not modify BotOptions - cmd/bot/status.go, cmd/bot/stop.go: removed opts.Ctx assignments Result: lark-cli now compiles successfully with Go 1.26.2. Verified: ./lark-cli bot --help works correctly.
Added 4 test files for bot core modules: 1. session_test.go (355 lines) - Session manager creation and initialization - Get/Set/Delete operations - TTL expiration and cleanup - Special characters handling (Chinese, slashes) - Concurrent access (thread safety) - List operations 2. router_test.go (280 lines) - Router creation and built-in commands - Custom command registration - Command aliases - Whitelist enforcement - Pattern-based routing with priority - Fallback handlers - Command argument parsing 3. claude_test.go (180 lines) - Claude client creation and configuration - Default value handling - Retry logic for transient errors - Context cancellation handling - JSON response parsing 4. handler_test.go (290 lines) - Bot handler creation and validation - Message event parsing - Text content extraction (text/post types) - Error handling for malformed events - Statistics retrieval Test Coverage Goals: - Happy path: Core functionality with valid inputs - Error handling: Invalid inputs, missing data, failures - Edge cases: Empty values, special characters, concurrency - Thread safety: Concurrent get/set operations Status: Test framework established, ready for execution.
- 修复 Get() 持有读锁时调用 Delete(需要写锁) 导致的死锁 - 修复 CleanupExpired() 持有写锁时调用 List(需要读锁) 导致的死锁 - 修复 handler_test.go 中 missing header → missing event data - 移除递归 UnmarshalJSON 导致的栈溢出 - 修复 router_test.go 参数解析测试(使用自定义命令) - 添加 sender_test.go 消息发送模块测试 - 添加 subscribe_test.go 事件订阅辅助方法测试 - 覆盖率: 58.2% → 72.1%
- 添加 subscribe_integration_test.go 集成测试 - handleMessageEvent 成功路径测试 - sendReply 错误路径测试 - createEventHandler 边界情况测试 - 修复 sendReply 和 createEventHandler 的 nil 检查 - 使 MessageSender 可注入，便于测试 - 覆盖率: 76.6% → 85.1%
- Update scan date from 2026-04-10 to 2026-04-11 - Update file count: 549→558 (architecture), 520→558 (backend) - Mark bot implementation as Complete with 85.1% test coverage - Ignore docs/ coverage artifacts in .gitignore
- Replace placeholder sender with real lark-sdk-go client - Add LarkClient to EventSubscriber via app credentials - Implement SendMessage via im.V1.Message.Create API - Implement sendReply via im.V1.Message.Reply API - Add NewMessageSenderWithClient for real client injection - Keep NewMessageSender() for nil-client test compatibility - Update tests to reflect new API contract (nil client = error)
richardiitse
force-pushed
the
feature/claude-code-bot
branch
from
a5bff58 to
6774e3a
Compare
Author
|
add lark cli bot to connect Claude Code |
- Use &BotHandler{} for helper tests that don't need SessionManager
- Add clarifying comment for CleanupExpired test timing
1 task
Comment on lines
+75
to
+86
| response, err := h.claudeClient.ProcessMessage(ctx, msgEvent.Content, sessionID) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to process message with claude: %w", err) | ||
| } | ||
|
|
||
| // Update session | ||
| if _, err := h.sessionMgr.Set(msgEvent.ChatID, response.SessionID); err != nil { | ||
| // Log error but don't fail the response | ||
| fmt.Printf("Warning: failed to save session: %v\n", err) | ||
| } | ||
|
|
||
| return response.Result, nil |
There was a problem hiding this comment.
BotHandler.HandleMessage sends every incoming message directly to Claude via claudeClient.ProcessMessage, completely bypassing the Router defined in router.go. As a result, the built-in commands /help, /status, and /clear registered in registerBuiltInCommands() are unreachable — a user sending /clear will have their message forwarded verbatim to Claude instead of clearing the session.
The handler needs to consult the router first; something like:
// Process message with Router (handles commands like /help, /clear)
response, err := h.router.Route(ctx, msgEvent.Content, msgEvent.ChatID)
if err != nil {
// fall through to Claude
claudeResp, claudeErr := h.claudeClient.ProcessMessage(ctx, msgEvent.Content, sessionID)
...
}BotHandler would need a router *Router field wired in from the constructor.
Comment on lines
+103
to
+109
| config, err := core.LoadMultiAppConfig() | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return output.ErrWithHint(output.ExitValidation, "config", "not configured", "run: lark-cli config init") | ||
| } | ||
| return fmt.Errorf("failed to load config: %w", err) | ||
| } |
There was a problem hiding this comment.
--config flag is silently ignored
opts.Config holds the value of the --config CLI flag, but botStartRun calls core.LoadMultiAppConfig() unconditionally on line 103, never consulting opts.Config. Any path the user passes with --config is parsed and discarded — the flag is a no-op.
If core.LoadMultiAppConfig has an overload or the caller is expected to pass a path, use opts.Config here:
config, err := core.LoadMultiAppConfig(/* opts.Config */)Otherwise, remove the flag and its corresponding BotStartOptions.Config field to avoid misleading users.
- Rewrite README.bot.md to remove fork-specific references - Update cmd/bot/TEST.md with actual implementation status - Fix docs/bot-integration-plan.md directory structure references - Mark all Phase items as completed
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Comment on lines
+84
to
+112
| func (c *ClaudeClient) processMessageOnce(ctx context.Context, message string, sessionID string) (*ClaudeResponse, error) { | ||
| // Build command arguments | ||
| args := []string{ | ||
| "-p", message, | ||
| "--output-format", "json", | ||
| "--add-dir", c.workDir, | ||
| } | ||
|
|
||
| if c.skipPermissions { | ||
| args = append(args, "--dangerously-skip-permissions") | ||
| } | ||
|
|
||
| if sessionID != "" { | ||
| args = append(args, "--resume", sessionID) | ||
| } | ||
|
|
||
| // Create command with timeout | ||
| cmdCtx, cancel := context.WithTimeout(ctx, c.timeout) | ||
| defer cancel() | ||
|
|
||
| cmd := exec.CommandContext(cmdCtx, "claude", args...) | ||
|
|
||
| // Capture stdout and stderr | ||
| var stdout, stderr bytes.Buffer | ||
| cmd.Stdout = &stdout | ||
| cmd.Stderr = &stderr | ||
|
|
||
| // Execute command | ||
| err := cmd.Run() |
There was a problem hiding this comment.
c.workDir (hardcoded to /tmp/lark-claude-bot in botStartRun) is never created anywhere before processMessageOnce uses it. This means --add-dir c.workDir passes a non-existent path to the claude CLI on every invocation. Additionally, cmd.Dir is never set, so the claude subprocess runs in whatever directory the lark-cli process was launched from rather than the intended workdir.
Both issues together mean that every Claude invocation either errors on --add-dir validation or operates in the wrong directory, breaking file-context features.
func (c *ClaudeClient) processMessageOnce(ctx context.Context, message string, sessionID string) (*ClaudeResponse, error) {
// Ensure working directory exists
if c.workDir != "" {
if err := os.MkdirAll(c.workDir, 0750); err != nil {
return nil, fmt.Errorf("failed to create work dir: %w", err)
}
}
// ... build args ...
cmd := exec.CommandContext(cmdCtx, "claude", args...)
cmd.Dir = c.workDir // ← add this
// ...
}| } | ||
|
|
||
| // Ensure base directory exists | ||
| if err := os.MkdirAll(config.BaseDir, 0755); err != nil { |
There was a problem hiding this comment.
The sessions directory is created with 0755 and each session file is written with 0644 (line 127). On any multi-user system this lets every local OS user enumerate all active bot sessions, read chat IDs, and capture the Claude session IDs (which can be used to resume conversations). The directory and files should be owner-only:
Suggested change
| if err := os.MkdirAll(config.BaseDir, 0755); err != nil { | |
| if err := os.MkdirAll(config.BaseDir, 0700); err != nil { |
And on line 127:
if err := os.WriteFile(tmpPath, data, 0600); err != nil {
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
2 participants
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
This PR adds a new
lark-cli botcommand that integrates Claude Code with Feishu/Lark, enabling users to chat with Claude Code directly from Feishu messages.Changes
New Commands
lark-cli bot start- Start the Claude Code Botlark-cli bot status- View Bot statuslark-cli bot stop- Stop running BotCore Modules
shortcuts/bot/subscribe.go) - WebSocket-based Lark event listenershortcuts/bot/handler.go) - Event parsing and Claude routingshortcuts/bot/claude.go) - CLI invocation with JSON parsingshortcuts/bot/session.go) - TTL-based persistence with atomic writesshortcuts/bot/router.go) - Slash commands and pattern matchingshortcuts/bot/sender.go) - Real Lark IM API integrationDocumentation
Test Coverage
Test Plan
🤖 Generated with Claude Code