feat(shell): inject shell mode output into context & persist cd#1587
feat(shell): inject shell mode output into context & persist cd#1587n-WN wants to merge 8 commits intoMoonshotAI:mainfrom
Conversation
…ersist cd
Shell mode (Ctrl+X) commands now capture output and inject it into the
conversation context as a <system-reminder> user message, so the agent
is aware of what the user ran. `cd` is now persistent across commands
via os.chdir() with session.work_dir kept in sync.
Architecture:
- New `capture.py` shared module with three orthogonal concerns:
- `clean_output()`: normalize \r\n, strip C0 controls, resolve \r
overwrites, strip ANSI via rich
- `execute_with_pty_capture()`: openpty + async subprocess +
loop.add_reader tee — preserves TTY behaviour while capturing
- `execute_with_pipe_capture()`: PIPE + tee for future `!` prefix
- `inject_to_context()`: truncate + system-reminder wrap, decoupled
from soul via append_message callback
- PTY capture includes: termios save/restore, SIGINT forwarding,
in-memory capture hard limit (100KB), terminal size propagation
- `cd` resolved via real shell (supports ~, -, CDPATH), OLDPWD tracked
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bddb90f683
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/kimi_cli/ui/shell/__init__.py
Outdated
| new_cwd = stdout.decode("utf-8", errors="replace").strip() | ||
| if not new_cwd or not os.path.isdir(new_cwd): | ||
| console.print(f"[red]cd: not a directory: {target}[/red]") |
There was a problem hiding this comment.
cd - probe output as a single path
When the user runs cd -, POSIX shells print the destination directory to stdout; this probe also appends pwd, so stdout contains two lines. Here new_cwd is parsed with .strip() and then validated with os.path.isdir, which fails on the embedded newline, causing cd - to report “not a directory” even though the directory exists. Parse only the last non-empty stdout line (or avoid && pwd for cd -) before calling os.chdir.
Useful? React with 👍 / 👎.
src/kimi_cli/ui/shell/capture.py
Outdated
| import fcntl | ||
| import os | ||
| import re | ||
| import sys | ||
| import termios |
There was a problem hiding this comment.
This new module imports fcntl and termios at module load time, but both are unavailable on Windows. Because src/kimi_cli/ui/shell/__init__.py now imports this module unconditionally, entering shell UI on Windows will raise ModuleNotFoundError before any command runs. Make these imports conditional (or lazy inside POSIX-only code paths) so shell UI remains importable on Windows.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Fix import sorting (I001) - Replace try-except-pass with contextlib.suppress (SIM105) - Replace asyncio.TimeoutError with builtin TimeoutError (UP041) - Add logger usage in capture.py debug paths - ruff format
|
Simply put, fixed the issue where shell-mode did not enter the context @RealKai42 At the same time, allow the cd command in shell mode to switch directories (persist) Windows adaptation will be completed tonight Not yet make-changelog |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0abf650e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/kimi_cli/ui/shell/__init__.py
Outdated
| if split_cmd and split_cmd[0] == "cd": | ||
| await self._handle_cd(split_cmd) |
There was a problem hiding this comment.
cd commands execute in the shell
This condition now routes every command whose first token is cd into _handle_cd, so valid shell commands like cd /tmp && ls or cd dir; pwd are rejected as too many arguments instead of executing. Before this change those commands ran normally in shell mode, so this is a functional regression for common workflows that chain cd with other operations.
Useful? React with 👍 / 👎.
src/kimi_cli/ui/shell/capture.py
Outdated
| if captured_bytes < _CAPTURE_HARD_LIMIT: | ||
| captured.append(data) | ||
| captured_bytes += len(data) |
There was a problem hiding this comment.
The capture buffer stops appending once _CAPTURE_HARD_LIMIT is reached, which preserves the beginning of output, not the end. Since inject_to_context later keeps the last 50KB of what was captured, long commands end up injecting a mid-stream slice and can omit the final error/summary lines users and the agent typically need.
Useful? React with 👍 / 👎.
- Fix `cd ~` by expanding tilde on Python side before shlex.quote - Fix `cd -` output parsing: take last non-empty line (shell prints destination before pwd) - Guard fcntl/termios imports behind sys.platform != "win32" - Fix capture to keep tail not head: evict old chunks when buffer exceeds 2x hard limit, so inject_to_context gets the most recent output
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5a6b51660
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/kimi_cli/ui/shell/__init__.py
Outdated
| exit_code, raw_output = await execute_with_pty_capture( | ||
| command, env=get_clean_env(), cwd=os.getcwd() |
There was a problem hiding this comment.
_run_shell_command now unconditionally routes shell-mode commands through execute_with_pty_capture, but that helper depends on POSIX-only APIs (os.openpty, fcntl/termios, and loop.add_reader on file descriptors). In the shell UI path on Windows, this raises before command execution and the caller only prints a failure, so Ctrl+X shell mode cannot run normal commands there. Please gate PTY capture by platform and fall back to PIPE/create_subprocess_shell on non-POSIX runtimes.
Useful? React with 👍 / 👎.
src/kimi_cli/ui/shell/__init__.py
Outdated
| if split_cmd and split_cmd[0] == "cd": | ||
| await self._handle_cd(split_cmd) | ||
| return |
There was a problem hiding this comment.
The new cd interception triggers whenever the first token is cd, then _handle_cd rejects more than one argument; this makes valid shell commands like cd /tmp && ls or cd ..; pwd fail with cd: too many arguments instead of running. In shell mode this is a functional regression from the previous behavior, because any composite command beginning with cd is now blocked.
Useful? React with 👍 / 👎.
src/kimi_cli/ui/shell/__init__.py
Outdated
|
|
||
| # Let the shell resolve -, $HOME, CDPATH, etc. | ||
| probe = await asyncio.create_subprocess_shell( | ||
| f"cd {shlex.quote(target)} && pwd", |
There was a problem hiding this comment.
Quoting the cd target in the probe command forces variable references to be literal, so inputs like cd $HOME attempt to enter a directory named $HOME instead of expanding the environment variable. This breaks common shell syntax in the new persistent-cd path and contradicts the intended behavior of resolving shell-style targets via the real shell.
Useful? React with 👍 / 👎.
Only intercept bare `cd` / `cd <path>` (at most 2 tokens). Compound commands containing &&, ;, | etc. are passed through to the shell.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e4375ff6c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/kimi_cli/ui/shell/capture.py
Outdated
| # Drain any remaining buffered output after process exits. | ||
| with contextlib.suppress(TimeoutError): | ||
| await asyncio.wait_for(eof_event.wait(), timeout=0.5) | ||
| except KeyboardInterrupt: |
There was a problem hiding this comment.
execute_with_pty_capture only handles KeyboardInterrupt, but this CLI is driven by asyncio.run, where SIGINT commonly cancels the main task and raises asyncio.CancelledError (a BaseException) instead. In that path the exception bypasses this handler and also bypasses _run_shell_command's except Exception, so pressing Ctrl+C during a shell-mode command can tear down the shell loop instead of returning to the prompt after stopping the child process.
Useful? React with 👍 / 👎.
| async def _handle_cd(self, args: list[str]) -> None: | ||
| """Resolve ``cd`` via a real shell and persist the directory change. | ||
|
|
||
| Only called for bare ``cd`` / ``cd <path>`` (at most 2 tokens). | ||
| """ | ||
| target = args[1] if len(args) > 1 else "~" | ||
|
|
||
| # Expand ~ on Python side so shlex.quote won't suppress tilde expansion. | ||
| if target.startswith("~"): | ||
| target = os.path.expanduser(target) | ||
|
|
||
| # Provide OLDPWD so `cd -` works across invocations. | ||
| env = get_clean_env() | ||
| old_cwd = os.getcwd() | ||
| if "OLDPWD" not in env: | ||
| env["OLDPWD"] = old_cwd | ||
|
|
||
| # Let the shell resolve -, $HOME, CDPATH, etc. | ||
| probe = await asyncio.create_subprocess_shell( | ||
| f"cd {shlex.quote(target)} && pwd", | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| env=env, | ||
| ) | ||
| stdout, stderr = await probe.communicate() | ||
|
|
||
| if probe.returncode != 0: | ||
| err = stderr.decode("utf-8", errors="replace").strip() | ||
| console.print(f"[red]cd: {err or 'failed'}[/red]") | ||
| return | ||
|
|
||
| # `cd -` prints the destination before `pwd`; take the last non-empty line. | ||
| lines = [ln for ln in stdout.decode("utf-8", errors="replace").splitlines() if ln.strip()] | ||
| new_cwd = lines[-1].strip() if lines else "" | ||
| if not new_cwd or not os.path.isdir(new_cwd): | ||
| console.print(f"[red]cd: not a directory: {target}[/red]") | ||
| return | ||
|
|
||
| os.chdir(new_cwd) | ||
| # Set OLDPWD for the next `cd -` invocation. | ||
| os.environ["OLDPWD"] = old_cwd | ||
|
|
||
| # Keep the session's work_dir in sync so agent background tasks | ||
| # (which use session.work_dir as cwd) also see the new directory. | ||
| if isinstance(self.soul, KimiSoul): | ||
| from kaos.path import KaosPath | ||
|
|
||
| self.soul.runtime.session.work_dir = KaosPath.unsafe_from_local_path( | ||
| __import__("pathlib").Path(new_cwd) | ||
| ) |
There was a problem hiding this comment.
🔴 Unhandled exceptions in _handle_cd crash the interactive shell session
The _handle_cd method performs several fallible operations — os.getcwd() (line 555), asyncio.create_subprocess_shell (line 566), probe.communicate() (line 572), and os.chdir() (line 586) — but none are wrapped in try/except. The call site at src/kimi_cli/ui/shell/__init__.py:522 also lacks error handling. Any exception propagates through _run_shell_command into the main event loop (src/kimi_cli/ui/shell/__init__.py:453-454), which has only try/finally but no except, crashing the entire interactive session.
This contrasts with the general shell command path which wraps _execute_capture in try/except Exception at src/kimi_cli/ui/shell/__init__.py:529-535. A practical trigger: if the current working directory has been deleted (common during development), os.getcwd() at line 555 raises FileNotFoundError, killing the session instead of showing a friendly error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| await proc.wait() | ||
| # Drain any remaining buffered output after process exits. | ||
| with contextlib.suppress(TimeoutError): | ||
| await asyncio.wait_for(eof_event.wait(), timeout=0.5) | ||
| except KeyboardInterrupt: | ||
| # Forward SIGINT to child if it is still running. | ||
| logger.debug("PTY capture: KeyboardInterrupt, forwarding SIGINT to child") | ||
| if proc.returncode is None: | ||
| proc.send_signal(2) # SIGINT | ||
| try: | ||
| await asyncio.wait_for(proc.wait(), timeout=3.0) | ||
| except (TimeoutError, ProcessLookupError): | ||
| logger.debug("PTY capture: child did not exit after SIGINT, killing") | ||
| proc.kill() |
There was a problem hiding this comment.
🟡 asyncio.CancelledError silently swallowed in execute_with_pty_capture
The PR added asyncio.CancelledError to the exception handler at src/kimi_cli/ui/shell/capture.py:166, but the exception is caught and not re-raised after cleanup. This means if the enclosing asyncio task is cancelled during proc.wait(), the cancellation is silently suppressed — the function returns normally with partial output instead of propagating the cancellation. While KeyboardInterrupt (also caught here) is conventionally handled without re-raising in interactive apps, CancelledError should be re-raised after cleanup to honor the task cancellation contract. Without re-raising, the caller (_run_shell_command) will proceed to inject context into the conversation and return normally, defeating the cancellation.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6537e0c17b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| body = f"$ {command}\n{output}" | ||
|
|
||
| message = Message( | ||
| role="user", | ||
| content=[system_reminder(f"{header}\n\n{body}")], |
There was a problem hiding this comment.
inject_to_context embeds raw command/output directly inside a <system-reminder> wrapper, so shell output containing </system-reminder> (or nested <system-reminder>...) can break out and introduce new authoritative directives. This is exploitable whenever a user runs commands over untrusted content (for example cat on a repo file), and it is high impact because the default agent prompt explicitly treats any <system-reminder> tags as system-level instructions (src/kimi_cli/agents/default/system.md).
Useful? React with 👍 / 👎.
| needs_shell_expansion = target.startswith("~") or target.startswith("$") or target == "-" | ||
| quoted_target = target if needs_shell_expansion else shlex.quote(target) |
There was a problem hiding this comment.
🔴 Unquoted tilde/dollar targets in _handle_cd break paths containing spaces
The PR removed the Python-side os.path.expanduser() call for ~ targets and replaced it with passing the target unquoted to the shell when it starts with ~, $, or equals -. This regresses cd for paths containing spaces. For example, cd "~/My Documents" (shlex.split → ['cd', '~/My Documents']) previously worked because the old code expanded ~ with os.path.expanduser (→ /home/user/My Documents) then safely quoted it with shlex.quote. Now the unquoted target produces the shell command cd ~/My Documents && pwd, where the shell sees ~/My and Documents as separate words, causing cd to fail.
Reproduction trace
Old code path: target="~/My Documents" → os.path.expanduser(...) → "/home/user/My Documents" → shlex.quote(...) → "'/home/user/My Documents'" → shell: cd '/home/user/My Documents' && pwd → ✅
New code path: target="~/My Documents" → needs_shell_expansion=True → quoted_target="~/My Documents" (unquoted) → shell: cd ~/My Documents && pwd → ❌ (word-split by shell)
Prompt for agents
In src/kimi_cli/ui/shell/__init__.py, lines 559-567 in _handle_cd: the current approach of passing tilde and dollar targets unquoted to the shell breaks when the target contains spaces (e.g. cd ~/My Documents). The fix should properly handle shell expansion while also quoting for spaces. One approach is to use shlex.quote on the target but selectively handle tilde expansion: for ~ targets, expand on the Python side with os.path.expanduser first (as the old code did), then shlex.quote the result. For $ targets, you could expand with os.path.expandvars first, then shlex.quote. For - targets, shlex.quote('-') works fine. This ensures both shell-like expansion AND proper handling of spaces/special characters.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57c01db6c9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if split_cmd and split_cmd[0] == "cd" and len(split_cmd) <= 2: | ||
| await self._handle_cd(split_cmd) |
There was a problem hiding this comment.
The len(split_cmd) <= 2 gate still captures commands like cd /tmp&&pwd or cd ~/x;ls because shlex.split keeps &&/; inside the second token when no spaces are present. Those are valid shell command chains, but this branch routes them into _handle_cd instead of executing normally: for quoted targets it fails as cd '/tmp&&pwd', and for ~/$ targets it can execute extra probe-side commands with output swallowed. This is a functional regression for common no-space shell syntax.
Useful? React with 👍 / 👎.
src/kimi_cli/ui/shell/__init__.py
Outdated
| execute_with_pty_capture if sys.platform != "win32" else execute_with_pipe_capture | ||
| ) | ||
| exit_code, raw_output = await execute(command, env=get_clean_env(), cwd=os.getcwd()) | ||
| except Exception as e: |
There was a problem hiding this comment.
On win32, this function now calls execute_with_pipe_capture, but the surrounding handler only catches Exception. Interrupts during shell commands are raised as KeyboardInterrupt/asyncio.CancelledError (BaseException), so they bypass this block and can tear down the shell loop instead of returning to the prompt after stopping the subprocess. The previous implementation installed explicit SIGINT handling, so this is a regression for interactive interruption on the Windows execution path.
Useful? React with 👍 / 👎.
- Windows: fall back to pipe capture when PTY is unavailable - PTY: catch asyncio.CancelledError alongside KeyboardInterrupt to prevent Ctrl+C from tearing down the shell loop - Security: neutralise <system-reminder> tags in captured output to prevent injection via untrusted shell output - cd: preserve shell expansion for ~, $VAR, and - targets instead of quoting them with shlex.quote
n-WN
force-pushed
the
feat/shell-mode-context-injection
branch
from
57c01db to
96a434c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96a434c44a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| needs_shell_expansion = target.startswith("~") or target.startswith("$") or target == "-" | ||
| quoted_target = target if needs_shell_expansion else shlex.quote(target) |
There was a problem hiding this comment.
Because shlex.split() removes the user's original quotes, this branch emits target unquoted whenever it starts with ~ or $, then splices it into cd {quoted_target} && pwd. That breaks valid inputs like cd ~/'tmp space' or cd "$HOME/tmp space" (the probe sees multiple args and fails), and it can also reinterpret quoted literals containing shell metacharacters. The persistent-cd path should not drop quoting semantics for expansion cases.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96a434c44a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| # Let the shell resolve ~, -, $HOME, CDPATH, etc. | ||
| probe = await asyncio.create_subprocess_shell( | ||
| f"cd {quoted_target} && pwd", |
There was a problem hiding this comment.
pwd in cd resolver
The cd probe command is hardcoded as cd {quoted_target} && pwd, which assumes a POSIX shell, but _handle_cd is called on all platforms. In Windows shell mode, asyncio.create_subprocess_shell runs via cmd.exe, where pwd is not a builtin, so the probe fails and every cd command is treated as an error; as a result, os.chdir() and session.work_dir are never updated for Windows users.
Useful? React with 👍 / 👎.
| # expansion: ~, -, and $VAR references. For those we let the real | ||
| # shell handle expansion directly. | ||
| needs_shell_expansion = target.startswith("~") or target.startswith("$") or target == "-" | ||
| quoted_target = target if needs_shell_expansion else shlex.quote(target) |
There was a problem hiding this comment.
cd targets
For targets starting with ~ or $, the code intentionally bypasses shlex.quote and injects the raw token into the probe command, which loses the user's quoting context. Valid commands like cd ~/"Application Support" or cd "$HOME/My Folder" become unquoted (cd ~/Application Support && pwd), get word-split by the shell, and fail even though the original command is valid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 165b13b6f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| needs_shell_expansion = target.startswith("~") or target.startswith("$") or target == "-" | ||
| quoted_target = target if needs_shell_expansion else shlex.quote(target) |
There was a problem hiding this comment.
cd target
The cd resolver only treats targets as expandable when target.startswith("$"), so commands like cd src/$MODULE or cd ./$PROJECT are incorrectly quoted and passed to the shell as literals (e.g. cd 'src/$MODULE'), which prevents variable expansion and makes valid shell paths fail. This is a functional regression in the new persistent-cd flow for users who compose paths with env vars outside the first character.
Useful? React with 👍 / 👎.
| proc = await asyncio.create_subprocess_shell( | ||
| command, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.STDOUT, | ||
| env=env, | ||
| cwd=cwd, | ||
| ) | ||
| assert proc.stdout is not None | ||
| chunks: list[bytes] = [] | ||
| captured_bytes = 0 | ||
| while True: | ||
| chunk = await proc.stdout.read(4096) | ||
| if not chunk: | ||
| break | ||
| try: | ||
| sys.stdout.buffer.write(chunk) | ||
| sys.stdout.buffer.flush() | ||
| except (OSError, AttributeError): | ||
| pass | ||
| chunks.append(chunk) | ||
| captured_bytes += len(chunk) | ||
| if captured_bytes > _CAPTURE_HARD_LIMIT * 2: | ||
| while chunks and captured_bytes > _CAPTURE_HARD_LIMIT: | ||
| evicted = chunks.pop(0) | ||
| captured_bytes -= len(evicted) | ||
| await proc.wait() | ||
| raw = b"".join(chunks).decode("utf-8", errors="replace") | ||
| return proc.returncode, raw |
There was a problem hiding this comment.
🔴 Missing KeyboardInterrupt handling in execute_with_pipe_capture causes shell crash on Windows
On Windows, execute_with_pipe_capture is selected as _execute_capture (src/kimi_cli/ui/shell/__init__.py:32). Unlike execute_with_pty_capture which explicitly catches (KeyboardInterrupt, asyncio.CancelledError) at src/kimi_cli/ui/shell/capture.py:166, the pipe capture variant has no such handling. When Ctrl+C is pressed during a shell command on Windows, KeyboardInterrupt (a BaseException) propagates out of execute_with_pipe_capture, through _run_shell_command (whose except Exception at src/kimi_cli/ui/shell/__init__.py:533 does not catch BaseException subclasses), and up through the shell's run() loop which also lacks a KeyboardInterrupt catch — crashing the interactive session. The old code installed a custom SIGINT handler via install_sigint_handler that gracefully terminated the child process and returned control to the shell prompt. Additionally, the child process is not explicitly terminated and stdout pipe resources are not cleaned up.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 000e82bc93
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| parts.append(f"(Output truncated to last {SHELL_OUTPUT_MAX_BYTES} bytes)") | ||
|
|
||
| header = " ".join(parts) | ||
| body = f"$ {command}\n{output}" |
There was a problem hiding this comment.
inject_to_context now escapes <system-reminder> tags in output, but it still interpolates raw command into the same wrapped reminder body. A shell command containing </system-reminder> (for example via copied untrusted command text) will prematurely close the wrapper and can inject new authoritative reminders into model context, because this content is later embedded inside system_reminder(...) without sanitizing the command string.
Useful? React with 👍 / 👎.
| parts.append(f"(Output truncated to last {SHELL_OUTPUT_MAX_BYTES} bytes)") | ||
|
|
||
| header = " ".join(parts) | ||
| body = f"$ {command}\n{output}" |
There was a problem hiding this comment.
🟡 Command string not sanitized for system-reminder tag injection in inject_to_context
The output is explicitly sanitized to neutralize <system-reminder> / </system-reminder> tags (lines 258-259), but the command string is embedded in the body at line 278 (f"$ {command}\n{output}") without the same sanitization. Since the body is wrapped via system_reminder() (src/kimi_cli/soul/message.py:24) which adds <system-reminder>…</system-reminder> tags, a command containing </system-reminder> (e.g. from a copy-pasted script or test command) would prematurely close the outer tag, corrupting the structured message. This is inconsistent with the comment at line 257 stating the intent to "prevent injection".
| body = f"$ {command}\n{output}" | |
| command_safe = command.replace("<system-reminder>", "<system-reminder>").replace("</system-reminder>", "</system-reminder>") | |
| body = f"$ {command_safe}\n{output}" |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
<system-reminder>user messages, so the agent knows what the user rancdis now persistent — resolved via real shell (~,-,CDPATHall work),os.chdir()+session.work_dirkept in synccapture.pymodule avoids duplicating logic between shell mode and future!prefixArchitecture
capture.py— shared layer with four orthogonal functionsclean_output()\r\nnormalize → C0 strip →\roverwrite resolve → ANSI strip via richexecute_with_pty_capture()openpty+ async subprocess +add_readertee — TTY preservedexecute_with_pipe_capture()!prefix, no TTY needed)inject_to_context()<system-reminder>wrap, decoupled viaappend_messagecallbackPTY capture design
loop.add_reader(master_fd)tees output to real stdout + capture buffertermiossave/restore to handle commands that corrupt terminal stateKeyboardInterrupt→ forward SIGINT → 3s timeout → kill_CAPTURE_HARD_LIMIT(100KB) to preventcat /dev/urandomOOMTIOCGWINSZ/TIOCSWINSZcdpersistencecd target && pwdin real shell resolves all edge cases (~,-,$HOME,CDPATH, symlinks)OLDPWDtracked inos.environforcd -across invocationssession.work_dirupdated so agent background tasks see the new directoryTest plan
uv run pytest tests— 1336 passed, 6 skippeduv run pytest tests/e2e— 45 passed, 1 skipped, 1 xfailedls, switch back to agent mode, ask "what did I just run" — agent should knowcd /tmp→pwdconfirms/tmp→ agent's Shell tool also runs in/tmpcd -toggles back to previous directory