Add Kimi CLI driver - #113
Conversation
|
@simone-stacks, thank you for the PR! Planning to test (and potentially merge soon). 🚀 |
|
@simone-stacks Hello from the other side! 👋 I'm going to start looking into your PRs. First thing I noticed is a flaky test (unrelated to this PR) that we're going to fix in a separate PR. Next I'll be running some local testing and eyeball your changes. I'll also spin up some review agents to make sure we're not missing anything. |
radubahmata
left a comment
radubahmata
left a comment
There was a problem hiding this comment.
Huge work! Some comments:
| agent_is_retriable() { | ||
| local logfile="$1" | ||
| local _rate='429\|rate.limit\|too many requests\|quota\|usage.limit\|hit your.*limit' | ||
| local _transient='connection reset\|connection closed\|connection refused\|gateway timeout\|bad gateway\|service unavailable\|\b50[234]\b\|timed out\|temporarily unavailable\|at capacity\|overloaded' | ||
| for f in "$logfile" "${logfile}.err"; do | ||
| [ -f "$f" ] || continue | ||
| grep -qi "$_rate" "$f" 2>/dev/null \ | ||
| && echo "rate_limited" && return | ||
| grep -qi "$_transient" "$f" 2>/dev/null \ | ||
| && echo "transient" && return | ||
| done | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
The classifier misses retriable errors such as provider.connection_error and HTTP 408/409/500. After the harness' initial generic zero-token retry, a repeated connection error is treated as fatal.
Kimi 0.40.1 also treats connection errors and HTTP 408, 409, and 500 as retriable. The Kimi driver does not recognize those structured errors: https://github.com/MoonshotAI/kimi-code/blob/a7087694bb9dcc83c60c01fe51eb8c250d4656af/packages/agent-core-v2/src/kosong/contract/errors.ts#L234-L250
The fix would be to recognize Kimi's structured connection error and supported HTTP statuses.
Can you please also add some regression tests for provider.connection_error and HTTP 408, 409, and 500 in the existing Kimi retry-classification tests?
| EOF | ||
| RETRY_OUT=$(agent_is_retriable "$TMPDIR/kimi-503.jsonl" 1) | ||
| assert_not_empty "kimi 503 is retriable" "$RETRY_OUT" | ||
|
|
There was a problem hiding this comment.
These should be good enough as regression tests wrt the prev comment. Ran the suite locally and it fails. Let's do a TDD approach: please accept the suggestion so we can see the CI red, then turn green as soon as fix is pushed:
| : > "$TMPDIR/kimi-connection.jsonl" | |
| cat > "$TMPDIR/kimi-connection.jsonl.err" <<'EOF' | |
| error: failed to run prompt: provider.connection_error: network error | |
| EOF | |
| RETRY_OUT=$(agent_is_retriable "$TMPDIR/kimi-connection.jsonl" 1) | |
| assert_eq "kimi connection error is transient" "transient" "$RETRY_OUT" | |
| for status in 408 409 500; do | |
| printf '{"role":"meta","type":"turn.step.retrying",' > "$TMPDIR/kimi-${status}.jsonl" | |
| printf '"status_code":%s}\n' "$status" >> "$TMPDIR/kimi-${status}.jsonl" | |
| RETRY_OUT=$(agent_is_retriable "$TMPDIR/kimi-${status}.jsonl" 1) | |
| assert_eq "kimi ${status} is transient" "transient" "$RETRY_OUT" | |
| done | |
| Groups with `api_key` or `auth_token` ignore the `auth` | ||
| field; their custom credential is always used. When neither | ||
| is set, `auth` determines which host credential to inject. |
There was a problem hiding this comment.
Noticed that the Kimi driver's pattern matching found here contradicts with this statement. Thought to add the comment there, but after further digging I realized that this holds only for Claude Code, while all remaining drivers clear the API key for auth: oauth/chatgpt. It's a nit, but since this is directly agent-facing and may lead to misleading swarmfiles I think we should fix/drop it.
| agent_extract_stats() { | ||
| local logfile="$1" | ||
| local turns | ||
| turns=$(grep -c '"role"[[:space:]]*:[[:space:]]*"assistant"' \ | ||
| "$logfile" 2>/dev/null || true) | ||
| turns="${turns:-0}" | ||
| printf "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s" \ | ||
| "0" "0" "0" "0" "0" "0" "0" "$turns" | ||
| } |
There was a problem hiding this comment.
Kimi's stream-json output does not include usage, but its LLM requester records provider-reported usage as usage.record entries. You can check this session fixture, it shows these entries serialized in agents/*/wire.jsonl with input, output, cache-read, and cache-creation counts. This function ignores those records and always returns zero; the harness derives pricing from those fields, so every Kimi run remains 0 regardless of actual usage.
Please resolve the emitted session ID and aggregate its usage.record entries across every agent wire file before returning stats. We should also add regression fixtures covering this.
3 participants
Summary
kimi-clidriver so swarms can run Kimi Code CLI alongside the existing drivers.kimi_cli_versionpin.Changes
lib/drivers/kimi-cli.sh: new driver implementing the swarm interface.Dockerfile+launch.sh: install Kimi CLI and exposekimi_cli_version.dashboard.sh,USAGE.md,README.md,CHANGELOG.md: surface the new driver.tests/: add driver/config/dashboard/harness/launch coverage and Kimi fixture swarmfiles.lib/upstream-clone.sh,lib/harness.sh,lib/interactive.sh: retry upstream clones on transient failures.tests/test_upstream_clone.sh: unit tests for the clone helper.Self-review checklist
shellcheck -s bashpasses on all modified.shfiles.Test plan
shellcheck -s bash lib/upstream-clone.sh lib/harness.sh lib/interactive.sh lib/drivers/kimi-cli.sh tests/test_upstream_clone.shtests/test_upstream_clone.shtests/test_harness.shtests/test_drivers.sh./tests/test.sh --unit(note:test_harvest.shandtest_manual_interactive_e2e.shhave pre-existing issues on this branch)