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 (1 warning, 1 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 adds a Confidence Score: 3/5Not safe to merge — the bot cannot send any replies due to an empty access token, and previously flagged deadlocks and a hardcoded permission bypass remain open. One new P1 (empty tenant token breaks the entire reply path), plus multiple prior P0/P1 findings still open: recursive RLock deadlock in session.List(), /help deadlock in the router, and SkipPermissions hardcoded to true. Compile errors and the no-op sender from earlier rounds are resolved, which is meaningful progress, but the core message-sending path still does not work end-to-end. shortcuts/bot/sender.go (empty token), shortcuts/bot/session.go (recursive lock), shortcuts/bot/router.go (/help deadlock), cmd/bot/start.go (hardcoded SkipPermissions) Important Files Changed
Sequence DiagramsequenceDiagram
participant LarkWS as Lark WebSocket
participant Sub as EventSubscriber
participant Handler as BotHandler
participant Claude as ClaudeClient (CLI)
participant Session as SessionManager
participant Sender as MessageSender
participant LarkAPI as Lark IM API
LarkWS->>Sub: im.message.receive_v1 event
Sub->>Handler: HandleMessage(event)
Handler->>Session: Get(chatID)
Session-->>Handler: sessionID (or nil)
Handler->>Claude: ProcessMessage(text, sessionID)
Claude-->>Handler: ClaudeResponse{Result, SessionID}
Handler->>Session: Set(chatID, newSessionID)
Handler-->>Sub: response text
Sub->>Sender: SendMessage(chatID, text, messageID)
Note over Sender,LarkAPI: WithTenantAccessToken("") overrides SDK token
Sender->>LarkAPI: POST /im/v1/messages (empty token)
LarkAPI-->>Sender: 401 Unauthorized
Reviews (6): Last reviewed commit: "feat(bot): integrate real Lark IM API fo..." | 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
Comment on lines
+57
to
+59
| if err := bot.ValidateClaudeCLI(ctx); err != nil { | ||
| return fmt.Errorf("claude CLI validation failed: %w", err) | ||
| } |
There was a problem hiding this comment.
Use output.Errorf instead of fmt.Errorf for RunE errors.
Per coding guidelines for cmd/**/*.go, RunE functions must return output.Errorf or output.ErrWithHint—never bare fmt.Errorf. Error messages should be structured and actionable for AI agents.
Proposed fix
if err := bot.ValidateClaudeCLI(ctx); err != nil {
- return fmt.Errorf("claude CLI validation failed: %w", err)
+ return output.Errorf(1, "claude_cli_validation_failed", "claude CLI validation failed: %v", err)
}Also applies to lines 68 and 88. As per coding guidelines: "RunE functions in commands must return output.Errorf or output.ErrWithHint — never bare fmt.Errorf"
📝 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
| if err := bot.ValidateClaudeCLI(ctx); err != nil { | |
| return fmt.Errorf("claude CLI validation failed: %w", err) | |
| } | |
| if err := bot.ValidateClaudeCLI(ctx); err != nil { | |
| return output.Errorf(1, "claude_cli_validation_failed", "claude CLI validation failed: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/bot/start.go` around lines 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.
Comment on lines
+58
to
+67
| for attempt := 0; attempt < c.maxRetries; attempt++ { | ||
| if attempt > 0 { | ||
| // Exponential backoff before retry | ||
| waitTime := time.Duration(attempt) * time.Second | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(waitTime): | ||
| } | ||
| } |
There was a problem hiding this comment.
Backoff is linear, not exponential as documented.
The comment says "Exponential backoff" but waitTime := time.Duration(attempt) * time.Second produces linear delays (1s, 2s, 3s...). True exponential backoff would be 2^attempt seconds.
Proposed fix for exponential backoff
if attempt > 0 {
- // Exponential backoff before retry
- waitTime := time.Duration(attempt) * time.Second
+ // Exponential backoff before retry (1s, 2s, 4s, 8s...)
+ waitTime := time.Duration(1<<uint(attempt-1)) * time.Second
select {
case <-ctx.Done():
return nil, ctx.Err()
case <-time.After(waitTime):
}
}📝 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
| for attempt := 0; attempt < c.maxRetries; attempt++ { | |
| if attempt > 0 { | |
| // Exponential backoff before retry | |
| waitTime := time.Duration(attempt) * time.Second | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case <-time.After(waitTime): | |
| } | |
| } | |
| for attempt := 0; attempt < c.maxRetries; attempt++ { | |
| if attempt > 0 { | |
| // Exponential backoff before retry (1s, 2s, 4s, 8s...) | |
| waitTime := time.Duration(1<<uint(attempt-1)) * time.Second | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case <-time.After(waitTime): | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/claude.go` around lines 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.
Comment on lines
+142
to
+148
| retryablePatterns := []string{ | ||
| "timeout", | ||
| "connection refused", | ||
| "temporary failure", | ||
| "resource temporarily unavailable", | ||
| "context deadline exceeded", | ||
| } |
There was a problem hiding this comment.
context deadline exceeded may not be retryable for parent context timeouts.
If the parent context (passed to ProcessMessage) is cancelled or times out, retrying won't help. The current check treats all "context deadline exceeded" errors as retryable, but only the per-attempt timeout (line 103) should trigger retries.
Proposed fix: check parent context before retry
func (c *ClaudeClient) ProcessMessage(ctx context.Context, message string, sessionID string) (*ClaudeResponse, error) {
var lastErr error
for attempt := 0; attempt < c.maxRetries; attempt++ {
+ // Don't retry if parent context is done
+ if ctx.Err() != nil {
+ return nil, ctx.Err()
+ }
+
if attempt > 0 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/claude.go` around lines 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.
Comment on lines
+91
to
+139
| // Route routes a message to the appropriate command handler | ||
| func (r *Router) Route(ctx context.Context, message string, chatID string) (string, error) { | ||
| r.mu.RLock() | ||
| defer r.mu.RUnlock() | ||
|
|
||
| message = strings.TrimSpace(message) | ||
| if message == "" { | ||
| return "", nil | ||
| } | ||
|
|
||
| // Check if message is a command | ||
| if !strings.HasPrefix(message, "/") { | ||
| // Not a command, use default handler | ||
| if r.defaultHandler != nil { | ||
| return r.defaultHandler(ctx, []string{message}, chatID) | ||
| } | ||
| return "", fmt.Errorf("no default handler registered") | ||
| } | ||
|
|
||
| // Parse command and arguments | ||
| parts := strings.Fields(message) | ||
| if len(parts) == 0 { | ||
| return "", fmt.Errorf("empty command") | ||
| } | ||
|
|
||
| command := strings.ToLower(strings.TrimPrefix(parts[0], "/")) | ||
| var args []string | ||
| if len(parts) > 1 { | ||
| args = parts[1:] | ||
| } | ||
|
|
||
| // Resolve alias | ||
| if target, isAlias := r.aliases[command]; isAlias { | ||
| command = target | ||
| } | ||
|
|
||
| // Find handler | ||
| handler, exists := r.commands[command] | ||
| if !exists { | ||
| // Unknown command, use default handler | ||
| if r.defaultHandler != nil { | ||
| return r.defaultHandler(ctx, []string{message}, chatID) | ||
| } | ||
| return "", fmt.Errorf("unknown command: /%s", command) | ||
| } | ||
|
|
||
| // Execute handler | ||
| return handler(ctx, args, chatID) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Route holds lock while executing handlers, preventing handlers from accessing router state.
The Route method holds RLock for the entire duration including handler execution (line 138). This prevents handlers from calling any Router methods that require locks, making it impossible for handlers to introspect the router (like /help needs to do).
Proposed fix: release lock before calling handler
func (r *Router) Route(ctx context.Context, message string, chatID string) (string, error) {
r.mu.RLock()
- defer r.mu.RUnlock()
message = strings.TrimSpace(message)
if message == "" {
+ r.mu.RUnlock()
return "", nil
}
// ... command parsing logic ...
// Find handler
handler, exists := r.commands[command]
+ defaultHandler := r.defaultHandler
+ r.mu.RUnlock()
+
if !exists {
// Unknown command, use default handler
- if r.defaultHandler != nil {
- return r.defaultHandler(ctx, []string{message}, chatID)
+ if defaultHandler != nil {
+ return defaultHandler(ctx, []string{message}, chatID)
}
return "", fmt.Errorf("unknown command: /%s", command)
}
// Execute handler
return handler(ctx, args, chatID)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/router.go` around lines 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).
Comment on lines
+160
to
+170
| // /help - Show available commands | ||
| r.RegisterCommand("help", func(ctx context.Context, args []string, chatID string) (string, error) { | ||
| r.mu.RLock() | ||
| defer r.mu.RUnlock() | ||
|
|
||
| help := "Available commands:\n" | ||
| for cmd := range r.commands { | ||
| help += fmt.Sprintf(" /%s\n", cmd) | ||
| } | ||
| return help, nil | ||
| }) |
There was a problem hiding this comment.
Deadlock: /help handler acquires RLock while Route already holds RLock.
Route() acquires RLock at line 93 and calls the command handler at line 138. The /help handler then tries to acquire RLock again at line 162. Since Go's sync.RWMutex is not reentrant, this will deadlock when /help is invoked.
Proposed fix: use unlocked list helper
// /help - Show available commands
r.RegisterCommand("help", func(ctx context.Context, args []string, chatID string) (string, error) {
- r.mu.RLock()
- defer r.mu.RUnlock()
-
help := "Available commands:\n"
- for cmd := range r.commands {
+ for _, cmd := range r.ListCommands() {
help += fmt.Sprintf(" /%s\n", cmd)
}
return help, nil
})However, this still has issues since Route holds the lock when calling handlers. A better approach is to either:
- Copy
r.commandskeys before calling handlers (release lock first) - Make handlers not require the lock by passing necessary data as arguments
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/router.go` around lines 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.
Comment on lines
+62
to
+91
| func (sm *SessionManager) Get(chatID string) (*SessionData, error) { | ||
| sm.mu.RLock() | ||
| defer sm.mu.RUnlock() | ||
|
|
||
| sessionPath := sm.sessionPath(chatID) | ||
|
|
||
| // Check if file exists | ||
| if _, err := os.Stat(sessionPath); os.IsNotExist(err) { | ||
| return nil, nil // Session not found (not an error) | ||
| } | ||
|
|
||
| // Read session file | ||
| data, err := os.ReadFile(sessionPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read session file: %w", err) | ||
| } | ||
|
|
||
| var session SessionData | ||
| if err := json.Unmarshal(data, &session); err != nil { | ||
| return nil, fmt.Errorf("failed to parse session data: %w", err) | ||
| } | ||
|
|
||
| // Check if session has expired | ||
| if sm.isExpired(&session) { | ||
| _ = sm.Delete(chatID) // Clean up expired session | ||
| return nil, nil | ||
| } | ||
|
|
||
| return &session, nil | ||
| } |
There was a problem hiding this comment.
Deadlock: Get is called from List while holding RLock.
List() at line 170 acquires RLock and then calls sm.Get(chatID) which also tries to acquire RLock at line 63. Go's sync.RWMutex is not reentrant—this will deadlock.
Proposed fix: extract unlocked internal helper
// Get retrieves a session by chat ID
func (sm *SessionManager) Get(chatID string) (*SessionData, error) {
sm.mu.RLock()
defer sm.mu.RUnlock()
+ return sm.getUnlocked(chatID)
+}
+// getUnlocked retrieves a session without acquiring lock (caller must hold lock)
+func (sm *SessionManager) getUnlocked(chatID string) (*SessionData, error) {
sessionPath := sm.sessionPath(chatID)
// ... rest of implementationThen update List() to call getUnlocked instead of Get.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/session.go` around lines 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.
Comment on lines
+210
to
+216
| // sessionPath returns the file path for a given chat ID | ||
| func (sm *SessionManager) sessionPath(chatID string) string { | ||
| // Sanitize chat_id for filename (replace special chars) | ||
| safeChatID := strings.ReplaceAll(chatID, "/", "_") | ||
| safeChatID = strings.ReplaceAll(safeChatID, "\\", "_") | ||
| return filepath.Join(sm.baseDir, safeChatID+".json") | ||
| } |
There was a problem hiding this comment.
Weak path sanitization: only / and \ are replaced.
The sessionPath function only replaces / and \ with _. A malicious chatID containing .. could still attempt path traversal (e.g., .._.._etc_passwd after sanitization). Per coding guidelines, use validate.SafeInputPath before file I/O operations.
Proposed fix
func (sm *SessionManager) sessionPath(chatID string) string {
- // Sanitize chat_id for filename (replace special chars)
- safeChatID := strings.ReplaceAll(chatID, "/", "_")
- safeChatID = strings.ReplaceAll(safeChatID, "\\", "_")
+ // Sanitize chat_id for filename
+ safeChatID := strings.ReplaceAll(chatID, "/", "_")
+ safeChatID = strings.ReplaceAll(safeChatID, "\\", "_")
+ safeChatID = strings.ReplaceAll(safeChatID, "..", "_")
+ // Additional: only allow alphanumeric, underscore, hyphen
+ safeChatID = regexp.MustCompile(`[^a-zA-Z0-9_-]`).ReplaceAllString(safeChatID, "_")
return filepath.Join(sm.baseDir, safeChatID+".json")
}Or better, use validate.SafeInputPath as per project guidelines.
As per coding guidelines: "Validate paths using validate.SafeInputPath before any file I/O operations"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/session.go` around lines 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.
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
Comment on lines
+79
to
+85
| // 3. Initialize Claude client | ||
| claudeClient := bot.NewClaudeClient(bot.ClaudeClientConfig{ | ||
| WorkDir: "/tmp/lark-claude-bot", | ||
| Timeout: 5 * time.Minute, | ||
| MaxRetries: 3, | ||
| SkipPermissions: true, | ||
| }) |
There was a problem hiding this comment.
Hardcoded /tmp path is not cross-platform.
The path /tmp/lark-claude-bot won't work on Windows. Consider using os.TempDir() or filepath.Join(os.TempDir(), "lark-claude-bot") for cross-platform compatibility.
🐛 Suggested fix
+ workDir := filepath.Join(os.TempDir(), "lark-claude-bot")
+
// 3. Initialize Claude client
claudeClient := bot.NewClaudeClient(bot.ClaudeClientConfig{
- WorkDir: "/tmp/lark-claude-bot",
+ WorkDir: workDir,
Timeout: 5 * time.Minute,
MaxRetries: 3,
SkipPermissions: true,
})
// 4. Initialize bot handler
fmt.Fprintf(io.Out, "初始化 Bot 处理器...\n")
botHandler, err := bot.NewBotHandler(bot.BotHandlerConfig{
ClaudeClient: claudeClient,
SessionManager: sessionMgr,
- WorkDir: "/tmp/lark-claude-bot",
+ WorkDir: workDir,
})Add "path/filepath" to imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/bot/start.go` around lines 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.
Comment on lines
+25
to
+26
| eventCount int | ||
| quiet bool |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Lark oapi-sdk-go v3 WebSocket client dispatch event handlers concurrently or sequentially?
💡 Result:
In oapi-sdk-go v3, the WebSocket client dispatches event handling concurrently across messages:
- The receive loop spawns a new goroutine per incoming WebSocket frame (
go c.handleMessage(ctx, msg)), so multiple events can be processed in parallel. [1] - Inside that goroutine, the SDK calls the dispatcher synchronously (
c.eventHandler.Do(ctx, pl)), and the dispatcher in turn calls your registered handler synchronously (handler.Handle(ctx, event)), i.e., no extra serialization is added by the dispatcher itself. [1][2]
So: concurrent across events/messages; sequential within a single event’s handler invocation. [1][2]
Sources: [1] [2]
🏁 Script executed:
cat -n shortcuts/bot/subscribe.go | head -200Repository: larksuite/cli
Length of output: 6636
Data race on eventCount field requires synchronization.
The eventCount field is incremented in event handler callbacks (line 106) and read in signal handler (line 89) and GetStats() (line 182). Because the Lark WebSocket client dispatches event handlers concurrently across messages (spawning a new goroutine per frame), this field can be accessed by multiple goroutines simultaneously without synchronization, causing a data race.
Fix by using atomic.Int64:
Diff
+import "sync/atomic"
+
type EventSubscriber struct {
botHandler *BotHandler
sender *MessageSender
appID string
appSecret core.SecretInput
domain string
- eventCount int
+ eventCount atomic.Int64
quiet bool
}Then update usages:
- Line 106:
s.eventCount.Add(1) - Line 89:
s.eventCount.Load() - Line 182:
"events_received": s.eventCount.Load(),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/subscribe.go` around lines 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.
Comment on lines
+79
to
+84
| startErrCh := make(chan error, 1) | ||
| go func() { | ||
| startErrCh <- cli.Start(ctx) | ||
| }() | ||
|
|
||
| s.info("Connected. Waiting for events... (Ctrl+C to stop)") |
There was a problem hiding this comment.
"Connected" message printed before connection is confirmed.
The info message "Connected. Waiting for events..." is printed immediately after spawning the goroutine, but before cli.Start(ctx) has actually established the connection. If the connection fails, users will see "Connected" followed by an error, which is misleading.
💡 Suggestion
Consider either:
- Moving the "Connected" message inside the goroutine after
cli.Startsucceeds, or - Changing the message to "Connecting..." and printing "Connected" only after successful startup confirmation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/subscribe.go` around lines 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.
Comment on lines
+188
to
+193
| // info prints an info message if not quiet | ||
| func (s *EventSubscriber) info(msg string) { | ||
| if !s.quiet { | ||
| fmt.Println(msg) | ||
| } | ||
| } |
There was a problem hiding this comment.
Progress/info messages should go to stderr, not stdout.
The info() method uses fmt.Println which writes to stdout. Per coding guidelines, progress and informational messages should go to stderr to keep stdout clean for program output (JSON envelopes).
🐛 Suggested fix
// info prints an info message if not quiet
func (s *EventSubscriber) info(msg string) {
if !s.quiet {
- fmt.Println(msg)
+ fmt.Fprintln(os.Stderr, msg)
}
}Based on learnings: "Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr"
📝 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
| // info prints an info message if not quiet | |
| func (s *EventSubscriber) info(msg string) { | |
| if !s.quiet { | |
| fmt.Println(msg) | |
| } | |
| } | |
| // info prints an info message if not quiet | |
| func (s *EventSubscriber) info(msg string) { | |
| if !s.quiet { | |
| fmt.Fprintln(os.Stderr, msg) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/bot/subscribe.go` around lines 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.
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
Comment on lines
+63
to
+65
| resp, err := s.larkClient.Im.V1.Message.Create(ctx, req, | ||
| larkcore.WithTenantAccessToken(""), | ||
| ) |
There was a problem hiding this comment.
Both Create (line 63) and Reply (line 88) pass larkcore.WithTenantAccessToken(""), explicitly setting an empty string access token. This overrides the SDK's automatic token management configured when the client was created via lark.NewClient(appID, appSecret), so every API call will be rejected with an authentication error. Remove the explicit WithTenantAccessToken("") option from both calls to let the SDK use its internally managed tenant token.
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
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests
Chore