feat: mvp of cdb generator#62
Merged
Seele-Official merged 12 commits intoclice-io:mainfrom Mar 21, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds compiler-identification APIs and implementation, a CDB script service and scripts export, changes service onFinish to receive an ExecutionEvent and binds lifecycle callbacks, refactors IPC/session to use runtime pipe naming and ServiceBase, rewrites main to option-driven dispatch, and updates tests/configs. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI/Main
participant Dispatcher as Dispatcher
participant JS as JS Runtime
participant Session as Session
participant Service as ServiceImpl
participant IPC as IPC Layer
CLI->>Dispatcher: parse options (script, mode, args)
Dispatcher->>CLI: extracted config
CLI->>JS: load script (builtin or file) and call on_start
CLI->>Session: run(config)
Session->>Service: create service instance
Service->>IPC: connect/listen (pipe_name())
loop commands
IPC->>Service: receive command (cwd, exe, argv, id)
Service->>JS: on_command(payload)
JS-->>Service: action (inject/skip/modify)
Service->>IPC: write response/action
end
Session->>Service: finish
Service->>JS: on_finish(ExecutionEvent)
JS-->>CLI: script cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test/catter-proxy.cc (1)
108-108:⚠️ Potential issue | 🟡 MinorUnreachable code.
Line 108
return 0;is unreachable—all execution paths through the try-catch block already return at lines 100, 103, or 106.🧹 Proposed fix
} catch(...) { std::println("Unknown fatal error."); return 1; } - return 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test/catter-proxy.cc` at line 108, The trailing "return 0;" after the try-catch in the function (the extra return at line 108) is unreachable because all branches in the try/catch already return (lines 100, 103, 106); remove the redundant "return 0;" or consolidate returns so there is a single exit point in the function (e.g., have the try/catch set a result variable and return it once after the block) — locate the function (main or the test entry in catter-proxy.cc) and eliminate or refactor the unreachable return to resolve the dead code.
🧹 Nitpick comments (4)
src/catter/core/ipc.h (1)
23-25: Initializem_modeexplicitly to avoid indeterminate state.
m_modeis currently default-initialized as a fundamental enum field, which is not deterministic.Proposed change
private: - ServiceMode m_mode; + ServiceMode m_mode{};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/ipc.h` around lines 23 - 25, m_mode (the enum member in the class) is currently left uninitialized; initialize it explicitly to a sensible default to avoid indeterminate state by either providing an in-class initializer (e.g., "ServiceMode m_mode = <DEFAULT>") or setting it in the class constructor initializer list for the class that declares m_mode; reference the symbol m_mode and whichever constructor of that class (or the ServiceMode enum) to locate where to assign the chosen default value.tests/unit/catter/core/compiler.cc (1)
1-1: Unused include.The
<optional>header is included but not used in this test file.🧹 Proposed fix
-#include <optional> - `#include` <eventide/zest/macro.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/catter/core/compiler.cc` at line 1, Remove the unused header include <optional> from the test file: the line including <optional> should be deleted since no symbols from std::optional are referenced in this test (search for the include in compiler.cc and remove it).api/src/scripts/cdb.ts (1)
6-6: Unused import:Compilertype is imported but not used.The
Compilertype is imported fromcatter-cbut the code only uses the return value ofidentify_compilerdirectly. Consider removing if unused.♻️ Proposed fix
-import { identify_compiler, Compiler } from "catter-c"; +import { identify_compiler } from "catter-c";Or if the type annotation on
commandArrayis intended to use it:commandArray: Array<[Compiler, service.CommandData]> = [];Then you need to import the type. Verify that the
Compilerimport is correctly used for the type annotation on line 18.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/scripts/cdb.ts` at line 6, The import currently brings in both identify_compiler and Compiler from "catter-c" but Compiler is unused; either remove Compiler from the import list or make it a type-only import and use it in the commandArray annotation. Locate the import line with identify_compiler and Compiler and either delete Compiler from that import or change to `import type { Compiler }` and update commandArray to use Array<[Compiler, service.CommandData]>. Ensure identify_compiler remains imported and used as-is.src/catter/core/compiler.cc (1)
61-65: Minor inefficiency:std::stringallocation in loop.The
std::string(compiler_name)conversion happens on every iteration. Consider moving it outside the loop to avoid repeated allocations.♻️ Proposed optimization
+ std::string compiler_str(compiler_name); for(auto& compiler_pattern: DEFAULT_PATTERNS) { - if(std::regex_match(std::string(compiler_name), compiler_pattern.pattern)) { + if(std::regex_match(compiler_str, compiler_pattern.pattern)) { return compiler_pattern.type; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/compiler.cc` around lines 61 - 65, The loop repeatedly constructs std::string(compiler_name) causing unnecessary allocations; before iterating DEFAULT_PATTERNS create a single std::string (e.g. compilerStr = std::string(compiler_name)) and use that in the std::regex_match call (compare compilerStr against compiler_pattern.pattern) so the conversion happens once; update references to compiler_name in the loop to use compilerStr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/scripts/cdb.ts`:
- Around line 48-65: The stored commands loop in onFinish iterates
this.commandArray but only handles "clang", causing errors when onCommand
previously saved gcc/flang/ifort entries; update onCommand to only push entries
for compiler === "clang" (so this.commandArray contains only clang rows) or
alternatively extend the switch in onFinish to handle other compilers (e.g., add
cases for "gcc"/"flang"/"ifort" with appropriate parsing) — locate and update
the onCommand logic that populates this.commandArray (and/or the switch in
onFinish that references compiler) and ensure use of option.ClangID.ID_INPUT and
parse(...) remains consistent for clang entries.
- Around line 49-61: The const parsed declared inside the switch's case "clang":
can leak into other cases; wrap the case body in its own block (e.g., case
"clang": { ... break; }) so parsed (and any other block-scoped locals) are
scoped to that case. Update the switch handling in the function that calls
this.parse(compiler, command.argv.slice(1)) and uses option.ClangID.ID_INPUT,
command, and cdb to ensure parsed is declared inside a new { } block immediately
after case "clang": and before the break.
In `@src/catter/main.cc`:
- Around line 5-8: Remove the duplicate include of the <print> header in
src/catter/main.cc so only one `#include` <print> remains; inspect the include
block containing `#include` <print>, `#include` <cassert>, and `#include` <format> and
delete the redundant `#include` <print> line so the file only includes each header
once.
- Around line 179-183: Session::run returns an int64_t (the spawn task result)
but the current code discards it; capture the return value from
session.run(config.build_system_command, ServiceImpl::Factory{}) into a variable
(e.g., int64_t exit_code) and then use it to determine process outcome before
calling js::on_finish() — for example propagate it to the caller, convert to an
appropriate exit status, log/report failures, or pass it into js::on_finish() as
needed so the build system command exit code is not lost.
In `@src/common/config/ipc.h`:
- Around line 10-11: The static initialization of path in pipe_name() calls
util::get_catter_data_path() which can throw when HOME is unset; change
pipe_name() to avoid throwing at static init by computing the path at call time
with a non-throwing strategy: check getenv("HOME") (or catch exceptions from
util::get_catter_data_path()), and if unavailable fall back to a safe
non-throwing location such as /tmp or return an empty/optional string and
propagate an actionable error to caller; update callers that use pipe_name()
(IPC setup/cleanup paths) to handle the fallback/empty result explicitly instead
of relying on a thrown exception. Ensure references to pipe_name(),
util::get_catter_data_path(), and the static path variable are replaced so no
throw occurs during static init.
In `@src/common/opt/main/option.h`:
- Around line 2-5: The header is not self-sufficient: option.h declares
std::string fields but doesn't include <string>, so add `#include` <string> to the
top of option.h (alongside the existing includes like <vector>,
<eventide/deco/decl.h>, <eventide/deco/macro.h>) so declarations that use
std::string (the string fields in the Option-related declarations) compile
without relying on transitive includes.
---
Outside diff comments:
In `@tests/integration/test/catter-proxy.cc`:
- Line 108: The trailing "return 0;" after the try-catch in the function (the
extra return at line 108) is unreachable because all branches in the try/catch
already return (lines 100, 103, 106); remove the redundant "return 0;" or
consolidate returns so there is a single exit point in the function (e.g., have
the try/catch set a result variable and return it once after the block) — locate
the function (main or the test entry in catter-proxy.cc) and eliminate or
refactor the unreachable return to resolve the dead code.
---
Nitpick comments:
In `@api/src/scripts/cdb.ts`:
- Line 6: The import currently brings in both identify_compiler and Compiler
from "catter-c" but Compiler is unused; either remove Compiler from the import
list or make it a type-only import and use it in the commandArray annotation.
Locate the import line with identify_compiler and Compiler and either delete
Compiler from that import or change to `import type { Compiler }` and update
commandArray to use Array<[Compiler, service.CommandData]>. Ensure
identify_compiler remains imported and used as-is.
In `@src/catter/core/compiler.cc`:
- Around line 61-65: The loop repeatedly constructs std::string(compiler_name)
causing unnecessary allocations; before iterating DEFAULT_PATTERNS create a
single std::string (e.g. compilerStr = std::string(compiler_name)) and use that
in the std::regex_match call (compare compilerStr against
compiler_pattern.pattern) so the conversion happens once; update references to
compiler_name in the loop to use compilerStr.
In `@src/catter/core/ipc.h`:
- Around line 23-25: m_mode (the enum member in the class) is currently left
uninitialized; initialize it explicitly to a sensible default to avoid
indeterminate state by either providing an in-class initializer (e.g.,
"ServiceMode m_mode = <DEFAULT>") or setting it in the class constructor
initializer list for the class that declares m_mode; reference the symbol m_mode
and whichever constructor of that class (or the ServiceMode enum) to locate
where to assign the chosen default value.
In `@tests/unit/catter/core/compiler.cc`:
- Line 1: Remove the unused header include <optional> from the test file: the
line including <optional> should be deleted since no symbols from std::optional
are referenced in this test (search for the include in compiler.cc and remove
it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fdc741a-18c7-4f7e-a514-3f92d599d115
📒 Files selected for processing (26)
api/catter-c/capi.d.tsapi/src/index.tsapi/src/scripts/cdb.tsapi/src/scripts/index.tsapi/src/service.tsapi/src/util/cmd.tsscripts/package.jsonscripts/tsconfig.jsonsrc/catter-proxy/ipc.ccsrc/catter/core/capi/compiler.ccsrc/catter/core/compiler.ccsrc/catter/core/compiler.hsrc/catter/core/ipc.hsrc/catter/core/session.ccsrc/catter/core/session.hsrc/catter/main.ccsrc/common/config/catter-main.hsrc/common/config/catter.hsrc/common/config/ipc.hsrc/common/opt/main/option.hsrc/common/util/packet_io.htests/integration/test/catter-proxy.cctests/unit/catter-hook/unix/payload/command.cctests/unit/catter-hook/unix/payload/executor.cctests/unit/catter/core/compiler.ccxmake.lua
💤 Files with no reviewable changes (3)
- src/common/config/catter-main.h
- scripts/package.json
- scripts/tsconfig.json
Comment on lines
+48
to
+65
| for (const [compiler, command] of this.commandArray) { | ||
| switch (compiler) { | ||
| case "clang": | ||
| const parsed = this.parse(compiler, command.argv.slice(1)); | ||
| for (const item of parsed) { | ||
| if (item.id === option.ClangID.ID_INPUT) { | ||
| cdb.push({ | ||
| directory: command.cwd, | ||
| file: item.key, | ||
| command: command.argv.join(" "), | ||
| }); | ||
| } | ||
| } | ||
| break; | ||
| default: | ||
| throw new Error(`Unsupported compiler: ${compiler}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Logic mismatch: onCommand accepts all compilers, but onFinish only handles "clang".
onCommand stores commands for any non-"unknown" compiler (gcc, flang, ifort, etc.), but onFinish throws an error for anything other than "clang". This will cause runtime failures if any gcc/flang/ifort commands are captured.
Either filter to only "clang" in onCommand, or extend onFinish to handle other compilers.
🔧 Option 1: Filter in onCommand (simpler for MVP)
onCommand(
id: number,
data: service.CommandData | service.CatterErr,
): service.Action {
if ("msg" in data) {
io.println(`CDB received error: ${data.msg}`);
} else {
const compiler = identify_compiler(data.exe);
- if (compiler !== "unknown") {
+ if (compiler === "clang") {
this.commandArray.push([compiler, data]);
}
}🧰 Tools
🪛 Biome (2.4.7)
[error] 51-51: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/scripts/cdb.ts` around lines 48 - 65, The stored commands loop in
onFinish iterates this.commandArray but only handles "clang", causing errors
when onCommand previously saved gcc/flang/ifort entries; update onCommand to
only push entries for compiler === "clang" (so this.commandArray contains only
clang rows) or alternatively extend the switch in onFinish to handle other
compilers (e.g., add cases for "gcc"/"flang"/"ifort" with appropriate parsing) —
locate and update the onCommand logic that populates this.commandArray (and/or
the switch in onFinish that references compiler) and ensure use of
option.ClangID.ID_INPUT and parse(...) remains consistent for clang entries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
api/src/scripts/cdb.ts (2)
56-68:⚠️ Potential issue | 🟡 MinorWrap the
case "clang"body in braces.Line 58 declares
const parseddirectly under a switch case. Block it to avoid cross-case scope leakage.🔧 Scoped switch-case declaration
switch (compiler) { - case "clang": + case "clang": { const parsed = this.parse(compiler, command.argv.slice(1)); for (const item of parsed) { if (item.id === option.ClangID.ID_INPUT) { cdb.push({ directory: command.cwd, file: item.key, command: command.argv.join(" "), }); } } break; + } default: throw new Error(`Unsupported compiler: ${compiler}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/scripts/cdb.ts` around lines 56 - 68, The switch case for "clang" declares a block-scoped const (parsed) without braces which can leak scope; wrap the entire case "clang" body in braces so that const parsed = this.parse(compiler, command.argv.slice(1)); and the for loop that checks item.id === option.ClangID.ID_INPUT and pushes into cdb (using command.cwd, item.key, command.argv.join(" ")) are all inside a local block, preserving correct scoping for parsed and any other block-scoped variables.
89-92:⚠️ Potential issue | 🟠 MajorPrevent non-clang commands from crashing
onFinish.Line 90 stores all known compilers, but Line 70 throws for anything except
"clang". This makes mixed-toolchain builds fail during CDB generation.🔧 MVP-safe fix (filter at ingestion)
- if (compiler !== "unknown") { + if (compiler === "clang") { this.commandArray.push([compiler, data]); }Also applies to: 55-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/scripts/cdb.ts` around lines 89 - 92, The code is crashing later in onFinish because non-clang toolchain entries are being processed or cause a throw; to fix, filter out non-clang commands at ingestion: in the path that calls identify_compiler(data.exe) and pushes into this.commandArray, only push when identify_compiler(...) === "clang" (or normalize identify_compiler to return "clang"/"unknown" without throwing), and ensure onFinish and related handlers (identify_compiler, this.commandArray processing) expect only clang entries so they no longer throw on mixed-toolchain builds; update identify_compiler and the ingestion site to skip/log non-clang entries instead of allowing them through to onFinish.
🧹 Nitpick comments (1)
api/src/scripts/cdb.ts (1)
8-14: Avoid rebuilding shell command withargv.join(" ").Line 64 can produce ambiguous/invalid commands when args contain spaces or quotes. Prefer serializing
argumentsdirectly in the CDB entry.🔧 Prefer `arguments` over joined command text
type CDBItem = { directory: string; file: string; - command: string; - arguments?: string[]; + arguments: string[]; + command?: string; output?: string; }; @@ cdb.push({ directory: command.cwd, file: item.key, - command: command.argv.join(" "), + arguments: command.argv, });Also applies to: 61-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/scripts/cdb.ts` around lines 8 - 14, The CDB entry currently rebuilds the shell command with argv.join(" "), which breaks when args contain spaces or quotes; instead populate the CDBItem.arguments array with the original argv array (do not create a single joined command string) and update any code that serializes or replays the CDB entries to use CDBItem.arguments as an args array (or pass it directly to spawn/execFile) rather than parsing a reconstructed command string; adjust places referencing CDBItem.command/arguments to ensure command stays the executable and arguments remains string[] so no manual join/escape is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/scripts/cdb.ts`:
- Line 74: The TextFileStream currently only supports "ascii" and using
io.TextFileStream.with(this.save_path, "ascii", ...) corrupts non-ASCII
paths/commands; add UTF-8 support to the TextFileStream implementation (expand
the encoding handling in the TextFileStream class to accept "utf8"/"utf-8" and
write using Buffer.from(string, "utf8") or equivalent write method) and then
change the call site to io.TextFileStream.with(this.save_path, "utf8", (stream)
=> {...}) so compile_commands.json is written with UTF-8 encoding; update any
encoding switch/validation logic and tests to include "utf8"/"utf-8" variants
and ensure stream.close/flush still behaves correctly.
In `@src/catter/main.cc`:
- Around line 166-180: js::on_start(...) returns an updated config that must be
used but is currently discarded; capture its return (e.g., auto effective =
js::on_start({...});) and then use fields from that returned config
(effective.buildSystemCommand, effective.runtime, effective.options.log,
effective.isScriptSupported, etc.) when constructing and invoking Session and
session.run instead of the original config so changes such as rewritten
buildSystemCommand and isScriptSupported are honored.
- Around line 52-56: The switch currently treats js::ActionType::drop the same
as js::ActionType::skip by returning data::action{.type = data::action::INJECT,
.cmd = cmd}; change the js::ActionType::drop branch to return the non-executing
IPC action instead (e.g., return data::action{.type = data::action::NONE} or the
project’s equivalent no-op/ DROP action) so that a drop request does not execute
the original cmd; update only the case for js::ActionType::drop in the switch
handling act.type.
- Around line 42-50: The CommandData passed to js::on_command is missing the
required .runtime field; populate CommandData.runtime (e.g., .runtime =
cmd.runtime or construct a Runtime object with the correct
type/supportActions/supportEvents) before calling js::on_command so scripts see
the correct capability set; update the aggregate that builds js::CommandData
(the code setting .cwd/.exe/.argv/.env/.parent where act is created) to include
.runtime filled from the incoming cmd or computed runtime info.
---
Duplicate comments:
In `@api/src/scripts/cdb.ts`:
- Around line 56-68: The switch case for "clang" declares a block-scoped const
(parsed) without braces which can leak scope; wrap the entire case "clang" body
in braces so that const parsed = this.parse(compiler, command.argv.slice(1));
and the for loop that checks item.id === option.ClangID.ID_INPUT and pushes into
cdb (using command.cwd, item.key, command.argv.join(" ")) are all inside a local
block, preserving correct scoping for parsed and any other block-scoped
variables.
- Around line 89-92: The code is crashing later in onFinish because non-clang
toolchain entries are being processed or cause a throw; to fix, filter out
non-clang commands at ingestion: in the path that calls
identify_compiler(data.exe) and pushes into this.commandArray, only push when
identify_compiler(...) === "clang" (or normalize identify_compiler to return
"clang"/"unknown" without throwing), and ensure onFinish and related handlers
(identify_compiler, this.commandArray processing) expect only clang entries so
they no longer throw on mixed-toolchain builds; update identify_compiler and the
ingestion site to skip/log non-clang entries instead of allowing them through to
onFinish.
---
Nitpick comments:
In `@api/src/scripts/cdb.ts`:
- Around line 8-14: The CDB entry currently rebuilds the shell command with
argv.join(" "), which breaks when args contain spaces or quotes; instead
populate the CDBItem.arguments array with the original argv array (do not create
a single joined command string) and update any code that serializes or replays
the CDB entries to use CDBItem.arguments as an args array (or pass it directly
to spawn/execFile) rather than parsing a reconstructed command string; adjust
places referencing CDBItem.command/arguments to ensure command stays the
executable and arguments remains string[] so no manual join/escape is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85cdfbca-5497-4146-96e2-88f0f8a27943
📒 Files selected for processing (9)
api/catter-c/capi.d.tsapi/src/scripts/cdb.tsapi/src/service.tsapi/test/service.tssrc/catter/core/js.ccsrc/catter/core/js.hsrc/catter/main.ccsrc/common/opt/main/option.htests/unit/catter/core/js.cc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/scripts/cdb.ts`:
- Around line 62-66: The cdb entry currently builds a single command string by
joining command.argv which corrupts args containing spaces; update the cdb.push
call (the object created where you currently set command: command.argv.join("
")) to instead set an arguments field with the raw array (arguments:
command.argv) per the Compilation Database spec (you can drop or keep command as
an optional convenience but ensure arguments is present and populated from
command.argv to preserve quoting/spaces).
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 by CodeRabbit
New Features
Refactor
Tests
Chores