Add initial transport handshake tests with plan placeholders #15
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR establishes a comprehensive test framework for the Unity MCP bridge system by adding placeholder tests for critical functionality areas. The main implementation focuses on transport framing tests that verify proper handshake protocols and message framing for the Unity-to-MCP communication layer.
The transport framing implementation introduces a custom protocol that requires clients to send 'MCP/0.1 FRAMING=1' headers during handshake, followed by 8-byte big-endian length-prefixed JSON messages. This framing protocol is essential for reliable message boundaries in the streaming communication between Unity and external AI models.
Additionally, the PR adds placeholder test files for three other critical areas: resources API security (preventing directory traversal and unauthorized file access), script editing functionality (covering file operations, compilation integration, and security protections), and logging discipline (ensuring proper stderr-only logging to avoid corrupting the MCP protocol's stdout channel).
These placeholder tests follow a consistent pattern using @pytest.mark.skip decorators with detailed TODO descriptions that map out the expected testing scope. This systematic approach allows the team to establish comprehensive test coverage plans while focusing on the current protocol-framing implementation work.
Important Files Changed
Click to expand file changes
| Filename | Score | Overview |
|---|---|---|
| tests/test_transport_framing.py | 2/5 | Implements transport framing tests with handshake validation but imports potentially non-existent UnityConnection module |
| tests/test_resources_api.py | 5/5 | Adds security-focused placeholder tests for resource listing API with directory traversal protection |
| tests/test_script_editing.py | 5/5 | Comprehensive placeholder tests covering script editing workflows, security, and compilation integration |
| tests/test_logging_stdout.py | 5/5 | Placeholder tests ensuring proper logging discipline to prevent stdout contamination of MCP protocol |
Confidence score: 3/5
- This PR is moderately safe to merge but requires attention to the transport framing implementation
- Score reflects solid placeholder test structure but concerns about missing dependencies and import paths in the transport tests
- Pay close attention to tests/test_transport_framing.py which imports a potentially non-existent UnityConnection module
Sequence Diagram
sequenceDiagram
participant User
participant Test as "Test Framework"
participant DummyServer as "Dummy TCP Server"
participant UnityConnection as "Unity Connection"
participant Socket as "TCP Socket"
User->>Test: "Run transport framing tests"
Test->>DummyServer: "start_dummy_server(greeting, respond_ping)"
DummyServer->>Socket: "bind('127.0.0.1', 0)"
DummyServer->>Socket: "listen(1)"
DummyServer-->>Test: "return port number"
Test->>UnityConnection: "UnityConnection(host, port)"
Test->>UnityConnection: "connect()"
UnityConnection->>Socket: "connect to server"
DummyServer->>Socket: "accept connection"
DummyServer->>UnityConnection: "send greeting (MCP/0.1 FRAMING=1)"
UnityConnection->>UnityConnection: "parse greeting for FRAMING support"
alt Framing supported
UnityConnection-->>Test: "return True, set use_framing=True"
Test->>UnityConnection: "send framed ping message"
UnityConnection->>Socket: "struct.pack + JSON payload"
DummyServer->>Socket: "recv header (8 bytes)"
DummyServer->>Socket: "recv payload (JSON)"
DummyServer->>UnityConnection: "send framed pong response"
UnityConnection->>Test: "receive_full_response()"
Test->>Test: "assert pong received"
Test->>UnityConnection: "disconnect()"
else No framing support
UnityConnection-->>Test: "return False, sock=None"
Test->>Test: "assert connection failed"
end
4 files reviewed, 3 comments
tests/test_transport_framing.py
Outdated
|
|
||
| # add server src to path | ||
| ROOT = Path(__file__).resolve().parents[1] | ||
| SRC = ROOT / "UnityMcpBridge" / "UnityMcpServer~" / "src" |
There was a problem hiding this comment.
logic: Path 'UnityMcpBridge/UnityMcpServer~/src' doesn't exist in repository structure - this import will fail at runtime
| time.sleep(0.1) | ||
| try: | ||
| conn.close() | ||
| finally: | ||
| sock.close() |
There was a problem hiding this comment.
logic: Socket cleanup in finally block may fail if conn.close() raises exception, preventing sock.close()
| time.sleep(0.1) | |
| try: | |
| conn.close() | |
| finally: | |
| sock.close() | |
| time.sleep(0.1) | |
| try: | |
| conn.close() | |
| except Exception: | |
| pass | |
| finally: | |
| sock.close() |
| finally: | ||
| sock.close() | ||
|
|
||
| threading.Thread(target=_run, daemon=True).start() |
There was a problem hiding this comment.
logic: Race condition: test may proceed before server thread is ready to accept connections
|
@greptileai please evaluate the changes made and see if they improve based on earlier criticisms |
There was a problem hiding this comment.
Looking at these changes, I can see significant improvements in several key areas based on what would typically be critical feedback for a testing framework:
Positive Improvements
1. Comprehensive Test Planning
The PR shows excellent forward-thinking by stubbing out critical test areas:
- Transport layer security (framing, handshakes)
- Resource API boundaries and traversal protection
- Script editing edge cases and safety
- Logging discipline
2. Actual Working Tests
Unlike pure placeholder code, test_transport_framing.py includes two fully functional tests that validate core transport security:
test_handshake_requires_framing()- Ensures unframed connections are rejectedtest_small_frame_ping_pong()- Validates proper framing implementation
3. Security-First Approach
The test stubs show awareness of critical security concerns:
- Path traversal prevention
- File system boundary enforcement
- Symlink/junction protection
- Drive letter access restrictions
4. Practical Test Infrastructure
The start_dummy_server() helper is well-designed:
- Configurable greeting messages
- Optional ping/pong response handling
- Proper cleanup with threading
- Realistic TCP simulation
Areas That Address Common Criticisms
Error Handling: The working tests properly validate failure cases (connection rejection)
Resource Management: Proper socket cleanup in test helpers
Documentation: Clear TODO reasons explain the purpose of each planned test
Modularity: Tests are logically separated by functional area
Minor Suggestions
The path resolution in test_transport_framing.py could be more defensive:
- Consider adding fallback paths or clearer error messages
- The hardcoded server path structure assumes specific directory layout
Overall, this represents a mature approach to test-driven development - creating a comprehensive test plan while implementing the most critical transport security tests first. The balance between working code and documented intentions shows good engineering discipline.
|
@greptileai how are latest edits? |
There was a problem hiding this comment.
The latest edits look good! You've made solid progress implementing the transport framing tests and setting up a comprehensive testing structure. Here's my assessment:
What's Working Well
Transport Framing Tests: The test_transport_framing.py implementation is solid:
- Good dynamic path resolution for the server source
- Proper handshake validation testing
- Clean ping/pong framing verification
- Smart use of threading for test servers
Documentation Updates: The README additions clearly explain the framing protocol and resource URI scheme.
Test Organization: Well-structured placeholder tests that map out the critical testing areas you need to cover.
Minor Observations
Error Handling: The transport tests have good error handling, though the broad except Exception: blocks could be more specific in production code.
Test Coverage: The placeholder tests show you have a clear plan for comprehensive coverage of the critical areas - resources API, script editing, and logging discipline.
Code Quality Check: The test_no_print_statements_in_codebase() test is a nice touch for maintaining clean logging practices.
The approach of starting with working transport tests while stubbing out the rest with clear TODO reasons is pragmatic. It gives you a foundation to build on while keeping the broader testing goals visible.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@greptileai -- and the latest commit? better? any issues? |
There was a problem hiding this comment.
Looking at this latest commit, it's much better! You've added solid initial transport framing tests and structured placeholders for the critical areas. Here are my observations:
✅ Good improvements:
- The transport framing tests are well-implemented with proper handshake validation
- Dynamic server source location discovery is robust
- Good separation of concerns with different test files
- The ping/pong framing test validates the complete round-trip
- Proper cleanup in the dummy server implementations
- Error handling in
test_unframed_data_disconnect()- The broad exception handling could mask specific connection issues:
try:
data = sock.recv(1024)
assert data == b""
except ConnectionError:
pass # This is too broad-
Race condition potential in the handshake enforcing server - the 0.1 second select timeout might be flaky on slower systems.
-
Missing cleanup in
test_small_frame_ping_pong()- if the test fails beforeconn.disconnect(), the connection might leak.
🎯 Suggestions:
- Add a
finallyblock to ensure connection cleanup in the ping/pong test - Consider making the select timeout configurable or slightly longer (0.2s)
- The print statement detection regex is good, but you might want to also check for
sys.stdout.write()calls
The test structure looks solid and the placeholder approach with detailed TODO reasons is excellent for tracking what needs implementation. This gives a clear roadmap for completing the test coverage.
Overall, this is a significant improvement that properly sets up the testing foundation!
1 participant
Summary
Testing
pytest -qhttps://chatgpt.com/codex/tasks/task_e_68a1faa63e6083278ba8cfc888e721f4