Conversation
Also adjust Claude NL suite licensing checks so it can run in the top-level repo.
Contributor
Reviewer's GuideUpdates the read_console tool to accept and validate the console Sequence diagram for claude-nl-suite Unity licensing checkssequenceDiagram
participant GitHub
participant Workflow as claude_nl_suite_workflow
participant JobCheck as job_check_secrets
participant JobRun as job_run_suite
participant SecretsStore as GitHub_Secrets
participant Unity as Unity_licensing_service
GitHub->>Workflow: Trigger workflow dispatch
Workflow->>JobCheck: Start check_secrets
JobCheck->>SecretsStore: Read ANTHROPIC_API_KEY
SecretsStore-->>JobCheck: Value or empty
JobCheck->>JobCheck: Set anthropic_ok flag
JobCheck->>SecretsStore: Read UNITY_LICENSE, UNITY_EMAIL, UNITY_PASSWORD, UNITY_SERIAL
SecretsStore-->>JobCheck: Values or empty
alt UNITY_LICENSE present OR (UNITY_EMAIL and UNITY_PASSWORD and UNITY_SERIAL present)
JobCheck->>JobCheck: unity_ok = true
else
JobCheck->>JobCheck: unity_ok = false
end
JobCheck-->>Workflow: Output anthropic_ok, unity_ok
Workflow->>JobRun: Start run_suite with outputs
JobRun->>SecretsStore: Read UNITY_LICENSE, UNITY_EMAIL, UNITY_PASSWORD, UNITY_SERIAL
SecretsStore-->>JobRun: Values or empty
JobRun->>JobRun: use_ulf = UNITY_LICENSE present
JobRun->>JobRun: use_ebl = UNITY_EMAIL and UNITY_PASSWORD and UNITY_SERIAL present
JobRun->>JobRun: has_serial = UNITY_SERIAL present
alt use_ulf is true
JobRun->>Unity: Activate with ULF license
else use_ebl is true
JobRun->>Unity: Activate with email/password/serial
else
JobRun->>JobRun: Skip Unity-dependent tests
end
Flow diagram for read_console types parsing and validationflowchart TD
A_start["read_console called"] --> B_check_types_nil{"types is None?"}
B_check_types_nil -- "Yes" --> B1_set_default["Set types = ['error','warning','log']"]
B1_set_default --> C_set_format_default["Set format default if needed"]
B_check_types_nil -- "No" --> D_is_str{"types is string?"}
D_is_str -- "Yes" --> D1_parse_json["types = parse_json_payload(types)"]
D1_parse_json --> E_validate_list
D_is_str -- "No" --> E_validate_list{"types is list?"}
E_validate_list -- "No" --> E1_error_not_list["Return error: types must be a list"]
E_validate_list -- "Yes" --> F_iterate["For each entry in types"]
F_iterate --> F1_check_str{"entry is string?"}
F1_check_str -- "No" --> F1_error_not_str["Return error: types entries must be strings"]
F1_check_str -- "Yes" --> F2_normalize["normalized = entry.strip().lower()"]
F2_normalize --> F3_allowed{"normalized in {error, warning, log, all}?"}
F3_allowed -- "No" --> F3_error_invalid["Return error: invalid types entry"]
F3_allowed -- "Yes" --> F4_append["Append normalized to normalized_types"]
F4_append --> F5_more{"More entries?"}
F5_more -- "Yes" --> F_iterate
F5_more -- "No" --> G_set_normalized["types = normalized_types"]
G_set_normalized --> C_set_format_default
C_set_format_default --> H_continue["Continue console read/clear logic"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR removes three GitHub Actions workflows, modifies license detection logic in two workflows requiring UNITY_SERIAL presence, and enhances the read_console service function with JSON string parsing, parameter validation, new paging/output parameters, and corresponding integration tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
✏️ Tip: You can disable this entire section by setting 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `Server/tests/integration/test_read_console_truncate.py:210-219` </location>
<code_context>
+ # Test with types as JSON string (the problematic case from issue #561)
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for JSON `types` strings that do not parse to a list to cover the new validation branch.
Right now we only test valid JSON list strings. Please also add tests where `types` is a JSON string that parses to a non-list value, for example:
- `types='"error"'` → expect `success is False`, the "types must be a list" message, and that `send_with_unity_instance` is not called.
- `types='{"type": "error"}'` → same expectations.
These will exercise the new error-handling branch in `read_console`.
</issue_to_address>
### Comment 2
<location> `Server/tests/integration/test_read_console_truncate.py:188-197` </location>
<code_context>
+ assert captured["params"]["types"] == ["error", "warning"]
+
+
+@pytest.mark.asyncio
+async def test_read_console_types_validation(monkeypatch):
+ """Test that read_console validates types entries and rejects invalid values."""
+ tools = setup_tools()
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the new "types must be a list" validation when a non-list, non-string is passed.
This new validation branch (when `types` is neither `None`, a list, nor a JSON string, e.g. `types=123` or `types=True`) isn’t covered yet. Please add a test that calls `read_console` with an invalid non-list value (like `types=123`) and asserts:
- `resp["success"] is False`
- the error message mentions that `types` must be a list / wrong type
- `send_with_unity_instance` is not called, consistent with the other negative tests.
Suggested implementation:
```python
@pytest.mark.asyncio
async def test_read_console_types_json_string(monkeypatch):
"""Test that read_console handles types parameter as JSON string (fixes issue #561)."""
tools = setup_tools()
read_console = tools["read_console"]
captured = {}
async def fake_send_with_unity_instance(_send_fn, _unity_instance, _command_type, params, **_kwargs):
captured["params"] = params
return {
"success": True,
"data": {"lines": [{"level": "error", "message": "test error"}]},
}
# Monkeypatch the function used by read_console to send the command to Unity.
# NOTE: adjust the target path here to match where send_with_unity_instance is imported/used.
monkeypatch.setattr(
"Server.tools.console_tools.send_with_unity_instance",
fake_send_with_unity_instance,
raising=True,
)
# Pass types as a JSON string and ensure it is parsed to a list internally.
resp = await read_console(types='["error", "warning"]')
assert resp["success"] is True
assert "data" in resp
assert captured["params"]["types"] == ["error", "warning"]
@pytest.mark.asyncio
async def test_read_console_types_validation_non_list(monkeypatch):
"""Test that read_console rejects invalid non-list, non-string types values."""
tools = setup_tools()
read_console = tools["read_console"]
calls = {"called": False}
async def fake_send_with_unity_instance(*_args, **_kwargs):
calls["called"] = True
return {
"success": True,
"data": {"lines": []},
}
# Monkeypatch the function used by read_console to ensure it is NOT called for invalid input.
# NOTE: adjust the target path here to match where send_with_unity_instance is imported/used.
monkeypatch.setattr(
"Server.tools.console_tools.send_with_unity_instance",
fake_send_with_unity_instance,
raising=True,
)
# Use an invalid non-list, non-string value for types (e.g. int) to trigger validation error.
resp = await read_console(types=123)
assert resp["success"] is False
# Error message should mention that `types` must be a list / has wrong type.
error_text = str(resp.get("error") or resp.get("message") or "")
assert "type" in error_text.lower() or "list" in error_text.lower()
assert "types" in error_text
# When validation fails, send_with_unity_instance must not be called.
assert calls["called"] is False
```
1. Replace `"Server.tools.console_tools.send_with_unity_instance"` in both `monkeypatch.setattr` calls with the actual import path used in `read_console` (e.g., if `read_console` does `from Server.tools.console_tools import send_with_unity_instance`, you should patch `"Server.tools.console_tools.send_with_unity_instance"`; if it re-exports it on a different object, patch that instead).
2. Ensure the way `read_console` is called (`await read_console(types=...)`) matches its real signature; if it expects a `params` dict or other arguments, adapt the calls accordingly while keeping the core idea: one call with a JSON string for the “happy path” test and one call with an invalid non-list value (e.g. `123`) for the new validation test.
3. If the error structure is different (for example, `resp` might be `{"success": False, "error": {"message": "..."}}`), update the `error_text` extraction logic so the assertions still check that the message mentions `types` and the requirement that it be a list / wrong type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Cleans up obsolete GitHub workflows, makes
unity-testsrunnable on forks when valid Unity secrets are present, and tightens Claude NL suite licensing to avoid upstream entitlement failures.Summary by Sourcery
Improve console tool robustness and simplify Unity-related CI workflows while tightening licensing requirements.
Bug Fixes:
Enhancements:
CI:
Tests:
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.