Feat/copilot#12
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new interactive TUI shell called agentline along with a comprehensive integration of the GitHub Copilot SDK, adding commands like fastgit copilot and local skill management. It also adds fastgit push and fastgit pull commands. Key feedback focuses on critical concurrency bugs, including a data race in Bubble Tea state updates and overwritten cancel functions in the ACP bridge. Additionally, memory leaks were identified in the permission and question brokers due to uncleaned pending requests upon context cancellation, alongside a Go-specific logic bug where a defer statement fails to modify an anonymous return value, and usability issues regarding real-time event streaming and silent command exits.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return &agentacp.CallbackClient{ | ||
| PermissionBroker: m.permissionBroker, | ||
| OnSessionUpdate: func(_ context.Context, params acp.SessionNotification) error { | ||
| m.appendACPSessionNotification(params) | ||
| return nil | ||
| }, | ||
| } |
There was a problem hiding this comment.
CRITICAL CONCURRENCY BUG: OnSessionUpdate 回调函数是在 Copilot SDK 的后台 goroutine 中异步执行的。在这里直接调用 m.appendACPSessionNotification(params) 会直接在后台线程中修改 agentlineModel 的状态（如 m.blocks、m.acpEventEntries 等），而 Bubble Tea 的主协程同时在 Update 和 View 中读取和修改这些状态。这会导致严重的数据竞争（Data Race），在高频事件流下极易导致程序崩溃（如 concurrent map read/write 或 slice bounds out of range）。\n\n解决方案：Bubble Tea 官方推荐的后台通信方式是通过 tea.Program.Send(msg) 发送消息。建议在 agentlineModel 中保存 tea.Program 的引用，或者提供一个线程安全的事件通道，在 OnSessionUpdate 中将事件作为 tea.Msg 发送给 Bubble Tea，然后在 Update 方法中安全地处理该消息并更新模型状态。
| promptCtx, cancel := context.WithCancel(ctx) | ||
| b.mu.Lock() | ||
| state.cancel = cancel | ||
| b.mu.Unlock() |
There was a problem hiding this comment.
在 Prompt 方法中，如果针对同一个 SessionId 并发调用 Prompt，后一个调用会直接覆盖 state.cancel。当先前的调用完成并执行 defer 时，它会将 current.cancel 置为 nil，这会导致后一个调用的 cancel 函数丢失且无法被外部 Cancel 中断。\n\n建议在设置新的 cancel 前，先检查并取消（或拒绝）已有的活跃 cancel，或者使用更安全的并发控制机制。
| b.mu.Lock() | ||
| b.pending = append(b.pending, req) | ||
| b.mu.Unlock() | ||
|
|
||
| select { | ||
| case resp := <-req.respCh: | ||
| b.remove(req.id) | ||
| return resp, nil | ||
| case <-ctx.Done(): | ||
| _ = b.ResolveCancelled(req.id) | ||
| return acp.RequestPermissionResponse{Outcome: acp.NewRequestPermissionOutcomeCancelled()}, nil | ||
| } |
There was a problem hiding this comment.
在 RequestPermission 中，如果 ctx.Done() 分支被触发，虽然调用了 ResolveCancelled，但从未调用 b.remove(req.id)。这会导致已取消的权限请求永久残留在 b.pending 列表中，造成内存泄漏和状态污染。\n\n建议使用 defer b.remove(req.id) 来确保无论通过哪个分支退出，请求都能被正确清理。
| b.pending = append(b.pending, q) | ||
| b.mu.Unlock() | ||
|
|
||
| select { | ||
| case resp := <-q.respCh: | ||
| b.remove(q.id) | ||
| return resp, nil | ||
| case <-ctx.Done(): | ||
| _ = b.Cancel(q.id) | ||
| return AskResponse{Cancelled: true}, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
与 PermissionBroker 类似，在 QuestionBroker.Request 中，如果 ctx.Done() 分支被触发，从未调用 b.remove(q.id)。这会导致已取消的问题请求永久残留在 b.pending 列表中，造成内存泄漏。\n\n建议使用 defer b.remove(q.id) 来确保请求在函数退出时被正确清理。
| runInv.Annotations[InteractionAnnotationKey] = &runtimeInteractionBridge{ | ||
| emitFn: func(_ context.Context, event InteractionEvent) error { | ||
| kind := interactionKindToBlockKind(event.Kind) | ||
| title := strings.TrimSpace(event.Title) | ||
| if title == "" { | ||
| title = "interaction" | ||
| } | ||
| lines := append([]string(nil), event.Lines...) | ||
| if len(lines) == 0 { | ||
| lines = []string{"(empty)"} | ||
| } | ||
| blocks = append(blocks, sessionBlock{Kind: kind, Title: title, Lines: lines}) | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
USABILITY / REAL-TIME STREAMING ISSUE: 在 emitFn 中，收到的中间态交互事件（InteractionEvent）只是被简单地追加到了局部的 blocks 切片中。由于该切片只有在整个命令执行完毕（runInv.Run() 退出）后才会通过 runResultMsg 返回给 UI，这导致所有的中间过程事件无法实时渲染，用户必须等待命令完全结束后才能一次性看到所有输出。\n\n建议：应该通过 tea.Program.Send 实时向 Bubble Tea 发送单条事件消息（例如 appendBlockMsg），让 UI 能够即时滚动更新，提供更好的实时交互反馈。
| func hydrateSession(ctx context.Context, client *copilot.Client, sessionID string, cfg hydrateConfig) hydrateSessionInfo { | ||
| info := hydrateSessionInfo{maxEvents: cfg.maxEvents} | ||
| if !cfg.enabled || sessionID == "" { | ||
| return info | ||
| } |
There was a problem hiding this comment.
在 hydrateSession 函数中，返回值 hydrateSessionInfo 是匿名的。\n\n函数内部定义了局部变量 info，并在 defer 中尝试修改 info.errorText。但是在 Go 语言中，如果返回值是匿名的，在执行 return info 时，Go 会先对 info 进行求值并复制到返回值栈空间，随后才执行 defer 函数。因此，defer 中对 info.errorText 的修改完全不会影响最终的返回值，导致所有的 Disconnect 错误被静默忽略。\n\n解决方案：将返回值命名为 info，这样 defer 中的修改就能正确作用于最终的返回值。
| isDirty := utils.IsDirty().Unwrap() | ||
| if isDirty { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
如果仓库是 dirty 状态，fastgit push 会直接静默返回 nil，不执行任何推送操作也不给用户任何提示。这可能会让用户感到困惑（以为推送成功了，或者不知道为什么没有推送）。\n\n建议在检测到 dirty 时，至少向用户输出一条提示信息（例如使用 log.Warn 或输出到 Stdout），说明因为存在未提交的改动而跳过了推送。
1 participant
No description provided.