From 9e118e631f484ceddfec5333bdcad2cd98bc2ae0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Apr 2026 15:13:33 +0800 Subject: [PATCH] fix: review fixes for pre-release stability (H1-H3, M1-M7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: Kimi readLoop — move cmd.Wait() before EventResult to prevent out-of-order EventError after turn completion. H2: Kimi tests — add skipUnlessKimiAvailable guard so CI without kimi binary skips gracefully instead of failing. H3: Claude shellJoinArgs — add comprehensive unit tests for shell quoting (spaces, single quotes, backslashes, newlines). M1: cli_path — document that paths with spaces are not supported. M3: needsDoneReaction — add streaming preview tests covering UpdateMessage trigger, discard reset, and disabled state. M5: /agentsid — add security warning about session ID sensitivity in the command response. M7: i18n — localize reply_footer_remaining for ZH/ZH-TW/JA/ES; add MsgAgentSIDWarning translations for all 5 languages. Made-with: Cursor --- agent/claudecode/claudecode.go | 2 + agent/claudecode/session_test.go | 30 +++++++++++++ agent/kimi/kimi_test.go | 16 ++++++- agent/kimi/session.go | 38 +++++++++------- core/engine.go | 2 +- core/i18n.go | 16 +++++-- core/streaming_test.go | 77 ++++++++++++++++++++++++++++++++ 7 files changed, 160 insertions(+), 21 deletions(-) diff --git a/agent/claudecode/claudecode.go b/agent/claudecode/claudecode.go index 11bd147..516c1ea 100644 --- a/agent/claudecode/claudecode.go +++ b/agent/claudecode/claudecode.go @@ -111,6 +111,8 @@ func New(opts map[string]any) (core.Agent, error) { cliBin := "claude" var cliExtraArgs []string if cliPath, _ := opts["cli_path"].(string); cliPath != "" { + // NOTE: paths containing spaces are not supported because Fields + // splits on whitespace. Use a symlink or wrapper script instead. parts := strings.Fields(cliPath) cliBin = parts[0] if len(parts) > 1 { diff --git a/agent/claudecode/session_test.go b/agent/claudecode/session_test.go index 9647f6f..53f029a 100644 --- a/agent/claudecode/session_test.go +++ b/agent/claudecode/session_test.go @@ -224,6 +224,36 @@ func TestClaudeSessionClose_IdempotentNoPanic(t *testing.T) { } } +func TestShellJoinArgs(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + {"empty", nil, ""}, + {"single_plain", []string{"--verbose"}, "--verbose"}, + {"multiple_plain", []string{"--verbose", "--model", "opus"}, "--verbose --model opus"}, + {"arg_with_space", []string{"--prompt", "hello world"}, "--prompt 'hello world'"}, + {"arg_with_tab", []string{"a\tb"}, "'a\tb'"}, + {"arg_with_newline", []string{"line1\nline2"}, "'line1\nline2'"}, + {"arg_with_single_quote", []string{"it's"}, "'it'\\''s'"}, + {"arg_with_double_quote", []string{`say "hi"`}, `'say "hi"'`}, + {"arg_with_backslash", []string{`path\to`}, `'path\to'`}, + {"mixed", []string{"--flag", "has space", "plain", "it's here"}, "--flag 'has space' plain 'it'\\''s here'"}, + {"empty_string_arg", []string{""}, ""}, + {"long_prompt", []string{"--append-system-prompt", "You are a helpful assistant.\nBe concise."}, "--append-system-prompt 'You are a helpful assistant.\nBe concise.'"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shellJoinArgs(tt.args) + if got != tt.want { + t.Errorf("shellJoinArgs(%v)\n got = %q\n want = %q", tt.args, got, tt.want) + } + }) + } +} + func helperCommand(ctx context.Context, mode string) *exec.Cmd { cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestHelperProcess", "--", mode) cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") diff --git a/agent/kimi/kimi_test.go b/agent/kimi/kimi_test.go index 500438e..f8150fc 100644 --- a/agent/kimi/kimi_test.go +++ b/agent/kimi/kimi_test.go @@ -2,6 +2,7 @@ package kimi import ( "context" + "os/exec" "testing" "time" @@ -10,6 +11,13 @@ import ( "github.com/stretchr/testify/require" ) +func skipUnlessKimiAvailable(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("kimi"); err != nil { + t.Skipf("kimi CLI not in PATH, skipping: %v", err) + } +} + func TestNormalizeMode(t *testing.T) { cases := []struct { input string @@ -34,7 +42,7 @@ func TestNormalizeMode(t *testing.T) { } func TestAgentNew(t *testing.T) { - // This test assumes `kimi` binary exists in PATH (true in this environment). + skipUnlessKimiAvailable(t) agentInf, err := New(map[string]any{ "work_dir": "/tmp", "model": "kimi-k2", @@ -52,6 +60,7 @@ func TestAgentNew(t *testing.T) { } func TestAgentSetters(t *testing.T) { + skipUnlessKimiAvailable(t) agentInf, err := New(map[string]any{"work_dir": "/tmp"}) require.NoError(t, err) a := agentInf.(*Agent) @@ -67,6 +76,7 @@ func TestAgentSetters(t *testing.T) { } func TestAgentPermissionModes(t *testing.T) { + skipUnlessKimiAvailable(t) agentInf, err := New(map[string]any{"work_dir": "/tmp"}) require.NoError(t, err) a := agentInf.(*Agent) @@ -80,6 +90,7 @@ func TestAgentPermissionModes(t *testing.T) { } func TestAgentProviderSwitcher(t *testing.T) { + skipUnlessKimiAvailable(t) agentInf, err := New(map[string]any{"work_dir": "/tmp"}) require.NoError(t, err) a := agentInf.(*Agent) @@ -100,6 +111,7 @@ func TestAgentProviderSwitcher(t *testing.T) { } func TestAgentStartSession(t *testing.T) { + skipUnlessKimiAvailable(t) agentInf, err := New(map[string]any{ "work_dir": "/tmp", "model": "kimi-k2", @@ -123,6 +135,7 @@ func TestAgentStartSession(t *testing.T) { } func TestAgentMemoryAndSkill(t *testing.T) { + skipUnlessKimiAvailable(t) agentInf, err := New(map[string]any{"work_dir": "/tmp/my-project"}) require.NoError(t, err) a := agentInf.(*Agent) @@ -137,6 +150,7 @@ func TestAgentMemoryAndSkill(t *testing.T) { } func TestAgentAvailableModels(t *testing.T) { + skipUnlessKimiAvailable(t) agentInf, err := New(map[string]any{"work_dir": "/tmp"}) require.NoError(t, err) a := agentInf.(*Agent) diff --git a/agent/kimi/session.go b/agent/kimi/session.go index 6a0353e..70ba282 100644 --- a/agent/kimi/session.go +++ b/agent/kimi/session.go @@ -200,18 +200,6 @@ func (ks *kimiSession) readLoop(ctx context.Context, cmd *exec.Cmd, stdout io.Re for _, f := range tempFiles { os.Remove(f) } - if err := cmd.Wait(); err != nil { - stderrMsg := strings.TrimSpace(stderrBuf.String()) - if stderrMsg != "" { - slog.Error("kimiSession: process failed", "error", err, "stderr", stderrMsg) - evt := core.Event{Type: core.EventError, Error: fmt.Errorf("%s", stderrMsg)} - select { - case ks.events <- evt: - case <-ks.ctx.Done(): - return - } - } - } }() go func() { @@ -222,6 +210,7 @@ func (ks *kimiSession) readLoop(ctx context.Context, cmd *exec.Cmd, stdout io.Re scanner := bufio.NewScanner(stdout) scanner.Buffer(make([]byte, 0, 64*1024), 10*1024*1024) + var scanErr error for scanner.Scan() { line := scanner.Text() if line == "" { @@ -247,10 +236,15 @@ func (ks *kimiSession) readLoop(ctx context.Context, cmd *exec.Cmd, stdout io.Re ks.handleEvent(raw) } + scanErr = scanner.Err() - if err := scanner.Err(); err != nil { - slog.Error("kimiSession: scanner error", "error", err) - evt := core.Event{Type: core.EventError, Error: fmt.Errorf("read stdout: %w", err)} + // Wait for process exit before sending any terminal event so the engine + // never sees EventError after EventResult from the same turn. + waitErr := cmd.Wait() + + if scanErr != nil { + slog.Error("kimiSession: scanner error", "error", scanErr) + evt := core.Event{Type: core.EventError, Error: fmt.Errorf("read stdout: %w", scanErr)} select { case ks.events <- evt: case <-ks.ctx.Done(): @@ -258,6 +252,20 @@ func (ks *kimiSession) readLoop(ctx context.Context, cmd *exec.Cmd, stdout io.Re } } + if waitErr != nil { + stderrMsg := strings.TrimSpace(stderrBuf.String()) + if stderrMsg != "" { + slog.Error("kimiSession: process failed", "error", waitErr, "stderr", stderrMsg) + evt := core.Event{Type: core.EventError, Error: fmt.Errorf("%s", stderrMsg)} + select { + case ks.events <- evt: + case <-ks.ctx.Done(): + return + } + return + } + } + // Flush any remaining pending messages as text and send result event. ks.flushPendingAsText() evt := core.Event{Type: core.EventResult, SessionID: ks.CurrentSessionID(), Done: true} diff --git a/core/engine.go b/core/engine.go index 841e05f..53ab301 100644 --- a/core/engine.go +++ b/core/engine.go @@ -4777,7 +4777,7 @@ func (e *Engine) cmdAgentSID(p Platform, msg *Message) { e.reply(p, msg.ReplyCtx, e.i18n.T(MsgSessionNotStarted)) return } - e.reply(p, msg.ReplyCtx, agentID) + e.reply(p, msg.ReplyCtx, e.i18n.T(MsgAgentSIDWarning)+"\n\n`"+agentID+"`") } func (e *Engine) cmdStatus(p Platform, msg *Message) { diff --git a/core/i18n.go b/core/i18n.go index 018d97a..f0656e1 100644 --- a/core/i18n.go +++ b/core/i18n.go @@ -241,6 +241,7 @@ const ( MsgStatusTitle MsgKey = "status_title" MsgReplyFooterRemaining MsgKey = "reply_footer_remaining" + MsgAgentSIDWarning MsgKey = "agent_sid_warning" MsgModelCurrent MsgKey = "model_current" MsgModelChanged MsgKey = "model_changed" @@ -1820,10 +1821,17 @@ var messages = map[MsgKey]map[Language]string{ }, MsgReplyFooterRemaining: { LangEnglish: "%d%% left", - LangChinese: "%d%% left", - LangTraditionalChinese: "%d%% left", - LangJapanese: "%d%% left", - LangSpanish: "%d%% left", + LangChinese: "剩余 %d%%", + LangTraditionalChinese: "剩餘 %d%%", + LangJapanese: "残り %d%%", + LangSpanish: "%d%% restante", + }, + MsgAgentSIDWarning: { + LangEnglish: "⚠️ Agent session ID (sensitive — do not share in public channels):", + LangChinese: "⚠️ Agent 会话 ID（敏感信息，请勿在公开频道分享）：", + LangTraditionalChinese: "⚠️ Agent 會話 ID（敏感資訊，請勿在公開頻道分享）：", + LangJapanese: "⚠️ エージェントセッションID（機密情報 — 公開チャンネルで共有しないでください）：", + LangSpanish: "⚠️ ID de sesión del agente (sensible — no compartir en canales públicos):", }, MsgModelCurrent: { LangEnglish: "Current model: %s", diff --git a/core/streaming_test.go b/core/streaming_test.go index 94e994e..62887e4 100644 --- a/core/streaming_test.go +++ b/core/streaming_test.go @@ -294,6 +294,83 @@ func TestStreamPreview_FinishKeepsPreviewWhenPlatformPrefersInPlaceFinalize(t *t } } +func TestStreamPreview_NeedsDoneReaction_TrueAfterUpdate(t *testing.T) { + mp := &mockUpdaterPlatform{} + cfg := StreamPreviewCfg{ + Enabled: true, + IntervalMs: 50, + MinDeltaChars: 1, + MaxChars: 500, + } + + sp := newStreamPreview(cfg, mp, "ctx", context.Background(), nil) + + if sp.needsDoneReaction() { + t.Error("needsDoneReaction should be false before any send") + } + + sp.appendText("Hello World") + time.Sleep(100 * time.Millisecond) + + if sp.needsDoneReaction() { + t.Error("needsDoneReaction should be false after only SendPreviewStart (no UpdateMessage yet)") + } + + sp.appendText(" more text to trigger update") + time.Sleep(100 * time.Millisecond) + + msgs := mp.getMessages() + hasUpdate := false + for _, m := range msgs { + if len(m) > 7 && m[:7] == "update:" { + hasUpdate = true + break + } + } + if !hasUpdate { + t.Fatal("expected at least one UpdateMessage call") + } + + if !sp.needsDoneReaction() { + t.Error("needsDoneReaction should be true after UpdateMessage was used") + } +} + +func TestStreamPreview_NeedsDoneReaction_FalseAfterDiscard(t *testing.T) { + mp := &mockUpdaterPlatform{} + cfg := StreamPreviewCfg{ + Enabled: true, + IntervalMs: 50, + MinDeltaChars: 1, + MaxChars: 500, + } + + sp := newStreamPreview(cfg, mp, "ctx", context.Background(), nil) + sp.appendText("Hello World") + time.Sleep(100 * time.Millisecond) + sp.appendText(" more text") + time.Sleep(100 * time.Millisecond) + + sp.discard() + + if sp.needsDoneReaction() { + t.Error("needsDoneReaction should be false after discard (previewMsgID cleared)") + } +} + +func TestStreamPreview_NeedsDoneReaction_FalseWhenDisabled(t *testing.T) { + mp := &mockUpdaterPlatform{} + cfg := StreamPreviewCfg{Enabled: false} + + sp := newStreamPreview(cfg, mp, "ctx", context.Background(), nil) + sp.appendText("Hello") + time.Sleep(100 * time.Millisecond) + + if sp.needsDoneReaction() { + t.Error("needsDoneReaction should be false when preview is disabled") + } +} + func TestStreamPreview_AppliesTransform(t *testing.T) { mp := &mockUpdaterPlatform{} cfg := StreamPreviewCfg{