feat: opencode-adapter#135
Conversation
Add OpenCode as a registered provider-agnostic CLI adapter and parser, with mocked coverage for command construction, resume sessions, install checks, parser events, and registry lookup. OpenCode was not installed locally, so CLI flags were verified against public OpenCode docs/source and runtime behavior is covered with unit tests; full verification required applying existing Ruff cleanup across Python tests/modules. Made-with: Cursor
…-adapter Made-with: Cursor # Conflicts: # tests/cli/test_single_writer.py
Capture OpenCode step_start session IDs before filtering noise and pass prompts after an end-of-options sentinel so prompt text beginning with dashes is not parsed as flags. Also address the requested cleanup items around install, clean typing, test timeouts, tmp_path usage, and git branch validation. Made-with: Cursor
Only suppress invalid JSON and filesystem errors when loading Gemini settings so unexpected failures can surface during configuration handling. Made-with: Cursor
Handle step_start as its own session-init event instead of also classifying it as parser noise, preserving session IDs regardless of future parse check ordering. Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces a new Kimi adapter for CLI code generation, refactors command modules by removing unused imports and improving subprocess error handling, standardises code formatting across the codebase, and updates configuration to support the new Kimi model integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
python/cube/core/updater.py (1)
32-38: Leverage Git's native validation instead of a custom branch ruleset.The current
is_safe_git_branchfunction is overly permissive. It incorrectly allows branch names that Git would reject—such as those with a leading slash (/foo), double slashes (foo//bar), or trailing dots (foo.)—because the final character-by-character check doesn't account for these patterns. Consider delegating validation togit check-ref-format --branchto ensure parity with Git's own reference validation and avoid maintaining a custom ruleset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/core/updater.py` around lines 32 - 38, Replace the custom validation in is_safe_git_branch with a call to Git's own validator: invoke "git check-ref-format --branch <branch>" (e.g., via subprocess.run) and return True only if the command exits successfully; return False on non-zero exit or exceptions. Ensure you still reject empty/None branch inputs before calling Git and catch OSError/subprocess.CalledProcessError to safely return False (or decide fallback behavior), and keep the function signature is_safe_git_branch(branch: str) intact.python/test_all_commands.py (1)
11-11: Refactorrun_cmdto use argument lists instead of shell=True.The function at line 11 uses
shell=Truewithsubprocess.run(), which is unnecessary for static, locally-defined test commands. Rewrite the function to pass command arguments as a list withshell=Falseto avoid shell parsing overhead and improve robustness.Example refactor:
def run_cmd(cmd, should_fail=False): """Run a command and check result.""" # For list-based calls result = subprocess.run(cmd, shell=False, capture_output=True, text=True) # Update test commands to use lists: ["python3", "-m", "cube.cli", "--version"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/test_all_commands.py` at line 11, The test helper run_cmd currently calls subprocess.run(cmd, shell=True); change it to call subprocess.run with shell=False and expect cmd to be a sequence (list/tuple) so subprocess runs without a shell (e.g., subprocess.run(cmd, shell=False, capture_output=True, text=True)); update all callers/tests that pass string commands to pass argument lists instead (reference the run_cmd function and any test that invokes it) or add a single normalization inside run_cmd to raise if cmd is a str to force callers to use lists.tests/cli/test_adapters.py (1)
65-71: Minor tidy-up: use iterable unpacking for side-effect listThis will satisfy Ruff
RUF005and keep the helper concise.Proposed fix
- mock_process.stdout.read.side_effect = list(chunks) + [b""] + mock_process.stdout.read.side_effect = [*chunks, b""]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_adapters.py` around lines 65 - 71, The helper make_mock_process sets mock_process.stdout.read.side_effect to list(chunks) + [b""], which triggers RUF005; change it to use iterable unpacking by assigning side_effect to [*chunks, b""] so the list is created succinctly and satisfies the linter—update the reference in make_mock_process where mock_process.stdout.read.side_effect is set.python/cube/core/parsers/__init__.py (1)
10-10: Sort__all__to satisfy RuffRUF022Line 10 currently trips the unsorted
__all__rule. Sorting this list avoids avoidable lint noise.Proposed fix
-__all__ = ["ParserAdapter", "ClaudeParser", "CursorParser", "GeminiParser", "CLIReviewParser", "OpenCodeParser"] +__all__ = ["CLIReviewParser", "ClaudeParser", "CursorParser", "GeminiParser", "OpenCodeParser", "ParserAdapter"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/core/parsers/__init__.py` at line 10, The __all__ list in python/cube/core/parsers/__init__.py is unsorted and triggers Ruff RUF022; fix it by alphabetically sorting the exported names in the __all__ assignment (e.g., ensure "CLIReviewParser", "ClaudeParser", "CursorParser", "GeminiParser", "OpenCodeParser", "ParserAdapter" are ordered lexicographically) so the __all__ sequence is consistently sorted and the lint rule is satisfied.python/cube/core/adapters/opencode.py (1)
109-117: Tighten fallback error detection to reduce false positivesLine 115 currently treats any line containing
"error"/"failed"as an error, even when it is normal assistant/tool content. That can overwritelast_errorwith misleading context.Proposed fix
if isinstance(data, dict): error = data.get("error") if isinstance(error, dict): text = " ".join(str(value) for value in OpenCodeAdapter._flatten_error_values(error)) elif error: text = str(error) elif data.get("type") == "error": text = str(data) + else: + return None lower = text.lower() if any(token in lower for token in ("auth", "unauthorized", "forbidden", "api key", "login")): return "Authentication required" if "model" in lower and any(token in lower for token in ("not found", "unknown", "invalid", "unsupported")): return "Model unavailable" if "provider" in lower and any(token in lower for token in ("not found", "unknown", "invalid", "missing")): return "Model unavailable" if "error" in lower or "failed" in lower: return text[:200]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/core/adapters/opencode.py` around lines 109 - 117, The generic check that returns text[:200] whenever "error" or "failed" appears is too broad; change the condition using the existing variables lower and text so we only treat it as an error when "error"/"failed" appears with additional error context (e.g., presence of "exception", "traceback", "code", "status", "http", "stack", or when followed by ":" or appears at the start of the string), otherwise return None; update the branch that currently reads if "error" in lower or "failed" in lower: to require those contextual tokens or punctuation before returning text[:200].python/cube/core/parsers/opencode.py (1)
152-161: Consider inferring tool completion from terminal fields, not onlystatus.If OpenCode emits terminal payloads with
output/exitCodebut nostatus, this currently remainssubtype="started"and skips result capture.Refactor sketch
- status = (_first_string(state, "status") or _first_string(data, "status") or "").lower() - subtype = "completed" if status in ("completed", "success", "failed", "error") else "started" + status = (_first_string(state, "status") or _first_string(data, "status") or "").lower() + has_terminal_signal = ( + status in ("completed", "success", "failed", "error") + or any(k in state for k in ("output", "exitCode", "exit_code")) + or any(k in data for k in ("output", "result", "exitCode", "exit_code")) + ) + subtype = "completed" if has_terminal_signal else "started"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/core/parsers/opencode.py` around lines 152 - 161, The code only sets subtype = "completed" when status is one of ("completed","success","failed","error"), which misses terminal payloads that include output or exitCode but no status; change the subtype detection (around variables status, subtype) to also treat the call as completed if an output or exitCode is present in state/data/source (use _first_string(state,"output","title") or _dict_value(...,"exitCode") or similar checks against state/data/source), and then keep the existing logic that copies output into tool_args["result"] (respecting _MAX_RESULT_CONTENT) so results are captured even when status is missing; reference symbols: status, subtype, _first_string, _dict_value, args, tool_args, output, _MAX_RESULT_CONTENT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cube/commands/clean.py`:
- Around line 160-178: The subprocess.run calls in the worktrees_to_remove loop
and the branches_to_remove loop are not checked before incrementing
removed_count; change the logic to inspect the CompletedProcess return (e.g.,
result = subprocess.run(...)) and only increment removed_count when
result.returncode == 0, logging or printing a warning with result.stderr/text on
failure; for worktrees, if git remove returns non-zero fall back to
shutil.rmtree (as already intended) and increment only on successful rmtree or
successful git call, and for branch deletion do not increment removed_count on
non-zero return but emit a warning referencing the branch name and git stderr.
In `@python/cube/commands/ui.py`:
- Around line 38-40: The subprocess spawn currently uses a bare "npm" which
depends on PATH; before calling subprocess.Popen in the code that creates
frontend_process (the call with ["npm", "run", "dev"], cwd=web_ui_dir), resolve
the npm executable with shutil.which("npm") and if it returns None raise a clear
exception (or sys.exit with an error message) indicating npm was not found; then
pass the absolute path returned by shutil.which to subprocess.Popen instead of
the bare "npm" to ensure deterministic execution.
In `@python/cube/core/parsers/opencode.py`:
- Around line 121-135: The _assistant_text function currently returns text for
data/type=="text" and part/type=="text" without checking the sender role, which
can emit user/system messages as assistant; update both branches in
_assistant_text to read the role via data.get("role") (or part.get("role") when
appropriate) and only return the text if role is missing or equals "assistant" —
otherwise return None; keep existing handling for types "assistant",
"assistant_chunk", "message" in the middle branch unchanged (use _first_string
as before).
In `@python/cube/core/updater.py`:
- Line 15: Resolve the git binary once at module import (e.g., assign
shutil.which("git") to a module-level GIT_BIN and raise/handle gracefully if not
found), then update all subprocess.run calls in auto_update() and the module
(including the rev-parse, fetch, and pull invocations referenced) to call
GIT_BIN instead of "git", supply env with GIT_TERMINAL_PROMPT="0" to suppress
interactive prompts, and add explicit timeouts (5–20 seconds as appropriate) to
each subprocess.run call while keeping capture_output=True, text=True, and
check=True; ensure subprocess exceptions (TimeoutExpired, CalledProcessError)
are caught and logged/handled by the same functions (e.g., auto_update) so
startup won’t hang on network stalls.
In `@tests/conftest.py`:
- Around line 51-56: The test server subprocesses are created with
stdout/stderr=subprocess.PIPE which can deadlock if not drained; update both
subprocess.Popen(...) invocations that assign to proc (the backend server start)
and the other server start later in the file to stop piping: set
stdout=subprocess.DEVNULL and stderr=subprocess.DEVNULL (or
stderr=subprocess.STDOUT) instead of subprocess.PIPE so the child cannot block
on full buffers; ensure you change both Popen calls to the same non-piping
configuration.
---
Nitpick comments:
In `@python/cube/core/adapters/opencode.py`:
- Around line 109-117: The generic check that returns text[:200] whenever
"error" or "failed" appears is too broad; change the condition using the
existing variables lower and text so we only treat it as an error when
"error"/"failed" appears with additional error context (e.g., presence of
"exception", "traceback", "code", "status", "http", "stack", or when followed by
":" or appears at the start of the string), otherwise return None; update the
branch that currently reads if "error" in lower or "failed" in lower: to require
those contextual tokens or punctuation before returning text[:200].
In `@python/cube/core/parsers/__init__.py`:
- Line 10: The __all__ list in python/cube/core/parsers/__init__.py is unsorted
and triggers Ruff RUF022; fix it by alphabetically sorting the exported names in
the __all__ assignment (e.g., ensure "CLIReviewParser", "ClaudeParser",
"CursorParser", "GeminiParser", "OpenCodeParser", "ParserAdapter" are ordered
lexicographically) so the __all__ sequence is consistently sorted and the lint
rule is satisfied.
In `@python/cube/core/parsers/opencode.py`:
- Around line 152-161: The code only sets subtype = "completed" when status is
one of ("completed","success","failed","error"), which misses terminal payloads
that include output or exitCode but no status; change the subtype detection
(around variables status, subtype) to also treat the call as completed if an
output or exitCode is present in state/data/source (use
_first_string(state,"output","title") or _dict_value(...,"exitCode") or similar
checks against state/data/source), and then keep the existing logic that copies
output into tool_args["result"] (respecting _MAX_RESULT_CONTENT) so results are
captured even when status is missing; reference symbols: status, subtype,
_first_string, _dict_value, args, tool_args, output, _MAX_RESULT_CONTENT.
In `@python/cube/core/updater.py`:
- Around line 32-38: Replace the custom validation in is_safe_git_branch with a
call to Git's own validator: invoke "git check-ref-format --branch <branch>"
(e.g., via subprocess.run) and return True only if the command exits
successfully; return False on non-zero exit or exceptions. Ensure you still
reject empty/None branch inputs before calling Git and catch
OSError/subprocess.CalledProcessError to safely return False (or decide fallback
behavior), and keep the function signature is_safe_git_branch(branch: str)
intact.
In `@python/test_all_commands.py`:
- Line 11: The test helper run_cmd currently calls subprocess.run(cmd,
shell=True); change it to call subprocess.run with shell=False and expect cmd to
be a sequence (list/tuple) so subprocess runs without a shell (e.g.,
subprocess.run(cmd, shell=False, capture_output=True, text=True)); update all
callers/tests that pass string commands to pass argument lists instead
(reference the run_cmd function and any test that invokes it) or add a single
normalization inside run_cmd to raise if cmd is a str to force callers to use
lists.
In `@tests/cli/test_adapters.py`:
- Around line 65-71: The helper make_mock_process sets
mock_process.stdout.read.side_effect to list(chunks) + [b""], which triggers
RUF005; change it to use iterable unpacking by assigning side_effect to
[*chunks, b""] so the list is created succinctly and satisfies the linter—update
the reference in make_mock_process where mock_process.stdout.read.side_effect is
set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2af1806-b022-4799-8717-44bf05200b9e
📒 Files selected for processing (29)
python/cube/automation/message_broadcaster.pypython/cube/commands/clean.pypython/cube/commands/install.pypython/cube/commands/logs.pypython/cube/commands/sessions.pypython/cube/commands/ui.pypython/cube/commands/version.pypython/cube/commands/writers.pypython/cube/core/adapters/__init__.pypython/cube/core/adapters/opencode.pypython/cube/core/adapters/registry.pypython/cube/core/agent_logger.pypython/cube/core/config.pypython/cube/core/gemini_config.pypython/cube/core/master_log.pypython/cube/core/output.pypython/cube/core/parsers/__init__.pypython/cube/core/parsers/base.pypython/cube/core/parsers/cli_review.pypython/cube/core/parsers/opencode.pypython/cube/core/parsers/registry.pypython/cube/core/single_layout.pypython/cube/core/updater.pypython/cube/ui/routes/__init__.pypython/test_all_commands.pytests/cli/test_adapters.pytests/cli/test_runners.pytests/conftest.pytests/e2e/test_web_ui.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.12)
python/cube/commands/ui.py
[error] 39-39: Starting a process with a partial executable path
(S607)
python/cube/core/parsers/__init__.py
[warning] 10-10: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
python/cube/core/output.py
[warning] 22-22: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
tests/cli/test_runners.py
[error] 64-64: Probable insecure usage of temporary file or directory: "/tmp/mock-worktree"
(S108)
tests/e2e/test_web_ui.py
[error] 67-67: Probable insecure usage of temporary file or directory: "/tmp/test/path"
(S108)
python/cube/core/updater.py
[error] 15-15: Starting a process with a partial executable path
(S607)
[warning] 17-17: Do not catch blind exception: Exception
(BLE001)
[error] 25-25: Starting a process with a partial executable path
(S607)
[warning] 28-28: Do not catch blind exception: Exception
(BLE001)
[error] 47-47: subprocess call: check for execution of untrusted input
(S603)
[error] 47-47: Starting a process with a partial executable path
(S607)
[error] 50-50: subprocess call: check for execution of untrusted input
(S603)
[error] 51-51: Starting a process with a partial executable path
(S607)
[warning] 60-60: Do not catch blind exception: Exception
(BLE001)
[error] 70-70: subprocess call: check for execution of untrusted input
(S603)
[error] 70-70: Starting a process with a partial executable path
(S607)
[warning] 72-72: Do not catch blind exception: Exception
(BLE001)
python/test_all_commands.py
[error] 11-11: subprocess call with shell=True identified, security issue
(S602)
[error] 100-100: Probable insecure usage of temporary file or directory: "/tmp/test-task.md"
(S108)
python/cube/commands/clean.py
[error] 81-81: subprocess call: check for execution of untrusted input
(S603)
[error] 82-82: Starting a process with a partial executable path
(S607)
[error] 88-89: try-except-pass detected, consider logging the exception
(S110)
[warning] 88-88: Do not catch blind exception: Exception
(BLE001)
[error] 162-162: subprocess call: check for execution of untrusted input
(S603)
[error] 162-162: Starting a process with a partial executable path
(S607)
[warning] 164-164: Do not catch blind exception: Exception
(BLE001)
[warning] 171-171: Do not catch blind exception: Exception
(BLE001)
[error] 177-177: subprocess call: check for execution of untrusted input
(S603)
[error] 177-177: Starting a process with a partial executable path
(S607)
[warning] 179-179: Do not catch blind exception: Exception
(BLE001)
tests/cli/test_adapters.py
[warning] 70-70: Consider [*list(chunks), b""] instead of concatenation
Replace with [*list(chunks), b""]
(RUF005)
tests/conftest.py
[error] 51-51: subprocess call: check for execution of untrusted input
(S603)
[error] 105-105: subprocess call: check for execution of untrusted input
(S603)
[error] 106-106: Starting a process with a partial executable path
(S607)
🔇 Additional comments (30)
python/cube/ui/routes/__init__.py (2)
6-11: Sanity check: router registration and exports are consistent.
api_routeris created with prefix"/api", both routers are included (tasks then stream), and__all__correctly exports onlyapi_router. No issues spotted in this wiring.
3-5: LGTM — import reorder appears behaviour-neutral.Moving
tasks_routerto appear afterstream_routershouldn’t change runtime behaviour becauseapi_router.include_router(...)calls and the actualrouterreferences remain the same. Only concern would be side effects/circular imports inside those modules, but that isn’t visible from this file.python/cube/core/agent_logger.py (5)
6-6: ✅ Import cleanup is safe (and consistent with usage).Removing the unused import(s) is fine. The remaining
AsyncIteratorandOptionaltypes are used byagent_logging_context’s return type and parameters, so there’s no impact on runtime behaviour.
18-35: ✅ Docstring/formatting tweaks don’t affect logic.The docstring whitespace normalisation and the minor formatting changes around this function don’t alter control flow or return values. No functional concerns.
49-84: ✅AgentLogger.__init__signature formatting is behaviour-preserving.Trailing comma additions / whitespace adjustments in the
__init__signature (includingmetadata: Optional[str] = None,) are syntactic-only and won’t change how arguments are interpreted.
91-121: ✅ Newline handling and file writing are unchanged in effect.Switching
'\n'→"\n"(Line [119]) is equivalent in Python. The log-writing sequence (write_line→flush) and the_line_countincrement remain intact, so behaviour should be unchanged.
123-151: ✅agent_logging_contexttrailing commas / pass-through are safe.The trailing comma/whitespace cleanups in the
agent_logging_contextsignature and themetadata=metadataargument pass-through are formatting-only and shouldn’t impact parsing or the yieldedAgentLogger.python/cube/core/single_layout.py (3)
30-33: Docstring whitespace normalization looks good.These are formatting/docstring-only adjustments (no behavioural change). The updated indentation/readability is fine—no action needed.
45-47: Forward-reference type annotation change is fine; optional simplification available.Switching to
Optional["SingleAgentLayout"]is harmless and improves consistency. If you want to reduce string-quoting long-term, considerfrom __future__ import annotationsortyping.Self(Python version permitting), but that’s optional.
101-107: Start/close singleton guard remains correct.The
start()implementation still correctly serialises access with_lockand callsBaseThinkingLayout.start(cls._instance)to avoid recursion. The trailing newline/whitespace cleanup at EOF is also fine.python/cube/core/updater.py (1)
43-44: Good defensive gate before remote operations.The early returns on invalid branches in
has_updatesandpull_updatesare a solid safety improvement and reduce risky subprocess inputs.Also applies to: 66-67
python/cube/core/parsers/base.py (1)
3-20: Looks good — interface cleanup is safe.No behavioural change here; this is a clean import/format tidy-up.
python/cube/core/parsers/cli_review.py (1)
7-42: LGTM — formatting-only changes in parser path.The updated formatting keeps the same parse behaviour for both simple and delegated branches.
python/cube/core/output.py (1)
10-74: Nice cleanup — no functional regression spotted.Import and formatting changes are safe, and output helpers retain existing behaviour.
python/cube/core/gemini_config.py (1)
20-24: Good hardening on settings load path.Catching targeted decode/filesystem errors is a cleaner failure model than broad exception swallowing.
python/cube/commands/version.py (1)
3-10: Looks good — straightforward command module cleanup.No runtime behaviour changes introduced.
python/cube/commands/install.py (1)
7-43: LGTM — tidy command cleanup without behaviour change.The edits are clean and keep install UX intact.
python/cube/automation/message_broadcaster.py (1)
22-63: Looks good — payload formatting changes are safe.No behavioural drift in CLI/SSE broadcast paths from these edits.
python/cube/commands/sessions.py (1)
25-25: Good output ordering for session listing.Placing the summary after the per-session block makes the CLI output easier to scan.
python/cube/core/master_log.py (1)
91-101: Nice lock scoping around context entry.This keeps the global lock limited to instance setup and avoids unnecessary contention while writing logs.
tests/e2e/test_web_ui.py (1)
10-10: Good reliability improvement on health check.Adding a timeout here reduces the chance of e2e hangs when the backend is slow or unreachable.
python/cube/commands/logs.py (1)
93-95: Good fix to writer/judge log filtering.Using the correct loop variable here restores accurate categorisation in the no-task listing path.
python/cube/commands/writers.py (1)
31-40: Resume fallback handling looks solid.The default resume message path is clear and keeps the resume flow deterministic when no message is provided.
python/cube/core/adapters/registry.py (1)
10-17: OpenCode adapter registry wiring looks correctThe
OpenCodeAdapterimport and"opencode"mapping are consistent with the existing factory pattern and should resolve cleanly viaget_adapter("opencode").python/cube/core/parsers/registry.py (1)
10-17: OpenCode parser registry entry is correctly integratedThe new
OpenCodeParserimport and"opencode"key in_PARSERSare correctly wired forget_parser("opencode").python/cube/core/adapters/__init__.py (1)
4-16: Adapter exports are tidy and completeRe-exporting
ClaudeAdapterandOpenCodeAdapterin__all__aligns with the new adapter surface.tests/cli/test_runners.py (1)
3-63: Test refactor is clean and behaviour-preservingThe import cleanup and parenthesised multi-patch context keep the tests readable without changing runtime behaviour.
tests/cli/test_adapters.py (1)
78-243: Great coverage for OpenCode adapter/parser pathsThe new tests cover command construction, resume semantics, install checks, parser mapping, and registry wiring thoroughly.
python/cube/core/parsers/opencode.py (2)
50-57: Good explicitstep_start→system/initmapping.This is a solid guardrail for resume/session tracking, especially when session metadata arrives before other useful payloads.
22-29: Nice bounded fallback behaviour for malformed/unknown/error payloads.Truncating unknown/error content reduces noisy payload blast radius while preserving debugging value.
Also applies to: 35-37, 61-64, 191-200
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli/test_adapters.py (1)
65-71: Use starred-list unpacking instead of list concatenation for cleaner code.Line 70 can be simplified from
list(chunks) + [b""]to[*chunks, b""]. This avoids the intermediate list creation and follows the more idiomatic Python convention for constructing lists with unpacking.Suggested tidy-up
def make_mock_process(*chunks): mock_process = AsyncMock() mock_process.wait.return_value = 0 mock_process.returncode = 0 mock_process.stdout = AsyncMock() - mock_process.stdout.read.side_effect = list(chunks) + [b""] + mock_process.stdout.read.side_effect = [*chunks, b""] return mock_process🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_adapters.py` around lines 65 - 71, In make_mock_process, replace the current side_effect construction that uses list(chunks) + [b""] with starred-list unpacking [*chunks, b""] to avoid the intermediate list creation; update the assignment to mock_process.stdout.read.side_effect accordingly so the AsyncMock yields the provided chunks followed by b"".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cli/test_adapters.py`:
- Around line 65-71: In make_mock_process, replace the current side_effect
construction that uses list(chunks) + [b""] with starred-list unpacking
[*chunks, b""] to avoid the intermediate list creation; update the assignment to
mock_process.stdout.read.side_effect accordingly so the AsyncMock yields the
provided chunks followed by b"".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a546fe9d-ea51-4983-a4e8-0296f1b972bb
📒 Files selected for processing (4)
.env.example.gitignorepython/cube/core/adapters/opencode.pytests/cli/test_adapters.py
✅ Files skipped from review due to trivial changes (2)
- .env.example
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cube/core/adapters/opencode.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.12)
tests/cli/test_adapters.py
[warning] 70-70: Consider [*list(chunks), b""] instead of concatenation
Replace with [*list(chunks), b""]
(RUF005)
🔇 Additional comments (4)
tests/cli/test_adapters.py (4)
12-53: Good upgrade totmp_path-based Cursor adapter test coverage.This is a solid improvement: it removes
/tmpcoupling and keeps subprocess-stream assertions focused on command structure and emitted lines.
57-63: Error-path test is clear and correctly scoped.The not-logged-in runtime error path is validated cleanly with
pytest.raises, which tightens behaviour guarantees.
79-143: OpenCode adapter command/env/resume tests are well targeted.Great coverage for argv ordering, cwd/env forwarding, dotenv precedence behaviour, and conditional
--sessioninclusion.
154-247: Parser and registry tests provide strong behavioural coverage.These cases map well to the parser contract (assistant/system/tool/noise/unknown/malformed) and confirm registry wiring.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cube.yaml`:
- Line 14: Replace the shortened model identifier "moonshotai/kimi-k2.6" with
the full OpenRouter identifier "openrouter/moonshotai/kimi-k2.6" wherever the
test expects the model (e.g., in tests.cli.test_adapters where the model ID
string is asserted), and make the same change for the matching entries
referenced in cube.yaml so the configured model and tests use the identical
"openrouter/moonshotai/kimi-k2.6" value; update the literal strings and any
related assertions/mocks to use the full identifier and re-run tests.
In `@tests/cli/test_adapters.py`:
- Around line 190-231: The OpenCodeParser.parse currently calls dict(args)
without guarding for missing tool input, causing a TypeError when a tool_use
event has no state.input; update the parsing logic in OpenCodeParser.parse to
default the tool args to an empty dict (e.g., args = part.get("state",
{}).get("input") or {}) before calling dict(args), and adjust the branch that
maps tool names (e.g., mapping "bash" -> "shell") so it builds tool_args from
that safe dict; add a test case mirroring
test_opencode_parser_emits_tool_messages that sends a tool_use event with no
input/state.input to assert tool_args == {} and no exception is raised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07d2696b-f194-4245-bce3-58bae3da72aa
📒 Files selected for processing (3)
python/cube.yamlpython/cube/core/adapters/opencode.pytests/cli/test_adapters.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cube/core/adapters/opencode.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.12)
tests/cli/test_adapters.py
[warning] 70-70: Consider [*list(chunks), b""] instead of concatenation
Replace with [*list(chunks), b""]
(RUF005)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/cli/test_adapters.py (2)
194-203:⚠️ Potential issue | 🟡 MinorUse a verified OpenRouter model identifier in this OpenCode command assertion.
Line 194/Line 202 currently assert
moonshotai/kimi-k2.6. That may drift from the configured/provider-qualified contract and can lock in the wrong CLI behaviour.For the current OpenCode CLI version, when targeting an OpenRouter-hosted Kimi model, should `opencode run --model` use `openrouter/moonshotai/kimi-k2.6` or `moonshotai/kimi-k2.6`?🔧 If provider-qualified format is required, update the test like this
- async for line in adapter.run(tmp_path, "moonshotai/kimi-k2.6", "--explain this prompt"): + async for line in adapter.run(tmp_path, "openrouter/moonshotai/kimi-k2.6", "--explain this prompt"): @@ - "moonshotai/kimi-k2.6", + "openrouter/moonshotai/kimi-k2.6",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_adapters.py` around lines 194 - 203, The test asserts an unqualified model id "moonshotai/kimi-k2.6" which may be incorrect; update the expected args in tests/cli/test_adapters.py (the assertion that builds args from collect_exec_args(mock_exec) used after async for line in adapter.run(...)) to use the provider-qualified identifier "openrouter/moonshotai/kimi-k2.6" so the expected ["--model", ...] matches the CLI's OpenRouter model format.
289-330:⚠️ Potential issue | 🟡 MinorAdd the no-input tool event regression case here as well.
This still only exercises input-bearing tool events. Please also cover the
tool_usebranch wherestate.inputis absent, so parser behaviour is guarded against that edge case.🧪 Suggested test addition
def test_opencode_parser_emits_tool_messages(): @@ assert completed.tool_name == "shell" assert completed.tool_args == {"command": "ls", "result": "ok"} assert completed.exit_code == 0 + + no_input = parser.parse( + json.dumps( + { + "type": "tool_use", + "sessionID": "ses_tool", + "part": { + "type": "tool", + "tool": "bash", + "state": {"status": "completed", "exitCode": 0}, + }, + } + ) + ) + assert no_input is not None + assert no_input.tool_args == {} + assert no_input.exit_code == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_adapters.py` around lines 289 - 330, Add a new sub-case in test_opencode_parser_emits_tool_messages that exercises a tool_use event whose part.state has no input key: call OpenCodeParser().parse with a JSON tool_use payload where part.type == "tool", part.tool set (e.g., "read"), and state contains status (e.g., "started") but no input field; then assert the returned message is not None, message.type == "tool_call", message.subtype matches the status, message.tool_name matches the tool, and message.tool_args is an empty dict (or whatever the parser's no-input canonical value is) to ensure the parser handles missing state.input without crashing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cube/core/parsers/kimi.py`:
- Around line 20-27: Move the resume regex check to run only after JSON parsing
fails: first attempt json.loads(clean) and proceed normally; if json.loads
raises json.JSONDecodeError then run the existing resume_match =
re.search(r"kimi\s+-r\s+([0-9a-fA-F-]{36})", clean) and, if it matches, return
the StreamMessage(type="system", subtype="init",
session_id=resume_match.group(1)); otherwise fall back to returning
StreamMessage(type="unknown", content=clean[:500]). This ensures the
resume_match lookup does not preempt valid assistant JSON content.
---
Duplicate comments:
In `@tests/cli/test_adapters.py`:
- Around line 194-203: The test asserts an unqualified model id
"moonshotai/kimi-k2.6" which may be incorrect; update the expected args in
tests/cli/test_adapters.py (the assertion that builds args from
collect_exec_args(mock_exec) used after async for line in adapter.run(...)) to
use the provider-qualified identifier "openrouter/moonshotai/kimi-k2.6" so the
expected ["--model", ...] matches the CLI's OpenRouter model format.
- Around line 289-330: Add a new sub-case in
test_opencode_parser_emits_tool_messages that exercises a tool_use event whose
part.state has no input key: call OpenCodeParser().parse with a JSON tool_use
payload where part.type == "tool", part.tool set (e.g., "read"), and state
contains status (e.g., "started") but no input field; then assert the returned
message is not None, message.type == "tool_call", message.subtype matches the
status, message.tool_name matches the tool, and message.tool_args is an empty
dict (or whatever the parser's no-input canonical value is) to ensure the parser
handles missing state.input without crashing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53777e96-3d7d-430c-93e7-aca103593919
📒 Files selected for processing (8)
python/cube.yamlpython/cube/core/adapters/__init__.pypython/cube/core/adapters/kimi.pypython/cube/core/adapters/registry.pypython/cube/core/parsers/__init__.pypython/cube/core/parsers/kimi.pypython/cube/core/parsers/registry.pytests/cli/test_adapters.py
🚧 Files skipped from review as they are similar to previous changes (4)
- python/cube/core/parsers/registry.py
- python/cube/core/adapters/registry.py
- python/cube/core/parsers/init.py
- python/cube/core/adapters/init.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.12)
tests/cli/test_adapters.py
[warning] 71-71: Consider [*list(chunks), b""] instead of concatenation
Replace with [*list(chunks), b""]
(RUF005)
🔇 Additional comments (2)
python/cube.yaml (1)
14-14: Good config wiring for Kimi alias/tool/judge flow.Line 14, Line 34, and Lines 65-66 are consistent with the alias-resolution and CLI-tool lookup path, so
judge_3should route cleanly to the Kimi adapter.Also applies to: 34-34, 65-66
python/cube/core/adapters/kimi.py (1)
13-37: Adapter command construction and streaming path look solid.The
run()implementation is clean and aligned with the shared subprocess streaming contract, including guarded install checks and resume-session handling.
|
Pivot direction for this PR: Stop trying to make OpenCode the Kimi path. OpenCode recognizes OpenRouter/Kimi and completes internally, but Keep:
Remove/defer:
Expected final PR shape: Verified locally: |
Remove the deferred OpenCode runtime from the PR and harden the reviewed subprocess paths so the branch focuses on the official Kimi CLI judge adapter. Made-with: Cursor
|
Fixed in 7616660 Agent Cube |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
planning/generic-cli-adapter.md (1)
41-64: Keep the Kimi example Kimi-specific.The
kimiexample still loads~/.config/opencode/.env, which blurs the line between the official Kimi path and the experimental OpenCode path. If that shared env is intentional, call it out; otherwise, drop it from the Kimi example.Suggested tweak
env_files: - "~/.config/kimi/.env" - - "~/.config/opencode/.env" - "{{worktree}}/.env"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/generic-cli-adapter.md` around lines 41 - 64, The kimi example under generic_cli currently lists "~/.config/opencode/.env" in env_files which conflates Kimi-specific and OpenCode-specific config; either remove that path from the generic_cli.kimi.env_files array so only Kimi-specific files remain (e.g., "~/.config/kimi/.env" and "{{worktree}}/.env"), or explicitly document in the kimi block that the "~/.config/opencode/.env" entry is intentionally shared with OpenCode (add a comment or prose near the generic_cli.kimi.env_files section stating why it's included).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cube/commands/clean.py`:
- Around line 80-89: The try/except around subprocess.run in clean.py currently
swallows all errors (see subprocess.run, branches_to_remove, task_id,
PROJECT_ROOT); change the except to capture the exception (except Exception as
e) and emit a warning that includes the exception message and any stderr/stdout
available so the user is informed about the git branch listing failure, then
continue (do not re-raise) so cleanup proceeds but is not silently incomplete;
use the module logger (getLogger(__name__)) or the existing CLI logging
mechanism/echo to log the warning with context including the failing command and
task_id.
---
Nitpick comments:
In `@planning/generic-cli-adapter.md`:
- Around line 41-64: The kimi example under generic_cli currently lists
"~/.config/opencode/.env" in env_files which conflates Kimi-specific and
OpenCode-specific config; either remove that path from the
generic_cli.kimi.env_files array so only Kimi-specific files remain (e.g.,
"~/.config/kimi/.env" and "{{worktree}}/.env"), or explicitly document in the
kimi block that the "~/.config/opencode/.env" entry is intentionally shared with
OpenCode (add a comment or prose near the generic_cli.kimi.env_files section
stating why it's included).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21995dc0-75df-48ff-bec3-ccc8680ef5b4
📒 Files selected for processing (13)
planning/generic-cli-adapter.mdpython/cube.yamlpython/cube/commands/clean.pypython/cube/commands/ui.pypython/cube/core/adapters/__init__.pypython/cube/core/adapters/kimi.pypython/cube/core/adapters/registry.pypython/cube/core/parsers/__init__.pypython/cube/core/parsers/kimi.pypython/cube/core/parsers/registry.pypython/cube/core/updater.pytests/cli/test_adapters.pytests/conftest.py
✅ Files skipped from review due to trivial changes (3)
- python/cube/core/parsers/registry.py
- python/cube/core/adapters/init.py
- python/cube.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cube/core/adapters/registry.py
- python/cube/core/adapters/kimi.py
- python/cube/core/parsers/kimi.py
📜 Review details
🧰 Additional context used
🪛 LanguageTool
planning/generic-cli-adapter.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...CLI. OpenCode is useful as an experiment but its run command is currently unreliab...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...e Kimi/Qwen/OpenCode-style integrations cheaper without forcing Cursor/Claude/Gemini in...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...}}` 4. Support optional resume args. 5. Support optional env files for local secrets an...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[misspelling] ~20-~20: This word is normally spelled as one.
Context: ...ecrets and provider config. 6. Preserve hand-written adapters where they have real custom be...
(EN_COMPOUNDS_HAND_WRITTEN)
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...raw OpenAI-compatible tool loop here. - Do not hide parser differences behind conf...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~29-~29: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ide parser differences behind config. - Do not make opencode the default Kimi pa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~30-~30: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...until its run behavior is reliable. - Do not store API keys in tracked config. ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~154-~154: Possible missing comma found.
Context: ...Keep the concrete KimiAdapter for one release or delete it immediately if the diff is...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~158-~158: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...y if opencode run is reliable enough. Otherwise leave it experimental or remove it. --...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~191-~191: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...upport timeout_seconds per adapter? - Should env files be global config only, or sho...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~192-~192: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... .env be standard for all adapters? - Should Kimi use --no-thinking for judge spee...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.15.12)
python/cube/commands/ui.py
[error] 42-42: subprocess call: check for execution of untrusted input
(S603)
tests/conftest.py
[error] 51-51: subprocess call: check for execution of untrusted input
(S603)
[error] 105-105: subprocess call: check for execution of untrusted input
(S603)
[error] 106-106: Starting a process with a partial executable path
(S607)
python/cube/commands/clean.py
[error] 81-81: subprocess call: check for execution of untrusted input
(S603)
[error] 82-82: Starting a process with a partial executable path
(S607)
[error] 88-89: try-except-pass detected, consider logging the exception
(S110)
[warning] 88-88: Do not catch blind exception: Exception
(BLE001)
[error] 163-163: subprocess call: check for execution of untrusted input
(S603)
[error] 164-164: Starting a process with a partial executable path
(S607)
[error] 189-189: subprocess call: check for execution of untrusted input
(S603)
[error] 189-189: Starting a process with a partial executable path
(S607)
python/cube/core/updater.py
[error] 22-22: subprocess call: check for execution of untrusted input
(S603)
[warning] 31-31: Do not catch blind exception: Exception
(BLE001)
[error] 41-41: subprocess call: check for execution of untrusted input
(S603)
[warning] 51-51: Do not catch blind exception: Exception
(BLE001)
[error] 70-70: subprocess call: check for execution of untrusted input
(S603)
[error] 81-81: subprocess call: check for execution of untrusted input
(S603)
[warning] 93-93: Do not catch blind exception: Exception
(BLE001)
[error] 103-103: subprocess call: check for execution of untrusted input
(S603)
[warning] 113-113: Do not catch blind exception: Exception
(BLE001)
tests/cli/test_adapters.py
[warning] 70-70: Consider [*list(chunks), b""] instead of concatenation
Replace with [*list(chunks), b""]
(RUF005)
🔇 Additional comments (8)
python/cube/commands/clean.py (2)
5-16: Signature update stays compatible.
Optional[str]matches the Typer caller, so the positionaltask_idinvocation inpython/cube/cli.pystill works as expected.
159-195: Deletion accounting is fixed.Only incrementing
removed_countafter successful git/rmtree operations closes the earlier false-success bug, and the stderr in the branch/worktree warnings is useful.python/cube/core/updater.py (1)
12-114: Good hardening around git discovery and prompts.Resolving
gitonce, settingGIT_TERMINAL_PROMPT=0, and gating update calls on a safe ref all make the startup path less brittle. The bounded timeouts are also a good fit here.python/cube/commands/ui.py (1)
6-15: Looks good: resolvingnpmfirst makes the launch behaviour deterministic.The import cleanup is fine too, and this avoids relying on ambient
PATHwhen starting the frontend.Also applies to: 39-43
python/cube/core/parsers/__init__.py (1)
4-17: Looks good: exportingKimiParseralongside the existing parsers keeps the package API aligned.tests/conftest.py (1)
4-21: Looks good: the port helper and both server fixtures clean up without draining pipes.This keeps the suite from stalling while preserving the same yielded URLs and start-up checks.
Also applies to: 51-75, 78-130
tests/cli/test_adapters.py (2)
1-76: Looks good: the cursor coverage and shared subprocess helpers keep the test setup tidy.
78-194: Looks good: the new Kimi adapter, resume, parser, and registry tests line up with the production contract.
jacsamell
left a comment
jacsamell
left a comment
There was a problem hiding this comment.
🤖 Agent Cube Review
🤖 Agent Cube Peer Review
jacsamell
left a comment
jacsamell
left a comment
There was a problem hiding this comment.
🤖 Agent Cube Review
🤖 Agent Cube Peer Review
Log Kimi CLI output, preserve mixed tool-call payloads, and surface cleanup/config notes from the judge review. Made-with: Cursor
|
Fixed in 6aa5c14 Agent Cube |
|
Fixed in 6aa5c14 Agent Cube |
2 similar comments
|
Fixed in 6aa5c14 Agent Cube |
|
Fixed in 6aa5c14 Agent Cube |
1 participant
Autonomous implementation via Agent Cube
Winner: Writer writer_a
Branch: writer-codex/opencode-adapter
Review decisions in
.prompts/decisions/opencode-adapter-*.jsonKimi CLI Judge Adapter Implementation
Adds official support for Kimi Code as a CLI-based judge adapter, refocusing the PR away from the initial OpenCode implementation due to reliability concerns with
opencode run.Core Implementation
KimiAdapter (
python/cube/core/adapters/kimi.py): CLI adapter executingkimi --print --output-format stream-jsonwith configurable model, worktree, and prompt. Includes resume support via--session <session_id>when available. Validates Kimi installation and provides setup instructions.KimiParser (
python/cube/core/parsers/kimi.py): Parses Kimi's streaming JSONL output, extracting assistant messages, tool calls, and session IDs for resumable workflows. Handles JSON decode failures gracefully by detecting resume patterns and normalizing tool names.Configuration & Registry
python/cube.yaml: Addedkimi_k26_openrouterCLI tool mapping andkimimodel alias; migratedjudge_3from Grok to Kimi persona.KimiAdapterandKimiParserin adapter/parser registries for"kimi"lookups.OPENROUTER_API_KEYto.env.example.Test Coverage
Expanded
tests/cli/test_adapters.pywith comprehensive Kimi tests covering:Code Maintenance
typer,Path, unused output helpers) across command modules.python/cube/core/updater.pywith git branch validation and timeout/environment controls to prevent unvalidated input execution.python/cube/commands/clean.pywith capture and reporting of subprocess failures.python/cube/commands/ui.pyto explicitly locatenpmviashutil.which, failing fast if unavailable.Documentation
Added
planning/generic-cli-adapter.md: Specification for config-driven generic CLI adapter pattern to support future Kimi migrations and similar tools whilst maintaining parser modularity.Environment
Updated
.gitignoreto exclude.envand.env.localfiles from tracking.