Add Kimi CLI driver - #113
simone-stacks wants to merge 10 commits into
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.
|
@simone-stacks Here's where I pushed the smoke tests, looking good: simone-stacks/claude-swarm@feat/kimi-cli-driver...radubahmata:claude-swarm:kimi-smoke |
| agent_detect_fatal() { | ||
| local logfile="$1" | ||
|
|
||
| if [ -f "${logfile}.err" ]; then | ||
| local err_msg | ||
| err_msg=$(grep -i 'error\|invalid.*key\|unauthorized' \ | ||
| "${logfile}.err" 2>/dev/null \ | ||
| | head -1 || true) | ||
| if [ -n "$err_msg" ] && \ | ||
| ! grep -q '"role"[[:space:]]*:[[:space:]]*"assistant"' \ | ||
| "$logfile" 2>/dev/null; then | ||
| echo "$err_msg" | ||
| fi | ||
| fi | ||
| } |
There was a problem hiding this comment.
Just found this with Codex and I'm pretty sure I remember this happening during real runs:
agent_detect_fatal ignores the exit code passed by the harness and returns a stderr error only when the log contains no assistant message. Kimi can emit assistant or tool output during an early step, then exit nonzero when a later provider request fails. In that case the detector returns no fatal error.
This PR's zero-valued statistics currently cause the harness's generic nonzero-exit and zero-token check to catch this indirectly. The stacked PR #114 adds real usage from Kimi's wire log. Once an earlier step has recorded tokens, the zero-token fallback no longer applies, so the combined branch treats the incomplete run as normal and may ship partial commits before eventually exiting successfully at the idle limit.
Direct reproduction with one assistant event, a nonzero exit, wire usage, and error: failed to run prompt: provider.connection_error: Connection error. produced an empty fatal result.
Use the supplied exit code. When Kimi exits nonzero, surface its terminal stderr error even if earlier assistant output exists; continue ignoring nonfatal stderr warnings on successful exits. Add a regression test with assistant output, a nonzero exit, and nonzero wire usage through the harness path.
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)