Expand MCP protocol coverage - #225
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughRemoved CI/headless skip logic from Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Harness
participant PipeIn as OS Pipe (stdin)
participant MCP as MCPServer
participant PipeOut as OS Pipe (stdout)
participant Parser as Test JSON Parser
Test->>PipeIn: write framed message / malformed header
PipeIn->>MCP: data available (onReadyRead)
MCP->>MCP: parse framing, recover from bad header
MCP->>PipeOut: write framed JSON-RPC response
PipeOut->>Test: response bytes
Test->>Parser: extract Content-Length body and parse JSON
Parser-->>Test: structured result/assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MCPServer_test.cpp`:
- Around line 3712-3714: The test currently hard-fails on OGRE init via
ASSERT_TRUE(tryInitOgre()); change it to skip when OGRE cannot initialize: call
tryInitOgre() and if it returns false, invoke the test skip mechanism (e.g.
GTEST_SKIP() with a short message) and return early so the rest of the test
(including createStandardOgreMaterials()) is not run; update the block around
tryInitOgre() / createStandardOgreMaterials() accordingly.
- Around line 14-18: Wrap POSIX-only code in MCPServer_test.cpp with a Windows
guard: surround the `#include` <unistd.h> and every POSIX call (pipe(), read(),
write(), close()) used in the SetUp() and TearDown() methods and the OnReadyRead
test with `#ifndef` Q_OS_WIN ... `#endif` so Windows builds skip them; in places
where behavior is required on Windows, replace or provide an alternative
implementation (e.g., use a named pipe or socket mock) inside the same guarded
blocks. Specifically update the include near the top, the pipe()/close() usage
in SetUp() and TearDown(), and the pipe()/write()/close() sequence in the
OnReadyRead test to be compiled only when Q_OS_WIN is not defined, and add
Windows-safe fallbacks or test skips for those tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93ca0780-390c-48b8-a2d5-5c9b3c25e1bf
📒 Files selected for processing (2)
src/EditorViewport_test.cppsrc/MCPServer_test.cpp
| #include <unistd.h> | ||
|
|
||
| #define private public | ||
| #include "MCPServer.h" | ||
| #undef private |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "POSIX transport APIs referenced from src/MCPServer_test.cpp:"
rg -n -C2 '<unistd\.h>|\b(pipe|read|write|close)\s*\(' src/MCPServer_test.cpp
echo
echo "Windows guards in src/MCPServer_test.cpp:"
rg -n -C1 'Q_OS_WIN' src/MCPServer_test.cpp || trueRepository: fernandotonon/QtMeshEditor
Length of output: 1578
Guard POSIX APIs with #ifndef Q_OS_WIN for Windows compatibility.
The file uses <unistd.h> and POSIX functions (pipe, read, write, close) without platform guards. These APIs are unavailable on Windows.
Wrap the include and all POSIX usage in:
- Line 14:
#include <unistd.h> - Lines 107–118:
pipe()andclose()inSetUp()andTearDown() - Lines 3749–3772:
pipe(),write(), andclose()in theOnReadyReadtest
Either guard with #ifndef Q_OS_WIN or provide Windows-safe alternatives (e.g., named pipes or sockets).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 14 - 18, Wrap POSIX-only code in
MCPServer_test.cpp with a Windows guard: surround the `#include` <unistd.h> and
every POSIX call (pipe(), read(), write(), close()) used in the SetUp() and
TearDown() methods and the OnReadyRead test with `#ifndef` Q_OS_WIN ... `#endif` so
Windows builds skip them; in places where behavior is required on Windows,
replace or provide an alternative implementation (e.g., use a named pipe or
socket mock) inside the same guarded blocks. Specifically update the include
near the top, the pipe()/close() usage in SetUp() and TearDown(), and the
pipe()/write()/close() sequence in the OnReadyRead test to be compiled only when
Q_OS_WIN is not defined, and add Windows-safe fallbacks or test skips for those
tests.
| ASSERT_TRUE(tryInitOgre()); | ||
| createStandardOgreMaterials(); | ||
|
|
There was a problem hiding this comment.
Skip this OGRE-backed test when headless init is unavailable.
Every other OGRE-dependent test in this file skips on tryInitOgre() failure. Hard-failing here makes the protocol suite brittle on runners where the display stack is unavailable even though the protocol path itself is fine.
💡 Suggested change
- ASSERT_TRUE(tryInitOgre());
- createStandardOgreMaterials();
+ if (!tryInitOgre()) {
+ GTEST_SKIP() << "Skipping: Ogre initialization failed";
+ }
+ createStandardOgreMaterials();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 3712 - 3714, The test currently
hard-fails on OGRE init via ASSERT_TRUE(tryInitOgre()); change it to skip when
OGRE cannot initialize: call tryInitOgre() and if it returns false, invoke the
test skip mechanism (e.g. GTEST_SKIP() with a short message) and return early so
the rest of the test (including createStandardOgreMaterials()) is not run;
update the block around tryInitOgre() / createStandardOgreMaterials()
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/MCPServer_test.cpp (2)
3712-3715:⚠️ Potential issue | 🟠 MajorSkip instead of hard-failing when Ogre init is unavailable in headless CI.
This test still uses
ASSERT_TRUE(tryInitOgre()); under headless runners it should skip like the rest of the suite instead of failing the job.Based on learnings, "Tests must work under Xvfb (headless X11) — avoid assumptions about a real display".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 3712 - 3715, In TEST_F(MCPServerProtocolTest, HandleResourcesReadSceneInfoReturnsSerializedText) replace the hard assertion on tryInitOgre() with a conditional skip: call tryInitOgre() and if it returns false invoke GTEST_SKIP() (or equivalent test-skip macro) so the test is skipped in headless CI; update the block surrounding tryInitOgre() / createStandardOgreMaterials() to use the skip path rather than ASSERT_TRUE to avoid failing the job when Ogre cannot initialize.
14-15:⚠️ Potential issue | 🔴 CriticalGuard POSIX-only APIs for Windows builds.
<unistd.h>andpipe/read/write/closeare used without#ifndef Q_OS_WIN, which breaks Windows compilation for this test file.As per coding guidelines, "Guard platform-specific APIs like <execinfo.h> (backtrace, backtrace_symbols_fd) and <unistd.h> (dup, STDERR_FILENO, SIGBUS) with
#ifndefQ_OS_WIN for Windows compatibility".Also applies to: 107-119, 3748-3772
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 14 - 15, The test file unconditionally includes <unistd.h> and uses POSIX APIs (pipe, read, write, close, dup, STDERR_FILENO, SIGBUS and related backtrace functions) which breaks Windows builds; wrap the platform-specific include and every code block that calls pipe/read/write/close (and any execinfo/backtrace usage) with `#ifndef` Q_OS_WIN ... `#endif` so the include and POSIX calls are excluded on Windows, and in the guarded sections either skip the test or provide a Windows-safe alternative/stub (use QSKIP or an `#else` stub) to preserve test semantics; specifically locate the <unistd.h> include and the functions named pipe, read, write, close, dup, backtrace, backtrace_symbols_fd and guard those regions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MCPServer_test.cpp`:
- Around line 3760-3762: The test currently uses ASSERT_GT(write(inputPipe[1],
payload.constData(), payload.size()), 0) which permits partial writes and can
cause onReadyRead() to receive truncated frames; change the assertion to verify
the full payload was written by capturing the return value of write(...) into a
variable (e.g., ssize_t written) and asserting written == payload.size() so the
test fails on partial writes and eliminates flaky truncated-frame behavior when
writing the QByteArray payload constructed with
makeTransport(QJsonDocument(request).toJson(...)).
- Around line 40-49: readTransportMessage currently does a single blocking read
which can hang tests; change it to set readFd to non-blocking, then use a
bounded poll/select loop with a short timeout to wait for readability and
perform repeated reads appending into response until no more data or the overall
timeout elapses, handling EAGAIN/EWOULDBLOCK between reads, and finally restore
the original file flags; apply the same non-blocking poll/read pattern to the
other helper usages mentioned (lines 126-130 / related helper functions) so
processAndRead() cannot stall indefinitely.
---
Duplicate comments:
In `@src/MCPServer_test.cpp`:
- Around line 3712-3715: In TEST_F(MCPServerProtocolTest,
HandleResourcesReadSceneInfoReturnsSerializedText) replace the hard assertion on
tryInitOgre() with a conditional skip: call tryInitOgre() and if it returns
false invoke GTEST_SKIP() (or equivalent test-skip macro) so the test is skipped
in headless CI; update the block surrounding tryInitOgre() /
createStandardOgreMaterials() to use the skip path rather than ASSERT_TRUE to
avoid failing the job when Ogre cannot initialize.
- Around line 14-15: The test file unconditionally includes <unistd.h> and uses
POSIX APIs (pipe, read, write, close, dup, STDERR_FILENO, SIGBUS and related
backtrace functions) which breaks Windows builds; wrap the platform-specific
include and every code block that calls pipe/read/write/close (and any
execinfo/backtrace usage) with `#ifndef` Q_OS_WIN ... `#endif` so the include and
POSIX calls are excluded on Windows, and in the guarded sections either skip the
test or provide a Windows-safe alternative/stub (use QSKIP or an `#else` stub) to
preserve test semantics; specifically locate the <unistd.h> include and the
functions named pipe, read, write, close, dup, backtrace, backtrace_symbols_fd
and guard those regions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| static QByteArray readTransportMessage(int readFd) | ||
| { | ||
| QByteArray response; | ||
| char buffer[4096]; | ||
| ssize_t bytesRead = read(readFd, buffer, sizeof(buffer)); | ||
| if (bytesRead > 0) { | ||
| response.append(buffer, bytesRead); | ||
| } | ||
| return response; | ||
| } |
There was a problem hiding this comment.
Avoid blocking read() in transport helper; it can hang the test run.
readTransportMessage() does a single blocking read(). If processMessage() does not emit a frame (or emits later), processAndRead() can stall indefinitely and make CI flaky. Use non-blocking fd + bounded poll/read loop with timeout.
💡 Suggested fix
static QByteArray readTransportMessage(int readFd)
{
QByteArray response;
char buffer[4096];
- ssize_t bytesRead = read(readFd, buffer, sizeof(buffer));
- if (bytesRead > 0) {
- response.append(buffer, bytesRead);
- }
+ QElapsedTimer timer;
+ timer.start();
+ while (timer.elapsed() < 1000) {
+ ssize_t bytesRead = read(readFd, buffer, sizeof(buffer));
+ if (bytesRead > 0) {
+ response.append(buffer, bytesRead);
+ break;
+ }
+ QCoreApplication::processEvents(QEventLoop::AllEvents, 10);
+ QThread::msleep(5);
+ }
return response;
}Also applies to: 126-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 40 - 49, readTransportMessage currently
does a single blocking read which can hang tests; change it to set readFd to
non-blocking, then use a bounded poll/select loop with a short timeout to wait
for readability and perform repeated reads appending into response until no more
data or the overall timeout elapses, handling EAGAIN/EWOULDBLOCK between reads,
and finally restore the original file flags; apply the same non-blocking
poll/read pattern to the other helper usages mentioned (lines 126-130 / related
helper functions) so processAndRead() cannot stall indefinitely.
| const QByteArray payload = QByteArray("garbage\r\n\r\n") + makeTransport(QJsonDocument(request).toJson(QJsonDocument::Compact)); | ||
| ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0); | ||
|
|
There was a problem hiding this comment.
Assert full pipe write to avoid truncated-frame flakes.
ASSERT_GT(write(...), 0) accepts partial writes; then onReadyRead() may parse a truncated payload intermittently. Assert written == payload.size().
💡 Suggested fix
- ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0);
+ const ssize_t written = write(inputPipe[1], payload.constData(), payload.size());
+ ASSERT_EQ(written, static_cast<ssize_t>(payload.size()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const QByteArray payload = QByteArray("garbage\r\n\r\n") + makeTransport(QJsonDocument(request).toJson(QJsonDocument::Compact)); | |
| ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0); | |
| const QByteArray payload = QByteArray("garbage\r\n\r\n") + makeTransport(QJsonDocument(request).toJson(QJsonDocument::Compact)); | |
| const ssize_t written = write(inputPipe[1], payload.constData(), payload.size()); | |
| ASSERT_EQ(written, static_cast<ssize_t>(payload.size())); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 3760 - 3762, The test currently uses
ASSERT_GT(write(inputPipe[1], payload.constData(), payload.size()), 0) which
permits partial writes and can cause onReadyRead() to receive truncated frames;
change the assertion to verify the full payload was written by capturing the
return value of write(...) into a variable (e.g., ssize_t written) and asserting
written == payload.size() so the test fails on partial writes and eliminates
flaky truncated-frame behavior when writing the QByteArray payload constructed
with makeTransport(QJsonDocument(request).toJson(...)).
|
1 participant
Summary
Validation
Summary by CodeRabbit