Skip to content

Conversation

@dsarno
Copy link
Owner

@dsarno dsarno commented Jan 15, 2026

Summary\n- Parse ypes when provided as JSON string for

ead_console (fixes CoplayDev#561).\n- Validate entries and normalize to allowed values.\n- Add tests for JSON string and invalid entries.\n\n## Testing\n- uv run pytest tests/integration/test_read_console_truncate.py -v\n\n## Issue\n- Fixes CoplayDev#561

Summary by CodeRabbit

  • New Features

    • Accept types as either a list or a JSON string with automatic parsing.
    • Normalize types to lowercase and default to error, warning, and log when omitted.
    • Improved validation and explicit structured error responses for invalid types.
  • Tests

    • Added integration tests covering JSON-string parsing, normalization, validation, and error behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@cursor
Copy link

cursor bot commented Jan 15, 2026

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 7.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

read_console now accepts types as either a list or a JSON string, parses and validates it (normalizes to lowercase, restricts to error/warning/log/all), returns structured errors for invalid inputs, and adds tests covering JSON parsing and validation (tests appear duplicated).

Changes

Cohort / File(s) Summary
Service Enhancement
Server/src/services/tools/read_console.py
Accepts types as `list[...]
Integration Tests
Server/tests/integration/test_read_console_truncate.py
Added tests test_read_console_types_json_string and test_read_console_types_validation to verify JSON-string parsing, normalization, validation, and error paths. Note: the two new test blocks are duplicated in the file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibble JSON strings in the night,
Turn types to lists and tidy them right,
Lowercase whispers, rules held tight,
Errors caught with gentle fright,
Hopping on logs till morning light.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: parse and validate read_console types' clearly and concisely describes the primary changes in the pull request, which involve parsing types as JSON strings and validating entries in the read_console function.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b554546 and 9b3ba61.

📒 Files selected for processing (2)
  • Server/src/services/tools/read_console.py
  • Server/tests/integration/test_read_console_truncate.py
🔇 Additional comments (6)
Server/src/services/tools/read_console.py (3)

11-11: LGTM!

Import addition for parse_json_payload aligns with the new JSON string parsing functionality.


33-35: LGTM!

Type annotation update correctly documents the expanded input capability for the types parameter.


55-67: LGTM!

JSON string parsing with clear error messaging when the parsed result isn't a list. The error message helpfully indicates the expected format.

Server/tests/integration/test_read_console_truncate.py (3)

188-228: LGTM!

Comprehensive test coverage for the JSON string parsing feature:

  • JSON string input parsing
  • Case normalization to lowercase
  • Regular list input backward compatibility

231-262: LGTM!

Good validation tests covering:

  • Invalid type entries returning clear error messages
  • Non-string entries being rejected
  • Verification that no RPC call is made on validation failure

186-262: No duplicate test functions found.

Verification of the complete file confirms no test function definitions are duplicated. All six test functions appear exactly once.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Jan 15, 2026

Greptile Summary

This PR fixes issue CoplayDev#561 by adding JSON string parsing and validation for the types parameter in read_console. The implementation leverages the existing parse_json_payload utility to handle cases where MCP clients serialize the types array as a JSON string (e.g., '["error", "warning"]').

Key Changes:

  • Parse types parameter when provided as JSON string using parse_json_payload
  • Validate that types is a list after parsing, with clear error messages
  • Validate each entry is a string and matches allowed values (error, warning, log, all)
  • Normalize entries with .strip().lower() for case-insensitive matching
  • Add comprehensive test coverage for JSON string parsing and validation edge cases

Code Quality:

  • Follows existing patterns in the codebase (similar to how other tools use parse_json_payload)
  • Provides clear, actionable error messages for invalid inputs
  • Test coverage includes both the fix scenario and edge cases (invalid entries, non-string entries)
  • Early returns prevent invalid data from being sent to Unity

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - it's a focused fix with good test coverage
  • Score reflects well-tested implementation with proper error handling. Minor deduction for small style improvement opportunity (deduplication of types entries) that doesn't affect correctness
  • No files require special attention - both files have clear changes with appropriate test coverage

Important Files Changed

Filename Overview
Server/src/services/tools/read_console.py Added JSON string parsing and comprehensive validation for types parameter, with proper error handling and normalization
Server/tests/integration/test_read_console_truncate.py Added comprehensive test coverage for JSON string parsing and validation of types parameter

Sequence Diagram

sequenceDiagram
    participant Client as MCP Client
    participant RC as read_console
    participant PJP as parse_json_payload
    participant Unity as Unity Instance
    
    Client->>RC: call with types='["error", "warning"]'
    
    alt types is string
        RC->>PJP: parse_json_payload(types)
        PJP->>PJP: detect JSON array format
        PJP->>PJP: json.loads(value)
        PJP-->>RC: return parsed list
    end
    
    alt types is not a list after parsing
        RC-->>Client: return error (types must be a list)
    end
    
    RC->>RC: validate types list
    loop for each entry in types
        alt entry is not string
            RC-->>Client: return error (entries must be strings)
        end
        RC->>RC: normalize: entry.strip().lower()
        alt normalized not in allowed_types
            RC-->>Client: return error (invalid types entry)
        end
        RC->>RC: append to normalized_types
    end
    
    RC->>RC: build params_dict with validated types
    RC->>Unity: send_with_unity_instance(params_dict)
    Unity-->>RC: console data response
    RC-->>Client: return response
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

f"Allowed values: {sorted(allowed_types)}"
)
}
normalized_types.append(normalized)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: duplicate entries in types list aren't deduplicated (e.g. ["error", "error"] passes through as-is). Consider using list(dict.fromkeys(normalized_types)) to preserve order while deduplicating.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/services/tools/read_console.py
Line: 85:85

Comment:
**style:** duplicate entries in `types` list aren't deduplicated (e.g. `["error", "error"]` passes through as-is). Consider using `list(dict.fromkeys(normalized_types))` to preserve order while deduplicating.

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

@dsarno dsarno force-pushed the fix/read-console-types branch from b554546 to 9b3ba61 Compare January 15, 2026 16:55
@dsarno
Copy link
Owner Author

dsarno commented Jan 15, 2026

Update: addressed review feedback

  • Updated Types type hint to document JSON string support.
  • Added normalization test for mixed-case types input.
  • Renamed unused mock args to _-prefixed names to avoid lint noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error validating tool 'read_console': 1 validation error for call[read_console]

2 participants