fix: review fixes for pre-release stability (H1-H3, M1-M7)
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
This commit is contained in:
1 parent
66efe3256f
commit
9e118e631f
7 files changed
+160
-21
No files matched your search
@@ -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 {
|
||||
|
||||
@@ -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")
|
||||
|
||||
+15
-1
@@ -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)
|
||||
|
||||
+23
-15
@@ -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}
|
||||
|
||||
+1
-1
@@ -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) {
|
||||
|
||||
+12
-4
@@ -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",
|
||||
|
||||
@@ -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{
|
||||
|
||||
Reference in new issue
Block a user