feat(vis): add session dir actions and Windows open-in support#1416
feat(vis): add session dir actions and Windows open-in support#1416
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ff47633ed
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| application.include_router(sessions_router) | ||
| application.include_router(statistics_router) | ||
| application.include_router(open_in_router) |
There was a problem hiding this comment.
Protect open-in route before mounting it in vis app
Including open_in_router in the vis server exposes /api/open-in on an app that has permissive CORS and no auth checks, so any web page the user visits can issue cross-origin POSTs to the local vis process and trigger local app launches while it is running. This is a regression in protection for a sensitive endpoint: in the web server, the same router is behind AuthMiddleware and can be disabled via restrict_sensitive_apis, but this mount bypasses those safeguards.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds “open/copy session directory” functionality to the Tracing Visualizer UI by exposing a new /api/open-in endpoint from the vis server and returning the session directory path in the sessions listing, with Windows support and accompanying tests.
Changes:
- Broaden Vite dev proxy to forward all
/api/*requests (so/api/open-inworks in dev). - Return
session_dirin vis session listings and surface UI actions to open/copy the directory. - Extend
open-inbackend API to support Windows + add tests for the new API surface.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vis/vite.config.ts | Proxy /api/* to backend (enables /api/open-in during frontend dev). |
| vis/src/lib/api.ts | Adds session_dir to SessionInfo and implements openInPath() client call. |
| vis/src/App.tsx | Adds UI actions to open/copy the current session directory; improves title derivation and refresh behavior. |
| tests/web/test_open_in.py | Verifies Windows command invocation behavior for open-in. |
| tests/vis/test_app.py | Ensures session_dir is included in /api/vis/sessions and that /api/open-in is mounted. |
| src/kimi_cli/web/api/open_in.py | Adds Windows implementations and broadens platform support beyond macOS. |
| src/kimi_cli/vis/app.py | Mounts the open-in router into the vis FastAPI app. |
| src/kimi_cli/vis/api/sessions.py | Includes session_dir in session scan results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| application.include_router(sessions_router) | ||
| application.include_router(statistics_router) | ||
| application.include_router(open_in_router) |
There was a problem hiding this comment.
Including open_in_router in the vis app exposes a sensitive local-operation endpoint (/api/open-in) without the auth / origin controls used by the main web app (see src/kimi_cli/web/app.py, which gates this behind restrict_sensitive_apis + AuthMiddleware). With the current permissive CORS in vis (allow_origins=['*']), this increases risk of cross-site requests triggering local app launches or probing local filesystem paths. Please gate this route behind a similar "restrict sensitive APIs" flag and/or add request-origin / local-client checks before enabling it.
|
|
||
|
|
||
| def _open_windows_app(command: str, path: Path) -> None: | ||
| _run_command(["cmd", "/c", "start", "", command, str(path)]) |
There was a problem hiding this comment.
_open_windows_app uses cmd /c start with command and path passed as separate args. On Windows this runs through cmd.exe parsing, and arguments that don’t contain spaces may not be quoted/escaped (e.g. paths containing &, |, etc.), which can allow command injection if a crafted existing path is provided. Prefer launching the executable directly (e.g. subprocess.Popen([command, str(path)], ...) with appropriate detached creation flags) or ensure the arguments are safely quoted/escaped for cmd regardless of spaces.
| _run_command(["cmd", "/c", "start", "", command, str(path)]) | |
| # Launch the Windows application directly to avoid cmd.exe parsing | |
| subprocess.Popen([command, str(path)], close_fds=True) |
src/kimi_cli/web/api/open_in.py
Outdated
| @router.post("", summary="Open a path in a local application") | ||
| async def open_in(request: OpenInRequest) -> OpenInResponse: | ||
| if sys.platform != "darwin": | ||
| if sys.platform not in {"darwin", "win32"}: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Open-in is only supported on macOS.", | ||
| detail="Open-in is only supported on macOS and Windows.", | ||
| ) | ||
|
|
||
| path = _resolve_path(request.path) | ||
| is_file = path.is_file() | ||
|
|
||
| try: | ||
| match request.app: | ||
| case "finder": | ||
| if is_file: | ||
| # Reveal file in Finder | ||
| _run_command(["open", "-R", str(path)]) | ||
| else: | ||
| _run_command(["open", str(path)]) | ||
| case "cursor": | ||
| _open_app("Cursor", path) | ||
| case "vscode": | ||
| _open_app("Visual Studio Code", path, fallback="Code") | ||
| case "antigravity": | ||
| _open_app("Antigravity", path) | ||
| case "iterm": | ||
| # Terminal apps need directory | ||
| directory = path.parent if is_file else path | ||
| _open_iterm(directory) | ||
| case "terminal": | ||
| directory = path.parent if is_file else path | ||
| _open_terminal(directory) | ||
| case _: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=f"Unsupported app: {request.app}", | ||
| ) | ||
| if sys.platform == "darwin": | ||
| _open_in_macos(request, path, is_file=is_file) | ||
| else: | ||
| _open_in_windows(request, path, is_file=is_file) |
There was a problem hiding this comment.
open_in is declared async but calls subprocess.run(...) (via _run_command) directly. This blocks the event loop while the OS command executes, which can stall other requests on the vis server. Consider running the open operation in a worker thread (e.g. await asyncio.to_thread(...)) or otherwise offloading the blocking call so the FastAPI app stays responsive.
src/kimi_cli/web/api/open_in.py
Outdated
| def _open_windows_explorer(path: Path, *, is_file: bool) -> None: | ||
| if is_file: | ||
| _run_command(["explorer", f"/select,{path}"]) | ||
| else: | ||
| _run_command(["explorer", str(path)]) |
There was a problem hiding this comment.
🔴 explorer.exe returns non-zero exit code on success, causing false error with check=True
_open_windows_explorer invokes explorer.exe directly via _run_command (src/kimi_cli/web/api/open_in.py:50-56), which calls subprocess.run(..., check=True). On Windows, explorer.exe is well-known for returning exit code 1 even when the operation succeeds (because it delegates to the already-running Explorer shell process via COM). Since check=True raises CalledProcessError on any non-zero exit code, the call at line 99 or 101 will raise an exception in typical Windows desktop usage. The outer exception handler in open_in() at src/kimi_cli/web/api/open_in.py:179-185 then converts this to an HTTP 500 error—even though the Explorer window actually opened correctly. The user sees a "Failed to open session directory" error alert in the UI despite the folder being shown. Other Windows functions (_open_windows_app, _open_windows_terminal) avoid this by going through cmd /c start, which returns 0 immediately.
| def _open_windows_explorer(path: Path, *, is_file: bool) -> None: | |
| if is_file: | |
| _run_command(["explorer", f"/select,{path}"]) | |
| else: | |
| _run_command(["explorer", str(path)]) | |
| def _open_windows_explorer(path: Path, *, is_file: bool) -> None: | |
| if is_file: | |
| subprocess.Popen(["explorer", f"/select,{path}"]) | |
| else: | |
| subprocess.Popen(["explorer", str(path)]) | |
Was this helpful? React with 👍 or 👎 to provide feedback.
…ccess on macOS and Windows
Signed-off-by: Kai <me@kaiyi.cool>
Summary
Open DirandCopy DIRactions to the Kimi vis session pagesession_dirin the vis API and wire/api/open-ininto the vis serverCopy DIRcopy the raw session directory pathopen-into support Windows in addition to macOSopen-inworks in local devopen-inbehaviorChecklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.