Repository navigation
refactor(server): remove file watcher, daemon/relay/socket modes, rename to serve - #473
Merged
Merged
Conversation
…ame server to serve - Remove file_watcher_task and fs_event-based file watching (future: periodic stat-based) - Remove Daemon, Relay, Socket server modes; simplify to Pipe + Tcp - Remove run_daemon_mode, run_relay_mode, DaemonConnection, relay_forward - Remove default_socket_path utility - Rename server subcommand to serve, run_server_mode to run_serve_mode - Add --workspace support to serve mode for standalone (no-editor) startup - Rename extension socket mode to tcp, update package.json enum - Update all test files to use "serve" subcommand and "tcp" mode
|
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:
📝 WalkthroughWalkthroughRenames the Changesserve subcommand and mode simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/en/dev/test-and-debug.md (1)
135-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate remaining "socket" references to "tcp".
Line 135: "socket config" → "tcp config"
Line 136: "VSCode Extension (socket)" → "VSCode Extension (tcp)" (verify against.vscode/launch.jsonconfiguration names)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/en/dev/test-and-debug.md` around lines 135 - 136, Update the remaining “socket” terminology to “tcp” in the test-and-debug docs, including the launch option referenced in the section that mentions VSCode extension debugging. Make sure the text now matches the actual configuration names used in .vscode/launch.json, and update any nearby references such as the “socket config” wording and the “VSCode Extension (socket)” label to “tcp”.docs/zh/guide/editors.md (1)
99-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win将 Vim 配置中的
server更新为serve。Vim 配置仍引用旧的
server子命令，应改为['clice', 'serve']。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/zh/guide/editors.md` at line 99, The Vim configuration example still references the पुराने `server` subcommand instead of `serve`; update the example in the editor guide to use the new `clice` command target so it matches the current CLI. Locate the snippet containing `server_info` and the `cmd` mapping, and replace the old subcommand reference with `serve` consistently.docs/zh/dev/test-and-debug.md (1)
133-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win将 "socket 配置" 更新为 "tcp 配置" 以匹配上方配置示例。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/zh/dev/test-and-debug.md` at line 133, The documentation text references “socket 配置” where it should match the earlier TCP example; update the wording in the test/debug guide to “tcp 配置” so the instruction is consistent with the surrounding configuration example. Locate the sentence containing the `.vscode/settings.json` step and replace the mismatched term only.docs/en/guide/editors.md (1)
99-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate Vim config to use
servesubcommand.The Vim configuration still references the old
serversubcommand. Change['clice', 'server']to['clice', 'serve']to match the renamed CLI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/en/guide/editors.md` at line 99, The Vim configuration example still points to the old server subcommand; update the command mapping in the editors guide to use the renamed CLI entry point. In the snippet that defines the Vim server command, replace the `server` subcommand with `serve` so it matches the current `clice` CLI and the rest of the docs.
🧹 Nitpick comments (1)
src/server/service/master_server.h (1)
83-83: 📐 Maintainability & Code Quality | 🔵 TrivialTrack the file-watcher replacement outside this inline TODO.
The removed watcher previously handled workspace freshness; leaving only this TODO makes it easy to ship without a follow-up for compile database/source-change invalidation.
Do you want me to draft an issue for the periodic stat-based watcher?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/service/master_server.h` at line 83, The inline TODO in master_server.h is too easy to miss, so move the file-watcher replacement work into a tracked follow-up item instead of leaving only the comment. Update the relevant master_server handling around the watcher/freshness path to reference a concrete issue or task for periodic stat-based file watching and compile database/source-change invalidation, and keep the TODO only as a short pointer to that tracked work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.vscode/launch.json:
- Around line 15-17: The debug launch entry is mislabeled: the visible name in
the launch configuration still says socket even though it now uses tcp mode.
Update the name field in the launch config so it matches the mode used by the
clice launch entry, keeping the debug picker consistent with the arguments.
In `@docs/en/dev/test-and-debug.md`:
- Line 28: The wording in the test-and-debug docs is awkward because “clice
serve” is used as a plain phrase instead of a command reference. Update the
sentence in the relevant docs section to rephrase it naturally and wrap the
command name in backticks, using the existing “end-to-end tests” sentence as the
target so it reads like a real `clice serve` instance communicating via the LSP
protocol.
- Line 82: The wording in the debugger setup text uses “socket mode” but the
referenced command is for TCP mode; update the phrasing in the affected
documentation sentence to say “tcp mode” so it matches the command and
terminology used elsewhere in the same section.
In `@docs/zh/dev/test-and-debug.md`:
- Line 82: The documentation sentence currently says “socket 模式,” but it should
match the command shown below by using “tcp 模式” instead. Update the wording in
the relevant debug guide text so the description and example are consistent, and
keep the surrounding guidance about starting clice separately and then
connecting the client.
In `@editors/vscode/src/setting.ts`:
- Around line 15-18: The mode validation in getSetting currently rejects legacy
persisted values, so add a compatibility mapping for the old "socket" setting
before the unexpected-mode check and normalize it to "tcp" so existing users
keep working. Update the getSetting flow to handle this alias alongside the
current "pipe" and "tcp" values, and preserve the existing error path for any
other invalid mode.
In `@src/server/service/master_server.cpp`:
- Around line 407-410: The TCP startup path in master_server.cpp is currently
allowing the default port 0, which can create an unknown ephemeral bind when
`--mode tcp` is used. Update the `opts.port` handling in the master server
startup flow (including the related logic near the TCP listen/setup code and the
duplicate location mentioned in the review) to validate ports the same way the
query command does: require 1–65535 for TCP mode, while preserving 0 only as the
disabled/default value for pipe mode. Use the existing `ServerMode` and the
master server’s port parsing/listening logic to keep the behavior consistent.
- Around line 417-419: Move the pre-initialization in master_server.cpp so
server.initialize(ws) only runs after stdio/TCP startup has succeeded, since
calling it early can launch background work before later failures return. Update
the startup flow around server.initialize, the stdio/TCP setup path, and any
early-return failure branches to either defer initialization or invoke
shutdown_and_cleanup before exiting. Also apply the same cleanup/defer fix in
the related startup sections noted by the review.
In `@tests/conftest.py`:
- Around line 20-23: The pytest fixture currently advertises tcp support but the
`client()` setup still only starts the stdio path, so `--mode tcp` is not
actually wired through. Update the fixture logic around `client()` and
`start_io()` to either restrict the option to pipe-only or branch on `--mode` so
tcp mode passes the configured `--port` and connects with
`CliceClient.start_tcp()`. Make sure the tcp path uses the correct server
startup and client connection flow instead of the stdio client.
---
Outside diff comments:
In `@docs/en/dev/test-and-debug.md`:
- Around line 135-136: Update the remaining “socket” terminology to “tcp” in the
test-and-debug docs, including the launch option referenced in the section that
mentions VSCode extension debugging. Make sure the text now matches the actual
configuration names used in .vscode/launch.json, and update any nearby
references such as the “socket config” wording and the “VSCode Extension
(socket)” label to “tcp”.
In `@docs/en/guide/editors.md`:
- Line 99: The Vim configuration example still points to the old server
subcommand; update the command mapping in the editors guide to use the renamed
CLI entry point. In the snippet that defines the Vim server command, replace the
`server` subcommand with `serve` so it matches the current `clice` CLI and the
rest of the docs.
In `@docs/zh/dev/test-and-debug.md`:
- Line 133: The documentation text references “socket 配置” where it should match
the earlier TCP example; update the wording in the test/debug guide to “tcp 配置”
so the instruction is consistent with the surrounding configuration example.
Locate the sentence containing the `.vscode/settings.json` step and replace the
mismatched term only.
In `@docs/zh/guide/editors.md`:
- Line 99: The Vim configuration example still references the पुराने `server`
subcommand instead of `serve`; update the example in the editor guide to use the
new `clice` command target so it matches the current CLI. Locate the snippet
containing `server_info` and the `cmd` mapping, and replace the old subcommand
reference with `serve` consistently.
---
Nitpick comments:
In `@src/server/service/master_server.h`:
- Line 83: The inline TODO in master_server.h is too easy to miss, so move the
file-watcher replacement work into a tracked follow-up item instead of leaving
only the comment. Update the relevant master_server handling around the
watcher/freshness path to reference a concrete issue or task for periodic
stat-based file watching and compile database/source-change invalidation, and
keep the TODO only as a short pointer to that tracked work.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f12f6ba-238c-4e04-b4b1-7d3b06183dc2
📒 Files selected for processing (21)
.vscode/launch.jsondocs/en/dev/test-and-debug.mddocs/en/guide/editors.mddocs/zh/dev/test-and-debug.mddocs/zh/guide/editors.mdeditors/vscode/package.jsoneditors/vscode/src/extension.tseditors/vscode/src/setting.tseditors/zed/src/clice.rssrc/clice.ccsrc/server/service/agentic.cppsrc/server/service/agentic.hsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/support/filesystem.htests/conftest.pytests/integration/agentic/test_agentic.pytests/integration/agentic/test_cli.pytests/integration/lifecycle/test_protocol_robustness.pytests/replay.pytests/stress.py
💤 Files with no reviewable changes (3)
- src/support/filesystem.h
- src/server/service/agentic.h
- src/server/service/agentic.cpp
![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: cc5ad179c5
ℹ️ 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".
- Fix stale launch.json label (socket → tcp) - Update nvim editor references (server → serve) - Fix docs: "socket mode" → "tcp mode", backtick clice serve - Add socket → tcp compat alias in VS Code extension settings - Validate --port range for tcp mode - Defer server.initialize(ws) until after transport setup succeeds
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editors/vscode/src/setting.ts (1)
24-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject out-of-range TCP ports here.
Line 24 only checks truthiness, so values like
-1or70000still pass validation and get returned as a usable setting. Tightening this to1..65535keeps invalid config out of the TCP path.Proposed fix
- if (mode === "tcp" && (!host || !port)) { - vscode.window.showErrorMessage("TCP mode requires both host and port to be configured."); + if ( + mode === "tcp" && + (!host || !Number.isInteger(port) || port < 1 || port > 65535) + ) { + vscode.window.showErrorMessage("TCP mode requires a host and a port between 1 and 65535."); return undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editors/vscode/src/setting.ts` around lines 24 - 25, The TCP validation in the setting lookup is only checking truthiness, so invalid numeric ports like -1 or 70000 can still pass through. Update the validation in the setting getter around the TCP branch to explicitly validate the port range for the existing mode/host/port handling, and only allow TCP when the port is an integer between 1 and 65535; keep the error path in the same vscode.window.showErrorMessage flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@editors/vscode/src/setting.ts`:
- Around line 24-25: The TCP validation in the setting lookup is only checking
truthiness, so invalid numeric ports like -1 or 70000 can still pass through.
Update the validation in the setting getter around the TCP branch to explicitly
validate the port range for the existing mode/host/port handling, and only allow
TCP when the port is an integer between 1 and 65535; keep the error path in the
same vscode.window.showErrorMessage flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b69cab5-2c8a-417d-8f9b-155659593496
📒 Files selected for processing (7)
.vscode/launch.jsondocs/en/dev/test-and-debug.mddocs/zh/dev/test-and-debug.mdeditors/nvim/doc/clice.luaeditors/nvim/tests/e2e.luaeditors/vscode/src/setting.tssrc/server/service/master_server.cpp
✅ Files skipped from review due to trivial changes (1)
- editors/nvim/doc/clice.lua
🚧 Files skipped from review as they are similar to previous changes (3)
- .vscode/launch.json
- docs/zh/dev/test-and-debug.md
- src/server/service/master_server.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09c903960b
ℹ️ 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".
When --workspace pre-initializes the server, TCP connections are agentic-only. Don't consume the LSP slot for query clients.
The test fixture always uses pipe mode via start_io(). The --mode flag was never properly wired for tcp and would cause hangs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8efbbe26a
ℹ️ 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".
The --mode and --port pytest options were removed since tests always use pipe mode. Remove the corresponding documentation section.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55ef9849ec
ℹ️ 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".
The socket mode name was already established and changing it to tcp introduced unnecessary breaking changes for existing users.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
file_watcher_taskandfs_event-based file watching (TODO: future periodic stat-based version)Daemon,Relayserver modes; simplifyServerModeenum to{ Pipe, Socket }run_daemon_mode,run_relay_mode,DaemonConnection,relay_forward,default_socket_pathserversubcommand toserve,run_server_modetorun_serve_mode--workspaceflag to serve mode for standalone (no-editor) startuppackage.jsonenumTest plan