refactor(command): subcommand-based CLI with SubCommander - #447
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR replaces flag-based --mode invocations with subcommands (server, query, worker, lint, format), adds per-command option structs and log-level parsing, updates server/daemon APIs to accept ServerOptions+self_path, changes worker spawn argv to positional "worker" with optional --memory-limit, and updates tests, editors, and docs. ChangesCLI Subcommand Refactor
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Master as MasterServer
participant WorkerPool as WorkerPool
participant Worker as WorkerProcess
participant TestHarness as Tests/Editors
CLI->>Master: "server --mode socket --port 50051" (dispatch)
CLI->>Master: "query ..." (dispatch)
Master->>WorkerPool: request spawn worker (self_path, "worker", [--memory-limit])
WorkerPool->>Worker: start process with args {self_path, "worker", ...}
TestHarness->>CLI: launch clice via "server"/"query"
TestHarness->>Master: connect to host:port or stdio (as applicable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 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 `@src/clice.cc`:
- Around line 113-142: In apply_log_level, when spdlog::level::from_str returns
spdlog::level::off but the input string isn't "off" (both in the "--log-level
<val>" branch that uses *std::next(it) and the "--log-level=<val>" branch that
uses val), after printing the stderr error message immediately terminate parsing
with a non-zero exit (e.g., call std::exit(1) or throw a fatal error) instead of
continuing; update both places where the unknown-level path is handled so the
process stops on invalid log-level input rather than consuming the argument and
proceeding.
🪄 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: 5c4d5d49-c888-4628-8c25-70b50a962583
📒 Files selected for processing (11)
.vscode/launch.jsoneditors/nvim/doc/clice.luaeditors/vscode/src/extension.tseditors/zed/src/clice.rssrc/clice.ccsrc/server/worker/worker_pool.cpptests/conftest.pytests/integration/agentic/test_agentic.pytests/integration/agentic/test_cli.pytests/replay.pytests/unit/server/worker_test_helpers.h
16bit-ykiko
force-pushed
the
refactor/subcommander-cli
branch
2 times, most recently
from
June 7, 2026 13:43
0569359 to
042d847
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/clice.cc`:
- Around line 19-27: Each subcommand's options structs (e.g., LintOpts,
FormatOpts and the other per-subcommand option structs in this file) only
declare --help and lack a --version flag, causing "clice <subcommand> --version"
to error; add a DecoFlag for version to each subcommand options struct (for
example DecoFlag(names = {"-V", "--version"}, help = "Show version", required =
false) version;) and update each subcommand handler (the functions that inspect
those structs) to detect the new version field, print the program/version string
and exit/return early when set.
- Around line 68-70: The global --log-level flag must be preprocessed from the
argv vector before subcommand parsing: after you build args with
deco::util::argvify(argc, argv) (the args variable near the current self_path
usage), scan args for a global --log-level (and short forms if supported) and
apply it to the global logger/configuration immediately (call your logger
configuration helper or setLogLevel/processLogger) before constructing or
invoking subcommand parsers; then remove or defer any duplicate
per-subcommand-only handling so that invocations like "clice --log-level debug
server" take effect prior to parsing/dispatching subcommands (see the blocks
around lines referenced: the args creation and the subcommand parsing sections).
In `@src/server/service/master_server.cpp`:
- Around line 434-442: The socket-mode startup currently allows port==0
(ephemeral) which logs host:0 and is unusable; in the ServerMode::Socket branch
(check for mode == ServerMode::Socket) validate that the parsed port is non-zero
before calling kota::tcp::listen, and if port==0 log an explicit error via
LOG_ERROR (mentioning the provided host and that a non-zero --port is required)
and return a non-zero exit code; apply this check just before calling
kota::tcp::listen/accept_connections so acceptor is never created with port 0
and callers can discover the required fix.
🪄 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: 44272818-a23e-42be-b816-865c98cb74b6
📒 Files selected for processing (19)
.vscode/launch.jsoneditors/nvim/doc/clice.luaeditors/vscode/src/extension.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/server/worker/worker_pool.cpptests/conftest.pytests/integration/agentic/test_agentic.pytests/integration/agentic/test_cli.pytests/replay.pytests/unit/server/module_worker_tests.cpptests/unit/server/pch_worker_tests.cpptests/unit/server/stateful_worker_tests.cpptests/unit/server/stateless_worker_tests.cpptests/unit/server/worker_test_helpers.h
✅ Files skipped from review due to trivial changes (1)
- editors/vscode/src/extension.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- .vscode/launch.json
- editors/zed/src/clice.rs
- tests/replay.py
- editors/nvim/doc/clice.lua
- tests/integration/agentic/test_agentic.py
- tests/conftest.py
- tests/integration/agentic/test_cli.py
16bit-ykiko
force-pushed
the
refactor/subcommander-cli
branch
3 times, most recently
from
June 8, 2026 18:00
c0cf81e to
6b8fe77
Compare
Member
Author
![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: e2efbc1de6
ℹ️ 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".
- Guard `opts.port` dereference with `value_or(0)` instead of `*` - Add LOG_ERROR for unreachable server mode fallthrough - Remove `using deco::decl::KVStyle` from headers to avoid namespace pollution - Replace comma operator returns with explicit two-statement form
- Add --stateful flag to worker subcommand instead of implicitly inferring worker type from --memory-limit presence - --memory-limit becomes optional config for stateful workers (default 4GB) - Add test_daemon_requires_workspace (daemon without --workspace exits non-zero) - Add test_socket_mode_connects (socket mode accepts LSP connections)
16bit-ykiko
force-pushed
the
refactor/subcommander-cli
branch
from
June 9, 2026 14:24
e2efbc1 to
69c13dc
Compare
16bit-ykiko
changed the title
4 tasks
6 tasks
16bit-ykiko
added a commit
that referenced
this pull request
Jul 3, 2026
## Background Server-side error feedback is the operability foundation for v1.0 (roadmap: server task 3 "error handling & logging"). Before this PR, internal failures died silently in logs (or as silent `null` responses), users got no signal when their setup was broken, and there was no structured way to profile the server on real codebases. This PR supersedes #436 (reimplemented on current main rather than rebased — it predates the #437/#447/#449 restructures). ## Feedback channels **Anomaly (soft assertions, `src/support/anomaly.h`)** — `LOG_ANOMALY(id, fmt, ...)` for internal states that must be unreachable when clice works correctly. Debug builds abort after logging (the CI Debug matrix and local dev catch bugs earliest; `CLICE_ANOMALY_NO_TRAP` exists for tests); Release logs `[anomaly:<id>]`, pushes `window/logMessage` (master) and continues. Per-ID rate limit with a final suppression notice; the gate runs **before** format arguments are evaluated (lazy contract locked by unit tests with side-effecting args). IDs: `PCHBuildFail`, `PCMBuildFail`, `CompileFail`, `WorkerRequestFail`, `WorkerCrash`, `WorkerSpawnFail`, `PositionMapFail` (markers are the enumerator names, rendered by the reflective enum formatter and pinned by `MarkerNamesStable`). Situations reachable by user input or normal operation are deliberately **not** anomalies. That split is structural: worker build failures carry `BuildResult.has_user_errors`, and master-side dispatch failures carry `dispatch_errc` codes (`cancelled` for memory-pressure preemption, `worker_unavailable` for crash/restart windows) so `is_operational_error()` keeps them out of the anomaly channel. **Guidance** — `LOG_GUIDANCE(...)` (`[guidance]` + Warning logMessage) for user-actionable situations: no compile_commands.json, invalid initializationOptions, config problems. **LSP errors instead of silent null** — feature requests on closed documents → `Document not open`; unresolvable call/type hierarchy items → error; reversed ranges → `InvalidParams`; empty hierarchy/workspace-symbol results return `[]` instead of `null`. Out-of-range positions **clamp** per the LSP spec (character → line end, line → end of content) instead of erroring. **Guidance diagnostics** — `fill_compile_args()` returns a `CommandSource` (`CDBExact`/`IncludeGraph`/`Inferred` (reserved)/`Fallback`) and emits a per-file decision log (tiers tried, tier hit, args hash). When a *guessed* command produces file-not-found errors, the publish merges a file-top Warning explaining it (code `inferred-compile-command`, linking the quick-start guide). Exact CDB matches never get it. Config rule appends now reach synthesized fallback commands, so `-I` rules work without a CDB. **clice.toml diagnostics** — parse/type errors (Error, defaults apply) and unknown keys (Warning via a strict second decode pass, config still applies) are published on the config file's URI. Line/column ranges wait for the kotatsu TOML location feature (FIXMEs mark the re-enable points); until then diagnostics anchor at the file top. ## Logging system - **Design doc in `logging.h`**: channel selection is decision-oriented — pick by *who must act and whether it is clice's fault*. Levels, perf topics and process ownership are specified there. - **`LOG_PERF(topic, ...)`** emits greppable `[perf:<topic>] key=value` lines for profiling on real codebases: `startup` (CDB load, dep scan, index load), `index` (per-file index/merge timings, run summary, save), `cache` (PCH/PCM hit/miss with reason, evictions), `request` (per-request `wait_ms`/`total_ms` for query/build/format/compile). - **Per-process log files**: workers log only to their own `<session>/<worker>.log` (no stderr mirror), so the master log no longer duplicates worker output. Worker stderr is reserved for unexpected third-party output (sanitizers, libc asserts), relayed line-by-line into the master log by the pool. - **Crash backtraces** land in the owning process's log file (`install_crash_handler`, covers SIGABRT so `LOG_FATAL` and Debug anomaly traps too); a worker's backtrace goes *only* to its own log, verified by an integration test that SIGABRTs a worker and asserts the trace stays out of the master log. - **Startup flow** is fully logged: master banner (pid/mode/workspace), config source, session log dir, worker pool summary, CDB/dep-scan/index-load timings. ## Notable fixes uncovered along the way - `CompilationDatabase::lookup()` synthesizes a command for unknown files, so the old `results.empty()` checks were dead: the automatic include-graph header-context tier was unreachable, and headers without entries silently compiled as bare `clang`. Tier selection now uses `has_entry()`; background indexing skips Fallback-sourced files. - initializationOptions now overlay **before** `apply_defaults()`: a client-provided `cache_dir` previously didn't propagate into the derived `logging_dir`/`index_dir`. - Six unchecked `*map.to_range()` dereferences (empty-optional UB) in feature code converted to checked helpers reporting `PositionMapFail`. - Anomaly notify/trap hooks are mutex-synchronized (copy under lock, invoke outside), making reporting safe from any thread. ## Test infrastructure - `CliceClient` records `window/logMessage`; **`assert_no_anomaly()` runs in every integration teardown** (all fixtures and `make_client` sessions), scanning notifications and master/worker log files even when shutdown fails. `tests/replay.py` fails a smoke trace on any anomaly push. - E2E: guidance-diagnostic lifecycle (no CDB → appears; add CDB + restart → gone while the include error stays), fallback rule-append (no CDB + clice.toml `-I` rule → clean compile), clice.toml error/unknown-key/clear scenarios, worker SIGABRT → `WorkerCrash` anomaly + backtrace ownership, closed/unknown documents via `pytest.raises`, position/range clamping. - Unit triggers for `PositionMapFail`, `WorkerCrash`, `WorkerSpawnFail`; `dispatch_errc` classification; fallback append; lazy-evaluation and rate-limit contracts. - Named timing constants (`MTIME_GRANULARITY`/`SETTLE_TIME`/`IDLE_TIMEOUT`) replace hardcoded sleeps; yield-based workspace cleanup; log dump on test failure. ## Out of scope Module-cycle guidance diagnostics wait for the module refactor; kotatsu TOML line/column re-enable after the kotatsu-side PR merges; worker logMessage forwarding and request-correlation IDs belong to the worker-pool follow-up. ## Test plan - [x] RelWithDebInfo: 732 unit / 187 integration / 3 smoke — all green locally - [x] Debug: 732 unit / 187 integration / 3 smoke — all green locally (anomalies abort in Debug, so this run is the "zero anomalies on regular paths" acceptance) - [x] `pixi run format` clean; two rounds of 3 parallel review subagents (correctness / style / tests) — all findings fixed or triaged - [x] Full CI matrix green (Linux/macOS/Windows native, aarch64/x64/arm64 cross, editor test) Closes #436 (superseded).
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
--modeflag with git-style subcommands using kotatsu'sSubCommanderAPI:clice server,clice query,clice worker,clice lint,clice format. Each subcommand has its own--help.ServerModeenum (Pipe|Socket|Relay|Daemon) replaces string comparisons; the deco framework auto-parses enum values and renders them in help text.--statefulworker flag: Worker type is no longer inferred from the presence of--memory-limit.clice worker --statefulexplicitly selects stateful mode;--memory-limitis an optional config (defaults to 4 GB).Optionsstruct is split intoServerOptions,QueryOptions,WorkerOptions,LintOptions,FormatOptions— each declares only the flags it needs. Intermediate adapter structs (ServerOptions/DaemonOptions/AgenticQueryOptionsin old code) are eliminated; run functions receive the deco option structs directly.clice server [OPTIONS]syntax.Details
The old CLI used a single
kota::deco::cli::parse<Options>call with a--modestring that dispatched to 7 different code paths (pipe, socket, daemon, relay, agentic, stateless-worker, stateful-worker). This made help output noisy (all flags shown regardless of mode) and made it impossible to validate mode-specific required flags early.The new design registers each subcommand as an independent
Command<T>with its own option struct and handler. TheSubCommanderdispatches to the correct handler, and each handler validates only its own flags (e.g.queryvalidates--portrange before callingrun_agentic_mode).lintandformatare registered as placeholders — they print "not yet implemented" and exit. They're visible in help intentionally, to establish the public interface early.Test plan
serversubcommand)test_daemon_requires_workspace,test_socket_mode_connects)clice --help,clice --version,clice server --help,clice query --helpclice server --mode socket --port 50051accepts connectionsclice server --mode daemonwithout--workspaceexits non-zero