fix(kilocode): detect the installed CLI instead of assuming one (#1015) - #1024
fix(kilocode): detect the installed CLI instead of assuming one (#1015)#1024frankbria wants to merge 3 commits into
Conversation
@kilocode/cli was rewritten between 0.22.0 (2026-01-15) and 7.x (2026-07-29) —
213 releases apart — and the two invocations share nothing:
0.22.0 7.4.17
usage kilocode [options] [prompt] kilo run [message..]
workspace --workspace <path> --dir <path>
--auto "run in autonomous mode" "auto-approve ALL permissions"
That last row is why this could not be the rename the issue described. On 7.x
`--auto` *is* the old `--yolo` — the permission bypass #916 established must
stay off — so carrying the flag across would have silently upgraded the adapter
into a bypass while looking like a cosmetic fix. `kilo run` is non-interactive
on its own, exactly like `opencode run`, so the flag is not needed at all.
Which invocation to build is detected from the CLI's own `--help`, never from a
version string (the issue's AC3, and this repo has been bitten three times by
adapters that assumed a surface: #913, #914, #1012). Detection is cached per
binary for the process, and an unreadable --help falls back to modern — the
version a new install gets, and the one whose `run` fails loudly rather than
opening the TUI that hung until timeout in #1012.
Version floor is 7.x, documented on the class. 0.22.0 is still driven correctly
because #1012 verified it end-to-end and an unupgraded machine should not
silently break.
Oversized prompts: Linux caps one argv entry at 128 KiB, under CodeFrame's
~100K-token budget, so on 7.x the prompt moves to stdin. `kilo run` accepts it
there when given no positional — verified, not assumed: `echo "say ok" | kilo
run --dir /tmp` reaches the model call. 0.22.0 has no stdin path, so there it
stays positional and fails loudly.
Both surfaces verified live by installing each version in turn. Detection is
tested against verbatim captured `--help` from both real CLIs, checked in under
fixtures/kilocode_help/ (the #914 schema-fixture pattern) rather than text
invented for the test.
The two #1012 tests that assert the legacy form now pin that surface explicitly,
since which command is built no longer depends on the adapter alone.
…il (#1015) AC4. The tier now expects `kilo run` + `--dir` via #1007's help_subcommand — kilocode's --dir lives under `run`, exactly like opencode's, so the top-level help the tier used to read could not see it. The xfail added in #1014 is gone: with the adapter migrated it passes on its own, which is what strict=False was there to allow. known_drift is now unused by every engine. Kept, with a note: three of four engines drifted in a single week, so the next one is a matter of time and re-deriving the mechanism then would be wasted work. The contract failure message now also names the version floor, since a machine below it (this one was on 0.22.0) sees the same missing-flag assertion as real upstream drift.
|
Claude finished @frankbria's task in 5m 26s —— View job 🔍 Bug-hunting review — completeScoped to concrete defects only (logic, null/None, security, data loss, contract breaks, resource leaks). Style/architecture/naming/coverage are covered by the general review.
1 finding — details and a No other in-scope defects. In particular, checked and found consistent:
|
|
|
||
| try: | ||
| proc = subprocess.run( | ||
| [binary_path, "--help"], capture_output=True, text=True, timeout=90 |
There was a problem hiding this comment.
[major] _detect_surface reads kilo --help with text=True (strict decode), so on a POSIX / LANG=C locale the modern 7.x banner raises UnicodeDecodeError — which is not caught by (OSError, subprocess.SubprocessError) — and propagates out of build_command/run() instead of falling back to modern.
Failure scenario: CodeFrame runs under LANG=C (the default on many Docker / slim base images, where locale.getpreferredencoding() is ANSI_X3.4-1968 = ASCII), kilocode 7.x — the supported floor — is installed, and a task starts via the kilocode engine. build_command → _detect_surface → subprocess.run([kilo, "--help"], text=True); kilo prints its Unicode banner (the first line of the checked-in help-7.4.17.txt is ██ ██ ██🬺🬏 …), the strict ASCII decode raises on the first 0xE2 byte, and because build_command runs outside run()'s try/except (subprocess_adapter.py:145), the exception crashes the task with a traceback rather than running it. The PR's own tests miss this because _with_help returns an already-decoded str, bypassing the decode step. Detection only needs the ASCII "kilo run" marker, so errors="replace" is safe and never raises.
| [binary_path, "--help"], capture_output=True, text=True, timeout=90 | |
| [binary_path, "--help"], capture_output=True, text=True, errors="replace", timeout=90 |
Closes #1015.
Not the rename it looked like
@kilocode/cliwas rewritten between 0.22.0 (2026-01-15) and 7.x (2026-07-29) — 213 releases apart:kilocode [options] [prompt]kilo run [message..]--workspace <path>--dir <path>--autoThat last row is the reason this could not be a
--workspace→--dirrename. On 7.x,--autois the old--yolo— the permission bypass #916 established must stay off. Carrying the flag across would have silently upgraded the adapter into a full bypass while looking like a cosmetic fix.kilo runis non-interactive on its own, exactly likeopencode run, so the flag is not needed at all. Both eras now end up without a bypass, by different means, and a test pins that.Detection, not assumption
AC3 rules out guessing from a version string, and this repo has been bitten three times by adapters that assumed a CLI surface (#913, #914, #1012). So
_detect_surfacereads the CLI's own--helpand looks for therunsubcommand. A test pins the distinction: a legacy CLI reporting version7.9.9is still driven the legacy way.Cached per binary for the process —
build_commandruns per task and would otherwise add a subprocess to every run. An unreadable--helpfalls back to modern: it is what a new install gets, and itsrunfails loudly rather than opening the TUI that hung until timeout in #1012.Version floor (AC1)
7.x, documented on the adapter class. 0.22.0 is still driven correctly, because #1012 verified it end-to-end and an unupgraded machine should not silently break — but it is not the target and can be dropped once nobody is on it.
Oversized prompts
Linux caps one argv entry at 128 KiB, under CodeFrame's ~100K-token budget. On 7.x the prompt moves to stdin, which
kilo runaccepts when given no positional — verified, not assumed:0.22.0 has no stdin path, so there it stays positional and fails loudly rather than silently doing nothing.
Verified against both real CLIs
Installed each version in turn and ran the adapter against it:
Detection is tested against verbatim captured
--helpfrom both CLIs, checked in underfixtures/kilocode_help/(the #914 schema-fixture pattern) rather than text invented for the test.Tier update (AC4)
kilocode's
--dirlives underrun, like opencode's, so the tier needed #1007'shelp_subcommandto see it — which is why this PR waited on #1022. The tier now pinskilo run+--dir, and the xfail added in #1014 is gone: with the adapter migrated it passes on its own, which is whatstrict=Falsewas there to allow.known_driftis now unused by every engine. Kept, with a note — three of four engines drifted in a single week, so the next one is a matter of time.The contract failure message now also names the version floor, since a machine below it sees the same missing-flag assertion as real upstream drift. (Mine did, on 0.22.0.)
Testing
tests/core/adapters/: 313 passed,ruffclean, tier green with kilo 7.4.17 installed. The two #1012 tests that assert the legacy form now pin that surface explicitly, since which command is built no longer depends on the adapter alone.