Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR adds a .gitignore pattern, simplifies and reworks CMake to build a cxtest test binary, removes a demo executable, introduces a comprehensive test harness (Evaluate.cpp), expands CatalystCX.hpp with richer cross‑platform process/signal/resource handling, and adds a multi‑job GitHub Actions CI workflow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as TestRunner
participant Cmd as Command
participant OS as OS
participant SI as SignalInfo
rect rgba(224,241,255,0.5)
Tester->>Cmd: Spawn(args, env, cwd, pipes)
alt Apple
Cmd->>OS: posix\_spawn(file, argv, envp, file_actions)
else Windows
Cmd->>OS: CreateProcess + pipe handles
else POSIX
Cmd->>OS: fork() -> execve() with redirected fds
end
OS-->>Cmd: pid or error
end
par Read IO
Cmd->>Cmd: Read stdout/stderr (async/non‑blocking)
and Wait
Tester->>Cmd: Wait(timeout)
Cmd->>OS: waitpid / WaitForSingleObject
OS-->>Cmd: exit code / signal / stopped
end
rect rgba(232,255,232,0.6)
Cmd->>Cmd: Populate CommandResult (ExitCode, KilledBySignal, TerminatingSignal, CoreDumped, Stopped, Usage*)
Cmd->>SI: GetSignalName(TerminatingSignal)
SI-->>Cmd: signal name
Cmd-->>Tester: CommandResult
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CatalystCX.hpp (2)
154-186: Potential deadlock + indefinite hang on timeout.
- Deadlock risk: parent waits before reading pipes; child can block when pipe buffers fill.
- Timeout path sends SIGTERM then blocks on
wait4forever if the child ignores it.Apply both fixes:
- Read stdout/stderr concurrently while waiting.
- After timeout, escalate from SIGTERM to SIGKILL with a grace period.
inline CommandResult Child::Wait(std::optional<std::chrono::duration<double>> timeout) const { auto start_time = std::chrono::steady_clock::now(); CommandResult result; int status = 0; rusage usage{}; + // Start reading pipes concurrently to avoid deadlock + auto reader_future = std::async(std::launch::async, AsyncPipeReader::ReadPipes, StdoutFd, StderrFd); + if (timeout) { while (true) { const int wait_result = waitpid(ProcessId, &status, WNOHANG); if (wait_result == ProcessId) { break; // Process finished } if (wait_result == -1) { result.ExitCode = 127; result.Stderr = "waitpid failed"; break; } if (auto current_time = std::chrono::steady_clock::now(); current_time - start_time > *timeout) { - Kill(); - result.TimedOut = true; - wait4(ProcessId, &status, 0, &usage); // Clean up the zombie process and get usage - break; + // Graceful terminate, then escalate + Kill(SIGTERM); + result.TimedOut = true; + const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(750); + while (true) { + if (int w = wait4(ProcessId, &status, WNOHANG, &usage); w == ProcessId) { + break; + } + if (std::chrono::steady_clock::now() >= deadline) { + kill(ProcessId, SIGKILL); + (void)wait4(ProcessId, &status, 0, &usage); + break; + } + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + break; } std::this_thread::sleep_for(std::chrono::milliseconds(10)); } } else { wait4(ProcessId, &status, 0, &usage); } auto end_time = std::chrono::steady_clock::now(); result.ExecutionTime = end_time - start_time; - auto [stdout_result, stderr_result] = AsyncPipeReader::ReadPipes(StdoutFd, StderrFd); + // Join the reader and collect output + auto [stdout_result, stderr_result] = reader_future.get(); result.Stdout = std::move(stdout_result); result.Stderr += stderr_result; close(StdoutFd); close(StderrFd);Also applies to: 190-196
358-360: Don’t clobber file status flags when enabling nonblocking.Use get‑then‑or; current code overwrites flags.
- fcntl(stdout_fd, F_SETFL, O_NONBLOCK); - fcntl(stderr_fd, F_SETFL, O_NONBLOCK); + int out_flags = fcntl(stdout_fd, F_GETFL); + if (out_flags != -1) fcntl(stdout_fd, F_SETFL, out_flags | O_NONBLOCK); + int err_flags = fcntl(stderr_fd, F_GETFL); + if (err_flags != -1) fcntl(stderr_fd, F_SETFL, err_flags | O_NONBLOCK);
🧹 Nitpick comments (11)
CMakeLists.txt (2)
7-8: Prefer target-based flags; avoid global CMAKE_CXX_FLAGS.Don’t force
-O2/warnings globally; set per-target and add-Wpedantic.-# Compiler flags -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -O2") +# Compiler flags (per-target, compiler-aware) +target_compile_options(cxtest PRIVATE + $<$<CXX_COMPILER_ID:GNU,Clang>:-Wall -Wextra -Wpedantic> +)If you want opt flags, prefer CMake build types (Release/RelWithDebInfo) instead of hardcoding
-O2.Also applies to: 10-11
13-18: Integrate with CTest instead of a customtesttarget.This lets CI run
ctestand properly fail on non‑zero exits.-# Custom target to run tests -add_custom_target(test - COMMAND cxtest - DEPENDS cxtest - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} -) +include(CTest) +enable_testing() +add_test(NAME cxtest COMMAND cxtest)Also applies to: 20-21
Evaluate.cpp (4)
65-68: Relax time assertion to reduce flakiness.Wall‑clock jitter can cause
< 1.0soccasionally.- runner.Assert(result.ExecutionTime.count() >= 1.0, "Sleep duration correct"); + runner.Assert(result.ExecutionTime.count() >= 0.9, "Sleep duration correct");
79-81: Add a small cushion on timeout enforcement.Occasional scheduler delays can breach 2s.
- runner.Assert(duration < std::chrono::seconds(2), "Timeout enforced quickly"); + runner.Assert(duration <= std::chrono::seconds(3), "Timeout enforced quickly");
152-156: Guard external tool availability to avoid platform variance.
seq/shtypically exist, but on minimal images they may not. UseExecutionValidatorto skip when absent.- auto result = Command("seq").Args({"1", "1000"}).Status(); + if (!ExecutionValidator::IsCommandExecutable("seq")) { + std::cout << "[SKIP] seq not available\n"; + return; + } + auto result = Command("seq").Args({"1", "1000"}).Status();Similarly for
shif desired.Also applies to: 157-161
202-205: Avoid ultra‑short 1ms timeout; increase to reduce flakiness.1ms can race on loaded CI.
- result = Command("sleep").Arg("1").Timeout(std::chrono::milliseconds(1)).Status(); + result = Command("sleep").Arg("1").Timeout(std::chrono::milliseconds(10)).Status();CatalystCX.hpp (5)
398-405: HandleEAGAIN/EWOULDBLOCK/EINTRexplicitly in nonblocking reads.Returning
bytes_read != 0treats all errors as “keep reading”; be explicit.inline bool AsyncPipeReader::ReadFromPipe(PipeData& pipe_data, std::array<char, 8192>& buffer) { const ssize_t bytes_read = read(pipe_data.Fd, buffer.data(), buffer.size()); if (bytes_read > 0) { pipe_data.Buffer.append(buffer.data(), bytes_read); return true; } - return bytes_read != 0; + if (bytes_read == 0) return false; // EOF + // bytes_read < 0 + if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) return true; // try again + return false; // treat other errors as terminal }Add missing header:
#include <string> +#include <cerrno>
411-414: Executable check should respect real user permissions and regular files.
stat+S_IXUSRignores group/other bits and ownership; useaccess(X_OK)and verify regular file.inline bool ExecutionValidator::IsFileExecutable(const std::string& path) { - struct stat st{}; - return stat(path.c_str(), &st) == 0 && st.st_mode & S_IXUSR; + struct stat st{}; + return stat(path.c_str(), &st) == 0 && S_ISREG(st.st_mode) && access(path.c_str(), X_OK) == 0; }
428-439: Treat empty PATH segments as current directory.This matches POSIX semantics for
:in PATH.- std::string dir = path_str.substr(start, end - start); + std::string dir = path_str.substr(start, end - start); + if (dir.empty()) dir = ".";
63-81: Consider RAII close for FDs whenWait()isn’t called.If a
Childis dropped withoutWait(), FDs leak.You could add a destructor that closes
StdoutFd/StderrFdif still open, guarded to avoid double‑close. Alternatively, document thatWait()must be called.
448-472: Optional: Expand signal names for completeness.Consider adding common signals like
SIGHUP,SIGTRAP, etc., or mapping viastrsignal()where available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)CMakeLists.txt(1 hunks)CatalystCX.cpp(0 hunks)CatalystCX.hpp(6 hunks)Evaluate.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- CatalystCX.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
Evaluate.cpp (1)
CatalystCX.hpp (10)
result(474-489)result(474-474)Command(85-85)Command(85-85)duration(112-115)duration(112-112)IsCommandExecutable(416-442)IsCommandExecutable(416-416)CanExecuteCommand(444-446)CanExecuteCommand(444-444)
🔇 Additional comments (1)
.gitignore (1)
2-2: Ignore rule LGTM.
.amazonqignore is fine and harmless.
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)
CatalystCX.hpp (1)
366-391: Use wait4 with WNOHANG in the timed loop to capture rusage when the process exits.Currently, when the child exits before timeout, usage stays zero-initialized. Prefer wait4 in the loop to fill rusage at exit.
Apply this diff:
- while (true) { - const int wait_result = waitpid(ProcessId, &status, WNOHANG); + while (true) { + const int wait_result = wait4(ProcessId, &status, WNOHANG, &usage); if (wait_result == ProcessId) { break; // Process finished } if (wait_result == -1) { result.ExitCode = 127; result.Stderr = "waitpid failed"; break; }
♻️ Duplicate comments (1)
CatalystCX.hpp (1)
498-500: posix_spawnp for PATH search — good fix.Switching to posix_spawnp addresses PATH lookup on macOS. Matches prior review guidance.
🧹 Nitpick comments (8)
CatalystCX.hpp (5)
27-38: Missing headers for POSIX: close() and errno.close() and errno usage require unistd.h and . Add them in the non-Windows branch.
Apply this diff:
#else #include <csignal> #include <fcntl.h> #include <poll.h> +#include <unistd.h> +#include <cerrno>
395-398: Avoid unnecessary append; move-assign stderr.Symmetry with Stdout; one less copy.
Apply this diff:
- result.Stdout = std::move(stdout_result); - result.Stderr += stderr_result; + result.Stdout = std::move(stdout_result); + result.Stderr = std::move(stderr_result);
592-599: Handle EAGAIN/EINTR correctly in ReadFromPipe.Returning true on any error can cause spin; explicitly treat EAGAIN/EINTR as “try again” and EOF as finished.
Apply this diff:
inline bool AsyncPipeReader::ReadFromPipe(PipeData& pipe_data, std::array<char, 8192>& buffer) { const ssize_t bytes_read = read(pipe_data.Fd, buffer.data(), buffer.size()); - if (bytes_read > 0) { + if (bytes_read > 0) { pipe_data.Buffer.append(buffer.data(), bytes_read); return true; } - return bytes_read != 0; + if (bytes_read == -1 && (errno == EAGAIN || errno == EINTR)) { + return true; // keep polling + } + return false; // EOF or fatal error }
329-355: Windows PATH resolution should honor PATHEXT and quote-aware joins.Today only bare name or .exe are checked. Respect PATHEXT to allow .com/.bat/.cmd and others.
Apply this diff:
inline bool ExecutionValidator::IsCommandExecutable(const std::string& command) { if (command.find('\\') != std::string::npos || command.find('/') != std::string::npos) { return IsFileExecutable(command) || IsFileExecutable(command + ".exe"); } const char* path_env = getenv("PATH"); if (!path_env) return false; + const char* pathext_env = getenv("PATHEXT"); + std::vector<std::string> exts; + if (pathext_env) { + std::stringstream ss(pathext_env); + std::string ext; + while (std::getline(ss, ext, ';')) exts.push_back(ext); + } else { + exts = {".EXE", ".COM", ".BAT", ".CMD"}; + } std::string path_str(path_env); size_t start = 0; size_t end = path_str.find(';'); while (start < path_str.length()) { std::string dir = path_str.substr(start, end - start); - std::string full_path = dir + "\\" + command; - - if (IsFileExecutable(full_path) || IsFileExecutable(full_path + ".exe")) { - return true; - } + std::string base = dir + "\\" + command; + if (IsFileExecutable(base)) return true; + for (const auto& ext : exts) { + if (IsFileExecutable(base + ext)) return true; + } if (end == std::string::npos) break; start = end + 1; end = path_str.find(';', start); } return false; }
633-637: POSIX executable check: use access(X_OK) and ensure regular file.The current check only tests owner execute bit. Prefer access() and S_ISREG.
Apply this diff:
inline bool ExecutionValidator::IsFileExecutable(const std::string& path) { - struct stat st{}; - return stat(path.c_str(), &st) == 0 && st.st_mode & S_IXUSR; + struct stat st{}; + if (stat(path.c_str(), &st) != 0) return false; + if (!S_ISREG(st.st_mode)) return false; + return access(path.c_str(), X_OK) == 0; }.github/workflows/ci.yml (3)
76-80: cppcheck CTU false ODRs: define platform for analysis.Help cppcheck pick the intended branch to avoid cross-TU ODR noise.
Apply this diff:
- - name: Run cppcheck - run: | - cppcheck --enable=all --std=c++20 --suppress=missingIncludeSystem \ - --error-exitcode=1 CatalystCX.hpp + - name: Run cppcheck + run: | + cppcheck --enable=all --std=c++20 --platform=unix64 -D__linux__ \ + --suppress=missingIncludeSystem --error-exitcode=1 CatalystCX.hpp
62-64: Prefer ctest to run tests.ctest reports failures and integrates with CMake config types.
Apply this diff:
- - name: Run Tests - run: cmake --build build --target test --config Release + - name: Run Tests + run: ctest --test-dir build -C Release --output-on-failure
155-155: Add newline at end of file.Fixes YAML lint warning.
Apply this diff:
- ./build/cxtest 2>/dev/null || true \ No newline at end of file + ./build/cxtest 2>/dev/null || true +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(1 hunks)CMakeLists.txt(1 hunks)CatalystCX.hpp(7 hunks)Evaluate.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- Evaluate.cpp
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 155-155: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Actions: CI/CD Pipeline
CatalystCX.hpp
[warning] 534-534: cppcheck: style: Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm]
[warning] 80-80: cppcheck: performance: Variable 'ProcessId' is assigned in constructor body. Consider performing initialization list. [useInitializationList]
[warning] 479-479: cppcheck: style: The scope of the variable 'env_strings' can be reduced. [variableScope]
[warning] 475-475: cppcheck: style: Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm]
[warning] 492-492: cppcheck: style: Consider using std::transform algorithm instead of a raw loop. [useStlAlgorithm]
[error] 168-168: cppcheck: The one definition rule is violated, different classes/structs have the same name 'AsyncPipeReader::PipeData' [ctuOneDefinitionRuleViolation]
[error] 66-66: cppcheck: The one definition rule is violated, different classes/structs have the same name 'CommandResult::ResourceUsage' [ctuOneDefinitionRuleViolation]
[warning] 93-93: cppcheck: The function 'GetPid' is never used. [unusedFunction]
[warning] 118-118: cppcheck: The function 'Arg' is never used. [unusedFunction]
[warning] 123-123: cppcheck: The function 'Args' is never used. [unusedFunction]
[warning] 128-128: cppcheck: The function 'WorkingDirectory' is never used. [unusedFunction]
[warning] 133-133: cppcheck: The function 'Environment' is never used. [unusedFunction]
[warning] 138-138: cppcheck: The function 'Timeout' is never used. [unusedFunction]
[warning] 640-640: cppcheck: The function 'Status' is never used. [unusedFunction]
[warning] 683-683: cppcheck: The function 'GetProcessInfo' is never used. [unusedFunction]
[error] 1-1: cppcheck failed with exit code 1.
1 participant
Summary by CodeRabbit
New Features
Tests
Refactor
Chores
CI